[PATCH v2 0/3] pci: resource reserve capability found

[Jing Liu]

This patch serial is about QEMU resource reserve capability finding
in firmware.

Firstly, this fixes a logic bug. When the capability is truncated,
return zero instead of the truncated offset. Secondly, this modified
the debug messages when the capability is not found and when the vendor
ID or device Id doesn't match REDHAT special ones.

Last, this enables the firmware recongizing the REDHAT PCI BRIDGE device ID,
so that QEMU can also reserve PCI bridge resource capability.

Change Log:
v2 -> v1
* Add two patches for fixing small bugs

Jing Liu (3):
  pci: fix the return value for truncated capability
  pci: clean up the debug message for pci capability found
  pci: recognize RH PCI legacy bridge resource reservation capability

src/fw/pciinit.c | 45 ++++++++++++++++++++++++++++-----------------
 src/hw/pci_ids.h |  1 +
 2 files changed, 29 insertions(+), 17 deletions(-)

[Liu, Jing2]

Hi all,
I re-send the serial because of wrong numbering last time.
Sorry for troubling.

Thanks!
Jing
On 8/16/2018 10:32 AM, Jing Liu wrote:
> This patch serial is about QEMU resource reserve capability finding
> in firmware.
>
> Firstly, this fixes a logic bug. When the capability is truncated,
> return zero instead of the truncated offset. Secondly, this modified
> the debug messages when the capability is not found and when the vendor
> ID or device Id doesn't match REDHAT special ones.
>
> Last, this enables the firmware recongizing the REDHAT PCI BRIDGE device ID,
> so that QEMU can also reserve PCI bridge resource capability.
>
> Change Log:
> v2 -> v1
> Add two patches for fixing small bugs
>
> Jing Liu (3):
>    pci: fix the return value for truncated capability
>    pci: clean up the debug message for pci capability found
>    pci: recognize RH PCI legacy bridge resource reservation capability
>
> src/fw/pciinit.c | 45 ++++++++++++++++++++++++++++-----------------
>   src/hw/pci_ids.h |  1 +
>   2 files changed, 29 insertions(+), 17 deletions(-)

[PATCH v2 1/3] pci: fix the return value for truncated capability

[Jing Liu]

Return zero when finding truncated capability.

Signed-off-by: Jing Liu 
---
 src/fw/pciinit.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index 3a2f747..d2cea2b 100644
--- a/src/fw/pciinit.c
+++ b/src/fw/pciinit.c
@@ -539,6 +539,7 @@ static u8 pci_find_resource_reserve_capability(u16 bdf)
             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;

[PATCH v2 1/3] pci: fix the return value for truncated capability

[Marcel Apfelbaum]

On 08/16/2018 05:32 AM, Jing Liu wrote:
> Return zero when finding truncated capability.
> Signed-off-by: Jing Liu
>
> src/fw/pciinit.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index 3a2f747..d2cea2b 100644
> --- a/src/fw/pciinit.c
> +++ b/src/fw/pciinit.c
> @@ -539,6 +539,7 @@ static u8 pci_find_resource_reserve_capability(u16 bdf)
>               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;

Reviewed-by: Marcel Apfelbaum

Thanks,
Marcel

[PATCH v2 2/3] pci: clean up the debug message for pci capability found

[Jing Liu]

Improve the debug message when QEMU resource reserve cap
is not found and when the vendor-id or device-id does't match
REDHAT special ones.

Signed-off-by: Jing Liu 
---
 src/fw/pciinit.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index d2cea2b..62a32f1 100644
--- a/src/fw/pciinit.c
+++ b/src/fw/pciinit.c
@@ -541,10 +541,12 @@ static u8 pci_find_resource_reserve_capability(u16 bdf)
                         cap_len);
                 return 0;
             }
+        } else {
+            dprintf(1, "PCI: QEMU resource reserve cap not found\n");
         }
         return cap;
     } else {
-        dprintf(1, "PCI: QEMU resource reserve cap not found\n");
+        dprintf(1, "PCI: QEMU resource reserve cap VID or DID doesn't match.\n");
         return 0;
     }
 }

[PATCH v2 2/3] pci: clean up the debug message for pci capability found

[Marcel Apfelbaum]

On 08/16/2018 05:32 AM, Jing Liu wrote:
> Improve the debug message when QEMU resource reserve cap
> is not found and when the vendor-id or device-id does't match
> REDHAT special ones.
> Signed-off-by: Jing Liu
>
> src/fw/pciinit.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index d2cea2b..62a32f1 100644
> --- a/src/fw/pciinit.c
> +++ b/src/fw/pciinit.c
> @@ -541,10 +541,12 @@ static u8 pci_find_resource_reserve_capability(u16 bdf)
>                           cap_len);
>                   return 0;
>               }
>    } else {
>        dprintf(1, "PCI: QEMU resource reserve cap not found\n");
>     }
>     return cap;
> } else {
>    dprintf(1, "PCI: QEMU resource reserve cap not found\n");
>    dprintf(1, "PCI: QEMU resource reserve cap VID or DID doesn't match.\n");
>     return 0;
> }
> }

Reviewed-by: Marcel Apfelbaum

Thanks,
Marcel

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

[Jing Liu]

Enable the firmware recognizing RedHat legacy PCI bridge device ID,
so QEMU can reserve additional PCI bridge resource capability.

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..795d84a 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(1, "PCI: QEMU resource reserve cap vendor ID doesn't match.\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;
+        dprintf(1, "PCI: QEMU resource reserve cap not found\n");
     }
+    return cap;
 }

/****************************************************************
diff --git a/src/hw/pci_ids.h b/src/hw/pci_ids.h
index 38fa2ca..1096461 100644
--- a/src/hw/pci_ids.h
+++ b/src/hw/pci_ids.h
@@ -2265,6 +2265,7 @@

#define PCI_VENDOR_ID_REDHAT		0x1b36
 #define PCI_DEVICE_ID_REDHAT_ROOT_PORT	0x000C
+#define PCI_DEVICE_ID_REDHAT_BRIDGE	0x0001

#define PCI_VENDOR_ID_TEKRAM		0x1de1
 #define PCI_DEVICE_ID_TEKRAM_DC290	0xdc29

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

[Liu, Jing2]

Hi Zihan,

I copy your comments of this patch several days ago here,
and hope we could continue some discussion in this serial.

On 8/16/2018 10:32 AM, Jing Liu wrote:
[...]
> 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(1, "PCI: QEMU resource reserve cap vendor ID doesn't match.\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) {

======= comments by Zihan Yang =========
I think PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE shoud be added too, in case we put
a pcie_pci_bridge at the host bus.
========================================
My reply:
In theory all the pci bridge could add this capability but I'm not sure
if we really need that? Would like to hear some more suggestion!

Thanks,
Jing

[...]

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

[Zihan Yang]

Liu, Jing2  于2018年8月16日周四 上午10:39写道：
> Hi Zihan,
>
> I copy your comments of this patch several days ago here,
> and hope we could continue some discussion in this serial.

Hi Jing,

Sorry for the delay. I tested at the code just now and find that pcie
bridge does not
use a vendor specific capability. I met this warning in another
ongoing patch so I just
added pcie bridge to suppress the warning.

Now that it turns out pcie bridge does not use this vendor specific
capability, I think you
might want to stick with your original patch, and add it in the future
when redhat decides
to add specific capabilities in pcie bridge. I'm not quite an expert
in this field, so unfortunately
I don't have much insights to give, but I'm open for a discussion.
> On 8/16/2018 10:32 AM, Jing Liu wrote:
> [...]
> > 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(1, "PCI: QEMU resource reserve cap vendor ID doesn't match.\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) {
>
> ======= comments by Zihan Yang =========
> I think PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE shoud be added too, in case we put
> a pcie_pci_bridge at the host bus.
> ========================================
> My reply:
> In theory all the pci bridge could add this capability but I'm not sure
> if we really need that? Would like to hear some more suggestion!
>
> Thanks,
> Jing
>
> [...]

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

[Gerd Hoffmann]

Hi,
> I think PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE shoud be added too, in case we put
> a pcie_pci_bridge at the host bus.

Yes, please.

thanks,
  Gerd

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

[Liu, Jing2]

Hi Gerd,

On 8/16/2018 3:19 PM, Gerd Hoffmann wrote:
> Hi,
> > I think PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE shoud be added too, in case we put
> > a pcie_pci_bridge at the host bus.
>
> Yes, please.

Thanks for comments. Sure, can be added simply. While this bridge 
doesn't have resource reserve capability in qemu now.  Do we really need 
this for now?

Thanks
Jing
> thanks,
>    Gerd

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

[Gerd Hoffmann]

On Thu, Aug 16, 2018 at 05:30:45PM +0800, Liu, Jing2 wrote:
> Hi Gerd,
>
> On 8/16/2018 3:19 PM, Gerd Hoffmann wrote:
> > Hi,
> > > I think PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE shoud be added too, in case we put
> > > a pcie_pci_bridge at the host bus.
> >
> > Yes, please.
>
> Thanks for comments. Sure, can be added simply. While this bridge doesn't
> have resource reserve capability in qemu now.  Do we really need this for
> now?

No.  I assumed all bridges have that without actually checking.  So this
can wait until qemu added the options.  Once the refactoring is done
whitelisting more bridges is a simple two-line patch.

cheers,
  Gerd

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

[Liu, Jing2]

On 8/16/2018 9:10 PM, Gerd Hoffmann wrote:
> On Thu, Aug 16, 2018 at 05:30:45PM +0800, Liu, Jing2 wrote:
> > Hi Gerd,
> >
> > On 8/16/2018 3:19 PM, Gerd Hoffmann wrote:
> > > Hi,
> > >
> > > > I think PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE shoud be added too, in case we put
> > > > a pcie_pci_bridge at the host bus.
> > >
> > > Yes, please.
> >
> > Thanks for comments. Sure, can be added simply. While this bridge doesn't
> > have resource reserve capability in qemu now.  Do we really need this for
> > now?
>
> No.  I assumed all bridges have that without actually checking.  So this
> can wait until qemu added the options.

OK, so let's just keep the patch and add the support for other bridges
until qemu has this capability in the future, right?
> Once the refactoring is done
> whitelisting more bridges is a simple two-line patch.

Yes, I agree.

Thanks,
Jing
> cheers,
>    Gerd

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

[Gerd Hoffmann]

Hi,
> if (pci_config_readw(bdf, PCI_VENDOR_ID) != PCI_VENDOR_ID_REDHAT) {
>    dprintf(1, "PCI: QEMU resource reserve cap vendor ID doesn't match.\n");

I'd suggest to use a higher debug level for this one, 3 would be a good
pick I think.  level 1 messages are printed by default, and we should
not spam the log just because there is a non-qemu bridge present in the
system.

cheers,
  Gerd

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

[Liu, Jing2]

On 8/16/2018 3:18 PM, Gerd Hoffmann wrote:
> Hi,
> > if (pci_config_readw(bdf, PCI_VENDOR_ID) != PCI_VENDOR_ID_REDHAT) {
> >    dprintf(1, "PCI: QEMU resource reserve cap vendor ID doesn't match.\n");
>
> I'd suggest to use a higher debug level for this one, 3 would be a good
> pick I think.  level 1 messages are printed by default, and we should
> not spam the log just because there is a non-qemu bridge present in the
> system.

OK. Will do that.

Jing
> cheers,
>    Gerd

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

[Laszlo Ersek]

On 08/16/18 12:43, Liu, Jing2 wrote:
> On 8/16/2018 3:18 PM, Gerd Hoffmann wrote:
> > Hi,
> > > +    if (pci_config_readw(bdf, PCI_VENDOR_ID) != PCI_VENDOR_ID_REDHAT) {
> > > +        dprintf(1, "PCI: QEMU resource reserve cap vendor ID doesn't
> > > match.\n");
> >
> > I'd suggest to use a higher debug level for this one, 3 would be a good
> > pick I think.  level 1 messages are printed by default, and we should
> > not spam the log just because there is a non-qemu bridge present in the
> > system.
>
> OK. Will do that.

With the debug level update, I'm ready to give my R-b for this series.

Thanks,
Laszlo

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

[Liu, Jing2]

Hi Laszlo,

On 8/22/2018 5:13 PM, Laszlo Ersek wrote:
> On 08/16/18 12:43, Liu, Jing2 wrote:
> > On 8/16/2018 3:18 PM, Gerd Hoffmann wrote:
> > > Hi,
> > > > +    if (pci_config_readw(bdf, PCI_VENDOR_ID) != PCI_VENDOR_ID_REDHAT) {
> > > > +        dprintf(1, "PCI: QEMU resource reserve cap vendor ID doesn't
> > > > match.\n");
> > >
> > > I'd suggest to use a higher debug level for this one, 3 would be a good
> > > pick I think.  level 1 messages are printed by default, and we should
> > > not spam the log just because there is a non-qemu bridge present in the
> > > system.
> >
> > OK. Will do that.
>
> With the debug level update, I'm ready to give my R-b for this series.

Thanks for your feedback!
So do I need update another version and with your R-b?

Thanks,
Jing
> Thanks,
> Laszlo

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

[Laszlo Ersek]

On 08/24/18 04:23, Liu, Jing2 wrote:
> Hi Laszlo,
>
> On 8/22/2018 5:13 PM, Laszlo Ersek wrote:
> > On 08/16/18 12:43, Liu, Jing2 wrote:
> > > On 8/16/2018 3:18 PM, Gerd Hoffmann wrote:
> > > > Hi,
> > > > > +    if (pci_config_readw(bdf, PCI_VENDOR_ID) !=
> > > > > PCI_VENDOR_ID_REDHAT) {
> > > > > +        dprintf(1, "PCI: QEMU resource reserve cap vendor ID doesn't
> > > > > match.\n");
> > > >
> > > > I'd suggest to use a higher debug level for this one, 3 would be a good
> > > > pick I think.  level 1 messages are printed by default, and we should
> > > > not spam the log just because there is a non-qemu bridge present in the
> > > > system.
> > >
> > > OK. Will do that.
> >
> > With the debug level update, I'm ready to give my R-b for this series.
>
> Thanks for your feedback!
> So do I need update another version and with your R-b?

I imagine you'd post v3 with the update Gerd requested for the debug
level(s), and then I'd respond with my R-b. (Obviously I'm not a SeaBIOS
maintainer so that'll not be "decisive" by any means.)

Thanks
Laszlo

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

[Liu, Jing2]

On 8/24/2018 3:12 PM, Laszlo Ersek wrote:
> On 08/24/18 04:23, Liu, Jing2 wrote:
> > Hi Laszlo,
> >
> > On 8/22/2018 5:13 PM, Laszlo Ersek wrote:
> > > On 08/16/18 12:43, Liu, Jing2 wrote:
> > > > On 8/16/2018 3:18 PM, Gerd Hoffmann wrote:
> > > > > Hi,
> > > > > > +    if (pci_config_readw(bdf, PCI_VENDOR_ID) !=
> > > > > > PCI_VENDOR_ID_REDHAT) {
> > > > > > +        dprintf(1, "PCI: QEMU resource reserve cap vendor ID doesn't
> > > > > > match.\n");
> > > > >
> > > > > I'd suggest to use a higher debug level for this one, 3 would be a good
> > > > > pick I think.  level 1 messages are printed by default, and we should
> > > > > not spam the log just because there is a non-qemu bridge present in the
> > > > > system.
> > > >
> > > > OK. Will do that.
> > >
> > > With the debug level update, I'm ready to give my R-b for this series.
> >
> > Thanks for your feedback!
> > So do I need update another version and with your R-b?
>
> I imagine you'd post v3 with the update Gerd requested for the debug
> level(s), and then I'd respond with my R-b. (Obviously I'm not a SeaBIOS
> maintainer so that'll not be "decisive" by any means.)

Got it. Will send later!

Thanks,
Jing
> Thanks
> Laszlo

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

[Liu, Jing2]

On 8/24/2018 3:12 PM, Laszlo Ersek wrote:
> On 08/24/18 04:23, Liu, Jing2 wrote:
> > Hi Laszlo,
> >
> > On 8/22/2018 5:13 PM, Laszlo Ersek wrote:
> > > On 08/16/18 12:43, Liu, Jing2 wrote:
> > > > On 8/16/2018 3:18 PM, Gerd Hoffmann wrote:
> > > > > Hi,
> > > > > > +    if (pci_config_readw(bdf, PCI_VENDOR_ID) !=
> > > > > > PCI_VENDOR_ID_REDHAT) {
> > > > > > +        dprintf(1, "PCI: QEMU resource reserve cap vendor ID doesn't
> > > > > > match.\n");
> > > > >
> > > > > I'd suggest to use a higher debug level for this one, 3 would be a good
> > > > > pick I think.  level 1 messages are printed by default, and we should
> > > > > not spam the log just because there is a non-qemu bridge present in the
> > > > > system.
> > > >
> > > > OK. Will do that.
> > >
> > > With the debug level update, I'm ready to give my R-b for this series.
> >
> > Thanks for your feedback!
> > So do I need update another version and with your R-b?
>
> I imagine you'd post v3 with the update Gerd requested for the debug
> level(s), and then I'd respond with my R-b. (Obviously I'm not a SeaBIOS
> maintainer so that'll not be "decisive" by any means.)

Oh, BTW, I am considering, if only dismatch vendor-id stands for
"non-qemu bridge" or dismatch both vid and did? I guess I need change both.

Thanks,
Jing
> Thanks
> Laszlo

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

[Laszlo Ersek]

On 08/24/18 09:48, Liu, Jing2 wrote:
> On 8/24/2018 3:12 PM, Laszlo Ersek wrote:
> > On 08/24/18 04:23, Liu, Jing2 wrote:
> > > Hi Laszlo,
> > >
> > > On 8/22/2018 5:13 PM, Laszlo Ersek wrote:
> > > > On 08/16/18 12:43, Liu, Jing2 wrote:
> > > > > On 8/16/2018 3:18 PM, Gerd Hoffmann wrote:
> > > > > > Hi,
> > > > > >
> > > > > > > +    if (pci_config_readw(bdf, PCI_VENDOR_ID) !=
> > > > > > > PCI_VENDOR_ID_REDHAT) {
> > > > > > > +        dprintf(1, "PCI: QEMU resource reserve cap vendor ID
> > > > > > > doesn't
> > > > > > > match.\n");
> > > > > >
> > > > > > I'd suggest to use a higher debug level for this one, 3 would be a
> > > > > > good
> > > > > > pick I think.  level 1 messages are printed by default, and we should
> > > > > > not spam the log just because there is a non-qemu bridge present
> > > > > > in the
> > > > > > system.
> > > > >
> > > > > OK. Will do that.
> > > >
> > > > With the debug level update, I'm ready to give my R-b for this series.
> > >
> > > Thanks for your feedback!
> > > So do I need update another version and with your R-b?
> >
> > I imagine you'd post v3 with the update Gerd requested for the debug
> > level(s), and then I'd respond with my R-b. (Obviously I'm not a SeaBIOS
> > maintainer so that'll not be "decisive" by any means.)
>
> Oh, BTW, I am considering, if only dismatch vendor-id stands for
> "non-qemu bridge" or dismatch both vid and did? I guess I need change both.

I don't understand. Can you post an incremental diff in this thread just
for illustration?

Thanks
Laszlo

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

[Liu, Jing2]

On 8/24/2018 4:02 PM, Laszlo Ersek wrote:
> On 08/24/18 09:48, Liu, Jing2 wrote:

[...]
> > > > > > >> +    if (pci_config_readw(bdf, PCI_VENDOR_ID) !=
> > > > > > >> PCI_VENDOR_ID_REDHAT) {
> > > > > > >> +        dprintf(1, "PCI: QEMU resource reserve cap vendor ID
> > > > > > >> doesn't
> > > > > > >> match.\n");
> > > > > > >
> > > > > > > I'd suggest to use a higher debug level for this one, 3 would be a
> > > > > > > good
> > > > > > > pick I think.  level 1 messages are printed by default, and we should
> > > > > > > not spam the log just because there is a non-qemu bridge present
> > > > > > > in the
> > > > > > > system.
> > > > > > OK. Will do that.
> > > > >
> > > > > With the debug level update, I'm ready to give my R-b for this series.
> > > >
> > > > Thanks for your feedback!
> > > > So do I need update another version and with your R-b?
> > >
> > > I imagine you'd post v3 with the update Gerd requested for the debug
> > > level(s), and then I'd respond with my R-b. (Obviously I'm not a SeaBIOS
> > > maintainer so that'll not be "decisive" by any means.)
> >
> > Oh, BTW, I am considering, if only dismatch vendor-id stands for
> > "non-qemu bridge" or dismatch both vid and did? I guess I need change both.
>
> I don't understand. Can you post an incremental diff in this thread just
> for illustration?

We check vendor-id/device-id and report when dismatch.
My question is,
Gerd suggested "not spam the log just because there is a non-qemu bridge 
present", so I think if both vendor-id and device-id dismatch means 
non-qemu bridge present?

+    if (pci_config_readw(bdf, PCI_VENDOR_ID) != PCI_VENDOR_ID_REDHAT) {
+        dprintf(3, "PCI: QEMU resource reserve cap vendor ID doesn't 
match.\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(3, "PCI: QEMU resource reserve cap device ID doesn't 
match.\n");
+        return 0;
+    }
> Thanks
> Laszlo

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

[Gerd Hoffmann]

> > I imagine you'd post v3 with the update Gerd requested for the debug
> > level(s), and then I'd respond with my R-b. (Obviously I'm not a SeaBIOS
> > maintainer so that'll not be "decisive" by any means.)
>
> Oh, BTW, I am considering, if only dismatch vendor-id stands for
> "non-qemu bridge" or dismatch both vid and did? I guess I need change both.

Yes, only the vendor id.  If you found a pci bridge with redhat vendor
id you know it's qemu.

cheers,
  Gerd

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

[Liu, Jing2]

Hi Gerd,

On 8/24/2018 4:24 PM, Gerd Hoffmann wrote:
> > > I imagine you'd post v3 with the update Gerd requested for the debug
> > > level(s), and then I'd respond with my R-b. (Obviously I'm not a SeaBIOS
> > > maintainer so that'll not be "decisive" by any means.)
> >
> > Oh, BTW, I am considering, if only dismatch vendor-id stands for
> > "non-qemu bridge" or dismatch both vid and did? I guess I need change both.
>
> Yes, only the vendor id.  If you found a pci bridge with redhat vendor
> id you know it's qemu.

Oh, got it! Thanks Gerd!
Will send v3 later.

Jing
> cheers,
>    Gerd

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

[Gerd Hoffmann]

On Fri, Aug 24, 2018 at 09:12:47AM +0200, Laszlo Ersek wrote:
> On 08/24/18 04:23, Liu, Jing2 wrote:
> > Hi Laszlo,
> >
> > On 8/22/2018 5:13 PM, Laszlo Ersek wrote:
> > > On 08/16/18 12:43, Liu, Jing2 wrote:
> > > > On 8/16/2018 3:18 PM, Gerd Hoffmann wrote:
> > > > > Hi,
> > > > > > +    if (pci_config_readw(bdf, PCI_VENDOR_ID) !=
> > > > > > PCI_VENDOR_ID_REDHAT) {
> > > > > > +        dprintf(1, "PCI: QEMU resource reserve cap vendor ID doesn't
> > > > > > match.\n");
> > > > >
> > > > > I'd suggest to use a higher debug level for this one, 3 would be a good
> > > > > pick I think.  level 1 messages are printed by default, and we should
> > > > > not spam the log just because there is a non-qemu bridge present in the
> > > > > system.
> > > >
> > > > OK. Will do that.
> > >
> > > With the debug level update, I'm ready to give my R-b for this series.
> >
> > Thanks for your feedback!
> > So do I need update another version and with your R-b?
>
> I imagine you'd post v3 with the update Gerd requested for the debug
> level(s), and then I'd respond with my R-b. (Obviously I'm not a SeaBIOS
> maintainer so that'll not be "decisive" by any means.)

Yep, I've expected you post a v3 with the debug level change (whole
series).

cheers,
  Gerd