Re: [Xen-devel] [PATCH V15 5/9] xen: Make gpfn related memops compatible with wider return values

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.04.15 at 16:33, <tklengyel@xxxxxxxxxxxxx> wrote:
> On Tue, Apr 21, 2015 at 4:14 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 21.04.15 at 15:23, <ian.campbell@xxxxxxxxxx> wrote:
>> > On Mon, 2015-04-20 at 16:22 +0100, Andrew Cooper wrote:
>> >> On 20/04/15 16:06, Tamas K Lengyel wrote:
>> >> > The current implementation of three memops,
>> XENMEM_current_reservation,
>> >> > XENMEM_maximum_reservation and XENMEM_maximum_gpfn return values as an
>> >> > int. However, in ARM64 we could potentially have 36-bit pfn's, thus
>> >> > in preparation for the ARM patch, in this patch we update the existing
>> >> > memop routines to use a struct, xen_get_gpfn, to exchange the gpfn
>> info
>> >> > as a uin64_t.
>> >> >
>> >> > This patch also adds error checking on the toolside in case the memop
>> >> > fails.
>> >> >
>> >> > Signed-off-by: Tamas K Lengyel <tklengyel@xxxxxxxxxxxxx>
>> >>
>> >> XENMEM, unlikely domctls/sysctls is a guest-visible stable ABI/API.
>> >>
>> >> You cannot make adjustments like this, but you can add a brand new op
>> >> with appropriate parameters and list the old ops as deprecated.
>> >
>> > Right. For the benefit of callers using the old API it seems what we
>> > usually do is rename the old op XENMEM_foo_compat and use the name with
>> > a new number for the new functionality, then use a
>> > __XEN_INTERFACE_VERSION__ to #define back to the old name.
>> >
>> > The handling of __HYPERVISOR_sched_op in public/xen.h seems like a
>> > reasonable example, I couldn't find one specifically for the memory ops.
>>
>> And there's no need to afaict: This complication isn't needed in the
>> first place. The patch's context already makes this clear:
>>
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -838,12 +838,16 @@ long do_memory_op(unsigned long cmd,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>
>> Note the "long" return type. Yet the patch description, for
>> whatever reason, claims the hypercall to only return an "int".
>> Maybe because (as pointed out before) the respective Linux
>> hypercall stub wrongly an "int" return type?
> 
> The privcmd driver on Linux certainly does return an int via the ioctl.

That's clearly a bug in Linux:

static long privcmd_ioctl(struct file *file,
                          unsigned int cmd, unsigned long data)
{
        int ret = -ENOSYS;
        void __user *udata = (void __user *) data;

        switch (cmd) {
        case IOCTL_PRIVCMD_HYPERCALL:
                ret = privcmd_ioctl_hypercall(udata);
                break;
[...]
        return ret;
}

Why in the world is ret an int? That's certainly not something
inherited from XenoLinux, where all involved types are long.

Jan


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