Re: [PATCH 2/2] m68k: introduce a virtual m68k machine

From: Geert Uytterhoeven
Date: Wed Apr 28 2021 - 08:04:25 EST

Next message: Jaroslav Kysela: "Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices"
Previous message: Colin King: "[PATCH][next] net: dsa: ksz: Make reg_mib_cnt a u8 as it never exceeds 255"
In reply to: Laurent Vivier: "Re: [PATCH 2/2] m68k: introduce a virtual m68k machine"
Next in thread: Josh Juran: "Re: [PATCH 2/2] m68k: introduce a virtual m68k machine"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Laurent,

On Tue, Mar 23, 2021 at 11:14 PM Laurent Vivier <laurent@xxxxxxxxx> wrote:
> This machine allows to have up to 3.2 GiB and 128 Virtio devices.
>
> It is based on android goldfish devices.
>
> Signed-off-by: Laurent Vivier <laurent@xxxxxxxxx>

Thanks for your patch!

> --- a/arch/m68k/Kconfig.machine
> +++ b/arch/m68k/Kconfig.machine
> @@ -145,6 +145,23 @@ config SUN3
>
> If you don't want to compile a kernel exclusively for a Sun 3, say N.
>
> +config VIRT
> + bool "Virtual M68k Machine support"
> + depends on MMU
> + select MMU_MOTOROLA if MMU
> + select M68040
> + select LEGACY_TIMER_TICK

Can we avoid selecting this for a new platform?

> + select VIRTIO_MENU

VIRTIO_MENU defaults to y (should it?), so this can be dropped.

> + select VIRTIO_MMIO
> + select GOLDFISH
> + select TTY
> + select GOLDFISH_TTY
> + select RTC_CLASS
> + select RTC_DRV_GOLDFISH

Please sort the selects.

> + help
> + This options enable a pure virtual machine based on m68k,
> + VIRTIO MMIO devices and GOLDFISH interfaces (TTY, RTC, PIC)
> +
> config PILOT
> bool
>
> diff --git a/arch/m68k/configs/virt_defconfig b/arch/m68k/configs/virt_defconfig
> new file mode 100644
> index 000000000000..51842acd5434
> --- /dev/null
> +++ b/arch/m68k/configs/virt_defconfig
> @@ -0,0 +1,93 @@

This is not a minimal config, please run "make savedefconfig"
and replace by the generated defconfig.

Please also add CONFIG_LOCALVERSION="-virt", for consistency with the
other configs.

> --- a/arch/m68k/include/asm/irq.h
> +++ b/arch/m68k/include/asm/irq.h
> @@ -12,7 +12,8 @@
> */
> #if defined(CONFIG_COLDFIRE)
> #define NR_IRQS 256
> -#elif defined(CONFIG_VME) || defined(CONFIG_SUN3) || defined(CONFIG_SUN3X)
> +#elif defined(CONFIG_VME) || defined(CONFIG_SUN3) || \
> + defined(CONFIG_SUN3X) || defined(CONFIG_VIRT)
> #define NR_IRQS 200

Is this related to NUM_VIRT_SOURCES?

Yes it is:

m68k_setup_irq_controller(&virt_irq_chip, handle_simple_irq, IRQ_USER,
NUM_VIRT_SOURCES - IRQ_USER);

> #elif defined(CONFIG_ATARI)
> #define NR_IRQS 141

> --- a/arch/m68k/include/asm/setup.h
> +++ b/arch/m68k/include/asm/setup.h
> @@ -170,6 +180,20 @@ extern unsigned long m68k_machtype;
> # define MACH_TYPE (MACH_SUN3X)
> #endif
>
> +#if !defined(CONFIG_VIRT)
> +# define MACH_IS_VIRT (0)
> +#elif defined(CONFIG_AMIGA) || defined(CONFIG_ATARI) || defined(CONFIG_APOLLO) \
> + || defined(CONFIG_MVME16x) || defined(CONFIG_BVME6000) \
> + || defined(CONFIG_HP300) || defined(CONFIG_Q40) \
> + || defined(CONFIG_SUN3X) || defined(CONFIG_MVME147) \
> + || defined(CONFIG_MAC)

Please move the MAC check between the ATARI and APOLLO checks.

> +# define MACH_IS_VIRT (m68k_machtype == MACH_VIRT)
> +#else
> +# define MACH_VIRTONLY

MACH_VIRT_ONLY (albeit unused)

> +# define MACH_IS_VIRT (1)
> +# define MACH_TYPE (MACH_VIRT)
> +#endif
> +
> #ifndef MACH_TYPE
> # define MACH_TYPE (m68k_machtype)
> #endif

> --- /dev/null
> +++ b/arch/m68k/include/asm/virt.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_VIRT_H
> +#define __ASM_VIRT_H
> +
> +#define NUM_VIRT_SOURCES 200
> +
> +struct virt_booter_device_data {
> + unsigned long mmio;
> + unsigned long irq;
> +};
> +
> +struct virt_booter_data {
> + unsigned long qemu_version;
> + struct virt_booter_device_data pic;
> + struct virt_booter_device_data rtc;
> + struct virt_booter_device_data tty;
> + struct virt_booter_device_data ctrl;
> + struct virt_booter_device_data virtio;
> +};
> +
> +extern struct virt_booter_data virt_bi_data;
> +
> +extern void __init virt_init_IRQ(void);
> +extern void __init virt_sched_init(void);
> +
> +#endif
> diff --git a/arch/m68k/include/uapi/asm/bootinfo-virt.h b/arch/m68k/include/uapi/asm/bootinfo-virt.h
> new file mode 100644
> index 000000000000..ab17fd9d200d
> --- /dev/null
> +++ b/arch/m68k/include/uapi/asm/bootinfo-virt.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * asm/bootinfo-virt.h -- Virtual-m68k-specific boot information definitions
> + */
> +
> +#ifndef _UAPI_ASM_M68K_BOOTINFO_VIRT_H
> +#define _UAPI_ASM_M68K_BOOTINFO_VIRT_H
> +
> +#define BI_VIRT_QEMU_VERSION 0x8000
> +#define BI_VIRT_GF_PIC_BASE 0x8001
> +#define BI_VIRT_GF_RTC_BASE 0x8002
> +#define BI_VIRT_GF_TTY_BASE 0x8003
> +#define BI_VIRT_VIRTIO_BASE 0x8004
> +#define BI_VIRT_CTRL_BASE 0x8005
> +
> +#define VIRT_BOOTI_VERSION MK_BI_VERSION(2, 0)
> +
> +#endif /* _UAPI_ASM_M68K_BOOTINFO_MAC_H */
> diff --git a/arch/m68k/include/uapi/asm/bootinfo.h b/arch/m68k/include/uapi/asm/bootinfo.h
> index 38d3140381fa..203d9cbf9630 100644
> --- a/arch/m68k/include/uapi/asm/bootinfo.h
> +++ b/arch/m68k/include/uapi/asm/bootinfo.h
> @@ -83,6 +83,7 @@ struct mem_info {
> #define MACH_SUN3X 11
> #define MACH_M54XX 12
> #define MACH_M5441X 13
> +#define MACH_VIRT 14

All of the above are already in use in qemu, so I have to accept them ;-)

(Do I see a missed opportunity for adding DT support?...)

> --- a/arch/m68k/kernel/head.S
> +++ b/arch/m68k/kernel/head.S

> @@ -3186,6 +3203,13 @@ func_start serial_putc,%d0/%d1/%a0/%a1
> 3:
> #endif
>
> +#ifdef CONFIG_VIRT
> + is_not_virt(L(serial_putc_done))

Please jump to a new label before the #endif, to make it easier to add
new platforms.

> +
> + movel L(virt_gf_tty_base),%a1
> + moveb %d0,%a1@(GF_PUT_CHAR)
> +#endif
> +
> L(serial_putc_done):
> func_return serial_putc

> --- a/arch/m68k/mm/kmap.c
> +++ b/arch/m68k/mm/kmap.c

> @@ -293,18 +299,20 @@ EXPORT_SYMBOL(__ioremap);
> */
> void iounmap(void __iomem *addr)
> {
> -#ifdef CONFIG_AMIGA
> - if ((!MACH_IS_AMIGA) ||
> - (((unsigned long)addr < 0x40000000) ||
> - ((unsigned long)addr > 0x60000000)))
> - free_io_area((__force void *)addr);
> +#if defined(CONFIG_AMIGA) || defined(CONFIG_VIRT)

Please split in two separate #ifdefs,...

> + if (MACH_IS_AMIGA &&
> + ((unsigned long)addr >= 0x40000000) &&
> + ((unsigned long)addr < 0x60000000))
> + return;
> + if (MACH_IS_VIRT && (unsigned long)addr >= 0xff000000)
> + return;
> #else

... drop the #else, ...

> #ifdef CONFIG_COLDFIRE
> if (cf_internalio(addr))
> return;
> #endif
> - free_io_area((__force void *)addr);
> #endif

... and drop this #endif

> + free_io_area((__force void *)addr);
> }
> EXPORT_SYMBOL(iounmap);

> --- /dev/null
> +++ b/arch/m68k/virt/config.c
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/serial_core.h>
> +
> +#include <asm/bootinfo.h>
> +#include <asm/bootinfo-virt.h>
> +#include <asm/byteorder.h>
> +

Please drop this blank line.

> +#include <asm/machdep.h>
> +#include <asm/virt.h>

> +static void virt_get_model(char *str)
> +{
> + /* str is 80 characters long */
> + sprintf(str, "QEMU Virtual M68K Machine (%d.%d.%d)",

Please use %u for unsigned numbers.

> + (u8)(virt_bi_data.qemu_version >> 24),
> + (u8)(virt_bi_data.qemu_version >> 16),
> + (u8)(virt_bi_data.qemu_version >> 8));
> +}
> +
> +extern void show_registers(struct pt_regs *);

This is unused.

> +void __init config_virt(void)
> +{
> + char earlycon[24];
> +
> + if (!MACH_IS_VIRT)
> + pr_err("ERROR: no Virtual M68k Machine, but %s called!!\n",
> + __func__);

This cannot happen, so please drop.

> diff --git a/arch/m68k/virt/ints.c b/arch/m68k/virt/ints.c
> new file mode 100644
> index 000000000000..aa94cb3b6d96
> --- /dev/null
> +++ b/arch/m68k/virt/ints.c
> @@ -0,0 +1,110 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/sched/debug.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/delay.h>
> +
> +#include <asm/virt.h>
> +#include <asm/irq.h>
> +#include <asm/hwtest.h>
> +#include <asm/irq_regs.h>

Please sort includes.

> +static void virt_irq_enable(struct irq_data *data)
> +{
> + GF_PIC(data->irq).enable = 1 << GF_IRQ(data->irq);
> +}
> +
> +static void virt_irq_disable(struct irq_data *data)
> +{
> + GF_PIC(data->irq).disable = 1 << GF_IRQ(data->irq);
> +}
> +
> +static unsigned int virt_irq_startup(struct irq_data *data)
> +{
> + GF_PIC(data->irq).enable = 1 << GF_IRQ(data->irq);

Just call virt_irq_enable()?

> + return 0;
> +}
> +
> +static void virt_irq_shutdown(struct irq_data *data)
> +{
> + GF_PIC(data->irq).disable = 1 << GF_IRQ(data->irq);
> +}

This is identical to virt_irq_disable(), so you can just use the latter below.

> +
> +static volatile int in_nmi;

This is used inside virt_nmi_handler() only, so please move it there.
> +
> +irqreturn_t virt_nmi_handler(int irq, void *dev_id)
> +{
> + if (in_nmi)
> + return IRQ_HANDLED;
> + in_nmi = 1;
> +
> + pr_info("Non-Maskable Interrupt\n");

pr_warn()? Or another pr_*()?

> + show_registers(get_irq_regs());
> +
> + in_nmi = 0;
> + return IRQ_HANDLED;
> +}
> +
> +static struct irq_chip virt_irq_chip = {
> + .name = "virt",
> + .irq_enable = virt_irq_enable,
> + .irq_disable = virt_irq_disable,
> + .irq_startup = virt_irq_startup,
> + .irq_shutdown = virt_irq_shutdown,

... = virt_irq_disable,

> +};
> +
> +static void goldfish_pic_irq(struct irq_desc *desc)
> +{
> + u32 irq_pending, irq_bit;
> + int irq_num;
> +
> + irq_pending = gf_pic[desc->irq_data.irq - 1].irq_pending;
> + irq_num = IRQ_USER + (desc->irq_data.irq - 1) * 32;
> + irq_bit = 1;
> +
> + do {
> + if (irq_pending & irq_bit) {
> + generic_handle_irq(irq_num);
> + irq_pending &= ~irq_bit;
> + }
> + ++irq_num;
> + irq_bit <<= 1;
> + } while (irq_pending);

This can be simplified by shifting irq_pending instead of irq_bit:

do {
if (irq_pending & 1)
generic_handle_irq(irq_num);

++irq_num;
irq_pending >>= 1;
} while (irq_pending);

Unfortunately m68k doesn't have a single-instruction __ffs().

> +}
> +
> +void __init virt_init_IRQ(void)
> +{
> + int i;
> +
> + m68k_setup_irq_controller(&virt_irq_chip, handle_simple_irq, IRQ_USER,
> + NUM_VIRT_SOURCES - IRQ_USER);
> +
> + for (i = 0; i < 6; i++) {

6 = NUM_VIRT_SOURCES / 32?
If yes, what about the last 8 irqs?

> + irq_set_chained_handler(virt_bi_data.pic.irq + i,
> + goldfish_pic_irq);
> + }

> --- /dev/null
> +++ b/arch/m68k/virt/platform.c
> @@ -0,0 +1,80 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <asm/virt.h>
> +#include <asm/irq.h>
> +
> +static struct platform_device virt_m68k_goldfish_tty = {
> + .name = "goldfish_tty",
> + .id = PLATFORM_DEVID_NONE,
> + .num_resources = 2,
> + .resource = (struct resource [2]) { },
> +};
> +static struct platform_device virt_m68k_goldfish_rtc = {
> + .name = "goldfish_rtc",
> + .id = PLATFORM_DEVID_NONE,
> + .num_resources = 2,
> + .resource = (struct resource [2]) { },
> +};
> +
> +#define VIRTIO_BUS_NB 128
> +static struct platform_device virt_m68k_virtio_mmio_device[VIRTIO_BUS_NB];
> +static struct resource virt_m68k_virtio_mmio_resources[VIRTIO_BUS_NB][2];

This consumes more than 40 KiB, even when unused.
While this doesn't matter much for a virtual machine with 3.2 GiB of
RAM, it does matter for running a multi-platform kernel on a real
machine. Hence please allocate dynamically.

> +
> +static int __init virt_platform_init(void)
> +{
> + int err;
> + int i;
> + extern unsigned long min_low_pfn;

Please use reverse Christmas tree declaration order.

> +
> + if (!MACH_IS_VIRT)
> + return -ENODEV;
> +
> + min_low_pfn = 0;

Why is this needed?

> +
> + virt_m68k_goldfish_tty.resource[0].flags = IORESOURCE_MEM;
> + virt_m68k_goldfish_tty.resource[0].start = virt_bi_data.tty.mmio;
> + virt_m68k_goldfish_tty.resource[0].end = virt_bi_data.tty.mmio;
> + virt_m68k_goldfish_tty.resource[1].flags = IORESOURCE_IRQ;
> + virt_m68k_goldfish_tty.resource[1].start = virt_bi_data.tty.irq;
> + virt_m68k_goldfish_tty.resource[1].end = virt_bi_data.tty.irq;
> +
> + err = platform_device_register(&virt_m68k_goldfish_tty);

You could probably save a little bit of memory by calling
platform_device_register_simple() instead.

> + if (err)
> + return err;
> +
> + virt_m68k_goldfish_rtc.resource[0].flags = IORESOURCE_MEM;
> + virt_m68k_goldfish_rtc.resource[0].start = virt_bi_data.rtc.mmio + 0x1000;
> + virt_m68k_goldfish_rtc.resource[0].end = virt_bi_data.rtc.mmio + 0x1fff;
> + virt_m68k_goldfish_rtc.resource[1].flags = IORESOURCE_IRQ;
> + virt_m68k_goldfish_rtc.resource[1].start = virt_bi_data.rtc.irq + 1;
> + virt_m68k_goldfish_rtc.resource[1].end = virt_bi_data.rtc.irq + 1;
> + err = platform_device_register(&virt_m68k_goldfish_rtc);

Likewise.

> --- /dev/null
> +++ b/arch/m68k/virt/timer.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/interrupt.h>
> +#include <linux/ioport.h>
> +#include <linux/clocksource.h>
> +#include <asm/virt.h>
> +
> +struct goldfish_timer {
> + u32 time_low;
> + u32 time_high;
> + u32 alarm_low;
> + u32 alarm_high;
> + u32 irq_enabled;
> + u32 clear_alarm;
> + u32 alarm_status;
> + u32 clear_interrupt;
> +};
> +
> +#define gf_timer ((volatile struct goldfish_timer *)virt_bi_data.rtc.mmio)
> +
> +static u64 goldfish_timer_read(struct clocksource *cs)
> +{
> + u64 ticks;
> +
> + ticks = gf_timer->time_low;
> + ticks += (u64)gf_timer->time_high << 32;

Can time_low wrap in between the two reads?

> +
> + return ticks;
> +}
> +
> +static struct clocksource goldfish_timer = {
> + .name = "goldfish_timer",
> + .rating = 400,
> + .read = goldfish_timer_read,
> + .mask = CLOCKSOURCE_MASK(64),
> + .flags = 0,
> + .max_idle_ns = LONG_MAX,
> +};
> +
> +static irqreturn_t golfish_timer_handler(int irq, void *dev_id)
> +{
> + unsigned long flags;
> + u64 now;
> +
> + local_irq_save(flags);

Do we need this in an interrupt handler?

> + gf_timer->clear_interrupt = 1;
> +
> + now = gf_timer->time_low;
> + now += (u64)gf_timer->time_high << 32;

now = goldfish_timer_read();

> +
> + legacy_timer_tick(1);
> +
> + now += NSEC_PER_SEC / HZ;
> + gf_timer->alarm_high = now >> 32;

upper_32_bits(now)

> + gf_timer->alarm_low = (u32)now;

lower_32_bits(now)

> + local_irq_restore(flags);
> +
> + return IRQ_HANDLED;
> +}
> +
> +void __init virt_sched_init(void)
> +{
> + u64 now;
> + static struct resource sched_res;

Please use reverse Christmas tree declaration order.

> +
> + sched_res.name = "goldfish_timer";
> + sched_res.start = virt_bi_data.rtc.mmio;
> + sched_res.end = virt_bi_data.rtc.mmio + 0xfff;
> +
> + if (request_resource(&iomem_resource, &sched_res)) {
> + pr_err("Cannot allocate goldfish-timer resource\n");
> + return;
> + }
> +
> + if (request_irq(virt_bi_data.rtc.irq, golfish_timer_handler, IRQF_TIMER,
> + "timer", NULL)) {
> + pr_err("Couldn't register timer interrupt\n");
> + return;
> + }
> +
> + now = gf_timer->time_low;
> + now += (u64)gf_timer->time_high << 32;

now = goldfish_timer_read();

> + now += NSEC_PER_SEC / HZ;
> +
> + gf_timer->clear_interrupt = 1;
> + gf_timer->alarm_high = now >> 32;

upper_32_bits(now)

> + gf_timer->alarm_low = (u32)now;

lower_32_bits(now)

> + gf_timer->irq_enabled = 1;
> +
> + clocksource_register_hz(&goldfish_timer, NSEC_PER_SEC);
> +}

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Next message: Jaroslav Kysela: "Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices"
Previous message: Colin King: "[PATCH][next] net: dsa: ksz: Make reg_mib_cnt a u8 as it never exceeds 255"
In reply to: Laurent Vivier: "Re: [PATCH 2/2] m68k: introduce a virtual m68k machine"
Next in thread: Josh Juran: "Re: [PATCH 2/2] m68k: introduce a virtual m68k machine"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]