[V7,10/11] xen/arm: translate virtual PCI bus topology for guests

Commit Message

Oleksandr Tyshchenko July 19, 2022, 5:42 p.m. UTC

From: Oleksandr Andrushchenko <[e-mail]>

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.

Signed-off-by: Oleksandr Andrushchenko <[e-mail]>
---
Since v6:
- add pcidevs locking to vpci_translate_virtual_device
- update wrt to the new locking scheme
Since v5:
- add vpci_translate_virtual_device for #ifndef CONFIG_HAS_VPCI_GUEST_SUPPORT
  case to simplify ifdefery
- add ASSERT(!is_hardware_domain(d)); to vpci_translate_virtual_device
- reset output register on failed virtual SBDF translation
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     | 17 +++++++++++++++++
 xen/drivers/vpci/vpci.c | 26 ++++++++++++++++++++++++++
 xen/include/xen/vpci.h  |  7 +++++++
 3 files changed, 50 insertions(+)

Comments

Jan Beulich July 26, 2022, 3:16 p.m. UTC | #1

On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
> --- a/xen/arch/arm/vpci.c
> +++ b/xen/arch/arm/vpci.c
> @@ -41,6 +41,16 @@ 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;
>  
> +    /*
> +     * 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) )
> +    {
> +        *r = ~0ul;
> +        return 1;
> +    }

I'm probably simply lacking specific Arm-side knowledge, but it strikes
me as odd that the need for translation would be dependent upon "bridge".

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -158,6 +158,32 @@ static void vpci_remove_virtual_device(const struct pci_dev *pdev)
>      }
>  }
>  
> +/*
> + * 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)
> +{
> +    struct pci_dev *pdev;

const wherever possible please (i.e. likely also for the first function
parameter).

> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -276,6 +276,7 @@ static inline bool __must_check vpci_process_pending(struct vcpu *v)
>  /* Notify vPCI that device is assigned/de-assigned to/from guest. */
>  int vpci_assign_device(struct pci_dev *pdev);
>  void vpci_deassign_device(struct pci_dev *pdev);
> +bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf);
>  #else
>  static inline int vpci_assign_device(struct pci_dev *pdev)
>  {
> @@ -285,6 +286,12 @@ static inline int vpci_assign_device(struct pci_dev *pdev)
>  static inline void vpci_deassign_device(struct pci_dev *pdev)
>  {
>  };
> +
> +static inline bool vpci_translate_virtual_device(struct domain *d,
> +                                                 pci_sbdf_t *sbdf)
> +{
> +    return false;
> +}

Please don't add stubs which aren't really needed (which, afaict, is the
case for this one).

Jan

Oleksandr Tyshchenko July 27, 2022, 5:54 p.m. UTC | #2

On 26.07.22 18:16, Jan Beulich wrote:

Hello Jan

> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>> --- a/xen/arch/arm/vpci.c
>> +++ b/xen/arch/arm/vpci.c
>> @@ -41,6 +41,16 @@ 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;
>>   
>> +    /*
>> +     * 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) )
>> +    {
>> +        *r = ~0ul;
>> +        return 1;
>> +    }
> I'm probably simply lacking specific Arm-side knowledge, but it strikes
> me as odd that the need for translation would be dependent upon "bridge".


I am afraid I cannot answer immediately.

I will analyze that question and provide an answer later on.



>
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -158,6 +158,32 @@ static void vpci_remove_virtual_device(const struct pci_dev *pdev)
>>       }
>>   }
>>   
>> +/*
>> + * 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)
>> +{
>> +    struct pci_dev *pdev;
> const wherever possible please (i.e. likely also for the first function
> parameter).


ok, will do


>
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -276,6 +276,7 @@ static inline bool __must_check vpci_process_pending(struct vcpu *v)
>>   /* Notify vPCI that device is assigned/de-assigned to/from guest. */
>>   int vpci_assign_device(struct pci_dev *pdev);
>>   void vpci_deassign_device(struct pci_dev *pdev);
>> +bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf);
>>   #else
>>   static inline int vpci_assign_device(struct pci_dev *pdev)
>>   {
>> @@ -285,6 +286,12 @@ static inline int vpci_assign_device(struct pci_dev *pdev)
>>   static inline void vpci_deassign_device(struct pci_dev *pdev)
>>   {
>>   };
>> +
>> +static inline bool vpci_translate_virtual_device(struct domain *d,
>> +                                                 pci_sbdf_t *sbdf)
>> +{
>> +    return false;
>> +}
> Please don't add stubs which aren't really needed (which, afaict, is the
> case for this one).


I assume, this is needed if HAS_VPCI is present, but 
HAS_VPCI_GUEST_SUPPORT is not. And the author added that stub 
specifically to drop a few "#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT" from 
Arm's code.

Or I really missed something?



>
> Jan

Oleksandr Tyshchenko July 27, 2022, 7:39 p.m. UTC | #3

Hello Jan


On 27.07.22 20:54, Oleksandr wrote:
>
> On 26.07.22 18:16, Jan Beulich wrote:
>
> Hello Jan
>
>> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>>> --- a/xen/arch/arm/vpci.c
>>> +++ b/xen/arch/arm/vpci.c
>>> @@ -41,6 +41,16 @@ 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;
>>>   +    /*
>>> +     * 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) )
>>> +    {
>>> +        *r = ~0ul;
>>> +        return 1;
>>> +    }
>> I'm probably simply lacking specific Arm-side knowledge, but it strikes
>> me as odd that the need for translation would be dependent upon 
>> "bridge".
>
>
> I am afraid I cannot answer immediately.
>
> I will analyze that question and provide an answer later on.


Well, most likely that "valid" bridge pointer here is just used as an 
indicator of hwdom currently, so no need to perform virt->phys 
translation for sbdf.

You can see that domain_vpci_init() passes a valid value for hwdom and 
NULL for other domains when setting up vpci_mmio* callbacks.

Alternatively, I guess we could use "!is_hardware_domain(v->domain)" 
instead of "!bridge" in the first part of that check. Shall I?

Jan Beulich July 28, 2022, 7:15 a.m. UTC | #4

On 27.07.2022 21:39, Oleksandr wrote:
> On 27.07.22 20:54, Oleksandr wrote:
>> On 26.07.22 18:16, Jan Beulich wrote:
>>> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>>>> --- a/xen/arch/arm/vpci.c
>>>> +++ b/xen/arch/arm/vpci.c
>>>> @@ -41,6 +41,16 @@ 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;
>>>>   +    /*
>>>> +     * 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) )
>>>> +    {
>>>> +        *r = ~0ul;
>>>> +        return 1;
>>>> +    }
>>> I'm probably simply lacking specific Arm-side knowledge, but it strikes
>>> me as odd that the need for translation would be dependent upon 
>>> "bridge".
>>
>>
>> I am afraid I cannot answer immediately.
>>
>> I will analyze that question and provide an answer later on.
> 
> 
> Well, most likely that "valid" bridge pointer here is just used as an 
> indicator of hwdom currently, so no need to perform virt->phys 
> translation for sbdf.
> 
> You can see that domain_vpci_init() passes a valid value for hwdom and 
> NULL for other domains when setting up vpci_mmio* callbacks.

Oh, I see.

> Alternatively, I guess we could use "!is_hardware_domain(v->domain)" 
> instead of "!bridge" in the first part of that check. Shall I?

Maybe simply add a comment? Surely checking "bridge" is cheaper than
using is_hardware_domain(), so I can see the benefit. But the larger
arm/vpci.c grows, the less obvious the connection will be without a
comment. (Instead of a comment, an alternative may be a suitable
assertion, which then documents the connection at the same time, e.g.
ASSERT(!bridge == !is_hardware_domain(v->domain)). But that won't be
possible in e.g. vpci_sbdf_from_gpa(), where apparently a similar
assumption is being made.)

Jan

Oleksandr Tyshchenko July 28, 2022, 4:35 p.m. UTC | #5

On 28.07.22 10:15, Jan Beulich wrote:

Hello Jan

> On 27.07.2022 21:39, Oleksandr wrote:
>> On 27.07.22 20:54, Oleksandr wrote:
>>> On 26.07.22 18:16, Jan Beulich wrote:
>>>> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>>>>> --- a/xen/arch/arm/vpci.c
>>>>> +++ b/xen/arch/arm/vpci.c
>>>>> @@ -41,6 +41,16 @@ 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;
>>>>>    +    /*
>>>>> +     * 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) )
>>>>> +    {
>>>>> +        *r = ~0ul;
>>>>> +        return 1;
>>>>> +    }
>>>> I'm probably simply lacking specific Arm-side knowledge, but it strikes
>>>> me as odd that the need for translation would be dependent upon
>>>> "bridge".
>>>
>>> I am afraid I cannot answer immediately.
>>>
>>> I will analyze that question and provide an answer later on.
>>
>> Well, most likely that "valid" bridge pointer here is just used as an
>> indicator of hwdom currently, so no need to perform virt->phys
>> translation for sbdf.
>>
>> You can see that domain_vpci_init() passes a valid value for hwdom and
>> NULL for other domains when setting up vpci_mmio* callbacks.
> Oh, I see.
>
>> Alternatively, I guess we could use "!is_hardware_domain(v->domain)"
>> instead of "!bridge" in the first part of that check. Shall I?
> Maybe simply add a comment? Surely checking "bridge" is cheaper than
> using is_hardware_domain(), so I can see the benefit. But the larger
> arm/vpci.c grows, the less obvious the connection will be without a
> comment.


Agree the connection is worth a comment ...



>   (Instead of a comment, an alternative may be a suitable
> assertion, which then documents the connection at the same time, e.g.
> ASSERT(!bridge == !is_hardware_domain(v->domain)). But that won't be
> possible in e.g. vpci_sbdf_from_gpa(), where apparently a similar
> assumption is being made.)


    ... or indeed to put such ASSERT _before_ vpci_sbdf_from_gpa().

This will cover assumption being made in both places.


diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
index a9fc5817f9..1d4b1ef39e 100644
--- a/xen/arch/arm/vpci.c
+++ b/xen/arch/arm/vpci.c
@@ -37,10 +37,24 @@ static int vpci_mmio_read(struct vcpu *v, 
mmio_info_t *info,
                            register_t *r, void *p)
  {
      struct pci_host_bridge *bridge = p;
-    pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
+    pci_sbdf_t sbdf;
      /* data is needed to prevent a pointer cast on 32bit */
      unsigned long data;

+    ASSERT(!bridge == !is_hardware_domain(v->domain));
+
+    sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
+
+    /*
+     * 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) )
+    {
+        *r = ~0ul;
+        return 1;
+    }
+
      if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
                          1U << info->dabt.size, &data) )
      {
@@ -57,7 +71,18 @@ static int vpci_mmio_write(struct vcpu *v, 
mmio_info_t *info,
                             register_t r, void *p)
  {
      struct pci_host_bridge *bridge = p;
-    pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
+    pci_sbdf_t sbdf;
+
+    ASSERT(!bridge == !is_hardware_domain(v->domain));
+
+    sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
+
+    /*
+     * 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;

      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 d4601ecf9b..fc2c51dc3e 100644


Any preference here?


Personally, I think that such ASSERT will better explain the connection 
than the comment will do.


>
> Jan

Jan Beulich July 29, 2022, 6:06 a.m. UTC | #6

On 28.07.2022 18:35, Oleksandr wrote:
> On 28.07.22 10:15, Jan Beulich wrote:
>> On 27.07.2022 21:39, Oleksandr wrote:
>>> On 27.07.22 20:54, Oleksandr wrote:
>>>> On 26.07.22 18:16, Jan Beulich wrote:
>>>>> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>>>>>> --- a/xen/arch/arm/vpci.c
>>>>>> +++ b/xen/arch/arm/vpci.c
>>>>>> @@ -41,6 +41,16 @@ 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;
>>>>>>    +    /*
>>>>>> +     * 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) )
>>>>>> +    {
>>>>>> +        *r = ~0ul;
>>>>>> +        return 1;
>>>>>> +    }
>>>>> I'm probably simply lacking specific Arm-side knowledge, but it strikes
>>>>> me as odd that the need for translation would be dependent upon
>>>>> "bridge".
>>>>
>>>> I am afraid I cannot answer immediately.
>>>>
>>>> I will analyze that question and provide an answer later on.
>>>
>>> Well, most likely that "valid" bridge pointer here is just used as an
>>> indicator of hwdom currently, so no need to perform virt->phys
>>> translation for sbdf.
>>>
>>> You can see that domain_vpci_init() passes a valid value for hwdom and
>>> NULL for other domains when setting up vpci_mmio* callbacks.
>> Oh, I see.
>>
>>> Alternatively, I guess we could use "!is_hardware_domain(v->domain)"
>>> instead of "!bridge" in the first part of that check. Shall I?
>> Maybe simply add a comment? Surely checking "bridge" is cheaper than
>> using is_hardware_domain(), so I can see the benefit. But the larger
>> arm/vpci.c grows, the less obvious the connection will be without a
>> comment.
> 
> 
> Agree the connection is worth a comment ...
> 
> 
> 
>>   (Instead of a comment, an alternative may be a suitable
>> assertion, which then documents the connection at the same time, e.g.
>> ASSERT(!bridge == !is_hardware_domain(v->domain)). But that won't be
>> possible in e.g. vpci_sbdf_from_gpa(), where apparently a similar
>> assumption is being made.)
> 
> 
>     ... or indeed to put such ASSERT _before_ vpci_sbdf_from_gpa().
> 
> This will cover assumption being made in both places.
> 
> 
> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> index a9fc5817f9..1d4b1ef39e 100644
> --- a/xen/arch/arm/vpci.c
> +++ b/xen/arch/arm/vpci.c
> @@ -37,10 +37,24 @@ static int vpci_mmio_read(struct vcpu *v, 
> mmio_info_t *info,
>                             register_t *r, void *p)
>   {
>       struct pci_host_bridge *bridge = p;
> -    pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
> +    pci_sbdf_t sbdf;
>       /* data is needed to prevent a pointer cast on 32bit */
>       unsigned long data;
> 
> +    ASSERT(!bridge == !is_hardware_domain(v->domain));
> +
> +    sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
> +
> +    /*
> +     * 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) )
> +    {
> +        *r = ~0ul;
> +        return 1;
> +    }
> +
>       if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>                           1U << info->dabt.size, &data) )
>       {
> @@ -57,7 +71,18 @@ static int vpci_mmio_write(struct vcpu *v, 
> mmio_info_t *info,
>                              register_t r, void *p)
>   {
>       struct pci_host_bridge *bridge = p;
> -    pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
> +    pci_sbdf_t sbdf;
> +
> +    ASSERT(!bridge == !is_hardware_domain(v->domain));
> +
> +    sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
> +
> +    /*
> +     * 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;
> 
>       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 d4601ecf9b..fc2c51dc3e 100644
> 
> 
> Any preference here?
> 
> 
> Personally, I think that such ASSERT will better explain the connection 
> than the comment will do.

Indeed I'd also prefer ASSERT()s being put there. But my opinion is
secondary here, as I'm not a maintainer of this code.

Jan

Oleksandr Tyshchenko July 29, 2022, 4:26 p.m. UTC | #7

On 29.07.22 09:06, Jan Beulich wrote:

Hello Jan

> On 28.07.2022 18:35, Oleksandr wrote:
>> On 28.07.22 10:15, Jan Beulich wrote:
>>> On 27.07.2022 21:39, Oleksandr wrote:
>>>> On 27.07.22 20:54, Oleksandr wrote:
>>>>> On 26.07.22 18:16, Jan Beulich wrote:
>>>>>> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>>>>>>> --- a/xen/arch/arm/vpci.c
>>>>>>> +++ b/xen/arch/arm/vpci.c
>>>>>>> @@ -41,6 +41,16 @@ 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;
>>>>>>>     +    /*
>>>>>>> +     * 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) )
>>>>>>> +    {
>>>>>>> +        *r = ~0ul;
>>>>>>> +        return 1;
>>>>>>> +    }
>>>>>> I'm probably simply lacking specific Arm-side knowledge, but it strikes
>>>>>> me as odd that the need for translation would be dependent upon
>>>>>> "bridge".
>>>>> I am afraid I cannot answer immediately.
>>>>>
>>>>> I will analyze that question and provide an answer later on.
>>>> Well, most likely that "valid" bridge pointer here is just used as an
>>>> indicator of hwdom currently, so no need to perform virt->phys
>>>> translation for sbdf.
>>>>
>>>> You can see that domain_vpci_init() passes a valid value for hwdom and
>>>> NULL for other domains when setting up vpci_mmio* callbacks.
>>> Oh, I see.
>>>
>>>> Alternatively, I guess we could use "!is_hardware_domain(v->domain)"
>>>> instead of "!bridge" in the first part of that check. Shall I?
>>> Maybe simply add a comment? Surely checking "bridge" is cheaper than
>>> using is_hardware_domain(), so I can see the benefit. But the larger
>>> arm/vpci.c grows, the less obvious the connection will be without a
>>> comment.
>>
>> Agree the connection is worth a comment ...
>>
>>
>>
>>>    (Instead of a comment, an alternative may be a suitable
>>> assertion, which then documents the connection at the same time, e.g.
>>> ASSERT(!bridge == !is_hardware_domain(v->domain)). But that won't be
>>> possible in e.g. vpci_sbdf_from_gpa(), where apparently a similar
>>> assumption is being made.)
>>
>>      ... or indeed to put such ASSERT _before_ vpci_sbdf_from_gpa().
>>
>> This will cover assumption being made in both places.
>>
>>
>> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
>> index a9fc5817f9..1d4b1ef39e 100644
>> --- a/xen/arch/arm/vpci.c
>> +++ b/xen/arch/arm/vpci.c
>> @@ -37,10 +37,24 @@ static int vpci_mmio_read(struct vcpu *v,
>> mmio_info_t *info,
>>                              register_t *r, void *p)
>>    {
>>        struct pci_host_bridge *bridge = p;
>> -    pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
>> +    pci_sbdf_t sbdf;
>>        /* data is needed to prevent a pointer cast on 32bit */
>>        unsigned long data;
>>
>> +    ASSERT(!bridge == !is_hardware_domain(v->domain));
>> +
>> +    sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
>> +
>> +    /*
>> +     * 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) )
>> +    {
>> +        *r = ~0ul;
>> +        return 1;
>> +    }
>> +
>>        if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>>                            1U << info->dabt.size, &data) )
>>        {
>> @@ -57,7 +71,18 @@ static int vpci_mmio_write(struct vcpu *v,
>> mmio_info_t *info,
>>                               register_t r, void *p)
>>    {
>>        struct pci_host_bridge *bridge = p;
>> -    pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
>> +    pci_sbdf_t sbdf;
>> +
>> +    ASSERT(!bridge == !is_hardware_domain(v->domain));
>> +
>> +    sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
>> +
>> +    /*
>> +     * 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;
>>
>>        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 d4601ecf9b..fc2c51dc3e 100644
>>
>>
>> Any preference here?
>>
>>
>> Personally, I think that such ASSERT will better explain the connection
>> than the comment will do.
> Indeed I'd also prefer ASSERT()s being put there.

good


>   But my opinion is
> secondary here, as I'm not a maintainer of this code.


sure, let's see what the Arm maintainers will say


>
> Jan

Patch

diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
index a9fc5817f9..84b2b068a0 100644
--- a/xen/arch/arm/vpci.c
+++ b/xen/arch/arm/vpci.c
@@ -41,6 +41,16 @@  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;
 
+    /*
+     * 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) )
+    {
+        *r = ~0ul;
+        return 1;
+    }
+
     if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
                         1U << info->dabt.size, &data) )
     {
@@ -59,6 +69,13 @@  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);
 
+    /*
+     * 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;
+
     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 d4601ecf9b..fc2c51dc3e 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -158,6 +158,32 @@  static void vpci_remove_virtual_device(const struct pci_dev *pdev)
     }
 }
 
+/*
+ * 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)
+{
+    struct pci_dev *pdev;
+
+    ASSERT(!is_hardware_domain(d));
+
+    pcidevs_read_lock();
+    for_each_pdev( d, pdev )
+    {
+        if ( pdev->vpci && (pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf) )
+        {
+            /* Replace guest SBDF with the physical one. */
+            *sbdf = pdev->sbdf;
+            pcidevs_read_unlock();
+            return true;
+        }
+    }
+
+    pcidevs_read_unlock();
+    return false;
+}
+
 /* Notify vPCI that device is assigned to guest. */
 int vpci_assign_device(struct pci_dev *pdev)
 {
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index cc14b0086d..5749d8da78 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -276,6 +276,7 @@  static inline bool __must_check vpci_process_pending(struct vcpu *v)
 /* Notify vPCI that device is assigned/de-assigned to/from guest. */
 int vpci_assign_device(struct pci_dev *pdev);
 void vpci_deassign_device(struct pci_dev *pdev);
+bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf);
 #else
 static inline int vpci_assign_device(struct pci_dev *pdev)
 {
@@ -285,6 +286,12 @@  static inline int vpci_assign_device(struct pci_dev *pdev)
 static inline void vpci_deassign_device(struct pci_dev *pdev)
 {
 };
+
+static inline bool vpci_translate_virtual_device(struct domain *d,
+                                                 pci_sbdf_t *sbdf)
+{
+    return false;
+}
 #endif
 
 #endif