Re: [PATCH 1/2] KVM: arm64: Make vcpu_cp1x() work on Big Endian hosts

2020-06-10 Thread James Morse 
Hi Marc, Robin,

On 09/06/2020 12:48, Marc Zyngier wrote:
> On 2020-06-09 12:41, Robin Murphy wrote:
>> On 2020-06-09 09:49, Marc Zyngier wrote:
>>> AArch32 CP1x registers are overlayed on their AArch64 counterparts
>>> in the vcpu struct. This leads to an interesting problem as they
>>> are stored in their CPU-local format, and thus a CP1x register
>>> doesn't "hit" the lower 32bit portion of the AArch64 register on
>>> a BE host.
>>>
>>> To workaround this unfortunate situation, introduce a bias trick
>>> in the vcpu_cp1x() accessors which picks the correct half of the
>>> 64bit register.

>>> diff --git a/arch/arm64/include/asm/kvm_host.h 
>>> b/arch/arm64/include/asm/kvm_host.h
>>> index 59029e90b557..e80c0e06f235 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -404,8 +404,14 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 
>>> val, int reg);
>>>    * CP14 and CP15 live in the same array, as they are backed by the
>>>    * same system registers.
>>>    */
>>> -#define vcpu_cp14(v,r)    ((v)->arch.ctxt.copro[(r)])
>>> -#define vcpu_cp15(v,r)    ((v)->arch.ctxt.copro[(r)])
>>> +#ifdef CPU_BIG_ENDIAN
>>
>> Ahem... I think you're missing a "CONFIG_" there ;)
> 
> Duh! As I said, I didn't test the thing at all! ;-)
> 
>> Bonus trickery - for a 0 or 1 value you can simply use IS_ENABLED().
> 
> Beautiful! Definitely a must! :D

With Robin's suggestion of:
---%<---
diff --git a/arch/arm64/include/asm/kvm_host.h 
b/arch/arm64/include/asm/kvm_host.h
index 2a935457712b..54e9c7eb3596 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -405,11 +405,7 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, 
int reg);
  * CP14 and CP15 live in the same array, as they are backed by the
  * same system registers.
  */
-#ifdef CPU_BIG_ENDIAN
-#define CPx_OFFSET 1
-#else
-#define CPx_OFFSET 0
-#endif
+#define CPx_OFFSET IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)

 #define vcpu_cp14(v,r) ((v)->arch.ctxt.copro[(r) ^ CPx_OFFSET])
 #define vcpu_cp15(v,r) ((v)->arch.ctxt.copro[(r) ^ CPx_OFFSET])
---%<---

Tested-by: James Morse 
Acked-by: James Morse 


Thanks,

James

-
Before this patch, an aarch32 guest of a BE host reading sysregs KVM is 
trap-and-undeffing
gets:
| Bad mode in prefetch abort handler detected
| Internal error: Oops - bad mode: 0 [#1] SMP THUMB2
| Modules linked in:
| CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.7.0+ #260
| Hardware name: Generic DT based system
| PC is at 0x4
| LR is at smp_cpus_done+0x85/0x98
| pc : [<0004>]lr : [<808035cb>]psr: 609b
| sp : 9f4a1f08  ip : 0003  fp : 
| r10:   r9 :   r8 : 
| r7 : 80904ea8  r6 : 80904f6c  r5 : 0002  r4 : 000f4240
| r3 : bc605c12  r2 : bc605c12  r1 : 1f38c000  r0 : c348
| Flags: nZCv  IRQs off  FIQs on  Mode UND_32  ISA ARM  Segment none
| Control: 50c5383d  Table: 8000406a  DAC: bc605c12
| Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
[...]
| [<808035cb>] (smp_cpus_done) from [<0002>] (0x2)
| Code: bad PC value
| ---[ end trace b37275bf489ca225 ]---


instead of the undef it so richly deserved:
| Internal error: Oops - undefined instruction: 0 [#1] SMP THUMB2
| Modules linked in:
| CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.7.0+ #260
| Hardware name: Generic DT based system
| PC is at smp_cpus_done+0x88/0x98
| LR is at smp_cpus_done+0x85/0x98
| pc : [<808035ce>]lr : [<808035cb>]psr: 6073
| sp : 9f495f50  ip : 0001  fp : 
| r10:   r9 :   r8 : 
| r7 : 80904ea8  r6 : 80904f6c  r5 : 0001  r4 : 0007a120
| r3 : 7f3828d2  r2 : 7f3828d2  r1 : 1f39f000  r0 : c348
| Flags: nZCv  IRQs on  FIQs off  Mode SVC_32  ISA Thumb  Segment none
| Control: 50c5383d  Table: 8000406a  DAC: 0051
| Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
[...]
| [<808035ce>] (smp_cpus_done) from [<80800f73>] 
(kernel_init_freeable+0xdf/0x204)
| [<80800f73>] (kernel_init_freeable) from [<805aa2a7>] (kernel_init+0x7/0xc8)
| [<805aa2a7>] (kernel_init) from [<80100159>] (ret_from_fork+0x11/0x38)
| Code: f7ff f8b9 f24c 3048 (ee11) 1f30
| ---[ end trace 4c78dcd8460e6041 ]---


___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/2] KVM: arm64: Make vcpu_cp1x() work on Big Endian hosts

2020-06-09 Thread Marc Zyngier 

Hi Robin,

On 2020-06-09 12:41, Robin Murphy wrote:

On 2020-06-09 09:49, Marc Zyngier wrote:

AArch32 CP1x registers are overlayed on their AArch64 counterparts
in the vcpu struct. This leads to an interesting problem as they
are stored in their CPU-local format, and thus a CP1x register
doesn't "hit" the lower 32bit portion of the AArch64 register on
a BE host.

To workaround this unfortunate situation, introduce a bias trick
in the vcpu_cp1x() accessors which picks the correct half of the
64bit register.

Cc: sta...@vger.kernel.org
Reported-by: James Morse 
Signed-off-by: Marc Zyngier 
---
  arch/arm64/include/asm/kvm_host.h | 10 --
  1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h 
b/arch/arm64/include/asm/kvm_host.h

index 59029e90b557..e80c0e06f235 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -404,8 +404,14 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, 
u64 val, int reg);

   * CP14 and CP15 live in the same array, as they are backed by the
   * same system registers.
   */
-#define vcpu_cp14(v,r) ((v)->arch.ctxt.copro[(r)])
-#define vcpu_cp15(v,r) ((v)->arch.ctxt.copro[(r)])
+#ifdef CPU_BIG_ENDIAN


Ahem... I think you're missing a "CONFIG_" there ;)


Duh! As I said, I didn't test the thing at all! ;-)


Bonus trickery - for a 0 or 1 value you can simply use IS_ENABLED().


Beautiful! Definitely a must! :D

Thanks,

M.
--
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/2] KVM: arm64: Make vcpu_cp1x() work on Big Endian hosts

2020-06-09 Thread Robin Murphy 

On 2020-06-09 09:49, Marc Zyngier wrote:

AArch32 CP1x registers are overlayed on their AArch64 counterparts
in the vcpu struct. This leads to an interesting problem as they
are stored in their CPU-local format, and thus a CP1x register
doesn't "hit" the lower 32bit portion of the AArch64 register on
a BE host.

To workaround this unfortunate situation, introduce a bias trick
in the vcpu_cp1x() accessors which picks the correct half of the
64bit register.

Cc: sta...@vger.kernel.org
Reported-by: James Morse 
Signed-off-by: Marc Zyngier 
---
  arch/arm64/include/asm/kvm_host.h | 10 --
  1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h 
b/arch/arm64/include/asm/kvm_host.h
index 59029e90b557..e80c0e06f235 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -404,8 +404,14 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, 
int reg);
   * CP14 and CP15 live in the same array, as they are backed by the
   * same system registers.
   */
-#define vcpu_cp14(v,r) ((v)->arch.ctxt.copro[(r)])
-#define vcpu_cp15(v,r) ((v)->arch.ctxt.copro[(r)])
+#ifdef CPU_BIG_ENDIAN


Ahem... I think you're missing a "CONFIG_" there ;)

Bonus trickery - for a 0 or 1 value you can simply use IS_ENABLED().

Robin.


+#define CPx_OFFSET 1
+#else
+#define CPx_OFFSET 0
+#endif
+
+#define vcpu_cp14(v,r) ((v)->arch.ctxt.copro[(r) ^ CPx_OFFSET])
+#define vcpu_cp15(v,r) ((v)->arch.ctxt.copro[(r) ^ CPx_OFFSET])
  
  struct kvm_vm_stat {

ulong remote_tlb_flush;


___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 1/2] KVM: arm64: Make vcpu_cp1x() work on Big Endian hosts

2020-06-09 Thread Marc Zyngier 
AArch32 CP1x registers are overlayed on their AArch64 counterparts
in the vcpu struct. This leads to an interesting problem as they
are stored in their CPU-local format, and thus a CP1x register
doesn't "hit" the lower 32bit portion of the AArch64 register on
a BE host.

To workaround this unfortunate situation, introduce a bias trick
in the vcpu_cp1x() accessors which picks the correct half of the
64bit register.

Cc: sta...@vger.kernel.org
Reported-by: James Morse 
Signed-off-by: Marc Zyngier 
---
 arch/arm64/include/asm/kvm_host.h | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h 
b/arch/arm64/include/asm/kvm_host.h
index 59029e90b557..e80c0e06f235 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -404,8 +404,14 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, 
int reg);
  * CP14 and CP15 live in the same array, as they are backed by the
  * same system registers.
  */
-#define vcpu_cp14(v,r) ((v)->arch.ctxt.copro[(r)])
-#define vcpu_cp15(v,r) ((v)->arch.ctxt.copro[(r)])
+#ifdef CPU_BIG_ENDIAN
+#define CPx_OFFSET 1
+#else
+#define CPx_OFFSET 0
+#endif
+
+#define vcpu_cp14(v,r) ((v)->arch.ctxt.copro[(r) ^ CPx_OFFSET])
+#define vcpu_cp15(v,r) ((v)->arch.ctxt.copro[(r) ^ CPx_OFFSET])
 
 struct kvm_vm_stat {
ulong remote_tlb_flush;
-- 
2.26.2

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm