qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Yi Liu
Subject: Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue
Date: Tue, 5 Nov 2024 14:50:14 +0800
User-agent: Mozilla Thunderbird

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]