diff mbox

[RFC,v2,4/9] s390x/pci: do not advertise pci on non-pci builds

Message ID 20170718142455.32676-5-cohuck@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Cornelia Huck July 18, 2017, 2:24 p.m. UTC
Only set the zpci and aen feature bits on builds that actually
support pci.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 target/s390x/kvm.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Christian Borntraeger July 18, 2017, 7:56 p.m. UTC | #1
On 07/18/2017 04:24 PM, Cornelia Huck wrote:
> Only set the zpci and aen feature bits on builds that actually
> support pci.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  target/s390x/kvm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 831492f9a2..880eccd58a 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2685,8 +2685,10 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>      }
> 
>      /* set zpci and aen facilities */
> +#ifdef CONFIG_PCI
>      set_bit(S390_FEAT_ZPCI, model->features);
>      set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
> +#endif
> 
>      if (s390_known_cpu_type(cpu_type)) {
>          /* we want the exact model, even if some features are missing */
> 

Not strictly necessary but do you also want to ifdef this

 kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);

call?

If not you could actually even allow AEN but not PCI for !CONFIG_PCI.
Cornelia Huck July 19, 2017, 8 a.m. UTC | #2
On Tue, 18 Jul 2017 21:56:26 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 07/18/2017 04:24 PM, Cornelia Huck wrote:
> > Only set the zpci and aen feature bits on builds that actually
> > support pci.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  target/s390x/kvm.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> > index 831492f9a2..880eccd58a 100644
> > --- a/target/s390x/kvm.c
> > +++ b/target/s390x/kvm.c
> > @@ -2685,8 +2685,10 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
> >      }
> > 
> >      /* set zpci and aen facilities */
> > +#ifdef CONFIG_PCI
> >      set_bit(S390_FEAT_ZPCI, model->features);
> >      set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
> > +#endif
> > 
> >      if (s390_known_cpu_type(cpu_type)) {
> >          /* we want the exact model, even if some features are missing */
> >   
> 
> Not strictly necessary but do you also want to ifdef this
> 
>  kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
> 
> call?
> 
> If not you could actually even allow AEN but not PCI for !CONFIG_PCI.

I'm a bit unsure about the relationship of ais and aen with pci. I
remember that only adapters for pci currently support suppression,
although it could spread to other adapter types in the future. Not sure
about aen.

So I'd keep the ais enablement call, even though it won't have much of
an effect as no pci adapters will be registered.

As I don't quite remember what aen governed, I need to rely on your
feedback here.
Yi Min Zhao July 19, 2017, 8:56 a.m. UTC | #3
在 2017/7/19 下午4:00, Cornelia Huck 写道:
> On Tue, 18 Jul 2017 21:56:26 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>
>> On 07/18/2017 04:24 PM, Cornelia Huck wrote:
>>> Only set the zpci and aen feature bits on builds that actually
>>> support pci.
>>>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>   target/s390x/kvm.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>> index 831492f9a2..880eccd58a 100644
>>> --- a/target/s390x/kvm.c
>>> +++ b/target/s390x/kvm.c
>>> @@ -2685,8 +2685,10 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>>>       }
>>>
>>>       /* set zpci and aen facilities */
>>> +#ifdef CONFIG_PCI
>>>       set_bit(S390_FEAT_ZPCI, model->features);
>>>       set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
>>> +#endif
>>>
>>>       if (s390_known_cpu_type(cpu_type)) {
>>>           /* we want the exact model, even if some features are missing */
>>>    
>> Not strictly necessary but do you also want to ifdef this
>>
>>   kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
>>
>> call?
>>
>> If not you could actually even allow AEN but not PCI for !CONFIG_PCI.
> I'm a bit unsure about the relationship of ais and aen with pci. I
> remember that only adapters for pci currently support suppression,
> although it could spread to other adapter types in the future. Not sure
> about aen.
>
> So I'd keep the ais enablement call, even though it won't have much of
> an effect as no pci adapters will be registered.
>
> As I don't quite remember what aen governed, I need to rely on your
> feedback here.
>
>
My understanding is that zpci replies on aen. But aen could exist 
independently.
After all, there is other device type using aen. I think only wrapping 
zpci is
enough.
Cornelia Huck July 19, 2017, 9:24 a.m. UTC | #4
On Wed, 19 Jul 2017 16:56:18 +0800
Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:

> 在 2017/7/19 下午4:00, Cornelia Huck 写道:
> > On Tue, 18 Jul 2017 21:56:26 +0200
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >  
> >> On 07/18/2017 04:24 PM, Cornelia Huck wrote:  
> >>> Only set the zpci and aen feature bits on builds that actually
> >>> support pci.
> >>>
> >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> >>> ---
> >>>   target/s390x/kvm.c | 2 ++
> >>>   1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> >>> index 831492f9a2..880eccd58a 100644
> >>> --- a/target/s390x/kvm.c
> >>> +++ b/target/s390x/kvm.c
> >>> @@ -2685,8 +2685,10 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
> >>>       }
> >>>
> >>>       /* set zpci and aen facilities */
> >>> +#ifdef CONFIG_PCI
> >>>       set_bit(S390_FEAT_ZPCI, model->features);
> >>>       set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
> >>> +#endif
> >>>
> >>>       if (s390_known_cpu_type(cpu_type)) {
> >>>           /* we want the exact model, even if some features are missing */
> >>>      
> >> Not strictly necessary but do you also want to ifdef this
> >>
> >>   kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
> >>
> >> call?
> >>
> >> If not you could actually even allow AEN but not PCI for !CONFIG_PCI.  
> > I'm a bit unsure about the relationship of ais and aen with pci. I
> > remember that only adapters for pci currently support suppression,
> > although it could spread to other adapter types in the future. Not sure
> > about aen.
> >
> > So I'd keep the ais enablement call, even though it won't have much of
> > an effect as no pci adapters will be registered.
> >
> > As I don't quite remember what aen governed, I need to rely on your
> > feedback here.
> >
> >  
> My understanding is that zpci replies on aen. But aen could exist 
> independently.
> After all, there is other device type using aen. I think only wrapping 
> zpci is
> enough.

Ah, was aen the indicator bits related support? If yes, I agree that we
should only turn off zpci.
Yi Min Zhao July 19, 2017, 9:27 a.m. UTC | #5
在 2017/7/19 下午5:24, Cornelia Huck 写道:
> On Wed, 19 Jul 2017 16:56:18 +0800
> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
>
>> 在 2017/7/19 下午4:00, Cornelia Huck 写道:
>>> On Tue, 18 Jul 2017 21:56:26 +0200
>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>   
>>>> On 07/18/2017 04:24 PM, Cornelia Huck wrote:
>>>>> Only set the zpci and aen feature bits on builds that actually
>>>>> support pci.
>>>>>
>>>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>>>> ---
>>>>>    target/s390x/kvm.c | 2 ++
>>>>>    1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>>> index 831492f9a2..880eccd58a 100644
>>>>> --- a/target/s390x/kvm.c
>>>>> +++ b/target/s390x/kvm.c
>>>>> @@ -2685,8 +2685,10 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>>>>>        }
>>>>>
>>>>>        /* set zpci and aen facilities */
>>>>> +#ifdef CONFIG_PCI
>>>>>        set_bit(S390_FEAT_ZPCI, model->features);
>>>>>        set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
>>>>> +#endif
>>>>>
>>>>>        if (s390_known_cpu_type(cpu_type)) {
>>>>>            /* we want the exact model, even if some features are missing */
>>>>>       
>>>> Not strictly necessary but do you also want to ifdef this
>>>>
>>>>    kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
>>>>
>>>> call?
>>>>
>>>> If not you could actually even allow AEN but not PCI for !CONFIG_PCI.
>>> I'm a bit unsure about the relationship of ais and aen with pci. I
>>> remember that only adapters for pci currently support suppression,
>>> although it could spread to other adapter types in the future. Not sure
>>> about aen.
>>>
>>> So I'd keep the ais enablement call, even though it won't have much of
>>> an effect as no pci adapters will be registered.
>>>
>>> As I don't quite remember what aen governed, I need to rely on your
>>> feedback here.
>>>
>>>   
>> My understanding is that zpci replies on aen. But aen could exist
>> independently.
>> After all, there is other device type using aen. I think only wrapping
>> zpci is
>> enough.
> Ah, was aen the indicator bits related support? If yes, I agree that we
> should only turn off zpci.
>
>
Yes, set summary and indicator bits. Related stuff is in flic, but not 
in zpci.
Yi Min Zhao July 19, 2017, 9:38 a.m. UTC | #6
在 2017/7/19 下午5:27, Yi Min Zhao 写道:
>
>
> 在 2017/7/19 下午5:24, Cornelia Huck 写道:
>> On Wed, 19 Jul 2017 16:56:18 +0800
>> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:
>>
>>> 在 2017/7/19 下午4:00, Cornelia Huck 写道:
>>>> On Tue, 18 Jul 2017 21:56:26 +0200
>>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>>> On 07/18/2017 04:24 PM, Cornelia Huck wrote:
>>>>>> Only set the zpci and aen feature bits on builds that actually
>>>>>> support pci.
>>>>>>
>>>>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>>>>> ---
>>>>>>    target/s390x/kvm.c | 2 ++
>>>>>>    1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>>>> index 831492f9a2..880eccd58a 100644
>>>>>> --- a/target/s390x/kvm.c
>>>>>> +++ b/target/s390x/kvm.c
>>>>>> @@ -2685,8 +2685,10 @@ void 
>>>>>> kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>>>>>>        }
>>>>>>
>>>>>>        /* set zpci and aen facilities */
>>>>>> +#ifdef CONFIG_PCI
>>>>>>        set_bit(S390_FEAT_ZPCI, model->features);
>>>>>>        set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, 
>>>>>> model->features);
>>>>>> +#endif
>>>>>>
>>>>>>        if (s390_known_cpu_type(cpu_type)) {
>>>>>>            /* we want the exact model, even if some features are 
>>>>>> missing */
>>>>> Not strictly necessary but do you also want to ifdef this
>>>>>
>>>>>    kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
>>>>>
>>>>> call?
>>>>>
>>>>> If not you could actually even allow AEN but not PCI for !CONFIG_PCI.
>>>> I'm a bit unsure about the relationship of ais and aen with pci. I
>>>> remember that only adapters for pci currently support suppression,
>>>> although it could spread to other adapter types in the future. Not 
>>>> sure
>>>> about aen.
>>>>
>>>> So I'd keep the ais enablement call, even though it won't have much of
>>>> an effect as no pci adapters will be registered.
>>>>
>>>> As I don't quite remember what aen governed, I need to rely on your
>>>> feedback here.
>>>>
>>> My understanding is that zpci replies on aen. But aen could exist
>>> independently.
>>> After all, there is other device type using aen. I think only wrapping
>>> zpci is
>>> enough.
>> Ah, was aen the indicator bits related support? If yes, I agree that we
>> should only turn off zpci.
>>
>>
> Yes, set summary and indicator bits. Related stuff is in flic, but not 
> in zpci.
>
>
>
I think of another problem. If we didn't config pci, then we don't have zpci
feature in max cpu model. So how to process the conflict between requested
cpu model and max cpu model. For example, if we start 2.10 machine and
want to use z12 cpu model, maybe the guest cannot startup because of
missing zpci feature. So the only way is we explicitly turn it off in 
qemu cmdline.
But I'm not sure if it's an issue.
Thomas Huth July 19, 2017, 11:41 a.m. UTC | #7
On 18.07.2017 16:24, Cornelia Huck wrote:
> Only set the zpci and aen feature bits on builds that actually
> support pci.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  target/s390x/kvm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 831492f9a2..880eccd58a 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2685,8 +2685,10 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>      }
>  
>      /* set zpci and aen facilities */
> +#ifdef CONFIG_PCI
>      set_bit(S390_FEAT_ZPCI, model->features);
>      set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
> +#endif
>  
>      if (s390_known_cpu_type(cpu_type)) {
>          /* we want the exact model, even if some features are missing */

Have you tried whether this actually still works right in builds where
CONFIG_PCI is enabled? I'm afraid, but I think the #ifdef does not work
here, since CONFIG_PCI is a Makefile-only setting, and it does not get
set in config-target.h at all...

 Thomas
Cornelia Huck July 19, 2017, 12:18 p.m. UTC | #8
On Wed, 19 Jul 2017 13:41:04 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 18.07.2017 16:24, Cornelia Huck wrote:
> > Only set the zpci and aen feature bits on builds that actually
> > support pci.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  target/s390x/kvm.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> > index 831492f9a2..880eccd58a 100644
> > --- a/target/s390x/kvm.c
> > +++ b/target/s390x/kvm.c
> > @@ -2685,8 +2685,10 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
> >      }
> >  
> >      /* set zpci and aen facilities */
> > +#ifdef CONFIG_PCI
> >      set_bit(S390_FEAT_ZPCI, model->features);
> >      set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
> > +#endif
> >  
> >      if (s390_known_cpu_type(cpu_type)) {
> >          /* we want the exact model, even if some features are missing */  
> 
> Have you tried whether this actually still works right in builds where
> CONFIG_PCI is enabled? I'm afraid, but I think the #ifdef does not work
> here, since CONFIG_PCI is a Makefile-only setting, and it does not get
> set in config-target.h at all...

Of course it does not. Sigh.

I think I'll just move this into a helper function in the zpci code
resp. the zpci stub.
diff mbox

Patch

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 831492f9a2..880eccd58a 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2685,8 +2685,10 @@  void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
     }
 
     /* set zpci and aen facilities */
+#ifdef CONFIG_PCI
     set_bit(S390_FEAT_ZPCI, model->features);
     set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
+#endif
 
     if (s390_known_cpu_type(cpu_type)) {
         /* we want the exact model, even if some features are missing */