Hi Thank you for review! On 26/04/12 03:29, Gerd Hoffmann wrote:
Okay - I missed that. I think the patches look okay to be committed - any additional changes can be made on top. Gerd - do you have any comments?
Great job overall. Can't spot issues in the code. And the patches do fine in testing too. Some minor issues poped up:
Issue #1: seabios can't boot from a virtio-scsi disk if the controller is behind a pci bridge. I think the reason simply is that (according to the seabios log) only root bus pci devices are initialized. Probably even isn't related to this patch set, just trapped into this while testing the bridge mapping code of the patch series.
I observed similar but ignored since "bridge test" requires special qemu build. It's a general bridge support issue. This series don't address this issue. This issue occurs regardless if the patches applied or not.
Issue #2: root bus (non-pref) memory regions are mapped above 4G if they are 64bit capable. That happened to include the xhci usb controller. I don't think we want that. Some day seabios will get xhci support, and having the bar above 4G makes it unreachable in 32bit mode. So this needs some refinement. Options I can think of:
(1) Don't bother mapping non-prefmem bars above 4G. (2) Only map them above 4G if they are larger than a certain limit. (3) Allow devices to be excluded on certain conditions, for example when seabios has a driver, when they have an option rom, when they have a specific pci id, ...
I like (3) more than the (1) and (2).
These are certainly no blockers and can be discussed and solved after committing this series.
I've just fixed what you have suggested to fix. Just very minor changes in patches 10/12 and 11/12: - Simplify a bit list operation. - Remove function calls away of macros - Add condition to check crossing the end of 64bit PCI range
Thanks, Alexey