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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 30.09.21 12:06, Jan Beulich wrote:
> 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.
I will remove that check then. Do we want a comment about rc == 0,
so it is seen why there is no check for rc?
>
>>>> --- 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
Well, it is all in the RFC for PCI passthrough on Arm which is mentioned
in series from Arm and EPAM (part 2). I didn't mention the RFC in the
cover letter for this series though.
>   (by
> adding suitable conditionals), may I ask that you do so in yet another
> patch (unless I've overlooked where this gets done)?
Could you please elaborate more on which conditionals you are
talking about here? I'm afraid I didn't understand this part.
>
>>>> +    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.
Please see below
>
>>>    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).
Yes, the code is going to always remain ifdef'ed, so we don't have
dead code for x86 (at least).
So, does the above mean that you are ok with "#ifdef 
CONFIG_HAS_VPCI_GUEST_SUPPORT"
and there is no need for "if ( !IS_ENABLED() )"?
>
> Jan
>
>
Thank you,
Oleksandr