Re: [Qemu-devel] [PATCH RFC] msix_init: input params *_offset isn't the real one

[Marcel Apfelbaum]

On 08/10/2016 06:18 AM, Cao jin wrote:
> The parameter table_offset & pba_offset is kind of confusing, they shouldn't
> include bir field.
>
> Signed-off-by: Cao jin <address@hidden>
> ---

Hi,

> According to the passed arguments, I guess all the callers of msix_init()
> has the same feeling with me, but I am not quite sure about this, so, RFC.
>
>  hw/pci/msix.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 0ec1cb1..3a16d83 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -264,8 +264,7 @@ int msix_init(struct PCIDevice *dev, unsigned short 
> nentries,
>      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) {
> +        pba_offset + pba_size > memory_region_size(pba_bar)) {
>          return -EINVAL;

I think we should keep the '(table_offset | pba_offset) & 
PCI_MSIX_FLAGS_BIRMASK)'
test since it is required by spec, please see: PCI Spec "6.8.2.4. Table Offset/Table 
BIR for MSI-X"

 Table offset: ...The lower 3 Table BIR bits are masked off (set to zero) by 
software
               to form a 32-bit QWORD -aligned offset.

This function gets the offset parameters as the whole 32-BIT QWORD and checks 
it does not collide
with the BIR offset.


>      }
>
> @@ -282,8 +281,8 @@ int msix_init(struct PCIDevice *dev, unsigned short 
> nentries,
>      dev->msix_entries_nr = nentries;
>      dev->msix_function_masked = true;
>
> -    pci_set_long(config + PCI_MSIX_TABLE, table_offset | table_bar_nr);
> -    pci_set_long(config + PCI_MSIX_PBA, pba_offset | pba_bar_nr);
> +    pci_set_long(config + PCI_MSIX_TABLE, (table_offset << 3) | table_bar_nr);
> +    pci_set_long(config + PCI_MSIX_PBA, (pba_offset << 3) | pba_bar_nr);

Here is a similar issue. Your interpretation suggests we need to shift left the 
offset
to make room for BIR, but I think current implementation looks at it 
differently already
receiving the offset as a 32-bit QWORD and simply does not "look" to the lower 
bits
implying them 0.

Thanks,
Marcel

>      /* Make flags bit writable. */
>      dev->wmask[cap + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |