diff mbox

[PATCH/s390-next,2/3] s390x/cpumodel: add zpci, aen and ais facilities

Message ID 1499942429-55449-3-git-send-email-borntraeger@de.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Christian Borntraeger July 13, 2017, 10:40 a.m. UTC
From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>

zPCI instructions and facilities are available since IBM zEnterprise
EC12. To support z/PCI in QEMU we enable zpci, aen and ais facilities
starting with zEC12 GA1. And we always set zpci and aen bits in max cpu
model. Later they might be switched off due to applied real cpu model.
For ais bit, we only provide it in the full cpu model beginning with
zEC12 and defer its enablement in the default cpu model to a later point
in time. At the same time, disable them for 2.9 and older machines.

Because of introducing AIS facility, we could check if it's enabled to
initialize flic->ais_supported with the real value.

Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/intc/s390_flic.c             | 3 ++-
 hw/intc/s390_flic_kvm.c         | 3 ---
 hw/s390x/s390-virtio-ccw.c      | 3 +++
 target/s390x/cpu_features.c     | 3 +++
 target/s390x/cpu_features_def.h | 3 +++
 target/s390x/gen-features.c     | 5 +++++
 target/s390x/kvm.c              | 7 +++++++
 7 files changed, 23 insertions(+), 4 deletions(-)

Comments

Cornelia Huck July 13, 2017, 12:11 p.m. UTC | #1
On Thu, 13 Jul 2017 12:40:28 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> 
> zPCI instructions and facilities are available since IBM zEnterprise
> EC12. To support z/PCI in QEMU we enable zpci, aen and ais facilities
> starting with zEC12 GA1. And we always set zpci and aen bits in max cpu
> model. Later they might be switched off due to applied real cpu model.
> For ais bit, we only provide it in the full cpu model beginning with
> zEC12 and defer its enablement in the default cpu model to a later point
> in time. At the same time, disable them for 2.9 and older machines.
> 
> Because of introducing AIS facility, we could check if it's enabled to
> initialize flic->ais_supported with the real value.
> 
> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/intc/s390_flic.c             | 3 ++-
>  hw/intc/s390_flic_kvm.c         | 3 ---
>  hw/s390x/s390-virtio-ccw.c      | 3 +++
>  target/s390x/cpu_features.c     | 3 +++
>  target/s390x/cpu_features_def.h | 3 +++
>  target/s390x/gen-features.c     | 5 +++++
>  target/s390x/kvm.c              | 7 +++++++
>  7 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> index ff6e4ec..6e7c610 100644
> --- a/hw/intc/s390_flic.c
> +++ b/hw/intc/s390_flic.c
> @@ -163,9 +163,10 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
>      if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
>          error_setg(errp, "flic property adapter_routes_max_batch too big"
>                     " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
> +        return;

Hunk should go into a previous patch?

(And does it really matter?)

>      }
>  
> -    fs->ais_supported = true;
> +    fs->ais_supported = s390_has_feat(S390_FEAT_ADAPTER_INT_SUPPRESSION);
>  }
>  
>  static void s390_flic_class_init(ObjectClass *oc, void *data)

> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 78ebe83..1901153 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -302,6 +302,9 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>          }
>      }
>  
> +    /* Try to enable AIS facility */
> +    kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);

What happens if you fail to enable it? You probably don't want to allow
the feature bit, then?

> +
>      qemu_mutex_init(&qemu_sigp_mutex);
>  
>      return 0;

Let's summarize to make sure that I'm not confused:

- Starting with zEC12 GA1, we provide zpci, aen, ais in the full model
- Starting with zEC12 GA1, we provide zpci and aen in the default model
- In the host model, we add zpci and aen; they might be switched off
  after applying the found model
- Compat for 2.9 and earlier switches off zpci, aen, ais
- We unconditionally enable the kvm part of ais

I'm still not sure what's supposed to happen with new qemu + old kernel
(no ais) + full zEC12 GA1 or later model.
Christian Borntraeger July 13, 2017, 12:29 p.m. UTC | #2
On 07/13/2017 02:11 PM, Cornelia Huck wrote:
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 78ebe83..1901153 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -302,6 +302,9 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>          }
>>      }
>>  
>> +    /* Try to enable AIS facility */
>> +    kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
> 
> What happens if you fail to enable it? You probably don't want to allow
> the feature bit, then?

Then this bit is off. This call will enable it in the kernel, if that fails
the kernel will return this bit as disabled.
> 
>> +
>>      qemu_mutex_init(&qemu_sigp_mutex);
>>  
>>      return 0;
> 
> Let's summarize to make sure that I'm not confused:
> 
> - Starting with zEC12 GA1, we provide zpci, aen, ais in the full model
yes

> - Starting with zEC12 GA1, we provide zpci and aen in the default model
yes. ais has to be enabled manually for z12 and z13.
The alternative is to have ais as part of the default model. This has the big
disadvantage that -cpu zEC12 and -cpu z13 will stop working for all available
distro kernels. I believe that a working -cpu z13 is more important than the
need to manually enable ais.
For the future
 - We can make ais part of the default model for a future system when that happens since
   the features for a new system will require a new host kernel anyway
 - We can also make ais part of the default model for a future machine type (e.g. 2.13)
   when we believe that the world has moved on to a newer kernel


> - In the host model, we add zpci and aen; they might be switched off
>   after applying the found model

We also get ais from the kernel, so the host model will have zpci,ais and
aen

> - Compat for 2.9 and earlier switches off zpci, aen, ais

yes 

> - We unconditionally enable the kvm part of ais

We tell the KVM code in the kernel to enable the facility bit (before the cpu
model might take it way) and if QEMU really uses ais, that the kernel does
the right thing then
> 
> I'm still not sure what's supposed to happen with new qemu + old kernel
> (no ais) + full zEC12 GA1 or later model.

We enable aen and zpci, but disable ais for that guest. In theory a guest
can drive PCI devices without AIS. This is a valid configuration since zpci
does not require ais. 
The fact that Linux requires ais to use PCI is unfortunate but that could 
be "fixed" Linux if necessary.
Christian Borntraeger July 13, 2017, 12:41 p.m. UTC | #3
On 07/13/2017 02:11 PM, Cornelia Huck wrote:
> On Thu, 13 Jul 2017 12:40:28 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>
>> zPCI instructions and facilities are available since IBM zEnterprise
>> EC12. To support z/PCI in QEMU we enable zpci, aen and ais facilities
>> starting with zEC12 GA1. And we always set zpci and aen bits in max cpu
>> model. Later they might be switched off due to applied real cpu model.
>> For ais bit, we only provide it in the full cpu model beginning with
>> zEC12 and defer its enablement in the default cpu model to a later point
>> in time. At the same time, disable them for 2.9 and older machines.
>>
>> Because of introducing AIS facility, we could check if it's enabled to
>> initialize flic->ais_supported with the real value.
>>
>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  hw/intc/s390_flic.c             | 3 ++-
>>  hw/intc/s390_flic_kvm.c         | 3 ---
>>  hw/s390x/s390-virtio-ccw.c      | 3 +++
>>  target/s390x/cpu_features.c     | 3 +++
>>  target/s390x/cpu_features_def.h | 3 +++
>>  target/s390x/gen-features.c     | 5 +++++
>>  target/s390x/kvm.c              | 7 +++++++
>>  7 files changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
>> index ff6e4ec..6e7c610 100644
>> --- a/hw/intc/s390_flic.c
>> +++ b/hw/intc/s390_flic.c
>> @@ -163,9 +163,10 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
>>      if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
>>          error_setg(errp, "flic property adapter_routes_max_batch too big"
>>                     " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
>> +        return;
> 
> Hunk should go into a previous patch?
> 
> (And does it really matter?)

I can
- move to the previous series patch 8 (s390x/flic: introduce modify_ais_mode callback)
- remove the return as it does not matter (this is realize, and realize failures here
are fatal as far as I can tell)
- keep it here since this version was tested and it does not hurt
Cornelia Huck July 13, 2017, 12:57 p.m. UTC | #4
On Thu, 13 Jul 2017 14:41:05 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 07/13/2017 02:11 PM, Cornelia Huck wrote:
> > On Thu, 13 Jul 2017 12:40:28 +0200
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >>
> >> zPCI instructions and facilities are available since IBM zEnterprise
> >> EC12. To support z/PCI in QEMU we enable zpci, aen and ais facilities
> >> starting with zEC12 GA1. And we always set zpci and aen bits in max cpu
> >> model. Later they might be switched off due to applied real cpu model.
> >> For ais bit, we only provide it in the full cpu model beginning with
> >> zEC12 and defer its enablement in the default cpu model to a later point
> >> in time. At the same time, disable them for 2.9 and older machines.
> >>
> >> Because of introducing AIS facility, we could check if it's enabled to
> >> initialize flic->ais_supported with the real value.
> >>
> >> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> ---
> >>  hw/intc/s390_flic.c             | 3 ++-
> >>  hw/intc/s390_flic_kvm.c         | 3 ---
> >>  hw/s390x/s390-virtio-ccw.c      | 3 +++
> >>  target/s390x/cpu_features.c     | 3 +++
> >>  target/s390x/cpu_features_def.h | 3 +++
> >>  target/s390x/gen-features.c     | 5 +++++
> >>  target/s390x/kvm.c              | 7 +++++++
> >>  7 files changed, 23 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> >> index ff6e4ec..6e7c610 100644
> >> --- a/hw/intc/s390_flic.c
> >> +++ b/hw/intc/s390_flic.c
> >> @@ -163,9 +163,10 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
> >>      if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
> >>          error_setg(errp, "flic property adapter_routes_max_batch too big"
> >>                     " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
> >> +        return;  
> > 
> > Hunk should go into a previous patch?
> > 
> > (And does it really matter?)  
> 
> I can
> - move to the previous series patch 8 (s390x/flic: introduce modify_ais_mode callback)
> - remove the return as it does not matter (this is realize, and realize failures here
> are fatal as far as I can tell)
> - keep it here since this version was tested and it does not hurt
> 

I'd say 2 or 3, no need to juggle existing patches for what is
basically a non-issue.
Cornelia Huck July 13, 2017, 1:06 p.m. UTC | #5
On Thu, 13 Jul 2017 14:29:55 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 07/13/2017 02:11 PM, Cornelia Huck wrote:
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> >> index 78ebe83..1901153 100644
> >> --- a/target/s390x/kvm.c
> >> +++ b/target/s390x/kvm.c
> >> @@ -302,6 +302,9 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> >>          }
> >>      }
> >>  
> >> +    /* Try to enable AIS facility */
> >> +    kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);  
> > 
> > What happens if you fail to enable it? You probably don't want to allow
> > the feature bit, then?  
> 
> Then this bit is off. This call will enable it in the kernel, if that fails
> the kernel will return this bit as disabled.

Looked at the kernel code again. I thought there was more to it, but I
misremembered. No further complaints here.

> >   
> >> +
> >>      qemu_mutex_init(&qemu_sigp_mutex);
> >>  
> >>      return 0;  
> > 
> > Let's summarize to make sure that I'm not confused:
> > 
> > - Starting with zEC12 GA1, we provide zpci, aen, ais in the full model  
> yes
> 
> > - Starting with zEC12 GA1, we provide zpci and aen in the default model  
> yes. ais has to be enabled manually for z12 and z13.
> The alternative is to have ais as part of the default model. This has the big
> disadvantage that -cpu zEC12 and -cpu z13 will stop working for all available
> distro kernels. I believe that a working -cpu z13 is more important than the
> need to manually enable ais.

Agreed.

> For the future
>  - We can make ais part of the default model for a future system when that happens since
>    the features for a new system will require a new host kernel anyway

Sounds fine.

>  - We can also make ais part of the default model for a future machine type (e.g. 2.13)
>    when we believe that the world has moved on to a newer kernel

I'd rather avoid relying on that.

> 
> 
> > - In the host model, we add zpci and aen; they might be switched off
> >   after applying the found model  
> 
> We also get ais from the kernel, so the host model will have zpci,ais and
> aen
> 
> > - Compat for 2.9 and earlier switches off zpci, aen, ais  
> 
> yes 
> 
> > - We unconditionally enable the kvm part of ais  
> 
> We tell the KVM code in the kernel to enable the facility bit (before the cpu
> model might take it way) and if QEMU really uses ais, that the kernel does
> the right thing then

Yeah, that's fine, see above.

> > 
> > I'm still not sure what's supposed to happen with new qemu + old kernel
> > (no ais) + full zEC12 GA1 or later model.  
> 
> We enable aen and zpci, but disable ais for that guest. In theory a guest
> can drive PCI devices without AIS. This is a valid configuration since zpci
> does not require ais. 

Yes, also fine. Looking at the kernel code cleared things up :)

> The fact that Linux requires ais to use PCI is unfortunate but that could 
> be "fixed" Linux if necessary.

I'm not sure there's a need for this. Once a change like this has
landed in distro kernels, the same distros will also have the ais
changes in their hypervisor kernels.

Thanks for spelling things out again, this stuff always gives me a
headache.
Halil Pasic July 13, 2017, 1:07 p.m. UTC | #6
On 07/13/2017 02:57 PM, Cornelia Huck wrote:
> On Thu, 13 Jul 2017 14:41:05 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 07/13/2017 02:11 PM, Cornelia Huck wrote:
>>> On Thu, 13 Jul 2017 12:40:28 +0200
>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>   
>>>> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>>
>>>> zPCI instructions and facilities are available since IBM zEnterprise
>>>> EC12. To support z/PCI in QEMU we enable zpci, aen and ais facilities
>>>> starting with zEC12 GA1. And we always set zpci and aen bits in max cpu
>>>> model. Later they might be switched off due to applied real cpu model.
>>>> For ais bit, we only provide it in the full cpu model beginning with
>>>> zEC12 and defer its enablement in the default cpu model to a later point
>>>> in time. At the same time, disable them for 2.9 and older machines.
>>>>
>>>> Because of introducing AIS facility, we could check if it's enabled to
>>>> initialize flic->ais_supported with the real value.
>>>>
>>>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> ---
>>>>  hw/intc/s390_flic.c             | 3 ++-
>>>>  hw/intc/s390_flic_kvm.c         | 3 ---
>>>>  hw/s390x/s390-virtio-ccw.c      | 3 +++
>>>>  target/s390x/cpu_features.c     | 3 +++
>>>>  target/s390x/cpu_features_def.h | 3 +++
>>>>  target/s390x/gen-features.c     | 5 +++++
>>>>  target/s390x/kvm.c              | 7 +++++++
>>>>  7 files changed, 23 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
>>>> index ff6e4ec..6e7c610 100644
>>>> --- a/hw/intc/s390_flic.c
>>>> +++ b/hw/intc/s390_flic.c
>>>> @@ -163,9 +163,10 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
>>>>      if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
>>>>          error_setg(errp, "flic property adapter_routes_max_batch too big"
>>>>                     " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
>>>> +        return;  
>>>
>>> Hunk should go into a previous patch?
>>>
>>> (And does it really matter?)  
>>
>> I can
>> - move to the previous series patch 8 (s390x/flic: introduce modify_ais_mode callback)
>> - remove the return as it does not matter (this is realize, and realize failures here
>> are fatal as far as I can tell)
>> - keep it here since this version was tested and it does not hurt
>>
> 
> I'd say 2 or 3, no need to juggle existing patches for what is
> basically a non-issue.
> 

Realize failures are supposed to be fatal and it's also working
like that for flic.

I'm in favor of 3 (keeping) as the resulting code is cleaner:
it does not make any sense to 'continue realizing', even if 
'continue realizing' and set a correct ais_supported just
to fail later does not hurt.
Christian Borntraeger July 13, 2017, 1:11 p.m. UTC | #7
On 07/13/2017 03:06 PM, Cornelia Huck wrote:
> On Thu, 13 Jul 2017 14:29:55 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 07/13/2017 02:11 PM, Cornelia Huck wrote:
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>> index 78ebe83..1901153 100644
>>>> --- a/target/s390x/kvm.c
>>>> +++ b/target/s390x/kvm.c
>>>> @@ -302,6 +302,9 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>>>          }
>>>>      }
>>>>  
>>>> +    /* Try to enable AIS facility */
>>>> +    kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);  
>>>
>>> What happens if you fail to enable it? You probably don't want to allow
>>> the feature bit, then?  
>>
>> Then this bit is off. This call will enable it in the kernel, if that fails
>> the kernel will return this bit as disabled.
> 
> Looked at the kernel code again. I thought there was more to it, but I
> misremembered. No further complaints here.
> 
>>>   
>>>> +
>>>>      qemu_mutex_init(&qemu_sigp_mutex);
>>>>  
>>>>      return 0;  
>>>
>>> Let's summarize to make sure that I'm not confused:
>>>
>>> - Starting with zEC12 GA1, we provide zpci, aen, ais in the full model  
>> yes
>>
>>> - Starting with zEC12 GA1, we provide zpci and aen in the default model  
>> yes. ais has to be enabled manually for z12 and z13.
>> The alternative is to have ais as part of the default model. This has the big
>> disadvantage that -cpu zEC12 and -cpu z13 will stop working for all available
>> distro kernels. I believe that a working -cpu z13 is more important than the
>> need to manually enable ais.
> 
> Agreed.
> 
>> For the future
>>  - We can make ais part of the default model for a future system when that happens since
>>    the features for a new system will require a new host kernel anyway
> 
> Sounds fine.
> 
>>  - We can also make ais part of the default model for a future machine type (e.g. 2.13)
>>    when we believe that the world has moved on to a newer kernel
> 
> I'd rather avoid relying on that.
> 
>>
>>
>>> - In the host model, we add zpci and aen; they might be switched off
>>>   after applying the found model  
>>
>> We also get ais from the kernel, so the host model will have zpci,ais and
>> aen
>>
>>> - Compat for 2.9 and earlier switches off zpci, aen, ais  
>>
>> yes 
>>
>>> - We unconditionally enable the kvm part of ais  
>>
>> We tell the KVM code in the kernel to enable the facility bit (before the cpu
>> model might take it way) and if QEMU really uses ais, that the kernel does
>> the right thing then
> 
> Yeah, that's fine, see above.
> 
>>>
>>> I'm still not sure what's supposed to happen with new qemu + old kernel
>>> (no ais) + full zEC12 GA1 or later model.  
>>
>> We enable aen and zpci, but disable ais for that guest. In theory a guest
>> can drive PCI devices without AIS. This is a valid configuration since zpci
>> does not require ais. 
> 
> Yes, also fine. Looking at the kernel code cleared things up :)
> 
>> The fact that Linux requires ais to use PCI is unfortunate but that could 
>> be "fixed" Linux if necessary.
> 
> I'm not sure there's a need for this. Once a change like this has
> landed in distro kernels, the same distros will also have the ais
> changes in their hypervisor kernels.
> 
> Thanks for spelling things out again, this stuff always gives me a
> headache.

Assuming that I will keep the return because I like Halils explanation of
"I'm in favor of 3 (keeping) as the resulting code is cleaner:
it does not make any sense to 'continue realizing', even if 
'continue realizing' and set a correct ais_supported just
to fail later does not hurt.
"

Is that an Acked-by or Reviewed-by ?
Cornelia Huck July 13, 2017, 1:15 p.m. UTC | #8
On Thu, 13 Jul 2017 15:11:15 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 07/13/2017 03:06 PM, Cornelia Huck wrote:
> > On Thu, 13 Jul 2017 14:29:55 +0200
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> On 07/13/2017 02:11 PM, Cornelia Huck wrote:
> >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c  
> >>>> index 78ebe83..1901153 100644
> >>>> --- a/target/s390x/kvm.c
> >>>> +++ b/target/s390x/kvm.c
> >>>> @@ -302,6 +302,9 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> >>>>          }
> >>>>      }
> >>>>  
> >>>> +    /* Try to enable AIS facility */
> >>>> +    kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);    
> >>>
> >>> What happens if you fail to enable it? You probably don't want to allow
> >>> the feature bit, then?    
> >>
> >> Then this bit is off. This call will enable it in the kernel, if that fails
> >> the kernel will return this bit as disabled.  
> > 
> > Looked at the kernel code again. I thought there was more to it, but I
> > misremembered. No further complaints here.
> >   
> >>>     
> >>>> +
> >>>>      qemu_mutex_init(&qemu_sigp_mutex);
> >>>>  
> >>>>      return 0;    
> >>>
> >>> Let's summarize to make sure that I'm not confused:
> >>>
> >>> - Starting with zEC12 GA1, we provide zpci, aen, ais in the full model    
> >> yes
> >>  
> >>> - Starting with zEC12 GA1, we provide zpci and aen in the default model    
> >> yes. ais has to be enabled manually for z12 and z13.
> >> The alternative is to have ais as part of the default model. This has the big
> >> disadvantage that -cpu zEC12 and -cpu z13 will stop working for all available
> >> distro kernels. I believe that a working -cpu z13 is more important than the
> >> need to manually enable ais.  
> > 
> > Agreed.
> >   
> >> For the future
> >>  - We can make ais part of the default model for a future system when that happens since
> >>    the features for a new system will require a new host kernel anyway  
> > 
> > Sounds fine.
> >   
> >>  - We can also make ais part of the default model for a future machine type (e.g. 2.13)
> >>    when we believe that the world has moved on to a newer kernel  
> > 
> > I'd rather avoid relying on that.
> >   
> >>
> >>  
> >>> - In the host model, we add zpci and aen; they might be switched off
> >>>   after applying the found model    
> >>
> >> We also get ais from the kernel, so the host model will have zpci,ais and
> >> aen
> >>  
> >>> - Compat for 2.9 and earlier switches off zpci, aen, ais    
> >>
> >> yes 
> >>  
> >>> - We unconditionally enable the kvm part of ais    
> >>
> >> We tell the KVM code in the kernel to enable the facility bit (before the cpu
> >> model might take it way) and if QEMU really uses ais, that the kernel does
> >> the right thing then  
> > 
> > Yeah, that's fine, see above.
> >   
> >>>
> >>> I'm still not sure what's supposed to happen with new qemu + old kernel
> >>> (no ais) + full zEC12 GA1 or later model.    
> >>
> >> We enable aen and zpci, but disable ais for that guest. In theory a guest
> >> can drive PCI devices without AIS. This is a valid configuration since zpci
> >> does not require ais.   
> > 
> > Yes, also fine. Looking at the kernel code cleared things up :)
> >   
> >> The fact that Linux requires ais to use PCI is unfortunate but that could 
> >> be "fixed" Linux if necessary.  
> > 
> > I'm not sure there's a need for this. Once a change like this has
> > landed in distro kernels, the same distros will also have the ais
> > changes in their hypervisor kernels.
> > 
> > Thanks for spelling things out again, this stuff always gives me a
> > headache.  
> 
> Assuming that I will keep the return because I like Halils explanation of
> "I'm in favor of 3 (keeping) as the resulting code is cleaner:
> it does not make any sense to 'continue realizing', even if 
> 'continue realizing' and set a correct ais_supported just
> to fail later does not hurt.
> "
> 
> Is that an Acked-by or Reviewed-by ?
> 

_This_ is an R-b ;)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
diff mbox

Patch

diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
index ff6e4ec..6e7c610 100644
--- a/hw/intc/s390_flic.c
+++ b/hw/intc/s390_flic.c
@@ -163,9 +163,10 @@  static void s390_flic_common_realize(DeviceState *dev, Error **errp)
     if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
         error_setg(errp, "flic property adapter_routes_max_batch too big"
                    " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
+        return;
     }
 
-    fs->ais_supported = true;
+    fs->ais_supported = s390_has_feat(S390_FEAT_ADAPTER_INT_SUPPRESSION);
 }
 
 static void s390_flic_class_init(ObjectClass *oc, void *data)
diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index a587ace..d93503f 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -444,7 +444,6 @@  typedef struct KVMS390FLICStateClass {
 
 static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
 {
-    S390FLICState *fs = S390_FLIC_COMMON(dev);
     KVMS390FLICState *flic_state = KVM_S390_FLIC(dev);
     struct kvm_create_device cd = {0};
     struct kvm_device_attr test_attr = {0};
@@ -476,8 +475,6 @@  static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
     test_attr.group = KVM_DEV_FLIC_CLEAR_IO_IRQ;
     flic_state->clear_io_supported = !ioctl(flic_state->fd,
                                             KVM_HAS_DEVICE_ATTR, test_attr);
-
-    fs->ais_supported = false;
     return;
 fail:
     error_propagate(errp, errp_local);
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 23e9658..e484aed 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -503,6 +503,9 @@  DEFINE_CCW_MACHINE(2_10, "2.10", true);
 static void ccw_machine_2_9_instance_options(MachineState *machine)
 {
     ccw_machine_2_10_instance_options(machine);
+    s390_cpudef_featoff_greater(12, 1, S390_FEAT_ZPCI);
+    s390_cpudef_featoff_greater(12, 1, S390_FEAT_ADAPTER_INT_SUPPRESSION);
+    s390_cpudef_featoff_greater(12, 1, S390_FEAT_ADAPTER_EVENT_NOTIFICATION);
 }
 
 static void ccw_machine_2_9_class_options(MachineClass *mc)
diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
index 0436dc2..8ab5cd7 100644
--- a/target/s390x/cpu_features.c
+++ b/target/s390x/cpu_features.c
@@ -74,6 +74,9 @@  static const S390FeatDef s390_features[] = {
     FEAT_INIT("stfle53", S390_FEAT_TYPE_STFL, 53, "Various facilities introduced with z13"),
     FEAT_INIT("msa5-base", S390_FEAT_TYPE_STFL, 57, "Message-security-assist-extension-5 facility (excluding subfunctions)"),
     FEAT_INIT("ri", S390_FEAT_TYPE_STFL, 64, "CPU runtime-instrumentation facility"),
+    FEAT_INIT("zpci", S390_FEAT_TYPE_STFL, 69, "z/PCI facility"),
+    FEAT_INIT("aen", S390_FEAT_TYPE_STFL, 71, "General-purpose-adapter-event-notification facility"),
+    FEAT_INIT("ais", S390_FEAT_TYPE_STFL, 72, "General-purpose-adapter-interruption-suppression facility"),
     FEAT_INIT("te", S390_FEAT_TYPE_STFL, 73, "Transactional-execution facility"),
     FEAT_INIT("sthyi", S390_FEAT_TYPE_STFL, 74, "Store-hypervisor-information facility"),
     FEAT_INIT("aefsi", S390_FEAT_TYPE_STFL, 75, "Access-exception-fetch/store-indication facility"),
diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
index f5bb7ed..c939a00 100644
--- a/target/s390x/cpu_features_def.h
+++ b/target/s390x/cpu_features_def.h
@@ -65,6 +65,9 @@  typedef enum {
     S390_FEAT_STFLE_53,
     S390_FEAT_MSA_EXT_5,
     S390_FEAT_RUNTIME_INSTRUMENTATION,
+    S390_FEAT_ZPCI,
+    S390_FEAT_ADAPTER_EVENT_NOTIFICATION,
+    S390_FEAT_ADAPTER_INT_SUPPRESSION,
     S390_FEAT_TRANSACTIONAL_EXE,
     S390_FEAT_STORE_HYPERVISOR_INFO,
     S390_FEAT_ACCESS_EXCEPTION_FS_INDICATION,
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 8ca2b47..622ee24 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -389,6 +389,9 @@  static uint16_t full_GEN12_GA1[] = {
     S390_FEAT_CONSTRAINT_TRANSACTIONAL_EXE,
     S390_FEAT_TRANSACTIONAL_EXE,
     S390_FEAT_RUNTIME_INSTRUMENTATION,
+    S390_FEAT_ZPCI,
+    S390_FEAT_ADAPTER_EVENT_NOTIFICATION,
+    S390_FEAT_ADAPTER_INT_SUPPRESSION,
     S390_FEAT_EDAT_2,
 };
 
@@ -446,6 +449,8 @@  static uint16_t default_GEN12_GA1[] = {
     S390_FEAT_CONSTRAINT_TRANSACTIONAL_EXE,
     S390_FEAT_TRANSACTIONAL_EXE,
     S390_FEAT_RUNTIME_INSTRUMENTATION,
+    S390_FEAT_ZPCI,
+    S390_FEAT_ADAPTER_EVENT_NOTIFICATION,
     S390_FEAT_EDAT_2,
 };
 
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 78ebe83..1901153 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -302,6 +302,9 @@  int kvm_arch_init(MachineState *ms, KVMState *s)
         }
     }
 
+    /* Try to enable AIS facility */
+    kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
+
     qemu_mutex_init(&qemu_sigp_mutex);
 
     return 0;
@@ -2635,6 +2638,10 @@  void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
         set_bit(S390_FEAT_CMM, model->features);
     }
 
+    /* set zpci and aen facilities */
+    set_bit(S390_FEAT_ZPCI, model->features);
+    set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
+
     if (s390_known_cpu_type(cpu_type)) {
         /* we want the exact model, even if some features are missing */
         model->def = s390_find_cpu_def(cpu_type, ibc_gen(unblocked_ibc),