RE: [patch V2 12/33] PCI/MSI: Add support for per device MSI[X] domains

From: Thomas Gleixner <tglx@linutronix.de>
To: "Tian, Kevin" <kevin.tian@intel.com>,
	LKML <linux-kernel@vger.kernel.org>
Cc: "x86@kernel.org" <x86@kernel.org>, Joerg Roedel <joro@8bytes.org>,
	Will Deacon <will@kernel.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Marc Zyngier <maz@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jason Gunthorpe <jgg@mellanox.com>,
	"Jiang, Dave" <dave.jiang@intel.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	"Williams, Dan J" <dan.j.williams@intel.com>,
	Logan Gunthorpe <logang@deltatee.com>,
	"Raj, Ashok" <ashok.raj@intel.com>, Jon Mason <jdmason@kudzu.us>,
	Allen Hubbe <allenbh@gmail.com>,
	"Ahmed S. Darwish" <darwi@linutronix.de>
Subject: RE: [patch V2 12/33] PCI/MSI: Add support for per device MSI[X] domains
Date: Wed, 23 Nov 2022 12:41:05 +0100	[thread overview]
Message-ID: <87zgchew4u.ffs@tglx> (raw)
In-Reply-To: <BN9PR11MB5276E9AD0F86A27A23ED8CFB8C0C9@BN9PR11MB5276.namprd11.prod.outlook.com>

On Wed, Nov 23 2022 at 08:08, Kevin Tian wrote:
>> From: Thomas Gleixner <tglx@linutronix.de>
>
> pci_mask_msi() and pci_msi_mask() are confusing.
>
> Probably pci_irq_mask_msi() given the parameter is irq_data.

maybe.

>> +bool pci_setup_msi_device_domain(struct pci_dev *pdev)
>> +{
>> +	if (WARN_ON_ONCE(pdev->msix_enabled))
>> +		return false;
>
> the check already exists in __pci_enable_msi_range()
>
>> +bool pci_setup_msix_device_domain(struct pci_dev *pdev, unsigned int
>> hwsize)
>> +{
>> +	if (WARN_ON_ONCE(pdev->msix_enabled))
>> +		return false;
>
> ditto.
>
> btw according to the comment this should check pdev->msi_enabled.

Yeah, those are probably redundant.

>
>> @@ -152,13 +316,33 @@ bool pci_msi_domain_supports(struct pci_
>>  {
>>  	struct msi_domain_info *info;
>>  	struct irq_domain *domain;
>> +	unsigned int supported;
>> 
>>  	domain = dev_get_msi_domain(&pdev->dev);
>> 
>>  	if (!domain || !irq_domain_is_hierarchy(domain))
>>  		return mode == ALLOW_LEGACY;
>> -	info = domain->host_data;
>> -	return (info->flags & feature_mask) == feature_mask;
>> +
>> +	if (!irq_domain_is_msi_parent(domain)) {
>> +		/*
>> +		 * For "global" PCI/MSI interrupt domains the associated
>> +		 * msi_domain_info::flags is the authoritive source of
>> +		 * information.
>> +		 */
>> +		info = domain->host_data;
>> +		supported = info->flags;
>> +	} else {
>> +		/*
>> +		 * For MSI parent domains the supported feature set
>> +		 * is avaliable in the parent ops. This makes checks
>> +		 * possible before actually instantiating the
>> +		 * per device domain because the parent is never
>> +		 * expanding the PCI/MSI functionality.
>> +		 */
>> +		supported = domain->msi_parent_ops->supported_flags;
>
> This is general PCI MSI logic. So an open related to my rely to patch02,
> is it correct for PCI core to assume that the real parent imposes all the
> restrictions and there is no need to further go down the hierarchy?

That was my working assumption and it turned out to be correct with both
x86 and ARM.

Thanks,

        tglx