Re: [Qemu-devel] [PATCH 2/2] machine_parse(): list supported machine types in their registration order

[Marcel Apfelbaum]

On Mon, 2014-09-22 at 15:43 +0200, Laszlo Ersek wrote:
> On 09/22/14 14:50, Marcel Apfelbaum wrote:
> > On Mon, 2014-09-22 at 15:36 +0300, Michael S. Tsirkin wrote:
> >> On Mon, Sep 22, 2014 at 02:29:08PM +0200, Laszlo Ersek wrote:
> >>> On 09/22/14 14:04, Andreas Färber wrote:
> >>>> Am 22.09.2014 um 13:26 schrieb Laszlo Ersek:
> >>>>> Based on the registration order captured in the previous patch, we
> >>>>> sort the ad-hoc list printed for
> >>>>>
> >>>>>   qemu-system-XXXX -M \?
> >>>>
> >>>> Agree that the order is worth sanitizing. I would however argue that
> >>>> registration order is not entirely stable either if you think of
> >>>> non-PC cases where there's dozens of source files registering one
> >>>> machine each. I would therefore propose alphabetical order as we do
> >>>> for QOM'ified CPUs.
> >>>
> >>> I did mention alphabetical order in the message of patch #1 -- in fact
> >>> that was what I implemented first:
> >>>
> >>>> The first idea to restore ordering is to sort the ad-hoc list in
> >>>> machine_parse() by "MachineClass.name". Such a name-based ordering
> >>>> would have to be reverse, so that more recent versioned machine types
> >>>> appear higher on the list than older versioned machine types (eg. with
> >>>> qemu-system-x86_64). However, such a reverse sort wreaks havoc between
> >>>> non-versioned machine types (such as those of qemu-system-aarch64).
> >>>
> >>> Simply put, it looks very ugly. Namely, if you sort it in alphabetically
> >>> *ascending* order, then for the x86_64 target, you get:
> >>>
> >>>> isapc                ISA-only PC
> >>>> none                 empty machine
> >>>> pc                   Standard PC (i440FX + PIIX, 1996) (alias of 
> >>>> pc-i440fx-2.2)
> >>>> pc-0.10              Standard PC (i440FX + PIIX, 1996)
> >>>> pc-0.11              Standard PC (i440FX + PIIX, 1996)
> >>>> pc-0.12              Standard PC (i440FX + PIIX, 1996)
> >>>> pc-0.13              Standard PC (i440FX + PIIX, 1996)
> >>>> pc-0.14              Standard PC (i440FX + PIIX, 1996)
> >>>> pc-0.15              Standard PC (i440FX + PIIX, 1996)
> >>>> pc-1.0               Standard PC (i440FX + PIIX, 1996)
> >>>> pc-1.1               Standard PC (i440FX + PIIX, 1996)
> >>>> pc-1.2               Standard PC (i440FX + PIIX, 1996)
> >>>> pc-1.3               Standard PC (i440FX + PIIX, 1996)
> >>>> pc-i440fx-1.4        Standard PC (i440FX + PIIX, 1996)
> >>>> pc-i440fx-1.5        Standard PC (i440FX + PIIX, 1996)
> >>>> pc-i440fx-1.6        Standard PC (i440FX + PIIX, 1996)
> >>>> pc-i440fx-1.7        Standard PC (i440FX + PIIX, 1996)
> >>>> pc-i440fx-2.0        Standard PC (i440FX + PIIX, 1996)
> >>>> pc-i440fx-2.1        Standard PC (i440FX + PIIX, 1996)
> >>>> pc-i440fx-2.2        Standard PC (i440FX + PIIX, 1996) (default)
> >>>> pc-q35-1.4           Standard PC (Q35 + ICH9, 2009)
> >>>> pc-q35-1.5           Standard PC (Q35 + ICH9, 2009)
> >>>> pc-q35-1.6           Standard PC (Q35 + ICH9, 2009)
> >>>> pc-q35-1.7           Standard PC (Q35 + ICH9, 2009)
> >>>> pc-q35-2.0           Standard PC (Q35 + ICH9, 2009)
> >>>> pc-q35-2.1           Standard PC (Q35 + ICH9, 2009)
> >>>> pc-q35-2.2           Standard PC (Q35 + ICH9, 2009)
> >>>> q35                  Standard PC (Q35 + ICH9, 2009) (alias of pc-q35-2.2)
> >>>
> >>> which is very bad / unusual. Okay, so let's sort it in alphabetically
> >>> descending order:
> >>>
> >>>> q35                  Standard PC (Q35 + ICH9, 2009) (alias of pc-q35-2.2)
> >>>> pc-q35-2.2           Standard PC (Q35 + ICH9, 2009)
> >>>> pc-q35-2.1           Standard PC (Q35 + ICH9, 2009)
> >>>> pc-q35-2.0           Standard PC (Q35 + ICH9, 2009)
> >>>> pc-q35-1.7           Standard PC (Q35 + ICH9, 2009)
> >>>> pc-q35-1.6           Standard PC (Q35 + ICH9, 2009)
> >>>> pc-q35-1.5           Standard PC (Q35 + ICH9, 2009)
> >>>> pc-q35-1.4           Standard PC (Q35 + ICH9, 2009)
> >>>> pc-i440fx-2.2        Standard PC (i440FX + PIIX, 1996) (default)
> >>>> pc-i440fx-2.1        Standard PC (i440FX + PIIX, 1996)
> >>>> pc-i440fx-2.0        Standard PC (i440FX + PIIX, 1996)
> >>>> pc-i440fx-1.7        Standard PC (i440FX + PIIX, 1996)
> >>>> pc-i440fx-1.6        Standard PC (i440FX + PIIX, 1996)
> >>>> pc-i440fx-1.5        Standard PC (i440FX + PIIX, 1996)
> >>>> pc-i440fx-1.4        Standard PC (i440FX + PIIX, 1996)
> >>>> pc-1.3               Standard PC (i440FX + PIIX, 1996)
> >>>> pc-1.2               Standard PC (i440FX + PIIX, 1996)
> >>>> pc-1.1               Standard PC (i440FX + PIIX, 1996)
> >>>> pc-1.0               Standard PC (i440FX + PIIX, 1996)
> >>>> pc-0.15              Standard PC (i440FX + PIIX, 1996)
> >>>> pc-0.14              Standard PC (i440FX + PIIX, 1996)
> >>>> pc-0.13              Standard PC (i440FX + PIIX, 1996)
> >>>> pc-0.12              Standard PC (i440FX + PIIX, 1996)
> >>>> pc-0.11              Standard PC (i440FX + PIIX, 1996)
> >>>> pc-0.10              Standard PC (i440FX + PIIX, 1996)
> >>>> pc                   Standard PC (i440FX + PIIX, 1996) (alias of 
> >>>> pc-i440fx-2.2)
> >>>> none                 empty machine
> >>>> isapc                ISA-only PC
> >>>
> >>> Okay, this is certainly bearable. However, let's see what the reverse
> >>> alpha sort does to aarch64:
> >>>
> >>>> z2                   Zipit Z2 (PXA27x)
> >>>> xilinx-zynq-a9       Xilinx Zynq Platform Baseboard for Cortex-A9
> >>>> virt                 ARM Virtual Machine
> >>>> vexpress-a9          ARM Versatile Express for Cortex-A9
> >>>> vexpress-a15         ARM Versatile Express for Cortex-A15
> >>>> versatilepb          ARM Versatile/PB (ARM926EJ-S)
> >>>> versatileab          ARM Versatile/AB (ARM926EJ-S)
> >>>> verdex               Gumstix Verdex (PXA270)
> >>>> tosa                 Tosa PDA (PXA255)
> >>>> terrier              Terrier PDA (PXA270)
> >>>> sx1-v1               Siemens SX1 (OMAP310) V1
> >>>> sx1                  Siemens SX1 (OMAP310) V2
> >>>> spitz                Spitz PDA (PXA270)
> >>>> smdkc210             Samsung SMDKC210 board (Exynos4210)
> >>>> realview-pbx-a9      ARM RealView Platform Baseboard Explore for 
> >>>> Cortex-A9
> >>>> realview-pb-a8       ARM RealView Platform Baseboard for Cortex-A8
> >>>> realview-eb-mpcore   ARM RealView Emulation Baseboard (ARM11MPCore)
> >>>> realview-eb          ARM RealView Emulation Baseboard (ARM926EJ-S)
> >>>> nuri                 Samsung NURI board (Exynos4210)
> >>>> none                 empty machine
> >>>> n810                 Nokia N810 tablet aka. RX-44 (OMAP2420)
> >>>> n800                 Nokia N800 tablet aka. RX-34 (OMAP2420)
> >>>> musicpal             Marvell 88w8618 / MusicPal (ARM926EJ-S)
> >>>> midway               Calxeda Midway (ECX-2000)
> >>>> mainstone            Mainstone II (PXA27x)
> >>>> lm3s811evb           Stellaris LM3S811EVB
> >>>> lm3s6965evb          Stellaris LM3S6965EVB
> >>>> kzm                  ARM KZM Emulation Baseboard (ARM1136)
> >>>> integratorcp         ARM Integrator/CP (ARM926EJ-S)
> >>>> highbank             Calxeda Highbank (ECX-1000)
> >>>> cubieboard           cubietech cubieboard
> >>>> connex               Gumstix Connex (PXA255)
> >>>> collie               Collie PDA (SA-1110)
> >>>> cheetah              Palm Tungsten|E aka. Cheetah PDA (OMAP310)
> >>>> canon-a1100          Canon PowerShot A1100 IS
> >>>> borzoi               Borzoi PDA (PXA270)
> >>>> akita                Akita PDA (PXA270)
> >>>
> >>> Ugh? The reverse sort is painfully obvious, and the user wonders why it
> >>> is necessary. Why is z2 at the top, and akita at the bottom? Why is n810
> >>> above n800 and not below it? And so on.
> >>>
> >>> Instead, the patchset restores the behavior that had been visible before
> >>> commit 261747f1: programmer-controlled logical order *within* clusters,
> >>> and unspecified order *between* clusters.
> >>
> >> We could add a cluster name in the API.
> >> Use that for sort between clusters only.
> >> Hmm?
> > Since the machines have now QOM types we can leverage the types
> > as "clusters", and each machine hierarchy can have its own
> > sorting interface to arrange their descendants however is desired.
> > 
> > It will look like:
> > 1. Get all machine types that implement "sort" interface.
> > 2. Define a sort order between them (a interface property ?)
> > 3. Ask for each of them to sort its descendants.
> > 
> > In this way we have maximum control on the output and
> > give each cluster a proper way to decide the ordering
> > based on the semantics and not alphabetical/registering order. 
> 
> Guys -- no offense meant, but this architecture astronautics &
> bikeshedding is exactly why I, in my foresight, haven't assigned the
> RHBZ in question to myself, and why I had unsubscribed from qemu-devel
> months ago (or at least stopped mail delivery, can't recall exactly).
> 
> Here's the situation. Commit 261747f1 has broken some behavior of qemu
> that is technically trivial / irrelevant, but a nuisance for users.
> Noone to my knowledge had ever complained about the machtype listing
> previously to commit 261747f1. Therefore the pre-261747f1 behavior is
> reasonable and sound as a user-visible target to restore. The patchset I
> posted is a technically simple solution that behaves identically to qemu
> prior to 261747f1. The diffstat of the patchset is "20 insertions".
> 
> In the span of three followups in the thread, we've arrived at an
> abstract sort interface, just so the user can see a more or less
> traditionally ordered machtype list with -M '?'.
> 
> I think this is completely wrong; both specifically for the issue at
> hand -- creeping object orientation -- and in general as a development /
> community process  -- qemu is now basically untouchable for people who
> want (or must) spend their time primarily on different things.
> 
> I agree that projects shouldn't take drive-by patches from irresponsible
> contributors. At the same time I don't think I would qualify as one. My
> patchset is not one that I throw over the wall. This is the solution
> that I consider technically good, and what I have time for.
> 
> I guess it would be easier for me to understand this level of
> nit-picking / over-engineering if my patchset ran a high risk of
> regressions in some area. Whereas I'm only trying to undo an already
> in-tree regression, as non-intrusively as possible
> 
> I only wanted to help out Marcel with RHBZ 1145042, because the breakage
> in the visible machtype ordering is a consequence of his earlier patch,
> and it seemed easy enough to address. If my patchset is not acceptable
> for technical reasons, I can live with that (albeit in disagreement);
> Marcel or someone else will hopefully fix the issue then.
Hi Laszlo,
This was only a suggestion, trying to discuss a possible "cluster" solution,
and nothing more.
It is not related to the patch submission which I find it reasonable
and I completely agree that it solves a regression introduced by my patches.

I'll be more careful in the feature to emphasize clearly when I am
discussing the patches and when I am only throwing ideas for others to comment.


Thanks,
Marcel

> 
> Cheers
> Laszlo
>