Re: [Qemu-devel] [PATCH 17/19] uninorth: create new uninorth device

[Philippe Mathieu-Daudé]

Hi Mark,

On 03/21/2018 12:29 AM, David Gibson wrote:
> On Tue, Mar 06, 2018 at 08:31:01PM +0000, Mark Cave-Ayland wrote:
>> Commit 4e46dcdbd3 "PPC: Newworld: Add uninorth token register" added a TODO
>> which was to convert the uninorth registers hack to a proper device. Move
>> these registers to a new uninorth device, removing the old hacks from
>> mac_newworld.c.
>>
>> Signed-off-by: Mark Cave-Ayland <address@hidden>
>> ---
>>  hw/pci-host/trace-events       |  2 ++
>>  hw/pci-host/uninorth.c         | 58 
>> ++++++++++++++++++++++++++++++++++++++++++
>>  hw/ppc/mac_newworld.c          | 41 +++++------------------------
>>  hw/ppc/trace-events            |  4 ---
>>  include/hw/pci-host/uninorth.h | 11 ++++++++
>>  5 files changed, 77 insertions(+), 39 deletions(-)
>>
>> diff --git a/hw/pci-host/trace-events b/hw/pci-host/trace-events
>> index 341a87a702..dd7a398e96 100644
>> --- a/hw/pci-host/trace-events
>> +++ b/hw/pci-host/trace-events
>> @@ -18,3 +18,5 @@ unin_set_irq(int irq_num, int level) "setting INT %d = %d"
>>  unin_get_config_reg(uint32_t reg, uint32_t addr, uint32_t retval) 
>> "converted config space accessor 0x%"PRIx32 "/0x%"PRIx32 " -> 0x%"PRIx32
>>  unin_data_write(uint64_t addr, unsigned len, uint64_t val) "write addr 
>> 0x%"PRIx64 " len %d val 0x%"PRIx64
>>  unin_data_read(uint64_t addr, unsigned len, uint64_t val) "read addr 
>> 0x%"PRIx64 " len %d val 0x%"PRIx64
>> +unin_write(uint64_t addr, uint64_t value) "addr=0x%" PRIx64 " val=0x%"PRIx64
>> +unin_read(uint64_t addr, uint64_t value) "addr=0x%" PRIx64 " val=0x%"PRIx64
>> diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
>> index fada0ffd5f..dbfad01d9d 100644
>> --- a/hw/pci-host/uninorth.c
>> +++ b/hw/pci-host/uninorth.c
>> @@ -519,6 +519,62 @@ static const TypeInfo pci_unin_internal_info = {
>>      .class_init    = pci_unin_internal_class_init,
>>  };
>>  
>> +/* UniN device */
>> +static void unin_write(void *opaque, hwaddr addr, uint64_t value,
>> +                       unsigned size)
>> +{
>> +    trace_unin_write(addr, value);
>> +    if (addr == 0x0) {
>> +        *(int *)opaque = value;

This looks a dirty insecure optimization...

I see this is code motion, can you clean up using UNINState:

        UNINState *s = UNI_NORTH(obj);

        ...
            s->token = value;

>> +    }
>> +}
>> +
>> +static uint64_t unin_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    uint32_t value;
>> +
>> +    value = 0;
>> +    switch (addr) {
>> +    case 0:
>> +        value = *(int *)opaque;

Same,

>> +    }
>> +
>> +    trace_unin_read(addr, value);
>> +
>> +    return value;
>> +}
>> +
>> +static const MemoryRegionOps unin_ops = {
>> +    .read = unin_read,
>> +    .write = unin_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
> 
> DEVICE_NATIVE_ENDIAN is almost always wrong.  I'm pretty sure it
> should be DEVICE_BIG_ENDIAN.  I realize this is just a code motion,
> but while you're making a proper device of it, you might as well fix
> this to.
> 
>> +};
>> +
>> +static void unin_init(Object *obj)
>> +{
>> +    UNINState *s = UNI_NORTH(obj);
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> +
>> +    memory_region_init_io(&s->mem, obj, &unin_ops, &s->token, "unin", 
>> 0x1000);

And here:

    memory_region_init_io(&s->mem, obj, &unin_ops, s, "unin", 0x1000);

>> +
>> +    sysbus_init_mmio(sbd, &s->mem);
>> +}
>> +
>> +static void unin_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>> +}
>> +
>> +static const TypeInfo unin_info = {
>> +    .name          = TYPE_UNI_NORTH,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(UNINState),
>> +    .instance_init = unin_init,
>> +    .class_init    = unin_class_init,
>> +};
>> +
>>  static void unin_register_types(void)
>>  {
>>      type_register_static(&unin_main_pci_host_info);
>> @@ -530,6 +586,8 @@ static void unin_register_types(void)
>>      type_register_static(&pci_u3_agp_info);
>>      type_register_static(&pci_unin_agp_info);
>>      type_register_static(&pci_unin_internal_info);
>> +
>> +    type_register_static(&unin_info);
>>  }
>>  
>>  type_init(unin_register_types)
>> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
>> index ae0de4e36e..2fcb101982 100644
>> --- a/hw/ppc/mac_newworld.c
>> +++ b/hw/ppc/mac_newworld.c
>> @@ -83,36 +83,6 @@
>>  
>>  #define NDRV_VGA_FILENAME "qemu_vga.ndrv"
>>  
>> -/* UniN device */
>> -static void unin_write(void *opaque, hwaddr addr, uint64_t value,
>> -                       unsigned size)
>> -{
>> -    trace_mac99_uninorth_write(addr, value);
>> -    if (addr == 0x0) {
>> -        *(int*)opaque = value;
>> -    }
>> -}
>> -
>> -static uint64_t unin_read(void *opaque, hwaddr addr, unsigned size)
>> -{
>> -    uint32_t value;
>> -
>> -    value = 0;
>> -    switch (addr) {
>> -    case 0:
>> -        value = *(int*)opaque;
>> -    }
>> -
>> -    trace_mac99_uninorth_read(addr, value);
>> -
>> -    return value;
>> -}
>> -
>> -static const MemoryRegionOps unin_ops = {
>> -    .read = unin_read,
>> -    .write = unin_write,
>> -    .endianness = DEVICE_NATIVE_ENDIAN,
>> -};
>>  
>>  static void fw_cfg_boot_set(void *opaque, const char *boot_device,
>>                              Error **errp)
>> @@ -146,7 +116,6 @@ static void ppc_core99_init(MachineState *machine)
>>      CPUPPCState *env = NULL;
>>      char *filename;
>>      qemu_irq *pic, **openpic_irqs;
>> -    MemoryRegion *unin_memory = g_new(MemoryRegion, 1);
>>      int linux_boot, i, j, k;
>>      MemoryRegion *ram = g_new(MemoryRegion, 1), *bios = g_new(MemoryRegion, 
>> 1);
>>      hwaddr kernel_base, initrd_base, cmdline_base = 0;
>> @@ -165,7 +134,6 @@ static void ppc_core99_init(MachineState *machine)
>>      int machine_arch;
>>      SysBusDevice *s;
>>      DeviceState *dev, *pic_dev;
>> -    int *token = g_new(int, 1);
>>      hwaddr nvram_addr = 0xFFF04000;
>>      uint64_t tbfreq;
>>  
>> @@ -273,9 +241,12 @@ static void ppc_core99_init(MachineState *machine)
>>          }
>>      }
>>  
>> -    /* UniN init: XXX should be a real device */
>> -    memory_region_init_io(unin_memory, NULL, &unin_ops, token, "unin", 
>> 0x1000);
>> -    memory_region_add_subregion(get_system_memory(), 0xf8000000, 
>> unin_memory);
>> +    /* UniN init */
>> +    dev = qdev_create(NULL, TYPE_UNI_NORTH);
>> +    qdev_init_nofail(dev);
>> +    s = SYS_BUS_DEVICE(dev);
>> +    memory_region_add_subregion(get_system_memory(), 0xf8000000,
>> +                                sysbus_mmio_get_region(s, 0));
>>  
>>      openpic_irqs = g_malloc0(smp_cpus * sizeof(qemu_irq *));
>>      openpic_irqs[0] =
>> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
>> index 66ec7eda6e..dc5e65aee9 100644
>> --- a/hw/ppc/trace-events
>> +++ b/hw/ppc/trace-events
>> @@ -92,10 +92,6 @@ rs6000mc_size_read(uint32_t addr, uint32_t val) "read 
>> addr=0x%x val=0x%x"
>>  rs6000mc_size_write(uint32_t addr, uint32_t val) "write addr=0x%x val=0x%x"
>>  rs6000mc_parity_read(uint32_t addr, uint32_t val) "read addr=0x%x val=0x%x"
>>  
>> -# hw/ppc/mac_newworld.c
>> -mac99_uninorth_write(uint64_t addr, uint64_t value) "addr=0x%" PRIx64 " 
>> val=0x%"PRIx64
>> -mac99_uninorth_read(uint64_t addr, uint64_t value) "addr=0x%" PRIx64 " 
>> val=0x%"PRIx64
>> -
>>  # hw/ppc/ppc4xx_pci.c
>>  ppc4xx_pci_map_irq(int32_t devfn, int irq_num, int slot) "devfn 0x%x irq %d 
>> -> %d"
>>  ppc4xx_pci_set_irq(int irq_num) "PCI irq %d"
>> diff --git a/include/hw/pci-host/uninorth.h b/include/hw/pci-host/uninorth.h
>> index d1424b165a..b0eb093e72 100644
>> --- a/include/hw/pci-host/uninorth.h
>> +++ b/include/hw/pci-host/uninorth.h
>> @@ -51,4 +51,15 @@ typedef struct UNINHostState {
>>      MemoryRegion pci_io;
>>  } UNINHostState;
>>  
>> +typedef struct UNINState {
>> +    SysBusDevice parent_obj;
>> +
>> +    MemoryRegion mem;
>> +    int token[1];

having 'token' being 'int' type also looks weird.

Maybe target_ulong is better?

>> +} UNINState;
>> +
>> +#define TYPE_UNI_NORTH "uni-north"
>> +#define UNI_NORTH(obj) \
>> +    OBJECT_CHECK(UNINState, (obj), TYPE_UNI_NORTH)
>> +
>>  #endif /* UNINORTH_H */
> 

Regards,

Phil.

signature.asc
Description: OpenPGP digital signature