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

[Robert Hoo]

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&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?

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.
> > > 
> > >   
[...]