Cui, Dexuan
2009-Jul-28 09:06 UTC
[Xen-devel] [PATCH]xend: pass-through: fix "xm pci-list-assignable-devices'' for pv_guest
xc.test_assign_device(dev) can only tell us if dev is assigned to hvm guest; if a device is assigned to pv guest, xc.test_assign_device() still tells us the device is not assigned yet, as a result, the device still appears in the output of "xm pci-list-assignable-devices", and xend would not prevent us from assigning the device to a new pv or hvm guest. To judge if a device has been assigned to guest, we have to scan xenstore. Thanks, -- Dexuan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Simon Horman
2009-Jul-28 10:56 UTC
[Xen-devel] Re: [PATCH]xend: pass-through: fix "xm pci-list-assignable-devices'' for pv_guest
On Tue, Jul 28, 2009 at 05:06:43PM +0800, Cui, Dexuan wrote:> > xc.test_assign_device(dev) can only tell us if dev is assigned to hvm guest; > if a device is assigned to pv guest, xc.test_assign_device() still tells us > the device is not assigned yet, as a result, the device still appears in the > output of "xm pci-list-assignable-devices", and xend would not prevent us from > assigning the device to a new pv or hvm guest. > > To judge if a device has been assigned to guest, we have to scan xenstore. > > Thanks, > -- Dexuan > > xend: pass-through: fix "xm pci-list-assignable-devices'' for pv_guest > > xc.test_assign_device(dev) can only tell us if dev is assigned to hvm guest; > if a device is assigned to pv guest, xc.test_assign_device() still tells us > the device is not assigned yet, as a result, the device still appears in the > output of "xm pci-list-assignable-devices", and xend would not prevent us from > assigning the device to a new pv or hvm guest. > > To judge if a device has been assigned to guest, we have to scan xenstore. > > Signed-off-by: Dexuan Cui <dexuan.cui@intel.com> > > diff -r 8af26fef898c tools/python/xen/xend/XendConfig.py > --- a/tools/python/xen/xend/XendConfig.py Fri Jul 24 12:08:54 2009 +0100 > +++ b/tools/python/xen/xend/XendConfig.py Tue Jul 28 16:42:21 2009 +0800 > @@ -2059,9 +2059,6 @@ class XendConfig(dict): > return self[''platform''].get(''hap'', 0) > > def update_platform_pci(self): > - if not self.is_hvm(): > - return > - > pci = [] > for dev_type, dev_info in self.all_devices_sxpr(): > if dev_type != ''pci'': > diff -r 8af26fef898c tools/python/xen/xend/XendDomainInfo.py > --- a/tools/python/xen/xend/XendDomainInfo.py Fri Jul 24 12:08:54 2009 +0100 > +++ b/tools/python/xen/xend/XendDomainInfo.py Tue Jul 28 16:42:21 2009 +0800 > @@ -42,7 +42,7 @@ from xen.util.pci import serialise_pci_o > from xen.util.pci import serialise_pci_opts, pci_opts_list_to_sxp, \ > pci_dict_to_bdf_str, pci_dict_to_xc_str, \ > pci_convert_sxp_to_dict, pci_convert_dict_to_sxp, \ > - pci_dict_cmp, PCI_DEVFN, PCI_SLOT, PCI_FUNC > + pci_dict_cmp, PCI_DEVFN, PCI_SLOT, PCI_FUNC, parse_hex > > from xen.xend import balloon, sxp, uuid, image, arch > from xen.xend import XendOptions, XendNode, XendConfig > @@ -296,20 +296,11 @@ def dom_get(dom): > log.trace("domain_getinfo(%d) failed, ignoring: %s", dom, str(err)) > return None > > -def get_assigned_pci_devices(domid): > - dev_str_list = [] > - path = ''/local/domain/0/backend/pci/%u/0/'' % domid > - num_devs = xstransact.Read(path + ''num_devs''); > - if num_devs is None or num_devs == "": > - return dev_str_list > - num_devs = int(num_devs); > - for i in range(num_devs): > - dev_str = xstransact.Read(path + ''dev-%i'' % i) > - dev_str_list = dev_str_list + [dev_str] > - return dev_str_list > +from xen.xend.server.pciif import parse_pci_name, PciDevice,\ > + get_assigned_pci_devices, get_all_assigned_pci_devices > + > > def do_FLR(domid): > - from xen.xend.server.pciif import parse_pci_name, PciDevice > dev_str_list = get_assigned_pci_devices(domid) > > for dev_str in dev_str_list: > @@ -686,15 +677,14 @@ class XendDomainInfo: > raise VmError("device is already inserted") > > # Test whether the devices can be assigned with VT-d > - bdf = xc.test_assign_device(0, pci_dict_to_xc_str(new_dev)) > - if bdf != 0: > - if bdf == -1: > - raise VmError("failed to assign device: maybe the platform" > - " doesn''t support VT-d, or VT-d isn''t enabled" > - " properly?") > - raise VmError("fail to assign device(%s): maybe it has" > - " already been assigned to other domain, or maybe" > - " it doesn''t exist." % pci_dict_to_bdf_str(new_dev)) > + pci_name = ''%04x:%02x:%02x.%x'' % \ > + (parse_hex(new_dev[''domain'']),\ > + parse_hex(new_dev[''bus'']),\ > + parse_hex(new_dev[''slot'']),\ > + parse_hex(new_dev[''func'']))You should be able to use pci_name = pci_dict_to_bdf_str(new_dev) here instead of the above 5 lines.> + if pci_name in get_all_assigned_pci_devices(): > + raise VmError("failed to assign device %s that has" > + " already been assigned to other domain." % pci_str)Should pci_str be pci_name in the line above ?> > # Here, we duplicate some checkings (in some cases, we mustn''t allow > # a device to be hot-plugged into an HVM guest) that are also done in > @@ -732,8 +722,7 @@ class XendDomainInfo: > pci_device.devs_check_driver(coassignment_list) > assigned_pci_device_str_list = self._get_assigned_pci_devices() > for pci_str in coassignment_list: > - pci_dev = parse_pci_name(pci_str) > - if xc.test_assign_device(0, pci_dict_to_xc_str(pci_dev)) == 0: > + if not (pci_str in get_all_assigned_pci_devices()): > continue > if not pci_str in assigned_pci_device_str_list: > raise VmError(("pci: failed to pci-attach %s to domain %s" + \ > @@ -859,7 +848,7 @@ class XendDomainInfo: > pci_dev = sxp.children(dev_sxp, ''dev'')[0] > dev_config = pci_convert_sxp_to_dict(dev_sxp) > dev = dev_config[''devs''][0] > - > + > # Do HVM specific processing > if self.info.is_hvm(): > if pci_state == ''Initialising'': > @@ -2454,7 +2443,8 @@ class XendDomainInfo: > if pci and len(pci) > 0: > pci = map(lambda x: x[0:4], pci) # strip options > pci_str = str(pci) > - if hvm and pci_str: > + > + if hvm and pci_str != '''': > bdf = xc.test_assign_device(0, pci_str) > if bdf != 0: > if bdf == -1: > @@ -2465,9 +2455,17 @@ class XendDomainInfo: > devfn = (bdf >> 8) & 0xff > dev = (devfn >> 3) & 0x1f > func = devfn & 0x7 > - raise VmError("fail to assign device(%x:%x.%x): maybe it has" > + raise VmError("failed to assign device(%x:%x.%x): maybe it has" > " already been assigned to other domain, or maybe" > " it doesn''t exist." % (bus, dev, func)) > + > + # This test is done for both pv and hvm guest. > + for p in pci: > + pci_name = ''%04x:%02x:%02x.%x'' % \ > + (parse_hex(p[0]), parse_hex(p[1]), parse_hex(p[2]), parse_hex(p[3])) > + if pci_name in get_all_assigned_pci_devices(): > + raise VmError("failed to assign device %s that has" > + " already been assigned to other domain." % pci_name) > > # register the domain in the list > from xen.xend import XendDomain > diff -r 8af26fef898c tools/python/xen/xend/XendNode.py > --- a/tools/python/xen/xend/XendNode.py Fri Jul 24 12:08:54 2009 +0100 > +++ b/tools/python/xen/xend/XendNode.py Tue Jul 28 16:42:21 2009 +0800 > @@ -834,6 +834,9 @@ class XendNode: > > > def pciinfo(self): > + from xen.xend.server.pciif import get_all_assigned_pci_devices > + assigned_devs = get_all_assigned_pci_devices() > + > # Each element of dev_list is a PciDevice > dev_list = PciUtil.find_all_assignable_devices() > > @@ -847,11 +850,7 @@ class XendNode: > for dev_list in devs_list: > available = True > for d in dev_list: > - pci_str = ''0x%x,0x%x,0x%x,0x%x'' %(d.domain, d.bus, d.slot, d.func) > - # Xen doesn''t care what the domid is, so we pass 0 here... > - domid = 0 > - bdf = self.xc.test_assign_device(domid, pci_str) > - if bdf != 0: > + if d.name in assigned_devs:Nice use of d.name :-)> available = False > break > if available: > diff -r 8af26fef898c tools/python/xen/xend/server/pciif.py > --- a/tools/python/xen/xend/server/pciif.py Fri Jul 24 12:08:54 2009 +0100 > +++ b/tools/python/xen/xend/server/pciif.py Tue Jul 28 16:43:49 2009 +0800 > @@ -57,6 +57,25 @@ def parse_hex(val): > return val > except ValueError: > return None > + > +def get_assigned_pci_devices(domid): > + dev_str_list = [] > + path = ''/local/domain/0/backend/pci/%u/0/'' % domid > + num_devs = xstransact.Read(path + ''num_devs''); > + if num_devs is None or num_devs == "": > + return dev_str_list > + num_devs = int(num_devs) > + for i in range(num_devs): > + dev_str = xstransact.Read(path + ''dev-%i'' % i) > + dev_str_list = dev_str_list + [dev_str] > + return dev_str_list > + > +def get_all_assigned_pci_devices(): > + dom_list = xstransact.List(''/local/domain'') > + pci_str_list = [] > + for d in dom_list: > + pci_str_list = pci_str_list + get_assigned_pci_devices(int(d)) > + return pci_str_list > > class PciController(DevController): > > @@ -368,11 +387,8 @@ class PciController(DevController): > dev.devs_check_driver(funcs) > for f in funcs: > if not f in pci_str_list: > - (f_dom, f_bus, f_slot, f_func) = parse_pci_name(f) > - f_pci_str = ''0x%x,0x%x,0x%x,0x%x'' % \ > - (f_dom, f_bus, f_slot, f_func) > # f has been assigned to other guest? > - if xc.test_assign_device(0, f_pci_str) != 0: > + if f in get_all_assigned_pci_devices(): > err_msg = ''pci: %s must be co-assigned to'' + \ > '' the same guest with %s'' > raise VmError(err_msg % (f, dev.name)) > @@ -396,9 +412,8 @@ class PciController(DevController): > dev.devs_check_driver(devs_str) > for s in devs_str: > if not s in pci_str_list: > - s_pci_str = pci_dict_to_bdf_str(parse_pci_name(s)) > # s has been assigned to other guest? > - if xc.test_assign_device(0, s_pci_str) != 0: > + if s in get_all_assigned_pci_devices(): > err_msg = ''pci: %s must be co-assigned to the''+\ > '' same guest with %s'' > raise VmError(err_msg % (s, dev.name)) > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Cui, Dexuan
2009-Jul-29 02:26 UTC
[Xen-devel] RE: [PATCH]xend: pass-through: fix "xm pci-list-assignable-devices'' for pv_guest
Simon Horman wrote:>> pci_dict_to_bdf_str(new_dev)) + pci_name >> ''%04x:%02x:%02x.%x'' % \ + (parse_hex(new_dev[''domain'']),\ >> + parse_hex(new_dev[''bus'']),\ >> + parse_hex(new_dev[''slot'']),\ >> + parse_hex(new_dev[''func''])) > > You should be able to use pci_name = pci_dict_to_bdf_str(new_dev) > here instead of the above 5 lines.Hi Simon, Agree. Your suggestion is good.> >> + if pci_name in get_all_assigned_pci_devices(): >> + raise VmError("failed to assign device %s that has" >> + " already been assigned to other domain." >> % pci_str) > > Should pci_str be pci_name in the line above ?Thanks a lot for the careful review! Actually I corrected the mistake in my side, but I didn''t send the latest version... :-( I''ll ack the fix you sent out just now. Thanks, -- Dexuan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Simon Horman
2009-Jul-29 04:07 UTC
[Xen-devel] Re: [PATCH]xend: pass-through: fix "xm pci-list-assignable-devices'' for pv_guest
On Wed, Jul 29, 2009 at 10:26:21AM +0800, Cui, Dexuan wrote:> Simon Horman wrote: > >> pci_dict_to_bdf_str(new_dev)) + pci_name > >> ''%04x:%02x:%02x.%x'' % \ + (parse_hex(new_dev[''domain'']),\ > >> + parse_hex(new_dev[''bus'']),\ > >> + parse_hex(new_dev[''slot'']),\ > >> + parse_hex(new_dev[''func''])) > > > > You should be able to use pci_name = pci_dict_to_bdf_str(new_dev) > > here instead of the above 5 lines. > Hi Simon, > Agree. Your suggestion is good. > > > > > >> + if pci_name in get_all_assigned_pci_devices(): > >> + raise VmError("failed to assign device %s that has" > >> + " already been assigned to other domain." > >> % pci_str) > > > > Should pci_str be pci_name in the line above ? > Thanks a lot for the careful review! > Actually I corrected the mistake in my side, but I didn''t send the latest version... :-( > I''ll ack the fix you sent out just now.Thanks. In practice I''m not sure that error condition can ever fire, it seemed to be caught earlier on my system. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel