| Message ID | alpine.DEB.2.02.1408071519110.2293@kaball.uk.xensource.com |
|---|---|
| State | New |
| Headers | show |
On 7 Aug 2014, at 15:26, Stefano Stabellini <Stefano.Stabellini@eu.citrix.com> wrote: > On Thu, 7 Aug 2014, David Vrabel wrote: >> On 07/08/14 14:37, Dave Scott wrote: >>> >>> On 28 Jul 2014, at 15:22, David Vrabel <david.vrabel@citrix.com> wrote: >>> >>>> On 22/07/14 16:05, David Scott wrote: >>>>> >>>>> Note: I've seen a problem with some Linux console frontends which attempt >>>>> to overwrite the read-only key 'type' (= xenconsoled|ioemu) in the frontend >>>>> directory. [ this key is already present, it's not added by these patches ] >>>>> Since 'type' refers to the toolstack's choice of implementation service >>>>> (which is none of the guest's business) I think the read/only permissions >>>>> are right. The location of the key is not ideal (it should be in the >>>>> backend directory IMHO) but this is the case with several other keys located >>>>> in the frontend such as 'limit' and 'tty'. I think this is a Linux frontend >>>>> bug which should be fixed there. These patches work with Mirage VMs and >>>>> with Linux if I workaround the permissions by giving the guest read/write >>>>> to the 'type' key. >>>> >>>> Which Linux front ends? >>> >>> I just tested a Debian jessie with >>> >>> # uname -a >>> Linux jessie 3.13-6-686-pae #1 SMP Debian 3.13.5-1 (2014-03-04) i686 GNU/Linux >>> >>> In my xenstored access log I see: >>> >>> Aug 7 14:28:30 localhost xenstored: D40.6 write device/console/1/ring-ref 8 >>> Aug 7 14:28:30 localhost xenstored: D40.6 write device/console/1/port 10 >>> Aug 7 14:28:30 localhost xenstored: D40.6 write device/console/1/type ioemu >>> >>> — I think the last line is suspicious. >> >> This was introduced by Stefano in 3.4 with 02e19f9c7cac (hvc_xen: >> implement multiconsole support). >> >> Stefano, can you advise on whether it it safe to remove the write of the >> "type" key or whether we need to make it conditional on it being absent. > > I think that the original idea was that since only ioemu backends (QEMU) > are capable of handling multiple consoles and the new xenstore based > console protocol, then the "type" had to be "ioemu". > > I agree that the type key has no business being in the frontend > directory. > I also agree that the frontend shouldn't be writing it, since the > backend would have already been started anyway. Would you like me to move the key to the backend directory? I could leave a writable (but unused) key in the frontend directory for backwards compatibility. Or should I leave this alone? > > I think it would be OK to: > > --- > xen_hvc: no reason to write the type key on xenstore > > The backend type is chosen by the toolstack. Regardless the frontend > should not care. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c > index 636c9ba..b8491f5 100644 > --- a/drivers/tty/hvc/hvc_xen.c > +++ b/drivers/tty/hvc/hvc_xen.c > @@ -402,9 +402,6 @@ static int xencons_connect_backend(struct xenbus_device *dev, > evtchn); > if (ret) > goto error_xenbus; > - ret = xenbus_printf(xbt, dev->nodename, "type", "ioemu"); > - if (ret) > - goto error_xenbus; > ret = xenbus_transaction_end(xbt, 0); > if (ret) { > if (ret == -EAGAIN) This looks good to me :-) Cheers, Dave
On Mon, 11 Aug 2014, Dave Scott wrote: > On 7 Aug 2014, at 15:26, Stefano Stabellini <Stefano.Stabellini@eu.citrix.com> wrote: > > > On Thu, 7 Aug 2014, David Vrabel wrote: > >> On 07/08/14 14:37, Dave Scott wrote: > >>> > >>> On 28 Jul 2014, at 15:22, David Vrabel <david.vrabel@citrix.com> wrote: > >>> > >>>> On 22/07/14 16:05, David Scott wrote: > >>>>> > >>>>> Note: I've seen a problem with some Linux console frontends which attempt > >>>>> to overwrite the read-only key 'type' (= xenconsoled|ioemu) in the frontend > >>>>> directory. [ this key is already present, it's not added by these patches ] > >>>>> Since 'type' refers to the toolstack's choice of implementation service > >>>>> (which is none of the guest's business) I think the read/only permissions > >>>>> are right. The location of the key is not ideal (it should be in the > >>>>> backend directory IMHO) but this is the case with several other keys located > >>>>> in the frontend such as 'limit' and 'tty'. I think this is a Linux frontend > >>>>> bug which should be fixed there. These patches work with Mirage VMs and > >>>>> with Linux if I workaround the permissions by giving the guest read/write > >>>>> to the 'type' key. > >>>> > >>>> Which Linux front ends? > >>> > >>> I just tested a Debian jessie with > >>> > >>> # uname -a > >>> Linux jessie 3.13-6-686-pae #1 SMP Debian 3.13.5-1 (2014-03-04) i686 GNU/Linux > >>> > >>> In my xenstored access log I see: > >>> > >>> Aug 7 14:28:30 localhost xenstored: D40.6 write device/console/1/ring-ref 8 > >>> Aug 7 14:28:30 localhost xenstored: D40.6 write device/console/1/port 10 > >>> Aug 7 14:28:30 localhost xenstored: D40.6 write device/console/1/type ioemu > >>> > >>> — I think the last line is suspicious. > >> > >> This was introduced by Stefano in 3.4 with 02e19f9c7cac (hvc_xen: > >> implement multiconsole support). > >> > >> Stefano, can you advise on whether it it safe to remove the write of the > >> "type" key or whether we need to make it conditional on it being absent. > > > > I think that the original idea was that since only ioemu backends (QEMU) > > are capable of handling multiple consoles and the new xenstore based > > console protocol, then the "type" had to be "ioemu". > > > > I agree that the type key has no business being in the frontend > > directory. > > I also agree that the frontend shouldn't be writing it, since the > > backend would have already been started anyway. > > Would you like me to move the key to the backend directory? I could leave a writable (but unused) key in the frontend directory for backwards compatibility. Or should I leave this alone? Like you say, we cannot get rid of key in the frontend directory without breaking existing backends. In particular QEMU is often built out of the Xen tree by distros: we cannot rely on it having the necessary modifications. Duplicating the key to the backend directory, with the intent of removing the copy in the frontend directory might be the right thing to do. We could change all the backends to look in the backend directory then in a couple of years finally remove the copy in the frontend path. I am not sure if it's worth the effort though :-) > > I think it would be OK to: > > > > --- > > xen_hvc: no reason to write the type key on xenstore > > > > The backend type is chosen by the toolstack. Regardless the frontend > > should not care. > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > > > diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c > > index 636c9ba..b8491f5 100644 > > --- a/drivers/tty/hvc/hvc_xen.c > > +++ b/drivers/tty/hvc/hvc_xen.c > > @@ -402,9 +402,6 @@ static int xencons_connect_backend(struct xenbus_device *dev, > > evtchn); > > if (ret) > > goto error_xenbus; > > - ret = xenbus_printf(xbt, dev->nodename, "type", "ioemu"); > > - if (ret) > > - goto error_xenbus; > > ret = xenbus_transaction_end(xbt, 0); > > if (ret) { > > if (ret == -EAGAIN) > > This looks good to me :-) I'll send out the patch.
diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c index 636c9ba..b8491f5 100644 --- a/drivers/tty/hvc/hvc_xen.c +++ b/drivers/tty/hvc/hvc_xen.c @@ -402,9 +402,6 @@ static int xencons_connect_backend(struct xenbus_device *dev, evtchn); if (ret) goto error_xenbus; - ret = xenbus_printf(xbt, dev->nodename, "type", "ioemu"); - if (ret) - goto error_xenbus; ret = xenbus_transaction_end(xbt, 0); if (ret) { if (ret == -EAGAIN)