Re: [PATCH v1 18/27] xen/riscv: add vaplic access check

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/16/26 3:01 PM, Jan Beulich wrote:
> On 14.04.2026 13:45, Oleksii Kurochko wrote:
> > On 4/2/26 3:10 PM, Jan Beulich wrote:
> > > On 10.03.2026 18:08, Oleksii Kurochko wrote:
> > > > --- a/xen/arch/riscv/aplic.c
> > > > +++ b/xen/arch/riscv/aplic.c
> > > > @@ -38,6 +38,7 @@ static struct aplic_priv aplic = {
> > > >    
> > > >    static struct intc_info __ro_after_init aplic_info = {
> > > >        .hw_version = INTC_APLIC,
> > > > +    .private = &aplic,
> > > Isn't this the host instance again? How can you ...
> > >
> > > > --- a/xen/arch/riscv/vaplic.c
> > > > +++ b/xen/arch/riscv/vaplic.c
> > > > @@ -127,6 +127,20 @@ int vaplic_map_device_irqs_to_domain(struct domain *d,
> > > >        return 0;
> > > >    }
> > > >    
> > > > +static int cf_check vaplic_is_access(const struct vcpu *vcpu,
> > > > +                                     const unsigned long addr)
> > > > +{
> > > > +    const struct vaplic *vaplic = to_vaplic(vcpu->domain->arch.vintc);
> > > > +    const struct aplic_priv *priv = vaplic->base.info->private;
> > > > +    const paddr_t paddr_end = priv->paddr_start + priv->size;
> > > > +
> > > > +    /* check if it is an APLIC access */
> > > > +    if ( priv->paddr_start <= addr && addr < paddr_end )
> > > ... use that here? Or asked differently, again: Where's the virtualization,
> > > i.e. the abstraction away from host properties?
> > With the current use case it was easier to choose such approach then
> > provide the full abstraction.
> >
> > > Furthermore, is it really sufficient to check just the starting address of
> > > an access? Shouldn't the last byte accessed also fall into the range in
> > > question?
> > I think that it is okay, my understanding is that *paddr_end technically
> > is another range.
> Of course it is. But a multi-byte access crossing the paddr_end boundary
> isn't purely an APLIC one. You can reject such for simplicity, but I'm
> unconvinced that you can claim you will be able to correctly handle it
> without proper merging.
Lets say guest has the following description of vAPLIC in its DTB:
 aplic@d000000 {
   phandle = <0x06>;
   riscv,num-sources = <0x60>;
   reg = <0x00 0xd000000 0x00 0x8000>;
   ...
 }
What means vAPLIC's MMIO range is [0xd000000, 0xD007FFF]. If some is 
trying to access 0xd008000 it is not an MMIO address which belongs to 
vAPLIC so vaplic_is_access() should return 0.
IIUC, you concern is that if someone will try to access 0xD007FFF which 
from this function point of view is legal. I think it is okay to return 
here 1 what tells that this address is from our vAPLIC range as it will 
be rejected that on vaplic_emulate_{load,store}() side as addr (more 
accurate offset got from addr) should be properly aligned:
    const unsigned int offset = addr & APLIC_REG_OFFSET_MASK;
    ...
    if ( offset & 3 )
    {
        gdprintk(XENLOG_WARNING, "Misaligned APLIC access at offset %#x\n",
                 offset);
        return -EINVAL;
    }

Is it okay? Actually I think we could add ( addr & 3 ) check in 
vaplic_is_access() function too...
~ Oleksii