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