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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-08-07 at 14:13 +0100, Jan Beulich wrote:
> >>> On 07.08.12 at 14:36, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> > 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.
> 
> ??? This structure is already shared between 32-bit and 64-bit
> implementations, and the fields are intentionally fitting both by
> using "unsigned long". Are you suggesting to add #ifdef-ery to
> public/xen.h?

Oh, I thought multicall_entry was in an arch header.

In any case I would have expected a typedef xen_multicall_arg_t or
something.

> 
> Jan
> 



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