Re: [PATCH 1/1] OvmfPkg/XenPvBlkDxe: Fix memory barrier macro

[Andrew Cooper <Andrew.Cooper3@citrix.com>]

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