Re: [Qemu-devel] [PATCH v3 02/16] register: Add Register API

[Peter Crosthwaite <crosthwaitepeter@gmail.com>]

On Tue, Feb 9, 2016 at 11:35 AM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Tue, Feb 9, 2016 at 8:06 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Alistair Francis <alistair.francis@xilinx.com> writes:
>>
>>> 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.
>>>
>>> Helper functions are then used to access the register which observe the
>>> semantics defined by the RegisterAccessInfo struct.
>>>
>>> Some features:
>>> Bits can be marked as read_only (ro field)
>>> Bits can be marked as write-1-clear (w1c field)
>>> Bits can be marked as reserved (rsvd field)
>>> Reset values can be defined (reset)
>>> Bits can throw guest errors when written certain values (ge0, ge1)
>>> Bits can throw unimp errors when written certain values (ui0, ui1)
>>> Bits can be marked clear on read (cor)
>>> Pre and post action callbacks can be added to read and write ops
>>> Verbose debugging info can be enabled/disabled
>>>
>>> Useful for defining device register spaces in a data driven way. Cuts
>>> down on a lot of the verbosity and repetition in the switch-case blocks
>>> in the standard foo_mmio_read/write functions.
>>>
>>> Also useful for automated generation of device models from hardware
>>> design sources.
>>>
>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> ---
>>> V3:
>>>  - Address some comments from Fred
>>>
>>>  hw/core/Makefile.objs |   1 +
>>>  hw/core/register.c    | 189 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/hw/register.h | 132 +++++++++++++++++++++++++++++++++++
>>>  3 files changed, 322 insertions(+)
>>>  create mode 100644 hw/core/register.c
>>>  create mode 100644 include/hw/register.h
>>>
>>> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
>>> index abb3560..bf95db5 100644
>>> --- a/hw/core/Makefile.objs
>>> +++ b/hw/core/Makefile.objs
>>> @@ -14,4 +14,5 @@ common-obj-$(CONFIG_SOFTMMU) += machine.o
>>>  common-obj-$(CONFIG_SOFTMMU) += null-machine.o
>>>  common-obj-$(CONFIG_SOFTMMU) += loader.o
>>>  common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
>>> +common-obj-$(CONFIG_SOFTMMU) += register.o
>>>  common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o
>>> diff --git a/hw/core/register.c b/hw/core/register.c
>>> new file mode 100644
>>> index 0000000..f0fc39c
>>> --- /dev/null
>>> +++ b/hw/core/register.c
>>> @@ -0,0 +1,189 @@
>>> +/*
>>> + * Register Definition API
>>> + *
>>> + * Copyright (c) 2013 Xilinx Inc.
>>> + * Copyright (c) 2013 Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>>> + * the COPYING file in the top-level directory.
>>> + */
>>> +
>>> +#include "hw/register.h"
>>> +#include "qemu/log.h"
>>> +
>>> +static inline void register_write_log(RegisterInfo *reg, int dir, uint64_t val,
>>> +                                      int mask, const char *msg,
>>> +                                      const char *reason)
>>> +{
>>> +    qemu_log_mask(mask, "%s:%s bits %#" PRIx64 " %s write of %d%s%s\n",
>>> +                  reg->prefix, reg->access->name, val, msg, dir,
>>> +                  reason ? ": " : "", reason ? reason : "");
>>
>> I'm not sure mask needs to be passed down as every use of it seems to
>> imply LOG_GUEST_USER. If you think this might change I would consider
>> passing the mask as the first option to align with other logging functions.
>
> Removing this function as it is no longer required.
>
>>
>>> +}
>>> +
>>> +static inline void register_write_val(RegisterInfo *reg, uint64_t val)
>>> +{
>>> +    if (!reg->data) {
>>> +        return;
>>> +    }
>>
>> Can you have a defined register without a data field? If it is invalid I
>> would use a g_assert() instead.
>
> Good point, I can't see any cases where there wouldn't be a data field.
>

>From the register API point of view it should be valid. It just so
happens when use the array based registration it never comes up. So I
think assert is not quite right even though the path is dead as of
this series.

Regards,
Peter