Re: [PATCH v6 2/7] xen/riscv: aplic_init() implementation

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 7/8/25 3:58 PM, Jan Beulich wrote:

> On 07.07.2025 11:01, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/aplic-priv.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: MIT */
> > +
> > +/*
> > + * xen/arch/riscv/aplic-priv.h
> > + *
> > + * Private part of aplic.h header.
> > + *
> > + * RISC-V Advanced Platform-Level Interrupt Controller support
> > + *
> > + * Copyright (c) Microchip.
> > + * Copyright (c) Vates.
> > + */
> > +
> > +#ifndef ASM_RISCV_PRIV_APLIC_H
> > +#define ASM_RISCV_PRIV_APLIC_H
> > +
> > +#include <xen/types.h>
> > +
> > +#include <asm/aplic.h>
> > +#include <asm/imsic.h>
> >       
> Why is the latter one needed here? Certainly not for ...
>
> > +struct aplic_priv {
> > +    /* Base physical address and size */
> > +    paddr_t paddr_start;
> > +    size_t  size;
> > +
> > +    /* Registers */
> > +    volatile struct aplic_regs __iomem *regs;
> > +
> > +    /* IMSIC configuration */
> > +    const struct imsic_config *imsic_cfg;
> >       
> ... this field decl (which itself serves as a forward decl of the struct
> tag).
>     
The purpose was to deal with a forward decl but I agree that in this case
inclusion of <asm/imsic.h> could be dropped.

> > --- a/xen/arch/riscv/aplic.c
> > +++ b/xen/arch/riscv/aplic.c
> > @@ -9,19 +9,118 @@
> >   * Copyright (c) 2024-2025 Vates
> >   */
> >  
> > +#include <xen/device_tree.h>
> >  #include <xen/errno.h>
> >  #include <xen/init.h>
> >  #include <xen/irq.h>
> > +#include <xen/mm.h>
> >  #include <xen/sections.h>
> >  #include <xen/types.h>
> > +#include <xen/vmap.h>
> > +
> > +#include "aplic-priv.h"
> >  
> >  #include <asm/device.h>
> > +#include <asm/imsic.h>
> >  #include <asm/intc.h>
> > +#include <asm/riscv_encoding.h>
> > +
> > +#define APLIC_DEFAULT_PRIORITY  1
> > +
> > +static struct aplic_priv aplic;
> >       
> Is this altered post-init? IOW can it be __ro_after_init? Or otherwise at
> least __read_mostly?
>     
Looking at the current downstream code there is, at least one case, where aplic->regs
are changing (during vaplic_emulate_store()).
So __read_mostly would be better in this case.

> With these two taken care of (one way or another):
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> If you clarify what is wanted / needed, I'm also happy to make adjustments
> while committing.
>     
It would be nice to do the following:
- Drop the inclusion of <asm/imsic.h> in aplic-priv.h.
- Add __read_mostly to the definition of the aplic variable.

Thanks for adjustments.

~ Oleksii