| 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: <57e57ba5-62ea-f1ff-0d83-5605d57be92d@redhat.com>
Date: Thu, 15 Jul 2021 12:01:51 +0200
From: David Hildenbrand <david@...hat.com>
To: Cornelia Huck <cohuck@...hat.com>,
Pierre Morel <pmorel@...ux.ibm.com>, kvm@...r.kernel.org
Cc: linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
borntraeger@...ibm.com, frankja@...ux.ibm.com, thuth@...hat.com,
imbrenda@...ux.ibm.com, hca@...ux.ibm.com, gor@...ux.ibm.com
Subject: Re: [PATCH v1 1/2] s390x: KVM: accept STSI for CPU topology
information
On 15.07.21 11:30, Cornelia Huck wrote:
> On Thu, Jul 15 2021, David Hildenbrand <david@...hat.com> wrote:
>
>> On 14.07.21 17:25, Pierre Morel wrote:
>>> STSI(15.1.x) gives information on the CPU configuration topology.
>>> Let's accept the interception of STSI with the function code 15 and
>>> let the userland part of the hypervisor handle it.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@...ux.ibm.com>
>>> ---
>>> arch/s390/kvm/priv.c | 11 ++++++++++-
>>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
>>> index 9928f785c677..4ab5f8b7780e 100644
>>> --- a/arch/s390/kvm/priv.c
>>> +++ b/arch/s390/kvm/priv.c
>>> @@ -856,7 +856,7 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
>>> if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
>>> return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
>>>
>>> - if (fc > 3) {
>>> + if (fc > 3 && fc != 15) {
>>> kvm_s390_set_psw_cc(vcpu, 3);
>>> return 0;
>>> }
>>> @@ -893,6 +893,15 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
>>> goto out_no_data;
>>> handle_stsi_3_2_2(vcpu, (void *) mem);
>>> break;
>>> + case 15:
>>> + if (sel1 != 1 || sel2 < 2 || sel2 > 6)
>>> + goto out_no_data;
>>> + if (vcpu->kvm->arch.user_stsi) {
>>> + insert_stsi_usr_data(vcpu, operand2, ar, fc, sel1, sel2);
>>> + return -EREMOTE;
>
> This bypasses the trace event further down.
>
>>> + }
>>> + kvm_s390_set_psw_cc(vcpu, 3);
>>> + return 0;
>>> }
>>> if (kvm_s390_pv_cpu_is_protected(vcpu)) {
>>> memcpy((void *)sida_origin(vcpu->arch.sie_block), (void *)mem,
>>>
>>
>> 1. Setting GPRS to 0
>>
>> I was wondering why we have the "vcpu->run->s.regs.gprs[0] = 0;"
>> for existing fc 1,2,3 in case we set cc=0.
>>
>> Looking at the doc, all I find is:
>>
>> "CC 0: Requested configuration-level number placed in
>> general register 0 or requested SYSIB informa-
>> tion stored"
>>
>> But I don't find where it states that we are supposed to set
>> general register 0 to 0. Wouldn't we also have to do it for
>> fc=15 or for none?
>>
>> If fc 1,2,3 and 15 are to be handled equally, I suggest the following:
>>
>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
>> index 9928f785c677..6eb86fa58b0b 100644
>> --- a/arch/s390/kvm/priv.c
>> +++ b/arch/s390/kvm/priv.c
>> @@ -893,17 +893,23 @@ static int handle_stsi(struct kvm_vcpu *vcpu)
>> goto out_no_data;
>> handle_stsi_3_2_2(vcpu, (void *) mem);
>> break;
>> + case 15:
>> + if (sel1 != 1 || sel2 < 2 || sel2 > 6)
>> + goto out_no_data;
>> + break;
>> }
>> - if (kvm_s390_pv_cpu_is_protected(vcpu)) {
>> - memcpy((void *)sida_origin(vcpu->arch.sie_block), (void *)mem,
>> - PAGE_SIZE);
>> - rc = 0;
>> - } else {
>> - rc = write_guest(vcpu, operand2, ar, (void *)mem, PAGE_SIZE);
>> - }
>> - if (rc) {
>> - rc = kvm_s390_inject_prog_cond(vcpu, rc);
>> - goto out;
>> + if (mem) {
>> + if (kvm_s390_pv_cpu_is_protected(vcpu)) {
>> + memcpy((void *)sida_origin(vcpu->arch.sie_block),
>> + (void *)mem, PAGE_SIZE);
>> + } else {
>> + rc = write_guest(vcpu, operand2, ar, (void *)mem,
>> + PAGE_SIZE);
>> + if (rc) {
>> + rc = kvm_s390_inject_prog_cond(vcpu, rc);
>> + goto out;
>> + }
>> + }
>> }
>> if (vcpu->kvm->arch.user_stsi) {
>> insert_stsi_usr_data(vcpu, operand2, ar, fc, sel1, sel2);
>
> Something like that sounds good, the code is getting a bit convoluted.
>
>>
>>
>> 2. maximum-MNest facility
>>
>> "
>> 1. If the maximum-MNest facility is installed and
>> selector 2 exceeds the nonzero model-depen-
>> dent maximum-selector-2 value."
>>
>> 2. If the maximum-MNest facility is not installed and
>> selector 2 is not specified as two.
>> "
>>
>> We will we be handling the presence/absence of the maximum-MNest facility
>> (for our guest?) in QEMU, corect?
>>
>> I do wonder if we should just let any fc=15 go to user space let the whole
>> sel1 / sel2 checking be handled there. I don't think it's a fast path after all.
>> But no strong opinion.
>
> If that makes handling easier, I think it would be a good idea.
>
>>
>> How do we identify availability of maximum-MNest facility?
>>
>>
>> 3. User space awareness
>>
>> How can user space identify that we actually forward these intercepts?
>> How can it enable them? The old KVM_CAP_S390_USER_STSI capability
>> is not sufficient.
>
> Why do you think that it is not sufficient? USER_STSI basically says
> "you may get an exit that tells you about a buffer to fill in some more
> data for a stsi command, and we also tell you which call". If userspace
> does not know what to add for a certain call, it is free to just do
> nothing, and if it does not get some calls it would support, that should
> not be a problem, either?
If you migrate your VM from machine a to machine b, from kernel a to
kernel b, and kernel b does not trigger exits to user space for fc=15,
how could QEMU spot and catch the different capabilities to make sure
the guest can continue using the feature?
--
Thanks,
David / dhildenb
Powered by blists - more mailing lists