hw/sd/sdcard: Correct SCR 'DATA_STAT_AFTER_ERASE' bit

[PATCH-for-10.2] hw/sd/sdcard: Correct SCR 'DATA_STAT_AFTER_ERASE' bit

Posted by Philippe Mathieu-Daudé 2 months, 2 weeks ago

As a 'SD card vendor', QEMU chose to fill blocks with '1'
during erase operation. Update the DATA_STAT_AFTER_ERASE
SCR bit appropriately.

Fixes: 818a5cdcfcf ("hw/sd: sd: Actually perform the erase operation")
Reported-by: Christian Speich <c.speich@avm.de>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 40a75a43ffb..d35537702b2 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -446,7 +446,7 @@ static void sd_set_scr(SDState *sd)
     /* reserved for manufacturer usage */
     sd->scr[4] = 0x00;
     sd->scr[5] = 0x00;
-    sd->scr[6] = 0x00;
+    sd->scr[6] = 1 << 7;        /* Data after an erase operation is 0xff */
     sd->scr[7] = 0x00;
 }
 
-- 
2.51.0

Re: [PATCH-for-10.2] hw/sd/sdcard: Correct SCR 'DATA_STAT_AFTER_ERASE' bit

Posted by Christian Speich 2 months, 2 weeks ago

On Mon, Nov 24, 2025 at 05:20:08AM +0100, Philippe Mathieu-Daudé wrote:
> As a 'SD card vendor', QEMU chose to fill blocks with '1'
> during erase operation. Update the DATA_STAT_AFTER_ERASE
> SCR bit appropriately.

Would it be appropiate to let the guest know whether or not the
DATA_STAT_AFTER_ERASE bit is reliable?

Considering, erase-to-zero may become an option with[1] a guest
needs to actually consider this SCR bit.

CID contains a product revision field, maybe that could be
bumped?

Greetings,
Christian

[1] https://lore.kernel.org/qemu-devel/20250919-sdcard-performance-b4-v1-3-e1037e481a19@avm.de/

> 
> Fixes: 818a5cdcfcf ("hw/sd: sd: Actually perform the erase operation")
> Reported-by: Christian Speich <c.speich@avm.de>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/sd/sd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 40a75a43ffb..d35537702b2 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -446,7 +446,7 @@ static void sd_set_scr(SDState *sd)
>      /* reserved for manufacturer usage */
>      sd->scr[4] = 0x00;
>      sd->scr[5] = 0x00;
> -    sd->scr[6] = 0x00;
> +    sd->scr[6] = 1 << 7;        /* Data after an erase operation is 0xff */
>      sd->scr[7] = 0x00;
>  }
>  
> -- 
> 2.51.0
> 
>

Re: [PATCH-for-10.2] hw/sd/sdcard: Correct SCR 'DATA_STAT_AFTER_ERASE' bit

Posted by Christian Speich 2 months, 2 weeks ago

On Mon, Nov 24, 2025 at 05:20:08AM +0100, Philippe Mathieu-Daudé wrote:
> As a 'SD card vendor', QEMU chose to fill blocks with '1'
> during erase operation. Update the DATA_STAT_AFTER_ERASE
> SCR bit appropriately.
> 
> Fixes: 818a5cdcfcf ("hw/sd: sd: Actually perform the erase operation")
> Reported-by: Christian Speich <c.speich@avm.de>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/sd/sd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 40a75a43ffb..d35537702b2 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -446,7 +446,7 @@ static void sd_set_scr(SDState *sd)
>      /* reserved for manufacturer usage */
>      sd->scr[4] = 0x00;
>      sd->scr[5] = 0x00;
> -    sd->scr[6] = 0x00;
> +    sd->scr[6] = 1 << 7;        /* Data after an erase operation is 0xff */

Although the rest of this function works on magic values, maybe it would be
usefull to introduce a DATA_STAT_AFTER_ERASE define for this bit? This would
make it easier to search for it in the spec (or other projects, google).

Otherwise, lgtm. Reviewed-by: Christian Speich <c.speich@avm.de>

Greetings,
Christian

>      sd->scr[7] = 0x00;
>  }
>  
> -- 
> 2.51.0
> 
>

Re: [PATCH-for-10.2] hw/sd/sdcard: Correct SCR 'DATA_STAT_AFTER_ERASE' bit

Posted by Christian Speich 2 months, 2 weeks ago

On Tue, Nov 25, 2025 at 10:31:45AM +0100, Christian Speich wrote:
> On Mon, Nov 24, 2025 at 05:20:08AM +0100, Philippe Mathieu-Daudé wrote:
> > As a 'SD card vendor', QEMU chose to fill blocks with '1'
> > during erase operation. Update the DATA_STAT_AFTER_ERASE
> > SCR bit appropriately.
> > 
> > Fixes: 818a5cdcfcf ("hw/sd: sd: Actually perform the erase operation")
> > Reported-by: Christian Speich <c.speich@avm.de>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >  hw/sd/sd.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > index 40a75a43ffb..d35537702b2 100644
> > --- a/hw/sd/sd.c
> > +++ b/hw/sd/sd.c
> > @@ -446,7 +446,7 @@ static void sd_set_scr(SDState *sd)
> >      /* reserved for manufacturer usage */
> >      sd->scr[4] = 0x00;
> >      sd->scr[5] = 0x00;
> > -    sd->scr[6] = 0x00;
> > +    sd->scr[6] = 1 << 7;        /* Data after an erase operation is 0xff */
> 
> Although the rest of this function works on magic values, maybe it would be
> usefull to introduce a DATA_STAT_AFTER_ERASE define for this bit? This would
> make it easier to search for it in the spec (or other projects, google).
> 
> Otherwise, lgtm. Reviewed-by: Christian Speich <c.speich@avm.de>

Sorry for the many mails, I'm not really familiar with SD/QEMU.

I've think this is wrong. Looking at the other fields set in sd_set_scr
I think scr[0] is the highest byte/bit not the lowest.

This should put DATA_STAT_AFTER_ERASE into scr[1] bit 7?

Greetings,
Christian

> 
> Greetings,
> Christian
> 
> >      sd->scr[7] = 0x00;
> >  }
> >  
> > -- 
> > 2.51.0
> > 
> > 
>