Re: [Qemu-devel] [PATCH qemu v18 4/5] vfio/spapr: Create DMA window dynamically (SPAPR IOMMU v2)

[Alexey Kardashevskiy]

On 23/06/16 00:38, Laurent Vivier wrote:
> 
> 
> On 21/06/2016 03:14, Alexey Kardashevskiy wrote:
>> New VFIO_SPAPR_TCE_v2_IOMMU type supports dynamic DMA window management.
>> This adds ability to VFIO common code to dynamically allocate/remove
>> DMA windows in the host kernel when new VFIO container is added/removed.
>>
>> This adds a helper to vfio_listener_region_add which makes
>> VFIO_IOMMU_SPAPR_TCE_CREATE ioctl and adds just created IOMMU into
>> the host IOMMU list; the opposite action is taken in
>> vfio_listener_region_del.
>>
>> When creating a new window, this uses heuristic to decide on the TCE table
>> levels number.
>>
>> This should cause no guest visible change in behavior.
>>
>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>> ---
>> Changes:
>> v18:
>> * moved trace definitions under hw/vfio/spapr.c section
>> * moved trace_vfio_spapr_remove_window to vfio_spapr_remove_window()
>> * vfio_host_win_del() now checks for exact window size
>> * one ctz() less in vfio_spapr_create_window()
>>
>> v17:
>> * moved spapr window create/remove helpers to separate file
>> * added hw_error() if vfio_host_win_del() failed
>>
>> v16:
>> * used memory_region_iommu_get_page_sizes() in vfio_listener_region_add()
>> * enforced no intersections between windows
>>
>> v14:
>> * new to the series
>> ---
>>  hw/vfio/common.c              | 79 
>> +++++++++++++++++++++++++++++++++++++------
>>  hw/vfio/spapr.c               | 71 ++++++++++++++++++++++++++++++++++++++
>>  include/hw/vfio/vfio-common.h |  6 ++++
>>  trace-events                  |  2 ++
>>  4 files changed, 148 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index b53a1db..8e3466c 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -265,6 +265,21 @@ static void vfio_host_win_add(VFIOContainer *container,
>>      QLIST_INSERT_HEAD(&container->hostwin_list, hostwin, hostwin_next);
>>  }
>>  
>> +static int vfio_host_win_del(VFIOContainer *container, hwaddr min_iova,
>> +                             hwaddr max_iova)
>> +{
>> +    VFIOHostDMAWindow *hostwin;
>> +
>> +    QLIST_FOREACH(hostwin, &container->hostwin_list, hostwin_next) {
>> +        if (hostwin->min_iova == min_iova && hostwin->max_iova == max_iova) 
>> {
>> +            QLIST_REMOVE(hostwin, hostwin_next);
>> +            return 0;
>> +        }
>> +    }
>> +
>> +    return -1;
>> +}
>> +
>>  static bool vfio_listener_skipped_section(MemoryRegionSection *section)
>>  {
>>      return (!memory_region_is_ram(section->mr) &&
>> @@ -380,6 +395,30 @@ static void vfio_listener_region_add(MemoryListener 
>> *listener,
>>      }
>>      end = int128_get64(int128_sub(llend, int128_one()));
>>  
>> +    if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
>> +        VFIOHostDMAWindow *hostwin;
>> +        hwaddr pgsize = 0;
>> +
>> +        /* For now intersections are not allowed, we may relax this later */
>> +        QLIST_FOREACH(hostwin, &container->hostwin_list, hostwin_next) {
>> +            if (ranges_overlap(hostwin->min_iova,
>> +                               hostwin->max_iova - hostwin->min_iova + 1,
>> +                               section->offset_within_address_space,
>> +                               int128_get64(section->size))) {
>> +                goto fail;
> 
> ret is not initialized and it is used in "fail:".
> 
> hw/vfio/common.c: In function ‘vfio_listener_region_add’:
> hw/vfio/common.c:493:30: error: ‘ret’ may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
>              container->error = ret;

Oh. Thanks for reporting. I use cross gcc and there must be something I am
doing wrong as I do not see these warnings but I do see them when compile
with native compiler...



-- 
Alexey