[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()

Posted by Huang Borong 1 year ago

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()

Posted by Andrew Jones 1 year ago

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

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

Posted by Daniel Henrique Barboza 1 year ago


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;