Re: [Qemu-devel] [edk2] [RFC PATCH] Distinguish between reset types

[Laszlo Ersek]

On 02/21/13 09:36, Paolo Bonzini wrote:
> Il 21/02/2013 02:10, Laszlo Ersek ha scritto:
>> OTOH if the keyboard reset gets soft in qemu, then OVMF's hard reset
>> (the above code) will break. Maybe I could cycle between 0xCF9 and 0x64
>> in ResetCold(), starting with 0xCF9.
> 
> Yes, that's the right thing to do.
> 
> Also, in QEMU you're doing:
> 
>     if (val & 4) {
>         qemu_system_reset_request();
>         return;
>     }
>     d->rcr = val & 2; /* keep System Reset type only */
> 
> It looks like d->rcr should be set first to match what hardware does
> (writing 0x6 causes a cold reset, and that's what you usually find in
> the ACPI tables).

I was thinking about that, yes. The language in the PIIX3 spec in fact
suggested something like this to me:

    if (val & 4) {
        if ((val & 2) || (d->rcr & 2)) {
            /* Either current write or the previous setting is asking
             * for hard reset. Don't bother storing any value since the
             * register will be cleared anyway during hard reset.
             */
            request_hard_reset();
            return;
        }

        /* Soft reset requested. The reset type is stored & survives the
         * reset, but bit 2 (value 4) can never read as 1.
         *
         * Actually if we reach this point,
         * (val & 2) == 0 && (d->rcr & 2) == 0, so there's nothing to
         * store. Other bits in d->rcr don't have to be cleared because
         * we never allow them to be set.
         */
        request_soft_reset();
        return;
    }

    /* No reset requested, just save the reset type for later. */
    d->rcr = val & 2;

(Regarding the logical disjunction before the hard reset, the PIIX3 spec
says "For example, to initiate a soft reset via the CF9 Reset Control
Register, write 00h then 04h, then read the CF9 register." Ie. you can't
just blindly write 0x04, because the previously set SRST could still
force a hard reset.)

Now add to the above code: "if we reset, we always do it the hard way"
-- I took that for an "axiom":

    if (val & 4) {
        if (true) {
            request_hard_reset();
            return;
        }
        /* not reached */
    }
    d->rcr = val & 2;

And then you get what I submitted in the patch.

(Plus of course if reset callbacks actually care about d->rcr, then the
"don't bother" thing isn't valid anymore, but when I submitted the
patch, I was just adding d->rcr so nobody could have cared.)

Laszlo