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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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
>
> Jan
>
Thank you,
Oleksandr