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

[Tian, Kevin]

> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Sent: Monday, November 21, 2022 10:38 PM
> 
> +
> +static void pci_mask_msi(struct irq_data *data)
> +{
> +	struct msi_desc *desc = irq_data_get_msi_desc(data);
> +
> +	pci_msi_mask(desc, BIT(data->irq - desc->irq));

pci_mask_msi() and pci_msi_mask() are confusing.

Probably pci_irq_mask_msi() given the parameter is irq_data.

> +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.

> @@ -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?