Hi Jason, > -----Original Message----- > From: Jason Gunthorpe <jgg@xxxxxxxxxxxx> > Sent: Wednesday, July 22, 2020 12:59 PM > To: Marc Zyngier <maz@xxxxxxxxxx> > Cc: Jiang, Dave <dave.jiang@xxxxxxxxx>; vkoul@xxxxxxxxxx; Dey, Megha > <megha.dey@xxxxxxxxx>; bhelgaas@xxxxxxxxxx; rafael@xxxxxxxxxx; > gregkh@xxxxxxxxxxxxxxxxxxx; tglx@xxxxxxxxxxxxx; hpa@xxxxxxxxx; > alex.williamson@xxxxxxxxxx; Pan, Jacob jun <jacob.jun.pan@xxxxxxxxx>; Raj, > Ashok <ashok.raj@xxxxxxxxx>; Liu, Yi L <yi.l.liu@xxxxxxxxx>; Lu, Baolu > <baolu.lu@xxxxxxxxx>; Tian, Kevin <kevin.tian@xxxxxxxxx>; Kumar, Sanjay K > <sanjay.k.kumar@xxxxxxxxx>; Luck, Tony <tony.luck@xxxxxxxxx>; Lin, Jing > <jing.lin@xxxxxxxxx>; Williams, Dan J <dan.j.williams@xxxxxxxxx>; > kwankhede@xxxxxxxxxx; eric.auger@xxxxxxxxxx; parav@xxxxxxxxxxxx; > Hansen, Dave <dave.hansen@xxxxxxxxx>; netanelg@xxxxxxxxxxxx; > shahafs@xxxxxxxxxxxx; yan.y.zhao@xxxxxxxxxxxxxxx; pbonzini@xxxxxxxxxx; > Ortiz, Samuel <samuel.ortiz@xxxxxxxxx>; Hossain, Mona > <mona.hossain@xxxxxxxxx>; dmaengine@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; > kvm@xxxxxxxxxxxxxxx > Subject: Re: [PATCH RFC v2 02/18] irq/dev-msi: Add support for a new DEV_MSI > irq domain > > On Wed, Jul 22, 2020 at 07:52:33PM +0100, Marc Zyngier wrote: > > > Which is exactly what platform-MSI already does. Why do we need > > something else? > > It looks to me like all the code is around managing the > dev->msi_domain of the devices. > > The intended use would have PCI drivers create children devices using mdev or > virtbus and those devices wouldn't have a msi_domain from the platform. Looks > like platform_msi_alloc_priv_data() fails immediately because dev->msi_domain > will be NULL for these kinds of devices. > > Maybe that issue should be handled directly instead of wrappering > platform_msi_*? > > For instance a trivial addition to the platform_msi API: > > platform_msi_assign_domain(struct_device *newly_created_virtual_device, > struct device *physical_device); > > Which could set the msi_domain of new device using the topology of > physical_device to deduce the correct domain? > > Then the question is how to properly create a domain within the hardware > topology of physical_device with the correct parameters for the platform. > > Why do we need a dummy msi_domain anyhow? Can this just use > physical_device->msi_domain directly? (I'm at my limit here of how much of this > I remember, sorry) > > If you solve that it should solve the remapping problem too, as the > physical_device is already assigned by the platform to a remapping irq domain if > that is what the platform wants. Yeah most of what you said is right. For the most part, we are simply introducing a new IRQ domain which provides specific domain info ops for the classes of devices which want to provide custom mask/unmask callbacks.. Also, from your other comments, I've realized the same IRQ domain can be used when interrupt remapping is enabled/disabled. Hence we will only have one create_dev_msi_domain which can be called by any device driver that wants to use the dev-msi IRQ domain to alloc/free IRQs. It would be the responsibility of the device driver to provide the correct device and update the dev->msi_domain. > > >> + parent = irq_get_default_host(); > > Really? How is it going to work once you have devices sending their > > MSIs to two different downstream blocks? This looks rather > > short-sighted. > > .. and fix this too, the parent domain should be derived from the topology of the > physical_device which is originating the interrupt messages. > Yes > > On the other hand, masking an interrupt is an irqchip operation, and > > only concerns the irqchip level. Here, you seem to be making it an > > end-point operation, which doesn't really make sense to me. Or is this > > device its own interrupt controller as well? That would be extremely > > surprising, and I'd expect some block downstream of the device to be > > able to control the masking of the interrupt. > > These are message interrupts so they originate directly from the device and > generally travel directly to the CPU APIC. On the wire there is no difference > between a MSI, MSI-X and a device using the dev-msi approach. > > IIRC on Intel/AMD at least once a MSI is launched it is not maskable. > > So the model for MSI is always "mask at source". The closest mapping to the > Linux IRQ model is to say the end device has a irqchip that encapsulates the > ability of the device to generate the MSI in the first place. > > It looks like existing platform_msi drivers deal with "masking" > implicitly by halting the device interrupt generation before releasing the > interrupt and have no way for the generic irqchip layer to mask the interrupt. > > I suppose the motivation to make it explicit is related to vfio using the generic > mask/unmask functionality? > > Explicit seems better, IMHO. I don't think I understand this fully, ive still kept the device specific mask/unmask calls in the next patch series, please let me know if it needs further modifications. > > Jason
