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

["Michael S. Tsirkin" <mst@redhat.com>]

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