On Wed, Jan 06, 2010 at 01:48:52PM +0800, Cui, Dexuan wrote:
> 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.
Understood.
>
> >> @@ -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. :-)
Thanks for clarifying that, I missed the difference between
devs and pci_dev_list.
>
> >>
> >> 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.
I have no problem with duplicate invocations of setupOneDevice()
as long as the result is correct. Removing the logic above
seems to slightly simplify things. And thats good :-)
> 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.
Yes, I found that that too. It was very difficult for me to make changes too.
>
> It would be really great somebody can help to re-organize/cleanup the
> code some time. :-)
Is the ocaml replacement for xend still being worked on?
Refactoring the current (python) code is incredibly painful
in my experience.
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home