Re: [Xen-devel] [PATCH 02/11] xen/arm: vpl011: Add new hvm params in Xen for ring buffer/event setup

[Julien Grall <julien.grall@xxxxxxx>]

(CC "The REST" maintainers)

Hi Bhupinder,

On 02/21/2017 11:25 AM, Bhupinder Thakur wrote:
> Three new HVM param handlers added for:
>     - allocating a new VIRQ and return to the toolstack
This is not necessary. We could hardcode it.

>     - allocating a new event channel for sending/receiving events from Xen and 
> return
>       to the toolstack
>     - mapping the PFN allocted by the toolstack to be used as IN/OUT ring 
> buffers
s/allocted/allocated/

> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
> ---
>  xen/arch/arm/hvm.c              | 39 +++++++++++++++++++++++++++++++++++++++
>  xen/include/public/hvm/params.h | 10 +++++++++-
>  2 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> index d999bde..f3b9eb1 100644
> --- a/xen/arch/arm/hvm.c
> +++ b/xen/arch/arm/hvm.c
> @@ -23,6 +23,8 @@
>  #include <xen/guest_access.h>
>  #include <xen/sched.h>
>  #include <xen/monitor.h>
> +#include <xen/event.h>
> +#include <xen/vmap.h>
>
>  #include <xsm/xsm.h>
>
> @@ -31,6 +33,7 @@
>  #include <public/hvm/hvm_op.h>
>
>  #include <asm/hypercall.h>
> +#include "vpl011.h"
>
>  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
> @@ -61,9 +64,45 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( op == HVMOP_set_param )
>          {
>              d->arch.hvm_domain.params[a.index] = a.value;
> +
> +#ifdef CONFIG_VPL011_CONSOLE
> +            /*
> +             * if it is a vpl011 console pfn then map it to its
> +             * own address space
> +             */
> +            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN )
> +            {
> +                vpl011_map_guest_page(d);
vpl011_map_guest_page(...) is will return an error if it fails to map 
the page. Shouldn't you propagate the value.
Also, this code does not prevent the new HVM param to be set twice or 
even set by the guest. We probably want to introduce an helper to check 
if the domain is allowed to set it. See hvm_allow_set_param on x86.
> +            }
> +#else
> +            /*
> +             * if VPL011 is not compiled in then disallow setting of any
> +             * related HVM params
> +             */
We really don't care if the toolstack set unused HVM param as they 
should not be used.
> +            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN ||
> +                 a.index == HVM_PARAM_VPL011_CONSOLE_EVTCHN ||
> +                 a.index == HVM_PARAM_VPL011_VIRQ )
> +            {
> +                rc = -1;
> +                goto param_fail;
> +            }
> +#endif
>          }
>          else
>          {
> +#ifndef CONFIG_VPL011_CONSOLE
> +            /*
> +             * if VPL011 is not compiled in then disallow setting of any
> +             * related HVM params
> +             */
> +            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN ||
> +                 a.index == HVM_PARAM_VPL011_CONSOLE_EVTCHN ||
> +                 a.index == HVM_PARAM_VPL011_VIRQ )
> +            {
> +                rc = -1;
> +                goto param_fail;
> +            }
> +#endif
Ditto.

>              a.value = d->arch.hvm_domain.params[a.index];
We probably don't want the guest to be able to read the console pfn 
page. See hvm_allow_get_param.
>              rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>          }
> diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
> index 3f54a49..13bf719 100644
> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -203,10 +203,17 @@
>   */
>  #define HVM_PARAM_ACPI_IOPORTS_LOCATION 19
>
> -/* Deprecated */
> +#if defined(__arm__) || defined(__aarch64__)
> +#define HVM_PARAM_VPL011_CONSOLE_PFN    20
> +#define HVM_PARAM_VPL011_CONSOLE_EVTCHN 21
> +#define HVM_PARAM_VPL011_VIRQ           22
> +#else
>  #define HVM_PARAM_MEMORY_EVENT_CR0          20
>  #define HVM_PARAM_MEMORY_EVENT_CR3          21
>  #define HVM_PARAM_MEMORY_EVENT_CR4          22
Those parameters are still deprecated but you drop the comment stating that.

> +#endif
> +
Those params are x86 specific so should have never been set on ARM. But 
I am not sure if it is fine to re-purpose deprecated number.
I have CCed "The REST" maintainers to have their input here.

> +/* Deprecated */
>  #define HVM_PARAM_MEMORY_EVENT_INT3         23
>  #define HVM_PARAM_MEMORY_EVENT_SINGLE_STEP  25
>  #define HVM_PARAM_MEMORY_EVENT_MSR          30
> @@ -253,6 +260,7 @@
>   */
>  #define HVM_PARAM_X87_FIP_WIDTH 36
>
> +
Spurious change.

>  #define HVM_NR_PARAMS 37
>
>  #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
Cheers,

--
Julien Grall

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