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 <maxim.davydov@openvz.org>
---
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, &error_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 7c7d777781..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, &local_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);
}