WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH] xend: passthrough: also do_FLR when a device is

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