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

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

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

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