lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 16 May 2019 13:45:35 +0200
From:   Auger Eric <eric.auger@...hat.com>
To:     Jean-Philippe Brucker <jean-philippe.brucker@....com>,
        eric.auger.pro@...il.com, joro@...tes.org,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
        dwmw2@...radead.org, lorenzo.pieralisi@....com,
        robin.murphy@....com, will.deacon@....com, hanjun.guo@...aro.org,
        sudeep.holla@....com
Cc:     alex.williamson@...hat.com
Subject: Re: [PATCH v3 6/7] iommu: Introduce IOMMU_RESV_DIRECT_RELAXABLE
 reserved memory regions

Hi Jean-Philippe,

On 5/16/19 1:16 PM, Jean-Philippe Brucker wrote:
> On 16/05/2019 11:08, Eric Auger wrote:
>> Note: At the moment the sysfs ABI is not changed. However I wonder
>> whether it wouldn't be preferable to report the direct region as
>> "direct_relaxed" there. At the moment, in case the same direct
>> region is used by 2 devices, one USB/GFX and another not belonging
>> to the previous categories, the direct region will be output twice
>> with "direct" type.
>>
>> This would unblock Shameer's series:
>> [PATCH v6 0/7] vfio/type1: Add support for valid iova list management
>> https://patchwork.kernel.org/patch/10425309/
> 
> Thanks for doing this!
> 
>> which failed to get pulled for 4.18 merge window due to IGD
>> device assignment regression.
>>
>> v2 -> v3:
>> - fix direct type check
>> ---
>>  drivers/iommu/iommu.c | 12 +++++++-----
>>  include/linux/iommu.h |  6 ++++++
>>  2 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index ae4ea5c0e6f9..28c3d6351832 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -73,10 +73,11 @@ struct iommu_group_attribute {
>>  };
>>  
>>  static const char * const iommu_group_resv_type_string[] = {
>> -	[IOMMU_RESV_DIRECT]	= "direct",
>> -	[IOMMU_RESV_RESERVED]	= "reserved",
>> -	[IOMMU_RESV_MSI]	= "msi",
>> -	[IOMMU_RESV_SW_MSI]	= "msi",
>> +	[IOMMU_RESV_DIRECT]			= "direct",
>> +	[IOMMU_RESV_DIRECT_RELAXABLE]		= "direct",
>> +	[IOMMU_RESV_RESERVED]			= "reserved",
>> +	[IOMMU_RESV_MSI]			= "msi",
>> +	[IOMMU_RESV_SW_MSI]			= "msi",
>>  };
>>  
>>  #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)		\
>> @@ -573,7 +574,8 @@ static int iommu_group_create_direct_mappings(struct iommu_group *group,
>>  		start = ALIGN(entry->start, pg_size);
>>  		end   = ALIGN(entry->start + entry->length, pg_size);
>>  
>> -		if (entry->type != IOMMU_RESV_DIRECT)
>> +		if (entry->type != IOMMU_RESV_DIRECT &&
>> +		    entry->type != IOMMU_RESV_DIRECT_RELAXABLE)
> 
> I'm trying to understand why you need to create direct mappings at all
> for these relaxable regions. In the host the region is needed for legacy
> device features, which are disabled (and cannot be re-enabled) when
> assigning the device to a guest?
This follows Kevin's comment in the thread below:
https://patchwork.kernel.org/patch/10449103/#21957279

In normal DMA API host path, those regions need to be 1-1 mapped. They
are likely to be accessed by the driver or FW at early boot phase or
even during execution, depending on features being used.

That's the reason, according to Kevin we couldn't hide them.

We just know that, in general, they are not used anymore when assigning
the device or if accesses are attempted this generally does not block
the assignment use case. For example, it is said in
https://github.com/qemu/qemu/blob/master/docs/igd-assign.txt that in
legacy IGD assignment use case, there may be "a small numbers of DMAR
faults when initially assigned".

Thanks

Eric


> 
> Thanks,
> Jean
> 

Powered by blists - more mailing lists