Re: [PATCH v2 21/26] xen/riscv: implement virtual APLIC MMIO emulation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.06.2026 11:07, Oleksii Kurochko wrote:
> On 6/15/26 5:13 PM, Jan Beulich wrote:
>> On 08.05.2026 16:43, Oleksii Kurochko wrote:
>>> +    spin_lock_irqsave(&aplic.lock, flags);
>>> +    val = readl((void __iomem *)((uintptr_t)aplic.regs + offset)) & mask;
>>
>> Easier as
>>
>>      val = readl((volatile void __iomem *)aplic.regs + offset) & mask;
>>
>> ? (Note that like const, volatile also shouldn't be cast away.)
> 
> Is arithmetic on void * pointers correct from the C standard's point of 
> view?
> 
> It works with GCC (see 
> https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html), but I can't find 
> anything that guarantees the same behavior for other compilers.
> 
> I'm okay with the suggested change if it's acceptable for Xen to rely on 
> GCC's void * pointer arithmetic extension.

We use that all over the place. Even docs/misra/C-language-toolchain.rst
mentions it as explicitly permitted.

>>> --- a/xen/arch/riscv/include/asm/vaplic.h
>>> +++ b/xen/arch/riscv/include/asm/vaplic.h
>>> @@ -26,6 +26,9 @@ struct vaplic_regs {
>>>   struct vaplic {
>>>       struct vintc vintc;
>>>       struct vaplic_regs regs;
>>> +
>>> +    paddr_t regs_start;
>>> +    paddr_t regs_size;
>>
>> Can regs_size really go beyond 4G?
> 
> Good question and it depends on an amount of vCPUs:
> 
> #define APLIC_MIN_SIZE          0x4000
> #define APLIC_SIZE_ALIGN(x)     ROUNDUP(x, APLIC_MIN_SIZE)
> 
> #define APLIC_SIZE(nr_cpus)     (APLIC_MIN_SIZE + \
>                                   APLIC_SIZE_ALIGN(APLIC_IDC_SIZE * 
> (nr_cpus)))
> 
> paddr_t aplic_size = APLIC_SIZE(d->max_vcpus);
> 
> With the current limitation of 128 vCPUs max (IIRC) it won't beyond 4G.

Tying to the overly low limit of 128 isn't very helpful, I guess. With
APLIC_IDC_SIZE resolving to 32, the limit would be millions of vCPU-s
aiui, so imo not a concern at all.

>>> --- a/xen/arch/riscv/vaplic.c
>>> +++ b/xen/arch/riscv/vaplic.c
>>> @@ -26,6 +26,283 @@
>>>   
>>>   #define FDT_VAPLIC_INT_CELLS 2
>>>   
>>> +#define AUTH_IRQ_BIT(d, irqn) ( \
>>> +    ((irqn) <= (d)->arch.vintc->irq_nums) && \
>>> +    test_bit(irqn, (d)->arch.vintc->allocated_irqs) )
>>> +
>>> +#define regindx_to_irqn(reg_val) ((reg_val) / sizeof(uint32_t))
>>> +
>>> +static inline uint32_t generate_auth_mask(const struct domain *d,
>>> +                                          unsigned int irqsn)
>>> +{
>>> +    if ( irqsn >= DIV_ROUND_UP(d->arch.vintc->irq_nums,
>>> +                               sizeof(uint32_t) * BITS_PER_BYTE) )
>>
>> Why the rounding up? Isn't ->irqs_num the proper upper bound?
> 
> Probably irqsn isn't a correct name here as it looks like "IRQ source 
> number" (an IRQ number), but regindx_to_irqn(offset & MASK) actually 
> computes offset / 4 - a word index into the used_irqs bitmap. Word 0 
> covers IRQs 0–31, word 1 covers IRQs 32–63, etc.
> Given that, the bounds check:
>    irqsn >= DIV_ROUND_UP(irq_nums, sizeof(uint32_t) * BITS_PER_BYTE)
> is "word index >= number of 32-bit words in the bitmap" which is 
> correct. The DIV_ROUND_UP converts the IRQ count into a word count to 
> compare against the word index.
> 
> So the real issue is naming. There are two options to resolve it:
> - rename to reflect reality; call it word_idx (not irqsn), and rename 
> regindx_to_irqn to something like regoffset_to_word_idx. The 
> DIV_ROUND_UP check then reads clearly.
> 
> - Change the API to take an actual IRQ number, pass irqsn * 32 (the 
> first IRQ in the word) and check irqsn >= irq_nums directly, computing 
> the word index inside generate_auth_mask. This aligns with how 
> AUTH_IRQ_BIT works.
> 
> Which one option do you prefer?

I don't care very much as long as the result is self-consistent.

>>> +    default:
>>> +        gdprintk(XENLOG_WARNING, "Unhandled APLIC read at offset %#x\n",
>>> +                 offset);
>>> +
>>> +        domain_crash(vcpu->domain);
>>> +
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    *out = aplic_hw_read_reg(offset, auth_mask);
>>
>> You blindly assume a 32-bit access here (and also in the write counterpart).
>> How do you end up knowing?
> 
> he APLIC spec requires all register accesses to be 32-bit wide.
> 
> Also, I have the following at the caller side (yes, it can't be 
> understand from the current patch):
> 
>      /* Fault address should be aligned to length of MMIO */
>      if ( fault_addr & (len - 1) )
>          return -EIO;
> 
>      if ( vintc->ops->is_access(vcpu, fault_addr) )
>      {
>          /* PLIC/APLIC access are always on 32bit */
>          ASSERT( len == 4 );

"len" being guest controlled, how can you have such an assertion?

>          rc = vintc->ops->emulate_store(vcpu, fault_addr, data32);
>          if ( rc < 0 )
>              return rc;
>      }
> 
> Probably it would be better addi a size parameter to both callbacks:
> 
> int (*emulate_load)(const struct vcpu *vcpu, unsigned long addr,
>                      unsigned int size, uint32_t *out);
> int (*emulate_store)(const struct vcpu *vcpu, unsigned long addr,
>                       unsigned int size, uint32_t in);

Yes please.

Jan