Re: [PATCH v4 07/12] x86/dpci: switch to use a GSI EOI callback

[Jan Beulich <jbeulich@suse.com>]

On 20.04.2021 16:07, Roger Pau Monne wrote:
> @@ -476,6 +476,7 @@ int pt_irq_create_bind(
>      {
>          struct dev_intx_gsi_link *digl = NULL;
>          struct hvm_girq_dpci_mapping *girq = NULL;
> +        struct hvm_gsi_eoi_callback *cb = NULL;

I wonder if this wouldn't benefit from a brief "hwdom only" comment.

> @@ -502,11 +503,22 @@ int pt_irq_create_bind(
>              girq->bus = digl->bus = pt_irq_bind->u.pci.bus;
>              girq->device = digl->device = pt_irq_bind->u.pci.device;
>              girq->intx = digl->intx = pt_irq_bind->u.pci.intx;
> -            list_add_tail(&digl->list, &pirq_dpci->digl_list);
> +            girq->cb.callback = dpci_eoi;
>  
>              guest_gsi = hvm_pci_intx_gsi(digl->device, digl->intx);
>              link = hvm_pci_intx_link(digl->device, digl->intx);
>  
> +            rc = hvm_gsi_register_callback(d, guest_gsi, &girq->cb);
> +            if ( rc )
> +            {
> +                spin_unlock(&d->event_lock);
> +                xfree(girq);
> +                xfree(digl);
> +                return rc;
> +            }
> +
> +            list_add_tail(&digl->list, &pirq_dpci->digl_list);
> +
>              hvm_irq_dpci->link_cnt[link]++;

Could we keep calculation and use of "link" together, please, so the
compiler can avoid spilling the value to the stack or allocating a
callee-saved register for it?

> @@ -514,17 +526,43 @@ int pt_irq_create_bind(
>          }
>          else
>          {
> +            /*
> +             * NB: the callback structure allocated below will never be freed
> +             * once setup because it's used by the hardware domain and will
> +             * never be unregistered.
> +             */
> +            cb = xzalloc(struct hvm_gsi_eoi_callback);
> +
>              ASSERT(is_hardware_domain(d));
>  
> +            if ( !cb )
> +            {
> +                spin_unlock(&d->event_lock);
> +                return -ENOMEM;
> +            }

I'm inclined to ask that the ASSERT() remain first in this "else" block.
In fact, you could ...

>              /* MSI_TRANSLATE is not supported for the hardware domain. */
>              if ( pt_irq_bind->irq_type != PT_IRQ_TYPE_PCI ||
>                   pirq >= hvm_domain_irq(d)->nr_gsis )
>              {
>                  spin_unlock(&d->event_lock);
> -
> +                xfree(cb);

... avoid this extra cleanup by ...

>                  return -EINVAL;
>              }

... putting the allocation here.

>              guest_gsi = pirq;
> +
> +            cb->callback = dpci_eoi;
> +            /*
> +             * IRQ binds created for the hardware domain are never destroyed,
> +             * so it's fine to not keep a reference to cb here.
> +             */
> +            rc = hvm_gsi_register_callback(d, guest_gsi, cb);

In reply to a v3 comment of mine you said "I should replace IRQ with
GSI in the comment above to make it clearer." And while the question
of the comment being (and going to remain) true in the first place
was discussed, I would have hoped for the commit message to say a
word on this. If this ever changed, chances are the place here would
go unnoticed and unchanged, leading to a memory leak.

> @@ -596,12 +634,17 @@ int pt_irq_create_bind(
>                      list_del(&digl->list);
>                      link = hvm_pci_intx_link(digl->device, digl->intx);
>                      hvm_irq_dpci->link_cnt[link]--;
> +                    hvm_gsi_unregister_callback(d, guest_gsi, &girq->cb);
>                  }
> +                else
> +                    hvm_gsi_unregister_callback(d, guest_gsi, cb);
> +
>                  pirq_dpci->flags = 0;
>                  pirq_cleanup_check(info, d);
>                  spin_unlock(&d->event_lock);
>                  xfree(girq);
>                  xfree(digl);
> +                xfree(cb);

May I suggest that you move the xfree() into the "else" you add, and
perhaps even make it conditional upon the un-register being successful?

> @@ -708,6 +752,11 @@ int pt_irq_destroy_bind(
>                   girq->machine_gsi == machine_gsi )
>              {
>                  list_del(&girq->list);
> +                rc = hvm_gsi_unregister_callback(d, guest_gsi, &girq->cb);
> +                if ( rc )
> +                    printk(XENLOG_G_WARNING
> +                           "%pd: unable to remove callback for GSI %u: %d\n",
> +                           d, guest_gsi, rc);
>                  xfree(girq);

If the un-registration really failed (here as well as further up),
is it safe to free girq?

Jan