* Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used
2015-01-20 10:52 [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used Ian Campbell
@ 2015-01-20 11:05 ` Jan Beulich
2015-01-20 11:11 ` Ian Campbell
2015-01-20 12:35 ` Julien Grall
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2015-01-20 11:05 UTC (permalink / raw)
To: Ian Campbell
Cc: Keir Fraser, Ard Biesheuvel, julien.grall, tim, xen-devel,
Anthony PERARD, stefano.stabellini
>>> 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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used
2015-01-20 11:05 ` Jan Beulich
@ 2015-01-20 11:11 ` Ian Campbell
2015-01-20 11:19 ` Jan Beulich
0 siblings, 1 reply; 12+ messages in thread
From: Ian Campbell @ 2015-01-20 11:11 UTC (permalink / raw)
To: Jan Beulich
Cc: Keir Fraser, Ard Biesheuvel, julien.grall, tim, xen-devel,
Anthony PERARD, stefano.stabellini
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.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used
2015-01-20 11:11 ` Ian Campbell
@ 2015-01-20 11:19 ` Jan Beulich
2015-01-20 12:05 ` Ian Campbell
0 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2015-01-20 11:19 UTC (permalink / raw)
To: Ian Campbell
Cc: Keir Fraser, Ard Biesheuvel, julien.grall, tim, xen-devel,
Anthony PERARD, stefano.stabellini
>>> 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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used
2015-01-20 11:19 ` Jan Beulich
@ 2015-01-20 12:05 ` Ian Campbell
2015-01-20 13:03 ` Jan Beulich
0 siblings, 1 reply; 12+ messages in thread
From: Ian Campbell @ 2015-01-20 12:05 UTC (permalink / raw)
To: Jan Beulich
Cc: Keir Fraser, Ard Biesheuvel, julien.grall, tim, xen-devel,
Anthony PERARD, stefano.stabellini
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used
2015-01-20 12:05 ` Ian Campbell
@ 2015-01-20 13:03 ` Jan Beulich
0 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2015-01-20 13:03 UTC (permalink / raw)
To: Ian Campbell
Cc: Keir Fraser, Ard Biesheuvel, julien.grall, tim, xen-devel,
Anthony PERARD, stefano.stabellini
>>> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used
2015-01-20 10:52 [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used Ian Campbell
2015-01-20 11:05 ` Jan Beulich
@ 2015-01-20 12:35 ` Julien Grall
2015-01-27 11:51 ` Ard Biesheuvel
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Julien Grall @ 2015-01-20 12:35 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: Keir Fraser, Ard Biesheuvel, stefano.stabellini, tim, Jan Beulich,
Anthony PERARD
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used
2015-01-20 10:52 [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used Ian Campbell
2015-01-20 11:05 ` Jan Beulich
2015-01-20 12:35 ` Julien Grall
@ 2015-01-27 11:51 ` Ard Biesheuvel
2015-01-27 17:14 ` Ian Campbell
2015-02-04 9:41 ` Ard Biesheuvel
2015-06-26 10:19 ` Julien Grall
4 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2015-01-27 11:51 UTC (permalink / raw)
To: Ian Campbell
Cc: Keir Fraser, Stefano Stabellini, Julien Grall, tim, xen-devel,
Jan Beulich, Anthony PERARD
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used
2015-01-27 11:51 ` Ard Biesheuvel
@ 2015-01-27 17:14 ` Ian Campbell
0 siblings, 0 replies; 12+ messages in thread
From: Ian Campbell @ 2015-01-27 17:14 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Keir Fraser, Stefano Stabellini, Julien Grall, tim, xen-devel,
Jan Beulich, Anthony PERARD
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used
2015-01-20 10:52 [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used Ian Campbell
` (2 preceding siblings ...)
2015-01-27 11:51 ` Ard Biesheuvel
@ 2015-02-04 9:41 ` Ard Biesheuvel
2015-02-04 10:16 ` Ian Campbell
2015-06-26 10:19 ` Julien Grall
4 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2015-02-04 9:41 UTC (permalink / raw)
To: Ian Campbell
Cc: Keir Fraser, Stefano Stabellini, Julien Grall, tim, xen-devel,
Jan Beulich, Anthony PERARD
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used
2015-02-04 9:41 ` Ard Biesheuvel
@ 2015-02-04 10:16 ` Ian Campbell
0 siblings, 0 replies; 12+ messages in thread
From: Ian Campbell @ 2015-02-04 10:16 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Keir Fraser, Stefano Stabellini, Julien Grall, tim, xen-devel,
Jan Beulich, Anthony PERARD
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!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used
2015-01-20 10:52 [PATCH RFC] xen: arm: Log a warning message when a deprecated hypercall is used Ian Campbell
` (3 preceding siblings ...)
2015-02-04 9:41 ` Ard Biesheuvel
@ 2015-06-26 10:19 ` Julien Grall
4 siblings, 0 replies; 12+ messages in thread
From: Julien Grall @ 2015-06-26 10:19 UTC (permalink / raw)
To: Ian Campbell, xen-devel
Cc: Keir Fraser, Ard Biesheuvel, julien.grall, tim, Jan Beulich,
Anthony PERARD, stefano.stabellini
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
^ permalink raw reply [flat|nested] 12+ messages in thread