Re: [PATCH v2 08/11] x86/dpci: switch to use a GSI EOI callback

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.09.2020 12:41, Roger Pau Monne wrote:
> --- a/xen/drivers/passthrough/io.c
> +++ b/xen/drivers/passthrough/io.c
> @@ -327,9 +327,10 @@ static void hvm_gsi_eoi(struct domain *d, unsigned int 
> gsi,
>      hvm_pirq_eoi(pirq, ent);
>  }
>  
> -void hvm_dpci_eoi(unsigned int guest_gsi, const union vioapic_redir_entry 
> *ent)
> +static void dpci_eoi(unsigned int guest_gsi, void *data)
>  {
>      struct domain *d = current->domain;
> +    const union vioapic_redir_entry *ent = data;
>      const struct hvm_irq_dpci *hvm_irq_dpci;
>      const struct hvm_girq_dpci_mapping *girq;
>  
> @@ -565,7 +566,7 @@ int pt_irq_create_bind(
>              unsigned int link;
>  
>              digl = xmalloc(struct dev_intx_gsi_link);
> -            girq = xmalloc(struct hvm_girq_dpci_mapping);
> +            girq = xzalloc(struct hvm_girq_dpci_mapping);
>  
>              if ( !digl || !girq )
>              {
> @@ -578,11 +579,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);

So this is where my question on the earlier patch gets answered:
You utilize passing NULL data to the callback to actually get
passed the IO-APIC redir entry pointer into the callback. This is
perhaps okay in principle if it was half way visible. May I ask
that at the very least instead of switching to xzalloc above you
set ->data to NULL here explicitly, accompanied by a comment on
the effect?

However, I wonder whether it wouldn't be better to have the
callback be passed const union vioapic_redir_entry * right away.
Albeit I haven't looked at the later patches yes, where it may
well be I'd find arguments against.

> @@ -590,8 +602,17 @@ int pt_irq_create_bind(
>          }
>          else
>          {
> +            struct hvm_gsi_eoi_callback *cb =
> +                xzalloc(struct hvm_gsi_eoi_callback);

I can't seem to be able to spot anywhere that this would get freed
(except on an error path in this function).

>              ASSERT(is_hardware_domain(d));
>  
> +            if ( !cb )
> +            {
> +                spin_unlock(&d->event_lock);
> +                return -ENOMEM;
> +            }
> +
>              /* 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 )
> @@ -601,6 +622,19 @@ int pt_irq_create_bind(
>                  return -EINVAL;
>              }

There's an error path here where you don't free cb, and I think
one or two more further down (where you then also may need to
unregister it first).

Jan