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;
}
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);
}
--
2.31.129.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);
> }
>
--
Best regards,
Vladimir
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 <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?
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 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);
>> }
>
>
--
Best regards,
Maxim Davydov
On Tue, 29 Mar 2022 00:15:33 +0300
Maxim Davydov <maxim.davydov@openvz.org> 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 <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;
> }
>
> 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);
> }
>
© 2016 - 2026 Red Hat, Inc.