| Message ID | 1594242521-14281-2-git-send-email-arbab@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | libpdbg/kernel: Fix FSI address masking | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-build | success | Successfully ran 1 jobs. |
| snowpatch_ozlabs/github-test | success | Successfully ran 1 jobs. |
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.
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.
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 --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) {
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(-)