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
