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

Â

(Tamas, while I had asked you to avoid Cc-ing me on ARM only
patches, I would have been able to spot this earlier if you had
Cc-ed me on this non-ARM change. Again, please Cc
maintainers, but especially on larger series make sure you
don't Cc everyone on every patch. I'm sure avoiding the extra
mail load is being appreciated not just by me.)

Jan

Ack, I took you off only after you repeatedly asked me to exclude you from this series. I agree that patches should be only cc-d to the respective maintainers and not all maintainers who are involved in the series. I'll look into how that could be automated because doing that by hand is not very pleasant.

Thanks,

Tamas

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