* [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-01-17 17:45 [PATCH v2 0/7] ARM virt: Add NVDIMM support Shameer Kolothum
@ 2020-01-17 17:45 ` Shameer Kolothum
2020-02-04 15:23 ` Igor Mammedov
2020-01-17 17:45 ` [PATCH v2 2/7] hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length Shameer Kolothum
` (6 subsequent siblings)
7 siblings, 1 reply; 45+ messages in thread
From: Shameer Kolothum @ 2020-01-17 17:45 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, xiaoguangrong.eric, mst, linuxarm, xuwei5,
shannon.zhaosl, lersek
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. The is because in the
virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
path, qemu_ram_resize() uses used_length (ram_block size which is
aligned to PAGE size) and the "resize callback" to update the size
seen by firmware is not getting invoked.
Hence make sure callback is called if the new size is different
from original requested size.
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
Please find the previous discussions on this issue here,
https://patchwork.kernel.org/patch/11174947/
But this one attempts a different solution to fix it by introducing
req_length var to RAMBlock struct.
---
exec.c | 36 +++++++++++++++++++++++-------------
include/exec/ram_addr.h | 5 +++--
2 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/exec.c b/exec.c
index d4b769d0d4..9ce33992f8 100644
--- a/exec.c
+++ b/exec.c
@@ -2123,16 +2123,18 @@ static int memory_try_enable_merging(void *addr, size_t len)
* resize callback to update device state and/or add assertions to detect
* misuse, if necessary.
*/
-int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
+int qemu_ram_resize(RAMBlock *block, ram_addr_t size, Error **errp)
{
- assert(block);
+ ram_addr_t newsize;
- newsize = HOST_PAGE_ALIGN(newsize);
+ assert(block);
- if (block->used_length == newsize) {
+ if (block->req_length == size) {
return 0;
}
+ newsize = HOST_PAGE_ALIGN(size);
+
if (!(block->flags & RAM_RESIZEABLE)) {
error_setg_errno(errp, EINVAL,
"Length mismatch: %s: 0x" RAM_ADDR_FMT
@@ -2149,13 +2151,19 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
return -EINVAL;
}
- cpu_physical_memory_clear_dirty_range(block->offset, block->used_length);
- 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);
+ block->req_length = size;
+
+ if (newsize != block->used_length) {
+ cpu_physical_memory_clear_dirty_range(block->offset,
+ block->used_length);
+ 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);
+ }
+
if (block->resized) {
- block->resized(block->idstr, newsize, block->host);
+ block->resized(block->idstr, block->req_length, block->host);
}
return 0;
}
@@ -2412,16 +2420,18 @@ RAMBlock *qemu_ram_alloc_internal(ram_addr_t size, ram_addr_t max_size,
MemoryRegion *mr, Error **errp)
{
RAMBlock *new_block;
+ ram_addr_t newsize;
Error *local_err = NULL;
- size = HOST_PAGE_ALIGN(size);
+ newsize = HOST_PAGE_ALIGN(size);
max_size = HOST_PAGE_ALIGN(max_size);
new_block = g_malloc0(sizeof(*new_block));
new_block->mr = mr;
new_block->resized = resized;
- new_block->used_length = size;
+ new_block->req_length = size;
+ new_block->used_length = newsize;
new_block->max_length = max_size;
- assert(max_size >= size);
+ assert(max_size >= newsize);
new_block->fd = -1;
new_block->page_size = qemu_real_host_page_size;
new_block->host = host;
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 5adebb0bc7..fd13082224 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -31,8 +31,9 @@ struct RAMBlock {
uint8_t *host;
uint8_t *colo_cache; /* For colo, VM's ram cache */
ram_addr_t offset;
- ram_addr_t used_length;
- ram_addr_t max_length;
+ ram_addr_t req_length; /* Original requested size, used if RAM_RESIZEABLE */
+ ram_addr_t used_length; /* aligned to qemu_host_page_size */
+ ram_addr_t max_length; /* aligned to qemu_host_page_size */
void (*resized)(const char*, uint64_t length, void *host);
uint32_t flags;
/* Protected by iothread lock. */
--
2.17.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-01-17 17:45 ` [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback Shameer Kolothum
@ 2020-02-04 15:23 ` Igor Mammedov
2020-02-04 16:44 ` David Hildenbrand
0 siblings, 1 reply; 45+ messages in thread
From: Igor Mammedov @ 2020-02-04 15:23 UTC (permalink / raw)
To: Shameer Kolothum
Cc: peter.maydell, xiaoguangrong.eric, mst, shannon.zhaosl,
David Hildenbrand, qemu-devel, xuwei5, linuxarm, eric.auger,
qemu-arm, lersek
On Fri, 17 Jan 2020 17:45:16 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> 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. The is because in the
> virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
> path, qemu_ram_resize() uses used_length (ram_block size which is
> aligned to PAGE size) and the "resize callback" to update the size
> seen by firmware is not getting invoked.
>
> Hence make sure callback is called if the new size is different
> from original requested size.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> Please find the previous discussions on this issue here,
> https://patchwork.kernel.org/patch/11174947/
>
> But this one attempts a different solution to fix it by introducing
> req_length var to RAMBlock struct.
>
looks fine to me, so
Acked-by: Igor Mammedov <imammedo@redhat.com>
CCing David who touches this area in his latest series for and
might have an opinion on how it should be handled.
> ---
> exec.c | 36 +++++++++++++++++++++++-------------
> include/exec/ram_addr.h | 5 +++--
> 2 files changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index d4b769d0d4..9ce33992f8 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2123,16 +2123,18 @@ static int memory_try_enable_merging(void *addr, size_t len)
> * resize callback to update device state and/or add assertions to detect
> * misuse, if necessary.
> */
> -int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
> +int qemu_ram_resize(RAMBlock *block, ram_addr_t size, Error **errp)
> {
> - assert(block);
> + ram_addr_t newsize;
>
> - newsize = HOST_PAGE_ALIGN(newsize);
> + assert(block);
>
> - if (block->used_length == newsize) {
> + if (block->req_length == size) {
> return 0;
> }
>
> + newsize = HOST_PAGE_ALIGN(size);
> +
> if (!(block->flags & RAM_RESIZEABLE)) {
> error_setg_errno(errp, EINVAL,
> "Length mismatch: %s: 0x" RAM_ADDR_FMT
> @@ -2149,13 +2151,19 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
> return -EINVAL;
> }
>
> - cpu_physical_memory_clear_dirty_range(block->offset, block->used_length);
> - 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);
> + block->req_length = size;
> +
> + if (newsize != block->used_length) {
> + cpu_physical_memory_clear_dirty_range(block->offset,
> + block->used_length);
> + 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);
> + }
> +
> if (block->resized) {
> - block->resized(block->idstr, newsize, block->host);
> + block->resized(block->idstr, block->req_length, block->host);
> }
> return 0;
> }
> @@ -2412,16 +2420,18 @@ RAMBlock *qemu_ram_alloc_internal(ram_addr_t size, ram_addr_t max_size,
> MemoryRegion *mr, Error **errp)
> {
> RAMBlock *new_block;
> + ram_addr_t newsize;
> Error *local_err = NULL;
>
> - size = HOST_PAGE_ALIGN(size);
> + newsize = HOST_PAGE_ALIGN(size);
> max_size = HOST_PAGE_ALIGN(max_size);
> new_block = g_malloc0(sizeof(*new_block));
> new_block->mr = mr;
> new_block->resized = resized;
> - new_block->used_length = size;
> + new_block->req_length = size;
> + new_block->used_length = newsize;
> new_block->max_length = max_size;
> - assert(max_size >= size);
> + assert(max_size >= newsize);
> new_block->fd = -1;
> new_block->page_size = qemu_real_host_page_size;
> new_block->host = host;
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 5adebb0bc7..fd13082224 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -31,8 +31,9 @@ struct RAMBlock {
> uint8_t *host;
> uint8_t *colo_cache; /* For colo, VM's ram cache */
> ram_addr_t offset;
> - ram_addr_t used_length;
> - ram_addr_t max_length;
> + ram_addr_t req_length; /* Original requested size, used if RAM_RESIZEABLE */
> + ram_addr_t used_length; /* aligned to qemu_host_page_size */
> + ram_addr_t max_length; /* aligned to qemu_host_page_size */
> void (*resized)(const char*, uint64_t length, void *host);
> uint32_t flags;
> /* Protected by iothread lock. */
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-04 15:23 ` Igor Mammedov
@ 2020-02-04 16:44 ` David Hildenbrand
2020-02-04 19:05 ` David Hildenbrand
0 siblings, 1 reply; 45+ messages in thread
From: David Hildenbrand @ 2020-02-04 16:44 UTC (permalink / raw)
To: Igor Mammedov, Shameer Kolothum
Cc: peter.maydell, xiaoguangrong.eric, mst, shannon.zhaosl,
qemu-devel, xuwei5, linuxarm, eric.auger, qemu-arm, lersek
On 04.02.20 16:23, Igor Mammedov wrote:
> On Fri, 17 Jan 2020 17:45:16 +0000
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
>
>> 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. The is because in the
>> virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
>> path, qemu_ram_resize() uses used_length (ram_block size which is
>> aligned to PAGE size) and the "resize callback" to update the size
>> seen by firmware is not getting invoked.
>>
>> Hence make sure callback is called if the new size is different
>> from original requested size.
>>
>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>> ---
>> Please find the previous discussions on this issue here,
>> https://patchwork.kernel.org/patch/11174947/
>>
>> But this one attempts a different solution to fix it by introducing
>> req_length var to RAMBlock struct.
>>
>
> looks fine to me, so
> Acked-by: Igor Mammedov <imammedo@redhat.com>
Thanks for CCing.
This in fact collides with my changes ... but not severely :)
>
> CCing David who touches this area in his latest series for and
> might have an opinion on how it should be handled.
>
So we are talking about sub-page size changes? I somewhat dislike
storing "req_length" in ram blocks. Looks like sub-pages stuff does not
belong there.
Ram blocks only operate on page granularity. Ram block notifiers only
operate on page granularity. Memory regions only operate on page
granularity. Dirty bitmaps operate on page granularity. Especially,
memory_region_size(mr) will always return aligned values.
I think users/owner should deal with anything smaller manually if
they really need it.
What about always calling the resized() callback and letting the
actual owner figure out if the size changed on sub-page granularity
or not? (by looking up the size manually using some mechanism not glued to
memory regions/ram blocks/whatever)
diff --git a/exec.c b/exec.c
index 67e520d18e..59d46cc388 100644
--- a/exec.c
+++ b/exec.c
@@ -2130,6 +2130,13 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
newsize = HOST_PAGE_ALIGN(newsize);
if (block->used_length == newsize) {
+ /*
+ * The owner might want to handle sub-page resizes. We only provide
+ * the aligned size - because ram blocks are always page aligned.
+ */
+ if (block->resized) {
+ block->resized(block->idstr, newsize, block->host);
+ }
return 0;
}
--
Thanks,
David / dhildenb
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-04 16:44 ` David Hildenbrand
@ 2020-02-04 19:05 ` David Hildenbrand
2020-02-05 16:29 ` Shameerali Kolothum Thodi
0 siblings, 1 reply; 45+ messages in thread
From: David Hildenbrand @ 2020-02-04 19:05 UTC (permalink / raw)
To: Igor Mammedov, Shameer Kolothum
Cc: peter.maydell, xiaoguangrong.eric, mst, shannon.zhaosl,
qemu-devel, xuwei5, linuxarm, eric.auger, qemu-arm, lersek
On 04.02.20 17:44, David Hildenbrand wrote:
> On 04.02.20 16:23, Igor Mammedov wrote:
>> On Fri, 17 Jan 2020 17:45:16 +0000
>> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
>>
>>> 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. The is because in the
>>> virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
>>> path, qemu_ram_resize() uses used_length (ram_block size which is
>>> aligned to PAGE size) and the "resize callback" to update the size
>>> seen by firmware is not getting invoked.
>>>
>>> Hence make sure callback is called if the new size is different
>>> from original requested size.
>>>
>>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>>> ---
>>> Please find the previous discussions on this issue here,
>>> https://patchwork.kernel.org/patch/11174947/
>>>
>>> But this one attempts a different solution to fix it by introducing
>>> req_length var to RAMBlock struct.
>>>
>>
>> looks fine to me, so
>> Acked-by: Igor Mammedov <imammedo@redhat.com>
>
> Thanks for CCing.
>
> This in fact collides with my changes ... but not severely :)
>
>>
>> CCing David who touches this area in his latest series for and
>> might have an opinion on how it should be handled.
>>
>
> So we are talking about sub-page size changes? I somewhat dislike
> storing "req_length" in ram blocks. Looks like sub-pages stuff does not
> belong there.
>
> Ram blocks only operate on page granularity. Ram block notifiers only
> operate on page granularity. Memory regions only operate on page
> granularity. Dirty bitmaps operate on page granularity. Especially,
> memory_region_size(mr) will always return aligned values.
>
> I think users/owner should deal with anything smaller manually if
> they really need it.
>
> What about always calling the resized() callback and letting the
> actual owner figure out if the size changed on sub-page granularity
> or not? (by looking up the size manually using some mechanism not glued to
> memory regions/ram blocks/whatever)
>
> diff --git a/exec.c b/exec.c
> index 67e520d18e..59d46cc388 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2130,6 +2130,13 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
> newsize = HOST_PAGE_ALIGN(newsize);
>
> if (block->used_length == newsize) {
> + /*
> + * The owner might want to handle sub-page resizes. We only provide
> + * the aligned size - because ram blocks are always page aligned.
> + */
> + if (block->resized) {
> + block->resized(block->idstr, newsize, block->host);
> + }
> return 0;
> }
>
Oh, and one more reason why the proposal in this patch is inconsistent:
When migrating resizable memory regions (RAM_SAVE_FLAG_MEM_SIZE) we
store the block->used_length (ram_save_setup()) and use that value to
resize the region on the target (ram_load_precopy() -> qemu_ram_resize()).
This will be the value the callback will be called with. Page aligned.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread* RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-04 19:05 ` David Hildenbrand
@ 2020-02-05 16:29 ` Shameerali Kolothum Thodi
2020-02-05 16:40 ` David Hildenbrand
0 siblings, 1 reply; 45+ messages in thread
From: Shameerali Kolothum Thodi @ 2020-02-05 16:29 UTC (permalink / raw)
To: David Hildenbrand, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger
Hi David,
> -----Original Message-----
> From: Qemu-devel
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> u.org] On Behalf Of David Hildenbrand
> Sent: 04 February 2020 19:05
> To: Igor Mammedov <imammedo@redhat.com>; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>
> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com
> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
>
> On 04.02.20 17:44, David Hildenbrand wrote:
> > On 04.02.20 16:23, Igor Mammedov wrote:
> >> On Fri, 17 Jan 2020 17:45:16 +0000
> >> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> >>
> >>> 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. The is because in the
> >>> virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
> >>> path, qemu_ram_resize() uses used_length (ram_block size which is
> >>> aligned to PAGE size) and the "resize callback" to update the size
> >>> seen by firmware is not getting invoked.
> >>>
> >>> Hence make sure callback is called if the new size is different
> >>> from original requested size.
> >>>
> >>> Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> >>> ---
> >>> Please find the previous discussions on this issue here,
> >>> https://patchwork.kernel.org/patch/11174947/
> >>>
> >>> But this one attempts a different solution to fix it by introducing
> >>> req_length var to RAMBlock struct.
> >>>
> >>
> >> looks fine to me, so
> >> Acked-by: Igor Mammedov <imammedo@redhat.com>
> >
> > Thanks for CCing.
> >
> > This in fact collides with my changes ... but not severely :)
> >
> >>
> >> CCing David who touches this area in his latest series for and
> >> might have an opinion on how it should be handled.
> >>
> >
> > So we are talking about sub-page size changes? I somewhat dislike
> > storing "req_length" in ram blocks. Looks like sub-pages stuff does not
> > belong there.
Thanks for taking a look at this. Agree, I didn’t like that "req_length" either.
> > Ram blocks only operate on page granularity. Ram block notifiers only
> > operate on page granularity. Memory regions only operate on page
> > granularity. Dirty bitmaps operate on page granularity. Especially,
> > memory_region_size(mr) will always return aligned values.
> >
> > I think users/owner should deal with anything smaller manually if
> > they really need it.
> >
> > What about always calling the resized() callback and letting the
> > actual owner figure out if the size changed on sub-page granularity
> > or not? (by looking up the size manually using some mechanism not glued to
> > memory regions/ram blocks/whatever)
> >
> > diff --git a/exec.c b/exec.c
> > index 67e520d18e..59d46cc388 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -2130,6 +2130,13 @@ int qemu_ram_resize(RAMBlock *block,
> ram_addr_t newsize, Error **errp)
> > newsize = HOST_PAGE_ALIGN(newsize);
> >
> > if (block->used_length == newsize) {
> > + /*
> > + * The owner might want to handle sub-page resizes. We only
> provide
> > + * the aligned size - because ram blocks are always page aligned.
> > + */
> > + if (block->resized) {
> > + block->resized(block->idstr, newsize, block->host);
Does it make sense to pass the requested size in the callback than the aligned size
as the owner might be interested more in the org_req_size vs new_req _size case?
> > + }
> > return 0;
> > }
> >
> Oh, and one more reason why the proposal in this patch is inconsistent:
>
> When migrating resizable memory regions (RAM_SAVE_FLAG_MEM_SIZE) we
> store the block->used_length (ram_save_setup()) and use that value to
> resize the region on the target (ram_load_precopy() -> qemu_ram_resize()).
>
> This will be the value the callback will be called with. Page aligned.
>
Sorry, I didn’t quite get that point and not sure how "req_length" approach
will affect the migration.
Anyway, I have reworked the patch(below) with the above suggestion, that is
always calling the resized() callback, but modified it to pass the requested size
instead of the page aligned size. Please let me know your feedback.
Thanks,
Shameer
diff --git a/exec.c b/exec.c
index 67e520d18e..c9cb9a54fa 100644
--- a/exec.c
+++ b/exec.c
@@ -2123,13 +2123,22 @@ static int memory_try_enable_merging(void *addr, size_t len)
* resize callback to update device state and/or add assertions to detect
* misuse, if necessary.
*/
-int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
+int qemu_ram_resize(RAMBlock *block, ram_addr_t size, Error **errp)
{
+ ram_addr_t newsize;
assert(block);
- newsize = HOST_PAGE_ALIGN(newsize);
+ newsize = HOST_PAGE_ALIGN(size);
if (block->used_length == newsize) {
+ /*
+ * RAM blocks are always page aligned, but the owner might want
+ * to handle sub-page resizes. Let the owner know about any
+ * sub-page changes.
+ */
+ if (block->resized) {
+ block->resized(block->idstr, size, block->host);
+ }
return 0;
}
@@ -2155,7 +2164,7 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
DIRTY_CLIENTS_ALL);
memory_region_set_size(block->mr, newsize);
if (block->resized) {
- block->resized(block->idstr, newsize, block->host);
+ block->resized(block->idstr, size, block->host);
}
return 0;
}
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 179b302f01..ec9cfa4d10 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -934,9 +934,11 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
for (i = 0; i < index; i++) {
if (strcmp(filename, s->files->f[i].name) == 0) {
- ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i,
- data, len);
- s->files->f[i].size = cpu_to_be32(len);
+ if (s->files->f[i].size != cpu_to_be32(len)) {
+ ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i,
+ data, len);
+ s->files->f[i].size = cpu_to_be32(len);
+ }
return ptr;
}
}
---
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-05 16:29 ` Shameerali Kolothum Thodi
@ 2020-02-05 16:40 ` David Hildenbrand
2020-02-06 10:20 ` Shameerali Kolothum Thodi
0 siblings, 1 reply; 45+ messages in thread
From: David Hildenbrand @ 2020-02-05 16:40 UTC (permalink / raw)
To: Shameerali Kolothum Thodi, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger
>> Oh, and one more reason why the proposal in this patch is inconsistent:
>>
>> When migrating resizable memory regions (RAM_SAVE_FLAG_MEM_SIZE) we
>> store the block->used_length (ram_save_setup()) and use that value to
>> resize the region on the target (ram_load_precopy() -> qemu_ram_resize()).
>>
>> This will be the value the callback will be called with. Page aligned.
>>
>
> Sorry, I didn’t quite get that point and not sure how "req_length" approach
> will affect the migration.
The issue is that on migration, you will lose the sub-page size either
way. So your callback will be called
- on the migration source with a sub-page size (via
memory_region_ram_resize() from e.g., hw/i386/acpi-build.c)
- on the migration target with a page-aligned size (via
qemu_ram_resize() from migration/ram.c)
So this is inconsistent, especially when migrating.
Is there a way to get access to the sub-page size without passing it
through the callback?
Like in fw_cfg_modify_file() do some fancy lookup and use the sub-page
size instead of the passed size? (might have to be stored somewhere and
refetched - and migrated)
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-05 16:40 ` David Hildenbrand
@ 2020-02-06 10:20 ` Shameerali Kolothum Thodi
2020-02-06 10:55 ` David Hildenbrand
0 siblings, 1 reply; 45+ messages in thread
From: Shameerali Kolothum Thodi @ 2020-02-06 10:20 UTC (permalink / raw)
To: David Hildenbrand, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger
> -----Original Message-----
> From: David Hildenbrand [mailto:david@redhat.com]
> Sent: 05 February 2020 16:41
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> Igor Mammedov <imammedo@redhat.com>
> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com
> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
>
> >> Oh, and one more reason why the proposal in this patch is inconsistent:
> >>
> >> When migrating resizable memory regions (RAM_SAVE_FLAG_MEM_SIZE)
> we
> >> store the block->used_length (ram_save_setup()) and use that value to
> >> resize the region on the target (ram_load_precopy() -> qemu_ram_resize()).
> >>
> >> This will be the value the callback will be called with. Page aligned.
> >>
> >
> > Sorry, I didn’t quite get that point and not sure how "req_length" approach
> > will affect the migration.
>
> The issue is that on migration, you will lose the sub-page size either
> way. So your callback will be called
> - on the migration source with a sub-page size (via
> memory_region_ram_resize() from e.g., hw/i386/acpi-build.c)
> - on the migration target with a page-aligned size (via
> qemu_ram_resize() from migration/ram.c)
>
> So this is inconsistent, especially when migrating.
Thanks for explaining. I tried to add some debug prints to further understand
what actually happens during migration case.
Guest-source with initial one nvdimm
----------------------------------------------------
-object memory-backend-ram,id=mem1,size=1G \
-device nvdimm,id=dimm1,memdev=mem1 \
fw_cfg_add_file_callback: filename etc/boot-fail-wait size 0x4
fw_cfg_add_file_callback: filename etc/acpi/nvdimm-mem size 0x1000
fw_cfg_add_file_callback: filename etc/acpi/tables size 0x55f4
fw_cfg_add_file_callback: filename etc/table-loader size 0xd00
fw_cfg_add_file_callback: filename etc/tpm/log size 0x0
fw_cfg_add_file_callback: filename etc/acpi/rsdp size 0x24
fw_cfg_add_file_callback: filename etc/smbios/smbios-tables size 0x104
fw_cfg_add_file_callback: filename etc/smbios/smbios-anchor size 0x18
fw_cfg_modify_file: filename bootorder len 0x0
fw_cfg_add_file_callback: filename bootorder size 0x0
fw_cfg_modify_file: filename bios-geometry len 0x0
fw_cfg_add_file_callback: filename bios-geometry size 0x0
fw_cfg_modify_file: filename etc/acpi/tables len 0x55f4
fw_cfg_modify_file: filename etc/acpi/rsdp len 0x24
fw_cfg_modify_file: filename etc/table-loader len 0xd00
....
hot add another nvdimm device,
(qemu) object_add memory-backend-ram,id=mem2,size=1G
(qemu) device_add nvdimm,id=dimm2,memdev=mem2
root@ubuntu:/# cat /dev/pmem
pmem0 pmem1
Guest-target with two nvdimms
-----------------------------------------------
-object memory-backend-ram,id=mem1,size=1G \
-device nvdimm,id=dimm1,memdev=mem1 \
-object memory-backend-ram,id=mem2,size=1G \
-device nvdimm,id=dimm2,memdev=mem2 \
fw_cfg_add_file_callback: filename etc/boot-fail-wait size 0x4
fw_cfg_add_file_callback: filename etc/acpi/nvdimm-mem size 0x1000
fw_cfg_add_file_callback: filename etc/acpi/tables size 0x56ac
fw_cfg_add_file_callback: filename etc/table-loader size 0xd00
fw_cfg_add_file_callback: filename etc/tpm/log size 0x0
fw_cfg_add_file_callback: filename etc/acpi/rsdp size 0x24
fw_cfg_add_file_callback: filename etc/smbios/smbios-tables size 0x104
fw_cfg_add_file_callback: filename etc/smbios/smbios-anchor size 0x18
fw_cfg_modify_file: filename bootorder len 0x0
fw_cfg_add_file_callback: filename bootorder size 0x0
fw_cfg_modify_file: filename bios-geometry len 0x0
fw_cfg_add_file_callback: filename bios-geometry size 0x0
Initiate migration Source --> Target,
ram_load_precopy: Ram blk mach-virt.ram length 0x100000000 used_length 0x100000000
ram_load_precopy: Ram blk mem1 length 0x40000000 used_length 0x40000000
ram_load_precopy: Ram blk mem2 length 0x40000000 used_length 0x40000000
ram_load_precopy: Ram blk virt.flash0 length 0x4000000 used_length 0x4000000
ram_load_precopy: Ram blk virt.flash1 length 0x4000000 used_length 0x4000000
ram_load_precopy: Ram blk /rom@etc/acpi/tables length 0x6000 used_length 0x6000
ram_load_precopy: Ram blk 0000:00:01.0/virtio-net-pci.rom length 0x40000 used_length 0x40000
ram_load_precopy: Ram blk /rom@etc/table-loader length 0x1000 used_length 0x1000
ram_load_precopy: Ram blk /rom@etc/acpi/rsdp length 0x1000 used_length 0x1000
root@ubuntu:/# cat /dev/pmem
pmem0 pmem1
From the logs, it looks like the ram_load_precopy() --> qemu_ram_resize() is not
called as length == used_length and both seems to be page aligned values.
And from https://github.com/qemu/qemu/blob/master/migration/ram.c#L3421
qemu_ram_resize() is called with length if length != used_length.
Of course my knowledge on Qemu migration is very limited and maybe I am
missing something here. But I am trying to see under what circumstances
qemu_ram_resize() will get invoked with a page aligned size that will be different
to the size used in the FWCfgEntry . Please let me know,
Much appreciated,
Shameer
> Is there a way to get access to the sub-page size without passing it
> through the callback?
>
> Like in fw_cfg_modify_file() do some fancy lookup and use the sub-page
> size instead of the passed size? (might have to be stored somewhere and
> refetched - and migrated)
>
> --
> Thanks,
>
> David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-06 10:20 ` Shameerali Kolothum Thodi
@ 2020-02-06 10:55 ` David Hildenbrand
2020-02-06 11:28 ` Shameerali Kolothum Thodi
0 siblings, 1 reply; 45+ messages in thread
From: David Hildenbrand @ 2020-02-06 10:55 UTC (permalink / raw)
To: Shameerali Kolothum Thodi, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger
On 06.02.20 11:20, Shameerali Kolothum Thodi wrote:
>
>
>> -----Original Message-----
>> From: David Hildenbrand [mailto:david@redhat.com]
>> Sent: 05 February 2020 16:41
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> Igor Mammedov <imammedo@redhat.com>
>> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
>> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
>> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
>> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com
>> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
>>
>>>> Oh, and one more reason why the proposal in this patch is inconsistent:
>>>>
>>>> When migrating resizable memory regions (RAM_SAVE_FLAG_MEM_SIZE)
>> we
>>>> store the block->used_length (ram_save_setup()) and use that value to
>>>> resize the region on the target (ram_load_precopy() -> qemu_ram_resize()).
>>>>
>>>> This will be the value the callback will be called with. Page aligned.
>>>>
>>>
>>> Sorry, I didn’t quite get that point and not sure how "req_length" approach
>>> will affect the migration.
>>
>> The issue is that on migration, you will lose the sub-page size either
>> way. So your callback will be called
>> - on the migration source with a sub-page size (via
>> memory_region_ram_resize() from e.g., hw/i386/acpi-build.c)
>> - on the migration target with a page-aligned size (via
>> qemu_ram_resize() from migration/ram.c)
>>
>> So this is inconsistent, especially when migrating.
>
> Thanks for explaining. I tried to add some debug prints to further understand
> what actually happens during migration case.
>
> Guest-source with initial one nvdimm
> ----------------------------------------------------
>
> -object memory-backend-ram,id=mem1,size=1G \
> -device nvdimm,id=dimm1,memdev=mem1 \
>
> fw_cfg_add_file_callback: filename etc/boot-fail-wait size 0x4
> fw_cfg_add_file_callback: filename etc/acpi/nvdimm-mem size 0x1000
> fw_cfg_add_file_callback: filename etc/acpi/tables size 0x55f4
> fw_cfg_add_file_callback: filename etc/table-loader size 0xd00
> fw_cfg_add_file_callback: filename etc/tpm/log size 0x0
> fw_cfg_add_file_callback: filename etc/acpi/rsdp size 0x24
> fw_cfg_add_file_callback: filename etc/smbios/smbios-tables size 0x104
> fw_cfg_add_file_callback: filename etc/smbios/smbios-anchor size 0x18
> fw_cfg_modify_file: filename bootorder len 0x0
> fw_cfg_add_file_callback: filename bootorder size 0x0
> fw_cfg_modify_file: filename bios-geometry len 0x0
> fw_cfg_add_file_callback: filename bios-geometry size 0x0
> fw_cfg_modify_file: filename etc/acpi/tables len 0x55f4
> fw_cfg_modify_file: filename etc/acpi/rsdp len 0x24
> fw_cfg_modify_file: filename etc/table-loader len 0xd00
> ....
>
> hot add another nvdimm device,
>
> (qemu) object_add memory-backend-ram,id=mem2,size=1G
> (qemu) device_add nvdimm,id=dimm2,memdev=mem2
>
>
> root@ubuntu:/# cat /dev/pmem
> pmem0 pmem1
>
> Guest-target with two nvdimms
> -----------------------------------------------
>
> -object memory-backend-ram,id=mem1,size=1G \
> -device nvdimm,id=dimm1,memdev=mem1 \
> -object memory-backend-ram,id=mem2,size=1G \
> -device nvdimm,id=dimm2,memdev=mem2 \
>
> fw_cfg_add_file_callback: filename etc/boot-fail-wait size 0x4
> fw_cfg_add_file_callback: filename etc/acpi/nvdimm-mem size 0x1000
> fw_cfg_add_file_callback: filename etc/acpi/tables size 0x56ac
> fw_cfg_add_file_callback: filename etc/table-loader size 0xd00
> fw_cfg_add_file_callback: filename etc/tpm/log size 0x0
> fw_cfg_add_file_callback: filename etc/acpi/rsdp size 0x24
> fw_cfg_add_file_callback: filename etc/smbios/smbios-tables size 0x104
> fw_cfg_add_file_callback: filename etc/smbios/smbios-anchor size 0x18
> fw_cfg_modify_file: filename bootorder len 0x0
> fw_cfg_add_file_callback: filename bootorder size 0x0
> fw_cfg_modify_file: filename bios-geometry len 0x0
> fw_cfg_add_file_callback: filename bios-geometry size 0x0
>
>
> Initiate migration Source --> Target,
>
> ram_load_precopy: Ram blk mach-virt.ram length 0x100000000 used_length 0x100000000
> ram_load_precopy: Ram blk mem1 length 0x40000000 used_length 0x40000000
> ram_load_precopy: Ram blk mem2 length 0x40000000 used_length 0x40000000
> ram_load_precopy: Ram blk virt.flash0 length 0x4000000 used_length 0x4000000
> ram_load_precopy: Ram blk virt.flash1 length 0x4000000 used_length 0x4000000
> ram_load_precopy: Ram blk /rom@etc/acpi/tables length 0x6000 used_length 0x6000
> ram_load_precopy: Ram blk 0000:00:01.0/virtio-net-pci.rom length 0x40000 used_length 0x40000
> ram_load_precopy: Ram blk /rom@etc/table-loader length 0x1000 used_length 0x1000
> ram_load_precopy: Ram blk /rom@etc/acpi/rsdp length 0x1000 used_length 0x1000
>
>
> root@ubuntu:/# cat /dev/pmem
> pmem0 pmem1
>
> From the logs, it looks like the ram_load_precopy() --> qemu_ram_resize() is not
> called as length == used_length and both seems to be page aligned values.
> And from https://github.com/qemu/qemu/blob/master/migration/ram.c#L3421
> qemu_ram_resize() is called with length if length != used_length.
Assume on your source, the old size is 12345 bytes. So 16384 aligned up
(4 pages).
Assume on your target, the new size is 123456 bytes, so 126976 aligned
up (31 pages).
If you migrate from source to destination, the migration code would
resize to 16384, although the "actual size" is 12345. The callback will
be called with the aligned size, not the actual size. Same the other way
around. That's what's inconsistent IMHO.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-06 10:55 ` David Hildenbrand
@ 2020-02-06 11:28 ` Shameerali Kolothum Thodi
2020-02-06 14:54 ` David Hildenbrand
0 siblings, 1 reply; 45+ messages in thread
From: Shameerali Kolothum Thodi @ 2020-02-06 11:28 UTC (permalink / raw)
To: David Hildenbrand, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger
> -----Original Message-----
> From: David Hildenbrand [mailto:david@redhat.com]
> Sent: 06 February 2020 10:56
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> Igor Mammedov <imammedo@redhat.com>
> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com
> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
[...]
> > root@ubuntu:/# cat /dev/pmem
> > pmem0 pmem1
> >
> > From the logs, it looks like the ram_load_precopy() --> qemu_ram_resize() is
> not
> > called as length == used_length and both seems to be page aligned values.
> > And from
> https://github.com/qemu/qemu/blob/master/migration/ram.c#L3421
> > qemu_ram_resize() is called with length if length != used_length.
>
> Assume on your source, the old size is 12345 bytes. So 16384 aligned up
> (4 pages).
>
> Assume on your target, the new size is 123456 bytes, so 126976 aligned
> up (31 pages).
>
> If you migrate from source to destination, the migration code would
> resize to 16384, although the "actual size" is 12345. The callback will
> be called with the aligned size, not the actual size. Same the other way
> around. That's what's inconsistent IMHO.
Thanks. You are right. I didn’t consider the case where the target can be
configured with a larger number of devices than the source. I can replicate
the scenario now,
Source:
fw_cfg_add_file_callback: filename etc/boot-fail-wait size 0x4
fw_cfg_add_file_callback: filename etc/acpi/nvdimm-mem size 0x1000
fw_cfg_add_file_callback: filename etc/acpi/tables size 0x6210
Target:
ram_load_precopy: Ram blk mem1 length 0x40000000 used_length 0x40000000
ram_load_precopy: Ram blk virt.flash0 length 0x4000000 used_length 0x4000000
ram_load_precopy: Ram blk virt.flash1 length 0x4000000 used_length 0x4000000
ram_load_precopy: Ram blk /rom@etc/acpi/tables length 0x7000 used_length 0x8000
fw_cfg_modify_file: filename etc/acpi/tables len 0x7000
Target updates FWCfgEntry with a page aligned size :(. I will look into this and see how
we can solve this. Any pointers welcome.
Cheers,
Shameer
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-06 11:28 ` Shameerali Kolothum Thodi
@ 2020-02-06 14:54 ` David Hildenbrand
2020-02-07 16:05 ` Shameerali Kolothum Thodi
0 siblings, 1 reply; 45+ messages in thread
From: David Hildenbrand @ 2020-02-06 14:54 UTC (permalink / raw)
To: Shameerali Kolothum Thodi, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger
On 06.02.20 12:28, Shameerali Kolothum Thodi wrote:
>
>
>> -----Original Message-----
>> From: David Hildenbrand [mailto:david@redhat.com]
>> Sent: 06 February 2020 10:56
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> Igor Mammedov <imammedo@redhat.com>
>> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
>> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
>> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
>> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com
>> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
>
> [...]
>
>>> root@ubuntu:/# cat /dev/pmem
>>> pmem0 pmem1
>>>
>>> From the logs, it looks like the ram_load_precopy() --> qemu_ram_resize() is
>> not
>>> called as length == used_length and both seems to be page aligned values.
>>> And from
>> https://github.com/qemu/qemu/blob/master/migration/ram.c#L3421
>>> qemu_ram_resize() is called with length if length != used_length.
>>
>> Assume on your source, the old size is 12345 bytes. So 16384 aligned up
>> (4 pages).
>>
>> Assume on your target, the new size is 123456 bytes, so 126976 aligned
>> up (31 pages).
>>
>> If you migrate from source to destination, the migration code would
>> resize to 16384, although the "actual size" is 12345. The callback will
>> be called with the aligned size, not the actual size. Same the other way
>> around. That's what's inconsistent IMHO.
>
> Thanks. You are right. I didn’t consider the case where the target can be
> configured with a larger number of devices than the source. I can replicate
> the scenario now,
>
> Source:
>
> fw_cfg_add_file_callback: filename etc/boot-fail-wait size 0x4
> fw_cfg_add_file_callback: filename etc/acpi/nvdimm-mem size 0x1000
> fw_cfg_add_file_callback: filename etc/acpi/tables size 0x6210
>
> Target:
> ram_load_precopy: Ram blk mem1 length 0x40000000 used_length 0x40000000
> ram_load_precopy: Ram blk virt.flash0 length 0x4000000 used_length 0x4000000
> ram_load_precopy: Ram blk virt.flash1 length 0x4000000 used_length 0x4000000
> ram_load_precopy: Ram blk /rom@etc/acpi/tables length 0x7000 used_length 0x8000
> fw_cfg_modify_file: filename etc/acpi/tables len 0x7000
>
> Target updates FWCfgEntry with a page aligned size :(. I will look into this and see how
> we can solve this. Any pointers welcome.
Can you look the original value up somehow and us the resize callback
only as a notification that something changed? (that value would have to
be stored somewhere and migrated I assume - maybe that's already being done)
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-06 14:54 ` David Hildenbrand
@ 2020-02-07 16:05 ` Shameerali Kolothum Thodi
2020-02-10 9:29 ` David Hildenbrand
0 siblings, 1 reply; 45+ messages in thread
From: Shameerali Kolothum Thodi @ 2020-02-07 16:05 UTC (permalink / raw)
To: David Hildenbrand, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger
> -----Original Message-----
> From: David Hildenbrand [mailto:david@redhat.com]
> Sent: 06 February 2020 14:55
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> Igor Mammedov <imammedo@redhat.com>
> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com
> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
>
> On 06.02.20 12:28, Shameerali Kolothum Thodi wrote:
> >
> >
> >> -----Original Message-----
> >> From: David Hildenbrand [mailto:david@redhat.com]
> >> Sent: 06 February 2020 10:56
> >> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> >> Igor Mammedov <imammedo@redhat.com>
> >> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
> >> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
> >> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
> >> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com
> >> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
> >
> > [...]
> >
> >>> root@ubuntu:/# cat /dev/pmem
> >>> pmem0 pmem1
> >>>
> >>> From the logs, it looks like the ram_load_precopy() --> qemu_ram_resize()
> is
> >> not
> >>> called as length == used_length and both seems to be page aligned values.
> >>> And from
> >> https://github.com/qemu/qemu/blob/master/migration/ram.c#L3421
> >>> qemu_ram_resize() is called with length if length != used_length.
> >>
> >> Assume on your source, the old size is 12345 bytes. So 16384 aligned up
> >> (4 pages).
> >>
> >> Assume on your target, the new size is 123456 bytes, so 126976 aligned
> >> up (31 pages).
> >>
> >> If you migrate from source to destination, the migration code would
> >> resize to 16384, although the "actual size" is 12345. The callback will
> >> be called with the aligned size, not the actual size. Same the other way
> >> around. That's what's inconsistent IMHO.
> >
> > Thanks. You are right. I didn’t consider the case where the target can be
> > configured with a larger number of devices than the source. I can replicate
> > the scenario now,
> >
> > Source:
> >
> > fw_cfg_add_file_callback: filename etc/boot-fail-wait size 0x4
> > fw_cfg_add_file_callback: filename etc/acpi/nvdimm-mem size 0x1000
> > fw_cfg_add_file_callback: filename etc/acpi/tables size 0x6210
> >
> > Target:
> > ram_load_precopy: Ram blk mem1 length 0x40000000 used_length
> 0x40000000
> > ram_load_precopy: Ram blk virt.flash0 length 0x4000000 used_length
> 0x4000000
> > ram_load_precopy: Ram blk virt.flash1 length 0x4000000 used_length
> 0x4000000
> > ram_load_precopy: Ram blk /rom@etc/acpi/tables length 0x7000
> used_length 0x8000
> > fw_cfg_modify_file: filename etc/acpi/tables len 0x7000
> >
> > Target updates FWCfgEntry with a page aligned size :(. I will look into this and
> see how
> > we can solve this. Any pointers welcome.
>
> Can you look the original value up somehow and us the resize callback
> only as a notification that something changed? (that value would have to
> be stored somewhere and migrated I assume - maybe that's already being
> done)
Ok. I will take a look at that. But can we instead pass the block->used_length to
fw_cfg_add_file_callback(). That way we don’t have to change the qemu_ram_resize()
as well. I think Igor has suggested this before[1] and I had a go at it before coming up
with the "req_length" proposal here.
Thanks,
Shameer
[1] https://lore.kernel.org/qemu-devel/323aa74a92934b6a989e6e4dbe0dfe21@huawei.com/
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-07 16:05 ` Shameerali Kolothum Thodi
@ 2020-02-10 9:29 ` David Hildenbrand
2020-02-10 9:50 ` Shameerali Kolothum Thodi
0 siblings, 1 reply; 45+ messages in thread
From: David Hildenbrand @ 2020-02-10 9:29 UTC (permalink / raw)
To: Shameerali Kolothum Thodi, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger
>> Can you look the original value up somehow and us the resize callback
>> only as a notification that something changed? (that value would have to
>> be stored somewhere and migrated I assume - maybe that's already being
>> done)
>
> Ok. I will take a look at that. But can we instead pass the block->used_length to
> fw_cfg_add_file_callback(). That way we don’t have to change the qemu_ram_resize()
> as well. I think Igor has suggested this before[1] and I had a go at it before coming up
> with the "req_length" proposal here.
You mean, passing the old size as well? I don't see how that will solve
the issue, but yeah, nothing speaks against simply sending the old and
the new size.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-10 9:29 ` David Hildenbrand
@ 2020-02-10 9:50 ` Shameerali Kolothum Thodi
2020-02-10 9:53 ` David Hildenbrand
0 siblings, 1 reply; 45+ messages in thread
From: Shameerali Kolothum Thodi @ 2020-02-10 9:50 UTC (permalink / raw)
To: David Hildenbrand, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger
> -----Original Message-----
> From: David Hildenbrand [mailto:david@redhat.com]
> Sent: 10 February 2020 09:29
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> Igor Mammedov <imammedo@redhat.com>
> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com
> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
>
> >> Can you look the original value up somehow and us the resize callback
> >> only as a notification that something changed? (that value would have to
> >> be stored somewhere and migrated I assume - maybe that's already being
> >> done)
> >
> > Ok. I will take a look at that. But can we instead pass the block->used_length
> to
> > fw_cfg_add_file_callback(). That way we don’t have to change the
> qemu_ram_resize()
> > as well. I think Igor has suggested this before[1] and I had a go at it before
> coming up
> > with the "req_length" proposal here.
>
> You mean, passing the old size as well? I don't see how that will solve
> the issue, but yeah, nothing speaks against simply sending the old and
> the new size.
Nope. I actually meant using the block->used_length to store in the
s->files->f[index].size.
virt_acpi_setup()
acpi_add_rom_blob()
rom_add_blob()
rom_set_mr() --> used_length = page aligned blob size
fw_cfg_add_file_callback() --> uses actual blob size.
Right now what we do is use the actual blob size to store in FWCfgEntry.
Instead pass the RAMBlock used_length to fw_cfg_add_file_callback().
Of course by this, the firmware will see an aligned size, but that is fine I think.
But at the same time this means the qemu_ram_resize() can stay as it is
because it will invoke the callback when the size changes beyond the aligned
page size. And also during migration, there won't be any inconsistency as everyone
works on aligned page size.
Does that make sense? Or I am again missing something here?
Thanks,
Shameer
> --
> Thanks,
>
> David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-10 9:50 ` Shameerali Kolothum Thodi
@ 2020-02-10 9:53 ` David Hildenbrand
2020-02-12 17:07 ` Shameerali Kolothum Thodi
0 siblings, 1 reply; 45+ messages in thread
From: David Hildenbrand @ 2020-02-10 9:53 UTC (permalink / raw)
To: Shameerali Kolothum Thodi, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger
On 10.02.20 10:50, Shameerali Kolothum Thodi wrote:
>
>
>> -----Original Message-----
>> From: David Hildenbrand [mailto:david@redhat.com]
>> Sent: 10 February 2020 09:29
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> Igor Mammedov <imammedo@redhat.com>
>> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
>> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
>> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
>> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com
>> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
>>
>>>> Can you look the original value up somehow and us the resize callback
>>>> only as a notification that something changed? (that value would have to
>>>> be stored somewhere and migrated I assume - maybe that's already being
>>>> done)
>>>
>>> Ok. I will take a look at that. But can we instead pass the block->used_length
>> to
>>> fw_cfg_add_file_callback(). That way we don’t have to change the
>> qemu_ram_resize()
>>> as well. I think Igor has suggested this before[1] and I had a go at it before
>> coming up
>>> with the "req_length" proposal here.
>>
>> You mean, passing the old size as well? I don't see how that will solve
>> the issue, but yeah, nothing speaks against simply sending the old and
>> the new size.
>
> Nope. I actually meant using the block->used_length to store in the
> s->files->f[index].size.
>
> virt_acpi_setup()
> acpi_add_rom_blob()
> rom_add_blob()
> rom_set_mr() --> used_length = page aligned blob size
> fw_cfg_add_file_callback() --> uses actual blob size.
>
>
> Right now what we do is use the actual blob size to store in FWCfgEntry.
> Instead pass the RAMBlock used_length to fw_cfg_add_file_callback().
> Of course by this, the firmware will see an aligned size, but that is fine I think.
> But at the same time this means the qemu_ram_resize() can stay as it is
> because it will invoke the callback when the size changes beyond the aligned
> page size. And also during migration, there won't be any inconsistency as everyone
> works on aligned page size.
>
> Does that make sense? Or I am again missing something here?
Oh, you mean simply rounding up to full pages in the fw entries? If we
can drop the "sub-page" restriction, that would be awesome!
Need to double check if that could be an issue for fw/migration/whatever.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-10 9:53 ` David Hildenbrand
@ 2020-02-12 17:07 ` Shameerali Kolothum Thodi
2020-02-12 18:20 ` David Hildenbrand
0 siblings, 1 reply; 45+ messages in thread
From: Shameerali Kolothum Thodi @ 2020-02-12 17:07 UTC (permalink / raw)
To: David Hildenbrand, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger
> -----Original Message-----
> From: David Hildenbrand [mailto:david@redhat.com]
> Sent: 10 February 2020 09:54
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> Igor Mammedov <imammedo@redhat.com>
> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com
> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
>
> On 10.02.20 10:50, Shameerali Kolothum Thodi wrote:
> >
> >
> >> -----Original Message-----
> >> From: David Hildenbrand [mailto:david@redhat.com]
> >> Sent: 10 February 2020 09:29
> >> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> >> Igor Mammedov <imammedo@redhat.com>
> >> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
> >> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
> >> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
> >> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com
> >> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
> >>
> >>>> Can you look the original value up somehow and us the resize callback
> >>>> only as a notification that something changed? (that value would have to
> >>>> be stored somewhere and migrated I assume - maybe that's already
> being
> >>>> done)
> >>>
> >>> Ok. I will take a look at that. But can we instead pass the
> block->used_length
> >> to
> >>> fw_cfg_add_file_callback(). That way we don’t have to change the
> >> qemu_ram_resize()
> >>> as well. I think Igor has suggested this before[1] and I had a go at it before
> >> coming up
> >>> with the "req_length" proposal here.
> >>
> >> You mean, passing the old size as well? I don't see how that will solve
> >> the issue, but yeah, nothing speaks against simply sending the old and
> >> the new size.
> >
> > Nope. I actually meant using the block->used_length to store in the
> > s->files->f[index].size.
> >
> > virt_acpi_setup()
> > acpi_add_rom_blob()
> > rom_add_blob()
> > rom_set_mr() --> used_length = page aligned blob size
> > fw_cfg_add_file_callback() --> uses actual blob size.
> >
> >
> > Right now what we do is use the actual blob size to store in FWCfgEntry.
> > Instead pass the RAMBlock used_length to fw_cfg_add_file_callback().
> > Of course by this, the firmware will see an aligned size, but that is fine I think.
> > But at the same time this means the qemu_ram_resize() can stay as it is
> > because it will invoke the callback when the size changes beyond the aligned
> > page size. And also during migration, there won't be any inconsistency as
> everyone
> > works on aligned page size.
> >
> > Does that make sense? Or I am again missing something here?
>
> Oh, you mean simply rounding up to full pages in the fw entries? If we
> can drop the "sub-page" restriction, that would be awesome!
>
> Need to double check if that could be an issue for fw/migration/whatever.
Hmm..it breaks x86 + seabios boot. The issue is seabios expects RSDP in FSEG
memory. With the above proposed change, RSDP will be aligned to PAGE_SIZE and
seabios mem allocation for RSDP fails at,
https://github.com/coreboot/seabios/blob/master/src/fw/romfile_loader.c#L85
To get pass the above, I changed "alloc_fseg" flag to false in build_rsdp(), but
seabios seems to make the assumption that RSDP has to be placed in FSEG memory
here,
https://github.com/coreboot/seabios/blob/master/src/fw/biostables.c#L126
So doesn’t look like there is an easy fix for this without changing the seabios code.
Between, OVMF works fine with the aligned size on x86.
One thing we can do is treat the RSDP case separately or only use the aligned
page size for "etc/acpi/tables" as below,
diff --git a/hw/core/loader.c b/hw/core/loader.c
index d1b78f60cd..f07f6a7a35 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -60,6 +60,7 @@
#include "hw/boards.h"
#include "qemu/cutils.h"
#include "sysemu/runstate.h"
+#include "hw/acpi/aml-build.h"
#include <zlib.h>
@@ -1056,6 +1057,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
if (fw_file_name && fw_cfg) {
char devpath[100];
void *data;
+ size_t size = rom->datasize;
if (read_only) {
snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
@@ -1066,13 +1068,21 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
if (mc->rom_file_has_mr) {
data = rom_set_mr(rom, OBJECT(fw_cfg), devpath, read_only);
mr = rom->mr;
+ /*
+ * Use the RAMblk used_length for acpi tables as this avoids any
+ * inconsistency with table size changes during hot add and during
+ * migration.
+ */
+ if (strcmp(fw_file_name, ACPI_BUILD_TABLE_FILE) == 0) {
+ size = memory_region_get_used_length(mr);
+ }
} else {
data = rom->data;
}
fw_cfg_add_file_callback(fw_cfg, fw_file_name,
fw_callback, NULL, callback_opaque,
- data, rom->datasize, read_only);
+ data, size, read_only);
}
return mr;
}
Thoughts?
Thanks,
Shameer
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-12 17:07 ` Shameerali Kolothum Thodi
@ 2020-02-12 18:20 ` David Hildenbrand
2020-02-13 16:38 ` Shameerali Kolothum Thodi
0 siblings, 1 reply; 45+ messages in thread
From: David Hildenbrand @ 2020-02-12 18:20 UTC (permalink / raw)
To: Shameerali Kolothum Thodi, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst,
Juan Jose Quintela Carreira, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger, dgilbert
On 12.02.20 18:07, Shameerali Kolothum Thodi wrote:
>
>
>> -----Original Message-----
>> From: David Hildenbrand [mailto:david@redhat.com]
>> Sent: 10 February 2020 09:54
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> Igor Mammedov <imammedo@redhat.com>
>> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
>> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
>> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
>> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com
>> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
>>
>> On 10.02.20 10:50, Shameerali Kolothum Thodi wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: David Hildenbrand [mailto:david@redhat.com]
>>>> Sent: 10 February 2020 09:29
>>>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>>>> Igor Mammedov <imammedo@redhat.com>
>>>> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
>>>> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
>>>> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
>>>> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com
>>>> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
>>>>
>>>>>> Can you look the original value up somehow and us the resize callback
>>>>>> only as a notification that something changed? (that value would have to
>>>>>> be stored somewhere and migrated I assume - maybe that's already
>> being
>>>>>> done)
>>>>>
>>>>> Ok. I will take a look at that. But can we instead pass the
>> block->used_length
>>>> to
>>>>> fw_cfg_add_file_callback(). That way we don’t have to change the
>>>> qemu_ram_resize()
>>>>> as well. I think Igor has suggested this before[1] and I had a go at it before
>>>> coming up
>>>>> with the "req_length" proposal here.
>>>>
>>>> You mean, passing the old size as well? I don't see how that will solve
>>>> the issue, but yeah, nothing speaks against simply sending the old and
>>>> the new size.
>>>
>>> Nope. I actually meant using the block->used_length to store in the
>>> s->files->f[index].size.
>>>
>>> virt_acpi_setup()
>>> acpi_add_rom_blob()
>>> rom_add_blob()
>>> rom_set_mr() --> used_length = page aligned blob size
>>> fw_cfg_add_file_callback() --> uses actual blob size.
>>>
>>>
>>> Right now what we do is use the actual blob size to store in FWCfgEntry.
>>> Instead pass the RAMBlock used_length to fw_cfg_add_file_callback().
>>> Of course by this, the firmware will see an aligned size, but that is fine I think.
>>> But at the same time this means the qemu_ram_resize() can stay as it is
>>> because it will invoke the callback when the size changes beyond the aligned
>>> page size. And also during migration, there won't be any inconsistency as
>> everyone
>>> works on aligned page size.
>>>
>>> Does that make sense? Or I am again missing something here?
>>
>> Oh, you mean simply rounding up to full pages in the fw entries? If we
>> can drop the "sub-page" restriction, that would be awesome!
>>
>> Need to double check if that could be an issue for fw/migration/whatever.
>
> Hmm..it breaks x86 + seabios boot. The issue is seabios expects RSDP in FSEG
> memory. With the above proposed change, RSDP will be aligned to PAGE_SIZE and
> seabios mem allocation for RSDP fails at,
>
> https://github.com/coreboot/seabios/blob/master/src/fw/romfile_loader.c#L85
>
> To get pass the above, I changed "alloc_fseg" flag to false in build_rsdp(), but
> seabios seems to make the assumption that RSDP has to be placed in FSEG memory
> here,
> https://github.com/coreboot/seabios/blob/master/src/fw/biostables.c#L126
>
> So doesn’t look like there is an easy fix for this without changing the seabios code.
>
> Between, OVMF works fine with the aligned size on x86.
>
> One thing we can do is treat the RSDP case separately or only use the aligned
> page size for "etc/acpi/tables" as below,
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index d1b78f60cd..f07f6a7a35 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -60,6 +60,7 @@
> #include "hw/boards.h"
> #include "qemu/cutils.h"
> #include "sysemu/runstate.h"
> +#include "hw/acpi/aml-build.h"
>
> #include <zlib.h>
>
> @@ -1056,6 +1057,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
> if (fw_file_name && fw_cfg) {
> char devpath[100];
> void *data;
> + size_t size = rom->datasize;
>
> if (read_only) {
> snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
> @@ -1066,13 +1068,21 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
> if (mc->rom_file_has_mr) {
> data = rom_set_mr(rom, OBJECT(fw_cfg), devpath, read_only);
> mr = rom->mr;
> + /*
> + * Use the RAMblk used_length for acpi tables as this avoids any
> + * inconsistency with table size changes during hot add and during
> + * migration.
> + */
> + if (strcmp(fw_file_name, ACPI_BUILD_TABLE_FILE) == 0) {
> + size = memory_region_get_used_length(mr);
> + }
> } else {
> data = rom->data;
> }
>
> fw_cfg_add_file_callback(fw_cfg, fw_file_name,
> fw_callback, NULL, callback_opaque,
> - data, rom->datasize, read_only);
> + data, size, read_only);
> }
> return mr;
> }
>
>
> Thoughts?
I don't think introducing memory_region_get_used_length() is a
good idea. I also dislike, that the ram block size can differ
to the memory region size. I wasn't aware of that condition, sorry!
Making the memory region always store an aligned size might break other use cases.
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.
3. When migrating, we migrate the aligned size.
What about something like this: (untested)
From d84c21bc67e15acdac2f6265cd1576d8dd920211 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Wed, 12 Feb 2020 19:16:34 +0100
Subject: [PATCH v1] tmp
Signed-off-by: David Hildenbrand <david@redhat.com>
---
exec.c | 14 ++++++++++++--
migration/ram.c | 44 ++++++++++++++++++++++++++++++++------------
2 files changed, 44 insertions(+), 14 deletions(-)
diff --git a/exec.c b/exec.c
index 05cfe868ab..d41a1e11b5 100644
--- a/exec.c
+++ b/exec.c
@@ -2130,11 +2130,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;
}
@@ -2158,9 +2168,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/migration/ram.c b/migration/ram.c
index ed23ed1c7c..2acc4b85ca 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1761,28 +1761,43 @@ void acct_update_position(QEMUFile *f, size_t size, bool zero)
}
}
-static uint64_t ram_bytes_total_common(bool count_ignored)
+static uint64_t ramblock_ram_bytes_migration(RAMBlock *block)
+{
+ /*
+ * When resizing on the target, we need the unaligned size,
+ * otherwise, we lose the unaligned size during migration.
+ */
+ if (block->mr && (block->flags & RAM_RESIZEABLE)) {
+ return memory_region_size(block->mr);
+ } else {
+ return block->used_length;
+ }
+}
+
+static uint64_t ram_bytes_total_migration(void)
{
RAMBlock *block;
uint64_t total = 0;
RCU_READ_LOCK_GUARD();
- if (count_ignored) {
- RAMBLOCK_FOREACH_MIGRATABLE(block) {
- total += block->used_length;
- }
- } else {
- RAMBLOCK_FOREACH_NOT_IGNORED(block) {
- total += block->used_length;
- }
+ RAMBLOCK_FOREACH_MIGRATABLE(block) {
+ total += ramblock_ram_bytes_migration(block);
}
return total;
}
uint64_t ram_bytes_total(void)
{
- return ram_bytes_total_common(false);
+ RAMBlock *block;
+ uint64_t total = 0;
+
+ RCU_READ_LOCK_GUARD();
+
+ RAMBLOCK_FOREACH_NOT_IGNORED(block) {
+ total += block->used_length;
+ }
+ return total;
}
static void xbzrle_load_setup(void)
@@ -2432,12 +2447,17 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
(*rsp)->f = f;
WITH_RCU_READ_LOCK_GUARD() {
- qemu_put_be64(f, ram_bytes_total_common(true) | RAM_SAVE_FLAG_MEM_SIZE);
+ qemu_put_be64(f, ram_bytes_total_migration() | RAM_SAVE_FLAG_MEM_SIZE);
RAMBLOCK_FOREACH_MIGRATABLE(block) {
qemu_put_byte(f, strlen(block->idstr));
qemu_put_buffer(f, (uint8_t *)block->idstr, strlen(block->idstr));
- qemu_put_be64(f, block->used_length);
+ /*
+ * When resizing on the target, we need the unaligned size,
+ * otherwise we lose the unaligned sise during migration.
+ */
+ qemu_put_be64(f, ramblock_ram_bytes_migration(block));
+
if (migrate_postcopy_ram() && block->page_size !=
qemu_host_page_size) {
qemu_put_be64(f, block->page_size);
--
2.24.1
--
Thanks,
David / dhildenb
^ permalink raw reply related [flat|nested] 45+ messages in thread* RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-12 18:20 ` David Hildenbrand
@ 2020-02-13 16:38 ` Shameerali Kolothum Thodi
2020-02-13 16:59 ` David Hildenbrand
0 siblings, 1 reply; 45+ messages in thread
From: Shameerali Kolothum Thodi @ 2020-02-13 16:38 UTC (permalink / raw)
To: David Hildenbrand, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst,
Juan Jose Quintela Carreira, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger, dgilbert
> -----Original Message-----
> From: David Hildenbrand [mailto:david@redhat.com]
> Sent: 12 February 2020 18:21
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> Igor Mammedov <imammedo@redhat.com>
> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com;
> dgilbert@redhat.com; Juan Jose Quintela Carreira <quintela@redhat.com>
> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
[...]
> > Hmm..it breaks x86 + seabios boot. The issue is seabios expects RSDP in FSEG
> > memory. With the above proposed change, RSDP will be aligned to PAGE_SIZE
> and
> > seabios mem allocation for RSDP fails at,
> >
> >
> https://github.com/coreboot/seabios/blob/master/src/fw/romfile_loader.c#L8
> 5
> >
> > To get pass the above, I changed "alloc_fseg" flag to false in build_rsdp(), but
> > seabios seems to make the assumption that RSDP has to be placed in FSEG
> memory
> > here,
> > https://github.com/coreboot/seabios/blob/master/src/fw/biostables.c#L126
> >
> > So doesn’t look like there is an easy fix for this without changing the seabios
> code.
> >
> > Between, OVMF works fine with the aligned size on x86.
> >
> > One thing we can do is treat the RSDP case separately or only use the aligned
> > page size for "etc/acpi/tables" as below,
[...]
> >
> > Thoughts?
>
> I don't think introducing memory_region_get_used_length() is a
> good idea. I also dislike, that the ram block size can differ
> to the memory region size. I wasn't aware of that condition, sorry!
Right. They can differ in size and is the case here.
> Making the memory region always store an aligned size might break other use
> cases.
>
> 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.
> 3. When migrating, we migrate the aligned size.
>
>
> What about something like this: (untested)
Thanks for that. I had a go with the below patch and it indeed fixes the issue
of callback not being called on resize. But the migration fails with the below
error,
For x86
---------
qemu-system-x86_64: Unknown combination of migration flags: 0x14
qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram'
qemu-system-x86_64: load of migration failed: Invalid argument
For arm64
--------------
qemu-system-aarch64: Received an unexpected compressed page
qemu-system-aarch64: error while loading state for instance 0x0 of device 'ram'
qemu-system-aarch64: load of migration failed: Invalid argument
I haven’t debugged this further but looks like there is a corruption happening.
Please let me know if you have any clue.
Thanks,
Shameer
>
> From d84c21bc67e15acdac2f6265cd1576d8dd920211 Mon Sep 17 00:00:00
> 2001
> From: David Hildenbrand <david@redhat.com>
> Date: Wed, 12 Feb 2020 19:16:34 +0100
> Subject: [PATCH v1] tmp
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> exec.c | 14 ++++++++++++--
> migration/ram.c | 44 ++++++++++++++++++++++++++++++++------------
> 2 files changed, 44 insertions(+), 14 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 05cfe868ab..d41a1e11b5 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2130,11 +2130,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;
> }
>
> @@ -2158,9 +2168,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/migration/ram.c b/migration/ram.c
> index ed23ed1c7c..2acc4b85ca 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1761,28 +1761,43 @@ void acct_update_position(QEMUFile *f, size_t
> size, bool zero)
> }
> }
>
> -static uint64_t ram_bytes_total_common(bool count_ignored)
> +static uint64_t ramblock_ram_bytes_migration(RAMBlock *block)
> +{
> + /*
> + * When resizing on the target, we need the unaligned size,
> + * otherwise, we lose the unaligned size during migration.
> + */
> + if (block->mr && (block->flags & RAM_RESIZEABLE)) {
> + return memory_region_size(block->mr);
> + } else {
> + return block->used_length;
> + }
> +}
> +
> +static uint64_t ram_bytes_total_migration(void)
> {
> RAMBlock *block;
> uint64_t total = 0;
>
> RCU_READ_LOCK_GUARD();
>
> - if (count_ignored) {
> - RAMBLOCK_FOREACH_MIGRATABLE(block) {
> - total += block->used_length;
> - }
> - } else {
> - RAMBLOCK_FOREACH_NOT_IGNORED(block) {
> - total += block->used_length;
> - }
> + RAMBLOCK_FOREACH_MIGRATABLE(block) {
> + total += ramblock_ram_bytes_migration(block);
> }
> return total;
> }
>
> uint64_t ram_bytes_total(void)
> {
> - return ram_bytes_total_common(false);
> + RAMBlock *block;
> + uint64_t total = 0;
> +
> + RCU_READ_LOCK_GUARD();
> +
> + RAMBLOCK_FOREACH_NOT_IGNORED(block) {
> + total += block->used_length;
> + }
> + return total;
> }
>
> static void xbzrle_load_setup(void)
> @@ -2432,12 +2447,17 @@ static int ram_save_setup(QEMUFile *f, void
> *opaque)
> (*rsp)->f = f;
>
> WITH_RCU_READ_LOCK_GUARD() {
> - qemu_put_be64(f, ram_bytes_total_common(true) |
> RAM_SAVE_FLAG_MEM_SIZE);
> + qemu_put_be64(f, ram_bytes_total_migration() |
> RAM_SAVE_FLAG_MEM_SIZE);
>
> RAMBLOCK_FOREACH_MIGRATABLE(block) {
> qemu_put_byte(f, strlen(block->idstr));
> qemu_put_buffer(f, (uint8_t *)block->idstr,
> strlen(block->idstr));
> - qemu_put_be64(f, block->used_length);
> + /*
> + * When resizing on the target, we need the unaligned size,
> + * otherwise we lose the unaligned sise during migration.
> + */
> + qemu_put_be64(f, ramblock_ram_bytes_migration(block));
> +
> if (migrate_postcopy_ram() && block->page_size !=
> qemu_host_page_size) {
> qemu_put_be64(f, block->page_size);
> --
> 2.24.1
>
>
> --
> Thanks,
>
> David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-13 16:38 ` Shameerali Kolothum Thodi
@ 2020-02-13 16:59 ` David Hildenbrand
2020-02-13 17:09 ` David Hildenbrand
0 siblings, 1 reply; 45+ messages in thread
From: David Hildenbrand @ 2020-02-13 16:59 UTC (permalink / raw)
To: Shameerali Kolothum Thodi, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst,
Juan Jose Quintela Carreira, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger, dgilbert
On 13.02.20 17:38, Shameerali Kolothum Thodi wrote:
>
>
>> -----Original Message-----
>> From: David Hildenbrand [mailto:david@redhat.com]
>> Sent: 12 February 2020 18:21
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> Igor Mammedov <imammedo@redhat.com>
>> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
>> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
>> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
>> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com;
>> dgilbert@redhat.com; Juan Jose Quintela Carreira <quintela@redhat.com>
>> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
>
> [...]
>
>>> Hmm..it breaks x86 + seabios boot. The issue is seabios expects RSDP in FSEG
>>> memory. With the above proposed change, RSDP will be aligned to PAGE_SIZE
>> and
>>> seabios mem allocation for RSDP fails at,
>>>
>>>
>> https://github.com/coreboot/seabios/blob/master/src/fw/romfile_loader.c#L8
>> 5
>>>
>>> To get pass the above, I changed "alloc_fseg" flag to false in build_rsdp(), but
>>> seabios seems to make the assumption that RSDP has to be placed in FSEG
>> memory
>>> here,
>>> https://github.com/coreboot/seabios/blob/master/src/fw/biostables.c#L126
>>>
>>> So doesn’t look like there is an easy fix for this without changing the seabios
>> code.
>>>
>>> Between, OVMF works fine with the aligned size on x86.
>>>
>>> One thing we can do is treat the RSDP case separately or only use the aligned
>>> page size for "etc/acpi/tables" as below,
>
> [...]
>
>>>
>>> Thoughts?
>>
>> I don't think introducing memory_region_get_used_length() is a
>> good idea. I also dislike, that the ram block size can differ
>> to the memory region size. I wasn't aware of that condition, sorry!
>
> Right. They can differ in size and is the case here.
>
>> Making the memory region always store an aligned size might break other use
>> cases.
>>
>> 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.
>> 3. When migrating, we migrate the aligned size.
>>
>>
>> What about something like this: (untested)
>
> Thanks for that. I had a go with the below patch and it indeed fixes the issue
> of callback not being called on resize. But the migration fails with the below
> error,
>
> For x86
> ---------
> qemu-system-x86_64: Unknown combination of migration flags: 0x14
> qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram'
> qemu-system-x86_64: load of migration failed: Invalid argument
>
> For arm64
> --------------
> qemu-system-aarch64: Received an unexpected compressed page
> qemu-system-aarch64: error while loading state for instance 0x0 of device 'ram'
> qemu-system-aarch64: load of migration failed: Invalid argument
>
> I haven’t debugged this further but looks like there is a corruption happening.
> Please let me know if you have any clue.
The issue is
qemu_put_be64(f, ram_bytes_total_common(true) | RAM_SAVE_FLAG_MEM_SIZE)
The total ram size we store must be page aligned, otherwise it will be
detected as flags. Hm ... maybe we can round it up ...
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-13 16:59 ` David Hildenbrand
@ 2020-02-13 17:09 ` David Hildenbrand
2020-02-28 16:49 ` Shameerali Kolothum Thodi
0 siblings, 1 reply; 45+ messages in thread
From: David Hildenbrand @ 2020-02-13 17:09 UTC (permalink / raw)
To: Shameerali Kolothum Thodi, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst,
Juan Jose Quintela Carreira, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger, dgilbert
On 13.02.20 17:59, David Hildenbrand wrote:
> On 13.02.20 17:38, Shameerali Kolothum Thodi wrote:
>>
>>
>>> -----Original Message-----
>>> From: David Hildenbrand [mailto:david@redhat.com]
>>> Sent: 12 February 2020 18:21
>>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>>> Igor Mammedov <imammedo@redhat.com>
>>> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
>>> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
>>> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
>>> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com;
>>> dgilbert@redhat.com; Juan Jose Quintela Carreira <quintela@redhat.com>
>>> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
>>
>> [...]
>>
>>>> Hmm..it breaks x86 + seabios boot. The issue is seabios expects RSDP in FSEG
>>>> memory. With the above proposed change, RSDP will be aligned to PAGE_SIZE
>>> and
>>>> seabios mem allocation for RSDP fails at,
>>>>
>>>>
>>> https://github.com/coreboot/seabios/blob/master/src/fw/romfile_loader.c#L8
>>> 5
>>>>
>>>> To get pass the above, I changed "alloc_fseg" flag to false in build_rsdp(), but
>>>> seabios seems to make the assumption that RSDP has to be placed in FSEG
>>> memory
>>>> here,
>>>> https://github.com/coreboot/seabios/blob/master/src/fw/biostables.c#L126
>>>>
>>>> So doesn’t look like there is an easy fix for this without changing the seabios
>>> code.
>>>>
>>>> Between, OVMF works fine with the aligned size on x86.
>>>>
>>>> One thing we can do is treat the RSDP case separately or only use the aligned
>>>> page size for "etc/acpi/tables" as below,
>>
>> [...]
>>
>>>>
>>>> Thoughts?
>>>
>>> I don't think introducing memory_region_get_used_length() is a
>>> good idea. I also dislike, that the ram block size can differ
>>> to the memory region size. I wasn't aware of that condition, sorry!
>>
>> Right. They can differ in size and is the case here.
>>
>>> Making the memory region always store an aligned size might break other use
>>> cases.
>>>
>>> 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.
>>> 3. When migrating, we migrate the aligned size.
>>>
>>>
>>> What about something like this: (untested)
>>
>> Thanks for that. I had a go with the below patch and it indeed fixes the issue
>> of callback not being called on resize. But the migration fails with the below
>> error,
>>
>> For x86
>> ---------
>> qemu-system-x86_64: Unknown combination of migration flags: 0x14
>> qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram'
>> qemu-system-x86_64: load of migration failed: Invalid argument
>>
>> For arm64
>> --------------
>> qemu-system-aarch64: Received an unexpected compressed page
>> qemu-system-aarch64: error while loading state for instance 0x0 of device 'ram'
>> qemu-system-aarch64: load of migration failed: Invalid argument
>>
>> I haven’t debugged this further but looks like there is a corruption happening.
>> Please let me know if you have any clue.
>
> The issue is
>
> qemu_put_be64(f, ram_bytes_total_common(true) | RAM_SAVE_FLAG_MEM_SIZE)
>
> The total ram size we store must be page aligned, otherwise it will be
> detected as flags. Hm ... maybe we can round it up ...
>
I'm afraid we can't otherwise we will run into issues in
ram_load_precopy(). Hm ...
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-13 17:09 ` David Hildenbrand
@ 2020-02-28 16:49 ` Shameerali Kolothum Thodi
2020-02-28 17:59 ` David Hildenbrand
0 siblings, 1 reply; 45+ messages in thread
From: Shameerali Kolothum Thodi @ 2020-02-28 16:49 UTC (permalink / raw)
To: David Hildenbrand, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst,
Juan Jose Quintela Carreira, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger, dgilbert
> -----Original Message-----
> From: David Hildenbrand [mailto:david@redhat.com]
> Sent: 13 February 2020 17:09
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> Igor Mammedov <imammedo@redhat.com>
> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com;
> dgilbert@redhat.com; Juan Jose Quintela Carreira <quintela@redhat.com>
> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
[...]
> >> Thanks for that. I had a go with the below patch and it indeed fixes the issue
> >> of callback not being called on resize. But the migration fails with the below
> >> error,
> >>
> >> For x86
> >> ---------
> >> qemu-system-x86_64: Unknown combination of migration flags: 0x14
> >> qemu-system-x86_64: error while loading state for instance 0x0 of device
> 'ram'
> >> qemu-system-x86_64: load of migration failed: Invalid argument
> >>
> >> For arm64
> >> --------------
> >> qemu-system-aarch64: Received an unexpected compressed page
> >> qemu-system-aarch64: error while loading state for instance 0x0 of device
> 'ram'
> >> qemu-system-aarch64: load of migration failed: Invalid argument
> >>
> >> I haven’t debugged this further but looks like there is a corruption
> happening.
> >> Please let me know if you have any clue.
> >
> > The issue is
> >
> > qemu_put_be64(f, ram_bytes_total_common(true) |
> RAM_SAVE_FLAG_MEM_SIZE)
> >
> > The total ram size we store must be page aligned, otherwise it will be
> > detected as flags. Hm ... maybe we can round it up ...
> >
>
> I'm afraid we can't otherwise we will run into issues in
> ram_load_precopy(). Hm ...
Sorry, took a while to get back on this. Yes, round up indeed breaks in
ram_load_precopy() . I had the below on top of your patch and that
seems to do the job (sanity tested on arm/virt).
Please take a look and let me know if you see any issues with this approach.
Thanks,
Shameer
diff --git a/migration/ram.c b/migration/ram.c
index 2acc4b85ca..7447f0cefa 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1782,7 +1782,7 @@ static uint64_t ram_bytes_total_migration(void)
RCU_READ_LOCK_GUARD();
RAMBLOCK_FOREACH_MIGRATABLE(block) {
- total += ramblock_ram_bytes_migration(block);
+ total += block->used_length;
}
return total;
}
@@ -3479,7 +3479,7 @@ static int ram_load_precopy(QEMUFile *f)
ret = -EINVAL;
}
- total_ram_bytes -= length;
+ total_ram_bytes -= block->used_length;
}
break;
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-28 16:49 ` Shameerali Kolothum Thodi
@ 2020-02-28 17:59 ` David Hildenbrand
2020-03-11 17:28 ` Shameerali Kolothum Thodi
0 siblings, 1 reply; 45+ messages in thread
From: David Hildenbrand @ 2020-02-28 17:59 UTC (permalink / raw)
To: Shameerali Kolothum Thodi, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst,
Juan Jose Quintela Carreira, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger, dgilbert
On 28.02.20 17:49, Shameerali Kolothum Thodi wrote:
>
>
>> -----Original Message-----
>> From: David Hildenbrand [mailto:david@redhat.com]
>> Sent: 13 February 2020 17:09
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> Igor Mammedov <imammedo@redhat.com>
>> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
>> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
>> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
>> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com;
>> dgilbert@redhat.com; Juan Jose Quintela Carreira <quintela@redhat.com>
>> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
>
> [...]
>
>>>> Thanks for that. I had a go with the below patch and it indeed fixes the issue
>>>> of callback not being called on resize. But the migration fails with the below
>>>> error,
>>>>
>>>> For x86
>>>> ---------
>>>> qemu-system-x86_64: Unknown combination of migration flags: 0x14
>>>> qemu-system-x86_64: error while loading state for instance 0x0 of device
>> 'ram'
>>>> qemu-system-x86_64: load of migration failed: Invalid argument
>>>>
>>>> For arm64
>>>> --------------
>>>> qemu-system-aarch64: Received an unexpected compressed page
>>>> qemu-system-aarch64: error while loading state for instance 0x0 of device
>> 'ram'
>>>> qemu-system-aarch64: load of migration failed: Invalid argument
>>>>
>>>> I haven’t debugged this further but looks like there is a corruption
>> happening.
>>>> Please let me know if you have any clue.
>>>
>>> The issue is
>>>
>>> qemu_put_be64(f, ram_bytes_total_common(true) |
>> RAM_SAVE_FLAG_MEM_SIZE)
>>>
>>> The total ram size we store must be page aligned, otherwise it will be
>>> detected as flags. Hm ... maybe we can round it up ...
>>>
>>
>> I'm afraid we can't otherwise we will run into issues in
>> ram_load_precopy(). Hm ...
>
> Sorry, took a while to get back on this. Yes, round up indeed breaks in
> ram_load_precopy() . I had the below on top of your patch and that
> seems to do the job (sanity tested on arm/virt).
>
> Please take a look and let me know if you see any issues with this approach.
>
> Thanks,
> Shameer
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 2acc4b85ca..7447f0cefa 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1782,7 +1782,7 @@ static uint64_t ram_bytes_total_migration(void)
> RCU_READ_LOCK_GUARD();
>
> RAMBLOCK_FOREACH_MIGRATABLE(block) {
> - total += ramblock_ram_bytes_migration(block);
> + total += block->used_length;
> }
> return total;
> }
> @@ -3479,7 +3479,7 @@ static int ram_load_precopy(QEMUFile *f)
> ret = -EINVAL;
> }
>
> - total_ram_bytes -= length;
> + total_ram_bytes -= block->used_length;
> }
> break;
>
>
>
What you mean is the following:
commit 702f4325086c3a8d6083787f8bc8503f7523bac8 (HEAD -> master)
Author: David Hildenbrand <david@redhat.com>
Date: Wed Feb 12 19:16:34 2020 +0100
tmp
Signed-off-by: David Hildenbrand <david@redhat.com>
diff --git a/exec.c b/exec.c
index 67e520d18e..cec643b914 100644
--- a/exec.c
+++ b/exec.c
@@ -2125,11 +2125,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;
}
@@ -2153,9 +2163,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/migration/ram.c b/migration/ram.c
index d2208b5534..249d3edede 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3412,7 +3412,15 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
RAMBLOCK_FOREACH_MIGRATABLE(block) {
qemu_put_byte(f, strlen(block->idstr));
qemu_put_buffer(f, (uint8_t *)block->idstr, strlen(block->idstr));
- qemu_put_be64(f, block->used_length);
+ /*
+ * When resizing on the target, we need the unaligned size,
+ * otherwise we lose the unaligned sise during migration.
+ *
+ * Note: The sum of all ram blocks will differ to
+ * ram_bytes_total_common(true) stored above.
+ */
+ qemu_put_be64(f, ramblock_ram_bytes_migration(block));
+
if (migrate_postcopy_ram() && block->page_size !=
qemu_host_page_size) {
qemu_put_be64(f, block->page_size);
@@ -4429,7 +4437,7 @@ static int ram_load_precopy(QEMUFile *f)
ret = -EINVAL;
}
- total_ram_bytes -= length;
+ total_ram_bytes -= block->used_length;
}
break;
Please note that this will *for sure* break migration between QEMU versions.
So I don't think this will work.
We should instead think about
1. Migrating the actual size of the 3 memory regions separately and setting them via
memory_region_ram_resize() when loading the vmstate. This will trigger another FW cfg
fixup and should be fine (with the same qemu_ram_resize() above).
2. Introduce a new RAM_SAVE_FLAG_MEM_SIZE_2, that e.g., stores the number of ramblocks,
not the total amount of memory of the ram blocks. But it's hacky, because we migrate
something for RAM blocks, that is not a RAM block concept (sub-block sizes).
I think you should look into 1. Shouldn't be too hard I think.
--
Thanks,
David / dhildenb
^ permalink raw reply related [flat|nested] 45+ messages in thread* RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
2020-02-28 17:59 ` David Hildenbrand
@ 2020-03-11 17:28 ` Shameerali Kolothum Thodi
0 siblings, 0 replies; 45+ messages in thread
From: Shameerali Kolothum Thodi @ 2020-03-11 17:28 UTC (permalink / raw)
To: David Hildenbrand, Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst,
Juan Jose Quintela Carreira, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
lersek, eric.auger, dgilbert
> -----Original Message-----
> From: David Hildenbrand [mailto:david@redhat.com]
> Sent: 28 February 2020 18:00
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> Igor Mammedov <imammedo@redhat.com>
> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com;
> dgilbert@redhat.com; Juan Jose Quintela Carreira <quintela@redhat.com>
> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
>
[...]
>
> We should instead think about
>
> 1. Migrating the actual size of the 3 memory regions separately and setting
> them via
> memory_region_ram_resize() when loading the vmstate. This will trigger
> another FW cfg
> fixup and should be fine (with the same qemu_ram_resize() above).
>
> 2. Introduce a new RAM_SAVE_FLAG_MEM_SIZE_2, that e.g., stores the
> number of ramblocks,
> not the total amount of memory of the ram blocks. But it's hacky, because we
> migrate
> something for RAM blocks, that is not a RAM block concept (sub-block sizes).
>
> I think you should look into 1. Shouldn't be too hard I think.
I have send out v3 of this series ([PATCH v3 00/10] ARM virt: Add NVDIMM support)
with an attempt to migrate the memory regions separately. It also includes
your patch for qemu_ram_resize() callback fix. Please take a look and let me know.
Thanks,
Shameer
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v2 2/7] hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length
2020-01-17 17:45 [PATCH v2 0/7] ARM virt: Add NVDIMM support Shameer Kolothum
2020-01-17 17:45 ` [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback Shameer Kolothum
@ 2020-01-17 17:45 ` Shameer Kolothum
2020-01-28 17:08 ` Auger Eric
2020-02-06 16:06 ` Igor Mammedov
2020-01-17 17:45 ` [PATCH v2 3/7] nvdimm: Use configurable ACPI IO base and size Shameer Kolothum
` (5 subsequent siblings)
7 siblings, 2 replies; 45+ messages in thread
From: Shameer Kolothum @ 2020-01-17 17:45 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, xiaoguangrong.eric, mst, linuxarm, xuwei5,
shannon.zhaosl, lersek
As per ACPI spec 6.3, Table 19-419 Object Conversion Rules, if
the Buffer Field <= to the size of an Integer (in bits), it will
be treated as an integer. Moreover, the integer size depends on
DSDT tables revision number. If revision number is < 2, integer
size is 32 bits, otherwise it is 64 bits. Current NVDIMM common
DSM aml code (NCAL) uses CreateField() for creating DSM output
buffer. This creates an issue in arm/virt platform where DSDT
revision number is 2 and results in DSM buffer with a wrong
size(8 bytes) gets returned when actual length is < 8 bytes.
This causes guest kernel to report,
"nfit ACPI0012:00: found a zero length table '0' parsing nfit"
In order to fix this, aml code is now modified such that it builds
the DSM output buffer in a byte by byte fashion when length is
smaller than Integer size.
Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
Please find the previous discussion on this here,
https://patchwork.kernel.org/cover/11174959/
---
hw/acpi/nvdimm.c | 36 +++++++++++++++++++--
tests/qtest/bios-tables-test-allowed-diff.h | 2 ++
2 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 9fdad6dc3f..5e7b8318d0 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -964,6 +964,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
+ Aml *whilectx, *offset;
uint8_t byte_list[1];
method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
@@ -1117,13 +1118,42 @@ static void nvdimm_build_common_dsm(Aml *dev)
/* RLEN is not included in the payload returned to guest. */
aml_append(method, aml_subtract(aml_name(NVDIMM_DSM_OUT_BUF_SIZE),
aml_int(4), dsm_out_buf_size));
+
+ /*
+ * As per ACPI spec 6.3, Table 19-419 Object Conversion Rules, if
+ * the Buffer Field <= to the size of an Integer (in bits), it will
+ * be treated as an integer. Moreover, the integer size depends on
+ * DSDT tables revision number. If revision number is < 2, integer
+ * size is 32 bits, otherwise it is 64 bits.
+ * Because of this CreateField() canot be used if RLEN < Integer Size.
+ * Hence build dsm_out_buf byte by byte.
+ */
+ ifctx = aml_if(aml_lless(dsm_out_buf_size, aml_sizeof(aml_int(0))));
+ offset = aml_local(2);
+ aml_append(ifctx, aml_store(aml_int(0), offset));
+ aml_append(ifctx, aml_name_decl("TBUF", aml_buffer(1, NULL)));
+ aml_append(ifctx, aml_store(aml_buffer(0, NULL), dsm_out_buf));
+
+ whilectx = aml_while(aml_lless(offset, dsm_out_buf_size));
+ /* Copy 1 byte at offset from ODAT to temporary buffer(TBUF). */
+ aml_append(whilectx, aml_store(aml_derefof(aml_index(
+ aml_name(NVDIMM_DSM_OUT_BUF), offset)),
+ aml_index(aml_name("TBUF"), aml_int(0))));
+ aml_append(whilectx, aml_concatenate(dsm_out_buf, aml_name("TBUF"),
+ dsm_out_buf));
+ aml_append(whilectx, aml_increment(offset));
+ aml_append(ifctx, whilectx);
+
+ aml_append(ifctx, aml_return(dsm_out_buf));
+ aml_append(method, ifctx);
+
+ /* If RLEN >= Integer size, just use CreateField() operator */
aml_append(method, aml_store(aml_shiftleft(dsm_out_buf_size, aml_int(3)),
dsm_out_buf_size));
aml_append(method, aml_create_field(aml_name(NVDIMM_DSM_OUT_BUF),
aml_int(0), dsm_out_buf_size, "OBUF"));
- aml_append(method, aml_concatenate(aml_buffer(0, NULL), aml_name("OBUF"),
- dsm_out_buf));
- aml_append(method, aml_return(dsm_out_buf));
+ aml_append(method, aml_return(aml_name("OBUF")));
+
aml_append(dev, method);
}
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..eb8bae1407 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,3 @@
/* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/pc/SSDT.dimmpxm",
+"tests/data/acpi/q35/SSDT.dimmpxm",
--
2.17.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 2/7] hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length
2020-01-17 17:45 ` [PATCH v2 2/7] hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length Shameer Kolothum
@ 2020-01-28 17:08 ` Auger Eric
2020-02-06 16:06 ` Igor Mammedov
1 sibling, 0 replies; 45+ messages in thread
From: Auger Eric @ 2020-01-28 17:08 UTC (permalink / raw)
To: Shameer Kolothum, qemu-devel, qemu-arm, imammedo
Cc: peter.maydell, xiaoguangrong.eric, mst, linuxarm, xuwei5,
shannon.zhaosl, lersek
Hi Shameer,
On 1/17/20 6:45 PM, Shameer Kolothum wrote:
> As per ACPI spec 6.3, Table 19-419 Object Conversion Rules, if
> the Buffer Field <= to the size of an Integer (in bits), it will
> be treated as an integer. Moreover, the integer size depends on
> DSDT tables revision number. If revision number is < 2, integer
> size is 32 bits, otherwise it is 64 bits. Current NVDIMM common
> DSM aml code (NCAL) uses CreateField() for creating DSM output
> buffer. This creates an issue in arm/virt platform where DSDT
> revision number is 2 and results in DSM buffer with a wrong
> size(8 bytes) gets returned when actual length is < 8 bytes.
> This causes guest kernel to report,
>
> "nfit ACPI0012:00: found a zero length table '0' parsing nfit"
>
> In order to fix this, aml code is now modified such that it builds
> the DSM output buffer in a byte by byte fashion when length is
> smaller than Integer size.
>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> Please find the previous discussion on this here,
> https://patchwork.kernel.org/cover/11174959/
>
> ---
> hw/acpi/nvdimm.c | 36 +++++++++++++++++++--
> tests/qtest/bios-tables-test-allowed-diff.h | 2 ++
> 2 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 9fdad6dc3f..5e7b8318d0 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -964,6 +964,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
> Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
> Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
> Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
> + Aml *whilectx, *offset;
> uint8_t byte_list[1];
>
> method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
> @@ -1117,13 +1118,42 @@ static void nvdimm_build_common_dsm(Aml *dev)
> /* RLEN is not included in the payload returned to guest. */
> aml_append(method, aml_subtract(aml_name(NVDIMM_DSM_OUT_BUF_SIZE),
> aml_int(4), dsm_out_buf_size));
> +
> + /*
> + * As per ACPI spec 6.3, Table 19-419 Object Conversion Rules, if
> + * the Buffer Field <= to the size of an Integer (in bits), it will
> + * be treated as an integer. Moreover, the integer size depends on
> + * DSDT tables revision number. If revision number is < 2, integer
> + * size is 32 bits, otherwise it is 64 bits.
> + * Because of this CreateField() canot be used if RLEN < Integer Size.
nit: Because of this, CreateField() cannot
Thanks
Eric
> + * Hence build dsm_out_buf byte by byte.
> + */
> + ifctx = aml_if(aml_lless(dsm_out_buf_size, aml_sizeof(aml_int(0))));
> + offset = aml_local(2);
> + aml_append(ifctx, aml_store(aml_int(0), offset));
> + aml_append(ifctx, aml_name_decl("TBUF", aml_buffer(1, NULL)));
> + aml_append(ifctx, aml_store(aml_buffer(0, NULL), dsm_out_buf));
> +
> + whilectx = aml_while(aml_lless(offset, dsm_out_buf_size));
> + /* Copy 1 byte at offset from ODAT to temporary buffer(TBUF). */
> + aml_append(whilectx, aml_store(aml_derefof(aml_index(
> + aml_name(NVDIMM_DSM_OUT_BUF), offset)),
> + aml_index(aml_name("TBUF"), aml_int(0))));
> + aml_append(whilectx, aml_concatenate(dsm_out_buf, aml_name("TBUF"),
> + dsm_out_buf));
> + aml_append(whilectx, aml_increment(offset));
> + aml_append(ifctx, whilectx);
> +
> + aml_append(ifctx, aml_return(dsm_out_buf));
> + aml_append(method, ifctx);
> +
> + /* If RLEN >= Integer size, just use CreateField() operator */
> aml_append(method, aml_store(aml_shiftleft(dsm_out_buf_size, aml_int(3)),
> dsm_out_buf_size));
> aml_append(method, aml_create_field(aml_name(NVDIMM_DSM_OUT_BUF),
> aml_int(0), dsm_out_buf_size, "OBUF"));
> - aml_append(method, aml_concatenate(aml_buffer(0, NULL), aml_name("OBUF"),
> - dsm_out_buf));
> - aml_append(method, aml_return(dsm_out_buf));
> + aml_append(method, aml_return(aml_name("OBUF")));
> +
> aml_append(dev, method);
> }
>
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index dfb8523c8b..eb8bae1407 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1 +1,3 @@
> /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/pc/SSDT.dimmpxm",
> +"tests/data/acpi/q35/SSDT.dimmpxm",
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 2/7] hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length
2020-01-17 17:45 ` [PATCH v2 2/7] hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length Shameer Kolothum
2020-01-28 17:08 ` Auger Eric
@ 2020-02-06 16:06 ` Igor Mammedov
2020-03-10 11:22 ` Shameerali Kolothum Thodi
1 sibling, 1 reply; 45+ messages in thread
From: Igor Mammedov @ 2020-02-06 16:06 UTC (permalink / raw)
To: Shameer Kolothum
Cc: peter.maydell, xiaoguangrong.eric, mst, shannon.zhaosl,
qemu-devel, xuwei5, linuxarm, eric.auger, qemu-arm, lersek
On Fri, 17 Jan 2020 17:45:17 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> As per ACPI spec 6.3, Table 19-419 Object Conversion Rules, if
> the Buffer Field <= to the size of an Integer (in bits), it will
> be treated as an integer. Moreover, the integer size depends on
> DSDT tables revision number. If revision number is < 2, integer
> size is 32 bits, otherwise it is 64 bits. Current NVDIMM common
> DSM aml code (NCAL) uses CreateField() for creating DSM output
> buffer. This creates an issue in arm/virt platform where DSDT
> revision number is 2 and results in DSM buffer with a wrong
> size(8 bytes) gets returned when actual length is < 8 bytes.
> This causes guest kernel to report,
>
> "nfit ACPI0012:00: found a zero length table '0' parsing nfit"
>
> In order to fix this, aml code is now modified such that it builds
> the DSM output buffer in a byte by byte fashion when length is
> smaller than Integer size.
>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> Please find the previous discussion on this here,
> https://patchwork.kernel.org/cover/11174959/
>
> ---
> hw/acpi/nvdimm.c | 36 +++++++++++++++++++--
> tests/qtest/bios-tables-test-allowed-diff.h | 2 ++
> 2 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 9fdad6dc3f..5e7b8318d0 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -964,6 +964,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
> Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
> Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
> Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
> + Aml *whilectx, *offset;
> uint8_t byte_list[1];
>
> method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
> @@ -1117,13 +1118,42 @@ static void nvdimm_build_common_dsm(Aml *dev)
> /* RLEN is not included in the payload returned to guest. */
> aml_append(method, aml_subtract(aml_name(NVDIMM_DSM_OUT_BUF_SIZE),
> aml_int(4), dsm_out_buf_size));
> +
> + /*
> + * As per ACPI spec 6.3, Table 19-419 Object Conversion Rules, if
> + * the Buffer Field <= to the size of an Integer (in bits), it will
> + * be treated as an integer. Moreover, the integer size depends on
> + * DSDT tables revision number. If revision number is < 2, integer
> + * size is 32 bits, otherwise it is 64 bits.
> + * Because of this CreateField() canot be used if RLEN < Integer Size.
> + * Hence build dsm_out_buf byte by byte.
> + */
> + ifctx = aml_if(aml_lless(dsm_out_buf_size, aml_sizeof(aml_int(0))));
this decomplies into
If (Local1 < SizeOf ())
which doesn't look right
> + offset = aml_local(2);
> + aml_append(ifctx, aml_store(aml_int(0), offset));
> + aml_append(ifctx, aml_name_decl("TBUF", aml_buffer(1, NULL)));
> + aml_append(ifctx, aml_store(aml_buffer(0, NULL), dsm_out_buf));
> +
> + whilectx = aml_while(aml_lless(offset, dsm_out_buf_size));
> + /* Copy 1 byte at offset from ODAT to temporary buffer(TBUF). */
> + aml_append(whilectx, aml_store(aml_derefof(aml_index(
> + aml_name(NVDIMM_DSM_OUT_BUF), offset)),
> + aml_index(aml_name("TBUF"), aml_int(0))));
> + aml_append(whilectx, aml_concatenate(dsm_out_buf, aml_name("TBUF"),
> + dsm_out_buf));
> + aml_append(whilectx, aml_increment(offset));
> + aml_append(ifctx, whilectx);
> +
> + aml_append(ifctx, aml_return(dsm_out_buf));
> + aml_append(method, ifctx);
> +
> + /* If RLEN >= Integer size, just use CreateField() operator */
> aml_append(method, aml_store(aml_shiftleft(dsm_out_buf_size, aml_int(3)),
> dsm_out_buf_size));
> aml_append(method, aml_create_field(aml_name(NVDIMM_DSM_OUT_BUF),
> aml_int(0), dsm_out_buf_size, "OBUF"));
> - aml_append(method, aml_concatenate(aml_buffer(0, NULL), aml_name("OBUF"),
> - dsm_out_buf));
> - aml_append(method, aml_return(dsm_out_buf));
> + aml_append(method, aml_return(aml_name("OBUF")));
> +
> aml_append(dev, method);
> }
>
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index dfb8523c8b..eb8bae1407 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1 +1,3 @@
> /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/pc/SSDT.dimmpxm",
> +"tests/data/acpi/q35/SSDT.dimmpxm",
^ permalink raw reply [flat|nested] 45+ messages in thread* RE: [PATCH v2 2/7] hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length
2020-02-06 16:06 ` Igor Mammedov
@ 2020-03-10 11:22 ` Shameerali Kolothum Thodi
2020-03-10 11:36 ` Michael S. Tsirkin
0 siblings, 1 reply; 45+ messages in thread
From: Shameerali Kolothum Thodi @ 2020-03-10 11:22 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, xiaoguangrong.eric, mst, shannon.zhaosl,
qemu-devel, Linuxarm, eric.auger, qemu-arm, xuwei (O),
lersek
> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: 06 February 2020 16:06
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org;
> xiaoguangrong.eric@gmail.com; mst@redhat.com; Linuxarm
> <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>;
> shannon.zhaosl@gmail.com; lersek@redhat.com
> Subject: Re: [PATCH v2 2/7] hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM
> output buffer length
>
> On Fri, 17 Jan 2020 17:45:17 +0000
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
>
> > As per ACPI spec 6.3, Table 19-419 Object Conversion Rules, if the
> > Buffer Field <= to the size of an Integer (in bits), it will be
> > treated as an integer. Moreover, the integer size depends on DSDT
> > tables revision number. If revision number is < 2, integer size is 32
> > bits, otherwise it is 64 bits. Current NVDIMM common DSM aml code
> > (NCAL) uses CreateField() for creating DSM output buffer. This creates
> > an issue in arm/virt platform where DSDT revision number is 2 and
> > results in DSM buffer with a wrong
> > size(8 bytes) gets returned when actual length is < 8 bytes.
> > This causes guest kernel to report,
> >
> > "nfit ACPI0012:00: found a zero length table '0' parsing nfit"
> >
> > In order to fix this, aml code is now modified such that it builds the
> > DSM output buffer in a byte by byte fashion when length is smaller
> > than Integer size.
> >
> > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> > Please find the previous discussion on this here,
> > https://patchwork.kernel.org/cover/11174959/
> >
> > ---
> > hw/acpi/nvdimm.c | 36
> +++++++++++++++++++--
> > tests/qtest/bios-tables-test-allowed-diff.h | 2 ++
> > 2 files changed, 35 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c index
> > 9fdad6dc3f..5e7b8318d0 100644
> > --- a/hw/acpi/nvdimm.c
> > +++ b/hw/acpi/nvdimm.c
> > @@ -964,6 +964,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
> > Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem,
> *elsectx2;
> > Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
> > Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf,
> > *dsm_out_buf_size;
> > + Aml *whilectx, *offset;
> > uint8_t byte_list[1];
> >
> > method = aml_method(NVDIMM_COMMON_DSM, 5,
> AML_SERIALIZED); @@
> > -1117,13 +1118,42 @@ static void nvdimm_build_common_dsm(Aml *dev)
> > /* RLEN is not included in the payload returned to guest. */
> > aml_append(method,
> aml_subtract(aml_name(NVDIMM_DSM_OUT_BUF_SIZE),
> > aml_int(4), dsm_out_buf_size));
> > +
> > + /*
> > + * As per ACPI spec 6.3, Table 19-419 Object Conversion Rules, if
> > + * the Buffer Field <= to the size of an Integer (in bits), it will
> > + * be treated as an integer. Moreover, the integer size depends on
> > + * DSDT tables revision number. If revision number is < 2, integer
> > + * size is 32 bits, otherwise it is 64 bits.
> > + * Because of this CreateField() canot be used if RLEN < Integer Size.
> > + * Hence build dsm_out_buf byte by byte.
> > + */
> > + ifctx = aml_if(aml_lless(dsm_out_buf_size,
> > + aml_sizeof(aml_int(0))));
>
> this decomplies into
>
> If (Local1 < SizeOf ())
>
> which doesn't look right
Ok. I tried printing the value returned(SizeOf) and that looks alright.
Anyway, changed it into aml_int(1) which decompiles to
If (Local1 < SizeOf (One))
Hope this is acceptable.
Thanks,
Shameer
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 2/7] hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length
2020-03-10 11:22 ` Shameerali Kolothum Thodi
@ 2020-03-10 11:36 ` Michael S. Tsirkin
2020-03-10 11:59 ` Shameerali Kolothum Thodi
0 siblings, 1 reply; 45+ messages in thread
From: Michael S. Tsirkin @ 2020-03-10 11:36 UTC (permalink / raw)
To: Shameerali Kolothum Thodi
Cc: peter.maydell, xiaoguangrong.eric, shannon.zhaosl, qemu-devel,
Linuxarm, eric.auger, qemu-arm, xuwei (O),
Igor Mammedov, lersek
On Tue, Mar 10, 2020 at 11:22:05AM +0000, Shameerali Kolothum Thodi wrote:
>
>
> > -----Original Message-----
> > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > Sent: 06 February 2020 16:06
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> > eric.auger@redhat.com; peter.maydell@linaro.org;
> > xiaoguangrong.eric@gmail.com; mst@redhat.com; Linuxarm
> > <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>;
> > shannon.zhaosl@gmail.com; lersek@redhat.com
> > Subject: Re: [PATCH v2 2/7] hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM
> > output buffer length
> >
> > On Fri, 17 Jan 2020 17:45:17 +0000
> > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> >
> > > As per ACPI spec 6.3, Table 19-419 Object Conversion Rules, if the
> > > Buffer Field <= to the size of an Integer (in bits), it will be
> > > treated as an integer. Moreover, the integer size depends on DSDT
> > > tables revision number. If revision number is < 2, integer size is 32
> > > bits, otherwise it is 64 bits. Current NVDIMM common DSM aml code
> > > (NCAL) uses CreateField() for creating DSM output buffer. This creates
> > > an issue in arm/virt platform where DSDT revision number is 2 and
> > > results in DSM buffer with a wrong
> > > size(8 bytes) gets returned when actual length is < 8 bytes.
> > > This causes guest kernel to report,
> > >
> > > "nfit ACPI0012:00: found a zero length table '0' parsing nfit"
> > >
> > > In order to fix this, aml code is now modified such that it builds the
> > > DSM output buffer in a byte by byte fashion when length is smaller
> > > than Integer size.
> > >
> > > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > > ---
> > > Please find the previous discussion on this here,
> > > https://patchwork.kernel.org/cover/11174959/
> > >
> > > ---
> > > hw/acpi/nvdimm.c | 36
> > +++++++++++++++++++--
> > > tests/qtest/bios-tables-test-allowed-diff.h | 2 ++
> > > 2 files changed, 35 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c index
> > > 9fdad6dc3f..5e7b8318d0 100644
> > > --- a/hw/acpi/nvdimm.c
> > > +++ b/hw/acpi/nvdimm.c
> > > @@ -964,6 +964,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
> > > Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem,
> > *elsectx2;
> > > Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
> > > Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf,
> > > *dsm_out_buf_size;
> > > + Aml *whilectx, *offset;
> > > uint8_t byte_list[1];
> > >
> > > method = aml_method(NVDIMM_COMMON_DSM, 5,
> > AML_SERIALIZED); @@
> > > -1117,13 +1118,42 @@ static void nvdimm_build_common_dsm(Aml *dev)
> > > /* RLEN is not included in the payload returned to guest. */
> > > aml_append(method,
> > aml_subtract(aml_name(NVDIMM_DSM_OUT_BUF_SIZE),
> > > aml_int(4), dsm_out_buf_size));
> > > +
> > > + /*
> > > + * As per ACPI spec 6.3, Table 19-419 Object Conversion Rules, if
> > > + * the Buffer Field <= to the size of an Integer (in bits), it will
> > > + * be treated as an integer. Moreover, the integer size depends on
> > > + * DSDT tables revision number. If revision number is < 2, integer
> > > + * size is 32 bits, otherwise it is 64 bits.
> > > + * Because of this CreateField() canot be used if RLEN < Integer Size.
> > > + * Hence build dsm_out_buf byte by byte.
> > > + */
> > > + ifctx = aml_if(aml_lless(dsm_out_buf_size,
> > > + aml_sizeof(aml_int(0))));
> >
> > this decomplies into
> >
> > If (Local1 < SizeOf ())
> >
> > which doesn't look right
>
> Ok. I tried printing the value returned(SizeOf) and that looks alright.
Well it's illegal in ACPI, it's possible that OSPMs handle it the way
you want them to, but it's probably not a good idea to assume they will
always do.
The spec says:
DefSizeOf := SizeOfOp SuperName
> Anyway, changed it into aml_int(1) which decompiles to
>
> If (Local1 < SizeOf (One))
>
> Hope this is acceptable.
>
> Thanks,
> Shameer
I suspect it doesn't. And going into semantics, since they are set by
ASL:
19.6.125 SizeOf (Get Data Object Size)
Syntax
SizeOf (ObjectName) => Integer
Arguments
ObjectName must be a buffer, string or package object.
Description
Returns the size of a buffer, string, or package data object.
For a buffer, it returns the size in bytes of the data. For a string, it returns the size in bytes of the
string, not counting the trailing NULL. For a package, it returns the number of elements. For an
object reference, the size of the referenced object is returned. Other data types cause a fatal run-time
error.
Bottom line, I don't think you can figure out the integer size like this.
What's wrong with just assuming 8 byte integers? I guess sizes 5 to 8
will be slower with a 32 bit DSDT but why is that a problem?
--
MST
^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH v2 2/7] hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length
2020-03-10 11:36 ` Michael S. Tsirkin
@ 2020-03-10 11:59 ` Shameerali Kolothum Thodi
0 siblings, 0 replies; 45+ messages in thread
From: Shameerali Kolothum Thodi @ 2020-03-10 11:59 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: peter.maydell, xiaoguangrong.eric, shannon.zhaosl, qemu-devel,
Linuxarm, eric.auger, qemu-arm, xuwei (O),
Igor Mammedov, lersek
> -----Original Message-----
> From: Qemu-devel
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> u.org] On Behalf Of Michael S. Tsirkin
> Sent: 10 March 2020 11:36
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
> shannon.zhaosl@gmail.com; qemu-devel@nongnu.org; Linuxarm
> <linuxarm@huawei.com>; eric.auger@redhat.com; qemu-arm@nongnu.org;
> xuwei (O) <xuwei5@huawei.com>; Igor Mammedov
> <imammedo@redhat.com>; lersek@redhat.com
> Subject: Re: [PATCH v2 2/7] hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM
> output buffer length
>
> On Tue, Mar 10, 2020 at 11:22:05AM +0000, Shameerali Kolothum Thodi
> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > > Sent: 06 February 2020 16:06
> > > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> > > eric.auger@redhat.com; peter.maydell@linaro.org;
> > > xiaoguangrong.eric@gmail.com; mst@redhat.com; Linuxarm
> > > <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>;
> > > shannon.zhaosl@gmail.com; lersek@redhat.com
> > > Subject: Re: [PATCH v2 2/7] hw/acpi/nvdimm: Fix for NVDIMM incorrect
> DSM
> > > output buffer length
> > >
> > > On Fri, 17 Jan 2020 17:45:17 +0000
> > > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> > >
> > > > As per ACPI spec 6.3, Table 19-419 Object Conversion Rules, if the
> > > > Buffer Field <= to the size of an Integer (in bits), it will be
> > > > treated as an integer. Moreover, the integer size depends on DSDT
> > > > tables revision number. If revision number is < 2, integer size is 32
> > > > bits, otherwise it is 64 bits. Current NVDIMM common DSM aml code
> > > > (NCAL) uses CreateField() for creating DSM output buffer. This creates
> > > > an issue in arm/virt platform where DSDT revision number is 2 and
> > > > results in DSM buffer with a wrong
> > > > size(8 bytes) gets returned when actual length is < 8 bytes.
> > > > This causes guest kernel to report,
> > > >
> > > > "nfit ACPI0012:00: found a zero length table '0' parsing nfit"
> > > >
> > > > In order to fix this, aml code is now modified such that it builds the
> > > > DSM output buffer in a byte by byte fashion when length is smaller
> > > > than Integer size.
> > > >
> > > > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > > > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > > > ---
> > > > Please find the previous discussion on this here,
> > > > https://patchwork.kernel.org/cover/11174959/
> > > >
> > > > ---
> > > > hw/acpi/nvdimm.c | 36
> > > +++++++++++++++++++--
> > > > tests/qtest/bios-tables-test-allowed-diff.h | 2 ++
> > > > 2 files changed, 35 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c index
> > > > 9fdad6dc3f..5e7b8318d0 100644
> > > > --- a/hw/acpi/nvdimm.c
> > > > +++ b/hw/acpi/nvdimm.c
> > > > @@ -964,6 +964,7 @@ static void nvdimm_build_common_dsm(Aml
> *dev)
> > > > Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem,
> > > *elsectx2;
> > > > Aml *elsectx, *unsupport, *unpatched, *expected_uuid,
> *uuid_invalid;
> > > > Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf,
> > > > *dsm_out_buf_size;
> > > > + Aml *whilectx, *offset;
> > > > uint8_t byte_list[1];
> > > >
> > > > method = aml_method(NVDIMM_COMMON_DSM, 5,
> > > AML_SERIALIZED); @@
> > > > -1117,13 +1118,42 @@ static void nvdimm_build_common_dsm(Aml
> *dev)
> > > > /* RLEN is not included in the payload returned to guest. */
> > > > aml_append(method,
> > > aml_subtract(aml_name(NVDIMM_DSM_OUT_BUF_SIZE),
> > > > aml_int(4), dsm_out_buf_size));
> > > > +
> > > > + /*
> > > > + * As per ACPI spec 6.3, Table 19-419 Object Conversion Rules, if
> > > > + * the Buffer Field <= to the size of an Integer (in bits), it will
> > > > + * be treated as an integer. Moreover, the integer size depends on
> > > > + * DSDT tables revision number. If revision number is < 2, integer
> > > > + * size is 32 bits, otherwise it is 64 bits.
> > > > + * Because of this CreateField() canot be used if RLEN < Integer
> Size.
> > > > + * Hence build dsm_out_buf byte by byte.
> > > > + */
> > > > + ifctx = aml_if(aml_lless(dsm_out_buf_size,
> > > > + aml_sizeof(aml_int(0))));
> > >
> > > this decomplies into
> > >
> > > If (Local1 < SizeOf ())
> > >
> > > which doesn't look right
> >
> > Ok. I tried printing the value returned(SizeOf) and that looks alright.
>
> Well it's illegal in ACPI, it's possible that OSPMs handle it the way
> you want them to, but it's probably not a good idea to assume they will
> always do.
>
> The spec says:
>
> DefSizeOf := SizeOfOp SuperName
>
>
>
> > Anyway, changed it into aml_int(1) which decompiles to
> >
> > If (Local1 < SizeOf (One))
> >
> > Hope this is acceptable.
> >
> > Thanks,
> > Shameer
>
> I suspect it doesn't. And going into semantics, since they are set by
> ASL:
>
>
> 19.6.125 SizeOf (Get Data Object Size)
> Syntax
> SizeOf (ObjectName) => Integer
> Arguments
> ObjectName must be a buffer, string or package object.
> Description
> Returns the size of a buffer, string, or package data object.
> For a buffer, it returns the size in bytes of the data. For a string, it returns the
> size in bytes of the
> string, not counting the trailing NULL. For a package, it returns the number of
> elements. For an
> object reference, the size of the referenced object is returned. Other data
> types cause a fatal run-time
> error.
Yes, I read that and was concerned. I did some experiments with SizeOf() with
different integer numbers and all were returning 8. But yes, it doesn't look like
the right approach.
>
> Bottom line, I don't think you can figure out the integer size like this.
> What's wrong with just assuming 8 byte integers? I guess sizes 5 to 8
> will be slower with a 32 bit DSDT but why is that a problem?
Right. I guess that would work. I will add a comment to explain why we
are using 8.
Thanks,
Shameer
> MST
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v2 3/7] nvdimm: Use configurable ACPI IO base and size
2020-01-17 17:45 [PATCH v2 0/7] ARM virt: Add NVDIMM support Shameer Kolothum
2020-01-17 17:45 ` [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback Shameer Kolothum
2020-01-17 17:45 ` [PATCH v2 2/7] hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length Shameer Kolothum
@ 2020-01-17 17:45 ` Shameer Kolothum
2020-01-17 17:45 ` [PATCH v2 4/7] hw/arm/virt: Add nvdimm hot-plug infrastructure Shameer Kolothum
` (4 subsequent siblings)
7 siblings, 0 replies; 45+ messages in thread
From: Shameer Kolothum @ 2020-01-17 17:45 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, xiaoguangrong.eric, mst, linuxarm, xuwei5,
shannon.zhaosl, lersek
From: Kwangwoo Lee <kwangwoo.lee@sk.com>
This patch makes IO base and size configurable to create NPIO AML for
ACPI NFIT. Since a different architecture like AArch64 does not use
port-mapped IO, a configurable IO base is required to create correct
mapping of ACPI IO address and size.
Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
hw/acpi/nvdimm.c | 32 ++++++++++++++++++++++----------
hw/i386/acpi-build.c | 6 ++++++
hw/i386/acpi-build.h | 3 +++
hw/i386/pc_piix.c | 2 ++
hw/i386/pc_q35.c | 2 ++
include/hw/mem/nvdimm.h | 3 +++
6 files changed, 38 insertions(+), 10 deletions(-)
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 5e7b8318d0..c28c665b13 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -926,11 +926,13 @@ void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev)
}
void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
+ struct AcpiGenericAddress dsm_io,
FWCfgState *fw_cfg, Object *owner)
{
+ state->dsm_io = dsm_io;
memory_region_init_io(&state->io_mr, owner, &nvdimm_dsm_ops, state,
- "nvdimm-acpi-io", NVDIMM_ACPI_IO_LEN);
- memory_region_add_subregion(io, NVDIMM_ACPI_IO_BASE, &state->io_mr);
+ "nvdimm-acpi-io", dsm_io.bit_width >> 3);
+ memory_region_add_subregion(io, dsm_io.address, &state->io_mr);
state->dsm_mem = g_array_new(false, true /* clear */, 1);
acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));
@@ -959,13 +961,15 @@ void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
#define NVDIMM_QEMU_RSVD_UUID "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62"
-static void nvdimm_build_common_dsm(Aml *dev)
+static void nvdimm_build_common_dsm(Aml *dev,
+ NVDIMMState *nvdimm_state)
{
Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
Aml *whilectx, *offset;
uint8_t byte_list[1];
+ AmlRegionSpace rs;
method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
uuid = aml_arg(0);
@@ -976,9 +980,16 @@ static void nvdimm_build_common_dsm(Aml *dev)
aml_append(method, aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR), dsm_mem));
+ if (nvdimm_state->dsm_io.space_id == AML_AS_SYSTEM_IO) {
+ rs = AML_SYSTEM_IO;
+ } else {
+ rs = AML_SYSTEM_MEMORY;
+ }
+
/* map DSM memory and IO into ACPI namespace. */
- aml_append(method, aml_operation_region(NVDIMM_DSM_IOPORT, AML_SYSTEM_IO,
- aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));
+ aml_append(method, aml_operation_region(NVDIMM_DSM_IOPORT, rs,
+ aml_int(nvdimm_state->dsm_io.address),
+ nvdimm_state->dsm_io.bit_width >> 3));
aml_append(method, aml_operation_region(NVDIMM_DSM_MEMORY,
AML_SYSTEM_MEMORY, dsm_mem, sizeof(NvdimmDsmIn)));
@@ -993,7 +1004,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
field = aml_field(NVDIMM_DSM_IOPORT, AML_DWORD_ACC, AML_NOLOCK,
AML_PRESERVE);
aml_append(field, aml_named_field(NVDIMM_DSM_NOTIFY,
- NVDIMM_ACPI_IO_LEN * BITS_PER_BYTE));
+ nvdimm_state->dsm_io.bit_width));
aml_append(method, field);
/*
@@ -1290,7 +1301,8 @@ static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t ram_slots)
}
static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
- BIOSLinker *linker, GArray *dsm_dma_area,
+ BIOSLinker *linker,
+ NVDIMMState *nvdimm_state,
uint32_t ram_slots)
{
Aml *ssdt, *sb_scope, *dev;
@@ -1318,7 +1330,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
*/
aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));
- nvdimm_build_common_dsm(dev);
+ nvdimm_build_common_dsm(dev, nvdimm_state);
/* 0 is reserved for root device. */
nvdimm_build_device_dsm(dev, 0);
@@ -1337,7 +1349,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
NVDIMM_ACPI_MEM_ADDR);
bios_linker_loader_alloc(linker,
- NVDIMM_DSM_MEM_FILE, dsm_dma_area,
+ NVDIMM_DSM_MEM_FILE, nvdimm_state->dsm_mem,
sizeof(NvdimmDsmIn), false /* high memory */);
bios_linker_loader_add_pointer(linker,
ACPI_BUILD_TABLE_FILE, mem_addr_offset, sizeof(uint32_t),
@@ -1359,7 +1371,7 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
return;
}
- nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
+ nvdimm_build_ssdt(table_offsets, table_data, linker, state,
ram_slots);
device_list = nvdimm_get_device_list();
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index e25df838f0..e75f213470 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -127,6 +127,12 @@ typedef struct FwCfgTPMConfig {
static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg);
+const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio = {
+ .space_id = AML_AS_SYSTEM_IO,
+ .address = NVDIMM_ACPI_IO_BASE,
+ .bit_width = NVDIMM_ACPI_IO_LEN << 3
+};
+
static void init_common_fadt_data(MachineState *ms, Object *o,
AcpiFadtData *data)
{
diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
index 007332e51c..74df5fc612 100644
--- a/hw/i386/acpi-build.h
+++ b/hw/i386/acpi-build.h
@@ -1,6 +1,9 @@
#ifndef HW_I386_ACPI_BUILD_H
#define HW_I386_ACPI_BUILD_H
+#include "hw/acpi/acpi-defs.h"
+
+extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
void acpi_setup(void);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index fa12203079..22032a1f4a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -60,6 +60,7 @@
#include "migration/global_state.h"
#include "migration/misc.h"
#include "sysemu/numa.h"
+#include "hw/i386/acpi-build.h"
#define MAX_IDE_BUS 2
@@ -296,6 +297,7 @@ else {
if (machine->nvdimms_state->is_enabled) {
nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
+ x86_nvdimm_acpi_dsmio,
x86ms->fw_cfg, OBJECT(pcms));
}
}
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 84cf925cf4..01a334b038 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -53,6 +53,7 @@
#include "qapi/error.h"
#include "qemu/error-report.h"
#include "sysemu/numa.h"
+#include "hw/i386/acpi-build.h"
/* ICH9 AHCI has 6 ports */
#define MAX_SATA_PORTS 6
@@ -314,6 +315,7 @@ static void pc_q35_init(MachineState *machine)
if (machine->nvdimms_state->is_enabled) {
nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
+ x86_nvdimm_acpi_dsmio,
x86ms->fw_cfg, OBJECT(pcms));
}
}
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 523a9b3d4a..5fe440861e 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -25,6 +25,7 @@
#include "hw/mem/pc-dimm.h"
#include "hw/acpi/bios-linker-loader.h"
+#include "hw/acpi/aml-build.h"
#define NVDIMM_DEBUG 0
#define nvdimm_debug(fmt, ...) \
@@ -140,10 +141,12 @@ struct NVDIMMState {
*/
int32_t persistence;
char *persistence_string;
+ struct AcpiGenericAddress dsm_io;
};
typedef struct NVDIMMState NVDIMMState;
void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
+ struct AcpiGenericAddress dsm_io,
FWCfgState *fw_cfg, Object *owner);
void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data,
BIOSLinker *linker, NVDIMMState *state,
--
2.17.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v2 4/7] hw/arm/virt: Add nvdimm hot-plug infrastructure
2020-01-17 17:45 [PATCH v2 0/7] ARM virt: Add NVDIMM support Shameer Kolothum
` (2 preceding siblings ...)
2020-01-17 17:45 ` [PATCH v2 3/7] nvdimm: Use configurable ACPI IO base and size Shameer Kolothum
@ 2020-01-17 17:45 ` Shameer Kolothum
2020-01-28 13:02 ` Auger Eric
2020-02-10 13:35 ` Igor Mammedov
2020-01-17 17:45 ` [PATCH v2 5/7] hw/arm/virt: Add nvdimm hotplug support Shameer Kolothum
` (3 subsequent siblings)
7 siblings, 2 replies; 45+ messages in thread
From: Shameer Kolothum @ 2020-01-17 17:45 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, xiaoguangrong.eric, mst, linuxarm, xuwei5,
shannon.zhaosl, lersek
From: Kwangwoo Lee <kwangwoo.lee@sk.com>
Prepare pre-plug and plug handlers for NVDIMM support.
Please note nvdimm_support is not yet enabled.
Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
hw/arm/Kconfig | 1 +
hw/arm/virt-acpi-build.c | 6 ++++++
hw/arm/virt.c | 19 +++++++++++++++++++
hw/mem/Kconfig | 2 +-
include/hw/arm/virt.h | 1 +
5 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index c6e7782580..851dd81289 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -24,6 +24,7 @@ config ARM_VIRT
select DIMM
select ACPI_MEMORY_HOTPLUG
select ACPI_HW_REDUCED
+ select ACPI_NVDIMM
config CHEETAH
bool
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index bd5f771e9b..c51eae549e 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -44,6 +44,7 @@
#include "hw/pci/pcie_host.h"
#include "hw/pci/pci.h"
#include "hw/arm/virt.h"
+#include "hw/mem/nvdimm.h"
#include "sysemu/numa.h"
#include "sysemu/reset.h"
#include "kvm_arm.h"
@@ -839,6 +840,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
}
}
+ if (ms->nvdimms_state->is_enabled) {
+ nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
+ ms->nvdimms_state, ms->ram_slots);
+ }
+
if (its_class_name() && !vmc->no_its) {
acpi_add_table(table_offsets, tables_blob);
build_iort(tables_blob, tables->linker, vms);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 39ab5f47e0..7987c8f5b8 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -143,6 +143,7 @@ static const MemMapEntry base_memmap[] = {
[VIRT_SMMU] = { 0x09050000, 0x00020000 },
[VIRT_PCDIMM_ACPI] = { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
[VIRT_ACPI_GED] = { 0x09080000, ACPI_GED_EVT_SEL_LEN },
+ [VIRT_NVDIMM_ACPI] = { 0x09090000, NVDIMM_ACPI_IO_LEN},
[VIRT_MMIO] = { 0x0a000000, 0x00000200 },
/* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
[VIRT_PLATFORM_BUS] = { 0x0c000000, 0x02000000 },
@@ -1749,6 +1750,18 @@ static void machvirt_init(MachineState *machine)
create_platform_bus(vms);
+ if (machine->nvdimms_state->is_enabled) {
+ const struct AcpiGenericAddress arm_virt_nvdimm_acpi_dsmio = {
+ .space_id = AML_AS_SYSTEM_MEMORY,
+ .address = vms->memmap[VIRT_NVDIMM_ACPI].base,
+ .bit_width = NVDIMM_ACPI_IO_LEN << 3
+ };
+
+ nvdimm_init_acpi_state(machine->nvdimms_state, sysmem,
+ arm_virt_nvdimm_acpi_dsmio,
+ vms->fw_cfg, OBJECT(vms));
+ }
+
vms->bootinfo.ram_size = machine->ram_size;
vms->bootinfo.nb_cpus = smp_cpus;
vms->bootinfo.board_id = -1;
@@ -1936,6 +1949,8 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
{
HotplugHandlerClass *hhc;
VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+ MachineState *ms = MACHINE(hotplug_dev);
+ bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
Error *local_err = NULL;
pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), &local_err);
@@ -1943,6 +1958,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
goto out;
}
+ if (is_nvdimm) {
+ nvdimm_plug(ms->nvdimms_state);
+ }
+
hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &error_abort);
out:
diff --git a/hw/mem/Kconfig b/hw/mem/Kconfig
index 620fd4cb59..0d5f8f321a 100644
--- a/hw/mem/Kconfig
+++ b/hw/mem/Kconfig
@@ -8,4 +8,4 @@ config MEM_DEVICE
config NVDIMM
bool
default y
- depends on PC
+ depends on PC || ARM_VIRT
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 38f0c33c77..9c9eaaa89d 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -79,6 +79,7 @@ enum {
VIRT_SECURE_MEM,
VIRT_PCDIMM_ACPI,
VIRT_ACPI_GED,
+ VIRT_NVDIMM_ACPI,
VIRT_LOWMEMMAP_LAST,
};
--
2.17.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 4/7] hw/arm/virt: Add nvdimm hot-plug infrastructure
2020-01-17 17:45 ` [PATCH v2 4/7] hw/arm/virt: Add nvdimm hot-plug infrastructure Shameer Kolothum
@ 2020-01-28 13:02 ` Auger Eric
2020-02-10 13:35 ` Igor Mammedov
1 sibling, 0 replies; 45+ messages in thread
From: Auger Eric @ 2020-01-28 13:02 UTC (permalink / raw)
To: Shameer Kolothum, qemu-devel, qemu-arm, imammedo
Cc: peter.maydell, xiaoguangrong.eric, mst, linuxarm, xuwei5,
shannon.zhaosl, lersek
Hi Shameer,
On 1/17/20 6:45 PM, Shameer Kolothum wrote:
> From: Kwangwoo Lee <kwangwoo.lee@sk.com>
>
> Prepare pre-plug and plug handlers for NVDIMM support.
> Please note nvdimm_support is not yet enabled.
>
> Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> hw/arm/Kconfig | 1 +
> hw/arm/virt-acpi-build.c | 6 ++++++
> hw/arm/virt.c | 19 +++++++++++++++++++
> hw/mem/Kconfig | 2 +-
> include/hw/arm/virt.h | 1 +
> 5 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index c6e7782580..851dd81289 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -24,6 +24,7 @@ config ARM_VIRT
> select DIMM
> select ACPI_MEMORY_HOTPLUG
> select ACPI_HW_REDUCED
> + select ACPI_NVDIMM
>
> config CHEETAH
> bool
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index bd5f771e9b..c51eae549e 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -44,6 +44,7 @@
> #include "hw/pci/pcie_host.h"
> #include "hw/pci/pci.h"
> #include "hw/arm/virt.h"
> +#include "hw/mem/nvdimm.h"
> #include "sysemu/numa.h"
> #include "sysemu/reset.h"
> #include "kvm_arm.h"
> @@ -839,6 +840,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> }
> }
>
> + if (ms->nvdimms_state->is_enabled) {
> + nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> + ms->nvdimms_state, ms->ram_slots);
> + }
> +
> if (its_class_name() && !vmc->no_its) {
> acpi_add_table(table_offsets, tables_blob);
> build_iort(tables_blob, tables->linker, vms);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 39ab5f47e0..7987c8f5b8 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -143,6 +143,7 @@ static const MemMapEntry base_memmap[] = {
> [VIRT_SMMU] = { 0x09050000, 0x00020000 },
> [VIRT_PCDIMM_ACPI] = { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
> [VIRT_ACPI_GED] = { 0x09080000, ACPI_GED_EVT_SEL_LEN },
> + [VIRT_NVDIMM_ACPI] = { 0x09090000, NVDIMM_ACPI_IO_LEN},
> [VIRT_MMIO] = { 0x0a000000, 0x00000200 },
> /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
> [VIRT_PLATFORM_BUS] = { 0x0c000000, 0x02000000 },
> @@ -1749,6 +1750,18 @@ static void machvirt_init(MachineState *machine)
>
> create_platform_bus(vms);
>
> + if (machine->nvdimms_state->is_enabled) {
> + const struct AcpiGenericAddress arm_virt_nvdimm_acpi_dsmio = {
> + .space_id = AML_AS_SYSTEM_MEMORY,
> + .address = vms->memmap[VIRT_NVDIMM_ACPI].base,
> + .bit_width = NVDIMM_ACPI_IO_LEN << 3
> + };
> +
> + nvdimm_init_acpi_state(machine->nvdimms_state, sysmem,
> + arm_virt_nvdimm_acpi_dsmio,
> + vms->fw_cfg, OBJECT(vms));
> + }
> +
> vms->bootinfo.ram_size = machine->ram_size;
> vms->bootinfo.nb_cpus = smp_cpus;
> vms->bootinfo.board_id = -1;
> @@ -1936,6 +1949,8 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
> {
> HotplugHandlerClass *hhc;
> VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> + MachineState *ms = MACHINE(hotplug_dev);
> + bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> Error *local_err = NULL;
>
> pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), &local_err);
> @@ -1943,6 +1958,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
> goto out;
> }
>
> + if (is_nvdimm) {
> + nvdimm_plug(ms->nvdimms_state);
> + }
> +
> hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &error_abort);
we have a small conflict here due to
53eccc7034 hw/arm: Use helper function to trigger hotplug handler plug
Thanks
Eric
> out:
> diff --git a/hw/mem/Kconfig b/hw/mem/Kconfig
> index 620fd4cb59..0d5f8f321a 100644
> --- a/hw/mem/Kconfig
> +++ b/hw/mem/Kconfig
> @@ -8,4 +8,4 @@ config MEM_DEVICE
> config NVDIMM
> bool
> default y
> - depends on PC
> + depends on PC || ARM_VIRT
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 38f0c33c77..9c9eaaa89d 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -79,6 +79,7 @@ enum {
> VIRT_SECURE_MEM,
> VIRT_PCDIMM_ACPI,
> VIRT_ACPI_GED,
> + VIRT_NVDIMM_ACPI,
> VIRT_LOWMEMMAP_LAST,
> };
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 4/7] hw/arm/virt: Add nvdimm hot-plug infrastructure
2020-01-17 17:45 ` [PATCH v2 4/7] hw/arm/virt: Add nvdimm hot-plug infrastructure Shameer Kolothum
2020-01-28 13:02 ` Auger Eric
@ 2020-02-10 13:35 ` Igor Mammedov
1 sibling, 0 replies; 45+ messages in thread
From: Igor Mammedov @ 2020-02-10 13:35 UTC (permalink / raw)
To: Shameer Kolothum
Cc: peter.maydell, xiaoguangrong.eric, mst, shannon.zhaosl,
qemu-devel, xuwei5, linuxarm, eric.auger, qemu-arm, lersek
On Fri, 17 Jan 2020 17:45:19 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> From: Kwangwoo Lee <kwangwoo.lee@sk.com>
>
> Prepare pre-plug and plug handlers for NVDIMM support.
> Please note nvdimm_support is not yet enabled.
looks fine to me,
except of commit message which should be updated to reflect
what patch actually does.
>
> Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> hw/arm/Kconfig | 1 +
> hw/arm/virt-acpi-build.c | 6 ++++++
> hw/arm/virt.c | 19 +++++++++++++++++++
> hw/mem/Kconfig | 2 +-
> include/hw/arm/virt.h | 1 +
> 5 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index c6e7782580..851dd81289 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -24,6 +24,7 @@ config ARM_VIRT
> select DIMM
> select ACPI_MEMORY_HOTPLUG
> select ACPI_HW_REDUCED
> + select ACPI_NVDIMM
>
> config CHEETAH
> bool
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index bd5f771e9b..c51eae549e 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -44,6 +44,7 @@
> #include "hw/pci/pcie_host.h"
> #include "hw/pci/pci.h"
> #include "hw/arm/virt.h"
> +#include "hw/mem/nvdimm.h"
> #include "sysemu/numa.h"
> #include "sysemu/reset.h"
> #include "kvm_arm.h"
> @@ -839,6 +840,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> }
> }
>
> + if (ms->nvdimms_state->is_enabled) {
> + nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
> + ms->nvdimms_state, ms->ram_slots);
> + }
> +
> if (its_class_name() && !vmc->no_its) {
> acpi_add_table(table_offsets, tables_blob);
> build_iort(tables_blob, tables->linker, vms);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 39ab5f47e0..7987c8f5b8 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -143,6 +143,7 @@ static const MemMapEntry base_memmap[] = {
> [VIRT_SMMU] = { 0x09050000, 0x00020000 },
> [VIRT_PCDIMM_ACPI] = { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
> [VIRT_ACPI_GED] = { 0x09080000, ACPI_GED_EVT_SEL_LEN },
> + [VIRT_NVDIMM_ACPI] = { 0x09090000, NVDIMM_ACPI_IO_LEN},
> [VIRT_MMIO] = { 0x0a000000, 0x00000200 },
> /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
> [VIRT_PLATFORM_BUS] = { 0x0c000000, 0x02000000 },
> @@ -1749,6 +1750,18 @@ static void machvirt_init(MachineState *machine)
>
> create_platform_bus(vms);
>
> + if (machine->nvdimms_state->is_enabled) {
> + const struct AcpiGenericAddress arm_virt_nvdimm_acpi_dsmio = {
> + .space_id = AML_AS_SYSTEM_MEMORY,
> + .address = vms->memmap[VIRT_NVDIMM_ACPI].base,
> + .bit_width = NVDIMM_ACPI_IO_LEN << 3
> + };
> +
> + nvdimm_init_acpi_state(machine->nvdimms_state, sysmem,
> + arm_virt_nvdimm_acpi_dsmio,
> + vms->fw_cfg, OBJECT(vms));
> + }
> +
> vms->bootinfo.ram_size = machine->ram_size;
> vms->bootinfo.nb_cpus = smp_cpus;
> vms->bootinfo.board_id = -1;
> @@ -1936,6 +1949,8 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
> {
> HotplugHandlerClass *hhc;
> VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> + MachineState *ms = MACHINE(hotplug_dev);
> + bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> Error *local_err = NULL;
>
> pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), &local_err);
> @@ -1943,6 +1958,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
> goto out;
> }
>
> + if (is_nvdimm) {
> + nvdimm_plug(ms->nvdimms_state);
> + }
> +
> hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, &error_abort);
> out:
> diff --git a/hw/mem/Kconfig b/hw/mem/Kconfig
> index 620fd4cb59..0d5f8f321a 100644
> --- a/hw/mem/Kconfig
> +++ b/hw/mem/Kconfig
> @@ -8,4 +8,4 @@ config MEM_DEVICE
> config NVDIMM
> bool
> default y
> - depends on PC
> + depends on PC || ARM_VIRT
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 38f0c33c77..9c9eaaa89d 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -79,6 +79,7 @@ enum {
> VIRT_SECURE_MEM,
> VIRT_PCDIMM_ACPI,
> VIRT_ACPI_GED,
> + VIRT_NVDIMM_ACPI,
> VIRT_LOWMEMMAP_LAST,
> };
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v2 5/7] hw/arm/virt: Add nvdimm hotplug support
2020-01-17 17:45 [PATCH v2 0/7] ARM virt: Add NVDIMM support Shameer Kolothum
` (3 preceding siblings ...)
2020-01-17 17:45 ` [PATCH v2 4/7] hw/arm/virt: Add nvdimm hot-plug infrastructure Shameer Kolothum
@ 2020-01-17 17:45 ` Shameer Kolothum
2020-01-28 16:29 ` Auger Eric
2020-02-10 13:43 ` Igor Mammedov
2020-01-17 17:45 ` [PATCH v2 6/7] tests: Update ACPI tables list for upcoming arm/virt test changes Shameer Kolothum
` (2 subsequent siblings)
7 siblings, 2 replies; 45+ messages in thread
From: Shameer Kolothum @ 2020-01-17 17:45 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, xiaoguangrong.eric, mst, linuxarm, xuwei5,
shannon.zhaosl, lersek
This adds support for nvdimm hotplug events through GED
and enables nvdimm for the arm/virt. Now Guests with ACPI
can have both cold and hot plug of nvdimms.
Hot removal functionality is not yet supported.
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
docs/specs/acpi_hw_reduced_hotplug.rst | 1 +
hw/acpi/generic_event_device.c | 13 +++++++++++++
hw/arm/virt.c | 16 +++++++++++-----
include/hw/acpi/generic_event_device.h | 1 +
4 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/docs/specs/acpi_hw_reduced_hotplug.rst b/docs/specs/acpi_hw_reduced_hotplug.rst
index 911a98255b..e3abe975bf 100644
--- a/docs/specs/acpi_hw_reduced_hotplug.rst
+++ b/docs/specs/acpi_hw_reduced_hotplug.rst
@@ -63,6 +63,7 @@ GED IO interface (4 byte access)
bits:
0: Memory hotplug event
1: System power down event
+ 2: NVDIMM hotplug event
2-31: Reserved
**write_access:**
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 9cee90cc70..ad1b684304 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -16,6 +16,7 @@
#include "hw/acpi/generic_event_device.h"
#include "hw/irq.h"
#include "hw/mem/pc-dimm.h"
+#include "hw/mem/nvdimm.h"
#include "hw/qdev-properties.h"
#include "migration/vmstate.h"
#include "qemu/error-report.h"
@@ -23,6 +24,7 @@
static const uint32_t ged_supported_events[] = {
ACPI_GED_MEM_HOTPLUG_EVT,
ACPI_GED_PWR_DOWN_EVT,
+ ACPI_GED_NVDIMM_HOTPLUG_EVT,
};
/*
@@ -110,6 +112,11 @@ void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev,
aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
aml_int(0x80)));
break;
+ case ACPI_GED_NVDIMM_HOTPLUG_EVT:
+ aml_append(if_ctx,
+ aml_notify(aml_name("\\_SB.NVDR"),
+ aml_int(0x80)));
+ break;
default:
/*
* Please make sure all the events in ged_supported_events[]
@@ -175,7 +182,11 @@ static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
AcpiGedState *s = ACPI_GED(hotplug_dev);
if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+ if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+ nvdimm_acpi_plug_cb(hotplug_dev, dev);
+ } else {
acpi_memory_plug_cb(hotplug_dev, &s->memhp_state, dev, errp);
+ }
} else {
error_setg(errp, "virt: device plug request for unsupported device"
" type: %s", object_get_typename(OBJECT(dev)));
@@ -192,6 +203,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
sel = ACPI_GED_MEM_HOTPLUG_EVT;
} else if (ev & ACPI_POWER_DOWN_STATUS) {
sel = ACPI_GED_PWR_DOWN_EVT;
+ } else if (ev & ACPI_NVDIMM_HOTPLUG_STATUS) {
+ sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
} else {
/* Unknown event. Return without generating interrupt. */
warn_report("GED: Unsupported event %d. No irq injected", ev);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7987c8f5b8..5ea2584491 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -543,6 +543,10 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
event |= ACPI_GED_MEM_HOTPLUG_EVT;
}
+ if (ms->nvdimms_state->is_enabled) {
+ event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;
+ }
+
dev = qdev_create(NULL, TYPE_ACPI_GED);
qdev_prop_set_uint32(dev, "ged-event", event);
@@ -1928,19 +1932,20 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
Error **errp)
{
VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+ const MachineState *ms = MACHINE(hotplug_dev);
const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
- if (is_nvdimm) {
- error_setg(errp, "nvdimm is not yet supported");
- return;
- }
-
if (!vms->acpi_dev) {
error_setg(errp,
"memory hotplug is not enabled: missing acpi-ged device");
return;
}
+ if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
+ error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
+ return;
+ }
+
pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
}
@@ -2071,6 +2076,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
hc->plug = virt_machine_device_plug_cb;
hc->unplug_request = virt_machine_device_unplug_request_cb;
mc->numa_mem_supported = true;
+ mc->nvdimm_supported = true;
mc->auto_enable_numa_with_memhp = true;
}
diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
index d157eac088..9eb86ca4fd 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -82,6 +82,7 @@
*/
#define ACPI_GED_MEM_HOTPLUG_EVT 0x1
#define ACPI_GED_PWR_DOWN_EVT 0x2
+#define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
typedef struct GEDState {
MemoryRegion io;
--
2.17.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 5/7] hw/arm/virt: Add nvdimm hotplug support
2020-01-17 17:45 ` [PATCH v2 5/7] hw/arm/virt: Add nvdimm hotplug support Shameer Kolothum
@ 2020-01-28 16:29 ` Auger Eric
2020-02-10 13:43 ` Igor Mammedov
1 sibling, 0 replies; 45+ messages in thread
From: Auger Eric @ 2020-01-28 16:29 UTC (permalink / raw)
To: Shameer Kolothum, qemu-devel, qemu-arm, imammedo
Cc: peter.maydell, xiaoguangrong.eric, mst, linuxarm, xuwei5,
shannon.zhaosl, lersek
Hi Shameer,
On 1/17/20 6:45 PM, Shameer Kolothum wrote:
> This adds support for nvdimm hotplug events through GED
> and enables nvdimm for the arm/virt. Now Guests with ACPI
> can have both cold and hot plug of nvdimms.
>
> Hot removal functionality is not yet supported.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
> ---
> docs/specs/acpi_hw_reduced_hotplug.rst | 1 +
> hw/acpi/generic_event_device.c | 13 +++++++++++++
> hw/arm/virt.c | 16 +++++++++++-----
> include/hw/acpi/generic_event_device.h | 1 +
> 4 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/docs/specs/acpi_hw_reduced_hotplug.rst b/docs/specs/acpi_hw_reduced_hotplug.rst
> index 911a98255b..e3abe975bf 100644
> --- a/docs/specs/acpi_hw_reduced_hotplug.rst
> +++ b/docs/specs/acpi_hw_reduced_hotplug.rst
> @@ -63,6 +63,7 @@ GED IO interface (4 byte access)
> bits:
> 0: Memory hotplug event
> 1: System power down event
> + 2: NVDIMM hotplug event
> 2-31: Reserved
>
> **write_access:**
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index 9cee90cc70..ad1b684304 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -16,6 +16,7 @@
> #include "hw/acpi/generic_event_device.h"
> #include "hw/irq.h"
> #include "hw/mem/pc-dimm.h"
> +#include "hw/mem/nvdimm.h"
> #include "hw/qdev-properties.h"
> #include "migration/vmstate.h"
> #include "qemu/error-report.h"
> @@ -23,6 +24,7 @@
> static const uint32_t ged_supported_events[] = {
> ACPI_GED_MEM_HOTPLUG_EVT,
> ACPI_GED_PWR_DOWN_EVT,
> + ACPI_GED_NVDIMM_HOTPLUG_EVT,
> };
>
> /*
> @@ -110,6 +112,11 @@ void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev,
> aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
> aml_int(0x80)));
> break;
> + case ACPI_GED_NVDIMM_HOTPLUG_EVT:
> + aml_append(if_ctx,
> + aml_notify(aml_name("\\_SB.NVDR"),
> + aml_int(0x80)));
> + break;
> default:
> /*
> * Please make sure all the events in ged_supported_events[]
> @@ -175,7 +182,11 @@ static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
> AcpiGedState *s = ACPI_GED(hotplug_dev);
>
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> + if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> + nvdimm_acpi_plug_cb(hotplug_dev, dev);
> + } else {
> acpi_memory_plug_cb(hotplug_dev, &s->memhp_state, dev, errp);
> + }
> } else {
> error_setg(errp, "virt: device plug request for unsupported device"
> " type: %s", object_get_typename(OBJECT(dev)));
> @@ -192,6 +203,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
> sel = ACPI_GED_MEM_HOTPLUG_EVT;
> } else if (ev & ACPI_POWER_DOWN_STATUS) {
> sel = ACPI_GED_PWR_DOWN_EVT;
> + } else if (ev & ACPI_NVDIMM_HOTPLUG_STATUS) {
> + sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
> } else {
> /* Unknown event. Return without generating interrupt. */
> warn_report("GED: Unsupported event %d. No irq injected", ev);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 7987c8f5b8..5ea2584491 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -543,6 +543,10 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
> event |= ACPI_GED_MEM_HOTPLUG_EVT;
> }
>
> + if (ms->nvdimms_state->is_enabled) {
> + event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;
> + }
> +
> dev = qdev_create(NULL, TYPE_ACPI_GED);
> qdev_prop_set_uint32(dev, "ged-event", event);
>
> @@ -1928,19 +1932,20 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> Error **errp)
> {
> VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> + const MachineState *ms = MACHINE(hotplug_dev);
> const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>
> - if (is_nvdimm) {
> - error_setg(errp, "nvdimm is not yet supported");
> - return;
> - }
> -
> if (!vms->acpi_dev) {
> error_setg(errp,
> "memory hotplug is not enabled: missing acpi-ged device");
> return;
> }
>
> + if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
> + error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> + return;
> + }
> +
> pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
> }
>
> @@ -2071,6 +2076,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
> hc->plug = virt_machine_device_plug_cb;
> hc->unplug_request = virt_machine_device_unplug_request_cb;
> mc->numa_mem_supported = true;
> + mc->nvdimm_supported = true;
> mc->auto_enable_numa_with_memhp = true;
> }
>
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index d157eac088..9eb86ca4fd 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -82,6 +82,7 @@
> */
> #define ACPI_GED_MEM_HOTPLUG_EVT 0x1
> #define ACPI_GED_PWR_DOWN_EVT 0x2
> +#define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
>
> typedef struct GEDState {
> MemoryRegion io;
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 5/7] hw/arm/virt: Add nvdimm hotplug support
2020-01-17 17:45 ` [PATCH v2 5/7] hw/arm/virt: Add nvdimm hotplug support Shameer Kolothum
2020-01-28 16:29 ` Auger Eric
@ 2020-02-10 13:43 ` Igor Mammedov
1 sibling, 0 replies; 45+ messages in thread
From: Igor Mammedov @ 2020-02-10 13:43 UTC (permalink / raw)
To: Shameer Kolothum
Cc: peter.maydell, xiaoguangrong.eric, mst, shannon.zhaosl,
qemu-devel, xuwei5, linuxarm, eric.auger, qemu-arm, lersek
On Fri, 17 Jan 2020 17:45:20 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> This adds support for nvdimm hotplug events through GED
> and enables nvdimm for the arm/virt. Now Guests with ACPI
> can have both cold and hot plug of nvdimms.
>
> Hot removal functionality is not yet supported.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> docs/specs/acpi_hw_reduced_hotplug.rst | 1 +
> hw/acpi/generic_event_device.c | 13 +++++++++++++
> hw/arm/virt.c | 16 +++++++++++-----
> include/hw/acpi/generic_event_device.h | 1 +
> 4 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/docs/specs/acpi_hw_reduced_hotplug.rst b/docs/specs/acpi_hw_reduced_hotplug.rst
> index 911a98255b..e3abe975bf 100644
> --- a/docs/specs/acpi_hw_reduced_hotplug.rst
> +++ b/docs/specs/acpi_hw_reduced_hotplug.rst
> @@ -63,6 +63,7 @@ GED IO interface (4 byte access)
> bits:
> 0: Memory hotplug event
> 1: System power down event
> + 2: NVDIMM hotplug event
> 2-31: Reserved
>
> **write_access:**
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index 9cee90cc70..ad1b684304 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -16,6 +16,7 @@
> #include "hw/acpi/generic_event_device.h"
> #include "hw/irq.h"
> #include "hw/mem/pc-dimm.h"
> +#include "hw/mem/nvdimm.h"
> #include "hw/qdev-properties.h"
> #include "migration/vmstate.h"
> #include "qemu/error-report.h"
> @@ -23,6 +24,7 @@
> static const uint32_t ged_supported_events[] = {
> ACPI_GED_MEM_HOTPLUG_EVT,
> ACPI_GED_PWR_DOWN_EVT,
> + ACPI_GED_NVDIMM_HOTPLUG_EVT,
> };
>
> /*
> @@ -110,6 +112,11 @@ void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev,
> aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
> aml_int(0x80)));
> break;
> + case ACPI_GED_NVDIMM_HOTPLUG_EVT:
> + aml_append(if_ctx,
> + aml_notify(aml_name("\\_SB.NVDR"),
> + aml_int(0x80)));
> + break;
> default:
> /*
> * Please make sure all the events in ged_supported_events[]
> @@ -175,7 +182,11 @@ static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
> AcpiGedState *s = ACPI_GED(hotplug_dev);
>
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> + if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> + nvdimm_acpi_plug_cb(hotplug_dev, dev);
> + } else {
> acpi_memory_plug_cb(hotplug_dev, &s->memhp_state, dev, errp);
> + }
> } else {
> error_setg(errp, "virt: device plug request for unsupported device"
> " type: %s", object_get_typename(OBJECT(dev)));
> @@ -192,6 +203,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
> sel = ACPI_GED_MEM_HOTPLUG_EVT;
> } else if (ev & ACPI_POWER_DOWN_STATUS) {
> sel = ACPI_GED_PWR_DOWN_EVT;
> + } else if (ev & ACPI_NVDIMM_HOTPLUG_STATUS) {
> + sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
> } else {
> /* Unknown event. Return without generating interrupt. */
> warn_report("GED: Unsupported event %d. No irq injected", ev);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 7987c8f5b8..5ea2584491 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -543,6 +543,10 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
> event |= ACPI_GED_MEM_HOTPLUG_EVT;
> }
>
> + if (ms->nvdimms_state->is_enabled) {
> + event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;
> + }
> +
> dev = qdev_create(NULL, TYPE_ACPI_GED);
> qdev_prop_set_uint32(dev, "ged-event", event);
>
> @@ -1928,19 +1932,20 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> Error **errp)
> {
> VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> + const MachineState *ms = MACHINE(hotplug_dev);
> const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>
> - if (is_nvdimm) {
> - error_setg(errp, "nvdimm is not yet supported");
> - return;
> - }
> -
> if (!vms->acpi_dev) {
> error_setg(errp,
> "memory hotplug is not enabled: missing acpi-ged device");
> return;
> }
>
> + if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
> + error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
s/missing 'nvdimm' in '-M'/add 'nvdimm=on' to '-M'/
> + return;
> + }
> +
> pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
> }
>
> @@ -2071,6 +2076,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
> hc->plug = virt_machine_device_plug_cb;
> hc->unplug_request = virt_machine_device_unplug_request_cb;
> mc->numa_mem_supported = true;
> + mc->nvdimm_supported = true;
> mc->auto_enable_numa_with_memhp = true;
> }
>
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index d157eac088..9eb86ca4fd 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -82,6 +82,7 @@
> */
> #define ACPI_GED_MEM_HOTPLUG_EVT 0x1
> #define ACPI_GED_PWR_DOWN_EVT 0x2
> +#define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
>
> typedef struct GEDState {
> MemoryRegion io;
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v2 6/7] tests: Update ACPI tables list for upcoming arm/virt test changes
2020-01-17 17:45 [PATCH v2 0/7] ARM virt: Add NVDIMM support Shameer Kolothum
` (4 preceding siblings ...)
2020-01-17 17:45 ` [PATCH v2 5/7] hw/arm/virt: Add nvdimm hotplug support Shameer Kolothum
@ 2020-01-17 17:45 ` Shameer Kolothum
2020-01-17 17:45 ` [PATCH v2 7/7] tests/bios-tables-test: Update arm/virt memhp test Shameer Kolothum
2020-01-28 15:29 ` [PATCH v2 0/7] ARM virt: Add NVDIMM support Auger Eric
7 siblings, 0 replies; 45+ messages in thread
From: Shameer Kolothum @ 2020-01-17 17:45 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, xiaoguangrong.eric, mst, linuxarm, xuwei5,
shannon.zhaosl, lersek
This is in preparation to update test_acpi_virt_tcg_memhp()
with pc-dimm and nvdimm. Update the bios-tables-test-allowed-diff.h
with the affected ACPI tables so that "make check" doesn't fail.
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
tests/qtest/bios-tables-test-allowed-diff.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index eb8bae1407..862c49e675 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,3 +1,6 @@
/* List of comma-separated changed AML files to ignore */
"tests/data/acpi/pc/SSDT.dimmpxm",
"tests/data/acpi/q35/SSDT.dimmpxm",
+"tests/data/acpi/virt/DSDT.memhp",
+"tests/data/acpi/virt/SSDT.memhp",
+"tests/data/acpi/virt/NFIT.memhp",
--
2.17.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v2 7/7] tests/bios-tables-test: Update arm/virt memhp test
2020-01-17 17:45 [PATCH v2 0/7] ARM virt: Add NVDIMM support Shameer Kolothum
` (5 preceding siblings ...)
2020-01-17 17:45 ` [PATCH v2 6/7] tests: Update ACPI tables list for upcoming arm/virt test changes Shameer Kolothum
@ 2020-01-17 17:45 ` Shameer Kolothum
2020-01-28 16:29 ` Auger Eric
2020-02-11 10:20 ` Michael S. Tsirkin
2020-01-28 15:29 ` [PATCH v2 0/7] ARM virt: Add NVDIMM support Auger Eric
7 siblings, 2 replies; 45+ messages in thread
From: Shameer Kolothum @ 2020-01-17 17:45 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, xiaoguangrong.eric, mst, linuxarm, xuwei5,
shannon.zhaosl, lersek
Since we now have both pc-dimm and nvdimm support, update
test_acpi_virt_tcg_memhp() to include those.
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
tests/data/acpi/virt/NFIT.memhp | 0
tests/data/acpi/virt/SSDT.memhp | 0
tests/qtest/bios-tables-test.c | 9 +++++++--
3 files changed, 7 insertions(+), 2 deletions(-)
create mode 100644 tests/data/acpi/virt/NFIT.memhp
create mode 100644 tests/data/acpi/virt/SSDT.memhp
diff --git a/tests/data/acpi/virt/NFIT.memhp b/tests/data/acpi/virt/NFIT.memhp
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/data/acpi/virt/SSDT.memhp b/tests/data/acpi/virt/SSDT.memhp
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index f1ac2d7e96..695d2e7fac 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -913,12 +913,17 @@ static void test_acpi_virt_tcg_memhp(void)
};
data.variant = ".memhp";
- test_acpi_one(" -cpu cortex-a57"
+ test_acpi_one(" -machine nvdimm=on"
+ " -cpu cortex-a57"
" -m 256M,slots=3,maxmem=1G"
" -object memory-backend-ram,id=ram0,size=128M"
" -object memory-backend-ram,id=ram1,size=128M"
" -numa node,memdev=ram0 -numa node,memdev=ram1"
- " -numa dist,src=0,dst=1,val=21",
+ " -numa dist,src=0,dst=1,val=21"
+ " -object memory-backend-ram,id=ram2,size=128M"
+ " -object memory-backend-ram,id=nvm0,size=128M"
+ " -device pc-dimm,id=dimm0,memdev=ram2,node=0"
+ " -device nvdimm,id=dimm1,memdev=nvm0,node=1",
&data);
free_test_data(&data);
--
2.17.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v2 7/7] tests/bios-tables-test: Update arm/virt memhp test
2020-01-17 17:45 ` [PATCH v2 7/7] tests/bios-tables-test: Update arm/virt memhp test Shameer Kolothum
@ 2020-01-28 16:29 ` Auger Eric
2020-01-29 10:35 ` Shameerali Kolothum Thodi
2020-02-11 10:20 ` Michael S. Tsirkin
1 sibling, 1 reply; 45+ messages in thread
From: Auger Eric @ 2020-01-28 16:29 UTC (permalink / raw)
To: Shameer Kolothum, qemu-devel, qemu-arm, imammedo
Cc: peter.maydell, xiaoguangrong.eric, mst, linuxarm, xuwei5,
shannon.zhaosl, lersek
Hi Shameer,
On 1/17/20 6:45 PM, Shameer Kolothum wrote:
> Since we now have both pc-dimm and nvdimm support, update
> test_acpi_virt_tcg_memhp() to include those.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> tests/data/acpi/virt/NFIT.memhp | 0
> tests/data/acpi/virt/SSDT.memhp | 0
Is it normal to have those 2 above void files? I lost track about the
process.
> tests/qtest/bios-tables-test.c | 9 +++++++--
> 3 files changed, 7 insertions(+), 2 deletions(-)
> create mode 100644 tests/data/acpi/virt/NFIT.memhp
> create mode 100644 tests/data/acpi/virt/SSDT.memhp
>
> diff --git a/tests/data/acpi/virt/NFIT.memhp b/tests/data/acpi/virt/NFIT.memhp
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/tests/data/acpi/virt/SSDT.memhp b/tests/data/acpi/virt/SSDT.memhp
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index f1ac2d7e96..695d2e7fac 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -913,12 +913,17 @@ static void test_acpi_virt_tcg_memhp(void)
> };
>
> data.variant = ".memhp";
> - test_acpi_one(" -cpu cortex-a57"
> + test_acpi_one(" -machine nvdimm=on"
nit: maybe keep the same order as before ...
> + " -cpu cortex-a57"
> " -m 256M,slots=3,maxmem=1G"
and simply add ,nvdimm=on to above line.
> " -object memory-backend-ram,id=ram0,size=128M"
> " -object memory-backend-ram,id=ram1,size=128M"
> " -numa node,memdev=ram0 -numa node,memdev=ram1"
> - " -numa dist,src=0,dst=1,val=21",
> + " -numa dist,src=0,dst=1,val=21"
> + " -object memory-backend-ram,id=ram2,size=128M"
> + " -object memory-backend-ram,id=nvm0,size=128M"
> + " -device pc-dimm,id=dimm0,memdev=ram2,node=0"
> + " -device nvdimm,id=dimm1,memdev=nvm0,node=1",
> &data);
>
> free_test_data(&data);
>
Thanks
Eric
^ permalink raw reply [flat|nested] 45+ messages in thread* RE: [PATCH v2 7/7] tests/bios-tables-test: Update arm/virt memhp test
2020-01-28 16:29 ` Auger Eric
@ 2020-01-29 10:35 ` Shameerali Kolothum Thodi
2020-01-29 13:01 ` Auger Eric
0 siblings, 1 reply; 45+ messages in thread
From: Shameerali Kolothum Thodi @ 2020-01-29 10:35 UTC (permalink / raw)
To: Auger Eric, qemu-devel, qemu-arm, imammedo
Cc: peter.maydell, xiaoguangrong.eric, mst, Linuxarm, shannon.zhaosl,
xuwei (O),
lersek
Hi Eric,
> -----Original Message-----
> From: Qemu-devel
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> u.org] On Behalf Of Auger Eric
> Sent: 28 January 2020 16:29
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> qemu-devel@nongnu.org; qemu-arm@nongnu.org; imammedo@redhat.com
> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
> mst@redhat.com; Linuxarm <linuxarm@huawei.com>; xuwei (O)
> <xuwei5@huawei.com>; shannon.zhaosl@gmail.com; lersek@redhat.com
> Subject: Re: [PATCH v2 7/7] tests/bios-tables-test: Update arm/virt memhp
> test
>
> Hi Shameer,
>
> On 1/17/20 6:45 PM, Shameer Kolothum wrote:
> > Since we now have both pc-dimm and nvdimm support, update
> > test_acpi_virt_tcg_memhp() to include those.
> >
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> > tests/data/acpi/virt/NFIT.memhp | 0
> > tests/data/acpi/virt/SSDT.memhp | 0
> Is it normal to have those 2 above void files? I lost track about the
> process.
I guess so :). From tests/qtest/bios-tables-test.c,
/*
* How to add or update the tests:
* Contributor:
* 1. add empty files for new tables, if any, under tests/data/acpi
* 2. list any changed files in tests/bios-tables-test-allowed-diff.h
* 3. commit the above *before* making changes that affect the tables
...
After reading that again, I am not sure those empty files can be in this
Patch or not. I can move it to 6/7.
> > tests/qtest/bios-tables-test.c | 9 +++++++--
> > 3 files changed, 7 insertions(+), 2 deletions(-)
> > create mode 100644 tests/data/acpi/virt/NFIT.memhp
> > create mode 100644 tests/data/acpi/virt/SSDT.memhp
> >
> > diff --git a/tests/data/acpi/virt/NFIT.memhp
> b/tests/data/acpi/virt/NFIT.memhp
> > new file mode 100644
> > index 0000000000..e69de29bb2
> > diff --git a/tests/data/acpi/virt/SSDT.memhp
> b/tests/data/acpi/virt/SSDT.memhp
> > new file mode 100644
> > index 0000000000..e69de29bb2
> > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> > index f1ac2d7e96..695d2e7fac 100644
> > --- a/tests/qtest/bios-tables-test.c
> > +++ b/tests/qtest/bios-tables-test.c
> > @@ -913,12 +913,17 @@ static void test_acpi_virt_tcg_memhp(void)
> > };
> >
> > data.variant = ".memhp";
> > - test_acpi_one(" -cpu cortex-a57"
> > + test_acpi_one(" -machine nvdimm=on"
> nit: maybe keep the same order as before ...
> > + " -cpu cortex-a57"
> > " -m 256M,slots=3,maxmem=1G"
> and simply add ,nvdimm=on to above line.
> > " -object memory-backend-ram,id=ram0,size=128M"
> > " -object memory-backend-ram,id=ram1,size=128M"
> > " -numa node,memdev=ram0 -numa
> node,memdev=ram1"
> > - " -numa dist,src=0,dst=1,val=21",
> > + " -numa dist,src=0,dst=1,val=21"
> > + " -object memory-backend-ram,id=ram2,size=128M"
> > + " -object memory-backend-ram,id=nvm0,size=128M"
> > + " -device pc-dimm,id=dimm0,memdev=ram2,node=0"
> > + " -device nvdimm,id=dimm1,memdev=nvm0,node=1",
> > &data);
> >
> > free_test_data(&data);
> >
Ok. Noted.
Thanks,
Shameer
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v2 7/7] tests/bios-tables-test: Update arm/virt memhp test
2020-01-29 10:35 ` Shameerali Kolothum Thodi
@ 2020-01-29 13:01 ` Auger Eric
0 siblings, 0 replies; 45+ messages in thread
From: Auger Eric @ 2020-01-29 13:01 UTC (permalink / raw)
To: Shameerali Kolothum Thodi, qemu-devel, qemu-arm, imammedo
Cc: peter.maydell, xiaoguangrong.eric, mst, Linuxarm, shannon.zhaosl,
xuwei (O),
lersek
Hi Shameer,
On 1/29/20 11:35 AM, Shameerali Kolothum Thodi wrote:
> Hi Eric,
>
>> -----Original Message-----
>> From: Qemu-devel
>> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
>> u.org] On Behalf Of Auger Eric
>> Sent: 28 January 2020 16:29
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> qemu-devel@nongnu.org; qemu-arm@nongnu.org; imammedo@redhat.com
>> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
>> mst@redhat.com; Linuxarm <linuxarm@huawei.com>; xuwei (O)
>> <xuwei5@huawei.com>; shannon.zhaosl@gmail.com; lersek@redhat.com
>> Subject: Re: [PATCH v2 7/7] tests/bios-tables-test: Update arm/virt memhp
>> test
>>
>> Hi Shameer,
>>
>> On 1/17/20 6:45 PM, Shameer Kolothum wrote:
>>> Since we now have both pc-dimm and nvdimm support, update
>>> test_acpi_virt_tcg_memhp() to include those.
>>>
>>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>>> ---
>>> tests/data/acpi/virt/NFIT.memhp | 0
>>> tests/data/acpi/virt/SSDT.memhp | 0
>> Is it normal to have those 2 above void files? I lost track about the
>> process.
>
> I guess so :). From tests/qtest/bios-tables-test.c,
>
> /*
> * How to add or update the tests:
> * Contributor:
> * 1. add empty files for new tables, if any, under tests/data/acpi
> * 2. list any changed files in tests/bios-tables-test-allowed-diff.h
> * 3. commit the above *before* making changes that affect the tables
> ...
Thank you for reminding me of the process and doc location
>
> After reading that again, I am not sure those empty files can be in this
> Patch or not. I can move it to 6/7.
yep, maybe better then to put them in the same patch.
>
>>> tests/qtest/bios-tables-test.c | 9 +++++++--
>>> 3 files changed, 7 insertions(+), 2 deletions(-)
>>> create mode 100644 tests/data/acpi/virt/NFIT.memhp
>>> create mode 100644 tests/data/acpi/virt/SSDT.memhp
>>>
>>> diff --git a/tests/data/acpi/virt/NFIT.memhp
>> b/tests/data/acpi/virt/NFIT.memhp
>>> new file mode 100644
>>> index 0000000000..e69de29bb2
>>> diff --git a/tests/data/acpi/virt/SSDT.memhp
>> b/tests/data/acpi/virt/SSDT.memhp
>>> new file mode 100644
>>> index 0000000000..e69de29bb2
>>> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
>>> index f1ac2d7e96..695d2e7fac 100644
>>> --- a/tests/qtest/bios-tables-test.c
>>> +++ b/tests/qtest/bios-tables-test.c
>>> @@ -913,12 +913,17 @@ static void test_acpi_virt_tcg_memhp(void)
>>> };
>>>
>>> data.variant = ".memhp";
>>> - test_acpi_one(" -cpu cortex-a57"
>>> + test_acpi_one(" -machine nvdimm=on"
>> nit: maybe keep the same order as before ...
>>> + " -cpu cortex-a57"
>>> " -m 256M,slots=3,maxmem=1G"
>> and simply add ,nvdimm=on to above line.
>>> " -object memory-backend-ram,id=ram0,size=128M"
>>> " -object memory-backend-ram,id=ram1,size=128M"
>>> " -numa node,memdev=ram0 -numa
>> node,memdev=ram1"
>>> - " -numa dist,src=0,dst=1,val=21",
>>> + " -numa dist,src=0,dst=1,val=21"
>>> + " -object memory-backend-ram,id=ram2,size=128M"
>>> + " -object memory-backend-ram,id=nvm0,size=128M"
>>> + " -device pc-dimm,id=dimm0,memdev=ram2,node=0"
>>> + " -device nvdimm,id=dimm1,memdev=nvm0,node=1",
>>> &data);
>>>
>>> free_test_data(&data);
>>>
>
> Ok. Noted.
Thanks
Eric
>
> Thanks,
> Shameer
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 7/7] tests/bios-tables-test: Update arm/virt memhp test
2020-01-17 17:45 ` [PATCH v2 7/7] tests/bios-tables-test: Update arm/virt memhp test Shameer Kolothum
2020-01-28 16:29 ` Auger Eric
@ 2020-02-11 10:20 ` Michael S. Tsirkin
1 sibling, 0 replies; 45+ messages in thread
From: Michael S. Tsirkin @ 2020-02-11 10:20 UTC (permalink / raw)
To: Shameer Kolothum
Cc: peter.maydell, xiaoguangrong.eric, shannon.zhaosl, qemu-devel,
xuwei5, linuxarm, eric.auger, qemu-arm, imammedo, lersek
On Fri, Jan 17, 2020 at 05:45:22PM +0000, Shameer Kolothum wrote:
> Since we now have both pc-dimm and nvdimm support, update
> test_acpi_virt_tcg_memhp() to include those.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
And you can add a last patch on top updating the
expected files and blowing out the allowed diff file.
include the ASL changes in the log ...
> ---
> tests/data/acpi/virt/NFIT.memhp | 0
> tests/data/acpi/virt/SSDT.memhp | 0
> tests/qtest/bios-tables-test.c | 9 +++++++--
> 3 files changed, 7 insertions(+), 2 deletions(-)
> create mode 100644 tests/data/acpi/virt/NFIT.memhp
> create mode 100644 tests/data/acpi/virt/SSDT.memhp
>
> diff --git a/tests/data/acpi/virt/NFIT.memhp b/tests/data/acpi/virt/NFIT.memhp
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/tests/data/acpi/virt/SSDT.memhp b/tests/data/acpi/virt/SSDT.memhp
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index f1ac2d7e96..695d2e7fac 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -913,12 +913,17 @@ static void test_acpi_virt_tcg_memhp(void)
> };
>
> data.variant = ".memhp";
> - test_acpi_one(" -cpu cortex-a57"
> + test_acpi_one(" -machine nvdimm=on"
> + " -cpu cortex-a57"
> " -m 256M,slots=3,maxmem=1G"
> " -object memory-backend-ram,id=ram0,size=128M"
> " -object memory-backend-ram,id=ram1,size=128M"
> " -numa node,memdev=ram0 -numa node,memdev=ram1"
> - " -numa dist,src=0,dst=1,val=21",
> + " -numa dist,src=0,dst=1,val=21"
> + " -object memory-backend-ram,id=ram2,size=128M"
> + " -object memory-backend-ram,id=nvm0,size=128M"
> + " -device pc-dimm,id=dimm0,memdev=ram2,node=0"
> + " -device nvdimm,id=dimm1,memdev=nvm0,node=1",
> &data);
>
> free_test_data(&data);
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 0/7] ARM virt: Add NVDIMM support
2020-01-17 17:45 [PATCH v2 0/7] ARM virt: Add NVDIMM support Shameer Kolothum
` (6 preceding siblings ...)
2020-01-17 17:45 ` [PATCH v2 7/7] tests/bios-tables-test: Update arm/virt memhp test Shameer Kolothum
@ 2020-01-28 15:29 ` Auger Eric
2020-01-29 10:44 ` Shameerali Kolothum Thodi
7 siblings, 1 reply; 45+ messages in thread
From: Auger Eric @ 2020-01-28 15:29 UTC (permalink / raw)
To: Shameer Kolothum, qemu-devel, qemu-arm, imammedo
Cc: peter.maydell, xiaoguangrong.eric, mst, linuxarm, xuwei5,
shannon.zhaosl, lersek
Hi Shameer,
On 1/17/20 6:45 PM, Shameer Kolothum wrote:
> This series adds NVDIMM support to arm/virt platform.
> The series reuses some of the patches posted by Eric
> in his earlier attempt here[1].
>
> Patch #1 is a fix to the Guest reboot issue on NVDIMM
> hot add case described here[2] and patch #2 is another
> fix to the nvdimm aml issue discussed here[3].
>
> I have done a basic sanity testing of NVDIMM deviecs
> with Guest booting with both ACPI and DT. Further testing
> is always welcome.
>
> Please let me know your feedback.
With this version, I do not get the former spurious warning reported on v1.
I can see the nvdimm device topology using ndctl. So it looks fine to me.
Unfortunately we cannot test with DAX as kernel dependencies are not yet
resolved yet but this is an independent problem.
Thanks
Eric
>
> Thanks,
> Shameer
>
> [1] https://patchwork.kernel.org/cover/10830777/
> [2] https://patchwork.kernel.org/patch/11154757/
> [3] https://patchwork.kernel.org/cover/11174959/
>
> v1 --> v2
> -Reworked patch #1 and now fix is inside qemu_ram_resize().
> -Added patch #2 to fix the nvdim aml issue.
> -Dropped support to DT cold plug.
> -Updated test_acpi_virt_tcg_memhp() with pc-dimm and nvdimms(patch #7)
>
> Kwangwoo Lee (2):
> nvdimm: Use configurable ACPI IO base and size
> hw/arm/virt: Add nvdimm hot-plug infrastructure
>
> Shameer Kolothum (5):
> exec: Fix for qemu_ram_resize() callback
> hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length
> hw/arm/virt: Add nvdimm hotplug support
> tests: Update ACPI tables list for upcoming arm/virt test changes
> tests/bios-tables-test: Update arm/virt memhp test
>
> docs/specs/acpi_hw_reduced_hotplug.rst | 1 +
> exec.c | 36 +++++++----
> hw/acpi/generic_event_device.c | 13 ++++
> hw/acpi/nvdimm.c | 68 +++++++++++++++++----
> hw/arm/Kconfig | 1 +
> hw/arm/virt-acpi-build.c | 6 ++
> hw/arm/virt.c | 35 +++++++++--
> hw/i386/acpi-build.c | 6 ++
> hw/i386/acpi-build.h | 3 +
> hw/i386/pc_piix.c | 2 +
> hw/i386/pc_q35.c | 2 +
> hw/mem/Kconfig | 2 +-
> include/exec/ram_addr.h | 5 +-
> include/hw/acpi/generic_event_device.h | 1 +
> include/hw/arm/virt.h | 1 +
> include/hw/mem/nvdimm.h | 3 +
> tests/data/acpi/virt/NFIT.memhp | 0
> tests/data/acpi/virt/SSDT.memhp | 0
> tests/qtest/bios-tables-test-allowed-diff.h | 5 ++
> tests/qtest/bios-tables-test.c | 9 ++-
> 20 files changed, 163 insertions(+), 36 deletions(-)
> create mode 100644 tests/data/acpi/virt/NFIT.memhp
> create mode 100644 tests/data/acpi/virt/SSDT.memhp
>
^ permalink raw reply [flat|nested] 45+ messages in thread* RE: [PATCH v2 0/7] ARM virt: Add NVDIMM support
2020-01-28 15:29 ` [PATCH v2 0/7] ARM virt: Add NVDIMM support Auger Eric
@ 2020-01-29 10:44 ` Shameerali Kolothum Thodi
2020-01-29 12:55 ` Auger Eric
0 siblings, 1 reply; 45+ messages in thread
From: Shameerali Kolothum Thodi @ 2020-01-29 10:44 UTC (permalink / raw)
To: Auger Eric, qemu-devel, qemu-arm, imammedo
Cc: peter.maydell, xiaoguangrong.eric, mst, Linuxarm, shannon.zhaosl,
xuwei (O),
lersek
Hi Eric,
> -----Original Message-----
> From: Auger Eric [mailto:eric.auger@redhat.com]
> Sent: 28 January 2020 15:29
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> qemu-devel@nongnu.org; qemu-arm@nongnu.org; imammedo@redhat.com
> Cc: peter.maydell@linaro.org; shannon.zhaosl@gmail.com; mst@redhat.com;
> xiaoguangrong.eric@gmail.com; xuwei (O) <xuwei5@huawei.com>;
> lersek@redhat.com; Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH v2 0/7] ARM virt: Add NVDIMM support
>
> Hi Shameer,
>
> On 1/17/20 6:45 PM, Shameer Kolothum wrote:
> > This series adds NVDIMM support to arm/virt platform.
> > The series reuses some of the patches posted by Eric
> > in his earlier attempt here[1].
> >
> > Patch #1 is a fix to the Guest reboot issue on NVDIMM
> > hot add case described here[2] and patch #2 is another
> > fix to the nvdimm aml issue discussed here[3].
> >
> > I have done a basic sanity testing of NVDIMM deviecs
> > with Guest booting with both ACPI and DT. Further testing
> > is always welcome.
> >
> > Please let me know your feedback.
>
>
> With this version, I do not get the former spurious warning reported on v1.
>
> I can see the nvdimm device topology using ndctl. So it looks fine to me.
Thanks for giving it a spin and confirming.
> Unfortunately we cannot test with DAX as kernel dependencies are not yet
> resolved yet but this is an independent problem.
True. I did previously test DAX with "arm64/mm: Enable memory hot remove"
Patch series and that seems to work fine.
Cheers,
Shameer
> Thanks
>
> Eric
> >
> > Thanks,
> > Shameer
> >
> > [1] https://patchwork.kernel.org/cover/10830777/
> > [2] https://patchwork.kernel.org/patch/11154757/
> > [3] https://patchwork.kernel.org/cover/11174959/
> >
> > v1 --> v2
> > -Reworked patch #1 and now fix is inside qemu_ram_resize().
> > -Added patch #2 to fix the nvdim aml issue.
> > -Dropped support to DT cold plug.
> > -Updated test_acpi_virt_tcg_memhp() with pc-dimm and nvdimms(patch
> #7)
> >
> > Kwangwoo Lee (2):
> > nvdimm: Use configurable ACPI IO base and size
> > hw/arm/virt: Add nvdimm hot-plug infrastructure
> >
> > Shameer Kolothum (5):
> > exec: Fix for qemu_ram_resize() callback
> > hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length
> > hw/arm/virt: Add nvdimm hotplug support
> > tests: Update ACPI tables list for upcoming arm/virt test changes
> > tests/bios-tables-test: Update arm/virt memhp test
> >
> > docs/specs/acpi_hw_reduced_hotplug.rst | 1 +
> > exec.c | 36 +++++++----
> > hw/acpi/generic_event_device.c | 13 ++++
> > hw/acpi/nvdimm.c | 68
> +++++++++++++++++----
> > hw/arm/Kconfig | 1 +
> > hw/arm/virt-acpi-build.c | 6 ++
> > hw/arm/virt.c | 35 +++++++++--
> > hw/i386/acpi-build.c | 6 ++
> > hw/i386/acpi-build.h | 3 +
> > hw/i386/pc_piix.c | 2 +
> > hw/i386/pc_q35.c | 2 +
> > hw/mem/Kconfig | 2 +-
> > include/exec/ram_addr.h | 5 +-
> > include/hw/acpi/generic_event_device.h | 1 +
> > include/hw/arm/virt.h | 1 +
> > include/hw/mem/nvdimm.h | 3 +
> > tests/data/acpi/virt/NFIT.memhp | 0
> > tests/data/acpi/virt/SSDT.memhp | 0
> > tests/qtest/bios-tables-test-allowed-diff.h | 5 ++
> > tests/qtest/bios-tables-test.c | 9 ++-
> > 20 files changed, 163 insertions(+), 36 deletions(-)
> > create mode 100644 tests/data/acpi/virt/NFIT.memhp
> > create mode 100644 tests/data/acpi/virt/SSDT.memhp
> >
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v2 0/7] ARM virt: Add NVDIMM support
2020-01-29 10:44 ` Shameerali Kolothum Thodi
@ 2020-01-29 12:55 ` Auger Eric
0 siblings, 0 replies; 45+ messages in thread
From: Auger Eric @ 2020-01-29 12:55 UTC (permalink / raw)
To: Shameerali Kolothum Thodi, qemu-devel, qemu-arm, imammedo
Cc: peter.maydell, xiaoguangrong.eric, mst, Linuxarm, shannon.zhaosl,
xuwei (O),
lersek
Hi Shameer,
On 1/29/20 11:44 AM, Shameerali Kolothum Thodi wrote:
> Hi Eric,
>
>> -----Original Message-----
>> From: Auger Eric [mailto:eric.auger@redhat.com]
>> Sent: 28 January 2020 15:29
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> qemu-devel@nongnu.org; qemu-arm@nongnu.org; imammedo@redhat.com
>> Cc: peter.maydell@linaro.org; shannon.zhaosl@gmail.com; mst@redhat.com;
>> xiaoguangrong.eric@gmail.com; xuwei (O) <xuwei5@huawei.com>;
>> lersek@redhat.com; Linuxarm <linuxarm@huawei.com>
>> Subject: Re: [PATCH v2 0/7] ARM virt: Add NVDIMM support
>>
>> Hi Shameer,
>>
>> On 1/17/20 6:45 PM, Shameer Kolothum wrote:
>>> This series adds NVDIMM support to arm/virt platform.
>>> The series reuses some of the patches posted by Eric
>>> in his earlier attempt here[1].
>>>
>>> Patch #1 is a fix to the Guest reboot issue on NVDIMM
>>> hot add case described here[2] and patch #2 is another
>>> fix to the nvdimm aml issue discussed here[3].
>>>
>>> I have done a basic sanity testing of NVDIMM deviecs
>>> with Guest booting with both ACPI and DT. Further testing
>>> is always welcome.
>>>
>>> Please let me know your feedback.
>>
>>
>> With this version, I do not get the former spurious warning reported on v1.
>>
>> I can see the nvdimm device topology using ndctl. So it looks fine to me.
>
> Thanks for giving it a spin and confirming.
>
>> Unfortunately we cannot test with DAX as kernel dependencies are not yet
>> resolved yet but this is an independent problem.
>
> True. I did previously test DAX with "arm64/mm: Enable memory hot remove"
> Patch series and that seems to work fine.
Yes you're correct. I tested with v12 which unfortunately missed the
next kernel merge window if I am not wrong
(https://lkml.org/lkml/2020/1/21/1217). With that series we can
effectovely test with DAX on guest.
ndctl create-namespace --force --mode=fsdax --reconfig=namespace0.0
mkfs.xfs -f -m reflink=0 /dev/pmem0
sudo mount -o dax /dev/pmem0 /mnt/mem0
[ 539.970608] XFS (pmem0): DAX enabled. Warning: EXPERIMENTAL, use at
your own risk
[ 539.972947] XFS (pmem0): Mounting V5 Filesystem
[ 539.977978] XFS (pmem0): Ending clean mount
[ 539.979343] xfs filesystem being mounted at /mnt/mem0 supports
timestamps until 2038 (0x7fffffff)
It is useless for me to send my Tested-by at this point as you need to
remove the tiny conflict. However as soon as you respin I will be
pleased to send it.
As for the 2 first patches, I do not feel sufficiently comfortable on
that part to review them it in decent time and I cowardly leave it to
experts :-(
Thanks
Eric
>
> Cheers,
> Shameer
>
>
>
>> Thanks
>>
>> Eric
>>>
>>> Thanks,
>>> Shameer
>>>
>>> [1] https://patchwork.kernel.org/cover/10830777/
>>> [2] https://patchwork.kernel.org/patch/11154757/
>>> [3] https://patchwork.kernel.org/cover/11174959/
>>>
>>> v1 --> v2
>>> -Reworked patch #1 and now fix is inside qemu_ram_resize().
>>> -Added patch #2 to fix the nvdim aml issue.
>>> -Dropped support to DT cold plug.
>>> -Updated test_acpi_virt_tcg_memhp() with pc-dimm and nvdimms(patch
>> #7)
>>>
>>> Kwangwoo Lee (2):
>>> nvdimm: Use configurable ACPI IO base and size
>>> hw/arm/virt: Add nvdimm hot-plug infrastructure
>>>
>>> Shameer Kolothum (5):
>>> exec: Fix for qemu_ram_resize() callback
>>> hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length
>>> hw/arm/virt: Add nvdimm hotplug support
>>> tests: Update ACPI tables list for upcoming arm/virt test changes
>>> tests/bios-tables-test: Update arm/virt memhp test
>>>
>>> docs/specs/acpi_hw_reduced_hotplug.rst | 1 +
>>> exec.c | 36 +++++++----
>>> hw/acpi/generic_event_device.c | 13 ++++
>>> hw/acpi/nvdimm.c | 68
>> +++++++++++++++++----
>>> hw/arm/Kconfig | 1 +
>>> hw/arm/virt-acpi-build.c | 6 ++
>>> hw/arm/virt.c | 35 +++++++++--
>>> hw/i386/acpi-build.c | 6 ++
>>> hw/i386/acpi-build.h | 3 +
>>> hw/i386/pc_piix.c | 2 +
>>> hw/i386/pc_q35.c | 2 +
>>> hw/mem/Kconfig | 2 +-
>>> include/exec/ram_addr.h | 5 +-
>>> include/hw/acpi/generic_event_device.h | 1 +
>>> include/hw/arm/virt.h | 1 +
>>> include/hw/mem/nvdimm.h | 3 +
>>> tests/data/acpi/virt/NFIT.memhp | 0
>>> tests/data/acpi/virt/SSDT.memhp | 0
>>> tests/qtest/bios-tables-test-allowed-diff.h | 5 ++
>>> tests/qtest/bios-tables-test.c | 9 ++-
>>> 20 files changed, 163 insertions(+), 36 deletions(-)
>>> create mode 100644 tests/data/acpi/virt/NFIT.memhp
>>> create mode 100644 tests/data/acpi/virt/SSDT.memhp
>>>
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread