Re: [PATCH v5 04/11] iommu/vt-d: Add helper to setup pasid nested translation

[Yi Liu <yi.l.liu@intel.com>]

On 2023/9/27 14:37, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Thursday, September 21, 2023 3:54 PM
>> +
>> +/**
>> + * intel_pasid_setup_nested() - Set up PASID entry for nested translation.
>> + * @iommu:      IOMMU which the device belong to
>> + * @dev:        Device to be set up for translation
>> + * @pasid:      PASID to be programmed in the device PASID table
>> + * @domain:     User first-level domain nested on a s2 domain
> 
> let's use stage-1/stage-2 consistently

ok

>> + *
>> + * This is used for nested translation based vIOMMU. e.g. guest IOVA and
>> + * guest shared virtual address. In this case, the first-level page tables
>> + * are used for GVA/GIOVA-GPA translation in the guest, the second-level
>> + * page tables are used for GPA-HPA translation.
> 
> No need to mention guest or vIOMMU. Just stick to the fact of
> nested configuration.

how about below?

* This is used for nested translation. The input domain should be
* nested type and nested on a parent with 'is_nested_parent' flag
* set.

>> + */
>> +int intel_pasid_setup_nested(struct intel_iommu *iommu, struct device
>> *dev,
>> +			     u32 pasid, struct dmar_domain *domain)
>> +{
>> +	struct iommu_hwpt_vtd_s1 *s1_cfg = &domain->s1_cfg;
>> +	pgd_t *s1_gpgd = (pgd_t *)(uintptr_t)domain->s1_pgtbl;
>> +	struct dmar_domain *s2_domain = domain->s2_domain;
>> +	u16 did = domain_id_iommu(domain, iommu);
>> +	struct dma_pte *pgd = s2_domain->pgd;
>> +	struct pasid_entry *pte;
>> +
>> +	if (!ecap_nest(iommu->ecap)) {
>> +		pr_err_ratelimited("%s: No nested translation support\n",
>> +				   iommu->name);
>> +		return -ENODEV;
>> +	}
> 
> this check has been done when creating the nest_parent s2 and iommufd
> shouldn't request a nested setup when the specified s2 is not nest_parent.

yes, it has been checked when allocating nest_parent s2. Current iommufd
nesting series does not check nested setup specifically. All the attach
logic is common across the domain types. So it seems no need for iommufd
to check it.

> If you still want to keep this check then a WARN_ON.

may just remove it.

>> +
>> +	/*
>> +	 * Address width should match in two dimensions: CPU vs. IOMMU,
>> +	 * guest vs. host.
>> +	 */
> 
> No 'guest'. Just that the user requested addr width must be supported by
> the hardware.

sure.

> 
>> +	switch (s1_cfg->addr_width) {
>> +	case ADDR_WIDTH_4LEVEL:
>> +		break;
>> +#ifdef CONFIG_X86
>> +	case ADDR_WIDTH_5LEVEL:
>> +		if (!cpu_feature_enabled(X86_FEATURE_LA57) ||
>> +		    !cap_fl5lp_support(iommu->cap)) {
>> +			dev_err_ratelimited(dev,
>> +					    "5-level paging not supported\n");
>> +			return -EINVAL;
>> +		}
>> +		break;
> 
> I wonder whether this check is too strict. a nested configuration doesn't
> mandate vSVM. e.g. for guest IOVA the s1 page table is not walked by
> the CPU. Adding a CPU capability check here doesn't make much sense.
> 
> Of course doing so may not cause a problem reality as very likely all
> Intel platforms have same 5level support in both CPU/IOMMU. But
> code-wise it's still better to do it right.
> 
> Ideally the guest will be notified with 5level support only when the CPU
> supports it then it's the guest's business to choose the right format to
> match the CPU capability. Misconfiguration will be caught by the CPU
> virtualization path?

yes, it may make more sense that CPU and IOMMU side take its own check to
avoid misuage of 5lvl. For vSVM, it's guest's bussiness to ensure its vCPU
and vIOMMU both have 5lvl. will relax the check here.

>> +#endif
>> +	default:
>> +		dev_err_ratelimited(dev, "Invalid guest address width %d\n",
>> +				    s1_cfg->addr_width);
> 
> ditto. use 'stage-1 address width' instead of 'guest'.

sure.

> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	if ((s1_cfg->flags & IOMMU_VTD_S1_SRE) && !ecap_srs(iommu-
>>> ecap)) {
>> +		pr_err_ratelimited("No supervisor request support on %s\n",
>> +				   iommu->name);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if ((s1_cfg->flags & IOMMU_VTD_S1_EAFE) && !ecap_eafs(iommu-
>>> ecap)) {
>> +		pr_err_ratelimited("No extended access flag support
>> on %s\n",
>> +				   iommu->name);
>> +		return -EINVAL;
>> +	}
>> +
>> +	spin_lock(&iommu->lock);
>> +	pte = intel_pasid_get_entry(dev, pasid);
>> +	if (!pte) {
>> +		spin_unlock(&iommu->lock);
>> +		return -ENODEV;
>> +	}
>> +	if (pasid_pte_is_present(pte)) {
>> +		spin_unlock(&iommu->lock);
>> +		return -EBUSY;
>> +	}
>> +
>> +	pasid_clear_entry(pte);
>> +
>> +	if (s1_cfg->addr_width == ADDR_WIDTH_5LEVEL)
>> +		pasid_set_flpm(pte, 1);
>> +
>> +	pasid_set_flptr(pte, (uintptr_t)s1_gpgd);
>> +
>> +	if (s1_cfg->flags & IOMMU_VTD_S1_SRE) {
>> +		pasid_set_sre(pte);
>> +		if (s1_cfg->flags & IOMMU_VTD_S1_WPE)
>> +			pasid_set_wpe(pte);
>> +	}
>> +
>> +	if (s1_cfg->flags & IOMMU_VTD_S1_EAFE)
>> +		pasid_set_eafe(pte);
>> +
>> +	if (s2_domain->force_snooping)
>> +		pasid_set_pgsnp(pte);
>> +
>> +	pasid_set_slptr(pte, virt_to_phys(pgd));
>> +	pasid_set_fault_enable(pte);
>> +	pasid_set_domain_id(pte, did);
>> +	pasid_set_address_width(pte, s2_domain->agaw);
>> +	pasid_set_page_snoop(pte, !!ecap_smpwc(iommu->ecap));
>> +	pasid_set_translation_type(pte, PASID_ENTRY_PGTT_NESTED);
>> +	pasid_set_present(pte);
>> +	spin_unlock(&iommu->lock);
> 
> All changes within iommu->lock are specific to the device specific
> PASID entry. Probably this is one potential cleanup TODO to
> use a per-device lock instead.

yeah, a separate cleanup. is it, @Baolu?

>> +
>> +	pasid_flush_caches(iommu, pte, pasid, did);
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
>> index 4e9e68c3c388..7906d73f4ded 100644
>> --- a/drivers/iommu/intel/pasid.h
>> +++ b/drivers/iommu/intel/pasid.h
>> @@ -109,6 +109,8 @@ int intel_pasid_setup_second_level(struct
>> intel_iommu *iommu,
>>   int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
>>   				   struct dmar_domain *domain,
>>   				   struct device *dev, u32 pasid);
>> +int intel_pasid_setup_nested(struct intel_iommu *iommu, struct device
>> *dev,
>> +			     u32 pasid, struct dmar_domain *domain);
>>   void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
>>   				 struct device *dev, u32 pasid,
>>   				 bool fault_ignore);
>> --
>> 2.34.1
> 

-- 
Regards,
Yi Liu