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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: <xen-devel@lists.xenproject.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	Paul Durrant <paul@xen.org>
Subject: Re: [PATCH v2 04/11] x86/vmsi: use the newly introduced EOI callbacks
Date: Tue, 13 Oct 2020 16:47:24 +0200	[thread overview]
Message-ID: <20201013144724.GR19254@Air-de-Roger> (raw)
In-Reply-To: <785f80d6-3a0a-6a58-fd9a-05d8ff87f6fe@suse.com>

On Fri, Oct 02, 2020 at 05:25:34PM +0200, Jan Beulich wrote:
> 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).

Sure. But this direct call will be made for each vlapic EOI, while my
added callback will only be executed if the vector was injected by
thee vmsi code, and hence will remove pointless calls to
hvm_dpci_msi_eoi.

It's IMO not feasible to be adding hardcoded calls to
vlapic_handle_EOI for each possible subsystem or emulated device that
wants to be notified of EOIs, hence we need some kind of generic
framework to achieve this.

> > @@ -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.

Yes, I agree. Would you be fine with converting the check in the
callback into an assert, or would you rather have it removed
completely?

> > --- 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?

Yes, I think that's fine - we already have the domain pointer in
vmsi_deliver_callback so it could be passed as the callback private
data.

Thanks, Roger.