Re: [PATCH v4 3/5] vpci: allow queueing of mapping operations

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2026-10-15 10:08, Mykyta Poturai wrote:
> From: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
>
> Introduce vPCI BAR mapping task queue. Store information needed to
> map/unmap BARs in struct vpci_map_task. Allow queueing of BAR map/unmap
> operations in a list, thus making it possible to perform multiple p2m
> operations associated with single PCI config space register access.
>
> The rangesets describing the ranges to be (un)mapped move from struct
> vpci_bar into the task. They are only live for the duration of the
> operation, and a single per-device set cannot describe more than one
"per-device rangeset"?

> queued operation. Allocate them along with the task and free them once
> the task has been processed.
>
> This is preparatory work for further changes that need to perform
> multiple unmap/map operations before returning to guest.
>
> At the moment, only a single operation will be queued. However, when
> multiple operations are queued, there is a check in modify_bars() to
> skip BARs already in the requested state that will no longer be
> accurate. Remove this check in preparation of upcoming changes.
>
> Amends: 622bdd962822 ("vpci/header: handle p2m range sets per BAR")
> Message-ID: <20260406191203.97662-3-stewart.hildebrand@xxxxxxx>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> @@ -280,12 +321,48 @@ static int __init apply_map(struct domain *d, const 
> struct pci_dev *pdev,
> -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;
> +    unsigned int i;
> +
> +    task = xvzalloc(struct vpci_map_task);
> +
Nit: No newline needed here.

> +    if ( !task )
> +        return NULL;
> +
> +    for ( i = 0; i < ARRAY_SIZE(task->bars); i++ )
> +    {
> +        if ( !MAPPABLE_BAR(&pdev->vpci->header.bars[i]) )
> +            continue;
> +
> +        task->bars[i].mem = rangeset_new(pdev->domain, NULL,
> +                                         RANGESETF_no_print);
> +
Nit: Extra newline.

> +        if ( !task->bars[i].mem )
> +        {
> +            destroy_map_task(task);
> +            return NULL;
> +        }
> +
> +        task->bars[i].addr = pdev->vpci->header.bars[i].addr;
> +        task->bars[i].guest_addr = pdev->vpci->header.bars[i].guest_addr;
> +    }
> +
> +    task->cmd = cmd;
> +    task->rom_only = rom_only;
> +    task->pdev = pdev;
> +
> +    return task;
> +}
> +
> +static void defer_map(struct vpci_map_task *task)
>   {
>       struct vcpu *curr = current;
>   
> @@ -312,11 +388,17 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t 
> cmd, bool rom_only)
>       struct pci_dev *tmp;
>       const struct domain *d;
>       const struct vpci_msix *msix = pdev->vpci->msix;
> +    struct vpci_map_task *task;
>       unsigned int i, j;
>       int rc;
>   
>       ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
>   
> +    task = alloc_map_task(pdev, cmd, rom_only);
> +
Nit: Extra newline

> +    if ( !task )
> +        return -ENOMEM;
> +
>       /*
>        * Create a rangeset per BAR that represents the current device memory
>        * region and compare it against all the currently active BAR memory
> @@ -343,9 +425,7 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t 
> cmd, bool rom_only)
>   
>           if ( !MAPPABLE_BAR(bar) ||
>                (rom_only ? bar->type != VPCI_BAR_ROM
> -                       : (bar->type == VPCI_BAR_ROM && !header->rom_enabled)) 
> ||
> -             /* Skip BARs already in the requested state. */
> -             bar->enabled == !!(cmd & PCI_COMMAND_MEMORY) )
This is the removed check mentioned in the commit message?  And the 
reason is that a queue of operation 1 unmap, operation 2 map needs to be 
processed sequentially, but cmd won't be changing?
> +                       : (bar->type == VPCI_BAR_ROM && !header->rom_enabled)) )
>               continue;
>   
>           if ( !pci_check_bar(pdev, _mfn(start), _mfn(end)) )
> @@ -152,14 +153,24 @@ struct vpci {
>   #endif
>   };
>   
> -struct vpci_vcpu {
> +#ifdef __XEN__
> +struct vpci_map_task {
>       /* Per-vcpu structure to store state while {un}mapping of PCI BARs. */
> -    const struct pci_dev *pdev;
> +    struct list_head next;
> +    struct vpci_bar_map {
> +        uint64_t addr;
> +        uint64_t guest_addr;
> +        struct rangeset *mem;
> +    } bars[ARRAY_SIZE(((struct vpci_header *)NULL)->bars)];
>       uint16_t cmd;
>       bool rom_only : 1;
> +    const struct pci_dev *pdev;
Is there a reason to move this pointer?  You introduce a hole in the 
structure before it.
Thanks,
Jason