Re: [PATCH] x86/viridian: use hv_timer_message_payload struct

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Nov 13, 2025 at 05:52:47PM +0000, Andrew Cooper wrote:
> On 13/11/2025 5:24 pm, Roger Pau Monne wrote:
> > diff --git a/xen/arch/x86/hvm/viridian/synic.c 
> > b/xen/arch/x86/hvm/viridian/synic.c
> > index e6cba7548f1b..6d7b6bd0eda2 100644
> > --- a/xen/arch/x86/hvm/viridian/synic.c
> > +++ b/xen/arch/x86/hvm/viridian/synic.c
> > @@ -327,15 +327,10 @@ bool viridian_synic_deliver_timer_msg(struct vcpu *v, 
> > unsigned int sintx,
> >      struct viridian_vcpu *vv = v->arch.hvm.viridian;
> >      const union hv_synic_sint *vs = &vv->sint[sintx];
> >      struct hv_message *msg = vv->simp.ptr;
> > -    struct {
> > -        uint32_t TimerIndex;
> > -        uint32_t Reserved;
> > -        uint64_t ExpirationTime;
> > -        uint64_t DeliveryTime;
> > -    } payload = {
> > -        .TimerIndex = index,
> > -        .ExpirationTime = expiration,
> > -        .DeliveryTime = delivery,
> > +    const struct hv_timer_message_payload payload = {
> > +        .timer_index = index,
> > +        .expiration_time = expiration,
> > +        .delivery_time = delivery,
> 
> Align these = for readability?

Sure, can do.

> >      };
> >  
> >      /* Don't assume SIM page to be mapped. */
> > @@ -359,8 +354,8 @@ bool viridian_synic_deliver_timer_msg(struct vcpu *v, 
> > unsigned int sintx,
> >      msg->header.message_flags.msg_pending = 0;
> >      msg->header.payload_size = sizeof(payload);
> >  
> > -    BUILD_BUG_ON(sizeof(payload) > sizeof(msg->u.payload));
> > -    memcpy(msg->u.payload, &payload, sizeof(payload));
> > +    BUILD_BUG_ON(sizeof(msg->payload.timer) > sizeof(msg->payload.raw));
> 
> This BUILD_BUG_ON() was only needed because of the memcpy() between
> different types.  With structure assignment, the compiler will tell you
> if the type mismatches.

I've keep it to ensure the size of the hv_timer_message_payload
doesn't exceed the maximum payload size (240 bytes), as
msg->payload.raw is the maximum payload size defined by the standard.

> Therefore, it's safe to drop.
> 
> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Let me know if you are fine with keeping the BUILD_BUG_ON() given the
justification above, as that would be my preference.

Thanks, Roger.