diff mbox

Re: [PATCH 10/10] introduce qemu_ram_map

Message ID 20100426185055.GG21425@amt.cnet
State New
Headers show

Commit Message

Marcelo Tosatti April 26, 2010, 6:50 p.m. UTC 
On Mon, Apr 26, 2010 at 01:29:06PM -0500, Anthony Liguori wrote:
> On 04/26/2010 12:59 PM, Marcelo Tosatti wrote:
> >Which allows drivers to register an mmaped region into ram block mappings.
> >To be used by device assignment driver.
> >
> 
> This is not kvm specific and not required by this pull request so it
> shouldn't really be part of the pull.  Something like this should
> only be added when there's an actual consumer.

The user will be hw/device-assignment.c in qemu-kvm. And also Cam has
the need for a similar interface for shared memory drivers.

Comments

Anthony Liguori April 26, 2010, 6:57 p.m. UTC | #1 
On 04/26/2010 01:50 PM, Marcelo Tosatti wrote:
> On Mon, Apr 26, 2010 at 01:29:06PM -0500, Anthony Liguori wrote:
>    
>> On 04/26/2010 12:59 PM, Marcelo Tosatti wrote:
>>      
>>> Which allows drivers to register an mmaped region into ram block mappings.
>>> To be used by device assignment driver.
>>>
>>>        
>> This is not kvm specific and not required by this pull request so it
>> shouldn't really be part of the pull.  Something like this should
>> only be added when there's an actual consumer.
>>      
> The user will be hw/device-assignment.c in qemu-kvm. And also Cam has
> the need for a similar interface for shared memory drivers.
>    

It should be part of one of those submissions.

> Index: qemu-kvm/hw/device-assignment.c
> ===================================================================
> --- qemu-kvm.orig/hw/device-assignment.c	2010-04-22 16:21:30.000000000 -0400
> +++ qemu-kvm/hw/device-assignment.c	2010-04-22 17:36:57.000000000 -0400
> @@ -256,10 +256,7 @@
>       AssignedDevice *r_dev = container_of(pci_dev, AssignedDevice, dev);
>       AssignedDevRegion *region =&r_dev->v_addrs[region_num];
>       PCIRegion *real_region =&r_dev->real_device.regions[region_num];
> -    pcibus_t old_ephys = region->e_physbase;
> -    pcibus_t old_esize = region->e_size;
> -    int first_map = (region->e_size == 0);
> -    int ret = 0;
> +    int ret = 0, flags = 0;
>
>       DEBUG("e_phys=%08" FMT_PCIBUS " r_virt=%p type=%d len=%08" FMT_PCIBUS " region_num=%d \n",
>             e_phys, region->u.r_virtbase, type, e_size, region_num);
> @@ -267,30 +264,22 @@
>       region->e_physbase = e_phys;
>       region->e_size = e_size;
>
> -    if (!first_map)
> -	kvm_destroy_phys_mem(kvm_context, old_ephys,
> -                             TARGET_PAGE_ALIGN(old_esize));
> -
>       if (e_size>  0) {
> +
> +        if (region_num == PCI_ROM_SLOT)
> +            flags |= IO_MEM_ROM;
> +
> +        cpu_register_physical_memory(e_phys, e_size, region->memory_index | flags);
> +
>           /* deal with MSI-X MMIO page */
>           if (real_region->base_addr<= r_dev->msix_table_addr&&
>                   real_region->base_addr + real_region->size>=
>                   r_dev->msix_table_addr) {
>               int offset = r_dev->msix_table_addr - real_region->base_addr;
> -            ret = munmap(region->u.r_virtbase + offset, TARGET_PAGE_SIZE);
> -            if (ret == 0)
> -                DEBUG("munmap done, virt_base 0x%p\n",
> -                        region->u.r_virtbase + offset);
> -            else {
> -                fprintf(stderr, "%s: fail munmap msix table!\n", __func__);
> -                exit(1);
> -            }
> +
>               cpu_register_physical_memory(e_phys + offset,
>                       TARGET_PAGE_SIZE, r_dev->mmio_index);
>           }
> -	ret = kvm_register_phys_mem(kvm_context, e_phys,
> -                                    region->u.r_virtbase,
> -                                    TARGET_PAGE_ALIGN(e_size), 0);
>       }
>
>       if (ret != 0) {
> @@ -539,6 +528,15 @@
>               pci_dev->v_addrs[i].u.r_virtbase +=
>                   (cur_region->base_addr&  0xFFF);
>
> +
> +            if (!slow_map) {
> +                void *virtbase = pci_dev->v_addrs[i].u.r_virtbase;
> +
> +                pci_dev->v_addrs[i].memory_index = qemu_ram_map(cur_region->size,
> +                                                                virtbase);
> +            } else
> +                pci_dev->v_addrs[i].memory_index = 0;
> +
>               pci_register_bar((PCIDevice *) pci_dev, i,
>                                cur_region->size, t,
>                                slow_map ? assigned_dev_iomem_map_slow
> @@ -726,10 +724,6 @@
>                   kvm_remove_ioperm_data(region->u.r_baseport, region->r_size);
>                   continue;
>               } else if (pci_region->type&  IORESOURCE_MEM) {
> -                if (region->e_size>  0)
> -                    kvm_destroy_phys_mem(kvm_context, region->e_physbase,
> -                                         TARGET_PAGE_ALIGN(region->e_size));
> -
>                   if (region->u.r_virtbase) {
>                       int ret = munmap(region->u.r_virtbase,
>                                        (pci_region->size + 0xFFF)&  0xFFFFF000);
>    

How does hot unplug get dealt with?

Regards,

Anthony Liguori

> Index: qemu-kvm/hw/device-assignment.h
> ===================================================================
> --- qemu-kvm.orig/hw/device-assignment.h	2010-04-22 16:21:30.000000000 -0400
> +++ qemu-kvm/hw/device-assignment.h	2010-04-22 16:24:32.000000000 -0400
> @@ -63,7 +63,7 @@
>
>   typedef struct {
>       pcibus_t e_physbase;
> -    uint32_t memory_index;
> +    ram_addr_t memory_index;
>       union {
>           void *r_virtbase;    /* mmapped access address for memory regions */
>           uint32_t r_baseport; /* the base guest port for I/O regions */
>
Marcelo Tosatti April 26, 2010, 7:14 p.m. UTC | #2 
On Mon, Apr 26, 2010 at 01:57:37PM -0500, Anthony Liguori wrote:
> On 04/26/2010 01:50 PM, Marcelo Tosatti wrote:
> >On Mon, Apr 26, 2010 at 01:29:06PM -0500, Anthony Liguori wrote:
> >>On 04/26/2010 12:59 PM, Marcelo Tosatti wrote:
> >>>Which allows drivers to register an mmaped region into ram block mappings.
> >>>To be used by device assignment driver.
> >>>
> >>This is not kvm specific and not required by this pull request so it
> >>shouldn't really be part of the pull.  Something like this should
> >>only be added when there's an actual consumer.
> >The user will be hw/device-assignment.c in qemu-kvm. And also Cam has
> >the need for a similar interface for shared memory drivers.
> 
> It should be part of one of those submissions.

OK

> >@@ -726,10 +724,6 @@
> >                  kvm_remove_ioperm_data(region->u.r_baseport, region->r_size);
> >                  continue;
> >              } else if (pci_region->type&  IORESOURCE_MEM) {
> >-                if (region->e_size>  0)
> >-                    kvm_destroy_phys_mem(kvm_context, region->e_physbase,
> >-                                         TARGET_PAGE_ALIGN(region->e_size));
> >-
> >                  if (region->u.r_virtbase) {
> >                      int ret = munmap(region->u.r_virtbase,
> >                                       (pci_region->size + 0xFFF)&  0xFFFFF000);
> 
> How does hot unplug get dealt with?

The regions will have such mappings unmapped from QEMU (and KVM) via
cpu_register_physical_memory(IO_MEM_UNASSIGNED) via
pci_unregister_io_regions.

Just pushed a new tree without the patch, please pull if you
are OK with the other changes.
Anthony Liguori April 26, 2010, 7:20 p.m. UTC | #3 
On 04/26/2010 02:14 PM, Marcelo Tosatti wrote:
> On Mon, Apr 26, 2010 at 01:57:37PM -0500, Anthony Liguori wrote:
>    
>> On 04/26/2010 01:50 PM, Marcelo Tosatti wrote:
>>      
>>> On Mon, Apr 26, 2010 at 01:29:06PM -0500, Anthony Liguori wrote:
>>>        
>>>> On 04/26/2010 12:59 PM, Marcelo Tosatti wrote:
>>>>          
>>>>> Which allows drivers to register an mmaped region into ram block mappings.
>>>>> To be used by device assignment driver.
>>>>>
>>>>>            
>>>> This is not kvm specific and not required by this pull request so it
>>>> shouldn't really be part of the pull.  Something like this should
>>>> only be added when there's an actual consumer.
>>>>          
>>> The user will be hw/device-assignment.c in qemu-kvm. And also Cam has
>>> the need for a similar interface for shared memory drivers.
>>>        
>> It should be part of one of those submissions.
>>      
> OK
>
>    
>>> @@ -726,10 +724,6 @@
>>>                   kvm_remove_ioperm_data(region->u.r_baseport, region->r_size);
>>>                   continue;
>>>               } else if (pci_region->type&   IORESOURCE_MEM) {
>>> -                if (region->e_size>   0)
>>> -                    kvm_destroy_phys_mem(kvm_context, region->e_physbase,
>>> -                                         TARGET_PAGE_ALIGN(region->e_size));
>>> -
>>>                   if (region->u.r_virtbase) {
>>>                       int ret = munmap(region->u.r_virtbase,
>>>                                        (pci_region->size + 0xFFF)&   0xFFFFF000);
>>>        
>> How does hot unplug get dealt with?
>>      
> The regions will have such mappings unmapped from QEMU (and KVM) via
> cpu_register_physical_memory(IO_MEM_UNASSIGNED) via
> pci_unregister_io_regions.
>    

But how do you qemu_ram_unmap()?  I see you munmap() that address but it 
looks like the qemu ram region gets leaked pointing to an invalid pointer.

Regards,

Anthony Liguori

> Just pushed a new tree without the patch, please pull if you
> are OK with the other changes.
>    

Yes, I am.
>
>
Marcelo Tosatti April 26, 2010, 7:45 p.m. UTC | #4 
On Mon, Apr 26, 2010 at 02:20:42PM -0500, Anthony Liguori wrote:
> On 04/26/2010 02:14 PM, Marcelo Tosatti wrote:
> >On Mon, Apr 26, 2010 at 01:57:37PM -0500, Anthony Liguori wrote:
> >>On 04/26/2010 01:50 PM, Marcelo Tosatti wrote:
> >>>On Mon, Apr 26, 2010 at 01:29:06PM -0500, Anthony Liguori wrote:
> >>>>On 04/26/2010 12:59 PM, Marcelo Tosatti wrote:
> >>>>>Which allows drivers to register an mmaped region into ram block mappings.
> >>>>>To be used by device assignment driver.
> >>>>>
> >>>>This is not kvm specific and not required by this pull request so it
> >>>>shouldn't really be part of the pull.  Something like this should
> >>>>only be added when there's an actual consumer.
> >>>The user will be hw/device-assignment.c in qemu-kvm. And also Cam has
> >>>the need for a similar interface for shared memory drivers.
> >>It should be part of one of those submissions.
> >OK
> >
> >>>@@ -726,10 +724,6 @@
> >>>                  kvm_remove_ioperm_data(region->u.r_baseport, region->r_size);
> >>>                  continue;
> >>>              } else if (pci_region->type&   IORESOURCE_MEM) {
> >>>-                if (region->e_size>   0)
> >>>-                    kvm_destroy_phys_mem(kvm_context, region->e_physbase,
> >>>-                                         TARGET_PAGE_ALIGN(region->e_size));
> >>>-
> >>>                  if (region->u.r_virtbase) {
> >>>                      int ret = munmap(region->u.r_virtbase,
> >>>                                       (pci_region->size + 0xFFF)&   0xFFFFF000);
> >>How does hot unplug get dealt with?
> >The regions will have such mappings unmapped from QEMU (and KVM) via
> >cpu_register_physical_memory(IO_MEM_UNASSIGNED) via
> >pci_unregister_io_regions.
> 
> But how do you qemu_ram_unmap()?  I see you munmap() that address
> but it looks like the qemu ram region gets leaked pointing to an
> invalid pointer.

Yes, qemu_ram_free() is not implemented. last_ram_offset always moves
forward. But there should be no references to the memory mapping
anymore, after the device is hot-unplugged.
diff mbox

Patch

Index: qemu-kvm/hw/device-assignment.c
===================================================================
--- qemu-kvm.orig/hw/device-assignment.c	2010-04-22 16:21:30.000000000 -0400
+++ qemu-kvm/hw/device-assignment.c	2010-04-22 17:36:57.000000000 -0400
@@ -256,10 +256,7 @@ 
     AssignedDevice *r_dev = container_of(pci_dev, AssignedDevice, dev);
     AssignedDevRegion *region = &r_dev->v_addrs[region_num];
     PCIRegion *real_region = &r_dev->real_device.regions[region_num];
-    pcibus_t old_ephys = region->e_physbase;
-    pcibus_t old_esize = region->e_size;
-    int first_map = (region->e_size == 0);
-    int ret = 0;
+    int ret = 0, flags = 0;
 
     DEBUG("e_phys=%08" FMT_PCIBUS " r_virt=%p type=%d len=%08" FMT_PCIBUS " region_num=%d \n",
           e_phys, region->u.r_virtbase, type, e_size, region_num);
@@ -267,30 +264,22 @@ 
     region->e_physbase = e_phys;
     region->e_size = e_size;
 
-    if (!first_map)
-	kvm_destroy_phys_mem(kvm_context, old_ephys,
-                             TARGET_PAGE_ALIGN(old_esize));
-
     if (e_size > 0) {
+
+        if (region_num == PCI_ROM_SLOT)
+            flags |= IO_MEM_ROM;
+
+        cpu_register_physical_memory(e_phys, e_size, region->memory_index | flags);
+
         /* deal with MSI-X MMIO page */
         if (real_region->base_addr <= r_dev->msix_table_addr &&
                 real_region->base_addr + real_region->size >=
                 r_dev->msix_table_addr) {
             int offset = r_dev->msix_table_addr - real_region->base_addr;
-            ret = munmap(region->u.r_virtbase + offset, TARGET_PAGE_SIZE);
-            if (ret == 0)
-                DEBUG("munmap done, virt_base 0x%p\n",
-                        region->u.r_virtbase + offset);
-            else {
-                fprintf(stderr, "%s: fail munmap msix table!\n", __func__);
-                exit(1);
-            }
+
             cpu_register_physical_memory(e_phys + offset,
                     TARGET_PAGE_SIZE, r_dev->mmio_index);
         }
-	ret = kvm_register_phys_mem(kvm_context, e_phys,
-                                    region->u.r_virtbase,
-                                    TARGET_PAGE_ALIGN(e_size), 0);
     }
 
     if (ret != 0) {
@@ -539,6 +528,15 @@ 
             pci_dev->v_addrs[i].u.r_virtbase +=
                 (cur_region->base_addr & 0xFFF);
 
+
+            if (!slow_map) {
+                void *virtbase = pci_dev->v_addrs[i].u.r_virtbase;
+
+                pci_dev->v_addrs[i].memory_index = qemu_ram_map(cur_region->size,
+                                                                virtbase);
+            } else
+                pci_dev->v_addrs[i].memory_index = 0;
+
             pci_register_bar((PCIDevice *) pci_dev, i,
                              cur_region->size, t,
                              slow_map ? assigned_dev_iomem_map_slow
@@ -726,10 +724,6 @@ 
                 kvm_remove_ioperm_data(region->u.r_baseport, region->r_size);
                 continue;
             } else if (pci_region->type & IORESOURCE_MEM) {
-                if (region->e_size > 0)
-                    kvm_destroy_phys_mem(kvm_context, region->e_physbase,
-                                         TARGET_PAGE_ALIGN(region->e_size));
-
                 if (region->u.r_virtbase) {
                     int ret = munmap(region->u.r_virtbase,
                                      (pci_region->size + 0xFFF) & 0xFFFFF000);
Index: qemu-kvm/hw/device-assignment.h
===================================================================
--- qemu-kvm.orig/hw/device-assignment.h	2010-04-22 16:21:30.000000000 -0400
+++ qemu-kvm/hw/device-assignment.h	2010-04-22 16:24:32.000000000 -0400
@@ -63,7 +63,7 @@ 
 
 typedef struct {
     pcibus_t e_physbase;
-    uint32_t memory_index;
+    ram_addr_t memory_index;
     union {
         void *r_virtbase;    /* mmapped access address for memory regions */
         uint32_t r_baseport; /* the base guest port for I/O regions */