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

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

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 <huangborong@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 <dbarboza@ventanamicro.com>


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