| Message ID | 1449263769-2408-3-git-send-email-Andrew.Baumann@microsoft.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Dec 4, 2015 at 1:16 PM, Andrew Baumann <Andrew.Baumann@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 implements a simple workaround: the first time an ACMD41 > is issued, the OCR is returned without the power up bit set. Upon > subsequent reads, the OCR reports power up. > So mandating two ACMD41's to get a power-up signal would introduce a bug in guests with the opposite race condition. I.e. you are facilitating a guest that (incorrectly) assumes a lower bound on the card power up, but that could break a guest (incorrectly) assuming an upper bound. The other broken code would look something like this: do_card_power_on(); sleep(1); if (!get_acmd_41_power_up()) { abort(); /* Card did not power up in one second */ } granted, this is a bad guest, but I think it can be solved both ways by a more realistic model of the behaviour. Instead of using the ACMD41 as the trigger for the state change, use a QEMU timer and model an actual timed power up delay. Regards, Peter > [1] https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c#L279 > (This is the loop starting with "We need to wait for the MMC or SD > card is ready") > > Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> > --- > Obviously this is a bug that should be fixed in EDK2. However, this > initialisation appears to have been around for quite a while in EDK2 > (in various forms), and the fact that it has obviously worked with so > many real SD/MMC cards makes me think that it would be pragmatic to > have the workaround in QEMU as well. > > hw/sd/sd.c | 30 ++++++++++++++++++++++++++---- > 1 file changed, 26 insertions(+), 4 deletions(-) > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > index 98af8bc..31a25bc 100644 > --- a/hw/sd/sd.c > +++ b/hw/sd/sd.c > @@ -80,6 +80,7 @@ struct SDState { > uint32_t mode; /* current card mode, one of SDCardModes */ > int32_t state; /* current card state, one of SDCardStates */ > uint32_t ocr; > + bool ocr_initdelay; > uint8_t scr[8]; > uint8_t cid[16]; > uint8_t csd[16]; > @@ -195,8 +196,21 @@ static uint16_t sd_crc16(void *message, size_t width) > > static void sd_set_ocr(SDState *sd) > { > - /* All voltages OK, card power-up OK, Standard Capacity SD Memory Card */ > - sd->ocr = 0x80ffff00; > + /* All voltages OK, Standard Capacity SD Memory Card, not yet powered up */ > + sd->ocr = 0x00ffff00; > + sd->ocr_initdelay = false; > +} > + > +static bool sd_model_ocr_init_delay(SDState *sd) > +{ > + if (!sd->ocr_initdelay) { > + sd->ocr_initdelay = true; > + return false; > + } else { > + /* Set powered up bit in OCR */ > + sd->ocr |= 0x80000000; > + return true; > + } > } > > static void sd_set_scr(SDState *sd) > @@ -451,6 +465,7 @@ static const VMStateDescription sd_vmstate = { > .fields = (VMStateField[]) { > VMSTATE_UINT32(mode, SDState), > VMSTATE_INT32(state, SDState), > + VMSTATE_BOOL(ocr_initdelay, SDState), > VMSTATE_UINT8_ARRAY(cid, SDState, 16), > VMSTATE_UINT8_ARRAY(csd, SDState, 16), > VMSTATE_UINT16(rca, SDState), > @@ -1300,10 +1315,17 @@ static sd_rsp_type_t sd_app_command(SDState *sd, > case sd_idle_state: > /* We accept any voltage. 10000 V is nothing. > * > - * We don't model init delay so just advance straight to ready state > + * We model an init "delay" of one polling cycle, as a > + * workaround for around a bug in TianoCore EDK2 which > + * sends an initial zero ACMD41, but nevertheless assumes > + * that the card is in ready state as soon as it sees the > + * power up bit set. > + * > + * Once we've delayed, we advance straight to ready state > * unless it's an enquiry ACMD41 (bits 23:0 == 0). > */ > - if (req.arg & ACMD41_ENQUIRY_MASK) { > + if (sd_model_ocr_init_delay(sd) > + && (req.arg & ACMD41_ENQUIRY_MASK)) { > sd->state = sd_ready_state; > } > > -- > 2.5.3 > >
diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 98af8bc..31a25bc 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -80,6 +80,7 @@ struct SDState { uint32_t mode; /* current card mode, one of SDCardModes */ int32_t state; /* current card state, one of SDCardStates */ uint32_t ocr; + bool ocr_initdelay; uint8_t scr[8]; uint8_t cid[16]; uint8_t csd[16]; @@ -195,8 +196,21 @@ static uint16_t sd_crc16(void *message, size_t width) static void sd_set_ocr(SDState *sd) { - /* All voltages OK, card power-up OK, Standard Capacity SD Memory Card */ - sd->ocr = 0x80ffff00; + /* All voltages OK, Standard Capacity SD Memory Card, not yet powered up */ + sd->ocr = 0x00ffff00; + sd->ocr_initdelay = false; +} + +static bool sd_model_ocr_init_delay(SDState *sd) +{ + if (!sd->ocr_initdelay) { + sd->ocr_initdelay = true; + return false; + } else { + /* Set powered up bit in OCR */ + sd->ocr |= 0x80000000; + return true; + } } static void sd_set_scr(SDState *sd) @@ -451,6 +465,7 @@ static const VMStateDescription sd_vmstate = { .fields = (VMStateField[]) { VMSTATE_UINT32(mode, SDState), VMSTATE_INT32(state, SDState), + VMSTATE_BOOL(ocr_initdelay, SDState), VMSTATE_UINT8_ARRAY(cid, SDState, 16), VMSTATE_UINT8_ARRAY(csd, SDState, 16), VMSTATE_UINT16(rca, SDState), @@ -1300,10 +1315,17 @@ static sd_rsp_type_t sd_app_command(SDState *sd, case sd_idle_state: /* We accept any voltage. 10000 V is nothing. * - * We don't model init delay so just advance straight to ready state + * We model an init "delay" of one polling cycle, as a + * workaround for around a bug in TianoCore EDK2 which + * sends an initial zero ACMD41, but nevertheless assumes + * that the card is in ready state as soon as it sees the + * power up bit set. + * + * Once we've delayed, we advance straight to ready state * unless it's an enquiry ACMD41 (bits 23:0 == 0). */ - if (req.arg & ACMD41_ENQUIRY_MASK) { + if (sd_model_ocr_init_delay(sd) + && (req.arg & ACMD41_ENQUIRY_MASK)) { sd->state = sd_ready_state; }
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 implements a simple workaround: the first time an ACMD41 is issued, the OCR is returned without the power up bit set. Upon subsequent reads, the OCR reports power up. [1] https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c#L279 (This is the loop starting with "We need to wait for the MMC or SD card is ready") Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> --- Obviously this is a bug that should be fixed in EDK2. However, this initialisation appears to have been around for quite a while in EDK2 (in various forms), and the fact that it has obviously worked with so many real SD/MMC cards makes me think that it would be pragmatic to have the workaround in QEMU as well. hw/sd/sd.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-)