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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 11/07/2023 13:29, Jan Beulich wrote:
> On 10.07.2023 22:59, Julien Grall wrote:
> > > ---
> > > 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?
> (Leaving this in context, as it'll be relevant for the last comment you
> gave.)
Sorry I missed this comment.

> so quite possibly the entire domctl is unused there?

You are right, the domctl permission is not used on Arm.

>  Yet then how is access to particular device IRQs being granted/revoked?

At the moment, a device can only be attached at domain creation and 
detached when the domain is destroyed. Also, only the toolstack can map 
IRQs. So we don't need to worry for granting/revoking IRQs.
> > > --- 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.
> I did consider that, but it looked quite a bit more intrusive to
> me than the few #ifdef-s added to this file. (The ones in other
> files would be yet uglier to eliminate, if that was to be implied
> from your remark.)
I was under the impression we wanted to limit the number of #ifdef in 
the common code. They didn't seem to be warrant here and, from a brief 
look, it didn't seem the option would be that "ugly". But I guess this 
is a matter of taste.
> > > @@ -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.
> Right, there's likely further cleanup possible.
>
> > > --- 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.
> As per the post-commit-message remark first of all I need to understand
> why things were the way they were, and why (whether) that was correct
> (or at least entirely benign) for Arm in the first place. Only then I'll
> (hopefully) be in the position of putting a sensible comment here.
>
> One thing is clear, I suppose: Without the #ifdef the code wouldn't
> build. Yet imo if things all matched up, it should have been buildable
> either way already in the past. Hence the questions.
Right, it would not build. But does this check really matter even in the 
case where CONFIG_HAS_PIRQ=y? Looking at the code, it sounds like more 
an optimization/a way to return a different error code if there value is 
too high. For the domctl, it doesn't seem to be worth it, the more if we 
need to add #ifdef.
With that, the rest of the domctl should mostly work for Arm.

Cheers,

--
Julien Grall