Hi Jan, Ralf, 

Thank you both for your feedback on this RFC. Glad you like it.

<snip>

>>> diff --git a/configs/arm64/cell-helper.h b/configs/arm64/cell-helper.h
>>> new file mode 100644
>>> index 00000000..d35bc0fb
>>> --- /dev/null
>>> +++ b/configs/arm64/cell-helper.h
>>> @@ -0,0 +1,76 @@
>>> +/*
>>> + * Cell Configuration - Generic definitions
>>> + *
>>> + * Copyright 2021 NXP
>>> + *
>>> + * Authors:
>>> + *  Stephane Viau <[email protected]>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>>> + * the COPYING file in the top-level directory.
>>> + *
>>> + */
>>> +
>>> +#ifndef KB
>>> +#define KB(bytes)   (1024 * (bytes))
>>> +#define MB(bytes)   (1024 * KB(bytes))
>>> +#endif
>>> +
>>> +#define REGION(phys, virt, bytes) \
>>> +    .phys_start = (phys), \
>>> +    .virt_start = (virt), \
>>> +    .size = (bytes) \
>>> +
>>> +#define MEM_REGION_RW(phys, virt, bytes) \
>>> +    { \
>>> +            REGION(phys, virt, bytes), \
>>> +            .flags = JAILHOUSE_MEM_READ | JAILHOUSE_MEM_WRITE, \
>>> +    }
>>> +
>>> +#define COMM_REGION_RW(virt, bytes) \
>>> +    { \
>>> +            REGION(0x00000000, virt, bytes), \
>>> +            .flags = JAILHOUSE_MEM_READ | JAILHOUSE_MEM_WRITE | \
>>> +                     JAILHOUSE_MEM_COMM_REGION, \
>>> +    }
>>> +
>>> +#define MEM_REGION_RWX(phys, virt, bytes) \
>>> +    { \
>>> +            REGION(phys, virt, bytes), \
>>> +            .flags = JAILHOUSE_MEM_READ | JAILHOUSE_MEM_WRITE | \
>>> +                     JAILHOUSE_MEM_EXECUTE, \
>>> +            }
>>> +
>>> +#define MEM_REGION_RWXL(phys, virt, bytes) \
>>> +    { \
>>> +            REGION(phys, virt, bytes), \
>>> +            .flags = JAILHOUSE_MEM_READ | JAILHOUSE_MEM_WRITE | \
>>> +                     JAILHOUSE_MEM_EXECUTE | JAILHOUSE_MEM_LOADABLE, \
>>> +    }
>>> +
>>> +#define MMIO_REGION_RO(phys, virt, bytes) \
>>> +    { \
>>> +            REGION(phys, virt, bytes), \
>>> +            .flags = JAILHOUSE_MEM_READ | JAILHOUSE_MEM_IO, \
>>> +    }
>>> +
>>> +#define MMIO_REGION_ROS(phys, virt, bytes) \
>>> +    { \
>>> +            REGION(phys, virt, bytes), \
>>> +            .flags = JAILHOUSE_MEM_READ | JAILHOUSE_MEM_IO | \
>>> +                     JAILHOUSE_MEM_ROOTSHARED, \
>>> +    }
>>> +
>>> +#define MMIO_REGION_RW(phys, virt, bytes) \
>>> +    { \
>>> +            REGION(phys, virt, bytes), \
>>> +            .flags = JAILHOUSE_MEM_READ | JAILHOUSE_MEM_WRITE | \
>>> +                     JAILHOUSE_MEM_IO, \
>>> +    }
>>> +
>>> +#define MMIO_REGION_RWS(phys, virt, bytes) \
>>> +    { \
>>> +            REGION(phys, virt, bytes), \
>>> +            .flags = JAILHOUSE_MEM_READ | JAILHOUSE_MEM_WRITE | \
>>> +                     JAILHOUSE_MEM_IO | JAILHOUSE_MEM_ROOTSHARED, \
>>> +    }
>
>Nice, I guess that at least these macros really help. Sysconfigs would
>also benefit from it.

Definitely help creating cell configs (we use something like ~10 variants
that look alike) - it's much faster - and more readable - this way.

>
>>> diff --git a/configs/arm64/cell-template.c b/configs/arm64/cell-template.c
>>> new file mode 100644
>>> index 00000000..bf731101
>>> --- /dev/null
>>> +++ b/configs/arm64/cell-template.c
>>> @@ -0,0 +1,32 @@
>>> +/*
>>> + * Cell Configuration - Structure definition
>>> + *
>>> + * Copyright 2021 NXP
>>> + *
>>> + * Authors:
>>> + *  Stephane Viau <[email protected]>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>>> + * the COPYING file in the top-level directory.
>>> + */
>>> +
>>> +#include "cell-helper.h"
>>> +
>>> +/* Name, cores, entry point */
>>> +#define CONFIG_INMATE_NAME          "inmate-cell-name"
>>> +#define CONFIG_INMATE_CORE_BITMAP   (0b1100) /* inmate uses cores 2 and 3 
>>> */
>>> +#define CONFIG_INMATE_BASE          (0xc0000000) /* entry point in DDR */
>>> +
>>> +/* Memory & peripherals */
>>> +#define CONFIG_INMATE_REGIONS_NUM   (1)
>>> +#define CONFIG_INMATE_REGIONS               \
>>> +    MEM_REGION_RWXL(0xc0000000, 0xc0000000, MB(16)),   /* RAM */    \
>>> +
>>> +/* GIC */
>>> +#define CONFIG_INMATE_IRQCHIPS_NUM  (1)
>>> +#define CONFIG_INMATE_IRQCHIPS_ADDR (0x38800000) /* GIC DISTRIBUTOR BASE 
>>> ADDR */
>>> +#define CONFIG_INMATE_IRQCHIPS_BASE (32)
>>> +#define CONFIG_INMATE_IRQCHIPS_BITMAP       \
>>> +    (1 << (29 + 32 - 32)) /* UART4 */
>
>In case of the irqchips, I don't see that the definitions bring any
>help. It's about the same size as rolling out the structures directly.

Well, not much true. But since I'm trying to hide field names of the structure, 
why not.

>
>>> +
>>> +#include "cell-create.h"
>>>
>>
>> Thanks for the proposal. Could you share some converted inmates as well
>> so that we can see the impact more clearly?

Let me send a v2 with an example as a 2nd patch.

>
>Do you also have any plans for system configurations? At least the
>MEM_REGION-macros could help to condense stuff there as well.

Hmm... not sure yet what that is (I'm just starting using Jailhouse) ; but will
surely take a look at it after sending my v2 patch set.

BR,
Stephane.

>
>Thanks
>   Ralf
>
>>
>> Jan
>>
>

-- 
You received this message because you are subscribed to the Google Groups 
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
To view this discussion on the web visit 
https://groups.google.com/d/msgid/jailhouse-dev/PAXPR04MB88637977C317FB8FB91A3203A7679%40PAXPR04MB8863.eurprd04.prod.outlook.com.

Reply via email to