[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & mo
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers |
Date: |
Thu, 03 Mar 2016 15:24:55 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Marcel Apfelbaum <address@hidden> writes:
> On 03/02/2016 11:13 AM, Markus Armbruster wrote:
>> This got lost over the Christmas break, sorry.
>>
>> Cc'ing Marcel for additional PCI expertise.
>>
>> Cao jin <address@hidden> writes:
>>
>>> msi_init() is a supporting function in PCI device initialization,
>>> in order to convert .init() to .realize(), it should be modified first.
>>
>> "Supporting function" doesn't imply "should use Error to report
>> errors". HACKING explains:
>>
>> Use the simplest suitable method to communicate success / failure to
>> callers. Stick to common methods: non-negative on success / -1 on
>> error, non-negative / -errno, non-null / null, or Error objects.
>>
>> Example: when a function returns a non-null pointer on success, and it
>> can fail only in one way (as far as the caller is concerned), returning
>> null on failure is just fine, and certainly simpler and a lot easier on
>> the eyes than propagating an Error object through an Error ** parameter.
>>
>> Example: when a function's callers need to report details on failure
>> only the function really knows, use Error **, and set suitable errors.
>>
>> Do not report an error to the user when you're also returning an error
>> for somebody else to handle. Leave the reporting to the place that
>> consumes the error returned.
>>
>> As we'll see in your patch of msi.c, msi_init() can fail in multiple
>> ways, and uses -errno to comunicate them. That can be okay even in
>> realize(). It also reports to the user. That's what makes it
>> unsuitable for realize().
>>
>>> Also modify the callers
>>
>> Actually, you're *fixing* callers! But the bugs aren't 100% clear, yet,
>> see below for details. Once we know what the bugs are, we'll want to
>> reword the commit message to describe the bugs and their impact.
>>
>> I recommend to skip ahead to msi.c, then come back to the device models.
>>
>>> Bonus: add more comment for msi_init().
>>> Signed-off-by: Cao jin <address@hidden>
>>> ---
>>> hw/audio/intel-hda.c | 10 ++++-
>>> hw/ide/ich.c | 2 +-
>>> hw/net/vmxnet3.c | 13 +++---
>>> hw/pci-bridge/ioh3420.c | 7 +++-
>>> hw/pci-bridge/pci_bridge_dev.c | 8 +++-
>>> hw/pci-bridge/xio3130_downstream.c | 8 +++-
>>> hw/pci-bridge/xio3130_upstream.c | 8 +++-
>>> hw/pci/msi.c | 18 +++++++--
>>> hw/scsi/megasas.c | 15 +++++--
>>> hw/scsi/vmw_pvscsi.c | 13 ++++--
>>> hw/usb/hcd-xhci.c | 81
>>> +++++++++++++++++++++-----------------
>>> hw/vfio/pci.c | 20 +++++-----
>>> include/hw/pci/msi.h | 4 +-
>>> 13 files changed, 135 insertions(+), 72 deletions(-)
>>>
>
> [...]
>
>> Except I'm not sure the function should fail in the first place! See
>> below.
>>
>>> +int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int
>>> nr_vectors,
>>> + bool msi64bit, bool msi_per_vector_mask, Error **errp)
>>> {
>>> unsigned int vectors_order;
>>> - uint16_t flags;
>>> + uint16_t flags; /* Message Control register value */
>>> uint8_t cap_size;
>>> int config_offset;
>>>
>>> if (!msi_supported) {
>>> + error_setg(errp, "MSI is not supported by interrupt controller");
>>> return -ENOTSUP;
>>
>> First failure mode: board does not support MSI (-ENOTSUP).
>>
>> Question to the PCI guys: why is this even an error? A device with
>> capability MSI should work just fine in such a board.
>
> Hi Markus,
>
> Adding Jan Kiszka, maybe he can help.
>
> That's a fair question. Is there any history for this decision?
> The board not supporting MSI has nothing to do with the capability being
> there.
> The HW should not change because the board doe not support it.
>
> The capability should be present but not active.
>
>>
>>> }
>>>
>>> @@ -182,7 +190,8 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
>>> }
>>>
>>> cap_size = msi_cap_sizeof(flags);
>>> - config_offset = pci_add_capability(dev, PCI_CAP_ID_MSI, offset,
>>> cap_size);
>>> + config_offset = pci_add_capability2(dev, PCI_CAP_ID_MSI, offset,
>>> + cap_size, errp);
>>
>> pci_add_capability() is a wrapper around pci_add_capability2() that
>> additionally reports errors with error_report_err(). This is what makes
>> it unsuitable for realize().
>>
>>> if (config_offset < 0) {
>>> return config_offset;
>>
>> Inherits failing modes from pci_add_capability2(). Two: out of PCI
>> config space (-ENOSPC), and capability overlap (-EINVAL).
>>
>> Question for the PCI guys: how can these happen? When they happen, is
>> it a programming error?
>
> out of PCI config space: a device emulation error, not enough room
> for all its capabilities - it seems to be a programming error.
Programming error should be an assertion failure, not falling to a
variant of the device the user didn't order and that might not even
exist in the real world.
> capability overlap: is for device assignment. This checks for a real HW
> that is broke. - not a programming error.
Okay.
Thanks!
[...]
- Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers, (continued)
- Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers, Marcel Apfelbaum, 2016/03/03
- Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers, Michael S. Tsirkin, 2016/03/03
- Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers, Marcel Apfelbaum, 2016/03/03
- Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers, Michael S. Tsirkin, 2016/03/03
- Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers, Markus Armbruster, 2016/03/03
- Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers, Michael S. Tsirkin, 2016/03/03
- Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers, Markus Armbruster, 2016/03/04
- Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers, Michael S. Tsirkin, 2016/03/04
- Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers, Markus Armbruster, 2016/03/04
- Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers, Michael S. Tsirkin, 2016/03/04
Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers,
Markus Armbruster <=
Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers, Cao jin, 2016/03/23