Re: [Qemu-ppc] [PATCH 5/8] vfio: Add guest side IOMMU support

[Alexey Kardashevskiy]

On 08/19/2013 11:12 PM, Paolo Bonzini wrote:
> Il 15/08/2013 08:02, Alexey Kardashevskiy ha scritto:
>> On 08/13/2013 08:07 AM, Alex Williamson wrote:
>>>> +static void vfio_listener_region_add(MemoryListener *listener,
>>>> +                                     MemoryRegionSection *section)
>>>> +{
>>>> +    VFIOContainer *container = container_of(listener, VFIOContainer,
>>>> +                                            iommu_data.listener);
>>>> +    hwaddr iova, end;
>>>> +    int ret;
>>>>  
>>>>      if (vfio_listener_skipped_section(section)) {
>>>>          DPRINTF("SKIPPING region_add %"HWADDR_PRIx" - %"PRIx64"\n",
>>>> @@ -1952,19 +2011,51 @@ static void 
>>>> vfio_listener_region_add(MemoryListener *listener,
>>>>          return;
>>>>      }
>>>>  
>>>> -    vaddr = memory_region_get_ram_ptr(section->mr) +
>>>> -            section->offset_within_region +
>>>> -            (iova - section->offset_within_address_space);
>>>> -
>>>> -    DPRINTF("region_add %"HWADDR_PRIx" - %"HWADDR_PRIx" [%p]\n",
>>>> -            iova, end - 1, vaddr);
>>>> -
>>>> -    memory_region_ref(section->mr);
>>>> -    ret = vfio_dma_map(container, iova, end - iova, vaddr, 
>>>> section->readonly);
>>>> -    if (ret) {
>>>> -        error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
>>>> -                     "0x%"HWADDR_PRIx", %p) = %d (%m)",
>>>> -                     container, iova, end - iova, vaddr, ret);
>>>> +    if (memory_region_is_iommu(section->mr)) {
>>>> +        VFIOGuestIOMMU *giommu;
>>>> +
>>>> +        DPRINTF("region_add [iommu] %"HWADDR_PRIx" - %"HWADDR_PRIx"\n",
>>>> +                iova, end - 1);
>>>> +
>>>> +        memory_region_ref(section->mr);
>>>> +        /*
>>>> +         * FIXME: We should do some checking to see if the
>>>> +         * capabilities of the host VFIO IOMMU are adequate to model
>>>> +         * the guest IOMMU
>>>> +         *
>>>> +         * FIXME: This assumes that the guest IOMMU is empty of
>>>> +         * mappings at this point - we should either enforce this, or
>>>> +         * loop through existing mappings to map them into VFIO.
>>>> +         *
>>>> +         * FIXME: For VFIO iommu types which have KVM acceleration to
>>>> +         * avoid bouncing all map/unmaps through qemu this way, this
>>>> +         * would be the right place to wire that up (tell the KVM
>>>> +         * device emulation the VFIO iommu handles to use).
>>>> +         */
>>>> +        giommu = g_malloc0(sizeof(*giommu));
>>>> +        giommu->iommu = section->mr;
>>>> +        giommu->container = container;
>>>> +        giommu->n.notify = vfio_iommu_map_notify;
>>>> +        QLIST_INSERT_HEAD(&container->guest_iommus, giommu, list);
>>>> +        memory_region_register_iommu_notifier(giommu->iommu, &giommu->n);
>>>> +
>>>> +    } else if (memory_region_is_ram(section->mr)) {
> 
> Please change this to an "else", and leave the refs outside the if, just
> after checking for vfio_listener_skipped_section.

Why right after vfio_listener_skipped_section? The whole block looks now as:

===
static void vfio_listener_region_add(MemoryListener *listener,
                                     MemoryRegionSection *section)
{
    VFIOContainer *container = container_of(listener, VFIOContainer,
                                            iommu_data.listener);
    hwaddr iova, end;
    void *vaddr;
    int ret;

    if (vfio_listener_skipped_section(section)) {
        DPRINTF("SKIPPING region_add %"HWADDR_PRIx" - %"PRIx64"\n",
                section->offset_within_address_space,
                section->offset_within_address_space + section->size - 1);
        return;
    }

    if (unlikely((section->offset_within_address_space & ~TARGET_PAGE_MASK) !=
                 (section->offset_within_region & ~TARGET_PAGE_MASK))) {
        error_report("%s received unaligned region", __func__);
        return;
    }

    iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
    end = (section->offset_within_address_space +
int128_get64(section->size)) &
          TARGET_PAGE_MASK;

    if (iova >= end) {
        return;
    }

    memory_region_ref(section->mr);
===

You want me to move memory_region_ref() earlier to add a reference even if
two last checks fail?


>  This way it's clearer
> that vfio_listener_region_add matches vfio_listener_region_del.

Yes, true, reworked. Thanks for comments!



-- 
Alexey