Re: [PATCH v2 3/4] xen/riscv: implement basic aplic_preinit()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 3/27/25 8:39 AM, Jan Beulich wrote:

> On 26.03.2025 17:49, Oleksii Kurochko wrote:
> > On 3/26/25 4:19 PM, Jan Beulich wrote:
> > > On 25.03.2025 18:36, Oleksii Kurochko wrote:
> > > > Introduce preinitialization stuff for the RISC-V Advanced Platform-Level
> > > > Interrupt Controller (APLIC) in Xen:
> > > >   - Implementing the APLIC pre-initialization function (`aplic_preinit()`),
> > > >     ensuring that only one APLIC instance is supported in S mode.
> > > >   - Initialize APLIC's correspoinding DT node.
> > > >   - Declaring the DT device match table for APLIC.
> > > >   - Setting `aplic_info.hw_version` during its declaration.
> > > >   - Declaring an APLIC device.
> > > >
> > > > Since Microchip originally developed aplic.c [1], an internal discussion
> > > > with them led to the decision to use the MIT license instead of the default
> > > > GPL-2.0-only.
> > > >
> > > > [1]https://gitlab.com/xen-project/people/olkur/xen/-/commit/7cfb4bd4748ca268142497ac5c327d2766fb342d
> > > >
> > > > Signed-off-by: Romain Caritey<Romain.Caritey@xxxxxxxxxxxxx>
> > > > Signed-off-by: Oleksii Kurochko<oleksii.kurochko@xxxxxxxxx>
> > > >           
> > > You recall that From: != 1st S-o-b is unusual, and wants some explanation.
> > > IOW it's unclear who the original author of this patch is.
> > >         
> > I'm not 100% sure who should be the author. Such patch doesn't exist before but I took the changes
> > based on the changes mentioned in commit message as [1].
> >
> > If you think that the author should be Romain, I am okay with that.
> >       
> I can't sensibly form an opinion here. This needs settling between him and you.
> >From your reply I'm not even convinced his S-o-b is legitimately there then.
> You may want to use another, less standard tag in such a case (like the
> Co-developed-by: that I've seen in use here and there) to still give credit to
> him.
>     
I'll have a conversation with Romain and then just re-send the patch.
> > > > --- /dev/null
> > > > +++ b/xen/arch/riscv/aplic.c
> > > > @@ -0,0 +1,49 @@
> > > > +/* SPDX-License-Identifier: MIT */
> > > > +
> > > > +/*
> > > > + * xen/arch/riscv/aplic.c
> > > > + *
> > > > + * RISC-V Advanced Platform-Level Interrupt Controller support
> > > > + *
> > > > + * Copyright (c) 2023-2024 Microchip.
> > > > + * Copyright (c) 2024-2025 Vates
> > > > + */
> > > > +
> > > > +#include <xen/errno.h>
> > > > +#include <xen/init.h>
> > > > +#include <xen/types.h>
> > > > +
> > > > +#include <asm/device.h>
> > > > +#include <asm/intc.h>
> > > > +
> > > > +static struct intc_info aplic_info = {
> > > > +    .hw_version = INTC_APLIC
> > > > +};
> > > >           
> > > Is this going to be written to (much) post-init? IOW - __read_mostly or
> > > even __ro_after_init?
> > >         
> > I think that __read_mostly would be better because intc_info structure in the future
> > will contain member "void *private". And in `private` it can be a data which can
> > be changed.
> >       
> You mean the pointer can change? Or merely what it points to, i.e. ...
>
> > For example, `private` can contain an aplic_priv structure:
> > struct aplic_priv {
> >      /* number of irqs */
> >      uint32_t   nr_irqs;
> >
> >      /* base physical address and size */
> >      paddr_t    paddr_start;
> >      paddr_t    paddr_end;
> >      uint64_t   size;
> >
> >      /* registers */
> >      struct aplic_regs   *regs;
> >
> >      /* imsic configuration */
> >      const struct imsic_config *imsic_cfg;
> > };
> >
> > and regs from aplic_priv structure can be changed in runtime.
> >       
> ... the contents of such a struct? In this latter case the struct instance
> here can still be __ro_after_init as long as the pointer is set from an
> __init function.
>     
I meant that the data will be changed. The pointer will be initialized once
in the following way:
static struct intc_info aplic_info = {
    .hw_version = INTC_APLIC,
    .private = &aplic
};

I will update for both structures __ro_after_init() then.

Thanks for clarification.

~ Oleksii