| Message ID | 2e085c19aac78e6c4335eac4fffeb5cfca4bbb26.1272304746.git.mtosatti@redhat.com |
|---|---|
| State | New |
| Headers | show |
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; >
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; >
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 >
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 >> >>
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 >>> >>> > >
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.
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.
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.
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;
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(-)