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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

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.

> 
>> +        return 1;
>> +
>> +    return 0;
>> +}
> 
> This function looks to want to return bool (and then use true/false).

Agree, then it will also need to update function pointer prototype in 
vintc_ops.

Thanks.

~ Oleksii