On Mon, 17 Jul 2023 at 17:56, Eric Auger <eric.au...@redhat.com> wrote: > > > Hi Peter, > On 7/17/23 12:50, Peter Maydell wrote: > > On Tue, 11 Jul 2023 at 00:04, Michael S. Tsirkin <m...@redhat.com> wrote: > >> From: Eric Auger <eric.au...@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.au...@redhat.com> > > Hi; Coverity complains about this change (CID 1517772): > > thank you for reporting the issue > > > >> +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? > To me this cannot happen. The page_size_mask is initialized with > qemu_target_page_mask(), then further constrained with > virtio_iommu_set_page_size_mask() which would call error_setg if the new > mask is 0 or (cur_mask & new_mask) == 0 > > What can I do to give coverity a hint that page_size_mask cannot be NULL?
You can assert() it if you believe it to be true and you think an assert() would help a human reader, or we could just say "this is a false positive" and mark it that way in the Coverity UI. We don't need to change things purely to make Coverity happy. > > 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. > agreed. Looking more closely at this, the file is not entirely consistent about how it handles the page_size_mask: * in virtio_iommu_translate() we call ctz32() and then use an open-coded 1 << n on that * in virtio_iommu_set_page_size_mask() we call ctz64() and use BIT() * in virtio_iommu_freeze_granule() we call ctz64() and then use BIT() So I suspect it's actually true (or at least assumed) that the granule value can never be 32 or higher, as well as it being non-zero. thanks -- PMM