Re: [PATCH v1 3/9] mem: appropriate handling getting mem region
On 3/30/22 14:27, Vladimir Sementsov-Ogievskiy wrote:
29.03.2022 00:15, Maxim Davydov wrote:
Attempt to get memory region if the device doesn't have hostmem may
not be
an error. This can be happen immediately after initialization (getting
value without default one).
Signed-off-by: Maxim Davydov
---
hw/i386/sgx-epc.c | 5 -
hw/mem/nvdimm.c | 6 ++
hw/mem/pc-dimm.c | 5 +
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/hw/i386/sgx-epc.c b/hw/i386/sgx-epc.c
index d664829d35..1a4c8acdcc 100644
--- a/hw/i386/sgx-epc.c
+++ b/hw/i386/sgx-epc.c
@@ -121,9 +121,12 @@ static MemoryRegion
*sgx_epc_md_get_memory_region(MemoryDeviceState *md,
{
SGXEPCDevice *epc = SGX_EPC(md);
HostMemoryBackend *hostmem;
+ DeviceState *dev = DEVICE(epc);
if (!epc->hostmem) {
- error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property must be
set");
+ if (dev->realized) {
+ error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property
must be set");
+ }
return NULL;
}
I can't judge, is it really and error or not.
But the way you change the logic is not correct, as you change the
semantics:
Old semantics: on error return NULL and set errp, on success return
non-NULL and not set errp
New semantics: on error return NULL and set errp, on success return
anything (may be NULL) and not set errp.
Callers are not prepared to this. For example, look at
memory_device_unplug:
it does
mr = mdc->get_memory_region(md, _abort);
assume it returns NULL, which is not an error (so we don't crash on
error_abort)
and then pass mr to memory_region_del_subregion(), which in turn
access mr->container, which will crash if mr is NULL.
Most probably the situation I describe is not possible, but I just
want to illustrate the idea.
Moreover, in QEMU functions which has "Error **errp" argument and
return pointer are recommended to return NULL on failure and nonNULL
on success. In other words, return value of function with "Error
**errp" argument should report success/failure information. And having
NULL as possible success return value is not recommended, as it's
ambiguous and leads to bugs (see big comment at start of
include/qapi/error.h).
So, if it's really needed to change the semantics in such
not-recommended way, you should check that all callers are OK with it
and also describe new semantics in a comment near get_memory_region
declaration. But better is deal with returned error as it is.. What is
an exact problem you trying to solve with this commit?
I tried to solve the problem with errors from request MemoryRegion (via
*md_get_memory_region()) that was called immediately after
object_new_with_class(). But it does seem to change the semantics.
Perhaps better solution would be to ignore these errors or to add an
exception to handle the object properties correctly.
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 7c7d81..61e77e5476 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -166,9 +166,15 @@ static MemoryRegion
*nvdimm_md_get_memory_region(MemoryDeviceState *md,
Error **errp)
{
NVDIMMDevice *nvdimm = NVDIMM(md);
+ PCDIMMDevice *dimm = PC_DIMM(nvdimm);
Error *local_err = NULL;
if (!nvdimm->nvdimm_mr) {
+ /* Not error if we try get memory region after init */
+ if (!dimm->hostmem) {
+ return NULL;
+ }
+
nvdimm_prepare_memory_region(nvdimm, _err);
if (local_err) {
error_propagate(errp, local_err);
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index f27e1a11ba..6fd74de97f 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -240,6 +240,11 @@ static void
pc_dimm_md_set_addr(MemoryDeviceState *md, uint64_t addr,
static MemoryRegion *pc_dimm_md_get_memory_region(MemoryDeviceState
*md,
Error **errp)
{
+ PCDIMMDevice *dimm = PC_DIMM(md);
+ /* Not error if we try get memory region after init */
+ if (!dimm->hostmem) {
+ return NULL;
+ }
return pc_dimm_get_memory_region(PC_DIMM(md), errp);
}
--
Best regards,
Maxim Davydov
Re: [PATCH v1 3/9] mem: appropriate handling getting mem region
On Tue, 29 Mar 2022 00:15:33 +0300
Maxim Davydov wrote:
> Attempt to get memory region if the device doesn't have hostmem may not be
> an error. This can be happen immediately after initialization (getting
> value without default one).
Above statement begs for expanded explanation
Pls rephrase and explain why it's needed and how it will be used.
>
> Signed-off-by: Maxim Davydov
> ---
> hw/i386/sgx-epc.c | 5 -
> hw/mem/nvdimm.c | 6 ++
> hw/mem/pc-dimm.c | 5 +
> 3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/sgx-epc.c b/hw/i386/sgx-epc.c
> index d664829d35..1a4c8acdcc 100644
> --- a/hw/i386/sgx-epc.c
> +++ b/hw/i386/sgx-epc.c
> @@ -121,9 +121,12 @@ static MemoryRegion
> *sgx_epc_md_get_memory_region(MemoryDeviceState *md,
> {
> SGXEPCDevice *epc = SGX_EPC(md);
> HostMemoryBackend *hostmem;
> +DeviceState *dev = DEVICE(epc);
>
> if (!epc->hostmem) {
> -error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property must be set");
> +if (dev->realized) {
> +error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property must be
> set");
> +}
> return NULL;
> }
>
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 7c7d81..61e77e5476 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -166,9 +166,15 @@ static MemoryRegion
> *nvdimm_md_get_memory_region(MemoryDeviceState *md,
> Error **errp)
> {
> NVDIMMDevice *nvdimm = NVDIMM(md);
> +PCDIMMDevice *dimm = PC_DIMM(nvdimm);
> Error *local_err = NULL;
>
> if (!nvdimm->nvdimm_mr) {
> +/* Not error if we try get memory region after init */
> +if (!dimm->hostmem) {
> +return NULL;
> +}
> +
> nvdimm_prepare_memory_region(nvdimm, _err);
> if (local_err) {
> error_propagate(errp, local_err);
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index f27e1a11ba..6fd74de97f 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -240,6 +240,11 @@ static void pc_dimm_md_set_addr(MemoryDeviceState *md,
> uint64_t addr,
> static MemoryRegion *pc_dimm_md_get_memory_region(MemoryDeviceState *md,
>Error **errp)
> {
> +PCDIMMDevice *dimm = PC_DIMM(md);
> +/* Not error if we try get memory region after init */
> +if (!dimm->hostmem) {
> +return NULL;
> +}
> return pc_dimm_get_memory_region(PC_DIMM(md), errp);
> }
>
Re: [PATCH v1 3/9] mem: appropriate handling getting mem region
29.03.2022 00:15, Maxim Davydov wrote:
Attempt to get memory region if the device doesn't have hostmem may not be
an error. This can be happen immediately after initialization (getting
value without default one).
Signed-off-by: Maxim Davydov
---
hw/i386/sgx-epc.c | 5 -
hw/mem/nvdimm.c | 6 ++
hw/mem/pc-dimm.c | 5 +
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/hw/i386/sgx-epc.c b/hw/i386/sgx-epc.c
index d664829d35..1a4c8acdcc 100644
--- a/hw/i386/sgx-epc.c
+++ b/hw/i386/sgx-epc.c
@@ -121,9 +121,12 @@ static MemoryRegion
*sgx_epc_md_get_memory_region(MemoryDeviceState *md,
{
SGXEPCDevice *epc = SGX_EPC(md);
HostMemoryBackend *hostmem;
+DeviceState *dev = DEVICE(epc);
if (!epc->hostmem) {
-error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property must be set");
+if (dev->realized) {
+error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property must be set");
+}
return NULL;
}
I can't judge, is it really and error or not.
But the way you change the logic is not correct, as you change the semantics:
Old semantics: on error return NULL and set errp, on success return non-NULL
and not set errp
New semantics: on error return NULL and set errp, on success return anything
(may be NULL) and not set errp.
Callers are not prepared to this. For example, look at memory_device_unplug:
it does
mr = mdc->get_memory_region(md, _abort);
assume it returns NULL, which is not an error (so we don't crash on error_abort)
and then pass mr to memory_region_del_subregion(), which in turn access
mr->container, which will crash if mr is NULL.
Most probably the situation I describe is not possible, but I just want to
illustrate the idea.
Moreover, in QEMU functions which has "Error **errp" argument and return pointer are
recommended to return NULL on failure and nonNULL on success. In other words, return value of
function with "Error **errp" argument should report success/failure information. And
having NULL as possible success return value is not recommended, as it's ambiguous and leads to
bugs (see big comment at start of include/qapi/error.h).
So, if it's really needed to change the semantics in such not-recommended way,
you should check that all callers are OK with it and also describe new
semantics in a comment near get_memory_region declaration. But better is deal
with returned error as it is.. What is an exact problem you trying to solve
with this commit?
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 7c7d81..61e77e5476 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -166,9 +166,15 @@ static MemoryRegion
*nvdimm_md_get_memory_region(MemoryDeviceState *md,
Error **errp)
{
NVDIMMDevice *nvdimm = NVDIMM(md);
+PCDIMMDevice *dimm = PC_DIMM(nvdimm);
Error *local_err = NULL;
if (!nvdimm->nvdimm_mr) {
+/* Not error if we try get memory region after init */
+if (!dimm->hostmem) {
+return NULL;
+}
+
nvdimm_prepare_memory_region(nvdimm, _err);
if (local_err) {
error_propagate(errp, local_err);
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index f27e1a11ba..6fd74de97f 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -240,6 +240,11 @@ static void pc_dimm_md_set_addr(MemoryDeviceState *md,
uint64_t addr,
static MemoryRegion *pc_dimm_md_get_memory_region(MemoryDeviceState *md,
Error **errp)
{
+PCDIMMDevice *dimm = PC_DIMM(md);
+/* Not error if we try get memory region after init */
+if (!dimm->hostmem) {
+return NULL;
+}
return pc_dimm_get_memory_region(PC_DIMM(md), errp);
}
--
Best regards,
Vladimir
[PATCH v1 3/9] mem: appropriate handling getting mem region
Attempt to get memory region if the device doesn't have hostmem may not be
an error. This can be happen immediately after initialization (getting
value without default one).
Signed-off-by: Maxim Davydov
---
hw/i386/sgx-epc.c | 5 -
hw/mem/nvdimm.c | 6 ++
hw/mem/pc-dimm.c | 5 +
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/hw/i386/sgx-epc.c b/hw/i386/sgx-epc.c
index d664829d35..1a4c8acdcc 100644
--- a/hw/i386/sgx-epc.c
+++ b/hw/i386/sgx-epc.c
@@ -121,9 +121,12 @@ static MemoryRegion
*sgx_epc_md_get_memory_region(MemoryDeviceState *md,
{
SGXEPCDevice *epc = SGX_EPC(md);
HostMemoryBackend *hostmem;
+DeviceState *dev = DEVICE(epc);
if (!epc->hostmem) {
-error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property must be set");
+if (dev->realized) {
+error_setg(errp, "'" SGX_EPC_MEMDEV_PROP "' property must be set");
+}
return NULL;
}
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 7c7d81..61e77e5476 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -166,9 +166,15 @@ static MemoryRegion
*nvdimm_md_get_memory_region(MemoryDeviceState *md,
Error **errp)
{
NVDIMMDevice *nvdimm = NVDIMM(md);
+PCDIMMDevice *dimm = PC_DIMM(nvdimm);
Error *local_err = NULL;
if (!nvdimm->nvdimm_mr) {
+/* Not error if we try get memory region after init */
+if (!dimm->hostmem) {
+return NULL;
+}
+
nvdimm_prepare_memory_region(nvdimm, _err);
if (local_err) {
error_propagate(errp, local_err);
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index f27e1a11ba..6fd74de97f 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -240,6 +240,11 @@ static void pc_dimm_md_set_addr(MemoryDeviceState *md,
uint64_t addr,
static MemoryRegion *pc_dimm_md_get_memory_region(MemoryDeviceState *md,
Error **errp)
{
+PCDIMMDevice *dimm = PC_DIMM(md);
+/* Not error if we try get memory region after init */
+if (!dimm->hostmem) {
+return NULL;
+}
return pc_dimm_get_memory_region(PC_DIMM(md), errp);
}
--
2.31.1
