Re: [Qemu-devel] [qemu-s390x] [PATCH v2 3/6] s390x/pci: Warn when adding PCI devices without the 'zpci' feature
On Mon, 4 Feb 2019 23:45:35 +0100
David Hildenbrand wrote:
> On 04.02.19 23:42, Collin Walling wrote:
> > 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
> Signed-off-by: David Hildenbrand
> ---
> 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.
>
> "Plugging a PCI/zPCI device without the 'zpci' CPU feature enabled. The
> guest will not be able to see/use this device."
>
> would make sense to me!
Ok, I have now
warn_report("Plugging a PCI/zPCI device without the 'zpci' CPU "
"feature enabled; the guest will not be able to see/use "
"this device");
>
> >
> > 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
> >
>
> Thanks!
>
Re: [Qemu-devel] [qemu-s390x] [PATCH v2 3/6] s390x/pci: Warn when adding PCI devices without the 'zpci' feature
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
Signed-off-by: David Hildenbrand
---
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
Re: [Qemu-devel] [qemu-s390x] [PATCH v2 3/6] s390x/pci: Warn when adding PCI devices without the 'zpci' feature
On 04.02.19 23:42, Collin Walling wrote:
> 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
Signed-off-by: David Hildenbrand
---
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.
"Plugging a PCI/zPCI device without the 'zpci' CPU feature enabled. The
guest will not be able to see/use this device."
would make sense to me!
>
> 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
>
Thanks!
--
Thanks,
David / dhildenb
Re: [Qemu-devel] [qemu-s390x] [PATCH v2 3/6] s390x/pci: Warn when adding PCI devices without the 'zpci' feature
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
>> Signed-off-by: David Hildenbrand
>> ---
>> 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!
--
Thanks,
David / dhildenb
Re: [Qemu-devel] [qemu-s390x] [PATCH v2 3/6] s390x/pci: Warn when adding PCI devices without the 'zpci' feature
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
Signed-off-by: David Hildenbrand
---
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.
