[PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used

[PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used

[Ian Campbell]

A few folks have been caught out by OSes which call e.g.
HYPERVISOR_event_channel_op_compat which has been deprecated since
3.2.2 (i.e. long before Xen on ARM). Existing x86 code can still
safely and quietly using those calls, waiting for an unsuspecting ARM
porter to turn up and trip over it. This turns out to be rather
perplexing when it happens, since it can be obscured e.g. by various
conditionals like __XEN_INTERFACE_VERSION__ what is actually being
called.

Note that I'm making a distinction here between hypercalls which are
simply not used/implemented on arm (yet) and those which were
deprecated and replaced by a newer variant prior to Xen on ARM even
being invented.  The latter will never be implemented on ARM and have
non-deprecated aliases leading to confusion so those are the ones for
which a warning is useful.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
---
RFC since I'm not sure how extreme our reaction ought to be here, e.g.
I considered domain_crash() or even panic() when in a debug build. A
XENLOG_DEBUG message is about the most benign of the options.

Jan/Keir, although this is ARM specific I'd welcome your views as
x86/REST maintainers.

Ard, I've not actually run this -- any chance you could re-b0rk your
Tianocore image and give it a go?
---
 xen/arch/arm/traps.c |   30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ad046e8..89cbde6 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1148,6 +1148,22 @@ die:
 }
 #endif
 
+static register_t do_deprecated_hypercall(void)
+{
+    struct cpu_user_regs *regs = guest_cpu_user_regs();
+    const register_t op =
+#ifdef CONFIG_ARM_64
+        !is_32bit_domain(current->domain) ?
+            regs->x16
+        :
+#endif
+            regs->r12;
+
+    gdprintk(XENLOG_DEBUG, "%pv: deprecated hypercall %ld\n",
+             current, (unsigned long)op);
+    return -ENOSYS;
+}
+
 typedef register_t (*arm_hypercall_fn_t)(
     register_t, register_t, register_t, register_t, register_t);
 
@@ -1167,15 +1183,29 @@ typedef struct {
         .fn = (arm_hypercall_fn_t) &do_arm_ ## _name,                \
         .nr_args = _nr_args,                                         \
     }
+/*
+ * Only use this for hypercalls which were deprecated (i.e. replaced
+ * by something else) before Xen on ARM was created, i.e. *not* for
+ * hypercalls which are simply not yet used on ARM.
+ */
+#define HYPERCALL_DEPRECATED(_name, _nr_args)                   \
+    [ __HYPERVISOR_##_name ] = {                                \
+        .fn = (arm_hypercall_fn_t) &do_deprecated_hypercall,    \
+        .nr_args = _nr_args,                                    \
+    }
+
 static arm_hypercall_t arm_hypercall_table[] = {
     HYPERCALL(memory_op, 2),
     HYPERCALL(domctl, 1),
     HYPERCALL(sched_op, 2),
+    HYPERCALL_DEPRECATED(sched_op_compat, 2),
     HYPERCALL(console_io, 3),
     HYPERCALL(xen_version, 2),
     HYPERCALL(xsm_op, 1),
     HYPERCALL(event_channel_op, 2),
+    HYPERCALL_DEPRECATED(event_channel_op_compat, 1),
     HYPERCALL(physdev_op, 2),
+    HYPERCALL_DEPRECATED(physdev_op_compat, 1),
     HYPERCALL(sysctl, 2),
     HYPERCALL(hvm_op, 2),
     HYPERCALL(grant_table_op, 3),
-- 
1.7.10.4

Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used

[Jan Beulich]

>>> On 20.01.15 at 11:52, <ian.campbell@citrix.com> wrote:
> RFC since I'm not sure how extreme our reaction ought to be here, e.g.
> I considered domain_crash() or even panic() when in a debug build. A
> XENLOG_DEBUG message is about the most benign of the options.

And I think it shouldn't be more than that in the first round. We
may want to consider logging a message on x86 too, but of
course only optionally depending on a command line option.

> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1148,6 +1148,22 @@ die:
>  }
>  #endif
>  
> +static register_t do_deprecated_hypercall(void)
> +{
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    const register_t op =
> +#ifdef CONFIG_ARM_64
> +        !is_32bit_domain(current->domain) ?
> +            regs->x16
> +        :
> +#endif
> +            regs->r12;
> +
> +    gdprintk(XENLOG_DEBUG, "%pv: deprecated hypercall %ld\n",
> +             current, (unsigned long)op);

If this was x86 code, I'd complain about the cast...

> @@ -1167,15 +1183,29 @@ typedef struct {
>          .fn = (arm_hypercall_fn_t) &do_arm_ ## _name,                \
>          .nr_args = _nr_args,                                         \
>      }
> +/*
> + * Only use this for hypercalls which were deprecated (i.e. replaced
> + * by something else) before Xen on ARM was created, i.e. *not* for
> + * hypercalls which are simply not yet used on ARM.
> + */
> +#define HYPERCALL_DEPRECATED(_name, _nr_args)                   \
> +    [ __HYPERVISOR_##_name ] = {                                \
> +        .fn = (arm_hypercall_fn_t) &do_deprecated_hypercall,    \

... and the redundant &.

Jan

Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used

[Ian Campbell]

On Tue, 2015-01-20 at 11:05 +0000, Jan Beulich wrote:
> >>> On 20.01.15 at 11:52, <ian.campbell@citrix.com> wrote:
> > RFC since I'm not sure how extreme our reaction ought to be here, e.g.
> > I considered domain_crash() or even panic() when in a debug build. A
> > XENLOG_DEBUG message is about the most benign of the options.
> 
> And I think it shouldn't be more than that in the first round. We
> may want to consider logging a message on x86 too, but of
> course only optionally depending on a command line option.

OK.

> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1148,6 +1148,22 @@ die:
> >  }
> >  #endif
> >  
> > +static register_t do_deprecated_hypercall(void)
> > +{
> > +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> > +    const register_t op =
> > +#ifdef CONFIG_ARM_64
> > +        !is_32bit_domain(current->domain) ?
> > +            regs->x16
> > +        :
> > +#endif
> > +            regs->r12;
> > +
> > +    gdprintk(XENLOG_DEBUG, "%pv: deprecated hypercall %ld\n",
> > +             current, (unsigned long)op);
> 
> If this was x86 code, I'd complain about the cast...

The correct format code is PRIregister (since register_t can be 32- or
64-bit for arm32 vs arm64 respectively), but it is in hex and xen.h
lists __HYPERVISOR_* in decimal so that's what I wanted to print for
each of manually looking.

> > @@ -1167,15 +1183,29 @@ typedef struct {
> >          .fn = (arm_hypercall_fn_t) &do_arm_ ## _name,                \
> >          .nr_args = _nr_args,                                         \
> >      }
> > +/*
> > + * Only use this for hypercalls which were deprecated (i.e. replaced
> > + * by something else) before Xen on ARM was created, i.e. *not* for
> > + * hypercalls which are simply not yet used on ARM.
> > + */
> > +#define HYPERCALL_DEPRECATED(_name, _nr_args)                   \
> > +    [ __HYPERVISOR_##_name ] = {                                \
> > +        .fn = (arm_hypercall_fn_t) &do_deprecated_hypercall,    \
> 
> ... and the redundant &.

It's consistent with the other variants of this macro.

Ian.

Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used

[Jan Beulich]

>>> On 20.01.15 at 12:11, <Ian.Campbell@citrix.com> wrote:
> On Tue, 2015-01-20 at 11:05 +0000, Jan Beulich wrote:
>> >>> On 20.01.15 at 11:52, <ian.campbell@citrix.com> wrote:
>> > +static register_t do_deprecated_hypercall(void)
>> > +{
>> > +    struct cpu_user_regs *regs = guest_cpu_user_regs();
>> > +    const register_t op =
>> > +#ifdef CONFIG_ARM_64
>> > +        !is_32bit_domain(current->domain) ?
>> > +            regs->x16
>> > +        :
>> > +#endif
>> > +            regs->r12;
>> > +
>> > +    gdprintk(XENLOG_DEBUG, "%pv: deprecated hypercall %ld\n",
>> > +             current, (unsigned long)op);
>> 
>> If this was x86 code, I'd complain about the cast...
> 
> The correct format code is PRIregister (since register_t can be 32- or
> 64-bit for arm32 vs arm64 respectively), but it is in hex and xen.h
> lists __HYPERVISOR_* in decimal so that's what I wanted to print for
> each of manually looking.

Which makes sense. I'd rather question whether PRIregister is
badly defined then, in that it not just specifies an eventual size
prefix. I.e. have it be just like C99's PRIx64 and alike, commonly
implemented by a (library internal) abstraction just holding the
necessary size prefix.

Jan

Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used

[Ian Campbell]

On Tue, 2015-01-20 at 11:19 +0000, Jan Beulich wrote:
> >>> On 20.01.15 at 12:11, <Ian.Campbell@citrix.com> wrote:
> > On Tue, 2015-01-20 at 11:05 +0000, Jan Beulich wrote:
> >> >>> On 20.01.15 at 11:52, <ian.campbell@citrix.com> wrote:
> >> > +static register_t do_deprecated_hypercall(void)
> >> > +{
> >> > +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> >> > +    const register_t op =
> >> > +#ifdef CONFIG_ARM_64
> >> > +        !is_32bit_domain(current->domain) ?
> >> > +            regs->x16
> >> > +        :
> >> > +#endif
> >> > +            regs->r12;
> >> > +
> >> > +    gdprintk(XENLOG_DEBUG, "%pv: deprecated hypercall %ld\n",
> >> > +             current, (unsigned long)op);
> >> 
> >> If this was x86 code, I'd complain about the cast...
> > 
> > The correct format code is PRIregister (since register_t can be 32- or
> > 64-bit for arm32 vs arm64 respectively), but it is in hex and xen.h
> > lists __HYPERVISOR_* in decimal so that's what I wanted to print for
> > each of manually looking.
> 
> Which makes sense. I'd rather question whether PRIregister is
> badly defined then, in that it not just specifies an eventual size
> prefix. I.e. have it be just like C99's PRIx64 and alike, commonly
> implemented by a (library internal) abstraction just holding the
> necessary size prefix.

Yes, it probably is, it's just that 99% of the time hex is what we want
so we haven't hit the activation energy to go and fix them.

Your suggestion is PRIxREGISTER and PRIdREGISTER etc, right?

There are several other non-ARM specific ones, PRIpaddr, PRI_xen_pfn and
PRI_xen_ulong etc (which may well have been added by me too during the
ARM port, so I won't claim them as precedent). Should we be changing
them all? I suppose paddr and pfn are less likely to be desired in
decimal.

Ian

Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used

[Julien Grall]

Hi Ian,

On 20/01/15 10:52, Ian Campbell wrote:
> A few folks have been caught out by OSes which call e.g.
> HYPERVISOR_event_channel_op_compat which has been deprecated since
> 3.2.2 (i.e. long before Xen on ARM). Existing x86 code can still
> safely and quietly using those calls, waiting for an unsuspecting ARM
> porter to turn up and trip over it. This turns out to be rather
> perplexing when it happens, since it can be obscured e.g. by various
> conditionals like __XEN_INTERFACE_VERSION__ what is actually being
> called.
> 
> Note that I'm making a distinction here between hypercalls which are
> simply not used/implemented on arm (yet) and those which were
> deprecated and replaced by a newer variant prior to Xen on ARM even
> being invented.  The latter will never be implemented on ARM and have
> non-deprecated aliases leading to confusion so those are the ones for
> which a warning is useful.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> ---
> RFC since I'm not sure how extreme our reaction ought to be here, e.g.
> I considered domain_crash() or even panic() when in a debug build. A
> XENLOG_DEBUG message is about the most benign of the options.

panic() should only be used if we can't recover from a Xen issue. I
would be very annoyed to see my host crashing while I'm adding support
of Xen in the guest.

It may be very long to reboot the host and/or maybe be shared with other
people.

I think the code would benefit to a domain_crash() in general. It will
avoid to spend time looking why it's not working.

> 
> Jan/Keir, although this is ARM specific I'd welcome your views as
> x86/REST maintainers.
> 
> Ard, I've not actually run this -- any chance you could re-b0rk your
> Tianocore image and give it a go?
> ---
>  xen/arch/arm/traps.c |   30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index ad046e8..89cbde6 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1148,6 +1148,22 @@ die:
>  }
>  #endif
>  
> +static register_t do_deprecated_hypercall(void)
> +{
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    const register_t op =
> +#ifdef CONFIG_ARM_64
> +        !is_32bit_domain(current->domain) ?
> +            regs->x16
> +        :
> +#endif
> +            regs->r12;
> +
> +    gdprintk(XENLOG_DEBUG, "%pv: deprecated hypercall %ld\n",

op is casted to an unsigned long, so I would use %lu.

Regards,

-- 
Julien Grall

Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used

[Jan Beulich]

>>> On 20.01.15 at 13:05, <Ian.Campbell@citrix.com> wrote:
> On Tue, 2015-01-20 at 11:19 +0000, Jan Beulich wrote:
>> >>> On 20.01.15 at 12:11, <Ian.Campbell@citrix.com> wrote:
>> > On Tue, 2015-01-20 at 11:05 +0000, Jan Beulich wrote:
>> >> >>> On 20.01.15 at 11:52, <ian.campbell@citrix.com> wrote:
>> >> > +static register_t do_deprecated_hypercall(void)
>> >> > +{
>> >> > +    struct cpu_user_regs *regs = guest_cpu_user_regs();
>> >> > +    const register_t op =
>> >> > +#ifdef CONFIG_ARM_64
>> >> > +        !is_32bit_domain(current->domain) ?
>> >> > +            regs->x16
>> >> > +        :
>> >> > +#endif
>> >> > +            regs->r12;
>> >> > +
>> >> > +    gdprintk(XENLOG_DEBUG, "%pv: deprecated hypercall %ld\n",
>> >> > +             current, (unsigned long)op);
>> >> 
>> >> If this was x86 code, I'd complain about the cast...
>> > 
>> > The correct format code is PRIregister (since register_t can be 32- or
>> > 64-bit for arm32 vs arm64 respectively), but it is in hex and xen.h
>> > lists __HYPERVISOR_* in decimal so that's what I wanted to print for
>> > each of manually looking.
>> 
>> Which makes sense. I'd rather question whether PRIregister is
>> badly defined then, in that it not just specifies an eventual size
>> prefix. I.e. have it be just like C99's PRIx64 and alike, commonly
>> implemented by a (library internal) abstraction just holding the
>> necessary size prefix.
> 
> Yes, it probably is, it's just that 99% of the time hex is what we want
> so we haven't hit the activation energy to go and fix them.
> 
> Your suggestion is PRIxREGISTER and PRIdREGISTER etc, right?

PRIxREG etc perhaps.

> There are several other non-ARM specific ones, PRIpaddr, PRI_xen_pfn and
> PRI_xen_ulong etc (which may well have been added by me too during the
> ARM port, so I won't claim them as precedent). Should we be changing
> them all? I suppose paddr and pfn are less likely to be desired in
> decimal.

Right - out of these only the ulong one may want to get relaxed.

Jan

Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used

[Ard Biesheuvel]

On 20 January 2015 at 10:52, Ian Campbell <ian.campbell@citrix.com> wrote:
> A few folks have been caught out by OSes which call e.g.
> HYPERVISOR_event_channel_op_compat which has been deprecated since
> 3.2.2 (i.e. long before Xen on ARM). Existing x86 code can still
> safely and quietly using those calls, waiting for an unsuspecting ARM
> porter to turn up and trip over it. This turns out to be rather
> perplexing when it happens, since it can be obscured e.g. by various
> conditionals like __XEN_INTERFACE_VERSION__ what is actually being
> called.
>
> Note that I'm making a distinction here between hypercalls which are
> simply not used/implemented on arm (yet) and those which were
> deprecated and replaced by a newer variant prior to Xen on ARM even
> being invented.  The latter will never be implemented on ARM and have
> non-deprecated aliases leading to confusion so those are the ones for
> which a warning is useful.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> ---
> RFC since I'm not sure how extreme our reaction ought to be here, e.g.
> I considered domain_crash() or even panic() when in a debug build. A
> XENLOG_DEBUG message is about the most benign of the options.
>
> Jan/Keir, although this is ARM specific I'd welcome your views as
> x86/REST maintainers.
>
> Ard, I've not actually run this -- any chance you could re-b0rk your
> Tianocore image and give it a go?

Hello,

I have been trying not to code against a moving Xen target, so I
haven't had a go with this yet. Once I go back to testing the GICv3
support, I will need to rebuild Xen and kernels etc anyway so I will
give it a try (by the end of the week)

-- 
Ard.


> ---
>  xen/arch/arm/traps.c |   30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index ad046e8..89cbde6 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1148,6 +1148,22 @@ die:
>  }
>  #endif
>
> +static register_t do_deprecated_hypercall(void)
> +{
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    const register_t op =
> +#ifdef CONFIG_ARM_64
> +        !is_32bit_domain(current->domain) ?
> +            regs->x16
> +        :
> +#endif
> +            regs->r12;
> +
> +    gdprintk(XENLOG_DEBUG, "%pv: deprecated hypercall %ld\n",
> +             current, (unsigned long)op);
> +    return -ENOSYS;
> +}
> +
>  typedef register_t (*arm_hypercall_fn_t)(
>      register_t, register_t, register_t, register_t, register_t);
>
> @@ -1167,15 +1183,29 @@ typedef struct {
>          .fn = (arm_hypercall_fn_t) &do_arm_ ## _name,                \
>          .nr_args = _nr_args,                                         \
>      }
> +/*
> + * Only use this for hypercalls which were deprecated (i.e. replaced
> + * by something else) before Xen on ARM was created, i.e. *not* for
> + * hypercalls which are simply not yet used on ARM.
> + */
> +#define HYPERCALL_DEPRECATED(_name, _nr_args)                   \
> +    [ __HYPERVISOR_##_name ] = {                                \
> +        .fn = (arm_hypercall_fn_t) &do_deprecated_hypercall,    \
> +        .nr_args = _nr_args,                                    \
> +    }
> +
>  static arm_hypercall_t arm_hypercall_table[] = {
>      HYPERCALL(memory_op, 2),
>      HYPERCALL(domctl, 1),
>      HYPERCALL(sched_op, 2),
> +    HYPERCALL_DEPRECATED(sched_op_compat, 2),
>      HYPERCALL(console_io, 3),
>      HYPERCALL(xen_version, 2),
>      HYPERCALL(xsm_op, 1),
>      HYPERCALL(event_channel_op, 2),
> +    HYPERCALL_DEPRECATED(event_channel_op_compat, 1),
>      HYPERCALL(physdev_op, 2),
> +    HYPERCALL_DEPRECATED(physdev_op_compat, 1),
>      HYPERCALL(sysctl, 2),
>      HYPERCALL(hvm_op, 2),
>      HYPERCALL(grant_table_op, 3),
> --
> 1.7.10.4
>

Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used

[Ian Campbell]

On Tue, 2015-01-27 at 11:51 +0000, Ard Biesheuvel wrote:
> On 20 January 2015 at 10:52, Ian Campbell <ian.campbell@citrix.com> wrote:
> > A few folks have been caught out by OSes which call e.g.
> > HYPERVISOR_event_channel_op_compat which has been deprecated since
> > 3.2.2 (i.e. long before Xen on ARM). Existing x86 code can still
> > safely and quietly using those calls, waiting for an unsuspecting ARM
> > porter to turn up and trip over it. This turns out to be rather
> > perplexing when it happens, since it can be obscured e.g. by various
> > conditionals like __XEN_INTERFACE_VERSION__ what is actually being
> > called.
> >
> > Note that I'm making a distinction here between hypercalls which are
> > simply not used/implemented on arm (yet) and those which were
> > deprecated and replaced by a newer variant prior to Xen on ARM even
> > being invented.  The latter will never be implemented on ARM and have
> > non-deprecated aliases leading to confusion so those are the ones for
> > which a warning is useful.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Jan Beulich <JBeulich@suse.com>
> > Cc: Keir Fraser <keir@xen.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > RFC since I'm not sure how extreme our reaction ought to be here, e.g.
> > I considered domain_crash() or even panic() when in a debug build. A
> > XENLOG_DEBUG message is about the most benign of the options.
> >
> > Jan/Keir, although this is ARM specific I'd welcome your views as
> > x86/REST maintainers.
> >
> > Ard, I've not actually run this -- any chance you could re-b0rk your
> > Tianocore image and give it a go?
> 
> Hello,
> 
> I have been trying not to code against a moving Xen target, so I
> haven't had a go with this yet. Once I go back to testing the GICv3
> support, I will need to rebuild Xen and kernels etc anyway so I will
> give it a try (by the end of the week)

Thanks, no particular hurry, this patch isn't super urgent or anthing.

Ian.

Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used

[Ard Biesheuvel]

On 20 January 2015 at 10:52, Ian Campbell <ian.campbell@citrix.com> wrote:
> A few folks have been caught out by OSes which call e.g.
> HYPERVISOR_event_channel_op_compat which has been deprecated since
> 3.2.2 (i.e. long before Xen on ARM). Existing x86 code can still
> safely and quietly using those calls, waiting for an unsuspecting ARM
> porter to turn up and trip over it. This turns out to be rather
> perplexing when it happens, since it can be obscured e.g. by various
> conditionals like __XEN_INTERFACE_VERSION__ what is actually being
> called.
>
> Note that I'm making a distinction here between hypercalls which are
> simply not used/implemented on arm (yet) and those which were
> deprecated and replaced by a newer variant prior to Xen on ARM even
> being invented.  The latter will never be implemented on ARM and have
> non-deprecated aliases leading to confusion so those are the ones for
> which a warning is useful.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> ---
> RFC since I'm not sure how extreme our reaction ought to be here, e.g.
> I considered domain_crash() or even panic() when in a debug build. A
> XENLOG_DEBUG message is about the most benign of the options.
>
> Jan/Keir, although this is ARM specific I'd welcome your views as
> x86/REST maintainers.
>
> Ard, I've not actually run this -- any chance you could re-b0rk your
> Tianocore image and give it a go?

I get a bunch of lines in my log looking like

(XEN) traps.c:1162:d4v0 d4v0: deprecated hypercall 16

so

Tested-by: Ard Biesheuvel <ard@linaro.org>

-- 
Ard.

> ---
>  xen/arch/arm/traps.c |   30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index ad046e8..89cbde6 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1148,6 +1148,22 @@ die:
>  }
>  #endif
>
> +static register_t do_deprecated_hypercall(void)
> +{
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    const register_t op =
> +#ifdef CONFIG_ARM_64
> +        !is_32bit_domain(current->domain) ?
> +            regs->x16
> +        :
> +#endif
> +            regs->r12;
> +
> +    gdprintk(XENLOG_DEBUG, "%pv: deprecated hypercall %ld\n",
> +             current, (unsigned long)op);
> +    return -ENOSYS;
> +}
> +
>  typedef register_t (*arm_hypercall_fn_t)(
>      register_t, register_t, register_t, register_t, register_t);
>
> @@ -1167,15 +1183,29 @@ typedef struct {
>          .fn = (arm_hypercall_fn_t) &do_arm_ ## _name,                \
>          .nr_args = _nr_args,                                         \
>      }
> +/*
> + * Only use this for hypercalls which were deprecated (i.e. replaced
> + * by something else) before Xen on ARM was created, i.e. *not* for
> + * hypercalls which are simply not yet used on ARM.
> + */
> +#define HYPERCALL_DEPRECATED(_name, _nr_args)                   \
> +    [ __HYPERVISOR_##_name ] = {                                \
> +        .fn = (arm_hypercall_fn_t) &do_deprecated_hypercall,    \
> +        .nr_args = _nr_args,                                    \
> +    }
> +
>  static arm_hypercall_t arm_hypercall_table[] = {
>      HYPERCALL(memory_op, 2),
>      HYPERCALL(domctl, 1),
>      HYPERCALL(sched_op, 2),
> +    HYPERCALL_DEPRECATED(sched_op_compat, 2),
>      HYPERCALL(console_io, 3),
>      HYPERCALL(xen_version, 2),
>      HYPERCALL(xsm_op, 1),
>      HYPERCALL(event_channel_op, 2),
> +    HYPERCALL_DEPRECATED(event_channel_op_compat, 1),
>      HYPERCALL(physdev_op, 2),
> +    HYPERCALL_DEPRECATED(physdev_op_compat, 1),
>      HYPERCALL(sysctl, 2),
>      HYPERCALL(hvm_op, 2),
>      HYPERCALL(grant_table_op, 3),
> --
> 1.7.10.4
>

Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used

[Ian Campbell]

On Wed, 2015-02-04 at 09:41 +0000, Ard Biesheuvel wrote:
> On 20 January 2015 at 10:52, Ian Campbell <ian.campbell@citrix.com> wrote:
> > A few folks have been caught out by OSes which call e.g.
> > HYPERVISOR_event_channel_op_compat which has been deprecated since
> > 3.2.2 (i.e. long before Xen on ARM). Existing x86 code can still
> > safely and quietly using those calls, waiting for an unsuspecting ARM
> > porter to turn up and trip over it. This turns out to be rather
> > perplexing when it happens, since it can be obscured e.g. by various
> > conditionals like __XEN_INTERFACE_VERSION__ what is actually being
> > called.
> >
> > Note that I'm making a distinction here between hypercalls which are
> > simply not used/implemented on arm (yet) and those which were
> > deprecated and replaced by a newer variant prior to Xen on ARM even
> > being invented.  The latter will never be implemented on ARM and have
> > non-deprecated aliases leading to confusion so those are the ones for
> > which a warning is useful.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Jan Beulich <JBeulich@suse.com>
> > Cc: Keir Fraser <keir@xen.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > RFC since I'm not sure how extreme our reaction ought to be here, e.g.
> > I considered domain_crash() or even panic() when in a debug build. A
> > XENLOG_DEBUG message is about the most benign of the options.
> >
> > Jan/Keir, although this is ARM specific I'd welcome your views as
> > x86/REST maintainers.
> >
> > Ard, I've not actually run this -- any chance you could re-b0rk your
> > Tianocore image and give it a go?
> 
> I get a bunch of lines in my log looking like
> 
> (XEN) traps.c:1162:d4v0 d4v0: deprecated hypercall 16
> 
> so
> 
> Tested-by: Ard Biesheuvel <ard@linaro.org>

Thanks!

Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used

[Julien Grall]

Hi Ian,

Do you plan to resend this patch?

Regards,

On 20/01/2015 11:52, Ian Campbell wrote:
> A few folks have been caught out by OSes which call e.g.
> HYPERVISOR_event_channel_op_compat which has been deprecated since
> 3.2.2 (i.e. long before Xen on ARM). Existing x86 code can still
> safely and quietly using those calls, waiting for an unsuspecting ARM
> porter to turn up and trip over it. This turns out to be rather
> perplexing when it happens, since it can be obscured e.g. by various
> conditionals like __XEN_INTERFACE_VERSION__ what is actually being
> called.
>
> Note that I'm making a distinction here between hypercalls which are
> simply not used/implemented on arm (yet) and those which were
> deprecated and replaced by a newer variant prior to Xen on ARM even
> being invented.  The latter will never be implemented on ARM and have
> non-deprecated aliases leading to confusion so those are the ones for
> which a warning is useful.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> ---
> RFC since I'm not sure how extreme our reaction ought to be here, e.g.
> I considered domain_crash() or even panic() when in a debug build. A
> XENLOG_DEBUG message is about the most benign of the options.
>
> Jan/Keir, although this is ARM specific I'd welcome your views as
> x86/REST maintainers.
>
> Ard, I've not actually run this -- any chance you could re-b0rk your
> Tianocore image and give it a go?
> ---
>   xen/arch/arm/traps.c |   30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index ad046e8..89cbde6 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1148,6 +1148,22 @@ die:
>   }
>   #endif
>
> +static register_t do_deprecated_hypercall(void)
> +{
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    const register_t op =
> +#ifdef CONFIG_ARM_64
> +        !is_32bit_domain(current->domain) ?
> +            regs->x16
> +        :
> +#endif
> +            regs->r12;
> +
> +    gdprintk(XENLOG_DEBUG, "%pv: deprecated hypercall %ld\n",
> +             current, (unsigned long)op);
> +    return -ENOSYS;
> +}
> +
>   typedef register_t (*arm_hypercall_fn_t)(
>       register_t, register_t, register_t, register_t, register_t);
>
> @@ -1167,15 +1183,29 @@ typedef struct {
>           .fn = (arm_hypercall_fn_t) &do_arm_ ## _name,                \
>           .nr_args = _nr_args,                                         \
>       }
> +/*
> + * Only use this for hypercalls which were deprecated (i.e. replaced
> + * by something else) before Xen on ARM was created, i.e. *not* for
> + * hypercalls which are simply not yet used on ARM.
> + */
> +#define HYPERCALL_DEPRECATED(_name, _nr_args)                   \
> +    [ __HYPERVISOR_##_name ] = {                                \
> +        .fn = (arm_hypercall_fn_t) &do_deprecated_hypercall,    \
> +        .nr_args = _nr_args,                                    \
> +    }
> +
>   static arm_hypercall_t arm_hypercall_table[] = {
>       HYPERCALL(memory_op, 2),
>       HYPERCALL(domctl, 1),
>       HYPERCALL(sched_op, 2),
> +    HYPERCALL_DEPRECATED(sched_op_compat, 2),
>       HYPERCALL(console_io, 3),
>       HYPERCALL(xen_version, 2),
>       HYPERCALL(xsm_op, 1),
>       HYPERCALL(event_channel_op, 2),
> +    HYPERCALL_DEPRECATED(event_channel_op_compat, 1),
>       HYPERCALL(physdev_op, 2),
> +    HYPERCALL_DEPRECATED(physdev_op_compat, 1),
>       HYPERCALL(sysctl, 2),
>       HYPERCALL(hvm_op, 2),
>       HYPERCALL(grant_table_op, 3),
>

-- 
Julien Grall