Date:   Tue, 15 May 2018 16:55:32 +0200
From:   Pierre Morel <pmorel@...ux.ibm.com>
To:     Tony Krowiak <akrowiak@...ux.vnet.ibm.com>,
        linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Cc:     freude@...ibm.com, schwidefsky@...ibm.com,
        heiko.carstens@...ibm.com, borntraeger@...ibm.com,
        cohuck@...hat.com, kwankhede@...dia.com,
        bjsdjshi@...ux.vnet.ibm.com, pbonzini@...hat.com,
        alex.williamson@...hat.com, pmorel@...ux.vnet.ibm.com,
        alifm@...ux.vnet.ibm.com, mjrosato@...ux.vnet.ibm.com,
        jjherne@...ux.vnet.ibm.com, thuth@...hat.com,
        pasic@...ux.vnet.ibm.com, berrange@...hat.com,
        fiuczy@...ux.vnet.ibm.com, buendgen@...ibm.com
Subject: Re: [PATCH v5 06/13] KVM: s390: interfaces to manage guest's AP
 matrix

On 07/05/2018 17:11, Tony Krowiak wrote:
> Provides interfaces to manage the AP adapters, usage domains
> and control domains assigned to a KVM guest.
>
> The guest's SIE state description has a satellite structure called the
> Crypto Control Block (CRYCB) containing three bitmask fields
> identifying the adapters, queues (domains) and control domains
> assigned to the KVM guest:
>
> * The AP Adapter Mask (APM) field identifies the AP adapters assigned to
>    the KVM guest
>
> * The AP Queue Mask (AQM) field identifies the AP queues assigned to
>    the KVM guest. Each AP queue is connected to a usage domain within
>    an AP adapter.
>
> * The AP Domain Mask (ADM) field identifies the control domains
>    assigned to the KVM guest.
>
> Each adapter, queue (usage domain) and control domain are identified by
> a number from 0 to 255. The bits in each mask, from most significant to
> least significant bit, correspond to the numbers 0-255. When a bit is
> set, the corresponding adapter, queue (usage domain) or control domain
> is assigned to the KVM guest.
>
> This patch will set the bits in the APM, AQM and ADM fields of the
> CRYCB referenced by the KVM guest's SIE state description. The process
> used is:
>
> 1. Verify that the bits to be set do not exceed the maximum bit
>     number for the given mask.
>
> 2. Verify that the APQNs that can be derived from the cross product
>     of the bits set in the APM and AQM fields of the KVM guest's CRYCB
>     are not assigned to any other KVM guest running on the same linux
>     host.
>
> 3. Set the APM, AQM and ADM in the CRYCB according to the matrix
>     configured for the mediated matrix device via its sysfs
>     assign_adapter, assign_domain and assign_control domain attribute
>     files respectively.
>
> Signed-off-by: Tony Krowiak <akrowiak@...ux.vnet.ibm.com>
> ---
>   arch/s390/include/asm/kvm-ap.h   |   52 ++++++++++++
>   arch/s390/include/asm/kvm_host.h |    1 +
>   arch/s390/kvm/kvm-ap.c           |  161 ++++++++++++++++++++++++++++++++++++++
>   3 files changed, 214 insertions(+), 0 deletions(-)
>
> diff --git a/arch/s390/include/asm/kvm-ap.h b/arch/s390/include/asm/kvm-ap.h
> index 6af1ff8..21fe9f2 100644
> --- a/arch/s390/include/asm/kvm-ap.h
> +++ b/arch/s390/include/asm/kvm-ap.h
> @@ -12,8 +12,33 @@
>
>   #include <linux/types.h>
>   #include <linux/kvm_host.h>
> +#include <linux/bitops.h>
>   #include <asm/ap.h>
>
> +#define KVM_AP_MASK_BYTES(n)	DIV_ROUND_UP(n, BITS_PER_BYTE)
> +
> +/**
> + * The AP matrix is comprised of three bit masks identifying the adapters,
> + * queues (domains) and control domains that belong to an AP matrix. The bits in
> + * each mask, from least significant to most significant bit, correspond to IDs
> + * 0 to 255. When a bit is set, the corresponding ID belongs to the matrix.
> + *
> + * @apm identifies the AP adapters in the matrix
> + * @apm_max: max adapter number in @apm
> + * @aqm identifies the AP queues (domains) in the matrix
> + * @aqm_max: max domain number in @aqm
> + * @adm identifies the AP control domains in the matrix
> + * @adm_max: max domain number in @adm
> + */
> +struct kvm_ap_matrix {
> +	unsigned long apm_max;
> +	DECLARE_BITMAP(apm, 256);
> +	unsigned long aqm_max;
> +	DECLARE_BITMAP(aqm, 256);
> +	unsigned long adm_max;
> +	DECLARE_BITMAP(adm, 256);

Just a possible performance impact:
you may have interest to put all bitmaps first to take adventage
of quadword handling (If bitmaps use it) and put unsigned longs
at the end.

> +};
> +
>   /**
>    * kvm_ap_apxa_installed
>    *
> @@ -57,4 +82,31 @@
>    */
>   bool kvm_ap_instructions_available(void);
>
> +/**
> + * kvm_ap_configure_matrix
> + *
> + * Configure the AP matrix for a KVM guest.
> + *
> + * @kvm:		 the KVM guest
> + * @matrix:		 the matrix configuration information
> + *
> + * Returns 0 if:
> + *	1. The AP instructions are installed on the guest
> + *	2. The APQNs derived from the intersection of the set of adapter
> + *	   IDs (APM) and queue indexes (AQM) in @matrix are not configured for
> + *	   any other KVM guest running on the same linux host.
> + * Otherwise returns an error code.
> + */
> +int kvm_ap_configure_matrix(struct kvm *kvm, struct kvm_ap_matrix *matrix);
> +
> +/**
> + * kvm_ap_deconfigure_matrix
> + *
> + * Deconfigure the AP matrix for a KVM guest. Clears all of the bits in the
> + * APM, AQM and ADM in the guest's CRYCB.
> + *
> + * @kvm: the KVM guest
> + */
> +void kvm_ap_deconfigure_matrix(struct kvm *kvm);
> +
>   #endif /* _ASM_KVM_AP */
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index ef4b237..8736cde 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -257,6 +257,7 @@ struct kvm_s390_sie_block {
>   	__u64	tecmc;			/* 0x00e8 */
>   	__u8	reservedf0[12];		/* 0x00f0 */
>   #define CRYCB_FORMAT_MASK 0x00000003
> +#define CRYCB_FORMAT0 0x00000000
>   #define CRYCB_FORMAT1 0x00000001
>   #define CRYCB_FORMAT2 0x00000003
>   	__u32	crycbd;			/* 0x00fc */
> diff --git a/arch/s390/kvm/kvm-ap.c b/arch/s390/kvm/kvm-ap.c
> index 00bcfb0..98b53c7 100644
> --- a/arch/s390/kvm/kvm-ap.c
> +++ b/arch/s390/kvm/kvm-ap.c
> @@ -7,6 +7,7 @@
>    * Author(s): Tony Krowiak <akrowia@...ux.vnet.ibm.com>
>    */
>   #include <linux/kernel.h>
> +#include <linux/bitops.h>
>   #include <asm/kvm-ap.h>
>
>   #include "kvm-s390.h"
> @@ -81,3 +82,163 @@ int kvm_ap_apxa_installed(void)
>   	return 0;
>   }
>   EXPORT_SYMBOL(kvm_ap_apxa_installed);
> +
> +static inline void kvm_ap_clear_crycb_masks(struct kvm *kvm)
> +{
> +	memset(&kvm->arch.crypto.crycb->apcb0, 0,
> +	       sizeof(kvm->arch.crypto.crycb->apcb0));

Here you prefer to set both structure to 0 instead of testing which
structure to erase.

> +	memset(&kvm->arch.crypto.crycb->apcb1, 0,
> +	       sizeof(kvm->arch.crypto.crycb->apcb1));
> +}
> +

...snip...

> +/**
> + * kvm_ap_validate_queue_sharing
> + *
> + * Verifies that the APQNs derived from the cross product of the AP adapter IDs
> + * and AP queue indexes comprising the AP matrix are not configured for
> + * another guest. AP queue sharing is not allowed.
> + *
> + * @kvm: the KVM guest
> + * @matrix: the AP matrix
> + *
> + * Returns 0 if the APQNs are valid, otherwise; returns -EBUSY.
> + */
> +static int kvm_ap_validate_queue_sharing(struct kvm *kvm,
> +					 struct kvm_ap_matrix *matrix)
> +{
> +	struct kvm *vm;
> +	unsigned long *apm, *aqm;
> +	unsigned long apid, apqi;
> +
> +
> +	/* No other VM may share an AP Queue with the input VM */
> +	list_for_each_entry(vm, &vm_list, vm_list) {
> +		if (kvm == vm)
> +			continue;
> +
> +		apm = kvm_ap_get_crycb_apm(vm);
> +		if (!bitmap_and(apm, apm, matrix->apm, matrix->apm_max + 1))
> +			continue;
> +
> +		aqm = kvm_ap_get_crycb_aqm(vm);
> +		if (!bitmap_and(aqm, aqm, matrix->aqm, matrix->aqm_max + 1))
> +			continue;
> +
> +		for_each_set_bit_inv(apid, apm, matrix->apm_max + 1)
> +			for_each_set_bit_inv(apqi, aqm, matrix->aqm_max + 1)
> +				kvm_ap_log_sharing_err(vm, apid, apqi);
> +
> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}

This function (ap_validate_queue_sharing) only verifies that VM don't 
share queues.
What about the queues used by a host application?

I understand that you want to implement  these checks within KVM but this is
related to which queue devices are bound to the matrix and which one are 
not.

I think that this should be related somehow to the bounded queue devices and
therefor implemented inside the matrix driver.

Regards,

Pierre

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

Powered by blists - more mailing lists