|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] xend: passthrough: also do_FLR when a device is assigned
> 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@xxxxxxxxx>
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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |