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

[Julien Grall <julien.grall@linaro.org>]

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