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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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;

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)) )
             {
                 okay = 0;
                 break;

but I think it makes the code worse.

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