[PATCH 3/3] arch: general comments in the source code

[Claudio Scordino]

This patch contains a few general comments in the source code of the
arch subsystem.

Signed-off-by: Claudio Scordino <cla...@evidence.eu.com>
---
hypervisor/arch/arm/entry.S | 8 +++++++-
hypervisor/arch/arm/include/asm/percpu.h | 3 +++
hypervisor/arch/x86/apic.c | 30 +++++++++++++++++++++++++++++-
hypervisor/arch/x86/control.c | 13 +++++++++++++
hypervisor/arch/x86/entry.S | 8 +++++++-
hypervisor/arch/x86/include/asm/cell.h | 2 +-
hypervisor/arch/x86/setup.c | 4 ++++
hypervisor/arch/x86/vmx.c | 8 ++++++++
8 files changed, 72 insertions(+), 4 deletions(-)

diff --git a/hypervisor/arch/arm/entry.S b/hypervisor/arch/arm/entry.S
index 8c53edc..c990e0a 100644
--- a/hypervisor/arch/arm/entry.S
+++ b/hypervisor/arch/arm/entry.S
@@ -15,7 +15,13 @@
#include <asm/percpu.h>
#include <asm/processor.h>

-/* Entry point for Linux loader module on JAILHOUSE_ENABLE */
+/**
+ * Entry point for Linux loader module on JAILHOUSE_ENABLE.
+ *
+ * This code locates the per_cpu region for a given cpu_id, stores the Linux
+ * stack pointer and cpu_id in it, sets the Jailhouse stack, and calls the
+ * architecture-independent entry() function, passing it a pointer to cpu_data.
+ */
.text
.globl arch_entry
arch_entry:
diff --git a/hypervisor/arch/arm/include/asm/percpu.h b/hypervisor/arch/arm/include/asm/percpu.h
index 97f902e..4d39ffa 100644
--- a/hypervisor/arch/arm/include/asm/percpu.h
+++ b/hypervisor/arch/arm/include/asm/percpu.h
@@ -31,7 +31,10 @@
(BITS_PER_LONG - __builtin_clzl(sizeof(struct per_cpu) - 1))

struct per_cpu {
+ /** Stack used while in hypervisor mode. */
u8 stack[PAGE_SIZE];
+
+ /** Linux stack pointer, used for handover to hypervisor. */
unsigned long linux_sp;
unsigned long linux_ret;
unsigned long linux_flags;
diff --git a/hypervisor/arch/x86/apic.c b/hypervisor/arch/x86/apic.c
index e37ecda..c1439f0 100644
--- a/hypervisor/arch/x86/apic.c
+++ b/hypervisor/arch/x86/apic.c
@@ -23,7 +23,29 @@

#define XAPIC_REG(x2apic_reg) ((x2apic_reg) << 4)

+ /**
+ * Modern x86 processors are equipped with a "local advanced programmable
+ * interrupt controller" (LAPIC) that handles delivery of inter-processor
+ * interrupts (IPIs) as well as external interrupts that the I/O APIC, which
+ * is part of the system's chipset, generates.
+ *
+ * The LAPIC can work in two modes
+ * - xAPIC: programmed via memory mapped I/O (MMIO)
+ * - x2APIC: programmed throughs model-specific registers (MSRs) and
+ * backward-compatible with xAPIC.
+ *
+ * Currently, Jailhouse virtualizes the LAPIC only, while moderates access to
+ * the I/O APIC.
+ */
+
bool using_x2apic;
+
+/**
+ * Mapping from the apic_id to the cpu_id.
+ * Used to enforce isolation by preventing a CPU from sending an IPI to a CPU
+ * which is not in its own CPU set.
+ * @note See apic_send_ipi()
+ */
u8 apic_to_cpu_id[] = { [0 ... APIC_MAX_PHYS_ID] = CPU_ID_INVALID };

/* Initialized for x2APIC, adjusted for xAPIC during init */
@@ -54,7 +76,7 @@ static struct {
u32 (*read)(unsigned int reg);
u32 (*read_id)(void);
void (*write)(unsigned int reg, u32 val);
- void (*send_ipi)(u32 apic_id, u32 icr_lo);
+ void (*send_ipi)(u32 apic_id, u32 icr_lo); /* send_x*apic_ipi() */
} apic_ops;

static u32 read_xapic(unsigned int reg)
@@ -163,13 +185,16 @@ int apic_init(void)
unsigned long apicbase = read_msr(MSR_IA32_APICBASE);
int err;

+ /* Check if x2APIC mode of LAPIC is enabled: */
if (apicbase & APIC_BASE_EXTD) {
+ /* x2APIC mode: */
apic_ops.read = read_x2apic;
apic_ops.read_id = read_x2apic_id;
apic_ops.write = write_x2apic;
apic_ops.send_ipi = send_x2apic_ipi;
using_x2apic = true;
} else if (apicbase & APIC_BASE_EN) {
+ /* xAPIC mode: */
xapic_page = page_alloc(&remap_pool, 1);
if (!xapic_page)
return trace_error(-ENOMEM);
@@ -359,6 +384,9 @@ static bool apic_valid_ipi_mode(u32 lo_val)
return true;
}

+/**
+ * Send an IPI only if the recipient is in the sender's CPU set.
+ */
static void apic_send_ipi(unsigned int target_cpu_id, u32 orig_icr_hi,
u32 icr_lo)
{
diff --git a/hypervisor/arch/x86/control.c b/hypervisor/arch/x86/control.c
index 46bf2cb..376b8be 100644
--- a/hypervisor/arch/x86/control.c
+++ b/hypervisor/arch/x86/control.c
@@ -148,6 +148,17 @@ void arch_shutdown(void)
ioapic_shutdown();
}

+/**
+ * CPU suspension is used during maintenance operations on one of the
+ * cells. In that case, a specific CPU of the root cell issues a request to
+ * suspend all CPUs of some cell, the root cell or some non-root cell.
+ * Suspension works by sending those CPUs (in case of the root cell, the
+ * caller CPU is excluded) a maintenance signal (NMI on x86, SGI_EVENT on
+ * ARM) and sets a per-CPU flag to trigger the suspension. The target CPU
+ * will leave the guest and handle that request in the event loop
+ * (x86_check_events, check_events). Suspension is a simple busy-wait loop
+ * in hypervisor mode, because it usually doesn't take very long.
+ */
void arch_suspend_cpu(unsigned int cpu_id)
{
struct per_cpu *target_data = per_cpu(cpu_id);
@@ -161,8 +172,10 @@ void arch_suspend_cpu(unsigned int cpu_id)
spin_unlock(&target_data->control_lock);

if (!target_suspended) {
+ /* Signal a NMI to each CPU */
apic_send_nmi_ipi(target_data);

+ /* Wait for target_data->cpu_suspend...*/
while (!target_data->cpu_suspended)
cpu_relax();
}
diff --git a/hypervisor/arch/x86/entry.S b/hypervisor/arch/x86/entry.S
index 986bfee..08a1029 100644
--- a/hypervisor/arch/x86/entry.S
+++ b/hypervisor/arch/x86/entry.S
@@ -12,7 +12,13 @@

#include <asm/asm-defines.h>

-/* Entry point for Linux loader module on JAILHOUSE_ENABLE */
+/**
+ * Entry point for Linux loader module on JAILHOUSE_ENABLE.
+ *
+ * This code locates the per_cpu region for a given cpu_id, stores the Linux
+ * stack pointer and cpu_id in it, sets the Jailhouse stack, and calls the
+ * architecture-independent entry() function, passing it a pointer to cpu_data.
+ */
.text
.globl arch_entry
arch_entry:
diff --git a/hypervisor/arch/x86/include/asm/cell.h b/hypervisor/arch/x86/include/asm/cell.h
index 2223532..ba38121 100644
--- a/hypervisor/arch/x86/include/asm/cell.h
+++ b/hypervisor/arch/x86/include/asm/cell.h
@@ -54,7 +54,7 @@ struct arch_cell {
/** Shadow value of PCI config space address port register. */
u32 pci_addr_port_val;

- /** List of IOAPICs assigned to this cell. */
+ /** List of IOAPICs assigned to this cell (Jailhouse only virtualizes LAPICs). */
struct cell_ioapic *ioapics;
/** Number of assigned IOAPICs. */
unsigned int num_ioapics;
diff --git a/hypervisor/arch/x86/setup.c b/hypervisor/arch/x86/setup.c
index 8234167..0e13c8a 100644
--- a/hypervisor/arch/x86/setup.c
+++ b/hypervisor/arch/x86/setup.c
@@ -50,6 +50,9 @@ static void set_idt_int_gate(unsigned int vector, unsigned long entry)
idt[vector * 4 + 2] = entry >> 32;
}

+/*
+ * Initialize APIC and create Jailhouse's Interrupt Descriptor Table (IDT)
+ */
int arch_init_early(void)
{
unsigned long entry;
@@ -203,6 +206,7 @@ int arch_cpu_init(struct per_cpu *cpu_data)
if (err)
goto error_out;

+ /* Configure Virtual Machine Extensions (VMX) for the CPU */
err = vcpu_init(cpu_data);
if (err)
goto error_out;
diff --git a/hypervisor/arch/x86/vmx.c b/hypervisor/arch/x86/vmx.c
index f832612..f8fa704 100644
--- a/hypervisor/arch/x86/vmx.c
+++ b/hypervisor/arch/x86/vmx.c
@@ -78,7 +78,10 @@ static u8 __attribute__((aligned(PAGE_SIZE))) msr_bitmap[][0x2000/8] = {
[ 0/8 ... 0x1fff/8 ] = 0,
},
};
+
+/* Special access page to trap guest's attempts of accessing LAPIC in xAPIC mode */
static u8 __attribute__((aligned(PAGE_SIZE))) apic_access_page[PAGE_SIZE];
+
static struct paging ept_paging[EPT_PAGE_DIR_LEVELS];
static u32 secondary_exec_addon;
static unsigned long cr_maybe1[2], cr_required1[2];
@@ -339,6 +342,8 @@ int vcpu_vendor_cell_init(struct cell *cell)
cell->arch.vmx.ept_structs.root_table =
(page_table_t)cell->arch.root_table_page;

+ /* Map the special LAPIC access page into the guest's physical address
+ * space at XAPIC_BASE */
err = paging_create(&cell->arch.vmx.ept_structs,
paging_hvirt2phys(apic_access_page),
PAGE_SIZE, XAPIC_BASE,
@@ -511,6 +516,8 @@ static bool vmcs_setup(struct per_cpu *cpu_data)

ok &= vmcs_write64(HOST_RSP, (unsigned long)cpu_data->stack +
sizeof(cpu_data->stack));
+
+ /* Function executed when trapping to the hypervisor */
ok &= vmcs_write64(HOST_RIP, (unsigned long)vmx_vmexit);

ok &= vmx_set_guest_cr(CR0_IDX, cpu_data->linux_cr0);
@@ -672,6 +679,7 @@ int vcpu_init(struct per_cpu *cpu_data)

cpu_data->vmx_state = VMXON;

+ /* Prepare the Virtual Machine Control Structure (VMCS) */
if (!vmcs_clear(cpu_data) ||
!vmcs_load(cpu_data) ||
!vmcs_setup(cpu_data))
--
2.7.4

[Jan Kiszka]

Would it make sense to associate the individual steps with the related
code blocks, ie. move the comments inline?

> + */
> .text
> .globl arch_entry
> arch_entry:
> diff --git a/hypervisor/arch/arm/include/asm/percpu.h b/hypervisor/arch/arm/include/asm/percpu.h
> index 97f902e..4d39ffa 100644
> --- a/hypervisor/arch/arm/include/asm/percpu.h
> +++ b/hypervisor/arch/arm/include/asm/percpu.h
> @@ -31,7 +31,10 @@
> (BITS_PER_LONG - __builtin_clzl(sizeof(struct per_cpu) - 1))
>
> struct per_cpu {
> + /** Stack used while in hypervisor mode. */
> u8 stack[PAGE_SIZE];
> +
> + /** Linux stack pointer, used for handover to hypervisor. */

A start :)

> unsigned long linux_sp;
> unsigned long linux_ret;
> unsigned long linux_flags;
> diff --git a/hypervisor/arch/x86/apic.c b/hypervisor/arch/x86/apic.c
> index e37ecda..c1439f0 100644
> --- a/hypervisor/arch/x86/apic.c
> +++ b/hypervisor/arch/x86/apic.c
> @@ -23,7 +23,29 @@
>
> #define XAPIC_REG(x2apic_reg) ((x2apic_reg) << 4)
>
> + /**
> + * Modern x86 processors are equipped with a "local advanced programmable
> + * interrupt controller" (LAPIC) that handles delivery of inter-processor
> + * interrupts (IPIs) as well as external interrupts that the I/O APIC, which
> + * is part of the system's chipset, generates.

You forgot MSIs in this picture. And interrupt remapping (the x2APIC
only receives external interrupts from there).

> + *
> + * The LAPIC can work in two modes
> + * - xAPIC: programmed via memory mapped I/O (MMIO)
> + * - x2APIC: programmed throughs model-specific registers (MSRs) and
> + * backward-compatible with xAPIC.

The x2APIC is not really backward compatible, see what
x2apic_handle_read has to do e.g.

> + *
> + * Currently, Jailhouse virtualizes the LAPIC only, while moderates access to
> + * the I/O APIC.

Not true. Jailhouse moderates the access to the IOAPIC as well, see
hypervisor/arch/x86/ioapic.c

> + */
> +
> bool using_x2apic;
> +
> +/**
> + * Mapping from the apic_id to the cpu_id.
> + * Used to enforce isolation by preventing a CPU from sending an IPI to a CPU
> + * which is not in its own CPU set.

One step too far: It is used to associate a physical APIC ID with a
logical CPU ID as used by Jailhouse (and Linux). There are multiple use
cases where you need that, one of them being that enforcement, another
the lookup of the target per_cpu structure.

Just noticed that we no longer need to export this variable - vtd.c as a
former external user switched to an encapsulating service. Trivial
cleanup...

> + * @note See apic_send_ipi()
> + */
> u8 apic_to_cpu_id[] = { [0 ... APIC_MAX_PHYS_ID] = CPU_ID_INVALID };
>
> /* Initialized for x2APIC, adjusted for xAPIC during init */
> @@ -54,7 +76,7 @@ static struct {
> u32 (*read)(unsigned int reg);
> u32 (*read_id)(void);
> void (*write)(unsigned int reg, u32 val);
> - void (*send_ipi)(u32 apic_id, u32 icr_lo);
> + void (*send_ipi)(u32 apic_id, u32 icr_lo); /* send_x*apic_ipi() */

That reads more like a personal comment. If documenting this, the
semantics of the callbacks should be described.

> } apic_ops;
>
> static u32 read_xapic(unsigned int reg)
> @@ -163,13 +185,16 @@ int apic_init(void)
> unsigned long apicbase = read_msr(MSR_IA32_APICBASE);
> int err;
>
> + /* Check if x2APIC mode of LAPIC is enabled: */
> if (apicbase & APIC_BASE_EXTD) {
> + /* x2APIC mode: */
> apic_ops.read = read_x2apic;
> apic_ops.read_id = read_x2apic_id;
> apic_ops.write = write_x2apic;
> apic_ops.send_ipi = send_x2apic_ipi;
> using_x2apic = true;
> } else if (apicbase & APIC_BASE_EN) {
> + /* xAPIC mode: */

Fine, maybe without colons.

> xapic_page = page_alloc(&remap_pool, 1);
> if (!xapic_page)
> return trace_error(-ENOMEM);
> @@ -359,6 +384,9 @@ static bool apic_valid_ipi_mode(u32 lo_val)
> return true;
> }
>
> +/**
> + * Send an IPI only if the recipient is in the sender's CPU set.

The function is about sending an IPI. Full stop. That it also checks if
the target ID is allowed for the calling CPU is an additional property,
that is not completely unreadable in the code, a few lines below. But
you may point out that the check is performed like "caller and target
CPU have to be in the same cell". Inline.

> + */
> static void apic_send_ipi(unsigned int target_cpu_id, u32 orig_icr_hi,
> u32 icr_lo)
> {
> diff --git a/hypervisor/arch/x86/control.c b/hypervisor/arch/x86/control.c
> index 46bf2cb..376b8be 100644
> --- a/hypervisor/arch/x86/control.c
> +++ b/hypervisor/arch/x86/control.c
> @@ -148,6 +148,17 @@ void arch_shutdown(void)
> ioapic_shutdown();
> }
>
> +/**
> + * CPU suspension is used during maintenance operations on one of the
> + * cells. In that case, a specific CPU of the root cell issues a request to
> + * suspend all CPUs of some cell, the root cell or some non-root cell.
> + * Suspension works by sending those CPUs (in case of the root cell, the
> + * caller CPU is excluded) a maintenance signal (NMI on x86, SGI_EVENT on
> + * ARM) and sets a per-CPU flag to trigger the suspension. The target CPU
> + * will leave the guest and handle that request in the event loop
> + * (x86_check_events, check_events). Suspension is a simple busy-wait loop
> + * in hypervisor mode, because it usually doesn't take very long.
> + */
> void arch_suspend_cpu(unsigned int cpu_id)

The function is already documented, see jailhouse/control.h. Please
improve the existing documentation if necessary.

> {
> struct per_cpu *target_data = per_cpu(cpu_id);
> @@ -161,8 +172,10 @@ void arch_suspend_cpu(unsigned int cpu_id)
> spin_unlock(&target_data->control_lock);
>
> if (!target_suspended) {
> + /* Signal a NMI to each CPU */

Not to each...

> apic_send_nmi_ipi(target_data);
>
> + /* Wait for target_data->cpu_suspend...*/

Hmm, is the code that unclear (relates to both comments)?

> while (!target_data->cpu_suspended)
> cpu_relax();
> }
> diff --git a/hypervisor/arch/x86/entry.S b/hypervisor/arch/x86/entry.S
> index 986bfee..08a1029 100644
> --- a/hypervisor/arch/x86/entry.S
> +++ b/hypervisor/arch/x86/entry.S
> @@ -12,7 +12,13 @@
>
> #include <asm/asm-defines.h>
>
> -/* Entry point for Linux loader module on JAILHOUSE_ENABLE */
> +/**
> + * Entry point for Linux loader module on JAILHOUSE_ENABLE.
> + *
> + * This code locates the per_cpu region for a given cpu_id, stores the Linux
> + * stack pointer and cpu_id in it, sets the Jailhouse stack, and calls the
> + * architecture-independent entry() function, passing it a pointer to cpu_data.
> + */

Same as for ARM. But if you want to read really interesting code, check
out ARM64. ;)

> .text
> .globl arch_entry
> arch_entry:
> diff --git a/hypervisor/arch/x86/include/asm/cell.h b/hypervisor/arch/x86/include/asm/cell.h
> index 2223532..ba38121 100644
> --- a/hypervisor/arch/x86/include/asm/cell.h
> +++ b/hypervisor/arch/x86/include/asm/cell.h
> @@ -54,7 +54,7 @@ struct arch_cell {
> /** Shadow value of PCI config space address port register. */
> u32 pci_addr_port_val;
>
> - /** List of IOAPICs assigned to this cell. */
> + /** List of IOAPICs assigned to this cell (Jailhouse only virtualizes LAPICs). */

Wrong, see above.

> struct cell_ioapic *ioapics;
> /** Number of assigned IOAPICs. */
> unsigned int num_ioapics;
> diff --git a/hypervisor/arch/x86/setup.c b/hypervisor/arch/x86/setup.c
> index 8234167..0e13c8a 100644
> --- a/hypervisor/arch/x86/setup.c
> +++ b/hypervisor/arch/x86/setup.c
> @@ -50,6 +50,9 @@ static void set_idt_int_gate(unsigned int vector, unsigned long entry)
> idt[vector * 4 + 2] = entry >> 32;
> }
>
> +/*
> + * Initialize APIC and create Jailhouse's Interrupt Descriptor Table (IDT)

It also calls into the vendor-specific init code. Again, such comments
out of line, even that close to the code, can become stale. Better
comment inline if the related code is not obvious.

> + */
> int arch_init_early(void)
> {
> unsigned long entry;
> @@ -203,6 +206,7 @@ int arch_cpu_init(struct per_cpu *cpu_data)
> if (err)
> goto error_out;
>
> + /* Configure Virtual Machine Extensions (VMX) for the CPU */

VMX is for Intel, but we also support AMD (SVM).

Jan

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