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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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.
>
> Thanks, Roger.
>
Thank you,
Oleksandr