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

[Jan Beulich <jbeulich@xxxxxxxx>]

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