| Message ID | 1452074233-25803-1-git-send-email-clg@fr.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, 6 Jan 2016 10:57:13 +0100 Cédric Le Goater <clg@fr.ibm.com> wrote: > The buffer size used by the BT driver defaults to 64bytes even when > the BMC reports a different size. This patch removes the restriction > as there is no apparent reason for it. > The only reason it was written like this was to avoid breaking anything, I like this change especially if we have things out there that can make use of bigger buffers. > The extra byte added on the input and output buffer size in the test > also seems superfluous. The buffer lengths used later in the code are > correct as they take into account the extra byte needed for the > message size : > > bt.caps.input_buf_len = msg->data[1] + 1; > bt.caps.output_buf_len = msg->data[2] + 1; > > > From IPMI Specs: > > 22.10 Get BT Interface Capabilities Command > > For Bytes 3 and 4 (Input and Output Buffer size), the buffer > message size is the largest value allowed in first byte (length > field) of any BT request or response message. For a send, this > means if Get BT Interface Capabilities returns 255 in byte 3 > (input buffer size) the driver can actually write 256 bytes to the > input buffer (adding one for the length byte (byte 1) that is sent > in with the request.) > > > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> > --- > > Tested on palmetto which uses a 64bytes fifo and qemu which uses > 255bytes. > > hw/bt.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > Index: skiboot.git/hw/bt.c > =================================================================== > --- skiboot.git.orig/hw/bt.c > +++ skiboot.git/hw/bt.c > @@ -164,16 +164,14 @@ static void get_bt_caps_complete(struct > goto out; > } > > - if (msg->data[1] + 1 != BT_FIFO_LEN) { > + if (msg->data[1] != BT_FIFO_LEN) { The aspeed2400 doc (I can't seem to find where I got it from... sorry) states that the hardware is 64 bytes so i would expect the BMC to return 63 here? Admittedly I never actually checked what the BMC returns, have you observed something different from BMC firmware? > BT_DBG("Got a input buffer len (%u) cap which differs from the default\n", > msg->data[1]); > - goto out; > } > > - if (msg->data[2] + 1 != BT_FIFO_LEN) { > + if (msg->data[2] != BT_FIFO_LEN) { > BT_DBG("Got a output buffer len (%u) cap which differs from the default\n", > msg->data[2]); > - goto out; > } > > /* > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
On 01/11/2016 07:09 AM, Cyril Bur wrote: > On Wed, 6 Jan 2016 10:57:13 +0100 > Cédric Le Goater <clg@fr.ibm.com> wrote: > >> The buffer size used by the BT driver defaults to 64bytes even when >> the BMC reports a different size. This patch removes the restriction >> as there is no apparent reason for it. >> > > The only reason it was written like this was to avoid breaking anything, I like > this change especially if we have things out there that can make use of bigger > buffers. Now that the IMPI patchset from Corey has been merged, you can try it out with qemu, which uses 255 bytes buffer size. If you feel adventurous, here is my dev branch which contains a merge of Ben's patchset on top of qemu's HEAD an a serie of mine : https://github.com/legoater/qemu/commits/powernv-ipmi run as usual : qemu-system-ppc64 -m 2G -M powernv -device ipmi-bmc-sim,id=bmc0 -device isa-ipmi-bt,bmc=bmc0,irq=10 -nographic -nodefaults -monitor stdio -serial pty -snapshot -S -bios skiboot.lid -kernel zImage.epapr -initrd rootfs.cpio.xz skiboot should contain some enablement for qemu. Here is a patch : https://github.com/legoater/skiboot/commit/16c1317c8c95b8429ee82fbf3a6d3c9876e7208e >> The extra byte added on the input and output buffer size in the test >> also seems superfluous. The buffer lengths used later in the code are >> correct as they take into account the extra byte needed for the >> message size : >> >> bt.caps.input_buf_len = msg->data[1] + 1; >> bt.caps.output_buf_len = msg->data[2] + 1; >> >> >> From IPMI Specs: >> >> 22.10 Get BT Interface Capabilities Command >> >> For Bytes 3 and 4 (Input and Output Buffer size), the buffer >> message size is the largest value allowed in first byte (length >> field) of any BT request or response message. For a send, this >> means if Get BT Interface Capabilities returns 255 in byte 3 >> (input buffer size) the driver can actually write 256 bytes to the >> input buffer (adding one for the length byte (byte 1) that is sent >> in with the request.) >> >> >> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> >> --- >> >> Tested on palmetto which uses a 64bytes fifo and qemu which uses >> 255bytes. >> >> hw/bt.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> Index: skiboot.git/hw/bt.c >> =================================================================== >> --- skiboot.git.orig/hw/bt.c >> +++ skiboot.git/hw/bt.c >> @@ -164,16 +164,14 @@ static void get_bt_caps_complete(struct >> goto out; >> } >> >> - if (msg->data[1] + 1 != BT_FIFO_LEN) { >> + if (msg->data[1] != BT_FIFO_LEN) { > > The aspeed2400 doc (I can't seem to find where I got it from... sorry) states > that the hardware is 64 bytes so i would expect the BMC to return 63 here? > Admittedly I never actually checked what the BMC returns, have you observed > something different from BMC firmware? We get 64 bytes from the BMC (palmetto and habanero). Here are the logs with this patch : [1055047556,7] BT: seq 0x00 netfn 0x18 cmd 0x36: IPMI MSG done [1055050249,7] BT: BMC BT capabilities received: [1055051990,7] BT: buffer sizes: 65 input 65 output [1055054016,7] BT: number of requests: 2 [1055055689,7] BT: msg timeout: 2 max retries: 1 >> BT_DBG("Got a input buffer len (%u) cap which differs from the default\n", >> msg->data[1]); >> - goto out; >> } >> >> - if (msg->data[2] + 1 != BT_FIFO_LEN) { >> + if (msg->data[2] != BT_FIFO_LEN) { >> BT_DBG("Got a output buffer len (%u) cap which differs from the default\n", >> msg->data[2]); >> - goto out; >> } >> >> /* >> >> _______________________________________________ >> Skiboot mailing list >> Skiboot@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/skiboot >
Cédric Le Goater <clg@fr.ibm.com> writes: > The buffer size used by the BT driver defaults to 64bytes even when > the BMC reports a different size. This patch removes the restriction > as there is no apparent reason for it. > > The extra byte added on the input and output buffer size in the test > also seems superfluous. The buffer lengths used later in the code are > correct as they take into account the extra byte needed for the > message size : > > bt.caps.input_buf_len = msg->data[1] + 1; > bt.caps.output_buf_len = msg->data[2] + 1; > > > From IPMI Specs: > > 22.10 Get BT Interface Capabilities Command > > For Bytes 3 and 4 (Input and Output Buffer size), the buffer > message size is the largest value allowed in first byte (length > field) of any BT request or response message. For a send, this > means if Get BT Interface Capabilities returns 255 in byte 3 > (input buffer size) the driver can actually write 256 bytes to the > input buffer (adding one for the length byte (byte 1) that is sent > in with the request.) > > > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> > --- > > Tested on palmetto which uses a 64bytes fifo and qemu which uses > 255bytes. > > hw/bt.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) After having read even more of the IPMI spec than I ever thought I'd be doing, and testing on habanero, looks good and merged to master as of 4eb9807.
Index: skiboot.git/hw/bt.c =================================================================== --- skiboot.git.orig/hw/bt.c +++ skiboot.git/hw/bt.c @@ -164,16 +164,14 @@ static void get_bt_caps_complete(struct goto out; } - if (msg->data[1] + 1 != BT_FIFO_LEN) { + if (msg->data[1] != BT_FIFO_LEN) { BT_DBG("Got a input buffer len (%u) cap which differs from the default\n", msg->data[1]); - goto out; } - if (msg->data[2] + 1 != BT_FIFO_LEN) { + if (msg->data[2] != BT_FIFO_LEN) { BT_DBG("Got a output buffer len (%u) cap which differs from the default\n", msg->data[2]); - goto out; } /*
The buffer size used by the BT driver defaults to 64bytes even when the BMC reports a different size. This patch removes the restriction as there is no apparent reason for it. The extra byte added on the input and output buffer size in the test also seems superfluous. The buffer lengths used later in the code are correct as they take into account the extra byte needed for the message size : bt.caps.input_buf_len = msg->data[1] + 1; bt.caps.output_buf_len = msg->data[2] + 1; From IPMI Specs: 22.10 Get BT Interface Capabilities Command For Bytes 3 and 4 (Input and Output Buffer size), the buffer message size is the largest value allowed in first byte (length field) of any BT request or response message. For a send, this means if Get BT Interface Capabilities returns 255 in byte 3 (input buffer size) the driver can actually write 256 bytes to the input buffer (adding one for the length byte (byte 1) that is sent in with the request.) Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> --- Tested on palmetto which uses a 64bytes fifo and qemu which uses 255bytes. hw/bt.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)