Re: [PATCH v4 03/24] q800: introduce Q800MachineState

[BALATON Zoltan]

On Wed, 21 Jun 2023, Mark Cave-Ayland wrote:
> This provides an overall container and owner for Machine-related objects such
> as MemoryRegions.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> MAINTAINERS            |  1 +
> hw/m68k/q800.c         |  2 ++
> include/hw/m68k/q800.h | 40 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 43 insertions(+)
> create mode 100644 include/hw/m68k/q800.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 88b5a7ee0a..748a66fbaa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1236,6 +1236,7 @@ F: include/hw/misc/mac_via.h
> F: include/hw/nubus/*
> F: include/hw/display/macfb.h
> F: include/hw/block/swim.h
> +F: include/hw/m68k/q800.h
>
> virt
> M: Laurent Vivier <laurent@vivier.eu>
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index 465c510c18..c0256c8a90 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -38,6 +38,7 @@
> #include "standard-headers/asm-m68k/bootinfo.h"
> #include "standard-headers/asm-m68k/bootinfo-mac.h"
> #include "bootinfo.h"
> +#include "hw/m68k/q800.h"
> #include "hw/misc/mac_via.h"
> #include "hw/input/adb.h"
> #include "hw/nubus/mac-nubus-bridge.h"
> @@ -749,6 +750,7 @@ static void q800_machine_class_init(ObjectClass *oc, void 
> *data)
> static const TypeInfo q800_machine_typeinfo = {
>     .name       = MACHINE_TYPE_NAME("q800"),
>     .parent     = TYPE_MACHINE,
> +    .instance_size = sizeof(Q800MachineState),
>     .class_init = q800_machine_class_init,
> };
>
> diff --git a/include/hw/m68k/q800.h b/include/hw/m68k/q800.h
> new file mode 100644
> index 0000000000..f3bc17aa1b
> --- /dev/null
> +++ b/include/hw/m68k/q800.h

Why is this defined in a public header? Moving struct definitions of 
devices to allow them to be embedded in other structs makes sense but is 
there ever a reason to embed a machine state anywhere else than using it 
in q800.c? I don't think so, thus to preserve locality and save some lines 
in this series I think this machine state should just be in q800.c like I 
have similar struct in pegasos2.c. It may only make sense to put it in a 
header if q800.c was split up to multiple files but even then it should be 
a local header in hw/m68k and not a public header in my opinion.

Regards,
BALATON Zoltan

> @@ -0,0 +1,40 @@
> +/*
> + * QEMU Motorla 680x0 Macintosh hardware System Emulator
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef HW_Q800_H
> +#define HW_Q800_H
> +
> +#include "hw/boards.h"
> +#include "qom/object.h"
> +
> +/*
> + * The main Q800 machine
> + */
> +
> +struct Q800MachineState {
> +    MachineState parent_obj;
> +};
> +
> +#define TYPE_Q800_MACHINE MACHINE_TYPE_NAME("q800")
> +OBJECT_DECLARE_SIMPLE_TYPE(Q800MachineState, Q800_MACHINE)
> +
> +#endif