On Mon, 11 Nov 2019 17:39:15 +0100
Janosch Frank <frankja@xxxxxxxxxxxxx> wrote:
> On 11/11/19 5:25 PM, Cornelia Huck wrote:
> > On Fri, 8 Nov 2019 08:36:35 +0100
> > Janosch Frank <frankja@xxxxxxxxxxxxx> wrote:
> >
> >> On 11/7/19 5:29 PM, Cornelia Huck wrote:
> >>> On Thu, 24 Oct 2019 07:40:26 -0400
> >>> Janosch Frank <frankja@xxxxxxxxxxxxx> wrote:
> >
> >>>> @@ -2157,6 +2164,96 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
> >>>> return r;
> >>>> }
> >>>>
> >>>> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
> >>>> +static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
> >>>> +{
> >>>> + int r = 0;
> >>>> + void __user *argp = (void __user *)cmd->data;
> >>>> +
> >>>> + switch (cmd->cmd) {
> >>>> + case KVM_PV_VM_CREATE: {
> >>>> + r = kvm_s390_pv_alloc_vm(kvm);
> >>>> + if (r)
> >>>> + break;
> >>>> +
> >>>> + mutex_lock(&kvm->lock);
> >>>> + kvm_s390_vcpu_block_all(kvm);
> >>>> + /* FMT 4 SIE needs esca */
> >>>> + r = sca_switch_to_extended(kvm);
> >
> > Looking at this again: this function calls kvm_s390_vcpu_block_all()
> > (which probably does not hurt), but then kvm_s390_vcpu_unblock_all()...
> > don't we want to keep the block across pv_create_vm() as well?
>
> Yeah
>
> >
> > Also, can you maybe skip calling this function if we use the esca
> > already?
>
> Did I forget to include that in the patchset?
> I extended sca_switch_to_extended() to just return in that case.
If you did, I likely missed it; way too much stuff to review :)
>
> >
> >>>> + if (!r)
> >>>> + r = kvm_s390_pv_create_vm(kvm);
> >>>> + kvm_s390_vcpu_unblock_all(kvm);
> >>>> + mutex_unlock(&kvm->lock);
> >>>> + break;
> >>>> + }
(...)
> >>>> @@ -2555,8 +2671,13 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
> >>>> {
> >>>> kvm_free_vcpus(kvm);
> >>>> sca_dispose(kvm);
> >>>> - debug_unregister(kvm->arch.dbf);
> >>>> kvm_s390_gisa_destroy(kvm);
> >>>> + if (IS_ENABLED(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST) &&
> >>>> + kvm_s390_pv_is_protected(kvm)) {
> >>>> + kvm_s390_pv_destroy_vm(kvm);
> >>>> + kvm_s390_pv_dealloc_vm(kvm);
> >>>
> >>> It seems the pv vm can be either destroyed via the ioctl above or in
> >>> the course of normal vm destruction. When is which way supposed to be
> >>> used? Also, it seems kvm_s390_pv_destroy_vm() can fail -- can that be a
> >>> problem in this code path?
> >>
> >> On a reboot we need to tear down the protected VM and boot from
> >> unprotected mode again. If the VM shuts down we go through this cleanup
> >> path. If it fails the kernel will loose the memory that was allocated to
> >> start the VM.
> >
> > Shouldn't you at least log a moan in that case? Hopefully, this happens
> > very rarely, but the dbf will be gone...
>
> That's why I created the uv dbf :-)
Again, way too easy to get lost in these changes :)
> Well, it shouldn't happen at all so maybe a WARN will be a good option
Yeah, if it this is one of these "should not happen" things, a WARN
sounds good.
>
> >
> >>
> >>>
> >>>> + }
> >>>> + debug_unregister(kvm->arch.dbf);
> >>>> free_page((unsigned long)kvm->arch.sie_page2);
> >>>> if (!kvm_is_ucontrol(kvm))
> >>>> gmap_remove(kvm->arch.gmap);
Attachment:
pgppcUWliRoCf.pgp
Description: OpenPGP digital signature
