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@...>
Acked-by: Yu Wang <yu1.wang@...>
---
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

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

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

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@...>
Acked-by: Yu Wang <yu1.wang@...>
---
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 6ee9acb..0ee841b 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] == VPORT_FREE) {
+ if (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;
@@ -3364,7 +3364,7 @@ pci_xhci_parse_bus_port(struct pci_xhci_vdev *xdev, char *opts)
goto errout;
}

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

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@...>
Acked-by: Yu Wang <yu1.wang@...>
---
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

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

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@...>
Acked-by: Yu Wang <yu1.wang@...>
---
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

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 | 306 ++++++++++++++++++++--------------
devicemodel/hw/platform/usb_pmapper.c | 95 +++++------
devicemodel/include/usb_core.h | 14 +-
devicemodel/include/usb_pmapper.h | 9 +-
4 files changed, 243 insertions(+), 181 deletions(-)

--
2.7.4

On 18-08-15 09:02:36, Wu, Xiaoguang wrote:

On 18-08-15 06:20:51, Yu Wang wrote:

On 18-08-15 02:46:58, Wu, Xiaoguang wrote:

On 18-08-14 23:15:19, Yu Wang wrote:

On 18-08-14 21:39:13, Wu, Xiaoguang wrote:

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

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

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

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

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

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

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

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

+/* port mapping status */
#define PT_NOT_ASSIGNED (0)
-#define PT_ASSIGNED_WITH_NO_DEV (-1)
+#define PT_ASSIGNED_WITH_NO_DEV (1)
+#define PT_ASSIGNED_WITH_NEW_DEV (2)
+#define PT_ASSIGNED_WITH_EMU_DEV (3)

Just like the previous comment. These names are not good. Please change
to:
VPORT_FREE
VPORT_ASSIGNED
VPORT_CONNECTED
VPORT_EMULATED

xgwu:

ok, will change. thanks.

+
+/* helpers for get port mapping information */
+#define PT_GET_NUMBER(state) (state & 0xFF)
+#define PT_GET_STATUS(state) ((state >> 8) & 0xFF)
+#define PT_MAKE_STATE(status, num) (((status & 0xFF) << 8) | (num & 0xFF))

Don't use PT. The PT in hypervisor is means passthrough... It is easy
confused..

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

xgwu:

will change to:

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

this means STATE = STATUS + NUMBER

Please don't mix the state and status, they are almost same mean. It
easy confused. Please consider other solution.

xgwu:

ok, will change to:

VPORT_NUM
VPORT_STATE
VPORT_NUM_STATE

That is good.

thanks.

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

xdev = hci_data;
- di = dev_data;

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

+ for (slot = 1; slot < XHCI_MAX_SLOTS; ++slot)
+ if (xdev->slots[slot] == edev)
+ break;
+
+ assert(slot < USB_NATIVE_NUM_BUS);
+
+ status = PT_GET_STATUS(xdev->port_map_tbl[di.bus][di.port]);
+ assert(status == PT_ASSIGNED_WITH_EMU_DEV ||
+ status == PT_ASSIGNED_WITH_NEW_DEV);

Let's unify all status to state.. Don't mix them..

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

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

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

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

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

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

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

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

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

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

On 08/15/2018 08:53 AM, Xu, Anthony wrote:

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

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

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

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

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

+/**
+ * @pre only called by instruction emulation and check was done during
+ * instruction decode
+ *
+ * @remark This function can only be called in instruction emulation and
+ * suppose always success because the check was done during instruction
+ * decode.

What's the benefit?
Why not allow Instruction emulation to inject fault?

This is for correctness.

Eddie raised the question about the sequence of read MMIO.
Some MMIO is mapped to register and multiple read could have side
effect.

When we emulate the MMIO read operation, we read the mmio first. And the
do instruction emulation. If we inject the fault at instruction
emulation, that means the mmio could be read twice (one is before we
inject fault to guest. the second is after guest handled the fault
and re-trigger the VM exit and do emulation). That could be problem.

So I am trying to move the check to instruction decode phase. So
the fault will be injected to guest before the MMIO read.

vie_calculate_gla is called twice for instruction using SI or DI.

I had plan to cache the gva we got here and used without re-calculate
in instruction emulation phase. Will try to see whether it's possible
after this patch series.

Regards
Yin, Fengwei

Anthony

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

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

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

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

- *fault = 0;
return 0;

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

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

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

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

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

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

On 08/15/2018 07:42 AM, Xu, Anthony wrote:

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

Let me add log to capture this case. As I know, trusty also
trigger EPT violation. Not sure what kind of instruction it
will use.

Regards
Yin, Fengwei

Can we print error message when it happens?
Anthony

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

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

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

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

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

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

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

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

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

--
2.17.0

Acked-by: Yan, Like <like.yan@...>

On 08/15/2018 05:33 AM, Xu, Anthony wrote:

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

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

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

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

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

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

+ /* SIB */
+ if (vie->mod != VIE_MOD_DIRECT && vie->rm == VIE_RM_SIB) {
+ goto done;
+ }
+
+ vie->base_register = vie->rm;
+
+ switch (vie->mod) {
+ case VIE_MOD_INDIRECT_DISP8:
+ vie->disp_bytes = 1U;
+ break;
+ case VIE_MOD_INDIRECT_DISP32:
+ vie->disp_bytes = 4U;
+ break;

decode_sib has the same logic, can we consolidate them?

It looks like not easy. This is part of switch..case. And there are
other specific operations for each...

Regards
Yin, Fengwei

+ 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

Could anyone help to review this patch series?

Could anyone help to review this patch series?

On 18-08-15 06:20:51, Yu Wang wrote:

On 18-08-15 02:46:58, Wu, Xiaoguang wrote:

On 18-08-14 23:15:19, Yu Wang wrote:

On 18-08-14 21:39:13, Wu, Xiaoguang wrote:

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

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

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

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

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

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

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

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

+/* port mapping status */
#define PT_NOT_ASSIGNED (0)
-#define PT_ASSIGNED_WITH_NO_DEV (-1)
+#define PT_ASSIGNED_WITH_NO_DEV (1)
+#define PT_ASSIGNED_WITH_NEW_DEV (2)
+#define PT_ASSIGNED_WITH_EMU_DEV (3)

Just like the previous comment. These names are not good. Please change
to:
VPORT_FREE
VPORT_ASSIGNED
VPORT_CONNECTED
VPORT_EMULATED

xgwu:

ok, will change. thanks.

+
+/* helpers for get port mapping information */
+#define PT_GET_NUMBER(state) (state & 0xFF)
+#define PT_GET_STATUS(state) ((state >> 8) & 0xFF)
+#define PT_MAKE_STATE(status, num) (((status & 0xFF) << 8) | (num & 0xFF))

Don't use PT. The PT in hypervisor is means passthrough... It is easy
confused..

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

xgwu:

will change to:

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

this means STATE = STATUS + NUMBER

Please don't mix the state and status, they are almost same mean. It
easy confused. Please consider other solution.

xgwu:

ok, will change to:

VPORT_NUM
VPORT_STATE
VPORT_NUM_STATE

thanks.

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

xdev = hci_data;
- di = dev_data;

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

+ for (slot = 1; slot < XHCI_MAX_SLOTS; ++slot)
+ if (xdev->slots[slot] == edev)
+ break;
+
+ assert(slot < USB_NATIVE_NUM_BUS);
+
+ status = PT_GET_STATUS(xdev->port_map_tbl[di.bus][di.port]);
+ assert(status == PT_ASSIGNED_WITH_EMU_DEV ||
+ status == PT_ASSIGNED_WITH_NEW_DEV);

Let's unify all status to state.. Don't mix them..

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

extern bool x2apic_enabled;

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

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

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

#define ACRN_DBG_EPT 6U

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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