[PATCH] spapr: Add "memop" hypercall

[PATCH] spapr: Add "memop" hypercall

[Benjamin Herrenschmidt]

This adds a kvm-specific hypervisor call to the pseries machine
which allows to do what amounts to memmove, memcpy and xor over
regions of physical memory such as the framebuffer.

This is the simplest way to get usable framebuffer speed from
SLOF since the framebuffer isn't mapped in the VRMA and so would
otherwise require an hcall per 8 bytes access.

The performance is still not great but usable, and can be improved
with a more complex implementation of the hcall itself if needed.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 hw/spapr.h       |    3 ++-
 hw/spapr_hcall.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/hw/spapr.h b/hw/spapr.h
index 7c497aa..0343f33 100644
--- a/hw/spapr.h
+++ b/hw/spapr.h
@@ -264,7 +264,8 @@ typedef struct sPAPREnvironment {
  */
 #define KVMPPC_HCALL_BASE       0xf000
 #define KVMPPC_H_RTAS           (KVMPPC_HCALL_BASE + 0x0)
-#define KVMPPC_HCALL_MAX        KVMPPC_H_RTAS
+#define KVMPPC_H_LOGICAL_MEMOP  (KVMPPC_HCALL_BASE + 0x1)
+#define KVMPPC_HCALL_MAX        KVMPPC_H_LOGICAL_MEMOP
 
 extern sPAPREnvironment *spapr;
 
diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c
index 94bb504..c5c26dc 100644
--- a/hw/spapr_hcall.c
+++ b/hw/spapr_hcall.c
@@ -608,6 +608,54 @@ static target_ulong h_logical_store(CPUPPCState *env, sPAPREnvironment *spapr,
     return H_PARAMETER;
 }
 
+static target_ulong h_logical_memop(CPUPPCState *env, sPAPREnvironment *spapr,
+				    target_ulong opcode, target_ulong *args)
+{
+    target_ulong dst   = args[0]; /* Destination address */
+    target_ulong src   = args[1]; /* Source address */
+    target_ulong esize = args[2]; /* Element size (0=1,1=2,2=4,3=8) */
+    target_ulong count = args[3]; /* Element count */
+    target_ulong op    = args[4]; /* 0 = copy, 1 = invert */
+    uint64_t tmp;
+    unsigned int mask = (1 << esize) - 1;
+    int step = 1 << esize;
+
+    if (count > 0x80000000)
+	return H_PARAMETER;
+
+    if ((dst & mask) || (src & mask))
+	return H_PARAMETER;
+
+    if (dst >= src && dst < (src + (count << esize))) {
+	    dst = dst + ((count - 1) << esize);
+	    src = src + ((count - 1) << esize);
+	    step = -step;
+    }
+
+    while(count--) {
+        switch (esize) {
+        case 0: tmp = ldub_phys(src); break;
+        case 1: tmp = lduw_phys(src); break;
+        case 2: tmp = ldl_phys(src);  break;
+        case 3: tmp = ldq_phys(src);  break;
+        default:
+        return H_PARAMETER;
+	}
+        if (op)
+		tmp = ~tmp;
+        switch (esize) {
+        case 0: stb_phys(dst, tmp); break;
+        case 1: stw_phys(dst, tmp); break;
+        case 2: stl_phys(dst, tmp); break;
+        case 3: stq_phys(dst, tmp); break;
+	}
+	dst = dst + step;
+	src = src + step;
+    }
+
+    return H_SUCCESS;
+}
+
 static target_ulong h_logical_icbi(CPUPPCState *env, sPAPREnvironment *spapr,
                                    target_ulong opcode, target_ulong *args)
 {
@@ -700,6 +748,7 @@ static void hypercall_register_types(void)
     spapr_register_hypercall(H_LOGICAL_CACHE_STORE, h_logical_store);
     spapr_register_hypercall(H_LOGICAL_ICBI, h_logical_icbi);
     spapr_register_hypercall(H_LOGICAL_DCBF, h_logical_dcbf);
+    spapr_register_hypercall(KVMPPC_H_LOGICAL_MEMOP, h_logical_memop);
 
     /* qemu/KVM-PPC specific hcalls */
     spapr_register_hypercall(KVMPPC_H_RTAS, h_rtas);

Re: [PATCH] spapr: Add "memop" hypercall

[Alexander Graf]

On 21.05.2012, at 09:24, Benjamin Herrenschmidt wrote:

> This adds a kvm-specific hypervisor call to the pseries machine
> which allows to do what amounts to memmove, memcpy and xor over
> regions of physical memory such as the framebuffer.
> 
> This is the simplest way to get usable framebuffer speed from
> SLOF since the framebuffer isn't mapped in the VRMA and so would
> otherwise require an hcall per 8 bytes access.
> 
> The performance is still not great but usable, and can be improved
> with a more complex implementation of the hcall itself if needed.

Shouldn't we try and implement the same thing in QEMU as well to make things compatible? Also, what would the downside be of always going through QEMU for this hypercall?

The reason I'm asking is that we might want to do memmove,memcpy,xor on MMIO memory, which then user space could easily do, but which incurs quite some headaches to do it from inside KVM.


Alex

> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> hw/spapr.h       |    3 ++-
> hw/spapr_hcall.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/spapr.h b/hw/spapr.h
> index 7c497aa..0343f33 100644
> --- a/hw/spapr.h
> +++ b/hw/spapr.h
> @@ -264,7 +264,8 @@ typedef struct sPAPREnvironment {
>  */
> #define KVMPPC_HCALL_BASE       0xf000
> #define KVMPPC_H_RTAS           (KVMPPC_HCALL_BASE + 0x0)
> -#define KVMPPC_HCALL_MAX        KVMPPC_H_RTAS
> +#define KVMPPC_H_LOGICAL_MEMOP  (KVMPPC_HCALL_BASE + 0x1)
> +#define KVMPPC_HCALL_MAX        KVMPPC_H_LOGICAL_MEMOP
> 
> extern sPAPREnvironment *spapr;
> 
> diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c
> index 94bb504..c5c26dc 100644
> --- a/hw/spapr_hcall.c
> +++ b/hw/spapr_hcall.c
> @@ -608,6 +608,54 @@ static target_ulong h_logical_store(CPUPPCState *env, sPAPREnvironment *spapr,
>     return H_PARAMETER;
> }
> 
> +static target_ulong h_logical_memop(CPUPPCState *env, sPAPREnvironment *spapr,
> +				    target_ulong opcode, target_ulong *args)
> +{
> +    target_ulong dst   = args[0]; /* Destination address */
> +    target_ulong src   = args[1]; /* Source address */
> +    target_ulong esize = args[2]; /* Element size (0=1,1=2,2=4,3=8) */
> +    target_ulong count = args[3]; /* Element count */
> +    target_ulong op    = args[4]; /* 0 = copy, 1 = invert */
> +    uint64_t tmp;
> +    unsigned int mask = (1 << esize) - 1;
> +    int step = 1 << esize;
> +
> +    if (count > 0x80000000)
> +	return H_PARAMETER;
> +
> +    if ((dst & mask) || (src & mask))
> +	return H_PARAMETER;
> +
> +    if (dst >= src && dst < (src + (count << esize))) {
> +	    dst = dst + ((count - 1) << esize);
> +	    src = src + ((count - 1) << esize);
> +	    step = -step;
> +    }
> +
> +    while(count--) {
> +        switch (esize) {
> +        case 0: tmp = ldub_phys(src); break;
> +        case 1: tmp = lduw_phys(src); break;
> +        case 2: tmp = ldl_phys(src);  break;
> +        case 3: tmp = ldq_phys(src);  break;
> +        default:
> +        return H_PARAMETER;
> +	}
> +        if (op)
> +		tmp = ~tmp;
> +        switch (esize) {
> +        case 0: stb_phys(dst, tmp); break;
> +        case 1: stw_phys(dst, tmp); break;
> +        case 2: stl_phys(dst, tmp); break;
> +        case 3: stq_phys(dst, tmp); break;
> +	}
> +	dst = dst + step;
> +	src = src + step;
> +    }
> +
> +    return H_SUCCESS;
> +}
> +
> static target_ulong h_logical_icbi(CPUPPCState *env, sPAPREnvironment *spapr,
>                                    target_ulong opcode, target_ulong *args)
> {
> @@ -700,6 +748,7 @@ static void hypercall_register_types(void)
>     spapr_register_hypercall(H_LOGICAL_CACHE_STORE, h_logical_store);
>     spapr_register_hypercall(H_LOGICAL_ICBI, h_logical_icbi);
>     spapr_register_hypercall(H_LOGICAL_DCBF, h_logical_dcbf);
> +    spapr_register_hypercall(KVMPPC_H_LOGICAL_MEMOP, h_logical_memop);
> 
>     /* qemu/KVM-PPC specific hcalls */
>     spapr_register_hypercall(KVMPPC_H_RTAS, h_rtas);
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spapr: Add "memop" hypercall

[Benjamin Herrenschmidt]

On Mon, 2012-05-21 at 10:38 +0200, Alexander Graf wrote:
> On 21.05.2012, at 09:24, Benjamin Herrenschmidt wrote:
> 
> > This adds a kvm-specific hypervisor call to the pseries machine
> > which allows to do what amounts to memmove, memcpy and xor over
> > regions of physical memory such as the framebuffer.
> > 
> > This is the simplest way to get usable framebuffer speed from
> > SLOF since the framebuffer isn't mapped in the VRMA and so would
> > otherwise require an hcall per 8 bytes access.
> > 
> > The performance is still not great but usable, and can be improved
> > with a more complex implementation of the hcall itself if needed.
> 
> Shouldn't we try and implement the same thing in QEMU as well to make
> things compatible? Also, what would the downside be of always going
> through QEMU for this hypercall?
> 
> The reason I'm asking is that we might want to do memmove,memcpy,xor
> on MMIO memory, which then user space could easily do, but which
> incurs quite some headaches to do it from inside KVM.

I don't understand your question ... this is implemented in qemu...

The problem with SLOF is specific to -M pseries, because it runs in
"guest" real mode, it doesn't have access to device memory unless it
does it via hcalls.

Cheers,
Ben.

> 
> Alex
> 
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> > hw/spapr.h       |    3 ++-
> > hw/spapr_hcall.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 51 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/spapr.h b/hw/spapr.h
> > index 7c497aa..0343f33 100644
> > --- a/hw/spapr.h
> > +++ b/hw/spapr.h
> > @@ -264,7 +264,8 @@ typedef struct sPAPREnvironment {
> >  */
> > #define KVMPPC_HCALL_BASE       0xf000
> > #define KVMPPC_H_RTAS           (KVMPPC_HCALL_BASE + 0x0)
> > -#define KVMPPC_HCALL_MAX        KVMPPC_H_RTAS
> > +#define KVMPPC_H_LOGICAL_MEMOP  (KVMPPC_HCALL_BASE + 0x1)
> > +#define KVMPPC_HCALL_MAX        KVMPPC_H_LOGICAL_MEMOP
> > 
> > extern sPAPREnvironment *spapr;
> > 
> > diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c
> > index 94bb504..c5c26dc 100644
> > --- a/hw/spapr_hcall.c
> > +++ b/hw/spapr_hcall.c
> > @@ -608,6 +608,54 @@ static target_ulong h_logical_store(CPUPPCState *env, sPAPREnvironment *spapr,
> >     return H_PARAMETER;
> > }
> > 
> > +static target_ulong h_logical_memop(CPUPPCState *env, sPAPREnvironment *spapr,
> > +				    target_ulong opcode, target_ulong *args)
> > +{
> > +    target_ulong dst   = args[0]; /* Destination address */
> > +    target_ulong src   = args[1]; /* Source address */
> > +    target_ulong esize = args[2]; /* Element size (0=1,1=2,2=4,3=8) */
> > +    target_ulong count = args[3]; /* Element count */
> > +    target_ulong op    = args[4]; /* 0 = copy, 1 = invert */
> > +    uint64_t tmp;
> > +    unsigned int mask = (1 << esize) - 1;
> > +    int step = 1 << esize;
> > +
> > +    if (count > 0x80000000)
> > +	return H_PARAMETER;
> > +
> > +    if ((dst & mask) || (src & mask))
> > +	return H_PARAMETER;
> > +
> > +    if (dst >= src && dst < (src + (count << esize))) {
> > +	    dst = dst + ((count - 1) << esize);
> > +	    src = src + ((count - 1) << esize);
> > +	    step = -step;
> > +    }
> > +
> > +    while(count--) {
> > +        switch (esize) {
> > +        case 0: tmp = ldub_phys(src); break;
> > +        case 1: tmp = lduw_phys(src); break;
> > +        case 2: tmp = ldl_phys(src);  break;
> > +        case 3: tmp = ldq_phys(src);  break;
> > +        default:
> > +        return H_PARAMETER;
> > +	}
> > +        if (op)
> > +		tmp = ~tmp;
> > +        switch (esize) {
> > +        case 0: stb_phys(dst, tmp); break;
> > +        case 1: stw_phys(dst, tmp); break;
> > +        case 2: stl_phys(dst, tmp); break;
> > +        case 3: stq_phys(dst, tmp); break;
> > +	}
> > +	dst = dst + step;
> > +	src = src + step;
> > +    }
> > +
> > +    return H_SUCCESS;
> > +}
> > +
> > static target_ulong h_logical_icbi(CPUPPCState *env, sPAPREnvironment *spapr,
> >                                    target_ulong opcode, target_ulong *args)
> > {
> > @@ -700,6 +748,7 @@ static void hypercall_register_types(void)
> >     spapr_register_hypercall(H_LOGICAL_CACHE_STORE, h_logical_store);
> >     spapr_register_hypercall(H_LOGICAL_ICBI, h_logical_icbi);
> >     spapr_register_hypercall(H_LOGICAL_DCBF, h_logical_dcbf);
> > +    spapr_register_hypercall(KVMPPC_H_LOGICAL_MEMOP, h_logical_memop);
> > 
> >     /* qemu/KVM-PPC specific hcalls */
> >     spapr_register_hypercall(KVMPPC_H_RTAS, h_rtas);
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spapr: Add "memop" hypercall

[Alexander Graf]

On 21.05.2012, at 10:59, Benjamin Herrenschmidt wrote:

> On Mon, 2012-05-21 at 10:38 +0200, Alexander Graf wrote:
>> On 21.05.2012, at 09:24, Benjamin Herrenschmidt wrote:
>> 
>>> This adds a kvm-specific hypervisor call to the pseries machine
>>> which allows to do what amounts to memmove, memcpy and xor over
>>> regions of physical memory such as the framebuffer.
>>> 
>>> This is the simplest way to get usable framebuffer speed from
>>> SLOF since the framebuffer isn't mapped in the VRMA and so would
>>> otherwise require an hcall per 8 bytes access.
>>> 
>>> The performance is still not great but usable, and can be improved
>>> with a more complex implementation of the hcall itself if needed.
>> 
>> Shouldn't we try and implement the same thing in QEMU as well to make
>> things compatible? Also, what would the downside be of always going
>> through QEMU for this hypercall?
>> 
>> The reason I'm asking is that we might want to do memmove,memcpy,xor
>> on MMIO memory, which then user space could easily do, but which
>> incurs quite some headaches to do it from inside KVM.
> 
> I don't understand your question ... this is implemented in qemu...
> 
> The problem with SLOF is specific to -M pseries, because it runs in
> "guest" real mode, it doesn't have access to device memory unless it
> does it via hcalls.

Yikes. Call me stupid.


Alex

Re: [PATCH] spapr: Add "memop" hypercall

[Avi Kivity]

On 05/21/2012 10:24 AM, Benjamin Herrenschmidt wrote:
> This adds a kvm-specific hypervisor call to the pseries machine
> which allows to do what amounts to memmove, memcpy and xor over
> regions of physical memory such as the framebuffer.
>
> This is the simplest way to get usable framebuffer speed from
> SLOF since the framebuffer isn't mapped in the VRMA and so would
> otherwise require an hcall per 8 bytes access.
>
> The performance is still not great but usable, and can be improved
> with a more complex implementation of the hcall itself if needed.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  hw/spapr.h       |    3 ++-
>  hw/spapr_hcall.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+), 1 deletion(-)
>

Shouldn't these be documented somewhere?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] spapr: Add "memop" hypercall

[Benjamin Herrenschmidt]

On Mon, 2012-05-21 at 12:47 +0300, Avi Kivity wrote:
> On 05/21/2012 10:24 AM, Benjamin Herrenschmidt wrote:
> > This adds a kvm-specific hypervisor call to the pseries machine
> > which allows to do what amounts to memmove, memcpy and xor over
> > regions of physical memory such as the framebuffer.
> >
> > This is the simplest way to get usable framebuffer speed from
> > SLOF since the framebuffer isn't mapped in the VRMA and so would
> > otherwise require an hcall per 8 bytes access.
> >
> > The performance is still not great but usable, and can be improved
> > with a more complex implementation of the hcall itself if needed.
> >
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> >  hw/spapr.h       |    3 ++-
> >  hw/spapr_hcall.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 51 insertions(+), 1 deletion(-)
> >
> 
> Shouldn't these be documented somewhere?

Hrm, that's a good point. So far we've been mostly implementing the PAPR
spec so the documentation exists.

Before that patch we only had one "special" hcall not in PAPR, which we
use for the RTAS firmware calls (this part of the FW normally lives
inside the guest on real pHyp and communicates with the hypervisor using
private hcalls, on qemu, we just turn all the RTAS calls to qemu via a
single H_RTAS multiplexer). We haven't documented it.

Now I'm adding another one, so yes, it's looking like a trend :-) I'll
look into it, at this stage with only those two, adding some comments in
the header might be plenty enough.

Cheers,
Ben.

Re: [PATCH] spapr: Add "memop" hypercall

[Avi Kivity]

On 05/21/2012 01:04 PM, Benjamin Herrenschmidt wrote:
> On Mon, 2012-05-21 at 12:47 +0300, Avi Kivity wrote:
> > On 05/21/2012 10:24 AM, Benjamin Herrenschmidt wrote:
> > > This adds a kvm-specific hypervisor call to the pseries machine
> > > which allows to do what amounts to memmove, memcpy and xor over
> > > regions of physical memory such as the framebuffer.
> > >
> > > This is the simplest way to get usable framebuffer speed from
> > > SLOF since the framebuffer isn't mapped in the VRMA and so would
> > > otherwise require an hcall per 8 bytes access.
> > >
> > > The performance is still not great but usable, and can be improved
> > > with a more complex implementation of the hcall itself if needed.
> > >
> > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > > ---
> > >  hw/spapr.h       |    3 ++-
> > >  hw/spapr_hcall.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 51 insertions(+), 1 deletion(-)
> > >
> > 
> > Shouldn't these be documented somewhere?
>
> Hrm, that's a good point. So far we've been mostly implementing the PAPR
> spec so the documentation exists.
>
> Before that patch we only had one "special" hcall not in PAPR, which we
> use for the RTAS firmware calls (this part of the FW normally lives
> inside the guest on real pHyp and communicates with the hypervisor using
> private hcalls, on qemu, we just turn all the RTAS calls to qemu via a
> single H_RTAS multiplexer). We haven't documented it.
>
> Now I'm adding another one, so yes, it's looking like a trend :-) I'll
> look into it, at this stage with only those two, adding some comments in
> the header might be plenty enough.

Documentation/virtual/kvm/ppc-pv.txt is a nice central place for these. 
It would be even better if you could add them to the spec.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] spapr: Add "memop" hypercall

[Benjamin Herrenschmidt]

On Mon, 2012-05-21 at 13:07 +0300, Avi Kivity wrote:
> > Now I'm adding another one, so yes, it's looking like a trend :-) I'll
> > look into it, at this stage with only those two, adding some comments in
> > the header might be plenty enough.
> 
> Documentation/virtual/kvm/ppc-pv.txt is a nice central place for these. 
> It would be even better if you could add them to the spec.

They don't quite fit with the other PV calls in there which use a
different HV calling mechanism alltogether, but I can certainly add a
specific section.

As to adding things to PAPR, let's assume for now that this isn't
possible :-) Besides, those two hypercalls are pretty specific to the
way things are implemented in qemu and are in both case more or less
private mechanisms used to communicate between qemu and the SLOF
firmware we run in inside qemu, they aren't really used by random guest
SW and aren't meant to be.

It's a bit like if you had private calls between seabios and qemu...

Cheers,
Ben.

Re: [PATCH] spapr: Add "memop" hypercall

[Avi Kivity]

On 05/21/2012 02:48 PM, Benjamin Herrenschmidt wrote:
> On Mon, 2012-05-21 at 13:07 +0300, Avi Kivity wrote:
> > > Now I'm adding another one, so yes, it's looking like a trend :-) I'll
> > > look into it, at this stage with only those two, adding some comments in
> > > the header might be plenty enough.
> > 
> > Documentation/virtual/kvm/ppc-pv.txt is a nice central place for these. 
> > It would be even better if you could add them to the spec.
>
> They don't quite fit with the other PV calls in there which use a
> different HV calling mechanism alltogether, but I can certainly add a
> specific section.
>
> As to adding things to PAPR, let's assume for now that this isn't
> possible :-) Besides, those two hypercalls are pretty specific to the
> way things are implemented in qemu and are in both case more or less
> private mechanisms used to communicate between qemu and the SLOF
> firmware we run in inside qemu, they aren't really used by random guest
> SW and aren't meant to be.

Okay.  But let's have a spec, even a kvm-private one, and then an
implementation of that spec, instead of an implementation and some
documentation added as an afterthought (or not).

> It's a bit like if you had private calls between seabios and qemu...

We document those too.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] spapr: Add "memop" hypercall

[Benjamin Herrenschmidt]

On Mon, 2012-05-21 at 16:55 +0300, Avi Kivity wrote:
> 
> Okay.  But let's have a spec, even a kvm-private one, and then an
> implementation of that spec, instead of an implementation and some
> documentation added as an afterthought (or not).
> 
> > It's a bit like if you had private calls between seabios and qemu...
> 
> We document those too.

Well, we have the implementation so in any case the implementation
-will- be an afterthought :-) Anyways, I'll look into adding that to the
patch.

Cheers,
Ben.

Re: [PATCH] spapr: Add "memop" hypercall

[Benjamin Herrenschmidt]

On Mon, 2012-05-21 at 16:55 +0300, Avi Kivity wrote:
> 
> > As to adding things to PAPR, let's assume for now that this isn't
> > possible :-) Besides, those two hypercalls are pretty specific to
> the
> > way things are implemented in qemu and are in both case more or less
> > private mechanisms used to communicate between qemu and the SLOF
> > firmware we run in inside qemu, they aren't really used by random
> guest
> > SW and aren't meant to be.
> 
> Okay.  But let's have a spec, even a kvm-private one, and then an
> implementation of that spec, instead of an implementation and some
> documentation added as an afterthought (or not).
> 
> > It's a bit like if you had private calls between seabios and qemu...
> 
> We document those too.

BTW. This is a qemu patch, and that hypercall isn't KVM related at all,
ie, it's implemented in qemu and is used with or without KVM, so
documenting it in the kernel tree makes little sense. Same goes with
H_RTAS.

I'll add a doc to qemu in my next spin of it.

Cheers,
Ben.

Re: [PATCH] spapr: Add "memop" hypercall

[Avi Kivity]

On 05/25/2012 06:12 AM, Benjamin Herrenschmidt wrote:
> 
> BTW. This is a qemu patch, and that hypercall isn't KVM related at all,
> ie, it's implemented in qemu and is used with or without KVM, so
> documenting it in the kernel tree makes little sense. Same goes with
> H_RTAS.
> 
> I'll add a doc to qemu in my next spin of it.
> 

Depends.  How do you detect it exists?  Are you detecting kvm, or qemu,
or the hypercall itself?

I'd hate us to find ourselves in a maze of disconnected documentation
with no clear guidelines on when a feature is available and when it is not.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] spapr: Add "memop" hypercall

[Alexander Graf]

On 28.05.2012, at 12:40, Avi Kivity wrote:

> On 05/25/2012 06:12 AM, Benjamin Herrenschmidt wrote:
>> 
>> BTW. This is a qemu patch, and that hypercall isn't KVM related at all,
>> ie, it's implemented in qemu and is used with or without KVM, so
>> documenting it in the kernel tree makes little sense. Same goes with
>> H_RTAS.
>> 
>> I'll add a doc to qemu in my next spin of it.
>> 
> 
> Depends.  How do you detect it exists?  Are you detecting kvm, or qemu,
> or the hypercall itself?

The hypercall itself. SLOF is the only user. QEMU provides SLOF. SLOF calls the hypercall. If the hypercall returns "I don't exist", it doesn't exist. :)

> I'd hate us to find ourselves in a maze of disconnected documentation
> with no clear guidelines on when a feature is available and when it is not.

Yeah, but semantically these hypercalls are on the same layer as fw_cfg. So they clearly belong to QEMU. In fact, they're also used when running with emulation.


Alex

Re: [PATCH] spapr: Add "memop" hypercall

[Benjamin Herrenschmidt]

On Mon, 2012-05-28 at 13:40 +0300, Avi Kivity wrote:
> Depends.  How do you detect it exists?  Are you detecting kvm, or qemu,
> or the hypercall itself?
> 
> I'd hate us to find ourselves in a maze of disconnected documentation
> with no clear guidelines on when a feature is available and when it is not.

At the moment SLOF just "uses it" when using the frame buffer. We could
advertise its presence via the device-tree, there's already stuff there
to expose what hypercalls or set of hypecalls are implemented for PAPR,
we could add qemu specific extensions.

Cheers,
Ben.

[PATCH v2] spapr: Add "memop" hypercall

[Benjamin Herrenschmidt]

This adds a qemu-specific hypervisor call to the pseries machine
which allows to do what amounts to memmove, memcpy and xor over
regions of physical memory such as the framebuffer.

This is the simplest way to get usable framebuffer speed from
SLOF since the framebuffer isn't mapped in the VRMA and so would
otherwise require an hcall per 8 bytes access.

The performance is still not great but usable, and can be improved
with a more complex implementation of the hcall itself if needed.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

v2: - Added documentation for our private hcalls
    - Fixed coding style issues

 docs/specs/ppc-spapr-hcalls.txt |   78 +++++++++++++++++++++++++++++++++++++++
 hw/spapr.h                      |    3 +-
 hw/spapr_hcall.c                |   52 ++++++++++++++++++++++++++
 3 files changed, 132 insertions(+), 1 deletions(-)
 create mode 100644 docs/specs/ppc-spapr-hcalls.txt

diff --git a/docs/specs/ppc-spapr-hcalls.txt b/docs/specs/ppc-spapr-hcalls.txt
new file mode 100644
index 0000000..4b3fa9a
--- /dev/null
+++ b/docs/specs/ppc-spapr-hcalls.txt
@@ -0,0 +1,78 @@
+When used with the "pseries" machine type, qemu-system-ppc64 implement
+a set of hypervisor calls using a subset of the server "PAPR" specification
+(IBM internal at this point), which is also what IBM proprietary hypervisor
+adheres too.
+
+The subset is selected based on the requirements of Linux as a guest.
+
+In addition to those calls, we have added our own private hypervisor
+calls which are mostly used as a private interface between the firmware
+running in the guest and qemu.
+
+All those hypercalls start at hcall number 0xf000 which correspond
+to a implementation specific range in PAPR.
+
+- H_RTAS (0xf000)
+
+RTAS is a set of runtime services generally provided by the firmware
+inside the guest to the operating system. It predates the existence
+of hypervisors (it was originally an extension to Open Firmware) and
+is still used by PAPR to provide various services that aren't performance
+sensitive.
+
+We currently implement the RTAS services in qemu itself. The actual RTAS
+"firmware" blob in the guest is a small stub of a few instructions which
+calls our private H_RTAS hypervisor call to pass the RTAS calls to qemu.
+
+Arguments:
+
+  r3 : H_RTAS (0xf000)
+  r4 : Guest physical address of RTAS parameter block
+
+Returns:
+
+  H_SUCCESS   : Successully called the RTAS function (RTAS result
+                will have been stored in the parameter block)
+  H_PARAMETER : Unknown token
+
+- H_LOGICAL_MEMOP (0xf001)
+
+When the guest runs in "real mode" (in powerpc lingua this means
+with MMU disabled, ie guest effective = guest physical), it only
+has access to a subset of memory and no IOs.
+
+PAPR provides a set of hypervisor calls to perform cachable or
+non-cachable accesses to any guest physical addresses that the
+guest can use in order to access IO devices while in real mode.
+
+This is typically used by the firmware running in the guest.
+
+However, doing a hypercall for each access is extremely inefficient
+(even more so when running KVM) when accessing the frame buffer. In
+that case, things like scrolling become unusably slow.
+
+This hypercall allows the guest to request a "memory op" to be applied
+to memory. The supported memory ops at this point are to copy a range
+of memory (supports overlap of source and destination) and XOR which
+is used by our SLOF firmware to invert the screen.
+
+Arguments:
+
+  r3: H_LOGICAL_MEMOP (0xf001)
+  r4: Guest physical address of destination
+  r5: Guest physical address of source
+  r6: Individual element size
+        0 = 1 byte
+        1 = 2 bytes
+        2 = 4 bytes
+        3 = 8 bytes
+  r7: Number of elements
+  r8: Operation
+        0 = copy
+        1 = xor
+
+Returns:
+
+  H_SUCCESS   : Success
+  H_PARAMETER : Invalid argument
+
diff --git a/hw/spapr.h b/hw/spapr.h
index 7c497aa..0343f33 100644
--- a/hw/spapr.h
+++ b/hw/spapr.h
@@ -264,7 +264,8 @@ typedef struct sPAPREnvironment {
  */
 #define KVMPPC_HCALL_BASE       0xf000
 #define KVMPPC_H_RTAS           (KVMPPC_HCALL_BASE + 0x0)
-#define KVMPPC_HCALL_MAX        KVMPPC_H_RTAS
+#define KVMPPC_H_LOGICAL_MEMOP  (KVMPPC_HCALL_BASE + 0x1)
+#define KVMPPC_HCALL_MAX        KVMPPC_H_LOGICAL_MEMOP
 
 extern sPAPREnvironment *spapr;
 
diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c
index 94bb504..5211364 100644
--- a/hw/spapr_hcall.c
+++ b/hw/spapr_hcall.c
@@ -608,6 +608,57 @@ static target_ulong h_logical_store(CPUPPCState *env, sPAPREnvironment *spapr,
     return H_PARAMETER;
 }
 
+static target_ulong h_logical_memop(CPUPPCState *env, sPAPREnvironment *spapr,
+                                    target_ulong opcode, target_ulong *args)
+{
+    target_ulong dst   = args[0]; /* Destination address */
+    target_ulong src   = args[1]; /* Source address */
+    target_ulong esize = args[2]; /* Element size (0=1,1=2,2=4,3=8) */
+    target_ulong count = args[3]; /* Element count */
+    target_ulong op    = args[4]; /* 0 = copy, 1 = invert */
+    uint64_t tmp;
+    unsigned int mask = (1 << esize) - 1;
+    int step = 1 << esize;
+
+    if (count > 0x80000000) {
+	return H_PARAMETER;
+    }
+
+    if ((dst & mask) || (src & mask)) {
+        return H_PARAMETER;
+    }
+
+    if (dst >= src && dst < (src + (count << esize))) {
+            dst = dst + ((count - 1) << esize);
+            src = src + ((count - 1) << esize);
+            step = -step;
+    }
+
+    while (count--) {
+        switch (esize) {
+        case 0: tmp = ldub_phys(src); break;
+        case 1: tmp = lduw_phys(src); break;
+        case 2: tmp = ldl_phys(src);  break;
+        case 3: tmp = ldq_phys(src);  break;
+        default:
+        return H_PARAMETER;
+        }
+        if (op) {
+            tmp = ~tmp;
+        }
+        switch (esize) {
+        case 0: stb_phys(dst, tmp); break;
+        case 1: stw_phys(dst, tmp); break;
+        case 2: stl_phys(dst, tmp); break;
+        case 3: stq_phys(dst, tmp); break;
+        }
+        dst = dst + step;
+        src = src + step;
+    }
+
+    return H_SUCCESS;
+}
+
 static target_ulong h_logical_icbi(CPUPPCState *env, sPAPREnvironment *spapr,
                                    target_ulong opcode, target_ulong *args)
 {
@@ -700,6 +751,7 @@ static void hypercall_register_types(void)
     spapr_register_hypercall(H_LOGICAL_CACHE_STORE, h_logical_store);
     spapr_register_hypercall(H_LOGICAL_ICBI, h_logical_icbi);
     spapr_register_hypercall(H_LOGICAL_DCBF, h_logical_dcbf);
+    spapr_register_hypercall(KVMPPC_H_LOGICAL_MEMOP, h_logical_memop);
 
     /* qemu/KVM-PPC specific hcalls */
     spapr_register_hypercall(KVMPPC_H_RTAS, h_rtas);

Re: [PATCH v2] spapr: Add "memop" hypercall

[Alexander Graf]

On 25.05.2012, at 05:53, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> This adds a qemu-specific hypervisor call to the pseries machine
> which allows to do what amounts to memmove, memcpy and xor over
> regions of physical memory such as the framebuffer.
> 
> This is the simplest way to get usable framebuffer speed from
> SLOF since the framebuffer isn't mapped in the VRMA and so would
> otherwise require an hcall per 8 bytes access.
> 
> The performance is still not great but usable, and can be improved
> with a more complex implementation of the hcall itself if needed.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> 
> v2: - Added documentation for our private hcalls

Very cool :). This really does belong to QEMU's documentation, as these hypercall are basically private SLOF <-> QEMU backdoor interface.

>    - Fixed coding style issues
> 
> docs/specs/ppc-spapr-hcalls.txt |   78 +++++++++++++++++++++++++++++++++++++++
> hw/spapr.h                      |    3 +-
> hw/spapr_hcall.c                |   52 ++++++++++++++++++++++++++
> 3 files changed, 132 insertions(+), 1 deletions(-)
> create mode 100644 docs/specs/ppc-spapr-hcalls.txt
> 
> diff --git a/docs/specs/ppc-spapr-hcalls.txt b/docs/specs/ppc-spapr-hcalls.txt
> new file mode 100644
> index 0000000..4b3fa9a
> --- /dev/null
> +++ b/docs/specs/ppc-spapr-hcalls.txt
> @@ -0,0 +1,78 @@
> +When used with the "pseries" machine type, qemu-system-ppc64 implement

s

> +a set of hypervisor calls using a subset of the server "PAPR" specification
> +(IBM internal at this point), which is also what IBM

's

> proprietary hypervisor
> +adheres too.
> +
> +The subset is selected based on the requirements of Linux as a guest.
> +
> +In addition to those calls, we have added our own private hypervisor
> +calls which are mostly used as a private interface between the firmware
> +running in the guest and qemu.

QEMU

> +
> +All those hypercalls start at hcall number 0xf000 which correspond
> +to a implementation specific range in PAPR.
> +
> +- H_RTAS (0xf000)
> +
> +RTAS is a set of runtime services generally provided by the firmware
> +inside the guest to the operating system. It predates the existence
> +of hypervisors (it was originally an extension to Open Firmware) and
> +is still used by PAPR to provide various services that aren't performance
> +sensitive.
> +
> +We currently implement the RTAS services in qemu

QEMU (won't comment on it below, please just search&replace it ;))

> itself. The actual RTAS
> +"firmware" blob in the guest is a small stub of a few instructions which
> +calls our private H_RTAS hypervisor call to pass the RTAS calls to qemu.
> +
> +Arguments:
> +
> +  r3 : H_RTAS (0xf000)
> +  r4 : Guest physical address of RTAS parameter block
> +
> +Returns:
> +
> +  H_SUCCESS   : Successully called the RTAS function (RTAS result
> +                will have been stored in the parameter block)
> +  H_PARAMETER : Unknown token
> +
> +- H_LOGICAL_MEMOP (0xf001)
> +
> +When the guest runs in "real mode" (in powerpc lingua this means
> +with MMU disabled, ie guest effective = guest physical), it only
> +has access to a subset of memory and no IOs.
> +
> +PAPR provides a set of hypervisor calls to perform cachable or
> +non-cachable accesses to any guest physical addresses that the
> +guest can use in order to access IO devices while in real mode.
> +
> +This is typically used by the firmware running in the guest.
> +
> +However, doing a hypercall for each access is extremely inefficient
> +(even more so when running KVM) when accessing the frame buffer. In
> +that case, things like scrolling become unusably slow.
> +
> +This hypercall allows the guest to request a "memory op" to be applied
> +to memory. The supported memory ops at this point are to copy a range
> +of memory (supports overlap of source and destination) and XOR which
> +is used by our SLOF firmware to invert the screen.
> +
> +Arguments:
> +
> +  r3: H_LOGICAL_MEMOP (0xf001)
> +  r4: Guest physical address of destination
> +  r5: Guest physical address of source
> +  r6: Individual element size
> +        0 = 1 byte
> +        1 = 2 bytes
> +        2 = 4 bytes
> +        3 = 8 bytes
> +  r7: Number of elements
> +  r8: Operation
> +        0 = copy
> +        1 = xor
> +
> +Returns:
> +
> +  H_SUCCESS   : Success
> +  H_PARAMETER : Invalid argument
> +
> diff --git a/hw/spapr.h b/hw/spapr.h
> index 7c497aa..0343f33 100644
> --- a/hw/spapr.h
> +++ b/hw/spapr.h
> @@ -264,7 +264,8 @@ typedef struct sPAPREnvironment {
>  */
> #define KVMPPC_HCALL_BASE       0xf000
> #define KVMPPC_H_RTAS           (KVMPPC_HCALL_BASE + 0x0)
> -#define KVMPPC_HCALL_MAX        KVMPPC_H_RTAS
> +#define KVMPPC_H_LOGICAL_MEMOP  (KVMPPC_HCALL_BASE + 0x1)
> +#define KVMPPC_HCALL_MAX        KVMPPC_H_LOGICAL_MEMOP
> 
> extern sPAPREnvironment *spapr;
> 
> diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c
> index 94bb504..5211364 100644
> --- a/hw/spapr_hcall.c
> +++ b/hw/spapr_hcall.c
> @@ -608,6 +608,57 @@ static target_ulong h_logical_store(CPUPPCState *env, sPAPREnvironment *spapr,
>     return H_PARAMETER;
> }
> 
> +static target_ulong h_logical_memop(CPUPPCState *env, sPAPREnvironment *spapr,
> +                                    target_ulong opcode, target_ulong *args)
> +{
> +    target_ulong dst   = args[0]; /* Destination address */
> +    target_ulong src   = args[1]; /* Source address */
> +    target_ulong esize = args[2]; /* Element size (0=1,1=2,2=4,3=8) */
> +    target_ulong count = args[3]; /* Element count */
> +    target_ulong op    = args[4]; /* 0 = copy, 1 = invert */
> +    uint64_t tmp;
> +    unsigned int mask = (1 << esize) - 1;
> +    int step = 1 << esize;
> +
> +    if (count > 0x80000000) {
> +    return H_PARAMETER;

Indentation?

> +    }
> +
> +    if ((dst & mask) || (src & mask)) {
> +        return H_PARAMETER;
> +    }

If (op > 1) return H_PARAMETER;

> +
> +    if (dst >= src && dst < (src + (count << esize))) {
> +            dst = dst + ((count - 1) << esize);
> +            src = src + ((count - 1) << esize);
> +            step = -step;
> +    }
> +
> +    while (count--) {
> +        switch (esize) {
> +        case 0: tmp = ldub_phys(src);

I'm surprised checkpatch didn't complain here. Please do

case x:
    foo();
    break();

> break;
> +        case 1: tmp = lduw_phys(src); break;
> +        case 2: tmp = ldl_phys(src);  break;
> +        case 3: tmp = ldq_phys(src);  break;
> +        default:
> +        return H_PARAMETER;

Indentation?

> +        }
> +        if (op) {

op = 1

> +            tmp = ~tmp;
> +        }
> +        switch (esize) {
> +        case 0: stb_phys(dst, tmp); break;
> +        case 1: stw_phys(dst, tmp); break;
> +        case 2: stl_phys(dst, tmp); break;
> +        case 3: stq_phys(dst, tmp);

Same as above

Otherwise a good idea :).


Alex

> break;
> +        }
> +        dst = dst + step;
> +        src = src + step;
> +    }
> +
> +    return H_SUCCESS;
> +}
> +
> static target_ulong h_logical_icbi(CPUPPCState *env, sPAPREnvironment *spapr,
>                                    target_ulong opcode, target_ulong *args)
> {
> @@ -700,6 +751,7 @@ static void hypercall_register_types(void)
>     spapr_register_hypercall(H_LOGICAL_CACHE_STORE, h_logical_store);
>     spapr_register_hypercall(H_LOGICAL_ICBI, h_logical_icbi);
>     spapr_register_hypercall(H_LOGICAL_DCBF, h_logical_dcbf);
> +    spapr_register_hypercall(KVMPPC_H_LOGICAL_MEMOP, h_logical_memop);
> 
>     /* qemu/KVM-PPC specific hcalls */
>     spapr_register_hypercall(KVMPPC_H_RTAS, h_rtas);
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] spapr: Add "memop" hypercall

[Benjamin Herrenschmidt]

On Fri, 2012-05-25 at 10:30 +0200, Alexander Graf wrote:

> > +    while (count--) {
> > +        switch (esize) {
> > +        case 0: tmp = ldub_phys(src);
> 
> I'm surprised checkpatch didn't complain here. Please do
> 
> case x:
>     foo();
>     break();
> 
> > break;
> > +        case 1: tmp = lduw_phys(src); break;
> > +        case 2: tmp = ldl_phys(src);  break;
> > +        case 3: tmp = ldq_phys(src);  break;
> > +        default:
> > +        return H_PARAMETER;

Checkpatch absolutely complained and I decided to ignore it, seriously,
you really want to replace a nice & readable piece of code with
something that takes 3 pages and is generally gross & ugly ?

Some times, you have to ignore check patch and let sanity prevail.

Ben.

> Indentation?

Not sure what's up with identation, I had it all fixed up to please
checkpatch, maybe I screwed up the sending of the patch itself. Oh
well, I'm off to hospital on monday so that will have to wait til I'm
back (I regret you didn't make those comments on the previous iteration
of the patch though).

Ben.

Re: [PATCH v2] spapr: Add "memop" hypercall

[Alexander Graf]

On 25.05.2012, at 10:36, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Fri, 2012-05-25 at 10:30 +0200, Alexander Graf wrote:
> 
>>> +    while (count--) {
>>> +        switch (esize) {
>>> +        case 0: tmp = ldub_phys(src);
>> 
>> I'm surprised checkpatch didn't complain here. Please do
>> 
>> case x:
>>    foo();
>>    break();
>> 
>>> break;
>>> +        case 1: tmp = lduw_phys(src); break;
>>> +        case 2: tmp = ldl_phys(src);  break;
>>> +        case 3: tmp = ldq_phys(src);  break;
>>> +        default:
>>> +        return H_PARAMETER;
> 
> Checkpatch absolutely complained and I decided to ignore it, seriously,
> you really want to replace a nice & readable piece of code with
> something that takes 3 pages and is generally gross & ugly ?
> 
> Some times, you have to ignore check patch and let sanity prevail.

I'm not all that keen on coding style rules. But check out arch/powerpc/kvm/emulate.c and tell me that it's a good idea to go with this "clean" approach. If you want it really clean, put the whole chunk above into a geberic helper that allows for everyone to say "read n bytes of data with native endianness into a u64". In that code, the more verbose coding style checkpatch suggests doesn't hurt and your function becomes even easier to read :)

> 
> Ben.
> 
>> Indentation?
> 
> Not sure what's up with identation, I had it all fixed up to please
> checkpatch, maybe I screwed up the sending of the patch itself.

It could be my mailer too, no idea :). Just stumbled over it.

> Oh
> well, I'm off to hospital on monday so that will have to wait til I'm
> back (I regret you didn't make those comments on the previous iteration
> of the patch though).

Yeah, it's a shame I didn't read through it more thoroughly earlier - at least it didn't take weeks in this round ;).

No worries though, if you can't make it until Monday, I'll fix it up myself afterwards :). There's no black magic involved here, so I should be ok to respin myself.


Alex

Re: [PATCH v2] spapr: Add "memop" hypercall

[Benjamin Herrenschmidt]

On Fri, 2012-05-25 at 10:54 +0200, Alexander Graf wrote:

> >> case x:
> >>    foo();
> >>    break();
> >> 
> >>> break;
> >>> +        case 1: tmp = lduw_phys(src); break;
> >>> +        case 2: tmp = ldl_phys(src);  break;
> >>> +        case 3: tmp = ldq_phys(src);  break;
> >>> +        default:
> >>> +        return H_PARAMETER;
> > 
> > Checkpatch absolutely complained and I decided to ignore it, seriously,
> > you really want to replace a nice & readable piece of code with
> > something that takes 3 pages and is generally gross & ugly ?
> > 
> > Some times, you have to ignore check patch and let sanity prevail.
> 
> I'm not all that keen on coding style rules. But check out
> arch/powerpc/kvm/emulate.c and tell me that it's a good idea to go
> with this "clean" approach. If you want it really clean, put the whole
> chunk above into a geberic helper that allows for everyone to say
> "read n bytes of data with native endianness into a u64". In that
> code, the more verbose coding style checkpatch suggests doesn't hurt
> and your function becomes even easier to read :)

I find your lack of taste disturbing Luke :-)

> Yeah, it's a shame I didn't read through it more thoroughly earlier - at least it didn't take weeks in this round ;).
> 
> No worries though, if you can't make it until Monday, I'll fix it up myself afterwards :). There's no black magic involved here,
> so I should be ok to respin myself.

Sure. No worries. To test it properly you really need a newer SLOF and
the patch to add -vga tho, I'll sort that out when I'm back.

Cheers,
Ben.

> Alex

Re: [PATCH v2] spapr: Add "memop" hypercall

[Alexander Graf]

On 25.05.2012, at 11:24, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Fri, 2012-05-25 at 10:54 +0200, Alexander Graf wrote:
> 
>>>> case x:
>>>>   foo();
>>>>   break();
>>>> 
>>>>> break;
>>>>> +        case 1: tmp = lduw_phys(src); break;
>>>>> +        case 2: tmp = ldl_phys(src);  break;
>>>>> +        case 3: tmp = ldq_phys(src);  break;
>>>>> +        default:
>>>>> +        return H_PARAMETER;
>>> 
>>> Checkpatch absolutely complained and I decided to ignore it, seriously,
>>> you really want to replace a nice & readable piece of code with
>>> something that takes 3 pages and is generally gross & ugly ?
>>> 
>>> Some times, you have to ignore check patch and let sanity prevail.
>> 
>> I'm not all that keen on coding style rules. But check out
>> arch/powerpc/kvm/emulate.c and tell me that it's a good idea to go
>> with this "clean" approach. If you want it really clean, put the whole
>> chunk above into a geberic helper that allows for everyone to say
>> "read n bytes of data with native endianness into a u64". In that
>> code, the more verbose coding style checkpatch suggests doesn't hurt
>> and your function becomes even easier to read :)
> 
> I find your lack of taste disturbing Luke :-)

Heh :). I merely try to not collide with laws I can't influence easily and don't bother to change enough ;).

> 
>> Yeah, it's a shame I didn't read through it more thoroughly earlier - at least it didn't take weeks in this round ;).
>> 
>> No worries though, if you can't make it until Monday, I'll fix it up myself afterwards :). There's no black magic involved here,
>> so I should be ok to respin myself.
> 
> Sure. No worries. To test it properly you really need a newer SLOF and
> the patch to add -vga tho, I'll sort that out when I'm back.

Hrm. Send them over in a private mail and push the WIP SLOF to a branch somewhere then please. There are some community folks interested in VGA with -M pseries, so I'd like to at least have it prototype style available for folks to play with :)


Alex

Re: [PATCH v2] spapr: Add "memop" hypercall

[Benjamin Herrenschmidt]

 
> > I find your lack of taste disturbing Luke :-)
> 
> Heh :). I merely try to not collide with laws I can't influence easily and don't bother to change enough ;).

Compliance with established authority has never been my strong point :-)

> >> Yeah, it's a shame I didn't read through it more thoroughly earlier - at least it didn't take weeks in this round ;).
> >> 
> >> No worries though, if you can't make it until Monday, I'll fix it up myself afterwards :). There's no black magic involved here,
> >> so I should be ok to respin myself.
> > 
> > Sure. No worries. To test it properly you really need a newer SLOF and
> > the patch to add -vga tho, I'll sort that out when I'm back.
> 
> Hrm. Send them over in a private mail and push the WIP SLOF to a
> branch somewhere then please. There are some community folks interested
> in VGA with -M pseries, so I'd like to at least have it prototype style
> available for folks to play with :)

Will try to shoot stuff somewhere you can catch it before I get
on the operating table.

Cheers,
Ben.