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 5:13 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> On 21.04.15 at 16:42, <tklengyel@xxxxxxxxxxxxx> wrote:
> 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?
>

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

For this specific one - is there a reasonable use case? Other than
for host PFN, we have control over guest ones, and I'm not sure
managing a guest with GPFNs extending past 4 billion can be
expected to work if only this one hypercall got fixed. IOW I'm
expecting to NAK any such addition without proper rationale.

Jan

AFAIK everything works for me as it is already without this patch. I'm not sure if the cornercase of pfn's > 32-bit wide on ARM64 is actually something that would work/is supported at the moment.

Tamas

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