Re: [PATCH for-8.1] virtio-iommu: Standardize granule extraction and formatting

[Jean-Philippe Brucker]

On Tue, Jul 18, 2023 at 08:21:36PM +0200, Eric Auger wrote:
> At several locations we compute the granule from the config
> page_size_mask using ctz() and then format it in traces using
> BIT(). As the page_size_mask is 64b we should use ctz64 and
> BIT_ULL() for formatting. We failed to be consistent.
> 
> Note the page_size_mask is garanteed to be non null. The spec
> mandates the device to set at least one bit, so ctz64 cannot
> return 64. This is garanteed by the fact the device
> initializes the page_size_mask to qemu_target_page_mask()
> and then the page_size_mask is further constrained by
> virtio_iommu_set_page_size_mask() callback which can't
> result in a new mask being null. So if Coverity complains
> round those ctz64/BIT_ULL with CID 1517772 this is a false
> positive
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Fixes: 94df5b2180 ("virtio-iommu: Fix 64kB host page size VFIO device 
> assignment")

Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

> ---
>  hw/virtio/virtio-iommu.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index 201127c488..c6ee4d7a3c 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -852,17 +852,19 @@ static IOMMUTLBEntry 
> virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>      VirtIOIOMMUEndpoint *ep;
>      uint32_t sid, flags;
>      bool bypass_allowed;
> +    int granule;
>      bool found;
>      int i;
>  
>      interval.low = addr;
>      interval.high = addr + 1;
> +    granule = ctz64(s->config.page_size_mask);
>  
>      IOMMUTLBEntry entry = {
>          .target_as = &address_space_memory,
>          .iova = addr,
>          .translated_addr = addr,
> -        .addr_mask = (1 << ctz32(s->config.page_size_mask)) - 1,
> +        .addr_mask = BIT_ULL(granule) - 1,
>          .perm = IOMMU_NONE,
>      };
>  
> @@ -1115,7 +1117,7 @@ static int 
> virtio_iommu_set_page_size_mask(IOMMUMemoryRegion *mr,
>      if (s->granule_frozen) {
>          int cur_granule = ctz64(cur_mask);
>  
> -        if (!(BIT(cur_granule) & new_mask)) {
> +        if (!(BIT_ULL(cur_granule) & new_mask)) {
>              error_setg(errp, "virtio-iommu %s does not support frozen 
> granule 0x%llx",
>                         mr->parent_obj.name, BIT_ULL(cur_granule));
>              return -1;
> @@ -1161,7 +1163,7 @@ static void virtio_iommu_freeze_granule(Notifier 
> *notifier, void *data)
>      }
>      s->granule_frozen = true;
>      granule = ctz64(s->config.page_size_mask);
> -    trace_virtio_iommu_freeze_granule(BIT(granule));
> +    trace_virtio_iommu_freeze_granule(BIT_ULL(granule));
>  }
>  
>  static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
> -- 
> 2.38.1
>