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-andr2...@gmail.com/
