Re: [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
> > >
> > > 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

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