[PATCH 0/3] intel_iommu: Add missed sanity check for invalidae descriptor

[PATCH 0/3] intel_iommu: Add missed sanity check for invalidae descriptor

[Zhenzhong Duan]

Hi,

This adds missed sanity check when IQ size is 256-bit per Yi's suggestion,
see patch for details.

I don't add check for VTD_INV_DESC_PC and VTD_INV_DESC_PIOTLB, will do it
in "intel_iommu: Enable stage-1 translation for emulated device" series.

Thanks
Zhenzhong

Zhenzhong Duan (3):
  intel_iommu: Send IQE event when setting reserved bit in IQT_TAIL
  intel_iommu: Add missed sanity check for 256-bit invalidation queue
  intel_iommu: Add missed reserved bit check for IEC descriptor

 hw/i386/intel_iommu_internal.h |  4 ++
 hw/i386/intel_iommu.c          | 89 +++++++++++++++++++++++++---------
 2 files changed, 71 insertions(+), 22 deletions(-)

-- 
2.34.1

[PATCH 1/3] intel_iommu: Send IQE event when setting reserved bit in IQT_TAIL

[Zhenzhong Duan]

According to VTD spec, Figure 11-22, Invalidation Queue Tail Register,
"When Descriptor Width (DW) field in Invalidation Queue Address Register
(IQA_REG) is Set (256-bit descriptors), hardware treats bit-4 as reserved
and a value of 1 in the bit will result in invalidation queue error."

Current code missed to send IQE event to guest, fix it.

Fixes: c0c1d351849b ("intel_iommu: add 256 bits qi_desc support")
Suggested-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/i386/intel_iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 8612d0917b..1ecfe47963 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2847,6 +2847,7 @@ static void vtd_handle_iqt_write(IntelIOMMUState *s)
     if (s->iq_dw && (val & VTD_IQT_QT_256_RSV_BIT)) {
         error_report_once("%s: RSV bit is set: val=0x%"PRIx64,
                           __func__, val);
+        vtd_handle_inv_queue_error(s);
         return;
     }
     s->iq_tail = VTD_IQT_QT(s->iq_dw, val);
-- 
2.34.1

[PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue

[Zhenzhong Duan]

According to VTD spec, a 256-bit descriptor will result in an invalid
descriptor error if submitted in an IQ that is setup to provide hardware
with 128-bit descriptors (IQA_REG.DW=0). Meanwhile, there are old inv desc
types (e.g. iotlb_inv_desc) that can be either 128bits or 256bits. If a
128-bit version of this descriptor is submitted into an IQ that is setup
to provide hardware with 256-bit descriptors will also result in an invalid
descriptor error.

The 2nd will be captured by the tail register update. So we only need to
focus on the 1st.

Because the reserved bit check between different types of invalidation desc
are common, so introduce a common function vtd_inv_desc_reserved_check()
to do all the checks and pass the differences as parameters.

With this change, need to replace error_report_once() call with error_report()
to catch different call sites. This isn't an issue as error_report_once()
here is mainly used to help debug guest error, but it only dumps once in
qemu life cycle and doesn't help much, we need error_report() instead.

Fixes: c0c1d351849b ("intel_iommu: add 256 bits qi_desc support")
Suggested-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/i386/intel_iommu_internal.h |  1 +
 hw/i386/intel_iommu.c          | 80 ++++++++++++++++++++++++----------
 2 files changed, 59 insertions(+), 22 deletions(-)

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 2f9bc0147d..75ccd501b0 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -356,6 +356,7 @@ union VTDInvDesc {
 typedef union VTDInvDesc VTDInvDesc;
 
 /* Masks for struct VTDInvDesc */
+#define VTD_INV_DESC_ALL_ONE            -1ULL
 #define VTD_INV_DESC_TYPE(val)          ((((val) >> 5) & 0x70ULL) | \
                                          ((val) & 0xfULL))
 #define VTD_INV_DESC_CC                 0x1 /* Context-cache Invalidate Desc */
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 1ecfe47963..2fc3866433 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2532,15 +2532,51 @@ static bool vtd_get_inv_desc(IntelIOMMUState *s,
     return true;
 }
 
+static bool vtd_inv_desc_reserved_check(IntelIOMMUState *s,
+                                        VTDInvDesc *inv_desc,
+                                        uint64_t mask[4], bool dw,
+                                        const char *func_name,
+                                        const char *desc_type)
+{
+    if (s->iq_dw) {
+        if (inv_desc->val[0] & mask[0] || inv_desc->val[1] & mask[1] ||
+            inv_desc->val[2] & mask[2] || inv_desc->val[3] & mask[3]) {
+            error_report("%s: invalid %s desc val[3]: 0x%"PRIx64
+                         " val[2]: 0x%"PRIx64" val[1]=0x%"PRIx64
+                         " val[0]=0x%"PRIx64" (reserved nonzero)",
+                         func_name, desc_type, inv_desc->val[3],
+                         inv_desc->val[2], inv_desc->val[1],
+                         inv_desc->val[0]);
+            return false;
+        }
+    } else {
+        if (dw) {
+            error_report("%s: 256-bit %s desc in 128-bit invalidation queue",
+                         func_name, desc_type);
+            return false;
+        }
+
+        if (inv_desc->lo & mask[0] || inv_desc->hi & mask[1]) {
+            error_report("%s: invalid %s desc: hi=%"PRIx64", lo=%"PRIx64
+                         " (reserved nonzero)", func_name, desc_type,
+                         inv_desc->hi, inv_desc->lo);
+            return false;
+        }
+    }
+
+    return true;
+}
+
 static bool vtd_process_wait_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
 {
-    if ((inv_desc->hi & VTD_INV_DESC_WAIT_RSVD_HI) ||
-        (inv_desc->lo & VTD_INV_DESC_WAIT_RSVD_LO)) {
-        error_report_once("%s: invalid wait desc: hi=%"PRIx64", lo=%"PRIx64
-                          " (reserved nonzero)", __func__, inv_desc->hi,
-                          inv_desc->lo);
+    uint64_t mask[4] = {VTD_INV_DESC_WAIT_RSVD_LO, VTD_INV_DESC_WAIT_RSVD_HI,
+                        VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};
+
+    if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,
+                                     __func__, "wait")) {
         return false;
     }
+
     if (inv_desc->lo & VTD_INV_DESC_WAIT_SW) {
         /* Status Write */
         uint32_t status_data = (uint32_t)(inv_desc->lo >>
@@ -2574,13 +2610,14 @@ static bool vtd_process_context_cache_desc(IntelIOMMUState *s,
                                            VTDInvDesc *inv_desc)
 {
     uint16_t sid, fmask;
+    uint64_t mask[4] = {VTD_INV_DESC_CC_RSVD, VTD_INV_DESC_ALL_ONE,
+                        VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};
 
-    if ((inv_desc->lo & VTD_INV_DESC_CC_RSVD) || inv_desc->hi) {
-        error_report_once("%s: invalid cc inv desc: hi=%"PRIx64", lo=%"PRIx64
-                          " (reserved nonzero)", __func__, inv_desc->hi,
-                          inv_desc->lo);
+    if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,
+                                     __func__, "cc inv")) {
         return false;
     }
+
     switch (inv_desc->lo & VTD_INV_DESC_CC_G) {
     case VTD_INV_DESC_CC_DOMAIN:
         trace_vtd_inv_desc_cc_domain(
@@ -2610,12 +2647,11 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
     uint16_t domain_id;
     uint8_t am;
     hwaddr addr;
+    uint64_t mask[4] = {VTD_INV_DESC_IOTLB_RSVD_LO, VTD_INV_DESC_IOTLB_RSVD_HI,
+                        VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};
 
-    if ((inv_desc->lo & VTD_INV_DESC_IOTLB_RSVD_LO) ||
-        (inv_desc->hi & VTD_INV_DESC_IOTLB_RSVD_HI)) {
-        error_report_once("%s: invalid iotlb inv desc: hi=0x%"PRIx64
-                          ", lo=0x%"PRIx64" (reserved bits unzero)",
-                          __func__, inv_desc->hi, inv_desc->lo);
+    if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,
+                                     __func__, "iotlb inv")) {
         return false;
     }
 
@@ -2705,19 +2741,19 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s,
     hwaddr addr;
     uint16_t sid;
     bool size;
+    uint64_t mask[4] = {VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO,
+                        VTD_INV_DESC_DEVICE_IOTLB_RSVD_HI,
+                        VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};
+
+    if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,
+                                     __func__, "dev-iotlb inv")) {
+        return false;
+    }
 
     addr = VTD_INV_DESC_DEVICE_IOTLB_ADDR(inv_desc->hi);
     sid = VTD_INV_DESC_DEVICE_IOTLB_SID(inv_desc->lo);
     size = VTD_INV_DESC_DEVICE_IOTLB_SIZE(inv_desc->hi);
 
-    if ((inv_desc->lo & VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO) ||
-        (inv_desc->hi & VTD_INV_DESC_DEVICE_IOTLB_RSVD_HI)) {
-        error_report_once("%s: invalid dev-iotlb inv desc: hi=%"PRIx64
-                          ", lo=%"PRIx64" (reserved nonzero)", __func__,
-                          inv_desc->hi, inv_desc->lo);
-        return false;
-    }
-
     /*
      * Using sid is OK since the guest should have finished the
      * initialization of both the bus and device.
-- 
2.34.1

[PATCH 3/3] intel_iommu: Add missed reserved bit check for IEC descriptor

[Zhenzhong Duan]

IEC descriptor is 128-bit invalidation descriptor, must be padded with
128-bits of 0s in the upper bytes to create a 256-bit descriptor when
the invalidation queue is configured for 256-bit descriptors (IQA_REG.DW=1).

Fixes: 02a2cbc872df ("x86-iommu: introduce IEC notifiers")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/i386/intel_iommu_internal.h | 3 +++
 hw/i386/intel_iommu.c          | 8 ++++++++
 2 files changed, 11 insertions(+)

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 75ccd501b0..4323fc5d6d 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -410,6 +410,9 @@ typedef union VTDInvDesc VTDInvDesc;
 #define VTD_INV_DESC_DEVICE_IOTLB_RSVD_HI 0xffeULL
 #define VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO 0xffff0000ffe0f1f0
 
+/* Masks for Interrupt Entry Invalidate Descriptor */
+#define VTD_INV_DESC_IEC_RSVD           0xffff000007fff1e0ULL
+
 /* Rsvd field masks for spte */
 #define VTD_SPTE_SNP 0x800ULL
 
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 2fc3866433..4c0d1d7d47 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2692,6 +2692,14 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
 static bool vtd_process_inv_iec_desc(IntelIOMMUState *s,
                                      VTDInvDesc *inv_desc)
 {
+    uint64_t mask[4] = {VTD_INV_DESC_IEC_RSVD, VTD_INV_DESC_ALL_ONE,
+                        VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};
+
+    if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,
+                                     __func__, "iec inv")) {
+        return false;
+    }
+
     trace_vtd_inv_desc_iec(inv_desc->iec.granularity,
                            inv_desc->iec.index,
                            inv_desc->iec.index_mask);
-- 
2.34.1

Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue

[Michael S. Tsirkin]

On Mon, Nov 04, 2024 at 08:55:35PM +0800, Zhenzhong Duan wrote:
> According to VTD spec, a 256-bit descriptor will result in an invalid
> descriptor error if submitted in an IQ that is setup to provide hardware
> with 128-bit descriptors (IQA_REG.DW=0). Meanwhile, there are old inv desc
> types (e.g. iotlb_inv_desc) that can be either 128bits or 256bits. If a
> 128-bit version of this descriptor is submitted into an IQ that is setup
> to provide hardware with 256-bit descriptors will also result in an invalid
> descriptor error.
> 
> The 2nd will be captured by the tail register update. So we only need to
> focus on the 1st.
> 
> Because the reserved bit check between different types of invalidation desc
> are common, so introduce a common function vtd_inv_desc_reserved_check()
> to do all the checks and pass the differences as parameters.
> 
> With this change, need to replace error_report_once() call with error_report()
> to catch different call sites. This isn't an issue as error_report_once()
> here is mainly used to help debug guest error, but it only dumps once in
> qemu life cycle and doesn't help much, we need error_report() instead.
> 
> Fixes: c0c1d351849b ("intel_iommu: add 256 bits qi_desc support")
> Suggested-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  hw/i386/intel_iommu_internal.h |  1 +
>  hw/i386/intel_iommu.c          | 80 ++++++++++++++++++++++++----------
>  2 files changed, 59 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 2f9bc0147d..75ccd501b0 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -356,6 +356,7 @@ union VTDInvDesc {
>  typedef union VTDInvDesc VTDInvDesc;
>  
>  /* Masks for struct VTDInvDesc */
> +#define VTD_INV_DESC_ALL_ONE            -1ULL
>  #define VTD_INV_DESC_TYPE(val)          ((((val) >> 5) & 0x70ULL) | \
>                                           ((val) & 0xfULL))
>  #define VTD_INV_DESC_CC                 0x1 /* Context-cache Invalidate Desc */
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 1ecfe47963..2fc3866433 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2532,15 +2532,51 @@ static bool vtd_get_inv_desc(IntelIOMMUState *s,
>      return true;
>  }
>  
> +static bool vtd_inv_desc_reserved_check(IntelIOMMUState *s,
> +                                        VTDInvDesc *inv_desc,
> +                                        uint64_t mask[4], bool dw,
> +                                        const char *func_name,
> +                                        const char *desc_type)
> +{
> +    if (s->iq_dw) {
> +        if (inv_desc->val[0] & mask[0] || inv_desc->val[1] & mask[1] ||
> +            inv_desc->val[2] & mask[2] || inv_desc->val[3] & mask[3]) {
> +            error_report("%s: invalid %s desc val[3]: 0x%"PRIx64
> +                         " val[2]: 0x%"PRIx64" val[1]=0x%"PRIx64
> +                         " val[0]=0x%"PRIx64" (reserved nonzero)",
> +                         func_name, desc_type, inv_desc->val[3],
> +                         inv_desc->val[2], inv_desc->val[1],
> +                         inv_desc->val[0]);

Hmm.
But these are guest errors.
should all these actually be

qemu_log_mask(LOG_GUEST_ERROR, ...)


?


> +            return false;
> +        }
> +    } else {
> +        if (dw) {
> +            error_report("%s: 256-bit %s desc in 128-bit invalidation queue",
> +                         func_name, desc_type);
> +            return false;
> +        }
> +
> +        if (inv_desc->lo & mask[0] || inv_desc->hi & mask[1]) {
> +            error_report("%s: invalid %s desc: hi=%"PRIx64", lo=%"PRIx64
> +                         " (reserved nonzero)", func_name, desc_type,
> +                         inv_desc->hi, inv_desc->lo);
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
>  static bool vtd_process_wait_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
>  {
> -    if ((inv_desc->hi & VTD_INV_DESC_WAIT_RSVD_HI) ||
> -        (inv_desc->lo & VTD_INV_DESC_WAIT_RSVD_LO)) {
> -        error_report_once("%s: invalid wait desc: hi=%"PRIx64", lo=%"PRIx64
> -                          " (reserved nonzero)", __func__, inv_desc->hi,
> -                          inv_desc->lo);
> +    uint64_t mask[4] = {VTD_INV_DESC_WAIT_RSVD_LO, VTD_INV_DESC_WAIT_RSVD_HI,
> +                        VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};
> +
> +    if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,
> +                                     __func__, "wait")) {
>          return false;
>      }
> +
>      if (inv_desc->lo & VTD_INV_DESC_WAIT_SW) {
>          /* Status Write */
>          uint32_t status_data = (uint32_t)(inv_desc->lo >>
> @@ -2574,13 +2610,14 @@ static bool vtd_process_context_cache_desc(IntelIOMMUState *s,
>                                             VTDInvDesc *inv_desc)
>  {
>      uint16_t sid, fmask;
> +    uint64_t mask[4] = {VTD_INV_DESC_CC_RSVD, VTD_INV_DESC_ALL_ONE,
> +                        VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};
>  
> -    if ((inv_desc->lo & VTD_INV_DESC_CC_RSVD) || inv_desc->hi) {
> -        error_report_once("%s: invalid cc inv desc: hi=%"PRIx64", lo=%"PRIx64
> -                          " (reserved nonzero)", __func__, inv_desc->hi,
> -                          inv_desc->lo);
> +    if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,
> +                                     __func__, "cc inv")) {
>          return false;
>      }
> +
>      switch (inv_desc->lo & VTD_INV_DESC_CC_G) {
>      case VTD_INV_DESC_CC_DOMAIN:
>          trace_vtd_inv_desc_cc_domain(
> @@ -2610,12 +2647,11 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
>      uint16_t domain_id;
>      uint8_t am;
>      hwaddr addr;
> +    uint64_t mask[4] = {VTD_INV_DESC_IOTLB_RSVD_LO, VTD_INV_DESC_IOTLB_RSVD_HI,
> +                        VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};
>  
> -    if ((inv_desc->lo & VTD_INV_DESC_IOTLB_RSVD_LO) ||
> -        (inv_desc->hi & VTD_INV_DESC_IOTLB_RSVD_HI)) {
> -        error_report_once("%s: invalid iotlb inv desc: hi=0x%"PRIx64
> -                          ", lo=0x%"PRIx64" (reserved bits unzero)",
> -                          __func__, inv_desc->hi, inv_desc->lo);
> +    if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,
> +                                     __func__, "iotlb inv")) {
>          return false;
>      }
>  
> @@ -2705,19 +2741,19 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s,
>      hwaddr addr;
>      uint16_t sid;
>      bool size;
> +    uint64_t mask[4] = {VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO,
> +                        VTD_INV_DESC_DEVICE_IOTLB_RSVD_HI,
> +                        VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};
> +
> +    if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,
> +                                     __func__, "dev-iotlb inv")) {
> +        return false;
> +    }
>  
>      addr = VTD_INV_DESC_DEVICE_IOTLB_ADDR(inv_desc->hi);
>      sid = VTD_INV_DESC_DEVICE_IOTLB_SID(inv_desc->lo);
>      size = VTD_INV_DESC_DEVICE_IOTLB_SIZE(inv_desc->hi);
>  
> -    if ((inv_desc->lo & VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO) ||
> -        (inv_desc->hi & VTD_INV_DESC_DEVICE_IOTLB_RSVD_HI)) {
> -        error_report_once("%s: invalid dev-iotlb inv desc: hi=%"PRIx64
> -                          ", lo=%"PRIx64" (reserved nonzero)", __func__,
> -                          inv_desc->hi, inv_desc->lo);
> -        return false;
> -    }
> -
>      /*
>       * Using sid is OK since the guest should have finished the
>       * initialization of both the bus and device.
> -- 
> 2.34.1

RE: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue

[Duan, Zhenzhong]

>-----Original Message-----
>From: Michael S. Tsirkin <mst@redhat.com>
>Subject: Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit
>invalidation queue
>
>On Mon, Nov 04, 2024 at 08:55:35PM +0800, Zhenzhong Duan wrote:
>> According to VTD spec, a 256-bit descriptor will result in an invalid
>> descriptor error if submitted in an IQ that is setup to provide hardware
>> with 128-bit descriptors (IQA_REG.DW=0). Meanwhile, there are old inv desc
>> types (e.g. iotlb_inv_desc) that can be either 128bits or 256bits. If a
>> 128-bit version of this descriptor is submitted into an IQ that is setup
>> to provide hardware with 256-bit descriptors will also result in an invalid
>> descriptor error.
>>
>> The 2nd will be captured by the tail register update. So we only need to
>> focus on the 1st.
>>
>> Because the reserved bit check between different types of invalidation desc
>> are common, so introduce a common function vtd_inv_desc_reserved_check()
>> to do all the checks and pass the differences as parameters.
>>
>> With this change, need to replace error_report_once() call with error_report()
>> to catch different call sites. This isn't an issue as error_report_once()
>> here is mainly used to help debug guest error, but it only dumps once in
>> qemu life cycle and doesn't help much, we need error_report() instead.
>>
>> Fixes: c0c1d351849b ("intel_iommu: add 256 bits qi_desc support")
>> Suggested-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  hw/i386/intel_iommu_internal.h |  1 +
>>  hw/i386/intel_iommu.c          | 80 ++++++++++++++++++++++++----------
>>  2 files changed, 59 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
>> index 2f9bc0147d..75ccd501b0 100644
>> --- a/hw/i386/intel_iommu_internal.h
>> +++ b/hw/i386/intel_iommu_internal.h
>> @@ -356,6 +356,7 @@ union VTDInvDesc {
>>  typedef union VTDInvDesc VTDInvDesc;
>>
>>  /* Masks for struct VTDInvDesc */
>> +#define VTD_INV_DESC_ALL_ONE            -1ULL
>>  #define VTD_INV_DESC_TYPE(val)          ((((val) >> 5) & 0x70ULL) | \
>>                                           ((val) & 0xfULL))
>>  #define VTD_INV_DESC_CC                 0x1 /* Context-cache Invalidate Desc */
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 1ecfe47963..2fc3866433 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -2532,15 +2532,51 @@ static bool vtd_get_inv_desc(IntelIOMMUState *s,
>>      return true;
>>  }
>>
>> +static bool vtd_inv_desc_reserved_check(IntelIOMMUState *s,
>> +                                        VTDInvDesc *inv_desc,
>> +                                        uint64_t mask[4], bool dw,
>> +                                        const char *func_name,
>> +                                        const char *desc_type)
>> +{
>> +    if (s->iq_dw) {
>> +        if (inv_desc->val[0] & mask[0] || inv_desc->val[1] & mask[1] ||
>> +            inv_desc->val[2] & mask[2] || inv_desc->val[3] & mask[3]) {
>> +            error_report("%s: invalid %s desc val[3]: 0x%"PRIx64
>> +                         " val[2]: 0x%"PRIx64" val[1]=0x%"PRIx64
>> +                         " val[0]=0x%"PRIx64" (reserved nonzero)",
>> +                         func_name, desc_type, inv_desc->val[3],
>> +                         inv_desc->val[2], inv_desc->val[1],
>> +                         inv_desc->val[0]);
>
>Hmm.
>But these are guest errors.
>should all these actually be
>
>qemu_log_mask(LOG_GUEST_ERROR, ...)
>
>
>?
Yes, make sense. I see you have sent pull request, not clear
if error_report() is reluctantly ok for you or I should send a fix.

Thanks
Zhenzhong

Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue

[Yi Liu]

On 2024/11/4 20:55, Zhenzhong Duan wrote:
> According to VTD spec, a 256-bit descriptor will result in an invalid
> descriptor error if submitted in an IQ that is setup to provide hardware
> with 128-bit descriptors (IQA_REG.DW=0). Meanwhile, there are old inv desc
> types (e.g. iotlb_inv_desc) that can be either 128bits or 256bits. If a
> 128-bit version of this descriptor is submitted into an IQ that is setup
> to provide hardware with 256-bit descriptors will also result in an invalid
> descriptor error.
> 
> The 2nd will be captured by the tail register update. So we only need to
> focus on the 1st.
> 
> Because the reserved bit check between different types of invalidation desc
> are common, so introduce a common function vtd_inv_desc_reserved_check()
> to do all the checks and pass the differences as parameters.
> 
> With this change, need to replace error_report_once() call with error_report()
> to catch different call sites. This isn't an issue as error_report_once()
> here is mainly used to help debug guest error, but it only dumps once in
> qemu life cycle and doesn't help much, we need error_report() instead.
> 
> Fixes: c0c1d351849b ("intel_iommu: add 256 bits qi_desc support")
> Suggested-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/i386/intel_iommu_internal.h |  1 +
>   hw/i386/intel_iommu.c          | 80 ++++++++++++++++++++++++----------
>   2 files changed, 59 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 2f9bc0147d..75ccd501b0 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -356,6 +356,7 @@ union VTDInvDesc {
>   typedef union VTDInvDesc VTDInvDesc;
>   
>   /* Masks for struct VTDInvDesc */
> +#define VTD_INV_DESC_ALL_ONE            -1ULL
>   #define VTD_INV_DESC_TYPE(val)          ((((val) >> 5) & 0x70ULL) | \
>                                            ((val) & 0xfULL))
>   #define VTD_INV_DESC_CC                 0x1 /* Context-cache Invalidate Desc */
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 1ecfe47963..2fc3866433 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2532,15 +2532,51 @@ static bool vtd_get_inv_desc(IntelIOMMUState *s,
>       return true;
>   }
>   
> +static bool vtd_inv_desc_reserved_check(IntelIOMMUState *s,
> +                                        VTDInvDesc *inv_desc,
> +                                        uint64_t mask[4], bool dw,
> +                                        const char *func_name,
> +                                        const char *desc_type)
> +{
> +    if (s->iq_dw) {
> +        if (inv_desc->val[0] & mask[0] || inv_desc->val[1] & mask[1] ||
> +            inv_desc->val[2] & mask[2] || inv_desc->val[3] & mask[3]) {
> +            error_report("%s: invalid %s desc val[3]: 0x%"PRIx64
> +                         " val[2]: 0x%"PRIx64" val[1]=0x%"PRIx64
> +                         " val[0]=0x%"PRIx64" (reserved nonzero)",
> +                         func_name, desc_type, inv_desc->val[3],
> +                         inv_desc->val[2], inv_desc->val[1],
> +                         inv_desc->val[0]);
> +            return false;
> +        }
> +    } else {
> +        if (dw) {
> +            error_report("%s: 256-bit %s desc in 128-bit invalidation queue",
> +                         func_name, desc_type);
> +            return false;
> +        }
> +

If a respin is made, I'd prefer to move this check out of this helper since
it's not about reserved bit check. Another reason is you cannot find a good
naming for the @dw parameter. It's confusing as s->iq_dw is checked as
well. So put this check out of this helper may be better.

-- 
Regards,
Yi Liu

RE: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue

[Duan, Zhenzhong]

>-----Original Message-----
>From: Liu, Yi L <yi.l.liu@intel.com>
>Sent: Tuesday, November 5, 2024 1:05 PM
>Subject: Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit
>invalidation queue
>
>On 2024/11/4 20:55, Zhenzhong Duan wrote:
>> According to VTD spec, a 256-bit descriptor will result in an invalid
>> descriptor error if submitted in an IQ that is setup to provide hardware
>> with 128-bit descriptors (IQA_REG.DW=0). Meanwhile, there are old inv desc
>> types (e.g. iotlb_inv_desc) that can be either 128bits or 256bits. If a
>> 128-bit version of this descriptor is submitted into an IQ that is setup
>> to provide hardware with 256-bit descriptors will also result in an invalid
>> descriptor error.
>>
>> The 2nd will be captured by the tail register update. So we only need to
>> focus on the 1st.
>>
>> Because the reserved bit check between different types of invalidation desc
>> are common, so introduce a common function vtd_inv_desc_reserved_check()
>> to do all the checks and pass the differences as parameters.
>>
>> With this change, need to replace error_report_once() call with error_report()
>> to catch different call sites. This isn't an issue as error_report_once()
>> here is mainly used to help debug guest error, but it only dumps once in
>> qemu life cycle and doesn't help much, we need error_report() instead.
>>
>> Fixes: c0c1d351849b ("intel_iommu: add 256 bits qi_desc support")
>> Suggested-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   hw/i386/intel_iommu_internal.h |  1 +
>>   hw/i386/intel_iommu.c          | 80 ++++++++++++++++++++++++----------
>>   2 files changed, 59 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
>> index 2f9bc0147d..75ccd501b0 100644
>> --- a/hw/i386/intel_iommu_internal.h
>> +++ b/hw/i386/intel_iommu_internal.h
>> @@ -356,6 +356,7 @@ union VTDInvDesc {
>>   typedef union VTDInvDesc VTDInvDesc;
>>
>>   /* Masks for struct VTDInvDesc */
>> +#define VTD_INV_DESC_ALL_ONE            -1ULL
>>   #define VTD_INV_DESC_TYPE(val)          ((((val) >> 5) & 0x70ULL) | \
>>                                            ((val) & 0xfULL))
>>   #define VTD_INV_DESC_CC                 0x1 /* Context-cache Invalidate Desc */
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 1ecfe47963..2fc3866433 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -2532,15 +2532,51 @@ static bool vtd_get_inv_desc(IntelIOMMUState *s,
>>       return true;
>>   }
>>
>> +static bool vtd_inv_desc_reserved_check(IntelIOMMUState *s,
>> +                                        VTDInvDesc *inv_desc,
>> +                                        uint64_t mask[4], bool dw,
>> +                                        const char *func_name,
>> +                                        const char *desc_type)
>> +{
>> +    if (s->iq_dw) {
>> +        if (inv_desc->val[0] & mask[0] || inv_desc->val[1] & mask[1] ||
>> +            inv_desc->val[2] & mask[2] || inv_desc->val[3] & mask[3]) {
>> +            error_report("%s: invalid %s desc val[3]: 0x%"PRIx64
>> +                         " val[2]: 0x%"PRIx64" val[1]=0x%"PRIx64
>> +                         " val[0]=0x%"PRIx64" (reserved nonzero)",
>> +                         func_name, desc_type, inv_desc->val[3],
>> +                         inv_desc->val[2], inv_desc->val[1],
>> +                         inv_desc->val[0]);
>> +            return false;
>> +        }
>> +    } else {
>> +        if (dw) {
>> +            error_report("%s: 256-bit %s desc in 128-bit invalidation queue",
>> +                         func_name, desc_type);
>> +            return false;
>> +        }
>> +
>
>If a respin is made, I'd prefer to move this check out of this helper since
>it's not about reserved bit check. Another reason is you cannot find a good
>naming for the @dw parameter. It's confusing as s->iq_dw is checked as
>well. So put this check out of this helper may be better.

I see, @dw hints inv desc size, s->iq_dw hints the inv queue element size.
Moving that check out will produce duplicate code for VTD_INV_DESC_PC,
VTD_INV_DESC_PIOTLB and VTD_INV_DESC_DEV_PIOTLB handlers.
Maybe s/ vtd_inv_desc_reserved_check/ vtd_inv_desc_sanity_check?

Michael, let me know if it's viable to send a v2 after pull request.

Thanks
Zhenzhong

Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue

[CLEMENT MATHIEU--DRIF]

I saw the pull request, just a few questions/comments in case there is a 
new spin.
These are not hard requirements, the current version looks good as well.

On 04/11/2024 13:55, Zhenzhong Duan wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
> According to VTD spec, a 256-bit descriptor will result in an invalid
> descriptor error if submitted in an IQ that is setup to provide hardware
> with 128-bit descriptors (IQA_REG.DW=0). Meanwhile, there are old inv desc
> types (e.g. iotlb_inv_desc) that can be either 128bits or 256bits. If a
> 128-bit version of this descriptor is submitted into an IQ that is setup
> to provide hardware with 256-bit descriptors will also result in an invalid
> descriptor error.
>
> The 2nd will be captured by the tail register update. So we only need to
> focus on the 1st.
>
> Because the reserved bit check between different types of invalidation desc
> are common, so introduce a common function vtd_inv_desc_reserved_check()
> to do all the checks and pass the differences as parameters.
>
> With this change, need to replace error_report_once() call with error_report()
> to catch different call sites. This isn't an issue as error_report_once()
> here is mainly used to help debug guest error, but it only dumps once in
> qemu life cycle and doesn't help much, we need error_report() instead.
>
> Fixes: c0c1d351849b ("intel_iommu: add 256 bits qi_desc support")
> Suggested-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/i386/intel_iommu_internal.h |  1 +
>   hw/i386/intel_iommu.c          | 80 ++++++++++++++++++++++++----------
>   2 files changed, 59 insertions(+), 22 deletions(-)
>
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 2f9bc0147d..75ccd501b0 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -356,6 +356,7 @@ union VTDInvDesc {
>   typedef union VTDInvDesc VTDInvDesc;
>
>   /* Masks for struct VTDInvDesc */
> +#define VTD_INV_DESC_ALL_ONE            -1ULL

s/one/ones
And maybe ~0ull is better. It's up to you

>   #define VTD_INV_DESC_TYPE(val)          ((((val) >> 5) & 0x70ULL) | \
>                                            ((val) & 0xfULL))
>   #define VTD_INV_DESC_CC                 0x1 /* Context-cache Invalidate Desc */
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 1ecfe47963..2fc3866433 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2532,15 +2532,51 @@ static bool vtd_get_inv_desc(IntelIOMMUState *s,
>       return true;
>   }
>
> +static bool vtd_inv_desc_reserved_check(IntelIOMMUState *s,
> +                                        VTDInvDesc *inv_desc,
> +                                        uint64_t mask[4], bool dw,
> +                                        const char *func_name,
> +                                        const char *desc_type)
> +{
> +    if (s->iq_dw) {
> +        if (inv_desc->val[0] & mask[0] || inv_desc->val[1] & mask[1] ||
> +            inv_desc->val[2] & mask[2] || inv_desc->val[3] & mask[3]) {
> +            error_report("%s: invalid %s desc val[3]: 0x%"PRIx64
> +                         " val[2]: 0x%"PRIx64" val[1]=0x%"PRIx64
> +                         " val[0]=0x%"PRIx64" (reserved nonzero)",
> +                         func_name, desc_type, inv_desc->val[3],
> +                         inv_desc->val[2], inv_desc->val[1],
> +                         inv_desc->val[0]);
> +            return false;
> +        }
> +    } else {
> +        if (dw) {
> +            error_report("%s: 256-bit %s desc in 128-bit invalidation queue",
> +                         func_name, desc_type);
> +            return false;
> +        }
> +
> +        if (inv_desc->lo & mask[0] || inv_desc->hi & mask[1]) {
> +            error_report("%s: invalid %s desc: hi=%"PRIx64", lo=%"PRIx64
> +                         " (reserved nonzero)", func_name, desc_type,
> +                         inv_desc->hi, inv_desc->lo);
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
>   static bool vtd_process_wait_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
>   {
> -    if ((inv_desc->hi & VTD_INV_DESC_WAIT_RSVD_HI) ||
> -        (inv_desc->lo & VTD_INV_DESC_WAIT_RSVD_LO)) {
> -        error_report_once("%s: invalid wait desc: hi=%"PRIx64", lo=%"PRIx64
> -                          " (reserved nonzero)", __func__, inv_desc->hi,
> -                          inv_desc->lo);
> +    uint64_t mask[4] = {VTD_INV_DESC_WAIT_RSVD_LO, VTD_INV_DESC_WAIT_RSVD_HI,
> +                        VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};

Why don't we declare the full masks outside of the functions (called 
something like ..._DW_MASK)?

> +
> +    if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,

Maybe the dw argument should be declared using #define in the internal 
header.

> +                                     __func__, "wait")) {
>           return false;
>       }
> +
>       if (inv_desc->lo & VTD_INV_DESC_WAIT_SW) {
>           /* Status Write */
>           uint32_t status_data = (uint32_t)(inv_desc->lo >>
> @@ -2574,13 +2610,14 @@ static bool vtd_process_context_cache_desc(IntelIOMMUState *s,
>                                              VTDInvDesc *inv_desc)
>   {
>       uint16_t sid, fmask;
> +    uint64_t mask[4] = {VTD_INV_DESC_CC_RSVD, VTD_INV_DESC_ALL_ONE,
> +                        VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};
>
> -    if ((inv_desc->lo & VTD_INV_DESC_CC_RSVD) || inv_desc->hi) {
> -        error_report_once("%s: invalid cc inv desc: hi=%"PRIx64", lo=%"PRIx64
> -                          " (reserved nonzero)", __func__, inv_desc->hi,
> -                          inv_desc->lo);
> +    if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,
> +                                     __func__, "cc inv")) {
>           return false;
>       }
> +
>       switch (inv_desc->lo & VTD_INV_DESC_CC_G) {
>       case VTD_INV_DESC_CC_DOMAIN:
>           trace_vtd_inv_desc_cc_domain(
> @@ -2610,12 +2647,11 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
>       uint16_t domain_id;
>       uint8_t am;
>       hwaddr addr;
> +    uint64_t mask[4] = {VTD_INV_DESC_IOTLB_RSVD_LO, VTD_INV_DESC_IOTLB_RSVD_HI,
> +                        VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};
>
> -    if ((inv_desc->lo & VTD_INV_DESC_IOTLB_RSVD_LO) ||
> -        (inv_desc->hi & VTD_INV_DESC_IOTLB_RSVD_HI)) {
> -        error_report_once("%s: invalid iotlb inv desc: hi=0x%"PRIx64
> -                          ", lo=0x%"PRIx64" (reserved bits unzero)",
> -                          __func__, inv_desc->hi, inv_desc->lo);
> +    if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,
> +                                     __func__, "iotlb inv")) {
>           return false;
>       }
>
> @@ -2705,19 +2741,19 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s,
>       hwaddr addr;
>       uint16_t sid;
>       bool size;
> +    uint64_t mask[4] = {VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO,
> +                        VTD_INV_DESC_DEVICE_IOTLB_RSVD_HI,
> +                        VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};
> +
> +    if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,
> +                                     __func__, "dev-iotlb inv")) {
> +        return false;
> +    }
>
>       addr = VTD_INV_DESC_DEVICE_IOTLB_ADDR(inv_desc->hi);
>       sid = VTD_INV_DESC_DEVICE_IOTLB_SID(inv_desc->lo);
>       size = VTD_INV_DESC_DEVICE_IOTLB_SIZE(inv_desc->hi);
>
> -    if ((inv_desc->lo & VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO) ||
> -        (inv_desc->hi & VTD_INV_DESC_DEVICE_IOTLB_RSVD_HI)) {
> -        error_report_once("%s: invalid dev-iotlb inv desc: hi=%"PRIx64
> -                          ", lo=%"PRIx64" (reserved nonzero)", __func__,
> -                          inv_desc->hi, inv_desc->lo);
> -        return false;
> -    }
> -
>       /*
>        * Using sid is OK since the guest should have finished the
>        * initialization of both the bus and device.
> --
> 2.34.1
>

Re: [PATCH 1/3] intel_iommu: Send IQE event when setting reserved bit in IQT_TAIL

[CLEMENT MATHIEU--DRIF]

Hi,

lgtm

Thanks
cmd


On 04/11/2024 13:55, Zhenzhong Duan wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
> According to VTD spec, Figure 11-22, Invalidation Queue Tail Register,
> "When Descriptor Width (DW) field in Invalidation Queue Address Register
> (IQA_REG) is Set (256-bit descriptors), hardware treats bit-4 as reserved
> and a value of 1 in the bit will result in invalidation queue error."
>
> Current code missed to send IQE event to guest, fix it.
>
> Fixes: c0c1d351849b ("intel_iommu: add 256 bits qi_desc support")
> Suggested-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/i386/intel_iommu.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 8612d0917b..1ecfe47963 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2847,6 +2847,7 @@ static void vtd_handle_iqt_write(IntelIOMMUState *s)
>       if (s->iq_dw && (val & VTD_IQT_QT_256_RSV_BIT)) {
>           error_report_once("%s: RSV bit is set: val=0x%"PRIx64,
>                             __func__, val);
> +        vtd_handle_inv_queue_error(s);
>           return;
>       }
>       s->iq_tail = VTD_IQT_QT(s->iq_dw, val);
> --
> 2.34.1
>

Re: [PATCH 3/3] intel_iommu: Add missed reserved bit check for IEC descriptor

[CLEMENT MATHIEU--DRIF]

Hi,

lgtm

Thanks
cmd



On 04/11/2024 13:55, Zhenzhong Duan wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
> IEC descriptor is 128-bit invalidation descriptor, must be padded with
> 128-bits of 0s in the upper bytes to create a 256-bit descriptor when
> the invalidation queue is configured for 256-bit descriptors (IQA_REG.DW=1).
>
> Fixes: 02a2cbc872df ("x86-iommu: introduce IEC notifiers")
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/i386/intel_iommu_internal.h | 3 +++
>   hw/i386/intel_iommu.c          | 8 ++++++++
>   2 files changed, 11 insertions(+)
>
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 75ccd501b0..4323fc5d6d 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -410,6 +410,9 @@ typedef union VTDInvDesc VTDInvDesc;
>   #define VTD_INV_DESC_DEVICE_IOTLB_RSVD_HI 0xffeULL
>   #define VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO 0xffff0000ffe0f1f0
>
> +/* Masks for Interrupt Entry Invalidate Descriptor */
> +#define VTD_INV_DESC_IEC_RSVD           0xffff000007fff1e0ULL
> +
>   /* Rsvd field masks for spte */
>   #define VTD_SPTE_SNP 0x800ULL
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 2fc3866433..4c0d1d7d47 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2692,6 +2692,14 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
>   static bool vtd_process_inv_iec_desc(IntelIOMMUState *s,
>                                        VTDInvDesc *inv_desc)
>   {
> +    uint64_t mask[4] = {VTD_INV_DESC_IEC_RSVD, VTD_INV_DESC_ALL_ONE,
> +                        VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};
> +
> +    if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,
> +                                     __func__, "iec inv")) {
> +        return false;
> +    }
> +
>       trace_vtd_inv_desc_iec(inv_desc->iec.granularity,
>                              inv_desc->iec.index,
>                              inv_desc->iec.index_mask);
> --
> 2.34.1
>

Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue

[Yi Liu]

On 2024/11/5 14:12, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Tuesday, November 5, 2024 1:05 PM
>> Subject: Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit
>> invalidation queue
>>
>> On 2024/11/4 20:55, Zhenzhong Duan wrote:
>>> According to VTD spec, a 256-bit descriptor will result in an invalid
>>> descriptor error if submitted in an IQ that is setup to provide hardware
>>> with 128-bit descriptors (IQA_REG.DW=0). Meanwhile, there are old inv desc
>>> types (e.g. iotlb_inv_desc) that can be either 128bits or 256bits. If a
>>> 128-bit version of this descriptor is submitted into an IQ that is setup
>>> to provide hardware with 256-bit descriptors will also result in an invalid
>>> descriptor error.
>>>
>>> The 2nd will be captured by the tail register update. So we only need to
>>> focus on the 1st.
>>>
>>> Because the reserved bit check between different types of invalidation desc
>>> are common, so introduce a common function vtd_inv_desc_reserved_check()
>>> to do all the checks and pass the differences as parameters.
>>>
>>> With this change, need to replace error_report_once() call with error_report()
>>> to catch different call sites. This isn't an issue as error_report_once()
>>> here is mainly used to help debug guest error, but it only dumps once in
>>> qemu life cycle and doesn't help much, we need error_report() instead.
>>>
>>> Fixes: c0c1d351849b ("intel_iommu: add 256 bits qi_desc support")
>>> Suggested-by: Yi Liu <yi.l.liu@intel.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>    hw/i386/intel_iommu_internal.h |  1 +
>>>    hw/i386/intel_iommu.c          | 80 ++++++++++++++++++++++++----------
>>>    2 files changed, 59 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
>>> index 2f9bc0147d..75ccd501b0 100644
>>> --- a/hw/i386/intel_iommu_internal.h
>>> +++ b/hw/i386/intel_iommu_internal.h
>>> @@ -356,6 +356,7 @@ union VTDInvDesc {
>>>    typedef union VTDInvDesc VTDInvDesc;
>>>
>>>    /* Masks for struct VTDInvDesc */
>>> +#define VTD_INV_DESC_ALL_ONE            -1ULL
>>>    #define VTD_INV_DESC_TYPE(val)          ((((val) >> 5) & 0x70ULL) | \
>>>                                             ((val) & 0xfULL))
>>>    #define VTD_INV_DESC_CC                 0x1 /* Context-cache Invalidate Desc */
>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>> index 1ecfe47963..2fc3866433 100644
>>> --- a/hw/i386/intel_iommu.c
>>> +++ b/hw/i386/intel_iommu.c
>>> @@ -2532,15 +2532,51 @@ static bool vtd_get_inv_desc(IntelIOMMUState *s,
>>>        return true;
>>>    }
>>>
>>> +static bool vtd_inv_desc_reserved_check(IntelIOMMUState *s,
>>> +                                        VTDInvDesc *inv_desc,
>>> +                                        uint64_t mask[4], bool dw,
>>> +                                        const char *func_name,
>>> +                                        const char *desc_type)
>>> +{
>>> +    if (s->iq_dw) {
>>> +        if (inv_desc->val[0] & mask[0] || inv_desc->val[1] & mask[1] ||
>>> +            inv_desc->val[2] & mask[2] || inv_desc->val[3] & mask[3]) {
>>> +            error_report("%s: invalid %s desc val[3]: 0x%"PRIx64
>>> +                         " val[2]: 0x%"PRIx64" val[1]=0x%"PRIx64
>>> +                         " val[0]=0x%"PRIx64" (reserved nonzero)",
>>> +                         func_name, desc_type, inv_desc->val[3],
>>> +                         inv_desc->val[2], inv_desc->val[1],
>>> +                         inv_desc->val[0]);
>>> +            return false;
>>> +        }
>>> +    } else {
>>> +        if (dw) {
>>> +            error_report("%s: 256-bit %s desc in 128-bit invalidation queue",
>>> +                         func_name, desc_type);
>>> +            return false;
>>> +        }
>>> +
>>
>> If a respin is made, I'd prefer to move this check out of this helper since
>> it's not about reserved bit check. Another reason is you cannot find a good
>> naming for the @dw parameter. It's confusing as s->iq_dw is checked as
>> well. So put this check out of this helper may be better.
> 
> I see, @dw hints inv desc size, s->iq_dw hints the inv queue element size.
> Moving that check out will produce duplicate code for VTD_INV_DESC_PC,
> VTD_INV_DESC_PIOTLB and VTD_INV_DESC_DEV_PIOTLB handlers.
> Maybe s/ vtd_inv_desc_reserved_check/ vtd_inv_desc_sanity_check?

in that case, renaming @dw to something different would be better. At
the first glance, I was wondering if anything wrong here since dw is
checked twice.. Perhaps 'p_inv_type' as all the 256bits types are for
pasid related. Add a description for this helper would be nice as well.
This can document what each parameter means.

-- 
Regards,
Yi Liu

Re: [PATCH 1/3] intel_iommu: Send IQE event when setting reserved bit in IQT_TAIL

[Yi Liu]

On 2024/11/4 20:55, Zhenzhong Duan wrote:
> According to VTD spec, Figure 11-22, Invalidation Queue Tail Register,
> "When Descriptor Width (DW) field in Invalidation Queue Address Register
> (IQA_REG) is Set (256-bit descriptors), hardware treats bit-4 as reserved
> and a value of 1 in the bit will result in invalidation queue error."
> 
> Current code missed to send IQE event to guest, fix it.

a nit: mention the spec revision as well since the Figure number may be
modified.

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

> 
> Fixes: c0c1d351849b ("intel_iommu: add 256 bits qi_desc support")
> Suggested-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/i386/intel_iommu.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 8612d0917b..1ecfe47963 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2847,6 +2847,7 @@ static void vtd_handle_iqt_write(IntelIOMMUState *s)
>       if (s->iq_dw && (val & VTD_IQT_QT_256_RSV_BIT)) {
>           error_report_once("%s: RSV bit is set: val=0x%"PRIx64,
>                             __func__, val);
> +        vtd_handle_inv_queue_error(s);
>           return;
>       }
>       s->iq_tail = VTD_IQT_QT(s->iq_dw, val);

-- 
Regards,
Yi Liu

Re: [PATCH 3/3] intel_iommu: Add missed reserved bit check for IEC descriptor

[Yi Liu]

On 2024/11/4 20:55, Zhenzhong Duan wrote:
> IEC descriptor is 128-bit invalidation descriptor, must be padded with
> 128-bits of 0s in the upper bytes to create a 256-bit descriptor when
> the invalidation queue is configured for 256-bit descriptors (IQA_REG.DW=1).
> 
> Fixes: 02a2cbc872df ("x86-iommu: introduce IEC notifiers")
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/i386/intel_iommu_internal.h | 3 +++
>   hw/i386/intel_iommu.c          | 8 ++++++++
>   2 files changed, 11 insertions(+)

It might be updated if patch 02 of this series has been respined. But this
patch is already in good shape.

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 75ccd501b0..4323fc5d6d 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -410,6 +410,9 @@ typedef union VTDInvDesc VTDInvDesc;
>   #define VTD_INV_DESC_DEVICE_IOTLB_RSVD_HI 0xffeULL
>   #define VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO 0xffff0000ffe0f1f0
>   
> +/* Masks for Interrupt Entry Invalidate Descriptor */
> +#define VTD_INV_DESC_IEC_RSVD           0xffff000007fff1e0ULL
> +
>   /* Rsvd field masks for spte */
>   #define VTD_SPTE_SNP 0x800ULL
>   
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 2fc3866433..4c0d1d7d47 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2692,6 +2692,14 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
>   static bool vtd_process_inv_iec_desc(IntelIOMMUState *s,
>                                        VTDInvDesc *inv_desc)
>   {
> +    uint64_t mask[4] = {VTD_INV_DESC_IEC_RSVD, VTD_INV_DESC_ALL_ONE,
> +                        VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};
> +
> +    if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,
> +                                     __func__, "iec inv")) {
> +        return false;
> +    }
> +
>       trace_vtd_inv_desc_iec(inv_desc->iec.granularity,
>                              inv_desc->iec.index,
>                              inv_desc->iec.index_mask);

-- 
Regards,
Yi Liu

RE: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue

[Duan, Zhenzhong]

>-----Original Message-----
>From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>Sent: Tuesday, November 5, 2024 2:36 PM
>Subject: Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit
>invalidation queue
>
>I saw the pull request, just a few questions/comments in case there is a
>new spin.
>These are not hard requirements, the current version looks good as well.
>
>On 04/11/2024 13:55, Zhenzhong Duan wrote:
>> Caution: External email. Do not open attachments or click links, unless this
>email comes from a known sender and you know the content is safe.
>>
>>
>> According to VTD spec, a 256-bit descriptor will result in an invalid
>> descriptor error if submitted in an IQ that is setup to provide hardware
>> with 128-bit descriptors (IQA_REG.DW=0). Meanwhile, there are old inv desc
>> types (e.g. iotlb_inv_desc) that can be either 128bits or 256bits. If a
>> 128-bit version of this descriptor is submitted into an IQ that is setup
>> to provide hardware with 256-bit descriptors will also result in an invalid
>> descriptor error.
>>
>> The 2nd will be captured by the tail register update. So we only need to
>> focus on the 1st.
>>
>> Because the reserved bit check between different types of invalidation desc
>> are common, so introduce a common function vtd_inv_desc_reserved_check()
>> to do all the checks and pass the differences as parameters.
>>
>> With this change, need to replace error_report_once() call with error_report()
>> to catch different call sites. This isn't an issue as error_report_once()
>> here is mainly used to help debug guest error, but it only dumps once in
>> qemu life cycle and doesn't help much, we need error_report() instead.
>>
>> Fixes: c0c1d351849b ("intel_iommu: add 256 bits qi_desc support")
>> Suggested-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   hw/i386/intel_iommu_internal.h |  1 +
>>   hw/i386/intel_iommu.c          | 80 ++++++++++++++++++++++++----------
>>   2 files changed, 59 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
>> index 2f9bc0147d..75ccd501b0 100644
>> --- a/hw/i386/intel_iommu_internal.h
>> +++ b/hw/i386/intel_iommu_internal.h
>> @@ -356,6 +356,7 @@ union VTDInvDesc {
>>   typedef union VTDInvDesc VTDInvDesc;
>>
>>   /* Masks for struct VTDInvDesc */
>> +#define VTD_INV_DESC_ALL_ONE            -1ULL
>
>s/one/ones
>And maybe ~0ull is better. It's up to you

OK, will do if respin.

>
>>   #define VTD_INV_DESC_TYPE(val)          ((((val) >> 5) & 0x70ULL) | \
>>                                            ((val) & 0xfULL))
>>   #define VTD_INV_DESC_CC                 0x1 /* Context-cache Invalidate Desc */
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 1ecfe47963..2fc3866433 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -2532,15 +2532,51 @@ static bool vtd_get_inv_desc(IntelIOMMUState *s,
>>       return true;
>>   }
>>
>> +static bool vtd_inv_desc_reserved_check(IntelIOMMUState *s,
>> +                                        VTDInvDesc *inv_desc,
>> +                                        uint64_t mask[4], bool dw,
>> +                                        const char *func_name,
>> +                                        const char *desc_type)
>> +{
>> +    if (s->iq_dw) {
>> +        if (inv_desc->val[0] & mask[0] || inv_desc->val[1] & mask[1] ||
>> +            inv_desc->val[2] & mask[2] || inv_desc->val[3] & mask[3]) {
>> +            error_report("%s: invalid %s desc val[3]: 0x%"PRIx64
>> +                         " val[2]: 0x%"PRIx64" val[1]=0x%"PRIx64
>> +                         " val[0]=0x%"PRIx64" (reserved nonzero)",
>> +                         func_name, desc_type, inv_desc->val[3],
>> +                         inv_desc->val[2], inv_desc->val[1],
>> +                         inv_desc->val[0]);
>> +            return false;
>> +        }
>> +    } else {
>> +        if (dw) {
>> +            error_report("%s: 256-bit %s desc in 128-bit invalidation queue",
>> +                         func_name, desc_type);
>> +            return false;
>> +        }
>> +
>> +        if (inv_desc->lo & mask[0] || inv_desc->hi & mask[1]) {
>> +            error_report("%s: invalid %s desc: hi=%"PRIx64", lo=%"PRIx64
>> +                         " (reserved nonzero)", func_name, desc_type,
>> +                         inv_desc->hi, inv_desc->lo);
>> +            return false;
>> +        }
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>   static bool vtd_process_wait_desc(IntelIOMMUState *s, VTDInvDesc
>*inv_desc)
>>   {
>> -    if ((inv_desc->hi & VTD_INV_DESC_WAIT_RSVD_HI) ||
>> -        (inv_desc->lo & VTD_INV_DESC_WAIT_RSVD_LO)) {
>> -        error_report_once("%s: invalid wait desc: hi=%"PRIx64", lo=%"PRIx64
>> -                          " (reserved nonzero)", __func__, inv_desc->hi,
>> -                          inv_desc->lo);
>> +    uint64_t mask[4] = {VTD_INV_DESC_WAIT_RSVD_LO,
>VTD_INV_DESC_WAIT_RSVD_HI,
>> +                        VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};
>
>Why don't we declare the full masks outside of the functions (called
>something like ..._DW_MASK)?

Do you mean moving mask[4] out as a static array?
Is ..._DW_MASK the array name?

>
>> +
>> +    if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,
>
>Maybe the dw argument should be declared using #define in the internal
>header.

I see, maybe define ..._256_BIT and ..._128_BIT.
But a bool is enough for the purpose, we just want to know if it's 256 bit desc.

Thanks
Zhenzhong

RE: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue

[Duan, Zhenzhong]

>-----Original Message-----
>From: Liu, Yi L <yi.l.liu@intel.com>
>Sent: Tuesday, November 5, 2024 2:50 PM
>Subject: Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit
>invalidation queue
>
>On 2024/11/5 14:12, Duan, Zhenzhong wrote:
>>
>>
>>> -----Original Message-----
>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>> Sent: Tuesday, November 5, 2024 1:05 PM
>>> Subject: Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit
>>> invalidation queue
>>>
>>> On 2024/11/4 20:55, Zhenzhong Duan wrote:
>>>> According to VTD spec, a 256-bit descriptor will result in an invalid
>>>> descriptor error if submitted in an IQ that is setup to provide hardware
>>>> with 128-bit descriptors (IQA_REG.DW=0). Meanwhile, there are old inv desc
>>>> types (e.g. iotlb_inv_desc) that can be either 128bits or 256bits. If a
>>>> 128-bit version of this descriptor is submitted into an IQ that is setup
>>>> to provide hardware with 256-bit descriptors will also result in an invalid
>>>> descriptor error.
>>>>
>>>> The 2nd will be captured by the tail register update. So we only need to
>>>> focus on the 1st.
>>>>
>>>> Because the reserved bit check between different types of invalidation desc
>>>> are common, so introduce a common function
>vtd_inv_desc_reserved_check()
>>>> to do all the checks and pass the differences as parameters.
>>>>
>>>> With this change, need to replace error_report_once() call with error_report()
>>>> to catch different call sites. This isn't an issue as error_report_once()
>>>> here is mainly used to help debug guest error, but it only dumps once in
>>>> qemu life cycle and doesn't help much, we need error_report() instead.
>>>>
>>>> Fixes: c0c1d351849b ("intel_iommu: add 256 bits qi_desc support")
>>>> Suggested-by: Yi Liu <yi.l.liu@intel.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>> ---
>>>>    hw/i386/intel_iommu_internal.h |  1 +
>>>>    hw/i386/intel_iommu.c          | 80 ++++++++++++++++++++++++----------
>>>>    2 files changed, 59 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/hw/i386/intel_iommu_internal.h
>b/hw/i386/intel_iommu_internal.h
>>>> index 2f9bc0147d..75ccd501b0 100644
>>>> --- a/hw/i386/intel_iommu_internal.h
>>>> +++ b/hw/i386/intel_iommu_internal.h
>>>> @@ -356,6 +356,7 @@ union VTDInvDesc {
>>>>    typedef union VTDInvDesc VTDInvDesc;
>>>>
>>>>    /* Masks for struct VTDInvDesc */
>>>> +#define VTD_INV_DESC_ALL_ONE            -1ULL
>>>>    #define VTD_INV_DESC_TYPE(val)          ((((val) >> 5) & 0x70ULL) | \
>>>>                                             ((val) & 0xfULL))
>>>>    #define VTD_INV_DESC_CC                 0x1 /* Context-cache Invalidate Desc
>*/
>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>> index 1ecfe47963..2fc3866433 100644
>>>> --- a/hw/i386/intel_iommu.c
>>>> +++ b/hw/i386/intel_iommu.c
>>>> @@ -2532,15 +2532,51 @@ static bool vtd_get_inv_desc(IntelIOMMUState
>*s,
>>>>        return true;
>>>>    }
>>>>
>>>> +static bool vtd_inv_desc_reserved_check(IntelIOMMUState *s,
>>>> +                                        VTDInvDesc *inv_desc,
>>>> +                                        uint64_t mask[4], bool dw,
>>>> +                                        const char *func_name,
>>>> +                                        const char *desc_type)
>>>> +{
>>>> +    if (s->iq_dw) {
>>>> +        if (inv_desc->val[0] & mask[0] || inv_desc->val[1] & mask[1] ||
>>>> +            inv_desc->val[2] & mask[2] || inv_desc->val[3] & mask[3]) {
>>>> +            error_report("%s: invalid %s desc val[3]: 0x%"PRIx64
>>>> +                         " val[2]: 0x%"PRIx64" val[1]=0x%"PRIx64
>>>> +                         " val[0]=0x%"PRIx64" (reserved nonzero)",
>>>> +                         func_name, desc_type, inv_desc->val[3],
>>>> +                         inv_desc->val[2], inv_desc->val[1],
>>>> +                         inv_desc->val[0]);
>>>> +            return false;
>>>> +        }
>>>> +    } else {
>>>> +        if (dw) {
>>>> +            error_report("%s: 256-bit %s desc in 128-bit invalidation queue",
>>>> +                         func_name, desc_type);
>>>> +            return false;
>>>> +        }
>>>> +
>>>
>>> If a respin is made, I'd prefer to move this check out of this helper since
>>> it's not about reserved bit check. Another reason is you cannot find a good
>>> naming for the @dw parameter. It's confusing as s->iq_dw is checked as
>>> well. So put this check out of this helper may be better.
>>
>> I see, @dw hints inv desc size, s->iq_dw hints the inv queue element size.
>> Moving that check out will produce duplicate code for VTD_INV_DESC_PC,
>> VTD_INV_DESC_PIOTLB and VTD_INV_DESC_DEV_PIOTLB handlers.
>> Maybe s/ vtd_inv_desc_reserved_check/ vtd_inv_desc_sanity_check?
>
>in that case, renaming @dw to something different would be better. At
>the first glance, I was wondering if anything wrong here since dw is
>checked twice.. Perhaps 'p_inv_type' as all the 256bits types are for
>pasid related. Add a description for this helper would be nice as well.
>This can document what each parameter means.

Yes, good suggestions.

Thanks
Zhenzhong

Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue

[CLEMENT MATHIEU--DRIF]

On 05/11/2024 08:38, Duan, Zhenzhong wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
>> -----Original Message-----
>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>> Sent: Tuesday, November 5, 2024 2:36 PM
>> Subject: Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit
>> invalidation queue
>>
>> I saw the pull request, just a few questions/comments in case there is a
>> new spin.
>> These are not hard requirements, the current version looks good as well.
>>
>> On 04/11/2024 13:55, Zhenzhong Duan wrote:
>>> Caution: External email. Do not open attachments or click links, unless this
>> email comes from a known sender and you know the content is safe.
>>>
>>> According to VTD spec, a 256-bit descriptor will result in an invalid
>>> descriptor error if submitted in an IQ that is setup to provide hardware
>>> with 128-bit descriptors (IQA_REG.DW=0). Meanwhile, there are old inv desc
>>> types (e.g. iotlb_inv_desc) that can be either 128bits or 256bits. If a
>>> 128-bit version of this descriptor is submitted into an IQ that is setup
>>> to provide hardware with 256-bit descriptors will also result in an invalid
>>> descriptor error.
>>>
>>> The 2nd will be captured by the tail register update. So we only need to
>>> focus on the 1st.
>>>
>>> Because the reserved bit check between different types of invalidation desc
>>> are common, so introduce a common function vtd_inv_desc_reserved_check()
>>> to do all the checks and pass the differences as parameters.
>>>
>>> With this change, need to replace error_report_once() call with error_report()
>>> to catch different call sites. This isn't an issue as error_report_once()
>>> here is mainly used to help debug guest error, but it only dumps once in
>>> qemu life cycle and doesn't help much, we need error_report() instead.
>>>
>>> Fixes: c0c1d351849b ("intel_iommu: add 256 bits qi_desc support")
>>> Suggested-by: Yi Liu <yi.l.liu@intel.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>    hw/i386/intel_iommu_internal.h |  1 +
>>>    hw/i386/intel_iommu.c          | 80 ++++++++++++++++++++++++----------
>>>    2 files changed, 59 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
>>> index 2f9bc0147d..75ccd501b0 100644
>>> --- a/hw/i386/intel_iommu_internal.h
>>> +++ b/hw/i386/intel_iommu_internal.h
>>> @@ -356,6 +356,7 @@ union VTDInvDesc {
>>>    typedef union VTDInvDesc VTDInvDesc;
>>>
>>>    /* Masks for struct VTDInvDesc */
>>> +#define VTD_INV_DESC_ALL_ONE            -1ULL
>> s/one/ones
>> And maybe ~0ull is better. It's up to you
> OK, will do if respin.
>
>>>    #define VTD_INV_DESC_TYPE(val)          ((((val) >> 5) & 0x70ULL) | \
>>>                                             ((val) & 0xfULL))
>>>    #define VTD_INV_DESC_CC                 0x1 /* Context-cache Invalidate Desc */
>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>> index 1ecfe47963..2fc3866433 100644
>>> --- a/hw/i386/intel_iommu.c
>>> +++ b/hw/i386/intel_iommu.c
>>> @@ -2532,15 +2532,51 @@ static bool vtd_get_inv_desc(IntelIOMMUState *s,
>>>        return true;
>>>    }
>>>
>>> +static bool vtd_inv_desc_reserved_check(IntelIOMMUState *s,
>>> +                                        VTDInvDesc *inv_desc,
>>> +                                        uint64_t mask[4], bool dw,
>>> +                                        const char *func_name,
>>> +                                        const char *desc_type)
>>> +{
>>> +    if (s->iq_dw) {
>>> +        if (inv_desc->val[0] & mask[0] || inv_desc->val[1] & mask[1] ||
>>> +            inv_desc->val[2] & mask[2] || inv_desc->val[3] & mask[3]) {
>>> +            error_report("%s: invalid %s desc val[3]: 0x%"PRIx64
>>> +                         " val[2]: 0x%"PRIx64" val[1]=0x%"PRIx64
>>> +                         " val[0]=0x%"PRIx64" (reserved nonzero)",
>>> +                         func_name, desc_type, inv_desc->val[3],
>>> +                         inv_desc->val[2], inv_desc->val[1],
>>> +                         inv_desc->val[0]);
>>> +            return false;
>>> +        }
>>> +    } else {
>>> +        if (dw) {
>>> +            error_report("%s: 256-bit %s desc in 128-bit invalidation queue",
>>> +                         func_name, desc_type);
>>> +            return false;
>>> +        }
>>> +
>>> +        if (inv_desc->lo & mask[0] || inv_desc->hi & mask[1]) {
>>> +            error_report("%s: invalid %s desc: hi=%"PRIx64", lo=%"PRIx64
>>> +                         " (reserved nonzero)", func_name, desc_type,
>>> +                         inv_desc->hi, inv_desc->lo);
>>> +            return false;
>>> +        }
>>> +    }
>>> +
>>> +    return true;
>>> +}
>>> +
>>>    static bool vtd_process_wait_desc(IntelIOMMUState *s, VTDInvDesc
>> *inv_desc)
>>>    {
>>> -    if ((inv_desc->hi & VTD_INV_DESC_WAIT_RSVD_HI) ||
>>> -        (inv_desc->lo & VTD_INV_DESC_WAIT_RSVD_LO)) {
>>> -        error_report_once("%s: invalid wait desc: hi=%"PRIx64", lo=%"PRIx64
>>> -                          " (reserved nonzero)", __func__, inv_desc->hi,
>>> -                          inv_desc->lo);
>>> +    uint64_t mask[4] = {VTD_INV_DESC_WAIT_RSVD_LO,
>> VTD_INV_DESC_WAIT_RSVD_HI,
>>> +                        VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};
>> Why don't we declare the full masks outside of the functions (called
>> something like ..._DW_MASK)?
> Do you mean moving mask[4] out as a static array?
exactly
> Is ..._DW_MASK the array name?

yes, for instance

>
>>> +
>>> +    if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,
>> Maybe the dw argument should be declared using #define in the internal
>> header.
> I see, maybe define ..._256_BIT and ..._128_BIT.
> But a bool is enough for the purpose, we just want to know if it's 256 bit desc.

Yes, the purpose is to make the callsite more readable by adding 
semantic to the arguments

>
> Thanks
> Zhenzhong