Date   

Re: [PATCH 1/2] vhm: add ioeventfd support for ACRN hypervisor service module

Shuo A Liu
 

On Mon 13.Aug'18 at 17:03:44 +0800, Jie Deng wrote:

On 2018/8/13 14:40, Shuo Liu wrote:
+
+static struct acrn_vhm_ioeventfd *vhm_ioeventfd_match(
+ struct vhm_ioeventfd_info *info,
+ u64 addr, u64 data, int length, int type)
+{
+ struct acrn_vhm_ioeventfd *p = NULL;
+
+ list_for_each_entry(p, &info->ioeventfds, list) {
+ if (p->type == type &&
+ p->addr == addr &&
+ p->length == length &&
+ (p->data == data || p->wildcard))
+ return p;
Is it better to be "(p->wildcard || p->data == data)" ?
Sure. I will change to
(p->wildcard || p->data == data)

Thanks




[PATCH v2] 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.

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 0/3] add hkdf key derivation support based on mbedtls

Zhu, Bing
 

The loc of code is 1731. (most of ~5900 loc are comments)

-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
C 5 323 155 1359
C/C++ Header 5 288 3428 372
-------------------------------------------------------------------------------
SUM: 10 611 3583 1731
-------------------------------------------------------------------------------

-----Original Message-----
From: Xu, Anthony
Sent: Tuesday, August 14, 2018 2:31 AM
To: Zhu, Bing <bing.zhu@...>; Dong, Eddie <eddie.dong@...>; acrn-
dev@...
Cc: Wang, Kai Z <kai.z.wang@...>; Chen, Gang G <gang.g.chen@...>
Subject: RE: [acrn-dev] [PATCH 0/3] add hkdf key derivation support based on
mbedtls

15 files changed, 5975 insertions(+), 70 deletions(-)

This patch adds ~ 5900 LOC. Way too big, it increases the ACRN hypervisor code
size by > 20%. that's my concern.

Any other solutions?
SOS is a special VM, If we let SOS know the physical platform, what's the real
impact?


Anthony

-----Original Message-----
From: Zhu, Bing
Sent: Saturday, August 11, 2018 3:09 AM
To: Dong, Eddie <eddie.dong@...>; Xu, Anthony
<anthony.xu@...>; acrn-dev@...
Cc: Wang, Kai Z <kai.z.wang@...>; Chen, Gang G
<gang.g.chen@...>
Subject: RE: [acrn-dev] [PATCH 0/3] add hkdf key derivation support
based on mbedtls

Right, and the key derivation is pretty fast, and this operation is
called only whenever a new UOS starts, just only once.

SOS, like any other guest VMs, must not have knowledge of that
physical platform secret (this secret behaves a root key/secret from
VM's perspective)

Bing

-----Original Message-----
From: Dong, Eddie
Sent: Saturday, August 11, 2018 6:05 PM
To: Xu, Anthony <anthony.xu@...>;
acrn-dev@...
Cc: Zhu, Bing <bing.zhu@...>; Wang, Kai Z
<kai.z.wang@...>;
Chen,
Gang G <gang.g.chen@...>
Subject: RE: [acrn-dev] [PATCH 0/3] add hkdf key derivation support
based
on
mbedtls


Is this operation performance critical?
Can we let SOS do the en/decryption through a new request type?
It is not performance critical, but we cannot let SOS know the
secret of
physical
platform.


Re: [RFC PATCH 2/2] hv: add a dummy file to do compile time assert

Xu, Anthony
 

Can we generate the OFFSET MACRO at build time?

Anthony

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Junjie Mao
Sent: Monday, August 13, 2018 3:24 AM
To: Wu, Binbin <binbin.wu@...>
Cc: acrn-dev@...
Subject: Re: [acrn-dev] [RFC PATCH 2/2] hv: add a dummy file to do compile
time assert

"Wu, Binbin" <binbin.wu@...> writes:

Add a dummy file to do compile time assert for member offset within a
struct.
If the statement is not true, there will be error during compile time.
The file will not increase the size of HV binary.

Signed-off-by: Binbin Wu <binbin.wu@...>
Reviewed-by: Junjie Mao <junjie.mao@...>

This looks good to me as it is explicitly required that constant array
sizes shall be positive in clause 2, section 6.7.5.2, C99.

---
hypervisor/Makefile | 1 +
hypervisor/arch/x86/dummy.c | 24 ++++++++++++++++++++++++
2 files changed, 25 insertions(+)
create mode 100644 hypervisor/arch/x86/dummy.c

diff --git a/hypervisor/Makefile b/hypervisor/Makefile
index b0588b7..81d23bd 100644
--- a/hypervisor/Makefile
+++ b/hypervisor/Makefile
@@ -140,6 +140,7 @@ C_SRCS += arch/x86/cpu_state_tbl.c
C_SRCS += arch/x86/mtrr.c
C_SRCS += arch/x86/pm.c
S_SRCS += arch/x86/wakeup.S
+C_SRCS += arch/x86/dummy.c
C_SRCS += arch/x86/guest/vcpu.c
C_SRCS += arch/x86/guest/vm.c
C_SRCS += arch/x86/guest/vlapic.c
diff --git a/hypervisor/arch/x86/dummy.c b/hypervisor/arch/x86/dummy.c
new file mode 100644
index 0000000..d1b5ca3
--- /dev/null
+++ b/hypervisor/arch/x86/dummy.c
@@ -0,0 +1,24 @@
+#include <lib/types.h>
+#include <lib/util.h>
+#include <vm0_boot.h>
+
+#define CAT_(A,B) A ## B
+#define CTASSERT(expr) \
+typedef int CAT_(CTA_DummyType,__LINE__)[(expr) ? 1 : -1]
+
+CTASSERT(BOOT_CTX_CR0_OFFSET == offsetof(struct boot_ctx, cr0));
+CTASSERT(BOOT_CTX_CR3_OFFSET == offsetof(struct boot_ctx, cr3));
+CTASSERT(BOOT_CTX_CR4_OFFSET == offsetof(struct boot_ctx, cr4));
+CTASSERT(BOOT_CTX_IDT_OFFSET == offsetof(struct boot_ctx, idt));
+CTASSERT(BOOT_CTX_GDT_OFFSET == offsetof(struct boot_ctx, gdt));
+CTASSERT(BOOT_CTX_LDT_SEL_OFFSET == offsetof(struct boot_ctx,
ldt_sel));
+CTASSERT(BOOT_CTX_TR_SEL_OFFSET == offsetof(struct boot_ctx,
tr_sel));
+CTASSERT(BOOT_CTX_CS_SEL_OFFSET == offsetof(struct boot_ctx,
cs_sel));
+CTASSERT(BOOT_CTX_SS_SEL_OFFSET == offsetof(struct boot_ctx,
ss_sel));
+CTASSERT(BOOT_CTX_DS_SEL_OFFSET == offsetof(struct boot_ctx,
ds_sel));
+CTASSERT(BOOT_CTX_ES_SEL_OFFSET == offsetof(struct boot_ctx,
es_sel));
+CTASSERT(BOOT_CTX_FS_SEL_OFFSET == offsetof(struct boot_ctx,
fs_sel));
+CTASSERT(BOOT_CTX_GS_SEL_OFFSET == offsetof(struct boot_ctx,
gs_sel));
+CTASSERT(BOOT_CTX_CS_AR_OFFSET == offsetof(struct boot_ctx,
cs_ar));
+CTASSERT(BOOT_CTX_EFER_LOW_OFFSET == offsetof(struct boot_ctx,
ia32_efer));
+CTASSERT(BOOT_CTX_EFER_HIGH_OFFSET == offsetof(struct boot_ctx,
ia32_efer) + 4);
--
2.7.4




Re: [PATCH 0/3] add hkdf key derivation support based on mbedtls

Xu, Anthony
 

15 files changed, 5975 insertions(+), 70 deletions(-)

This patch adds ~ 5900 LOC. Way too big, it increases the ACRN hypervisor code size by > 20%. that's my concern.

Any other solutions?
SOS is a special VM, If we let SOS know the physical platform, what's the real impact?


Anthony

-----Original Message-----
From: Zhu, Bing
Sent: Saturday, August 11, 2018 3:09 AM
To: Dong, Eddie <eddie.dong@...>; Xu, Anthony
<anthony.xu@...>; acrn-dev@...
Cc: Wang, Kai Z <kai.z.wang@...>; Chen, Gang G
<gang.g.chen@...>
Subject: RE: [acrn-dev] [PATCH 0/3] add hkdf key derivation support based
on mbedtls

Right, and the key derivation is pretty fast, and this operation is called only
whenever a new UOS starts, just only once.

SOS, like any other guest VMs, must not have knowledge of that physical
platform secret (this secret behaves a root key/secret from VM's perspective)

Bing

-----Original Message-----
From: Dong, Eddie
Sent: Saturday, August 11, 2018 6:05 PM
To: Xu, Anthony <anthony.xu@...>; acrn-dev@...
Cc: Zhu, Bing <bing.zhu@...>; Wang, Kai Z <kai.z.wang@...>;
Chen,
Gang G <gang.g.chen@...>
Subject: RE: [acrn-dev] [PATCH 0/3] add hkdf key derivation support based
on
mbedtls


Is this operation performance critical?
Can we let SOS do the en/decryption through a new request type?
It is not performance critical, but we cannot let SOS know the secret of
physical
platform.


Re: [PATCH v2] tools: acrntrace: Add ring buffer mode

Eddie Dong
 

We don't want to complicate the debug tools too much. I remember we can already set the size of the log file. That is enough.

Thx Eddie

-----Original Message-----
From: acrn-dev@...
[mailto:acrn-dev@...] On Behalf Of Zhipeng Gong
Sent: Monday, August 13, 2018 9:31 PM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH v2] tools: acrntrace: Add ring buffer mode

When running longevity test and capturing acrntrace, generated acrntrace
files sizes are too big.
Sometimes we don't care very old trace. This patch adds ring buffer mode,
fixes acrntrace file size and overwrites the oldest trace with new trace when
the buffer is full.

v2:
- update README.rst

Signed-off-by: Zhipeng Gong <zhipeng.gong@...>
---
tools/acrntrace/README.rst | 4 ++--
tools/acrntrace/acrntrace.c | 44
+++++++++++++++++++++++++++++++++++++++++---
tools/acrntrace/acrntrace.h | 1 +
3 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/tools/acrntrace/README.rst b/tools/acrntrace/README.rst index
433f161..f25c5b0 100644
--- a/tools/acrntrace/README.rst
+++ b/tools/acrntrace/README.rst
@@ -21,8 +21,8 @@ Options:
-i period specify polling interval in milliseconds [1-999]
-t max_time max time to capture trace data (in second)
-c clear the buffered old data
--r free_space amount of free space (in MB) remaining on the
disk
- before acrntrace stops
+-r ring_buffer_size ring buffer size (in MB), which enables ring buffer
+ mode and overwrites old trace with new
trace
+when full

The ``acrntrace_format.py`` is a offline tool for parsing trace data (as
output by acrntrace) to human-readable formats based on given format.
diff --git a/tools/acrntrace/acrntrace.c b/tools/acrntrace/acrntrace.c index
39fda35..e4b3899 100644
--- a/tools/acrntrace/acrntrace.c
+++ b/tools/acrntrace/acrntrace.c
@@ -37,16 +37,19 @@ static char trace_file_dir[TRACE_FILE_DIR_LEN];

static reader_struct *reader;
static int pcpu_num = 0;
+static int ring_buffer_size = 0;

static void display_usage(void)
{
printf("acrntrace - tool to collect ACRN trace data\n"
- "[Usage] acrntrace [-i] [period in msec] [-ch]\n\n"
+ "[Usage] acrntrace [-i] [period in msec] [-r] [ring buffer size]
[-ch]\n\n"
"[Options]\n"
"\t-h: print this message\n"
"\t-i: period_in_ms: specify polling interval [1-999]\n"
"\t-t: max time to capture trace data (in second)\n"
- "\t-c: clear the buffered old data\n");
+ "\t-c: clear the buffered old data\n"
+ "\t-r: ring_buffer_size (in MB), which enables ring buffer mode"
+ " and overwrites old trace with new trace when full\n");
}

static void timer_handler(union sigval sv) @@ -113,6 +116,15 @@ static int
parse_opt(int argc, char *argv[])
timeout = ret;
pr_dbg("Capture trace data for at most %ds\n", ret);
break;
+ case 'r':
+ ret = atoi(optarg);
+ if (ret <= 0) {
+ pr_err("'-r' require integer greater than 0\n");
+ return -EINVAL;
+ }
+ ring_buffer_size = ret * 1024 * 1024;
+ pr_dbg("Ring buffer size is %dM\n", ret);
+ break;
case 'c':
flags |= FLAG_CLEAR_BUF;
break;
@@ -206,6 +218,15 @@ static void reader_fn(param_t * param)
trace_ev_t e;
struct statvfs stat;
uint64_t freespace;
+ int pos = 0;
+
+ if (ring_buffer_size != 0) {
+ param->buffer = malloc(ring_buffer_size);
+ if (!param->buffer) {
+ perror("Failed to allocate ring buffer\n");
+ return;
+ }
+ }

pr_dbg("reader thread[%lu] created for FILE*[0x%p]\n",
pthread_self(), fp);
@@ -219,7 +240,13 @@ static void reader_fn(param_t * param)

while (1) {
do {
- ret = sbuf_write(fd, sbuf);
+ if (ring_buffer_size != 0) {
+ ret = sbuf_get(sbuf, param->buffer + pos);
+ pos += sbuf->ele_size;
+ if (pos + sbuf->ele_size > ring_buffer_size)
+ pos = 0;
+ } else
+ ret = sbuf_write(fd, sbuf);
} while (ret > 0);

usleep(period);
@@ -284,6 +311,17 @@ static void destory_reader(reader_struct * reader)
reader->thrd = 0;
}

+ if (ring_buffer_size != 0 && reader->param.buffer) {
+ int ret;
+
+ ret = write(reader->param.trace_fd, reader->param.buffer,
+ ring_buffer_size);
+ if (ret != ring_buffer_size) {
+ perror("Failed to write ring buffer\n");
+ }
+ free(reader->param.buffer);
+ }
+
if (reader->param.sbuf) {
munmap(reader->param.sbuf, MMAP_SIZE);
reader->param.sbuf = NULL;
diff --git a/tools/acrntrace/acrntrace.h b/tools/acrntrace/acrntrace.h index
ea54f4d..6615157 100644
--- a/tools/acrntrace/acrntrace.h
+++ b/tools/acrntrace/acrntrace.h
@@ -76,6 +76,7 @@ typedef struct {
int trace_fd;
shared_buf_t *sbuf;
pthread_mutex_t *sbuf_lock;
+ uint8_t *buffer;
} param_t;

typedef struct {
--
2.7.4



Re: [PATCH] hv: treewide: fix Macro redefine, usage -- and operation violations

Eddie Dong
 

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

+ d = d-1;
This seems to be better to be "d--".


-----Original Message-----
From: acrn-dev@...
[mailto:acrn-dev@...] On Behalf Of junjunshan1
Sent: Monday, August 13, 2018 7:39 PM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH] hv: treewide: fix Macro redefine, usage -- and
operation violations

MISARC has requirements about Marco redefinition, usage of ++ or -- and
assignment operator in boolean expression.
This patch is used to solve these violations.

The modifications are summarized as following:
1.The HC_VM_SET_MEMORY_REGION, HC_VM_GPA2HPA,
HC_VM_SET_MEMORY_REGIONS are redefined twice in acrn_hv_des.h, so
delete them to solve the macro redefinition violations.
2.The macro BUS_LOCK are redefined in bits.h and atomic.h, then delete the
declaration in both two files, add a new declaration in cpu.h and include the
header file.
3.modify the code to deprecate the usage of -- operators in string.c.
4.modify the while loop to for loop to avoid assignment operator in boolean
expression in vlapic.c.

Signed-off-by: junjunshan1 <junjun.shan@...>
---
hypervisor/arch/x86/guest/vlapic.c | 3 ++-
hypervisor/include/arch/x86/cpu.h | 2 ++
hypervisor/include/lib/atomic.h | 3 +--
hypervisor/include/lib/bits.h | 3 +--
hypervisor/include/public/acrn_hv_defs.h | 3 ---
hypervisor/lib/string.c | 6 ++++--
6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hypervisor/arch/x86/guest/vlapic.c
b/hypervisor/arch/x86/guest/vlapic.c
index 35be79c..72acb76 100644
--- a/hypervisor/arch/x86/guest/vlapic.c
+++ b/hypervisor/arch/x86/guest/vlapic.c
@@ -1135,7 +1135,8 @@ vlapic_icrlo_write_handler(struct acrn_vlapic
*vlapic)
break;
}

- while ((vcpu_id = ffs64(dmask)) != INVALID_BIT_INDEX) {
+ for (vcpu_id = ffs64(dmask); vcpu_id != INVALID_BIT_INDEX;
+ vcpu_id = ffs64(dmask)) {
bitmap_clear_lock(vcpu_id, &dmask);
target_vcpu = vcpu_from_vid(vlapic->vm, vcpu_id);
if (target_vcpu == NULL) {
diff --git a/hypervisor/include/arch/x86/cpu.h
b/hypervisor/include/arch/x86/cpu.h
index e56f443..d24228d 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -152,6 +152,8 @@

#ifndef ASSEMBLER

+#define BUS_LOCK "lock ; "
+
/**
*
* Identifiers for architecturally defined registers.
diff --git a/hypervisor/include/lib/atomic.h
b/hypervisor/include/lib/atomic.h
index 264917c..6085863 100644
--- a/hypervisor/include/lib/atomic.h
+++ b/hypervisor/include/lib/atomic.h
@@ -29,8 +29,7 @@

#ifndef ATOMIC_H
#define ATOMIC_H
-
-#define BUS_LOCK "lock ; "
+#include <cpu.h>

#define build_atomic_load(name, size, type, ptr) \
static inline type name(const volatile type *ptr) \
diff --git a/hypervisor/include/lib/bits.h b/hypervisor/include/lib/bits.h
index 04a5057..ea0e8a1 100644
--- a/hypervisor/include/lib/bits.h
+++ b/hypervisor/include/lib/bits.h
@@ -29,8 +29,7 @@

#ifndef BITS_H
#define BITS_H
-
-#define BUS_LOCK "lock ; "
+#include <cpu.h>
/**
*
* INVALID_BIT_INDEX means when input paramter is zero,
diff --git a/hypervisor/include/public/acrn_hv_defs.h
b/hypervisor/include/public/acrn_hv_defs.h
index cfeb834..700ce92 100644
--- a/hypervisor/include/public/acrn_hv_defs.h
+++ b/hypervisor/include/public/acrn_hv_defs.h
@@ -55,9 +55,6 @@
#define HC_VM_GPA2HPA BASE_HC_ID(HC_ID,
HC_ID_MEM_BASE + 0x01UL)
#define HC_VM_SET_MEMORY_REGIONS BASE_HC_ID(HC_ID,
HC_ID_MEM_BASE + 0x02UL)
#define HC_VM_WRITE_PROTECT_PAGE BASE_HC_ID(HC_ID,
HC_ID_MEM_BASE + 0x03UL)
-#define HC_VM_SET_MEMORY_REGION BASE_HC_ID(HC_ID,
HC_ID_MEM_BASE + 0x00UL)
-#define HC_VM_GPA2HPA BASE_HC_ID(HC_ID,
HC_ID_MEM_BASE + 0x01UL)
-#define HC_VM_SET_MEMORY_REGIONS BASE_HC_ID(HC_ID,
HC_ID_MEM_BASE + 0x02UL)

/* PCI assignment*/
#define HC_ID_PCI_BASE 0x50UL
diff --git a/hypervisor/lib/string.c b/hypervisor/lib/string.c
index db4f145..7d2a5a5 100644
--- a/hypervisor/lib/string.c
+++ b/hypervisor/lib/string.c
@@ -210,7 +210,8 @@ char *strcpy_s(char *d_arg, size_t dmax, const char
*s_arg)
while (dest_avail > 0U) {
if (overlap_guard == 0U) {
pr_err("%s: overlap happened.", __func__);
- *(--d) = '\0';
+ d = d-1;
+ *d = '\0';
return NULL;
}

@@ -293,7 +294,8 @@ char *strncpy_s(char *d_arg, size_t dmax, const
char *s_arg, size_t slen_arg)
while (dest_avail > 0U) {
if (overlap_guard == 0U) {
pr_err("%s: overlap happened.", __func__);
- *(--d) = '\0';
+ d = d-1;
+ *d = '\0';
return NULL;
}

--
2.7.4



Re: [PATCH v2 1/3] HV: Add the emulation of CPUID with 0x16 leaf

Li, Fei1
 

On Mon, Aug 13, 2018 at 10:18:24AM +0000, Zhao, Yakui wrote:


-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Eddie Dong
Sent: Monday, August 13, 2018 4:04 PM
To: acrn-dev@...; Xu, Anthony <anthony.xu@...>
Subject: Re: [acrn-dev] [PATCH v2 1/3] HV: Add the emulation of CPUID with
0x16 leaf

Hi Yakui,

ACRN doesn't emulate CPUID >=0x80000000U at all.
So for CPUID >=0x80000000U, ACRN just read from hardware in
guest_cpuid().
init_vcpuid_entry doesn't need to handle CPUID >=0x80000000U.
After Eddie's suggestion is followed to add the limit check(>=0x15
CPUID), the mentioned check is not needed any more.

I keep them so that the init_vcpuid_entry still can return the zero
entry for the out-of-range CPUID input.

If you think that the check is redundant, I can remove them.
The vcpuid base on these assumption:
(1) if we don't plan to support this feature, we could filter it.
(2) if the leaf greater than the limit (base or enternal), we should return the value in the limit leaf.
(3) If we plan to emulate some feature, like the cpu frequency, we could add an emulate entry for it.
So init_vcpuid_entry only needs to take care the leaf which needs to emulate/fliter/disable
some features. I don't think what you did about leaf over 0x80000000U lie in this scope.
Thanks.
If it is physical out of ranger, returning what hardware returns is better than
fixed-zero.

We want the code to be slim to save FuSa effort.
OK. I will remove them.


Thx Eddie




Re: [PATCH 5/6] DM USB: xHCI: change flow of creation of virtual USB device

Yu Wang
 

On 18-08-13 16:48:34, 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 disconnection happened in the SOS, which
I think it should be re-connection instead of disconnection, right?

will bind class driver to the physical USB device instead of usbfs,
hence the libusb device handle in DM is invalid.

Currently when the SOS do the resuming from S3, there are some uncertain
bugs which unbind the usbfs for the USB devices, so the DM will lose
control and can't continue emulation.
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 | 220 +++++++++++++++++++++++++++++++++-------------
1 file changed, 158 insertions(+), 62 deletions(-)

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index 2df7ce3..579021a 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -383,6 +383,7 @@ struct pci_xhci_vdev {
* is stored in native_assign_ports[x][y].
*/
int8_t *native_assign_ports[USB_NATIVE_NUM_BUS];
+ struct usb_native_devinfo *native_assign_info[USB_NATIVE_NUM_BUS];
Can we merge the native_assign_ports and usb_native_devinfo?... Why the
usb_native_devinfo can't include the port info...

And I just realized the USB_NATIVE_NUM_BUS... Why it is too huge.. 255
USB buses?...

struct timespec mf_prev_time; /* previous time of accessing MFINDEX */
};

@@ -475,36 +476,21 @@ 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 port;
int intr = 1;
+ int i, j;

xdev = hci_data;
- di = dev_data;

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

- de = pci_xhci_dev_create(xdev, di->priv_data);
- 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",
@@ -517,43 +503,53 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
goto errout;
}

+ assert(xdev->native_assign_info[di->bus]);
UPRINTF(LDBG, "%04x:%04x %d-%d belong to this vm.\r\n", di->vid,
di->pid, di->bus, di->port);

- if (di->bcd < 0x300)
+ if (di->bcd < 0x300) {
port_start = xdev->usb2_port_start;
- else
+ port_end = port_start + (XHCI_MAX_DEVS / 2);
+ } else {
port_start = xdev->usb3_port_start;
Please change the port_start/port_end to vport_start/vport_end.
Otherwise, it is easy messed with native ports..

-
- slot_start = 1;
- port_end = port_start + (XHCI_MAX_DEVS / 2);
- slot_end = XHCI_MAX_SLOTS;
+ port_end = port_start + (XHCI_MAX_DEVS / 2);
+ }

/* find free port */
- for (port = port_start; port < port_end; port++)
- if (!xdev->devices[port])
- break;
+ /* FIXME:
+ * Will find a decent way to deal with the relationship among Slot,
+ * Port and Device.
+ */
+ for (port = port_start; port < port_end; port++) {
+ if (xdev->devices[port])
+ continue;
+
+ for (i = 0; i < USB_NATIVE_NUM_BUS; ++i) {
+ if (!xdev->native_assign_ports[i])
+ continue;
+
+ for (j = 0; j < USB_NATIVE_NUM_PORT; ++j)
+ if (xdev->native_assign_ports[i][j] == port)
+ break;
Confused, you are trying to find one free virtual port? Why every native
bus only allow to as one virtual port? Totally confused now.


- /* find free slot */
- for (slot = slot_start; slot < slot_end; slot++)
- if (!xdev->slots[slot])
+ if (j < USB_NATIVE_NUM_PORT)
+ break;
+ }
+ if (i >= USB_NATIVE_NUM_BUS)
break;
+ }
We need to rework this loop-loop-loop-if-if-if code... It is too ugly
and confused.. At least, please define one function for it with readily
comprehensible function name.


- if (port >= port_end || slot >= slot_end) {
- UPRINTF(LFTL, "no free resource: port %d slot %d\r\n",
- port, slot);
+ if (port >= port_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, "%X:%X %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_assign_ports[di->bus][di->port] = port;
+ xdev->native_assign_info[di->bus][di->port] = *di;

/* Trigger port change event for the arriving device */
if (pci_xhci_port_connect(xdev, port, di->speed, intr))
@@ -561,7 +557,6 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)

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

@@ -571,8 +566,8 @@ 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_dev *udev;
- uint8_t port, native_port;
- int intr = 1;
+ uint8_t port, slot, native_port;
+ int i, j, intr = 1;

assert(hci_data);
assert(dev_data);
@@ -601,6 +596,24 @@ 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;
+
+ /* FIXME: again, find a better relationship for Slot, Port and Device */
+ for (i = 0; i < USB_NATIVE_NUM_BUS; ++i) {
+ if (!xdev->native_assign_ports[i])
+ continue;
+
+ for (j = 0; j < USB_NATIVE_NUM_PORT; ++j)
+ if (xdev->native_assign_ports[i][j] == port)
+ break;
+
+ if (j < USB_NATIVE_NUM_PORT)
+ break;
+ }
Ditto.

+ assert(i < USB_NATIVE_NUM_BUS);
+ xdev->native_assign_ports[i][j] = -1;
Define one macro instead of -1.

UPRINTF(LDBG, "report virtual port %d status\r\n", port);
if (pci_xhci_port_disconnect(xdev, port, intr)) {
UPRINTF(LFTL, "fail to report event\r\n");
@@ -1405,21 +1418,61 @@ 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, j, port;

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;
+
+ for (i = 0; i < USB_NATIVE_NUM_BUS; ++i) {
+ if (!xdev->native_assign_ports[i])
+ continue;
+
+ for (j = 0; j < USB_NATIVE_NUM_PORT; ++j) {
+ if (xdev->native_assign_ports[i][j] > 0) {
+ port = xdev->native_assign_ports[i][j];
+ assert(port <= XHCI_MAX_DEVS);
+
+ if (!xdev->devices[port])
+ break;
}
}
+ if (j < USB_NATIVE_NUM_PORT)
+ break;
Ditto

+ }
+
+ if (i >= USB_NATIVE_NUM_BUS) {
+ UPRINTF(LWRN, "unexpected Enable Slot commnad\r\n");
+ return -1;
+ }
+
+ assert(xdev->native_assign_info[i]);
+ di = &xdev->native_assign_info[i][j];
+
+ assert(di->priv_data);
+ dev = pci_xhci_dev_create(xdev, di->priv_data);
+ if (!dev) {
+ UPRINTF(LFTL, "fail to create device\r\n");
+ return -1;
+ }
+
+ port = xdev->native_assign_ports[i][j];
+ assert(port > 0);
+ assert(!xdev->devices[port]);
+
+ xdev->devices[port] = 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;
+ break;
+ }
+ }

UPRINTF(LDBG, "enable slot (error=%d) slot %u\r\n",
cmderr != XHCI_TRB_ERROR_SUCCESS, *slot);
@@ -1431,7 +1484,10 @@ static uint32_t
pci_xhci_cmd_disable_slot(struct pci_xhci_vdev *xdev, uint32_t slot)
{
struct pci_xhci_dev_emu *dev;
+ struct usb_devemu *ue;
+ uint8_t native_port, native_bus;
uint32_t cmderr;
+ void *ud;
int i;

UPRINTF(LDBG, "pci_xhci disable slot %u\r\n", slot);
@@ -1454,6 +1510,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)
@@ -1463,8 +1522,25 @@ 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);
+
+ ud = dev->dev_instance;
+ ue = dev->dev_ue;
+
+ assert(ud);
+ assert(ue);
+
+ ue->ue_info(ud, USB_INFO_BUS, &native_bus, sizeof(uint8_t));
+ ue->ue_info(ud, USB_INFO_PORT, &native_port, sizeof(uint8_t));
+
xdev->devices[i] = NULL;
xdev->slots[slot] = NULL;
+
+ assert(xdev->native_assign_ports[native_bus]);
+ assert(xdev->native_assign_ports[native_bus][native_port]);
+ UPRINTF(LINF, "disable slot %d for native device %d-%d"
+ "\r\n", slot, native_bus, native_port);
+
+ xdev->native_assign_ports[native_bus][native_port] = -1;
pci_xhci_dev_destroy(dev);
} else
UPRINTF(LWRN, "invalid slot %d\r\n", slot);
@@ -1632,7 +1708,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);
@@ -3352,8 +3431,10 @@ errout:
static int
pci_xhci_parse_bus_port(struct pci_xhci_vdev *xdev, char *opts)
{
- int rc = 0, cnt;
+ int rc = 0, cnt, portcnt;
uint32_t port, bus;
+ int8_t *pports;
+ struct usb_native_devinfo *pinfo;

assert(xdev);
assert(opts);
@@ -3372,15 +3453,30 @@ pci_xhci_parse_bus_port(struct pci_xhci_vdev *xdev, char *opts)
goto errout;
}

- if (!xdev->native_assign_ports[bus]) {
- xdev->native_assign_ports[bus] = calloc(USB_NATIVE_NUM_PORT,
- sizeof(int8_t));
- if (!xdev->native_assign_ports[bus]) {
+ pports = xdev->native_assign_ports[bus];
+ pinfo = xdev->native_assign_info[bus];
+
+ /* those two pointers should always be consistant */
+ assert((!pports) == (!pinfo));
+
+ if (!pports) {
+ portcnt = USB_NATIVE_NUM_PORT;
+ pports = calloc(portcnt, sizeof(uint8_t));
+ pinfo = calloc(portcnt, sizeof(struct usb_native_devinfo));
+ if (!pports || !pinfo) {
rc = -3;
- goto errout;
+ goto release_out;
}
+
+ xdev->native_assign_ports[bus] = pports;
+ xdev->native_assign_info[bus] = pinfo;
}
xdev->native_assign_ports[bus][port] = -1;
+ return 0;
+
+release_out:
+ free(pinfo);
+ free(pports);

errout:
if (rc)
--
2.7.4




Re: [PATCH v2] tools:acrn-crashlog: Document of configuration file

Kinder, David B
 

I don't see a PR for this in GitHub. Can it be submitted there for review?
-- david

-----Original Message-----
From: VanCutsem, Geoffroy
Sent: Monday, August 13, 2018 6:19 AM
To: acrn-dev@...; Kinder, David B
<david.b.kinder@...>
Cc: Chen, Gang C <gang.c.chen@...>; Zhang, Di
<di.zhang@...>; Jin, Zhi <zhi.jin@...>; Liu, Xiaojing
<xiaojing.liu@...>
Subject: RE: [acrn-dev] [PATCH v2] tools:acrn-crashlog: Document of
configuration file

Thanks!

@David, can you take a look at this when you get a chance?
Patch v3 have been confirmed by local doc building system. I put the
"images/dot files" into conf.rst, since the doc building system
collects only rst files to doc directory.
Thanks,
Geoffroy

-----Original Message-----
From: acrn-dev@...
[mailto:acrn-dev@...]
On Behalf Of Liu xinwu
Sent: Monday, August 13, 2018 4:38 AM
To: VanCutsem, Geoffroy <geoffroy.vancutsem@...>; acrn-
dev@...
Cc: Chen, Gang C <gang.c.chen@...>; Zhang, Di
<di.zhang@...>; Jin, Zhi <zhi.jin@...>; Liu, Xiaojing
<xiaojing.liu@...>
Subject: Re: [acrn-dev] [PATCH v2] tools:acrn-crashlog: Document of
configuration file

I send out the patch V3


On 08/10/2018 03:04 PM, VanCutsem, Geoffroy wrote:
Just a couple of nitpicks below then you can move forward with my ACK.
I'm sure David will have suggestions for you but these are best
addressed directly in the PR phase.

Corrected.


I'm assuming you have built that documentation locally and verified
it, can
you just confirm?

Patch v3 have been confirmed by local doc building system. I put the
"images/dot files" into conf.rst, since the doc building system
collects only rst files to doc directory.

Thanks,
Geoffroy

-----Original Message-----
From: acrn-dev@...
[mailto:acrn-dev@...]
On Behalf Of Liu xinwu
Sent: Thursday, August 2, 2018 8:23 AM
To: acrn-dev@...
Cc: Chen, Gang C <gang.c.chen@...>; Zhang, Di
<di.zhang@...>; Liu, Xinwu <xinwu.liu@...>; Jin, Zhi
<zhi.jin@...>; Liu, Xiaojing <xiaojing.liu@...>;
VanCutsem, Geoffroy <geoffroy.vancutsem@...>
Subject: [acrn-dev] [PATCH v2] tools:acrn-crashlog: Document of
configuration file

This document explains all feilds in acrnprobe.xml.
s/fields/fields

User could use it to control acrnprobe's behavior and configure
their own events.

Signed-off-by: Liu, Xinwu <xinwu.liu@...>
---

v1->v2:
1. refer conf.rst from README.rst
2. drawing by graphviz instead of raw text.

tools/acrn-crashlog/acrnprobe/README.rst | 2 +
tools/acrn-crashlog/acrnprobe/conf.rst | 337
+++++++++++++++++++++
.../acrn-crashlog/acrnprobe/images/build-crash.dot | 21 ++
.../acrn- crashlog/acrnprobe/images/crash-match.dot | 25 ++
4 files changed, 385 insertions(+)
create mode 100644 tools/acrn-crashlog/acrnprobe/conf.rst
create mode 100644 tools/acrn-crashlog/acrnprobe/images/build-
crash.dot
create mode 100644 tools/acrn-crashlog/acrnprobe/images/crash-
match.dot

diff --git a/tools/acrn-crashlog/acrnprobe/README.rst b/tools/acrn-
crashlog/acrnprobe/README.rst index fa137f3..5f537ae 100644
--- a/tools/acrn-crashlog/acrnprobe/README.rst
+++ b/tools/acrn-crashlog/acrnprobe/README.rst
@@ -175,4 +175,6 @@ Configuration files

Custom configuration file that ``acrnprobe`` reads.

+More details about configuration file, please refer to
+:ref:`acrnprobe-
conf`.
+
.. _`telemetrics-client`:
https://github.com/clearlinux/telemetrics-client
diff --git a/tools/acrn-crashlog/acrnprobe/conf.rst b/tools/acrn-
crashlog/acrnprobe/conf.rst new file mode 100644 index
0000000..7d8ec40
--- /dev/null
+++ b/tools/acrn-crashlog/acrnprobe/conf.rst
@@ -0,0 +1,337 @@
+.. _acrnprobe-conf:
+
+Configuration Of Acrnprobe
+##########################
+
+Description
+***********
+``Acrnprobe`` uses XML as the format of its configuration file,
+namely ``acrnprobe.xml``, so the configuration file should reach
+the `XML standard`_ at least.
+
+Layout
+******
+
+.. code-block:: xml
+
+ <?xml version="1.0" encoding="UTF-8"?>
+ <conf>
+ Root node of configuration.
+
+ <senders>
+ Configuration section of senders.
+ </senders>
+
+ <triggers>
+ Configuration section of triggers.
+ </triggers>
+
+ <vms>
+ Configuration section of virtual machines.
+ </vms>
+
+ <logs>
+ Configuration section of logs.
+ </logs>
+
+ <crashes>
+ Configuration section of crashes.
+ Note that this section should be configured after triggers and logs, as
+ crashes depend on these two sections.
+ </crashes>
+
+ <infos>
+ Configuration section of infos.
+ Note that this section should be configured after triggers and logs, as
+ infos depend on these two sections.
+ </infos>
+
+ </conf>
+
+As for the definition of ``sender``, ``trigger``, ``crash`` and
+``info`` and information of supported ``vm``, please refer to
:ref:`acrnprobe_doc`.
+
+Properties of modules
+*********************
+
+Common properties
+=================
+
+- ``id``:
+ The index, which grows from 1, in its group.
+- ``enable``:
+ This part of configuration can only take effect when the value is
``true``.
+
+Other properties
+================
+
+- ``inherit``:
+ Specify a parent for a certain crash.
+ The child crash will inherit all configurations from the
+specified (by id)
+ crash. These inherited configurations could be overwritten by
+new
ones.
+ Also, this property helps build the crash tree in ``acrnprobe``.
+- ``expression``:
+ See `Crash`_.
+
+Crash tree in acrnprobe
+***********************
+
+There could be a parent-child relationship between crashes. Refer
+to the diagrams below, crash B and D are the children of crash A,
+because crash B and D inherit from crash A, and crash C is the
+child of
crash B.
+
+Build crash tree in configuration
+=================================
+
+.. graphviz:: images/build-crash.dot
+ :name: build-crash
+ :align: center
+ :caption: Build crash tree in configuration
+
+Crash type matches at runtime
+=============================
+
+In order to find a more specific type, if one crash type matches
+successfully ``acrnprobe`` will do match for each child of it (if
+it
+has) continually, and return the last successful one.
+Supposing these crash trees are like the diagram above at runtime:
+If a crash E is triggered, crash E will be returned immediately.
+If a crash A is triggered, then the candidates are crash A, B, C and D.
+The following diagram describes what ``acrnprobe`` will do if the
+matched result is crash D.
+
+.. graphviz:: images/crash-match.dot
+ :name: crash-match
+ :align: center
+ :caption: Crash type matches at runtime
+
+Sections
+********
+
+Sender
+======
+
+Example:
+
+.. code-block:: xml
+
+ <sender id="1" enable="true">
+ <name>crashlog</name>
+ <outdir>/var/log/crashlog</outdir>
+ <maxcrashdirs>1000</maxcrashdirs>
+ <maxlines>5000</maxlines>
+ <spacequota>90</spacequota>
+ <uptime>
+ <name>UPTIME</name>
+ <frequency>5</frequency>
+ <eventhours>6</eventhours>
+ </uptime>
+ </sender>
+
+* ``name``:
+ Name of sender. ``Acrnprobe`` uses this label to distinguish
+different
+ senders.
+ More information about sender, please refer to :ref:`acrnprobe_doc`.
+* ``outdir``:
+ Directory to store generated files of sender. ``Acrnprobe`` will
+create
+ this directory if it doesn't exist.
+* ``maxcrashdirs``:
+ The maximum serial number of generated ``crash directories``,
+``stat directories``
+ and ``vmevent directories``. The serial number will back to 0 if
+it reaches
+ the maximum. Only used by sender crashlog.
+* ``maxlines``:
+ The maximum lines of file ``history_event``. It will trigger
+action
+ ``"mv history_event history_event.bak"`` that the lines of
+``history_event``
+ reaches the ``maxlines``.
+* ``spacequota``:
+ ``Acrnprobe`` will stop collecting logs if it is true that
+ ``(used space / total space) * 100 > spacequota``. Only used by
+sender
+ crashlog.
+* ``uptime``:
+ Configuration to trigger ``UPTIME`` event.
+ sub-nodes:
+
+ + ``name``:
+ The name of event.
+ + ``frequency``:
+ Time interval in seconds to trigger ``uptime`` event.
+ + ``eventhours``:
+ Time interval in hours to generate a record.
+
+
+Trigger
+=======
+
+Example:
+
+.. code-block:: xml
+
+ <trigger id="1" enable="true">
+ <name>t_pstore</name>
+ <type>node</type>
+ <path>/sys/fs/pstore/console-ramoops-0</path>
+ </trigger>
+
+* ``name``:
+ The name of trigger. It's used by crash and info configuration module.
+* ``type`` and ``path``:
+ These two labels are used to get the content of trigger files.
+ ``Types`` have been implemented:
+
+ + ``node``:
+ ``Path`` can not support ``mmap(2)-like`` operations.
+ ``Acrnprobe`` can
"cannot" in one word.

+ only use ``read(2)`` to get its content.
+ + ``file``:
+ ``Path`` is a regular file which supports ``mmap(2)-like`` operations.
+ + ``dir``:
+ ``Path`` is a directory.
+ + ``rebootreason``:
+ The reboot reason of system. The content of ``rebootreason`` is not
+ obtained in a common way. So, it doesn't work with ``path``.
+ + ``cmd``:
+ ``Path`` is a command which will be launched by ``execvp(3)``.
+
+ Some programs often use format ``string%d`` instead of static
+ file name to generate target file dynamically. So ``path``
+ supports simple formats for these cases:
+
+ + /.../dir/string[*] --> all files with prefix "string" under dir.
+ + /.../dir/string[0] --> the first file of files, sorted by ``alphasort(3)``,
+ with prefix "string" under dir.
+ + /.../dir/string[-1] --> the last file of files, sorted by ``alphasort(3)``,
+ with prefix "string" under dir.
+
+Vm
+==
+
+Example:
+
+.. code-block:: xml
+
+ <vm id="1" enable="true">
+ <name>VM1</name>
+ <channel>polling</channel>
+ <interval>60</interval>
+ <syncevent id="1">CRASH/TOMBSTONE</syncevent>
+ <syncevent id="2">CRASH/UIWDT</syncevent>
+ <syncevent id="3">CRASH/IPANIC</syncevent>
+ <syncevent id="4">REBOOT</syncevent>
+ </vm>
+
+* ``name``:
+ The name of virtual machine.
+* ``channel``:
+ The ``channel`` name to get the virtual machine events.
+* ``interval``:
+ Time interval in seconds of polling vm's image.
+* ``syncevent``:
+ Event type ``acrnprobe`` will synchronize from virtual machine's
``crashlog``.
+ User could specify different types by id. The event type can
+ also be indicated by ``type/subtype``.
+
+Log
+===
+
+Example:
+
+.. code-block:: xml
+
+ <log id="1" enable="true">
+ <name>pstore</name>
+ <type>node</type>
+ <path>/sys/fs/pstore/console-ramoops-0</path>
+ </log>
+
+* ``name``:
+ By default, ``acrnprobe`` will take this ``name`` as generated
+log's name in
+ ``outdir`` of sender crashlog.
+ If ``path`` is specified by simple formats (includes [*], [0] or
+[-1]) the
+ file name of generated logs will be the same as original. More
+details about
+ simple formats, see `Trigger`_.
+* ``type`` and ``path``:
+ Same as `Trigger`_.
+* ``lines``:
+ By default, all contents in the original will be copied to generated log.
+ If this label is configured, only the ``lines`` at the end in
+the original
+ will be copied to the generated log. It takes effect only when
+the ``type`` is
+ ``file``.
+
+Crash
+=====
+
+Example:
+
+.. code-block:: xml
+
+ <crash id='1' inherit='0' enable='true'>
+ <name>UNKNOWN</name>
+ <trigger>t_rebootreason</trigger>
+ <channel>oneshot</channel>
+ <content id='1'>WARM</content>
+ <log id='1'>pstore</log>
+ <log id='2'>acrnlog_last</log>
+ </crash>
+ <crash id='2' inherit='1' enable='true'>
+ <name>IPANIC</name>
+ <trigger>t_pstore</trigger>
+ <content id='1'> </content>
+ <mightcontent expression='1' id='1'>Kernel panic - not
syncing:</mightcontent>
+ <mightcontent expression='1' id='2'>BUG: unable to
+ handle
kernel</mightcontent>
+ <data id='1'>kernel BUG at</data>
+ <data id='2'>EIP is at</data>
+ <data id='3'>Comm:</data>
+ </crash>
+
+* ``name``:
+ The type of the ``crash``.
+* ``trigger``:
+ The trigger name of the crash.
+* ``channel``:
+ The name of channel crash use.
+* ``content`` and ``mightcontent``:
+ They're used to match crash type. The match is successful if all
+the
+ following conditions are met:
+
+ a. All ``contents`` with different ``ids`` are included in trigger's
+ content.
+ b. One of ``mightcontents`` with the same ``expression`` is included in
+ trigger's content at least.
+ c. If there are ``mightcontents`` with different
+ ``expressions``, each
group
+ with the same ``expression`` should meet condition b.
+* ``log``:
+ The log to be collected. The value is the configured ``name`` in
+log
module.
+ User could specify different logs by ``id``.
+* ``data``:
+ It is used to generated ``DATA`` fields in ``crashfile``.
+``Acrnprobe`` will
+ copy the line which starts with configured ``data`` in trigger's
+content
+ to ``DATA`` fields. There are 3 fields in ``crashfile`` and they
+could be
+ specified by ``id`` 1, 2, 3.
+
+Info
+=====
+
+Example:
+
+.. code-block:: xml
+
+ <info id='1' enable='true'>
+ <name>BOOT_LOGS</name>
+ <trigger>t_boot</trigger>
+ <channel>oneshot</channel>
+ <log id='1'>kmsg</log>
+ <log id='2'>cmdline</log>
+ <log id='3'>acrnlog_cur</log>
+ <log id='4'>acrnlog_last</log>
+ </info>
+
+* ``name``:
+ The type of info.
+* ``trigger``:
+ The trigger name of the info.
+* ``channel``:
+ The name of channel info use.
+* ``log``:
+ The log to be collected. The value is the configured name in log
+module. User
+ could specify different logs by id.
+
+.. _`XML standard`: http://www.w3.org/TR/REC-xml
diff --git a/tools/acrn-crashlog/acrnprobe/images/build-crash.dot
b/tools/acrn-crashlog/acrnprobe/images/build-crash.dot
new file mode 100644
index 0000000..08e7628
--- /dev/null
+++ b/tools/acrn-crashlog/acrnprobe/images/build-crash.dot
@@ -0,0 +1,21 @@
+digraph {
+ {
+ node [shape=plaintext];
+ "level 1" -> "level 2" -> "level 3";
+ }
+
+ node [shape=box;style="rounded,filled";color=AntiqueWhite;];
+ c1 [ label="crash A\nid 1\ncrash root" ];
+ c2 [ label="crash B\nid 2" ];
+ c3 [ label="crash C\nid 3\ncrash leaf" ];
+ c4 [ label="crash D\nid 4\ncrash leaf" ];
+ c5 [ label="crash E\nid 5\ncrash root\ncrash leaf" ];
+ { rank = same; "level 1"; c1; c5;}
+ { rank = same; "level 2"; c2; c4;}
+ { rank = same; "level 3"; c3;}
+
+ node [shape=box;color="transparent";];
+ "None" -> {c1 c5} [ label="inherit 0" ];
+ c1 -> {c2 c4} [ label="inherit 1" ];
+ c2 -> c3 [ label="inherit 2" ]; }
diff --git a/tools/acrn-crashlog/acrnprobe/images/crash-match.dot
b/tools/acrn-crashlog/acrnprobe/images/crash-match.dot
new file mode 100644
index 0000000..7da5ce0
--- /dev/null
+++ b/tools/acrn-crashlog/acrnprobe/images/crash-match.dot
@@ -0,0 +1,25 @@
+digraph {
+ {
+ node [shape=plaintext];
+ "level 1" -> "level 2" -> "level 3";
+ }
+
+ node [shape=box;style="rounded,filled";color=AntiqueWhite;];
+ c1 [ label="crash A\nid 1\ncrash root" ];
+ c2 [ label="crash B\nid 2" ];
+ c3 [ label="crash C\nid 3\ncrash leaf" ];
+ c4 [ label="crash D\nid 4\ncrash leaf" ];
+ { rank = same; "level 1"; c1;}
+ { rank = same; "level 2"; c2; c4;}
+ { rank = same; "level 3"; c3;}
+
+ node [shape=box;style="rounded,dashed";];
+ exp1 [ label="crash B matches fail\nmatch for the next
+ child\nof crash
A"];
+ exp2 [ label="crash D matches successfully\nreturn crash D"];
+
+ node [shape=box;style="invis";];
+ "channel" -> c1 [ label="trigger" ]
+ c1 -> {exp1 exp2}
+ exp1 -> c2 -> c3 [ style=dashed dir=none]
+ exp2 -> c4
+}
--
2.7.4




Re: [PATCH v2] tools: acrntrace: Add ring buffer mode

Geoffroy Van Cutsem
 

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Zhipeng Gong
Sent: Monday, August 13, 2018 3:31 PM
To: acrn-dev@...
Subject: [acrn-dev] [PATCH v2] tools: acrntrace: Add ring buffer mode

When running longevity test and capturing acrntrace, generated acrntrace
files sizes are too big.
Sometimes we don't care very old trace. This patch adds ring buffer mode,
fixes acrntrace file size and overwrites the oldest trace with new trace when
the buffer is full.

v2:
- update README.rst

Signed-off-by: Zhipeng Gong <zhipeng.gong@...>
---
tools/acrntrace/README.rst | 4 ++--
tools/acrntrace/acrntrace.c | 44
+++++++++++++++++++++++++++++++++++++++++---
tools/acrntrace/acrntrace.h | 1 +
3 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/tools/acrntrace/README.rst b/tools/acrntrace/README.rst index
433f161..f25c5b0 100644
--- a/tools/acrntrace/README.rst
+++ b/tools/acrntrace/README.rst
@@ -21,8 +21,8 @@ Options:
-i period specify polling interval in milliseconds [1-999]
-t max_time max time to capture trace data (in second)
-c clear the buffered old data
--r free_space amount of free space (in MB) remaining on the disk
- before acrntrace stops
+-r ring_buffer_size ring buffer size (in MB), which enables ring buffer
+ mode and overwrites old trace with new trace
+when full
Suggestion: s/when full/when reaching the end of the buffer

There is another paragraph later in the README.rst files that says:
512MB storage space will be reserved by default. This ensures that acrntrace
will exit automatically when the free storage space on the disk is less than
reserved space. Reserved space on the disk is configurable through '-r'.
This should be updated too, shouldn't it?

Thanks,
Geoffroy


The ``acrntrace_format.py`` is a offline tool for parsing trace data (as output
by acrntrace) to human-readable formats based on given format.
diff --git a/tools/acrntrace/acrntrace.c b/tools/acrntrace/acrntrace.c index
39fda35..e4b3899 100644
--- a/tools/acrntrace/acrntrace.c
+++ b/tools/acrntrace/acrntrace.c
@@ -37,16 +37,19 @@ static char trace_file_dir[TRACE_FILE_DIR_LEN];

static reader_struct *reader;
static int pcpu_num = 0;
+static int ring_buffer_size = 0;

static void display_usage(void)
{
printf("acrntrace - tool to collect ACRN trace data\n"
- "[Usage] acrntrace [-i] [period in msec] [-ch]\n\n"
+ "[Usage] acrntrace [-i] [period in msec] [-r] [ring buffer size] [-
ch]\n\n"
"[Options]\n"
"\t-h: print this message\n"
"\t-i: period_in_ms: specify polling interval [1-999]\n"
"\t-t: max time to capture trace data (in second)\n"
- "\t-c: clear the buffered old data\n");
+ "\t-c: clear the buffered old data\n"
+ "\t-r: ring_buffer_size (in MB), which enables ring buffer mode"
+ " and overwrites old trace with new trace when full\n");
}

static void timer_handler(union sigval sv) @@ -113,6 +116,15 @@ static int
parse_opt(int argc, char *argv[])
timeout = ret;
pr_dbg("Capture trace data for at most %ds\n", ret);
break;
+ case 'r':
+ ret = atoi(optarg);
+ if (ret <= 0) {
+ pr_err("'-r' require integer greater than 0\n");
+ return -EINVAL;
+ }
+ ring_buffer_size = ret * 1024 * 1024;
+ pr_dbg("Ring buffer size is %dM\n", ret);
+ break;
case 'c':
flags |= FLAG_CLEAR_BUF;
break;
@@ -206,6 +218,15 @@ static void reader_fn(param_t * param)
trace_ev_t e;
struct statvfs stat;
uint64_t freespace;
+ int pos = 0;
+
+ if (ring_buffer_size != 0) {
+ param->buffer = malloc(ring_buffer_size);
+ if (!param->buffer) {
+ perror("Failed to allocate ring buffer\n");
+ return;
+ }
+ }

pr_dbg("reader thread[%lu] created for FILE*[0x%p]\n",
pthread_self(), fp);
@@ -219,7 +240,13 @@ static void reader_fn(param_t * param)

while (1) {
do {
- ret = sbuf_write(fd, sbuf);
+ if (ring_buffer_size != 0) {
+ ret = sbuf_get(sbuf, param->buffer + pos);
+ pos += sbuf->ele_size;
+ if (pos + sbuf->ele_size > ring_buffer_size)
+ pos = 0;
+ } else
+ ret = sbuf_write(fd, sbuf);
} while (ret > 0);

usleep(period);
@@ -284,6 +311,17 @@ static void destory_reader(reader_struct * reader)
reader->thrd = 0;
}

+ if (ring_buffer_size != 0 && reader->param.buffer) {
+ int ret;
+
+ ret = write(reader->param.trace_fd, reader->param.buffer,
+ ring_buffer_size);
+ if (ret != ring_buffer_size) {
+ perror("Failed to write ring buffer\n");
+ }
+ free(reader->param.buffer);
+ }
+
if (reader->param.sbuf) {
munmap(reader->param.sbuf, MMAP_SIZE);
reader->param.sbuf = NULL;
diff --git a/tools/acrntrace/acrntrace.h b/tools/acrntrace/acrntrace.h index
ea54f4d..6615157 100644
--- a/tools/acrntrace/acrntrace.h
+++ b/tools/acrntrace/acrntrace.h
@@ -76,6 +76,7 @@ typedef struct {
int trace_fd;
shared_buf_t *sbuf;
pthread_mutex_t *sbuf_lock;
+ uint8_t *buffer;
} param_t;

typedef struct {
--
2.7.4



Re: [PATCH 4/6] DM USB: xHCI: refine port assignment logic

Yu Wang
 

On 18-08-13 16:48:33, 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 assgned to UOS.
The logic is zero reserve and nonzero assignement.

In this patch, the meaning of native_assign_ports is expand to the
following logic:
native_assign_ports[x][y] = 0: native port x-y is not assigned.
native_assign_ports[x][y] < 0: native port x-y is assigned but
currently no virtual device attached.
native_assign_ports[x][y] > 0: native paort x-y is assigned and
virtual device attached to virtual port whose number
is stored in native_assign_ports[x][y].

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

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index f12126b..2df7ce3 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -373,7 +373,16 @@ 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];
+
+ /* native_assign_ports is used to record the assigment information:
+ * native_assign_ports[x][y] = 0: native port x-y is not assigned.
+ * native_assign_ports[x][y] < 0: native port x-y is assigned but
+ * currently no virtual device attached.
+ * native_assign_ports[x][y] > 0: native paort x-y is assigned and
For "= 0" and "< 0", please define some macros...

+ * virtual device attached to virtual port whose number
+ * is stored in native_assign_ports[x][y].
+ */
+ int8_t *native_assign_ports[USB_NATIVE_NUM_BUS];
struct timespec mf_prev_time; /* previous time of accessing MFINDEX */
};

@@ -3365,14 +3374,14 @@ pci_xhci_parse_bus_port(struct pci_xhci_vdev *xdev, char *opts)

if (!xdev->native_assign_ports[bus]) {
xdev->native_assign_ports[bus] = calloc(USB_NATIVE_NUM_PORT,
- sizeof(uint8_t));
+ sizeof(int8_t));
if (!xdev->native_assign_ports[bus]) {
rc = -3;
goto errout;
}
}
+ xdev->native_assign_ports[bus][port] = -1;

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




Re: [PATCH 3/6] DM USB: introduce struct usb_native_devinfo

Yu Wang
 

On 18-08-13 16:48:32, Wu, Xiaoguang wrote:
Current design cannot get physical USB device information without
the creation of pci_xhci_dev_emu. This brings some dificulties in
dificulties?

certain situations, hence struct usb_native_devinfo is introduced
to describe neccessary information to solve this trouble.

Signed-off-by: Xiaoguang Wu <xiaoguang.wu@...>
Reviewed-by: Liang Yang <liang3.yang@...>
---
devicemodel/hw/pci/xhci.c | 31 ++++++++++++-------------------
devicemodel/hw/platform/usb_pmapper.c | 14 +++++++++++++-
devicemodel/include/usb_core.h | 10 ++++++++++
3 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index 490219a..f12126b 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -469,23 +469,22 @@ 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;
- uint8_t native_bus, native_pid, native_port;
- uint16_t native_vid;
- int native_speed;
int intr = 1;

xdev = hci_data;
+ di = dev_data;

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

- de = pci_xhci_dev_create(xdev, dev_data);
+ de = pci_xhci_dev_create(xdev, di->priv_data);
if (!de) {
UPRINTF(LFTL, "fail to create device\r\n");
return -1;
@@ -499,26 +498,20 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
assert(ue);

/* print physical information about new device */
- ue->ue_info(ud, USB_INFO_BUS, &native_bus, sizeof(native_bus));
- ue->ue_info(ud, USB_INFO_PORT, &native_port, sizeof(native_port));
- ue->ue_info(ud, USB_INFO_VID, &native_vid, sizeof(native_vid));
- ue->ue_info(ud, USB_INFO_PID, &native_pid, sizeof(native_pid));
- ue->ue_info(ud, USB_INFO_SPEED, &native_speed, sizeof(native_speed));
Then usb_dev_info is not useful now? Should we clear it?

UPRINTF(LDBG, "%04x:%04x %d-%d connecting.\r\n",
- native_vid, native_pid, native_bus, native_port);
+ di->vid, di->pid, di->bus, di->port);

- if (!xdev->native_assign_ports[native_bus] ||
- !xdev->native_assign_ports[native_bus][native_port]) {
+ if (!xdev->native_assign_ports[di->bus] ||
+ !xdev->native_assign_ports[di->bus][di->port]) {
UPRINTF(LDBG, "%04x:%04x %d-%d doesn't belong to this vm, bye."
- "\r\n", native_vid, native_pid, native_bus,
- native_port);
+ "\r\n", di->vid, di->pid, di->bus, di->port);
goto errout;
}

- UPRINTF(LDBG, "%04x:%04x %d-%d belong to this vm.\r\n", native_vid,
- native_pid, native_bus, native_port);
+ UPRINTF(LDBG, "%04x:%04x %d-%d belong to this vm.\r\n", di->vid,
+ di->pid, di->bus, di->port);

- if (ue->ue_usbver == 2)
+ if (di->bcd < 0x300)
port_start = xdev->usb2_port_start;
else
port_start = xdev->usb3_port_start;
@@ -550,11 +543,11 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)

pci_xhci_reset_slot(xdev, slot);
UPRINTF(LDBG, "%X:%X %d-%d locates in slot %d port %d.\r\n",
- native_vid, native_pid, native_bus, native_port,
+ di->vid, di->pid, di->bus, di->port,
slot, port);

/* Trigger port change event for the arriving device */
- if (pci_xhci_port_connect(xdev, port, native_speed, intr))
+ if (pci_xhci_port_connect(xdev, port, di->speed, intr))
UPRINTF(LFTL, "fail to report port event\n");

return 0;
diff --git a/devicemodel/hw/platform/usb_pmapper.c b/devicemodel/hw/platform/usb_pmapper.c
index 9f57f1c..a9c8608 100644
--- a/devicemodel/hw/platform/usb_pmapper.c
+++ b/devicemodel/hw/platform/usb_pmapper.c
@@ -1020,6 +1020,9 @@ static int
usb_dev_native_sys_conn_cb(struct libusb_context *ctx, struct libusb_device
*ldev, libusb_hotplug_event event, void *pdata)
{
+ struct libusb_device_descriptor d;
+ struct usb_native_devinfo di;
+
UPRINTF(LDBG, "connect event\r\n");

if (!ctx || !ldev) {
@@ -1027,8 +1030,17 @@ usb_dev_native_sys_conn_cb(struct libusb_context *ctx, struct libusb_device
return -1;
}

+ libusb_get_device_descriptor(ldev, &d);
+ di.bus = libusb_get_bus_number(ldev);
+ di.speed = libusb_get_device_speed(ldev);
+ di.port = libusb_get_port_number(ldev);
+ di.pid = d.idProduct;
+ di.vid = d.idVendor;
+ di.bcd = d.bcdUSB;
+ di.priv_data = ldev;
+
if (g_ctx.conn_cb)
- g_ctx.conn_cb(g_ctx.hci_data, ldev);
+ g_ctx.conn_cb(g_ctx.hci_data, &di);

return 0;
}
diff --git a/devicemodel/include/usb_core.h b/devicemodel/include/usb_core.h
index 3a9648a..8727b8b 100644
--- a/devicemodel/include/usb_core.h
+++ b/devicemodel/include/usb_core.h
@@ -163,6 +163,16 @@ struct usb_data_xfer {
pthread_mutex_t mtx;
};

+struct usb_native_devinfo {
+ int speed;
+ uint8_t bus;
+ uint8_t port;
+ uint16_t bcd;
+ uint16_t pid;
+ uint16_t vid;
+ void *priv_data;
+};
This new structure is confused to me. There has already one structure
struct usb_dev which included native usb device information. Why define
a new one? Should we refine the struct usb_dev to replace these native
description members?

+
enum USB_ERRCODE {
USB_ACK,
USB_NAK,
--
2.7.4




[PATCH v2] tools: acrntrace: Add ring buffer mode

Zhipeng Gong <zhipeng.gong@...>
 

When running longevity test and capturing acrntrace, generated acrntrace
files sizes are too big.
Sometimes we don't care very old trace. This patch adds ring buffer
mode, fixes acrntrace file size and overwrites the oldest trace with new
trace when the buffer is full.

v2:
- update README.rst

Signed-off-by: Zhipeng Gong <zhipeng.gong@...>
---
tools/acrntrace/README.rst | 4 ++--
tools/acrntrace/acrntrace.c | 44 +++++++++++++++++++++++++++++++++++++++++---
tools/acrntrace/acrntrace.h | 1 +
3 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/tools/acrntrace/README.rst b/tools/acrntrace/README.rst
index 433f161..f25c5b0 100644
--- a/tools/acrntrace/README.rst
+++ b/tools/acrntrace/README.rst
@@ -21,8 +21,8 @@ Options:
-i period specify polling interval in milliseconds [1-999]
-t max_time max time to capture trace data (in second)
-c clear the buffered old data
--r free_space amount of free space (in MB) remaining on the disk
- before acrntrace stops
+-r ring_buffer_size ring buffer size (in MB), which enables ring buffer
+ mode and overwrites old trace with new trace when full

The ``acrntrace_format.py`` is a offline tool for parsing trace data (as output
by acrntrace) to human-readable formats based on given format.
diff --git a/tools/acrntrace/acrntrace.c b/tools/acrntrace/acrntrace.c
index 39fda35..e4b3899 100644
--- a/tools/acrntrace/acrntrace.c
+++ b/tools/acrntrace/acrntrace.c
@@ -37,16 +37,19 @@ static char trace_file_dir[TRACE_FILE_DIR_LEN];

static reader_struct *reader;
static int pcpu_num = 0;
+static int ring_buffer_size = 0;

static void display_usage(void)
{
printf("acrntrace - tool to collect ACRN trace data\n"
- "[Usage] acrntrace [-i] [period in msec] [-ch]\n\n"
+ "[Usage] acrntrace [-i] [period in msec] [-r] [ring buffer size] [-ch]\n\n"
"[Options]\n"
"\t-h: print this message\n"
"\t-i: period_in_ms: specify polling interval [1-999]\n"
"\t-t: max time to capture trace data (in second)\n"
- "\t-c: clear the buffered old data\n");
+ "\t-c: clear the buffered old data\n"
+ "\t-r: ring_buffer_size (in MB), which enables ring buffer mode"
+ " and overwrites old trace with new trace when full\n");
}

static void timer_handler(union sigval sv)
@@ -113,6 +116,15 @@ static int parse_opt(int argc, char *argv[])
timeout = ret;
pr_dbg("Capture trace data for at most %ds\n", ret);
break;
+ case 'r':
+ ret = atoi(optarg);
+ if (ret <= 0) {
+ pr_err("'-r' require integer greater than 0\n");
+ return -EINVAL;
+ }
+ ring_buffer_size = ret * 1024 * 1024;
+ pr_dbg("Ring buffer size is %dM\n", ret);
+ break;
case 'c':
flags |= FLAG_CLEAR_BUF;
break;
@@ -206,6 +218,15 @@ static void reader_fn(param_t * param)
trace_ev_t e;
struct statvfs stat;
uint64_t freespace;
+ int pos = 0;
+
+ if (ring_buffer_size != 0) {
+ param->buffer = malloc(ring_buffer_size);
+ if (!param->buffer) {
+ perror("Failed to allocate ring buffer\n");
+ return;
+ }
+ }

pr_dbg("reader thread[%lu] created for FILE*[0x%p]\n",
pthread_self(), fp);
@@ -219,7 +240,13 @@ static void reader_fn(param_t * param)

while (1) {
do {
- ret = sbuf_write(fd, sbuf);
+ if (ring_buffer_size != 0) {
+ ret = sbuf_get(sbuf, param->buffer + pos);
+ pos += sbuf->ele_size;
+ if (pos + sbuf->ele_size > ring_buffer_size)
+ pos = 0;
+ } else
+ ret = sbuf_write(fd, sbuf);
} while (ret > 0);

usleep(period);
@@ -284,6 +311,17 @@ static void destory_reader(reader_struct * reader)
reader->thrd = 0;
}

+ if (ring_buffer_size != 0 && reader->param.buffer) {
+ int ret;
+
+ ret = write(reader->param.trace_fd, reader->param.buffer,
+ ring_buffer_size);
+ if (ret != ring_buffer_size) {
+ perror("Failed to write ring buffer\n");
+ }
+ free(reader->param.buffer);
+ }
+
if (reader->param.sbuf) {
munmap(reader->param.sbuf, MMAP_SIZE);
reader->param.sbuf = NULL;
diff --git a/tools/acrntrace/acrntrace.h b/tools/acrntrace/acrntrace.h
index ea54f4d..6615157 100644
--- a/tools/acrntrace/acrntrace.h
+++ b/tools/acrntrace/acrntrace.h
@@ -76,6 +76,7 @@ typedef struct {
int trace_fd;
shared_buf_t *sbuf;
pthread_mutex_t *sbuf_lock;
+ uint8_t *buffer;
} param_t;

typedef struct {
--
2.7.4


Re: [PATCH 2/6] DM USB: xHCI: refine xHCI PORTSC Register related functions

Yu Wang
 

On 18-08-13 16:48:31, Wu, Xiaoguang wrote:
PORTSC (Port Status and Control Register) register play a very
important role in USB sub-system. This patch is used to refine
related manipulation functions.

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

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index 792d930..490219a 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -439,11 +439,14 @@ static void pci_xhci_update_ep_ring(struct pci_xhci_vdev *xdev,
struct xhci_endp_ctx *ep_ctx,
uint32_t streamid, uint64_t ringaddr,
int ccs);
-static void pci_xhci_init_port(struct pci_xhci_vdev *xdev, int portn);
+static void pci_xhci_port_init(struct pci_xhci_vdev *xdev, int portn);
+static int pci_xhci_port_connect(struct pci_xhci_vdev *xdev, int port,
+ int usb_speed, int intr);
+static int pci_xhci_port_disconnect(struct pci_xhci_vdev *xdev, int port,
+ int intr);
static struct pci_xhci_dev_emu *pci_xhci_dev_create(struct pci_xhci_vdev *
xdev, void *dev_data);
static void pci_xhci_dev_destroy(struct pci_xhci_dev_emu *de);
-static int pci_xhci_port_chg(struct pci_xhci_vdev *xdev, int port, int conn);
static void pci_xhci_set_evtrb(struct xhci_trb *evtrb, uint64_t port,
uint32_t errcode, uint32_t evtype);
static int pci_xhci_xfer_complete(struct pci_xhci_vdev *xdev,
@@ -472,6 +475,8 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
void *ud;
uint8_t native_bus, native_pid, native_port;
uint16_t native_vid;
+ int native_speed;
+ int intr = 1;
Can we remove intr variable? Is hasn't changed in this function and pass
to pci_xhci_port_disconnect directly..


xdev = hci_data;

@@ -498,6 +503,7 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
ue->ue_info(ud, USB_INFO_PORT, &native_port, sizeof(native_port));
ue->ue_info(ud, USB_INFO_VID, &native_vid, sizeof(native_vid));
ue->ue_info(ud, USB_INFO_PID, &native_pid, sizeof(native_pid));
+ ue->ue_info(ud, USB_INFO_SPEED, &native_speed, sizeof(native_speed));
UPRINTF(LDBG, "%04x:%04x %d-%d connecting.\r\n",
native_vid, native_pid, native_bus, native_port);

@@ -543,14 +549,12 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
xdev->ndevices++;

pci_xhci_reset_slot(xdev, slot);
- pci_xhci_init_port(xdev, port);
-
UPRINTF(LDBG, "%X:%X %d-%d locates in slot %d port %d.\r\n",
native_vid, native_pid, native_bus, native_port,
slot, port);

/* Trigger port change event for the arriving device */
- if (pci_xhci_port_chg(xdev, port, 1))
+ if (pci_xhci_port_connect(xdev, port, native_speed, intr))
UPRINTF(LFTL, "fail to report port event\n");

return 0;
@@ -566,6 +570,7 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
struct pci_xhci_dev_emu *edev;
struct usb_dev *udev;
uint8_t port, native_port;
+ int intr = 1;
Ditto.


assert(hci_data);
assert(dev_data);
@@ -595,7 +600,7 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
}

UPRINTF(LDBG, "report virtual port %d status\r\n", port);
- if (pci_xhci_port_chg(xdev, port, 0)) {
+ if (pci_xhci_port_disconnect(xdev, port, intr)) {
UPRINTF(LFTL, "fail to report event\r\n");
return -1;
}
@@ -784,32 +789,29 @@ pci_xhci_convert_speed(int lspeed)
}

static int
-pci_xhci_port_chg(struct pci_xhci_vdev *xdev, int port, int conn)
+pci_xhci_port_change(struct pci_xhci_vdev *xdev, int port, int usb_speed,
+ int conn, int intr)
{
int speed, error;
struct xhci_trb evtrb;
struct pci_xhci_portregs *reg;
- struct pci_xhci_dev_emu *dev;

assert(xdev != NULL);

reg = XHCI_PORTREG_PTR(xdev, port);
- dev = XHCI_DEVINST_PTR(xdev, port);
- if (!dev || !dev->dev_ue || !reg) {
- UPRINTF(LWRN, "find nullptr with port %d\r\n", port);
- return -1;
- }
-
if (conn == 0) {
reg->portsc &= ~XHCI_PS_CCS;
reg->portsc |= (XHCI_PS_CSC |
XHCI_PS_PLS_SET(UPS_PORT_LS_RX_DET));
} else {
- speed = pci_xhci_convert_speed(dev->dev_ue->ue_usbspeed);
+ speed = pci_xhci_convert_speed(usb_speed);
reg->portsc = XHCI_PS_CCS | XHCI_PS_PP | XHCI_PS_CSC;
reg->portsc |= XHCI_PS_SPEED_SET(speed);
}

+ if (!intr)
+ return 0;
What is mean of intr? From this patch, it almost useless..

+
/* make an event for the guest OS */
pci_xhci_set_evtrb(&evtrb,
port,
@@ -825,6 +827,20 @@ pci_xhci_port_chg(struct pci_xhci_vdev *xdev, int port, int conn)
return (error == XHCI_TRB_ERROR_SUCCESS) ? 0 : -1;
}

+static int
+pci_xhci_port_connect(struct pci_xhci_vdev *xdev, int port, int usb_speed,
+ int intr)
+{
+ return pci_xhci_port_change(xdev, port, usb_speed, 1, intr);
+}
+
+static int
+pci_xhci_port_disconnect(struct pci_xhci_vdev *xdev, int port, int intr)
+{
+ /* for disconnect, the speed is useless */
+ return pci_xhci_port_change(xdev, port, 0, 0, intr);
+}
+
static void
pci_xhci_set_evtrb(struct xhci_trb *evtrb, uint64_t port, uint32_t errcode,
uint32_t evtype)
@@ -3206,32 +3222,10 @@ pci_xhci_reset_port(struct pci_xhci_vdev *xdev, int portn, int warm)
}

static void
-pci_xhci_init_port(struct pci_xhci_vdev *xdev, int portn)
+pci_xhci_port_init(struct pci_xhci_vdev *xdev, int portn)
Let's keep pci_xhci_init_port..

{
- struct pci_xhci_portregs *port;
- struct pci_xhci_dev_emu *dev;
-
- port = XHCI_PORTREG_PTR(xdev, portn);
- dev = XHCI_DEVINST_PTR(xdev, portn);
- if (dev) {
- port->portsc = XHCI_PS_CCS | /* connected */
- XHCI_PS_PP; /* port power */
-
- if (dev->dev_ue->ue_usbver == 2) {
- port->portsc |= XHCI_PS_PLS_SET(UPS_PORT_LS_POLL) |
- XHCI_PS_SPEED_SET(dev->dev_ue->ue_usbspeed);
- } else {
- port->portsc |= XHCI_PS_PLS_SET(UPS_PORT_LS_U0) |
- XHCI_PS_PED | /* enabled */
- XHCI_PS_SPEED_SET(dev->dev_ue->ue_usbspeed);
- }
-
- UPRINTF(LDBG, "Init port %d 0x%x\n", portn, port->portsc);
- } else {
- port->portsc = XHCI_PS_PLS_SET(UPS_PORT_LS_RX_DET) | XHCI_PS_PP;
- UPRINTF(LDBG, "Init empty port %d 0x%x\n",
- portn, port->portsc);
- }
+ XHCI_PORTREG_PTR(xdev, portn)->portsc =
+ XHCI_PS_PLS_SET(UPS_PORT_LS_RX_DET) | XHCI_PS_PP;
}

static int
@@ -3567,7 +3561,7 @@ pci_xhci_parse_opts(struct pci_xhci_vdev *xdev, char *opts)

/* do not use the zero index element */
for (i = 1; i <= XHCI_MAX_DEVS; i++)
- pci_xhci_init_port(xdev, i);
+ pci_xhci_port_init(xdev, i);

errout:
if (rc) {
--
2.7.4




Re: [PATCH v2] tools:acrn-crashlog: Document of configuration file

Geoffroy Van Cutsem
 

Thanks!

@David, can you take a look at this when you get a chance?
Patch v3 have been confirmed by local doc building system. I put the
"images/dot files" into conf.rst, since the doc building system collects only rst
files to doc directory.
Thanks,
Geoffroy

-----Original Message-----
From: acrn-dev@... [mailto:acrn-dev@...]
On Behalf Of Liu xinwu
Sent: Monday, August 13, 2018 4:38 AM
To: VanCutsem, Geoffroy <geoffroy.vancutsem@...>; acrn-
dev@...
Cc: Chen, Gang C <gang.c.chen@...>; Zhang, Di <di.zhang@...>;
Jin, Zhi <zhi.jin@...>; Liu, Xiaojing <xiaojing.liu@...>
Subject: Re: [acrn-dev] [PATCH v2] tools:acrn-crashlog: Document of
configuration file

I send out the patch V3


On 08/10/2018 03:04 PM, VanCutsem, Geoffroy wrote:
Just a couple of nitpicks below then you can move forward with my ACK.
I'm sure David will have suggestions for you but these are best addressed
directly in the PR phase.

Corrected.


I'm assuming you have built that documentation locally and verified it, can
you just confirm?

Patch v3 have been confirmed by local doc building system. I put the
"images/dot files" into conf.rst, since the doc building system collects only rst
files to doc directory.

Thanks,
Geoffroy

-----Original Message-----
From: acrn-dev@...
[mailto:acrn-dev@...]
On Behalf Of Liu xinwu
Sent: Thursday, August 2, 2018 8:23 AM
To: acrn-dev@...
Cc: Chen, Gang C <gang.c.chen@...>; Zhang, Di
<di.zhang@...>; Liu, Xinwu <xinwu.liu@...>; Jin, Zhi
<zhi.jin@...>; Liu, Xiaojing <xiaojing.liu@...>;
VanCutsem, Geoffroy <geoffroy.vancutsem@...>
Subject: [acrn-dev] [PATCH v2] tools:acrn-crashlog: Document of
configuration file

This document explains all feilds in acrnprobe.xml.
s/fields/fields

User could use it to control acrnprobe's behavior and configure their
own events.

Signed-off-by: Liu, Xinwu <xinwu.liu@...>
---

v1->v2:
1. refer conf.rst from README.rst
2. drawing by graphviz instead of raw text.

tools/acrn-crashlog/acrnprobe/README.rst | 2 +
tools/acrn-crashlog/acrnprobe/conf.rst | 337
+++++++++++++++++++++
.../acrn-crashlog/acrnprobe/images/build-crash.dot | 21 ++
.../acrn- crashlog/acrnprobe/images/crash-match.dot | 25 ++
4 files changed, 385 insertions(+)
create mode 100644 tools/acrn-crashlog/acrnprobe/conf.rst
create mode 100644 tools/acrn-crashlog/acrnprobe/images/build-
crash.dot
create mode 100644 tools/acrn-crashlog/acrnprobe/images/crash-
match.dot

diff --git a/tools/acrn-crashlog/acrnprobe/README.rst b/tools/acrn-
crashlog/acrnprobe/README.rst index fa137f3..5f537ae 100644
--- a/tools/acrn-crashlog/acrnprobe/README.rst
+++ b/tools/acrn-crashlog/acrnprobe/README.rst
@@ -175,4 +175,6 @@ Configuration files

Custom configuration file that ``acrnprobe`` reads.

+More details about configuration file, please refer to :ref:`acrnprobe-
conf`.
+
.. _`telemetrics-client`:
https://github.com/clearlinux/telemetrics-client
diff --git a/tools/acrn-crashlog/acrnprobe/conf.rst b/tools/acrn-
crashlog/acrnprobe/conf.rst new file mode 100644 index
0000000..7d8ec40
--- /dev/null
+++ b/tools/acrn-crashlog/acrnprobe/conf.rst
@@ -0,0 +1,337 @@
+.. _acrnprobe-conf:
+
+Configuration Of Acrnprobe
+##########################
+
+Description
+***********
+``Acrnprobe`` uses XML as the format of its configuration file,
+namely ``acrnprobe.xml``, so the configuration file should reach the
+`XML standard`_ at least.
+
+Layout
+******
+
+.. code-block:: xml
+
+ <?xml version="1.0" encoding="UTF-8"?>
+ <conf>
+ Root node of configuration.
+
+ <senders>
+ Configuration section of senders.
+ </senders>
+
+ <triggers>
+ Configuration section of triggers.
+ </triggers>
+
+ <vms>
+ Configuration section of virtual machines.
+ </vms>
+
+ <logs>
+ Configuration section of logs.
+ </logs>
+
+ <crashes>
+ Configuration section of crashes.
+ Note that this section should be configured after triggers and logs, as
+ crashes depend on these two sections.
+ </crashes>
+
+ <infos>
+ Configuration section of infos.
+ Note that this section should be configured after triggers and logs, as
+ infos depend on these two sections.
+ </infos>
+
+ </conf>
+
+As for the definition of ``sender``, ``trigger``, ``crash`` and
+``info`` and information of supported ``vm``, please refer to
:ref:`acrnprobe_doc`.
+
+Properties of modules
+*********************
+
+Common properties
+=================
+
+- ``id``:
+ The index, which grows from 1, in its group.
+- ``enable``:
+ This part of configuration can only take effect when the value is ``true``.
+
+Other properties
+================
+
+- ``inherit``:
+ Specify a parent for a certain crash.
+ The child crash will inherit all configurations from the specified
+(by id)
+ crash. These inherited configurations could be overwritten by new
ones.
+ Also, this property helps build the crash tree in ``acrnprobe``.
+- ``expression``:
+ See `Crash`_.
+
+Crash tree in acrnprobe
+***********************
+
+There could be a parent-child relationship between crashes. Refer to
+the diagrams below, crash B and D are the children of crash A,
+because crash B and D inherit from crash A, and crash C is the child of
crash B.
+
+Build crash tree in configuration
+=================================
+
+.. graphviz:: images/build-crash.dot
+ :name: build-crash
+ :align: center
+ :caption: Build crash tree in configuration
+
+Crash type matches at runtime
+=============================
+
+In order to find a more specific type, if one crash type matches
+successfully ``acrnprobe`` will do match for each child of it (if it
+has) continually, and return the last successful one.
+Supposing these crash trees are like the diagram above at runtime:
+If a crash E is triggered, crash E will be returned immediately.
+If a crash A is triggered, then the candidates are crash A, B, C and D.
+The following diagram describes what ``acrnprobe`` will do if the
+matched result is crash D.
+
+.. graphviz:: images/crash-match.dot
+ :name: crash-match
+ :align: center
+ :caption: Crash type matches at runtime
+
+Sections
+********
+
+Sender
+======
+
+Example:
+
+.. code-block:: xml
+
+ <sender id="1" enable="true">
+ <name>crashlog</name>
+ <outdir>/var/log/crashlog</outdir>
+ <maxcrashdirs>1000</maxcrashdirs>
+ <maxlines>5000</maxlines>
+ <spacequota>90</spacequota>
+ <uptime>
+ <name>UPTIME</name>
+ <frequency>5</frequency>
+ <eventhours>6</eventhours>
+ </uptime>
+ </sender>
+
+* ``name``:
+ Name of sender. ``Acrnprobe`` uses this label to distinguish
+different
+ senders.
+ More information about sender, please refer to :ref:`acrnprobe_doc`.
+* ``outdir``:
+ Directory to store generated files of sender. ``Acrnprobe`` will
+create
+ this directory if it doesn't exist.
+* ``maxcrashdirs``:
+ The maximum serial number of generated ``crash directories``,
+``stat directories``
+ and ``vmevent directories``. The serial number will back to 0 if
+it reaches
+ the maximum. Only used by sender crashlog.
+* ``maxlines``:
+ The maximum lines of file ``history_event``. It will trigger
+action
+ ``"mv history_event history_event.bak"`` that the lines of
+``history_event``
+ reaches the ``maxlines``.
+* ``spacequota``:
+ ``Acrnprobe`` will stop collecting logs if it is true that
+ ``(used space / total space) * 100 > spacequota``. Only used by
+sender
+ crashlog.
+* ``uptime``:
+ Configuration to trigger ``UPTIME`` event.
+ sub-nodes:
+
+ + ``name``:
+ The name of event.
+ + ``frequency``:
+ Time interval in seconds to trigger ``uptime`` event.
+ + ``eventhours``:
+ Time interval in hours to generate a record.
+
+
+Trigger
+=======
+
+Example:
+
+.. code-block:: xml
+
+ <trigger id="1" enable="true">
+ <name>t_pstore</name>
+ <type>node</type>
+ <path>/sys/fs/pstore/console-ramoops-0</path>
+ </trigger>
+
+* ``name``:
+ The name of trigger. It's used by crash and info configuration module.
+* ``type`` and ``path``:
+ These two labels are used to get the content of trigger files.
+ ``Types`` have been implemented:
+
+ + ``node``:
+ ``Path`` can not support ``mmap(2)-like`` operations.
+ ``Acrnprobe`` can
"cannot" in one word.

+ only use ``read(2)`` to get its content.
+ + ``file``:
+ ``Path`` is a regular file which supports ``mmap(2)-like`` operations.
+ + ``dir``:
+ ``Path`` is a directory.
+ + ``rebootreason``:
+ The reboot reason of system. The content of ``rebootreason`` is not
+ obtained in a common way. So, it doesn't work with ``path``.
+ + ``cmd``:
+ ``Path`` is a command which will be launched by ``execvp(3)``.
+
+ Some programs often use format ``string%d`` instead of static file
+ name to generate target file dynamically. So ``path`` supports
+ simple formats for these cases:
+
+ + /.../dir/string[*] --> all files with prefix "string" under dir.
+ + /.../dir/string[0] --> the first file of files, sorted by ``alphasort(3)``,
+ with prefix "string" under dir.
+ + /.../dir/string[-1] --> the last file of files, sorted by ``alphasort(3)``,
+ with prefix "string" under dir.
+
+Vm
+==
+
+Example:
+
+.. code-block:: xml
+
+ <vm id="1" enable="true">
+ <name>VM1</name>
+ <channel>polling</channel>
+ <interval>60</interval>
+ <syncevent id="1">CRASH/TOMBSTONE</syncevent>
+ <syncevent id="2">CRASH/UIWDT</syncevent>
+ <syncevent id="3">CRASH/IPANIC</syncevent>
+ <syncevent id="4">REBOOT</syncevent>
+ </vm>
+
+* ``name``:
+ The name of virtual machine.
+* ``channel``:
+ The ``channel`` name to get the virtual machine events.
+* ``interval``:
+ Time interval in seconds of polling vm's image.
+* ``syncevent``:
+ Event type ``acrnprobe`` will synchronize from virtual machine's
``crashlog``.
+ User could specify different types by id. The event type can also
+ be indicated by ``type/subtype``.
+
+Log
+===
+
+Example:
+
+.. code-block:: xml
+
+ <log id="1" enable="true">
+ <name>pstore</name>
+ <type>node</type>
+ <path>/sys/fs/pstore/console-ramoops-0</path>
+ </log>
+
+* ``name``:
+ By default, ``acrnprobe`` will take this ``name`` as generated
+log's name in
+ ``outdir`` of sender crashlog.
+ If ``path`` is specified by simple formats (includes [*], [0] or
+[-1]) the
+ file name of generated logs will be the same as original. More
+details about
+ simple formats, see `Trigger`_.
+* ``type`` and ``path``:
+ Same as `Trigger`_.
+* ``lines``:
+ By default, all contents in the original will be copied to generated log.
+ If this label is configured, only the ``lines`` at the end in the
+original
+ will be copied to the generated log. It takes effect only when the
+``type`` is
+ ``file``.
+
+Crash
+=====
+
+Example:
+
+.. code-block:: xml
+
+ <crash id='1' inherit='0' enable='true'>
+ <name>UNKNOWN</name>
+ <trigger>t_rebootreason</trigger>
+ <channel>oneshot</channel>
+ <content id='1'>WARM</content>
+ <log id='1'>pstore</log>
+ <log id='2'>acrnlog_last</log>
+ </crash>
+ <crash id='2' inherit='1' enable='true'>
+ <name>IPANIC</name>
+ <trigger>t_pstore</trigger>
+ <content id='1'> </content>
+ <mightcontent expression='1' id='1'>Kernel panic - not
syncing:</mightcontent>
+ <mightcontent expression='1' id='2'>BUG: unable to handle
kernel</mightcontent>
+ <data id='1'>kernel BUG at</data>
+ <data id='2'>EIP is at</data>
+ <data id='3'>Comm:</data>
+ </crash>
+
+* ``name``:
+ The type of the ``crash``.
+* ``trigger``:
+ The trigger name of the crash.
+* ``channel``:
+ The name of channel crash use.
+* ``content`` and ``mightcontent``:
+ They're used to match crash type. The match is successful if all
+the
+ following conditions are met:
+
+ a. All ``contents`` with different ``ids`` are included in trigger's
+ content.
+ b. One of ``mightcontents`` with the same ``expression`` is included in
+ trigger's content at least.
+ c. If there are ``mightcontents`` with different ``expressions``, each
group
+ with the same ``expression`` should meet condition b.
+* ``log``:
+ The log to be collected. The value is the configured ``name`` in log
module.
+ User could specify different logs by ``id``.
+* ``data``:
+ It is used to generated ``DATA`` fields in ``crashfile``.
+``Acrnprobe`` will
+ copy the line which starts with configured ``data`` in trigger's
+content
+ to ``DATA`` fields. There are 3 fields in ``crashfile`` and they
+could be
+ specified by ``id`` 1, 2, 3.
+
+Info
+=====
+
+Example:
+
+.. code-block:: xml
+
+ <info id='1' enable='true'>
+ <name>BOOT_LOGS</name>
+ <trigger>t_boot</trigger>
+ <channel>oneshot</channel>
+ <log id='1'>kmsg</log>
+ <log id='2'>cmdline</log>
+ <log id='3'>acrnlog_cur</log>
+ <log id='4'>acrnlog_last</log>
+ </info>
+
+* ``name``:
+ The type of info.
+* ``trigger``:
+ The trigger name of the info.
+* ``channel``:
+ The name of channel info use.
+* ``log``:
+ The log to be collected. The value is the configured name in log
+module. User
+ could specify different logs by id.
+
+.. _`XML standard`: http://www.w3.org/TR/REC-xml
diff --git a/tools/acrn-crashlog/acrnprobe/images/build-crash.dot
b/tools/acrn-crashlog/acrnprobe/images/build-crash.dot
new file mode 100644
index 0000000..08e7628
--- /dev/null
+++ b/tools/acrn-crashlog/acrnprobe/images/build-crash.dot
@@ -0,0 +1,21 @@
+digraph {
+ {
+ node [shape=plaintext];
+ "level 1" -> "level 2" -> "level 3";
+ }
+
+ node [shape=box;style="rounded,filled";color=AntiqueWhite;];
+ c1 [ label="crash A\nid 1\ncrash root" ];
+ c2 [ label="crash B\nid 2" ];
+ c3 [ label="crash C\nid 3\ncrash leaf" ];
+ c4 [ label="crash D\nid 4\ncrash leaf" ];
+ c5 [ label="crash E\nid 5\ncrash root\ncrash leaf" ];
+ { rank = same; "level 1"; c1; c5;}
+ { rank = same; "level 2"; c2; c4;}
+ { rank = same; "level 3"; c3;}
+
+ node [shape=box;color="transparent";];
+ "None" -> {c1 c5} [ label="inherit 0" ];
+ c1 -> {c2 c4} [ label="inherit 1" ];
+ c2 -> c3 [ label="inherit 2" ];
+}
diff --git a/tools/acrn-crashlog/acrnprobe/images/crash-match.dot
b/tools/acrn-crashlog/acrnprobe/images/crash-match.dot
new file mode 100644
index 0000000..7da5ce0
--- /dev/null
+++ b/tools/acrn-crashlog/acrnprobe/images/crash-match.dot
@@ -0,0 +1,25 @@
+digraph {
+ {
+ node [shape=plaintext];
+ "level 1" -> "level 2" -> "level 3";
+ }
+
+ node [shape=box;style="rounded,filled";color=AntiqueWhite;];
+ c1 [ label="crash A\nid 1\ncrash root" ];
+ c2 [ label="crash B\nid 2" ];
+ c3 [ label="crash C\nid 3\ncrash leaf" ];
+ c4 [ label="crash D\nid 4\ncrash leaf" ];
+ { rank = same; "level 1"; c1;}
+ { rank = same; "level 2"; c2; c4;}
+ { rank = same; "level 3"; c3;}
+
+ node [shape=box;style="rounded,dashed";];
+ exp1 [ label="crash B matches fail\nmatch for the next child\nof crash
A"];
+ exp2 [ label="crash D matches successfully\nreturn crash D"];
+
+ node [shape=box;style="invis";];
+ "channel" -> c1 [ label="trigger" ]
+ c1 -> {exp1 exp2}
+ exp1 -> c2 -> c3 [ style=dashed dir=none]
+ exp2 -> c4
+}
--
2.7.4




Re: [PATCH v3] tools:acrn-crashlog: Document of configuration file

Geoffroy Van Cutsem
 

-----Original Message-----
From: Liu, Xinwu
Sent: Monday, August 13, 2018 4:37 AM
To: acrn-dev@...
Cc: Chen, Gang C <gang.c.chen@...>; Zhang, Di <di.zhang@...>;
Liu, Xinwu <xinwu.liu@...>; Jin, Zhi <zhi.jin@...>; Liu, Xiaojing
<xiaojing.liu@...>; VanCutsem, Geoffroy
<geoffroy.vancutsem@...>
Subject: [PATCH v3] tools:acrn-crashlog: Document of configuration file

This document explains all fields in acrnprobe.xml.
User could use it to control acrnprobe's behavior and configure their own
events.

Signed-off-by: Liu, Xinwu <xinwu.liu@...>
Acked-by: Geoffroy Van Cutsem <geoffroy.vancutsem@...>

---
v1->v2:
1. refer conf.rst from README.rst
2. drawing by graphviz instead of raw text.

v2->v3:
1. correct some typo
2. put dot files into conf.rst, since the build system of doc collects only rst
files

tools/acrn-crashlog/acrnprobe/README.rst | 2 +
tools/acrn-crashlog/acrnprobe/conf.rst | 379
+++++++++++++++++++++++++++++++
2 files changed, 381 insertions(+)
create mode 100644 tools/acrn-crashlog/acrnprobe/conf.rst

diff --git a/tools/acrn-crashlog/acrnprobe/README.rst b/tools/acrn-
crashlog/acrnprobe/README.rst
index fa137f3..5f537ae 100644
--- a/tools/acrn-crashlog/acrnprobe/README.rst
+++ b/tools/acrn-crashlog/acrnprobe/README.rst
@@ -175,4 +175,6 @@ Configuration files

Custom configuration file that ``acrnprobe`` reads.

+More details about configuration file, please refer to :ref:`acrnprobe-conf`.
+
.. _`telemetrics-client`: https://github.com/clearlinux/telemetrics-client
diff --git a/tools/acrn-crashlog/acrnprobe/conf.rst b/tools/acrn-
crashlog/acrnprobe/conf.rst
new file mode 100644
index 0000000..f88d411
--- /dev/null
+++ b/tools/acrn-crashlog/acrnprobe/conf.rst
@@ -0,0 +1,379 @@
+.. _acrnprobe-conf:
+
+Configuration Of Acrnprobe
+##########################
+
+Description
+***********
+``Acrnprobe`` uses XML as the format of its configuration file, namely
+``acrnprobe.xml``, so the configuration file should reach the `XML
+standard`_ at least.
+
+Layout
+******
+
+.. code-block:: xml
+
+ <?xml version="1.0" encoding="UTF-8"?>
+ <conf>
+ Root node of configuration.
+
+ <senders>
+ Configuration section of senders.
+ </senders>
+
+ <triggers>
+ Configuration section of triggers.
+ </triggers>
+
+ <vms>
+ Configuration section of virtual machines.
+ </vms>
+
+ <logs>
+ Configuration section of logs.
+ </logs>
+
+ <crashes>
+ Configuration section of crashes.
+ Note that this section should be configured after triggers and logs, as
+ crashes depend on these two sections.
+ </crashes>
+
+ <infos>
+ Configuration section of infos.
+ Note that this section should be configured after triggers and logs, as
+ infos depend on these two sections.
+ </infos>
+
+ </conf>
+
+As for the definition of ``sender``, ``trigger``, ``crash`` and
+``info`` and information of supported ``vm``, please refer to
:ref:`acrnprobe_doc`.
+
+Properties of modules
+*********************
+
+Common properties
+=================
+
+- ``id``:
+ The index, which grows from 1, in its group.
+- ``enable``:
+ This part of configuration can only take effect when the value is ``true``.
+
+Other properties
+================
+
+- ``inherit``:
+ Specify a parent for a certain crash.
+ The child crash will inherit all configurations from the specified
+(by id)
+ crash. These inherited configurations could be overwritten by new ones.
+ Also, this property helps build the crash tree in ``acrnprobe``.
+- ``expression``:
+ See `Crash`_.
+
+Crash tree in acrnprobe
+***********************
+
+There could be a parent-child relationship between crashes. Refer to
+the diagrams below, crash B and D are the children of crash A, because
+crash B and D inherit from crash A, and crash C is the child of crash B.
+
+Build crash tree in configuration
+=================================
+
+.. graphviz::
+
+ digraph {
+ {
+ node [shape=plaintext];
+ "level 1" -> "level 2" -> "level 3";
+ }
+
+ node [shape=box;style="rounded,filled";color=AntiqueWhite;];
+ c1 [ label="crash A\nid 1\ncrash root" ];
+ c2 [ label="crash B\nid 2" ];
+ c3 [ label="crash C\nid 3\ncrash leaf" ];
+ c4 [ label="crash D\nid 4\ncrash leaf" ];
+ c5 [ label="crash E\nid 5\ncrash root\ncrash leaf" ];
+ { rank = same; "level 1"; c1; c5;}
+ { rank = same; "level 2"; c2; c4;}
+ { rank = same; "level 3"; c3;}
+
+ node [shape=box;color="transparent";];
+ "None" -> {c1 c5} [ label="inherit 0" ];
+ c1 -> {c2 c4} [ label="inherit 1" ];
+ c2 -> c3 [ label="inherit 2" ];
+ }
+
+Crash type matches at runtime
+=============================
+
+In order to find a more specific type, if one crash type matches
+successfully ``acrnprobe`` will do match for each child of it (if it
+has) continually, and return the last successful one.
+Supposing these crash trees are like the diagram above at runtime:
+If a crash E is triggered, crash E will be returned immediately.
+If a crash A is triggered, then the candidates are crash A, B, C and D.
+The following diagram describes what ``acrnprobe`` will do if the
+matched result is crash D.
+
+.. graphviz::
+
+ digraph {
+ {
+ node [shape=plaintext];
+ "level 1" -> "level 2" -> "level 3";
+ }
+
+ node [shape=box;style="rounded,filled";color=AntiqueWhite;];
+ c1 [ label="crash A\nid 1\ncrash root" ];
+ c2 [ label="crash B\nid 2" ];
+ c3 [ label="crash C\nid 3\ncrash leaf" ];
+ c4 [ label="crash D\nid 4\ncrash leaf" ];
+ { rank = same; "level 1"; c1;}
+ { rank = same; "level 2"; c2; c4;}
+ { rank = same; "level 3"; c3;}
+
+ node [shape=box;style="rounded,dashed";];
+ exp1 [ label="crash B matches fail\nmatch for the next child\nof crash
A"];
+ exp2 [ label="crash D matches successfully\nreturn crash D"];
+
+ node [shape=box;style="invis";];
+ "channel" -> c1 [ label="trigger" ]
+ c1 -> {exp1 exp2}
+ exp1 -> c2 -> c3 [ style=dashed dir=none]
+ exp2 -> c4
+ }
+
+Sections
+********
+
+Sender
+======
+
+Example:
+
+.. code-block:: xml
+
+ <sender id="1" enable="true">
+ <name>crashlog</name>
+ <outdir>/var/log/crashlog</outdir>
+ <maxcrashdirs>1000</maxcrashdirs>
+ <maxlines>5000</maxlines>
+ <spacequota>90</spacequota>
+ <uptime>
+ <name>UPTIME</name>
+ <frequency>5</frequency>
+ <eventhours>6</eventhours>
+ </uptime>
+ </sender>
+
+* ``name``:
+ Name of sender. ``Acrnprobe`` uses this label to distinguish
+different
+ senders.
+ More information about sender, please refer to :ref:`acrnprobe_doc`.
+* ``outdir``:
+ Directory to store generated files of sender. ``Acrnprobe`` will
+create
+ this directory if it doesn't exist.
+* ``maxcrashdirs``:
+ The maximum serial number of generated ``crash directories``, ``stat
+directories``
+ and ``vmevent directories``. The serial number will back to 0 if it
+reaches
+ the maximum. Only used by sender crashlog.
+* ``maxlines``:
+ The maximum lines of file ``history_event``. It will trigger action
+ ``"mv history_event history_event.bak"`` that the lines of
+``history_event``
+ reaches the ``maxlines``.
+* ``spacequota``:
+ ``Acrnprobe`` will stop collecting logs if it is true that
+ ``(used space / total space) * 100 > spacequota``. Only used by
+sender
+ crashlog.
+* ``uptime``:
+ Configuration to trigger ``UPTIME`` event.
+ sub-nodes:
+
+ + ``name``:
+ The name of event.
+ + ``frequency``:
+ Time interval in seconds to trigger ``uptime`` event.
+ + ``eventhours``:
+ Time interval in hours to generate a record.
+
+
+Trigger
+=======
+
+Example:
+
+.. code-block:: xml
+
+ <trigger id="1" enable="true">
+ <name>t_pstore</name>
+ <type>node</type>
+ <path>/sys/fs/pstore/console-ramoops-0</path>
+ </trigger>
+
+* ``name``:
+ The name of trigger. It's used by crash and info configuration module.
+* ``type`` and ``path``:
+ These two labels are used to get the content of trigger files.
+ ``Types`` have been implemented:
+
+ + ``node``:
+ ``Path`` cannot support ``mmap(2)-like`` operations. ``Acrnprobe`` can
+ only use ``read(2)`` to get its content.
+ + ``file``:
+ ``Path`` is a regular file which supports ``mmap(2)-like`` operations.
+ + ``dir``:
+ ``Path`` is a directory.
+ + ``rebootreason``:
+ The reboot reason of system. The content of ``rebootreason`` is not
+ obtained in a common way. So, it doesn't work with ``path``.
+ + ``cmd``:
+ ``Path`` is a command which will be launched by ``execvp(3)``.
+
+ Some programs often use format ``string%d`` instead of static file
+ name to generate target file dynamically. So ``path`` supports simple
+ formats for these cases:
+
+ + /.../dir/string[*] --> all files with prefix "string" under dir.
+ + /.../dir/string[0] --> the first file of files, sorted by ``alphasort(3)``,
+ with prefix "string" under dir.
+ + /.../dir/string[-1] --> the last file of files, sorted by ``alphasort(3)``,
+ with prefix "string" under dir.
+
+Vm
+==
+
+Example:
+
+.. code-block:: xml
+
+ <vm id="1" enable="true">
+ <name>VM1</name>
+ <channel>polling</channel>
+ <interval>60</interval>
+ <syncevent id="1">CRASH/TOMBSTONE</syncevent>
+ <syncevent id="2">CRASH/UIWDT</syncevent>
+ <syncevent id="3">CRASH/IPANIC</syncevent>
+ <syncevent id="4">REBOOT</syncevent>
+ </vm>
+
+* ``name``:
+ The name of virtual machine.
+* ``channel``:
+ The ``channel`` name to get the virtual machine events.
+* ``interval``:
+ Time interval in seconds of polling vm's image.
+* ``syncevent``:
+ Event type ``acrnprobe`` will synchronize from virtual machine's
``crashlog``.
+ User could specify different types by id. The event type can also be
+ indicated by ``type/subtype``.
+
+Log
+===
+
+Example:
+
+.. code-block:: xml
+
+ <log id="1" enable="true">
+ <name>pstore</name>
+ <type>node</type>
+ <path>/sys/fs/pstore/console-ramoops-0</path>
+ </log>
+
+* ``name``:
+ By default, ``acrnprobe`` will take this ``name`` as generated log's
+name in
+ ``outdir`` of sender crashlog.
+ If ``path`` is specified by simple formats (includes [*], [0] or
+[-1]) the
+ file name of generated logs will be the same as original. More
+details about
+ simple formats, see `Trigger`_.
+* ``type`` and ``path``:
+ Same as `Trigger`_.
+* ``lines``:
+ By default, all contents in the original will be copied to generated log.
+ If this label is configured, only the ``lines`` at the end in the
+original
+ will be copied to the generated log. It takes effect only when the
+``type`` is
+ ``file``.
+
+Crash
+=====
+
+Example:
+
+.. code-block:: xml
+
+ <crash id='1' inherit='0' enable='true'>
+ <name>UNKNOWN</name>
+ <trigger>t_rebootreason</trigger>
+ <channel>oneshot</channel>
+ <content id='1'>WARM</content>
+ <log id='1'>pstore</log>
+ <log id='2'>acrnlog_last</log>
+ </crash>
+ <crash id='2' inherit='1' enable='true'>
+ <name>IPANIC</name>
+ <trigger>t_pstore</trigger>
+ <content id='1'> </content>
+ <mightcontent expression='1' id='1'>Kernel panic - not
syncing:</mightcontent>
+ <mightcontent expression='1' id='2'>BUG: unable to handle
kernel</mightcontent>
+ <data id='1'>kernel BUG at</data>
+ <data id='2'>EIP is at</data>
+ <data id='3'>Comm:</data>
+ </crash>
+
+* ``name``:
+ The type of the ``crash``.
+* ``trigger``:
+ The trigger name of the crash.
+* ``channel``:
+ The name of channel crash use.
+* ``content`` and ``mightcontent``:
+ They're used to match crash type. The match is successful if all the
+ following conditions are met:
+
+ a. All ``contents`` with different ``ids`` are included in trigger's
+ content.
+ b. One of ``mightcontents`` with the same ``expression`` is included in
+ trigger's content at least.
+ c. If there are ``mightcontents`` with different ``expressions``, each group
+ with the same ``expression`` should meet condition b.
+* ``log``:
+ The log to be collected. The value is the configured ``name`` in log module.
+ User could specify different logs by ``id``.
+* ``data``:
+ It is used to generated ``DATA`` fields in ``crashfile``.
+``Acrnprobe`` will
+ copy the line which starts with configured ``data`` in trigger's
+content
+ to ``DATA`` fields. There are 3 fields in ``crashfile`` and they
+could be
+ specified by ``id`` 1, 2, 3.
+
+Info
+=====
+
+Example:
+
+.. code-block:: xml
+
+ <info id='1' enable='true'>
+ <name>BOOT_LOGS</name>
+ <trigger>t_boot</trigger>
+ <channel>oneshot</channel>
+ <log id='1'>kmsg</log>
+ <log id='2'>cmdline</log>
+ <log id='3'>acrnlog_cur</log>
+ <log id='4'>acrnlog_last</log>
+ </info>
+
+* ``name``:
+ The type of info.
+* ``trigger``:
+ The trigger name of the info.
+* ``channel``:
+ The name of channel info use.
+* ``log``:
+ The log to be collected. The value is the configured name in log
+module. User
+ could specify different logs by id.
+
+.. _`XML standard`: http://www.w3.org/TR/REC-xml
--
2.7.4


Re: [PATCH] HV: hv_main: Add #ifdef HV_DEBUG before debug specific code #ifdef

Kaige Fu
 

On 08-13 Mon 10:01, Eddie Dong wrote:
In general we should do in different patch for bug fix. But this one is simple for those compile option --- this looks like MISRAC patches, we can do together to save
Got it.

-----Original Message-----
From: acrn-dev@...
[mailto:acrn-dev@...] On Behalf Of Kaige Fu
Sent: Monday, August 13, 2018 5:18 PM
To: acrn-dev@...
Subject: Re: [acrn-dev] [PATCH] HV: hv_main: Add #ifdef HV_DEBUG before
debug specific code

On 08-13 Mon 08:27, Eddie Dong wrote:

On 08-13 Mon 07:59, Eddie Dong wrote:
This is part of SBUF component, what kind of compile option do we
use?
Let us use same one.
shell command "vmexit" is the only one user of vmexit_time/cnt and
we use the get_vmexit_profile, enclosed by HV_DEBUG, to access these
variables.

Ok then.


BTW, hcall side also needs this kind of compile option.
I can't follow here. Can you make it more clear.
The sbuf has a hypercall, we need compile option to cover all sbuf code...
Absolutely, we should do it. This should be done in another patch, right?






Re: [PATCH] hv: treewide: fix Macro redefine, usage -- and operation violations

Junjie Mao
 

junjunshan1 <junjun.shan@...> writes:

MISARC has requirements about Marco redefinition, usage of ++ or -- and assignment operator in boolean expression.
This patch is used to solve these violations.

The modifications are summarized as following:
1.The HC_VM_SET_MEMORY_REGION, HC_VM_GPA2HPA, HC_VM_SET_MEMORY_REGIONS are redefined twice in acrn_hv_des.h, so delete them to solve the macro redefinition violations.
2.The macro BUS_LOCK are redefined in bits.h and atomic.h, then delete the declaration in both two files, add a new declaration in cpu.h and include the header file.
3.modify the code to deprecate the usage of -- operators in string.c.
4.modify the while loop to for loop to avoid assignment operator in
boolean expression in vlapic.c.
Some general conventions about commit logs:

1. Each line should be no more than 80 characters. Break the
paragraph if it is longer than that.

2. Due to the first rule, there typically is an empty line between
adjacent paragraphs/items.


Signed-off-by: junjunshan1 <junjun.shan@...>
Reviewed-by: Junjie Mao <junjie.mao@...>

With a minor style issue below.

Please also set your name properly by executing 'git config --global
user.name <your name>'.

---
hypervisor/arch/x86/guest/vlapic.c | 3 ++-
hypervisor/include/arch/x86/cpu.h | 2 ++
hypervisor/include/lib/atomic.h | 3 +--
hypervisor/include/lib/bits.h | 3 +--
hypervisor/include/public/acrn_hv_defs.h | 3 ---
hypervisor/lib/string.c | 6 ++++--
6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hypervisor/arch/x86/guest/vlapic.c b/hypervisor/arch/x86/guest/vlapic.c
index 35be79c..72acb76 100644
--- a/hypervisor/arch/x86/guest/vlapic.c
+++ b/hypervisor/arch/x86/guest/vlapic.c
@@ -1135,7 +1135,8 @@ vlapic_icrlo_write_handler(struct acrn_vlapic *vlapic)
break;
}

- while ((vcpu_id = ffs64(dmask)) != INVALID_BIT_INDEX) {
+ for (vcpu_id = ffs64(dmask); vcpu_id != INVALID_BIT_INDEX;
+ vcpu_id = ffs64(dmask)) {
bitmap_clear_lock(vcpu_id, &dmask);
target_vcpu = vcpu_from_vid(vlapic->vm, vcpu_id);
if (target_vcpu == NULL) {
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index e56f443..d24228d 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -152,6 +152,8 @@

#ifndef ASSEMBLER

+#define BUS_LOCK "lock ; "
+
/**
*
* Identifiers for architecturally defined registers.
diff --git a/hypervisor/include/lib/atomic.h b/hypervisor/include/lib/atomic.h
index 264917c..6085863 100644
--- a/hypervisor/include/lib/atomic.h
+++ b/hypervisor/include/lib/atomic.h
@@ -29,8 +29,7 @@

#ifndef ATOMIC_H
#define ATOMIC_H
-
-#define BUS_LOCK "lock ; "
+#include <cpu.h>

#define build_atomic_load(name, size, type, ptr) \
static inline type name(const volatile type *ptr) \
diff --git a/hypervisor/include/lib/bits.h b/hypervisor/include/lib/bits.h
index 04a5057..ea0e8a1 100644
--- a/hypervisor/include/lib/bits.h
+++ b/hypervisor/include/lib/bits.h
@@ -29,8 +29,7 @@

#ifndef BITS_H
#define BITS_H
-
-#define BUS_LOCK "lock ; "
+#include <cpu.h>
/**
*
* INVALID_BIT_INDEX means when input paramter is zero,
diff --git a/hypervisor/include/public/acrn_hv_defs.h b/hypervisor/include/public/acrn_hv_defs.h
index cfeb834..700ce92 100644
--- a/hypervisor/include/public/acrn_hv_defs.h
+++ b/hypervisor/include/public/acrn_hv_defs.h
@@ -55,9 +55,6 @@
#define HC_VM_GPA2HPA BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x01UL)
#define HC_VM_SET_MEMORY_REGIONS BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x02UL)
#define HC_VM_WRITE_PROTECT_PAGE BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x03UL)
-#define HC_VM_SET_MEMORY_REGION BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x00UL)
-#define HC_VM_GPA2HPA BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x01UL)
-#define HC_VM_SET_MEMORY_REGIONS BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x02UL)

/* PCI assignment*/
#define HC_ID_PCI_BASE 0x50UL
diff --git a/hypervisor/lib/string.c b/hypervisor/lib/string.c
index db4f145..7d2a5a5 100644
--- a/hypervisor/lib/string.c
+++ b/hypervisor/lib/string.c
@@ -210,7 +210,8 @@ char *strcpy_s(char *d_arg, size_t dmax, const char *s_arg)
while (dest_avail > 0U) {
if (overlap_guard == 0U) {
pr_err("%s: overlap happened.", __func__);
- *(--d) = '\0';
+ d = d-1;
A single space is required between a binary operator and either of its
operand per our coding style.

+ *d = '\0';
return NULL;
}

@@ -293,7 +294,8 @@ char *strncpy_s(char *d_arg, size_t dmax, const char *s_arg, size_t slen_arg)
while (dest_avail > 0U) {
if (overlap_guard == 0U) {
pr_err("%s: overlap happened.", __func__);
- *(--d) = '\0';
+ d = d-1;
Ditto.

--
Best Regards
Junjie Mao

+ *d = '\0';
return NULL;
}


[PATCH] hv: treewide: fix Macro redefine, usage -- and operation violations

Junjun Shan
 

MISARC has requirements about Marco redefinition, usage of ++ or -- and assignment operator in boolean expression.
This patch is used to solve these violations.

The modifications are summarized as following:
1.The HC_VM_SET_MEMORY_REGION, HC_VM_GPA2HPA, HC_VM_SET_MEMORY_REGIONS are redefined twice in acrn_hv_des.h, so delete them to solve the macro redefinition violations.
2.The macro BUS_LOCK are redefined in bits.h and atomic.h, then delete the declaration in both two files, add a new declaration in cpu.h and include the header file.
3.modify the code to deprecate the usage of -- operators in string.c.
4.modify the while loop to for loop to avoid assignment operator in boolean expression in vlapic.c.

Signed-off-by: junjunshan1 <junjun.shan@...>
---
hypervisor/arch/x86/guest/vlapic.c | 3 ++-
hypervisor/include/arch/x86/cpu.h | 2 ++
hypervisor/include/lib/atomic.h | 3 +--
hypervisor/include/lib/bits.h | 3 +--
hypervisor/include/public/acrn_hv_defs.h | 3 ---
hypervisor/lib/string.c | 6 ++++--
6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hypervisor/arch/x86/guest/vlapic.c b/hypervisor/arch/x86/guest/vlapic.c
index 35be79c..72acb76 100644
--- a/hypervisor/arch/x86/guest/vlapic.c
+++ b/hypervisor/arch/x86/guest/vlapic.c
@@ -1135,7 +1135,8 @@ vlapic_icrlo_write_handler(struct acrn_vlapic *vlapic)
break;
}

- while ((vcpu_id = ffs64(dmask)) != INVALID_BIT_INDEX) {
+ for (vcpu_id = ffs64(dmask); vcpu_id != INVALID_BIT_INDEX;
+ vcpu_id = ffs64(dmask)) {
bitmap_clear_lock(vcpu_id, &dmask);
target_vcpu = vcpu_from_vid(vlapic->vm, vcpu_id);
if (target_vcpu == NULL) {
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index e56f443..d24228d 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -152,6 +152,8 @@

#ifndef ASSEMBLER

+#define BUS_LOCK "lock ; "
+
/**
*
* Identifiers for architecturally defined registers.
diff --git a/hypervisor/include/lib/atomic.h b/hypervisor/include/lib/atomic.h
index 264917c..6085863 100644
--- a/hypervisor/include/lib/atomic.h
+++ b/hypervisor/include/lib/atomic.h
@@ -29,8 +29,7 @@

#ifndef ATOMIC_H
#define ATOMIC_H
-
-#define BUS_LOCK "lock ; "
+#include <cpu.h>

#define build_atomic_load(name, size, type, ptr) \
static inline type name(const volatile type *ptr) \
diff --git a/hypervisor/include/lib/bits.h b/hypervisor/include/lib/bits.h
index 04a5057..ea0e8a1 100644
--- a/hypervisor/include/lib/bits.h
+++ b/hypervisor/include/lib/bits.h
@@ -29,8 +29,7 @@

#ifndef BITS_H
#define BITS_H
-
-#define BUS_LOCK "lock ; "
+#include <cpu.h>
/**
*
* INVALID_BIT_INDEX means when input paramter is zero,
diff --git a/hypervisor/include/public/acrn_hv_defs.h b/hypervisor/include/public/acrn_hv_defs.h
index cfeb834..700ce92 100644
--- a/hypervisor/include/public/acrn_hv_defs.h
+++ b/hypervisor/include/public/acrn_hv_defs.h
@@ -55,9 +55,6 @@
#define HC_VM_GPA2HPA BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x01UL)
#define HC_VM_SET_MEMORY_REGIONS BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x02UL)
#define HC_VM_WRITE_PROTECT_PAGE BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x03UL)
-#define HC_VM_SET_MEMORY_REGION BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x00UL)
-#define HC_VM_GPA2HPA BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x01UL)
-#define HC_VM_SET_MEMORY_REGIONS BASE_HC_ID(HC_ID, HC_ID_MEM_BASE + 0x02UL)

/* PCI assignment*/
#define HC_ID_PCI_BASE 0x50UL
diff --git a/hypervisor/lib/string.c b/hypervisor/lib/string.c
index db4f145..7d2a5a5 100644
--- a/hypervisor/lib/string.c
+++ b/hypervisor/lib/string.c
@@ -210,7 +210,8 @@ char *strcpy_s(char *d_arg, size_t dmax, const char *s_arg)
while (dest_avail > 0U) {
if (overlap_guard == 0U) {
pr_err("%s: overlap happened.", __func__);
- *(--d) = '\0';
+ d = d-1;
+ *d = '\0';
return NULL;
}

@@ -293,7 +294,8 @@ char *strncpy_s(char *d_arg, size_t dmax, const char *s_arg, size_t slen_arg)
while (dest_avail > 0U) {
if (overlap_guard == 0U) {
pr_err("%s: overlap happened.", __func__);
- *(--d) = '\0';
+ d = d-1;
+ *d = '\0';
return NULL;
}

--
2.7.4