[edk2-devel] [PATCH 1/1] OvmfPkg/XenPvBlkDxe: Fix memory barrier macro

[Andrew Cooper via groups.io]

On 19/07/2022 14:52, Anthony Perard wrote:
> diff --git a/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.h b/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.h
> index 350b7bd309c0..67ee1899e9a8 100644
> --- a/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.h
> +++ b/OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.h
> @@ -11,8 +11,9 @@
>  #define __EFI_XEN_PV_BLK_DXE_H__
>  
>  #include <Uefi.h>
> +#include "FullMemoryFence.h"
>  
> -#define xen_mb()   MemoryFence()
> +#define xen_mb()   FullMemoryFence()
>  #define xen_rmb()  MemoryFence()
>  #define xen_wmb()  MemoryFence()

Ok, so the old MemoryFence() is definitely bogus here.

However, it doesn't need to be an mfence instruction.  All that is
needed is smp_mb(), which these days is

asm volatile ( "lock addl $0, -4(%%rsp)" ::: "memory" )

because that has the required read/write ordering properties without the
extra serialising property that mfence has.

Furthermore, ...

>  
> diff --git a/OvmfPkg/XenPvBlkDxe/X86GccFullMemoryFence.c b/OvmfPkg/XenPvBlkDxe/X86GccFullMemoryFence.c
> new file mode 100644
> index 000000000000..92d107def470
> --- /dev/null
> +++ b/OvmfPkg/XenPvBlkDxe/X86GccFullMemoryFence.c
> @@ -0,0 +1,20 @@
> +/** @file
> +  Copyright (C) 2022, Citrix Ltd.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include "FullMemoryFence.h"
> +
> +//
> +// Like MemoryFence() but prevent stores from been reorded with loads by
> +// the CPU on X64.
> +//
> +VOID
> +EFIAPI
> +FullMemoryFence (
> +  VOID
> +  )
> +{
> +  __asm__ __volatile__ ("mfence":::"memory");
> +}

... stuff like this needs to come from a single core location, and not
opencoded for each driver.

~Andrew


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91668): https://edk2.groups.io/g/devel/message/91668
Mute This Topic: https://groups.io/mt/92482724/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-