Message ID | 20220621202427.2680413-1-irischenlj@fb.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v4] hw: m25p80: add WP# pin and SRWD bit for write protection | expand |
On [2022 Jun 21] Tue 13:24:27, Iris Chen wrote: > From: Iris Chen <irischenlj@gmail.com> > > Signed-off-by: Iris Chen <irischenlj@gmail.com> Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com> > --- > Fixed .needed for subsection and suggestions from Francisco > > hw/block/m25p80.c | 82 ++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 67 insertions(+), 15 deletions(-) > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c > index 81ba3da4df..3045dda53b 100644 > --- a/hw/block/m25p80.c > +++ b/hw/block/m25p80.c > @@ -472,11 +472,13 @@ struct Flash { > uint8_t spansion_cr2v; > uint8_t spansion_cr3v; > uint8_t spansion_cr4v; > + bool wp_level; > bool write_enable; > bool four_bytes_address_mode; > bool reset_enable; > bool quad_enable; > bool aai_enable; > + bool status_register_write_disabled; > uint8_t ear; > > int64_t dirty_page; > @@ -723,6 +725,8 @@ static void complete_collecting_data(Flash *s) > flash_erase(s, s->cur_addr, s->cmd_in_progress); > break; > case WRSR: > + s->status_register_write_disabled = extract32(s->data[0], 7, 1); > + > switch (get_man(s)) { > case MAN_SPANSION: > s->quad_enable = !!(s->data[1] & 0x02); > @@ -1165,22 +1169,34 @@ static void decode_new_cmd(Flash *s, uint32_t value) > break; > > case WRSR: > - if (s->write_enable) { > - switch (get_man(s)) { > - case MAN_SPANSION: > - s->needed_bytes = 2; > - s->state = STATE_COLLECTING_DATA; > - break; > - case MAN_MACRONIX: > - s->needed_bytes = 2; > - s->state = STATE_COLLECTING_VAR_LEN_DATA; > - break; > - default: > - s->needed_bytes = 1; > - s->state = STATE_COLLECTING_DATA; > - } > - s->pos = 0; > + /* > + * If WP# is low and status_register_write_disabled is high, > + * status register writes are disabled. > + * This is also called "hardware protected mode" (HPM). All other > + * combinations of the two states are called "software protected mode" > + * (SPM), and status register writes are permitted. > + */ > + if ((s->wp_level == 0 && s->status_register_write_disabled) > + || !s->write_enable) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "M25P80: Status register write is disabled!\n"); > + break; > + } > + > + switch (get_man(s)) { > + case MAN_SPANSION: > + s->needed_bytes = 2; > + s->state = STATE_COLLECTING_DATA; > + break; > + case MAN_MACRONIX: > + s->needed_bytes = 2; > + s->state = STATE_COLLECTING_VAR_LEN_DATA; > + break; > + default: > + s->needed_bytes = 1; > + s->state = STATE_COLLECTING_DATA; > } > + s->pos = 0; > break; > > case WRDI: > @@ -1195,6 +1211,8 @@ 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; > + > if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) { > s->data[0] |= (!!s->quad_enable) << 6; > } > @@ -1484,6 +1502,14 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss, uint32_t tx) > return r; > } > > +static void m25p80_write_protect_pin_irq_handler(void *opaque, int n, int level) > +{ > + Flash *s = M25P80(opaque); > + /* WP# is just a single pin. */ > + assert(n == 0); > + s->wp_level = !!level; > +} > + > static void m25p80_realize(SSIPeripheral *ss, Error **errp) > { > Flash *s = M25P80(ss); > @@ -1515,12 +1541,18 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp) > s->storage = blk_blockalign(NULL, s->size); > memset(s->storage, 0xFF, s->size); > } > + > + qdev_init_gpio_in_named(DEVICE(s), > + m25p80_write_protect_pin_irq_handler, "WP#", 1); > } > > static void m25p80_reset(DeviceState *d) > { > Flash *s = M25P80(d); > > + s->wp_level = true; > + s->status_register_write_disabled = false; > + > reset_memory(s); > } > > @@ -1587,6 +1619,25 @@ static const VMStateDescription vmstate_m25p80_aai_enable = { > } > }; > > +static bool m25p80_wp_level_srwd_needed(void *opaque) > +{ > + Flash *s = (Flash *)opaque; > + > + return !s->wp_level || s->status_register_write_disabled; > +} > + > +static const VMStateDescription vmstate_m25p80_write_protect = { > + .name = "m25p80/write_protect", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = m25p80_wp_level_srwd_needed, > + .fields = (VMStateField[]) { > + VMSTATE_BOOL(wp_level, Flash), > + VMSTATE_BOOL(status_register_write_disabled, Flash), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription vmstate_m25p80 = { > .name = "m25p80", > .version_id = 0, > @@ -1618,6 +1669,7 @@ static const VMStateDescription vmstate_m25p80 = { > .subsections = (const VMStateDescription * []) { > &vmstate_m25p80_data_read_loop, > &vmstate_m25p80_aai_enable, > + &vmstate_m25p80_write_protect, > NULL > } > }; > -- > 2.30.2 > >
Alistair, Francisco, On 6/22/22 11:45, Francisco Iglesias wrote: > On [2022 Jun 21] Tue 13:24:27, Iris Chen wrote: >> From: Iris Chen <irischenlj@gmail.com> >> >> Signed-off-by: Iris Chen <irischenlj@gmail.com> > > Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com> I am planning to include this patch in the next aspeed PR if that's OK with you. Thanks, C. > >> --- >> Fixed .needed for subsection and suggestions from Francisco >> >> hw/block/m25p80.c | 82 ++++++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 67 insertions(+), 15 deletions(-) >> >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c >> index 81ba3da4df..3045dda53b 100644 >> --- a/hw/block/m25p80.c >> +++ b/hw/block/m25p80.c >> @@ -472,11 +472,13 @@ struct Flash { >> uint8_t spansion_cr2v; >> uint8_t spansion_cr3v; >> uint8_t spansion_cr4v; >> + bool wp_level; >> bool write_enable; >> bool four_bytes_address_mode; >> bool reset_enable; >> bool quad_enable; >> bool aai_enable; >> + bool status_register_write_disabled; >> uint8_t ear; >> >> int64_t dirty_page; >> @@ -723,6 +725,8 @@ static void complete_collecting_data(Flash *s) >> flash_erase(s, s->cur_addr, s->cmd_in_progress); >> break; >> case WRSR: >> + s->status_register_write_disabled = extract32(s->data[0], 7, 1); >> + >> switch (get_man(s)) { >> case MAN_SPANSION: >> s->quad_enable = !!(s->data[1] & 0x02); >> @@ -1165,22 +1169,34 @@ static void decode_new_cmd(Flash *s, uint32_t value) >> break; >> >> case WRSR: >> - if (s->write_enable) { >> - switch (get_man(s)) { >> - case MAN_SPANSION: >> - s->needed_bytes = 2; >> - s->state = STATE_COLLECTING_DATA; >> - break; >> - case MAN_MACRONIX: >> - s->needed_bytes = 2; >> - s->state = STATE_COLLECTING_VAR_LEN_DATA; >> - break; >> - default: >> - s->needed_bytes = 1; >> - s->state = STATE_COLLECTING_DATA; >> - } >> - s->pos = 0; >> + /* >> + * If WP# is low and status_register_write_disabled is high, >> + * status register writes are disabled. >> + * This is also called "hardware protected mode" (HPM). All other >> + * combinations of the two states are called "software protected mode" >> + * (SPM), and status register writes are permitted. >> + */ >> + if ((s->wp_level == 0 && s->status_register_write_disabled) >> + || !s->write_enable) { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "M25P80: Status register write is disabled!\n"); >> + break; >> + } >> + >> + switch (get_man(s)) { >> + case MAN_SPANSION: >> + s->needed_bytes = 2; >> + s->state = STATE_COLLECTING_DATA; >> + break; >> + case MAN_MACRONIX: >> + s->needed_bytes = 2; >> + s->state = STATE_COLLECTING_VAR_LEN_DATA; >> + break; >> + default: >> + s->needed_bytes = 1; >> + s->state = STATE_COLLECTING_DATA; >> } >> + s->pos = 0; >> break; >> >> case WRDI: >> @@ -1195,6 +1211,8 @@ 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; >> + >> if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) { >> s->data[0] |= (!!s->quad_enable) << 6; >> } >> @@ -1484,6 +1502,14 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss, uint32_t tx) >> return r; >> } >> >> +static void m25p80_write_protect_pin_irq_handler(void *opaque, int n, int level) >> +{ >> + Flash *s = M25P80(opaque); >> + /* WP# is just a single pin. */ >> + assert(n == 0); >> + s->wp_level = !!level; >> +} >> + >> static void m25p80_realize(SSIPeripheral *ss, Error **errp) >> { >> Flash *s = M25P80(ss); >> @@ -1515,12 +1541,18 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp) >> s->storage = blk_blockalign(NULL, s->size); >> memset(s->storage, 0xFF, s->size); >> } >> + >> + qdev_init_gpio_in_named(DEVICE(s), >> + m25p80_write_protect_pin_irq_handler, "WP#", 1); >> } >> >> static void m25p80_reset(DeviceState *d) >> { >> Flash *s = M25P80(d); >> >> + s->wp_level = true; >> + s->status_register_write_disabled = false; >> + >> reset_memory(s); >> } >> >> @@ -1587,6 +1619,25 @@ static const VMStateDescription vmstate_m25p80_aai_enable = { >> } >> }; >> >> +static bool m25p80_wp_level_srwd_needed(void *opaque) >> +{ >> + Flash *s = (Flash *)opaque; >> + >> + return !s->wp_level || s->status_register_write_disabled; >> +} >> + >> +static const VMStateDescription vmstate_m25p80_write_protect = { >> + .name = "m25p80/write_protect", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .needed = m25p80_wp_level_srwd_needed, >> + .fields = (VMStateField[]) { >> + VMSTATE_BOOL(wp_level, Flash), >> + VMSTATE_BOOL(status_register_write_disabled, Flash), >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> static const VMStateDescription vmstate_m25p80 = { >> .name = "m25p80", >> .version_id = 0, >> @@ -1618,6 +1669,7 @@ static const VMStateDescription vmstate_m25p80 = { >> .subsections = (const VMStateDescription * []) { >> &vmstate_m25p80_data_read_loop, >> &vmstate_m25p80_aai_enable, >> + &vmstate_m25p80_write_protect, >> NULL >> } >> }; >> -- >> 2.30.2 >> >>
On [2022 Jun 28] Tue 17:52:50, Cédric Le Goater wrote: > Alistair, Francisco, > > On 6/22/22 11:45, Francisco Iglesias wrote: > > On [2022 Jun 21] Tue 13:24:27, Iris Chen wrote: > > > From: Iris Chen <irischenlj@gmail.com> > > > > > > Signed-off-by: Iris Chen <irischenlj@gmail.com> > > > > Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com> > > I am planning to include this patch in the next aspeed PR if that's > OK with you. Sounds good to me Cédric! Best regards, Francisco > > Thanks, > > C. > > > > > > --- > > > Fixed .needed for subsection and suggestions from Francisco > > > > > > hw/block/m25p80.c | 82 ++++++++++++++++++++++++++++++++++++++--------- > > > 1 file changed, 67 insertions(+), 15 deletions(-) > > > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c > > > index 81ba3da4df..3045dda53b 100644 > > > --- a/hw/block/m25p80.c > > > +++ b/hw/block/m25p80.c > > > @@ -472,11 +472,13 @@ struct Flash { > > > uint8_t spansion_cr2v; > > > uint8_t spansion_cr3v; > > > uint8_t spansion_cr4v; > > > + bool wp_level; > > > bool write_enable; > > > bool four_bytes_address_mode; > > > bool reset_enable; > > > bool quad_enable; > > > bool aai_enable; > > > + bool status_register_write_disabled; > > > uint8_t ear; > > > int64_t dirty_page; > > > @@ -723,6 +725,8 @@ static void complete_collecting_data(Flash *s) > > > flash_erase(s, s->cur_addr, s->cmd_in_progress); > > > break; > > > case WRSR: > > > + s->status_register_write_disabled = extract32(s->data[0], 7, 1); > > > + > > > switch (get_man(s)) { > > > case MAN_SPANSION: > > > s->quad_enable = !!(s->data[1] & 0x02); > > > @@ -1165,22 +1169,34 @@ static void decode_new_cmd(Flash *s, uint32_t value) > > > break; > > > case WRSR: > > > - if (s->write_enable) { > > > - switch (get_man(s)) { > > > - case MAN_SPANSION: > > > - s->needed_bytes = 2; > > > - s->state = STATE_COLLECTING_DATA; > > > - break; > > > - case MAN_MACRONIX: > > > - s->needed_bytes = 2; > > > - s->state = STATE_COLLECTING_VAR_LEN_DATA; > > > - break; > > > - default: > > > - s->needed_bytes = 1; > > > - s->state = STATE_COLLECTING_DATA; > > > - } > > > - s->pos = 0; > > > + /* > > > + * If WP# is low and status_register_write_disabled is high, > > > + * status register writes are disabled. > > > + * This is also called "hardware protected mode" (HPM). All other > > > + * combinations of the two states are called "software protected mode" > > > + * (SPM), and status register writes are permitted. > > > + */ > > > + if ((s->wp_level == 0 && s->status_register_write_disabled) > > > + || !s->write_enable) { > > > + qemu_log_mask(LOG_GUEST_ERROR, > > > + "M25P80: Status register write is disabled!\n"); > > > + break; > > > + } > > > + > > > + switch (get_man(s)) { > > > + case MAN_SPANSION: > > > + s->needed_bytes = 2; > > > + s->state = STATE_COLLECTING_DATA; > > > + break; > > > + case MAN_MACRONIX: > > > + s->needed_bytes = 2; > > > + s->state = STATE_COLLECTING_VAR_LEN_DATA; > > > + break; > > > + default: > > > + s->needed_bytes = 1; > > > + s->state = STATE_COLLECTING_DATA; > > > } > > > + s->pos = 0; > > > break; > > > case WRDI: > > > @@ -1195,6 +1211,8 @@ 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; > > > + > > > if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) { > > > s->data[0] |= (!!s->quad_enable) << 6; > > > } > > > @@ -1484,6 +1502,14 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss, uint32_t tx) > > > return r; > > > } > > > +static void m25p80_write_protect_pin_irq_handler(void *opaque, int n, int level) > > > +{ > > > + Flash *s = M25P80(opaque); > > > + /* WP# is just a single pin. */ > > > + assert(n == 0); > > > + s->wp_level = !!level; > > > +} > > > + > > > static void m25p80_realize(SSIPeripheral *ss, Error **errp) > > > { > > > Flash *s = M25P80(ss); > > > @@ -1515,12 +1541,18 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp) > > > s->storage = blk_blockalign(NULL, s->size); > > > memset(s->storage, 0xFF, s->size); > > > } > > > + > > > + qdev_init_gpio_in_named(DEVICE(s), > > > + m25p80_write_protect_pin_irq_handler, "WP#", 1); > > > } > > > static void m25p80_reset(DeviceState *d) > > > { > > > Flash *s = M25P80(d); > > > + s->wp_level = true; > > > + s->status_register_write_disabled = false; > > > + > > > reset_memory(s); > > > } > > > @@ -1587,6 +1619,25 @@ static const VMStateDescription vmstate_m25p80_aai_enable = { > > > } > > > }; > > > +static bool m25p80_wp_level_srwd_needed(void *opaque) > > > +{ > > > + Flash *s = (Flash *)opaque; > > > + > > > + return !s->wp_level || s->status_register_write_disabled; > > > +} > > > + > > > +static const VMStateDescription vmstate_m25p80_write_protect = { > > > + .name = "m25p80/write_protect", > > > + .version_id = 1, > > > + .minimum_version_id = 1, > > > + .needed = m25p80_wp_level_srwd_needed, > > > + .fields = (VMStateField[]) { > > > + VMSTATE_BOOL(wp_level, Flash), > > > + VMSTATE_BOOL(status_register_write_disabled, Flash), > > > + VMSTATE_END_OF_LIST() > > > + } > > > +}; > > > + > > > static const VMStateDescription vmstate_m25p80 = { > > > .name = "m25p80", > > > .version_id = 0, > > > @@ -1618,6 +1669,7 @@ static const VMStateDescription vmstate_m25p80 = { > > > .subsections = (const VMStateDescription * []) { > > > &vmstate_m25p80_data_read_loop, > > > &vmstate_m25p80_aai_enable, > > > + &vmstate_m25p80_write_protect, > > > NULL > > > } > > > }; > > > -- > > > 2.30.2 > > > > > > >
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index 81ba3da4df..3045dda53b 100644 --- a/hw/block/m25p80.c +++ b/hw/block/m25p80.c @@ -472,11 +472,13 @@ struct Flash { uint8_t spansion_cr2v; uint8_t spansion_cr3v; uint8_t spansion_cr4v; + bool wp_level; bool write_enable; bool four_bytes_address_mode; bool reset_enable; bool quad_enable; bool aai_enable; + bool status_register_write_disabled; uint8_t ear; int64_t dirty_page; @@ -723,6 +725,8 @@ static void complete_collecting_data(Flash *s) flash_erase(s, s->cur_addr, s->cmd_in_progress); break; case WRSR: + s->status_register_write_disabled = extract32(s->data[0], 7, 1); + switch (get_man(s)) { case MAN_SPANSION: s->quad_enable = !!(s->data[1] & 0x02); @@ -1165,22 +1169,34 @@ static void decode_new_cmd(Flash *s, uint32_t value) break; case WRSR: - if (s->write_enable) { - switch (get_man(s)) { - case MAN_SPANSION: - s->needed_bytes = 2; - s->state = STATE_COLLECTING_DATA; - break; - case MAN_MACRONIX: - s->needed_bytes = 2; - s->state = STATE_COLLECTING_VAR_LEN_DATA; - break; - default: - s->needed_bytes = 1; - s->state = STATE_COLLECTING_DATA; - } - s->pos = 0; + /* + * If WP# is low and status_register_write_disabled is high, + * status register writes are disabled. + * This is also called "hardware protected mode" (HPM). All other + * combinations of the two states are called "software protected mode" + * (SPM), and status register writes are permitted. + */ + if ((s->wp_level == 0 && s->status_register_write_disabled) + || !s->write_enable) { + qemu_log_mask(LOG_GUEST_ERROR, + "M25P80: Status register write is disabled!\n"); + break; + } + + switch (get_man(s)) { + case MAN_SPANSION: + s->needed_bytes = 2; + s->state = STATE_COLLECTING_DATA; + break; + case MAN_MACRONIX: + s->needed_bytes = 2; + s->state = STATE_COLLECTING_VAR_LEN_DATA; + break; + default: + s->needed_bytes = 1; + s->state = STATE_COLLECTING_DATA; } + s->pos = 0; break; case WRDI: @@ -1195,6 +1211,8 @@ 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; + if (get_man(s) == MAN_MACRONIX || get_man(s) == MAN_ISSI) { s->data[0] |= (!!s->quad_enable) << 6; } @@ -1484,6 +1502,14 @@ static uint32_t m25p80_transfer8(SSIPeripheral *ss, uint32_t tx) return r; } +static void m25p80_write_protect_pin_irq_handler(void *opaque, int n, int level) +{ + Flash *s = M25P80(opaque); + /* WP# is just a single pin. */ + assert(n == 0); + s->wp_level = !!level; +} + static void m25p80_realize(SSIPeripheral *ss, Error **errp) { Flash *s = M25P80(ss); @@ -1515,12 +1541,18 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp) s->storage = blk_blockalign(NULL, s->size); memset(s->storage, 0xFF, s->size); } + + qdev_init_gpio_in_named(DEVICE(s), + m25p80_write_protect_pin_irq_handler, "WP#", 1); } static void m25p80_reset(DeviceState *d) { Flash *s = M25P80(d); + s->wp_level = true; + s->status_register_write_disabled = false; + reset_memory(s); } @@ -1587,6 +1619,25 @@ static const VMStateDescription vmstate_m25p80_aai_enable = { } }; +static bool m25p80_wp_level_srwd_needed(void *opaque) +{ + Flash *s = (Flash *)opaque; + + return !s->wp_level || s->status_register_write_disabled; +} + +static const VMStateDescription vmstate_m25p80_write_protect = { + .name = "m25p80/write_protect", + .version_id = 1, + .minimum_version_id = 1, + .needed = m25p80_wp_level_srwd_needed, + .fields = (VMStateField[]) { + VMSTATE_BOOL(wp_level, Flash), + VMSTATE_BOOL(status_register_write_disabled, Flash), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription vmstate_m25p80 = { .name = "m25p80", .version_id = 0, @@ -1618,6 +1669,7 @@ static const VMStateDescription vmstate_m25p80 = { .subsections = (const VMStateDescription * []) { &vmstate_m25p80_data_read_loop, &vmstate_m25p80_aai_enable, + &vmstate_m25p80_write_protect, NULL } };