On Fri, Apr 7, 2023 at 12:22 AM Peter Maydell <peter.mayd...@linaro.org> wrote: > > 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.
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 >