Re: [EXT] Re: [PATCH v9 2/9] memory: Add interface to set iommu page size mask

[Auger Eric]

Hi Bharat,

On 3/27/20 6:33 AM, Bharat Bhushan wrote:
> Hi Eric,
> 
>> -----Original Message-----
>> From: Auger Eric <address@hidden>
>> Sent: Thursday, March 26, 2020 9:36 PM
>> To: Bharat Bhushan <address@hidden>; address@hidden;
>> address@hidden; address@hidden; address@hidden;
>> address@hidden; address@hidden; Tomasz Nowicki [C]
>> <address@hidden>; address@hidden; address@hidden; qemu-
>> address@hidden; address@hidden; address@hidden; jean-
>> address@hidden; address@hidden
>> Subject: [EXT] Re: [PATCH v9 2/9] memory: Add interface to set iommu page 
>> size
>> mask
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> Hi Bharat,
>> On 3/23/20 9:46 AM, Bharat Bhushan wrote:
>>> Allow to set page size mask to be supported by iommu.
>> by iommu memory region. I mean this is not global to the IOMMU.
> 
> Yes.
> 
>>> This is required to expose page size mask compatible with host with
>>> virtio-iommu.
>>>
>>> Signed-off-by: Bharat Bhushan <address@hidden>
>>> ---
>>>  include/exec/memory.h | 20 ++++++++++++++++++++
>>>  memory.c              | 10 ++++++++++
>>>  2 files changed, 30 insertions(+)
>>>
>>> diff --git a/include/exec/memory.h b/include/exec/memory.h index
>>> e85b7de99a..063c424854 100644
>>> --- a/include/exec/memory.h
>>> +++ b/include/exec/memory.h
>>> @@ -355,6 +355,16 @@ typedef struct IOMMUMemoryRegionClass {
>>>       * @iommu: the IOMMUMemoryRegion
>>>       */
>>>      int (*num_indexes)(IOMMUMemoryRegion *iommu);
>>> +
>>> +    /*
>>> +     * Set supported IOMMU page size
>>> +     *
>>> +     * Optional method: if this is supported then set page size that
>>> +     * can be supported by IOMMU. This is called to set supported page
>>> +     * size as per host Linux.
>> What about: If supported, allows to restrict the page size mask that can be
>> supported with a given IOMMU memory region. For example, this allows to
>> propagate host physical IOMMU page size mask limitations to the virtual IOMMU
>> (vfio assignment with virtual iommu).
> 
> Much better 
> 
>>> +     */
>>> +     void (*iommu_set_page_size_mask)(IOMMUMemoryRegion *iommu,
>>> +                                      uint64_t page_size_mask);
>>>  } IOMMUMemoryRegionClass;
>>>
>>>  typedef struct CoalescedMemoryRange CoalescedMemoryRange; @@ -1363,6
>>> +1373,16 @@ int
>> memory_region_iommu_attrs_to_index(IOMMUMemoryRegion *iommu_mr,
>>>   */
>>>  int memory_region_iommu_num_indexes(IOMMUMemoryRegion *iommu_mr);
>>>
>>> +/**
>>> + * memory_region_iommu_set_page_size_mask: set the supported pages
>>> + * size by iommu.
>> supported page sizes for a given IOMMU memory region
>>> + *
>>> + * @iommu_mr: the memory region
>> IOMMU memory region
>>> + * @page_size_mask: supported page size mask  */ void
>>> +memory_region_iommu_set_page_size_mask(IOMMUMemoryRegion
>> *iommu_mr,
>>> +                                            uint64_t page_size_mask);
>>> +
>>>  /**
>>>   * memory_region_name: get a memory region's name
>>>   *
>>> diff --git a/memory.c b/memory.c
>>> index aeaa8dcc9e..14c8783084 100644
>>> --- a/memory.c
>>> +++ b/memory.c
>>> @@ -1833,6 +1833,16 @@ static int
>> memory_region_update_iommu_notify_flags(IOMMUMemoryRegion
>> *iommu_mr,
>>>      return ret;
>>>  }
>>>
>>> +void memory_region_iommu_set_page_size_mask(IOMMUMemoryRegion
>> *iommu_mr,
>>> +                                            uint64_t page_size_mask)
>>> +{
>>> +    IOMMUMemoryRegionClass *imrc =
>>> +IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr);
>>> +
>>> +    if (imrc->iommu_set_page_size_mask) {
>>> +        imrc->iommu_set_page_size_mask(iommu_mr, page_size_mask);
>> Shouldn't it return an int in case the setting cannot be applied?
> 
> iommu_set_page_size_mask() is setting page-size-mask for endpoint. Below 
> function from code
> 
>       static void virtio_iommu_set_page_size_mask(IOMMUMemoryRegion *mr,
>                                                     uint64_t page_size_mask)
>       {
>               IOMMUDevice *sdev = container_of(mr, IOMMUDevice, iommu_mr);
> 
>                     sdev->page_size_mask = page_size_mask;
>       }
> 
> Do you see any reason it cannot be applied, am I missing something?
maybe the function can be called several times and one setting is
already in place. In other contexts, maybe the setting we try to apply
is not compatible with other settings enforced?

Thanks

Eric
> 
> Thanks
> -Bharat
> 
>>> +    }
>>> +}
>>> +
>>>  int memory_region_register_iommu_notifier(MemoryRegion *mr,
>>>                                            IOMMUNotifier *n, Error
>>> **errp)  {
>>>
>> Thanks
>> Eric
>