On Fri, 2 Feb 2024 at 15:36, Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> From: Nabih Estefan Diaz <nabiheste...@google.com>
>
> - Implementation of Transmit function for packets
> - Implementation for reading and writing from and to descriptors in
> memory for Tx
>
> Added relevant trace-events
>
> NOTE: This function implements the steps detailed in the datasheet for
> transmitting messages from the GMAC.
>
> Change-Id: Icf14f9fcc6cc7808a41acd872bca67c9832087e6
> Signed-off-by: Nabih Estefan <nabiheste...@google.com>
> Reviewed-by: Tyrone Ting <kft...@nuvoton.com>
> Message-id: 20240131002800.989285-6-nabiheste...@google.com
> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
> ---
Hi; Coverity points out an issue in this code (CID 1534027):
> +static void gmac_try_send_next_packet(NPCMGMACState *gmac)
> +{
> + /*
> + * Comments about steps refer to steps for
> + * transmitting in page 384 of datasheet
> + */
> + uint16_t tx_buffer_size = 2048;
> + g_autofree uint8_t *tx_send_buffer = g_malloc(tx_buffer_size);
> + uint32_t desc_addr;
> + struct NPCMGMACTxDesc tx_desc;
> + uint32_t tx_buf_addr, tx_buf_len;
> + uint16_t length = 0;
> + uint8_t *buf = tx_send_buffer;
> + uint32_t prev_buf_size = 0;
> + int csum = 0;
> +
> + /* steps 1&2 */
> + if (!gmac->regs[R_NPCM_DMA_HOST_TX_DESC]) {
> + gmac->regs[R_NPCM_DMA_HOST_TX_DESC] =
> + NPCM_DMA_HOST_TX_DESC_MASK(gmac->regs[R_NPCM_DMA_TX_BASE_ADDR]);
> + }
> + desc_addr = gmac->regs[R_NPCM_DMA_HOST_TX_DESC];
> +
> + while (true) {
In this loop...
> + gmac_dma_set_state(gmac, NPCM_DMA_STATUS_TX_PROCESS_STATE_SHIFT,
> + NPCM_DMA_STATUS_TX_RUNNING_FETCHING_STATE);
> + if (gmac_read_tx_desc(desc_addr, &tx_desc)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "TX Descriptor @ 0x%x can't be read\n",
> + desc_addr);
> + return;
> + }
> + /* step 3 */
> +
> + trace_npcm_gmac_packet_desc_read(DEVICE(gmac)->canonical_path,
> + desc_addr);
> + trace_npcm_gmac_debug_desc_data(DEVICE(gmac)->canonical_path,
> &tx_desc,
> + tx_desc.tdes0, tx_desc.tdes1, tx_desc.tdes2, tx_desc.tdes3);
> +
> + /* 1 = DMA Owned, 0 = Software Owned */
> + if (!(tx_desc.tdes0 & TX_DESC_TDES0_OWN)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "TX Descriptor @ 0x%x is owned by software\n",
> + desc_addr);
> + gmac->regs[R_NPCM_DMA_STATUS] |= NPCM_DMA_STATUS_TU;
> + gmac_dma_set_state(gmac, NPCM_DMA_STATUS_TX_PROCESS_STATE_SHIFT,
> + NPCM_DMA_STATUS_TX_SUSPENDED_STATE);
> + gmac_update_irq(gmac);
> + return;
> + }
> +
> + gmac_dma_set_state(gmac, NPCM_DMA_STATUS_TX_PROCESS_STATE_SHIFT,
> + NPCM_DMA_STATUS_TX_RUNNING_READ_STATE);
> + /* Give the descriptor back regardless of what happens. */
> + tx_desc.tdes0 &= ~TX_DESC_TDES0_OWN;
> +
> + if (tx_desc.tdes1 & TX_DESC_TDES1_FIRST_SEG_MASK) {
> + csum = gmac_tx_get_csum(tx_desc.tdes1);
> + }
> +
> + /* step 4 */
> + tx_buf_addr = tx_desc.tdes2;
> + gmac->regs[R_NPCM_DMA_CUR_TX_BUF_ADDR] = tx_buf_addr;
> + tx_buf_len = TX_DESC_TDES1_BFFR1_SZ_MASK(tx_desc.tdes1);
> + buf = &tx_send_buffer[prev_buf_size];
...we never use 'buf' before we initialize it down here in step 4...
> +
> + if ((prev_buf_size + tx_buf_len) > sizeof(buf)) {
> + tx_buffer_size = prev_buf_size + tx_buf_len;
> + tx_send_buffer = g_realloc(tx_send_buffer, tx_buffer_size);
> + buf = &tx_send_buffer[prev_buf_size];
> + }
> +
> + /* step 5 */
> + if (dma_memory_read(&address_space_memory, tx_buf_addr, buf,
> + tx_buf_len, MEMTXATTRS_UNSPECIFIED)) {
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: Failed to read packet @
> 0x%x\n",
> + __func__, tx_buf_addr);
> + return;
> + }
> + length += tx_buf_len;
> + prev_buf_size += tx_buf_len;
> +
> + /* If not chained we'll have a second buffer. */
> + if (!(tx_desc.tdes1 & TX_DESC_TDES1_SEC_ADDR_CHND_MASK)) {
> + tx_buf_addr = tx_desc.tdes3;
> + gmac->regs[R_NPCM_DMA_CUR_TX_BUF_ADDR] = tx_buf_addr;
> + tx_buf_len = TX_DESC_TDES1_BFFR2_SZ_MASK(tx_desc.tdes1);
> + buf = &tx_send_buffer[prev_buf_size];
> +
> + if ((prev_buf_size + tx_buf_len) > sizeof(buf)) {
> + tx_buffer_size = prev_buf_size + tx_buf_len;
> + tx_send_buffer = g_realloc(tx_send_buffer, tx_buffer_size);
> + buf = &tx_send_buffer[prev_buf_size];
> + }
> +
> + if (dma_memory_read(&address_space_memory, tx_buf_addr, buf,
> + tx_buf_len, MEMTXATTRS_UNSPECIFIED)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Failed to read packet @ 0x%x\n",
> + __func__, tx_buf_addr);
> + return;
> + }
> + length += tx_buf_len;
> + prev_buf_size += tx_buf_len;
> + }
> + if (tx_desc.tdes1 & TX_DESC_TDES1_LAST_SEG_MASK) {
> + net_checksum_calculate(tx_send_buffer, length, csum);
> + qemu_send_packet(qemu_get_queue(gmac->nic), tx_send_buffer,
> length);
> + trace_npcm_gmac_packet_sent(DEVICE(gmac)->canonical_path,
> length);
> + buf = tx_send_buffer;
> + length = 0;
...and in this bit of code, we set 'buf' to something else...
> + }
> +
> + /* step 6 */
> + gmac_dma_set_state(gmac, NPCM_DMA_STATUS_TX_PROCESS_STATE_SHIFT,
> + NPCM_DMA_STATUS_TX_RUNNING_CLOSING_STATE);
> + gmac_write_tx_desc(desc_addr, &tx_desc);
> + if (tx_desc.tdes1 & TX_DESC_TDES1_TX_END_RING_MASK) {
> + desc_addr = gmac->regs[R_NPCM_DMA_TX_BASE_ADDR];
> + } else if (tx_desc.tdes1 & TX_DESC_TDES1_SEC_ADDR_CHND_MASK) {
> + desc_addr = tx_desc.tdes3;
> + } else {
> + desc_addr += sizeof(tx_desc);
> + }
> + gmac->regs[R_NPCM_DMA_HOST_TX_DESC] = desc_addr;
> +
> + /* step 7 */
> + if (tx_desc.tdes1 & TX_DESC_TDES1_INTERR_COMP_MASK) {
> + gmac->regs[R_NPCM_DMA_STATUS] |= NPCM_DMA_STATUS_TI;
> + gmac_update_irq(gmac);
> + }
...but we never use 'buf' in the rest of the loop before we go back
around to the top again, so that value that we set will never be
used before it is overwritten with something else in the next
iteration's step 4.
What was the intention here?
> + }
> +}
thanks
-- PMM
