| Message ID | 20210716133915.14697-2-eajames@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi: fsi: Reduce max transfer size to 8 bytes | expand |
On Fri, Jul 16, 2021 at 08:39:14AM -0500, Eddie James wrote: > Security changes have forced the SPI controllers to be limited to > 8 byte reads. Refactor the sequencing to just handle 8 bytes at a > time. Which security changes where - somewhere else in Linux?
On Fri, 2021-07-16 at 18:19 +0100, Mark Brown wrote: > On Fri, Jul 16, 2021 at 08:39:14AM -0500, Eddie James wrote: > > Security changes have forced the SPI controllers to be limited to > > 8 byte reads. Refactor the sequencing to just handle 8 bytes at a > > time. Security changes in the SPI controller - in the device microcode. I can reword the commit if you like. Thanks, Eddie > > Which security changes where - somewhere else in Linux?
On Fri, Jul 16, 2021 at 01:34:38PM -0500, Eddie James wrote: > Security changes in the SPI controller - in the device microcode. I can > reword the commit if you like. How will people end up running this device microcode? Is this a bug fix, or is this going to needlessly reduce performance for people with existing hardware?
On Mon, 2021-07-19 at 16:20 +0100, Mark Brown wrote: > On Fri, Jul 16, 2021 at 01:34:38PM -0500, Eddie James wrote: > > > Security changes in the SPI controller - in the device microcode. I > > can > > reword the commit if you like. > > How will people end up running this device microcode? Is this a bug > fix, or is this going to needlessly reduce performance for people > with > existing hardware? The hardware is still in development. As part of the development, the device microcode was changed to restrict transfers. The reason for this restriction was "security concerns". This restriction disallows the loop (or branch-if-not-equal-and-increment) sequencer command. It also does not allow the read (or shift in if you prefer) command to specify the number of bytes in the command itself. Rather, the number of bits to shift in must be specified in a separate control register. This effectively means that the controller cannot transfer more than 8 bytes at a time. Therefore I suppose this is effectively a bug fix. There will be no hardware available without these restrictions, so it is not a needless reduction in performance. Every system that can run this driver will run the more restrictive device microcode. Thanks, Eddie
From: Eddie James <eajames@linux.ibm.com> > Sent: 19 July 2021 16:47 > To: Mark Brown <broonie@kernel.org> > Cc: devicetree@vger.kernel.org; openbmc@lists.ozlabs.org; robh+dt@kernel.org; linux- > kernel@vger.kernel.org; linux-spi@vger.kernel.org > Subject: Re: [PATCH 1/2] spi: fsi: Reduce max transfer size to 8 bytes > > On Mon, 2021-07-19 at 16:20 +0100, Mark Brown wrote: > > On Fri, Jul 16, 2021 at 01:34:38PM -0500, Eddie James wrote: > > > > > Security changes in the SPI controller - in the device microcode. I > > > can > > > reword the commit if you like. > > > > How will people end up running this device microcode? Is this a bug > > fix, or is this going to needlessly reduce performance for people > > with > > existing hardware? > > The hardware is still in development. As part of the development, the > device microcode was changed to restrict transfers. The reason for this > restriction was "security concerns". This restriction disallows the > loop (or branch-if-not-equal-and-increment) sequencer command. It also > does not allow the read (or shift in if you prefer) command to specify > the number of bytes in the command itself. Rather, the number of bits > to shift in must be specified in a separate control register. This > effectively means that the controller cannot transfer more than 8 bytes > at a time. > Therefore I suppose this is effectively a bug fix. There will be no > hardware available without these restrictions, so it is not a needless > reduction in performance. Every system that can run this driver will > run the more restrictive device microcode. So just say that release versions of the hardware won't support more than 8 byte transfers. Having said that, you might want a loop in the driver so that application requests for longer transfers are implemented with multiple hardware requests. I do also wonder why there is support in the main kernel sources for hardware that doesn't actually exist. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Jul 20, 2021 at 01:04:38PM +0000, David Laight wrote: > Having said that, you might want a loop in the driver so that > application requests for longer transfers are implemented > with multiple hardware requests. No, that's something that should be and indeed is done in the core - this isn't the only hardware out there with some kind of restriction on length. > I do also wonder why there is support in the main kernel sources > for hardware that doesn't actually exist. We encourage vendors to get support for their devices upstream prior to hardware availability so that users are able to run upstream when they get access to hardware, this means users aren't forced to run out of tree code needlessly and greatly eases deployment.
From: Mark Brown > Sent: 20 July 2021 14:13 > > On Tue, Jul 20, 2021 at 01:04:38PM +0000, David Laight wrote: > > > Having said that, you might want a loop in the driver so that > > application requests for longer transfers are implemented > > with multiple hardware requests. > > No, that's something that should be and indeed is done in the core - > this isn't the only hardware out there with some kind of restriction on > length. Ah, ok, there is another loop before any 'users'. > > I do also wonder why there is support in the main kernel sources > > for hardware that doesn't actually exist. > > We encourage vendors to get support for their devices upstream prior to > hardware availability so that users are able to run upstream when they > get access to hardware, this means users aren't forced to run out of > tree code needlessly and greatly eases deployment. This one just seemed a bit premature. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/drivers/spi/spi-fsi.c b/drivers/spi/spi-fsi.c index 87f8829c3995..829770b8ec74 100644 --- a/drivers/spi/spi-fsi.c +++ b/drivers/spi/spi-fsi.c @@ -25,16 +25,11 @@ #define SPI_FSI_BASE 0x70000 #define SPI_FSI_INIT_TIMEOUT_MS 1000 -#define SPI_FSI_MAX_XFR_SIZE 2048 -#define SPI_FSI_MAX_XFR_SIZE_RESTRICTED 8 +#define SPI_FSI_MAX_RX_SIZE 8 +#define SPI_FSI_MAX_TX_SIZE 40 #define SPI_FSI_ERROR 0x0 #define SPI_FSI_COUNTER_CFG 0x1 -#define SPI_FSI_COUNTER_CFG_LOOPS(x) (((u64)(x) & 0xffULL) << 32) -#define SPI_FSI_COUNTER_CFG_N2_RX BIT_ULL(8) -#define SPI_FSI_COUNTER_CFG_N2_TX BIT_ULL(9) -#define SPI_FSI_COUNTER_CFG_N2_IMPLICIT BIT_ULL(10) -#define SPI_FSI_COUNTER_CFG_N2_RELOAD BIT_ULL(11) #define SPI_FSI_CFG1 0x2 #define SPI_FSI_CLOCK_CFG 0x3 #define SPI_FSI_CLOCK_CFG_MM_ENABLE BIT_ULL(32) @@ -76,8 +71,6 @@ struct fsi_spi { struct device *dev; /* SPI controller device */ struct fsi_device *fsi; /* FSI2SPI CFAM engine device */ u32 base; - size_t max_xfr_size; - bool restricted; }; struct fsi_spi_sequence { @@ -241,7 +234,7 @@ static int fsi_spi_reset(struct fsi_spi *ctx) return fsi_spi_write_reg(ctx, SPI_FSI_STATUS, 0ULL); } -static int fsi_spi_sequence_add(struct fsi_spi_sequence *seq, u8 val) +static void fsi_spi_sequence_add(struct fsi_spi_sequence *seq, u8 val) { /* * Add the next byte of instruction to the 8-byte sequence register. @@ -251,8 +244,6 @@ static int fsi_spi_sequence_add(struct fsi_spi_sequence *seq, u8 val) */ seq->data |= (u64)val << seq->bit; seq->bit -= 8; - - return ((64 - seq->bit) / 8) - 2; } static void fsi_spi_sequence_init(struct fsi_spi_sequence *seq) @@ -261,71 +252,11 @@ static void fsi_spi_sequence_init(struct fsi_spi_sequence *seq) seq->data = 0ULL; } -static int fsi_spi_sequence_transfer(struct fsi_spi *ctx, - struct fsi_spi_sequence *seq, - struct spi_transfer *transfer) -{ - int loops; - int idx; - int rc; - u8 val = 0; - u8 len = min(transfer->len, 8U); - u8 rem = transfer->len % len; - - loops = transfer->len / len; - - if (transfer->tx_buf) { - val = SPI_FSI_SEQUENCE_SHIFT_OUT(len); - idx = fsi_spi_sequence_add(seq, val); - - if (rem) - rem = SPI_FSI_SEQUENCE_SHIFT_OUT(rem); - } else if (transfer->rx_buf) { - val = SPI_FSI_SEQUENCE_SHIFT_IN(len); - idx = fsi_spi_sequence_add(seq, val); - - if (rem) - rem = SPI_FSI_SEQUENCE_SHIFT_IN(rem); - } else { - return -EINVAL; - } - - if (ctx->restricted && loops > 1) { - dev_warn(ctx->dev, - "Transfer too large; no branches permitted.\n"); - return -EINVAL; - } - - if (loops > 1) { - u64 cfg = SPI_FSI_COUNTER_CFG_LOOPS(loops - 1); - - fsi_spi_sequence_add(seq, SPI_FSI_SEQUENCE_BRANCH(idx)); - - if (transfer->rx_buf) - cfg |= SPI_FSI_COUNTER_CFG_N2_RX | - SPI_FSI_COUNTER_CFG_N2_TX | - SPI_FSI_COUNTER_CFG_N2_IMPLICIT | - SPI_FSI_COUNTER_CFG_N2_RELOAD; - - rc = fsi_spi_write_reg(ctx, SPI_FSI_COUNTER_CFG, cfg); - if (rc) - return rc; - } else { - fsi_spi_write_reg(ctx, SPI_FSI_COUNTER_CFG, 0ULL); - } - - if (rem) - fsi_spi_sequence_add(seq, rem); - - return 0; -} - static int fsi_spi_transfer_data(struct fsi_spi *ctx, struct spi_transfer *transfer) { int rc = 0; u64 status = 0ULL; - u64 cfg = 0ULL; if (transfer->tx_buf) { int nb; @@ -363,16 +294,6 @@ static int fsi_spi_transfer_data(struct fsi_spi *ctx, u64 in = 0ULL; u8 *rx = transfer->rx_buf; - rc = fsi_spi_read_reg(ctx, SPI_FSI_COUNTER_CFG, &cfg); - if (rc) - return rc; - - if (cfg & SPI_FSI_COUNTER_CFG_N2_IMPLICIT) { - rc = fsi_spi_write_reg(ctx, SPI_FSI_DATA_TX, 0); - if (rc) - return rc; - } - while (transfer->len > recv) { do { rc = fsi_spi_read_reg(ctx, SPI_FSI_STATUS, @@ -439,6 +360,10 @@ static int fsi_spi_transfer_init(struct fsi_spi *ctx) } } while (seq_state && (seq_state != SPI_FSI_STATUS_SEQ_STATE_IDLE)); + rc = fsi_spi_write_reg(ctx, SPI_FSI_COUNTER_CFG, 0ULL); + if (rc) + return rc; + rc = fsi_spi_read_reg(ctx, SPI_FSI_CLOCK_CFG, &clock_cfg); if (rc) return rc; @@ -459,6 +384,7 @@ static int fsi_spi_transfer_one_message(struct spi_controller *ctlr, { int rc; u8 seq_slave = SPI_FSI_SEQUENCE_SEL_SLAVE(mesg->spi->chip_select + 1); + unsigned int len; struct spi_transfer *transfer; struct fsi_spi *ctx = spi_controller_get_devdata(ctlr); @@ -471,8 +397,7 @@ static int fsi_spi_transfer_one_message(struct spi_controller *ctlr, struct spi_transfer *next = NULL; /* Sequencer must do shift out (tx) first. */ - if (!transfer->tx_buf || - transfer->len > (ctx->max_xfr_size + 8)) { + if (!transfer->tx_buf || transfer->len > SPI_FSI_MAX_TX_SIZE) { rc = -EINVAL; goto error; } @@ -486,9 +411,13 @@ static int fsi_spi_transfer_one_message(struct spi_controller *ctlr, fsi_spi_sequence_init(&seq); fsi_spi_sequence_add(&seq, seq_slave); - rc = fsi_spi_sequence_transfer(ctx, &seq, transfer); - if (rc) - goto error; + len = transfer->len; + while (len > 8) { + fsi_spi_sequence_add(&seq, + SPI_FSI_SEQUENCE_SHIFT_OUT(8)); + len -= 8; + } + fsi_spi_sequence_add(&seq, SPI_FSI_SEQUENCE_SHIFT_OUT(len)); if (!list_is_last(&transfer->transfer_list, &mesg->transfers)) { @@ -496,7 +425,9 @@ static int fsi_spi_transfer_one_message(struct spi_controller *ctlr, /* Sequencer can only do shift in (rx) after tx. */ if (next->rx_buf) { - if (next->len > ctx->max_xfr_size) { + u8 shift; + + if (next->len > SPI_FSI_MAX_RX_SIZE) { rc = -EINVAL; goto error; } @@ -504,10 +435,8 @@ static int fsi_spi_transfer_one_message(struct spi_controller *ctlr, dev_dbg(ctx->dev, "Sequence rx of %d bytes.\n", next->len); - rc = fsi_spi_sequence_transfer(ctx, &seq, - next); - if (rc) - goto error; + shift = SPI_FSI_SEQUENCE_SHIFT_IN(next->len); + fsi_spi_sequence_add(&seq, shift); } else { next = NULL; } @@ -541,9 +470,7 @@ static int fsi_spi_transfer_one_message(struct spi_controller *ctlr, static size_t fsi_spi_max_transfer_size(struct spi_device *spi) { - struct fsi_spi *ctx = spi_controller_get_devdata(spi->controller); - - return ctx->max_xfr_size; + return SPI_FSI_MAX_RX_SIZE; } static int fsi_spi_probe(struct device *dev) @@ -582,14 +509,6 @@ static int fsi_spi_probe(struct device *dev) ctx->fsi = fsi; ctx->base = base + SPI_FSI_BASE; - if (of_device_is_compatible(np, "ibm,fsi2spi-restricted")) { - ctx->restricted = true; - ctx->max_xfr_size = SPI_FSI_MAX_XFR_SIZE_RESTRICTED; - } else { - ctx->restricted = false; - ctx->max_xfr_size = SPI_FSI_MAX_XFR_SIZE; - } - rc = devm_spi_register_controller(dev, ctlr); if (rc) spi_controller_put(ctlr);
Security changes have forced the SPI controllers to be limited to 8 byte reads. Refactor the sequencing to just handle 8 bytes at a time. Signed-off-by: Eddie James <eajames@linux.ibm.com> --- drivers/spi/spi-fsi.c | 125 ++++++++---------------------------------- 1 file changed, 22 insertions(+), 103 deletions(-)