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

[Cao jin]

On 08/18/2016 06:54 PM, Marcel Apfelbaum wrote:
> 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.

Hi Marcel,
    Thanks very much for pointing the accurate reference out.
    I also checked how kernel code handle this field, it is just like 
you said.

    Sorry for this noise.

    Cao jin
> >      }
> >
> > @@ -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