[RFC PATCH] arm/arm64: KVM: Fix BE accesses to GICv2 EISR and ELRSR regs
The EIRSR and ELRSR registers are 32-bit registers on GICv2, and we
store these as an array of two such registers on the vgic vcpu struct.
However, we access them as a single 64-bit value or as a bitmap pointer
in the generic vgic code, which breaks BE support.
Instead, store them as u64 values on the vgic structure and do the
word-swapping in the assembly code, which already handles the byte order
for BE systems.
Signed-off-by: Christoffer Dall
---
I don't have a working BE configuration, so sending this out as an
*UNTESTED on BE* RFC in hoping that someone with a working setup can
give it a test for me (Will tells me Virtio is broken with kvmtool no BE
hosts though), and to avoid someone else also doing this work.
arch/arm/kvm/interrupts_head.S | 7 +++
arch/arm64/kvm/vgic-v2-switch.S | 12
include/kvm/arm_vgic.h | 4 ++--
virt/kvm/arm/vgic-v2.c | 24 +++-
virt/kvm/arm/vgic.c | 18 --
5 files changed, 36 insertions(+), 29 deletions(-)
diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index 98c8c5b..14d4883 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -433,10 +433,17 @@ ARM_BE8(rev r10, r10)
str r3, [r11, #VGIC_V2_CPU_HCR]
str r4, [r11, #VGIC_V2_CPU_VMCR]
str r5, [r11, #VGIC_V2_CPU_MISR]
+#ifdef CONFIG_CPU_ENDIAN_BE8
+ str r6, [r11, #(VGIC_V2_CPU_EISR + 4)]
+ str r7, [r11, #VGIC_V2_CPU_EISR]
+ str r8, [r11, #(VGIC_V2_CPU_ELRSR + 4)]
+ str r9, [r11, #VGIC_V2_CPU_ELRSR]
+#else
str r6, [r11, #VGIC_V2_CPU_EISR]
str r7, [r11, #(VGIC_V2_CPU_EISR + 4)]
str r8, [r11, #VGIC_V2_CPU_ELRSR]
str r9, [r11, #(VGIC_V2_CPU_ELRSR + 4)]
+#endif
str r10, [r11, #VGIC_V2_CPU_APR]
/* Clear GICH_HCR */
diff --git a/arch/arm64/kvm/vgic-v2-switch.S b/arch/arm64/kvm/vgic-v2-switch.S
index ae21177..f002fe1 100644
--- a/arch/arm64/kvm/vgic-v2-switch.S
+++ b/arch/arm64/kvm/vgic-v2-switch.S
@@ -67,10 +67,14 @@ CPU_BE( rev w11, w11 )
str w4, [x3, #VGIC_V2_CPU_HCR]
str w5, [x3, #VGIC_V2_CPU_VMCR]
str w6, [x3, #VGIC_V2_CPU_MISR]
- str w7, [x3, #VGIC_V2_CPU_EISR]
- str w8, [x3, #(VGIC_V2_CPU_EISR + 4)]
- str w9, [x3, #VGIC_V2_CPU_ELRSR]
- str w10, [x3, #(VGIC_V2_CPU_ELRSR + 4)]
+CPU_LE(str w7, [x3, #VGIC_V2_CPU_EISR] )
+CPU_LE(str w8, [x3, #(VGIC_V2_CPU_EISR + 4)] )
+CPU_LE(str w9, [x3, #VGIC_V2_CPU_ELRSR] )
+CPU_LE(str w10, [x3, #(VGIC_V2_CPU_ELRSR + 4)] )
+CPU_BE(str w7, [x3, #(VGIC_V2_CPU_EISR + 4)] )
+CPU_BE(str w8, [x3, #VGIC_V2_CPU_EISR] )
+CPU_BE(str w9, [x3, #(VGIC_V2_CPU_ELRSR + 4)] )
+CPU_BE(str w10, [x3, #VGIC_V2_CPU_ELRSR] )
str w11, [x3, #VGIC_V2_CPU_APR]
/* Clear GICH_HCR */
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 35b0c12..c66dc9ed 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -168,8 +168,8 @@ struct vgic_v2_cpu_if {
u32 vgic_hcr;
u32 vgic_vmcr;
u32 vgic_misr; /* Saved only */
- u32 vgic_eisr[2]; /* Saved only */
- u32 vgic_elrsr[2]; /* Saved only */
+ u64 vgic_eisr; /* Saved only */
+ u64 vgic_elrsr; /* Saved only */
u32 vgic_apr;
u32 vgic_lr[VGIC_V2_MAX_LRS];
};
diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
index 416baed..2935405 100644
--- a/virt/kvm/arm/vgic-v2.c
+++ b/virt/kvm/arm/vgic-v2.c
@@ -71,35 +71,17 @@ static void vgic_v2_sync_lr_elrsr(struct kvm_vcpu *vcpu,
int lr,
struct vgic_lr lr_desc)
{
if (!(lr_desc.state & LR_STATE_MASK))
- __set_bit(lr, (unsigned long
*)vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr);
+ vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr |= (1ULL << lr);
}
static u64 vgic_v2_get_elrsr(const struct kvm_vcpu *vcpu)
{
- u64 val;
-
-#if BITS_PER_LONG == 64
- val = vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr[1];
- val <<= 32;
- val |= vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr[0];
-#else
- val = *(u64 *)vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr;
-#endif
- return val;
+ return vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr;
}
static u64 vgic_v2_get_eisr(const struct kvm_vcpu *vcpu)
{
- u64 val;
-
-#if BITS_PER_LONG == 64
- val = vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr[1];
- val <<= 32;
- val |= vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr[0];
-#else
- val = *(u64 *)vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr;
-#endif
- return val;
+ return vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr;
}
static u32 vgic_v2_get_interrupt_status(c
Re: [RFC PATCH] arm/arm64: KVM: Fix BE accesses to GICv2 EISR and ELRSR regs
Hi Christoffer,
On 28/09/14 15:04, Christoffer Dall wrote:
> The EIRSR and ELRSR registers are 32-bit registers on GICv2, and we
> store these as an array of two such registers on the vgic vcpu struct.
> However, we access them as a single 64-bit value or as a bitmap pointer
> in the generic vgic code, which breaks BE support.
>
> Instead, store them as u64 values on the vgic structure and do the
> word-swapping in the assembly code, which already handles the byte order
> for BE systems.
I am wondering if that approach isn't too involved.
EISR and ELRSR are 32-bit registers, and AFAIK the GIC is always LE on
the hardware side. So by claiming we have a 64bit register we introduce
too much hassle, right?
Wouldn't it be better to just fix the registers shortly before we
actually use them?
With my limited endianess experience: Wouldn't this patch solve the EISR
part (copy & pasted, so "for eyes only" ;-)
@@ -92,13 +89,10 @@ static u64 vgic_v2_get_eisr(const struct kvm_vcpu *vcpu)
{
u64 val;
-#if BITS_PER_LONG == 64
- val = vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr[1];
+ val = le32_to_cpu(vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr[1]);
val <<= 32;
- val |= vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr[0];
-#else
- val = *(u64 *)vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr;
-#endif
+ val |= le32_to_cpu(vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr[0]);
+
return val;
}
(BTW: Wasn't that 64 bit optimization path the wrong way round here?)
Please bear with me if this is complete nonsense, could well twisted my
mind once too much ;-)
Still thinking about a clever solution for that strange sync_elrsr()
thing ...
Cheers,
Andre.
>
> Signed-off-by: Christoffer Dall
> ---
> I don't have a working BE configuration, so sending this out as an
> *UNTESTED on BE* RFC in hoping that someone with a working setup can
> give it a test for me (Will tells me Virtio is broken with kvmtool no BE
> hosts though), and to avoid someone else also doing this work.
>
> arch/arm/kvm/interrupts_head.S | 7 +++
> arch/arm64/kvm/vgic-v2-switch.S | 12
> include/kvm/arm_vgic.h | 4 ++--
> virt/kvm/arm/vgic-v2.c | 24 +++-
> virt/kvm/arm/vgic.c | 18 --
> 5 files changed, 36 insertions(+), 29 deletions(-)
>
> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
> index 98c8c5b..14d4883 100644
> --- a/arch/arm/kvm/interrupts_head.S
> +++ b/arch/arm/kvm/interrupts_head.S
> @@ -433,10 +433,17 @@ ARM_BE8(rev r10, r10)
> str r3, [r11, #VGIC_V2_CPU_HCR]
> str r4, [r11, #VGIC_V2_CPU_VMCR]
> str r5, [r11, #VGIC_V2_CPU_MISR]
> +#ifdef CONFIG_CPU_ENDIAN_BE8
> + str r6, [r11, #(VGIC_V2_CPU_EISR + 4)]
> + str r7, [r11, #VGIC_V2_CPU_EISR]
> + str r8, [r11, #(VGIC_V2_CPU_ELRSR + 4)]
> + str r9, [r11, #VGIC_V2_CPU_ELRSR]
> +#else
> str r6, [r11, #VGIC_V2_CPU_EISR]
> str r7, [r11, #(VGIC_V2_CPU_EISR + 4)]
> str r8, [r11, #VGIC_V2_CPU_ELRSR]
> str r9, [r11, #(VGIC_V2_CPU_ELRSR + 4)]
> +#endif
> str r10, [r11, #VGIC_V2_CPU_APR]
>
> /* Clear GICH_HCR */
> diff --git a/arch/arm64/kvm/vgic-v2-switch.S b/arch/arm64/kvm/vgic-v2-switch.S
> index ae21177..f002fe1 100644
> --- a/arch/arm64/kvm/vgic-v2-switch.S
> +++ b/arch/arm64/kvm/vgic-v2-switch.S
> @@ -67,10 +67,14 @@ CPU_BE( rev w11, w11 )
> str w4, [x3, #VGIC_V2_CPU_HCR]
> str w5, [x3, #VGIC_V2_CPU_VMCR]
> str w6, [x3, #VGIC_V2_CPU_MISR]
> - str w7, [x3, #VGIC_V2_CPU_EISR]
> - str w8, [x3, #(VGIC_V2_CPU_EISR + 4)]
> - str w9, [x3, #VGIC_V2_CPU_ELRSR]
> - str w10, [x3, #(VGIC_V2_CPU_ELRSR + 4)]
> +CPU_LE( str w7, [x3, #VGIC_V2_CPU_EISR] )
> +CPU_LE( str w8, [x3, #(VGIC_V2_CPU_EISR + 4)] )
> +CPU_LE( str w9, [x3, #VGIC_V2_CPU_ELRSR] )
> +CPU_LE( str w10, [x3, #(VGIC_V2_CPU_ELRSR + 4)] )
> +CPU_BE( str w7, [x3, #(VGIC_V2_CPU_EISR + 4)] )
> +CPU_BE( str w8, [x3, #VGIC_V2_CPU_EISR] )
> +CPU_BE( str w9, [x3, #(VGIC_V2_CPU_ELRSR + 4)] )
> +CPU_BE( str w10, [x3, #VGIC_V2_CPU_ELRSR] )
> str w11, [x3, #VGIC_V2_CPU_APR]
>
> /* Clear GICH_HCR */
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 35b0c12..c66dc9ed 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -168,8 +168,8 @@ struct vgic_v2_cpu_if {
> u32 vgic_hcr;
> u32 vgic_vmcr;
> u32 vgic_misr; /* Saved only */
> - u32 vgic_eisr[2]; /* Saved only */
> - u32 vgic_elrsr[2]; /* Saved only */
> + u64 vgic_eisr; /* Saved only */
> + u64 vgic_elrsr; /* Saved only */
> u32 vgic_apr;
> u32 vgic_lr[VGIC_V2_MAX_LRS];
> };
> diff --git a
Re: [RFC PATCH] arm/arm64: KVM: Fix BE accesses to GICv2 EISR and ELRSR regs
Hi Andre,
On Wed, Oct 01, 2014 at 05:55:26PM +0100, Andre Przywara wrote:
> Hi Christoffer,
>
> On 28/09/14 15:04, Christoffer Dall wrote:
> > The EIRSR and ELRSR registers are 32-bit registers on GICv2, and we
> > store these as an array of two such registers on the vgic vcpu struct.
> > However, we access them as a single 64-bit value or as a bitmap pointer
> > in the generic vgic code, which breaks BE support.
> >
> > Instead, store them as u64 values on the vgic structure and do the
> > word-swapping in the assembly code, which already handles the byte order
> > for BE systems.
>
> I am wondering if that approach isn't too involved.
> EISR and ELRSR are 32-bit registers, and AFAIK the GIC is always LE on
> the hardware side. So by claiming we have a 64bit register we introduce
> too much hassle, right?
You have to deal with this mess somehow, the question is just where.
> Wouldn't it be better to just fix the registers shortly before we
> actually use them?
if you're looking at it isolated, I agree completely, having those two
32-bit registers in the struct would be 'nicer'. However, we already
jump through a lot of hoops here to store things in a way that makes it
possible for us to share code between 32-bit and 64-bit and between
gicv2 and gicv3, so it's not completely out of line.
> With my limited endianess experience: Wouldn't this patch solve the EISR
> part (copy & pasted, so "for eyes only" ;-)
>
> @@ -92,13 +89,10 @@ static u64 vgic_v2_get_eisr(const struct kvm_vcpu *vcpu)
> {
> u64 val;
>
> -#if BITS_PER_LONG == 64
> - val = vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr[1];
> + val = le32_to_cpu(vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr[1]);
don't do this - the reverse instructions for dealing with the inner-word
endianness already takes place in the assembly code, so that's not the
problem.
> val <<= 32;
> - val |= vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr[0];
> -#else
> - val = *(u64 *)vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr;
> -#endif
> + val |= le32_to_cpu(vcpu->arch.vgic_cpu.vgic_v2.vgic_eisr[0]);
what that whole block above does, is that when an unsigned long is 64
bits, if you just convert the &eisr[0] pointer to an unsigned long *,
you'll get this on a LE system:
| 63 .. 32 | 31 .. 0 |
| eisr[1] |eisr[0] |
which is what you want, but on a BE system, you'll get:
| 63 .. 32 | 31 .. 0 |
| eisr[0] |eisr[1] |
which is obviously not what the caller expects. The current code block
(before my patch) always gives you the first one, which is what you'd
want.
> +
> return val;
> }
>
> (BTW: Wasn't that 64 bit optimization path the wrong way round here?)
Not sure I understand your question?
>
> Please bear with me if this is complete nonsense, could well twisted my
> mind once too much ;-)
>
> Still thinking about a clever solution for that strange sync_elrsr()
> thing ...
>
You have to factor that into the equation and not split up the things,
that's where I think you go wrong.
I tried to do what you asked before, but it becomes a bloody mess, with
a more convoluted patch and resulting code path than what I suggest,
IMHO.
Try to write out the whole thing yourself and be very careful that
you're getting it right.
If you can come up with a way that works with the callers using
for_each_set_bit() (open-coding that one doesn't exactly make that
twited vgic code any more pretty), then I'll be very happy to throw this
away and take yours.
Happy brain twisting :)
Thanks for looking at this stuff.
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] arm/arm64: KVM: Fix BE accesses to GICv2 EISR and ELRSR regs
On Sun, Sep 28 2014 at 03:04:26 PM, Christoffer Dall wrote: > The EIRSR and ELRSR registers are 32-bit registers on GICv2, and we > store these as an array of two such registers on the vgic vcpu struct. > However, we access them as a single 64-bit value or as a bitmap pointer > in the generic vgic code, which breaks BE support. > > Instead, store them as u64 values on the vgic structure and do the > word-swapping in the assembly code, which already handles the byte order > for BE systems. > > Signed-off-by: Christoffer Dall (still going through my email backlog, hence the delay). This looks like a valuable fix. Haven't had a chance to try it (no BE setup at hand) but maybe Victor can help reproducing this?. Acked-by: Marc Zyngier M. -- Jazz is not dead. It just smells funny. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] arm/arm64: KVM: Fix BE accesses to GICv2 EISR and ELRSR regs
On 14 October 2014 02:47, Marc Zyngier wrote: > On Sun, Sep 28 2014 at 03:04:26 PM, Christoffer Dall > wrote: >> The EIRSR and ELRSR registers are 32-bit registers on GICv2, and we >> store these as an array of two such registers on the vgic vcpu struct. >> However, we access them as a single 64-bit value or as a bitmap pointer >> in the generic vgic code, which breaks BE support. >> >> Instead, store them as u64 values on the vgic structure and do the >> word-swapping in the assembly code, which already handles the byte order >> for BE systems. >> >> Signed-off-by: Christoffer Dall > > (still going through my email backlog, hence the delay). > > This looks like a valuable fix. Haven't had a chance to try it (no BE > setup at hand) but maybe Victor can help reproducing this?. I'll give it a spin. Thanks, Victor > Acked-by: Marc Zyngier > > M. > -- > Jazz is not dead. It just smells funny. > ___ > kvmarm mailing list > kvm...@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] arm/arm64: KVM: Fix BE accesses to GICv2 EISR and ELRSR regs
On 14 October 2014 08:21, Victor Kamensky wrote: > On 14 October 2014 02:47, Marc Zyngier wrote: >> On Sun, Sep 28 2014 at 03:04:26 PM, Christoffer Dall >> wrote: >>> The EIRSR and ELRSR registers are 32-bit registers on GICv2, and we >>> store these as an array of two such registers on the vgic vcpu struct. >>> However, we access them as a single 64-bit value or as a bitmap pointer >>> in the generic vgic code, which breaks BE support. >>> >>> Instead, store them as u64 values on the vgic structure and do the >>> word-swapping in the assembly code, which already handles the byte order >>> for BE systems. >>> >>> Signed-off-by: Christoffer Dall >> >> (still going through my email backlog, hence the delay). >> >> This looks like a valuable fix. Haven't had a chance to try it (no BE >> setup at hand) but maybe Victor can help reproducing this?. > > I'll give it a spin. Tested-by: Victor Kamensky Tested on v3.17 + this fix on TC2 (V7) and Mustang (V8) with BE kvm host, tried different combination of guests BE/LE V7/V8. All looks good. Only with latest qemu in BE V8 mode in v3.17 without this fix I was able to reproduce the issue that Will spotted. With kvmtool, and older qemu V8 BE code never hit vgic_v2_set_lr function so that is why we did not run into it before. I guess fix in qemu in pl011 mentioned by 1f2bb4acc125, uncovered vgic_v2_set_lr code path and this BE issue. With this patch it works fine now. Thanks, Victor > Thanks, > Victor > >> Acked-by: Marc Zyngier >> >> M. >> -- >> Jazz is not dead. It just smells funny. >> ___ >> kvmarm mailing list >> kvm...@lists.cs.columbia.edu >> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] arm/arm64: KVM: Fix BE accesses to GICv2 EISR and ELRSR regs
Hi Victor, On Thu, Oct 16, 2014 at 1:54 AM, Victor Kamensky wrote: > On 14 October 2014 08:21, Victor Kamensky wrote: >> On 14 October 2014 02:47, Marc Zyngier wrote: >>> On Sun, Sep 28 2014 at 03:04:26 PM, Christoffer Dall >>> wrote: The EIRSR and ELRSR registers are 32-bit registers on GICv2, and we store these as an array of two such registers on the vgic vcpu struct. However, we access them as a single 64-bit value or as a bitmap pointer in the generic vgic code, which breaks BE support. Instead, store them as u64 values on the vgic structure and do the word-swapping in the assembly code, which already handles the byte order for BE systems. Signed-off-by: Christoffer Dall >>> >>> (still going through my email backlog, hence the delay). >>> >>> This looks like a valuable fix. Haven't had a chance to try it (no BE >>> setup at hand) but maybe Victor can help reproducing this?. >> >> I'll give it a spin. > > Tested-by: Victor Kamensky > > Tested on v3.17 + this fix on TC2 (V7) and Mustang (V8) with BE > kvm host, tried different combination of guests BE/LE V7/V8. All looks > good. > > Only with latest qemu in BE V8 mode in v3.17 without this > fix I was able to reproduce the issue that Will spotted. With kvmtool, > and older qemu V8 BE code never hit vgic_v2_set_lr function so > that is why we did not run into it before. I guess fix in qemu in > pl011 mentioned by 1f2bb4acc125, uncovered vgic_v2_set_lr > code path and this BE issue. With this patch it works fine now. > Thanks for the detailed testing and explanation. I'll apply this one to next. -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
