Re: [PATCH v4 11/11] xen/arm: translate virtual PCI bus topology for guests

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 08.11.21 17:28, Oleksandr Andrushchenko wrote:
>
> On 08.11.21 16:23, Roger Pau Monné wrote:
>> On Mon, Nov 08, 2021 at 11:16:42AM +0000, Oleksandr Andrushchenko wrote:
>>> On 08.11.21 13:10, Jan Beulich wrote:
>>>> On 05.11.2021 07:56, Oleksandr Andrushchenko wrote:
>>>>> --- a/xen/arch/arm/vpci.c
>>>>> +++ b/xen/arch/arm/vpci.c
>>>>> @@ -41,6 +41,15 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t 
>>>>> *info,
>>>>>         /* data is needed to prevent a pointer cast on 32bit */
>>>>>         unsigned long data;
>>>>>     
>>>>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>>>>> +    /*
>>>>> +     * For the passed through devices we need to map their virtual SBDF
>>>>> +     * to the physical PCI device being passed through.
>>>>> +     */
>>>>> +    if ( !bridge && !vpci_translate_virtual_device(v->domain, &sbdf) )
>>>>> +            return 1;
>>>> Nit: Indentation.
>>> Ouch, sure
>>>>> @@ -59,6 +68,15 @@ static int vpci_mmio_write(struct vcpu *v, mmio_info_t 
>>>>> *info,
>>>>>         struct pci_host_bridge *bridge = p;
>>>>>         pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
>>>>>     
>>>>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>>>>> +    /*
>>>>> +     * For the passed through devices we need to map their virtual SBDF
>>>>> +     * to the physical PCI device being passed through.
>>>>> +     */
>>>>> +    if ( !bridge && !vpci_translate_virtual_device(v->domain, &sbdf) )
>>>>> +            return 1;
>>>> Again.
>>> Will fix
>>>>> @@ -172,10 +175,37 @@ REGISTER_VPCI_INIT(vpci_add_virtual_device, 
>>>>> VPCI_PRIORITY_MIDDLE);
>>>>>     static void vpci_remove_virtual_device(struct domain *d,
>>>>>                                            const struct pci_dev *pdev)
>>>>>     {
>>>>> +    ASSERT(pcidevs_locked());
>>>>> +
>>>>>         clear_bit(pdev->vpci->guest_sbdf.dev, &d->vpci_dev_assigned_map);
>>>>>         pdev->vpci->guest_sbdf.sbdf = ~0;
>>>>>     }
>>>>>     
>>>>> +/*
>>>>> + * Find the physical device which is mapped to the virtual device
>>>>> + * and translate virtual SBDF to the physical one.
>>>>> + */
>>>>> +bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf)
>>>> const struct domain *d ?
>>> Will change
>>>>> +{
>>>>> +    const struct pci_dev *pdev;
>>>>> +    bool found = false;
>>>>> +
>>>>> +    pcidevs_lock();
>>>>> +    for_each_pdev( d, pdev )
>>>>> +    {
>>>>> +        if ( pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf )
>>>>> +        {
>>>>> +            /* Replace virtual SBDF with the physical one. */
>>>>> +            *sbdf = pdev->sbdf;
>>>>> +            found = true;
>>>>> +            break;
>>>>> +        }
>>>>> +    }
>>>>> +    pcidevs_unlock();
>>>> I think the description wants to at least mention that in principle
>>>> this is too coarse grained a lock, providing justification for why
>>>> it is deemed good enough nevertheless. (Personally, as expressed
>>>> before, I don't think the lock should be used here, but as long as
>>>> Roger agrees with you, you're fine.)
>>> Yes, makes sense
>> Seeing as we don't take the lock in vpci_{read,write} I'm not sure we
>> need it here either then.
> Yes, I was not feeling confident while adding locking
>> Since on Arm you will add devices to the guest at runtime (ie: while
>> there could already be PCI accesses), have you seen issues with not
>> taking the lock here?
> No, I didn't. Neither I am aware of Arm had problems
> But this could just mean that we were lucky not to step on it
>> I think the whole pcidevs locking needs to be clarified, as it's
>> currently a mess.
> Agree
>>    If you want to take it here that's fine, but overall
>> there are issues in other places that would make removing a device at
>> runtime not reliable.
> So, what's the decision? I would leave the locks where I put them,
> so at least this part won't need fixes.
As I am about to use the lock outside vpci struct in v5 all these go away
>> Thanks, Roger.
>>
> Thank you,
> Oleksandr