| Message ID | 20181022183656.4902-1-sameo@linux.intel.com |
|---|---|
| Headers | show |
| Series | ACPI hardware-reduced support | expand |
On Mon, Oct 22, 2018 at 08:36:30PM +0200, Samuel Ortiz wrote: > This patch set implements support for the ACPI hardware-reduced > specification. > > 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 rest of the patchset adds the hardware-reduced support on top of > this code reorganization. Here again, the implementation is machine > type, platform and architecture independent. > > [1] https://github.com/intel/nemu Thanks for the patch! I'm traveling so won't be able to review until next week. I wonder whether the code can be refactored slightly differently: instead of common code calling out to platform specific one, have platform code call out to generic one. That would avoid indicectiin but I'm not sure at what cost in complexity. Still that's a generic question as I have not looked at the patches in depth. Hope this helps, MST > > The following changes since commit b312532fd03413d0e6ae6767ec793a3e30f487b8: > > Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2018-10-19 19:01:07 +0100) > > are available in the Git repository at: > > git@github.com:intel/nemu topic/upstream/acpi > > for you to fetch changes up to 4133ebe0d638d0a3d583bd89aa2360ef35ba746b: > > hw: acpi: reduced: Add PCI hotplug support (2018-10-22 20:32:23 +0200) > > ---------------------------------------------------------------- > Jing Liu (2): > hw: acpi: reduced: Add shutdown support > hw: acpi: reduced: Add reboot support > > Samuel Ortiz (10): > 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: arm: Switch to the AML build RSDP building routine > hw: acpi: Do not create hotplug method when handler is not defined > hw: i386: Make the hotpluggable memory size property more generic > hw: fw-build: Add firmware build methods and state > hw: i386: Convert PC machine type to firmware build methods > hw: acpi: Initial hardware-reduced support > hw: acpi: reduced: Generic Event Device support > > Sebastien Boeuf (4): > hw: acpi: Export the PCI hotplug API > hw: acpi: Retrieve the PCI bus from AcpiPciHpState > hw: acpi: reduced: Add NFIT support > hw: acpi: reduced: Add PCI hotplug support > > Yang Zhong (10): > hw: acpi: Generalize AML build routines > hw: acpi: Factorize _OSC AML across architectures > hw: i386: Refactor PCI host getter > hw: acpi: Export and generalize the PCI host AML API > hw: acpi: Export the MCFG getter > hw: acpi: Export the SRAT AML build API > hw: acpi: Fix memory hotplug AML generation error > hw: acpi: reduced: Add MCFG support > hw: acpi: reduced: Add memory hotplug support > hw: acpi: reduced: Add SRAT table > > default-configs/i386-softmmu.mak | 1 + > hw/acpi/Makefile.objs | 1 + > hw/acpi/aml-build.c | 1139 +++++++++++++++++++++++++++++++++++- > hw/acpi/cpu.c | 8 +- > hw/acpi/cpu_hotplug.c | 9 +- > hw/acpi/memory_hotplug.c | 21 +- > hw/acpi/pcihp.c | 10 +- > hw/acpi/reduced.c | 472 +++++++++++++++ > hw/arm/virt-acpi-build.c | 86 +-- > hw/i386/acpi-build.c | 1172 +++----------------------------------- > hw/i386/acpi-build.h | 4 +- > hw/i386/pc.c | 188 +++--- > hw/i386/pc_piix.c | 21 +- > hw/i386/pc_q35.c | 21 +- > hw/pci-host/piix.c | 8 - > include/hw/acpi/acpi-defs.h | 15 + > include/hw/acpi/acpi.h | 47 ++ > include/hw/acpi/aml-build.h | 48 ++ > include/hw/acpi/reduced.h | 46 ++ > include/hw/boards.h | 5 + > include/hw/fw-build.h | 57 ++ > include/hw/i386/acpi.h | 27 + > include/hw/i386/pc.h | 21 +- > include/hw/mem/memory-device.h | 2 + > stubs/Makefile.objs | 1 - > stubs/pci-host-piix.c | 6 - > 26 files changed, 2107 insertions(+), 1329 deletions(-) > create mode 100644 hw/acpi/reduced.c > create mode 100644 include/hw/acpi/reduced.h > create mode 100644 include/hw/fw-build.h > create mode 100644 include/hw/i386/acpi.h > delete mode 100644 stubs/pci-host-piix.c > > -- > 2.17.2 >
On Mon, Oct 22, 2018 at 05:28:54PM -0400, Michael S. Tsirkin wrote: > On Mon, Oct 22, 2018 at 08:36:30PM +0200, Samuel Ortiz wrote: > > This patch set implements support for the ACPI hardware-reduced > > specification. > > > > 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 rest of the patchset adds the hardware-reduced support on top of > > this code reorganization. Here again, the implementation is machine > > type, platform and architecture independent. > > > > [1] https://github.com/intel/nemu > > > Thanks for the patch! > I'm traveling so won't be able to review until next week. No problem. I'm assuming you'll be attending the KVM Forum, Rob Bradford and myself will be there as well. > I wonder whether the code can be refactored slightly differently: > instead of common code calling out to platform specific one, > have platform code call out to generic one. > That would avoid indicectiin but I'm not sure at what cost > in complexity. I may be misunderstanding your point, so forgive me if that's the case. But the code refactoring actually lead to both of the code paths you're describing. The generic code calls into platform specific hooks for potentially calling custom ACPI tables, but the platform code also uses the now shared generic code. > Still that's a generic question as I have not looked at the > patches in depth. Looking forward to getting your review. Cheers, Samuel.
On Tue, Oct 23, 2018 at 11:00:18AM +0200, Samuel Ortiz wrote: > > I wonder whether the code can be refactored slightly differently: > > instead of common code calling out to platform specific one, > > have platform code call out to generic one. > > That would avoid indicectiin should have been indirection > > but I'm not sure at what cost > > in complexity. > I may be misunderstanding your point, so forgive me if that's the case. > But the code refactoring actually lead to both of the code paths you're > describing. The generic code calls into platform specific hooks for > potentially calling custom ACPI tables, but the platform code also uses > the now shared generic code. Right. So if instead of both we could have only platform calling generic code, we could drop some of the hooks. What I'm saying is that if I look at patches 1-17 which are supposed to be just refactorings, somehow at the surface instead of reducing lines of code they add 100 LOC. And the whole patchset adds about 1000 LOC, by comparison e.g. pc_q35.c is just 400 LOC since q35 and pc share a bunch of code. I wonder why can't more code be shared with e.g. ARM which also has a virt machine type. In other words, assuming we are doing PV, can't it be more portable. Again all that's still because all I read is the diffstats, it's without reading the patches, sorry about that.
On 22 October 2018 at 19:36, Samuel Ortiz <sameo@linux.intel.com> wrote: > This patch set implements support for the ACPI hardware-reduced > specification. > > 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 rest of the patchset adds the hardware-reduced support on top of > this code reorganization. Here again, the implementation is machine > type, platform and architecture independent. > > [1] https://github.com/intel/nemu > > > The following changes since commit b312532fd03413d0e6ae6767ec793a3e30f487b8: > > Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2018-10-19 19:01:07 +0100) > > are available in the Git repository at: > > git@github.com:intel/nemu topic/upstream/acpi > > for you to fetch changes up to 4133ebe0d638d0a3d583bd89aa2360ef35ba746b: > > hw: acpi: reduced: Add PCI hotplug support (2018-10-22 20:32:23 +0200) Hi -- I'd appreciate it if you don't send pull-requests for patch series. They show up in my queue of things to merge directly into master otherwise. Just send them as a patch-series email with a cover letter, please. thanks -- PMM
Hi Peter, On Tue, Oct 23, 2018 at 08:01:26PM +0100, Peter Maydell wrote: > On 22 October 2018 at 19:36, Samuel Ortiz <sameo@linux.intel.com> wrote: > > This patch set implements support for the ACPI hardware-reduced > > specification. > > > > 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 rest of the patchset adds the hardware-reduced support on top of > > this code reorganization. Here again, the implementation is machine > > type, platform and architecture independent. > > > > [1] https://github.com/intel/nemu > > > > > > The following changes since commit b312532fd03413d0e6ae6767ec793a3e30f487b8: > > > > Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2018-10-19 19:01:07 +0100) > > > > are available in the Git repository at: > > > > git@github.com:intel/nemu topic/upstream/acpi > > > > for you to fetch changes up to 4133ebe0d638d0a3d583bd89aa2360ef35ba746b: > > > > hw: acpi: reduced: Add PCI hotplug support (2018-10-22 20:32:23 +0200) > > Hi -- I'd appreciate it if you don't send pull-requests for > patch series. They show up in my queue of things to merge directly > into master otherwise. Just send them as a patch-series email with > a cover letter, please. Apologies, I did not know. I'll fix that with the next revision of this patch set. Cheers, Samuel.
On 23 October 2018 at 20:08, Samuel Ortiz <sameo@linux.intel.com> wrote: > Hi Peter, > > On Tue, Oct 23, 2018 at 08:01:26PM +0100, Peter Maydell wrote: >> Hi -- I'd appreciate it if you don't send pull-requests for >> patch series. They show up in my queue of things to merge directly >> into master otherwise. Just send them as a patch-series email with >> a cover letter, please. > Apologies, I did not know. I'll fix that with the next revision of > this patch set. No problem. You can give a git branch URL for reviewers to look at if you think it's helpful; just avoid using the stock git pull-request email wording about 'for you to fetch changes', because that's what my email filters look for :-) thanks -- PMM
On 22/10/2018 20:36, Samuel Ortiz wrote: > This patch set implements support for the ACPI hardware-reduced > specification. > > 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 rest of the patchset adds the hardware-reduced support on top of > this code reorganization. Here again, the implementation is machine > type, platform and architecture independent. > > [1] https://github.com/intel/nemu I think the first part of the series is very valuable, and also the FirmwareBuildMethods idea is good even though I have some comments on how to model it in C. It won't make it in 3.1, but I suppose you expected that. Including the second part in QEMU however would be premature, since it is not used by anything. As mentioned earlier, my suggestion is that you convert the ARM virt machine type to FirmwareBuildMethods and HW-reduced ACPI. It shouldn't be a large amount of work and it should let you reduce the ACPI delta between QEMU and NEMU. Also, I would like to understand if you NEMU people are interested in refactoring the ACPI builder code to move DSDT-building code closer to the device, or perhaps have already done that. For example, the infamous floppy cannot be disabled right now because of the call to isa_fdc_get_drive_type in hw/i386/acpi-build.c. As I understand it, your initial patches have the side effect of letting you bypass this issue---because you don't include hw/i386/acpi-build.c in your virt machine type. That is totally fine, but of course it doesn't help if you want to compile the floppy out of a QEMU that supports -M q35 but not -M pc. Therefore I would love to know that I'm wrong, or just that you're planning to fix that too. :) Paolo
This patch set implements support for the ACPI hardware-reduced specification. 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 rest of the patchset adds the hardware-reduced support on top of this code reorganization. Here again, the implementation is machine type, platform and architecture independent. [1] https://github.com/intel/nemu The following changes since commit b312532fd03413d0e6ae6767ec793a3e30f487b8: Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2018-10-19 19:01:07 +0100) are available in the Git repository at: git@github.com:intel/nemu topic/upstream/acpi for you to fetch changes up to 4133ebe0d638d0a3d583bd89aa2360ef35ba746b: hw: acpi: reduced: Add PCI hotplug support (2018-10-22 20:32:23 +0200) ---------------------------------------------------------------- Jing Liu (2): hw: acpi: reduced: Add shutdown support hw: acpi: reduced: Add reboot support Samuel Ortiz (10): 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: arm: Switch to the AML build RSDP building routine hw: acpi: Do not create hotplug method when handler is not defined hw: i386: Make the hotpluggable memory size property more generic hw: fw-build: Add firmware build methods and state hw: i386: Convert PC machine type to firmware build methods hw: acpi: Initial hardware-reduced support hw: acpi: reduced: Generic Event Device support Sebastien Boeuf (4): hw: acpi: Export the PCI hotplug API hw: acpi: Retrieve the PCI bus from AcpiPciHpState hw: acpi: reduced: Add NFIT support hw: acpi: reduced: Add PCI hotplug support Yang Zhong (10): hw: acpi: Generalize AML build routines hw: acpi: Factorize _OSC AML across architectures hw: i386: Refactor PCI host getter hw: acpi: Export and generalize the PCI host AML API hw: acpi: Export the MCFG getter hw: acpi: Export the SRAT AML build API hw: acpi: Fix memory hotplug AML generation error hw: acpi: reduced: Add MCFG support hw: acpi: reduced: Add memory hotplug support hw: acpi: reduced: Add SRAT table default-configs/i386-softmmu.mak | 1 + hw/acpi/Makefile.objs | 1 + hw/acpi/aml-build.c | 1139 +++++++++++++++++++++++++++++++++++- hw/acpi/cpu.c | 8 +- hw/acpi/cpu_hotplug.c | 9 +- hw/acpi/memory_hotplug.c | 21 +- hw/acpi/pcihp.c | 10 +- hw/acpi/reduced.c | 472 +++++++++++++++ hw/arm/virt-acpi-build.c | 86 +-- hw/i386/acpi-build.c | 1172 +++----------------------------------- hw/i386/acpi-build.h | 4 +- hw/i386/pc.c | 188 +++--- hw/i386/pc_piix.c | 21 +- hw/i386/pc_q35.c | 21 +- hw/pci-host/piix.c | 8 - include/hw/acpi/acpi-defs.h | 15 + include/hw/acpi/acpi.h | 47 ++ include/hw/acpi/aml-build.h | 48 ++ include/hw/acpi/reduced.h | 46 ++ include/hw/boards.h | 5 + include/hw/fw-build.h | 57 ++ include/hw/i386/acpi.h | 27 + include/hw/i386/pc.h | 21 +- include/hw/mem/memory-device.h | 2 + stubs/Makefile.objs | 1 - stubs/pci-host-piix.c | 6 - 26 files changed, 2107 insertions(+), 1329 deletions(-) create mode 100644 hw/acpi/reduced.c create mode 100644 include/hw/acpi/reduced.h create mode 100644 include/hw/fw-build.h create mode 100644 include/hw/i386/acpi.h delete mode 100644 stubs/pci-host-piix.c