Re: [PATCH v6 02/11] hw/intc/xilinx_intc: Make device endianness configurable

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 12/2/25 12:42, Thomas Huth wrote:
> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote:
>> Replace the DEVICE_NATIVE_ENDIAN MemoryRegionOps by a pair
>> of DEVICE_LITTLE_ENDIAN / DEVICE_BIG_ENDIAN.
>> Add the "little-endian" property to select the device
>> endianness, defaulting to little endian.
>> Set the proper endianness for each machine using the device.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/intc/xilinx_intc.c                    | 60 ++++++++++++++++++------
>>   hw/microblaze/petalogix_ml605_mmu.c      |  1 +
>>   hw/microblaze/petalogix_s3adsp1800_mmu.c |  3 ++
>>   hw/ppc/virtex_ml507.c                    |  1 +
>>   hw/riscv/microblaze-v-generic.c          |  1 +
>>   5 files changed, 52 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c
>> index 6930f83907a..523402b688c 100644
>> --- a/hw/intc/xilinx_intc.c
>> +++ b/hw/intc/xilinx_intc.c
>> @@ -3,6 +3,9 @@
>>    *
>>    * Copyright (c) 2009 Edgar E. Iglesias.
>>    *
>> + * https://docs.amd.com/v/u/en-US/xps_intc
>> + * DS572: LogiCORE IP XPS Interrupt Controller (v2.01a)
>> + *
>>    * Permission is hereby granted, free of charge, to any person 
>> obtaining a copy
>>    * of this software and associated documentation files (the 
>> "Software"), to deal
>>    * in the Software without restriction, including without limitation 
>> the rights
>> @@ -23,10 +26,12 @@
>>    */
>>   #include "qemu/osdep.h"
>> +#include "qapi/error.h"
>>   #include "hw/sysbus.h"
>>   #include "qemu/module.h"
>>   #include "hw/irq.h"
>>   #include "hw/qdev-properties.h"
>> +#include "hw/qdev-properties-system.h"
>>   #include "qom/object.h"
>>   #define D(x)
>> @@ -49,6 +54,7 @@ struct XpsIntc
>>   {
>>       SysBusDevice parent_obj;
>> +    EndianMode model_endianness;
>>       MemoryRegion mmio;
>>       qemu_irq parent_irq;
>> @@ -140,18 +146,29 @@ static void pic_write(void *opaque, hwaddr addr,
>>       update_irq(p);
>>   }
>> -static const MemoryRegionOps pic_ops = {
>> -    .read = pic_read,
>> -    .write = pic_write,
>> -    .endianness = DEVICE_NATIVE_ENDIAN,
>> -    .impl = {
>> -        .min_access_size = 4,
>> -        .max_access_size = 4,
>> +static const MemoryRegionOps pic_ops[2] = {
>> +    [0 ... 1] = {
> 
> Hmm, ok, so here we have now an assumption that ENDIAN_MODE_BIG and 
> ENDIAN_MODE_LITTLE match 0 and 1, which would not work anymore when 
> using 0 as unspecified... a little bit ugly ... so maybe instead of 
> changing pic_ops into an array ....
> 
>> +        .read = pic_read,
>> +        .write = pic_write,
>> +        .endianness = DEVICE_BIG_ENDIAN,
>> +        .impl = {
>> +            .min_access_size = 4,
>> +            .max_access_size = 4,
>> +        },
>> +        .valid = {
>> +            /*
>> +             * All XPS INTC registers are accessed through the PLB 
>> interface.
>> +             * The base address for these registers is provided by the
>> +             * configuration parameter, C_BASEADDR. Each register is 
>> 32 bits
>> +             * although some bits may be unused and is accessed on a 
>> 4-byte
>> +             * boundary offset from the base address.
>> +             */
>> +            .min_access_size = 4,
>> +            .max_access_size = 4,
>> +        },
>>       },
>> -    .valid = {
>> -        .min_access_size = 4,
>> -        .max_access_size = 4
>> -    }
>> +    [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN,
>> +    [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN,
>>   };
>>   static void irq_handler(void *opaque, int irq, int level)
>> @@ -174,13 +191,27 @@ static void xilinx_intc_init(Object *obj)
>>       qdev_init_gpio_in(DEVICE(obj), irq_handler, 32);
>>       sysbus_init_irq(SYS_BUS_DEVICE(obj), &p->parent_irq);
>> -
>> -    memory_region_init_io(&p->mmio, obj, &pic_ops, p, "xlnx.xps-intc",
>> -                          R_MAX * 4);
>>       sysbus_init_mmio(SYS_BUS_DEVICE(obj), &p->mmio);
>>   }
>> +static void xilinx_intc_realize(DeviceState *dev, Error **errp)
>> +{
>> +    XpsIntc *p = XILINX_INTC(dev);
>> +
>> +    if (p->model_endianness == ENDIAN_MODE_UNSPECIFIED) {
>> +        error_setg(errp, TYPE_XILINX_INTC " property 'endianness'"
>> +                         " must be set to 'big' or 'little'");
>> +        return;
>> +    }
> 
> ... would it be possible to patch in the right value for pic_ops.endianness
> here instead?

Ah, clever than my reply on the previous patch. I'll give it a try, thanks!

> 
>   Thomas