qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 4/5] xilinx_devcfg: Zynq devcfg device model


From: Peter Crosthwaite
Subject: Re: [Qemu-devel] [PATCH v3 4/5] xilinx_devcfg: Zynq devcfg device model
Date: Wed, 29 May 2013 23:54:21 +1000

Hi Paolo,

On Wed, May 29, 2013 at 6:51 PM, Paolo Bonzini <address@hidden> wrote:
> Il 24/05/2013 07:49, address@hidden ha scritto:
>> +static const MemoryRegionOps devcfg_reg_ops = {
>> +    .read = register_read_memory_le,
>> +    .write = register_write_memory_le,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .valid = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>
> What happens if you have registers of mixed size within the same "bank"?
>

You Probably should change these access restrictions per-register
accordingly. Note that min_access_size = 4 is a restriction of the
devcfg device and not the register API. We could change that to 1 no
problems. max_access should probably be no greater than the size of
the register, otherwise you have defined a non-nonsensical memory
region for the register. But note that defining it as less than the
register size is perfectly valid (for the weird case where you allow
e.g. only byte accesses to a register described as 32 bits).

The degenerate case is allowing greater-than-register size or
misaligned accesses, which is dependent on the behaviour of the memory
API. I can do some research, but do you know if the memory API
supports misaligned transactions that span a memory region boundary?
If so then there are no concerns, as the memory API will handle it for
us. If not then I don't see it as an issue as its very rare (at least
in my experience) for registers to support such strange misaligned or
mulit-register accesses. That or we can look into memory API for
answers.

>> +    }
>> +};
>> +
>> +static void xilinx_devcfg_realize(DeviceState *dev, Error **errp)
>> +{
>> +    XilinxDevcfg *s = XILINX_DEVCFG(dev);
>> +    const char *prefix = object_get_canonical_path(OBJECT(dev));
>> +    int i;
>> +
>> +    for (i = 0; i < R_MAX; ++i) {
>> +        RegisterInfo *r = &s->regs_info[i];
>> +
>> +        *r = (RegisterInfo) {
>> +            .data = &s->regs[i],
>> +            .data_size = sizeof(uint32_t),
>> +            .access = &xilinx_devcfg_regs_info[i],
>> +            .debug = XILINX_DEVCFG_ERR_DEBUG,
>> +            .prefix = prefix,
>> +            .opaque = s,
>> +        };
>> +        memory_region_init_io(&r->mem, &devcfg_reg_ops, r, "devcfg-regs", 
>> 4);
>
> Could you add a register_init function that does register_reset +
> memory_region_init_io?
>

I wanted to factor this loop out into a helper as a next step so I
think its already in my todo list, but i'm confused as to why reset
and memory_region_init would bundled together - arent they normally in
Device::reset and Device::realize (or Object::Init) respectively?

Regards,
Peter

>> +        memory_region_add_subregion(&s->iomem, i * 4, &r->mem);
>
> Paolo
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]