Patchwork [5/6] KVM: PPC: Book3S HV: Access XICS in BE

login
register
mail settings
Submitter Alexander Graf
Date June 11, 2014, 10:33 a.m.
Message ID <1402482831-8941-6-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/358598/
State New
Headers show

Comments

Alexander Graf - June 11, 2014, 10:33 a.m.
On the exit path from the guest we check what type of interrupt we received
if we received one. This means we're doing hardware access to the XICS interrupt
controller.

However, when running on a little endian system, this access is byte reversed.

So let's make sure to swizzle the bytes back again and virtuall make XICS
accesses big endian.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kvm/book3s_hv_rmhandlers.S | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
Paul Mackerras - June 17, 2014, 1:02 a.m.
On Wed, Jun 11, 2014 at 12:33:50PM +0200, Alexander Graf wrote:
> On the exit path from the guest we check what type of interrupt we received
> if we received one. This means we're doing hardware access to the XICS interrupt
> controller.
> 
> However, when running on a little endian system, this access is byte reversed.
> 
> So let's make sure to swizzle the bytes back again and virtuall make XICS
> accesses big endian.
...

> @@ -2241,7 +2253,8 @@ kvmppc_read_intr:
>  42:	/* It's not an IPI and it's for the host, stash it in the PACA
>  	 * before exit, it will be picked up by the host ICP driver
>  	 */
> -	stw	r0, HSTATE_SAVED_XIRR(r13)
> +	li	r4, HSTATE_SAVED_XIRR
> +	STWX_BE	r0, r13, r4

This is a paca field, not something mandated by PAPR or shared with
the guest, so why do we need to keep it BE?  If you do make it BE,
don't you also need to fix kvmppc_get_xics_latch()?

Paul.
--
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
Alexander Graf - June 17, 2014, 8:37 a.m.
On 17.06.14 03:02, Paul Mackerras wrote:
> On Wed, Jun 11, 2014 at 12:33:50PM +0200, Alexander Graf wrote:
>> On the exit path from the guest we check what type of interrupt we received
>> if we received one. This means we're doing hardware access to the XICS interrupt
>> controller.
>>
>> However, when running on a little endian system, this access is byte reversed.
>>
>> So let's make sure to swizzle the bytes back again and virtuall make XICS
>> accesses big endian.
> ...
>
>> @@ -2241,7 +2253,8 @@ kvmppc_read_intr:
>>   42:	/* It's not an IPI and it's for the host, stash it in the PACA
>>   	 * before exit, it will be picked up by the host ICP driver
>>   	 */
>> -	stw	r0, HSTATE_SAVED_XIRR(r13)
>> +	li	r4, HSTATE_SAVED_XIRR
>> +	STWX_BE	r0, r13, r4
> This is a paca field, not something mandated by PAPR or shared with
> the guest, so why do we need to keep it BE?  If you do make it BE,
> don't you also need to fix kvmppc_get_xics_latch()?

Yikes. Yes. Thanks a lot for the catch!


Alex

--
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
Alexander Graf - June 17, 2014, 10:22 a.m.
On 17.06.14 10:37, Alexander Graf wrote:
>
> On 17.06.14 03:02, Paul Mackerras wrote:
>> On Wed, Jun 11, 2014 at 12:33:50PM +0200, Alexander Graf wrote:
>>> On the exit path from the guest we check what type of interrupt we 
>>> received
>>> if we received one. This means we're doing hardware access to the 
>>> XICS interrupt
>>> controller.
>>>
>>> However, when running on a little endian system, this access is byte 
>>> reversed.
>>>
>>> So let's make sure to swizzle the bytes back again and virtuall make 
>>> XICS
>>> accesses big endian.
>> ...
>>
>>> @@ -2241,7 +2253,8 @@ kvmppc_read_intr:
>>>   42:    /* It's not an IPI and it's for the host, stash it in the PACA
>>>        * before exit, it will be picked up by the host ICP driver
>>>        */
>>> -    stw    r0, HSTATE_SAVED_XIRR(r13)
>>> +    li    r4, HSTATE_SAVED_XIRR
>>> +    STWX_BE    r0, r13, r4
>> This is a paca field, not something mandated by PAPR or shared with
>> the guest, so why do we need to keep it BE?  If you do make it BE,
>> don't you also need to fix kvmppc_get_xics_latch()?
>
> Yikes. Yes. Thanks a lot for the catch!

Eh, no. What we do is we read (good on BE, byte reversed) into r0. Then 
we swab32() from r0 to r3 on LE, mr from r0 to r3 on BE.

r3 gets truncated along the way.

The reason we maintain r0 as wrong-endian is that we write it back using 
the cache inhibited stwcix instruction:

>         stwcix  r0, r6, r7              /* EOI it */

So during the lifetime of r0 as XIRR it's always byte-reversed on LE. 
That's why we store it using STWX_BE into hstate, because that's the 
time when we actually swab32() it for further interpretation.

Alternatively I could clobber a different register and maintain the byte 
swapped variant in there if you prefer.


Alex

--
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
Paul Mackerras - June 17, 2014, 12:13 p.m.
On Tue, Jun 17, 2014 at 12:22:32PM +0200, Alexander Graf wrote:
> 
> Eh, no. What we do is we read (good on BE, byte reversed) into r0. Then we
> swab32() from r0 to r3 on LE, mr from r0 to r3 on BE.
> 
> r3 gets truncated along the way.
> 
> The reason we maintain r0 as wrong-endian is that we write it back using the
> cache inhibited stwcix instruction:
> 
> >        stwcix  r0, r6, r7              /* EOI it */
> 
> So during the lifetime of r0 as XIRR it's always byte-reversed on LE. That's
> why we store it using STWX_BE into hstate, because that's the time when we
> actually swab32() it for further interpretation.

So the STWX_BE is more like a be32_to_cpu than a cpu_to_be32, which is
what the name STWX_BE would suggest.  Sounds like it at least deserves
a comment, or (as you suggest) rearrange the register usage so a
normal store works.

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

Patch

diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index 3530d06..40dec37 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -36,6 +36,13 @@ 
 #define NAPPING_CEDE	1
 #define NAPPING_NOVCPU	2
 
+.macro bswap32 regd, regs
+	srwi	\regd,\regs,24
+	rlwimi	\regd,\regs,24,16,23
+	rlwimi	\regd,\regs,8,8,15
+	rlwimi	\regd,\regs,24,0,7
+.endm
+
 /*
  * Call kvmppc_hv_entry in real mode.
  * Must be called with interrupts hard-disabled.
@@ -2206,7 +2213,12 @@  kvmppc_read_intr:
 	cmpdi	r6, 0
 	beq-	1f
 	lwzcix	r0, r6, r7
-	rlwinm.	r3, r0, 0, 0xffffff
+#ifdef __LITTLE_ENDIAN__
+	bswap32	r3, r0
+#else
+	mr	r3, r0
+#endif
+	rlwinm.	r3, r3, 0, 0xffffff
 	sync
 	beq	1f			/* if nothing pending in the ICP */
 
@@ -2241,7 +2253,8 @@  kvmppc_read_intr:
 42:	/* It's not an IPI and it's for the host, stash it in the PACA
 	 * before exit, it will be picked up by the host ICP driver
 	 */
-	stw	r0, HSTATE_SAVED_XIRR(r13)
+	li	r4, HSTATE_SAVED_XIRR
+	STWX_BE	r0, r13, r4
 	li	r3, 1
 	b	1b