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

2017-01-05 Thread Laszlo Ersek 
On 01/05/17 14:09, Yao, Jiewen wrote:
> Yes, I also agree to add a new MACRO.
> 
>  
> 
> My only concern is the name - POWER_MGMT_REGISTER_Q35_EFI is too weird.
> 
> I have no idea on the meaning to use _EFI as suffix.
> 
>  
> 
> Since we already defined below in PciRootBridgeIo.h,
> 
> #defineEFI_PCI_ADDRESS(bus, dev, func, reg) \
> 
>   (UINT64) ( \
> 
>   (((UINTN) bus) << 24) | \
> 
>   (((UINTN) dev) << 16) | \
> 
>   (((UINTN) func) << 8) | \
> 
>   (((UINTN) (reg)) < 256 ? ((UINTN) (reg)) : (UINT64) (LShiftU64
> ((UINT64) (reg), 32
> 
>  
> 
>  
> 
> How about we use POWER_MGMT_REGISTER_Q35_EFI_PCI_ADDRESS, or
> EFI_PCI_ADDRESS_POWER_MGMT_REGISTER_Q35 ?

POWER_MGMT_REGISTER_Q35_EFI_PCI_ADDRESS() looks fine to me, if Jordan
agrees.

Thanks!
Laszlo

> 
>  
> 
> Thank you
> 
> Yao Jiewen
> 
>  
> 
>  
> 
>  
> 
> *From:*Laszlo Ersek [mailto:ler...@redhat.com]
> *Sent:* Thursday, January 5, 2017 7:48 PM
> *To:* Yao, Jiewen <jiewen....@intel.com>; Justen, Jordan L
> <jordan.l.jus...@intel.com>; edk2-devel-01 <edk2-de...@ml01.01.org>
> *Subject:* Re: [edk2] [PATCH] OvmfPkg/SmmControl2Dxe: correct
> PCI_CONFIG_READ_WRITE in S3 boot script
> 
>  
> 
> On 01/05/17 02:45, Yao, Jiewen wrote:
>> Sorry, fix typo: I agree we do **not** use EFI_ prefix here.
>> 
>> But using _EFI as suffix is also odd. :)
>> 
>> Do we have better name?
>> 
>> Such as POWER_MGMT_REGISTER_Q35_ADDRESS ?
> 
> POWER_MGMT_REGISTER_Q35() already returns a flat address, just in PciLib
> encoding. So an _ADDRESS suffix for the variant with the UEFI spec
> encoding is not particularly telling.
> 
> The new macro name should reflect the PciLib encoding <-> UEFI spec
> encoding difference.
> 
> Thanks!
> Laszlo
> 
> 
>> 
>>  
>> 
>> Thank you
>> 
>> Yao Jiewen
>> 
>>  
>> 
>> *From:*edk2-devel [mailto:edk2-devel-boun...@lists.01.org] *On Behalf Of
>> *Yao, Jiewen
>> *Sent:* Thursday, January 5, 2017 9:03 AM
>> *To:* Justen, Jordan L <jordan.l.jus...@intel.com 
>> <mailto:jordan.l.jus...@intel.com>>; Laszlo Ersek
>> <ler...@redhat.com
> <mailto:ler...@redhat.com>>; edk2-devel-01 <edk2-devel@lists.01.org
> <mailto:edk2-devel@lists.01.org>>
>> *Subject:* Re: [edk2] [PATCH] OvmfPkg/SmmControl2Dxe: correct
>> PCI_CONFIG_READ_WRITE in S3 boot script
>> 
>>  
>> 
>> Hi
>> I agree we do use EFI_ prefix here.
>> 
>> But using _EFI as suffix is also odd. :)
>> 
>> Do we have better name?
>> 
>> Thank you
>> Yao Jiewen
>> 
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
>> Jordan Justen
>> Sent: Thursday, January 5, 2017 6:02 AM
>> To: Laszlo Ersek <ler...@redhat.com
> <mailto:ler...@redhat.com%0b>> <mailto:ler...@redhat.com>>; edk2-devel-01 
> <edk2-devel@lists.01.org
> <mailto:edk2-devel@lists.01.org%0b>> <mailto:edk2-devel@lists.01.org>>
>> Subject: Re: [edk2] [PATCH] OvmfPkg/SmmControl2Dxe: correct 
>> PCI_CONFIG_READ_WRITE in S3 boot script
>> 
>> On 2017-01-04 03:19:20, Laszlo Ersek wrote:
>>> On 01/04/17 02:30, Jordan Justen wrote:
>>> > On 2016-12-02 02:48:44, Laszlo Ersek wrote:
>>> >> @@ -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?
>> 
>> Good point. Yeah, that name seems fine to me.
>> 
>> -Jordan
>> ___
>> edk2-devel mailing list
>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org
> <mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>
>> <mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
>> https://lists.01.org/mailman/listinfo/edk2-devel
>> ___
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
> <mailto:edk2-devel@lists.01.org> <mailto:edk2-devel@lists.01.org>
>> https://lists.01.org/mailman/listinfo/edk2-devel
>> 
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2017-01-05 Thread Yao, Jiewen 
Yes, I also agree to add a new MACRO.

My only concern is the name - POWER_MGMT_REGISTER_Q35_EFI is too weird.
I have no idea on the meaning to use _EFI as suffix.

Since we already defined below in PciRootBridgeIo.h,
#define EFI_PCI_ADDRESS(bus, dev, func, reg) \
  (UINT64) ( \
  (((UINTN) bus) << 24) | \
  (((UINTN) dev) << 16) | \
  (((UINTN) func) << 8) | \
  (((UINTN) (reg)) < 256 ? ((UINTN) (reg)) : (UINT64) (LShiftU64 ((UINT64) 
(reg), 32


How about we use POWER_MGMT_REGISTER_Q35_EFI_PCI_ADDRESS, or 
EFI_PCI_ADDRESS_POWER_MGMT_REGISTER_Q35 ?

Thank you
Yao Jiewen



From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Thursday, January 5, 2017 7:48 PM
To: Yao, Jiewen <jiewen@intel.com>; Justen, Jordan L 
<jordan.l.jus...@intel.com>; edk2-devel-01 <edk2-de...@ml01.01.org>
Subject: Re: [edk2] [PATCH] OvmfPkg/SmmControl2Dxe: correct 
PCI_CONFIG_READ_WRITE in S3 boot script

On 01/05/17 02:45, Yao, Jiewen wrote:
> Sorry, fix typo: I agree we do **not** use EFI_ prefix here.
>
> But using _EFI as suffix is also odd. :)
>
> Do we have better name?
>
> Such as POWER_MGMT_REGISTER_Q35_ADDRESS ?

POWER_MGMT_REGISTER_Q35() already returns a flat address, just in PciLib
encoding. So an _ADDRESS suffix for the variant with the UEFI spec
encoding is not particularly telling.

The new macro name should reflect the PciLib encoding <-> UEFI spec
encoding difference.

Thanks!
Laszlo


>
>
>
> Thank you
>
> Yao Jiewen
>
>
>
> *From:*edk2-devel [mailto:edk2-devel-boun...@lists.01.org] *On Behalf Of
> *Yao, Jiewen
> *Sent:* Thursday, January 5, 2017 9:03 AM
> *To:* Justen, Jordan L 
> <jordan.l.jus...@intel.com<mailto:jordan.l.jus...@intel.com>>; Laszlo Ersek
> <ler...@redhat.com<mailto:ler...@redhat.com>>; edk2-devel-01 
> <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>
> *Subject:* Re: [edk2] [PATCH] OvmfPkg/SmmControl2Dxe: correct
> PCI_CONFIG_READ_WRITE in S3 boot script
>
>
>
> Hi
> I agree we do use EFI_ prefix here.
>
> But using _EFI as suffix is also odd. :)
>
> Do we have better name?
>
> Thank you
> Yao Jiewen
>
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jordan 
> Justen
> Sent: Thursday, January 5, 2017 6:02 AM
> To: Laszlo Ersek <ler...@redhat.com
<mailto:ler...@redhat.com%0b>> <mailto:ler...@redhat.com>>; edk2-devel-01 
<edk2-devel@lists.01.org
<mailto:edk2-devel@lists.01.org%0b>> <mailto:edk2-devel@lists.01.org>>
> Subject: Re: [edk2] [PATCH] OvmfPkg/SmmControl2Dxe: correct 
> PCI_CONFIG_READ_WRITE in S3 boot script
>
> On 2017-01-04 03:19:20, Laszlo Ersek wrote:
>> On 01/04/17 02:30, Jordan Justen wrote:
>> > On 2016-12-02 02:48:44, Laszlo Ersek wrote:
>> >> @@ -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?
>
> Good point. Yeah, that name seems fine to me.
>
> -Jordan
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>
> <mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> https://lists.01.org/mailman/listinfo/edk2-devel
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 
> <mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2017-01-05 Thread Laszlo Ersek 
On 01/05/17 02:45, Yao, Jiewen wrote:
> Sorry, fix typo: I agree we do **not** use EFI_ prefix here.
> 
> But using _EFI as suffix is also odd. :)
> 
> Do we have better name?
> 
> Such as POWER_MGMT_REGISTER_Q35_ADDRESS ?

POWER_MGMT_REGISTER_Q35() already returns a flat address, just in PciLib
encoding. So an _ADDRESS suffix for the variant with the UEFI spec
encoding is not particularly telling.

The new macro name should reflect the PciLib encoding <-> UEFI spec
encoding difference.

Thanks!
Laszlo


> 
>  
> 
> Thank you
> 
> Yao Jiewen
> 
>  
> 
> *From:*edk2-devel [mailto:edk2-devel-boun...@lists.01.org] *On Behalf Of
> *Yao, Jiewen
> *Sent:* Thursday, January 5, 2017 9:03 AM
> *To:* Justen, Jordan L <jordan.l.jus...@intel.com>; Laszlo Ersek
> <ler...@redhat.com>; edk2-devel-01 <edk2-devel@lists.01.org>
> *Subject:* Re: [edk2] [PATCH] OvmfPkg/SmmControl2Dxe: correct
> PCI_CONFIG_READ_WRITE in S3 boot script
> 
>  
> 
> Hi
> I agree we do use EFI_ prefix here.
> 
> But using _EFI as suffix is also odd. :)
> 
> Do we have better name?
> 
> Thank you
> Yao Jiewen
> 
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jordan 
> Justen
> Sent: Thursday, January 5, 2017 6:02 AM
> To: Laszlo Ersek <ler...@redhat.com
> <mailto:ler...@redhat.com>>; edk2-devel-01 <edk2-devel@lists.01.org
> <mailto:edk2-devel@lists.01.org>>
> Subject: Re: [edk2] [PATCH] OvmfPkg/SmmControl2Dxe: correct 
> PCI_CONFIG_READ_WRITE in S3 boot script
> 
> On 2017-01-04 03:19:20, Laszlo Ersek wrote:
>> On 01/04/17 02:30, Jordan Justen wrote:
>> > On 2016-12-02 02:48:44, Laszlo Ersek wrote:
>> >> @@ -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?
> 
> Good point. Yeah, that name seems fine to me.
> 
> -Jordan
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org
> <mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> https://lists.01.org/mailman/listinfo/edk2-devel
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2017-01-04 Thread Yao, Jiewen 
Sorry, fix typo: I agree we do *not* use EFI_ prefix here.

But using _EFI as suffix is also odd. :)

Do we have better name?

Such as POWER_MGMT_REGISTER_Q35_ADDRESS ?

Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Yao, 
Jiewen
Sent: Thursday, January 5, 2017 9:03 AM
To: Justen, Jordan L <jordan.l.jus...@intel.com>; Laszlo Ersek 
<ler...@redhat.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Subject: Re: [edk2] [PATCH] OvmfPkg/SmmControl2Dxe: correct 
PCI_CONFIG_READ_WRITE in S3 boot script

Hi
I agree we do use EFI_ prefix here.

But using _EFI as suffix is also odd. :)

Do we have better name?

Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jordan 
Justen
Sent: Thursday, January 5, 2017 6:02 AM
To: Laszlo Ersek <ler...@redhat.com<mailto:ler...@redhat.com>>; edk2-devel-01 
<edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>
Subject: Re: [edk2] [PATCH] OvmfPkg/SmmControl2Dxe: correct 
PCI_CONFIG_READ_WRITE in S3 boot script

On 2017-01-04 03:19:20, Laszlo Ersek wrote:
> On 01/04/17 02:30, Jordan Justen wrote:
> > On 2016-12-02 02:48:44, Laszlo Ersek wrote:
> >> @@ -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?

Good point. Yeah, that name seems fine to me.

-Jordan
___
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2017-01-04 Thread Yao, Jiewen 
Hi
I agree we do use EFI_ prefix here.

But using _EFI as suffix is also odd. :)

Do we have better name?

Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jordan 
Justen
Sent: Thursday, January 5, 2017 6:02 AM
To: Laszlo Ersek <ler...@redhat.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Subject: Re: [edk2] [PATCH] OvmfPkg/SmmControl2Dxe: correct 
PCI_CONFIG_READ_WRITE in S3 boot script

On 2017-01-04 03:19:20, Laszlo Ersek wrote:
> On 01/04/17 02:30, Jordan Justen wrote:
> > On 2016-12-02 02:48:44, Laszlo Ersek wrote:
> >> @@ -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?

Good point. Yeah, that name seems fine to me.

-Jordan
___
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2017-01-04 Thread Laszlo Ersek 
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 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek 
>> ---
>>  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 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>  
>> @@ -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] EventEvent 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
> 
>> +),
>>,
>>
>>);
>> -- 
>> 2.9.2
>>

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2017-01-03 Thread Jordan Justen 
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 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -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] EventEvent 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.

-Jordan

> +),
>,
>
>);
> -- 
> 2.9.2
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2017-01-03 Thread Laszlo Ersek 
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 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -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] EventEvent 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)
> +),
>,
>
>);
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel