Re: [PATCH v2 04/17] intel_iommu: Flush stage-2 cache in PASID-selective PASID-based iotlb invalidation

[CLEMENT MATHIEU--DRIF]

On 13/08/2024 09:13, CLEMENT MATHIEU--DRIF wrote:
>
> On 13/08/2024 04:12, 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>
>>> Subject: Re: [PATCH v2 04/17] intel_iommu: Flush stage-2 cache in PASID-
>>> selective PASID-based iotlb invalidation
>>>
>>>
>>>
>>> On 08/08/2024 14:40, 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.
>>>>
>>>>
>>>> On 8/6/2024 2:35 PM, CLEMENT MATHIEU--DRIF wrote:
>>>>> On 05/08/2024 08:27, 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.
>>>>>>
>>>>>>
>>>>>> Per spec 6.5.2.4, PADID-selective PASID-based iotlb invalidation will
>>>>>> flush stage-2 iotlb entries with matching domain id and pasid.
>>>>>>
>>>>>> With scalable modern mode introduced, guest could send PASID-
>>> selective
>>>>>> PASID-based iotlb invalidation to flush both stage-1 and stage-2
>>>>>> entries.
>>>>>>
>>>>>> By this chance, remove old IOTLB related definition.
>>>>>>
>>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>>>> ---
>>>>>>      hw/i386/intel_iommu_internal.h | 14 +++---
>>>>>>      hw/i386/intel_iommu.c          | 81
>>>>>> ++++++++++++++++++++++++++++++++++
>>>>>>      2 files changed, 90 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/i386/intel_iommu_internal.h
>>>>>> b/hw/i386/intel_iommu_internal.h
>>>>>> index 8fa27c7f3b..19e4ed52ca 100644
>>>>>> --- a/hw/i386/intel_iommu_internal.h
>>>>>> +++ b/hw/i386/intel_iommu_internal.h
>>>>>> @@ -402,11 +402,6 @@ typedef union VTDInvDesc VTDInvDesc;
>>>>>>      #define VTD_INV_DESC_IOTLB_AM(val)      ((val) & 0x3fULL)
>>>>>>      #define VTD_INV_DESC_IOTLB_RSVD_LO 0xffffffff0000ff00ULL
>>>>>>      #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)
>>>>>> -#define VTD_INV_DESC_IOTLB_PASID(val)   (((val) >> 32) &
>>>>>> VTD_PASID_ID_MASK)
>>>>>> -#define VTD_INV_DESC_IOTLB_PASID_RSVD_LO
>>> 0xfff00000000001c0ULL
>>>>>> -#define VTD_INV_DESC_IOTLB_PASID_RSVD_HI      0xf80ULL
>>>>>>
>>>>>>      /* Mask for Device IOTLB Invalidate Descriptor */
>>>>>>      #define VTD_INV_DESC_DEVICE_IOTLB_ADDR(val) ((val) &
>>>>>> 0xfffffffffffff000ULL)
>>>>>> @@ -438,6 +433,15 @@ typedef union VTDInvDesc VTDInvDesc;
>>>>>>              (0x3ffff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM |
>>>>>> VTD_SL_TM)) : \
>>>>>>              (0x3ffff800ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM))
>>>>>>
>>>>>> +/* Masks for PIOTLB Invalidate Descriptor */
>>>>>> +#define VTD_INV_DESC_PIOTLB_G             (3ULL << 4)
>>>>>> +#define VTD_INV_DESC_PIOTLB_ALL_IN_PASID  (2ULL << 4)
>>>>>> +#define VTD_INV_DESC_PIOTLB_PSI_IN_PASID  (3ULL << 4)
>>>>>> +#define VTD_INV_DESC_PIOTLB_DID(val)      (((val) >> 16) &
>>>>>> VTD_DOMAIN_ID_MASK)
>>>>>> +#define VTD_INV_DESC_PIOTLB_PASID(val)    (((val) >> 32) & 0xfffffULL)
>>>>>> +#define VTD_INV_DESC_PIOTLB_RSVD_VAL0 0xfff000000000f1c0ULL
>>>>> Why did this value change since last post? The 'type' field should
>>>>> always be zero in this desc
>>>> Yes, type[6:4] are all zero for all existing invalidation type. But they
>>>> are not real reserved bits.
>>>>
>>>> So I removed them from VTD_INV_DESC_PIOTLB_RSVD_VAL0.
>>> Other masks consider these zeroes as reserved.
>>> I think we should do the same.
>>> For instance, context cache invalidation is : #define
>>> VTD_INV_DESC_CC_RSVD 0xfffc00000000ffc0ULL
>> Yes, I'll make a separate patch to fix it.
> Oops, I just saw your patch, sorry for the misunderstanding!!
> I think we should continue treating these bits as reserved because the
> descriptor type detection code only checks the 4 LSB.
Oh, you fixed it as well,
so, ok I agree with the changes.
Sorry for that
>> Thanks
>> Zhenzhong