On 06/03/2026 17:53, Julien Grall wrote:
Hi Ayan,
Hi Julien,
On 06/03/2026 17:19, Halder, Ayan Kumar wrote:
On 06/03/2026 10:40, Julien Grall wrote:
Hi Ayan,
Hi Julien,
On 06/03/2026 09:51, Halder, Ayan Kumar wrote:
On 06/03/2026 09:24, Julien Grall wrote:
Hi Ayan,
Hi Julien,
On 05/03/2026 19:43, Ayan Kumar Halder wrote:
Set GICV3_NR_LRS as per the number of list registers in the
supported
hardware. This ensures:
1. In gicv3_save_lrs()/gicv3_restore_lrs(), use the number of list
registers from GICV3_NR_LRS (if defined) instead of
gicv3_info.nr_lrs.
This ensures that if the hardware does not support more than 4 LRs
(for example), the code accessing LR 4-15 is never reached. The
compiler can eliminate the unsupported cases as the switch case
uses a
constant conditional.
2. Similarly In gicv3_ich_read_lr()/gicv3_ich_write_lr() , we can
justify that the unsupported LRs (4-15) will never be reached as Xen
will panic if the runtime value (lr) exceeds GICV3_NR_LRS. Some
compiler can eliminate the code accessing LR 4-15.
In this situation, using panic() is better than accessing a list
register which is not present in the hardware
3. Whenever GICV3_NR_LRS is defined, the default condition and the
related BUG() cannot be reached at all.
I am not sure how this is better. You will still crash Xen is 'lr'
>= GICV3_NR_LRS. Can you provide some details?
> > As part of functional safety effort, we are trying to enable
system
integrator to configure Xen for a specific platform with a predefind
set of GICv3 list registers. So that we can minimize the chance of
runtime issues and reduce the codesize that will execute.
Signed-off-by: Ayan Kumar Halder <[email protected]>
Signed-off-by: Michal Orzel <[email protected]>
---
xen/arch/arm/Kconfig | 9 +++++++++
xen/arch/arm/gic-v3.c | 12 ++++++++++--
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 2f2b501fda..6540013f97 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -276,6 +276,15 @@ config PCI_PASSTHROUGH
endmenu
+config GICV3_NR_LRS
+ int "Number of GICv3 Link Registers supported" if EXPERT
+ depends on GICV3
+ range 0 16
+ default 0
+ help
+ Controls the number of Link registers to be accessed.
+ Keep it set to 0 to use a value obtained from a hardware
register.
+
menu "ARM errata workaround via the alternative framework"
depends on HAS_ALTERNATIVE
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index bc07f97c16..fb2985fd52 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -51,6 +51,8 @@ static DEFINE_PER_CPU(void __iomem*, rbase);
#define GICD (gicv3.map_dbase)
#define GICD_RDIST_BASE (this_cpu(rbase))
#define GICD_RDIST_SGI_BASE (GICD_RDIST_BASE + SZ_64K)
+#define lrs (CONFIG_GICV3_NR_LRS ?: \
+ gicv3_info.nr_lrs)
We should avoid lowercase define, in particular with generic names
like 'lrs'. I think in this case, I would rather update
gicv3_info.nr_lrs:
gicv3_info.nr_lrs = min(gv3_info.nr_lrs, CONFIG_GICV3_NR_LRS);
But we want to enforce the user to set CONFIG_GICV3_NR_LRS , so
that we don't have to rely on gicv3_info.nr_lrs.
> The only reason to use gicv3_info.nr_lrs is for backward
compatibility
i.e. when the user forgot to set the config and as a result it used
the default value as 0. We don't want to allow this for the safety
use cases.
Xen upstream has to support various use cases. One of the use-case
is Linux distributions where they want one Xen binary booting on
multiple HW. So "gicv3_info.nr_lrs" will have to stay forever.
We can default to using gicv3_info.nr_lrs if the user does not set a
value in GICV3_NR_LRS. This is what I am trying to do in the patch.
I am still missing something. Why can't we just write GICV3_NR_LRS in
gicv3_info.nr_lrs? This would simplify the code and avoid unnecessary
churn.
I am trying to use compiler to do dead code elimination using constant
conditional. Refer the snippet:
static inline void gicv3_save_lrs(struct vcpu *v)
{
/* Fall through for all the cases */
switch ( lrs )
Here the pre-processor evaluates lrs to a compile time constant
(GICV3_NR_LRS). Thus, the compiler eliminates the code to access
ich_lr4_el2, etc when GICV3_NR_LRS == 4.
Is there a better way to achieve DCE ?
Now with CONFIG_GICV3_NR_LRS in place, I am concerned that someone
will try to configure the value to let say 16 but their HW support
only 4.
If the user has set it to an incorrect value, then imo Xen should
trust the value that user has set. And then any malfunction caused
will be the responsibility of the user.
> > IOW, we want to put the responsibility on the user to provide the
correct values.
I think this is the same things as the device-tree. We could trust the
user didn't shoot itself in the foot (e.g. asking for a feature which
doesn't exist). But at least for Xen on Arm, we try to be nice and
tell the user that something is wrong early.
I don't see why we should diverge here. This is defense in depth which
would save a crash during steady state if this was missed. I am
assuming that it is better to crash while your car is parked than
while you are driving ;).
Yes, I am fine to put a ASSERT() or BUG_ON() in gicv3_hyp_init()
This would solve another problem where you don't sanity check that
the system effectively support CONFIG_GICV3_NR_LRS.
@@ -121,7 +123,7 @@ static inline void gicv3_save_lrs(struct vcpu
*v)
static inline void gicv3_restore_lrs(const struct vcpu *v)
{
/* Fall through for all the cases */
- switch ( gicv3_info.nr_lrs )
+ switch ( lrs )
{
case 16:
WRITE_SYSREG_LR(v->arch.gic.v3.lr[15], 15);
@@ -178,6 +180,9 @@ static inline void gicv3_restore_lrs(const
struct vcpu *v)
static uint64_t gicv3_ich_read_lr(int lr)
{
+ if ( lr >= lrs )
+ panic("Unsupported number of LRs\n");
Do we really have to panic in production build? Wouldn't it be
better to return '0' and maybe use ASSERT for a crash in debug
build? Same below.
You are right, we may not need this. One option I am thinking is ...
+
switch ( lr )
switch ( lr & (lrs - 1) )
This ensures that we do not hit the unsupported cases.
What about implementing it as RAZ-WI?
You mean
case 4: (lr >= lrs) ? 0 : READ_SYSREG_LR(4); /* read */
case 4 : (lr >= lrs) ? : WRITE_SYSREG_LR(val, 4); /* write */
I was thinking to do the check before the 'switch' as i don't think it
needs to be duplicated per 'case'.
Yes, I agree.
- Ayan
