Re: [PATCH 5/7] virtio-iommu : Retrieve page size mask on virtio_iommu_set_iommu_device()

[Eric Auger]

Hi Zhenhong,

On 6/27/24 13:32, Duan, Zhenzhong wrote:
> Hi Eric,
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Subject: [PATCH 5/7] virtio-iommu : Retrieve page size mask on
>> virtio_iommu_set_iommu_device()
>>
>> Retrieve the Host IOMMU Device page size mask when this latter is set.
>> This allows to get the information much sooner than when relying on
>> IOMMU MR set_page_size_mask() call, whcih happens when the IOMMU
>> MR
>> gets enabled. We introduce check_page_size_mask() helper whose code
>> is inherited from current virtio_iommu_set_page_size_mask()
>> implementation. This callback will be removed in a subsequent patch.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>> hw/virtio/virtio-iommu.c | 55
>> ++++++++++++++++++++++++++++++++++++++--
>> hw/virtio/trace-events   |  1 +
>> 2 files changed, 54 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>> index b8f75d2b1a..631589735a 100644
>> --- a/hw/virtio/virtio-iommu.c
>> +++ b/hw/virtio/virtio-iommu.c
>> @@ -598,9 +598,39 @@ out:
>>     return ret;
>> }
>>
>> +static bool check_page_size_mask(VirtIOIOMMU *viommu, uint64_t
>> new_mask,
>> +                                 Error **errp)
>> +{
>> +    uint64_t cur_mask = viommu->config.page_size_mask;
>> +
>> +    if ((cur_mask & new_mask) == 0) {
>> +        error_setg(errp, "virtio-iommu reports a page size mask 0x%"PRIx64
>> +                   " incompatible with currently supported mask 0x%"PRIx64,
>> +                   new_mask, cur_mask);
>> +        return false;
>> +    }
>> +    /*
>> +     * 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.
>> +     */
>> +    if (viommu->granule_frozen) {
>> +        int cur_granule = ctz64(cur_mask);
>> +
>> +        if (!(BIT_ULL(cur_granule) & new_mask)) {
>> +            error_setg(errp,
>> +                       "virtio-iommu does not support frozen granule 
>> 0x%llx",
>> +                       BIT_ULL(cur_granule));
>> +            return false;
>> +        }
>> +    }
>> +    return true;
>> +}
>> +
>> static bool virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque,
>> int devfn,
>>                                           HostIOMMUDevice *hiod, Error 
>> **errp)
>> {
>> +    ERRP_GUARD();
>>     VirtIOIOMMU *viommu = opaque;
>>     HostIOMMUDeviceClass *hiodc =
>> HOST_IOMMU_DEVICE_GET_CLASS(hiod);
>>     struct hiod_key *new_key;
>> @@ -623,8 +653,26 @@ static bool
>> virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
>>                                                 hiod->aliased_devfn,
>>                                                 host_iova_ranges, errp);
>>         if (ret) {
>> -            g_list_free_full(host_iova_ranges, g_free);
>> -            return false;
>> +            goto error;
>> +        }
>> +    }
>> +    if (hiodc->get_page_size_mask) {
>> +        uint64_t new_mask = hiodc->get_page_size_mask(hiod);
>> +
>> +        if (check_page_size_mask(viommu, new_mask, errp)) {
>> +            /*
>> +             * The default mask depends on the "granule" property. For 
>> example,
>> +             * with 4k granule, it is -(4 * KiB). When an assigned device 
>> has
>> +             * page size restrictions due to the hardware IOMMU 
>> configuration,
>> +             * apply this restriction to the mask.
>> +             */
>> +            trace_virtio_iommu_update_page_size_mask(hiod->name,
>> +                                                     
>> viommu->config.page_size_mask,
>> +                                                     new_mask);
>> +            viommu->config.page_size_mask &= new_mask;
> This is a bit different from original logic, it may update page_size_mask 
> after frozen.
> Will that make issue?
You're fully right. I need to replace

viommu->config.page_size_mask &= new_mask;
by
if (s->granule_frozen) {
    viommu->config.page_size_mask &= new_mask;
}    

> Except this question, for all other patches,
>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Thanks!

Eric
>
> Thanks
> Zhenzhong
>
>> +        } else {
>> +            error_prepend(errp, "%s: ", hiod->name);
>> +            goto error;
>>         }
>>     }
>>
>> @@ -637,6 +685,9 @@ static bool
>> virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
>>     g_list_free_full(host_iova_ranges, g_free);
>>
>>     return true;
>> +error:
>> +    g_list_free_full(host_iova_ranges, g_free);
>> +    return false;
>> }
>>
>> static void
>> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
>> index 3cf84e04a7..599d855ff6 100644
>> --- a/hw/virtio/trace-events
>> +++ b/hw/virtio/trace-events
>> @@ -132,6 +132,7 @@ virtio_iommu_notify_map(const char *name,
>> uint64_t virt_start, uint64_t virt_end
>> virtio_iommu_notify_unmap(const char *name, uint64_t virt_start,
>> uint64_t virt_end) "mr=%s virt_start=0x%"PRIx64" virt_end=0x%"PRIx64
>> virtio_iommu_remap(const char *name, uint64_t virt_start, uint64_t
>> virt_end, uint64_t phys_start) "mr=%s virt_start=0x%"PRIx64"
>> virt_end=0x%"PRIx64" phys_start=0x%"PRIx64
>> virtio_iommu_set_page_size_mask(const char *name, uint64_t old,
>> uint64_t new) "mr=%s old_mask=0x%"PRIx64" new_mask=0x%"PRIx64
>> +virtio_iommu_update_page_size_mask(const char *name, uint64_t old,
>> uint64_t new) "host iommu device=%s old_mask=0x%"PRIx64"
>> new_mask=0x%"PRIx64
>> virtio_iommu_notify_flag_add(const char *name) "add notifier to mr %s"
>> virtio_iommu_notify_flag_del(const char *name) "del notifier from mr %s"
>> virtio_iommu_switch_address_space(uint8_t bus, uint8_t slot, uint8_t fn,
>> bool on) "Device %02x:%02x.%x switching address space (iommu
>> enabled=%d)"
>> --
>> 2.41.0