Re: [Qemu-devel] [PATCH/RFC 4/5] s390x/kvm: test whether a cpu is STOPPED when checking has_work

[David Hildenbrand]

 
 On 10.07.14 15:10, Christian Borntraeger wrote:
  From: David Hildenbrand d...@linux.vnet.ibm.com
 
  If a cpu is stopped, it must never be allowed to run and no interrupt may 
  wake it
  up. A cpu also has to be unhalted if it is halted and has work to do - this
  scenario wasn't hit in kvm case yet, as only disabled wait is processed 
  within
  QEMU.
 
  Signed-off-by: David Hildenbrand d...@linux.vnet.ibm.com
  Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
  Reviewed-by: Christian Borntraeger borntrae...@de.ibm.com
  Signed-off-by: Christian Borntraeger borntrae...@de.ibm.com
 
 This looks like it's something that generic infrastructure should take 
 care of, no? How does this work for the other archs? They always get an 
 interrupt on the transition between !has_work - has_work. Why don't we 
 get one for s390x?
 
 
 Alex
 
 

Well, we have the special case on s390 as a CPU that is in the STOPPED or the
CHECK STOP state may never run - even if there is an interrupt. It's
basically like this CPU has been switched off.

Imagine that it is tried to inject an interrupt into a stopped vcpu. It
will kick the stopped vcpu and thus lead to a call to
kvm_arch_process_async_events(). We have to deny that this vcpu will ever
run as long as it is stopped. It's like a way to suppress the
interrupt for such a transition you mentioned.

Later, another vcpu might decide to turn that vcpu back on (by e.g. sending a
SIGP START to that vcpu).

I am not sure if such a mechanism/scenario is applicable to any other arch. They
all seem to reset the cs-halted flag if they know they are able to run (e.g.
due to an interrupt) - they have no such thing as stopped cpus, only
halted/waiting cpus.

David

Re: [Qemu-devel] [PATCH/RFC 4/5] s390x/kvm: test whether a cpu is STOPPED when checking has_work

[David Hildenbrand]

 
 On 28.07.2014, at 16:16, David Hildenbrand d...@linux.vnet.ibm.com wrote:
 
  
  On 10.07.14 15:10, Christian Borntraeger wrote:
  From: David Hildenbrand d...@linux.vnet.ibm.com
  
  If a cpu is stopped, it must never be allowed to run and no interrupt may 
  wake it
  up. A cpu also has to be unhalted if it is halted and has work to do - 
  this
  scenario wasn't hit in kvm case yet, as only disabled wait is processed 
  within
  QEMU.
  
  Signed-off-by: David Hildenbrand d...@linux.vnet.ibm.com
  Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
  Reviewed-by: Christian Borntraeger borntrae...@de.ibm.com
  Signed-off-by: Christian Borntraeger borntrae...@de.ibm.com
  
  This looks like it's something that generic infrastructure should take 
  care of, no? How does this work for the other archs? They always get an 
  interrupt on the transition between !has_work - has_work. Why don't we 
  get one for s390x?
  
  
  Alex
  
  
  
  Well, we have the special case on s390 as a CPU that is in the STOPPED or 
  the
  CHECK STOP state may never run - even if there is an interrupt. It's
  basically like this CPU has been switched off.
  
  Imagine that it is tried to inject an interrupt into a stopped vcpu. It
  will kick the stopped vcpu and thus lead to a call to
  kvm_arch_process_async_events(). We have to deny that this vcpu will ever
  run as long as it is stopped. It's like a way to suppress the
  interrupt for such a transition you mentioned.
 
 An interrupt kick usually just means we go back into the main loop. From 
 there we check the interrupt bitmap which interrupt to handle. Check out the 
 handling code here:
 
   
 http://git.qemu.org/?p=qemu.git;a=blob;f=cpu-exec.c;h=38e5f02a307523d99134f4e2e6c51683bb10b45b;hb=HEAD#l580
 
 If you just check for the stopped state in here, do_interrupt() will never 
 get called and thus the CPU shouldn't ever get executed. Unless I'm heavily 
 mistaken :).

So you would rather move the check out of has_work() into the main loop in
cpu-exec.c and directly into kvm_arch_process_async_events()?

This would on the other hand lead to an unhalt of the vcpu in cpu_exec() on any
CPU_INTERRUPT_HARD. A VCPU might thus be unhalted although it is not able to 
run. Is okay?

Looking at cpu.c:cpu_thread_is_idle(), we would maybe return false, although we
are idle (because we are idle when we are stopped)?

My qemu kvm knowledge is way better than the qemu emulation knowledge, so I
appreciate any insights :)

 
  
  Later, another vcpu might decide to turn that vcpu back on (by e.g. sending 
  a
  SIGP START to that vcpu).
 
 Yes, in that case that other CPU generates a signal (a different bit in 
 interrupt_request) and the first CPU would see that it has to wake up and 
 wake up.
 
  I am not sure if such a mechanism/scenario is applicable to any other arch. 
  They
  all seem to reset the cs-halted flag if they know they are able to run 
  (e.g.
  due to an interrupt) - they have no such thing as stopped cpus, only
  halted/waiting cpus.
 
 There's not really much difference between the two. The only difference from 
 a software point of view is that a stopped CPU has its external interrupt 
 bits masked off, no?

Well the difference is, that a STOPPED vcpu can be woken up by non-interrupt
like things (SIGP START) AND a special interrupt (SIGP RESTART - which is like
a SIPI++ as it performs a psw exchange - NMI). So we basically have two
paths that can lead to a state change. All interrupt bits may be in any
combination (SIGP RESTART interrupts can't be masked out, nor can SIGP START be
denied).

The other thing may be that on s390, each vcpu (including itself) can put
another vcpu into the STOPPED state - I assume that this is different for x86 
INIT_RECEIVED. For this reason we have to watch out for bad race conditions
(e.g. multiple vcpus working on another vcpu)...

David

 
 
 Alex
 

Re: [Qemu-devel] [PATCH/RFC 4/5] s390x/kvm: test whether a cpu is STOPPED when checking has_work

[David Hildenbrand]

 Il 28/07/2014 17:03, David Hildenbrand ha scritto:
  Well the difference is, that a STOPPED vcpu can be woken up by non-interrupt
  like things (SIGP START) AND a special interrupt (SIGP RESTART - which is 
  like
  a SIPI++ as it performs a psw exchange - NMI). So we basically have two
  paths that can lead to a state change.  All interrupt bits may be in any
  combination (SIGP RESTART interrupts can't be masked out, nor can SIGP 
  START be
  denied).
  
  The other thing may be that on s390, each vcpu (including itself) can put
  another vcpu into the STOPPED state - I assume that this is different for 
  x86 
  INIT_RECEIVED. For this reason we have to watch out for bad race conditions
  (e.g. multiple vcpus working on another vcpu)...
 
 You can do that in x86 by sending an INIT inter-processor interrupt.  A
 SIPI is ignored if the CPU is not in INIT_RECEIVED state.
 
 Commit 66450a21f99636af4fafac2afd33f1a40631bc3a introduced the current
 implementation.
 
 - an INIT cancels a previous SIPI;
 
 - if both INIT and SIPI are sent, on real hardware you need to have a
 few hundred microseconds between them, but KVM will reliably process
 INIT before SIPI.
 
 See commit 299018f44ac553dce3caf84df1d14c4764faa279 for an example of
 the races that can happen.
 
 Note that x86 has KVM_MP_STATE_SIPI_RECEIVED state but it is obsolete,
 we go straight from KVM_MP_STATE_INIT_RECEIVED to KVM_MP_STATE_RUNNABLE.
 

Thanks for the explanation Paolo!

Looks like from an interrupt point of view, the states have a lot in common.
The major thing that differs on s390 is probably the way these interrupts are
generated and what else they influence (all the power of the SIGP facility :)
+ special check-stop state that can't be left by an interrupt, only by SIGP
  CPU resets).

David

Re: [Qemu-devel] [PATCH/RFC 4/5] s390x/kvm: test whether a cpu is STOPPED when checking has_work

[David Hildenbrand]

  We have
  - wait (wait bit in PSW)
  - disabled wait (wait bit and interrupt fencing in PSW)
  - STOPPED (not related to PSW, state change usually handled via service 
  processor or hypervisor)
 
  I think we have to differentiate between KVM/TCG. On KVM we always do in 
  kernel halt and qemu sees a halted only for STOPPED or disabled wait. TCG 
  has to take care of the normal wait as well.
 
   From a first glimpse, a disabled wait and STOPPED look similar, but there 
  are (important) differences, e.g. other CPUs get a different a different 
  result from a SIGP SENSE. This makes a big difference, e.g. for Linux 
  guests, that send a SIGP STOP, followed by a SIGP SENSE loop until the CPU 
  is down on hotplug (and shutdown, kexec..) So I think we agree, that 
  handling the cpu states natively makes sense.
 
  The question is now only how to model it correctly without breaking TCG/KVM 
  and reuse as much common code as possible. Correct?
 
  Do I understand you correctly, that your collapsing of stopped and halted 
  is only in the qemu coding sense, IOW maybe we could just modify 
  kvm_arch_process_async_events to consider the STOPPED state, as TCGs sigp 
  implementation does not support SMP anyway?
 
 That works for me, yes.
 
 
 Alex
 

I had a look at it yesterday and it seems like we can totally drop this patch:

1. TCG doesn't support multiple CPUs and the TCG SIGP implementation isn't
ready for proper STOP/START/SENSE. Testing for STOPPED cpus in cpu_has_work()
can be dropped. To be able to support TCG was the main reason for this patch -
as we don't want to do so for now, we can leave it as is. We can still decide
to support the cpu states later using a mechanism suggest by Alex
(interrupt_requests).

Even if cpu_has_work() would make cpu.c:cpu_thread_is_idle() return false,
kvm_arch_process_async_events() called by kvm-all.c:kvm_cpu_exec() would make
it go back to sleep. Therefore a stopped VCPU will never be able to run in the
KVM case (because it always has cs-halted = true).

2. The unhalt in kvm_arch_process_async_events is for a special case where a
VCPU is in disabled wait and receives e.g. a machine-check interrupt. These
might happen in the future, for now we will never see them (the only
way to get a vcpu out of disabled wait are SIGP RESTART/CPU RESET - so we
don't break anything at that point).

David

Re: [Qemu-devel] [PATCH 4/4] s390x/kvm: hw debugging support via guest PER facility

[David Hildenbrand]

 On 30/05/14 11:01, Alexander Graf wrote:
  
  On 30.05.14 10:57, Christian Borntraeger wrote:
  On 30/05/14 10:32, Alexander Graf wrote:
 
  +case KVM_HW_BP:
  +if (find_hw_breakpoint(arch_info-addr, -1, arch_info-type)) {
  +ret = EXCP_DEBUG;
  +}
  +break;
  +case KVM_SINGLESTEP:
  +if (cs-singlestep_enabled) {
  +ret = EXCP_DEBUG;
  +}
  +break;
  +default:
  +ret = -ENOSYS;
  +}
  +
  +return ret;
  What happens to the diag 501 now? Are we safe to just drop it?
  There can only be a small number of HW breakpoints (basically only one 
  from-to range on s390).
  So gdb can (and will) use both (hbreak vs. break)
  
  Ah, let me explain what I'm referring to here. On x86 (and PPC, though the 
  patches are still missing), we use a generic breakpoint instruction for 
  sw breakpoints. The specific breakpoint interrupt generated by that 
  instruction traps into KVM which forwards it to QEMU.
  
  If QEMU now detects that it didn't put the breakpoint into place, it 
  assumes that it's the guest that wanted the breakpoint to happen, so it 
  deflects a breakpoint interrupt into the guest.
  
  My question here is whether we need something similar on s390x. With DIAG, 
  I think we're safe, as the guest can't expect that one to do anything 
  useful, but if we want to switch to a 2-byte breakpoint instruction 
  instead, it might make sense to implement the deflection mechanism.
 
 Oh, I though What happens to the diag 501 now? Are we safe to just drop it? 
 was a question if we can get rid of the code.
 Regarding deflection, yes if guest and host hardware breakpoints (PER) we 
 need to handle that (The host kernel is doing that in filter_guest_per_event)
 With software breakpoints: yes diag501 is safe to use. When we change the 
 instruction later on then we have to see if we need deflection (could be).
 
 Christian

Hi Alex,

I am already working on a solution for 2 byte software breakpoints.
The solution will most likely look like what we have on x86: A generic
breakpoint instruction (e.g. invalid opcode 0x0001) that is filtered in
QEMU. We'll need kernel support to allow invalid instructions to be
intercepted and handled in QEMU. I already have a prototype running.

David

Re: [Qemu-devel] [PATCH/RFC 0/5] s390x/kvm: track the logical cpu state in QEMU and propagate it to kvm

[David Hildenbrand]

 This is the qemu part of kernel series Let user space control the
 cpu states
 
 Christian Borntraeger (1):
   update linux headers with with cpustate changes
 
 David Hildenbrand (4):
   s390x/kvm: introduce proper states for s390 cpus
   s390x/kvm: proper use of the cpu states OPERATING and STOPPED
   s390x/kvm: test whether a cpu is STOPPED when checking has_work
   s390x/kvm: propagate s390 cpu state to kvm
 
  hw/s390x/ipl.c|   2 +-
  hw/s390x/s390-virtio.c|  32 --
  linux-headers/linux/kvm.h |   7 ++-
  target-s390x/cpu.c| 106 
 +++---
  target-s390x/cpu.h|  33 +--
  target-s390x/helper.c |  11 ++---
  target-s390x/kvm.c|  49 ++---
  trace-events  |   6 +++
  8 files changed, 179 insertions(+), 67 deletions(-)
 

Looks good to me.

Re: [Qemu-devel] [PATCH/RFC 0/5] s390x/kvm: track the logical cpu state in QEMU and propagate it to kvm

[David Hildenbrand]

 This is the qemu part of kernel series Let user space control the
 cpu states
 
 Christian Borntraeger (1):
   update linux headers with with cpustate changes
 
 David Hildenbrand (4):
   s390x/kvm: introduce proper states for s390 cpus
   s390x/kvm: proper use of the cpu states OPERATING and STOPPED
   s390x/kvm: test whether a cpu is STOPPED when checking has_work
   s390x/kvm: propagate s390 cpu state to kvm
 
  hw/s390x/ipl.c|   2 +-
  hw/s390x/s390-virtio.c|  32 --
  linux-headers/linux/kvm.h |   7 ++-
  target-s390x/cpu.c| 106 
 +++---
  target-s390x/cpu.h|  33 +--
  target-s390x/helper.c |  11 ++---
  target-s390x/kvm.c|  49 ++---
  trace-events  |   6 +++
  8 files changed, 179 insertions(+), 67 deletions(-)
 

Looks good to me

Re: [Qemu-devel] [PATCH/RFC 0/5] s390x/kvm: track the logical cpu state in QEMU and propagate it to kvm

[David Hildenbrand]

  This is the qemu part of kernel series Let user space control the
  cpu states
  
  Christian Borntraeger (1):
update linux headers with with cpustate changes
  
  David Hildenbrand (4):
s390x/kvm: introduce proper states for s390 cpus
s390x/kvm: proper use of the cpu states OPERATING and STOPPED
s390x/kvm: test whether a cpu is STOPPED when checking has_work
s390x/kvm: propagate s390 cpu state to kvm
  
   hw/s390x/ipl.c|   2 +-
   hw/s390x/s390-virtio.c|  32 --
   linux-headers/linux/kvm.h |   7 ++-
   target-s390x/cpu.c| 106 
  +++---
   target-s390x/cpu.h|  33 +--
   target-s390x/helper.c |  11 ++---
   target-s390x/kvm.c|  49 ++---
   trace-events  |   6 +++
   8 files changed, 179 insertions(+), 67 deletions(-)
  
 
 Looks good to me
 
 --
 To unsubscribe from this list: send the line unsubscribe kvm in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 

@all thought it was the final internal review :)

Re: [Qemu-devel] [PATCH 5/5] gdb: provide the name of the architecture in the target.xml

[David Hildenbrand]

 [ccing Andreas in case he wants to review the QOM aspects of this,
 though they're fairly straightforward I think.]
 
 On 29 August 2014 14:52, Jens Freimann jf...@linux.vnet.ibm.com wrote:
  From: David Hildenbrand d...@linux.vnet.ibm.com
 
  This patch provides the name of the architecture in the target.xml if 
  available.
 
  This allows the remote gdb to detect the target architecture on its own - so
  there is no need to specify it manually (e.g. if gdb is started without a
  binary) using set arch *arch_name*.
 
 This is neat; I didn't realise gdb let you do this.
 
  The name of the architecture has been added to all archs that provide a
  target.xml (by supplying a gdb_core_xml_file) and have a unique architecture
  name in gdb's feature xml files.
 
 What about 32-bit ARM? You set the architecture name for AArch64
 but not the 32 bit case.
 

Well, my point was to not break anything :)

On my way through the possible architecture names
(binutils-gdb/gdb/features/*.xml), I wasn't able to come up with the right name
for arm 32 bit (arm-core.xml) - they don't specify any. This patch therefore
adapts to the xml files from gdb.

The architecture should be known at the same point when specifying the xml file.
So if anyone can come up with the proper arm name in the future (or even some
kind of detection algorithm), it can simply be set in target-arm/cpu.c (after
arm-core.xml).

David

Re: [Qemu-devel] [PATCH 5/5] gdb: provide the name of the architecture in the target.xml

[David Hildenbrand]

 On Wed, Sep 03, 2014 at 11:37:24AM +0200, David Hildenbrand wrote:
   [ccing Andreas in case he wants to review the QOM aspects of this,
   though they're fairly straightforward I think.]
   
   On 29 August 2014 14:52, Jens Freimann jf...@linux.vnet.ibm.com wrote:
From: David Hildenbrand d...@linux.vnet.ibm.com
   
This patch provides the name of the architecture in the target.xml if 
available.
   
This allows the remote gdb to detect the target architecture on its own 
- so
there is no need to specify it manually (e.g. if gdb is started without 
a
binary) using set arch *arch_name*.
   
   This is neat; I didn't realise gdb let you do this.
   
The name of the architecture has been added to all archs that provide a
target.xml (by supplying a gdb_core_xml_file) and have a unique 
architecture
name in gdb's feature xml files.
   
   What about 32-bit ARM? You set the architecture name for AArch64
   but not the 32 bit case.
   
  
  Well, my point was to not break anything :)
  
  On my way through the possible architecture names
  (binutils-gdb/gdb/features/*.xml), I wasn't able to come up with the right 
  name
  for arm 32 bit (arm-core.xml) - they don't specify any. This patch therefore
  adapts to the xml files from gdb.
  
  The architecture should be known at the same point when specifying the xml 
  file.
  So if anyone can come up with the proper arm name in the future (or even 
  some
  kind of detection algorithm), it can simply be set in target-arm/cpu.c 
  (after
  arm-core.xml).
 
 Hi,
 
 I've got some similar patches in my tree. I used the following:
 

Thanks! So arm seems to be the proper name for arm32, right?

 
 commit 26932a453da466d111b67c37b93dec71fb3ae111
 Author: Edgar E. Iglesias edgar.igles...@xilinx.com
 Date:   Wed Aug 20 19:22:10 2014 +1000
 
 gdbstub: Emit the CPUs GDB architecture if available
 
 Allows GDB to autodetect the architecture.
 
 Signed-off-by: Edgar E. Iglesias edgar.igles...@xilinx.com
 
 diff --git a/gdbstub.c b/gdbstub.c
 index 7f82186..5b62c50 100644
 --- a/gdbstub.c
 +++ b/gdbstub.c
 @@ -604,6 +604,11 @@ static const char *get_feature_xml(const char *p, const 
 char **newp,
  pstrcat(target_xml, sizeof(target_xml), r-xml);
  pstrcat(target_xml, sizeof(target_xml), \/);
  }
 +if (cc-gdb_arch) {
 +pstrcat(target_xml, sizeof(target_xml), architecture);
 +pstrcat(target_xml, sizeof(target_xml), cc-gdb_arch);
 +pstrcat(target_xml, sizeof(target_xml), /architecture);
 +}

Please not that gdb-target.dtd specifies the architecture to come directly at
the beginning of the target section.

Putting it after the xml-includes, to the end of the target section makes the
whole XML failing to be recognized on my tests with s390x.

David

  pstrcat(target_xml, sizeof(target_xml), /target);
  }
  return target_xml;
 

Re: [Qemu-devel] [PATCH] gdb: provide the name of the architecture in the target.xml

[David Hildenbrand]

 On 6 October 2014 16:08, Cornelia Huck cornelia.h...@de.ibm.com wrote:
  On Tue, 30 Sep 2014 17:23:47 +0200
  Jens Freimann jf...@linux.vnet.ibm.com wrote:
 
  From: David Hildenbrand d...@linux.vnet.ibm.com
 
  This patch provides the name of the architecture in the target.xml if 
  available.
 
  This allows the remote gdb to detect the target architecture on its own - 
  so
  there is no need to specify it manually (e.g. if gdb is started without a
  binary) using set arch *arch_name*.
 
  The name of the architecture has been added to all archs that provide a
  target.xml (by supplying a gdb_core_xml_file) and have a unique 
  architecture
  name in gdb's feature xml files.
 
  Signed-off-by: David Hildenbrand d...@linux.vnet.ibm.com
  Acked-by: Cornelia Huck cornelia.h...@de.ibm.com
  Acked-by: Christian Borntraeger borntrae...@de.ibm.com
  Signed-off-by: Jens Freimann jf...@linux.vnet.ibm.com
  Reviewed-by: Andreas Färber afaer...@suse.de
  Cc: Andrzej Zaborowski balr...@gmail.com
  Cc: Peter Maydell peter.mayd...@linaro.org
  Cc: Vassili Karpov (malc) av1...@comtv.ru
  CC: Edgar Iglesias edgar.igles...@gmail.com
  CC: Richard Henderson r...@twiddle.net
  ---
   gdbstub.c   | 19 ---
   include/qom/cpu.h   |  2 ++
   target-arm/cpu64.c  |  1 +
   target-ppc/translate_init.c |  2 ++
   target-s390x/cpu.c  |  1 +
   5 files changed, 18 insertions(+), 7 deletions(-)
 
  I will send this with the next pile of s390x updates, unless someone on
  cc: has any objections.
 
 I'm still hoping for an answer about why this is setting
 the name for 64 bit ARM and not 32 bit ARM, and whether
 there are any cases which need to actually be able to set
 the architecture name in a more complicated name than
 simply a string. I raised those in the last lot of review
 and there doesn't seem to have been any answer.
 
 thanks
 -- PMM
 

Hi Peter,

actually the questions were addressed in the last review. Haven't received any
answer from you to my reply. Maybe some mails got lost in the system.

32bit arm:
-On my way through the possible architecture names
  (binutils-gdb/gdb/features/*.xml), I wasn't able to come up with the right 
name
  for arm 32 bit (arm-core.xml) - they don't specify any. This patch therefore
  adapts to the xml files from gdb.
- Not included in this patch as Edgar provided a patch in the previous thread
  (that's why he is on cc) that can easily be adopted. I don't want to simply
  include his effort in my patch :) And we have to make sure that this name is
  the right one.

More complicated names:
- The architecture should be known at the same point when specifying the xml
  file. So if anyone can come up with the proper arm name in the future (or 
even some
  kind of detection algorithm), it can simply be set in target-arm/cpu.c (after
  arm-core.xml).
- The same should apply for all architectures. So we can set (or even build)
  the proper string when also specifying the core xml file.

Do you have something in mind like your powerpc:common and powerpc:e500
example? To build the names based on some pattern?

I don't think that we can generalize the name format here (at least aarch64
makes me assume that :) ). I think it would be enough to set the complete
strings in the class init functions.

What do you think? Any suggestions?

Thanks!

David

Re: [Qemu-devel] [PATCH] gdb: provide the name of the architecture in the target.xml

[David Hildenbrand]

 On 30 September 2014 16:23, Jens Freimann jf...@linux.vnet.ibm.com wrote:
  From: David Hildenbrand d...@linux.vnet.ibm.com
 
  This patch provides the name of the architecture in the target.xml if 
  available.
 
  This allows the remote gdb to detect the target architecture on its own - so
  there is no need to specify it manually (e.g. if gdb is started without a
  binary) using set arch *arch_name*.
 
  The name of the architecture has been added to all archs that provide a
  target.xml (by supplying a gdb_core_xml_file) and have a unique architecture
  name in gdb's feature xml files.
 
 gdb seems to support more than one powerpc architecture
 name. Do we need to report powerpc:e500 for
 our e500 cpu models, for instance?
 
 -- PMM
 

Hi Peter,

good point. I was hoping for more feedback from the powerpc folks.

My gdb multi-arch seems to support the following architectures:

(gdb) set arch
Requires an argument. Valid arguments are i386, i386:x86-64, i386:x64-32, 
i8086, i386:intel, i386:x86-64:intel, i386:x64-32:intel, i386:nacl, 
i386:x86-64:nacl, i386:x64-32:nacl, s390:64-bit, rs6000:6000, rs6000:rs1, 
rs6000:rsc, rs6000:rs2, powerpc:common64, powerpc:common, powerpc:603, 
powerpc:EC603e, powerpc:604, powerpc:403, powerpc:601, powerpc:620, 
powerpc:630, powerpc:a35, powerpc:rs64ii, powerpc:rs64iii, powerpc:7400, 
powerpc:e500, powerpc:e500mc, powerpc:e500mc64, powerpc:MPC8XX, powerpc:750, 
powerpc:titan, powerpc:vle, powerpc:e5500, powerpc:e6500, arm, armv2, armv2a, 
armv3, armv3m, armv4, armv4t, armv5, armv5t, armv5te, xscale, ep9312, iwmmxt, 
iwmmxt2, aarch64, aarch64:ilp32, auto.

However I am not sure if there are duplicates / compatible ones among them. The
available registers are all defined in the XML supplied by the gdbserver - so
I am not sure if they are part of the more specific architecture names.

Maybe it makes sense to leave powerpc and arm out of this patch. So I would
just set s390:64-bit in the first shot (unless there are any experts saying
that e.g. powerpc:common always works). At least for s390:64-bit I am very
sure :)

Of course, the mechanism to set the name should be flexible enough (if we find
the existing one to be too strict).

Thanks!

David

Re: [Qemu-devel] [PATCH] gdb: provide the name of the architecture in the target.xml

[David Hildenbrand]

 On 6 October 2014 20:14, David Hildenbrand d...@linux.vnet.ibm.com wrote:
  actually the questions were addressed in the last review. Haven't received 
  any
  answer from you to my reply. Maybe some mails got lost in the system.
 
  32bit arm:
  -On my way through the possible architecture names
(binutils-gdb/gdb/features/*.xml), I wasn't able to come up with the 
  right name
for arm 32 bit (arm-core.xml) - they don't specify any. This patch 
  therefore
adapts to the xml files from gdb.
 
 But can we specify it for 32 bit ARM? Saying set arch arm at
 a gdb prompt works OK, so can we pass arm through in the XML too?
 The gdb documentation says that anything that works as an arch
 string at the gdb prompt is OK to pass through in the XML.

Yes, arm should work. But this whole armvX thing (see my other mail) makes me
wonder which is the right one. And if there is any difference (at least for
our purpose).

 
  - Not included in this patch as Edgar provided a patch in the previous 
  thread
(that's why he is on cc) that can easily be adopted. I don't want to 
  simply
include his effort in my patch :) And we have to make sure that this name 
  is
the right one.
 
  More complicated names:
  - The architecture should be known at the same point when specifying the 
  xml
file. So if anyone can come up with the proper arm name in the future (or 
  even some
kind of detection algorithm), it can simply be set in target-arm/cpu.c 
  (after
arm-core.xml).
 
 At the moment we specify the XML file in the CPU class init
 function, whereas specific CPU subfeatures may not be known
 until rather later when we actually instantiate a CPU
 object. Are there any cases where we'd actually need to care?
 I can't tell, because you seem to be taking the we're going
 to ignore all the CPU types where the answer isn't obvious
 approach. The ARM iwmmxt architecture would be an interesting
 one to check, since if we really need to use that string rather
 than a generic arm string we need to postpone choosing the
 name til CPU init time when we set up the feature bits.

You're right, I was most likely only thinking about the kvm case :) But we of
course chose to emulate (almost) every cpu version/feature we want.

Even if the e500 might work with the common definition, you identified one
potential case that won't work, so we can't go with this generic approach.

So should each architecture rather provide a function that gives the name back?
Like
- NULL if not known
- Generic name if not further specified
- Complex name if specific version was initialized?

 
  - The same should apply for all architectures. So we can set (or even build)
the proper string when also specifying the core xml file.
 
  Do you have something in mind like your powerpc:common and powerpc:e500
  example? To build the names based on some pattern?
 
  I don't think that we can generalize the name format here (at least 
  aarch64
  makes me assume that :) ). I think it would be enough to set the complete
  strings in the class init functions.
 
 Only if the complete string doesn't depend on things we don't
 know at that point. Maybe we can always pick a generic
 name for each architecture, in which case we should do that
 (and do it for all architectures now, not only three).
 What does gdb do differently if you specify powerpc:common
 versus powerpc:e500, for instance? Is it anything we care
 about?

I would have thought that the generic one would have worked for us (especially
because the gdb supplied xml files don't contain most of the special names). But
you point was right.

We should rather focus on the really supported architectures than on the ones
given in the xml files.

Thanks!

David

Re: [Qemu-devel] [PATCH v4 1/5] block: add bdrv functions for geometry and blocksize

[David Hildenbrand]

 Add driver functions for geometry and blocksize detection
 
 Signed-off-by: Ekaterina Tumanova tuman...@linux.vnet.ibm.com
 Reviewed-by: Thomas Huth th...@linux.vnet.ibm.com
 ---
  block.c   | 34 ++
  include/block/block.h | 13 +
  include/block/block_int.h | 15 +++
  3 files changed, 62 insertions(+)
 
 diff --git a/block.c b/block.c
 index 4165d42..93409f5 100644
 --- a/block.c
 +++ b/block.c
 @@ -548,6 +548,40 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error 
 **errp)
  }
  }
 
 +/**
 + * Get @bs's logical and physical block size, store them in @bsz.
 + * @bs must not be empty.
 + */
 +void bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
 +{
 +BlockDriver *drv = bs-drv;
 +
 +assert(drv != NULL);
 +if (drv-bdrv_probe_blocksizes 
 +!drv-bdrv_probe_blocksizes(bs, bsz)) {

So we want to ignore/drop any error?

 +return;
 +}


Looks good to me.

David

Re: [Qemu-devel] [PATCH for-2.5 7/8] s390x: Migrate guest storage keys (initial memory only)

[David Hildenbrand]

 
 So if I've got this code right, you send here a header that announces
 a packet with all pages ...
 
  +while (handled_count  total_count) {
  +cur_count = MIN(total_count - handled_count, 
  S390_SKEYS_BUFFER_SIZE);
  +
  +ret = skeyclass-get_skeys(ss, cur_gfn, cur_count, buf);
  +if (ret  0) {
  +error_report(S390_GET_KEYS error %d\n, ret);
  +break;
 
 ... but when an error occurs here, you suddenly stop in the middle of
 that packet with all pages ...

Indeed, although that should never fail, we never know.
We don't want to overengineer the protocol but still abort migration at least
on the loading side in that (theoretical) case.

 
  +}
  +
  +/* write keys to stream */
  +qemu_put_buffer(f, buf, cur_count);
  +
  +cur_gfn += cur_count;
  +handled_count += cur_count;
  +}
  +
  +g_free(buf);
  +end_stream:
  +qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_EOS);
 
 ... and send an EOS marker here instead ...
 
  +}
  +
  +static int s390_storage_keys_load(QEMUFile *f, void *opaque, int 
  version_id)
  +{
  +S390SKeysState *ss = S390_SKEYS(opaque);
  +S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
  +int ret = 0;
  +
  +while (!ret) {
  +ram_addr_t addr;
  +int flags;
  +
  +addr = qemu_get_be64(f);
  +flags = addr  ~TARGET_PAGE_MASK;
  +addr = TARGET_PAGE_MASK;
  +
  +switch (flags) {
  +case S390_SKEYS_SAVE_FLAG_SKEYS: {
  +const uint64_t total_count = qemu_get_be64(f);
  +uint64_t handled_count = 0, cur_count;
  +uint64_t cur_gfn = addr / TARGET_PAGE_SIZE;
  +uint8_t *buf = g_try_malloc(S390_SKEYS_BUFFER_SIZE);
  +
  +if (!buf) {
  +error_report(storage key load could not allocate 
  memory\n);
  +ret = -ENOMEM;
  +break;
  +}
  +
  +while (handled_count  total_count) {
  +cur_count = MIN(total_count - handled_count,
  +S390_SKEYS_BUFFER_SIZE);
  +qemu_get_buffer(f, buf, cur_count);
 
 ... while the receiver can not handle the EOS marker here.
 
 This looks fishy to me (or I might have just missed something), but
 anyway please double check whether your error handling in the sender
 really makes sense.

My shot would be, to send invalid storage keys if getting the keys from the
kernel fails. So we can detect it on the loading side and abort migration
gracefully.

 
  +ret = skeyclass-set_skeys(ss, cur_gfn, cur_count, buf);
  +if (ret  0) {
  +error_report(S390_SET_KEYS error %d\n, ret);
  +break;
  +}
  +handled_count += cur_count;
  +cur_gfn += cur_count;
  +}
  +g_free(buf);
  +break;
  +}
  +case S390_SKEYS_SAVE_FLAG_EOS:
  +/* normal exit */
  +return 0;
  +default:
  +error_report(Unexpected storage key flag data: %#x, flags);
  +ret = -EINVAL;
  +}
  +}
  +
  +return ret;
  +}
 
  Thomas

Thanks Thomas!


David

Re: [Qemu-devel] [PULL 0/9] Next set of s390x patches

[David Hildenbrand]

> On Tue, 20 Oct 2015 16:35:44 +0100
> Peter Maydell  wrote:
> 
> > On 20 October 2015 at 16:00, Cornelia Huck  wrote:
> > > The following changes since commit 
> > > ee9dfed242610ecb91418270fd46b875ed56e201:
> > >
> > >   Merge remote-tracking branch 
> > > 'remotes/kraxel/tags/pull-input-20151020-1' into staging (2015-10-20 
> > > 12:56:45 +0100)
> > >
> > > are available in the git repository at:
> > >
> > >   git://github.com/cohuck/qemu tags/s390x-20151020
> > >
> > > for you to fetch changes up to bde5d1bcb63973802c21183d730e0ea7c0ae227c:
> > >
> > >   s390x/cmma: clean up cmma reset (2015-10-20 16:21:01 +0200)
> > >
> > > 
> > > More s390x patches. The first ones are fixes: A regression, missed
> > > compat and a missed part of the SIMD support. The others contain
> > > optimizations and cleanup.
> > >
> > > 
> > 
> > Hi; I'm afraid this fails to build with clang:
> > /home/petmay01/linaro/qemu-for-merges/hw/s390x/s390-virtio-ccw.c:38:12:
> > error: duplicate 'const' declaration specifier
> > [-Werror,-Wduplicate-decl-specifier]
> > const char const *reset_dev_types[] = {
> >^
> > 
> > 
> > 
> > Did you mean "const char * const" ? (I always have trouble with this
> > corner of C syntax...)
> 
> Not only you...
> 
> David, can you take a look at this?

Looks like I'm also having trouble with it ;) Guess I should start to also
compile with clang.

yes, "const char * const" should be the right thing ... if not simply "const
char *".

David

Re: [Qemu-devel] [PATCH 3/4] virtio-ccw: feature bits > 31 handling

[David Hildenbrand]

> We currently switch off the VERSION_1 feature bit if the guest has
> not negotiated at least revision 1. As no feature bits beyond 31 are
> valid however unless VERSION_1 has been negotiated, make sure that
> legacy guests never see a feature bit beyond 31.
> 
> Signed-off-by: Cornelia Huck <cornelia.h...@de.ibm.com>
> ---
>  hw/s390x/virtio-ccw.c | 21 -
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 85e2a5d..eed7b3e 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -468,15 +468,12 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>  NULL);
>  if (features.index == 0) {
>  features.features = (uint32_t)vdev->host_features;
> -} else if (features.index == 1) {
> -features.features = (uint32_t)(vdev->host_features >> 32);
> +} else if ((features.index == 1) && (dev->revision >= 1)) {
>  /*
> - * Don't offer version 1 to the guest if it did not
> - * negotiate at least revision 1.
> + * Only offer feature bits beyond 31 if the guest has
> + * negotiated at least revision 1.
>   */
> -if (dev->revision <= 0) {
> -features.features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> -}
> +features.features = (uint32_t)(vdev->host_features >> 32);
>  } else {
>  /* Return zeroes if the guest supports more feature bits. */
>  features.features = 0;
> @@ -515,14 +512,12 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>  virtio_set_features(vdev,
>  (vdev->guest_features & 
> 0xULL) |
>  features.features);
> -} else if (features.index == 1) {
> +} else if ((features.index == 1) && (dev->revision >= 1)) {
>  /*
> - * The guest should not set version 1 if it didn't
> - * negotiate a revision >= 1.
> + * If the guest did not negotiate at least revision 1,
> + * we did not offer it any feature bits beyond 31. Such a
> + * guest passing us any bit here is therefore buggy.
>   */
> -if (dev->revision <= 0) {
> -features.features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> -    }
>  virtio_set_features(vdev,
>  (vdev->guest_features & 
> 0xULL) |
>  ((uint64_t)features.features << 32));

Looks sane to me!

Reviewed-by: David Hildenbrand <d...@linux.vnet.ibm.com>

David

Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices

[David Hildenbrand]

> No knowledge should be required for object_new(). Classes' instance_init
> functions should have no side-effects outside the object itself.

While this should theoretically be true, I can guarantee to you that this is
not the case for all devices :) (especially as there are too many unwritten
laws related to that whole qmp/qom thing flying around).

E.g. we can save ourselves from the creation of other devices (init + realize)
by setting a device to !hotpluggable in the device class. This code simply
bypasses such checks and assumes that QEMUs internal structure can deal with
that. We saw that this doesn't work this far.

Other interfaces (object_add) seem to rely on TYPE_USER_CREATABLE, so we can't
trigger "everything" from outside QEMU.

One can argue that we simply have to fix the devices - nevertheless I think this
is the wrong approach to the problem.

> 
> > 
> > Feels a little like hacking an interface to a java program, which allows to
> > create any object of a special kind dynamically (constructor arguments?),
> > reading some member variable (names) via reflections and then throwing that
> > object away. Which sounds very wrong to me.
> 
> I wouldn't call it wrong. Confusing, maybe. We use a mix of class-based
> and prototype-based approaches to inheritance and property
> introspection.
> 
> > 
> > Wonder if it wouldn't make more sense to query only the static properties 
> > of a
> > device. I mean if the dynamic properties are dynamic by definition (and can
> > change during runtime). So looking up the static properties via the typename
> > feels more KIS-style to me. Of course, the relevant properties would have 
> > to be
> > defined statically then, which shouldn't be a problem.
> 
> It depends on your definition of "shouldn't be a problem". :)

Well, it might require some internal rework of that property thingy, hehe ;)

> 
> The static property system is more limited than the QOM dynamic property
> system, today. We already depend on introspection of dynamic properties
> registered by instance_init functions, so we would need to move
> everything to a better static property system if we want to stop doing
> object_new() during class introspection without regressions.
> 
> > 
> > Dynamic properties that are actually created could depend on almost
> > everything in the system - why fake something that we don't know.
> 
> Properties registered on instance_init shouldn't depend on anything else
> on the system. Properties registered later in the object lifetime (e.g.
> on realize) can.
> 

Okay, so if the properties in instance_init should depend on nothing else in
the system, they are always the same for one QEMU version + device.

So maybe it would make sense to define all these "fixed properties" on a device
class level (e.g. via dc->props) and fill them with life afterwards (if we can't
completely specify them at that point). Of course this would need some
rework/careful thinking (e.g. concept of uninitialized properties).

Looking at the errors we get with that approach tells me that it will easily
break again in the future (we need tests for all devices - do we already have
it?) and makes me wonder if a definition on a device class level wouldn't be the
right thing to do - a clean interface description that is valid for each device
instance.

David

Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices

[David Hildenbrand]

> Am 18.09.2015 um 14:00 schrieb Markus Armbruster:
> > Several devices don't survive object_unref(object_new(T)): they crash
> > or hang during cleanup, or they leave dangling pointers behind.
> > 
> > This breaks at least device-list-properties, because
> > qmp_device_list_properties() needs to create a device to find its
> > properties.  Broken in commit f4eb32b "qmp: show QOM properties in
> > device-list-properties", v2.1.  Example reproducer:
> > 
> > $ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp 
> > stdio
> > {"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, 
> > "package": ""}, "capabilities": []}}
> > { "execute": "qmp_capabilities" }
> > {"return": {}}
> > { "execute": "device-list-properties", "arguments": { "typename": 
> > "pxa2xx-pcmcia" } }
> > qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: 
> > memory_region_finalize: Assertion `((>subregions)->tqh_first == ((void 
> > *)0))' failed.
> > Aborted (core dumped)
> > [Exit 134 (SIGABRT)]
> > 
> > Unfortunately, I can't fix the problems in these devices right now.
> > Instead, add DeviceClass member cannot_even_create_with_object_new_yet
> > to mark them:
> > 
> > * Crash or hang during cleanup (didn't debug them, so I can't say
> >   why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
> >   "s390-sclp-event-facility", "sclp"
> 
> Ack for the sclp things. Theses devices are created by the machine and
> sclp creates the event-facility, so not having a way to query properties
> for these devices is better than a hang.
> 
> David, can you have a look on why these devices fail as outlined?
> 

The problem was that the quiesce and cpu hotplug sclp event devices had no
parent (onoly a parent bus). So when the bus (inside the event facility) was
removed, it looped forever trying remove/unparent it's "bus childs" (which had
no parent).

sclp (parent=machine)
-> even-facility (parent=sclp)
-> bus (parent=event-facility)
  -> quiesce (parent=null)
  -> cpu hotplug (parent=null)

Maybe that helps others struggling with the same symptoms.


Just a quick comment on the introspection:

I don't think it is a good idea to expose properties that way. Temporarily
creating devices for the sake of querying property names sounds very wrong to
me, because certain devices require certain knowledge about how and when they
can be created.

Feels a little like hacking an interface to a java program, which allows to
create any object of a special kind dynamically (constructor arguments?),
reading some member variable (names) via reflections and then throwing that
object away. Which sounds very wrong to me.

Wonder if it wouldn't make more sense to query only the static properties of a
device. I mean if the dynamic properties are dynamic by definition (and can
change during runtime). So looking up the static properties via the typename
feels more KIS-style to me. Of course, the relevant properties would have to be
defined statically then, which shouldn't be a problem.

Dynamic properties that are actually created could depend on almost
everything in the system - why fake something that we don't know.

David

Re: [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err

[David Hildenbrand]

> On Mon, 14 Dec 2015 10:59:36 +0100
> Markus Armbruster <arm...@redhat.com> wrote:
> 
> > Separate patch, like this:
> > 
> > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> > index 9a117c9..74f2b40 100644
> > --- a/hw/s390x/sclp.c
> > +++ b/hw/s390x/sclp.c
> > @@ -463,21 +463,18 @@ static void sclp_realize(DeviceState *dev, Error 
> > **errp)
> >  object_property_set_bool(OBJECT(sclp->event_facility), true, 
> > "realized",
> >   );
> >  if (err) {
> > -goto error;
> > +goto out;
> >  }
> > 
> >  ret = s390_set_memory_limit(machine->maxram_size, _limit);
> >  if (ret == -E2BIG) {
> >  error_setg(, "qemu: host supports a maximum of %" PRIu64 " GB",
> > hw_limit >> 30);
> > -goto error;
> >  } else if (ret) {
> >  error_setg(, "qemu: setting the guest size failed");
> > -goto error;
> >  }
> > -return;
> > -error:
> > -    assert(err);
> > +
> > +out:
> >  error_propagate(errp, err);
> >  }
> > 
> > If you like it, I'll stick it into my respin.
> 
> Not speaking for David, but this would get my Ack.

Yes, this looks good to me!

Reviewed-by: David Hildenbrand <d...@linux.vnet.ibm.com>

Thanks!

David

Re: [Qemu-devel] [PATCH 4/4] hw/s390x: Rename local variables Error *l_err to just err

[David Hildenbrand]

> 
>  static Property s390_ipl_properties[] = {
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index a061b49..9a117c9 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -456,29 +456,29 @@ static void sclp_realize(DeviceState *dev, Error **errp)
>  {
>  MachineState *machine = MACHINE(qdev_get_machine());
>  SCLPDevice *sclp = SCLP(dev);
> -Error *l_err = NULL;
> +Error *err = NULL;
>  uint64_t hw_limit;
>  int ret;
> 
>  object_property_set_bool(OBJECT(sclp->event_facility), true, "realized",
> - _err);
> -if (l_err) {
> + );
> +if (err) {
>  goto error;
>  }
> 
>  ret = s390_set_memory_limit(machine->maxram_size, _limit);
>  if (ret == -E2BIG) {
> -error_setg(_err, "qemu: host supports a maximum of %" PRIu64 " GB",
> +error_setg(, "qemu: host supports a maximum of %" PRIu64 " GB",
> hw_limit >> 30);
>  goto error;
>  } else if (ret) {
> -error_setg(_err, "qemu: setting the guest size failed");
> +error_setg(, "qemu: setting the guest size failed");
>  goto error;
>  }
>  return;
>  error:
> -assert(l_err);
> -error_propagate(errp, l_err);
> +assert(err);

We could also get rid of that assert and remove the return; above (naming the
label "out").

> +error_propagate(errp, err);
>  }
> 
>  static void sclp_memory_init(SCLPDevice *sclp)

Reviewed-by: David Hildenbrand <d...@linux.vnet.ibm.com>

David

Re: [Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[David Hildenbrand]

> Current CLI option -cpu cpux,features serves as template
> for all created cpus of type: cpux. However QEMU parses
> "features" every time it creates a cpu instance and applies
> them to it while doing parsing.
> 
> That doesn't work well with -device/device_add infrastructure
> as it has no idea about cpu specific hooks that's used for
> parsing "features".
> In order to make -device/device_add utilize "-cpu features"
> template convert it into a set of global properties, so that
> every new CPU created will have them applied automatically.
> 
> That also allows to parse features only once, instread of
> doing it for every CPU instance created.

While I think this makes sense for most cases, we (s390x) are
currently working on a mechanism to compare and baseline cpu models via
a qmp interface, to not have to replicate CPU models in libvirt
every time we do some changes.

To do that, we are creating temporary CPUs to handle the model
parsing. So, with our current prototype, we rely on the mechanism
to parse properties multiple time, as we are really creating
different CPUs.

While we could somehow change our mechanism I don't think this is
the right thing to do.

We will have to support heterogeneous cpu models (I think arm was one of
the guys requesting this if I'm not mistaking) and it somehow
contradicts to the general mechanism of device_add fully specifying
parameters. These would now be implicit parameters.

Would it be possible to do this for x86 only? Or find another way
to handle the "create just another ordinary CPU we already have"?

David

Re: [Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[David Hildenbrand]

> I assume Igor explained it, already, and his suggestion sounds OK
> to you. But I will answer your questions to confirm that this is
> really the case:

Yes, this all sounds good to me, thanks for the additional explanation!
[...]

> 
> > 
> > d) has to work for us. Otherwise we will have to fallback to manual
> > property parsing.  
> 
> It will be affected by the globals, but I assume management code
> is not going to use add extra -cpu arguments when probing for CPU
> model information, right?

Yes, that's also what I figured out, this should not be a problem.

> 
> Users will need to be aware that -cpu is equivalent to -global,
> and will affect CPU information returned by query-cpu-definitions
> (or similar commands).
> 
> >   
> > > 
> > > If all you need is to parse properties, why can't you reuse the
> > > existing QOM/Device mechanisms to handle properties (the one used
> > > by -device and device_add), instead of the -cpu code?  
> > 
> > We can, if my given example works. And the global properties
> > don't interfere with cpus.  
> 
> They do, but only the model specified in -cpu.
> 
> >   
> > > 
> > > We need to use less of the infrastructure that exists for the
> > > legacy -cpu option (and use more of the generic QOM/Device
> > > mechanisms), not more of it.  
> > 
> > It is better to have one way of creating cpus that two.  
> 
> Unfortunately we already have two ways of creating CPUs: -cpu and
> device_add. We are trying to translate -cpu to something
> equivalent to generic mechanisms (-device and -global), so we
> have only one underlying mechanism.

Yes, understood. Sounds sane to me!
[...]

> I understand that this may be confusing, but that's because -smp
> and -cpu don't fit the QEMU device/object models. We are moving
> towards allowing CPU topologies to be created using only -device.
> 
> To do that, we are gradually translating -cpu to the generic
> mechanisms used by -device/-global, so command-line options could
> be easily converted to use the new mechanisms in the future.
> 
> Does that make sense?
> 

Absolutely!

David

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[David Hildenbrand]

> It is not exactly a special case (it is a read-only property like
> any other), but it's worth mentioning. I will change it to:
> 
> # If the QOM property is read-only, that means there's no known
> # way to make the CPU model run in the current host. If
> # absolutely no extra information will be returned to explain why
> # the CPU model is not runnable, implementations may simply
> # return "type" as the property name.
> 
> >   
> > > +# If @unavailable-features is an empty list, the CPU model is
> > > +# runnable using the current host and machine-type.
> > > +# If @unavailable-features is not present, runnability
> > > +# information for the CPU model is not available.
> > >  #
> > >  # Since: 1.2.0
> > >  ##  
> > 
> > I'm happy with this interface.  Thanks!  
> 
> Thanks!
> 

Yes, sounds also good to me. For "hw generation too new" and similar errors
we will simply return "type". Thanks.

David

Re: [Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[David Hildenbrand]

> On Thu, Jun 02, 2016 at 22:44:49 +0200, David Hildenbrand wrote:
> > > Current CLI option -cpu cpux,features serves as template
> > > for all created cpus of type: cpux. However QEMU parses
> > > "features" every time it creates a cpu instance and applies
> > > them to it while doing parsing.
> > > 
> > > That doesn't work well with -device/device_add infrastructure
> > > as it has no idea about cpu specific hooks that's used for
> > > parsing "features".
> > > In order to make -device/device_add utilize "-cpu features"
> > > template convert it into a set of global properties, so that
> > > every new CPU created will have them applied automatically.
> > > 
> > > That also allows to parse features only once, instread of
> > > doing it for every CPU instance created.  
> > 
> > While I think this makes sense for most cases, we (s390x) are
> > currently working on a mechanism to compare and baseline cpu models via
> > a qmp interface, to not have to replicate CPU models in libvirt
> > every time we do some changes.  
> 
> BTW, we don't replicate any change to QEMU CPU models in libvirt. We
> merely add new CPU models that match the current QEMU definition.
> Existing CPU models are never changed. And while the CPU models are
> currently used to check compatibility with host CPU, we will stop doing
> that in the near future and our CPU models will mostly be used for
> detecting what the host CPU is.
> 
> Jirka
> 


Hi Jirka,

thanks for that info!

unfortunately we will have to perform changes to the cpu models.
On z we have various cpu features that are confidential and only
added over time, although they are around for quite some time.
And we have features that are not implemented yet by KVM and will not
be implemented in the near future.
Therefore we will be adding features to our "flexible models"
while keeping migration safe variants with base features stable.

Some day in the future, we will also have all features around :)

Another problematic thing for us is, that we really have to rely
on a host cpu model detection using KVM. The features that
are visible in user space (similar to cpuid()) don't give any
guarantees to what we are able to virtualize.

David

Re: [Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[David Hildenbrand]

> > e.g. in terms of s390x: z13 includes both vx and tx
> > -cpu z13,vx=off,tx=off  
> Above -cpu template will be translated into a corresponding
> global properties template:
> 
> -global z13-s390-cpu.vx=off -global z13-s390-cpu.tx=off

This description makes it much clearer how you expect -cpu to behave. Thanks.
The important part for me was that -global applies to this cpu type only.

And our internal hierarchy should make sure that this is guaranteed. 
And good to known that it is a common schema we are following here.

> > d) object_new("z13-s390-cpu")); // will I get a clean z13 with tx and vx 
> > on?  
> nope, you'll z13 with tx and vx off
> 
> > d) has to work for us. Otherwise we will have to fallback to manual
> > property parsing.  
> could you elaborate more why do you need (d) work
> in combination with -cpu z13,vx=off,tx=off ?
> Perhaps there is another way to do what you need.

Say we started QEMU with -z13,vx=off,tx=off.
When I do a QMP query to e.g. compare two cpu models, say a temporary cpu
zBC12,tx=on and zBC13,tx=on is created. And the additional
parameters only affect these instances as I learned.

When I now compare them, the result will depend on the
way QEMU has been started. But most likely this is okay, as
we expect this interface to be used without any cpu model at all.

This then just has to be documented accordingly.

> 
> Current code uses -cpu for creating homogeneous cpus of given type
> (i.e.) as template and that's exactly what -global cpufoo,feat=x ... does.
> 
> > > If all you need is to parse properties, why can't you reuse the
> > > existing QOM/Device mechanisms to handle properties (the one used
> > > by -device and device_add), instead of the -cpu code?
> > 
> > We can, if my given example works. And the global properties
> > don't interfere with cpus.  
> if you need pure -device/device_add handling then don't use
> -cpu CPUFOO,feat=x,... template as its current semantic
> is to create all CPUs of type CPUFOO with feat=x,... applied
> > It is a bad thing as soon as they affect other devices.
> > If I did a -cpu z13,tx=off, I don't expect
> > 
> > a) a hot-plugged z13 to suddenly have tx=off  
> you should expect it though, as -cpu z13,tx=off is template
> for all z13 CPUs, it's not per instance thingy
> 
> > b) a hot-plugged zBC12 to suddenly have tx off  
> that will not have tx off unless zBC12 is inherited from z13

They don't inherit, so this should be fine.

> 
> > Won't libvirt have to specify the cpu name either way in device-add?
> > And your plan seems to be that the properties are suddenly implicit.
> > I don't see a problem with libvirt having to specify the properties
> > manually on device add.  
> libvirt could do verbose
> #1  -device CPUFOO,feat1=x -device CPUFOO,feat1=x ...
> or
> #2  -cpu CPUFOO,feat1=x -device CPUFOO -device CPUFOO ...
>   which is equivalent of generic device syntax and not use -cpu at all:
> #3  -global CPUFOO.feat1=x -device CPUFOO -device CPUFOO ...
> 
> #3 is where we are heading to by making -device/device_add available for
> CPUs. If you wish to have CPUs of the same type but with different
> features, don't use global properties for that features.
> Just like with any other device.

So this should work for us, we just have to document that -global for cpus
or -cpu should not be used when trying to work with the new QMP 
queries.

Thanks for the explanation!

David

Re: [Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[David Hildenbrand]

> On Thu, Jun 02, 2016 at 10:44:49PM +0200, David Hildenbrand wrote:
> > > Current CLI option -cpu cpux,features serves as template
> > > for all created cpus of type: cpux. However QEMU parses
> > > "features" every time it creates a cpu instance and applies
> > > them to it while doing parsing.
> > > 
> > > That doesn't work well with -device/device_add infrastructure
> > > as it has no idea about cpu specific hooks that's used for
> > > parsing "features".
> > > In order to make -device/device_add utilize "-cpu features"
> > > template convert it into a set of global properties, so that
> > > every new CPU created will have them applied automatically.
> > > 
> > > That also allows to parse features only once, instread of
> > > doing it for every CPU instance created.  
> > 
> > While I think this makes sense for most cases, we (s390x) are
> > currently working on a mechanism to compare and baseline cpu models via
> > a qmp interface, to not have to replicate CPU models in libvirt
> > every time we do some changes.
> > 
> > To do that, we are creating temporary CPUs to handle the model
> > parsing. So, with our current prototype, we rely on the mechanism
> > to parse properties multiple time, as we are really creating
> > different CPUs.  
> 
> This series only changes the code that exists for parsing the
> -cpu option, and nothing else. Is this (the code that parses
> -cpu) really what you need to reuse?

I was reading "every new CPU created will have them applied automatically".
If I was having a basic understanding problem here, very good :)

The problematic part is when the properties are applied where the
"changed" data is stored (class. vs. instance).

e.g. in terms of s390x: z13 includes both vx and tx
-cpu z13,vx=off,tx=off

Now, what would happen on
a) device_add z13-s390-cpu // I assume vx=off, tx=off ?

b) device_add z13-s390-cpu,vx=on // vx=on suddenly for all vcpus or one
instance? I assume just this instance

c) device_add zBC12-s390-cpu // will I suddenly get a z13?
Or a zBC12 without tx and vx? I assume the latter.

d) object_new("z13-s390-cpu")); // will I get a clean z13 with tx and vx on?

d) has to work for us. Otherwise we will have to fallback to manual
property parsing.

> 
> If all you need is to parse properties, why can't you reuse the
> existing QOM/Device mechanisms to handle properties (the one used
> by -device and device_add), instead of the -cpu code?

We can, if my given example works. And the global properties
don't interfere with cpus.

> 
> We need to use less of the infrastructure that exists for the
> legacy -cpu option (and use more of the generic QOM/Device
> mechanisms), not more of it.

It is better to have one way of creating cpus that two.

> 
> 
> > 
> > While we could somehow change our mechanism I don't think this is
> > the right thing to do.
> >   
> 
> If reusing the existing parsing code is something you absolutely
> need, we could split the process in two parts: 1) converting the
> feature string to a list of property=value pairs; 2) registering
> the property=value pairs as global properties. Then you coulde
> reuse (1) only. But do you really need to reuse the parser for
> the legacy -cpu option in your mechanism?

It's really not about the parser, more about the global properties.

> 
> > We will have to support heterogeneous cpu models (I think arm was one of
> > the guys requesting this if I'm not mistaking) and it somehow
> > contradicts to the general mechanism of device_add fully specifying
> > parameters. These would now be implicit parameters.  
> 
> The -cpu interface really does contradict the general mechanism
> of device_add. This whole series is about translating the
> handling of -cpu to a more generic mechanism (-global), to allow
> us to deprecate -cpu in the future. Why is that a bad thing?

It is a bad thing as soon as they affect other devices.
If I did a -cpu z13,tx=off, I don't expect

a) a hot-plugged z13 to suddenly have tx=off
b) a hot-plugged zBC12 to suddenly have tx off

Won't libvirt have to specify the cpu name either way in device-add?
And your plan seems to be that the properties are suddenly implicit.
I don't see a problem with libvirt having to specify the properties
manually on device add.

I agree, cleaning up the parsing function indeed makes sense.

David

Re: [Qemu-devel] [RFC 00/28] s390x CPU models: exposing features

[David Hildenbrand]

> On Tue, Jun 21, 2016 at 13:44:31 -0300, Eduardo Habkost wrote:
> > (CCing libvirt people)
> > 
> > On Tue, Jun 21, 2016 at 03:02:05PM +0200, David Hildenbrand wrote:
> > > This is our second attempt to implement CPU models for s390x. We realized
> > > that we also want to have features exposed via the CPU model. While doing
> > > that we realized that we want to have a better interface for libvirt.
> > 
> > Before getting into the details, I would like to clarify how the
> > following could be accomplished using the new commands:
> > 
> > Example:
> > 
> > 1) User configures libvirt with:
> >
> >Westmere
> >
> >
> > 2) libvirt will translate that to:
> >"-cpu Westmere,+aes" or "-cpu Westmere,aes=on"
> > 3) libvirt wants to know if "-cpu Westmere,aes=on" is usable in
> >the current host, before trying to start the VM.
> 
> While this is what libvirt currently does, I don't think it's necessary
> to keep doing that... see below.
> 
> > > a) "query-cpu-model-expansion" - tell us what the "host" or a migration
> > >unsafe model looks like. Either falling back to a stable model or
> > >completely exposing all properties. We are interested in stable models.
> > > b) "query-cpu-model-comparison" - tell us how two CPU models compare,
> > > indicating which properties were responsible for the decision.
> > > c) "query-cpu-model-baseline" - create a new model out of two models,
> > > taking a requested level of stability into account.
> 
> This looks like it copies current libvirt APIs, which I think is not a
> very good idea. Both CPU compare and baseline APIs in libvirt will need
> to be changed (or rather new APIs with similar functionality added) to
> become useful. I think we should first focus on making guest CPU
> configuration work the way it should work. For that I think we need some
> higher level commands.

Hi Jiri,

thanks for your reply!

Please be aware that we (s390x) have no libvirt cpu model support in place. We
are coming up with a solution that solves our problems, but instead of tagging
all interfaces with "s390x" we want interfaces that everybody might use.

I understand that x86 has some very advanced libvirt CPU model magic in place.
You can still stick to what you have right now. It doesn't work for us.

So we provide 3 basic functions (baseline, compare, expand) that can easily by
added for s390x into the CPU model. So I don't see a problem introducing
these operations in libvirt - it just works without any magic and knowledge
about features and models in libvirt.

In the end it keeps the CPU model where it belongs (QEMU) and allows us to
just fit in fine into the libvirt CPU model world that was heavily inspired
by x86 (an I can tell you, that is a very challenging task).

But - of course - we have to do some additional queries on a QEMU instance. I
don't think that this is a problem (at least not for s390x).

> 
> Let me sum up what libvirt is doing (or will be doing) with guest
> CPUs... Libvirt supports three guest CPU configuration modes:

One important thing to mention here is that all of these was inspired by x86.
Some stuff doesn't even make sense on s390x ("mimic the real host"), as
it would even in practice never be possible. See my reply to Eduards patchset
I'll post later.

We want to focus on the real use cases and I think they are
- Making sure models ("Guest API") doesn't change during migration
- Allowing the user to use a certain CPU model
- Baselining/Comparing/Using the -cpu host model


> 
> - host-passthrough -- this is easy, it's just asking for "-cpu host" and
>   no fancy commands are required.

This should map to expanding the host model in our case.

> 
> - host-model -- for this we need to know what CPU configuration we need
>   to ask for to get as close to the host CPU as possible while being
>   able to ask for the exact same CPU on a different host (after
>   migration). And we need to be able to ask for this at probing time
>   (when libvirtd starts rather than when starting a new domain) using
>   "-machine none,accel=kvm:tcg", that is without actually creating any
>   guest CPU. This is what Eduardo's query-host-cpu QMP command is useful
>   for. In x86 world we could just query the guest CPU after running QEMU
>   with "-cpu host", but that's unusable with "-machine none", which is
>   why another way of getting this info is needed.

Again, we will be relying on "-cpu host" here, because the real host model
will never be possible to be fordwarded on s390x. And what's the 

Re: [Qemu-devel] [RFC 00/28] s390x CPU models: exposing features

[David Hildenbrand]

> On Tue, Jun 21, 2016 at 17:33:09 -0300, Eduardo Habkost wrote:
> > On Tue, Jun 21, 2016 at 07:01:44PM +0200, David Hildenbrand wrote:  
> > > > (CCing libvirt people)
> > > > 
> > > > On Tue, Jun 21, 2016 at 03:02:05PM +0200, David Hildenbrand wrote:  
> > > > > This is our second attempt to implement CPU models for s390x. We 
> > > > > realized
> > > > > that we also want to have features exposed via the CPU model. While 
> > > > > doing
> > > > > that we realized that we want to have a better interface for libvirt. 
> > > > >
> > > > 
> > > > Before getting into the details, I would like to clarify how the
> > > > following could be accomplished using the new commands:
> > > > 
> > > > Example:
> > > > 
> > > > 1) User configures libvirt with:
> > > >
> > > >Westmere
> > > >
> > > >
> > > > 2) libvirt will translate that to:
> > > >"-cpu Westmere,+aes" or "-cpu Westmere,aes=on"
> > > > 3) libvirt wants to know if "-cpu Westmere,aes=on" is usable in
> > > >the current host, before trying to start the VM.
> > > > 
> > > > How exactly would this be done using the new commands?  
> > > 
> > > Hi Eduardo,
> > > 
> > > thanks for having a look - highly appreciated that you actually map this
> > > to libvirt requirements!
> > > 
> > > That would map to a compare operation between "host" and 
> > > "Westmere,aes=on".
> > > 
> > > Host could at that point already be expanded by libvirt. Doesn't matter 
> > > at that
> > > point.
> > > 
> > > If the result is "identica"l or "superset", it is runnable. If the result 
> > > is
> > > "subset" or "incompatible", details about the responsible properties is
> > > indicated. (I actually took that idea from your patch for indicating
> > > runnability).  
> > 
> > So, I have two worries about the proposal:
> > 
> > 
> > 1) "query-cpu-model-expansion model=host" vs "query-host-cpu":
> > 
> > I still don't think we want to set in stone that "the result the
> > guest sees when using -cpu host" is always the same as "what the
> > host supports running".
> > 
> > For example: let's assume a given architecture have two features
> > (A and B) that are both supported by the host but can never be
> > enabled together. For actual "-cpu host" usage, QEMU would have
> > to choose between enabling A and B. For querying host
> > capabilities, we still want to let management software know that
> > either A or B are supported.  
> 
> What libvirt is really interested in is the guest CPU which would be
> used with -cpu host. This is actually what I thought query-host-cpu was
> all about. Perhaps because there's no difference for x86.

That's also what I had in mind first. Let me explain why they are not the same
on s390x and why it is problematic (actually both types are not the same!):

1. Certain CPU features are only valid for LPAR guests, they can't be
virtualized for KVM guests (remember that we always have at least one level of
virtualization on s390x). So we will never be able to provide these features to
a KVM guest, therefore we will never be able to completely mimic the real host
model.

2. We have a whole lot of unpublished features. We would have to include them in
the "real host model", but we can't. For the "host" model, this is not a
problem, because we simply don't virtualize them. But the "real host model"
would then vary between say QEMZ versions, which looks really strage, because
in essance, QEMU/KVM looks like the wrong interface to query for a "real host
model".

3. We don't have the kernel interfaces in place to query the "real host model".
We can only query what we are able to virtualize. And that is indeed different
compared to what I said previously.

And as I can't see any use case for s390x, we most probably will never be able
to support the interface you are suggesting here. Which is fine, if it makes
sense for x86.

> 
> > 2) Requiring a running QEMU instance to run
> >query-cpu-model-comparison
> > 
> > With my previous query-host-cpu proposal, the task of comparing
> > the configuration requested by the user with host capabilities
> > can be done directly by libvirt. This way, no extra QEMU instance
> > needs to be run before starting a VM.  
> 
> I think we can just easily get around this by not comparing a guest CPU
> to host (except for the explicit virConnectCompareCPU, which is not very
> useful in its current form anyway).

If there is some flexible way around that, great. But I think we (s390x) could
life without this additional query.


So your first concern is valid, and if you really need this information, you
should probably really create a new interface. Your second concern should not
be trivial for now. We don't want to optimize for total speed at that point.

David

Re: [Qemu-devel] [RFC 06/28] s390x/cpumodel: introduce CPU feature group definitions

[David Hildenbrand]

> On 21.06.2016 15:02, David Hildenbrand wrote:
> > Let's use the generated groups to create feature group representations for
> > the user. These groups can later be used to enable/disable multiple
> > features in one shot and will be used to reduce the amount of reported
> > features to the user if all subfeatures are in place.
> > 
> > Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
> > Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
> > ---
> >  target-s390x/cpu_features.c | 44 
> > +++-
> >  target-s390x/cpu_features.h | 23 +++
> >  2 files changed, 66 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target-s390x/cpu_features.c b/target-s390x/cpu_features.c
> > index c78a189..6ec2bfc 100644
> > --- a/target-s390x/cpu_features.c
> > +++ b/target-s390x/cpu_features.c
> > @@ -12,6 +12,7 @@
> >  
> >  #include "qemu/osdep.h"
> >  #include "cpu_features.h"
> > +#include "gen-features.h"
> >  
> >  #define FEAT_INIT(_name, _type, _bit, _desc) \
> >  {\
> > @@ -321,14 +322,55 @@ void s390_add_from_feat_block(S390FeatBitmap 
> > features, S390FeatType type,
> >  }
> >  }
> >  
> > -void s390_feat_bitmap_to_ascii(const S390FeatBitmap bitmap, void *opaque,
> > +void s390_feat_bitmap_to_ascii(const S390FeatBitmap features, void *opaque,
> > void (*fn)(const char *name, void *opaque))
> >  {
> > +S390FeatBitmap bitmap, tmp;
> > +S390FeatGroup group;
> >  S390Feat feat;
> >  
> > +bitmap_copy(bitmap, features, S390_FEAT_MAX);
> > +
> > +/* process whole groups first */
> > +for (group = 0; group < S390_FEAT_GROUP_MAX; group++) {
> > +const S390FeatGroupDef *def = s390_feat_group_def(group);
> > +
> > +bitmap_and(tmp, bitmap, def->feat, S390_FEAT_MAX);
> > +if (bitmap_equal(tmp, def->feat, S390_FEAT_MAX)) {
> > +bitmap_andnot(bitmap, bitmap, def->feat, S390_FEAT_MAX);
> > +(*fn)(def->name, opaque);  
> 
> Maybe simply write
>fn(dev->name, opaque);
> instead?

Will that work? As fn is a pointer I am not a 100% sure. If so, I'll change it!

Thanks!

David

Re: [Qemu-devel] [RFC 00/28] s390x CPU models: exposing features

[David Hildenbrand]

> On Wed, Jun 22, 2016 at 09:34:49 +0200, David Hildenbrand wrote:
> > I think the coffee didn't do its work already :) . I wanted to write that 
> > we can
> > _with_ this additional query. Meaning the involved overhead would be ok - 
> > in my
> > opinion for s390x.
> > 
> > What we could do to avoid one compare operation would be:
> > 
> > a) Expand the host model
> > b) Expand the target model (because on s390x we could have migration unsafe
> > model)
> > c) Work with the runnability information returned via query-cpu-definitions
> > 
> > But as we have to do b) either way on s390x, we can directly do a compare
> > operation. (which makes implementation a lot simpler, because libvirt then
> > doesn't have to deal with any feature/model names).  
> 
> But why do you even need to do any comparison? Isn't it possible to let
> QEMU do it when a domain starts? The thing is we should avoid doing
> completely different things on each architecture.
> 

Sure, QEMU will of course double check when starting the guest! So trying to
start and failing is of course an option! So no check is needed if that is
acceptable.

David

Re: [Qemu-devel] [RFC 00/28] s390x CPU models: exposing features

[David Hildenbrand]

> > > > 2) Requiring a running QEMU instance to run
> > > >query-cpu-model-comparison
> > > > 
> > > > With my previous query-host-cpu proposal, the task of comparing
> > > > the configuration requested by the user with host capabilities
> > > > can be done directly by libvirt. This way, no extra QEMU instance
> > > > needs to be run before starting a VM.
> > > 
> > > I think we can just easily get around this by not comparing a guest CPU
> > > to host (except for the explicit virConnectCompareCPU, which is not very
> > > useful in its current form anyway).  
> > 
> > If there is some flexible way around that, great. But I think we (s390x) 
> > could
> > life without this additional query.  
> 
> So if I understand correctly, you say you don't need the API to compare
> guest CPU to host CPU, right? If so, that's exactly what I said too.
> 

I think the coffee didn't do its work already :) . I wanted to write that we can
_with_ this additional query. Meaning the involved overhead would be ok - in my
opinion for s390x.

What we could do to avoid one compare operation would be:

a) Expand the host model
b) Expand the target model (because on s390x we could have migration unsafe
model)
c) Work with the runnability information returned via query-cpu-definitions

But as we have to do b) either way on s390x, we can directly do a compare
operation. (which makes implementation a lot simpler, because libvirt then
doesn't have to deal with any feature/model names).

David

Re: [Qemu-devel] [libvirt] [RFC 00/28] s390x CPU models: exposing features

[David Hildenbrand]

> On Tue, Jun 21, 2016 at 18:22:30 -0300, Eduardo Habkost wrote:
> > On Tue, Jun 21, 2016 at 11:09:49PM +0200, Jiri Denemark wrote:
> > [...]  
> > > > 1) "query-cpu-model-expansion model=host" vs "query-host-cpu":
> > > > 
> > > > I still don't think we want to set in stone that "the result the
> > > > guest sees when using -cpu host" is always the same as "what the
> > > > host supports running".
> > > > 
> > > > For example: let's assume a given architecture have two features
> > > > (A and B) that are both supported by the host but can never be
> > > > enabled together. For actual "-cpu host" usage, QEMU would have
> > > > to choose between enabling A and B. For querying host
> > > > capabilities, we still want to let management software know that
> > > > either A or B are supported.  
> > > 
> > > What libvirt is really interested in is the guest CPU which would be
> > > used with -cpu host. This is actually what I thought query-host-cpu was
> > > all about. Perhaps because there's no difference for x86.  
> > 
> > In that case, I think it makes sense to just extend
> > query-cpu-definitions or use "query-cpu-model-expansion
> > model=host" instead of a query-host-cpu command.
> > 
> > Probably query-cpu-model-expansion is better than just extending
> > query-cpu-definitions, because it would allow the expansion of
> > extra CPU options, like "host,migratable=off".  
> 
> Yeah, this would be even better.
> 
> Jirka
> 

Please be aware that we don't have anything like that on s390x, but
I prepared for that requirement by being able to tell
query-cpu-model-expansion how to expand (full, migratable, stable).
Actually full and migratable looks the same on s390x.

The plan for us is to rely on "stable" most of the time, full and migratable
might be what you're looking for.

David

[Qemu-devel] [RFC 10/28] s390x/cpumodel: let the CPU model handle feature checks

[David Hildenbrand]

If we have certain features enabled, we have to migrate additional state
(e.g. vector registers or runtime-instrumentation registers). Let the
CPU model control that unless we have no "host" CPU model in the KVM
case. This will later on be the case for compatibility machines, so
migration from QEMU versions without the CPU model will still work.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 target-s390x/cpu_models.c | 24 
 target-s390x/cpu_models.h |  2 ++
 target-s390x/machine.c| 14 ++
 3 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index 93e63cb..7e3f544 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -73,6 +73,30 @@ static const S390CPUDef s390_cpu_defs[] = {
 CPUDEF_INIT(0x2965, 13, 2, 47, 0x0800U, "z13s", "IBM z13s GA1"),
 };
 
+bool s390_has_feat(S390Feat feat)
+{
+static S390CPU *cpu;
+
+if (!cpu) {
+cpu = S390_CPU(qemu_get_cpu(0));
+}
+
+if (!cpu || !cpu->model) {
+#ifdef CONFIG_KVM
+if (kvm_enabled()) {
+if (feat == S390_FEAT_VECTOR) {
+return kvm_check_extension(kvm_state, 
KVM_CAP_S390_VECTOR_REGISTERS);
+}
+if (feat == S390_FEAT_RUNTIME_INSTRUMENTATION) {
+return kvm_s390_get_ri();
+}
+}
+#endif
+return 0;
+}
+return test_bit(feat, cpu->model->features);
+}
+
 struct S390PrintCpuListInfo {
 FILE *f;
 fprintf_function print;
diff --git a/target-s390x/cpu_models.h b/target-s390x/cpu_models.h
index 244256b..fe988cc 100644
--- a/target-s390x/cpu_models.h
+++ b/target-s390x/cpu_models.h
@@ -43,4 +43,6 @@ typedef struct S390CPUModel {
 uint8_t cpu_ver;/* CPU version, usually "ff" for kvm */
 } S390CPUModel;
 
+bool s390_has_feat(S390Feat feat);
+
 #endif /* TARGET_S390X_CPU_MODELS_H */
diff --git a/target-s390x/machine.c b/target-s390x/machine.c
index aa39e5d..edc3a47 100644
--- a/target-s390x/machine.c
+++ b/target-s390x/machine.c
@@ -78,12 +78,7 @@ static const VMStateDescription vmstate_fpu = {
 
 static bool vregs_needed(void *opaque)
 {
-#ifdef CONFIG_KVM
-if (kvm_enabled()) {
-return kvm_check_extension(kvm_state, KVM_CAP_S390_VECTOR_REGISTERS);
-}
-#endif
-return 0;
+return s390_has_feat(S390_FEAT_VECTOR);
 }
 
 static const VMStateDescription vmstate_vregs = {
@@ -147,12 +142,7 @@ static const VMStateDescription vmstate_vregs = {
 
 static bool riccb_needed(void *opaque)
 {
-#ifdef CONFIG_KVM
-if (kvm_enabled()) {
-return kvm_s390_get_ri();
-}
-#endif
-return 0;
+return s390_has_feat(S390_FEAT_RUNTIME_INSTRUMENTATION);
 }
 
 const VMStateDescription vmstate_riccb = {
-- 
2.6.6

[Qemu-devel] [RFC 04/28] s390x/cpumodel: generate CPU feature lists for CPU models

[David Hildenbrand]

From: Michael Mueller <m...@linux.vnet.ibm.com>

This patch introduces the helper "gen-features" which allows to generate
feature list definitions at compile time. Its flexibility is better and the
error-proneness is lower when compared to static programming time added
statements.

The helper includes "target-s390x/cpu_features.h" to be able to use named
facility bits instead of numbers. The generated defines will be used for
the definition of CPU models.

We generate feature lists for each HW generation and GA for EC models. BC
models are always based on a EC version and have no separate definitions.

Base features: Features we expect to be always available in sane setups.
Migration safe - will never change. Can be seen as "minimum features
required for a CPU model".

Default features: Features we expect to be stable and around in latest
setups (e.g. having KVM support) - not migration safe.

Max features: All supported features that are theoretically allowed for a
CPU model. Exceeding these features could otherwise produce problems with
IBC (instruction blocking controls) in KVM.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: Michael Mueller <m...@linux.vnet.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
[generate base, default and models. renaming and cleanup]
---
 Makefile.target |   2 +-
 rules.mak   |   1 +
 target-s390x/Makefile.objs  |  20 ++
 target-s390x/gen-features.c | 506 
 4 files changed, 528 insertions(+), 1 deletion(-)
 create mode 100644 target-s390x/gen-features.c

diff --git a/Makefile.target b/Makefile.target
index 495b474..2ed5add 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -215,7 +215,7 @@ hmp-commands-info.h: $(SRC_PATH)/hmp-commands-info.hx 
$(SRC_PATH)/scripts/hxtool
 qmp-commands-old.h: $(SRC_PATH)/qmp-commands.hx $(SRC_PATH)/scripts/hxtool
$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > $@,"  GEN  
 $(TARGET_DIR)$@")
 
-clean:
+clean: clean-target
rm -f *.a *~ $(PROGS)
rm -f $(shell find . -name '*.[od]')
rm -f hmp-commands.h qmp-commands-old.h gdbstub-xml.c
diff --git a/rules.mak b/rules.mak
index 72c5955..ce0a2a5 100644
--- a/rules.mak
+++ b/rules.mak
@@ -14,6 +14,7 @@ MAKEFLAGS += -rR
 %.cpp:
 %.m:
 %.mak:
+clean-target:
 
 # Flags for C++ compilation
 QEMU_CXXFLAGS = -D__STDC_LIMIT_MACROS $(filter-out -Wstrict-prototypes 
-Wmissing-prototypes -Wnested-externs -Wold-style-declaration 
-Wold-style-definition -Wredundant-decls, $(QEMU_CFLAGS))
diff --git a/target-s390x/Makefile.objs b/target-s390x/Makefile.objs
index 4266c87..745d501 100644
--- a/target-s390x/Makefile.objs
+++ b/target-s390x/Makefile.objs
@@ -3,3 +3,23 @@ obj-y += int_helper.o fpu_helper.o cc_helper.o mem_helper.o 
misc_helper.o
 obj-y += gdbstub.o cpu_models.o cpu_features.o
 obj-$(CONFIG_SOFTMMU) += machine.o ioinst.o arch_dump.o mmu_helper.o
 obj-$(CONFIG_KVM) += kvm.o
+
+# build and run feature list generator
+feat-src = $(SRC_PATH)/target-$(TARGET_BASE_ARCH)/
+feat-dst = $(BUILD_DIR)/$(TARGET_DIR)
+ifneq ($(MAKECMDGOALS),clean)
+GENERATED_HEADERS += $(feat-dst)gen-features.h
+endif
+
+$(feat-dst)gen-features.h: $(feat-dst)gen-features.h-timestamp
+   @cmp $< $@ >/dev/null 2>&1 || cp $< $@
+$(feat-dst)gen-features.h-timestamp: $(feat-dst)gen-features
+   $(call quiet-command,$< >$@,"  GEN   $(TARGET_DIR)gen-features.h")
+
+$(feat-dst)gen-features: $(feat-src)gen-features.c config-target.h
+   $(call quiet-command,$(CC) $(QEMU_INCLUDES) $(QEMU_CFLAGS) $(CFLAGS) -o 
$@ $<,"  CC$(TARGET_DIR)gen-features")
+
+clean-target:
+   rm -f gen-features.h-timestamp
+   rm -f gen-features.h
+   rm -f gen-features
diff --git a/target-s390x/gen-features.c b/target-s390x/gen-features.c
new file mode 100644
index 000..3a7c373
--- /dev/null
+++ b/target-s390x/gen-features.c
@@ -0,0 +1,506 @@
+/*
+ * S390 feature list generator
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * Author(s): Michael Mueller <m...@linux.vnet.ibm.com>
+ *David Hildenbrand <d...@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "cpu_features.h"
+
+/* BEGIN FEATURE DEFS */
+
+#define S390_FEAT_GROUP_PLO \
+S390_FEAT_PLO_CL, \
+S390_FEAT_PLO_CLG, \
+S390_FEAT_PLO_CLGR, \
+S390_FEAT_PLO_CLX, \
+S390_FEAT_PLO_CS, \
+S390_FEAT_PLO_CSG, \
+S390_FEAT_PLO_CSGR, \
+S390_FEAT_PLO_CSX, \
+S390_FEAT_PLO_DCS, \
+S390_FEAT_PLO_DCSG, \
+S390_FEAT_PLO_DCSGR, \
+S390_FEAT_PLO_DCSX, \
+S390_FEAT_PLO_CSST, \
+S390_FEAT_PLO_CSSTG, \
+S390_FEAT_PLO_

[Qemu-devel] [RFC 28/28] s390x/cpumodel: implement QMP interface "query-cpu-model-baseline"

[David Hildenbrand]

Let's implement that interface by reusing our conversion code and
lookup code for cpu definitions.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 target-s390x/cpu_models.c | 61 +++
 1 file changed, 61 insertions(+)

diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index 75d808b..56b945d 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -503,6 +503,67 @@ CpuModelCompareInfo 
*arch_query_cpu_model_comparison(CpuModelInfo *infoa,
 }
 return compare_info;
 }
+
+CpuModelBaselineInfo *arch_query_cpu_model_baseline(CpuModelBaselineType type,
+CpuModelInfo *infoa,
+CpuModelInfo *infob,
+Error **errp)
+{
+CpuModelBaselineInfo *baseline_info;
+S390CPUModel modela, modelb, model;
+uint16_t cpu_type;
+uint8_t max_gen_ga;
+uint8_t max_gen;
+
+/* convert both models to our internal representation */
+cpu_model_from_info(, infoa, errp);
+if (*errp) {
+return NULL;
+}
+
+cpu_model_from_info(, infob, errp);
+if (*errp) {
+return NULL;
+}
+
+/* features both models support */
+bitmap_and(model.features, modela.features, modelb.features, 
S390_FEAT_MAX);
+
+/* detect the maximum model not regarding features */
+if (modela.def->gen == modelb.def->gen) {
+if (modela.def->type == modelb.def->type) {
+cpu_type = modela.def->type;
+} else {
+cpu_type = 0;
+}
+max_gen = modela.def->gen;
+max_gen_ga = MIN(modela.def->ec_ga, modelb.def->ec_ga);
+} else if (modela.def->gen > modelb.def->gen) {
+cpu_type = modelb.def->type;
+max_gen = modelb.def->gen;
+max_gen_ga = modelb.def->ec_ga;
+} else {
+cpu_type = modela.def->type;
+max_gen = modela.def->gen;
+max_gen_ga = modela.def->ec_ga;
+}
+
+model.def = s390_find_cpu_def(cpu_type, max_gen, max_gen_ga,
+  model.features);
+/* make sure we don't exceed full features of that vcpu */
+if (type == CPU_MODEL_BASELINE_TYPE_STABLE) {
+bitmap_and(model.features, model.features, model.def->default_feat,
+   S390_FEAT_MAX);
+} else {
+bitmap_and(model.features, model.features, model.def->full_feat,
+   S390_FEAT_MAX);
+}
+
+baseline_info = g_malloc0(sizeof(*baseline_info));
+baseline_info->model = g_malloc0(sizeof(*baseline_info->model));
+cpu_info_from_model(baseline_info->model, , true);
+return baseline_info;
+}
 #endif
 
 static void check_consistency(const S390CPUModel *model)
-- 
2.6.6

[Qemu-devel] [RFC 14/28] s390x/sclp: indicate sclp features

[David Hildenbrand]

We have three different blocks in the SCLP read-SCP information response
that indicate sclp features. Let's prepare propagation.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 hw/s390x/sclp.c   |  9 +
 target-s390x/cpu_models.c | 14 ++
 target-s390x/cpu_models.h |  1 +
 3 files changed, 24 insertions(+)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 15d7114..3c126ee 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -31,11 +31,14 @@ static inline SCLPDevice *get_sclp_device(void)
 
 static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int count)
 {
+uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
 int i;
 
+s390_get_feat_block(S390_FEAT_TYPE_SCLP_CPU, features);
 for (i = 0; i < count; i++) {
 entry[i].address = i;
 entry[i].type = 0;
+memcpy(entry[i].features, features, sizeof(entry[i].features));
 }
 }
 
@@ -59,6 +62,12 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
 read_info->highest_cpu = cpu_to_be16(max_cpus);
 
+/* Configuration Characteristic (Extension) */
+s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR,
+ read_info->conf_char);
+s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT,
+ read_info->conf_char_ext);
+
 prepare_cpu_entries(sclp, read_info->entries, cpu_count);
 
 read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index e7bcea9..571ceaa 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -74,6 +74,20 @@ static const S390CPUDef s390_cpu_defs[] = {
 CPUDEF_INIT(0x2965, 13, 2, 47, 0x0800U, "z13s", "IBM z13s GA1"),
 };
 
+void s390_get_feat_block(S390FeatType type, uint8_t *data)
+{
+static S390CPU *cpu;
+
+if (!cpu) {
+cpu = S390_CPU(qemu_get_cpu(0));
+}
+
+if (!cpu || !cpu->model) {
+return;
+}
+return s390_fill_feat_block(cpu->model->features, type, data);
+}
+
 bool s390_has_feat(S390Feat feat)
 {
 static S390CPU *cpu;
diff --git a/target-s390x/cpu_models.h b/target-s390x/cpu_models.h
index fe988cc..04c47c6 100644
--- a/target-s390x/cpu_models.h
+++ b/target-s390x/cpu_models.h
@@ -43,6 +43,7 @@ typedef struct S390CPUModel {
 uint8_t cpu_ver;/* CPU version, usually "ff" for kvm */
 } S390CPUModel;
 
+void s390_get_feat_block(S390FeatType type, uint8_t *data);
 bool s390_has_feat(S390Feat feat);
 
 #endif /* TARGET_S390X_CPU_MODELS_H */
-- 
2.6.6

[Qemu-devel] [RFC 18/28] update linux headers (CPU model)

[David Hildenbrand]

Update linux headers to include the new cpu model attributes

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 linux-headers/asm-s390/kvm.h | 40 
 1 file changed, 40 insertions(+)

diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
index 09ae5dc..dd4f22e 100644
--- a/linux-headers/asm-s390/kvm.h
+++ b/linux-headers/asm-s390/kvm.h
@@ -93,6 +93,46 @@ struct kvm_s390_vm_cpu_machine {
__u64 fac_list[256];
 };
 
+#define KVM_S390_VM_CPU_PROCESSOR_FEAT 2
+#define KVM_S390_VM_CPU_MACHINE_FEAT   3
+
+#define KVM_S390_VM_CPU_FEAT_NR_BITS   1024
+#define KVM_S390_VM_CPU_FEAT_ESOP  0
+#define KVM_S390_VM_CPU_FEAT_SIEF2 1
+#define KVM_S390_VM_CPU_FEAT_64BSCAO   2
+#define KVM_S390_VM_CPU_FEAT_SIIF  3
+#define KVM_S390_VM_CPU_FEAT_GPERE 4
+#define KVM_S390_VM_CPU_FEAT_GSLS  5
+#define KVM_S390_VM_CPU_FEAT_IB6
+#define KVM_S390_VM_CPU_FEAT_CEI   7
+#define KVM_S390_VM_CPU_FEAT_IBS   8
+#define KVM_S390_VM_CPU_FEAT_SKEY  9
+#define KVM_S390_VM_CPU_FEAT_CMMA  10
+#define KVM_S390_VM_CPU_FEAT_PFMFI 11
+#define KVM_S390_VM_CPU_FEAT_SIGPIF12
+struct kvm_s390_vm_cpu_feat {
+   __u64 feat[16];
+};
+
+#define KVM_S390_VM_CPU_PROCESSOR_SUBFUNC  4
+#define KVM_S390_VM_CPU_MACHINE_SUBFUNC5
+struct kvm_s390_vm_cpu_subfunc {
+   __u8 plo[32];   /* always */
+   __u8 ptff[16];  /* with TOD-clock steering */
+   __u8 kmac[16];  /* with MSA */
+   __u8 kmc[16];   /* with MSA */
+   __u8 km[16];/* with MSA */
+   __u8 kimd[16];  /* with MSA */
+   __u8 klmd[16];  /* with MSA */
+   __u8 pckmo[16]; /* with MSA3 */
+   __u8 kmctr[16]; /* with MSA4 */
+   __u8 kmf[16];   /* with MSA4 */
+   __u8 kmo[16];   /* with MSA4 */
+   __u8 pcc[16];   /* with MSA4 */
+   __u8 ppno[16];  /* with MSA5 */
+   __u8 reserved[1824];
+};
+
 /* kvm attributes for crypto */
 #define KVM_S390_VM_CRYPTO_ENABLE_AES_KW   0
 #define KVM_S390_VM_CRYPTO_ENABLE_DEA_KW   1
-- 
2.6.6

[Qemu-devel] [RFC 21/28] s390x/kvm: disable host model for existing compat machines

[David Hildenbrand]

Compatibility machines that touch runtime-instrumentation should not
be used with the CPU model. Otherwise the host model will look different,
depending on the QEMU machine QEMU has been started with.

So let's simply disable the host model for existing compatibility machines
that all disable ri.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 target-s390x/kvm.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 6002cf9..a4f5762 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -2440,6 +2440,10 @@ static int configure_cpu_feat(const S390FeatBitmap 
features)
 
 bool kvm_s390_cpu_models_supported(void)
 {
+if (!ri_allowed()) {
+/* compatibility machines interfere with the cpu model */
+return false;
+}
 return kvm_vm_check_attr(kvm_state, KVM_S390_VM_CPU_MODEL,
  KVM_S390_VM_CPU_MACHINE) &&
kvm_vm_check_attr(kvm_state, KVM_S390_VM_CPU_MODEL,
-- 
2.6.6

[Qemu-devel] [RFC 20/28] s390x/kvm: implement CPU model support

[David Hildenbrand]

Let's implement our two hooks so we can support CPU models.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 target-s390x/cpu_models.c |  75 +++-
 target-s390x/cpu_models.h |  50 
 target-s390x/kvm.c| 295 ++
 3 files changed, 417 insertions(+), 3 deletions(-)

diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index 32d9a09..b8ae601 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -161,6 +161,61 @@ bool s390_has_feat(S390Feat feat)
 return test_bit(feat, cpu->model->features);
 }
 
+uint8_t s390_get_gen_for_cpu_type(uint16_t type)
+{
+int i;
+
+for (i = 0; i < ARRAY_SIZE(s390_cpu_defs); i++) {
+if (s390_cpu_defs[i].type == type) {
+return s390_cpu_defs[i].gen;
+}
+}
+return 0;
+}
+
+const S390CPUDef *s390_find_cpu_def(uint16_t type, uint8_t gen, uint8_t ec_ga,
+S390FeatBitmap features)
+{
+const S390CPUDef *last_compatible = NULL;
+int i;
+
+if (!gen) {
+ec_ga = 0;
+}
+if (!gen && type) {
+gen = s390_get_gen_for_cpu_type(type);
+}
+
+for (i = 0; i < ARRAY_SIZE(s390_cpu_defs); i++) {
+const S390CPUDef *def = _cpu_defs[i];
+S390FeatBitmap missing;
+
+/* don't even try newer generations if we know the generation */
+if (gen) {
+if (def->gen > gen) {
+break;
+} else if (def->gen == gen && ec_ga && def->ec_ga > ec_ga) {
+break;
+}
+}
+
+if (features) {
+/* see if the model satisfies the minimum features */
+bitmap_andnot(missing, def->base_feat, features, S390_FEAT_MAX);
+if (!bitmap_empty(missing, S390_FEAT_MAX)) {
+break;
+}
+}
+
+/* stop the search if we found the exact model */
+if (def->type == type && def->ec_ga == ec_ga) {
+return def;
+}
+last_compatible = def;
+}
+return last_compatible;
+}
+
 struct S390PrintCpuListInfo {
 FILE *f;
 fprintf_function print;
@@ -312,7 +367,7 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
 }
 
 if (kvm_enabled()) {
-error_setg(errp, "KVM does not support CPU models.");
+kvm_s390_get_host_cpu_model(_model, errp);
 } else {
 /* TCG enulates a z900 */
 max_model.def = _cpu_defs[0];
@@ -345,8 +400,7 @@ static inline void apply_cpu_model(const S390CPUModel 
*model, Error **errp)
 }
 
 if (kvm_enabled()) {
-/* FIXME KVM */
-error_setg(errp, "KVM doesn't support CPU models.");
+kvm_s390_apply_cpu_model(model, errp);
 } else if (model) {
 /* FIXME TCG - use data for stdip/stfl */
 }
@@ -551,6 +605,21 @@ static void s390_cpu_model_initfn(Object *obj)
 #ifdef CONFIG_KVM
 static void s390_host_cpu_model_initfn(Object *obj)
 {
+S390CPU *cpu = S390_CPU(obj);
+Error *err = NULL;
+
+if (!kvm_enabled() || !kvm_s390_cpu_models_supported()) {
+return;
+}
+
+cpu->model = g_malloc0(sizeof(*cpu->model));
+kvm_s390_get_host_cpu_model(cpu->model, );
+if (err) {
+error_report_err(err);
+g_free(cpu->model);
+/* fallback to unsupported cpu models */
+cpu->model = NULL;
+}
 }
 #endif
 
diff --git a/target-s390x/cpu_models.h b/target-s390x/cpu_models.h
index 986f7cb..a1ee3d6 100644
--- a/target-s390x/cpu_models.h
+++ b/target-s390x/cpu_models.h
@@ -43,7 +43,25 @@ typedef struct S390CPUModel {
 uint8_t cpu_ver;/* CPU version, usually "ff" for kvm */
 } S390CPUModel;
 
+/*
+ * CPU ID
+ *
+ * bits 0-7: Zeroes (ff for kvm)
+ * bits 8-31: CPU ID (serial number)
+ * bits 32-48: Machine type
+ * bits 48-63: Zeroes
+ */
+#define cpuid_type(x) (((x) >> 16) & 0x)
+#define cpuid_id(x)   (((x) >> 32) & 0xff)
+#define cpuid_ver(x)  (((x) >> 56) & 0xff)
+
+#define lowest_ibc(x) (((uint32_t)(x) >> 16) & 0xfff)
+#define unblocked_ibc(x)  ((uint32_t)(x) & 0xfff)
+#define has_ibc(x)(lowest_ibc(x) != 0)
+
 #define S390_GEN_Z10 0xa
+#define ibc_gen(x)(x == 0 ? 0 : ((x >> 4) + S390_GEN_Z10))
+#define ibc_ec_ga(x)  (x & 0xf)
 
 uint32_t s390_get_hmfai(void);
 uint8_t s390_get_mha_pow(void);
@@ -59,5 +77,37 @@ static inline uint16_t s390_ibc_from_cpu_model(const 
S390CPUModel *model)
 }
 void s390_get_feat_block(S390FeatType type, uint8_t *data);
 bool s390_has_feat(S390Feat feat);
+uint8_t s390_get_gen_for_cpu_type(uint16_t type);
+static inline bool s390_known_cpu_type(uint16_t type)
+{
+return s390_get_gen_for_cpu_type(type) != 0;
+}
+static inline uint64_t s390_cpui

[Qemu-devel] [RFC 25/28] qmp: add QMP interface "query-cpu-model-baseline"

[David Hildenbrand]

Let's provide a standardized interface to baseline two CPU models, to
create a third, compatible one.

For now, we allow two baseline modes. "stable" tries to create a
better tested model, e.g. by minimizing CPU definition changes.
"maximum" rather tries to get the maxmimum possible model,
e.g. by maximizing features.

The host CPU model has the same semantics as for "query-cpu-model-expansion".

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 include/sysemu/arch_init.h|  4 
 qapi-schema.json  | 44 +++
 qmp-commands.hx   |  6 +
 qmp.c |  8 +++
 stubs/Makefile.objs   |  1 +
 stubs/arch-query-cpu-model-baseline.c | 13 +++
 6 files changed, 76 insertions(+)
 create mode 100644 stubs/arch-query-cpu-model-baseline.c

diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
index 96d47c0..1b35e90 100644
--- a/include/sysemu/arch_init.h
+++ b/include/sysemu/arch_init.h
@@ -41,5 +41,9 @@ CpuModelExpansionInfo 
*arch_query_cpu_model_expansion(CpuModelExpansionType type
 CpuModelCompareInfo *arch_query_cpu_model_comparison(CpuModelInfo *modela,
  CpuModelInfo *modelb,
  Error **errp);
+CpuModelBaselineInfo *arch_query_cpu_model_baseline(CpuModelBaselineType type,
+CpuModelInfo *modela,
+CpuModelInfo *modelb,
+Error **errp);
 
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 34df86f..3a3fccb 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3166,6 +3166,50 @@
   'data': { 'modela': 'CpuModelInfo', 'modelb': 'CpuModelInfo' },
   'returns': 'CpuModelCompareInfo' }
 
+##
+# @CpuModelBaselineType
+#
+# An enumeration of CPU model baseline types.
+#
+# @stable: Find a compatible, stable model (e.g. least feature changes).
+#
+# @maximum: Find a compatible, maximum model (e.g. maximizing features)
+#
+# Since: 2.7.0
+##
+{ 'enum': 'CpuModelBaselineType',
+  'data': [ 'stable', 'maximum' ] }
+
+##
+# @CpuModelBaselineInfo
+#
+# The result of a CPU model baseline.
+#
+# @model: the baselined CpuModelInfo.
+#
+# Since: 2.7.0
+##
+{ 'struct': 'CpuModelBaselineInfo',
+  'data': { 'model': 'CpuModelInfo' } }
+
+##
+# @query-cpu-model-baseline:
+#
+# Baseline two CPU models, creating a compatible third model.
+#
+# Returns: a CpuModelBaselineInfo. Returns an error if CPU models are not
+#  supported, if a model cannot be used, if the model contains
+#  an unknown cpu definition name, unknown properties or properties
+#  with a wrong type.
+#
+# Since: 2.7.0
+##
+{ 'command': 'query-cpu-model-baseline',
+  'data': { 'type' : 'CpuModelBaselineType',
+'modela': 'CpuModelInfo',
+'modelb': 'CpuModelInfo' },
+  'returns': 'CpuModelBaselineInfo' }
+
 # @AddfdInfo:
 #
 # Information about a file descriptor that was added to an fd set.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 4ee7937..42b3853 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3942,6 +3942,12 @@ EQMP
 },
 
 {
+.name   = "query-cpu-model-baseline",
+.args_type  = "type:s,modela:q,modelb:q",
+.mhandler.cmd_new = qmp_marshal_query_cpu_model_baseline,
+},
+
+{
 .name   = "query-target",
 .args_type  = "",
 .mhandler.cmd_new = qmp_marshal_query_target,
diff --git a/qmp.c b/qmp.c
index afa8c77..6e5437f 100644
--- a/qmp.c
+++ b/qmp.c
@@ -621,6 +621,14 @@ CpuModelCompareInfo 
*qmp_query_cpu_model_comparison(CpuModelInfo *modela,
 return arch_query_cpu_model_comparison(modela, modelb, errp);
 }
 
+CpuModelBaselineInfo *qmp_query_cpu_model_baseline(CpuModelBaselineType type,
+   CpuModelInfo *modela,
+   CpuModelInfo *modelb,
+   Error **errp)
+{
+return arch_query_cpu_model_baseline(type, modela, modelb, errp);
+}
+
 void qmp_add_client(const char *protocol, const char *fdname,
 bool has_skipauth, bool skipauth, bool has_tls, bool tls,
 Error **errp)
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index b831dbc..56bddd8 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -1,6 +1,7 @@
 stub-obj-y += arch-query-cpu-def.o
 stub-obj-y += arch-query-cpu-model-expansion.o
 stub-obj-y += arch-query-cpu-model-comparison.o
+stub-obj-y += arch-query-cpu-model-baseline.o
 stub-obj-y += bdrv-next-monitor-owned.o
 stub-obj-y += blk-commit-all.o
 stub-obj-y += blockdev-c

[Qemu-devel] [RFC 09/28] s390x/cpumodel: expose features and feature groups as properties

[David Hildenbrand]

Let's add all features and feature groups as properties to all CPU models.
If the "host" CPU model is unknown, we can neither query nor change
features. KVM will just continue to work like it did until now.

We will not allow to enable features that were not part of the original
CPU model, because that could collide with the IBC in KVM.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 target-s390x/cpu.c|   1 +
 target-s390x/cpu.h|   1 +
 target-s390x/cpu_models.c | 149 ++
 3 files changed, 151 insertions(+)

diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index d7d0b62..2f3c8e2 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -291,6 +291,7 @@ static void s390_cpu_initfn(Object *obj)
 cs->exception_index = EXCP_HLT;
 object_property_add(OBJECT(cpu), "id", "int64_t", s390x_cpu_get_id,
 s390x_cpu_set_id, NULL, NULL, NULL);
+s390_cpu_model_register_props(obj);
 #if !defined(CONFIG_USER_ONLY)
 qemu_get_timedate(, 0);
 env->tod_offset = TOD_UNIX_EPOCH +
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index ec60b3b..47a1861 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -633,6 +633,7 @@ extern void subsystem_reset(void);
 
 void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 #define cpu_list s390_cpu_list
+void s390_cpu_model_register_props(Object *obj);
 void s390_cpu_model_class_register_props(ObjectClass *oc);
 void s390_realize_cpu_model(CPUState *cs, Error **errp);
 ObjectClass *s390_cpu_class_by_name(const char *name);
diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index d54b795..93e63cb 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -14,6 +14,7 @@
 #include "cpu.h"
 #include "gen-features.h"
 #include "qapi/error.h"
+#include "qapi/visitor.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
 #endif
@@ -96,8 +97,24 @@ void s390_cpu_list(FILE *f, fprintf_function print)
 f = f,
 print = print,
 };
+S390FeatGroup group;
+S390Feat feat;
 
 object_class_foreach(print_cpu_model_list, TYPE_S390_CPU, false, );
+
+(*print)(f, "\nRecognized feature flags:\n");
+for (feat = 0; feat < S390_FEAT_MAX; feat++) {
+const S390FeatDef *def = s390_feat_def(feat);
+
+(*print)(f, "%-20s %-50s\n", def->name, def->desc);
+}
+
+(*print)(f, "\nRecognized feature groups:\n");
+for (group = 0; group < S390_FEAT_GROUP_MAX; group++) {
+const S390FeatGroupDef *def = s390_feat_group_def(group);
+
+(*print)(f, "%-20s %-50s\n", def->name, def->desc);
+}
 }
 
 #ifndef CONFIG_USER_ONLY
@@ -139,6 +156,138 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
 }
 }
 
+static void get_feature(Object *obj, Visitor *v, const char *name,
+void *opaque, Error **errp)
+{
+S390Feat feat = (S390Feat) opaque;
+S390CPU *cpu = S390_CPU(obj);
+bool value;
+
+if (!cpu->model) {
+error_setg(errp, "Details about the host CPU model are not available, "
+ "features cannot be queried.");
+return;
+}
+
+value = test_bit(feat, cpu->model->features);
+visit_type_bool(v, name, , errp);
+}
+
+static void set_feature(Object *obj, Visitor *v, const char *name,
+void *opaque, Error **errp)
+{
+S390Feat feat = (S390Feat) opaque;
+DeviceState *dev = DEVICE(obj);
+S390CPU *cpu = S390_CPU(obj);
+bool value;
+
+if (dev->realized) {
+error_setg(errp, "Attempt to set property '%s' on '%s' after "
+   "it was realized", name, object_get_typename(obj));
+return;
+} else if (!cpu->model) {
+error_setg(errp, "Details about the host CPU model are not available, "
+ "features cannot be changed.");
+return;
+}
+
+visit_type_bool(v, name, , errp);
+if (*errp) {
+return;
+}
+if (value) {
+if (!test_bit(feat, cpu->model->def->full_feat)) {
+error_setg(errp, "Feature '%s' is not available for CPU model 
'%s',"
+   " it was introduced with later models.",
+   name, cpu->model->def->name);
+return;
+}
+set_bit(feat, cpu->model->features);
+} else {
+clear_bit(feat, cpu->model->features);
+}
+}
+
+static void get_feature_group(Object *obj, Visitor *v, const char *name,
+  void *opaque, Error **errp)
+{
+S390FeatGroup group = (S390FeatGroup) opaque;
+const S390Feat

[Qemu-devel] [RFC 08/28] s390x/cpumodel: store the CPU model in the CPU instance

[David Hildenbrand]

A CPU model consists of a CPU definition, to which delta changes are
applied - features added or removed (e.g. z13-base,vx=on). In addition,
certain properties (e.g. cpu id) can later on change during migration
but belong into the CPU model. This data will later be filled from the
host model in the KVM case.

Therefore, store the configured CPU model inside the CPU instance, so
we can later on perform delta changes using properties.

For the "qemu" model, we emulate in TCG a z900. "host" will be
uninitialized (cpu->model == NULL) unless we have CPU model support in KVM
later on. The other models are all initialized from their definitions.
Only the "host" model can have a cpu->model == NULL.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 target-s390x/cpu.h|  1 +
 target-s390x/cpu_models.c | 26 ++
 target-s390x/cpu_models.h | 10 ++
 3 files changed, 37 insertions(+)

diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 832da89..ec60b3b 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -187,6 +187,7 @@ struct S390CPU {
 
 CPUS390XState env;
 int64_t id;
+S390CPUModel *model;
 /* needed for live migration */
 void *irqstate;
 uint32_t irqstate_saved_size;
diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index 50b395a..d54b795 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -141,6 +141,21 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
 
 static void s390_cpu_model_initfn(Object *obj)
 {
+S390CPU *cpu = S390_CPU(obj);
+S390CPUClass *xcc = S390_CPU_GET_CLASS(cpu);
+
+cpu->model = g_malloc0(sizeof(*cpu->model));
+/* copy the model, so we can modify it */
+cpu->model->def = xcc->cpu_def;
+if (xcc->migration_safe) {
+/* base model - features will mever change */
+bitmap_copy(cpu->model->features, cpu->model->def->base_feat,
+S390_FEAT_MAX);
+} else {
+/* latest model - features can change */
+bitmap_copy(cpu->model->features,
+cpu->model->def->default_feat, S390_FEAT_MAX);
+}
 }
 
 #ifdef CONFIG_KVM
@@ -151,10 +166,21 @@ static void s390_host_cpu_model_initfn(Object *obj)
 
 static void s390_qemu_cpu_model_initfn(Object *obj)
 {
+S390CPU *cpu = S390_CPU(obj);
+
+cpu->model = g_malloc0(sizeof(*cpu->model));
+/* TCG emulates a z900 */
+cpu->model->def = _cpu_defs[0];
+bitmap_copy(cpu->model->features, cpu->model->def->default_feat,
+S390_FEAT_MAX);
 }
 
 static void s390_cpu_model_finalize(Object *obj)
 {
+S390CPU *cpu = S390_CPU(obj);
+
+g_free(cpu->model);
+cpu->model = NULL;
 }
 
 static bool get_migratable(Object *obj, Error **errp)
diff --git a/target-s390x/cpu_models.h b/target-s390x/cpu_models.h
index 13f7217..244256b 100644
--- a/target-s390x/cpu_models.h
+++ b/target-s390x/cpu_models.h
@@ -33,4 +33,14 @@ typedef struct S390CPUDef {
 S390FeatBitmap full_feat;
 } S390CPUDef;
 
+/* CPU model based on a CPU definition */
+typedef struct S390CPUModel {
+const S390CPUDef *def;
+S390FeatBitmap features;
+/* values copied from the "host" model, can change during migration */
+uint16_t lowest_ibc;/* lowest IBC that the hardware supports */
+uint32_t cpu_id;/* CPU id */
+uint8_t cpu_ver;/* CPU version, usually "ff" for kvm */
+} S390CPUModel;
+
 #endif /* TARGET_S390X_CPU_MODELS_H */
-- 
2.6.6

[Qemu-devel] [RFC 22/28] s390x/kvm: let the CPU model control CMM(A)

[David Hildenbrand]

Starting with recent kernels, if the cmma attributes are available, we
actually have hardware support. Enabling CMMA then means providing the
guest VCPU with CMM, therefore enabling its CMM facility.

Let's not blindly enable CMM anymore but let's control it using CPU models.
For disabled CPU models, CMMA will continue to always get enabled.

Also enable it in the applicable default models.

Please note that CMM doesn't work with hugetlbfs, therefore we will
warn the user and keep it disabled. Migrating from/to a hugetlbfs
configuration works, as it will be disabled on both sides.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 target-s390x/gen-features.c |  1 +
 target-s390x/kvm.c  | 47 ++---
 2 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/target-s390x/gen-features.c b/target-s390x/gen-features.c
index d4f4b29..52d46dc 100644
--- a/target-s390x/gen-features.c
+++ b/target-s390x/gen-features.c
@@ -371,6 +371,7 @@ static uint16_t full_GEN13_GA1[] = {
 #define default_GEN8_GA5 EmptyFeat
 static uint16_t default_GEN9_GA1[] = {
 S390_FEAT_GROUP_MSA_EXT_1,
+S390_FEAT_CMM,
 };
 #define default_GEN9_GA2 EmptyFeat
 #define default_GEN9_GA3 EmptyFeat
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index a4f5762..dd2abec 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -174,6 +174,18 @@ int kvm_s390_set_mem_limit(KVMState *s, uint64_t 
new_limit, uint64_t *hw_limit)
 return kvm_vm_ioctl(s, KVM_SET_DEVICE_ATTR, );
 }
 
+static bool kvm_s390_cmma_available(void)
+{
+static bool initialized, value;
+
+if (!initialized) {
+initialized = true;
+value = kvm_vm_check_mem_attr(kvm_state, KVM_S390_VM_MEM_ENABLE_CMMA) 
&&
+kvm_vm_check_mem_attr(kvm_state, KVM_S390_VM_MEM_CLR_CMMA);
+}
+return value;
+}
+
 void kvm_s390_cmma_reset(void)
 {
 int rc;
@@ -182,11 +194,15 @@ void kvm_s390_cmma_reset(void)
 .attr = KVM_S390_VM_MEM_CLR_CMMA,
 };
 
+if (!mem_path || !kvm_s390_cmma_available()) {
+return;
+}
+
 rc = kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, );
 trace_kvm_clear_cmma(rc);
 }
 
-static void kvm_s390_enable_cmma(KVMState *s)
+static void kvm_s390_enable_cmma(void)
 {
 int rc;
 struct kvm_device_attr attr = {
@@ -194,12 +210,7 @@ static void kvm_s390_enable_cmma(KVMState *s)
 .attr = KVM_S390_VM_MEM_ENABLE_CMMA,
 };
 
-if (!kvm_vm_check_mem_attr(s, KVM_S390_VM_MEM_ENABLE_CMMA) ||
-!kvm_vm_check_mem_attr(s, KVM_S390_VM_MEM_CLR_CMMA)) {
-return;
-}
-
-rc = kvm_vm_ioctl(s, KVM_SET_DEVICE_ATTR, );
+rc = kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, );
 trace_kvm_enable_cmma(rc);
 }
 
@@ -259,10 +270,6 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
 cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
 cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
 
-if (!mem_path) {
-kvm_s390_enable_cmma(s);
-}
-
 if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
 || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
 phys_mem_set_alloc(legacy_s390_alloc);
@@ -2509,6 +2516,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, 
Error **errp)
 return;
 }
 
+/* with cpu model support, CMM is only indicated if really available */
+if (kvm_s390_cmma_available()) {
+set_bit(S390_FEAT_CMM, model->features);
+}
+
 if (s390_known_cpu_type(cpu_type)) {
 /* we want the exact model, even if some features are missing */
 model->def = s390_find_cpu_def(cpu_type, ibc_gen(unblocked_ibc),
@@ -2541,6 +2553,10 @@ void kvm_s390_apply_cpu_model(const S390CPUModel *model, 
Error **errp)
 int rc;
 
 if (!model) {
+/* compatibility handling if cpu models are disabled */
+if (kvm_s390_cmma_available() && !mem_path) {
+kvm_s390_enable_cmma();
+}
 return;
 }
 if (!kvm_s390_cpu_models_supported()) {
@@ -2569,4 +2585,13 @@ void kvm_s390_apply_cpu_model(const S390CPUModel *model, 
Error **errp)
 error_setg(errp, "KVM: Error configuring CPU subfunctions: %d", rc);
 return;
 }
+/* enable CMM via CMMA - disable on hugetlbfs */
+if (test_bit(S390_FEAT_CMM, model->features)) {
+if (mem_path) {
+error_report("Warning: CMM will not be enabled because it is not "
+ "compatible to hugetlbfs.");
+} else {
+kvm_s390_enable_cmma();
+}
+}
 }
-- 
2.6.6

[Qemu-devel] [RFC 15/28] s390x/sclp: propagate the ibc val(lowest and unblocked ibc)

[David Hildenbrand]

If we have a lowest ibc, we can indicate the ibc to the guest.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 hw/s390x/sclp.c   |  2 ++
 include/hw/s390x/sclp.h   |  3 ++-
 target-s390x/cpu_models.c | 21 +
 target-s390x/cpu_models.h | 12 
 4 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 3c126ee..52f8bb9 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -62,6 +62,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
 read_info->highest_cpu = cpu_to_be16(max_cpus);
 
+read_info->ibc_val = cpu_to_be32(s390_get_ibc_val());
+
 /* Configuration Characteristic (Extension) */
 s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR,
  read_info->conf_char);
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index 743abd4..4580134 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -120,7 +120,8 @@ typedef struct ReadInfo {
 uint8_t  loadparm[8];   /* 24-31 */
 uint8_t  _reserved3[48 - 32];   /* 32-47 */
 uint64_t facilities;/* 48-55 */
-uint8_t  _reserved0[80 - 56];   /* 56-79 */
+uint8_t  _reserved0[76 - 56];   /* 56-75 */
+uint32_t ibc_val;
 uint8_t  conf_char[96 - 80];/* 80-95 */
 uint8_t  _reserved4[100 - 96];  /* 96-99 */
 uint32_t rnsize2;
diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index 571ceaa..d58b931 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -74,6 +74,27 @@ static const S390CPUDef s390_cpu_defs[] = {
 CPUDEF_INIT(0x2965, 13, 2, 47, 0x0800U, "z13s", "IBM z13s GA1"),
 };
 
+uint32_t s390_get_ibc_val(void)
+{
+uint16_t unblocked_ibc, lowest_ibc;
+static S390CPU *cpu;
+
+if (!cpu) {
+cpu = S390_CPU(qemu_get_cpu(0));
+}
+
+if (!cpu || !cpu->model) {
+return 0;
+}
+unblocked_ibc = s390_ibc_from_cpu_model(cpu->model);
+lowest_ibc = cpu->model->lowest_ibc;
+/* the lowest_ibc always has to be <= unblocked_ibc */
+if (!lowest_ibc || lowest_ibc > unblocked_ibc) {
+return 0;
+}
+return ((uint32_t) lowest_ibc << 16) | unblocked_ibc;
+}
+
 void s390_get_feat_block(S390FeatType type, uint8_t *data)
 {
 static S390CPU *cpu;
diff --git a/target-s390x/cpu_models.h b/target-s390x/cpu_models.h
index 04c47c6..bbb85ac 100644
--- a/target-s390x/cpu_models.h
+++ b/target-s390x/cpu_models.h
@@ -43,6 +43,18 @@ typedef struct S390CPUModel {
 uint8_t cpu_ver;/* CPU version, usually "ff" for kvm */
 } S390CPUModel;
 
+#define S390_GEN_Z10 0xa
+
+uint32_t s390_get_ibc_val(void);
+static inline uint16_t s390_ibc_from_cpu_model(const S390CPUModel *model)
+{
+uint16_t ibc = 0;
+
+if (model->def->gen >= S390_GEN_Z10) {
+ibc = ((model->def->gen - S390_GEN_Z10) << 4) + model->def->ec_ga;
+}
+return ibc;
+}
 void s390_get_feat_block(S390FeatType type, uint8_t *data);
 bool s390_has_feat(S390Feat feat);
 
-- 
2.6.6

[Qemu-devel] [RFC 23/28] qmp: add QMP interface "query-cpu-model-expansion"

[David Hildenbrand]

Let's provide a standardized interface to expand CPU models, like the
host model.

To take care of all architectures, different detail levels for an expansion
are introduced. Certain architectures might not support all detail levels.

When the host CPU model is to be expanded, at least the accelerator
QEMU has been started with affects the result. Some architectures
might decide to take the QEMU machine also into account.

E.g. for s390x, the host CPU model will only depend on the accelerator.

The result  of "stable" will be a migration safe CPU model, that will
produce the same guest ABI on other QEMU versions. This interface is very
helpful when CPU models are to be updated between QEMU versions. The
updated model can then simply be expanded to a migration safe
representation.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 include/sysemu/arch_init.h |  3 ++
 qapi-schema.json   | 81 ++
 qmp-commands.hx|  6 +++
 qmp.c  |  7 +++
 stubs/Makefile.objs|  1 +
 stubs/arch-query-cpu-model-expansion.c | 12 +
 6 files changed, 110 insertions(+)
 create mode 100644 stubs/arch-query-cpu-model-expansion.c

diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
index d690dfa..37b2e86 100644
--- a/include/sysemu/arch_init.h
+++ b/include/sysemu/arch_init.h
@@ -35,5 +35,8 @@ int kvm_available(void);
 int xen_available(void);
 
 CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp);
+CpuModelExpansionInfo *arch_query_cpu_model_expansion(CpuModelExpansionType 
type,
+  CpuModelInfo *mode,
+  Error **errp);
 
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 0964eec..5b72dc0 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3026,6 +3026,87 @@
 ##
 { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'] }
 
+##
+# @CpuModelInfo:
+#
+# Virtual CPU model.
+#
+# A CPU model consists of the name of a CPU definition, to which
+# delta changes are applied (e.g. features added/removed). Most magic values
+# that an architecture might require should be hidden behind the name.
+# However, if required, architectures can expose relevant properties.
+#
+# @name: the name of the CPU definition the model is based on
+# @props: #optional a dictionary of properties to be applied
+#
+# Since: 2.7.0
+##
+{ 'struct': 'CpuModelInfo',
+  'data': { 'name': 'str',
+'*props': 'any' } }
+
+##
+# @CpuModelExpansionType
+#
+# An enumeration of CPU model expansion types.
+#
+# @stable: Expand to a stable CPU model, creating a migration-safe
+#  representation with only delta changes.
+#
+# @migratable: Expand all migratable properties, hiding unmigratable
+#  properties.
+#
+# @full: Expand all properties, including unmigratable ones.
+#
+# Since: 2.7.0
+##
+{ 'enum': 'CpuModelExpansionType',
+  'data': [ 'stable', 'migratable', 'full' ] }
+
+
+##
+# @CpuModelExpansionInfo
+#
+# The result of a cpu model expansion.
+#
+# @model: the expanded CpuModelInfo.
+#
+# Since: 2.7.0
+##
+{ 'struct': 'CpuModelExpansionInfo',
+  'data': { 'model': 'CpuModelInfo' } }
+
+
+##
+# @query-cpu-model-expansion:
+#
+# Expands the given CPU model to the requested detail level.
+# For "stable", a migration-safe representation is created, which will look
+# the same on all QEMU versions and only contains delta changes. "migratable"
+# and "full" will expose properties in a more detailed way. Not all types
+# might be supported for an architecture.
+#
+# Expanding CPU models is in general independant of the accelerator, except
+# for models like "host" that explicitly rely on an accelerator and can
+# vary in different configurations. On certain architectures, the result may
+# rely on the QEMU machine.
+#
+# This interface can therefore also be used to query the "host" capabilities
+# on supporting architectures.
+#
+# Returns: a CpuModelExpansionInfo. Returns an error if CPU models are not
+#  supported, if the model cannot be expanded, if the model contains
+#  an unknown cpu definition name, unknown properties or properties
+#  with a wrong type. Also returns an error if an expansion type is
+#  not supported.
+#
+# Since: 2.7.0
+##
+{ 'command': 'query-cpu-model-expansion',
+  'data': { 'type': 'CpuModelExpansionType',
+'model': 'CpuModelInfo' },
+  'returns': 'CpuModelExpansionInfo' }
+
 # @AddfdInfo:
 #
 # Information about a file descriptor that was added to an fd set.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b444c20..b279fc5 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3930,6 +3930,12 @@ EQMP
 },
 
 {
+.n

[Qemu-devel] [RFC 02/28] s390x/cpumodel: expose CPU class properties

[David Hildenbrand]

Let's expose the description and migration safety, as class properties,
this can be helpful in the future.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 target-s390x/cpu.c|  1 +
 target-s390x/cpu.h|  1 +
 target-s390x/cpu_models.c | 18 ++
 3 files changed, 20 insertions(+)

diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 44e53ec..d7d0b62 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -445,6 +445,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
  * object_unref().
  */
 dc->cannot_destroy_with_object_finalize_yet = true;
+s390_cpu_model_class_register_props(oc);
 }
 
 static const TypeInfo s390_cpu_type_info = {
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index f1cb9a2..832da89 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -632,6 +632,7 @@ extern void subsystem_reset(void);
 
 void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 #define cpu_list s390_cpu_list
+void s390_cpu_model_class_register_props(ObjectClass *oc);
 void s390_realize_cpu_model(CPUState *cs, Error **errp);
 ObjectClass *s390_cpu_class_by_name(const char *name);
 
diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index 08ed640..18638a9 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -98,6 +98,24 @@ static void s390_cpu_model_finalize(Object *obj)
 {
 }
 
+static bool get_migratable(Object *obj, Error **errp)
+{
+return S390_CPU_GET_CLASS(obj)->migration_safe;
+}
+
+static char *get_description(Object *obj, Error **errp)
+{
+return g_strdup(S390_CPU_GET_CLASS(obj)->desc);
+}
+
+void s390_cpu_model_class_register_props(ObjectClass *oc)
+{
+object_class_property_add_bool(oc, "migratable", get_migratable, NULL,
+   NULL);
+object_class_property_add_str(oc, "description", get_description, NULL,
+  NULL);
+}
+
 #ifdef CONFIG_KVM
 static void s390_host_cpu_model_class_init(ObjectClass *oc, void *data)
 {
-- 
2.6.6

[Qemu-devel] [RFC 06/28] s390x/cpumodel: introduce CPU feature group definitions

[David Hildenbrand]

Let's use the generated groups to create feature group representations for
the user. These groups can later be used to enable/disable multiple
features in one shot and will be used to reduce the amount of reported
features to the user if all subfeatures are in place.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 target-s390x/cpu_features.c | 44 +++-
 target-s390x/cpu_features.h | 23 +++
 2 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/target-s390x/cpu_features.c b/target-s390x/cpu_features.c
index c78a189..6ec2bfc 100644
--- a/target-s390x/cpu_features.c
+++ b/target-s390x/cpu_features.c
@@ -12,6 +12,7 @@
 
 #include "qemu/osdep.h"
 #include "cpu_features.h"
+#include "gen-features.h"
 
 #define FEAT_INIT(_name, _type, _bit, _desc) \
 {\
@@ -321,14 +322,55 @@ void s390_add_from_feat_block(S390FeatBitmap features, 
S390FeatType type,
 }
 }
 
-void s390_feat_bitmap_to_ascii(const S390FeatBitmap bitmap, void *opaque,
+void s390_feat_bitmap_to_ascii(const S390FeatBitmap features, void *opaque,
void (*fn)(const char *name, void *opaque))
 {
+S390FeatBitmap bitmap, tmp;
+S390FeatGroup group;
 S390Feat feat;
 
+bitmap_copy(bitmap, features, S390_FEAT_MAX);
+
+/* process whole groups first */
+for (group = 0; group < S390_FEAT_GROUP_MAX; group++) {
+const S390FeatGroupDef *def = s390_feat_group_def(group);
+
+bitmap_and(tmp, bitmap, def->feat, S390_FEAT_MAX);
+if (bitmap_equal(tmp, def->feat, S390_FEAT_MAX)) {
+bitmap_andnot(bitmap, bitmap, def->feat, S390_FEAT_MAX);
+(*fn)(def->name, opaque);
+}
+}
+
+/* report leftovers as separate features */
 feat = find_first_bit(bitmap, S390_FEAT_MAX);
 while (feat < S390_FEAT_MAX) {
 (*fn)(s390_feat_def(feat)->name, opaque);
 feat = find_next_bit(bitmap, S390_FEAT_MAX, feat + 1);
 };
 }
+
+#define FEAT_GROUP_INIT(_name, _group, _desc)\
+{\
+.name = _name,   \
+.desc = _desc,   \
+.feat = { S390_FEAT_GROUP_LIST_ ## _group }, \
+}
+
+/* indexed by feature group number for easy lookup */
+static const S390FeatGroupDef s390_feature_groups[] = {
+FEAT_GROUP_INIT("plo", PLO, "Perform-locked-operation facility"),
+FEAT_GROUP_INIT("tods", TOD_CLOCK_STEERING, "Tod-clock-steering facility"),
+FEAT_GROUP_INIT("gen13ptff", GEN13_PTFF, "PTFF enhancements introduced 
with z13"),
+FEAT_GROUP_INIT("msa", MSA, "Message-security-assist facility"),
+FEAT_GROUP_INIT("msa1", MSA_EXT_1, "Message-security-assist-extension 1 
facility"),
+FEAT_GROUP_INIT("msa2", MSA_EXT_2, "Message-security-assist-extension 2 
facility"),
+FEAT_GROUP_INIT("msa3", MSA_EXT_3, "Message-security-assist-extension 3 
facility"),
+FEAT_GROUP_INIT("msa4", MSA_EXT_4, "Message-security-assist-extension 4 
facility"),
+FEAT_GROUP_INIT("msa5", MSA_EXT_5, "Message-security-assist-extension 5 
facility"),
+};
+
+const S390FeatGroupDef *s390_feat_group_def(S390FeatGroup group)
+{
+return _feature_groups[group];
+}
diff --git a/target-s390x/cpu_features.h b/target-s390x/cpu_features.h
index 485446c..e40a636 100644
--- a/target-s390x/cpu_features.h
+++ b/target-s390x/cpu_features.h
@@ -273,6 +273,29 @@ void s390_add_from_feat_block(S390FeatBitmap features, 
S390FeatType type,
 void s390_feat_bitmap_to_ascii(const S390FeatBitmap features, void *opaque,
void (*fn)(const char *name, void *opaque));
 
+/* static groups that will never change */
+typedef enum {
+S390_FEAT_GROUP_PLO,
+S390_FEAT_GROUP_TOD_CLOCK_STEERING,
+S390_FEAT_GROUP_GEN13_PTFF_ENH,
+S390_FEAT_GROUP_MSA,
+S390_FEAT_GROUP_MSA_EXT_1,
+S390_FEAT_GROUP_MSA_EXT_2,
+S390_FEAT_GROUP_MSA_EXT_3,
+S390_FEAT_GROUP_MSA_EXT_4,
+S390_FEAT_GROUP_MSA_EXT_5,
+S390_FEAT_GROUP_MAX,
+} S390FeatGroup;
+
+/* Definition of a CPU feature group */
+typedef struct {
+const char *name;   /* name exposed to the user */
+const char *desc;   /* description exposed to the user */
+S390FeatBitmap feat;/* features contained in the group */
+} S390FeatGroupDef;
+
+const S390FeatGroupDef *s390_feat_group_def(S390FeatGroup group);
+
 #define BE_BIT_NR(BIT) (BIT ^ (BITS_PER_LONG - 1))
 #define BE_BIT(BIT) (1ULL < BE_BIT_NR(BIT))
 
-- 
2.6.6

[Qemu-devel] [RFC 01/28] s390x/cpumodel: "host" and "qemu" as CPU subclasses

[David Hildenbrand]

This patch introduces two CPU models, "host" and "qemu".
"qemu" is used as default when running under TCG. "host" is used
as default when running under KVM. "host" cannot be used without KVM.
Bot models are not migration safe. They inherit from the base s390x CPU,
which is turned into an abstract class.

This patch also changes CPU creation to take care of the passed CPU string
and reuses common code parse_features() function for that purpose. Unknown
CPU definitions are now reported. The "-cpu ?" and "query-cpu-definition"
commands are changed to list all CPU subclasses automatically.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 hw/s390x/s390-virtio.c |   6 +-
 target-s390x/Makefile.objs |   2 +-
 target-s390x/cpu-qom.h |   3 +
 target-s390x/cpu.c |  33 +++--
 target-s390x/cpu.h |   2 +
 target-s390x/cpu_models.c  | 164 +
 target-s390x/helper.c  |  29 +++-
 7 files changed, 211 insertions(+), 28 deletions(-)
 create mode 100644 target-s390x/cpu_models.c

diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 544c616..0a96347 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -101,7 +101,11 @@ void s390_init_cpus(MachineState *machine)
 gchar *name;
 
 if (machine->cpu_model == NULL) {
-machine->cpu_model = "host";
+if (kvm_enabled()) {
+machine->cpu_model = "host";
+} else {
+machine->cpu_model = "qemu";
+}
 }
 
 cpu_states = g_new0(S390CPU *, max_cpus);
diff --git a/target-s390x/Makefile.objs b/target-s390x/Makefile.objs
index dd62cbd..34bd693 100644
--- a/target-s390x/Makefile.objs
+++ b/target-s390x/Makefile.objs
@@ -1,5 +1,5 @@
 obj-y += translate.o helper.o cpu.o interrupt.o
 obj-y += int_helper.o fpu_helper.o cc_helper.o mem_helper.o misc_helper.o
-obj-y += gdbstub.o
+obj-y += gdbstub.o cpu_models.o
 obj-$(CONFIG_SOFTMMU) += machine.o ioinst.o arch_dump.o mmu_helper.o
 obj-$(CONFIG_KVM) += kvm.o
diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
index 66b5d18..ae0be73 100644
--- a/target-s390x/cpu-qom.h
+++ b/target-s390x/cpu-qom.h
@@ -45,6 +45,9 @@ typedef struct S390CPUClass {
 /*< private >*/
 CPUClass parent_class;
 /*< public >*/
+bool kvm_required;
+bool migration_safe;
+const char *desc;
 
 int64_t next_cpu_id;
 
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index e43e2d6..44e53ec 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -44,30 +44,6 @@
 #define CR0_RESET   0xE0UL
 #define CR14_RESET  0xC200UL;
 
-/* generate CPU information for cpu -? */
-void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf)
-{
-#ifdef CONFIG_KVM
-(*cpu_fprintf)(f, "s390 %16s\n", "host");
-#endif
-}
-
-#ifndef CONFIG_USER_ONLY
-CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
-{
-CpuDefinitionInfoList *entry;
-CpuDefinitionInfo *info;
-
-info = g_malloc0(sizeof(*info));
-info->name = g_strdup("host");
-
-entry = g_malloc0(sizeof(*entry));
-entry->value = info;
-
-return entry;
-}
-#endif
-
 static void s390_cpu_set_pc(CPUState *cs, vaddr value)
 {
 S390CPU *cpu = S390_CPU(cs);
@@ -206,6 +182,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
**errp)
 CPUS390XState *env = >env;
 Error *err = NULL;
 
+/* the model has to be realized before qemu_init_vcpu() due to kvm */
+s390_realize_cpu_model(cs, );
+if (err) {
+goto out;
+}
+
 #if !defined(CONFIG_USER_ONLY)
 if (cpu->id >= max_cpus) {
 error_setg(, "Unable to add CPU: %" PRIi64
@@ -435,6 +417,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
 scc->cpu_reset = s390_cpu_reset;
 scc->initial_cpu_reset = s390_cpu_initial_reset;
 cc->reset = s390_cpu_full_reset;
+cc->class_by_name = s390_cpu_class_by_name,
 cc->has_work = s390_cpu_has_work;
 cc->do_interrupt = s390_cpu_do_interrupt;
 cc->dump_state = s390_cpu_dump_state;
@@ -470,7 +453,7 @@ static const TypeInfo s390_cpu_type_info = {
 .instance_size = sizeof(S390CPU),
 .instance_init = s390_cpu_initfn,
 .instance_finalize = s390_cpu_finalize,
-.abstract = false,
+.abstract = true,
 .class_size = sizeof(S390CPUClass),
 .class_init = s390_cpu_class_init,
 };
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index bd6b2e5..f1cb9a2 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -632,6 +632,8 @@ extern void subsystem_reset(void);
 
 void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 #define cpu_list s390_cpu_list
+void s390_realize_cpu_model(CPUState *cs, Error **errp);
+ObjectClass *s390_cpu_class_b

[Qemu-devel] [RFC 00/28] s390x CPU models: exposing features

[David Hildenbrand]

This is our second attempt to implement CPU models for s390x. We realized
that we also want to have features exposed via the CPU model. While doing
that we realized that we want to have a better interface for libvirt.

Unfortunately, CPU models on s390x are special and we have to take care of:
- A CPU like z13 looks differently in various environments (under different
  LPAR versions, under different z/VM versions, under different KVM
  versions, export regulation) - we have _a lot_ of feature variability.
- We still have certain features that are not published but might be
  implemented/introduced in the future. As they are a theoretical part
  of a CPU already, we have to find a way to model these future changes.
- We still have certain features that are already published, but not
  implemented. Implementation might be added in the future in KVM.
- We heavily rely on KVM to tell us which features it can actually
  virtualize - user space queries like "STFL(e)" give no guarantees.
- Certain "subfeatures" were introduced in one run. In practice, they are
  always around, but in theory, subfeatures could be dropped in the future.
- Just because two CPU models have the same features doesn't mean they
  are equal - some internal numbers might be different. E.g. we won't allow
  running a z13 under a zBC12 just by turning off features.
- We cannot blindly enable all possible features for a CPU generation,
  the IBC "Instruction Blocking Control" in KVM will try to block
  instructions introduced with certain features. So a CPU generation always
  has some maximum feature set that is guaranteed to work.

It all boils down to a specific released CPU to have.
a) A fixed feature set that we expect it to be have on every hypervisor.
b) A variable part that depends on the hypervisor and that could be
   extended in the future (adding not yet implemented features) that we
   always want to enable later on.
c) A variable part that we want to enable only if requested - nested
   virtualization ("vsie") and assists are one example.

But, the fixed feature set is not really what we want to use as a default.
It is just like a really minimum, stable base.

So we have
a) A "stable" CPU model for each released CPU that will never change and
   maps to the minimum feature set we expect to be around on all
   hypervisors. e.g. "z13-base" or "z10EC.2-base". These are migration
   safe.
b) A "default" CPU model for each released CPU, that can change between
   QEMU versions and that will always include the features we expect to
   be around in our currently supported environments and will contain only
   features we expect to be stable. E.g. nested virtualization will not be
   contained in these models. These models are not migration safe, e.g
   "z13" or "z10EC.2". The feature set can differ between QEMU versions.
c) An internal "maximum" CPU model for each generation that tells us which
   features were supported as a maximum back when the hardware was
   released. This will not be exposed

To not have to replicate all CPU model changes ("new default fetaures") in
libvirt, to not duplicate the logic about compatibility and the like,
our approach tries to keep all the QEMU logic in libvirt and provide
standardized interfaces for libvirt to e.g. baseline, compare. This
allows libvirt to not have to care about any model names or feature names,
it can just pass the data from interface to interface and report it to
the user.

Also, libvirt might want to know what the "host" model looks like and
convert a CPU model to a migration safe variant. For this reason, a QMP
command is added that can create a migration safe variant of a variable
CPU model, indicating only the delta changes done to a stable model.

So we have:
a) "query-cpu-model-expansion" - tell us what the "host" or a migration
   unsafe model looks like. Either falling back to a stable model or
   completely exposing all properties. We are interested in stable models.
b) "query-cpu-model-comparison" - tell us how two CPU models compare,
indicating which properties were responsible for the decision.
c) "query-cpu-model-baseline" - create a new model out of two models,
taking a requested level of stability into account.

As we are aware that e.g. x86 has their own idea of a CPU model and their
existing implementation in place, but are also looking into to ways to e.g.
expand the "host" CPU model to a detailed representation, we designed the
"expansion" interface to also allow that.

Comments are very welcome, but please always keep the restrictions and
specialties in mind when suggesting some major design changes.

The header update will be replaced by a kvm-next header update as soon as
the VSIE patches are upstream. The major KVM interface changes are already
part of kvm-nex

[Qemu-devel] [RFC 11/28] s390x/cpumodel: check and apply the CPU model

[David Hildenbrand]

We have to test if a configured CPU model is runnable in the current
configuration, and if not report why that is the case. This is done by
comparing it to the maximum supported model (host for KVM or z900 for TCG).
Also, we want to do some base sanity checking for a configured CPU model.

We'll cache the maximum model and the applied model (for performance
reasons and because KVM can only be configured before any VCPU is created).

For unavailable "host" model, we have to make sure that we inform KVM,
so it can do some compatibility stuff (enable CMMA later on to be precise).

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 target-s390x/cpu_models.c | 154 ++
 1 file changed, 154 insertions(+)

diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index 7e3f544..e7bcea9 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -15,6 +15,7 @@
 #include "gen-features.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
+#include "qemu/error-report.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
 #endif
@@ -170,14 +171,167 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error 
**errp)
 }
 #endif
 
+static void check_consistency(const S390CPUModel *model)
+{
+static int dep[][2] = {
+{ S390_FEAT_IPTE_RANGE, S390_FEAT_DAT_ENH_1 },
+{ S390_FEAT_IDTE_SEGMENT, S390_FEAT_DAT_ENH_1 },
+{ S390_FEAT_IDTE_REGION, S390_FEAT_DAT_ENH_1 },
+{ S390_FEAT_IDTE_REGION, S390_FEAT_IDTE_SEGMENT },
+{ S390_FEAT_LOCAL_TLB_CLEARING, S390_FEAT_DAT_ENH_1},
+{ S390_FEAT_LONG_DISPLACEMENT_FAST, S390_FEAT_LONG_DISPLACEMENT },
+{ S390_FEAT_DFP_FAST, S390_FEAT_DFP },
+{ S390_FEAT_TRANSACTIONAL_EXE, S390_FEAT_GEN12_ENH },
+{ S390_FEAT_CONSTRAINT_TRANSACTIONAL_EXE, S390_FEAT_TRANSACTIONAL_EXE 
},
+{ S390_FEAT_EDAT_2, S390_FEAT_EDAT_1},
+{ S390_FEAT_MSA_EXT_5, S390_FEAT_KIMD_SHA_512 },
+{ S390_FEAT_MSA_EXT_5, S390_FEAT_KLMD_SHA_512 },
+{ S390_FEAT_MSA_EXT_4, S390_FEAT_MSA_EXT_3 },
+{ S390_FEAT_SIE_CMMA, S390_FEAT_CMM },
+{ S390_FEAT_SIE_CMMA, S390_FEAT_SIE_GSLS },
+{ S390_FEAT_SIE_PFMFI, S390_FEAT_EDAT_1 },
+};
+int i;
+
+for (i = 0; i < ARRAY_SIZE(dep); i++) {
+if (test_bit(dep[i][0], model->features) &&
+!test_bit(dep[i][1], model->features)) {
+error_report("Warning: \'%s\' requires \'%s\'.",
+ s390_feat_def(dep[i][0])->name,
+ s390_feat_def(dep[i][1])->name);
+}
+}
+}
+
+static void error_prepend_missing_feat(const char *name, void *opaque)
+{
+error_prepend((Error **) opaque, "%s ", name);
+}
+
+static void check_compatibility(const S390CPUModel *max_model,
+const S390CPUModel *model, Error **errp)
+{
+S390FeatBitmap missing;
+
+if (model->def->gen > max_model->def->gen) {
+error_setg(errp, "Selected CPU generation is too new. Maximum "
+   "supported model in the configuration: \'%s\'",
+   max_model->def->name);
+return;
+} else if (model->def->gen == max_model->def->gen &&
+   model->def->ec_ga > max_model->def->ec_ga) {
+error_setg(errp, "Selected CPU GA level is too new. Maximum "
+   "supported model in the configuration: \'%s\'",
+   max_model->def->name);
+return;
+}
+
+/* detect the missing features to properly report them */
+bitmap_andnot(missing, model->features, max_model->features, 
S390_FEAT_MAX);
+if (bitmap_empty(missing, S390_FEAT_MAX)) {
+return;
+}
+
+error_setg(errp, " ");
+s390_feat_bitmap_to_ascii(missing, errp, error_prepend_missing_feat);
+error_prepend(errp, "Some features requested in the CPU model are not "
+  "available in the configuration: ");
+}
+
+static S390CPUModel *get_max_cpu_model(Error **errp)
+{
+#ifndef CONFIG_USER_ONLY
+static S390CPUModel max_model;
+static bool cached;
+
+if (cached) {
+return _model;
+}
+
+if (kvm_enabled()) {
+error_setg(errp, "KVM does not support CPU models.");
+} else {
+/* TCG enulates a z900 */
+max_model.def = _cpu_defs[0];
+bitmap_copy(max_model.features, max_model.def->default_feat,
+S390_FEAT_MAX);
+}
+if (!*errp) {
+cached = true;
+return _model;
+}
+#endif
+return NULL;
+}
+
+static inline void apply_cpu_model(const S390CPUModel *model, Error **errp)
+{
+#ifndef CONFIG_USER_ONLY
+st

[Qemu-devel] [RFC 13/28] s390x/sclp: introduce sclp feature blocks

[David Hildenbrand]

The sclp "read cpu info" and "read scp info" commands can include
features for the cpu info and configuration characteristics (extended),
decribing some advanced features available in the configuration.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 include/hw/s390x/sclp.h | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index b0c71b5..743abd4 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -97,11 +97,14 @@ typedef struct SCCBHeader {
 } QEMU_PACKED SCCBHeader;
 
 #define SCCB_DATA_LEN (SCCB_SIZE - sizeof(SCCBHeader))
+#define SCCB_CPU_FEATURE_LEN 6
 
 /* CPU information */
 typedef struct CPUEntry {
 uint8_t address;
-uint8_t reserved0[13];
+uint8_t reserved0;
+uint8_t features[SCCB_CPU_FEATURE_LEN];
+uint8_t reserved2[6];
 uint8_t type;
 uint8_t reserved1;
 } QEMU_PACKED CPUEntry;
@@ -117,10 +120,13 @@ typedef struct ReadInfo {
 uint8_t  loadparm[8];   /* 24-31 */
 uint8_t  _reserved3[48 - 32];   /* 32-47 */
 uint64_t facilities;/* 48-55 */
-uint8_t  _reserved0[100 - 56];
+uint8_t  _reserved0[80 - 56];   /* 56-79 */
+uint8_t  conf_char[96 - 80];/* 80-95 */
+uint8_t  _reserved4[100 - 96];  /* 96-99 */
 uint32_t rnsize2;
 uint64_t rnmax2;
-uint8_t  _reserved4[120-112];   /* 112-119 */
+uint8_t  _reserved6[116 - 112]; /* 112-115 */
+uint8_t  conf_char_ext[120 - 116];   /* 116-119 */
 uint16_t highest_cpu;
 uint8_t  _reserved5[128 - 122]; /* 122-127 */
 struct CPUEntry entries[0];
-- 
2.6.6

[Qemu-devel] [RFC 26/28] s390x/cpumodel: implement QMP interface "query-cpu-model-expansion"

[David Hildenbrand]

In order to expand CPU models, we create temporary cpus that handle the
feature/group parsing.

When converting the data structure back, we always fall back to the
base cpu model, which is by definition migration safe.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 target-s390x/cpu_models.c | 132 ++
 1 file changed, 132 insertions(+)

diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index b8ae601..988b9d7 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -16,6 +16,9 @@
 #include "qapi/error.h"
 #include "qapi/visitor.h"
 #include "qemu/error-report.h"
+#include "qapi/qmp/qerror.h"
+#include "qapi/qmp-input-visitor.h"
+#include "qapi/qmp/qbool.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
 #endif
@@ -287,6 +290,135 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error 
**errp)
 
 return list;
 }
+
+static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
+Error **errp)
+{
+const QDict *qdict = NULL;
+QmpInputVisitor *qiv;
+const QDictEntry *e;
+ObjectClass *oc;
+S390CPU *cpu;
+Object *obj;
+
+if (info->props) {
+qdict = qobject_to_qdict(info->props);
+if (!qdict) {
+error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
+return;
+}
+}
+
+oc = cpu_class_by_name(TYPE_S390_CPU, info->name);
+if (!oc) {
+error_setg(errp, "The CPU definition \'%s\' is unknown.", info->name);
+return;
+}
+if (S390_CPU_CLASS(oc)->kvm_required && !kvm_enabled()) {
+error_setg(errp, "The CPU definition '%s' requires KVM", info->name);
+return;
+}
+obj = object_new(object_class_get_name(oc));
+cpu = S390_CPU(obj);
+
+if (!cpu->model) {
+error_setg(errp, "Details about the host CPU model are not available, "
+ "it cannot be used.");
+object_unref(obj);
+return;
+}
+
+if (qdict) {
+qiv = qmp_input_visitor_new(info->props, true);
+for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
+object_property_set(obj, qmp_input_get_visitor(qiv), e->key, errp);
+if (*errp) {
+qmp_input_visitor_cleanup(qiv);
+object_unref(obj);
+return;
+}
+}
+qmp_input_visitor_cleanup(qiv);
+}
+
+/* copy the model and throw the cpu away */
+memcpy(model, cpu->model, sizeof(*model));
+object_unref(obj);
+}
+
+static void qdict_add_disabled_feat(const char *name, void *opaque)
+{
+qdict_put((QDict *) opaque, name, qbool_from_bool(false));
+}
+
+static void qdict_add_enabled_feat(const char *name, void *opaque)
+{
+qdict_put((QDict *) opaque, name, qbool_from_bool(true));
+}
+
+/* convert S390CPUDef into migration safe CpuModelInfo */
+static void cpu_info_from_model(CpuModelInfo *info, const S390CPUModel *model,
+bool delta_changes)
+{
+QDict *qdict = qdict_new();
+S390FeatBitmap bitmap;
+
+/* always fallback to the stable base model */
+info->name = g_strdup_printf("%s-base", model->def->name);
+
+if (delta_changes) {
+/* features deleted from the base feature set */
+bitmap_andnot(bitmap, model->def->base_feat, model->features,
+  S390_FEAT_MAX);
+if (!bitmap_empty(bitmap, S390_FEAT_MAX)) {
+s390_feat_bitmap_to_ascii(bitmap, qdict, qdict_add_disabled_feat);
+}
+
+/* features added to the base feature set */
+bitmap_andnot(bitmap, model->features, model->def->base_feat,
+  S390_FEAT_MAX);
+if (!bitmap_empty(bitmap, S390_FEAT_MAX)) {
+s390_feat_bitmap_to_ascii(bitmap, qdict, qdict_add_enabled_feat);
+}
+} else {
+/* expand all features */
+s390_feat_bitmap_to_ascii(model->features, qdict, 
qdict_add_enabled_feat);
+bitmap_complement(bitmap, model->features, S390_FEAT_MAX);
+s390_feat_bitmap_to_ascii(model->features, qdict, 
qdict_add_disabled_feat);
+}
+
+if (!qdict_size(qdict)) {
+QDECREF(qdict);
+} else {
+info->props = QOBJECT(qdict);
+info->has_props = true;
+}
+}
+
+CpuModelExpansionInfo *arch_query_cpu_model_expansion(CpuModelExpansionType 
type,
+  CpuModelInfo *model,
+  Error **errp)
+{
+CpuModelExpansionInfo *expansion_info = NULL;
+S390CPUModel s390_model;
+bool delta_changes = false;

Re: [Qemu-devel] [PATCH 0/3] qmp: query-host-cpu command

[David Hildenbrand]

> On Tue, Jun 21, 2016 at 08:20:40AM +0200, David Hildenbrand wrote:
> > > Add QMP command to allow management software to query for
> > > CPU information for the running host.
> > > 
> > > The data returned by the command is in the form of a dictionary
> > > of QOM properties.
> > > 
> > > This series depends on the "Add runnability info to
> > > query-cpu-definitions" series I sent 2 weeks ago.
> > > 
> > > Git tree:
> > >   https://github.com/ehabkost/qemu-hacks.git work/query-host-cpu
> > >   
> > 
> > I like that interface, I'm going to post (maybe today? :) ) a similar 
> > interface
> > that allows to also expand other cpu models, not just the host model.  
> 
> In x86 I want to avoid exposing the details of other CPU models
> to libvirt because the details depend on machine-type.
> 
> But if it is useful for you, I believe the same "qom-properties"
> dict could be returned in query-cpu-definitions.
> 
> > 
> > Maybe we can then decide which one makes sense for all of us. But in 
> > general,
> > this interface is much better compared to what we had before.   
> 
> Maybe both? I think it's better to have a separate interface for
> querying "what exactly this host supports" and another one for
> querying for "what happens if I use -cpu host". In the case of
> x86, both are equivalent, but we can't guarantee this on all
> architectures.
> 

I'll post my patches in a couple of minutes, let's discuss it then.

We might want to avoid having multiple interfaces carrying out the same task.

David

[Qemu-devel] [RFC 24/28] qmp: add QMP interface "query-cpu-model-comparison"

[David Hildenbrand]

Let's provide a standardized interface to compare two CPU models.

query-cpu-model-compare takes two models and returns what it knows about
their compability.

If modelA is a subset of modelB or if both are identical, modelA will run
in the same configuration as modelB. If modelA is however a superset of
modelB or if both are incompatible, one can try to create a compatible one
by "baselining" both models (follow up patch).

The host CPU model has the same semantics as for "query-cpu-model-expansion".

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 include/sysemu/arch_init.h  |  3 ++
 qapi-schema.json| 59 +
 qmp-commands.hx |  6 
 qmp.c   |  7 
 stubs/Makefile.objs |  1 +
 stubs/arch-query-cpu-model-comparison.c | 12 +++
 6 files changed, 88 insertions(+)
 create mode 100644 stubs/arch-query-cpu-model-comparison.c

diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
index 37b2e86..96d47c0 100644
--- a/include/sysemu/arch_init.h
+++ b/include/sysemu/arch_init.h
@@ -38,5 +38,8 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error 
**errp);
 CpuModelExpansionInfo *arch_query_cpu_model_expansion(CpuModelExpansionType 
type,
   CpuModelInfo *mode,
   Error **errp);
+CpuModelCompareInfo *arch_query_cpu_model_comparison(CpuModelInfo *modela,
+ CpuModelInfo *modelb,
+ Error **errp);
 
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 5b72dc0..34df86f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3107,6 +3107,65 @@
 'model': 'CpuModelInfo' },
   'returns': 'CpuModelExpansionInfo' }
 
+##
+# @CpuModelCompareResult:
+#
+# An enumeration of CPU model comparation results.
+#
+# @incompatible: both model definition are incompatible
+#
+# @identical: model A == model B
+#
+# @superset: model A > model B
+#
+# @subset: model A < model B
+#
+# Since: 2.7.0
+##
+{ 'enum': 'CpuModelCompareResult',
+  'data': [ 'incompatible', 'identical', 'superset', 'subset' ] }
+
+##
+# @CpuModelCompareInfo
+#
+# The result of a CPU model comparison.
+#
+# @result: The result of the compare operation.
+# @responsible-properties: List of properties that led to the comparison result
+#  not being identical.
+#
+# @responsible-properties is a list of QOM property names that lead to
+# both CPUs not being detected as identical. For identical models, this
+# list is empty.
+# If the QOM property is read-only, that means there's no known
+# way to make the CPU models identical. If the special property name
+# "type" is included, the models are by definition not identical and
+# cannot be made identical.
+#
+# Since: 2.7.0
+##
+{ 'struct': 'CpuModelCompareInfo',
+  'data': {'result': 'CpuModelCompareResult',
+   'responsible-properties': ['str']
+  }
+}
+
+##
+# @query-cpu-model-comparison:
+#
+# Compares two CPU models.
+#
+# Returns: a CpuModelCompareInfo. Returns an error if CPU models are not
+#  supported, if a model cannot be used, if the model contains
+#  an unknown cpu definition name, unknown properties or properties
+#  with a wrong type.
+#
+# Since: 2.7.0
+##
+{ 'command': 'query-cpu-model-comparison',
+  'data': { 'modela': 'CpuModelInfo', 'modelb': 'CpuModelInfo' },
+  'returns': 'CpuModelCompareInfo' }
+
 # @AddfdInfo:
 #
 # Information about a file descriptor that was added to an fd set.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b279fc5..4ee7937 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3936,6 +3936,12 @@ EQMP
 },
 
 {
+.name   = "query-cpu-model-comparison",
+.args_type  = "modela:q,modelb:q",
+.mhandler.cmd_new = qmp_marshal_query_cpu_model_comparison,
+},
+
+{
 .name   = "query-target",
 .args_type  = "",
 .mhandler.cmd_new = qmp_marshal_query_target,
diff --git a/qmp.c b/qmp.c
index c54f0a0..afa8c77 100644
--- a/qmp.c
+++ b/qmp.c
@@ -614,6 +614,13 @@ CpuModelExpansionInfo 
*qmp_query_cpu_model_expansion(CpuModelExpansionType type,
 return arch_query_cpu_model_expansion(type, model, errp);
 }
 
+CpuModelCompareInfo *qmp_query_cpu_model_comparison(CpuModelInfo *modela,
+CpuModelInfo *modelb,
+Error **errp)
+{
+return arch_query_cpu_model_comparison(modela, modelb, errp);
+}
+
 void qmp_add_client(const char *protocol, const char *fdname,
 bool has_skipauth, bool

[Qemu-devel] [RFC 17/28] s390x/sclp: propagate hmfai

[David Hildenbrand]

hmfai is provided on CPU models >= z196. Let's propagate it properly.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 hw/s390x/sclp.c   |  1 +
 include/hw/s390x/sclp.h   |  3 ++-
 target-s390x/cpu_models.c | 14 ++
 target-s390x/cpu_models.h |  1 +
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 5ffcf51..883592c 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -106,6 +106,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 read_info->facilities |= cpu_to_be64(SCLP_FC_ASSIGN_ATTACH_READ_STOR);
 }
 read_info->mha_pow = s390_get_mha_pow();
+read_info->hmfai = cpu_to_be32(s390_get_hmfai());
 
 rnsize = 1 << (sclp->increment_size - 20);
 if (rnsize <= 128) {
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index 7a610cd..004cb13 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -130,7 +130,8 @@ typedef struct ReadInfo {
 uint8_t  _reserved6[116 - 112]; /* 112-115 */
 uint8_t  conf_char_ext[120 - 116];   /* 116-119 */
 uint16_t highest_cpu;
-uint8_t  _reserved5[128 - 122]; /* 122-127 */
+uint8_t  _reserved5[124 - 122]; /* 122-123 */
+uint32_t hmfai;
 struct CPUEntry entries[0];
 } QEMU_PACKED ReadInfo;
 
diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index edfbb60..32d9a09 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -74,6 +74,20 @@ static const S390CPUDef s390_cpu_defs[] = {
 CPUDEF_INIT(0x2965, 13, 2, 47, 0x0800U, "z13s", "IBM z13s GA1"),
 };
 
+uint32_t s390_get_hmfai(void)
+{
+static S390CPU *cpu;
+
+if (!cpu) {
+cpu = S390_CPU(qemu_get_cpu(0));
+}
+
+if (!cpu || !cpu->model) {
+return 0;
+}
+return cpu->model->def->hmfai;
+}
+
 uint8_t s390_get_mha_pow(void)
 {
 static S390CPU *cpu;
diff --git a/target-s390x/cpu_models.h b/target-s390x/cpu_models.h
index ee019b4..986f7cb 100644
--- a/target-s390x/cpu_models.h
+++ b/target-s390x/cpu_models.h
@@ -45,6 +45,7 @@ typedef struct S390CPUModel {
 
 #define S390_GEN_Z10 0xa
 
+uint32_t s390_get_hmfai(void);
 uint8_t s390_get_mha_pow(void);
 uint32_t s390_get_ibc_val(void);
 static inline uint16_t s390_ibc_from_cpu_model(const S390CPUModel *model)
-- 
2.6.6

[Qemu-devel] [RFC 12/28] s390x/sclp: factor out preparation of cpu entries

[David Hildenbrand]

Let's factor out the common code of "read cpu info" and "read scp
info". This will make the introduction of new cpu entry fields easier.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 hw/s390x/sclp.c | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index fca37f5..15d7114 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -29,6 +29,16 @@ static inline SCLPDevice *get_sclp_device(void)
 return SCLP(object_resolve_path_type("", TYPE_SCLP, NULL));
 }
 
+static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int count)
+{
+int i;
+
+for (i = 0; i < count; i++) {
+entry[i].address = i;
+entry[i].type = 0;
+}
+}
+
 /* Provide information about the configuration, CPUs and storage */
 static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 {
@@ -37,7 +47,6 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 sclpMemoryHotplugDev *mhd = get_sclp_memory_hotplug_dev();
 CPUState *cpu;
 int cpu_count = 0;
-int i = 0;
 int rnsize, rnmax;
 int slots = MIN(machine->ram_slots, s390_get_memslot_count(kvm_state));
 
@@ -50,10 +59,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
 read_info->highest_cpu = cpu_to_be16(max_cpus);
 
-for (i = 0; i < cpu_count; i++) {
-read_info->entries[i].address = i;
-read_info->entries[i].type = 0;
-}
+prepare_cpu_entries(sclp, read_info->entries, cpu_count);
 
 read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
 SCLP_HAS_PCI_RECONFIG);
@@ -304,7 +310,6 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB *sccb)
 ReadCpuInfo *cpu_info = (ReadCpuInfo *) sccb;
 CPUState *cpu;
 int cpu_count = 0;
-int i = 0;
 
 CPU_FOREACH(cpu) {
 cpu_count++;
@@ -318,10 +323,7 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB 
*sccb)
 cpu_info->offset_standby = cpu_to_be16(cpu_info->offset_configured
 + cpu_info->nr_configured*sizeof(CPUEntry));
 
-for (i = 0; i < cpu_count; i++) {
-cpu_info->entries[i].address = i;
-cpu_info->entries[i].type = 0;
-}
+prepare_cpu_entries(sclp, cpu_info->entries, cpu_count);
 
 sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
 }
-- 
2.6.6

[Qemu-devel] [RFC 27/28] s390x/cpumodel: implement QMP interface "query-cpu-model-comparison"

[David Hildenbrand]

Let's implement that interface by reusing our convertion code
implemented for expansion.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 target-s390x/cpu_models.c | 84 +++
 1 file changed, 84 insertions(+)

diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index 988b9d7..75d808b 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -419,6 +419,90 @@ CpuModelExpansionInfo 
*arch_query_cpu_model_expansion(CpuModelExpansionType type
 cpu_info_from_model(expansion_info->model, _model, delta_changes);
 return expansion_info;
 }
+
+static void list_add_feat(const char *name, void *opaque)
+{
+strList **last = (strList **) opaque;
+strList *entry;
+
+entry = g_malloc0(sizeof(*entry));
+entry->value = g_strdup(name);
+entry->next = *last;
+*last = entry;
+}
+
+CpuModelCompareInfo *arch_query_cpu_model_comparison(CpuModelInfo *infoa,
+ CpuModelInfo *infob,
+ Error **errp)
+{
+CpuModelCompareResult feat_result, gen_result;
+CpuModelCompareInfo *compare_info;
+S390FeatBitmap missing, added;
+S390CPUModel modela, modelb;
+
+/* convert both models to our internal representation */
+cpu_model_from_info(, infoa, errp);
+if (*errp) {
+return NULL;
+}
+cpu_model_from_info(, infob, errp);
+if (*errp) {
+return NULL;
+}
+compare_info = g_malloc0(sizeof(*compare_info));
+
+/* check the cpu generation and ga level */
+if (modela.def->gen == modelb.def->gen) {
+if (modela.def->ec_ga == modelb.def->ec_ga) {
+/* ec and corresponding bc are identical */
+gen_result = CPU_MODEL_COMPARE_RESULT_IDENTICAL;
+} else if (modela.def->ec_ga < modelb.def->ec_ga) {
+gen_result = CPU_MODEL_COMPARE_RESULT_SUBSET;
+} else {
+gen_result = CPU_MODEL_COMPARE_RESULT_SUPERSET;
+}
+} else if (modela.def->gen < modelb.def->gen) {
+gen_result = CPU_MODEL_COMPARE_RESULT_SUBSET;
+} else {
+gen_result = CPU_MODEL_COMPARE_RESULT_SUPERSET;
+}
+if (gen_result != CPU_MODEL_COMPARE_RESULT_IDENTICAL) {
+/* both models cannot be made identical */
+list_add_feat("type", _info->responsible_properties);
+}
+
+/* check the feature set */
+if (bitmap_equal(modela.features, modelb.features, S390_FEAT_MAX)) {
+feat_result = CPU_MODEL_COMPARE_RESULT_IDENTICAL;
+} else {
+bitmap_andnot(missing, modela.features, modelb.features, 
S390_FEAT_MAX);
+s390_feat_bitmap_to_ascii(missing,
+  _info->responsible_properties,
+  list_add_feat);
+bitmap_andnot(added, modelb.features, modela.features, S390_FEAT_MAX);
+s390_feat_bitmap_to_ascii(added, _info->responsible_properties,
+  list_add_feat);
+if (bitmap_empty(missing, S390_FEAT_MAX)) {
+feat_result = CPU_MODEL_COMPARE_RESULT_SUBSET;
+} else if (bitmap_empty(added, S390_FEAT_MAX)) {
+feat_result = CPU_MODEL_COMPARE_RESULT_SUPERSET;
+} else {
+feat_result = CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE;
+}
+}
+
+/* combine the results */
+if (gen_result == feat_result) {
+compare_info->result = gen_result;
+} else if (feat_result == CPU_MODEL_COMPARE_RESULT_IDENTICAL) {
+compare_info->result = gen_result;
+} else if (gen_result == CPU_MODEL_COMPARE_RESULT_IDENTICAL) {
+compare_info->result = feat_result;
+} else {
+compare_info->result = CPU_MODEL_COMPARE_RESULT_INCOMPATIBLE;
+}
+return compare_info;
+}
 #endif
 
 static void check_consistency(const S390CPUModel *model)
-- 
2.6.6

[Qemu-devel] [RFC 03/28] s390x/cpumodel: introduce CPU features

[David Hildenbrand]

From: Michael Mueller <m...@linux.vnet.ibm.com>

The patch introduces s390x CPU features (most of them refered to as
facilities) along with their discription and some functions that will be
helpful when working with the features later on.

Please note that we don't introduce all known CPU features, only the
ones currently supported by KVM + QEMU. We don't want to enable later
on blindly any facilities, for which we don't know yet if we need QEMU
support to properly support them (e.g. migrate additional state when
active). We can update QEMU later on.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: Michael Mueller <m...@linux.vnet.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
[reworked to include non-stfle features, added definitions]
---
 target-s390x/Makefile.objs  |   2 +-
 target-s390x/cpu_features.c | 334 
 target-s390x/cpu_features.h | 279 
 3 files changed, 614 insertions(+), 1 deletion(-)
 create mode 100644 target-s390x/cpu_features.c
 create mode 100644 target-s390x/cpu_features.h

diff --git a/target-s390x/Makefile.objs b/target-s390x/Makefile.objs
index 34bd693..4266c87 100644
--- a/target-s390x/Makefile.objs
+++ b/target-s390x/Makefile.objs
@@ -1,5 +1,5 @@
 obj-y += translate.o helper.o cpu.o interrupt.o
 obj-y += int_helper.o fpu_helper.o cc_helper.o mem_helper.o misc_helper.o
-obj-y += gdbstub.o cpu_models.o
+obj-y += gdbstub.o cpu_models.o cpu_features.o
 obj-$(CONFIG_SOFTMMU) += machine.o ioinst.o arch_dump.o mmu_helper.o
 obj-$(CONFIG_KVM) += kvm.o
diff --git a/target-s390x/cpu_features.c b/target-s390x/cpu_features.c
new file mode 100644
index 000..c78a189
--- /dev/null
+++ b/target-s390x/cpu_features.c
@@ -0,0 +1,334 @@
+/*
+ * CPU features/facilities for s390x
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * Author(s): David Hildenbrand <d...@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#include "qemu/osdep.h"
+#include "cpu_features.h"
+
+#define FEAT_INIT(_name, _type, _bit, _desc) \
+{\
+.name = _name,   \
+.type = _type,   \
+.bit = _bit, \
+.desc = _desc,   \
+}
+
+/* indexed by feature number for easy lookup */
+static const S390FeatDef s390_features[] = {
+FEAT_INIT("n3", S390_FEAT_TYPE_STFL, 0, "Instructions marked as n3"),
+FEAT_INIT("z", S390_FEAT_TYPE_STFL, 1, "z/Architecture architectural 
mode"),
+FEAT_INIT("dateh1", S390_FEAT_TYPE_STFL, 3, "DAT-enhancement facility 1"),
+FEAT_INIT("idtes", S390_FEAT_TYPE_STFL, 4, "IDTE selective TLB 
segment-table clearing"),
+FEAT_INIT("idter", S390_FEAT_TYPE_STFL, 5, "IDTE selective TLB 
region-table clearing"),
+FEAT_INIT("asnlxr", S390_FEAT_TYPE_STFL, 6, "ASN-and-LX reuse facility"),
+FEAT_INIT("stfle", S390_FEAT_TYPE_STFL, 7, "Store-facility-list-extended 
facility"),
+FEAT_INIT("edat1", S390_FEAT_TYPE_STFL, 8, "Enhanced-DAT facility 1"),
+FEAT_INIT("srs", S390_FEAT_TYPE_STFL, 9, "Sense-running-status facility"),
+FEAT_INIT("csske", S390_FEAT_TYPE_STFL, 10, "Conditional-SSKE facility"),
+FEAT_INIT("ctop", S390_FEAT_TYPE_STFL, 11, "Configuration-topology 
facility"),
+FEAT_INIT("ipter", S390_FEAT_TYPE_STFL, 13, "IPTE-range facility"),
+FEAT_INIT("nonqks", S390_FEAT_TYPE_STFL, 14, "Nonquiescing key-setting 
facility"),
+FEAT_INIT("etf2", S390_FEAT_TYPE_STFL, 16, "Extended-translation facility 
2"),
+FEAT_INIT("msa-base", S390_FEAT_TYPE_STFL, 17, "Message-security-assist 
facility (excluding subfunctions)"),
+FEAT_INIT("ldisp", S390_FEAT_TYPE_STFL, 18, "Long-displacement facility"),
+FEAT_INIT("ldisphp", S390_FEAT_TYPE_STFL, 19, "Long-displacement facility 
has high performance"),
+FEAT_INIT("hfpm", S390_FEAT_TYPE_STFL, 20, "HFP-multiply-add/subtract 
facility"),
+FEAT_INIT("eimm", S390_FEAT_TYPE_STFL, 21, "Extended-immediate facility"),
+FEAT_INIT("etf3", S390_FEAT_TYPE_STFL, 22, "Extended-translation facility 
3"),
+FEAT_INIT("hfpue", S390_FEAT_TYPE_STFL, 23, "HFP-unnormalized-extension 
facility"),
+FEAT_INIT("etf2eh", S390_FEAT_TYPE_STFL, 24, "ETF2-enhancement facility"),
+FEAT_INIT("stckf

[Qemu-devel] [RFC 16/28] s390x/sclp: propagate the mha via sclp

[David Hildenbrand]

The mha is provided in the CPU model, so get any CPU and extract the value.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 hw/s390x/sclp.c   |  1 +
 include/hw/s390x/sclp.h   |  3 ++-
 target-s390x/cpu_models.c | 14 ++
 target-s390x/cpu_models.h |  1 +
 4 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 52f8bb9..5ffcf51 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -105,6 +105,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 
 read_info->facilities |= cpu_to_be64(SCLP_FC_ASSIGN_ATTACH_READ_STOR);
 }
+read_info->mha_pow = s390_get_mha_pow();
 
 rnsize = 1 << (sclp->increment_size - 20);
 if (rnsize <= 128) {
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index 4580134..7a610cd 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -123,7 +123,8 @@ typedef struct ReadInfo {
 uint8_t  _reserved0[76 - 56];   /* 56-75 */
 uint32_t ibc_val;
 uint8_t  conf_char[96 - 80];/* 80-95 */
-uint8_t  _reserved4[100 - 96];  /* 96-99 */
+uint8_t  _reserved4[99 - 96];   /* 96-98 */
+uint8_t mha_pow;
 uint32_t rnsize2;
 uint64_t rnmax2;
 uint8_t  _reserved6[116 - 112]; /* 112-115 */
diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index d58b931..edfbb60 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -74,6 +74,20 @@ static const S390CPUDef s390_cpu_defs[] = {
 CPUDEF_INIT(0x2965, 13, 2, 47, 0x0800U, "z13s", "IBM z13s GA1"),
 };
 
+uint8_t s390_get_mha_pow(void)
+{
+static S390CPU *cpu;
+
+if (!cpu) {
+cpu = S390_CPU(qemu_get_cpu(0));
+}
+
+if (!cpu || !cpu->model) {
+return 0;
+}
+return cpu->model->def->mha_pow;
+}
+
 uint32_t s390_get_ibc_val(void)
 {
 uint16_t unblocked_ibc, lowest_ibc;
diff --git a/target-s390x/cpu_models.h b/target-s390x/cpu_models.h
index bbb85ac..ee019b4 100644
--- a/target-s390x/cpu_models.h
+++ b/target-s390x/cpu_models.h
@@ -45,6 +45,7 @@ typedef struct S390CPUModel {
 
 #define S390_GEN_Z10 0xa
 
+uint8_t s390_get_mha_pow(void);
 uint32_t s390_get_ibc_val(void);
 static inline uint16_t s390_ibc_from_cpu_model(const S390CPUModel *model)
 {
-- 
2.6.6

[Qemu-devel] [RFC 07/28] s390x/cpumodel: register defined CPU models as subclasses

[David Hildenbrand]

This patch adds the CPU model definitions that are known on s390x -
like z900, zBC12 or z13. For each definition, introduce two CPU models:

1. Base model (e.g. z13-base): Minimum feature set we expect to be around
   on all z13 systems. These models are migration save and will never
   change.
2. Flexible models (e.g. z13): Models that can change between QEMU versions
   and will be extended over time as we implement further features that
   are already part of such a model in real hardware of certain
   configurations.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 target-s390x/cpu-qom.h|   2 +
 target-s390x/cpu_models.c | 111 ++
 target-s390x/cpu_models.h |  36 +++
 3 files changed, 149 insertions(+)
 create mode 100644 target-s390x/cpu_models.h

diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
index ae0be73..d619cbd 100644
--- a/target-s390x/cpu-qom.h
+++ b/target-s390x/cpu-qom.h
@@ -21,6 +21,7 @@
 #define QEMU_S390_CPU_QOM_H
 
 #include "qom/cpu.h"
+#include "cpu_models.h"
 
 #define TYPE_S390_CPU "s390-cpu"
 
@@ -45,6 +46,7 @@ typedef struct S390CPUClass {
 /*< private >*/
 CPUClass parent_class;
 /*< public >*/
+const S390CPUDef *cpu_def;
 bool kvm_required;
 bool migration_safe;
 const char *desc;
diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index 18638a9..50b395a 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -12,11 +12,66 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
+#include "gen-features.h"
 #include "qapi/error.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
 #endif
 
+#define CPUDEF_INIT(_type, _gen, _ec_ga, _mha_pow, _hmfai, _name, _desc) \
+{ \
+.name = _name,\
+.type = _type,\
+.gen = _gen,  \
+.ec_ga = _ec_ga,  \
+.mha_pow = _mha_pow,  \
+.hmfai = _hmfai,  \
+.desc = _desc,\
+.base_feat = { S390_FEAT_LIST_GEN ## _gen ## _GA ## _ec_ga ## _BASE }, 
 \
+.default_feat = { S390_FEAT_LIST_GEN ## _gen ## _GA ## _ec_ga ## 
_DEFAULT },  \
+.full_feat = { S390_FEAT_LIST_GEN ## _gen ## _GA ## _ec_ga ## _FULL }, 
 \
+}
+
+/*
+ * CPU definiton list in order of release. For now, base features of a
+ * following release are always a subset of base features of the previous
+ * release. Same is correct for the other feature sets.
+ * A BC release always follows the corresponding EC release.
+ */
+static const S390CPUDef s390_cpu_defs[] = {
+CPUDEF_INIT(0x2064, 7, 1, 38, 0xU, "z900", "IBM zSeries 900 GA1"),
+CPUDEF_INIT(0x2064, 7, 2, 38, 0xU, "z900.2", "IBM zSeries 900 
GA2"),
+CPUDEF_INIT(0x2064, 7, 3, 38, 0xU, "z900.3", "IBM zSeries 900 
GA3"),
+CPUDEF_INIT(0x2066, 7, 3, 38, 0xU, "z800", "IBM zSeries 800 GA1"),
+CPUDEF_INIT(0x2084, 8, 1, 38, 0xU, "z990", "IBM zSeries 990 GA1"),
+CPUDEF_INIT(0x2084, 8, 2, 38, 0xU, "z990.2", "IBM zSeries 990 
GA2"),
+CPUDEF_INIT(0x2084, 8, 3, 38, 0xU, "z990.3", "IBM zSeries 990 
GA3"),
+CPUDEF_INIT(0x2086, 8, 3, 38, 0xU, "z890", "IBM zSeries 880 GA1"),
+CPUDEF_INIT(0x2084, 8, 4, 38, 0xU, "z990.4", "IBM zSeries 990 
GA4"),
+CPUDEF_INIT(0x2086, 8, 4, 38, 0xU, "z890.2", "IBM zSeries 880 
GA2"),
+CPUDEF_INIT(0x2084, 8, 5, 38, 0xU, "z990.5", "IBM zSeries 990 
GA5"),
+CPUDEF_INIT(0x2086, 8, 5, 38, 0xU, "z890.3", "IBM zSeries 880 
GA3"),
+CPUDEF_INIT(0x2094, 9, 1, 40, 0xU, "z9EC", "IBM System z9 EC GA1"),
+CPUDEF_INIT(0x2094, 9, 2, 40, 0xU, "z9EC.2", "IBM System z9 EC 
GA2"),
+CPUDEF_INIT(0x2096, 9, 2, 40, 0xU, "z9BC", "IBM System z9 BC GA1"),
+CPUDEF_INIT(0x2094, 9, 3, 40, 0xU, "z9EC.3", "IBM System z9 EC 
GA3"),
+CPUDEF_INIT(0x2096, 9, 3, 40, 0xU, "z9BC.2", "IBM System z9 BC 
GA2"),
+CPUDEF_INIT(0x2097, 10, 1, 43, 0xU, "z10EC", "IBM System z10 EC 
GA1"),
+CPUDEF_INIT(0x2097, 10, 2, 43, 0xU, "z10EC.2", "IBM System z10

[Qemu-devel] [RFC 05/28] s390x/cpumodel: generate CPU feature group lists

[David Hildenbrand]

Feature groups will be very helpful to reduce the amount of features
typically available in sane configurations. E.g. the MSA facilities
introduced loads of subfunctions, which could - in theory - go away
in the future, but we want to avoid reporting hundrets of features to
the user if usually all of them are in place.

Groups only contain features that were introduced in one shot, not just
random features. Therefore, groups can never change. This is an important
property regarding migration.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 target-s390x/gen-features.c | 80 +
 1 file changed, 80 insertions(+)

diff --git a/target-s390x/gen-features.c b/target-s390x/gen-features.c
index 3a7c373..d4f4b29 100644
--- a/target-s390x/gen-features.c
+++ b/target-s390x/gen-features.c
@@ -178,6 +178,35 @@
 S390_FEAT_MSA_EXT_5, \
 S390_FEAT_PPNO_SHA_512_DRNG
 
+/* cpu feature groups */
+static uint16_t group_PLO[] = {
+S390_FEAT_GROUP_PLO,
+};
+static uint16_t group_TOD_CLOCK_STEERING[] = {
+S390_FEAT_GROUP_TOD_CLOCK_STEERING,
+};
+static uint16_t group_GEN13_PTFF[] = {
+S390_FEAT_GROUP_GEN13_PTFF,
+};
+static uint16_t group_MSA[] = {
+S390_FEAT_GROUP_MSA,
+};
+static uint16_t group_MSA_EXT_1[] = {
+S390_FEAT_GROUP_MSA_EXT_1,
+};
+static uint16_t group_MSA_EXT_2[] = {
+S390_FEAT_GROUP_MSA_EXT_2,
+};
+static uint16_t group_MSA_EXT_3[] = {
+S390_FEAT_GROUP_MSA_EXT_3,
+};
+static uint16_t group_MSA_EXT_4[] = {
+S390_FEAT_GROUP_MSA_EXT_4,
+};
+static uint16_t group_MSA_EXT_5[] = {
+S390_FEAT_GROUP_MSA_EXT_5,
+};
+
 /* base features in order of release */
 static uint16_t base_GEN7_GA1[] = {
 S390_FEAT_GROUP_PLO,
@@ -430,6 +459,34 @@ static CpuFeatDefSpec CpuFeatDef[] = {
 CPU_FEAT_INITIALIZER(GEN13_GA2),
 };
 
+#define FEAT_GROUP_INITIALIZER(_name)  \
+{  \
+.name = "S390_FEAT_GROUP_LIST_" #_name,\
+.bits =\
+{ .data = group_##_name,   \
+  .len = ARRAY_SIZE(group_##_name) },  \
+}
+
+typedef struct {
+const char *name;
+BitSpec bits;
+} FeatGroupDefSpec;
+
+/***
+ * feature groups
+ ***/
+static FeatGroupDefSpec FeatGroupDef[] = {
+FEAT_GROUP_INITIALIZER(PLO),
+FEAT_GROUP_INITIALIZER(TOD_CLOCK_STEERING),
+FEAT_GROUP_INITIALIZER(GEN13_PTFF),
+FEAT_GROUP_INITIALIZER(MSA),
+FEAT_GROUP_INITIALIZER(MSA_EXT_1),
+FEAT_GROUP_INITIALIZER(MSA_EXT_2),
+FEAT_GROUP_INITIALIZER(MSA_EXT_3),
+FEAT_GROUP_INITIALIZER(MSA_EXT_4),
+FEAT_GROUP_INITIALIZER(MSA_EXT_5),
+};
+
 static void set_bits(uint64_t list[], BitSpec bits)
 {
 uint32_t i;
@@ -487,6 +544,28 @@ static void print_feature_defs(void)
 }
 }
 
+static void print_feature_group_defs(void)
+{
+int i, j;
+
+printf("\n/* CPU feature group list data */\n");
+
+for (i = 0; i < ARRAY_SIZE(FeatGroupDef); i++) {
+uint64_t feat[S390_FEAT_MAX / 64 + 1] = {};
+
+set_bits(feat, FeatGroupDef[i].bits);
+printf("#define %s\t", FeatGroupDef[i].name);
+for (j = 0; j < ARRAY_SIZE(feat); j++) {
+printf("0x%016"PRIx64"UL", feat[j]);
+if (j < ARRAY_SIZE(feat) - 1) {
+printf(",");
+} else {
+printf("\n");
+}
+}
+}
+}
+
 int main(int argc, char *argv[])
 {
 printf("/*\n"
@@ -501,6 +580,7 @@ int main(int argc, char *argv[])
" */\n\n"
"#ifndef %s\n#define %s\n", __FILE__, _YEARS, _NAME_H, _NAME_H);
 print_feature_defs();
+print_feature_group_defs();
 printf("\n#endif\n");
 return 0;
 }
-- 
2.6.6

[Qemu-devel] [RFC 19/28] s390x/kvm: allow runtime-instrumentation for "none" machine

[David Hildenbrand]

To be able to query the correct host model for the "none" machine,
let's allow runtime-instrumentation for that machine.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 52f079a..704b5b5 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -248,6 +248,11 @@ bool ri_allowed(void)
 
 return s390mc->ri_allowed;
 }
+/*
+ * Make sure the "none" machine can have ri, otherwise it won't * be
+ * unlocked in KVM and therefore the host CPU model might be wrong.
+ */
+return true;
 }
 return 0;
 }
-- 
2.6.6

Re: [Qemu-devel] [RFC 00/28] s390x CPU models: exposing features

[David Hildenbrand]

> (CCing libvirt people)
> 
> On Tue, Jun 21, 2016 at 03:02:05PM +0200, David Hildenbrand wrote:
> > This is our second attempt to implement CPU models for s390x. We realized
> > that we also want to have features exposed via the CPU model. While doing
> > that we realized that we want to have a better interface for libvirt.  
> 
> Before getting into the details, I would like to clarify how the
> following could be accomplished using the new commands:
> 
> Example:
> 
> 1) User configures libvirt with:
>
>Westmere
>
>
> 2) libvirt will translate that to:
>"-cpu Westmere,+aes" or "-cpu Westmere,aes=on"
> 3) libvirt wants to know if "-cpu Westmere,aes=on" is usable in
>the current host, before trying to start the VM.
> 
> How exactly would this be done using the new commands?

Hi Eduardo,

thanks for having a look - highly appreciated that you actually map this
to libvirt requirements!

That would map to a compare operation between "host" and "Westmere,aes=on".

Host could at that point already be expanded by libvirt. Doesn't matter at that
point.

If the result is "identica"l or "superset", it is runnable. If the result is
"subset" or "incompatible", details about the responsible properties is
indicated. (I actually took that idea from your patch for indicating
runnability).

Thanks!

David

Re: [Qemu-devel] [PATCH 0/3] qmp: query-host-cpu command

[David Hildenbrand]

> On Tue, Jun 21, 2016 at 02:52:54PM +0200, David Hildenbrand wrote:
> > > On Tue, Jun 21, 2016 at 08:20:40AM +0200, David Hildenbrand wrote:  
> > > > > Add QMP command to allow management software to query for
> > > > > CPU information for the running host.
> > > > > 
> > > > > The data returned by the command is in the form of a dictionary
> > > > > of QOM properties.
> > > > > 
> > > > > This series depends on the "Add runnability info to
> > > > > query-cpu-definitions" series I sent 2 weeks ago.
> > > > > 
> > > > > Git tree:
> > > > >   https://github.com/ehabkost/qemu-hacks.git work/query-host-cpu
> > > > > 
> > > > 
> > > > I like that interface, I'm going to post (maybe today? :) ) a similar 
> > > > interface
> > > > that allows to also expand other cpu models, not just the host model.   
> > > >  
> > > 
> > > In x86 I want to avoid exposing the details of other CPU models
> > > to libvirt because the details depend on machine-type.
> > > 
> > > But if it is useful for you, I believe the same "qom-properties"
> > > dict could be returned in query-cpu-definitions.
> > >   
> > > > 
> > > > Maybe we can then decide which one makes sense for all of us. But in 
> > > > general,
> > > > this interface is much better compared to what we had before. 
> > > 
> > > Maybe both? I think it's better to have a separate interface for
> > > querying "what exactly this host supports" and another one for
> > > querying for "what happens if I use -cpu host". In the case of
> > > x86, both are equivalent, but we can't guarantee this on all
> > > architectures.
> > >   
> > 
> > I'll post my patches in a couple of minutes, let's discuss it
> > then.
> > 
> > We might want to avoid having multiple interfaces carrying out
> > the same task.  
> 
> OK, I will wait for the patches before discussing it. My
> assumption is that both look similar, but are actually different
> tasks.
> 

For us, also "-cpu host" and querying the host model is identical. Not sure
if there would for now really be a requirement for some other architecture
to handle this differently. Do you have anything specific already in mind?

But let's see if it indeed makes sense to split this up after looking at
our interface proposal.

David

Re: [Qemu-devel] [RFC 00/28] s390x CPU models: exposing features

[David Hildenbrand]

> On Wed, Jun 22, 2016 at 09:54:51 +0200, David Hildenbrand wrote:
> > > On Wed, Jun 22, 2016 at 09:34:49 +0200, David Hildenbrand wrote:  
> > > > I think the coffee didn't do its work already :) . I wanted to write 
> > > > that we can
> > > > _with_ this additional query. Meaning the involved overhead would be ok 
> > > > - in my
> > > > opinion for s390x.
> > > > 
> > > > What we could do to avoid one compare operation would be:
> > > > 
> > > > a) Expand the host model
> > > > b) Expand the target model (because on s390x we could have migration 
> > > > unsafe
> > > > model)
> > > > c) Work with the runnability information returned via 
> > > > query-cpu-definitions
> > > > 
> > > > But as we have to do b) either way on s390x, we can directly do a 
> > > > compare
> > > > operation. (which makes implementation a lot simpler, because libvirt 
> > > > then
> > > > doesn't have to deal with any feature/model names).
> > > 
> > > But why do you even need to do any comparison? Isn't it possible to let
> > > QEMU do it when a domain starts? The thing is we should avoid doing
> > > completely different things on each architecture.
> > >   
> > 
> > Sure, QEMU will of course double check when starting the guest! So trying to
> > start and failing is of course an option! So no check is needed if that is
> > acceptable.  
> 
> Yeah, I think it's the safest and easiest option now.
> 
> Jirka
> 

Alright then, this RFC already handles that properly, so that seems to be
solved. The question now is if you guys see a fundamental problem in the way we
want to handle CPU models.

Especially
a) Having flexible, not migration safe CPU models that can be expanded to
migration safe models (using the expansion interface).

b) Letting QEMU carry out the task of comparing and baselining to be used
for e.g. for "virsh cpu-baseline" or "virsh cpu-compare".

c) Indicating the host model as the expansion of "-cpu host", e.g. for "virsh
capabilities" (which says "host" for now for us).

Also, it will be good to know if the "expansion" interface with parameters
"full" or "migratable" is really helpful to you or if I should drop that and
you will come up with an own "query-host-cpu".

We are also planning to implement the "query-cpu-definitions" runnability
information in the future, because as you said, it might be a good way to
quickly indicate runnable CPU models. But we are most likely not going to use it
for e.g. comparing or baselining or detection of runnability of a more complex
cpu model (having a lot of feature changes).

Thanks!

David

Re: [Qemu-devel] [RFC 00/28] s390x CPU models: exposing features

[David Hildenbrand]

> Question: KVM supports x2apic (and enables it by default) on any
> host CPU, because it is all emulated by KVM. Should "host-model"
> include x2apic on all hosts, or only when the host CPU has it?
> ("-cpu host" does include it).
> 
> Including x2apic sounds more useful, but it doesn't match the
> "get as close to the host CPU as possible" part of your
> description.
> 

From a s390x point of view, our "host" model is about indicating the maximum
possible configuration we are able to virtualize, not strictly limiting it only
to the real host features.

We also have a feature called "sthyi", emulated by KVM, which might not be
around for the real "host". We will include that in "host", because otherwise
things seem to get really complicated with no obvious benefit (to me :) ).

Not sure how you want to handle that.

David

Re: [Qemu-devel] [PATCH 0/3] qmp: query-host-cpu command

[David Hildenbrand]

> Add QMP command to allow management software to query for
> CPU information for the running host.
> 
> The data returned by the command is in the form of a dictionary
> of QOM properties.
> 
> This series depends on the "Add runnability info to
> query-cpu-definitions" series I sent 2 weeks ago.
> 
> Git tree:
>   https://github.com/ehabkost/qemu-hacks.git work/query-host-cpu
> 

I like that interface, I'm going to post (maybe today? :) ) a similar interface
that allows to also expand other cpu models, not just the host model.

Maybe we can then decide which one makes sense for all of us. But in general,
this interface is much better compared to what we had before. 

David

Re: [Qemu-devel] [PATCH v3 00/10] Allow hotplug of s390 CPUs

[David Hildenbrand]

> Changes from v2->v3:
> 
> * Call cpu_remove_sync rather than cpu_remove().
> * Pull latest version of patches from pseries set (v6).  Trivial change to 
>   "Reclaim VCPU objects" to fix checkpatch error.
> * Add object_unparent during s390_cpu_release to accomodate changes in 
>   Patch 4 "Reclaim VCPU objects."
> * Remove a cleanup patch in favor of 2 patches from pseries set.
> 
> **
> 
> The following patchset enables hotplug of s390 CPUs.
> 
> The standard interface is used -- to configure a guest with 2 CPUs online at 
> boot and 4 maximum:
> 
> qemu -smp 2,maxcpus=4
> 
> To subsequently hotplug a CPU:
> 
> Issue 'device_add s390-cpu,id=' from monitor.

(questions for the bigger audience)

For x86, cpu models are realized by making x86_64-cpu an abstract class and
creating loads of new classes, e.g. host-x86_64-cpu or haswell-x86_64-cpu.

How does 'device_add ' play together with the x86 cpu model
approach? And with cpu models specified via "-cpu" in general?

Or does that in return mean, that "making models own classes" is outdated? Or
will some internal conversion happen that I am missing?

What is the plan for cpu models and cpu hotplug? How are cpu models to be
defined in the future?

David

Re: [Qemu-devel] [PATCH v3 00/10] Allow hotplug of s390 CPUs

[David Hildenbrand]

> Am 10.02.2016 um 16:28 schrieb David Hildenbrand:
> > For x86, cpu models are realized by making x86_64-cpu an abstract class and
> > creating loads of new classes, e.g. host-x86_64-cpu or haswell-x86_64-cpu.
> > 
> > How does 'device_add ' play together with the x86 cpu model
> > approach? And with cpu models specified via "-cpu" in general?  
> 
> device_add needs to use an instantiatable type, like the ones you sketch
> above.
> 
> > Or does that in return mean, that "making models own classes" is outdated? 
> > Or
> > will some internal conversion happen that I am missing?
> > 
> > What is the plan for cpu models and cpu hotplug? How are cpu models to be
> > defined in the future?  
> 
> Someone at IBM was working on defining models for s390x - not sure what
> the status is?

That one is me right now :) Michael Mueller was working on a version without
explicit features last year. I'm now looking into models with features that can
be turned on/off - like x86 has.

As I'm trying to get a view of the bigger picture I also have to take care of
cpu hotplug, and I am not quite sure yet if we (s390) really want or need a
device_add.

a) Specification of cpu model and cpu hotplug on QEMU start

-smp 2,maxcpus=6 -cpu zBC12,+feata,+featb,prop=value

Here, it is quite clear that all cpus will get the same feature set. We don't
need any information about internals (e.g. which class is used internally for
the cpu)

b) Adding cpus via the monitor "cpu_add"

cpu-add id=3

Quite easy, we get what we ordered when starting QEMU, a cpu just like the
others.

c) Adding a cpu via device_add

device_add s390-cpu,id=3
-> Not uniform. We _want_ cpu models as cpu subclasses
(http://wiki.qemu.org/Features/CPUHotplug)

OR

device_add zBC12-s390-cpu,id=3
-> Not uniform. We don't specify the properties. But we have to specify some
magic class that we didn't have to specify on the command line. Implicitly
used information for a device.

OR

device_add zBC12-s390-cpu,id=3,feata=on,featb=off,prop=value
-> Fully specified. Again. And we need information about the internally used
class. Implicitly used information for a device.

Especially the last two examples are bad:
1) We could hotplug _different_ cpus, which is absolutely not what we want on
s390.
2) In every sane setup, we have to respecify what we already setup up at QEMU
start. (and I don't see any benefit)
3) Interface that is much more complex and more error prone to use.


d) Benefits of the new interface.

Unfortunately I can't seem to find any
(http://wiki.qemu.org/Features/CPUHotplug) but what I can think of

1) Specify something like topology more detailed (IMHO not applicable for s390)
2) Do a device_del (IMHO not applicable for s390)


Both of these points could easily be realized by extending the existing cpu-add
and by introducing a cpu_del (if really needed).


I am not against this, I just want to understand what the plan is. Because this
highly overcomplicates matter for us (s390) and requires yet another interface
to be maintained (I have some quote about new interfaces in the back of my hand
from some Linus guy ;) )

"Targets are encouraged to (re)design CPU creation so that it would be possible
to use device_add/device-del interface for it. However if due to target
design or a necessary long re-factoring time to use CPU with
device_add/device-del interface, it is possible speed-up CPU hot-add feature
development by using cpu-add interface."
(http://wiki.qemu.org/Features/CPUHotplug)

If nobody can convince me that this is the way to go and everything I said is
already clear or wrong, then I'd vote for keeping it simple and using cpu-add.

David

Re: [Qemu-devel] [PATCH v3 07/10] s390x/cpu: Move some CPU initialization into realize

[David Hildenbrand]

> In preparation for hotplug, defer some CPU initialization
> until the device is actually being realized.
> 
> Signed-off-by: Matthew Rosato <mjros...@linux.vnet.ibm.com>
> ---

Reviewed-by: David Hildenbrand <d...@linux.vnet.ibm.com>

David

Re: [Qemu-devel] [PATCH v4 4/5] s390x/cpu: Add functions to register CPU state

[David Hildenbrand]

> Introduce s390_register_cpustate, which will set the
> machine/cpu[n] link with the current CPU state.  Additionally,
> maintain an array of state pointers indexed by CPU id for fast lookup
> during interrupt handling.
> 
> Signed-off-by: Matthew Rosato <mjros...@linux.vnet.ibm.com>

Acked-by: David Hildenbrand <d...@linux.vnet.ibm.com>

David

Re: [Qemu-devel] [PATCH v4 5/5] s390x/cpu: Allow hotplug of CPUs

[David Hildenbrand]

> Implement cpu hotplug routine and add the machine hook.
> 
> Signed-off-by: Matthew Rosato <mjros...@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c |  1 +
>  target-s390x/cpu.c | 40 
>  target-s390x/cpu.h |  1 +
>  3 files changed, 42 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index b05ed8b..a15904d 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -163,6 +163,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void 
> *data)
>  
>  mc->init = ccw_init;
>  mc->reset = s390_machine_reset;
> +mc->hot_add_cpu = s390_hot_add_cpu;
>  mc->block_default_type = IF_VIRTIO;
>  mc->no_cdrom = 1;
>  mc->no_floppy = 1;
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 620b2e3..b4c59c9 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -33,6 +33,8 @@
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/boards.h"
> +#include "hw/s390x/sclp.h"
>  #endif
>  
>  #define CR0_RESET   0xE0UL
> @@ -226,6 +228,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  #endif
>  
>  scc->parent_realize(dev, errp);
> +
> +#if !defined(CONFIG_USER_ONLY)
> +if (dev->hotplugged) {
> +raise_irq_cpu_hotplug();
> +}
> +#endif
>  }
>  
>  static void s390_cpu_initfn(Object *obj)
> @@ -269,6 +277,38 @@ static void s390_cpu_finalize(Object *obj)
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> +void s390_hot_add_cpu(const int64_t id, Error **errp)
> +{
> +MachineState *machine = MACHINE(qdev_get_machine());
> +
> +if (id < 0) {
> +error_setg(errp, "Invalid CPU id: %" PRIi64, id);
> +return;
> +}
> +
> +if (cpu_exists(id)) {
> +error_setg(errp, "Unable to add CPU: %" PRIi64
> +   ", it already exists", id);
> +return;
> +}
> +
> +if (id >= max_cpus) {
> +error_setg(errp, "Unable to add CPU: %" PRIi64
> +   ", max allowed: %d", id, max_cpus - 1);
> +return;
> +}
> +
> +if (id != next_cpu_id) {
> +error_setg(errp, "Unable to add CPU: %" PRIi64
> +   ", The next available id is %d", id, next_cpu_id);
> +return;
> +}
> +
> +cpu_s390x_init(machine->cpu_model);
> +}
> +#endif
> +
> +#if !defined(CONFIG_USER_ONLY)

This #if block could be combined with the previous one. But it's not worth
blocking this series so

Acked-by: David Hildenbrand <d...@linux.vnet.ibm.com>

As we have a clean interface now (cpu-add id=x) most of this series is internal
stuff, so I think this is ready to be picked up :) .

David

Re: [Qemu-devel] [PATCH v4 5/5] s390x/cpu: Allow hotplug of CPUs

[David Hildenbrand]

> >  #if !defined(CONFIG_USER_ONLY)
> > +void s390_hot_add_cpu(const int64_t id, Error **errp)
> > +{  
> to make it future-proof wrt migration it could be better to
> enforce here that 'id' grows in +1 steps so user
> won't be able create cpus with gaps.

That should be already covered by:

if (id != next_cpu_id)
...

or am I missing something?

David

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[David Hildenbrand]

> > After all the discussions about
> > -device-add s390-cpu,id=XX
> > 
> > As substitute/addition in the future for hotplug it is the straightforward
> > approach to allow setting the id as property. Nobody knows what crazy new
> > hotplug method we will come up with. But doing it the device way with 
> > properties
> > cannot be wrong. And the id is a fundamental concept of a vcpu (cpu-add 
> > id=XX).  
> with device_add 'id' is not a vcpu concept but and arbitrary user supplied 
> string
> property owned by Device. But since s390 matches current x86 thread based 
> model it could be migrated to device_add the same way, for example:
>   device_add s390-cpu,thread=XX

So should we name the property thread then?
Looks like the id property is really special.

What do you suggest?

> 
> > 
> > So I'd like to avoid reworking everything again, to realize later that we
> > want it as a property and rewriting it once again.  
> for s390, the thing about not rewriting everything once again could be
> replacing places where cpu_index is used with CpuClass.arch_id().
> arch_id() defaults to cpu_index but you can later override it with
> your own id (whatever s390 uses for identifying cpus on baremetal)
> so switching to device_add won't break anything.

Okay, this way we could get rid of cpu_index later.

David

Re: [Qemu-devel] [PATCH v9 6/7] s390x/cpu: Add error handling to cpu creation

[David Hildenbrand]

> Check for and propogate errors during s390 cpu creation.
> 
> Signed-off-by: Matthew Rosato <mjros...@linux.vnet.ibm.com>

Looks good to me.

Reviewed-by: David Hildenbrand <d...@linux.vnet.ibm.com>

David

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[David Hildenbrand]

> On Fri, Mar 04, 2016 at 12:07:28PM +0100, David Hildenbrand wrote:
> >   
> > > >  cpu_exec_init(cs, );
> > > >  if (err != NULL) {
> > > >  error_propagate(errp, err);
> > > >  return;
> > > >  }
> > > > +scc->next_cpu_id = cs->cpu_index + 1;
> > > 
> > > It appears that scc->next_cpu_id (and hence cpu->id) is some sort of 
> > > arch_id
> > > for you. If it is just going to be monotonically increasing like 
> > > cs->cpu_index,
> > > couldn't you just use cs->cpu_index instead of introducing additional IDs 
> > > ?
> > > 
> > > Note that cpu_exec_init(cs, ) returns with the next available 
> > > cpu_index
> > > which can be compared against max_cpus directly.
> > > 
> > > Regards,
> > > Bharata.  
> > 
> > I don't think that we should mix the id setting and cpu_index for now.
> > 
> > We can't simply set cpu_index before the device is realized. That logic
> > belongs to cpu_exec_init().  
> 
> Yes, I see that, but apart from the following obvious uses of the id
> property from realizefn, are there other uses ?
> 
> 1 Checking against max_cpus
>   cpu_index can be used for this.
> 
> 2 Checking if cpu with such an id exists
>   cpu_exec_init() would never return with an in-use index. Hence cpu_index
>   can be used here too given that you don't define ->get_arch_id()
> 
> 3 Checking if the id is the next expected one
>   cpu_exec_init/cpu_exec_exit take care of deletion too, so I guess you will
>   always have the next available id to be used in cpu_index.
> 
> Did I miss any other use other than these which makes it necessary to have
> an explicit id property here ?

After all the discussions about
-device-add s390-cpu,id=XX

As substitute/addition in the future for hotplug it is the straightforward
approach to allow setting the id as property. Nobody knows what crazy new
hotplug method we will come up with. But doing it the device way with properties
cannot be wrong. And the id is a fundamental concept of a vcpu (cpu-add id=XX).

So I'd like to avoid reworking everything again, to realize later that we
want it as a property and rewriting it once again.

David

Re: [Qemu-devel] [PATCH v8 3/7] s390x/cpu: Get rid of side effects when creating a vcpu

[David Hildenbrand]

> In preparation for hotplug, defer some CPU initialization
> until the device is actually being realized, including
> cpu_exec_init.
> 
> Signed-off-by: Matthew Rosato <mjros...@linux.vnet.ibm.com>

Looks good to me!

Reviewed-by: David Hildenbrand <d...@linux.vnet.ibm.com>

David

Re: [Qemu-devel] [PATCH v8 4/7] s390x/cpu: Tolerate max_cpus

[David Hildenbrand]

> Once hotplug is enabled, interrupts may come in for CPUs
> with an address > smp_cpus.  Allocate for this and allow
> search routines to look beyond smp_cpus.
> 
> Signed-off-by: Matthew Rosato 
> ---
>  hw/s390x/s390-virtio.c | 13 +++--
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index c501a48..90bc58a 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -58,15 +58,16 @@
>  #define S390_TOD_CLOCK_VALUE_MISSING0x00
>  #define S390_TOD_CLOCK_VALUE_PRESENT0x01
> 
> -static S390CPU **ipi_states;
> +static S390CPU **cpu_states;
> 
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
> -if (cpu_addr >= smp_cpus) {
> +if (cpu_addr >= max_cpus) {
>  return NULL;
>  }
> 
> -return ipi_states[cpu_addr];
> +/* Fast lookup via CPU ID */
> +return cpu_states[cpu_addr];
>  }
> 
>  void s390_init_ipl_dev(const char *kernel_filename,
> @@ -101,14 +102,14 @@ void s390_init_cpus(MachineState *machine)
>  machine->cpu_model = "host";
>  }
> 
> -ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
> +cpu_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
> 
> -for (i = 0; i < smp_cpus; i++) {
> +for (i = 0; i < max_cpus; i++) {
>  S390CPU *cpu;
> 
>  cpu = cpu_s390x_init(machine->cpu_model);
> 
> -ipi_states[i] = cpu;
> +cpu_states[i] = cpu;

This looks wrong (creating all cpus). But the net patch fixes it again.

Can you make this patch a simple rename patch and move the max_cpu stuff into
the next patch if this makes sense?

Or simply set the cpu_state for everything above smp_cpus to zero in this patch.

Whatever you think makes sense.

>  }
>  }
> 


David

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[David Hildenbrand]

> Check for and propogate errors during s390 cpu creation.
> 
> Signed-off-by: Matthew Rosato 
> ---
>  hw/s390x/s390-virtio.c |  2 +-
>  target-s390x/cpu-qom.h |  1 +
>  target-s390x/cpu.c | 56 
> +-
>  target-s390x/cpu.h |  2 ++
>  target-s390x/helper.c  | 42 +++--
>  5 files changed, 99 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index f00d6b4..2ab7b94 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -116,7 +116,7 @@ void s390_init_cpus(MachineState *machine)
>  }
> 
>  for (i = 0; i < smp_cpus; i++) {
> -cpu_s390x_init(machine->cpu_model);
> +s390_new_cpu(machine->cpu_model, i, _fatal);
>  }
>  }
> 
> diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
> index 56d82f2..1c90933 100644
> --- a/target-s390x/cpu-qom.h
> +++ b/target-s390x/cpu-qom.h
> @@ -68,6 +68,7 @@ typedef struct S390CPU {
>  /*< public >*/
> 
>  CPUS390XState env;
> +int64_t id;
>  /* needed for live migration */
>  void *irqstate;
>  uint32_t irqstate_saved_size;
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 76c8eaf..d1b7af9 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -30,8 +30,10 @@
>  #include "qemu/error-report.h"
>  #include "hw/hw.h"
>  #include "trace.h"
> +#include "qapi/visitor.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
> +#include "sysemu/sysemu.h"
>  #endif
> 
>  #define CR0_RESET   0xE0UL
> @@ -199,16 +201,36 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  CPUS390XState *env = >env;
>  Error *err = NULL;
> 
> +#if !defined(CONFIG_USER_ONLY)
> +if (cpu->id >= max_cpus) {
> +error_setg(errp, "Unable to add CPU: %" PRIi64
> +   ", max allowed: %d", cpu->id, max_cpus - 1);

not sure if we should report this into err and then to an error_propgate().

> +return;
> +}
> +#endif
> +if (cpu_exists(cpu->id)) {
> +error_setg(errp, "Unable to add CPU: %" PRIi64
> +   ", it already exists", cpu->id);

dito

> +return;
> +}
> +if (cpu->id != scc->next_cpu_id) {
> +error_setg(errp, "Unable to add CPU: %" PRIi64
> +   ", The next available id is %" PRIi64, cpu->id,
> +   scc->next_cpu_id);

dito

> +return;
> +}
> +
>  cpu_exec_init(cs, );
>  if (err != NULL) {
>  error_propagate(errp, err);
>  return;
>  }
> +scc->next_cpu_id = cs->cpu_index + 1;

Why can't we simply do a scc->next_cpu_id++ as before and leave cs->cpu_index
logic out?

> 
>  #if !defined(CONFIG_USER_ONLY)
>  qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
>  #endif
> -env->cpu_num = scc->next_cpu_id++;
> +env->cpu_num = cpu->id;
>  s390_cpu_gdb_init(cs);
>  qemu_init_vcpu(cs);
>  #if !defined(CONFIG_USER_ONLY)
> @@ -220,6 +242,36 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  scc->parent_realize(dev, errp);
>  }
> 
> +static void s390_cpu_set_id(Object *obj, Visitor *v, const char *name,
> +void *opaque, Error **errp)

s/s390_/s390x_/ ?

> +{
> +S390CPU *cpu = S390_CPU(obj);
> +DeviceState *dev = DEVICE(obj);
> +const int64_t min = 0;
> +const int64_t max = UINT32_MAX;
> +Error *err = NULL;
> +int64_t value;
> +
> +if (dev->realized) {
> +error_setg(errp, "Attempt to set property '%s' on '%s' after "
> +   "it was realized", name, object_get_typename(obj));
> +return;
> +}
> +
> +visit_type_int(v, name, , );
> +if (err) {
> +error_propagate(errp, err);
> +return;
> +}
> +if (value < min || value > max) {
> +error_setg(errp, "Property %s.%s doesn't take value %" PRId64
> +   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
> +   object_get_typename(obj), name, value, min, max);
> +return;
> +}
> +cpu->id = value;
> +}
> +
>  static void s390_cpu_initfn(Object *obj)
>  {
>  CPUState *cs = CPU(obj);
> @@ -233,6 +285,8 @@ static void s390_cpu_initfn(Object *obj)
>  cs->env_ptr = env;
>  cs->halted = 1;
>  cs->exception_index = EXCP_HLT;
> +object_property_add(OBJECT(cpu), "id", "int64_t", NULL, s390_cpu_set_id,
> +NULL, NULL, NULL);

Would it be helpful for introspection to also get the id of a cpu?


>  #if !defined(CONFIG_USER_ONLY)
>  qemu_get_timedate(, 0);
>  env->tod_offset = TOD_UNIX_EPOCH +
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 49c8415..6667cc0 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -413,6 +413,8 @@ void trigger_pgm_exception(CPUS390XState *env, uint32_t 
> code, uint32_t ilen);
>  #endif
> 
>  S390CPU *cpu_s390x_init(const 

Re: [Qemu-devel] [PATCH v8 7/7] s390x/cpu: Allow hotplug of CPUs

[David Hildenbrand]

> Implement cpu hotplug routine and add the machine hook.
> 
> Signed-off-by: Matthew Rosato <mjros...@linux.vnet.ibm.com>
> Reviewed-by: David Hildenbrand <d...@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 13 +
>  target-s390x/cpu.c |  7 +++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 7fc1879..174a2f8 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -186,6 +186,18 @@ static HotplugHandler 
> *s390_get_hotplug_handler(MachineState *machine,
>  return NULL;
>  }
> 
> +static void s390_hot_add_cpu(const int64_t id, Error **errp)
> +{
> +MachineState *machine = MACHINE(qdev_get_machine());
> +Error *err = NULL;
> +
> +s390_new_cpu(machine->cpu_model, id, );
> +if (err) {
> +error_propagate(errp, err);
> +return;
> +}

You could unconditionally call error_propagate(errp, err); here

> +}
> +


Still looks good to me!

David

Re: [Qemu-devel] [PATCH v8 5/7] s390x/cpu: Add CPU property links

[David Hildenbrand]

> Link each CPUState as property machine/cpu[n] during initialization.
> Add a hotplug handler to s390-virtio-ccw machine and set the
> state during plug.
> 
> Signed-off-by: Matthew Rosato <mjros...@linux.vnet.ibm.com>

Reviewed-by: David Hildenbrand <d...@linux.vnet.ibm.com>

David

Re: [Qemu-devel] [PATCH v8 6/7] s390x/cpu: Add error handling to cpu creation

[David Hildenbrand]

> >  cpu_exec_init(cs, );
> >  if (err != NULL) {
> >  error_propagate(errp, err);
> >  return;
> >  }
> > +scc->next_cpu_id = cs->cpu_index + 1;  
> 
> It appears that scc->next_cpu_id (and hence cpu->id) is some sort of arch_id
> for you. If it is just going to be monotonically increasing like 
> cs->cpu_index,
> couldn't you just use cs->cpu_index instead of introducing additional IDs ?
> 
> Note that cpu_exec_init(cs, ) returns with the next available cpu_index
> which can be compared against max_cpus directly.
> 
> Regards,
> Bharata.

I don't think that we should mix the id setting and cpu_index for now.

We can't simply set cpu_index before the device is realized. That logic
belongs to cpu_exec_init(). But I agree that cpu_index and id should always
match after a successful realize.

That id / cpu_index stuff can be cleaned up later. We should not treat cpu_index
as a property for now (by exposing it as "id").

David

Re: [Qemu-devel] [PATCH v7 5/6] s390x/cpu: Add error handling to cpu creation

[David Hildenbrand]

> Check for and propogate errors during s390 cpu creation.
> 
> Signed-off-by: Matthew Rosato 
> ---
>  hw/s390x/s390-virtio-ccw.c | 30 +
>  hw/s390x/s390-virtio.c |  2 +-
>  hw/s390x/s390-virtio.h |  1 +
>  target-s390x/cpu-qom.h |  3 +++
>  target-s390x/cpu.c | 65 
> --
>  target-s390x/cpu.h |  1 +
>  target-s390x/helper.c  | 31 --
>  7 files changed, 128 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 3090e76..4886dbf 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -112,6 +112,36 @@ void s390_memory_init(ram_addr_t mem_size)
>  s390_skeys_init();
>  }
> 
> +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp)

Just a thought, if not passing machine but the model string, we could make

cpu_s390x_init() call s390_new_cpu().

But then s390_new_cpu() would have to be moved again. Not sure if this is
really worth it.


David

Re: [Qemu-devel] [PATCH v7 3/6] s390x/cpu: Move some CPU initialization into realize

[David Hildenbrand]

> In preparation for hotplug, defer some CPU initialization
> until the device is actually being realized.
> 
> Signed-off-by: Matthew Rosato 
> Reviewed-by: Andreas Färber 
> ---
>  target-s390x/cpu.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 603c2a1..8dfd063 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -195,7 +195,13 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  {
>  CPUState *cs = CPU(dev);
>  S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
> +S390CPU *cpu = S390_CPU(dev);
> +CPUS390XState *env = >env;
> 
> +#if !defined(CONFIG_USER_ONLY)
> +qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
> +#endif
> +env->cpu_num = cs->cpu_index;
>  s390_cpu_gdb_init(cs);
>  qemu_init_vcpu(cs);
>  #if !defined(CONFIG_USER_ONLY)
> @@ -213,7 +219,6 @@ static void s390_cpu_initfn(Object *obj)
>  S390CPU *cpu = S390_CPU(obj);
>  CPUS390XState *env = >env;
>  static bool inited;
> -static int cpu_num = 0;
>  #if !defined(CONFIG_USER_ONLY)
>  struct tm tm;
>  #endif
> @@ -223,7 +228,6 @@ static void s390_cpu_initfn(Object *obj)
>  cs->exception_index = EXCP_HLT;
>  cpu_exec_init(cs, _abort);
>  #if !defined(CONFIG_USER_ONLY)
> -qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
>  qemu_get_timedate(, 0);
>  env->tod_offset = TOD_UNIX_EPOCH +
>(time2tod(mktimegm()) * 10ULL);
> @@ -232,7 +236,6 @@ static void s390_cpu_initfn(Object *obj)
>  env->cpu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
>  s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
>  #endif
> -env->cpu_num = cpu_num++;
> 
>  if (tcg_enabled() && !inited) {
>  inited = true;

Hmm, two things

a) env is not needed in this patch
b) This patch breaks cpu creation temporarily (cpu number rework). cpu_num is
always 0 - a problem at east for tcg.

You should introduce scc->next_cpu_id in this patch, cpu->id can be left
in the error handling patch.

David

Re: [Qemu-devel] [PATCH v7 6/6] s390x/cpu: Allow hotplug of CPUs

[David Hildenbrand]

> Implement cpu hotplug routine and add the machine hook.
> 
> Signed-off-by: Matthew Rosato <mjros...@linux.vnet.ibm.com>

Reviewed-by: David Hildenbrand <d...@linux.vnet.ibm.com>

> ---
>  hw/s390x/s390-virtio-ccw.c | 13 +
>  target-s390x/cpu.c |  7 +++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4886dbf..cfb2ef4 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -215,6 +215,18 @@ static HotplugHandler 
> *s390_get_hotplug_handler(MachineState *machine,
>  return NULL;
>  }
> 
> +static void s390_hot_add_cpu(const int64_t id, Error **errp)
> +{
> +MachineState *machine = MACHINE(qdev_get_machine());
> +Error *local_err = NULL;
> +
> +s390_new_cpu(machine, id, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
> +}
> +
>  static void ccw_machine_class_init(ObjectClass *oc, void *data)
>  {
>  MachineClass *mc = MACHINE_CLASS(oc);
> @@ -223,6 +235,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void 
> *data)
> 
>  mc->init = ccw_init;
>  mc->reset = s390_machine_reset;
> +mc->hot_add_cpu = s390_hot_add_cpu;
>  mc->block_default_type = IF_VIRTIO;
>  mc->no_cdrom = 1;
>  mc->no_floppy = 1;
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index ec66ed6..6585f04 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -33,6 +33,7 @@
>  #include "qapi/visitor.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
> +#include "hw/s390x/sclp.h"
>  #endif
> 
>  #define CR0_RESET   0xE0UL
> @@ -227,6 +228,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  #endif
> 
>  scc->parent_realize(dev, errp);
> +
> +#if !defined(CONFIG_USER_ONLY)
> +if (dev->hotplugged) {
> +raise_irq_cpu_hotplug();
> +}
> +#endif
>  }
> 
>  static void s390_cpu_get_id(Object *obj, Visitor *v, const char *name,



David

Re: [Qemu-devel] [PATCH v7 5/6] s390x/cpu: Add error handling to cpu creation

[David Hildenbrand]

> Check for and propogate errors during s390 cpu creation.
> 
> Signed-off-by: Matthew Rosato 
> ---
>  hw/s390x/s390-virtio-ccw.c | 30 +
>  hw/s390x/s390-virtio.c |  2 +-
>  hw/s390x/s390-virtio.h |  1 +
>  target-s390x/cpu-qom.h |  3 +++
>  target-s390x/cpu.c | 65 
> --
>  target-s390x/cpu.h |  1 +
>  target-s390x/helper.c  | 31 --
>  7 files changed, 128 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 3090e76..4886dbf 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -112,6 +112,36 @@ void s390_memory_init(ram_addr_t mem_size)
>  s390_skeys_init();
>  }
> 
> +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp)
> +{
> +S390CPU *cpu = NULL;
> +Error *local_err = NULL;

Think the naming schema is "err" now.

> +
> +if (id >= max_cpus) {
> +error_setg(errp, "Unable to add CPU: %" PRIi64
> +   ", max allowed: %d", id, max_cpus - 1);
> +goto out;

Could we also move this check to the realize function?

> +}
> +
> +cpu = cpu_s390x_create(machine->cpu_model, _err);
> +if (local_err != NULL) {
> +goto out;
> +}
> +
> +object_property_set_int(OBJECT(cpu), id, "id", _err);

We should add a check in between

if (err) {
goto out;
}

> +object_property_set_bool(OBJECT(cpu), true, "realized", _err);
> +
> +out:
> +if (cpu != NULL) {
> +object_unref(OBJECT(cpu));

Is the object_unref() here correct?
I know that we have one reference from VCPU creation. Where does the second one
come from (is it from the hotplug handler? then I'd prefer a comment here :D )

> +}
> +if (local_err) {
> +error_propagate(errp, local_err);
> +cpu = NULL;
> +}
> +return cpu;
> +}
> +
>  static void ccw_init(MachineState *machine)
>  {
>  int ret;
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index 6bd9803..6f71ab0 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -118,7 +118,7 @@ void s390_init_cpus(MachineState *machine)
>  }
> 
>  for (i = 0; i < smp_cpus; i++) {
> -cpu_s390x_init(machine->cpu_model);
> +s390_new_cpu(machine, i, _fatal);
>  }
>  }
> 
> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
> index ffd014c..fbcfdb8 100644
> --- a/hw/s390x/s390-virtio.h
> +++ b/hw/s390x/s390-virtio.h
> @@ -29,4 +29,5 @@ void s390_create_virtio_net(BusState *bus, const char 
> *name);
>  void s390_nmi(NMIState *n, int cpu_index, Error **errp);
>  void s390_machine_reset(void);
>  void s390_memory_init(ram_addr_t mem_size);
> +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp);
>  #endif
> diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
> index 029a44a..1c90933 100644
> --- a/target-s390x/cpu-qom.h
> +++ b/target-s390x/cpu-qom.h
> @@ -47,6 +47,8 @@ typedef struct S390CPUClass {
>  CPUClass parent_class;
>  /*< public >*/
> 
> +int64_t next_cpu_id;
> +
>  DeviceRealize parent_realize;
>  void (*parent_reset)(CPUState *cpu);
>  void (*load_normal)(CPUState *cpu);
> @@ -66,6 +68,7 @@ typedef struct S390CPU {
>  /*< public >*/
> 
>  CPUS390XState env;
> +int64_t id;
>  /* needed for live migration */
>  void *irqstate;
>  uint32_t irqstate_saved_size;
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 8dfd063..ec66ed6 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -30,6 +30,7 @@
>  #include "qemu/error-report.h"
>  #include "hw/hw.h"
>  #include "trace.h"
> +#include "qapi/visitor.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
>  #endif
> @@ -197,11 +198,26 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
>  S390CPU *cpu = S390_CPU(dev);
>  CPUS390XState *env = >env;
> +Error *local_err = NULL;
> +
> +if (cpu->id != scc->next_cpu_id) {
> +error_setg(errp, "Unable to add CPU: %" PRIi64
> +   ", The next available id is %" PRIi64, cpu->id,
> +   scc->next_cpu_id);
> +return;
> +}
> +
> +cpu_exec_init(cs, _err);
> +if (local_err != NULL) {
> +error_propagate(errp, local_err);
> +return;
> +}
> +scc->next_cpu_id = cs->cpu_index + 1;
> 
>  #if !defined(CONFIG_USER_ONLY)
>  qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
>  #endif
> -env->cpu_num = cs->cpu_index;
> +env->cpu_num = cpu->id;
>  s390_cpu_gdb_init(cs);
>  qemu_init_vcpu(cs);
>  #if !defined(CONFIG_USER_ONLY)
> @@ -213,6 +229,49 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>  scc->parent_realize(dev, errp);
>  }
> 
> +static void s390_cpu_get_id(Object *obj, Visitor *v, const char 

Re: [Qemu-devel] [PATCH v7 5/6] s390x/cpu: Add error handling to cpu creation

[David Hildenbrand]

> >> +static void s390_cpu_get_id(Object *obj, Visitor *v, const char *name,
> >> +void *opaque, Error **errp)
> >> +{
> >> +S390CPU *cpu = S390_CPU(obj);
> >> +int64_t value = cpu->id;
> >> +
> >> +visit_type_int(v, name, , errp);
> >> +}
> >> +
> >> +static void s390_cpu_set_id(Object *obj, Visitor *v, const char *name,
> >> +void *opaque, Error **errp)
> >> +{
> >> +S390CPU *cpu = S390_CPU(obj);
> >> +DeviceState *dev = DEVICE(obj);
> >> +const int64_t min = 0;
> >> +const int64_t max = UINT32_MAX;
> >> +Error *local_err = NULL;
> >> +int64_t value;
> >> +
> >> +if (dev->realized) {
> >> +error_setg(errp, "Attempt to set property '%s' on '%s' after "
> >> +   "it was realized", name, object_get_typename(obj));
> >> +return;
> >> +}
> >> +
> >> +visit_type_int(v, name, , _err);
> >> +if (local_err) {
> >> +error_propagate(errp, local_err);
> >> +return;
> >> +}
> >> +if (value < min || value > max) {
> >> +error_setg(errp, "Property %s.%s doesn't take value %" PRId64
> >> +   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
> >> +   object_get_typename(obj), name, value, min, max);
> >> +return;
> >> +}
> >> +if ((value != cpu->id) && cpu_exists(value)) {
> >> +error_setg(errp, "CPU with ID %" PRIi64 " exists", value);
> >> +return;
> >> +}
> >> +cpu->id = value;
> >> +}  
> > 
> > Just curious, what about using a simple
> > 
> > object_property_set_int() and doing all the checks in realize() ?
> > 
> > Then we could live without manual getter/setter (and without the realize 
> > check).
> >   
> 
> I think we still need at least a manual setter, even if you want to move
> the checks to realize.
> 
> See something like object_property_add_uint64_ptr() -- It sets a
> boilerplate get routine, and no set routine -- I think this presumes you
> set your property upfront (at add time), never change it for the life of
> the object, but want to read it later.
> By comparison, S390CPU.id is set sometime after instance_init, based on
> input.
> 
> So, we call object_property_set_int() to update it --  This just passes
> the provided int value to the setter routine associated with the
> property.  If one doesn't exist, you get:
> qemu: Insufficient permission to perform this operation
> 
> I think this is also why we want to check for dev->realized in the
> setter routine, to make sure the property is not being changed "too
> late" -- Once the cpu is realized, the ID is baked and can't be changed.
> 
> Or did I misunderstand your idea here?

If we care about malicious users, wanting to set id's after realize that is
true. But I am no QOM expert and don't know if that is a scenarios that
has to be taken care of. But as I see similar code for other properties,
I assume we are better off doing it also that way.

David

Re: [Qemu-devel] [PATCH v9 4/7] s390x/cpu: Tolerate max_cpus

[David Hildenbrand]

> Once hotplug is enabled, interrupts may come in for CPUs
> with an address > smp_cpus.  Allocate for this and allow
> search routines to look beyond smp_cpus.
> 
> Signed-off-by: Matthew Rosato <mjros...@linux.vnet.ibm.com>

Reviewed-by: David Hildenbrand <d...@linux.vnet.ibm.com>

> ---
>  hw/s390x/s390-virtio.c | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index c501a48..57c3c88 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -58,15 +58,16 @@
>  #define S390_TOD_CLOCK_VALUE_MISSING0x00
>  #define S390_TOD_CLOCK_VALUE_PRESENT0x01
> 
> -static S390CPU **ipi_states;
> +static S390CPU **cpu_states;
> 
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
> -if (cpu_addr >= smp_cpus) {
> +if (cpu_addr >= max_cpus) {
>  return NULL;
>  }
> 
> -return ipi_states[cpu_addr];
> +/* Fast lookup via CPU ID */
> +return cpu_states[cpu_addr];
>  }
> 
>  void s390_init_ipl_dev(const char *kernel_filename,
> @@ -101,14 +102,14 @@ void s390_init_cpus(MachineState *machine)
>  machine->cpu_model = "host";
>  }
> 
> -ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
> +cpu_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
> 
>  for (i = 0; i < smp_cpus; i++) {
>  S390CPU *cpu;
> 
>  cpu = cpu_s390x_init(machine->cpu_model);
> 
> -ipi_states[i] = cpu;
> +cpu_states[i] = cpu;
>  }
>  }
> 



David

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[David Hildenbrand]

> Updated to:
> 
> ##
> # @CpuDefinitionInfo:
> #
> # Virtual CPU definition.
> #
> # @name: the name of the CPU definition
> # @runnable: #optional. whether the CPU model us usable with the
> #current machine and accelerator. Omitted if we don't
> #know the answer. (since 2.7)
> # @unavailable-features: List of attributes that prevent the CPU

"List of properties" ?

> #model from running in the current host.
> #(since 2.7)
> #
> # @unavailable-features is a list of QOM property names that
> # represent CPU model attributes that prevent the CPU from running.
> # If the QOM property is read-only, that means the CPU model can
> # never run in the current host. If the property is read-write, it
> # means that it MAY be possible to run the CPU model in the current
> # host if that property is changed. Management software can use it
> # as hints to suggest or choose an alternative for the user, or
> # just to generate meaningful error messages explaining why the CPU
> # model can't be used.
> #
> # Since: 1.2.0
> ##
> 

what about changing unavailable-features to

problematic-properties
responsible-properties

... anything else making clear that we are dealing with properties, not
only features?

Apart from that, sounds good to me.

David

Re: [Qemu-devel] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[David Hildenbrand]

> > Yes, I think so. However to really make good hints, upper layers would most
> > likely need more information about the exact problem with a property -
> > maybe something like an enum value per problematic property.
> > (UNAVAILABLE_FEATURE, VALUE_TOO_BIG, VALUE_TOO_SMALL, UNSUPPORTED_VALUE) 
> > ...  
> 
> If a more powerful interface is needed, we can add extra fields,
> yes. Although I'm not sure we really start providing that level
> of detail in a generic way (I guess it will depend on how much
> arch-independent code libvirt will use to handle runnability
> information).

And I would actually (later) prefer to have something like
bool too_new; (name tbd)

to cover the cpu-generation problem instead of having to expose read-only
properties just for the sake of communicating that a cpu model is simply too new
and cannot be made runnable toggling features.

> 
> >   
> > > > > 
> > > > > We could replace this with something more generic, like:
> > > > > 
> > > > > @runnability-blockers: List of attributes that prevent the CPU
> > > > >   model from running in the current host.
> > > > >   
> > > > >   A list of QOM property names that represent CPU model
> > > > >   attributes that prevent the CPU from running. If the QOM
> > > > >   property is read-only, that means the CPU model can never run
> > > > >   in the current host. If the property is read-write, it means
> > > > >   that it MAY be possible to run the CPU model in the current
> > > > >   host if that property is changed.
> > > > >   
> > > > >   Management software can use it as hints to suggest or choose an
> > > > >   alternative for the user, or just to generate meaningful error
> > > > >   messages explaining why the CPU model can't be used.
> > > > > 
> > > > > (I am looking for a better name than "runnability-blockers").
> > > > > 
> > 
> > Not sure which approach would be better.  
> 
> Note that this is only a change in documentation of the new
> field, to allow it to cover extra cases without any changes in
> the interface.
> 
> I also don't like the "runnability-blockers" name, but I prefer
> the new documentation I wrote above because it is more explicit
> about what exactly the field means. I plan to keep the
> "unavailable-features" name but use the above documentation text
> in the next version. Does that sound OK?
> 

I like the read-only part about that, but still you should somehow clarify that
we are dealing with boolean properties a.k.a features. At least that's my
opinion.

Apart from that, this is fine with me!

David

Re: [Qemu-devel] [PATCH 0/9] Add runnability info to query-cpu-definitions

[David Hildenbrand]

> This series extends query-cpu-definitions to include two extra
> fields: "runnable", and "unavailable-features".
> 
> This will return information based on the current machine and
> accelerator only. In the future we may extend these mechanisms to
> allow querying other machines and other accelerators without
> restarting QEMU, but it will require some reorganization of
> QEMU's main code.

Hi Eduardo,

just as discussed during/after the last kvm forum offline, we are currently
working on a new set of qmp functions to keep all the cpu model details
in QEMU and not have to replicate them in libvirt and to have a nice way
to query the host cpu model.

We already have a prototype running, just need to clarify some s390x details
(feature set of the CPU generations) before we can send out an RFC. I could,
however, send out just the interface changes + details on the concept upfront,
if anybody is interested.

In the current implementation:
- s390x cpu models will be independent of the QEMU machine, so for s390x the
  runnability information will only depend on the accelerator (host cpu model).
  However, for compatibility machines, cpu models will be disabled, and we won't
  therefore have any runnability information.
- we will not touch query-cpu-definitions
- the new interfaces will also make use of the current accelerator and the
  current machine, so the context of these functions are the same.
  
Adding functionality to query for other machines is not necessary for s390x.
Other accelerators don't really make sense to me, let's keep it simple here.

So this change is fine from my point of view. It doesn't contradict to out
concept.

David

Re: [Qemu-devel] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[David Hildenbrand]

> Extend query-cpu-definitions schema to allow it to return two new
> optional fields: "runnable" and "unavailable-features".
> "runnable" will tell if the CPU model can be run in the current
> host. "unavailable-features" will contain a list of CPU
> properties that are preventing the CPU model from running in the
> current host.
> 
> Cc: David Hildenbrand <d...@linux.vnet.ibm.com>
> Cc: Michael Mueller <m...@linux.vnet.ibm.com>
> Cc: Christian Borntraeger <borntrae...@de.ibm.com>
> Cc: Cornelia Huck <cornelia.h...@de.ibm.com>
> Cc: Jiri Denemark <jdene...@redhat.com>
> Cc: libvir-l...@redhat.com
> Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
> ---
>  qapi-schema.json | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 54634c4..450e6e7 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2948,11 +2948,19 @@
>  # Virtual CPU definition.
>  #
>  # @name: the name of the CPU definition
> +# @runnable: true if the CPU model is runnable using the current
> +#machine and accelerator. Optional. Since 2.6.
> +# @unavailable-features: List of properties that prevent the CPU
> +#model from running in the current host,
> +#if @runnable is false. Optional.
> +#Since 2.6.

Just FYI, on other architectures (e.g. s390x), other conditions (e.g. cpu
generation) also define if a CPU model is runnable, so the pure availability of
features does not mean that a cpu model is runnable.

We could have runnable=false and unavailable-features being an empty list.


>  #
>  # Since: 1.2.0
>  ##
>  { 'struct': 'CpuDefinitionInfo',
> -  'data': { 'name': 'str' } }
> +  'data': { 'name': 'str',
> +'*runnable': 'bool',
> +'*unavailable-features': [ 'str' ] } }
> 
>  ##
>  # @query-cpu-definitions:



David

Re: [Qemu-devel] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[David Hildenbrand]

> On Mon, May 09, 2016 at 10:54:53AM +0200, David Hildenbrand wrote:
> > > Extend query-cpu-definitions schema to allow it to return two new
> > > optional fields: "runnable" and "unavailable-features".
> > > "runnable" will tell if the CPU model can be run in the current
> > > host. "unavailable-features" will contain a list of CPU
> > > properties that are preventing the CPU model from running in the
> > > current host.
> > > 
> > > Cc: David Hildenbrand <d...@linux.vnet.ibm.com>
> > > Cc: Michael Mueller <m...@linux.vnet.ibm.com>
> > > Cc: Christian Borntraeger <borntrae...@de.ibm.com>
> > > Cc: Cornelia Huck <cornelia.h...@de.ibm.com>
> > > Cc: Jiri Denemark <jdene...@redhat.com>
> > > Cc: libvir-l...@redhat.com
> > > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
> > > ---
> > >  qapi-schema.json | 10 +-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > index 54634c4..450e6e7 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -2948,11 +2948,19 @@
> > >  # Virtual CPU definition.
> > >  #
> > >  # @name: the name of the CPU definition
> > > +# @runnable: true if the CPU model is runnable using the current
> > > +#machine and accelerator. Optional. Since 2.6.
> > > +# @unavailable-features: List of properties that prevent the CPU
> > > +#model from running in the current host,
> > > +#if @runnable is false. Optional.
> > > +#Since 2.6.  
> > 
> > Just FYI, on other architectures (e.g. s390x), other conditions (e.g. cpu
> > generation) also define if a CPU model is runnable, so the pure 
> > availability of
> > features does not mean that a cpu model is runnable.
> > 
> > We could have runnable=false and unavailable-features being an empty list.  
> 
> Even on those cases, can't the root cause be mapped to a QOM
> property name (e.g. "cpu-generation"), even if it's property that
> can't be changed by the user?

In the current state, no.

So I think for runnable=false:
a) unavailable-features set -> can be made runnable
b) unavailable-features not set -> cannot be made runnable

would be enough.

> 
> We could replace this with something more generic, like:
> 
> @runnability-blockers: List of attributes that prevent the CPU
>   model from running in the current host.
>   
>   A list of QOM property names that represent CPU model
>   attributes that prevent the CPU from running. If the QOM
>   property is read-only, that means the CPU model can never run
>   in the current host. If the property is read-write, it means
>   that it MAY be possible to run the CPU model in the current
>   host if that property is changed.
>   
>   Management software can use it as hints to suggest or choose an
>   alternative for the user, or just to generate meaningful error
>   messages explaining why the CPU model can't be used.
> 
> (I am looking for a better name than "runnability-blockers").
> 



David

Re: [Qemu-devel] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[David Hildenbrand]

> > > > 
> > > > Just FYI, on other architectures (e.g. s390x), other conditions (e.g. 
> > > > cpu
> > > > generation) also define if a CPU model is runnable, so the pure 
> > > > availability of
> > > > features does not mean that a cpu model is runnable.
> > > > 
> > > > We could have runnable=false and unavailable-features being an empty 
> > > > list.
> > > 
> > > Even on those cases, can't the root cause be mapped to a QOM
> > > property name (e.g. "cpu-generation"), even if it's property that
> > > can't be changed by the user?  
> > 
> > In the current state, no.  
> 
> But it could be implemented by s390x in the future, if it's
> considered useful, right?

Yes, we could fit that into read-only properties if we would ever need it
(like cpu-generation you mentioned and cpu-ga-level - both will always be
read-only).

However we could come up with more optional fields for that in the future.
(like unsupported-values or sth. like that). I actually prefer
unavailable-features over runnability-blockers.

> 
> > 
> > So I think for runnable=false:
> > a) unavailable-features set -> can be made runnable
> > b) unavailable-features not set -> cannot be made runnable
> > 
> > would be enough.  
> 
> I understand it would be enough, but I would like to at least
> define semantics that would make sense for all architectures in
> case it gets implemented in the future. Would the proposal below
> make sense?
> 

Yes, I think so. However to really make good hints, upper layers would most
likely need more information about the exact problem with a property -
maybe something like an enum value per problematic property.
(UNAVAILABLE_FEATURE, VALUE_TOO_BIG, VALUE_TOO_SMALL, UNSUPPORTED_VALUE) ...

> > > 
> > > We could replace this with something more generic, like:
> > > 
> > > @runnability-blockers: List of attributes that prevent the CPU
> > >   model from running in the current host.
> > >   
> > >   A list of QOM property names that represent CPU model
> > >   attributes that prevent the CPU from running. If the QOM
> > >   property is read-only, that means the CPU model can never run
> > >   in the current host. If the property is read-write, it means
> > >   that it MAY be possible to run the CPU model in the current
> > >   host if that property is changed.
> > >   
> > >   Management software can use it as hints to suggest or choose an
> > >   alternative for the user, or just to generate meaningful error
> > >   messages explaining why the CPU model can't be used.
> > > 
> > > (I am looking for a better name than "runnability-blockers").
> > >   

Not sure which approach would be better.

David

[Qemu-devel] [Patch v1 11/29] s390x/cpumodel: let the CPU model handle feature checks

[David Hildenbrand]

If we have certain features enabled, we have to migrate additional state
(e.g. vector registers or runtime-instrumentation registers). Let the
CPU model control that unless we have no "host" CPU model in the KVM
case. This will later on be the case for compatibility machines, so
migration from QEMU versions without the CPU model will still work.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 target-s390x/cpu_models.c | 24 
 target-s390x/cpu_models.h |  2 ++
 target-s390x/machine.c| 14 ++
 3 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index 9532b46..b698b80 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -73,6 +73,30 @@ static const S390CPUDef s390_cpu_defs[] = {
 CPUDEF_INIT(0x2965, 13, 2, 47, 0x0800U, "z13s", "IBM z13s GA1"),
 };
 
+bool s390_has_feat(S390Feat feat)
+{
+static S390CPU *cpu;
+
+if (!cpu) {
+cpu = S390_CPU(qemu_get_cpu(0));
+}
+
+if (!cpu || !cpu->model) {
+#ifdef CONFIG_KVM
+if (kvm_enabled()) {
+if (feat == S390_FEAT_VECTOR) {
+return kvm_check_extension(kvm_state, 
KVM_CAP_S390_VECTOR_REGISTERS);
+}
+if (feat == S390_FEAT_RUNTIME_INSTRUMENTATION) {
+return kvm_s390_get_ri();
+}
+}
+#endif
+return 0;
+}
+return test_bit(feat, cpu->model->features);
+}
+
 struct S390PrintCpuListInfo {
 FILE *f;
 fprintf_function print;
diff --git a/target-s390x/cpu_models.h b/target-s390x/cpu_models.h
index 244256b..fe988cc 100644
--- a/target-s390x/cpu_models.h
+++ b/target-s390x/cpu_models.h
@@ -43,4 +43,6 @@ typedef struct S390CPUModel {
 uint8_t cpu_ver;/* CPU version, usually "ff" for kvm */
 } S390CPUModel;
 
+bool s390_has_feat(S390Feat feat);
+
 #endif /* TARGET_S390X_CPU_MODELS_H */
diff --git a/target-s390x/machine.c b/target-s390x/machine.c
index aa39e5d..edc3a47 100644
--- a/target-s390x/machine.c
+++ b/target-s390x/machine.c
@@ -78,12 +78,7 @@ static const VMStateDescription vmstate_fpu = {
 
 static bool vregs_needed(void *opaque)
 {
-#ifdef CONFIG_KVM
-if (kvm_enabled()) {
-return kvm_check_extension(kvm_state, KVM_CAP_S390_VECTOR_REGISTERS);
-}
-#endif
-return 0;
+return s390_has_feat(S390_FEAT_VECTOR);
 }
 
 static const VMStateDescription vmstate_vregs = {
@@ -147,12 +142,7 @@ static const VMStateDescription vmstate_vregs = {
 
 static bool riccb_needed(void *opaque)
 {
-#ifdef CONFIG_KVM
-if (kvm_enabled()) {
-return kvm_s390_get_ri();
-}
-#endif
-return 0;
+return s390_has_feat(S390_FEAT_RUNTIME_INSTRUMENTATION);
 }
 
 const VMStateDescription vmstate_riccb = {
-- 
2.6.6

[Qemu-devel] [Patch v1 08/29] s390x/cpumodel: register defined CPU models as subclasses

[David Hildenbrand]

This patch adds the CPU model definitions that are known on s390x -
like z900, zBC12 or z13. For each definition, introduce two CPU models:

1. Base model (e.g. z13-base): Minimum feature set we expect to be around
   on all z13 systems. These models are migration-safe and will never
   change.
2. Flexible models (e.g. z13): Models that can change between QEMU versions
   and will be extended over time as we implement further features that
   are already part of such a model in real hardware of certain
   configurations.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 target-s390x/cpu-qom.h|   2 +
 target-s390x/cpu_models.c | 113 ++
 target-s390x/cpu_models.h |  36 +++
 3 files changed, 151 insertions(+)
 create mode 100644 target-s390x/cpu_models.h

diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
index bb993d4..4e936e7 100644
--- a/target-s390x/cpu-qom.h
+++ b/target-s390x/cpu-qom.h
@@ -21,6 +21,7 @@
 #define QEMU_S390_CPU_QOM_H
 
 #include "qom/cpu.h"
+#include "cpu_models.h"
 
 #define TYPE_S390_CPU "s390-cpu"
 
@@ -45,6 +46,7 @@ typedef struct S390CPUClass {
 /*< private >*/
 CPUClass parent_class;
 /*< public >*/
+const S390CPUDef *cpu_def;
 bool kvm_required;
 bool is_static;
 bool is_migration_safe;
diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index c875719..2bd5048 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -12,11 +12,66 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
+#include "gen-features.h"
 #include "qapi/error.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
 #endif
 
+#define CPUDEF_INIT(_type, _gen, _ec_ga, _mha_pow, _hmfai, _name, _desc) \
+{ \
+.name = _name,\
+.type = _type,\
+.gen = _gen,  \
+.ec_ga = _ec_ga,  \
+.mha_pow = _mha_pow,  \
+.hmfai = _hmfai,  \
+.desc = _desc,\
+.base_feat = { S390_FEAT_LIST_GEN ## _gen ## _GA ## _ec_ga ## _BASE }, 
 \
+.default_feat = { S390_FEAT_LIST_GEN ## _gen ## _GA ## _ec_ga ## 
_DEFAULT },  \
+.full_feat = { S390_FEAT_LIST_GEN ## _gen ## _GA ## _ec_ga ## _FULL }, 
 \
+}
+
+/*
+ * CPU definiton list in order of release. For now, base features of a
+ * following release are always a subset of base features of the previous
+ * release. Same is correct for the other feature sets.
+ * A BC release always follows the corresponding EC release.
+ */
+static const S390CPUDef s390_cpu_defs[] = {
+CPUDEF_INIT(0x2064, 7, 1, 38, 0xU, "z900", "IBM zSeries 900 GA1"),
+CPUDEF_INIT(0x2064, 7, 2, 38, 0xU, "z900.2", "IBM zSeries 900 
GA2"),
+CPUDEF_INIT(0x2064, 7, 3, 38, 0xU, "z900.3", "IBM zSeries 900 
GA3"),
+CPUDEF_INIT(0x2066, 7, 3, 38, 0xU, "z800", "IBM zSeries 800 GA1"),
+CPUDEF_INIT(0x2084, 8, 1, 38, 0xU, "z990", "IBM zSeries 990 GA1"),
+CPUDEF_INIT(0x2084, 8, 2, 38, 0xU, "z990.2", "IBM zSeries 990 
GA2"),
+CPUDEF_INIT(0x2084, 8, 3, 38, 0xU, "z990.3", "IBM zSeries 990 
GA3"),
+CPUDEF_INIT(0x2086, 8, 3, 38, 0xU, "z890", "IBM zSeries 880 GA1"),
+CPUDEF_INIT(0x2084, 8, 4, 38, 0xU, "z990.4", "IBM zSeries 990 
GA4"),
+CPUDEF_INIT(0x2086, 8, 4, 38, 0xU, "z890.2", "IBM zSeries 880 
GA2"),
+CPUDEF_INIT(0x2084, 8, 5, 38, 0xU, "z990.5", "IBM zSeries 990 
GA5"),
+CPUDEF_INIT(0x2086, 8, 5, 38, 0xU, "z890.3", "IBM zSeries 880 
GA3"),
+CPUDEF_INIT(0x2094, 9, 1, 40, 0xU, "z9EC", "IBM System z9 EC GA1"),
+CPUDEF_INIT(0x2094, 9, 2, 40, 0xU, "z9EC.2", "IBM System z9 EC 
GA2"),
+CPUDEF_INIT(0x2096, 9, 2, 40, 0xU, "z9BC", "IBM System z9 BC GA1"),
+CPUDEF_INIT(0x2094, 9, 3, 40, 0xU, "z9EC.3", "IBM System z9 EC 
GA3"),
+CPUDEF_INIT(0x2096, 9, 3, 40, 0xU, "z9BC.2", "IBM System z9 BC 
GA2"),
+CPUDEF_INIT(0x2097, 10, 1, 43, 0xU, "z10EC", "IBM System z10 EC 
GA1"),
+CPUDEF_INIT(0x2097, 10, 2, 43, 0xU, "z10EC.2", "IBM System z10

[Qemu-devel] [Patch v1 15/29] s390x/sclp: indicate sclp features

[David Hildenbrand]

We have three different blocks in the SCLP read-SCP information response
that indicate sclp features. Let's prepare propagation.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 hw/s390x/sclp.c   |  9 +
 target-s390x/cpu_models.c | 14 ++
 target-s390x/cpu_models.h |  1 +
 3 files changed, 24 insertions(+)

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 15d7114..3c126ee 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -31,11 +31,14 @@ static inline SCLPDevice *get_sclp_device(void)
 
 static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int count)
 {
+uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
 int i;
 
+s390_get_feat_block(S390_FEAT_TYPE_SCLP_CPU, features);
 for (i = 0; i < count; i++) {
 entry[i].address = i;
 entry[i].type = 0;
+memcpy(entry[i].features, features, sizeof(entry[i].features));
 }
 }
 
@@ -59,6 +62,12 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 read_info->offset_cpu = cpu_to_be16(offsetof(ReadInfo, entries));
 read_info->highest_cpu = cpu_to_be16(max_cpus);
 
+/* Configuration Characteristic (Extension) */
+s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR,
+ read_info->conf_char);
+s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT,
+ read_info->conf_char_ext);
+
 prepare_cpu_entries(sclp, read_info->entries, cpu_count);
 
 read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index 3fe85fa..641aad0 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -74,6 +74,20 @@ static const S390CPUDef s390_cpu_defs[] = {
 CPUDEF_INIT(0x2965, 13, 2, 47, 0x0800U, "z13s", "IBM z13s GA1"),
 };
 
+void s390_get_feat_block(S390FeatType type, uint8_t *data)
+{
+static S390CPU *cpu;
+
+if (!cpu) {
+cpu = S390_CPU(qemu_get_cpu(0));
+}
+
+if (!cpu || !cpu->model) {
+return;
+}
+return s390_fill_feat_block(cpu->model->features, type, data);
+}
+
 bool s390_has_feat(S390Feat feat)
 {
 static S390CPU *cpu;
diff --git a/target-s390x/cpu_models.h b/target-s390x/cpu_models.h
index fe988cc..04c47c6 100644
--- a/target-s390x/cpu_models.h
+++ b/target-s390x/cpu_models.h
@@ -43,6 +43,7 @@ typedef struct S390CPUModel {
 uint8_t cpu_ver;/* CPU version, usually "ff" for kvm */
 } S390CPUModel;
 
+void s390_get_feat_block(S390FeatType type, uint8_t *data);
 bool s390_has_feat(S390Feat feat);
 
 #endif /* TARGET_S390X_CPU_MODELS_H */
-- 
2.6.6

[Qemu-devel] [Patch v1 00/29] s390x CPU models: exposing features

[David Hildenbrand]

-model-comparison",
  "arguments":{
"modela":{
  "name":"z13.2-base",
  "props":{ ... }
},
"modelb":{
  "name":"zEC12.2-base",
  "props":{ ... }
  }
}}}
{"return":{
"result":"incompatible",
"responsible-properties":[
  "cei",
  "ib",
  "siif",
  "gpereh",
  "sief2",
  "ibs",
  "64bscao",
  "gsls",
  "vx",
  "dfppc",
  "gen13e",
  "gen13ptff",
  "type"
]
}}
Indicated are all z13 features + nested virtualization features. "type"
indicates that both models can never be made identical (as different
CPU generations).

Baselining both models (dropping features for readability)
{ "execute":"query-cpu-model-baseline",
  "arguments":{
"modela":{
  "name":"z13.2-base",
  "props":{ ... }
    },
    "modelb":{
  "name":"zEC12.2-base",
  "props":{ ... }

{
  "return":{
"model":{
  "name":"zEC12.2-base",
  "props":{
"aefsi":true,
"msa5":true,
"msa4":true,
"msa3":true,
"msa2":true,
"msa1":true,
"ri":true,
"edat2":true,
"edat1":true,
"ipter":true,
"esop":true,
"ctx":true,
"cmm":true,
"tx":true

Which is in fact a zEC12.2 without nested virtualization.

Changes---

RFC -> Patch v1:
- Rebased to master, had to fixup qmp_visitor and CPU ceation stuff
- Changed function calls from (*fn)(...) to fn(...)
- Mark models not only as migration-safe but also as static
- Introduced "qmp: details about CPU definitions in query-cpu-definitions"
- Expose "static" as CPU class property
- All CPU models except "host" are now migration-safe (e.g. qemu and z13)
- Replaced linux-header update by a proper kvm/next update
- Removed baseline type parameter from "query-cpu-model-baseline" to KIS
- Renamed "stable" expansion to "static" (as stable sounds misleading)
- Removed "migratable" expansion type, can be done via "migratable=off"
- Heavily improved description of the new QMP interfaces
- Actually tested all QMP interfaces. Various bugfixes.

David Hildenbrand (27):
  qmp: details about CPU definitions in query-cpu-definitions
  s390x/cpumodel: "host" and "qemu" as CPU subclasses
  s390x/cpumodel: expose CPU class properties
  s390x/cpumodel: generate CPU feature group lists
  s390x/cpumodel: introduce CPU feature group definitions
  s390x/cpumodel: register defined CPU models as subclasses
  s390x/cpumodel: store the CPU model in the CPU instance
  s390x/cpumodel: expose features and feature groups as properties
  s390x/cpumodel: let the CPU model handle feature checks
  s390x/cpumodel: check and apply the CPU model
  s390x/sclp: factor out preparation of cpu entries
  s390x/sclp: introduce sclp feature blocks
  s390x/sclp: indicate sclp features
  s390x/sclp: propagate the ibc val(lowest and unblocked ibc)
  s390x/sclp: propagate the mha via sclp
  s390x/sclp: propagate hmfai
  linux-headers: update against kvm/next
  s390x/kvm: allow runtime-instrumentation for "none" machine
  s390x/kvm: implement CPU model support
  s390x/kvm: disable host model for existing compat machines
  s390x/kvm: let the CPU model control CMM(A)
  qmp: add QMP interface "query-cpu-model-expansion"
  qmp: add QMP interface "query-cpu-model-comparison"
  qmp: add QMP interface "query-cpu-model-baseline"
  s390x/cpumodel: implement QMP interface "query-cpu-model-expansion"
  s390x/cpumodel: implement QMP interface "query-cpu-model-comparison"
  s390x/cpumodel: implement QMP interface "query-cpu-model-baseline"

Michael Mueller (2):
  s390x/cpumodel: introduce CPU features
  s390x/cpumodel: generate CPU feature lists for CPU models

 Makefile.target |2 +-
 hw/s390x/s390-virtio-ccw.c  |5 +
 hw/s390x/s390-virtio.c  |6 +-
 hw/s390x/sclp.c |   35 +-
 include/hw/s390x/sclp.h |   17 +-
 include/sysemu/arch_init.h  |9 +
 linux-headers/asm-arm64/kvm.h   |2 +
 linux-headers/asm-s390/kvm.h|   41 ++
 linux-headers/linux/kvm.h   |   12 +-
 qapi-schema.json|  200 +-
 qmp-commands.hx |   18 +
 qmp.c   |   21 +
 rules.mak   |1 +
 stubs/Makefile.objs |3 +
 stubs/arch-query-cpu-model-baseline.c   |   12 +
 stubs/arch-query-cpu-model-comparison.c |   12 +
 stubs/arch-query-cpu-model-expansion.c  |   12 +
 target-s390x/Makefile.objs  |   22 +-
 target-s390x/cpu-qom.h  |6 +
 target-s390x/cpu.c  |   35 +-
 target-s390x/cpu.h  |5 +
 target-s390x/cpu_features.c |  376 +++
 target-s390x/cpu_features.h |  302 +
 target-s390x/cpu_models.c   | 1083 +++
 target-s390x/cpu_models.h   |  113 
 target-s390x/gen-features.c |  587 +
 target-s390x/helper.c   |   33 +-
 target-s390x/kvm.c  |  346 +-
 target-s390x/machine.c  |   14 +-
 29 files changed, 3262 insertions(+), 68 deletions(-)
 create mode 100644 stubs/arch-query-cpu-model-baseline.c
 create mode 100644 stubs/arch-query-cpu-model-comparison.c
 create mode 100644 stubs/arch-query-cpu-model-expansion.c
 create mode 100644 target-s390x/cpu_features.c
 create mode 100644 target-s390x/cpu_features.h
 create mode 100644 target-s390x/cpu_models.c
 create mode 100644 target-s390x/cpu_models.h
 create mode 100644 target-s390x/gen-features.c

-- 
2.6.6

[Qemu-devel] [Patch v1 09/29] s390x/cpumodel: store the CPU model in the CPU instance

[David Hildenbrand]

A CPU model consists of a CPU definition, to which delta changes are
applied - features added or removed (e.g. z13-base,vx=on). In addition,
certain properties (e.g. cpu id) can later on change during migration
but belong into the CPU model. This data will later be filled from the
host model in the KVM case.

Therefore, store the configured CPU model inside the CPU instance, so
we can later on perform delta changes using properties.

For the "qemu" model, we emulate in TCG a z900. "host" will be
uninitialized (cpu->model == NULL) unless we have CPU model support in KVM
later on. The other models are all initialized from their definitions.
Only the "host" model can have a cpu->model == NULL.

Acked-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Signed-off-by: David Hildenbrand <d...@linux.vnet.ibm.com>
---
 target-s390x/cpu.h|  1 +
 target-s390x/cpu_models.c | 26 ++
 target-s390x/cpu_models.h | 10 ++
 3 files changed, 37 insertions(+)

diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 075bb37..3b76654 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -188,6 +188,7 @@ struct S390CPU {
 
 CPUS390XState env;
 int64_t id;
+S390CPUModel *model;
 /* needed for live migration */
 void *irqstate;
 uint32_t irqstate_saved_size;
diff --git a/target-s390x/cpu_models.c b/target-s390x/cpu_models.c
index 2bd5048..bd88a9e 100644
--- a/target-s390x/cpu_models.c
+++ b/target-s390x/cpu_models.c
@@ -154,6 +154,21 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
 
 static void s390_cpu_model_initfn(Object *obj)
 {
+S390CPU *cpu = S390_CPU(obj);
+S390CPUClass *xcc = S390_CPU_GET_CLASS(cpu);
+
+cpu->model = g_malloc0(sizeof(*cpu->model));
+/* copy the model, so we can modify it */
+cpu->model->def = xcc->cpu_def;
+if (xcc->is_static) {
+/* base model - features will never change */
+bitmap_copy(cpu->model->features, cpu->model->def->base_feat,
+S390_FEAT_MAX);
+} else {
+/* latest model - features can change */
+bitmap_copy(cpu->model->features,
+cpu->model->def->default_feat, S390_FEAT_MAX);
+}
 }
 
 #ifdef CONFIG_KVM
@@ -164,10 +179,21 @@ static void s390_host_cpu_model_initfn(Object *obj)
 
 static void s390_qemu_cpu_model_initfn(Object *obj)
 {
+S390CPU *cpu = S390_CPU(obj);
+
+cpu->model = g_malloc0(sizeof(*cpu->model));
+/* TCG emulates a z900 */
+cpu->model->def = _cpu_defs[0];
+bitmap_copy(cpu->model->features, cpu->model->def->default_feat,
+S390_FEAT_MAX);
 }
 
 static void s390_cpu_model_finalize(Object *obj)
 {
+S390CPU *cpu = S390_CPU(obj);
+
+g_free(cpu->model);
+cpu->model = NULL;
 }
 
 static bool get_is_migration_safe(Object *obj, Error **errp)
diff --git a/target-s390x/cpu_models.h b/target-s390x/cpu_models.h
index 13f7217..244256b 100644
--- a/target-s390x/cpu_models.h
+++ b/target-s390x/cpu_models.h
@@ -33,4 +33,14 @@ typedef struct S390CPUDef {
 S390FeatBitmap full_feat;
 } S390CPUDef;
 
+/* CPU model based on a CPU definition */
+typedef struct S390CPUModel {
+const S390CPUDef *def;
+S390FeatBitmap features;
+/* values copied from the "host" model, can change during migration */
+uint16_t lowest_ibc;/* lowest IBC that the hardware supports */
+uint32_t cpu_id;/* CPU id */
+uint8_t cpu_ver;/* CPU version, usually "ff" for kvm */
+} S390CPUModel;
+
 #endif /* TARGET_S390X_CPU_MODELS_H */
-- 
2.6.6