On 8/8/2018 6:44 PM, Laszlo Ersek wrote:
On 08/08/18 05:24, Liu, Jing2 wrote:
On 8/7/2018 7:43 PM, Laszlo Ersek wrote:
On 08/07/18 09:20, Jing Liu wrote:
[snip]
- if (pci_config_readw(bdf, PCI_VENDOR_ID) == PCI_VENDOR_ID_REDHAT && - pci_config_readw(bdf, PCI_DEVICE_ID) == - PCI_DEVICE_ID_REDHAT_ROOT_PORT) { + u16 vendor_id = pci_config_readw(bdf, PCI_VENDOR_ID); + u16 device_id = pci_config_readw(bdf, PCI_DEVICE_ID);
[snip]
(5) Regarding the code: I'm not sure how careful SeaBIOS is about unnecessary config space accesses (i.e., unnecessary traps to the host). Personally I would prefer if we didn't unconditionally read the device ID post-patch either -- that is, if the vendor ID doesn't match, we shouldn't read the device ID. Something like:
Do you mean we need prevent the compiler read device ID in advanced when vendor ID does not matched? If not, why the original codes will read device ID when the vendor Id check fails?
What I mean is that the original code (see in the context above) uses the "logical and" (&&) operator; if the Vendor ID does not match PCI_VENDOR_ID_REDHAT, then the Device ID is not read from config space at all. After the patch (see above again), the Device ID is read unconditionally, even if we later find that the Vendor ID is a mismatch, and so throw away the Device ID. In that case, the Device ID read (which is a trap from the guest to KVM to QEMU) is wasted.
It's likely not extremely important to be as frugal as possible with config space accesses (traps); however, if it's not a big complication code-wise to avoid possibly wasted reads (traps), then I think we should be frugal.
Ah, yes! I didn't realize that and I aggree with you! Will change that in new version later.
Jing
Thanks Laszlo