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