Thomas Huth <th...@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-ayl...@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.
