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
