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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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>