| Message ID | 20230921034816.320655-1-andrew@codeconstruct.com.au |
|---|---|
| State | New |
| Headers | show |
| Series | eeprom_at24c: Model 8-bit data addressing for 16-bit devices | expand |
Cc: Klaus On 9/21/23 05:48, Andrew Jeffery wrote: > It appears some (many?) EEPROMs that implement 16-bit data addressing > will accept an 8-bit address and clock out non-uniform data for the > read. This behaviour is exploited by an EEPROM detection routine in part > of OpenBMC userspace with a reasonably broad user base: > > https://github.com/openbmc/entity-manager/blob/0422a24bb6033605ce75479f675fedc76abb1167/src/fru_device.cpp#L197-L229 > > The diversity of the set of EEPROMs that it operates against is unclear, > but this code has been around for a while now. > > Separately, The NVM Express Management Interface Specification dictates > the provided behaviour in section 8.2 Vital Product Data: > >> If only one byte of the Command Offset is provided by the Management >> Controller, then the least significant byte of the internal offset >> shall be set to that value and the most-significant byte of the >> internal offset shall be cleared to 0h > > https://nvmexpress.org/wp-content/uploads/NVM-Express-Management-Interface-Specification-1.2c-2022.10.06-Ratified.pdf > > This change makes it possible to expose NVMe VPD in a manner that can be > dynamically detected by OpenBMC. > > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au> It seems that the "at24c-eeprom" model doesn't have a maintainer. Until this is sorted out, may be this change could go through the NVMe queue since it is related. Thanks, C. > --- > hw/nvram/eeprom_at24c.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c > index 613c4929e327..64a61cc0e468 100644 > --- a/hw/nvram/eeprom_at24c.c > +++ b/hw/nvram/eeprom_at24c.c > @@ -98,12 +98,20 @@ uint8_t at24c_eeprom_recv(I2CSlave *s) > EEPROMState *ee = AT24C_EE(s); > uint8_t ret; > > - /* > - * If got the byte address but not completely with address size > - * will return the invalid value > - */ > if (ee->haveaddr > 0 && ee->haveaddr < ee->asize) { > - return 0xff; > + /* > + * Provide behaviour that aligns with NVMe MI 1.2c, section 8.2. > + * > + * https://nvmexpress.org/wp-content/uploads/NVM-Express-Management-Interface-Specification-1.2c-2022.10.06-Ratified.pdf > + * > + * Otherwise, the clocked-out data is meaningless anyway, and so reading > + * off memory is as good a behaviour as anything. This also happens to > + * help the address-width detection heuristic in OpenBMC's userspace. > + * > + * https://github.com/openbmc/entity-manager/blob/0422a24bb6033605ce75479f675fedc76abb1167/src/fru_device.cpp#L197-L229 > + */ > + ee->haveaddr = ee->asize; > + ee->cur %= ee->rsize; > } > > ret = ee->mem[ee->cur];
On Oct 25 11:14, Cédric Le Goater wrote: > Cc: Klaus > > On 9/21/23 05:48, Andrew Jeffery wrote: > > It appears some (many?) EEPROMs that implement 16-bit data addressing > > will accept an 8-bit address and clock out non-uniform data for the > > read. This behaviour is exploited by an EEPROM detection routine in part > > of OpenBMC userspace with a reasonably broad user base: > > > > https://github.com/openbmc/entity-manager/blob/0422a24bb6033605ce75479f675fedc76abb1167/src/fru_device.cpp#L197-L229 > > > > The diversity of the set of EEPROMs that it operates against is unclear, > > but this code has been around for a while now. > > > > Separately, The NVM Express Management Interface Specification dictates > > the provided behaviour in section 8.2 Vital Product Data: > > > > > If only one byte of the Command Offset is provided by the Management > > > Controller, then the least significant byte of the internal offset > > > shall be set to that value and the most-significant byte of the > > > internal offset shall be cleared to 0h > > > > https://nvmexpress.org/wp-content/uploads/NVM-Express-Management-Interface-Specification-1.2c-2022.10.06-Ratified.pdf > > > > This change makes it possible to expose NVMe VPD in a manner that can be > > dynamically detected by OpenBMC. > > > > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au> > > It seems that the "at24c-eeprom" model doesn't have a maintainer. Until > this is sorted out, may be this change could go through the NVMe queue > since it is related. > I can, but I'm not that confident on determining if we choose to implement this behavior broadly. I have no qualms, but someone who works more with embedded stuff might?
On Wed, 2023-10-25 at 11:22 +0200, Klaus Jensen wrote: > On Oct 25 11:14, Cédric Le Goater wrote: > > It seems that the "at24c-eeprom" model doesn't have a maintainer. Until > > this is sorted out, may be this change could go through the NVMe queue > > since it is related. > > > > I can, but I'm not that confident on determining if we choose to > implement this behavior broadly. I have no qualms, but someone who works > more with embedded stuff might? What are the feelings on putting the behaviour behind a flag? We could add it as a property that we can set, e.g. when defining a machine. Andrew
diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c index 613c4929e327..64a61cc0e468 100644 --- a/hw/nvram/eeprom_at24c.c +++ b/hw/nvram/eeprom_at24c.c @@ -98,12 +98,20 @@ uint8_t at24c_eeprom_recv(I2CSlave *s) EEPROMState *ee = AT24C_EE(s); uint8_t ret; - /* - * If got the byte address but not completely with address size - * will return the invalid value - */ if (ee->haveaddr > 0 && ee->haveaddr < ee->asize) { - return 0xff; + /* + * Provide behaviour that aligns with NVMe MI 1.2c, section 8.2. + * + * https://nvmexpress.org/wp-content/uploads/NVM-Express-Management-Interface-Specification-1.2c-2022.10.06-Ratified.pdf + * + * Otherwise, the clocked-out data is meaningless anyway, and so reading + * off memory is as good a behaviour as anything. This also happens to + * help the address-width detection heuristic in OpenBMC's userspace. + * + * https://github.com/openbmc/entity-manager/blob/0422a24bb6033605ce75479f675fedc76abb1167/src/fru_device.cpp#L197-L229 + */ + ee->haveaddr = ee->asize; + ee->cur %= ee->rsize; } ret = ee->mem[ee->cur];