[Qemu-devel] [PATCH v2 0/5] s390x: vfio-ap: guest dedicated crypto adapters

[Qemu-devel] [PATCH v2 0/5] s390x: vfio-ap: guest dedicated crypto adapters

[Tony Krowiak]

This patch series is the QEMU counterpart to the KVM/kernel support for 
guest dedicated crypto adapters. The KVM/kernel model is built on the 
VFIO mediated device framework and provides the infrastructure for 
granting exclusive guest access to crypto devices installed on the linux 
host. This patch series introduces a new QEMU command line option, QEMU 
object model and CPU model features to exploit the KVM/kernel model.

See the detailed specifications for AP virtualization provided by this 
patch set in docs/vfio-ap.txt for a more complete discussion of the 
design introduced by this patch series.

v1 -> v2 Change log:
===================
* Removed unnecessary S390APMatrixDevice, S390APMatrixDeviceClass 
* Removed ioctl to configure the AP matrix for the guest: letting the
  vfio_ap device driver's 'open' callback configure the AP matrix
  for the guest
* Removed masks from object model: Unnecessary at this point because they 
  are not currently used 
* Renamed:
  * VFIOAPMatrixDevice to VFIOAPDevice
  * VFIOAPMatrixDeviceClass to VFIOAPDeviceClass
  * APMatrixDevice to APDevice
  * APMatrixDeviceClass to APDeviceClass
  * ap-matrix.c to ap.c (in hw/vfio)
  * ap-matrix-device.c to ap-device.c (in hw/s390x)
  * ap-matrix-device.h to ap-device.h (in include/hw/s390x)
* Added CPU model feature for AP facilities installed on guest and 
  facilities features for QCI Instructions Available (STFLE.12) and AP 
  Facilities Test facility installed (STFLE.15).

Tony Krowiak (5):
  s390x/ap: base Adjunct Processor (AP) object
  s390x/vfio: ap: VFIO: linux header updates
  s390x/vfio: ap: Introduce VFIO AP device
  s390x/cpumodel: Set up CPU model for AP device support
  s390: doc: detailed specifications for AP virtualization

 default-configs/s390x-softmmu.mak |    1 +
 docs/vfio-ap.txt                  |  540 +++++++++++++++++++++++++++++++++++++
 hw/s390x/Makefile.objs            |    1 +
 hw/s390x/ap-device.c              |   38 +++
 hw/vfio/Makefile.objs             |    1 +
 hw/vfio/ap.c                      |  176 ++++++++++++
 include/hw/s390x/ap-device.h      |   38 +++
 include/hw/vfio/vfio-common.h     |    1 +
 linux-headers/asm-s390/kvm.h      |    1 +
 linux-headers/linux/vfio.h        |    2 +
 target/s390x/cpu_features.c       |    3 +
 target/s390x/cpu_features_def.h   |    3 +
 target/s390x/cpu_models.c         |   12 +
 target/s390x/gen-features.c       |    3 +
 target/s390x/kvm.c                |    6 +
 15 files changed, 826 insertions(+), 0 deletions(-)
 create mode 100644 docs/vfio-ap.txt
 create mode 100644 hw/s390x/ap-device.c
 create mode 100644 hw/vfio/ap.c
 create mode 100644 include/hw/s390x/ap-device.h

[Qemu-devel] [PATCH v2 1/5] s390: doc: detailed specifications for AP virtualization

[Tony Krowiak]

This patch provides documentation describing the AP architecture and
design concepts behind the virtualization of AP devices. It also
includes an example of how to configure AP devices for exclusive
use of KVM guests.

Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
---
 docs/vfio-ap.txt |  624 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 624 insertions(+), 0 deletions(-)
 create mode 100644 docs/vfio-ap.txt

diff --git a/docs/vfio-ap.txt b/docs/vfio-ap.txt
new file mode 100644
index 0000000..54e7523
--- /dev/null
+++ b/docs/vfio-ap.txt
@@ -0,0 +1,624 @@
+Adjunct Processor (AP) Device
+=============================
+
+Contents:
+=========
+* Introduction
+* AP Architectural Overview
+* Start Interpretive Execution (SIE) Instruction
+* AP Matrix Configuration on Linux Host
+* AP Matrix Configuration for a Linux Guest
+* Starting a Linux Guest Configured with an AP Matrix
+* Example: Configure AP Matrices for Two Linux Guests
+
+Introduction:
+============
+The IBM Adjunct Processor (AP) Cryptographic Facility is comprised
+of three AP instructions and from 1 to 256 PCIe cryptographic adapter cards.
+These AP devices provide cryptographic functions to all CPUs assigned to a
+linux system running in an IBM Z system LPAR.
+
+On s390x, AP adapter cards are exposed via the AP bus. This document
+describes how those cards may be made available to KVM guests using the
+VFIO mediated device framework.
+
+AP Architectural Overview:
+=========================
+In order understand the terminology used in the rest of this document, let's
+start with some definitions:
+
+* AP adapter
+
+  An AP adapter is an IBM Z adapter card that can perform cryptographic
+  functions. There can be from 0 to 256 adapters assigned to an LPAR. Adapters
+  assigned to the LPAR in which a linux host is running will be available to
+  the linux host. Each adapter is identified by a number from 0 to 255. When
+  installed, an AP adapter is accessed by AP instructions executed by any CPU.
+
+* AP domain
+
+  An adapter is partitioned into domains. Each domain can be thought of as
+  a set of hardware registers for processing AP instructions. An adapter can
+  hold up to 256 domains. Each domain is identified by a number from 0 to 255.
+  Domains can be further classified into two types:
+
+    * Usage domains are domains that can be accessed directly to process AP
+      commands
+
+    * Control domains are domains that are accessed indirectly by AP
+      commands sent to a usage domain to control or change the domain, for
+      example; to set a secure private key for the domain.
+
+* AP Queue
+
+  An AP queue is the means by which an AP command-request message is sent to an
+  AP usage domain inside a specific AP. An AP queue is identified by a tuple
+  comprised of an AP adapter ID (APID) and an AP queue index (APQI). The
+  APQI corresponds to a given usage domain number within the adapter. This tuple
+  forms an AP Queue Number (APQN) uniquely identifying an AP queue. AP
+  instructions include a field containing the APQN to identify the AP queue to
+  which the AP command-request message is to be sent for processing.
+
+* AP Instructions:
+
+  There are three AP instructions:
+
+  * NQAP: to enqueue an AP command-request message to a queue
+  * DQAP: to dequeue an AP command-reply message from a queue
+  * PQAP: to administer the queues
+
+Start Interpretive Execution (SIE) Instruction
+==============================================
+A KVM guest is started by executing the Start Interpretive Execution (SIE)
+instruction. The SIE state description is a control block that contains the
+state information for a KVM guest and is supplied as input to the SIE
+instruction. The SIE state description contains a field that references
+a Crypto Control Block (CRYCB). The CRYCB contains three fields to identify the
+adapters, usage domains and control domains assigned to the KVM guest:
+
+* The AP Mask (APM) field is a bit mask that identifies the AP adapters assigned
+  to the KVM guest. Each bit in the mask, from most significant to least
+  significant bit, corresponds to an APID from 0-255. If a bit is set, the
+  corresponding adapter is valid for use by the KVM guest.
+
+* The AP Queue Mask (AQM) field is a bit mask identifying the AP queues assigned
+  to the KVM guest. Each bit in the mask, from most significant to least
+  significant bit, corresponds to an AP queue index (APQI) from 0-255. If a bit
+  is set, the corresponding queue is valid for use by the KVM guest.
+
+* The AP Domain Mask field is a bit mask that identifies the AP control domains
+  assigned to the KVM guest. The ADM bit mask controls which domains can be
+  changed by an AP command-request message sent to a usage domain from the
+  guest. Each bit in the mask, from least significant to most significant bit,
+  corresponds to a domain from 0-255. If a bit is set, the corresponding domain
+  can be modified by an AP command-request message sent to a usage domain
+  configured for the KVM guest.
+
+If you recall from the description of an AP Queue, AP instructions include
+an APQN to identify the AP adapter and AP queue to which an AP command-request
+message is to be sent (NQAP and PQAP instructions), or from which a
+command-reply message is to be received (DQAP instruction). The validity of an
+APQN is defined by the matrix calculated from the APM and AQM; it is the
+intersection of all assigned adapter numbers (APM) with all assigned queue
+indexes (AQM). For example, if adapters 1 and 2 and usage domains 5 and 6 are
+assigned to a guest, the APQNs (1,5), (1,6), (2,5) and (2,6) will be valid for
+the guest.
+
+The APQNs provide secure key functionality - i.e., a private key is stored on
+the adapter card for each of its domains - so each APQN must be assigned to at
+most one guest or the linux host.
+
+   Example 1: Valid configuration:
+   ------------------------------
+   Guest1: adapters 1,2  domains 5,6
+   Guest2: adapter  1,2  domain 7
+
+   This is valid because both guests have a unique set of APQNs: Guest1 has
+   APQNs (1,5), (1,6), (2,5) and (2,6); Guest2 has APQNs (1,7) and (2,7).
+
+   Example 2: Invalid configuration:
+   --------------------------------
+   Guest1: adapters 1,2  domains 5,6
+   Guest2: adapter  1    domains 6,7
+
+   This is an invalid configuration because both guests have access to
+   APQN (1,6).
+
+AP device Configuration on Linux Host:
+=====================================
+A linux system is a guest of the LPAR in which it is running and has access to
+the AP resources configured for the LPAR. The LPAR's AP matrix is
+configured using the 'Customize/Delete Activation Profiles' dialog from the HMC.
+This dialog displays the activation profiles configured for the linux system.
+Selecting the specific activation profile to be edited and clicking the
+'Customize Profile' button will open the 'Customize Image Profiles' dialog.
+Selecting the 'Crypto' link in the tree view on the left hand side of the dialog
+will display the AP matrix configuration in the right hand panel. There, one can
+assign AP adapters - called Cryptos - and domains to the LPAR. When the linux
+system is started using this activation profile, it will have access to the
+matrix of AP adapters and domains configured via the activation profile.
+
+When the linux system is started, the AP adapter devices will be connected to
+the AP bus and the following AP matrix interfaces will be created in sysfs:
+
+/sys/bus/ap
+... [devices]
+...... xx.yyyy
+...... ...
+...... cardxx
+...... ...
+
+Where:
+    cardxx     is adapter number xx (in hex)
+    yyyy       is a usage domain number yyyy (in hex)
+....xx.yyyy    is APQN (xx,yyyy)
+
+For example, if AP adapters 5 and 6 and domains 4 and 71 (0x47) are configured
+for the LPAR, the sysfs representation on the linux system would look like this:
+
+/sys/bus/ap
+... [devices]
+...... 05.0004
+...... 05.0047
+...... 06.0004
+...... 06.0047
+...... card05
+...... card06
+
+There will also be AP device drivers created to control each type of AP matrix
+interface available to the IBM Z system:
+
+/sys/bus/ap
+... [drivers]
+...... [cex2acard]        for Crypto Express 2/3 accelerator cards
+...... [cex2aqueue]       for AP queues served by Crypto Express 2/3
+                          accelerator cards
+...... [cex4card]         for Crypto Express 4/5/6 accelerator and coprocessor
+                          cards
+...... [cex4queue]        for AP queues served by Crypto Express 4/5/6
+                          accelerator and coprocessor cards
+...... [pcixcccard]       for Crypto Express 2/3 coprocessor cards
+...... [pcixccqueue]      for AP queues served by Crypto Express 2/3
+                          coprocessor cards
+
+Links to the AP interfaces controlled by each AP device driver will be created
+in the device driver's sysfs directory. For example, if AP adapter 5 and domains
+4 and 71 (0x47) are assigned to the LPAR and adapter 5 is a CEX5 card, the
+following links will be created in the CEX5 drivers' sysfs directories:
+
+/sys/bus/ap
+... [drivers]
+...... [cex4card]
+......... [card05]
+...... [cex4queue]
+......... [05.0004]
+......... [05.0047]
+
+AP Matrix Configuration for a Linux Guest:
+=========================================
+In order to configure the AP matrix for a guest, the adapters, usage domains
+and control domains to be used by the guest must be assigned to the guest. This
+section describes how to configure a guest's AP matrix.
+
+The kernel interfaces for configuring an AP matrix for a linux guest are built
+on the VFIO mediated device framework and are provided by the vfio_ap
+kernel module. By default, the vfio_ap module is a loadable module, The
+dependency chain for the vfio_ap module is:
+* vfio
+* mdev
+* vfio_mdev
+* vfio_ap
+
+When installed, the vfio_ap module is initialized. During module initialization,
+a vfio_ap driver is created and registered with the AP bus creating the
+following sysfs interfaces:
+
+ /sys/bus/ap/drivers/
+...[vfio_ap]
+...... bind
+...... unbind
+
+The vfio_ap device driver will create a 'matrix' device to hold the APQNs
+reserved for exclusive use by KVM guests:
+
+/sys/devices/
+... [vfio_ap]
+......[matrix] symlink to the matrix device directory
+
+The vfio_ap device driver serves several purposes:
+1. Provides an interface for securing APQNs preventing their use by the host
+   linux system and reserving their use by one or more guests.
+2. Creates the sysfs interfaces for configuring an AP matrix for a linux guest.
+
+Securing APQNs
+--------------
+   An APQN is reserved by unbinding an AP queue device AP bus device driver and
+   binding it to the vfio_ap device driver. For example, suppose we want to
+   secure APQN (05,0004). Assuming that the AP adapter card 5 is a CEX5
+   coprocessor card:
+
+   echo 05.0004 > /sys/bus/ap/drivers/cex4queue/unbind
+   echo 05.0004 > /sys/bus/ap/drivers/vfio_ap/bind
+
+   This action will store the APQN in the /sys/devices/vfio_ap/matrix device
+   which makes it available for use by a linux guest.
+
+Configuring an AP matrix for a linux guest.
+------------------------------------------
+These sysfs interfaces are built on the VFIO mediated device framework. To
+configure an AP matrix for a guest, a mediated matrix device must first be
+created for the /sys/devices/vfio_ap/matrix device. The sysfs interfaceAPQI corresponding to
+for creating a mediated matrix device is in:
+
+/sys/devices
+... [vfio_ap]
+......[matrix]
+......... [mdev_supported_types]
+............ [vfio_ap-passthrough]
+............... create
+............... [devices]
+
+A mediated AP matrix device is created by writing a UUID to the attribute
+file named 'create', for example:
+
+   uuidgen > create
+
+When a mediated AP matrix device is created, a sysfs directory named after
+the UUID:
+
+/sys/devices
+... [vfio_ap]
+......[matrix]
+......... [mdev_supported_types]
+............ [vfio_ap-passthrough]
+............... create
+............... [devices]
+.................. [$uuid]
+
+There will also be three sets of attribute files created in the mediated
+matrix device's sysfs directory to configure an AP matrix for the
+KVM guest:
+
+/sys/devices
+... [vfio_ap]
+......[matrix]
+......... [mdev_supported_types]
+............ [vfio_ap-passthrough]
+............... create
+............... [devices]
+.................. [$uuid]
+..................... assign_adapter
+..................... assign_control_domain
+..................... assign_domain
+..................... matrix
+..................... unassign_adapter
+..................... unassign_control_domain
+..................... unassign_domain
+
+assign_adapter
+   To assign an AP adapter to the mediated matrix device, its APID is written
+   'assign_adapter' file. This may be done multiple times to assign more than
+   one adapter. The APID may be specified using conventional semantics
+   as a decimal, hexidecimal, or octal number. For example, to assign adapters
+   4, 5 and 16 to mediated matrix device $uuid in decimal, hexidecimal and octal
+   respectively:
+
+       echo 4 > assign_adapter
+       echo 0x5 > assign_adapter
+       echo 020
+
+unassign_adapter
+   To unassign an AP adapter, its APID is written to the 'unassign_adapter'
+   file. This may also be done multiple times to unassign more than one adapter.
+
+assign_domain
+   To assign a usage domain, the APQI corresponding to the domain number is
+   written into the 'assign_domain' file. This may be done multiple times to
+   assign more than one usage domain. The APQI may be specified using
+   conventional semantics as a decimal, hexidecimal, or octal number. For
+   example, to assign usage domains 4, 8, and 71 to mediated matrix device
+   $uuid in decimal, hexidecimal and octal respectively:
+
+      echo 4 > assign_domain
+      echo 0x8 > assign_domain
+      echo 0107 > assign_domain
+
+unassign_domain
+   To unassign a usage domain, the APQI corresponding to the domain number is
+   written into the 'unassign_domain' file. This may be done multiple times to
+   unassign more than one usage domain.
+
+assign_control_domain
+   To assign a control domain, the domain number is written into the
+   'assign_control_domain' file. This may be done multiple times to
+   assign more than one control domain. The domain number may be specified using
+   conventional semantics as a decimal, hexidecimal, or octal number. For
+   example, to assign  control domains 4, 8, and 71 to mediated matrix device
+   $uuid in decimal, hexidecimal and octal respectively:
+
+      echo 4 > assign_domain
+      echo 0x8 > assign_domain
+      echo 0107 > assign_domain
+
+unassign_control_domain
+   To unassign a control domain, the domain number is written into the
+   'unassign_domain' file. This may be done multiple times to unassign more than
+   one control domain.
+
+Notes:
+* Hot plug/unplug is not currently supported for mediated AP matrix devices,
+  so the AP matrix resulting from assignment and/or unassignment of AP
+  adapters, usage domains and control domains to a mediated AP matrix device
+  while the guest is running will not take affect until the linux guest is
+  rebooted.
+* By architectural convention, all usage domains configured for a KVM guest
+  will also be implicitly assigned as control domains also, to there is no
+  need to assign control domains that are assigned as usage domains.
+
+Starting a Linux Guest Configured with an AP Matrix:
+===================================================
+In addition to providing the sysfs interfaces for configuring the AP matrix for
+a linux guest, a mediated matrix device also acts as a communication pathway
+between QEMU and the vfio_ap device driver. To gain access to the
+device driver, the following option must be specified on the QEMU command line:
+
+   -device vfio_ap,sysfsdev=$path-to-mdev
+
+The sysfsdev parameter specifies the path to the mediated matrix device.
+There are a number of ways to specify this path:
+
+/sys/devices/vfio_ap/matrix/$uuid
+/sys/bus/mdev/devices/$uuid
+/sys/bus/mdev/drivers/vfio_mdev/$uuid
+/sys/devices/vfio_ap/matrix/mdev_supported_types/vfio_ap-passthrough/devices/$uuid
+
+When the linux guest is subsequently started, the guest will open the mediated
+matrix device's file descriptor to get information about the mediated matrix
+device. The vfio_ap device driver will update the APM, AQM, and ADM fields in the
+guest's CRYCB with the adapter, usage domain and control domains assigned to
+via the mediated matrix device's sysfs attribute files. Programs running on the
+linux guest will then:
+
+1. Have direct access to the APQNs derived from the intersection of the AP
+   adapter and usage domain numbers specified in the APM and AQM respectively
+
+2. Have authorization to process AP commands to change - e.g., store a new
+   secure key - a control domain identified in an AP instruction sent to a valid
+   APQN.
+
+CPU model features:
+
+Three CPU model features are available for controlling guest access to AP
+facilities:
+
+1. AP facilities feature
+
+   The AP facilities feature indicates that AP facilities are installed on the
+   guest. This feature will be enabled by the kernel only if the AP facilities
+   are installed on the host system. It will turned on automatically for guests
+   started with CPU model zEC12 or newer. The feature is s390-specific and is
+   represented as a parameter of the -cpu option on the QEMU command line:
+
+      qemu-system-s390x -cpu $model,ap=on|off
+
+      Where:
+
+         $model is the CPU model defined for the guest (defaults to the model of
+                the host system if not specified).
+
+         ap=on|off indicates whether AP facilities are installed (on) or not
+                   installed (off). The default for CPU models zEC12 or newer
+                   is ap=on. AP facilities must be installed when this parameter
+                   is used in conjunction with -device vfio-ap,sysfsdev=$path or
+                   the guest will not start.
+
+2. Query Configuration Information (QCI) facility
+
+   The QCI facility is used by the AP bus running on the guest to query the
+   configuration of the AP facilities. This facility will be enabled by
+   the kernel only if the QCI facility is installed on the host system. It will
+   be turned on automatically for guests started with CPU model zEC12 or newer.
+   The feature is s390-specific and is represented as a parameter of the -cpu
+   option on the QEMU command line:
+
+      qemu-system-s390x -cpu $model,qci=on|off
+
+      Where:
+
+         $model is the CPU model defined for the guest
+
+         qci=on|off indicates whether the QCI facility is installed (on) or not
+                    installed (off). The default for CPU models zEC12 or newer
+                    is qci=on. Turning the QCI facility on makes no sense if it
+                    is not used in conjunction with the
+                    '-device vfio-ap,sysfsdev=$path' option. A warning will be
+                    presented if QCI is turned on and the AP facilities are not
+                    installed.
+
+                    If the QCI facility is turned off, APQNs with an APQI
+                    greater than 15 will not be accessible from the guest.
+
+3. Adjunct Process Facility Test (APFT) facility
+
+   The APFT facility is used by the AP bus running on the guest to test the
+   AP facilities available for a given AP queue. This facility will be enabled
+   by the kernel only if the APFT facility is installed on the host system. It
+   will be turned on automatically for guests started with CPU model zEC12 or
+   newer. The feature is s390-specific and is represented as a parameter of the
+   -cpu option on the QEMU command line:
+
+      qemu-system-s390x -cpu $model,apft=on|off
+
+      Where:
+
+         $model is the CPU model defined for the guest (defaults to the model of
+                the host system if not specified).
+
+         apft=on|off indicates whether the APFT facility is installed (on) or
+                     not installed (off). The default for CPU models zEC12 and
+                     newer is apft=on. Turning the APFT facility on makes no
+                     sense if it is not used in conjunction with the
+                     -device vfio-ap,sysfsdev=$path option. A warning will be
+                     presented if APFT is turned on and the AP facilities are
+                     not installed.
+
+                     It also makes no sense to turn APFT off when used in
+                     conjunction with the vfio-ap device because the APFT
+                     facility is required; the AP bus running on the guest will
+                     not detect CEX4 and newer devices without it. Since only
+                     CEX4 and newer devices are supported for guest usage, no AP
+                     devices can be made accessible to a guest started without
+                     APFT installed.
+
+Example: Configure AP Matrixes for Two Linux Guests:
+===================================================
+Let's now provide an example to illustrate how KVM guests may be given
+access to AP facilities. For this example, we will show how to configure
+two guests such that executing the lszcrypt command on the guests would
+look like this:
+
+Guest1
+------
+CARD.DOMAIN TYPE  MODE       
+------------------------------
+05          CEX5C CCA-Coproc 
+05.0004     CEX5C CCA-Coproc
+05.00ab     CEX5C CCA-Coproc 
+06          CEX5A Accelerator
+06.0004     CEX5A Accelerator
+06.00ab     CEX5C CCA-Coproc 
+
+Guest2
+------
+CARD.DOMAIN TYPE  MODE       
+------------------------------
+05          CEX5A Accelerator
+05.0047     CEX5A Accelerator
+05.00ff     CEX5A Accelerator
+
+These are the steps for configuring the Guest1 and Guest2:
+
+1. The first thing that needs to be done is to secure the AP queues to be
+   used by the two guests so that the host can not access them. This is done
+   by unbinding each AP Queue device from its respective AP driver. In our
+   example, these queues are bound to the cex4queue driver. This would be
+   the sysfs location of these devices:
+
+   /sys/bus/ap
+   --- [drivers]
+   ------ [cex4queue]
+   --------- [05.0004]
+   --------- [05.0047]   --------------------- control_domains
+   --------------------- domains
+   --------- [05.00ab]
+   --------- [05.00ff]
+   --------- [06.0004]
+   --------- [06.00ab]
+   --------- unbind
+
+   To unbind AP queue 05.0004 from the cex4queue device driver:
+
+    echo 05.0004 > unbind
+
+   This must also be done for AP queues 05.00ab, 05.0047, 05.00ff, 06.0004,
+   and 06.00ab.
+
+2. The next step is to reserve the queues for use by the two KVM guests.
+   This is accomplished by binding them to the VFIO AP device driver.
+   This is the sysfs location of the VFIO AP device driver:
+
+   /sys/bus/ap
+   ---[drivers]
+   ------ [vfio_ap]
+   ---------- bind
+
+   To bind queue 05.0004 to the vfio_ap driver:
+
+    echo 05.0004 > bind
+
+   This must also be done for AP queues 05.00ab, 05.0047, 05.00ff, 06.0004,
+   and 06.00ab.
+
+3. Create the mediated devices needed to configure the AP matrixes for the
+   two guests and to provide an interface to the vfio_ap driver for
+   use by the guests:
+
+   /sys/devices/
+   --- [vfio_ap]
+   ------ [matrix] (this is the matrix device)
+   --------- [mdev_supported_types]
+   ------------ [vfio_ap-passthrough] (passthrough mediated matrix device type)
+   --------------- create
+   --------------- [devices]
+
+   To create the mediated devices for the two guests:
+
+    uuidgen > create
+    uuidgen > create
+
+   This will create two mediated devices in the [devices] subdirectory named
+   with the UUID written to the create attribute file. We call them $uuid1
+   and $uuid2:
+
+   /sys/devices/
+   --- [vfio_ap]
+   ------ [matrix]
+   --------- [mdev_supported_types]
+   ------------ [vfio_ap-passthrough]
+   --------------- [devices]
+   ------------------ [$uuid1]
+   --------------------- assign_adapter
+   --------------------- assign_control_domain
+   --------------------- assign_domain
+   --------------------- matrix
+   --------------------- unassign_adapter
+   --------------------- unassign_control_domain
+   --------------------- unassign_domain
+
+   ------------------ [$uuid2]
+   --------------------- assign_adapter
+   --------------------- assign_cTo assign an adapter, the APID of the adapter is written to the
+      file. ontrol_domain
+   --------------------- assign_domain
+   --------------------- matrix
+   --------------------- unassign_adapter
+   --------------------- unassign_control_domain
+   --------------------- unassign_domain
+
+4. The administrator now needs to configure the matrixes for mediated
+   devices $uuid1 (for Guest1) and $uuid2 (for Guest2).
+
+   This is how the matrix is configured for Guest1:
+
+   echo 5 > assign_adapter
+   echo 6 > assign_adapter
+   echo 4 > assign_domain
+   echo 0xab > assign_domain
+
+   By architectural convention, all usage domains - i.e., domains assigned
+   via the assign_domain attribute file - will also be configured in the ADM
+   field of the KVM guest's CRYCB, so there is no need to assign control
+   domains here unless you want to assign control domains that are not
+   assigned as usage domains.
+
+   If a mistake is made configuring an adapter, domain or control domain,
+   you can use the unassign_xxx files to unassign the adapter, domain or
+   control domain.
+
+   To display the matrix configuration for Guest1:
+
+   cat matrix
+
+   This is how the matrix is configured for Guest2:
+
+   echo 5 > assign_adapter
+   echo 0x47 > assign_domain
+   echo 0xff > assign_domain
+
+5. Start Guest1
+
+   /usr/bin/qemu-system-s390x ... -device vfio-ap,sysfsdev=/sys/devices/vfio_ap/matrix/$uuid1 ...
+
+6. Start Guest2
+
+   /usr/bin/qemu-system-s390x ... -device vfio-ap,sysfsdev=/sys/devices/vfio_ap/matrix/$uuid2 ...
-- 
1.7.1

[Qemu-devel] [PATCH v2 2/5] s390x/ap: base Adjunct Processor (AP) object

[Tony Krowiak]

This patch introduces the base object for an AP device.

Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
---
 hw/s390x/Makefile.objs       |    1 +
 hw/s390x/ap-device.c         |   38 ++++++++++++++++++++++++++++++++++++++
 include/hw/s390x/ap-device.h |   38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 0 deletions(-)
 create mode 100644 hw/s390x/ap-device.c
 create mode 100644 include/hw/s390x/ap-device.h

diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index dc704b5..3247a07 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -17,3 +17,4 @@ obj-y += s390-stattrib.o
 obj-$(CONFIG_KVM) += s390-skeys-kvm.o
 obj-$(CONFIG_KVM) += s390-stattrib-kvm.o
 obj-y += s390-ccw.o
+obj-y += ap-device.o
diff --git a/hw/s390x/ap-device.c b/hw/s390x/ap-device.c
new file mode 100644
index 0000000..dda0d74
--- /dev/null
+++ b/hw/s390x/ap-device.c
@@ -0,0 +1,38 @@
+/*
+ * Adjunct Processor (AP) matrix device
+ *
+ * Copyright 2017 IBM Corp.
+ * Author(s): Tony Krowiak <akrowiak@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 "qemu/module.h"
+#include "qapi/error.h"
+#include "hw/qdev.h"
+#include "hw/s390x/ap-device.h"
+
+static void ap_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->desc = "AP device class";
+}
+
+static const TypeInfo ap_device_info = {
+    .name = AP_DEVICE_TYPE,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(APDevice),
+    .class_size = sizeof(APDeviceClass),
+    .class_init = ap_class_init,
+    .abstract = true,
+};
+
+static void ap_device_register(void)
+{
+    type_register_static(&ap_device_info);
+}
+
+type_init(ap_device_register)
diff --git a/include/hw/s390x/ap-device.h b/include/hw/s390x/ap-device.h
new file mode 100644
index 0000000..64fb343
--- /dev/null
+++ b/include/hw/s390x/ap-device.h
@@ -0,0 +1,38 @@
+/*
+ * Adjunct Processor (AP) matrix device interfaces
+ *
+ * Copyright 2017 IBM Corp.
+ * Author(s): Tony Krowiak <akrowiak@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.
+ */
+#ifndef HW_S390X_AP_DEVICE_H
+#define HW_S390X_AP_DEVICE_H
+
+#define AP_DEVICE_TYPE       "ap-device"
+
+typedef struct APDevice {
+    DeviceState parent_obj;
+} APDevice;
+
+typedef struct APDeviceClass {
+    DeviceClass parent_class;
+} APDeviceClass;
+
+static inline APDevice *to_ap_dev(DeviceState *dev)
+{
+    return container_of(dev, APDevice, parent_obj);
+}
+
+#define AP_DEVICE(obj) \
+    OBJECT_CHECK(APDevice, (obj), AP_DEVICE_TYPE)
+
+#define AP_DEVICE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(APDeviceClass, (obj), AP_DEVICE_TYPE)
+
+#define AP_DEVICE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(APDeviceClass, (klass), AP_DEVICE_TYPE)
+
+#endif /* HW_S390X_AP_DEVICE_H */
-- 
1.7.1

[Qemu-devel] [PATCH v2 3/5] s390x/vfio: ap: VFIO: linux header updates

[Tony Krowiak]

Updates the vfio header files in preparation for introduction
of the VFIO AP device.

Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
---
 linux-headers/linux/vfio.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index 4312e96..91298dc 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -200,6 +200,7 @@ struct vfio_device_info {
 #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)	/* vfio-platform device */
 #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)	/* vfio-amba device */
 #define VFIO_DEVICE_FLAGS_CCW	(1 << 4)	/* vfio-ccw device */
+#define VFIO_DEVICE_FLAGS_AP (1 << 5)		/* vfio-ap device */
 	__u32	num_regions;	/* Max region index + 1 */
 	__u32	num_irqs;	/* Max IRQ index + 1 */
 };
@@ -215,6 +216,7 @@ struct vfio_device_info {
 #define VFIO_DEVICE_API_PLATFORM_STRING		"vfio-platform"
 #define VFIO_DEVICE_API_AMBA_STRING		"vfio-amba"
 #define VFIO_DEVICE_API_CCW_STRING		"vfio-ccw"
+#define VFIO_DEVICE_API_AP_STRING		"vfio-ap"
 
 /**
  * VFIO_DEVICE_GET_REGION_INFO - _IOWR(VFIO_TYPE, VFIO_BASE + 8,
-- 
1.7.1

[Qemu-devel] [PATCH v2 4/5] s390x/vfio: ap: Introduce VFIO AP device

[Tony Krowiak]

Introduces a VFIO based AP device. The device is defined via
the QEMU command line by specifying:

    -device vfio-ap,sysfsdev=<path-to-mediated-matrix-device>

The mediated matrix device is created by the VFIO AP device
driver by writing a UUID to a sysfs attribute file (see
docs/vfio-ap.txt). The mediated matrix device will be named
after the UUID. Symbolic links to the $uuid are created in
many places, so the path to the mediated matrix device $uuid
can be specified in any of the following ways:

/sys/devices/vfio_ap/matrix/$uuid
/sys/devices/vfio_ap/matrix/mdev_supported_types/vfio_ap-passthrough/devices/$uuid
/sys/bus/mdev/devices/$uuid
/sys/bus/mdev/drivers/vfio_mdev/$uuid

When the vfio-ap device is realized, it acquires and opens the
VFIO iommu group to which the mediated matrix device is
bound. This causes a VFIO group notification event to be
signaled. The vfio_ap device driver's group notification
handler will get called at which time the device driver
will configure the the AP devices to which the guest will
be granted access.

Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
---
 default-configs/s390x-softmmu.mak |    1 +
 hw/vfio/Makefile.objs             |    1 +
 hw/vfio/ap.c                      |  167 +++++++++++++++++++++++++++++++++++++
 include/hw/vfio/vfio-common.h     |    1 +
 4 files changed, 170 insertions(+), 0 deletions(-)
 create mode 100644 hw/vfio/ap.c

diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
index 2f4bfe7..0b784b6 100644
--- a/default-configs/s390x-softmmu.mak
+++ b/default-configs/s390x-softmmu.mak
@@ -9,3 +9,4 @@ CONFIG_S390_FLIC=y
 CONFIG_S390_FLIC_KVM=$(CONFIG_KVM)
 CONFIG_VFIO_CCW=$(CONFIG_LINUX)
 CONFIG_WDT_DIAG288=y
+CONFIG_VFIO_AP=$(CONFIG_LINUX)
diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
index c3ab909..7300860 100644
--- a/hw/vfio/Makefile.objs
+++ b/hw/vfio/Makefile.objs
@@ -6,4 +6,5 @@ obj-$(CONFIG_SOFTMMU) += platform.o
 obj-$(CONFIG_VFIO_XGMAC) += calxeda-xgmac.o
 obj-$(CONFIG_VFIO_AMD_XGBE) += amd-xgbe.o
 obj-$(CONFIG_SOFTMMU) += spapr.o
+obj-$(CONFIG_VFIO_AP) += ap.o
 endif
diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
new file mode 100644
index 0000000..8aa5221
--- /dev/null
+++ b/hw/vfio/ap.c
@@ -0,0 +1,167 @@
+/*
+ * VFIO based AP matrix device assignment
+ *
+ * Copyright 2017 IBM Corp.
+ * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or(at
+ * your option) any version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#include <linux/vfio.h>
+#include <sys/ioctl.h>
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/sysbus.h"
+#include "hw/vfio/vfio.h"
+#include "hw/vfio/vfio-common.h"
+#include "hw/s390x/ap-device.h"
+#include "qemu/error-report.h"
+#include "qemu/queue.h"
+
+#define VFIO_AP_DEVICE_TYPE      "vfio-ap"
+#define AP_SYSFSDEV_PROP_NAME    "sysfsdev"
+
+typedef struct VFIOAPDevice {
+    APDevice apdev;
+    VFIODevice vdev;
+    QTAILQ_ENTRY(VFIOAPDevice) sibling;
+} VFIOAPDevice;
+
+static void vfio_ap_compute_needs_reset(VFIODevice *vdev)
+{
+    vdev->needs_reset = false;
+}
+
+/*
+ * We don't need vfio_hot_reset_multi and vfio_eoi operations for
+ * vfio-ap-matrix device now.
+ */
+struct VFIODeviceOps vfio_ap_ops = {
+    .vfio_compute_needs_reset = vfio_ap_compute_needs_reset,
+};
+
+static QTAILQ_HEAD(, VFIOAPDevice) vfio_ap_devices =
+        QTAILQ_HEAD_INITIALIZER(vfio_ap_devices);
+
+static void vfio_put_device(VFIOAPDevice *apdev)
+{
+    g_free(apdev->vdev.name);
+    vfio_put_base_device(&apdev->vdev);
+}
+
+static VFIOGroup *vfio_ap_get_group(VFIOAPDevice *vapdev, Error **errp)
+{
+    char *tmp, group_path[PATH_MAX];
+    ssize_t len;
+    int groupid;
+
+    tmp = g_strdup_printf("%s/iommu_group", vapdev->vdev.sysfsdev);
+    len = readlink(tmp, group_path, sizeof(group_path));
+    g_free(tmp);
+
+    if (len <= 0 || len >= sizeof(group_path)) {
+        error_setg(errp, "%s: no iommu_group found for %s",
+                   VFIO_AP_DEVICE_TYPE, vapdev->vdev.sysfsdev);
+        return NULL;
+    }
+
+    group_path[len] = 0;
+
+    if (sscanf(basename(group_path), "%d", &groupid) != 1) {
+        error_setg(errp, "vfio: failed to read %s", group_path);
+        return NULL;
+    }
+
+    return vfio_get_group(groupid, &address_space_memory, errp);
+}
+
+static void vfio_ap_realize(DeviceState *dev, Error **errp)
+{
+    VFIODevice *vbasedev;
+    VFIOGroup *vfio_group;
+    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
+    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
+    char *mdevid;
+    Error *local_err = NULL;
+    int ret;
+
+    vfio_group = vfio_ap_get_group(vapdev, &local_err);
+    if (!vfio_group) {
+        goto out_err;
+    }
+
+    vapdev->vdev.ops = &vfio_ap_ops;
+    vapdev->vdev.type = VFIO_DEVICE_TYPE_AP;
+    mdevid = basename(vapdev->vdev.sysfsdev);
+    vapdev->vdev.name = g_strdup_printf("%s", mdevid);
+    vapdev->vdev.dev = dev;
+    QLIST_FOREACH(vbasedev, &vfio_group->device_list, next) {
+        if (strcmp(vbasedev->name, vapdev->vdev.name) == 0) {
+            error_setg(&local_err,
+                       "%s: AP device %s has already been realized",
+                       VFIO_AP_DEVICE_TYPE, vapdev->vdev.name);
+            goto out_device_err;
+        }
+    }
+
+    ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, &local_err);
+    if (ret) {
+        goto out_device_err;
+    }
+
+    QTAILQ_INSERT_TAIL(&vfio_ap_devices, vapdev, sibling);
+
+    return;
+
+out_device_err:
+    vfio_put_group(vfio_group);
+out_err:
+    error_propagate(errp, local_err);
+}
+
+static void vfio_ap_unrealize(DeviceState *dev, Error **errp)
+{
+    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
+    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
+    VFIOGroup *group = vapdev->vdev.group;
+
+    vfio_put_device(vapdev);
+    vfio_put_group(group);
+}
+
+static Property vfio_ap_properties[] = {
+    DEFINE_PROP_STRING(AP_SYSFSDEV_PROP_NAME, VFIOAPDevice, vdev.sysfsdev),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static const VMStateDescription vfio_ap_vmstate = {
+    .name = VFIO_AP_DEVICE_TYPE,
+    .unmigratable = 1,
+};
+
+static void vfio_ap_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->props = vfio_ap_properties;
+    dc->vmsd = &vfio_ap_vmstate;
+    dc->desc = "VFIO-based AP device assignment";
+    dc->realize = vfio_ap_realize;
+    dc->unrealize = vfio_ap_unrealize;
+}
+
+static const TypeInfo vfio_ap_info = {
+    .name = VFIO_AP_DEVICE_TYPE,
+    .parent = AP_DEVICE_TYPE,
+    .instance_size = sizeof(VFIOAPDevice),
+    .class_init = vfio_ap_class_init,
+};
+
+static void register_vfio_ap_type(void)
+{
+    type_register_static(&vfio_ap_info);
+}
+
+type_init(register_vfio_ap_type)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index f3a2ac9..f1f22d9 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -46,6 +46,7 @@ enum {
     VFIO_DEVICE_TYPE_PCI = 0,
     VFIO_DEVICE_TYPE_PLATFORM = 1,
     VFIO_DEVICE_TYPE_CCW = 2,
+    VFIO_DEVICE_TYPE_AP = 3,
 };
 
 typedef struct VFIOMmap {
-- 
1.7.1

[Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Tony Krowiak]

A new CPU model feature and two new CPU model facilities are
introduced to support AP devices for a KVM guest.

CPU model features:

1. The KVM_S390_VM_CPU_FEAT_AP CPU model feature indicates that
   AP facilities are installed. This feature will be enabled by
   the kernel only if the AP facilities are installed on the linux
   host. This feature must be turned on from userspace to access
   AP devices from the KVM guest. The QEMU command line to turn
   this feature looks something like this:

	qemu-system-s390x ... -cpu xxx,ap=on

CPU model facilities:

1. The S390_FEAT_AP_QUERY_CONFIG_INFO feature indicates the AP Query
   Configuration Information (QCI) facility is installed. This feature
   will be enabled by the kernel only if the QCI is installed on
   the host. This facility will be set by QEMU only if the
   KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on.
   (see CPU model features above)

2. The S390_FEAT_AP_FACILITY_TEST feature indicates that the AP
   Test Facility (APFT) facility is installed. This feature will
   be enabled by the kernel only if the APFT facility is installed
   on the host. This facility will be set by QEMU for the KVM guest
   only if the KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on.
   (see CPU model features above)

Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
---
 hw/vfio/ap.c                    |    9 +++++++++
 linux-headers/asm-s390/kvm.h    |    1 +
 target/s390x/cpu_features.c     |    3 +++
 target/s390x/cpu_features_def.h |    3 +++
 target/s390x/cpu_models.c       |   12 ++++++++++++
 target/s390x/gen-features.c     |    3 +++
 target/s390x/kvm.c              |    6 ++++++
 7 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 8aa5221..b93f7d9 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -19,6 +19,7 @@
 #include "hw/s390x/ap-device.h"
 #include "qemu/error-report.h"
 #include "qemu/queue.h"
+#include "cpu.h"
 
 #define VFIO_AP_DEVICE_TYPE      "vfio-ap"
 #define AP_SYSFSDEV_PROP_NAME    "sysfsdev"
@@ -87,6 +88,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
     Error *local_err = NULL;
     int ret;
 
+    if (!s390_has_feat(S390_FEAT_AP)) {
+        error_setg(&local_err, "Invalid device configuration: ");
+        error_append_hint(&local_err,
+                          "Verify AP facilities are enabled for the guest"
+                          "(ap=on)\n");
+        goto out_err;
+    }
+
     vfio_group = vfio_ap_get_group(vapdev, &local_err);
     if (!vfio_group) {
         goto out_err;
diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
index 11def14..35a6d04 100644
--- a/linux-headers/asm-s390/kvm.h
+++ b/linux-headers/asm-s390/kvm.h
@@ -130,6 +130,7 @@ struct kvm_s390_vm_cpu_machine {
 #define KVM_S390_VM_CPU_FEAT_PFMFI	11
 #define KVM_S390_VM_CPU_FEAT_SIGPIF	12
 #define KVM_S390_VM_CPU_FEAT_KSS	13
+#define KVM_S390_VM_CPU_FEAT_AP		14
 struct kvm_s390_vm_cpu_feat {
 	__u64 feat[16];
 };
diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
index a5619f2..65b91bd 100644
--- a/target/s390x/cpu_features.c
+++ b/target/s390x/cpu_features.c
@@ -36,8 +36,10 @@ static const S390FeatDef s390_features[] = {
     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("qci", S390_FEAT_TYPE_STFL, 12, "Query AP Configuration 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("apft", S390_FEAT_TYPE_STFL, 15, "Adjunct Processor Facilities Test 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"),
@@ -125,6 +127,7 @@ static const S390FeatDef s390_features[] = {
 
     FEAT_INIT("dateh2", S390_FEAT_TYPE_MISC, 0, "DAT-enhancement facility 2"),
     FEAT_INIT("cmm", S390_FEAT_TYPE_MISC, 0, "Collaborative-memory-management facility"),
+    FEAT_INIT("ap", S390_FEAT_TYPE_MISC, 1, "AP facilities installed"),
 
     FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32 bit in general registers)"),
     FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64 bit in parameter list)"),
diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
index 7c5915c..8998b65 100644
--- a/target/s390x/cpu_features_def.h
+++ b/target/s390x/cpu_features_def.h
@@ -27,8 +27,10 @@ typedef enum {
     S390_FEAT_SENSE_RUNNING_STATUS,
     S390_FEAT_CONDITIONAL_SSKE,
     S390_FEAT_CONFIGURATION_TOPOLOGY,
+    S390_FEAT_AP_QUERY_CONFIG_INFO,
     S390_FEAT_IPTE_RANGE,
     S390_FEAT_NONQ_KEY_SETTING,
+    S390_FEAT_AP_FACILITIES_TEST,
     S390_FEAT_EXTENDED_TRANSLATION_2,
     S390_FEAT_MSA,
     S390_FEAT_LONG_DISPLACEMENT,
@@ -118,6 +120,7 @@ typedef enum {
     /* Misc */
     S390_FEAT_DAT_ENH_2,
     S390_FEAT_CMM,
+    S390_FEAT_AP,
 
     /* PLO */
     S390_FEAT_PLO_CL,
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 1d5f0da..35f91ea 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -770,6 +770,8 @@ static void check_consistency(const S390CPUModel *model)
         { S390_FEAT_PRNO_TRNG_QRTCR, S390_FEAT_MSA_EXT_5 },
         { S390_FEAT_PRNO_TRNG, S390_FEAT_MSA_EXT_5 },
         { S390_FEAT_SIE_KSS, S390_FEAT_SIE_F2 },
+        { S390_FEAT_AP_QUERY_CONFIG_INFO, S390_FEAT_AP },
+        { S390_FEAT_AP_FACILITIES_TEST, S390_FEAT_AP },
     };
     int i;
 
@@ -900,6 +902,16 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
     cpu->model->cpu_id_format = max_model->cpu_id_format;
     cpu->model->cpu_ver = max_model->cpu_ver;
 
+    /*
+     * If the AP facilities are not installed on the guest, then it makes
+     * no sense to enable the QCI or APFT facilities because they are only
+     * needed by AP facilities.
+     */
+    if (!test_bit(S390_FEAT_AP, cpu->model->features)) {
+        clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, cpu->model->features);
+        clear_bit(S390_FEAT_AP_FACILITIES_TEST, cpu->model->features);
+    }
+
     check_consistency(cpu->model);
     check_compatibility(max_model, cpu->model, errp);
     if (*errp) {
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 0cdbc15..2d01b52 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -447,6 +447,9 @@ static uint16_t full_GEN12_GA1[] = {
     S390_FEAT_ADAPTER_INT_SUPPRESSION,
     S390_FEAT_EDAT_2,
     S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
+    S390_FEAT_AP,
+    S390_FEAT_AP_QUERY_CONFIG_INFO,
+    S390_FEAT_AP_FACILITIES_TEST,
 };
 
 static uint16_t full_GEN12_GA2[] = {
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index e13c890..ae20ed8 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2105,6 +2105,7 @@ static int kvm_to_feat[][2] = {
     { KVM_S390_VM_CPU_FEAT_PFMFI, S390_FEAT_SIE_PFMFI},
     { KVM_S390_VM_CPU_FEAT_SIGPIF, S390_FEAT_SIE_SIGPIF},
     { KVM_S390_VM_CPU_FEAT_KSS, S390_FEAT_SIE_KSS},
+    { KVM_S390_VM_CPU_FEAT_AP, S390_FEAT_AP},
 };
 
 static int query_cpu_feat(S390FeatBitmap features)
@@ -2214,6 +2215,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
         error_setg(errp, "KVM: Error querying CPU features: %d", rc);
         return;
     }
+    /* AP facilities support is required to enable QCI and APFT support */
+    if (!test_bit(S390_FEAT_AP, model->features)) {
+        clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, model->features);
+        clear_bit(S390_FEAT_AP_FACILITIES_TEST, model->features);
+    }
     /* get supported cpu subfunctions indicated via query / test bit */
     rc = query_cpu_subfunc(model->features);
     if (rc) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2 0/5] s390x: vfio-ap: guest dedicated crypto adapters

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1519746259-27710-1-git-send-email-akrowiak@linux.vnet.ibm.com
Subject: [Qemu-devel] [PATCH v2 0/5] s390x: vfio-ap: guest dedicated crypto adapters

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/1519746259-27710-1-git-send-email-akrowiak@linux.vnet.ibm.com -> patchew/1519746259-27710-1-git-send-email-akrowiak@linux.vnet.ibm.com
Switched to a new branch 'test'
e9e1d68b87 s390x/cpumodel: Set up CPU model for AP device support
2fabd0f576 s390x/vfio: ap: Introduce VFIO AP device
bb505ee5d6 s390x/vfio: ap: VFIO: linux header updates
4ea89ebf38 s390x/ap: base Adjunct Processor (AP) object
4fc31e63ea s390: doc: detailed specifications for AP virtualization

=== OUTPUT BEGIN ===
Checking PATCH 1/5: s390: doc: detailed specifications for AP virtualization...
Checking PATCH 2/5: s390x/ap: base Adjunct Processor (AP) object...
Checking PATCH 3/5: s390x/vfio: ap: VFIO: linux header updates...
Checking PATCH 4/5: s390x/vfio: ap: Introduce VFIO AP device...
Checking PATCH 5/5: s390x/cpumodel: Set up CPU model for AP device support...
WARNING: line over 80 characters
#86: FILE: target/s390x/cpu_features.c:39:
+    FEAT_INIT("qci", S390_FEAT_TYPE_STFL, 12, "Query AP Configuration facility"),

ERROR: line over 90 characters
#89: FILE: target/s390x/cpu_features.c:42:
+    FEAT_INIT("apft", S390_FEAT_TYPE_STFL, 15, "Adjunct Processor Facilities Test facility"),

total: 1 errors, 1 warnings, 113 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Cornelia Huck]

On Tue, 27 Feb 2018 10:44:19 -0500
Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote:
> A new CPU model feature and two new CPU model facilities are
> introduced to support AP devices for a KVM guest.
> 
> CPU model features:
> 
> 1. The KVM_S390_VM_CPU_FEAT_AP CPU model feature indicates that
>    AP facilities are installed. This feature will be enabled by
>    the kernel only if the AP facilities are installed on the linux
>    host. This feature must be turned on from userspace to access
>    AP devices from the KVM guest. The QEMU command line to turn
>    this feature looks something like this:
> 
> 	qemu-system-s390x ... -cpu xxx,ap=on
> 
> CPU model facilities:
> 
> 1. The S390_FEAT_AP_QUERY_CONFIG_INFO feature indicates the AP Query
>    Configuration Information (QCI) facility is installed. This feature
>    will be enabled by the kernel only if the QCI is installed on
>    the host. This facility will be set by QEMU only if the
>    KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on.
>    (see CPU model features above)
> 
> 2. The S390_FEAT_AP_FACILITY_TEST feature indicates that the AP
>    Test Facility (APFT) facility is installed. This feature will
>    be enabled by the kernel only if the APFT facility is installed
>    on the host. This facility will be set by QEMU for the KVM guest
>    only if the KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on.
>    (see CPU model features above)
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
> ---
>  hw/vfio/ap.c                    |    9 +++++++++
>  linux-headers/asm-s390/kvm.h    |    1 +
>  target/s390x/cpu_features.c     |    3 +++
>  target/s390x/cpu_features_def.h |    3 +++
>  target/s390x/cpu_models.c       |   12 ++++++++++++
>  target/s390x/gen-features.c     |    3 +++
>  target/s390x/kvm.c              |    6 ++++++
>  7 files changed, 37 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 8aa5221..b93f7d9 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -19,6 +19,7 @@
>  #include "hw/s390x/ap-device.h"
>  #include "qemu/error-report.h"
>  #include "qemu/queue.h"
> +#include "cpu.h"
>  
>  #define VFIO_AP_DEVICE_TYPE      "vfio-ap"
>  #define AP_SYSFSDEV_PROP_NAME    "sysfsdev"
> @@ -87,6 +88,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>      Error *local_err = NULL;
>      int ret;
>  
> +    if (!s390_has_feat(S390_FEAT_AP)) {
> +        error_setg(&local_err, "Invalid device configuration: ");

"AP support not available" ?

[The hint is not visible in every circumstance IIRC]

> +        error_append_hint(&local_err,
> +                          "Verify AP facilities are enabled for the guest"
> +                          "(ap=on)\n");
> +        goto out_err;
> +    }
> +
>      vfio_group = vfio_ap_get_group(vapdev, &local_err);
>      if (!vfio_group) {
>          goto out_err;
> diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
> index 11def14..35a6d04 100644
> --- a/linux-headers/asm-s390/kvm.h
> +++ b/linux-headers/asm-s390/kvm.h
> @@ -130,6 +130,7 @@ struct kvm_s390_vm_cpu_machine {
>  #define KVM_S390_VM_CPU_FEAT_PFMFI	11
>  #define KVM_S390_VM_CPU_FEAT_SIGPIF	12
>  #define KVM_S390_VM_CPU_FEAT_KSS	13
> +#define KVM_S390_VM_CPU_FEAT_AP		14
>  struct kvm_s390_vm_cpu_feat {
>  	__u64 feat[16];
>  };

Shouldn't that hunk have been in the previous headers update already?

> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index a5619f2..65b91bd 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -36,8 +36,10 @@ static const S390FeatDef s390_features[] = {
>      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("qci", S390_FEAT_TYPE_STFL, 12, "Query AP Configuration 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("apft", S390_FEAT_TYPE_STFL, 15, "Adjunct Processor Facilities Test 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"),
> @@ -125,6 +127,7 @@ static const S390FeatDef s390_features[] = {
>  
>      FEAT_INIT("dateh2", S390_FEAT_TYPE_MISC, 0, "DAT-enhancement facility 2"),
>      FEAT_INIT("cmm", S390_FEAT_TYPE_MISC, 0, "Collaborative-memory-management facility"),
> +    FEAT_INIT("ap", S390_FEAT_TYPE_MISC, 1, "AP facilities installed"),

FEAT_TYPE_MISC does not use bit numbering (and there's a new
initializer for these bits queued in s390-next).

>  
>      FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32 bit in general registers)"),
>      FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64 bit in parameter list)"),
> diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
> index 7c5915c..8998b65 100644
> --- a/target/s390x/cpu_features_def.h
> +++ b/target/s390x/cpu_features_def.h
> @@ -27,8 +27,10 @@ typedef enum {
>      S390_FEAT_SENSE_RUNNING_STATUS,
>      S390_FEAT_CONDITIONAL_SSKE,
>      S390_FEAT_CONFIGURATION_TOPOLOGY,
> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>      S390_FEAT_IPTE_RANGE,
>      S390_FEAT_NONQ_KEY_SETTING,
> +    S390_FEAT_AP_FACILITIES_TEST,
>      S390_FEAT_EXTENDED_TRANSLATION_2,
>      S390_FEAT_MSA,
>      S390_FEAT_LONG_DISPLACEMENT,
> @@ -118,6 +120,7 @@ typedef enum {
>      /* Misc */
>      S390_FEAT_DAT_ENH_2,
>      S390_FEAT_CMM,
> +    S390_FEAT_AP,
>  
>      /* PLO */
>      S390_FEAT_PLO_CL,
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 1d5f0da..35f91ea 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -770,6 +770,8 @@ static void check_consistency(const S390CPUModel *model)
>          { S390_FEAT_PRNO_TRNG_QRTCR, S390_FEAT_MSA_EXT_5 },
>          { S390_FEAT_PRNO_TRNG, S390_FEAT_MSA_EXT_5 },
>          { S390_FEAT_SIE_KSS, S390_FEAT_SIE_F2 },
> +        { S390_FEAT_AP_QUERY_CONFIG_INFO, S390_FEAT_AP },
> +        { S390_FEAT_AP_FACILITIES_TEST, S390_FEAT_AP },
>      };
>      int i;
>  
> @@ -900,6 +902,16 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
>      cpu->model->cpu_id_format = max_model->cpu_id_format;
>      cpu->model->cpu_ver = max_model->cpu_ver;
>  
> +    /*
> +     * If the AP facilities are not installed on the guest, then it makes


"not provided in the model" ?

> +     * no sense to enable the QCI or APFT facilities because they are only
> +     * needed by AP facilities.
> +     */
> +    if (!test_bit(S390_FEAT_AP, cpu->model->features)) {
> +        clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, cpu->model->features);
> +        clear_bit(S390_FEAT_AP_FACILITIES_TEST, cpu->model->features);
> +    }
> +
>      check_consistency(cpu->model);
>      check_compatibility(max_model, cpu->model, errp);
>      if (*errp) {
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index 0cdbc15..2d01b52 100644
> --- a/target/s390x/gen-features.c
> +++ b/target/s390x/gen-features.c
> @@ -447,6 +447,9 @@ static uint16_t full_GEN12_GA1[] = {
>      S390_FEAT_ADAPTER_INT_SUPPRESSION,
>      S390_FEAT_EDAT_2,
>      S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
> +    S390_FEAT_AP,
> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
> +    S390_FEAT_AP_FACILITIES_TEST,
>  };
>  
>  static uint16_t full_GEN12_GA2[] = {
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index e13c890..ae20ed8 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2105,6 +2105,7 @@ static int kvm_to_feat[][2] = {
>      { KVM_S390_VM_CPU_FEAT_PFMFI, S390_FEAT_SIE_PFMFI},
>      { KVM_S390_VM_CPU_FEAT_SIGPIF, S390_FEAT_SIE_SIGPIF},
>      { KVM_S390_VM_CPU_FEAT_KSS, S390_FEAT_SIE_KSS},
> +    { KVM_S390_VM_CPU_FEAT_AP, S390_FEAT_AP},
>  };
>  
>  static int query_cpu_feat(S390FeatBitmap features)
> @@ -2214,6 +2215,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>          error_setg(errp, "KVM: Error querying CPU features: %d", rc);
>          return;
>      }
> +    /* AP facilities support is required to enable QCI and APFT support */
> +    if (!test_bit(S390_FEAT_AP, model->features)) {
> +        clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, model->features);
> +        clear_bit(S390_FEAT_AP_FACILITIES_TEST, model->features);
> +    }

Hm, do you need this twice?

>      /* get supported cpu subfunctions indicated via query / test bit */
>      rc = query_cpu_subfunc(model->features);
>      if (rc) {

I'm leaving a general review of the cpu model things to David.

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Halil Pasic]

On 02/27/2018 05:27 PM, Cornelia Huck wrote:
> On Tue, 27 Feb 2018 10:44:19 -0500
> Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote:
>> A new CPU model feature and two new CPU model facilities are
>> introduced to support AP devices for a KVM guest.
>>
>> CPU model features:
>>
>> 1. The KVM_S390_VM_CPU_FEAT_AP CPU model feature indicates that
>>    AP facilities are installed. This feature will be enabled by
>>    the kernel only if the AP facilities are installed on the linux
>>    host. This feature must be turned on from userspace to access
>>    AP devices from the KVM guest. The QEMU command line to turn
>>    this feature looks something like this:
>>
>> 	qemu-system-s390x ... -cpu xxx,ap=on
>>
>> CPU model facilities:
>>
>> 1. The S390_FEAT_AP_QUERY_CONFIG_INFO feature indicates the AP Query
>>    Configuration Information (QCI) facility is installed. This feature
>>    will be enabled by the kernel only if the QCI is installed on
>>    the host. This facility will be set by QEMU only if the
>>    KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on.
>>    (see CPU model features above)
>>
>> 2. The S390_FEAT_AP_FACILITY_TEST feature indicates that the AP
>>    Test Facility (APFT) facility is installed. This feature will
>>    be enabled by the kernel only if the APFT facility is installed
>>    on the host. This facility will be set by QEMU for the KVM guest
>>    only if the KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on.
>>    (see CPU model features above)
>>

This may needs to be reworded. See my comments below.

>> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
>> ---
>>  hw/vfio/ap.c                    |    9 +++++++++
>>  linux-headers/asm-s390/kvm.h    |    1 +
>>  target/s390x/cpu_features.c     |    3 +++
>>  target/s390x/cpu_features_def.h |    3 +++
>>  target/s390x/cpu_models.c       |   12 ++++++++++++
>>  target/s390x/gen-features.c     |    3 +++
>>  target/s390x/kvm.c              |    6 ++++++
>>  7 files changed, 37 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>> index 8aa5221..b93f7d9 100644
>> --- a/hw/vfio/ap.c
>> +++ b/hw/vfio/ap.c
>> @@ -19,6 +19,7 @@
>>  #include "hw/s390x/ap-device.h"
>>  #include "qemu/error-report.h"
>>  #include "qemu/queue.h"
>> +#include "cpu.h"
>>  
>>  #define VFIO_AP_DEVICE_TYPE      "vfio-ap"
>>  #define AP_SYSFSDEV_PROP_NAME    "sysfsdev"
>> @@ -87,6 +88,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>>      Error *local_err = NULL;
>>      int ret;
>>  
>> +    if (!s390_has_feat(S390_FEAT_AP)) {
>> +        error_setg(&local_err, "Invalid device configuration: ");
> 
> "AP support not available" ?
> 
> [The hint is not visible in every circumstance IIRC]
> 

I agree, it does not make sense to split this into a message and 
a hint.

[..]

>> @@ -900,6 +902,16 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
>>      cpu->model->cpu_id_format = max_model->cpu_id_format;
>>      cpu->model->cpu_ver = max_model->cpu_ver;
>>  
>> +    /*
>> +     * If the AP facilities are not installed on the guest, then it makes
> 
> 
> "not provided in the model" ?
> 
>> +     * no sense to enable the QCI or APFT facilities because they are only
>> +     * needed by AP facilities.
>> +     */
>> +    if (!test_bit(S390_FEAT_AP, cpu->model->features)) {
>> +        clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, cpu->model->features);
>> +        clear_bit(S390_FEAT_AP_FACILITIES_TEST, cpu->model->features);
>> +    }
>> +

I don't like this at all. Never liked software that overrules my user input.
If I say --cpu z13,ap=off,qci=on,apft=on this would silently overrule to 
--cpu z13,ap=off,qci=off,apft=off from the guest perspective. There also might be
other reasons why this is a bad idea.

>>      check_consistency(cpu->model);
>>      check_compatibility(max_model, cpu->model, errp);
>>      if (*errp) {
>> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
>> index 0cdbc15..2d01b52 100644
>> --- a/target/s390x/gen-features.c
>> +++ b/target/s390x/gen-features.c
>> @@ -447,6 +447,9 @@ static uint16_t full_GEN12_GA1[] = {
>>      S390_FEAT_ADAPTER_INT_SUPPRESSION,
>>      S390_FEAT_EDAT_2,
>>      S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
>> +    S390_FEAT_AP,
>> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>> +    S390_FEAT_AP_FACILITIES_TEST,
>>  };
>>  
>>  static uint16_t full_GEN12_GA2[] = {
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index e13c890..ae20ed8 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -2105,6 +2105,7 @@ static int kvm_to_feat[][2] = {
>>      { KVM_S390_VM_CPU_FEAT_PFMFI, S390_FEAT_SIE_PFMFI},
>>      { KVM_S390_VM_CPU_FEAT_SIGPIF, S390_FEAT_SIE_SIGPIF},
>>      { KVM_S390_VM_CPU_FEAT_KSS, S390_FEAT_SIE_KSS},
>> +    { KVM_S390_VM_CPU_FEAT_AP, S390_FEAT_AP},
>>  };
>>  
>>  static int query_cpu_feat(S390FeatBitmap features)
>> @@ -2214,6 +2215,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>>          error_setg(errp, "KVM: Error querying CPU features: %d", rc);
>>          return;
>>      }
>> +    /* AP facilities support is required to enable QCI and APFT support */
>> +    if (!test_bit(S390_FEAT_AP, model->features)) {
>> +        clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, model->features);
>> +        clear_bit(S390_FEAT_AP_FACILITIES_TEST, model->features);
>> +    }
> 
> Hm, do you need this twice?

In my opinion this has only value if we assume that HW and/or KVM is buggy and
we are running host model (or it's expansion).

And even the we would get a warning, and nothing bad would happen with a linux
guest.

While I'm not strongly opposing this, I would not mind it dropped. If we want
to make sure things are consistent I would prefer the consistency check being
generating an error (instead of a warning).

> 
>>      /* get supported cpu subfunctions indicated via query / test bit */
>>      rc = query_cpu_subfunc(model->features);
>>      if (rc) {
> 
> I'm leaving a general review of the cpu model things to David.
> 

Except for these it's LGTM (r-b level LGTM).

Re: [Qemu-devel] [PATCH v2 4/5] s390x/vfio: ap: Introduce VFIO AP device

[Cornelia Huck]

On Tue, 27 Feb 2018 10:44:18 -0500
Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote:

> Introduces a VFIO based AP device. The device is defined via
> the QEMU command line by specifying:
> 
>     -device vfio-ap,sysfsdev=<path-to-mediated-matrix-device>
> 
> The mediated matrix device is created by the VFIO AP device
> driver by writing a UUID to a sysfs attribute file (see
> docs/vfio-ap.txt). The mediated matrix device will be named
> after the UUID. Symbolic links to the $uuid are created in
> many places, so the path to the mediated matrix device $uuid
> can be specified in any of the following ways:
> 
> /sys/devices/vfio_ap/matrix/$uuid
> /sys/devices/vfio_ap/matrix/mdev_supported_types/vfio_ap-passthrough/devices/$uuid
> /sys/bus/mdev/devices/$uuid
> /sys/bus/mdev/drivers/vfio_mdev/$uuid
> 
> When the vfio-ap device is realized, it acquires and opens the
> VFIO iommu group to which the mediated matrix device is
> bound. This causes a VFIO group notification event to be
> signaled. The vfio_ap device driver's group notification
> handler will get called at which time the device driver
> will configure the the AP devices to which the guest will
> be granted access.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
> ---
>  default-configs/s390x-softmmu.mak |    1 +
>  hw/vfio/Makefile.objs             |    1 +
>  hw/vfio/ap.c                      |  167 +++++++++++++++++++++++++++++++++++++
>  include/hw/vfio/vfio-common.h     |    1 +
>  4 files changed, 170 insertions(+), 0 deletions(-)
>  create mode 100644 hw/vfio/ap.c
> 

> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> new file mode 100644
> index 0000000..8aa5221
> --- /dev/null
> +++ b/hw/vfio/ap.c
> @@ -0,0 +1,167 @@
> +/*
> + * VFIO based AP matrix device assignment
> + *
> + * Copyright 2017 IBM Corp.

Happy new year?

[Also the other new files, here and in the Linux part.]

> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or(at
> + * your option) any version. See the COPYING file in the top-level

That probably should be "any later version" (I'm not even sure what GPL
v1 says :)

And I just noticed that the vfio-ccw code has the same problem...

> + * directory.
> + */
> +
> +#include <linux/vfio.h>
> +#include <sys/ioctl.h>
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "hw/sysbus.h"
> +#include "hw/vfio/vfio.h"
> +#include "hw/vfio/vfio-common.h"
> +#include "hw/s390x/ap-device.h"
> +#include "qemu/error-report.h"
> +#include "qemu/queue.h"
> +
> +#define VFIO_AP_DEVICE_TYPE      "vfio-ap"
> +#define AP_SYSFSDEV_PROP_NAME    "sysfsdev"

Using a #define for a property name seems unusual (and I think it
decreases readability).

Otherwise, looks fine (on first read-through).

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[David Hildenbrand]

>      vfio_group = vfio_ap_get_group(vapdev, &local_err);
>      if (!vfio_group) {
>          goto out_err;
> diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
> index 11def14..35a6d04 100644
> --- a/linux-headers/asm-s390/kvm.h
> +++ b/linux-headers/asm-s390/kvm.h
> @@ -130,6 +130,7 @@ struct kvm_s390_vm_cpu_machine {
>  #define KVM_S390_VM_CPU_FEAT_PFMFI	11
>  #define KVM_S390_VM_CPU_FEAT_SIGPIF	12
>  #define KVM_S390_VM_CPU_FEAT_KSS	13
> +#define KVM_S390_VM_CPU_FEAT_AP		14
>  struct kvm_s390_vm_cpu_feat {
>  	__u64 feat[16];
>  };
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index a5619f2..65b91bd 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -36,8 +36,10 @@ static const S390FeatDef s390_features[] = {
>      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("qci", S390_FEAT_TYPE_STFL, 12, "Query AP Configuration 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("apft", S390_FEAT_TYPE_STFL, 15, "Adjunct Processor Facilities Test 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"),
> @@ -125,6 +127,7 @@ static const S390FeatDef s390_features[] = {
>  
>      FEAT_INIT("dateh2", S390_FEAT_TYPE_MISC, 0, "DAT-enhancement facility 2"),
>      FEAT_INIT("cmm", S390_FEAT_TYPE_MISC, 0, "Collaborative-memory-management facility"),
> +    FEAT_INIT("ap", S390_FEAT_TYPE_MISC, 1, "AP facilities installed"),

How exactly is this feature communicated to the guest? How does KVM
sense support for it?

IOW: is this really a CPU model feature?

>  
>      FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32 bit in general registers)"),
>      FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64 bit in parameter list)"),
> diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
> index 7c5915c..8998b65 100644
> --- a/target/s390x/cpu_features_def.h
> +++ b/target/s390x/cpu_features_def.h
> @@ -27,8 +27,10 @@ typedef enum {
>      S390_FEAT_SENSE_RUNNING_STATUS,
>      S390_FEAT_CONDITIONAL_SSKE,
>      S390_FEAT_CONFIGURATION_TOPOLOGY,
> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>      S390_FEAT_IPTE_RANGE,
>      S390_FEAT_NONQ_KEY_SETTING,
> +    S390_FEAT_AP_FACILITIES_TEST,
>      S390_FEAT_EXTENDED_TRANSLATION_2,
>      S390_FEAT_MSA,
>      S390_FEAT_LONG_DISPLACEMENT,
> @@ -118,6 +120,7 @@ typedef enum {
>      /* Misc */
>      S390_FEAT_DAT_ENH_2,
>      S390_FEAT_CMM,
> +    S390_FEAT_AP,
>  
>      /* PLO */
>      S390_FEAT_PLO_CL,
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 1d5f0da..35f91ea 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -770,6 +770,8 @@ static void check_consistency(const S390CPUModel *model)
>          { S390_FEAT_PRNO_TRNG_QRTCR, S390_FEAT_MSA_EXT_5 },
>          { S390_FEAT_PRNO_TRNG, S390_FEAT_MSA_EXT_5 },
>          { S390_FEAT_SIE_KSS, S390_FEAT_SIE_F2 },
> +        { S390_FEAT_AP_QUERY_CONFIG_INFO, S390_FEAT_AP },
> +        { S390_FEAT_AP_FACILITIES_TEST, S390_FEAT_AP },
>      };
>      int i;
>  
> @@ -900,6 +902,16 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
>      cpu->model->cpu_id_format = max_model->cpu_id_format;
>      cpu->model->cpu_ver = max_model->cpu_ver;
>  
> +    /*
> +     * If the AP facilities are not installed on the guest, then it makes
> +     * no sense to enable the QCI or APFT facilities because they are only
> +     * needed by AP facilities.
> +     */
> +    if (!test_bit(S390_FEAT_AP, cpu->model->features)) {
> +        clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, cpu->model->features);
> +        clear_bit(S390_FEAT_AP_FACILITIES_TEST, cpu->model->features);
> +    }

Please don't silently disable things. Instead

a) Add consistency checks (check_consistency())
b) Mask the bits out in the KVM CPU model sensing part
  (kvm_s390_get_host_cpu_model()) - which you already have :)

> +
>      check_consistency(cpu->model);
>      check_compatibility(max_model, cpu->model, errp);
>      if (*errp) {
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index 0cdbc15..2d01b52 100644
> --- a/target/s390x/gen-features.c
> +++ b/target/s390x/gen-features.c
> @@ -447,6 +447,9 @@ static uint16_t full_GEN12_GA1[] = {
>      S390_FEAT_ADAPTER_INT_SUPPRESSION,
>      S390_FEAT_EDAT_2,
>      S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
> +    S390_FEAT_AP,
> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
> +    S390_FEAT_AP_FACILITIES_TEST,
>  };

Please keep the order as defined in target/s390x/cpu_features_def.h

>  
>  static uint16_t full_GEN12_GA2[] = {
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index e13c890..ae20ed8 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2105,6 +2105,7 @@ static int kvm_to_feat[][2] = {
>      { KVM_S390_VM_CPU_FEAT_PFMFI, S390_FEAT_SIE_PFMFI},
>      { KVM_S390_VM_CPU_FEAT_SIGPIF, S390_FEAT_SIE_SIGPIF},
>      { KVM_S390_VM_CPU_FEAT_KSS, S390_FEAT_SIE_KSS},
> +    { KVM_S390_VM_CPU_FEAT_AP, S390_FEAT_AP},

Nothing speaks against the STFL bits, want to learn more about the
S390_FEAT_AP feature :)


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[David Hildenbrand]

>> Hm, do you need this twice?
> 
> In my opinion this has only value if we assume that HW and/or KVM is buggy and
> we are running host model (or it's expansion).
> 

The "sanity" checks in KVM sensing code don't really hurt. But I agree,
sane KVM should not produce this.

> And even the we would get a warning, and nothing bad would happen with a linux
> guest.
> 
> While I'm not strongly opposing this, I would not mind it dropped. If we want
> to make sure things are consistent I would prefer the consistency check being
> generating an error (instead of a warning).
> 

We use a warning as it is helpful for development (e.g. under TCG you
can enable msa5, although we yield a warning due to a failing
consistency check).

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Halil Pasic]

On 02/27/2018 06:52 PM, David Hildenbrand wrote:
>>      vfio_group = vfio_ap_get_group(vapdev, &local_err);
>>      if (!vfio_group) {
>>          goto out_err;
>> diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
>> index 11def14..35a6d04 100644
>> --- a/linux-headers/asm-s390/kvm.h
>> +++ b/linux-headers/asm-s390/kvm.h
>> @@ -130,6 +130,7 @@ struct kvm_s390_vm_cpu_machine {
>>  #define KVM_S390_VM_CPU_FEAT_PFMFI	11
>>  #define KVM_S390_VM_CPU_FEAT_SIGPIF	12
>>  #define KVM_S390_VM_CPU_FEAT_KSS	13
>> +#define KVM_S390_VM_CPU_FEAT_AP		14
>>  struct kvm_s390_vm_cpu_feat {
>>  	__u64 feat[16];
>>  };
>> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
>> index a5619f2..65b91bd 100644
>> --- a/target/s390x/cpu_features.c
>> +++ b/target/s390x/cpu_features.c
>> @@ -36,8 +36,10 @@ static const S390FeatDef s390_features[] = {
>>      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("qci", S390_FEAT_TYPE_STFL, 12, "Query AP Configuration 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("apft", S390_FEAT_TYPE_STFL, 15, "Adjunct Processor Facilities Test 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"),
>> @@ -125,6 +127,7 @@ static const S390FeatDef s390_features[] = {
>>  
>>      FEAT_INIT("dateh2", S390_FEAT_TYPE_MISC, 0, "DAT-enhancement facility 2"),
>>      FEAT_INIT("cmm", S390_FEAT_TYPE_MISC, 0, "Collaborative-memory-management facility"),
>> +    FEAT_INIT("ap", S390_FEAT_TYPE_MISC, 1, "AP facilities installed"),
> 
> How exactly is this feature communicated to the guest? How does KVM
> sense support for it?
> 

The ap_bus has a function for determining if the ap instructions are
installed. I think it's basically trial execution. 

> IOW: is this really a CPU model feature?
> 

I think it's best modeled with a CPU model feature. In the end
it's about having or not having ap instructions in the guest, and
making stable guest ABI is exactly the thing of cpu-models AFAIU.

>>  
>>      FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32 bit in general registers)"),
>>      FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64 bit in parameter list)"),
>> diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
>> index 7c5915c..8998b65 100644
>> --- a/target/s390x/cpu_features_def.h
>> +++ b/target/s390x/cpu_features_def.h
>> @@ -27,8 +27,10 @@ typedef enum {
>>      S390_FEAT_SENSE_RUNNING_STATUS,
>>      S390_FEAT_CONDITIONAL_SSKE,
>>      S390_FEAT_CONFIGURATION_TOPOLOGY,
>> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>>      S390_FEAT_IPTE_RANGE,
>>      S390_FEAT_NONQ_KEY_SETTING,
>> +    S390_FEAT_AP_FACILITIES_TEST,
>>      S390_FEAT_EXTENDED_TRANSLATION_2,
>>      S390_FEAT_MSA,
>>      S390_FEAT_LONG_DISPLACEMENT,
>> @@ -118,6 +120,7 @@ typedef enum {
>>      /* Misc */
>>      S390_FEAT_DAT_ENH_2,
>>      S390_FEAT_CMM,
>> +    S390_FEAT_AP,
>>  
>>      /* PLO */
>>      S390_FEAT_PLO_CL,
>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>> index 1d5f0da..35f91ea 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -770,6 +770,8 @@ static void check_consistency(const S390CPUModel *model)
>>          { S390_FEAT_PRNO_TRNG_QRTCR, S390_FEAT_MSA_EXT_5 },
>>          { S390_FEAT_PRNO_TRNG, S390_FEAT_MSA_EXT_5 },
>>          { S390_FEAT_SIE_KSS, S390_FEAT_SIE_F2 },
>> +        { S390_FEAT_AP_QUERY_CONFIG_INFO, S390_FEAT_AP },
>> +        { S390_FEAT_AP_FACILITIES_TEST, S390_FEAT_AP },
>>      };
>>      int i;
>>  
>> @@ -900,6 +902,16 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
>>      cpu->model->cpu_id_format = max_model->cpu_id_format;
>>      cpu->model->cpu_ver = max_model->cpu_ver;
>>  
>> +    /*
>> +     * If the AP facilities are not installed on the guest, then it makes
>> +     * no sense to enable the QCI or APFT facilities because they are only
>> +     * needed by AP facilities.
>> +     */
>> +    if (!test_bit(S390_FEAT_AP, cpu->model->features)) {
>> +        clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, cpu->model->features);
>> +        clear_bit(S390_FEAT_AP_FACILITIES_TEST, cpu->model->features);
>> +    }
> 
> Please don't silently disable things. Instead
>

I agree, this has to go (already commented on this).
 
> a) Add consistency checks (check_consistency())

The consistency checks are already in place. As already stated
before, one could make it produce an error.

> b) Mask the bits out in the KVM CPU model sensing part
>   (kvm_s390_get_host_cpu_model()) - which you already have :)
> 

Getting no ap but qci and apft indicated by KVM is unlikely to
happen, ever.

>> +
>>      check_consistency(cpu->model);
>>      check_compatibility(max_model, cpu->model, errp);
>>      if (*errp) {
>> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
>> index 0cdbc15..2d01b52 100644
>> --- a/target/s390x/gen-features.c
>> +++ b/target/s390x/gen-features.c
>> @@ -447,6 +447,9 @@ static uint16_t full_GEN12_GA1[] = {
>>      S390_FEAT_ADAPTER_INT_SUPPRESSION,
>>      S390_FEAT_EDAT_2,
>>      S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
>> +    S390_FEAT_AP,
>> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>> +    S390_FEAT_AP_FACILITIES_TEST,
>>  };
> 
> Please keep the order as defined in target/s390x/cpu_features_def.h
> 
>>  
>>  static uint16_t full_GEN12_GA2[] = {
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index e13c890..ae20ed8 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -2105,6 +2105,7 @@ static int kvm_to_feat[][2] = {
>>      { KVM_S390_VM_CPU_FEAT_PFMFI, S390_FEAT_SIE_PFMFI},
>>      { KVM_S390_VM_CPU_FEAT_SIGPIF, S390_FEAT_SIE_SIGPIF},
>>      { KVM_S390_VM_CPU_FEAT_KSS, S390_FEAT_SIE_KSS},
>> +    { KVM_S390_VM_CPU_FEAT_AP, S390_FEAT_AP},
> 
> Nothing speaks against the STFL bits, want to learn more about the
> S390_FEAT_AP feature :)
> 
> 

Kernel series wise what you are looking for is 
'[PATCH v2 04/15] KVM: s390: CPU model support for AP virtualization'(MID: <1519741693-17440-5-git-send-email-akrowiak@linux.vnet.ibm.com>) 
and
'[PATCH v2 08/15] KVM: s390: interface to enable AP execution mode' (MID: <1519741693-17440-9-git-send-email-akrowiak@linux.vnet.ibm.com>)

Happy reviewing!

Regards,
Halil

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Tony Krowiak]

On 02/27/2018 11:27 AM, Cornelia Huck wrote:
> On Tue, 27 Feb 2018 10:44:19 -0500
> Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote:
>> A new CPU model feature and two new CPU model facilities are
>> introduced to support AP devices for a KVM guest.
>>
>> CPU model features:
>>
>> 1. The KVM_S390_VM_CPU_FEAT_AP CPU model feature indicates that
>>     AP facilities are installed. This feature will be enabled by
>>     the kernel only if the AP facilities are installed on the linux
>>     host. This feature must be turned on from userspace to access
>>     AP devices from the KVM guest. The QEMU command line to turn
>>     this feature looks something like this:
>>
>> 	qemu-system-s390x ... -cpu xxx,ap=on
>>
>> CPU model facilities:
>>
>> 1. The S390_FEAT_AP_QUERY_CONFIG_INFO feature indicates the AP Query
>>     Configuration Information (QCI) facility is installed. This feature
>>     will be enabled by the kernel only if the QCI is installed on
>>     the host. This facility will be set by QEMU only if the
>>     KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on.
>>     (see CPU model features above)
>>
>> 2. The S390_FEAT_AP_FACILITY_TEST feature indicates that the AP
>>     Test Facility (APFT) facility is installed. This feature will
>>     be enabled by the kernel only if the APFT facility is installed
>>     on the host. This facility will be set by QEMU for the KVM guest
>>     only if the KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on.
>>     (see CPU model features above)
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
>> ---
>>   hw/vfio/ap.c                    |    9 +++++++++
>>   linux-headers/asm-s390/kvm.h    |    1 +
>>   target/s390x/cpu_features.c     |    3 +++
>>   target/s390x/cpu_features_def.h |    3 +++
>>   target/s390x/cpu_models.c       |   12 ++++++++++++
>>   target/s390x/gen-features.c     |    3 +++
>>   target/s390x/kvm.c              |    6 ++++++
>>   7 files changed, 37 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>> index 8aa5221..b93f7d9 100644
>> --- a/hw/vfio/ap.c
>> +++ b/hw/vfio/ap.c
>> @@ -19,6 +19,7 @@
>>   #include "hw/s390x/ap-device.h"
>>   #include "qemu/error-report.h"
>>   #include "qemu/queue.h"
>> +#include "cpu.h"
>>   
>>   #define VFIO_AP_DEVICE_TYPE      "vfio-ap"
>>   #define AP_SYSFSDEV_PROP_NAME    "sysfsdev"
>> @@ -87,6 +88,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>>       Error *local_err = NULL;
>>       int ret;
>>   
>> +    if (!s390_has_feat(S390_FEAT_AP)) {
>> +        error_setg(&local_err, "Invalid device configuration: ");
> "AP support not available" ?
>
> [The hint is not visible in every circumstance IIRC]
>
>> +        error_append_hint(&local_err,
>> +                          "Verify AP facilities are enabled for the guest"
>> +                          "(ap=on)\n");
>> +        goto out_err;
>> +    }
>> +
>>       vfio_group = vfio_ap_get_group(vapdev, &local_err);
>>       if (!vfio_group) {
>>           goto out_err;
>> diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
>> index 11def14..35a6d04 100644
>> --- a/linux-headers/asm-s390/kvm.h
>> +++ b/linux-headers/asm-s390/kvm.h
>> @@ -130,6 +130,7 @@ struct kvm_s390_vm_cpu_machine {
>>   #define KVM_S390_VM_CPU_FEAT_PFMFI	11
>>   #define KVM_S390_VM_CPU_FEAT_SIGPIF	12
>>   #define KVM_S390_VM_CPU_FEAT_KSS	13
>> +#define KVM_S390_VM_CPU_FEAT_AP		14
>>   struct kvm_s390_vm_cpu_feat {
>>   	__u64 feat[16];
>>   };
> Shouldn't that hunk have been in the previous headers update already?
>
>> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
>> index a5619f2..65b91bd 100644
>> --- a/target/s390x/cpu_features.c
>> +++ b/target/s390x/cpu_features.c
>> @@ -36,8 +36,10 @@ static const S390FeatDef s390_features[] = {
>>       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("qci", S390_FEAT_TYPE_STFL, 12, "Query AP Configuration 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("apft", S390_FEAT_TYPE_STFL, 15, "Adjunct Processor Facilities Test 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"),
>> @@ -125,6 +127,7 @@ static const S390FeatDef s390_features[] = {
>>   
>>       FEAT_INIT("dateh2", S390_FEAT_TYPE_MISC, 0, "DAT-enhancement facility 2"),
>>       FEAT_INIT("cmm", S390_FEAT_TYPE_MISC, 0, "Collaborative-memory-management facility"),
>> +    FEAT_INIT("ap", S390_FEAT_TYPE_MISC, 1, "AP facilities installed"),
> FEAT_TYPE_MISC does not use bit numbering (and there's a new
> initializer for these bits queued in s390-next).
>
>>   
>>       FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32 bit in general registers)"),
>>       FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64 bit in parameter list)"),
>> diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
>> index 7c5915c..8998b65 100644
>> --- a/target/s390x/cpu_features_def.h
>> +++ b/target/s390x/cpu_features_def.h
>> @@ -27,8 +27,10 @@ typedef enum {
>>       S390_FEAT_SENSE_RUNNING_STATUS,
>>       S390_FEAT_CONDITIONAL_SSKE,
>>       S390_FEAT_CONFIGURATION_TOPOLOGY,
>> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>>       S390_FEAT_IPTE_RANGE,
>>       S390_FEAT_NONQ_KEY_SETTING,
>> +    S390_FEAT_AP_FACILITIES_TEST,
>>       S390_FEAT_EXTENDED_TRANSLATION_2,
>>       S390_FEAT_MSA,
>>       S390_FEAT_LONG_DISPLACEMENT,
>> @@ -118,6 +120,7 @@ typedef enum {
>>       /* Misc */
>>       S390_FEAT_DAT_ENH_2,
>>       S390_FEAT_CMM,
>> +    S390_FEAT_AP,
>>   
>>       /* PLO */
>>       S390_FEAT_PLO_CL,
>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>> index 1d5f0da..35f91ea 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -770,6 +770,8 @@ static void check_consistency(const S390CPUModel *model)
>>           { S390_FEAT_PRNO_TRNG_QRTCR, S390_FEAT_MSA_EXT_5 },
>>           { S390_FEAT_PRNO_TRNG, S390_FEAT_MSA_EXT_5 },
>>           { S390_FEAT_SIE_KSS, S390_FEAT_SIE_F2 },
>> +        { S390_FEAT_AP_QUERY_CONFIG_INFO, S390_FEAT_AP },
>> +        { S390_FEAT_AP_FACILITIES_TEST, S390_FEAT_AP },
>>       };
>>       int i;
>>   
>> @@ -900,6 +902,16 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
>>       cpu->model->cpu_id_format = max_model->cpu_id_format;
>>       cpu->model->cpu_ver = max_model->cpu_ver;
>>   
>> +    /*
>> +     * If the AP facilities are not installed on the guest, then it makes
>
> "not provided in the model" ?
>
>> +     * no sense to enable the QCI or APFT facilities because they are only
>> +     * needed by AP facilities.
>> +     */
>> +    if (!test_bit(S390_FEAT_AP, cpu->model->features)) {
>> +        clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, cpu->model->features);
>> +        clear_bit(S390_FEAT_AP_FACILITIES_TEST, cpu->model->features);
>> +    }
>> +
>>       check_consistency(cpu->model);
>>       check_compatibility(max_model, cpu->model, errp);
>>       if (*errp) {
>> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
>> index 0cdbc15..2d01b52 100644
>> --- a/target/s390x/gen-features.c
>> +++ b/target/s390x/gen-features.c
>> @@ -447,6 +447,9 @@ static uint16_t full_GEN12_GA1[] = {
>>       S390_FEAT_ADAPTER_INT_SUPPRESSION,
>>       S390_FEAT_EDAT_2,
>>       S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
>> +    S390_FEAT_AP,
>> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>> +    S390_FEAT_AP_FACILITIES_TEST,
>>   };
>>   
>>   static uint16_t full_GEN12_GA2[] = {
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index e13c890..ae20ed8 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -2105,6 +2105,7 @@ static int kvm_to_feat[][2] = {
>>       { KVM_S390_VM_CPU_FEAT_PFMFI, S390_FEAT_SIE_PFMFI},
>>       { KVM_S390_VM_CPU_FEAT_SIGPIF, S390_FEAT_SIE_SIGPIF},
>>       { KVM_S390_VM_CPU_FEAT_KSS, S390_FEAT_SIE_KSS},
>> +    { KVM_S390_VM_CPU_FEAT_AP, S390_FEAT_AP},
>>   };
>>   
>>   static int query_cpu_feat(S390FeatBitmap features)
>> @@ -2214,6 +2215,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>>           error_setg(errp, "KVM: Error querying CPU features: %d", rc);
>>           return;
>>       }
>> +    /* AP facilities support is required to enable QCI and APFT support */
>> +    if (!test_bit(S390_FEAT_AP, model->features)) {
>> +        clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, model->features);
>> +        clear_bit(S390_FEAT_AP_FACILITIES_TEST, model->features);
>> +    }
> Hm, do you need this twice?
No, I don't even need it once. It is a relic of a previous revision.
>
>>       /* get supported cpu subfunctions indicated via query / test bit */
>>       rc = query_cpu_subfunc(model->features);
>>       if (rc) {
> I'm leaving a general review of the cpu model things to David.
>

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Tony Krowiak]

On 02/27/2018 11:49 AM, Halil Pasic wrote:
>
> On 02/27/2018 05:27 PM, Cornelia Huck wrote:
>> On Tue, 27 Feb 2018 10:44:19 -0500
>> Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote:
>>> A new CPU model feature and two new CPU model facilities are
>>> introduced to support AP devices for a KVM guest.
>>>
>>> CPU model features:
>>>
>>> 1. The KVM_S390_VM_CPU_FEAT_AP CPU model feature indicates that
>>>     AP facilities are installed. This feature will be enabled by
>>>     the kernel only if the AP facilities are installed on the linux
>>>     host. This feature must be turned on from userspace to access
>>>     AP devices from the KVM guest. The QEMU command line to turn
>>>     this feature looks something like this:
>>>
>>> 	qemu-system-s390x ... -cpu xxx,ap=on
>>>
>>> CPU model facilities:
>>>
>>> 1. The S390_FEAT_AP_QUERY_CONFIG_INFO feature indicates the AP Query
>>>     Configuration Information (QCI) facility is installed. This feature
>>>     will be enabled by the kernel only if the QCI is installed on
>>>     the host. This facility will be set by QEMU only if the
>>>     KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on.
>>>     (see CPU model features above)
>>>
>>> 2. The S390_FEAT_AP_FACILITY_TEST feature indicates that the AP
>>>     Test Facility (APFT) facility is installed. This feature will
>>>     be enabled by the kernel only if the APFT facility is installed
>>>     on the host. This facility will be set by QEMU for the KVM guest
>>>     only if the KVM_S390_VM_CPU_FEAT_AP CPU model feature is turned on.
>>>     (see CPU model features above)
>>>
> This may needs to be reworded. See my comments below.
>
>>> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
>>> ---
>>>   hw/vfio/ap.c                    |    9 +++++++++
>>>   linux-headers/asm-s390/kvm.h    |    1 +
>>>   target/s390x/cpu_features.c     |    3 +++
>>>   target/s390x/cpu_features_def.h |    3 +++
>>>   target/s390x/cpu_models.c       |   12 ++++++++++++
>>>   target/s390x/gen-features.c     |    3 +++
>>>   target/s390x/kvm.c              |    6 ++++++
>>>   7 files changed, 37 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>> index 8aa5221..b93f7d9 100644
>>> --- a/hw/vfio/ap.c
>>> +++ b/hw/vfio/ap.c
>>> @@ -19,6 +19,7 @@
>>>   #include "hw/s390x/ap-device.h"
>>>   #include "qemu/error-report.h"
>>>   #include "qemu/queue.h"
>>> +#include "cpu.h"
>>>   
>>>   #define VFIO_AP_DEVICE_TYPE      "vfio-ap"
>>>   #define AP_SYSFSDEV_PROP_NAME    "sysfsdev"
>>> @@ -87,6 +88,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>>>       Error *local_err = NULL;
>>>       int ret;
>>>   
>>> +    if (!s390_has_feat(S390_FEAT_AP)) {
>>> +        error_setg(&local_err, "Invalid device configuration: ");
>> "AP support not available" ?
>>
>> [The hint is not visible in every circumstance IIRC]
>>
> I agree, it does not make sense to split this into a message and
> a hint.
>
> [..]
>
>>> @@ -900,6 +902,16 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
>>>       cpu->model->cpu_id_format = max_model->cpu_id_format;
>>>       cpu->model->cpu_ver = max_model->cpu_ver;
>>>   
>>> +    /*
>>> +     * If the AP facilities are not installed on the guest, then it makes
>>
>> "not provided in the model" ?
>>
>>> +     * no sense to enable the QCI or APFT facilities because they are only
>>> +     * needed by AP facilities.
>>> +     */
>>> +    if (!test_bit(S390_FEAT_AP, cpu->model->features)) {
>>> +        clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, cpu->model->features);
>>> +        clear_bit(S390_FEAT_AP_FACILITIES_TEST, cpu->model->features);
>>> +    }
>>> +
> I don't like this at all. Never liked software that overrules my user input.
> If I say --cpu z13,ap=off,qci=on,apft=on this would silently overrule to
> --cpu z13,ap=off,qci=off,apft=off from the guest perspective. There also might be
> other reasons why this is a bad idea.
>
>>>       check_consistency(cpu->model);
>>>       check_compatibility(max_model, cpu->model, errp);
>>>       if (*errp) {
>>> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
>>> index 0cdbc15..2d01b52 100644
>>> --- a/target/s390x/gen-features.c
>>> +++ b/target/s390x/gen-features.c
>>> @@ -447,6 +447,9 @@ static uint16_t full_GEN12_GA1[] = {
>>>       S390_FEAT_ADAPTER_INT_SUPPRESSION,
>>>       S390_FEAT_EDAT_2,
>>>       S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
>>> +    S390_FEAT_AP,
>>> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>>> +    S390_FEAT_AP_FACILITIES_TEST,
>>>   };
>>>   
>>>   static uint16_t full_GEN12_GA2[] = {
>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>> index e13c890..ae20ed8 100644
>>> --- a/target/s390x/kvm.c
>>> +++ b/target/s390x/kvm.c
>>> @@ -2105,6 +2105,7 @@ static int kvm_to_feat[][2] = {
>>>       { KVM_S390_VM_CPU_FEAT_PFMFI, S390_FEAT_SIE_PFMFI},
>>>       { KVM_S390_VM_CPU_FEAT_SIGPIF, S390_FEAT_SIE_SIGPIF},
>>>       { KVM_S390_VM_CPU_FEAT_KSS, S390_FEAT_SIE_KSS},
>>> +    { KVM_S390_VM_CPU_FEAT_AP, S390_FEAT_AP},
>>>   };
>>>   
>>>   static int query_cpu_feat(S390FeatBitmap features)
>>> @@ -2214,6 +2215,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>>>           error_setg(errp, "KVM: Error querying CPU features: %d", rc);
>>>           return;
>>>       }
>>> +    /* AP facilities support is required to enable QCI and APFT support */
>>> +    if (!test_bit(S390_FEAT_AP, model->features)) {
>>> +        clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, model->features);
>>> +        clear_bit(S390_FEAT_AP_FACILITIES_TEST, model->features);
>>> +    }
>> Hm, do you need this twice?
> In my opinion this has only value if we assume that HW and/or KVM is buggy and
> we are running host model (or it's expansion).
>
> And even the we would get a warning, and nothing bad would happen with a linux
> guest.
It is going, going ..... gone!
>
> While I'm not strongly opposing this, I would not mind it dropped. If we want
> to make sure things are consistent I would prefer the consistency check being
> generating an error (instead of a warning).
>
>>>       /* get supported cpu subfunctions indicated via query / test bit */
>>>       rc = query_cpu_subfunc(model->features);
>>>       if (rc) {
>> I'm leaving a general review of the cpu model things to David.
I'm looking forward to David's comments.
>>
> Except for these it's LGTM (r-b level LGTM).

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Tony Krowiak]

On 02/27/2018 12:52 PM, David Hildenbrand wrote:
>>       vfio_group = vfio_ap_get_group(vapdev, &local_err);
>>       if (!vfio_group) {
>>           goto out_err;
>> diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
>> index 11def14..35a6d04 100644
>> --- a/linux-headers/asm-s390/kvm.h
>> +++ b/linux-headers/asm-s390/kvm.h
>> @@ -130,6 +130,7 @@ struct kvm_s390_vm_cpu_machine {
>>   #define KVM_S390_VM_CPU_FEAT_PFMFI	11
>>   #define KVM_S390_VM_CPU_FEAT_SIGPIF	12
>>   #define KVM_S390_VM_CPU_FEAT_KSS	13
>> +#define KVM_S390_VM_CPU_FEAT_AP		14
>>   struct kvm_s390_vm_cpu_feat {
>>   	__u64 feat[16];
>>   };
>> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
>> index a5619f2..65b91bd 100644
>> --- a/target/s390x/cpu_features.c
>> +++ b/target/s390x/cpu_features.c
>> @@ -36,8 +36,10 @@ static const S390FeatDef s390_features[] = {
>>       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("qci", S390_FEAT_TYPE_STFL, 12, "Query AP Configuration 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("apft", S390_FEAT_TYPE_STFL, 15, "Adjunct Processor Facilities Test 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"),
>> @@ -125,6 +127,7 @@ static const S390FeatDef s390_features[] = {
>>   
>>       FEAT_INIT("dateh2", S390_FEAT_TYPE_MISC, 0, "DAT-enhancement facility 2"),
>>       FEAT_INIT("cmm", S390_FEAT_TYPE_MISC, 0, "Collaborative-memory-management facility"),
>> +    FEAT_INIT("ap", S390_FEAT_TYPE_MISC, 1, "AP facilities installed"),
> How exactly is this feature communicated to the guest? How does KVM
> sense support for it?
KVM detects whether the AP instructions are installed on the host. If
the instructions are installed, the feature is allowed (enabled) and
can be turned on by userspace (QEMU).
>
> IOW: is this really a CPU model feature?
I believe it is a necessary feature and came about due to review comments
from Christian and Connie in v1.
>
>>   
>>       FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32 bit in general registers)"),
>>       FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64 bit in parameter list)"),
>> diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
>> index 7c5915c..8998b65 100644
>> --- a/target/s390x/cpu_features_def.h
>> +++ b/target/s390x/cpu_features_def.h
>> @@ -27,8 +27,10 @@ typedef enum {
>>       S390_FEAT_SENSE_RUNNING_STATUS,
>>       S390_FEAT_CONDITIONAL_SSKE,
>>       S390_FEAT_CONFIGURATION_TOPOLOGY,
>> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>>       S390_FEAT_IPTE_RANGE,
>>       S390_FEAT_NONQ_KEY_SETTING,
>> +    S390_FEAT_AP_FACILITIES_TEST,
>>       S390_FEAT_EXTENDED_TRANSLATION_2,
>>       S390_FEAT_MSA,
>>       S390_FEAT_LONG_DISPLACEMENT,
>> @@ -118,6 +120,7 @@ typedef enum {
>>       /* Misc */
>>       S390_FEAT_DAT_ENH_2,
>>       S390_FEAT_CMM,
>> +    S390_FEAT_AP,
>>   
>>       /* PLO */
>>       S390_FEAT_PLO_CL,
>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>> index 1d5f0da..35f91ea 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -770,6 +770,8 @@ static void check_consistency(const S390CPUModel *model)
>>           { S390_FEAT_PRNO_TRNG_QRTCR, S390_FEAT_MSA_EXT_5 },
>>           { S390_FEAT_PRNO_TRNG, S390_FEAT_MSA_EXT_5 },
>>           { S390_FEAT_SIE_KSS, S390_FEAT_SIE_F2 },
>> +        { S390_FEAT_AP_QUERY_CONFIG_INFO, S390_FEAT_AP },
>> +        { S390_FEAT_AP_FACILITIES_TEST, S390_FEAT_AP },
>>       };
>>       int i;
>>   
>> @@ -900,6 +902,16 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
>>       cpu->model->cpu_id_format = max_model->cpu_id_format;
>>       cpu->model->cpu_ver = max_model->cpu_ver;
>>   
>> +    /*
>> +     * If the AP facilities are not installed on the guest, then it makes
>> +     * no sense to enable the QCI or APFT facilities because they are only
>> +     * needed by AP facilities.
>> +     */
>> +    if (!test_bit(S390_FEAT_AP, cpu->model->features)) {
>> +        clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, cpu->model->features);
>> +        clear_bit(S390_FEAT_AP_FACILITIES_TEST, cpu->model->features);
>> +    }
> Please don't silently disable things. Instead
>
> a) Add consistency checks (check_consistency())
> b) Mask the bits out in the KVM CPU model sensing part
>    (kvm_s390_get_host_cpu_model()) - which you already have :)
This is a remnant of a previous iteration that somehow made its way into
this patch series. It will be removed.
>
>> +
>>       check_consistency(cpu->model);
>>       check_compatibility(max_model, cpu->model, errp);
>>       if (*errp) {
>> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
>> index 0cdbc15..2d01b52 100644
>> --- a/target/s390x/gen-features.c
>> +++ b/target/s390x/gen-features.c
>> @@ -447,6 +447,9 @@ static uint16_t full_GEN12_GA1[] = {
>>       S390_FEAT_ADAPTER_INT_SUPPRESSION,
>>       S390_FEAT_EDAT_2,
>>       S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
>> +    S390_FEAT_AP,
>> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>> +    S390_FEAT_AP_FACILITIES_TEST,
>>   };
> Please keep the order as defined in target/s390x/cpu_features_def.h
Will do.
>
>>   
>>   static uint16_t full_GEN12_GA2[] = {
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index e13c890..ae20ed8 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -2105,6 +2105,7 @@ static int kvm_to_feat[][2] = {
>>       { KVM_S390_VM_CPU_FEAT_PFMFI, S390_FEAT_SIE_PFMFI},
>>       { KVM_S390_VM_CPU_FEAT_SIGPIF, S390_FEAT_SIE_SIGPIF},
>>       { KVM_S390_VM_CPU_FEAT_KSS, S390_FEAT_SIE_KSS},
>> +    { KVM_S390_VM_CPU_FEAT_AP, S390_FEAT_AP},
> Nothing speaks against the STFL bits, want to learn more about the
> S390_FEAT_AP feature :)
There are a couple of primary reasons for the addition of this feature.

* Let's start with the fact that AP instructions absolutely must be 
installed on the host in order to virtualize
   AP devices for a guest using this patch series. There is a bit in the 
in the SIE block (ECA.28) that controls
   whether AP instructions executed on the guest are interpreted by SIE 
or intercepted by KVM. This patch series sets
   this bit so that AP instructions executed on the guest are 
interpreted by the firmware passed directly through
   to the AP devices configured for the guest in the CRYCB- a satellite 
control block of the SIE block to configure
   the AP facilities for the guest. If the AP instructions are not 
installed, the AP bus running in the guest will
   not initialize and the guest will not have access to any AP devices. 
So, the primary reason for the S390_FEAT_AP
   feature is to protect against this scenario.

* In the review of v1, Christian suggested creating a feature to prevent 
migration of a guest with AP devices
   to a system without AP support, or a system without AP instructions. 
In order to migrate to another system,
   the S390_FEAT_AP feature must be available on the target system.

I hope this clears things up for you.
>
>

Re: [Qemu-devel] [PATCH v2 4/5] s390x/vfio: ap: Introduce VFIO AP device

[Tony Krowiak]

On 02/27/2018 12:04 PM, Cornelia Huck wrote:
> On Tue, 27 Feb 2018 10:44:18 -0500
> Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote:
>
>> Introduces a VFIO based AP device. The device is defined via
>> the QEMU command line by specifying:
>>
>>      -device vfio-ap,sysfsdev=<path-to-mediated-matrix-device>
>>
>> The mediated matrix device is created by the VFIO AP device
>> driver by writing a UUID to a sysfs attribute file (see
>> docs/vfio-ap.txt). The mediated matrix device will be named
>> after the UUID. Symbolic links to the $uuid are created in
>> many places, so the path to the mediated matrix device $uuid
>> can be specified in any of the following ways:
>>
>> /sys/devices/vfio_ap/matrix/$uuid
>> /sys/devices/vfio_ap/matrix/mdev_supported_types/vfio_ap-passthrough/devices/$uuid
>> /sys/bus/mdev/devices/$uuid
>> /sys/bus/mdev/drivers/vfio_mdev/$uuid
>>
>> When the vfio-ap device is realized, it acquires and opens the
>> VFIO iommu group to which the mediated matrix device is
>> bound. This causes a VFIO group notification event to be
>> signaled. The vfio_ap device driver's group notification
>> handler will get called at which time the device driver
>> will configure the the AP devices to which the guest will
>> be granted access.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com>
>> ---
>>   default-configs/s390x-softmmu.mak |    1 +
>>   hw/vfio/Makefile.objs             |    1 +
>>   hw/vfio/ap.c                      |  167 +++++++++++++++++++++++++++++++++++++
>>   include/hw/vfio/vfio-common.h     |    1 +
>>   4 files changed, 170 insertions(+), 0 deletions(-)
>>   create mode 100644 hw/vfio/ap.c
>>
>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>> new file mode 100644
>> index 0000000..8aa5221
>> --- /dev/null
>> +++ b/hw/vfio/ap.c
>> @@ -0,0 +1,167 @@
>> +/*
>> + * VFIO based AP matrix device assignment
>> + *
>> + * Copyright 2017 IBM Corp.
> Happy new year?
Happy New Year to you too ;)
>
> [Also the other new files, here and in the Linux part.]
I'll fix this in all new files in the patch series.
>
>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or(at
>> + * your option) any version. See the COPYING file in the top-level
> That probably should be "any later version" (I'm not even sure what GPL
> v1 says :)
Will do.
>
> And I just noticed that the vfio-ccw code has the same problem...
I'm not fixing those :)
>
>> + * directory.
>> + */
>> +
>> +#include <linux/vfio.h>
>> +#include <sys/ioctl.h>
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "hw/sysbus.h"
>> +#include "hw/vfio/vfio.h"
>> +#include "hw/vfio/vfio-common.h"
>> +#include "hw/s390x/ap-device.h"
>> +#include "qemu/error-report.h"
>> +#include "qemu/queue.h"
>> +
>> +#define VFIO_AP_DEVICE_TYPE      "vfio-ap"
>> +#define AP_SYSFSDEV_PROP_NAME    "sysfsdev"
> Using a #define for a property name seems unusual (and I think it
> decreases readability).
I'll remove it
>
> Otherwise, looks fine (on first read-through).
>

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[David Hildenbrand]

> KVM detects whether the AP instructions are installed on the host. If
> the instructions are installed, the feature is allowed (enabled) and
> can be turned on by userspace (QEMU).

As mentioned in the KVM thread, I'd like to verify if there is not a AP
interpretation facility.

>>
>> IOW: is this really a CPU model feature?
> I believe it is a necessary feature and came about due to review comments
> from Christian and Connie in v1.

Yes, I can see how this is guest ABI. But we always have to take care of
ever potentially wanting to emulate this in QEMU. But if we can turn
interpretation on/off using the feature flag, I am happy.

>>
>>>   
>>>       FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32 bit in general registers)"),
>>>       FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64 bit in parameter list)"),
>>> diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
>>> index 7c5915c..8998b65 100644
>>> --- a/target/s390x/cpu_features_def.h
>>> +++ b/target/s390x/cpu_features_def.h
>>> @@ -27,8 +27,10 @@ typedef enum {
>>>       S390_FEAT_SENSE_RUNNING_STATUS,
>>>       S390_FEAT_CONDITIONAL_SSKE,
>>>       S390_FEAT_CONFIGURATION_TOPOLOGY,
>>> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>>>       S390_FEAT_IPTE_RANGE,
>>>       S390_FEAT_NONQ_KEY_SETTING,
>>> +    S390_FEAT_AP_FACILITIES_TEST,
>>>       S390_FEAT_EXTENDED_TRANSLATION_2,
>>>       S390_FEAT_MSA,
>>>       S390_FEAT_LONG_DISPLACEMENT,
>>> @@ -118,6 +120,7 @@ typedef enum {
>>>       /* Misc */
>>>       S390_FEAT_DAT_ENH_2,
>>>       S390_FEAT_CMM,
>>> +    S390_FEAT_AP,
>>>   
>>>       /* PLO */
>>>       S390_FEAT_PLO_CL,
>>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>>> index 1d5f0da..35f91ea 100644
>>> --- a/target/s390x/cpu_models.c
>>> +++ b/target/s390x/cpu_models.c
>>> @@ -770,6 +770,8 @@ static void check_consistency(const S390CPUModel *model)
>>>           { S390_FEAT_PRNO_TRNG_QRTCR, S390_FEAT_MSA_EXT_5 },
>>>           { S390_FEAT_PRNO_TRNG, S390_FEAT_MSA_EXT_5 },
>>>           { S390_FEAT_SIE_KSS, S390_FEAT_SIE_F2 },
>>> +        { S390_FEAT_AP_QUERY_CONFIG_INFO, S390_FEAT_AP },
>>> +        { S390_FEAT_AP_FACILITIES_TEST, S390_FEAT_AP },

Saw this way too late :)

>>
>>>   
>>>   static uint16_t full_GEN12_GA2[] = {
>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>> index e13c890..ae20ed8 100644
>>> --- a/target/s390x/kvm.c
>>> +++ b/target/s390x/kvm.c
>>> @@ -2105,6 +2105,7 @@ static int kvm_to_feat[][2] = {
>>>       { KVM_S390_VM_CPU_FEAT_PFMFI, S390_FEAT_SIE_PFMFI},
>>>       { KVM_S390_VM_CPU_FEAT_SIGPIF, S390_FEAT_SIE_SIGPIF},
>>>       { KVM_S390_VM_CPU_FEAT_KSS, S390_FEAT_SIE_KSS},
>>> +    { KVM_S390_VM_CPU_FEAT_AP, S390_FEAT_AP},
>> Nothing speaks against the STFL bits, want to learn more about the
>> S390_FEAT_AP feature :)
> There are a couple of primary reasons for the addition of this feature.
> 
> * Let's start with the fact that AP instructions absolutely must be 
> installed on the host in order to virtualize
>    AP devices for a guest using this patch series. There is a bit in the 
> in the SIE block (ECA.28) that controls
>    whether AP instructions executed on the guest are interpreted by SIE 
> or intercepted by KVM. This patch series sets
>    this bit so that AP instructions executed on the guest are 
> interpreted by the firmware passed directly through
>    to the AP devices configured for the guest in the CRYCB- a satellite 
> control block of the SIE block to configure
>    the AP facilities for the guest. If the AP instructions are not 
> installed, the AP bus running in the guest will
>    not initialize and the guest will not have access to any AP devices. 
> So, the primary reason for the S390_FEAT_AP
>    feature is to protect against this scenario.

Then I request the following change in KVM:

If KVM_S390_VM_CPU_FEAT_AP is enabled in KVM, ECA.28 is _always_ set
(not just if an AP device is configured). This especially makes things a
lot easier when it comes to handling hotplugged CPUs and avoiding race
conditions when enabling these bits as mentioned in the KVM series.

KVM_S390_VM_CPU_FEAT_AP == AP instructions available for the guest
(don't throw an operation exception).

So this feature then really is guest ABI. The instructions are
available. If there is no device configured, bad luck.

> 
> * In the review of v1, Christian suggested creating a feature to prevent 
> migration of a guest with AP devices
>    to a system without AP support, or a system without AP instructions. 
> In order to migrate to another system,
>    the S390_FEAT_AP feature must be available on the target system.

I wonder if it would make sense to split this even further up.

E.g. to be able to differentiate between format0 and format2 crycb
format support. (which is necessary to properly migrate guests with
vSIE) But these would then be SIE specific CPU features in addition most
properly. But also depend if there is a AP interpretation facility :)

> 
> I hope this clears things up for you.

Yes, very helpful, thanks a lot!


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[David Hildenbrand]

> The ap_bus has a function for determining if the ap instructions are
> installed. I think it's basically trial execution. 
> 

Okay, just like CMM. Bad system design. But it is what it is.

>>
> 
> I think it's best modeled with a CPU model feature. In the end
> it's about having or not having ap instructions in the guest, and
> making stable guest ABI is exactly the thing of cpu-models AFAIU.

Indeed, as mentioned in the other mail, the AP feature but then always
has to say "AP instructions available", not just if an AP device has
been defined.

> 
>>>  
>>>      FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32 bit in general registers)"),
>>>      FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64 bit in parameter list)"),
>>> diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
>>> index 7c5915c..8998b65 100644
>>> --- a/target/s390x/cpu_features_def.h
>>> +++ b/target/s390x/cpu_features_def.h
>>> @@ -27,8 +27,10 @@ typedef enum {
>>>      S390_FEAT_SENSE_RUNNING_STATUS,
>>>      S390_FEAT_CONDITIONAL_SSKE,
>>>      S390_FEAT_CONFIGURATION_TOPOLOGY,
>>> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>>>      S390_FEAT_IPTE_RANGE,
>>>      S390_FEAT_NONQ_KEY_SETTING,
>>> +    S390_FEAT_AP_FACILITIES_TEST,
>>>      S390_FEAT_EXTENDED_TRANSLATION_2,
>>>      S390_FEAT_MSA,
>>>      S390_FEAT_LONG_DISPLACEMENT,
>>> @@ -118,6 +120,7 @@ typedef enum {
>>>      /* Misc */
>>>      S390_FEAT_DAT_ENH_2,
>>>      S390_FEAT_CMM,
>>> +    S390_FEAT_AP,
>>>  
>>>      /* PLO */
>>>      S390_FEAT_PLO_CL,
>>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>>> index 1d5f0da..35f91ea 100644
>>> --- a/target/s390x/cpu_models.c
>>> +++ b/target/s390x/cpu_models.c
>>> @@ -770,6 +770,8 @@ static void check_consistency(const S390CPUModel *model)
>>>          { S390_FEAT_PRNO_TRNG_QRTCR, S390_FEAT_MSA_EXT_5 },
>>>          { S390_FEAT_PRNO_TRNG, S390_FEAT_MSA_EXT_5 },
>>>          { S390_FEAT_SIE_KSS, S390_FEAT_SIE_F2 },
>>> +        { S390_FEAT_AP_QUERY_CONFIG_INFO, S390_FEAT_AP },
>>> +        { S390_FEAT_AP_FACILITIES_TEST, S390_FEAT_AP },
>>>      };
>>>      int i;
>>>  
>>> @@ -900,6 +902,16 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
>>>      cpu->model->cpu_id_format = max_model->cpu_id_format;
>>>      cpu->model->cpu_ver = max_model->cpu_ver;
>>>  
>>> +    /*
>>> +     * If the AP facilities are not installed on the guest, then it makes
>>> +     * no sense to enable the QCI or APFT facilities because they are only
>>> +     * needed by AP facilities.
>>> +     */
>>> +    if (!test_bit(S390_FEAT_AP, cpu->model->features)) {
>>> +        clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, cpu->model->features);
>>> +        clear_bit(S390_FEAT_AP_FACILITIES_TEST, cpu->model->features);
>>> +    }
>>
>> Please don't silently disable things. Instead
>>
> 
> I agree, this has to go (already commented on this).
>  
>> a) Add consistency checks (check_consistency())
> 
> The consistency checks are already in place. As already stated
> before, one could make it produce an error.
> 
>> b) Mask the bits out in the KVM CPU model sensing part
>>   (kvm_s390_get_host_cpu_model()) - which you already have :)
>>
> 
> Getting no ap but qci and apft indicated by KVM is unlikely to
> happen, ever.

I assume it would happen right now under vSIE (or I missed where we
enable the new ECA bit in the vSIE code). Having such simple masking
operations in the "sensing" part usually doesn't hurt.

We try to produce a consistent model even though the hardware/KVM might
be weird.

> 
>>> +
>>>      check_consistency(cpu->model);
>>>      check_compatibility(max_model, cpu->model, errp);
>>>      if (*errp) {
>>> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
>>> index 0cdbc15..2d01b52 100644
>>> --- a/target/s390x/gen-features.c
>>> +++ b/target/s390x/gen-features.c
>>> @@ -447,6 +447,9 @@ static uint16_t full_GEN12_GA1[] = {
>>>      S390_FEAT_ADAPTER_INT_SUPPRESSION,
>>>      S390_FEAT_EDAT_2,
>>>      S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
>>> +    S390_FEAT_AP,
>>> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>>> +    S390_FEAT_AP_FACILITIES_TEST,
>>>  };
>>
>> Please keep the order as defined in target/s390x/cpu_features_def.h
>>
>>>  
>>>  static uint16_t full_GEN12_GA2[] = {
>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>> index e13c890..ae20ed8 100644
>>> --- a/target/s390x/kvm.c
>>> +++ b/target/s390x/kvm.c
>>> @@ -2105,6 +2105,7 @@ static int kvm_to_feat[][2] = {
>>>      { KVM_S390_VM_CPU_FEAT_PFMFI, S390_FEAT_SIE_PFMFI},
>>>      { KVM_S390_VM_CPU_FEAT_SIGPIF, S390_FEAT_SIE_SIGPIF},
>>>      { KVM_S390_VM_CPU_FEAT_KSS, S390_FEAT_SIE_KSS},
>>> +    { KVM_S390_VM_CPU_FEAT_AP, S390_FEAT_AP},
>>
>> Nothing speaks against the STFL bits, want to learn more about the
>> S390_FEAT_AP feature :)
>>
>>
> 
> Kernel series wise what you are looking for is 
> '[PATCH v2 04/15] KVM: s390: CPU model support for AP virtualization'(MID: <1519741693-17440-5-git-send-email-akrowiak@linux.vnet.ibm.com>) 
> and
> '[PATCH v2 08/15] KVM: s390: interface to enable AP execution mode' (MID: <1519741693-17440-9-git-send-email-akrowiak@linux.vnet.ibm.com>)
> 

Found it, thanks for the pointer!

> Happy reviewing!
> 
> Regards,
> Halil
> 
> 
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Cornelia Huck]

On Wed, 28 Feb 2018 11:26:30 +0100
David Hildenbrand <david@redhat.com> wrote:

> Then I request the following change in KVM:
> 
> If KVM_S390_VM_CPU_FEAT_AP is enabled in KVM, ECA.28 is _always_ set
> (not just if an AP device is configured). This especially makes things a
> lot easier when it comes to handling hotplugged CPUs and avoiding race
> conditions when enabling these bits as mentioned in the KVM series.
> 
> KVM_S390_VM_CPU_FEAT_AP == AP instructions available for the guest
> (don't throw an operation exception).
> 
> So this feature then really is guest ABI. The instructions are
> available. If there is no device configured, bad luck.

Sounds sensible from my POV.

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Pierre Morel]

On 28/02/2018 12:40, Cornelia Huck wrote:
> On Wed, 28 Feb 2018 11:26:30 +0100
> David Hildenbrand <david@redhat.com> wrote:
>
>> Then I request the following change in KVM:
>>
>> If KVM_S390_VM_CPU_FEAT_AP is enabled in KVM, ECA.28 is _always_ set
>> (not just if an AP device is configured). This especially makes things a
>> lot easier when it comes to handling hotplugged CPUs and avoiding race
>> conditions when enabling these bits as mentioned in the KVM series.
>>
>> KVM_S390_VM_CPU_FEAT_AP == AP instructions available for the guest
>> (don't throw an operation exception).
>>
>> So this feature then really is guest ABI. The instructions are
>> available. If there is no device configured, bad luck.
> Sounds sensible from my POV.
>

I have a concern with this proposition and with the original code:

1) ap=on is a guest ABI feature saying to the guest you can use AP 
instructions

2) How we provide AP instructions to the guest can be done in three 
different ways:
  - SIE Interpretation
  - interception with VFIO
  - interception with emulation

3) We implement this with a device in QEMU and a certain level kernel 
support.

It seems possible to set or not ECA.28 , based on the type of kernel device:
- SIE interpretation -> MATRIX KVM device -> ECA.28
- Interception with VFIO and virtualization -> no ECA.28
- interception with emulation -> no ECA.28

I understand the concern with the vCPU but I think we can handle it with 
an indirect variable
like:

SIE interpretation Device + KVM_S390_VM_CPU_FEAT_AP -> set the variable  
ap_to_be_sie_interpreted=1
Then in vCPU initialization set ECA.28 based on this variable.

I think it let us more doors open, what is your opinion?

Regards,

Pierre


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[David Hildenbrand]

On 01.03.2018 15:12, Pierre Morel wrote:
> On 28/02/2018 12:40, Cornelia Huck wrote:
>> On Wed, 28 Feb 2018 11:26:30 +0100
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> Then I request the following change in KVM:
>>>
>>> If KVM_S390_VM_CPU_FEAT_AP is enabled in KVM, ECA.28 is _always_ set
>>> (not just if an AP device is configured). This especially makes things a
>>> lot easier when it comes to handling hotplugged CPUs and avoiding race
>>> conditions when enabling these bits as mentioned in the KVM series.
>>>
>>> KVM_S390_VM_CPU_FEAT_AP == AP instructions available for the guest
>>> (don't throw an operation exception).
>>>
>>> So this feature then really is guest ABI. The instructions are
>>> available. If there is no device configured, bad luck.
>> Sounds sensible from my POV.
>>
> 
> I have a concern with this proposition and with the original code:

Very good, this is exactly what I talked to Conny about yesterday.

Short version: CPU model is guest ABI, everything else is configuration.

> 
> 1) ap=on is a guest ABI feature saying to the guest you can use AP 
> instructions

Indeed, that's what belongs into the CPU model.

> 
> 2) How we provide AP instructions to the guest can be done in three 
> different ways:
>   - SIE Interpretation
>   - interception with VFIO
>   - interception with emulation
> 

Due to bad AP design we can't handle this like zPCI - completely in
QEMU. That's what should control it.

> 3) We implement this with a device in QEMU and a certain level kernel 
> support.
> 
> It seems possible to set or not ECA.28 , based on the type of kernel device:
> - SIE interpretation -> MATRIX KVM device -> ECA.28
> - Interception with VFIO and virtualization -> no ECA.28
> - interception with emulation -> no ECA.28
> 
> I understand the concern with the vCPU but I think we can handle it with 
> an indirect variable
> like:
> 
> SIE interpretation Device + KVM_S390_VM_CPU_FEAT_AP -> set the variable  
> ap_to_be_sie_interpreted=1
> Then in vCPU initialization set ECA.28 based on this variable.

That's exactly why we have the cpu feature interface. E.g. CMMA -> if
not enabled, not interpreted by HW (however also not intercepted to user
space - no sense in doing that right now).

> 
> I think it let us more doors open, what is your opinion?

In general, for now we don't care. The kernel is the only AP bus provider.

If KVM presents AP support -> AP feature can be enabled. And it should
always enable it.

Once we have a QEMU AP bus implementation, things get more complicated.
We could provide the AP feature also without KVM (like zPCI).

But weather or not to enable the KVM control has to be concluded from
the other configuration. Only user space can now that and has to decide
before enabling AP in KVM.

So I think for now we are fine. Later, this might be tricky to check but
not impossible.

> 
> Regards,
> 
> Pierre
> 
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Halil Pasic]

On 03/01/2018 03:36 PM, David Hildenbrand wrote:
>> I have a concern with this proposition and with the original code:
> Very good, this is exactly what I talked to Conny about yesterday.
> 
> Short version: CPU model is guest ABI, everything else is configuration.
> 

Nod.

> 
> So I think for now we are fine. Later, this might be tricky to check but
> not impossible.
>

I think we are all in agreement on the important stuff. I think, we also all
agree, that certain things need to be improved. E.g. the KVM code
manipulating ECA.28, -cpu xxx,ap=on needs to imply no Operation Exception when
guest executes an AP instruction (this is currently not the case as
we can have a vm a vfio-ap device -- so open won't get called -- but with ap=on).

Hope Tony will address these in the next version.

Regards,
Halil

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Tony Krowiak]

On 03/01/2018 09:12 AM, Pierre Morel wrote:
> On 28/02/2018 12:40, Cornelia Huck wrote:
>> On Wed, 28 Feb 2018 11:26:30 +0100
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> Then I request the following change in KVM:
>>>
>>> If KVM_S390_VM_CPU_FEAT_AP is enabled in KVM, ECA.28 is _always_ set
>>> (not just if an AP device is configured). This especially makes 
>>> things a
>>> lot easier when it comes to handling hotplugged CPUs and avoiding race
>>> conditions when enabling these bits as mentioned in the KVM series.
>>>
>>> KVM_S390_VM_CPU_FEAT_AP == AP instructions available for the guest
>>> (don't throw an operation exception).
>>>
>>> So this feature then really is guest ABI. The instructions are
>>> available. If there is no device configured, bad luck.
>> Sounds sensible from my POV.
>>
>
> I have a concern with this proposition and with the original code:
>
> 1) ap=on is a guest ABI feature saying to the guest you can use AP 
> instructions
>
> 2) How we provide AP instructions to the guest can be done in three 
> different ways:
>  - SIE Interpretation
>  - interception with VFIO
>  - interception with emulation
>
> 3) We implement this with a device in QEMU and a certain level kernel 
> support.
>
> It seems possible to set or not ECA.28 , based on the type of kernel 
> device:
> - SIE interpretation -> MATRIX KVM device -> ECA.28
> - Interception with VFIO and virtualization -> no ECA.28
> - interception with emulation -> no ECA.28
>
> I understand the concern with the vCPU but I think we can handle it 
> with an indirect variable
> like:
>
> SIE interpretation Device + KVM_S390_VM_CPU_FEAT_AP -> set the 
> variable  ap_to_be_sie_interpreted=1
> Then in vCPU initialization set ECA.28 based on this variable.
>
> I think it let us more doors open, what is your opinion?
I've already implemented a proof of concept similar to what you suggest 
to verify whether it would.
I wasn't completely sure of the flow of control between the KVM 
notification to the device driver
and the vcpu setup. If the variable is set when the device driver is 
notified about KVM,
it has to happen before vcpu setup for this to work. I was able to 
verify that with my proof
of concept. This discussion really belongs in the KVM/kernel patches, so 
I am going to continue
the discussion of my proposal there.
>
> Regards,
>
> Pierre
>
>

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Tony Krowiak]

On 03/01/2018 09:36 AM, David Hildenbrand wrote:
> On 01.03.2018 15:12, Pierre Morel wrote:
>> On 28/02/2018 12:40, Cornelia Huck wrote:
>>> On Wed, 28 Feb 2018 11:26:30 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>
>>>> Then I request the following change in KVM:
>>>>
>>>> If KVM_S390_VM_CPU_FEAT_AP is enabled in KVM, ECA.28 is _always_ set
>>>> (not just if an AP device is configured). This especially makes things a
>>>> lot easier when it comes to handling hotplugged CPUs and avoiding race
>>>> conditions when enabling these bits as mentioned in the KVM series.
>>>>
>>>> KVM_S390_VM_CPU_FEAT_AP == AP instructions available for the guest
>>>> (don't throw an operation exception).
>>>>
>>>> So this feature then really is guest ABI. The instructions are
>>>> available. If there is no device configured, bad luck.
>>> Sounds sensible from my POV.
>>>
>> I have a concern with this proposition and with the original code:
> Very good, this is exactly what I talked to Conny about yesterday.
>
> Short version: CPU model is guest ABI, everything else is configuration.
>
>> 1) ap=on is a guest ABI feature saying to the guest you can use AP
>> instructions
> Indeed, that's what belongs into the CPU model.
>
>> 2) How we provide AP instructions to the guest can be done in three
>> different ways:
>>    - SIE Interpretation
>>    - interception with VFIO
>>    - interception with emulation
>>
> Due to bad AP design we can't handle this like zPCI - completely in
> QEMU. That's what should control it.
>
>> 3) We implement this with a device in QEMU and a certain level kernel
>> support.
>>
>> It seems possible to set or not ECA.28 , based on the type of kernel device:
>> - SIE interpretation -> MATRIX KVM device -> ECA.28
>> - Interception with VFIO and virtualization -> no ECA.28
>> - interception with emulation -> no ECA.28
>>
>> I understand the concern with the vCPU but I think we can handle it with
>> an indirect variable
>> like:
>>
>> SIE interpretation Device + KVM_S390_VM_CPU_FEAT_AP -> set the variable
>> ap_to_be_sie_interpreted=1
>> Then in vCPU initialization set ECA.28 based on this variable.
> That's exactly why we have the cpu feature interface. E.g. CMMA -> if
> not enabled, not interpreted by HW (however also not intercepted to user
> space - no sense in doing that right now).
I'm not sure I am interpreting what you are saying here correctly, but
in the case of AP, if ECA.28 is not set, the AP instructions will not be
interpreted by HW but WILL be intercepted and forwarded to user space.
>
>> I think it let us more doors open, what is your opinion?
> In general, for now we don't care. The kernel is the only AP bus provider.
>
> If KVM presents AP support -> AP feature can be enabled. And it should
> always enable it.
>
> Once we have a QEMU AP bus implementation, things get more complicated.
> We could provide the AP feature also without KVM (like zPCI).
>
> But weather or not to enable the KVM control has to be concluded from
> the other configuration. Only user space can now that and has to decide
> before enabling AP in KVM.
>
> So I think for now we are fine. Later, this might be tricky to check but
> not impossible.
Maybe we are applying the wrong semantics to this feature. The
premise for this feature was to control the setting of ECA.28.
It grew beyond this premise because of observations related to
future considerations about emulation and full virtualization (i.e.,
the things Pierre mentioned above). Instead of this feature
indicating AP facilities are installed on the guest, it might
behoove us to return to its original intended purpose which was
to indicate that AP instructions executed by the guest are
interpreted by HW. In this case, we can resume setting it in
the vcpu setup like it was in the earlier patch series.
>
>> Regards,
>>
>> Pierre
>>
>>
>

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Tony Krowiak]

On 03/01/2018 09:36 AM, David Hildenbrand wrote:
> On 01.03.2018 15:12, Pierre Morel wrote:
>> On 28/02/2018 12:40, Cornelia Huck wrote:
>>> On Wed, 28 Feb 2018 11:26:30 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>
>>>> Then I request the following change in KVM:
>>>>
>>>> If KVM_S390_VM_CPU_FEAT_AP is enabled in KVM, ECA.28 is _always_ set
>>>> (not just if an AP device is configured). This especially makes things a
>>>> lot easier when it comes to handling hotplugged CPUs and avoiding race
>>>> conditions when enabling these bits as mentioned in the KVM series.
>>>>
>>>> KVM_S390_VM_CPU_FEAT_AP == AP instructions available for the guest
>>>> (don't throw an operation exception).
>>>>
>>>> So this feature then really is guest ABI. The instructions are
>>>> available. If there is no device configured, bad luck.
>>> Sounds sensible from my POV.
>>>
>> I have a concern with this proposition and with the original code:
> Very good, this is exactly what I talked to Conny about yesterday.
>
> Short version: CPU model is guest ABI, everything else is configuration.
>
>> 1) ap=on is a guest ABI feature saying to the guest you can use AP
>> instructions
> Indeed, that's what belongs into the CPU model.
>
>> 2) How we provide AP instructions to the guest can be done in three
>> different ways:
>>    - SIE Interpretation
AP instructions executed on the guest will be interpreted and passed 
directly
through to a real AP device installed on the host according to the APQN
specified in the instruction, so the AP instructions must be available 
on the
host.
>>    - interception with VFIO
AP instructions executed on the guest will be intercepted, and 
interpreted by
QEMU then forwarded to a real AP device installed on the host according 
to how
the AP devices are configured in userspace - i.e., whether there is 
remapping,
multiplexing or sharing of adapters/domains etc. This also requires that AP
instructions be available on the host.
>>    - interception with emulation
AP instructions executed on the guest will be intercepted, interpreted 
by QEMU
and emulated. This will not require AP instructions be available on the 
host.

In all cases above, the need to set ECA_APIE is dependent upon the device
type configured for the guest.
>>
> Due to bad AP design we can't handle this like zPCI - completely in
> QEMU. That's what should control it.
>
>> 3) We implement this with a device in QEMU and a certain level kernel
>> support.
>>
>> It seems possible to set or not ECA.28 , based on the type of kernel device:
>> - SIE interpretation -> MATRIX KVM device -> ECA.28
>> - Interception with VFIO and virtualization -> no ECA.28
>> - interception with emulation -> no ECA.28
>>
>> I understand the concern with the vCPU but I think we can handle it with
>> an indirect variable
>> like:
>>
>> SIE interpretation Device + KVM_S390_VM_CPU_FEAT_AP -> set the variable
>> ap_to_be_sie_interpreted=1
>> Then in vCPU initialization set ECA.28 based on this variable.
> That's exactly why we have the cpu feature interface. E.g. CMMA -> if
> not enabled, not interpreted by HW (however also not intercepted to user
> space - no sense in doing that right now).
There are two factors at play here, the device type (i.e., -device 
vfio_ap) and
the KVM_S390_VM_CPU_FEAT_AP feature. Setting ECA_APIE makes no sense if the
vfio_ap device is not configured. For the passthrough implementation, this
means that the AP bus will successfully load on the guest, but there will
be no AP devices detected. If the expectation is that ap=on will allow 
access
to AP devices on the guest, that would be a mistaken assumption.

If down the road a new guest AP device is introduced that allows 
multiplexing,
device remapping etc., then it will be necessary to intercept AP 
instructions
executed on the guest. In this case, if ap=on results in setting ECA_APIE,
then the instructions will be interpreted and passed through to a device
that does not exist because it won't be defined in the guest's CRYCB.

Based on these two scenarios, I think what we are really saying with ap=on
is that the guest will require use of the AP instructions installed on the
host. Whether those instructions are executed as a result of interpretation
by the hardware or because they are executed by the device driver is a
separate matter. So, I am inclined to agree with Pierre for that reason.
ECA_APIE should be set only if ap=on (i.e., AP instructions are available
on the host) and the user of those instructions (i.e., the driver) indicate
an intent to use them.
>
>> I think it let us more doors open, what is your opinion?
> In general, for now we don't care. The kernel is the only AP bus provider.
True today, but not necessarily in the future.
>
> If KVM presents AP support -> AP feature can be enabled. And it should
> always enable it.
I disagree for the reasons stated above.
>
> Once we have a QEMU AP bus implementation, things get more complicated.
> We could provide the AP feature also without KVM (like zPCI).
I am not familiar with zPCI, so I can't comment.
>
> But weather or not to enable the KVM control has to be concluded from
> the other configuration. Only user space can now that and has to decide
> before enabling AP in KVM.
>
> So I think for now we are fine. Later, this might be tricky to check but
> not impossible.
>
>> Regards,
>>
>> Pierre
>>
>>
>

Re: [Qemu-devel] [PATCH v2 0/5] s390x: vfio-ap: guest dedicated crypto adapters

[David Hildenbrand]

On 27.02.2018 16:44, Tony Krowiak wrote:
> This patch series is the QEMU counterpart to the KVM/kernel support for 
> guest dedicated crypto adapters. The KVM/kernel model is built on the 
> VFIO mediated device framework and provides the infrastructure for 
> granting exclusive guest access to crypto devices installed on the linux 
> host. This patch series introduces a new QEMU command line option, QEMU 
> object model and CPU model features to exploit the KVM/kernel model.
> 
> See the detailed specifications for AP virtualization provided by this 
> patch set in docs/vfio-ap.txt for a more complete discussion of the 
> design introduced by this patch series.
> 
> v1 -> v2 Change log:
> ===================
> * Removed unnecessary S390APMatrixDevice, S390APMatrixDeviceClass 
> * Removed ioctl to configure the AP matrix for the guest: letting the
>   vfio_ap device driver's 'open' callback configure the AP matrix
>   for the guest
> * Removed masks from object model: Unnecessary at this point because they 
>   are not currently used 
> * Renamed:
>   * VFIOAPMatrixDevice to VFIOAPDevice
>   * VFIOAPMatrixDeviceClass to VFIOAPDeviceClass
>   * APMatrixDevice to APDevice
>   * APMatrixDeviceClass to APDeviceClass
>   * ap-matrix.c to ap.c (in hw/vfio)
>   * ap-matrix-device.c to ap-device.c (in hw/s390x)
>   * ap-matrix-device.h to ap-device.h (in include/hw/s390x)
> * Added CPU model feature for AP facilities installed on guest and 
>   facilities features for QCI Instructions Available (STFLE.12) and AP 
>   Facilities Test facility installed (STFLE.15).
> 
> Tony Krowiak (5):
>   s390x/ap: base Adjunct Processor (AP) object
>   s390x/vfio: ap: VFIO: linux header updates
>   s390x/vfio: ap: Introduce VFIO AP device
>   s390x/cpumodel: Set up CPU model for AP device support
>   s390: doc: detailed specifications for AP virtualization
> 

I'm going to highlight an issue that stems from bad HW design: The lack
of an AP interpretation facility (indication). We e.g. have something
like that for zPCI (and all other I/O besides AP as far as I remember).

Let's assume L1 provides AP to L2.
Let's assume L2 provides AP to L3.

L2 can blindly forward APs to L3 because it sees the AP facility. This
requires AP vSIE support. We have no separate way of indicating that
support, it comes with the AP feature. So let's assume L2 does not
emulate devices but uses interpretation for L3.

Everything is fine as long as L1 does not emulate AP
devices/instructions for L2. All instructions are interpreted by HW.

But what happens if L1 emulates AP devices for L2? intepretation is
disabled. QEMU handles it.

However L2 can simply forward AP devices to L3. At this point, we must
also intercept and emulate AP instructions issued by L3 in _L1_.

This is what we call the nightmare of nested virtualization (see x86),
because we have to emulate L3 instructions in L1 - but even worse, not
even in L1 kernel space but in L1 user space.


Long story short:

Making this scenario work would require a _huge_ effort (going to user
space with nested guest state - or communicating with the user space
part using some other mechanism).

So we could never provide the AP feature reliably with the SIE feature.
We want to avoid interdependence between CPU features. (because
everything else makes CPU feature detection ugly - CMMA is a good
example and the only exception so far)


Long story even shorter:

No emulated AP devices with KVM.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 0/5] s390x: vfio-ap: guest dedicated crypto adapters

[Pierre Morel]

On 06/03/2018 11:01, David Hildenbrand wrote:
> On 27.02.2018 16:44, Tony Krowiak wrote:
>> This patch series is the QEMU counterpart to the KVM/kernel support for
>> guest dedicated crypto adapters. The KVM/kernel model is built on the
>> VFIO mediated device framework and provides the infrastructure for
>> granting exclusive guest access to crypto devices installed on the linux
>> host. This patch series introduces a new QEMU command line option, QEMU
>> object model and CPU model features to exploit the KVM/kernel model.
>>
>> See the detailed specifications for AP virtualization provided by this
>> patch set in docs/vfio-ap.txt for a more complete discussion of the
>> design introduced by this patch series.
>>
>> v1 -> v2 Change log:
>> ===================
>> * Removed unnecessary S390APMatrixDevice, S390APMatrixDeviceClass
>> * Removed ioctl to configure the AP matrix for the guest: letting the
>>    vfio_ap device driver's 'open' callback configure the AP matrix
>>    for the guest
>> * Removed masks from object model: Unnecessary at this point because they
>>    are not currently used
>> * Renamed:
>>    * VFIOAPMatrixDevice to VFIOAPDevice
>>    * VFIOAPMatrixDeviceClass to VFIOAPDeviceClass
>>    * APMatrixDevice to APDevice
>>    * APMatrixDeviceClass to APDeviceClass
>>    * ap-matrix.c to ap.c (in hw/vfio)
>>    * ap-matrix-device.c to ap-device.c (in hw/s390x)
>>    * ap-matrix-device.h to ap-device.h (in include/hw/s390x)
>> * Added CPU model feature for AP facilities installed on guest and
>>    facilities features for QCI Instructions Available (STFLE.12) and AP
>>    Facilities Test facility installed (STFLE.15).
>>
>> Tony Krowiak (5):
>>    s390x/ap: base Adjunct Processor (AP) object
>>    s390x/vfio: ap: VFIO: linux header updates
>>    s390x/vfio: ap: Introduce VFIO AP device
>>    s390x/cpumodel: Set up CPU model for AP device support
>>    s390: doc: detailed specifications for AP virtualization
>>
> I'm going to highlight an issue that stems from bad HW design: The lack
> of an AP interpretation facility (indication). We e.g. have something
> like that for zPCI (and all other I/O besides AP as far as I remember).
>
> Let's assume L1 provides AP to L2.
> Let's assume L2 provides AP to L3.
>
> L2 can blindly forward APs to L3 because it sees the AP facility. This
> requires AP vSIE support. We have no separate way of indicating that
> support, it comes with the AP feature. So let's assume L2 does not
> emulate devices but uses interpretation for L3.
>
> Everything is fine as long as L1 does not emulate AP
> devices/instructions for L2. All instructions are interpreted by HW.

If L1 emulates AP, there is no need it sets any bit in the L2 SIE CRYCB.
In fact we better do not set any bit in the CRYCB.

>
> But what happens if L1 emulates AP devices for L2? intepretation is
> disabled. QEMU handles it.
>
> However L2 can simply forward AP devices to L3. At this point, we must
> also intercept and emulate AP instructions issued by L3 in _L1_.

If L2 forward devices to L3 through SIE ECA.28 but no bit is set is in 
the CRYCB of L2,
L3 will not see any device.

>
> This is what we call the nightmare of nested virtualization (see x86),
> because we have to emulate L3 instructions in L1 - but even worse, not
> even in L1 kernel space but in L1 user space.

As soon as one level begin to virtualize, all levels under it
must virtualize too so that L3 instructions will be handled in L2
which will issue instructions that will be handled in L1.

>
>
> Long story short:
>
> Making this scenario work would require a _huge_ effort (going to user
> space with nested guest state - or communicating with the user space
> part using some other mechanism).

A funny game with big overhead but same virtualization whatever the 
level is.

>
> So we could never provide the AP feature reliably with the SIE feature.

I think we should change a little this sentence to:
We can not provide SIE interpretation to a guest from which
any guest level N-1 does not use SIE interpretation.

Nothing bad will occur for the host, the hardware or other guests,
but the guest will just not get any device.

> We want to avoid interdependence between CPU features. (because
> everything else makes CPU feature detection ugly - CMMA is a good
> example and the only exception so far)
>
>
> Long story even shorter:
>
> No emulated AP devices with KVM.
>
I agree with: KVM should never set bits in CRYCB for emulated devices.


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v2 0/5] s390x: vfio-ap: guest dedicated crypto adapters

[David Hildenbrand]

> If L2 forward devices to L3 through SIE ECA.28 but no bit is set is in 
> the CRYCB of L2,
> L3 will not see any device.

Exactly and this is the problem: How should L2 know that these devices
are special and cannot be forwarded.

> 
>>
>> This is what we call the nightmare of nested virtualization (see x86),
>> because we have to emulate L3 instructions in L1 - but even worse, not
>> even in L1 kernel space but in L1 user space.
> 
> As soon as one level begin to virtualize, all levels under it
> must virtualize too so that L3 instructions will be handled in L2
> which will issue instructions that will be handled in L1.

By virtualize I assume you mean emulate? If so, yes.

>>
>> So we could never provide the AP feature reliably with the SIE feature.
> 
> I think we should change a little this sentence to:
> We can not provide SIE interpretation to a guest from which
> any guest level N-1 does not use SIE interpretation.

Exactly, and as said, there is no way to tell a guest that it has AP but
cannot use AP interpretation but has to intercept and handle manually.

> 
> Nothing bad will occur for the host, the hardware or other guests,
> but the guest will just not get any device.
> 
>> We want to avoid interdependence between CPU features. (because
>> everything else makes CPU feature detection ugly - CMMA is a good
>> example and the only exception so far)
>>
>>
>> Long story even shorter:
>>
>> No emulated AP devices with KVM.
>>
> I agree with: KVM should never set bits in CRYCB for emulated devices.

I think this is stronger: emulated AP devices should not be used with
KVM because it can potentially lead to architectural (v)SIE conflicts.

But the details are buried in some AP documentation not accessible to me.

Anyhow, if the scenario I described cannot be worked around via:

a) telling a guest that AP virtualization cannot be used - which doesn't
seem to be possible
b) provoking for selected devices a SIE exit when an AP instruction is
executed on these devices - and this is totally fine with the documented
AP architecture

I assume we would have to live with !emualted AP devices.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[David Hildenbrand]

>>> 1) ap=on is a guest ABI feature saying to the guest you can use AP
>>> instructions
>> Indeed, that's what belongs into the CPU model.
>>
>>> 2) How we provide AP instructions to the guest can be done in three
>>> different ways:
>>>    - SIE Interpretation
> AP instructions executed on the guest will be interpreted and passed 
> directly
> through to a real AP device installed on the host according to the APQN
> specified in the instruction, so the AP instructions must be available 
> on the
> host.
>>>    - interception with VFIO
> AP instructions executed on the guest will be intercepted, and 
> interpreted by
> QEMU then forwarded to a real AP device installed on the host according 
> to how
> the AP devices are configured in userspace - i.e., whether there is 
> remapping,
> multiplexing or sharing of adapters/domains etc. This also requires that AP
> instructions be available on the host.

See my other mail: I think this is a conflict with vSIE.

>>>    - interception with emulation
> AP instructions executed on the guest will be intercepted, interpreted 
> by QEMU
> and emulated. This will not require AP instructions be available on the 
> host.

See my other mail: I think this is a conflict with vSIE.

> 
> In all cases above, the need to set ECA_APIE is dependent upon the device
> type configured for the guest.
>>>
>> Due to bad AP design we can't handle this like zPCI - completely in
>> QEMU. That's what should control it.
>>
>>> 3) We implement this with a device in QEMU and a certain level kernel
>>> support.
>>>
>>> It seems possible to set or not ECA.28 , based on the type of kernel device:
>>> - SIE interpretation -> MATRIX KVM device -> ECA.28
>>> - Interception with VFIO and virtualization -> no ECA.28
>>> - interception with emulation -> no ECA.28
>>>
>>> I understand the concern with the vCPU but I think we can handle it with
>>> an indirect variable
>>> like:
>>>
>>> SIE interpretation Device + KVM_S390_VM_CPU_FEAT_AP -> set the variable
>>> ap_to_be_sie_interpreted=1
>>> Then in vCPU initialization set ECA.28 based on this variable.
>> That's exactly why we have the cpu feature interface. E.g. CMMA -> if
>> not enabled, not interpreted by HW (however also not intercepted to user
>> space - no sense in doing that right now).
> There are two factors at play here, the device type (i.e., -device 
> vfio_ap) and
> the KVM_S390_VM_CPU_FEAT_AP feature. Setting ECA_APIE makes no sense if the
> vfio_ap device is not configured. For the passthrough implementation, this
> means that the AP bus will successfully load on the guest, but there will
> be no AP devices detected. If the expectation is that ap=on will allow 
> access
> to AP devices on the guest, that would be a mistaken assumption.
> 
> If down the road a new guest AP device is introduced that allows 
> multiplexing,
> device remapping etc., then it will be necessary to intercept AP 
> instructions
> executed on the guest. In this case, if ap=on results in setting ECA_APIE,
> then the instructions will be interpreted and passed through to a device
> that does not exist because it won't be defined in the guest's CRYCB.

Again, see my other mail, this discussion is superfluous if we can't get
vSIE to work with emulated devices. And it smells like this is the case.
But I don't have access to documentation.

> 
> Based on these two scenarios, I think what we are really saying with ap=on
> is that the guest will require use of the AP instructions installed on the
> host. Whether those instructions are executed as a result of interpretation
> by the hardware or because they are executed by the device driver is a
> separate matter. So, I am inclined to agree with Pierre for that reason.
> ECA_APIE should be set only if ap=on (i.e., AP instructions are available
> on the host) and the user of those instructions (i.e., the driver) indicate
> an intent to use them.

ap=on -> set ECA_APIE is what I proposed.

>>
>>> I think it let us more doors open, what is your opinion?
>> In general, for now we don't care. The kernel is the only AP bus provider.
> True today, but not necessarily in the future.

I think so (vSIE).

>>
>> If KVM presents AP support -> AP feature can be enabled. And it should
>> always enable it.
> I disagree for the reasons stated above.

By always enable I of course mean "ap=on" (the point was: independent of
devices right now).




-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Pierre Morel]

On 06/03/2018 18:15, David Hildenbrand wrote:
>>>> 1) ap=on is a guest ABI feature saying to the guest you can use AP
>>>> instructions
>>> Indeed, that's what belongs into the CPU model.
>>>
>>>> 2) How we provide AP instructions to the guest can be done in three
>>>> different ways:
>>>>     - SIE Interpretation
>> AP instructions executed on the guest will be interpreted and passed
>> directly
>> through to a real AP device installed on the host according to the APQN
>> specified in the instruction, so the AP instructions must be available
>> on the
>> host.
>>>>     - interception with VFIO
>> AP instructions executed on the guest will be intercepted, and
>> interpreted by
>> QEMU then forwarded to a real AP device installed on the host according
>> to how
>> the AP devices are configured in userspace - i.e., whether there is
>> remapping,
>> multiplexing or sharing of adapters/domains etc. This also requires that AP
>> instructions be available on the host.
> See my other mail: I think this is a conflict with vSIE.
>
>>>>     - interception with emulation
>> AP instructions executed on the guest will be intercepted, interpreted
>> by QEMU
>> and emulated. This will not require AP instructions be available on the
>> host.
> See my other mail: I think this is a conflict with vSIE.
>
>> In all cases above, the need to set ECA_APIE is dependent upon the device
>> type configured for the guest.
>>> Due to bad AP design we can't handle this like zPCI - completely in
>>> QEMU. That's what should control it.
>>>
>>>> 3) We implement this with a device in QEMU and a certain level kernel
>>>> support.
>>>>
>>>> It seems possible to set or not ECA.28 , based on the type of kernel device:
>>>> - SIE interpretation -> MATRIX KVM device -> ECA.28
>>>> - Interception with VFIO and virtualization -> no ECA.28
>>>> - interception with emulation -> no ECA.28
>>>>
>>>> I understand the concern with the vCPU but I think we can handle it with
>>>> an indirect variable
>>>> like:
>>>>
>>>> SIE interpretation Device + KVM_S390_VM_CPU_FEAT_AP -> set the variable
>>>> ap_to_be_sie_interpreted=1
>>>> Then in vCPU initialization set ECA.28 based on this variable.
>>> That's exactly why we have the cpu feature interface. E.g. CMMA -> if
>>> not enabled, not interpreted by HW (however also not intercepted to user
>>> space - no sense in doing that right now).
>> There are two factors at play here, the device type (i.e., -device
>> vfio_ap) and
>> the KVM_S390_VM_CPU_FEAT_AP feature. Setting ECA_APIE makes no sense if the
>> vfio_ap device is not configured. For the passthrough implementation, this
>> means that the AP bus will successfully load on the guest, but there will
>> be no AP devices detected. If the expectation is that ap=on will allow
>> access
>> to AP devices on the guest, that would be a mistaken assumption.
>>
>> If down the road a new guest AP device is introduced that allows
>> multiplexing,
>> device remapping etc., then it will be necessary to intercept AP
>> instructions
>> executed on the guest. In this case, if ap=on results in setting ECA_APIE,
>> then the instructions will be interpreted and passed through to a device
>> that does not exist because it won't be defined in the guest's CRYCB.
> Again, see my other mail, this discussion is superfluous if we can't get
> vSIE to work with emulated devices. And it smells like this is the case.
> But I don't have access to documentation.
>
>> Based on these two scenarios, I think what we are really saying with ap=on
>> is that the guest will require use of the AP instructions installed on the
>> host. Whether those instructions are executed as a result of interpretation
>> by the hardware or because they are executed by the device driver is a
>> separate matter. So, I am inclined to agree with Pierre for that reason.
>> ECA_APIE should be set only if ap=on (i.e., AP instructions are available
>> on the host) and the user of those instructions (i.e., the driver) indicate
>> an intent to use them.
> ap=on -> set ECA_APIE is what I proposed.

True if we only support SIE interpretation, what you propose.
but
It is not what I meant.
What I mean is the reverse implication

ECA_APIE => ap=on

But you can have ap = on and ECA_APIE = off
This is interception or emulation.

and the second thing is that we need two QEMU cpu features
AP : guest API to say we provide AP instructions to the guest (what ever 
we do to provide it)
ECA_APIE : kernel will setup the SIE with interpretation

other said:
if( !ap)
     return -ENODEVICE
if(ECA_API)
     set_interpretation()
else
     set_interception()




-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v2 0/5] s390x: vfio-ap: guest dedicated crypto adapters

[Pierre Morel]

On 06/03/2018 18:10, David Hildenbrand wrote:
>> If L2 forward devices to L3 through SIE ECA.28 but no bit is set is in
>> the CRYCB of L2,
>> L3 will not see any device.
> Exactly and this is the problem: How should L2 know that these devices
> are special and cannot be forwarded.
>
>>> This is what we call the nightmare of nested virtualization (see x86),
>>> because we have to emulate L3 instructions in L1 - but even worse, not
>>> even in L1 kernel space but in L1 user space.
>> As soon as one level begin to virtualize, all levels under it
>> must virtualize too so that L3 instructions will be handled in L2
>> which will issue instructions that will be handled in L1.
> By virtualize I assume you mean emulate? If so, yes.
>
>>> So we could never provide the AP feature reliably with the SIE feature.
>> I think we should change a little this sentence to:
>> We can not provide SIE interpretation to a guest from which
>> any guest level N-1 does not use SIE interpretation.
> Exactly, and as said, there is no way to tell a guest that it has AP but
> cannot use AP interpretation but has to intercept and handle manually.


vSIE must clear ECA28 during running of the guest if the host itself do 
not have ECA28 set.
Since ECA28 set for the host means AP instructions available for the host
then we can sum it up by: vSIE should never set ECA28 in the shadow SIE
if no AP instructions available.

Pierre


>
>> Nothing bad will occur for the host, the hardware or other guests,
>> but the guest will just not get any device.
>>
>>> We want to avoid interdependence between CPU features. (because
>>> everything else makes CPU feature detection ugly - CMMA is a good
>>> example and the only exception so far)
>>>
>>>
>>> Long story even shorter:
>>>
>>> No emulated AP devices with KVM.
>>>
>> I agree with: KVM should never set bits in CRYCB for emulated devices.
> I think this is stronger: emulated AP devices should not be used with
> KVM because it can potentially lead to architectural (v)SIE conflicts.
>
> But the details are buried in some AP documentation not accessible to me.
>
> Anyhow, if the scenario I described cannot be worked around via:
>
> a) telling a guest that AP virtualization cannot be used - which doesn't
> seem to be possible
> b) provoking for selected devices a SIE exit when an AP instruction is
> executed on these devices - and this is totally fine with the documented
> AP architecture
>
> I assume we would have to live with !emualted AP devices.
>

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v2 0/5] s390x: vfio-ap: guest dedicated crypto adapters

[Christian Borntraeger]

On 03/07/2018 11:22 AM, Pierre Morel wrote:
> On 06/03/2018 18:10, David Hildenbrand wrote:
>>> If L2 forward devices to L3 through SIE ECA.28 but no bit is set is in
>>> the CRYCB of L2,
>>> L3 will not see any device.
>> Exactly and this is the problem: How should L2 know that these devices
>> are special and cannot be forwarded.
>>
>>>> This is what we call the nightmare of nested virtualization (see x86),
>>>> because we have to emulate L3 instructions in L1 - but even worse, not
>>>> even in L1 kernel space but in L1 user space.
>>> As soon as one level begin to virtualize, all levels under it
>>> must virtualize too so that L3 instructions will be handled in L2
>>> which will issue instructions that will be handled in L1.
>> By virtualize I assume you mean emulate? If so, yes.
>>
>>>> So we could never provide the AP feature reliably with the SIE feature.
>>> I think we should change a little this sentence to:
>>> We can not provide SIE interpretation to a guest from which
>>> any guest level N-1 does not use SIE interpretation.
>> Exactly, and as said, there is no way to tell a guest that it has AP but
>> cannot use AP interpretation but has to intercept and handle manually.
> 
> 
> vSIE must clear ECA28 during running of the guest if the host itself do not have ECA28 set.
> Since ECA28 set for the host means AP instructions available for the host
> then we can sum it up by: vSIE should never set ECA28 in the shadow SIE
> if no AP instructions available.

To say it differently, architecturally ECA28 is an effective control so we might
put the burden on the guest2 by saying even it you set eca.28 you might still
get exits for NQAP,PQAP,DQAP and handle it appropriately.


> 
> Pierre
> 
> 
>>
>>> Nothing bad will occur for the host, the hardware or other guests,
>>> but the guest will just not get any device.
>>>
>>>> We want to avoid interdependence between CPU features. (because
>>>> everything else makes CPU feature detection ugly - CMMA is a good
>>>> example and the only exception so far)
>>>>
>>>>
>>>> Long story even shorter:
>>>>
>>>> No emulated AP devices with KVM.
>>>>
>>> I agree with: KVM should never set bits in CRYCB for emulated devices.
>> I think this is stronger: emulated AP devices should not be used with
>> KVM because it can potentially lead to architectural (v)SIE conflicts.
>>
>> But the details are buried in some AP documentation not accessible to me.
>>
>> Anyhow, if the scenario I described cannot be worked around via:
>>
>> a) telling a guest that AP virtualization cannot be used - which doesn't
>> seem to be possible
>> b) provoking for selected devices a SIE exit when an AP instruction is
>> executed on these devices - and this is totally fine with the documented
>> AP architecture
>>
>> I assume we would have to live with !emualted AP devices.
>>
> 

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Cornelia Huck]

On Wed, 7 Mar 2018 11:09:46 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> What I mean is the reverse implication
> 
> ECA_APIE => ap=on
> 
> But you can have ap = on and ECA_APIE = off
> This is interception or emulation.
> 
> and the second thing is that we need two QEMU cpu features
> AP : guest API to say we provide AP instructions to the guest (what ever 
> we do to provide it)
> ECA_APIE : kernel will setup the SIE with interpretation
> 
> other said:
> if( !ap)
>      return -ENODEVICE
> if(ECA_API)
>      set_interpretation()
> else
>      set_interception()

This discussion is giving me a headache, so let's take a step back and
figure out what is needed/wanted/possible.

* straight passthrough of tuples, SIE doing the heavy lifting
  -> what this patchset is doing, and should be fine, even regarding
     nesting

* remapping of tuples, SIE doing most of the work (IIRC it's possible
  to only intercept for a subset of instructions?)
  -> that's where it gets complicated, and IIUC we can't have any mixed
     straight/remap setups, and we may have issues regarding nesting
  Question: How important is that use case? Can we drop it and make our
  lives much easier?

* full emulation (which would be the only option for tcg, obviously)
  -> even if it were doable, I doubt it would be very useful
  It would be great if we could have a design that would also
  accommodate this (and I have rooted for that in the past), but the
  more I hear about the issues here, the more I doubt whether this is
  something we should spend time on.

Another question: Can some of the use cases be serviced via
virtio-crypto as well (clear key)? Would that in combination with
straight passthrough be enough?

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Pierre Morel]

On 07/03/2018 15:41, Cornelia Huck wrote:
> On Wed, 7 Mar 2018 11:09:46 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>
>> What I mean is the reverse implication
>>
>> ECA_APIE => ap=on
>>
>> But you can have ap = on and ECA_APIE = off
>> This is interception or emulation.
>>
>> and the second thing is that we need two QEMU cpu features
>> AP : guest API to say we provide AP instructions to the guest (what ever
>> we do to provide it)
>> ECA_APIE : kernel will setup the SIE with interpretation
>>
>> other said:
>> if( !ap)
>>       return -ENODEVICE
>> if(ECA_API)
>>       set_interpretation()
>> else
>>       set_interception()
> This discussion is giving me a headache, so let's take a step back and
> figure out what is needed/wanted/possible.
>
> * straight passthrough of tuples, SIE doing the heavy lifting
>    -> what this patchset is doing, and should be fine, even regarding
>       nesting
>
> * remapping of tuples, SIE doing most of the work

Currently the SIE do not allow remapping.
Only interception can allow remapping.

> (IIRC it's possible
>    to only intercept for a subset of instructions?)

More than possible: some AP instructions, when existing, are always 
intercepted and some
other are intercepted based on a specific condition and on STFLE bits 
but for them
SIE Execution control bit is ignored.

However, we do not use these instructions in this patch series.

>    -> that's where it gets complicated, and IIUC we can't have any mixed
>       straight/remap setups, and we may have issues regarding nesting

We do not have issues regarding nesting, we can not nest
a guest doing SIE interpretation inside another doing interception.
It is an architectural design, not an issue.

To guaranty this, the architecture provide Effective Execution Control,
EEC.

The handling of ECA28 for guests execution is combined into a
single description, the EECA, when operating at guest level 2
the EECA.28 is the logical AND of the guest level 1 ECA.28 and the
guest level 2 ECA.28

When using vSIE we need to propagate this handling.

>    Question: How important is that use case? Can we drop it and make our
>    lives much easier?

AFAIK, and as long as my information is up to date, we can not close the 
door to interception.

In other word, we need to separate the CPU feature defining "if AP 
instructions are available"
from the QEMU property defining "How we provide the instructions".

ECA28 obviously belongs to the "how" and not to the "if".



>
> * full emulation (which would be the only option for tcg, obviously)
>    -> even if it were doable, I doubt it would be very useful
>    It would be great if we could have a design that would also
>    accommodate this (and I have rooted for that in the past), but the
>    more I hear about the issues here, the more I doubt whether this is
>    something we should spend time on.
>
> Another question: Can some of the use cases be serviced via
> virtio-crypto as well (clear key)? Would that in combination with
> straight passthrough be enough?
>

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Tony Krowiak]

On 03/07/2018 09:41 AM, Cornelia Huck wrote:
> On Wed, 7 Mar 2018 11:09:46 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>
>> What I mean is the reverse implication
>>
>> ECA_APIE => ap=on
>>
>> But you can have ap = on and ECA_APIE = off
>> This is interception or emulation.
>>
>> and the second thing is that we need two QEMU cpu features
>> AP : guest API to say we provide AP instructions to the guest (what ever
>> we do to provide it)
>> ECA_APIE : kernel will setup the SIE with interpretation
>>
>> other said:
>> if( !ap)
>>       return -ENODEVICE
>> if(ECA_API)
>>       set_interpretation()
>> else
>>       set_interception()
> This discussion is giving me a headache, so let's take a step back and
> figure out what is needed/wanted/possible.
>
> * straight passthrough of tuples, SIE doing the heavy lifting
>    -> what this patchset is doing, and should be fine, even regarding
>       nesting
>
> * remapping of tuples, SIE doing most of the work (IIRC it's possible
>    to only intercept for a subset of instructions?)
Under the current architecture, instructions are either interpreted (with
some instructions being intercepted under specific conditions) or 
intercepted.
Therefore, when remapping tuples, it will be necessary to intercept all
instructions.
>    -> that's where it gets complicated, and IIUC we can't have any mixed
>       straight/remap setups, and we may have issues regarding nesting
As I said above, under the current architecture instructions are either
interpreted (ECA.28 is set) or intercepted (ECA.28 is cleared). 
Consequently,
we can't mix guests that use interpretation with guests that don't.
>    Question: How important is that use case? Can we drop it and make our
>    lives much easier?
We've already had requests.
>
> * full emulation (which would be the only option for tcg, obviously)
>    -> even if it were doable, I doubt it would be very useful
>    It would be great if we could have a design that would also
>    accommodate this (and I have rooted for that in the past), but the
>    more I hear about the issues here, the more I doubt whether this is
>    something we should spend time on.
If I'm not mistaken, the discussions about full emulation centered around
problems related to second level guests (VSIE). It seems possible to
employ full emulation for guest level 1. I'm not in a position to say
whether it would be worth the effort or not.
>
> Another question: Can some of the use cases be serviced via
> virtio-crypto as well (clear key)? Would that in combination with
> straight passthrough be enough?
I don't know enough about virtio-crypto to say.
>