Re: [PATCH 07/11] xen/arm: vpl011: Add two new vpl011 parameters to xenstore

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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

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