| Message ID | 20170817162347.1590-3-anthony.perard@citrix.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 17, 2017 at 05:23:46PM +0100, Anthony PERARD wrote: > This means that the function will be call and the property > acpi-pcihp-bsel will be set even if ACPI build is disable. > > To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be > set, but this was done only when ACPI tables are built which is not > needed for a Xen guest. The need for the property starts with commit > "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice" > (f0c9d64a68b776374ec4732424a3e27753ce37b6). > > Reported-by: Sander Eikelenboom <linux@eikelenboom.it> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > > --- > Changes in V3: > - move acpi_set_pci_info to pcihp instead > > Changes in V2: > - check for acpi_enabled before calling acpi_set_pci_info. > - set the property on the root bus only. > > This patch would be a canditade to backport to 2.9, along with > "hw/acpi: Limit hotplug to root bus on legacy mode" > > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Bruce Rogers <brogers@suse.com> > --- > hw/acpi/pcihp.c | 31 +++++++++++++++++++++++++++++++ > hw/i386/acpi-build.c | 32 -------------------------------- > 2 files changed, 31 insertions(+), 32 deletions(-) > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > index 9db3c2eaf2..44e8842db8 100644 > --- a/hw/acpi/pcihp.c > +++ b/hw/acpi/pcihp.c > @@ -75,6 +75,36 @@ static int acpi_pcihp_get_bsel(PCIBus *bus) > } > } > > +/* Assign BSEL property to all buses. In the future, this can be changed > + * to only assign to buses that support hotplug. > + */ > +static void *acpi_set_bsel(PCIBus *bus, void *opaque) > +{ > + unsigned *bsel_alloc = opaque; > + unsigned *bus_bsel; > + > + if (qbus_is_hotpluggable(BUS(bus))) { > + bus_bsel = g_malloc(sizeof *bus_bsel); > + > + *bus_bsel = (*bsel_alloc)++; > + object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, > + bus_bsel, &error_abort); > + } > + > + return bsel_alloc; > +} > + > +static void acpi_set_pci_info(void) > +{ > + PCIBus *bus = find_i440fx(); /* TODO: Q35 support */ > + unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT; > + > + if (bus) { > + /* Scan all PCI buses. Set property to enable acpi based hotplug. */ > + pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc); > + } > +} > + > static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque) > { > AcpiPciHpFind *find = opaque; > @@ -177,6 +207,7 @@ static void acpi_pcihp_update(AcpiPciHpState *s) > > void acpi_pcihp_reset(AcpiPciHpState *s) > { > + acpi_set_pci_info(); > acpi_pcihp_update(s); > } IIUC doing this on reset will add property over and over again leaking memory. I think that we need to do it on machine done. Igor, I think reordering acpi-build like earlier version did is less intrusive and more appropriate for 2.10. For 2.10 I would like to see ideally some changes that are all if (xen) making it obvious non xen is not affected. I can then ack it and it will be merged in xen tree. Clean it up after 2.10. > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 98dd424678..4d19d91e1b 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -493,36 +493,6 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms) > table_data->len - madt_start, 1, NULL, NULL); > } > > -/* Assign BSEL property to all buses. In the future, this can be changed > - * to only assign to buses that support hotplug. > - */ > -static void *acpi_set_bsel(PCIBus *bus, void *opaque) > -{ > - unsigned *bsel_alloc = opaque; > - unsigned *bus_bsel; > - > - if (qbus_is_hotpluggable(BUS(bus))) { > - bus_bsel = g_malloc(sizeof *bus_bsel); > - > - *bus_bsel = (*bsel_alloc)++; > - object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, > - bus_bsel, &error_abort); > - } > - > - return bsel_alloc; > -} > - > -static void acpi_set_pci_info(void) > -{ > - PCIBus *bus = find_i440fx(); /* TODO: Q35 support */ > - unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT; > - > - if (bus) { > - /* Scan all PCI buses. Set property to enable acpi based hotplug. */ > - pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc); > - } > -} > - > static void build_append_pcihp_notify_entry(Aml *method, int slot) > { > Aml *if_ctx; > @@ -2888,8 +2858,6 @@ void acpi_setup(void) > > build_state = g_malloc0(sizeof *build_state); > > - acpi_set_pci_info(); > - > acpi_build_tables_init(&tables); > acpi_build(&tables, MACHINE(pcms)); > > -- > Anthony PERARD
On Fri, 18 Aug 2017 04:40:02 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Aug 17, 2017 at 05:23:46PM +0100, Anthony PERARD wrote: > > This means that the function will be call and the property > > acpi-pcihp-bsel will be set even if ACPI build is disable. > > > > To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be > > set, but this was done only when ACPI tables are built which is not > > needed for a Xen guest. The need for the property starts with commit > > "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice" > > (f0c9d64a68b776374ec4732424a3e27753ce37b6). > > > > Reported-by: Sander Eikelenboom <linux@eikelenboom.it> > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > > > > --- > > Changes in V3: > > - move acpi_set_pci_info to pcihp instead > > > > Changes in V2: > > - check for acpi_enabled before calling acpi_set_pci_info. > > - set the property on the root bus only. > > > > This patch would be a canditade to backport to 2.9, along with > > "hw/acpi: Limit hotplug to root bus on legacy mode" > > > > CC: Stefano Stabellini <sstabellini@kernel.org> > > CC: Bruce Rogers <brogers@suse.com> > > --- > > hw/acpi/pcihp.c | 31 +++++++++++++++++++++++++++++++ > > hw/i386/acpi-build.c | 32 -------------------------------- > > 2 files changed, 31 insertions(+), 32 deletions(-) > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > > index 9db3c2eaf2..44e8842db8 100644 > > --- a/hw/acpi/pcihp.c > > +++ b/hw/acpi/pcihp.c > > @@ -75,6 +75,36 @@ static int acpi_pcihp_get_bsel(PCIBus *bus) > > } > > } > > > > +/* Assign BSEL property to all buses. In the future, this can be changed > > + * to only assign to buses that support hotplug. > > + */ > > +static void *acpi_set_bsel(PCIBus *bus, void *opaque) > > +{ > > + unsigned *bsel_alloc = opaque; > > + unsigned *bus_bsel; > > + > > + if (qbus_is_hotpluggable(BUS(bus))) { > > + bus_bsel = g_malloc(sizeof *bus_bsel); > > + > > + *bus_bsel = (*bsel_alloc)++; > > + object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, > > + bus_bsel, &error_abort); > > + } > > + > > + return bsel_alloc; > > +} > > + > > +static void acpi_set_pci_info(void) > > +{ > > + PCIBus *bus = find_i440fx(); /* TODO: Q35 support */ > > + unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT; > > + > > + if (bus) { > > + /* Scan all PCI buses. Set property to enable acpi based hotplug. */ > > + pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc); > > + } > > +} > > + > > static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque) > > { > > AcpiPciHpFind *find = opaque; > > @@ -177,6 +207,7 @@ static void acpi_pcihp_update(AcpiPciHpState *s) > > > > void acpi_pcihp_reset(AcpiPciHpState *s) > > { > > + acpi_set_pci_info(); > > acpi_pcihp_update(s); > > } > > IIUC doing this on reset will add property over and over again leaking > memory. in v2 I've explicitly suggested to call it once, like: acpi_set_pci_info() { static bool bsel_is set; if (bsel_is set) return; bsel_is set = true; ... } not patch related: BTW bsel assignment is not stable in hotplug + migration use case, and we probably should fix it up in 2.11 (CCing Marcel) > I think that we need to do it on machine done. > > Igor, I think reordering acpi-build like earlier version did > is less intrusive and more appropriate for 2.10. > > For 2.10 I would like to see ideally some changes that > are all if (xen) making it obvious non xen is not > affected. I can then ack it and it will be merged in xen tree. it didn't work before so I'd just push fix to 2.11 without intermediate fix. But if you guys think it's worth to fix in 2.10, I'm fine with v2 for it if Anthony will take care of it (rebase this series) in 2.11 merge window. > > Clean it up after 2.10. > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index 98dd424678..4d19d91e1b 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -493,36 +493,6 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms) > > table_data->len - madt_start, 1, NULL, NULL); > > } > > > > -/* Assign BSEL property to all buses. In the future, this can be changed > > - * to only assign to buses that support hotplug. > > - */ > > -static void *acpi_set_bsel(PCIBus *bus, void *opaque) > > -{ > > - unsigned *bsel_alloc = opaque; > > - unsigned *bus_bsel; > > - > > - if (qbus_is_hotpluggable(BUS(bus))) { > > - bus_bsel = g_malloc(sizeof *bus_bsel); > > - > > - *bus_bsel = (*bsel_alloc)++; > > - object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, > > - bus_bsel, &error_abort); > > - } > > - > > - return bsel_alloc; > > -} > > - > > -static void acpi_set_pci_info(void) > > -{ > > - PCIBus *bus = find_i440fx(); /* TODO: Q35 support */ > > - unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT; > > - > > - if (bus) { > > - /* Scan all PCI buses. Set property to enable acpi based hotplug. */ > > - pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc); > > - } > > -} > > - > > static void build_append_pcihp_notify_entry(Aml *method, int slot) > > { > > Aml *if_ctx; > > @@ -2888,8 +2858,6 @@ void acpi_setup(void) > > > > build_state = g_malloc0(sizeof *build_state); > > > > - acpi_set_pci_info(); > > - > > acpi_build_tables_init(&tables); > > acpi_build(&tables, MACHINE(pcms)); > > > > -- > > Anthony PERARD
On Thu, 17 Aug 2017 17:23:46 +0100 Anthony PERARD <anthony.perard@citrix.com> wrote: > This means that the function will be call and the property > acpi-pcihp-bsel will be set even if ACPI build is disable. s/call/called/ s/disable/disabled/ Maybe something along this lines: HW part of APCI PCI hotplug in QEMU depends on ACPI_PCIHP_PROP_BSEL being set on a PCI bus that supports ACPI hotplug. It should work regardless of source of ACPI tables (QEMU generator/legacy Seabios/Xen). So move ACPI_PCIHP_PROP_BSEL initialization into HW ACPI impl. part from QEMU's ACPI table generator. > > To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be > set, but this was done only when ACPI tables are built which is not > needed for a Xen guest. The need for the property starts with commit > "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice" > (f0c9d64a68b776374ec4732424a3e27753ce37b6). > > Reported-by: Sander Eikelenboom <linux@eikelenboom.it> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > > --- > Changes in V3: > - move acpi_set_pci_info to pcihp instead > > Changes in V2: > - check for acpi_enabled before calling acpi_set_pci_info. > - set the property on the root bus only. > > This patch would be a canditade to backport to 2.9, along with > "hw/acpi: Limit hotplug to root bus on legacy mode" > > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Bruce Rogers <brogers@suse.com> > --- > hw/acpi/pcihp.c | 31 +++++++++++++++++++++++++++++++ > hw/i386/acpi-build.c | 32 -------------------------------- > 2 files changed, 31 insertions(+), 32 deletions(-) > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > index 9db3c2eaf2..44e8842db8 100644 > --- a/hw/acpi/pcihp.c > +++ b/hw/acpi/pcihp.c > @@ -75,6 +75,36 @@ static int acpi_pcihp_get_bsel(PCIBus *bus) > } > } > > +/* Assign BSEL property to all buses. In the future, this can be changed > + * to only assign to buses that support hotplug. > + */ > +static void *acpi_set_bsel(PCIBus *bus, void *opaque) > +{ > + unsigned *bsel_alloc = opaque; > + unsigned *bus_bsel; > + > + if (qbus_is_hotpluggable(BUS(bus))) { > + bus_bsel = g_malloc(sizeof *bus_bsel); > + > + *bus_bsel = (*bsel_alloc)++; > + object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, > + bus_bsel, &error_abort); > + } > + > + return bsel_alloc; > +} > + > +static void acpi_set_pci_info(void) > +{ > + PCIBus *bus = find_i440fx(); /* TODO: Q35 support */ > + unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT; > + > + if (bus) { > + /* Scan all PCI buses. Set property to enable acpi based hotplug. */ > + pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc); > + } > +} > + > static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque) > { > AcpiPciHpFind *find = opaque; > @@ -177,6 +207,7 @@ static void acpi_pcihp_update(AcpiPciHpState *s) > > void acpi_pcihp_reset(AcpiPciHpState *s) > { > + acpi_set_pci_info(); > acpi_pcihp_update(s); > } > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 98dd424678..4d19d91e1b 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -493,36 +493,6 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms) > table_data->len - madt_start, 1, NULL, NULL); > } > > -/* Assign BSEL property to all buses. In the future, this can be changed > - * to only assign to buses that support hotplug. > - */ > -static void *acpi_set_bsel(PCIBus *bus, void *opaque) > -{ > - unsigned *bsel_alloc = opaque; > - unsigned *bus_bsel; > - > - if (qbus_is_hotpluggable(BUS(bus))) { > - bus_bsel = g_malloc(sizeof *bus_bsel); > - > - *bus_bsel = (*bsel_alloc)++; > - object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, > - bus_bsel, &error_abort); > - } > - > - return bsel_alloc; > -} > - > -static void acpi_set_pci_info(void) > -{ > - PCIBus *bus = find_i440fx(); /* TODO: Q35 support */ > - unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT; > - > - if (bus) { > - /* Scan all PCI buses. Set property to enable acpi based hotplug. */ > - pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc); > - } > -} > - > static void build_append_pcihp_notify_entry(Aml *method, int slot) > { > Aml *if_ctx; > @@ -2888,8 +2858,6 @@ void acpi_setup(void) > > build_state = g_malloc0(sizeof *build_state); > > - acpi_set_pci_info(); > - > acpi_build_tables_init(&tables); > acpi_build(&tables, MACHINE(pcms)); >
On Fri, Aug 18, 2017 at 11:37:34AM +0200, Igor Mammedov wrote: > On Fri, 18 Aug 2017 04:40:02 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Thu, Aug 17, 2017 at 05:23:46PM +0100, Anthony PERARD wrote: > > > This means that the function will be call and the property > > > acpi-pcihp-bsel will be set even if ACPI build is disable. > > > > > > To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be > > > set, but this was done only when ACPI tables are built which is not > > > needed for a Xen guest. The need for the property starts with commit > > > "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice" > > > (f0c9d64a68b776374ec4732424a3e27753ce37b6). > > > > > > Reported-by: Sander Eikelenboom <linux@eikelenboom.it> > > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > > > > > > --- > > > Changes in V3: > > > - move acpi_set_pci_info to pcihp instead > > > > > > Changes in V2: > > > - check for acpi_enabled before calling acpi_set_pci_info. > > > - set the property on the root bus only. > > > > > > This patch would be a canditade to backport to 2.9, along with > > > "hw/acpi: Limit hotplug to root bus on legacy mode" > > > > > > CC: Stefano Stabellini <sstabellini@kernel.org> > > > CC: Bruce Rogers <brogers@suse.com> > > > --- > > > hw/acpi/pcihp.c | 31 +++++++++++++++++++++++++++++++ > > > hw/i386/acpi-build.c | 32 -------------------------------- > > > 2 files changed, 31 insertions(+), 32 deletions(-) > > > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > > > index 9db3c2eaf2..44e8842db8 100644 > > > --- a/hw/acpi/pcihp.c > > > +++ b/hw/acpi/pcihp.c > > > @@ -75,6 +75,36 @@ static int acpi_pcihp_get_bsel(PCIBus *bus) > > > } > > > } > > > > > > +/* Assign BSEL property to all buses. In the future, this can be changed > > > + * to only assign to buses that support hotplug. > > > + */ > > > +static void *acpi_set_bsel(PCIBus *bus, void *opaque) > > > +{ > > > + unsigned *bsel_alloc = opaque; > > > + unsigned *bus_bsel; > > > + > > > + if (qbus_is_hotpluggable(BUS(bus))) { > > > + bus_bsel = g_malloc(sizeof *bus_bsel); > > > + > > > + *bus_bsel = (*bsel_alloc)++; > > > + object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, > > > + bus_bsel, &error_abort); > > > + } > > > + > > > + return bsel_alloc; > > > +} > > > + > > > +static void acpi_set_pci_info(void) > > > +{ > > > + PCIBus *bus = find_i440fx(); /* TODO: Q35 support */ > > > + unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT; > > > + > > > + if (bus) { > > > + /* Scan all PCI buses. Set property to enable acpi based hotplug. */ > > > + pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc); > > > + } > > > +} > > > + > > > static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque) > > > { > > > AcpiPciHpFind *find = opaque; > > > @@ -177,6 +207,7 @@ static void acpi_pcihp_update(AcpiPciHpState *s) > > > > > > void acpi_pcihp_reset(AcpiPciHpState *s) > > > { > > > + acpi_set_pci_info(); > > > acpi_pcihp_update(s); > > > } > > > > IIUC doing this on reset will add property over and over again leaking > > memory. > in v2 I've explicitly suggested to call it once, like: Sorry I misunderstood. I'll fix it. > acpi_set_pci_info() { > > static bool bsel_is set; > > if (bsel_is set) > return; > bsel_is set = true; > > ... > } > > not patch related: > BTW bsel assignment is not stable in hotplug + migration use case, > and we probably should fix it up in 2.11 (CCing Marcel) > > > I think that we need to do it on machine done. > > > > Igor, I think reordering acpi-build like earlier version did > > is less intrusive and more appropriate for 2.10. > > > > For 2.10 I would like to see ideally some changes that > > are all if (xen) making it obvious non xen is not > > affected. I can then ack it and it will be merged in xen tree. > it didn't work before so I'd just push fix to 2.11 without > intermediate fix. > But if you guys think it's worth to fix in 2.10, I'm fine with v2 > for it if Anthony will take care of it (rebase this series) > in 2.11 merge window. Yes, I can take care of this series for 2.11, and find out how to build the mips-softmmu target which does not build because it's missing find_i440fx. > > > > Clean it up after 2.10. > > So is the v2 good enough or do I need to resend it? Thanks,
On Fri, 18 Aug 2017 14:31:07 +0100 Anthony PERARD <anthony.perard@citrix.com> wrote: > On Fri, Aug 18, 2017 at 11:37:34AM +0200, Igor Mammedov wrote: > > On Fri, 18 Aug 2017 04:40:02 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Thu, Aug 17, 2017 at 05:23:46PM +0100, Anthony PERARD wrote: > > > > This means that the function will be call and the property > > > > acpi-pcihp-bsel will be set even if ACPI build is disable. > > > > > > > > To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be > > > > set, but this was done only when ACPI tables are built which is not > > > > needed for a Xen guest. The need for the property starts with commit > > > > "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice" > > > > (f0c9d64a68b776374ec4732424a3e27753ce37b6). > > > > > > > > Reported-by: Sander Eikelenboom <linux@eikelenboom.it> > > > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > > > > > > > > --- > > > > Changes in V3: > > > > - move acpi_set_pci_info to pcihp instead > > > > > > > > Changes in V2: > > > > - check for acpi_enabled before calling acpi_set_pci_info. > > > > - set the property on the root bus only. > > > > > > > > This patch would be a canditade to backport to 2.9, along with > > > > "hw/acpi: Limit hotplug to root bus on legacy mode" > > > > > > > > CC: Stefano Stabellini <sstabellini@kernel.org> > > > > CC: Bruce Rogers <brogers@suse.com> > > > > --- > > > > hw/acpi/pcihp.c | 31 +++++++++++++++++++++++++++++++ > > > > hw/i386/acpi-build.c | 32 -------------------------------- > > > > 2 files changed, 31 insertions(+), 32 deletions(-) > > > > > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > > > > index 9db3c2eaf2..44e8842db8 100644 > > > > --- a/hw/acpi/pcihp.c > > > > +++ b/hw/acpi/pcihp.c > > > > @@ -75,6 +75,36 @@ static int acpi_pcihp_get_bsel(PCIBus *bus) > > > > } > > > > } > > > > > > > > +/* Assign BSEL property to all buses. In the future, this can be changed > > > > + * to only assign to buses that support hotplug. > > > > + */ > > > > +static void *acpi_set_bsel(PCIBus *bus, void *opaque) > > > > +{ > > > > + unsigned *bsel_alloc = opaque; > > > > + unsigned *bus_bsel; > > > > + > > > > + if (qbus_is_hotpluggable(BUS(bus))) { > > > > + bus_bsel = g_malloc(sizeof *bus_bsel); > > > > + > > > > + *bus_bsel = (*bsel_alloc)++; > > > > + object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, > > > > + bus_bsel, &error_abort); > > > > + } > > > > + > > > > + return bsel_alloc; > > > > +} > > > > + > > > > +static void acpi_set_pci_info(void) > > > > +{ > > > > + PCIBus *bus = find_i440fx(); /* TODO: Q35 support */ > > > > + unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT; > > > > + > > > > + if (bus) { > > > > + /* Scan all PCI buses. Set property to enable acpi based hotplug. */ > > > > + pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc); > > > > + } > > > > +} > > > > + > > > > static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque) > > > > { > > > > AcpiPciHpFind *find = opaque; > > > > @@ -177,6 +207,7 @@ static void acpi_pcihp_update(AcpiPciHpState *s) > > > > > > > > void acpi_pcihp_reset(AcpiPciHpState *s) > > > > { > > > > + acpi_set_pci_info(); > > > > acpi_pcihp_update(s); > > > > } > > > > > > IIUC doing this on reset will add property over and over again leaking > > > memory. > > in v2 I've explicitly suggested to call it once, like: > > Sorry I misunderstood. I'll fix it. > > > acpi_set_pci_info() { > > > > static bool bsel_is set; > > > > if (bsel_is set) > > return; > > bsel_is set = true; > > > > ... > > } > > > > not patch related: > > BTW bsel assignment is not stable in hotplug + migration use case, > > and we probably should fix it up in 2.11 (CCing Marcel) > > > > > I think that we need to do it on machine done. > > > > > > Igor, I think reordering acpi-build like earlier version did > > > is less intrusive and more appropriate for 2.10. > > > > > > For 2.10 I would like to see ideally some changes that > > > are all if (xen) making it obvious non xen is not > > > affected. I can then ack it and it will be merged in xen tree. > > it didn't work before so I'd just push fix to 2.11 without > > intermediate fix. > > But if you guys think it's worth to fix in 2.10, I'm fine with v2 > > for it if Anthony will take care of it (rebase this series) > > in 2.11 merge window. > > Yes, I can take care of this series for 2.11, and find out how to build > the mips-softmmu target which does not build because it's missing > find_i440fx. it doesn't look like mips supports ACPI, so we probably shouldn't build acpi/pcihp for it at all but I'm not sure how hard it would be to factor out if possible at all > > > > > > Clean it up after 2.10. > > > > > So is the v2 good enough or do I need to resend it? Do you really need it in 2.10? it's only 2 days left till release so unless it's blocker I'd wait till after release and do clean fix. > Thanks, >
On Fri, Aug 18, 2017 at 04:19:57PM +0200, Igor Mammedov wrote: > On Fri, 18 Aug 2017 14:31:07 +0100 > Anthony PERARD <anthony.perard@citrix.com> wrote: > > > On Fri, Aug 18, 2017 at 11:37:34AM +0200, Igor Mammedov wrote: > > > On Fri, 18 Aug 2017 04:40:02 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Thu, Aug 17, 2017 at 05:23:46PM +0100, Anthony PERARD wrote: > > > > > This means that the function will be call and the property > > > > > acpi-pcihp-bsel will be set even if ACPI build is disable. > > > > > > > > > > To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be > > > > > set, but this was done only when ACPI tables are built which is not > > > > > needed for a Xen guest. The need for the property starts with commit > > > > > "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice" > > > > > (f0c9d64a68b776374ec4732424a3e27753ce37b6). > > > > > > > > > > Reported-by: Sander Eikelenboom <linux@eikelenboom.it> > > > > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > > > > > > > > > > --- > > > > > Changes in V3: > > > > > - move acpi_set_pci_info to pcihp instead > > > > > > > > > > Changes in V2: > > > > > - check for acpi_enabled before calling acpi_set_pci_info. > > > > > - set the property on the root bus only. > > > > > > > > > > This patch would be a canditade to backport to 2.9, along with > > > > > "hw/acpi: Limit hotplug to root bus on legacy mode" > > > > > > > > > > CC: Stefano Stabellini <sstabellini@kernel.org> > > > > > CC: Bruce Rogers <brogers@suse.com> > > > > > --- > > > > > hw/acpi/pcihp.c | 31 +++++++++++++++++++++++++++++++ > > > > > hw/i386/acpi-build.c | 32 -------------------------------- > > > > > 2 files changed, 31 insertions(+), 32 deletions(-) > > > > > > > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > > > > > index 9db3c2eaf2..44e8842db8 100644 > > > > > --- a/hw/acpi/pcihp.c > > > > > +++ b/hw/acpi/pcihp.c > > > > > @@ -75,6 +75,36 @@ static int acpi_pcihp_get_bsel(PCIBus *bus) > > > > > } > > > > > } > > > > > > > > > > +/* Assign BSEL property to all buses. In the future, this can be changed > > > > > + * to only assign to buses that support hotplug. > > > > > + */ > > > > > +static void *acpi_set_bsel(PCIBus *bus, void *opaque) > > > > > +{ > > > > > + unsigned *bsel_alloc = opaque; > > > > > + unsigned *bus_bsel; > > > > > + > > > > > + if (qbus_is_hotpluggable(BUS(bus))) { > > > > > + bus_bsel = g_malloc(sizeof *bus_bsel); > > > > > + > > > > > + *bus_bsel = (*bsel_alloc)++; > > > > > + object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, > > > > > + bus_bsel, &error_abort); > > > > > + } > > > > > + > > > > > + return bsel_alloc; > > > > > +} > > > > > + > > > > > +static void acpi_set_pci_info(void) > > > > > +{ > > > > > + PCIBus *bus = find_i440fx(); /* TODO: Q35 support */ > > > > > + unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT; > > > > > + > > > > > + if (bus) { > > > > > + /* Scan all PCI buses. Set property to enable acpi based hotplug. */ > > > > > + pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc); > > > > > + } > > > > > +} > > > > > + > > > > > static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque) > > > > > { > > > > > AcpiPciHpFind *find = opaque; > > > > > @@ -177,6 +207,7 @@ static void acpi_pcihp_update(AcpiPciHpState *s) > > > > > > > > > > void acpi_pcihp_reset(AcpiPciHpState *s) > > > > > { > > > > > + acpi_set_pci_info(); > > > > > acpi_pcihp_update(s); > > > > > } > > > > > > > > IIUC doing this on reset will add property over and over again leaking > > > > memory. > > > in v2 I've explicitly suggested to call it once, like: > > > > Sorry I misunderstood. I'll fix it. > > > > > acpi_set_pci_info() { > > > > > > static bool bsel_is set; > > > > > > if (bsel_is set) > > > return; > > > bsel_is set = true; > > > > > > ... > > > } > > > > > > not patch related: > > > BTW bsel assignment is not stable in hotplug + migration use case, > > > and we probably should fix it up in 2.11 (CCing Marcel) > > > > > > > I think that we need to do it on machine done. > > > > > > > > Igor, I think reordering acpi-build like earlier version did > > > > is less intrusive and more appropriate for 2.10. > > > > > > > > For 2.10 I would like to see ideally some changes that > > > > are all if (xen) making it obvious non xen is not > > > > affected. I can then ack it and it will be merged in xen tree. > > > it didn't work before so I'd just push fix to 2.11 without > > > intermediate fix. > > > But if you guys think it's worth to fix in 2.10, I'm fine with v2 > > > for it if Anthony will take care of it (rebase this series) > > > in 2.11 merge window. > > > > Yes, I can take care of this series for 2.11, and find out how to build > > the mips-softmmu target which does not build because it's missing > > find_i440fx. > it doesn't look like mips supports ACPI, so we probably shouldn't > build acpi/pcihp for it at all but I'm not sure how hard it > would be to factor out if possible at all > > > > > > > > > > Clean it up after 2.10. > > > > > > > > So is the v2 good enough or do I need to resend it? > Do you really need it in 2.10? > it's only 2 days left till release so unless it's blocker > I'd wait till after release and do clean fix. It mostly means that someone building QEMU 2.10 would be unable to do PCI passthrough hotplug. But PCI PT works fine when the device is added before a guest is started. Maybe hotplug can work as well with extra steps done in the guest to force to probe for new devices. So I would say it is not a blocker, and could be added in the known issue of the release notes?
On Fri, Aug 18, 2017 at 04:19:57PM +0200, Igor Mammedov wrote: > > > > > > > > Clean it up after 2.10. > > > > > > > > So is the v2 good enough or do I need to resend it? > Do you really need it in 2.10? > it's only 2 days left till release so unless it's blocker > I'd wait till after release and do clean fix. Well it's a regression isn't it? > > > Thanks, > >
On Fri, Aug 18, 2017 at 05:00:18PM +0100, Anthony PERARD wrote: > > > > > > > > > > Clean it up after 2.10. > > > > > > > > > > > So is the v2 good enough or do I need to resend it? > > Do you really need it in 2.10? > > it's only 2 days left till release so unless it's blocker > > I'd wait till after release and do clean fix. > > It mostly means that someone building QEMU 2.10 would be unable to do > PCI passthrough hotplug. But PCI PT works fine when the device is added > before a guest is started. Maybe hotplug can work as well with extra > steps done in the guest to force to probe for new devices. > > So I would say it is not a blocker, and could be added in the known > issue of the release notes? Regressions aren't nice. But risk to working setups isn't nice either. If you can come up with a patch that obviously limits the effect to xen only, I'll merge it or ack for merge through Xen tree. > -- > Anthony PERARD
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c index 9db3c2eaf2..44e8842db8 100644 --- a/hw/acpi/pcihp.c +++ b/hw/acpi/pcihp.c @@ -75,6 +75,36 @@ static int acpi_pcihp_get_bsel(PCIBus *bus) } } +/* Assign BSEL property to all buses. In the future, this can be changed + * to only assign to buses that support hotplug. + */ +static void *acpi_set_bsel(PCIBus *bus, void *opaque) +{ + unsigned *bsel_alloc = opaque; + unsigned *bus_bsel; + + if (qbus_is_hotpluggable(BUS(bus))) { + bus_bsel = g_malloc(sizeof *bus_bsel); + + *bus_bsel = (*bsel_alloc)++; + object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, + bus_bsel, &error_abort); + } + + return bsel_alloc; +} + +static void acpi_set_pci_info(void) +{ + PCIBus *bus = find_i440fx(); /* TODO: Q35 support */ + unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT; + + if (bus) { + /* Scan all PCI buses. Set property to enable acpi based hotplug. */ + pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc); + } +} + static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque) { AcpiPciHpFind *find = opaque; @@ -177,6 +207,7 @@ static void acpi_pcihp_update(AcpiPciHpState *s) void acpi_pcihp_reset(AcpiPciHpState *s) { + acpi_set_pci_info(); acpi_pcihp_update(s); } diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 98dd424678..4d19d91e1b 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -493,36 +493,6 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms) table_data->len - madt_start, 1, NULL, NULL); } -/* Assign BSEL property to all buses. In the future, this can be changed - * to only assign to buses that support hotplug. - */ -static void *acpi_set_bsel(PCIBus *bus, void *opaque) -{ - unsigned *bsel_alloc = opaque; - unsigned *bus_bsel; - - if (qbus_is_hotpluggable(BUS(bus))) { - bus_bsel = g_malloc(sizeof *bus_bsel); - - *bus_bsel = (*bsel_alloc)++; - object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, - bus_bsel, &error_abort); - } - - return bsel_alloc; -} - -static void acpi_set_pci_info(void) -{ - PCIBus *bus = find_i440fx(); /* TODO: Q35 support */ - unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT; - - if (bus) { - /* Scan all PCI buses. Set property to enable acpi based hotplug. */ - pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc); - } -} - static void build_append_pcihp_notify_entry(Aml *method, int slot) { Aml *if_ctx; @@ -2888,8 +2858,6 @@ void acpi_setup(void) build_state = g_malloc0(sizeof *build_state); - acpi_set_pci_info(); - acpi_build_tables_init(&tables); acpi_build(&tables, MACHINE(pcms));
This means that the function will be call and the property acpi-pcihp-bsel will be set even if ACPI build is disable. To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be set, but this was done only when ACPI tables are built which is not needed for a Xen guest. The need for the property starts with commit "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice" (f0c9d64a68b776374ec4732424a3e27753ce37b6). Reported-by: Sander Eikelenboom <linux@eikelenboom.it> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Changes in V3: - move acpi_set_pci_info to pcihp instead Changes in V2: - check for acpi_enabled before calling acpi_set_pci_info. - set the property on the root bus only. This patch would be a canditade to backport to 2.9, along with "hw/acpi: Limit hotplug to root bus on legacy mode" CC: Stefano Stabellini <sstabellini@kernel.org> CC: Bruce Rogers <brogers@suse.com> --- hw/acpi/pcihp.c | 31 +++++++++++++++++++++++++++++++ hw/i386/acpi-build.c | 32 -------------------------------- 2 files changed, 31 insertions(+), 32 deletions(-)