diff mbox series

[v4,12/12] iommu/vt-d: Disallow nesting on domains with read-only mappings

Message ID 20230724111335.107427-13-yi.l.liu@intel.com (mailing list archive)
State New
Headers show
Series Add Intel VT-d nested translation | expand

Commit Message

Yi Liu July 24, 2023, 11:13 a.m. UTC 
From: Lu Baolu <baolu.lu@linux.intel.com>

When remapping hardware is configured by system software in scalable mode
as Nested (PGTT=011b) and with PWSNP field Set in the PASID-table-entry,
it may Set Accessed bit and Dirty bit (and Extended Access bit if enabled)
in first-stage page-table entries even when second-stage mappings indicate
that corresponding first-stage page-table is Read-Only.

As the result, contents of pages designated by VMM as Read-Only can be
modified by IOMMU via PML5E (PML4E for 4-level tables) access as part of
address translation process due to DMAs issued by Guest.

Disallow the nested translation when there are read-only pages in the
corresponding second-stage mappings. And, no read-only pages are allowed
to be configured in the second-stage table of a nested translation.

For simplicity the 2nd restriction is not relaxed even when the nesting
is turned off later due to vIOMMU config change. In concept if the user
understands this errata and does expect to enable nested translation
it should never install any RO mapping in stage-2 in the entire VM life
cycle. Accordingly introduce a single sticky bit to mark the parent role
on a domain instead of tracking the role with a counter.

Reference from Sapphire Rapids Specification Update [1], errata details,
SPR17.

[1] https://www.intel.com/content/www/us/en/content-details/772415/content-details.html

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c  | 14 ++++++++++++++
 drivers/iommu/intel/iommu.h  |  4 ++++
 drivers/iommu/intel/nested.c | 14 +++++++++++++-
 include/uapi/linux/iommufd.h | 12 +++++++++++-
 4 files changed, 42 insertions(+), 2 deletions(-)

Comments

Tian, Kevin Aug. 2, 2023, 7:59 a.m. UTC | #1 
> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Monday, July 24, 2023 7:14 PM
>
> @@ -2147,6 +2148,18 @@ __domain_mapping(struct dmar_domain
> *domain, unsigned long iov_pfn,
>  	if ((prot & (DMA_PTE_READ|DMA_PTE_WRITE)) == 0)
>  		return -EINVAL;
> 
> +	if (!(prot & DMA_PTE_WRITE) && !domain->read_only_mapped) {
> +		spin_lock_irqsave(&domain->lock, flags);
> +		if (domain->set_nested) {
> +			pr_err_ratelimited("No read-only mapping
> permitted\n");

"Read-only mapping is disallowed on the domain which serves as the
parent in a nested configuration, due to HW errata (ERRATA_772415_SPR17)"

> +	u8 read_only_mapped:1;		/* domain has mappings with
> read-only
> +					 * permission.
> +					 */
> +	u8 set_nested:1;		/* has other domains nested on it */

what about "is_parent"?

> 
> +	spin_lock_irqsave(&s2_dmar_domain->lock, flags);
> +	if (s2_dmar_domain->read_only_mapped) {
> +		spin_unlock_irqrestore(&s2_dmar_domain->lock, flags);
> +		pr_err_ratelimited("S2 domain has read-only mappings\n");

"Nested configuration is disallowed when the stage-2 domain already
has read-only mappings, due to HW errata (ERRATA_772415_SPR17)"
Baolu Lu Aug. 3, 2023, 3:27 a.m. UTC | #2 
On 2023/8/2 15:59, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Monday, July 24, 2023 7:14 PM
>>
>> @@ -2147,6 +2148,18 @@ __domain_mapping(struct dmar_domain
>> *domain, unsigned long iov_pfn,
>>   	if ((prot & (DMA_PTE_READ|DMA_PTE_WRITE)) == 0)
>>   		return -EINVAL;
>>
>> +	if (!(prot & DMA_PTE_WRITE) && !domain->read_only_mapped) {
>> +		spin_lock_irqsave(&domain->lock, flags);
>> +		if (domain->set_nested) {
>> +			pr_err_ratelimited("No read-only mapping
>> permitted\n");
> 
> "Read-only mapping is disallowed on the domain which serves as the
> parent in a nested configuration, due to HW errata (ERRATA_772415_SPR17)"

Ack.

> 
>> +	u8 read_only_mapped:1;		/* domain has mappings with
>> read-only
>> +					 * permission.
>> +					 */
>> +	u8 set_nested:1;		/* has other domains nested on it */
> 
> what about "is_parent"?

"is_parent" is still a bit generic. How about "is_nested_parent"?

> 
>>
>> +	spin_lock_irqsave(&s2_dmar_domain->lock, flags);
>> +	if (s2_dmar_domain->read_only_mapped) {
>> +		spin_unlock_irqrestore(&s2_dmar_domain->lock, flags);
>> +		pr_err_ratelimited("S2 domain has read-only mappings\n");
> 
> "Nested configuration is disallowed when the stage-2 domain already
> has read-only mappings, due to HW errata (ERRATA_772415_SPR17)"

Ack.

Best regards,
baolu
Tian, Kevin Aug. 3, 2023, 4 a.m. UTC | #3 
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, August 3, 2023 11:27 AM
> 
> On 2023/8/2 15:59, Tian, Kevin wrote:
> >> From: Liu, Yi L <yi.l.liu@intel.com>
> >> Sent: Monday, July 24, 2023 7:14 PM
> >>
> >> +	u8 read_only_mapped:1;		/* domain has mappings with
> >> read-only
> >> +					 * permission.
> >> +					 */
> >> +	u8 set_nested:1;		/* has other domains nested on it */
> >
> > what about "is_parent"?
> 
> "is_parent" is still a bit generic. How about "is_nested_parent"?
> 

looks good.
diff mbox series

Patch

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index ba34827045e6..caaa3a58dc94 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -2138,6 +2138,7 @@  __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
 	struct dma_pte *first_pte = NULL, *pte = NULL;
 	unsigned int largepage_lvl = 0;
 	unsigned long lvl_pages = 0;
+	unsigned long flags;
 	phys_addr_t pteval;
 	u64 attr;
 
@@ -2147,6 +2148,18 @@  __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
 	if ((prot & (DMA_PTE_READ|DMA_PTE_WRITE)) == 0)
 		return -EINVAL;
 
+	if (!(prot & DMA_PTE_WRITE) && !domain->read_only_mapped) {
+		spin_lock_irqsave(&domain->lock, flags);
+		if (domain->set_nested) {
+			pr_err_ratelimited("No read-only mapping permitted\n");
+			spin_unlock_irqrestore(&domain->lock, flags);
+			return -EINVAL;
+		}
+
+		domain->read_only_mapped = true;
+		spin_unlock_irqrestore(&domain->lock, flags);
+	}
+
 	attr = prot & (DMA_PTE_READ | DMA_PTE_WRITE | DMA_PTE_SNP);
 	attr |= DMA_FL_PTE_PRESENT;
 	if (domain->use_first_level) {
@@ -4758,6 +4771,7 @@  static void *intel_iommu_hw_info(struct device *dev, u32 *length)
 	if (!vtd)
 		return ERR_PTR(-ENOMEM);
 
+	vtd->flags = IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17;
 	vtd->cap_reg = iommu->cap;
 	vtd->ecap_reg = iommu->ecap;
 	*length = sizeof(*vtd);
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 5b292213bcb8..2a14fab6ac4f 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -592,6 +592,10 @@  struct dmar_domain {
 					 * otherwise, goes through the second
 					 * level.
 					 */
+	u8 read_only_mapped:1;		/* domain has mappings with read-only
+					 * permission.
+					 */
+	u8 set_nested:1;		/* has other domains nested on it */
 
 	spinlock_t lock;		/* Protect device tracking lists */
 	struct list_head devices;	/* all devices' list */
diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
index 2739c0d7880d..50934da613fa 100644
--- a/drivers/iommu/intel/nested.c
+++ b/drivers/iommu/intel/nested.c
@@ -142,14 +142,26 @@  struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *s2_domain,
 					       const union iommu_domain_user_data *user_data)
 {
 	const struct iommu_hwpt_vtd_s1 *vtd = (struct iommu_hwpt_vtd_s1 *)user_data;
+	struct dmar_domain *s2_dmar_domain = to_dmar_domain(s2_domain);
 	struct dmar_domain *domain;
+	unsigned long flags;
 
 	domain = kzalloc(sizeof(*domain), GFP_KERNEL_ACCOUNT);
 	if (!domain)
 		return NULL;
 
+	spin_lock_irqsave(&s2_dmar_domain->lock, flags);
+	if (s2_dmar_domain->read_only_mapped) {
+		spin_unlock_irqrestore(&s2_dmar_domain->lock, flags);
+		pr_err_ratelimited("S2 domain has read-only mappings\n");
+		kfree(domain);
+		return NULL;
+	}
+	s2_dmar_domain->set_nested = true;
+	spin_unlock_irqrestore(&s2_dmar_domain->lock, flags);
+
 	domain->use_first_level = true;
-	domain->s2_domain = to_dmar_domain(s2_domain);
+	domain->s2_domain = s2_dmar_domain;
 	domain->s1_pgtbl = vtd->pgtbl_addr;
 	domain->s1_cfg = *vtd;
 	domain->domain.ops = &intel_nested_domain_ops;
diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
index 0dfb6f3d8dda..2f8f2dab95a7 100644
--- a/include/uapi/linux/iommufd.h
+++ b/include/uapi/linux/iommufd.h
@@ -435,10 +435,20 @@  struct iommu_hwpt_alloc {
 };
 #define IOMMU_HWPT_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_HWPT_ALLOC)
 
+/**
+ * enum iommu_hw_info_vtd_flags - Flags for VT-d hw_info
+ * @IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17: If set, disallow nesting on domains
+ *                                   with read-only mapping.
+ *                                   https://www.intel.com/content/www/us/en/content-details/772415/content-details.html
+ */
+enum iommu_hw_info_vtd_flags {
+	IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17 = 1 << 0,
+};
+
 /**
  * struct iommu_hw_info_vtd - Intel VT-d hardware information
  *
- * @flags: Must be 0
+ * @flags: Combination of enum iommu_hw_info_vtd_flags
  * @__reserved: Must be 0
  *
  * @cap_reg: Value of Intel VT-d capability register defined in VT-d spec