Re: [patch 13/33] x86/apic/vector: Provide MSI parent domain

[Thomas Gleixner]

On Wed, Nov 16 2022 at 15:18, Jason Gunthorpe wrote:
> On Fri, Nov 11, 2022 at 02:58:31PM +0100, Thomas Gleixner wrote:
>> +static bool x86_init_dev_msi_info(struct device *dev, struct irq_domain *domain,
>> +				  struct irq_domain *real_parent, struct msi_domain_info *info)
>> +{
>> +	const struct msi_parent_ops *pops = real_parent->msi_parent_ops;
>> +
>> +	/* MSI parent domain specific settings */
>> +	switch (real_parent->bus_token) {
>> +	case DOMAIN_BUS_ANY:
>> +		/* Only the vector domain can have the ANY token */
>> +		if (WARN_ON_ONCE(domain != real_parent))
>> +			return false;
>> +		info->chip->irq_set_affinity = msi_set_affinity;
>> +		/* See msi_set_affinity() for the gory details */
>> +		info->flags |= MSI_FLAG_NOMASK_QUIRK;
>> +		break;
>> +	default:
>> +		WARN_ON_ONCE(1);
>> +		return false;
>> +	}
>> +
>> +	/* Is the target supported? */
>> +	switch(info->bus_token) {
>> +	case DOMAIN_BUS_PCI_DEVICE_MSI:
>> +	case DOMAIN_BUS_PCI_DEVICE_MSIX:
>> +		break;
>> +	default:
>> +		WARN_ON_ONCE(1);
>> +		return false;
>
> Why does x86 care how the vector is ultimately programmed into the
> device?

That's not the point.

> The leaking of the MSI programming model into the irq implementations
> seems like there is still a troubled modularity.
>
> I understand that some implementations rely on a hypercall/trap or
> whatever and must know MSI vs MSI-X, but I'm surprised to see this
> here.

Why? It's the 'init a new per device domain' code which can rightfully
have a say whether it is willing to support something or not or to put
constraints on it. Those constraints can very much depend on the device
type or the MSI type. Creating random MSI domains seems to be pretty
envogue today and I really have no interest to deal with the fallout
once the fancy muck is merged in some random subsystem and the developer
moved on. I have no idea why everyone thinks that driver writers should
be granted the ultimate freedom to do what they want and anything which
puts an constraint on something is bad and troubled to begin with.

Since I started to strictly encapsulate and fence of things, the amount
of horrors I had to debug and then mop up has significantly decreased.
It also forces people who want to add some new fancy stuff to talk to
the infrastructure people so that the new functionality can be looked at
in the broader picture and solutions can be found upfront and not after
the fact when the resulting damage is discovered.

Quite some of the issues I discovered during last years discussions,
like the VFIO disable/enable trainwreck, the IRQ_VIRTUAL nonsense and
other random hacks could have neen avoided if people would actually talk
to each other and not just run off and hack something into place which
then gets somehow merged.

On the ARM side there is even a fundamental requirement for this today
due to the way how the existing infrastructure handles PCI/MSI[X] and
platform MSI, unless we go and rewrite half of the underlying code first
or in parallel.

It was also a migration aid to catch issues in the gradual conversion.

Again, we are not starting from a clean slate. I might be overly
cautious, but for very good reasons.

Thanks,

        tglx