| Message ID | 20200924070013.165026-4-jusual@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Use ACPI PCI hot-plug for Q35 | expand |
On Thu, Sep 24, 2020 at 09:00:09AM +0200, Julia Suvorova wrote: > Instead of changing the hot-plug type in _OSC register, do not > initialize the slot capability or set the 'Slot Implemented' flag. > This way guest will choose ACPI hot-plug if it is preferred and leave > the option to use SHPC with pcie-pci-bridge. > > Signed-off-by: Julia Suvorova <jusual@redhat.com> > --- > hw/i386/acpi-build.h | 2 ++ > hw/i386/acpi-build.c | 2 +- > hw/pci/pcie.c | 16 ++++++++++++++++ > 3 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h > index 487ec7710f..4c5bfb3d0b 100644 > --- a/hw/i386/acpi-build.h > +++ b/hw/i386/acpi-build.h > @@ -11,4 +11,6 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio; > > void acpi_setup(void); > > +Object *object_resolve_type_unambiguous(const char *typename); > + > #endif > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index cf503b16af..b7811a8912 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -174,7 +174,7 @@ static void init_common_fadt_data(MachineState *ms, Object *o, > *data = fadt; > } > > -static Object *object_resolve_type_unambiguous(const char *typename) > +Object *object_resolve_type_unambiguous(const char *typename) > { > bool ambig; > Object *o = object_resolve_path_type("", typename, &ambig); > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index 5b48bae0f6..c1a082e8b9 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -27,6 +27,8 @@ > #include "hw/pci/pci_bus.h" > #include "hw/pci/pcie_regs.h" > #include "hw/pci/pcie_port.h" > +#include "hw/i386/ich9.h" > +#include "hw/i386/acpi-build.h" > #include "qemu/range.h" > > //#define DEBUG_PCIE Not really happy with pcie.c getting an i386 dependency. > @@ -515,12 +517,26 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev, > pcie_cap_slot_push_attention_button(hotplug_pdev); > } > > +static bool acpi_pcihp_enabled(void) > +{ > + Object *lpc = object_resolve_type_unambiguous(TYPE_ICH9_LPC_DEVICE); > + > + return lpc && > + object_property_get_bool(lpc, "acpi-pci-hotplug-with-bridge-support", > + NULL); > + > +} > + Why not just check the property unconditionally? > /* pci express slot for pci express root/downstream port > PCI express capability slot registers */ > void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s) > { > uint32_t pos = dev->exp.exp_cap; > > + if (acpi_pcihp_enabled()) { > + return; > + } > + I think I would rather not teach pcie about acpi. How about we change the polarity, name the property "pci-native-hotplug" or whatever makes sense. > pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_FLAGS, > PCI_EXP_FLAGS_SLOT); > > -- > 2.25.4
On Thu, 24 Sep 2020 09:00:09 +0200 Julia Suvorova <jusual@redhat.com> wrote: > Instead of changing the hot-plug type in _OSC register, do not > initialize the slot capability or set the 'Slot Implemented' flag. > This way guest will choose ACPI hot-plug if it is preferred and leave > the option to use SHPC with pcie-pci-bridge. > > Signed-off-by: Julia Suvorova <jusual@redhat.com> > --- > hw/i386/acpi-build.h | 2 ++ > hw/i386/acpi-build.c | 2 +- > hw/pci/pcie.c | 16 ++++++++++++++++ > 3 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h > index 487ec7710f..4c5bfb3d0b 100644 > --- a/hw/i386/acpi-build.h > +++ b/hw/i386/acpi-build.h > @@ -11,4 +11,6 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio; > > void acpi_setup(void); > > +Object *object_resolve_type_unambiguous(const char *typename); > + > #endif > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index cf503b16af..b7811a8912 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -174,7 +174,7 @@ static void init_common_fadt_data(MachineState *ms, Object *o, > *data = fadt; > } > > -static Object *object_resolve_type_unambiguous(const char *typename) > +Object *object_resolve_type_unambiguous(const char *typename) > { > bool ambig; > Object *o = object_resolve_path_type("", typename, &ambig); > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index 5b48bae0f6..c1a082e8b9 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -27,6 +27,8 @@ > #include "hw/pci/pci_bus.h" > #include "hw/pci/pcie_regs.h" > #include "hw/pci/pcie_port.h" > +#include "hw/i386/ich9.h" > +#include "hw/i386/acpi-build.h" > #include "qemu/range.h" > > //#define DEBUG_PCIE > @@ -515,12 +517,26 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev, > pcie_cap_slot_push_attention_button(hotplug_pdev); > } > > +static bool acpi_pcihp_enabled(void) > +{ > + Object *lpc = object_resolve_type_unambiguous(TYPE_ICH9_LPC_DEVICE); > + > + return lpc && > + object_property_get_bool(lpc, "acpi-pci-hotplug-with-bridge-support", > + NULL); > + > +} > + > /* pci express slot for pci express root/downstream port > PCI express capability slot registers */ > void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s) > { > uint32_t pos = dev->exp.exp_cap; > > + if (acpi_pcihp_enabled()) { > + return; > + } > + > pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_FLAGS, > PCI_EXP_FLAGS_SLOT); > why do you drop all slot caps instead of disabling just "if (s->hotplug) {" branch?
On Thu, 24 Sep 2020, Igor Mammedov wrote: > On Thu, 24 Sep 2020 09:00:09 +0200 > Julia Suvorova <jusual@redhat.com> wrote: > >> Instead of changing the hot-plug type in _OSC register, do not >> initialize the slot capability or set the 'Slot Implemented' flag. >> This way guest will choose ACPI hot-plug if it is preferred and leave >> the option to use SHPC with pcie-pci-bridge. >> >> Signed-off-by: Julia Suvorova <jusual@redhat.com> >> --- >> hw/i386/acpi-build.h | 2 ++ >> hw/i386/acpi-build.c | 2 +- >> hw/pci/pcie.c | 16 ++++++++++++++++ >> 3 files changed, 19 insertions(+), 1 deletion(-) >> >> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h >> index 487ec7710f..4c5bfb3d0b 100644 >> --- a/hw/i386/acpi-build.h >> +++ b/hw/i386/acpi-build.h >> @@ -11,4 +11,6 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio; >> >> void acpi_setup(void); >> >> +Object *object_resolve_type_unambiguous(const char *typename); >> + >> #endif >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> index cf503b16af..b7811a8912 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -174,7 +174,7 @@ static void init_common_fadt_data(MachineState *ms, Object *o, >> *data = fadt; >> } >> >> -static Object *object_resolve_type_unambiguous(const char *typename) >> +Object *object_resolve_type_unambiguous(const char *typename) >> { >> bool ambig; >> Object *o = object_resolve_path_type("", typename, &ambig); >> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >> index 5b48bae0f6..c1a082e8b9 100644 >> --- a/hw/pci/pcie.c >> +++ b/hw/pci/pcie.c >> @@ -27,6 +27,8 @@ >> #include "hw/pci/pci_bus.h" >> #include "hw/pci/pcie_regs.h" >> #include "hw/pci/pcie_port.h" >> +#include "hw/i386/ich9.h" >> +#include "hw/i386/acpi-build.h" >> #include "qemu/range.h" >> >> //#define DEBUG_PCIE >> @@ -515,12 +517,26 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev, >> pcie_cap_slot_push_attention_button(hotplug_pdev); >> } >> >> +static bool acpi_pcihp_enabled(void) >> +{ >> + Object *lpc = object_resolve_type_unambiguous(TYPE_ICH9_LPC_DEVICE); >> + >> + return lpc && >> + object_property_get_bool(lpc, "acpi-pci-hotplug-with-bridge-support", >> + NULL); >> + >> +} >> + >> /* pci express slot for pci express root/downstream port >> PCI express capability slot registers */ >> void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s) >> { >> uint32_t pos = dev->exp.exp_cap; >> >> + if (acpi_pcihp_enabled()) { >> + return; >> + } >> + >> pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_FLAGS, >> PCI_EXP_FLAGS_SLOT); >> > why do you drop all slot caps instead of disabling just "if (s->hotplug) {" branch? +1 to Igor's suggestion. > >
diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h index 487ec7710f..4c5bfb3d0b 100644 --- a/hw/i386/acpi-build.h +++ b/hw/i386/acpi-build.h @@ -11,4 +11,6 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio; void acpi_setup(void); +Object *object_resolve_type_unambiguous(const char *typename); + #endif diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index cf503b16af..b7811a8912 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -174,7 +174,7 @@ static void init_common_fadt_data(MachineState *ms, Object *o, *data = fadt; } -static Object *object_resolve_type_unambiguous(const char *typename) +Object *object_resolve_type_unambiguous(const char *typename) { bool ambig; Object *o = object_resolve_path_type("", typename, &ambig); diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 5b48bae0f6..c1a082e8b9 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -27,6 +27,8 @@ #include "hw/pci/pci_bus.h" #include "hw/pci/pcie_regs.h" #include "hw/pci/pcie_port.h" +#include "hw/i386/ich9.h" +#include "hw/i386/acpi-build.h" #include "qemu/range.h" //#define DEBUG_PCIE @@ -515,12 +517,26 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev, pcie_cap_slot_push_attention_button(hotplug_pdev); } +static bool acpi_pcihp_enabled(void) +{ + Object *lpc = object_resolve_type_unambiguous(TYPE_ICH9_LPC_DEVICE); + + return lpc && + object_property_get_bool(lpc, "acpi-pci-hotplug-with-bridge-support", + NULL); + +} + /* pci express slot for pci express root/downstream port PCI express capability slot registers */ void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s) { uint32_t pos = dev->exp.exp_cap; + if (acpi_pcihp_enabled()) { + return; + } + pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_FLAGS, PCI_EXP_FLAGS_SLOT);
Instead of changing the hot-plug type in _OSC register, do not initialize the slot capability or set the 'Slot Implemented' flag. This way guest will choose ACPI hot-plug if it is preferred and leave the option to use SHPC with pcie-pci-bridge. Signed-off-by: Julia Suvorova <jusual@redhat.com> --- hw/i386/acpi-build.h | 2 ++ hw/i386/acpi-build.c | 2 +- hw/pci/pcie.c | 16 ++++++++++++++++ 3 files changed, 19 insertions(+), 1 deletion(-)