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 supporton %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);-- 2.34.1