Re: [Qemu-devel] [PATCH 2/6] pci: Convert msix_init() to Error and fix callers to check it

[Cao jin]

On 08/18/2016 03:55 PM, Markus Armbruster wrote:
> Cao jin <address@hidden> writes:

> > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> > index 40a2ebc..b8511ee 100644
> > --- a/hw/misc/ivshmem.c
> > +++ b/hw/misc/ivshmem.c
> > @@ -899,6 +899,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error 
> > **errp)
> >
> >           if (ivshmem_setup_interrupts(s) < 0) {
> >               error_setg(errp, "failed to initialize interrupts");
> > +            error_append_hint(errp, "MSI-X is not supported by interrupt 
> > controller");
> >               return;
> >           }
> >       }
>
> How is this related to the stated purpose of the patch?

Because I don't propagate error via msix_init_exclusive_bar(), so add 
this to show the detail error message to user.(Please also see my 
comment on msix_init_exclusive_bar(), then come back to this comment)

> > diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> > index d001c96..82a7be1 100644
> > --- a/hw/net/e1000e.c
> > +++ b/hw/net/e1000e.c
> > @@ -295,7 +295,7 @@ e1000e_init_msix(E1000EState *s)
>         int res = msix_init(PCI_DEVICE(s), E1000E_MSIX_VEC_NUM,
>                             &s->msix,
> >                           E1000E_MSIX_IDX, E1000E_MSIX_TABLE,
> >                           &s->msix,
> >                           E1000E_MSIX_IDX, E1000E_MSIX_PBA,
> > -                        0xA0);
> > +                        0xA0, NULL);
>
> Further down, you convert msix_init() from error_report() to
> error_setg().  It now relies on its callers to report errors.  However,
> this caller doesn't.  Suppressing error reports like that may be
> appropriate, since the function doesn't fail.  But such a change
> shouldn't be hidden in a larger patch without at least a mention in the
> commit message.

For this device, I was planning 1)make this patch as small as possible 
for reviewer's convenience sake(so suppressed error report here), then 
2) drop this function as another patch, and then 3) convert this device 
to .realize(), error report or setting error could be placed at one of 
last two steps. For other devices, the plan is basically the same.

> Here's how I'd skin this cat.  First convert msix_init() without
> changing behavior, by having every caller of msix_init() immediately
> pass the error received to error_report_err().  Then clean up the
> callers one after the other.  The ones that are running within a
> realize() method should propagate the error to the realize() method.
> The ones that are still running within an init() method keep the
> error_report_err().  e1000e_init_msix() may want to ignore the error.
> Separaring the cleanups that way lets you explain each actual change
> neatly in a commit message.
>
> >       if (res < 0) {
> >           trace_e1000e_msix_init_fail(res);
> > diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
> > index 30f2ce4..769b1fd 100644
> > --- a/hw/net/rocker/rocker.c
> > +++ b/hw/net/rocker/rocker.c
> > @@ -1262,7 +1262,7 @@ static int rocker_msix_init(Rocker *r)
>         err = msix_init(dev, ROCKER_MSIX_VEC_COUNT(r->fp_ports),
>                         &r->msix_bar,
> >                       ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_TABLE_OFFSET,
> >                       &r->msix_bar,
> >                       ROCKER_PCI_MSIX_BAR_IDX, ROCKER_PCI_MSIX_PBA_OFFSET,
> > -                    0);
> > +                    0, NULL);
> >       if (err) {
> >           return err;
> >       }
>
> This one runs in an init() method.  You're losing an error message here.

Indeed... planned to propagate or set error object when convert the 
device to realize because the only failure is -ENOTSUP

> > @@ -336,7 +340,7 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short 
> > nentries,
> >       ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr,
> >                       0, &dev->msix_exclusive_bar,
> >                       bar_nr, bar_pba_offset,
> > -                    0);
> > +                    0, NULL);
> >       if (ret) {
> >           return ret;
> >       }
>
> This is a case where you have to propagate the error.  First step:
> convert msix_exclusive_bar() to Error, too.

I was thinking: all devices call msix_init_exclusive_bar() will only see 
-ENOTSUP on failure, so, to make this patch short and simple, we could 
set error or report error in the caller(or caller's caller, that is what 
I have done in ivshmem_common_realize() at the top of this patch).

> > @@ -445,8 +449,10 @@ void msix_notify(PCIDevice *dev, unsigned vector)
> >   {
> >       MSIMessage msg;
> >
> > -    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
> > +    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector]) {
> >           return;
> > +    }
> > +
> >       if (msix_is_masked(dev, vector)) {
> >           msix_set_pending(dev, vector);
> >           return;
>
> Let's drop this hunk.

Sorry, I forget to make the coding style changes into a separated one.

--
Yours Sincerely,

Cao jin