diff mbox

[v2,03/25] KVM: arm64: Make kvm_condition_valid32() accessible from EL2

Message ID 20170601102117.17750-4-marc.zyngier@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Marc Zyngier June 1, 2017, 10:20 a.m. UTC
As we're about to trap CP15 accesses and handle them at EL2, we
need to evaluate whether or not the condition flags are valid,
as an implementation is allowed to trap despite the condition
not being met.

Tagging the function as __hyp_text allows this.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/aarch32.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Christoffer Dall June 4, 2017, 12:11 p.m. UTC | #1
On Thu, Jun 01, 2017 at 11:20:55AM +0100, Marc Zyngier wrote:
> As we're about to trap CP15 accesses and handle them at EL2, we
> need to evaluate whether or not the condition flags are valid,
> as an implementation is allowed to trap despite the condition
> not being met.
> 
> Tagging the function as __hyp_text allows this.

is the cc_map also guaranteed to work (by simple reference) in EL2 then?


Thanks,
-Christoffer

> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  virt/kvm/arm/aarch32.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/arm/aarch32.c b/virt/kvm/arm/aarch32.c
> index 528af4b2d09e..79c7c357804b 100644
> --- a/virt/kvm/arm/aarch32.c
> +++ b/virt/kvm/arm/aarch32.c
> @@ -60,7 +60,7 @@ static const unsigned short cc_map[16] = {
>  /*
>   * Check if a trapped instruction should have been executed or not.
>   */
> -bool kvm_condition_valid32(const struct kvm_vcpu *vcpu)
> +bool __hyp_text kvm_condition_valid32(const struct kvm_vcpu *vcpu)
>  {
>  	unsigned long cpsr;
>  	u32 cpsr_cond;
> -- 
> 2.11.0
>
Marc Zyngier June 5, 2017, 8:13 a.m. UTC | #2
On 04/06/17 13:11, Christoffer Dall wrote:
> On Thu, Jun 01, 2017 at 11:20:55AM +0100, Marc Zyngier wrote:
>> As we're about to trap CP15 accesses and handle them at EL2, we
>> need to evaluate whether or not the condition flags are valid,
>> as an implementation is allowed to trap despite the condition
>> not being met.
>>
>> Tagging the function as __hyp_text allows this.
> 
> is the cc_map also guaranteed to work (by simple reference) in EL2 then?

Yes. By virtue of being const, this ends up in the read-only part of the
kernel, which we always map at EL2.

Thanks,

	M.
Christoffer Dall June 5, 2017, 8:23 a.m. UTC | #3
On Mon, Jun 05, 2017 at 09:13:53AM +0100, Marc Zyngier wrote:
> On 04/06/17 13:11, Christoffer Dall wrote:
> > On Thu, Jun 01, 2017 at 11:20:55AM +0100, Marc Zyngier wrote:
> >> As we're about to trap CP15 accesses and handle them at EL2, we
> >> need to evaluate whether or not the condition flags are valid,
> >> as an implementation is allowed to trap despite the condition
> >> not being met.
> >>
> >> Tagging the function as __hyp_text allows this.
> > 
> > is the cc_map also guaranteed to work (by simple reference) in EL2 then?
> 
> Yes. By virtue of being const, this ends up in the read-only part of the
> kernel, which we always map at EL2.
> 

And why don't we have to do any address-translation-to-hyp tricks on the
address?  Are we guaranteed that it's a relative address and everything
is relocated with the same offset, or how was that again?

Thanks,
-Christoffer
Marc Zyngier June 5, 2017, 9:10 a.m. UTC | #4
On 05/06/17 09:23, Christoffer Dall wrote:
> On Mon, Jun 05, 2017 at 09:13:53AM +0100, Marc Zyngier wrote:
>> On 04/06/17 13:11, Christoffer Dall wrote:
>>> On Thu, Jun 01, 2017 at 11:20:55AM +0100, Marc Zyngier wrote:
>>>> As we're about to trap CP15 accesses and handle them at EL2, we
>>>> need to evaluate whether or not the condition flags are valid,
>>>> as an implementation is allowed to trap despite the condition
>>>> not being met.
>>>>
>>>> Tagging the function as __hyp_text allows this.
>>>
>>> is the cc_map also guaranteed to work (by simple reference) in EL2 then?
>>
>> Yes. By virtue of being const, this ends up in the read-only part of the
>> kernel, which we always map at EL2.
>>
> 
> And why don't we have to do any address-translation-to-hyp tricks on the
> address?  Are we guaranteed that it's a relative address and everything
> is relocated with the same offset, or how was that again?

Arghh, I completely missed this. Yeah, it works because this is a single
compilation unit, that the array is static, used only once, and that the
compiler is using relative addressing and not playing any dirty tricks
on us.

We could make the whole thing private to kvm_condition_valid32, but
that's not more of a guarantee that things won't break if the compiler
decides to generate absolute addresses.

Thanks,

	M.
diff mbox

Patch

diff --git a/virt/kvm/arm/aarch32.c b/virt/kvm/arm/aarch32.c
index 528af4b2d09e..79c7c357804b 100644
--- a/virt/kvm/arm/aarch32.c
+++ b/virt/kvm/arm/aarch32.c
@@ -60,7 +60,7 @@  static const unsigned short cc_map[16] = {
 /*
  * Check if a trapped instruction should have been executed or not.
  */
-bool kvm_condition_valid32(const struct kvm_vcpu *vcpu)
+bool __hyp_text kvm_condition_valid32(const struct kvm_vcpu *vcpu)
 {
 	unsigned long cpsr;
 	u32 cpsr_cond;