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

[Oleksandr <olekstysh@gmail.com>]

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

-- 
Regards,

Oleksandr Tyshchenko