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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

Jan


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