Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Device should be connected to downstream port bus using device number 0 #1819

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rayx
Copy link
Contributor

@rayx rayx commented Nov 13, 2018

QPCISwitchBus was designed to hide creation of downstream port from
user. To do that, it first creates a downstream port device and its bus,
connects the downstream port device to upstream port bus (represented by
QPCISwitchBus) at address specified by the device's addr parameter. Then
it connects the device to the downstream port bus (repsented by QPCIBus)
using the same address. This doesn't work when test wants to create a
topology like the following, because the device 2 would be connected to
downstream port 2 using a non-zero addr value, which cause the device
invisible to guest OS (QEMU doens't have a check for this so it starts
up happily).

root port -> upstream port +-> downstream port 1 -> device 1
+-> downstream port 2 -> devive 2

The nature of the issue is the current code reuses device attr value for
two different purposes: the address to connect downstream port to
upstream port bus and the address to connect device to downstream port
bus. These two values are not necessarily same. They actually have to be
different in many cases.

The change fixes this issue by making sure device is always connected to
downstream port bus using device number 0. It does this by introducing a
new QPCIDownstreamPortBus class and provides its own copy of insert(),
which is almost same as QSparseBus's insert() except it hardcodes device
address to [0, 0]. This is not the best way to fix it, but is the
simplest way I can find without changing API of related classes.

Signed-off-by: Huan Xiong [email protected]

QPCISwitchBus was designed to hide creation of downstream port from
user. To do that, it first creates a downstream port device and its bus,
connects the downstream port device to upstream port bus (represented by
QPCISwitchBus) at address specified by the device's addr parameter. Then
it connects the device to the downstream port bus (repsented by QPCIBus)
using the same address. This doesn't work when test wants to create a
topology like the following, because the device 2 would be connected to
downstream port 2 using a non-zero addr value, which cause the device
invisible to guest OS (QEMU doens't have a check for this so it starts
up happily).

   root port -> upstream port +-> downstream port 1 -> device 1
                              +-> downstream port 2 -> devive 2

The nature of the issue is the current code reuses device attr value for
two different purposes: the address to connect downstream port to
upstream port bus and the address to connect device to downstream port
bus. These two values are not necessarily same. They actually have to be
different in many cases.

The change fixes this issue by making sure device is always connected to
downstream port bus using device number 0. It does this by introducing a
new QPCIDownstreamPortBus class and provides its own copy of insert(),
which is almost same as QSparseBus's insert() except it hardcodes device
address to [0, 0]. This is not the best way to fix it, but is the
simplest way I can find without changing API of related classes.

Signed-off-by: Huan Xiong <[email protected]>
@rayx
Copy link
Contributor Author

rayx commented Nov 13, 2018

I observed this issue when I look into this failure. With this patch, the test generated correct command line (all xhci controllers' attr option are 0x0) and passed.

BTW, I didn't look into if rootport code has similar issue. I'd like to focus on downstream port in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant