Hi Marc,
On 2021/4/10 17:54, Marc Zyngier wrote:
> On Sat, 10 Apr 2021 09:16:37 +0100,
> Shaokun Zhang <zhangshaokun@xxxxxxxxxxxxx> wrote:
>>
>> CCSIDR_EL1 can be implemented as 64-bit format inferred by CCIDX field
>> in ID_AA64MMFR2_EL1. The bits of Numsets and Associativity are different
>> from the 32-bit format. Let's support this feature.
>>
>> Cc: Marc Zyngier <maz@xxxxxxxxxx>
>> Cc: James Morse <james.morse@xxxxxxx>
>> Cc: Alexandru Elisei <alexandru.elisei@xxxxxxx>
>> Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>> Signed-off-by: Shaokun Zhang <zhangshaokun@xxxxxxxxxxxxx>
>> ---
>> arch/arm64/kvm/sys_regs.c | 27 +++++++++++++++++++--------
>> 1 file changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 52fdb9a015a4..0dc822cef20b 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -18,6 +18,7 @@
>>
>> #include <asm/cacheflush.h>
>> #include <asm/cputype.h>
>> +#include <asm/cpufeature.h>
>
> If you are going to add this (why?), at least add it in alphabetic order.
cpuid_feature_extract_unsigned_field will be used later.
It shall do in alphabetic order.
>
>> #include <asm/debug-monitors.h>
>> #include <asm/esr.h>
>> #include <asm/kvm_arm.h>
>> @@ -95,9 +96,9 @@ static u32 cache_levels;
>> #define CSSELR_MAX 14
>>
>> /* Which cache CCSIDR represents depends on CSSELR value. */
>> -static u32 get_ccsidr(u32 csselr)
>> +static u64 get_ccsidr(u32 csselr)
>> {
>> - u32 ccsidr;
>> + u64 ccsidr;
>>
>> /* Make sure noone else changes CSSELR during this! */
>> local_irq_disable();
>> @@ -1275,12 +1276,16 @@ static bool access_csselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> static bool access_ccsidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> const struct sys_reg_desc *r)
>> {
>> - u32 csselr;
>> + u32 csselr, ccidx;
>> + u64 mmfr2;
>>
>> if (p->is_write)
>> return write_to_read_only(vcpu, p, r);
>>
>> csselr = vcpu_read_sys_reg(vcpu, CSSELR_EL1);
>> + mmfr2 = read_sysreg_s(SYS_ID_AA64MMFR2_EL1);
>> + ccidx = cpuid_feature_extract_unsigned_field(mmfr2,
>> + ID_AA64MMFR2_CCIDX_SHIFT);
>
> What happens on an asymmetric system where only some of the CPUs have
> FEAT_CCIDX?
If other CPUs don't have FEAT_CCIDX, its value is 0b0000 while CCSIDR_EL1 is 32-bit format.
>
>> p->regval = get_ccsidr(csselr);
>>
>> /*
>> @@ -1295,8 +1300,13 @@ static bool access_ccsidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>> * geometry (which is not permitted by the architecture), they would
>> * only do so for virtually indexed caches.]
>> */
>> - if (!(csselr & 1)) // data or unified cache
>> - p->regval &= ~GENMASK(27, 3);
>> + if (!(csselr & 1)) { // data or unified cache
>> + if (ccidx)
>> + p->regval &= ~(GENMASK(23, 3) | GENMASK(55, 32));
>> + else
>> + p->regval &= ~GENMASK(27, 3);
>> + }
>> +
>> return true;
>> }
>>
>> @@ -2521,7 +2531,7 @@ static bool is_valid_cache(u32 val)
>> static int demux_c15_get(u64 id, void __user *uaddr)
>> {
>> u32 val;
>> - u32 __user *uval = uaddr;
>> + u64 __user *uval = uaddr;
>
> What? Has the user API changed while I wasn't looking? Getting CCSIDR
> can only return a 32bit quantity on AArch32. In fact, CCSIDR is
> *always* 32bit, CCIDX or not. I have no idea what you are trying to do
> here, but at best you are now corrupting 32bit of userspace.
>
Oops, I really missed this.
>>
>> /* Fail if we have unknown bits set. */
>> if (id & ~(KVM_REG_ARCH_MASK|KVM_REG_SIZE_MASK|KVM_REG_ARM_COPROC_MASK
>> @@ -2545,8 +2555,9 @@ static int demux_c15_get(u64 id, void __user *uaddr)
>>
>> static int demux_c15_set(u64 id, void __user *uaddr)
>> {
>> - u32 val, newval;
>> - u32 __user *uval = uaddr;
>> + u32 val;
>> + u64 newval;
>> + u64 __user *uval = uaddr;
>
> Same brokenness.
>
>>
>> /* Fail if we have unknown bits set. */
>> if (id & ~(KVM_REG_ARCH_MASK|KVM_REG_SIZE_MASK|KVM_REG_ARM_COPROC_MASK
>
> What about CCSIDR2_EL1, which is mandatory when FEAT_CCSIDX is
> present? How about the AArch32 handling of that register? I don't
> think you have though this one through.
To be honest, AArch32 is not considered directly and I sent this only
as RFC. When I wrote some flush cache code using by set/way mode and
noticed that CCSIDR_EL1 is used here.
>
> Another question is: why should we care at all? Why don't we drop all
> that and only implement a virtual cache topology? A VM shouldn't care
> at all about this, and we are already lying about the topology anyway.
> We could even get the VMM to set whatever stupid topology it wants for
> the sake of it (and to restore previously created VMs).
Got it,
Thanks for your detailed comments.
Shaokun
>
> M.
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
