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

[Amitay Isaacs]

Hi Reza,

On Tue, 2020-08-11 at 14:27 -0500, Reza Arbab wrote:
> 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)

Ok. I understand the motivation.

This logic should only be applied if we are using fsi device other than
fsi0 (primary fsi).  If I enable hmfsi driver (instead of kernel
driver) for secondary fsi devices, then any fsi_read() on them
translates to fsi_read() on primary fsi device.  Now all those
addresses need to be preserved, otherwise all fsi devices get mapped to
the primary fsi device and that's clearly wrong.

> 
> 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.

Sure. As I explained above, to make hmfsi driver work in libpdbg, the
mask needs to be applied only to secondary fsi devices.

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

Amitay.
-- 

When you have been wronged, a poor memory is your best response.