Re: [RFC PATCH 3/3] hw/net/tulip: Set descriptor error bit when lenght is incorrect
On 4/23/20 6:16 PM, Philippe Mathieu-Daudé wrote: When a frame lenght is incorrect, set the RDES0 'Error Summary' length (here, and in the subject) and 'Frame too long' bits. Then stop the receive process and trigger an abnormal interrupt. See [4.3.5 Receive Process]. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [RFC PATCH 3/3] hw/net/tulip: Set descriptor error bit when lenght is incorrect
On 24.04.20 04:16, Jason Wang wrote:
>
> On 2020/4/24 上午7:16, Philippe Mathieu-Daudé wrote:
>> When a frame lenght is incorrect, set the RDES0 'Error Summary'
>> and 'Frame too long' bits. Then stop the receive process and
>> trigger an abnormal interrupt. See [4.3.5 Receive Process].
>>
>> Cc: Li Qiang
>> Cc: Li Qiang
>> Cc: Ziming Zhang
>> Cc: Jason Wang
>> Cc: Prasad J Pandit
>> Fixes: 8ffb7265af ("check frame size and r/w data length")
>> Buglink: https://bugs.launchpad.net/bugs/1874539
>> Reported-by: Helge Deller
>> Signed-off-by: Philippe Mathieu-Daudé
>
>
> Hi Philippe:
>
> It's still unclear to me that how this fixes the stuck. Did you mean guest
> trigger the error condition and then recvoer by abnormal interrupt?
>
> If yes, this sounds still like a bug somewhere in the code.
Sadly the patches do not fix the bug.
Helge
>
>> ---
>> hw/net/tulip.c | 16
>> 1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/net/tulip.c b/hw/net/tulip.c
>> index 470f635acb..671f79b6f4 100644
>> --- a/hw/net/tulip.c
>> +++ b/hw/net/tulip.c
>> @@ -158,7 +158,7 @@ static void tulip_next_rx_descriptor(TULIPState *s,
>> s->current_rx_desc &= ~3ULL;
>> }
>> -static void tulip_copy_rx_bytes(TULIPState *s, struct tulip_descriptor
>> *desc)
>> +static int tulip_copy_rx_bytes(TULIPState *s, struct tulip_descriptor *desc)
>> {
>> int len1 = (desc->control >> RDES1_BUF1_SIZE_SHIFT) &
>> RDES1_BUF1_SIZE_MASK;
>> int len2 = (desc->control >> RDES1_BUF2_SIZE_SHIFT) &
>> RDES1_BUF2_SIZE_MASK;
>> @@ -177,7 +177,8 @@ static void tulip_copy_rx_bytes(TULIPState *s, struct
>> tulip_descriptor *desc)
>> "(ofs: %u, len:%d, size:%zu)\n",
>> __func__, s->rx_frame_len, len,
>> sizeof(s->rx_frame));
>> - return;
>> + s->rx_frame_len = 0;
>> + return -1;
>> }
>> pci_dma_write(&s->dev, desc->buf_addr1, s->rx_frame +
>> (s->rx_frame_size - s->rx_frame_len), len);
>> @@ -197,12 +198,15 @@ static void tulip_copy_rx_bytes(TULIPState *s, struct
>> tulip_descriptor *desc)
>> "(ofs: %u, len:%d, size:%zu)\n",
>> __func__, s->rx_frame_len, len,
>> sizeof(s->rx_frame));
>> - return;
>> + s->rx_frame_len = 0;
>> + return -1;
>> }
>> pci_dma_write(&s->dev, desc->buf_addr2, s->rx_frame +
>> (s->rx_frame_size - s->rx_frame_len), len);
>> s->rx_frame_len -= len;
>> }
>> +
>> + return 0;
>> }
>> static bool tulip_filter_address(TULIPState *s, const uint8_t *addr)
>> @@ -274,7 +278,11 @@ static ssize_t tulip_receive(TULIPState *s, const
>> uint8_t *buf, size_t size)
>> s->rx_frame_len = s->rx_frame_size;
>> }
>> - tulip_copy_rx_bytes(s, &desc);
>> + if (tulip_copy_rx_bytes(s, &desc)) {
>> + desc.status |= RDES0_ES | RDES0_TL; /* Error: frame too long */
>> + s->csr[5] |= CSR5_RPS; /* Receive process stopped */
>> + tulip_update_int(s);
>> + }
>> if (!s->rx_frame_len) {
>> desc.status |= s->rx_status;
>
Re: [RFC PATCH 3/3] hw/net/tulip: Set descriptor error bit when lenght is incorrect
On 2020/4/24 上午7:16, Philippe Mathieu-Daudé wrote:
When a frame lenght is incorrect, set the RDES0 'Error Summary'
and 'Frame too long' bits. Then stop the receive process and
trigger an abnormal interrupt. See [4.3.5 Receive Process].
Cc: Li Qiang
Cc: Li Qiang
Cc: Ziming Zhang
Cc: Jason Wang
Cc: Prasad J Pandit
Fixes: 8ffb7265af ("check frame size and r/w data length")
Buglink: https://bugs.launchpad.net/bugs/1874539
Reported-by: Helge Deller
Signed-off-by: Philippe Mathieu-Daudé
Hi Philippe:
It's still unclear to me that how this fixes the stuck. Did you mean
guest trigger the error condition and then recvoer by abnormal interrupt?
If yes, this sounds still like a bug somewhere in the code.
Thanks
---
hw/net/tulip.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/hw/net/tulip.c b/hw/net/tulip.c
index 470f635acb..671f79b6f4 100644
--- a/hw/net/tulip.c
+++ b/hw/net/tulip.c
@@ -158,7 +158,7 @@ static void tulip_next_rx_descriptor(TULIPState *s,
s->current_rx_desc &= ~3ULL;
}
-static void tulip_copy_rx_bytes(TULIPState *s, struct tulip_descriptor *desc)
+static int tulip_copy_rx_bytes(TULIPState *s, struct tulip_descriptor *desc)
{
int len1 = (desc->control >> RDES1_BUF1_SIZE_SHIFT) &
RDES1_BUF1_SIZE_MASK;
int len2 = (desc->control >> RDES1_BUF2_SIZE_SHIFT) &
RDES1_BUF2_SIZE_MASK;
@@ -177,7 +177,8 @@ static void tulip_copy_rx_bytes(TULIPState *s, struct
tulip_descriptor *desc)
"(ofs: %u, len:%d, size:%zu)\n",
__func__, s->rx_frame_len, len,
sizeof(s->rx_frame));
-return;
+s->rx_frame_len = 0;
+return -1;
}
pci_dma_write(&s->dev, desc->buf_addr1, s->rx_frame +
(s->rx_frame_size - s->rx_frame_len), len);
@@ -197,12 +198,15 @@ static void tulip_copy_rx_bytes(TULIPState *s, struct
tulip_descriptor *desc)
"(ofs: %u, len:%d, size:%zu)\n",
__func__, s->rx_frame_len, len,
sizeof(s->rx_frame));
-return;
+s->rx_frame_len = 0;
+return -1;
}
pci_dma_write(&s->dev, desc->buf_addr2, s->rx_frame +
(s->rx_frame_size - s->rx_frame_len), len);
s->rx_frame_len -= len;
}
+
+return 0;
}
static bool tulip_filter_address(TULIPState *s, const uint8_t *addr)
@@ -274,7 +278,11 @@ static ssize_t tulip_receive(TULIPState *s, const uint8_t
*buf, size_t size)
s->rx_frame_len = s->rx_frame_size;
}
-tulip_copy_rx_bytes(s, &desc);
+if (tulip_copy_rx_bytes(s, &desc)) {
+desc.status |= RDES0_ES | RDES0_TL; /* Error: frame too long */
+s->csr[5] |= CSR5_RPS; /* Receive process stopped */
+tulip_update_int(s);
+}
if (!s->rx_frame_len) {
desc.status |= s->rx_status;
