Re: [PATCH v5 12/14] xen/arm: translate virtual PCI bus topology for guests

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Roger!

On 13.01.22 14:18, Roger Pau Monné wrote:
> On Thu, Nov 25, 2021 at 01:02:49PM +0200, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> There are three  originators for the PCI configuration space access:
>> 1. The domain that owns physical host bridge: MMIO handlers are
>> there so we can update vPCI register handlers with the values
>> written by the hardware domain, e.g. physical view of the registers
>> vs guest's view on the configuration space.
>> 2. Guest access to the passed through PCI devices: we need to properly
>> map virtual bus topology to the physical one, e.g. pass the configuration
>> space access to the corresponding physical devices.
>> 3. Emulated host PCI bridge access. It doesn't exist in the physical
>> topology, e.g. it can't be mapped to some physical host bridge.
>> So, all access to the host bridge itself needs to be trapped and
>> emulated.
> I'm kind of lost in this commit message. You are just adding a
> translate function in order for domUs to translate from virtual SBDF
> to the physical SBDF of the device. I realize you do that based on
> whether 'bridge' is set or not, so I assume this is just a way to
> signal whether the domain is a hardware domain or not. Ie:
> !!bridge == is_hardware_domain(v->domain).
Simply put: yes
>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>> ---
>> Since v4:
>> - indentation fixes
>> - constify struct domain
>> - updated commit message
>> - updates to the new locking scheme (pdev->vpci_lock)
>> Since v3:
>> - revisit locking
>> - move code to vpci.c
>> Since v2:
>>   - pass struct domain instead of struct vcpu
>>   - constify arguments where possible
>>   - gate relevant code with CONFIG_HAS_VPCI_GUEST_SUPPORT
>> New in v2
>> ---
>>   xen/arch/arm/vpci.c     | 18 ++++++++++++++++++
>>   xen/drivers/vpci/vpci.c | 27 +++++++++++++++++++++++++++
>>   xen/include/xen/vpci.h  |  1 +
>>   3 files changed, 46 insertions(+)
>>
>> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
>> index 8e801f275879..3d134f42d07e 100644
>> --- 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;
> I'm unsure what returning 1 implies for Arm here, but you likely need
> to set '*r = ~0ul;'.
Good catch, will add
>
>> +#endif
>> +
>>       if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>>                           1U << info->dabt.size, &data) )
>>       {
>> @@ -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;
>> +#endif
>> +
>>       return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa),
>>                              1U << info->dabt.size, r);
>>   }
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index c2fb4d4db233..bdc8c63f73fa 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -195,6 +195,33 @@ static void vpci_remove_virtual_device(struct domain *d,
>>       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(const struct domain *d, pci_sbdf_t *sbdf)
>> +{
>> +    struct pci_dev *pdev;
>> +
> I would add:
>
> ASSERT(!is_hardware_domain(d));
>
> To make sure this is not used for the hardware domain.
Will add
>
>> +    for_each_pdev( d, pdev )
>> +    {
>> +        bool found;
>> +
>> +        spin_lock(&pdev->vpci_lock);
>> +        found = pdev->vpci && (pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf);
>> +        spin_unlock(&pdev->vpci_lock);
>> +
>> +        if ( found )
>> +        {
>> +            /* Replace guest SBDF with the physical one. */
>> +            *sbdf = pdev->sbdf;
>> +            return true;
>> +        }
>> +    }
>> +
>> +    return false;
>> +}
>> +
>>   /* Notify vPCI that device is assigned to guest. */
>>   int vpci_assign_device(struct domain *d, struct pci_dev *pdev)
>>   {
>> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
>> index e5258bd7ce90..21d76929391f 100644
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -280,6 +280,7 @@ static inline void vpci_cancel_pending_locked(struct 
>> pci_dev *pdev)
>>   /* Notify vPCI that device is assigned/de-assigned to/from guest. */
>>   int vpci_assign_device(struct domain *d, struct pci_dev *pdev);
>>   int vpci_deassign_device(struct domain *d, struct pci_dev *pdev);
>> +bool vpci_translate_virtual_device(const struct domain *d, pci_sbdf_t 
>> *sbdf);
>>   #else
>>   static inline int vpci_assign_device(struct domain *d, struct pci_dev 
>> *pdev)
>>   {
> If you add a dummy vpci_translate_virtual_device helper that returns
> false unconditionally here you could drop the #ifdefs in arm/vpci.c
> AFAICT.
Will try to do so
>
> Thanks, Roger.
Thank you,
Oleksandr