>-----Original Message-----
>From: Liu, Yi L <yi.l....@intel.com>
>Subject: Re: [PATCH v5 11/21] intel_iommu: Handle PASID entry removal and
>update
>
>On 2025/9/1 11:31, Duan, Zhenzhong wrote:
>>
>>
>>> -----Original Message-----
>>> From: Liu, Yi L <yi.l....@intel.com>
>>> Subject: Re: [PATCH v5 11/21] intel_iommu: Handle PASID entry removal
>and
>>> update
>>>
>>> On 2025/8/22 14:40, Zhenzhong Duan wrote:
>>>> This adds an new entry VTDPASIDCacheEntry in VTDAddressSpace to
>cache
>>> the
>>>> pasid entry and track PASID usage and future PASID tagged DMA address
>>>> translation support in vIOMMU.
>>>
>>> Have you seen any extra code needed based on this series to support non
>>> rid_pasid PASIDs? If no, may just relax the scope of this series.
>>> otherwise, you may need to tweak the patch a little bit. e.g. factor
>>> out setting x-flts and x-pasid-mode at the same time.
>>
>> There are quite a few code are common for both non-rid_pasid and
>rid_pasid.
>> So in this series, there are some infrastructure code that looks like it's
>> for
>non-rid_pasid.
>>
>> But to support non-rid_pasid, we need pasid_attach/detach() which is not
>implemented in this series.
>
>I see. Besides that, the vIOMMU internal infrastructure should be ready
>for non-rid_pasid after this series.
Okey, I'll following you and Eric's suggestion to simplify this series with
only rid_pasid support.
Thanks
>
>> Even if x-flts and x-pasid-mode both on, pasid isn't enabled since VFIO
>device doesn't > expose pasid capability to guest, so guest never use
>non-rid_pasid
>with this VFIO device.
>
>ok. Given that 1st stage for emulated device has already sbeen upported,
>it's fine to rely on the knob in device side.
>
>>>>
>>>> VTDAddressSpace of PCI_NO_PASID is allocated when device is plugged
>and
>>>> never freed. For other pasid, VTDAddressSpace instance is
>>> created/destroyed
>>>> per the guest pasid entry set up/destroy.
>>>
>>>> When guest removes or updates a PASID entry, QEMU will capture the
>guest
>>> pasid
>>>> selective pasid cache invalidation, removes VTDAddressSpace or update
>>> cached
>>>> PASID entry.
>>>>
>>>> vIOMMU emulator could figure out the reason by fetching latest guest
>pasid
>>> entry
>>>> and compare it with cached PASID entry.
>>>>
>>>> Signed-off-by: Yi Liu <yi.l....@intel.com>
>>>> Signed-off-by: Yi Sun <yi.y....@linux.intel.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
>>>> ---
>>>> hw/i386/intel_iommu_internal.h | 27 ++++-
>>>> include/hw/i386/intel_iommu.h | 6 +
>>>> hw/i386/intel_iommu.c | 196
>>> +++++++++++++++++++++++++++++++--
>>>> hw/i386/trace-events | 3 +
>>>> 4 files changed, 220 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/hw/i386/intel_iommu_internal.h
>>> b/hw/i386/intel_iommu_internal.h
>>>> index f7510861d1..b9b76dd996 100644
>>>> --- a/hw/i386/intel_iommu_internal.h
>>>> +++ b/hw/i386/intel_iommu_internal.h
>>>> @@ -316,6 +316,7 @@ typedef enum VTDFaultReason {
>>>> * request while disabled */
>>>> VTD_FR_IR_SID_ERR = 0x26, /* Invalid Source-ID */
>>>>
>>>> + VTD_FR_RTADDR_INV_TTM = 0x31, /* Invalid TTM in RTADDR */
>>>> /* PASID directory entry access failure */
>>>> VTD_FR_PASID_DIR_ACCESS_ERR = 0x50,
>>>> /* The Present(P) field of pasid directory entry is 0 */
>>>> @@ -493,6 +494,15 @@ typedef union VTDInvDesc VTDInvDesc;
>>>> #define VTD_INV_DESC_PIOTLB_RSVD_VAL0
>>> 0xfff000000000f1c0ULL
>>>> #define VTD_INV_DESC_PIOTLB_RSVD_VAL1 0xf80ULL
>>>>
>>>> +/* PASID-cache Invalidate Descriptor (pc_inv_dsc) fields */
>>>> +#define VTD_INV_DESC_PASIDC_G(x) extract64((x)->val[0], 4,
>2)
>>>> +#define VTD_INV_DESC_PASIDC_G_DSI 0
>>>> +#define VTD_INV_DESC_PASIDC_G_PASID_SI 1
>>>> +#define VTD_INV_DESC_PASIDC_G_GLOBAL 3
>>>> +#define VTD_INV_DESC_PASIDC_DID(x) extract64((x)->val[0], 16,
>>> 16)
>>>> +#define VTD_INV_DESC_PASIDC_PASID(x) extract64((x)->val[0], 32,
>>> 20)
>>>> +#define VTD_INV_DESC_PASIDC_RSVD_VAL0
>0xfff000000000f1c0ULL
>>>> +
>>>> /* Information about page-selective IOTLB invalidate */
>>>> struct VTDIOTLBPageInvInfo {
>>>> uint16_t domain_id;
>>>> @@ -553,6 +563,21 @@ typedef struct VTDRootEntry VTDRootEntry;
>>>> #define VTD_SM_CONTEXT_ENTRY_RSVD_VAL0(aw) (0x1e0ULL |
>>> ~VTD_HAW_MASK(aw))
>>>> #define VTD_SM_CONTEXT_ENTRY_RSVD_VAL1
>>> 0xffffffffffe00000ULL
>>>>
>>>> +typedef enum VTDPCInvType {
>>>> + /* VTD spec defined PASID cache invalidation type */
>>>> + VTD_PASID_CACHE_DOMSI = VTD_INV_DESC_PASIDC_G_DSI,
>>>> + VTD_PASID_CACHE_PASIDSI =
>VTD_INV_DESC_PASIDC_G_PASID_SI,
>>>> + VTD_PASID_CACHE_GLOBAL_INV =
>>> VTD_INV_DESC_PASIDC_G_GLOBAL,
>>>> +} VTDPCInvType;
>>>> +
>>>> +typedef struct VTDPASIDCacheInfo {
>>>> + VTDPCInvType type;
>>>> + uint16_t did;
>>>> + uint32_t pasid;
>>>> + PCIBus *bus;
>>>> + uint16_t devfn;
>>>> +} VTDPASIDCacheInfo;
>>>> +
>>>> /* PASID Table Related Definitions */
>>>> #define VTD_PASID_DIR_BASE_ADDR_MASK (~0xfffULL)
>>>> #define VTD_PASID_TABLE_BASE_ADDR_MASK (~0xfffULL)
>>>> @@ -574,7 +599,7 @@ typedef struct VTDRootEntry VTDRootEntry;
>>>> #define VTD_SM_PASID_ENTRY_PT (4ULL << 6)
>>>>
>>>> #define VTD_SM_PASID_ENTRY_AW 7ULL /* Adjusted
>>> guest-address-width */
>>>> -#define VTD_SM_PASID_ENTRY_DID(val) ((val) &
>>> VTD_DOMAIN_ID_MASK)
>>>> +#define VTD_SM_PASID_ENTRY_DID(x) extract64((x)->val[1], 0,
>16)
>>>>
>>>> #define VTD_SM_PASID_ENTRY_FLPM 3ULL
>>>> #define VTD_SM_PASID_ENTRY_FLPTPTR (~0xfffULL)
>>>> diff --git a/include/hw/i386/intel_iommu.h
>>> b/include/hw/i386/intel_iommu.h
>>>> index 50f9b27a45..0e3826f6f0 100644
>>>> --- a/include/hw/i386/intel_iommu.h
>>>> +++ b/include/hw/i386/intel_iommu.h
>>>> @@ -95,6 +95,11 @@ struct VTDPASIDEntry {
>>>> uint64_t val[8];
>>>> };
>>>>
>>>> +typedef struct VTDPASIDCacheEntry {
>>>> + struct VTDPASIDEntry pasid_entry;
>>>> + bool valid;
>>>> +} VTDPASIDCacheEntry;
>>>> +
>>>> struct VTDAddressSpace {
>>>> PCIBus *bus;
>>>> uint8_t devfn;
>>>> @@ -107,6 +112,7 @@ struct VTDAddressSpace {
>>>> MemoryRegion iommu_ir_fault; /* Interrupt region for catching
>>> fault */
>>>> IntelIOMMUState *iommu_state;
>>>> VTDContextCacheEntry context_cache_entry;
>>>> + VTDPASIDCacheEntry pasid_cache_entry;
>>>> QLIST_ENTRY(VTDAddressSpace) next;
>>>> /* Superset of notifier flags that this address space has */
>>>> IOMMUNotifierFlag notifier_flags;
>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>> index 1801f1cdf6..a2ee6d684e 100644
>>>> --- a/hw/i386/intel_iommu.c
>>>> +++ b/hw/i386/intel_iommu.c
>>>> @@ -1675,7 +1675,7 @@ static uint16_t
>>> vtd_get_domain_id(IntelIOMMUState *s,
>>>>
>>>> if (s->root_scalable) {
>>>> vtd_ce_get_pasid_entry(s, ce, &pe, pasid);
>>>> - return VTD_SM_PASID_ENTRY_DID(pe.val[1]);
>>>> + return VTD_SM_PASID_ENTRY_DID(&pe);
>>>> }
>>>>
>>>> return VTD_CONTEXT_ENTRY_DID(ce->hi);
>>>> @@ -3112,6 +3112,183 @@ static bool
>>> vtd_process_piotlb_desc(IntelIOMMUState *s,
>>>> return true;
>>>> }
>>>>
>>>> +static inline int vtd_dev_get_pe_from_pasid(VTDAddressSpace *vtd_as,
>>>> + uint32_t pasid,
>>> VTDPASIDEntry *pe)
>>>> +{
>>>> + IntelIOMMUState *s = vtd_as->iommu_state;
>>>> + VTDContextEntry ce;
>>>> + int ret;
>>>> +
>>>> + if (!s->root_scalable) {
>>>> + return -VTD_FR_RTADDR_INV_TTM;
>>>> + }
>>>> +
>>>> + ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),
>>> vtd_as->devfn,
>>>> + &ce);
>>>> + if (ret) {
>>>> + return ret;
>>>> + }
>>>> +
>>>> + return vtd_ce_get_pasid_entry(s, &ce, pe, pasid);
>>>> +}
>>>> +
>>>> +static bool vtd_pasid_entry_compare(VTDPASIDEntry *p1,
>VTDPASIDEntry
>>> *p2)
>>>> +{
>>>> + return !memcmp(p1, p2, sizeof(*p1));
>>>> +}
>>>> +
>>>> +/*
>>>> + * This function is a loop function which return value determines if
>>>> + * vtd_as including cached pasid entry is removed.
>>>> + *
>>>> + * For PCI_NO_PASID, when corresponding cached pasid entry is
>cleared,
>>>> + * it returns false so that vtd_as is reserved as it's owned by PCI
>>>> + * sub-system. For other pasid, it returns true so vtd_as is removed.
>>>
>>> also, this helper will always return true if this series does not
>>> support non-rid_pasid PASID.
>>
>> Do you mean return false? I don't think it will return true.
>> For non-rid_pasid, it may return false.
>
>aha, yes. for rid_pasid, you need to keep the vtd_as instance.
>
>Regards,
>Yi Liu
