Re: [PATCH v2 1/2] restrict concept of pIRQ to x86

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 03/05/2023 16:33, Jan Beulich wrote:
> ... by way of a new arch-selectable Kconfig control.
>
> Note that some smaller pieces of code are left without #ifdef, to keep
> things better readable. Hence items like ECS_PIRQ, nr_static_irqs, or
> domain_pirq_to_irq() remain uniformly.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> I'm not really certain about XEN_DOMCTL_irq_permission: With pIRQ-s not
> used, the prior pIRQ -> IRQ translation cannot have succeeded on Arm, so
> quite possibly the entire domctl is unused there? Yet then how is access
> to particular device IRQs being granted/revoked?
> ---
> v2: New.
>
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1120,7 +1120,7 @@ introduced with the Nehalem architecture
>         intended as an emergency option for people who first chose fast, then
>         change their minds to secure, and wish not to reboot.**
>   
> -### extra_guest_irqs
> +### extra_guest_irqs (x86)
>   > `= [<domU number>][,<dom0 number>]`
>   
>   > Default: `32,<variable>`
> --- a/xen/arch/arm/include/asm/irq.h
> +++ b/xen/arch/arm/include/asm/irq.h
> @@ -52,7 +52,6 @@ struct arch_irq_desc {
>   
>   extern const unsigned int nr_irqs;
>   #define nr_static_irqs NR_IRQS
> -#define arch_hwdom_irqs(domid) NR_IRQS
>   
>   struct irq_desc;
>   struct irqaction;
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -25,6 +25,7 @@ config X86
>         select HAS_PCI
>         select HAS_PCI_MSI
>         select HAS_PDX
> +       select HAS_PIRQ
>         select HAS_SCHED_GRANULARITY
>         select HAS_UBSAN
>         select HAS_VPCI if HVM
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -56,6 +56,9 @@ config HAS_KEXEC
>   config HAS_PDX
>         bool
>   
> +config HAS_PIRQ
> +       bool
> +
>   config HAS_PMAP
>         bool
>   
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -350,6 +350,8 @@ static int late_hwdom_init(struct domain
>   #endif
>   }
>   
> +#ifdef CONFIG_HAS_PIRQ
> +
>   static unsigned int __read_mostly extra_hwdom_irqs;
>   static unsigned int __read_mostly extra_domU_irqs = 32;
>   
> @@ -364,6 +366,8 @@ static int __init cf_check parse_extra_g
>   }
>   custom_param("extra_guest_irqs", parse_extra_guest_irqs);
>   
> +#endif /* CONFIG_HAS_PIRQ */
NIT: I would suggest create a file pirq.c and move anything PIRQ 
specific there. This should reduce the number of #ifdef in the code.
> +
>   /*
>    * Release resources held by a domain.  There may or may not be live
>    * references to the domain, and it may or may not be fully constructed.
> @@ -653,6 +657,7 @@ struct domain *domain_create(domid_t dom
>       if ( is_system_domain(d) && !is_idle_domain(d) )
>           return d;
>   
> +#ifdef CONFIG_HAS_PIRQ
>       if ( !is_idle_domain(d) )
>       {
>           if ( !is_hardware_domain(d) )
> @@ -664,6 +669,7 @@ struct domain *domain_create(domid_t dom
>   
>           radix_tree_init(&d->pirq_tree);
>       }
> +#endif
>   
>       if ( (err = arch_domain_create(d, config, flags)) != 0 )
>           goto fail;
> @@ -755,7 +761,9 @@ struct domain *domain_create(domid_t dom
>       {
>           evtchn_destroy(d);
>           evtchn_destroy_final(d);
> +#ifdef CONFIG_HAS_PIRQ
>           radix_tree_destroy(&d->pirq_tree, free_pirq_struct);
> +#endif
>       }
>       if ( init_status & INIT_watchdog )
>           watchdog_domain_destroy(d);
> @@ -1151,7 +1159,9 @@ static void cf_check complete_domain_des
>   
>       evtchn_destroy_final(d);
>   
> +#ifdef CONFIG_HAS_PIRQ
>       radix_tree_destroy(&d->pirq_tree, free_pirq_struct);
> +#endif
>   
>       xfree(d->vcpu);
>   
> @@ -1864,6 +1874,8 @@ long do_vm_assist(unsigned int cmd, unsi
>   }
>   #endif
>   
> +#ifdef CONFIG_HAS_PIRQ
> +
With this change and some others, we probably can remove 
alloc_pirq_struct() & co. I will have a look to send a clean-up patch 
after this goes in.
>   struct pirq *pirq_get_info(struct domain *d, int pirq)
>   {
>       struct pirq *info = pirq_info(d, pirq);
> @@ -1893,6 +1905,8 @@ void cf_check free_pirq_struct(void *ptr
>       call_rcu(&pirq->rcu_head, _free_pirq_struct);
>   }
>   
> +#endif /* CONFIG_HAS_PIRQ */
> +
>   struct migrate_info {
>       long (*func)(void *data);
>       void *data;
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -683,11 +683,13 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>           unsigned int pirq = op->u.irq_permission.pirq, irq;
>           int allow = op->u.irq_permission.allow_access;
>   
> +#ifdef CONFIG_HAS_PIRQ
>           if ( pirq >= current->domain->nr_pirqs )
>           {
>               ret = -EINVAL;
>               break;
>           }
> +#endif
This #ifdef reads a little bit strange. If we can get away with the 
check for Arm, then why can't when CONFIG_HAS_PIRQ=y? Overall, a comment 
would be helpful.
>           irq = pirq_access_permitted(current->domain, pirq);
>           if ( !irq || xsm_irq_permission(XSM_HOOK, d, irq, allow) )
>               ret = -EPERM;
Cheers,

--
Julien Grall