Re: [Qemu-devel] [PATCH v16 10/14] pci: introduce pci bus pre reset

[Chen Fan]

On 01/15/2016 04:36 AM, Alex Williamson wrote:
> On Tue, 2016-01-12 at 10:43 +0800, Cao jin wrote:
> > From: Chen Fan <address@hidden>
> >
> > avoid repeat bus reset, here introduce a sequence ID for each time
> > bus hot reset, so each vfio device could know whether they've already
> > been reset for that sequence ID.
> >
> > Signed-off-by: Chen Fan <address@hidden>
> > ---
> >   hw/pci/pci.c             | 13 +++++++++++++
> >   hw/pci/pci_bridge.c      |  3 +++
> >   include/hw/pci/pci.h     |  1 +
> >   include/hw/pci/pci_bus.h |  3 +++
> >   4 files changed, 20 insertions(+)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index f6ca6ef..ceb72d5 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -91,6 +91,18 @@ static void pci_bus_unrealize(BusState *qbus,
> > Error **errp)
> >       vmstate_unregister(NULL, &vmstate_pcibus, bus);
> >   }
> >   
> > +void pci_bus_pre_reset(PCIBus *bus, uint32_t seqid)
> > +{
> > +    PCIBus *sec;
> > +
> > +    bus->in_reset = true;
> > +    bus->reset_seqid = seqid;
> > +
> > +    QLIST_FOREACH(sec, &bus->child, sibling) {
> > +        pci_bus_pre_reset(sec, seqid);
> > +    }
> > +}
> > +
> >   static bool pcibus_is_root(PCIBus *bus)
> >   {
> >       return !bus->parent_dev;
> > @@ -276,6 +288,7 @@ static void pcibus_reset(BusState *qbus)
> >       for (i = 0; i < bus->nirq; i++) {
> >           assert(bus->irq_count[i] == 0);
> >       }
> > +    bus->in_reset = false;
> >   }
> >   
> >   static void pci_host_bus_register(PCIBus *bus, DeviceState *parent)
> > diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> > index 40c97b1..c7f15a1 100644
> > --- a/hw/pci/pci_bridge.c
> > +++ b/hw/pci/pci_bridge.c
> > @@ -268,6 +268,9 @@ void pci_bridge_write_config(PCIDevice *d,
> >       newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> >       if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) {
> >           /* Trigger hot reset on 0->1 transition. */
> > +        uint32_t seqid = s->sec_bus.reset_seqid++;
> Doesn't this need to come from a global sequence ID?  Imagine the case
> of a nested bus, the leaf bus is reset incrementing the sequence ID.
> The devices on that bus store that sequence ID as they're reset.  The
> parent bus is then reset, but all the devices on the leaf bus have
> already been reset for that sequence ID and ignore the reset.
>
> > +
> > +        pci_bus_pre_reset(&s->sec_bus, seqid ? seqid : 1);
> Does this work?  Seems like this would make devices ignore the second
> bus reset after the VM is instantiated.  ie.  the first bus reset seqid
> is 0, so we call pre_reset with 1, the second time we call it with 1
> again.
>
> >           qbus_reset_all(&s->sec_bus.qbus);
> I'd be tempted to call qbus_walk_children() directly, it already has a
> pre_busfn callback hook.
Hi Alex,

this looks like need to change much pci core code,  as Michael suggested 
in 00/14,
maybe we should simply the aer implementation. what do you think of that?

Thanks,
Chen


> >       }
> >   }
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index 379b6e1..b811279 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -381,6 +381,7 @@ void pci_bus_fire_intx_routing_notifier(PCIBus
> > *bus);
> >   void pci_device_set_intx_routing_notifier(PCIDevice *dev,
> >                                             PCIINTxRoutingNotifier
> > notifier);
> >   void pci_device_reset(PCIDevice *dev);
> > +void pci_bus_pre_reset(PCIBus *bus, uint32_t seqid);
> >   
> >   PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus,
> >                                  const char *default_model,
> > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> > index 7812fa9..dd6aaf1 100644
> > --- a/include/hw/pci/pci_bus.h
> > +++ b/include/hw/pci/pci_bus.h
> > @@ -40,6 +40,9 @@ struct PCIBus {
> >       int nirq;
> >       int *irq_count;
> >   
> > +    bool in_reset;
> > +    uint32_t reset_seqid;
> > +
> >       NotifierWithReturnList hotplug_notifiers;
> >   };
> >   
>
>
> .