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

[Wei Liu]

On Mon, Jul 25, 2016 at 01:26:29PM +0200, Sergej Proskurin wrote:
> 
> 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?
> 

Yes, we still need one. Something like

  LIBXL_HAVE_COMMON_ALTP2M

?

I'm bad at naming things though.

> 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?
> 

I think using altp2m is fine. But altp2mhvm needs to continue to work
because it is trivial to keep the old one.

In any case, document the new parameter, deprecate the old one and
document and the relationship between the two.

Wei.

> Best regards
> ~Sergej
> 

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

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

[Sergej Proskurin]

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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Wei Liu]

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.


Wei.

---8<---
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index ef614be..81d3ae5 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -494,6 +494,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
 # Note that the partial device tree should avoid to use the phandle
 # 65000 which is reserved by the toolstack.
 ("device_tree",  string),
+("altp2m",   libxl_defbool),
 ("u", KeyedUnion(None, libxl_domain_type, "type",
 [("hvm", Struct(None, [("firmware", string),
("bios", libxl_bios_type),
@@ -512,7 +513,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("mmio_hole_memkb",  MemKB),
("timer_mode",   libxl_timer_mode),
("nested_hvm",   libxl_defbool),
-   ("altp2m",   libxl_defbool),
+   ("altp2m",   libxl_defbool), # 
deprecated, please use common field
("smbios_firmware",  string),
("acpi_firmware",string),
("hdtype",   libxl_hdtype),

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

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

[Julien Grall]

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.


Regards,

--
Julien Grall

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

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

[Sergej Proskurin]

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 
 ---
 Cc: Ian Jackson 
 Cc: Wei Liu 
 ---
  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(_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 

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

[Wei Liu]

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 
> >> ---
> >> Cc: Ian Jackson 
> >> Cc: Wei Liu 
> >> ---
> >>  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(_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! :-)

> >> +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.

> > 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.

Wei.

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

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

[Sergej Proskurin]

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 
>> ---
>> Cc: Ian Jackson 
>> Cc: Wei Liu 
>> ---
>>  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(_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.

>> +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.

> 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?

>>])),
>>   ("invalid", None),
>>   ], keyvar_init_val = "LIBXL_DOMAIN_TYPE_INVALID")),
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 6459eec..12c6e48 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -1718,6 +1718,11 @@ static void parse_config_data(const char 
>> *config_source,
>>  exit(1);
>>  }
>>  
>> +#if defined(__arm__) || defined(__aarch64__)
>> +/* Enable altp2m for PV guests solely on ARM */
>> +xlu_cfg_get_defbool(config, "altp2m", _info->u.pv.altp2m, 0);
>> +#endif
>> +
>>  break;
>>  }
>>  default:
>> -- 
>> 2.8.3
>>

Thank you.

Best regards,
~Sergej

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

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

[Wei Liu]

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 
> ---
> Cc: Ian Jackson 
> Cc: Wei Liu 
> ---
>  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(_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.

> +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?

You would also need a LIBXL_HAVE macro in libxl.h for the new field.

>])),
>   ("invalid", None),
>   ], keyvar_init_val = "LIBXL_DOMAIN_TYPE_INVALID")),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 6459eec..12c6e48 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1718,6 +1718,11 @@ static void parse_config_data(const char 
> *config_source,
>  exit(1);
>  }
>  
> +#if defined(__arm__) || defined(__aarch64__)
> +/* Enable altp2m for PV guests solely on ARM */
> +xlu_cfg_get_defbool(config, "altp2m", _info->u.pv.altp2m, 0);
> +#endif
> +
>  break;
>  }
>  default:
> -- 
> 2.8.3
> 

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

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

[Sergej Proskurin]

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 
---
Cc: Ian Jackson 
Cc: Wei Liu 
---
 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(_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) )
+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),
   ])),
  ("invalid", None),
  ], keyvar_init_val = "LIBXL_DOMAIN_TYPE_INVALID")),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 6459eec..12c6e48 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1718,6 +1718,11 @@ static void parse_config_data(const char *config_source,
 exit(1);
 }
 
+#if defined(__arm__) || defined(__aarch64__)
+/* Enable altp2m for PV guests solely on ARM */
+xlu_cfg_get_defbool(config, "altp2m", _info->u.pv.altp2m, 0);
+#endif
+
 break;
 }
 default:
-- 
2.8.3


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Sergej Proskurin]

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 
---
Cc: Ian Jackson 
Cc: Wei Liu 
---
 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(_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) )
+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),
   ])),
  ("invalid", None),
  ], keyvar_init_val = "LIBXL_DOMAIN_TYPE_INVALID")),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 6459eec..12c6e48 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1718,6 +1718,11 @@ static void parse_config_data(const char *config_source,
 exit(1);
 }
 
+#if defined(__arm__) || defined(__aarch64__)
+/* Enable altp2m for PV guests solely on ARM */
+xlu_cfg_get_defbool(config, "altp2m", _info->u.pv.altp2m, 0);
+#endif
+
 break;
 }
 default:
-- 
2.8.3


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel