[libvirt] [PATCH] fix the failure of nodedev-reattach caused by the variables from nodedev-attach

[Guannan Ren]

On 07/02/2011 06:10 PM, Wen Congyang wrote:
> At 2011-7-2 13:27, Guannan Ren write:
>> On 07/01/2011 11:30 PM, ghostwcy wrote:
>>> At 2011-7-1 18:24, Guannan Ren write:
>>>> the "virsh nodedev-reattch" command could return successfully, but
>>>> the pci device is still
>>>> bound to pci-stub driver. The reason is noddev-reattach trys to use
>>>> the variables set by
>>>> nodedev-dettach commands. Becuase these variables is not persistent,
>>>> this is not we expected.
>>>> the patch try to fix it.
>>>
>>> I do not agree with this patch. You should read this mail:
>>> https://www.redhat.com/archives/libvir-list/2011-April/msg00315.html
>>>
>>> This patch will cause regression about hotpluging host pci device.
>>>
>>> I think the problem is that: the init value of these variables is 
>>> wrong.
>>> We can fix this bug by modifing pciGetDevice() or its caller.
>>>
>>>>
>>
>> I read the patch, it introduced the mechanism to check whether the
>> device is bound to
>> pci-stub when we write dev-id to new_id if a new device with this ID is
>> hotpluggged, or if
>> a probe is triggered for such a device, I think my patch kept it 
>> working.
>
> I think my explanation is not detailed. For example:
>
> The user runs 'virsh attach-device' to pass through host pci device to 
> guest
> os. We will call the function qemuPrepareHostdevPCIDevices() to bind 
> the pci
> device to pci-stub, and reset it. If the pci device does not support 
> reset function,
> we will call pciReAttachDevice() to rollback the things we have done.
>
> If the device is alread bound to pci-stub before the user runs 'virsh 
> attach-device',
> we should do nothing. If the device is not bound to any driver, we should
> unbound it from pci-stub. If the device is bound to other driver, we 
> should
> unbound it from pci-stub, and reprobe it.
>
> When we call pciReAttachDevice(), the device is bound to pci-stub, but 
> we do
> the different thing, because the origin state of the pci device is 
> different.
> So I add these three variables to remember what we do in 
> pciDettachDevice().
>
          Yep,  the state of the pci device is different possibly or 
probably, I made the determination
          in pciUnbindDeviceFromStub() by examining the name and 
existence of pci device driver.
>
>> The overall idea is as follows:
>> If the device is already bound to pci-stub, then do nothing.
>> If not, try to write dev-id to "new_id", then check the state of its
>> driver(hotplug issue)
>> Unbind from its original driver , then bind to pci-stub driver
>> Anyway, we should write dev-id to "remove_id" to protect the device from
>> causing
>> problems when reattaching it.
>> About fixing the problem from pciGetDevice(), unless we feedback the
>> state of pci device to
>> libvirtd but I don't think it is easier.
>
> We already have pciDeviceSetManaged() and pciDeviceGetManaged().
> I think we should add some similar APIs in util/pci.c to modify and read
> these three variables.
          The "managed" is from the xml file to "virsh attach-device" 
executed each time,
          but the values of these three variables are generated in 
running time , it is hard
          to keep them to the next command.
>
> And we call these new APIs in qemudNodeDeviceDettach() to set these three
> variables to correct value(I think these three variables should be 1).