Re: [PATCH V2] arm64/cpufeature: Validate hypervisor capabilities during CPU hotplug
On 05/09/2020 04:29 PM, Marc Zyngier wrote:
> On Fri, 08 May 2020 03:52:38 +0100,
> Anshuman Khandual wrote:
>>
>> This validates hypervisor capabilities like VMID width, IPA range for any
>> hot plug CPU against system finalized values. While here, it factors out
>> get_vmid_bits() for general use and also defines ID_AA64MMFR0_PARANGE_MASK.
>
> Maybe add a quick word on the fact that we use KVM's view of the IPA
> space to allow a CPU to come up.
Sure, will do.
>
>>
>> Cc: Catalin Marinas
>> Cc: Will Deacon
>> Cc: Marc Zyngier
>> Cc: Mark Rutland
>> Cc: James Morse
>> Cc: Suzuki K Poulose
>> Cc: linux-arm-ker...@lists.infradead.org
>> Cc: kvmarm@lists.cs.columbia.edu
>> Cc: linux-ker...@vger.kernel.org
>>
>> Suggested-by: Suzuki Poulose
>> Signed-off-by: Anshuman Khandual
>> ---
>> Changes in V2:
>>
>> - Added is_hyp_mode_available() check per Marc
>> - Moved verify_kvm_capabilities() into cpufeature.c per Marc
>> - Added helper get_kvm_ipa_limit() to fetch kvm_ipa_limit per Marc
>> - Renamed kvm as hyp including the commit message per Marc
>>
>> Changes in V1: (https://patchwork.kernel.org/patch/11532565/)
>>
>> arch/arm64/include/asm/cpufeature.h | 20 +
>> arch/arm64/include/asm/kvm_mmu.h| 2 +-
>> arch/arm64/include/asm/sysreg.h | 1 +
>> arch/arm64/kernel/cpufeature.c | 33 +
>> arch/arm64/kvm/reset.c | 11 --
>> 5 files changed, 64 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/cpufeature.h
>> b/arch/arm64/include/asm/cpufeature.h
>> index afe08251ff95..fbbb4d2216f0 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -745,6 +745,26 @@ static inline bool cpu_has_hw_af(void)
>> extern bool cpu_has_amu_feat(int cpu);
>> #endif
>>
>> +static inline unsigned int get_vmid_bits(u64 mmfr1)
>> +{
>> +int vmid_bits;
>> +
>> +vmid_bits = cpuid_feature_extract_unsigned_field(mmfr1,
>> +ID_AA64MMFR1_VMIDBITS_SHIFT);
>> +if (vmid_bits == ID_AA64MMFR1_VMIDBITS_16)
>> +return 16;
>> +
>> +/*
>> + * Return the default here even if any reserved
>> + * value is fetched from the system register.
>> + */
>> +return 8;
>> +}
>> +
>> +#ifdef CONFIG_KVM_ARM_HOST
>
> nit: useless #ifdefery.
Dropped.
>
>> +u32 get_kvm_ipa_limit(void);
>> +#endif
>> +
>> #endif /* __ASSEMBLY__ */
>>
>> #endif
>> diff --git a/arch/arm64/include/asm/kvm_mmu.h
>> b/arch/arm64/include/asm/kvm_mmu.h
>> index 30b0e8d6b895..a7137e144b97 100644
>> --- a/arch/arm64/include/asm/kvm_mmu.h
>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>> @@ -416,7 +416,7 @@ static inline unsigned int kvm_get_vmid_bits(void)
>> {
>> int reg = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1);
>>
>> -return (cpuid_feature_extract_unsigned_field(reg,
>> ID_AA64MMFR1_VMIDBITS_SHIFT) == 2) ? 16 : 8;
>> +return get_vmid_bits(reg);
>> }
>>
>> /*
>> diff --git a/arch/arm64/include/asm/sysreg.h
>> b/arch/arm64/include/asm/sysreg.h
>> index c4ac0ac25a00..3510a4668970 100644
>> --- a/arch/arm64/include/asm/sysreg.h
>> +++ b/arch/arm64/include/asm/sysreg.h
>> @@ -705,6 +705,7 @@
>> #define ID_AA64MMFR0_TGRAN16_SUPPORTED 0x1
>> #define ID_AA64MMFR0_PARANGE_48 0x5
>> #define ID_AA64MMFR0_PARANGE_52 0x6
>> +#define ID_AA64MMFR0_PARANGE_MASK 0x7
>
> I realise this is already like this in the current code, but using 7
> as a mask value for the feature feels wrong. If we ever get a value
> with bit 3 of the capability being set, we will confuse it with some
> other configuration.
>
> We should be more careful and pass the full value of the feature to
> id_aa64mmfr0_parange_to_phys_shift(), which already does the right
> thing.
So we should instead pass complete SYS_ID_AA64MMFR0_EL1 value (sanitized)
and do the masking inside id_aa64mmfr0_parange_to_phys_shift(), probably
dropping "_parange_to" from it's name. But kvm_arm_setup_stage2() fetches
only parange not IPA range. Otherwise a new helper id_aa64mmfr0_parange()
which takes full SYS_ID_AA64MMFR0_EL1 value and does the masking before
returning the parange could achieve the same result i.e localizing this
parange mask.
Actually, we could have both the above changes i.e there will be following
two helpers with ID_AA64MMFR0_PARANGE_MASK defined locally.
1. id_aa64mmfr0_phys_shift (u64 mmfr0)
2. id_aa64mmfr0_parange (u64 mmfr0)
Thoughts ?
>
>>
>> #ifdef CONFIG_ARM64_PA_BITS_52
>> #define ID_AA64MMFR0_PARANGE_MAXID_AA64MMFR0_PARANGE_52
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index 9fac745aa7bb..7e5ff452574c 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -2181,6 +2181,36 @@ static void verify_sve_features(void)
>> /* Add checks on other ZCR bits here if necessary */
>> }
>>
>> +#ifdef CONFIG_KVM_ARM_HOST
>> +void
Re: [PATCH 03/15] arm64: kvm: Fix symbol dependency in __hyp_call_panic_nvhe
Hi David, On Thu, 07 May 2020 15:36:17 +0100, David Brazdil wrote: > > Hi Marc, > > > > > What breaks without this constraint? Is it a fix that should go in > > early? Otherwise looks good. > > This only becomes an issue when __hyp_call_panic_nvhe() and > __hyp_call_panic_vhe() are moved to separate files, so I don't think > it's necessary to go in early. > > Currently the string variable (declared static) is seen by the C > compiler as used by __hyp_call_panic_vhe(). But when split, the > variable in the nVHE source file becomes unused, is dropped by the > compiler and the inline assembly's reference is unresolved. We could > then alias __hyp_text___hyp_panic_string back to the VHE copy, but > this is the right way of addressing it. Thanks for the detailed explanation. I think some of it should make it in the commit message, pointing what breaks and when. M. -- Without deviation from the norm, progress is not possible. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 02/15] arm64: kvm: Formalize host-hyp hypcall ABI
Hi Quentin,
On Thu, 07 May 2020 14:33:20 +0100,
Quentin Perret wrote:
>
> Hey Marc,
>
> On Thursday 07 May 2020 at 14:10:30 (+0100), Marc Zyngier wrote:
> > Hi David, Quentin,
[...]
> > > +#define __KVM_HOST_HCALL_TABLE_IDX___vgic_v3_save_aprs 13
> > > +__KVM_HOST_HCALL(__vgic_v3_save_aprs)
> > > +
> > > +#define __KVM_HOST_HCALL_TABLE_IDX_SIZE 14
> >
> > This whole thing screams "enum" into my ears. Having to maintain these
> > as #defines feels like a pain, specially if the numbers are never used
> > in assembly code. (and for that, we have asm-offset.h).
>
> This is essentially inspired from the various 'unistd.h' files we have
> in the kernel, e.g. include/uapi/asm-generic/unistd.h, which have
> exactly this type of construct. So, this was really just for consistency,
> but no strong opinion from me.
I think part of the of the reason for not using an enum in the syscall
code is that part of is is (was?) used from assembly code. In our
case, I'm pretty sure we won't go back to handling calls from asm any
time soon, so a generated enum (and associated function pointer array)
would be better.
>
> >
> > > +
> > > +/* XXX - Arbitrary offset for KVM-related hypercalls */
> > > +#define __KVM_HOST_HCALL_BASE_SHIFT 8
> > > +#define __KVM_HOST_HCALL_BASE (1ULL << __KVM_HOST_HCALL_BASE_SHIFT)
> > > +#define __KVM_HOST_HCALL_END (__KVM_HOST_HCALL_BASE + \
> > > + __KVM_HOST_HCALL_TABLE_IDX_SIZE)
> >
> > I don't really get what is this offset for. It is too small to be used
> > to skip the stub hypercalls (you'd need to start at 0x1000).
>
> Oh, OK. I assumed anything above HVC_STUB_HCALL_NR would be fine. But
> yes, this offset is really arbitrary, so if 0x1000 is better then that
> totally works. For my education, where is that 0x1000 coming from ?
We assumed that we wouldn't get a function pointer in the first 4kB,
and documented as such in hyp.S. I would say that either we keep the
current convention of leaving the first 4k function codes for the
hyp-stubs, or we drop any sort of gap.
Another thing to consider is that there is at least *one* external
hypervisor (Jailhouse) that uses the stubs, so I'd like to make sure
we don't break that (even if we made no promise whatsoever in that
respect).
>
> > Given
> > that you store the whole thing in an array, you're wasting some
> > memory. I'm sure you have a good story for it though! ;-)
>
> Note that the array's length is __KVM_HOST_HCALL_TABLE_IDX_SIZE, which
> doesn't include the offset, so we shouldn't be wasting memory here.
Ah, you're right. I got confused between _SIZE and _END.
>
> > > +
> > > +/* Hypercall number = kvm offset + table idx */
> > > +#define KVM_HOST_HCALL_NR(name) (__KVM_HOST_HCALL_TABLE_IDX_##name + \
> > > + __KVM_HOST_HCALL_BASE)
> > > diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> > > index 8c9880783839..29e2b2cd2fbc 100644
> > > --- a/arch/arm64/kvm/hyp/Makefile
> > > +++ b/arch/arm64/kvm/hyp/Makefile
> > > @@ -9,7 +9,7 @@ ccflags-y += -fno-stack-protector
> > > -DDISABLE_BRANCH_PROFILING \
> > > obj-$(CONFIG_KVM) += hyp.o
> > >
> > > hyp-y := vgic-v3-sr.o timer-sr.o aarch32.o vgic-v2-cpuif-proxy.o
> > > sysreg-sr.o \
> > > - debug-sr.o entry.o switch.o fpsimd.o tlb.o hyp-entry.o
> > > + debug-sr.o entry.o switch.o fpsimd.o tlb.o host_hypercall.o hyp-entry.o
> > >
> > > # KVM code is run at a different exception code with a different map, so
> > > # compiler instrumentation that inserts callbacks or checks into the
> > > code may
> > > diff --git a/arch/arm64/kvm/hyp/host_hypercall.c
> > > b/arch/arm64/kvm/hyp/host_hypercall.c
> > > new file mode 100644
> > > index ..6b31310f36a8
> > > --- /dev/null
> > > +++ b/arch/arm64/kvm/hyp/host_hypercall.c
> > > @@ -0,0 +1,22 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2020 Google, inc
> > > + */
> > > +
> > > +#include
> > > +
> > > +typedef long (*kvm_hcall_fn_t)(void);
> > > +
> > > +static long __hyp_text kvm_hc_ni(void)
> > > +{
> > > + return -ENOSYS;
> > > +}
> > > +
> > > +#undef __KVM_HOST_HCALL
> > > +#define __KVM_HOST_HCALL(name) \
> > > + [__KVM_HOST_HCALL_TABLE_IDX_##name] = (long (*)(void))name,
> > > +
> > > +const kvm_hcall_fn_t kvm_hcall_table[__KVM_HOST_HCALL_TABLE_IDX_SIZE] = {
> > > + [0 ... __KVM_HOST_HCALL_TABLE_IDX_SIZE-1] = kvm_hc_ni,
> > > +#include
> > > +};
> >
> > Cunning. At the same time, if you can do this once, you can do it
> > twice, and generating the __KVM_HOST_HCALL_TABLE_IDX_* as an enum
> > should be pretty easy, and could live in its own include file.
>
> Right, and that again is inspired from the arm64 syscall table (see
> arch/arm64/kernel/sys.c). So the first intention was to keep things
> consistent. But again, no strong opinion :)
So let's try to build it with an enum instead, and see if it works. If
it doesn't, at least we will know why.
[...]
