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

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

On 21/05/14 17:31, David Daney wrote:
>>> +    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.

Okay, fair enough. I suggest adding a comment to that effect (I think
checkpatch now complains about barriers without comments :) ).

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

I don't think making it global should be a problem (sounds ideal if it
works without changes on multiple hypervisors), but it probably makes
sense to ensure that other stakeholders are aware of it (those working
on other hypervisors and semihosting stuff).

Cheers
James