Re: [Qemu-devel] [RFC 2/2] hw/i386/pc: reflect an explicitly created, sole FDC in the CMOS

[Markus Armbruster]

Wow, that was quick!

Laszlo Ersek <address@hidden> writes:

> With the pc-q35-2.4 machine type, if the user creates an ISA FDC manually:
>
>   -device isa-fdc,driveA=drive-fdc0-0-0 \
>   -drive file=...,if=none,id=drive-fdc0-0-0,format=raw
>
> then the board-default FDC will be skipped, and only the explicitly
> requested FDC will exist. qtree-wise, this is correct; however such an FDC
> is currently not registered in the CMOS, because that code is only reached
> for the board-default FDC.
>
> The pc_cmos_init_late() one-shot reset handler -- one-shot because the
> CMOS is not reprogrammed during warm reset -- should search for a single,
> explicitly created ISA FDC device, if there has been no board default, and
> reprogram the CMOS if found.

I don't think we want "single".  We want the isa-fdc at I/O address
0x3f0.

Test case:

    -M q35 -device isa-fdc,id=fdc1 -device isa-fdc,id=fdc2,iobase=0x370

should work, and CMOS should be set according to fdc1.

> Cc: Jan Tomko <address@hidden>
> Cc: John Snow <address@hidden>
> Cc: Markus Armbruster <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Reported-by: Jan Tomko <address@hidden>
> Suggested-by: Markus Armbruster <address@hidden>
> Signed-off-by: Laszlo Ersek <address@hidden>
> ---
>  hw/i386/pc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 1ca0cdd..47a3082 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -333,8 +333,30 @@ static void pc_cmos_init_floppy(ISADevice *rtc_state, 
> ISADevice *floppy)
>  typedef struct pc_cmos_init_late_arg {
>      ISADevice *rtc_state;
>      BusState *idebus[2];
> +    ISADevice *board_floppy;
>  } pc_cmos_init_late_arg;
>  
> +typedef struct check_fdc_state {
> +    ISADevice *floppy;
> +    bool multiple;
> +} CheckFdcState;
> +
> +static int check_fdc(Object *obj, void *opaque)
> +{
> +    CheckFdcState *state = opaque;
> +    Object *fdc;
> +
> +    fdc = object_dynamic_cast(obj, TYPE_ISA_FDC);
> +    if (fdc) {
> +        if (state->floppy) {
> +            state->multiple = true;
> +        } else {
> +            state->floppy = ISA_DEVICE(obj);
> +        }
> +    }

Something like

    fdc = object_dynamic_cast(obj, TYPE_ISA_FDC);
    if (fdc
        && object_property_get_int(obj, "iobase", &error_abort) == 0x3f0) {
        assert(!state->floppy);
        state->floppy = ISA_DEVICE(obj);
    }

The &error_abort is probably wrong, but I trust you get the general
idea.

The assertion is only valid if we actually prevent multiple ISA devices
claiming the same I/O port.  Checking... crap, multiple -device isa-fdc
are accepted.  No idea which one actually gets the port.

Two solutions:

* Find a way to determine whether the device is in possession of the
  port.  If it is, pick it and stop the search.  If not, continue the
  search.

  Alternative: always continue the search, assert that we pick at most
  one (because if we pick more, our "is in posession of the port" test
  is broken).

* Pick one that's easy to pick.  Might be the wrong one when there's
  more than one.  Feel free to print a warning then.  I'd happily accept
  such a stupid solution, as long as its stupidity is reasonably
  documented.

> +    return 0;
> +}
> +
>  static void pc_cmos_init_late(void *opaque)
>  {
>      pc_cmos_init_late_arg *arg = opaque;
> @@ -372,6 +394,29 @@ static void pc_cmos_init_late(void *opaque)
>      }
>      rtc_set_memory(s, 0x39, val);
>  
> +    /*
> +     * If the board initialization code created no FDC, but exactly one FDC 
> has
> +     * been created since then explicitly, then we configure the CMOS 
> registers
> +     * now, in accordance with that one FDC.
> +     */
> +    if (arg->board_floppy == NULL) {
> +        static const char * const paths[] = { "/peripheral",
> +            "/peripheral-anon", NULL };
> +        const char * const * path;
> +        CheckFdcState state = { 0 };
> +
> +        for (path = paths; *path; ++path) {
> +            Object *container;
> +
> +            container = container_get(qdev_get_machine(), *path);
> +            object_child_foreach(container, check_fdc, &state);
> +        }
> +
> +        if (state.floppy && !state.multiple) {
> +            pc_cmos_init_floppy(s, state.floppy);
> +        }
> +    }
> +
>      qemu_unregister_reset(pc_cmos_init_late, opaque);
>  }

This assumes that the onboard isa-fdc wins the fight over the I/O port.

Perhaps passing the onboard isa-fdc to check_fdc() would be cleaner.

> @@ -447,6 +492,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t 
> above_4g_mem_size,
>      arg.rtc_state = s;
>      arg.idebus[0] = idebus0;
>      arg.idebus[1] = idebus1;
> +    arg.board_floppy = floppy;
>      qemu_register_reset(pc_cmos_init_late, &arg);
>  }

Thanks for tackling this!