Re: [PATCH v3 04/11] vpci/header: Add and remove register handlers dynamically

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 01.10.21 16:26, Jan Beulich wrote:
> On 30.09.2021 09:52, Oleksandr Andrushchenko wrote:
>> @@ -445,14 +456,25 @@ 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)
>> +static void guest_rom_write(const struct pci_dev *pdev, unsigned int reg,
>> +                            uint32_t val, void *data)
>> +{
>> +}
>> +
>> +static uint32_t guest_rom_read(const struct pci_dev *pdev, unsigned int reg,
>> +                               void *data)
>> +{
>> +    return 0xffffffff;
>> +}
>> +
>> +static int add_bar_handlers(const struct pci_dev *pdev, bool is_hwdom)
> I remain unconvinced that this boolean is the best way to go here,
I can remove "bool is_hwdom" and have the checks like:

static int add_bar_handlers(const struct pci_dev *pdev)
{
...
     if ( is_hardware_domain(pdev->domain) )
         rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write,
                                PCI_COMMAND, 2, header);
     else
         rc = vpci_add_register(pdev->vpci, vpci_hw_read16, guest_cmd_write,
                                PCI_COMMAND, 2, header);
Is this going to be better?
>   but
> I'll leave the decision there to Roger. Just a couple of nits:
>
>> @@ -593,6 +625,30 @@ static int init_bars(struct pci_dev *pdev)
>>   }
>>   REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
>>   
>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> +int vpci_bar_add_handlers(const struct domain *d, const struct pci_dev 
>> *pdev)
>> +{
>> +    int rc;
>> +
>> +    /* Remove previously added registers. */
>> +    vpci_remove_device_registers(pdev);
>> +
>> +    rc = add_bar_handlers(pdev, is_hardware_domain(d));
>> +    if ( rc )
>> +        gdprintk(XENLOG_ERR,
>> +                 "%pp: failed to add BAR handlers for dom%pd: %d\n",
> Only %pd please, as that already expands to d<num>.
Good catch, thank you!
>
>> +                 &pdev->sbdf, d, rc);
>> +    return rc;
> Blank line please ahead of the main return statement of a function.
Will add
>
> Jan
>
Thank you,
Oleksandr