| Message ID | 20250114025320.52696-1-huangborong@bosc.ac.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] hw/intc/riscv_aplic: Remove redundant masking of hart_idx in riscv_aplic_msi_send() | expand |
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;
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 > >
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;
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(-)