On 26/06/2019 06:31, Julien Thierry wrote:
>
>
> On 06/21/2019 10:38 AM, Marc Zyngier wrote:
>> From: Jintack Lim <jintack.lim@xxxxxxxxxx>
>>
>> Forward traps due to HCR_EL2.NV bit to the virtual EL2 if they are not
>> coming from the virtual EL2 and the virtual HCR_EL2.NV bit is set.
>>
>> In addition to EL2 register accesses, setting NV bit will also make EL12
>> register accesses trap to EL2. To emulate this for the virtual EL2,
>> forword traps due to EL12 register accessses to the virtual EL2 if the
>> virtual HCR_EL2.NV bit is set.
>>
>> This is for recursive nested virtualization.
>>
>> Signed-off-by: Jintack Lim <jintack.lim@xxxxxxxxxx>
>> [Moved code to emulate-nested.c]
>> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxx>
>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>> ---
>> arch/arm64/include/asm/kvm_arm.h | 1 +
>> arch/arm64/include/asm/kvm_nested.h | 2 ++
>> arch/arm64/kvm/emulate-nested.c | 28 ++++++++++++++++++++++++++++
>> arch/arm64/kvm/handle_exit.c | 6 ++++++
>> arch/arm64/kvm/sys_regs.c | 18 ++++++++++++++++++
>> 5 files changed, 55 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
>> index 48e15af2bece..d21486274eeb 100644
>> --- a/arch/arm64/include/asm/kvm_arm.h
>> +++ b/arch/arm64/include/asm/kvm_arm.h
>> @@ -24,6 +24,7 @@
>>
>> /* Hyp Configuration Register (HCR) bits */
>> #define HCR_FWB (UL(1) << 46)
>> +#define HCR_NV (UL(1) << 42)
>> #define HCR_API (UL(1) << 41)
>> #define HCR_APK (UL(1) << 40)
>> #define HCR_TEA (UL(1) << 37)
>> diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h
>> index 645e5e11b749..61e71d0d2151 100644
>> --- a/arch/arm64/include/asm/kvm_nested.h
>> +++ b/arch/arm64/include/asm/kvm_nested.h
>> @@ -11,5 +11,7 @@ static inline bool nested_virt_in_use(const struct kvm_vcpu *vcpu)
>> }
>>
>> int handle_wfx_nested(struct kvm_vcpu *vcpu, bool is_wfe);
>> +extern bool forward_traps(struct kvm_vcpu *vcpu, u64 control_bit);
>> +extern bool forward_nv_traps(struct kvm_vcpu *vcpu);
>>
>> #endif /* __ARM64_KVM_NESTED_H */
>> diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
>> index f829b8b04dc8..c406fd688b9f 100644
>> --- a/arch/arm64/kvm/emulate-nested.c
>> +++ b/arch/arm64/kvm/emulate-nested.c
>> @@ -24,6 +24,27 @@
>>
>> #include "trace.h"
>>
>> +bool forward_traps(struct kvm_vcpu *vcpu, u64 control_bit)
>
> Should this one be static?
$ git grep forward_traps
arch/arm64/include/asm/kvm_nested.h:extern bool forward_traps(struct kvm_vcpu *vcpu, u64 control_bit);
arch/arm64/kvm/emulate-nested.c:bool forward_traps(struct kvm_vcpu *vcpu, u64 control_bit)
arch/arm64/kvm/emulate-nested.c: return forward_traps(vcpu, HCR_NV);
arch/arm64/kvm/sys_regs.c: return forward_traps(vcpu, HCR_AT);
arch/arm64/kvm/sys_regs.c: return forward_traps(vcpu, HCR_TTLB);
arch/arm64/kvm/sys_regs.c: return forward_traps(vcpu, HCR_NV1);
I guess not.
>
>> +{
>> + bool control_bit_set;
>> +
>> + if (!nested_virt_in_use(vcpu))
>> + return false;
>> +
>> + control_bit_set = __vcpu_sys_reg(vcpu, HCR_EL2) & control_bit;
>> + if (!vcpu_mode_el2(vcpu) && control_bit_set) {
>> + kvm_inject_nested_sync(vcpu, kvm_vcpu_get_hsr(vcpu));
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> +bool forward_nv_traps(struct kvm_vcpu *vcpu)
>> +{
>> + return forward_traps(vcpu, HCR_NV);
>> +}
>> +
>> +
>> /* This is borrowed from get_except_vector in inject_fault.c */
>> static u64 get_el2_except_vector(struct kvm_vcpu *vcpu,
>> enum exception_type type)
>> @@ -55,6 +76,13 @@ void kvm_emulate_nested_eret(struct kvm_vcpu *vcpu)
>> u64 spsr, elr, mode;
>> bool direct_eret;
>>
>> + /*
>> + * Forward this trap to the virtual EL2 if the virtual
>> + * HCR_EL2.NV bit is set and this is coming from !EL2.
>> + */
>> + if (forward_nv_traps(vcpu))
>> + return;
>> +
>> /*
>> * Going through the whole put/load motions is a waste of time
>> * if this is a VHE guest hypervisor returning to its own
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index 39602a4c1d61..7e8b1ec1d251 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -72,6 +72,12 @@ static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> {
>> int ret;
>>
>> + /*
>> + * Forward this trapped smc instruction to the virtual EL2.
>> + */
>> + if ((vcpu_read_sys_reg(vcpu, HCR_EL2) & HCR_TSC) && forward_nv_traps(vcpu))
>
> Not sure I understand why this would be only when the guest hyp also has
> NV set.
>
> If the guest hyp requested to trap smc instructions and that we received
> one while in vel1, shouldn't we always forward it to the guest hyp to
> let it implement the smc response the way it wants?
There is a small difference, but I'm not sure it matters: If EL3 is not
implemented, SMC UNDEFs at EL1 unless NV is set. So we know here that our
guest is running a guest hypervisor. But does it change a thing?
I need to think about it again... :-(
M.
--
Jazz is not dead. It just smells funny...
