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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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@xxxxxxxxxx> 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@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:  
> > 
> > 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?

-- 
Anthony PERARD

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