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

[Andreas Herrmann <andreas.herrmann@caviumnetworks.com>]

On Wed, May 21, 2014 at 02:39:00PM +0100, James Hogan wrote:
> On 20/05/14 15:47, Andreas Herrmann wrote:

 -- 8< --

> > 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?

Yes.

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

 -- 8< --

> > +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?

Hmm, no, if we can't modify CPU affinity we shouldn't provide it.

> > +
> > +static void irq_pci_cpu_offline(struct irq_data *data)
> > +{
> > +}
> 
> Or this one?

No.

> > +
> > +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,
> > +};

 -- 8< --

> > +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)

Ok.

> > +	wmb();
 
 -- 8< --

> > +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.

Agreed. I think when the definitions are moved to kvm_para.h,
appropriate macros for the hypercall numbers will also be provided
etc.


Andreas