Date:   Sat, 27 Nov 2021 18:09:56 -0500
From:   "Michael S. Tsirkin" <mst@redhat.com>
To:     Eric Auger <eric.auger@redhat.com>
Cc:     Jean-Philippe Brucker <jean-philippe@linaro.org>, joro@8bytes.org,
        will@kernel.org, jasowang@redhat.com,
        virtualization@lists.linux-foundation.org,
        iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
        sebastien.boeuf@intel.com, kevin.tian@intel.com,
        pasic@linux.ibm.com
Subject: Re: [PATCH v2 4/5] iommu/virtio: Pass end address to
 viommu_add_mapping()
Message-ID: <20211127180742-mutt-send-email-mst@kernel.org>
References: <20211123155301.1047943-1-jean-philippe@linaro.org>
 <20211123155301.1047943-5-jean-philippe@linaro.org>
 <7b79fe9b-9d51-8bda-2868-b48781f07fc9@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7b79fe9b-9d51-8bda-2868-b48781f07fc9@redhat.com>
Precedence: bulk

On Sat, Nov 27, 2021 at 06:09:40PM +0100, Eric Auger wrote:
> 
> 
> On 11/23/21 4:53 PM, Jean-Philippe Brucker wrote:
> > To support identity mappings, the virtio-iommu driver must be able to
> > represent full 64-bit ranges internally. Pass (start, end) instead of
> > (start, size) to viommu_add/del_mapping().
> >
> > Clean comments. The one about the returned size was never true: when
> > sweeping the whole address space the returned size will most certainly
> > be smaller than 2^64.
> >
> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 
> Eric
> 
> > ---
> >  drivers/iommu/virtio-iommu.c | 31 +++++++++++++++----------------
> >  1 file changed, 15 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> > index d63ec4d11b00..eceb9281c8c1 100644
> > --- a/drivers/iommu/virtio-iommu.c
> > +++ b/drivers/iommu/virtio-iommu.c
> > @@ -311,8 +311,8 @@ static int viommu_send_req_sync(struct viommu_dev *viommu, void *buf,
> >   *
> >   * On success, return the new mapping. Otherwise return NULL.
> >   */
> > -static int viommu_add_mapping(struct viommu_domain *vdomain, unsigned long iova,
> > -			      phys_addr_t paddr, size_t size, u32 flags)
> > +static int viommu_add_mapping(struct viommu_domain *vdomain, u64 iova, u64 end,
> > +			      phys_addr_t paddr, u32 flags)
> >  {
> >  	unsigned long irqflags;
> >  	struct viommu_mapping *mapping;

I am worried that API changes like that will cause subtle
bugs since types of arguments change but not their
number. If we forgot to update some callers it will all be messed up.

How about passing struct Range instead?

> > @@ -323,7 +323,7 @@ static int viommu_add_mapping(struct viommu_domain *vdomain, unsigned long iova,
> >  
> >  	mapping->paddr		= paddr;
> >  	mapping->iova.start	= iova;
> > -	mapping->iova.last	= iova + size - 1;
> > +	mapping->iova.last	= end;
> >  	mapping->flags		= flags;
> >  
> >  	spin_lock_irqsave(&vdomain->mappings_lock, irqflags);
> > @@ -338,26 +338,24 @@ static int viommu_add_mapping(struct viommu_domain *vdomain, unsigned long iova,
> >   *
> >   * @vdomain: the domain
> >   * @iova: start of the range
> > - * @size: size of the range. A size of 0 corresponds to the entire address
> > - *	space.
> > + * @end: end of the range
> >   *
> > - * On success, returns the number of unmapped bytes (>= size)
> > + * On success, returns the number of unmapped bytes
> >   */
> >  static size_t viommu_del_mappings(struct viommu_domain *vdomain,
> > -				  unsigned long iova, size_t size)
> > +				  u64 iova, u64 end)
> >  {
> >  	size_t unmapped = 0;
> >  	unsigned long flags;
> > -	unsigned long last = iova + size - 1;
> >  	struct viommu_mapping *mapping = NULL;
> >  	struct interval_tree_node *node, *next;
> >  
> >  	spin_lock_irqsave(&vdomain->mappings_lock, flags);
> > -	next = interval_tree_iter_first(&vdomain->mappings, iova, last);
> > +	next = interval_tree_iter_first(&vdomain->mappings, iova, end);
> >  	while (next) {
> >  		node = next;
> >  		mapping = container_of(node, struct viommu_mapping, iova);
> > -		next = interval_tree_iter_next(node, iova, last);
> > +		next = interval_tree_iter_next(node, iova, end);
> >  
> >  		/* Trying to split a mapping? */
> >  		if (mapping->iova.start < iova)
> > @@ -656,8 +654,8 @@ static void viommu_domain_free(struct iommu_domain *domain)
> >  {
> >  	struct viommu_domain *vdomain = to_viommu_domain(domain);
> >  
> > -	/* Free all remaining mappings (size 2^64) */
> > -	viommu_del_mappings(vdomain, 0, 0);
> > +	/* Free all remaining mappings */
> > +	viommu_del_mappings(vdomain, 0, ULLONG_MAX);
> >  
> >  	if (vdomain->viommu)
> >  		ida_free(&vdomain->viommu->domain_ids, vdomain->id);
> > @@ -742,6 +740,7 @@ static int viommu_map(struct iommu_domain *domain, unsigned long iova,
> >  {
> >  	int ret;
> >  	u32 flags;
> > +	u64 end = iova + size - 1;
> >  	struct virtio_iommu_req_map map;
> >  	struct viommu_domain *vdomain = to_viommu_domain(domain);
> >  
> > @@ -752,7 +751,7 @@ static int viommu_map(struct iommu_domain *domain, unsigned long iova,
> >  	if (flags & ~vdomain->map_flags)
> >  		return -EINVAL;
> >  
> > -	ret = viommu_add_mapping(vdomain, iova, paddr, size, flags);
> > +	ret = viommu_add_mapping(vdomain, iova, end, paddr, flags);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -761,7 +760,7 @@ static int viommu_map(struct iommu_domain *domain, unsigned long iova,
> >  		.domain		= cpu_to_le32(vdomain->id),
> >  		.virt_start	= cpu_to_le64(iova),
> >  		.phys_start	= cpu_to_le64(paddr),
> > -		.virt_end	= cpu_to_le64(iova + size - 1),
> > +		.virt_end	= cpu_to_le64(end),
> >  		.flags		= cpu_to_le32(flags),
> >  	};
> >  
> > @@ -770,7 +769,7 @@ static int viommu_map(struct iommu_domain *domain, unsigned long iova,
> >  
> >  	ret = viommu_send_req_sync(vdomain->viommu, &map, sizeof(map));
> >  	if (ret)
> > -		viommu_del_mappings(vdomain, iova, size);
> > +		viommu_del_mappings(vdomain, iova, end);
> >  
> >  	return ret;
> >  }
> > @@ -783,7 +782,7 @@ static size_t viommu_unmap(struct iommu_domain *domain, unsigned long iova,
> >  	struct virtio_iommu_req_unmap unmap;
> >  	struct viommu_domain *vdomain = to_viommu_domain(domain);
> >  
> > -	unmapped = viommu_del_mappings(vdomain, iova, size);
> > +	unmapped = viommu_del_mappings(vdomain, iova, iova + size - 1);
> >  	if (unmapped < size)
> >  		return 0;
> >