Re: [PATCH 2/5] x86/vlapic: introduce an EOI callback mechanism

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/08/2020 13:47, Roger Pau Monne wrote:
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 7b5c633033..7369be468b 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -159,8 +160,26 @@ void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, 
> uint8_t trig)
>      else
>          vlapic_clear_vector(vec, &vlapic->regs->data[APIC_TMR]);
>  
> +    if ( callback )
> +    {
> +        unsigned long flags;
> +
> +        spin_lock_irqsave(&vlapic->callback_lock, flags);
> +        vlapic->callbacks[vec].callback = callback;
> +        vlapic->callbacks[vec].data = data;
> +        spin_unlock_irqrestore(&vlapic->callback_lock, flags);
> +    }
> +    else
> +        /*
> +         * Removing the callback can be done with a single atomic operation
> +         * without requiring the lock, as the callback data doesn't need to 
> be
> +         * cleared.
> +         */
> +        write_atomic(&vlapic->callbacks[vec].callback, NULL);
> +
>      if ( hvm_funcs.update_eoi_exit_bitmap )
> -        alternative_vcall(hvm_funcs.update_eoi_exit_bitmap, target, vec, 
> trig);
> +        alternative_vcall(hvm_funcs.update_eoi_exit_bitmap, target, vec,
> +                          trig || callback);

I can't reason about this being correct.

When you say EOI, what property do you want, exactly, because there are
a couple of options.

All interrupts, edge or level, require acking at the local APIC, to mark
the interrupt as no longer in service.  For edge interrupts and hardware
APIC acceleration, this will be completed without a VMExit.

Level interrupts from the IO-APIC further require EOI'ing there. 
Whether this requires an explicit action or not depends on the LAPIC
"EOI Broadcast" setting.  I'm not sure if we virtualise and/or support
this setting.


What exactly are we going to want to do from these callbacks
(eventually, not just in this series alone)?

If it can't be made to work for level interrupts only, I'm afraid I
don't think this plan is viable.

(Some trivial comments to follow, but this is the meaty question.)

>  
>      if ( hvm_funcs.deliver_posted_intr )
>          alternative_vcall(hvm_funcs.deliver_posted_intr, target, vec);
> @@ -168,6 +187,11 @@ void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, 
> uint8_t trig)
>          vcpu_kick(target);
>  }
>  
> +void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
> +{
> +    vlapic_set_irq_callback(vlapic, vec, trig, NULL, NULL);

Static inline in the header file?

> @@ -1636,9 +1671,23 @@ int vlapic_init(struct vcpu *v)
>      }
>      clear_page(vlapic->regs);
>  
> +    if ( !vlapic->callbacks )
> +    {
> +        vlapic->callbacks = xmalloc_array(typeof(*(vlapic->callbacks)),
> +                                          X86_NR_VECTORS);

This is a large data structure.  At a minimum, you can subtract 16 from
it, because vectors 0 thru 0xf can't legally be targetted by interrupts.

Sadly, I don't think it can be reduced beyond that, because a guest
could arrange for every other vector to become pending in level mode,
even if the overwhelming common option for VMs these days would be to
have no level interrupts at all.

~Andrew