diff mbox series

[RESEND] hw/mem/nvdimm: fix error message for 'unarmed' flag

Message ID 20221018152524.137598-1-jusual@redhat.com (mailing list archive)
State New, archived
Headers show
Series [RESEND] hw/mem/nvdimm: fix error message for 'unarmed' flag | expand

Commit Message

Julia Suvorova Oct. 18, 2022, 3:25 p.m. UTC 
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(-)

Comments

Philippe Mathieu-Daudé Oct. 18, 2022, 4:17 p.m. UTC | #1 
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>
Michael S. Tsirkin Oct. 18, 2022, 4:49 p.m. UTC | #2 
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>
David Hildenbrand Oct. 19, 2022, 12:40 p.m. UTC | #3 
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.
Julia Suvorova Oct. 19, 2022, 7:31 p.m. UTC | #4 
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>
>
Pankaj Gupta Oct. 20, 2022, 5:17 a.m. UTC | #5 
> 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 mbox series

Patch

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;