[PATCH] eeprom_at24c: Model 8-bit data addressing for 16-bit devices

[PATCH] eeprom_at24c: Model 8-bit data addressing for 16-bit devices

[Andrew Jeffery]

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>
---
 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];
-- 
2.39.2

Re: [PATCH] eeprom_at24c: Model 8-bit data addressing for 16-bit devices

[Cédric Le Goater]

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];

Re: [PATCH] eeprom_at24c: Model 8-bit data addressing for 16-bit devices

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1751 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] eeprom_at24c: Model 8-bit data addressing for 16-bit devices

[Andrew Jeffery]

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