[RFC] arm64: Early printk support for virtio-mmio console devices.
Alexander Graf
agraf at suse.de
Thu Apr 18 04:53:58 EDT 2013
Am 18.04.2013 um 10:48 schrieb Pranavkumar Sawargaonkar <pranavkumar at linaro.org>:
> Hi Marc,
>
> On 18 April 2013 13:06, Marc Zyngier <marc.zyngier at arm.com> wrote:
>> On Thu, 18 Apr 2013 12:47:18 +0530, Pranavkumar Sawargaonkar
>> <pranavkumar at linaro.org> wrote:
>>> Hi Marc,
>>>
>>> On 18 April 2013 12:19, Marc Zyngier <maz at misterjones.org> wrote:
>>>
>>>> Hi Pranavkumar,
>>>>
>>>> On Thu, 18 Apr 2013 11:22:24 +0530, PranavkumarSawargaonkar
>>>> <pranavkumar at linaro.org> wrote:
>>>>> From: Pranavkumar Sawargaonkar <pranavkumar at 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 at 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 at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
More information about the linux-arm-kernel
mailing list