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
