On Mon, Feb 08, 2016 at 02:37:25PM +0000, Marc Zyngier wrote:
> So far, our handling of cache maintenance by VA has been pretty
> simple: Either the access is in the guest RAM and generates a S2
> fault, which results in the page being mapped RW, or we go down
> the io_mem_abort() path, and nuke the guest.
>
> The first one is fine, but the second one is extremely weird.
> Treating the CM as an I/O is wrong, and nothing in the ARM ARM
> indicates that we should generate a fault for something that
> cannot end-up in the cache anyway (even if the guest maps it,
> it will keep on faulting at stage-2 for emulation).
>
> So let's just skip this instruction, and let the guest get away
> with it.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> ---
> arch/arm/include/asm/kvm_emulate.h | 5 +++++
> arch/arm/kvm/mmu.c | 17 +++++++++++++++++
> arch/arm64/include/asm/kvm_emulate.h | 5 +++++
> 3 files changed, 27 insertions(+)
>
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index 3095df0..f768797 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -143,6 +143,11 @@ static inline bool kvm_vcpu_dabt_iss1tw(struct kvm_vcpu *vcpu)
> return kvm_vcpu_get_hsr(vcpu) & HSR_DABT_S1PTW;
> }
>
> +static inline bool kvm_vcpu_dabt_iscm(struct kvm_vcpu *vcpu)
> +{
> + return !!(kvm_vcpu_get_hsr(vcpu) & HSR_DABT_CM);
> +}
> +
> /* Get Access Size from a data abort */
> static inline int kvm_vcpu_dabt_get_as(struct kvm_vcpu *vcpu)
> {
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index aba61fd..1a5f2ea 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -1431,6 +1431,23 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
> }
>
> /*
> + * Check for a cache maintenance operation. Since we
> + * ended-up here, we know it is outside of any memory
> + * slot. But we can't find out if that is for a device,
> + * or if the guest is just being stupid (going all the
> + * way to userspace is not an option - what would you
> + * write?).
I'm not sure what the stuff about "what would you write?" means, I'm
assuming it means that the ISV bit is clear.
I think the point is more that if there's no S2 mapping, then there
could never be any cache entries as a result of memory accesses using
the GVA in question, so there's nothing to invalidate (like you state in
your commit message).
> + *
> + * So let's assume that the guest is just being
> + * cautious, and skip the instruction.
> + */
> + if (kvm_vcpu_dabt_iscm(vcpu)) {
> + kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> + ret = 1;
> + goto out_unlock;
> + }
> +
> + /*
> * The IPA is reported as [MAX:12], so we need to
> * complement it with the bottom 12 bits from the
> * faulting VA. This is always 12 bits, irrespective
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 3066328..01cdf5f 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -185,6 +185,11 @@ static inline bool kvm_vcpu_dabt_iss1tw(const struct kvm_vcpu *vcpu)
> return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_S1PTW);
> }
>
> +static inline bool kvm_vcpu_dabt_iscm(const struct kvm_vcpu *vcpu)
> +{
> + return !!(kvm_vcpu_get_hsr(vcpu) & ESR_ELx_CM);
> +}
> +
> static inline int kvm_vcpu_dabt_get_as(const struct kvm_vcpu *vcpu)
> {
> return 1 << ((kvm_vcpu_get_hsr(vcpu) & ESR_ELx_SAS) >> ESR_ELx_SAS_SHIFT);
> --
> 2.1.4
>
some nitpicking: if you modify anything here, I think is_cm is more clear than iscm.
But besides the cosmetics:
Reviewed-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
