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

[Markus Armbruster]

Cao jin <address@hidden> writes:

> Hi, Markus
>
> On 11/24/2015 06:08 PM, Markus Armbruster wrote:
>> Cao jin <address@hidden> writes:
>>
>>> detailed error info could show user about how to use it.
>>>
>>> Signed-off-by: Cao jin <address@hidden>
>>> ---
>>>   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.