Date   

Re: [PATCH v4 2/4] HV: return bool in sanitize_vm_config

Eddie Dong
 

Acked-by: Eddie Dong <eddie.dong@...>

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Victor Sun
Sent: Tuesday, April 9, 2019 9:31 PM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH v4 2/4] HV: return bool in sanitize_vm_config

Return true if vm configs is sanitized successfully, otherwise return false;

Signed-off-by: Victor Sun <victor.sun@...>
---
hypervisor/arch/x86/configs/vm_config.c | 16 ++++++++--------
hypervisor/arch/x86/cpu.c | 2 +-
hypervisor/include/arch/x86/vm_config.h | 2 +-
3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hypervisor/arch/x86/configs/vm_config.c
b/hypervisor/arch/x86/configs/vm_config.c
index 77a96a6..b357e9d 100644
--- a/hypervisor/arch/x86/configs/vm_config.c
+++ b/hypervisor/arch/x86/configs/vm_config.c
@@ -23,9 +23,9 @@ struct acrn_vm_config *get_vm_config(uint16_t vm_id)
/**
* @pre vm_config != NULL
*/
-int32_t sanitize_vm_config(void)
+bool sanitize_vm_config(void)
{
- int32_t ret = 0;
+ bool ret = true;
uint16_t vm_id;
uint64_t sos_pcpu_bitmap, pre_launch_pcpu_bitmap;
struct acrn_vm_config *vm_config;
@@ -43,11 +43,11 @@ int32_t sanitize_vm_config(void)
switch (vm_config->type) {
case PRE_LAUNCHED_VM:
if (vm_config->pcpu_bitmap == 0U) {
- ret = -EINVAL;
+ ret = false;
/* GUEST_FLAG_RT must be set if we have
GUEST_FLAG_LAPIC_PASSTHROUGH set in guest_flags */
} else if (((vm_config->guest_flags &
GUEST_FLAG_LAPIC_PASSTHROUGH) != 0U)
&& ((vm_config->guest_flags & GUEST_FLAG_RT) ==
0U)) {
- ret = -EINVAL;
+ ret = false;
} else {
pre_launch_pcpu_bitmap |= vm_config->pcpu_bitmap;
}
@@ -56,13 +56,13 @@ int32_t sanitize_vm_config(void)
/* Deduct pcpus of PRE_LAUNCHED_VMs */
sos_pcpu_bitmap ^= pre_launch_pcpu_bitmap;
if ((sos_pcpu_bitmap == 0U) || ((vm_config->guest_flags &
GUEST_FLAG_LAPIC_PASSTHROUGH) != 0U)) {
- ret = -EINVAL;
+ ret = false;
} else {
vm_config->pcpu_bitmap = sos_pcpu_bitmap;
}
break;
case NORMAL_VM:
- ret = -EINVAL;
+ ret = false;
break;
default:
/* Nothing to do for a UNDEFINED_VM, break directly. */ @@
-74,11 +74,11 @@ int32_t sanitize_vm_config(void)
cat_cap_info.enabled = true;
} else {
pr_err("%s set wrong CLOS or CAT is not supported\n",
__func__);
- ret = -EINVAL;
+ ret = false;
}
}

- if (ret != 0) {
+ if (!ret) {
break;
}
}
diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c index
1a4966e..d654641 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -198,7 +198,7 @@ void init_cpu_post(uint16_t pcpu_id)
panic("hardware not support!");
}

- if (sanitize_vm_config() != 0) {
+ if (!sanitize_vm_config()) {
panic("VM Configuration Error!");
}

diff --git a/hypervisor/include/arch/x86/vm_config.h
b/hypervisor/include/arch/x86/vm_config.h
index faa8728..d97f74c 100644
--- a/hypervisor/include/arch/x86/vm_config.h
+++ b/hypervisor/include/arch/x86/vm_config.h
@@ -63,7 +63,7 @@ struct acrn_vm_config { } __aligned(8);

struct acrn_vm_config *get_vm_config(uint16_t vm_id); -int32_t
sanitize_vm_config(void);
+bool sanitize_vm_config(void);

extern struct acrn_vm_config vm_configs[CONFIG_MAX_VM_NUM];

--
2.7.4



Re: [PATCH v4 1/4] HV: use term of UUID

Eddie Dong
 

Acked-by: Eddie Dong <eddie.dong@...>

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Victor Sun
Sent: Tuesday, April 9, 2019 9:31 PM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH v4 1/4] HV: use term of UUID

The code mixed the usage on term of UUID and GUID, now use UUID to make
code more consistent, also will use lowercase (i.e. uuid) in variable name
definition.

Signed-off-by: Victor Sun <victor.sun@...>
---
devicemodel/core/vmmapi.c | 2 +-
hypervisor/arch/x86/guest/trusty.c | 2 +-
hypervisor/arch/x86/guest/vm.c | 4 ++--
hypervisor/common/hypercall.c | 2 +-
hypervisor/debug/profiling.c | 4 ++--
hypervisor/include/arch/x86/guest/vm.h | 2 +-
hypervisor/include/arch/x86/vm_config.h | 2 +-
hypervisor/include/debug/profiling_internal.h | 2 +-
hypervisor/include/public/acrn_common.h | 4 ++--
9 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/devicemodel/core/vmmapi.c b/devicemodel/core/vmmapi.c index
202f02f..ebd3e0b 100644
--- a/devicemodel/core/vmmapi.c
+++ b/devicemodel/core/vmmapi.c
@@ -126,7 +126,7 @@ vm_create(const char *name, uint64_t req_buf)
uuid_copy(ctx->vm_uuid, vm_uuid);

/* Pass uuid as parameter of create vm*/
- uuid_copy(create_vm.GUID, vm_uuid);
+ uuid_copy(create_vm.uuid, vm_uuid);

ctx->fd = devfd;
ctx->lowmem_limit = 2 * GB;
diff --git a/hypervisor/arch/x86/guest/trusty.c
b/hypervisor/arch/x86/guest/trusty.c
index 508ce98..9fe38f8 100644
--- a/hypervisor/arch/x86/guest/trusty.c
+++ b/hypervisor/arch/x86/guest/trusty.c
@@ -331,7 +331,7 @@ static bool setup_trusty_info(struct acrn_vcpu *vcpu,
uint32_t mem_size, uint64_
/* Derive dvseed from dseed for Trusty */
if (derive_virtual_seed(&key_info.dseed_list[0U], &key_info.num_seeds,
NULL, 0U,
- vcpu->vm->GUID, sizeof(vcpu->vm->GUID))) {
+ vcpu->vm->uuid, sizeof(vcpu->vm->uuid))) {
/* Derive encryption key of attestation keybox from dseed */
if (derive_attkb_enc_key(key_info.attkb_enc_key)) {
/* Prepare trusty startup param */
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index 09b95e5..c981a3c 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -392,8 +392,8 @@ int32_t create_vm(uint16_t vm_id, struct
acrn_vm_config *vm_config, struct acrn_
snprintf(vm_config->name, 16, "ACRN VM_%d", vm_id);
}

- (void)memcpy_s(&vm->GUID[0], sizeof(vm->GUID),
- &vm_config->GUID[0], sizeof(vm_config->GUID));
+ (void)memcpy_s(&vm->uuid[0], sizeof(vm->uuid),
+ &vm_config->uuid[0], sizeof(vm_config->uuid));

if (vm_config->type == PRE_LAUNCHED_VM) {
create_prelaunched_vm_e820(vm);
diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index 96b2fab..a1ca134 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -134,7 +134,7 @@ int32_t hcall_create_vm(struct acrn_vm *vm,
uint64_t param)
/* TODO: set by DM */
vm_config->type = NORMAL_VM;
vm_config->guest_flags |= cv.vm_flag;
- (void)memcpy_s(&vm_config->GUID[0], 16U, &cv.GUID[0],
16U);
+ (void)memcpy_s(&vm_config->uuid[0], 16U, &cv.uuid[0], 16U);

/* GUEST_FLAG_RT must be set if we have
GUEST_FLAG_LAPIC_PASSTHROUGH set in guest_flags */
if (((vm_config->guest_flags &
GUEST_FLAG_LAPIC_PASSTHROUGH) != 0U) diff --git
a/hypervisor/debug/profiling.c b/hypervisor/debug/profiling.c index
7c5c5c6..5e0600e 100644
--- a/hypervisor/debug/profiling.c
+++ b/hypervisor/debug/profiling.c
@@ -888,8 +888,8 @@ int32_t profiling_vm_list_info(struct acrn_vm *vm,
uint64_t addr)
vm_idx++;

vm_info_list.vm_list[vm_idx].vm_id_num = tmp_vm->vm_id;
- (void)memcpy_s((void *)vm_info_list.vm_list[vm_idx].guid,
- 16U, tmp_vm->GUID, 16U);
+ (void)memcpy_s((void *)vm_info_list.vm_list[vm_idx].uuid,
+ 16U, tmp_vm->uuid, 16U);
snprintf(vm_info_list.vm_list[vm_idx].vm_name, 16U, "vm_%d",
tmp_vm->vm_id, 16U);
vm_info_list.vm_list[vm_idx].num_vcpus = 0; diff --git
a/hypervisor/include/arch/x86/guest/vm.h
b/hypervisor/include/arch/x86/guest/vm.h
index c3d2e83..b1b6437 100644
--- a/hypervisor/include/arch/x86/guest/vm.h
+++ b/hypervisor/include/arch/x86/guest/vm.h
@@ -136,7 +136,7 @@ struct acrn_vm {
struct mem_io_node
emul_mmio[CONFIG_MAX_EMULATED_MMIO_REGIONS];
hv_mem_io_handler_t default_read_write;

- uint8_t GUID[16];
+ uint8_t uuid[16];
struct secure_world_control sworld_control;

/* Secure World's snapshot
diff --git a/hypervisor/include/arch/x86/vm_config.h
b/hypervisor/include/arch/x86/vm_config.h
index 46cf78c..faa8728 100644
--- a/hypervisor/include/arch/x86/vm_config.h
+++ b/hypervisor/include/arch/x86/vm_config.h
@@ -45,7 +45,7 @@ struct acrn_vm_pci_ptdev_config { struct
acrn_vm_config {
enum acrn_vm_type type; /* specify the type of VM */
char name[MAX_VM_OS_NAME_LEN]; /* VM name identifier,
useful for debug. */
- uint8_t GUID[16]; /* GUID of the VM */
+ uint8_t uuid[16]; /* UUID of the VM */
uint64_t pcpu_bitmap; /* from pcpu bitmap, we could
know VM core number */
uint64_t guest_flags; /* VM flags that we want to
configure for guest
* Now we have two flags:
diff --git a/hypervisor/include/debug/profiling_internal.h
b/hypervisor/include/debug/profiling_internal.h
index afbca0c..32daffa 100644
--- a/hypervisor/include/debug/profiling_internal.h
+++ b/hypervisor/include/debug/profiling_internal.h
@@ -111,7 +111,7 @@ struct profiling_vcpu_pcpu_map {

struct profiling_vm_info {
uint16_t vm_id_num;
- uint8_t guid[16];
+ uint8_t uuid[16];
char vm_name[16];
uint16_t num_vcpus;
struct profiling_vcpu_pcpu_map
cpu_map[CONFIG_MAX_VCPUS_PER_VM]; diff --git
a/hypervisor/include/public/acrn_common.h
b/hypervisor/include/public/acrn_common.h
index 1922540..70454be 100644
--- a/hypervisor/include/public/acrn_common.h
+++ b/hypervisor/include/public/acrn_common.h
@@ -351,8 +351,8 @@ struct acrn_create_vm {
/** Reserved */
uint16_t reserved1;

- /** the GUID of this VM */
- uint8_t GUID[16];
+ /** the UUID of this VM */
+ uint8_t uuid[16];

/* VM flag bits from Guest OS, now used
* GUEST_FLAG_SECURE_WORLD_ENABLED (1UL<<0)
--
2.7.4



[PATCH v4 4/4] HV: show VM UUID in shell

Victor Sun
 

Enhance "vm_list" command in shell to Show VM UUID;

Tracked-On: #2291

Signed-off-by: Victor Sun <victor.sun@...>
Acked-by: Eddie Dong <eddie.dong@...>
---
doc/user-guides/acrn-shell.rst | 2 +-
hypervisor/debug/shell.c | 14 ++++++++++----
hypervisor/debug/shell_priv.h | 2 +-
3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/doc/user-guides/acrn-shell.rst b/doc/user-guides/acrn-shell.rst
index 86c7bff..e599e40 100644
--- a/doc/user-guides/acrn-shell.rst
+++ b/doc/user-guides/acrn-shell.rst
@@ -14,7 +14,7 @@ The ACRN hypervisor shell supports the following commands:
* - help
- Display information about supported hypervisor shell commands
* - vm_list
- - List all VMs, displaying the VM name, ID, and state ("Started"=running)
+ - List all VMs, displaying the VM UUID, ID, name, and state ("Started"=running)
* - vcpu_list
- List all vCPUs in all VMs
* - vcpu_dumpreg <vm_id> <vcpu_id>
diff --git a/hypervisor/debug/shell.c b/hypervisor/debug/shell.c
index ef45df3..ae3feaa 100644
--- a/hypervisor/debug/shell.c
+++ b/hypervisor/debug/shell.c
@@ -561,8 +561,8 @@ static int32_t shell_list_vm(__unused int32_t argc, __unused char **argv)
uint16_t vm_id;
char state[32];

- shell_puts("\r\nVM NAME\t\t\t\tVM ID\t\tVM STATE"
- "\r\n=======\t\t\t\t=====\t\t========\r\n");
+ shell_puts("\r\nVM_UUID VM_ID VM_NAME VM_STATE"
+ "\r\n================================ ===== ================================ ========\r\n");

for (vm_id = 0U; vm_id < CONFIG_MAX_VM_NUM; vm_id++) {
vm = get_vm_from_vmid(vm_id);
@@ -581,8 +581,14 @@ static int32_t shell_list_vm(__unused int32_t argc, __unused char **argv)
break;
}
vm_config = get_vm_config(vm_id);
- if (vm_config->type != UNDEFINED_VM) {
- snprintf(temp_str, MAX_STR_SIZE, "%-34s%-14d%-8s\r\n", vm_config->name, vm_id, state);
+ if (is_valid_vm(vm)) {
+ int8_t i;
+
+ for (i = 0; i < 16; i++) {
+ snprintf(temp_str + 2 * i, 3U, "%02x", vm->uuid[i]);
+ }
+ snprintf(temp_str + 32, MAX_STR_SIZE - 32U, " %-3d %-32s %-8s\r\n",
+ vm_id, vm_config->name, state);

/* Output information for this task */
shell_puts(temp_str);
diff --git a/hypervisor/debug/shell_priv.h b/hypervisor/debug/shell_priv.h
index 8bc6370..14ded89 100644
--- a/hypervisor/debug/shell_priv.h
+++ b/hypervisor/debug/shell_priv.h
@@ -40,7 +40,7 @@ struct shell {

#define SHELL_CMD_VM_LIST "vm_list"
#define SHELL_CMD_VM_LIST_PARAM NULL
-#define SHELL_CMD_VM_LIST_HELP "List all VMs, displaying the VM name, ID, and state"
+#define SHELL_CMD_VM_LIST_HELP "List all VMs, displaying the VM UUID, ID, name and state"

#define SHELL_CMD_VCPU_LIST "vcpu_list"
#define SHELL_CMD_VCPU_LIST_PARAM NULL
--
2.7.4


[PATCH v4 3/4] HV: make vm id statically by uuid

Victor Sun
 

Currently VM id of NORMAL_VM is allocated dymatically, we need to make
VM id statically for FuSa compliance.

This patch will pre-configure UUID for all VMs, then NORMAL_VM could
get its VM id/configuration from vm_configs array by indexing the UUID.

If UUID collisions is found in vm configs array, HV will refuse to
load the VM;

Signed-off-by: Victor Sun <victor.sun@...>
---
hypervisor/arch/x86/configs/vm_config.c | 64 +++++++++++++++++++++-
hypervisor/arch/x86/guest/vm.c | 33 +++--------
hypervisor/common/hypercall.c | 9 +--
hypervisor/include/arch/x86/guest/vm.h | 2 +-
hypervisor/include/arch/x86/vm_config.h | 8 ++-
.../logical_partition/vm_configurations.c | 4 ++
hypervisor/scenarios/sdc/vm_configurations.c | 7 +++
7 files changed, 94 insertions(+), 33 deletions(-)

diff --git a/hypervisor/arch/x86/configs/vm_config.c b/hypervisor/arch/x86/configs/vm_config.c
index b357e9d..d854165 100644
--- a/hypervisor/arch/x86/configs/vm_config.c
+++ b/hypervisor/arch/x86/configs/vm_config.c
@@ -21,6 +21,62 @@ struct acrn_vm_config *get_vm_config(uint16_t vm_id)
}

/**
+ * @pre vmid < CONFIG_MAX_VM_NUM
+ */
+bool vm_configured_uuid(uint16_t vmid, uint8_t *vm_uuid)
+{
+ bool ret = false;
+ union acrn_vm_uuid_config *uuid = (union acrn_vm_uuid_config *)vm_uuid;
+ struct acrn_vm_config *vm_config = get_vm_config(vmid);
+
+ if ((uuid->uuid64[0] == vm_config->uuid.uuid64[0])
+ && (uuid->uuid64[1] == vm_config->uuid.uuid64[1])
+ && (vm_config->type != UNDEFINED_VM)) {
+ ret = true;
+ }
+ return ret;
+}
+
+/**
+ * check whether UUID of two VMs in vm configs array is equal
+ *
+ * @pre vm1 < CONFIG_MAX_VM_NUM && vm1 < CONFIG_MAX_VM_NUM && vm1 != vm2
+ */
+bool vm_uuid_is_equal(uint16_t vm1, uint16_t vm2)
+{
+ bool ret = false;
+ struct acrn_vm_config *vm1_config = get_vm_config(vm1);
+ struct acrn_vm_config *vm2_config = get_vm_config(vm2);
+
+ if ((vm1_config->uuid.uuid64[0] == vm2_config->uuid.uuid64[0])
+ && (vm1_config->uuid.uuid64[1] == vm2_config->uuid.uuid64[1])
+ && (vm1_config->type != UNDEFINED_VM)
+ && (vm2_config->type != UNDEFINED_VM)) {
+ ret = true;
+ }
+ return ret;
+}
+
+/**
+ * check whether any UUID collision in vm configs array start from vm_configs[vm_id]
+ *
+ * @pre vm_id < CONFIG_MAX_VM_NUM
+ */
+static bool find_vm_uuid_collision(uint16_t vm_id)
+{
+ uint16_t i;
+ bool ret = false;
+
+ for (i = vm_id + 1U; i < CONFIG_MAX_VM_NUM; i++) {
+ if (vm_uuid_is_equal(vm_id, i)) {
+ ret = true;
+ break;
+ }
+ }
+ return ret;
+}
+
+/**
* @pre vm_config != NULL
*/
bool sanitize_vm_config(void)
@@ -62,7 +118,7 @@ bool sanitize_vm_config(void)
}
break;
case NORMAL_VM:
- ret = false;
+ /* Nothing to do here for a NORMAL_VM, break directly. */
break;
default:
/* Nothing to do for a UNDEFINED_VM, break directly. */
@@ -81,6 +137,12 @@ bool sanitize_vm_config(void)
if (!ret) {
break;
}
+ /* make sure no identical UUID in following VM configurations */
+ if (find_vm_uuid_collision(vm_id)) {
+ ret = false;
+ break;
+ }
+
}
return ret;
}
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index c981a3c..c5ab2f3 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -34,28 +34,17 @@ static struct acrn_vm vm_array[CONFIG_MAX_VM_NUM] __aligned(PAGE_SIZE);

static struct acrn_vm *sos_vm_ptr = NULL;

-uint16_t find_free_vm_id(void)
+uint16_t get_vmid_by_uuid(uint8_t *uuid)
{
- uint16_t id;
- struct acrn_vm_config *vm_config;
+ uint16_t i, vm_id = INVALID_VM_ID;

- for (id = 0U; id < CONFIG_MAX_VM_NUM; id++) {
- vm_config = get_vm_config(id);
- if (vm_config->type == UNDEFINED_VM) {
+ for (i = 0; i < CONFIG_MAX_VM_NUM; i++) {
+ if (vm_configured_uuid(i, uuid)) {
+ vm_id = i;
break;
}
}
- return (vm_config->type == UNDEFINED_VM) ? id : INVALID_VM_ID;
-}
-
-/**
- * @pre vm != NULL && vm->vmid < CONFIG_MAX_VM_NUM
- */
-static inline void free_vm_id(const struct acrn_vm *vm)
-{
- struct acrn_vm_config *vm_config = get_vm_config(vm->vm_id);
-
- vm_config->type = UNDEFINED_VM;
+ return vm_id;
}

bool is_valid_vm(const struct acrn_vm *vm)
@@ -363,6 +352,9 @@ int32_t create_vm(uint16_t vm_id, struct acrn_vm_config *vm_config, struct acrn_
register_mmio_default_emulation_handler(vm);
}

+ (void)memcpy_s(&vm->uuid[0], sizeof(vm->uuid),
+ &vm_config->uuid, sizeof(vm_config->uuid));
+
if (is_sos_vm(vm)) {
/* Only for SOS_VM */
create_sos_vm_e820(vm);
@@ -392,9 +384,6 @@ int32_t create_vm(uint16_t vm_id, struct acrn_vm_config *vm_config, struct acrn_
snprintf(vm_config->name, 16, "ACRN VM_%d", vm_id);
}

- (void)memcpy_s(&vm->uuid[0], sizeof(vm->uuid),
- &vm_config->uuid[0], sizeof(vm_config->uuid));
-
if (vm_config->type == PRE_LAUNCHED_VM) {
create_prelaunched_vm_e820(vm);
prepare_prelaunched_vm_memmap(vm, vm_config);
@@ -469,7 +458,6 @@ int32_t create_vm(uint16_t vm_id, struct acrn_vm_config *vm_config, struct acrn_
if (vm->arch_vm.nworld_eptp != NULL) {
(void)memset(vm->arch_vm.nworld_eptp, 0U, PAGE_SIZE);
}
- free_vm_id(vm);
}
return status;
}
@@ -506,9 +494,6 @@ int32_t shutdown_vm(struct acrn_vm *vm)
/* Free EPT allocated resources assigned to VM */
destroy_ept(vm);

- /* Free vm id */
- free_vm_id(vm);
-
ret = 0;
} else {
ret = -EINVAL;
diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index a1ca134..d446879 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -126,15 +126,10 @@ int32_t hcall_create_vm(struct acrn_vm *vm, uint64_t param)

(void)memset((void *)&cv, 0U, sizeof(cv));
if (copy_from_gpa(vm, &cv, param, sizeof(cv)) == 0) {
- /* check whether there is a free vm id for use */
- /* TODO: pass vm id from DM to make vm_id static */
- vm_id = find_free_vm_id();
+ vm_id = get_vmid_by_uuid(&cv.uuid[0]);
if (vm_id < CONFIG_MAX_VM_NUM) {
vm_config = get_vm_config(vm_id);
- /* TODO: set by DM */
- vm_config->type = NORMAL_VM;
vm_config->guest_flags |= cv.vm_flag;
- (void)memcpy_s(&vm_config->uuid[0], 16U, &cv.uuid[0], 16U);

/* GUEST_FLAG_RT must be set if we have GUEST_FLAG_LAPIC_PASSTHROUGH set in guest_flags */
if (((vm_config->guest_flags & GUEST_FLAG_LAPIC_PASSTHROUGH) != 0U)
@@ -157,6 +152,8 @@ int32_t hcall_create_vm(struct acrn_vm *vm, uint64_t param)
ret = -1;
}
}
+ } else {
+ ret = -ENODEV;
}
} else {
pr_err("%s: Unable copy param to vm\n", __func__);
diff --git a/hypervisor/include/arch/x86/guest/vm.h b/hypervisor/include/arch/x86/guest/vm.h
index b1b6437..e8bb690 100644
--- a/hypervisor/include/arch/x86/guest/vm.h
+++ b/hypervisor/include/arch/x86/guest/vm.h
@@ -212,7 +212,7 @@ void launch_vms(uint16_t pcpu_id);
bool is_valid_vm(const struct acrn_vm *vm);
bool is_sos_vm(const struct acrn_vm *vm);
bool is_prelaunched_vm(const struct acrn_vm *vm);
-uint16_t find_free_vm_id(void);
+uint16_t get_vmid_by_uuid(uint8_t *uuid);
struct acrn_vm *get_vm_from_vmid(uint16_t vm_id);
struct acrn_vm *get_sos_vm(void);

diff --git a/hypervisor/include/arch/x86/vm_config.h b/hypervisor/include/arch/x86/vm_config.h
index d97f74c..15f10a9 100644
--- a/hypervisor/include/arch/x86/vm_config.h
+++ b/hypervisor/include/arch/x86/vm_config.h
@@ -27,6 +27,11 @@ enum acrn_vm_type {
NORMAL_VM /* Post-launched VM */
};

+union acrn_vm_uuid_config {
+ uint8_t uuid8[16];
+ uint64_t uuid64[2];
+} __aligned(8);
+
struct acrn_vm_mem_config {
uint64_t start_hpa; /* the start HPA of VM memory configuration, for pre-launched VMs only */
uint64_t size; /* VM memory size configuration */
@@ -45,7 +50,7 @@ struct acrn_vm_pci_ptdev_config {
struct acrn_vm_config {
enum acrn_vm_type type; /* specify the type of VM */
char name[MAX_VM_OS_NAME_LEN]; /* VM name identifier, useful for debug. */
- uint8_t uuid[16]; /* UUID of the VM */
+ union acrn_vm_uuid_config uuid; /* UUID of the VM */
uint64_t pcpu_bitmap; /* from pcpu bitmap, we could know VM core number */
uint64_t guest_flags; /* VM flags that we want to configure for guest
* Now we have two flags:
@@ -63,6 +68,7 @@ struct acrn_vm_config {
} __aligned(8);

struct acrn_vm_config *get_vm_config(uint16_t vm_id);
+bool vm_configured_uuid(uint16_t vmid, uint8_t *vm_uuid);
bool sanitize_vm_config(void);

extern struct acrn_vm_config vm_configs[CONFIG_MAX_VM_NUM];
diff --git a/hypervisor/scenarios/logical_partition/vm_configurations.c b/hypervisor/scenarios/logical_partition/vm_configurations.c
index 9c81f19..85d5eed 100644
--- a/hypervisor/scenarios/logical_partition/vm_configurations.c
+++ b/hypervisor/scenarios/logical_partition/vm_configurations.c
@@ -15,6 +15,8 @@ struct acrn_vm_config vm_configs[CONFIG_MAX_VM_NUM] = {
{ /* VM0 */
.type = PRE_LAUNCHED_VM,
.name = "ACRN PRE-LAUNCHED VM0",
+ .uuid.uuid64 = {0xd8478a8fd8e0c526UL, 0x5e1ad6eb01f20981UL},
+ /* 26c5e0d8-8f8a-47d8-8109-f201ebd61a5e */
.pcpu_bitmap = VM0_CONFIG_PCPU_BITMAP,
.guest_flags = GUEST_FLAG_IO_COMPLETION_POLLING,
.clos = 0U,
@@ -38,6 +40,8 @@ struct acrn_vm_config vm_configs[CONFIG_MAX_VM_NUM] = {
{ /* VM1 */
.type = PRE_LAUNCHED_VM,
.name = "ACRN PRE-LAUNCHED VM1",
+ .uuid.uuid64 = {0x3d47f96608ce87ddUL, 0x5e937f83057658bcUL},
+ /* dd87ce08-66f9-473d-bc58-7605837f935e */
.pcpu_bitmap = VM1_CONFIG_PCPU_BITMAP,
.guest_flags = GUEST_FLAG_IO_COMPLETION_POLLING,
.clos = 0U,
diff --git a/hypervisor/scenarios/sdc/vm_configurations.c b/hypervisor/scenarios/sdc/vm_configurations.c
index 904070a..a258f38 100644
--- a/hypervisor/scenarios/sdc/vm_configurations.c
+++ b/hypervisor/scenarios/sdc/vm_configurations.c
@@ -12,6 +12,8 @@ struct acrn_vm_config vm_configs[CONFIG_MAX_VM_NUM] = {
{
.type = SOS_VM,
.name = "ACRN SOS VM",
+ .uuid.uuid64 = {0x1642577a34d4bbdbUL, 0x4002abf101222ca1UL},
+ /* dbbbd434-7a57-4216-a12c-2201f1ab0240 */
.guest_flags = GUEST_FLAG_IO_COMPLETION_POLLING,
.clos = 0U,
.memory = {
@@ -22,4 +24,9 @@ struct acrn_vm_config vm_configs[CONFIG_MAX_VM_NUM] = {
.name = "ACRN Service OS",
},
},
+ {
+ .type = NORMAL_VM,
+ .uuid.uuid64 = {0xe811d625385479d2UL, 0x4346b3187acb4e86UL},
+ /* d2795438-25d6-11e8-864e-cb7a18b34643 */
+ }
};
--
2.7.4


[PATCH v4 2/4] HV: return bool in sanitize_vm_config

Victor Sun
 

Return true if vm configs is sanitized successfully, otherwise return false;

Signed-off-by: Victor Sun <victor.sun@...>
---
hypervisor/arch/x86/configs/vm_config.c | 16 ++++++++--------
hypervisor/arch/x86/cpu.c | 2 +-
hypervisor/include/arch/x86/vm_config.h | 2 +-
3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hypervisor/arch/x86/configs/vm_config.c b/hypervisor/arch/x86/configs/vm_config.c
index 77a96a6..b357e9d 100644
--- a/hypervisor/arch/x86/configs/vm_config.c
+++ b/hypervisor/arch/x86/configs/vm_config.c
@@ -23,9 +23,9 @@ struct acrn_vm_config *get_vm_config(uint16_t vm_id)
/**
* @pre vm_config != NULL
*/
-int32_t sanitize_vm_config(void)
+bool sanitize_vm_config(void)
{
- int32_t ret = 0;
+ bool ret = true;
uint16_t vm_id;
uint64_t sos_pcpu_bitmap, pre_launch_pcpu_bitmap;
struct acrn_vm_config *vm_config;
@@ -43,11 +43,11 @@ int32_t sanitize_vm_config(void)
switch (vm_config->type) {
case PRE_LAUNCHED_VM:
if (vm_config->pcpu_bitmap == 0U) {
- ret = -EINVAL;
+ ret = false;
/* GUEST_FLAG_RT must be set if we have GUEST_FLAG_LAPIC_PASSTHROUGH set in guest_flags */
} else if (((vm_config->guest_flags & GUEST_FLAG_LAPIC_PASSTHROUGH) != 0U)
&& ((vm_config->guest_flags & GUEST_FLAG_RT) == 0U)) {
- ret = -EINVAL;
+ ret = false;
} else {
pre_launch_pcpu_bitmap |= vm_config->pcpu_bitmap;
}
@@ -56,13 +56,13 @@ int32_t sanitize_vm_config(void)
/* Deduct pcpus of PRE_LAUNCHED_VMs */
sos_pcpu_bitmap ^= pre_launch_pcpu_bitmap;
if ((sos_pcpu_bitmap == 0U) || ((vm_config->guest_flags & GUEST_FLAG_LAPIC_PASSTHROUGH) != 0U)) {
- ret = -EINVAL;
+ ret = false;
} else {
vm_config->pcpu_bitmap = sos_pcpu_bitmap;
}
break;
case NORMAL_VM:
- ret = -EINVAL;
+ ret = false;
break;
default:
/* Nothing to do for a UNDEFINED_VM, break directly. */
@@ -74,11 +74,11 @@ int32_t sanitize_vm_config(void)
cat_cap_info.enabled = true;
} else {
pr_err("%s set wrong CLOS or CAT is not supported\n", __func__);
- ret = -EINVAL;
+ ret = false;
}
}

- if (ret != 0) {
+ if (!ret) {
break;
}
}
diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e..d654641 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -198,7 +198,7 @@ void init_cpu_post(uint16_t pcpu_id)
panic("hardware not support!");
}

- if (sanitize_vm_config() != 0) {
+ if (!sanitize_vm_config()) {
panic("VM Configuration Error!");
}

diff --git a/hypervisor/include/arch/x86/vm_config.h b/hypervisor/include/arch/x86/vm_config.h
index faa8728..d97f74c 100644
--- a/hypervisor/include/arch/x86/vm_config.h
+++ b/hypervisor/include/arch/x86/vm_config.h
@@ -63,7 +63,7 @@ struct acrn_vm_config {
} __aligned(8);

struct acrn_vm_config *get_vm_config(uint16_t vm_id);
-int32_t sanitize_vm_config(void);
+bool sanitize_vm_config(void);

extern struct acrn_vm_config vm_configs[CONFIG_MAX_VM_NUM];

--
2.7.4


[PATCH v4 1/4] HV: use term of UUID

Victor Sun
 

The code mixed the usage on term of UUID and GUID, now use UUID to make
code more consistent, also will use lowercase (i.e. uuid) in variable name
definition.

Signed-off-by: Victor Sun <victor.sun@...>
---
devicemodel/core/vmmapi.c | 2 +-
hypervisor/arch/x86/guest/trusty.c | 2 +-
hypervisor/arch/x86/guest/vm.c | 4 ++--
hypervisor/common/hypercall.c | 2 +-
hypervisor/debug/profiling.c | 4 ++--
hypervisor/include/arch/x86/guest/vm.h | 2 +-
hypervisor/include/arch/x86/vm_config.h | 2 +-
hypervisor/include/debug/profiling_internal.h | 2 +-
hypervisor/include/public/acrn_common.h | 4 ++--
9 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/devicemodel/core/vmmapi.c b/devicemodel/core/vmmapi.c
index 202f02f..ebd3e0b 100644
--- a/devicemodel/core/vmmapi.c
+++ b/devicemodel/core/vmmapi.c
@@ -126,7 +126,7 @@ vm_create(const char *name, uint64_t req_buf)
uuid_copy(ctx->vm_uuid, vm_uuid);

/* Pass uuid as parameter of create vm*/
- uuid_copy(create_vm.GUID, vm_uuid);
+ uuid_copy(create_vm.uuid, vm_uuid);

ctx->fd = devfd;
ctx->lowmem_limit = 2 * GB;
diff --git a/hypervisor/arch/x86/guest/trusty.c b/hypervisor/arch/x86/guest/trusty.c
index 508ce98..9fe38f8 100644
--- a/hypervisor/arch/x86/guest/trusty.c
+++ b/hypervisor/arch/x86/guest/trusty.c
@@ -331,7 +331,7 @@ static bool setup_trusty_info(struct acrn_vcpu *vcpu, uint32_t mem_size, uint64_
/* Derive dvseed from dseed for Trusty */
if (derive_virtual_seed(&key_info.dseed_list[0U], &key_info.num_seeds,
NULL, 0U,
- vcpu->vm->GUID, sizeof(vcpu->vm->GUID))) {
+ vcpu->vm->uuid, sizeof(vcpu->vm->uuid))) {
/* Derive encryption key of attestation keybox from dseed */
if (derive_attkb_enc_key(key_info.attkb_enc_key)) {
/* Prepare trusty startup param */
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index 09b95e5..c981a3c 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -392,8 +392,8 @@ int32_t create_vm(uint16_t vm_id, struct acrn_vm_config *vm_config, struct acrn_
snprintf(vm_config->name, 16, "ACRN VM_%d", vm_id);
}

- (void)memcpy_s(&vm->GUID[0], sizeof(vm->GUID),
- &vm_config->GUID[0], sizeof(vm_config->GUID));
+ (void)memcpy_s(&vm->uuid[0], sizeof(vm->uuid),
+ &vm_config->uuid[0], sizeof(vm_config->uuid));

if (vm_config->type == PRE_LAUNCHED_VM) {
create_prelaunched_vm_e820(vm);
diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index 96b2fab..a1ca134 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -134,7 +134,7 @@ int32_t hcall_create_vm(struct acrn_vm *vm, uint64_t param)
/* TODO: set by DM */
vm_config->type = NORMAL_VM;
vm_config->guest_flags |= cv.vm_flag;
- (void)memcpy_s(&vm_config->GUID[0], 16U, &cv.GUID[0], 16U);
+ (void)memcpy_s(&vm_config->uuid[0], 16U, &cv.uuid[0], 16U);

/* GUEST_FLAG_RT must be set if we have GUEST_FLAG_LAPIC_PASSTHROUGH set in guest_flags */
if (((vm_config->guest_flags & GUEST_FLAG_LAPIC_PASSTHROUGH) != 0U)
diff --git a/hypervisor/debug/profiling.c b/hypervisor/debug/profiling.c
index 7c5c5c6..5e0600e 100644
--- a/hypervisor/debug/profiling.c
+++ b/hypervisor/debug/profiling.c
@@ -888,8 +888,8 @@ int32_t profiling_vm_list_info(struct acrn_vm *vm, uint64_t addr)
vm_idx++;

vm_info_list.vm_list[vm_idx].vm_id_num = tmp_vm->vm_id;
- (void)memcpy_s((void *)vm_info_list.vm_list[vm_idx].guid,
- 16U, tmp_vm->GUID, 16U);
+ (void)memcpy_s((void *)vm_info_list.vm_list[vm_idx].uuid,
+ 16U, tmp_vm->uuid, 16U);
snprintf(vm_info_list.vm_list[vm_idx].vm_name, 16U, "vm_%d",
tmp_vm->vm_id, 16U);
vm_info_list.vm_list[vm_idx].num_vcpus = 0;
diff --git a/hypervisor/include/arch/x86/guest/vm.h b/hypervisor/include/arch/x86/guest/vm.h
index c3d2e83..b1b6437 100644
--- a/hypervisor/include/arch/x86/guest/vm.h
+++ b/hypervisor/include/arch/x86/guest/vm.h
@@ -136,7 +136,7 @@ struct acrn_vm {
struct mem_io_node emul_mmio[CONFIG_MAX_EMULATED_MMIO_REGIONS];
hv_mem_io_handler_t default_read_write;

- uint8_t GUID[16];
+ uint8_t uuid[16];
struct secure_world_control sworld_control;

/* Secure World's snapshot
diff --git a/hypervisor/include/arch/x86/vm_config.h b/hypervisor/include/arch/x86/vm_config.h
index 46cf78c..faa8728 100644
--- a/hypervisor/include/arch/x86/vm_config.h
+++ b/hypervisor/include/arch/x86/vm_config.h
@@ -45,7 +45,7 @@ struct acrn_vm_pci_ptdev_config {
struct acrn_vm_config {
enum acrn_vm_type type; /* specify the type of VM */
char name[MAX_VM_OS_NAME_LEN]; /* VM name identifier, useful for debug. */
- uint8_t GUID[16]; /* GUID of the VM */
+ uint8_t uuid[16]; /* UUID of the VM */
uint64_t pcpu_bitmap; /* from pcpu bitmap, we could know VM core number */
uint64_t guest_flags; /* VM flags that we want to configure for guest
* Now we have two flags:
diff --git a/hypervisor/include/debug/profiling_internal.h b/hypervisor/include/debug/profiling_internal.h
index afbca0c..32daffa 100644
--- a/hypervisor/include/debug/profiling_internal.h
+++ b/hypervisor/include/debug/profiling_internal.h
@@ -111,7 +111,7 @@ struct profiling_vcpu_pcpu_map {

struct profiling_vm_info {
uint16_t vm_id_num;
- uint8_t guid[16];
+ uint8_t uuid[16];
char vm_name[16];
uint16_t num_vcpus;
struct profiling_vcpu_pcpu_map cpu_map[CONFIG_MAX_VCPUS_PER_VM];
diff --git a/hypervisor/include/public/acrn_common.h b/hypervisor/include/public/acrn_common.h
index 1922540..70454be 100644
--- a/hypervisor/include/public/acrn_common.h
+++ b/hypervisor/include/public/acrn_common.h
@@ -351,8 +351,8 @@ struct acrn_create_vm {
/** Reserved */
uint16_t reserved1;

- /** the GUID of this VM */
- uint8_t GUID[16];
+ /** the UUID of this VM */
+ uint8_t uuid[16];

/* VM flag bits from Guest OS, now used
* GUEST_FLAG_SECURE_WORLD_ENABLED (1UL<<0)
--
2.7.4


Re: [PATCH v2 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Wu, Xiangyang
 

On 2019/4/10 0:44, Kaige Fu wrote:
The physical core of lapic_pt vm should be reset for security and correctness when
shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 2 +-
hypervisor/arch/x86/guest/vm.c | 8 ++++++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e8..b46cae4d 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -259,7 +259,7 @@ static uint16_t get_cpu_id_from_lapic_id(uint32_t lapic_id)
return pcpu_id;
}
-static void start_cpu(uint16_t pcpu_id)
+void start_cpu(uint16_t pcpu_id)
{
uint32_t timeout;
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index cbc70785..4c24f573 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -496,6 +496,14 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and correctness */
+ if (is_lapic_pt(vm)) {
+ make_pcpu_offline(vcpu->pcpu_id);
+ start_cpu(vcpu->pcpu_id);
+ } else {
+ /* No other state currently, do nothing */
+ }
Hi Kaige,


       Can we remove useless "else" here and we can use "if" directly? please double check. BTW, "else if" should be followed by "else". Thanks!


BRs,

Xiangyang Wu


}
ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..5eca144f 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void);
void load_cpu_state_data(void);
void init_cpu_pre(uint16_t pcpu_id_args);
void init_cpu_post(uint16_t pcpu_id);
+void start_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);


Re: [PATCH v2 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Yin, Fengwei <fengwei.yin@...>
 

On 4/10/2019 1:03 AM, Kaige Fu wrote:
On 04-09 Tue 17:02, Yin, Fengwei wrote:
On 4/10/2019 12:50 AM, Kaige Fu wrote:
On 04-09 Tue 16:51, Yin, Fengwei wrote:
On 4/10/2019 12:44 AM, Kaige Fu wrote:
The physical core of lapic_pt vm should be reset for security and correctness when
shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 2 +-
hypervisor/arch/x86/guest/vm.c | 8 ++++++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e8..b46cae4d 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -259,7 +259,7 @@ static uint16_t get_cpu_id_from_lapic_id(uint32_t lapic_id)
return pcpu_id;
}
-static void start_cpu(uint16_t pcpu_id)
+void start_cpu(uint16_t pcpu_id)
{
uint32_t timeout;
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index cbc70785..4c24f573 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -496,6 +496,14 @@ int32_t shutdown_vm(struct acrn_vm *vm)
For lapic_pt vm, this function is called in SOS pCPU (hypercall from
DM). Right?
Yes.
Then, we need to check some flags in HV (like check the guest VM
shutdown flag) instead of invoke make pcpu_offline/start_cpu directly
here. It's to make sure the RTVM is in a state that we could do these
operations for its vcpu. Or we already check the flag in advance?
Actually, we already check it before.
/* Only allow shutdown paused vm */
if (vm->state == VM_PAUSED) {
...
/* All pcpu of lapic_pt vm should be reset for security and correctness */
if (is_lapic_pt(vm)) {
make_pcpu_offline(vcpu->pcpu_id);
start_cpu(vcpu->pcpu_id);
} else {
/* No other state currently, do nothing */
}
}
We can only pause vm (set its state as VM_PAUSED) when the RTVM is trying to poweroff by itself.
So it is safe here to reset the pcpu.
OK. That's fine.

Regards
Yin, Fengwei


Regards
Yin, Fengwei

Regards
Yin, Fengwei

foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and correctness */
+ if (is_lapic_pt(vm)) {
+ make_pcpu_offline(vcpu->pcpu_id);
+ start_cpu(vcpu->pcpu_id);
+ } else {
+ /* No other state currently, do nothing */
+ }
}
ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..5eca144f 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void);
void load_cpu_state_data(void);
void init_cpu_pre(uint16_t pcpu_id_args);
void init_cpu_post(uint16_t pcpu_id);
+void start_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);






Re: [PATCH v2 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Eddie Dong
 

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Kaige Fu
Sent: Wednesday, April 10, 2019 12:45 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH v2 2/2] HV: Reset physical core of lapic_pt vm
when shutdown

The physical core of lapic_pt vm should be reset for security and correctness
when shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 2 +-
hypervisor/arch/x86/guest/vm.c | 8 ++++++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c index
1a4966e8..b46cae4d 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -259,7 +259,7 @@ static uint16_t get_cpu_id_from_lapic_id(uint32_t
lapic_id)
return pcpu_id;
}

-static void start_cpu(uint16_t pcpu_id)
+void start_cpu(uint16_t pcpu_id)
{
uint32_t timeout;

diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index cbc70785..4c24f573 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -496,6 +496,14 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and
correctness */
+ if (is_lapic_pt(vm)) {
+ make_pcpu_offline(vcpu->pcpu_id);
Not sure how you address the following issues:
make_pcpu_offline relies on send_single_ipi(vcpu->pcpu_id, VECTOR_NOTIFY_VCPU), which further relies on the scheduler to tick to execute cpu_dead()... In this case, the caller PCPU needs to wait till the target PCPU is VMX off. I didn't see this waiting mechanism... Did I miss something?


+ start_cpu(vcpu->pcpu_id);
+ } else {
+ /* No other state currently, do nothing */
+ }
}

ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h
b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..5eca144f 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void); void
load_cpu_state_data(void); void init_cpu_pre(uint16_t pcpu_id_args); void
init_cpu_post(uint16_t pcpu_id);
+void start_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);
--
2.20.0



Re: [PATCH v2 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Kaige Fu
 

On 04-09 Tue 17:02, Yin, Fengwei wrote:
On 4/10/2019 12:50 AM, Kaige Fu wrote:
On 04-09 Tue 16:51, Yin, Fengwei wrote:
On 4/10/2019 12:44 AM, Kaige Fu wrote:
The physical core of lapic_pt vm should be reset for security and correctness when
shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 2 +-
hypervisor/arch/x86/guest/vm.c | 8 ++++++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e8..b46cae4d 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -259,7 +259,7 @@ static uint16_t get_cpu_id_from_lapic_id(uint32_t lapic_id)
return pcpu_id;
}
-static void start_cpu(uint16_t pcpu_id)
+void start_cpu(uint16_t pcpu_id)
{
uint32_t timeout;
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index cbc70785..4c24f573 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -496,6 +496,14 @@ int32_t shutdown_vm(struct acrn_vm *vm)
For lapic_pt vm, this function is called in SOS pCPU (hypercall from
DM). Right?
Yes.
Then, we need to check some flags in HV (like check the guest VM
shutdown flag) instead of invoke make pcpu_offline/start_cpu directly
here. It's to make sure the RTVM is in a state that we could do these
operations for its vcpu. Or we already check the flag in advance?
Actually, we already check it before.

/* Only allow shutdown paused vm */
if (vm->state == VM_PAUSED) {

...

/* All pcpu of lapic_pt vm should be reset for security and correctness */
if (is_lapic_pt(vm)) {
make_pcpu_offline(vcpu->pcpu_id);
start_cpu(vcpu->pcpu_id);
} else {
/* No other state currently, do nothing */
}
}


We can only pause vm (set its state as VM_PAUSED) when the RTVM is trying to poweroff by itself.
So it is safe here to reset the pcpu.

Regards
Yin, Fengwei

Regards
Yin, Fengwei

foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and correctness */
+ if (is_lapic_pt(vm)) {
+ make_pcpu_offline(vcpu->pcpu_id);
+ start_cpu(vcpu->pcpu_id);
+ } else {
+ /* No other state currently, do nothing */
+ }
}
ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..5eca144f 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void);
void load_cpu_state_data(void);
void init_cpu_pre(uint16_t pcpu_id_args);
void init_cpu_post(uint16_t pcpu_id);
+void start_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);






Re: [PATCH v2 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Yin, Fengwei <fengwei.yin@...>
 

On 4/10/2019 12:50 AM, Kaige Fu wrote:
On 04-09 Tue 16:51, Yin, Fengwei wrote:
On 4/10/2019 12:44 AM, Kaige Fu wrote:
The physical core of lapic_pt vm should be reset for security and correctness when
shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 2 +-
hypervisor/arch/x86/guest/vm.c | 8 ++++++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e8..b46cae4d 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -259,7 +259,7 @@ static uint16_t get_cpu_id_from_lapic_id(uint32_t lapic_id)
return pcpu_id;
}
-static void start_cpu(uint16_t pcpu_id)
+void start_cpu(uint16_t pcpu_id)
{
uint32_t timeout;
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index cbc70785..4c24f573 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -496,6 +496,14 @@ int32_t shutdown_vm(struct acrn_vm *vm)
For lapic_pt vm, this function is called in SOS pCPU (hypercall from
DM). Right?
Yes.
Then, we need to check some flags in HV (like check the guest VM
shutdown flag) instead of invoke make pcpu_offline/start_cpu directly
here. It's to make sure the RTVM is in a state that we could do these
operations for its vcpu. Or we already check the flag in advance?

Regards
Yin, Fengwei


Regards
Yin, Fengwei

foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and correctness */
+ if (is_lapic_pt(vm)) {
+ make_pcpu_offline(vcpu->pcpu_id);
+ start_cpu(vcpu->pcpu_id);
+ } else {
+ /* No other state currently, do nothing */
+ }
}
ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..5eca144f 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void);
void load_cpu_state_data(void);
void init_cpu_pre(uint16_t pcpu_id_args);
void init_cpu_post(uint16_t pcpu_id);
+void start_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);



Re: [PATCH v2 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Kaige Fu
 

On 04-09 Tue 16:51, Yin, Fengwei wrote:
On 4/10/2019 12:44 AM, Kaige Fu wrote:
The physical core of lapic_pt vm should be reset for security and correctness when
shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 2 +-
hypervisor/arch/x86/guest/vm.c | 8 ++++++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e8..b46cae4d 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -259,7 +259,7 @@ static uint16_t get_cpu_id_from_lapic_id(uint32_t lapic_id)
return pcpu_id;
}
-static void start_cpu(uint16_t pcpu_id)
+void start_cpu(uint16_t pcpu_id)
{
uint32_t timeout;
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index cbc70785..4c24f573 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -496,6 +496,14 @@ int32_t shutdown_vm(struct acrn_vm *vm)
For lapic_pt vm, this function is called in SOS pCPU (hypercall from
DM). Right?
Yes.

Regards
Yin, Fengwei

foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and correctness */
+ if (is_lapic_pt(vm)) {
+ make_pcpu_offline(vcpu->pcpu_id);
+ start_cpu(vcpu->pcpu_id);
+ } else {
+ /* No other state currently, do nothing */
+ }
}
ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..5eca144f 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void);
void load_cpu_state_data(void);
void init_cpu_pre(uint16_t pcpu_id_args);
void init_cpu_post(uint16_t pcpu_id);
+void start_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);



Re: [PATCH v2 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Yin, Fengwei <fengwei.yin@...>
 

On 4/10/2019 12:44 AM, Kaige Fu wrote:
The physical core of lapic_pt vm should be reset for security and correctness when
shutdown the vm.
Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 2 +-
hypervisor/arch/x86/guest/vm.c | 8 ++++++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e8..b46cae4d 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -259,7 +259,7 @@ static uint16_t get_cpu_id_from_lapic_id(uint32_t lapic_id)
return pcpu_id;
}
-static void start_cpu(uint16_t pcpu_id)
+void start_cpu(uint16_t pcpu_id)
{
uint32_t timeout;
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index cbc70785..4c24f573 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -496,6 +496,14 @@ int32_t shutdown_vm(struct acrn_vm *vm)
For lapic_pt vm, this function is called in SOS pCPU (hypercall from
DM). Right?

Regards
Yin, Fengwei

foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and correctness */
+ if (is_lapic_pt(vm)) {
+ make_pcpu_offline(vcpu->pcpu_id);
+ start_cpu(vcpu->pcpu_id);
+ } else {
+ /* No other state currently, do nothing */
+ }
}
ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..5eca144f 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void);
void load_cpu_state_data(void);
void init_cpu_pre(uint16_t pcpu_id_args);
void init_cpu_post(uint16_t pcpu_id);
+void start_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);


[PATCH v2 2/2] HV: Reset physical core of lapic_pt vm when shutdown

Kaige Fu
 

The physical core of lapic_pt vm should be reset for security and correctness when
shutdown the vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/cpu.c | 2 +-
hypervisor/arch/x86/guest/vm.c | 8 ++++++++
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 1a4966e8..b46cae4d 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -259,7 +259,7 @@ static uint16_t get_cpu_id_from_lapic_id(uint32_t lapic_id)
return pcpu_id;
}

-static void start_cpu(uint16_t pcpu_id)
+void start_cpu(uint16_t pcpu_id)
{
uint32_t timeout;

diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index cbc70785..4c24f573 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -496,6 +496,14 @@ int32_t shutdown_vm(struct acrn_vm *vm)
foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
offline_vcpu(vcpu);
+
+ /* All pcpu of lapic_pt vm should be reset for security and correctness */
+ if (is_lapic_pt(vm)) {
+ make_pcpu_offline(vcpu->pcpu_id);
+ start_cpu(vcpu->pcpu_id);
+ } else {
+ /* No other state currently, do nothing */
+ }
}

ptdev_release_all_entries(vm);
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index 13c2fd9e..5eca144f 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -259,6 +259,7 @@ void trampoline_start16(void);
void load_cpu_state_data(void);
void init_cpu_pre(uint16_t pcpu_id_args);
void init_cpu_post(uint16_t pcpu_id);
+void start_cpu(uint16_t pcpu_id);
void start_cpus(void);
void stop_cpus(void);
void wait_sync_change(uint64_t *sync, uint64_t wake_sync);
--
2.20.0


[PATCH v2 1/2] HV: Reset vm and vm_config structure to initial value

Kaige Fu
 

Variables in vm and vm_config may be modified (e.g. vm_config->type, vm_config->guest_flags ...).
So, the vm and vm_config structure should be reset to initial value (zero) when we shutdown the
vm.

Signed-off-by: Kaige Fu <kaige.fu@...>
---
hypervisor/arch/x86/guest/vm.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index 09b95e56..cbc70785 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -51,11 +51,15 @@ uint16_t find_free_vm_id(void)
/**
* @pre vm != NULL && vm->vmid < CONFIG_MAX_VM_NUM
*/
-static inline void free_vm_id(const struct acrn_vm *vm)
+static inline void free_vm_id(struct acrn_vm *vm)
{
struct acrn_vm_config *vm_config = get_vm_config(vm->vm_id);

+ memset(vm_config, 0, sizeof(struct acrn_vm_config));
vm_config->type = UNDEFINED_VM;
+
+ memset(vm, 0, sizeof(struct acrn_vm));
+ vm->state = VM_STATE_INVALID;
}

bool is_valid_vm(const struct acrn_vm *vm)
--
2.20.0


[PATCH v2 0/2] Minor fixes about vm and vm_config and lapic_pt vm

Kaige Fu
 

Patch 1/2 reset vm and vm_config structure to initial value when shutdown the vm.

Patch 2/2 fixes the following issue.
The normal vm can't boot up on the pcpu which ever run lapic_pt vm. The reason
is that the guest will directly write to the passthroughed physical lapic msrs.
The lapic will stay at disabled state or other invalid state. For example, the
lapic will stay at software disabled state when shutdown the linux guest with LAPIC_PT.

So the 2/2 patch reset the physical core of lapic_pt vm when shutdown the vm for security
and correctness. Physical core and lapic will return to initial state after reset.

---
v1 -> v2:
- Reset physical core of lapic_pt vm instead of save/restore lapic msrs.
- Change the cover letter title "Save/restore LAPIC MSRs for LAPIC_PT vm".

Kaige Fu (2):
HV: Reset vm and vm_config structure to initial value
HV: Reset physical core of lapic_pt vm when shutdown

hypervisor/arch/x86/cpu.c | 2 +-
hypervisor/arch/x86/guest/vm.c | 14 +++++++++++++-
hypervisor/include/arch/x86/cpu.h | 1 +
3 files changed, 15 insertions(+), 2 deletions(-)

--
2.20.0


Re: [PATCH 3/3] HV: Save/restore LAPIC MSRs for LAPIC_PT vm

Kaige Fu
 

-----Original Message-----
From: Dong, Eddie
Sent: Tuesday, April 9, 2019 4:04 PM
To: Fu, Kaige <kaige.fu@...>; acrn-dev@...
Cc: Dong, Eddie <eddie.dong@...>
Subject: RE: [acrn-dev] [PATCH 3/3] HV: Save/restore LAPIC MSRs for LAPIC_PT
vm

That is much clean to me.
Use the name "offline_pcpu " to replace "stop_cpu "? or directly use
make_pcpu_offline...
Will use make_pcpu_offline and send the v2 based on the PoC patch.

Thx Eddie

-----Original Message-----
From: Fu, Kaige
Sent: Tuesday, April 9, 2019 3:51 PM
To: Dong, Eddie <eddie.dong@...>; acrn-dev@...
Subject: RE: [acrn-dev] [PATCH 3/3] HV: Save/restore LAPIC MSRs for
LAPIC_PT vm

Hi Eddie,

the attached is PoC patch for reset core/lapic when shuting down lapic_pt
vm.
Please let me know when I can move forward to v2.

--
Thanks
Kaige Fu


-----Original Message-----
From: Dong, Eddie
Sent: Tuesday, April 9, 2019 1:08 PM
To: Fu, Kaige <kaige.fu@...>; acrn-dev@...
Cc: Dong, Eddie <eddie.dong@...>
Subject: RE: [acrn-dev] [PATCH 3/3] HV: Save/restore LAPIC MSRs for
LAPIC_PT vm

The #INIT signal triggers the hardware reset to the core and apic.
Then the action software need to do is to re-program the state
to the HV
default...
Do we need a new API? I thought it exists in current code when
we power
on.
Try to reuse code as more as possible. Check init_lapic()...
The VMX will stay ON in the whole lifecycle of HV. In this
situation, INIT signal will be block per SDM Vol3 23.8:
The INIT signal is blocked whenever a logical processor is in
VMX root operation.
It is not blocked in VMX nonroot operation. Instead, INITs cause
VM exits

In order to unblock the INIT signal, we should call vmx_off before
asserting INIT signal and vmx_on in INIT handler.

Do you think we can do like this way?
I am afraid we must reset the core / lapic for security and
correctness. Turn off vmx is ok to me. Need more comments from
Anthony
and others.
Please try a POC patch first.

Thx eddie


Re: [PATCH 3/3] HV: Save/restore LAPIC MSRs for LAPIC_PT vm

Eddie Dong
 

That is much clean to me.
Use the name "offline_pcpu " to replace "stop_cpu "? or directly use make_pcpu_offline...

Thx Eddie

-----Original Message-----
From: Fu, Kaige
Sent: Tuesday, April 9, 2019 3:51 PM
To: Dong, Eddie <eddie.dong@...>; acrn-dev@...
Subject: RE: [acrn-dev] [PATCH 3/3] HV: Save/restore LAPIC MSRs for LAPIC_PT
vm

Hi Eddie,

the attached is PoC patch for reset core/lapic when shuting down lapic_pt vm.
Please let me know when I can move forward to v2.

--
Thanks
Kaige Fu


-----Original Message-----
From: Dong, Eddie
Sent: Tuesday, April 9, 2019 1:08 PM
To: Fu, Kaige <kaige.fu@...>; acrn-dev@...
Cc: Dong, Eddie <eddie.dong@...>
Subject: RE: [acrn-dev] [PATCH 3/3] HV: Save/restore LAPIC MSRs for
LAPIC_PT vm

The #INIT signal triggers the hardware reset to the core and apic.
Then the action software need to do is to re-program the state to
the HV
default...
Do we need a new API? I thought it exists in current code when we
power
on.
Try to reuse code as more as possible. Check init_lapic()...
The VMX will stay ON in the whole lifecycle of HV. In this
situation, INIT signal will be block per SDM Vol3 23.8:
The INIT signal is blocked whenever a logical processor is in VMX
root operation.
It is not blocked in VMX nonroot operation. Instead, INITs cause
VM exits

In order to unblock the INIT signal, we should call vmx_off before
asserting INIT signal and vmx_on in INIT handler.

Do you think we can do like this way?
I am afraid we must reset the core / lapic for security and
correctness. Turn off vmx is ok to me. Need more comments from Anthony
and others.
Please try a POC patch first.

Thx eddie


Re: [PATCH 1/1] tools:acrn-manager:re-implement function check_dir,modify where they are used

Geoffroy Van Cutsem
 

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Jiang
Sent: Wednesday, April 10, 2019 9:16 AM
To: acrn-dev@...
Cc: Jiang, MaoX <maox.jiang@...>
Subject: [acrn-dev] [PATCH 1/1] tools:acrn-manager:re-implement function
check_dir,modify where they are used

Acrn-manager has several definition of check_dir,no matter how you use
check_dir,it will create derectory if not exist,however,only acrnd and DM
monitor can create the directory,this commit fix 3 points, first reimplement
function check_dir with adding a param flags which can conctrl if create
directory, second delete repeat defination of check_dir in another file,third
move and create path definitions only in one file.
There are quite a number of typos and punctuation oddities in there, can you fix those?


Track-On: #2886
Tracked-On:

Signed-off-by: Mao Jiang <maox.jiang@...>
---
devicemodel/core/monitor.c | 26 ++-----------------
tools/acrn-manager/acrn_mngr.c | 43 +++++++++++++++++++-------------
tools/acrn-manager/acrn_mngr.h | 19 ++++++++++++++
tools/acrn-manager/acrn_vm_ops.c | 26 +++----------------
tools/acrn-manager/acrnctl.h | 6 -----
tools/acrn-manager/acrnd.c | 14 ++++++++++-
6 files changed, 62 insertions(+), 72 deletions(-)

diff --git a/devicemodel/core/monitor.c b/devicemodel/core/monitor.c index
a69486a0..757bbdeb 100644
--- a/devicemodel/core/monitor.c
+++ b/devicemodel/core/monitor.c
@@ -225,28 +225,6 @@ int acrn_parse_intr_monitor(const char *opt)
return 0;
}

-
-/* helpers */
-/* Check if @path is a directory, and create if not exist */ -static int
check_dir(const char *path) -{
- struct stat st;
-
- if (stat(path, &st)) {
- if (mkdir(path, 0666)) {
- perror(path);
- return -1;
- }
- return 0;
- }
-
- if (S_ISDIR(st.st_mode))
- return 0;
-
- fprintf(stderr, "%s exist, and not a directory!\n", path);
- return -1;
-}
-
struct vm_ops {
char name[16];
void *arg;
@@ -426,13 +404,13 @@ int monitor_init(struct vmctx *ctx)
int ret;
char path[128] = {};

- ret = check_dir("/run/acrn/");
+ ret = check_dir(ACRN_DM_BASE_PATH,C_CREAT);
if (ret) {
fprintf(stderr, "%s %d\r\n", __FUNCTION__, __LINE__);
goto dir_err;
}

- ret = check_dir("/run/acrn/mngr");
+ ret = check_dir(ACRN_DM_SOCK_PATH,C_CREAT);
if (ret) {
fprintf(stderr, "%s %d\r\n", __FUNCTION__, __LINE__);
goto dir_err;
diff --git a/tools/acrn-manager/acrn_mngr.c b/tools/acrn-manager/acrn_mngr.c
index fd250414..cc733214 100644
--- a/tools/acrn-manager/acrn_mngr.c
+++ b/tools/acrn-manager/acrn_mngr.c
@@ -18,25 +18,32 @@
#include "acrn_mngr.h"

/* helpers */
-/* Check if @path is a directory, and create if not exist */ -static int
check_dir(const char *path)
+/* Check if @path exists and if is a directory ,if not existence,create
+or warn according to the flag */ int check_dir(const char *path,int
+flags)
{
- struct stat st;
-
- if (stat(path, &st)) {
- if (mkdir(path, 0666)) {
- perror(path);
- return -1;
- }
- return 0;
- }
-
- if (S_ISDIR(st.st_mode))
- return 0;
-
- return -1;
+ struct stat st;
+
+ if (stat(path, &st)) {
+ if (flags) {
+ if (mkdir(path, 0666)) {
+ perror(path);
+ return -1;
+ }
+ return 0;
+ } else {
+ printf("%s doesn't exist!\n",path);
+ return -1;
+ }
+ }
+
+ if (S_ISDIR(st.st_mode))
+ return 0;
+
+ fprintf(stderr, "%s exist, and not a directory!\n", path);
fprintf(stderr, "%s exists, but is not a directory!\n", path);

+ return -1;
}

+
#define MNGR_SOCK_FMT "/run/acrn/mngr/%s.%d.socket"
#define MNGR_MAX_HANDLER 8
#define MNGR_MAX_CLIENT 4
@@ -489,8 +496,8 @@ static void close_client(struct mngr_fd *mfd)

int mngr_open_un(const char *name, int flags) {
- check_dir("/run/acrn");
- check_dir("/run/acrn/mngr");
+ check_dir(ACRN_DM_BASE_PATH,C_ONLY);
+ check_dir(ACRN_DM_SOCK_PATH,C_ONLY);

if (!name) {
printf("%s: No socket name configured\n", __func__); diff --git
a/tools/acrn-manager/acrn_mngr.h b/tools/acrn-manager/acrn_mngr.h index
91cad989..d4fac136 100644
--- a/tools/acrn-manager/acrn_mngr.h
+++ b/tools/acrn-manager/acrn_mngr.h
@@ -12,6 +12,13 @@
#define MNGR_MSG_MAGIC 0x67736d206d6d76 /* that is char[8] "mngr
msg", on X86 */
#define PATH_LEN 128

+#define ACRN_CONF_PATH "/usr/share/acrn/conf"
+#define ACRN_CONF_PATH_ADD ACRN_CONF_PATH "/add"
+#define ACRN_CONF_TIMER_LIST ACRN_CONF_PATH "/timer_list"
+
+#define ACRN_DM_BASE_PATH "/run/acrn"
+#define ACRN_DM_SOCK_PATH "/run/acrn/mngr"
+
struct mngr_msg {
unsigned long long magic; /* Make sure you get a mngr_msg */
unsigned int msgid;
@@ -110,6 +117,9 @@ enum sos_lcs_msgid {
#define MNGR_SERVER 1 /* create a server fd, which you can add
handlers onto it */
#define MNGR_CLIENT 0 /* create a client, just send req and read ack */

+#define C_CREAT 1 /*create a directory,if not exist*/
+#define C_ONLY 0 /*check if the directory exist only*/
+
/**
* @brief create a descripter for vm management IPC
*
@@ -152,4 +162,13 @@ int mngr_add_handler(int desc, unsigned id, int
mngr_send_msg(int desc, struct mngr_msg *req, struct mngr_msg *ack,
unsigned timeout);

+/**
+ *@brief check @path existence and create or report error accoding
+to the flag
I find it easier to read if you also add a <space> in between the asterisk and '@'

* @brief check if @path exists, create it or return an error according to the flag

+ *
+ *@param path: path to check
Add a space in between the asterisk and '@' and remove the colon after the parameter, i.e.
* @param path path to check

+ *@param flags: C_CREAT to create directory,C_ONLY check directory
+only
Add a space in between the asterisk and '@' and remove the colon after the parameter, i.e.
* @param flags C_CREAT to create the directory if it does not exist, C_ONLY only checks if it exists

+ *@return 0 on success,-1 on error
Insert space in between the asterisk and '@'

+ */
+int check_dir(const char *path,int flags);
+
#endif /* ACRN_MANAGER_H */
diff --git a/tools/acrn-manager/acrn_vm_ops.c b/tools/acrn-
manager/acrn_vm_ops.c
index 77661163..7b157664 100644
--- a/tools/acrn-manager/acrn_vm_ops.c
+++ b/tools/acrn-manager/acrn_vm_ops.c
@@ -27,26 +27,6 @@ const char *state_str[] = {
[VM_UNTRACKED] = "untracked",
};

-/* Check if @path is a directory, and create if not exist */ -static int
check_dir(const char *path) -{
- struct stat st;
-
- if (stat(path, &st)) {
- if (mkdir(path, 0666)) {
- perror(path);
- return -1;
- }
- return 0;
- }
-
- if (S_ISDIR(st.st_mode))
- return 0;
-
- fprintf(stderr, "%s exist, and not a directory!\n", path);
- return -1;
-}
-
/* List head of all vm */
static pthread_mutex_t vmmngr_mutex = PTHREAD_MUTEX_INITIALIZER;
struct vmmngr_list_struct vmmngr_head = { NULL }; @@ -131,7 +111,7 @@
static void _scan_alive_vm(void)
int pid;
int ret;

- ret = check_dir(ACRN_DM_SOCK_PATH);
+ ret = check_dir(ACRN_DM_SOCK_PATH,C_ONLY);
if (ret) {
printf("%s: Failed to check directory %s, err: %d\n", __func__,
ACRN_DM_SOCK_PATH, ret);
return;
@@ -228,13 +208,13 @@ static void _scan_added_vm(void)
char suffix[PATH_LEN];
int ret;

- ret = check_dir(ACRN_CONF_PATH);
+ ret = check_dir(ACRN_CONF_PATH,C_ONLY);
if (ret) {
printf("%s: Failed to check directory %s, err: %d\n", __func__,
ACRN_CONF_PATH, ret);
return;
}

- ret = check_dir(ACRN_CONF_PATH_ADD);
+ ret = check_dir(ACRN_CONF_PATH_ADD,C_ONLY);
if (ret) {
printf("%s: Failed to check directory %s, err: %d\n", __func__,
ACRN_CONF_PATH_ADD, ret);
return;
diff --git a/tools/acrn-manager/acrnctl.h b/tools/acrn-manager/acrnctl.h index
bf9591ca..71e73326 100644
--- a/tools/acrn-manager/acrnctl.h
+++ b/tools/acrn-manager/acrnctl.h
@@ -10,12 +10,6 @@
#include <acrn_common.h>
#include "acrn_mngr.h"

-#define ACRN_CONF_PATH "/usr/share/acrn/conf"
-#define ACRN_CONF_PATH_ADD ACRN_CONF_PATH "/add"
-#define ACRN_CONF_TIMER_LIST ACRN_CONF_PATH "/timer_list"
-
-#define ACRN_DM_SOCK_PATH "/run/acrn/mngr"
-
enum vm_state {
VM_STATE_UNKNOWN = 0,
VM_CREATED, /* VM created / awaiting start (boot) */
diff --git a/tools/acrn-manager/acrnd.c b/tools/acrn-manager/acrnd.c index
75bd378b..e9d03080 100644
--- a/tools/acrn-manager/acrnd.c
+++ b/tools/acrn-manager/acrnd.c
@@ -670,7 +670,7 @@ static const char optString[] = "t";

int main(int argc, char *argv[])
{
- int opt;
+ int opt,ret;

while ((opt = getopt(argc, argv, optString)) != -1) {
switch (opt) {
@@ -688,6 +688,18 @@ int main(int argc, char *argv[])
platform_has_hw_ioc = 0;
}

+ ret = check_dir(ACRN_DM_BASE_PATH,C_CREAT);
+ if (ret) {
+ fprintf(stderr, "%s %d\r\n", __FUNCTION__, __LINE__);
+ return -1;
+ }
+
+ ret = check_dir(ACRN_DM_SOCK_PATH,C_CREAT);
+ if (ret) {
+ fprintf(stderr, "%s %d\r\n", __FUNCTION__, __LINE__);
+ return -1;
+ }
+
/* create listening thread */
acrnd_fd = mngr_open_un(ACRND_NAME, MNGR_SERVER);
if (acrnd_fd < 0) {
--
2.17.1



Re: [PATCH 3/3] HV: Save/restore LAPIC MSRs for LAPIC_PT vm

Kaige Fu
 

Hi Eddie,

the attached is PoC patch for reset core/lapic when shuting down lapic_pt vm.
Please let me know when I can move forward to v2.

--
Thanks
Kaige Fu

-----Original Message-----
From: Dong, Eddie
Sent: Tuesday, April 9, 2019 1:08 PM
To: Fu, Kaige <kaige.fu@...>; acrn-dev@...
Cc: Dong, Eddie <eddie.dong@...>
Subject: RE: [acrn-dev] [PATCH 3/3] HV: Save/restore LAPIC MSRs for LAPIC_PT
vm

The #INIT signal triggers the hardware reset to the core and apic.
Then the action software need to do is to re-program the state to
the HV
default...
Do we need a new API? I thought it exists in current code when we
power
on.
Try to reuse code as more as possible. Check init_lapic()...
The VMX will stay ON in the whole lifecycle of HV. In this situation,
INIT signal will be block per SDM Vol3 23.8:
The INIT signal is blocked whenever a logical processor is in VMX
root operation.
It is not blocked in VMX nonroot operation. Instead, INITs cause VM
exits

In order to unblock the INIT signal, we should call vmx_off before
asserting INIT signal and vmx_on in INIT handler.

Do you think we can do like this way?
I am afraid we must reset the core / lapic for security and correctness. Turn off
vmx is ok to me. Need more comments from Anthony and others.
Please try a POC patch first.

Thx eddie

16781 - 16800 of 37344