Re: [PATCH v1] hw/intc/riscv_aplic: Remove redundant masking of hart_idx in riscv_aplic_msi_send()

[Daniel Henrique Barboza]

On 1/13/25 11:53 PM, Huang Borong wrote:

The line "hart_idx &= APLIC_xMSICFGADDR_PPN_LHX_MASK(lhxw);" was removed
because the same operation is performed later in the address calculation.
This change improves code clarity and avoids unnecessary operations.

Signed-off-by: Huang Borong <huangbor...@bosc.ac.cn>
---
  hw/intc/riscv_aplic.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/hw/intc/riscv_aplic.c b/hw/intc/riscv_aplic.c
index 4866649115..0974c6a5db 100644
--- a/hw/intc/riscv_aplic.c
+++ b/hw/intc/riscv_aplic.c
@@ -421,7 +421,6 @@ static void riscv_aplic_msi_send(RISCVAPLICState *aplic,
              APLIC_xMSICFGADDRH_HHXW_MASK;

group_idx = hart_idx >> lhxw;

-    hart_idx &= APLIC_xMSICFGADDR_PPN_LHX_MASK(lhxw);

It is worth noticing that this will change 'hart_idx' in the qemu_log_mask() in 
the
end:

    if (result != MEMTX_OK) {
        qemu_log_mask(LOG_GUEST_ERROR, "%s: MSI write failed for "
                      "hart_index=%d guest_index=%d eiid=%d\n",
                      __func__, hart_idx, guest_idx, eiid);
    }


But I believe 'hart_idx' in that context should be the original 'hart_idx' 
parameter,
not the masked value like we're doing today.


Reviewed-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com>

addr = msicfgaddr;

      addr |= ((uint64_t)(msicfgaddrH & APLIC_xMSICFGADDRH_BAPPN_MASK)) << 32;