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

[Chen Fan <chen.fan.fnst@cn.fujitsu.com>]

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 <chen.fan.fnst@cn.fujitsu.com>
>>
>> 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 <chen.fan.fnst@cn.fujitsu.com>
>> ---
>>   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;
>>   };
>>   
>
>
> .
>