Patchwork [10/10] introduce qemu_ram_map

login
register
mail settings
Submitter Marcelo Tosatti
Date April 26, 2010, 5:59 p.m.
Message ID <2e085c19aac78e6c4335eac4fffeb5cfca4bbb26.1272304746.git.mtosatti@redhat.com>
Download mbox | patch
Permalink /patch/50993/
State New
Headers show

Comments

Marcelo Tosatti - April 26, 2010, 5:59 p.m.
Which allows drivers to register an mmaped region into ram block mappings.
To be used by device assignment driver.

CC: Cam Macdonell <cam@cs.ualberta.ca>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
---
 cpu-common.h |    1 +
 exec.c       |   28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)
Anthony Liguori - April 26, 2010, 6:27 p.m.
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 doesn't make much sense to me.

Do you use this like:

qemu_ram_map(64k, ptr);
assert(qemu_ram_alloc(64k) == ptr);

If so, I think this is not the best API.  I'd rather see qemu_ram_map() 
register a symbolic name for the region and for there to be a 
qemu_ram_alloc() variant that allocated by name.

Regards,

Anthony Liguori

> CC: Cam Macdonell<cam@cs.ualberta.ca>
> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
> ---
>   cpu-common.h |    1 +
>   exec.c       |   28 ++++++++++++++++++++++++++++
>   2 files changed, 29 insertions(+), 0 deletions(-)
>
> diff --git a/cpu-common.h b/cpu-common.h
> index b24cecc..2dfde6f 100644
> --- a/cpu-common.h
> +++ b/cpu-common.h
> @@ -40,6 +40,7 @@ static inline void cpu_register_physical_memory(target_phys_addr_t start_addr,
>   }
>
>   ram_addr_t cpu_get_physical_page_desc(target_phys_addr_t addr);
> +ram_addr_t qemu_ram_map(ram_addr_t size, void *host);
>   ram_addr_t qemu_ram_alloc(ram_addr_t);
>   void qemu_ram_free(ram_addr_t addr);
>   /* This should only be used for ram local to a device.  */
> diff --git a/exec.c b/exec.c
> index 14d1fd7..648a9c9 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2789,6 +2789,34 @@ static void *file_ram_alloc(ram_addr_t memory, const char *path)
>   }
>   #endif
>
> +ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
> +{
> +    RAMBlock *new_block;
> +
> +    size = TARGET_PAGE_ALIGN(size);
> +    new_block = qemu_malloc(sizeof(*new_block));
> +
> +    new_block->host = host;
> +
> +    new_block->offset = last_ram_offset;
> +    new_block->length = size;
> +
> +    new_block->next = ram_blocks;
> +    ram_blocks = new_block;
> +
> +    phys_ram_dirty = qemu_realloc(phys_ram_dirty,
> +        (last_ram_offset + size)>>  TARGET_PAGE_BITS);
> +    memset(phys_ram_dirty + (last_ram_offset>>  TARGET_PAGE_BITS),
> +           0xff, size>>  TARGET_PAGE_BITS);
> +
> +    last_ram_offset += size;
> +
> +    if (kvm_enabled())
> +        kvm_setup_guest_memory(new_block->host, size);
> +
> +    return new_block->offset;
> +}
> +
>   ram_addr_t qemu_ram_alloc(ram_addr_t size)
>   {
>       RAMBlock *new_block;
>
Anthony Liguori - April 26, 2010, 6:29 p.m.
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.

Regards,

Anthony Liguori

> CC: Cam Macdonell<cam@cs.ualberta.ca>
> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
> ---
>   cpu-common.h |    1 +
>   exec.c       |   28 ++++++++++++++++++++++++++++
>   2 files changed, 29 insertions(+), 0 deletions(-)
>
> diff --git a/cpu-common.h b/cpu-common.h
> index b24cecc..2dfde6f 100644
> --- a/cpu-common.h
> +++ b/cpu-common.h
> @@ -40,6 +40,7 @@ static inline void cpu_register_physical_memory(target_phys_addr_t start_addr,
>   }
>
>   ram_addr_t cpu_get_physical_page_desc(target_phys_addr_t addr);
> +ram_addr_t qemu_ram_map(ram_addr_t size, void *host);
>   ram_addr_t qemu_ram_alloc(ram_addr_t);
>   void qemu_ram_free(ram_addr_t addr);
>   /* This should only be used for ram local to a device.  */
> diff --git a/exec.c b/exec.c
> index 14d1fd7..648a9c9 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2789,6 +2789,34 @@ static void *file_ram_alloc(ram_addr_t memory, const char *path)
>   }
>   #endif
>
> +ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
> +{
> +    RAMBlock *new_block;
> +
> +    size = TARGET_PAGE_ALIGN(size);
> +    new_block = qemu_malloc(sizeof(*new_block));
> +
> +    new_block->host = host;
> +
> +    new_block->offset = last_ram_offset;
> +    new_block->length = size;
> +
> +    new_block->next = ram_blocks;
> +    ram_blocks = new_block;
> +
> +    phys_ram_dirty = qemu_realloc(phys_ram_dirty,
> +        (last_ram_offset + size)>>  TARGET_PAGE_BITS);
> +    memset(phys_ram_dirty + (last_ram_offset>>  TARGET_PAGE_BITS),
> +           0xff, size>>  TARGET_PAGE_BITS);
> +
> +    last_ram_offset += size;
> +
> +    if (kvm_enabled())
> +        kvm_setup_guest_memory(new_block->host, size);
> +
> +    return new_block->offset;
> +}
> +
>   ram_addr_t qemu_ram_alloc(ram_addr_t size)
>   {
>       RAMBlock *new_block;
>
Marcelo Tosatti - April 26, 2010, 6:49 p.m.
On Mon, Apr 26, 2010 at 01:27:30PM -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 doesn't make much sense to me.
> 
> Do you use this like:
> 
> qemu_ram_map(64k, ptr);
> assert(qemu_ram_alloc(64k) == ptr);

No. hw/device-assignment.c in qemu-kvm mmaps
/sys/bus/pci/devices/x:y:z/resourcen (the PCI devices memory regions) to
the guest.

> If so, I think this is not the best API.  I'd rather see
> qemu_ram_map() register a symbolic name for the region and for there
> to be a qemu_ram_alloc() variant that allocated by name.
> 
> Regards,
> 
> Anthony Liguori
>
Anthony Liguori - April 26, 2010, 6:54 p.m.
On 04/26/2010 01:49 PM, Marcelo Tosatti wrote:
> On Mon, Apr 26, 2010 at 01:27:30PM -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 doesn't make much sense to me.
>>
>> Do you use this like:
>>
>> qemu_ram_map(64k, ptr);
>> assert(qemu_ram_alloc(64k) == ptr);
>>      
> No. hw/device-assignment.c in qemu-kvm mmaps
> /sys/bus/pci/devices/x:y:z/resourcen (the PCI devices memory regions) to
> the guest.
>    

I understand, but how do you use qemu_ram_map() to actually map that 
memory to a given PCI device resource?  I assume you rely on it getting 
put on the front of the list so that the next qemu_ram_alloc() will be 
at that location.

Regards,

Anthony Liguori

>> If so, I think this is not the best API.  I'd rather see
>> qemu_ram_map() register a symbolic name for the region and for there
>> to be a qemu_ram_alloc() variant that allocated by name.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>
Cam Macdonell - April 27, 2010, 2:28 p.m.
On Mon, Apr 26, 2010 at 12:54 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 04/26/2010 01:49 PM, Marcelo Tosatti wrote:
>>
>> On Mon, Apr 26, 2010 at 01:27:30PM -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 doesn't make much sense to me.
>>>
>>> Do you use this like:
>>>
>>> qemu_ram_map(64k, ptr);
>>> assert(qemu_ram_alloc(64k) == ptr);
>>>
>>
>> No. hw/device-assignment.c in qemu-kvm mmaps
>> /sys/bus/pci/devices/x:y:z/resourcen (the PCI devices memory regions) to
>> the guest.
>>
>
> I understand, but how do you use qemu_ram_map() to actually map that memory
> to a given PCI device resource?  I assume you rely on it getting put on the
> front of the list so that the next qemu_ram_alloc() will be at that
> location.

In my shared memory patch, I passed the offset returned from
qemu_ram_mmap to cpu_register_physical_memory from within the map
function passed to pci_register_bar.  Could the same not be done?  Is
there something incorrect with this approach?

>
> Regards,
>
> Anthony Liguori
>
>>> If so, I think this is not the best API.  I'd rather see
>>> qemu_ram_map() register a symbolic name for the region and for there
>>> to be a qemu_ram_alloc() variant that allocated by name.
>>>
>>> Regards,
>>>
>>> Anthony Liguori
>>>
>>>
>
>
Cam Macdonell - April 27, 2010, 2:32 p.m.
On Mon, Apr 26, 2010 at 11:59 AM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> Which allows drivers to register an mmaped region into ram block mappings.
> To be used by device assignment driver.
>
> CC: Cam Macdonell <cam@cs.ualberta.ca>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> ---
>  cpu-common.h |    1 +
>  exec.c       |   28 ++++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+), 0 deletions(-)
>
> diff --git a/cpu-common.h b/cpu-common.h
> index b24cecc..2dfde6f 100644
> --- a/cpu-common.h
> +++ b/cpu-common.h
> @@ -40,6 +40,7 @@ static inline void cpu_register_physical_memory(target_phys_addr_t start_addr,
>  }
>
>  ram_addr_t cpu_get_physical_page_desc(target_phys_addr_t addr);
> +ram_addr_t qemu_ram_map(ram_addr_t size, void *host);
>  ram_addr_t qemu_ram_alloc(ram_addr_t);
>  void qemu_ram_free(ram_addr_t addr);
>  /* This should only be used for ram local to a device.  */
> diff --git a/exec.c b/exec.c
> index 14d1fd7..648a9c9 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2789,6 +2789,34 @@ static void *file_ram_alloc(ram_addr_t memory, const char *path)
>  }
>  #endif
>
> +ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
> +{
> +    RAMBlock *new_block;
> +
> +    size = TARGET_PAGE_ALIGN(size);
> +    new_block = qemu_malloc(sizeof(*new_block));
> +
> +    new_block->host = host;
> +
> +    new_block->offset = last_ram_offset;
> +    new_block->length = size;
> +
> +    new_block->next = ram_blocks;
> +    ram_blocks = new_block;
> +
> +    phys_ram_dirty = qemu_realloc(phys_ram_dirty,
> +        (last_ram_offset + size) >> TARGET_PAGE_BITS);
> +    memset(phys_ram_dirty + (last_ram_offset >> TARGET_PAGE_BITS),
> +           0xff, size >> TARGET_PAGE_BITS);
> +
> +    last_ram_offset += size;
> +
> +    if (kvm_enabled())
> +        kvm_setup_guest_memory(new_block->host, size);
> +
> +    return new_block->offset;
> +}
> +
>  ram_addr_t qemu_ram_alloc(ram_addr_t size)
>  {
>     RAMBlock *new_block;
> --
> 1.6.6.1
>

Sorry for being late to reply, is there a strong reason not to have
the function handle the mmap itself?  As As Anthony points out, that
way we don't have worry about realloc changing the pointer in the
function.
Marcelo Tosatti - April 27, 2010, 2:49 p.m.
On Tue, Apr 27, 2010 at 08:28:15AM -0600, Cam Macdonell wrote:
> On Mon, Apr 26, 2010 at 12:54 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> > On 04/26/2010 01:49 PM, Marcelo Tosatti wrote:
> >>
> >> On Mon, Apr 26, 2010 at 01:27:30PM -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 doesn't make much sense to me.
> >>>
> >>> Do you use this like:
> >>>
> >>> qemu_ram_map(64k, ptr);
> >>> assert(qemu_ram_alloc(64k) == ptr);
> >>>
> >>
> >> No. hw/device-assignment.c in qemu-kvm mmaps
> >> /sys/bus/pci/devices/x:y:z/resourcen (the PCI devices memory regions) to
> >> the guest.
> >>
> >
> > I understand, but how do you use qemu_ram_map() to actually map that memory
> > to a given PCI device resource?  I assume you rely on it getting put on the
> > front of the list so that the next qemu_ram_alloc() will be at that
> > location.
> 
> In my shared memory patch, I passed the offset returned from
> qemu_ram_mmap to cpu_register_physical_memory from within the map
> function passed to pci_register_bar.  Could the same not be done?  Is
> there something incorrect with this approach?

No, its correct. Its similar to what hw/device-assignment.c in qemu-kvm 
will do.
Marcelo Tosatti - April 27, 2010, 2:50 p.m.
On Tue, Apr 27, 2010 at 08:32:27AM -0600, Cam Macdonell wrote:
> >
> > +ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
> > +{
> > +    RAMBlock *new_block;
> > +
> > +    size = TARGET_PAGE_ALIGN(size);
> > +    new_block = qemu_malloc(sizeof(*new_block));
> > +
> > +    new_block->host = host;
> > +
> > +    new_block->offset = last_ram_offset;
> > +    new_block->length = size;
> > +
> > +    new_block->next = ram_blocks;
> > +    ram_blocks = new_block;
> > +
> > +    phys_ram_dirty = qemu_realloc(phys_ram_dirty,
> > +        (last_ram_offset + size) >> TARGET_PAGE_BITS);
> > +    memset(phys_ram_dirty + (last_ram_offset >> TARGET_PAGE_BITS),
> > +           0xff, size >> TARGET_PAGE_BITS);
> > +
> > +    last_ram_offset += size;
> > +
> > +    if (kvm_enabled())
> > +        kvm_setup_guest_memory(new_block->host, size);
> > +
> > +    return new_block->offset;
> > +}
> > +
> >  ram_addr_t qemu_ram_alloc(ram_addr_t size)
> >  {
> >     RAMBlock *new_block;
> > --
> > 1.6.6.1
> >
> 
> Sorry for being late to reply, is there a strong reason not to have
> the function handle the mmap itself?  As As Anthony points out, that
> way we don't have worry about realloc changing the pointer in the
> function.

The caller might want a different protection for the memory map.

Patch

diff --git a/cpu-common.h b/cpu-common.h
index b24cecc..2dfde6f 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -40,6 +40,7 @@  static inline void cpu_register_physical_memory(target_phys_addr_t start_addr,
 }
 
 ram_addr_t cpu_get_physical_page_desc(target_phys_addr_t addr);
+ram_addr_t qemu_ram_map(ram_addr_t size, void *host);
 ram_addr_t qemu_ram_alloc(ram_addr_t);
 void qemu_ram_free(ram_addr_t addr);
 /* This should only be used for ram local to a device.  */
diff --git a/exec.c b/exec.c
index 14d1fd7..648a9c9 100644
--- a/exec.c
+++ b/exec.c
@@ -2789,6 +2789,34 @@  static void *file_ram_alloc(ram_addr_t memory, const char *path)
 }
 #endif
 
+ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
+{
+    RAMBlock *new_block;
+
+    size = TARGET_PAGE_ALIGN(size);
+    new_block = qemu_malloc(sizeof(*new_block));
+
+    new_block->host = host;
+
+    new_block->offset = last_ram_offset;
+    new_block->length = size;
+
+    new_block->next = ram_blocks;
+    ram_blocks = new_block;
+
+    phys_ram_dirty = qemu_realloc(phys_ram_dirty,
+        (last_ram_offset + size) >> TARGET_PAGE_BITS);
+    memset(phys_ram_dirty + (last_ram_offset >> TARGET_PAGE_BITS),
+           0xff, size >> TARGET_PAGE_BITS);
+
+    last_ram_offset += size;
+
+    if (kvm_enabled())
+        kvm_setup_guest_memory(new_block->host, size);
+
+    return new_block->offset;
+}
+
 ram_addr_t qemu_ram_alloc(ram_addr_t size)
 {
     RAMBlock *new_block;