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


Yu Wang
 

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

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

This design will fail if the disconnection happened in the SOS, which
I think it should be re-connection instead of disconnection, right?

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

Currently when the SOS do the resuming from S3, there are some uncertain
bugs which unbind the usbfs for the USB devices, so the DM will lose
control and can't continue emulation.
Currently, the native S3 will disable the vbus for all xHCI ports and
re-drive during S3 resume. This behavior cause native USB driver unbind
the usbfs and bind to related class driver, then made the DM lost
control and failed to continue emulation.


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

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

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

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

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

@@ -475,36 +476,21 @@ static struct pci_xhci_option_elem xhci_option_table[] = {
static int
pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
{
- struct pci_xhci_dev_emu *de;
struct pci_xhci_vdev *xdev;
- struct usb_devemu *ue;
struct usb_native_devinfo *di;
int port_start, port_end;
- int slot_start, slot_end;
- int port, slot;
- void *ud;
+ int port;
int intr = 1;
+ int i, j;

xdev = hci_data;
- di = dev_data;

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

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

/* print physical information about new device */
UPRINTF(LDBG, "%04x:%04x %d-%d connecting.\r\n",
@@ -517,43 +503,53 @@ pci_xhci_native_usb_dev_conn_cb(void *hci_data, void *dev_data)
goto errout;
}

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

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

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

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


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


- if (port >= port_end || slot >= slot_end) {
- UPRINTF(LFTL, "no free resource: port %d slot %d\r\n",
- port, slot);
+ if (port >= port_end) {
+ UPRINTF(LFTL, "no free virtual port for native device %d-%d"
+ "\r\n", di->bus, di->port);
goto errout;
}

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

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

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

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

@@ -571,8 +566,8 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
struct pci_xhci_vdev *xdev;
struct pci_xhci_dev_emu *edev;
struct usb_dev *udev;
- uint8_t port, native_port;
- int intr = 1;
+ uint8_t port, slot, native_port;
+ int i, j, intr = 1;

assert(hci_data);
assert(dev_data);
@@ -601,6 +596,24 @@ pci_xhci_native_usb_dev_disconn_cb(void *hci_data, void *dev_data)
return -1;
}

+ for (slot = 1; slot < XHCI_MAX_SLOTS; ++slot)
+ if (xdev->slots[slot] == edev)
+ break;
+
+ /* FIXME: again, find a better relationship for Slot, Port and Device */
+ for (i = 0; i < USB_NATIVE_NUM_BUS; ++i) {
+ if (!xdev->native_assign_ports[i])
+ continue;
+
+ for (j = 0; j < USB_NATIVE_NUM_PORT; ++j)
+ if (xdev->native_assign_ports[i][j] == port)
+ break;
+
+ if (j < USB_NATIVE_NUM_PORT)
+ break;
+ }
Ditto.

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

UPRINTF(LDBG, "report virtual port %d status\r\n", port);
if (pci_xhci_port_disconnect(xdev, port, intr)) {
UPRINTF(LFTL, "fail to report event\r\n");
@@ -1405,21 +1418,61 @@ static uint32_t
pci_xhci_cmd_enable_slot(struct pci_xhci_vdev *xdev, uint32_t *slot)
{
struct pci_xhci_dev_emu *dev;
- uint32_t cmderr;
- int i;
+ uint32_t cmderr;
+ struct usb_native_devinfo *di;
+ int i, j, port;

cmderr = XHCI_TRB_ERROR_NO_SLOTS;
- if (xdev->portregs != NULL)
- for (i = 1; i <= XHCI_MAX_SLOTS; i++) {
- dev = XHCI_SLOTDEV_PTR(xdev, i);
- if (dev && dev->dev_slotstate == XHCI_ST_DISABLED) {
- *slot = i;
- dev->dev_slotstate = XHCI_ST_ENABLED;
- cmderr = XHCI_TRB_ERROR_SUCCESS;
- dev->hci.hci_address = i;
- break;
+
+ for (i = 0; i < USB_NATIVE_NUM_BUS; ++i) {
+ if (!xdev->native_assign_ports[i])
+ continue;
+
+ for (j = 0; j < USB_NATIVE_NUM_PORT; ++j) {
+ if (xdev->native_assign_ports[i][j] > 0) {
+ port = xdev->native_assign_ports[i][j];
+ assert(port <= XHCI_MAX_DEVS);
+
+ if (!xdev->devices[port])
+ break;
}
}
+ if (j < USB_NATIVE_NUM_PORT)
+ break;
Ditto

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

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

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

for (i = 1; i <= XHCI_MAX_DEVS; ++i)
@@ -1463,8 +1522,25 @@ pci_xhci_cmd_disable_slot(struct pci_xhci_vdev *xdev, uint32_t slot)
if (i <= XHCI_MAX_DEVS && XHCI_PORTREG_PTR(xdev, i)) {
XHCI_PORTREG_PTR(xdev, i)->portsc &= ~(XHCI_PS_CSC |
XHCI_PS_CCS | XHCI_PS_PED | XHCI_PS_PP);
+
+ ud = dev->dev_instance;
+ ue = dev->dev_ue;
+
+ assert(ud);
+ assert(ue);
+
+ ue->ue_info(ud, USB_INFO_BUS, &native_bus, sizeof(uint8_t));
+ ue->ue_info(ud, USB_INFO_PORT, &native_port, sizeof(uint8_t));
+
xdev->devices[i] = NULL;
xdev->slots[slot] = NULL;
+
+ assert(xdev->native_assign_ports[native_bus]);
+ assert(xdev->native_assign_ports[native_bus][native_port]);
+ UPRINTF(LINF, "disable slot %d for native device %d-%d"
+ "\r\n", slot, native_bus, native_port);
+
+ xdev->native_assign_ports[native_bus][native_port] = -1;
pci_xhci_dev_destroy(dev);
} else
UPRINTF(LWRN, "invalid slot %d\r\n", slot);
@@ -1632,7 +1708,10 @@ pci_xhci_cmd_config_ep(struct pci_xhci_vdev *xdev,
UPRINTF(LDBG, "config_ep slot %u\r\n", slot);

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

if ((trb->dwTrb3 & XHCI_TRB_3_DCEP_BIT) != 0) {
UPRINTF(LDBG, "config_ep - deconfigure ep slot %u\r\n", slot);
@@ -3352,8 +3431,10 @@ errout:
static int
pci_xhci_parse_bus_port(struct pci_xhci_vdev *xdev, char *opts)
{
- int rc = 0, cnt;
+ int rc = 0, cnt, portcnt;
uint32_t port, bus;
+ int8_t *pports;
+ struct usb_native_devinfo *pinfo;

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

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

errout:
if (rc)
--
2.7.4



Join acrn-dev@lists.projectacrn.org to automatically receive all group messages.