Re: [PATCH 10/15] MIPS: Add code for new system 'paravirt'.

[James Hogan <james.hogan@imgtec.com>]

On 20/05/14 15:47, Andreas Herrmann wrote:
> diff --git a/arch/mips/include/asm/mach-paravirt/kernel-entry-init.h b/arch/mips/include/asm/mach-paravirt/kernel-entry-init.h
> new file mode 100644
> index 0000000..c812efa
> --- /dev/null
> +++ b/arch/mips/include/asm/mach-paravirt/kernel-entry-init.h
> @@ -0,0 +1,49 @@

> +/*
> + * Do SMP slave processor setup necessary before we can safely execute
> + * C code.
> + */
> +	.macro  smp_slave_setup
> +	mfc0	t0, CP0_EBASE
> +	andi	t0, t0, 0x3ff		# CPUNum
> +	slti	t1, t0, NR_CPUS
> +	bnez	t1, 1f
> +2:
> +	di
> +	wait
> +	b	2b			# Unknown CPU, loop forever.
> +1:
> +	PTR_LA	t1, paravirt_smp_sp
> +	PTR_SLL	t0, PTR_SCALESHIFT
> +	PTR_ADDU t1, t1, t0
> +3:
> +	PTR_L	sp, 0(t1)
> +	beqz	sp, 3b			# Spin until told to proceed.
> +
> +	PTR_LA	t1, paravirt_smp_gp
> +	PTR_ADDU t1, t1, t0

Usually smp_wmb() at the writer needs to be paired with smp_rmb() at the
reader (i.e. here) to guarantee that the two memory locations become
visible to this CPU in the correct order, so I think you need a sync of
some kind between here to be portable beyond Octeon.

> +	PTR_L	gp, 0(t1)
> +	.endm


> diff --git a/arch/mips/paravirt/paravirt-irq.c b/arch/mips/paravirt/paravirt-irq.c
> new file mode 100644
> index 0000000..e1603dd
> --- /dev/null
> +++ b/arch/mips/paravirt/paravirt-irq.c
> @@ -0,0 +1,388 @@
> +/*
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * Copyright (C) 2013 Cavium, Inc.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/cpumask.h>
> +#include <linux/kernel.h>
> +#include <linux/mutex.h>
> +
> +#include <asm/io.h>
> +
> +#define MBOX_BITS_PER_CPU 2
> +
> +int cpunum_for_cpu(int cpu)

static?

> +{
> +#ifdef CONFIG_SMP
> +	return cpu_logical_map(cpu);
> +#else
> +	return mips_cpunum();
> +#endif
> +}

> +static void irq_core_set_enable_local(void *arg)
> +{
> +	struct irq_data *data = arg;
> +	struct core_chip_data *cd = irq_data_get_irq_chip_data(data);
> +	unsigned int mask = 0x100 << cd->bit;
> +
> +	/*
> +	 * Interrupts are already disabled, so these are atomic.

Really? Even when called directly from irq_core_bus_sync_unlock with
only a single core online?

> +	 */
> +	if (cd->desired_en)
> +		set_c0_status(mask);
> +	else
> +		clear_c0_status(mask);
> +
> +}
> +
> +static void irq_core_disable(struct irq_data *data)
> +{
> +	struct core_chip_data *cd = irq_data_get_irq_chip_data(data);
> +	cd->desired_en = false;
> +}
> +
> +static void irq_core_enable(struct irq_data *data)
> +{
> +	struct core_chip_data *cd = irq_data_get_irq_chip_data(data);
> +	cd->desired_en = true;
> +}
> +
> +static void irq_core_bus_lock(struct irq_data *data)
> +{
> +	struct core_chip_data *cd = irq_data_get_irq_chip_data(data);
> +
> +	mutex_lock(&cd->core_irq_mutex);
> +}
> +
> +static void irq_core_bus_sync_unlock(struct irq_data *data)
> +{
> +	struct core_chip_data *cd = irq_data_get_irq_chip_data(data);
> +
> +	if (cd->desired_en != cd->current_en) {
> +		/*
> +		 * Can be called in early init when on_each_cpu() will
> +		 * unconditionally enable irqs, so handle the case
> +		 * where only a single CPU is online specially, and
> +		 * directly call.
> +		 */
> +		if (num_online_cpus() == 1)
> +			irq_core_set_enable_local(data);
> +		else
> +			on_each_cpu(irq_core_set_enable_local, data, 1);
> +
> +		cd->current_en = cd->desired_en;
> +	}
> +
> +	mutex_unlock(&cd->core_irq_mutex);
> +}


> +static int irq_pci_set_affinity(struct irq_data *data, const struct cpumask *dest, bool force)
> +{
> +	return 0;
> +}

Is there any point even providing this callback?

> +
> +static void irq_pci_cpu_offline(struct irq_data *data)
> +{
> +}

Or this one?

> +
> +static struct irq_chip irq_chip_pci = {
> +	.name = "PCI",
> +	.irq_enable = irq_pci_enable,
> +	.irq_disable = irq_pci_disable,
> +	.irq_ack = irq_pci_ack,
> +	.irq_mask = irq_pci_mask,
> +	.irq_unmask = irq_pci_unmask,
> +	.irq_set_affinity = irq_pci_set_affinity,
> +	.irq_cpu_offline = irq_pci_cpu_offline,
> +};


> diff --git a/arch/mips/paravirt/paravirt-smp.c b/arch/mips/paravirt/paravirt-smp.c
> new file mode 100644
> index 0000000..52f86eb
> --- /dev/null
> +++ b/arch/mips/paravirt/paravirt-smp.c

> +static void paravirt_smp_finish(void)
> +{
> +	/* to generate the first CPU timer interrupt */
> +	write_c0_compare(read_c0_count() + mips_hpt_frequency / HZ);

This strikes me as a bit hacky. Are you sure it's actually necessary? (I
would have expected some generic hotplug notifier somewhere to ensure
that percpu clocksources gets initialised sensibly when a new CPU is
brought up)


> +static void paravirt_boot_secondary(int cpu, struct task_struct *idle)
> +{
> +	paravirt_smp_gp[cpu] = (unsigned long)(task_thread_info(idle));

spurious brackets around task_thread_info(idle)

> +	wmb();

Wouldn't smp_wmb() be more accurate?

> +	paravirt_smp_sp[cpu] = __KSTK_TOS(idle);
> +	mb();

is this barrier necessary?

> diff --git a/arch/mips/paravirt/serial.c b/arch/mips/paravirt/serial.c
> new file mode 100644
> index 0000000..e3f98b2
> --- /dev/null
> +++ b/arch/mips/paravirt/serial.c
> @@ -0,0 +1,38 @@
> +/*
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * Copyright (C) 2013 Cavium, Inc.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/virtio_console.h>
> +
> +#include <asm/mipsregs.h>
> +
> +/*
> + * Emit one character to the boot console.
> + */
> +int prom_putchar(char c)
> +{
> +	hypcall3(0 /* Console output */, 0 /*  port 0 */, (unsigned long)&c, 1 /* len == 1 */);

I think the hypcall API needs to be clearly specified and Documented
somewhere along with its HYPCALL codes and scope. I.e. is it specific to
kvmtool, or attempting to be a standard API across MIPS hypervisors.

It probably should have nice definitions in a header and wrappers
somewhere to make the arguments explicit and so there's no need for the
comments explaining what the magic values mean.

Cheers
James