On Fri, Jul 05, 2019 at 07:01:43PM +0800, Liu Yi L wrote:
> This patch introduces new fields in VTDAddressSpace for further PASID support
> in Intel vIOMMU. In old time, each device has a VTDAddressSpace instance to
> stand for its guest IOVA address space when vIOMMU is enabled. However, when
> PASID is exposed to guest, device will have multiple address spaces which
> are tagged with PASID. To suit this change, VTDAddressSpace should be tagged
> with PASIDs in Intel vIOMMU.
>
> To record PASID tagged VTDAddressSpaces, a hash table is introduced. The
> data in the hash table can be used for future sanity check and retrieve
> previous PASID configs of guest and also future emulated SVA DMA support
> for emulated SVA capable devices. The lookup key is a string and its format
> is as below:
>
> "rsv%04dpasid%010dsid%06d" -- totally 32 bytes
Can we make it simply a struct?
struct pasid_key {
uint32_t pasid;
uint16_t sid;
}
Also I think we don't need to keep reserved bits because it'll be a
structure that'll only be used by QEMU so we can extend it easily in
the future when necessary.
[...]
> +static int vtd_pasid_cache_dsi(IntelIOMMUState *s, uint16_t domain_id)
> +{
> + VTDPASIDCacheInfo pc_info;
> +
> + trace_vtd_pasid_cache_dsi(domain_id);
> +
> + pc_info.flags = VTD_PASID_CACHE_DOMSI;
> + pc_info.domain_id = domain_id;
> +
> + /*
> + * use g_hash_table_foreach_remove(), which will free the
> + * vtd_pasid_as instances.
> + */
> + g_hash_table_foreach_remove(s->vtd_pasid_as, vtd_flush_pasid, &pc_info);
> + /*
> + * TODO: Domain selective PASID cache invalidation
> + * may be issued wrongly by programmer, to be safe,
> + * after invalidating the pasid caches, emulator
> + * needs to replay the pasid bindings by walking guest
> + * pasid dir and pasid table.
> + */
It seems to me that this is still unchanged for the whole series.
It's fine for RFC, but just a reminder that please either comment on
why we don't have something or implement what we need here...
[...]
> /* Unmap the whole range in the notifier's scope. */
> static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
> {
> @@ -3914,6 +4076,8 @@ static void vtd_realize(DeviceState *dev, Error **errp)
> g_free, g_free);
> s->vtd_as_by_busptr = g_hash_table_new_full(vtd_uint64_hash, vtd_uint64_equal,
> g_free, g_free);
> + s->vtd_pasid_as = g_hash_table_new_full(&g_str_hash, &g_str_equal,
> + g_free, hash_pasid_as_free);
Can use g_free() and drop hash_pasid_as_free()?
Also, this patch only tries to drop entries of the hash table but the
hash table is never inserted or used. I would suggest that you put
that part to be with this patch as a whole otherwise it's hard to
clarify how this hash table will be used.
Regards,
--
Peter Xu
