Re: [PATCH 4/6] sparc32-ledma: don't reference nd_table directly within the device

[Mark Cave-Ayland]

On 21/09/2020 10:25, Markus Armbruster wrote:

> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:
> 
>> Instead use qdev_set_nic_properties() to configure the on-board NIC at the
>> sun4m machine level.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/dma/sparc32_dma.c |  5 -----
>>  hw/sparc/sun4m.c     | 21 +++++++++++++--------
>>  2 files changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
>> index 2cbe331959..b643b413c5 100644
>> --- a/hw/dma/sparc32_dma.c
>> +++ b/hw/dma/sparc32_dma.c
>> @@ -342,12 +342,7 @@ static void sparc32_ledma_device_realize(DeviceState 
>> *dev, Error **errp)
>>  {
>>      LEDMADeviceState *s = SPARC32_LEDMA_DEVICE(dev);
>>      SysBusPCNetState *lance = SYSBUS_PCNET(&s->lance);
>> -    NICInfo *nd = &nd_table[0];
>>  
>> -    /* FIXME use qdev NIC properties instead of nd_table[] */
>> -    qemu_check_nic_model(nd, TYPE_LANCE);
>> -
>> -    qdev_set_nic_properties(DEVICE(lance), nd);
>>      object_property_set_link(OBJECT(lance), "dma", OBJECT(dev), 
>> &error_abort);
>>      sysbus_realize(SYS_BUS_DEVICE(lance), &error_fatal);
>>  }
>> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
>> index 947b69d159..ed5f3ccd9f 100644
>> --- a/hw/sparc/sun4m.c
>> +++ b/hw/sparc/sun4m.c
>> @@ -319,7 +319,7 @@ static void *iommu_init(hwaddr addr, uint32_t version, 
>> qemu_irq irq)
>>  
>>  static void *sparc32_dma_init(hwaddr dma_base,
>>                                hwaddr esp_base, qemu_irq espdma_irq,
>> -                              hwaddr le_base, qemu_irq ledma_irq)
>> +                              hwaddr le_base, qemu_irq ledma_irq, NICInfo 
>> *nd)
>>  {
>>      DeviceState *dma;
>>      ESPDMADeviceState *espdma;
>> @@ -328,16 +328,11 @@ static void *sparc32_dma_init(hwaddr dma_base,
>>      SysBusPCNetState *lance;
>>  
>>      dma = qdev_new(TYPE_SPARC32_DMA);
>> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), &error_fatal);
>> -    sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);
>> -
>>      espdma = SPARC32_ESPDMA_DEVICE(object_resolve_path_component(
>>                                     OBJECT(dma), "espdma"));
>>      sysbus_connect_irq(SYS_BUS_DEVICE(espdma), 0, espdma_irq);
>>  
>>      esp = ESP(object_resolve_path_component(OBJECT(espdma), "esp"));
>> -    sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base);
>> -    scsi_bus_legacy_handle_cmdline(&esp->esp.bus);
>>  
>>      ledma = SPARC32_LEDMA_DEVICE(object_resolve_path_component(
>>                                   OBJECT(dma), "ledma"));
>> @@ -345,6 +340,14 @@ static void *sparc32_dma_init(hwaddr dma_base,
>>  
>>      lance = SYSBUS_PCNET(object_resolve_path_component(
>>                           OBJECT(ledma), "lance"));
>> +    qdev_set_nic_properties(DEVICE(lance), nd);
>> +
>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), &error_fatal);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);
>> +
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base);
>> +    scsi_bus_legacy_handle_cmdline(&esp->esp.bus);
>> +
>>      sysbus_mmio_map(SYS_BUS_DEVICE(lance), 0, le_base);
>>  
>>      return dma;
> 
> You delay a bit of work on devices @dma and @esp.  Can you explain why?

This is where it starts to get a little hazy: the sysbus_mmio_map() for the dma
device should be fine, since that's the container device for ledma and espdma 
devices
which is realised the line above.

The call to scsi_bus_legacy_handle_cmdline() is interesting - AFAICT if the esp
device within the dma device hasn't been realised then I get a crash, and 
that's why
I moved the legacy command line handling to after realise.

The lance and esp devices are embedded within ledma and espdma devices 
respectively,
but are actually sysbus devices because they can be used by other machines. I'm 
not
sure if lance is used anywhere else, but esp certainly is. Hence they are mapped
after the dma device is realised as it feels odd to attach devices to sysbus 
outside
of a machine init function.


ATB,

Mark.