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

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

On 07/25/2016 12:08 PM, Wei Liu wrote:
> On Mon, Jul 25, 2016 at 10:49:41AM +0100, Julien Grall wrote:
>> Hello,
>>
>> On 25/07/16 10:04, Sergej Proskurin wrote:
>>> On 07/25/2016 10:32 AM, Wei Liu wrote:
>>>> On Sun, Jul 24, 2016 at 06:06:00PM +0200, Sergej Proskurin wrote:
>>>>>>> +        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.
>> Would not it make more sense to introduce a generic field 'altp2m' (i.e
>> outside of hvm and arch_arm)?
>>
>> Otherwise, toolstack such as libvirt will need to have specific code to
>> handle ARM and x86.
>>
> Hmm... this is a compelling reason.
>
> After mulling over this issue a bit more, I think I'm fine with
> promoting altp2m to a common field.
>
> Sergej, this is a patch to get you started. Please make sure setting the
> old field still works. If I'm not clear enough, please ask.
>

Alright, thank you. One question up front: Do we still need two
different LIBXL_HAVE macros then?

Also, concerning the dom configuration parameters: Do you think we
should use two different dom configuration parameters (the legacy
"altp2mhvm" and the new "altp2m") or shall we merge both into "altp2m"
as we will address only one common altp2m field in the struct?

Best regards
~Sergej


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