Re: [PATCH v3 03/11] vpci/header: Move register assignments from init_bars

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

On Thu, Sep 30, 2021 at 10:52:15AM +0300, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> This is in preparation for dynamic assignment of the vPCI register
> handlers depending on the domain: hwdom or guest.
> The need for this step is that it is easier to have all related functionality
> put at one place. When the subsequent patches add decisions on which
> handlers to install, e.g. hwdom or guest handlers, then this is easily
> achievable.

Won't it be possible to select the handlers to install in init_bars
itself?

Splitting it like that means you need to iterate over the numbers of
BARs twice (one in add_bar_handlers and one in init_bars), which makes
it more likely to introduce errors or divergences.

Decoupling the filling of vpci_bar data with setting the handlers
seems slightly confusing.

> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> ---
> Since v1:
>  - constify struct pci_dev where possible
>  - extend patch description
> ---
>  xen/drivers/vpci/header.c | 83 ++++++++++++++++++++++++++-------------
>  1 file changed, 56 insertions(+), 27 deletions(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index f8cd55e7c024..3d571356397a 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -445,6 +445,55 @@ static void rom_write(const struct pci_dev *pdev, 
> unsigned int reg,
>          rom->addr = val & PCI_ROM_ADDRESS_MASK;
>  }
>  
> +static int add_bar_handlers(const struct pci_dev *pdev)

Making this const is again misleading IMO, as you end up modifying
fields inside the pdev, you get away with it because vpci data is
stored in a pointer.

> +{
> +    unsigned int i;
> +    struct vpci_header *header = &pdev->vpci->header;
> +    struct vpci_bar *bars = header->bars;
> +    int rc;
> +
> +    /* Setup a handler for the command register. */
> +    rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write, 
> PCI_COMMAND,
> +                           2, header);
> +    if ( rc )
> +        return rc;
> +
> +    if ( pdev->ignore_bars )
> +        return 0;

You can join both ifs above:

if ( rc || pdev->ignore_bars )
    return rc;

> +
> +    for ( i = 0; i < PCI_HEADER_NORMAL_NR_BARS + 1; i++ )

init_bars deals with both TYPE_NORMAL and TYPE_BRIDGE classes, yet you
seem to unconditionally assume PCI_HEADER_NORMAL_NR_BARS here (even
when below you take into account the different ROM BAR position).

> +    {
> +        if ( (bars[i].type == VPCI_BAR_IO) || (bars[i].type == 
> VPCI_BAR_EMPTY) )
> +            continue;
> +
> +        if ( bars[i].type == VPCI_BAR_ROM )
> +        {
> +            unsigned int rom_reg;
> +            uint8_t header_type = pci_conf_read8(pdev->sbdf,
> +                                                 PCI_HEADER_TYPE) & 0x7f;

Missing newline, and unsigned int preferably for header_type.

> +            if ( header_type == PCI_HEADER_TYPE_NORMAL )
> +                rom_reg = PCI_ROM_ADDRESS;
> +            else
> +                rom_reg = PCI_ROM_ADDRESS1;
> +            rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write,
> +                                   rom_reg, 4, &bars[i]);
> +            if ( rc )
> +                return rc;
> +        }
> +        else
> +        {
> +            uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;

unsigned int please, we try to avoid using explicitly sized types
unless strictly necessary (ie: when dealing with hardware values for
example).

> +
> +            /* This is either VPCI_BAR_MEM32 or VPCI_BAR_MEM64_{LO|HI}. */
> +            rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, 
> reg,
> +                                   4, &bars[i]);
> +            if ( rc )
> +                return rc;
> +        }
> +    }
> +    return 0;
> +}
> +
>  static int init_bars(struct pci_dev *pdev)
>  {
>      uint16_t cmd;
> @@ -470,14 +519,8 @@ static int init_bars(struct pci_dev *pdev)
>          return -EOPNOTSUPP;
>      }
>  
> -    /* Setup a handler for the command register. */
> -    rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write, 
> PCI_COMMAND,
> -                           2, header);
> -    if ( rc )
> -        return rc;

I don't think you need to move the handler for the command register
inside add_bar_handlers: for once it makes the function name not
reflect what it actually does (as it then deals with both BARs and the
command register), and it would also prevent you from having to call
add_bar_handlers in if ignore_bars is set.

Thanks, Roger.