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

[Yi Liu <yi.l.liu@intel.com>]

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