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
