Re: [PATCH 16/18] arm/altp2m: Extended libxl to activate altp2m on ARM.

[Sergej Proskurin <proskurin@sec.in.tum.de>]

Hi Wei,


On 07/25/2016 10:32 AM, Wei Liu wrote:
> On Sun, Jul 24, 2016 at 06:06:00PM +0200, Sergej Proskurin wrote:
>> Hi Wei,
>>
>> On 07/07/2016 06:27 PM, Wei Liu wrote:
>>> On Mon, Jul 04, 2016 at 01:45:45PM +0200, Sergej Proskurin wrote:
>>>> The current implementation allows to set the parameter HVM_PARAM_ALTP2M.
>>>> This parameter allows further usage of altp2m on ARM. For this, we
>>>> define an additional altp2m field for PV domains as part of the
>>>> libxl_domain_type struct. This field can be set only on ARM systems
>>>> through the "altp2m" switch in the domain's configuration file (i.e.
>>>> set altp2m=1).
>>>>
>>>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>>>> ---
>>>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>>>> Cc: Wei Liu <wei.liu2@citrix.com>
>>>> ---
>>>>  tools/libxl/libxl_create.c  |  1 +
>>>>  tools/libxl/libxl_dom.c     | 14 ++++++++++++++
>>>>  tools/libxl/libxl_types.idl |  1 +
>>>>  tools/libxl/xl_cmdimpl.c    |  5 +++++
>>>>  4 files changed, 21 insertions(+)
>>>>
>>>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>>>> index 1b99472..40b5f61 100644
>>>> --- a/tools/libxl/libxl_create.c
>>>> +++ b/tools/libxl/libxl_create.c
>>>> @@ -400,6 +400,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>>>>              b_info->cmdline = b_info->u.pv.cmdline;
>>>>              b_info->u.pv.cmdline = NULL;
>>>>          }
>>>> +        libxl_defbool_setdefault(&b_info->u.pv.altp2m, false);
>>>>          break;
>>>>      default:
>>>>          LOG(ERROR, "invalid domain type %s in create info",
>>>> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
>>>> index ec29060..ab023a2 100644
>>>> --- a/tools/libxl/libxl_dom.c
>>>> +++ b/tools/libxl/libxl_dom.c
>>>> @@ -277,6 +277,16 @@ err:
>>>>  }
>>>>  #endif
>>>>  
>>>> +#if defined(__arm__) || defined(__aarch64__)
>>>> +static void pv_set_conf_params(xc_interface *handle, uint32_t domid,
>>>> +                               libxl_domain_build_info *const info)
>>>> +{
>>>> +    if ( libxl_defbool_val(info->u.pv.altp2m) )
>>> Coding style.
>>>
>> I am not sure which part does not fulfill Xen's coding style. Could you
>> be more specific please? Thank you.
>>
> There is a CODING_STYLE file in libxl as well. The coding style in
> tools/ is generally different from the coding style  in hypervisor with
> the exception of libxc.
>
> Sorry for being too terse on this. Do ask questions! :-)

It's all good, no worries :) Thank you for the hint (it must be the
indentation inside the if-clause brackets). I was still in the Xen
coding style, when I wrote this part: Now, I know better, thank you :)

>>>> +        xc_hvm_param_set(handle, domid, HVM_PARAM_ALTP2M,
>>>> +                         libxl_defbool_val(info->u.pv.altp2m));
>>>> +}
>>>> +#endif
>>>> +
>>>>  static void hvm_set_conf_params(xc_interface *handle, uint32_t domid,
>>>>                                  libxl_domain_build_info *const info)
>>>>  {
>>>> @@ -433,6 +443,10 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>>>>              return rc;
>>>>  #endif
>>>>      }
>>>> +#if defined(__arm__) || defined(__aarch64__)
>>>> +    else /* info->type == LIBXL_DOMAIN_TYPE_PV */
>>>> +        pv_set_conf_params(ctx->xch, domid, info);
>>>> +#endif
>>>>  
>>>>      rc = libxl__arch_domain_create(gc, d_config, domid);
>>>>  
>>>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>>>> index ef614be..0a164f9 100644
>>>> --- a/tools/libxl/libxl_types.idl
>>>> +++ b/tools/libxl/libxl_types.idl
>>>> @@ -554,6 +554,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>>>                                        ("features", string, {'const': True}),
>>>>                                        # Use host's E820 for PCI passthrough.
>>>>                                        ("e820_host", libxl_defbool),
>>>> +                                      ("altp2m", libxl_defbool),
>>> Why is this placed in PV instead of arch_arm?
>>>
>> The current implementation mirrors the x86's altp2m, where the altp2m
>> field represents a property of HVM guests in b_info->u.hvm.altp2m. Since
>> guests on ARM are marked as PV, I have placed the field altp2m into
>> b_info->u.pv.altp2m.
>>
>> However, if you believe that it would make more sense to place altp2m
>> into b_info->arch_arm.altp2m, I will try to adapt the affected fields.
>>
> OK. I don't think that one should be placed in u.pv because that union
> is for x86. However it is also not suitable to just promote that to
> a common field because x86 pv doesn't have altp2m support.
>
> I think arch_arm would be a better location.
>

Alright, I will move the field altp2m to arch_arm and adopt the
initialization routines.

>>> You would also need a LIBXL_HAVE macro in libxl.h for the new field.
>>>
>> There is already a LIBXL_HAVE_ALTP2M macro defined in libxl.h. Or did
>> you mean using an explicit one for ARM?
>>
> Yes, we need a new one for ARM.
>

Ok. I will provide a LIBXL_HAVE_ARM_ALTP2M macro.

Thank you.

Best regards,
~Sergej

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