Message ID | fc1a6594429e549af90037e0ba0a256680a95cf6.1687278381.git.jupham125@gmail.com |
---|---|
State | New |
Headers | show |
Series | Q35 support for Xen | expand |
On Tue, 20 Jun 2023 13:24:36 -0400 Joel Upham <jupham125@gmail.com> wrote: > On Q35 we still need to assign BSEL property to bus(es) for PCI device > add/hotplug to work. > Extend acpi_set_pci_info() function to support Q35 as well. This patch adds new (trivial) > function find_q35() which returns root PCIBus object on Q35, in a way > similar to what find_i440fx does. I think patch is mostly obsolete, q35 ACPI PCI hotplug is supported in upstream QEMU. Also see comment below. > > Signed-off-by: Alexey Gerasimenko <x1917x@xxxxxxxxx> > Signed-off-by: Joel Upham <jupham125@gmail.com> > --- > hw/acpi/pcihp.c | 4 +++- > hw/pci-host/q35.c | 9 +++++++++ > include/hw/i386/pc.h | 3 +++ > 3 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > index cdd6f775a1..f4e39d7a9c 100644 > --- a/hw/acpi/pcihp.c > +++ b/hw/acpi/pcihp.c > @@ -40,6 +40,7 @@ > #include "qapi/error.h" > #include "qom/qom-qobject.h" > #include "trace.h" > +#include "sysemu/xen.h" > > #define ACPI_PCIHP_SIZE 0x0018 > #define PCI_UP_BASE 0x0000 > @@ -84,7 +85,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque) > bool is_bridge = IS_PCI_BRIDGE(br); > > /* hotplugged bridges can't be described in ACPI ignore them */ > - if (qbus_is_hotpluggable(BUS(bus))) { > + /* Xen requires hotplugging to the root device, even on the Q35 chipset */ pls explain what 'root device' is. Why can't you use root-ports for hotplug? > + if (qbus_is_hotpluggable(BUS(bus)) || xen_enabled()) { > if (!is_bridge || (!br->hotplugged && info->has_bridge_hotplug)) { > bus_bsel = g_malloc(sizeof *bus_bsel); > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > index fd18920e7f..fe5fc0f47c 100644 > --- a/hw/pci-host/q35.c > +++ b/hw/pci-host/q35.c > @@ -259,6 +259,15 @@ static void q35_host_initfn(Object *obj) > qdev_prop_allow_set_link_before_realize, 0); > } > > +PCIBus *find_q35(void) > +{ > + PCIHostState *s = OBJECT_CHECK(PCIHostState, > + object_resolve_path("/machine/q35", NULL), > + TYPE_PCI_HOST_BRIDGE); > + return s ? s->bus : NULL; > +} > + > + > static const TypeInfo q35_host_info = { > .name = TYPE_Q35_HOST_DEVICE, > .parent = TYPE_PCIE_HOST_BRIDGE, > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index c661e9cc80..550f8fa221 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -196,6 +196,9 @@ void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids, > /* sgx.c */ > void pc_machine_init_sgx_epc(PCMachineState *pcms); > > +/* q35.c */ > +PCIBus *find_q35(void); > + > extern GlobalProperty pc_compat_8_0[]; > extern const size_t pc_compat_8_0_len; >
On Wed, Jun 21, 2023 at 7:28 AM Igor Mammedov <imammedo@redhat.com> wrote: > On Tue, 20 Jun 2023 13:24:36 -0400 > Joel Upham <jupham125@gmail.com> wrote: > > > On Q35 we still need to assign BSEL property to bus(es) for PCI device > > add/hotplug to work. > > Extend acpi_set_pci_info() function to support Q35 as well. This patch > adds new (trivial) > > function find_q35() which returns root PCIBus object on Q35, in a way > > similar to what find_i440fx does. > > I think patch is mostly obsolete, q35 ACPI PCI hotplug is supported in > upstream QEMU. > > Also see comment below. > > I make use of the find_q35() function in later patches, but I agree now a majority of this patch is a bit different. > > > > Signed-off-by: Alexey Gerasimenko <x1917x@xxxxxxxxx> > > Signed-off-by: Joel Upham <jupham125@gmail.com> > > --- > > hw/acpi/pcihp.c | 4 +++- > > hw/pci-host/q35.c | 9 +++++++++ > > include/hw/i386/pc.h | 3 +++ > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > > index cdd6f775a1..f4e39d7a9c 100644 > > --- a/hw/acpi/pcihp.c > > +++ b/hw/acpi/pcihp.c > > @@ -40,6 +40,7 @@ > > #include "qapi/error.h" > > #include "qom/qom-qobject.h" > > #include "trace.h" > > +#include "sysemu/xen.h" > > > > #define ACPI_PCIHP_SIZE 0x0018 > > #define PCI_UP_BASE 0x0000 > > @@ -84,7 +85,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque) > > bool is_bridge = IS_PCI_BRIDGE(br); > > > > /* hotplugged bridges can't be described in ACPI ignore them */ > > - if (qbus_is_hotpluggable(BUS(bus))) { > > > + /* Xen requires hotplugging to the root device, even on the Q35 > chipset */ > pls explain what 'root device' is. > Why can't you use root-ports for hotplug? > > Wording may have been incorrect. Root port is correct. This may not be needed anymore, and may have been left over for when I was debugging PCIe hotplugging problems. I will retest and fix patch once I know more. Xen expects the PCIe device to be on the root port. I can move the function to a different patch that uses it. > > + if (qbus_is_hotpluggable(BUS(bus)) || xen_enabled()) { > > if (!is_bridge || (!br->hotplugged && > info->has_bridge_hotplug)) { > > bus_bsel = g_malloc(sizeof *bus_bsel); > > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > > index fd18920e7f..fe5fc0f47c 100644 > > --- a/hw/pci-host/q35.c > > +++ b/hw/pci-host/q35.c > > @@ -259,6 +259,15 @@ static void q35_host_initfn(Object *obj) > > qdev_prop_allow_set_link_before_realize, > 0); > > } > > > > +PCIBus *find_q35(void) > > +{ > > + PCIHostState *s = OBJECT_CHECK(PCIHostState, > > + object_resolve_path("/machine/q35", > NULL), > > + TYPE_PCI_HOST_BRIDGE); > > + return s ? s->bus : NULL; > > +} > > + > > + > > static const TypeInfo q35_host_info = { > > .name = TYPE_Q35_HOST_DEVICE, > > .parent = TYPE_PCIE_HOST_BRIDGE, > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > > index c661e9cc80..550f8fa221 100644 > > --- a/include/hw/i386/pc.h > > +++ b/include/hw/i386/pc.h > > @@ -196,6 +196,9 @@ void pc_madt_cpu_entry(int uid, const CPUArchIdList > *apic_ids, > > /* sgx.c */ > > void pc_machine_init_sgx_epc(PCMachineState *pcms); > > > > +/* q35.c */ > > +PCIBus *find_q35(void); > > + > > extern GlobalProperty pc_compat_8_0[]; > > extern const size_t pc_compat_8_0_len; > > > >
On Wed, 21 Jun 2023 13:24:42 -0400 Joel Upham <jupham125@gmail.com> wrote: > On Wed, Jun 21, 2023 at 7:28 AM Igor Mammedov <imammedo@redhat.com> wrote: > > > On Tue, 20 Jun 2023 13:24:36 -0400 > > Joel Upham <jupham125@gmail.com> wrote: > > > > > On Q35 we still need to assign BSEL property to bus(es) for PCI device > > > add/hotplug to work. > > > Extend acpi_set_pci_info() function to support Q35 as well. This patch > > adds new (trivial) > > > function find_q35() which returns root PCIBus object on Q35, in a way > > > similar to what find_i440fx does. > > > > I think patch is mostly obsolete, q35 ACPI PCI hotplug is supported in > > upstream QEMU. > > > > Also see comment below. > > > > I make use of the find_q35() function in later patches, but I agree now a > majority of this patch is a bit different. There is likely an existing alternative already. (probably introduced by ACPI PIC hotplug for q35) > > > > > > > Signed-off-by: Alexey Gerasimenko <x1917x@xxxxxxxxx> > > > Signed-off-by: Joel Upham <jupham125@gmail.com> > > > --- > > > hw/acpi/pcihp.c | 4 +++- > > > hw/pci-host/q35.c | 9 +++++++++ > > > include/hw/i386/pc.h | 3 +++ > > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > > > index cdd6f775a1..f4e39d7a9c 100644 > > > --- a/hw/acpi/pcihp.c > > > +++ b/hw/acpi/pcihp.c > > > @@ -40,6 +40,7 @@ > > > #include "qapi/error.h" > > > #include "qom/qom-qobject.h" > > > #include "trace.h" > > > +#include "sysemu/xen.h" > > > > > > #define ACPI_PCIHP_SIZE 0x0018 > > > #define PCI_UP_BASE 0x0000 > > > @@ -84,7 +85,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque) > > > bool is_bridge = IS_PCI_BRIDGE(br); > > > > > > /* hotplugged bridges can't be described in ACPI ignore them */ > > > - if (qbus_is_hotpluggable(BUS(bus))) { > > > > > + /* Xen requires hotplugging to the root device, even on the Q35 > > chipset */ > > pls explain what 'root device' is. > > Why can't you use root-ports for hotplug? > > > > Wording may have been incorrect. Root port is correct. This may not be > needed anymore, > and may have been left over for when I was debugging PCIe hotplugging > problems. > I will retest and fix patch once I know more. Xen expects the PCIe device > to be on the root port. > > I can move the function to a different patch that uses it. > > > > + if (qbus_is_hotpluggable(BUS(bus)) || xen_enabled()) { > > > if (!is_bridge || (!br->hotplugged && > > info->has_bridge_hotplug)) { > > > bus_bsel = g_malloc(sizeof *bus_bsel); > > > > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > > > index fd18920e7f..fe5fc0f47c 100644 > > > --- a/hw/pci-host/q35.c > > > +++ b/hw/pci-host/q35.c > > > @@ -259,6 +259,15 @@ static void q35_host_initfn(Object *obj) > > > qdev_prop_allow_set_link_before_realize, > > 0); > > > } > > > > > > +PCIBus *find_q35(void) > > > +{ > > > + PCIHostState *s = OBJECT_CHECK(PCIHostState, > > > + object_resolve_path("/machine/q35", > > NULL), > > > + TYPE_PCI_HOST_BRIDGE); > > > + return s ? s->bus : NULL; > > > +} > > > + > > > + > > > static const TypeInfo q35_host_info = { > > > .name = TYPE_Q35_HOST_DEVICE, > > > .parent = TYPE_PCIE_HOST_BRIDGE, > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > > > index c661e9cc80..550f8fa221 100644 > > > --- a/include/hw/i386/pc.h > > > +++ b/include/hw/i386/pc.h > > > @@ -196,6 +196,9 @@ void pc_madt_cpu_entry(int uid, const CPUArchIdList > > *apic_ids, > > > /* sgx.c */ > > > void pc_machine_init_sgx_epc(PCMachineState *pcms); > > > > > > +/* q35.c */ > > > +PCIBus *find_q35(void); > > > + > > > extern GlobalProperty pc_compat_8_0[]; > > > extern const size_t pc_compat_8_0_len; > > > > > > >
On Thu, Jun 22, 2023 at 9:36 AM Igor Mammedov <imammedo@redhat.com> wrote: > > On Wed, 21 Jun 2023 13:24:42 -0400 > Joel Upham <jupham125@gmail.com> wrote: > > > On Wed, Jun 21, 2023 at 7:28 AM Igor Mammedov <imammedo@redhat.com> wrote: > > > > > On Tue, 20 Jun 2023 13:24:36 -0400 > > > Joel Upham <jupham125@gmail.com> wrote: > > > > > > > On Q35 we still need to assign BSEL property to bus(es) for PCI device > > > > add/hotplug to work. > > > > Extend acpi_set_pci_info() function to support Q35 as well. This patch > > > adds new (trivial) > > > > function find_q35() which returns root PCIBus object on Q35, in a way > > > > similar to what find_i440fx does. > > > > > > I think patch is mostly obsolete, q35 ACPI PCI hotplug is supported in > > > upstream QEMU. > > > > > > Also see comment below. > > > > > > I make use of the find_q35() function in later patches, but I agree now a > > majority of this patch is a bit different. > > There is likely an existing alternative already. (probably introduced by ACPI PIC hotplug for q35) There is a similar function acpi_get_i386_pci_host() in hw/i386/acpi-build.c Best regards, Julia Suvorova. > > > > > > > > > > Signed-off-by: Alexey Gerasimenko <x1917x@xxxxxxxxx> > > > > Signed-off-by: Joel Upham <jupham125@gmail.com> > > > > --- > > > > hw/acpi/pcihp.c | 4 +++- > > > > hw/pci-host/q35.c | 9 +++++++++ > > > > include/hw/i386/pc.h | 3 +++ > > > > 3 files changed, 15 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > > > > index cdd6f775a1..f4e39d7a9c 100644 > > > > --- a/hw/acpi/pcihp.c > > > > +++ b/hw/acpi/pcihp.c > > > > @@ -40,6 +40,7 @@ > > > > #include "qapi/error.h" > > > > #include "qom/qom-qobject.h" > > > > #include "trace.h" > > > > +#include "sysemu/xen.h" > > > > > > > > #define ACPI_PCIHP_SIZE 0x0018 > > > > #define PCI_UP_BASE 0x0000 > > > > @@ -84,7 +85,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque) > > > > bool is_bridge = IS_PCI_BRIDGE(br); > > > > > > > > /* hotplugged bridges can't be described in ACPI ignore them */ > > > > - if (qbus_is_hotpluggable(BUS(bus))) { > > > > > > > + /* Xen requires hotplugging to the root device, even on the Q35 > > > chipset */ > > > pls explain what 'root device' is. > > > Why can't you use root-ports for hotplug? > > > > > > Wording may have been incorrect. Root port is correct. This may not be > > needed anymore, > > and may have been left over for when I was debugging PCIe hotplugging > > problems. > > I will retest and fix patch once I know more. Xen expects the PCIe device > > to be on the root port. > > > > I can move the function to a different patch that uses it. > > > > > > + if (qbus_is_hotpluggable(BUS(bus)) || xen_enabled()) { > > > > if (!is_bridge || (!br->hotplugged && > > > info->has_bridge_hotplug)) { > > > > bus_bsel = g_malloc(sizeof *bus_bsel); > > > > > > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > > > > index fd18920e7f..fe5fc0f47c 100644 > > > > --- a/hw/pci-host/q35.c > > > > +++ b/hw/pci-host/q35.c > > > > @@ -259,6 +259,15 @@ static void q35_host_initfn(Object *obj) > > > > qdev_prop_allow_set_link_before_realize, > > > 0); > > > > } > > > > > > > > +PCIBus *find_q35(void) > > > > +{ > > > > + PCIHostState *s = OBJECT_CHECK(PCIHostState, > > > > + object_resolve_path("/machine/q35", > > > NULL), > > > > + TYPE_PCI_HOST_BRIDGE); > > > > + return s ? s->bus : NULL; > > > > +} > > > > + > > > > + > > > > static const TypeInfo q35_host_info = { > > > > .name = TYPE_Q35_HOST_DEVICE, > > > > .parent = TYPE_PCIE_HOST_BRIDGE, > > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > > > > index c661e9cc80..550f8fa221 100644 > > > > --- a/include/hw/i386/pc.h > > > > +++ b/include/hw/i386/pc.h > > > > @@ -196,6 +196,9 @@ void pc_madt_cpu_entry(int uid, const CPUArchIdList > > > *apic_ids, > > > > /* sgx.c */ > > > > void pc_machine_init_sgx_epc(PCMachineState *pcms); > > > > > > > > +/* q35.c */ > > > > +PCIBus *find_q35(void); > > > > + > > > > extern GlobalProperty pc_compat_8_0[]; > > > > extern const size_t pc_compat_8_0_len; > > > > > > > > > > > >
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c index cdd6f775a1..f4e39d7a9c 100644 --- a/hw/acpi/pcihp.c +++ b/hw/acpi/pcihp.c @@ -40,6 +40,7 @@ #include "qapi/error.h" #include "qom/qom-qobject.h" #include "trace.h" +#include "sysemu/xen.h" #define ACPI_PCIHP_SIZE 0x0018 #define PCI_UP_BASE 0x0000 @@ -84,7 +85,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque) bool is_bridge = IS_PCI_BRIDGE(br); /* hotplugged bridges can't be described in ACPI ignore them */ - if (qbus_is_hotpluggable(BUS(bus))) { + /* Xen requires hotplugging to the root device, even on the Q35 chipset */ + if (qbus_is_hotpluggable(BUS(bus)) || xen_enabled()) { if (!is_bridge || (!br->hotplugged && info->has_bridge_hotplug)) { bus_bsel = g_malloc(sizeof *bus_bsel); diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c index fd18920e7f..fe5fc0f47c 100644 --- a/hw/pci-host/q35.c +++ b/hw/pci-host/q35.c @@ -259,6 +259,15 @@ static void q35_host_initfn(Object *obj) qdev_prop_allow_set_link_before_realize, 0); } +PCIBus *find_q35(void) +{ + PCIHostState *s = OBJECT_CHECK(PCIHostState, + object_resolve_path("/machine/q35", NULL), + TYPE_PCI_HOST_BRIDGE); + return s ? s->bus : NULL; +} + + static const TypeInfo q35_host_info = { .name = TYPE_Q35_HOST_DEVICE, .parent = TYPE_PCIE_HOST_BRIDGE, diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index c661e9cc80..550f8fa221 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -196,6 +196,9 @@ void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids, /* sgx.c */ void pc_machine_init_sgx_epc(PCMachineState *pcms); +/* q35.c */ +PCIBus *find_q35(void); + extern GlobalProperty pc_compat_8_0[]; extern const size_t pc_compat_8_0_len;