On Fri, Apr 5, 2013 at 7:26 PM, Peter Maydell <peter.mayd...@linaro.org> wrote: > On 5 April 2013 09:43, Peter Crosthwaite <peter.crosthwa...@xilinx.com> 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 >