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

Message ID 20161202104844.6093-1-lersek@redhat.com
State New
Headers show

Commit Message

Laszlo Ersek Dec. 2, 2016, 10:48 a.m.
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(-)

-- 
2.9.2

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

Comments

Laszlo Ersek Jan. 3, 2017, 11:41 a.m. | #1
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

>                            );

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Jordan Justen Jan. 4, 2017, 1:30 a.m. | #2
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.

-Jordan

> +                            ),

>                            &GenPmCon1OrMask,

>                            &GenPmCon1AndMask

>                            );

> -- 

> 2.9.2

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Jan. 4, 2017, 11:19 a.m. | #3
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

>>


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Jordan Justen Jan. 4, 2017, 10:01 p.m. | #4
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
https://lists.01.org/mailman/listinfo/edk2-devel
Yao, Jiewen Jan. 5, 2017, 1:03 a.m. | #5
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-bounces@lists.01.org] On Behalf Of Jordan Justen

Sent: Thursday, January 5, 2017 6:02 AM
To: Laszlo Ersek <lersek@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
Yao, Jiewen Jan. 5, 2017, 1:45 a.m. | #6
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-bounces@lists.01.org] On Behalf Of Yao, Jiewen

Sent: Thursday, January 5, 2017 9:03 AM
To: Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@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-bounces@lists.01.org] On Behalf Of Jordan Justen

Sent: Thursday, January 5, 2017 6:02 AM
To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@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
Laszlo Ersek Jan. 5, 2017, 11:47 a.m. | #7
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-bounces@lists.01.org] *On Behalf Of

> *Yao, Jiewen

> *Sent:* Thursday, January 5, 2017 9:03 AM

> *To:* Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek

> <lersek@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-bounces@lists.01.org] On Behalf Of Jordan Justen

> Sent: Thursday, January 5, 2017 6:02 AM

> To: Laszlo Ersek <lersek@redhat.com

> <mailto:lersek@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
Yao, Jiewen Jan. 5, 2017, 1:09 p.m. | #8
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:lersek@redhat.com]

Sent: Thursday, January 5, 2017 7:48 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel-01 <edk2-devel@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-bounces@lists.01.org] *On Behalf Of

> *Yao, Jiewen

> *Sent:* Thursday, January 5, 2017 9:03 AM

> *To:* Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Laszlo Ersek

> <lersek@redhat.com<mailto:lersek@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-bounces@lists.01.org] On Behalf Of Jordan Justen

> Sent: Thursday, January 5, 2017 6:02 AM

> To: Laszlo Ersek <lersek@redhat.com

<mailto:lersek@redhat.com%0b>> <mailto:lersek@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
Laszlo Ersek Jan. 5, 2017, 1:15 p.m. | #9
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:lersek@redhat.com]

> *Sent:* Thursday, January 5, 2017 7:48 PM

> *To:* Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan L

> <jordan.l.justen@intel.com>; edk2-devel-01 <edk2-devel@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-bounces@lists.01.org] *On Behalf Of

>> *Yao, Jiewen

>> *Sent:* Thursday, January 5, 2017 9:03 AM

>> *To:* Justen, Jordan L <jordan.l.justen@intel.com <mailto:jordan.l.justen@intel.com>>; Laszlo Ersek

>> <lersek@redhat.com

> <mailto:lersek@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-bounces@lists.01.org] On Behalf Of Jordan Justen

>> Sent: Thursday, January 5, 2017 6:02 AM

>> To: Laszlo Ersek <lersek@redhat.com

> <mailto:lersek@redhat.com%0b>> <mailto:lersek@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

Patch

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
                           );