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

[Christian Borntraeger]

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@xxxxxxxxxxxxx>
>>>> Reviewed-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
>>>> ---
>>>> Â 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);
>> Â }
>> Â
>