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
