Re: [PATCH v2 07/11] vpci/header: program p2m with guest BAR view

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Oleksandr,

On 23.09.2021 14:54, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> Take into account guest's BAR view and program its p2m accordingly:
> gfn is guest's view of the BAR and mfn is the physical BAR value as set
> up by the host bridge in the hardware domain.
> This way hardware doamin sees physical BAR values and guest sees
> emulated ones.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> ---
> Since v1:
>  - s/MSI/MSI-X in comments
> ---
>  xen/drivers/vpci/header.c | 35 +++++++++++++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 9c603d26d302..bdd18599b205 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -30,6 +30,10 @@
>  
>  struct map_data {
>      struct domain *d;
> +    /* Start address of the BAR as seen by the guest. */
> +    gfn_t start_gfn;
> +    /* Physical start address of the BAR. */
> +    mfn_t start_mfn;
>      bool map;
>  };
>  
> @@ -37,12 +41,28 @@ static int map_range(unsigned long s, unsigned long e, 
> void *data,
>                       unsigned long *c)
>  {
>      const struct map_data *map = data;
> +    gfn_t start_gfn;
>      int rc;
>  
>      for ( ; ; )
>      {
>          unsigned long size = e - s + 1;
>  
> +        /*
> +         * Any BAR may have holes in its memory we want to map, e.g.
> +         * we don't want to map MSI-X regions which may be a part of that 
> BAR,
> +         * e.g. when a single BAR is used for both MMIO and MSI-X.
> +         * In this case MSI-X regions are subtracted from the mapping, but
> +         * map->start_gfn still points to the very beginning of the BAR.
> +         * So if there is a hole present then we need to adjust start_gfn
> +         * to reflect the fact of that substraction.
> +         */
> +        start_gfn = gfn_add(map->start_gfn, s - mfn_x(map->start_mfn));
> +
> +        printk(XENLOG_G_DEBUG
> +               "%smap [%lx, %lx] -> %#"PRI_gfn" for d%d\n",
> +               map->map ? "" : "un", s, e, gfn_x(start_gfn),
> +               map->d->domain_id);
>          /*
>           * ARM TODOs:
>           * - On ARM whether the memory is prefetchable or not should be 
> passed
> @@ -52,8 +72,10 @@ static int map_range(unsigned long s, unsigned long e, 
> void *data,
>           * - {un}map_mmio_regions doesn't support preemption.
>           */
>  
> -        rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, _mfn(s))
> -                      : unmap_mmio_regions(map->d, _gfn(s), size, _mfn(s));
> +        rc = map->map ? map_mmio_regions(map->d, start_gfn,
> +                                         size, _mfn(s))
> +                      : unmap_mmio_regions(map->d, start_gfn,
> +                                           size, _mfn(s));
>          if ( rc == 0 )
>          {
>              *c += size;
> @@ -69,6 +91,7 @@ static int map_range(unsigned long s, unsigned long e, void 
> *data,
>          ASSERT(rc < size);
>          *c += rc;
>          s += rc;
> +        gfn_add(map->start_gfn, rc);
>          if ( general_preempt_check() )
>                  return -ERESTART;
>      }
> @@ -149,6 +172,10 @@ bool vpci_process_pending(struct vcpu *v)
>              if ( !bar->mem )
>                  continue;
>  
> +            data.start_gfn = is_hardware_domain(v->vpci.pdev->domain) ?
> +                _gfn(PFN_DOWN(bar->addr)) :
> +                _gfn(PFN_DOWN(bar->guest_addr));
I believe this would look better with the following alignment:
data.start_gfn = is_hardware_domain(v->vpci.pdev->domain)
                 ? _gfn(PFN_DOWN(bar->addr))
                 : _gfn(PFN_DOWN(bar->guest_addr));
> +            data.start_mfn = _mfn(PFN_DOWN(bar->addr));
>              rc = rangeset_consume_ranges(bar->mem, map_range, &data);
>  
>              if ( rc == -ERESTART )
> @@ -194,6 +221,10 @@ static int __init apply_map(struct domain *d, const 
> struct pci_dev *pdev,
>          if ( !bar->mem )
>              continue;
>  
> +        data.start_gfn = is_hardware_domain(d) ?
> +            _gfn(PFN_DOWN(bar->addr)) :
> +            _gfn(PFN_DOWN(bar->guest_addr));
This one also.
> +        data.start_mfn = _mfn(PFN_DOWN(bar->addr));
>          while ( (rc = rangeset_consume_ranges(bar->mem, map_range,
>                                                &data)) == -ERESTART )
>              process_pending_softirqs();
> 

Cheers,
Michal