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

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

[-- Attachment #1: Type: text/plain, Size: 2778 bytes --]


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@microchip.com>
>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
> 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.

>
>> --- /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. 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.

>
> With authorship clarified and this variable adjusted according to whatever
> the longer term use of it is
> Acked-by: Jan Beulich<jbeulich@suse.com>

Thanks.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 4085 bytes --]