Re: [PATCH v3 11/11] xen/arm: Translate virtual PCI bus topology for guests

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Roger!

On 26.10.21 16:30, Roger Pau Monné wrote:
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index fa6fcc5e467c..095671742ad8 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -797,6 +797,7 @@ void arch_domain_destroy(struct domain *d)
>>                          get_order_from_bytes(d->arch.efi_acpi_len));
>>   #endif
>>       domain_io_free(d);
>> +    domain_vpci_free(d);
> It's a nit, but I think from a logical PoV this should be inverted?
> You first free the handlers and then the IO infrastructure.
Indeed, thanks
>
>>   }
>>   
>>   void arch_domain_shutdown(struct domain *d)
>> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
>> index 5d6c29c8dcd9..26ec2fa7cf2d 100644
>> --- a/xen/arch/arm/vpci.c
>> +++ b/xen/arch/arm/vpci.c
>> @@ -17,6 +17,14 @@
>>   
>>   #define REGISTER_OFFSET(addr)  ( (addr) & 0x00000fff)
>>   
>> +struct vpci_mmio_priv {
>> +    /*
>> +     * Set to true if the MMIO handlers were set up for the emulated
>> +     * ECAM host PCI bridge.
>> +     */
>> +    bool is_virt_ecam;
>> +};
> Is this strictly required? It feels a bit odd to have a structure to
> store and single boolean.
>
> I think you could replace it's usage with is_hardware_domain.
I am working on some "earlier" patch fixes [1] which already needs some private
to be passed to the handlers: we need to set sbdf.seg to the proper
host bridge segment instead of always setting it to 0.
And then I can pass "struct pci_host_bridge *bridge" as the private member
and use is_hardware_domain(v->domain) to see if this is guest or hwdom.
So, I'll remove the structure completely

[snip]

>> + */
>>   static int vpci_setup_mmio_handler(struct domain *d,
>>                                      struct pci_host_bridge *bridge)
>>   {
>> -    struct pci_config_window *cfg = bridge->cfg;
>> +    struct vpci_mmio_priv *priv;
>> +
>> +    priv = xzalloc(struct vpci_mmio_priv);
>> +    if ( !priv )
>> +        return -ENOMEM;
>> +
>> +    priv->is_virt_ecam = !is_hardware_domain(d);
>>   
>> -    register_mmio_handler(d, &vpci_mmio_handler,
>> -                          cfg->phys_addr, cfg->size, NULL);
>> +    if ( is_hardware_domain(d) )
>> +    {
>> +        struct pci_config_window *cfg = bridge->cfg;
>> +
>> +        bridge->mmio_priv = priv;
>> +        register_mmio_handler(d, &vpci_mmio_handler,
>> +                              cfg->phys_addr, cfg->size,
>> +                              priv);
>> +    }
>> +    else
>> +    {
>> +        d->vpci_mmio_priv = priv;
>> +        /* Guest domains use what is programmed in their device tree. */
>> +        register_mmio_handler(d, &vpci_mmio_handler,
>> +                              GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE,
>> +                              priv);
>> +    }
>>       return 0;
>>   }
>>   
>> @@ -95,14 +154,25 @@ int domain_vpci_init(struct domain *d)
>>       if ( !has_vpci(d) )
>>           return 0;
>>   
>> +    return pci_host_iterate_bridges(d, vpci_setup_mmio_handler);
> I think this is wrong for unprivileged domains: you iterate against
> host bridges but just setup a single ECAM region from
> GUEST_VPCI_ECAM_BASE to GUEST_VPCI_ECAM_SIZE, so you are leaking
> multiple allocations of vpci_mmio_priv, and also adding a bunch of
> duplicated IO handlers for the same ECAM region.
>
> IMO you should iterate against host bridges only for the hardware
> domain case. For the unpriviledged domain case there's no need to
> iterate against the list of physical host bridges as you end up
> exposing a fully emulated bus which bears no resemblance to the
> physical setup.
Yes, I am moving this code into that "earlier" patch [1] and already
spotted the leak: thus I am also re-working this code.
>
>> +}
>> +
>> +static int domain_vpci_free_cb(struct domain *d,
>> +                               struct pci_host_bridge *bridge)
>> +{
>>       if ( is_hardware_domain(d) )
>> -        return pci_host_iterate_bridges(d, vpci_setup_mmio_handler);
>> +        XFREE(bridge->mmio_priv);
>> +    else
>> +        XFREE(d->vpci_mmio_priv);
>> +    return 0;
>> +}
>>   
>> -    /* Guest domains use what is programmed in their device tree. */
>> -    register_mmio_handler(d, &vpci_mmio_handler,
>> -                          GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL);
>> +void domain_vpci_free(struct domain *d)
>> +{
>> +    if ( !has_vpci(d) )
>> +        return;
>>   
>> -    return 0;
>> +    pci_host_iterate_bridges(d, domain_vpci_free_cb);
> Why do we need to iterate the host bridges for unprivileged domains?
No need, I am taking care of this
> AFAICT it just causes duplicated calls to XFREE(d->vpci_mmio_priv). I
> would expect something like:
>
> static int bridge_free_cb(struct domain *d,
>                            struct pci_host_bridge *bridge)
> {
>      ASSERT(is_hardware_domain(d));
>      XFREE(bridge->mmio_priv);
>      return 0;
> }
>
> void domain_vpci_free(struct domain *d)
> {
>      if ( !has_vpci(d) )
>          return;
>
>      if ( is_hardware_domain(d) )
>          pci_host_iterate_bridges(d, bridge_free_cb);
>      else
>          XFREE(d->vpci_mmio_priv);
> }
>
> Albeit I think there's no need for vpci_mmio_priv in the first place.
>
> Thanks, Roger.
Thank you,
Oleksandr

[1] 
https://patchwork.kernel.org/project/xen-devel/patch/20211008055535.337436-9-andr2000@xxxxxxxxx/