Date   

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

Yu Wang
 

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

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

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

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index 792d930..f18660c 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -440,10 +440,13 @@ static void pci_xhci_update_ep_ring(struct pci_xhci_vdev *xdev,
uint32_t streamid, uint64_t ringaddr,
int ccs);
static void pci_xhci_init_port(struct pci_xhci_vdev *xdev, int portn);
+static int pci_xhci_connect_port(struct pci_xhci_vdev *xdev, int port,
+ int usb_speed, int need_intr);
+static int pci_xhci_disconnect_port(struct pci_xhci_vdev *xdev, int port,
+ int need_intr);
static struct pci_xhci_dev_emu *pci_xhci_dev_create(struct pci_xhci_vdev *
xdev, void *dev_data);
static void pci_xhci_dev_destroy(struct pci_xhci_dev_emu *de);
-static int pci_xhci_port_chg(struct pci_xhci_vdev *xdev, int port, int conn);
static void pci_xhci_set_evtrb(struct xhci_trb *evtrb, uint64_t port,
uint32_t errcode, uint32_t evtype);
static int pci_xhci_xfer_complete(struct pci_xhci_vdev *xdev,
@@ -472,6 +475,7 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
void *ud;
uint8_t native_bus, native_pid, native_port;
uint16_t native_vid;
+ int native_speed;

xdev = hci_data;

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

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

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

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

return 0;
@@ -595,7 +598,7 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
}

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

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

assert(xdev != NULL);

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

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

+static int
+pci_xhci_connect_port(struct pci_xhci_vdev *xdev, int port, int usb_speed,
+ int intr)
+{
+ return pci_xhci_change_port(xdev, port, usb_speed, 1, intr);
+}
+
+static int
+pci_xhci_disconnect_port(struct pci_xhci_vdev *xdev, int port, int intr)
+{
+ /* for disconnect, the speed is useless */
+ return pci_xhci_change_port(xdev, port, 0, 0, intr);
+}
+
static void
pci_xhci_set_evtrb(struct xhci_trb *evtrb, uint64_t port, uint32_t errcode,
uint32_t evtype)
@@ -3208,30 +3222,8 @@ pci_xhci_reset_port(struct pci_xhci_vdev *xdev, int portn, int warm)
static void
pci_xhci_init_port(struct pci_xhci_vdev *xdev, int portn)
{
- struct pci_xhci_portregs *port;
- struct pci_xhci_dev_emu *dev;
-
- port = XHCI_PORTREG_PTR(xdev, portn);
- dev = XHCI_DEVINST_PTR(xdev, portn);
- if (dev) {
- port->portsc = XHCI_PS_CCS | /* connected */
- XHCI_PS_PP; /* port power */
-
- if (dev->dev_ue->ue_usbver == 2) {
- port->portsc |= XHCI_PS_PLS_SET(UPS_PORT_LS_POLL) |
- XHCI_PS_SPEED_SET(dev->dev_ue->ue_usbspeed);
- } else {
- port->portsc |= XHCI_PS_PLS_SET(UPS_PORT_LS_U0) |
- XHCI_PS_PED | /* enabled */
- XHCI_PS_SPEED_SET(dev->dev_ue->ue_usbspeed);
- }
-
- UPRINTF(LDBG, "Init port %d 0x%x\n", portn, port->portsc);
- } else {
- port->portsc = XHCI_PS_PLS_SET(UPS_PORT_LS_RX_DET) | XHCI_PS_PP;
- UPRINTF(LDBG, "Init empty port %d 0x%x\n",
- portn, port->portsc);
- }
+ XHCI_PORTREG_PTR(xdev, portn)->portsc =
+ XHCI_PS_PLS_SET(UPS_PORT_LS_RX_DET) | XHCI_PS_PP;
}

static int
--
2.7.4




[PATCH v2 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 | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)

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

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

xdev = hci_data;

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

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

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

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

+ if (vm_get_suspend_mode() != VM_SUSPEND_NONE) {
+ XHCI_PORTREG_PTR(xdev, port)->portsc &= ~(XHCI_PS_CSC |
+ XHCI_PS_CCS | XHCI_PS_PED | XHCI_PS_PP);
+ edev->dev_slotstate = XHCI_ST_DISABLED;
+ xdev->devices[port] = NULL;
+ xdev->slots[slot] = NULL;
+ pci_xhci_dev_destroy(edev);
+ need_intr = 0;
+ }
+
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


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

Wu, Xiaoguang
 

The xHCI emulation greatly depends on the user space library libusb
which is based on the usbfs module in Linux kernel. The libusb will
bind usbfs to physical USB device which makes hardware control over
libusb in user space possible.

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

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

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

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

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

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

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

+/* port mapping status */
#define PT_NOT_ASSIGNED (0)
-#define PT_ASSIGNED_WITH_NO_DEV (-1)
+#define PT_ASSIGNED_WITH_NO_DEV (1)
+#define PT_ASSIGNED_WITH_NEW_DEV (2)
+#define PT_ASSIGNED_WITH_EMU_DEV (3)
+
+/* helpers for get port mapping information */
+#define PT_GET_NUMBER(state) (state & 0xFF)
+#define PT_GET_STATUS(state) ((state >> 8) & 0xFF)
+#define PT_MAKE_STATE(status, num) (((status & 0xFF) << 8) | (num & 0xFF))

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

xdev = hci_data;
- di = dev_data;

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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


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

Wu, Xiaoguang
 

Replace 'native_assign_ports' with 'port_map_tbl' to be more accurate
for the role of this variable plays.

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

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

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

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

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


[PATCH v2 4/8] DM USB: xHCI: limit bus and port numbers of xHCI

Wu, Xiaoguang
 

Currently the maximum number of bus and port for xHCI are
both set to 255, it is theoretically possible but in fact
not neccessary. This patch changes those two values to be
more proper: 4 buses and 20 ports.

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

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

@@ -500,8 +500,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] ||
- !xdev->native_assign_ports[di->bus][di->port]) {
+ if (!xdev->native_assign_ports[di->bus][di->port]) {
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;
@@ -3361,15 +3360,6 @@ pci_xhci_parse_bus_port(struct pci_xhci_vdev *xdev, char *opts)
goto errout;
}

- if (!xdev->native_assign_ports[bus]) {
- xdev->native_assign_ports[bus] = calloc(USB_NATIVE_NUM_PORT,
- sizeof(uint8_t));
- if (!xdev->native_assign_ports[bus]) {
- rc = -3;
- goto errout;
- }
- }
-
xdev->native_assign_ports[bus][port] = 1;
errout:
if (rc)
diff --git a/devicemodel/include/usb_core.h b/devicemodel/include/usb_core.h
index 8727b8b..add4cb4 100644
--- a/devicemodel/include/usb_core.h
+++ b/devicemodel/include/usb_core.h
@@ -223,8 +223,8 @@ enum USB_ERRCODE {
#define NATIVE_USBSYS_DEVDIR "/sys/bus/usb/devices"
#define NATIVE_USB2_SPEED "480"
#define NATIVE_USB3_SPEED "5000"
-#define USB_NATIVE_NUM_PORT 255
-#define USB_NATIVE_NUM_BUS 255
+#define USB_NATIVE_NUM_PORT 20
+#define USB_NATIVE_NUM_BUS 4

extern int usb_log_level;
inline int usb_get_log_level(void) { return usb_log_level; }
--
2.7.4


[PATCH v2 3/8] DM USB: introduce struct usb_native_devinfo

Wu, Xiaoguang
 

Current design cannot get physical USB device information without
the creation of pci_xhci_dev_emu. This brings some difficulties in
certain situations, hence struct usb_native_devinfo is introduced
to describe neccessary information to solve this trouble.

Signed-off-by: Xiaoguang Wu <xiaoguang.wu@...>
Reviewed-by: Liang Yang <liang3.yang@...>
---
devicemodel/hw/pci/xhci.c | 33 +++++-------
devicemodel/hw/platform/usb_pmapper.c | 95 ++++++++++++++++++-----------------
devicemodel/include/usb_core.h | 10 ++++
devicemodel/include/usb_pmapper.h | 9 +---
4 files changed, 73 insertions(+), 74 deletions(-)

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index f18660c..b50d435 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -469,22 +469,21 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
struct pci_xhci_dev_emu *de;
struct pci_xhci_vdev *xdev;
struct usb_devemu *ue;
+ struct usb_native_devinfo *di;
int port_start, port_end;
int slot_start, slot_end;
int port, slot;
void *ud;
- uint8_t native_bus, native_pid, native_port;
- uint16_t native_vid;
- int native_speed;

xdev = hci_data;
+ di = dev_data;

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

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

/* print physical information about new device */
- ue->ue_info(ud, USB_INFO_BUS, &native_bus, sizeof(native_bus));
- ue->ue_info(ud, USB_INFO_PORT, &native_port, sizeof(native_port));
- ue->ue_info(ud, USB_INFO_VID, &native_vid, sizeof(native_vid));
- ue->ue_info(ud, USB_INFO_PID, &native_pid, sizeof(native_pid));
- ue->ue_info(ud, USB_INFO_SPEED, &native_speed, sizeof(native_speed));
UPRINTF(LDBG, "%04x:%04x %d-%d connecting.\r\n",
- native_vid, native_pid, native_bus, native_port);
+ di->vid, di->pid, di->bus, di->port);

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

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

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

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

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

return 0;
@@ -588,7 +581,7 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
continue;

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

diff --git a/devicemodel/hw/platform/usb_pmapper.c b/devicemodel/hw/platform/usb_pmapper.c
index 9f57f1c..2aedf28 100644
--- a/devicemodel/hw/platform/usb_pmapper.c
+++ b/devicemodel/hw/platform/usb_pmapper.c
@@ -383,7 +383,7 @@ usb_dev_update_ep(struct usb_dev *udev)
int i, j;

assert(udev);
- if (libusb_get_active_config_descriptor(udev->ldev, &cfg))
+ if (libusb_get_active_config_descriptor(udev->info.priv_data, &cfg))
return;

for (i = 0; i < cfg->bNumInterfaces; i++) {
@@ -409,13 +409,12 @@ usb_dev_native_toggle_if(struct usb_dev *udev, int claim)

assert(udev);
assert(udev->handle);
- assert(udev->ldev);
assert(claim == 1 || claim == 0);

- b = udev->bus;
- p = udev->port;
+ b = udev->info.bus;
+ p = udev->info.port;

- r = libusb_get_active_config_descriptor(udev->ldev, &config);
+ r = libusb_get_active_config_descriptor(udev->info.priv_data, &config);
if (r) {
UPRINTF(LWRN, "%d-%d: can't get config\r\n", b, p);
return -1;
@@ -458,13 +457,12 @@ usb_dev_native_toggle_if_drivers(struct usb_dev *udev, int attach)

assert(udev);
assert(udev->handle);
- assert(udev->ldev);
assert(attach == 1 || attach == 0);

- b = udev->bus;
- p = udev->port;
+ b = udev->info.bus;
+ p = udev->info.port;

- r = libusb_get_active_config_descriptor(udev->ldev, &config);
+ r = libusb_get_active_config_descriptor(udev->info.priv_data, &config);
if (r) {
UPRINTF(LWRN, "%d-%d: can't get config\r\n", b, p);
return -1;
@@ -503,7 +501,6 @@ usb_dev_set_config(struct usb_dev *udev, struct usb_data_xfer *xfer, int config)
struct libusb_config_descriptor *cfg;

assert(udev);
- assert(udev->ldev);
assert(udev->handle);

/*
@@ -521,7 +518,7 @@ usb_dev_set_config(struct usb_dev *udev, struct usb_data_xfer *xfer, int config)
}

/* claim all the interfaces of this configuration */
- rc = libusb_get_active_config_descriptor(udev->ldev, &cfg);
+ rc = libusb_get_active_config_descriptor(udev->info.priv_data, &cfg);
if (rc) {
UPRINTF(LWRN, "fail to get config rc %d\r\n", rc);
goto err2;
@@ -545,7 +542,8 @@ err1:
usb_dev_native_toggle_if(udev, 0);
libusb_free_config_descriptor(cfg);
err2:
- UPRINTF(LWRN, "%d-%d: fail to set config\r\n", udev->bus, udev->port);
+ UPRINTF(LWRN, "%d-%d: fail to set config\r\n", udev->info.bus,
+ udev->info.port);
xfer->status = USB_ERR_STALLED;
}

@@ -560,8 +558,8 @@ usb_dev_set_if(struct usb_dev *udev, int iface, int alt, struct usb_data_xfer
if (iface >= USB_NUM_INTERFACE)
goto errout;

- UPRINTF(LDBG, "%d-%d set if, iface %d alt %d\r\n", udev->bus,
- udev->port, iface, alt);
+ UPRINTF(LDBG, "%d-%d set if, iface %d alt %d\r\n", udev->info.bus,
+ udev->info.port, iface, alt);

if (libusb_set_interface_alt_setting(udev->handle, iface, alt))
goto errout;
@@ -578,7 +576,7 @@ usb_dev_set_if(struct usb_dev *udev, int iface, int alt, struct usb_data_xfer
errout:
xfer->status = USB_ERR_STALLED;
UPRINTF(LDBG, "%d-%d fail to set if, iface %d alt %d\r\n",
- udev->bus, udev->port, iface, alt);
+ udev->info.bus, udev->info.port, iface, alt);
}

static struct usb_data_xfer_block *
@@ -773,7 +771,7 @@ usb_dev_request(void *pdata, struct usb_data_xfer *xfer)
assert(udev);

xfer->status = USB_ERR_NORMAL_COMPLETION;
- if (!udev->ldev || !xfer->ureq) {
+ if (!udev->info.priv_data || !xfer->ureq) {
UPRINTF(LWRN, "invalid request\r\n");
xfer->status = USB_ERR_IOERROR;
goto out;
@@ -861,22 +859,18 @@ void *
usb_dev_init(void *pdata, char *opt)
{
struct usb_dev *udev = NULL;
- struct libusb_device *ldev;
struct libusb_device_descriptor desc;
- uint8_t bus, port;
- int speed, ver;
+ struct usb_native_devinfo *di;
+ int ver;

assert(pdata);

- ldev = pdata;
- speed = libusb_get_device_speed(ldev);
- port = libusb_get_port_number(ldev);
- bus = libusb_get_bus_number(ldev);
- libusb_get_device_descriptor(ldev, &desc);
+ di = pdata;
+ libusb_get_device_descriptor(di->priv_data, &desc);
UPRINTF(LINF, "Found USB device: %d-%d\r\nPID(0x%X), VID(0x%X) CLASS"
- "(0x%X) SUBCLASS(0x%X) BCD(0x%X) SPEED(%d)\r\n", bus,
- port, desc.idProduct, desc.idVendor, desc.bDeviceClass,
- desc.bDeviceSubClass, desc.bcdUSB, speed);
+ "(0x%X) SUBCLASS(0x%X) BCD(0x%X) SPEED(%d)\r\n",
+ di->bus, di->port, di->pid, di->vid, desc.bDeviceClass,
+ desc.bDeviceSubClass, di->bcd, di->speed);

/* allocate and populate udev */
udev = calloc(1, sizeof(struct usb_dev));
@@ -884,7 +878,7 @@ usb_dev_init(void *pdata, char *opt)
goto errout;

/* this is a root hub */
- if (port == 0)
+ if (di->port == 0)
goto errout;

switch (desc.bcdUSB) {
@@ -908,17 +902,12 @@ usb_dev_init(void *pdata, char *opt)
goto errout;
}

- udev->speed = speed;
- udev->ldev = ldev;
+ udev->info = *di;
udev->version = ver;
udev->handle = NULL;
- udev->port = port;
- udev->bus = bus;
- udev->pid = desc.idProduct;
- udev->vid = desc.idVendor;

/* configure physical device through libusb library */
- if (libusb_open(udev->ldev, &udev->handle)) {
+ if (libusb_open(udev->info.priv_data, &udev->handle)) {
UPRINTF(LWRN, "fail to open device.\r\n");
goto errout;
}
@@ -973,25 +962,25 @@ usb_dev_info(void *pdata, int type, void *value, int size)
pv = &udev->version;
break;
case USB_INFO_SPEED:
- sz = sizeof(udev->speed);
- udev->speed = libusb_speed_to_usb_speed(udev->speed);
- pv = &udev->speed;
+ sz = sizeof(udev->info.speed);
+ udev->info.speed = libusb_speed_to_usb_speed(udev->info.speed);
+ pv = &udev->info.speed;
break;
case USB_INFO_BUS:
- sz = sizeof(udev->bus);
- pv = &udev->bus;
+ sz = sizeof(udev->info.bus);
+ pv = &udev->info.bus;
break;
case USB_INFO_PORT:
- sz = sizeof(udev->port);
- pv = &udev->port;
+ sz = sizeof(udev->info.port);
+ pv = &udev->info.port;
break;
case USB_INFO_VID:
- sz = sizeof(udev->vid);
- pv = &udev->vid;
+ sz = sizeof(udev->info.vid);
+ pv = &udev->info.vid;
break;
case USB_INFO_PID:
- sz = sizeof(udev->pid);
- pv = &udev->pid;
+ sz = sizeof(udev->info.pid);
+ pv = &udev->info.pid;
break;
default:
return -1;
@@ -1020,6 +1009,9 @@ static int
usb_dev_native_sys_conn_cb(struct libusb_context *ctx, struct libusb_device
*ldev, libusb_hotplug_event event, void *pdata)
{
+ struct libusb_device_descriptor d;
+ struct usb_native_devinfo di;
+
UPRINTF(LDBG, "connect event\r\n");

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

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

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

+struct usb_native_devinfo {
+ int speed;
+ uint8_t bus;
+ uint8_t port;
+ uint16_t bcd;
+ uint16_t pid;
+ uint16_t vid;
+ void *priv_data;
+};
+
enum USB_ERRCODE {
USB_ACK,
USB_NAK,
diff --git a/devicemodel/include/usb_pmapper.h b/devicemodel/include/usb_pmapper.h
index 144c68f..43cb09f 100644
--- a/devicemodel/include/usb_pmapper.h
+++ b/devicemodel/include/usb_pmapper.h
@@ -36,14 +36,10 @@ struct usb_dev_ep {

struct usb_dev {
/* physical device info */
+ struct usb_native_devinfo info;
int addr;
int version;
- int speed;
int configuration;
- uint8_t port;
- uint8_t bus;
- uint8_t pid;
- uint16_t vid;

/* interface info */
int if_num;
@@ -55,8 +51,7 @@ struct usb_dev {
struct usb_dev_ep epo[USB_NUM_ENDPOINT];

/* libusb data */
- libusb_device *ldev;
- libusb_device_handle *handle;
+ libusb_device_handle *handle;
};

/*
--
2.7.4


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

Wu, Xiaoguang
 

The variable native_assign_ports in struct pci_xhci_vdev is used
to record wether certain root hub port in SOS is assigned to UOS.
The logic uses zero to express 'not assigned' and nonzero to express
'assigned'. In this patch, use macro to replace number to express
better.

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

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

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

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


[PATCH v2 2/8] DM USB: xHCI: refine xHCI PORTSC Register related functions

Wu, Xiaoguang
 

PORTSC (Port Status and Control Register) register play a very
important role in USB sub-system. This patch is used to refine
related manipulation functions.

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

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index 792d930..f18660c 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -440,10 +440,13 @@ static void pci_xhci_update_ep_ring(struct pci_xhci_vdev *xdev,
uint32_t streamid, uint64_t ringaddr,
int ccs);
static void pci_xhci_init_port(struct pci_xhci_vdev *xdev, int portn);
+static int pci_xhci_connect_port(struct pci_xhci_vdev *xdev, int port,
+ int usb_speed, int need_intr);
+static int pci_xhci_disconnect_port(struct pci_xhci_vdev *xdev, int port,
+ int need_intr);
static struct pci_xhci_dev_emu *pci_xhci_dev_create(struct pci_xhci_vdev *
xdev, void *dev_data);
static void pci_xhci_dev_destroy(struct pci_xhci_dev_emu *de);
-static int pci_xhci_port_chg(struct pci_xhci_vdev *xdev, int port, int conn);
static void pci_xhci_set_evtrb(struct xhci_trb *evtrb, uint64_t port,
uint32_t errcode, uint32_t evtype);
static int pci_xhci_xfer_complete(struct pci_xhci_vdev *xdev,
@@ -472,6 +475,7 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
void *ud;
uint8_t native_bus, native_pid, native_port;
uint16_t native_vid;
+ int native_speed;

xdev = hci_data;

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

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

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

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

return 0;
@@ -595,7 +598,7 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
}

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

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

assert(xdev != NULL);

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

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

+static int
+pci_xhci_connect_port(struct pci_xhci_vdev *xdev, int port, int usb_speed,
+ int intr)
+{
+ return pci_xhci_change_port(xdev, port, usb_speed, 1, intr);
+}
+
+static int
+pci_xhci_disconnect_port(struct pci_xhci_vdev *xdev, int port, int intr)
+{
+ /* for disconnect, the speed is useless */
+ return pci_xhci_change_port(xdev, port, 0, 0, intr);
+}
+
static void
pci_xhci_set_evtrb(struct xhci_trb *evtrb, uint64_t port, uint32_t errcode,
uint32_t evtype)
@@ -3208,30 +3222,8 @@ pci_xhci_reset_port(struct pci_xhci_vdev *xdev, int portn, int warm)
static void
pci_xhci_init_port(struct pci_xhci_vdev *xdev, int portn)
{
- struct pci_xhci_portregs *port;
- struct pci_xhci_dev_emu *dev;
-
- port = XHCI_PORTREG_PTR(xdev, portn);
- dev = XHCI_DEVINST_PTR(xdev, portn);
- if (dev) {
- port->portsc = XHCI_PS_CCS | /* connected */
- XHCI_PS_PP; /* port power */
-
- if (dev->dev_ue->ue_usbver == 2) {
- port->portsc |= XHCI_PS_PLS_SET(UPS_PORT_LS_POLL) |
- XHCI_PS_SPEED_SET(dev->dev_ue->ue_usbspeed);
- } else {
- port->portsc |= XHCI_PS_PLS_SET(UPS_PORT_LS_U0) |
- XHCI_PS_PED | /* enabled */
- XHCI_PS_SPEED_SET(dev->dev_ue->ue_usbspeed);
- }
-
- UPRINTF(LDBG, "Init port %d 0x%x\n", portn, port->portsc);
- } else {
- port->portsc = XHCI_PS_PLS_SET(UPS_PORT_LS_RX_DET) | XHCI_PS_PP;
- UPRINTF(LDBG, "Init empty port %d 0x%x\n",
- portn, port->portsc);
- }
+ XHCI_PORTREG_PTR(xdev, portn)->portsc =
+ XHCI_PS_PLS_SET(UPS_PORT_LS_RX_DET) | XHCI_PS_PP;
}

static int
--
2.7.4


[PATCH v2 1/8] DM USB: xHCI: fix an xHCI issue to enable UOS s3 feature

Wu, Xiaoguang
 

Current DM design use two variables to do the indexing of xHCI
Event Ring: er_enq_idx and er_events_cnt. They are members of
the struct pci_xhci_rtsregs.

In UOS, during the process of xHCI resuming, the xHCI driver
will restore the ERSTBA (Event Ring Segment Table Base Address)
register to be the value before suspending. And at this point,
the old DM implementation will set both er_enq_idx and
er_events_cnt to be zero, so the DM will access the Event Ring
from the start position in the buffer. But at the same time the
UOS xHCI driver still wants to access the old position in the
Event Ring before suspending, which will result of unexpected
errors.

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

diff --git a/devicemodel/hw/pci/xhci.c b/devicemodel/hw/pci/xhci.c
index da0c42a..792d930 100644
--- a/devicemodel/hw/pci/xhci.c
+++ b/devicemodel/hw/pci/xhci.c
@@ -2732,9 +2732,6 @@ pci_xhci_rtsregs_write(struct pci_xhci_vdev *xdev,
rts->erst_p = XHCI_GADDR(xdev,
xdev->rtsregs.erstba_p->qwEvrsTablePtr & ~0x3FUL);

- rts->er_enq_idx = 0;
- rts->er_events_cnt = 0;
-
UPRINTF(LDBG, "wr erstba erst (%p) ptr 0x%lx, sz %u\r\n",
rts->erstba_p,
rts->erstba_p->qwEvrsTablePtr,
--
2.7.4


[PATCH v2 0/8] USB xHCI S3 support

Wu, Xiaoguang
 

This patchset enable the USB xHCI S3 feature.

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

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

--
2.7.4


[PATCH 8/8] hv: add gva check for the case gva is from instruction decode

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

For the instructions other than MOVS and STO, the gva comes
from instruction decoding. Apply the gva check for this type
of gva and inject exception to guest accordingly during
instruction decoding phase.

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

diff --git a/hypervisor/arch/x86/guest/instr_emul.c b/hypervisor/arch/x86/guest/instr_emul.c
index 49a25ded..f1f23ffa 100644
--- a/hypervisor/arch/x86/guest/instr_emul.c
+++ b/hypervisor/arch/x86/guest/instr_emul.c
@@ -2173,6 +2173,91 @@ static int instr_check_di_si(struct vcpu *vcpu,
}
}

+static int instr_check_gva(struct vcpu *vcpu, struct instr_emul_ctxt *emul_ctxt,
+ enum vm_cpu_mode cpu_mode)
+{
+ int ret = 0;
+ uint64_t base, segbase, idx, gva, gpa;
+ uint32_t err_code;
+ enum cpu_reg_name seg;
+ struct instr_emul_vie *vie = &emul_ctxt->vie;
+
+ base = 0UL;
+ if (vie->base_register != CPU_REG_LAST) {
+ base = vm_get_register(vcpu, vie->base_register);
+
+ /* RIP relative addressing starts from the
+ * following instruction
+ */
+ if (vie->base_register == CPU_REG_RIP)
+ base += vie->num_processed;
+
+ }
+
+ idx = 0UL;
+ if (vie->index_register != CPU_REG_LAST) {
+ idx = vm_get_register(vcpu, vie->index_register);
+ }
+
+ /* "Specifying a Segment Selector" of SDM Vol1 3.7.4
+ *
+ * In legacy IA-32 mode, when ESP or EBP register is used as
+ * base, the SS segment is default segment.
+ *
+ * All data references, except when relative to stack or
+ * string destination, DS is default segment.
+ *
+ * segment override could overwrite the default segment
+ *
+ * 64bit mode, segmentation is generally disabled. The
+ * exception are FS and GS.
+ */
+ if (vie->seg_override != 0U) {
+ seg = vie->segment_register;
+ } else if ((vie->base_register == CPU_REG_RSP) ||
+ (vie->base_register == CPU_REG_RBP)) {
+ seg = CPU_REG_SS;
+ } else {
+ seg = CPU_REG_DS;
+ }
+
+ if ((cpu_mode == CPU_MODE_64BIT) && (seg != CPU_REG_FS) &&
+ (seg != CPU_REG_GS)) {
+ segbase = 0UL;
+ } else {
+ struct seg_desc desc;
+
+ vm_get_seg_desc(seg, &desc);
+
+ segbase = desc.base;
+ }
+
+ gva = segbase + base + vie->scale * idx + vie->displacement;
+
+ if (vie_canonical_check(cpu_mode, gva) != 0) {
+ if (seg == CPU_REG_SS) {
+ vcpu_inject_ss(vcpu);
+ } else {
+ vcpu_inject_gp(vcpu, 0U);
+ }
+ return -EFAULT;
+ }
+
+ err_code = (vcpu->req.reqs.mmio.direction == REQUEST_WRITE) ?
+ PAGE_FAULT_WR_FLAG : 0U;
+
+ ret = gva2gpa(vcpu, gva, &gpa, &err_code);
+ if (ret < 0) {
+ if (ret == -EFAULT) {
+ vcpu_inject_pf(vcpu, gva,
+ err_code);
+ }
+ return ret;
+ }
+
+ return 0;
+}
+
int decode_instruction(struct vcpu *vcpu)
{
struct instr_emul_ctxt *emul_ctxt;
@@ -2219,6 +2304,8 @@ int decode_instruction(struct vcpu *vcpu)
retval = instr_check_di_si(vcpu, emul_ctxt);
if (retval < 0)
return retval;
+ } else {
+ instr_check_gva(vcpu, emul_ctxt, cpu_mode);
}

return emul_ctxt->vie.opsize;
--
2.17.0


[PATCH 7/8] hv: add failure case check for MOVS/STO

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

MOVS and STO instruction get the operand line address from DI
and SI. This patch adds line address check after instruction
decoding is done. Inject exception accordingly if check fails.

We add check during instruction decoding phase. So we could
assume the following instruction emulation will always success.

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

diff --git a/hypervisor/arch/x86/guest/instr_emul.c b/hypervisor/arch/x86/guest/instr_emul.c
index 2012945c..49a25ded 100644
--- a/hypervisor/arch/x86/guest/instr_emul.c
+++ b/hypervisor/arch/x86/guest/instr_emul.c
@@ -55,7 +55,7 @@
#define VIE_OP_F_IMM8 (1U << 1) /* 8-bit immediate operand */
#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_GVA_IN_DI_SI (1U << 4) /* GVA from (R|E)SI/DI */
#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] = {
@@ -121,19 +121,19 @@ static const struct instr_emul_vie_op one_byte_opcodes[256] = {
},
[0xA4] = {
.op_type = VIE_OP_TYPE_MOVS,
- .op_flags = VIE_OP_F_NO_MODRM | VIE_OP_F_NO_GLA_VERIFICATION
+ .op_flags = VIE_OP_F_NO_MODRM | VIE_OP_F_GVA_IN_DI_SI
},
[0xA5] = {
.op_type = VIE_OP_TYPE_MOVS,
- .op_flags = VIE_OP_F_NO_MODRM | VIE_OP_F_NO_GLA_VERIFICATION
+ .op_flags = VIE_OP_F_NO_MODRM | VIE_OP_F_GVA_IN_DI_SI
},
[0xAA] = {
.op_type = VIE_OP_TYPE_STOS,
- .op_flags = VIE_OP_F_NO_MODRM | VIE_OP_F_NO_GLA_VERIFICATION
+ .op_flags = VIE_OP_F_NO_MODRM | VIE_OP_F_GVA_IN_DI_SI
},
[0xAB] = {
.op_type = VIE_OP_TYPE_STOS,
- .op_flags = VIE_OP_F_NO_MODRM | VIE_OP_F_NO_GLA_VERIFICATION
+ .op_flags = VIE_OP_F_NO_MODRM | VIE_OP_F_GVA_IN_DI_SI
},
[0xC6] = {
/* XXX Group 11 extended opcode - not just MOV */
@@ -969,7 +969,7 @@ static int emulate_movs(struct vcpu *vcpu, struct instr_emul_vie *vie)
{
uint64_t dstaddr, srcaddr;
uint64_t rcx, rdi, rsi, rflags;
- int error, fault, repeat;
+ int error, repeat;
uint8_t opsize;
enum cpu_reg_name seg;

@@ -1549,7 +1549,6 @@ static int emulate_bittest(struct vcpu *vcpu, struct instr_emul_vie *vie)

static int vmm_emulate_instruction(struct instr_emul_ctxt *ctxt)
{
- struct vm_guest_paging *paging = &ctxt->paging;
struct instr_emul_vie *vie = &ctxt->vie;
struct vcpu *vcpu = ctxt->vcpu;
int error;
@@ -2146,6 +2145,34 @@ static int local_decode_instruction(enum vm_cpu_mode cpu_mode,
return 0;
}

+/* for instruction MOVS/STO, check the gva gotten from DI/SI. */
+static int instr_check_di_si(struct vcpu *vcpu,
+ struct instr_emul_ctxt *emul_ctxt)
+{
+ int ret;
+ struct instr_emul_vie *vie = &emul_ctxt->vie;
+ uint64_t gva;
+ enum cpu_reg_name seg;
+
+ ret = get_gva_di_si_check(vcpu, vie->addrsize, PROT_WRITE,
+ CPU_REG_ES, CPU_REG_RDI, &gva);
+
+ if (ret < 0) {
+ return -EFAULT;
+ }
+
+ if (vie->op.op_type == VIE_OP_TYPE_MOVS) {
+ seg = (vie->seg_override != 0U) ?
+ (vie->segment_register) : CPU_REG_DS;
+ ret = get_gva_di_si_check(vcpu, vie->addrsize, PROT_READ,
+ seg, CPU_REG_RSI, &gva);
+
+ if (ret < 0) {
+ return -EFAULT;
+ }
+ }
+}
+
int decode_instruction(struct vcpu *vcpu)
{
struct instr_emul_ctxt *emul_ctxt;
@@ -2183,6 +2210,17 @@ int decode_instruction(struct vcpu *vcpu)
return -EFAULT;
}

+ /*
+ * We do operand check in instruction decode phase and
+ * inject exception accordingly. In late instruction
+ * emulation, it will always sucess.
+ */
+ if (emul_ctxt->vie.op.op_flags & VIE_OP_F_GVA_IN_DI_SI) {
+ retval = instr_check_di_si(vcpu, emul_ctxt);
+ if (retval < 0)
+ return retval;
+ }
+
return emul_ctxt->vie.opsize;
}

--
2.17.0


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

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

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

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

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

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

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

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

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

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

- *fault = 0;
return 0;

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

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

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

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

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

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


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

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

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

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

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

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

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

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

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


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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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


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

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

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


[PATCH 3/8] hv: fix use without initialization build error

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

Fix two use without initialization build error in instr_emul.c

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

diff --git a/hypervisor/arch/x86/guest/instr_emul.c b/hypervisor/arch/x86/guest/instr_emul.c
index d62c6eaa..ab9340e1 100644
--- a/hypervisor/arch/x86/guest/instr_emul.c
+++ b/hypervisor/arch/x86/guest/instr_emul.c
@@ -326,7 +326,7 @@ static uint32_t get_vmcs_field(enum cpu_reg_name ident)
*/
static uint64_t vm_get_register(struct vcpu *vcpu, enum cpu_reg_name reg)
{
- uint64_t reg_val;
+ uint64_t reg_val = 0;

if ((reg >= CPU_REG_GENERAL_FIRST) && (reg <= CPU_REG_GENERAL_LAST)) {
reg_val = vcpu_get_gpreg(vcpu, reg);
@@ -379,7 +379,7 @@ static void vm_set_register(struct vcpu *vcpu, enum cpu_reg_name reg,
*/
static void vm_get_seg_desc(enum cpu_reg_name seg, struct seg_desc *desc)
{
- struct seg_desc tdesc;
+ struct seg_desc tdesc = {0UL, 0U, 0U};

/* tdesc->access != 0xffffffffU in this function */
encode_vmcs_seg_desc(seg, &tdesc);
--
2.17.0


[PATCH 2/8] hv: extend the decode_modrm

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

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

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

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

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

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

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

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

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

--
2.17.0


[PATCH 0/8] instruction decoding refine

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

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

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

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

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

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

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

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

--
2.17.0


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

Mingqiang Chi
 

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

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

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

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

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

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

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

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

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

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

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

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

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

rte = vioapic->rtbl[pin];

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

#endif /* _VPIC_H_ */
--
2.7.4