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

[Peter Maydell]

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.

> +           (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 ?

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,
which would let that struct fit into 16 bytes rather than 18.

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

thanks
-- PMM