Re: [PATCH 2/7] hw/intc: sifive_plic: Cleanup the write function

[Richard Henderson]

On 12/7/21 10:42 PM, Alistair Francis wrote:

From: Alistair Francis 

Signed-off-by: Alistair Francis 
Reviewed-by: Bin Meng 
---
  hw/intc/sifive_plic.c | 82 +--
  1 file changed, 33 insertions(+), 49 deletions(-)

diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index 35f097799a..c1fa689868 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -33,6 +33,17 @@
  
  #define RISCV_DEBUG_PLIC 0
  
+static bool addr_between(uint32_t addr, uint32_t base, uint32_t num)

+{
+uint32_t end = base + num;
+
+if (addr >= base && addr < end) {
+return true;
+}
+
+return false;
+}


It may well not matter for your use case, but this will fail for addresses at the end of 
the range.  Better as


return addr >= base && addr - base < num;


r~

[PATCH 2/7] hw/intc: sifive_plic: Cleanup the write function

[Alistair Francis]

From: Alistair Francis 

Signed-off-by: Alistair Francis 
Reviewed-by: Bin Meng 
---
 hw/intc/sifive_plic.c | 82 +--
 1 file changed, 33 insertions(+), 49 deletions(-)

diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index 35f097799a..c1fa689868 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -33,6 +33,17 @@
 
 #define RISCV_DEBUG_PLIC 0
 
+static bool addr_between(uint32_t addr, uint32_t base, uint32_t num)
+{
+uint32_t end = base + num;
+
+if (addr >= base && addr < end) {
+return true;
+}
+
+return false;
+}
+
 static PLICMode char_to_mode(char c)
 {
 switch (c) {
@@ -269,80 +280,53 @@ static void sifive_plic_write(void *opaque, hwaddr addr, 
uint64_t value,
 {
 SiFivePLICState *plic = opaque;
 
-/* writes must be 4 byte words */
-if ((addr & 0x3) != 0) {
-goto err;
-}
-
-if (addr >= plic->priority_base && /* 4 bytes per source */
-addr < plic->priority_base + (plic->num_sources << 2))
-{
+if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) {
 uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
+
 plic->source_priority[irq] = value & 7;
-if (RISCV_DEBUG_PLIC) {
-qemu_log("plic: write priority: irq=%d priority=%d\n",
-irq, plic->source_priority[irq]);
-}
 sifive_plic_update(plic);
-return;
-} else if (addr >= plic->pending_base && /* 1 bit per source */
-   addr < plic->pending_base + (plic->num_sources >> 3))
-{
+} else if (addr_between(addr, plic->pending_base,
+plic->num_sources >> 3)) {
 qemu_log_mask(LOG_GUEST_ERROR,
   "%s: invalid pending write: 0x%" HWADDR_PRIx "",
   __func__, addr);
-return;
-} else if (addr >= plic->enable_base && /* 1 bit per source */
-addr < plic->enable_base + plic->num_addrs * plic->enable_stride)
-{
+} else if (addr_between(addr, plic->enable_base,
+plic->num_addrs * plic->enable_stride)) {
 uint32_t addrid = (addr - plic->enable_base) / plic->enable_stride;
 uint32_t wordid = (addr & (plic->enable_stride - 1)) >> 2;
+
 if (wordid < plic->bitfield_words) {
 plic->enable[addrid * plic->bitfield_words + wordid] = value;
-if (RISCV_DEBUG_PLIC) {
-qemu_log("plic: write enable: hart%d-%c word=%d value=%x\n",
-plic->addr_config[addrid].hartid,
-mode_to_char(plic->addr_config[addrid].mode), wordid,
-plic->enable[addrid * plic->bitfield_words + wordid]);
-}
-return;
+} else {
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: Invalid enable write 0x%" HWADDR_PRIx "\n",
+  __func__, addr);
 }
-} else if (addr >= plic->context_base && /* 4 bytes per reg */
-addr < plic->context_base + plic->num_addrs * plic->context_stride)
-{
+} else if (addr_between(addr, plic->context_base,
+plic->num_addrs * plic->context_stride)) {
 uint32_t addrid = (addr - plic->context_base) / plic->context_stride;
 uint32_t contextid = (addr & (plic->context_stride - 1));
+
 if (contextid == 0) {
-if (RISCV_DEBUG_PLIC) {
-qemu_log("plic: write priority: hart%d-%c priority=%x\n",
-plic->addr_config[addrid].hartid,
-mode_to_char(plic->addr_config[addrid].mode),
-plic->target_priority[addrid]);
-}
 if (value <= plic->num_priorities) {
 plic->target_priority[addrid] = value;
 sifive_plic_update(plic);
 }
-return;
 } else if (contextid == 4) {
-if (RISCV_DEBUG_PLIC) {
-qemu_log("plic: write claim: hart%d-%c irq=%x\n",
-plic->addr_config[addrid].hartid,
-mode_to_char(plic->addr_config[addrid].mode),
-(uint32_t)value);
-}
 if (value < plic->num_sources) {
 sifive_plic_set_claimed(plic, value, false);
 sifive_plic_update(plic);
 }
-return;
+} else {
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: Invalid context write 0x%" HWADDR_PRIx "\n",
+  __func__, addr);
 }
+} else {
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: Invalid register write 0x%" HWADDR_PRIx "\n",
+  __func__, addr);
 }
-
-err:
-qemu_log_mask(LOG_GUEST_ERROR,
-  "%s: Invalid register write 0x%" HWADDR_PRIx "\n",
-  __func__, addr);
 }
 
 static const MemoryRegionOps sifive_plic_ops = {
--