Re: [Qemu-devel] [RFC PATCH] powerpc: add PVR mask support

[Anthony Liguori <anthony@codemonkey.ws>]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> On Thu, 2013-08-15 at 08:03 +0200, Alexander Graf wrote:
>
>> >> How does the user select that he wants a v2.3 p7 cpu with this
>> patch?
>> > 
>> > Why would he want that? The behaviour would not change because of
>> the
>> > version - all definitions use the same POWERPC_FAMILY(POWER7) and
>> PVR is
>> > not virtualized anyway.
>> 
>> Quite frankly I don't know what to say here. Are you trying to play
>> dumb or are you just one of those totally sloppy people who don't care
>> about anything outside of their own scope of work?
>
> Can you stop the bloody personal attacks on Alexey ? It's becoming
> tiresome.


First, everyone needs to tone it down a couple levels.  This isn't LKML
and Jonathan Corbet doesn't read qemu-devel so pithy remarks aren't
going to get you a quote of the week.

Everyone is talking past each other and no one is addressing the real
problem.  There are two distinct issues here:

1) We have two ABIs that cannot be changed unless there's a very good
   reason to.  Alexey's original patch breaks both.  The guest ABI
   cannot change given a fixed command line.

   IOW, the exposed PVR value for -cpu POWER7 cannot change across
   versions of QEMU or when running on different hardware.  This breaks
   live migration and save/resume.

   We also cannot break the command line interface.  If the last version
   of QEMU supported -cpu POWER7_v2.1, then we must continue to support
   that.

   If there's a good reason to break either of these, that's fine but
   that justification needs be up front in the patch commit message.

2) The only "-cpu" that makes sense is "-cpu host" for KVM on HV (or
   whatever ya'll call it).  POWER does not have the ability to
   virtualize the hardware PVR value.  There is a virtual PVR in the
   device tree but that's orthogonal to what we think of as the VCPU (it
   essentially means IIUC that the cpu is compatible with that PVR).

   We should explicitly disallow any -cpu value when KVM on HV is
   enabled other than host.

   The implementation of "-cpu host" is also goofy on PPC.  -cpu host
   does a match on existing CPU models meaning that we have to define a
   CPU model for any possible CPU we run on.  This would require having
   every possible CPU model implemented in QEMU which is silly.
   Instead, we should have a passthrough CPU model for use with "-cpu
   host" which is essentially what Alexey's patch turns -cpu POWER7
   into.

Regards,

Anthony Liguori

>
> He makes a very valid point. The ability to specify a specific revision
> of the processor is pointless for pretty much any use case we have in
> mind at the moment, and is even more pointless as long as we emulate
> them all exactly the same way.
>
> Besides, we can probably still organize the table from "more precise" to
> "less precise" entries and match that way if you *really* want to have
> specific entries for obscure revision of the chip.
>
> Ben.
>
>> With HV KVM we can not trap PVR, yes. With PR KVM we do trap PVR and
>> we emulate it. With TCG we do trap PVR and we emulate it.
>> 
>> > May be (may be) ppc_cpu_class_by_name() needs to be fixed to try to
>> find
>> > the PPC CPU class with the biggest mask to choose (for example)
>> > 004a0201/ffffffff rather than more common 004a0000/ffff0000 (if
>> 004a0201 is
>> > added to the list separately from the common definition for some
>> reason).
>> 
>> I think the class split as Linux has it should work just fine, no?
>> 
>> 
>> Alex