Re: [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script

[Laszlo Ersek <lersek@redhat.com>]

Jordan, Ray, Jiewen,

can one of you please review this patch?

The change is not specific to OVMF / QEMU behavior, it's about the
correct encoding of PCI config space addresses, for the related S3 boot
script operations.

The POWER_MGMT_REGISTER_Q35() macro seen below is

#define POWER_MGMT_REGISTER_Q35(Offset) \
  PCI_LIB_ADDRESS (0, 0x1f, 0, (Offset))

from "OvmfPkg/Include/IndustryStandard/Q35MchIch9.h".

Thanks!
Laszlo

On 12/02/16 11:48, Laszlo Ersek wrote:
> EFI_BOOT_SCRIPT_PCI_CONFIG_READ_WRITE_OPCODE expects the PCI address to
> access in UEFI encoding, not in edk2/PciLib encoding. Convert the
> ICH9_GEN_PMCON_1 register's address to UEFI representation before storing
> it in the boot script.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
> index c5e5ed02f5ad..3694282c82ad 100644
> --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
> +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
> @@ -33,6 +33,7 @@
>  #include <Library/PciLib.h>
>  #include <Library/QemuFwCfgLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
> +#include <Protocol/PciRootBridgeIo.h>
>  #include <Protocol/S3SaveState.h>
>  #include <Protocol/SmmControl2.h>
>  
> @@ -307,6 +308,33 @@ FatalError:
>  }
>  
>  /**
> +  Convert a PCI address originally composed with PCI_LIB_ADDRESS() to
> +  EFI_PCI_ADDRESS() representation (see Table 111. "PCI Configuration Address"
> +  in UEFI-2.6).
> +
> +  @param[in] PciLibAddress  A PCI address originally composed with
> +                            PCI_LIB_ADDRESS().
> +
> +  @return  The converted address suitable for consumers that expect
> +           EFI_PCI_ADDRESS() representation.
> +**/
> +STATIC
> +UINT64
> +ConvertPciLibToEfiPciAddress (
> +  IN UINT32 PciLibAddress
> +  )
> +{
> +  UINT32 Bus, Device, Function, Register;
> +
> +  Register = BitFieldRead32 (PciLibAddress,  0, 11);
> +  Function = BitFieldRead32 (PciLibAddress, 12, 14);
> +  Device   = BitFieldRead32 (PciLibAddress, 15, 19);
> +  Bus      = BitFieldRead32 (PciLibAddress, 20, 27);
> +
> +  return EFI_PCI_ADDRESS (Bus, Device, Function, Register);
> +}
> +
> +/**
>    Notification callback for S3SaveState installation.
>  
>    @param[in] Event    Event whose notification function is being invoked.
> @@ -362,7 +390,9 @@ OnS3SaveStateInstalled (
>                            S3SaveState,
>                            EFI_BOOT_SCRIPT_PCI_CONFIG_READ_WRITE_OPCODE,
>                            EfiBootScriptWidthUint16,
> -                          (UINT64)POWER_MGMT_REGISTER_Q35 (ICH9_GEN_PMCON_1),
> +                          ConvertPciLibToEfiPciAddress (
> +                            POWER_MGMT_REGISTER_Q35 (ICH9_GEN_PMCON_1)
> +                            ),
>                            &GenPmCon1OrMask,
>                            &GenPmCon1AndMask
>                            );
>