Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] vfio/iommu_type1: report the IOMMU aperture info
To:     Alex Williamson <alex.williamson@redhat.com>
Cc:     cohuck@redhat.com, borntraeger@de.ibm.com,
        zyimin@linux.vnet.ibm.com, pasic@linux.vnet.ibm.com,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org
References: <1512041678-4563-1-git-send-email-pmorel@linux.vnet.ibm.com>
 <20171130070844.7d6c5053@t450s.home>
From:   Pierre Morel <pmorel@linux.vnet.ibm.com>
Date:   Thu, 30 Nov 2017 16:11:35 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <20171130070844.7d6c5053@t450s.home>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Message-Id: <c5341d60-3b65-f64e-21ea-85608d1b4ec4@linux.vnet.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 30/11/2017 15:08, Alex Williamson wrote:
> On Thu, 30 Nov 2017 12:34:38 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> When userland VFIO defines a new IOMMU for a guest it may
>> want to specify to the guest the physical limits of
>> the underlying host IOMMU to avoid access to forbidden
>> memory ranges.
>>
>> Currently, the vfio_iommu_type1 driver does not report this
>> information to userland.
>>
>> Let's extend the vfio_iommu_type1_info structure reported
>> by the ioctl VFIO_IOMMU_GET_INFO command to report the
>> IOMMU limits as new uint64_t entries aperture_start and
>> aperture_end.
>>
>> Let's also extend the flags bit map to add a flag specifying
>> if this extension of the info structure is reported or not.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> ---
>>   drivers/vfio/vfio_iommu_type1.c | 42 +++++++++++++++++++++++++++++++++++++++++
>>   include/uapi/linux/vfio.h       |  3 +++
>>   2 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 8549cb1..7da5fe0 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -1526,6 +1526,40 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
>>   	return ret;
>>   }
>>   
>> +/**
>> + * vfio_get_aperture - report minimal aperture of a vfio_iommu
>> + * @iommu: the current vfio_iommu
>> + * @start: a pointer to the aperture start
>> + * @end  : a pointer to the aperture end
>> + *
>> + * This function iterate on the domains using the given vfio_iommu
>> + * and restrict the aperture to the minimal aperture common
>> + * to all domains sharing this vfio_iommu.
>> + */
>> +static void vfio_get_aperture(struct vfio_iommu *iommu, uint64_t *start,
>> +				uint64_t *end)
>> +{
>> +	struct iommu_domain_geometry geometry;
>> +	struct vfio_domain *domain;
>> +
>> +	*start = 0;
>> +	*end = U64_MAX;
>> +
>> +	mutex_lock(&iommu->lock);
>> +	/* loop on all domains using this vfio_iommu */
>> +	list_for_each_entry(domain, &iommu->domain_list, next) {
>> +		iommu_domain_get_attr(domain->domain, DOMAIN_ATTR_GEOMETRY,
>> +					&geometry);
>> +		if (geometry.force_aperture) {
>> +			if (geometry.aperture_start > *start)
>> +				*start = geometry.aperture_start;
>> +			if (geometry.aperture_end < *end)
>> +				*end = geometry.aperture_end;
>> +		}
>> +	}
>> +	mutex_unlock(&iommu->lock);
>> +}
>> +
>>   static long vfio_iommu_type1_ioctl(void *iommu_data,
>>   				   unsigned int cmd, unsigned long arg)
>>   {
>> @@ -1560,6 +1594,14 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>>   
>>   		info.iova_pgsizes = vfio_pgsize_bitmap(iommu);
>>   
>> +		minsz = min_t(size_t, info.argsz, sizeof(info));
>> +		if (minsz >= offsetofend(struct vfio_iommu_type1_info,
>> +					 aperture_end)) {
>> +			info.flags |= VFIO_IOMMU_INFO_APERTURE;
>> +			vfio_get_aperture(iommu, &info.aperture_start,
>> +					  &info.aperture_end);
>> +		}
>> +
>>   		return copy_to_user((void __user *)arg, &info, minsz) ?
>>   			-EFAULT : 0;
>>   
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index 0fb25fb..780d909 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -519,6 +519,9 @@ struct vfio_iommu_type1_info {
>>   	__u32	flags;
>>   #define VFIO_IOMMU_INFO_PGSIZES (1 << 0)	/* supported page sizes info */
>>   	__u64	iova_pgsizes;		/* Bitmap of supported page sizes */
>> +#define VFIO_IOMMU_INFO_APERTURE (1 << 1)	/* supported aperture info */
>> +	__u64   aperture_start;		/* start of DMA aperture */
>> +	__u64   aperture_end;		/* end of DMA aperture */
>>   };
>>   
>>   #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
> 
> This only supports the most simple topology, even x86 cannot claim to
> have a single contiguous aperture, it's typically bisected by an MSI
> window.  I think we need an API that supports one or more apertures
> out of the box.  Also as Eric indicates, a capability is probably the
> better option for creating a flexible structure.  Thanks,
> 
> Alex
> 


Yes, I understand that a capability here is a must, I will follow this way.

For having multiple aperture and MSI protection, I understood it was 
done using windows and reserved regions.
Can you point me to my error?

Thanks,

Pierre

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany


From 1585503044672071238@xxx Thu Nov 30 14:52:12 +0000 2017
X-GM-THRID: 1585490648319889089
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread