Re: [PATCH 01/26] KVM: arm64: Check advertised Stage-2 page size capability

Suzuki K Poulose
2020-04-22T14:10:13Z

On 04/22/2020 03:07 PM, Marc Zyngier wrote:
> Hi Suzuki,
>
> On 2020-04-22 14:40, Suzuki K Poulose wrote:
>> Hi Marc,
>>
>> On 04/22/2020 01:00 PM, Marc Zyngier wrote:
>>> With ARMv8.5-GTG, the hardware (or more likely a hypervisor) can
>>> advertise the supported Stage-2 page sizes.
>>>
>>> Let's check this at boot time.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>   arch/arm64/include/asm/kvm_host.h |  2 +-
>>>   arch/arm64/include/asm/sysreg.h   |  3 +++
>>>   arch/arm64/kernel/cpufeature.c    |  8 +++++++
>>>   arch/arm64/kvm/reset.c            | 40 ++++++++++++++++++++++++++++---
>>>   virt/kvm/arm/arm.c                |  4 +---
>>>   5 files changed, 50 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_host.h
>>> b/arch/arm64/include/asm/kvm_host.h
>>> index 32c8a675e5a4a..7dd8fefa6aecd 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -670,7 +670,7 @@ static inline int kvm_arm_have_ssbd(void)
>>>   void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu);
>>>   void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu);
>>>   -void kvm_set_ipa_limit(void);
>>> +int kvm_set_ipa_limit(void);
>>>     #define __KVM_HAVE_ARCH_VM_ALLOC
>>>   struct kvm *kvm_arch_alloc_vm(void);
>>> diff --git a/arch/arm64/include/asm/sysreg.h
>>> b/arch/arm64/include/asm/sysreg.h
>>> index ebc6224328318..5d10c9148e844 100644
>>> --- a/arch/arm64/include/asm/sysreg.h
>>> +++ b/arch/arm64/include/asm/sysreg.h
>>> @@ -686,6 +686,9 @@
>>>   #define ID_AA64ZFR0_SVEVER_SVE2        0x1
>>>     /* id_aa64mmfr0 */
>>> +#define ID_AA64MMFR0_TGRAN4_2_SHIFT    40
>>> +#define ID_AA64MMFR0_TGRAN64_2_SHIFT    36
>>> +#define ID_AA64MMFR0_TGRAN16_2_SHIFT    32
>>>   #define ID_AA64MMFR0_TGRAN4_SHIFT    28
>>>   #define ID_AA64MMFR0_TGRAN64_SHIFT    24
>>>   #define ID_AA64MMFR0_TGRAN16_SHIFT    20
>>> diff --git a/arch/arm64/kernel/cpufeature.c
>>> b/arch/arm64/kernel/cpufeature.c
>>> index 9fac745aa7bb2..9892a845d06c9 100644
>>> --- a/arch/arm64/kernel/cpufeature.c
>>> +++ b/arch/arm64/kernel/cpufeature.c
>>> @@ -208,6 +208,14 @@ static const struct arm64_ftr_bits
>>> ftr_id_aa64zfr0[] = {
>>>   };
>>>     static const struct arm64_ftr_bits ftr_id_aa64mmfr0[] = {
>>> +    /*
>>> +     * Page size not being supported at Stage-2 are not fatal. You
>>> +     * just give up KVM if PAGE_SIZE isn't supported there. Go fix
>>> +     * your favourite nesting hypervisor.
>>> +     */
>>> +    ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_EXACT,
>>> ID_AA64MMFR0_TGRAN4_2_SHIFT, 4, 1),
>>> +    ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_EXACT,
>>> ID_AA64MMFR0_TGRAN64_2_SHIFT, 4, 1),
>>> +    ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_EXACT,
>>> ID_AA64MMFR0_TGRAN16_2_SHIFT, 4, 1),
>>
>> One minor issue with this is, if we get a system with cpus having values
>> 0 and 2 (both of which indicates the stage-2 support), we might reset
>> the value to 1 for the feature indicating, we don't support and block
>> KVM. But, we can blame the nesting hypervisor for not emulating this to
>> (2). Do we need a comment to make this explicit here ?
>
> Sure. How about something like:
>
> "There is a small corner case where the hypervisor could explicitly
> advertise
>  a given granule size at Stage-2 (value 2) on some vCPUs, and use the
> fallback
>  to Stage-1 (value 0) for other vCPUs. Although this is not forbidden
> by the
>  architecture, it indicates that the hypervisor is being silly (or buggy).
>  We make no effort to cope with this and pretend that if these fields are
>  inconsistent across vCPUs, then it isn't worth trying to bring KVM up."
>

Looks fine to me.

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>