Re: [Qemu-devel] [RFC v1 10/18] intel_iommu: tag VTDAddressSpace instance with PASID

[Peter Xu]

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