* Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue
2024-11-04 12:55 ` [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue Zhenzhong Duan
@ 2024-11-04 14:46 ` Michael S. Tsirkin
2024-11-05 2:40 ` Duan, Zhenzhong
2024-11-05 5:05 ` Yi Liu
2024-11-05 6:36 ` CLEMENT MATHIEU--DRIF
2 siblings, 1 reply; 17+ messages in thread
From: Michael S. Tsirkin @ 2024-11-04 14:46 UTC (permalink / raw)
To: Zhenzhong Duan
Cc: qemu-devel, jasowang, yi.l.liu, clement.mathieu--drif,
chao.p.peng, Marcel Apfelbaum, Paolo Bonzini, Richard Henderson,
Eduardo Habkost
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
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue
2024-11-04 14:46 ` Michael S. Tsirkin
@ 2024-11-05 2:40 ` Duan, Zhenzhong
0 siblings, 0 replies; 17+ messages in thread
From: Duan, Zhenzhong @ 2024-11-05 2:40 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel@nongnu.org, jasowang@redhat.com, Liu, Yi L,
clement.mathieu--drif@eviden.com, Peng, Chao P, Marcel Apfelbaum,
Paolo Bonzini, Richard Henderson, Eduardo Habkost
>-----Original Message-----
>From: Michael S. Tsirkin <mst@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.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, ...)
>
>
>?
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue
2024-11-04 12:55 ` [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue Zhenzhong Duan
2024-11-04 14:46 ` Michael S. Tsirkin
@ 2024-11-05 5:05 ` Yi Liu
2024-11-05 6:12 ` Duan, Zhenzhong
2024-11-05 6:36 ` CLEMENT MATHIEU--DRIF
2 siblings, 1 reply; 17+ messages in thread
From: Yi Liu @ 2024-11-05 5:05 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel
Cc: mst, jasowang, clement.mathieu--drif, chao.p.peng,
Marcel Apfelbaum, Paolo Bonzini, Richard Henderson,
Eduardo Habkost
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
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue
2024-11-05 5:05 ` Yi Liu
@ 2024-11-05 6:12 ` Duan, Zhenzhong
2024-11-05 6:50 ` Yi Liu
0 siblings, 1 reply; 17+ messages in thread
From: Duan, Zhenzhong @ 2024-11-05 6:12 UTC (permalink / raw)
To: Liu, Yi L, qemu-devel@nongnu.org
Cc: mst@redhat.com, jasowang@redhat.com,
clement.mathieu--drif@eviden.com, Peng, Chao P, Marcel Apfelbaum,
Paolo Bonzini, Richard Henderson, Eduardo Habkost
>-----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?
Michael, let me know if it's viable to send a v2 after pull request.
Thanks
Zhenzhong
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue
2024-11-05 6:12 ` Duan, Zhenzhong
@ 2024-11-05 6:50 ` Yi Liu
2024-11-05 7:43 ` Duan, Zhenzhong
0 siblings, 1 reply; 17+ messages in thread
From: Yi Liu @ 2024-11-05 6:50 UTC (permalink / raw)
To: Duan, Zhenzhong, qemu-devel@nongnu.org
Cc: mst@redhat.com, jasowang@redhat.com,
clement.mathieu--drif@eviden.com, Peng, Chao P, Marcel Apfelbaum,
Paolo Bonzini, Richard Henderson, Eduardo Habkost
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.
--
Regards,
Yi Liu
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue
2024-11-05 6:50 ` Yi Liu
@ 2024-11-05 7:43 ` Duan, Zhenzhong
0 siblings, 0 replies; 17+ messages in thread
From: Duan, Zhenzhong @ 2024-11-05 7:43 UTC (permalink / raw)
To: Liu, Yi L, qemu-devel@nongnu.org
Cc: mst@redhat.com, jasowang@redhat.com,
clement.mathieu--drif@eviden.com, Peng, Chao P, Marcel Apfelbaum,
Paolo Bonzini, Richard Henderson, Eduardo Habkost
>-----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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue
2024-11-04 12:55 ` [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue Zhenzhong Duan
2024-11-04 14:46 ` Michael S. Tsirkin
2024-11-05 5:05 ` Yi Liu
@ 2024-11-05 6:36 ` CLEMENT MATHIEU--DRIF
2024-11-05 7:38 ` Duan, Zhenzhong
2 siblings, 1 reply; 17+ messages in thread
From: CLEMENT MATHIEU--DRIF @ 2024-11-05 6:36 UTC (permalink / raw)
To: Zhenzhong Duan, qemu-devel@nongnu.org
Cc: mst@redhat.com, jasowang@redhat.com, yi.l.liu@intel.com,
chao.p.peng@intel.com, Marcel Apfelbaum, Paolo Bonzini,
Richard Henderson, Eduardo Habkost
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
> #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)?
> +
> + if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,
Maybe the dw argument should be declared using #define in the internal
header.
> + __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
>
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue
2024-11-05 6:36 ` CLEMENT MATHIEU--DRIF
@ 2024-11-05 7:38 ` Duan, Zhenzhong
2024-11-05 8:03 ` CLEMENT MATHIEU--DRIF
0 siblings, 1 reply; 17+ messages in thread
From: Duan, Zhenzhong @ 2024-11-05 7:38 UTC (permalink / raw)
To: CLEMENT MATHIEU--DRIF, qemu-devel@nongnu.org
Cc: mst@redhat.com, jasowang@redhat.com, Liu, Yi L, Peng, Chao P,
Marcel Apfelbaum, Paolo Bonzini, Richard Henderson,
Eduardo Habkost
>-----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?
Is ..._DW_MASK the array name?
>
>> +
>> + 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.
Thanks
Zhenzhong
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/3] intel_iommu: Add missed sanity check for 256-bit invalidation queue
2024-11-05 7:38 ` Duan, Zhenzhong
@ 2024-11-05 8:03 ` CLEMENT MATHIEU--DRIF
0 siblings, 0 replies; 17+ messages in thread
From: CLEMENT MATHIEU--DRIF @ 2024-11-05 8:03 UTC (permalink / raw)
To: Duan, Zhenzhong, qemu-devel@nongnu.org
Cc: mst@redhat.com, jasowang@redhat.com, Liu, Yi L, Peng, Chao P,
Marcel Apfelbaum, Paolo Bonzini, Richard Henderson,
Eduardo Habkost
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
^ permalink raw reply [flat|nested] 17+ messages in thread