| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <79a49a19-e42d-d53c-27a2-2e96a20156d4@linux.ibm.com>
Date: Wed, 9 Feb 2022 10:12:25 +0100
From: Janosch Frank <frankja@...ux.ibm.com>
To: Michael Mueller <mimu@...ux.ibm.com>, kvm@...r.kernel.org
Cc: cohuck@...hat.com, borntraeger@...ibm.com, thuth@...hat.com,
pasic@...ux.ibm.com, david@...hat.com, linux-s390@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] KVM: s390: pv: make use of ultravisor AIV support
On 2/8/22 17:53, Michael Mueller wrote:
> This patch enables the ultravisor adapter interruption vitualization
> support indicated by UV feature BIT_UV_FEAT_AIV. This allows ISC
> interruption injection directly into the GISA IPM for PV kvm guests.
>
> Hardware that does not support this feature will continue to use the
> UV interruption interception method to deliver ISC interruptions to
> PV kvm guests. For this purpose, the ECA_AIV bit for all guest cpus
> will be cleared and the GISA will be disabled during PV CPU setup.
>
> In addition a check in __inject_io() has been removed. That reduces the
> required instructions for interruption handling for PV and traditional
> kvm guests.
>
> Signed-off-by: Michael Mueller <mimu@...ux.ibm.com>
> ---
> arch/s390/include/asm/uv.h | 1 +
> arch/s390/kvm/interrupt.c | 53 +++++++++++++++++++++++++++++++++-----
> arch/s390/kvm/kvm-s390.c | 12 ++++++---
> arch/s390/kvm/kvm-s390.h | 11 ++++++++
> 4 files changed, 68 insertions(+), 9 deletions(-)
>
> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
> index 86218382d29c..a2d376b8bce3 100644
> --- a/arch/s390/include/asm/uv.h
> +++ b/arch/s390/include/asm/uv.h
> @@ -80,6 +80,7 @@ enum uv_cmds_inst {
>
> enum uv_feat_ind {
> BIT_UV_FEAT_MISC = 0,
> + BIT_UV_FEAT_AIV = 1,
> };
>
> struct uv_cb_header {
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index db933c252dbc..d45e26e31d3d 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -1901,13 +1901,12 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
> isc = int_word_to_isc(inti->io.io_int_word);
>
> /*
> - * Do not make use of gisa in protected mode. We do not use the lock
> - * checking variant as this is just a performance optimization and we
> - * do not hold the lock here. This is ok as the code will pick
> - * interrupts from both "lists" for delivery.
> + * We do not use the lock checking variant as this is just a
> + * performance optimization and we do not hold the lock here.
> + * This is ok as the code will pick interrupts from both "lists"
> + * for delivery.
> */
> - if (!kvm_s390_pv_get_handle(kvm) &&
> - gi->origin && inti->type & KVM_S390_INT_IO_AI_MASK) {
> + if (gi->origin && inti->type & KVM_S390_INT_IO_AI_MASK) {
> VM_EVENT(kvm, 4, "%s isc %1u", "inject: I/O (AI/gisa)", isc);
> gisa_set_ipm_gisc(gi->origin, isc);
> kfree(inti);
> @@ -3171,9 +3170,32 @@ void kvm_s390_gisa_init(struct kvm *kvm)
> VM_EVENT(kvm, 3, "gisa 0x%pK initialized", gi->origin);
> }
>
> +void kvm_s390_gisa_enable(struct kvm *kvm)
> +{
> + struct kvm_s390_gisa_interrupt *gi = &kvm->arch.gisa_int;
> + struct kvm_vcpu *vcpu;
> + unsigned long i;
> +
> + if (gi->origin)
> + return;
> + kvm_s390_gisa_init(kvm);
> + kvm_for_each_vcpu(i, vcpu, kvm) {
> + mutex_lock(&vcpu->mutex);
> + vcpu->arch.sie_block->gd = kvm_s390_get_gisa_desc(kvm);
The gisa is a per VM structure so this value could be cached outside the
loop. Also I'd expect that we would need to do the != 0 check only once, no?
> + if (vcpu->arch.sie_block->gd) {
> + vcpu->arch.sie_block->eca |= ECA_AIV;
> + VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u",
> + vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id);
> + }
> + mutex_unlock(&vcpu->mutex);
> + }
> +}
> +
> +
One \n too much
> void kvm_s390_gisa_destroy(struct kvm *kvm)
> {
> struct kvm_s390_gisa_interrupt *gi = &kvm->arch.gisa_int;
> + struct kvm_s390_gisa *gisa = gi->origin;
>
> if (!gi->origin)
> return;
> @@ -3184,6 +3206,25 @@ void kvm_s390_gisa_destroy(struct kvm *kvm)
> cpu_relax();
> hrtimer_cancel(&gi->timer);
> gi->origin = NULL;
> + VM_EVENT(kvm, 3, "gisa 0x%pK destroyed", gisa);
> +}
> +
> +void kvm_s390_gisa_disable(struct kvm *kvm)
> +{
> + struct kvm_s390_gisa_interrupt *gi = &kvm->arch.gisa_int;
> + struct kvm_vcpu *vcpu;
> + unsigned long i;
> +
> + if (!gi->origin)
> + return;
> + kvm_for_each_vcpu(i, vcpu, kvm) {
> + mutex_lock(&vcpu->mutex);
> + vcpu->arch.sie_block->eca &= ~ECA_AIV;
> + vcpu->arch.sie_block->gd = 0U;
> + mutex_unlock(&vcpu->mutex);
> + VCPU_EVENT(vcpu, 3, "AIV disabled for cpu %03u", vcpu->vcpu_id);
> + }
> + kvm_s390_gisa_destroy(kvm);
> }
>
> /**
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 577f1ead6a51..72a3c9a7c9dd 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2194,6 +2194,9 @@ static int kvm_s390_cpus_from_pv(struct kvm *kvm, u16 *rcp, u16 *rrcp)
> }
> mutex_unlock(&vcpu->mutex);
> }
> + /* Finally enable the GISA if required for traditional KVM guests */
Ensure that we re-enable gisa if the non-PV guest used it but the PV
guest did not.
> + if (use_gisa)
> + kvm_s390_gisa_enable(kvm);
> return ret;
> }
>
> @@ -2205,6 +2208,10 @@ static int kvm_s390_cpus_to_pv(struct kvm *kvm, u16 *rc, u16 *rrc)
>
> struct kvm_vcpu *vcpu;
>
> + /* First disable the GISA if the ultravisor does not support AIV. */
I think we should drop the "First" here.
> + if (!test_bit_inv(BIT_UV_FEAT_AIV, &uv_info.uv_feature_indications))
> + kvm_s390_gisa_disable(kvm);
> +
> kvm_for_each_vcpu(i, vcpu, kvm) {
> mutex_lock(&vcpu->mutex);
> r = kvm_s390_pv_create_cpu(vcpu, rc, rrc);
> @@ -2268,6 +2275,7 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
> */
> if (r)
> break;
> +
Stray whitespace change
> r = kvm_s390_pv_deinit_vm(kvm, &cmd->rc, &cmd->rrc);
>
> /* no need to block service interrupts any more */
> @@ -3263,9 +3271,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>
> vcpu->arch.sie_block->icpua = vcpu->vcpu_id;
> spin_lock_init(&vcpu->arch.local_int.lock);
> - vcpu->arch.sie_block->gd = (u32)(u64)vcpu->kvm->arch.gisa_int.origin;
> - if (vcpu->arch.sie_block->gd && sclp.has_gisaf)
> - vcpu->arch.sie_block->gd |= GISA_FORMAT1;
> + vcpu->arch.sie_block->gd = kvm_s390_get_gisa_desc(vcpu->kvm);
I like that way of handling the gisa setup of VCPUs
> seqcount_init(&vcpu->arch.cputm_seqcount);
>
> vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index 098831e815e6..4ba8fc30d87a 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -231,6 +231,15 @@ static inline unsigned long kvm_s390_get_gfn_end(struct kvm_memslots *slots)
> return ms->base_gfn + ms->npages;
> }
>
> +static inline u32 kvm_s390_get_gisa_desc(struct kvm *kvm)
> +{
> + u32 gd = (u32)(u64)kvm->arch.gisa_int.origin;
> +
> + if (gd && sclp.has_gisaf)
> + gd |= GISA_FORMAT1;
> + return gd;
> +}
> +
> /* implemented in pv.c */
> int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc);
> int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc);
> @@ -450,6 +459,8 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu,
> void kvm_s390_gisa_init(struct kvm *kvm);
> void kvm_s390_gisa_clear(struct kvm *kvm);
> void kvm_s390_gisa_destroy(struct kvm *kvm);
> +void kvm_s390_gisa_disable(struct kvm *kvm);
> +void kvm_s390_gisa_enable(struct kvm *kvm);
> int kvm_s390_gib_init(u8 nisc);
> void kvm_s390_gib_destroy(void);
>
Powered by blists - more mailing lists