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
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
