Xen devel - Oct 2007 - [VTD][PATCH] Don''t allow assigning the same device twice without release the first assignment

System will hang, if assign the same device to domains twice without
release the first assignment. This patch checks whether the device
already has been assigned or not before creating domain, if yes, display
prompt message and won''t create the domain. 

Signed-off-by: Weidong Han <weidong.han@intel.com>


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

The DOMCTL_assign_device should check whether the device is already
assigned. This has the benefit that it can atomically check-and-allocate,
under the domctl lock.

You''ll have to work out how to propagate DOMCTL_assign_device failure
to the
user. Either you have to get the error out of ioemu, or perhaps you can
assign the device in xend before starting ioemu.

 -- Keir

On 15/10/07 16:19, "Han, Weidong" <weidong.han@intel.com> wrote:
> System will hang, if assign the same device to domains twice without
> release the first assignment. This patch checks whether the device
> already has been assigned or not before creating domain, if yes, display
> prompt message and won''t create the domain.
> 
> Signed-off-by: Weidong Han <weidong.han@intel.com>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


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

Keir Fraser wrote:
> The DOMCTL_assign_device should check whether the device is already
> assigned. This has the benefit that it can atomically
> check-and-allocate, under the domctl lock.
> 
> You''ll have to work out how to propagate DOMCTL_assign_device
failure
> to the user. Either you have to get the error out of ioemu, or
> perhaps you can assign the device in xend before starting ioemu.
> 
>  -- Keir
> 
Yes, adding the check on DOMCTL_assign_device is simple, but I didn''t
find a good way to propagate DOMCTL_assign_device failure to user. This
patch adds the check in xend before starting ioemu. In addtion, adding
the check in Xend can prompt user on the screen when the device has
already been assigned. Thanks.

-- Weidong

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

On 16/10/07 03:04, "Han, Weidong" <weidong.han@intel.com> wrote:
>> The DOMCTL_assign_device should check whether the device is already
>> assigned. This has the benefit that it can atomically
>> check-and-allocate, under the domctl lock.
>> 
>> You''ll have to work out how to propagate DOMCTL_assign_device
failure
>> to the user. Either you have to get the error out of ioemu, or
>> perhaps you can assign the device in xend before starting ioemu.
> 
> Yes, adding the check on DOMCTL_assign_device is simple, but I
didn''t
> find a good way to propagate DOMCTL_assign_device failure to user. This
> patch adds the check in xend before starting ioemu. In addtion, adding
> the check in Xend can prompt user on the screen when the device has
> already been assigned. Thanks.
Well, that''s too bad because I won''t take the original patch.
Why can''t the
device be assigned by xend?

 -- Keir



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

Keir Fraser wrote:
> On 16/10/07 03:04, "Han, Weidong" <weidong.han@intel.com>
wrote:
> 
>>> The DOMCTL_assign_device should check whether the device is already
>>> assigned. This has the benefit that it can atomically
>>> check-and-allocate, under the domctl lock.
>>> 
>>> You''ll have to work out how to propagate
DOMCTL_assign_device
>>> failure to the user. Either you have to get the error out of ioemu,
>>> or perhaps you can assign the device in xend before starting ioemu.
>> 
>> Yes, adding the check on DOMCTL_assign_device is simple, but I
didn''t
>> find a good way to propagate DOMCTL_assign_device failure to user.
>> This patch adds the check in xend before starting ioemu. In addtion,
>> adding the check in Xend can prompt user on the screen when the
>> device has already been assigned. Thanks.
> 
> Well, that''s too bad because I won''t take the original
patch. Why
> can''t the device be assigned by xend?
> 
>  -- Keir
Ok, I will try your suggestion. 

-- Weidong

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

Keir,

Resend the patch. This patch is implemented according to your
suggestion. Asssigns device in xend before starting ioemu, and add the
check on DOMCTL_assign_device hypercall. Thanks.

-- Weidong

Keir Fraser wrote:
> On 16/10/07 03:04, "Han, Weidong" <weidong.han@intel.com>
wrote:
> 
>>> The DOMCTL_assign_device should check whether the device is already
>>> assigned. This has the benefit that it can atomically
>>> check-and-allocate, under the domctl lock.
>>> 
>>> You''ll have to work out how to propagate
DOMCTL_assign_device
>>> failure to the user. Either you have to get the error out of ioemu,
>>> or perhaps you can assign the device in xend before starting ioemu.
>> 
>> Yes, adding the check on DOMCTL_assign_device is simple, but I
didn''t
>> find a good way to propagate DOMCTL_assign_device failure to user.
>> This patch adds the check in xend before starting ioemu. In addtion,
>> adding the check in Xend can prompt user on the screen when the
>> device has already been assigned. Thanks.
> 
> Well, that''s too bad because I won''t take the original
patch. Why
> can''t the device be assigned by xend?
> 
>  -- Keir


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

That''s a lot better. I''ll take the Xen portion but the tools
changes need
more work:

The PCI-string parsing code cannot be placed in xc_private.[ch] and then
exported outside the library. Also using strtok() is invalid as it is not
thread-safe. You should use strtok_r() instead. I think you can get rid of
pci_count() altogether and have first_bdf/next_bdf return a boolean whether
they have reached the end of the string or not (strtok_r will return NULL
when the last token has been parsed). Then the caller would use them
something like:
 for (done = first_bdf(); !done; done = next_bdf())

Given the small amount of code for first_bdf/next_bdf, and given that
pci_count can be got rid of entirely, I would just duplicate that simple
strtok code in both the python wrapper and ioemu. I wouldn''t add that
rather
specific parsing code to libxc.

Please fix up and re-spin the tools part of the patch.

 -- Keir

On 18/10/07 02:51, "Han, Weidong" <weidong.han@intel.com> wrote:
> Keir,
> 
> Resend the patch. This patch is implemented according to your
> suggestion. Asssigns device in xend before starting ioemu, and add the
> check on DOMCTL_assign_device hypercall. Thanks.
> 
> -- Weidong
> 
> Keir Fraser wrote:
>> On 16/10/07 03:04, "Han, Weidong"
<weidong.han@intel.com> wrote:
>> 
>>>> The DOMCTL_assign_device should check whether the device is
already
>>>> assigned. This has the benefit that it can atomically
>>>> check-and-allocate, under the domctl lock.
>>>> 
>>>> You''ll have to work out how to propagate
DOMCTL_assign_device
>>>> failure to the user. Either you have to get the error out of
ioemu,
>>>> or perhaps you can assign the device in xend before starting
ioemu.
>>> 
>>> Yes, adding the check on DOMCTL_assign_device is simple, but I
didn''t
>>> find a good way to propagate DOMCTL_assign_device failure to user.
>>> This patch adds the check in xend before starting ioemu. In
addtion,
>>> adding the check in Xend can prompt user on the screen when the
>>> device has already been assigned. Thanks.
>> 
>> Well, that''s too bad because I won''t take the
original patch. Why
>> can''t the device be assigned by xend?
>> 
>>  -- Keir
> 


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

Keir, thanks for your valuable suggestion. I have changed the tools
part. As you said, PCI-string parsing is the small amount of code, so I
duplicate it in both python wrapper and ioemu. Attached patch is new
tools part patch. 

-- Weidong

Keir Fraser wrote:
> That''s a lot better. I''ll take the Xen portion but the
tools changes
> need more work:
> 
> The PCI-string parsing code cannot be placed in xc_private.[ch] and
> then exported outside the library. Also using strtok() is invalid as
> it is not thread-safe. You should use strtok_r() instead. I think you
> can get rid of pci_count() altogether and have first_bdf/next_bdf
> return a boolean whether they have reached the end of the string or
> not (strtok_r will return NULL when the last token has been parsed).
> Then the caller would use them something like:
>  for (done = first_bdf(); !done; done = next_bdf())
> 
> Given the small amount of code for first_bdf/next_bdf, and given that
> pci_count can be got rid of entirely, I would just duplicate that
> simple strtok code in both the python wrapper and ioemu. I
wouldn''t
> add that rather specific parsing code to libxc.
> 
> Please fix up and re-spin the tools part of the patch.
> 
>  -- Keir
> 
> On 18/10/07 02:51, "Han, Weidong" <weidong.han@intel.com>
wrote:
> 
>> Keir,
>> 
>> Resend the patch. This patch is implemented according to your
>> suggestion. Asssigns device in xend before starting ioemu, and add
>> the check on DOMCTL_assign_device hypercall. Thanks.
>> 
>> -- Weidong
>> 
>> Keir Fraser wrote:
>>> On 16/10/07 03:04, "Han, Weidong"
<weidong.han@intel.com> wrote:
>>> 
>>>>> The DOMCTL_assign_device should check whether the device is
>>>>> already assigned. This has the benefit that it can
atomically
>>>>> check-and-allocate, under the domctl lock.
>>>>> 
>>>>> You''ll have to work out how to propagate
DOMCTL_assign_device
>>>>> failure to the user. Either you have to get the error out
of
>>>>> ioemu, or perhaps you can assign the device in xend before
>>>>> starting ioemu. 
>>>> 
>>>> Yes, adding the check on DOMCTL_assign_device is simple, but I
>>>> didn''t find a good way to propagate
DOMCTL_assign_device failure
>>>> to user. This patch adds the check in xend before starting
ioemu.
>>>> In addtion, adding the check in Xend can prompt user on the
screen
>>>> when the device has already been assigned. Thanks.
>>> 
>>> Well, that''s too bad because I won''t take the
original patch. Why
>>> can''t the device be assigned by xend?
>>> 
>>>  -- Keir
>> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


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

Almost there, but you are misusing strtok_r(). The char** final argument has
to be a pointer to the _same_ char* across a sequence of calls to
strtok_r(). That is the whole point of it!

This means you cannot hide the char* inside first_bdf/next_bdf. You will
have to add a char** final argument to first_bdf/next_bdf, and have the
char* allocated in the caller''s stack frame.

If you try a multi-device pci string with the current patch, you''ll
probably
find you crash or something...

Try again. ;-)

 -- Keir

On 18/10/07 14:35, "Han, Weidong" <weidong.han@intel.com> wrote:
> Keir, thanks for your valuable suggestion. I have changed the tools
> part. As you said, PCI-string parsing is the small amount of code, so I
> duplicate it in both python wrapper and ioemu. Attached patch is new
> tools part patch.
> 
> -- Weidong
> 
> Keir Fraser wrote:
>> That''s a lot better. I''ll take the Xen portion but
the tools changes
>> need more work:
>> 
>> The PCI-string parsing code cannot be placed in xc_private.[ch] and
>> then exported outside the library. Also using strtok() is invalid as
>> it is not thread-safe. You should use strtok_r() instead. I think you
>> can get rid of pci_count() altogether and have first_bdf/next_bdf
>> return a boolean whether they have reached the end of the string or
>> not (strtok_r will return NULL when the last token has been parsed).
>> Then the caller would use them something like:
>>  for (done = first_bdf(); !done; done = next_bdf())
>> 
>> Given the small amount of code for first_bdf/next_bdf, and given that
>> pci_count can be got rid of entirely, I would just duplicate that
>> simple strtok code in both the python wrapper and ioemu. I
wouldn''t
>> add that rather specific parsing code to libxc.
>> 
>> Please fix up and re-spin the tools part of the patch.
>> 
>>  -- Keir
>> 
>> On 18/10/07 02:51, "Han, Weidong"
<weidong.han@intel.com> wrote:
>> 
>>> Keir,
>>> 
>>> Resend the patch. This patch is implemented according to your
>>> suggestion. Asssigns device in xend before starting ioemu, and add
>>> the check on DOMCTL_assign_device hypercall. Thanks.
>>> 
>>> -- Weidong
>>> 
>>> Keir Fraser wrote:
>>>> On 16/10/07 03:04, "Han, Weidong"
<weidong.han@intel.com> wrote:
>>>> 
>>>>>> The DOMCTL_assign_device should check whether the
device is
>>>>>> already assigned. This has the benefit that it can
atomically
>>>>>> check-and-allocate, under the domctl lock.
>>>>>> 
>>>>>> You''ll have to work out how to propagate
DOMCTL_assign_device
>>>>>> failure to the user. Either you have to get the error
out of
>>>>>> ioemu, or perhaps you can assign the device in xend
before
>>>>>> starting ioemu.
>>>>> 
>>>>> Yes, adding the check on DOMCTL_assign_device is simple,
but I
>>>>> didn''t find a good way to propagate
DOMCTL_assign_device failure
>>>>> to user. This patch adds the check in xend before starting
ioemu.
>>>>> In addtion, adding the check in Xend can prompt user on the
screen
>>>>> when the device has already been assigned. Thanks.
>>>> 
>>>> Well, that''s too bad because I won''t take the
original patch. Why
>>>> can''t the device be assigned by xend?
>>>> 
>>>>  -- Keir
>>> 
>> 
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
> 


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

Have fixed it. New patch is attached. Thanks.

Randy

Keir Fraser wrote:
> Almost there, but you are misusing strtok_r(). The char** final
> argument has to be a pointer to the _same_ char* across a sequence of
> calls to strtok_r(). That is the whole point of it!
> 
> This means you cannot hide the char* inside first_bdf/next_bdf. You
> will have to add a char** final argument to first_bdf/next_bdf, and
> have the char* allocated in the caller''s stack frame.
> 
> If you try a multi-device pci string with the current patch,
you''ll
> probably find you crash or something...
> 
> Try again. ;-)
> 
>  -- Keir
> 
> On 18/10/07 14:35, "Han, Weidong" <weidong.han@intel.com>
wrote:
> 
>> Keir, thanks for your valuable suggestion. I have changed the tools
>> part. As you said, PCI-string parsing is the small amount of code,
>> so I duplicate it in both python wrapper and ioemu. Attached patch
>> is new tools part patch. 
>> 
>> -- Weidong
>> 
>> Keir Fraser wrote:
>>> That''s a lot better. I''ll take the Xen portion
but the tools
>>> changes need more work: 
>>> 
>>> The PCI-string parsing code cannot be placed in xc_private.[ch] and
>>> then exported outside the library. Also using strtok() is invalid
as
>>> it is not thread-safe. You should use strtok_r() instead. I think
>>> you can get rid of pci_count() altogether and have
>>> first_bdf/next_bdf return a boolean whether they have reached the
>>> end of the string or not (strtok_r will return NULL when the last
>>> token has been parsed). Then the caller would use them something
>>>  like: for (done = first_bdf(); !done; done = next_bdf())
>>> 
>>> Given the small amount of code for first_bdf/next_bdf, and given
>>> that pci_count can be got rid of entirely, I would just duplicate
>>> that simple strtok code in both the python wrapper and ioemu. I
>>> wouldn''t add that rather specific parsing code to libxc.
>>> 
>>> Please fix up and re-spin the tools part of the patch.
>>> 
>>>  -- Keir
>>> 
>>> On 18/10/07 02:51, "Han, Weidong"
<weidong.han@intel.com> wrote:
>>> 
>>>> Keir,
>>>> 
>>>> Resend the patch. This patch is implemented according to your
>>>> suggestion. Asssigns device in xend before starting ioemu, and
add
>>>> the check on DOMCTL_assign_device hypercall. Thanks.
>>>> 
>>>> -- Weidong
>>>> 
>>>> Keir Fraser wrote:
>>>>> On 16/10/07 03:04, "Han, Weidong"
<weidong.han@intel.com> wrote:
>>>>> 
>>>>>>> The DOMCTL_assign_device should check whether the
device is
>>>>>>> already assigned. This has the benefit that it can
atomically
>>>>>>> check-and-allocate, under the domctl lock.
>>>>>>> 
>>>>>>> You''ll have to work out how to propagate
DOMCTL_assign_device
>>>>>>> failure to the user. Either you have to get the
error out of
>>>>>>> ioemu, or perhaps you can assign the device in xend
before
>>>>>>> starting ioemu.
>>>>>> 
>>>>>> Yes, adding the check on DOMCTL_assign_device is
simple, but I
>>>>>> didn''t find a good way to propagate
DOMCTL_assign_device failure
>>>>>> to user. This patch adds the check in xend before
starting ioemu.
>>>>>> In addtion, adding the check in Xend can prompt user on
the
>>>>>> screen when the device has already been assigned.
Thanks.
>>>>> 
>>>>> Well, that''s too bad because I won''t take
the original patch. Why
>>>>> can''t the device be assigned by xend?
>>>>> 
>>>>>  -- Keir
>>>> 
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xensource.com
>>> http://lists.xensource.com/xen-devel


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