Re: [PATCH v1 08/14] xen/riscv: imsic_init() implementation

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

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


On 4/16/25 8:31 AM, Jan Beulich wrote:
>>>> --- /dev/null
>>>> +++ b/xen/arch/riscv/include/asm/imsic.h
>>>> @@ -0,0 +1,66 @@
>>>> +/* SPDX-License-Identifier: MIT */
>>>> +
>>>> +/*
>>>> + * xen/arch/riscv/imsic.h
>>>> + *
>>>> + * RISC-V Incoming MSI Controller support
>>>> + *
>>>> + * (c) 2023 Microchip Technology Inc.
>>>> + */
>>>> +
>>>> +#ifndef ASM__RISCV__IMSIC_H
>>>> +#define ASM__RISCV__IMSIC_H
>>>> +
>>>> +#include <xen/types.h>
>>>> +
>>>> +#define IMSIC_MMIO_PAGE_SHIFT   12
>>>> +#define IMSIC_MMIO_PAGE_SZ      (1UL << IMSIC_MMIO_PAGE_SHIFT)
>>>> +
>>>> +#define IMSIC_MIN_ID            63
>>>> +#define IMSIC_MAX_ID            2048
>>>> +
>>>> +struct imsic_msi {
>>>> +    paddr_t base_addr;
>>>> +    unsigned long offset;
>>>> +};
>>>> +
>>>> +struct imsic_mmios {
>>>> +    paddr_t base_addr;
>>>> +    unsigned long size;
>>>> +    bool harts[NR_CPUS];
>>> An array of bool - won't a bitmap do here? Even then I wouldn't be overly
>>> happy to see it dimensioned by NR_CPUS.
>> Bitmap will fit here well. But for DECLARE_BITMAP() is necessary the size
>> of bitmap so NR_CPUS should be used again.
>> Could you please remind me why it isn't good to use it?
>> Because NR_CPUS not always equal to an amount of physical cpus?
> "Not equal" wouldn't be overly problematic. But NR_CPUS=4000 and the actual
> number of CPUs being 4 would be wasteful in general. More when its wider
> than a bit that's needed per CPU, but where would you draw the line if you
> permitted use of NR_CPUS here?
>
>> Should I use non-static version of bitmap declaration? (if we have such...)
> That's simply "unsigned long *" then, or - at the tail of a dynamically
> allocated struct - possibly unsigned long[].
>
>>>> +};
>>>> +
>>>> +struct imsic_config {
>>>> +    /* base address */
>>>> +    paddr_t base_addr;
>>>> +
>>>> +    /* Bits representing Guest index, HART index, and Group index */
>>>> +    unsigned int guest_index_bits;
>>>> +    unsigned int hart_index_bits;
>>>> +    unsigned int group_index_bits;
>>>> +    unsigned int group_index_shift;
>>>> +
>>>> +    /* imsic phandle */
>>>> +    unsigned int phandle;
>>>> +
>>>> +    /* number of parent irq */
>>>> +    unsigned int nr_parent_irqs;
>>>> +
>>>> +    /* number off interrupt identities */
>>>> +    unsigned int nr_ids;
>>>> +
>>>> +    /* mmios */
>>>> +    unsigned int nr_mmios;
>>>> +    struct imsic_mmios *mmios;
>>>> +
>>>> +    /* MSI */
>>>> +    struct imsic_msi msi[NR_CPUS];
>>> You surely can avoid wasting perhaps a lot of memory by allocating this
>>> based on the number of CPUs in use?
>> It make sense. I'll allocate then this dynamically.
> Or, as per above, when put at the tail and the struct itself is
> dynamically allocated, use struct imsic_msi[]. We even have dedicated
> xmalloc() flavors for this kind of allocation.

Do you mean xzalloc_flex_struct()?

I think, I can't use for both of the cases (allocation of mmios and msi).
For msi[] then it is needed to allocate imsic_config also dynamically, isn't it?
So something like:
  imsic_config = xzalloc_flex_struct(struct imsic_config, msi, NR_CPUS).
But now it is allocated statically.

For *mmios and harts[] (a member inside struct imsic_mmios):
   mmios = xzalloc_flex_struct(struct imsic_mmios, harts, NR_CPUS); // NR_CPUs just for example...
It will allocate only one mmios, but it is needed mmios[nr_mmios].
Maybe, something like _xmalloc((offsetof(struct imsic_mmios, harts[NR_CPUS])) * NR_CPUS, sizeof(struct imsic_mmios)) will work.

Am I missing something?

~ Oleksii

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