Re: [PATCH 06/10 v2] xen/arm: vpl011: Add vuart ring-buf and evtchn to xenstore

From: Bhupinder Thakur <bhupinder.thakur@linaro.org>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wei.liu2@citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	Jan Beulich <jbeulich@suse.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH 06/10 v2] xen/arm: vpl011: Add vuart ring-buf and evtchn to xenstore
Date: Fri, 5 May 2017 15:22:47 +0530	[thread overview]
Message-ID: <CACtJ1JT8RpkyO5kSEgAcW1ZLLbE+q4zyoScqn3PoUWZFWpKdvA@mail.gmail.com> (raw)
In-Reply-To: <alpine.DEB.2.10.1705041332170.9240@sstabellini-ThinkPad-X260>

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

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