[Pdbg] [PATCH] libpdbg/kernel: Fix FSI address masking

[Reza Arbab]

On Tue, Aug 11, 2020 at 06:04:36PM +1000, Amitay Isaacs wrote:
>addr64 = 0x100c09
>addr = (0x100c09 & 0x7fc00) | ((0x100c09 & 0x3ff) << 2)
>     = 0xc00 | 0x24
>     = 0xc24  (should be 0x100c24)

That is the intent of the patch. The addr we are computing above gets 
used with the raw sysfs device. So which of the following is correct?

     lseek(fd="/sys/class/fsi-master/fsi0/slave at 01:00/raw", addr=0x100c24)
     lseek(fd="/sys/class/fsi-master/fsi0/slave at 01:00/raw", addr=0xc24)

I would argue for the latter. Since fd already specifies link 1, the 
address should be relative to that. This is the kernel call chain for 
the bad case:

     fsi_slave_sysfs_raw_read
         fsi_slave_read
             fsi_master_read
                 master->read(link=1, slave_id=0, addr=0x100c24)

I think we haven't noticed this before because link=1 may be a new 
scenario. For instance, the fsi-master-gpio driver (used by AST2500 BMC) 
explicitly does not support it:

     fsi_master_gpio_read(link=1, id=0, addr=0x100c24)
         if (link != 0)
             return -ENODEV;

Since this master only supports link=0, it works without needing to mask 
off the link bits of addr. Those bits are already zero.

I noticed because I'm working on a fsi-master-ppc driver (on-chip 
hMFSI), which will actually get used with link=1:

     fsi_master_ppc_read(link=1, id=0, addr=0x100c24)
         opb_addr = base + (link * 0x80000) + (id * 0x20000) + addr;
                  = 0x80000 + (1 * 0x80000) + (0 * 0x20000) + 0x100c24;
                  = 0x200c24 /* wrong */

It should be

     fsi_master_ppc_read(link=1, id=0, addr=0xc24)
         opb_addr = base + (link * 0x80000) + (id * 0x20000) + addr;
                  = 0x80000 + (1 * 0x80000) + (0 * 0x20000) + 0xc24;
                  = 0x100c24

I guess I'll also mask addr in my kernel code, as a safeguard for this, 
but it seems like libpdbg should be doing the right thing too.

>I am assuming this is same for both P9 and P10.  Or is it supposed to
>work for P10 only with modified mask?

Should be the same for p9 and p10, afaik.

-- 
Reza Arbab