Date   

Re: [PATCH 6/8] hv: add new function to get gva for MOVS/STO instruction

Xu, Anthony
 

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Yin, Fengwei
Sent: Tuesday, August 14, 2018 6:04 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH 6/8] hv: add new function to get gva for MOVS/STO
instruction

Drop the get_gla function and add
- get_gva_di_si_nocheck
get gva from ES:DI and DS(other segment):SI without
check faulure case
- get_gva_di_si_check
get gva from ES:DI and DS(other segment):SI with failure
case checked

Signed-off-by: Yin Fengwei <fengwei.yin@...>
---
hypervisor/arch/x86/guest/instr_emul.c | 116 ++++++++++++++++---------
1 file changed, 76 insertions(+), 40 deletions(-)

diff --git a/hypervisor/arch/x86/guest/instr_emul.c
b/hypervisor/arch/x86/guest/instr_emul.c
index b2485c48..2012945c 100644
--- a/hypervisor/arch/x86/guest/instr_emul.c
+++ b/hypervisor/arch/x86/guest/instr_emul.c
@@ -470,7 +470,6 @@ static int vie_calculate_gla(enum vm_cpu_mode
cpu_mode, enum cpu_reg_name seg,
uint64_t firstoff, segbase;
uint64_t offset = offset_arg;
uint8_t glasize;
- uint32_t type;

firstoff = offset;
glasize = (cpu_mode == CPU_MODE_64BIT) ? 8U: 4U;
@@ -876,52 +875,97 @@ static int emulate_movx(struct vcpu *vcpu, struct
instr_emul_vie *vie)
return error;
}

+/**
+ * @pre only called by instruction emulation and check was done during
+ * instruction decode
+ *
+ * @remark This function can only be called in instruction emulation and
+ * suppose always success because the check was done during instruction
+ * decode.
What's the benefit?
Why not allow Instruction emulation to inject fault?

vie_calculate_gla is called twice for instruction using SI or DI.



Anthony



+ *
+ * It's only used by MOVS/STO
+ */
+static void get_gva_di_si_nocheck(struct vcpu *vcpu, uint8_t addrsize,
+ enum cpu_reg_name seg, enum cpu_reg_name gpr, uint64_t
*gva)
+{
+ uint64_t val;
+ struct seg_desc desc;
+ enum vm_cpu_mode cpu_mode;
+
+ val = vm_get_register(vcpu, gpr);
+ vm_get_seg_desc(seg, &desc);
+ cpu_mode = get_vcpu_mode(vcpu);
+
+ (void)vie_calculate_gla(cpu_mode, seg, &desc, val, addrsize, gva);
+
+ return;
+}
+
/*
- * Helper function to calculate and validate a linear address.
+ * @pre only called during instruction decode phase
+ *
+ * @remark This function get gva from ES:DI and DS(other segment):SI. And
+ * do check the failure condition and inject exception to guest accordingly.
+ *
+ * It's only used by MOVS/STO
*/
-static int get_gla(struct vcpu *vcpu, __unused struct instr_emul_vie *vie,
- struct vm_guest_paging *paging,
- uint8_t opsize, uint8_t addrsize, uint32_t prot, enum cpu_reg_name
seg,
- enum cpu_reg_name gpr, uint64_t *gla, int *fault)
+static int get_gva_di_si_check(struct vcpu *vcpu, uint8_t addrsize,
+ uint32_t prot, enum cpu_reg_name seg, enum cpu_reg_name
gpr,
+ uint64_t *gva)
{
int ret;
+ uint32_t err_code;
struct seg_desc desc;
- uint64_t cr0, val, rflags;
+ enum vm_cpu_mode cpu_mode;
+ uint64_t val, gpa;

- cr0 = vm_get_register(vcpu, CPU_REG_CR0);
- rflags = vm_get_register(vcpu, CPU_REG_RFLAGS);
val = vm_get_register(vcpu, gpr);
vm_get_seg_desc(seg, &desc);
+ cpu_mode = get_vcpu_mode(vcpu);

- if (vie_calculate_gla(paging->cpu_mode, seg, &desc, val,
- addrsize, gla) != 0) {
- if (seg == CPU_REG_SS) {
- pr_err("TODO: inject ss exception");
- } else {
- pr_err("TODO: inject gp exception");
+ if (!is_desc_valid(&desc, prot)) {
+ goto exception_inject;
+ }
+
+ if (cpu_mode == CPU_MODE_64BIT) {
+ if ((addrsize != 4U) && (addrsize != 8U)) {
+ goto exception_inject;
+ }
+ } else {
+ if ((addrsize != 2U) && (addrsize != 4U)) {
+ goto exception_inject;
}
- goto guest_fault;
}

- if (vie_canonical_check(paging->cpu_mode, *gla) != 0) {
- if (seg == CPU_REG_SS) {
- pr_err("TODO: inject ss exception");
- } else {
- pr_err("TODO: inject gp exception");
+ if (vie_calculate_gla(cpu_mode, seg, &desc, val, addrsize, gva) != 0) {
+ goto exception_inject;
+ }
+
+ if (vie_canonical_check(cpu_mode, *gva) != 0) {
+ goto exception_inject;
+ }
+
+ err_code = (prot == PROT_WRITE) ? PAGE_FAULT_WR_FLAG : 0U;
+ ret = gva2gpa(vcpu, (uint64_t)gva, &gpa, &err_code);
+ if (ret < 0) {
+ if (ret == -EFAULT) {
+ vcpu_inject_pf(vcpu, (uint64_t)gva, err_code);
}
- goto guest_fault;
+ return ret;
}

- *fault = 0;
return 0;

-guest_fault:
- *fault = 1;
- return ret;
+exception_inject:
+ if (seg == CPU_REG_SS) {
+ vcpu_inject_ss(vcpu);
+ } else {
+ vcpu_inject_gp(vcpu, 0U);
+ }
+ return -EFAULT;
}

-static int emulate_movs(struct vcpu *vcpu, struct instr_emul_vie *vie,
- struct vm_guest_paging *paging)
+static int emulate_movs(struct vcpu *vcpu, struct instr_emul_vie *vie)
{
uint64_t dstaddr, srcaddr;
uint64_t rcx, rdi, rsi, rflags;
@@ -955,18 +999,10 @@ static int emulate_movs(struct vcpu *vcpu, struct
instr_emul_vie *vie,
}

seg = (vie->seg_override != 0U) ? (vie->segment_register) :
CPU_REG_DS;
- error = get_gla(vcpu, vie, paging, opsize, vie->addrsize,
- PROT_READ, seg, CPU_REG_RSI, &srcaddr, &fault);
- if ((error != 0) || (fault != 0)) {
- goto done;
- }

- error = get_gla(vcpu, vie, paging, opsize, vie->addrsize,
- PROT_WRITE, CPU_REG_ES, CPU_REG_RDI, &dstaddr,
- &fault);
- if ((error != 0) || (fault != 0)) {
- goto done;
- }
+ get_gva_di_si_nocheck(vcpu, vie->addrsize, seg, CPU_REG_RSI,
&srcaddr);
+ get_gva_di_si_nocheck(vcpu, vie->addrsize, CPU_REG_ES,
CPU_REG_RDI,
+ &dstaddr);

(void)memcpy_s((void *)dstaddr, 16U, (void *)srcaddr, opsize);

@@ -1536,7 +1572,7 @@ static int vmm_emulate_instruction(struct
instr_emul_ctxt *ctxt)
error = emulate_movx(vcpu, vie);
break;
case VIE_OP_TYPE_MOVS:
- error = emulate_movs(vcpu, vie, paging);
+ error = emulate_movs(vcpu, vie);
break;
case VIE_OP_TYPE_STOS:
error = emulate_stos(vcpu, vie);
--
2.17.0



[PATCH v3] HV: Add const qualifiers where required

Arindam Roy <arindam.roy@...>
 

V1:
In order to better comply with MISRA C,
add const qualifiers whereeven required.
In the patch, these are being added to pointers
which are normally used in "get" functions.

V2: Corrected the issues in the patch
pointed by Junjie in his review comments.
Moved the const qualifiers to the correct
places. Removed some changes which are not
needed.

V3: Updated patch comment.
This modifies a subset of all the functions
which might need constant qualifiers
for the their parameters.
This is not and exhaustive patch. This only
targets obvious places where we can use
the const qualifier. More changes will be
submitted in future patches, if required.

Signed-off-by: Arindam Roy <arindam.roy@...>
---
hypervisor/arch/x86/cpu_state_tbl.c | 2 +-
hypervisor/arch/x86/cpuid.c | 6 +++---
hypervisor/arch/x86/ept.c | 14 +++++++-------
hypervisor/arch/x86/guest/guest.c | 10 +++++-----
hypervisor/include/arch/x86/cpuid.h | 2 +-
hypervisor/include/arch/x86/guest/guest.h | 6 +++---
hypervisor/include/arch/x86/io.h | 18 +++++++++---------
hypervisor/include/arch/x86/mmu.h | 22 +++++++++++-----------
hypervisor/include/arch/x86/pgtable.h | 8 ++++----
hypervisor/include/arch/x86/vmx.h | 2 +-
10 files changed, 45 insertions(+), 45 deletions(-)

diff --git a/hypervisor/arch/x86/cpu_state_tbl.c b/hypervisor/arch/x86/cpu_state_tbl.c
index 6f192dad..80093c96 100644
--- a/hypervisor/arch/x86/cpu_state_tbl.c
+++ b/hypervisor/arch/x86/cpu_state_tbl.c
@@ -89,7 +89,7 @@ static const struct cpu_state_table {
}
};

-static int get_state_tbl_idx(char *cpuname)
+static int get_state_tbl_idx(const char *cpuname)
{
int i;
int count = ARRAY_SIZE(cpu_state_tbl);
diff --git a/hypervisor/arch/x86/cpuid.c b/hypervisor/arch/x86/cpuid.c
index 18ebfcc7..215bdcce 100644
--- a/hypervisor/arch/x86/cpuid.c
+++ b/hypervisor/arch/x86/cpuid.c
@@ -8,7 +8,7 @@

extern bool x2apic_enabled;

-static inline struct vcpuid_entry *find_vcpuid_entry(struct vcpu *vcpu,
+static inline struct vcpuid_entry *find_vcpuid_entry(const struct vcpu *vcpu,
uint32_t leaf_arg, uint32_t subleaf)
{
uint32_t i = 0U, nr, half;
@@ -66,7 +66,7 @@ static inline struct vcpuid_entry *find_vcpuid_entry(struct vcpu *vcpu,
}

static inline int set_vcpuid_entry(struct vm *vm,
- struct vcpuid_entry *entry)
+ const struct vcpuid_entry *entry)
{
struct vcpuid_entry *tmp;
size_t entry_size = sizeof(struct vcpuid_entry);
@@ -273,7 +273,7 @@ int set_vcpuid_entries(struct vm *vm)
return 0;
}

-void guest_cpuid(struct vcpu *vcpu,
+void guest_cpuid(const struct vcpu *vcpu,
uint32_t *eax, uint32_t *ebx,
uint32_t *ecx, uint32_t *edx)
{
diff --git a/hypervisor/arch/x86/ept.c b/hypervisor/arch/x86/ept.c
index a7bcb43e..6a327574 100644
--- a/hypervisor/arch/x86/ept.c
+++ b/hypervisor/arch/x86/ept.c
@@ -10,7 +10,7 @@

#define ACRN_DBG_EPT 6U

-static uint64_t find_next_table(uint32_t table_offset, void *table_base)
+static uint64_t find_next_table(uint32_t table_offset, const void *table_base)
{
uint64_t table_entry;
uint64_t table_present;
@@ -100,7 +100,7 @@ void destroy_ept(struct vm *vm)
free_ept_mem(vm->arch_vm.m2p);
}

-uint64_t local_gpa2hpa(struct vm *vm, uint64_t gpa, uint32_t *size)
+uint64_t local_gpa2hpa(const struct vm *vm, uint64_t gpa, uint32_t *size)
{
uint64_t hpa = 0UL;
uint64_t *pgentry, pg_size = 0UL;
@@ -124,12 +124,12 @@ uint64_t local_gpa2hpa(struct vm *vm, uint64_t gpa, uint32_t *size)
}

/* using return value 0 as failure, make sure guest will not use hpa 0 */
-uint64_t gpa2hpa(struct vm *vm, uint64_t gpa)
+uint64_t gpa2hpa(const struct vm *vm, uint64_t gpa)
{
return local_gpa2hpa(vm, gpa, NULL);
}

-uint64_t hpa2gpa(struct vm *vm, uint64_t hpa)
+uint64_t hpa2gpa(const struct vm *vm, uint64_t hpa)
{
uint64_t *pgentry, pg_size = 0UL;

@@ -284,7 +284,7 @@ int ept_misconfig_vmexit_handler(__unused struct vcpu *vcpu)
return status;
}

-int ept_mr_add(struct vm *vm, uint64_t hpa_arg,
+int ept_mr_add(const struct vm *vm, uint64_t hpa_arg,
uint64_t gpa_arg, uint64_t size, uint32_t prot_arg)
{
struct mem_map_params map_params;
@@ -323,7 +323,7 @@ int ept_mr_add(struct vm *vm, uint64_t hpa_arg,
return 0;
}

-int ept_mr_modify(struct vm *vm, uint64_t *pml4_page,
+int ept_mr_modify(const struct vm *vm, uint64_t *pml4_page,
uint64_t gpa, uint64_t size,
uint64_t prot_set, uint64_t prot_clr)
{
@@ -341,7 +341,7 @@ int ept_mr_modify(struct vm *vm, uint64_t *pml4_page,
return ret;
}

-int ept_mr_del(struct vm *vm, uint64_t *pml4_page,
+int ept_mr_del(const struct vm *vm, uint64_t *pml4_page,
uint64_t gpa, uint64_t size)
{
struct vcpu *vcpu;
diff --git a/hypervisor/arch/x86/guest/guest.c b/hypervisor/arch/x86/guest/guest.c
index dfb33bca..47692e30 100644
--- a/hypervisor/arch/x86/guest/guest.c
+++ b/hypervisor/arch/x86/guest/guest.c
@@ -46,7 +46,7 @@ uint64_t vcpumask2pcpumask(struct vm *vm, uint64_t vdmask)
return dmask;
}

-bool vm_lapic_disabled(struct vm *vm)
+bool vm_lapic_disabled(const struct vm *vm)
{
uint16_t i;
struct vcpu *vcpu;
@@ -276,7 +276,7 @@ int gva2gpa(struct vcpu *vcpu, uint64_t gva, uint64_t *gpa,
return ret;
}

-static inline uint32_t local_copy_gpa(struct vm *vm, void *h_ptr, uint64_t gpa,
+static inline uint32_t local_copy_gpa(const struct vm *vm, void *h_ptr, uint64_t gpa,
uint32_t size, uint32_t fix_pg_size, bool cp_from_vm)
{
uint64_t hpa;
@@ -308,7 +308,7 @@ static inline uint32_t local_copy_gpa(struct vm *vm, void *h_ptr, uint64_t gpa,
return len;
}

-static inline int copy_gpa(struct vm *vm, void *h_ptr_arg, uint64_t gpa_arg,
+static inline int copy_gpa(const struct vm *vm, void *h_ptr_arg, uint64_t gpa_arg,
uint32_t size_arg, bool cp_from_vm)
{
void *h_ptr = h_ptr_arg;
@@ -385,12 +385,12 @@ static inline int copy_gva(struct vcpu *vcpu, void *h_ptr_arg, uint64_t gva_arg,
* - some other gpa from hypercall parameters, VHM should make sure it's
* continuous
*/
-int copy_from_gpa(struct vm *vm, void *h_ptr, uint64_t gpa, uint32_t size)
+int copy_from_gpa(const struct vm *vm, void *h_ptr, uint64_t gpa, uint32_t size)
{
return copy_gpa(vm, h_ptr, gpa, size, 1);
}

-int copy_to_gpa(struct vm *vm, void *h_ptr, uint64_t gpa, uint32_t size)
+int copy_to_gpa(const struct vm *vm, void *h_ptr, uint64_t gpa, uint32_t size)
{
return copy_gpa(vm, h_ptr, gpa, size, 0);
}
diff --git a/hypervisor/include/arch/x86/cpuid.h b/hypervisor/include/arch/x86/cpuid.h
index 8fef50fc..35003928 100644
--- a/hypervisor/include/arch/x86/cpuid.h
+++ b/hypervisor/include/arch/x86/cpuid.h
@@ -132,7 +132,7 @@ static inline void cpuid_subleaf(uint32_t leaf, uint32_t subleaf,
}

int set_vcpuid_entries(struct vm *vm);
-void guest_cpuid(struct vcpu *vcpu,
+void guest_cpuid(const struct vcpu *vcpu,
uint32_t *eax, uint32_t *ebx,
uint32_t *ecx, uint32_t *edx);

diff --git a/hypervisor/include/arch/x86/guest/guest.h b/hypervisor/include/arch/x86/guest/guest.h
index 0e291a75..46ef2143 100644
--- a/hypervisor/include/arch/x86/guest/guest.h
+++ b/hypervisor/include/arch/x86/guest/guest.h
@@ -93,7 +93,7 @@ enum vm_paging_mode {
/*
* VM related APIs
*/
-bool vm_lapic_disabled(struct vm *vm);
+bool vm_lapic_disabled(const struct vm *vm);
uint64_t vcpumask2pcpumask(struct vm *vm, uint64_t vdmask);

int gva2gpa(struct vcpu *vcpu, uint64_t gva, uint64_t *gpa, uint32_t *err_code);
@@ -121,8 +121,8 @@ int general_sw_loader(struct vm *vm, struct vcpu *vcpu);
typedef int (*vm_sw_loader_t)(struct vm *vm, struct vcpu *vcpu);
extern vm_sw_loader_t vm_sw_loader;

-int copy_from_gpa(struct vm *vm, void *h_ptr, uint64_t gpa, uint32_t size);
-int copy_to_gpa(struct vm *vm, void *h_ptr, uint64_t gpa, uint32_t size);
+int copy_from_gpa(const struct vm *vm, void *h_ptr, uint64_t gpa, uint32_t size);
+int copy_to_gpa(const struct vm *vm, void *h_ptr, uint64_t gpa, uint32_t size);
int copy_from_gva(struct vcpu *vcpu, void *h_ptr, uint64_t gva,
uint32_t size, uint32_t *err_code, uint64_t *fault_addr);
int copy_to_gva(struct vcpu *vcpu, void *h_ptr, uint64_t gva,
diff --git a/hypervisor/include/arch/x86/io.h b/hypervisor/include/arch/x86/io.h
index e2f4a8ec..e13dcb34 100644
--- a/hypervisor/include/arch/x86/io.h
+++ b/hypervisor/include/arch/x86/io.h
@@ -81,7 +81,7 @@ static inline uint32_t pio_read(uint16_t addr, size_t sz)
* @param value The 32 bit value to write.
* @param addr The memory address to write to.
*/
-static inline void mmio_write32(uint32_t value, void *addr)
+static inline void mmio_write32(uint32_t value, const void *addr)
{
volatile uint32_t *addr32 = (volatile uint32_t *)addr;
*addr32 = value;
@@ -92,7 +92,7 @@ static inline void mmio_write32(uint32_t value, void *addr)
* @param value The 16 bit value to write.
* @param addr The memory address to write to.
*/
-static inline void mmio_write16(uint16_t value, void *addr)
+static inline void mmio_write16(uint16_t value, const void *addr)
{
volatile uint16_t *addr16 = (volatile uint16_t *)addr;
*addr16 = value;
@@ -103,7 +103,7 @@ static inline void mmio_write16(uint16_t value, void *addr)
* @param value The 8 bit value to write.
* @param addr The memory address to write to.
*/
-static inline void mmio_write8(uint8_t value, void *addr)
+static inline void mmio_write8(uint8_t value, const void *addr)
{
volatile uint8_t *addr8 = (volatile uint8_t *)addr;
*addr8 = value;
@@ -115,7 +115,7 @@ static inline void mmio_write8(uint8_t value, void *addr)
*
* @return The 32 bit value read from the given address.
*/
-static inline uint32_t mmio_read32(void *addr)
+static inline uint32_t mmio_read32(const void *addr)
{
return *((volatile uint32_t *)addr);
}
@@ -126,7 +126,7 @@ static inline uint32_t mmio_read32(void *addr)
*
* @return The 16 bit value read from the given address.
*/
-static inline uint16_t mmio_read16(void *addr)
+static inline uint16_t mmio_read16(const void *addr)
{
return *((volatile uint16_t *)addr);
}
@@ -137,7 +137,7 @@ static inline uint16_t mmio_read16(void *addr)
*
* @return The 8 bit value read from the given address.
*/
-static inline uint8_t mmio_read8(void *addr)
+static inline uint8_t mmio_read8(const void *addr)
{
return *((volatile uint8_t *)addr);
}
@@ -150,7 +150,7 @@ static inline uint8_t mmio_read8(void *addr)
* @param mask The mask to apply to the value read.
* @param value The 32 bit value to write.
*/
-static inline void set32(void *addr, uint32_t mask, uint32_t value)
+static inline void set32(const void *addr, uint32_t mask, uint32_t value)
{
uint32_t temp_val;

@@ -165,7 +165,7 @@ static inline void set32(void *addr, uint32_t mask, uint32_t value)
* @param mask The mask to apply to the value read.
* @param value The 16 bit value to write.
*/
-static inline void set16(void *addr, uint16_t mask, uint16_t value)
+static inline void set16(const void *addr, uint16_t mask, uint16_t value)
{
uint16_t temp_val;

@@ -180,7 +180,7 @@ static inline void set16(void *addr, uint16_t mask, uint16_t value)
* @param mask The mask to apply to the value read.
* @param value The 8 bit value to write.
*/
-static inline void set8(void *addr, uint8_t mask, uint8_t value)
+static inline void set8(const void *addr, uint8_t mask, uint8_t value)
{
uint8_t temp_val;

diff --git a/hypervisor/include/arch/x86/mmu.h b/hypervisor/include/arch/x86/mmu.h
index 27699035..9918efe0 100644
--- a/hypervisor/include/arch/x86/mmu.h
+++ b/hypervisor/include/arch/x86/mmu.h
@@ -259,27 +259,27 @@ enum _page_table_present {
#define PAGE_SIZE_1G MEM_1G

/* Inline functions for reading/writing memory */
-static inline uint8_t mem_read8(void *addr)
+static inline uint8_t mem_read8(const void *addr)
{
return *(volatile uint8_t *)(addr);
}

-static inline uint16_t mem_read16(void *addr)
+static inline uint16_t mem_read16(const void *addr)
{
return *(volatile uint16_t *)(addr);
}

-static inline uint32_t mem_read32(void *addr)
+static inline uint32_t mem_read32(const void *addr)
{
return *(volatile uint32_t *)(addr);
}

-static inline uint64_t mem_read64(void *addr)
+static inline uint64_t mem_read64(const void *addr)
{
return *(volatile uint64_t *)(addr);
}

-static inline void mem_write8(void *addr, uint8_t data)
+static inline void mem_write8(const void *addr, uint8_t data)
{
volatile uint8_t *addr8 = (volatile uint8_t *)addr;
*addr8 = data;
@@ -379,15 +379,15 @@ static inline void clflush(volatile void *p)
bool is_ept_supported(void);
uint64_t create_guest_initial_paging(struct vm *vm);
void destroy_ept(struct vm *vm);
-uint64_t gpa2hpa(struct vm *vm, uint64_t gpa);
-uint64_t local_gpa2hpa(struct vm *vm, uint64_t gpa, uint32_t *size);
-uint64_t hpa2gpa(struct vm *vm, uint64_t hpa);
-int ept_mr_add(struct vm *vm, uint64_t hpa_arg,
+uint64_t gpa2hpa(const struct vm *vm, uint64_t gpa);
+uint64_t local_gpa2hpa(const struct vm *vm, uint64_t gpa, uint32_t *size);
+uint64_t hpa2gpa(const struct vm *vm, uint64_t hpa);
+int ept_mr_add(const struct vm *vm, uint64_t hpa_arg,
uint64_t gpa_arg, uint64_t size, uint32_t prot_arg);
-int ept_mr_modify(struct vm *vm, uint64_t *pml4_page,
+int ept_mr_modify(const struct vm *vm, uint64_t *pml4_page,
uint64_t gpa, uint64_t size,
uint64_t prot_set, uint64_t prot_clr);
-int ept_mr_del(struct vm *vm, uint64_t *pml4_page,
+int ept_mr_del(const struct vm *vm, uint64_t *pml4_page,
uint64_t gpa, uint64_t size);
void free_ept_mem(void *pml4_addr);
int ept_violation_vmexit_handler(struct vcpu *vcpu);
diff --git a/hypervisor/include/arch/x86/pgtable.h b/hypervisor/include/arch/x86/pgtable.h
index 51733611..ffa3b179 100644
--- a/hypervisor/include/arch/x86/pgtable.h
+++ b/hypervisor/include/arch/x86/pgtable.h
@@ -53,17 +53,17 @@ static inline uint64_t *pml4e_offset(uint64_t *pml4_page, uint64_t addr)
return pml4_page + pml4e_index(addr);
}

-static inline uint64_t *pdpte_offset(uint64_t *pml4e, uint64_t addr)
+static inline uint64_t *pdpte_offset(const uint64_t *pml4e, uint64_t addr)
{
return pml4e_page_vaddr(*pml4e) + pdpte_index(addr);
}

-static inline uint64_t *pde_offset(uint64_t *pdpte, uint64_t addr)
+static inline uint64_t *pde_offset(const uint64_t *pdpte, uint64_t addr)
{
return pdpte_page_vaddr(*pdpte) + pde_index(addr);
}

-static inline uint64_t *pte_offset(uint64_t *pde, uint64_t addr)
+static inline uint64_t *pte_offset(const uint64_t *pde, uint64_t addr)
{
return pde_page_vaddr(*pde) + pte_index(addr);
}
@@ -71,7 +71,7 @@ static inline uint64_t *pte_offset(uint64_t *pde, uint64_t addr)
/*
* pgentry may means pml4e/pdpte/pde/pte
*/
-static inline uint64_t get_pgentry(uint64_t *pte)
+static inline uint64_t get_pgentry(const uint64_t *pte)
{
return *pte;
}
diff --git a/hypervisor/include/arch/x86/vmx.h b/hypervisor/include/arch/x86/vmx.h
index 938a2017..ede9cf7b 100644
--- a/hypervisor/include/arch/x86/vmx.h
+++ b/hypervisor/include/arch/x86/vmx.h
@@ -449,7 +449,7 @@ int vmx_wrmsr_pat(struct vcpu *vcpu, uint64_t value);
int vmx_write_cr0(struct vcpu *vcpu, uint64_t cr0);
int vmx_write_cr4(struct vcpu *vcpu, uint64_t cr4);

-static inline enum vm_cpu_mode get_vcpu_mode(struct vcpu *vcpu)
+static inline enum vm_cpu_mode get_vcpu_mode(const struct vcpu *vcpu)
{
return vcpu->arch_vcpu.cpu_mode;
}
--
2.17.1


Re: [PATCH 5/8] hv: move check out of vie_calculate_gla

Xu, Anthony
 

Acked-by: Anthony Xu <anthony.xu@...>

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Yin, Fengwei
Sent: Tuesday, August 14, 2018 6:04 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH 5/8] hv: move check out of vie_calculate_gla

We will do check only during instruction decode phase.
vie_calculate_gla will be called both in instruction emulation
phase, we move the check out of vie_calculate_gla.

Signed-off-by: Yin Fengwei <fengwei.yin@...>
---
hypervisor/arch/x86/guest/instr_emul.c | 82 ++++++++++++--------------
1 file changed, 37 insertions(+), 45 deletions(-)

diff --git a/hypervisor/arch/x86/guest/instr_emul.c
b/hypervisor/arch/x86/guest/instr_emul.c
index 65bdecef..b2485c48 100644
--- a/hypervisor/arch/x86/guest/instr_emul.c
+++ b/hypervisor/arch/x86/guest/instr_emul.c
@@ -421,6 +421,40 @@ static int vie_canonical_check(enum vm_cpu_mode
cpu_mode, uint64_t gla)
}
}

+static bool is_desc_valid(struct seg_desc *desc, uint32_t prot)
+{
+ uint32_t type;
+
+ /* The descriptor type must indicate a code/data segment. */
+ type = SEG_DESC_TYPE(desc->access);
+ if (type < 16 || type > 31) {
+ return false;
+ }
+
+ if ((prot & PROT_READ) != 0U) {
+ /* #GP on a read access to a exec-only code segment */
+ if ((type & 0xAU) == 0x8U) {
+ return false;
+ }
+ }
+
+ if ((prot & PROT_WRITE) != 0U) {
+ /*
+ * #GP on a write access to a code segment or a
+ * read-only data segment.
+ */
+ if ((type & 0x8U) != 0U) { /* code segment */
+ return false;
+ }
+
+ if ((type & 0xAU) == 0U) { /* read-only data seg */
+ return false;
+ }
+ }
+
+ return true;
+}
+
/*
*@pre seg must be segment register index
*@pre length_arg must be 1, 2, 4 or 8
@@ -431,7 +465,7 @@ static int vie_canonical_check(enum vm_cpu_mode
cpu_mode, uint64_t gla)
*/
static int vie_calculate_gla(enum vm_cpu_mode cpu_mode, enum
cpu_reg_name seg,
struct seg_desc *desc, uint64_t offset_arg, uint8_t addrsize,
- uint32_t prot, uint64_t *gla)
+ uint64_t *gla)
{
uint64_t firstoff, segbase;
uint64_t offset = offset_arg;
@@ -439,49 +473,7 @@ static int vie_calculate_gla(enum vm_cpu_mode
cpu_mode, enum cpu_reg_name seg,
uint32_t type;

firstoff = offset;
- if (cpu_mode == CPU_MODE_64BIT) {
- if (addrsize != 4U && addrsize != 8U) {
- pr_dbg("%s: invalid addr size %d for cpu mode %d",
- __func__, addrsize, cpu_mode);
- return -1;
- }
- glasize = 8U;
- } else {
- if (addrsize != 2U && addrsize != 4U) {
- pr_dbg("%s: invalid addr size %d for cpu mode %d",
- __func__, addrsize, cpu_mode);
- return -1;
- }
- glasize = 4U;
-
- /* The descriptor type must indicate a code/data segment. */
- type = SEG_DESC_TYPE(desc->access);
- if (type < 16 || type > 31) {
- /*TODO: Inject #GP */
- return -1;
- }
-
- if ((prot & PROT_READ) != 0U) {
- /* #GP on a read access to a exec-only code segment
*/
- if ((type & 0xAU) == 0x8U) {
- return -1;
- }
- }
-
- if ((prot & PROT_WRITE) != 0U) {
- /*
- * #GP on a write access to a code segment or a
- * read-only data segment.
- */
- if ((type & 0x8U) != 0U) { /* code segment */
- return -1;
- }
-
- if ((type & 0xAU) == 0U) { /* read-only data seg
*/
- return -1;
- }
- }
- }
+ glasize = (cpu_mode == CPU_MODE_64BIT) ? 8U: 4U;

/*
* In 64-bit mode all segments except %fs and %gs have a segment
@@ -902,7 +894,7 @@ static int get_gla(struct vcpu *vcpu, __unused struct
instr_emul_vie *vie,
vm_get_seg_desc(seg, &desc);

if (vie_calculate_gla(paging->cpu_mode, seg, &desc, val,
- addrsize, prot, gla) != 0) {
+ addrsize, gla) != 0) {
if (seg == CPU_REG_SS) {
pr_err("TODO: inject ss exception");
} else {
--
2.17.0



Re: [PATCH 2/8] hv: extend the decode_modrm

Xu, Anthony
 

Did you see any OS use [rip] + disp32 to access MMIO?

Can we print error message when it happens?

Anthony

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Yin, Fengwei
Sent: Tuesday, August 14, 2018 6:04 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH 2/8] hv: extend the decode_modrm

If rm show there is no SIB following rm field, we should get
base_register info from rm.
---
hypervisor/arch/x86/guest/instr_emul.c | 42 +++++++++++++++++++++++++-
1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/hypervisor/arch/x86/guest/instr_emul.c
b/hypervisor/arch/x86/guest/instr_emul.c
index 222fa033..d62c6eaa 100644
--- a/hypervisor/arch/x86/guest/instr_emul.c
+++ b/hypervisor/arch/x86/guest/instr_emul.c
@@ -1652,6 +1652,11 @@ static int vie_init(struct instr_emul_vie *vie, struct
vcpu *vcpu)

(void)memset(vie, 0U, sizeof(struct instr_emul_vie));

+ /* init register fields in vie. */
+ vie->base_register = CPU_REG_LAST;
+ vie->index_register = CPU_REG_LAST;
+ vie->segment_register = CPU_REG_LAST;
+
err_code = PAGE_FAULT_ID_FLAG;
ret = copy_from_gva(vcpu, vie->inst, guest_rip_gva,
inst_len, &err_code, &fault_addr);
@@ -1877,6 +1882,41 @@ static int decode_modrm(struct instr_emul_vie
*vie, enum vm_cpu_mode cpu_mode)

vie->reg |= (vie->rex_r << 3);

+ /* SIB */
+ if (vie->mod != VIE_MOD_DIRECT && vie->rm == VIE_RM_SIB) {
+ goto done;
+ }
+
+ vie->base_register = vie->rm;
+
+ switch (vie->mod) {
+ case VIE_MOD_INDIRECT_DISP8:
+ vie->disp_bytes = 1U;
+ break;
+ case VIE_MOD_INDIRECT_DISP32:
+ vie->disp_bytes = 4U;
+ break;
+ case VIE_MOD_INDIRECT:
+ if (vie->rm == VIE_RM_DISP32) {
+ vie->disp_bytes = 4U;
+ /*
+ * Table 2-7. RIP-Relative Addressing
+ *
+ * In 64-bit mode mod=00 r/m=101 implies [rip] + disp32
+ * whereas in compatibility mode it just implies disp32.
+ */
+
+ if (cpu_mode == CPU_MODE_64BIT) {
+ vie->base_register = CPU_REG_RIP;
+ }
+ else {
+ vie->base_register = CPU_REG_LAST;
+ }
+ }
+ break;
+ }
+
+done:
vie_advance(vie);

return 0;
@@ -1953,7 +1993,7 @@ static int decode_sib(struct instr_emul_vie *vie)
}

/* 'scale' makes sense only in the context of an index register */
- if (vie->index_register <= CPU_REG_LAST) {
+ if (vie->index_register < CPU_REG_LAST) {
vie->scale = 1U << vie->ss;
}

--
2.17.0



Re: [PATCH 4/8] hv: remove unnecessary check for gva

Xu, Anthony
 

Acked-by: Anthony Xu <anthony.xu@...>

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Yin, Fengwei
Sent: Tuesday, August 14, 2018 6:04 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH 4/8] hv: remove unnecessary check for gva

According to SDM vol3 25.1.1
With VMX enabled, following exception will be handled by guest
without trigger VM exit:
- faults based on privilege level
- general protection due to relevent segment being unusable
- general protection due to offset beyond limit of relevent segment
- alignment check exception

ACRN always assume VMX is enabled. So we don't need to these check
in instruction emulation. But we need to do page fault related check.

Signed-off-by: Yin Fengwei <fengwei.yin@...>
---
hypervisor/arch/x86/guest/instr_emul.c | 79 +++-----------------------
1 file changed, 7 insertions(+), 72 deletions(-)

diff --git a/hypervisor/arch/x86/guest/instr_emul.c
b/hypervisor/arch/x86/guest/instr_emul.c
index ab9340e1..65bdecef 100644
--- a/hypervisor/arch/x86/guest/instr_emul.c
+++ b/hypervisor/arch/x86/guest/instr_emul.c
@@ -401,18 +401,6 @@ static void get_guest_paging_info(struct vcpu *vcpu,
struct instr_emul_ctxt *emu
emul_ctxt->paging.paging_mode = get_vcpu_paging_mode(vcpu);
}

-static int vie_alignment_check(uint8_t cpl, uint8_t size, uint64_t cr0,
- uint64_t rflags, uint64_t gla)
-{
- pr_dbg("Checking alignment with cpl: %hhu, addrsize: %hhu", cpl,
size);
-
- if (cpl < 3U || (cr0 & CR0_AM) == 0UL || (rflags & PSL_AC) == 0UL) {
- return 0;
- }
-
- return ((gla & (size - 1U)) != 0UL) ? 1 : 0;
-}
-
static int vie_canonical_check(enum vm_cpu_mode cpu_mode, uint64_t gla)
{
uint64_t mask;
@@ -442,12 +430,11 @@ static int vie_canonical_check(enum vm_cpu_mode
cpu_mode, uint64_t gla)
*return -1 - on failure
*/
static int vie_calculate_gla(enum vm_cpu_mode cpu_mode, enum
cpu_reg_name seg,
- struct seg_desc *desc, uint64_t offset_arg, uint8_t length_arg,
- uint8_t addrsize, uint32_t prot, uint64_t *gla)
+ struct seg_desc *desc, uint64_t offset_arg, uint8_t addrsize,
+ uint32_t prot, uint64_t *gla)
{
- uint64_t firstoff, low_limit, high_limit, segbase;
+ uint64_t firstoff, segbase;
uint64_t offset = offset_arg;
- uint8_t length = length_arg;
uint8_t glasize;
uint32_t type;

@@ -466,25 +453,6 @@ static int vie_calculate_gla(enum vm_cpu_mode
cpu_mode, enum cpu_reg_name seg,
return -1;
}
glasize = 4U;
- /*
- * If the segment selector is loaded with a NULL selector
- * then the descriptor is unusable and attempting to use
- * it results in a #GP(0).
- */
- if (SEG_DESC_UNUSABLE(desc->access)) {
- return -1;
- }
-
- /*
- * The processor generates a #NP exception when a segment
- * register is loaded with a selector that points to a
- * descriptor that is not present. If this was the case then
- * it would have been checked before the VM-exit.
- */
- if (SEG_DESC_PRESENT(desc->access) != 0) {
- /* TODO: Inject #NP */
- return -1;
- }

/* The descriptor type must indicate a code/data segment. */
type = SEG_DESC_TYPE(desc->access);
@@ -513,30 +481,6 @@ static int vie_calculate_gla(enum vm_cpu_mode
cpu_mode, enum cpu_reg_name seg,
return -1;
}
}
-
- /*
- * 'desc->limit' is fully expanded taking granularity into
- * account.
- */
- if ((type & 0xCU) == 0x4U) {
- /* expand-down data segment */
- low_limit = desc->limit + 1U;
- high_limit = SEG_DESC_DEF32(desc->access) ?
- 0xffffffffU : 0xffffU;
- } else {
- /* code segment or expand-up data segment */
- low_limit = 0U;
- high_limit = desc->limit;
- }
-
- while (length > 0U) {
- offset &= size2mask[addrsize];
- if (offset < low_limit || offset > high_limit) {
- return -1;
- }
- offset++;
- length--;
- }
}

/*
@@ -948,6 +892,7 @@ static int get_gla(struct vcpu *vcpu, __unused struct
instr_emul_vie *vie,
uint8_t opsize, uint8_t addrsize, uint32_t prot, enum cpu_reg_name
seg,
enum cpu_reg_name gpr, uint64_t *gla, int *fault)
{
+ int ret;
struct seg_desc desc;
uint64_t cr0, val, rflags;

@@ -956,13 +901,11 @@ static int get_gla(struct vcpu *vcpu, __unused struct
instr_emul_vie *vie,
val = vm_get_register(vcpu, gpr);
vm_get_seg_desc(seg, &desc);

- if (vie_calculate_gla(paging->cpu_mode, seg, &desc, val, opsize,
- addrsize, prot, gla) != 0) {
+ if (vie_calculate_gla(paging->cpu_mode, seg, &desc, val,
+ addrsize, prot, gla) != 0) {
if (seg == CPU_REG_SS) {
- /*vm_inject_ss(vcpu, 0);*/
pr_err("TODO: inject ss exception");
} else {
- /*vm_inject_gp(vcpu);*/
pr_err("TODO: inject gp exception");
}
goto guest_fault;
@@ -970,27 +913,19 @@ static int get_gla(struct vcpu *vcpu, __unused struct
instr_emul_vie *vie,

if (vie_canonical_check(paging->cpu_mode, *gla) != 0) {
if (seg == CPU_REG_SS) {
- /*vm_inject_ss(vcpu, 0);*/
pr_err("TODO: inject ss exception");
} else {
- /*vm_inject_gp(vcpu);*/
pr_err("TODO: inject gp exception");
}
goto guest_fault;
}

- if (vie_alignment_check(paging->cpl, opsize, cr0, rflags, *gla) != 0) {
- /*vm_inject_ac(vcpu, 0);*/
- pr_err("TODO: inject ac exception");
- goto guest_fault;
- }
-
*fault = 0;
return 0;

guest_fault:
*fault = 1;
- return 0;
+ return ret;
}

static int emulate_movs(struct vcpu *vcpu, struct instr_emul_vie *vie,
--
2.17.0



Re: [PATCH 1/2] HV: acrntrace: Output event time cost with milliseconds

Eddie Dong
 


On 08-14 Tue 07:52, Eddie Dong wrote:
We should use TSC rather than ms.
OK. Will drop this patch.
You may look at the current code to see if it is using TSC or ms. We should take simple and fast solution.
Same for HV...

Can you have a cleanup?

Thx Eddie


Re: [RFC PATCH] hv: vioapic: simple vioapic set rte

Eddie Dong
 

-----Original Message-----
From: acrn-dev@...
[mailto:acrn-dev@...] On Behalf Of Li, Fei1
Sent: Tuesday, August 14, 2018 10:28 AM
To: acrn-dev@...
Subject: [acrn-dev] [RFC PATCH] hv: vioapic: simple vioapic set rte

1. do nothing if nothing changed.
2. update ptdev native ioapic rte when (a) something changed and
(b) the old rte interrupt mask is not masked or the new rte interrupt mask is
not masked.

Signed-off-by: Li, Fei1 <fei1.li@...>
---
hypervisor/dm/vioapic.c | 28 ++++++++++------------------
1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/hypervisor/dm/vioapic.c b/hypervisor/dm/vioapic.c index
119eabe..7c1d95d 100644
--- a/hypervisor/dm/vioapic.c
+++ b/hypervisor/dm/vioapic.c
@@ -313,12 +313,12 @@ vioapic_indirect_write(struct vioapic *vioapic,
uint32_t addr, uint32_t data)
if ((regnum >= IOAPIC_REDTBL) &&
(regnum < (IOAPIC_REDTBL + (pincount * 2U)))) {
uint32_t addr_offset = regnum - IOAPIC_REDTBL;
- uint32_t rte_offset = addr_offset / 2U;
+ uint32_t rte_offset = addr_offset >> 1U;
pin = rte_offset;

last = vioapic->rtbl[pin];
new = last;
- if ((addr_offset % 2U) != 0U) {
+ if ((addr_offset & 1U) != 0U) {
new.u.hi_32 = data;
} else {
new.u.lo_32 &= RTBL_RO_BITS;
@@ -336,11 +336,13 @@ vioapic_indirect_write(struct vioapic *vioapic,
uint32_t addr, uint32_t data)
}

changed = last.full ^ new.full;
+ if (changed == 0UL) {
+ return;
+ }
/* pin0 from vpic mask/unmask */
if ((pin == 0U) && ((changed & IOAPIC_RTE_INTMASK) != 0UL)) {
/* mask -> umask */
- if (((last.full & IOAPIC_RTE_INTMASK) != 0UL) &&
- ((new.full & IOAPIC_RTE_INTMASK) == 0UL)) {
+ if ((last.full & IOAPIC_RTE_INTMASK) != 0UL) {
if ((vioapic->vm->wire_mode ==
VPIC_WIRE_NULL) ||
(vioapic->vm->wire_mode ==
@@ -354,8 +356,7 @@ vioapic_indirect_write(struct vioapic *vioapic,
uint32_t addr, uint32_t data)
return;
}
/* unmask -> mask */
- } else if (((last.full & IOAPIC_RTE_INTMASK) == 0UL) &&
- ((new.full & IOAPIC_RTE_INTMASK) != 0UL)) {
+ } else {
if (vioapic->vm->wire_mode ==
VPIC_WIRE_IOAPIC) {
vioapic->vm->wire_mode =
@@ -363,9 +364,6 @@ vioapic_indirect_write(struct vioapic *vioapic,
uint32_t addr, uint32_t data)
dev_dbg(ACRN_DBG_IOAPIC,
"vpic wire mode -> INTR");
}
- } else {
- /* Can never happen since IOAPIC_RTE_INTMASK
- * is changed. */
Don't remove this. It is MISRAC requirement.

}
}
vioapic->rtbl[pin] = new;
@@ -405,15 +403,9 @@ vioapic_indirect_write(struct vioapic *vioapic,
uint32_t addr, uint32_t data)
vioapic_send_intr(vioapic, pin);
}

- /* remap for active: interrupt mask -> unmask
- * remap for deactive: interrupt mask & vector set to 0
- * remap for trigger mode change
- * remap for polarity change
- */
- if ( ((changed & IOAPIC_RTE_INTMASK) != 0UL) ||
- ((changed & IOAPIC_RTE_TRGRMOD) != 0UL) ||
- ((changed & IOAPIC_RTE_INTPOL ) != 0UL) ) {
-
+ /* remap for ptdev */
+ if (((new.full & IOAPIC_RTE_INTMASK) == 0UL) ||
+ ((last.full & IOAPIC_RTE_INTMASK) == 0U)) {
0UL not 0U

For this change, can you explain a little?
Why (changed & IOAPIC_RTE_INTMASK) != 0UL) is worse?
And why remove TRIGMOD/INTPOL change condition?

/* VM enable intr */
struct ptdev_intx_info intx;

--
2.7.4



Re: [PATCH 0/8] instruction decoding refine

Eddie Dong
 


In ACRN HV, only LAPIC and IOAPIC virtualization need instruction decoding.
The instructions to access LAPIC and IOAPIC are restrained. Can we only
decode instructions which are used to access LAPIC and IOAPIC and defer
other instruction decoding to SOS?
In this case, the DM needs to be able to inject #PF, #GP and walk guest page table....
And emulate the vCPU register operation.

That needs large change ...


Re: [PATCH v2] hv:fix return value violations for vpic/vioapic

Xu, Anthony
 

Acked-by: Anthony Xu <anthony.xu@...>

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Mingqiang Chi
Sent: Tuesday, August 14, 2018 4:26 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH v2] hv:fix return value violations for vpic/vioapic

From: Mingqiang Chi <mingqiang.chi@...>

-- Change these APIs to void type, add pre-conditions,
and move parameter-check to upper-layer functions.
handle_vpic_irqline
vpic_set_irqstate
vpic_assert_irq
vpic_deassert_irq
vpic_pulse_irq
vpic_get_irq_trigger
handle_vioapic_irqline
vioapic_assert_irq
vioapic_deassert_irq
vioapic_pulse_irq
-- Remove dead code
vpic_set_irq_trigger

Signed-off-by: Mingqiang Chi <mingqiang.chi@...>
---
hypervisor/arch/x86/assign.c | 3 +-
hypervisor/common/hypercall.c | 61 ++++++++++--------
hypervisor/dm/vioapic.c | 40 +++++-------
hypervisor/dm/vpic.c | 99 +++++++----------------------
hypervisor/include/arch/x86/guest/vioapic.h | 6 +-
hypervisor/include/arch/x86/guest/vpic.h | 12 ++--
6 files changed, 83 insertions(+), 138 deletions(-)

diff --git a/hypervisor/arch/x86/assign.c b/hypervisor/arch/x86/assign.c
index 353baa6..4247034 100644
--- a/hypervisor/arch/x86/assign.c
+++ b/hypervisor/arch/x86/assign.c
@@ -896,7 +896,8 @@ int ptdev_add_intx_remapping(struct vm *vm,
{
struct ptdev_remapping_info *entry;

- if (vm == NULL) {
+ if (vm == NULL || (!pic_pin && virt_pin >= vioapic_pincount(vm))
+ || (pic_pin && virt_pin >= vpic_pincount())) {
pr_err("ptdev_add_intx_remapping fails!\n");
return -EINVAL;
}
diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index fd4bc29..289921d 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -68,24 +68,22 @@ int32_t hcall_get_api_version(struct vm *vm, uint64_t
param)
return 0;
}

-static int32_t
+/**
+ * @pre vm != NULL
+ */
+static void
handle_vpic_irqline(struct vm *vm, uint32_t irq, enum irq_mode mode)
{
- int32_t ret = -1;
-
- if (vm == NULL) {
- return ret;
- }

switch (mode) {
case IRQ_ASSERT:
- ret = vpic_assert_irq(vm, irq);
+ vpic_assert_irq(vm, irq);
break;
case IRQ_DEASSERT:
- ret = vpic_deassert_irq(vm, irq);
+ vpic_deassert_irq(vm, irq);
break;
case IRQ_PULSE:
- ret = vpic_pulse_irq(vm, irq);
+ vpic_pulse_irq(vm, irq);
default:
/*
* In this switch statement, mode shall either be IRQ_ASSERT
or
@@ -94,28 +92,23 @@ handle_vpic_irqline(struct vm *vm, uint32_t irq, enum
irq_mode mode)
*/
break;
}
-
- return ret;
}

-static int32_t
+/**
+ * @pre vm != NULL
+ */
+static void
handle_vioapic_irqline(struct vm *vm, uint32_t irq, enum irq_mode mode)
{
- int32_t ret = -1;
-
- if (vm == NULL) {
- return ret;
- }
-
switch (mode) {
case IRQ_ASSERT:
- ret = vioapic_assert_irq(vm, irq);
+ vioapic_assert_irq(vm, irq);
break;
case IRQ_DEASSERT:
- ret = vioapic_deassert_irq(vm, irq);
+ vioapic_deassert_irq(vm, irq);
break;
case IRQ_PULSE:
- ret = vioapic_pulse_irq(vm, irq);
+ vioapic_pulse_irq(vm, irq);
break;
default:
/*
@@ -125,7 +118,6 @@ handle_vioapic_irqline(struct vm *vm, uint32_t irq,
enum irq_mode mode)
*/
break;
}
- return ret;
}

static int32_t
@@ -136,8 +128,21 @@ handle_virt_irqline(struct vm *vm, uint16_t
target_vmid,
uint32_t intr_type;
struct vm *target_vm = get_vm_from_vmid(target_vmid);

- if ((vm == NULL) || (param == NULL)) {
- return -1;
+ if ((vm == NULL) || (param == NULL) || target_vm == NULL) {
+ return -EINVAL;
+ }
+
+ /* Check valid irq */
+ if (param->intr_type == ACRN_INTR_TYPE_IOAPIC
+ && param->ioapic_irq >= vioapic_pincount(vm)) {
+ return -EINVAL;
+ }
+
+ if (intr_type == ACRN_INTR_TYPE_ISA
+ && (param->pic_irq >= vpic_pincount()
+ || (param->ioapic_irq != (~0U)
+ && param->ioapic_irq >= vioapic_pincount(vm)))) {
+ return -EINVAL;
}

intr_type = param->intr_type;
@@ -145,24 +150,24 @@ handle_virt_irqline(struct vm *vm, uint16_t
target_vmid,
switch (intr_type) {
case ACRN_INTR_TYPE_ISA:
/* Call vpic for pic injection */
- ret = handle_vpic_irqline(target_vm, param->pic_irq, mode);
+ handle_vpic_irqline(target_vm, param->pic_irq, mode);

/* call vioapic for ioapic injection if ioapic_irq != ~0U*/
if (param->ioapic_irq != (~0U)) {
/* handle IOAPIC irqline */
- ret = handle_vioapic_irqline(target_vm,
+ handle_vioapic_irqline(target_vm,
param->ioapic_irq, mode);
}
break;
case ACRN_INTR_TYPE_IOAPIC:
/* handle IOAPIC irqline */
- ret = handle_vioapic_irqline(target_vm,
+ handle_vioapic_irqline(target_vm,
param->ioapic_irq, mode);
break;
default:
dev_dbg(ACRN_DBG_HYCALL, "vINTR inject failed. type=%d",
intr_type);
- ret = -1;
+ ret = -EINVAL;
}
return ret;
}
diff --git a/hypervisor/dm/vioapic.c b/hypervisor/dm/vioapic.c
index 119eabe..946a057 100644
--- a/hypervisor/dm/vioapic.c
+++ b/hypervisor/dm/vioapic.c
@@ -67,17 +67,15 @@ vm_ioapic(struct vm *vm)
return (struct vioapic *)vm->arch_vm.virt_ioapic;
}

+/**
+ * @pre pin < vioapic_pincount(vm)
+ */
static void
vioapic_send_intr(struct vioapic *vioapic, uint32_t pin)
{
uint32_t vector, dest, delmode;
union ioapic_rte rte;
bool level, phys;
- uint32_t pincount = vioapic_pincount(vioapic->vm);
-
- if (pin >= pincount) {
- pr_err("vioapic_send_intr: invalid pin number %hhu", pin);
- }

rte = vioapic->rtbl[pin];

@@ -105,16 +103,14 @@ vioapic_send_intr(struct vioapic *vioapic, uint32_t
pin)
delmode, vector,
false);
}

+/**
+ * @pre pin < vioapic_pincount(vm)
+ */
static void
vioapic_set_pinstate(struct vioapic *vioapic, uint32_t pin, bool newstate)
{
int oldcnt, newcnt;
bool needintr;
- uint32_t pincount = vioapic_pincount(vioapic->vm);
-
- if (pin >= pincount) {
- pr_err("vioapic_set_pinstate: invalid pin number %hhu", pin);
- }

oldcnt = vioapic->acnt[pin];
if (newstate) {
@@ -150,16 +146,15 @@ enum irqstate {
IRQSTATE_PULSE
};

-static int
+/**
+ * @pre irq < vioapic_pincount(vm)
+ */
+static void
vioapic_set_irqstate(struct vm *vm, uint32_t irq, enum irqstate irqstate)
{
struct vioapic *vioapic;
uint32_t pin = irq;

- if (pin >= vioapic_pincount(vm)) {
- return -EINVAL;
- }
-
vioapic = vm_ioapic(vm);

VIOAPIC_LOCK(vioapic);
@@ -178,26 +173,23 @@ vioapic_set_irqstate(struct vm *vm, uint32_t irq,
enum irqstate irqstate)
panic("vioapic_set_irqstate: invalid irqstate %d", irqstate);
}
VIOAPIC_UNLOCK(vioapic);
-
- return 0;
}

-int
+void
vioapic_assert_irq(struct vm *vm, uint32_t irq)
{
- return vioapic_set_irqstate(vm, irq, IRQSTATE_ASSERT);
+ vioapic_set_irqstate(vm, irq, IRQSTATE_ASSERT);
}

-int
-vioapic_deassert_irq(struct vm *vm, uint32_t irq)
+void vioapic_deassert_irq(struct vm *vm, uint32_t irq)
{
- return vioapic_set_irqstate(vm, irq, IRQSTATE_DEASSERT);
+ vioapic_set_irqstate(vm, irq, IRQSTATE_DEASSERT);
}

-int
+void
vioapic_pulse_irq(struct vm *vm, uint32_t irq)
{
- return vioapic_set_irqstate(vm, irq, IRQSTATE_PULSE);
+ vioapic_set_irqstate(vm, irq, IRQSTATE_PULSE);
}

/*
diff --git a/hypervisor/dm/vpic.c b/hypervisor/dm/vpic.c
index 266bd53..845b677 100644
--- a/hypervisor/dm/vpic.c
+++ b/hypervisor/dm/vpic.c
@@ -460,15 +460,15 @@ static int vpic_ocw3(struct acrn_vpic *vpic, struct
i8259_reg_state *i8259, uint
return 0;
}

+/**
+ * @pre pin < NR_VPIC_PINS_TOTAL
+ */
static void vpic_set_pinstate(struct acrn_vpic *vpic, uint8_t pin, bool
newstate)
{
struct i8259_reg_state *i8259;
int oldcnt, newcnt;
bool level;

- ASSERT(pin < NR_VPIC_PINS_TOTAL,
- "vpic_set_pinstate: invalid pin number");
-
i8259 = &vpic->i8259[pin >> 3U];

oldcnt = i8259->acnt[pin & 0x7U];
@@ -504,22 +504,22 @@ static void vpic_set_pinstate(struct acrn_vpic *vpic,
uint8_t pin, bool newstate
vpic_notify_intr(vpic);
}

-static int vpic_set_irqstate(struct vm *vm, uint32_t irq, enum irqstate
irqstate)
+/**
+ * @pre irq < NR_VPIC_PINS_TOTAL
+ */
+static void vpic_set_irqstate(struct vm *vm, uint32_t irq,
+ enum irqstate irqstate)
{
struct acrn_vpic *vpic;
struct i8259_reg_state *i8259;
uint8_t pin;

- if (irq >= NR_VPIC_PINS_TOTAL) {
- return -EINVAL;
- }
-
vpic = vm_pic(vm);
i8259 = &vpic->i8259[irq >> 3U];
pin = (uint8_t)irq;

if (i8259->ready == false) {
- return 0;
+ return;
}

VPIC_LOCK(vpic);
@@ -538,97 +538,46 @@ static int vpic_set_irqstate(struct vm *vm, uint32_t
irq, enum irqstate irqstate
ASSERT(false, "vpic_set_irqstate: invalid irqstate");
}
VPIC_UNLOCK(vpic);
-
- return 0;
}

/* hypervisor interface: assert/deassert/pulse irq */
-int vpic_assert_irq(struct vm *vm, uint32_t irq)
+void vpic_assert_irq(struct vm *vm, uint32_t irq)
{
- return vpic_set_irqstate(vm, irq, IRQSTATE_ASSERT);
+ vpic_set_irqstate(vm, irq, IRQSTATE_ASSERT);
}

-int vpic_deassert_irq(struct vm *vm, uint32_t irq)
+void vpic_deassert_irq(struct vm *vm, uint32_t irq)
{
- return vpic_set_irqstate(vm, irq, IRQSTATE_DEASSERT);
+ vpic_set_irqstate(vm, irq, IRQSTATE_DEASSERT);
}

-int vpic_pulse_irq(struct vm *vm, uint32_t irq)
+void vpic_pulse_irq(struct vm *vm, uint32_t irq)
{
- return vpic_set_irqstate(vm, irq, IRQSTATE_PULSE);
+ vpic_set_irqstate(vm, irq, IRQSTATE_PULSE);
}

-int vpic_set_irq_trigger(struct vm *vm, uint32_t irq, enum vpic_trigger trigger)
+uint32_t
+vpic_pincount(void)
{
- struct acrn_vpic *vpic;
- uint8_t pin_mask;
-
- if (irq >= NR_VPIC_PINS_TOTAL) {
- return -EINVAL;
- }
-
- /*
- * See comment in vpic_elc_handler. These IRQs must be
- * edge triggered.
- */
- if (trigger == LEVEL_TRIGGER) {
- switch (irq) {
- case 0U:
- case 1U:
- case 2U:
- case 8U:
- case 13U:
- return -EINVAL;
- default:
- /*
- * The IRQs handled earlier are the ones that could
only
- * support edge trigger, while the input parameter
- * 'trigger' is set as LEVEL_TRIGGER. So, an error code
- * (-EINVAL) shall be returned due to the invalid
- * operation.
- *
- * All the other IRQs will be handled properly after
- * this switch statement.
- */
- break;
- }
- }
-
- vpic = vm_pic(vm);
- pin_mask = (uint8_t)(1U << (irq & 0x7U));
-
- VPIC_LOCK(vpic);
-
- if (trigger == LEVEL_TRIGGER) {
- vpic->i8259[irq >> 3U].elc |= pin_mask;
- } else {
- vpic->i8259[irq >> 3U].elc &= ~pin_mask;
- }
-
- VPIC_UNLOCK(vpic);
-
- return 0;
+ return NR_VPIC_PINS_TOTAL;
}

-int vpic_get_irq_trigger(struct vm *vm, uint32_t irq, enum vpic_trigger
*trigger)
+/**
+ * @pre vm->vpic != NULL
+ * @pre irq < NR_VPIC_PINS_TOTAL
+ */
+void vpic_get_irq_trigger(struct vm *vm, uint32_t irq,
+ enum vpic_trigger *trigger)
{
struct acrn_vpic *vpic;

- if (irq >= NR_VPIC_PINS_TOTAL) {
- return -EINVAL;
- }
-
vpic = vm_pic(vm);
- if (vpic == NULL) {
- return -EINVAL;
- }

if ((vpic->i8259[irq >> 3U].elc & (1U << (irq & 0x7U))) != 0U) {
*trigger = LEVEL_TRIGGER;
} else {
*trigger = EDGE_TRIGGER;
}
- return 0;
}

void vpic_pending_intr(struct vm *vm, uint32_t *vecptr)
diff --git a/hypervisor/include/arch/x86/guest/vioapic.h
b/hypervisor/include/arch/x86/guest/vioapic.h
index 98ffc9e..29bb540 100644
--- a/hypervisor/include/arch/x86/guest/vioapic.h
+++ b/hypervisor/include/arch/x86/guest/vioapic.h
@@ -41,9 +41,9 @@ struct vioapic *vioapic_init(struct vm *vm);
void vioapic_cleanup(struct vioapic *vioapic);
void vioapic_reset(struct vioapic *vioapic);

-int vioapic_assert_irq(struct vm *vm, uint32_t irq);
-int vioapic_deassert_irq(struct vm *vm, uint32_t irq);
-int vioapic_pulse_irq(struct vm *vm, uint32_t irq);
+void vioapic_assert_irq(struct vm *vm, uint32_t irq);
+void vioapic_deassert_irq(struct vm *vm, uint32_t irq);
+void vioapic_pulse_irq(struct vm *vm, uint32_t irq);
void vioapic_update_tmr(struct vcpu *vcpu);

void vioapic_mmio_write(struct vm *vm, uint64_t gpa, uint32_t wval);
diff --git a/hypervisor/include/arch/x86/guest/vpic.h
b/hypervisor/include/arch/x86/guest/vpic.h
index b4565b8..a44bf8d 100644
--- a/hypervisor/include/arch/x86/guest/vpic.h
+++ b/hypervisor/include/arch/x86/guest/vpic.h
@@ -93,17 +93,15 @@ enum vpic_trigger {
void *vpic_init(struct vm *vm);
void vpic_cleanup(struct vm *vm);

-int vpic_assert_irq(struct vm *vm, uint32_t irq);
-int vpic_deassert_irq(struct vm *vm, uint32_t irq);
-int vpic_pulse_irq(struct vm *vm, uint32_t irq);
+void vpic_assert_irq(struct vm *vm, uint32_t irq);
+void vpic_deassert_irq(struct vm *vm, uint32_t irq);
+void vpic_pulse_irq(struct vm *vm, uint32_t irq);

void vpic_pending_intr(struct vm *vm, uint32_t *vecptr);
void vpic_intr_accepted(struct vm *vm, uint32_t vector);
-int vpic_set_irq_trigger(struct vm *vm, uint32_t irq,
- enum vpic_trigger trigger);
-int vpic_get_irq_trigger(struct vm *vm, uint32_t irq,
+void vpic_get_irq_trigger(struct vm *vm, uint32_t irq,
enum vpic_trigger *trigger);
-
+uint32_t vpic_pincount(void);
bool vpic_is_pin_mask(struct acrn_vpic *vpic, uint8_t virt_pin_arg);

#endif /* _VPIC_H_ */
--
2.7.4



Re: [PATCH v2 0/8] USB xHCI S3 support

Yu Wang
 

On 18-08-14 21:39:06, Wu, Xiaoguang wrote:
This patchset enable the USB xHCI S3 feature.
Please refer to other's cover letter to describe what is the v2 changs.


Xiaoguang Wu (8):
DM USB: xHCI: fix an xHCI issue to enable UOS s3 feature
DM USB: xHCI: refine xHCI PORTSC Register related functions
DM USB: introduce struct usb_native_devinfo
DM USB: xHCI: limit bus and port numbers of xHCI
DM USB: xHCI: refine port assignment logic
DM USB: xHCI: code cleanup: change variable name
DM USB: xHCI: change flow of creation of virtual USB device
DM USB: xHCI: enable xHCI SOS S3 support

devicemodel/hw/pci/xhci.c | 305 ++++++++++++++++++++--------------
devicemodel/hw/platform/usb_pmapper.c | 95 +++++------
devicemodel/include/usb_core.h | 14 +-
devicemodel/include/usb_pmapper.h | 9 +-
4 files changed, 242 insertions(+), 181 deletions(-)

--
2.7.4




Re: [PATCH v2 7/8] DM USB: xHCI: change flow of creation of virtual USB device

Yu Wang
 

On 18-08-15 02:46:58, Wu, Xiaoguang wrote:
On 18-08-14 23:15:19, Yu Wang wrote:
On 18-08-14 21:39:13, Wu, Xiaoguang wrote:
The xHCI emulation greatly depends on the user space library libusb
which is based on the usbfs module in Linux kernel. The libusb will
bind usbfs to physical USB device which makes hardware control over
libusb in user space possible.

The pci_xhci_dev_create is called in pci_xhci_native_usb_dev_conn_cb
which is a callback function triggered by physical USB device plugging.
This function will bind the physical USB device to usbfs in SOS, which
we depend to create the communication between UOS xHCI driver with
physical USB device.

This design will fail if the reconnection happened in the SOS, which
will bind class driver to the physical USB device instead of usbfs,
hence the libusb device handle in DM is invalid.

Currently, the native S3 will disable the vbus for all xHCI ports and
re-drive during S3 resume. This behavior cause native USB driver unbind
the usbfs and bind to related class driver, then made the DM lost
control and failed to continue emulation.

To fix this issue, place the pci_xhci_dev_create in the function
pci_xhci_cmd_enable_slot. According to the xHCI spec 4.5.3 Figure 10,
the UOS always send Enable Slot command when a device is attached or
recovered from errors (by Disable Slot command). So every time the SOS
can't resuming normally or some unexpected disconnection happens, this
desigen will always survive by Disable Slot and Enable Slot command
series from UOS xHCI driver.

Signed-off-by: Xiaoguang Wu <xiaoguang.wu@...>
Reviewed-by: Liang Yang <liang3.yang@...>
---
devicemodel/hw/pci/xhci.c | 197 ++++++++++++++++++++++++++++++----------------
1 file changed, 130 insertions(+), 67 deletions(-)

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index c5fce1e..ec215a0 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -373,7 +373,10 @@ struct pci_xhci_vdev {
int (*excap_write)(struct pci_xhci_vdev *, uint64_t, uint64_t);
int usb2_port_start;
int usb3_port_start;
- uint8_t port_map_tbl[USB_NATIVE_NUM_BUS][USB_NATIVE_NUM_PORT];
+
+ uint16_t port_map_tbl[USB_NATIVE_NUM_BUS][USB_NATIVE_NUM_PORT];
+ struct usb_native_devinfo
+ native_dev_info[USB_NATIVE_NUM_BUS][USB_NATIVE_NUM_PORT];
struct timespec mf_prev_time; /* previous time of accessing MFINDEX */
};

@@ -385,8 +388,16 @@ struct pci_xhci_vdev {
#define XHCI_GADDR(xdev, a) paddr_guest2host((xdev)->dev->vmctx, (a), \
XHCI_PADDR_SZ - ((a) & (XHCI_PADDR_SZ-1)))

+/* port mapping status */
#define PT_NOT_ASSIGNED (0)
-#define PT_ASSIGNED_WITH_NO_DEV (-1)
+#define PT_ASSIGNED_WITH_NO_DEV (1)
+#define PT_ASSIGNED_WITH_NEW_DEV (2)
+#define PT_ASSIGNED_WITH_EMU_DEV (3)
Just like the previous comment. These names are not good. Please change
to:
VPORT_FREE
VPORT_ASSIGNED
VPORT_CONNECTED
VPORT_EMULATED
xgwu:

ok, will change. thanks.

+
+/* helpers for get port mapping information */
+#define PT_GET_NUMBER(state) (state & 0xFF)
+#define PT_GET_STATUS(state) ((state >> 8) & 0xFF)
+#define PT_MAKE_STATE(status, num) (((status & 0xFF) << 8) | (num & 0xFF))
Don't use PT. The PT in hypervisor is means passthrough... It is easy
confused..

#define VPORT_NUM(val) (val & 0xFF)
#define VPORT_STATE(val) ((val >> 8) & 0xFF)
#define VPORT_SET_CONFIG(state, num) (((state & 0xFF) << 8) | (num & 0xFF))
xgwu:

will change to:

VPORT_GET_NUMBER
VPORT_GET_STATUS
VPORT_MAKE_STATE ---> this one just 'make', do not 'set'

this means STATE = STATUS + NUMBER
Please don't mix the state and status, they are almost same mean. It
easy confused. Please consider other solution.






struct pci_xhci_option_elem {
char *parse_opt;
@@ -470,83 +481,58 @@ static struct pci_xhci_option_elem xhci_option_table[] = {
static int
pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
{
- struct pci_xhci_dev_emu *de;
struct pci_xhci_vdev *xdev;
- struct usb_devemu *ue;
struct usb_native_devinfo *di;
- int port_start, port_end;
- int slot_start, slot_end;
- int port, slot;
- void *ud;
+ int vport_start, vport_end;
+ int port;

xdev = hci_data;
- di = dev_data;

assert(xdev);
assert(dev_data);
assert(xdev->devices);
assert(xdev->slots);

- de = pci_xhci_dev_create(xdev, di);
- if (!de) {
- UPRINTF(LFTL, "fail to create device\r\n");
- return -1;
- }
-
- /* find free port and slot for the new usb device */
- ud = de->dev_instance;
- ue = de->dev_ue;
-
- assert(ud);
- assert(ue);
+ di = dev_data;

/* print physical information about new device */
UPRINTF(LDBG, "%04x:%04x %d-%d connecting.\r\n",
di->vid, di->pid, di->bus, di->port);

- if (xdev->port_map_tbl[di->bus][di->port] == PT_NOT_ASSIGNED) {
+ if (PT_GET_STATUS(xdev->port_map_tbl[di->bus][di->port]) ==
+ PT_NOT_ASSIGNED) {
UPRINTF(LDBG, "%04x:%04x %d-%d doesn't belong to this vm, bye."
"\r\n", di->vid, di->pid, di->bus, di->port);
goto errout;
}
-
UPRINTF(LDBG, "%04x:%04x %d-%d belong to this vm.\r\n", di->vid,
di->pid, di->bus, di->port);

- if (di->bcd < 0x300)
- port_start = xdev->usb2_port_start;
- else
- port_start = xdev->usb3_port_start;
-
- slot_start = 1;
- port_end = port_start + (XHCI_MAX_DEVS / 2);
- slot_end = XHCI_MAX_SLOTS;
+ if (di->bcd < 0x300) {
+ vport_start = xdev->usb2_port_start;
+ vport_end = vport_start + (XHCI_MAX_DEVS / 2);
+ } else {
+ vport_start = xdev->usb3_port_start;
+ vport_end = vport_start + (XHCI_MAX_DEVS / 2);
+ }

/* find free port */
- for (port = port_start; port < port_end; port++)
+ for (port = vport_start; port < vport_end; port++)
if (!xdev->devices[port])
break;

- /* find free slot */
- for (slot = slot_start; slot < slot_end; slot++)
- if (!xdev->slots[slot])
- break;
-
- if (port >= port_end || slot >= slot_end) {
- UPRINTF(LFTL, "no free resource: port %d slot %d\r\n",
- port, slot);
+ if (port >= vport_end) {
+ UPRINTF(LFTL, "no free virtual port for native device %d-%d"
+ "\r\n", di->bus, di->port);
goto errout;
}

- /* use index of devices as port number */
- xdev->devices[port] = de;
- xdev->slots[slot] = de;
- xdev->ndevices++;
+ UPRINTF(LDBG, "%04X:%04X %d-%d is attached to virtual port %d.\r\n",
+ di->vid, di->pid, di->bus, di->port, port);

- pci_xhci_reset_slot(xdev, slot);
- UPRINTF(LDBG, "%X:%X %d-%d locates in slot %d port %d.\r\n",
- di->vid, di->pid, di->bus, di->port,
- slot, port);
+ xdev->native_dev_info[di->bus][di->port] = *di;
+ xdev->port_map_tbl[di->bus][di->port] =
+ PT_MAKE_STATE(PT_ASSIGNED_WITH_NEW_DEV, port);

/* Trigger port change event for the arriving device */
if (pci_xhci_connect_port(xdev, port, di->speed, 1))
@@ -554,7 +540,6 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)

return 0;
errout:
- pci_xhci_dev_destroy(de);
return -1;
}

@@ -563,8 +548,10 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
{
struct pci_xhci_vdev *xdev;
struct pci_xhci_dev_emu *edev;
+ struct usb_native_devinfo di;
struct usb_dev *udev;
- uint8_t port, native_port;
+ uint8_t port, slot, native_port;
+ uint8_t status;

assert(hci_data);
assert(dev_data);
@@ -584,8 +571,10 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
continue;

udev = edev->dev_instance;
- if (udev->info.port == native_port)
+ if (udev->info.port == native_port) {
+ di = udev->info;
break;
+ }
}

if (port == XHCI_MAX_DEVS + 1) {
@@ -593,6 +582,18 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
return -1;
}

+ for (slot = 1; slot < XHCI_MAX_SLOTS; ++slot)
+ if (xdev->slots[slot] == edev)
+ break;
+
+ assert(slot < USB_NATIVE_NUM_BUS);
+
+ status = PT_GET_STATUS(xdev->port_map_tbl[di.bus][di.port]);
+ assert(status == PT_ASSIGNED_WITH_EMU_DEV ||
+ status == PT_ASSIGNED_WITH_NEW_DEV);
Let's unify all status to state.. Don't mix them..

+ xdev->port_map_tbl[di.bus][di.port] = PT_MAKE_STATE(
+ PT_ASSIGNED_WITH_NO_DEV, port);
+
UPRINTF(LDBG, "report virtual port %d status\r\n", port);
if (pci_xhci_disconnect_port(xdev, port, 1)) {
UPRINTF(LFTL, "fail to report event\r\n");
@@ -1393,28 +1394,67 @@ done:
return err;
}

+static struct usb_native_devinfo *
+pci_xhci_find_native_devinfo(struct pci_xhci_vdev *xdev)
+{
+ int i, j;
+
+ assert(xdev);
+ for (i = 0; i < USB_NATIVE_NUM_BUS; ++i)
+ for (j = 0; j < USB_NATIVE_NUM_PORT; ++j)
+ if (PT_GET_STATUS(xdev->port_map_tbl[i][j]) ==
+ PT_ASSIGNED_WITH_NEW_DEV)
+ return &xdev->native_dev_info[i][j];
+
+ return NULL;
+}
+
static uint32_t
pci_xhci_cmd_enable_slot(struct pci_xhci_vdev *xdev, uint32_t *slot)
{
struct pci_xhci_dev_emu *dev;
- uint32_t cmderr;
- int i;
+ uint32_t cmderr;
+ struct usb_native_devinfo *di;
+ int i, vport;

cmderr = XHCI_TRB_ERROR_NO_SLOTS;
- if (xdev->portregs != NULL)
- for (i = 1; i <= XHCI_MAX_SLOTS; i++) {
- dev = XHCI_SLOTDEV_PTR(xdev, i);
- if (dev && dev->dev_slotstate == XHCI_ST_DISABLED) {
- *slot = i;
- dev->dev_slotstate = XHCI_ST_ENABLED;
- cmderr = XHCI_TRB_ERROR_SUCCESS;
- dev->hci.hci_address = i;
- break;
- }
+
+ di = pci_xhci_find_native_devinfo(xdev);
+ if (!di) {
+ UPRINTF(LWRN, "unexpected Enable Slot commnad\r\n");
+ return -1;
+ }
+
+ assert(di->priv_data);
+ dev = pci_xhci_dev_create(xdev, di);
+ if (!dev) {
+ UPRINTF(LFTL, "fail to create device\r\n");
+ return -1;
+ }
+
+ vport = PT_GET_NUMBER(xdev->port_map_tbl[di->bus][di->port]);
+ assert(vport > 0);
+ assert(!xdev->devices[vport]);
+
+ xdev->devices[vport] = dev;
+ xdev->ndevices++;
+
+ for (i = 1; i <= XHCI_MAX_SLOTS; i++) {
+ if (XHCI_SLOTDEV_PTR(xdev, i) == NULL) {
+ xdev->slots[i] = dev;
+ *slot = i;
+ dev->dev_slotstate = XHCI_ST_ENABLED;
+ cmderr = XHCI_TRB_ERROR_SUCCESS;
+ dev->hci.hci_address = i;
+ xdev->port_map_tbl[di->bus][di->port] =
+ PT_MAKE_STATE(PT_ASSIGNED_WITH_EMU_DEV, vport);
+ break;
}
+ }

- UPRINTF(LDBG, "enable slot (error=%d) slot %u\r\n",
- cmderr != XHCI_TRB_ERROR_SUCCESS, *slot);
+ UPRINTF(LDBG, "enable slot (error=%d) slot %u for native device "
+ "%d-%d\r\n", cmderr != XHCI_TRB_ERROR_SUCCESS, *slot,
+ di->bus, di->port);

return cmderr;
}
@@ -1423,6 +1463,8 @@ static uint32_t
pci_xhci_cmd_disable_slot(struct pci_xhci_vdev *xdev, uint32_t slot)
{
struct pci_xhci_dev_emu *dev;
+ struct usb_dev *udev;
+ struct usb_native_devinfo *di;
uint32_t cmderr;
int i;

@@ -1446,6 +1488,9 @@ pci_xhci_cmd_disable_slot(struct pci_xhci_vdev *xdev, uint32_t slot)
cmderr = XHCI_TRB_ERROR_SUCCESS;
/* TODO: reset events and endpoints */
}
+ } else {
+ UPRINTF(LDBG, "disable NULL device, slot %d\r\n", slot);
+ goto done;
}

for (i = 1; i <= XHCI_MAX_DEVS; ++i)
@@ -1455,8 +1500,20 @@ pci_xhci_cmd_disable_slot(struct pci_xhci_vdev *xdev, uint32_t slot)
if (i <= XHCI_MAX_DEVS && XHCI_PORTREG_PTR(xdev, i)) {
XHCI_PORTREG_PTR(xdev, i)->portsc &= ~(XHCI_PS_CSC |
XHCI_PS_CCS | XHCI_PS_PED | XHCI_PS_PP);
+
+ udev = dev->dev_instance;
+ assert(udev);
+
xdev->devices[i] = NULL;
xdev->slots[slot] = NULL;
+
+ di = &udev->info;
+ xdev->port_map_tbl[di->bus][di->port] =
+ PT_MAKE_STATE(PT_ASSIGNED_WITH_NO_DEV, 0);
+
+ UPRINTF(LINF, "disable slot %d for native device %d-%d"
+ "\r\n", slot, di->bus, di->port);
+
pci_xhci_dev_destroy(dev);
} else
UPRINTF(LWRN, "invalid slot %d\r\n", slot);
@@ -1624,7 +1681,10 @@ pci_xhci_cmd_config_ep(struct pci_xhci_vdev *xdev,
UPRINTF(LDBG, "config_ep slot %u\r\n", slot);

dev = XHCI_SLOTDEV_PTR(xdev, slot);
- assert(dev != NULL);
+ if (dev == NULL) {
+ cmderr = XHCI_TRB_ERROR_SLOT_NOT_ON;
+ goto done;
+ }

if ((trb->dwTrb3 & XHCI_TRB_3_DCEP_BIT) != 0) {
UPRINTF(LDBG, "config_ep - deconfigure ep slot %u\r\n", slot);
@@ -3364,7 +3424,10 @@ pci_xhci_parse_bus_port(struct pci_xhci_vdev *xdev, char *opts)
goto errout;
}

- xdev->port_map_tbl[bus][port] = PT_ASSIGNED_WITH_NO_DEV;
+ xdev->port_map_tbl[bus][port] =
+ PT_MAKE_STATE(PT_ASSIGNED_WITH_NO_DEV, 0);
+ return 0;
+
errout:
if (rc)
UPRINTF(LWRN, "%s fails, rc=%d\r\n", __func__, rc);
--
2.7.4






Re: [PATCH 0/8] instruction decoding refine

Xu, Anthony
 

In ACRN HV, only LAPIC and IOAPIC virtualization need instruction decoding.
The instructions to access LAPIC and IOAPIC are restrained. Can we only decode
instructions which are used to access LAPIC and IOAPIC and defer other instruction decoding
to SOS?


Anthony

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Yin, Fengwei
Sent: Tuesday, August 14, 2018 6:04 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH 0/8] instruction decoding refine

According to SDM, we should check whether the gva access from
guest is valid. If it's not, correct exception should be injected.

We only need to emulate the instructions which access the
memory and could trigger EPT violation or APIC access VM exit.
It's not necessary to cover the instructions which doesn't access
memory.

To eliminate the side effect of access mmio, we move all gva check
to instruction decoding phase. To make instruction emulation always
sucess.

There are two types of instructions:
- MOVS/STO. The gva is from DI/SI
- Others. The gva is from instruction decoding
We cover both of them.

The TODO work in next cyle refine:
- Fix issue in movs
- Optimize movs
- enable smep/smap check during gva2gpa
- cache the gpa during instruction decoding and avoid gva2gpa
during instruction emulation

Yin Fengwei (8):
hv: add lock prefix check for exception
hv: extend the decode_modrm
hv: fix use without initialization build error
hv: remove unnecessary check for gva
hv: move check out of vie_calculate_gla
hv: add new function to get gva for MOVS/STO instruction
hv: add failure case check for MOVS/STO
hv: add gva check for the case gva is from instruction decode

hypervisor/arch/x86/guest/instr_emul.c | 484 ++++++++++++++++---------
hypervisor/arch/x86/guest/instr_emul.h | 3 +-
2 files changed, 320 insertions(+), 167 deletions(-)

--
2.17.0



Re: [PATCH 2/8] hv: extend the decode_modrm

Xu, Anthony
 

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Yin, Fengwei
Sent: Tuesday, August 14, 2018 6:04 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH 2/8] hv: extend the decode_modrm

If rm show there is no SIB following rm field, we should get
base_register info from rm.
---
hypervisor/arch/x86/guest/instr_emul.c | 42 +++++++++++++++++++++++++-
1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/hypervisor/arch/x86/guest/instr_emul.c
b/hypervisor/arch/x86/guest/instr_emul.c
index 222fa033..d62c6eaa 100644
--- a/hypervisor/arch/x86/guest/instr_emul.c
+++ b/hypervisor/arch/x86/guest/instr_emul.c
@@ -1652,6 +1652,11 @@ static int vie_init(struct instr_emul_vie *vie, struct
vcpu *vcpu)

(void)memset(vie, 0U, sizeof(struct instr_emul_vie));

+ /* init register fields in vie. */
+ vie->base_register = CPU_REG_LAST;
+ vie->index_register = CPU_REG_LAST;
+ vie->segment_register = CPU_REG_LAST;
+
err_code = PAGE_FAULT_ID_FLAG;
ret = copy_from_gva(vcpu, vie->inst, guest_rip_gva,
inst_len, &err_code, &fault_addr);
@@ -1877,6 +1882,41 @@ static int decode_modrm(struct instr_emul_vie
*vie, enum vm_cpu_mode cpu_mode)

vie->reg |= (vie->rex_r << 3);

+ /* SIB */
+ if (vie->mod != VIE_MOD_DIRECT && vie->rm == VIE_RM_SIB) {
+ goto done;
+ }
+
+ vie->base_register = vie->rm;
+
+ switch (vie->mod) {
+ case VIE_MOD_INDIRECT_DISP8:
+ vie->disp_bytes = 1U;
+ break;
+ case VIE_MOD_INDIRECT_DISP32:
+ vie->disp_bytes = 4U;
+ break;
decode_sib has the same logic, can we consolidate them?





+ case VIE_MOD_INDIRECT:
+ if (vie->rm == VIE_RM_DISP32) {
+ vie->disp_bytes = 4U;
+ /*
+ * Table 2-7. RIP-Relative Addressing
+ *
+ * In 64-bit mode mod=00 r/m=101 implies [rip] + disp32
+ * whereas in compatibility mode it just implies disp32.
+ */
+
+ if (cpu_mode == CPU_MODE_64BIT) {
+ vie->base_register = CPU_REG_RIP;
+ }
+ else {
+ vie->base_register = CPU_REG_LAST;
+ }
+ }
+ break;
+ }
+
+done:
vie_advance(vie);

return 0;
@@ -1953,7 +1993,7 @@ static int decode_sib(struct instr_emul_vie *vie)
}

/* 'scale' makes sense only in the context of an index register */
- if (vie->index_register <= CPU_REG_LAST) {
+ if (vie->index_register < CPU_REG_LAST) {
vie->scale = 1U << vie->ss;
}

--
2.17.0



Re: [PATCH 1/8] hv: add lock prefix check for exception

Xu, Anthony
 

Does this trigger GP in guest before triggering VM_Exit?

Anthony

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Yin, Fengwei
Sent: Tuesday, August 14, 2018 6:04 AM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH 1/8] hv: add lock prefix check for exception

According to SDM Vol2 - Instruction Set Reference:
Some instructions can't work with prefix lock or can't work in
some situations (destination is not a memory operand).

We add the lock prefix check for the instructions we support and
inject UD if lock shouldn't be used.

Signed-off-by: Yin Fengwei <fengwei.yin@...>
---
hypervisor/arch/x86/guest/instr_emul.c | 34 ++++++++++++++++++++++----
hypervisor/arch/x86/guest/instr_emul.h | 3 ++-
2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/hypervisor/arch/x86/guest/instr_emul.c
b/hypervisor/arch/x86/guest/instr_emul.c
index 521f0d36..222fa033 100644
--- a/hypervisor/arch/x86/guest/instr_emul.c
+++ b/hypervisor/arch/x86/guest/instr_emul.c
@@ -56,20 +56,24 @@
#define VIE_OP_F_MOFFSET (1U << 2) /* 16/32/64-bit immediate
moffset */
#define VIE_OP_F_NO_MODRM (1U << 3)
#define VIE_OP_F_NO_GLA_VERIFICATION (1U << 4)
+#define VIE_OP_F_NO_LOCK (1U << 5) /* UD if lock prefix is used
*/

static const struct instr_emul_vie_op two_byte_opcodes[256] = {
[0xB6] = {
.op_type = VIE_OP_TYPE_MOVZX,
+ .op_flags = VIE_OP_F_NO_LOCK,
},
[0xB7] = {
.op_type = VIE_OP_TYPE_MOVZX,
+ .op_flags = VIE_OP_F_NO_LOCK,
},
[0xBA] = {
.op_type = VIE_OP_TYPE_BITTEST,
- .op_flags = VIE_OP_F_IMM8,
+ .op_flags = VIE_OP_F_IMM8 | VIE_OP_F_NO_LOCK,
},
[0xBE] = {
.op_type = VIE_OP_TYPE_MOVSX,
+ .op_flags = VIE_OP_F_NO_LOCK,
},
};

@@ -79,32 +83,41 @@ static const struct instr_emul_vie_op
one_byte_opcodes[256] = {
},
[0x2B] = {
.op_type = VIE_OP_TYPE_SUB,
+ .op_flags = VIE_OP_F_NO_LOCK, /* 0x2B has register as dst */
},
[0x39] = {
.op_type = VIE_OP_TYPE_CMP,
+ .op_flags = VIE_OP_F_NO_LOCK,
},
[0x3B] = {
.op_type = VIE_OP_TYPE_CMP,
+ .op_flags = VIE_OP_F_NO_LOCK,
},
[0x88] = {
.op_type = VIE_OP_TYPE_MOV,
+ .op_flags = VIE_OP_F_NO_LOCK,
},
[0x89] = {
.op_type = VIE_OP_TYPE_MOV,
+ .op_flags = VIE_OP_F_NO_LOCK,
},
[0x8A] = {
.op_type = VIE_OP_TYPE_MOV,
+ .op_flags = VIE_OP_F_NO_LOCK,
},
[0x8B] = {
.op_type = VIE_OP_TYPE_MOV,
+ .op_flags = VIE_OP_F_NO_LOCK,
},
[0xA1] = {
.op_type = VIE_OP_TYPE_MOV,
- .op_flags = VIE_OP_F_MOFFSET | VIE_OP_F_NO_MODRM,
+ .op_flags = VIE_OP_F_MOFFSET | VIE_OP_F_NO_MODRM |
+ VIE_OP_F_NO_LOCK,
},
[0xA3] = {
.op_type = VIE_OP_TYPE_MOV,
- .op_flags = VIE_OP_F_MOFFSET | VIE_OP_F_NO_MODRM,
+ .op_flags = VIE_OP_F_MOFFSET | VIE_OP_F_NO_MODRM |
+ VIE_OP_F_NO_LOCK,
},
[0xA4] = {
.op_type = VIE_OP_TYPE_MOVS,
@@ -125,14 +138,15 @@ static const struct instr_emul_vie_op
one_byte_opcodes[256] = {
[0xC6] = {
/* XXX Group 11 extended opcode - not just MOV */
.op_type = VIE_OP_TYPE_MOV,
- .op_flags = VIE_OP_F_IMM8,
+ .op_flags = VIE_OP_F_IMM8 | VIE_OP_F_NO_LOCK,
},
[0xC7] = {
.op_type = VIE_OP_TYPE_MOV,
- .op_flags = VIE_OP_F_IMM,
+ .op_flags = VIE_OP_F_IMM | VIE_OP_F_NO_LOCK,
},
[0x23] = {
.op_type = VIE_OP_TYPE_AND,
+ .op_flags = VIE_OP_F_NO_LOCK, /* 0x23 has register as dst */
},
[0x80] = {
/* Group 1 extended opcode */
@@ -151,9 +165,11 @@ static const struct instr_emul_vie_op
one_byte_opcodes[256] = {
},
[0x84] = {
.op_type = VIE_OP_TYPE_TEST,
+ .op_flags = VIE_OP_F_NO_LOCK,
},
[0x85] = {
.op_type = VIE_OP_TYPE_TEST,
+ .op_flags = VIE_OP_F_NO_LOCK,
},
[0x08] = {
.op_type = VIE_OP_TYPE_OR,
@@ -1714,6 +1730,8 @@ static int decode_prefixes(struct instr_emul_vie *vie,
vie->repz_present = 1U;
} else if (x == 0xF2U) {
vie->repnz_present = 1U;
+ } else if (x == 0xF0U) {
+ vie->lock_prefix = 1U;
} else if (segment_override(x, &vie->segment_register)) {
vie->seg_override = 1U;
} else {
@@ -2114,6 +2132,12 @@ static int local_decode_instruction(enum
vm_cpu_mode cpu_mode,
return -1;
}

+ /* If lock is used with instruction doesn't allow lock,
+ * inject invalid opcode exception UD to guest.
+ */
+ if (vie->lock_prefix && (vie->op.op_flags & VIE_OP_F_NO_LOCK))
+ return -1;
+
vie->decoded = 1U; /* success */

return 0;
diff --git a/hypervisor/arch/x86/guest/instr_emul.h
b/hypervisor/arch/x86/guest/instr_emul.h
index 6132332e..a7a20bdb 100644
--- a/hypervisor/arch/x86/guest/instr_emul.h
+++ b/hypervisor/arch/x86/guest/instr_emul.h
@@ -103,7 +103,8 @@ struct instr_emul_vie {
repnz_present:1, /* REPNE/REPNZ prefix */
opsize_override:1, /* Operand size override */
addrsize_override:1, /* Address size override */
- seg_override:1; /* Segment override */
+ seg_override:1, /* Segment override */
+ lock_prefix:1; /* has LOCK prefix */

uint8_t mod:2, /* ModRM byte */
reg:4,
--
2.17.0



Re: [PATCH v2 8/8] DM USB: xHCI: enable xHCI SOS S3 support

Wu, Xiaoguang
 

On 18-08-14 23:17:02, Yu Wang wrote:
On 18-08-14 21:39:14, Wu, Xiaoguang wrote:
This patch enable the support for SOS S3 from the perspective
of USB xHCI.

Signed-off-by: Xiaoguang Wu <xiaoguang.wu@...>
Reviewed-by: Liang Yang <liang3.yang@...>
---
devicemodel/hw/pci/xhci.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index ec215a0..3509fbd 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -91,6 +91,7 @@
#include "pci_core.h"
#include "xhci.h"
#include "usb_pmapper.h"
+#include "vmmapi.h"

#undef LOG_TAG
#define LOG_TAG "xHCI: "
@@ -485,6 +486,7 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
struct usb_native_devinfo *di;
int vport_start, vport_end;
int port;
+ int need_intr = 1;

xdev = hci_data;

@@ -534,8 +536,11 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
xdev->port_map_tbl[di->bus][di->port] =
PT_MAKE_STATE(PT_ASSIGNED_WITH_NEW_DEV, port);

+ if (vm_get_suspend_mode() != VM_SUSPEND_NONE)
+ need_intr = 0;
+
/* Trigger port change event for the arriving device */
- if (pci_xhci_connect_port(xdev, port, di->speed, 1))
+ if (pci_xhci_connect_port(xdev, port, di->speed, need_intr))
UPRINTF(LFTL, "fail to report port event\n");

return 0;
@@ -552,6 +557,7 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
struct usb_dev *udev;
uint8_t port, slot, native_port;
uint8_t status;
+ int need_intr = 1;

assert(hci_data);
assert(dev_data);
@@ -594,8 +600,18 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
xdev->port_map_tbl[di.bus][di.port] = PT_MAKE_STATE(
PT_ASSIGNED_WITH_NO_DEV, port);

+ if (vm_get_suspend_mode() != VM_SUSPEND_NONE) {
+ XHCI_PORTREG_PTR(xdev, port)->portsc &= ~(XHCI_PS_CSC |
+ XHCI_PS_CCS | XHCI_PS_PED | XHCI_PS_PP);
+ edev->dev_slotstate = XHCI_ST_DISABLED;
+ xdev->devices[port] = NULL;
+ xdev->slots[slot] = NULL;
+ pci_xhci_dev_destroy(edev);
+ need_intr = 0;
+ }
Please add one TODO. We need revisit this logic in future. This patch is
like one band-aid fix.

xgwu:

okey, will change, thanks.


+
UPRINTF(LDBG, "report virtual port %d status\r\n", port);
- if (pci_xhci_disconnect_port(xdev, port, 1)) {
+ if (pci_xhci_disconnect_port(xdev, port, need_intr)) {
UPRINTF(LFTL, "fail to report event\r\n");
return -1;
}
--
2.7.4





Re: [PATCH v2 7/8] DM USB: xHCI: change flow of creation of virtual USB device

Wu, Xiaoguang
 

On 18-08-14 23:15:19, Yu Wang wrote:
On 18-08-14 21:39:13, Wu, Xiaoguang wrote:
The xHCI emulation greatly depends on the user space library libusb
which is based on the usbfs module in Linux kernel. The libusb will
bind usbfs to physical USB device which makes hardware control over
libusb in user space possible.

The pci_xhci_dev_create is called in pci_xhci_native_usb_dev_conn_cb
which is a callback function triggered by physical USB device plugging.
This function will bind the physical USB device to usbfs in SOS, which
we depend to create the communication between UOS xHCI driver with
physical USB device.

This design will fail if the reconnection happened in the SOS, which
will bind class driver to the physical USB device instead of usbfs,
hence the libusb device handle in DM is invalid.

Currently, the native S3 will disable the vbus for all xHCI ports and
re-drive during S3 resume. This behavior cause native USB driver unbind
the usbfs and bind to related class driver, then made the DM lost
control and failed to continue emulation.

To fix this issue, place the pci_xhci_dev_create in the function
pci_xhci_cmd_enable_slot. According to the xHCI spec 4.5.3 Figure 10,
the UOS always send Enable Slot command when a device is attached or
recovered from errors (by Disable Slot command). So every time the SOS
can't resuming normally or some unexpected disconnection happens, this
desigen will always survive by Disable Slot and Enable Slot command
series from UOS xHCI driver.

Signed-off-by: Xiaoguang Wu <xiaoguang.wu@...>
Reviewed-by: Liang Yang <liang3.yang@...>
---
devicemodel/hw/pci/xhci.c | 197 ++++++++++++++++++++++++++++++----------------
1 file changed, 130 insertions(+), 67 deletions(-)

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index c5fce1e..ec215a0 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -373,7 +373,10 @@ struct pci_xhci_vdev {
int (*excap_write)(struct pci_xhci_vdev *, uint64_t, uint64_t);
int usb2_port_start;
int usb3_port_start;
- uint8_t port_map_tbl[USB_NATIVE_NUM_BUS][USB_NATIVE_NUM_PORT];
+
+ uint16_t port_map_tbl[USB_NATIVE_NUM_BUS][USB_NATIVE_NUM_PORT];
+ struct usb_native_devinfo
+ native_dev_info[USB_NATIVE_NUM_BUS][USB_NATIVE_NUM_PORT];
struct timespec mf_prev_time; /* previous time of accessing MFINDEX */
};

@@ -385,8 +388,16 @@ struct pci_xhci_vdev {
#define XHCI_GADDR(xdev, a) paddr_guest2host((xdev)->dev->vmctx, (a), \
XHCI_PADDR_SZ - ((a) & (XHCI_PADDR_SZ-1)))

+/* port mapping status */
#define PT_NOT_ASSIGNED (0)
-#define PT_ASSIGNED_WITH_NO_DEV (-1)
+#define PT_ASSIGNED_WITH_NO_DEV (1)
+#define PT_ASSIGNED_WITH_NEW_DEV (2)
+#define PT_ASSIGNED_WITH_EMU_DEV (3)
Just like the previous comment. These names are not good. Please change
to:
VPORT_FREE
VPORT_ASSIGNED
VPORT_CONNECTED
VPORT_EMULATED
xgwu:

ok, will change. thanks.

+
+/* helpers for get port mapping information */
+#define PT_GET_NUMBER(state) (state & 0xFF)
+#define PT_GET_STATUS(state) ((state >> 8) & 0xFF)
+#define PT_MAKE_STATE(status, num) (((status & 0xFF) << 8) | (num & 0xFF))
Don't use PT. The PT in hypervisor is means passthrough... It is easy
confused..

#define VPORT_NUM(val) (val & 0xFF)
#define VPORT_STATE(val) ((val >> 8) & 0xFF)
#define VPORT_SET_CONFIG(state, num) (((state & 0xFF) << 8) | (num & 0xFF))
xgwu:

will change to:

VPORT_GET_NUMBER
VPORT_GET_STATUS
VPORT_MAKE_STATE ---> this one just 'make', do not 'set'

this means STATE = STATUS + NUMBER




struct pci_xhci_option_elem {
char *parse_opt;
@@ -470,83 +481,58 @@ static struct pci_xhci_option_elem xhci_option_table[] = {
static int
pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
{
- struct pci_xhci_dev_emu *de;
struct pci_xhci_vdev *xdev;
- struct usb_devemu *ue;
struct usb_native_devinfo *di;
- int port_start, port_end;
- int slot_start, slot_end;
- int port, slot;
- void *ud;
+ int vport_start, vport_end;
+ int port;

xdev = hci_data;
- di = dev_data;

assert(xdev);
assert(dev_data);
assert(xdev->devices);
assert(xdev->slots);

- de = pci_xhci_dev_create(xdev, di);
- if (!de) {
- UPRINTF(LFTL, "fail to create device\r\n");
- return -1;
- }
-
- /* find free port and slot for the new usb device */
- ud = de->dev_instance;
- ue = de->dev_ue;
-
- assert(ud);
- assert(ue);
+ di = dev_data;

/* print physical information about new device */
UPRINTF(LDBG, "%04x:%04x %d-%d connecting.\r\n",
di->vid, di->pid, di->bus, di->port);

- if (xdev->port_map_tbl[di->bus][di->port] == PT_NOT_ASSIGNED) {
+ if (PT_GET_STATUS(xdev->port_map_tbl[di->bus][di->port]) ==
+ PT_NOT_ASSIGNED) {
UPRINTF(LDBG, "%04x:%04x %d-%d doesn't belong to this vm, bye."
"\r\n", di->vid, di->pid, di->bus, di->port);
goto errout;
}
-
UPRINTF(LDBG, "%04x:%04x %d-%d belong to this vm.\r\n", di->vid,
di->pid, di->bus, di->port);

- if (di->bcd < 0x300)
- port_start = xdev->usb2_port_start;
- else
- port_start = xdev->usb3_port_start;
-
- slot_start = 1;
- port_end = port_start + (XHCI_MAX_DEVS / 2);
- slot_end = XHCI_MAX_SLOTS;
+ if (di->bcd < 0x300) {
+ vport_start = xdev->usb2_port_start;
+ vport_end = vport_start + (XHCI_MAX_DEVS / 2);
+ } else {
+ vport_start = xdev->usb3_port_start;
+ vport_end = vport_start + (XHCI_MAX_DEVS / 2);
+ }

/* find free port */
- for (port = port_start; port < port_end; port++)
+ for (port = vport_start; port < vport_end; port++)
if (!xdev->devices[port])
break;

- /* find free slot */
- for (slot = slot_start; slot < slot_end; slot++)
- if (!xdev->slots[slot])
- break;
-
- if (port >= port_end || slot >= slot_end) {
- UPRINTF(LFTL, "no free resource: port %d slot %d\r\n",
- port, slot);
+ if (port >= vport_end) {
+ UPRINTF(LFTL, "no free virtual port for native device %d-%d"
+ "\r\n", di->bus, di->port);
goto errout;
}

- /* use index of devices as port number */
- xdev->devices[port] = de;
- xdev->slots[slot] = de;
- xdev->ndevices++;
+ UPRINTF(LDBG, "%04X:%04X %d-%d is attached to virtual port %d.\r\n",
+ di->vid, di->pid, di->bus, di->port, port);

- pci_xhci_reset_slot(xdev, slot);
- UPRINTF(LDBG, "%X:%X %d-%d locates in slot %d port %d.\r\n",
- di->vid, di->pid, di->bus, di->port,
- slot, port);
+ xdev->native_dev_info[di->bus][di->port] = *di;
+ xdev->port_map_tbl[di->bus][di->port] =
+ PT_MAKE_STATE(PT_ASSIGNED_WITH_NEW_DEV, port);

/* Trigger port change event for the arriving device */
if (pci_xhci_connect_port(xdev, port, di->speed, 1))
@@ -554,7 +540,6 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)

return 0;
errout:
- pci_xhci_dev_destroy(de);
return -1;
}

@@ -563,8 +548,10 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
{
struct pci_xhci_vdev *xdev;
struct pci_xhci_dev_emu *edev;
+ struct usb_native_devinfo di;
struct usb_dev *udev;
- uint8_t port, native_port;
+ uint8_t port, slot, native_port;
+ uint8_t status;

assert(hci_data);
assert(dev_data);
@@ -584,8 +571,10 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
continue;

udev = edev->dev_instance;
- if (udev->info.port == native_port)
+ if (udev->info.port == native_port) {
+ di = udev->info;
break;
+ }
}

if (port == XHCI_MAX_DEVS + 1) {
@@ -593,6 +582,18 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
return -1;
}

+ for (slot = 1; slot < XHCI_MAX_SLOTS; ++slot)
+ if (xdev->slots[slot] == edev)
+ break;
+
+ assert(slot < USB_NATIVE_NUM_BUS);
+
+ status = PT_GET_STATUS(xdev->port_map_tbl[di.bus][di.port]);
+ assert(status == PT_ASSIGNED_WITH_EMU_DEV ||
+ status == PT_ASSIGNED_WITH_NEW_DEV);
Let's unify all status to state.. Don't mix them..

+ xdev->port_map_tbl[di.bus][di.port] = PT_MAKE_STATE(
+ PT_ASSIGNED_WITH_NO_DEV, port);
+
UPRINTF(LDBG, "report virtual port %d status\r\n", port);
if (pci_xhci_disconnect_port(xdev, port, 1)) {
UPRINTF(LFTL, "fail to report event\r\n");
@@ -1393,28 +1394,67 @@ done:
return err;
}

+static struct usb_native_devinfo *
+pci_xhci_find_native_devinfo(struct pci_xhci_vdev *xdev)
+{
+ int i, j;
+
+ assert(xdev);
+ for (i = 0; i < USB_NATIVE_NUM_BUS; ++i)
+ for (j = 0; j < USB_NATIVE_NUM_PORT; ++j)
+ if (PT_GET_STATUS(xdev->port_map_tbl[i][j]) ==
+ PT_ASSIGNED_WITH_NEW_DEV)
+ return &xdev->native_dev_info[i][j];
+
+ return NULL;
+}
+
static uint32_t
pci_xhci_cmd_enable_slot(struct pci_xhci_vdev *xdev, uint32_t *slot)
{
struct pci_xhci_dev_emu *dev;
- uint32_t cmderr;
- int i;
+ uint32_t cmderr;
+ struct usb_native_devinfo *di;
+ int i, vport;

cmderr = XHCI_TRB_ERROR_NO_SLOTS;
- if (xdev->portregs != NULL)
- for (i = 1; i <= XHCI_MAX_SLOTS; i++) {
- dev = XHCI_SLOTDEV_PTR(xdev, i);
- if (dev && dev->dev_slotstate == XHCI_ST_DISABLED) {
- *slot = i;
- dev->dev_slotstate = XHCI_ST_ENABLED;
- cmderr = XHCI_TRB_ERROR_SUCCESS;
- dev->hci.hci_address = i;
- break;
- }
+
+ di = pci_xhci_find_native_devinfo(xdev);
+ if (!di) {
+ UPRINTF(LWRN, "unexpected Enable Slot commnad\r\n");
+ return -1;
+ }
+
+ assert(di->priv_data);
+ dev = pci_xhci_dev_create(xdev, di);
+ if (!dev) {
+ UPRINTF(LFTL, "fail to create device\r\n");
+ return -1;
+ }
+
+ vport = PT_GET_NUMBER(xdev->port_map_tbl[di->bus][di->port]);
+ assert(vport > 0);
+ assert(!xdev->devices[vport]);
+
+ xdev->devices[vport] = dev;
+ xdev->ndevices++;
+
+ for (i = 1; i <= XHCI_MAX_SLOTS; i++) {
+ if (XHCI_SLOTDEV_PTR(xdev, i) == NULL) {
+ xdev->slots[i] = dev;
+ *slot = i;
+ dev->dev_slotstate = XHCI_ST_ENABLED;
+ cmderr = XHCI_TRB_ERROR_SUCCESS;
+ dev->hci.hci_address = i;
+ xdev->port_map_tbl[di->bus][di->port] =
+ PT_MAKE_STATE(PT_ASSIGNED_WITH_EMU_DEV, vport);
+ break;
}
+ }

- UPRINTF(LDBG, "enable slot (error=%d) slot %u\r\n",
- cmderr != XHCI_TRB_ERROR_SUCCESS, *slot);
+ UPRINTF(LDBG, "enable slot (error=%d) slot %u for native device "
+ "%d-%d\r\n", cmderr != XHCI_TRB_ERROR_SUCCESS, *slot,
+ di->bus, di->port);

return cmderr;
}
@@ -1423,6 +1463,8 @@ static uint32_t
pci_xhci_cmd_disable_slot(struct pci_xhci_vdev *xdev, uint32_t slot)
{
struct pci_xhci_dev_emu *dev;
+ struct usb_dev *udev;
+ struct usb_native_devinfo *di;
uint32_t cmderr;
int i;

@@ -1446,6 +1488,9 @@ pci_xhci_cmd_disable_slot(struct pci_xhci_vdev *xdev, uint32_t slot)
cmderr = XHCI_TRB_ERROR_SUCCESS;
/* TODO: reset events and endpoints */
}
+ } else {
+ UPRINTF(LDBG, "disable NULL device, slot %d\r\n", slot);
+ goto done;
}

for (i = 1; i <= XHCI_MAX_DEVS; ++i)
@@ -1455,8 +1500,20 @@ pci_xhci_cmd_disable_slot(struct pci_xhci_vdev *xdev, uint32_t slot)
if (i <= XHCI_MAX_DEVS && XHCI_PORTREG_PTR(xdev, i)) {
XHCI_PORTREG_PTR(xdev, i)->portsc &= ~(XHCI_PS_CSC |
XHCI_PS_CCS | XHCI_PS_PED | XHCI_PS_PP);
+
+ udev = dev->dev_instance;
+ assert(udev);
+
xdev->devices[i] = NULL;
xdev->slots[slot] = NULL;
+
+ di = &udev->info;
+ xdev->port_map_tbl[di->bus][di->port] =
+ PT_MAKE_STATE(PT_ASSIGNED_WITH_NO_DEV, 0);
+
+ UPRINTF(LINF, "disable slot %d for native device %d-%d"
+ "\r\n", slot, di->bus, di->port);
+
pci_xhci_dev_destroy(dev);
} else
UPRINTF(LWRN, "invalid slot %d\r\n", slot);
@@ -1624,7 +1681,10 @@ pci_xhci_cmd_config_ep(struct pci_xhci_vdev *xdev,
UPRINTF(LDBG, "config_ep slot %u\r\n", slot);

dev = XHCI_SLOTDEV_PTR(xdev, slot);
- assert(dev != NULL);
+ if (dev == NULL) {
+ cmderr = XHCI_TRB_ERROR_SLOT_NOT_ON;
+ goto done;
+ }

if ((trb->dwTrb3 & XHCI_TRB_3_DCEP_BIT) != 0) {
UPRINTF(LDBG, "config_ep - deconfigure ep slot %u\r\n", slot);
@@ -3364,7 +3424,10 @@ pci_xhci_parse_bus_port(struct pci_xhci_vdev *xdev, char *opts)
goto errout;
}

- xdev->port_map_tbl[bus][port] = PT_ASSIGNED_WITH_NO_DEV;
+ xdev->port_map_tbl[bus][port] =
+ PT_MAKE_STATE(PT_ASSIGNED_WITH_NO_DEV, 0);
+ return 0;
+
errout:
if (rc)
UPRINTF(LWRN, "%s fails, rc=%d\r\n", __func__, rc);
--
2.7.4





Re: [PATCH v2 5/8] DM USB: xHCI: refine port assignment logic

Wu, Xiaoguang
 

On 18-08-14 22:59:18, Yu Wang wrote:
On 18-08-14 21:39:11, Wu, Xiaoguang wrote:
The variable native_assign_ports in struct pci_xhci_vdev is used
to record wether certain root hub port in SOS is assigned to UOS.
The logic uses zero to express 'not assigned' and nonzero to express
'assigned'. In this patch, use macro to replace number to express
better.

Signed-off-by: Xiaoguang Wu <xiaoguang.wu@...>
Reviewed-by: Liang Yang <liang3.yang@...>
---
devicemodel/hw/pci/xhci.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index 14009a8..b5d8555 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -384,6 +384,10 @@ struct pci_xhci_vdev {
#define XHCI_HALTED(xdev) ((xdev)->opregs.usbsts & XHCI_STS_HCH)
#define XHCI_GADDR(xdev, a) paddr_guest2host((xdev)->dev->vmctx, (a), \
XHCI_PADDR_SZ - ((a) & (XHCI_PADDR_SZ-1)))
+
+#define PT_NOT_ASSIGNED (0)
+#define PT_ASSIGNED_WITH_NO_DEV (-1)
The name is not good.

How about:
VPORT_FREE
VPORT_ASSIGNED
VPORT_CONNECTED

In your case, the VPORT_CONNECTED is > 0.
xgwu:

ok, will change, thanks.



+
struct pci_xhci_option_elem {
char *parse_opt;
int (*parse_fn)(struct pci_xhci_vdev *, char *);
@@ -500,7 +504,7 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
UPRINTF(LDBG, "%04x:%04x %d-%d connecting.\r\n",
di->vid, di->pid, di->bus, di->port);

- if (!xdev->native_assign_ports[di->bus][di->port]) {
+ if (xdev->native_assign_ports[di->bus][di->port] == PT_NOT_ASSIGNED) {
UPRINTF(LDBG, "%04x:%04x %d-%d doesn't belong to this vm, bye."
"\r\n", di->vid, di->pid, di->bus, di->port);
goto errout;
@@ -3360,7 +3364,7 @@ pci_xhci_parse_bus_port(struct pci_xhci_vdev *xdev, char *opts)
goto errout;
}

- xdev->native_assign_ports[bus][port] = 1;
+ xdev->native_assign_ports[bus][port] = PT_ASSIGNED_WITH_NO_DEV;
errout:
if (rc)
UPRINTF(LWRN, "%s fails, rc=%d\r\n", __func__, rc);
--
2.7.4





Re: [PATCH v2 8/8] DM USB: xHCI: enable xHCI SOS S3 support

Yu Wang
 

On 18-08-14 21:39:14, Wu, Xiaoguang wrote:
This patch enable the support for SOS S3 from the perspective
of USB xHCI.

Signed-off-by: Xiaoguang Wu <xiaoguang.wu@...>
Reviewed-by: Liang Yang <liang3.yang@...>
---
devicemodel/hw/pci/xhci.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index ec215a0..3509fbd 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -91,6 +91,7 @@
#include "pci_core.h"
#include "xhci.h"
#include "usb_pmapper.h"
+#include "vmmapi.h"

#undef LOG_TAG
#define LOG_TAG "xHCI: "
@@ -485,6 +486,7 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
struct usb_native_devinfo *di;
int vport_start, vport_end;
int port;
+ int need_intr = 1;

xdev = hci_data;

@@ -534,8 +536,11 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
xdev->port_map_tbl[di->bus][di->port] =
PT_MAKE_STATE(PT_ASSIGNED_WITH_NEW_DEV, port);

+ if (vm_get_suspend_mode() != VM_SUSPEND_NONE)
+ need_intr = 0;
+
/* Trigger port change event for the arriving device */
- if (pci_xhci_connect_port(xdev, port, di->speed, 1))
+ if (pci_xhci_connect_port(xdev, port, di->speed, need_intr))
UPRINTF(LFTL, "fail to report port event\n");

return 0;
@@ -552,6 +557,7 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
struct usb_dev *udev;
uint8_t port, slot, native_port;
uint8_t status;
+ int need_intr = 1;

assert(hci_data);
assert(dev_data);
@@ -594,8 +600,18 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
xdev->port_map_tbl[di.bus][di.port] = PT_MAKE_STATE(
PT_ASSIGNED_WITH_NO_DEV, port);

+ if (vm_get_suspend_mode() != VM_SUSPEND_NONE) {
+ XHCI_PORTREG_PTR(xdev, port)->portsc &= ~(XHCI_PS_CSC |
+ XHCI_PS_CCS | XHCI_PS_PED | XHCI_PS_PP);
+ edev->dev_slotstate = XHCI_ST_DISABLED;
+ xdev->devices[port] = NULL;
+ xdev->slots[slot] = NULL;
+ pci_xhci_dev_destroy(edev);
+ need_intr = 0;
+ }
Please add one TODO. We need revisit this logic in future. This patch is
like one band-aid fix.

+
UPRINTF(LDBG, "report virtual port %d status\r\n", port);
- if (pci_xhci_disconnect_port(xdev, port, 1)) {
+ if (pci_xhci_disconnect_port(xdev, port, need_intr)) {
UPRINTF(LFTL, "fail to report event\r\n");
return -1;
}
--
2.7.4




Re: [PATCH v2 7/8] DM USB: xHCI: change flow of creation of virtual USB device

Yu Wang
 

On 18-08-14 21:39:13, Wu, Xiaoguang wrote:
The xHCI emulation greatly depends on the user space library libusb
which is based on the usbfs module in Linux kernel. The libusb will
bind usbfs to physical USB device which makes hardware control over
libusb in user space possible.

The pci_xhci_dev_create is called in pci_xhci_native_usb_dev_conn_cb
which is a callback function triggered by physical USB device plugging.
This function will bind the physical USB device to usbfs in SOS, which
we depend to create the communication between UOS xHCI driver with
physical USB device.

This design will fail if the reconnection happened in the SOS, which
will bind class driver to the physical USB device instead of usbfs,
hence the libusb device handle in DM is invalid.

Currently, the native S3 will disable the vbus for all xHCI ports and
re-drive during S3 resume. This behavior cause native USB driver unbind
the usbfs and bind to related class driver, then made the DM lost
control and failed to continue emulation.

To fix this issue, place the pci_xhci_dev_create in the function
pci_xhci_cmd_enable_slot. According to the xHCI spec 4.5.3 Figure 10,
the UOS always send Enable Slot command when a device is attached or
recovered from errors (by Disable Slot command). So every time the SOS
can't resuming normally or some unexpected disconnection happens, this
desigen will always survive by Disable Slot and Enable Slot command
series from UOS xHCI driver.

Signed-off-by: Xiaoguang Wu <xiaoguang.wu@...>
Reviewed-by: Liang Yang <liang3.yang@...>
---
devicemodel/hw/pci/xhci.c | 197 ++++++++++++++++++++++++++++++----------------
1 file changed, 130 insertions(+), 67 deletions(-)

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index c5fce1e..ec215a0 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -373,7 +373,10 @@ struct pci_xhci_vdev {
int (*excap_write)(struct pci_xhci_vdev *, uint64_t, uint64_t);
int usb2_port_start;
int usb3_port_start;
- uint8_t port_map_tbl[USB_NATIVE_NUM_BUS][USB_NATIVE_NUM_PORT];
+
+ uint16_t port_map_tbl[USB_NATIVE_NUM_BUS][USB_NATIVE_NUM_PORT];
+ struct usb_native_devinfo
+ native_dev_info[USB_NATIVE_NUM_BUS][USB_NATIVE_NUM_PORT];
struct timespec mf_prev_time; /* previous time of accessing MFINDEX */
};

@@ -385,8 +388,16 @@ struct pci_xhci_vdev {
#define XHCI_GADDR(xdev, a) paddr_guest2host((xdev)->dev->vmctx, (a), \
XHCI_PADDR_SZ - ((a) & (XHCI_PADDR_SZ-1)))

+/* port mapping status */
#define PT_NOT_ASSIGNED (0)
-#define PT_ASSIGNED_WITH_NO_DEV (-1)
+#define PT_ASSIGNED_WITH_NO_DEV (1)
+#define PT_ASSIGNED_WITH_NEW_DEV (2)
+#define PT_ASSIGNED_WITH_EMU_DEV (3)
Just like the previous comment. These names are not good. Please change
to:
VPORT_FREE
VPORT_ASSIGNED
VPORT_CONNECTED
VPORT_EMULATED

+
+/* helpers for get port mapping information */
+#define PT_GET_NUMBER(state) (state & 0xFF)
+#define PT_GET_STATUS(state) ((state >> 8) & 0xFF)
+#define PT_MAKE_STATE(status, num) (((status & 0xFF) << 8) | (num & 0xFF))
Don't use PT. The PT in hypervisor is means passthrough... It is easy
confused..

#define VPORT_NUM(val) (val & 0xFF)
#define VPORT_STATE(val) ((val >> 8) & 0xFF)
#define VPORT_SET_CONFIG(state, num) (((state & 0xFF) << 8) | (num & 0xFF))


struct pci_xhci_option_elem {
char *parse_opt;
@@ -470,83 +481,58 @@ static struct pci_xhci_option_elem xhci_option_table[] = {
static int
pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
{
- struct pci_xhci_dev_emu *de;
struct pci_xhci_vdev *xdev;
- struct usb_devemu *ue;
struct usb_native_devinfo *di;
- int port_start, port_end;
- int slot_start, slot_end;
- int port, slot;
- void *ud;
+ int vport_start, vport_end;
+ int port;

xdev = hci_data;
- di = dev_data;

assert(xdev);
assert(dev_data);
assert(xdev->devices);
assert(xdev->slots);

- de = pci_xhci_dev_create(xdev, di);
- if (!de) {
- UPRINTF(LFTL, "fail to create device\r\n");
- return -1;
- }
-
- /* find free port and slot for the new usb device */
- ud = de->dev_instance;
- ue = de->dev_ue;
-
- assert(ud);
- assert(ue);
+ di = dev_data;

/* print physical information about new device */
UPRINTF(LDBG, "%04x:%04x %d-%d connecting.\r\n",
di->vid, di->pid, di->bus, di->port);

- if (xdev->port_map_tbl[di->bus][di->port] == PT_NOT_ASSIGNED) {
+ if (PT_GET_STATUS(xdev->port_map_tbl[di->bus][di->port]) ==
+ PT_NOT_ASSIGNED) {
UPRINTF(LDBG, "%04x:%04x %d-%d doesn't belong to this vm, bye."
"\r\n", di->vid, di->pid, di->bus, di->port);
goto errout;
}
-
UPRINTF(LDBG, "%04x:%04x %d-%d belong to this vm.\r\n", di->vid,
di->pid, di->bus, di->port);

- if (di->bcd < 0x300)
- port_start = xdev->usb2_port_start;
- else
- port_start = xdev->usb3_port_start;
-
- slot_start = 1;
- port_end = port_start + (XHCI_MAX_DEVS / 2);
- slot_end = XHCI_MAX_SLOTS;
+ if (di->bcd < 0x300) {
+ vport_start = xdev->usb2_port_start;
+ vport_end = vport_start + (XHCI_MAX_DEVS / 2);
+ } else {
+ vport_start = xdev->usb3_port_start;
+ vport_end = vport_start + (XHCI_MAX_DEVS / 2);
+ }

/* find free port */
- for (port = port_start; port < port_end; port++)
+ for (port = vport_start; port < vport_end; port++)
if (!xdev->devices[port])
break;

- /* find free slot */
- for (slot = slot_start; slot < slot_end; slot++)
- if (!xdev->slots[slot])
- break;
-
- if (port >= port_end || slot >= slot_end) {
- UPRINTF(LFTL, "no free resource: port %d slot %d\r\n",
- port, slot);
+ if (port >= vport_end) {
+ UPRINTF(LFTL, "no free virtual port for native device %d-%d"
+ "\r\n", di->bus, di->port);
goto errout;
}

- /* use index of devices as port number */
- xdev->devices[port] = de;
- xdev->slots[slot] = de;
- xdev->ndevices++;
+ UPRINTF(LDBG, "%04X:%04X %d-%d is attached to virtual port %d.\r\n",
+ di->vid, di->pid, di->bus, di->port, port);

- pci_xhci_reset_slot(xdev, slot);
- UPRINTF(LDBG, "%X:%X %d-%d locates in slot %d port %d.\r\n",
- di->vid, di->pid, di->bus, di->port,
- slot, port);
+ xdev->native_dev_info[di->bus][di->port] = *di;
+ xdev->port_map_tbl[di->bus][di->port] =
+ PT_MAKE_STATE(PT_ASSIGNED_WITH_NEW_DEV, port);

/* Trigger port change event for the arriving device */
if (pci_xhci_connect_port(xdev, port, di->speed, 1))
@@ -554,7 +540,6 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)

return 0;
errout:
- pci_xhci_dev_destroy(de);
return -1;
}

@@ -563,8 +548,10 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
{
struct pci_xhci_vdev *xdev;
struct pci_xhci_dev_emu *edev;
+ struct usb_native_devinfo di;
struct usb_dev *udev;
- uint8_t port, native_port;
+ uint8_t port, slot, native_port;
+ uint8_t status;

assert(hci_data);
assert(dev_data);
@@ -584,8 +571,10 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
continue;

udev = edev->dev_instance;
- if (udev->info.port == native_port)
+ if (udev->info.port == native_port) {
+ di = udev->info;
break;
+ }
}

if (port == XHCI_MAX_DEVS + 1) {
@@ -593,6 +582,18 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
return -1;
}

+ for (slot = 1; slot < XHCI_MAX_SLOTS; ++slot)
+ if (xdev->slots[slot] == edev)
+ break;
+
+ assert(slot < USB_NATIVE_NUM_BUS);
+
+ status = PT_GET_STATUS(xdev->port_map_tbl[di.bus][di.port]);
+ assert(status == PT_ASSIGNED_WITH_EMU_DEV ||
+ status == PT_ASSIGNED_WITH_NEW_DEV);
Let's unify all status to state.. Don't mix them..

+ xdev->port_map_tbl[di.bus][di.port] = PT_MAKE_STATE(
+ PT_ASSIGNED_WITH_NO_DEV, port);
+
UPRINTF(LDBG, "report virtual port %d status\r\n", port);
if (pci_xhci_disconnect_port(xdev, port, 1)) {
UPRINTF(LFTL, "fail to report event\r\n");
@@ -1393,28 +1394,67 @@ done:
return err;
}

+static struct usb_native_devinfo *
+pci_xhci_find_native_devinfo(struct pci_xhci_vdev *xdev)
+{
+ int i, j;
+
+ assert(xdev);
+ for (i = 0; i < USB_NATIVE_NUM_BUS; ++i)
+ for (j = 0; j < USB_NATIVE_NUM_PORT; ++j)
+ if (PT_GET_STATUS(xdev->port_map_tbl[i][j]) ==
+ PT_ASSIGNED_WITH_NEW_DEV)
+ return &xdev->native_dev_info[i][j];
+
+ return NULL;
+}
+
static uint32_t
pci_xhci_cmd_enable_slot(struct pci_xhci_vdev *xdev, uint32_t *slot)
{
struct pci_xhci_dev_emu *dev;
- uint32_t cmderr;
- int i;
+ uint32_t cmderr;
+ struct usb_native_devinfo *di;
+ int i, vport;

cmderr = XHCI_TRB_ERROR_NO_SLOTS;
- if (xdev->portregs != NULL)
- for (i = 1; i <= XHCI_MAX_SLOTS; i++) {
- dev = XHCI_SLOTDEV_PTR(xdev, i);
- if (dev && dev->dev_slotstate == XHCI_ST_DISABLED) {
- *slot = i;
- dev->dev_slotstate = XHCI_ST_ENABLED;
- cmderr = XHCI_TRB_ERROR_SUCCESS;
- dev->hci.hci_address = i;
- break;
- }
+
+ di = pci_xhci_find_native_devinfo(xdev);
+ if (!di) {
+ UPRINTF(LWRN, "unexpected Enable Slot commnad\r\n");
+ return -1;
+ }
+
+ assert(di->priv_data);
+ dev = pci_xhci_dev_create(xdev, di);
+ if (!dev) {
+ UPRINTF(LFTL, "fail to create device\r\n");
+ return -1;
+ }
+
+ vport = PT_GET_NUMBER(xdev->port_map_tbl[di->bus][di->port]);
+ assert(vport > 0);
+ assert(!xdev->devices[vport]);
+
+ xdev->devices[vport] = dev;
+ xdev->ndevices++;
+
+ for (i = 1; i <= XHCI_MAX_SLOTS; i++) {
+ if (XHCI_SLOTDEV_PTR(xdev, i) == NULL) {
+ xdev->slots[i] = dev;
+ *slot = i;
+ dev->dev_slotstate = XHCI_ST_ENABLED;
+ cmderr = XHCI_TRB_ERROR_SUCCESS;
+ dev->hci.hci_address = i;
+ xdev->port_map_tbl[di->bus][di->port] =
+ PT_MAKE_STATE(PT_ASSIGNED_WITH_EMU_DEV, vport);
+ break;
}
+ }

- UPRINTF(LDBG, "enable slot (error=%d) slot %u\r\n",
- cmderr != XHCI_TRB_ERROR_SUCCESS, *slot);
+ UPRINTF(LDBG, "enable slot (error=%d) slot %u for native device "
+ "%d-%d\r\n", cmderr != XHCI_TRB_ERROR_SUCCESS, *slot,
+ di->bus, di->port);

return cmderr;
}
@@ -1423,6 +1463,8 @@ static uint32_t
pci_xhci_cmd_disable_slot(struct pci_xhci_vdev *xdev, uint32_t slot)
{
struct pci_xhci_dev_emu *dev;
+ struct usb_dev *udev;
+ struct usb_native_devinfo *di;
uint32_t cmderr;
int i;

@@ -1446,6 +1488,9 @@ pci_xhci_cmd_disable_slot(struct pci_xhci_vdev *xdev, uint32_t slot)
cmderr = XHCI_TRB_ERROR_SUCCESS;
/* TODO: reset events and endpoints */
}
+ } else {
+ UPRINTF(LDBG, "disable NULL device, slot %d\r\n", slot);
+ goto done;
}

for (i = 1; i <= XHCI_MAX_DEVS; ++i)
@@ -1455,8 +1500,20 @@ pci_xhci_cmd_disable_slot(struct pci_xhci_vdev *xdev, uint32_t slot)
if (i <= XHCI_MAX_DEVS && XHCI_PORTREG_PTR(xdev, i)) {
XHCI_PORTREG_PTR(xdev, i)->portsc &= ~(XHCI_PS_CSC |
XHCI_PS_CCS | XHCI_PS_PED | XHCI_PS_PP);
+
+ udev = dev->dev_instance;
+ assert(udev);
+
xdev->devices[i] = NULL;
xdev->slots[slot] = NULL;
+
+ di = &udev->info;
+ xdev->port_map_tbl[di->bus][di->port] =
+ PT_MAKE_STATE(PT_ASSIGNED_WITH_NO_DEV, 0);
+
+ UPRINTF(LINF, "disable slot %d for native device %d-%d"
+ "\r\n", slot, di->bus, di->port);
+
pci_xhci_dev_destroy(dev);
} else
UPRINTF(LWRN, "invalid slot %d\r\n", slot);
@@ -1624,7 +1681,10 @@ pci_xhci_cmd_config_ep(struct pci_xhci_vdev *xdev,
UPRINTF(LDBG, "config_ep slot %u\r\n", slot);

dev = XHCI_SLOTDEV_PTR(xdev, slot);
- assert(dev != NULL);
+ if (dev == NULL) {
+ cmderr = XHCI_TRB_ERROR_SLOT_NOT_ON;
+ goto done;
+ }

if ((trb->dwTrb3 & XHCI_TRB_3_DCEP_BIT) != 0) {
UPRINTF(LDBG, "config_ep - deconfigure ep slot %u\r\n", slot);
@@ -3364,7 +3424,10 @@ pci_xhci_parse_bus_port(struct pci_xhci_vdev *xdev, char *opts)
goto errout;
}

- xdev->port_map_tbl[bus][port] = PT_ASSIGNED_WITH_NO_DEV;
+ xdev->port_map_tbl[bus][port] =
+ PT_MAKE_STATE(PT_ASSIGNED_WITH_NO_DEV, 0);
+ return 0;
+
errout:
if (rc)
UPRINTF(LWRN, "%s fails, rc=%d\r\n", __func__, rc);
--
2.7.4




Re: [PATCH v2 6/8] DM USB: xHCI: code cleanup: change variable name

Yu Wang
 

Acked-by: Yu Wang <yu1.wang@...>

On 18-08-14 21:39:12, Wu, Xiaoguang wrote:
Replace 'native_assign_ports' with 'port_map_tbl' to be more accurate
for the role of this variable plays.

Signed-off-by: Xiaoguang Wu <xiaoguang.wu@...>
Reviewed-by: Liang Yang <liang3.yang@...>
---
devicemodel/hw/pci/xhci.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index b5d8555..c5fce1e 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -373,7 +373,7 @@ struct pci_xhci_vdev {
int (*excap_write)(struct pci_xhci_vdev *, uint64_t, uint64_t);
int usb2_port_start;
int usb3_port_start;
- uint8_t native_assign_ports[USB_NATIVE_NUM_BUS][USB_NATIVE_NUM_PORT];
+ uint8_t port_map_tbl[USB_NATIVE_NUM_BUS][USB_NATIVE_NUM_PORT];
struct timespec mf_prev_time; /* previous time of accessing MFINDEX */
};

@@ -504,7 +504,7 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
UPRINTF(LDBG, "%04x:%04x %d-%d connecting.\r\n",
di->vid, di->pid, di->bus, di->port);

- if (xdev->native_assign_ports[di->bus][di->port] == PT_NOT_ASSIGNED) {
+ if (xdev->port_map_tbl[di->bus][di->port] == PT_NOT_ASSIGNED) {
UPRINTF(LDBG, "%04x:%04x %d-%d doesn't belong to this vm, bye."
"\r\n", di->vid, di->pid, di->bus, di->port);
goto errout;
@@ -3364,7 +3364,7 @@ pci_xhci_parse_bus_port(struct pci_xhci_vdev *xdev, char *opts)
goto errout;
}

- xdev->native_assign_ports[bus][port] = PT_ASSIGNED_WITH_NO_DEV;
+ xdev->port_map_tbl[bus][port] = PT_ASSIGNED_WITH_NO_DEV;
errout:
if (rc)
UPRINTF(LWRN, "%s fails, rc=%d\r\n", __func__, rc);
--
2.7.4