Re: [Qemu-devel] [PATCH v2 2/3] hw/sd: model a power-up delay, as a workaround for an EDK2 bug

[Peter Crosthwaite]

On Mon, Dec 21, 2015 at 2:25 PM, Andrew Baumann
<address@hidden> wrote:
>> From: address@hidden
>> [mailto:qemu-devel-
>> address@hidden On Behalf Of
>> Peter Crosthwaite
>> Sent: Monday, 21 December 2015 13:46
>> On Wed, Dec 16, 2015 at 11:02 AM, Andrew Baumann
>> <address@hidden> wrote:
>> > The SD spec for ACMD41 says that a zero argument is an "inquiry"
>> > ACMD41, which does not start initialisation and is used only for
>> > retrieving the OCR. However, Tianocore EDK2 (UEFI) has a bug [1]: it
>> > first sends an inquiry (zero) ACMD41. If that first request returns an
>> > OCR value with the power up bit (0x80000000) set, it assumes the card
>> > is ready and continues, leaving the card in the wrong state. (My
>> > assumption is that this works on hardware, because no real card is
>> > immediately powered up upon reset.)
>> >
>> > This change models a delay of 0.5ms from the first ACMD41 to the power
>> > being up. However, it also immediately sets the power on upon seeing a
>> > non-zero (non-enquiry) ACMD41. This speeds up UEFI boot; it should
>> > also account for guests that simply delay after card reset and then
>> > issue an ACMD41 that they expect will succeed.
> [...]
>> > @@ -449,6 +462,8 @@ static const VMStateDescription sd_vmstate = {
>> >      .fields = (VMStateField[]) {
>> >          VMSTATE_UINT32(mode, SDState),
>> >          VMSTATE_INT32(state, SDState),
>> > +        VMSTATE_UINT32(ocr, SDState),
>> > +        VMSTATE_TIMER_PTR(ocr_power_timer, SDState),
>>
>> If you change the VMSTATE layout, you need to bump the version. As
>> this is very common code, it may have stricter version bump
>> requirements. Last I knew however, there was a way to add new fields
>> at the end of VMSD without breaking backwards compatibility. Peter or
>> Juan may know more.
>
> I'll admit that I didn't think about these issues when adding the fields, but 
> after a (quick) look at vmstate_save_state() and vmstate_load_state(), they 
> seem to be using named fields in a json format, so I don't think the order of 
> fields should matter if we are adding new ones. However, if we want to be 
> able to migrate sd instances across across this change, then we'll need to 
> arrange for the OCR to appear already powered-on if we're coming from a 
> previous version. Does VMState have a way to do that? Essentially I just need 
> to specify a default value for the ocr field if coming from an old vmstate 
> version <= 1 that differs from the value set in sd_reset().
>

You can open code post_load logic as a callback, and I think you have
access to the image version from there.

Regards,
Peter

> Andrew