On 26.02.20 11:01, Cornelia Huck wrote:
> On Tue, 25 Feb 2020 16:48:22 -0500
> Christian Borntraeger <borntraeger@xxxxxxxxxx> wrote:
>
>> From: Janosch Frank <frankja@xxxxxxxxxxxxx>
>>
>> This contains 3 main changes:
>> 1. changes in SIE control block handling for secure guests
>> 2. helper functions for create/destroy/unpack secure guests
>> 3. KVM_S390_PV_COMMAND ioctl to allow userspace dealing with secure
>> machines
>>
>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
>> [borntraeger@xxxxxxxxxx: patch merging, splitting, fixing]
>> Signed-off-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>
>> ---
>> arch/s390/include/asm/kvm_host.h | 24 ++-
>> arch/s390/include/asm/uv.h | 69 ++++++++
>> arch/s390/kvm/Makefile | 2 +-
>> arch/s390/kvm/kvm-s390.c | 209 +++++++++++++++++++++++-
>> arch/s390/kvm/kvm-s390.h | 33 ++++
>> arch/s390/kvm/pv.c | 269 +++++++++++++++++++++++++++++++
>> include/uapi/linux/kvm.h | 31 ++++
>> 7 files changed, 633 insertions(+), 4 deletions(-)
>> create mode 100644 arch/s390/kvm/pv.c
>
> (...)
>
>> @@ -2165,6 +2168,160 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
>> return r;
>> }
>>
>> +static int kvm_s390_cpus_from_pv(struct kvm *kvm, u16 *rcp, u16 *rrcp)
>> +{
>> + struct kvm_vcpu *vcpu;
>> + u16 rc, rrc;
>> + int ret = 0;
>> + int i;
>> +
>> + /*
>> + * We ignore failures and try to destroy as many CPUs as possible.
>
> What is this 'destroying'? Is that really the right terminology? From a
> quick glance, I would expect something more in the vein of cpu
> unplugging, and I don't think that's what is happening here.
It is destroying the secure CPU at the ultravisor (its even the name of
the ultravisor call) so I think destroy fits nicely.
>
> (I have obviously not yet read the whole thing, please give people some
> more time to review this huge patch.)
>
>> + * At the same time we must not free the assigned resources when
>> + * this fails, as the ultravisor has still access to that memory.
>> + * So kvm_s390_pv_destroy_cpu can leave a "wanted" memory leak
>> + * behind.
>> + * We want to return the first failure rc and rrc, though.
>> + */
>> + kvm_for_each_vcpu(i, vcpu, kvm) {
>> + mutex_lock(&vcpu->mutex);
>> + if (kvm_s390_pv_destroy_cpu(vcpu, &rc, &rrc) && !ret) {
>> + *rcp = rc;
>> + *rrcp = rrc;
>> + ret = -EIO;
>> + }
>> + mutex_unlock(&vcpu->mutex);
>> + }
>> + return ret;
>> +}
>> +
>> +static int kvm_s390_cpus_to_pv(struct kvm *kvm, u16 *rc, u16 *rrc)
>> +{
>> + int i, r = 0;
>> + u16 dummy;
>> +
>> + struct kvm_vcpu *vcpu;
>> +
>> + kvm_for_each_vcpu(i, vcpu, kvm) {
>> + mutex_lock(&vcpu->mutex);
>> + r = kvm_s390_pv_create_cpu(vcpu, rc, rrc);
>> + mutex_unlock(&vcpu->mutex);
>> + if (r)
>> + break;
>> + }
>> + if (r)
>> + kvm_s390_cpus_from_pv(kvm, &dummy, &dummy);
>
> This is a rather unlikely case, so we don't need to optimize this,
> right?
Right. All the error cases here are "should not happen"
>
> Would rc/rrc from the rollback contain anything of interest if the
> create fails (that is, anything more interesting than what that
> function returns?
I think all rc/rrc from rollback would depend on the first error,
so this is what we return to userspace.
Good thing is that we log all those in the debug feature so if anyone
wants to see the full things its there in /sys/kernel/debug/s390dbf/kvm-uv/sprintf
>
> Similar comment for the 'create' as for the 'destroy' above. (Not
> trying to nitpick, just a bit confused.)
>
> Or is that not the cpu that is created/destroyed, but something else?
> Sorry, just trying to understand where this is coming from.
Its the CPU instance in the ultravisor.
>
>> + return r;
>> +}
>
> (...)
>
> Will look at the remainder of the patch later, maybe I understand the
> stuff above better after that.
>
