Re: [Qemu-devel] [PATCH 2/3] hw/i386/pc: reflect any FDC @ ioport 0x3f0 in the CMOS

[Markus Armbruster]

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 any ISA
> FDC devices, created implicitly (by board code) or explicitly, and set the
> CMOS accordingly to the ISA FDC(s) with iobase=0x3f0:
>
> - if there is no such FDC, report both drives absent,
> - if there is exactly one such FDC, report its drives in the CMOS,
> - if there are more than one such FDCs, then pick one (it is not specified
>   which one), and print a warning about the ambiguity.
>
> Cc: Jan Tomko <address@hidden>
> Cc: John Snow <address@hidden>
> Cc: Markus Armbruster <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Cc: Blue Swirl <address@hidden>
> Reported-by: Jan Tomko <address@hidden>
> Suggested-by: Markus Armbruster <address@hidden>
> Signed-off-by: Laszlo Ersek <address@hidden>
> ---
>
> Notes:
>     v1:
>     
>     - Look for ISA FDC with iobase=0x3f0, not distinguishing the board
>       default ISA FDC at all [Markus]. Handling the board-default ISA FDC
>       uniformly requires scanning the "/unattached" container.
>     
>     - Checkpatch barfs at this patch (it doesn't recognize the compound
>       literal (const char *[]) { ... }). I didn't care; this pattern is
>       widely used in the tree. Just run
>     
>       git grep -F '*[])'
>     
>       CC'd Blue Swirl for this reason.
>
>  hw/i386/pc.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 4a835be..bdb2d2d 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -335,6 +335,37 @@ typedef struct pc_cmos_init_late_arg {
>      BusState *idebus[2];
>  } 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;
> +    uint32_t iobase;
> +    Error *local_err = NULL;
> +
> +    fdc = object_dynamic_cast(obj, TYPE_ISA_FDC);
> +    if (!fdc) {
> +        return 0;
> +    }
> +
> +    iobase = object_property_get_int(obj, "iobase", &local_err);
> +    if (iobase != 0x3f0) {
> +        error_free(local_err);
> +        return 0;
> +    }

Works, because object_property_get_int() returns -1 on error, and
(uint32_t)-1 != 0x3f0.

Perhaps (local_err || iobase != 0x3f0) would be more obvious.

> +
> +    if (state->floppy) {
> +        state->multiple = true;
> +    } else {
> +        state->floppy = ISA_DEVICE(obj);
> +    }
> +    return 0;
> +}
> +
>  static void pc_cmos_init_late(void *opaque)
>  {
>      pc_cmos_init_late_arg *arg = opaque;
> @@ -343,6 +374,9 @@ static void pc_cmos_init_late(void *opaque)
>      int8_t heads, sectors;
>      int val;
>      int i, trans;
> +    const char **container_path;
> +    Object *container;
> +    CheckFdcState state = { 0 };
>  
>      val = 0;
>      if (ide_get_geometry(arg->idebus[0], 0,
> @@ -372,6 +406,26 @@ static void pc_cmos_init_late(void *opaque)
>      }
>      rtc_set_memory(s, 0x39, val);
>  
> +    /*
> +     * Locate the FDC at IO address 0x3f0, and configure the CMOS registers
> +     * accordingly.
> +     */
> +    container_path = (const char *[]) { "/unattached", "/peripheral",
> +                                        "/peripheral-anon", NULL };
> +    do {
> +        container = container_get(qdev_get_machine(), *container_path);
> +        object_child_foreach(container, check_fdc, &state);
> +        ++container_path;
> +    } while (*container_path);

I'd prefer a for loop, because it keeps the loop control in one place.
Stepping an index instead of the only pointer lets us use an
old-fashioned array initializer instead of a fancy compound literal.

    static const char *container_path[] = {
        "/unattached", "/peripheral", "/peripheral-anon"
    };

    for (i = 0; i < ARRAY_SIZE(container_path); i++) {
        container = container_get(qdev_get_machine(), container_path[i]);
        object_child_foreach(container, check_fdc, &state);
    }

Matter of taste.  I like old-fashioned and non-fancy :)

> +
> +    if (state.multiple) {
> +        error_report("warning: multiple floppy disk controllers with "
> +                     "iobase=0x3f0 have been found;\n"
> +                     "the one being picked for CMOS setup might not reflect "
> +                     "your intent");
> +    }
> +    pc_cmos_init_floppy(s, state.floppy);
> +
>      qemu_unregister_reset(pc_cmos_init_late, opaque);
>  }
>  
> @@ -441,9 +495,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t 
> above_4g_mem_size,
>      val |= 0x02; /* FPU is there */
>      val |= 0x04; /* PS/2 mouse installed */
>      rtc_set_memory(s, REG_EQUIPMENT_BYTE, val);
> -    pc_cmos_init_floppy(s, floppy);
>  
> -    /* hard drives */
> +    /* hard drives and FDC */
>      arg.rtc_state = s;
>      arg.idebus[0] = idebus0;
>      arg.idebus[1] = idebus1;