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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.10.2021 17:38, Roger Pau Monné wrote:
> On Thu, Oct 07, 2021 at 09:22:36AM +0200, Jan Beulich wrote:
>> On 04.10.2021 07:58, Oleksandr Andrushchenko wrote:
>>>
>>>
>>> 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?
>>
>> Marginally (plus you'd need to prove that pdev->domain can never be NULL
>> when making it here).
> 
> I think it would an anomaly to try to setup vPCI handlers for a device
> without pdev->domain being set. I'm quite sure other vPCI code already
> relies on pdev->domain being set.

Quite likely, and my point wasn't to request dealing with the NULL case
by adding a check here. I really meant "prove", mainly recalling that
another patch (in another related series?) altered code around the
setting of pdev->domain in pci_add_device(). It would need to be assured
that whatever goes on there guarantees pdev->domain to have got set.

Jan