[RFC] arm64: Early printk support for virtio-mmio console devices.

[RFC] arm64: Early printk support for virtio-mmio console devices.

[PranavkumarSawargaonkar]

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.

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);
+}
+
+/*
  * 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));
 
 /*
-- 
1.7.9.5

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Marc Zyngier]

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.
-- 
Who you jivin' with that Cosmik Debris?

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Rusty Russell]

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.

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Pranavkumar Sawargaonkar]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Pranavkumar Sawargaonkar]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Marc Zyngier]

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.
-- 
Fast, cheap, reliable. Pick two.

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Peter Maydell]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Alexander Graf]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Pranavkumar Sawargaonkar]

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.

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Alexander Graf]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Marc Zyngier]

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.
-- 
Fast, cheap, reliable. Pick two.

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Alexander Graf]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Pranavkumar Sawargaonkar]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Jean-Christophe PLAGNIOL-VILLARD]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Christopher Covington]

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

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Marc Zyngier]

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.
-- 
Fast, cheap, reliable. Pick two.

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Pranavkumar Sawargaonkar]

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.

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Pranavkumar Sawargaonkar]

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.

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Will Deacon]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Pranavkumar Sawargaonkar]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Will Deacon]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Peter Maydell]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Pranavkumar Sawargaonkar]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Will Deacon]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Will Deacon]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Peter Maydell]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Catalin Marinas]

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).

-- 
Catalin

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Peter Maydell]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Catalin Marinas]

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.

-- 
Catalin

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Peter Maydell]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Catalin Marinas]

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.

-- 
Catalin

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Peter Maydell]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Rusty Russell]

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.

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Anup Patel]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Rusty Russell]

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);
}

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Alexander Graf]

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

Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

[Pranavkumar Sawargaonkar]

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);
> }