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

[Laszlo Ersek]

adding Marcel; comments at the bottom

On 08/07/18 09:20, Jing Liu wrote:
> Add a device-specific capability for the RedHat PCI BRIDGE
> to enable reserving additional resources.
> Signed-off-by: Jing Liu
>
> src/fw/pciinit.c | 9 ++++++---
>  src/hw/pci_ids.h | 1 +
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index 3a2f747..0265e9d 100644
> --- a/src/fw/pciinit.c
> +++ b/src/fw/pciinit.c
> @@ -525,9 +525,12 @@ 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) {
> u16 vendor_id = pci_config_readw(bdf, PCI_VENDOR_ID);
> u16 device_id = pci_config_readw(bdf, PCI_DEVICE_ID);
> if (vendor_id == PCI_VENDOR_ID_REDHAT &&
>    (device_id == PCI_DEVICE_ID_REDHAT_ROOT_PORT ||
>     device_id == PCI_DEVICE_ID_REDHAT_BRIDGE)) {
>    u8 cap = 0;
>    do {
>        cap = pci_find_capability(bdf, PCI_CAP_ID_VNDR, 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

Given that you are touching this function, it's a good opportunity to
clean up its issues that I pointed out earlier in
https://bugzilla.redhat.com/show_bug.cgi?id=1536147#c0, noted as "side
comments (a), (b) and (c)". I'll re-iterate:

* The "PCI: QEMU resource reserve cap not found" debug message is
  printed under wrong conditions. Namely, it is both printed when it
  makes no sense (i.e., when the vendor-id/device-id don't match and we
  don't even go looking for the capability), and it's *not* printed when
  it does makes sense (the search loop completes without finding the
  capability).

* There is a logic bug: if we find the capability but it's truncated, we
  print a good error message, but then go ahead and return the offset of
  the broken (truncated) capability just the same. In this case, we
  should return zero.

So, I suggest that you please:

(1) send a patch that fixes the logic bug,

(2) send another patch that cleans up the debug messages,

(3) send yet another patch that recognizes the capability in question on
    the traditional bridge device too. (I.e., a variant of the current
    patch, rebased to (1) and (2)).

More comments for this patch:

(4) the subject line should be clarified, such as:

pci: recognize RH resource reservation capability on traditional bridges

(72 characters). The commit message body should be updated
    accordingly -- we're not adding the capability, just matching it on
    another device.

(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:
> u16 device_id;
>
> if (pci_config_readw(bdf, PCI_VENDOR_ID) != PCI_VENDOR_ID_REDHAT) {
>     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) {
>     return 0;
>   }
>
> /* success / failure messages are justified after this point */
>   ...

The vendor-id/device-id checks should likely be reorganized as shown
above in patch (2), as a part of the debug message cleanup. And then the
device-id check can be extended to cover PCI_DEVICE_ID_REDHAT_BRIDGE in
patch (3).

Just my opinion of course; I'm not a SeaBIOS maintainer.

Thanks,
Laszlo