linux-kernel - Re: [PATCH v9 4/4] s390: ap: kvm: Enable PQAP/AQIC facility for the guest

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <b103b619-9aab-b5f9-0c79-9f4399f69b49@de.ibm.com>
Date:   Thu, 27 Jun 2019 08:54:32 +0200
From:   Christian Borntraeger <borntraeger@...ibm.com>
To:     Tony Krowiak <akrowiak@...ux.ibm.com>,
        Pierre Morel <pmorel@...ux.ibm.com>
Cc:     alex.williamson@...hat.com, cohuck@...hat.com,
        linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
        kvm@...r.kernel.org, frankja@...ux.ibm.com, pasic@...ux.ibm.com,
        david@...hat.com, heiko.carstens@...ibm.com, freude@...ux.ibm.com,
        mimu@...ux.ibm.com
Subject: Re: [PATCH v9 4/4] s390: ap: kvm: Enable PQAP/AQIC facility for the
 guest



On 26.06.19 23:12, Tony Krowiak wrote:
> On 6/25/19 4:15 PM, Christian Borntraeger wrote:
>>
>>
>> On 25.06.19 22:13, Christian Borntraeger wrote:
>>>
>>>
>>> On 21.05.19 17:34, Pierre Morel wrote:
>>>> AP Queue Interruption Control (AQIC) facility gives
>>>> the guest the possibility to control interruption for
>>>> the Cryptographic Adjunct Processor queues.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@...ux.ibm.com>
>>>> Reviewed-by: Tony Krowiak <akrowiak@...ux.ibm.com>
>>>> ---
>>>>   arch/s390/tools/gen_facilities.c | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c
>>>> index 61ce5b5..aed14fc 100644
>>>> --- a/arch/s390/tools/gen_facilities.c
>>>> +++ b/arch/s390/tools/gen_facilities.c
>>>> @@ -114,6 +114,7 @@ static struct facility_def facility_defs[] = {
>>>>           .bits = (int[]){
>>>>               12, /* AP Query Configuration Information */
>>>>               15, /* AP Facilities Test */
>>>> +            65, /* AP Queue Interruption Control */
>>>>               156, /* etoken facility */
>>>>               -1  /* END */
>>>>           }
>>>>
>>>
>>> I think we should only set stfle.65 if we have the aiv facility (Because we do not
>>> have a GISA otherwise)
> 
> My assumption here is that you are taking the line added above
> (STFLE.65) out and replacing with one of the two suggestions
> below.

Yes, I want to replace this hunk.

 I am quite fuzzy on how all of this CPU model stuff works,
> but I am thinking that the above makes STFLE.65 available to be
> set via the CPU model (i.e., aqic=on on the QEMU command line) as
> long as it is supported by the host.

Yes, it makes it available when the host has stfle.65. But at the same
time it does not look if the adapter interruption virtualization facility
is available. For example for vsie the guest2 will enable stfle.65 for its
guests, but we do not support AIV.

 By taking that line out, we
> are relying on one of the suggestions below to make STFLE.65
> available to the guest only if AIV facility is available. Does that
> sound about right?
> 
> If that is the case, then wouldn't we also have to add a check to make
> sure that STFLE.65 is available on the host (i.e., test_facility(65))?

I think AIV in level n is enough to provide STFLE.65 in level n+1. 
On the other hand also checking for stfle.65 does not hurt.


> 
> 
>>>
>>> So something like this instead?
>>>
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index 28ebd64..1501cd6 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -2461,6 +2461,9 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>>>                  set_kvm_facility(kvm->arch.model.fac_list, 147);
>>>          }
>>>   +       if (css_general_characteristics.aiv)
>>> +               set_kvm_facility(kvm->arch.model.fac_mask, 65);
>>> +
>>>          kvm->arch.model.cpuid = kvm_s390_get_initial_cpuid();
>>>          kvm->arch.model.ibc = sclp.ibc & 0x0fff;
>>>  
>>
>> Maybe even just piggyback on gisa init (it will bail out early).
> 
> It could also go in the kvm_s390_crypto_init() function since it
> is related to crypto.
> 
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index 9dde4d7..9182a04 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -3100,6 +3100,7 @@ void kvm_s390_gisa_init(struct kvm *kvm)
>>          gi->timer.function = gisa_vcpu_kicker;
>>          memset(gi->origin, 0, sizeof(struct kvm_s390_gisa));
>>          gi->origin->next_alert = (u32)(u64)gi->origin;
>> +       set_kvm_facility(kvm->arch.model.fac_mask, 65);
>>          VM_EVENT(kvm, 3, "gisa 0x%pK initialized", gi->origin);
>>   }
>>  
>