>-----Original Message-----
>From: Eric Auger <eric.au...@redhat.com>
>Subject: Re: [PATCH v3 10/20] intel_iommu: Handle PASID entry removing and
>updating
...
>> +static bool vtd_pasid_entry_compare(VTDPASIDEntry *p1, VTDPASIDEntry
>*p2)
>> +{
>> + return !memcmp(p1, p2, sizeof(*p1));
>> +}
>> +
>> +/*
>> + * This function is used to update or clear cached pasid entry in vtd_as.
>> + * vtd_as is released when corresponding cached pasid entry is cleared,
>> + * except for PCI_NO_PASID which vtd_as is owen by PCI sub-system.
>I would document when it is supposed to return true (indicates that the
>cached pasid entry needs to be removed).
Will do.
>> + */
>> +static gboolean vtd_flush_pasid_locked(gpointer key, gpointer value,
>> + gpointer user_data)
>> +{
>> + VTDPASIDCacheInfo *pc_info = user_data;
>> + VTDAddressSpace *vtd_as = value;
>> + VTDPASIDCacheEntry *pc_entry = &vtd_as->pasid_cache_entry;
>> + VTDPASIDEntry pe;
>> + uint16_t did;
>> + uint32_t pasid;
>> + int ret;
>> +
>> + if (!pc_entry->valid) {
>> + return false;
>> + }
>> + did = VTD_SM_PASID_ENTRY_DID(&pc_entry->pasid_entry);
>> +
>> + if (vtd_as_to_iommu_pasid_locked(vtd_as, &pasid)) {
>> + goto remove;
>> + }
>> +
>> + switch (pc_info->type) {
>> + case VTD_PASID_CACHE_PASIDSI:
>> + if (pc_info->pasid != pasid) {
>> + return false;
>> + }
>> + /* fall through */
>> + case VTD_PASID_CACHE_DOMSI:
>> + if (pc_info->did != did) {
>> + return false;
>> + }
>> + /* fall through */
>> + case VTD_PASID_CACHE_GLOBAL_INV:
>> + break;
>> + default:
>> + error_setg(&error_fatal, "invalid pc_info->type for flush");
>> + }
>> +
>> + /*
>> + * pasid cache invalidation may indicate a present pasid
>> + * entry to present pasid entry modification. To cover such
>> + * case, vIOMMU emulator needs to fetch latest guest pasid
>> + * entry and compares with cached pasid entry, then update
>> + * pasid cache.
>> + */
>> + ret = vtd_dev_get_pe_from_pasid(vtd_as, pasid, &pe);
>> + if (ret) {
>> + /*
>> + * No valid pasid entry in guest memory. e.g. pasid entry
>> + * was modified to be either all-zero or non-present. Either
>> + * case means existing pasid cache should be removed.
>> + */
>> + goto remove;
>> + }
>> +
>> + /* No need to update if cached pasid entry is latest */
>that comment sounds really weird to me. In case the cache entry does not
>match the one in guest mem, you update it below - at least that's what I
>understand ;-) - However you want to return false because you don't want
>g_hash_table_foreach_remove() to remove the entry.
You understand totally right😊, will rephase to:
/* Update cached pasid entry if it's stale compared to what's in guest memory */
>> + if (!vtd_pasid_entry_compare(&pe, &pc_entry->pasid_entry)) {
>> + pc_entry->pasid_entry = pe;
>> + }
>> + return false;
>> +
>> +remove:
>> + pc_entry->valid = false;
>> +
>> + /*
>> + * Don't remove address space of PCI_NO_PASID which is created for
>PCI
>> + * sub-system.
>> + */
>> + if (vtd_as->pasid == PCI_NO_PASID) {
>> + return false;
>> + }
>> + return true;
>> +}
>> +
>> +/* Update the pasid cache in vIOMMU */
>> +static void vtd_pasid_cache_sync(IntelIOMMUState *s,
>VTDPASIDCacheInfo *pc_info)
>> +{
>> + if (!s->flts || !s->root_scalable || !s->dmar_enabled) {
>> + return;
>> + }
>> +
>> + /*
>> + * Regards to a pasid cache invalidation, e.g. a PSI.
>Regarded PASID cache invalidation?
>> + * It could be either cases of below:
>> + * a) a present pasid entry moved to non-present
>a present cache pasid entry needs to be removed
>> + * b) a present pasid entry to be a present entry
>above sounds a bit weird. A present cached pasid entry needs to be updated
>> + * c) a non-present pasid entry moved to present
>a present cached pasid entry needs to be created. As this is not handled
>in this patch I would move this to next one.
>Besides since there is another comment below I am not even sure this is
>requested or at least I would put this in a doc comment for the function
>and not within the code.
Will do.
>> --- a/hw/i386/intel_iommu_internal.h
>> +++ b/hw/i386/intel_iommu_internal.h
>> @@ -315,6 +315,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 */
>> @@ -492,6 +493,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)
>thanks for converting to extract64 ;-)
>> +#define VTD_INV_DESC_PASIDC_RSVD_VAL0 0xfff000000000f1c0ULL
>nit: I find such mask definition error prone and difficult to review.
>combined MAKE_64BIT_MASK() would make it clearer I think
Yes, so we need to be careful to ensure its correctness when coding.
But I think a single 0xfff000000000f1c0ULL is cleaner than many
MAKE_64BIT_MASK() combined.
Thanks
Zhenzhong
