Re: [Qemu-devel] [PATCH v2 1/5] msix_init: assert programming error

[Cao jin]

On 09/12/2016 09:29 PM, Markus Armbruster wrote:
> Cao jin <address@hidden> writes:
>
> > The input parameters is used for creating the msix capable device, so
> > they must obey the PCI spec, or else, it should be programming error.
>
> True when the the parameters come from a device model attempting to
> define a PCI device violating the spec.  But what if the parameters come
> from an actual PCI device violating the spec, via device assignment?

Before the patch, on invalid param, the vfio behaviour is:
  error_report("vfio: msix_init failed");
  then, device create fail.

After the patch, its behaviour is:
  asserted.

Do you mean we should still report some useful info to user on invalid 
params?

Cao jin
> For what it's worth, the new behavior seems consistent with msi_init(),
> which is good.
>
> > CC: Markus Armbruster <address@hidden>
> > CC: Marcel Apfelbaum <address@hidden>
> > CC: Michael S. Tsirkin <address@hidden>
> > Signed-off-by: Cao jin <address@hidden>
> > ---
> >   hw/pci/msix.c | 6 ++----
> >   1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> > index 0ec1cb1..384a29d 100644
> > --- a/hw/pci/msix.c
> > +++ b/hw/pci/msix.c
> > @@ -253,9 +253,7 @@ int msix_init(struct PCIDevice *dev, unsigned short 
> > nentries,
> >           return -ENOTSUP;
> >       }
> >
> > -    if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) {
> > -        return -EINVAL;
> > -    }
> > +    assert(nentries >= 1 && nentries <= PCI_MSIX_FLAGS_QSIZE + 1);
> >
> >       table_size = nentries * PCI_MSIX_ENTRY_SIZE;
> >       pba_size = QEMU_ALIGN_UP(nentries, 64) / 8;
> > @@ -266,7 +264,7 @@ int msix_init(struct PCIDevice *dev, unsigned short 
> > nentries,
>         /* Sanity test: table & pba don't overlap, fit within BARs, min aligned 
> */
>         if ((table_bar_nr == pba_bar_nr &&
>              ranges_overlap(table_offset, table_size, pba_offset, pba_size)) ||
> >           table_offset + table_size > memory_region_size(table_bar) ||
> >           pba_offset + pba_size > memory_region_size(pba_bar) ||
> >           (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) {
> > -        return -EINVAL;
> > +        assert(0);
> >       }
>
> Instead of
>
>      if (... complicated condition ...) {
>          assert(0);
>      }
>
> let's write
>
>      assert(... negation of the complicated condition ...);
>
> >       cap = pci_add_capability(dev, PCI_CAP_ID_MSIX, cap_pos, MSIX_CAP_LENGTH);
>
>
> .