Re: [PATCH for 8.1] intel_iommu: refine iotlb hash calculation

[Peter Xu]

On Mon, Apr 10, 2023 at 11:32:08AM +0800, Jason Wang wrote:
> Commit 1b2b12376c8 ("intel-iommu: PASID support") takes PASID into
> account when calculating iotlb hash like:
> 
> static guint vtd_iotlb_hash(gconstpointer v)
> {
>     const struct vtd_iotlb_key *key = v;
> 
>     return key->gfn | ((key->sid) << VTD_IOTLB_SID_SHIFT) |
>            (key->level) << VTD_IOTLB_LVL_SHIFT |
>            (key->pasid) << VTD_IOTLB_PASID_SHIFT;
> }
> 
> This turns out to be problematic since:
> 
> - the shift will lose bits if not converting to uint64_t
> - level should be off by one in order to fit into 2 bits
> - VTD_IOTLB_PASID_SHIFT is 30 but PASID is 20 bits which will waste
>   some bits
> 
> So this patch fixes them by
> 
> - converting the keys into uint64_t before doing the shift
> - off level by one to make it fit into two bits
> - change the sid, lvl and pasid shift to 26, 42 and 44 in order to
>   take the full width of uint64_t if possible
> 
> Fixes: Coverity CID 1508100
> Fixes: 1b2b12376c8 ("intel-iommu: PASID support")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/i386/intel_iommu.c          | 8 ++++----
>  hw/i386/intel_iommu_internal.h | 6 +++---
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index a62896759c..d394976e9b 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -64,8 +64,8 @@ struct vtd_as_key {
>  struct vtd_iotlb_key {
>      uint64_t gfn;
>      uint32_t pasid;
> -    uint32_t level;
>      uint16_t sid;
> +    uint8_t level;
>  };
>  
>  static void vtd_address_space_refresh_all(IntelIOMMUState *s);
> @@ -222,9 +222,9 @@ static guint vtd_iotlb_hash(gconstpointer v)
>  {
>      const struct vtd_iotlb_key *key = v;
>  
> -    return key->gfn | ((key->sid) << VTD_IOTLB_SID_SHIFT) |
> -           (key->level) << VTD_IOTLB_LVL_SHIFT |
> -           (key->pasid) << VTD_IOTLB_PASID_SHIFT;
> +    return key->gfn | ((uint64_t)(key->sid) << VTD_IOTLB_SID_SHIFT) |
> +        (uint64_t)(key->level - 1) << VTD_IOTLB_LVL_SHIFT |
> +        (uint64_t)(key->pasid) << VTD_IOTLB_PASID_SHIFT;
>  }
>  
>  static gboolean vtd_as_equal(gconstpointer v1, gconstpointer v2)
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index f090e61e11..2e61eec2f5 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -114,9 +114,9 @@
>                                       VTD_INTERRUPT_ADDR_FIRST + 1)
>  
>  /* The shift of source_id in the key of IOTLB hash table */
> -#define VTD_IOTLB_SID_SHIFT         20
> -#define VTD_IOTLB_LVL_SHIFT         28
> -#define VTD_IOTLB_PASID_SHIFT       30
> +#define VTD_IOTLB_SID_SHIFT         26
> +#define VTD_IOTLB_LVL_SHIFT         42
> +#define VTD_IOTLB_PASID_SHIFT       44

This is for the hash function only, IIUC it means anything over
sizeof(guint) will be ignored and not contributing anything to the hash
value being generated due to the uint64->guint conversion.

IOW, I think "level" and "pasid" will just be ignored.

If the whole point of hash function here is to try provide the best
distribution of hash value generated for keys... perhaps we can cut some of
the fields, but still we should fit all things into 32bits?

My wild guess is coverity complains mostly because of the shift overflow,
so that'll be addressed too if we explicit cut things off with uint64_t.

Thanks,

>  #define VTD_IOTLB_MAX_SIZE          1024    /* Max size of the hash table */
>  
>  /* IOTLB_REG */
> -- 
> 2.25.1
> 

-- 
Peter Xu