RE: [RFC v1 10/18] intel_iommu: tag VTDAddressSpace instance with PASID

["Liu, Yi L" <yi.l.liu@intel.com>]

> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf
> Of Peter Xu
> Sent: Tuesday, July 9, 2019 2:13 PM
> To: Liu, Yi L <yi.l.liu@intel.com>
> Subject: Re: [RFC v1 10/18] intel_iommu: tag VTDAddressSpace instance with PASID
> 
> 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;
>         }

Nice suggestion. Let me try it.

> 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.

If using structure, no need indeed. :-)

> [...]
> 
> > +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...

Yes, I haven’t added in this RFC. So listed it as a TODO here. This would be done
after the work flow is clear. :-)

> [...]
> 
> >  /* 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()?

Nice catch. I used hash_pasid_as_free() because of that I'd like to do something other
than free the VTDAddressSpace instance. e.g. destroy the AddressSpace MemoryRegion
instances before free VTDAddressSpace instance. That's related to another comment
from you in anther thread. :-)

For now, I think it is fine to drop it and just use g_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.

Good suggestion, will make it sound in next version.

Thanks,
Yi Liu

> Regards,
> 
> --
> Peter Xu