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

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2015-04-21 at 15:29 +0100, Jan Beulich wrote:
> >>> On 21.04.15 at 16:24, <ian.campbell@citrix.com> wrote:
> > On Tue, 2015-04-21 at 15:14 +0100, Jan Beulich wrote:
> >> >>> On 21.04.15 at 15:23, <ian.campbell@citrix.com> 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@sec.in.tum.de>
> >> >> 
> >> >> 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?
> > 
> > Isn't this still an issue for 32-bit toolstack (long == 4 bytes) on a 64
> > bit host (maximum pfn more than 2^32)?
> 
> It is, but do we really want to introduce other than just compat
> mode helper interfaces (i.e. leaving the current ones alone, and
> perhaps even making the new ones tools only) if we really care
> about such setups in the first place?

IIRC the original impetus for at least one part of this interface was
for in guest use. I don't recall what the use was, I think it was the
max pfn one which was used though.

Perhaps in that case we can assume that a signed long is sufficient for
any pfn they might see. But is that true? A 32-bit guest could see
PFN>2^31 I think.

Perhaps we should add these fixed version as a tools interface and
consider only doing a fixed guest visible version of the max pfn one?
Modulo confirming that I'm not misremembering...

Ian.