Grant unmap error checking in Dom0

Grant unmap error checking in Dom0

[Zoltan Kiss]

Hi,

Just out of curiosity I've checked how Dom0 handles errors during grant 
unmapping. Usually there is a BUG_ON(ret) for the return value of 
gnttab_unmap_refs in blkback and netback, gntdev drops just a WARN.
The return value can be non-zero only if Xen failed to copy the map 
operations back and forth to the guest supplied memory, so it's 
reasonable to crash there. However I'm wondering why gntdev is happy 
with just a WARN.
Another thing, we don't check the status of the operations if the return 
value is zero. We shouldn't normally do that, Xen logs info messages in 
some cases, but not always (e.g. XSM or IOMMU problems). For debugging 
purposes however it could be useful to have the ability to turn on 
checking in Dom0. A quick and dirty way to do this is to use 
printk_get_level to figure out if the loglevel is e.g. KERN_NOTICE or 
lower, but I'm sure there is a better way to do this :) It would be an 
overkill to introduce new config option, I'm thinking a runtime 
parameter to check in an unlikely(), so it won't cause performance 
penalty for normal operation. Any opinions on that?

Zoli

Re: Grant unmap error checking in Dom0

[Konrad Rzeszutek Wilk]

On Tue, Mar 18, 2014 at 01:21:18PM +0000, Zoltan Kiss wrote:
> Hi,
> 
> Just out of curiosity I've checked how Dom0 handles errors during
> grant unmapping. Usually there is a BUG_ON(ret) for the return value
> of gnttab_unmap_refs in blkback and netback, gntdev drops just a
> WARN.
> The return value can be non-zero only if Xen failed to copy the map
> operations back and forth to the guest supplied memory, so it's
> reasonable to crash there. However I'm wondering why gntdev is happy
> with just a WARN.
> Another thing, we don't check the status of the operations if the
> return value is zero. We shouldn't normally do that, Xen logs info
> messages in some cases, but not always (e.g. XSM or IOMMU problems).
> For debugging purposes however it could be useful to have the
> ability to turn on checking in Dom0. A quick and dirty way to do
> this is to use printk_get_level to figure out if the loglevel is
> e.g. KERN_NOTICE or lower, but I'm sure there is a better way to do
> this :) It would be an overkill to introduce new config option, I'm
> thinking a runtime parameter to check in an unlikely(), so it won't
> cause performance penalty for normal operation. Any opinions on
> that?

One can always just have printk(KERN_DEBUG and if the user did not
boot with 'debug' the messages go to /dev/null.

> 
> Zoli
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: Grant unmap error checking in Dom0

[Zoltan Kiss]

On 18/03/14 13:38, Konrad Rzeszutek Wilk wrote:
> On Tue, Mar 18, 2014 at 01:21:18PM +0000, Zoltan Kiss wrote:
>> Hi,
>>
>> Just out of curiosity I've checked how Dom0 handles errors during
>> grant unmapping. Usually there is a BUG_ON(ret) for the return value
>> of gnttab_unmap_refs in blkback and netback, gntdev drops just a
>> WARN.
>> The return value can be non-zero only if Xen failed to copy the map
>> operations back and forth to the guest supplied memory, so it's
>> reasonable to crash there. However I'm wondering why gntdev is happy
>> with just a WARN.
>> Another thing, we don't check the status of the operations if the
>> return value is zero. We shouldn't normally do that, Xen logs info
>> messages in some cases, but not always (e.g. XSM or IOMMU problems).
>> For debugging purposes however it could be useful to have the
>> ability to turn on checking in Dom0. A quick and dirty way to do
>> this is to use printk_get_level to figure out if the loglevel is
>> e.g. KERN_NOTICE or lower, but I'm sure there is a better way to do
>> this :) It would be an overkill to introduce new config option, I'm
>> thinking a runtime parameter to check in an unlikely(), so it won't
>> cause performance penalty for normal operation. Any opinions on
>> that?
>
> One can always just have printk(KERN_DEBUG and if the user did not
> boot with 'debug' the messages go to /dev/null.
Checking through the unmap_ops array itself can take a considerable 
amount of cycles, I think it would be better to avoid that.

Zoli

Re: Grant unmap error checking in Dom0

[Konrad Rzeszutek Wilk]

On Tue, Mar 18, 2014 at 03:42:56PM +0000, Zoltan Kiss wrote:
> On 18/03/14 13:38, Konrad Rzeszutek Wilk wrote:
> >On Tue, Mar 18, 2014 at 01:21:18PM +0000, Zoltan Kiss wrote:
> >>Hi,
> >>
> >>Just out of curiosity I've checked how Dom0 handles errors during
> >>grant unmapping. Usually there is a BUG_ON(ret) for the return value
> >>of gnttab_unmap_refs in blkback and netback, gntdev drops just a
> >>WARN.
> >>The return value can be non-zero only if Xen failed to copy the map
> >>operations back and forth to the guest supplied memory, so it's
> >>reasonable to crash there. However I'm wondering why gntdev is happy
> >>with just a WARN.
> >>Another thing, we don't check the status of the operations if the
> >>return value is zero. We shouldn't normally do that, Xen logs info
> >>messages in some cases, but not always (e.g. XSM or IOMMU problems).
> >>For debugging purposes however it could be useful to have the
> >>ability to turn on checking in Dom0. A quick and dirty way to do
> >>this is to use printk_get_level to figure out if the loglevel is
> >>e.g. KERN_NOTICE or lower, but I'm sure there is a better way to do
> >>this :) It would be an overkill to introduce new config option, I'm
> >>thinking a runtime parameter to check in an unlikely(), so it won't
> >>cause performance penalty for normal operation. Any opinions on
> >>that?
> >
> >One can always just have printk(KERN_DEBUG and if the user did not
> >boot with 'debug' the messages go to /dev/null.
> Checking through the unmap_ops array itself can take a considerable
> amount of cycles, I think it would be better to avoid that.


I was thinking it was triggered on your 'return value'.

In this case I would say just use asm goto (jump labels) and maybe have
it turned on via debugfs? That would make all of this based on the
CONFIG_XEN_DEBUGFS?


> 
> Zoli
> 

Re: Grant unmap error checking in Dom0

[Zoltan Kiss]

On 18/03/14 15:54, Konrad Rzeszutek Wilk wrote:
> On Tue, Mar 18, 2014 at 03:42:56PM +0000, Zoltan Kiss wrote:
>> On 18/03/14 13:38, Konrad Rzeszutek Wilk wrote:
>>> On Tue, Mar 18, 2014 at 01:21:18PM +0000, Zoltan Kiss wrote:
>>>> Hi,
>>>>
>>>> Just out of curiosity I've checked how Dom0 handles errors during
>>>> grant unmapping. Usually there is a BUG_ON(ret) for the return value
>>>> of gnttab_unmap_refs in blkback and netback, gntdev drops just a
>>>> WARN.
>>>> The return value can be non-zero only if Xen failed to copy the map
>>>> operations back and forth to the guest supplied memory, so it's
>>>> reasonable to crash there. However I'm wondering why gntdev is happy
>>>> with just a WARN.
>>>> Another thing, we don't check the status of the operations if the
>>>> return value is zero. We shouldn't normally do that, Xen logs info
>>>> messages in some cases, but not always (e.g. XSM or IOMMU problems).
>>>> For debugging purposes however it could be useful to have the
>>>> ability to turn on checking in Dom0. A quick and dirty way to do
>>>> this is to use printk_get_level to figure out if the loglevel is
>>>> e.g. KERN_NOTICE or lower, but I'm sure there is a better way to do
>>>> this :) It would be an overkill to introduce new config option, I'm
>>>> thinking a runtime parameter to check in an unlikely(), so it won't
>>>> cause performance penalty for normal operation. Any opinions on
>>>> that?
>>>
>>> One can always just have printk(KERN_DEBUG and if the user did not
>>> boot with 'debug' the messages go to /dev/null.
>> Checking through the unmap_ops array itself can take a considerable
>> amount of cycles, I think it would be better to avoid that.
>
>
> I was thinking it was triggered on your 'return value'.
>
> In this case I would say just use asm goto (jump labels) and maybe have
> it turned on via debugfs? That would make all of this based on the
> CONFIG_XEN_DEBUGFS?
I would prefer avoiding the introduction of a new config parameter, if 
possible. A module parameter rather, but as I'm lazy, and the importance 
of this checking is not too high, the best would be to bind this to some 
general "is debugging turned on?" option.
If there would be a shorthand function "is_debugfs_mounted", that would 
be reasonable, but I couldn't find it.

Zoli

Re: Grant unmap error checking in Dom0

[Konrad Rzeszutek Wilk]

On Tue, Mar 18, 2014 at 09:56:48PM +0000, Zoltan Kiss wrote:
> On 18/03/14 15:54, Konrad Rzeszutek Wilk wrote:
> >On Tue, Mar 18, 2014 at 03:42:56PM +0000, Zoltan Kiss wrote:
> >>On 18/03/14 13:38, Konrad Rzeszutek Wilk wrote:
> >>>On Tue, Mar 18, 2014 at 01:21:18PM +0000, Zoltan Kiss wrote:
> >>>>Hi,
> >>>>
> >>>>Just out of curiosity I've checked how Dom0 handles errors during
> >>>>grant unmapping. Usually there is a BUG_ON(ret) for the return value
> >>>>of gnttab_unmap_refs in blkback and netback, gntdev drops just a
> >>>>WARN.
> >>>>The return value can be non-zero only if Xen failed to copy the map
> >>>>operations back and forth to the guest supplied memory, so it's
> >>>>reasonable to crash there. However I'm wondering why gntdev is happy
> >>>>with just a WARN.
> >>>>Another thing, we don't check the status of the operations if the
> >>>>return value is zero. We shouldn't normally do that, Xen logs info
> >>>>messages in some cases, but not always (e.g. XSM or IOMMU problems).
> >>>>For debugging purposes however it could be useful to have the
> >>>>ability to turn on checking in Dom0. A quick and dirty way to do
> >>>>this is to use printk_get_level to figure out if the loglevel is
> >>>>e.g. KERN_NOTICE or lower, but I'm sure there is a better way to do
> >>>>this :) It would be an overkill to introduce new config option, I'm
> >>>>thinking a runtime parameter to check in an unlikely(), so it won't
> >>>>cause performance penalty for normal operation. Any opinions on
> >>>>that?
> >>>
> >>>One can always just have printk(KERN_DEBUG and if the user did not
> >>>boot with 'debug' the messages go to /dev/null.
> >>Checking through the unmap_ops array itself can take a considerable
> >>amount of cycles, I think it would be better to avoid that.
> >
> >
> >I was thinking it was triggered on your 'return value'.
> >
> >In this case I would say just use asm goto (jump labels) and maybe have
> >it turned on via debugfs? That would make all of this based on the
> >CONFIG_XEN_DEBUGFS?
> I would prefer avoiding the introduction of a new config parameter,
> if possible. A module parameter rather, but as I'm lazy, and the
> importance of this checking is not too high, the best would be to
> bind this to some general "is debugging turned on?" option.
> If there would be a shorthand function "is_debugfs_mounted", that
> would be reasonable, but I couldn't find it.

In that case I would just say go for 'early_bootparam' or a normal
parameter and make it use the jump label machinery. 

See arch/x86/xen/spinlock.c for the xen_pvspin parameter as an example.
> 
> Zoli