Re: [PATCH v4 3/3] virtio-iommu: Change the default granule to the host page size

[Eric Auger]

On 2/23/24 09:08, Philippe Mathieu-Daudé wrote:
> Hi Eric,
>
> On 23/2/24 08:27, Eric Auger wrote:
>> We used to set the default granule to 4KB but with VFIO assignment
>> it makes more sense to use the actual host page size.
>>
>> Indeed when hotplugging a VFIO device protected by a virtio-iommu
>> on a 64kB/64kB host/guest config, we current get a qemu crash:
>>
>> "vfio: DMA mapping failed, unable to continue"
>>
>> This is due to the hot-attached VFIO device calling
>> memory_region_iommu_set_page_size_mask() with 64kB granule
>> whereas the virtio-iommu granule was already frozen to 4KB on
>> machine init done.
>>
>> Set the granule property to "host" and introduce a new compat.
>>
>> Note that the new default will prevent 4kB guest on 64kB host
>> because the granule will be set to 64kB which would be larger
>> than the guest page size. In that situation, the virtio-iommu
>> driver fails on viommu_domain_finalise() with
>> "granule 0x10000 larger than system page size 0x1000".
>>
>> In that case the workaround is to request 4K granule.
>>
>> The current limitation of global granule in the virtio-iommu
>> should be removed and turned into per domain granule. But
>> until we get this upgraded, this new default is probably
>> better because I don't think anyone is currently interested in
>> running a 4KB page size guest with virtio-iommu on a 64KB host.
>> However supporting 64kB guest on 64kB host with virtio-iommu and
>> VFIO looks a more important feature.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>   hw/core/machine.c        | 1 +
>>   hw/virtio/virtio-iommu.c | 2 +-
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 70ac96954c..38851df4b8 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -35,6 +35,7 @@
>>     GlobalProperty hw_compat_8_2[] = {
>>       { TYPE_VIRTIO_IOMMU_PCI, "aw-bits", "64" },
>> +    { TYPE_VIRTIO_IOMMU_PCI, "granule", "4K" },
>
> IIUC the current value is qemu_target_page_mask(), not 4KiB.
> Could this be an issue for arm / mips / mips / sparc64 guests?
The virtio-iommu currently is only supported on ARM and x86 where
qemu_target_page_mask() corresponds to a 4K granule. So I don't think it
makes any issue.

Thanks

Eric
>
>>   };
>>   const size_t hw_compat_8_2_len = G_N_ELEMENTS(hw_compat_8_2);
>>   diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>> index 0461b87ef2..e9e44a8ad8 100644
>> --- a/hw/virtio/virtio-iommu.c
>> +++ b/hw/virtio/virtio-iommu.c
>> @@ -1558,7 +1558,7 @@ static Property virtio_iommu_properties[] = {
>>       DEFINE_PROP_BOOL("boot-bypass", VirtIOIOMMU, boot_bypass, true),
>>       DEFINE_PROP_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 0),
>>       DEFINE_PROP_GRANULE_MODE("granule", VirtIOIOMMU, granule_mode,
>> -                             GRANULE_MODE_4K),
>> +                             GRANULE_MODE_HOST),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>