diff mbox series

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

Message ID 1558452877-27822-5-git-send-email-pmorel@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series vfio: ap: AP Queue Interrupt Control | expand

Commit Message

Pierre Morel May 21, 2019, 3:34 p.m. UTC 
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@linux.ibm.com>
Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 arch/s390/tools/gen_facilities.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Harald Freudenberger June 12, 2019, 1:57 p.m. UTC | #1 
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@linux.ibm.com>
> Reviewed-by: Tony Krowiak <akrowiak@linux.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 */
>  		}
acked-by: Harald Freudenberger <freude@linux.ibm.com>
Christian Borntraeger June 25, 2019, 8:13 p.m. UTC | #2 
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@linux.ibm.com>
> Reviewed-by: Tony Krowiak <akrowiak@linux.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)

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;
Christian Borntraeger June 25, 2019, 8:15 p.m. UTC | #3 
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@linux.ibm.com>
>> Reviewed-by: Tony Krowiak <akrowiak@linux.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)
> 
> 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).

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);
 }
Anthony Krowiak June 26, 2019, 9:12 p.m. UTC | #4 
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@linux.ibm.com>
>>> Reviewed-by: Tony Krowiak <akrowiak@linux.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. 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. 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))?




>>
>> 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);
>   }
>   
>
Christian Borntraeger June 27, 2019, 6:54 a.m. UTC | #5 
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@linux.ibm.com>
>>>> Reviewed-by: Tony Krowiak <akrowiak@linux.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);
>>   }
>>  
>
Halil Pasic June 27, 2019, 12:04 p.m. UTC | #6 
On Tue, 25 Jun 2019 22:13:12 +0200
Christian Borntraeger <borntraeger@de.ibm.com> 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@linux.ibm.com>
> > Reviewed-by: Tony Krowiak <akrowiak@linux.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)
> 
> 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;

I will go with this option because it is more readable (easier to find)
IMHO. Will also add a chech for host sltfle.65. So I end up with:

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 28ebd647784c..1c4113f0f2a8 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 && test_facility(65))
+               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;
 
diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c
index d52f537b7169..cead9e0dcffb 100644
--- a/arch/s390/tools/gen_facilities.c
+++ b/arch/s390/tools/gen_facilities.c
@@ -111,7 +111,6 @@ 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 */
diff mbox series

Patch

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 */
 		}