| Message ID | 20230702154838.722809-11-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | q800: add support for booting MacOS Classic - part 2 | expand |
On 2/7/23 17:48, Mark Cave-Ayland wrote: > This determines whether the Apple Sound Chip (ASC) is set to enhanced mode > (default) or to original mode. The real Q800 hardware used an EASC chip however > a lot of older software only works with the older ASC chip. > > Adding this as a machine parameter allows QEMU to be used as an developer aid > for testing and migrating code from ASC to EASC. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/m68k/q800.c | 30 +++++++++++++++++++++++++++++- > include/hw/m68k/q800.h | 1 + > 2 files changed, 30 insertions(+), 1 deletion(-) > +static bool q800_get_easc(Object *obj, Error **errp) > +{ > + Q800MachineState *ms = Q800_MACHINE(obj); > + > + return ms->easc; > +} Is the getter useful? Otherwise: Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 07/07/2023 09:29, Philippe Mathieu-Daudé wrote: > On 2/7/23 17:48, Mark Cave-Ayland wrote: >> This determines whether the Apple Sound Chip (ASC) is set to enhanced mode >> (default) or to original mode. The real Q800 hardware used an EASC chip however >> a lot of older software only works with the older ASC chip. >> >> Adding this as a machine parameter allows QEMU to be used as an developer aid >> for testing and migrating code from ASC to EASC. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/m68k/q800.c | 30 +++++++++++++++++++++++++++++- >> include/hw/m68k/q800.h | 1 + >> 2 files changed, 30 insertions(+), 1 deletion(-) > > >> +static bool q800_get_easc(Object *obj, Error **errp) >> +{ >> + Q800MachineState *ms = Q800_MACHINE(obj); >> + >> + return ms->easc; >> +} > > Is the getter useful? Otherwise: Isn't it a requirement? Otherwise I can see that if we decide to enumerate machine properties (similar as to how device properties appear in "info qtree") then it would be impossible to display its value. Certainly at the moment we consider that adding an object property to an underlying struct effectively makes it "public". > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> ATB, Mark.
On 8/9/23 08:54, Mark Cave-Ayland wrote: > On 07/07/2023 09:29, Philippe Mathieu-Daudé wrote: > >> On 2/7/23 17:48, Mark Cave-Ayland wrote: >>> This determines whether the Apple Sound Chip (ASC) is set to enhanced >>> mode >>> (default) or to original mode. The real Q800 hardware used an EASC >>> chip however >>> a lot of older software only works with the older ASC chip. >>> >>> Adding this as a machine parameter allows QEMU to be used as an >>> developer aid >>> for testing and migrating code from ASC to EASC. >>> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> --- >>> hw/m68k/q800.c | 30 +++++++++++++++++++++++++++++- >>> include/hw/m68k/q800.h | 1 + >>> 2 files changed, 30 insertions(+), 1 deletion(-) >> >> >>> +static bool q800_get_easc(Object *obj, Error **errp) >>> +{ >>> + Q800MachineState *ms = Q800_MACHINE(obj); >>> + >>> + return ms->easc; >>> +} >> >> Is the getter useful? Otherwise: > > Isn't it a requirement? Otherwise I can see that if we decide to > enumerate machine properties (similar as to how device properties appear > in "info qtree") then it would be impossible to display its value. > Certainly at the moment we consider that adding an object property to an > underlying struct effectively makes it "public". Just FYI this is not a requirement, per "qom/object.h": /** * object_property_add_bool: * @obj: the object to add a property to * @name: the name of the property * @get: the getter or NULL if the property is write-only. * @set: the setter or NULL if the property is read-only I'm not sure when we want a write-only QOM boolean property, so I genuinely ask, since I agree introspecting QOM object fields from the monitor is helpful. Regards, Phil.
On 08/09/2023 10:42, Philippe Mathieu-Daudé wrote: > On 8/9/23 08:54, Mark Cave-Ayland wrote: >> On 07/07/2023 09:29, Philippe Mathieu-Daudé wrote: >> >>> On 2/7/23 17:48, Mark Cave-Ayland wrote: >>>> This determines whether the Apple Sound Chip (ASC) is set to enhanced mode >>>> (default) or to original mode. The real Q800 hardware used an EASC chip however >>>> a lot of older software only works with the older ASC chip. >>>> >>>> Adding this as a machine parameter allows QEMU to be used as an developer aid >>>> for testing and migrating code from ASC to EASC. >>>> >>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>> --- >>>> hw/m68k/q800.c | 30 +++++++++++++++++++++++++++++- >>>> include/hw/m68k/q800.h | 1 + >>>> 2 files changed, 30 insertions(+), 1 deletion(-) >>> >>> >>>> +static bool q800_get_easc(Object *obj, Error **errp) >>>> +{ >>>> + Q800MachineState *ms = Q800_MACHINE(obj); >>>> + >>>> + return ms->easc; >>>> +} >>> >>> Is the getter useful? Otherwise: >> >> Isn't it a requirement? Otherwise I can see that if we decide to enumerate machine >> properties (similar as to how device properties appear in "info qtree") then it >> would be impossible to display its value. Certainly at the moment we consider that >> adding an object property to an underlying struct effectively makes it "public". > > Just FYI this is not a requirement, per "qom/object.h": > > /** > * object_property_add_bool: > * @obj: the object to add a property to > * @name: the name of the property > * @get: the getter or NULL if the property is write-only. > * @set: the setter or NULL if the property is read-only > > I'm not sure when we want a write-only QOM boolean property, so I > genuinely ask, since I agree introspecting QOM object fields from > the monitor is helpful. Agreed, although I'd be interested to hear if anyone can come up with a compelling use case for write-only properties. In that case I'll assume your R-B stands when I re-send the latest version of the series ;) ATB, Mark.
On 8/9/23 18:03, Mark Cave-Ayland wrote: > On 08/09/2023 10:42, Philippe Mathieu-Daudé wrote: > >> On 8/9/23 08:54, Mark Cave-Ayland wrote: >>> On 07/07/2023 09:29, Philippe Mathieu-Daudé wrote: >>> >>>> On 2/7/23 17:48, Mark Cave-Ayland wrote: >>>>> This determines whether the Apple Sound Chip (ASC) is set to >>>>> enhanced mode >>>>> (default) or to original mode. The real Q800 hardware used an EASC >>>>> chip however >>>>> a lot of older software only works with the older ASC chip. >>>>> >>>>> Adding this as a machine parameter allows QEMU to be used as an >>>>> developer aid >>>>> for testing and migrating code from ASC to EASC. >>>>> >>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>> --- >>>>> hw/m68k/q800.c | 30 +++++++++++++++++++++++++++++- >>>>> include/hw/m68k/q800.h | 1 + >>>>> 2 files changed, 30 insertions(+), 1 deletion(-) >>>> >>>> >>>>> +static bool q800_get_easc(Object *obj, Error **errp) >>>>> +{ >>>>> + Q800MachineState *ms = Q800_MACHINE(obj); >>>>> + >>>>> + return ms->easc; >>>>> +} >>>> >>>> Is the getter useful? Otherwise: >>> >>> Isn't it a requirement? Otherwise I can see that if we decide to >>> enumerate machine properties (similar as to how device properties >>> appear in "info qtree") then it would be impossible to display its >>> value. Certainly at the moment we consider that adding an object >>> property to an underlying struct effectively makes it "public". >> >> Just FYI this is not a requirement, per "qom/object.h": >> >> /** >> * object_property_add_bool: >> * @obj: the object to add a property to >> * @name: the name of the property >> * @get: the getter or NULL if the property is write-only. >> * @set: the setter or NULL if the property is read-only >> >> I'm not sure when we want a write-only QOM boolean property, so I >> genuinely ask, since I agree introspecting QOM object fields from >> the monitor is helpful. > > Agreed, although I'd be interested to hear if anyone can come up with a > compelling use case for write-only properties. In that case I'll assume > your R-B stands when I re-send the latest version of the series ;) Sure R-b stands, this discussion is outside of the scope of your series.
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > On 8/9/23 08:54, Mark Cave-Ayland wrote: >> On 07/07/2023 09:29, Philippe Mathieu-Daudé wrote: >> >>> On 2/7/23 17:48, Mark Cave-Ayland wrote: >>>> This determines whether the Apple Sound Chip (ASC) is set to enhanced mode >>>> (default) or to original mode. The real Q800 hardware used an EASC chip however >>>> a lot of older software only works with the older ASC chip. >>>> >>>> Adding this as a machine parameter allows QEMU to be used as an developer aid >>>> for testing and migrating code from ASC to EASC. >>>> >>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>> --- >>>> hw/m68k/q800.c | 30 +++++++++++++++++++++++++++++- >>>> include/hw/m68k/q800.h | 1 + >>>> 2 files changed, 30 insertions(+), 1 deletion(-) >>> >>> >>>> +static bool q800_get_easc(Object *obj, Error **errp) >>>> +{ >>>> + Q800MachineState *ms = Q800_MACHINE(obj); >>>> + >>>> + return ms->easc; >>>> +} >>> >>> Is the getter useful? Otherwise: >> Isn't it a requirement? Otherwise I can see that if we decide to enumerate machine properties (similar as to how device properties appear in "info qtree") then it would be impossible to display its value. Certainly at the moment we consider that adding an object property to an underlying struct effectively makes it "public". > > Just FYI this is not a requirement, per "qom/object.h": > > /** > * object_property_add_bool: > * @obj: the object to add a property to > * @name: the name of the property > * @get: the getter or NULL if the property is write-only. > * @set: the setter or NULL if the property is read-only > > I'm not sure when we want a write-only QOM boolean property, so I > genuinely ask, since I agree introspecting QOM object fields from > the monitor is helpful. I suspect write-only properties came out of QOM's generality curse. Do we have even one? QOM's design makes this somewhat to tell.
On 11/09/2023 06:15, Markus Armbruster wrote: > Philippe Mathieu-Daudé <philmd@linaro.org> writes: > >> On 8/9/23 08:54, Mark Cave-Ayland wrote: >>> On 07/07/2023 09:29, Philippe Mathieu-Daudé wrote: >>> >>>> On 2/7/23 17:48, Mark Cave-Ayland wrote: >>>>> This determines whether the Apple Sound Chip (ASC) is set to enhanced mode >>>>> (default) or to original mode. The real Q800 hardware used an EASC chip however >>>>> a lot of older software only works with the older ASC chip. >>>>> >>>>> Adding this as a machine parameter allows QEMU to be used as an developer aid >>>>> for testing and migrating code from ASC to EASC. >>>>> >>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>>>> --- >>>>> hw/m68k/q800.c | 30 +++++++++++++++++++++++++++++- >>>>> include/hw/m68k/q800.h | 1 + >>>>> 2 files changed, 30 insertions(+), 1 deletion(-) >>>> >>>> >>>>> +static bool q800_get_easc(Object *obj, Error **errp) >>>>> +{ >>>>> + Q800MachineState *ms = Q800_MACHINE(obj); >>>>> + >>>>> + return ms->easc; >>>>> +} >>>> >>>> Is the getter useful? Otherwise: >>> Isn't it a requirement? Otherwise I can see that if we decide to enumerate machine properties (similar as to how device properties appear in "info qtree") then it would be impossible to display its value. Certainly at the moment we consider that adding an object property to an underlying struct effectively makes it "public". >> >> Just FYI this is not a requirement, per "qom/object.h": >> >> /** >> * object_property_add_bool: >> * @obj: the object to add a property to >> * @name: the name of the property >> * @get: the getter or NULL if the property is write-only. >> * @set: the setter or NULL if the property is read-only >> >> I'm not sure when we want a write-only QOM boolean property, so I >> genuinely ask, since I agree introspecting QOM object fields from >> the monitor is helpful. > > I suspect write-only properties came out of QOM's generality curse. Do > we have even one? QOM's design makes this somewhat to tell. Good question. Given that it's towards the beginning of the next dev cycle, perhaps it is worth sending a patch to find out? ;) ATB, Mark.
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes: > On 11/09/2023 06:15, Markus Armbruster wrote: > >> Philippe Mathieu-Daudé <philmd@linaro.org> writes: [...] >>> I'm not sure when we want a write-only QOM boolean property, so I >>> genuinely ask, since I agree introspecting QOM object fields from >>> the monitor is helpful. >> >> I suspect write-only properties came out of QOM's generality curse. Do >> we have even one? QOM's design makes this somewhat to tell. > > Good question. Given that it's towards the beginning of the next dev cycle, perhaps it is worth sending a patch to find out? ;) Getting rid of unused generality / unnecessary complexity is good.
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c index ae07aa20ff..5ae7c37760 100644 --- a/hw/m68k/q800.c +++ b/hw/m68k/q800.c @@ -484,7 +484,8 @@ static void q800_machine_init(MachineState *machine) /* Apple Sound Chip */ object_initialize_child(OBJECT(machine), "asc", &m->asc, TYPE_ASC); - qdev_prop_set_uint8(DEVICE(&m->asc), "asctype", ASC_TYPE_EASC); + qdev_prop_set_uint8(DEVICE(&m->asc), "asctype", m->easc ? ASC_TYPE_EASC + : ASC_TYPE_ASC); sysbus = SYS_BUS_DEVICE(&m->asc); sysbus_realize_and_unref(sysbus, &error_fatal); memory_region_add_subregion(&m->macio, ASC_BASE - IO_BASE, @@ -674,6 +675,28 @@ static void q800_machine_init(MachineState *machine) } } +static bool q800_get_easc(Object *obj, Error **errp) +{ + Q800MachineState *ms = Q800_MACHINE(obj); + + return ms->easc; +} + +static void q800_set_easc(Object *obj, bool value, Error **errp) +{ + Q800MachineState *ms = Q800_MACHINE(obj); + + ms->easc = value; +} + +static void q800_init(Object *obj) +{ + Q800MachineState *ms = Q800_MACHINE(obj); + + /* Default to EASC */ + ms->easc = true; +} + static GlobalProperty hw_compat_q800[] = { { "scsi-hd", "quirk_mode_page_vendor_specific_apple", "on" }, { "scsi-hd", "vendor", " SEAGATE" }, @@ -706,11 +729,16 @@ static void q800_machine_class_init(ObjectClass *oc, void *data) mc->block_default_type = IF_SCSI; mc->default_ram_id = "m68k_mac.ram"; compat_props_add(mc->compat_props, hw_compat_q800, hw_compat_q800_len); + + object_class_property_add_bool(oc, "easc", q800_get_easc, q800_set_easc); + object_class_property_set_description(oc, "easc", + "Set to off to use ASC rather than EASC"); } static const TypeInfo q800_machine_typeinfo = { .name = MACHINE_TYPE_NAME("q800"), .parent = TYPE_MACHINE, + .instance_init = q800_init, .instance_size = sizeof(Q800MachineState), .class_init = q800_machine_class_init, }; diff --git a/include/hw/m68k/q800.h b/include/hw/m68k/q800.h index 790cf433f3..fbaacd88bd 100644 --- a/include/hw/m68k/q800.h +++ b/include/hw/m68k/q800.h @@ -47,6 +47,7 @@ struct Q800MachineState { MachineState parent_obj; + bool easc; M68kCPU cpu; MemoryRegion rom; GLUEState glue;
This determines whether the Apple Sound Chip (ASC) is set to enhanced mode (default) or to original mode. The real Q800 hardware used an EASC chip however a lot of older software only works with the older ASC chip. Adding this as a machine parameter allows QEMU to be used as an developer aid for testing and migrating code from ASC to EASC. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/m68k/q800.c | 30 +++++++++++++++++++++++++++++- include/hw/m68k/q800.h | 1 + 2 files changed, 30 insertions(+), 1 deletion(-)