[Qemu-devel] [PATCH] 40p: don't use legacy fw_cfg_init_mem() function

[Qemu-devel] [PATCH] 40p: don't use legacy fw_cfg_init_mem() function

[Mark Cave-Ayland]

Instead initialise the device via qdev to allow us to set device properties
directly as required.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ppc/prep.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 3401570d98..9cf4a2adc3 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -706,7 +706,7 @@ static void ibm_40p_init(MachineState *machine)
     uint16_t cmos_checksum;
     PowerPCCPU *cpu;
     DeviceState *dev;
-    SysBusDevice *pcihost;
+    SysBusDevice *pcihost, *s;
     Nvram *m48t59 = NULL;
     PCIBus *pci_bus;
     ISABus *isa_bus;
@@ -799,7 +799,16 @@ static void ibm_40p_init(MachineState *machine)
     }
 
     /* Prepare firmware configuration for OpenBIOS */
-    fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
+    dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
+    fw_cfg = FW_CFG(dev);
+    qdev_prop_set_uint32(dev, "data_width", 1);
+    qdev_prop_set_bit(dev, "dma_enabled", false);
+    object_property_add_child(OBJECT(qdev_get_machine()), TYPE_FW_CFG,
+                              OBJECT(fw_cfg), NULL);
+    qdev_init_nofail(dev);
+    s = SYS_BUS_DEVICE(dev);
+    sysbus_mmio_map(s, 0, CFG_ADDR);
+    sysbus_mmio_map(s, 1, CFG_ADDR + 2);
 
     if (machine->kernel_filename) {
         /* load kernel */
-- 
2.11.0

Re: [Qemu-devel] [PATCH] 40p: don't use legacy fw_cfg_init_mem() function

[Hervé Poussineau]

Le 10/08/2018 à 14:04, Mark Cave-Ayland a écrit :
> Instead initialise the device via qdev to allow us to set device properties
> directly as required.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/ppc/prep.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 3401570d98..9cf4a2adc3 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -706,7 +706,7 @@ static void ibm_40p_init(MachineState *machine)
>       uint16_t cmos_checksum;
>       PowerPCCPU *cpu;
>       DeviceState *dev;
> -    SysBusDevice *pcihost;
> +    SysBusDevice *pcihost, *s;
>       Nvram *m48t59 = NULL;
>       PCIBus *pci_bus;
>       ISABus *isa_bus;
> @@ -799,7 +799,16 @@ static void ibm_40p_init(MachineState *machine)
>       }
>   
>       /* Prepare firmware configuration for OpenBIOS */
> -    fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
> +    dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
> +    fw_cfg = FW_CFG(dev);
> +    qdev_prop_set_uint32(dev, "data_width", 1);
> +    qdev_prop_set_bit(dev, "dma_enabled", false);
> +    object_property_add_child(OBJECT(qdev_get_machine()), TYPE_FW_CFG,
> +                              OBJECT(fw_cfg), NULL);
> +    qdev_init_nofail(dev);
> +    s = SYS_BUS_DEVICE(dev);
> +    sysbus_mmio_map(s, 0, CFG_ADDR);
> +    sysbus_mmio_map(s, 1, CFG_ADDR + 2);
>   
>       if (machine->kernel_filename) {
>           /* load kernel */
> 

So, you're inlining fw_cfg_init_mem() and resolving parameters and conditions.
Any reason to do this, as this function is also used in other places?

However,
Acked-by: Hervé Poussineau <hpoussin@reactos.org>

Hervé

Re: [Qemu-devel] [PATCH] 40p: don't use legacy fw_cfg_init_mem() function

[Mark Cave-Ayland]

On 10/08/18 21:18, Hervé Poussineau wrote:

> Le 10/08/2018 à 14:04, Mark Cave-Ayland a écrit :
>> Instead initialise the device via qdev to allow us to set device
>> properties
>> directly as required.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/ppc/prep.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
>> index 3401570d98..9cf4a2adc3 100644
>> --- a/hw/ppc/prep.c
>> +++ b/hw/ppc/prep.c
>> @@ -706,7 +706,7 @@ static void ibm_40p_init(MachineState *machine)
>>       uint16_t cmos_checksum;
>>       PowerPCCPU *cpu;
>>       DeviceState *dev;
>> -    SysBusDevice *pcihost;
>> +    SysBusDevice *pcihost, *s;
>>       Nvram *m48t59 = NULL;
>>       PCIBus *pci_bus;
>>       ISABus *isa_bus;
>> @@ -799,7 +799,16 @@ static void ibm_40p_init(MachineState *machine)
>>       }
>>         /* Prepare firmware configuration for OpenBIOS */
>> -    fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
>> +    dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
>> +    fw_cfg = FW_CFG(dev);
>> +    qdev_prop_set_uint32(dev, "data_width", 1);
>> +    qdev_prop_set_bit(dev, "dma_enabled", false);
>> +    object_property_add_child(OBJECT(qdev_get_machine()), TYPE_FW_CFG,
>> +                              OBJECT(fw_cfg), NULL);
>> +    qdev_init_nofail(dev);
>> +    s = SYS_BUS_DEVICE(dev);
>> +    sysbus_mmio_map(s, 0, CFG_ADDR);
>> +    sysbus_mmio_map(s, 1, CFG_ADDR + 2);
>>         if (machine->kernel_filename) {
>>           /* load kernel */
>>
> 
> So, you're inlining fw_cfg_init_mem() and resolving parameters and
> conditions.
> Any reason to do this, as this function is also used in other places?

Well, not any more as I've sent patches to remove just about all of the
others.

The problem with using *_init() functions is that you end up needing one
for every combination of parameters that you need, or having to map
function parameters to qdev parameters. And if you're doing that, you
might as well use the qdev parameters directly.

> However,
> Acked-by: Hervé Poussineau <hpoussin@reactos.org>


ATB,

Mark.

Re: [Qemu-devel] [PATCH] 40p: don't use legacy fw_cfg_init_mem() function

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]

On Fri, Aug 10, 2018 at 01:04:18PM +0100, Mark Cave-Ayland wrote:
> Instead initialise the device via qdev to allow us to set device properties
> directly as required.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Applied to ppc-for-3.1, thanks.

> ---
>  hw/ppc/prep.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 3401570d98..9cf4a2adc3 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -706,7 +706,7 @@ static void ibm_40p_init(MachineState *machine)
>      uint16_t cmos_checksum;
>      PowerPCCPU *cpu;
>      DeviceState *dev;
> -    SysBusDevice *pcihost;
> +    SysBusDevice *pcihost, *s;
>      Nvram *m48t59 = NULL;
>      PCIBus *pci_bus;
>      ISABus *isa_bus;
> @@ -799,7 +799,16 @@ static void ibm_40p_init(MachineState *machine)
>      }
>  
>      /* Prepare firmware configuration for OpenBIOS */
> -    fw_cfg = fw_cfg_init_mem(CFG_ADDR, CFG_ADDR + 2);
> +    dev = qdev_create(NULL, TYPE_FW_CFG_MEM);
> +    fw_cfg = FW_CFG(dev);
> +    qdev_prop_set_uint32(dev, "data_width", 1);
> +    qdev_prop_set_bit(dev, "dma_enabled", false);
> +    object_property_add_child(OBJECT(qdev_get_machine()), TYPE_FW_CFG,
> +                              OBJECT(fw_cfg), NULL);
> +    qdev_init_nofail(dev);
> +    s = SYS_BUS_DEVICE(dev);
> +    sysbus_mmio_map(s, 0, CFG_ADDR);
> +    sysbus_mmio_map(s, 1, CFG_ADDR + 2);
>  
>      if (machine->kernel_filename) {
>          /* load kernel */

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]