Re: [Xen-devel] [PATCH] grant table map error in __gnttab_map_grant_ref

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.02.12 at 12:38, Liuyongan <liuyongan@xxxxxxxxxx> wrote:

>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: Monday, February 06, 2012 5:03 PM
>> To: Liuyongan
>> Cc: hanweidong; Zhanghaoyu; Wang Liang; Qianhuibin; Andres Lagar-
>> Cavilla; xen-devel@xxxxxxxxxxxxxxxxxxx 
>> Subject: Re: [Xen-devel] [PATCH] grant table map error in
>> __gnttab_map_grant_ref
>> 
>>    >>> On 04.02.12 at 03:43, Liuyongan <liuyongan@xxxxxxxxxx> wrote:
>> > In file grant_table.c function __gnttab_map_grant_ref, if
>> __get_paged_frame
>> > failed, the effect of _set_status  previously called should be
>> rollback, so
>> > the flag GTF_reading and _GTF_writing will be recovered.
>> 
>> Not knowing too much about these, but isn't __acquire_grant_for_copy()
>> in need of a similar adjustment?
>   Well, I need to check it further.
>> 
>> Further, aren't the status flags cumulative (and hence isn't it wrong
>> to
>> simply clear flags without considering their original state)? 
>   When undo_out branch is executed, the flag set by _set_status function
>   will be rolled back by:
>        if ( !(op->flags & GNTMAP_readonly) &&
>          !(act->pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
>         gnttab_clear_flag(_GTF_writing, status);
> 
>        if ( !act->pin )
>         gnttab_clear_flag(_GTF_reading, status);
>   so action added by this patch should be correct. 

But this is not a roll-back, here the flags get simply cleared (whereas
"roll-back" to me means the restoration of their previous value).

>   And anyway, the  reading and writing flag should be recovered when mapping 
>   grant reference failed, so the up layer(eg. Netback driver) can decide 
>   freely whether retry mapping or fail directly.

Those flags, afaiu, are managed by Xen, so the layer issuing the
mapping operation shouldn't be concerned.

Jan

>> (I realize
>> that this is not a new issue introduced with the proposed patch, but
>> certainly with more ways of getting that code path run through, it
>> becomes more important for it to be correct.)
>> 
>> Jan
>> 
>> > Signed-off-by: Haoyu Zhang<haoyu.zhang@xxxxxxxxxx>; Liang
>> > Wang<hzwangliang.wang@xxxxxxxxxx>
>> >
>> > diff -r cbed91e1c878 xen-4.1.2/xen/common/grant_table.c
>> > --- a/xen-4.1.2/xen/common/grant_table.c   Sat Feb 04 18:36:13
>> 2012 +0800
>> > +++ b/xen-4.1.2/xen/common/grant_table.c   Sat Feb 04 18:40:02
>> 2012 +0800
>> > @@ -566,7 +566,7 @@
>> >              gfn = sha1 ? sha1->frame : sha2->full_page.frame;
>> >              rc = __get_paged_frame(gfn, &frame, !!(op->flags &
>> > GNTMAP_readonly), rd);
>> >              if ( rc != GNTST_okay )
>> > -                goto unlock_out;
>> > +                goto unlock_out_clear;
>> >              act->gfn = gfn;
>> >              act->domid = ld->domain_id;
>> >              act->frame = frame;
>> > @@ -721,7 +721,8 @@
>> >      if ( op->flags & GNTMAP_host_map )
>> >          act->pin -= (op->flags & GNTMAP_readonly) ?
>> >              GNTPIN_hstr_inc : GNTPIN_hstw_inc;
>> > -
>> > +
>> > + unlock_out_clear:
>> >      if ( !(op->flags & GNTMAP_readonly) &&
>> >           !(act->pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
>> >          gnttab_clear_flag(_GTF_writing, status);
>> 
>> 




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