[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [qemu-s390x] [PATCH v2 3/6] s390x/pci: Warn when adding PCI devices

From: Collin Walling
Subject: Re: [qemu-s390x] [PATCH v2 3/6] s390x/pci: Warn when adding PCI devices without the 'zpci' feature
Date: Mon, 4 Feb 2019 17:42:02 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1

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.


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>

reply via email to

[Prev in Thread] Current Thread [Next in Thread]