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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

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, Roger.