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

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

[Huang Borong]

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);
 
     addr = msicfgaddr;
     addr |= ((uint64_t)(msicfgaddrH & APLIC_xMSICFGADDRH_BAPPN_MASK)) << 32;
-- 
2.34.1

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 <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;

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

[Andrew Jones]

Drop "in riscv_aplic_msi_send()" from the patch summary to make it more
concise.

On Tue, Jan 14, 2025 at 10:53:19AM +0800, Huang Borong wrote:
> The line "hart_idx &= APLIC_xMSICFGADDR_PPN_LHX_MASK(lhxw);" was removed

This just states what we can easily read from the patch.

> because the same operation is performed later in the address calculation.

This is useful information that should stay in the commit message.

> This change improves code clarity and avoids unnecessary operations.

You don't need to justify removing redundant lines of code, you just need
to justify that they're actually redundant.

Daniel's point about the log message is important and should be pointed
out in the commit message.

Thanks,
drew

> 
> 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);
>  
>      addr = msicfgaddr;
>      addr |= ((uint64_t)(msicfgaddrH & APLIC_xMSICFGADDRH_BAPPN_MASK)) << 32;
> -- 
> 2.34.1
> 
>