[PATCH] core: reinitialise comm region on cell restart

[Ralf Ramsauer]

The communication region is RW for both, the inmate and the hypervisor.
As a consequence, an inmate may overwrite the whole region. Currently,
the region is filled in cell_create.

Move initialisation to cell_start to provide a proper initial state when
starting the cell. Move architecture specific initialisation to
arch_cell_reset, which will be called by cell_start.

Leave the cell's shut down state in cell_create to maintain a valid
state after cell creation.

Signed-off-by: Ralf Ramsauer <ralf.r...@oth-regensburg.de>
---

NOTE: only tested on ARM, compile-time tested on x86. Currently lacking
an x86 target.

hypervisor/arch/x86/control.c | 25 +++++++++++++------------
hypervisor/control.c | 20 ++++++++++++--------
2 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/hypervisor/arch/x86/control.c b/hypervisor/arch/x86/control.c
index e51cf52c..4a6776ce 100644
--- a/hypervisor/arch/x86/control.c
+++ b/hypervisor/arch/x86/control.c
@@ -32,24 +32,12 @@ struct exception_frame {

int arch_cell_create(struct cell *cell)
{
- struct jailhouse_comm_region *comm_region = &cell->comm_page.comm_region;
- unsigned int cpu;
int err;

err = vcpu_cell_init(cell);
if (err)
return err;

- comm_region->pm_timer_address =
- system_config->platform_info.x86.pm_timer_address;
- comm_region->pci_mmconfig_base =
- system_config->platform_info.pci_mmconfig_base;
- comm_region->num_cpus = 0;
- for_each_cpu(cpu, cell->cpu_set)
- comm_region->num_cpus++;
- comm_region->tsc_khz = system_config->platform_info.x86.tsc_khz;
- comm_region->apic_khz = system_config->platform_info.x86.apic_khz;
-
return 0;
}

@@ -103,6 +91,19 @@ void arch_cell_destroy(struct cell *cell)

void arch_cell_reset(struct cell *cell)
{
+ struct jailhouse_comm_region *comm_region = &cell->comm_page.comm_region;
+ unsigned int cpu;
+
+ comm_region->pm_timer_address =
+ system_config->platform_info.x86.pm_timer_address;
+ comm_region->pci_mmconfig_base =
+ system_config->platform_info.pci_mmconfig_base;
+ comm_region->num_cpus = 0;
+ for_each_cpu(cpu, cell->cpu_set)
+ comm_region->num_cpus++;
+ comm_region->tsc_khz = system_config->platform_info.x86.tsc_khz;
+ comm_region->apic_khz = system_config->platform_info.x86.apic_khz;
+
ioapic_cell_reset(cell);
}

diff --git a/hypervisor/control.c b/hypervisor/control.c
index 1025852f..78a7c221 100644
--- a/hypervisor/control.c
+++ b/hypervisor/control.c
@@ -341,7 +341,6 @@ static int cell_create(struct per_cpu *cpu_data, unsigned long config_address)
{
unsigned long cfg_page_offs = config_address & ~PAGE_MASK;
unsigned int cfg_pages, cell_pages, cpu, n;
- struct jailhouse_comm_region *comm_region;
const struct jailhouse_memory *mem;
struct jailhouse_cell_desc *cfg;
unsigned long cfg_total_size;
@@ -476,11 +475,7 @@ static int cell_create(struct per_cpu *cpu_data, unsigned long config_address)

config_commit(cell);

- comm_region = &cell->comm_page.comm_region;
- memcpy(comm_region->signature, COMM_REGION_MAGIC,
- sizeof(comm_region->signature));
- comm_region->revision = COMM_REGION_ABI_REVISION;
- comm_region->cell_state = JAILHOUSE_CELL_SHUT_DOWN;
+ cell->comm_page.comm_region.cell_state = JAILHOUSE_CELL_SHUT_DOWN;

last = &root_cell;
while (last->next)
@@ -558,6 +553,7 @@ static int cell_management_prologue(enum management_task task,

static int cell_start(struct per_cpu *cpu_data, unsigned long id)
{
+ struct jailhouse_comm_region *comm_region;
const struct jailhouse_memory *mem;
unsigned int cpu, n;
struct cell *cell;
@@ -582,8 +578,16 @@ static int cell_start(struct per_cpu *cpu_data, unsigned long id)
}

/* present a consistent Communication Region state to the cell */
- cell->comm_page.comm_region.cell_state = JAILHOUSE_CELL_RUNNING;
- cell->comm_page.comm_region.msg_to_cell = JAILHOUSE_MSG_NONE;
+ comm_region = &cell->comm_page.comm_region;
+
+ memset(&cell->comm_page, 0, sizeof(cell->comm_page));
+
+ comm_region->revision = COMM_REGION_ABI_REVISION;
+ memcpy(comm_region->signature, COMM_REGION_MAGIC,
+ sizeof(comm_region->signature));
+
+ comm_region->cell_state = JAILHOUSE_CELL_RUNNING;
+ comm_region->msg_to_cell = JAILHOUSE_MSG_NONE;

pci_cell_reset(cell);
arch_cell_reset(cell);
--
2.17.0

[Jan Kiszka]

/* comm_region is zero-initialized */

With the memset, these two can become comments:

/*
* Includes:
* cell_state = JAILHOUSE_CELL_RUNNING (0)
* msg_to_cell = JAILHOUSE_MSG_NONE (0)
*/
memset(...);

Jan

>
> pci_cell_reset(cell);
> arch_cell_reset(cell);
>

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

[Ralf Ramsauer]

I will replace those paths with comments, but we should keep those
implicit assumptions in mind....

Thanks
Ralf

>
> Jan
>
>>
>> pci_cell_reset(cell);
>> arch_cell_reset(cell);
>>
>

[Ralf Ramsauer]

The communication region is RW for both, the inmate and the hypervisor.
As a consequence, an inmate may overwrite the whole region. Currently,
the region is filled in cell_create.

Move initialisation to cell_start to provide a proper initial state when
starting the cell. Move architecture specific initialisation to
arch_cell_reset, which will be called by cell_start.

Leave the cell's shut down state in cell_create to maintain a valid
state after cell creation.

Signed-off-by: Ralf Ramsauer <ralf.r...@oth-regensburg.de>
---

since v1:
- assume that all fields are zero initialised, and save some
assignements. Leave comments.

hypervisor/arch/x86/control.c | 23 ++++++++++++-----------
hypervisor/control.c | 23 ++++++++++++++---------
2 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/hypervisor/arch/x86/control.c b/hypervisor/arch/x86/control.c
index e51cf52c..a13ea711 100644
--- a/hypervisor/arch/x86/control.c
+++ b/hypervisor/arch/x86/control.c
@@ -32,7 +32,6 @@ struct exception_frame {

int arch_cell_create(struct cell *cell)
{
- struct jailhouse_comm_region *comm_region = &cell->comm_page.comm_region;

unsigned int cpu;
int err;

@@ -40,16 +39,6 @@ int arch_cell_create(struct cell *cell)

if (err)
return err;

- comm_region->pm_timer_address =
- system_config->platform_info.x86.pm_timer_address;
- comm_region->pci_mmconfig_base =
- system_config->platform_info.pci_mmconfig_base;
- comm_region->num_cpus = 0;
- for_each_cpu(cpu, cell->cpu_set)
- comm_region->num_cpus++;
- comm_region->tsc_khz = system_config->platform_info.x86.tsc_khz;
- comm_region->apic_khz = system_config->platform_info.x86.apic_khz;
-
return 0;
}

@@ -103,6 +92,18 @@ void arch_cell_destroy(struct cell *cell)

void arch_cell_reset(struct cell *cell)
{
+ struct jailhouse_comm_region *comm_region = &cell->comm_page.comm_region;
+

+ comm_region->pm_timer_address =
+ system_config->platform_info.x86.pm_timer_address;
+ comm_region->pci_mmconfig_base =
+ system_config->platform_info.pci_mmconfig_base;

+ /* comm_region, and hence num_cpus, is zero-initialised */

+ for_each_cpu(cpu, cell->cpu_set)
+ comm_region->num_cpus++;
+ comm_region->tsc_khz = system_config->platform_info.x86.tsc_khz;
+ comm_region->apic_khz = system_config->platform_info.x86.apic_khz;
+
ioapic_cell_reset(cell);
}

diff --git a/hypervisor/control.c b/hypervisor/control.c

index 1025852f..9bb1c865 100644

@@ -581,9 +577,18 @@ static int cell_start(struct per_cpu *cpu_data, unsigned long id)
cell->loadable = false;
}

- /* present a consistent Communication Region state to the cell */

- cell->comm_page.comm_region.cell_state = JAILHOUSE_CELL_RUNNING;
- cell->comm_page.comm_region.msg_to_cell = JAILHOUSE_MSG_NONE;

+ /* Present a consistent Communication Region state to the cell. Zero the
+ * whole region,+ as it might be dirty. This implies:
+ * - cell_state = JAILHOUSE_CELL_RUNNING (0)
+ * - msg_to_cell = JAILHOUSE_MSG_NONE (0)
+ */

+ comm_region = &cell->comm_page.comm_region;
+ memset(&cell->comm_page, 0, sizeof(cell->comm_page));
+
+ comm_region->revision = COMM_REGION_ABI_REVISION;
+ memcpy(comm_region->signature, COMM_REGION_MAGIC,
+ sizeof(comm_region->signature));
+

pci_cell_reset(cell);
arch_cell_reset(cell);
--
2.17.0

[Jan Kiszka]

Applied to next, with some some massaging (cpu variable had to be moved
as well, comment in hunk above adjusted).

Thanks,
Jan

[Ralf Ramsauer]

Hmm. Must have happened while rebasing, I hit this when compile-time
testing it yesterday. Anyway, thanks.

Ralf

>
> Thanks,
> Jan
>