Re: [PATCH 2/2] virtio-iommu: Rework the trace in virtio_iommu_set_page_size_mask()

[Eric Auger]

Hi Jean,

On 7/6/23 16:35, Jean-Philippe Brucker wrote:
> On Wed, Jul 05, 2023 at 03:16:31PM +0200, Eric Auger wrote:
>>>>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c index
>>>>> 1eaf81bab5..0d9f7196fe 100644
>>>>> --- a/hw/virtio/virtio-iommu.c
>>>>> +++ b/hw/virtio/virtio-iommu.c
>>>>> @@ -1101,29 +1101,24 @@ static int
>>>>> virtio_iommu_set_page_size_mask(IOMMUMemoryRegion *mr,
>>>>>                                           new_mask);
>>>>>
>>>>>     if ((cur_mask & new_mask) == 0) {
>>>>> -        error_setg(errp, "virtio-iommu page mask 0x%"PRIx64
>>>>> -                   " is incompatible with mask 0x%"PRIx64, cur_mask, 
>>>>> new_mask);
>>>>> +        error_setg(errp, "virtio-iommu %s reports a page size mask 
>>>>> 0x%"PRIx64
>>>>> +                   " incompatible with currently supported mask 
>>>>> 0x%"PRIx64,
>>>>> +                   mr->parent_obj.name, new_mask, cur_mask);
>>>>>         return -1;
>>>>>     }
>>>>>
>>>>>     /*
>>>>>      * Once the granule is frozen we can't change the mask anymore. If by
>>>>>      * chance the hotplugged device supports the same granule, we can 
>>>>> still
>>>>> -     * accept it. Having a different masks is possible but the guest 
>>>>> will use
>>>>> -     * sub-optimal block sizes, so warn about it.
>>>>> +     * accept it.
>>>>>      */
>>>>>     if (s->granule_frozen) {
>>>>> -        int new_granule = ctz64(new_mask);
>>>>>         int cur_granule = ctz64(cur_mask);
>>>>>
>>>>> -        if (new_granule != cur_granule) {
>>>>> -            error_setg(errp, "virtio-iommu page mask 0x%"PRIx64
>>>>> -                       " is incompatible with mask 0x%"PRIx64, cur_mask,
>>>>> -                       new_mask);
>>>>> +        if (!(BIT(cur_granule) & new_mask)) {
>>> Sorry, I read this piece code again and got a question, if new_mask has 
>>> finer
>>> granularity than cur_granule, should we allow it to pass even though
>>> BIT(cur_granule) is not set?
>> I think this should work but this is not straightforward to test.
>> virtio-iommu would use the current granule for map/unmap. In map/unmap
>> notifiers, this is split into pow2 ranges and cascaded to VFIO through
>> vfio_dma_map/unmap. The iova and size are aligned with the smaller
>> supported granule.
>>
>> Jean, do you share this understanding or do I miss something.
> Yes, I also think that would work. The guest would only issue mappings
> with the larger granularity, which can be applied by VFIO with a finer
> granule. However I doubt we're going to encounter this case, because
> seeing a cur_granule larger than 4k here means that a VFIO device has
> already been assigned with a large granule like 64k, and we're trying to
> add a new device with 4k. This indicates two HW IOMMUs supporting
> different granules in the same system, which seems unlikely.

agreed
>
> Hopefully by the time we actually need this (if ever) we will support
> per-endpoint probe properties, which allow informing the guest about
> different hardware properties instead of relying on one global property in
> the virtio config.
yes looking forward to reviewing that.

Thanks

Eric
>
> Thanks,
> Jean
>