Re: [QEMU PATCH v2 4/6] nvdimm: Implement ACPI NVDIMM Label Methods
On Wed, 27 Jul 2022 13:22:34 +0800
Robert Hoo wrote:
> On Thu, 2022-07-21 at 10:58 +0200, Igor Mammedov wrote:
> [...]
> Thanks Igor for review.
> > > > The patch it is too intrusive and my hunch is that it breaks
> > > > ABI and needs a bunch of compat knobs to work properly and
> > > > that I'd like to avoid unless there is not other way around
> > > > the problem.
> > >
> > > Is the ABI here you mentioned the "struct NvdimmMthdIn{}" stuff?
> > > and the compat knobs refers to related functions' input/output
> > > params?
> >
> > ABI are structures that guest and QEMU pass through information
> > between each other. And knobs in this case would be compat
> > variable[s]
> > to keep old behavior in place for old machine types.
>
> My humble opinion:
> The changes of the compat variable(s) here don't break the ABI, the ABI
> between guest and host/qemu is the ACPI spec which we don't change and
> fully conform to it; actually we're implementing it.
> e.g. with these patches, old guest can boot up with no difference nor
> changes.
it's not about booting but about migration.
boot on old QEMU and then migrate to one with your patches,
then make guest use _DSM again. You will see that migrated
guest still uses _old_ ACPI tables/AML and ABI in new QEMU
_must_ be compatible with that.
As for the patch, it's too big, and looking at it I wasn't
able to convince myself that it's correct.
> >
> > > My thoughts is that eventually, sooner or later, more ACPI methods
> > > will
> > > be implemented per request, although now we can play the trick of
> > > wrapper new methods over the pipe of old _DSM implementation.
> > > Though this changes a little on existing struct NvdimmDsmIn {}, it
> > > paves the way for the future; and actually the change is more an
> > > extension or generalization, not fundamentally changes the
> > > framework.
> > >
> > > In short, my point is the change/generalization/extension will be
> > > inevitable, even if not present.
> >
> > Expanding ABI (interface between host) has 2 drawbacks
> > * it exposes more attack surface of VMM to hostile guest
> >and rises chances that vulnerability would slip through
> >review/testing
>
> This patch doesn't increase attach surface, I think.
>
> > * migration wise, QEMU has to support any ABI for years
> >and not only latest an greatest interface but also old
> >ones to keep guest started on older QEMU working across
> >migration, so any ABI change should be considered very
> >carefully before being implemented otherwise it all
> >quickly snowballs in unsupportable mess of compat
> >variables smeared across host/guest.
> >Reducing exposed ABI and constant need to expand it
> >was a reason why we have moved ACPI code from firmware
> >into QEMU, so we could describe hardware without costs
> >associated with of maintaining ABI.
>
> Yeah, migration is the only broken thing. With this patch, guest ACPI
> table changes, live guest migrate between new and old qemus will have
> problem. But I think this is not the only example of such kind of
> problem. How about other similar cases?
Upstream policy for version-ed machine types (pc-*/q35-*/...),
forward migration _must_ work.
If you consider your device should e supported/usable downstream,
you also need take in account backward migration as well.
> In fact, the point of our contention is around this
> https://www.qemu.org/docs/master/specs/acpi_nvdimm.html, whether or not
> change the implementation protocol by this patch. The protocol was for
> _DSM only. Unless we're not going to support any ACPI methods, it
> should be updated, and the _LS{I,R,W} are ACPI methods, we can play the
> trick in this special case, but definitely not next time.
>
> I suggest to do it now, nevertheless, you maintainers make the final
> decision.
Not for this case (i.e. make patches minimal, touching only AML side
and reusing data that QEMU already provides via MMIO).
If ABI needs extending in future, that should be discussed separately
when there is actual need for it.
> >
> > There might be need to extend ABI eventually, but not in this case.
> >
> > > > I was skeptical about this approach during v1 review and
> > > > now I'm pretty much sure it's over-engineered and we can
> > > > just repack data we receive from existing label _DSM functions
> > > > to provide _LS{I,R,W} like it was suggested in v1.
> > > > It will be much simpler and affect only AML side without
> > > > complicating ABI and without any compat cruft and will work
> > > > with ping-pong migration without any issues.
> > >
> > > Ostensibly it may looks simpler, actually not, I think. The AML
> > > "common
> > > pipe" NCAL() is already complex, it packs all _DSMs and NFIT()
> > > function
> > > logics there, packing new stuff in/through it will be bug-prone.
> > > Though this time we can avert touching it, as the new ACPI methods
> > > deprecating old _DSM
Re: [QEMU PATCH v2 4/6] nvdimm: Implement ACPI NVDIMM Label Methods
On Thu, 2022-07-21 at 10:58 +0200, Igor Mammedov wrote:
[...]
Thanks Igor for review.
> > > The patch it is too intrusive and my hunch is that it breaks
> > > ABI and needs a bunch of compat knobs to work properly and
> > > that I'd like to avoid unless there is not other way around
> > > the problem.
> >
> > Is the ABI here you mentioned the "struct NvdimmMthdIn{}" stuff?
> > and the compat knobs refers to related functions' input/output
> > params?
>
> ABI are structures that guest and QEMU pass through information
> between each other. And knobs in this case would be compat
> variable[s]
> to keep old behavior in place for old machine types.
My humble opinion:
The changes of the compat variable(s) here don't break the ABI, the ABI
between guest and host/qemu is the ACPI spec which we don't change and
fully conform to it; actually we're implementing it.
e.g. with these patches, old guest can boot up with no difference nor
changes.
>
> > My thoughts is that eventually, sooner or later, more ACPI methods
> > will
> > be implemented per request, although now we can play the trick of
> > wrapper new methods over the pipe of old _DSM implementation.
> > Though this changes a little on existing struct NvdimmDsmIn {}, it
> > paves the way for the future; and actually the change is more an
> > extension or generalization, not fundamentally changes the
> > framework.
> >
> > In short, my point is the change/generalization/extension will be
> > inevitable, even if not present.
>
> Expanding ABI (interface between host) has 2 drawbacks
> * it exposes more attack surface of VMM to hostile guest
>and rises chances that vulnerability would slip through
>review/testing
This patch doesn't increase attach surface, I think.
> * migration wise, QEMU has to support any ABI for years
>and not only latest an greatest interface but also old
>ones to keep guest started on older QEMU working across
>migration, so any ABI change should be considered very
>carefully before being implemented otherwise it all
>quickly snowballs in unsupportable mess of compat
>variables smeared across host/guest.
>Reducing exposed ABI and constant need to expand it
>was a reason why we have moved ACPI code from firmware
>into QEMU, so we could describe hardware without costs
>associated with of maintaining ABI.
Yeah, migration is the only broken thing. With this patch, guest ACPI
table changes, live guest migrate between new and old qemus will have
problem. But I think this is not the only example of such kind of
problem. How about other similar cases?
In fact, the point of our contention is around this
https://www.qemu.org/docs/master/specs/acpi_nvdimm.html, whether or not
change the implementation protocol by this patch. The protocol was for
_DSM only. Unless we're not going to support any ACPI methods, it
should be updated, and the _LS{I,R,W} are ACPI methods, we can play the
trick in this special case, but definitely not next time.
I suggest to do it now, nevertheless, you maintainers make the final
decision.
>
> There might be need to extend ABI eventually, but not in this case.
>
> > > I was skeptical about this approach during v1 review and
> > > now I'm pretty much sure it's over-engineered and we can
> > > just repack data we receive from existing label _DSM functions
> > > to provide _LS{I,R,W} like it was suggested in v1.
> > > It will be much simpler and affect only AML side without
> > > complicating ABI and without any compat cruft and will work
> > > with ping-pong migration without any issues.
> >
> > Ostensibly it may looks simpler, actually not, I think. The AML
> > "common
> > pipe" NCAL() is already complex, it packs all _DSMs and NFIT()
> > function
> > logics there, packing new stuff in/through it will be bug-prone.
> > Though this time we can avert touching it, as the new ACPI methods
> > deprecating old _DSM functionally is almost the same.
> > How about next time? are we going to always packing new methods
> > logic
> > in NCAL()?
> > My point is that we should implement new methods as itself, of
> > course,
> > as a general programming rule, we can/should abstract common
> > routines,
> > but not packing them in one large function.
> > >
> > >
[...]
Re: [QEMU PATCH v2 4/6] nvdimm: Implement ACPI NVDIMM Label Methods
On Fri, 01 Jul 2022 17:23:04 +0800
Robert Hoo wrote:
> On Thu, 2022-06-16 at 14:32 +0200, Igor Mammedov wrote:
> > On Mon, 30 May 2022 11:40:45 +0800
> > Robert Hoo wrote:
> >
> > > Recent ACPI spec [1] has defined NVDIMM Label Methods _LS{I,R,W},
> > > which
> > > depricates corresponding _DSM Functions defined by PMEM _DSM
> > > Interface spec
> > > [2].
> > >
> > > In this implementation, we do 2 things
> > > 1. Generalize the QEMU<->ACPI BIOS NVDIMM interface, wrap it with
> > > ACPI
> > > method dispatch, _DSM is one of the branches. This also paves the
> > > way for
> > > adding other ACPI methods for NVDIMM.
> > > 2. Add _LS{I,R,W} method in each NVDIMM device in SSDT.
> > > ASL form of SSDT changes can be found in next test/qtest/bios-
> > > table-test
> > > commit message.
> > >
> > > [1] ACPI Spec v6.4, 6.5.10 NVDIMM Label Methods
> > > https://uefi.org/sites/default/files/resources/ACPI_Spec_6_4_Jan22.pdf
> > > [2] Intel PMEM _DSM Interface Spec v2.0, 3.10 Deprecated Functions
> > > https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf
> > >
> > > Signed-off-by: Robert Hoo
> > > Reviewed-by: Jingqi Liu
> > > ---
> > > hw/acpi/nvdimm.c| 424 +++-
> > >
> >
> > This patch is too large and doing to many things to be reviewable.
> > It needs to be split into smaller distinct chunks.
> > (however hold your horses and read on)
> >
> > The patch it is too intrusive and my hunch is that it breaks
> > ABI and needs a bunch of compat knobs to work properly and
> > that I'd like to avoid unless there is not other way around
> > the problem.
>
> Is the ABI here you mentioned the "struct NvdimmMthdIn{}" stuff?
> and the compat knobs refers to related functions' input/output params?
ABI are structures that guest and QEMU pass through information
between each other. And knobs in this case would be compat variable[s]
to keep old behavior in place for old machine types.
> My thoughts is that eventually, sooner or later, more ACPI methods will
> be implemented per request, although now we can play the trick of
> wrapper new methods over the pipe of old _DSM implementation.
> Though this changes a little on existing struct NvdimmDsmIn {}, it
> paves the way for the future; and actually the change is more an
> extension or generalization, not fundamentally changes the framework.
>
> In short, my point is the change/generalization/extension will be
> inevitable, even if not present.
Expanding ABI (interface between host) has 2 drawbacks
* it exposes more attack surface of VMM to hostile guest
and rises chances that vulnerability would slip through
review/testing
* migration wise, QEMU has to support any ABI for years
and not only latest an greatest interface but also old
ones to keep guest started on older QEMU working across
migration, so any ABI change should be considered very
carefully before being implemented otherwise it all
quickly snowballs in unsupportable mess of compat
variables smeared across host/guest.
Reducing exposed ABI and constant need to expand it
was a reason why we have moved ACPI code from firmware
into QEMU, so we could describe hardware without costs
associated with of maintaining ABI.
There might be need to extend ABI eventually, but not in this case.
> > I was skeptical about this approach during v1 review and
> > now I'm pretty much sure it's over-engineered and we can
> > just repack data we receive from existing label _DSM functions
> > to provide _LS{I,R,W} like it was suggested in v1.
> > It will be much simpler and affect only AML side without
> > complicating ABI and without any compat cruft and will work
> > with ping-pong migration without any issues.
>
> Ostensibly it may looks simpler, actually not, I think. The AML "common
> pipe" NCAL() is already complex, it packs all _DSMs and NFIT() function
> logics there, packing new stuff in/through it will be bug-prone.
> Though this time we can avert touching it, as the new ACPI methods
> deprecating old _DSM functionally is almost the same.
> How about next time? are we going to always packing new methods logic
> in NCAL()?
> My point is that we should implement new methods as itself, of course,
> as a general programming rule, we can/should abstract common routines,
> but not packing them in one large function.
> >
> >
> > > include/hw/mem/nvdimm.h | 6 +
> > > 2 files changed, 338 insertions(+), 92 deletions(-)
> > >
> > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > > index 59b42afcf1..50ee85866b 100644
> > > --- a/hw/acpi/nvdimm.c
> > > +++ b/hw/acpi/nvdimm.c
> > > @@ -416,17 +416,22 @@ static void nvdimm_build_nfit(NVDIMMState
> > > *state, GArray *table_offsets,
> > >
> > > #define NVDIMM_DSM_MEMORY_SIZE 4096
> > >
> > > -struct NvdimmDsmIn {
> > > +struct NvdimmMthdIn {
> > > uint32_t handle;
> > > +uint32_t method;
> > > +uint8_t args[4088];
> > > +} QEMU_PACKED;
> >
Re: [QEMU PATCH v2 4/6] nvdimm: Implement ACPI NVDIMM Label Methods
On Tue, Jul 19, 2022 at 10:46:38AM +0800, Robert Hoo wrote: > Ping... Igor could you respond? It's been 3 weeks ...
Re: [QEMU PATCH v2 4/6] nvdimm: Implement ACPI NVDIMM Label Methods
Ping...
On Fri, 2022-07-01 at 17:23 +0800, Robert Hoo wrote:
> On Thu, 2022-06-16 at 14:32 +0200, Igor Mammedov wrote:
> > On Mon, 30 May 2022 11:40:45 +0800
> > Robert Hoo wrote:
> >
> > > Recent ACPI spec [1] has defined NVDIMM Label Methods _LS{I,R,W},
> > > which
> > > depricates corresponding _DSM Functions defined by PMEM _DSM
> > > Interface spec
> > > [2].
> > >
> > > In this implementation, we do 2 things
> > > 1. Generalize the QEMU<->ACPI BIOS NVDIMM interface, wrap it with
> > > ACPI
> > > method dispatch, _DSM is one of the branches. This also paves the
> > > way for
> > > adding other ACPI methods for NVDIMM.
> > > 2. Add _LS{I,R,W} method in each NVDIMM device in SSDT.
> > > ASL form of SSDT changes can be found in next test/qtest/bios-
> > > table-test
> > > commit message.
> > >
> > > [1] ACPI Spec v6.4, 6.5.10 NVDIMM Label Methods
> > > https://uefi.org/sites/default/files/resources/ACPI_Spec_6_4_Jan22.pdf
> > > [2] Intel PMEM _DSM Interface Spec v2.0, 3.10 Deprecated
> > > Functions
> > > https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf
> > >
> > > Signed-off-by: Robert Hoo
> > > Reviewed-by: Jingqi Liu
> > > ---
> > > hw/acpi/nvdimm.c| 424 +++---
> > > --
> > >
> >
> > This patch is too large and doing to many things to be reviewable.
> > It needs to be split into smaller distinct chunks.
> > (however hold your horses and read on)
> >
> > The patch it is too intrusive and my hunch is that it breaks
> > ABI and needs a bunch of compat knobs to work properly and
> > that I'd like to avoid unless there is not other way around
> > the problem.
>
> Is the ABI here you mentioned the "struct NvdimmMthdIn{}" stuff?
> and the compat knobs refers to related functions' input/output
> params?
>
> My thoughts is that eventually, sooner or later, more ACPI methods
> will
> be implemented per request, although now we can play the trick of
> wrapper new methods over the pipe of old _DSM implementation.
> Though this changes a little on existing struct NvdimmDsmIn {}, it
> paves the way for the future; and actually the change is more an
> extension or generalization, not fundamentally changes the framework.
>
> In short, my point is the change/generalization/extension will be
> inevitable, even if not present.
> >
> > I was skeptical about this approach during v1 review and
> > now I'm pretty much sure it's over-engineered and we can
> > just repack data we receive from existing label _DSM functions
> > to provide _LS{I,R,W} like it was suggested in v1.
> > It will be much simpler and affect only AML side without
> > complicating ABI and without any compat cruft and will work
> > with ping-pong migration without any issues.
>
> Ostensibly it may looks simpler, actually not, I think. The AML
> "common
> pipe" NCAL() is already complex, it packs all _DSMs and NFIT()
> function
> logics there, packing new stuff in/through it will be bug-prone.
> Though this time we can avert touching it, as the new ACPI methods
> deprecating old _DSM functionally is almost the same.
> How about next time? are we going to always packing new methods logic
> in NCAL()?
> My point is that we should implement new methods as itself, of
> course,
> as a general programming rule, we can/should abstract common
> routines,
> but not packing them in one large function.
> >
> >
> > > include/hw/mem/nvdimm.h | 6 +
> > > 2 files changed, 338 insertions(+), 92 deletions(-)
> > >
> > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > > index 59b42afcf1..50ee85866b 100644
> > > --- a/hw/acpi/nvdimm.c
> > > +++ b/hw/acpi/nvdimm.c
> > > @@ -416,17 +416,22 @@ static void nvdimm_build_nfit(NVDIMMState
> > > *state, GArray *table_offsets,
> > >
> > > #define NVDIMM_DSM_MEMORY_SIZE 4096
> > >
> > > -struct NvdimmDsmIn {
> > > +struct NvdimmMthdIn {
> > > uint32_t handle;
> > > +uint32_t method;
> > > +uint8_t args[4088];
> > > +} QEMU_PACKED;
> > > +typedef struct NvdimmMthdIn NvdimmMthdIn;
> > > +struct NvdimmDsmIn {
> > > uint32_t revision;
> > > uint32_t function;
> > > /* the remaining size in the page is used by arg3. */
> > > union {
> > > -uint8_t arg3[4084];
> > > +uint8_t arg3[4080];
> > > };
> > > } QEMU_PACKED;
> > > typedef struct NvdimmDsmIn NvdimmDsmIn;
> > > -QEMU_BUILD_BUG_ON(sizeof(NvdimmDsmIn) !=
> > > NVDIMM_DSM_MEMORY_SIZE);
> > > +QEMU_BUILD_BUG_ON(sizeof(NvdimmMthdIn) !=
> > > NVDIMM_DSM_MEMORY_SIZE);
> > >
> > > struct NvdimmDsmOut {
> > > /* the size of buffer filled by QEMU. */
> > > @@ -470,7 +475,8 @@ struct NvdimmFuncGetLabelDataIn {
> > > } QEMU_PACKED;
> > > typedef struct NvdimmFuncGetLabelDataIn
> > > NvdimmFuncGetLabelDataIn;
> > > QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataIn) +
> > > - offsetof(NvdimmDsmIn, arg3) >
> > > NVDIMM_DSM_MEMORY_SIZE);
> > > + offsetof(NvdimmDsmIn, arg3) +
> > > offsetof(NvdimmMthdIn,
Re: [QEMU PATCH v2 4/6] nvdimm: Implement ACPI NVDIMM Label Methods
On Thu, 2022-06-16 at 14:32 +0200, Igor Mammedov wrote:
> On Mon, 30 May 2022 11:40:45 +0800
> Robert Hoo wrote:
>
> > Recent ACPI spec [1] has defined NVDIMM Label Methods _LS{I,R,W},
> > which
> > depricates corresponding _DSM Functions defined by PMEM _DSM
> > Interface spec
> > [2].
> >
> > In this implementation, we do 2 things
> > 1. Generalize the QEMU<->ACPI BIOS NVDIMM interface, wrap it with
> > ACPI
> > method dispatch, _DSM is one of the branches. This also paves the
> > way for
> > adding other ACPI methods for NVDIMM.
> > 2. Add _LS{I,R,W} method in each NVDIMM device in SSDT.
> > ASL form of SSDT changes can be found in next test/qtest/bios-
> > table-test
> > commit message.
> >
> > [1] ACPI Spec v6.4, 6.5.10 NVDIMM Label Methods
> > https://uefi.org/sites/default/files/resources/ACPI_Spec_6_4_Jan22.pdf
> > [2] Intel PMEM _DSM Interface Spec v2.0, 3.10 Deprecated Functions
> > https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf
> >
> > Signed-off-by: Robert Hoo
> > Reviewed-by: Jingqi Liu
> > ---
> > hw/acpi/nvdimm.c| 424 +++-
> >
>
> This patch is too large and doing to many things to be reviewable.
> It needs to be split into smaller distinct chunks.
> (however hold your horses and read on)
>
> The patch it is too intrusive and my hunch is that it breaks
> ABI and needs a bunch of compat knobs to work properly and
> that I'd like to avoid unless there is not other way around
> the problem.
Is the ABI here you mentioned the "struct NvdimmMthdIn{}" stuff?
and the compat knobs refers to related functions' input/output params?
My thoughts is that eventually, sooner or later, more ACPI methods will
be implemented per request, although now we can play the trick of
wrapper new methods over the pipe of old _DSM implementation.
Though this changes a little on existing struct NvdimmDsmIn {}, it
paves the way for the future; and actually the change is more an
extension or generalization, not fundamentally changes the framework.
In short, my point is the change/generalization/extension will be
inevitable, even if not present.
>
> I was skeptical about this approach during v1 review and
> now I'm pretty much sure it's over-engineered and we can
> just repack data we receive from existing label _DSM functions
> to provide _LS{I,R,W} like it was suggested in v1.
> It will be much simpler and affect only AML side without
> complicating ABI and without any compat cruft and will work
> with ping-pong migration without any issues.
Ostensibly it may looks simpler, actually not, I think. The AML "common
pipe" NCAL() is already complex, it packs all _DSMs and NFIT() function
logics there, packing new stuff in/through it will be bug-prone.
Though this time we can avert touching it, as the new ACPI methods
deprecating old _DSM functionally is almost the same.
How about next time? are we going to always packing new methods logic
in NCAL()?
My point is that we should implement new methods as itself, of course,
as a general programming rule, we can/should abstract common routines,
but not packing them in one large function.
>
>
> > include/hw/mem/nvdimm.h | 6 +
> > 2 files changed, 338 insertions(+), 92 deletions(-)
> >
> > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > index 59b42afcf1..50ee85866b 100644
> > --- a/hw/acpi/nvdimm.c
> > +++ b/hw/acpi/nvdimm.c
> > @@ -416,17 +416,22 @@ static void nvdimm_build_nfit(NVDIMMState
> > *state, GArray *table_offsets,
> >
> > #define NVDIMM_DSM_MEMORY_SIZE 4096
> >
> > -struct NvdimmDsmIn {
> > +struct NvdimmMthdIn {
> > uint32_t handle;
> > +uint32_t method;
> > +uint8_t args[4088];
> > +} QEMU_PACKED;
> > +typedef struct NvdimmMthdIn NvdimmMthdIn;
> > +struct NvdimmDsmIn {
> > uint32_t revision;
> > uint32_t function;
> > /* the remaining size in the page is used by arg3. */
> > union {
> > -uint8_t arg3[4084];
> > +uint8_t arg3[4080];
> > };
> > } QEMU_PACKED;
> > typedef struct NvdimmDsmIn NvdimmDsmIn;
> > -QEMU_BUILD_BUG_ON(sizeof(NvdimmDsmIn) != NVDIMM_DSM_MEMORY_SIZE);
> > +QEMU_BUILD_BUG_ON(sizeof(NvdimmMthdIn) != NVDIMM_DSM_MEMORY_SIZE);
> >
> > struct NvdimmDsmOut {
> > /* the size of buffer filled by QEMU. */
> > @@ -470,7 +475,8 @@ struct NvdimmFuncGetLabelDataIn {
> > } QEMU_PACKED;
> > typedef struct NvdimmFuncGetLabelDataIn NvdimmFuncGetLabelDataIn;
> > QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataIn) +
> > - offsetof(NvdimmDsmIn, arg3) >
> > NVDIMM_DSM_MEMORY_SIZE);
> > + offsetof(NvdimmDsmIn, arg3) +
> > offsetof(NvdimmMthdIn, args) >
> > + NVDIMM_DSM_MEMORY_SIZE);
> >
> > struct NvdimmFuncGetLabelDataOut {
> > /* the size of buffer filled by QEMU. */
> > @@ -488,14 +494,16 @@ struct NvdimmFuncSetLabelDataIn {
> > } QEMU_PACKED;
> > typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
> >
Re: [QEMU PATCH v2 4/6] nvdimm: Implement ACPI NVDIMM Label Methods
On Mon, 30 May 2022 11:40:45 +0800
Robert Hoo wrote:
> Recent ACPI spec [1] has defined NVDIMM Label Methods _LS{I,R,W}, which
> depricates corresponding _DSM Functions defined by PMEM _DSM Interface spec
> [2].
>
> In this implementation, we do 2 things
> 1. Generalize the QEMU<->ACPI BIOS NVDIMM interface, wrap it with ACPI
> method dispatch, _DSM is one of the branches. This also paves the way for
> adding other ACPI methods for NVDIMM.
> 2. Add _LS{I,R,W} method in each NVDIMM device in SSDT.
> ASL form of SSDT changes can be found in next test/qtest/bios-table-test
> commit message.
>
> [1] ACPI Spec v6.4, 6.5.10 NVDIMM Label Methods
> https://uefi.org/sites/default/files/resources/ACPI_Spec_6_4_Jan22.pdf
> [2] Intel PMEM _DSM Interface Spec v2.0, 3.10 Deprecated Functions
> https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf
>
> Signed-off-by: Robert Hoo
> Reviewed-by: Jingqi Liu
> ---
> hw/acpi/nvdimm.c| 424 +++-
This patch is too large and doing to many things to be reviewable.
It needs to be split into smaller distinct chunks.
(however hold your horses and read on)
The patch it is too intrusive and my hunch is that it breaks
ABI and needs a bunch of compat knobs to work properly and
that I'd like to avoid unless there is not other way around
the problem.
I was skeptical about this approach during v1 review and
now I'm pretty much sure it's over-engineered and we can
just repack data we receive from existing label _DSM functions
to provide _LS{I,R,W} like it was suggested in v1.
It will be much simpler and affect only AML side without
complicating ABI and without any compat cruft and will work
with ping-pong migration without any issues.
> include/hw/mem/nvdimm.h | 6 +
> 2 files changed, 338 insertions(+), 92 deletions(-)
>
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 59b42afcf1..50ee85866b 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -416,17 +416,22 @@ static void nvdimm_build_nfit(NVDIMMState *state,
> GArray *table_offsets,
>
> #define NVDIMM_DSM_MEMORY_SIZE 4096
>
> -struct NvdimmDsmIn {
> +struct NvdimmMthdIn {
> uint32_t handle;
> +uint32_t method;
> +uint8_t args[4088];
> +} QEMU_PACKED;
> +typedef struct NvdimmMthdIn NvdimmMthdIn;
> +struct NvdimmDsmIn {
> uint32_t revision;
> uint32_t function;
> /* the remaining size in the page is used by arg3. */
> union {
> -uint8_t arg3[4084];
> +uint8_t arg3[4080];
> };
> } QEMU_PACKED;
> typedef struct NvdimmDsmIn NvdimmDsmIn;
> -QEMU_BUILD_BUG_ON(sizeof(NvdimmDsmIn) != NVDIMM_DSM_MEMORY_SIZE);
> +QEMU_BUILD_BUG_ON(sizeof(NvdimmMthdIn) != NVDIMM_DSM_MEMORY_SIZE);
>
> struct NvdimmDsmOut {
> /* the size of buffer filled by QEMU. */
> @@ -470,7 +475,8 @@ struct NvdimmFuncGetLabelDataIn {
> } QEMU_PACKED;
> typedef struct NvdimmFuncGetLabelDataIn NvdimmFuncGetLabelDataIn;
> QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataIn) +
> - offsetof(NvdimmDsmIn, arg3) > NVDIMM_DSM_MEMORY_SIZE);
> + offsetof(NvdimmDsmIn, arg3) + offsetof(NvdimmMthdIn, args)
> >
> + NVDIMM_DSM_MEMORY_SIZE);
>
> struct NvdimmFuncGetLabelDataOut {
> /* the size of buffer filled by QEMU. */
> @@ -488,14 +494,16 @@ struct NvdimmFuncSetLabelDataIn {
> } QEMU_PACKED;
> typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
> QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
> - offsetof(NvdimmDsmIn, arg3) > NVDIMM_DSM_MEMORY_SIZE);
> + offsetof(NvdimmDsmIn, arg3) + offsetof(NvdimmMthdIn, args)
> >
> + NVDIMM_DSM_MEMORY_SIZE);
>
> struct NvdimmFuncReadFITIn {
> uint32_t offset; /* the offset into FIT buffer. */
> } QEMU_PACKED;
> typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn;
> QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) +
> - offsetof(NvdimmDsmIn, arg3) > NVDIMM_DSM_MEMORY_SIZE);
> + offsetof(NvdimmDsmIn, arg3) + offsetof(NvdimmMthdIn, args)
> >
> + NVDIMM_DSM_MEMORY_SIZE);
>
> struct NvdimmFuncReadFITOut {
> /* the size of buffer filled by QEMU. */
> @@ -636,7 +644,8 @@ static uint32_t nvdimm_get_max_xfer_label_size(void)
> * the max data ACPI can write one time which is transferred by
> * 'Set Namespace Label Data' function.
> */
> -max_set_size = dsm_memory_size - offsetof(NvdimmDsmIn, arg3) -
> +max_set_size = dsm_memory_size - offsetof(NvdimmMthdIn, args) -
> + offsetof(NvdimmDsmIn, arg3) -
> sizeof(NvdimmFuncSetLabelDataIn);
>
> return MIN(max_get_size, max_set_size);
> @@ -697,16 +706,15 @@ static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice
> *nvdimm,
> /*
> * DSM Spec Rev1 4.5 Get Namespace Label Data (Function Index 5).
> */
> -static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
> -
[QEMU PATCH v2 4/6] nvdimm: Implement ACPI NVDIMM Label Methods
Recent ACPI spec [1] has defined NVDIMM Label Methods _LS{I,R,W}, which
depricates corresponding _DSM Functions defined by PMEM _DSM Interface spec
[2].
In this implementation, we do 2 things
1. Generalize the QEMU<->ACPI BIOS NVDIMM interface, wrap it with ACPI
method dispatch, _DSM is one of the branches. This also paves the way for
adding other ACPI methods for NVDIMM.
2. Add _LS{I,R,W} method in each NVDIMM device in SSDT.
ASL form of SSDT changes can be found in next test/qtest/bios-table-test
commit message.
[1] ACPI Spec v6.4, 6.5.10 NVDIMM Label Methods
https://uefi.org/sites/default/files/resources/ACPI_Spec_6_4_Jan22.pdf
[2] Intel PMEM _DSM Interface Spec v2.0, 3.10 Deprecated Functions
https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf
Signed-off-by: Robert Hoo
Reviewed-by: Jingqi Liu
---
hw/acpi/nvdimm.c| 424 +++-
include/hw/mem/nvdimm.h | 6 +
2 files changed, 338 insertions(+), 92 deletions(-)
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 59b42afcf1..50ee85866b 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -416,17 +416,22 @@ static void nvdimm_build_nfit(NVDIMMState *state, GArray
*table_offsets,
#define NVDIMM_DSM_MEMORY_SIZE 4096
-struct NvdimmDsmIn {
+struct NvdimmMthdIn {
uint32_t handle;
+uint32_t method;
+uint8_t args[4088];
+} QEMU_PACKED;
+typedef struct NvdimmMthdIn NvdimmMthdIn;
+struct NvdimmDsmIn {
uint32_t revision;
uint32_t function;
/* the remaining size in the page is used by arg3. */
union {
-uint8_t arg3[4084];
+uint8_t arg3[4080];
};
} QEMU_PACKED;
typedef struct NvdimmDsmIn NvdimmDsmIn;
-QEMU_BUILD_BUG_ON(sizeof(NvdimmDsmIn) != NVDIMM_DSM_MEMORY_SIZE);
+QEMU_BUILD_BUG_ON(sizeof(NvdimmMthdIn) != NVDIMM_DSM_MEMORY_SIZE);
struct NvdimmDsmOut {
/* the size of buffer filled by QEMU. */
@@ -470,7 +475,8 @@ struct NvdimmFuncGetLabelDataIn {
} QEMU_PACKED;
typedef struct NvdimmFuncGetLabelDataIn NvdimmFuncGetLabelDataIn;
QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataIn) +
- offsetof(NvdimmDsmIn, arg3) > NVDIMM_DSM_MEMORY_SIZE);
+ offsetof(NvdimmDsmIn, arg3) + offsetof(NvdimmMthdIn, args) >
+ NVDIMM_DSM_MEMORY_SIZE);
struct NvdimmFuncGetLabelDataOut {
/* the size of buffer filled by QEMU. */
@@ -488,14 +494,16 @@ struct NvdimmFuncSetLabelDataIn {
} QEMU_PACKED;
typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
- offsetof(NvdimmDsmIn, arg3) > NVDIMM_DSM_MEMORY_SIZE);
+ offsetof(NvdimmDsmIn, arg3) + offsetof(NvdimmMthdIn, args) >
+ NVDIMM_DSM_MEMORY_SIZE);
struct NvdimmFuncReadFITIn {
uint32_t offset; /* the offset into FIT buffer. */
} QEMU_PACKED;
typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn;
QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) +
- offsetof(NvdimmDsmIn, arg3) > NVDIMM_DSM_MEMORY_SIZE);
+ offsetof(NvdimmDsmIn, arg3) + offsetof(NvdimmMthdIn, args) >
+ NVDIMM_DSM_MEMORY_SIZE);
struct NvdimmFuncReadFITOut {
/* the size of buffer filled by QEMU. */
@@ -636,7 +644,8 @@ static uint32_t nvdimm_get_max_xfer_label_size(void)
* the max data ACPI can write one time which is transferred by
* 'Set Namespace Label Data' function.
*/
-max_set_size = dsm_memory_size - offsetof(NvdimmDsmIn, arg3) -
+max_set_size = dsm_memory_size - offsetof(NvdimmMthdIn, args) -
+ offsetof(NvdimmDsmIn, arg3) -
sizeof(NvdimmFuncSetLabelDataIn);
return MIN(max_get_size, max_set_size);
@@ -697,16 +706,15 @@ static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice
*nvdimm,
/*
* DSM Spec Rev1 4.5 Get Namespace Label Data (Function Index 5).
*/
-static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
- hwaddr dsm_mem_addr)
+static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm,
+NvdimmFuncGetLabelDataIn *get_label_data,
+hwaddr dsm_mem_addr)
{
NVDIMMClass *nvc = NVDIMM_GET_CLASS(nvdimm);
-NvdimmFuncGetLabelDataIn *get_label_data;
NvdimmFuncGetLabelDataOut *get_label_data_out;
uint32_t status;
int size;
-get_label_data = (NvdimmFuncGetLabelDataIn *)in->arg3;
get_label_data->offset = le32_to_cpu(get_label_data->offset);
get_label_data->length = le32_to_cpu(get_label_data->length);
@@ -737,15 +745,13 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice
*nvdimm, NvdimmDsmIn *in,
/*
* DSM Spec Rev1 4.6 Set Namespace Label Data (Function Index 6).
*/
-static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
+static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm,
+
