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

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

On Thu, Sep 30, 2021 at 10:52:19AM +0300, 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 v2:
> - improve readability for data.start_gfn and restructure ?: construct
> Since v1:
>  - s/MSI/MSI-X in comments
> ---
>  xen/drivers/vpci/header.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 9c603d26d302..f23c956cde6c 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.

IMO there are too many 'e.g.' here.

> +         * 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.
> +         */

I would simply the comment a bit:

/*
 * Ranges to be mapped don't always start at the BAR start address, as
 * there can be holes or partially consumed ranges. Account for the
 * offset of the current address from the BAR start.
 */

Apart from MSI-X related holes on x86 at least we support preemption
here, which means a range could be partially mapped before yielding.

> +        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);

I think increasing map->start_gfn is wrong here, as it would get out
of sync with map->start_mfn then, and the calculations done to obtain
start_gfn would then be wrong.

>          if ( general_preempt_check() )
>                  return -ERESTART;
>      }
> @@ -149,6 +172,10 @@ bool vpci_process_pending(struct vcpu *v)
>              if ( !bar->mem )
>                  continue;
>  
> +            data.start_gfn =
> +                 _gfn(PFN_DOWN(is_hardware_domain(v->vpci.pdev->domain)

You can just use v->domain here.

> +                               ? bar->addr : bar->guest_addr));

I would place the '?' in the line above, but that's just my taste.

Thanks, Roger.