Re: [PATCH v7 20/22] KVM: s390: add KVM_S390_ZPCI_OP to manage guest zPCI devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 16/05/2022 17.35, Matthew Rosato wrote:
On 5/16/22 5:52 AM, Thomas Huth wrote:
On 13/05/2022 21.15, Matthew Rosato wrote:
The KVM_S390_ZPCI_OP ioctl provides a mechanism for managing
hardware-assisted virtualization features for s390X zPCI passthrough.

s/s390X/s390x/

Add the first 2 operations, which can be used to enable/disable
the specified device for Adapter Event Notification interpretation.

Signed-off-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>
---
  Documentation/virt/kvm/api.rst | 45 +++++++++++++++++++
  arch/s390/kvm/kvm-s390.c       | 23 ++++++++++
  arch/s390/kvm/pci.c            | 81 ++++++++++++++++++++++++++++++++++
  arch/s390/kvm/pci.h            |  2 +
  include/uapi/linux/kvm.h       | 31 +++++++++++++
  5 files changed, 182 insertions(+)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 4a900cdbc62e..a7cd5ebce031 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -5645,6 +5645,51 @@ enabled with ``arch_prctl()``, but this may change in the future.   The offsets of the state save areas in struct kvm_xsave follow the contents
  of CPUID leaf 0xD on the host.
+4.135 KVM_S390_ZPCI_OP
+--------------------
+
+:Capability: KVM_CAP_S390_ZPCI_OP
+:Architectures: s390
+:Type: vcpu ioctl

vcpu? ... you're wiring it up in  kvm_arch_vm_ioctl() later, so I assume it's rather a VM ioctl?

Yup, VM ioctl, bad copy/paste job...


+:Parameters: struct kvm_s390_zpci_op (in)
+:Returns: 0 on success, <0 on error
+
+Used to manage hardware-assisted virtualization features for zPCI devices.
+
+Parameters are specified via the following structure::
+
+  struct kvm_s390_zpci_op {
+    /* in */

If all is "in", why is there a copy_to_user() in the code later?


Oh no, this is a leftover from a prior version...  Good catch.  There should no longer be a copy_to_user.

+    __u32 fh;        /* target device */
+    __u8  op;        /* operation to perform */
+    __u8  pad[3];
+    union {
+        /* for KVM_S390_ZPCIOP_REG_AEN */
+        struct {
+            __u64 ibv;    /* Guest addr of interrupt bit vector */
+            __u64 sb;    /* Guest addr of summary bit */

If this is really a vcpu ioctl, what kind of addresses are you talking about here? virtual addresses? real addresses? absolute addresses?

It's a VM ioctl.  These are guest kernel physical addresses that are later pinned in arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() as part of handling the ioctl.


+            __u32 flags;
+            __u32 noi;    /* Number of interrupts */
+            __u8 isc;    /* Guest interrupt subclass */
+            __u8 sbo;    /* Offset of guest summary bit vector */
+            __u16 pad;
+        } reg_aen;
+        __u64 reserved[8];
+    } u;
+  };
+
+The type of operation is specified in the "op" field.
+KVM_S390_ZPCIOP_REG_AEN is used to register the VM for adapter event
+notification interpretation, which will allow firmware delivery of adapter
+events directly to the vm, with KVM providing a backup delivery mechanism;
+KVM_S390_ZPCIOP_DEREG_AEN is used to subsequently disable interpretation of
+adapter event notifications.
+
+The target zPCI function must also be specified via the "fh" field. For the
+KVM_S390_ZPCIOP_REG_AEN operation, additional information to establish firmware
+delivery must be provided via the "reg_aen" struct.
+
+The "reserved" field is meant for future extensions.

Maybe also mention the "pad" fields? And add should these also be initialized to 0 by the calling userspace program?

Sure, I can mention them.  And yes, I agree that userspace should initialize them to 0, I'll update the QEMU series accordingly.

I just spotted the corresponding patch in the QEMU series, and I think it should already be fine there, since you're using "= { ... }" while declaring the variables:

+int s390_pci_kvm_aif_disable(S390PCIBusDevice *pbdev)
+{
+    struct kvm_s390_zpci_op args = {
+        .fh = pbdev->fh,
+        .op = KVM_S390_ZPCIOP_DEREG_AEN
+    };

That means unspecified fields will be set to 0 by the compiler already, as far as I know.

 Thomas




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux