Re: [PATCH v2] OvmfPkg: Update I/O port related to ACPI devices for CloudHv

["Yao, Jiewen" <jiewen.yao@intel.com>]

I see. The is hard to let a base lib access the HOB.

I think we can integrate this patch at first to make it work, with known limitation.


> -----Original Message-----
> From: Boeuf, Sebastien <sebastien.boeuf@intel.com>
> Sent: Wednesday, September 7, 2022 11:21 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: kraxel@redhat.com; Justen, Jordan L <jordan.l.justen@intel.com>;
> devel@edk2.groups.io
> Subject: Re: [PATCH v2] OvmfPkg: Update I/O port related to ACPI devices
> for CloudHv
> 
> I had a quick try and I've realized
> OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.c didn't have access
> to the EFI_HOB_PLATFORM_INFO. Is there an alternative?
> 
> Thanks,
> Sebastien
> 
> On Wed, 2022-09-07 at 16:23 +0200, Sebastien Boeuf wrote:
> > Hi Jiewen,
> >
> > After I looked into the UefiPayload example, I have a few questions
> > on
> > how to implement things in OvmfPkg:
> >
> > - Do you expect EFI_HOB_PLATFORM_INFO to be extended with two
> > additional fields AcpiTimerAddress and AcpiShutdownAddress? Or do you
> > think the ACPI_BOARD_INFO should be copied over from the the
> > UefiPayload package?
> >
> > - Is InitializePlatform() from OvmfPkg/PlatformPei/Platform.c the
> > correct place where the FADT parsing should happen? I would need the
> > platform info HOB to be accessible from
> > OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c,
> > OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.c,
> > OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c,
> > OvmfPkg/Library/ResetSystemLib/DxeResetShutdown.c, and
> > OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c.
> >
> > Thanks,
> > Sebastien
> >
> > On Tue, 2022-09-06 at 15:57 +0000, Yao, Jiewen wrote:
> > > One good example is in UefiPayloadPkg.
> > >
> > > 1. At entrypoint, the UefiPayload parses the ACPI table and build
> > > gUefiAcpiBoardInfoGuid.https://github.com/tianocore/edk2/blob/mas
> > > ter/UefiPayloadPkg/UefiPayloadEntry/AcpiTable.c#L23
> > >
> > > 1. Later, AcpiTimer driver uses the ACPI data in
> > > gUefiAcpiBoardInfoGuid.https://github.com/tianocore/edk2/blob/mas
> > > ter/UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.c#L49
> > >
> > > 1. Other driver may also use the ACPI data, such as
> > > PciExpressAddress. (from
> > > MCFG).https://github.com/tianocore/edk2/blob/master/UefiPayloadPk
> > > g/Library/PciSegmentInfoLibAcpiBoardInfo/PciSegmentInfoLibAcpiBoa
> > > rdInfo.c#L55
> > >
> > >
> > > Thank you
> > > Yao Jiewen
> > >
> > >
> > > From: Boeuf, Sebastien <sebastien.boeuf@intel.com>
> > > Sent: Tuesday, September 6, 2022 11:41 PM
> > > To: Yao, Jiewen <jiewen.yao@intel.com>
> > > Cc: Justen, Jordan L <jordan.l.justen@intel.com>;
> > > kraxel@redhat.com;
> > > devel@edk2.groups.io
> > > Subject: Re: [PATCH v2] OvmfPkg: Update I/O port related to ACPI
> > > devices for CloudHv
> > >
> > > Hi Jiewen,
> > >
> > > We patched Cloud Hypervisor to support both I/O ports for at least
> > > two versions. And of course at some point users will have to rely
> > > on
> > > latest CloudHv binary (which we always build).
> > >
> > > One improvement could be to retrieve the I/O ports addresses from
> > > the
> > > FADT table. Do you know if there's some code already doing that in
> > > OVMF?
> > >
> > > Thanks,
> > > Sebastien
> > > From: Yao, Jiewen <jiewen.yao@intel.com>
> > > Sent: Tuesday, September 6, 2022 4:27 PM
> > > To: Boeuf, Sebastien <sebastien.boeuf@intel.com>
> > > Cc: Justen, Jordan L <jordan.l.justen@intel.com>;kraxel@redhat.com
> > > <kraxel@redhat.com>;devel@edk2.groups.io <devel@edk2.groups.io>
> > > Subject: RE: [PATCH v2] OvmfPkg: Update I/O port related to ACPI
> > > devices for CloudHv
> > >
> > > This seems a big incompatible change.
> > >
> > > I feel this is weird to hardcode the configuration here.
> > >
> > > How the OVMF binary knows it runs on a new CloudHv or old CloudHv?
> > >
> > > Can we have a mechanism to detect the data at runtime? E.g. read
> > > some
> > > PCI register ?
> > >
> > >
> > >
> > >
> > > From: Boeuf, Sebastien <sebastien.boeuf@intel.com>
> > > Sent: Tuesday, September 6, 2022 10:08 PM
> > > To: Yao, Jiewen <jiewen.yao@intel.com>
> > > Cc: Justen, Jordan L <jordan.l.justen@intel.com>;kraxel@redhat.com;
> > > devel@edk2.groups.io
> > > Subject: Re: [PATCH v2] OvmfPkg: Update I/O port related to ACPI
> > > devices for CloudHv
> > >
> > > Hi Jiewen,
> > >
> > > Do you think this could be merged?
> > >
> > > Thanks,
> > > Sebastien
> > > From: Boeuf, Sebastien <sebastien.boeuf@intel.com>
> > > Sent: Friday, August 19, 2022 11:59 AM
> > > To: devel@edk2.groups.io <devel@edk2.groups.io>
> > > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan L
> > > <jordan.l.justen@intel.com>;kraxel@redhat.com <kraxel@redhat.com>;
> > > Boeuf, Sebastien <sebastien.boeuf@intel.com>
> > > Subject: [PATCH v2] OvmfPkg: Update I/O port related to ACPI
> > > devices
> > > for CloudHv
> > >
> > > From: Sebastien Boeuf <sebastien.boeuf@intel.com>
> > >
> > > Both ACPI shutdown and ACPI PM timer devices has been moved to
> > > different
> > > port addresses in the latest version of Cloud Hypervisor. These
> > > changes
> > > need to be reflected on the OVMF firmware.
> > >
> > > Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> > > Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>
> > > ---
> > >  OvmfPkg/Include/IndustryStandard/CloudHv.h | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/OvmfPkg/Include/IndustryStandard/CloudHv.h
> > > b/OvmfPkg/Include/IndustryStandard/CloudHv.h
> > > index d31ecc9eec..527c236f48 100644
> > > --- a/OvmfPkg/Include/IndustryStandard/CloudHv.h
> > > +++ b/OvmfPkg/Include/IndustryStandard/CloudHv.h
> > > @@ -16,12 +16,12 @@
> > >  //
> > >
> > >  // ACPI timer address
> > >
> > >  //
> > >
> > > -#define CLOUDHV_ACPI_TIMER_IO_ADDRESS  0xb008
> > >
> > > +#define CLOUDHV_ACPI_TIMER_IO_ADDRESS  0x0608
> > >
> > >
> > >
> > >  //
> > >
> > >  // ACPI shutdown device address
> > >
> > >  //
> > >
> > > -#define CLOUDHV_ACPI_SHUTDOWN_IO_ADDRESS  0x03c0
> > >
> > > +#define CLOUDHV_ACPI_SHUTDOWN_IO_ADDRESS  0x0600
> > >
> > >
> > >
> > >  //
> > >
> > >  // 32-bit MMIO memory hole base address
> > >
> >