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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.07.2023 17:24, Jan Beulich wrote:
> 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.

The more I think about it, the better this option looks to me. libxl
doesn't use the sub-op for Arm (from all I can tell), so the only
worrying in-tree parts are that the libxc functions is exposed both
via the Python and OCamL bindings (without there being an in-tree
consumer, again from all I can tell).

Since I'd like to get v3 out (first and foremost because meanwhile
I've also found bugs fixing of which preferably would take this
change as a prereq, or else the build would break on Arm afaict), I'd
appreciate feedback (of course also from anyone else on the Cc list).
I guess unless I hear otherwise, I'll make the change.

Jan