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

[Mark Cave-Ayland]

On 21/06/2023 12:33, BALATON Zoltan wrote:

> 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.

This is just following our standard guidelines since MachineState is a QOM object of 
TYPE_MACHINE. Note that there are also a number of existing examples of this 
currently within the QEMU tree.


ATB,

Mark.