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 Konrad,

On 4 March 2017 at 01:32, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> wrote:
> On Tue, Feb 21, 2017 at 04:55:59PM +0530, Bhupinder Thakur wrote:
>> Three new HVM param handlers added for:
>>     - allocating a new VIRQ and return to the toolstack
>>     - 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
>>
>> 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
>
> s/if/If/
>> +             * own address space
>
> And you can also add an . here.
>
>> +             */
>> +            if ( a.index == HVM_PARAM_VPL011_CONSOLE_PFN )
>> +            {
>> +                vpl011_map_guest_page(d);
>> +            }
>> +#else
>> +            /*
>> +             * 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
>>          }
>>          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;
>
> This and the above look like it could be nicely folded in a function?
>
> Say:
>
> static bool vpl011_built(unsigned int idx)
> {
> #ifdef CONFIG_VPL011_CONSOLE
>         if ( idx == ..
>                 return true;
> #else
>         return false;
> }
>
> And here you can just do:
>         if (vpl011_built())
>         {
>                 rc = -1;
>         .. and so on.
>
>> +            }
>> +#endif
Using vpl011_built() removes the #ifdef from the code. But I still
need to check that the param is a vpl011 param before calling
vpl011_built() to decide whether to handle it or
flag an error.
So I have modified the code like this:

static bool vpl011_built()
{
#ifdef CONFIG_VPL011_CONSOLE
         return true;
#else
         return false;
#endif
}

if ( a.index == vpl011_param1 || a.index == vpl011_param2 || ... )
{
      if ( vpl011_built() )
      {
            /* handle vpl011 params */
           ...
      }
      else
      {
             rc = -1;
             ...
      }
}
... continue as usual for other parameters

>>              a.value = d->arch.hvm_domain.params[a.index];
>>              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
>> +#endif
>> +
>> +/* 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
>>
>> +
Removed extra line.
>
>
> ???
>>  #define HVM_NR_PARAMS 37
>>
>>  #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
>> --
>> 2.7.4
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@xxxxxxxxxxxxx
>> https://lists.xen.org/xen-devel

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