On 03/10/2016 06:02 PM, John Ferlan wrote:
>
>
> On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
>> We might be just fine looking up the information in pcidevs, but
>> it wouldn't save us any trouble and it's better to be consistent.
>> ---
>> src/util/virhostdev.c | 41 ++++++++++++++++++++++-------------------
>> 1 file changed, 22 insertions(+), 19 deletions(-)
>>
So I went through this one again (new day, fresh start, partially clear
mind).
>> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
>> index a431f0a..03c3445 100644
>> --- a/src/util/virhostdev.c
>> +++ b/src/util/virhostdev.c
First off - perhaps something for the previous documentation patch -
PrepareDevices is called during the qemuProcessLaunch processing (eg
domain startup) for all known host devices. It's also called during
qemuDomainAttachHostPCIDevice or libxlDomainAttachHostPCIDevice
(hotplug) for the *one* new device to be attached.
The purpose of the function is to take the passed hostdev list, move the
devices to a managed active host device list for the guest. Devices that
do not have the managed attribute are not processed.
>> @@ -650,7 +650,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
>>
So after step 6, all the pcidevs are on the activePCIHostdevs list.
Theoretically at least ;-). None are on the inactivePCIHostdevs list.
Why in step 7 do we run through pcidevs, to find the device on the
activePCIHostdev list in order to set the UsedBy of the actual device on
the active list. Why not run the 'activePCIHostdev' list here too?
Since we really don't need pcidevs any more - can we delete it now?
E.G. Step 9 it... Steps 7 & 8 don't really need pcidevs, right? There
is no error path for either. I'm not asking for a patch - just making
sure I'm reading things right!
>> /* Step 8: Now set the original states for hostdev def */
>> for (i = 0; i < nhostdevs; i++) {
>> - virPCIDevicePtr pci;
>> + virPCIDevicePtr actual;
>> virDomainHostdevDefPtr hostdev = hostdevs[i];
>> virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
>>
>> @@ -659,24 +659,27 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
>> if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
>> continue;
>>
>> - pci = virPCIDeviceListFindByIDs(pcidevs,
>> - pcisrc->addr.domain,
>> - pcisrc->addr.bus,
>> - pcisrc->addr.slot,
>> - pcisrc->addr.function);
>> + /* We need to look up the actual device because it's the one
>> + * that contains the information we care about (unbind_from_stub,
>> + * remove_slot, reprobe) */
>
> When a device goes from the inactivePCIHostdevs list to the
> activePCIHostdevs list "at some point in time" in the future - does it
> do a similar save?
I'll answer my own question - because this is the Prepare function and
we don't have to worry about it since everything is on the active list.
>
> That is this change only grabs devices from the active list for the
> save; whereas, prior to this change it would pull from all pcidevs
>
So, I believe this part is correct albeit a bit confusing...
>> + actual = virPCIDeviceListFindByIDs(mgr->activePCIHostdevs,
>> + pcisrc->addr.domain,
>> + pcisrc->addr.bus,
>> + pcisrc->addr.slot,
>> + pcisrc->addr.function);
>>
>> /* Appropriate values for the unbind_from_stub, remove_slot
>> * and reprobe properties of the device were set earlier
>> * by virPCIDeviceDetach() */
>> - if (pci) {
>> + if (actual) {
>> VIR_DEBUG("Saving network configuration of PCI device %s",
>> - virPCIDeviceGetName(pci));
>> + virPCIDeviceGetName(actual));
>> hostdev->origstates.states.pci.unbind_from_stub =
>> - virPCIDeviceGetUnbindFromStub(pci);
>> + virPCIDeviceGetUnbindFromStub(actual);
>> hostdev->origstates.states.pci.remove_slot =
>> - virPCIDeviceGetRemoveSlot(pci);
>> + virPCIDeviceGetRemoveSlot(actual);
>> hostdev->origstates.states.pci.reprobe =
>> - virPCIDeviceGetReprobe(pci);
>> + virPCIDeviceGetReprobe(actual);
>> }
>> }
>>
>> @@ -844,17 +847,17 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
More documentation thoughts... Because what exists now for the function
doesn't really make sense - it's only describing one variable.
Anyway, the purpose of this function is to take the device(s) passed on
the hostdev list and return them to the host (?if they are managed?).
This function is called either from hotplug device remove, hotplug
device attach failure path, or from qemuProcessStop during process
shutdown. When called from the host device remove or attach failure
paths, there is only 1 hostdev in the list.
...
Prior to patch 15, the pcidev's was a copy of the "active list", now
it's a list of all pcidevs found. Hopefully on either one of the two
lists. That is I hope a found hostdev couldn't be on neither list.
Prior to patch 15, step1 would either remove the device from pcidevs or
from the activeList depending on the matching driver name. When done,
the pcidevs would contain the list of devices just removed from the
activeList. Note that virPCIDeviceListDel will repeat the active list
virPCIDeviceListFind (essentially)...
After patch 15, the pcidevs gets reduced for devices not on the
activeList or without a matching drv/dom name. Whether or not that is on
the inactiveList isn't checked, but assumed.
Step 2 is I believe the antecedent of step 4 during the Prepare path.
Step 4 would be called after Reset and before placing devices on active
list replacing the net config for any PCINet device...
Prior to patch 15, step2 would walk the hostdev's list looking for
devices on the pcidevs list (eg. the ones removed from the activeList).
It would then take any PCINet device and restore it's config.
After patch 15 and with these changes, walk the inactiveList and perform
the same call. This is not the devices we just removed from the
activeList, so I don't think using the inactiveList in this case is right.
Before patch 15, step 3 would take any of the devices we removed from
the activeList and perform a reset on them.
After patch 15, step 3 would do a similar function assuming that no
device could not be on neither list.
Hopefully this all makes sense - I keep reading it and going back and
fort between old and new code. The comment left in the code after step
1 is most helpful...
John
>>
>> if (virHostdevIsPCINetDevice(hostdev)) {
>> virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci;
>> - virPCIDevicePtr pci;
>> + virPCIDevicePtr actual;
>>
>> - pci = virPCIDeviceListFindByIDs(pcidevs,
>> - pcisrc->addr.domain,
>> - pcisrc->addr.bus,
>> - pcisrc->addr.slot,
>> - pcisrc->addr.function);
>
> So the previous loop took care of the activePCIHostdevs, correct? And
> this loop takes care of the inactivePCIHostdevs for reattachement? I
> think this part is correct - although is it worthy of splitting into
> it's own separate patch?
>
> John
>
> Going to stop here for now (we have company).
>
>> + actual = virPCIDeviceListFindByIDs(mgr->inactivePCIHostdevs,
>> + pcisrc->addr.domain,
>> + pcisrc->addr.bus,
>> + pcisrc->addr.slot,
>> + pcisrc->addr.function);
>>
>> - if (pci) {
>> + if (actual) {
>> VIR_DEBUG("Restoring network configuration of PCI device %s",
>> - virPCIDeviceGetName(pci));
>> + virPCIDeviceGetName(actual));
>> virHostdevNetConfigRestore(hostdev, mgr->stateDir,
>> oldStateDir);
>> }
>>
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list