[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH, RFC] Re: x86: gnttab_clear_flag() abusing clear_bit()



>>> On 07.02.12 at 06:10, Keir Fraser <keir.xen@xxxxxxxxx> wrote:
> On 07/02/2012 10:34, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>>>>> On 06.02.12 at 18:06, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>>> Back in c/s 17194:af33f2054f47 bitops got restricted to 4-bytes and
>>> larger operands only. gnttab_clear_flag() cheats in casting a uint16_t *
>>> to unsigned long * - how is that not a problem in the context of the
>>> goal that said c/s set, in particular for v2 of the interface? (Given that
>>> this is being implemented as arch-specific routine - so far for reasons
>>> that escape me - this should be simple to fix by using inline assembly
>>> rather than clear_bit().)
>>> 
>>> Further I just spotted one instance where the or of two bit positions
>>> gets passed to this function - that's quite definitely a bug I would say.
>>> 
>>> And, quite the opposite, __fixup_status_for_pin() ands out the
>>> negation of bit positions rather than masks... (Which also raises
>>> the question whether it really would need to be clear_bit() above
>>> instead of the cheaper __clear_bit().)
>> 
>> Below the tentative fix for all of the above problems. In the light
>> of the comment at the top of x86's bitops.h I'm awaiting our gcc
>> experts' response regarding the safety of using "+m" here.
> 
> Looks fine to me, in principle. I would add a comment to the x86
> gnttab_clear_flag() explaining why we have to open code something that looks
> a lot like clear_bit().

That one I already did, will submit soon (desiring clarification on the
below).

As to the "+m" constraint - I'm being told that "+m" (var) is equivalent
to "=m" (var) : "m" (var), no matter what the documentation says
regarding '+' (but they're also not seeing a need to adjust the docs
accordingly).

The question is whether we should go with the (documentation-wise
correct) form, or the shorter one (which they're unlikely to change
the meaning of, given in how many places "+m" is used in e.g. Linux).

Jan


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.