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

[Laszlo Ersek <lersek@redhat.com>]

On 01/04/17 02:30, Jordan Justen wrote:
> On 2016-12-02 02:48:44, 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)
> 
> I think we should just add a EFI_POWER_MGMT_REGISTER_Q35 macro.

I thought of that, but I didn't want to use the EFI_ prefix for a macro
that has nothing to do with the UEFI / PI specs. Can you suggest an
alternative name? Perhaps POWER_MGMT_REGISTER_Q35_EFI?

Thanks!
Laszlo

> 
> -Jordan
> 
>> +                            ),
>>                            &GenPmCon1OrMask,
>>                            &GenPmCon1AndMask
>>                            );
>> -- 
>> 2.9.2
>>