[PATCH] hw/sd: print bad s->arglen in unexpected response

[PATCH] hw/sd: print bad s->arglen in unexpected response

[Ben Dooks]

If we get "ssi_sd: error: Unexpected response to cmd" then having
the bad s->arglen would be useful debug and does not add any complexity
to the code.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 hw/sd/ssi-sd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 6c90a86ab4..f1441d2c97 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -183,7 +183,7 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
                 s->response[0] = 1;
                 memcpy(&s->response[1], longresp, 4);
             } else if (s->arglen != 4) {
-                BADF("Unexpected response to cmd %d\n", s->cmd);
+                BADF("Unexpected response to cmd %d, arglen=%d\n", s->cmd, s->arglen);
                 /* Illegal command is about as near as we can get.  */
                 s->arglen = 1;
                 s->response[0] = 4;
-- 
2.37.2.352.g3c44437643

Re: [PATCH] hw/sd: print bad s->arglen in unexpected response

[Alex Bennée]

Ben Dooks <ben.dooks@codethink.co.uk> writes:

(Add maintainers to CC)

You should get your patch workflow to use scripts/get_maintainer.pl so
they get CC'd and reduces the chance of it being missed in the fire-hose
of qemu-devel.

> If we get "ssi_sd: error: Unexpected response to cmd" then having
> the bad s->arglen would be useful debug and does not add any complexity
> to the code.

Generally we should be removing the old-style DPRINTF debug and
replacing them with tracepoints where they are warranted. The main
problem with the old style DPRINTF's is the format strings tend to
bitrot because they are not enabled by default.

>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  hw/sd/ssi-sd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index 6c90a86ab4..f1441d2c97 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -183,7 +183,7 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
>                  s->response[0] = 1;
>                  memcpy(&s->response[1], longresp, 4);
>              } else if (s->arglen != 4) {
> -                BADF("Unexpected response to cmd %d\n", s->cmd);
> +                BADF("Unexpected response to cmd %d, arglen=%d\n", s->cmd, s->arglen);

That said BADF is defined in both cases (although the exit(1) for the
debug leg is a bit aggressive). Is this an error of the guest
miss-programming the device with invalid data?

There could be an argument for using:

  qemu_log_mask(LOG_GUEST_ERROR, "Unexpected response to cmd %d, arglen=%d\n", s->cmd, s->arglen);

instead.

Phillipe WDYT?

>                  /* Illegal command is about as near as we can get.  */
>                  s->arglen = 1;
>                  s->response[0] = 4;

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] hw/sd: print bad s->arglen in unexpected response

[Peter Maydell]

On Wed, 23 Jul 2025 at 13:47, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Ben Dooks <ben.dooks@codethink.co.uk> writes:
>
> (Add maintainers to CC)
>
> You should get your patch workflow to use scripts/get_maintainer.pl so
> they get CC'd and reduces the chance of it being missed in the fire-hose
> of qemu-devel.
>
> > If we get "ssi_sd: error: Unexpected response to cmd" then having
> > the bad s->arglen would be useful debug and does not add any complexity
> > to the code.
>
> Generally we should be removing the old-style DPRINTF debug and
> replacing them with tracepoints where they are warranted. The main
> problem with the old style DPRINTF's is the format strings tend to
> bitrot because they are not enabled by default.
>
> >
> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> > ---
> >  hw/sd/ssi-sd.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> > index 6c90a86ab4..f1441d2c97 100644
> > --- a/hw/sd/ssi-sd.c
> > +++ b/hw/sd/ssi-sd.c
> > @@ -183,7 +183,7 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
> >                  s->response[0] = 1;
> >                  memcpy(&s->response[1], longresp, 4);
> >              } else if (s->arglen != 4) {
> > -                BADF("Unexpected response to cmd %d\n", s->cmd);
> > +                BADF("Unexpected response to cmd %d, arglen=%d\n", s->cmd, s->arglen);
>
> That said BADF is defined in both cases (although the exit(1) for the
> debug leg is a bit aggressive). Is this an error of the guest
> miss-programming the device with invalid data?
>
> There could be an argument for using:
>
>   qemu_log_mask(LOG_GUEST_ERROR, "Unexpected response to cmd %d, arglen=%d\n", s->cmd, s->arglen);
>
> instead.

This unexpected response comes from QEMU's SD card emulation,
so I'm not sure to what extent LOG_GUEST_ERROR is appropriate.
Is this triggered by the guest doing something silly, or by
a bug in QEMU itself?

I agree that the BADF() macro is rather a legacy relic
(you can see it in a handful of other files too). exit(1)
seems unlikely to be very useful behaviour even if you are
trying to debug this code, and we have slowly been weeding
out places where QEMU just barfs on error cases.

thanks
-- PMM

Re: [PATCH] hw/sd: print bad s->arglen in unexpected response

[Ben Dooks]

On 23/07/2025 14:38, Peter Maydell wrote:
> On Wed, 23 Jul 2025 at 13:47, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Ben Dooks <ben.dooks@codethink.co.uk> writes:
>>
>> (Add maintainers to CC)
>>
>> You should get your patch workflow to use scripts/get_maintainer.pl so
>> they get CC'd and reduces the chance of it being missed in the fire-hose
>> of qemu-devel.

$ git show | ./scripts/get_maintainer.pl
"Philippe Mathieu-Daudé" <philmd@linaro.org> (odd fixer:SD (Secure Card))
Bin Meng <bmeng.cn@gmail.com> (odd fixer:SD (Secure Card))
qemu-block@nongnu.org (open list:SD (Secure Card))
qemu-devel@nongnu.org (open list:All patches CC here)

so, no maintainers listed, just sent to the two lists.

>>> If we get "ssi_sd: error: Unexpected response to cmd" then having
>>> the bad s->arglen would be useful debug and does not add any complexity
>>> to the code.
>>
>> Generally we should be removing the old-style DPRINTF debug and
>> replacing them with tracepoints where they are warranted. The main
>> problem with the old style DPRINTF's is the format strings tend to
>> bitrot because they are not enabled by default.
>>
>>>
>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>> ---
>>>   hw/sd/ssi-sd.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
>>> index 6c90a86ab4..f1441d2c97 100644
>>> --- a/hw/sd/ssi-sd.c
>>> +++ b/hw/sd/ssi-sd.c
>>> @@ -183,7 +183,7 @@ static uint32_t ssi_sd_transfer(SSIPeripheral *dev, uint32_t val)
>>>                   s->response[0] = 1;
>>>                   memcpy(&s->response[1], longresp, 4);
>>>               } else if (s->arglen != 4) {
>>> -                BADF("Unexpected response to cmd %d\n", s->cmd);
>>> +                BADF("Unexpected response to cmd %d, arglen=%d\n", s->cmd, s->arglen);
>>
>> That said BADF is defined in both cases (although the exit(1) for the
>> debug leg is a bit aggressive). Is this an error of the guest
>> miss-programming the device with invalid data?
>>
>> There could be an argument for using:
>>
>>    qemu_log_mask(LOG_GUEST_ERROR, "Unexpected response to cmd %d, arglen=%d\n", s->cmd, s->arglen);
>>
>> instead.
> 
> This unexpected response comes from QEMU's SD card emulation,
> so I'm not sure to what extent LOG_GUEST_ERROR is appropriate.
> Is this triggered by the guest doing something silly, or by
> a bug in QEMU itself?

I am currently trying to track down two errors with mmc-spi.

The first looks like u-boot is sending a couple of CMDs (9, 10)
in the wrong state (currently this works however with a real SD
card) so I have a tmp-fix in to just ignore the two checks in
these and u-boot is now working.

I'm also getting multiple versions of linux failing with QEMU10
on Debian/testing and my own close to the current git tree with
Linux and CMD13...

[    0.579845] EXT4-fs (mmcblk0): INFO: recovery required on readonly 
filesystem
[    0.580222] EXT4-fs (mmcblk0): write access will be enabled during 
recovery
ssi_sd: error: Unexpected response to cmd 13, arglen=16
ssi_sd: error: Unexpected response to cmd 13, arglen=16
ssi_sd: error: Unexpected response to cmd 13, arglen=16
ssi_sd: error: Unexpected response to cmd 13, arglen=16

Then the system stops working.

Systems are riscv sifive_u and my own cva6 machine

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [PATCH] hw/sd: print bad s->arglen in unexpected response

[Philippe Mathieu-Daudé]

Hi Ben,

On 23/7/25 16:55, Ben Dooks wrote:

> I am currently trying to track down two errors with mmc-spi.
> 
> The first looks like u-boot is sending a couple of CMDs (9, 10)
> in the wrong state (currently this works however with a real SD
> card) so I have a tmp-fix in to just ignore the two checks in
> these and u-boot is now working.
> 
> I'm also getting multiple versions of linux failing with QEMU10
> on Debian/testing and my own close to the current git tree with
> Linux and CMD13...
> 
> [    0.579845] EXT4-fs (mmcblk0): INFO: recovery required on readonly 
> filesystem
> [    0.580222] EXT4-fs (mmcblk0): write access will be enabled during 
> recovery
> ssi_sd: error: Unexpected response to cmd 13, arglen=16
> ssi_sd: error: Unexpected response to cmd 13, arglen=16
> ssi_sd: error: Unexpected response to cmd 13, arglen=16
> ssi_sd: error: Unexpected response to cmd 13, arglen=16

CMD13's arg len should be 2 in SSI mode.

> Then the system stops working.
> 
> Systems are riscv sifive_u and my own cva6 machine
Do you mind sharing a reproducer?

Even better if contributed as a functional test :)
(see tests/functional/test_arm_sx1.py for example).

Regards,

Phil.

Re: [PATCH] hw/sd: print bad s->arglen in unexpected response

[Ben Dooks]

On 23/07/2025 17:30, Philippe Mathieu-Daudé wrote:
> Hi Ben,
> 
> On 23/7/25 16:55, Ben Dooks wrote:
> 
>> I am currently trying to track down two errors with mmc-spi.
>>
>> The first looks like u-boot is sending a couple of CMDs (9, 10)
>> in the wrong state (currently this works however with a real SD
>> card) so I have a tmp-fix in to just ignore the two checks in
>> these and u-boot is now working.
>>
>> I'm also getting multiple versions of linux failing with QEMU10
>> on Debian/testing and my own close to the current git tree with
>> Linux and CMD13...
>>
>> [    0.579845] EXT4-fs (mmcblk0): INFO: recovery required on readonly 
>> filesystem
>> [    0.580222] EXT4-fs (mmcblk0): write access will be enabled during 
>> recovery
>> ssi_sd: error: Unexpected response to cmd 13, arglen=16
>> ssi_sd: error: Unexpected response to cmd 13, arglen=16
>> ssi_sd: error: Unexpected response to cmd 13, arglen=16
>> ssi_sd: error: Unexpected response to cmd 13, arglen=16
> 
> CMD13's arg len should be 2 in SSI mode.
> 
>> Then the system stops working.
>>
>> Systems are riscv sifive_u and my own cva6 machine
> Do you mind sharing a reproducer?

Currently buildroot riscv64 with the sifive_u machine is the
test case.

It looks to be a qemu issue, v8.x works, v9.0 works and v9.1
does not. Currently doing a bisect to see if there's an obvious
issue to point at.

> Even better if contributed as a functional test :)
> (see tests/functional/test_arm_sx1.py for example).

I'll see, but I am off next week at FOSSY and then not sure if
this project will be continued.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [PATCH] hw/sd: print bad s->arglen in unexpected response

[Ben Dooks]

On 23/07/2025 17:30, Philippe Mathieu-Daudé wrote:
> Hi Ben,
> 
> On 23/7/25 16:55, Ben Dooks wrote:
> 
>> I am currently trying to track down two errors with mmc-spi.
>>
>> The first looks like u-boot is sending a couple of CMDs (9, 10)
>> in the wrong state (currently this works however with a real SD
>> card) so I have a tmp-fix in to just ignore the two checks in
>> these and u-boot is now working.
>>
>> I'm also getting multiple versions of linux failing with QEMU10
>> on Debian/testing and my own close to the current git tree with
>> Linux and CMD13...
>>
>> [    0.579845] EXT4-fs (mmcblk0): INFO: recovery required on readonly 
>> filesystem
>> [    0.580222] EXT4-fs (mmcblk0): write access will be enabled during 
>> recovery
>> ssi_sd: error: Unexpected response to cmd 13, arglen=16
>> ssi_sd: error: Unexpected response to cmd 13, arglen=16
>> ssi_sd: error: Unexpected response to cmd 13, arglen=16
>> ssi_sd: error: Unexpected response to cmd 13, arglen=16
> 
> CMD13's arg len should be 2 in SSI mode.
> 
>> Then the system stops working.
>>
>> Systems are riscv sifive_u and my own cva6 machine
> Do you mind sharing a reproducer?
> 
> Even better if contributed as a functional test :)
> (see tests/functional/test_arm_sx1.py for example).
> 
> Regards,


So looks like running a kernel directly on sifive_u is stopping
because of this commit:

commit da954d0e32444f122a41c24948d4d1c718bf66d4
Author: Philippe Mathieu-Daudé <philmd@linaro.org>
Date:   Wed Jun 12 23:16:40 2024 +0200

     hw/sd/sdcard: Add spi_cmd_SEND_CSD/CID handlers (CMD9 & CMD10)

     Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
     Tested-by: Cédric Le Goater <clg@redhat.com>
     Reviewed-by: Cédric Le Goater <clg@redhat.com>
     Message-Id: <20240628070216.92609-52-philmd@linaro.org>

  hw/sd/sd.c | 50 ++++++++++++++++++++------------------------------
  1 file changed, 20 insertions(+), 30 deletions(-)

I think the CMD13 might be something else so going to look into that
now.

FYI, qemu command line is

$qemu -M sifive_u -m 1G -s \
	  -kernel ./linux-kernel-mmc/arch/riscv/boot/Image \
	  -append "root=/dev/mmcblk0 rootwait earlycon=sbi conole=ttyS0,115200" \
	  -drive file=./rootfs-le.tmp.ext2,if=sd \
	  -netdev user,id=usernet,hostfwd=tcp::5556-:22 \
	  -nographic 2>&1


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html