Re: [PATCH v3 02/11] vpci: Add hooks for PCI device assign/de-assign

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.09.2021 10:45, Oleksandr Andrushchenko wrote:
> On 30.09.21 11:21, Jan Beulich wrote:
>> On 30.09.2021 09:52, Oleksandr Andrushchenko wrote:
>>> @@ -1429,6 +1433,11 @@ static int assign_device(struct domain *d, u16 seg, 
>>> u8 bus, u8 devfn, u32 flag)
>>>           rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), 
>>> flag);
>>>       }
>>>   
>>> +    if ( rc )
>>> +        goto done;
>>  From all I can tell this is dead code.
> Before the change rc was set in the loop. And then we fall through
> to the "done" label. I do agree that the way this code is done the
> value of that rc will only reflect the last assignment done in the loop,
> but with my change I didn't want to change the existing behavior,
> thus "if ( rc"

rc is always 0 upon loop exit, afaict:

    for ( ; pdev->phantom_stride; rc = 0 )

Granted this is unusual and hence possibly unexpected.

>>> --- a/xen/drivers/vpci/vpci.c
>>> +++ b/xen/drivers/vpci/vpci.c
>>> @@ -86,6 +86,29 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
>>>   
>>>       return rc;
>>>   }
>>> +
>>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>>> +/* Notify vPCI that device is assigned to guest. */
>>> +int vpci_assign_device(struct domain *d, const struct pci_dev *dev)
>>> +{
>>> +    /* It only makes sense to assign for hwdom or guest domain. */
>> Could you clarify for me in how far this code path is indeed intended
>> to be taken by hwdom? Because if it is, I'd like to further understand
>> the interaction with setup_hwdom_pci_devices().
> setup_hwdom_pci_devices is not used on Arm as we do rely on
> Dom0 to perform PCI host initialization and PCI device enumeration.
> 
> This is because of the fact that on Arm it is not a trivial task to
> initialize a PCI host bridge in Xen, e.g. you need to properly initialize
> power domains, clocks, quirks etc. for different SoCs.
> All these make the task too complex and it was decided that at the
> moment we do not want to bring PCI device drivers in Xen for that.
> It was also decided that we expect Dom0 to take care of initialization
> and enumeration.
> Some day, when firmware can do PCI initialization for us and then we
> can easily access ECAM, this will change. Then setup_hwdom_pci_devices
> will be used on Arm as well.
> 
> Thus, we need to take care that Xen knows about the discovered
> PCI devices via assign_device etc.

Fair enough, but since I've not spotted a patch expressing this (by
adding suitable conditionals), may I ask that you do so in yet another
patch (unless I've overlooked where this gets done)?

>>> +    if ( is_system_domain(d) || !has_vpci(d) )
>>> +        return 0;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/* Notify vPCI that device is de-assigned from guest. */
>>> +int vpci_deassign_device(struct domain *d, const struct pci_dev *dev)
>>> +{
>>> +    /* It only makes sense to de-assign from hwdom or guest domain. */
>>> +    if ( is_system_domain(d) || !has_vpci(d) )
>>> +        return 0;
>>> +
>>> +    return 0;
>>> +}
>>> +#endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
>> At this point of the series #ifdef is the less preferable variant of
>> arranging for dead code to get compiled out.
> What is that other preferable way then?

"if ( !IS_ENABLED() )" as I did already point out to you yesterday in
reply to v2 of patch 10 of this very series.

>>   I expect later patches
>> will change that?
> No, it is going to be this way all the time

The question wasn't whether you switch away from the #ifdef-s, but
whether later patches leave that as the only choice (avoiding build
breakage).

Jan