Date   

[PATCH V2] hv: vuart: fix 'Shifting value too far'

Shiqing Gao
 

MISRA-C requires that shift operation cannot exceed the word length.

What this patch does:
- Fix the bug in 'vuart_init'
The register 'dll' and 'dlh' should be uint8_t rather than char.
'dll' is the lower 8-bit of divisor.
'dlh' is the higher 8-bit of divisor.
So, the shift value should be 8U rather than 16U.
- Fix other data type issues regarding to the registers in 'struct
vuart'
The registers should be unsigned variables.

v1 -> v2:
* Use a local variable 'uint8_t value_u8 = (uint8_t)value' to avoid
mutiple times type conversion
* Use '(uint8_t)divisor' rather than '(uint8_t)(divisor & 0xFFU)' to
get the lower 8 bit of 'divisor'
Direct type conversion is safe and simple per Xiangyang's suggestion.

Tracked-On: #861
Signed-off-by: Shiqing Gao <shiqing.gao@...>
---
hypervisor/debug/vuart.c | 42 ++++++++++++++++++++--------------------
hypervisor/include/debug/vuart.h | 20 +++++++++----------
2 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/hypervisor/debug/vuart.c b/hypervisor/debug/vuart.c
index 57e2611..cac6fc0 100644
--- a/hypervisor/debug/vuart.c
+++ b/hypervisor/debug/vuart.c
@@ -133,12 +133,13 @@ static void vuart_toggle_intr(struct vuart *vu)
}
}

-static void vuart_write(__unused struct vm_io_handler *hdlr,
- struct vm *vm, uint16_t offset_arg,
- __unused size_t width, uint32_t value)
+static void vuart_write(__unused struct vm_io_handler *hdlr, struct vm *vm,
+ uint16_t offset_arg, __unused size_t width, uint32_t value)
{
uint16_t offset = offset_arg;
struct vuart *vu = vm_vuart(vm);
+ uint8_t value_u8 = (uint8_t)value;
+
offset -= vu->base;
vuart_lock(vu);
/*
@@ -146,19 +147,19 @@ static void vuart_write(__unused struct vm_io_handler *hdlr,
*/
if ((vu->lcr & LCR_DLAB) != 0) {
if (offset == UART16550_DLL) {
- vu->dll = value;
+ vu->dll = value_u8;
goto done;
}

if (offset == UART16550_DLM) {
- vu->dlh = value;
+ vu->dlh = value_u8;
goto done;
}
}

switch (offset) {
case UART16550_THR:
- fifo_putchar(&vu->txfifo, value);
+ fifo_putchar(&vu->txfifo, value_u8);
vu->thre_int_pending = true;
break;
case UART16550_IER:
@@ -166,26 +167,26 @@ static void vuart_write(__unused struct vm_io_handler *hdlr,
* Apply mask so that bits 4-7 are 0
* Also enables bits 0-3 only if they're 1
*/
- vu->ier = value & 0x0FU;
+ vu->ier = value_u8 & 0x0FU;
break;
case UART16550_FCR:
/*
* The FCR_ENABLE bit must be '1' for the programming
* of other FCR bits to be effective.
*/
- if ((value & FCR_FIFOE) == 0U) {
- vu->fcr = 0;
+ if ((value_u8 & FCR_FIFOE) == 0U) {
+ vu->fcr = 0U;
} else {
- if ((value & FCR_RFR) != 0U) {
+ if ((value_u8 & FCR_RFR) != 0U) {
fifo_reset(&vu->rxfifo);
}

- vu->fcr = value &
+ vu->fcr = value_u8 &
(FCR_FIFOE | FCR_DMA | FCR_RX_MASK);
}
break;
case UART16550_LCR:
- vu->lcr = value;
+ vu->lcr = value_u8;
break;
case UART16550_MCR:
/* ignore modem */
@@ -202,7 +203,7 @@ static void vuart_write(__unused struct vm_io_handler *hdlr,
*/
break;
case UART16550_SCR:
- vu->scr = value;
+ vu->scr = value_u8;
break;
default:
/*
@@ -218,14 +219,13 @@ done:
vuart_unlock(vu);
}

-static uint32_t vuart_read(__unused struct vm_io_handler *hdlr,
- struct vm *vm, uint16_t offset_arg,
- __unused size_t width)
+static uint32_t vuart_read(__unused struct vm_io_handler *hdlr, struct vm *vm,
+ uint16_t offset_arg, __unused size_t width)
{
uint16_t offset = offset_arg;
- char iir, reg;
- uint8_t intr_reason;
+ uint8_t iir, reg, intr_reason;
struct vuart *vu = vm_vuart(vm);
+
offset -= vu->base;
vuart_lock(vu);
/*
@@ -295,7 +295,7 @@ static uint32_t vuart_read(__unused struct vm_io_handler *hdlr,
done:
vuart_toggle_intr(vu);
vuart_unlock(vu);
- return reg;
+ return (uint32_t)reg;
}

static void vuart_register_io_handler(struct vm *vm)
@@ -370,8 +370,8 @@ void *vuart_init(struct vm *vm)

/* Set baud rate*/
divisor = UART_CLOCK_RATE / BAUD_9600 / 16U;
- vu->dll = divisor;
- vu->dlh = divisor >> 16U;
+ vu->dll = (uint8_t)divisor;
+ vu->dlh = (uint8_t)(divisor >> 8U);

vu->active = false;
vu->base = COM1_BASE;
diff --git a/hypervisor/include/debug/vuart.h b/hypervisor/include/debug/vuart.h
index 38499a1..ea0754c 100644
--- a/hypervisor/include/debug/vuart.h
+++ b/hypervisor/include/debug/vuart.h
@@ -39,16 +39,16 @@ struct fifo {
};

struct vuart {
- char data; /* Data register (R/W) */
- char ier; /* Interrupt enable register (R/W) */
- char lcr; /* Line control register (R/W) */
- char mcr; /* Modem control register (R/W) */
- char lsr; /* Line status register (R/W) */
- char msr; /* Modem status register (R/W) */
- char fcr; /* FIFO control register (W) */
- char scr; /* Scratch register (R/W) */
- char dll; /* Baudrate divisor latch LSB */
- char dlh; /* Baudrate divisor latch MSB */
+ uint8_t data; /* Data register (R/W) */
+ uint8_t ier; /* Interrupt enable register (R/W) */
+ uint8_t lcr; /* Line control register (R/W) */
+ uint8_t mcr; /* Modem control register (R/W) */
+ uint8_t lsr; /* Line status register (R/W) */
+ uint8_t msr; /* Modem status register (R/W) */
+ uint8_t fcr; /* FIFO control register (W) */
+ uint8_t scr; /* Scratch register (R/W) */
+ uint8_t dll; /* Baudrate divisor latch LSB */
+ uint8_t dlh; /* Baudrate divisor latch MSB */

struct fifo rxfifo;
struct fifo txfifo;
--
1.9.1


[PATCH v5 5/5] hv: pirq: change the order of functions within irq.c

Yan, Like
 

This commit changes the order of functions in arch/x86/irq.c, to
make it looks cleaner, with no change within any function.

Signed-off-by: Yan, Like <like.yan@...>
Reviewed-by: Anthony Xu <anthony.xu@...>
---
hypervisor/arch/x86/irq.c | 261 +++++++++++++++++++-------------------
1 file changed, 129 insertions(+), 132 deletions(-)

diff --git a/hypervisor/arch/x86/irq.c b/hypervisor/arch/x86/irq.c
index 52211a20..d06eae68 100644
--- a/hypervisor/arch/x86/irq.c
+++ b/hypervisor/arch/x86/irq.c
@@ -15,39 +15,12 @@ static uint32_t vector_to_irq[NR_MAX_VECTOR + 1];

spurious_handler_t spurious_handler;

-static inline void handle_irq(struct irq_desc *desc);
-
#define NR_STATIC_MAPPINGS (2U)
static uint32_t irq_static_mappings[NR_STATIC_MAPPINGS][2] = {
{TIMER_IRQ, VECTOR_TIMER},
{NOTIFY_IRQ, VECTOR_NOTIFY_VCPU},
};

-static void init_irq_desc(void)
-{
- uint32_t i;
-
- for (i = 0U; i < NR_IRQS; i++) {
- irq_desc_array[i].irq = i;
- irq_desc_array[i].vector = VECTOR_INVALID;
- spinlock_init(&irq_desc_array[i].lock);
- }
-
- for (i = 0U; i <= NR_MAX_VECTOR; i++) {
- vector_to_irq[i] = IRQ_INVALID;
- }
-
- /* init fixed mapping for specific irq and vector */
- for (i = 0U; i < NR_STATIC_MAPPINGS; i++) {
- uint32_t irq = irq_static_mappings[i][0];
- uint32_t vr = irq_static_mappings[i][1];
-
- irq_desc_array[irq].vector = vr;
- irq_desc_array[irq].used = IRQ_ASSIGNED;
- vector_to_irq[vr] = irq;
- }
-}
-
/*
* find available vector VECTOR_DYNAMIC_START ~ VECTOR_DYNAMIC_END
*/
@@ -194,7 +167,6 @@ void free_vector_for_irq(uint32_t irq)
spinlock_irqrestore_release(&irq_alloc_spinlock);
}

-
/*
* The function prepares irq and vector before registering handler.
* There are four cases:
@@ -259,12 +231,6 @@ static void release_irq_resource(uint32_t irq)
}
}

-static void disable_pic_irq(void)
-{
- pio_write8(0xffU, 0xA1U);
- pio_write8(0xffU, 0x21U);
-}
-
/* setup irq_desc fields */
static inline int32_t setup_irq(struct irq_desc *desc,
irq_action_t action_fn,
@@ -339,44 +305,49 @@ int32_t request_irq(uint32_t req_irq,
return (int32_t)irq;
}

-uint32_t irq_to_vector(uint32_t irq)
+void free_irq(uint32_t irq)
{
- if (irq < NR_IRQS) {
- return irq_desc_array[irq].vector;
- } else {
- return VECTOR_INVALID;
- }
-}
+ struct irq_desc *desc;

-void init_default_irqs(uint16_t cpu_id)
-{
- if (cpu_id != BOOT_CPU_ID) {
+ if (irq >= NR_IRQS) {
return;
}

- init_irq_desc();
+ desc = &irq_desc_array[irq];
+ dev_dbg(ACRN_DBG_IRQ, "[%s] %s irq%d vr:0x%x",
+ __func__, desc->name, irq, irq_to_vector(irq));

- /* we use ioapic only, disable legacy PIC */
- disable_pic_irq();
- setup_ioapic_irq();
- init_softirq();
+ unset_irq(desc);
+ release_irq_resource(irq);
}

-void dispatch_exception(struct intr_excp_ctx *ctx)
+void set_irq_trigger_mode(uint32_t irq, bool is_level_trigger)
{
- uint16_t pcpu_id = get_cpu_id();
+ struct irq_desc *desc;

- /* Obtain lock to ensure exception dump doesn't get corrupted */
- spinlock_obtain(&exception_spinlock);
+ spinlock_rflags;

- /* Dump exception context */
- dump_exception(ctx, pcpu_id);
+ if (irq >= NR_IRQS) {
+ return;
+ }

- /* Release lock to let other CPUs handle exception */
- spinlock_release(&exception_spinlock);
+ desc = &irq_desc_array[irq];
+ spinlock_irqsave_obtain(&desc->lock);
+ if (is_level_trigger == true) {
+ desc->flags |= IRQF_LEVEL;
+ } else {
+ desc->flags &= ~IRQF_LEVEL;
+ }
+ spinlock_irqrestore_release(&desc->lock);
+}

- /* Halt the CPU */
- cpu_dead(pcpu_id);
+uint32_t irq_to_vector(uint32_t irq)
+{
+ if (irq < NR_IRQS) {
+ return irq_desc_array[irq].vector;
+ } else {
+ return VECTOR_INVALID;
+ }
}

static void handle_spurious_interrupt(uint32_t vector)
@@ -392,6 +363,43 @@ static void handle_spurious_interrupt(uint32_t vector)
}
}

+static inline bool irq_need_mask(struct irq_desc *desc)
+{
+ /* level triggered gsi should be masked */
+ return (((desc->flags & IRQF_LEVEL) != 0U)
+ && irq_is_gsi(desc->irq));
+}
+
+static inline bool irq_need_unmask(struct irq_desc *desc)
+{
+ /* level triggered gsi for non-ptdev should be unmasked */
+ return (((desc->flags & IRQF_LEVEL) != 0U)
+ && ((desc->flags & IRQF_PT) == 0U)
+ && irq_is_gsi(desc->irq));
+}
+
+static inline void handle_irq(struct irq_desc *desc)
+{
+ irq_action_t action = desc->action;
+
+
+ /* mask iopaic pin */
+ if (irq_need_mask(desc)) {
+ GSI_MASK_IRQ(desc->irq);
+ }
+
+ /* Send EOI to LAPIC/IOAPIC IRR */
+ send_lapic_eoi();
+
+ if (action != NULL) {
+ action(desc->irq, desc->priv_data);
+ }
+
+ if (irq_need_unmask(desc)) {
+ GSI_UNMASK_IRQ(desc->irq);
+ }
+}
+
/* do_IRQ() */
void dispatch_interrupt(struct intr_excp_ctx *ctx)
{
@@ -423,6 +431,23 @@ ERR:
return;
}

+void dispatch_exception(struct intr_excp_ctx *ctx)
+{
+ uint16_t pcpu_id = get_cpu_id();
+
+ /* Obtain lock to ensure exception dump doesn't get corrupted */
+ spinlock_obtain(&exception_spinlock);
+
+ /* Dump exception context */
+ dump_exception(ctx, pcpu_id);
+
+ /* Release lock to let other CPUs handle exception */
+ spinlock_release(&exception_spinlock);
+
+ /* Halt the CPU */
+ cpu_dead(pcpu_id);
+}
+
#ifdef CONFIG_PARTITION_MODE
void partition_mode_dispatch_interrupt(struct intr_excp_ctx *ctx)
{
@@ -445,79 +470,6 @@ void partition_mode_dispatch_interrupt(struct intr_excp_ctx *ctx)
}
#endif

-static inline bool irq_need_mask(struct irq_desc *desc)
-{
- /* level triggered gsi should be masked */
- return (((desc->flags & IRQF_LEVEL) != 0U)
- && irq_is_gsi(desc->irq));
-}
-
-static inline bool irq_need_unmask(struct irq_desc *desc)
-{
- /* level triggered gsi for non-ptdev should be unmasked */
- return (((desc->flags & IRQF_LEVEL) != 0U)
- && ((desc->flags & IRQF_PT) == 0U)
- && irq_is_gsi(desc->irq));
-}
-
-static inline void handle_irq(struct irq_desc *desc)
-{
- irq_action_t action = desc->action;
-
-
- /* mask iopaic pin */
- if (irq_need_mask(desc)) {
- GSI_MASK_IRQ(desc->irq);
- }
-
- /* Send EOI to LAPIC/IOAPIC IRR */
- send_lapic_eoi();
-
- if (action != NULL) {
- action(desc->irq, desc->priv_data);
- }
-
- if (irq_need_unmask(desc)) {
- GSI_UNMASK_IRQ(desc->irq);
- }
-}
-
-void set_irq_trigger_mode(uint32_t irq, bool is_level_trigger)
-{
- struct irq_desc *desc;
-
- spinlock_rflags;
-
- if (irq >= NR_IRQS) {
- return;
- }
-
- desc = &irq_desc_array[irq];
- spinlock_irqsave_obtain(&desc->lock);
- if (is_level_trigger == true) {
- desc->flags |= IRQF_LEVEL;
- } else {
- desc->flags &= ~IRQF_LEVEL;
- }
- spinlock_irqrestore_release(&desc->lock);
-}
-
-void free_irq(uint32_t irq)
-{
- struct irq_desc *desc;
-
- if (irq >= NR_IRQS) {
- return;
- }
-
- desc = &irq_desc_array[irq];
- dev_dbg(ACRN_DBG_IRQ, "[%s] %s irq%d vr:0x%x",
- __func__, desc->name, irq, irq_to_vector(irq));
-
- unset_irq(desc);
- release_irq_resource(irq);
-}
-
#ifdef HV_DEBUG
void get_cpu_interrupt_info(char *str_arg, int str_max)
{
@@ -556,6 +508,51 @@ void get_cpu_interrupt_info(char *str_arg, int str_max)
}
#endif /* HV_DEBUG */

+static void init_irq_desc(void)
+{
+ uint32_t i;
+
+ for (i = 0U; i < NR_IRQS; i++) {
+ irq_desc_array[i].irq = i;
+ irq_desc_array[i].vector = VECTOR_INVALID;
+ spinlock_init(&irq_desc_array[i].lock);
+ }
+
+ for (i = 0U; i <= NR_MAX_VECTOR; i++) {
+ vector_to_irq[i] = IRQ_INVALID;
+ }
+
+ /* init fixed mapping for specific irq and vector */
+ for (i = 0U; i < NR_STATIC_MAPPINGS; i++) {
+ uint32_t irq = irq_static_mappings[i][0];
+ uint32_t vr = irq_static_mappings[i][1];
+
+ irq_desc_array[irq].vector = vr;
+ irq_desc_array[irq].used = IRQ_ASSIGNED;
+ vector_to_irq[vr] = irq;
+ }
+}
+
+static void disable_pic_irq(void)
+{
+ pio_write8(0xffU, 0xA1U);
+ pio_write8(0xffU, 0x21U);
+}
+
+void init_default_irqs(uint16_t cpu_id)
+{
+ if (cpu_id != BOOT_CPU_ID) {
+ return;
+ }
+
+ init_irq_desc();
+
+ /* we use ioapic only, disable legacy PIC */
+ disable_pic_irq();
+ setup_ioapic_irq();
+ init_softirq();
+}
+
void interrupt_init(uint16_t pcpu_id)
{
struct host_idt_descriptor *idtd = &HOST_IDTR;
--
2.18.0


[PATCH v5 4/5] hv: pirq: clean up fields of struct irq_desc

Yan, Like
 

This commit cleans up fiels of struct irq_desc:
- remove irq_ prefix of irq_lock field of struct irq_desc;
- remove irq_desc_state, irq_cnt and irq_lost_cnt which are not used.
- change enum irq_state to enum irq_use_state;

Signed-off-by: Yan, Like <like.yan@...>
Reviewed-by: Anthony Xu <anthony.xu@...>
---
hypervisor/arch/x86/irq.c | 21 +++++++--------------
hypervisor/include/common/irq.h | 14 +++-----------
2 files changed, 10 insertions(+), 25 deletions(-)

diff --git a/hypervisor/arch/x86/irq.c b/hypervisor/arch/x86/irq.c
index 79470115..52211a20 100644
--- a/hypervisor/arch/x86/irq.c
+++ b/hypervisor/arch/x86/irq.c
@@ -30,7 +30,7 @@ static void init_irq_desc(void)
for (i = 0U; i < NR_IRQS; i++) {
irq_desc_array[i].irq = i;
irq_desc_array[i].vector = VECTOR_INVALID;
- spinlock_init(&irq_desc_array[i].irq_lock);
+ spinlock_init(&irq_desc_array[i].lock);
}

for (i = 0U; i <= NR_MAX_VECTOR; i++) {
@@ -125,7 +125,6 @@ void free_irq_desc(uint32_t irq)
if (irq < NR_IRQS) {
desc = &irq_desc_array[irq];
desc->used = IRQ_NOT_ASSIGNED;
- desc->state = IRQ_DESC_PENDING;
}

spinlock_irqrestore_release(&irq_alloc_spinlock);
@@ -276,7 +275,7 @@ static inline int32_t setup_irq(struct irq_desc *desc,
int32_t retval = 0;;
spinlock_rflags;

- spinlock_irqsave_obtain(&desc->irq_lock);
+ spinlock_irqsave_obtain(&desc->lock);

if (desc->action == NULL) {
desc->priv_data = priv_data;
@@ -289,7 +288,7 @@ static inline int32_t setup_irq(struct irq_desc *desc,
} else {
retval = -EBUSY;
}
- spinlock_irqrestore_release(&desc->irq_lock);
+ spinlock_irqrestore_release(&desc->lock);

return retval;
}
@@ -299,14 +298,14 @@ static inline void unset_irq(struct irq_desc *desc)
{
spinlock_rflags;

- spinlock_irqsave_obtain(&desc->irq_lock);
+ spinlock_irqsave_obtain(&desc->lock);

desc->action = NULL;
desc->priv_data = NULL;
desc->flags = IRQF_NONE;
memset(desc->name, '\0', 32U);

- spinlock_irqrestore_release(&desc->irq_lock);
+ spinlock_irqrestore_release(&desc->lock);
}

int32_t request_irq(uint32_t req_irq,
@@ -494,13 +493,13 @@ void set_irq_trigger_mode(uint32_t irq, bool is_level_trigger)
}

desc = &irq_desc_array[irq];
- spinlock_irqsave_obtain(&desc->irq_lock);
+ spinlock_irqsave_obtain(&desc->lock);
if (is_level_trigger == true) {
desc->flags |= IRQF_LEVEL;
} else {
desc->flags &= ~IRQF_LEVEL;
}
- spinlock_irqrestore_release(&desc->irq_lock);
+ spinlock_irqrestore_release(&desc->lock);
}

void free_irq(uint32_t irq)
@@ -536,9 +535,6 @@ void get_cpu_interrupt_info(char *str_arg, int str_max)
size -= len;
str += len;
}
- len = snprintf(str, size, "\tLOST");
- size -= len;
- str += len;

for (irq = 0U; irq < NR_IRQS; irq++) {
desc = &irq_desc_array[irq];
@@ -554,9 +550,6 @@ void get_cpu_interrupt_info(char *str_arg, int str_max)
size -= len;
str += len;
}
- len = snprintf(str, size, "\t%d", desc->irq_lost_cnt);
- size -= len;
- str += len;
}
}
snprintf(str, size, "\r\n");
diff --git a/hypervisor/include/common/irq.h b/hypervisor/include/common/irq.h
index 358d0912..2ad1291e 100644
--- a/hypervisor/include/common/irq.h
+++ b/hypervisor/include/common/irq.h
@@ -17,23 +17,17 @@ enum irq_mode {
IRQ_DEASSERT,
};

-enum irq_state {
+enum irq_use_state {
IRQ_NOT_ASSIGNED = 0,
IRQ_ASSIGNED,
};

-enum irq_desc_state {
- IRQ_DESC_PENDING,
- IRQ_DESC_IN_PROCESS,
-};
-
typedef int (*irq_action_t)(uint32_t irq, void *priv_data);

/* any field change in below required irq_lock protection with irqsave */
struct irq_desc {
uint32_t irq; /* index to irq_desc_base */
- enum irq_state used; /* this irq have assigned to device */
- enum irq_desc_state state; /* irq_desc status */
+ enum irq_use_state used; /* this irq have assigned to device */
uint32_t vector; /* assigned vector */

uint32_t flags; /* flags for trigger mode/ptdev/percpu */
@@ -41,9 +35,7 @@ struct irq_desc {
void *priv_data; /* irq_action private data */
char name[32]; /* name of component */

- spinlock_t irq_lock;
- uint64_t *irq_cnt; /* this irq cnt happened on CPUs */
- uint64_t irq_lost_cnt;
+ spinlock_t lock;
};

int32_t request_irq(uint32_t irq,
--
2.18.0


[PATCH v5 3/5] hv: pirq: clean up irq handlers

Yan, Like
 

There are several similar irq handlers with confusing function names and it's
not friendly to call update_irq_handler() to update a proper handler after irq
registration.

With this commit, a single generic irq handler is being used, in which, no lock
need to be acquired because our design could guarantee there is no concurrent
irq handling and irq handler request/free.
A flags field is added to irq_desc struct to select the proper processing flow
for an irq. Irqflags is defined as follows:
IRQF_NONE (0U)
IRQF_LEVEL (1U << 1U) /* 1: level trigger; 0: edge trigger */
IRQF_PT (1U << 2U) /* 1: for passthrough dev */

Because we have only one irq handler, update_irq_handler() should be replace by
set_irq_trigger_mode(), whichs set trigger mode flag of a certian irq.

Signed-off-by: Yan, Like <like.yan@...>
---
hypervisor/arch/x86/assign.c | 54 ++-----------
hypervisor/arch/x86/ioapic.c | 4 +-
hypervisor/arch/x86/irq.c | 139 +++++++-------------------------
hypervisor/arch/x86/notify.c | 2 +-
hypervisor/arch/x86/timer.c | 8 +-
hypervisor/arch/x86/vtd.c | 1 +
hypervisor/common/ptdev.c | 2 +-
hypervisor/include/common/irq.h | 11 ++-
8 files changed, 50 insertions(+), 171 deletions(-)

diff --git a/hypervisor/arch/x86/assign.c b/hypervisor/arch/x86/assign.c
index 353baa66..08725ca8 100644
--- a/hypervisor/arch/x86/assign.c
+++ b/hypervisor/arch/x86/assign.c
@@ -139,51 +139,6 @@ lookup_entry_by_vintx(struct vm *vm, uint8_t vpin,
return entry;
}

-static void
-ptdev_update_irq_handler(struct vm *vm, struct ptdev_remapping_info *entry)
-{
- uint32_t phys_irq = entry->allocated_pirq;
- struct ptdev_intx_info *intx = &entry->ptdev_intr_info.intx;
-
- if (entry->type == PTDEV_INTR_MSI) {
- /* all other MSI and normal maskable */
- update_irq_handler(phys_irq, common_handler_edge);
- }
- /* update irq handler for IOAPIC */
- if ((entry->type == PTDEV_INTR_INTX)
- && (intx->vpin_src
- == PTDEV_VPIN_IOAPIC)) {
- union ioapic_rte rte;
- bool trigger_lvl = false;
-
- /* VPIN_IOAPIC src means we have vioapic enabled */
- vioapic_get_rte(vm, intx->virt_pin, &rte);
- if ((rte.full & IOAPIC_RTE_TRGRMOD) == IOAPIC_RTE_TRGRLVL) {
- trigger_lvl = true;
- }
-
- if (trigger_lvl) {
- update_irq_handler(phys_irq, common_dev_handler_level);
- } else {
- update_irq_handler(phys_irq, common_handler_edge);
- }
- }
- /* update irq handler for PIC */
- if ((entry->type == PTDEV_INTR_INTX) && (phys_irq < NR_LEGACY_IRQ)
- && (intx->vpin_src == PTDEV_VPIN_PIC)) {
- enum vpic_trigger trigger;
-
- /* VPIN_PIC src means we have vpic enabled */
- vpic_get_irq_trigger(vm,
- intx->virt_pin, &trigger);
- if (trigger == LEVEL_TRIGGER) {
- update_irq_handler(phys_irq, common_dev_handler_level);
- } else {
- update_irq_handler(phys_irq, common_handler_edge);
- }
- }
-}
-
static bool ptdev_hv_owned_intx(struct vm *vm, struct ptdev_intx_info *info)
{
/* vm0 pin 4 (uart) is owned by hypervisor under debug version */
@@ -677,9 +632,6 @@ int ptdev_msix_remap(struct vm *vm, uint16_t virt_bdf,
entry->ptdev_intr_info.msi.phys_vector =
irq_to_vector(entry->allocated_pirq);

- /* update irq handler according to info in guest */
- ptdev_update_irq_handler(vm, entry);
-
dev_dbg(ACRN_DBG_IRQ,
"PCI %x:%x.%x MSI VR[%d] 0x%x->0x%x assigned to vm%d",
(entry->virt_bdf >> 8) & 0xFFU,
@@ -711,6 +663,7 @@ static void activate_physical_ioapic(struct vm *vm,
{
union ioapic_rte rte;
uint32_t phys_irq = entry->allocated_pirq;
+ bool is_lvl_trigger = false;

/* disable interrupt */
GSI_MASK_IRQ(phys_irq);
@@ -723,7 +676,10 @@ static void activate_physical_ioapic(struct vm *vm,
ioapic_set_rte(phys_irq, rte);

/* update irq handler according to info in guest */
- ptdev_update_irq_handler(vm, entry);
+ if ((rte.full & IOAPIC_RTE_TRGRMOD) == IOAPIC_RTE_TRGRLVL) {
+ is_lvl_trigger = true;
+ }
+ set_irq_trigger_mode(phys_irq, is_lvl_trigger);

/* enable interrupt */
GSI_UNMASK_IRQ(phys_irq);
diff --git a/hypervisor/arch/x86/ioapic.c b/hypervisor/arch/x86/ioapic.c
index c3d7a858..675ca9bf 100644
--- a/hypervisor/arch/x86/ioapic.c
+++ b/hypervisor/arch/x86/ioapic.c
@@ -220,9 +220,9 @@ static void ioapic_set_routing(uint32_t gsi, uint32_t vr)
ioapic_set_rte_entry(addr, gsi_table[gsi].pin, rte);

if ((rte.full & IOAPIC_RTE_TRGRMOD) != 0UL) {
- update_irq_handler(gsi, handle_level_interrupt_common);
+ set_irq_trigger_mode(gsi, true);
} else {
- update_irq_handler(gsi, common_handler_edge);
+ set_irq_trigger_mode(gsi, false);
}

dev_dbg(ACRN_DBG_IRQ, "GSI: irq:%d pin:%hhu rte:%lx",
diff --git a/hypervisor/arch/x86/irq.c b/hypervisor/arch/x86/irq.c
index 87876aef..79470115 100644
--- a/hypervisor/arch/x86/irq.c
+++ b/hypervisor/arch/x86/irq.c
@@ -15,6 +15,8 @@ static uint32_t vector_to_irq[NR_MAX_VECTOR + 1];

spurious_handler_t spurious_handler;

+static inline void handle_irq(struct irq_desc *desc);
+
#define NR_STATIC_MAPPINGS (2U)
static uint32_t irq_static_mappings[NR_STATIC_MAPPINGS][2] = {
{TIMER_IRQ, VECTOR_TIMER},
@@ -268,19 +270,18 @@ static void disable_pic_irq(void)
static inline int32_t setup_irq(struct irq_desc *desc,
irq_action_t action_fn,
void *priv_data,
+ uint32_t flags,
const char *name)
{
int32_t retval = 0;;
spinlock_rflags;

spinlock_irqsave_obtain(&desc->irq_lock);
- if (desc->irq_handler == NULL) {
- desc->irq_handler = common_handler_edge;
- }

if (desc->action == NULL) {
desc->priv_data = priv_data;
desc->action = action_fn;
+ desc->flags = flags;
/* we are okay using strcpy_s here even with spinlock
* since no #PG in HV right now
*/
@@ -302,6 +303,7 @@ static inline void unset_irq(struct irq_desc *desc)

desc->action = NULL;
desc->priv_data = NULL;
+ desc->flags = IRQF_NONE;
memset(desc->name, '\0', 32U);

spinlock_irqrestore_release(&desc->irq_lock);
@@ -310,6 +312,7 @@ static inline void unset_irq(struct irq_desc *desc)
int32_t request_irq(uint32_t req_irq,
irq_action_t action_fn,
void *priv_data,
+ uint32_t flags,
const char *name)
{
struct irq_desc *desc;
@@ -323,7 +326,7 @@ int32_t request_irq(uint32_t req_irq,

desc = &irq_desc_array[irq];

- if (setup_irq(desc, action_fn, priv_data, name) < 0) {
+ if (setup_irq(desc, action_fn, priv_data, flags, name) < 0) {
pr_err("%s: request irq(%u) vr(%u) for %s failed,\
already requested", __func__,
irq, irq_to_vector(irq), name);
@@ -408,12 +411,13 @@ void dispatch_interrupt(struct intr_excp_ctx *ctx)
goto ERR;
}

- if ((desc->used == IRQ_NOT_ASSIGNED) || (desc->irq_handler == NULL)) {
+ if (desc->used == IRQ_NOT_ASSIGNED) {
/* mask irq if possible */
goto ERR;
}

- desc->irq_handler(desc, NULL);
+ handle_irq(desc);
+
return;
ERR:
handle_spurious_interrupt(vr);
@@ -442,98 +446,28 @@ void partition_mode_dispatch_interrupt(struct intr_excp_ctx *ctx)
}
#endif

-int handle_level_interrupt_common(struct irq_desc *desc,
- __unused void *handler_data)
+static inline bool irq_need_mask(struct irq_desc *desc)
{
- irq_action_t action = desc->action;
- spinlock_rflags;
-
- /*
- * give other Core a try to return without hold irq_lock
- * and record irq_lost count here
- */
- if (desc->state != IRQ_DESC_PENDING) {
- send_lapic_eoi();
- desc->irq_lost_cnt++;
- return 0;
- }
-
- spinlock_irqsave_obtain(&desc->irq_lock);
- desc->state = IRQ_DESC_IN_PROCESS;
-
- /* mask iopaic pin */
- if (irq_is_gsi(desc->irq)) {
- GSI_MASK_IRQ(desc->irq);
- }
-
- /* Send EOI to LAPIC/IOAPIC IRR */
- send_lapic_eoi();
-
- if (action != NULL) {
- action(desc->irq, desc->priv_data);
- }
-
- if (irq_is_gsi(desc->irq)) {
- GSI_UNMASK_IRQ(desc->irq);
- }
-
- desc->state = IRQ_DESC_PENDING;
- spinlock_irqrestore_release(&desc->irq_lock);
-
- return 0;
+ /* level triggered gsi should be masked */
+ return (((desc->flags & IRQF_LEVEL) != 0U)
+ && irq_is_gsi(desc->irq));
}

-int common_handler_edge(struct irq_desc *desc, __unused void *handler_data)
+static inline bool irq_need_unmask(struct irq_desc *desc)
{
- irq_action_t action = desc->action;
- spinlock_rflags;
-
- /*
- * give other Core a try to return without hold irq_lock
- * and record irq_lost count here
- */
- if (desc->state != IRQ_DESC_PENDING) {
- send_lapic_eoi();
- desc->irq_lost_cnt++;
- return 0;
- }
-
- spinlock_irqsave_obtain(&desc->irq_lock);
- desc->state = IRQ_DESC_IN_PROCESS;
-
- /* Send EOI to LAPIC/IOAPIC IRR */
- send_lapic_eoi();
-
- if (action != NULL) {
- action(desc->irq, desc->priv_data);
- }
-
- desc->state = IRQ_DESC_PENDING;
- spinlock_irqrestore_release(&desc->irq_lock);
-
- return 0;
+ /* level triggered gsi for non-ptdev should be unmasked */
+ return (((desc->flags & IRQF_LEVEL) != 0U)
+ && ((desc->flags & IRQF_PT) == 0U)
+ && irq_is_gsi(desc->irq));
}

-int common_dev_handler_level(struct irq_desc *desc, __unused void *handler_data)
+static inline void handle_irq(struct irq_desc *desc)
{
irq_action_t action = desc->action;
- spinlock_rflags;

- /*
- * give other Core a try to return without hold irq_lock
- * and record irq_lost count here
- */
- if (desc->state != IRQ_DESC_PENDING) {
- send_lapic_eoi();
- desc->irq_lost_cnt++;
- return 0;
- }
-
- spinlock_irqsave_obtain(&desc->irq_lock);
- desc->state = IRQ_DESC_IN_PROCESS;

/* mask iopaic pin */
- if (irq_is_gsi(desc->irq)) {
+ if (irq_need_mask(desc)) {
GSI_MASK_IRQ(desc->irq);
}

@@ -544,29 +478,12 @@ int common_dev_handler_level(struct irq_desc *desc, __unused void *handler_data)
action(desc->irq, desc->priv_data);
}

- desc->state = IRQ_DESC_PENDING;
- spinlock_irqrestore_release(&desc->irq_lock);
-
- /* we did not unmask irq until guest EOI the vector */
- return 0;
-}
-
-/* no desc->irq_lock for quick handling local interrupt like lapic timer */
-int quick_handler_nolock(struct irq_desc *desc, __unused void *handler_data)
-{
- irq_action_t action = desc->action;
-
- /* Send EOI to LAPIC/IOAPIC IRR */
- send_lapic_eoi();
-
- if (action != NULL) {
- action(desc->irq, desc->priv_data);
- }
-
- return 0;
+ if (irq_need_unmask(desc)) {
+ GSI_UNMASK_IRQ(desc->irq);
+ }
}

-void update_irq_handler(uint32_t irq, irq_handler_t func)
+void set_irq_trigger_mode(uint32_t irq, bool is_level_trigger)
{
struct irq_desc *desc;

@@ -578,7 +495,11 @@ void update_irq_handler(uint32_t irq, irq_handler_t func)

desc = &irq_desc_array[irq];
spinlock_irqsave_obtain(&desc->irq_lock);
- desc->irq_handler = func;
+ if (is_level_trigger == true) {
+ desc->flags |= IRQF_LEVEL;
+ } else {
+ desc->flags &= ~IRQF_LEVEL;
+ }
spinlock_irqrestore_release(&desc->irq_lock);
}

diff --git a/hypervisor/arch/x86/notify.c b/hypervisor/arch/x86/notify.c
index 77351ae9..9ce079db 100644
--- a/hypervisor/arch/x86/notify.c
+++ b/hypervisor/arch/x86/notify.c
@@ -29,7 +29,7 @@ static int request_notification_irq(irq_action_t func, void *data,
}

/* all cpu register the same notification vector */
- retval = request_irq(NOTIFY_IRQ, func, data, name);
+ retval = request_irq(NOTIFY_IRQ, func, data, IRQF_NONE, name);
if (retval < 0) {
pr_err("Failed to add notify isr");
return -ENODEV;
diff --git a/hypervisor/arch/x86/timer.c b/hypervisor/arch/x86/timer.c
index c7a57e4e..baefefe4 100644
--- a/hypervisor/arch/x86/timer.c
+++ b/hypervisor/arch/x86/timer.c
@@ -24,7 +24,7 @@ static void run_timer(struct hv_timer *timer)
}

/* run in interrupt context */
-static int tsc_deadline_handler(__unused int irq, __unused void *data)
+static int tsc_deadline_handler(__unused uint32_t irq, __unused void *data)
{
fire_softirq(SOFTIRQ_TIMER);
return 0;
@@ -111,10 +111,8 @@ static int request_timer_irq(irq_action_t func, const char *name)
{
int32_t retval;

- retval = request_irq(TIMER_IRQ, func, NULL, name);
- if (retval >= 0) {
- update_irq_handler(TIMER_IRQ, quick_handler_nolock);
- } else {
+ retval = request_irq(TIMER_IRQ, func, NULL, IRQF_NONE, name);
+ if (retval < 0) {
pr_err("Failed to add timer isr");
return -ENODEV;
}
diff --git a/hypervisor/arch/x86/vtd.c b/hypervisor/arch/x86/vtd.c
index 414d400d..c292f803 100644
--- a/hypervisor/arch/x86/vtd.c
+++ b/hypervisor/arch/x86/vtd.c
@@ -829,6 +829,7 @@ static int dmar_setup_interrupt(struct dmar_drhd_rt *dmar_uint)
retval = request_irq(IRQ_INVALID,
dmar_fault_handler,
dmar_uint,
+ IRQF_NONE,
"dmar_fault_event");

if (retval < 0 ) {
diff --git a/hypervisor/common/ptdev.c b/hypervisor/common/ptdev.c
index ef12ce0b..c5d9e950 100644
--- a/hypervisor/common/ptdev.c
+++ b/hypervisor/common/ptdev.c
@@ -135,7 +135,7 @@ ptdev_activate_entry(struct ptdev_remapping_info *entry, uint32_t phys_irq)

/* register and allocate host vector/irq */
retval = request_irq(phys_irq, ptdev_interrupt_handler,
- (void *)entry, "dev assign");
+ (void *)entry, IRQF_PT, "dev assign");

ASSERT(retval >= 0, "dev register failed");
entry->allocated_pirq = (uint32_t)retval;
diff --git a/hypervisor/include/common/irq.h b/hypervisor/include/common/irq.h
index ac6edbdf..358d0912 100644
--- a/hypervisor/include/common/irq.h
+++ b/hypervisor/include/common/irq.h
@@ -7,6 +7,10 @@
#ifndef COMMON_IRQ_H
#define COMMON_IRQ_H

+#define IRQF_NONE (0U)
+#define IRQF_LEVEL (1U << 1U) /* 1: level trigger; 0: edge trigger */
+#define IRQF_PT (1U << 2U) /* 1: for passthrough dev */
+
enum irq_mode {
IRQ_PULSE,
IRQ_ASSERT,
@@ -32,8 +36,7 @@ struct irq_desc {
enum irq_desc_state state; /* irq_desc status */
uint32_t vector; /* assigned vector */

- int (*irq_handler)(struct irq_desc *irq_desc, void *handler_data);
- /* callback for irq flow handling */
+ uint32_t flags; /* flags for trigger mode/ptdev/percpu */
irq_action_t action; /* callback registered from component */
void *priv_data; /* irq_action private data */
char name[32]; /* name of component */
@@ -46,10 +49,10 @@ struct irq_desc {
int32_t request_irq(uint32_t irq,
irq_action_t action_fn,
void *priv_data,
+ uint32_t flags,
const char *name);

void free_irq(uint32_t irq);

-typedef int (*irq_handler_t)(struct irq_desc *desc, void *handler_data);
-void update_irq_handler(uint32_t irq, irq_handler_t func);
+void set_irq_trigger_mode(uint32_t irq, bool is_level_trigger);
#endif /* COMMON_IRQ_H */
--
2.18.0


[PATCH v5 2/5] hv: pirq: add setup_irq()/unset_irq()

Yan, Like
 

This commit adds setup_irq()/unset_irq() to setup/unset
following fields of irq_desc: irq_handler, action, action_data, flags, name,
in order to make request_irq()/free_irq() cleaner.

Signed-off-by: Yan, Like <like.yan@...>
Reviewed-by: Anthony Xu <anthony.xu@...>
---
hypervisor/arch/x86/irq.c | 74 ++++++++++++++++++++++++---------------
1 file changed, 46 insertions(+), 28 deletions(-)

diff --git a/hypervisor/arch/x86/irq.c b/hypervisor/arch/x86/irq.c
index 9e20bc06..87876aef 100644
--- a/hypervisor/arch/x86/irq.c
+++ b/hypervisor/arch/x86/irq.c
@@ -264,23 +264,15 @@ static void disable_pic_irq(void)
pio_write8(0xffU, 0x21U);
}

-int32_t request_irq(uint32_t req_irq,
- irq_action_t action_fn,
- void *priv_data,
- const char *name)
+/* setup irq_desc fields */
+static inline int32_t setup_irq(struct irq_desc *desc,
+ irq_action_t action_fn,
+ void *priv_data,
+ const char *name)
{
- struct irq_desc *desc;
- uint32_t irq;
+ int32_t retval = 0;;
spinlock_rflags;

- irq = prepare_irq_resource(req_irq);
- if (irq >= NR_IRQS) {
- pr_err("%s: failed to prepare irq", __func__);
- return -EINVAL;
- }
-
- desc = &irq_desc_array[irq];
-
spinlock_irqsave_obtain(&desc->irq_lock);
if (desc->irq_handler == NULL) {
desc->irq_handler = common_handler_edge;
@@ -289,22 +281,56 @@ int32_t request_irq(uint32_t req_irq,
if (desc->action == NULL) {
desc->priv_data = priv_data;
desc->action = action_fn;
-
/* we are okay using strcpy_s here even with spinlock
* since no #PG in HV right now
*/
(void)strcpy_s(desc->name, 32U, name);
-
} else {
- spinlock_irqrestore_release(&desc->irq_lock);
- release_irq_resource(irq);
+ retval = -EBUSY;
+ }
+ spinlock_irqrestore_release(&desc->irq_lock);
+
+ return retval;
+}
+
+/* clear the fields setup by setup_irq */
+static inline void unset_irq(struct irq_desc *desc)
+{
+ spinlock_rflags;
+
+ spinlock_irqsave_obtain(&desc->irq_lock);
+
+ desc->action = NULL;
+ desc->priv_data = NULL;
+ memset(desc->name, '\0', 32U);
+
+ spinlock_irqrestore_release(&desc->irq_lock);
+}
+
+int32_t request_irq(uint32_t req_irq,
+ irq_action_t action_fn,
+ void *priv_data,
+ const char *name)
+{
+ struct irq_desc *desc;
+ uint32_t irq;
+
+ irq = prepare_irq_resource(req_irq);
+ if (irq >= NR_IRQS) {
+ pr_err("%s: failed to prepare irq", __func__);
+ return -EINVAL;
+ }
+
+ desc = &irq_desc_array[irq];
+
+ if (setup_irq(desc, action_fn, priv_data, name) < 0) {
pr_err("%s: request irq(%u) vr(%u) for %s failed,\
already requested", __func__,
irq, irq_to_vector(irq), name);
+ release_irq_resource(irq);
return -EBUSY;
}

- spinlock_irqrestore_release(&desc->irq_lock);
dev_dbg(ACRN_DBG_IRQ, "[%s] %s irq%d vr:0x%x",
__func__, desc->name, irq, desc->vector);

@@ -560,8 +586,6 @@ void free_irq(uint32_t irq)
{
struct irq_desc *desc;

- spinlock_rflags;
-
if (irq >= NR_IRQS) {
return;
}
@@ -570,14 +594,8 @@ void free_irq(uint32_t irq)
dev_dbg(ACRN_DBG_IRQ, "[%s] %s irq%d vr:0x%x",
__func__, desc->name, irq, irq_to_vector(irq));

- spinlock_irqsave_obtain(&desc->irq_lock);
-
- desc->action = NULL;
- desc->priv_data = NULL;
- memset(desc->name, '\0', 32U);
+ unset_irq(desc);
release_irq_resource(irq);
-
- spinlock_irqrestore_release(&desc->irq_lock);
}

#ifdef HV_DEBUG
--
2.18.0


[PATCH v5 1/5] hv: pirq: refactor irq/vector allocation/free code

Yan, Like
 

This commit refactors the irq_desc and vector allocation/free codes, with:
1) add a global spinlock to protect the irq_desc/vector allocation;

2) three pairs of funtions are defined for irq_desc/vector allocation and
deallocation:
- uint32_t alloc_irq_desc(uint32_t irq)
- if irq is valid, mark the irq_desc as used, or else, alloc an available irq_desc
- return: irq which is an index to the irq_desc on success, IRQ_INVALID on failure.

- void free_irq_desc(uint32_t irq)
- mark the irq_desc unused.

- uint32_t alloc_vector_for_irq(uint32_t irq)
- alloc an available vector, and bind it to irq;
- return: valid vector on success, VECTOR_INVALID on failure.

- void free_vector_for_irq(uint32_t irq)
- free vector and clear certain field of irq_desc.

- uint32_t prepare_irq_resource(uint32_t irq)
- alloc irq_desc and vector by calling alloc_irq_desc() and irq_desc_alloc_vector
according to irq type before setup irq;
- return: irq which is an index to the irq_desc on success, IRQ_INVALID on failure.

- void release_irq_resource(uint32_t irq)
- release dynamical allocated irq_desc and vector.

Signed-off-by: Yan, Like <like.yan@...>
Reviewed-by: Anthony Xu <anthony.xu@...>
---
hypervisor/arch/x86/ioapic.c | 4 +-
hypervisor/arch/x86/irq.c | 306 +++++++++++++++---------------
hypervisor/include/arch/x86/irq.h | 7 +
hypervisor/include/common/irq.h | 7 -
4 files changed, 161 insertions(+), 163 deletions(-)

diff --git a/hypervisor/arch/x86/ioapic.c b/hypervisor/arch/x86/ioapic.c
index 954905b6..c3d7a858 100644
--- a/hypervisor/arch/x86/ioapic.c
+++ b/hypervisor/arch/x86/ioapic.c
@@ -362,7 +362,7 @@ void setup_ioapic_irq(void)
}

/* pinned irq before use it */
- if (irq_mark_used(gsi) >= NR_IRQS) {
+ if (alloc_irq_desc(gsi) >= NR_IRQS) {
pr_err("failed to alloc IRQ[%d]", gsi);
gsi++;
continue;
@@ -372,7 +372,7 @@ void setup_ioapic_irq(void)
* for legacy irq, reserved vector and never free
*/
if (gsi < NR_LEGACY_IRQ) {
- vr = irq_desc_alloc_vector(gsi);
+ vr = alloc_vector_for_irq(gsi);
if (vr > NR_MAX_VECTOR) {
pr_err("failed to alloc VR");
gsi++;
diff --git a/hypervisor/arch/x86/irq.c b/hypervisor/arch/x86/irq.c
index 3d9c5574..9e20bc06 100644
--- a/hypervisor/arch/x86/irq.c
+++ b/hypervisor/arch/x86/irq.c
@@ -8,6 +8,7 @@
#include <softirq.h>

static spinlock_t exception_spinlock = { .head = 0U, .tail = 0U, };
+static spinlock_t irq_alloc_spinlock = { .head = 0U, .tail = 0U, };

static struct irq_desc irq_desc_array[NR_IRQS];
static uint32_t vector_to_irq[NR_MAX_VECTOR + 1];
@@ -62,101 +63,198 @@ static uint32_t find_available_vector()
}

/*
- * check and set irq to be assigned
- * return: IRQ_INVALID if irq already assigned otherwise return irq
+ * find an available irq_desc between nr_gsi ~ NR_IRQS-1
*/
-uint32_t irq_mark_used(uint32_t irq)
+static inline uint32_t find_available_irq_desc()
{
+ uint32_t i;
struct irq_desc *desc;

- spinlock_rflags;
-
- if (irq >= NR_IRQS) {
- return IRQ_INVALID;
+ for (i = irq_gsi_num(); i < NR_IRQS; i++) {
+ desc = &irq_desc_array[i];
+ if (desc->used == IRQ_NOT_ASSIGNED) {
+ return i;
+ }
}

- desc = &irq_desc_array[irq];
- spinlock_irqsave_obtain(&desc->irq_lock);
- if (desc->used == IRQ_NOT_ASSIGNED) {
- desc->used = IRQ_ASSIGNED;
- }
- spinlock_irqrestore_release(&desc->irq_lock);
- return irq;
+ return IRQ_INVALID;
}

/*
- * system find available irq and set assigned
- * return: irq, VECTOR_INVALID not found
+ * alloc an irq_desc if req_irq is IRQ_INVALID, or else set assigned only
+ * return: valid irq as index to irq_desc on success, IRQ_INVALID on failure
*/
-static uint32_t alloc_irq(void)
+uint32_t alloc_irq_desc(uint32_t req_irq)
{
- uint32_t i;
+ uint32_t irq = req_irq;
struct irq_desc *desc;

spinlock_rflags;

- for (i = irq_gsi_num(); i < NR_IRQS; i++) {
- desc = &irq_desc_array[i];
- spinlock_irqsave_obtain(&desc->irq_lock);
+ spinlock_irqsave_obtain(&irq_alloc_spinlock);
+ if (irq == IRQ_INVALID) {
+ irq = find_available_irq_desc();
+ }
+
+ if (irq < NR_IRQS) {
+ desc = &irq_desc_array[irq];
if (desc->used == IRQ_NOT_ASSIGNED) {
desc->used = IRQ_ASSIGNED;
- spinlock_irqrestore_release(&desc->irq_lock);
- break;
+ spinlock_irqrestore_release(&irq_alloc_spinlock);
+ return irq;
}
- spinlock_irqrestore_release(&desc->irq_lock);
}
- return (i == NR_IRQS) ? IRQ_INVALID : i;
+
+ spinlock_irqrestore_release(&irq_alloc_spinlock);
+ pr_err("Failed to alloc irq desc, req_irq[%u]", req_irq);
+ return IRQ_INVALID;
}

-/* need irq_lock protection before use */
-static void local_irq_desc_set_vector(uint32_t irq, uint32_t vr)
+/*
+ * free irq_desc: set irq_desc unassigned
+ */
+void free_irq_desc(uint32_t irq)
{
struct irq_desc *desc;
+ spinlock_rflags;

- desc = &irq_desc_array[irq];
- vector_to_irq[vr] = irq;
- desc->vector = vr;
+ spinlock_irqsave_obtain(&irq_alloc_spinlock);
+
+ if (irq < NR_IRQS) {
+ desc = &irq_desc_array[irq];
+ desc->used = IRQ_NOT_ASSIGNED;
+ desc->state = IRQ_DESC_PENDING;
+ }
+
+ spinlock_irqrestore_release(&irq_alloc_spinlock);
}

-/* lock version of set vector */
-static void irq_desc_set_vector(uint32_t irq, uint32_t vr)
+/*
+ * alloc an vectror and bind it to irq
+ * retval: valid vector num on susccess, VECTOR_INVALID on failure.
+ */
+uint32_t alloc_vector_for_irq(uint32_t irq)
{
+ uint32_t vr;
struct irq_desc *desc;

spinlock_rflags;

+ if (irq >= NR_IRQS) {
+ pr_err("invalid irq[%u] to alloc vector", irq);
+ return VECTOR_INVALID;
+ }
+
desc = &irq_desc_array[irq];
- spinlock_irqsave_obtain(&desc->irq_lock);
- vector_to_irq[vr] = irq;
- desc->vector = vr;
- spinlock_irqrestore_release(&desc->irq_lock);
+
+ spinlock_irqsave_obtain(&irq_alloc_spinlock);
+ if (desc->vector != VECTOR_INVALID) {
+ pr_err("%s: vector[%u] already allocated for irq[%u]",
+ __func__, desc->vector, irq);
+ vr = VECTOR_INVALID;
+ goto OUT;
+ }
+
+ vr = find_available_vector();
+
+ /* FLAT mode, an irq connected to the same vector of each cpu */
+ if (vr > VECTOR_DYNAMIC_END) {
+ pr_err("no vector invalid for irq[%u]", irq);
+ vr = VECTOR_INVALID;
+ } else {
+ desc->vector = vr;
+ vector_to_irq[vr] = irq;
+ }
+OUT:
+ spinlock_irqrestore_release(&irq_alloc_spinlock);
+ return vr;
}

-/* used with holding irq_lock outside */
-static void _irq_desc_free_vector(uint32_t irq)
+/* unbind the vector to irq and free the vector */
+void free_vector_for_irq(uint32_t irq)
{
struct irq_desc *desc;
uint32_t vr;
- uint16_t pcpu_id;
+ spinlock_rflags;

if (irq >= NR_IRQS) {
return;
}

desc = &irq_desc_array[irq];
-
+ spinlock_irqsave_obtain(&irq_alloc_spinlock);
vr = desc->vector;
- desc->used = IRQ_NOT_ASSIGNED;
- desc->state = IRQ_DESC_PENDING;
desc->vector = VECTOR_INVALID;

vr &= NR_MAX_VECTOR;
if (vector_to_irq[vr] == irq) {
vector_to_irq[vr] = IRQ_INVALID;
}
+ spinlock_irqrestore_release(&irq_alloc_spinlock);
+}

- for (pcpu_id = 0U; pcpu_id < phys_cpu_num; pcpu_id++) {
- per_cpu(irq_count, pcpu_id)[irq] = 0UL;
+
+/*
+ * The function prepares irq and vector before registering handler.
+ * There are four cases:
+ * case 1: req_irq = IRQ_INVALID
+ * caller did not know which irq to use, and want system to
+ * allocate available irq for it. These irq are in range:
+ * nr_gsi ~ NR_IRQS
+ * an irq will be allocated and a vector will be assigned to this
+ * irq automatically.
+ * case 2: req_irq >= NR_LAGACY_IRQ and irq < nr_gsi
+ * caller want to add device ISR handler into ioapic pins.
+ * a vector will automatically assigned.
+ * case 3: req_irq >=0 and req_irq < NR_LAGACY_IRQ
+ * caller want to add device ISR handler into ioapic pins, which
+ * is a legacy irq, vector already reserved.
+ * Nothing to do in this case.
+ * case 4: irq with speical type (not from IOAPIC/MSI)
+ * These irq value are pre-defined for Timer, IPI, Spurious etc,
+ * which is listed in irq_static_mappings[].
+ * Nothing to do in this case.
+ *
+ * return value: valid irq if successfully prepared, otherwise IRQ_INVALID
+ */
+static uint32_t prepare_irq_resource(uint32_t req_irq)
+{
+ uint32_t irq = req_irq;
+ uint32_t vr;
+
+ if (irq >= NR_IRQS) {
+ /* case 1: */
+ irq = alloc_irq_desc(irq);
+ if (irq == IRQ_INVALID) {
+ return IRQ_INVALID;
+ }
+
+ vr = alloc_vector_for_irq(irq);
+ if (vr == VECTOR_INVALID) {
+ free_irq_desc(irq);
+ return IRQ_INVALID;
+ }
+ } else if (irq_is_gsi(irq) && irq >= NR_LEGACY_IRQ) {
+ /* case 2: */
+ vr = alloc_vector_for_irq(irq);
+ if (vr == VECTOR_INVALID) {
+ return IRQ_INVALID;
+ }
+ }
+
+ return irq;
+}
+
+/* release irq & vector if dynamically allocated */
+static void release_irq_resource(uint32_t irq)
+{
+ struct irq_desc *desc = &irq_desc_array[irq];
+
+ if (irq_is_gsi(irq) && irq >= NR_LEGACY_IRQ) {
+ free_vector_for_irq(irq);
+ } else if (!irq_is_gsi(irq) && desc->vector <= VECTOR_DYNAMIC_END) {
+ free_vector_for_irq(irq);
+ free_irq_desc(irq);
}
}

@@ -166,78 +264,29 @@ static void disable_pic_irq(void)
pio_write8(0xffU, 0x21U);
}

-int32_t request_irq(uint32_t irq_arg,
+int32_t request_irq(uint32_t req_irq,
irq_action_t action_fn,
void *priv_data,
const char *name)
{
struct irq_desc *desc;
- uint32_t irq = irq_arg, vector;
+ uint32_t irq;
spinlock_rflags;

- /* ======================================================
- * This is low level ISR handler registering function
- * case: irq = IRQ_INVALID
- * caller did not know which irq to use, and want system to
- * allocate available irq for it. These irq are in range:
- * nr_gsi ~ NR_IRQS
- * a irq will be allocated and the vector will be assigned to this
- * irq automatically.
- *
- * case: irq >=0 and irq < nr_gsi
- * caller want to add device ISR handler into ioapic pins.
- * two kind of devices: legacy device and PCI device with INTx
- * a vector will automatically assigned.
- *
- * case: irq with speical type (not from IOAPIC/MSI)
- * These irq value are pre-defined for Timer, IPI, Spurious etc
- * vectors are pre-defined also
- *
- * return value: pinned irq and assigned vector for this irq.
- * caller can use this irq to enable/disable/mask/unmask interrupt
- * and if this irq is for:
- * GSI legacy: nothing to do for legacy irq, already initialized
- * GSI other: need to progam PCI INTx to match this irq pin
- * MSI: caller need program vector to PCI device
- *
- * =====================================================
- */
-
- /* HV select a irq for device if irq < 0
- * this vector/irq match to APCI DSDT or PCI INTx/MSI
- */
- if (irq == IRQ_INVALID) {
- irq = alloc_irq();
- } else {
- irq = irq_mark_used(irq);
- }
-
+ irq = prepare_irq_resource(req_irq);
if (irq >= NR_IRQS) {
- pr_err("failed to assign IRQ");
+ pr_err("%s: failed to prepare irq", __func__);
return -EINVAL;
}

desc = &irq_desc_array[irq];
+
+ spinlock_irqsave_obtain(&desc->irq_lock);
if (desc->irq_handler == NULL) {
desc->irq_handler = common_handler_edge;
}

- vector = desc->vector;
- if (vector >= VECTOR_FIXED_START &&
- vector <= VECTOR_FIXED_END) {
- irq_desc_set_vector(irq, vector);
- } else if (vector > NR_MAX_VECTOR) {
- irq_desc_alloc_vector(irq);
- }
-
- if (desc->vector == VECTOR_INVALID) {
- pr_err("the input vector is not correct");
- /* FIXME: free allocated irq */
- return -EINVAL;
- }
-
if (desc->action == NULL) {
- spinlock_irqsave_obtain(&desc->irq_lock);
desc->priv_data = priv_data;
desc->action = action_fn;

@@ -246,73 +295,22 @@ int32_t request_irq(uint32_t irq_arg,
*/
(void)strcpy_s(desc->name, 32U, name);

- spinlock_irqrestore_release(&desc->irq_lock);
} else {
+ spinlock_irqrestore_release(&desc->irq_lock);
+ release_irq_resource(irq);
pr_err("%s: request irq(%u) vr(%u) for %s failed,\
already requested", __func__,
irq, irq_to_vector(irq), name);
return -EBUSY;
}

+ spinlock_irqrestore_release(&desc->irq_lock);
dev_dbg(ACRN_DBG_IRQ, "[%s] %s irq%d vr:0x%x",
__func__, desc->name, irq, desc->vector);

return (int32_t)irq;
}

-/* it is safe to call irq_desc_alloc_vector multiple times*/
-uint32_t irq_desc_alloc_vector(uint32_t irq)
-{
- uint32_t vr = VECTOR_INVALID;
- struct irq_desc *desc;
-
- spinlock_rflags;
-
- /* irq should be always available at this time */
- if (irq >= NR_IRQS) {
- return VECTOR_INVALID;
- }
-
- desc = &irq_desc_array[irq];
- spinlock_irqsave_obtain(&desc->irq_lock);
- if (desc->vector != VECTOR_INVALID) {
- /* already allocated a vector */
- goto OUT;
- }
-
- /* FLAT mode, a irq connected to every cpu's same vector */
- vr = find_available_vector();
- if (vr > NR_MAX_VECTOR) {
- pr_err("no vector found for irq[%d]", irq);
- goto OUT;
- }
- local_irq_desc_set_vector(irq, vr);
-OUT:
- spinlock_irqrestore_release(&desc->irq_lock);
- return vr;
-}
-
-void irq_desc_try_free_vector(uint32_t irq)
-{
- struct irq_desc *desc;
-
- spinlock_rflags;
-
- /* legacy irq's vector is reserved and should not be freed */
- if ((irq >= NR_IRQS) || (irq < NR_LEGACY_IRQ)) {
- return;
- }
-
- desc = &irq_desc_array[irq];
- spinlock_irqsave_obtain(&desc->irq_lock);
- if (desc->action == NULL) {
- _irq_desc_free_vector(irq);
- }
-
- spinlock_irqrestore_release(&desc->irq_lock);
-
-}
-
uint32_t irq_to_vector(uint32_t irq)
{
if (irq < NR_IRQS) {
@@ -577,9 +575,9 @@ void free_irq(uint32_t irq)
desc->action = NULL;
desc->priv_data = NULL;
memset(desc->name, '\0', 32U);
+ release_irq_resource(irq);

spinlock_irqrestore_release(&desc->irq_lock);
- irq_desc_try_free_vector(desc->irq);
}

#ifdef HV_DEBUG
diff --git a/hypervisor/include/arch/x86/irq.h b/hypervisor/include/arch/x86/irq.h
index 60c276c8..b67d9b8b 100644
--- a/hypervisor/include/arch/x86/irq.h
+++ b/hypervisor/include/arch/x86/irq.h
@@ -67,6 +67,13 @@ void setup_notification(void);
typedef void (*spurious_handler_t)(uint32_t vector);
extern spurious_handler_t spurious_handler;

+uint32_t alloc_irq_desc(uint32_t req_irq);
+void free_irq_desc(uint32_t irq);
+uint32_t alloc_vector_for_irq(uint32_t req_irq);
+void free_vector_for_irq(uint32_t irq);
+
+uint32_t irq_to_vector(uint32_t irq);
+
/*
* Some MSI message definitions
*/
diff --git a/hypervisor/include/common/irq.h b/hypervisor/include/common/irq.h
index 627b31dc..ac6edbdf 100644
--- a/hypervisor/include/common/irq.h
+++ b/hypervisor/include/common/irq.h
@@ -43,13 +43,6 @@ struct irq_desc {
uint64_t irq_lost_cnt;
};

-uint32_t irq_mark_used(uint32_t irq);
-
-uint32_t irq_desc_alloc_vector(uint32_t irq);
-void irq_desc_try_free_vector(uint32_t irq);
-
-uint32_t irq_to_vector(uint32_t irq);
-
int32_t request_irq(uint32_t irq,
irq_action_t action_fn,
void *priv_data,
--
2.18.0


[PATCH v5 0/5] Patches for physical irq reshuffle

Yan, Like
 

Patch [2,3,4,5/9] of v4 have been PRed. This series includes the rest patches, which:
- [1/5, 2/5] refactored codes for irq request/free code clearer and easy to read;
- [3/5] merged confusing irq handlers into a single generic one with lock removed;
- [4/5, 5/5] clean up codes.

---
Changes:
v1 --> v2:
- rebased the patches to the top of newer codebase;
- edited the commit messages;
- reordered the patches to bring related commits closer;
- removed ptdev dependency on irq sharing;
- added patches 8,9,10 to clean up codes.

v2 --> v3:
- [1/10] was PR separatly;
- change handle_irq_percpu() to fetch the desc->action w/ lock, execute action w/o lock;
- ptdev_remapping_info: allocated_irq changed to allocated_pirq;
- irq_desc: action_data changed to priv_data;
- assign desc before using it;
- pass the needed info via arguments to ptdev_update_irq_desc only;

v3 --> v4:
- removed lock in handle_irq() which is not necessary, merged handle_irq_percpu()
into handle_irq(), and revised with no lock acquired;
- removed unnecessary fields in irq_desc struct, including state, irq_lost_cnt, irq_handler;
- removed IRQF_PERCPU.

v4 --> v5:
- patch [2,3,4,5/9] of v4 have been PRed;
- reordered/rebased the rest patches.

Yan, Like (5):
hv: pirq: refactor irq/vector allocation/free code
hv: pirq: add setup_irq()/unset_irq()
hv: pirq: clean up irq handlers
hv: pirq: clean up fields of struct irq_desc
hv: pirq: change the order of functions within irq.c

hypervisor/arch/x86/assign.c | 54 +--
hypervisor/arch/x86/ioapic.c | 8 +-
hypervisor/arch/x86/irq.c | 667 +++++++++++++-----------------
hypervisor/arch/x86/notify.c | 2 +-
hypervisor/arch/x86/timer.c | 8 +-
hypervisor/arch/x86/vtd.c | 1 +
hypervisor/common/ptdev.c | 2 +-
hypervisor/include/arch/x86/irq.h | 7 +
hypervisor/include/common/irq.h | 32 +-
9 files changed, 329 insertions(+), 452 deletions(-)

--
2.18.0


Re: [PATCH] hv: vuart: fix 'Shifting value too far'

Wu, Xiangyang
 

On Tuesday, August 14, 2018 05:13 PM, Shiqing Gao wrote:
Is it safer to use "vu->dll = (uint8_t)(divisor & 0xFFU)"?
For this case, directly type conversion is safe and simple. BTW, we should use narrow type conversion carefully, it is dangerous in some case since it will drop high end value.

BRs,
Xiangyang Wu.


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

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

On 08/15/2018 10:27 AM, Xu, Anthony wrote:
Did you see VM_Exit with lock prefix?
If not, can we add error message for now.
OK. Let me add log for now.

Regards
Yin, Fengwei

Anthony

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


On 08/15/2018 05:07 AM, Xu, Anthony wrote:
Does this trigger GP in guest before triggering VM_Exit?
According to SDM 25.1.1 Relative priority of Faults and VM Exits,
The faults with higher priority than VM exit doesn't include lock
prefix.

Also there is no other place talking about the lock prefix behavior
in guest. So I assume it triggers VM exit instead of GP.

Regards
Yin, Fengwei


Anthony



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

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

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

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

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

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

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

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

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

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





Re: [PATCH] hv: vuart: fix 'Shifting value too far'

Shiqing Gao
 

On 8/15/2018 10:36 AM, bounce+13726+10562+767559+1700533@... wrote:


On Tuesday, August 14, 2018 05:13 PM, Shiqing Gao wrote:

On 8/14/2018 4:46 PM, bounce+13726+10486+767559+1700533@... wrote:


On Tuesday, August 14, 2018 03:29 PM, Shiqing Gao wrote:
MISRA-C requires that shift operation cannot exceed the word length.

What this patch does:
- Fix the bug in 'vuart_init'
   The register 'dll' and 'dlh' should be uint8_t rather than char.
   'dll' is the lower 8-bit of divisor.
   'dlh' is the higher 8-bit of divisor.
   So, the shift value should be 8U rather than 16U.
- Fix other data type issues regarding to the registers in 'struct
   vuart'
   The registers should be unsigned variables.

Tracked-On: #861
Signed-off-by: Shiqing Gao <shiqing.gao@...>
---
  hypervisor/debug/vuart.c         | 36 +++++++++++++++++-------------------
  hypervisor/include/debug/vuart.h | 20 ++++++++++----------
  2 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/hypervisor/debug/vuart.c b/hypervisor/debug/vuart.c
index 57e2611..549a8f4 100644
--- a/hypervisor/debug/vuart.c
+++ b/hypervisor/debug/vuart.c
@@ -133,9 +133,8 @@ static void vuart_toggle_intr(struct vuart *vu)
      }
  }
  -static void vuart_write(__unused struct vm_io_handler *hdlr,
-        struct vm *vm, uint16_t offset_arg,
-        __unused size_t width, uint32_t value)
+static void vuart_write(__unused struct vm_io_handler *hdlr, struct vm *vm,
+        uint16_t offset_arg, __unused size_t width, uint32_t value)
  {
      uint16_t offset = offset_arg;
      struct vuart *vu = vm_vuart(vm);
@@ -146,12 +145,12 @@ static void vuart_write(__unused struct vm_io_handler *hdlr,
       */
      if ((vu->lcr & LCR_DLAB) != 0) {
          if (offset == UART16550_DLL) {
-            vu->dll = value;
+            vu->dll = (uint8_t)value;
              goto done;
          }
            if (offset == UART16550_DLM) {
-            vu->dlh = value;
+            vu->dlh = (uint8_t)value;
              goto done;
          }
      }
@@ -166,7 +165,7 @@ static void vuart_write(__unused struct vm_io_handler *hdlr,
           * Apply mask so that bits 4-7 are 0
           * Also enables bits 0-3 only if they're 1
           */
-        vu->ier = value & 0x0FU;
+        vu->ier = (uint8_t)(value & 0x0FU);
Can we change "uint32_t value" as "uint8_t value"? Please check it.
No, vuart_write and vuart_read are used to register a io_emulation_handler.

        register_io_emulation_handler(vm, &range, vuart_read, vuart_write);

The last parameter of register_io_emulation_handler is a function pointer with predefined format.

register_io_emulation_handler are used multiple times in hypervisor. We are not able to change them all to uint8_t.

void register_io_emulation_handler(struct vm *vm, struct vm_io_range *range,
        io_read_fn_t io_read_fn_ptr,
        io_write_fn_t io_write_fn_ptr)

typedef
void (*io_write_fn_t)(struct vm_io_handler *handler, struct vm *vm,
                uint16_t port, size_t size, uint32_t val);

Got it, can we use temporary variable "uint8_t value8 = (uint8_t) value;", so we can avoid many type conversion?
Sure. I will update the patch per our discussion.

BRs,
Xiangyang Wu




          break;
      case UART16550_FCR:
          /*
@@ -174,18 +173,18 @@ static void vuart_write(__unused struct vm_io_handler *hdlr,
           * of other FCR bits to be effective.
           */
          if ((value & FCR_FIFOE) == 0U) {
-            vu->fcr = 0;
+            vu->fcr = 0U;
          } else {
              if ((value & FCR_RFR) != 0U) {
                  fifo_reset(&vu->rxfifo);
              }
  -            vu->fcr = value &
-                (FCR_FIFOE | FCR_DMA | FCR_RX_MASK);
+            vu->fcr = (uint8_t)(value &
+                (FCR_FIFOE | FCR_DMA | FCR_RX_MASK));
          }
          break;
      case UART16550_LCR:
-        vu->lcr = value;
+        vu->lcr = (uint8_t)value;
          break;
      case UART16550_MCR:
          /* ignore modem */
@@ -202,7 +201,7 @@ static void vuart_write(__unused struct vm_io_handler *hdlr,
           */
          break;
      case UART16550_SCR:
-        vu->scr = value;
+        vu->scr = (uint8_t)value;
          break;
      default:
          /*
@@ -218,14 +217,13 @@ done:
      vuart_unlock(vu);
  }
  -static uint32_t vuart_read(__unused struct vm_io_handler *hdlr,
-        struct vm *vm, uint16_t offset_arg,
-        __unused size_t width)
+static uint32_t vuart_read(__unused struct vm_io_handler *hdlr, struct vm *vm,
+        uint16_t offset_arg, __unused size_t width)
  {
      uint16_t offset = offset_arg;
-    char iir, reg;
-    uint8_t intr_reason;
+    uint8_t iir, reg, intr_reason;
      struct vuart *vu = vm_vuart(vm);
+
      offset -= vu->base;
      vuart_lock(vu);
      /*
@@ -295,7 +293,7 @@ static uint32_t vuart_read(__unused struct vm_io_handler *hdlr,
  done:
      vuart_toggle_intr(vu);
      vuart_unlock(vu);
-    return reg;
+    return (uint32_t)reg;
  }
    static void vuart_register_io_handler(struct vm *vm)
@@ -370,8 +368,8 @@ void *vuart_init(struct vm *vm)
        /* Set baud rate*/
      divisor = UART_CLOCK_RATE / BAUD_9600 / 16U;
-    vu->dll = divisor;
-    vu->dlh = divisor >> 16U;
+    vu->dll = (uint8_t)(divisor & 0xFFU);
Can we change it as "vu->dll = (uint8_t)divisor" ? please check it.
Is it safer to use "vu->dll = (uint8_t)(divisor & 0xFFU)"?


BRs,
Xiangyang Wu.

+    vu->dlh = (uint8_t)(divisor >> 8U);
        vu->active = false;
      vu->base = COM1_BASE;
diff --git a/hypervisor/include/debug/vuart.h b/hypervisor/include/debug/vuart.h
index 38499a1..ea0754c 100644
--- a/hypervisor/include/debug/vuart.h
+++ b/hypervisor/include/debug/vuart.h
@@ -39,16 +39,16 @@ struct fifo {
  };
    struct vuart {
-    char data;        /* Data register (R/W) */
-    char ier;        /* Interrupt enable register (R/W) */
-    char lcr;        /* Line control register (R/W) */
-    char mcr;        /* Modem control register (R/W) */
-    char lsr;        /* Line status register (R/W) */
-    char msr;        /* Modem status register (R/W) */
-    char fcr;        /* FIFO control register (W) */
-    char scr;        /* Scratch register (R/W) */
-    char dll;        /* Baudrate divisor latch LSB */
-    char dlh;        /* Baudrate divisor latch MSB */
+    uint8_t data;        /* Data register (R/W) */
+    uint8_t ier;        /* Interrupt enable register (R/W) */
+    uint8_t lcr;        /* Line control register (R/W) */
+    uint8_t mcr;        /* Modem control register (R/W) */
+    uint8_t lsr;        /* Line status register (R/W) */
+    uint8_t msr;        /* Modem status register (R/W) */
+    uint8_t fcr;        /* FIFO control register (W) */
+    uint8_t scr;        /* Scratch register (R/W) */
+    uint8_t dll;        /* Baudrate divisor latch LSB */
+    uint8_t dlh;        /* Baudrate divisor latch MSB */
        struct fifo rxfifo;
      struct fifo txfifo;





Re: [PATCH] hv: vuart: fix 'Shifting value too far'

Wu, Xiangyang
 

On Tuesday, August 14, 2018 05:13 PM, Shiqing Gao wrote:

On 8/14/2018 4:46 PM, bounce+13726+10486+767559+1700533@... wrote:


On Tuesday, August 14, 2018 03:29 PM, Shiqing Gao wrote:
MISRA-C requires that shift operation cannot exceed the word length.

What this patch does:
- Fix the bug in 'vuart_init'
   The register 'dll' and 'dlh' should be uint8_t rather than char.
   'dll' is the lower 8-bit of divisor.
   'dlh' is the higher 8-bit of divisor.
   So, the shift value should be 8U rather than 16U.
- Fix other data type issues regarding to the registers in 'struct
   vuart'
   The registers should be unsigned variables.

Tracked-On: #861
Signed-off-by: Shiqing Gao <shiqing.gao@...>
---
  hypervisor/debug/vuart.c         | 36 +++++++++++++++++-------------------
  hypervisor/include/debug/vuart.h | 20 ++++++++++----------
  2 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/hypervisor/debug/vuart.c b/hypervisor/debug/vuart.c
index 57e2611..549a8f4 100644
--- a/hypervisor/debug/vuart.c
+++ b/hypervisor/debug/vuart.c
@@ -133,9 +133,8 @@ static void vuart_toggle_intr(struct vuart *vu)
      }
  }
  -static void vuart_write(__unused struct vm_io_handler *hdlr,
-        struct vm *vm, uint16_t offset_arg,
-        __unused size_t width, uint32_t value)
+static void vuart_write(__unused struct vm_io_handler *hdlr, struct vm *vm,
+        uint16_t offset_arg, __unused size_t width, uint32_t value)
  {
      uint16_t offset = offset_arg;
      struct vuart *vu = vm_vuart(vm);
@@ -146,12 +145,12 @@ static void vuart_write(__unused struct vm_io_handler *hdlr,
       */
      if ((vu->lcr & LCR_DLAB) != 0) {
          if (offset == UART16550_DLL) {
-            vu->dll = value;
+            vu->dll = (uint8_t)value;
              goto done;
          }
            if (offset == UART16550_DLM) {
-            vu->dlh = value;
+            vu->dlh = (uint8_t)value;
              goto done;
          }
      }
@@ -166,7 +165,7 @@ static void vuart_write(__unused struct vm_io_handler *hdlr,
           * Apply mask so that bits 4-7 are 0
           * Also enables bits 0-3 only if they're 1
           */
-        vu->ier = value & 0x0FU;
+        vu->ier = (uint8_t)(value & 0x0FU);
Can we change "uint32_t value" as "uint8_t value"? Please check it.
No, vuart_write and vuart_read are used to register a io_emulation_handler.

        register_io_emulation_handler(vm, &range, vuart_read, vuart_write);

The last parameter of register_io_emulation_handler is a function pointer with predefined format.

register_io_emulation_handler are used multiple times in hypervisor. We are not able to change them all to uint8_t.

void register_io_emulation_handler(struct vm *vm, struct vm_io_range *range,
        io_read_fn_t io_read_fn_ptr,
        io_write_fn_t io_write_fn_ptr)

typedef
void (*io_write_fn_t)(struct vm_io_handler *handler, struct vm *vm,
                uint16_t port, size_t size, uint32_t val);

Got it, can we use temporary variable "uint8_t value8 = (uint8_t) value;", so we can avoid many type conversion?

BRs,
Xiangyang Wu




          break;
      case UART16550_FCR:
          /*
@@ -174,18 +173,18 @@ static void vuart_write(__unused struct vm_io_handler *hdlr,
           * of other FCR bits to be effective.
           */
          if ((value & FCR_FIFOE) == 0U) {
-            vu->fcr = 0;
+            vu->fcr = 0U;
          } else {
              if ((value & FCR_RFR) != 0U) {
                  fifo_reset(&vu->rxfifo);
              }
  -            vu->fcr = value &
-                (FCR_FIFOE | FCR_DMA | FCR_RX_MASK);
+            vu->fcr = (uint8_t)(value &
+                (FCR_FIFOE | FCR_DMA | FCR_RX_MASK));
          }
          break;
      case UART16550_LCR:
-        vu->lcr = value;
+        vu->lcr = (uint8_t)value;
          break;
      case UART16550_MCR:
          /* ignore modem */
@@ -202,7 +201,7 @@ static void vuart_write(__unused struct vm_io_handler *hdlr,
           */
          break;
      case UART16550_SCR:
-        vu->scr = value;
+        vu->scr = (uint8_t)value;
          break;
      default:
          /*
@@ -218,14 +217,13 @@ done:
      vuart_unlock(vu);
  }
  -static uint32_t vuart_read(__unused struct vm_io_handler *hdlr,
-        struct vm *vm, uint16_t offset_arg,
-        __unused size_t width)
+static uint32_t vuart_read(__unused struct vm_io_handler *hdlr, struct vm *vm,
+        uint16_t offset_arg, __unused size_t width)
  {
      uint16_t offset = offset_arg;
-    char iir, reg;
-    uint8_t intr_reason;
+    uint8_t iir, reg, intr_reason;
      struct vuart *vu = vm_vuart(vm);
+
      offset -= vu->base;
      vuart_lock(vu);
      /*
@@ -295,7 +293,7 @@ static uint32_t vuart_read(__unused struct vm_io_handler *hdlr,
  done:
      vuart_toggle_intr(vu);
      vuart_unlock(vu);
-    return reg;
+    return (uint32_t)reg;
  }
    static void vuart_register_io_handler(struct vm *vm)
@@ -370,8 +368,8 @@ void *vuart_init(struct vm *vm)
        /* Set baud rate*/
      divisor = UART_CLOCK_RATE / BAUD_9600 / 16U;
-    vu->dll = divisor;
-    vu->dlh = divisor >> 16U;
+    vu->dll = (uint8_t)(divisor & 0xFFU);
Can we change it as "vu->dll = (uint8_t)divisor" ? please check it.
Is it safer to use "vu->dll = (uint8_t)(divisor & 0xFFU)"?


BRs,
Xiangyang Wu.

+    vu->dlh = (uint8_t)(divisor >> 8U);
        vu->active = false;
      vu->base = COM1_BASE;
diff --git a/hypervisor/include/debug/vuart.h b/hypervisor/include/debug/vuart.h
index 38499a1..ea0754c 100644
--- a/hypervisor/include/debug/vuart.h
+++ b/hypervisor/include/debug/vuart.h
@@ -39,16 +39,16 @@ struct fifo {
  };
    struct vuart {
-    char data;        /* Data register (R/W) */
-    char ier;        /* Interrupt enable register (R/W) */
-    char lcr;        /* Line control register (R/W) */
-    char mcr;        /* Modem control register (R/W) */
-    char lsr;        /* Line status register (R/W) */
-    char msr;        /* Modem status register (R/W) */
-    char fcr;        /* FIFO control register (W) */
-    char scr;        /* Scratch register (R/W) */
-    char dll;        /* Baudrate divisor latch LSB */
-    char dlh;        /* Baudrate divisor latch MSB */
+    uint8_t data;        /* Data register (R/W) */
+    uint8_t ier;        /* Interrupt enable register (R/W) */
+    uint8_t lcr;        /* Line control register (R/W) */
+    uint8_t mcr;        /* Modem control register (R/W) */
+    uint8_t lsr;        /* Line status register (R/W) */
+    uint8_t msr;        /* Modem status register (R/W) */
+    uint8_t fcr;        /* FIFO control register (W) */
+    uint8_t scr;        /* Scratch register (R/W) */
+    uint8_t dll;        /* Baudrate divisor latch LSB */
+    uint8_t dlh;        /* Baudrate divisor latch MSB */
        struct fifo rxfifo;
      struct fifo txfifo;



[PATCH v4] HV: fix "missing for discarded return value" for vm related api

Victor Sun
 

- add handler if prepare_vm() failed;

- replace assert in start_vm()/prepare_vm() with return errno;

- return errno in start_vm()/reset_vm()/prepare_vm() so that caller
could handle it;

changelog:
v3 -> v4: add fix in partition mode;
replace return -1 with return errno;
v2 -> v3: replace panic with pr_fatal and return directly
if prepare_vm0() failed;
v1 -> v2: panic if prepare_vm0() failed instead of reboot system;

Signed-off-by: Victor Sun <victor.sun@...>
---
hypervisor/arch/x86/cpu.c | 10 ++++++++--
hypervisor/arch/x86/guest/vm.c | 21 +++++++++++----------
hypervisor/common/hypercall.c | 3 +--
3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 4d7bf08..f63909a 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -502,7 +502,10 @@ static void bsp_boot_post(void)

exec_vmxon_instr(BOOT_CPU_ID);

- prepare_vm(BOOT_CPU_ID);
+ if (prepare_vm(BOOT_CPU_ID) != 0) {
+ pr_fatal("Prepare VM0 failed!");
+ return;
+ }

default_idle();

@@ -579,7 +582,10 @@ static void cpu_secondary_post(void)
exec_vmxon_instr(get_cpu_id());

#ifdef CONFIG_PARTITION_MODE
- prepare_vm(get_cpu_id());
+ if (prepare_vm(get_cpu_id()) != 0) {
+ pr_fatal("Prepare VM failed!");
+ return;
+ }
#endif

default_idle();
diff --git a/hypervisor/arch/x86/guest/vm.c b/hypervisor/arch/x86/guest/vm.c
index f2efbf9..a8fbaa7 100644
--- a/hypervisor/arch/x86/guest/vm.c
+++ b/hypervisor/arch/x86/guest/vm.c
@@ -340,9 +340,12 @@ int start_vm(struct vm *vm)

/* Only start BSP (vid = 0) and let BSP start other APs */
vcpu = vcpu_from_vid(vm, 0U);
- ASSERT(vcpu != NULL, "vm%d, vcpu0", vm->vm_id);
+ if (vcpu == NULL) {
+ return -EINVAL;
+ }
schedule_vcpu(vcpu);

+ pr_acrnlog("Start VM%d", vm->vm_id);
return 0;
}

@@ -355,7 +358,7 @@ int reset_vm(struct vm *vm)
struct vcpu *vcpu = NULL;

if (vm->state != VM_PAUSED)
- return -1;
+ return -EINVAL;

foreach_vcpu(i, vm, vcpu) {
reset_vcpu(vcpu);
@@ -369,8 +372,7 @@ int reset_vm(struct vm *vm)
destroy_secure_world(vm, false);
vm->sworld_control.flag.active = 0UL;

- start_vm(vm);
- return 0;
+ return (start_vm(vm));
}

/**
@@ -450,7 +452,10 @@ int prepare_vm(uint16_t pcpu_id)

if (is_vm_bsp) {
ret = create_vm(vm_desc, &vm);
- ASSERT(ret == 0, "VM creation failed!");
+ if (ret != 0) {
+ pr_fatal("VM creation failed!");
+ return ret;
+ }

mptable_build(vm);

@@ -461,9 +466,7 @@ int prepare_vm(uint16_t pcpu_id)
prepare_vcpu(vm, vm_desc->vm_pcpu_ids[i]);

/* start vm BSP automatically */
- start_vm(vm);
-
- pr_acrnlog("Start VM%x", vm_desc->vm_id);
+ ret = start_vm(vm);
}

return ret;
@@ -499,8 +502,6 @@ int prepare_vm0(void)
/* start vm0 BSP automatically */
err = start_vm(vm);

- pr_acrnlog("Start VM0");
-
return err;
}

diff --git a/hypervisor/common/hypercall.c b/hypervisor/common/hypercall.c
index fd4bc29..fdcfca0 100644
--- a/hypervisor/common/hypercall.c
+++ b/hypervisor/common/hypercall.c
@@ -280,8 +280,7 @@ int32_t hcall_reset_vm(uint16_t vmid)
if ((target_vm == NULL) || is_vm0(target_vm))
return -1;

- reset_vm(target_vm);
- return 0;
+ return (reset_vm(target_vm));
}

int32_t hcall_assert_irqline(struct vm *vm, uint16_t vmid, uint64_t param)
--
2.7.4


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

Xu, Anthony
 

Did you see VM_Exit with lock prefix?

If not, can we add error message for now.


Anthony

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


On 08/15/2018 05:07 AM, Xu, Anthony wrote:
Does this trigger GP in guest before triggering VM_Exit?
According to SDM 25.1.1 Relative priority of Faults and VM Exits,
The faults with higher priority than VM exit doesn't include lock
prefix.

Also there is no other place talking about the lock prefix behavior
in guest. So I assume it triggers VM exit instead of GP.

Regards
Yin, Fengwei


Anthony



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

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

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

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

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

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

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

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

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

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





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

Kaige Fu
 

On 08-14 Tue 23:34, Eddie Dong wrote:

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

Can you have a cleanup?
Sure, will go through the current code and see if there is something should be clean up.

Thx Eddie



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

Yu Wang
 

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

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

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

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

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

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

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

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

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

+/* port mapping status */
#define VPORT_FREE (0)
-#define VPORT_ASSIGNED (-1)
+#define VPORT_ASSIGNED (1)
+#define VPORT_CONNECTED (2)
+#define VPORT_EMULATED (3)
+
+/* helpers for get port mapping information */
+#define VPORT_NUM(state) (state & 0xFF)
+#define VPORT_STATE(state) ((state >> 8) & 0xFF)
+#define VPORT_NUM_STATE(status, num) (((status & 0xFF) << 8) | (num & 0xFF))

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

xdev = hci_data;
- di = dev_data;

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

+ for (slot = 1; slot < XHCI_MAX_SLOTS; ++slot)
+ if (xdev->slots[slot] == edev)
+ break;
+
+ assert(slot < USB_NATIVE_NUM_BUS);
+
+ status = VPORT_STATE(xdev->port_map_tbl[di.bus][di.port]);
+ assert(status == VPORT_EMULATED || status == VPORT_CONNECTED);
+ xdev->port_map_tbl[di.bus][di.port] = VPORT_NUM_STATE(VPORT_ASSIGNED,
+ 0);
+
UPRINTF(LDBG, "report virtual port %d status\r\n", port);
if (pci_xhci_disconnect_port(xdev, port, 1)) {
UPRINTF(LFTL, "fail to report event\r\n");
@@ -1393,28 +1393,67 @@ done:
return err;
}

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

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

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

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

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

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

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

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

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




Re: ACRN Project Technical Community Meeting: @ Weekly 9PM-10PM (China-Shanghai), 6AM-7AM (US-West Coast), 3PM-4PM (Europe-London)

Wang, Hongbo
 

Attach the foils “ACPI Virtualization” for today’s meeting.
Best regards.
Hongbo
Tel: +86-21-6116 7445
MP: +86-1364 1793 689
 
 
-----Original Appointment-----

From: Wang, Hongbo
Sent: Monday, August 6, 2018 11:26 AM
To: Wang, Hongbo; Ren, Jack; VanCutsem, Geoffroy; Xie, Zhengtian; Goossens, Paul; Oehrlein, Scott; Teres Alexis, Alan Previn; Zou, Zachary; Sinnapan, Punitha; Aung, Diana; Selvaraju, Suman; Rai, Santoshkumar Laxminarayan; Kek, Kah Teik; Lam, Yat Seng; Ng, Wei Tee; Ooi, Cinly; Li, Susie; Dong, Eddie; Yao, Michael; Wang, Cherry; Osier-mixon, Jeffrey; Reddy, Raghuveer; Kinder, David B; Wu, James Y; 'acrn-dev@...'; OTC IAH ALL; Liu, WeiX W; Wang, Yu1; Zou, Terry; Wu, Xiangyang; Li, ZhijianX; 'Veeraiyan Chidambaram (RBEI/ECF3)'; Wang, Kai Z; Qi, Yadong; Zhu, Bing; Wu, John; Ma, FengjuanX; Chen, Luhai; Occhialino, Amy R; Perry, Karen A; Wang, Xufang; Zhang, Paul; mandal, Lax; Zhang, Grace H; Chang, Tommy; Lung, Bruce; Gao, Junhao; Choong, Yin Thong; 'info@...'; 'cc_houyong@...'; Ye, Chris; Wong, Gary Kueh Sing; Huang, Yang; Michael Dolan; Li, Jocelyn; Yao, Yi; OTC Linux Audio; Tang, FuweiX; Ingalsuo, Seppo; Shan, Junjun; De Jong, Karla; Arvind Murthy; iurri.konovalenko@...; iurii.konovalenko@...; Xu, Randy; Du, Alek; Marathe, Yogesh; Langlois, John; joel.markham@...; Diperna, Alex; Khandelwal, Sumit; Paidipala, Shravan KumarX; Radha Krishna Murthy, Bapatla VenkataX; Jekkareddy Lokesh, PushpaX; 'Srivastava, Harshit'; Lye, Mee Kim; Zhang, Qi1; Khor, Boon Pin
Cc: Liu, Shuo; 'Selvaraj Pandian, Ramesh'; Leung, Daniel; Owen, Craig T; Dinh, Kien T
Subject: ACRN Project Technical Community Meeting: @ Weekly 9PM-10PM (China-Shanghai), 6AM-7AM (US-West Coast), 3PM-4PM (Europe-London)
When: 2018年8月15日星期三 21:00-22:00 (UTC+08:00) Beijing, Chongqing, Hong Kong, Urumqi.
Where: Online - Zoom
 
 
Agenda (8/15): ACRN ACPI Management
 
WW Topic Presentator Status
WW21 ACRN roadmap introduction Ren, Jack Done
WW22 Patch submission process
ACRN feature list introduction
Wang, Hongbo
Ren, jack
Done
WW23 Memory Management Chen, Jascon Done
WW24 Boot flow and fast boot Wu, Binbin Done
WW25 Memory Management Chen, Jason C Done
WW26 Audio virtualization Li, Jocelyn Done
WW27 Trusty Security on ACRN Zhu, Bing’s team Done
WW28 Clear Linux and use on ACRN Du, Alek Done
WW29 GVT-g for ACRN (a.k.a AcrnGT) Gong, Zhipeng Done
WW30 Device pass-through Zhai, Edwin Done
WW31 ACRN logical partition Ren, Jack/Xu, Anthony Done
WW32 ACRN interrupt management Chen, Jason Done
WW33 ACRN ACPI virtualization Edwin Zhai Plan
WW34 ACRN P-state/C-state management Victor Sun Plan
WW35 ACRN Timer Li Fei Plan
WW36 CPU Virtualization Jason Chen Plan
WW37 ACRN S3/S5 management Fengwei Yin Plan
WW38 IPU Sharing Bandi, Kushal Plan
WW39 USB virtualization Yu Wang Plan
WW40 ACRN VT-d Binbin Wu Plan
WW41 ACRN GPIO virtualization Yu Wang Plan
 
 
 
<< OLE Object: Picture (Device Independent Bitmap) >> Project ACRN: A flexible, light-weight, open source reference hypervisor for IoT devices
 
We're still in the early stages of forming this TSC, so instead we invite you to attend a weekly "Technical Community" meeting where we'll meet community members and talk about the ACRN project and plans. As we explore community interest and involvement opportunities, we'll (re)schedule these meetings at a time convenient to most attendees:
  • Meets every Wednesday, Starting May 30, 2018: 9PM-10PM (China-Shanghai), 6AM-7AM (US-West Coast)
  • Chairperson: Hongbo Wang, hongbo.wang@... (Intel)
  • Online conference link: https://zoom.us/j/880368975
  • Zoom Meeting ID: 880-368-975
  • Online conference phone:
  • US: +1 669 900 6833  or +1 646 558 8656   or +1 877 369 0926 (Toll Free) or +1 855 880 1246 (Toll Free)
  • China: +86 010 87833177  or 400 669 9381 (Toll Free)
  • Germany: +49 (0) 30 3080 6188  or +49 800 724 3138 (Toll Free)
  • Additional international phone numbers
  • Meeting Notes:
 
 
Best regards.
Hongbo
Tel: +86-21-6116 7445
MP: +86-1364 1793 689
 
 
 
 


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

Yu Wang
 

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

On 18-08-15 09:32:58, Wu, Xiaoguang wrote:
This patch enable the support for SOS S3 from the perspective
of USB xHCI.

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

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

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

xdev = hci_data;

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

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

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

assert(hci_data);
assert(dev_data);
@@ -593,8 +600,19 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
xdev->port_map_tbl[di.bus][di.port] = VPORT_NUM_STATE(VPORT_ASSIGNED,
0);

+ /* TODO: should revisit this in deeper level */
+ if (vm_get_suspend_mode() != VM_SUSPEND_NONE) {
+ XHCI_PORTREG_PTR(xdev, port)->portsc &= ~(XHCI_PS_CSC |
+ XHCI_PS_CCS | XHCI_PS_PED | XHCI_PS_PP);
+ edev->dev_slotstate = XHCI_ST_DISABLED;
+ xdev->devices[port] = NULL;
+ xdev->slots[slot] = NULL;
+ pci_xhci_dev_destroy(edev);
+ need_intr = 0;
+ }
+
UPRINTF(LDBG, "report virtual port %d status\r\n", port);
- if (pci_xhci_disconnect_port(xdev, port, 1)) {
+ if (pci_xhci_disconnect_port(xdev, port, need_intr)) {
UPRINTF(LFTL, "fail to report event\r\n");
return -1;
}
--
2.7.4




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

Yu Wang
 

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

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

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

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

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

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

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

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

+/* port mapping status */
#define VPORT_FREE (0)
-#define VPORT_ASSIGNED (-1)
+#define VPORT_ASSIGNED (1)
+#define VPORT_CONNECTED (2)
+#define VPORT_EMULATED (3)
+
+/* helpers for get port mapping information */
+#define VPORT_NUM(state) (state & 0xFF)
+#define VPORT_STATE(state) ((state >> 8) & 0xFF)
+#define VPORT_NUM_STATE(status, num) (((status & 0xFF) << 8) | (num & 0xFF))

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

xdev = hci_data;
- di = dev_data;

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

+ for (slot = 1; slot < XHCI_MAX_SLOTS; ++slot)
+ if (xdev->slots[slot] == edev)
+ break;
+
+ assert(slot < USB_NATIVE_NUM_BUS);
+
+ status = VPORT_STATE(xdev->port_map_tbl[di.bus][di.port]);
+ assert(status == VPORT_EMULATED || status == VPORT_CONNECTED);
+ xdev->port_map_tbl[di.bus][di.port] = VPORT_NUM_STATE(VPORT_ASSIGNED,
+ 0);
+
UPRINTF(LDBG, "report virtual port %d status\r\n", port);
if (pci_xhci_disconnect_port(xdev, port, 1)) {
UPRINTF(LFTL, "fail to report event\r\n");
@@ -1393,28 +1393,67 @@ done:
return err;
}

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

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

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

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

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

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

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

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

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




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

Yu Wang
 

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

On 18-08-15 09:32:55, Wu, Xiaoguang wrote:
The variable native_assign_ports in struct pci_xhci_vdev is used
to record wether certain root hub port in SOS is assigned to UOS.
The logic uses zero to express 'not assigned' and nonzero to express
'assigned'. In this patch, use macro to replace number to express
better.

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

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index 14009a8..6ee9acb 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -384,6 +384,10 @@ struct pci_xhci_vdev {
#define XHCI_HALTED(xdev) ((xdev)->opregs.usbsts & XHCI_STS_HCH)
#define XHCI_GADDR(xdev, a) paddr_guest2host((xdev)->dev->vmctx, (a), \
XHCI_PADDR_SZ - ((a) & (XHCI_PADDR_SZ-1)))
+
+#define VPORT_FREE (0)
+#define VPORT_ASSIGNED (-1)
+
struct pci_xhci_option_elem {
char *parse_opt;
int (*parse_fn)(struct pci_xhci_vdev *, char *);
@@ -500,7 +504,7 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
UPRINTF(LDBG, "%04x:%04x %d-%d connecting.\r\n",
di->vid, di->pid, di->bus, di->port);

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

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




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

Wu, Xiaoguang
 

This patch enable the support for SOS S3 from the perspective
of USB xHCI.

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

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

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

xdev = hci_data;

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

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

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

assert(hci_data);
assert(dev_data);
@@ -593,8 +600,19 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
xdev->port_map_tbl[di.bus][di.port] = VPORT_NUM_STATE(VPORT_ASSIGNED,
0);

+ /* TODO: should revisit this in deeper level */
+ if (vm_get_suspend_mode() != VM_SUSPEND_NONE) {
+ XHCI_PORTREG_PTR(xdev, port)->portsc &= ~(XHCI_PS_CSC |
+ XHCI_PS_CCS | XHCI_PS_PED | XHCI_PS_PP);
+ edev->dev_slotstate = XHCI_ST_DISABLED;
+ xdev->devices[port] = NULL;
+ xdev->slots[slot] = NULL;
+ pci_xhci_dev_destroy(edev);
+ need_intr = 0;
+ }
+
UPRINTF(LDBG, "report virtual port %d status\r\n", port);
- if (pci_xhci_disconnect_port(xdev, port, 1)) {
+ if (pci_xhci_disconnect_port(xdev, port, need_intr)) {
UPRINTF(LFTL, "fail to report event\r\n");
return -1;
}
--
2.7.4