Re: [PATCH v3 05/11] x86/vioapic: switch to use the EOI callback mechanism

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

On Wed, Apr 07, 2021 at 05:19:06PM +0200, Jan Beulich wrote:
> On 31.03.2021 12:32, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/vioapic.c
> > +++ b/xen/arch/x86/hvm/vioapic.c
> > @@ -621,7 +624,43 @@ static int ioapic_load(struct domain *d, 
> > hvm_domain_context_t *h)
> >           d->arch.hvm.nr_vioapics != 1 )
> >          return -EOPNOTSUPP;
> >  
> > -    return hvm_load_entry(IOAPIC, h, &s->domU);
> > +    rc = hvm_load_entry(IOAPIC, h, &s->domU);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    for ( i = 0; i < ARRAY_SIZE(s->domU.redirtbl); i++ )
> > +    {
> > +        const union vioapic_redir_entry *ent = &s->domU.redirtbl[i];
> > +        unsigned int vector = ent->fields.vector;
> > +        unsigned int delivery_mode = ent->fields.delivery_mode;
> > +        struct vcpu *v;
> > +
> > +        /*
> > +         * Add a callback for each possible vector injected by a 
> > redirection
> > +         * entry.
> > +         */
> > +        if ( vector < 16 || !ent->fields.remote_irr ||
> > +             (delivery_mode != dest_LowestPrio && delivery_mode != 
> > dest_Fixed) )
> > +            continue;
> > +
> > +        for_each_vcpu ( d, v )
> > +        {
> > +            struct vlapic *vlapic = vcpu_vlapic(v);
> > +
> > +            /*
> > +             * NB: if the vlapic registers were restored before the 
> > vio-apic
> > +             * ones we could test whether the vector is set in the vlapic 
> > IRR
> > +             * or ISR registers before unconditionally setting the 
> > callback.
> > +             * This is harmless as eoi_callback is capable of dealing with
> > +             * spurious callbacks.
> > +             */
> > +            if ( vlapic_match_dest(vlapic, NULL, 0, ent->fields.dest_id,
> > +                                   ent->fields.dest_mode) )
> > +                vlapic_set_callback(vlapic, vector, eoi_callback, NULL);
> 
> eoi_callback()'s behavior is only one of the aspects to consider here.
> The other is vlapic_set_callback()'s complaining if it finds a
> callback already set. What guarantees that a mistakenly set callback
> here won't get in conflict with some future use of the same vector by
> the guest?

Such conflict would only manifest as a warning message, but won't
cause any malfunction, as the later callback would override the
current one.

This model I'm proposing doesn't support lapic vector sharing with
different devices that require EOI callbacks, I think we already
discussed this on a previous series and agreed it was fine.

> And btw - like in the earlier patch you could again pass d instead of
> NULL here, avoiding the need to establish it from current in the
> callback.

On the new version the vlapic callback gets passed a vcpu parameter,
as I will drop the prepatches to remove passing a domain parameter to
vioapic_update_EOI.

> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -192,7 +192,13 @@ void vlapic_set_irq_callback(struct vlapic *vlapic, 
> > uint8_t vec, uint8_t trig,
> >  
> >      if ( hvm_funcs.update_eoi_exit_bitmap )
> >          alternative_vcall(hvm_funcs.update_eoi_exit_bitmap, target, vec,
> > -                          trig || callback);
> > +                          /*
> > +                           * NB: need to explicitly convert to boolean to 
> > avoid
> > +                           * truncation wrongly result in false begin 
> > reported
> > +                           * for example when the pointer sits on a page
> > +                           * boundary.
> > +                           */
> > +                          !!callback);
> 
> I've had quite a bit of difficulty with the comment. Once I realized
> that you likely mean "being" instead of "begin" it got a bit better.
> I'd like to suggest also s/result/resulting/, a comma after "reported",
> and maybe then s/being reported/getting passed/.
> 
> As to explicitly converting to bool, wouldn't a cast to bool do? That's
> more explicitly an "explicit conversion" than using !!.

I've always used !! in the past for such cases because it's shorter, I
can explicitly cast if you prefer that instead.

Thanks, Roger.