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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-08-07 at 13:08 +0100, Stefano Stabellini wrote:
> On Mon, 6 Aug 2012, Jan Beulich wrote:
> > >>> On 06.08.12 at 16:12, Stefano Stabellini 
> > >>> <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > > There are still few unsigned long in the xen public interface: replace
> > > them with xen_ulong_t.
> > > 
> > > Also typedef xen_ulong_t to uint64_t on ARM.
> > 
> > While this change by itself already looks suspicious to me, I don't
> > follow what the global replacement is going to be good for, in
> > particular when done in places that ARM should be completely
> > ignorant of, e.g.
> 
> See below
> 
> 
> > > --- 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.
> 
> 
> > > --- a/xen/include/public/xen.h
> > > +++ b/xen/include/public/xen.h
> > > @@ -518,8 +518,8 @@ DEFINE_XEN_GUEST_HANDLE(mmu_update_t);
> > >   * NB. The fields are natural register size for this architecture.
> > >   */
> > >  struct multicall_entry {
> > > -    unsigned long op, result;
> > > -    unsigned long args[6];
> > > +    xen_ulong_t op, result;
> > > +    xen_ulong_t args[6];
> > 
> > And here I really start to wonder - what use is it to put all 64-bit
> > values here on a 32-bit arch? You certainly know a lot more about
> > ARM than me, but this looks pretty inefficient, the more that
> > you'll have to deal with checking the full values when converting
> > to e.g. pointers anyway, in order to avoid behavioral differences
> > between running on a 32- or 64-bit host. Zero-extending from
> > 32-bits when in a 64-bit hypervisor wouldn't have this problem.
> 
> Actually the multicall_entry change is wrong, thanks for point it out.
> 
> The idea is that pointers are always 8 bytes sized and 8 bytes aligned,
> except when they are passed as hypercall arguments, in which case a 32
> bit guest would use 32 bit pointers and a 64 bit guest would use 64 bit
> pointers.
> 
> Considering that each field of a multicall_entry is usually passed as an
> hypercall parameter, they should all remain unsigned long.

If possible, please make them an explicitly sized type, even if it is
now 32 bits.

Ian.




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