On Mon, 7 Nov 2022 at 22:53, Michael S. Tsirkin <m...@redhat.com> wrote:
>
> From: Jason Wang <jasow...@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
