Add a device-specific capability for the RedHat PCI BRIDGE
to enable reserving additional resources.
Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
---
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
--
1.8.3.1
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
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 <jing2.liu@linux.intel.com>
> ---
> 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
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
On 8/7/2018 7:43 PM, Laszlo Ersek wrote:
> 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 <jing2.liu@linux.intel.com>
>> ---
>> 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.
>
I agree with you!
> 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)).
>
I will prepare these patches and welcome comments.
> 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.
>
Got it, change the title and commit message later.
> (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?
>> 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.
>
Thank you very much, Laszlo!
Jing
> Thanks,
> Laszlo
>
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
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.
Thanks
Laszlo
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
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
>
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
© 2016 - 2024 Red Hat, Inc.