Re: [PATCH v5 08/14] vpci/header: program p2m with guest BAR view

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

>>> +        gdprintk(XENLOG_G_DEBUG,
>>> +                 "%smap [%lx, %lx] -> %#"PRI_gfn" for %pd\n",
>>> +                 map->map ? "" : "un", s, e, gfn_x(start_gfn),
>>> +                 map->d);
>> That's too chatty IMO, I could be fine with printing something along
>> this lines from modify_bars, but not here because that function can be
>> preempted and called multiple times.
> Ok, will move to modify_bars as these prints are really helpful for debug
I tried to implement the same, but now in init_bars:

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 667c04cee3ae..92407e617609 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -57,10 +57,6 @@ static int map_range(unsigned long s, unsigned long e, void 
*data,
           */
          start_gfn = gfn_add(start_gfn, s - mfn_x(start_mfn));

-        gdprintk(XENLOG_G_DEBUG,
-                 "%smap [%lx, %lx] -> %#"PRI_gfn" for %pd\n",
-                 map->map ? "" : "un", s, e, gfn_x(start_gfn),
-                 map->d);
          /*
           * ARM TODOs:
           * - On ARM whether the memory is prefetchable or not should be passed
@@ -258,6 +254,28 @@ static void defer_map(struct domain *d, struct pci_dev 
*pdev,
      raise_softirq(SCHEDULE_SOFTIRQ);
  }

+static int print_range(unsigned long s, unsigned long e, void *data)
+{
+    const struct map_data *map = data;
+
+    for ( ; ; )
+    {
+        gfn_t start_gfn = _gfn(PFN_DOWN(is_hardware_domain(map->d)
+                                        ? map->bar->addr
+                                        : map->bar->guest_reg));
+        mfn_t start_mfn = _mfn(PFN_DOWN(map->bar->addr));
+
+        start_gfn = gfn_add(start_gfn, s - mfn_x(start_mfn));
+
+        gdprintk(XENLOG_G_DEBUG,
+                 "%smap [%lx, %lx] -> %#"PRI_gfn" for %pd\n",
+                 map->map ? "" : "un", s, e, gfn_x(start_gfn),
+                 map->d);
+    }
+
+    return 0;
+}
+
  static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool 
rom_only)
  {
      struct vpci_header *header = &pdev->vpci->header;
@@ -423,7 +441,25 @@ static int modify_bars(const struct pci_dev *pdev, 
uint16_t cmd, bool rom_only)
      if ( !map_pending )
          pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
      else
+    {
+        struct map_data data = {
+            .d = pdev->domain,
+            .map = cmd & PCI_COMMAND_MEMORY,
+        };
+
+        for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+        {
+            const struct vpci_bar *bar = &header->bars[i];
+
+            if ( rangeset_is_empty(bar->mem) )
+                continue;
+
+            data.bar = bar;
+            rc = rangeset_report_ranges(bar->mem, 0, ~0ul, print_range, &data);
+        }
+
          defer_map(dev->domain, dev, cmd, rom_only);
+    }

      return 0;


To me, to implement a single DEBUG print, it is a bit an overkill.
I do understand your concerns that "that function can be
preempted and called multiple times", but taking look at the code
above I think we can accept that for DEBUG builds.

Could you please let me know if I:
1. Still need to implement (the patch above)
2. Drop DEBUG prints (those are really useful while debugging)
3. Leave the print where it was in map_range

Thank you in advance,
Oleksandr