Message ID | 0a14ebd735f88ae57a0d976cf5c1517a1ec0dadc.1466626975.git.alistair.francis@xilinx.com |
---|---|
State | New |
Headers | show |
On 22 June 2016 at 21:23, Alistair Francis <alistair.francis@xilinx.com> wrote: > Add memory io handlers that glue the register API to the memory API. > Just translation functions at this stage. Although it does allow for > devices to be created without all-in-one mmio r/w handlers. > > This patch also adds the RegisterInfoArray struct, which allows all of > the individual RegisterInfo structs to be grouped into a single memory > region. > > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > --- > V7: > - Remove endianess handling > - Remove assert() and log missing registers > V6: > - Add the memory region later > V5: > - Convert to using only one memory region > > hw/core/register.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++ > include/hw/register.h | 43 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 101 insertions(+) > > diff --git a/hw/core/register.c b/hw/core/register.c > index cc067f1..149aebb 100644 > --- a/hw/core/register.c > +++ b/hw/core/register.c > @@ -154,3 +154,61 @@ void register_reset(RegisterInfo *reg) > > register_write_val(reg, reg->access->reset); > } > + > +void register_write_memory(void *opaque, hwaddr addr, > + uint64_t value, unsigned size) > +{ > + RegisterInfoArray *reg_array = opaque; > + RegisterInfo *reg = NULL; > + uint64_t we = ~0; Really ~0 and not ~0ULL ? In any case this shouldn't need initializing as we will always set it later. > + int i; > + > + for (i = 0; i < reg_array->num_elements; i++) { > + if (reg_array->r[i]->access->addr == addr) { > + reg = reg_array->r[i]; > + break; > + } > + } > + > + if (!reg) { > + qemu_log_mask(LOG_UNIMP, "Write to unimplemented register at " \ > + "address: %#" PRIx64 "\n", addr); Shouldn't this be a LOG_GUEST_ERROR ? > + return; > + } > + > + /* Generate appropriate write enable mask */ > + if (reg->data_size < size) { > + we = MAKE_64BIT_MASK(0, reg->data_size * 8); > + } else if (reg->data_size >= size) { Why not just "else {" ? > + we = MAKE_64BIT_MASK(0, size * 8); > + } > + > + register_write(reg, value, we, reg_array->prefix, > + reg_array->debug); > +} > + > +uint64_t register_read_memory(void *opaque, hwaddr addr, > + unsigned size) > +{ > + RegisterInfoArray *reg_array = opaque; > + RegisterInfo *reg = NULL; > + uint64_t read_val; > + int i; > + > + for (i = 0; i < reg_array->num_elements; i++) { > + if (reg_array->r[i]->access->addr == addr) { > + reg = reg_array->r[i]; > + break; > + } > + } > + > + if (!reg) { > + qemu_log_mask(LOG_UNIMP, "Read to unimplemented register at " \ > + "address: %#" PRIx64 "\n", addr); > + return 0; > + } > + > + read_val = register_read(reg, reg_array->prefix, reg_array->debug); This will silently affect bits of the register outside the specified size with clear-on-read or other "reads have side effects" behaviour, which isn't consistent with how we handle writes. > /** > + * This structure is used to group all of the individual registers which are > + * modeled using the RegisterInfo strucutre. "structure" > + * > + * @r is an aray containing of all the relevent RegisterInfo structures. > + * > + * @num_elements is the number of elements in the array r > + * > + * @mem: optional Memory region for the register > + */ > + > +struct RegisterInfoArray { > + int num_elements; > + RegisterInfo **r; > + > + bool debug; > + const char *prefix; > +}; > + thanks -- PMM
On Thu, Jun 23, 2016 at 5:21 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 22 June 2016 at 21:23, Alistair Francis <alistair.francis@xilinx.com> wrote: >> Add memory io handlers that glue the register API to the memory API. >> Just translation functions at this stage. Although it does allow for >> devices to be created without all-in-one mmio r/w handlers. >> >> This patch also adds the RegisterInfoArray struct, which allows all of >> the individual RegisterInfo structs to be grouped into a single memory >> region. >> >> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> --- >> V7: >> - Remove endianess handling >> - Remove assert() and log missing registers >> V6: >> - Add the memory region later >> V5: >> - Convert to using only one memory region >> >> hw/core/register.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> include/hw/register.h | 43 ++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 101 insertions(+) >> >> diff --git a/hw/core/register.c b/hw/core/register.c >> index cc067f1..149aebb 100644 >> --- a/hw/core/register.c >> +++ b/hw/core/register.c >> @@ -154,3 +154,61 @@ void register_reset(RegisterInfo *reg) >> >> register_write_val(reg, reg->access->reset); >> } >> + >> +void register_write_memory(void *opaque, hwaddr addr, >> + uint64_t value, unsigned size) >> +{ >> + RegisterInfoArray *reg_array = opaque; >> + RegisterInfo *reg = NULL; >> + uint64_t we = ~0; > > Really ~0 and not ~0ULL ? In any case this shouldn't need > initializing as we will always set it later. > >> + int i; >> + >> + for (i = 0; i < reg_array->num_elements; i++) { >> + if (reg_array->r[i]->access->addr == addr) { >> + reg = reg_array->r[i]; >> + break; >> + } >> + } >> + >> + if (!reg) { >> + qemu_log_mask(LOG_UNIMP, "Write to unimplemented register at " \ >> + "address: %#" PRIx64 "\n", addr); > > Shouldn't this be a LOG_GUEST_ERROR ? > >> + return; >> + } >> + >> + /* Generate appropriate write enable mask */ >> + if (reg->data_size < size) { >> + we = MAKE_64BIT_MASK(0, reg->data_size * 8); >> + } else if (reg->data_size >= size) { > > Why not just "else {" ? > >> + we = MAKE_64BIT_MASK(0, size * 8); >> + } >> + >> + register_write(reg, value, we, reg_array->prefix, >> + reg_array->debug); >> +} >> + >> +uint64_t register_read_memory(void *opaque, hwaddr addr, >> + unsigned size) >> +{ >> + RegisterInfoArray *reg_array = opaque; >> + RegisterInfo *reg = NULL; >> + uint64_t read_val; >> + int i; >> + >> + for (i = 0; i < reg_array->num_elements; i++) { >> + if (reg_array->r[i]->access->addr == addr) { >> + reg = reg_array->r[i]; >> + break; >> + } >> + } >> + >> + if (!reg) { >> + qemu_log_mask(LOG_UNIMP, "Read to unimplemented register at " \ >> + "address: %#" PRIx64 "\n", addr); >> + return 0; >> + } >> + >> + read_val = register_read(reg, reg_array->prefix, reg_array->debug); > > This will silently affect bits of the register outside the > specified size with clear-on-read or other "reads have > side effects" behaviour, which isn't consistent with how > we handle writes. > >> /** >> + * This structure is used to group all of the individual registers which are >> + * modeled using the RegisterInfo strucutre. > > "structure" Fixed all. Thanks, Alistair > >> + * >> + * @r is an aray containing of all the relevent RegisterInfo structures. >> + * >> + * @num_elements is the number of elements in the array r >> + * >> + * @mem: optional Memory region for the register >> + */ >> + >> +struct RegisterInfoArray { >> + int num_elements; >> + RegisterInfo **r; >> + >> + bool debug; >> + const char *prefix; >> +}; >> + > > thanks > -- PMM >
diff --git a/hw/core/register.c b/hw/core/register.c index cc067f1..149aebb 100644 --- a/hw/core/register.c +++ b/hw/core/register.c @@ -154,3 +154,61 @@ void register_reset(RegisterInfo *reg) register_write_val(reg, reg->access->reset); } + +void register_write_memory(void *opaque, hwaddr addr, + uint64_t value, unsigned size) +{ + RegisterInfoArray *reg_array = opaque; + RegisterInfo *reg = NULL; + uint64_t we = ~0; + int i; + + for (i = 0; i < reg_array->num_elements; i++) { + if (reg_array->r[i]->access->addr == addr) { + reg = reg_array->r[i]; + break; + } + } + + if (!reg) { + qemu_log_mask(LOG_UNIMP, "Write to unimplemented register at " \ + "address: %#" PRIx64 "\n", addr); + return; + } + + /* Generate appropriate write enable mask */ + if (reg->data_size < size) { + we = MAKE_64BIT_MASK(0, reg->data_size * 8); + } else if (reg->data_size >= size) { + we = MAKE_64BIT_MASK(0, size * 8); + } + + register_write(reg, value, we, reg_array->prefix, + reg_array->debug); +} + +uint64_t register_read_memory(void *opaque, hwaddr addr, + unsigned size) +{ + RegisterInfoArray *reg_array = opaque; + RegisterInfo *reg = NULL; + uint64_t read_val; + int i; + + for (i = 0; i < reg_array->num_elements; i++) { + if (reg_array->r[i]->access->addr == addr) { + reg = reg_array->r[i]; + break; + } + } + + if (!reg) { + qemu_log_mask(LOG_UNIMP, "Read to unimplemented register at " \ + "address: %#" PRIx64 "\n", addr); + return 0; + } + + read_val = register_read(reg, reg_array->prefix, reg_array->debug); + + return extract64(read_val, 0, size * 8); +} diff --git a/include/hw/register.h b/include/hw/register.h index 04c6f47..e160150 100644 --- a/include/hw/register.h +++ b/include/hw/register.h @@ -15,6 +15,7 @@ typedef struct RegisterInfo RegisterInfo; typedef struct RegisterAccessInfo RegisterAccessInfo; +typedef struct RegisterInfoArray RegisterInfoArray; /** * Access description for a register that is part of guest accessible device @@ -51,6 +52,8 @@ struct RegisterAccessInfo { void (*post_write)(RegisterInfo *reg, uint64_t val); uint64_t (*post_read)(RegisterInfo *reg, uint64_t val); + + hwaddr addr; }; /** @@ -79,6 +82,25 @@ struct RegisterInfo { }; /** + * This structure is used to group all of the individual registers which are + * modeled using the RegisterInfo strucutre. + * + * @r is an aray containing of all the relevent RegisterInfo structures. + * + * @num_elements is the number of elements in the array r + * + * @mem: optional Memory region for the register + */ + +struct RegisterInfoArray { + int num_elements; + RegisterInfo **r; + + bool debug; + const char *prefix; +}; + +/** * write a value to a register, subject to its restrictions * @reg: register to write to * @val: value to write @@ -107,4 +129,25 @@ uint64_t register_read(RegisterInfo *reg, const char* prefix, bool debug); void register_reset(RegisterInfo *reg); +/** + * Memory API MMIO write handler that will write to a Register API register. + * @opaque: RegisterInfo to write to + * @addr: Address to write + * @value: Value to write + * @size: Number of bytes to write + */ + +void register_write_memory(void *opaque, hwaddr addr, uint64_t value, + unsigned size); + +/** + * Memory API MMIO read handler that will read from a Register API register. + * @opaque: RegisterInfo to read from + * @addr: Address to read + * @size: Number of bytes to read + * returns: Value read from register + */ + +uint64_t register_read_memory(void *opaque, hwaddr addr, unsigned size); + #endif