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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.07.2023 11:28, Julien Grall wrote:
> 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.

Thanks for clarifying.

>>>> --- 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.

I wouldn't call it an optimization. The check has always been there, with
b72cea07db32 transforming it (largely) into what we have today. In fact
in an initial attempt I had gone further and also #ifdef-ed out the
pirq_access_permitted() (and iirc the pirq variable altogether), seeing
that without HAS_PIRQ the incoming field can only sensibly hold an IRQ.
But then I thought that this would be going too far, leading to me
undoing part of the change.

If we dropped the check, we'd start relying on domain_pirq_to_irq()
(invoked by pirq_access_permitted()) to always fail cleanly for an out-
of-range input. While I think that holds, it would still feel a little
like making the code (slightly) less robust. But yes, I think doing so
would be an option. (Still I also think that returning EINVAL for
obviously out-of-range values is somewhat better than EPERM.) Yet then
...

> With that, the rest of the domctl should mostly work for Arm.

..., taking into account also your clarification at the top, I wonder
whether we shouldn't #ifdef out the entire subop.

Jan