* [PATCH] arm/arm64: KVM: Handle out-of-RAM cache maintenance as a NOP
@ 2016-02-08 14:37 Marc Zyngier
2016-02-11 8:43 ` Christoffer Dall
0 siblings, 1 reply; 3+ messages in thread
From: Marc Zyngier @ 2016-02-08 14:37 UTC (permalink / raw)
To: linux-arm-kernel
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@arm.com>
---
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?).
+ *
+ * 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
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH] arm/arm64: KVM: Handle out-of-RAM cache maintenance as a NOP
2016-02-08 14:37 [PATCH] arm/arm64: KVM: Handle out-of-RAM cache maintenance as a NOP Marc Zyngier
@ 2016-02-11 8:43 ` Christoffer Dall
2016-02-11 10:49 ` Marc Zyngier
0 siblings, 1 reply; 3+ messages in thread
From: Christoffer Dall @ 2016-02-11 8:43 UTC (permalink / raw)
To: linux-arm-kernel
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@arm.com>
> ---
> 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@linaro.org>
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH] arm/arm64: KVM: Handle out-of-RAM cache maintenance as a NOP
2016-02-11 8:43 ` Christoffer Dall
@ 2016-02-11 10:49 ` Marc Zyngier
0 siblings, 0 replies; 3+ messages in thread
From: Marc Zyngier @ 2016-02-11 10:49 UTC (permalink / raw)
To: linux-arm-kernel
On 11/02/16 08:43, Christoffer Dall wrote:
> 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@arm.com>
>> ---
>> 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.
What I was trying to say that is that if you really wanted to find out
whether the range the guest is trying to access is a device or just a
non-existent address, you'd have to go all the way to userspace to check
that there is actually a device there. But then, what should the access
be? Read or Write? We don't have a non-destructive "check access" operation.
> 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).
Agreed. I may just write that instead.
>
>> + *
>> + * 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.
Fair enough, I'll amend it.
>
> But besides the cosmetics:
>
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
>
Thanks!
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-02-11 10:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-08 14:37 [PATCH] arm/arm64: KVM: Handle out-of-RAM cache maintenance as a NOP Marc Zyngier
2016-02-11 8:43 ` Christoffer Dall
2016-02-11 10:49 ` Marc Zyngier
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).