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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/5/26 12:01 PM, Baptiste Le Duc wrote:

[...]

> > diff --git a/xen/arch/riscv/include/asm/mmio.h 
> > b/xen/arch/riscv/include/asm/mmio.h
> > new file mode 100644
> > index 0000000000..1bf48571a9
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/mmio.h
> > @@ -0,0 +1,63 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +#ifndef RISCV_MMIO_H
> Should be ASM__RISCV_MMIO_H instead to be consistant with other header
> files in riscv/include/asm.
According to the coding style document it should be 
<arch>_<something>_H. I understand that some headers in RISC-V looks in 
the way ASM__arch_something_H only becuase they were introduced before 
the header guard recommendations have been updated.
I think I could suggest a separate patch to rename all header guards, at 
least, for RISC-V to avoid such type of confusion in future.
> > +#define RISCV_MMIO_H
> > +
> > +#include <xen/lib.h>
> > +#include <xen/rwlock.h>
> > +
> > +struct domain;
> > +struct vcpu;
> > +
> > +#define MAX_IO_HANDLER  16
> > +
> > +typedef struct {
> > +    paddr_t gpa;
> > +    unsigned int len;  /* Access width in bytes */
> > +    bool is_write;
> > +    /* Store: value to write; load: value read (set by handler) */
> > +    register_t data;
> > +} mmio_info_t;
> > +
> > +enum io_state
> > +{
> Style: should be directly after enum io_state.
I will fix that.

> > +    IO_ABORT,       /* The IO was handled and led to an abort. */
> > +    IO_HANDLED,     /* The IO was successfully handled. */
> > +    IO_UNHANDLED,   /* No handler found for the IO. */
> > +};
> > +
[...]

> > +static enum io_state try_handle_mmio(mmio_info_t *info)
> > +{
> > +    struct vcpu *v = current;
> > +    struct mmio_handler handler = {};
> > +
> > +    if ( !find_mmio_handler(v->domain, info->gpa, &handler) )
> > +        return IO_UNHANDLED;
> > +
> > +    if ( info->is_write )
> > +        return handler.ops->write(v, info);
> > +    else
> The else is not necessary here, misra would call that "dead code".
> Here are two suggestions:
>
>        if ( info->is_write )
>            return handler.ops->write(v, info);
>
>        return handler.ops->read(v, info);
Sure, I'll drop the else and go with the first variant in the next
version.

I am not too much into MISRA stuff and just for my education could you 
please specify what MISRA rule has been violdated here?
I suppose it is because of Rule 2.2 (or 2.1?). I don't think Rule 2.2 
applies here. MISRA C:2012 defines dead code as "any operation that is 
executed but whose removal would not affect program behaviour" (as 
quoted in [1], Section IV). The else doesn't introduce any such 
operation: both forms have the same semantics, and the read() call under 
the else is executed whenever is_write is false, so it is neither dead 
nor unreachable (Rule 2.1).
FWIW, I also compared what gcc 16 generates for RISC-V: at -O0 and -Os
the variants with and without the else are byte-identical, and at
-O1/-O2 the only difference is which of the two calls is laid out as the
fall-through path; the instructions are otherwise the same. But probably 
this is not the best way to verify things...
Also, R2.2 is disapplied project-wide for Xen anyway (see 
docs/misra/deviations.rst).
[1] https://arxiv.org/abs/2212.13933

> or:
>
>        return info->is_write ? handler.ops->write(v, info)
>                              : handler.ops->read(v, info);
If I am not confused something above then why thos option doesn't 
violate MISRA rule abouve 'dead code' isn't ternary operation just an 
equivalent to if/else?
Thanks for review!

~ Oleksii