Re: [Xen-devel] [PATCH for-2.10 v3 2/3] hw/acpi: Move acpi_set_pci_info to pcihp

[Igor Mammedov <imammedo@xxxxxxxxxx>]

On Fri, 18 Aug 2017 04:40:02 +0300
"Michael S. Tsirkin" <mst@xxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> > 
> > ---
> > 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@xxxxxxxxxx>
> > CC: Bruce Rogers <brogers@xxxxxxxx>
> > ---
> >  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  


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel