Re: [PATCH v2 04/11] x86/vmsi: use the newly introduced EOI callbacks

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.09.2020 12:41, Roger Pau Monne wrote:
> Remove the unconditional call to hvm_dpci_msi_eoi in vlapic_handle_EOI
> and instead use the newly introduced EOI callback mechanism in order
> to register a callback for MSI vectors injected from passed through
> devices.

What I'm kind of missing here is a word on why this is an improvement:
After all ...

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -496,8 +496,6 @@ void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
>      if ( vlapic_test_vector(vector, &vlapic->regs->data[APIC_TMR]) )
>          vioapic_update_EOI(vector);
>  
> -    hvm_dpci_msi_eoi(vector);

... you're exchanging this direct call for a more complex model with
an indirect one (to the same function).

> @@ -119,7 +126,8 @@ void vmsi_deliver_pirq(struct domain *d, const struct 
> hvm_pirq_dpci *pirq_dpci)
>  
>      ASSERT(pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI);
>  
> -    vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
> +    vmsi_deliver_callback(d, vector, dest, dest_mode, delivery_mode, 
> trig_mode,
> +                          hvm_dpci_msi_eoi, NULL);
>  }

While I agree with your reply to Paul regarding Dom0, I still think
the entire if() in hvm_dpci_msi_eoi() should be converted into a
conditional here. There's no point registering the callback if it's
not going to do anything.

However, looking further, the "!hvm_domain_irq(d)->dpci &&
!is_hardware_domain(d)" can be simply dropped altogether, right away.
It's now fulfilled by the identical check at the top of
hvm_dirq_assist(), thus guarding the sole call site of this function.

The !is_iommu_enabled(d) is slightly more involved to prove, but it
should also be possible to simply drop. What might help here is a
separate change to suppress opening of HVM_DPCI_SOFTIRQ when there's
no IOMMU in the system, as then it becomes obvious that this part of
the condition is guaranteed by hvm_do_IRQ_dpci(), being the only
site where the softirq can get raised (apart from the softirq
handler itself).

To sum up - the call above can probably stay as is, but the callback
can be simplified as a result of the change.

> --- a/xen/drivers/passthrough/io.c
> +++ b/xen/drivers/passthrough/io.c
> @@ -874,7 +874,7 @@ static int _hvm_dpci_msi_eoi(struct domain *d,
>      return 0;
>  }
>  
> -void hvm_dpci_msi_eoi(unsigned int vector)
> +void hvm_dpci_msi_eoi(unsigned int vector, void *data)
>  {
>      struct domain *d = current->domain;

Instead of passing NULL for data and latching d from current, how
about you make the registration pass d to more easily use here?

Jan