>-----Original Message-----
>From: Michael S. Tsirkin <m...@redhat.com>
>Subject: Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit
>invalidation queue
>
>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....@intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@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, ...)
>
>
>?
Yes, make sense. I see you have sent pull request, not clear
if error_report() is reluctantly ok for you or I should send a fix.
Thanks
Zhenzhong
