* [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script
@ 2016-12-02 10:48 Laszlo Ersek
2017-01-03 11:41 ` Laszlo Ersek
2017-01-04 1:30 ` Jordan Justen
0 siblings, 2 replies; 10+ messages in thread
From: Laszlo Ersek @ 2016-12-02 10:48 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
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
);
--
2.9.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script
2016-12-02 10:48 [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script Laszlo Ersek
@ 2017-01-03 11:41 ` Laszlo Ersek
2017-01-04 1:30 ` Jordan Justen
1 sibling, 0 replies; 10+ messages in thread
From: Laszlo Ersek @ 2017-01-03 11:41 UTC (permalink / raw)
To: Jordan Justen, Ni, Ruiyu, Jiewen Yao; +Cc: edk2-devel-01
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
> );
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script
2016-12-02 10:48 [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script Laszlo Ersek
2017-01-03 11:41 ` Laszlo Ersek
@ 2017-01-04 1:30 ` Jordan Justen
2017-01-04 11:19 ` Laszlo Ersek
1 sibling, 1 reply; 10+ messages in thread
From: Jordan Justen @ 2017-01-04 1:30 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script
2017-01-04 1:30 ` Jordan Justen
@ 2017-01-04 11:19 ` Laszlo Ersek
2017-01-04 22:01 ` Jordan Justen
0 siblings, 1 reply; 10+ messages in thread
From: Laszlo Ersek @ 2017-01-04 11:19 UTC (permalink / raw)
To: Jordan Justen, edk2-devel-01
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
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script
2017-01-04 11:19 ` Laszlo Ersek
@ 2017-01-04 22:01 ` Jordan Justen
2017-01-05 1:03 ` Yao, Jiewen
0 siblings, 1 reply; 10+ messages in thread
From: Jordan Justen @ 2017-01-04 22:01 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script
2017-01-04 22:01 ` Jordan Justen
@ 2017-01-05 1:03 ` Yao, Jiewen
2017-01-05 1:45 ` Yao, Jiewen
0 siblings, 1 reply; 10+ messages in thread
From: Yao, Jiewen @ 2017-01-05 1:03 UTC (permalink / raw)
To: Justen, Jordan L, Laszlo Ersek, edk2-devel-01
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script
2017-01-05 1:03 ` Yao, Jiewen
@ 2017-01-05 1:45 ` Yao, Jiewen
2017-01-05 11:47 ` Laszlo Ersek
0 siblings, 1 reply; 10+ messages in thread
From: Yao, Jiewen @ 2017-01-05 1:45 UTC (permalink / raw)
To: Yao, Jiewen, Justen, Jordan L, Laszlo Ersek, edk2-devel-01
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script
2017-01-05 1:45 ` Yao, Jiewen
@ 2017-01-05 11:47 ` Laszlo Ersek
2017-01-05 13:09 ` Yao, Jiewen
0 siblings, 1 reply; 10+ messages in thread
From: Laszlo Ersek @ 2017-01-05 11:47 UTC (permalink / raw)
To: Yao, Jiewen, Justen, Jordan L, edk2-devel-01
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script
2017-01-05 11:47 ` Laszlo Ersek
@ 2017-01-05 13:09 ` Yao, Jiewen
2017-01-05 13:15 ` Laszlo Ersek
0 siblings, 1 reply; 10+ messages in thread
From: Yao, Jiewen @ 2017-01-05 13:09 UTC (permalink / raw)
To: Laszlo Ersek, Justen, Jordan L, edk2-devel-01
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script
2017-01-05 13:09 ` Yao, Jiewen
@ 2017-01-05 13:15 ` Laszlo Ersek
0 siblings, 0 replies; 10+ messages in thread
From: Laszlo Ersek @ 2017-01-05 13:15 UTC (permalink / raw)
To: Yao, Jiewen, Justen, Jordan L, edk2-devel-01
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
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-01-05 13:15 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-02 10:48 [PATCH] OvmfPkg/SmmControl2Dxe: correct PCI_CONFIG_READ_WRITE in S3 boot script Laszlo Ersek
2017-01-03 11:41 ` Laszlo Ersek
2017-01-04 1:30 ` Jordan Justen
2017-01-04 11:19 ` Laszlo Ersek
2017-01-04 22:01 ` Jordan Justen
2017-01-05 1:03 ` Yao, Jiewen
2017-01-05 1:45 ` Yao, Jiewen
2017-01-05 11:47 ` Laszlo Ersek
2017-01-05 13:09 ` Yao, Jiewen
2017-01-05 13:15 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox