[Xen-devel,07/11] xen/arm: vpl011: Add two new vpl011 parameters to xenstore

Message ID 1487676368-22356-8-git-send-email-bhupinder.thakur@linaro.org
State New
Headers show
Series
  • pl011 emulation support in Xen
Related show

Commit Message

Bhupinder Thakur Feb. 21, 2017, 11:26 a.m.
Add two new parameters to the xen store:
    - newly allocated PFN to be used as IN/OUT ring buffer by xenconsoled
    - a new event channel read from Xen using a hvm call to be used by xenconsoled
      for eventing

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
 tools/libxl/libxl.c          |  6 ++++++
 tools/libxl/libxl_dom.c      | 18 +++++++++++++++++-
 tools/libxl/libxl_internal.h |  4 ++++
 3 files changed, 27 insertions(+), 1 deletion(-)

Comments

Konrad Rzeszutek Wilk March 3, 2017, 8:58 p.m. | #1
On Tue, Feb 21, 2017 at 04:56:04PM +0530, Bhupinder Thakur wrote:
> Add two new parameters to the xen store:
>     - newly allocated PFN to be used as IN/OUT ring buffer by xenconsoled
>     - a new event channel read from Xen using a hvm call to be used by xenconsoled
>       for eventing

This should have a correspoinding change in the include/public/io/console.h
describing this new 'vpl011-port' and 'vpl011-ring-ref' Xenbus entry.

Feel free to lift from kbdif.h.

Unless an earlier patch already does this? In which case you should
mention it:

"Patch titled XYZ introduces these XenBus entries in console.h"

> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
>  tools/libxl/libxl.c          |  6 ++++++
>  tools/libxl/libxl_dom.c      | 18 +++++++++++++++++-
>  tools/libxl/libxl_internal.h |  4 ++++
>  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index d400fa2..cc00235 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -3159,6 +3159,12 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
>          flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->console_port));
>          flexarray_append(ro_front, "ring-ref");
>          flexarray_append(ro_front, GCSPRINTF("%lu", state->console_mfn));
> +#if defined(__arm__) || defined(__aarch64__)
> +        flexarray_append(ro_front, "vpl011-port");
> +        flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vpl011_console_port));
> +        flexarray_append(ro_front, "vpl011-ring-ref");
> +        flexarray_append(ro_front, GCSPRINTF("%lu", state->vpl011_console_mfn));
> +#endif

so... what if the VPL011 is not enabled in the guest? Should we still
create these XenBus entries?

>      } else {
>          flexarray_append(front, "state");
>          flexarray_append(front, GCSPRINTF("%d", XenbusStateInitialising));
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index d519c8d..39eaff6 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -302,7 +302,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      char *xs_domid, *con_domid;
>      int rc;
> -    uint64_t size;
> +    uint64_t size, val=-1;

So -1 for uint64_t is 0xffffffff... 

Is that what you meant? And why? Why not not uint32_t?


>  
>      if (xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus) != 0) {
>          LOG(ERROR, "Couldn't set max vcpu count");
> @@ -432,6 +432,16 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>      state->store_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->store_domid);
>      state->console_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->console_domid);
>  
> +#if defined(__arm__) || defined(__aarch64__)
> +    /* get the vpl011 event channel from Xen */

Please remove this comment.

> +    rc = xc_hvm_param_get(ctx->xch, domid, HVM_PARAM_VPL011_CONSOLE_EVTCHN,
> +            &val);
> +    if ( rc )
> +        state->vpl011_console_port = -1;
> +    else
> +        state->vpl011_console_port = (uint32_t)val;
> +#endif
> +
>      if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
>          hvm_set_conf_params(ctx->xch, domid, info);
>  #if defined(__i386__) || defined(__x86_64__)
> @@ -727,6 +737,9 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>  
>      dom->flags = flags;
>      dom->console_evtchn = state->console_port;
> +#if defined(__arm__) || defined(__aarch64__)
> +    dom->vpl011_console_evtchn = state->vpl011_console_port;
> +#endif
>      dom->console_domid = state->console_domid;
>      dom->xenstore_evtchn = state->store_port;
>      dom->xenstore_domid = state->store_domid;
> @@ -771,6 +784,9 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>      if (xc_dom_feature_translated(dom)) {
>          state->console_mfn = dom->console_pfn;
>          state->store_mfn = dom->xenstore_pfn;
> +#if defined(__arm__) || defined(__aarch64__)
> +        state->vpl011_console_mfn = dom->vpl011_console_pfn;
> +#endif
>      } else {
>          state->console_mfn = xc_dom_p2m(dom, dom->console_pfn);
>          state->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 5f46578..10e262e 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1128,6 +1128,10 @@ typedef struct {
>      uint32_t num_vmemranges;
>  
>      xc_domain_configuration_t config;
> +#if defined(__arm__) || defined(__aarch64__)
> +    unsigned long vpl011_console_mfn;
> +#endif

I am not a big fan of these #ifdef.

Could they go away and this could would be compiled on x86 too but
just never used? (The default value to use this owuld be disabled
for example)?

>  } libxl__domain_build_state;
>  
>  _hidden int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
Bhupinder Thakur March 28, 2017, 7:49 a.m. | #2
Hi Konrad,


>> Add two new parameters to the xen store:
>>     - newly allocated PFN to be used as IN/OUT ring buffer by xenconsoled
>>     - a new event channel read from Xen using a hvm call to be used by xenconsoled
>>       for eventing
>
> This should have a correspoinding change in the include/public/io/console.h
> describing this new 'vpl011-port' and 'vpl011-ring-ref' Xenbus entry.
>
> Feel free to lift from kbdif.h.
>
I tried looking at kbdif.h but could not find any xenbus entry
definitions. I looked at how "ring-ref" is defined which is used for
the PV console.
I tried to define "vpl011-ring-ref" in a similar way.

> Unless an earlier patch already does this? In which case you should
> mention it:
>

> "Patch titled XYZ introduces these XenBus entries in console.h"
>
>>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>> ---
>>  tools/libxl/libxl.c          |  6 ++++++
>>  tools/libxl/libxl_dom.c      | 18 +++++++++++++++++-
>>  tools/libxl/libxl_internal.h |  4 ++++
>>  3 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index d400fa2..cc00235 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -3159,6 +3159,12 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
>>          flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->console_port));
>>          flexarray_append(ro_front, "ring-ref");
>>          flexarray_append(ro_front, GCSPRINTF("%lu", state->console_mfn));
>> +#if defined(__arm__) || defined(__aarch64__)
>> +        flexarray_append(ro_front, "vpl011-port");
>> +        flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vpl011_console_port));
>> +        flexarray_append(ro_front, "vpl011-ring-ref");
>> +        flexarray_append(ro_front, GCSPRINTF("%lu", state->vpl011_console_mfn));
>> +#endif
>
> so... what if the VPL011 is not enabled in the guest? Should we still
> create these XenBus entries?

I will add a libxl option for enabling/disabling pl011 emulation. Only
when this option is enabled,
vpl011-port and vpl011-ring-ref would be added to xenstore.

>
>>      } else {
>>          flexarray_append(front, "state");
>>          flexarray_append(front, GCSPRINTF("%d", XenbusStateInitialising));
>> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
>> index d519c8d..39eaff6 100644
>> --- a/tools/libxl/libxl_dom.c
>> +++ b/tools/libxl/libxl_dom.c
>> @@ -302,7 +302,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>>      libxl_ctx *ctx = libxl__gc_owner(gc);
>>      char *xs_domid, *con_domid;
>>      int rc;
>> -    uint64_t size;
>> +    uint64_t size, val=-1;
>
> So -1 for uint64_t is 0xffffffff...
>
> Is that what you meant? And why? Why not not uint32_t?
I used -1 to indicate invalid event_channel since 0 might be a valid
event channel number.
Since xc_hvm_param_get() expects a uint64_t, I used a local variable
of type unit64_t.
>
>
>>
>>      if (xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus) != 0) {
>>          LOG(ERROR, "Couldn't set max vcpu count");
>> @@ -432,6 +432,16 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>>      state->store_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->store_domid);
>>      state->console_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->console_domid);
>>
>> +#if defined(__arm__) || defined(__aarch64__)
>> +    /* get the vpl011 event channel from Xen */
>
> Please remove this comment.
Ok.
>
>> +    rc = xc_hvm_param_get(ctx->xch, domid, HVM_PARAM_VPL011_CONSOLE_EVTCHN,
>> +            &val);
>> +    if ( rc )
>> +        state->vpl011_console_port = -1;
>> +    else
>> +        state->vpl011_console_port = (uint32_t)val;
>> +#endif
>> +
>>      if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
>>          hvm_set_conf_params(ctx->xch, domid, info);
>>  #if defined(__i386__) || defined(__x86_64__)
>> @@ -727,6 +737,9 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>>
>>      dom->flags = flags;
>>      dom->console_evtchn = state->console_port;
>> +#if defined(__arm__) || defined(__aarch64__)
>> +    dom->vpl011_console_evtchn = state->vpl011_console_port;
>> +#endif
>>      dom->console_domid = state->console_domid;
>>      dom->xenstore_evtchn = state->store_port;
>>      dom->xenstore_domid = state->store_domid;
>> @@ -771,6 +784,9 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>>      if (xc_dom_feature_translated(dom)) {
>>          state->console_mfn = dom->console_pfn;
>>          state->store_mfn = dom->xenstore_pfn;
>> +#if defined(__arm__) || defined(__aarch64__)
>> +        state->vpl011_console_mfn = dom->vpl011_console_pfn;
>> +#endif
>>      } else {
>>          state->console_mfn = xc_dom_p2m(dom, dom->console_pfn);
>>          state->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index 5f46578..10e262e 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -1128,6 +1128,10 @@ typedef struct {
>>      uint32_t num_vmemranges;
>>
>>      xc_domain_configuration_t config;
>> +#if defined(__arm__) || defined(__aarch64__)
>> +    unsigned long vpl011_console_mfn;
>> +#endif
>
> I am not a big fan of these #ifdef.
>
> Could they go away and this could would be compiled on x86 too but
> just never used? (The default value to use this owuld be disabled
> for example)?
>
>>  } libxl__domain_build_state;
>>
>>  _hidden int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>> --
>> 2.7.4
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> https://lists.xen.org/xen-devel

Patch hide | download patch | download mbox

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d400fa2..cc00235 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3159,6 +3159,12 @@  int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
         flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->console_port));
         flexarray_append(ro_front, "ring-ref");
         flexarray_append(ro_front, GCSPRINTF("%lu", state->console_mfn));
+#if defined(__arm__) || defined(__aarch64__)
+        flexarray_append(ro_front, "vpl011-port");
+        flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vpl011_console_port));
+        flexarray_append(ro_front, "vpl011-ring-ref");
+        flexarray_append(ro_front, GCSPRINTF("%lu", state->vpl011_console_mfn));
+#endif
     } else {
         flexarray_append(front, "state");
         flexarray_append(front, GCSPRINTF("%d", XenbusStateInitialising));
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index d519c8d..39eaff6 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -302,7 +302,7 @@  int libxl__build_pre(libxl__gc *gc, uint32_t domid,
     libxl_ctx *ctx = libxl__gc_owner(gc);
     char *xs_domid, *con_domid;
     int rc;
-    uint64_t size;
+    uint64_t size, val=-1;
 
     if (xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus) != 0) {
         LOG(ERROR, "Couldn't set max vcpu count");
@@ -432,6 +432,16 @@  int libxl__build_pre(libxl__gc *gc, uint32_t domid,
     state->store_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->store_domid);
     state->console_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->console_domid);
 
+#if defined(__arm__) || defined(__aarch64__)
+    /* get the vpl011 event channel from Xen */
+    rc = xc_hvm_param_get(ctx->xch, domid, HVM_PARAM_VPL011_CONSOLE_EVTCHN,
+            &val);
+    if ( rc )
+        state->vpl011_console_port = -1;
+    else
+        state->vpl011_console_port = (uint32_t)val;
+#endif
+
     if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
         hvm_set_conf_params(ctx->xch, domid, info);
 #if defined(__i386__) || defined(__x86_64__)
@@ -727,6 +737,9 @@  int libxl__build_pv(libxl__gc *gc, uint32_t domid,
 
     dom->flags = flags;
     dom->console_evtchn = state->console_port;
+#if defined(__arm__) || defined(__aarch64__)
+    dom->vpl011_console_evtchn = state->vpl011_console_port;
+#endif
     dom->console_domid = state->console_domid;
     dom->xenstore_evtchn = state->store_port;
     dom->xenstore_domid = state->store_domid;
@@ -771,6 +784,9 @@  int libxl__build_pv(libxl__gc *gc, uint32_t domid,
     if (xc_dom_feature_translated(dom)) {
         state->console_mfn = dom->console_pfn;
         state->store_mfn = dom->xenstore_pfn;
+#if defined(__arm__) || defined(__aarch64__)
+        state->vpl011_console_mfn = dom->vpl011_console_pfn;
+#endif
     } else {
         state->console_mfn = xc_dom_p2m(dom, dom->console_pfn);
         state->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 5f46578..10e262e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1128,6 +1128,10 @@  typedef struct {
     uint32_t num_vmemranges;
 
     xc_domain_configuration_t config;
+#if defined(__arm__) || defined(__aarch64__)
+    unsigned long vpl011_console_mfn;
+    uint32_t    vpl011_console_port;
+#endif
 } libxl__domain_build_state;
 
 _hidden int libxl__build_pre(libxl__gc *gc, uint32_t domid,