| Message ID | 20170718142455.32676-5-cohuck@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
在 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.
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.
在 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.
在 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.
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
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 --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 */
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(+)