Re: [PULL v4 74/83] intel-iommu: PASID support

[Jason Wang]

On Fri, Apr 7, 2023 at 12:22 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 7 Nov 2022 at 22:53, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Jason Wang <jasowang@redhat.com>
> >
> > This patch introduce ECAP_PASID via "x-pasid-mode".
>
> Hi; Coverity points out an issue with this code (CID 1508100):
>
> > -static guint vtd_uint64_hash(gconstpointer v)
> > +static guint vtd_iotlb_hash(gconstpointer v)
> >  {
> > -    return (guint)*(const uint64_t *)v;
> > +    const struct vtd_iotlb_key *key = v;
> > +
> > +    return key->gfn | ((key->sid) << VTD_IOTLB_SID_SHIFT) |
>
> key->sid is a uint16_t, and VTD_IOTLB_SID_SHIFT is 20. That
> means that the shift will be done as a signed 32 bit operation,
> losing the top 4 bits of key->sid; then it will get sign
> extended to 64 bits, so if bit 11 of key->sid is 1 then
> we will end up with 1 bits in 63..32 of the output hash value.
> This seems unlikely to be what was intended.

Right.

>
> > +           (key->level) << VTD_IOTLB_LVL_SHIFT |
> > +           (key->pasid) << VTD_IOTLB_PASID_SHIFT;
> >  }
>
> Also, VTD_IOTLB_LVL_SHIFT is only 28, so either the
> shift values are wrong or the type of key->sid is wrong:
> can there be 8 bits here, or 16 ?

It should be 16.

>
> Since PASID_SHIFT is 30, if key->pasid can be more than
> 2 bits wide we'll lose most of it.
>
> If key->level will fit into 2 bits as the SHIFT values
> suggest, vtd_iotlb_key could probably use a uint8_t for it,

Right.

> which would let that struct fit into 16 bytes rather than 18.

Ok, So to summarize:

1) key->gfn could be full 64bit
2) key->sid is at most 16bit
3) key->level is at most 2bit
4) key->pasid is at most 20bit

So we will lose some bits for sure.

Since the current VTD_IOTLB_PASID_SHIFT is 30, we might waste 14bits
there, then I tend to change

VTD_IOTLB_SID_SHIFT to 26 (42-16)
VTD_IOTLB_LVL_SHIFT to 42 (44-2)
VTD_IOTLB_PASID_SHIFT to 44 (64-20)

>
> > @@ -302,13 +321,6 @@ static void vtd_reset_caches(IntelIOMMUState *s)
> >      vtd_iommu_unlock(s);
> >  }
> >
> > -static uint64_t vtd_get_iotlb_key(uint64_t gfn, uint16_t source_id,
> > -                                  uint32_t level)
> > -{
> > -    return gfn | ((uint64_t)(source_id) << VTD_IOTLB_SID_SHIFT) |
> > -           ((uint64_t)(level) << VTD_IOTLB_LVL_SHIFT);
> > -}
>
> In the old code you can see that we did casts to uint64_t in order
> to ensure that all the arithmetic was done as unsigned 64 bits.

Right, I will post a fix.

Thanks

>
> thanks
> -- PMM
>