Re: [PATCH v3 01/11] x86/hvm: drop vcpu parameter from vlapic EOI callbacks

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.03.2021 12:32, Roger Pau Monne wrote:
> EOIs are always executed in guest vCPU context, so there's no reason to
> pass a vCPU parameter around as can be fetched from current.

While not overly problematic, I'd like to point out that there's not a
single vcpu parameter being dropped here - in both cases it's struct
domain *.

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -459,13 +459,10 @@ void vlapic_EOI_set(struct vlapic *vlapic)
>  
>  void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
>  {
> -    struct vcpu *v = vlapic_vcpu(vlapic);
> -    struct domain *d = v->domain;
> -
>      if ( vlapic_test_vector(vector, &vlapic->regs->data[APIC_TMR]) )
> -        vioapic_update_EOI(d, vector);
> +        vioapic_update_EOI(vector);
>  
> -    hvm_dpci_msi_eoi(d, vector);
> +    hvm_dpci_msi_eoi(vector);
>  }

The Viridian path pointed out before was only an example. I'm afraid
the call from vlapic_has_pending_irq() to vlapic_EOI_set() is also
far from obvious that it always has "v == current". What we end up
with here is a mix of passed in value (vlapic) and assumption of the
call being for the vCPU / domain we're running on. At the very least
I think this would want documenting here in some way (maybe ASSERT(),
definitely mentioning in the description), but even better would
perhaps be if the parameter of the function here as well as further
ones involved would also be dropped then.

Jan