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

[Markus Armbruster]

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?

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);