Re: [PATCH] Fix Vector table and PBA addresses print output

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 30 Jan 2018 16:08:07 +0000
Gustavo Pimentel <gustavo.pimentel@synopsys.com> wrote:

> Hi Alex,
> 
> On 30/01/2018 14:07, Alex Williamson wrote:
> > On Tue, 30 Jan 2018 10:34:08 +0000
> > Gustavo Pimentel <gustavo.pimentel@synopsys.com> wrote:
> >   
> >> Lack of 3-bit rotation to the right was leading to erroneous display of the
> >> vector table and PBA.
> >>
> >> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> >> ---
> >>  ls-caps.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/ls-caps.c b/ls-caps.c
> >> index 507771a..cd13a37 100644
> >> --- a/ls-caps.c
> >> +++ b/ls-caps.c
> >> @@ -1104,10 +1104,10 @@ cap_msix(struct device *d, int where, int cap)
> >>  
> >>    off = get_conf_long(d, where + PCI_MSIX_TABLE);
> >>    printf("\t\tVector table: BAR=%d offset=%08x\n",
> >> -	 off & PCI_MSIX_BIR, off & ~PCI_MSIX_BIR);
> >> +	 off & PCI_MSIX_BIR, (off & ~PCI_MSIX_BIR) >> 3);
> >>    off = get_conf_long(d, where + PCI_MSIX_PBA);
> >>    printf("\t\tPBA: BAR=%d offset=%08x\n",
> >> -	 off & PCI_MSIX_BIR, off & ~PCI_MSIX_BIR);
> >> +	 off & PCI_MSIX_BIR, (off & ~PCI_MSIX_BIR) >> 3);
> >>  }
> >>  
> >>  static void  
> > 
> > 
> > The code is correct as is, the register contains bits 31:3 of the
> > offset.  The spec states (PCI rev 3.0, 6.8.2.4, 6.8.2.5):  
> 
> I have just downloaded now the rev 3.0 (PCI_Express_Base_r3.0_10Nov10.pdf,
> November 10, 2010) from PCI-SIG and the chapters that you refer don't match with
> the downloaded version, can you confirm the doc date?

I'm referencing the PCI Local Bus Specification Rev 3.0, Feb 3, 2004.
MSI-X is compatible between PCI and PCIe, in fact my copy of the PCIe
3.1a spec refers to the PCI spec for these registers.

> > 
> >   The lower 3 Table BIR bits are masked off (set to zero) by software
> >   to form a 32-bit QWORD -aligned offset.
> > 
> >   The lower 3 PBA BIR bits are masked off (set to zero) by software to
> >   form a 32-bit QWORD-aligned offset.
> >  
> 
> Yes, I understand the meaning but for the user the value printed should not
> include (in my perspective) the offset provide by the Table BIR field or the PBA
> BIR field.

The offset is the value with the BIR field masked, shifting the value
results in the (offset/8), which is not a useful thing.
 
> > Shifting is incorrect, the mask operation fills the lower 3 bits.  
> The spec defines:
>        | 31         24 | 23         16 | 15         8 | 7        3 | 2     0 |
>   +00h |           Msg Ctrl Reg        |    PNext ID  |       CapID          |
>   +04h |                        Table offset                       | Tb BIR  |
>   +08h |                        PBA offset                         | PBA BIR |
>   +0Ch |            Reserved           |         Msg Data Reg                |
> 
> Let's apply a real example here, using the info from config space:
>        | 31         24 | 23         16 | 15         8 | 7                  0 |
> 0b+00h |      00       |      20       |      00      |         11           |
> 0b+04h |      00       |      00       |      00      |         00           |
> 0b+08h |      00       |      00       |      18      |         00           |
> 0b+0Ch |      00       |      00       |      00      |         00           |
> 
> If I decode manually the info from config space, I get for each field the
> following value:
> 
> Msg Ctrl Reg = 0x20
> PNext ID = 0x0
> CapID = 0x11
> Table offset = 0x0
> Table BIR = 0x0
> Table offset = 0x300

Correction, Table offset[31:3] = 0x300, the actual offset is 0x1800

> Table BIR = 0x0
> Msg Data Reg = 0x0
> 
> Using lspci, I get:
> ...
>     Capabilities: [b0] MSI-X: Enable+ Count=33 Masked-
>         Vector table: BAR=0 offset=00000000
>         PBA: BAR=0 offset=00001800

Which is correct

> ...
> In my perspective should be like:
> 
>     Capabilities: [b0] MSI-X: Enable+ Count=33 Masked-
>         Vector table: BAR=0 offset=00000000
>         PBA: BAR=0 offset=00000300

Which is a meaningless and misleading value.  The PBA lives at offset
0x1800 into BAR0 MMIO space, why is anything other than that relevant
to present to the user?  By your logic we should also present
count as 32 and let the user figure out that it's encoded as N-1.
Thanks,

Alex