diff mbox series

libpdbg/kernel: Fix FSI address masking

Message ID 1594242521-14281-2-git-send-email-arbab@linux.ibm.com
State New
Headers show
Series libpdbg/kernel: Fix FSI address masking | expand

Commit Message

Reza Arbab July 8, 2020, 9:08 p.m. UTC
With this backend, the FSI master port we're using is implicit, e.g.
/sys/class/fsi-master/fsi0/slave@01:00/raw is port 1, and target addresses
should be relative to that. Mask the port from the address accordingly.

Signed-off-by: Reza Arbab <arbab@linux.ibm.com>
---
 libpdbg/kernel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Amitay Isaacs Aug. 11, 2020, 8:04 a.m. UTC | #1
On Wed, 2020-07-08 at 16:08 -0500, Reza Arbab wrote:
> With this backend, the FSI master port we're using is implicit, e.g.
> /sys/class/fsi-master/fsi0/slave@01:00/raw is port 1, and target
> addresses
> should be relative to that. Mask the port from the address
> accordingly.
> 
> Signed-off-by: Reza Arbab <arbab@linux.ibm.com>
> ---
>  libpdbg/kernel.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libpdbg/kernel.c b/libpdbg/kernel.c
> index b2b977bc6c96..18d54a35d4a9 100644
> --- a/libpdbg/kernel.c
> +++ b/libpdbg/kernel.c
> @@ -63,7 +63,7 @@ const char *kernel_get_fsi_path(void)
>  static int kernel_fsi_getcfam(struct fsi *fsi, uint32_t addr64,
> uint32_t *value)
>  {
>  	int rc;
> -	uint32_t tmp, addr = (addr64 & 0x7ffc00) | ((addr64 & 0x3ff) <<
> 2);
> +	uint32_t tmp, addr = (addr64 & 0x7fc00) | ((addr64 & 0x3ff) <<
> 2);

This looks definitely wrong.

addr64 = 0xc09
addr = (0xc09 & 0x7fc00) | ((0xc09 & 0x3ff) << 2)
     = 0xc00 | 0x24
     = 0xc24

addr64 = 0x100c09
addr = (0x100c09 & 0x7fc00) | ((0x100c09 & 0x3ff) << 2)
     = 0xc00 | 0x24
     = 0xc24  (should be 0x100c24)

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

>  
>  	rc = lseek(fsi->fd, addr, SEEK_SET);
>  	if (rc < 0) {
> @@ -90,7 +90,7 @@ static int kernel_fsi_getcfam(struct fsi *fsi,
> uint32_t addr64, uint32_t *value)
>  static int kernel_fsi_putcfam(struct fsi *fsi, uint32_t addr64,
> uint32_t data)
>  {
>  	int rc;
> -	uint32_t tmp, addr = (addr64 & 0x7ffc00) | ((addr64 & 0x3ff) <<
> 2);
> +	uint32_t tmp, addr = (addr64 & 0x7fc00) | ((addr64 & 0x3ff) <<
> 2);
>  
>  	rc = lseek(fsi->fd, addr, SEEK_SET);
>  	if (rc < 0) {
> -- 
> 2.17.1
> 

Amitay.
Reza Arbab Aug. 11, 2020, 7:27 p.m. UTC | #2
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@01:00/raw", addr=0x100c24)
     lseek(fd="/sys/class/fsi-master/fsi0/slave@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.
Amitay Isaacs Aug. 12, 2020, 3:48 a.m. UTC | #3
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@01:00/raw",
> addr=0x100c24)
>      lseek(fd="/sys/class/fsi-master/fsi0/slave@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.
diff mbox series

Patch

diff --git a/libpdbg/kernel.c b/libpdbg/kernel.c
index b2b977bc6c96..18d54a35d4a9 100644
--- a/libpdbg/kernel.c
+++ b/libpdbg/kernel.c
@@ -63,7 +63,7 @@  const char *kernel_get_fsi_path(void)
 static int kernel_fsi_getcfam(struct fsi *fsi, uint32_t addr64, uint32_t *value)
 {
 	int rc;
-	uint32_t tmp, addr = (addr64 & 0x7ffc00) | ((addr64 & 0x3ff) << 2);
+	uint32_t tmp, addr = (addr64 & 0x7fc00) | ((addr64 & 0x3ff) << 2);
 
 	rc = lseek(fsi->fd, addr, SEEK_SET);
 	if (rc < 0) {
@@ -90,7 +90,7 @@  static int kernel_fsi_getcfam(struct fsi *fsi, uint32_t addr64, uint32_t *value)
 static int kernel_fsi_putcfam(struct fsi *fsi, uint32_t addr64, uint32_t data)
 {
 	int rc;
-	uint32_t tmp, addr = (addr64 & 0x7ffc00) | ((addr64 & 0x3ff) << 2);
+	uint32_t tmp, addr = (addr64 & 0x7fc00) | ((addr64 & 0x3ff) << 2);
 
 	rc = lseek(fsi->fd, addr, SEEK_SET);
 	if (rc < 0) {