[Qemu-devel] [PATCH] Give detailed info when pcie downstream port init failed

[Qemu-devel] [PATCH] Give detailed info when pcie downstream port init failed

[Cao jin]

detailed error info could show user about how to use it.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci-bridge/xio3130_downstream.c | 5 +++++
 1 file changed, 5 insertions(+)

When I learned to use root/upstream/downstream port like this:
qemu-system-x86_64 -M q35 -device ioh3420,bus=pcie.0,id=br1 -device x3130-upstream,id=up,bus=br1 -device xio3130-downstream,id=down,bus=up -device e1000,bus=down,id=net0 xxx...
downstream port just show me "Device initialization failed", while I don`t really know why at first. So it maybe good to let user know how to use it correctly.

diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index b3a6479..86b7970 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -23,6 +23,7 @@
 #include "hw/pci/msi.h"
 #include "hw/pci/pcie.h"
 #include "xio3130_downstream.h"
+#include "qemu/error-report.h"
 
 #define PCI_DEVICE_ID_TI_XIO3130D       0x8233  /* downstream port */
 #define XIO3130_REVISION                0x1
@@ -89,6 +90,10 @@ static int xio3130_downstream_initfn(PCIDevice *d)
     pcie_chassis_create(s->chassis);
     rc = pcie_chassis_add_slot(s);
     if (rc < 0) {
+        error_report("Slot number is reduplicate(chassis = %d, slot = %d). "
+                "Slot #(default 0) is unique within a chassis, chassis #"
+                "(default 0) is unique in the system. Please change chassis # "
+                "or slot #.", s->chassis, s->slot);
         goto err_pcie_cap;
     }
     pcie_cap_arifwd_init(d);
-- 
2.1.0


-- 
This message has been scanned for viruses and
dangerous content by Fujitsu, and is believed to be clean.

Re: [Qemu-devel] [PATCH] Give detailed info when pcie downstream port init failed

[Markus Armbruster]

Cao jin <caoj.fnst@cn.fujitsu.com> writes:

> detailed error info could show user about how to use it.
>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci-bridge/xio3130_downstream.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> When I learned to use root/upstream/downstream port like this:
> qemu-system-x86_64 -M q35 -device ioh3420,bus=pcie.0,id=br1 -device x3130-upstream,id=up,bus=br1 -device xio3130-downstream,id=down,bus=up -device e1000,bus=down,id=net0 xxx...
> downstream port just show me "Device initialization failed", while I don`t really know why at first. So it maybe good to let user know how to use it correctly.
>
> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> index b3a6479..86b7970 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -23,6 +23,7 @@
>  #include "hw/pci/msi.h"
>  #include "hw/pci/pcie.h"
>  #include "xio3130_downstream.h"
> +#include "qemu/error-report.h"
>  
>  #define PCI_DEVICE_ID_TI_XIO3130D       0x8233  /* downstream port */
>  #define XIO3130_REVISION                0x1
> @@ -89,6 +90,10 @@ static int xio3130_downstream_initfn(PCIDevice *d)
>      pcie_chassis_create(s->chassis);
>      rc = pcie_chassis_add_slot(s);
>      if (rc < 0) {
> +        error_report("Slot number is reduplicate(chassis = %d, slot = %d). "
> +                "Slot #(default 0) is unique within a chassis, chassis #"
> +                "(default 0) is unique in the system. Please change chassis # "
> +                "or slot #.", s->chassis, s->slot);
>          goto err_pcie_cap;
>      }
>      pcie_cap_arifwd_init(d);
> -- 
> 2.1.0

This covers one out of six error paths, and it doesn't work for QMP.

Please convert the device to realize() instead.  For an example, check
out commit 28b07e7.

Re: [Qemu-devel] [PATCH] Give detailed info when pcie downstream port init failed

[Cao jin]

Hi Markus

On 11/24/2015 06:08 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>
>> detailed error info could show user about how to use it.
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/pci-bridge/xio3130_downstream.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> When I learned to use root/upstream/downstream port like this:
>> qemu-system-x86_64 -M q35 -device ioh3420,bus=pcie.0,id=br1 -device x3130-upstream,id=up,bus=br1 -device xio3130-downstream,id=down,bus=up -device e1000,bus=down,id=net0 xxx...
>> downstream port just show me "Device initialization failed", while I don`t really know why at first. So it maybe good to let user know how to use it correctly.
>>
>> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
>> index b3a6479..86b7970 100644
>> --- a/hw/pci-bridge/xio3130_downstream.c
>> +++ b/hw/pci-bridge/xio3130_downstream.c
>> @@ -23,6 +23,7 @@
>>   #include "hw/pci/msi.h"
>>   #include "hw/pci/pcie.h"
>>   #include "xio3130_downstream.h"
>> +#include "qemu/error-report.h"
>>
>>   #define PCI_DEVICE_ID_TI_XIO3130D       0x8233  /* downstream port */
>>   #define XIO3130_REVISION                0x1
>> @@ -89,6 +90,10 @@ static int xio3130_downstream_initfn(PCIDevice *d)
>>       pcie_chassis_create(s->chassis);
>>       rc = pcie_chassis_add_slot(s);
>>       if (rc < 0) {
>> +        error_report("Slot number is reduplicate(chassis = %d, slot = %d). "
>> +                "Slot #(default 0) is unique within a chassis, chassis #"
>> +                "(default 0) is unique in the system. Please change chassis # "
>> +                "or slot #.", s->chassis, s->slot);
>>           goto err_pcie_cap;
>>       }
>>       pcie_cap_arifwd_init(d);
>> --
>> 2.1.0
>
> This covers one out of six error paths, and it doesn't work for QMP.
>
> Please convert the device to realize() instead.  For an example, check
> out commit 28b07e7.
>

Thanks for your quick review:)OK, will do the investigation right away

-- 
Yours Sincerely,

Cao Jin

-- 
This message has been scanned for viruses and
dangerous content by Fujitsu, and is believed to be clean.

Re: [Qemu-devel] [PATCH] Give detailed info when pcie downstream port init failed

[Cao jin]

Hi, Markus

On 11/24/2015 06:08 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>
>> detailed error info could show user about how to use it.
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/pci-bridge/xio3130_downstream.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> When I learned to use root/upstream/downstream port like this:
>> qemu-system-x86_64 -M q35 -device ioh3420,bus=pcie.0,id=br1 -device x3130-upstream,id=up,bus=br1 -device xio3130-downstream,id=down,bus=up -device e1000,bus=down,id=net0 xxx...
>> downstream port just show me "Device initialization failed", while I don`t really know why at first. So it maybe good to let user know how to use it correctly.
>>
>> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
>> index b3a6479..86b7970 100644
>> --- a/hw/pci-bridge/xio3130_downstream.c
>> +++ b/hw/pci-bridge/xio3130_downstream.c
>> @@ -23,6 +23,7 @@
>>   #include "hw/pci/msi.h"
>>   #include "hw/pci/pcie.h"
>>   #include "xio3130_downstream.h"
>> +#include "qemu/error-report.h"
>>
>>   #define PCI_DEVICE_ID_TI_XIO3130D       0x8233  /* downstream port */
>>   #define XIO3130_REVISION                0x1
>> @@ -89,6 +90,10 @@ static int xio3130_downstream_initfn(PCIDevice *d)
>>       pcie_chassis_create(s->chassis);
>>       rc = pcie_chassis_add_slot(s);
>>       if (rc < 0) {
>> +        error_report("Slot number is reduplicate(chassis = %d, slot = %d). "
>> +                "Slot #(default 0) is unique within a chassis, chassis #"
>> +                "(default 0) is unique in the system. Please change chassis # "
>> +                "or slot #.", s->chassis, s->slot);
>>           goto err_pcie_cap;
>>       }
>>       pcie_cap_arifwd_init(d);
>> --
>> 2.1.0
>
> This covers one out of six error paths, and it doesn't work for QMP.
>

I looked around in the code, and only find there are monitor & QMP may 
need be covered. Could give more hints about the "six error paths"?

> Please convert the device to realize() instead.  For an example, check
> out commit 28b07e7.
>

and this will cover to output to the monitor, right?

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH] Give detailed info when pcie downstream port init failed

[Markus Armbruster]

Cao jin <caoj.fnst@cn.fujitsu.com> writes:

> Hi, Markus
>
> On 11/24/2015 06:08 PM, Markus Armbruster wrote:
>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>
>>> detailed error info could show user about how to use it.
>>>
>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>>> ---
>>>   hw/pci-bridge/xio3130_downstream.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> When I learned to use root/upstream/downstream port like this:
>>> qemu-system-x86_64 -M q35 -device ioh3420,bus=pcie.0,id=br1 -device x3130-upstream,id=up,bus=br1 -device xio3130-downstream,id=down,bus=up -device e1000,bus=down,id=net0 xxx...
>>> downstream port just show me "Device initialization failed", while I don`t really know why at first. So it maybe good to let user know how to use it correctly.
>>>
>>> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
>>> index b3a6479..86b7970 100644
>>> --- a/hw/pci-bridge/xio3130_downstream.c
>>> +++ b/hw/pci-bridge/xio3130_downstream.c
>>> @@ -23,6 +23,7 @@
>>>   #include "hw/pci/msi.h"
>>>   #include "hw/pci/pcie.h"
>>>   #include "xio3130_downstream.h"
>>> +#include "qemu/error-report.h"
>>>
>>>   #define PCI_DEVICE_ID_TI_XIO3130D       0x8233  /* downstream port */
>>>   #define XIO3130_REVISION                0x1
>>> @@ -89,6 +90,10 @@ static int xio3130_downstream_initfn(PCIDevice *d)
>>>       pcie_chassis_create(s->chassis);
>>>       rc = pcie_chassis_add_slot(s);
>>>       if (rc < 0) {
>>> +        error_report("Slot number is reduplicate(chassis = %d, slot = %d). "
>>> +                "Slot #(default 0) is unique within a chassis, chassis #"
>>> +                "(default 0) is unique in the system. Please change chassis # "
>>> +                "or slot #.", s->chassis, s->slot);
>>>           goto err_pcie_cap;
>>>       }
>>>       pcie_cap_arifwd_init(d);
>>> --
>>> 2.1.0
>>
>> This covers one out of six error paths, and it doesn't work for QMP.
>>
>
> I looked around in the code, and only find there are monitor & QMP may
> need be covered. Could give more hints about the "six error paths"?
>
>> Please convert the device to realize() instead.  For an example, check
>> out commit 28b07e7.
>>
>
> and this will cover to output to the monitor, right?

The device still implements the old PCIDeviceClass.init() instead of the
new .realize().  All devices need to be converted to .realize().  You
can help by converting this one.

.init() reports errors with error_report() and returns 0 on success, -1
on failure.

.realize() passes errors to its callers via its errp argument.

Commit 28b07e7 can serve as example of how to convert a device to
realize().  There are many more.  The ones I did usually have "Convert
to realize()" in the commit message's first line.

Regarding six error paths: I counted six places that return -1 directly
or goto err or similar.  Your patch adds a suitable error message to one
of them.  The other five need one, too.  You'll have to examine the
called functions to find out whether they report anything.  If they do,
they need to be first converted to pass an Error to their callers
instead.

Re: [Qemu-devel] [PATCH] Give detailed info when pcie downstream port init failed

[Cao jin]

On 11/27/2015 10:22 PM, Markus Armbruster wrote:
> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>
>> Hi, Markus
>>
>> On 11/24/2015 06:08 PM, Markus Armbruster wrote:
[...]
>>
>> and this will cover to output to the monitor, right?
>
> The device still implements the old PCIDeviceClass.init() instead of the
> new .realize().  All devices need to be converted to .realize().  You
> can help by converting this one.
>
> .init() reports errors with error_report() and returns 0 on success, -1
> on failure.
>
> .realize() passes errors to its callers via its errp argument.
>

After reading you commit 28b07e7, I am aware of this;) Thanks very much 
for your detailed explanation:) I was always curious about why there are 
2 function(.init() & .realize()) for initializing device, now I guess I 
get it: it has just .init() at first, because of error report issue, the 
.realize() is added to replace it, in order to pass the error above. Do 
I understand it right ?

If I understand it right, I see there are still devices initialization 
using .init() of DeviceClass & PCIDeviceClass, maybe I can help to 
convert some of these.

> Commit 28b07e7 can serve as example of how to convert a device to
> realize().  There are many more.  The ones I did usually have "Convert
> to realize()" in the commit message's first line.
>

> Regarding six error paths: I counted six places that return -1 directly
> or goto err or similar.  Your patch adds a suitable error message to one
> of them.  The other five need one, too.  You'll have to examine the
> called functions to find out whether they report anything.  If they do,
> they need to be first converted to pass an Error to their callers
> instead.
>

It seems I have a misunderstanding about the "6 error paths":-[ I 
thought "error path" is a "error *reporting path*", like: fprintf the 
msg to stderr is a path, while output it via QMP is also a path. Because 
I did`t touch the code about -chardev/-mon/-monitor/-qmp before, so I 
spent 2 days reading the code:)

Now I am cleared, Thanks you very much for the clarifying, Markus, I 
think this mistake won`t happen to me later;) And actually, I was 
already thinking add suitable error msg to each error path:)

> .
>

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH] Give detailed info when pcie downstream port init failed

[Markus Armbruster]

Cao jin <caoj.fnst@cn.fujitsu.com> writes:

> On 11/27/2015 10:22 PM, Markus Armbruster wrote:
>> Cao jin <caoj.fnst@cn.fujitsu.com> writes:
>>
>>> Hi, Markus
>>>
>>> On 11/24/2015 06:08 PM, Markus Armbruster wrote:
> [...]
>>>
>>> and this will cover to output to the monitor, right?
>>
>> The device still implements the old PCIDeviceClass.init() instead of the
>> new .realize().  All devices need to be converted to .realize().  You
>> can help by converting this one.
>>
>> .init() reports errors with error_report() and returns 0 on success, -1
>> on failure.
>>
>> .realize() passes errors to its callers via its errp argument.
>>
>
> After reading you commit 28b07e7, I am aware of this;) Thanks very
> much for your detailed explanation:) I was always curious about why
> there are 2 function(.init() & .realize()) for initializing device,
> now I guess I get it: it has just .init() at first, because of error
> report issue, the .realize() is added to replace it, in order to pass
> the error above. Do I understand it right ?

Yes!

> If I understand it right, I see there are still devices initialization
> using .init() of DeviceClass & PCIDeviceClass, maybe I can help to
> convert some of these.

That would be helpful.  There are probably easy cases where you only
have to convert one device model at a time, and more complex cases,
where you have to convert supporting functions from error_report() to
error_set(), or convert device classes, not just individual devices.
Best try an easy one first.

>> Commit 28b07e7 can serve as example of how to convert a device to
>> realize().  There are many more.  The ones I did usually have "Convert
>> to realize()" in the commit message's first line.
>>
>
>> Regarding six error paths: I counted six places that return -1 directly
>> or goto err or similar.  Your patch adds a suitable error message to one
>> of them.  The other five need one, too.  You'll have to examine the
>> called functions to find out whether they report anything.  If they do,
>> they need to be first converted to pass an Error to their callers
>> instead.
>>
>
> It seems I have a misunderstanding about the "6 error paths":-[ I
> thought "error path" is a "error *reporting path*", like: fprintf the
> msg to stderr is a path, while output it via QMP is also a
> path. Because I did`t touch the code about -chardev/-mon/-monitor/-qmp
> before, so I spent 2 days reading the code:)

Welcome to the club :)

> Now I am cleared, Thanks you very much for the clarifying, Markus, I
> think this mistake won`t happen to me later;) And actually, I was
> already thinking add suitable error msg to each error path:)

Glad I could help you!