[Qemu-devel] [PATCH] hw/i386: check if nvdimm is enabled before plugging

[Qemu-devel] [PATCH] hw/i386: check if nvdimm is enabled before plugging

[Haozhong Zhang]

The missing of 'nvdimm' in the machine type option '-M' means NVDIMM
is disabled. QEMU should refuse to plug any NVDIMM device in this case
and report the misconfiguration.

Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
Message-Id: 20170112110928.GF4621@stefanha-x1.localdomain
Message-Id: 20170111093630.2088-1-stefanha@redhat.com
---
 hw/i386/pc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 25e8586..3907609 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1715,6 +1715,11 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
     }
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+        if (!pcms->acpi_nvdimm_state.is_enabled) {
+            error_setg(&local_err,
+                       "nvdimm is not enabled: missing 'nvdimm' in '-M'");
+            goto out;
+        }
         nvdimm_plug(&pcms->acpi_nvdimm_state);
     }
 
-- 
2.10.1

Re: [Qemu-devel] [PATCH] hw/i386: check if nvdimm is enabled before plugging

[Xiao Guangrong]

On 01/13/2017 07:56 PM, Haozhong Zhang wrote:
> The missing of 'nvdimm' in the machine type option '-M' means NVDIMM
> is disabled. QEMU should refuse to plug any NVDIMM device in this case
> and report the misconfiguration.

Thanks for your fix.

Reviewed-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>

Re: [Qemu-devel] [PATCH] hw/i386: check if nvdimm is enabled before plugging

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1424 bytes --]

On Fri, Jan 13, 2017 at 07:56:51PM +0800, Haozhong Zhang wrote:
> The missing of 'nvdimm' in the machine type option '-M' means NVDIMM
> is disabled. QEMU should refuse to plug any NVDIMM device in this case
> and report the misconfiguration.
> 
> Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> Message-Id: 20170112110928.GF4621@stefanha-x1.localdomain
> Message-Id: 20170111093630.2088-1-stefanha@redhat.com
> ---
>  hw/i386/pc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 25e8586..3907609 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1715,6 +1715,11 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>      }
>  
>      if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> +        if (!pcms->acpi_nvdimm_state.is_enabled) {
> +            error_setg(&local_err,
> +                       "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> +            goto out;
> +        }

A warning is definitely useful to notify users of a possible
configuration error.

I wonder what happens when you plug an NVDIMM into a motherboard where
the firmware lacks support.  Does it:
 * Refuse to boot?
 * Treat the DIMM as regular RAM?
 * Boot but the DIMM will not be used by firmware and kernel?

QEMU should act the same way as real hardware.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] hw/i386: check if nvdimm is enabled before plugging

[Haozhong Zhang]

On 01/13/17 13:17 +0000, Stefan Hajnoczi wrote:
>On Fri, Jan 13, 2017 at 07:56:51PM +0800, Haozhong Zhang wrote:
>> The missing of 'nvdimm' in the machine type option '-M' means NVDIMM
>> is disabled. QEMU should refuse to plug any NVDIMM device in this case
>> and report the misconfiguration.
>>
>> Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>> Message-Id: 20170112110928.GF4621@stefanha-x1.localdomain
>> Message-Id: 20170111093630.2088-1-stefanha@redhat.com
>> ---
>>  hw/i386/pc.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 25e8586..3907609 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1715,6 +1715,11 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>>      }
>>
>>      if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
>> +        if (!pcms->acpi_nvdimm_state.is_enabled) {
>> +            error_setg(&local_err,
>> +                       "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>> +            goto out;
>> +        }
>
>A warning is definitely useful to notify users of a possible
>configuration error.

If NVDIMM is plugged at boot time, QEMU with this patch will stop and report a
message like
    qemu-system-x86_64: -device nvdimm,...: nvdimm is not enabled: missing 'nvdimm' in '-M'

If NVDIMM is plugged via 'device_add' command, QEMU with this patch will report a warning message
    nvdimm is not enabled: missing 'nvdimm' in '-M'
and continue to run w/o plugging this device.

>
>I wonder what happens when you plug an NVDIMM into a motherboard where
>the firmware lacks support.  Does it:
> * Refuse to boot?
> * Treat the DIMM as regular RAM?
> * Boot but the DIMM will not be used by firmware and kernel?

Good question. Guangrong, any idea?

Thanks,
Haozhong

>
>QEMU should act the same way as real hardware.
>
>Stefan

Re: [Qemu-devel] [PATCH] hw/i386: check if nvdimm is enabled before plugging

[Eduardo Habkost]

On Fri, Jan 13, 2017 at 01:17:27PM +0000, Stefan Hajnoczi wrote:
> On Fri, Jan 13, 2017 at 07:56:51PM +0800, Haozhong Zhang wrote:
> > The missing of 'nvdimm' in the machine type option '-M' means NVDIMM
> > is disabled. QEMU should refuse to plug any NVDIMM device in this case
> > and report the misconfiguration.
> > 
> > Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > Message-Id: 20170112110928.GF4621@stefanha-x1.localdomain
> > Message-Id: 20170111093630.2088-1-stefanha@redhat.com
> > ---
> >  hw/i386/pc.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 25e8586..3907609 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1715,6 +1715,11 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> >      }
> >  
> >      if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> > +        if (!pcms->acpi_nvdimm_state.is_enabled) {
> > +            error_setg(&local_err,
> > +                       "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> > +            goto out;
> > +        }
> 
> A warning is definitely useful to notify users of a possible
> configuration error.
> 
> I wonder what happens when you plug an NVDIMM into a motherboard where
> the firmware lacks support.  Does it:
>  * Refuse to boot?
>  * Treat the DIMM as regular RAM?
>  * Boot but the DIMM will not be used by firmware and kernel?
> 
> QEMU should act the same way as real hardware.

If real hardware behavior is not useful in any way (e.g. first
and third options above), is there a good reason for QEMU to not
implement an additional safety mechanism preventing NVDIMM from
being connected to a machine that doesn't support it?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] hw/i386: check if nvdimm is enabled before plugging

[Xiao Guangrong]

On 01/14/2017 02:02 AM, Eduardo Habkost wrote:
> On Fri, Jan 13, 2017 at 01:17:27PM +0000, Stefan Hajnoczi wrote:
>> On Fri, Jan 13, 2017 at 07:56:51PM +0800, Haozhong Zhang wrote:
>>> The missing of 'nvdimm' in the machine type option '-M' means NVDIMM
>>> is disabled. QEMU should refuse to plug any NVDIMM device in this case
>>> and report the misconfiguration.
>>>
>>> Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>>> Message-Id: 20170112110928.GF4621@stefanha-x1.localdomain
>>> Message-Id: 20170111093630.2088-1-stefanha@redhat.com
>>> ---
>>>  hw/i386/pc.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index 25e8586..3907609 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -1715,6 +1715,11 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>>>      }
>>>
>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
>>> +        if (!pcms->acpi_nvdimm_state.is_enabled) {
>>> +            error_setg(&local_err,
>>> +                       "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>>> +            goto out;
>>> +        }
>>
>> A warning is definitely useful to notify users of a possible
>> configuration error.
>>
>> I wonder what happens when you plug an NVDIMM into a motherboard where
>> the firmware lacks support.  Does it:
>>  * Refuse to boot?
>>  * Treat the DIMM as regular RAM?
>>  * Boot but the DIMM will not be used by firmware and kernel?
>>
>> QEMU should act the same way as real hardware.
>
> If real hardware behavior is not useful in any way (e.g. first
> and third options above), is there a good reason for QEMU to not
> implement an additional safety mechanism preventing NVDIMM from
> being connected to a machine that doesn't support it?
>

Yes. i agree with Eduardo.

For the real hardware the behavior may be different between vendors, we
are asking our HW people to check what will happen on Intel's hardware
under this case.

Thanks!

Re: [Qemu-devel] [PATCH] hw/i386: check if nvdimm is enabled before plugging

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2450 bytes --]

On Mon, Jan 16, 2017 at 01:55:34PM +0800, Xiao Guangrong wrote:
> 
> 
> On 01/14/2017 02:02 AM, Eduardo Habkost wrote:
> > On Fri, Jan 13, 2017 at 01:17:27PM +0000, Stefan Hajnoczi wrote:
> > > On Fri, Jan 13, 2017 at 07:56:51PM +0800, Haozhong Zhang wrote:
> > > > The missing of 'nvdimm' in the machine type option '-M' means NVDIMM
> > > > is disabled. QEMU should refuse to plug any NVDIMM device in this case
> > > > and report the misconfiguration.
> > > > 
> > > > Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > > > Message-Id: 20170112110928.GF4621@stefanha-x1.localdomain
> > > > Message-Id: 20170111093630.2088-1-stefanha@redhat.com
> > > > ---
> > > >  hw/i386/pc.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > index 25e8586..3907609 100644
> > > > --- a/hw/i386/pc.c
> > > > +++ b/hw/i386/pc.c
> > > > @@ -1715,6 +1715,11 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> > > >      }
> > > > 
> > > >      if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> > > > +        if (!pcms->acpi_nvdimm_state.is_enabled) {
> > > > +            error_setg(&local_err,
> > > > +                       "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> > > > +            goto out;
> > > > +        }
> > > 
> > > A warning is definitely useful to notify users of a possible
> > > configuration error.
> > > 
> > > I wonder what happens when you plug an NVDIMM into a motherboard where
> > > the firmware lacks support.  Does it:
> > >  * Refuse to boot?
> > >  * Treat the DIMM as regular RAM?
> > >  * Boot but the DIMM will not be used by firmware and kernel?
> > > 
> > > QEMU should act the same way as real hardware.
> > 
> > If real hardware behavior is not useful in any way (e.g. first
> > and third options above), is there a good reason for QEMU to not
> > implement an additional safety mechanism preventing NVDIMM from
> > being connected to a machine that doesn't support it?
> > 
> 
> Yes. i agree with Eduardo.
> 
> For the real hardware the behavior may be different between vendors, we
> are asking our HW people to check what will happen on Intel's hardware
> under this case.

Let's find out what real hardware/firmware does.  I guess it's the Intel
MRC component that handles memory initialization.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] hw/i386: check if nvdimm is enabled before plugging

[Haozhong Zhang]

On 01/16/17 11:00 +0000, Stefan Hajnoczi wrote:
>On Mon, Jan 16, 2017 at 01:55:34PM +0800, Xiao Guangrong wrote:
>> On 01/14/2017 02:02 AM, Eduardo Habkost wrote:
>> > On Fri, Jan 13, 2017 at 01:17:27PM +0000, Stefan Hajnoczi wrote:
>> > > On Fri, Jan 13, 2017 at 07:56:51PM +0800, Haozhong Zhang wrote:
>> > > > The missing of 'nvdimm' in the machine type option '-M' means NVDIMM
>> > > > is disabled. QEMU should refuse to plug any NVDIMM device in this case
>> > > > and report the misconfiguration.
>> > > >
>> > > > Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
>> > > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>> > > > Message-Id: 20170112110928.GF4621@stefanha-x1.localdomain
>> > > > Message-Id: 20170111093630.2088-1-stefanha@redhat.com
>> > > > ---
>> > > >  hw/i386/pc.c | 5 +++++
>> > > >  1 file changed, 5 insertions(+)
>> > > >
>> > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> > > > index 25e8586..3907609 100644
>> > > > --- a/hw/i386/pc.c
>> > > > +++ b/hw/i386/pc.c
>> > > > @@ -1715,6 +1715,11 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>> > > >      }
>> > > >
>> > > >      if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
>> > > > +        if (!pcms->acpi_nvdimm_state.is_enabled) {
>> > > > +            error_setg(&local_err,
>> > > > +                       "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>> > > > +            goto out;
>> > > > +        }
>> > >
>> > > A warning is definitely useful to notify users of a possible
>> > > configuration error.
>> > >
>> > > I wonder what happens when you plug an NVDIMM into a motherboard where
>> > > the firmware lacks support.  Does it:
>> > >  * Refuse to boot?
>> > >  * Treat the DIMM as regular RAM?
>> > >  * Boot but the DIMM will not be used by firmware and kernel?
>> > >
>> > > QEMU should act the same way as real hardware.
>> >
>> > If real hardware behavior is not useful in any way (e.g. first
>> > and third options above), is there a good reason for QEMU to not
>> > implement an additional safety mechanism preventing NVDIMM from
>> > being connected to a machine that doesn't support it?
>> >
>>
>> Yes. i agree with Eduardo.
>>
>> For the real hardware the behavior may be different between vendors, we
>> are asking our HW people to check what will happen on Intel's hardware
>> under this case.
>
>Let's find out what real hardware/firmware does.  I guess it's the Intel
>MRC component that handles memory initialization.
>

The behavior of NVDIMM on unsupported platform (HW/FW) is vendor
specific. For some vendors, it's undefined and the platform may do
anything (e.g. the three points Stefan listed above). Thus, I think
QEMU is free to choose the implementation. Aborting QEMU
(i.e. refusing to boot) is the easiest one.

Haozhong

Re: [Qemu-devel] [PATCH] hw/i386: check if nvdimm is enabled before plugging

[Eduardo Habkost]

On Fri, Jan 20, 2017 at 08:55:58AM +0800, Haozhong Zhang wrote:
> On 01/16/17 11:00 +0000, Stefan Hajnoczi wrote:
> > On Mon, Jan 16, 2017 at 01:55:34PM +0800, Xiao Guangrong wrote:
> > > On 01/14/2017 02:02 AM, Eduardo Habkost wrote:
> > > > On Fri, Jan 13, 2017 at 01:17:27PM +0000, Stefan Hajnoczi wrote:
> > > > > On Fri, Jan 13, 2017 at 07:56:51PM +0800, Haozhong Zhang wrote:
> > > > > > The missing of 'nvdimm' in the machine type option '-M' means NVDIMM
> > > > > > is disabled. QEMU should refuse to plug any NVDIMM device in this case
> > > > > > and report the misconfiguration.
> > > > > >
> > > > > > Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > > > > > Message-Id: 20170112110928.GF4621@stefanha-x1.localdomain
> > > > > > Message-Id: 20170111093630.2088-1-stefanha@redhat.com
> > > > > > ---
> > > > > >  hw/i386/pc.c | 5 +++++
> > > > > >  1 file changed, 5 insertions(+)
> > > > > >
> > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > > > index 25e8586..3907609 100644
> > > > > > --- a/hw/i386/pc.c
> > > > > > +++ b/hw/i386/pc.c
> > > > > > @@ -1715,6 +1715,11 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> > > > > >      }
> > > > > >
> > > > > >      if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
> > > > > > +        if (!pcms->acpi_nvdimm_state.is_enabled) {
> > > > > > +            error_setg(&local_err,
> > > > > > +                       "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> > > > > > +            goto out;
> > > > > > +        }
> > > > >
> > > > > A warning is definitely useful to notify users of a possible
> > > > > configuration error.
> > > > >
> > > > > I wonder what happens when you plug an NVDIMM into a motherboard where
> > > > > the firmware lacks support.  Does it:
> > > > >  * Refuse to boot?
> > > > >  * Treat the DIMM as regular RAM?
> > > > >  * Boot but the DIMM will not be used by firmware and kernel?
> > > > >
> > > > > QEMU should act the same way as real hardware.
> > > >
> > > > If real hardware behavior is not useful in any way (e.g. first
> > > > and third options above), is there a good reason for QEMU to not
> > > > implement an additional safety mechanism preventing NVDIMM from
> > > > being connected to a machine that doesn't support it?
> > > >
> > > 
> > > Yes. i agree with Eduardo.
> > > 
> > > For the real hardware the behavior may be different between vendors, we
> > > are asking our HW people to check what will happen on Intel's hardware
> > > under this case.
> > 
> > Let's find out what real hardware/firmware does.  I guess it's the Intel
> > MRC component that handles memory initialization.
> > 
> 
> The behavior of NVDIMM on unsupported platform (HW/FW) is vendor
> specific. For some vendors, it's undefined and the platform may do
> anything (e.g. the three points Stefan listed above). Thus, I think
> QEMU is free to choose the implementation. Aborting QEMU
> (i.e. refusing to boot) is the easiest one.

I agree.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH] hw/i386: check if nvdimm is enabled before plugging

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]

On Fri, Jan 13, 2017 at 07:56:51PM +0800, Haozhong Zhang wrote:
> The missing of 'nvdimm' in the machine type option '-M' means NVDIMM
> is disabled. QEMU should refuse to plug any NVDIMM device in this case
> and report the misconfiguration.
> 
> Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> Message-Id: 20170112110928.GF4621@stefanha-x1.localdomain
> Message-Id: 20170111093630.2088-1-stefanha@redhat.com
> ---
>  hw/i386/pc.c | 5 +++++
>  1 file changed, 5 insertions(+)

Michael: Please add Haozhong's comment to the commit description when
merging:

"The behavior of NVDIMM on unsupported platform (HW/FW) is vendor
specific. For some vendors, it's undefined and the platform may do
anything. Thus, I think QEMU is free to choose the implementation.
Aborting QEMU (i.e. refusing to boot) is the easiest one."

This way we have a record that the behavior is okay according to the
spec and why we chose this option.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]