Re: [Xen-devel] [PATCH] OvmfPkg: Add ACPI support for Virt Xen ARM

[Shannon Zhao <zhaoshenglong@xxxxxxxxxx>]

On 2016/5/31 18:35, Laszlo Ersek wrote:
> On 05/31/16 06:59, Shannon Zhao wrote:
>> > From: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
>> > 
>> > Add ACPI support for Virt Xen ARM and it gets the ACPI tables through
>> > Xen ARM multiboot protocol.
>> > 
>> > Contributed-under: TianoCore Contribution Agreement 1.0
>> > Signed-off-by: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
>> > ---
>> > The corresponding Xen patches can be fetched from:
>> > http://git.linaro.org/people/shannon.zhao/xen.git/shortlog/refs/heads/domu_acpi
>> > ---
>> >  ArmVirtPkg/ArmVirtXen.dsc                         |   6 +
>> >  ArmVirtPkg/ArmVirtXen.fdf                         |   6 +
>> >  OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h            |   6 +
>> >  OvmfPkg/AcpiPlatformDxe/XenArmAcpi.c              | 207 
>> > ++++++++++++++++++++++
>> >  OvmfPkg/AcpiPlatformDxe/XenArmAcpiPlatform.c      |  38 ++++
>> >  OvmfPkg/AcpiPlatformDxe/XenArmAcpiPlatformDxe.inf |  59 ++++++
>> >  6 files changed, 322 insertions(+)
>> >  create mode 100644 OvmfPkg/AcpiPlatformDxe/XenArmAcpi.c
>> >  create mode 100644 OvmfPkg/AcpiPlatformDxe/XenArmAcpiPlatform.c
>> >  create mode 100644 OvmfPkg/AcpiPlatformDxe/XenArmAcpiPlatformDxe.inf
> Jordan and I might disagree about this patch, but here's my comments
> about it:
> 
> I don't think this patch belongs under OvmfPkg. Namely, the only file
> that XenArmAcpiPlatformDxe reuses from the existing
> OvmfPkg/AcpiPlatformDxe/ directory is "EntryPoint.c", which is (a)
> trivial, (b) its conditions and branches should be possible to evaluate
> statically for aarch64 Xen. (PcdPciDisableBusEnumeration should be set
> to TRUE in ArmVirtXen.dsc.)
> 
> Second, the new code uses the FDT library directly (from EmbeddedPkg),
> and accesses QEMU's DTB directly (with the fdt_*() APIs). However, in
> ArmVirtPkg we have moved away from this, and now we have a custom
> (edk2-only, non-standard) FdtClient protocol for accessing the FDT.
> Please see:
> - ArmVirtPkg/Include/Protocol/FdtClient.h
> - ArmVirtPkg/FdtClientDxe/
> 
> In order to rebase this patch to the FDT Client Protocol, while keeping
> it under OvmfPkg, the "XenArmAcpiPlatformDxe.inf" file would have to
> list "ArmVirtPkg/ArmVirtPkg.dec" in the [Packages] section. I think that
> would be very ugly. Thus far we have managed to avoid mutual references
> between OvmfPkg and ArmVirtPkg: OvmfPkg doesn't consume anything from
> ArmVirtPkg, and that's how things should stay in my opinion.
> 
> Which is why I suggest to implement this functionality entirely under
> ArmVirtPkg, and using the FDT Client Protocol at that.
> 
Thanks for your explanation. I will move this into ArmVirtPkg.

> If a non-trivial chunk of functionality can be identified between
> OvmfPkg and ArmVirtPkg in this regard (that currently exists under
> OvmfPkg/AcpiPlatformDxe/), then it should be extracted into a library
> (under OvmfPkg/Include/Library and OvmfPkg/Library), and the ArmVirtPkg
> code should become a client of that library. (You can find many similar
> OvmfPkg/Library/ resolutions in the ArmVirtPkg/ DSC files.)
> 
> NB: Ard is going to be on vacation for a while, and I think we should
> definitely await his feedback on this. First, my participation in
> ArmVirtXen matters is quite minimal. Second, Ard designed the FDT Client
> Protocol; in case you need extensions to the current APIs for
> implementing the feature, then Ard is the one to review them.
I think it will only use the existing FindCompatibleNodeReg() function
of FDT Client. So I'll move on next version.

Thanks,
-- 
Shannon


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel