Re: [Xen-devel] [PATCH RFC XEN v1 03/14] xen: arm: switch arch_do_domctl to a common exit path

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 9 Dec 2015, Ian Campbell wrote:
> ... with copyback functionality. A future domctl is going to want
> this, rather than end up with different ops having different return
> behaviour, simply switch everything over to a single exit path scheme.
> 
> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> ---
>  xen/arch/arm/domctl.c | 76 
> ++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 51 insertions(+), 25 deletions(-)
> 
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index 30453d8..d42b2bf 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -12,11 +12,15 @@
>  #include <xen/hypercall.h>
>  #include <xen/iocap.h>
>  #include <xsm/xsm.h>
> +#include <xen/guest_access.h>
>  #include <public/domctl.h>
>  
>  long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>                      XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>  {
> +    long rc;
> +    bool_t copyback = 0;
> +
>      switch ( domctl->cmd )
>      {
>      case XEN_DOMCTL_cacheflush:
> @@ -25,30 +29,36 @@ long arch_do_domctl(struct xen_domctl *domctl, struct 
> domain *d,
>          unsigned long e = s + domctl->u.cacheflush.nr_pfns;
>  
>          if ( domctl->u.cacheflush.nr_pfns > (1U<<MAX_ORDER) )
> -            return -EINVAL;
> -
> -        if ( e < s )
> -            return -EINVAL;
> +            rc = -EINVAL;
> +        else if ( e < s )
> +            rc = -EINVAL;
> +        else
> +            rc = p2m_cache_flush(d, s, e);
>  
> -        return p2m_cache_flush(d, s, e);
> +        break;
>      }
>      case XEN_DOMCTL_bind_pt_irq:
>      {
> -        int rc;
>          xen_domctl_bind_pt_irq_t *bind = &domctl->u.bind_pt_irq;
>          uint32_t irq = bind->u.spi.spi;
>          uint32_t virq = bind->machine_irq;
>  
>          /* We only support PT_IRQ_TYPE_SPI */
>          if ( bind->irq_type != PT_IRQ_TYPE_SPI )
> -            return -EOPNOTSUPP;
> +        {
> +            rc = -EOPNOTSUPP;
> +            break;
> +        }
>  
>          /*
>           * XXX: For now map the interrupt 1:1. Other support will require to
>           * modify domain_pirq_to_irq macro.
>           */
>          if ( irq != virq )
> -            return -EINVAL;
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
>  
>          /*
>           * ARM doesn't require separating IRQ assignation into 2
> @@ -60,66 +70,82 @@ long arch_do_domctl(struct xen_domctl *domctl, struct 
> domain *d,
>           */
>          rc = xsm_map_domain_irq(XSM_HOOK, d, irq, NULL);
>          if ( rc )
> -            return rc;
> +            break;
>  
>          rc = xsm_bind_pt_irq(XSM_HOOK, d, bind);
>          if ( rc )
> -            return rc;
> +            break;
>  
>          if ( !irq_access_permitted(current->domain, irq) )
> -            return -EPERM;
> +        {
> +            rc = -EPERM;
> +            break;
> +        }
>  
>          if ( !vgic_reserve_virq(d, virq) )
> -            return -EBUSY;
> +        {
> +            rc = -EBUSY;
> +            break;
> +        }
>  
>          rc = route_irq_to_guest(d, virq, irq, "routed IRQ");
>          if ( rc )
>              vgic_free_virq(d, virq);
>  
> -        return rc;
> +        break;
>      }
>      case XEN_DOMCTL_unbind_pt_irq:
>      {
> -        int rc;
>          xen_domctl_bind_pt_irq_t *bind = &domctl->u.bind_pt_irq;
>          uint32_t irq = bind->u.spi.spi;
>          uint32_t virq = bind->machine_irq;
>  
>          /* We only support PT_IRQ_TYPE_SPI */
>          if ( bind->irq_type != PT_IRQ_TYPE_SPI )
> -            return -EOPNOTSUPP;
> +        {
> +            rc = -EOPNOTSUPP;
> +            break;
> +        }
>  
>          /* For now map the interrupt 1:1 */
>          if ( irq != virq )
> -            return -EINVAL;
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
>  
>          rc = xsm_unbind_pt_irq(XSM_HOOK, d, bind);
>          if ( rc )
> -            return rc;
> +            break;
>  
>          if ( !irq_access_permitted(current->domain, irq) )
> -            return -EPERM;
> +        {
> +            rc = -EPERM;
> +            break;
> +        }
>  
>          rc = release_guest_irq(d, virq);
>          if ( rc )
> -            return rc;
> +            break;
>  
>          vgic_free_virq(d, virq);
>  
> -        return 0;
> +        rc = 0;
> +        break;
>      }
>      default:
> -    {
> -        int rc;
> -
>          rc = subarch_do_domctl(domctl, d, u_domctl);
>  
>          if ( rc == -ENOSYS )
>              rc = iommu_do_domctl(domctl, d, u_domctl);
>  
> -        return rc;
> -    }
> +        break;
>      }
> +
> +    if ( copyback && __copy_to_guest(u_domctl, domctl, 1) )
> +        rc = -EFAULT;

This code is already present in do_domctl (the caller of this function).
I wonder if it makes sense to pass a copyback* parameter to
arch_do_domctl and just rely on the caller's exit path.

All the other substitutions in this patch are fine.


> +    return rc;
>  }
>  
>  void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
> -- 
> 2.6.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel