[SeaBIOS] [PATCH] pci: add RedHat PCI BRIDGE capability

[Laszlo Ersek]

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.

Thanks
Laszlo