| Message ID | 1366264344-28025-1-git-send-email-pranavkumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Pranavkumar, On Thu, 18 Apr 2013 11:22:24 +0530, PranavkumarSawargaonkar <pranavkumar@linaro.org> wrote: > From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> > > This patch implements early printk support for virtio-mmio console devices > without using any hypercalls. > > The current virtio early printk code in kernel expects that hypervisor > will provide some mechanism generally a hypercall to support early printk. > This patch does not break existing hypercall based early print support. > > This implementation adds: > 1. Early read-write register named early_rw in virtio console's config > space. > 2. Two host feature flags namely VIRTIO_CONSOLE_F_EARLY_READ and > VIRTIO_CONSOLE_F_EARLY_WRITE for telling guest about early-read and > early-write capability in console device. > > Early write mechanism: > 1. When a guest wants to out some character, it has to simply write the > character to early_rw register in config space of virtio console device. > > Early read mechanism: > 1. When a guest wants to in some character, it has to simply read the > early_rw register in config space of virtio console device. Lets say we get > 32-bit value X. > 2. If most significant bit of X is set (i.e. X & 0x80000000 == 0x80000000) > then least significant 8 bits of X represents input charaacter else guest > need to try again reading early_rw register. > > Note: This patch only includes kernel side changes for early printk, the > host/hypervisor side emulation of early_rw register is out of scope here. Well, that's unfortunate, as it makes it quite difficult to understand the impact of this patch. Has the virtio side been posted somewhere? I expect you've implemented something in kvmtool... > Signed-off-by: Anup Patel <anup.patel@linaro.org> > --- > arch/arm64/kernel/early_printk.c | 24 ++++++++++++++++++++++++ > include/uapi/linux/virtio_console.h | 4 ++++ > 2 files changed, 28 insertions(+) > > diff --git a/arch/arm64/kernel/early_printk.c > b/arch/arm64/kernel/early_printk.c > index ac974f4..a82b5aa 100644 > --- a/arch/arm64/kernel/early_printk.c > +++ b/arch/arm64/kernel/early_printk.c > @@ -25,6 +25,9 @@ > > #include <linux/amba/serial.h> > #include <linux/serial_reg.h> > +#include <linux/virtio_ids.h> > +#include <linux/virtio_mmio.h> > +#include <linux/virtio_console.h> > > static void __iomem *early_base; > static void (*printch)(char ch); > @@ -53,6 +56,26 @@ static void smh_printch(char ch) > } > > /* > + * VIRTIO MMIO based debug console. > + */ > +static void virtio_console_early_printch(char ch) > +{ > + u32 tmp; > + struct virtio_console_config *p = early_base + VIRTIO_MMIO_CONFIG; > + > + tmp = readl_relaxed(early_base + VIRTIO_MMIO_DEVICE_ID); > + if (tmp != VIRTIO_ID_CONSOLE) { > + return; > + } > + > + tmp = readl_relaxed(early_base + VIRTIO_MMIO_HOST_FEATURES); > + if (!(tmp & (1 << VIRTIO_CONSOLE_F_EARLY_WRITE))) { > + return; > + } > + writeb_relaxed(ch, &p->early_rw); So here, you end up trapping 3 times per character being output on the console. Surely there's a better way. How about remembering the result of these tests in a static variable? > +} > + > +/* > * 8250/16550 (8-bit aligned registers) single character TX. > */ > static void uart8250_8bit_printch(char ch) > @@ -82,6 +105,7 @@ static const struct earlycon_match earlycon_match[] > __initconst = { > { .name = "smh", .printch = smh_printch, }, > { .name = "uart8250-8bit", .printch = uart8250_8bit_printch, }, > { .name = "uart8250-32bit", .printch = uart8250_32bit_printch, }, > + { .name = "virtio-console", .printch = virtio_console_early_printch, }, > {} > }; > > diff --git a/include/uapi/linux/virtio_console.h > b/include/uapi/linux/virtio_console.h > index ee13ab6..1171cb4 100644 > --- a/include/uapi/linux/virtio_console.h > +++ b/include/uapi/linux/virtio_console.h > @@ -38,6 +38,8 @@ > /* 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_EARLY_READ 2 /* Does host support early read? */ > +#define VIRTIO_CONSOLE_F_EARLY_WRITE 3 /* Does host support early write? > */ > > #define VIRTIO_CONSOLE_BAD_ID (~(u32)0) > > @@ -48,6 +50,8 @@ struct virtio_console_config { > __u16 rows; > /* max. number of ports this device can hold */ > __u32 max_nr_ports; > + /* early read/write register */ > + __u32 early_rw; > } __attribute__((packed)); > > /* So that bit is clearly a spec change. How does it work with PCI, or any other virtio transport? Overall, I'm a bit concerned with adding features that don't really match the way virtio is supposed to work. The whole goal of virtio is to minimize the amount of trapping, and here you end up trapping on each and every access. If you need an early console, why not simply wire the 8250 emulation in kvmtool to be useable from the MMIO bus? I reckon this would solve your problem in a more elegant way... Cheers, M.
PranavkumarSawargaonkar <pranavkumar@linaro.org> writes: > From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> > > This patch implements early printk support for virtio-mmio console devices without using any hypercalls. This makes some sense, though not sure that early console *read* makes much sense. I can see the PCI version of this being useful as well. The spec definition for this will be interesting, because it can be used before the device is initialized (something which is generally verboten). Cheers, Rusty.
Hi Marc, On 18 April 2013 12:19, Marc Zyngier <maz@misterjones.org> wrote: > Hi Pranavkumar, > > On Thu, 18 Apr 2013 11:22:24 +0530, PranavkumarSawargaonkar > <pranavkumar@linaro.org> wrote: >> From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >> >> This patch implements early printk support for virtio-mmio console > devices >> without using any hypercalls. >> >> The current virtio early printk code in kernel expects that hypervisor >> will provide some mechanism generally a hypercall to support early > printk. >> This patch does not break existing hypercall based early print support. >> >> This implementation adds: >> 1. Early read-write register named early_rw in virtio console's config >> space. >> 2. Two host feature flags namely VIRTIO_CONSOLE_F_EARLY_READ and >> VIRTIO_CONSOLE_F_EARLY_WRITE for telling guest about early-read and >> early-write capability in console device. >> >> Early write mechanism: >> 1. When a guest wants to out some character, it has to simply write the >> character to early_rw register in config space of virtio console device. >> >> Early read mechanism: >> 1. When a guest wants to in some character, it has to simply read the >> early_rw register in config space of virtio console device. Lets say we > get >> 32-bit value X. >> 2. If most significant bit of X is set (i.e. X & 0x80000000 == > 0x80000000) >> then least significant 8 bits of X represents input charaacter else > guest >> need to try again reading early_rw register. >> >> Note: This patch only includes kernel side changes for early printk, the >> host/hypervisor side emulation of early_rw register is out of scope > here. > > Well, that's unfortunate, as it makes it quite difficult to understand the > impact of this patch. > Has the virtio side been posted somewhere? I expect you've implemented > something in kvmtool... Yes i have implemented kvmtool side also and code change is really small (not really a clean code currently) I can post it also but since it is specific to kvmtool i have not posted it with rfc. > >> Signed-off-by: Anup Patel <anup.patel@linaro.org> >> --- >> arch/arm64/kernel/early_printk.c | 24 ++++++++++++++++++++++++ >> include/uapi/linux/virtio_console.h | 4 ++++ >> 2 files changed, 28 insertions(+) >> >> diff --git a/arch/arm64/kernel/early_printk.c >> b/arch/arm64/kernel/early_printk.c >> index ac974f4..a82b5aa 100644 >> --- a/arch/arm64/kernel/early_printk.c >> +++ b/arch/arm64/kernel/early_printk.c >> @@ -25,6 +25,9 @@ >> >> #include <linux/amba/serial.h> >> #include <linux/serial_reg.h> >> +#include <linux/virtio_ids.h> >> +#include <linux/virtio_mmio.h> >> +#include <linux/virtio_console.h> >> >> static void __iomem *early_base; >> static void (*printch)(char ch); >> @@ -53,6 +56,26 @@ static void smh_printch(char ch) >> } >> >> /* >> + * VIRTIO MMIO based debug console. >> + */ >> +static void virtio_console_early_printch(char ch) >> +{ >> + u32 tmp; >> + struct virtio_console_config *p = early_base + VIRTIO_MMIO_CONFIG; >> + >> + tmp = readl_relaxed(early_base + VIRTIO_MMIO_DEVICE_ID); >> + if (tmp != VIRTIO_ID_CONSOLE) { >> + return; >> + } >> + >> + tmp = readl_relaxed(early_base + VIRTIO_MMIO_HOST_FEATURES); >> + if (!(tmp & (1 << VIRTIO_CONSOLE_F_EARLY_WRITE))) { >> + return; >> + } >> + writeb_relaxed(ch, &p->early_rw); > > So here, you end up trapping 3 times per character being output on the > console. Surely there's a better way. How about remembering the result of > these tests in a static variable? Yeah surely it is a better idea to remember using static variable, so that after initialize once, it will trap only one time. > >> +} >> + >> +/* >> * 8250/16550 (8-bit aligned registers) single character TX. >> */ >> static void uart8250_8bit_printch(char ch) >> @@ -82,6 +105,7 @@ static const struct earlycon_match earlycon_match[] >> __initconst = { >> { .name = "smh", .printch = smh_printch, }, >> { .name = "uart8250-8bit", .printch = uart8250_8bit_printch, }, >> { .name = "uart8250-32bit", .printch = uart8250_32bit_printch, }, >> + { .name = "virtio-console", .printch = virtio_console_early_printch, > }, >> {} >> }; >> >> diff --git a/include/uapi/linux/virtio_console.h >> b/include/uapi/linux/virtio_console.h >> index ee13ab6..1171cb4 100644 >> --- a/include/uapi/linux/virtio_console.h >> +++ b/include/uapi/linux/virtio_console.h >> @@ -38,6 +38,8 @@ >> /* 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_EARLY_READ 2 /* Does host support early read? > */ >> +#define VIRTIO_CONSOLE_F_EARLY_WRITE 3 /* Does host support early > write? >> */ >> >> #define VIRTIO_CONSOLE_BAD_ID (~(u32)0) >> >> @@ -48,6 +50,8 @@ struct virtio_console_config { >> __u16 rows; >> /* max. number of ports this device can hold */ >> __u32 max_nr_ports; >> + /* early read/write register */ >> + __u32 early_rw; >> } __attribute__((packed)); >> >> /* > > So that bit is clearly a spec change. How does it work with PCI, or any > other virtio transport? > > Overall, I'm a bit concerned with adding features that don't really match > the way virtio is supposed to work. The whole goal of virtio is to minimize > the amount of trapping, and here you end up trapping on each and every > access. > > If you need an early console, why not simply wire the 8250 emulation in > kvmtool to be useable from the MMIO bus? I reckon this would solve your > problem in a more elegant way... > Emulation will solve the issue but having a virtio early read or write has one more advantage i.e. In case of mach-virt we might need early read-write support for virtio console to see if kernel is not panic before actual virtio drivers takes control. Also if someone wants to have UEFI or uboot running on mach-virt then we also need early input facility in virtio console. > Cheers, > > M. > -- > Who you jivin' with that Cosmik Debris? Thanks, Pranav
On 18 April 2013 12:21, Rusty Russell <rusty@rustcorp.com.au> wrote: > > PranavkumarSawargaonkar <pranavkumar@linaro.org> writes: > > From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> > > > > This patch implements early printk support for virtio-mmio console devices without using any hypercalls. > > This makes some sense, though not sure that early console *read* makes > much sense. I can see the PCI version of this being useful as well. Read can be useful for "mach-virt" which will have only virtio console as a console device. Then if someone wants to have UEFI or any other boot-loader emulation, which expects user to input few things, in that case read might become handy. > > > The spec definition for this will be interesting, because it can be used > before the device is initialized (something which is generally > verboten). > > Cheers, > Rusty. Thanks, Pranav
On Thu, 18 Apr 2013 12:47:18 +0530, Pranavkumar Sawargaonkar <pranavkumar@linaro.org> wrote: > Hi Marc, > > On 18 April 2013 12:19, Marc Zyngier <maz@misterjones.org> wrote: > >> Hi Pranavkumar, >> >> On Thu, 18 Apr 2013 11:22:24 +0530, PranavkumarSawargaonkar >> <pranavkumar@linaro.org> wrote: >> > From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >> > >> > This patch implements early printk support for virtio-mmio console >> devices >> > without using any hypercalls. >> > >> > The current virtio early printk code in kernel expects that hypervisor >> > will provide some mechanism generally a hypercall to support early >> printk. >> > This patch does not break existing hypercall based early print support. >> > >> > This implementation adds: >> > 1. Early read-write register named early_rw in virtio console's config >> > space. >> > 2. Two host feature flags namely VIRTIO_CONSOLE_F_EARLY_READ and >> > VIRTIO_CONSOLE_F_EARLY_WRITE for telling guest about early-read and >> > early-write capability in console device. >> > >> > Early write mechanism: >> > 1. When a guest wants to out some character, it has to simply write the >> > character to early_rw register in config space of virtio console >> > device. >> > >> > Early read mechanism: >> > 1. When a guest wants to in some character, it has to simply read the >> > early_rw register in config space of virtio console device. Lets say we >> get >> > 32-bit value X. >> > 2. If most significant bit of X is set (i.e. X & 0x80000000 == >> 0x80000000) >> > then least significant 8 bits of X represents input charaacter else >> guest >> > need to try again reading early_rw register. >> > >> > Note: This patch only includes kernel side changes for early printk, >> > the >> > host/hypervisor side emulation of early_rw register is out of scope >> here. >> >> Well, that's unfortunate, as it makes it quite difficult to understand >> the >> impact of this patch. >> Has the virtio side been posted somewhere? I expect you've implemented >> something in kvmtool... >> > > Yes i have implemented kvmtool side also and code change is really small > (not really a clean code currently) > I can post it also but since it is specific to kvmtool i have not posted it > with rfc. Doesn't really if the code needs some rework at this point (I expect the patch to be fairly small indeed). Any chance you could post it to the KVM list? >> >> > Signed-off-by: Anup Patel <anup.patel@linaro.org> >> > --- >> > arch/arm64/kernel/early_printk.c | 24 ++++++++++++++++++++++++ >> > include/uapi/linux/virtio_console.h | 4 ++++ >> > 2 files changed, 28 insertions(+) >> > >> > diff --git a/arch/arm64/kernel/early_printk.c >> > b/arch/arm64/kernel/early_printk.c >> > index ac974f4..a82b5aa 100644 >> > --- a/arch/arm64/kernel/early_printk.c >> > +++ b/arch/arm64/kernel/early_printk.c >> > @@ -25,6 +25,9 @@ >> > >> > #include <linux/amba/serial.h> >> > #include <linux/serial_reg.h> >> > +#include <linux/virtio_ids.h> >> > +#include <linux/virtio_mmio.h> >> > +#include <linux/virtio_console.h> >> > >> > static void __iomem *early_base; >> > static void (*printch)(char ch); >> > @@ -53,6 +56,26 @@ static void smh_printch(char ch) >> > } >> > >> > /* >> > + * VIRTIO MMIO based debug console. >> > + */ >> > +static void virtio_console_early_printch(char ch) >> > +{ >> > + u32 tmp; >> > + struct virtio_console_config *p = early_base + >> > VIRTIO_MMIO_CONFIG; >> > + >> > + tmp = readl_relaxed(early_base + VIRTIO_MMIO_DEVICE_ID); >> > + if (tmp != VIRTIO_ID_CONSOLE) { >> > + return; >> > + } >> > + >> > + tmp = readl_relaxed(early_base + VIRTIO_MMIO_HOST_FEATURES); >> > + if (!(tmp & (1 << VIRTIO_CONSOLE_F_EARLY_WRITE))) { >> > + return; >> > + } >> > + writeb_relaxed(ch, &p->early_rw); >> >> So here, you end up trapping 3 times per character being output on the >> console. Surely there's a better way. How about remembering the result of >> these tests in a static variable? >> > Yeah surely it is a better idea to remember using static variable, so that > after initialize once, it will trap only one time. Also, would it be possible to directly get the base address from DT? It would save having to pass the address (which is not known before runtime in the case of kvmtool). Not sure if it is available that early though... >> >> > +} >> > + >> > +/* >> > * 8250/16550 (8-bit aligned registers) single character TX. >> > */ >> > static void uart8250_8bit_printch(char ch) >> > @@ -82,6 +105,7 @@ static const struct earlycon_match earlycon_match[] >> > __initconst = { >> > { .name = "smh", .printch = smh_printch, }, >> > { .name = "uart8250-8bit", .printch = uart8250_8bit_printch, }, >> > { .name = "uart8250-32bit", .printch = uart8250_32bit_printch, }, >> > + { .name = "virtio-console", .printch = >> virtio_console_early_printch, >> }, >> > {} >> > }; >> > >> > diff --git a/include/uapi/linux/virtio_console.h >> > b/include/uapi/linux/virtio_console.h >> > index ee13ab6..1171cb4 100644 >> > --- a/include/uapi/linux/virtio_console.h >> > +++ b/include/uapi/linux/virtio_console.h >> > @@ -38,6 +38,8 @@ >> > /* 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_EARLY_READ 2 /* Does host support >> > early >> read? >> */ >> > +#define VIRTIO_CONSOLE_F_EARLY_WRITE 3 /* Does host support >> > early >> write? >> > */ >> > >> > #define VIRTIO_CONSOLE_BAD_ID (~(u32)0) >> > >> > @@ -48,6 +50,8 @@ struct virtio_console_config { >> > __u16 rows; >> > /* max. number of ports this device can hold */ >> > __u32 max_nr_ports; >> > + /* early read/write register */ >> > + __u32 early_rw; >> > } __attribute__((packed)); >> > >> > /* >> >> So that bit is clearly a spec change. How does it work with PCI, or any >> other virtio transport? >> > I am not sure about PCI hence just posted for MMIO. > >> >> Overall, I'm a bit concerned with adding features that don't really match >> the way virtio is supposed to work. The whole goal of virtio is to >> minimize >> the amount of trapping, and here you end up trapping on each and every >> access. >> >> If you need an early console, why not simply wire the 8250 emulation in >> kvmtool to be useable from the MMIO bus? I reckon this would solve your >> problem in a more elegant way... >> > Emulation will solve the issue but having a virtio early read or write has > one more advantage i.e. > In case of mach-virt we might need early read-write support for virtio > console to see if kernel is not panic before actual virtio drivers takes > control. > Also if someone wants to have UEFI or uboot running on mach-virt then we > also need early input facility in virtio console. That's exactly why I was suggesting using the 8250 emulation. It is supported by everything under the sun. I do not immediately see what the gain is with this virtio approach, as compared to 8250 emulation. Don't misunderstand me, I like the idea of having a virtio-only system, specially if we can make it work with other transports. I just want to outline that there may be a simpler way for your particular use case. Thanks, M.
On 18 April 2013 07:49, Marc Zyngier <maz@misterjones.org> wrote: > If you need an early console, why not simply wire the 8250 emulation in > kvmtool to be useable from the MMIO bus? I reckon this would solve your > problem in a more elegant way... The other approach I thought of would be something involving defining a hypercall interface for console I/O, in the same way that we have hypercalls for "start cpu"/"stop cpu"/etc. Is there any mileage in considering that approach, or is it a non-starter? thanks -- PMM
Am 18.04.2013 um 09:32 schrieb Pranavkumar Sawargaonkar <pranavkumar@linaro.org>: > On 18 April 2013 12:21, Rusty Russell <rusty@rustcorp.com.au> wrote: >> >> PranavkumarSawargaonkar <pranavkumar@linaro.org> writes: >>> From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >>> >>> This patch implements early printk support for virtio-mmio console devices without using any hypercalls. >> >> This makes some sense, though not sure that early console *read* makes >> much sense. I can see the PCI version of this being useful as well. > > Read can be useful for "mach-virt" which will have only virtio console > as a console device. Then if someone wants to have UEFI or any other > boot-loader emulation, which expects user to input few things, in that > case read might become handy. A boot loader should easily be able to implement virtio-console for real. In fact, you should be able to do a simple virtio-console implementation for early printk too, that polls the host for acks rather than use interrupts. Check out my s390-zipl code for reference. I use that there. The advantage to that would be that no host changes are required whatsoever and the interface strictly stays as it is. Alex
Hi Marc, On 18 April 2013 13:06, Marc Zyngier <marc.zyngier@arm.com> wrote: > On Thu, 18 Apr 2013 12:47:18 +0530, Pranavkumar Sawargaonkar > <pranavkumar@linaro.org> wrote: >> Hi Marc, >> >> On 18 April 2013 12:19, Marc Zyngier <maz@misterjones.org> wrote: >> >>> Hi Pranavkumar, >>> >>> On Thu, 18 Apr 2013 11:22:24 +0530, PranavkumarSawargaonkar >>> <pranavkumar@linaro.org> wrote: >>> > From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >>> > >>> > This patch implements early printk support for virtio-mmio console >>> devices >>> > without using any hypercalls. >>> > >>> > The current virtio early printk code in kernel expects that > hypervisor >>> > will provide some mechanism generally a hypercall to support early >>> printk. >>> > This patch does not break existing hypercall based early print > support. >>> > >>> > This implementation adds: >>> > 1. Early read-write register named early_rw in virtio console's > config >>> > space. >>> > 2. Two host feature flags namely VIRTIO_CONSOLE_F_EARLY_READ and >>> > VIRTIO_CONSOLE_F_EARLY_WRITE for telling guest about early-read and >>> > early-write capability in console device. >>> > >>> > Early write mechanism: >>> > 1. When a guest wants to out some character, it has to simply write > the >>> > character to early_rw register in config space of virtio console >>> > device. >>> > >>> > Early read mechanism: >>> > 1. When a guest wants to in some character, it has to simply read the >>> > early_rw register in config space of virtio console device. Lets say > we >>> get >>> > 32-bit value X. >>> > 2. If most significant bit of X is set (i.e. X & 0x80000000 == >>> 0x80000000) >>> > then least significant 8 bits of X represents input charaacter else >>> guest >>> > need to try again reading early_rw register. >>> > >>> > Note: This patch only includes kernel side changes for early printk, >>> > the >>> > host/hypervisor side emulation of early_rw register is out of scope >>> here. >>> >>> Well, that's unfortunate, as it makes it quite difficult to understand >>> the >>> impact of this patch. >>> Has the virtio side been posted somewhere? I expect you've implemented >>> something in kvmtool... >>> >> >> Yes i have implemented kvmtool side also and code change is really small >> (not really a clean code currently) >> I can post it also but since it is specific to kvmtool i have not posted > it >> with rfc. > > Doesn't really if the code needs some rework at this point (I expect the > patch to be fairly small indeed). Any chance you could post it to the KVM > list? Yeah patch is very small, i will post it on kvm list. I have tested patch on foundation model. > >>> >>> > Signed-off-by: Anup Patel <anup.patel@linaro.org> >>> > --- >>> > arch/arm64/kernel/early_printk.c | 24 ++++++++++++++++++++++++ >>> > include/uapi/linux/virtio_console.h | 4 ++++ >>> > 2 files changed, 28 insertions(+) >>> > >>> > diff --git a/arch/arm64/kernel/early_printk.c >>> > b/arch/arm64/kernel/early_printk.c >>> > index ac974f4..a82b5aa 100644 >>> > --- a/arch/arm64/kernel/early_printk.c >>> > +++ b/arch/arm64/kernel/early_printk.c >>> > @@ -25,6 +25,9 @@ >>> > >>> > #include <linux/amba/serial.h> >>> > #include <linux/serial_reg.h> >>> > +#include <linux/virtio_ids.h> >>> > +#include <linux/virtio_mmio.h> >>> > +#include <linux/virtio_console.h> >>> > >>> > static void __iomem *early_base; >>> > static void (*printch)(char ch); >>> > @@ -53,6 +56,26 @@ static void smh_printch(char ch) >>> > } >>> > >>> > /* >>> > + * VIRTIO MMIO based debug console. >>> > + */ >>> > +static void virtio_console_early_printch(char ch) >>> > +{ >>> > + u32 tmp; >>> > + struct virtio_console_config *p = early_base + >>> > VIRTIO_MMIO_CONFIG; >>> > + >>> > + tmp = readl_relaxed(early_base + VIRTIO_MMIO_DEVICE_ID); >>> > + if (tmp != VIRTIO_ID_CONSOLE) { >>> > + return; >>> > + } >>> > + >>> > + tmp = readl_relaxed(early_base + VIRTIO_MMIO_HOST_FEATURES); >>> > + if (!(tmp & (1 << VIRTIO_CONSOLE_F_EARLY_WRITE))) { >>> > + return; >>> > + } >>> > + writeb_relaxed(ch, &p->early_rw); >>> >>> So here, you end up trapping 3 times per character being output on the >>> console. Surely there's a better way. How about remembering the result > of >>> these tests in a static variable? >>> >> Yeah surely it is a better idea to remember using static variable, so > that >> after initialize once, it will trap only one time. > > Also, would it be possible to directly get the base address from DT? It > would save having to pass the address (which is not known before runtime in > the case of kvmtool). Not sure if it is available that early though... Early printk code initializes earlier (from parse_early_param in arch/arm64/setup.c) than fdt un-flattened call (unflatten_device_tree) . Hence using dts to pass this is not possible for passing the address. > >>> >>> > +} >>> > + >>> > +/* >>> > * 8250/16550 (8-bit aligned registers) single character TX. >>> > */ >>> > static void uart8250_8bit_printch(char ch) >>> > @@ -82,6 +105,7 @@ static const struct earlycon_match > earlycon_match[] >>> > __initconst = { >>> > { .name = "smh", .printch = smh_printch, }, >>> > { .name = "uart8250-8bit", .printch = uart8250_8bit_printch, }, >>> > { .name = "uart8250-32bit", .printch = uart8250_32bit_printch, > }, >>> > + { .name = "virtio-console", .printch = >>> virtio_console_early_printch, >>> }, >>> > {} >>> > }; >>> > >>> > diff --git a/include/uapi/linux/virtio_console.h >>> > b/include/uapi/linux/virtio_console.h >>> > index ee13ab6..1171cb4 100644 >>> > --- a/include/uapi/linux/virtio_console.h >>> > +++ b/include/uapi/linux/virtio_console.h >>> > @@ -38,6 +38,8 @@ >>> > /* 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_EARLY_READ 2 /* Does host support >>> > early >>> read? >>> */ >>> > +#define VIRTIO_CONSOLE_F_EARLY_WRITE 3 /* Does host support >>> > early >>> write? >>> > */ >>> > >>> > #define VIRTIO_CONSOLE_BAD_ID (~(u32)0) >>> > >>> > @@ -48,6 +50,8 @@ struct virtio_console_config { >>> > __u16 rows; >>> > /* max. number of ports this device can hold */ >>> > __u32 max_nr_ports; >>> > + /* early read/write register */ >>> > + __u32 early_rw; >>> > } __attribute__((packed)); >>> > >>> > /* >>> >>> So that bit is clearly a spec change. How does it work with PCI, or any >>> other virtio transport? >>> >> I am not sure about PCI hence just posted for MMIO. >> >>> >>> Overall, I'm a bit concerned with adding features that don't really > match >>> the way virtio is supposed to work. The whole goal of virtio is to >>> minimize >>> the amount of trapping, and here you end up trapping on each and every >>> access. >>> >>> If you need an early console, why not simply wire the 8250 emulation in >>> kvmtool to be useable from the MMIO bus? I reckon this would solve your >>> problem in a more elegant way... >>> >> Emulation will solve the issue but having a virtio early read or write > has >> one more advantage i.e. >> In case of mach-virt we might need early read-write support for virtio >> console to see if kernel is not panic before actual virtio drivers takes >> control. >> Also if someone wants to have UEFI or uboot running on mach-virt then we >> also need early input facility in virtio console. > > That's exactly why I was suggesting using the 8250 emulation. It is > supported by everything under the sun. I do not immediately see what the > gain is with this virtio approach, as compared to 8250 emulation. > > Don't misunderstand me, I like the idea of having a virtio-only system, Definitely not. > specially if we can make it work with other transports. I just want to > outline that there may be a simpler way for your particular use case. Actually i thought adding a config register will be easier to add a code than writing entire emulation as 8250 emulation will require to deal with dealing with more registers and more code. Thanks, Pranav > > Thanks, > > M. > -- > Fast, cheap, reliable. Pick two.
Am 18.04.2013 um 10:30 schrieb Peter Maydell <peter.maydell@linaro.org>: > On 18 April 2013 07:49, Marc Zyngier <maz@misterjones.org> wrote: >> If you need an early console, why not simply wire the 8250 emulation in >> kvmtool to be useable from the MMIO bus? I reckon this would solve your >> problem in a more elegant way... > > The other approach I thought of would be something involving > defining a hypercall interface for console I/O, in the same > way that we have hypercalls for "start cpu"/"stop cpu"/etc. > Is there any mileage in considering that approach, or is it > a non-starter? It's exactly what we had for the s390-virtio machine. Virtio-console as console device plus a hypercall for early printk. It was a mess. Trying to inject character output that lands in machine context, where hypercalls get handled, inside of a specific virtio-console device, which owns the char output, is hard. We haven't found any good solution in qemu to layer this properly. The closest approach to something workable was to create 2 char outputs and mux them together, like you usually would mus monitor and serial. Good luck muxing that one again with the monitor :). I'd rather spare you guys from going through the same pain. Alex
On Thu, 18 Apr 2013 09:30:52 +0100, Peter Maydell <peter.maydell@linaro.org> wrote: > On 18 April 2013 07:49, Marc Zyngier <maz@misterjones.org> wrote: >> If you need an early console, why not simply wire the 8250 emulation in >> kvmtool to be useable from the MMIO bus? I reckon this would solve your >> problem in a more elegant way... > > The other approach I thought of would be something involving > defining a hypercall interface for console I/O, in the same > way that we have hypercalls for "start cpu"/"stop cpu"/etc. > Is there any mileage in considering that approach, or is it > a non-starter? That's always possible, but that becomes architecture dependent, which means code duplication across architectures. I'd rather use the MMIO infrastructure, which has some commonalities across the board. M.
Am 18.04.2013 um 10:48 schrieb Pranavkumar Sawargaonkar <pranavkumar@linaro.org>: > Hi Marc, > > On 18 April 2013 13:06, Marc Zyngier <marc.zyngier@arm.com> wrote: >> On Thu, 18 Apr 2013 12:47:18 +0530, Pranavkumar Sawargaonkar >> <pranavkumar@linaro.org> wrote: >>> Hi Marc, >>> >>> On 18 April 2013 12:19, Marc Zyngier <maz@misterjones.org> wrote: >>> >>>> Hi Pranavkumar, >>>> >>>> On Thu, 18 Apr 2013 11:22:24 +0530, PranavkumarSawargaonkar >>>> <pranavkumar@linaro.org> wrote: >>>>> From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >>>>> >>>>> This patch implements early printk support for virtio-mmio console >>>> devices >>>>> without using any hypercalls. >>>>> >>>>> The current virtio early printk code in kernel expects that >> hypervisor >>>>> will provide some mechanism generally a hypercall to support early >>>> printk. >>>>> This patch does not break existing hypercall based early print >> support. >>>>> >>>>> This implementation adds: >>>>> 1. Early read-write register named early_rw in virtio console's >> config >>>>> space. >>>>> 2. Two host feature flags namely VIRTIO_CONSOLE_F_EARLY_READ and >>>>> VIRTIO_CONSOLE_F_EARLY_WRITE for telling guest about early-read and >>>>> early-write capability in console device. >>>>> >>>>> Early write mechanism: >>>>> 1. When a guest wants to out some character, it has to simply write >> the >>>>> character to early_rw register in config space of virtio console >>>>> device. >>>>> >>>>> Early read mechanism: >>>>> 1. When a guest wants to in some character, it has to simply read the >>>>> early_rw register in config space of virtio console device. Lets say >> we >>>> get >>>>> 32-bit value X. >>>>> 2. If most significant bit of X is set (i.e. X & 0x80000000 == >>>> 0x80000000) >>>>> then least significant 8 bits of X represents input charaacter else >>>> guest >>>>> need to try again reading early_rw register. >>>>> >>>>> Note: This patch only includes kernel side changes for early printk, >>>>> the >>>>> host/hypervisor side emulation of early_rw register is out of scope >>>> here. >>>> >>>> Well, that's unfortunate, as it makes it quite difficult to understand >>>> the >>>> impact of this patch. >>>> Has the virtio side been posted somewhere? I expect you've implemented >>>> something in kvmtool... >>> >>> Yes i have implemented kvmtool side also and code change is really small >>> (not really a clean code currently) >>> I can post it also but since it is specific to kvmtool i have not posted >> it >>> with rfc. >> >> Doesn't really if the code needs some rework at this point (I expect the >> patch to be fairly small indeed). Any chance you could post it to the KVM >> list? > Yeah patch is very small, i will post it on kvm list. I have tested > patch on foundation model. >> >>>> >>>>> Signed-off-by: Anup Patel <anup.patel@linaro.org> >>>>> --- >>>>> arch/arm64/kernel/early_printk.c | 24 ++++++++++++++++++++++++ >>>>> include/uapi/linux/virtio_console.h | 4 ++++ >>>>> 2 files changed, 28 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/kernel/early_printk.c >>>>> b/arch/arm64/kernel/early_printk.c >>>>> index ac974f4..a82b5aa 100644 >>>>> --- a/arch/arm64/kernel/early_printk.c >>>>> +++ b/arch/arm64/kernel/early_printk.c >>>>> @@ -25,6 +25,9 @@ >>>>> >>>>> #include <linux/amba/serial.h> >>>>> #include <linux/serial_reg.h> >>>>> +#include <linux/virtio_ids.h> >>>>> +#include <linux/virtio_mmio.h> >>>>> +#include <linux/virtio_console.h> >>>>> >>>>> static void __iomem *early_base; >>>>> static void (*printch)(char ch); >>>>> @@ -53,6 +56,26 @@ static void smh_printch(char ch) >>>>> } >>>>> >>>>> /* >>>>> + * VIRTIO MMIO based debug console. >>>>> + */ >>>>> +static void virtio_console_early_printch(char ch) >>>>> +{ >>>>> + u32 tmp; >>>>> + struct virtio_console_config *p = early_base + >>>>> VIRTIO_MMIO_CONFIG; >>>>> + >>>>> + tmp = readl_relaxed(early_base + VIRTIO_MMIO_DEVICE_ID); >>>>> + if (tmp != VIRTIO_ID_CONSOLE) { >>>>> + return; >>>>> + } >>>>> + >>>>> + tmp = readl_relaxed(early_base + VIRTIO_MMIO_HOST_FEATURES); >>>>> + if (!(tmp & (1 << VIRTIO_CONSOLE_F_EARLY_WRITE))) { >>>>> + return; >>>>> + } >>>>> + writeb_relaxed(ch, &p->early_rw); >>>> >>>> So here, you end up trapping 3 times per character being output on the >>>> console. Surely there's a better way. How about remembering the result >> of >>>> these tests in a static variable? >>> Yeah surely it is a better idea to remember using static variable, so >> that >>> after initialize once, it will trap only one time. >> >> Also, would it be possible to directly get the base address from DT? It >> would save having to pass the address (which is not known before runtime in >> the case of kvmtool). Not sure if it is available that early though... > > Early printk code initializes earlier (from parse_early_param in > arch/arm64/setup.c) than fdt un-flattened call (unflatten_device_tree) > . Hence using dts to pass this is not possible for passing the > address. Then don't print anything until the fdt is unflattened? Alex > >> >>>> >>>>> +} >>>>> + >>>>> +/* >>>>> * 8250/16550 (8-bit aligned registers) single character TX. >>>>> */ >>>>> static void uart8250_8bit_printch(char ch) >>>>> @@ -82,6 +105,7 @@ static const struct earlycon_match >> earlycon_match[] >>>>> __initconst = { >>>>> { .name = "smh", .printch = smh_printch, }, >>>>> { .name = "uart8250-8bit", .printch = uart8250_8bit_printch, }, >>>>> { .name = "uart8250-32bit", .printch = uart8250_32bit_printch, >> }, >>>>> + { .name = "virtio-console", .printch = >>>> virtio_console_early_printch, >>>> }, >>>>> {} >>>>> }; >>>>> >>>>> diff --git a/include/uapi/linux/virtio_console.h >>>>> b/include/uapi/linux/virtio_console.h >>>>> index ee13ab6..1171cb4 100644 >>>>> --- a/include/uapi/linux/virtio_console.h >>>>> +++ b/include/uapi/linux/virtio_console.h >>>>> @@ -38,6 +38,8 @@ >>>>> /* 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_EARLY_READ 2 /* Does host support >>>>> early >>>> read? >>>> */ >>>>> +#define VIRTIO_CONSOLE_F_EARLY_WRITE 3 /* Does host support >>>>> early >>>> write? >>>>> */ >>>>> >>>>> #define VIRTIO_CONSOLE_BAD_ID (~(u32)0) >>>>> >>>>> @@ -48,6 +50,8 @@ struct virtio_console_config { >>>>> __u16 rows; >>>>> /* max. number of ports this device can hold */ >>>>> __u32 max_nr_ports; >>>>> + /* early read/write register */ >>>>> + __u32 early_rw; >>>>> } __attribute__((packed)); >>>>> >>>>> /* >>>> >>>> So that bit is clearly a spec change. How does it work with PCI, or any >>>> other virtio transport? >>> I am not sure about PCI hence just posted for MMIO. >>> >>>> >>>> Overall, I'm a bit concerned with adding features that don't really >> match >>>> the way virtio is supposed to work. The whole goal of virtio is to >>>> minimize >>>> the amount of trapping, and here you end up trapping on each and every >>>> access. >>>> >>>> If you need an early console, why not simply wire the 8250 emulation in >>>> kvmtool to be useable from the MMIO bus? I reckon this would solve your >>>> problem in a more elegant way... >>> Emulation will solve the issue but having a virtio early read or write >> has >>> one more advantage i.e. >>> In case of mach-virt we might need early read-write support for virtio >>> console to see if kernel is not panic before actual virtio drivers takes >>> control. >>> Also if someone wants to have UEFI or uboot running on mach-virt then we >>> also need early input facility in virtio console. >> >> That's exactly why I was suggesting using the 8250 emulation. It is >> supported by everything under the sun. I do not immediately see what the >> gain is with this virtio approach, as compared to 8250 emulation. >> >> Don't misunderstand me, I like the idea of having a virtio-only system, > Definitely not. >> specially if we can make it work with other transports. I just want to >> outline that there may be a simpler way for your particular use case. > > Actually i thought adding a config register will be easier to add a > code than writing entire emulation as 8250 emulation will require to > deal with dealing with more registers and more code. > > Thanks, > Pranav > >> >> Thanks, >> >> M. >> -- >> Fast, cheap, reliable. Pick two. > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
Hi, On 18 April 2013 14:14, Alexander Graf <agraf@suse.de> wrote: > > > Am 18.04.2013 um 09:32 schrieb Pranavkumar Sawargaonkar <pranavkumar@linaro.org>: > >> On 18 April 2013 12:21, Rusty Russell <rusty@rustcorp.com.au> wrote: >>> >>> PranavkumarSawargaonkar <pranavkumar@linaro.org> writes: >>>> From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >>>> >>>> This patch implements early printk support for virtio-mmio console devices without using any hypercalls. >>> >>> This makes some sense, though not sure that early console *read* makes >>> much sense. I can see the PCI version of this being useful as well. >> >> Read can be useful for "mach-virt" which will have only virtio console >> as a console device. Then if someone wants to have UEFI or any other >> boot-loader emulation, which expects user to input few things, in that >> case read might become handy. > > A boot loader should easily be able to implement virtio-console for real. > > In fact, you should be able to do a simple virtio-console implementation for early printk too, that polls the host for acks rather than use interrupts. Check out my s390-zipl code for reference. I use that there. At the time of early printk we do not have virtio queues initialized so even if we use poll for ack than interrupt, without queues being set how that is possible ? Hence we have added simple config register which can be polled without setting up queues. > > The advantage to that would be that no host changes are required whatsoever and the interface strictly stays as it is. > > > Alex > Thanks, Pranav
On 13:02 Thu 18 Apr , Pranavkumar Sawargaonkar wrote: > On 18 April 2013 12:21, Rusty Russell <rusty@rustcorp.com.au> wrote: > > > > PranavkumarSawargaonkar <pranavkumar@linaro.org> writes: > > > From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> > > > > > > This patch implements early printk support for virtio-mmio console devices without using any hypercalls. > > > > This makes some sense, though not sure that early console *read* makes > > much sense. I can see the PCI version of this being useful as well. > > Read can be useful for "mach-virt" which will have only virtio console > as a console device. Then if someone wants to have UEFI or any other > boot-loader emulation, which expects user to input few things, in that > case read might become handy. yes I'm adding barebox on arm64 and will need it for virt target Best Regards, J. > > > > > > The spec definition for this will be interesting, because it can be used > > before the device is initialized (something which is generally > > verboten). > > > > Cheers, > > Rusty. > > Thanks, > Pranav > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Pranavkumar, On 04/18/2013 01:52 AM, PranavkumarSawargaonkar wrote: > From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> > > This patch implements early printk support for virtio-mmio console devices > without using any hypercalls. Is it possible that using DCC might be an easier solution? [...] Regards, Christopher
On Thu, 18 Apr 2013 11:25:56 -0400, Christopher Covington <cov@codeaurora.org> wrote: > Hi Pranavkumar, > > On 04/18/2013 01:52 AM, PranavkumarSawargaonkar wrote: >> From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >> >> This patch implements early printk support for virtio-mmio console >> devices >> without using any hypercalls. > > Is it possible that using DCC might be an easier solution? You would end up with the exact same problem as with a hypercall based solution: once you're in the hypervisor, what do you do with the data? You end up having to invent another channel to forward it down to your platform emulation in order to have it printed where the user expects it. Using a MMIO based solution is probably the best solution, as it uses the existing infrastructure. M.
On 18 April 2013 21:29, Marc Zyngier <marc.zyngier@arm.com> wrote: > On Thu, 18 Apr 2013 11:25:56 -0400, Christopher Covington > <cov@codeaurora.org> wrote: >> Hi Pranavkumar, >> >> On 04/18/2013 01:52 AM, PranavkumarSawargaonkar wrote: >>> From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >>> >>> This patch implements early printk support for virtio-mmio console >>> devices >>> without using any hypercalls. >> >> Is it possible that using DCC might be an easier solution? > > You would end up with the exact same problem as with a hypercall based > solution: once you're in the hypervisor, what do you do with the data? You > end up having to invent another channel to forward it down to your platform > emulation in order to have it printed where the user expects it. > > Using a MMIO based solution is probably the best solution, as it uses the > existing infrastructure. Completely agree with Marc that instead of writing hypercalls (which will have hypervisor and arch specific implementation) use of existing MMIO and virtio console layer is easier and good option. - Pranav > > M. > -- > Fast, cheap, reliable. Pick two.
On 18 April 2013 13:06, Marc Zyngier <marc.zyngier@arm.com> wrote: > On Thu, 18 Apr 2013 12:47:18 +0530, Pranavkumar Sawargaonkar > <pranavkumar@linaro.org> wrote: >> Hi Marc, >> >> On 18 April 2013 12:19, Marc Zyngier <maz@misterjones.org> wrote: >> >>> Hi Pranavkumar, >>> >>> On Thu, 18 Apr 2013 11:22:24 +0530, PranavkumarSawargaonkar >>> <pranavkumar@linaro.org> wrote: >>> > From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >>> > >>> > This patch implements early printk support for virtio-mmio console >>> devices >>> > without using any hypercalls. >>> > >>> > The current virtio early printk code in kernel expects that > hypervisor >>> > will provide some mechanism generally a hypercall to support early >>> printk. >>> > This patch does not break existing hypercall based early print > support. >>> > >>> > This implementation adds: >>> > 1. Early read-write register named early_rw in virtio console's > config >>> > space. >>> > 2. Two host feature flags namely VIRTIO_CONSOLE_F_EARLY_READ and >>> > VIRTIO_CONSOLE_F_EARLY_WRITE for telling guest about early-read and >>> > early-write capability in console device. >>> > >>> > Early write mechanism: >>> > 1. When a guest wants to out some character, it has to simply write > the >>> > character to early_rw register in config space of virtio console >>> > device. >>> > >>> > Early read mechanism: >>> > 1. When a guest wants to in some character, it has to simply read the >>> > early_rw register in config space of virtio console device. Lets say > we >>> get >>> > 32-bit value X. >>> > 2. If most significant bit of X is set (i.e. X & 0x80000000 == >>> 0x80000000) >>> > then least significant 8 bits of X represents input charaacter else >>> guest >>> > need to try again reading early_rw register. >>> > >>> > Note: This patch only includes kernel side changes for early printk, >>> > the >>> > host/hypervisor side emulation of early_rw register is out of scope >>> here. >>> >>> Well, that's unfortunate, as it makes it quite difficult to understand >>> the >>> impact of this patch. >>> Has the virtio side been posted somewhere? I expect you've implemented >>> something in kvmtool... >>> >> >> Yes i have implemented kvmtool side also and code change is really small >> (not really a clean code currently) >> I can post it also but since it is specific to kvmtool i have not posted > it >> with rfc. > > Doesn't really if the code needs some rework at this point (I expect the > patch to be fairly small indeed). Any chance you could post it to the KVM > list? I have posted kvmtool side changes on kvmarm list. [RFC] KVMTOOL: Early printk support for virtio-mmio console. Patch has an implementation of early_rw config register to print early writes. I have successfully booted a guest on armv8 foundation model throwing early printks before actual virtio driver takes control. Thanks, Pranav > >>> >>> > Signed-off-by: Anup Patel <anup.patel@linaro.org> >>> > --- >>> > arch/arm64/kernel/early_printk.c | 24 ++++++++++++++++++++++++ >>> > include/uapi/linux/virtio_console.h | 4 ++++ >>> > 2 files changed, 28 insertions(+) >>> > >>> > diff --git a/arch/arm64/kernel/early_printk.c >>> > b/arch/arm64/kernel/early_printk.c >>> > index ac974f4..a82b5aa 100644 >>> > --- a/arch/arm64/kernel/early_printk.c >>> > +++ b/arch/arm64/kernel/early_printk.c >>> > @@ -25,6 +25,9 @@ >>> > >>> > #include <linux/amba/serial.h> >>> > #include <linux/serial_reg.h> >>> > +#include <linux/virtio_ids.h> >>> > +#include <linux/virtio_mmio.h> >>> > +#include <linux/virtio_console.h> >>> > >>> > static void __iomem *early_base; >>> > static void (*printch)(char ch); >>> > @@ -53,6 +56,26 @@ static void smh_printch(char ch) >>> > } >>> > >>> > /* >>> > + * VIRTIO MMIO based debug console. >>> > + */ >>> > +static void virtio_console_early_printch(char ch) >>> > +{ >>> > + u32 tmp; >>> > + struct virtio_console_config *p = early_base + >>> > VIRTIO_MMIO_CONFIG; >>> > + >>> > + tmp = readl_relaxed(early_base + VIRTIO_MMIO_DEVICE_ID); >>> > + if (tmp != VIRTIO_ID_CONSOLE) { >>> > + return; >>> > + } >>> > + >>> > + tmp = readl_relaxed(early_base + VIRTIO_MMIO_HOST_FEATURES); >>> > + if (!(tmp & (1 << VIRTIO_CONSOLE_F_EARLY_WRITE))) { >>> > + return; >>> > + } >>> > + writeb_relaxed(ch, &p->early_rw); >>> >>> So here, you end up trapping 3 times per character being output on the >>> console. Surely there's a better way. How about remembering the result > of >>> these tests in a static variable? >>> >> Yeah surely it is a better idea to remember using static variable, so > that >> after initialize once, it will trap only one time. > > Also, would it be possible to directly get the base address from DT? It > would save having to pass the address (which is not known before runtime in > the case of kvmtool). Not sure if it is available that early though... > >>> >>> > +} >>> > + >>> > +/* >>> > * 8250/16550 (8-bit aligned registers) single character TX. >>> > */ >>> > static void uart8250_8bit_printch(char ch) >>> > @@ -82,6 +105,7 @@ static const struct earlycon_match > earlycon_match[] >>> > __initconst = { >>> > { .name = "smh", .printch = smh_printch, }, >>> > { .name = "uart8250-8bit", .printch = uart8250_8bit_printch, }, >>> > { .name = "uart8250-32bit", .printch = uart8250_32bit_printch, > }, >>> > + { .name = "virtio-console", .printch = >>> virtio_console_early_printch, >>> }, >>> > {} >>> > }; >>> > >>> > diff --git a/include/uapi/linux/virtio_console.h >>> > b/include/uapi/linux/virtio_console.h >>> > index ee13ab6..1171cb4 100644 >>> > --- a/include/uapi/linux/virtio_console.h >>> > +++ b/include/uapi/linux/virtio_console.h >>> > @@ -38,6 +38,8 @@ >>> > /* 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_EARLY_READ 2 /* Does host support >>> > early >>> read? >>> */ >>> > +#define VIRTIO_CONSOLE_F_EARLY_WRITE 3 /* Does host support >>> > early >>> write? >>> > */ >>> > >>> > #define VIRTIO_CONSOLE_BAD_ID (~(u32)0) >>> > >>> > @@ -48,6 +50,8 @@ struct virtio_console_config { >>> > __u16 rows; >>> > /* max. number of ports this device can hold */ >>> > __u32 max_nr_ports; >>> > + /* early read/write register */ >>> > + __u32 early_rw; >>> > } __attribute__((packed)); >>> > >>> > /* >>> >>> So that bit is clearly a spec change. How does it work with PCI, or any >>> other virtio transport? >>> >> I am not sure about PCI hence just posted for MMIO. >> >>> >>> Overall, I'm a bit concerned with adding features that don't really > match >>> the way virtio is supposed to work. The whole goal of virtio is to >>> minimize >>> the amount of trapping, and here you end up trapping on each and every >>> access. >>> >>> If you need an early console, why not simply wire the 8250 emulation in >>> kvmtool to be useable from the MMIO bus? I reckon this would solve your >>> problem in a more elegant way... >>> >> Emulation will solve the issue but having a virtio early read or write > has >> one more advantage i.e. >> In case of mach-virt we might need early read-write support for virtio >> console to see if kernel is not panic before actual virtio drivers takes >> control. >> Also if someone wants to have UEFI or uboot running on mach-virt then we >> also need early input facility in virtio console. > > That's exactly why I was suggesting using the 8250 emulation. It is > supported by everything under the sun. I do not immediately see what the > gain is with this virtio approach, as compared to 8250 emulation. > > Don't misunderstand me, I like the idea of having a virtio-only system, > specially if we can make it work with other transports. I just want to > outline that there may be a simpler way for your particular use case. > > Thanks, > > M. > -- > Fast, cheap, reliable. Pick two.
Hello, On Thu, Apr 18, 2013 at 09:48:49AM +0100, Pranavkumar Sawargaonkar wrote: > Actually i thought adding a config register will be easier to add a > code than writing entire emulation as 8250 emulation will require to > deal with dealing with more registers and more code. kvmtool already has an 8250 emulation! All we need to do is hack together something which will allow us to instantiate those ioport devices in a more flexible manner (namely, inside the MMIO space for ARM). For earlyprintk, I suspect we can get away without an interrupt too, which should simplify things a bit to start with. Regardless of the outcome of this discussion, I think getting the 8250 working on ARM is definitely something worth doing. If I get time, I'll take a look. Will
Hi Will, On 19 April 2013 14:35, Will Deacon <will.deacon@arm.com> wrote: > Hello, > > On Thu, Apr 18, 2013 at 09:48:49AM +0100, Pranavkumar Sawargaonkar wrote: >> Actually i thought adding a config register will be easier to add a >> code than writing entire emulation as 8250 emulation will require to >> deal with dealing with more registers and more code. > > kvmtool already has an 8250 emulation! All we need to do is hack together > something which will allow us to instantiate those ioport devices in a more > flexible manner (namely, inside the MMIO space for ARM). For earlyprintk, I > suspect we can get away without an interrupt too, which should simplify > things a bit to start with. > > Regardless of the outcome of this discussion, I think getting the 8250 > working on ARM is definitely something worth doing. If I get time, I'll take > a look. I am not against using 8250 emulation (as far as it solves printk issues for kernel booting logs), but my point is why not to add early read-write support for virtio console, which also can be useful in emulation less mach-virt environment also ? Thanks, Pranav > > Will
On Fri, Apr 19, 2013 at 10:25:35AM +0100, Pranavkumar Sawargaonkar wrote: > On 19 April 2013 14:35, Will Deacon <will.deacon@arm.com> wrote: > > On Thu, Apr 18, 2013 at 09:48:49AM +0100, Pranavkumar Sawargaonkar wrote: > >> Actually i thought adding a config register will be easier to add a > >> code than writing entire emulation as 8250 emulation will require to > >> deal with dealing with more registers and more code. > > > > kvmtool already has an 8250 emulation! All we need to do is hack together > > something which will allow us to instantiate those ioport devices in a more > > flexible manner (namely, inside the MMIO space for ARM). For earlyprintk, I > > suspect we can get away without an interrupt too, which should simplify > > things a bit to start with. > > > > Regardless of the outcome of this discussion, I think getting the 8250 > > working on ARM is definitely something worth doing. If I get time, I'll take > > a look. > > I am not against using 8250 emulation (as far as it solves printk > issues for kernel booting logs), but my point is why not to add early > read-write support for virtio console, which also can be useful in > emulation less mach-virt environment also ? We can have both, but only one of those requires a change to the virtio specification. Will
On 19 April 2013 10:27, Will Deacon <will.deacon@arm.com> wrote: > On Fri, Apr 19, 2013 at 10:25:35AM +0100, Pranavkumar Sawargaonkar wrote: >> I am not against using 8250 emulation (as far as it solves printk >> issues for kernel booting logs), but my point is why not to add early >> read-write support for virtio console, which also can be useful in >> emulation less mach-virt environment also ? > > We can have both, but only one of those requires a change to the virtio > specification. I don't think avoiding writing a spec is necessarily a good reason for insisting on emulation of a lump of hardware 95% of whose capabilities you aren't going to use... -- PMM
On 19 April 2013 15:00, Peter Maydell <peter.maydell@linaro.org> wrote: > On 19 April 2013 10:27, Will Deacon <will.deacon@arm.com> wrote: >> On Fri, Apr 19, 2013 at 10:25:35AM +0100, Pranavkumar Sawargaonkar wrote: >>> I am not against using 8250 emulation (as far as it solves printk >>> issues for kernel booting logs), but my point is why not to add early >>> read-write support for virtio console, which also can be useful in >>> emulation less mach-virt environment also ? >> >> We can have both, but only one of those requires a change to the virtio >> specification. > > I don't think avoiding writing a spec is necessarily a good reason > for insisting on emulation of a lump of hardware 95% of whose > capabilities you aren't going to use... True. Also 8250 will require emulation of registers, and i am not sure about if mach-virt will have any emulation of real hw ? Thanks, Pranav > > -- PMM
On Fri, Apr 19, 2013 at 10:30:40AM +0100, Peter Maydell wrote: > On 19 April 2013 10:27, Will Deacon <will.deacon@arm.com> wrote: > > On Fri, Apr 19, 2013 at 10:25:35AM +0100, Pranavkumar Sawargaonkar wrote: > >> I am not against using 8250 emulation (as far as it solves printk > >> issues for kernel booting logs), but my point is why not to add early > >> read-write support for virtio console, which also can be useful in > >> emulation less mach-virt environment also ? > > > > We can have both, but only one of those requires a change to the virtio > > specification. > > I don't think avoiding writing a spec is necessarily a good reason > for insisting on emulation of a lump of hardware 95% of whose > capabilities you aren't going to use... Don't get me wrong; I'm in favour of having earlyprintk over virtio console too, I'm just pointing out that we can also plug in the 8250 emulation that we already have with very little effort. I'm not insisting on anything. Will
On Fri, Apr 19, 2013 at 10:34:56AM +0100, Pranavkumar Sawargaonkar wrote: > On 19 April 2013 15:00, Peter Maydell <peter.maydell@linaro.org> wrote: > > On 19 April 2013 10:27, Will Deacon <will.deacon@arm.com> wrote: > >> On Fri, Apr 19, 2013 at 10:25:35AM +0100, Pranavkumar Sawargaonkar wrote: > >>> I am not against using 8250 emulation (as far as it solves printk > >>> issues for kernel booting logs), but my point is why not to add early > >>> read-write support for virtio console, which also can be useful in > >>> emulation less mach-virt environment also ? > >> > >> We can have both, but only one of those requires a change to the virtio > >> specification. > > > > I don't think avoiding writing a spec is necessarily a good reason > > for insisting on emulation of a lump of hardware 95% of whose > > capabilities you aren't going to use... > > True. > Also 8250 will require emulation of registers, and i am not sure about > if mach-virt will have any emulation of real hw ? The point of mach-virt is that it is completely parameterised. So, if you're not emulating an 8250, then don't tell the kernel that you have one! Similarly, if you *do* emulate it, then either create a device-tree node for it or pass the appropriate earlyprintk= string on the command line. As far as kvmtool is concerned, we'd probably have a new command-line option for arm64, allowing you to specify the early console device. Will
On 19 April 2013 10:39, Will Deacon <will.deacon@arm.com> wrote: > The point of mach-virt is that it is completely parameterised. So, if you're > not emulating an 8250, then don't tell the kernel that you have one! > Similarly, if you *do* emulate it, then either create a device-tree node for > it or pass the appropriate earlyprintk= string on the command line. > > As far as kvmtool is concerned, we'd probably have a new command-line option > for arm64, allowing you to specify the early console device. Please make the kernel pick the device out of the device tree blob. The whole point of device tree is that it's how to tell the kernel where things live -- making kvmtool/QEMU and/or the user also have to mess with the kernel command line is awkward and annoying. -- PMM
On Fri, Apr 19, 2013 at 11:05:47AM +0100, Peter Maydell wrote: > On 19 April 2013 10:39, Will Deacon <will.deacon@arm.com> wrote: > > The point of mach-virt is that it is completely parameterised. So, if you're > > not emulating an 8250, then don't tell the kernel that you have one! > > Similarly, if you *do* emulate it, then either create a device-tree node for > > it or pass the appropriate earlyprintk= string on the command line. > > > > As far as kvmtool is concerned, we'd probably have a new command-line option > > for arm64, allowing you to specify the early console device. > > Please make the kernel pick the device out of the device tree > blob. The whole point of device tree is that it's how to tell > the kernel where things live -- making kvmtool/QEMU and/or the > user also have to mess with the kernel command line is awkward > and annoying. For a normal console device, it indeed needs to get it from the DT. For early console, you want it earlier than DT parsing so we pass it on the kernel command line via the earlyprintk= parameter. arm64 earlyprintk support for 8250 is already queued for 3.10 (and in -next).
On 19 April 2013 17:12, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Fri, Apr 19, 2013 at 11:05:47AM +0100, Peter Maydell wrote: >> Please make the kernel pick the device out of the device tree >> blob. The whole point of device tree is that it's how to tell >> the kernel where things live -- making kvmtool/QEMU and/or the >> user also have to mess with the kernel command line is awkward >> and annoying. > > For a normal console device, it indeed needs to get it from the DT. For > early console, you want it earlier than DT parsing so we pass it on the > kernel command line via the earlyprintk= parameter. You should fix your DT handling code so you can get at the info when you need it rather than pushing the problem into bootloaders and QEMU, please. DT is your data structure so feel free to (re)design it so relevant information can be accessed early if that's useful. thanks -- PMM
On Fri, Apr 19, 2013 at 05:14:36PM +0100, Peter Maydell wrote: > On 19 April 2013 17:12, Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Fri, Apr 19, 2013 at 11:05:47AM +0100, Peter Maydell wrote: > >> Please make the kernel pick the device out of the device tree > >> blob. The whole point of device tree is that it's how to tell > >> the kernel where things live -- making kvmtool/QEMU and/or the > >> user also have to mess with the kernel command line is awkward > >> and annoying. > > > > For a normal console device, it indeed needs to get it from the DT. For > > early console, you want it earlier than DT parsing so we pass it on the > > kernel command line via the earlyprintk= parameter. > > You should fix your DT handling code so you can get at the info > when you need it rather than pushing the problem into bootloaders > and QEMU, please. DT is your data structure so feel free to > (re)design it so relevant information can be accessed early > if that's useful. earlyprintk is used for debugging early problems, like DT parsing. You don't have to use it unless you are debugging something. Without earlyprintk you just get a normal console during boot, based on the DT description.
On 19 April 2013 17:22, Catalin Marinas <catalin.marinas@arm.com> wrote: > earlyprintk is used for debugging early problems, like DT parsing. You > don't have to use it unless you are debugging something. Without > earlyprintk you just get a normal console during boot, based on the DT > description. The command line lives in the DTB anyway so if you can't look in the DTB you can't get at earlyprintk config either way. -- PMM
On Fri, Apr 19, 2013 at 05:33:18PM +0100, Peter Maydell wrote: > On 19 April 2013 17:22, Catalin Marinas <catalin.marinas@arm.com> wrote: > > earlyprintk is used for debugging early problems, like DT parsing. You > > don't have to use it unless you are debugging something. Without > > earlyprintk you just get a normal console during boot, based on the DT > > description. > > The command line lives in the DTB anyway so if you can't look > in the DTB you can't get at earlyprintk config either way. Linux indeed looks in the DT for the command line and that's what's triggering the earlyprintk console but at that stage the DT is flat. Unflattening the DT happens later (it requires slab allocator). I initially thought about extracting the early console device from the DT but when it is flat you can't parse the full hierarchy to get its address.
On 19 April 2013 18:14, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Fri, Apr 19, 2013 at 05:33:18PM +0100, Peter Maydell wrote: >> The command line lives in the DTB anyway so if you can't look >> in the DTB you can't get at earlyprintk config either way. > > Linux indeed looks in the DT for the command line and that's what's > triggering the earlyprintk console but at that stage the DT is flat. > Unflattening the DT happens later (it requires slab allocator). I > initially thought about extracting the early console device from the DT > but when it is flat you can't parse the full hierarchy to get its > address. So you could add a DT property that specifies the information in a format that you can get at at the right time. That's annoying duplication, but at least it's not in the command line (which is a raw ascii string that QEMU &co shouldn't have to be parsing or editing). "/console/type" and "/console/address" or something? -- PMM
Pranavkumar Sawargaonkar <pranavkumar@linaro.org> writes: > On 18 April 2013 12:21, Rusty Russell <rusty@rustcorp.com.au> wrote: >> >> PranavkumarSawargaonkar <pranavkumar@linaro.org> writes: >> > From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >> > >> > This patch implements early printk support for virtio-mmio console devices without using any hypercalls. >> >> This makes some sense, though not sure that early console *read* makes >> much sense. I can see the PCI version of this being useful as well. > > Read can be useful for "mach-virt" which will have only virtio console > as a console device. Then if someone wants to have UEFI or any other > boot-loader emulation, which expects user to input few things, in that > case read might become handy. But implementing virtio inside a bootloader has already been done for coreboot, for example. A bootloader probably wants a virtio block device, so a console is trivial. A single writable field for debugging makes sense. Anything more is far less certain. Thanks, Rusty.
On 22 April 2013 06:51, Rusty Russell <rusty@rustcorp.com.au> wrote: > > Pranavkumar Sawargaonkar <pranavkumar@linaro.org> writes: > > On 18 April 2013 12:21, Rusty Russell <rusty@rustcorp.com.au> wrote: > >> > >> PranavkumarSawargaonkar <pranavkumar@linaro.org> writes: > >> > From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> > >> > > >> > This patch implements early printk support for virtio-mmio console > >> > devices without using any hypercalls. > >> > >> This makes some sense, though not sure that early console *read* makes > >> much sense. I can see the PCI version of this being useful as well. > > > > Read can be useful for "mach-virt" which will have only virtio console > > as a console device. Then if someone wants to have UEFI or any other > > boot-loader emulation, which expects user to input few things, in that > > case read might become handy. > > But implementing virtio inside a bootloader has already been done for > coreboot, for example. A bootloader probably wants a virtio block > device, so a console is trivial. > > A single writable field for debugging makes sense. Anything more is far > less certain. The early read can be handy for bootloader who don't want to implement complete VirtIO programming. IMHO, early read would be totally optional for host and will not introduce any new config register so it is good to have in VirtIO console spec. Also, without early read the read behavior of early_rw field would be undefined in VirtIO console spec. > > Thanks, > Rusty. Best Regards, Anup
Anup Patel <anup.patel@linaro.org> writes: > On 22 April 2013 06:51, Rusty Russell <rusty@rustcorp.com.au> wrote: >> >> Pranavkumar Sawargaonkar <pranavkumar@linaro.org> writes: >> > On 18 April 2013 12:21, Rusty Russell <rusty@rustcorp.com.au> wrote: >> >> >> >> PranavkumarSawargaonkar <pranavkumar@linaro.org> writes: >> >> > From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >> >> > >> >> > This patch implements early printk support for virtio-mmio console >> >> > devices without using any hypercalls. >> >> >> >> This makes some sense, though not sure that early console *read* makes >> >> much sense. I can see the PCI version of this being useful as well. >> > >> > Read can be useful for "mach-virt" which will have only virtio console >> > as a console device. Then if someone wants to have UEFI or any other >> > boot-loader emulation, which expects user to input few things, in that >> > case read might become handy. >> >> But implementing virtio inside a bootloader has already been done for >> coreboot, for example. A bootloader probably wants a virtio block >> device, so a console is trivial. >> >> A single writable field for debugging makes sense. Anything more is far >> less certain. > > The early read can be handy for bootloader who don't want to implement > complete VirtIO programming. I've said this twice already. This is the last time. 1) We do not add complexity unless we have to. 2) Making it optional does not make it significantly less complex. 3) Having two ways of doing the same thing is always ugly. 4) Having an emergency output method makes sense for several use cases, an emergency input method does not. 5) A bootloader which uses virtio to get the images to boot can implement a console in less than 10 lines. 6) A bootloader which does not use any virtio devices but nonetheless wants to obtain input can implement a simple console in well under 100 lines. See below. Finally, in case you still don't understand: My task is to make this decision, and I've made it. Arguing with me is only useful if you bring new facts which you can change my mind. Hope that clarifies, Rusty. #define VIRTIO_MMIO_GUEST_PAGE_SIZE 0x028 #define VIRTIO_MMIO_QUEUE_SEL 0x030 #define VIRTIO_MMIO_QUEUE_NUM_MAX 0x034 #define VIRTIO_MMIO_QUEUE_NUM 0x038 #define VIRTIO_MMIO_QUEUE_ALIGN 0x03c #define VIRTIO_MMIO_QUEUE_PFN 0x040 #define VIRTIO_MMIO_QUEUE_NOTIFY 0x050 struct vring_desc { __u64 addr; __u32 len; __u16 flags; __u16 next; }; struct vring_used_elem { __u32 id; __u32 len; }; struct vconsole_ring { struct vring_desc desc[2]; __u16 avail_flags; __u16 avail_idx; __u16 available[2]; __u16 used_event_idx; __u16 pad; /* Hit 4-byte boundary */ // A ring of used descriptor heads with free-running index. __u16 used_flags; __u16 used_idx; struct vring_used_elem used[2]; __u16 avail_event_idx; }; static char console_in; static struct vconsole_ring vc_ring = { { { PHYSICAL_ADDR(console_in), 1, 2, 0 } }, 1, /* No interrupts */ } static void post_buffer(void *base) { vc_ring->avail_idx++; wmb(); writel(0, base + VIRTIO_MMIO_QUEUE_NOTIFY); } bool vc_read(void *base, char *c) { mb(); if (vc_ring->used_idx != vc_ring->avail_idx) return false; *c = console_in; post_buffer(base); return true; } void vc_init(void *base) { /* Alignment of 4 bytes, don't waste any. */ writel(4, base + VIRTIO_MMIO_GUEST_PAGE_SIZE); /* Setup incoming vq. */ writel(0, base + VIRTIO_MMIO_QUEUE_SEL); /* 2 elements */ writel(2, base + VIRTIO_MMIO_QUEUE_NUM); /* Alignment of 4 bytes */ writel(4, base + VIRTIO_MMIO_QUEUE_ALIGN); /* Location of queue. */ writel(PHYSICAL_ADDR(&vc_ring) >> 4, base + VIRTIO_MMIO_QUEUE_PFN); post_buffer(base); }
On 22.04.2013, at 05:10, Anup Patel wrote: > On 22 April 2013 06:51, Rusty Russell <rusty@rustcorp.com.au> wrote: >> >> Pranavkumar Sawargaonkar <pranavkumar@linaro.org> writes: >>> On 18 April 2013 12:21, Rusty Russell <rusty@rustcorp.com.au> wrote: >>>> >>>> PranavkumarSawargaonkar <pranavkumar@linaro.org> writes: >>>>> From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >>>>> >>>>> This patch implements early printk support for virtio-mmio console >>>>> devices without using any hypercalls. >>>> >>>> This makes some sense, though not sure that early console *read* makes >>>> much sense. I can see the PCI version of this being useful as well. >>> >>> Read can be useful for "mach-virt" which will have only virtio console >>> as a console device. Then if someone wants to have UEFI or any other >>> boot-loader emulation, which expects user to input few things, in that >>> case read might become handy. >> >> But implementing virtio inside a bootloader has already been done for >> coreboot, for example. A bootloader probably wants a virtio block >> device, so a console is trivial. >> >> A single writable field for debugging makes sense. Anything more is far >> less certain. > > The early read can be handy for bootloader who don't want to implement > complete VirtIO programming. Virtio is trivial. Seriously. Don't invent new secondary interfaces to the same thing just because you're afraid to write 5 lines of code instead of 2. Alex > > IMHO, early read would be totally optional for host and will not > introduce any new config register so it is good to have in VirtIO > console spec. Also, without early read the read behavior of early_rw > field would be undefined in VirtIO console spec. > >> >> Thanks, >> Rusty. > > Best Regards, > Anup > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
On 22 April 2013 10:45, Rusty Russell <rusty@rustcorp.com.au> wrote: > Anup Patel <anup.patel@linaro.org> writes: >> On 22 April 2013 06:51, Rusty Russell <rusty@rustcorp.com.au> wrote: >>> >>> Pranavkumar Sawargaonkar <pranavkumar@linaro.org> writes: >>> > On 18 April 2013 12:21, Rusty Russell <rusty@rustcorp.com.au> wrote: >>> >> >>> >> PranavkumarSawargaonkar <pranavkumar@linaro.org> writes: >>> >> > From: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >>> >> > >>> >> > This patch implements early printk support for virtio-mmio console >>> >> > devices without using any hypercalls. >>> >> >>> >> This makes some sense, though not sure that early console *read* makes >>> >> much sense. I can see the PCI version of this being useful as well. >>> > >>> > Read can be useful for "mach-virt" which will have only virtio console >>> > as a console device. Then if someone wants to have UEFI or any other >>> > boot-loader emulation, which expects user to input few things, in that >>> > case read might become handy. >>> >>> But implementing virtio inside a bootloader has already been done for >>> coreboot, for example. A bootloader probably wants a virtio block >>> device, so a console is trivial. >>> >>> A single writable field for debugging makes sense. Anything more is far >>> less certain. >> >> The early read can be handy for bootloader who don't want to implement >> complete VirtIO programming. > > I've said this twice already. This is the last time. > > 1) We do not add complexity unless we have to. > 2) Making it optional does not make it significantly less complex. > 3) Having two ways of doing the same thing is always ugly. > 4) Having an emergency output method makes sense for several use cases, > an emergency input method does not. Okay, i will restructure my patch by keeping just output write part and post it back. Thanks, Pranav > 5) A bootloader which uses virtio to get the images to boot can > implement a console in less than 10 lines. > 6) A bootloader which does not use any virtio devices but nonetheless > wants to obtain input can implement a simple console in well under 100 > lines. See below. > > Finally, in case you still don't understand: > > My task is to make this decision, and I've made it. Arguing with me is > only useful if you bring new facts which you can change my mind. > > Hope that clarifies, > Rusty. > > #define VIRTIO_MMIO_GUEST_PAGE_SIZE 0x028 > #define VIRTIO_MMIO_QUEUE_SEL 0x030 > #define VIRTIO_MMIO_QUEUE_NUM_MAX 0x034 > #define VIRTIO_MMIO_QUEUE_NUM 0x038 > #define VIRTIO_MMIO_QUEUE_ALIGN 0x03c > #define VIRTIO_MMIO_QUEUE_PFN 0x040 > #define VIRTIO_MMIO_QUEUE_NOTIFY 0x050 > > struct vring_desc { > __u64 addr; > __u32 len; > __u16 flags; > __u16 next; > }; > > struct vring_used_elem { > __u32 id; > __u32 len; > }; > > struct vconsole_ring { > struct vring_desc desc[2]; > __u16 avail_flags; > __u16 avail_idx; > __u16 available[2]; > __u16 used_event_idx; > __u16 pad; /* Hit 4-byte boundary */ > > // A ring of used descriptor heads with free-running index. > __u16 used_flags; > __u16 used_idx; > struct vring_used_elem used[2]; > __u16 avail_event_idx; > }; > > static char console_in; > static struct vconsole_ring vc_ring = { > { { PHYSICAL_ADDR(console_in), 1, 2, 0 } }, > 1, /* No interrupts */ > } > > static void post_buffer(void *base) > { > vc_ring->avail_idx++; > wmb(); > writel(0, base + VIRTIO_MMIO_QUEUE_NOTIFY); > } > > bool vc_read(void *base, char *c) > { > mb(); > if (vc_ring->used_idx != vc_ring->avail_idx) > return false; > *c = console_in; > post_buffer(base); > return true; > } > > void vc_init(void *base) > { > /* Alignment of 4 bytes, don't waste any. */ > writel(4, base + VIRTIO_MMIO_GUEST_PAGE_SIZE); > > /* Setup incoming vq. */ > writel(0, base + VIRTIO_MMIO_QUEUE_SEL); > > /* 2 elements */ > writel(2, base + VIRTIO_MMIO_QUEUE_NUM); > /* Alignment of 4 bytes */ > writel(4, base + VIRTIO_MMIO_QUEUE_ALIGN); > /* Location of queue. */ > writel(PHYSICAL_ADDR(&vc_ring) >> 4, base + VIRTIO_MMIO_QUEUE_PFN); > > post_buffer(base); > }
diff --git a/arch/arm64/kernel/early_printk.c b/arch/arm64/kernel/early_printk.c index ac974f4..a82b5aa 100644 --- a/arch/arm64/kernel/early_printk.c +++ b/arch/arm64/kernel/early_printk.c @@ -25,6 +25,9 @@ #include <linux/amba/serial.h> #include <linux/serial_reg.h> +#include <linux/virtio_ids.h> +#include <linux/virtio_mmio.h> +#include <linux/virtio_console.h> static void __iomem *early_base; static void (*printch)(char ch); @@ -53,6 +56,26 @@ static void smh_printch(char ch) } /* + * VIRTIO MMIO based debug console. + */ +static void virtio_console_early_printch(char ch) +{ + u32 tmp; + struct virtio_console_config *p = early_base + VIRTIO_MMIO_CONFIG; + + tmp = readl_relaxed(early_base + VIRTIO_MMIO_DEVICE_ID); + if (tmp != VIRTIO_ID_CONSOLE) { + return; + } + + tmp = readl_relaxed(early_base + VIRTIO_MMIO_HOST_FEATURES); + if (!(tmp & (1 << VIRTIO_CONSOLE_F_EARLY_WRITE))) { + return; + } + writeb_relaxed(ch, &p->early_rw); +} + +/* * 8250/16550 (8-bit aligned registers) single character TX. */ static void uart8250_8bit_printch(char ch) @@ -82,6 +105,7 @@ static const struct earlycon_match earlycon_match[] __initconst = { { .name = "smh", .printch = smh_printch, }, { .name = "uart8250-8bit", .printch = uart8250_8bit_printch, }, { .name = "uart8250-32bit", .printch = uart8250_32bit_printch, }, + { .name = "virtio-console", .printch = virtio_console_early_printch, }, {} }; diff --git a/include/uapi/linux/virtio_console.h b/include/uapi/linux/virtio_console.h index ee13ab6..1171cb4 100644 --- a/include/uapi/linux/virtio_console.h +++ b/include/uapi/linux/virtio_console.h @@ -38,6 +38,8 @@ /* 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_EARLY_READ 2 /* Does host support early read? */ +#define VIRTIO_CONSOLE_F_EARLY_WRITE 3 /* Does host support early write? */ #define VIRTIO_CONSOLE_BAD_ID (~(u32)0) @@ -48,6 +50,8 @@ struct virtio_console_config { __u16 rows; /* max. number of ports this device can hold */ __u32 max_nr_ports; + /* early read/write register */ + __u32 early_rw; } __attribute__((packed)); /*