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

[Tamas K Lengyel <tklengyel@xxxxxxxxxxxxx>]

On Tue, Apr 21, 2015 at 4:29 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> On 21.04.15 at 16:24, <ian.campbell@xxxxxxxxxx> wrote:
> On Tue, 2015-04-21 at 15:14 +0100, Jan Beulich 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?
>
> 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?

Jan

At the moment I just followed Andrew's advice and will introduce a new XENMEM_maximum_gpfn2 memop that returns the gpfn in a struct as xen_pfn_t. The old memops I'll leave untouched if that's OK.

Tamas

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