Re: [PATCH v2 1/2] x86/IRQ: make max number of guests for a shared IRQ configurable

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.12.2020 02:58, Igor Druzhinin wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1641,6 +1641,15 @@ This option is ignored in **pv-shim** mode.
>  ### nr_irqs (x86)
>  > `= <integer>`
>  
> +### irq_max_guests (x86)

As a rule of thumb, new options want to use - instead of _ as
separators. There was a respective discussion quite some time
ago. My patch to treat - and _ equally when parsing options
wasn't liked by Andrew ...

> @@ -435,6 +439,9 @@ int __init init_irq_data(void)
>      for ( ; irq < nr_irqs; irq++ )
>          irq_to_desc(irq)->irq = irq;
>  
> +    if ( !irq_max_guests || irq_max_guests > 255)

The 255 is a consequence of the struct field being u8, aiui?
There should be a direct connection between the two, i.e. when
changing the underlying property preferably only one place
should require touching (possible e.g. by converting to a bit
field with its width established via a #define), or comments
on either side should point at the other one.

Also as a nit, there's a blank missing ahead of the closing
paren.

> @@ -1564,7 +1570,8 @@ int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, 
> int will_share)
>          if ( newaction == NULL )
>          {
>              spin_unlock_irq(&desc->lock);
> -            if ( (newaction = xmalloc(irq_guest_action_t)) != NULL &&
> +            if ( (newaction = xmalloc_bytes(sizeof(irq_guest_action_t) +
> +                  irq_max_guests * sizeof(action->guest[0]))) != NULL &&

As said in the (later) reply to v1, please try to make use of
xmalloc_flex_struct() here.

> @@ -1633,11 +1640,11 @@ int pirq_guest_bind(struct vcpu *v, struct pirq 
> *pirq, int will_share)
>          goto retry;
>      }
>  
> -    if ( action->nr_guests == IRQ_MAX_GUESTS )
> +    if ( action->nr_guests == irq_max_guests )
>      {
>          printk(XENLOG_G_INFO "Cannot bind IRQ%d to dom%d. "
> -               "Already at max share.\n",
> -               pirq->pirq, v->domain->domain_id);
> +               "Already at max share %u, increase with irq_max_guests= 
> option.\n",
> +               pirq->pirq, v->domain->domain_id, irq_max_guests);

If you already need to largely redo this printk(), please
- switch to %pd at the same time,
- don't have the format string extend across multiple lines,
- preferably avoid to use full stops (we don't use any in the vast
  majority of log messages), e.g. by making it "cannot bind IRQ%d
  to %pd, already at max share %u (increase with irq-max-guests=
  option)", if you want to stay close to the original wording (I
  think this could be expressed more compactly as well).

Jan