| 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 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <BN9PR11MB5276DD2D6E5690F8FC7B18378CC2A@BN9PR11MB5276.namprd11.prod.outlook.com>
Date: Wed, 27 Sep 2023 06:37:20 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: "Liu, Yi L" <yi.l.liu@...el.com>,
"joro@...tes.org" <joro@...tes.org>,
"alex.williamson@...hat.com" <alex.williamson@...hat.com>,
"jgg@...dia.com" <jgg@...dia.com>,
"robin.murphy@....com" <robin.murphy@....com>,
"baolu.lu@...ux.intel.com" <baolu.lu@...ux.intel.com>
CC: "cohuck@...hat.com" <cohuck@...hat.com>,
"eric.auger@...hat.com" <eric.auger@...hat.com>,
"nicolinc@...dia.com" <nicolinc@...dia.com>,
"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"mjrosato@...ux.ibm.com" <mjrosato@...ux.ibm.com>,
"chao.p.peng@...ux.intel.com" <chao.p.peng@...ux.intel.com>,
"yi.y.sun@...ux.intel.com" <yi.y.sun@...ux.intel.com>,
"peterx@...hat.com" <peterx@...hat.com>,
"jasowang@...hat.com" <jasowang@...hat.com>,
"shameerali.kolothum.thodi@...wei.com"
<shameerali.kolothum.thodi@...wei.com>,
"lulu@...hat.com" <lulu@...hat.com>,
"suravee.suthikulpanit@....com" <suravee.suthikulpanit@....com>,
"iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
"Duan, Zhenzhong" <zhenzhong.duan@...el.com>,
"Martins, Joao" <joao.m.martins@...cle.com>,
Jacob Pan <jacob.jun.pan@...ux.intel.com>
Subject: RE: [PATCH v5 04/11] iommu/vt-d: Add helper to setup pasid nested
translation
> From: Liu, Yi L <yi.l.liu@...el.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
> + *
> + * 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.
> + */
> +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.
If you still want to keep this check then a WARN_ON.
> +
> + /*
> + * 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.
> + 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?
> +#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'.
> + 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.
> +
> + 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
Powered by blists - more mailing lists