Re: [PULL 46/66] virtio-iommu: Fix 64kB host page size VFIO device assignment

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jul 17, 2023 at 11:50:54AM +0100, Peter Maydell wrote:
> On Tue, 11 Jul 2023 at 00:04, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Eric Auger <eric.auger@redhat.com>
> >
> > When running on a 64kB page size host and protecting a VFIO device
> > with the virtio-iommu, qemu crashes with this kind of message:
> >
> > qemu-kvm: virtio-iommu page mask 0xfffffffffffff000 is incompatible
> > with mask 0x20010000
> > qemu: hardware error: vfio: DMA mapping failed, unable to continue
> >
> > This is due to the fact the IOMMU MR corresponding to the VFIO device
> > is enabled very late on domain attach, after the machine init.
> > The device reports a minimal 64kB page size but it is too late to be
> > applied. virtio_iommu_set_page_size_mask() fails and this causes
> > vfio_listener_region_add() to end up with hw_error();
> >
> > To work around this issue, we transiently enable the IOMMU MR on
> > machine init to collect the page size requirements and then restore
> > the bypass state.
> >
> > Fixes: 90519b9053 ("virtio-iommu: Add bypass mode support to assigned device")
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> Hi; Coverity complains about this change (CID 1517772):
> 
> > +static void virtio_iommu_freeze_granule(Notifier *notifier, void *data)
> > +{
> > +    VirtIOIOMMU *s = container_of(notifier, VirtIOIOMMU, machine_done);
> > +    int granule;
> > +
> > +    if (likely(s->config.bypass)) {
> > +        /*
> > +         * Transient IOMMU MR enable to collect page_size_mask requirements
> > +         * through memory_region_iommu_set_page_size_mask() called by
> > +         * VFIO region_add() callback
> > +         */
> > +         s->config.bypass = false;
> > +         virtio_iommu_switch_address_space_all(s);
> > +         /* restore default */
> > +         s->config.bypass = true;
> > +         virtio_iommu_switch_address_space_all(s);
> > +    }
> > +    s->granule_frozen = true;
> > +    granule = ctz64(s->config.page_size_mask);
> > +    trace_virtio_iommu_freeze_granule(BIT(granule));
> 
> Specifically, in this code, it thinks that ctz64() can
> return 64, in which case BIT(granule) is shifting off
> the end of the value, which is undefined behaviour.
> This can happen if s->config.page_size_mask is 0 -- are
> there assertions/checks that that can't happen elsewhere?
> 
> Secondly, BIT() only works for values up to 32, since
> it works on type unsigned long, which might be a 32-bit
> type on some hosts. Since you used ctz64()
> you probably want BIT_ULL() which uses the ULL type
> which definitely has 64 bits.
> 
> thanks
> -- PMM

Thanks! Eric can you fix pls?

-- 
MST