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

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

[Cui, Dexuan]

[-- Attachment #1: Type: text/plain, Size: 389 bytes --]

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.

Thanks,
-- Dexuan
 

[-- Attachment #2: do_FLR_on_dom_creation_and_pci_attach.patch --]
[-- Type: application/octet-stream, Size: 8708 bytes --]

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>

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))
         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)
 
             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)
+
+    def setupDevice(self, config):
+        """Setup devices from config
+        """
+        pci_dev_list = config.get('devs', [])
+        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

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

[Simon Horman]

> 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

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

[Cui, Dexuan]

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

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

[Simon Horman]

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@lists.xensource.com
> http://lists.xensource.com/xen-devel

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

[Cui, Dexuan]

Simon Horman wrote:
>> 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?
I know few about it.

> Refactoring the current (python) code is incredibly painful
> in my experience.
Agree.

Thanks
-- Dexuan

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

[Stefano Stabellini]

On Wed, 6 Jan 2010, Simon Horman wrote:
> > 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.
> 

Xend is definitely very difficult to work with; hacky code paths
started to appear long ago unfortunately.
Few changes some months ago tried to simplify the code but broke stubdoms
even more.
Then it was very difficult for me to fix stubdoms and at the same time
keep the normal case working.
The whole thing needs to be re-architectured from scratch and that is
what libxenlight is for.

> >
> > 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.
> 

Take a look a libxenlight: the idea is to have a common low level
library to take care of these things in the future.