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

[Peter Maydell <peter.maydell@linaro.org>]

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