qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v2 2/5] register: Add Register API


From: Peter Crosthwaite
Subject: Re: [Qemu-devel] [PATCH v2 2/5] register: Add Register API
Date: Fri, 5 Apr 2013 19:49:53 +1000

On Fri, Apr 5, 2013 at 7:26 PM, Peter Maydell <address@hidden> wrote:
> On 5 April 2013 09:43, Peter Crosthwaite <address@hidden> wrote:
>> This API provides some encapsulation of registers and factors our some
>> common functionality to common code. Bits of device state (usually MMIO
>> registers), often have all sorts of access restrictions and semantics
>> associated with them. This API allow you to define what those
>> restrictions are on a bit-by-bit basis.
>> +void register_write(RegisterInfo *reg, uint64_t val, uint64_t we)
>> +{
>> +    uint64_t old_val, new_val, test;
>> +    const RegisterAccessInfo *ac;
>> +    const RegisterAccessError *rae;
>> +
>> +    assert(reg);
>> +
>> +    ac = reg->access;
>> +    if (!ac || !ac->name) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state 
>> "
>> +                      "(written value: %#" PRIx64 ")\n", reg->prefix, val);
>> +        return;
>> +    }
>> +
>> +    uint32_t no_w0_mask = ac->ro | ac->w1c | ac->nw0 | ~we;
>> +    uint32_t no_w1_mask = ac->ro | ac->w1c | ac->nw1 | ~we;
>> +
>> +    if (reg->debug) {
>> +        fprintf(stderr, "%s:%s: write of value %#" PRIx64 "\n", reg->prefix,
>> +                ac->name, val);
>> +    }
>> +
>> +    if (qemu_loglevel_mask(LOG_GUEST_ERROR)) {
>> +        for (rae = ac->ge1; rae && rae->mask; rae++) {
>> +            test = val & rae->mask;
>> +            if (test) {
>> +                register_write_log(reg, 1, test, LOG_GUEST_ERROR,
>> +                                   "invalid", rae->reason);
>> +            }
>> +        }
>> +        for (rae = ac->ge0; rae && rae->mask; rae++) {
>> +            test = val & rae->mask;
>> +            if (test) {
>> +                register_write_log(reg, 0, test, LOG_GUEST_ERROR,
>> +                                   "invalid", rae->reason);
>> +            }
>> +        }
>> +    }
>> +
>> +    if (qemu_loglevel_mask(LOG_UNIMP)) {
>> +        for (rae = ac->ui1; rae && rae->mask; rae++) {
>> +            test = val & rae->mask;
>> +            if (test) {
>> +                register_write_log(reg, 1, test, LOG_GUEST_ERROR,
>> +                                   "unimplmented", rae->reason);
>> +            }
>> +        }
>> +        for (rae = ac->ui0; rae && rae->mask; rae++) {
>> +            test = val & rae->mask;
>> +            if (test) {
>> +                register_write_log(reg, 0, test, LOG_GUEST_ERROR,
>> +                                   "unimplemented", rae->reason);
>> +            }
>> +        }
>> +    }
>> +
>> +    assert(reg->data);
>> +    old_val = register_read_val(reg);
>> +
>> +    new_val = val & ~(no_w1_mask & val);
>> +    new_val |= no_w1_mask & old_val & val;
>> +    new_val |= no_w0_mask & old_val & ~val;
>> +    new_val &= ~(val & ac->w1c);
>> +
>> +    if (ac->pre_write) {
>> +        new_val = ac->pre_write(reg, new_val);
>> +    }
>> +    register_write_val(reg, new_val);
>> +    if (ac->post_write) {
>> +        ac->post_write(reg, new_val);
>> +    }
>> +}
>
> Wow, this feels pretty heavyweight for "write a register"...
>

Its a pritty fast path through if all the optionals turned off. If I
was going to lighten it up, Id get rid of the reg_size and the byte
loop first although that would mandate uint64_t as the data type for
the backing store.

But the intended primary usage of the API is for device control and
status registers, where you are generally not interested in
performance, unless you are doing some form of PIO. In that case, you
can opt out on a per register basis and make yourself a fast path for
the critical registers (r/w fifo heads access regs etc). This is
designed for developer and debugging convenience, where you have a
large number or registers with a heteorgenous mix of accesses and side
effects but at the same time the registers are infrequent access. This
is true of most device registers.

Another solution is some sort of "lite" mode. A single check in the
definition that disables a lot of this and cuts to the chase (the
actual write).

> -- PMM
>



reply via email to

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