Re: [PATCH v16 5/5] xen/arm: account IO handlers for emulated PCI MSI-X

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, May 22, 2024 at 06:59:24PM -0400, Stewart Hildebrand wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> At the moment, we always allocate an extra 16 slots for IO handlers
> (see MAX_IO_HANDLER). So while adding IO trap handlers for the emulated
> MSI-X registers we need to explicitly tell that we have additional IO
> handlers, so those are accounted.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Acked-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> This depends on a constant defined in ("vpci: add initial support for
> virtual PCI bus topology"), so cannot be committed without the
> dependency.
> 
> Since v5:
> - optimize with IS_ENABLED(CONFIG_HAS_PCI_MSI) since VPCI_MAX_VIRT_DEV is
>   defined unconditionally
> New in v5
> ---
>  xen/arch/arm/vpci.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> index 516933bebfb3..4779bbfa9be3 100644
> --- a/xen/arch/arm/vpci.c
> +++ b/xen/arch/arm/vpci.c
> @@ -132,6 +132,8 @@ static int vpci_get_num_handlers_cb(struct domain *d,
>  
>  unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d)
>  {
> +    unsigned int count;
> +
>      if ( !has_vpci(d) )
>          return 0;
>  
> @@ -152,7 +154,17 @@ unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d)
>       * For guests each host bridge requires one region to cover the
>       * configuration space. At the moment, we only expose a single host bridge.
>       */
> -    return 1;
> +    count = 1;
> +
> +    /*
> +     * There's a single MSI-X MMIO handler that deals with both PBA
> +     * and MSI-X tables per each PCI device being passed through.
> +     * Maximum number of emulated virtual devices is VPCI_MAX_VIRT_DEV.
> +     */
> +    if ( IS_ENABLED(CONFIG_HAS_PCI_MSI) )
> +        count += VPCI_MAX_VIRT_DEV;

I think this was already raised in a previous version, at some point
you will need to consider making this a linker list or similar.  The
array is also not very helpful, as you still need to iterate over the
slots in order to find which handler should dispatch the access.

(Not that I oppose to this patch, but the handlers array cannot be
expanded indefinitely).

Thanks, Roger.