Re: [PATCH v1 2/2] x86/viridian: Implement synthetic timer direct mode

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On 9/3/26 12:12 PM, Jan Beulich wrote:
> On 28.08.2026 15:11, Ross Lagerwall wrote:
> > In direct mode, the timer asserts an interrupt on expiration rather than
> > using a SynIC message. It is useful to implement this since Windows 11's
> > Hyper-V can only use synthetic timers in direct mode.
> >
> > Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> > ---
> >
> > Should this use a new Viridian feature bit or is it OK to use the
> > existing stimer bit?
> Not sure there. What you need to deal with though are migration related
> aspects:
> - A migrating-in guest should not suddenly see the CPUID bit set when it was
>    clear before.
> - As so far we don't even reject the .direct_mode bit to be set, it being set
>    in any of the MSRs of an incoming, unaware guest needs to be taken care of
>    (the guest must not suddenly get interrupts at the encoded .apic_vector).
> Dealing with this may actually be easier with a new feature bit added.
OK, I'll add a new feature bit to handle this.

> > --- a/xen/arch/x86/hvm/viridian/time.c
> > +++ b/xen/arch/x86/hvm/viridian/time.c
> > @@ -223,6 +223,14 @@ static void start_stimer(struct viridian_stimer *vs)
> >       set_timer(&vs->timer, timeout + NOW());
> >   }
> >   
> > +static void stimer_deliver_direct(struct vcpu *v, struct viridian_stimer *vs)
> Second parameter can be pointer-to-const.
>
> > +{
> > +    struct vlapic *vlapic = vcpu_vlapic(v);
> > +
> > +    if ( vlapic_enabled(vlapic) )
> > +        vlapic_set_irq(vcpu_vlapic(v), vs->config.apic_vector, 0);
> Before you use the vector, you will want to check its validity (along the lines
> of the check that patch 1 aims to avoid in a special case). Whether that needs
> doing at the time the MSR is written by the guest or at the call site I don't
> know: The (oldish) spec I'm looking at doesn't talk about validation of values
> written at all.
>
> Also please don't re-invoke vcpu_vlapic() when you have already latched its
> result in a local variable.
>
> > @@ -372,7 +382,7 @@ int viridian_time_wrmsr(struct vcpu *v, uint32_t idx, 
> > uint64_t val)
> >   
> >           vs->config.as_uint64 = val;
> >   
> > -        if ( !vs->config.sintx || !vs->count )
> > +        if ( (!vs->config.direct_mode && !vs->config.sintx) || !vs->count )
> >               vs->config.enable = 0;
> >   
> >           if ( vs->config.enable )
> > @@ -583,8 +593,11 @@ void viridian_time_load_vcpu_ctxt(
> >   
> >           vs->config.as_uint64 = ctxt->stimer_config_msr[i];
> >           vs->count = ctxt->stimer_count_msr[i];
> > -        if ( !vs->config.sintx || !vs->count )
> > -            /* Reject enabling with a zero sintx or count fields. */
> > +        if ( (!vs->config.direct_mode && !vs->config.sintx) || !vs->count )
> > +            /*
> > +             * Reject enabling with a zero sintx (if not using direct mode) or
> > +             * zero count field.
> > +             */
> >               vs->config.enable = 0;
> >       }
> >   }
> Related to possible validation needs: Is it perhaps also required that .sintx
> be clear when .direct_mode is set?
The relevant part from the most recent published version of the spec [1] says:

"""
It is not permitted to set the SINTx field to zero for an enabled timer (that
is not in direct mode). If attempted, the timer will be marked disabled (that
is, bit 0 cleared) immediately.
"""

It doesn't say anything about validating the APIC vector or disallowing other
combinations of input. However, it seems sensible to do that so I will, with
the same behaviour of disabling the timer on failure.

[1] 
https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/timers

Ross