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....@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
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
>
