Re: [PATCH 3/9] vpci/header: Move register assignments from init_bars

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.09.2021 12:08, 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.

I guess why exactly this is going to help is going to be seen in
subsequent patches. To aid review (i.e. to not force reviewers to
peek ahead) it would imo be helpful if you outlined how the result
is going to help. After all ...

> --- 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(struct pci_dev *pdev)

... this function name, for example, isn't Dom0-specific, so one
might expect the function body to gain conditionals. Yet then the
question is why these conditionals can't live in the original
function.

> +{
> +    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;
> +
> +    for ( i = 0; i < PCI_HEADER_NORMAL_NR_BARS + 1; i++ )
> +    {
> +        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;
> +            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;

I'm not the maintainer of this code, but if I was I'd ask for this and ...

> +        }
> +        else
> +        {
> +            uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;
> +
> +            /* 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;

... this to be moved ...

> +        }

... here to reduce redundancy.

> @@ -553,11 +580,13 @@ static int init_bars(struct pci_dev *pdev)
>          rom->addr = addr;
>          header->rom_enabled = pci_conf_read32(pdev->sbdf, rom_reg) &
>                                PCI_ROM_ADDRESS_ENABLE;
> +    }
>  
> -        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write, 
> rom_reg,
> -                               4, rom);
> -        if ( rc )
> -            rom->type = VPCI_BAR_EMPTY;
> +    rc = add_bar_handlers(pdev);
> +    if ( rc )
> +    {
> +        pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> +        return rc;
>      }

Seeing this moved (hence perhaps more a question to Roger than to
you) restoring of the command register - why is it that the error
path(s) here care(s) about restoring this, but ...

>      return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false) : 0;

... ones in modify_bars() (and downwards) don't? I was wondering
whether the restore could actually be done prior to the two calls
(or, in the original code, the one call), or perhaps even right
after the last call to pci_size_mem_bar(). At the very least the
comment further up suggests memory decode only gets disabled for
sizing BARs, which we're done with at this point.

Jan