[SeaBIOS] [PATCH v3 3/3] pci: recognize RH PCI legacy bridge resource reservation capability

[Marcel Apfelbaum]

On 08/27/2018 05:22 AM, Liu, Jing2 wrote:
> Hi Marcel,
>
> On 8/25/2018 11:59 PM, Marcel Apfelbaum wrote:
>>
>>
>> On 08/24/2018 11:53 AM, Jing Liu wrote:
>>> Enable the firmware recognizing RedHat legacy PCI bridge device ID,
>>> so QEMU can reserve additional PCI bridge resource capability.
>>> Change the debug level lower to 3 when it is non-QEMU bridge.
>>>
>>> Signed-off-by: Jing Liu <jing2.liu at linux.intel.com>
>>> ---
>>>   src/fw/pciinit.c | 50 
>>> +++++++++++++++++++++++++++++---------------------
>>>   src/hw/pci_ids.h |  1 +
>>>   2 files changed, 30 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
>>> index 62a32f1..c0634bc 100644
>>> --- a/src/fw/pciinit.c
>>> +++ b/src/fw/pciinit.c
>>> @@ -525,30 +525,38 @@ static void pci_bios_init_platform(void)
>>>   static u8 pci_find_resource_reserve_capability(u16 bdf)
>>>   {
>>> -    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) {
>>> -        u8 cap = 0;
>>> -        do {
>>> -            cap = pci_find_capability(bdf, PCI_CAP_ID_VNDR, cap);
>>> -        } while (cap &&
>>> -                 pci_config_readb(bdf, cap + 
>>> PCI_CAP_REDHAT_TYPE_OFFSET) !=
>>> -                        REDHAT_CAP_RESOURCE_RESERVE);
>>> -        if (cap) {
>>> -            u8 cap_len = pci_config_readb(bdf, cap + PCI_CAP_FLAGS);
>>> -            if (cap_len < RES_RESERVE_CAP_SIZE) {
>>> -                dprintf(1, "PCI: QEMU resource reserve cap length 
>>> %d is invalid\n",
>>> -                        cap_len);
>>> -                return 0;
>>> -            }
>>> -        } else {
>>> -            dprintf(1, "PCI: QEMU resource reserve cap not found\n");
>>> +    u16 device_id;
>>> +
>>> +    if (pci_config_readw(bdf, PCI_VENDOR_ID) != 
>>> PCI_VENDOR_ID_REDHAT) {
>>> +        dprintf(3, "PCI: This is non-QEMU bridge.\n");
>>> +        return 0;
>>> +    }
>>> +
>>> +    device_id = pci_config_readw(bdf, PCI_DEVICE_ID);
>>> +
>>> +    if (device_id != PCI_DEVICE_ID_REDHAT_ROOT_PORT &&
>>> +        device_id != PCI_DEVICE_ID_REDHAT_BRIDGE) {
>>> +        dprintf(1, "PCI: QEMU resource reserve cap device ID 
>>> doesn't match.\n");
>>> +        return 0;
>>> +    }
>>> +    u8 cap = 0;
>>> +
>>> +    do {
>>> +        cap = pci_find_capability(bdf, PCI_CAP_ID_VNDR, cap);
>>> +    } while (cap &&
>>> +             pci_config_readb(bdf, cap + 
>>> PCI_CAP_REDHAT_TYPE_OFFSET) !=
>>> +                              REDHAT_CAP_RESOURCE_RESERVE);
>>> +    if (cap) {
>>> +        u8 cap_len = pci_config_readb(bdf, cap + PCI_CAP_FLAGS);
>>> +        if (cap_len < RES_RESERVE_CAP_SIZE) {
>>> +            dprintf(1, "PCI: QEMU resource reserve cap length %d is 
>>> invalid\n",
>>> +                    cap_len);
>>> +            return 0;
>>>           }
>>> -        return cap;
>>>       } else {
>>> -        dprintf(1, "PCI: QEMU resource reserve cap VID or DID 
>>> doesn't match.\n");
>>> -        return 0;
>>
>> I am sorry for the late review.
>> Did you drop the above line in purpose?
>>
> Thanks for the review!
>
> I replaced the above report to following phase.
> Check the vendor-id and device-id respectively.
>
> +    if (pci_config_readw(bdf, PCI_VENDOR_ID) != PCI_VENDOR_ID_REDHAT) {
> +        dprintf(3, "PCI: This is non-QEMU bridge.\n");
> +        return 0;
> +    }
> +
> +    device_id = pci_config_readw(bdf, PCI_DEVICE_ID);
> +
> +    if (device_id != PCI_DEVICE_ID_REDHAT_ROOT_PORT &&
> +        device_id != PCI_DEVICE_ID_REDHAT_BRIDGE) {
> +        dprintf(1, "PCI: QEMU resource reserve cap device ID doesn't 
> match.\n");
> +        return 0;
> +    }
>

I understand.

Reviewed-by: Marcel Apfelbaum<marcel.apfelbaum at gmail.com>


Thanks,
Marcel

> Thanks,
> Jing
>
>> Thanks,
>> Marcel