Re: [PATCH 3/5] xen: few more xen_ulong_t substitutions

[Ian Campbell <Ian.Campbell@citrix.com>]

On Tue, 2012-08-07 at 13:54 +0100, Jan Beulich wrote:
> >>> On 07.08.12 at 14:08, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > On Mon, 6 Aug 2012, Jan Beulich wrote:
> >> >>> On 06.08.12 at 16:12, Stefano Stabellini <stefano.stabellini@eu.citrix.com>  wrote:
> >> > --- a/xen/include/public/physdev.h
> >> > +++ b/xen/include/public/physdev.h
> >> > @@ -124,7 +124,7 @@ DEFINE_XEN_GUEST_HANDLE(physdev_set_iobitmap_t);
> >> >  #define PHYSDEVOP_apic_write             9
> >> >  struct physdev_apic {
> >> >      /* IN */
> >> > -    unsigned long apic_physbase;
> >> > +    xen_ulong_t apic_physbase;
> >> >      uint32_t reg;
> >> >      /* IN or OUT */
> >> >      uint32_t value;
> >> >...
> > 
> > This change is actually not required, considering that ARM doesn't have
> > an APIC. I changed apic_physbase to xen_ulong_t only for consistency,
> > but it wouldn't make any difference for ARM (or x86).
> > If you think that it is better to keep it unsigned long, I'll remove
> > this chunk for the patch.
> 
> I'd prefer that. Also any others that you may not actually need
> to be converted.
> 
> Also, seems I was misremembering how PPC dealt with this - they
> indeed had xen_ulong_t defined to be a 64-bit value too.
> 
> > Considering that each field of a multicall_entry is usually passed as an
> > hypercall parameter, they should all remain unsigned long.
> 
> That'll give you subtle bugs I'm afraid: do_memory_op()'s
> encoding of a continuation start extent (into the 'cmd' value),
> for example, depends on being able to store the full value into
> the command field of the multicall structure. The limit checking
> of the permitted number of extents therefore is different
> between native (ULONG_MAX >> MEMOP_EXTENT_SHIFT) and
> compat (UINT_MAX >> MEMOP_EXTENT_SHIFT).

On compat this would be 2^(32-6) == 67 and a bit million extents. I
think we can live with this limit on 64bit ARM too.

We could have lived with it on 64bit X86 too but by the time we noticed
it was too late so we have to live with it.

>  I would
> neither find it very appealing to have do_memory_op() adjusted
> for dealing with this new special case, nor am I sure that's the
> only place your approach would cause problems if you excluded
> the multicall structure from the model change.

On the other hand it doesn't seem right for the multicall args to be
able to represent values which you couldn't pass to the regular
hypercall itself (since the 32 bit args are only 32 bit).

Perhaps if use of the upper portions for 32 bit guests were restricted
for the use of continuations only that might work -- it would rely on
the top half of the 64-bit x0 register surviving a trip back to 32 bit
mode and into hypervisor mode again. I think that can be expected (but
I'd have to double check a AArch64 docs to be sure). On the other-other
hand that would mean that a 32-on-32 guest would see different things to
a 32-on-64 guest which is bad.

I don't expect we will be able to get away with no compat layer what so
ever. At the minimum there will have to be compat hypercall entry points
which correctly extend the 32 bit arguments to 64 bit, and do the same
for the return value etc but if we can avoid the majority of the struct
munging (ideally completely avoiding the need for a hypercall arguments
translation area) then I think that is worth doing.

Ian.