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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> --- 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?

Jan