> From: Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com] > Sent: Monday, 21 December 2015 14:57 > On Mon, Dec 21, 2015 at 2:25 PM, Andrew Baumann > <andrew.baum...@microsoft.com> wrote: > >> From: qemu-devel- > bounces+andrew.baumann=microsoft....@nongnu.org > >> [mailto:qemu-devel- > >> bounces+andrew.baumann=microsoft....@nongnu.org] On Behalf Of > >> Peter Crosthwaite > >> Sent: Monday, 21 December 2015 13:46 > >> On Wed, Dec 16, 2015 at 11:02 AM, Andrew Baumann > >> <andrew.baum...@microsoft.com> 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.
So, how about something like this: +static int sd_vmstate_post_load(void *opaque, int version_id) +{ + SDState *sd = opaque; + + if (version_id < 2) { + /* prior versions did not save the OCR or model a power up + * delay, so we need to mirror this state */ + sd_ocr_powerup(sd); + } + + return 0; +} + static const VMStateDescription sd_vmstate = { .name = "sd-card", - .version_id = 1, + .version_id = 2, .minimum_version_id = 1, + .post_load = sd_vmstate_post_load, .fields = (VMStateField[]) { Any idea how I can easily test this? Sorry, I am not at all familiar with the vmstate/migration stuff. Is there a good/known working device model to test that uses SD? I guess I just do savevm on the old one and loadvm on the new one? Thanks, Andrew