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

[Igor Mammedov <imammedo@redhat.com>]

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,
> 


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