Re: [PATCH 1/2] hw: m25p80: Add Block Protect and Top Bottom bits for write protect

[Francisco Iglesias]

Hi Iris,

Looks good, a couple of minor comments below!

On [2022 Jun 27] Mon 11:52:33, Iris Chen wrote:
> Signed-off-by: Iris Chen <irischenlj@fb.com>
> ---
>  hw/block/m25p80.c | 74 +++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 62 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 50b523e5b1..0156a70f5e 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -38,21 +38,19 @@
>  #include "trace.h"
>  #include "qom/object.h"
>  
> -/* Fields for FlashPartInfo->flags */
> -
> -/* erase capabilities */
> -#define ER_4K 1
> -#define ER_32K 2
> -/* set to allow the page program command to write 0s back to 1. Useful for
> - * modelling EEPROM with SPI flash command set
> - */
> -#define EEPROM 0x100
> -
>  /* 16 MiB max in 3 byte address mode */
>  #define MAX_3BYTES_SIZE 0x1000000
> -
>  #define SPI_NOR_MAX_ID_LEN 6
>  
> +/* Fields for FlashPartInfo->flags */
> +enum spi_nor_option_flags {

(A suggestion is to s/nor/flash/ above (and s/SNOR_F_//  below) since there
looks to be nand flashes as W25N01GV using the protocol to).

> +    ER_4K                  = BIT(0),
> +    ER_32K                 = BIT(1),
> +    EEPROM                 = BIT(2),
> +    SNOR_F_HAS_SR_TB       = BIT(3),
> +    SNOR_F_HAS_SR_BP3_BIT6 = BIT(4),
> +};
> +
>  typedef struct FlashPartInfo {
>      const char *part_name;
>      /*
> @@ -253,7 +251,8 @@ static const FlashPartInfo known_devices[] = {
>      { INFO("n25q512a11",  0x20bb20,      0,  64 << 10, 1024, ER_4K) },
>      { INFO("n25q512a13",  0x20ba20,      0,  64 << 10, 1024, ER_4K) },
>      { INFO("n25q128",     0x20ba18,      0,  64 << 10, 256, 0) },
> -    { INFO("n25q256a",    0x20ba19,      0,  64 << 10, 512, ER_4K) },
> +    { INFO("n25q256a",    0x20ba19,      0,  64 << 10, 512,
> +           ER_4K | SNOR_F_HAS_SR_BP3_BIT6 | SNOR_F_HAS_SR_TB) },
>      { INFO("n25q512a",    0x20ba20,      0,  64 << 10, 1024, ER_4K) },
>      { INFO("n25q512ax3",  0x20ba20,  0x1000,  64 << 10, 1024, ER_4K) },
>      { INFO("mt25ql512ab", 0x20ba20, 0x1044, 64 << 10, 1024, ER_4K | ER_32K) 
> },
> @@ -480,6 +479,11 @@ struct Flash {
>      bool reset_enable;
>      bool quad_enable;
>      bool aai_enable;
> +    bool block_protect0;
> +    bool block_protect1;
> +    bool block_protect2;
> +    bool block_protect3;
> +    bool top_bottom_bit;
>      bool status_register_write_disabled;
>      uint8_t ear;
>  
> @@ -630,6 +634,29 @@ void flash_write8(Flash *s, uint32_t addr, uint8_t data)
>          qemu_log_mask(LOG_GUEST_ERROR, "M25P80: write with write 
> protect!\n");
>          return;
>      }
> +    uint32_t block_protect_value = (s->block_protect3 << 3) |
> +                                   (s->block_protect2 << 2) |
> +                                   (s->block_protect1 << 1) |
> +                                   (s->block_protect0 << 0);
> +
> +     uint32_t num_protected_sectors = 1 << (block_protect_value - 1);
> +     uint32_t sector = addr / s->pi->sector_size;
> +
> +     /* top_bottom_bit == 0 means TOP */

Indentation needs minor fixing on above lines, also the declarations should
be at the top of the function.

> +    if (!s->top_bottom_bit) {
> +        if (block_protect_value > 0 &&
> +            s->pi->n_sectors <= sector + num_protected_sectors) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "M25P80: write with write protect!\n");
> +            return;
> +        }
> +    } else {
> +        if (block_protect_value > 0 && sector < num_protected_sectors) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "M25P80: write with write protect!\n");
> +            return;
> +        }
> +    }
>  
>      if ((prev ^ data) & data) {
>          trace_m25p80_programming_zero_to_one(s, addr, prev, data);
> @@ -728,6 +755,15 @@ static void complete_collecting_data(Flash *s)
>          break;
>      case WRSR:
>          s->status_register_write_disabled = extract32(s->data[0], 7, 1);
> +        s->block_protect0 = extract32(s->data[0], 2, 1);
> +        s->block_protect1 = extract32(s->data[0], 3, 1);
> +        s->block_protect2 = extract32(s->data[0], 4, 1);
> +        if (s->pi->flags & SNOR_F_HAS_SR_TB) {
> +            s->top_bottom_bit = extract32(s->data[0], 5, 1);
> +        }
> +        if (s->pi->flags & SNOR_F_HAS_SR_BP3_BIT6) {
> +            s->block_protect3 = extract32(s->data[0], 6, 1);
> +        }
>  
>          switch (get_man(s)) {
>          case MAN_SPANSION:
> @@ -1213,6 +1249,15 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>      case RDSR:
>          s->data[0] = (!!s->write_enable) << 1;
>          s->data[0] |= (!!s->status_register_write_disabled) << 7;
> +        s->data[0] |= (!!s->block_protect0) << 2;
> +        s->data[0] |= (!!s->block_protect1) << 3;
> +        s->data[0] |= (!!s->block_protect2) << 4;
> +        if (s->pi->flags & SNOR_F_HAS_SR_TB) {
> +            s->data[0] |= (!!s->top_bottom_bit) << 5;
> +        }
> +        if (s->pi->flags & SNOR_F_HAS_SR_BP3_BIT6) {
> +            s->data[0] |= (!!s->block_protect3) << 6;
> +        }
>  
>          if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) {
>              s->data[0] |= (!!s->quad_enable) << 6;
> @@ -1553,6 +1598,11 @@ static void m25p80_reset(DeviceState *d)
>  
>      s->wp_level = true;
>      s->status_register_write_disabled = false;
> +    s->block_protect0 = false;
> +    s->block_protect1 = false;
> +    s->block_protect2 = false;
> +    s->block_protect3 = false;
> +    s->top_bottom_bit = false;

We need to place above ones in a subsection in the vmstate (similar to the your
previous patch).

Looks good to me otherwise!

Thanks!
Best regards,
Francisco Iglesias

>  
>      reset_memory(s);
>  }
> -- 
> 2.30.2
> 
>