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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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.

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