On Thu, Sep 30, 2021 at 10:52:19AM +0300, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
>
> 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_andrushche...@epam.com>
>
> ---
> 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.
