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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.04.2026 13:53, Oleksii Kurochko wrote:
> 
> 
> 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...

Perhaps best. The load/store functions could then simply assert that property.

Jan