On 7/17/23 13:51, Michael S. Tsirkin wrote:
> On Mon, Jul 17, 2023 at 11:50:54AM +0100, 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):
>>
>>> +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?
Sure
Eric
>
