Re: [PATCH v2 2/2] ns16550: enable use of PCI MSI

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Julien Grall <julien.grall@arm.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 2/2] ns16550: enable use of PCI MSI
Date: Mon, 3 Dec 2018 16:39:52 +0100	[thread overview]
Message-ID: <20181203153952.7feetls3yez37pwp@mac> (raw)
In-Reply-To: <5C04EBE60200007800202114@prv1-mh.provo.novell.com>

On Mon, Dec 03, 2018 at 01:40:06AM -0700, Jan Beulich wrote:
> >>> On 30.11.18 at 17:33, <roger.pau@citrix.com> wrote:
> > On Mon, Oct 01, 2018 at 10:26:05AM -0600, Jan Beulich wrote:
> >> --- a/xen/arch/x86/msi.c
> >> +++ b/xen/arch/x86/msi.c
> >> @@ -742,6 +742,16 @@ static int msi_capability_init(struct pc
> >>  
> >>      *desc = entry;
> >>      /* Restore the original MSI enabled bits  */
> >> +    if ( !hardware_domain )
> >> +    {
> >> +        /*
> >> +         * ..., except for internal requests (before Dom0 starts), in which
> >> +         * case we rather need to behave "normally", i.e. not follow the split
> >> +         * brain model where Dom0 actually enables MSI (and disables INTx).
> >> +         */
> >> +        pci_intx(dev, 0);
> >> +        control |= PCI_MSI_FLAGS_ENABLE;
> > 
> > Sorry for the split reply, I've been wondering about the MSI enabling
> > and INTX disabling done here. Xen already owns other PCI devices (AMD
> > IOMMU for example, see set_iommu_interrupt_handler) that use MSI, yet
> > they seem to manage to work without this by doing a manual MSI enable
> > (and I cannot figure out where the INTX disable is done).
> 
> That's because IOMMUs don't normally have a means to signal
> interrupts via a pin. Hence there's nothing to disable.
> 
> > Shouldn't Xen have a more uniform way of dealing with MSI interrupt
> > setup for such devices?
> 
> Perhaps, but the crude way of setting up IOMMU interrupts was
> invented by the CPU vendor engineers; on the AMD side later I
> then re-worked this to re-use at least some of the generic MSI
> code we have.
> 
> > And doesn't your change here imply that some code from the current
> > internal MSI users should be dropped? There's a call to
> > __msi_set_enable in the AMD IOMMU code (amd_iommu_msi_enable) that I
> > guess can be dropped?
> 
> Quite possible (I admit I didn't check in detail), but here I didn't
> want to fiddle with unrelated code.

Ack, I guess it's something to add to the list of nice-to-have.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel