Re: [PATCH] KVM: arm64: nv: Set ISTATUS for emulated timers, If timer expired

[Oliver Upton <oliver.upton@linux.dev>]

On Fri, Feb 07, 2025 at 06:38:22PM +0000, Marc Zyngier wrote:
> On Fri, 07 Feb 2025 18:09:58 +0000,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > Hey,
> > 
> > On Fri, Feb 07, 2025 at 05:45:33PM +0000, Marc Zyngier wrote:
> > > I found at least one issue that could fail the migration. Before the
> > > VM starts running, we limit the feature set to the subset we actually
> > > support with NV.
> > > 
> > > By doing this, we also change the value of IDreg fields that are not
> > > writable, because they describe features that we don't support.
> > > Obviously, that fails on restore.
> > > 
> > > I need to have a think...
> > 
> > We spoke about this a while ago (and I forgot til now), but I was
> > wondering if we could use vCPU feature flags to describe NV, including
> > the selection between FEAT_E2H0 and FEAT_VHE.
> > 
> > I think this might match userspace expectations a bit more closely where
> > the state of the ID registers after init gives the actual feature set
> > supported by the VM.
> 
> I'm not sure that's enough. Let me give you an example:
> 
> My host has FEAT_XNX, described in ID_AA64MMFR1_EL1.XNX. For whatever
> reason, we don't allow this field to be written to, even out of NV
> context. This is odd, because for an EL1 VM, this field means nothing
> at all.
> 
> However, we don't yet support FEAT_XNX with NV (it requires some extra
> surgery in the S2 walker, in the S2 shadowing code and in AT).
> 
> How would you manage this field if you had a vcpu flag saying E2H0 or
> not? I don't think it helps, at least not in that particular case. It
> may help for some things, but not all.
> 
> It feels that we need to define the field limit (and what is writable
> or not) based on the NV/!NV support, and maybe E2H0/VHE as well.

I think we're both aiming in the right direction, I just didn't make my
point clear.

The ID registers we make visible to userspace after KVM_ARM_VCPU_INIT
*must* describe a feature set we will actually honor for the VM. No late
masking of features (i.e. first KVM_RUN), since it pretty much
guarantees userspace won't be able to save/restore correctly.

While we describe NV up front, we've been talking about allowing
userspace to make the E2H0 v. VHE selection through the ID registers as
well. My suggestion is that we have userspace select the flavor of NV
it wants (E2H0 v. VHE) up front and completely nuke from orbit the idea
of late feature fixups.

This would solve your example of XNX, as we'd apply NV enforcement early
and not expose the feature. Same would also hold for VHE-only features,
such as recursive NV.

Hopefully I made more sense this time around :)

-- 
Thanks,
Oliver