Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <95df349e-3b8d-57b4-6e07-ba80f685d4f1@linux.ibm.com>
Date:   Tue, 4 Oct 2022 12:20:47 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.0
Subject: Re: [PATCH v4 2/5] iommu/s390: Get rid of s390_domain_device
Content-Language: en-US
To:     Niklas Schnelle <schnelle@linux.ibm.com>,
        Pierre Morel <pmorel@linux.ibm.com>, iommu@lists.linux.dev
Cc:     linux-s390@vger.kernel.org, borntraeger@linux.ibm.com,
        hca@linux.ibm.com, gor@linux.ibm.com,
        gerald.schaefer@linux.ibm.com, agordeev@linux.ibm.com,
        svens@linux.ibm.com, joro@8bytes.org, will@kernel.org,
        robin.murphy@arm.com, jgg@nvidia.com, linux-kernel@vger.kernel.org
References: <20221004120706.2957492-1-schnelle@linux.ibm.com>
 <20221004120706.2957492-3-schnelle@linux.ibm.com>
From:   Matthew Rosato <mjrosato@linux.ibm.com>
In-Reply-To: <20221004120706.2957492-3-schnelle@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/4/22 8:07 AM, Niklas Schnelle wrote:
> The struct s390_domain_device serves the sole purpose as list entry for
> the devices list of a struct s390_domain. As it contains no additional
> information besides a list_head and a pointer to the struct zpci_dev we
> can simplify things and just thread the device list through struct
> zpci_dev directly. This removes the need to allocate during domain
> attach and gets rid of one level of indirection during mapping
> operations.
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

> ---
>  arch/s390/include/asm/pci.h |  1 +
>  drivers/iommu/s390-iommu.c  | 45 ++++++++-----------------------------
>  2 files changed, 10 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> index 108e732d7b14..15f8714ca9b7 100644
> --- a/arch/s390/include/asm/pci.h
> +++ b/arch/s390/include/asm/pci.h
> @@ -117,6 +117,7 @@ struct zpci_bus {
>  struct zpci_dev {
>  	struct zpci_bus *zbus;
>  	struct list_head entry;		/* list of all zpci_devices, needed for hotplug, etc. */
> +	struct list_head iommu_list;
>  	struct kref kref;
>  	struct hotplug_slot hotplug_slot;
>  
> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
> index 0f58e897bc95..6f87dd4b85af 100644
> --- a/drivers/iommu/s390-iommu.c
> +++ b/drivers/iommu/s390-iommu.c
> @@ -29,11 +29,6 @@ struct s390_domain {
>  	spinlock_t		list_lock;
>  };
>  
> -struct s390_domain_device {
> -	struct list_head	list;
> -	struct zpci_dev		*zdev;
> -};
> -
>  static struct s390_domain *to_s390_domain(struct iommu_domain *dom)
>  {
>  	return container_of(dom, struct s390_domain, domain);
> @@ -87,21 +82,13 @@ static void s390_domain_free(struct iommu_domain *domain)
>  static void __s390_iommu_detach_device(struct zpci_dev *zdev)
>  {
>  	struct s390_domain *s390_domain = zdev->s390_domain;
> -	struct s390_domain_device *domain_device, *tmp;
>  	unsigned long flags;
>  
>  	if (!s390_domain)
>  		return;
>  
>  	spin_lock_irqsave(&s390_domain->list_lock, flags);
> -	list_for_each_entry_safe(domain_device, tmp, &s390_domain->devices,
> -				 list) {
> -		if (domain_device->zdev == zdev) {
> -			list_del(&domain_device->list);
> -			kfree(domain_device);
> -			break;
> -		}
> -	}
> +	list_del_init(&zdev->iommu_list);
>  	spin_unlock_irqrestore(&s390_domain->list_lock, flags);
>  
>  	zpci_unregister_ioat(zdev, 0);
> @@ -114,17 +101,12 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
>  {
>  	struct s390_domain *s390_domain = to_s390_domain(domain);
>  	struct zpci_dev *zdev = to_zpci_dev(dev);
> -	struct s390_domain_device *domain_device;
>  	unsigned long flags;
> -	int cc, rc = 0;
> +	int cc;
>  
>  	if (!zdev)
>  		return -ENODEV;
>  
> -	domain_device = kzalloc(sizeof(*domain_device), GFP_KERNEL);
> -	if (!domain_device)
> -		return -ENOMEM;
> -
>  	if (zdev->s390_domain)
>  		__s390_iommu_detach_device(zdev);
>  	else if (zdev->dma_table)
> @@ -133,10 +115,8 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
>  	zdev->dma_table = s390_domain->dma_table;
>  	cc = zpci_register_ioat(zdev, 0, zdev->start_dma, zdev->end_dma,
>  				virt_to_phys(zdev->dma_table));
> -	if (cc) {
> -		rc = -EIO;
> -		goto out_free;
> -	}
> +	if (cc)
> +		return -EIO;
>  
>  	spin_lock_irqsave(&s390_domain->list_lock, flags);
>  	/* First device defines the DMA range limits */
> @@ -147,21 +127,14 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
>  	/* Allow only devices with identical DMA range limits */
>  	} else if (domain->geometry.aperture_start != zdev->start_dma ||
>  		   domain->geometry.aperture_end != zdev->end_dma) {
> -		rc = -EINVAL;
>  		spin_unlock_irqrestore(&s390_domain->list_lock, flags);
> -		goto out_free;
> +		return -EINVAL;
>  	}
> -	domain_device->zdev = zdev;
>  	zdev->s390_domain = s390_domain;
> -	list_add(&domain_device->list, &s390_domain->devices);
> +	list_add(&zdev->iommu_list, &s390_domain->devices);
>  	spin_unlock_irqrestore(&s390_domain->list_lock, flags);
>  
>  	return 0;
> -
> -out_free:
> -	kfree(domain_device);
> -
> -	return rc;
>  }
>  
>  static void s390_iommu_detach_device(struct iommu_domain *domain,
> @@ -198,10 +171,10 @@ static int s390_iommu_update_trans(struct s390_domain *s390_domain,
>  				   phys_addr_t pa, dma_addr_t dma_addr,
>  				   size_t size, int flags)
>  {
> -	struct s390_domain_device *domain_device;
>  	phys_addr_t page_addr = pa & PAGE_MASK;
>  	dma_addr_t start_dma_addr = dma_addr;
>  	unsigned long irq_flags, nr_pages, i;
> +	struct zpci_dev *zdev;
>  	unsigned long *entry;
>  	int rc = 0;
>  
> @@ -226,8 +199,8 @@ static int s390_iommu_update_trans(struct s390_domain *s390_domain,
>  	}
>  
>  	spin_lock(&s390_domain->list_lock);
> -	list_for_each_entry(domain_device, &s390_domain->devices, list) {
> -		rc = zpci_refresh_trans((u64) domain_device->zdev->fh << 32,
> +	list_for_each_entry(zdev, &s390_domain->devices, iommu_list) {
> +		rc = zpci_refresh_trans((u64)zdev->fh << 32,
>  					start_dma_addr, nr_pages * PAGE_SIZE);
>  		if (rc)
>  			break;