Re: [PATCH 2/3] intel-iommu: drop VTDBus

[Peter Xu]

On Mon, Jan 17, 2022 at 09:27:03AM +0800, Peter Xu wrote:
> On Fri, Jan 14, 2022 at 05:15:35PM +0800, Jason Wang wrote:
> > 
> > 在 2022/1/14 上午10:32, Jason Wang 写道:
> > > > > dressSpace *as)
> > > > >   /* GHashTable functions */
> > > > >   static gboolean vtd_uint64_equal(gconstpointer v1, gconstpointer v2)
> > > > >   {
> > > > > -    return *((const uint64_t *)v1) == *((const uint64_t *)v2);
> > > > > +    const struct vtd_as_key *key1 = v1;
> > > > > +    const struct vtd_as_key *key2 = v2;
> > > > > +
> > > > > +    return (key1->bus == key2->bus) && (key1->devfn == key2->devfn);
> > > > >   }
> > > > >     static guint vtd_uint64_hash(gconstpointer v)
> > > > >   {
> > > > > -    return (guint)*(const uint64_t *)v;
> > > > > +    const struct vtd_as_key *key = v;
> > > > > +    guint value = (guint)(uintptr_t)key->bus;
> > > > > +
> > > > > +    return (guint)(value << 8 | key->devfn);
> > > > Note that value is a pointer to PCIBus*.  Just want to check with
> > > > you that it's
> > > > intended to use this hash value (or maybe you wanted to use Source
> > > > ID so it is
> > > > bus number to use not the bus pointer)?
> > > 
> > > 
> > > Right, SID should be used here.
> > 
> > 
> > Sorry for taking too long for the context switching ...
> > 
> > The hash and shift based the bus pointer is intended since we use bus
> > pointer as part of the key. The reason is still, during vtd_find_add_as(),
> > SID is not correct since guest might not finish the initialization of the
> > device and PCI bridge.
> 
> Ah, right.
> 
> However here value is left-shifted 8 bits so I'm not sure how it could
> guarantee no collision - logically any addresses that match lower 56 bits will
> hit with the same devfn by accident.
> 
> I don't think it'll matter in reality, but... wondering whether it's easier to
> simply use g_direct_hash() (the glibc provided hash function when hash==NULL 
> is
> passed over, IOW we simply pass hash_func=NULL for g_hash_table_new) so we'll
> hash with struct vtd_as_key* instead; IMHO that'll suffice too.

Please ignore this - I think what you said should work indeed, and hash
collision should be fine with the equal function.  Dangling vtd_as_key* may not
really work.

-- 
Peter Xu