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 <address@hidden>
> > > > > > Signed-off-by: Haozhong Zhang <address@hidden>
> > > > > > Message-Id: address@hidden
> > > > > > Message-Id: address@hidden
> > > > > > ---
> > > > > >  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 <address@hidden>

-- 
Eduardo