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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

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

Jan