Re: [PATCH 0/2] arm-commmon: gic-v3: Update detection of last GICR region

[Jan Kiszka]

On 2018-03-14 05:54, Lokesh Vutla wrote:
> From: Lokesh Vutla <lokes...@ti.com>
>
> This series introduces an api to find the last cpu available in the system
> CPU set. Using this, the GICR region corresponding to the last available CPU
> is marked as last.

What if the CPU set is not contiguous?

Jan

>
> Lokesh Vutla (2):
> core: Introduce an api to find last cpu
> arm-common: gic-v3: Mark last CPUs GICR as last
>
> hypervisor/arch/arm-common/gic-v3.c | 6 +++++-
> hypervisor/control.c | 17 +++++++++++++++++
> hypervisor/include/jailhouse/control.h | 1 +
> 3 files changed, 23 insertions(+), 1 deletion(-)
>

--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

[Lokesh Vutla]

On Wednesday 14 March 2018 06:53 PM, Jan Kiszka wrote:
> On 2018-03-14 05:54, Lokesh Vutla wrote:
>> From: Lokesh Vutla <lokes...@ti.com>
>>
>> This series introduces an api to find the last cpu available in the system
>> CPU set. Using this, the GICR region corresponding to the last available CPU
>> is marked as last.
>
> What if the CPU set is not contiguous?

I thought about non-contiguous CPU set in non-root cell and thought this
should work. If you are talking about non contiguous CPU set in root
cell, then yes i missed this point. This scenario will still fail.

Should we allow GICR region access till the last available CPU,
irrespective of the CPU validity?

Thanks and regards,
Lokesh

[Ralf Ramsauer]

Hi,

Jan, could you please check the spam folder of the google group? Things
seem to stuck there again, I didn't receive the series via the mailing list.

Thanks
Ralf

[Lokesh Vutla]

From: Lokesh Vutla <lokes...@ti.com>

Introduce cpu_id_last() api in order to find the
last cpu in the system's cpu set.

Signed-off-by: Lokesh Vutla <lokes...@ti.com>
---

hypervisor/control.c | 17 +++++++++++++++++
hypervisor/include/jailhouse/control.h | 1 +

2 files changed, 18 insertions(+)

diff --git a/hypervisor/control.c b/hypervisor/control.c
index 52ef8733..a8921b72 100644
--- a/hypervisor/control.c
+++ b/hypervisor/control.c
@@ -72,6 +72,23 @@ bool cpu_id_valid(unsigned long cpu_id)
test_bit(cpu_id, system_cpu_set));
}

+/**
+ * Check if the CPU ID is the last CPU in the system's CPU set.
+ * @param cpu_id CPU ID to identify if it is the last
+ *
+ * @return True if CPU ID is the last cpu else false.
+ */
+bool cpu_id_last(unsigned long cpu_id)
+{
+ const unsigned long *system_cpu_set =
+ jailhouse_cell_cpu_set(&system_config->root_cell);
+ unsigned long cpu_mask;
+
+ cpu_mask = (1UL << (cpu_id % BITS_PER_LONG)) - 1;
+
+ return (~cpu_mask & *system_cpu_set) == (cpu_mask + 1);
+}
+
/*
* Suspend all CPUs assigned to the cell except the one executing
* the function (if it is in the cell's CPU set) to prevent races.
diff --git a/hypervisor/include/jailhouse/control.h b/hypervisor/include/jailhouse/control.h
index 274dc642..ce00b88a 100644
--- a/hypervisor/include/jailhouse/control.h
+++ b/hypervisor/include/jailhouse/control.h
@@ -118,6 +118,7 @@ static inline bool cell_owns_cpu(struct cell *cell, unsigned int cpu_id)
}

bool cpu_id_valid(unsigned long cpu_id);
+bool cpu_id_last(unsigned long cpu_id);

int cell_init(struct cell *cell);

--
2.16.2

[Lokesh Vutla]

From: Lokesh Vutla <lokes...@ti.com>

This series introduces an api to find the last cpu available in the system
CPU set. Using this, the GICR region corresponding to the last available CPU
is marked as last.

Lokesh Vutla (2):
core: Introduce an api to find last cpu
arm-common: gic-v3: Mark last CPUs GICR as last

hypervisor/arch/arm-common/gic-v3.c | 6 +++++-

hypervisor/control.c | 17 +++++++++++++++++
hypervisor/include/jailhouse/control.h | 1 +

3 files changed, 23 insertions(+), 1 deletion(-)

--
2.16.2

[Jan Kiszka]

On 2018-03-14 06:34, Lokesh Vutla wrote:
>
>
> On Wednesday 14 March 2018 06:53 PM, Jan Kiszka wrote:
>> On 2018-03-14 05:54, Lokesh Vutla wrote:
>>> From: Lokesh Vutla <lokes...@ti.com>
>>>
>>> This series introduces an api to find the last cpu available in the system
>>> CPU set. Using this, the GICR region corresponding to the last available CPU
>>> is marked as last.
>>
>> What if the CPU set is not contiguous?
>
> I thought about non-contiguous CPU set in non-root cell and thought this
> should work. If you are talking about non contiguous CPU set in root
> cell, then yes i missed this point. This scenario will still fail.
>
> Should we allow GICR region access till the last available CPU,
> irrespective of the CPU validity?

I'm starting to get the problem, but I need to think it through again.

Basically, we need to set a terminator over the range of GICR regions we
allow to read if that region is not identical to the real one, thus is
lacking the terminator. In that case, non-contiguous CPU sets do not
make a difference, neither for root (which shouldn't scan anymore after
being booted, but even if it did) nor non-root cells.

Jan

[Jan Kiszka]

On 2018-03-14 05:54, Lokesh Vutla wrote:

Conceptually, CPU sets can be larger than fits into unsigned long. So
this is not generic. But I think we can avoid this API anyway.

Jan

[Lokesh Vutla]

hmm...I have this assumption from cpu_is_valid() api. It also uses
unsigned long.

[Jan Kiszka]

Unsigned long is fine to hold a CPU ID, but it is insufficient to hold
the complete mask. cpu_is_valid uses test_bit for mask access, in fact.

Jan

[Lokesh Vutla]

But mask should be within the same range of system_cpu_set right?
system_cpu_set is unsigned long. Or did I miss something?

[Jan Kiszka]

The bitmask that defines a cell's CPU set, which includes the initial
root cell set, is not limited to unsigned long. It can be as large as
jailhouse_cell_desc.cpu_set_size specifies, in theory.

Jan