From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Daney Subject: Re: [PATCH 10/15] MIPS: Add code for new system 'paravirt'. Date: Wed, 21 May 2014 09:31:02 -0700 Message-ID: <537CD4C6.5080905@caviumnetworks.com> References: <1400597236-11352-1-git-send-email-andreas.herrmann@caviumnetworks.com> <1400597236-11352-11-git-send-email-andreas.herrmann@caviumnetworks.com> <537CAC74.4030800@imgtec.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Cc: Andreas Herrmann , , David Daney , "Ralf Baechle" , , David Daney To: James Hogan Return-path: Received: from mail-bn1lp0139.outbound.protection.outlook.com ([207.46.163.139]:19144 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751801AbaEUQbS (ORCPT ); Wed, 21 May 2014 12:31:18 -0400 In-Reply-To: <537CAC74.4030800@imgtec.com> Sender: kvm-owner@vger.kernel.org List-ID: On 05/21/2014 06:39 AM, James Hogan wrote: [...] >> 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 [...] > >> +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? > Yes, but... >> + */ >> + 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); >> + ... This code is not correct. It was initially done as a workaround for the issues fixed in commit 202da4005. Now that on_each_cpu() is less buggy, we can unconditionally use it and the assertion above about "Interrupts are already disabled" will be true. >> + 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? I guess we can add them only when they are implemented. > >> + >> +static void irq_pci_cpu_offline(struct irq_data *data) >> +{ >> +} > > Or this one? Same. > >> + >> +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? Probably. > >> + paravirt_smp_sp[cpu] = __KSTK_TOS(idle); >> + mb(); > > is this barrier necessary? Really it is just make_writes_visible_asap(), but for OCTEON mb() or smp_wmb() is the closest that the kernel has. It may not be necessary, but it doesn't really harm anything. > >> 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 >> +#include >> + >> +#include >> + >> +/* >> + * 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. > I was intending it to be the later. (standard API across MIPS hypervisors.) The idea being that the first argument would be broken up into several ranges. 0..x : Globally available HYPCALL provided by all hypervisors. m..n : MIPS KVM specific. y..z : Reserved for the vendor. For some values of x, m, n, y and z. But perhaps it should just be MIPS KVM specific. If making it global is too much trouble. > 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 >