Re: [PATCH v2 1/3] vpci: allow queueing of mapping operations

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 7/25/25 03:58, Roger Pau Monné wrote:
> On Thu, Jul 24, 2025 at 06:44:32PM +0200, Roger Pau Monné wrote:
>> On Wed, Jul 23, 2025 at 12:37:41PM -0400, Stewart Hildebrand wrote:
>>> @@ -283,7 +297,48 @@ static int __init apply_map(struct domain *d, const 
>>> struct pci_dev *pdev,
>>>      return rc;
>>>  }
>>>  
>>> -static void defer_map(const struct pci_dev *pdev, uint16_t cmd, bool 
>>> rom_only)
>>> +static struct vpci_map_task *alloc_map_task(const struct pci_dev *pdev,
>>> +                                            uint16_t cmd, bool rom_only)
>>> +{
>>> +    struct vpci_map_task *task = xzalloc(struct vpci_map_task);
>>
>> xvzalloc() preferably.
>>
>> This however introduces run-time allocations as a result of guest
>> actions, which is not ideal IMO.  It would be preferable to do those
>> allocations as part of the header initialization, and re-use them.
> 
> I've been thinking over this, as I've realized that while commenting
> on it, I didn't provide any alternatives.
> 
> The usage of rangesets to figure out the regions to map is already not
> optimal, as adding/removing from a rangeset can lead to memory
> allocations.  It would be good if we could create rangesets with a
> pre-allocated number of ranges (iow: a pool of struct ranges), but
> that's for another patchset.  I think Jan already commented on this
> aspect long time ago.

+1

> I'm considering whether to allocate the deferred mapping structures
> per-vCPU instead of per-device.  That would for example mean moving
> the current vpci_bar->mem rangeset so it's allocated in vpci_vcpu
> struct instead.  The point would be to not have the rangesets per
> device (because there can be a lot of devices, specially for the
> hardware domain), but instead have those per-vCPU.  This should work
> because a vCPU can only queue a single vPCI operation, from a single
> device.
> 
> It should then be possible to allocate the deferred mapping structures
> at vCPU creation.  I also ponder if we really need a linked list to
> queue them; AFAIK there can only ever be an unmapping and a mapping
> operation pending (so 2 operations at most).  Hence we could use a
> more "fixed" structure like an array.  For example in struct vpci_vcpu
> you could introduce a struct vpci_map_task task[2] field?
> 
> Sorry, I know this is not a minor change to request.  It shouldn't
> change the overall logic much, but it would inevitably affect the
> code.  Let me know what you think.

Thanks for the feedback and suggestion. Yeah, I'll give this a try.
Here's roughly what I'm thinking so far. I'll keep playing with it.

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 5241a1629eeb..942c9fe7d364 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -387,6 +387,16 @@ static int vmtrace_alloc_buffer(struct vcpu *v)
  */
 static int vcpu_teardown(struct vcpu *v)
 {
+#ifdef CONFIG_HAS_VPCI
+    for ( unsigned int i = 0; i < ARRAY_SIZE(v->vpci.task); i++ )
+    {
+        struct vpci_map_task *task = &v->vpci.task[i];
+
+        for ( unsigned int j = 0; j < ARRAY_SIZE(task->bars); j++ )
+            rangeset_destroy(task->bars[j].mem);
+    }
+#endif
+
     vmtrace_free_buffer(v);
 
     return 0;
@@ -467,6 +477,26 @@ struct vcpu *vcpu_create(struct domain *d, unsigned int 
vcpu_id)
         d->vcpu[prev_id]->next_in_list = v;
     }
 
+#ifdef CONFIG_HAS_VPCI
+    for ( unsigned int i = 0; i < ARRAY_SIZE(v->vpci.task); i++ )
+    {
+        struct vpci_map_task *task = &v->vpci.task[i];
+
+        for ( unsigned int j = 0; j < ARRAY_SIZE(task->bars); j++ )
+        {
+            struct vpci_bar_map *bar = &task->bars[j];
+            char str[32];
+
+            snprintf(str, sizeof(str), "PCI map vcpu %u task %u BAR %u", 
vcpu_id, i, j);
+
+            bar->mem = rangeset_new(v->domain, str, RANGESETF_no_print);
+
+            if ( !bar->mem )
+                goto fail_sched;
+        }
+    }
+#endif
+
     /* Must be called after making new vcpu visible to for_each_vcpu(). */
     vcpu_check_shutdown(v);
 
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 17cfecb0aabf..afe78b00ffc9 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -116,7 +116,6 @@ struct vpci {
             uint64_t guest_addr;
             uint64_t size;
             uint64_t resizable_sizes;
-            struct rangeset *mem;
             enum {
                 VPCI_BAR_EMPTY,
                 VPCI_BAR_IO,
@@ -207,14 +206,23 @@ struct vpci {
 #endif
 };
 
+#ifdef __XEN__
 struct vpci_vcpu {
     /* Per-vcpu structure to store state while {un}mapping of PCI BARs. */
     const struct pci_dev *pdev;
-    uint16_t cmd;
-    bool rom_only : 1;
+    struct domain *domain;
+    unsigned int nr_pending_ops;
+    struct vpci_map_task {
+        struct vpci_bar_map {
+            uint64_t addr;
+            uint64_t guest_addr;
+            struct rangeset *mem;
+        } bars[PCI_HEADER_NORMAL_NR_BARS + 1];
+        uint16_t cmd;
+        bool rom_only : 1;
+    } task[2];
 };
 
-#ifdef __XEN__
 void vpci_dump_msi(void);
 
 /* Make sure there's a hole in the p2m for the MSIX mmio areas. */