| Message ID | 20161202104844.6093-1-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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
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
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
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 );
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