Re: [Xen-devel] [PATCH v2 5/5] xen: replace XEN_GUEST_HANDLE with XEN_GUEST_HANDLE_PARAM when appropriate

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.08.12 at 17:42, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
wrote:
> On Tue, 14 Aug 2012, Jan Beulich wrote:
>> >>> On 14.08.12 at 14:56, Stefano Stabellini 
>> >>> <stefano.stabellini@xxxxxxxxxxxxx> 
> wrote:
>> > On Tue, 14 Aug 2012, Jan Beulich wrote:
>> >> Perhaps we have a different understanding of embedded fields:
>> >> I'm thinking of structure field having XEN_GUEST_HANDLE() type.
>> >> An example would be struct mmuext_op's vcpumask field, which
>> >> is being passed to vcpumask_to_pcpumask(). This must remain to
>> >> be possible (and not just in x86-specific code, where it's mere luck
>> >> that both are really identical).
>> > 
>> > Thanks for the concrete example; glancing through the common code I
>> > didn't find any examples like this.
>> > As I wrote in the follow up email, guest_handle_cast is just what we
>> > need:
>> > 
>> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> > index 4d72700..70ffa58 100644
>> > --- a/xen/arch/x86/mm.c
>> > +++ b/xen/arch/x86/mm.c
>> > @@ -3198,7 +3198,9 @@ int do_mmuext_op(
>> >          {
>> >              cpumask_t pmask;
>> >  
>> > -            if ( unlikely(vcpumask_to_pcpumask(d, op.arg2.vcpumask, 
>> > &pmask)) 
> )
>> > +            if ( unlikely(vcpumask_to_pcpumask(d,
>> > +                            guest_handle_cast(op.arg2.vcpumask, 
> const_void),
>> 
>> No, the conversion should explicitly _not_ require specification
>> of the type, i.e. this should not be a true cast. Type safety
>> (checked by the compiler) can only be achieved if no intermediate
>> cast is involved.
> 
> guest_handle_cast is implemented as:
> 
> #define guest_handle_cast(hnd, type) ({         \
>     type *_x = (hnd).p;                         \
>     (XEN_GUEST_HANDLE_PARAM(type)) { _x };      \
> })
> 
> as you can see there is actually no explicit cast involved.
> If you specify the wrong type the compiler would fail at:
> 
> type *_x = (hnd).p;

Except if, as in your example, "type" is (a qualified version of)
"void"...

> I think that having to specify the type as parameter is acceptable if it
> makes up for simpler code over all.
> 
> The alternative would be something like the following:
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 4d72700..e6685c7 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3197,8 +3197,11 @@ int do_mmuext_op(
>          case MMUEXT_INVLPG_MULTI:
>          {
>              cpumask_t pmask;
> +            XEN_GUEST_HANDLE_PARAM(const_void) param;
>  
> -            if ( unlikely(vcpumask_to_pcpumask(d, op.arg2.vcpumask, &pmask)) 
> )
> +            set_xen_guest_handle(param, op.arg2.vcpumask.p);
> +
> +            if ( unlikely(vcpumask_to_pcpumask(d, param, &pmask)) )

No, I would expect this to be possible without an intermediate
variable (i.e. similar to the guest_handle_cast() approach, just
without specifying the type). And in no case should there be
an open coded access to the "p" member.

>              {
>                  okay = 0;
>                  break;
> 
> but I think it makes the code worse.

Agreed, this variant definitely looked worse.

Jan


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