[PATCH v3] hw/net: cadence_gem: feat: add logic for the DISABLE_MASK bit in type2_compare_x_word_1

[PATCH v3] hw/net: cadence_gem: feat: add logic for the DISABLE_MASK bit in type2_compare_x_word_1

[Andrew.Yuan]

From: Andrew Yuan <andrew.yuan@jaguarmicro.com>

As in the Cadence IP for Gigabit Ethernet MAC Part Number: IP7014 IP Rev: R1p12 - Doc Rev: 1.3 User Guide,
if the DISABLE_MASK bit in type2_compare_x_word_1 is set,
mask_value in type2_compare_x_word_0 is used as an additional 2 byte Compare Value

Signed-off-by: Andrew Yuan <andrew.yuan@jaguarmicro.com>
Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/net/cadence_gem.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 3fce01315f..7bd176951e 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -909,8 +909,8 @@ static int get_queue_from_screen(CadenceGEMState *s, uint8_t *rxbuf_ptr,
 
         /* Compare A, B, C */
         for (j = 0; j < 3; j++) {
-            uint32_t cr0, cr1, mask, compare;
-            uint16_t rx_cmp;
+            uint32_t cr0, cr1, mask, compare, disable_mask;
+            uint32_t rx_cmp;
             int offset;
             int cr_idx = extract32(reg, R_SCREENING_TYPE2_REG0_COMPARE_A_SHIFT + j * 6,
                                    R_SCREENING_TYPE2_REG0_COMPARE_A_LENGTH);
@@ -946,9 +946,23 @@ static int get_queue_from_screen(CadenceGEMState *s, uint8_t *rxbuf_ptr,
                 break;
             }
 
-            rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];
-            mask = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0, MASK_VALUE);
-            compare = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0, COMPARE_VALUE);
+            disable_mask =
+                FIELD_EX32(cr1, TYPE2_COMPARE_0_WORD_1, DISABLE_MASK);
+            if (disable_mask) {
+                /*
+                 * If disable_mask is set,
+                 * mask_value is used as an additional 2 byte Compare Value.
+                 * To simple, set mask = 0xFFFFFFFF, if disable_mask is set.
+                 */
+                rx_cmp = ldl_le_p(rxbuf_ptr + offset);
+                mask = 0xFFFFFFFF;
+                compare = cr0;
+            } else {
+                rx_cmp = lduw_le_p(rxbuf_ptr + offset);
+                mask = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0, MASK_VALUE);
+                compare =
+                    FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0, COMPARE_VALUE);
+            }
 
             if ((rx_cmp & mask) == (compare & mask)) {
                 matched = true;
-- 
2.25.1

Re: [PATCH v3] hw/net: cadence_gem: feat: add logic for the DISABLE_MASK bit in type2_compare_x_word_1

[Peter Maydell]

Edgar or Alistair -- could one of you review this
cadence GEM patch, please?

On Thu, 19 Dec 2024 at 06:17, Andrew.Yuan <andrew.yuan@jaguarmicro.com> wrote:
>
> From: Andrew Yuan <andrew.yuan@jaguarmicro.com>
>
> As in the Cadence IP for Gigabit Ethernet MAC Part Number: IP7014 IP Rev: R1p12 - Doc Rev: 1.3 User Guide,
> if the DISABLE_MASK bit in type2_compare_x_word_1 is set,
> mask_value in type2_compare_x_word_0 is used as an additional 2 byte Compare Value
>
> Signed-off-by: Andrew Yuan <andrew.yuan@jaguarmicro.com>
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/net/cadence_gem.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 3fce01315f..7bd176951e 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -909,8 +909,8 @@ static int get_queue_from_screen(CadenceGEMState *s, uint8_t *rxbuf_ptr,
>
>          /* Compare A, B, C */
>          for (j = 0; j < 3; j++) {
> -            uint32_t cr0, cr1, mask, compare;
> -            uint16_t rx_cmp;
> +            uint32_t cr0, cr1, mask, compare, disable_mask;
> +            uint32_t rx_cmp;
>              int offset;
>              int cr_idx = extract32(reg, R_SCREENING_TYPE2_REG0_COMPARE_A_SHIFT + j * 6,
>                                     R_SCREENING_TYPE2_REG0_COMPARE_A_LENGTH);
> @@ -946,9 +946,23 @@ static int get_queue_from_screen(CadenceGEMState *s, uint8_t *rxbuf_ptr,
>                  break;
>              }
>
> -            rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];
> -            mask = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0, MASK_VALUE);
> -            compare = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0, COMPARE_VALUE);
> +            disable_mask =
> +                FIELD_EX32(cr1, TYPE2_COMPARE_0_WORD_1, DISABLE_MASK);
> +            if (disable_mask) {
> +                /*
> +                 * If disable_mask is set,
> +                 * mask_value is used as an additional 2 byte Compare Value.
> +                 * To simple, set mask = 0xFFFFFFFF, if disable_mask is set.
> +                 */
> +                rx_cmp = ldl_le_p(rxbuf_ptr + offset);
> +                mask = 0xFFFFFFFF;
> +                compare = cr0;
> +            } else {
> +                rx_cmp = lduw_le_p(rxbuf_ptr + offset);

Is the change in behaviour in the !disable_mask codepath here
intentional? Previously we use one byte from rxbuf_ptr[offset],
duplicated into both halves of rx_cmp; now we will load two
different bytes from rxbuf_ptr[offset] and rxbuf_ptr[offset + 1].

If this is intended, we should say so in the commit message.


> +                mask = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0, MASK_VALUE);
> +                compare =
> +                    FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0, COMPARE_VALUE);
> +            }
>
>              if ((rx_cmp & mask) == (compare & mask)) {
>                  matched = true;
> --
> 2.25.1

thanks
-- PMM

Re: [PATCH v3] hw/net: cadence_gem: feat: add logic for the DISABLE_MASK bit in type2_compare_x_word_1

[Edgar E. Iglesias]

[-- Attachment #1: Type: text/plain, Size: 3482 bytes --]

On Mon, Jan 27, 2025 at 8:40 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> Edgar or Alistair -- could one of you review this
> cadence GEM patch, please?
>
>
Sorry for the delay!



> On Thu, 19 Dec 2024 at 06:17, Andrew.Yuan <andrew.yuan@jaguarmicro.com>
> wrote:
> >
> > From: Andrew Yuan <andrew.yuan@jaguarmicro.com>
> >
> > As in the Cadence IP for Gigabit Ethernet MAC Part Number: IP7014 IP
> Rev: R1p12 - Doc Rev: 1.3 User Guide,
> > if the DISABLE_MASK bit in type2_compare_x_word_1 is set,
> > mask_value in type2_compare_x_word_0 is used as an additional 2 byte
> Compare Value
> >
> > Signed-off-by: Andrew Yuan <andrew.yuan@jaguarmicro.com>
> > Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >  hw/net/cadence_gem.c | 24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> > index 3fce01315f..7bd176951e 100644
> > --- a/hw/net/cadence_gem.c
> > +++ b/hw/net/cadence_gem.c
> > @@ -909,8 +909,8 @@ static int get_queue_from_screen(CadenceGEMState *s,
> uint8_t *rxbuf_ptr,
> >
> >          /* Compare A, B, C */
> >          for (j = 0; j < 3; j++) {
> > -            uint32_t cr0, cr1, mask, compare;
> > -            uint16_t rx_cmp;
> > +            uint32_t cr0, cr1, mask, compare, disable_mask;
> > +            uint32_t rx_cmp;
> >              int offset;
> >              int cr_idx = extract32(reg,
> R_SCREENING_TYPE2_REG0_COMPARE_A_SHIFT + j * 6,
> >
>  R_SCREENING_TYPE2_REG0_COMPARE_A_LENGTH);
> > @@ -946,9 +946,23 @@ static int get_queue_from_screen(CadenceGEMState
> *s, uint8_t *rxbuf_ptr,
> >                  break;
> >              }
> >
> > -            rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];
> > -            mask = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0, MASK_VALUE);
> > -            compare = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0,
> COMPARE_VALUE);
> > +            disable_mask =
> > +                FIELD_EX32(cr1, TYPE2_COMPARE_0_WORD_1, DISABLE_MASK);
> > +            if (disable_mask) {
> > +                /*
> > +                 * If disable_mask is set,
> > +                 * mask_value is used as an additional 2 byte Compare
> Value.
> > +                 * To simple, set mask = 0xFFFFFFFF, if disable_mask is
> set.
> > +                 */
> > +                rx_cmp = ldl_le_p(rxbuf_ptr + offset);
> > +                mask = 0xFFFFFFFF;
> > +                compare = cr0;
> > +            } else {
> > +                rx_cmp = lduw_le_p(rxbuf_ptr + offset);
>
> Is the change in behaviour in the !disable_mask codepath here
> intentional? Previously we use one byte from rxbuf_ptr[offset],
> duplicated into both halves of rx_cmp; now we will load two
> different bytes from rxbuf_ptr[offset] and rxbuf_ptr[offset + 1].
>
> If this is intended, we should say so in the commit message.
>
>
I agree that it should be mentioned (looks like a correct bugfix).
Other than that this patch looks good to me!

Cheers,
Edgar


>
> > +                mask = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0,
> MASK_VALUE);
> > +                compare =
> > +                    FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0,
> COMPARE_VALUE);
> > +            }
> >
> >              if ((rx_cmp & mask) == (compare & mask)) {
> >                  matched = true;
> > --
> > 2.25.1
>
> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 5056 bytes --]

Re: [PATCH v3] hw/net: cadence_gem: feat: add logic for the DISABLE_MASK bit in type2_compare_x_word_1

[Peter Maydell]

On Thu, 30 Jan 2025 at 22:31, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> On Mon, Jan 27, 2025 at 8:40 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>> On Thu, 19 Dec 2024 at 06:17, Andrew.Yuan <andrew.yuan@jaguarmicro.com> wrote:
>> > -            rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];
>> > -            mask = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0, MASK_VALUE);
>> > -            compare = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0, COMPARE_VALUE);
>> > +            disable_mask =
>> > +                FIELD_EX32(cr1, TYPE2_COMPARE_0_WORD_1, DISABLE_MASK);
>> > +            if (disable_mask) {
>> > +                /*
>> > +                 * If disable_mask is set,
>> > +                 * mask_value is used as an additional 2 byte Compare Value.
>> > +                 * To simple, set mask = 0xFFFFFFFF, if disable_mask is set.
>> > +                 */
>> > +                rx_cmp = ldl_le_p(rxbuf_ptr + offset);
>> > +                mask = 0xFFFFFFFF;
>> > +                compare = cr0;
>> > +            } else {
>> > +                rx_cmp = lduw_le_p(rxbuf_ptr + offset);
>>
>> Is the change in behaviour in the !disable_mask codepath here
>> intentional? Previously we use one byte from rxbuf_ptr[offset],
>> duplicated into both halves of rx_cmp; now we will load two
>> different bytes from rxbuf_ptr[offset] and rxbuf_ptr[offset + 1].
>>
>> If this is intended, we should say so in the commit message.
>>
>
> I agree that it should be mentioned (looks like a correct bugfix).

Thanks. I've expanded the commit message:

    hw/net/cadence_gem:  Fix the mask/compare/disable-mask logic

    Our current handling of the mask/compare logic in the Cadence
    GEM ethernet device is wrong:
     (1) we load the same byte twice from rx_buf when
         creating the compare value
     (2) we ignore the DISABLE_MASK flag

    The "Cadence IP for Gigabit Ethernet MAC Part Number: IP7014 IP Rev:
    R1p12 - Doc Rev: 1.3 User Guide" states that if the DISABLE_MASK bit
    in type2_compare_x_word_1 is set, the mask_value field in
    type2_compare_x_word_0 is used as an additional 2 byte Compare Value.

    Correct these bugs:
     * in the !disable_mask codepath, use lduw_le_p() so we
       correctly load a 16-bit value for comparison
     * in the disable_mask codepath, we load a full 4-byte value
       from rx_buf for the comparison, set the compare value to
       the whole of the cr0 register (i.e. the concatenation of
       the mask and compare fields), and set mask to 0xffffffff
       to force a 32-bit comparison

and also tweaked the comment a bit:

+                /*
+                 * If disable_mask is set, mask_value is used as an
+                 * additional 2 byte Compare Value; that is equivalent
+                 * to using the whole cr0 register as the comparison value.
+                 * Load 32 bits of data from rx_buf, and set mask to
+                 * all-ones so we compare all 32 bits.
+                 */

and applied this to target-arm.next.

> Other than that this patch looks good to me!

Can I call that a Reviewed-by (with the above changes)?

thanks
-- PMM

答复: [PATCH v3] hw/net: cadence_gem: feat: add logic for the DISABLE_MASK bit in type2_compare_x_word_1

[andrew Yuan]

On Tue, 4 Feb 2025 at 22:37, Peter Maydell <peter.maydell@linaro.org> wrote:
> On Thu, 30 Jan 2025 at 22:31, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > On Mon, Jan 27, 2025 at 8:40 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> >> On Thu, 19 Dec 2024 at 06:17, Andrew.Yuan <andrew.yuan@jaguarmicro.com> wrote:
> >> > -            rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];
> >> > -            mask = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0, MASK_VALUE);
> >> > -            compare = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0, COMPARE_VALUE);
> >> > +            disable_mask =
> >> > +                FIELD_EX32(cr1, TYPE2_COMPARE_0_WORD_1, DISABLE_MASK);
> >> > +            if (disable_mask) {
> >> > +                /*
> >> > +                 * If disable_mask is set,
> >> > +                 * mask_value is used as an additional 2 byte Compare Value.
> >> > +                 * To simple, set mask = 0xFFFFFFFF, if disable_mask is set.
> >> > +                 */
> >> > +                rx_cmp = ldl_le_p(rxbuf_ptr + offset);
> >> > +                mask = 0xFFFFFFFF;
> >> > +                compare = cr0;
> >> > +            } else {
> >> > +                rx_cmp = lduw_le_p(rxbuf_ptr + offset);
> >>
> >> Is the change in behaviour in the !disable_mask codepath here
> >> intentional? Previously we use one byte from rxbuf_ptr[offset],
> >> duplicated into both halves of rx_cmp; now we will load two different
> >> bytes from rxbuf_ptr[offset] and rxbuf_ptr[offset + 1].
> >>
> >> If this is intended, we should say so in the commit message.
> >>
> >
> > I agree that it should be mentioned (looks like a correct bugfix).
> 
> Thanks. I've expanded the commit message:
> 
>     hw/net/cadence_gem:  Fix the mask/compare/disable-mask logic
> 
>     Our current handling of the mask/compare logic in the Cadence
>     GEM ethernet device is wrong:
>      (1) we load the same byte twice from rx_buf when
>          creating the compare value
>      (2) we ignore the DISABLE_MASK flag
> 
>     The "Cadence IP for Gigabit Ethernet MAC Part Number: IP7014 IP Rev:
>     R1p12 - Doc Rev: 1.3 User Guide" states that if the DISABLE_MASK bit
>     in type2_compare_x_word_1 is set, the mask_value field in
>     type2_compare_x_word_0 is used as an additional 2 byte Compare Value.
> 
>     Correct these bugs:
>      * in the !disable_mask codepath, use lduw_le_p() so we
>        correctly load a 16-bit value for comparison
>      * in the disable_mask codepath, we load a full 4-byte value
>        from rx_buf for the comparison, set the compare value to
>        the whole of the cr0 register (i.e. the concatenation of
>        the mask and compare fields), and set mask to 0xffffffff
>        to force a 32-bit comparison
> 
> and also tweaked the comment a bit:
> 
> +                /*
> +                 * If disable_mask is set, mask_value is used as an
> +                 * additional 2 byte Compare Value; that is equivalent
> +                 * to using the whole cr0 register as the comparison value.
> +                 * Load 32 bits of data from rx_buf, and set mask to
> +                 * all-ones so we compare all 32 bits.
> +                 */
> 
> and applied this to target-arm.next.
> 
> > Other than that this patch looks good to me!
> 
> Can I call that a Reviewed-by (with the above changes)?

Yes, Thanks for your time;

> thanks
> -- PMM

Re: [PATCH v3] hw/net: cadence_gem: feat: add logic for the DISABLE_MASK bit in type2_compare_x_word_1

[Edgar E. Iglesias]

[-- Attachment #1: Type: text/plain, Size: 3506 bytes --]

On Tue, 4 Feb 2025 at 08:37, Peter Maydell <peter.maydell@linaro.org> wrote:

> On Thu, 30 Jan 2025 at 22:31, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
> > On Mon, Jan 27, 2025 at 8:40 AM Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >> On Thu, 19 Dec 2024 at 06:17, Andrew.Yuan <andrew.yuan@jaguarmicro.com>
> wrote:
> >> > -            rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];
> >> > -            mask = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0,
> MASK_VALUE);
> >> > -            compare = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0,
> COMPARE_VALUE);
> >> > +            disable_mask =
> >> > +                FIELD_EX32(cr1, TYPE2_COMPARE_0_WORD_1,
> DISABLE_MASK);
> >> > +            if (disable_mask) {
> >> > +                /*
> >> > +                 * If disable_mask is set,
> >> > +                 * mask_value is used as an additional 2 byte
> Compare Value.
> >> > +                 * To simple, set mask = 0xFFFFFFFF, if disable_mask
> is set.
> >> > +                 */
> >> > +                rx_cmp = ldl_le_p(rxbuf_ptr + offset);
> >> > +                mask = 0xFFFFFFFF;
> >> > +                compare = cr0;
> >> > +            } else {
> >> > +                rx_cmp = lduw_le_p(rxbuf_ptr + offset);
> >>
> >> Is the change in behaviour in the !disable_mask codepath here
> >> intentional? Previously we use one byte from rxbuf_ptr[offset],
> >> duplicated into both halves of rx_cmp; now we will load two
> >> different bytes from rxbuf_ptr[offset] and rxbuf_ptr[offset + 1].
> >>
> >> If this is intended, we should say so in the commit message.
> >>
> >
> > I agree that it should be mentioned (looks like a correct bugfix).
>
> Thanks. I've expanded the commit message:
>
>     hw/net/cadence_gem:  Fix the mask/compare/disable-mask logic
>
>     Our current handling of the mask/compare logic in the Cadence
>     GEM ethernet device is wrong:
>      (1) we load the same byte twice from rx_buf when
>          creating the compare value
>      (2) we ignore the DISABLE_MASK flag
>
>     The "Cadence IP for Gigabit Ethernet MAC Part Number: IP7014 IP Rev:
>     R1p12 - Doc Rev: 1.3 User Guide" states that if the DISABLE_MASK bit
>     in type2_compare_x_word_1 is set, the mask_value field in
>     type2_compare_x_word_0 is used as an additional 2 byte Compare Value.
>
>     Correct these bugs:
>      * in the !disable_mask codepath, use lduw_le_p() so we
>        correctly load a 16-bit value for comparison
>      * in the disable_mask codepath, we load a full 4-byte value
>        from rx_buf for the comparison, set the compare value to
>        the whole of the cr0 register (i.e. the concatenation of
>        the mask and compare fields), and set mask to 0xffffffff
>        to force a 32-bit comparison
>
> and also tweaked the comment a bit:
>
> +                /*
> +                 * If disable_mask is set, mask_value is used as an
> +                 * additional 2 byte Compare Value; that is equivalent
> +                 * to using the whole cr0 register as the comparison
> value.
> +                 * Load 32 bits of data from rx_buf, and set mask to
> +                 * all-ones so we compare all 32 bits.
> +                 */
>
> and applied this to target-arm.next.
>
> > Other than that this patch looks good to me!
>
> Can I call that a Reviewed-by (with the above changes)?


Yes, thanks!!



>
> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 4948 bytes --]

Re: [PATCH v3] hw/net: cadence_gem: feat: add logic for the DISABLE_MASK bit in type2_compare_x_word_1

[Philippe Mathieu-Daudé]

On 4/2/25 15:37, Peter Maydell wrote:
> On Thu, 30 Jan 2025 at 22:31, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
>> On Mon, Jan 27, 2025 at 8:40 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On Thu, 19 Dec 2024 at 06:17, Andrew.Yuan <andrew.yuan@jaguarmicro.com> wrote:
>>>> -            rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];
>>>> -            mask = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0, MASK_VALUE);
>>>> -            compare = FIELD_EX32(cr0, TYPE2_COMPARE_0_WORD_0, COMPARE_VALUE);
>>>> +            disable_mask =
>>>> +                FIELD_EX32(cr1, TYPE2_COMPARE_0_WORD_1, DISABLE_MASK);
>>>> +            if (disable_mask) {
>>>> +                /*
>>>> +                 * If disable_mask is set,
>>>> +                 * mask_value is used as an additional 2 byte Compare Value.
>>>> +                 * To simple, set mask = 0xFFFFFFFF, if disable_mask is set.
>>>> +                 */
>>>> +                rx_cmp = ldl_le_p(rxbuf_ptr + offset);
>>>> +                mask = 0xFFFFFFFF;
>>>> +                compare = cr0;
>>>> +            } else {
>>>> +                rx_cmp = lduw_le_p(rxbuf_ptr + offset);
>>>
>>> Is the change in behaviour in the !disable_mask codepath here
>>> intentional? Previously we use one byte from rxbuf_ptr[offset],
>>> duplicated into both halves of rx_cmp; now we will load two
>>> different bytes from rxbuf_ptr[offset] and rxbuf_ptr[offset + 1].
>>>
>>> If this is intended, we should say so in the commit message.
>>>
>>
>> I agree that it should be mentioned (looks like a correct bugfix).
> 
> Thanks. I've expanded the commit message:
> 
>      hw/net/cadence_gem:  Fix the mask/compare/disable-mask logic
> 
>      Our current handling of the mask/compare logic in the Cadence
>      GEM ethernet device is wrong:
>       (1) we load the same byte twice from rx_buf when
>           creating the compare value
>       (2) we ignore the DISABLE_MASK flag
> 
>      The "Cadence IP for Gigabit Ethernet MAC Part Number: IP7014 IP Rev:
>      R1p12 - Doc Rev: 1.3 User Guide" states that if the DISABLE_MASK bit
>      in type2_compare_x_word_1 is set, the mask_value field in
>      type2_compare_x_word_0 is used as an additional 2 byte Compare Value.
> 
>      Correct these bugs:
>       * in the !disable_mask codepath, use lduw_le_p() so we
>         correctly load a 16-bit value for comparison
>       * in the disable_mask codepath, we load a full 4-byte value
>         from rx_buf for the comparison, set the compare value to
>         the whole of the cr0 register (i.e. the concatenation of
>         the mask and compare fields), and set mask to 0xffffffff
>         to force a 32-bit comparison
> 
> and also tweaked the comment a bit:
> 
> +                /*
> +                 * If disable_mask is set, mask_value is used as an
> +                 * additional 2 byte Compare Value; that is equivalent
> +                 * to using the whole cr0 register as the comparison value.
> +                 * Load 32 bits of data from rx_buf, and set mask to
> +                 * all-ones so we compare all 32 bits.
> +                 */
> 
> and applied this to target-arm.next.
> 
>> Other than that this patch looks good to me!
> 
> Can I call that a Reviewed-by (with the above changes)?

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>