Re: [QEMU PATCH v2 4/6] nvdimm: Implement ACPI NVDIMM Label Methods

[Igor Mammedov]

On Wed, 27 Jul 2022 13:22:34 +0800
Robert Hoo <robert.hu@linux.intel.com> 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&guest) 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 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.  
> > > > 
> > > >     
> [...]
>