Re: [Qemu-block] [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers

[Marcel Apfelbaum]

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.

capability overlap: is for device assignment. This checks for a real HW
that is broke. - not a programming error.

> >       }
> > @@ -205,6 +214,7 @@ int msi_init(struct PCIDevice *dev, uint8_t offset,
> >           pci_set_long(dev->wmask + msi_mask_off(dev, msi64bit),
> >                        0xffffffff >> (PCI_MSI_VECTORS_MAX - nr_vectors));
> >       }
> > +
> >       return config_offset;
> >   }


Thanks,
Marcel


[...]