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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 06.09.21 16:53, Jan Beulich wrote:
> 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.

Sure, will do next time. The need for this step is that is it easier to have

all related functionality (BARs here) put at one place and when the subsequent

patches add decisions on which handlers to install, e.g. hwdom or guest 
handlers,

this function is extended to accept a one more parameter, is_hwdom, and all

the assignment logic is put here. Of course I could have all the "if 
(is_hwdom)"'s

put at the original location, but dedicated function looked cleaner to me.

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

Answered above. I think it makes code cleaner and easier for modification

as handlers' assignment for BARs becomes grouped as it is done for MSI/MSI-X.


>
>> +{
>> +    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.

For all the above: what this patch does is a pure code move.

I had no intention to alter it in any other way rather than that.

If you think the code needs to be functionally modified I think this

deserves a dedicated work to be submitted, but IMO this patch

shouldn't touch anything.

>
> Jan
>
Thank you,

Oleksandr