[PATCH v2 0/2] intel_iommu minor fixes

[PATCH v2 0/2] intel_iommu minor fixes

[Zhenzhong Duan]

Hi

Fixes two minor issues in intel iommu.
See patch for details.

Tested scalable mode and legacy mode with vfio device passthrough: PASS
Tested intel-iommu.flat in kvm-unit-test: PASS

Thanks
Zhenzhong

Zhenzhong Duan (2):
  intel_iommu: Fix invalidation descriptor type field
  intel_iommu: Make PASID-cache and PIOTLB type invalid in legacy mode

 hw/i386/intel_iommu_internal.h | 11 ++++++-----
 hw/i386/intel_iommu.c          | 24 ++++++++++++------------
 2 files changed, 18 insertions(+), 17 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] intel_iommu: Fix invalidation descriptor type field

[Zhenzhong Duan]

According to spec, invalidation descriptor type is 7bits which is
concatenation of bits[11:9] and bits[3:0] of invalidation descriptor.

Currently we only pick bits[3:0] as the invalidation type and treat
bits[11:9] as reserved zero. This is not a problem for now as bits[11:9]
is zero for all current invalidation types. But it will break if newer
type occupies bits[11:9].

Fix it by taking bits[11:9] into type and make reserved bits check accurate.

Suggested-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Yi Liu <yi.l.liu@intel.com>
Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com>
---
 hw/i386/intel_iommu_internal.h | 11 ++++++-----
 hw/i386/intel_iommu.c          |  2 +-
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 5f32c36943..13d5d129ae 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -356,7 +356,8 @@ union VTDInvDesc {
 typedef union VTDInvDesc VTDInvDesc;
 
 /* Masks for struct VTDInvDesc */
-#define VTD_INV_DESC_TYPE               0xf
+#define VTD_INV_DESC_TYPE(val)          ((((val) >> 5) & 0x70ULL) | \
+                                         ((val) & 0xfULL))
 #define VTD_INV_DESC_CC                 0x1 /* Context-cache Invalidate Desc */
 #define VTD_INV_DESC_IOTLB              0x2
 #define VTD_INV_DESC_DEVICE             0x3
@@ -372,7 +373,7 @@ typedef union VTDInvDesc VTDInvDesc;
 #define VTD_INV_DESC_WAIT_IF            (1ULL << 4)
 #define VTD_INV_DESC_WAIT_FN            (1ULL << 6)
 #define VTD_INV_DESC_WAIT_DATA_SHIFT    32
-#define VTD_INV_DESC_WAIT_RSVD_LO       0Xffffff80ULL
+#define VTD_INV_DESC_WAIT_RSVD_LO       0Xfffff180ULL
 #define VTD_INV_DESC_WAIT_RSVD_HI       3ULL
 
 /* Masks for Context-cache Invalidation Descriptor */
@@ -383,7 +384,7 @@ typedef union VTDInvDesc VTDInvDesc;
 #define VTD_INV_DESC_CC_DID(val)        (((val) >> 16) & VTD_DOMAIN_ID_MASK)
 #define VTD_INV_DESC_CC_SID(val)        (((val) >> 32) & 0xffffUL)
 #define VTD_INV_DESC_CC_FM(val)         (((val) >> 48) & 3UL)
-#define VTD_INV_DESC_CC_RSVD            0xfffc00000000ffc0ULL
+#define VTD_INV_DESC_CC_RSVD            0xfffc00000000f1c0ULL
 
 /* Masks for IOTLB Invalidate Descriptor */
 #define VTD_INV_DESC_IOTLB_G            (3ULL << 4)
@@ -393,7 +394,7 @@ typedef union VTDInvDesc VTDInvDesc;
 #define VTD_INV_DESC_IOTLB_DID(val)     (((val) >> 16) & VTD_DOMAIN_ID_MASK)
 #define VTD_INV_DESC_IOTLB_ADDR(val)    ((val) & ~0xfffULL)
 #define VTD_INV_DESC_IOTLB_AM(val)      ((val) & 0x3fULL)
-#define VTD_INV_DESC_IOTLB_RSVD_LO      0xffffffff0000ff00ULL
+#define VTD_INV_DESC_IOTLB_RSVD_LO      0xffffffff0000f100ULL
 #define VTD_INV_DESC_IOTLB_RSVD_HI      0xf80ULL
 #define VTD_INV_DESC_IOTLB_PASID_PASID  (2ULL << 4)
 #define VTD_INV_DESC_IOTLB_PASID_PAGE   (3ULL << 4)
@@ -406,7 +407,7 @@ typedef union VTDInvDesc VTDInvDesc;
 #define VTD_INV_DESC_DEVICE_IOTLB_SIZE(val) ((val) & 0x1)
 #define VTD_INV_DESC_DEVICE_IOTLB_SID(val) (((val) >> 32) & 0xFFFFULL)
 #define VTD_INV_DESC_DEVICE_IOTLB_RSVD_HI 0xffeULL
-#define VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO 0xffff0000ffe0fff8
+#define VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO 0xffff0000ffe0f1f0
 
 /* Rsvd field masks for spte */
 #define VTD_SPTE_SNP 0x800ULL
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 16d2885fcc..68cb72a481 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2744,7 +2744,7 @@ static bool vtd_process_inv_desc(IntelIOMMUState *s)
         return false;
     }
 
-    desc_type = inv_desc.lo & VTD_INV_DESC_TYPE;
+    desc_type = VTD_INV_DESC_TYPE(inv_desc.lo);
     /* FIXME: should update at first or at last? */
     s->iq_last_desc_type = desc_type;
 
-- 
2.34.1

[PATCH v2 2/2] intel_iommu: Make PASID-cache and PIOTLB type invalid in legacy mode

[Zhenzhong Duan]

In vtd_process_inv_desc(), VTD_INV_DESC_PC and VTD_INV_DESC_PIOTLB are
bypassed without scalable mode check. These two types are not valid
in legacy mode and we should report error.

Suggested-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/i386/intel_iommu.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 68cb72a481..90cd4e5044 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2763,17 +2763,6 @@ static bool vtd_process_inv_desc(IntelIOMMUState *s)
         }
         break;
 
-    /*
-     * TODO: the entity of below two cases will be implemented in future series.
-     * To make guest (which integrates scalable mode support patch set in
-     * iommu driver) work, just return true is enough so far.
-     */
-    case VTD_INV_DESC_PC:
-        break;
-
-    case VTD_INV_DESC_PIOTLB:
-        break;
-
     case VTD_INV_DESC_WAIT:
         trace_vtd_inv_desc("wait", inv_desc.hi, inv_desc.lo);
         if (!vtd_process_wait_desc(s, &inv_desc)) {
@@ -2795,6 +2784,17 @@ static bool vtd_process_inv_desc(IntelIOMMUState *s)
         }
         break;
 
+    /*
+     * TODO: the entity of below two cases will be implemented in future series.
+     * To make guest (which integrates scalable mode support patch set in
+     * iommu driver) work, just return true is enough so far.
+     */
+    case VTD_INV_DESC_PC:
+    case VTD_INV_DESC_PIOTLB:
+        if (s->scalable_mode) {
+            break;
+        }
+    /* fallthrough */
     default:
         error_report_once("%s: invalid inv desc: hi=%"PRIx64", lo=%"PRIx64
                           " (unknown type)", __func__, inv_desc.hi,
-- 
2.34.1

Re: [PATCH v2 2/2] intel_iommu: Make PASID-cache and PIOTLB type invalid in legacy mode

[CLEMENT MATHIEU--DRIF]

Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com>

Super reactive!

Maybe we can continue along this path after the handlers are implemented.
It would be great to make sure we don't process PASID related descriptors when not in scalable mode.
What are your thoughts?

Thanks
>cmd



On 13/08/2024 09:44, 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.
>
>
> In vtd_process_inv_desc(), VTD_INV_DESC_PC and VTD_INV_DESC_PIOTLB are
> bypassed without scalable mode check. These two types are not valid
> in legacy mode and we should report error.
>
> Suggested-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/i386/intel_iommu.c | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 68cb72a481..90cd4e5044 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2763,17 +2763,6 @@ static bool vtd_process_inv_desc(IntelIOMMUState *s)
>           }
>           break;
>
> -    /*
> -     * TODO: the entity of below two cases will be implemented in future series.
> -     * To make guest (which integrates scalable mode support patch set in
> -     * iommu driver) work, just return true is enough so far.
> -     */
> -    case VTD_INV_DESC_PC:
> -        break;
> -
> -    case VTD_INV_DESC_PIOTLB:
> -        break;
> -
>       case VTD_INV_DESC_WAIT:
>           trace_vtd_inv_desc("wait", inv_desc.hi, inv_desc.lo);
>           if (!vtd_process_wait_desc(s, &inv_desc)) {
> @@ -2795,6 +2784,17 @@ static bool vtd_process_inv_desc(IntelIOMMUState *s)
>           }
>           break;
>
> +    /*
> +     * TODO: the entity of below two cases will be implemented in future series.
> +     * To make guest (which integrates scalable mode support patch set in
> +     * iommu driver) work, just return true is enough so far.
> +     */
> +    case VTD_INV_DESC_PC:
> +    case VTD_INV_DESC_PIOTLB:
> +        if (s->scalable_mode) {
> +            break;
> +        }
> +    /* fallthrough */
>       default:
>           error_report_once("%s: invalid inv desc: hi=%"PRIx64", lo=%"PRIx64
>                             " (unknown type)", __func__, inv_desc.hi,
> --
> 2.34.1
>
>

Re: [PATCH v2 2/2] intel_iommu: Make PASID-cache and PIOTLB type invalid in legacy mode

[Yi Liu]

On 2024/8/13 15:44, Zhenzhong Duan wrote:
> In vtd_process_inv_desc(), VTD_INV_DESC_PC and VTD_INV_DESC_PIOTLB are
> bypassed without scalable mode check. These two types are not valid
> in legacy mode and we should report error.
> 
> Suggested-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/i386/intel_iommu.c | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

Do you think a fix tag is needed or not? @Jason

> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 68cb72a481..90cd4e5044 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2763,17 +2763,6 @@ static bool vtd_process_inv_desc(IntelIOMMUState *s)
>           }
>           break;
>   
> -    /*
> -     * TODO: the entity of below two cases will be implemented in future series.
> -     * To make guest (which integrates scalable mode support patch set in
> -     * iommu driver) work, just return true is enough so far.
> -     */
> -    case VTD_INV_DESC_PC:
> -        break;
> -
> -    case VTD_INV_DESC_PIOTLB:
> -        break;
> -
>       case VTD_INV_DESC_WAIT:
>           trace_vtd_inv_desc("wait", inv_desc.hi, inv_desc.lo);
>           if (!vtd_process_wait_desc(s, &inv_desc)) {
> @@ -2795,6 +2784,17 @@ static bool vtd_process_inv_desc(IntelIOMMUState *s)
>           }
>           break;
>   
> +    /*
> +     * TODO: the entity of below two cases will be implemented in future series.
> +     * To make guest (which integrates scalable mode support patch set in
> +     * iommu driver) work, just return true is enough so far.
> +     */
> +    case VTD_INV_DESC_PC:
> +    case VTD_INV_DESC_PIOTLB:
> +        if (s->scalable_mode) {
> +            break;
> +        }
> +    /* fallthrough */
>       default:
>           error_report_once("%s: invalid inv desc: hi=%"PRIx64", lo=%"PRIx64
>                             " (unknown type)", __func__, inv_desc.hi,

-- 
Regards,
Yi Liu

Re: [PATCH v2 2/2] intel_iommu: Make PASID-cache and PIOTLB type invalid in legacy mode

[Jason Wang]

On Tue, Aug 13, 2024 at 4:31 PM Yi Liu <yi.l.liu@intel.com> wrote:
>
> On 2024/8/13 15:44, Zhenzhong Duan wrote:
> > In vtd_process_inv_desc(), VTD_INV_DESC_PC and VTD_INV_DESC_PIOTLB are
> > bypassed without scalable mode check. These two types are not valid
> > in legacy mode and we should report error.
> >
> > Suggested-by: Yi Liu <yi.l.liu@intel.com>
> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> > ---
> >   hw/i386/intel_iommu.c | 22 +++++++++++-----------
> >   1 file changed, 11 insertions(+), 11 deletions(-)
>
> Reviewed-by: Yi Liu <yi.l.liu@intel.com>
>
> Do you think a fix tag is needed or not? @Jason

I think so (as it might have a guest triggerable issue).

Thanks

Re: [PATCH v2 2/2] intel_iommu: Make PASID-cache and PIOTLB type invalid in legacy mode

[Yi Liu]

On 2024/8/13 16:00, CLEMENT MATHIEU--DRIF wrote:
> Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com>
> 
> Super reactive!
> 
> Maybe we can continue along this path after the handlers are implemented.
> It would be great to make sure we don't process PASID related descriptors when not in scalable mode.
> What are your thoughts?

yeah. let's keep it in mind when reviewing the stage-1 translation series.

> Thanks
>> cmd
> 
> 
> 
> On 13/08/2024 09:44, 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.
>>
>>
>> In vtd_process_inv_desc(), VTD_INV_DESC_PC and VTD_INV_DESC_PIOTLB are
>> bypassed without scalable mode check. These two types are not valid
>> in legacy mode and we should report error.
>>
>> Suggested-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>    hw/i386/intel_iommu.c | 22 +++++++++++-----------
>>    1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 68cb72a481..90cd4e5044 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -2763,17 +2763,6 @@ static bool vtd_process_inv_desc(IntelIOMMUState *s)
>>            }
>>            break;
>>
>> -    /*
>> -     * TODO: the entity of below two cases will be implemented in future series.
>> -     * To make guest (which integrates scalable mode support patch set in
>> -     * iommu driver) work, just return true is enough so far.
>> -     */
>> -    case VTD_INV_DESC_PC:
>> -        break;
>> -
>> -    case VTD_INV_DESC_PIOTLB:
>> -        break;
>> -
>>        case VTD_INV_DESC_WAIT:
>>            trace_vtd_inv_desc("wait", inv_desc.hi, inv_desc.lo);
>>            if (!vtd_process_wait_desc(s, &inv_desc)) {
>> @@ -2795,6 +2784,17 @@ static bool vtd_process_inv_desc(IntelIOMMUState *s)
>>            }
>>            break;
>>
>> +    /*
>> +     * TODO: the entity of below two cases will be implemented in future series.
>> +     * To make guest (which integrates scalable mode support patch set in
>> +     * iommu driver) work, just return true is enough so far.
>> +     */
>> +    case VTD_INV_DESC_PC:
>> +    case VTD_INV_DESC_PIOTLB:
>> +        if (s->scalable_mode) {
>> +            break;
>> +        }
>> +    /* fallthrough */
>>        default:
>>            error_report_once("%s: invalid inv desc: hi=%"PRIx64", lo=%"PRIx64
>>                              " (unknown type)", __func__, inv_desc.hi,
>> --
>> 2.34.1
>>
>>

-- 
Regards,
Yi Liu