Re: [PATCH] xend: passthrough: also do_FLR when a device is assigned

[Simon Horman <horms@verge.net.au>]

> 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.
> 
> Signed-off-by: Dexuan Cui <dexuan.cui@intel.com>

Hi Dexuan,

I'm not sure that I've entirely got my head around this change.
But it does seem like a reasonable approach. Comments below.

> 
> diff -r 4feec90815a0 tools/python/xen/xend/XendDomainInfo.py
> --- a/tools/python/xen/xend/XendDomainInfo.py	Tue Jan 05 08:40:18 2010 +0000
> +++ b/tools/python/xen/xend/XendDomainInfo.py	Tue Jan 05 22:17:13 2010 +0800
> @@ -689,11 +689,24 @@ class XendDomainInfo:
>                      raise VmError("device is already inserted")
>  
>          # Test whether the devices can be assigned.
> -        self.pci_device_check_attachability(new_dev)
> +        self.pci_dev_check_attachability_and_do_FLR(new_dev)
>  
>          return self.hvm_pci_device_insert_dev(new_dev)
>  
> -    def pci_device_check_attachability(self, new_dev):
> +    def pci_dev_check_assignability_and_do_FLR(self, config):
> +        """ In the case of static device assignment(i.e., the 'pci' string in
> +        guest config file), we check if the device(s) specified in the 'pci'
> +        can be  assigned to guest or not; if yes, we do_FLR the device(s).
> +        """
> +        pci_dev_ctrl = self.getDeviceController('pci')
> +        return pci_dev_ctrl.dev_check_assignability_and_do_FLR(config)
> +
> +    def pci_dev_check_attachability_and_do_FLR(self, new_dev):
> +        """ In the case of dynamic device assignment(i.e., xm pci-attach), we
> +        check if the device can be attached to guest or not; if yes, we do_FLR
> +        the device.
> +        """
> +
>          # Test whether the devices can be assigned
>  
>          pci_name = pci_dict_to_bdf_str(new_dev)
> @@ -720,6 +733,8 @@ class XendDomainInfo:
>  
>          # PV guest has less checkings.
>          if not self.info.is_hvm():
> +            # try to do FLR for PV guest
> +            pci_device.do_FLR(self.info.is_hvm(), strict_check)
>              return
>  
>          if not strict_check:
> @@ -748,6 +763,9 @@ class XendDomainInfo:
>                      " because one of its co-assignment device %s has been" + \
>                      " assigned to other domain." \
>                      )% (pci_device.name, self.info['name_label'], pci_str))
> +
> +        # try to do FLR for HVM guest
> +        pci_device.do_FLR(self.info.is_hvm(), strict_check)
>  
>      def hvm_pci_device_insert(self, dev_config):
>          log.debug("XendDomainInfo.hvm_pci_device_insert: %s"
> @@ -919,7 +937,7 @@ class XendDomainInfo:
>          # Do PV specific checking
>              if pci_state == 'Initialising':
>                  # PV PCI device attachment
> -                self.pci_device_check_attachability(dev)
> +                self.pci_dev_check_attachability_and_do_FLR(dev)
>  
>          # If pci platform does not exist, create and exit.
>          if existing_dev_info is None :
> @@ -1218,7 +1236,8 @@ class XendDomainInfo:
>          coassignment_list.remove(pci_device.name)
>          assigned_pci_device_str_list = self._get_assigned_pci_devices()
>          for pci_str in coassignment_list:
> -            if pci_str in assigned_pci_device_str_list:
> +            if xoptions.get_pci_dev_assign_strict_check() and \
> +                pci_str in assigned_pci_device_str_list:
>                  raise VmError(("pci: failed to pci-detach %s from domain %s" + \
>                      " because one of its co-assignment device %s is still " + \
>                      " assigned to the domain." \
> @@ -2312,6 +2331,10 @@ class XendDomainInfo:
>              if devclass in XendDevices.valid_devices() and devclass != 'vscsi':
>                  log.info("createDevice: %s : %s" % (devclass, scrub_password(config)))
>                  dev_uuid = config.get('uuid')
> +
> +                if devclass == 'pci':
> +                    self.pci_dev_check_assignability_and_do_FLR(config)
> +
>                  if devclass != 'pci' or not self.info.is_hvm() :
>                      devid = self._createDevice(devclass, config)
>                  
> 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".

>          return
>  
> @@ -292,16 +292,9 @@ class PciController(DevController):
>  
>          if dev.driver!='pciback' and dev.driver!='pci-stub':
>              raise VmError(("pci: PCI Backend and pci-stub don't own "+ \
> -                    "device %s\n") %(dev.name))
> +                    "device %s") %(dev.name))
>  
>          self.CheckSiblingDevices(fe_domid, dev)
> -
> -        # We don't do FLR when we create domain and hotplug device into guest,
> -        # namely, we only do FLR when we destroy domain or hotplug device from
> -        # guest. This is mainly to work around the race condition in hotplug code
> -        # paths. See the changeset's description for details.
> -        # if arch.type != "ia64":
> -        #    dev.do_FLR()
>  
>          if dev.driver == 'pciback':
>              PCIQuirk(dev)
> @@ -353,9 +346,7 @@ class PciController(DevController):
>                  raise VmError(('pci: failed to configure irq on device '+
>                              '%s - errno=%d')%(dev.name,rc))
>  
> -    def setupDevice(self, config):
> -        """Setup devices from config
> -        """
> +    def dev_check_assignability_and_do_FLR(self, config):
>          pci_dev_list = config.get('devs', [])
>          pci_str_list = map(pci_dict_to_bdf_str, pci_dev_list)
>  
> @@ -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?

>  
>              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() ?

> +
> +    def setupDevice(self, config):
> +        """Setup devices from config
> +        """

Some logic seems to be lost from above. Is that intentional?

> +        pci_dev_list = config.get('devs', [])

	   if not self.vm.info.is_hvm(): <--- Should this be here?

> +        for d in pci_dev_list:
> +            self.setupOneDevice(d)
>          wPath = '/local/domain/0/backend/pci/%u/0/aerState' % (self.getDomid())
>          self.aerStateWatch = xswatch(wPath, self._handleAerStateWatch)
>          log.debug('pci: register aer watch %s', wPath)
> @@ -477,7 +472,7 @@ class PciController(DevController):
>  
>          if dev.driver!='pciback' and dev.driver!='pci-stub':
>              raise VmError(("pci: PCI Backend and pci-stub don't own device "+ \
> -                    "%s\n") %(dev.name))
> +                    "%s") %(dev.name))
>  
>          # Need to do FLR here before deassign device in order to terminate
>          # DMA transaction, etc