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

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

Hi Julien,


>> Three new HVM param handlers added for:
>>     - allocating a new VIRQ and return to the toolstack
>
>
> This is not necessary. We could hardcode it.
I will modify the code to use a fixed SPI for vpl011.

>
>>     - 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.
I will 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.
How are these checks done on ARM currently?

>
>> +            }
>> +#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 the toolstack tries to set/get the vpl011 parameters (because user
has enabled pl011 emulation in libxl) while vpl011 support is compiled
out in Xen,
I wanted the toolstack to handle the failure case correctly.

>
>> +            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.
Added the deprecated comment for x86.
>
>> +#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