Simon Horman wrote:
>> xend: passthrough: also do_FLR when a device is assigned.
>>
>> To workaround a race condition about guest hotplug, c/s
>> 18338:7c10be016e4
>> disabled do_FLR when we create guest or 'xm pci-attach' device into
>> guest, so
>> now we actually only do_FLR when a guest is destroyed or 'xm
>> pci-detach'.
>>
>> By moving the FLR-related checking/do_FLR logic a little earlier,
>> this patch re-enables do_FLR in these 2 cases disabled by 18338.
> I'm not sure that I've entirely got my head around this change.
> But it does seem like a reasonable approach. Comments below.
Hi Simon,
Thanks for the review! Please see my replies below.
>> diff -r 4feec90815a0 tools/python/xen/xend/server/pciif.py
>> --- a/tools/python/xen/xend/server/pciif.py Tue Jan 05 08:40:18 2010
>> +0000 +++ b/tools/python/xen/xend/server/pciif.py Tue Jan 05
>> 22:06:37 2010 +0800 @@ -274,8 +274,8 @@ class
>> PciController(DevController): continue
>>
>> if sdev.driver!='pciback' and sdev.driver!='pci-stub':
>> - raise VmError(("pci: PCI Backend and pci-stub
>> don't\n "+ \
>> - "own sibling device %s of device %s\n"\
>> + raise VmError(("pci: PCI Backend and pci-stub don't
>> "+ \ + "own sibling device %s of device %s"\
>> )%(sdev.name, dev.name))
>
> Minor nit, and yes I realise many of the lines affected are my own.
> '\' is not needed to continue a line if the element being continued is
> enclosed in (). So if you are updating a multi-line error as above,
> you
> might as well get rid of the trailing '\' at the same time. There are
> a
> few other candidates below.
>
> Also, "don't" should be "doesn't".
My patch doesn't fix all the minor literal/format issues.
I just saw the obvious ones and fixed them in the patch.
I'm very glad if you can help to double check all the issues and send a patch.
:-)
At present I'm busy with some other more important bugs.
>> @@ -363,12 +354,18 @@ class PciController(DevController):
>> raise VmError('pci: duplicate devices specified in
>> guest config?')
>>
>> strict_check = xoptions.get_pci_dev_assign_strict_check() +
>> devs = [] for pci_dev in pci_dev_list:
>> try:
>> dev = PciDevice(pci_dev)
>> except Exception, e:
>> raise VmError("pci: failed to locate device and "+
>> "parse its resources - "+str(e))
>> + if dev.driver!='pciback' and dev.driver!='pci-stub':
>> + raise VmError(("pci: PCI Backend and pci-stub don't
>> own device"\ + " %s") %(dev.name))
>> +
>> + devs.append(dev)
>
> I'm not sure that I understand why devs is needed. There seem to be
> two cases:
>
> 1) An exception is thrown before devs is fully seeded
> 2) devs is fully seeded as a copy of pci_dev_list
>
> Can devs be removed and pci_dev_list be used directly below?
No, we can't.
An element in pci_dev_list is a python Dict object
An element in the 'devs' is a python PciDevice object.
The intention of "devs" is to record the PciDevice objects generated at the
beginning of dev_check_assignability_and_do_FLR(), so at the end of the
function, we can simply use
"for dev in devs: dev.do_FLR(self.vm.info.is_hvm(), strict_check)" without
generating the PciDevice objects again.
This is only for programming convenience and slightly faster execution of code.
:-)
>>
>> if dev.has_non_page_aligned_bar and strict_check:
>> raise VmError("pci: %s: non-page-aligned MMIO BAR
>> found." % dev.name) @@ -435,18 +432,16 @@ class
>> PciController(DevController):
>> err_msg = 'pci: %s must be
>> co-assigned to the'+\ ' same guest
>> with %s' raise VmError(err_msg % (s, dev.name)) -
>> - # Assigning device staticaly (namely, the pci string in
>> guest config
>> - # file) to PV guest needs this setupOneDevice().
>> - # Assigning device dynamically (namely, 'xm pci-attach') to
>> PV guest
>> - # would go through reconfigureDevice().
>> - #
>> - # For hvm guest, (from c/s 19679 on) assigning device
>> statically and
>> - # dynamically both go through reconfigureDevice(), so HERE
>> the
>> - # setupOneDevice() is not necessary.
>> - if not self.vm.info.is_hvm():
>> - for d in pci_dev_list:
>> - self.setupOneDevice(d)
>> + # try to do FLR
>> + for dev in devs:
>> + dev.do_FLR(self.vm.info.is_hvm(), strict_check)
>
> Not really part of this change, but can self.vm.info.is_hvm() just be
> moved to inside do_FLR() ?
I don't think we can. do_FLR() is a member function of the utility class
PciDevice in util/pci.py -- this file should not involve vm info.
>> +
>> + def setupDevice(self, config):
>> + """Setup devices from config
>> + """
>
> Some logic seems to be lost from above. Is that intentional?
No actual logic is lost. I just move the FLR-related checking logic ahead into
xend/XendDomainInfo.py: def _createDevices().
What setupDevice() does is simply invoking "for each dev: dev.setupOneDevice()".
You could "hg checkout 18044" to see the very old version of setupDevice() that
is before my original FLR patches(c/s 18045~18047)
>
>> + pci_dev_list = config.get('devs', [])
>
> - if not self.vm.info.is_hvm(): <--- Should this be here?
Here I removed the "if not" because I think we'd better also invoke
setupOneDevice for hvm guest (however, I'm not sure this is a must as I haven't
checked that very carefully; what I ses is in the bootup passthrough case, we
do invoke it) in the case of dynamic assignment(namely, xm pci-attach).
You can verify: in the current code (e.g, 20756), after we create an hvm gust
without any "pci" specified in guest config file and when we "xm pci-attach"
the *first* device into guest, setupOneDevice() is not invoked at all!
Yes, I know after my patch is checked in, there would be redundant invokings of
setupOneDevice. Anyway, this is a minor issue and dosesn't impact the
correctness and we can further improve it in future.
BTW: actually I found the pci passthrough code in xend had become very complex
and hacky now... Things are easily broken when new changes are made, e.g.
Stefano added the pci passthrough support in the stubdomain case and the code
broke non-stubdomain case for weeks and the msi-translate is still broken even
till now.
It would be really great somebody can help to re-organize/cleanup the code some
time. :-)
So I think for Xen 4.0, the patch should be fine, and after 4.0, hope somebody
could do some cleanup.
Thanks
-- Dexuan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home