Re: [qemu-s390x] [PATCH v2 3/6] s390x/pci: Warn when adding PCI devices without the 'zpci' feature

[Collin Walling]

On 2/4/19 4:54 PM, David Hildenbrand wrote:
> On 04.02.19 21:19, Collin Walling wrote:
> > On 1/30/19 10:57 AM, David Hildenbrand wrote:
> > > We decided to always create the PCI host bridge, even if 'zpci' is not
> > > enabled (due to migration compatibility). This however right now allows
> > > to add zPCI/PCI devices to a VM although the guest will never actually see
> > > them, confusing people that are using a simple CPU model that has no
> > > 'zpci' enabled - "Why isn't this working" (David Hildenbrand)
> > >
> > > Let's check for 'zpci' and at least print a warning that this will not
> > > work as expected. We could also bail out, however that might break
> > > existing QEMU commandlines.
> > >
> > > Reviewed-by: Thomas Huth <address@hidden>
> > > Signed-off-by: David Hildenbrand <address@hidden>
> > > ---
> > >    hw/s390x/s390-pci-bus.c | 5 +++++
> > >    1 file changed, 5 insertions(+)
> > >
> > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > > index 9b5c5fff60..2efd9186c2 100644
> > > --- a/hw/s390x/s390-pci-bus.c
> > > +++ b/hw/s390x/s390-pci-bus.c
> > > @@ -826,6 +826,11 @@ static void s390_pcihost_pre_plug(HotplugHandler 
> > > *hotplug_dev, DeviceState *dev,
> > >    {
> > >        S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
> > >    
> > > +    if (!s390_has_feat(S390_FEAT_ZPCI)) {
> > > +        warn_report("PCI/zPCI device without the 'zpci' CPU feature."
> > > +                    " The guest will not be able to see/use this device");
> > > +    }
> > > +
> > >        if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > >            PCIDevice *pdev = PCI_DEVICE(dev);
> > >    
> >
> > I wonder if someone might misconstrue this as "the _PCI device_ needs
> > the zpci feature." I think "'zpci' CPU feature required to support
> > PCI/zPCI devices." reads better. The last sentence is fine to me.
>
> Well, the guest needs the 'zpci' feature to see the device. And that's
> what that message says in my opinion. Not that a device needs to have a
> feature (I added "CPU feature" for this reason).
>
> "required to support" does it not make very clear what we actually want
> to say.
>
> Thanks!

I see your point. We can still plug in the device without the CPU
feature, but the device will ultimately be useless to the guest. Thanks
for clearing that up.

Still, the wording reads strangely to me. I read it as the PCI device
itself requires a "zpci CPU feature" which of course does not make sense
(and I fully understand that's not what you mean here).

What do you think about:

"PCI/zPCI device plugging without 'zpci' CPU feature enabled." along
with your second sentence, of course.

Either way you decide, it's still a good idea to have this warning in
here. I'm really just debating syntax and not semantics, so it's not
really important. I won't impede this patch over a differing opinion of
a small rewording. :)

Reviewed-by: Collin Walling <address@hidden>