Re: [PATCH v2 08/39] xen/riscv: introduce device-agnostic MMIO emulation dispatch

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

On 9/1/26 5:36 PM, Baptiste Le Duc wrote:
>> RISC-V guests can expose several virtual interrupt controllers at
>> distinct GPA ranges: vPLIC (hasn't been introduced yet) for legacy machines,
>> vAPLIC and vIMSIC for AIA-compliant ones (are being introduced in the follow
>> up patches).
> As Jan said here [1], we shouldn't use "as later in this series" in
> commit message...

I will reword this paragraph to:
```
A RISC-V guest can be given several emulated devices at distinct GPA
ranges; the virtual interrupt controllers alone account for vPLIC on
legacy machines and vAPLIC together with vIMSIC on AIA-compliant ones.
Routing MMIO faults via a per-device is_access() check in the trap
handler would couple that handler to every device it must serve,
requiring a new conditional branch in the fault path for each emulated
device added.
```

> 
> [1]: https://lore.kernel.org/xen-devel/cover.1787838835.git.oleksii.kurochko@gmail.com/T/#m56fbac1ceb0642d5d868e9dcb2b5ed93ecbe5058
>> Routing MMIO faults via a per-device is_access() check in the
>> trap handler would couple it to every device it must serve, requiring a
>> new conditional branch in the fault path each time a new emulated device is
>> added.
>>
>> Introduce a per-domain MMIO handler registration table, modeled
>> after the equivalent ARM framework, so that virtual devices
>> self-register their GPA ranges and read/write callbacks at domain
>> creation time. The MMIO fault path delegates to a single
>> try_handle_mmio() entry point and remains agnostic of which device
>> owns a particular address.
>>
>> A subsequent patch wires this into the MMIO fault path in traps.c.
> ...same here

I'll reword this to:
```
Nothing registers a handler and try_handle_mmio() has no callers yet,
so this patch is a no-op; the trap handler is left untouched.
```

+

I will add after Signed-off-by:
```
---
Wiring this into the MMIO fault path in traps.c is done in this patch
series later.
```


[...]

>> +
>> +int register_mmio_handler(struct domain *d,
>> +                          const struct mmio_handler_ops *ops,
>> +                          paddr_t addr, paddr_t size)
>> +{
>> +    struct vmmio *vmmio = &d->arch.vmmio;
>> +    struct mmio_handler *handlers = vmmio->handlers;
>> +    paddr_t end = addr + size;
>> +    unsigned int i;
>> +    int rc = 0;
>> +    bool overlap;
>> +
>> +    if ( !ops || !ops->read || !ops->write || !size || end < addr )
>> +        return -EINVAL;
> Just a question: is the aim of end < addr check to handle possible overflow of end?
> 

Yes, your understanding is correct.

Thanks.

~ Oleksii