Xen devel - May 2009 - [PATCH] disallow duplicate pci device strings in guest config file

When we specify duplicate pci device strings in guest config file, like
pci=[''01:00.0'', ''01:00.0''], or 
pci=[''01:00.0'', ''0000:01:00.0''], or 
pci=[''01:00.0'', ''0000:01:00.0@7''],
xend doesn''t detect this case and passes the pci string to ioemu and
ioemu invokes register_real_device() twice for the same physical device and this
could cause unexpected behavior.

The patch detects this case and makes the domain construction fail.

Signed-off-by: Dexuan Cui <dexuan.cui@intel.com>

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

Why is the right answer not: Don''t do that, then?

 -- Keir

On 15/05/2009 12:31, "Cui, Dexuan" <dexuan.cui@intel.com> wrote:
> When we specify duplicate pci device strings in guest config file, like
> pci=[''01:00.0'', ''01:00.0''], or 
pci=[''01:00.0'', ''0000:01:00.0''], or
> pci=[''01:00.0'', ''0000:01:00.0@7''],
> xend doesn''t detect this case and passes the pci string to ioemu
and ioemu
> invokes register_real_device() twice for the same physical device and this
> could cause unexpected behavior.
> 
> The patch detects this case and makes the domain construction fail.
> 
> Signed-off-by: Dexuan Cui <dexuan.cui@intel.com>


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

Keir Fraser wrote:
> Why is the right answer not: Don''t do that, then?
Yes, normally a user should never do that.
However, if a user really does that somehow(e.g., by mistake), without the small
patch, the user may not notice the mistake in time and may be surprised when
strange things happen (e.g., inside guest, ''lspci'' shows 2
same devices; or guest may crash at some uncertain time point).
So, I think the small patch is useful.

Thanks,
-- Dexuan


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

On 15/05/2009 12:52, "Cui, Dexuan" <dexuan.cui@intel.com> wrote:
> Keir Fraser wrote:
>> Why is the right answer not: Don''t do that, then?
> Yes, normally a user should never do that.
> However, if a user really does that somehow(e.g., by mistake), without the
> small patch, the user may not notice the mistake in time and may be
surprised
> when strange things happen (e.g., inside guest, ''lspci''
shows 2 same devices;
> or guest may crash at some uncertain time point).
> So, I think the small patch is useful.
Post 3.4 then. I don''t know enough about Python conversion rules to be
sure
on this one for 3.4.0.

 -- Keir



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

On Fri, May 15, 2009 at 07:31:23PM +0800, Cui, Dexuan wrote:


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

On Fri, May 15, 2009 at 07:31:23PM +0800, Cui, Dexuan
wrote:
> When we specify duplicate pci device strings in guest config file, like
pci=[''01:00.0'', ''01:00.0''], or 
pci=[''01:00.0'', ''0000:01:00.0''], or 
pci=[''01:00.0'', ''0000:01:00.0@7''],
> xend doesn''t detect this case and passes the pci string to ioemu
and ioemu invokes register_real_device() twice for the same physical device and
this could cause unexpected behavior.
> 
> The patch detects this case and makes the domain construction fail.
> 
> diff -r 40d4267296ad tools/python/xen/xend/server/pciif.py
> --- a/tools/python/xen/xend/server/pciif.py	Fri May 15 08:12:39 2009 +0100
> +++ b/tools/python/xen/xend/server/pciif.py	Fri May 15 17:14:43 2009 +0800
> @@ -396,6 +396,9 @@ class PciController(DevController):
>              pci_str = ''%04x:%02x:%02x.%01x'' % (domain,
bus, slot, func)
>              pci_str_list = pci_str_list + [pci_str]
>              pci_dev_list = pci_dev_list + [(domain, bus, slot, func)]
> +
> +        if pci_str_list != list(set(pci_str_list)):
> +            raise VmError(''pci: duplicate devices specified in
guest config?'')
>  
>          for (domain, bus, slot, func) in pci_dev_list:
>              try:
Hi Dexuan, Hi Keir,

At a glance it seems that this might not be correct,
as set() may not preserve the order of pci_str_list.
However, as set will remove any duplicates, its probably
sufficient to use:

	if len(pci_str_list) != len(set(pci_str_list):

Some musings in code:

# python
>>> pci_str_list = [ "01:00.1", "01:00.0" ]
>>> print pci_str_list
[''01:00.1'',
''01:00.0'']
>>> print set(pci_str_list)
set([''01:00.0'',
''01:00.1''])
>>> print list(set(pci_str_list))
[''01:00.0'', ''01:00.1'']
>>> pci_str_list= [ "01:00.1", "01:00.0",
"01:00.0" ]
>>> print list(set(pci_str_list))
>>> print len(set(pci_str_list))
2
>>> print len(pci_str_list)
3


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

Simon Horman wrote:
> On Fri, May 15, 2009 at 07:31:23PM +0800, Cui, Dexuan wrote:
>> When we specify duplicate pci device strings in guest config file,
>> like pci=[''01:00.0'', ''01:00.0''], or
pci=[''01:00.0'',
>> ''0000:01:00.0''], or 
pci=[''01:00.0'', ''0000:01:00.0@7''],
>> xend doesn''t detect this case and passes the pci string to
ioemu and
>> ioemu invokes register_real_device() twice for the same physical
>> device and this could cause unexpected behavior.  
>> 
>> The patch detects this case and makes the domain construction fail.
>> 
>> diff -r 40d4267296ad tools/python/xen/xend/server/pciif.py
>> --- a/tools/python/xen/xend/server/pciif.py	Fri May 15 08:12:39 2009
>> +0100 +++ b/tools/python/xen/xend/server/pciif.py	Fri May 15
>> 17:14:43 2009 +0800 @@ -396,6 +396,9 @@ class
>>              PciController(DevController): pci_str >>            
''%04x:%02x:%02x.%01x'' % (domain, bus, slot, func)
>>              pci_str_list = pci_str_list + [pci_str] pci_dev_list
>> pci_dev_list + [(domain, bus, slot, func)] + +        if
>> pci_str_list != list(set(pci_str_list)): +            raise
>> VmError(''pci: duplicate devices specified in guest
config?'')
>> 
>>          for (domain, bus, slot, func) in pci_dev_list:
>>              try:
> 
> Hi Dexuan, Hi Keir,
> 
> At a glance it seems that this might not be correct,
> as set() may not preserve the order of pci_str_list.
Thanks for pointing this out !
> However, as set will remove any duplicates, its probably
> sufficient to use:
> 
> 	if len(pci_str_list) != len(set(pci_str_list):
Yes. This should be the right way.

Thanks,
-- Dexuan


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