| Message ID | 87h9q4tc4d.fsf@blackfin.pond.sub.org |
|---|---|
| State | New |
| Headers | show |
On 06/19/15 09:01, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > >> From: Laszlo Ersek <lersek@redhat.com> >> >> It is Very annoying to carry forward an outdatEd coNtroller with a mOdern >> Machine type. >> >> Hence, let us not instantiate the FDC when all of the following apply: >> - the machine type is pc-q35-2.4 or later, >> - "-device isa-fdc" is not passed on the command line (nor in the config >> file), >> - no "-drive if=floppy,..." is requested. >> >> Cc: Markus Armbruster <armbru@redhat.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Gerd Hoffmann <kraxel@redhat.com> >> Cc: John Snow <jsnow@redhat.com> >> Cc: "Gabriel L. Somlo" <gsomlo@gmail.com> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Kevin Wolf <kwolf@redhat.com> >> Cc: qemu-block@nongnu.org >> Suggested-by: Markus Armbruster <armbru@redhat.com> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> Acked-by: Paolo Bonzini <pbonzini@redhat.com> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> Reviewed-by: Markus Armbruster <armbru@redhat.com> >> --- >> hw/i386/pc_q35.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c >> index 9f036c8..66220b3 100644 >> --- a/hw/i386/pc_q35.c >> +++ b/hw/i386/pc_q35.c >> @@ -392,6 +392,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m) >> pc_q35_machine_options(m); >> m->default_machine_opts = "firmware=bios-256k.bin"; >> m->default_display = "std"; >> + m->no_floppy = 1; >> m->alias = "q35"; >> } > > Botched merge, see [PATCH] q35: Re-enable FDC on pc-q35-2.3 and older. > > Your merge is closer to a rewrite than to conflict resolution, rendering > my R-by totally meaningless. > > In the future, please either fully test such a merge, or ask the > submitter / reviewers to review and test. Yes, please. While I *was* Cc'd on this patch in the pull request, I didn't review it. I never expect PULLs to amount to such intrusive changes -- I treat them only as a hint that my patch is going in -- so if such changes are necessary (because the original patch doesn't apply any longer), then please ask me to rebase, or give me an explicit heads-up that my verification / testing is needed. Thanks! Laszlo > For comparison, this is the patch that got actually reviewed and tested: > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index ad014e7..671ae69 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -424,7 +424,8 @@ static void pc_q35_init_1_4(MachineState *machine) > #define PC_Q35_2_4_MACHINE_OPTIONS \ > PC_Q35_MACHINE_OPTIONS, \ > .default_machine_opts = "firmware=bios-256k.bin", \ > - .default_display = "std" > + .default_display = "std", \ > + .no_floppy = 1 > > static QEMUMachine pc_q35_machine_v2_4 = { > PC_Q35_2_4_MACHINE_OPTIONS, > @@ -433,7 +434,10 @@ static QEMUMachine pc_q35_machine_v2_4 = { > .init = pc_q35_init, > }; > > -#define PC_Q35_2_3_MACHINE_OPTIONS PC_Q35_2_4_MACHINE_OPTIONS > +#define PC_Q35_2_3_MACHINE_OPTIONS \ > + PC_Q35_MACHINE_OPTIONS, \ > + .default_machine_opts = "firmware=bios-256k.bin", \ > + .default_display = "std" > > static QEMUMachine pc_q35_machine_v2_3 = { > PC_Q35_2_3_MACHINE_OPTIONS, >
On Fri, Jun 19, 2015 at 02:52:16PM +0200, Laszlo Ersek wrote: > > Your merge is closer to a rewrite than to conflict resolution, rendering > > my R-by totally meaningless. > > > > In the future, please either fully test such a merge, or ask the > > submitter / reviewers to review and test. > > Yes, please. While I *was* Cc'd on this patch in the pull request, I > didn't review it. I never expect PULLs to amount to such intrusive > changes -- I treat them only as a hint that my patch is going in -- so > if such changes are necessary (because the original patch doesn't apply > any longer), then please ask me to rebase, or give me an explicit > heads-up that my verification / testing is needed. > > Thanks! > Laszlo Yes, this wasn't intentional. I don't really remember what went here, sorry. Once thing we do need is a unit test for the floppy. the easiest way to do it is probably by booting a guest, using the bios tables test infrastructure.
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index ad014e7..671ae69 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -424,7 +424,8 @@ static void pc_q35_init_1_4(MachineState *machine) #define PC_Q35_2_4_MACHINE_OPTIONS \ PC_Q35_MACHINE_OPTIONS, \ .default_machine_opts = "firmware=bios-256k.bin", \ - .default_display = "std" + .default_display = "std", \ + .no_floppy = 1 static QEMUMachine pc_q35_machine_v2_4 = { PC_Q35_2_4_MACHINE_OPTIONS, @@ -433,7 +434,10 @@ static QEMUMachine pc_q35_machine_v2_4 = { .init = pc_q35_init, }; -#define PC_Q35_2_3_MACHINE_OPTIONS PC_Q35_2_4_MACHINE_OPTIONS +#define PC_Q35_2_3_MACHINE_OPTIONS \ + PC_Q35_MACHINE_OPTIONS, \ + .default_machine_opts = "firmware=bios-256k.bin", \ + .default_display = "std" static QEMUMachine pc_q35_machine_v2_3 = { PC_Q35_2_3_MACHINE_OPTIONS,