Wow, that was quick!
Laszlo Ersek <ler...@redhat.com> 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 <jto...@redhat.com>
> Cc: John Snow <js...@redhat.com>
> Cc: Markus Armbruster <arm...@redhat.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Reported-by: Jan Tomko <jto...@redhat.com>
> Suggested-by: Markus Armbruster <arm...@redhat.com>
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> ---
> 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!
