| Message ID | 20180427145102.5645-1-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 27, 2018 at 03:51:02PM +0100, Marc Zyngier wrote: > Proxying the cpuif accesses at EL2 makes use of vcpu_data_guest_to_host > and co, which check the endianness, which call into vcpu_read_sys_reg... > which isn't mapped at EL2 (it was inlined before, and got moved OoL > with the VHE optimizations). I thought we relied on static inline functions to always be inlined, but apparently not? Does this mean we have potential other bugs looming depending on the mood of the compiler, or was there something special that went wrong here? > > The result is of course a nice panic. Let's add some specialized > cruft to keep the broken platforms that require this hack alive. > I'd rather kill BE support, but hey, just in case... > > Fixes: d47533dab9f5 ("KVM: arm64: Introduce framework for accessing deferred sysregs") > Reported-by: Suzuki K Poulose <suzuki.poulose@arm.com> > Tested-by: Suzuki K Poulose <suzuki.poulose@arm.com> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> Otherwise: Reviewed-by: Christoffer Dall <christoffer.dall@arm.com> > --- > arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c | 33 ++++++++++++++++++++++++++------ > 1 file changed, 27 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c b/arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c > index 86801b6055d6..b83a669b26ac 100644 > --- a/arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c > +++ b/arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c > @@ -23,6 +23,30 @@ > #include <asm/kvm_hyp.h> > #include <asm/kvm_mmu.h> > > +static bool __hyp_text __is_be(struct kvm_vcpu *vcpu) > +{ > + if (vcpu_mode_is_32bit(vcpu)) > + return !!(read_sysreg_el2(spsr) & COMPAT_PSR_E_BIT); > + > + return !!(read_sysreg(SCTLR_EL1) & SCTLR_ELx_EE); > +} > + > +static u32 __hyp_text __host_to_guest_u32(struct kvm_vcpu *vcpu, u32 data) > +{ > + if (__is_be(vcpu)) > + return cpu_to_be32(data); > + > + return cpu_to_le32(data); > +} > + > +static u32 __hyp_text __guest_to_host_u32(struct kvm_vcpu *vcpu, u32 data) > +{ > + if (__is_be(vcpu)) > + return be32_to_cpu(data); > + > + return le32_to_cpu(data); > +} > + > /* > * __vgic_v2_perform_cpuif_access -- perform a GICV access on behalf of the > * guest. > @@ -64,14 +88,11 @@ int __hyp_text __vgic_v2_perform_cpuif_access(struct kvm_vcpu *vcpu) > addr += fault_ipa - vgic->vgic_cpu_base; > > if (kvm_vcpu_dabt_iswrite(vcpu)) { > - u32 data = vcpu_data_guest_to_host(vcpu, > - vcpu_get_reg(vcpu, rd), > - sizeof(u32)); > + u32 data = __guest_to_host_u32(vcpu, vcpu_get_reg(vcpu, rd)); > writel_relaxed(data, addr); > } else { > - u32 data = readl_relaxed(addr); > - vcpu_set_reg(vcpu, rd, vcpu_data_host_to_guest(vcpu, data, > - sizeof(u32))); > + u32 data = __host_to_guest_u32(vcpu, readl_relaxed(addr)); > + vcpu_set_reg(vcpu, rd, data); > } > > return 1; > -- > 2.14.2 >
On Sun, 29 Apr 2018 14:34:32 +0200 Christoffer Dall <christoffer.dall@arm.com> wrote: > On Fri, Apr 27, 2018 at 03:51:02PM +0100, Marc Zyngier wrote: > > Proxying the cpuif accesses at EL2 makes use of vcpu_data_guest_to_host > > and co, which check the endianness, which call into vcpu_read_sys_reg... > > which isn't mapped at EL2 (it was inlined before, and got moved OoL > > with the VHE optimizations). > > I thought we relied on static inline functions to always be inlined, but > apparently not? Does this mean we have potential other bugs looming > depending on the mood of the compiler, or was there something special > that went wrong here? We do rely on that behaviour. And that was the case until you moved vcpu_read_sys_reg() to be entirely out of line (see d47533dab9f5). At that point, kvm_vcpu_is_be() becomes a death trap. We missed it for two reasons: - It was only indirectly called, making it quite hard to notice the potential breakage - Nobody gives a damn about 64k pages, specially on something like Juno What we'd need is a way to find cross-section calls (text -> hyp-text should allowed, but not the reverse). We already have similar things in the kernel, it is probably only a matter of reusing the infrastructure for our own purpose. > > The result is of course a nice panic. Let's add some specialized > > cruft to keep the broken platforms that require this hack alive. > > I'd rather kill BE support, but hey, just in case... > > > > Fixes: d47533dab9f5 ("KVM: arm64: Introduce framework for accessing deferred sysregs") > > Reported-by: Suzuki K Poulose <suzuki.poulose@arm.com> > > Tested-by: Suzuki K Poulose <suzuki.poulose@arm.com> > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > Otherwise: > > Reviewed-by: Christoffer Dall <christoffer.dall@arm.com> Thanks, M.
On Sun, Apr 29, 2018 at 02:05:07PM +0100, Marc Zyngier wrote: > On Sun, 29 Apr 2018 14:34:32 +0200 > Christoffer Dall <christoffer.dall@arm.com> wrote: > > > On Fri, Apr 27, 2018 at 03:51:02PM +0100, Marc Zyngier wrote: > > > Proxying the cpuif accesses at EL2 makes use of vcpu_data_guest_to_host > > > and co, which check the endianness, which call into vcpu_read_sys_reg... > > > which isn't mapped at EL2 (it was inlined before, and got moved OoL > > > with the VHE optimizations). > > > > I thought we relied on static inline functions to always be inlined, but > > apparently not? Does this mean we have potential other bugs looming > > depending on the mood of the compiler, or was there something special > > that went wrong here? > > We do rely on that behaviour. And that was the case until you moved > vcpu_read_sys_reg() to be entirely out of line (see d47533dab9f5). ah, now I see, kvm_vcpu_is_be() calls vcpu_read_sys_reg(), I somehow missed this even though you spelled it out in the commit message. Sorry. > > At that point, kvm_vcpu_is_be() becomes a death trap. > > We missed it for two reasons: > - It was only indirectly called, making it quite hard to notice the > potential breakage > - Nobody gives a damn about 64k pages, specially on something like Juno > > What we'd need is a way to find cross-section calls (text -> hyp-text > should allowed, but not the reverse). We already have similar things in > the kernel, it is probably only a matter of reusing the infrastructure > for our own purpose. > That would be good indeed... Thanks for the further explanation. -Christoffer
Hi Marc, On 27/04/18 15:51, Marc Zyngier wrote: > Proxying the cpuif accesses at EL2 makes use of vcpu_data_guest_to_host > and co, which check the endianness, which call into vcpu_read_sys_reg... > which isn't mapped at EL2 (it was inlined before, and got moved OoL > with the VHE optimizations). > > The result is of course a nice panic. Let's add some specialized > cruft to keep the broken platforms that require this hack alive. > I'd rather kill BE support, but hey, just in case... I have this a spin on Juno with a big-endian host and 64K pages: Trying to boot a BE guest hangs. Trying to boot a LE guest hangs. > @@ -64,14 +88,11 @@ int __hyp_text __vgic_v2_perform_cpuif_access(struct kvm_vcpu *vcpu) > addr += fault_ipa - vgic->vgic_cpu_base; > > if (kvm_vcpu_dabt_iswrite(vcpu)) { > - u32 data = vcpu_data_guest_to_host(vcpu, > - vcpu_get_reg(vcpu, rd), > - sizeof(u32)); > + u32 data = __guest_to_host_u32(vcpu, vcpu_get_reg(vcpu, rd)); > writel_relaxed(data, addr); > } else { > - u32 data = readl_relaxed(addr); > - vcpu_set_reg(vcpu, rd, vcpu_data_host_to_guest(vcpu, data, > - sizeof(u32))); > + u32 data = __host_to_guest_u32(vcpu, readl_relaxed(addr)); > + vcpu_set_reg(vcpu, rd, data); > } This happens because readl()/writel() are doing their own swabbing on big-endian, even if the guest had already done this. As I've trampled all over this patch, I'll post a v2... Thanks, James
diff --git a/arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c b/arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c index 86801b6055d6..b83a669b26ac 100644 --- a/arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c +++ b/arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c @@ -23,6 +23,30 @@ #include <asm/kvm_hyp.h> #include <asm/kvm_mmu.h> +static bool __hyp_text __is_be(struct kvm_vcpu *vcpu) +{ + if (vcpu_mode_is_32bit(vcpu)) + return !!(read_sysreg_el2(spsr) & COMPAT_PSR_E_BIT); + + return !!(read_sysreg(SCTLR_EL1) & SCTLR_ELx_EE); +} + +static u32 __hyp_text __host_to_guest_u32(struct kvm_vcpu *vcpu, u32 data) +{ + if (__is_be(vcpu)) + return cpu_to_be32(data); + + return cpu_to_le32(data); +} + +static u32 __hyp_text __guest_to_host_u32(struct kvm_vcpu *vcpu, u32 data) +{ + if (__is_be(vcpu)) + return be32_to_cpu(data); + + return le32_to_cpu(data); +} + /* * __vgic_v2_perform_cpuif_access -- perform a GICV access on behalf of the * guest. @@ -64,14 +88,11 @@ int __hyp_text __vgic_v2_perform_cpuif_access(struct kvm_vcpu *vcpu) addr += fault_ipa - vgic->vgic_cpu_base; if (kvm_vcpu_dabt_iswrite(vcpu)) { - u32 data = vcpu_data_guest_to_host(vcpu, - vcpu_get_reg(vcpu, rd), - sizeof(u32)); + u32 data = __guest_to_host_u32(vcpu, vcpu_get_reg(vcpu, rd)); writel_relaxed(data, addr); } else { - u32 data = readl_relaxed(addr); - vcpu_set_reg(vcpu, rd, vcpu_data_host_to_guest(vcpu, data, - sizeof(u32))); + u32 data = __host_to_guest_u32(vcpu, readl_relaxed(addr)); + vcpu_set_reg(vcpu, rd, data); } return 1;
Proxying the cpuif accesses at EL2 makes use of vcpu_data_guest_to_host and co, which check the endianness, which call into vcpu_read_sys_reg... which isn't mapped at EL2 (it was inlined before, and got moved OoL with the VHE optimizations). The result is of course a nice panic. Let's add some specialized cruft to keep the broken platforms that require this hack alive. I'd rather kill BE support, but hey, just in case... Fixes: d47533dab9f5 ("KVM: arm64: Introduce framework for accessing deferred sysregs") Reported-by: Suzuki K Poulose <suzuki.poulose@arm.com> Tested-by: Suzuki K Poulose <suzuki.poulose@arm.com> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c | 33 ++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-)