Re: [dpdk-kmods] windows/netuio: fix bar parsing

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

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: