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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.11.2021 07:56, 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.

I'm sorry to be picky, but I don't think host bridges set up BARs. What
I think you mean is "as set up by the PCI bus driver in the hardware
domain". Of course this then still leaves out the case of firmware
doing so, and hence the dom0less case.

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

As of the previous patch you process this on a per-BAR basis. Why don't
you simply put const struct vpci_bar * here? This would e.g. avoid the
need to keep in sync the identical expressions in vpci_process_pending()
and apply_map().

> @@ -37,12 +41,24 @@ 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;

Imo this wants to move into the more narrow scope, ...

>      int rc;
>  
>      for ( ; ; )
>      {
>          unsigned long size = e - s + 1;
>  
> +        /*
> +         * 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.
> +         */
> +        start_gfn = gfn_add(map->start_gfn, s - mfn_x(map->start_mfn));

... allowing (in principle) for this to become its initializer.

Jan