In the Cadence IP for Gigabit Ethernet MAC Part Number: IP7014 IP Rev: R1p12 - Doc Rev: 1.3 User Guide, the specification for the type2_compare_x_word_0 register is as follows:
        The byte stored in bits [23:16] is compared against the byte in the received frame from the selected offset+0, and the byte stored in bits [31:24] is compared against the byte in
        the received frame from the selected offset+1.

        However, there is an implementation error in the cadence_gem model in qemu：
        the byte stored in bits [31:24] is compared against the byte in the received frame from the selected offset+0

        Now, the error code is as follows:
        rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];

        and needs to be corrected to：
        rx_cmp = rxbuf_ptr[offset + 1] << 8 | rxbuf_ptr[offset];

Signed-off-by: Andrew.Yuan <andrew.yuan@jaguarmicro.com>
---
 hw/net/cadence_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index ec7bf562e5..9c73ded0d3 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -946,7 +946,7 @@ static int get_queue_from_screen(CadenceGEMState *s, uint8_t *rxbuf_ptr,
                 break;
             }
 
-            rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];
+            rx_cmp = rxbuf_ptr[offset + 1] << 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);
 
-- 
2.37.0.windows.1

On Thu, Jun 6, 2024 at 12:00 PM Andrew.Yuan <andrew.yuan@jaguarmicro.com>
wrote:

>         In the Cadence IP for Gigabit Ethernet MAC Part Number: IP7014 IP
> Rev: R1p12 - Doc Rev: 1.3 User Guide, the specification for the
> type2_compare_x_word_0 register is as follows:
>         The byte stored in bits [23:16] is compared against the byte in
> the received frame from the selected offset+0, and the byte stored in bits
> [31:24] is compared against the byte in
>         the received frame from the selected offset+1.
>
>         However, there is an implementation error in the cadence_gem model
> in qemu：
>         the byte stored in bits [31:24] is compared against the byte in
> the received frame from the selected offset+0
>
>         Now, the error code is as follows:
>         rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];
>
>         and needs to be corrected to：
>         rx_cmp = rxbuf_ptr[offset + 1] << 8 | rxbuf_ptr[offset];
>
> Signed-off-by: Andrew.Yuan <andrew.yuan@jaguarmicro.com>
>


LGTM:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>

At some point it would be nice to add the missing logic for the
DISABLE_MASK bit that
extends the compare range from 16 to 32-bits.

Cheers,
Edgar



> ---
>  hw/net/cadence_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index ec7bf562e5..9c73ded0d3 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -946,7 +946,7 @@ static int get_queue_from_screen(CadenceGEMState *s,
> uint8_t *rxbuf_ptr,
>                  break;
>              }
>
> -            rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];
> +            rx_cmp = rxbuf_ptr[offset + 1] << 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);
>
> --
> 2.37.0.windows.1
>
>

On Thu, 6 Jun 2024 at 12:04, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
>
> On Thu, Jun 6, 2024 at 12:00 PM Andrew.Yuan <andrew.yuan@jaguarmicro.com> wrote:
>>
>>         In the Cadence IP for Gigabit Ethernet MAC Part Number: IP7014 IP Rev: R1p12 - Doc Rev: 1.3 User Guide, the specification for the type2_compare_x_word_0 register is as follows:
>>         The byte stored in bits [23:16] is compared against the byte in the received frame from the selected offset+0, and the byte stored in bits [31:24] is compared against the byte in
>>         the received frame from the selected offset+1.
>>
>>         However, there is an implementation error in the cadence_gem model in qemu：
>>         the byte stored in bits [31:24] is compared against the byte in the received frame from the selected offset+0
>>
>>         Now, the error code is as follows:
>>         rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];
>>
>>         and needs to be corrected to：
>>         rx_cmp = rxbuf_ptr[offset + 1] << 8 | rxbuf_ptr[offset];
>>
>> Signed-off-by: Andrew.Yuan <andrew.yuan@jaguarmicro.com>
>
>
>
> LGTM:
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
>
> At some point it would be nice to add the missing logic for the DISABLE_MASK bit that
> extends the compare range from 16 to 32-bits.

I had a look at this device's code, and I'm trying to
figure out how we know at this point that there really are
two bytes pointed to by rxbuf_ptr.
 * The get_queue_from_screen() function takes a rxbufsize
   argument, but it never uses it...
 * the callsite in gem_receive() will (in the "strip FCS" case)
   pass its buf argument as rxbuf_ptr, but it will use a
   rxbufsize argument which has been raised to at least
   GEM_DMACFG_RBUFSZ_MUL, even if the input size argument
   is smaller, so even if get_queue_from_screen() honoured
   its rxbufsize argument it wouldn't help

Would somebody who understands the device like to have a look ?

This is a separate issue from the incorrect array offset
argument this patch fixes, though.

thanks
-- PMM

Yes, Need to check whether the "offset" exceeds the "size" argument of gem_receive() in get_queue_from_screen() function;


-----邮件原件-----
发件人: Peter Maydell <peter.maydell@linaro.org> 
发送时间: 2024年6月6日 20:06
收件人: Edgar E. Iglesias <edgar.iglesias@gmail.com>
抄送: andrew Yuan <andrew.yuan@jaguarmicro.com>; luc.michel@amd.com; alistair@alistair23.me; jasowang@redhat.com; qemu-arm@nongnu.org; qemu-devel@nongnu.org
主题: Re: [PATCH] hw/net: cadence_gem: fix: type2_compare_x_word_0 error

External Mail: This email originated from OUTSIDE of the organization!
Do not click links, open attachments or provide ANY information unless you recognize the sender and know the content is safe.


On Thu, 6 Jun 2024 at 12:04, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
>
> On Thu, Jun 6, 2024 at 12:00 PM Andrew.Yuan <andrew.yuan@jaguarmicro.com> wrote:
>>
>>         In the Cadence IP for Gigabit Ethernet MAC Part Number: IP7014 IP Rev: R1p12 - Doc Rev: 1.3 User Guide, the specification for the type2_compare_x_word_0 register is as follows:
>>         The byte stored in bits [23:16] is compared against the byte in the received frame from the selected offset+0, and the byte stored in bits [31:24] is compared against the byte in
>>         the received frame from the selected offset+1.
>>
>>         However, there is an implementation error in the cadence_gem model in qemu：
>>         the byte stored in bits [31:24] is compared against the byte 
>> in the received frame from the selected offset+0
>>
>>         Now, the error code is as follows:
>>         rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];
>>
>>         and needs to be corrected to：
>>         rx_cmp = rxbuf_ptr[offset + 1] << 8 | rxbuf_ptr[offset];
>>
>> Signed-off-by: Andrew.Yuan <andrew.yuan@jaguarmicro.com>
>
>
>
> LGTM:
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
>
> At some point it would be nice to add the missing logic for the 
> DISABLE_MASK bit that extends the compare range from 16 to 32-bits.

I had a look at this device's code, and I'm trying to figure out how we know at this point that there really are two bytes pointed to by rxbuf_ptr.
 * The get_queue_from_screen() function takes a rxbufsize
   argument, but it never uses it...
 * the callsite in gem_receive() will (in the "strip FCS" case)
   pass its buf argument as rxbuf_ptr, but it will use a
   rxbufsize argument which has been raised to at least
   GEM_DMACFG_RBUFSZ_MUL, even if the input size argument
   is smaller, so even if get_queue_from_screen() honoured
   its rxbufsize argument it wouldn't help

Would somebody who understands the device like to have a look ?

This is a separate issue from the incorrect array offset argument this patch fixes, though.

thanks
-- PMM

On Thu, Jun 6, 2024 at 2:06 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Thu, 6 Jun 2024 at 12:04, Edgar E. Iglesias <edgar.iglesias@gmail.com>
> wrote:
> >
> > On Thu, Jun 6, 2024 at 12:00 PM Andrew.Yuan <andrew.yuan@jaguarmicro.com>
> wrote:
> >>
> >>         In the Cadence IP for Gigabit Ethernet MAC Part Number: IP7014
> IP Rev: R1p12 - Doc Rev: 1.3 User Guide, the specification for the
> type2_compare_x_word_0 register is as follows:
> >>         The byte stored in bits [23:16] is compared against the byte in
> the received frame from the selected offset+0, and the byte stored in bits
> [31:24] is compared against the byte in
> >>         the received frame from the selected offset+1.
> >>
> >>         However, there is an implementation error in the cadence_gem
> model in qemu：
> >>         the byte stored in bits [31:24] is compared against the byte in
> the received frame from the selected offset+0
> >>
> >>         Now, the error code is as follows:
> >>         rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];
> >>
> >>         and needs to be corrected to：
> >>         rx_cmp = rxbuf_ptr[offset + 1] << 8 | rxbuf_ptr[offset];
> >>
> >> Signed-off-by: Andrew.Yuan <andrew.yuan@jaguarmicro.com>
> >
> >
> >
> > LGTM:
> > Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> >
> > At some point it would be nice to add the missing logic for the
> DISABLE_MASK bit that
> > extends the compare range from 16 to 32-bits.
>
> I had a look at this device's code, and I'm trying to
> figure out how we know at this point that there really are
> two bytes pointed to by rxbuf_ptr.
>  * The get_queue_from_screen() function takes a rxbufsize
>    argument, but it never uses it...
>  * the callsite in gem_receive() will (in the "strip FCS" case)
>    pass its buf argument as rxbuf_ptr, but it will use a
>    rxbufsize argument which has been raised to at least
>    GEM_DMACFG_RBUFSZ_MUL, even if the input size argument
>    is smaller, so even if get_queue_from_screen() honoured
>    its rxbufsize argument it wouldn't help
>
> Would somebody who understands the device like to have a look ?
>
>
Yes, I agree that it looks strange. The padding to minimum 60B seems wrong
since we're blindly extending buf from something less than 60B to 60B
and then potentially copying from it...

Cheers,
Edgar


> This is a separate issue from the incorrect array offset
> argument this patch fixes, though.
>
> thanks
> -- PMM
>

OK，I will send another patch for the missing logic for the DISABLE_MASK bit;


发件人: Edgar E. Iglesias <edgar.iglesias@gmail.com>
发送时间: 2024年6月6日 19:04
收件人: andrew Yuan <andrew.yuan@jaguarmicro.com>
抄送: luc.michel@amd.com; alistair@alistair23.me; peter.maydell@linaro.org; jasowang@redhat.com; qemu-arm@nongnu.org; qemu-devel@nongnu.org
主题: Re: [PATCH] hw/net: cadence_gem: fix: type2_compare_x_word_0 error

External Mail: This email originated from OUTSIDE of the organization!
Do not click links, open attachments or provide ANY information unless you recognize the sender and know the content is safe.

On Thu, Jun 6, 2024 at 12:00 PM Andrew.Yuan <andrew.yuan@jaguarmicro.com<mailto:andrew.yuan@jaguarmicro.com>> wrote:
        In the Cadence IP for Gigabit Ethernet MAC Part Number: IP7014 IP Rev: R1p12 - Doc Rev: 1.3 User Guide, the specification for the type2_compare_x_word_0 register is as follows:
        The byte stored in bits [23:16] is compared against the byte in the received frame from the selected offset+0, and the byte stored in bits [31:24] is compared against the byte in
        the received frame from the selected offset+1.

        However, there is an implementation error in the cadence_gem model in qemu：
        the byte stored in bits [31:24] is compared against the byte in the received frame from the selected offset+0

        Now, the error code is as follows:
        rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];

        and needs to be corrected to：
        rx_cmp = rxbuf_ptr[offset + 1] << 8 | rxbuf_ptr[offset];

Signed-off-by: Andrew.Yuan <andrew.yuan@jaguarmicro.com<mailto:andrew.yuan@jaguarmicro.com>>


LGTM:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com<mailto:edgar.iglesias@amd.com>>

At some point it would be nice to add the missing logic for the DISABLE_MASK bit that
extends the compare range from 16 to 32-bits.

Cheers,
Edgar


---
 hw/net/cadence_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index ec7bf562e5..9c73ded0d3 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -946,7 +946,7 @@ static int get_queue_from_screen(CadenceGEMState *s, uint8_t *rxbuf_ptr,
                 break;
             }

-            rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];
+            rx_cmp = rxbuf_ptr[offset + 1] << 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);

--
2.37.0.windows.1