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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.03.2025 17:58, Oleksii Kurochko wrote:
> 
> On 3/17/25 4:40 PM, Jan Beulich wrote:
>> On 11.03.2025 17:19, Oleksii Kurochko wrote:
>>> Based on the code from [1] provided by Romain Caritey from Microchip
>>> with some minor changes():
>> "Based on", to me at least, suggests an import from some other project. The
>> reference looks to be to a Xen tree though. With that, a proper description
>> would then be needed here, rather than a list of differences.
> 
> Sorry, but I don't understand fully what kind of description is needed? Could
> you please give me some pointers?

Any patch introducing new functionality not taken from another project can
be used as reference. In a patch like this the description wants to say
what the patch does, and what - if anything - is specially noteworthy. The
patch here isn't very large, so it's not going to be much that needs saying.
Yet merely referring to elsewhere would mean the description of what is
being done can be found there (e.g. in the history of that other project's
file(s)). Which isn't the case here, afaict.

>>> +static int __init aplic_preinit(struct dt_device_node *node, const void 
>>> *dat)
>>> +{
>>> +    if ( aplic_info.node )
>>> +    {
>>> +        printk("XEN doesn't support more than one S mode APLIC\n");
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    /* don't process if APLIC node is not for S mode */
>>> +    if ( dt_get_property(node, "riscv,children", NULL) )
>>> +        return -ENODEV;
>>> +
>>> +    aplic_info.node = node;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct dt_device_match aplic_dt_match[] __initconst =
>>> +{
>>> +    DT_MATCH_COMPATIBLE("riscv,aplic"),
>> Iirc this requires use of __initconstrel, which would also be nice to
>> be put between type and identifier.
> 
> Could you please explain why do we need __initconstrel? Arm uses only 
> __initconst
> for such definitions.

And perhaps wrongly so. The distinction may not be that crucial anymore these
days, but there was a range of gcc versions where not using the correct one
could lead to "section type conflict" warnings / errors. Which one to use
depends on whether any of the field initializers require a relocation. That
is, a pointer type field initialized to other than NULL would normally mean
__initconstrel needs using.

Jan