[dpdk-kmods] windows/netuio: fix bar parsing
Checks
Commit Message
For certain PCIe devices, BAR values are not continuous.
This patch maps all the BARs and avoids skipping the next BAR addresses.
Fixes: e28aabd88279 ("windows/netuio: introduce NetUIO kernel driver")
Cc: navasile@microsoft.com
Signed-off-by: Qiao Liu <qiao.liu@intel.com>
Signed-off-by: Pallavi Kadam <pallavi.kadam@intel.com>
---
windows/netuio/netuio_dev.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Comments
2022-08-02 12:10 (UTC-0700), Pallavi Kadam:
> For certain PCIe devices, BAR values are not continuous.
> This patch maps all the BARs and avoids skipping the next BAR addresses.
>
> Fixes: e28aabd88279 ("windows/netuio: introduce NetUIO kernel driver")
> Cc: navasile@microsoft.com
>
> Signed-off-by: Qiao Liu <qiao.liu@intel.com>
> Signed-off-by: Pallavi Kadam <pallavi.kadam@intel.com>
> ---
> windows/netuio/netuio_dev.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/windows/netuio/netuio_dev.c b/windows/netuio/netuio_dev.c
> index b2deb10..e2cac3e 100644
> --- a/windows/netuio/netuio_dev.c
> +++ b/windows/netuio/netuio_dev.c
> @@ -172,6 +172,7 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST Resources, WDFCMRESLIST R
> ULONG next_descriptor = 0;
> ULONG curr_bar = 0;
> ULONG prev_bar = 0;
> + BOOLEAN bar_done = FALSE;
>
> /*
> * ResourcesTranslated report MMIO BARs in the correct order, but their
> @@ -197,7 +198,8 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST Resources, WDFCMRESLIST R
> for (INT bar_index = 0; bar_index < PCI_MAX_BAR; bar_index++) {
> prev_bar = curr_bar;
> curr_bar = pci_config.u.type0.BaseAddresses[bar_index];
> - if (curr_bar == 0 || (prev_bar & PCI_TYPE_64BIT)) {
> + if (curr_bar == 0 || ((prev_bar & PCI_TYPE_64BIT) && (bar_done))) {
> + bar_done = FALSE;
> continue;
> }
>
> @@ -219,6 +221,7 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST Resources, WDFCMRESLIST R
> ctx->bar[bar_index].virt_addr = MmMapIoSpace(descriptor->u.Memory.Start,
> descriptor->u.Memory.Length,
> MmNonCached);
> + bar_done = TRUE;
> if (ctx->bar[bar_index].virt_addr == NULL) {
> status = STATUS_INSUFFICIENT_RESOURCES;
> goto end;
Hi Pallavi,
In the first place, it was wrong to always test `prev_bar & PCI_TYPE_64BIT`
because only the first BAR slot of a 64-bit BAR contains flags.
The current code has a state to track (curr_bar, prev_bar),
and the fix is complicating it even more without solving the root cause.
I suggest a simpler fix (not tested!)
that eliminates both the incorrectness and the state to maintain:
diff --git a/windows/netuio/netuio_dev.c b/windows/netuio/netuio_dev.c
index d4662b6..8761d31 100644
--- a/windows/netuio/netuio_dev.c
+++ b/windows/netuio/netuio_dev.c
@@ -273,8 +273,6 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST
Resources, WDFCMRESLIST R
PCM_PARTIAL_RESOURCE_DESCRIPTOR descriptor;
ULONG next_descriptor = 0;
- ULONG curr_bar = 0;
- ULONG prev_bar = 0;
/*
* ResourcesTranslated report MMIO BARs in the correct order, but their
@@ -298,9 +296,9 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST
Resources, WDFCMRESLIST R
* searching for the next MMIO resource each time.
*/
for (INT bar_index = 0; bar_index < PCI_MAX_BAR; bar_index++) {
- prev_bar = curr_bar;
- curr_bar = pci_config.u.type0.BaseAddresses[bar_index];
- if (curr_bar == 0 || (prev_bar & PCI_TYPE_64BIT)) {
+ ULONG bar_value = pci_config.u.type0.BaseAddresses[bar_index];
+
+ if (bar_value == 0) {
continue;
}
@@ -339,6 +337,11 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST
Resources, WDFCMRESLIST R
}
ctx->dpdk_hw[bar_index].mem.size = ctx->bar[bar_index].size;
+
+ // Skip the next BAR slot used by the current 64-bit address.
+ if (bar_value & PCI_TYPE_64BIT) {
+ bar_index++;
+ }
} // for bar_index
end_of_loop:
Hi Dmitry,
On 8/6/2022 2:01 PM, Dmitry Kozlyuk wrote:
> 2022-08-02 12:10 (UTC-0700), Pallavi Kadam:
>> For certain PCIe devices, BAR values are not continuous.
>> This patch maps all the BARs and avoids skipping the next BAR addresses.
>>
>> Fixes: e28aabd88279 ("windows/netuio: introduce NetUIO kernel driver")
>> Cc:navasile@microsoft.com
>>
>> Signed-off-by: Qiao Liu<qiao.liu@intel.com>
>> Signed-off-by: Pallavi Kadam<pallavi.kadam@intel.com>
>> ---
>> windows/netuio/netuio_dev.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/windows/netuio/netuio_dev.c b/windows/netuio/netuio_dev.c
>> index b2deb10..e2cac3e 100644
>> --- a/windows/netuio/netuio_dev.c
>> +++ b/windows/netuio/netuio_dev.c
>> @@ -172,6 +172,7 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST Resources, WDFCMRESLIST R
>> ULONG next_descriptor = 0;
>> ULONG curr_bar = 0;
>> ULONG prev_bar = 0;
>> + BOOLEAN bar_done = FALSE;
>>
>> /*
>> * ResourcesTranslated report MMIO BARs in the correct order, but their
>> @@ -197,7 +198,8 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST Resources, WDFCMRESLIST R
>> for (INT bar_index = 0; bar_index < PCI_MAX_BAR; bar_index++) {
>> prev_bar = curr_bar;
>> curr_bar = pci_config.u.type0.BaseAddresses[bar_index];
>> - if (curr_bar == 0 || (prev_bar & PCI_TYPE_64BIT)) {
>> + if (curr_bar == 0 || ((prev_bar & PCI_TYPE_64BIT) && (bar_done))) {
>> + bar_done = FALSE;
>> continue;
>> }
>>
>> @@ -219,6 +221,7 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST Resources, WDFCMRESLIST R
>> ctx->bar[bar_index].virt_addr = MmMapIoSpace(descriptor->u.Memory.Start,
>> descriptor->u.Memory.Length,
>> MmNonCached);
>> + bar_done = TRUE;
>> if (ctx->bar[bar_index].virt_addr == NULL) {
>> status = STATUS_INSUFFICIENT_RESOURCES;
>> goto end;
> Hi Pallavi,
>
> In the first place, it was wrong to always test `prev_bar & PCI_TYPE_64BIT`
> because only the first BAR slot of a 64-bit BAR contains flags.
> The current code has a state to track (curr_bar, prev_bar),
> and the fix is complicating it even more without solving the root cause.
> I suggest a simpler fix (not tested!)
> that eliminates both the incorrectness and the state to maintain:
Thank you for these changes. Will test the patch and let you know.
>
> diff --git a/windows/netuio/netuio_dev.c b/windows/netuio/netuio_dev.c
> index d4662b6..8761d31 100644
> --- a/windows/netuio/netuio_dev.c
> +++ b/windows/netuio/netuio_dev.c
> @@ -273,8 +273,6 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST
> Resources, WDFCMRESLIST R
>
> PCM_PARTIAL_RESOURCE_DESCRIPTOR descriptor;
> ULONG next_descriptor = 0;
> - ULONG curr_bar = 0;
> - ULONG prev_bar = 0;
>
> /*
> * ResourcesTranslated report MMIO BARs in the correct order, but their
> @@ -298,9 +296,9 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST
> Resources, WDFCMRESLIST R
> * searching for the next MMIO resource each time.
> */
> for (INT bar_index = 0; bar_index < PCI_MAX_BAR; bar_index++) {
> - prev_bar = curr_bar;
> - curr_bar = pci_config.u.type0.BaseAddresses[bar_index];
> - if (curr_bar == 0 || (prev_bar & PCI_TYPE_64BIT)) {
> + ULONG bar_value = pci_config.u.type0.BaseAddresses[bar_index];
> +
> + if (bar_value == 0) {
> continue;
> }
>
> @@ -339,6 +337,11 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST
> Resources, WDFCMRESLIST R
> }
>
> ctx->dpdk_hw[bar_index].mem.size = ctx->bar[bar_index].size;
> +
> + // Skip the next BAR slot used by the current 64-bit address.
> + if (bar_value & PCI_TYPE_64BIT) {
> + bar_index++;
> + }
> } // for bar_index
> end_of_loop:
Hi Dmitry,
On 8/6/2022 2:01 PM, Dmitry Kozlyuk wrote:
> 2022-08-02 12:10 (UTC-0700), Pallavi Kadam:
>> For certain PCIe devices, BAR values are not continuous.
>> This patch maps all the BARs and avoids skipping the next BAR addresses.
>>
>> Fixes: e28aabd88279 ("windows/netuio: introduce NetUIO kernel driver")
>> Cc:navasile@microsoft.com
>>
>> Signed-off-by: Qiao Liu<qiao.liu@intel.com>
>> Signed-off-by: Pallavi Kadam<pallavi.kadam@intel.com>
>> ---
>> windows/netuio/netuio_dev.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/windows/netuio/netuio_dev.c b/windows/netuio/netuio_dev.c
>> index b2deb10..e2cac3e 100644
>> --- a/windows/netuio/netuio_dev.c
>> +++ b/windows/netuio/netuio_dev.c
>> @@ -172,6 +172,7 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST Resources, WDFCMRESLIST R
>> ULONG next_descriptor = 0;
>> ULONG curr_bar = 0;
>> ULONG prev_bar = 0;
>> + BOOLEAN bar_done = FALSE;
>>
>> /*
>> * ResourcesTranslated report MMIO BARs in the correct order, but their
>> @@ -197,7 +198,8 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST Resources, WDFCMRESLIST R
>> for (INT bar_index = 0; bar_index < PCI_MAX_BAR; bar_index++) {
>> prev_bar = curr_bar;
>> curr_bar = pci_config.u.type0.BaseAddresses[bar_index];
>> - if (curr_bar == 0 || (prev_bar & PCI_TYPE_64BIT)) {
>> + if (curr_bar == 0 || ((prev_bar & PCI_TYPE_64BIT) && (bar_done))) {
>> + bar_done = FALSE;
>> continue;
>> }
>>
>> @@ -219,6 +221,7 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST Resources, WDFCMRESLIST R
>> ctx->bar[bar_index].virt_addr = MmMapIoSpace(descriptor->u.Memory.Start,
>> descriptor->u.Memory.Length,
>> MmNonCached);
>> + bar_done = TRUE;
>> if (ctx->bar[bar_index].virt_addr == NULL) {
>> status = STATUS_INSUFFICIENT_RESOURCES;
>> goto end;
> Hi Pallavi,
>
> In the first place, it was wrong to always test `prev_bar & PCI_TYPE_64BIT`
> because only the first BAR slot of a 64-bit BAR contains flags.
> The current code has a state to track (curr_bar, prev_bar),
> and the fix is complicating it even more without solving the root cause.
> I suggest a simpler fix (not tested!)
> that eliminates both the incorrectness and the state to maintain:
Thank you. This change works for us.
Please let me know if you would like to submit this change as a new patch or if I should include it as a v2 of this same patch.
>
> diff --git a/windows/netuio/netuio_dev.c b/windows/netuio/netuio_dev.c
> index d4662b6..8761d31 100644
> --- a/windows/netuio/netuio_dev.c
> +++ b/windows/netuio/netuio_dev.c
> @@ -273,8 +273,6 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST
> Resources, WDFCMRESLIST R
>
> PCM_PARTIAL_RESOURCE_DESCRIPTOR descriptor;
> ULONG next_descriptor = 0;
> - ULONG curr_bar = 0;
> - ULONG prev_bar = 0;
>
> /*
> * ResourcesTranslated report MMIO BARs in the correct order, but their
> @@ -298,9 +296,9 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST
> Resources, WDFCMRESLIST R
> * searching for the next MMIO resource each time.
> */
> for (INT bar_index = 0; bar_index < PCI_MAX_BAR; bar_index++) {
> - prev_bar = curr_bar;
> - curr_bar = pci_config.u.type0.BaseAddresses[bar_index];
> - if (curr_bar == 0 || (prev_bar & PCI_TYPE_64BIT)) {
> + ULONG bar_value = pci_config.u.type0.BaseAddresses[bar_index];
> +
> + if (bar_value == 0) {
> continue;
> }
>
> @@ -339,6 +337,11 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST
> Resources, WDFCMRESLIST R
> }
>
> ctx->dpdk_hw[bar_index].mem.size = ctx->bar[bar_index].size;
> +
> + // Skip the next BAR slot used by the current 64-bit address.
> + if (bar_value & PCI_TYPE_64BIT) {
> + bar_index++;
> + }
> } // for bar_index
> end_of_loop:
2022-08-08 17:33 (UTC-0700), Kadam, Pallavi:
[...]
> > Hi Pallavi,
> >
> > In the first place, it was wrong to always test `prev_bar & PCI_TYPE_64BIT`
> > because only the first BAR slot of a 64-bit BAR contains flags.
> > The current code has a state to track (curr_bar, prev_bar),
> > and the fix is complicating it even more without solving the root cause.
> > I suggest a simpler fix (not tested!)
> > that eliminates both the incorrectness and the state to maintain:
>
> Thank you. This change works for us.
>
> Please let me know if you would like to submit this change as a new patch or if I should include it as a v2 of this same patch.
Please send v2.
You can include my Signed-off-by.
You might also update the commit message and capitalize "BAR" in the title.
On 8/9/2022 2:15 AM, Dmitry Kozlyuk wrote:
> 2022-08-08 17:33 (UTC-0700), Kadam, Pallavi:
> [...]
>>> Hi Pallavi,
>>>
>>> In the first place, it was wrong to always test `prev_bar & PCI_TYPE_64BIT`
>>> because only the first BAR slot of a 64-bit BAR contains flags.
>>> The current code has a state to track (curr_bar, prev_bar),
>>> and the fix is complicating it even more without solving the root cause.
>>> I suggest a simpler fix (not tested!)
>>> that eliminates both the incorrectness and the state to maintain:
>> Thank you. This change works for us.
>>
>> Please let me know if you would like to submit this change as a new patch or if I should include it as a v2 of this same patch.
> Please send v2.
> You can include my Signed-off-by.
> You might also update the commit message and capitalize "BAR" in the title.
Thanks, Dmitry. Have sent v2.
@@ -172,6 +172,7 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST Resources, WDFCMRESLIST R
ULONG next_descriptor = 0;
ULONG curr_bar = 0;
ULONG prev_bar = 0;
+ BOOLEAN bar_done = FALSE;
/*
* ResourcesTranslated report MMIO BARs in the correct order, but their
@@ -197,7 +198,8 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST Resources, WDFCMRESLIST R
for (INT bar_index = 0; bar_index < PCI_MAX_BAR; bar_index++) {
prev_bar = curr_bar;
curr_bar = pci_config.u.type0.BaseAddresses[bar_index];
- if (curr_bar == 0 || (prev_bar & PCI_TYPE_64BIT)) {
+ if (curr_bar == 0 || ((prev_bar & PCI_TYPE_64BIT) && (bar_done))) {
+ bar_done = FALSE;
continue;
}
@@ -219,6 +221,7 @@ netuio_map_hw_resources(WDFDEVICE Device, WDFCMRESLIST Resources, WDFCMRESLIST R
ctx->bar[bar_index].virt_addr = MmMapIoSpace(descriptor->u.Memory.Start,
descriptor->u.Memory.Length,
MmNonCached);
+ bar_done = TRUE;
if (ctx->bar[bar_index].virt_addr == NULL) {
status = STATUS_INSUFFICIENT_RESOURCES;
goto end;