lkml.org 
[lkml]   [2021]   [Sep]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/1] iommu/vt-d: Use second level for GPA->HPA translation
From
Date
Hi Kevin,

Thanks for reviewing my patch.

On 9/24/21 11:16 AM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Friday, September 24, 2021 10:30 AM
>>
>> The IOMMU VT-d implementation uses the first level for GPA->HPA
>> translation
>> by default. Although both the first level and the second level could handle
>> the DMA translation, they are different in some way. For example, the
>> second
>> level translation has separate controls for the Access/Dirty page tracking
>> and the page-level forcing snoop. With first level translation, there're
>> no such controls. This uses the second level for GPA->HPA translation so
>
> first-level has no page-granular snoop control, but has a global control
> in pasid entry.

Agreed. Will add this.

>
>> that it could provide a consistent hardware interface for use cases like
>> dirty page tracking during the VM live migration.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>> include/linux/intel-iommu.h | 7 ++-----
>> drivers/iommu/intel/iommu.c | 21 +++++++++++++--------
>> 2 files changed, 15 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
>> index 05a65eb155f7..a5fb20702201 100644
>> --- a/include/linux/intel-iommu.h
>> +++ b/include/linux/intel-iommu.h
>> @@ -517,22 +517,19 @@ struct context_entry {
>> u64 hi;
>> };
>>
>> -/* si_domain contains mulitple devices */
>> -#define DOMAIN_FLAG_STATIC_IDENTITY BIT(0)
>
> this is a separate cleanup. better mention it in the commit msg or
> put in another patch.

Sure.

>
>> -
>> /*
>> * When VT-d works in the scalable mode, it allows DMA translation to
>> * happen through either first level or second level page table. This
>> * bit marks that the DMA translation for the domain goes through the
>> * first level page table, otherwise, it goes through the second level.
>> */
>> -#define DOMAIN_FLAG_USE_FIRST_LEVEL BIT(1)
>> +#define DOMAIN_FLAG_USE_FIRST_LEVEL BIT(0)
>>
>> /*
>> * Domain represents a virtual machine which demands iommu nested
>> * translation mode support.
>> */
>> -#define DOMAIN_FLAG_NESTING_MODE BIT(2)
>> +#define DOMAIN_FLAG_NESTING_MODE BIT(1)
>>
>> struct dmar_domain {
>> int nid; /* node id */
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index d75f59ae28e6..c814fea0522e 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -522,7 +522,7 @@ static inline void free_devinfo_mem(void *vaddr)
>>
>> static inline int domain_type_is_si(struct dmar_domain *domain)
>> {
>> - return domain->flags & DOMAIN_FLAG_STATIC_IDENTITY;
>> + return domain->domain.type == IOMMU_DOMAIN_IDENTITY;
>> }
>>
>> static inline bool domain_use_first_level(struct dmar_domain *domain)
>> @@ -1874,12 +1874,18 @@ static void free_dmar_iommu(struct
>> intel_iommu *iommu)
>> * Check and return whether first level is used by default for
>> * DMA translation.
>> */
>> -static bool first_level_by_default(void)
>> +static bool first_level_by_default(unsigned int type)
>> {
>> - return scalable_mode_support() && intel_cap_flts_sanity();
>> + if (type == IOMMU_DOMAIN_UNMANAGED)
>> + return false;
>> +
>
> I think the order is not correct. what about 2nd level is even not
> present?

Fair enough. How about

#1) hardware only capable of first level, return true
#2) hardware only capable of second level, return false

(we fail iommu initialization if neither FL nor SL)
Then, both FL and SL are supported.

#3) domain is type of UNMANAGED, return false
#4) otherwise, return true.

Does this make sense?

>
>
>> + if (!scalable_mode_support() || !intel_cap_flts_sanity())
>> + return false;
>> +
>> + return true;
>> }
>>
>> -static struct dmar_domain *alloc_domain(int flags)
>> +static struct dmar_domain *alloc_domain(unsigned int type)
>> {
>> struct dmar_domain *domain;
>>
>> @@ -1889,8 +1895,7 @@ static struct dmar_domain *alloc_domain(int flags)
>>
>> memset(domain, 0, sizeof(*domain));
>> domain->nid = NUMA_NO_NODE;
>> - domain->flags = flags;
>> - if (first_level_by_default())
>> + if (first_level_by_default(type))
>> domain->flags |= DOMAIN_FLAG_USE_FIRST_LEVEL;
>> domain->has_iotlb_device = false;
>> INIT_LIST_HEAD(&domain->devices);
>> @@ -2708,7 +2713,7 @@ static int __init si_domain_init(int hw)
>> struct device *dev;
>> int i, nid, ret;
>>
>> - si_domain = alloc_domain(DOMAIN_FLAG_STATIC_IDENTITY);
>> + si_domain = alloc_domain(IOMMU_DOMAIN_IDENTITY);
>> if (!si_domain)
>> return -EFAULT;
>>
>> @@ -4517,7 +4522,7 @@ static struct iommu_domain
>> *intel_iommu_domain_alloc(unsigned type)
>> case IOMMU_DOMAIN_DMA:
>> case IOMMU_DOMAIN_DMA_FQ:
>> case IOMMU_DOMAIN_UNMANAGED:
>> - dmar_domain = alloc_domain(0);
>> + dmar_domain = alloc_domain(type);
>> if (!dmar_domain) {
>> pr_err("Can't allocate dmar_domain\n");
>> return NULL;
>> --
>> 2.25.1
>

Best regards,
baolu

\
 
 \ /
  Last update: 2021-09-26 04:47    [W:0.053 / U:0.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site