Hey Reiji,
On Mon, Jan 31, 2022 at 10:00:40PM -0800, Reiji Watanabe wrote:
> Hi Ricardo,
>
> On Sun, Jan 30, 2022 at 7:40 PM Ricardo Koller <ricarkol@xxxxxxxxxx> wrote:
> >
> > On Fri, Jan 28, 2022 at 09:52:21PM -0800, Reiji Watanabe wrote:
> > > Hi Ricardo,
> > >
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Set the guest's ID registers that are defined in sys_reg_descs[]
> > > > > > > + * with ID_SANITISED() to the host's sanitized value.
> > > > > > > + */
> > > > > > > +void set_default_id_regs(struct kvm *kvm)
> > > > > > > +{
> > > > > > > + int i;
> > > > > > > + u32 id;
> > > > > > > + const struct sys_reg_desc *rd;
> > > > > > > + u64 val;
> > > > > > > +
> > > > > > > + for (i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) {
> > > > > > > + rd = &sys_reg_descs[i];
> > > > > > > + if (rd->access != access_id_reg)
> > > > > > > + /* Not ID register, or hidden/reserved ID register */
> > > > > > > + continue;
> > > > > > > +
> > > > > > > + id = reg_to_encoding(rd);
> > > > > > > + if (WARN_ON_ONCE(!is_id_reg(id)))
> > > > > > > + /* Shouldn't happen */
> > > > > > > + continue;
> > > > > > > +
> > > > > > > + val = read_sanitised_ftr_reg(id);
> > > > > >
> > > > > > I'm a bit confused. Shouldn't the default+sanitized values already use
> > > > > > arm64_ftr_bits_kvm (instead of arm64_ftr_regs)?
> > > > >
> > > > > I'm not sure if I understand your question.
> > > > > arm64_ftr_bits_kvm is used for feature support checkings when
> > > > > userspace tries to modify a value of ID registers.
> > > > > With this patch, KVM just saves the sanitized values in the kvm's
> > > > > buffer, but userspace is still not allowed to modify values of ID
> > > > > registers yet.
> > > > > I hope it answers your question.
> > > >
> > > > Based on the previous commit I was assuming that some registers, like
> > > > id_aa64dfr0,
> > > > would default to the overwritten values as the sanitized values. More
> > > > specifically: if
> > > > userspace doesn't modify any ID reg, shouldn't the defaults have the
> > > > KVM overwritten
> > > > values (arm64_ftr_bits_kvm)?
> > >
> > > arm64_ftr_bits_kvm doesn't have arm64_ftr_reg but arm64_ftr_bits,
> > > and arm64_ftr_bits_kvm doesn't have the sanitized values.
> > >
> > > Thanks,
> >
> > Hey Reiji,
> >
> > Sorry, I wasn't very clear. This is what I meant.
> >
> > If I set DEBUGVER to 0x5 (w/ FTR_EXACT) using this patch on top of the
> > series:
> >
> > static struct arm64_ftr_bits ftr_id_aa64dfr0_kvm[MAX_FTR_BITS_LEN] = {
> > S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_PMUVER_SHIFT, 4, 0),
> > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6),
> > + ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x5),
> >
> > it means that userspace would not be able to set DEBUGVER to anything
> > but 0x5. But I'm not sure what it should mean for the default KVM value
> > of DEBUGVER, specifically the value calculated in set_default_id_regs().
> > As it is, KVM is still setting the guest-visible value to 0x6, and my
> > "desire" to only allow booting VMs with DEBUGVER=0x5 is being ignored: I
> > booted a VM and the DEBUGVER value from inside is still 0x6. I was
> > expecting it to not boot, or to show a warning.
>
> Thank you for the explanation!
>
> FTR_EXACT (in the existing code) means that the safe_val should be
> used if values of the field are not identical between CPUs (see how
> update_cpu_ftr_reg() uses arm64_ftr_safe_value()). For KVM usage,
> it means that if the field value for a vCPU is different from the one
> for the host's sanitized value, only the safe_val can be used safely
> for the guest (purely in terms of CPU feature).
Let me double check my understanding using the DEBUGVER example, please.
The safe_value would be DEBUGVER=5, and it contradicts the initial VM
value calculated on the KVM side. Q1: Can a contradiction like this
occur in practice? Q2: If the user saves and restores this id-reg on the
same kernel, the AA64DFR0 userspace write would fail (ftr_val !=
arm64_ftr_safe_value), right?
>
> If KVM wants to restrict some features due to some reasons (e.g.
> a feature for guests is not supported by the KVM yet), it should
> be done by KVM (not by cpufeature.c), and 'validate' function in
> "struct id_reg_info", which is introduced in patch-3, will be used
> for such cases (the following patches actually use).
>
Got it, thanks.
> Thanks,
> Reiji
>
Thanks,
Ricardo
> >
> > I think this has some implications for migrations. It would not be
> > possible to migrate the example VM on the patched kernel from above: you
> > can boot a VM with DEBUGVER=0x5 but you can't migrate it.
> >
> > Thanks,
> > Ricardo
