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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/16/26 11:24 AM, Jan Beulich wrote:
> 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.
Thanks for that. I will then just apply your suggestion.

> > > > --- 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.
Then 'unsigned int' should be more then enough.

> > > > +    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?
Agree, ASSERT() isn't the best option here. Either rejecting of such 
length should happen or domain_crash() instead. The first one option I 
think is more preferrable.
~ Oleksii