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

[Peter Crosthwaite]

Hi Peter,

On Fri, Apr 5, 2013 at 7:49 PM, Peter Crosthwaite
<address@hidden> wrote:
> 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"...
>>

I have been thinking about this more, and in the interest of getting
something merged, I'm happy to work on this. I could use some specific
feedback however, considering there was generally positive feedback on
v1 so im guessing the objections come from the v2 added features
(being the byte loop, ui, we and the hooks). Here are my current
"lightening" proposals:

- Add a "ne" (no/native endianess) version to the memory api handlers
which just passes the args straight through to register API, no byte
reversal logic needed.
- Remove nw0 and nw1, They are reasonably rare and can be handled with
the odd pre_write hook for those cases.
- Add a "lite" flag that is (automatically) computed once that takes a
fast path through the read/write handlers. A reg is lite if it
requires no read-modify-write (no ro, nw, ge, ui, wtc). Only the
actual write and post_write handlers are executed.
- Remove the byte loops. Just switch on register size and cast to
uint_XXt. Remove support for non power-of-two size registers and
non-host endianess (needed for potential PCI conversion) but should
make the code faster.
- Make data pointer optional. No data means register is read as reset
and no write occurs (post write hook only). This make wo redundant
(anyone wanting to do per-bit wo semantic uses pre-write hook).

If you care about performance of a particular reg, just add a
post_write hook, set it up as native endianess, with a NULL data
register, no access checks. Then its a very fast path from the memory
API through to your handler.

Regards,
Peter

>
> 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
>>