Re: [PATCH v2 5/6] macio: don't reference serial_hd() directly within the device

[Markus Armbruster <armbru@redhat.com>]

Thomas Huth <thuth@redhat.com> writes:

> On 04/11/2020 15.16, BALATON Zoltan wrote:
>> On Wed, 4 Nov 2020, Thomas Huth wrote:
>>> On 26/09/2020 16.02, Mark Cave-Ayland wrote:
>>>> Instead use qdev_prop_set_chr() to configure the ESCC serial chardevs at the
>>>> Mac Old World and New World machine level.
>>>>
>>>> Also remove the now obsolete comment referring to the use of serial_hd() and
>>>> the setting of user_creatable to false accordingly.
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>>  hw/misc/macio/macio.c | 4 ----
>>>>  hw/ppc/mac_newworld.c | 6 ++++++
>>>>  hw/ppc/mac_oldworld.c | 6 ++++++
>>>>  3 files changed, 12 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
>>>> index 679722628e..51368884d0 100644
>>>> --- a/hw/misc/macio/macio.c
>>>> +++ b/hw/misc/macio/macio.c
>>>> @@ -109,8 +109,6 @@ static void macio_common_realize(PCIDevice *d, Error **errp)
>>>>      qdev_prop_set_uint32(DEVICE(&s->escc), "disabled", 0);
>>>>      qdev_prop_set_uint32(DEVICE(&s->escc), "frequency", ESCC_CLOCK);
>>>>      qdev_prop_set_uint32(DEVICE(&s->escc), "it_shift", 4);
>>>> -    qdev_prop_set_chr(DEVICE(&s->escc), "chrA", serial_hd(0));
>>>> -    qdev_prop_set_chr(DEVICE(&s->escc), "chrB", serial_hd(1));
>>>>      qdev_prop_set_uint32(DEVICE(&s->escc), "chnBtype", escc_serial);
>>>>      qdev_prop_set_uint32(DEVICE(&s->escc), "chnAtype", escc_serial);
>>>>      if (!qdev_realize(DEVICE(&s->escc), BUS(&s->macio_bus), errp)) {
>>>> @@ -458,8 +456,6 @@ static void macio_class_init(ObjectClass *klass, void *data)
>>>>      k->class_id = PCI_CLASS_OTHERS << 8;
>>>>      device_class_set_props(dc, macio_properties);
>>>>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>>> -    /* Reason: Uses serial_hds in macio_instance_init */
>>>> -    dc->user_creatable = false;
>>>>  }
>>>
>>> Hi Mark,
>>>
>>> the macio device can now be used to crash QEMU:
>>>
>>> $ ./qemu-system-ppc -M sam460ex -device macio-newworld
>>> Segmentation fault (core dumped)
>>>
>>> I guess we should either restore the user_creatable flag or add some sanity
>>> checks elsewhere?
>> 
>> Looks like it needs to check if pic_dev is set:
[...]
>> Maybe something like:
>> 
>> if (!pic_dev) {
>>     error_setg(errp, "some meaningful error message");
>>     return;
>> }
>> 
>> before the sysbus_connect_irq calls but unless the user can set this via 
>> the command line somehow then keeping the user_creatable = false with 
>> comment adjusted to say that this device needs to be connected by board 
>> code is probably better.
>
> Yes, as far as I can see, there is no way a user could use these devices
> from the command line - the "pic" link has to be set up by code. So I'd also
> suggest to add the user_creatable = false back again.

When you do that, add a comment that explains why.  You might want to
examine existing comments for inspiration.