Re: [PATCH 3/3] pt-irq fixes and improvements

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 03/06/14 15:00, Jan Beulich wrote:

> Tools side:
> - don't silently ignore unrecognized PT_IRQ_TYPE_* values
> - respect that the interface type contains a union, making the code at
>   once no longer depend on the hypervisor ignoring the bus field of the
>   PCI portion of the interface structure)
>
> Hypervisor side:
> - don't ignore the PCI bus number passed in
> - don't store values (gsi, link) calculated from other stored values
> - avoid calling xfree() with a spin lock held where easily possible
> - have pt_irq_destroy_bind() respect the passed in type
> - scope reduction and constification of various variables
> - use switch instead of if/else-if chains
> - formatting
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>   with a few
suggestions for further cleanup...

> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -1702,15 +1702,20 @@ int xc_domain_bind_pt_irq(
>      bind->hvm_domid = domid;
>      bind->irq_type = irq_type;
>      bind->machine_irq = machine_irq;
> -    if ( irq_type == PT_IRQ_TYPE_PCI ||
> -         irq_type == PT_IRQ_TYPE_MSI_TRANSLATE )
> +    switch ( irq_type )
>      {
> +    case PT_IRQ_TYPE_PCI:
> +    case PT_IRQ_TYPE_MSI_TRANSLATE:
>          bind->u.pci.bus = bus;
>          bind->u.pci.device = device;    

Given all this other cleanup, can you nuke the trailing whitespace here...

>          bind->u.pci.intx = intx;
> -    } 
> -    else if ( irq_type == PT_IRQ_TYPE_ISA )
> +    case PT_IRQ_TYPE_ISA:
>          bind->u.isa.isa_irq = isa_irq;
> +        break;
> +    default:
> +        errno = EINVAL;
> +        return -1;
> +    }
>      

... and here.

>      rc = do_domctl(xch, &domctl);
>      return rc;
> @@ -1737,10 +1742,21 @@ int xc_domain_unbind_pt_irq(
>      bind->hvm_domid = domid;
>      bind->irq_type = irq_type;
>      bind->machine_irq = machine_irq;
> -    bind->u.pci.bus = bus;
> -    bind->u.pci.device = device;    
> -    bind->u.pci.intx = intx;
> -    bind->u.isa.isa_irq = isa_irq;
> +    switch ( irq_type )
> +    {
> +    case PT_IRQ_TYPE_PCI:
> +    case PT_IRQ_TYPE_MSI_TRANSLATE:
> +        bind->u.pci.bus = bus;
> +        bind->u.pci.device = device;
> +        bind->u.pci.intx = intx;
> +        break;
> +    case PT_IRQ_TYPE_ISA:
> +        bind->u.isa.isa_irq = isa_irq;
> +        break;
> +    default:
> +        errno = EINVAL;
> +        return -1;
> +    }
>      

And here in this function.

> @@ -96,13 +93,9 @@ void free_hvm_irq_dpci(struct hvm_irq_dp
>  int pt_irq_create_bind(
>      struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind)
>  {
> -    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
> +    struct hvm_irq_dpci *hvm_irq_dpci;
>      struct hvm_pirq_dpci *pirq_dpci;
>      struct pirq *info;
> -    uint32_t guest_gsi;
> -    uint32_t device, intx, link;
> -    struct dev_intx_gsi_link *digl;
> -    struct hvm_girq_dpci_mapping *girq;
>      int rc, pirq = pt_irq_bind->machine_irq;
>  
>      if ( pirq < 0 || pirq >= d->nr_pirqs )

Given all the cleanup, there is a further piece which could be done
between these two hunks.

There is an int i (declared in a for loop initialiser, despite our
fairly consistent C89 style), which should be unsigned.

> @@ -136,7 +129,9 @@ int pt_irq_create_bind(
>      }
>      pirq_dpci = pirq_dpci(info);
>  
> -    if ( pt_irq_bind->irq_type == PT_IRQ_TYPE_MSI )
> +    switch ( pt_irq_bind->irq_type )
> +    {
> +    case PT_IRQ_TYPE_MSI:
>      {
>          uint8_t dest, dest_mode;
>          int dest_vcpu_id;
>