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

[Liu, Jing2]

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
>