Re: [Qemu-devel] [PATCH v3 4/9] s390x/pci: do not advertise pci on non-pci builds

[Cornelia Huck <cohuck@redhat.com>]

On Wed, 26 Jul 2017 11:28:17 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 25.07.2017 17:33, Cornelia Huck wrote:
> > Only set the zpci feature bit on builds that actually support pci.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  hw/s390x/s390-pci-bus.c  | 5 +++++
> >  hw/s390x/s390-pci-bus.h  | 1 +
> >  hw/s390x/s390-pci-stub.c | 4 ++++
> >  target/s390x/kvm.c       | 2 +-
> >  4 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > index c57f6ebae0..7b30d4c7bd 100644
> > --- a/hw/s390x/s390-pci-bus.c
> > +++ b/hw/s390x/s390-pci-bus.c
> > @@ -34,6 +34,11 @@
> >          }                                                         \
> >      } while (0)
> >  
> > +void pci_enable_zpci_feature(S390CPUModel *model)
> > +{
> > +    set_bit(S390_FEAT_ZPCI, model->features);
> > +}
> > +
> >  S390pciState *s390_get_phb(void)
> >  {
> >      static S390pciState *phb;
> > diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
> > index 5df6292509..d8796536b0 100644
> > --- a/hw/s390x/s390-pci-bus.h
> > +++ b/hw/s390x/s390-pci-bus.h
> > @@ -333,4 +333,5 @@ S390PCIBusDevice *s390_pci_find_dev_by_fid(S390pciState *s, uint32_t fid);
> >  S390PCIBusDevice *s390_pci_find_next_avail_dev(S390pciState *s,
> >                                                 S390PCIBusDevice *pbdev);
> >  
> > +void pci_enable_zpci_feature(S390CPUModel *model);
> >  #endif
> > diff --git a/hw/s390x/s390-pci-stub.c b/hw/s390x/s390-pci-stub.c
> > index cc7278a865..8ceaf482e7 100644
> > --- a/hw/s390x/s390-pci-stub.c
> > +++ b/hw/s390x/s390-pci-stub.c
> > @@ -72,3 +72,7 @@ S390PCIBusDevice *s390_pci_find_dev_by_idx(S390pciState *s, uint32_t idx)
> >  {
> >      return NULL;
> >  }
> > +
> > +void pci_enable_zpci_feature(S390CPUModel *model)
> > +{
> > +}
> > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> > index c4c5791d27..866ac3d414 100644
> > --- a/target/s390x/kvm.c
> > +++ b/target/s390x/kvm.c
> > @@ -2662,7 +2662,7 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
> >      }
> >  
> >      /* We emulate a zPCI bus and AEN, therefore we don't need HW support */
> > -    set_bit(S390_FEAT_ZPCI, model->features);
> > +    pci_enable_zpci_feature(model);  
> 
> While I see how this solves the problem, I don't really like it. If
> there is a function "save_the_world()" I expect it to save the world in
> all scenarios ;)

What, you did not read the fine print? ;)

> 
> What about the same approach but rather
> 
> if(pci_configured())
> 	set_bit(S390_FEAT_ZPCI, model->features);
> 
> Or of course zpci_configured(), pci_zpci_bus_available() ...

I'm not sure that would help with readability. OTOH, it would be usable
to check for pci support in other places as well...

I can play with this a bit.

> 
> >      set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
> >  
> >      if (s390_known_cpu_type(cpu_type)) {
> >   
> 
>