Recent SDHCI expect cards to support the v3.01 spec
to negociate lower I/O voltage. Select it by default.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
---
 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 a48010cfc1..d0a1d5db18 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2280,7 +2280,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
 
 static Property sd_properties[] = {
     DEFINE_PROP_UINT8("spec_version", SDState,
-                      spec_version, SD_PHY_SPECv2_00_VERS),
+                      spec_version, SD_PHY_SPECv3_01_VERS),
     DEFINE_PROP_DRIVE("drive", SDState, blk),
     /* We do not model the chip select pin, so allow the board to select
      * whether card should be in SSI or MMC/SD mode.  It is also up to the
-- 
2.41.0

On Fri, Jun 28, 2024 at 09:00:38AM +0200, Philippe Mathieu-Daudé wrote:
> Recent SDHCI expect cards to support the v3.01 spec
> to negociate lower I/O voltage. Select it by default.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> ---
>  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 a48010cfc1..d0a1d5db18 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2280,7 +2280,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
>  
>  static Property sd_properties[] = {
>      DEFINE_PROP_UINT8("spec_version", SDState,
> -                      spec_version, SD_PHY_SPECv2_00_VERS),
> +                      spec_version, SD_PHY_SPECv3_01_VERS),

Shouldn't such a change be tied to machine type versions ?


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 28/6/24 10:14, Daniel P. Berrangé wrote:
> On Fri, Jun 28, 2024 at 09:00:38AM +0200, Philippe Mathieu-Daudé wrote:
>> Recent SDHCI expect cards to support the v3.01 spec
>> to negociate lower I/O voltage. Select it by default.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   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 a48010cfc1..d0a1d5db18 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -2280,7 +2280,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
>>   
>>   static Property sd_properties[] = {
>>       DEFINE_PROP_UINT8("spec_version", SDState,
>> -                      spec_version, SD_PHY_SPECv2_00_VERS),
>> +                      spec_version, SD_PHY_SPECv3_01_VERS),
> 
> Shouldn't such a change be tied to machine type versions ?

I don't think so, SD cards are external to machines (like
CDROM you can use any brand). IOW machines only provide a
SD card reader, user can insert any card there.

SD specs are backward compatible. If host FW only knows
about v2.00 commands (spec is from 2006 btw) a v3.01 card
will works.

BTW latest spec is v9.10 from 2023. The more recent card
I bought supports spec v6.00 from 2018.

On Fri, Jun 28, 2024 at 11:19:52AM +0200, Philippe Mathieu-Daudé wrote:
> On 28/6/24 10:14, Daniel P. Berrangé wrote:
> > On Fri, Jun 28, 2024 at 09:00:38AM +0200, Philippe Mathieu-Daudé wrote:
> > > Recent SDHCI expect cards to support the v3.01 spec
> > > to negociate lower I/O voltage. Select it by default.
> > > 
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > Reviewed-by: Cédric Le Goater <clg@redhat.com>
> > > ---
> > >   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 a48010cfc1..d0a1d5db18 100644
> > > --- a/hw/sd/sd.c
> > > +++ b/hw/sd/sd.c
> > > @@ -2280,7 +2280,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
> > >   static Property sd_properties[] = {
> > >       DEFINE_PROP_UINT8("spec_version", SDState,
> > > -                      spec_version, SD_PHY_SPECv2_00_VERS),
> > > +                      spec_version, SD_PHY_SPECv3_01_VERS),
> > 
> > Shouldn't such a change be tied to machine type versions ?
> 
> I don't think so, SD cards are external to machines (like
> CDROM you can use any brand). IOW machines only provide a
> SD card reader, user can insert any card there.

Consider we release this in QEMU 9.1.0, and the user starts a VM with the
8.0 machine type. The guest will see an SD card supporting v3.

Now live migrate that guest to a host with QEMU 9.0.0, still using the 8.0
machine type. The guest will expect the SD card to still be v3, but QEMU
will internally be set to v2, and will return illegal cmd errors if the
guest tries to run v3 only cmds. Versioned machine types exist to prevent
this kind of problem.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 28/6/24 11:37, Daniel P. Berrangé wrote:
> On Fri, Jun 28, 2024 at 11:19:52AM +0200, Philippe Mathieu-Daudé wrote:
>> On 28/6/24 10:14, Daniel P. Berrangé wrote:
>>> On Fri, Jun 28, 2024 at 09:00:38AM +0200, Philippe Mathieu-Daudé wrote:
>>>> Recent SDHCI expect cards to support the v3.01 spec
>>>> to negociate lower I/O voltage. Select it by default.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> Reviewed-by: Cédric Le Goater <clg@redhat.com>
>>>> ---
>>>>    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 a48010cfc1..d0a1d5db18 100644
>>>> --- a/hw/sd/sd.c
>>>> +++ b/hw/sd/sd.c
>>>> @@ -2280,7 +2280,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
>>>>    static Property sd_properties[] = {
>>>>        DEFINE_PROP_UINT8("spec_version", SDState,
>>>> -                      spec_version, SD_PHY_SPECv2_00_VERS),
>>>> +                      spec_version, SD_PHY_SPECv3_01_VERS),
>>>
>>> Shouldn't such a change be tied to machine type versions ?
>>
>> I don't think so, SD cards are external to machines (like
>> CDROM you can use any brand). IOW machines only provide a
>> SD card reader, user can insert any card there.
> 
> Consider we release this in QEMU 9.1.0, and the user starts a VM with the
> 8.0 machine type. The guest will see an SD card supporting v3.
> 
> Now live migrate that guest to a host with QEMU 9.0.0, still using the 8.0
> machine type. The guest will expect the SD card to still be v3, but QEMU
> will internally be set to v2, and will return illegal cmd errors if the
> guest tries to run v3 only cmds. Versioned machine types exist to prevent
> this kind of problem.

I guess I get it. So (besides rewording the commit description)
what is missing is:

-- >8 --
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 655d75c21f..4377f943d5 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -37,7 +37,8 @@
  GlobalProperty hw_compat_9_0[] = {
      {"arm-cpu", "backcompat-cntfrq", "true" },
      {"scsi-disk-base", "migrate-emulated-scsi-request", "false" },
      {"vfio-pci", "skip-vsc-check", "false" },
+    {"sd-card", "spec_version", "2" },
  };
  const size_t hw_compat_9_0_len = G_N_ELEMENTS(hw_compat_9_0);
---

?