1. Patchew
  2. QEMU
  3. [PATCH v2 00/16] hw/intc/loongarch_pch: Cleanup with memory region ops
  4. View patch

[PATCH v2 07/16] hw/intc/loongarch_pch: Use generic read callback for iomem32_low region

Bibo Mao posted 16 patches 10 months, 3 weeks ago
Only 15 patches received!
There is a newer version of this series
[PATCH v2 07/16] hw/intc/loongarch_pch: Use generic read callback for iomem32_low region
Posted by Bibo Mao 10 months, 3 weeks ago 
For memory region iomem32_low, generic read callback is used.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 hw/intc/loongarch_pch_pic.c | 71 +++++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 23 deletions(-)

diff --git a/hw/intc/loongarch_pch_pic.c b/hw/intc/loongarch_pch_pic.c
index 10b4231464..b495bd3a4d 100644
--- a/hw/intc/loongarch_pch_pic.c
+++ b/hw/intc/loongarch_pch_pic.c
@@ -7,6 +7,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/bitops.h"
+#include "qemu/log.h"
 #include "hw/irq.h"
 #include "hw/intc/loongarch_pch_pic.h"
 #include "trace.h"
@@ -71,47 +72,71 @@ static void pch_pic_irq_handler(void *opaque, int irq, int level)
     pch_pic_update_irq(s, mask, level);
 }
 
-static uint64_t loongarch_pch_pic_low_readw(void *opaque, hwaddr addr,
-                                            unsigned size)
+static uint64_t pch_pic_read(void *opaque, hwaddr addr, uint64_t field_mask)
 {
     LoongArchPICCommonState *s = LOONGARCH_PIC_COMMON(opaque);
     uint64_t val = 0;
+    uint32_t offset = addr & 7;
 
     switch (addr) {
-    case PCH_PIC_INT_ID:
-        val = s->id.data & UINT_MAX;
+    case PCH_PIC_INT_ID ... PCH_PIC_INT_ID + 7:
+        val = s->id.data;
         break;
-    case PCH_PIC_INT_ID + 4:
-        val = s->id.data >> 32;
+    case PCH_PIC_INT_MASK ... PCH_PIC_INT_MASK + 7:
+        val = s->int_mask;
         break;
-    case PCH_PIC_INT_MASK:
-        val = (uint32_t)s->int_mask;
+    case PCH_PIC_INT_EDGE ... PCH_PIC_INT_EDGE + 7:
+        val = s->intedge;
         break;
-    case PCH_PIC_INT_MASK + 4:
-        val = s->int_mask >> 32;
+    case PCH_PIC_HTMSI_EN ... PCH_PIC_HTMSI_EN + 7:
+        val = s->htmsi_en;
         break;
-    case PCH_PIC_INT_EDGE:
-        val = (uint32_t)s->intedge;
+    case PCH_PIC_AUTO_CTRL0 ... PCH_PIC_AUTO_CTRL0 + 7:
+    case PCH_PIC_AUTO_CTRL1 ... PCH_PIC_AUTO_CTRL1 + 7:
+        /* PCH PIC connect to EXTIOI always, discard auto_ctrl access */
         break;
-    case PCH_PIC_INT_EDGE + 4:
-        val = s->intedge >> 32;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pch_pic_read: Bad address 0x%"PRIx64"\n", addr);
         break;
-    case PCH_PIC_HTMSI_EN:
-        val = (uint32_t)s->htmsi_en;
+    }
+
+    return (val >> (offset * 8)) & field_mask;
+}
+
+static uint64_t loongarch_pch_pic_read(void *opaque, hwaddr addr,
+                                       unsigned size)
+{
+    uint64_t val = 0;
+
+    switch (size) {
+    case 1:
+        val = pch_pic_read(opaque, addr, 0xFF);
         break;
-    case PCH_PIC_HTMSI_EN + 4:
-        val = s->htmsi_en >> 32;
+    case 2:
+        val = pch_pic_read(opaque, addr, 0xFFFF);
         break;
-    case PCH_PIC_AUTO_CTRL0:
-    case PCH_PIC_AUTO_CTRL0 + 4:
-    case PCH_PIC_AUTO_CTRL1:
-    case PCH_PIC_AUTO_CTRL1 + 4:
-        /* PCH PIC connect to EXTIOI always, discard auto_ctrl access */
+    case 4:
+        val = pch_pic_read(opaque, addr, UINT_MAX);
+        break;
+    case 8:
+        val = pch_pic_read(opaque, addr, UINT64_MAX);
         break;
     default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "loongarch_pch_pic_read: Bad size %d\n", size);
         break;
     }
 
+    return val;
+}
+
+static uint64_t loongarch_pch_pic_low_readw(void *opaque, hwaddr addr,
+                                            unsigned size)
+{
+    uint64_t val;
+
+    val = loongarch_pch_pic_read(opaque, addr, size);
     trace_loongarch_pch_pic_low_readw(size, addr, val);
     return val;
 }
-- 
2.39.3
Re: [PATCH v2 07/16] hw/intc/loongarch_pch: Use generic read callback for iomem32_low region
Posted by Philippe Mathieu-Daudé 9 months, 3 weeks ago 
On 24/3/25 10:37, Bibo Mao wrote:
> For memory region iomem32_low, generic read callback is used.
> 
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>   hw/intc/loongarch_pch_pic.c | 71 +++++++++++++++++++++++++------------
>   1 file changed, 48 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/intc/loongarch_pch_pic.c b/hw/intc/loongarch_pch_pic.c
> index 10b4231464..b495bd3a4d 100644
> --- a/hw/intc/loongarch_pch_pic.c
> +++ b/hw/intc/loongarch_pch_pic.c
> @@ -7,6 +7,7 @@
>   
>   #include "qemu/osdep.h"
>   #include "qemu/bitops.h"
> +#include "qemu/log.h"
>   #include "hw/irq.h"
>   #include "hw/intc/loongarch_pch_pic.h"
>   #include "trace.h"
> @@ -71,47 +72,71 @@ static void pch_pic_irq_handler(void *opaque, int irq, int level)
>       pch_pic_update_irq(s, mask, level);
>   }
>   
> -static uint64_t loongarch_pch_pic_low_readw(void *opaque, hwaddr addr,
> -                                            unsigned size)
> +static uint64_t pch_pic_read(void *opaque, hwaddr addr, uint64_t field_mask)
>   {
>       LoongArchPICCommonState *s = LOONGARCH_PIC_COMMON(opaque);
>       uint64_t val = 0;
> +    uint32_t offset = addr & 7;
>   
>       switch (addr) {
> -    case PCH_PIC_INT_ID:
> -        val = s->id.data & UINT_MAX;
> +    case PCH_PIC_INT_ID ... PCH_PIC_INT_ID + 7:
> +        val = s->id.data;
>           break;
> -    case PCH_PIC_INT_ID + 4:
> -        val = s->id.data >> 32;
> +    case PCH_PIC_INT_MASK ... PCH_PIC_INT_MASK + 7:
> +        val = s->int_mask;
>           break;
> -    case PCH_PIC_INT_MASK:
> -        val = (uint32_t)s->int_mask;
> +    case PCH_PIC_INT_EDGE ... PCH_PIC_INT_EDGE + 7:
> +        val = s->intedge;
>           break;
> -    case PCH_PIC_INT_MASK + 4:
> -        val = s->int_mask >> 32;
> +    case PCH_PIC_HTMSI_EN ... PCH_PIC_HTMSI_EN + 7:
> +        val = s->htmsi_en;
>           break;
> -    case PCH_PIC_INT_EDGE:
> -        val = (uint32_t)s->intedge;
> +    case PCH_PIC_AUTO_CTRL0 ... PCH_PIC_AUTO_CTRL0 + 7:
> +    case PCH_PIC_AUTO_CTRL1 ... PCH_PIC_AUTO_CTRL1 + 7:
> +        /* PCH PIC connect to EXTIOI always, discard auto_ctrl access */
>           break;
> -    case PCH_PIC_INT_EDGE + 4:
> -        val = s->intedge >> 32;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "pch_pic_read: Bad address 0x%"PRIx64"\n", addr);
>           break;
> -    case PCH_PIC_HTMSI_EN:
> -        val = (uint32_t)s->htmsi_en;
> +    }
> +
> +    return (val >> (offset * 8)) & field_mask;

Maybe you want to simplify from a different angle:

--- a/hw/intc/loongarch_pch_pic.c
+++ b/hw/intc/loongarch_pch_pic.c
@@ -320,8 +320,7 @@ static const MemoryRegionOps 
loongarch_pch_pic_reg32_low_ops = {
          .max_access_size = 8,
      },
      .impl = {
-        .min_access_size = 4,
-        .max_access_size = 4,
+        .min_access_size = 8,
      },
      .endianness = DEVICE_LITTLE_ENDIAN,
  };
Re: [PATCH v2 07/16] hw/intc/loongarch_pch: Use generic read callback for iomem32_low region
Posted by bibo mao 9 months, 2 weeks ago 

On 2025/4/25 下午5:53, Philippe Mathieu-Daudé wrote:
> On 24/3/25 10:37, Bibo Mao wrote:
>> For memory region iomem32_low, generic read callback is used.
>>
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   hw/intc/loongarch_pch_pic.c | 71 +++++++++++++++++++++++++------------
>>   1 file changed, 48 insertions(+), 23 deletions(-)
>>
>> diff --git a/hw/intc/loongarch_pch_pic.c b/hw/intc/loongarch_pch_pic.c
>> index 10b4231464..b495bd3a4d 100644
>> --- a/hw/intc/loongarch_pch_pic.c
>> +++ b/hw/intc/loongarch_pch_pic.c
>> @@ -7,6 +7,7 @@
>>   #include "qemu/osdep.h"
>>   #include "qemu/bitops.h"
>> +#include "qemu/log.h"
>>   #include "hw/irq.h"
>>   #include "hw/intc/loongarch_pch_pic.h"
>>   #include "trace.h"
>> @@ -71,47 +72,71 @@ static void pch_pic_irq_handler(void *opaque, int 
>> irq, int level)
>>       pch_pic_update_irq(s, mask, level);
>>   }
>> -static uint64_t loongarch_pch_pic_low_readw(void *opaque, hwaddr addr,
>> -                                            unsigned size)
>> +static uint64_t pch_pic_read(void *opaque, hwaddr addr, uint64_t 
>> field_mask)
>>   {
>>       LoongArchPICCommonState *s = LOONGARCH_PIC_COMMON(opaque);
>>       uint64_t val = 0;
>> +    uint32_t offset = addr & 7;
>>       switch (addr) {
>> -    case PCH_PIC_INT_ID:
>> -        val = s->id.data & UINT_MAX;
>> +    case PCH_PIC_INT_ID ... PCH_PIC_INT_ID + 7:
>> +        val = s->id.data;
>>           break;
>> -    case PCH_PIC_INT_ID + 4:
>> -        val = s->id.data >> 32;
>> +    case PCH_PIC_INT_MASK ... PCH_PIC_INT_MASK + 7:
>> +        val = s->int_mask;
>>           break;
>> -    case PCH_PIC_INT_MASK:
>> -        val = (uint32_t)s->int_mask;
>> +    case PCH_PIC_INT_EDGE ... PCH_PIC_INT_EDGE + 7:
>> +        val = s->intedge;
>>           break;
>> -    case PCH_PIC_INT_MASK + 4:
>> -        val = s->int_mask >> 32;
>> +    case PCH_PIC_HTMSI_EN ... PCH_PIC_HTMSI_EN + 7:
>> +        val = s->htmsi_en;
>>           break;
>> -    case PCH_PIC_INT_EDGE:
>> -        val = (uint32_t)s->intedge;
>> +    case PCH_PIC_AUTO_CTRL0 ... PCH_PIC_AUTO_CTRL0 + 7:
>> +    case PCH_PIC_AUTO_CTRL1 ... PCH_PIC_AUTO_CTRL1 + 7:
>> +        /* PCH PIC connect to EXTIOI always, discard auto_ctrl access */
>>           break;
>> -    case PCH_PIC_INT_EDGE + 4:
>> -        val = s->intedge >> 32;
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "pch_pic_read: Bad address 0x%"PRIx64"\n", addr);
>>           break;
>> -    case PCH_PIC_HTMSI_EN:
>> -        val = (uint32_t)s->htmsi_en;
>> +    }
>> +
>> +    return (val >> (offset * 8)) & field_mask;
> 
> Maybe you want to simplify from a different angle:
> 
> --- a/hw/intc/loongarch_pch_pic.c
> +++ b/hw/intc/loongarch_pch_pic.c
> @@ -320,8 +320,7 @@ static const MemoryRegionOps 
> loongarch_pch_pic_reg32_low_ops = {
>           .max_access_size = 8,
>       },
>       .impl = {
> -        .min_access_size = 4,
> -        .max_access_size = 4,
> +        .min_access_size = 8,
>       },
>       .endianness = DEVICE_LITTLE_ENDIAN,
>   };
I do not follow this, do you mean something like this?
        .impl = {
  -        .min_access_size = 4,
  -        .max_access_size = 4,
  +        .min_access_size = 1,
  +        .max_access_size = 8,
        },

Since this driver is used by KVM, performance issue need be considered.
For normal aligned 1/2/4/8 bytes access, it had better be accessed once 
rather than concatenated with 1 byte access for many times.

Regards
Bibo Mao
Re: [PATCH v2 07/16] hw/intc/loongarch_pch: Use generic read callback for iomem32_low region
Posted by Philippe Mathieu-Daudé 9 months, 2 weeks ago 
On 27/4/25 09:09, bibo mao wrote:
> 
> 
> On 2025/4/25 下午5:53, Philippe Mathieu-Daudé wrote:
>> On 24/3/25 10:37, Bibo Mao wrote:
>>> For memory region iomem32_low, generic read callback is used.
>>>
>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>>> ---
>>>   hw/intc/loongarch_pch_pic.c | 71 +++++++++++++++++++++++++------------
>>>   1 file changed, 48 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/hw/intc/loongarch_pch_pic.c b/hw/intc/loongarch_pch_pic.c
>>> index 10b4231464..b495bd3a4d 100644
>>> --- a/hw/intc/loongarch_pch_pic.c
>>> +++ b/hw/intc/loongarch_pch_pic.c
>>> @@ -7,6 +7,7 @@
>>>   #include "qemu/osdep.h"
>>>   #include "qemu/bitops.h"
>>> +#include "qemu/log.h"
>>>   #include "hw/irq.h"
>>>   #include "hw/intc/loongarch_pch_pic.h"
>>>   #include "trace.h"
>>> @@ -71,47 +72,71 @@ static void pch_pic_irq_handler(void *opaque, int 
>>> irq, int level)
>>>       pch_pic_update_irq(s, mask, level);
>>>   }
>>> -static uint64_t loongarch_pch_pic_low_readw(void *opaque, hwaddr addr,
>>> -                                            unsigned size)
>>> +static uint64_t pch_pic_read(void *opaque, hwaddr addr, uint64_t 
>>> field_mask)
>>>   {
>>>       LoongArchPICCommonState *s = LOONGARCH_PIC_COMMON(opaque);
>>>       uint64_t val = 0;
>>> +    uint32_t offset = addr & 7;
>>>       switch (addr) {
>>> -    case PCH_PIC_INT_ID:
>>> -        val = s->id.data & UINT_MAX;
>>> +    case PCH_PIC_INT_ID ... PCH_PIC_INT_ID + 7:
>>> +        val = s->id.data;
>>>           break;
>>> -    case PCH_PIC_INT_ID + 4:
>>> -        val = s->id.data >> 32;
>>> +    case PCH_PIC_INT_MASK ... PCH_PIC_INT_MASK + 7:
>>> +        val = s->int_mask;
>>>           break;
>>> -    case PCH_PIC_INT_MASK:
>>> -        val = (uint32_t)s->int_mask;
>>> +    case PCH_PIC_INT_EDGE ... PCH_PIC_INT_EDGE + 7:
>>> +        val = s->intedge;
>>>           break;
>>> -    case PCH_PIC_INT_MASK + 4:
>>> -        val = s->int_mask >> 32;
>>> +    case PCH_PIC_HTMSI_EN ... PCH_PIC_HTMSI_EN + 7:
>>> +        val = s->htmsi_en;
>>>           break;
>>> -    case PCH_PIC_INT_EDGE:
>>> -        val = (uint32_t)s->intedge;
>>> +    case PCH_PIC_AUTO_CTRL0 ... PCH_PIC_AUTO_CTRL0 + 7:
>>> +    case PCH_PIC_AUTO_CTRL1 ... PCH_PIC_AUTO_CTRL1 + 7:
>>> +        /* PCH PIC connect to EXTIOI always, discard auto_ctrl 
>>> access */
>>>           break;
>>> -    case PCH_PIC_INT_EDGE + 4:
>>> -        val = s->intedge >> 32;
>>> +    default:
>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>> +                      "pch_pic_read: Bad address 0x%"PRIx64"\n", addr);
>>>           break;
>>> -    case PCH_PIC_HTMSI_EN:
>>> -        val = (uint32_t)s->htmsi_en;
>>> +    }
>>> +
>>> +    return (val >> (offset * 8)) & field_mask;
>>
>> Maybe you want to simplify from a different angle:
>>
>> --- a/hw/intc/loongarch_pch_pic.c
>> +++ b/hw/intc/loongarch_pch_pic.c
>> @@ -320,8 +320,7 @@ static const MemoryRegionOps 
>> loongarch_pch_pic_reg32_low_ops = {
>>           .max_access_size = 8,
>>       },
>>       .impl = {
>> -        .min_access_size = 4,
>> -        .max_access_size = 4,
>> +        .min_access_size = 8,
>>       },
>>       .endianness = DEVICE_LITTLE_ENDIAN,
>>   };
> I do not follow this, do you mean something like this?
>         .impl = {
>   -        .min_access_size = 4,
>   -        .max_access_size = 4,
>   +        .min_access_size = 1,
>   +        .max_access_size = 8,
>         },
> 
> Since this driver is used by KVM, performance issue need be considered.

We are in slow path anyway, I doubt performance is measurable, you are
doing the same maths manually before returning.
Anyway I'm not insisting.

> For normal aligned 1/2/4/8 bytes access, it had better be accessed once 
> rather than concatenated with 1 byte access for many times.
> 
> Regards
> Bibo Mao
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.