| Message ID | 87ip2wr8tp.fsf@rustcorp.com.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 6 May 2013 06:11, Rusty Russell <rusty@rustcorp.com.au> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: >> To be actually useful we need to also specify something in >> the device tree to say "here is where you will find your >> emergency output and what it is". > > Hmm, I'm not sure that's true. It looks like it needs: > > 1) An enhancment to the vdev->set_config callback to pass through (at > least) an offset, probably offset and length. > > 2) An emerg_write() function ptr which can be called at any time, set by > virtio_console.c's class_init: That all looks like sensible QEMU implementation possibilities but it seems to be a bit of a non-sequitur from "how do we tell the kernel to actually use this?" -- PMM
Hi Rusty, On 6 May 2013 10:41, Rusty Russell <rusty@rustcorp.com.au> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: >> On 1 May 2013 03:07, Rusty Russell <rusty@rustcorp.com.au> wrote: >>> An emergency output is a reasonable idea, and this is a reasonable >>> implementation. The question is practical: will it be used? Because we >>> don't implement reasonable ideas which aren't going to be used. >> >> If you think it fits reasonably into the virtio spec (ie doesn't >> implement things at the wrong level of the transport/backend >> abstraction) then we can implement it in QEMU, and I think it >> makes more sense to do this than to throw in a random extra >> serial port. >> >> To be actually useful we need to also specify something in >> the device tree to say "here is where you will find your >> emergency output and what it is". > > Hmm, I'm not sure that's true. It looks like it needs: > > 1) An enhancment to the vdev->set_config callback to pass through (at > least) an offset, probably offset and length. > > 2) An emerg_write() function ptr which can be called at any time, set by > virtio_console.c's class_init: > > static void emerg_write(VirtIOSerialPort *port, char c) > { > VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port); > > if (vcon->chr) > qemu_chr_fe_write(vcon->chr, &c, 1); > } > > 3) A routine to find an emerg-write-capable console in > virtio_serial_bus.c (or just assume port 0?): > > static VirtIOSerialPort *find_emerg_write_port(VirtIOSerial *vser) > { > VirtIOSerialPort *port; > > QTAILQ_FOREACH(port, &vser->ports, next) { > VirtIOSerialPortClass *vsc; > vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port); > > if (vsc->emerg_write) { > return port; > } > } > return NULL; > } > > 4) set_config in virtio_serial_bus.c to notice emergency writes: > > if (offset == offsetof(struct virtio_console_config, emerg_w) { > VirtIOSerial *vser; > vser = DO_UPCAST(VirtIOSerial, vdev, vdev); > VirtIOSerialPort *port; > > port = find_emerg_write_port(vser); > if (port) { > vsc->emerg_write(port, config.emerg_w); > } > } > > Amit might have more clue... Amit? > > Thanks, > Rusty. > > From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> > Subject: virtio: console: Add early writeonly register to config space > > This patch adds a emerg_wr register (writeonly) in config space of virtio console device which can be used for debugging. > > Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> > Signed-off-by: Anup Patel <anup.patel@linaro.org> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > --- > include/uapi/linux/virtio_console.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/include/uapi/linux/virtio_console.h b/include/uapi/linux/virtio_console.h > index ee13ab6..586678d 100644 > --- a/include/uapi/linux/virtio_console.h > +++ b/include/uapi/linux/virtio_console.h > @@ -38,6 +38,7 @@ > /* Feature bits */ > #define VIRTIO_CONSOLE_F_SIZE 0 /* Does host provide console size? */ > #define VIRTIO_CONSOLE_F_MULTIPORT 1 /* Does host provide multiple ports? */ > +#define VIRTIO_CONSOLE_F_EMERG_WRITE 2 /* Does host support emergency write? */ > > #define VIRTIO_CONSOLE_BAD_ID (~(u32)0) > > @@ -48,6 +49,8 @@ struct virtio_console_config { > __u16 rows; > /* max. number of ports this device can hold */ > __u32 max_nr_ports; > + /* emergency write register */ > + __u32 emerg_wr; > } __attribute__((packed)); > > /* I will send above patch with V2 tagged with the addition of documentation change and also separate arm64 early printk implementation for the same. Thanks, Pranav
Peter Maydell <peter.maydell@linaro.org> writes: > On 6 May 2013 06:11, Rusty Russell <rusty@rustcorp.com.au> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >>> To be actually useful we need to also specify something in >>> the device tree to say "here is where you will find your >>> emergency output and what it is". >> >> Hmm, I'm not sure that's true. It looks like it needs: >> >> 1) An enhancment to the vdev->set_config callback to pass through (at >> least) an offset, probably offset and length. >> >> 2) An emerg_write() function ptr which can be called at any time, set by >> virtio_console.c's class_init: > > That all looks like sensible QEMU implementation possibilities > but it seems to be a bit of a non-sequitur from "how do we > tell the kernel to actually use this?" You enable the feature in the virtio console device, and a kernel compiled with EARLY_PRINTK will use it? Cheers, Rusty.
On 7 May 2013 05:46, Rusty Russell <rusty@rustcorp.com.au> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: >> That all looks like sensible QEMU implementation possibilities >> but it seems to be a bit of a non-sequitur from "how do we >> tell the kernel to actually use this?" > > You enable the feature in the virtio console device, and a kernel > compiled with EARLY_PRINTK will use it? Well, at the moment EARLY_PRINTK is hardcoded to "use some specific UART or equivalent selected at compile time". So the equivalent presumably would be to hard-compile "use virtio-console", but then how does that code know where the virtio-console is in the address space? -- PMM
On 05/07/2013 08:19 AM, Peter Maydell wrote: > On 7 May 2013 05:46, Rusty Russell <rusty@rustcorp.com.au> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >>> That all looks like sensible QEMU implementation possibilities >>> but it seems to be a bit of a non-sequitur from "how do we >>> tell the kernel to actually use this?" >> >> You enable the feature in the virtio console device, and a kernel >> compiled with EARLY_PRINTK will use it? > > Well, at the moment EARLY_PRINTK is hardcoded to > "use some specific UART or equivalent selected at > compile time". So the equivalent presumably would > be to hard-compile "use virtio-console", but then > how does that code know where the virtio-console > is in the address space? arm64 uses a kernel argument. Christopher
On 7 May 2013 16:52, Christopher Covington <cov@codeaurora.org> wrote: > On 05/07/2013 08:19 AM, Peter Maydell wrote: >> Well, at the moment EARLY_PRINTK is hardcoded to >> "use some specific UART or equivalent selected at >> compile time". So the equivalent presumably would >> be to hard-compile "use virtio-console", but then >> how does that code know where the virtio-console >> is in the address space? > > arm64 uses a kernel argument. This mixes up "information that the user provides to the kernel" (ie configuration) with "information that QEMU or kvmtool should provide to the kernel" (ie hardware description), and would require QEMU/kvmtool to parse the user's commandline tool to figure out if they needed to override it or edit it (or alternatively, would require the user to know internal details of QEMU/kvmtool's address map for the guest). I think it would be nicer to keep them separate. thanks -- PMM
On Tue, May 07, 2013 at 04:58:23PM +0100, Peter Maydell wrote: > On 7 May 2013 16:52, Christopher Covington <cov@codeaurora.org> wrote: > > On 05/07/2013 08:19 AM, Peter Maydell wrote: > >> Well, at the moment EARLY_PRINTK is hardcoded to > >> "use some specific UART or equivalent selected at > >> compile time". So the equivalent presumably would > >> be to hard-compile "use virtio-console", but then > >> how does that code know where the virtio-console > >> is in the address space? > > > > arm64 uses a kernel argument. > > This mixes up "information that the user provides to the > kernel" (ie configuration) with "information that QEMU or > kvmtool should provide to the kernel" (ie hardware > description), and would require QEMU/kvmtool to parse > the user's commandline tool to figure out if they > needed to override it or edit it (or alternatively, > would require the user to know internal details of > QEMU/kvmtool's address map for the guest). I think it > would be nicer to keep them separate. I'm not sure it's worth it, the earlyprintk is meant for debugging the early kernel booting code, not for general use (you have the normal console for this). Someone doing platform port should know the address of the uart and pass the right information on the kernel command line to help debug early booting issues. But if someone comes up with some bindings, I won't reject the patch.
On Wed, May 8, 2013 at 3:17 PM, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Tue, May 07, 2013 at 04:58:23PM +0100, Peter Maydell wrote: >> On 7 May 2013 16:52, Christopher Covington <cov@codeaurora.org> wrote: >> This mixes up "information that the user provides to the >> kernel" (ie configuration) with "information that QEMU or >> kvmtool should provide to the kernel" (ie hardware >> description), and would require QEMU/kvmtool to parse >> the user's commandline tool to figure out if they >> needed to override it or edit it (or alternatively, >> would require the user to know internal details of >> QEMU/kvmtool's address map for the guest). I think it >> would be nicer to keep them separate. > > I'm not sure it's worth it, the earlyprintk is meant for debugging the > early kernel booting code, not for general use (you have the normal > console for this). Someone doing platform port should know the address > of the uart and pass the right information on the kernel command line to > help debug early booting issues. > > But if someone comes up with some bindings, I won't reject the patch. Nicolas and I have tossed back and forth the idea of having a trivial binding in the DT which specifies a couple of physical addresses and a mask; one write register, one status register, and the mask to say which bit to care about in the status. Basically the bare minimum to say "here is something that will output to an already set up serial device" without any reference to what it actually is. Something like that would theoretically work with any device that implements or emulates a UART write register. The thinking is that if it is trivial to parse then it becomes more useful as an early printk target and usable by the zImage wrapper (arm32 bit of course). Neither of us ever followed up on it though. g.
diff --git a/include/uapi/linux/virtio_console.h b/include/uapi/linux/virtio_console.h index ee13ab6..586678d 100644 --- a/include/uapi/linux/virtio_console.h +++ b/include/uapi/linux/virtio_console.h @@ -38,6 +38,7 @@ /* Feature bits */ #define VIRTIO_CONSOLE_F_SIZE 0 /* Does host provide console size? */ #define VIRTIO_CONSOLE_F_MULTIPORT 1 /* Does host provide multiple ports? */ +#define VIRTIO_CONSOLE_F_EMERG_WRITE 2 /* Does host support emergency write? */ #define VIRTIO_CONSOLE_BAD_ID (~(u32)0) @@ -48,6 +49,8 @@ struct virtio_console_config { __u16 rows; /* max. number of ports this device can hold */ __u32 max_nr_ports; + /* emergency write register */ + __u32 emerg_wr; } __attribute__((packed)); /*