| Message ID | 20200318221531.22910-2-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/acpi/piix4: Restrict 'system hotplug' feature to i440fx PC machine | expand |
On 18/03/20 23:15, Philippe Mathieu-Daudé wrote: > The I/O ranges registered by the piix4_acpi_system_hot_add_init() > function are not documented in the PIIX4 datasheet. > This appears to be a PC-only feature added in commit 5e3cb5347e > ("initialize hot add system / acpi gpe") which was then moved > to the PIIX4 device model in commit 9d5e77a22f ("make > qemu_system_device_hot_add piix independent") > Add a property (default enabled, to not modify the current > behavior) to allow machines wanting to model a simple PIIX4 > to disable this feature. Yes, all hotplug stuff (PCI/memory/CPU) are custom additions by QEMU. > + DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState, > + use_acpi_system_hotplug, true), Why not cpu-hotplug-support? Paolo
On 3/19/20 10:36 AM, Paolo Bonzini wrote: > On 18/03/20 23:15, Philippe Mathieu-Daudé wrote: >> The I/O ranges registered by the piix4_acpi_system_hot_add_init() >> function are not documented in the PIIX4 datasheet. >> This appears to be a PC-only feature added in commit 5e3cb5347e >> ("initialize hot add system / acpi gpe") which was then moved >> to the PIIX4 device model in commit 9d5e77a22f ("make >> qemu_system_device_hot_add piix independent") >> Add a property (default enabled, to not modify the current >> behavior) to allow machines wanting to model a simple PIIX4 >> to disable this feature. > > Yes, all hotplug stuff (PCI/memory/CPU) are custom additions by QEMU. > >> + DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState, >> + use_acpi_system_hotplug, true), > > Why not cpu-hotplug-support? Because I have no idea what this code is about, and it seems more than cpu (pci, memory): static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, PCIBus *bus, PIIX4PMState *s) { memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s, "acpi-gpe0", GPE_LEN); memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe); acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent, s->use_acpi_pci_hotplug); s->cpu_hotplug_legacy = true; object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy", piix4_get_cpu_hotplug_legacy, piix4_set_cpu_hotplug_legacy, NULL); legacy_acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu, PIIX4_CPU_HOTPLUG_IO_BASE); if (s->acpi_memory_hotplug.is_enabled) { acpi_memory_hotplug_init(parent, OBJECT(s), &s->acpi_memory_hotplug, ACPI_MEMORY_HOTPLUG_BASE); } }
On 19/03/20 10:42, Philippe Mathieu-Daudé wrote: > On 3/19/20 10:36 AM, Paolo Bonzini wrote: >> On 18/03/20 23:15, Philippe Mathieu-Daudé wrote: >>> The I/O ranges registered by the piix4_acpi_system_hot_add_init() >>> function are not documented in the PIIX4 datasheet. >>> This appears to be a PC-only feature added in commit 5e3cb5347e >>> ("initialize hot add system / acpi gpe") which was then moved >>> to the PIIX4 device model in commit 9d5e77a22f ("make >>> qemu_system_device_hot_add piix independent") >>> Add a property (default enabled, to not modify the current >>> behavior) to allow machines wanting to model a simple PIIX4 >>> to disable this feature. >> >> Yes, all hotplug stuff (PCI/memory/CPU) are custom additions by QEMU. >> >>> + DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState, >>> + use_acpi_system_hotplug, true), >> >> Why not cpu-hotplug-support? > > Because I have no idea what this code is about, and it seems more than > cpu (pci, memory): Right, I should have been more verbose. You mentioned I/O port 0xaf00 which is CPU hotplug. Perhaps unless you can also crash with PCI hotplug (0xae00-0xae0f) it's worth removing only CPU hotplug from MIPS machines, and keep PCI hotplug. Paolo > static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, > PCIBus *bus, PIIX4PMState *s) > { > memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s, > "acpi-gpe0", GPE_LEN); > memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe); > > acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent, > s->use_acpi_pci_hotplug); > > s->cpu_hotplug_legacy = true; > object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy", > piix4_get_cpu_hotplug_legacy, > piix4_set_cpu_hotplug_legacy, > NULL); > legacy_acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu, > PIIX4_CPU_HOTPLUG_IO_BASE); > > if (s->acpi_memory_hotplug.is_enabled) { > acpi_memory_hotplug_init(parent, OBJECT(s), > &s->acpi_memory_hotplug, > ACPI_MEMORY_HOTPLUG_BASE); > } > } >
On Wed, 18 Mar 2020 23:15:30 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > The I/O ranges registered by the piix4_acpi_system_hot_add_init() > function are not documented in the PIIX4 datasheet. > This appears to be a PC-only feature added in commit 5e3cb5347e > ("initialize hot add system / acpi gpe") which was then moved > to the PIIX4 device model in commit 9d5e77a22f ("make > qemu_system_device_hot_add piix independent") > Add a property (default enabled, to not modify the current > behavior) to allow machines wanting to model a simple PIIX4 > to disable this feature. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> it's already pretty twisted code and adding one more knob to workaround other compat knobs makes it worse. Even though it's not really welcomed approach, we can ifdef all hotplug parts and compile them out for mips dropping along the way linking with not needed dependencies or more often used, make stubs from hotplug parts for mips and link with them. > --- > Should I squash this with the next patch and start with > default=false, which is closer to the hardware model? > --- > hw/acpi/piix4.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c > index 964d6f5990..9c970336ac 100644 > --- a/hw/acpi/piix4.c > +++ b/hw/acpi/piix4.c > @@ -78,6 +78,7 @@ typedef struct PIIX4PMState { > > AcpiPciHpState acpi_pci_hotplug; > bool use_acpi_pci_hotplug; > + bool use_acpi_system_hotplug; > > uint8_t disable_s3; > uint8_t disable_s4; > @@ -503,8 +504,10 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp) > s->machine_ready.notify = piix4_pm_machine_ready; > qemu_add_machine_init_done_notifier(&s->machine_ready); > > - piix4_acpi_system_hot_add_init(pci_address_space_io(dev), > - pci_get_bus(dev), s); > + if (s->use_acpi_system_hotplug) { > + piix4_acpi_system_hot_add_init(pci_address_space_io(dev), > + pci_get_bus(dev), s); > + } > qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), OBJECT(s), &error_abort); > > piix4_pm_add_propeties(s); > @@ -635,6 +638,8 @@ static Property piix4_pm_properties[] = { > use_acpi_pci_hotplug, true), > DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState, > acpi_memory_hotplug.is_enabled, true), > + DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState, > + use_acpi_system_hotplug, true), > DEFINE_PROP_END_OF_LIST(), > }; >
On 3/19/20 11:44 AM, Igor Mammedov wrote: > On Wed, 18 Mar 2020 23:15:30 +0100 > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >> The I/O ranges registered by the piix4_acpi_system_hot_add_init() >> function are not documented in the PIIX4 datasheet. >> This appears to be a PC-only feature added in commit 5e3cb5347e >> ("initialize hot add system / acpi gpe") which was then moved >> to the PIIX4 device model in commit 9d5e77a22f ("make >> qemu_system_device_hot_add piix independent") >> Add a property (default enabled, to not modify the current >> behavior) to allow machines wanting to model a simple PIIX4 >> to disable this feature. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > it's already pretty twisted code and adding one more knob > to workaround other compat knobs makes it worse. > > Even though it's not really welcomed approach, > we can ifdef all hotplug parts and compile them out for mips > dropping along the way linking with not needed dependencies We can't use use target-specific poisoned definitions to ifdef out in generic hw/ code. > or > more often used, make stubs from hotplug parts for mips > and link with them. So the problem is this device doesn't match the hardware datasheet, has extra features helping virtualization, and now we can not simplify it due to backward compat. Once Michael said he doesn't care about the PIIX4, only the PIIX3 southbridge [1] [2], but then the i440fx pc machine uses a PIIX3 with a pci PM function from PIIX4, and made that PII4_PM Frankenstein. You are asking me to choose between worse versus ugly? The saner outcome I see is make the current PIIX4_PM x86-specific, not modifying the code, and start a fresh new copy respecting the datasheet. Note I'm not particularly interested in MIPS here, but having model respecting the hardware. [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg504270.html [2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg601512.html > >> --- >> Should I squash this with the next patch and start with >> default=false, which is closer to the hardware model? >> --- >> hw/acpi/piix4.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c >> index 964d6f5990..9c970336ac 100644 >> --- a/hw/acpi/piix4.c >> +++ b/hw/acpi/piix4.c >> @@ -78,6 +78,7 @@ typedef struct PIIX4PMState { >> >> AcpiPciHpState acpi_pci_hotplug; >> bool use_acpi_pci_hotplug; >> + bool use_acpi_system_hotplug; >> >> uint8_t disable_s3; >> uint8_t disable_s4; >> @@ -503,8 +504,10 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp) >> s->machine_ready.notify = piix4_pm_machine_ready; >> qemu_add_machine_init_done_notifier(&s->machine_ready); >> >> - piix4_acpi_system_hot_add_init(pci_address_space_io(dev), >> - pci_get_bus(dev), s); >> + if (s->use_acpi_system_hotplug) { >> + piix4_acpi_system_hot_add_init(pci_address_space_io(dev), >> + pci_get_bus(dev), s); >> + } >> qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), OBJECT(s), &error_abort); >> >> piix4_pm_add_propeties(s); >> @@ -635,6 +638,8 @@ static Property piix4_pm_properties[] = { >> use_acpi_pci_hotplug, true), >> DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState, >> acpi_memory_hotplug.is_enabled, true), >> + DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState, >> + use_acpi_system_hotplug, true), >> DEFINE_PROP_END_OF_LIST(), >> }; >> >
On Thu, 19 Mar 2020 12:04:11 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 3/19/20 11:44 AM, Igor Mammedov wrote: > > On Wed, 18 Mar 2020 23:15:30 +0100 > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > >> The I/O ranges registered by the piix4_acpi_system_hot_add_init() > >> function are not documented in the PIIX4 datasheet. > >> This appears to be a PC-only feature added in commit 5e3cb5347e > >> ("initialize hot add system / acpi gpe") which was then moved > >> to the PIIX4 device model in commit 9d5e77a22f ("make > >> qemu_system_device_hot_add piix independent") > >> Add a property (default enabled, to not modify the current > >> behavior) to allow machines wanting to model a simple PIIX4 > >> to disable this feature. > >> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > > it's already pretty twisted code and adding one more knob > > to workaround other compat knobs makes it worse. > > > > Even though it's not really welcomed approach, > > we can ifdef all hotplug parts and compile them out for mips > > dropping along the way linking with not needed dependencies > > We can't use use target-specific poisoned definitions to ifdef out in > generic hw/ code. > > > or > > more often used, make stubs from hotplug parts for mips > > and link with them. > > So the problem is this device doesn't match the hardware datasheet, has > extra features helping virtualization, and now we can not simplify it > due to backward compat. > > Once Michael said he doesn't care about the PIIX4, only the PIIX3 > southbridge [1] [2], but then the i440fx pc machine uses a PIIX3 with a > pci PM function from PIIX4, and made that PII4_PM Frankenstein. > > You are asking me to choose between worse versus ugly? That 'ugly' is typically used within QEMU to deal with such things probably due to its low complexity. > The saner outcome I see is make the current PIIX4_PM x86-specific, not > modifying the code, and start a fresh new copy respecting the datasheet. properly implementing spec would be quite a task (although if motivation is just for fun, then why not) > > Note I'm not particularly interested in MIPS here, but having model > respecting the hardware. > > [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg504270.html > [2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg601512.html > > > > >> --- > >> Should I squash this with the next patch and start with > >> default=false, which is closer to the hardware model? > >> --- > >> hw/acpi/piix4.c | 9 +++++++-- > >> 1 file changed, 7 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c > >> index 964d6f5990..9c970336ac 100644 > >> --- a/hw/acpi/piix4.c > >> +++ b/hw/acpi/piix4.c > >> @@ -78,6 +78,7 @@ typedef struct PIIX4PMState { > >> > >> AcpiPciHpState acpi_pci_hotplug; > >> bool use_acpi_pci_hotplug; > >> + bool use_acpi_system_hotplug; > >> > >> uint8_t disable_s3; > >> uint8_t disable_s4; > >> @@ -503,8 +504,10 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp) > >> s->machine_ready.notify = piix4_pm_machine_ready; > >> qemu_add_machine_init_done_notifier(&s->machine_ready); > >> > >> - piix4_acpi_system_hot_add_init(pci_address_space_io(dev), > >> - pci_get_bus(dev), s); > >> + if (s->use_acpi_system_hotplug) { > >> + piix4_acpi_system_hot_add_init(pci_address_space_io(dev), > >> + pci_get_bus(dev), s); > >> + } > >> qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), OBJECT(s), &error_abort); > >> > >> piix4_pm_add_propeties(s); > >> @@ -635,6 +638,8 @@ static Property piix4_pm_properties[] = { > >> use_acpi_pci_hotplug, true), > >> DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState, > >> acpi_memory_hotplug.is_enabled, true), > >> + DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState, > >> + use_acpi_system_hotplug, true), > >> DEFINE_PROP_END_OF_LIST(), > >> }; > >> > > > >
On 3/19/20 4:08 PM, Igor Mammedov wrote: > On Thu, 19 Mar 2020 12:04:11 +0100 > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >> On 3/19/20 11:44 AM, Igor Mammedov wrote: >>> On Wed, 18 Mar 2020 23:15:30 +0100 >>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >>> >>>> The I/O ranges registered by the piix4_acpi_system_hot_add_init() >>>> function are not documented in the PIIX4 datasheet. >>>> This appears to be a PC-only feature added in commit 5e3cb5347e >>>> ("initialize hot add system / acpi gpe") which was then moved >>>> to the PIIX4 device model in commit 9d5e77a22f ("make >>>> qemu_system_device_hot_add piix independent") >>>> Add a property (default enabled, to not modify the current >>>> behavior) to allow machines wanting to model a simple PIIX4 >>>> to disable this feature. >>>> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> >>> it's already pretty twisted code and adding one more knob >>> to workaround other compat knobs makes it worse. >>> >>> Even though it's not really welcomed approach, >>> we can ifdef all hotplug parts and compile them out for mips >>> dropping along the way linking with not needed dependencies >> >> We can't use use target-specific poisoned definitions to ifdef out in >> generic hw/ code. >> >>> or >>> more often used, make stubs from hotplug parts for mips >>> and link with them. >> >> So the problem is this device doesn't match the hardware datasheet, has >> extra features helping virtualization, and now we can not simplify it >> due to backward compat. >> >> Once Michael said he doesn't care about the PIIX4, only the PIIX3 >> southbridge [1] [2], but then the i440fx pc machine uses a PIIX3 with a >> pci PM function from PIIX4, and made that PII4_PM Frankenstein. >> >> You are asking me to choose between worse versus ugly? > That 'ugly' is typically used within QEMU to deal with such things > probably due to its low complexity. OK. Can you point me to the documentation for this feature? I can find reference of GPE in the ICH9, but I can't find where this IO address on the PIIX4 comes from: #define GPE_BASE 0xafe0 > >> The saner outcome I see is make the current PIIX4_PM x86-specific, not >> modifying the code, and start a fresh new copy respecting the datasheet. > properly implementing spec would be quite a task > (although if motivation is just for fun, then why not) Is not for fun. > >> >> Note I'm not particularly interested in MIPS here, but having model >> respecting the hardware. >> >> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg504270.html >> [2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg601512.html >> >>> >>>> --- >>>> Should I squash this with the next patch and start with >>>> default=false, which is closer to the hardware model? >>>> --- >>>> hw/acpi/piix4.c | 9 +++++++-- >>>> 1 file changed, 7 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c >>>> index 964d6f5990..9c970336ac 100644 >>>> --- a/hw/acpi/piix4.c >>>> +++ b/hw/acpi/piix4.c >>>> @@ -78,6 +78,7 @@ typedef struct PIIX4PMState { >>>> >>>> AcpiPciHpState acpi_pci_hotplug; >>>> bool use_acpi_pci_hotplug; >>>> + bool use_acpi_system_hotplug; >>>> >>>> uint8_t disable_s3; >>>> uint8_t disable_s4; >>>> @@ -503,8 +504,10 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp) >>>> s->machine_ready.notify = piix4_pm_machine_ready; >>>> qemu_add_machine_init_done_notifier(&s->machine_ready); >>>> >>>> - piix4_acpi_system_hot_add_init(pci_address_space_io(dev), >>>> - pci_get_bus(dev), s); >>>> + if (s->use_acpi_system_hotplug) { >>>> + piix4_acpi_system_hot_add_init(pci_address_space_io(dev), >>>> + pci_get_bus(dev), s); >>>> + } >>>> qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), OBJECT(s), &error_abort); >>>> >>>> piix4_pm_add_propeties(s); >>>> @@ -635,6 +638,8 @@ static Property piix4_pm_properties[] = { >>>> use_acpi_pci_hotplug, true), >>>> DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState, >>>> acpi_memory_hotplug.is_enabled, true), >>>> + DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState, >>>> + use_acpi_system_hotplug, true), >>>> DEFINE_PROP_END_OF_LIST(), >>>> }; >>>> >>> >> >> >
On 22/03/20 17:27, Philippe Mathieu-Daudé wrote: >>> >> That 'ugly' is typically used within QEMU to deal with such things >> probably due to its low complexity. > > OK. Can you point me to the documentation for this feature? I can find > reference of GPE in the ICH9, but I can't find where this IO address on > the PIIX4 comes from: > > #define GPE_BASE 0xafe0 It's made up. The implementation is placed in PIIX4_PM because it is referenced by the ACPI tables. Real hardware would probably place this in the ACPI embedded controller or in the BMC. Paolo
On Mon, Mar 23, 2020 at 09:05:06AM +0100, Paolo Bonzini wrote: > On 22/03/20 17:27, Philippe Mathieu-Daudé wrote: > >>> > >> That 'ugly' is typically used within QEMU to deal with such things > >> probably due to its low complexity. > > > > OK. Can you point me to the documentation for this feature? I can find > > reference of GPE in the ICH9, but I can't find where this IO address on > > the PIIX4 comes from: > > > > #define GPE_BASE 0xafe0 > > It's made up. The implementation is placed in PIIX4_PM because it is > referenced by the ACPI tables. Real hardware would probably place this > in the ACPI embedded controller or in the BMC. > > Paolo Yes, there was nothing at 0xafe0 at the time ACPI support was written.
Hi Igor, Paolo. On 3/23/20 12:04 PM, Marcelo Tosatti wrote: > On Mon, Mar 23, 2020 at 09:05:06AM +0100, Paolo Bonzini wrote: >> On 22/03/20 17:27, Philippe Mathieu-Daudé wrote: >>>>> >>>> That 'ugly' is typically used within QEMU to deal with such things >>>> probably due to its low complexity. >>> >>> OK. Can you point me to the documentation for this feature? I can find >>> reference of GPE in the ICH9, but I can't find where this IO address on >>> the PIIX4 comes from: >>> >>> #define GPE_BASE 0xafe0 >> >> It's made up. The implementation is placed in PIIX4_PM because it is >> referenced by the ACPI tables. Real hardware would probably place this >> in the ACPI embedded controller or in the BMC. >> >> Paolo > > Yes, there was nothing at 0xafe0 at the time ACPI support was written. > Igor earlier said: "it's already pretty twisted code and adding one more knob to workaround other compat knobs makes it worse." Is that OK to rename this file "hw/acpi/piix4_twisted.c" and copy/paste the same content to "hw/acpi/piix4.c" but remove the non-PIIX4 code (GPE from ICH9)? This seems counterproductive from a maintenance PoV, but the PIIX4 bug (https://bugs.launchpad.net/qemu/+bug/1835865) is more than 1 year old now... If someone has a clever idea, I'm open to listen and implement it, but keeping ignoring this issue is not good. Note there is a similar issue with the LPC bus not existing on the PIIX, so maybe renaming this to something like "piix_virt.c" and having someone writing the specs (or differences with the physical datasheet) is not a such bad idea. Thanks, Phil.
On Mon, Aug 03, 2020 at 07:10:11PM +0200, Philippe Mathieu-Daudé wrote: > Hi Igor, Paolo. > > On 3/23/20 12:04 PM, Marcelo Tosatti wrote: > > On Mon, Mar 23, 2020 at 09:05:06AM +0100, Paolo Bonzini wrote: > >> On 22/03/20 17:27, Philippe Mathieu-Daudé wrote: > >>>>> > >>>> That 'ugly' is typically used within QEMU to deal with such things > >>>> probably due to its low complexity. > >>> > >>> OK. Can you point me to the documentation for this feature? I can find > >>> reference of GPE in the ICH9, but I can't find where this IO address on > >>> the PIIX4 comes from: > >>> > >>> #define GPE_BASE 0xafe0 > >> > >> It's made up. The implementation is placed in PIIX4_PM because it is > >> referenced by the ACPI tables. Real hardware would probably place this > >> in the ACPI embedded controller or in the BMC. > >> > >> Paolo > > > > Yes, there was nothing at 0xafe0 at the time ACPI support was written. > > > > Igor earlier said: > "it's already pretty twisted code and adding one more knob > to workaround other compat knobs makes it worse." > > Is that OK to rename this file "hw/acpi/piix4_twisted.c" and > copy/paste the same content to "hw/acpi/piix4.c" but remove the > non-PIIX4 code (GPE from ICH9)? > > This seems counterproductive from a maintenance PoV, but the PIIX4 bug > (https://bugs.launchpad.net/qemu/+bug/1835865) is more than 1 year old > now... > > If someone has a clever idea, I'm open to listen and implement it, but > keeping ignoring this issue is not good. > > Note there is a similar issue with the LPC bus not existing on the > PIIX, so maybe renaming this to something like "piix_virt.c" and having > someone writing the specs (or differences with the physical datasheet) > is not a such bad idea. > > Thanks, > > Phil. Make the port address architecture specific ?
On 8/3/20 7:33 PM, Marcelo Tosatti wrote: > On Mon, Aug 03, 2020 at 07:10:11PM +0200, Philippe Mathieu-Daudé wrote: >> Hi Igor, Paolo. >> >> On 3/23/20 12:04 PM, Marcelo Tosatti wrote: >>> On Mon, Mar 23, 2020 at 09:05:06AM +0100, Paolo Bonzini wrote: >>>> On 22/03/20 17:27, Philippe Mathieu-Daudé wrote: >>>>>>> >>>>>> That 'ugly' is typically used within QEMU to deal with such things >>>>>> probably due to its low complexity. >>>>> >>>>> OK. Can you point me to the documentation for this feature? I can find >>>>> reference of GPE in the ICH9, but I can't find where this IO address on >>>>> the PIIX4 comes from: >>>>> >>>>> #define GPE_BASE 0xafe0 >>>> >>>> It's made up. The implementation is placed in PIIX4_PM because it is >>>> referenced by the ACPI tables. Real hardware would probably place this >>>> in the ACPI embedded controller or in the BMC. >>>> >>>> Paolo >>> >>> Yes, there was nothing at 0xafe0 at the time ACPI support was written. >>> >> >> Igor earlier said: >> "it's already pretty twisted code and adding one more knob >> to workaround other compat knobs makes it worse." >> >> Is that OK to rename this file "hw/acpi/piix4_twisted.c" and >> copy/paste the same content to "hw/acpi/piix4.c" but remove the >> non-PIIX4 code (GPE from ICH9)? >> >> This seems counterproductive from a maintenance PoV, but the PIIX4 bug >> (https://bugs.launchpad.net/qemu/+bug/1835865) is more than 1 year old >> now... >> >> If someone has a clever idea, I'm open to listen and implement it, but >> keeping ignoring this issue is not good. >> >> Note there is a similar issue with the LPC bus not existing on the >> PIIX, so maybe renaming this to something like "piix_virt.c" and having >> someone writing the specs (or differences with the physical datasheet) >> is not a such bad idea. >> >> Thanks, >> >> Phil. > > Make the port address architecture specific ? I find it worse than using a property set on the PC machine only (that would make the piix4 compiled for each target instead on only once in common-obj as now). But if Igor is OK with that, I don't mind much, let's move forward. Thanks, Phil.
On 3/19/20 11:02 AM, Paolo Bonzini wrote: > On 19/03/20 10:42, Philippe Mathieu-Daudé wrote: >> On 3/19/20 10:36 AM, Paolo Bonzini wrote: >>> On 18/03/20 23:15, Philippe Mathieu-Daudé wrote: >>>> The I/O ranges registered by the piix4_acpi_system_hot_add_init() >>>> function are not documented in the PIIX4 datasheet. >>>> This appears to be a PC-only feature added in commit 5e3cb5347e >>>> ("initialize hot add system / acpi gpe") which was then moved >>>> to the PIIX4 device model in commit 9d5e77a22f ("make >>>> qemu_system_device_hot_add piix independent") >>>> Add a property (default enabled, to not modify the current >>>> behavior) to allow machines wanting to model a simple PIIX4 >>>> to disable this feature. >>> >>> Yes, all hotplug stuff (PCI/memory/CPU) are custom additions by QEMU. >>> >>>> + DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState, >>>> + use_acpi_system_hotplug, true), >>> >>> Why not cpu-hotplug-support? >> >> Because I have no idea what this code is about, and it seems more than >> cpu (pci, memory): > > Right, I should have been more verbose. You mentioned I/O port 0xaf00 > which is CPU hotplug. Perhaps unless you can also crash with PCI > hotplug (0xae00-0xae0f) it's worth removing only CPU hotplug from MIPS > machines, and keep PCI hotplug. I am sorry I don't understand what PCI hotplug has to do with PIIX which is a PCI-slave southbridge... If MIPS or other arch is interested in PCI hotplug feature, that would be managed by the northbridge or another PCI bridge. > > Paolo > >> static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, >> PCIBus *bus, PIIX4PMState *s) >> { >> memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s, >> "acpi-gpe0", GPE_LEN); >> memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe); >> >> acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent, >> s->use_acpi_pci_hotplug); >> >> s->cpu_hotplug_legacy = true; >> object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy", >> piix4_get_cpu_hotplug_legacy, >> piix4_set_cpu_hotplug_legacy, >> NULL); >> legacy_acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu, >> PIIX4_CPU_HOTPLUG_IO_BASE); >> >> if (s->acpi_memory_hotplug.is_enabled) { >> acpi_memory_hotplug_init(parent, OBJECT(s), >> &s->acpi_memory_hotplug, >> ACPI_MEMORY_HOTPLUG_BASE); >> } >> } >> >
On 8/5/20 7:56 AM, Philippe Mathieu-Daudé wrote: > On 3/19/20 11:02 AM, Paolo Bonzini wrote: >> On 19/03/20 10:42, Philippe Mathieu-Daudé wrote: >>> On 3/19/20 10:36 AM, Paolo Bonzini wrote: >>>> On 18/03/20 23:15, Philippe Mathieu-Daudé wrote: >>>>> The I/O ranges registered by the piix4_acpi_system_hot_add_init() >>>>> function are not documented in the PIIX4 datasheet. >>>>> This appears to be a PC-only feature added in commit 5e3cb5347e >>>>> ("initialize hot add system / acpi gpe") which was then moved >>>>> to the PIIX4 device model in commit 9d5e77a22f ("make >>>>> qemu_system_device_hot_add piix independent") >>>>> Add a property (default enabled, to not modify the current >>>>> behavior) to allow machines wanting to model a simple PIIX4 >>>>> to disable this feature. >>>> >>>> Yes, all hotplug stuff (PCI/memory/CPU) are custom additions by QEMU. >>>> >>>>> + DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState, >>>>> + use_acpi_system_hotplug, true), >>>> >>>> Why not cpu-hotplug-support? >>> >>> Because I have no idea what this code is about, and it seems more than >>> cpu (pci, memory): >> >> Right, I should have been more verbose. You mentioned I/O port 0xaf00 >> which is CPU hotplug. Perhaps unless you can also crash with PCI >> hotplug (0xae00-0xae0f) it's worth removing only CPU hotplug from MIPS >> machines, and keep PCI hotplug. > > I am sorry I don't understand what PCI hotplug has to do with PIIX which > is a PCI-slave southbridge... If MIPS or other arch is interested in PCI > hotplug feature, that would be managed by the northbridge or another PCI > bridge. Ah, writing that comment made me realize the problem might be these 'virtualization' features have been implemented in the wrong place. Maybe the less disruptive path is to move them to the i440fx northbridge. That way we shouldn't need to maintain a piix_hw.c and piix_virt_twisted.c (child of piix_hw overloaded with hotplug features). I haven't looked at the history yet, maybe the problem happened when i440fx/piix was split. > >> >> Paolo >> >>> static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, >>> PCIBus *bus, PIIX4PMState *s) >>> { >>> memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s, >>> "acpi-gpe0", GPE_LEN); >>> memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe); >>> >>> acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent, >>> s->use_acpi_pci_hotplug); >>> >>> s->cpu_hotplug_legacy = true; >>> object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy", >>> piix4_get_cpu_hotplug_legacy, >>> piix4_set_cpu_hotplug_legacy, >>> NULL); >>> legacy_acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu, >>> PIIX4_CPU_HOTPLUG_IO_BASE); >>> >>> if (s->acpi_memory_hotplug.is_enabled) { >>> acpi_memory_hotplug_init(parent, OBJECT(s), >>> &s->acpi_memory_hotplug, >>> ACPI_MEMORY_HOTPLUG_BASE); >>> } >>> } >>> >> >
On Wed, 5 Aug 2020 08:01:24 +0200 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 8/5/20 7:56 AM, Philippe Mathieu-Daudé wrote: > > On 3/19/20 11:02 AM, Paolo Bonzini wrote: > >> On 19/03/20 10:42, Philippe Mathieu-Daudé wrote: > >>> On 3/19/20 10:36 AM, Paolo Bonzini wrote: > >>>> On 18/03/20 23:15, Philippe Mathieu-Daudé wrote: > >>>>> The I/O ranges registered by the piix4_acpi_system_hot_add_init() > >>>>> function are not documented in the PIIX4 datasheet. > >>>>> This appears to be a PC-only feature added in commit 5e3cb5347e > >>>>> ("initialize hot add system / acpi gpe") which was then moved > >>>>> to the PIIX4 device model in commit 9d5e77a22f ("make > >>>>> qemu_system_device_hot_add piix independent") > >>>>> Add a property (default enabled, to not modify the current > >>>>> behavior) to allow machines wanting to model a simple PIIX4 > >>>>> to disable this feature. > >>>> > >>>> Yes, all hotplug stuff (PCI/memory/CPU) are custom additions by QEMU. > >>>> > >>>>> + DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState, > >>>>> + use_acpi_system_hotplug, true), > >>>> > >>>> Why not cpu-hotplug-support? > >>> > >>> Because I have no idea what this code is about, and it seems more than > >>> cpu (pci, memory): > >> > >> Right, I should have been more verbose. You mentioned I/O port 0xaf00 > >> which is CPU hotplug. Perhaps unless you can also crash with PCI > >> hotplug (0xae00-0xae0f) it's worth removing only CPU hotplug from MIPS > >> machines, and keep PCI hotplug. > > > > I am sorry I don't understand what PCI hotplug has to do with PIIX which > > is a PCI-slave southbridge... If MIPS or other arch is interested in PCI > > hotplug feature, that would be managed by the northbridge or another PCI > > bridge. > > Ah, writing that comment made me realize the problem might be these > 'virtualization' features have been implemented in the wrong place. > Maybe the less disruptive path is to move them to the i440fx > northbridge. not sure if this option is on the table atm. You will need a means to remap migration state to another device to keep migration working. >That way we shouldn't need to maintain a piix_hw.c and > piix_virt_twisted.c (child of piix_hw overloaded with hotplug features). that's path I'd consider, i.e. split out virt parts out from piix hw and make virt child that will have our hacks on top of native piix. Still, You will need to keep typenames on virt part as it's now for not to break migration stream (but I'm not sure, CCing David). > > I haven't looked at the history yet, maybe the problem happened when > i440fx/piix was split. My guess would be due piix_pm having ACPI hw in spec (like sci/pm) so adding other related ACPI hw was considered as the least disruptive back then. > > > > >> > >> Paolo > >> > >>> static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, > >>> PCIBus *bus, PIIX4PMState *s) > >>> { > >>> memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s, > >>> "acpi-gpe0", GPE_LEN); > >>> memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe); > >>> > >>> acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent, > >>> s->use_acpi_pci_hotplug); > >>> > >>> s->cpu_hotplug_legacy = true; > >>> object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy", > >>> piix4_get_cpu_hotplug_legacy, > >>> piix4_set_cpu_hotplug_legacy, > >>> NULL); > >>> legacy_acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu, > >>> PIIX4_CPU_HOTPLUG_IO_BASE); > >>> > >>> if (s->acpi_memory_hotplug.is_enabled) { > >>> acpi_memory_hotplug_init(parent, OBJECT(s), > >>> &s->acpi_memory_hotplug, > >>> ACPI_MEMORY_HOTPLUG_BASE); > >>> } > >>> } > >>> > >> > > >
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c index 964d6f5990..9c970336ac 100644 --- a/hw/acpi/piix4.c +++ b/hw/acpi/piix4.c @@ -78,6 +78,7 @@ typedef struct PIIX4PMState { AcpiPciHpState acpi_pci_hotplug; bool use_acpi_pci_hotplug; + bool use_acpi_system_hotplug; uint8_t disable_s3; uint8_t disable_s4; @@ -503,8 +504,10 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp) s->machine_ready.notify = piix4_pm_machine_ready; qemu_add_machine_init_done_notifier(&s->machine_ready); - piix4_acpi_system_hot_add_init(pci_address_space_io(dev), - pci_get_bus(dev), s); + if (s->use_acpi_system_hotplug) { + piix4_acpi_system_hot_add_init(pci_address_space_io(dev), + pci_get_bus(dev), s); + } qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), OBJECT(s), &error_abort); piix4_pm_add_propeties(s); @@ -635,6 +638,8 @@ static Property piix4_pm_properties[] = { use_acpi_pci_hotplug, true), DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState, acpi_memory_hotplug.is_enabled, true), + DEFINE_PROP_BOOL("system-hotplug-support", PIIX4PMState, + use_acpi_system_hotplug, true), DEFINE_PROP_END_OF_LIST(), };
The I/O ranges registered by the piix4_acpi_system_hot_add_init() function are not documented in the PIIX4 datasheet. This appears to be a PC-only feature added in commit 5e3cb5347e ("initialize hot add system / acpi gpe") which was then moved to the PIIX4 device model in commit 9d5e77a22f ("make qemu_system_device_hot_add piix independent") Add a property (default enabled, to not modify the current behavior) to allow machines wanting to model a simple PIIX4 to disable this feature. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- Should I squash this with the next patch and start with default=false, which is closer to the hardware model? --- hw/acpi/piix4.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)