[Xen-devel,06/10,v2] xen/arm: vpl011: Add vuart ring-buf and evtchn to xenstore

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

Commit Message

Bhupinder Thakur April 28, 2017, 4:01 p.m.
Add two new parameters to the xen store to be used by xenconsoled:
    - newly allocated PFN to be used as IN/OUT ring buffer
    - get a new event channel allocated by Xen using a domctl call

These paramters are added to xenstore only if vuart console is enabled
by the user.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---

Changes since v1:
- Modified the xenstore key names to /vuart/0/ring-ref and 
  /vuart/0/port.
- Replaced the hvm call with domctl call to get the event channel.

 tools/libxl/libxl_console.c | 10 ++++++++++
 tools/libxl/libxl_dom.c     |  4 ++++
 2 files changed, 14 insertions(+)

Comments

Stefano Stabellini April 28, 2017, 9:57 p.m. | #1
On Fri, 28 Apr 2017, Bhupinder Thakur wrote:
> Add two new parameters to the xen store to be used by xenconsoled:
>     - newly allocated PFN to be used as IN/OUT ring buffer
>     - get a new event channel allocated by Xen using a domctl call
> 
> These paramters are added to xenstore only if vuart console is enabled
> by the user.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
> 
> Changes since v1:
> - Modified the xenstore key names to /vuart/0/ring-ref and 
>   /vuart/0/port.
> - Replaced the hvm call with domctl call to get the event channel.
> 
>  tools/libxl/libxl_console.c | 10 ++++++++++
>  tools/libxl/libxl_dom.c     |  4 ++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c
> index 446e766..ef3bd44 100644
> --- a/tools/libxl/libxl_console.c
> +++ b/tools/libxl/libxl_console.c
> @@ -67,6 +67,9 @@ int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num,
>      case LIBXL_CONSOLE_TYPE_SERIAL:
>          cons_type_s = "serial";
>          break;
> +    case LIBXL_CONSOLE_TYPE_VUART:
> +        cons_type_s = "vuart";
> +        break;
>      default:
>          goto out;
>      }
> @@ -326,6 +329,13 @@ 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 (state->vuart_enabled)
> +        {
> +            flexarray_append(ro_front, "vuart/0/port");
> +            flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vuart_port));
> +            flexarray_append(ro_front, "vuart/0/ring-ref");
> +            flexarray_append(ro_front, GCSPRINTF("%lu", state->vuart_mfn));
> +        }

It looks like you are reusing the libxl__device_console_add call for the
main PV console for the domain, to also add the vuart nodes to xenstore.

I don't think it is a good idea to mix the two. I suggest to introduce a
new libxl__device call to introduce the vuart nodes to xenstore, given
that they have no relantionship with the principal PV console of the
domain.


>      } 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 5d914a5..06ff3b7 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -434,6 +434,9 @@ 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 (state->vuart_enabled)
> +        xc_domain_vuart_get_evtchn(ctx->xch, domid, &state->vuart_port);
> +
>      if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
>          hvm_set_conf_params(ctx->xch, domid, info);
>  #if defined(__i386__) || defined(__x86_64__)
> @@ -788,6 +791,7 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>      if (xc_dom_translated(dom)) {
>          state->console_mfn = dom->console_pfn;
>          state->store_mfn = dom->xenstore_pfn;
> +        state->vuart_mfn = dom->vuart_pfn;
>      } else {
>          state->console_mfn = xc_dom_p2m(dom, dom->console_pfn);
>          state->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);

These two changes to libxl_dom.c probably belong to patch #4
Bhupinder Thakur May 1, 2017, 11:21 a.m. | #2
Hi Stefano,

>> diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c
>> index 446e766..ef3bd44 100644
>> --- a/tools/libxl/libxl_console.c
>> +++ b/tools/libxl/libxl_console.c
>> @@ -67,6 +67,9 @@ int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num,
>>      case LIBXL_CONSOLE_TYPE_SERIAL:
>>          cons_type_s = "serial";
>>          break;
>> +    case LIBXL_CONSOLE_TYPE_VUART:
>> +        cons_type_s = "vuart";
>> +        break;
>>      default:
>>          goto out;
>>      }
>> @@ -326,6 +329,13 @@ 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 (state->vuart_enabled)
>> +        {
>> +            flexarray_append(ro_front, "vuart/0/port");
>> +            flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vuart_port));
>> +            flexarray_append(ro_front, "vuart/0/ring-ref");
>> +            flexarray_append(ro_front, GCSPRINTF("%lu", state->vuart_mfn));
>> +        }
>
> It looks like you are reusing the libxl__device_console_add call for the
> main PV console for the domain, to also add the vuart nodes to xenstore.
>
> I don't think it is a good idea to mix the two. I suggest to introduce a
> new libxl__device call to introduce the vuart nodes to xenstore, given
> that they have no relantionship with the principal PV console of the
> domain.
>
I have a doubt here. Do I have to create a new console device
(libxl__device) to register the vuart console or can I use the
existing console device (which is used for registering the primary
console) to register the vuart nodes to xenstore?

I suspect that if I try to register with the same console device then
libxl__device_generic_add() may fail as it is already added.

>
>>      } 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 5d914a5..06ff3b7 100644
>> --- a/tools/libxl/libxl_dom.c
>> +++ b/tools/libxl/libxl_dom.c
>> @@ -434,6 +434,9 @@ 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 (state->vuart_enabled)
>> +        xc_domain_vuart_get_evtchn(ctx->xch, domid, &state->vuart_port);
>> +
>>      if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
>>          hvm_set_conf_params(ctx->xch, domid, info);
>>  #if defined(__i386__) || defined(__x86_64__)
>> @@ -788,6 +791,7 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>>      if (xc_dom_translated(dom)) {
>>          state->console_mfn = dom->console_pfn;
>>          state->store_mfn = dom->xenstore_pfn;
>> +        state->vuart_mfn = dom->vuart_pfn;
>>      } else {
>>          state->console_mfn = xc_dom_p2m(dom, dom->console_pfn);
>>          state->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
>
> These two changes to libxl_dom.c probably belong to patch #4
Ok. I will move these changes to the patch#4.

Regards,
Bhupinder
Stefano Stabellini May 1, 2017, 5:56 p.m. | #3
On Mon, 1 May 2017, Bhupinder Thakur wrote:
> Hi Stefano,
> 
> >> diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c
> >> index 446e766..ef3bd44 100644
> >> --- a/tools/libxl/libxl_console.c
> >> +++ b/tools/libxl/libxl_console.c
> >> @@ -67,6 +67,9 @@ int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num,
> >>      case LIBXL_CONSOLE_TYPE_SERIAL:
> >>          cons_type_s = "serial";
> >>          break;
> >> +    case LIBXL_CONSOLE_TYPE_VUART:
> >> +        cons_type_s = "vuart";
> >> +        break;
> >>      default:
> >>          goto out;
> >>      }
> >> @@ -326,6 +329,13 @@ 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 (state->vuart_enabled)
> >> +        {
> >> +            flexarray_append(ro_front, "vuart/0/port");
> >> +            flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vuart_port));
> >> +            flexarray_append(ro_front, "vuart/0/ring-ref");
> >> +            flexarray_append(ro_front, GCSPRINTF("%lu", state->vuart_mfn));
> >> +        }
> >
> > It looks like you are reusing the libxl__device_console_add call for the
> > main PV console for the domain, to also add the vuart nodes to xenstore.
> >
> > I don't think it is a good idea to mix the two. I suggest to introduce a
> > new libxl__device call to introduce the vuart nodes to xenstore, given
> > that they have no relantionship with the principal PV console of the
> > domain.
> >
> I have a doubt here. Do I have to create a new console device
> (libxl__device) to register the vuart console or can I use the
> existing console device (which is used for registering the primary
> console) to register the vuart nodes to xenstore?
> 
> I suspect that if I try to register with the same console device then
> libxl__device_generic_add() may fail as it is already added.

The vuart we are introducing with this patch series is a new and
different console from the existing PV console. So yes, I think we need
to create a new device for it.

We either need to introduce a brand new function to add the vuart to
xenstore, something like libxl__device_vuart_add, or, if we are going
to reuse libxl__device_console_add, then we we need to create both a new
console device (libxl__device_console), and a new libxl__device for it.
For clarity, libxl__device_console is the input parameter of
libxl__device_console_add, while libxl__device is the output parameter.


> >>      } 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 5d914a5..06ff3b7 100644
> >> --- a/tools/libxl/libxl_dom.c
> >> +++ b/tools/libxl/libxl_dom.c
> >> @@ -434,6 +434,9 @@ 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 (state->vuart_enabled)
> >> +        xc_domain_vuart_get_evtchn(ctx->xch, domid, &state->vuart_port);
> >> +
> >>      if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
> >>          hvm_set_conf_params(ctx->xch, domid, info);
> >>  #if defined(__i386__) || defined(__x86_64__)
> >> @@ -788,6 +791,7 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
> >>      if (xc_dom_translated(dom)) {
> >>          state->console_mfn = dom->console_pfn;
> >>          state->store_mfn = dom->xenstore_pfn;
> >> +        state->vuart_mfn = dom->vuart_pfn;
> >>      } else {
> >>          state->console_mfn = xc_dom_p2m(dom, dom->console_pfn);
> >>          state->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
> >
> > These two changes to libxl_dom.c probably belong to patch #4
> Ok. I will move these changes to the patch#4.
> 
> Regards,
> Bhupinder
>
Bhupinder Thakur May 3, 2017, 11 a.m. | #4
Hi,

>> > It looks like you are reusing the libxl__device_console_add call for the
>> > main PV console for the domain, to also add the vuart nodes to xenstore.
>> >
>> > I don't think it is a good idea to mix the two. I suggest to introduce a
>> > new libxl__device call to introduce the vuart nodes to xenstore, given
>> > that they have no relantionship with the principal PV console of the
>> > domain.
>> >
>> I have a doubt here. Do I have to create a new console device
>> (libxl__device) to register the vuart console or can I use the
>> existing console device (which is used for registering the primary
>> console) to register the vuart nodes to xenstore?
>>
>> I suspect that if I try to register with the same console device then
>> libxl__device_generic_add() may fail as it is already added.
>
> The vuart we are introducing with this patch series is a new and
> different console from the existing PV console. So yes, I think we need
> to create a new device for it.
>
> We either need to introduce a brand new function to add the vuart to
> xenstore, something like libxl__device_vuart_add, or, if we are going
> to reuse libxl__device_console_add, then we we need to create both a new
> console device (libxl__device_console), and a new libxl__device for it.
> For clarity, libxl__device_console is the input parameter of
> libxl__device_console_add, while libxl__device is the output parameter.
>
I am trying to add a new vuart device by defining a new device type
LIBXL__DEVICE_KIND_VUART  and a new function
libxl__device_vuart_add().

This function basically adds the vuart ring-ref ("0/ring-ref") and
port ("0/port") to ro_front array and then it registers the device
using the libxl__device_generic_add(). I pass NULL to "back" and
"front" array arguments to  libxl__device_generic_add() as I am adding
parameters only to "ro_front" array.

I am adding the device at /local/domain/<domid>/vuart.

However, xenconsoled fails to read the vuart ring-ref and port saying
"no such directory or file".I verified that
libxl__device_generic_add() is returning successfully.

In xenconsoled, I verified that I am reading
/local/domain/<domid>/vuart/0/ring-ref and
/local/domain/<domid>/vuart/0/port.

It is not clear why xenconsoled fails to read the values.

Regards,
Bhupinder
Stefano Stabellini May 3, 2017, 10:35 p.m. | #5
On Wed, 3 May 2017, Bhupinder Thakur wrote:
> Hi,
> 
> >> > It looks like you are reusing the libxl__device_console_add call for the
> >> > main PV console for the domain, to also add the vuart nodes to xenstore.
> >> >
> >> > I don't think it is a good idea to mix the two. I suggest to introduce a
> >> > new libxl__device call to introduce the vuart nodes to xenstore, given
> >> > that they have no relantionship with the principal PV console of the
> >> > domain.
> >> >
> >> I have a doubt here. Do I have to create a new console device
> >> (libxl__device) to register the vuart console or can I use the
> >> existing console device (which is used for registering the primary
> >> console) to register the vuart nodes to xenstore?
> >>
> >> I suspect that if I try to register with the same console device then
> >> libxl__device_generic_add() may fail as it is already added.
> >
> > The vuart we are introducing with this patch series is a new and
> > different console from the existing PV console. So yes, I think we need
> > to create a new device for it.
> >
> > We either need to introduce a brand new function to add the vuart to
> > xenstore, something like libxl__device_vuart_add, or, if we are going
> > to reuse libxl__device_console_add, then we we need to create both a new
> > console device (libxl__device_console), and a new libxl__device for it.
> > For clarity, libxl__device_console is the input parameter of
> > libxl__device_console_add, while libxl__device is the output parameter.
> >
> I am trying to add a new vuart device by defining a new device type
> LIBXL__DEVICE_KIND_VUART  and a new function
> libxl__device_vuart_add().

I think that is the right way to do it.


> This function basically adds the vuart ring-ref ("0/ring-ref") and
> port ("0/port") to ro_front array and then it registers the device
> using the libxl__device_generic_add(). I pass NULL to "back" and
> "front" array arguments to  libxl__device_generic_add() as I am adding
> parameters only to "ro_front" array.
> 
> I am adding the device at /local/domain/<domid>/vuart.
> 
> However, xenconsoled fails to read the vuart ring-ref and port saying
> "no such directory or file".I verified that
> libxl__device_generic_add() is returning successfully.
> 
> In xenconsoled, I verified that I am reading
> /local/domain/<domid>/vuart/0/ring-ref and
> /local/domain/<domid>/vuart/0/port.
> 
> It is not clear why xenconsoled fails to read the values.

Double check the permissions of those nodes with xenstore-ls -p. Make
sure there are no races between libxl writing the nodes and xenconsoled
reading them.
Bhupinder Thakur May 4, 2017, 7:37 p.m. | #6
Hi,


>> >> > It looks like you are reusing the libxl__device_console_add call for the
>> >> > main PV console for the domain, to also add the vuart nodes to xenstore.
>> >> >
>> >> > I don't think it is a good idea to mix the two. I suggest to introduce a
>> >> > new libxl__device call to introduce the vuart nodes to xenstore, given
>> >> > that they have no relantionship with the principal PV console of the
>> >> > domain.
>> >> >
>> >> I have a doubt here. Do I have to create a new console device
>> >> (libxl__device) to register the vuart console or can I use the
>> >> existing console device (which is used for registering the primary
>> >> console) to register the vuart nodes to xenstore?
>> >>
>> >> I suspect that if I try to register with the same console device then
>> >> libxl__device_generic_add() may fail as it is already added.
>> >
>> > The vuart we are introducing with this patch series is a new and
>> > different console from the existing PV console. So yes, I think we need
>> > to create a new device for it.
>> >
>> > We either need to introduce a brand new function to add the vuart to
>> > xenstore, something like libxl__device_vuart_add, or, if we are going
>> > to reuse libxl__device_console_add, then we we need to create both a new
>> > console device (libxl__device_console), and a new libxl__device for it.
>> > For clarity, libxl__device_console is the input parameter of
>> > libxl__device_console_add, while libxl__device is the output parameter.
>> >
>> I am trying to add a new vuart device by defining a new device type
>> LIBXL__DEVICE_KIND_VUART  and a new function
>> libxl__device_vuart_add().
>
> I think that is the right way to do it.
>
>
>> This function basically adds the vuart ring-ref ("0/ring-ref") and
>> port ("0/port") to ro_front array and then it registers the device
>> using the libxl__device_generic_add(). I pass NULL to "back" and
>> "front" array arguments to  libxl__device_generic_add() as I am adding
>> parameters only to "ro_front" array.
>>
>> I am adding the device at /local/domain/<domid>/vuart.
>>
>> However, xenconsoled fails to read the vuart ring-ref and port saying
>> "no such directory or file".I verified that
>> libxl__device_generic_add() is returning successfully.
>>
>> In xenconsoled, I verified that I am reading
>> /local/domain/<domid>/vuart/0/ring-ref and
>> /local/domain/<domid>/vuart/0/port.
>>
>> It is not clear why xenconsoled fails to read the values.
>
> Double check the permissions of those nodes with xenstore-ls -p. Make
> sure there are no races between libxl writing the nodes and xenconsoled
> reading them.

xenstore-ls -p" shows that the vuart node is created successfully. I
have pasted below the relevant part of the command output:

   vuart = ""   (n0,r1)
    backend = "/local/domain/0/backend/vuart/1/0"   (n0,r1)
    0 = ""   (n0,r1)
     port = "1"   (n0,r1)
     ring-ref = "233475"   (n0,r1)
     limit = "1048576"   (n0,r1)
     type = "xenconsoled"   (n0,r1)
   console = ""   (n0,r1)
    backend = "/local/domain/0/backend/console/1/0"   (n0,r1)
    backend-id = "0"   (n1,r0)
    limit = "1048576"   (n0,r1)
    type = "xenconsoled"   (n0,r1)
    output = "pty"   (n0,r1)
    tty = "/dev/pts/0"   (n0,r1)
    port = "3"   (n0,r1)
    ring-ref = "233472"   (n0,r1)

The vuart node exists at /local/domain/1/vuart. I am creating the
vuart node just before the pv console node to make sure that vuart
node becomes visible before the pv console node is created. The values
are populated properly in the vuart node. Still when xenconsoled tries
to read /loca/domain/1/vuart/0/ring-ref or
/loca/domain/1/vuart/0/port, the read fails.

PV console node reads happen properly even though both nodes exist at
the same level and their access permissions are also same.

Regards,
Bhupinder
Stefano Stabellini May 4, 2017, 8:38 p.m. | #7
On Fri, 5 May 2017, Bhupinder Thakur wrote:
> Hi,
> 
> 
> >> >> > It looks like you are reusing the libxl__device_console_add call for the
> >> >> > main PV console for the domain, to also add the vuart nodes to xenstore.
> >> >> >
> >> >> > I don't think it is a good idea to mix the two. I suggest to introduce a
> >> >> > new libxl__device call to introduce the vuart nodes to xenstore, given
> >> >> > that they have no relantionship with the principal PV console of the
> >> >> > domain.
> >> >> >
> >> >> I have a doubt here. Do I have to create a new console device
> >> >> (libxl__device) to register the vuart console or can I use the
> >> >> existing console device (which is used for registering the primary
> >> >> console) to register the vuart nodes to xenstore?
> >> >>
> >> >> I suspect that if I try to register with the same console device then
> >> >> libxl__device_generic_add() may fail as it is already added.
> >> >
> >> > The vuart we are introducing with this patch series is a new and
> >> > different console from the existing PV console. So yes, I think we need
> >> > to create a new device for it.
> >> >
> >> > We either need to introduce a brand new function to add the vuart to
> >> > xenstore, something like libxl__device_vuart_add, or, if we are going
> >> > to reuse libxl__device_console_add, then we we need to create both a new
> >> > console device (libxl__device_console), and a new libxl__device for it.
> >> > For clarity, libxl__device_console is the input parameter of
> >> > libxl__device_console_add, while libxl__device is the output parameter.
> >> >
> >> I am trying to add a new vuart device by defining a new device type
> >> LIBXL__DEVICE_KIND_VUART  and a new function
> >> libxl__device_vuart_add().
> >
> > I think that is the right way to do it.
> >
> >
> >> This function basically adds the vuart ring-ref ("0/ring-ref") and
> >> port ("0/port") to ro_front array and then it registers the device
> >> using the libxl__device_generic_add(). I pass NULL to "back" and
> >> "front" array arguments to  libxl__device_generic_add() as I am adding
> >> parameters only to "ro_front" array.
> >>
> >> I am adding the device at /local/domain/<domid>/vuart.
> >>
> >> However, xenconsoled fails to read the vuart ring-ref and port saying
> >> "no such directory or file".I verified that
> >> libxl__device_generic_add() is returning successfully.
> >>
> >> In xenconsoled, I verified that I am reading
> >> /local/domain/<domid>/vuart/0/ring-ref and
> >> /local/domain/<domid>/vuart/0/port.
> >>
> >> It is not clear why xenconsoled fails to read the values.
> >
> > Double check the permissions of those nodes with xenstore-ls -p. Make
> > sure there are no races between libxl writing the nodes and xenconsoled
> > reading them.
> 
> xenstore-ls -p" shows that the vuart node is created successfully. I
> have pasted below the relevant part of the command output:
> 
>    vuart = ""   (n0,r1)
>     backend = "/local/domain/0/backend/vuart/1/0"   (n0,r1)

Why is this here instead of under vuart/0?
Also, if these are the frontend nodes, where are the backend vuart nodes?


>     0 = ""   (n0,r1)
>      port = "1"   (n0,r1)
>      ring-ref = "233475"   (n0,r1)
>      limit = "1048576"   (n0,r1)
>      type = "xenconsoled"   (n0,r1)
>    console = ""   (n0,r1)
>     backend = "/local/domain/0/backend/console/1/0"   (n0,r1)
>     backend-id = "0"   (n1,r0)
>     limit = "1048576"   (n0,r1)
>     type = "xenconsoled"   (n0,r1)
>     output = "pty"   (n0,r1)
>     tty = "/dev/pts/0"   (n0,r1)
>     port = "3"   (n0,r1)
>     ring-ref = "233472"   (n0,r1)
> 
> The vuart node exists at /local/domain/1/vuart. I am creating the
> vuart node just before the pv console node to make sure that vuart
> node becomes visible before the pv console node is created. The values
> are populated properly in the vuart node. Still when xenconsoled tries
> to read /loca/domain/1/vuart/0/ring-ref or
> /loca/domain/1/vuart/0/port, the read fails.
> 
> PV console node reads happen properly even though both nodes exist at
> the same level and their access permissions are also same.

I don't know. I would add more debug printfs to the code until I
understood what was going on.
Bhupinder Thakur May 5, 2017, 9:52 a.m. | #8
Hi Stefano,

>> >> >> > It looks like you are reusing the libxl__device_console_add call for the
>> >> >> > main PV console for the domain, to also add the vuart nodes to xenstore.
>> >> >> >
>> >> >> > I don't think it is a good idea to mix the two. I suggest to introduce a
>> >> >> > new libxl__device call to introduce the vuart nodes to xenstore, given
>> >> >> > that they have no relantionship with the principal PV console of the
>> >> >> > domain.
>> >> >> >
>> >> >> I have a doubt here. Do I have to create a new console device
>> >> >> (libxl__device) to register the vuart console or can I use the
>> >> >> existing console device (which is used for registering the primary
>> >> >> console) to register the vuart nodes to xenstore?
>> >> >>
>> >> >> I suspect that if I try to register with the same console device then
>> >> >> libxl__device_generic_add() may fail as it is already added.
>> >> >
>> >> > The vuart we are introducing with this patch series is a new and
>> >> > different console from the existing PV console. So yes, I think we need
>> >> > to create a new device for it.
>> >> >
>> >> > We either need to introduce a brand new function to add the vuart to
>> >> > xenstore, something like libxl__device_vuart_add, or, if we are going
>> >> > to reuse libxl__device_console_add, then we we need to create both a new
>> >> > console device (libxl__device_console), and a new libxl__device for it.
>> >> > For clarity, libxl__device_console is the input parameter of
>> >> > libxl__device_console_add, while libxl__device is the output parameter.
>> >> >
>> >> I am trying to add a new vuart device by defining a new device type
>> >> LIBXL__DEVICE_KIND_VUART  and a new function
>> >> libxl__device_vuart_add().
>> >
>> > I think that is the right way to do it.
>> >
>> >
>> >> This function basically adds the vuart ring-ref ("0/ring-ref") and
>> >> port ("0/port") to ro_front array and then it registers the device
>> >> using the libxl__device_generic_add(). I pass NULL to "back" and
>> >> "front" array arguments to  libxl__device_generic_add() as I am adding
>> >> parameters only to "ro_front" array.
>> >>
>> >> I am adding the device at /local/domain/<domid>/vuart.
>> >>
>> >> However, xenconsoled fails to read the vuart ring-ref and port saying
>> >> "no such directory or file".I verified that
>> >> libxl__device_generic_add() is returning successfully.
>> >>
>> >> In xenconsoled, I verified that I am reading
>> >> /local/domain/<domid>/vuart/0/ring-ref and
>> >> /local/domain/<domid>/vuart/0/port.
>> >>
>> >> It is not clear why xenconsoled fails to read the values.
>> >
>> > Double check the permissions of those nodes with xenstore-ls -p. Make
>> > sure there are no races between libxl writing the nodes and xenconsoled
>> > reading them.
>>
>> xenstore-ls -p" shows that the vuart node is created successfully. I
>> have pasted below the relevant part of the command output:
>>
>>    vuart = ""   (n0,r1)
>>     backend = "/local/domain/0/backend/vuart/1/0"   (n0,r1)
>
> Why is this here instead of under vuart/0?
> Also, if these are the frontend nodes, where are the backend vuart nodes?

I created only frontend nodes so that xenconsoled could access
ring-ref/port. I understand that backend nodes are required for guest
domains to access some backend information. Since in this case guest
domain does not need such information I did not create the backend
node. Kindly let me know if my understanding is correct.
>
>
>>     0 = ""   (n0,r1)
>>      port = "1"   (n0,r1)
>>      ring-ref = "233475"   (n0,r1)
>>      limit = "1048576"   (n0,r1)
>>      type = "xenconsoled"   (n0,r1)
>>    console = ""   (n0,r1)
>>     backend = "/local/domain/0/backend/console/1/0"   (n0,r1)
>>     backend-id = "0"   (n1,r0)
>>     limit = "1048576"   (n0,r1)
>>     type = "xenconsoled"   (n0,r1)
>>     output = "pty"   (n0,r1)
>>     tty = "/dev/pts/0"   (n0,r1)
>>     port = "3"   (n0,r1)
>>     ring-ref = "233472"   (n0,r1)
>>
>> The vuart node exists at /local/domain/1/vuart. I am creating the
>> vuart node just before the pv console node to make sure that vuart
>> node becomes visible before the pv console node is created. The values
>> are populated properly in the vuart node. Still when xenconsoled tries
>> to read /loca/domain/1/vuart/0/ring-ref or
>> /loca/domain/1/vuart/0/port, the read fails.
>>
>> PV console node reads happen properly even though both nodes exist at
>> the same level and their access permissions are also same.
>
> I don't know. I would add more debug printfs to the code until I
> understood what was going on.
There was one issue in domain_create_ring() where if the vuart node
was populated later then it was not able to be open the tty session
since the check was based on whether pv tty was initialized (which was
initialized earlier).

Earlier, this was not the case since vuart and pv xenstore parameters
were always available together.

I have defined a new function console_create_ring() which is called
for both consoles. This new function removes lot of duplicate code for
pv and vuart consoles in domain_create_ring(). With this change, both
the consoles are working fine now.

Regards,
Bhupinder
Stefano Stabellini May 5, 2017, 6:59 p.m. | #9
On Fri, 5 May 2017, Bhupinder Thakur wrote:
> Hi Stefano,
> 
> >> >> >> > It looks like you are reusing the libxl__device_console_add call for the
> >> >> >> > main PV console for the domain, to also add the vuart nodes to xenstore.
> >> >> >> >
> >> >> >> > I don't think it is a good idea to mix the two. I suggest to introduce a
> >> >> >> > new libxl__device call to introduce the vuart nodes to xenstore, given
> >> >> >> > that they have no relantionship with the principal PV console of the
> >> >> >> > domain.
> >> >> >> >
> >> >> >> I have a doubt here. Do I have to create a new console device
> >> >> >> (libxl__device) to register the vuart console or can I use the
> >> >> >> existing console device (which is used for registering the primary
> >> >> >> console) to register the vuart nodes to xenstore?
> >> >> >>
> >> >> >> I suspect that if I try to register with the same console device then
> >> >> >> libxl__device_generic_add() may fail as it is already added.
> >> >> >
> >> >> > The vuart we are introducing with this patch series is a new and
> >> >> > different console from the existing PV console. So yes, I think we need
> >> >> > to create a new device for it.
> >> >> >
> >> >> > We either need to introduce a brand new function to add the vuart to
> >> >> > xenstore, something like libxl__device_vuart_add, or, if we are going
> >> >> > to reuse libxl__device_console_add, then we we need to create both a new
> >> >> > console device (libxl__device_console), and a new libxl__device for it.
> >> >> > For clarity, libxl__device_console is the input parameter of
> >> >> > libxl__device_console_add, while libxl__device is the output parameter.
> >> >> >
> >> >> I am trying to add a new vuart device by defining a new device type
> >> >> LIBXL__DEVICE_KIND_VUART  and a new function
> >> >> libxl__device_vuart_add().
> >> >
> >> > I think that is the right way to do it.
> >> >
> >> >
> >> >> This function basically adds the vuart ring-ref ("0/ring-ref") and
> >> >> port ("0/port") to ro_front array and then it registers the device
> >> >> using the libxl__device_generic_add(). I pass NULL to "back" and
> >> >> "front" array arguments to  libxl__device_generic_add() as I am adding
> >> >> parameters only to "ro_front" array.
> >> >>
> >> >> I am adding the device at /local/domain/<domid>/vuart.
> >> >>
> >> >> However, xenconsoled fails to read the vuart ring-ref and port saying
> >> >> "no such directory or file".I verified that
> >> >> libxl__device_generic_add() is returning successfully.
> >> >>
> >> >> In xenconsoled, I verified that I am reading
> >> >> /local/domain/<domid>/vuart/0/ring-ref and
> >> >> /local/domain/<domid>/vuart/0/port.
> >> >>
> >> >> It is not clear why xenconsoled fails to read the values.
> >> >
> >> > Double check the permissions of those nodes with xenstore-ls -p. Make
> >> > sure there are no races between libxl writing the nodes and xenconsoled
> >> > reading them.
> >>
> >> xenstore-ls -p" shows that the vuart node is created successfully. I
> >> have pasted below the relevant part of the command output:
> >>
> >>    vuart = ""   (n0,r1)
> >>     backend = "/local/domain/0/backend/vuart/1/0"   (n0,r1)
> >
> > Why is this here instead of under vuart/0?
> > Also, if these are the frontend nodes, where are the backend vuart nodes?
> 
> I created only frontend nodes so that xenconsoled could access
> ring-ref/port. I understand that backend nodes are required for guest
> domains to access some backend information. Since in this case guest
> domain does not need such information I did not create the backend
> node. Kindly let me know if my understanding is correct.

If there are no backend xenstore nodes, why do we have:

  backend = "/local/domain/0/backend/vuart/1/0"

on Xenstore?




> >
> >
> >>     0 = ""   (n0,r1)
> >>      port = "1"   (n0,r1)
> >>      ring-ref = "233475"   (n0,r1)
> >>      limit = "1048576"   (n0,r1)
> >>      type = "xenconsoled"   (n0,r1)
> >>    console = ""   (n0,r1)
> >>     backend = "/local/domain/0/backend/console/1/0"   (n0,r1)
> >>     backend-id = "0"   (n1,r0)
> >>     limit = "1048576"   (n0,r1)
> >>     type = "xenconsoled"   (n0,r1)
> >>     output = "pty"   (n0,r1)
> >>     tty = "/dev/pts/0"   (n0,r1)
> >>     port = "3"   (n0,r1)
> >>     ring-ref = "233472"   (n0,r1)
Bhupinder Thakur May 8, 2017, 5:37 a.m. | #10
Hi Stefano,

>> >> >> >> > It looks like you are reusing the libxl__device_console_add call for the
>> >> >> >> > main PV console for the domain, to also add the vuart nodes to xenstore.
>> >> >> >> >
>> >> >> >> > I don't think it is a good idea to mix the two. I suggest to introduce a
>> >> >> >> > new libxl__device call to introduce the vuart nodes to xenstore, given
>> >> >> >> > that they have no relantionship with the principal PV console of the
>> >> >> >> > domain.
>> >> >> >> >
>> >> >> >> I have a doubt here. Do I have to create a new console device
>> >> >> >> (libxl__device) to register the vuart console or can I use the
>> >> >> >> existing console device (which is used for registering the primary
>> >> >> >> console) to register the vuart nodes to xenstore?
>> >> >> >>
>> >> >> >> I suspect that if I try to register with the same console device then
>> >> >> >> libxl__device_generic_add() may fail as it is already added.
>> >> >> >
>> >> >> > The vuart we are introducing with this patch series is a new and
>> >> >> > different console from the existing PV console. So yes, I think we need
>> >> >> > to create a new device for it.
>> >> >> >
>> >> >> > We either need to introduce a brand new function to add the vuart to
>> >> >> > xenstore, something like libxl__device_vuart_add, or, if we are going
>> >> >> > to reuse libxl__device_console_add, then we we need to create both a new
>> >> >> > console device (libxl__device_console), and a new libxl__device for it.
>> >> >> > For clarity, libxl__device_console is the input parameter of
>> >> >> > libxl__device_console_add, while libxl__device is the output parameter.
>> >> >> >
>> >> >> I am trying to add a new vuart device by defining a new device type
>> >> >> LIBXL__DEVICE_KIND_VUART  and a new function
>> >> >> libxl__device_vuart_add().
>> >> >
>> >> > I think that is the right way to do it.
>> >> >
>> >> >
>> >> >> This function basically adds the vuart ring-ref ("0/ring-ref") and
>> >> >> port ("0/port") to ro_front array and then it registers the device
>> >> >> using the libxl__device_generic_add(). I pass NULL to "back" and
>> >> >> "front" array arguments to  libxl__device_generic_add() as I am adding
>> >> >> parameters only to "ro_front" array.
>> >> >>
>> >> >> I am adding the device at /local/domain/<domid>/vuart.
>> >> >>
>> >> >> However, xenconsoled fails to read the vuart ring-ref and port saying
>> >> >> "no such directory or file".I verified that
>> >> >> libxl__device_generic_add() is returning successfully.
>> >> >>
>> >> >> In xenconsoled, I verified that I am reading
>> >> >> /local/domain/<domid>/vuart/0/ring-ref and
>> >> >> /local/domain/<domid>/vuart/0/port.
>> >> >>
>> >> >> It is not clear why xenconsoled fails to read the values.
>> >> >
>> >> > Double check the permissions of those nodes with xenstore-ls -p. Make
>> >> > sure there are no races between libxl writing the nodes and xenconsoled
>> >> > reading them.
>> >>
>> >> xenstore-ls -p" shows that the vuart node is created successfully. I
>> >> have pasted below the relevant part of the command output:
>> >>
>> >>    vuart = ""   (n0,r1)
>> >>     backend = "/local/domain/0/backend/vuart/1/0"   (n0,r1)
>> >
>> > Why is this here instead of under vuart/0?
>> > Also, if these are the frontend nodes, where are the backend vuart nodes?
>>
>> I created only frontend nodes so that xenconsoled could access
>> ring-ref/port. I understand that backend nodes are required for guest
>> domains to access some backend information. Since in this case guest
>> domain does not need such information I did not create the backend
>> node. Kindly let me know if my understanding is correct.
>
> If there are no backend xenstore nodes, why do we have:
>
>   backend = "/local/domain/0/backend/vuart/1/0"
>
> on Xenstore?
>
libxl__device_generic_add() creates the references to backend and
frontend even if those nodes are not created (i.e. the node
information passed to the function are null). I will add a check in
libxl__device_generic_add() to add these references only if the
corresponding nodes have been created in xenstore.

Regards,
Bhupinder

Patch hide | download patch | download mbox

diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c
index 446e766..ef3bd44 100644
--- a/tools/libxl/libxl_console.c
+++ b/tools/libxl/libxl_console.c
@@ -67,6 +67,9 @@  int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num,
     case LIBXL_CONSOLE_TYPE_SERIAL:
         cons_type_s = "serial";
         break;
+    case LIBXL_CONSOLE_TYPE_VUART:
+        cons_type_s = "vuart";
+        break;
     default:
         goto out;
     }
@@ -326,6 +329,13 @@  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 (state->vuart_enabled)
+        {
+            flexarray_append(ro_front, "vuart/0/port");
+            flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vuart_port));
+            flexarray_append(ro_front, "vuart/0/ring-ref");
+            flexarray_append(ro_front, GCSPRINTF("%lu", state->vuart_mfn));
+        }
     } 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 5d914a5..06ff3b7 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -434,6 +434,9 @@  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 (state->vuart_enabled)
+        xc_domain_vuart_get_evtchn(ctx->xch, domid, &state->vuart_port);
+
     if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
         hvm_set_conf_params(ctx->xch, domid, info);
 #if defined(__i386__) || defined(__x86_64__)
@@ -788,6 +791,7 @@  int libxl__build_pv(libxl__gc *gc, uint32_t domid,
     if (xc_dom_translated(dom)) {
         state->console_mfn = dom->console_pfn;
         state->store_mfn = dom->xenstore_pfn;
+        state->vuart_mfn = dom->vuart_pfn;
     } else {
         state->console_mfn = xc_dom_p2m(dom, dom->console_pfn);
         state->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);