hw/acpi/Makefile.objs | 1 + hw/acpi/aml-build.c | 985 +++++++++++++++++++++++++++++ hw/acpi/builder.c | 97 +++ hw/acpi/cpu.c | 8 +- hw/acpi/cpu_hotplug.c | 9 +- hw/acpi/memory_hotplug.c | 21 +- hw/acpi/pcihp.c | 10 +- hw/arm/virt-acpi-build.c | 94 +-- hw/i386/acpi-build.c | 1071 +++----------------------------- hw/i386/acpi-build.h | 9 +- hw/i386/pc.c | 198 +++--- hw/i386/pc_piix.c | 36 +- hw/i386/pc_q35.c | 22 +- hw/i386/xen/xen-hvm.c | 19 +- hw/pci-host/piix.c | 32 +- include/hw/acpi/acpi-defs.h | 14 + include/hw/acpi/acpi.h | 44 ++ include/hw/acpi/aml-build.h | 47 ++ include/hw/acpi/builder.h | 100 +++ include/hw/i386/acpi.h | 27 + include/hw/i386/pc.h | 49 +- include/hw/mem/memory-device.h | 2 + include/hw/pci/pci_host.h | 6 + stubs/Makefile.objs | 1 - stubs/pci-host-piix.c | 6 - 25 files changed, 1648 insertions(+), 1260 deletions(-) create mode 100644 hw/acpi/builder.c create mode 100644 include/hw/acpi/builder.h create mode 100644 include/hw/i386/acpi.h delete mode 100644 stubs/pci-host-piix.c
This patch set provides an ACPI code reorganization in preparation for adding hardware-reduced support to QEMU. The changes are coming from the NEMU [1] project where we're defining a new x86 machine type: i386/virt. This is an EFI only, ACPI hardware-reduced platform and as such we had to implement support for the latter. As a preliminary for adding hardware-reduced support to QEMU, we did some ACPI code reorganization with the following goals: * Share as much as possible of the current ACPI build APIs between legacy and hardware-reduced ACPI. * Share the ACPI build code across machine types and architectures and remove the typical PC machine type dependency. Eventually we hope to see arm/virt also re-use much of that code. The patches are also available in their own git branch [2]. [1] https://github.com/intel/nemu [2] https://github.com/intel/nemu/tree/topic/upstream/acpi v1 -> v2: * Drop the hardware-reduced implementation for now. Our next patch set will add hardware-reduced and convert arm/virt to it. * Implement the ACPI build methods as a QOM Interface Class and convert the PC machine type to it. * acpi_conf_pc_init() uses a PCMachineState pointer and not a MachineState one as its argument. v2 -> v3: * Cc all relevant maintainers, no functional changes. v3 -> v4: * Renamed all AcpiConfiguration pointers from conf to acpi_conf. * Removed the ACPI_BUILD_ALIGN_SIZE export. * Temporarily updated the arm virt build_rsdp() prototype for bisectability purposes. * Removed unneeded pci headers from acpi-build.c. * Refactor the acpi PCI host getter so that it truly is architecture agnostic, by carrying the PCI host pointer through the AcpiConfiguration structure. * Splitted the PCI host AML builder API export patch from the PCI host and holes getter one. * Reduced the build_srat() export scope to hw/i386 instead of the broader hw/acpi. SRAT builders are truly architecture specific and can hardly be generalized. * Completed the ACPI builder documentation. Samuel Ortiz (15): hw: i386: Decouple the ACPI build from the PC machine type hw: acpi: Export ACPI build alignment API hw: acpi: Export the RSDP build API hw: acpi: Implement XSDT support for RSDP hw: arm: Switch to the AML build RSDP building routine hw: i386: Move PCI host definitions to pci_host.h hw: acpi: Export the PCI host and holes getters hw: acpi: Do not create hotplug method when handler is not defined hw: i386: Make the hotpluggable memory size property more generic hw: i386: Export the i386 ACPI SRAT build method hw: i386: Export the MADT build method hw: acpi: Define ACPI tables builder interface hw: i386: Implement the ACPI builder interface for PC hw: pci-host: piix: Return PCI host pointer instead of PCI bus hw: i386: Set ACPI configuration PCI host pointer Sebastien Boeuf (2): hw: acpi: Export the PCI hotplug API hw: acpi: Retrieve the PCI bus from AcpiPciHpState Yang Zhong (6): hw: acpi: Generalize AML build routines hw: acpi: Factorize _OSC AML across architectures hw: acpi: Export and generalize the PCI host AML API hw: acpi: Export the MCFG getter hw: acpi: Fix memory hotplug AML generation error hw: i386: Refactor PCI host getter hw/acpi/Makefile.objs | 1 + hw/acpi/aml-build.c | 985 +++++++++++++++++++++++++++++ hw/acpi/builder.c | 97 +++ hw/acpi/cpu.c | 8 +- hw/acpi/cpu_hotplug.c | 9 +- hw/acpi/memory_hotplug.c | 21 +- hw/acpi/pcihp.c | 10 +- hw/arm/virt-acpi-build.c | 94 +-- hw/i386/acpi-build.c | 1071 +++----------------------------- hw/i386/acpi-build.h | 9 +- hw/i386/pc.c | 198 +++--- hw/i386/pc_piix.c | 36 +- hw/i386/pc_q35.c | 22 +- hw/i386/xen/xen-hvm.c | 19 +- hw/pci-host/piix.c | 32 +- include/hw/acpi/acpi-defs.h | 14 + include/hw/acpi/acpi.h | 44 ++ include/hw/acpi/aml-build.h | 47 ++ include/hw/acpi/builder.h | 100 +++ include/hw/i386/acpi.h | 27 + include/hw/i386/pc.h | 49 +- include/hw/mem/memory-device.h | 2 + include/hw/pci/pci_host.h | 6 + stubs/Makefile.objs | 1 - stubs/pci-host-piix.c | 6 - 25 files changed, 1648 insertions(+), 1260 deletions(-) create mode 100644 hw/acpi/builder.c create mode 100644 include/hw/acpi/builder.h create mode 100644 include/hw/i386/acpi.h delete mode 100644 stubs/pci-host-piix.c -- 2.19.1
On Thu, 1 Nov 2018 11:22:40 +0100 Samuel Ortiz <sameo@linux.intel.com> wrote: Thanks for looking at ACPI mess we have in QEMU and trying to make it better, this series look a bit hackish probably because it was written to suit new virt board, so it needs some more clean up to be done. > This patch set provides an ACPI code reorganization in preparation for > adding hardware-reduced support to QEMU. QEMU already has hw reduced implementation, specifically in arm/virt board > The changes are coming from the NEMU [1] project where we're defining > a new x86 machine type: i386/virt. This is an EFI only, ACPI > hardware-reduced platform and as such we had to implement support > for the latter. > > As a preliminary for adding hardware-reduced support to QEMU, we did s:support to QEMU:support for new i386/virt machine: > some ACPI code reorganization with the following goals: > > * Share as much as possible of the current ACPI build APIs between > legacy and hardware-reduced ACPI. > * Share the ACPI build code across machine types and architectures and > remove the typical PC machine type dependency. > Eventually we hope to see arm/virt also re-use much of that code. it probably should be other way around, generalize and reuse as much of arm/virt acpi code, instead of adding new duplicated code without an actual user and then swapping/dropping old arm version in favor of the new one. It's hard to review when it done in this order and easy to miss issues that would be easier to spot if you reused arm versions (where applicable) as starting point for generalization. Here are some generic suggestions/nits that apply to whole series: * s/Factorize/Factor out/ * try to restructure series in following way 1. put bug fixes at the beginning of series. 'make V=1 check' should produce tables diffs to account for changes made in the tables. After error fixing, add an extra patch to update reference ACPI tables to simplify testing for reviewing and so for self-check when you'll be doing refactoring to make sure there aren't any changes during generalization/refactoring later. 2. instead of adding 'new' implementations try to generalize existing ones so that a user for new code will always exist. It's also makes patches easier to review/test. 3. Since you are touching/moving around existing fixed tables that are using legacy 'struct' based approach to construct them, it's a good as opportunity to switch to a newer approach and use build_append_int_noprefix() API to construct tables. Use build_amd_iommu() as example. And it's doubly true if/when you are adding new fixed tables (i.e. only build_append_int_noprefix() based ones are acceptable). 4. for patches during #2 and #3 stages, 'make V=1 check' should pass without any warnings, that will speed up review process. 5. add i386/virt board and related hardware reduced ACPI code that's specific to it. I'll try to review series during next week and will do per patch suggestions how to structure it or do other way. Hopefully after doing refactoring we would end up with simpler/smaller and cleaner ACPI code to the benefit of everyone. PS: if you need a quick advice wrt APCI parts, feel free to ping me on IRC (Paris timezone). > The patches are also available in their own git branch [2]. > > [1] https://github.com/intel/nemu > [2] https://github.com/intel/nemu/tree/topic/upstream/acpi > > v1 -> v2: > * Drop the hardware-reduced implementation for now. Our next patch set > will add hardware-reduced and convert arm/virt to it. > * Implement the ACPI build methods as a QOM Interface Class and convert > the PC machine type to it. > * acpi_conf_pc_init() uses a PCMachineState pointer and not a > MachineState one as its argument. > > v2 -> v3: > * Cc all relevant maintainers, no functional changes. > > v3 -> v4: > * Renamed all AcpiConfiguration pointers from conf to acpi_conf. > * Removed the ACPI_BUILD_ALIGN_SIZE export. > * Temporarily updated the arm virt build_rsdp() prototype for > bisectability purposes. > * Removed unneeded pci headers from acpi-build.c. > * Refactor the acpi PCI host getter so that it truly is architecture > agnostic, by carrying the PCI host pointer through the > AcpiConfiguration structure. > * Splitted the PCI host AML builder API export patch from the PCI host > and holes getter one. > * Reduced the build_srat() export scope to hw/i386 instead of the broader > hw/acpi. SRAT builders are truly architecture specific and can hardly be > generalized. > * Completed the ACPI builder documentation. > > Samuel Ortiz (15): > hw: i386: Decouple the ACPI build from the PC machine type > hw: acpi: Export ACPI build alignment API > hw: acpi: Export the RSDP build API > hw: acpi: Implement XSDT support for RSDP > hw: arm: Switch to the AML build RSDP building routine > hw: i386: Move PCI host definitions to pci_host.h > hw: acpi: Export the PCI host and holes getters > hw: acpi: Do not create hotplug method when handler is not defined > hw: i386: Make the hotpluggable memory size property more generic > hw: i386: Export the i386 ACPI SRAT build method > hw: i386: Export the MADT build method > hw: acpi: Define ACPI tables builder interface > hw: i386: Implement the ACPI builder interface for PC > hw: pci-host: piix: Return PCI host pointer instead of PCI bus > hw: i386: Set ACPI configuration PCI host pointer > > Sebastien Boeuf (2): > hw: acpi: Export the PCI hotplug API > hw: acpi: Retrieve the PCI bus from AcpiPciHpState > > Yang Zhong (6): > hw: acpi: Generalize AML build routines > hw: acpi: Factorize _OSC AML across architectures > hw: acpi: Export and generalize the PCI host AML API > hw: acpi: Export the MCFG getter > hw: acpi: Fix memory hotplug AML generation error > hw: i386: Refactor PCI host getter > > hw/acpi/Makefile.objs | 1 + > hw/acpi/aml-build.c | 985 +++++++++++++++++++++++++++++ > hw/acpi/builder.c | 97 +++ > hw/acpi/cpu.c | 8 +- > hw/acpi/cpu_hotplug.c | 9 +- > hw/acpi/memory_hotplug.c | 21 +- > hw/acpi/pcihp.c | 10 +- > hw/arm/virt-acpi-build.c | 94 +-- > hw/i386/acpi-build.c | 1071 +++----------------------------- > hw/i386/acpi-build.h | 9 +- > hw/i386/pc.c | 198 +++--- > hw/i386/pc_piix.c | 36 +- > hw/i386/pc_q35.c | 22 +- > hw/i386/xen/xen-hvm.c | 19 +- > hw/pci-host/piix.c | 32 +- > include/hw/acpi/acpi-defs.h | 14 + > include/hw/acpi/acpi.h | 44 ++ > include/hw/acpi/aml-build.h | 47 ++ > include/hw/acpi/builder.h | 100 +++ > include/hw/i386/acpi.h | 27 + > include/hw/i386/pc.h | 49 +- > include/hw/mem/memory-device.h | 2 + > include/hw/pci/pci_host.h | 6 + > stubs/Makefile.objs | 1 - > stubs/pci-host-piix.c | 6 - > 25 files changed, 1648 insertions(+), 1260 deletions(-) > create mode 100644 hw/acpi/builder.c > create mode 100644 include/hw/acpi/builder.h > create mode 100644 include/hw/i386/acpi.h > delete mode 100644 stubs/pci-host-piix.c >
Hi Igor, On Fri, Nov 02, 2018 at 01:29:25PM +0100, Igor Mammedov wrote: > On Thu, 1 Nov 2018 11:22:40 +0100 > Samuel Ortiz <sameo@linux.intel.com> wrote: > > Thanks for looking at ACPI mess we have in QEMU and trying to make it better, Thanks for the initial review and feedback. > this series look a bit hackish probably because it was written to suit new > virt board, so it needs some more clean up to be done. > > > This patch set provides an ACPI code reorganization in preparation for > > adding hardware-reduced support to QEMU. > QEMU already has hw reduced implementation, specifically in arm/virt board Back in May, I tried booting a virt machine type with "acpi=force" with no success, and today's HEAD still fails. With "acpi=on": [ 0.000000] ACPI: Early table checksum verification disabled [ 0.000000] ACPI: Failed to init ACPI tables So this code has been broken for several months and I suspect it's not really run by anyone. Moreover, even though the virt-acpi-build.c code aims at building a hardware-reduced ACPI compliant set of tables, the implementation is largely specific to the arm/virt board. We did take the generic parts from it in order to build a shareable API across architectures. So by "adding hardware-reduced support to QEMU", we meant providing a architecture and machine type agnostic hardware-reduced ACPI implementation that all QEMU machine types could use. I'll make sure to change the cover letter wording and rephrase it to reflect our intention. > > The changes are coming from the NEMU [1] project where we're defining > > a new x86 machine type: i386/virt. This is an EFI only, ACPI > > hardware-reduced platform and as such we had to implement support > > for the latter. > > > > As a preliminary for adding hardware-reduced support to QEMU, we did > s:support to QEMU:support for new i386/virt machine: Most of this serie moves non x86 specific stuff out of i386 into the (theoretically) architecture agnostic hw/acpi folder in order to a) reuse and share as much as possible of the non x86 specific ACPI code that currently lives under hw/i386 and b) improve and extend the current hw/acpi APIs. So on one hand I agree this cover letter needs massaging, but on the other hand I feel it's unfair to say there's anything specific to i386/virt on this serie. At least we tried really hard to avoid falling into that trap. > > some ACPI code reorganization with the following goals: > > > > * Share as much as possible of the current ACPI build APIs between > > legacy and hardware-reduced ACPI. > > * Share the ACPI build code across machine types and architectures and > > remove the typical PC machine type dependency. > > Eventually we hope to see arm/virt also re-use much of that code. > it probably should be other way around, generalize and reuse as much of > arm/virt acpi code, Our hardware-reduced implementation does that indeed, and the next patchset following this one will add the actual hardware-reduced ACPI API implementation together with the arm/virt switch to it (which will mostly be code removal from virt-acpi-build.c). > instead of adding new duplicated code without > an actual user and then swapping/dropping old arm version in favor of the > new one. This patch set is not adding any code duplication, I hope. It's really preparing the current ACPI code in order to precisely NOT add code duplication when building a machine type agnostic hardware reduced ACPI API. I initally added our hardware-reduced ACPI API to that patch serie but decided to remove it. Here is what it looks like: https://github.com/intel/nemu/blob/topic/virt-x86/hw/acpi/reduced.c It's consuming the factorization work that this serie provides, and the arm/virt machine type will use a fairly large part of that API. > It's hard to review when it done in this order and easy to miss > issues that would be easier to spot if you reused arm versions (where > applicable) as starting point for generalization. > > Here are some generic suggestions/nits that apply to whole series: > * s/Factorize/Factor out/ > * try to restructure series in following way > 1. put bug fixes at the beginning of series. I'll try to do that, yes. > 'make V=1 check' should produce tables diffs to account for > changes made in the tables. I'll run the make check tests, thanks for the reminder. > After error fixing, add an extra patch to update reference > ACPI tables to simplify testing for reviewing and so for > self-check when you'll be doing refactoring to make sure > there aren't any changes during generalization/refactoring > later. > > 2. instead of adding 'new' implementations try to generalize > existing ones so that a user for new code will always exist. > It's also makes patches easier to review/test. The PC machine type is consuming all the exported API and e.g. the new ACPI builder interface as well. > 3. Since you are touching/moving around existing fixed tables > that are using legacy 'struct' based approach to construct > them, it's a good as opportunity to switch to a newer approach > and use build_append_int_noprefix() API to construct tables. > Use build_amd_iommu() as example. And it's doubly true if/when > you are adding new fixed tables (i.e. only build_append_int_noprefix() > based ones are acceptable). I'd be happy to do that, but I'd appreciate if that could be done as a follow up patch set, in order to decouple the code movement/export from the actual reimplementation. > 4. for patches during #2 and #3 stages, 'make V=1 check' > should pass without any warnings, that will speed up review > process. > > 5. add i386/virt board and related hardware reduced ACPI code > that's specific to it. That is definitely going to be our last steps, but I think this would make a very large patch set to review at once. > I'll try to review series during next week and will do per patch > suggestions how to structure it or do other way. Thanks in advance. FWIW I just sent v5, addressing Philippe's comments. I'll wait for your feedback before sending v6. > Hopefully after doing refactoring we would end up with > simpler/smaller and cleaner ACPI code to the benefit of everyone. > > PS: > if you need a quick advice wrt APCI parts, feel free to ping me on IRC > (Paris timezone). Nice, same timezone for me :) I'll get in touch next week. Cheers, Samuel.
Igor, On Mon, Nov 05, 2018 at 03:10:28AM +0100, Samuel Ortiz wrote: > > QEMU already has hw reduced implementation, specifically in arm/virt board > Back in May, I tried booting a virt machine type with "acpi=force" with > no success, and today's HEAD still fails. With "acpi=on": > > [ 0.000000] ACPI: Early table checksum verification disabled > [ 0.000000] ACPI: Failed to init ACPI tables My bad, I was using the wrong firmware/pflash options. With the right ones, I can boot with an edk2 firmware and an EFI variable space. Thanks Philippe and Eric for the help. Cheers, Samuel.
On Mon, Nov 05, 2018 at 03:10:28AM +0100, Samuel Ortiz wrote: > Hi Igor, > > On Fri, Nov 02, 2018 at 01:29:25PM +0100, Igor Mammedov wrote: > > On Thu, 1 Nov 2018 11:22:40 +0100 > > Samuel Ortiz <sameo@linux.intel.com> wrote: > > > > Thanks for looking at ACPI mess we have in QEMU and trying to make it better, > Thanks for the initial review and feedback. > > > this series look a bit hackish probably because it was written to suit new > > virt board, so it needs some more clean up to be done. > > > > > This patch set provides an ACPI code reorganization in preparation for > > > adding hardware-reduced support to QEMU. > > QEMU already has hw reduced implementation, specifically in arm/virt board > Back in May, I tried booting a virt machine type with "acpi=force" with > no success, and today's HEAD still fails. With "acpi=on": > > [ 0.000000] ACPI: Early table checksum verification disabled > [ 0.000000] ACPI: Failed to init ACPI tables > > So this code has been broken for several months and I suspect it's not > really run by anyone. Hi Samuel, We use it all the time. To see if there had been an upstream regression I just tried latest qemu (v3.0.0-1763-gb2f7a038bb4c) with a latest kernel (v4.20-rc1) in my guest. It worked just fine, and there was no need for any kernel command line parameters. What guest kernel version did you try? Thanks, drew
Hi Andrew, On Mon, Nov 05, 2018 at 05:07:53PM +0100, Andrew Jones wrote: > > > QEMU already has hw reduced implementation, specifically in arm/virt board > > Back in May, I tried booting a virt machine type with "acpi=force" with > > no success, and today's HEAD still fails. With "acpi=on": > > > > [ 0.000000] ACPI: Early table checksum verification disabled > > [ 0.000000] ACPI: Failed to init ACPI tables > > > > So this code has been broken for several months and I suspect it's not > > really run by anyone. > > Hi Samuel, > > We use it all the time. To see if there had been an upstream regression I > just tried latest qemu (v3.0.0-1763-gb2f7a038bb4c) with a latest kernel > (v4.20-rc1) in my guest. It worked just fine, and there was no need for > any kernel command line parameters. > > What guest kernel version did you try? This is my bad, apologies for jumping the gun. I was not using the right pflash options and thus the ACPI tables were simply not passed to the kernel. Philippe and Eric helped me fix that, and now I can boot arm/virt on top of ACPI. Sorry for the bad noise. Cheers, Samuel.
On Mon, 5 Nov 2018 03:10:28 +0100 Samuel Ortiz <sameo@linux.intel.com> wrote: > Hi Igor, > > On Fri, Nov 02, 2018 at 01:29:25PM +0100, Igor Mammedov wrote: > > On Thu, 1 Nov 2018 11:22:40 +0100 > > Samuel Ortiz <sameo@linux.intel.com> wrote: [...] > > > some ACPI code reorganization with the following goals: > > > > > > * Share as much as possible of the current ACPI build APIs between > > > legacy and hardware-reduced ACPI. > > > * Share the ACPI build code across machine types and architectures and > > > remove the typical PC machine type dependency. > > > Eventually we hope to see arm/virt also re-use much of that code. > > it probably should be other way around, generalize and reuse as much of > > arm/virt acpi code, > Our hardware-reduced implementation does that indeed, and the next > patchset following this one will add the actual hardware-reduced ACPI > API implementation together with the arm/virt switch to it (which will > mostly be code removal from virt-acpi-build.c). That's where I disagree with patch ordering, i.e. you are adding impl. without user and than do sudden switch from old API (dropping old one in the process) to the new one some time in the future. For example 04/23 add new impl. of build_rsdp() with xsdt support without actual user, which is duplicate of arm/virt code modulo checksum fix and than later in 05/23 drops old arm impl. in favor of the new one. As you saw it causes confusion during review even within one series and it's much worse in cases where similar changes happen across several series. In addition bisection would point into wrong patch 05/23 (where code started to be used but not where it was introduced). > > instead of adding new duplicated code without > > an actual user and then swapping/dropping old arm version in favor of the > > new one. > This patch set is not adding any code duplication, I hope. > It's really preparing the current ACPI code in order to precisely NOT add > code duplication when building a machine type agnostic hardware reduced > ACPI API. > I initally added our hardware-reduced ACPI API to that patch serie but > decided to remove it. Here is what it looks like: > https://github.com/intel/nemu/blob/topic/virt-x86/hw/acpi/reduced.c > > It's consuming the factorization work that this serie provides, and the > arm/virt machine type will use a fairly large part of that API. the point is existing arm/virt machine or any other machine should use new API right away if applicable instead of some point in the future, otherwise we are doing something wrong, change might not belong to series. It's hard to find reasoning for merging new API/verify correctness without immediate user. > > It's hard to review when it done in this order and easy to miss > > issues that would be easier to spot if you reused arm versions (where > > applicable) as starting point for generalization. [...] > > 3. Since you are touching/moving around existing fixed tables > > that are using legacy 'struct' based approach to construct > > them, it's a good as opportunity to switch to a newer approach > > and use build_append_int_noprefix() API to construct tables. > > Use build_amd_iommu() as example. And it's doubly true if/when > > you are adding new fixed tables (i.e. only build_append_int_noprefix() > > based ones are acceptable). > I'd be happy to do that, but I'd appreciate if that could be done as a > follow up patch set, in order to decouple the code movement/export from > the actual reimplementation. Well, it fine to keep old style if one fixes code but in all other cases it's good opportunity to replace legacy approach with the preferred one. So unless it's a fix, the new code (including generalization) should use preferred approach (there are exceptions but this series is not the case). Considering this series is re-factoring to make ACPI code more reusable, we should do it properly instead of cutting corners. I apply this rule myself 'ex: build_fadt()' and others if re-factoring is related the series topic. > > 4. for patches during #2 and #3 stages, 'make V=1 check' > > should pass without any warnings, that will speed up review > > process. > > > > 5. add i386/virt board and related hardware reduced ACPI code > > that's specific to it. > That is definitely going to be our last steps, but I think this would > make a very large patch set to review at once. Agreed, as far as new API is used by already existing code. If API isn't used then it should be moved out of this series to the one that would actually use it. [...]
© 2016 - 2024 Red Hat, Inc.