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

[Jan Beulich <jbeulich@xxxxxxxx>]

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