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

[Oleksandr <olekstysh@gmail.com>]

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

-- 
Regards,

Oleksandr Tyshchenko