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


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