| Message ID | 20221018152524.137598-1-jusual@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] hw/mem/nvdimm: fix error message for 'unarmed' flag | expand |
On 18/10/22 17:25, Julia Suvorova wrote: > In the ACPI specification [1], the 'unarmed' bit is set when a device > cannot accept a persistent write. This means that when a memdev is > read-only, the 'unarmed' flag must be turned on. The logic is correct, > just changing the error message. > > [1] ACPI NFIT NVDIMM Region Mapping Structure "NVDIMM State Flags" Bit 3 > Fixes: dbd730e859 ("nvdimm: check -object memory-backend-file, readonly=on option") The documentation in 'docs/nvdimm.txt' is correct :) > Signed-off-by: Julia Suvorova <jusual@redhat.com> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > hw/mem/nvdimm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c > index 7c7d777781..bfb76818c1 100644 > --- a/hw/mem/nvdimm.c > +++ b/hw/mem/nvdimm.c > @@ -149,7 +149,7 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp) > if (!nvdimm->unarmed && memory_region_is_rom(mr)) { > HostMemoryBackend *hostmem = dimm->hostmem; > > - error_setg(errp, "'unarmed' property must be off since memdev %s " > + error_setg(errp, "'unarmed' property must be on since memdev %s " If you ever respin please quote 'on' for readability. > "is read-only", > object_get_canonical_path_component(OBJECT(hostmem))); > return; Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Tue, Oct 18, 2022 at 06:17:55PM +0200, Philippe Mathieu-Daudé wrote: > On 18/10/22 17:25, Julia Suvorova wrote: > > In the ACPI specification [1], the 'unarmed' bit is set when a device > > cannot accept a persistent write. This means that when a memdev is > > read-only, the 'unarmed' flag must be turned on. The logic is correct, > > just changing the error message. > > > > [1] ACPI NFIT NVDIMM Region Mapping Structure "NVDIMM State Flags" Bit 3 > > > > Fixes: dbd730e859 ("nvdimm: check -object memory-backend-file, readonly=on > option") > > The documentation in 'docs/nvdimm.txt' is correct :) > > > Signed-off-by: Julia Suvorova <jusual@redhat.com> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > hw/mem/nvdimm.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c > > index 7c7d777781..bfb76818c1 100644 > > --- a/hw/mem/nvdimm.c > > +++ b/hw/mem/nvdimm.c > > @@ -149,7 +149,7 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp) > > if (!nvdimm->unarmed && memory_region_is_rom(mr)) { > > HostMemoryBackend *hostmem = dimm->hostmem; > > - error_setg(errp, "'unarmed' property must be off since memdev %s " > > + error_setg(errp, "'unarmed' property must be on since memdev %s " > > If you ever respin please quote 'on' for readability. Yes make sense. Julia could you change this pls? > > "is read-only", > > object_get_canonical_path_component(OBJECT(hostmem))); > > return; > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 18.10.22 18:49, Michael S. Tsirkin wrote: > On Tue, Oct 18, 2022 at 06:17:55PM +0200, Philippe Mathieu-Daudé wrote: >> On 18/10/22 17:25, Julia Suvorova wrote: >>> In the ACPI specification [1], the 'unarmed' bit is set when a device >>> cannot accept a persistent write. This means that when a memdev is >>> read-only, the 'unarmed' flag must be turned on. The logic is correct, >>> just changing the error message. >>> >>> [1] ACPI NFIT NVDIMM Region Mapping Structure "NVDIMM State Flags" Bit 3 >>> >> >> Fixes: dbd730e859 ("nvdimm: check -object memory-backend-file, readonly=on >> option") >> >> The documentation in 'docs/nvdimm.txt' is correct :) >> >>> Signed-off-by: Julia Suvorova <jusual@redhat.com> >>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >>> --- >>> hw/mem/nvdimm.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c >>> index 7c7d777781..bfb76818c1 100644 >>> --- a/hw/mem/nvdimm.c >>> +++ b/hw/mem/nvdimm.c >>> @@ -149,7 +149,7 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp) >>> if (!nvdimm->unarmed && memory_region_is_rom(mr)) { >>> HostMemoryBackend *hostmem = dimm->hostmem; >>> - error_setg(errp, "'unarmed' property must be off since memdev %s " >>> + error_setg(errp, "'unarmed' property must be on since memdev %s " >> >> If you ever respin please quote 'on' for readability. > > > Yes make sense. Julia could you change this pls? Acked-by: David Hildenbrand <david@redhat.com> I can pick this up once resent. I'll most probably send a merge request by the end of next week.
On Tue, Oct 18, 2022 at 6:49 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Oct 18, 2022 at 06:17:55PM +0200, Philippe Mathieu-Daudé wrote: > > On 18/10/22 17:25, Julia Suvorova wrote: > > > In the ACPI specification [1], the 'unarmed' bit is set when a device > > > cannot accept a persistent write. This means that when a memdev is > > > read-only, the 'unarmed' flag must be turned on. The logic is correct, > > > just changing the error message. > > > > > > [1] ACPI NFIT NVDIMM Region Mapping Structure "NVDIMM State Flags" Bit 3 > > > > > > > Fixes: dbd730e859 ("nvdimm: check -object memory-backend-file, readonly=on > > option") > > > > The documentation in 'docs/nvdimm.txt' is correct :) > > > > > Signed-off-by: Julia Suvorova <jusual@redhat.com> > > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > > --- > > > hw/mem/nvdimm.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c > > > index 7c7d777781..bfb76818c1 100644 > > > --- a/hw/mem/nvdimm.c > > > +++ b/hw/mem/nvdimm.c > > > @@ -149,7 +149,7 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp) > > > if (!nvdimm->unarmed && memory_region_is_rom(mr)) { > > > HostMemoryBackend *hostmem = dimm->hostmem; > > > - error_setg(errp, "'unarmed' property must be off since memdev %s " > > > + error_setg(errp, "'unarmed' property must be on since memdev %s " > > > > If you ever respin please quote 'on' for readability. > > > Yes make sense. Julia could you change this pls? Sure, will do. > > > "is read-only", > > > object_get_canonical_path_component(OBJECT(hostmem))); > > > return; > > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >
> In the ACPI specification [1], the 'unarmed' bit is set when a device > cannot accept a persistent write. This means that when a memdev is > read-only, the 'unarmed' flag must be turned on. The logic is correct, > just changing the error message. > > [1] ACPI NFIT NVDIMM Region Mapping Structure "NVDIMM State Flags" Bit 3 > > Signed-off-by: Julia Suvorova <jusual@redhat.com> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > hw/mem/nvdimm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c > index 7c7d777781..bfb76818c1 100644 > --- a/hw/mem/nvdimm.c > +++ b/hw/mem/nvdimm.c > @@ -149,7 +149,7 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp) > if (!nvdimm->unarmed && memory_region_is_rom(mr)) { > HostMemoryBackend *hostmem = dimm->hostmem; > > - error_setg(errp, "'unarmed' property must be off since memdev %s " > + error_setg(errp, "'unarmed' property must be on since memdev %s " > "is read-only", > object_get_canonical_path_component(OBJECT(hostmem))); > return; With the suggested minor change. Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c index 7c7d777781..bfb76818c1 100644 --- a/hw/mem/nvdimm.c +++ b/hw/mem/nvdimm.c @@ -149,7 +149,7 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp) if (!nvdimm->unarmed && memory_region_is_rom(mr)) { HostMemoryBackend *hostmem = dimm->hostmem; - error_setg(errp, "'unarmed' property must be off since memdev %s " + error_setg(errp, "'unarmed' property must be on since memdev %s " "is read-only", object_get_canonical_path_component(OBJECT(hostmem))); return;