Re: [PATCH v2 1/9] hw/nvram: Introduce Xilinx eFuse QOM

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 23 Aug 2021 at 18:49, Tong Ho <tong.ho@xilinx.com> wrote:
>
> This introduces the QOM for Xilinx eFuse, an one-time
> field-programmable storage bit array.
>
> The actual mmio interface to the array varies by device
> families and will be provided in different change-sets.
>
> Co-authored-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Co-authored-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> Signed-off-by: Tong Ho <tong.ho@xilinx.com>

--- /dev/null
+++ b/hw/nvram/xlnx-efuse-crc.c
@@ -0,0 +1,118 @@
+/*
+ * Xilinx eFuse/bbram CRC calculator

+ */
+#include "hw/nvram/xlnx-efuse.h"

.c files must always include "qemu/osdep.h" as their first #include.

> +#ifndef XLNX_EFUSE_ERR_DEBUG
> +#define XLNX_EFUSE_ERR_DEBUG 0
> +#endif

This define doesn't seem to be used; you could just drop it.

> +#define XLNX_EFUSE(obj) \
> +     OBJECT_CHECK(XLNXEFuse, (obj), TYPE_XLNX_EFUSE)

This is a bit of an old-style way to write this. These days we
recommend using the OBJECT_DECLARE_TYPE macro in your .h file
(which will provide the cast macro/function and also some typedefs
that you're currently manually providing).

> +static void efuse_sync_bdrv(XLNXEFuse *s, unsigned int bit)
> +{
> +    const int bswap_adj = (const_le32(0x1234) != 0x1234 ? 3 : 0);

Don't do ad-hoc figuring out of the host endianness like this.
I would suggest using cpu_to_le32() on the relevant word
in fuse32[] and then writing that to the backing store.

> +    unsigned int efuse_byte;
> +
> +    if (!s->blk || s->blk_ro) {
> +        return;  /* Silient on read-only backend to avoid message flood */

"silent"

> +    }
> +
> +    efuse_byte = bit / 8;
> +
> +    if (blk_pwrite(s->blk, efuse_byte,
> +                   ((uint8_t *) s->fuse32) + (efuse_byte ^ bswap_adj),
> +                   1, 0) < 0) {
> +        error_report("%s: write error in byte %" PRIu32 ".",
> +                      __func__, efuse_byte);
> +    }
> +}

> +static void efuse_realize(DeviceState *dev, Error **errp)
> +{
> +    XLNXEFuse *s = XLNX_EFUSE(dev);
> +    BlockBackend *blk;
> +    DriveInfo *dinfo;
> +    unsigned int nr_bytes;
> +    const char *prefix = object_get_canonical_path(OBJECT(dev));
> +
> +    if (s->drv_index < 0) {
> +        /* Set legacy compatibility */
> +        s->drv_index = s->efuse_size <= 2048 ? 3 : 1;
> +    }
> +
> +    dinfo = drive_get_by_index(IF_PFLASH, s->drv_index);
> +    blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;

Don't get block backends like this in device models, please.
Instead, the device should have a property "drive" (use
DEFINE_PROP_DRIVE() to declare this), and the board should find
the drive and attach it to the device. In fact looking lower down
in the file I see you have a 'drive' property, so you should
just be using it (and getting rid of the drive-index property).

> +
> +    nr_bytes = ROUND_UP((s->efuse_nr * s->efuse_size) / 8, 4);
> +    s->fuse32 = g_malloc0(nr_bytes);
> +    if (blk) {
> +        qdev_prop_set_drive(dev, "drive", blk);
> +
> +        s->blk_ro = !blk_supports_write_perm(s->blk);
> +        if (s->blk_ro) {
> +            warn_report("%s: update not saved: backstore is read-only",
> +                        object_get_canonical_path(OBJECT(s)));
> +        }
> +        blk_set_perm(s->blk,
> +                     (BLK_PERM_CONSISTENT_READ
> +                      | (s->blk_ro ? 0 : BLK_PERM_WRITE)), BLK_PERM_ALL,
> +                     &error_abort);

&error_abort isn't really appropriate in a device model, unless
you know the function call really can't fail. Better to pass
the error back up to the caller. (Watch out that you need to free
the s->fuse32 you just allocated if you return early.)

> +
> +        if (blk_pread(s->blk, 0, (void *) s->fuse32, nr_bytes) < 0) {
> +            error_setg(&error_abort, "%s: Unable to read-out contents."
> +                         "backing file too small? Expecting %" PRIu32" bytes",
> +                          prefix,
> +                          (unsigned int) (nr_bytes));

You should pass this to the caller, not use error_abort.


> +        }
> +        if (const_le32(0x1234) != 0x1234) {

Again, no ad-hoc endianness testing, please.

> +            /* Convert from little-endian backstore for each 32-bit row */
> +            unsigned int nr_u32;
> +
> +            for (nr_u32 = 0; nr_u32 < (nr_bytes / 4); nr_u32++) {
> +                s->fuse32[nr_u32] = le32_to_cpu(s->fuse32[nr_u32]);
> +            }
> +        }
> +    }
> +
> +    /* Sort readonly-list for bsearch lookup */
> +    efuse_ro_bits_sort(s);
> +}



> +static const VMStateDescription vmstate_efuse = {
> +    .name = TYPE_XLNX_EFUSE,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,

You don't need to specify the minimum_version_id_old here.

> +    .fields = (VMStateField[]) {
> +        VMSTATE_END_OF_LIST(),

If the device genuinely has no internal state (and it looks
like in this case that is true), you don't need to write out
an empty vmstate. Instead put a comment in the class init function
like:

 /*
  * This device has no internal state (it is all kept in the
  * block device) so it does not need a vmstate.
  */




> +#ifndef XLNX_EFUSE_H
> +#define XLNX_EFUSE_H
> +
> +#include "qemu/osdep.h"

Headers should never include osdep.h.

> +#include "sysemu/block-backend.h"
> +#include "hw/qdev-core.h"
> +
> +#define TYPE_XLNX_EFUSE "xlnx,efuse"
> +
> +typedef struct XLNXEFuseLkSpec {
> +    uint16_t row;
> +    uint16_t lk_bit;
> +} XLNXEFuseLkSpec;

What's this struct for? Nothing in this patch or in these files uses it.

> +typedef struct XLNXEFuse {
> +    DeviceState parent_obj;
> +    BlockBackend *blk;
> +    bool blk_ro;
> +    uint32_t *fuse32;
> +
> +    DeviceState *dev;
> +
> +    bool init_tbits;
> +    int drv_index;
> +
> +    uint8_t efuse_nr;
> +    uint32_t efuse_size;
> +
> +    uint32_t *ro_bits;
> +    uint32_t ro_bits_cnt;
> +} XLNXEFuse;
> +
> +uint32_t xlnx_efuse_calc_crc(const uint32_t *data, unsigned u32_cnt,
> +                             unsigned zpads);

Where you're providing function prototypes in a header to be used
by other parts of QEMU, can you provide some brief doc-comment format
comments that describe what the API of those functions is, please ?

> +
> +bool xlnx_efuse_get_bit(XLNXEFuse *s, unsigned int bit);
> +bool xlnx_efuse_set_bit(XLNXEFuse *s, unsigned int bit);
> +bool xlnx_efuse_k256_check(XLNXEFuse *s, uint32_t crc, unsigned start);
> +uint32_t xlnx_efuse_tbits_check(XLNXEFuse *s);
> +
> +/* Return whole row containing the given bit address */
> +static inline uint32_t xlnx_efuse_get_row(XLNXEFuse *s, unsigned int bit)
> +{
> +    if (!(s->fuse32)) {
> +        return 0;
> +    } else {
> +        unsigned int row_idx = bit / 32;
> +
> +        assert(row_idx < (s->efuse_size * s->efuse_nr / 32));
> +        return s->fuse32[row_idx];
> +    }
> +}
> +
> +#endif

thanks
-- PMM