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

Mark Cave-Ayland posted 6 patches 5 years, 1 month ago
Maintainers: Artyom Tarasenko <atar4qemu@gmail.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, David Gibson <david@gibson.dropbear.id.au>
There is a newer version of this series
[PATCH 4/6] sparc32-ledma: don't reference nd_table directly within the device
Posted by Mark Cave-Ayland 5 years, 1 month ago
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;
@@ -854,6 +857,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
     unsigned int max_cpus = machine->smp.max_cpus;
     Object *ram_memdev = object_resolve_path_type(machine->ram_memdev_id,
                                                   TYPE_MEMORY_BACKEND, NULL);
+    NICInfo *nd = &nd_table[0];
 
     if (machine->ram_size > hwdef->max_mem) {
         error_report("Too much memory for this machine: %" PRId64 ","
@@ -914,9 +918,10 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
                         hwdef->iommu_pad_base, hwdef->iommu_pad_len);
     }
 
+    qemu_check_nic_model(nd, TYPE_LANCE);
     sparc32_dma_init(hwdef->dma_base,
                      hwdef->esp_base, slavio_irq[18],
-                     hwdef->le_base, slavio_irq[16]);
+                     hwdef->le_base, slavio_irq[16], nd);
 
     if (graphic_depth != 8 && graphic_depth != 24) {
         error_report("Unsupported depth: %d", graphic_depth);
@@ -1047,7 +1052,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
                                     machine->initrd_filename,
                                     machine->ram_size, &initrd_size);
 
-    nvram_init(nvram, (uint8_t *)&nd_table[0].macaddr, machine->kernel_cmdline,
+    nvram_init(nvram, (uint8_t *)&nd->macaddr, machine->kernel_cmdline,
                machine->boot_order, machine->ram_size, kernel_size,
                graphic_width, graphic_height, graphic_depth,
                hwdef->nvram_machine_id, "Sun4m");
-- 
2.20.1


Re: [PATCH 4/6] sparc32-ledma: don't reference nd_table directly within the device
Posted by Markus Armbruster 5 years, 1 month ago
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?

> @@ -854,6 +857,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>      unsigned int max_cpus = machine->smp.max_cpus;
>      Object *ram_memdev = object_resolve_path_type(machine->ram_memdev_id,
>                                                    TYPE_MEMORY_BACKEND, NULL);
> +    NICInfo *nd = &nd_table[0];
>  
>      if (machine->ram_size > hwdef->max_mem) {
>          error_report("Too much memory for this machine: %" PRId64 ","
> @@ -914,9 +918,10 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>                          hwdef->iommu_pad_base, hwdef->iommu_pad_len);
>      }
>  
> +    qemu_check_nic_model(nd, TYPE_LANCE);
>      sparc32_dma_init(hwdef->dma_base,
>                       hwdef->esp_base, slavio_irq[18],
> -                     hwdef->le_base, slavio_irq[16]);
> +                     hwdef->le_base, slavio_irq[16], nd);
>  
>      if (graphic_depth != 8 && graphic_depth != 24) {
>          error_report("Unsupported depth: %d", graphic_depth);
> @@ -1047,7 +1052,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>                                      machine->initrd_filename,
>                                      machine->ram_size, &initrd_size);
>  
> -    nvram_init(nvram, (uint8_t *)&nd_table[0].macaddr, machine->kernel_cmdline,
> +    nvram_init(nvram, (uint8_t *)&nd->macaddr, machine->kernel_cmdline,
>                 machine->boot_order, machine->ram_size, kernel_size,
>                 graphic_width, graphic_height, graphic_depth,
>                 hwdef->nvram_machine_id, "Sun4m");


Re: [PATCH 4/6] sparc32-ledma: don't reference nd_table directly within the device
Posted by Mark Cave-Ayland 5 years, 1 month ago
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.

Re: [PATCH 4/6] sparc32-ledma: don't reference nd_table directly within the device
Posted by Mark Cave-Ayland 5 years, 1 month ago
On 21/09/2020 18:03, Mark Cave-Ayland wrote:

> 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.

Actually I have a better idea for this: use sysbus_mmio_get_region() within the
sparc32-dma device to attach the lance and esp memory regions to its own container
memory region: then the single sysbus_mmio_map() for the sparc32-dma device will just
work on its own.


ATB,

Mark.

Re: [PATCH 4/6] sparc32-ledma: don't reference nd_table directly within the device
Posted by Mark Cave-Ayland 5 years, 1 month ago
On 21/09/2020 18:14, Mark Cave-Ayland wrote:

> On 21/09/2020 18:03, Mark Cave-Ayland wrote:
> 
>> 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.
> 
> Actually I have a better idea for this: use sysbus_mmio_get_region() within the
> sparc32-dma device to attach the lance and esp memory regions to its own container
> memory region: then the single sysbus_mmio_map() for the sparc32-dma device will just
> work on its own.

(goes and looks)

Ah now I remember - the DMA control registers and the lance/ESP devices are mapped to
2 different memory locations. So I think the current solution is the best one here.


ATB,

Mark.

Re: [PATCH 4/6] sparc32-ledma: don't reference nd_table directly within the device
Posted by Philippe Mathieu-Daudé 5 years, 1 month ago
Hi Mark,

On 9/20/20 10:20 AM, Mark Cave-Ayland wrote:
> 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)

Instead of passing NICInfo to sparc32_dma_init,
shouldn't you extract the lance code from it?

>  {
>      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;
> @@ -854,6 +857,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>      unsigned int max_cpus = machine->smp.max_cpus;
>      Object *ram_memdev = object_resolve_path_type(machine->ram_memdev_id,
>                                                    TYPE_MEMORY_BACKEND, NULL);
> +    NICInfo *nd = &nd_table[0];
>  
>      if (machine->ram_size > hwdef->max_mem) {
>          error_report("Too much memory for this machine: %" PRId64 ","
> @@ -914,9 +918,10 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>                          hwdef->iommu_pad_base, hwdef->iommu_pad_len);
>      }
>  
> +    qemu_check_nic_model(nd, TYPE_LANCE);
>      sparc32_dma_init(hwdef->dma_base,
>                       hwdef->esp_base, slavio_irq[18],
> -                     hwdef->le_base, slavio_irq[16]);
> +                     hwdef->le_base, slavio_irq[16], nd);
>  
>      if (graphic_depth != 8 && graphic_depth != 24) {
>          error_report("Unsupported depth: %d", graphic_depth);
> @@ -1047,7 +1052,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>                                      machine->initrd_filename,
>                                      machine->ram_size, &initrd_size);
>  
> -    nvram_init(nvram, (uint8_t *)&nd_table[0].macaddr, machine->kernel_cmdline,
> +    nvram_init(nvram, (uint8_t *)&nd->macaddr, machine->kernel_cmdline,
>                 machine->boot_order, machine->ram_size, kernel_size,
>                 graphic_width, graphic_height, graphic_depth,
>                 hwdef->nvram_machine_id, "Sun4m");
> 


Re: [PATCH 4/6] sparc32-ledma: don't reference nd_table directly within the device
Posted by Mark Cave-Ayland 5 years, 1 month ago
On 21/09/2020 10:57, Philippe Mathieu-Daudé wrote:

> Hi Mark,
> 
> On 9/20/20 10:20 AM, Mark Cave-Ayland wrote:
>> 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)
> 
> Instead of passing NICInfo to sparc32_dma_init,
> shouldn't you extract the lance code from it?

Hi Philippe,

I'm not sure I understand what you mean here? The sparc32-dma device is realised
within the sparc32_dma_init() function and qdev_set_nic_properties() must be called
before realise happens.

If you can explain a bit more about how you think it can be separated out then I can
take a look.


ATB,

Mark.

Re: [PATCH 4/6] sparc32-ledma: don't reference nd_table directly within the device
Posted by Philippe Mathieu-Daudé 5 years, 1 month ago
On 9/21/20 7:08 PM, Mark Cave-Ayland wrote:
> On 21/09/2020 10:57, Philippe Mathieu-Daudé wrote:
> 
>> Hi Mark,
>>
>> On 9/20/20 10:20 AM, Mark Cave-Ayland wrote:
>>> 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)
>>
>> Instead of passing NICInfo to sparc32_dma_init,
>> shouldn't you extract the lance code from it?
> 
> Hi Philippe,
> 
> I'm not sure I understand what you mean here? The sparc32-dma device is realised
> within the sparc32_dma_init() function and qdev_set_nic_properties() must be called
> before realise happens.
> 
> If you can explain a bit more about how you think it can be separated out then I can
> take a look.

Sorry I guess I got confused by the 2 different sparc32_dma_init()
functions.

Since ledma always expose lance, maybe you can use:

diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
index 2cbe331959a..9a907a30373 100644
--- a/hw/dma/sparc32_dma.c
+++ b/hw/dma/sparc32_dma.c
@@ -336,18 +336,14 @@ static void sparc32_ledma_device_init(Object *obj)
                           "ledma-mmio", DMA_SIZE);

     object_initialize_child(obj, "lance", &ls->lance, TYPE_LANCE);
+    qdev_alias_all_properties(DEVICE(&ls->lance), obj);
 }

This way you set the properties directly on the ledma and only
have to sysbus_map lance.

> 
> 
> ATB,
> 
> Mark.
> 

Re: [PATCH 4/6] sparc32-ledma: don't reference nd_table directly within the device
Posted by Mark Cave-Ayland 5 years, 1 month ago
On 21/09/2020 18:58, Philippe Mathieu-Daudé wrote:

> Sorry I guess I got confused by the 2 different sparc32_dma_init()
> functions.
> 
> Since ledma always expose lance, maybe you can use:
> 
> diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c
> index 2cbe331959a..9a907a30373 100644
> --- a/hw/dma/sparc32_dma.c
> +++ b/hw/dma/sparc32_dma.c
> @@ -336,18 +336,14 @@ static void sparc32_ledma_device_init(Object *obj)
>                            "ledma-mmio", DMA_SIZE);
> 
>      object_initialize_child(obj, "lance", &ls->lance, TYPE_LANCE);
> +    qdev_alias_all_properties(DEVICE(&ls->lance), obj);
>  }
> 
> This way you set the properties directly on the ledma and only
> have to sysbus_map lance.

Thanks for the hint. I've had a look at qdev_alias_all_properties() and for the
moment I'd prefer to get the reference to the internal lance/esp devices via
object_resolve_path_component(), since to me it makes it clearer on which device the
properties really belong from just looking at the code.


ATB,

Mark.