diff mbox series

[RFC] hw/registerfields: add `FIELDx_1CLEAR()` macro

Message ID 20220901010220.495112-1-wilfred.mallawa@opensource.wdc.com
State New
Headers show
Series [RFC] hw/registerfields: add `FIELDx_1CLEAR()` macro | expand

Commit Message

Wilfred Mallawa Sept. 1, 2022, 1:02 a.m. UTC 
From: Wilfred Mallawa <wilfred.mallawa@wdc.com>

Adds a helper macro that implements the `rw1c`
behaviour.

Ex:
  uint32_t data = FIELD32_1CLEAR(val, REG, FIELD);

if the specified `FIELD` is set (single/multi bit all fields)
then the respective field is cleared and returned to `data`.

If ALL bits of the bitfield are not set, then no change and
val is returned.

Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
---
 include/hw/registerfields.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Richard Henderson Sept. 1, 2022, 5:32 a.m. UTC | #1 
On 9/1/22 02:02, Wilfred Mallawa wrote:
> From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> 
> Adds a helper macro that implements the `rw1c`
> behaviour.
> 
> Ex:
>    uint32_t data = FIELD32_1CLEAR(val, REG, FIELD);
> 
> if the specified `FIELD` is set (single/multi bit all fields)
> then the respective field is cleared and returned to `data`.
> 
> If ALL bits of the bitfield are not set, then no change and
> val is returned.
> 
> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>

Why do these operations need to go into hw/registerfields.h?
It's not a common operation, since we've never needed it so far.



r~

> ---
>   include/hw/registerfields.h | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/include/hw/registerfields.h b/include/hw/registerfields.h
> index 1330ca77de..5a804f72e3 100644
> --- a/include/hw/registerfields.h
> +++ b/include/hw/registerfields.h
> @@ -115,6 +115,34 @@
>                     R_ ## reg ## _ ## field ## _LENGTH, _v.v);              \
>       _d; })
>   
> +/* Get the max value (uint) discribed by `num_bits` bits */
> +#define MAX_N_BITS(num_bits) ((1 << (num_bits)) - 1)
> +
> +/*
> + * Clear the specified field in reg_val if
> + * all field bits are set, else no changes made. Implements
> + * single/multi-bit `rw1c`
> + */
> +#define FIELD8_1CLEAR(reg_val, reg, field)                                \
> +    ((FIELD_EX8(reg_val, reg, field) ==                                   \
> +      MAX_N_BITS(R_ ## reg ## _ ## field ## _LENGTH)) ?                   \
> +      FIELD_DP8(reg_val, reg, field, 0x00) : reg_val)
> +
> +#define FIELD16_1CLEAR(reg_val, reg, field)                               \
> +    ((FIELD_EX16(reg_val, reg, field) ==                                  \
> +      MAX_N_BITS(R_ ## reg ## _ ## field ## _LENGTH)) ?                   \
> +      FIELD_DP16(reg_val, reg, field, 0x00) : reg_val)
> +
> +#define FIELD32_1CLEAR(reg_val, reg, field)                               \
> +    ((FIELD_EX32(reg_val, reg, field) ==                                  \
> +      MAX_N_BITS(R_ ## reg ## _ ## field ## _LENGTH)) ?                   \
> +      FIELD_DP32(reg_val, reg, field, 0x00) : reg_val)
> +
> +#define FIELD64_1CLEAR(reg_val, reg, field)                               \
> +    ((FIELD_EX64(reg_val, reg, field) ==                                  \
> +      MAX_N_BITS(R_ ## reg ## _ ## field ## _LENGTH)) ?                   \
> +      FIELD_DP64(reg_val, reg, field, 0x00) : reg_val)
> +
>   #define FIELD_SDP8(storage, reg, field, val) ({                           \
>       struct {                                                              \
>           signed int v:R_ ## reg ## _ ## field ## _LENGTH;                  \
Wilfred Mallawa Sept. 17, 2022, 1:14 a.m. UTC | #2 
Ping!

https://lore.kernel.org/qemu-devel/20220901010220.495112-1-wilfred.mallawa@opensource.wdc.com/
Wilfred

On Fri, 2022-09-02 at 01:18 +0200, Philippe Mathieu-Daudé wrote:
> On 1/9/22 07:32, Richard Henderson wrote:
> > On 9/1/22 02:02, Wilfred Mallawa wrote:
> > > From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > > 
> > > Adds a helper macro that implements the `rw1c`
> > > behaviour.
> > > 
> > > Ex:
> > >    uint32_t data = FIELD32_1CLEAR(val, REG, FIELD);
> > > 
> > > if the specified `FIELD` is set (single/multi bit all fields)
> > > then the respective field is cleared and returned to `data`.
> > > 
> > > If ALL bits of the bitfield are not set, then no change and
> > > val is returned.
> > > 
> > > Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> > 
> > Why do these operations need to go into hw/registerfields.h?
> > It's not a common operation, since we've never needed it so far.
> 
> I suggested it to improve readability of this patch:
> https://lore.kernel.org/qemu-devel/c33257a3-645f-9386-52e5-21a15ef0ebe5@amsat.org/
> 
> > > ---
> > >   include/hw/registerfields.h | 28 ++++++++++++++++++++++++++++
> > >   1 file changed, 28 insertions(+)
> > > 
> > > diff --git a/include/hw/registerfields.h
> > > b/include/hw/registerfields.h
> > > index 1330ca77de..5a804f72e3 100644
> > > --- a/include/hw/registerfields.h
> > > +++ b/include/hw/registerfields.h
> > > @@ -115,6 +115,34 @@
> > >                     R_ ## reg ## _ ## field ## _LENGTH, 
> > > _v.v);              \
> > >       _d; })
> > > +/* Get the max value (uint) discribed by `num_bits` bits */
> > > +#define MAX_N_BITS(num_bits) ((1 << (num_bits)) - 1)
> > > +
> > > +/*
> > > + * Clear the specified field in reg_val if
> > > + * all field bits are set, else no changes made. Implements
> > > + * single/multi-bit `rw1c`
> > > + */
> > > +#define FIELD8_1CLEAR(reg_val, reg, 
> > > field)                                \
> > > +    ((FIELD_EX8(reg_val, reg, field) 
> > > ==                                   \
> > > +      MAX_N_BITS(R_ ## reg ## _ ## field ## _LENGTH)) 
> > > ?                   \
> > > +      FIELD_DP8(reg_val, reg, field, 0x00) : reg_val)
> > > +
> > > +#define FIELD16_1CLEAR(reg_val, reg, 
> > > field)                               \
> > > +    ((FIELD_EX16(reg_val, reg, field) 
> > > ==                                  \
> > > +      MAX_N_BITS(R_ ## reg ## _ ## field ## _LENGTH)) 
> > > ?                   \
> > > +      FIELD_DP16(reg_val, reg, field, 0x00) : reg_val)
> > > +
> > > +#define FIELD32_1CLEAR(reg_val, reg, 
> > > field)                               \
> > > +    ((FIELD_EX32(reg_val, reg, field) 
> > > ==                                  \
> > > +      MAX_N_BITS(R_ ## reg ## _ ## field ## _LENGTH)) 
> > > ?                   \
> > > +      FIELD_DP32(reg_val, reg, field, 0x00) : reg_val)
> > > +
> > > +#define FIELD64_1CLEAR(reg_val, reg, 
> > > field)                               \
> > > +    ((FIELD_EX64(reg_val, reg, field) 
> > > ==                                  \
> > > +      MAX_N_BITS(R_ ## reg ## _ ## field ## _LENGTH)) 
> > > ?                   \
> > > +      FIELD_DP64(reg_val, reg, field, 0x00) : reg_val)
> > > +
> > >   #define FIELD_SDP8(storage, reg, field, val) 
> > > ({                           \
> > >       struct 
> > > {                                                              \
> > >           signed int v:R_ ## reg ## _ ## field ## 
> > > _LENGTH;                  \
> > 
> > 
>
Alistair Francis Sept. 23, 2022, 4:32 a.m. UTC | #3 
On Thu, Sep 1, 2022 at 11:03 AM Wilfred Mallawa
<wilfred.mallawa@opensource.wdc.com> wrote:
>
> From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
>
> Adds a helper macro that implements the `rw1c`
> behaviour.
>
> Ex:
>   uint32_t data = FIELD32_1CLEAR(val, REG, FIELD);
>
> if the specified `FIELD` is set (single/multi bit all fields)
> then the respective field is cleared and returned to `data`.
>
> If ALL bits of the bitfield are not set, then no change and
> val is returned.

I feel like the value is operating over the entire field.

Say for example there is an interrupt pending register, with a current
value of 0b0010.

If a guest writes 0b1111 they expect ALL of the bits to be cleared
(for a w1c register). I think the macro should handle that, instead of
requiring the exact bits to be set.

Alistair

>
> Signed-off-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> ---
>  include/hw/registerfields.h | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/include/hw/registerfields.h b/include/hw/registerfields.h
> index 1330ca77de..5a804f72e3 100644
> --- a/include/hw/registerfields.h
> +++ b/include/hw/registerfields.h
> @@ -115,6 +115,34 @@
>                    R_ ## reg ## _ ## field ## _LENGTH, _v.v);              \
>      _d; })
>
> +/* Get the max value (uint) discribed by `num_bits` bits */
> +#define MAX_N_BITS(num_bits) ((1 << (num_bits)) - 1)
> +
> +/*
> + * Clear the specified field in reg_val if
> + * all field bits are set, else no changes made. Implements
> + * single/multi-bit `rw1c`
> + */
> +#define FIELD8_1CLEAR(reg_val, reg, field)                                \
> +    ((FIELD_EX8(reg_val, reg, field) ==                                   \
> +      MAX_N_BITS(R_ ## reg ## _ ## field ## _LENGTH)) ?                   \
> +      FIELD_DP8(reg_val, reg, field, 0x00) : reg_val)
> +
> +#define FIELD16_1CLEAR(reg_val, reg, field)                               \
> +    ((FIELD_EX16(reg_val, reg, field) ==                                  \
> +      MAX_N_BITS(R_ ## reg ## _ ## field ## _LENGTH)) ?                   \
> +      FIELD_DP16(reg_val, reg, field, 0x00) : reg_val)
> +
> +#define FIELD32_1CLEAR(reg_val, reg, field)                               \
> +    ((FIELD_EX32(reg_val, reg, field) ==                                  \
> +      MAX_N_BITS(R_ ## reg ## _ ## field ## _LENGTH)) ?                   \
> +      FIELD_DP32(reg_val, reg, field, 0x00) : reg_val)
> +
> +#define FIELD64_1CLEAR(reg_val, reg, field)                               \
> +    ((FIELD_EX64(reg_val, reg, field) ==                                  \
> +      MAX_N_BITS(R_ ## reg ## _ ## field ## _LENGTH)) ?                   \
> +      FIELD_DP64(reg_val, reg, field, 0x00) : reg_val)
> +
>  #define FIELD_SDP8(storage, reg, field, val) ({                           \
>      struct {                                                              \
>          signed int v:R_ ## reg ## _ ## field ## _LENGTH;                  \
> --
> 2.37.2
>
>
diff mbox series

Patch

diff --git a/include/hw/registerfields.h b/include/hw/registerfields.h
index 1330ca77de..5a804f72e3 100644
--- a/include/hw/registerfields.h
+++ b/include/hw/registerfields.h
@@ -115,6 +115,34 @@ 
                   R_ ## reg ## _ ## field ## _LENGTH, _v.v);              \
     _d; })
 
+/* Get the max value (uint) discribed by `num_bits` bits */
+#define MAX_N_BITS(num_bits) ((1 << (num_bits)) - 1)
+
+/*
+ * Clear the specified field in reg_val if
+ * all field bits are set, else no changes made. Implements
+ * single/multi-bit `rw1c`
+ */
+#define FIELD8_1CLEAR(reg_val, reg, field)                                \
+    ((FIELD_EX8(reg_val, reg, field) ==                                   \
+      MAX_N_BITS(R_ ## reg ## _ ## field ## _LENGTH)) ?                   \
+      FIELD_DP8(reg_val, reg, field, 0x00) : reg_val)
+
+#define FIELD16_1CLEAR(reg_val, reg, field)                               \
+    ((FIELD_EX16(reg_val, reg, field) ==                                  \
+      MAX_N_BITS(R_ ## reg ## _ ## field ## _LENGTH)) ?                   \
+      FIELD_DP16(reg_val, reg, field, 0x00) : reg_val)
+
+#define FIELD32_1CLEAR(reg_val, reg, field)                               \
+    ((FIELD_EX32(reg_val, reg, field) ==                                  \
+      MAX_N_BITS(R_ ## reg ## _ ## field ## _LENGTH)) ?                   \
+      FIELD_DP32(reg_val, reg, field, 0x00) : reg_val)
+
+#define FIELD64_1CLEAR(reg_val, reg, field)                               \
+    ((FIELD_EX64(reg_val, reg, field) ==                                  \
+      MAX_N_BITS(R_ ## reg ## _ ## field ## _LENGTH)) ?                   \
+      FIELD_DP64(reg_val, reg, field, 0x00) : reg_val)
+
 #define FIELD_SDP8(storage, reg, field, val) ({                           \
     struct {                                                              \
         signed int v:R_ ## reg ## _ ## field ## _LENGTH;                  \