On 2020-07-22 20:59, Jason Gunthorpe wrote:
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?
That would seem like a sensible course of action, as losing
the topology information will likely result in problems down
the line.
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)
The parent device would be a PCI device, if I follow you correctly.
It would thus expect to be able to program the MSI the PCI way,
which wouldn't work. So we end-up with this custom MSI domain
that knows about *this* particular family of devices.
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.
+ 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.
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.
I understand that.
IIRC on Intel/AMD at least once a MSI is launched it is not maskable.
Really? So you can't shut a device with a screaming interrupt,
for example, should it become otherwise unresponsive?
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.
This is an x86'ism, I'm afraid. Systems I deal with can mask any
interrupt at the interrupt controller level, MSI or not.
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.
No. As I said above, the interrupt controller is perfectly capable
of masking interrupts on its own, without touching the device.
I suppose the motivation to make it explicit is related to vfio using
the generic mask/unmask functionality?
Explicit seems better, IMHO.
If masking at the source is the only way to shut the device up,
and assuming that the device provides the expected semantics
(a MSI raised by the device while the interrupt is masked
isn't lost and gets sent when unmasked), that's fair enough.
It's just ugly.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
