Re: [PATCH 2/2] pc-nvdimm acpi: build ACPI tables for pc-nvdimm devices

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Jan 05, 2016 at 10:01:26PM +0800, Haozhong Zhang wrote:
> On 01/05/16 11:00, Stefano Stabellini wrote:
> > On Mon, 4 Jan 2016, Konrad Rzeszutek Wilk wrote:
> > > On Mon, Jan 04, 2016 at 04:01:08PM +0000, Stefano Stabellini wrote:
> > > > CC'ing the Xen tools maintainers and Anthony.
> > > > 
> > > > On Tue, 29 Dec 2015, Haozhong Zhang wrote:
> > > > > Reuse existing NVDIMM ACPI code to build ACPI tables for pc-nvdimm
> > > > > devices. The resulting tables are then copied into Xen guest domain so
> > > > > tha they can be later loaded by Xen hvmloader.
> > > > > 
> > > > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > > > 
> > > > How much work would it be to generate the nvdimm acpi tables from the
> > > > Xen toolstack?
> > > 
> > > Why duplicate the code? The QEMU generates the NFIT tables and its sub-tables.
> > >
> > >
> > > > Getting the tables from QEMU doesn't seem like a good idea to me, unless
> > > > we start getting the whole set of ACPI tables that way.
> > > 
> > > There is also the ACPI DSDT code - which requires an memory region
> > > to be reserved for the AML code to drop the parameters so that QEMU
> > > can scan the NVDIMM for failures. The region (and size) should be
> > > determined by QEMU since it works on this data.
> > 
> > QEMU can generate the whole set of ACPI tables. Why should we take only
> > the nvdimm tables and not the others?
> >
> NVDIMM tables are the only tables required to support vNVDIMM in this
> patch series, and they are self-contained and not conflict with other
> existing tables built by hvmloader. For other tables built by QEMU, I
> have no idea whether they could work with Xen, so I only take nvdimm
> tables from QEMU.

But you also have to deal with the SSDT code right? And I was under the
impression that if you use hvmloader - it loads the ACPI DSDT which
it had compiled at build-time - only - but not the SSDT?

In other way - just having the ACPI NFIT won't give you the whole
picture - unless you also jam in ACPI _DSM (ACPI0012) methods as
part of the SSDT right?

Or does the ACPI tables generation also include an ACPI SSDT with the
ACPI _DSM methods as part of it? I only see 'nvdimm_build_acpi' and
NFIT comments, nothing about SSDT?

> 
> > I don't think it is wise to have two components which both think are in
> > control of generating ACPI tables, hvmloader (soon to be the toolstack
> > with Anthony's work) and QEMU. From an architectural perspective, it
> > doesn't look robust to me.
> >
> Do you mean whenever nvdimm code in QEMU is changed, we would have to
> make more efforts to ensure it still works with Xen?

Not sure I follow that. How is it different from the efforts to ensure
that the patches here (which only add one ACPI MADT table)
provide the proper support? Wouldn't you do the same type of checking
every time you modify the nvdimm_build_acpi code?


> 
> > Could we take this opportunity to switch to QEMU generating the whole
> > set of ACPI tables?
> >
> Not quite sure how much effort would be taken on this change. CCed
> hvmloader maintainers for their comments.
> 
> Thanks,
> Haozhong