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
