Hi Shaokun,
On Mon, 12 Apr 2021 03:22:03 +0100,
Shaokun Zhang <zhangshaokun@xxxxxxxxxxxxx> wrote:
>
> 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.
We already use this function all over the place, and it is already
dragged in via many other paths. Anyway, that's not the biggest
problem.
>
> >
> >> #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.
>
You are missing the point: CPU-A has CCIDX, CPU-B doesn't. My vcpu
runs on CPU-A, gets preempted right after the read of
ID_AA64MMFR2_EL1, and then scheduled on CPU-B. You will now compute
the guest view of CCSIDR_EL1 with CPU-B's value, but interpreting it
with CPU-A's configuration. That's totally broken.
> >
> >> 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.
If you don't consider AArch32 when writing a patch, please don't send
it. AArch32 guests are fully supported, and a patch that breaks them
isn't acceptable.
Also, a RFC patch doesn't mean things are allowed to break. We use RFC
as way to ask for people feedback on a design, but the proposed
implementation has to be a valid one. That's not a license to send
broken stuff.
> 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.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
