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

[Thomas Huth <thuth@redhat.com>]

On 04/11/2020 20.29, Mark Cave-Ayland wrote:
> On 04/11/2020 12:47, 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?
> 
> (goes and looks)
> 
> Ah okay it appears to be because the object property link to the PIC is
> missing, which is to be expected as it is only present on the Mac machines.
> 
> With the latest round of QOM updates I can see the solution but it's
> probably a bit much now that we've reached rc-0. The easiest thing for the
> moment is to switch user_creatable back to false if this is causing an issue.

+1 for setting user_creatable back to false ... can you send a patch or
shall I prepare one?

> Just out of interest how did you find this? My new workflow involves a local
> "make check" with all ppc targets and a pass through Travis-CI and it didn't
> show up there for me (or indeed Peter's pre-merge tests).

I've found it with the scripts/device-crash-test script. (You currently need
to apply Eduardo's patch "Check if path is actually an executable file" on
top first to run it)

 Thomas