| Message ID | 20200311172014.33052-4-shameerali.kolothum.thodi@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ARM virt: Add NVDIMM support | expand |
On 11.03.20 18:20, Shameer Kolothum wrote: > From: David Hildenbrand <david@redhat.com> > > Summarizing the issue: > 1. Memory regions contain ram blocks with a different size, if the > size is not properly aligned. While memory regions can have an > unaligned size, ram blocks can't. This is true when creating > resizable memory region with an unaligned size. > 2. When resizing a ram block/memory region, the size of the memory > region is set to the aligned size. The callback is called with > the aligned size. The unaligned piece is lost. > > Because of the above, if ACPI blob length modifications happens > after the initial virt_acpi_build() call, and the changed blob > length is within the PAGE size boundary, then the revised size > is not seen by the firmware on Guest reboot. > > Hence make sure callback is called if memory region size is changed, > irrespective of aligned or not. > > Signed-off-by: David Hildenbrand <david@redhat.com> > [Shameer: added commit log] > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > --- > Please find the discussion here, > https://patchwork.kernel.org/patch/11339591/ > --- > exec.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/exec.c b/exec.c > index 0cc500d53a..f8974cd303 100644 > --- a/exec.c > +++ b/exec.c > @@ -2073,11 +2073,21 @@ static int memory_try_enable_merging(void *addr, size_t len) > */ > int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp) > { > + const ram_addr_t unaligned_size = newsize; > + > assert(block); > > newsize = HOST_PAGE_ALIGN(newsize); > > if (block->used_length == newsize) { > + /* > + * We don't have to resize the ram block (which only knows aligned > + * sizes), however, we have to notify if the unaligned size changed. > + */ > + if (block->resized && unaligned_size != memory_region_size(block->mr)) { > + block->resized(block->idstr, unaligned_size, block->host); > + memory_region_set_size(block->mr, unaligned_size); > + } I guess we should do if (unaligned_size != memory_region_size(block->mr)) { memory_region_set_size(block->mr, unaligned_size); if (block->resized) { block->resized(block->idstr, unaligned_size, block->host); } } Instead - like in the case below. Note: This is not completely clean, the RAM block code should'n have to care about unaligned stuff. Also, the resized() callback for the RAM block is ugly, it should be a resized callback for the memory region. But these things imply requires bigger refactorings, so I guess this is good and simple enough for now. -- Thanks, David / dhildenb
On Wed, 11 Mar 2020 17:20:07 +0000 Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote: > From: David Hildenbrand <david@redhat.com> > > Summarizing the issue: > 1. Memory regions contain ram blocks with a different size, if the > size is not properly aligned. While memory regions can have an > unaligned size, ram blocks can't. This is true when creating > resizable memory region with an unaligned size. > 2. When resizing a ram block/memory region, the size of the memory > region is set to the aligned size. The callback is called with > the aligned size. The unaligned piece is lost. > > Because of the above, if ACPI blob length modifications happens > after the initial virt_acpi_build() call, and the changed blob > length is within the PAGE size boundary, then the revised size > is not seen by the firmware on Guest reboot. > > Hence make sure callback is called if memory region size is changed, > irrespective of aligned or not. > > Signed-off-by: David Hildenbrand <david@redhat.com> > [Shameer: added commit log] > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > Please find the discussion here, > https://patchwork.kernel.org/patch/11339591/ > --- > exec.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/exec.c b/exec.c > index 0cc500d53a..f8974cd303 100644 > --- a/exec.c > +++ b/exec.c > @@ -2073,11 +2073,21 @@ static int memory_try_enable_merging(void *addr, size_t len) > */ > int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp) > { > + const ram_addr_t unaligned_size = newsize; > + > assert(block); > > newsize = HOST_PAGE_ALIGN(newsize); > > if (block->used_length == newsize) { > + /* > + * We don't have to resize the ram block (which only knows aligned > + * sizes), however, we have to notify if the unaligned size changed. > + */ > + if (block->resized && unaligned_size != memory_region_size(block->mr)) { > + block->resized(block->idstr, unaligned_size, block->host); > + memory_region_set_size(block->mr, unaligned_size); > + } > return 0; > } > > @@ -2101,9 +2111,9 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp) > block->used_length = newsize; > cpu_physical_memory_set_dirty_range(block->offset, block->used_length, > DIRTY_CLIENTS_ALL); > - memory_region_set_size(block->mr, newsize); > + memory_region_set_size(block->mr, unaligned_size); > if (block->resized) { > - block->resized(block->idstr, newsize, block->host); > + block->resized(block->idstr, unaligned_size, block->host); > } > return 0; > }
diff --git a/exec.c b/exec.c index 0cc500d53a..f8974cd303 100644 --- a/exec.c +++ b/exec.c @@ -2073,11 +2073,21 @@ static int memory_try_enable_merging(void *addr, size_t len) */ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp) { + const ram_addr_t unaligned_size = newsize; + assert(block); newsize = HOST_PAGE_ALIGN(newsize); if (block->used_length == newsize) { + /* + * We don't have to resize the ram block (which only knows aligned + * sizes), however, we have to notify if the unaligned size changed. + */ + if (block->resized && unaligned_size != memory_region_size(block->mr)) { + block->resized(block->idstr, unaligned_size, block->host); + memory_region_set_size(block->mr, unaligned_size); + } return 0; } @@ -2101,9 +2111,9 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp) block->used_length = newsize; cpu_physical_memory_set_dirty_range(block->offset, block->used_length, DIRTY_CLIENTS_ALL); - memory_region_set_size(block->mr, newsize); + memory_region_set_size(block->mr, unaligned_size); if (block->resized) { - block->resized(block->idstr, newsize, block->host); + block->resized(block->idstr, unaligned_size, block->host); } return 0; }