Re: [XEN PATCH v11 2/8] x86/pvh: Allow (un)map_pirq when dom0 is PVH

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2024/7/4 14:38, Jan Beulich wrote:
> On 04.07.2024 04:56, Chen, Jiqian wrote:
>> On 2024/7/2 16:44, Jan Beulich wrote:
>>> On 02.07.2024 05:15, Chen, Jiqian wrote:
>>>> On 2024/7/1 15:44, Jan Beulich wrote:
>>>>> On 30.06.2024 14:33, Jiqian Chen wrote:
>>>>>> --- a/xen/arch/x86/physdev.c
>>>>>> +++ b/xen/arch/x86/physdev.c
>>>>>> @@ -323,6 +323,13 @@ ret_t do_physdev_op(int cmd, 
>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>          if ( !d )
>>>>>>              break;
>>>>>>  
>>>>>> +        /* Prevent mapping when the subject domain has no 
>>>>>> X86_EMU_USE_PIRQ */
>>>>>> +        if ( is_hvm_domain(d) && !has_pirq(d) )
>>>>>> +        {
>>>>>> +            rcu_unlock_domain(d);
>>>>>> +            return -EOPNOTSUPP;
>>>>>> +        }
>>>>>> +
>>>>>>          ret = physdev_map_pirq(d, map.type, &map.index, &map.pirq, 
>>>>>> &msi);
>>>>>>  
>>>>>>          rcu_unlock_domain(d);
>>>>>> @@ -346,6 +353,13 @@ ret_t do_physdev_op(int cmd, 
>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>          if ( !d )
>>>>>>              break;
>>>>>>  
>>>>>> +        /* Prevent unmapping when the subject domain has no 
>>>>>> X86_EMU_USE_PIRQ */
>>>>>> +        if ( is_hvm_domain(d) && !has_pirq(d) )
>>>>>> +        {
>>>>>> +            rcu_unlock_domain(d);
>>>>>> +            return -EOPNOTSUPP;
>>>>>> +        }
>>>>>> +
>>>>>>          ret = physdev_unmap_pirq(d, unmap.pirq);
>>>>>>  
>>>>>>          rcu_unlock_domain(d);
>>>>>
>>>>> If you did go look, you will have noticed that we use "return" in the 
>>>>> middle
>>>>> of this function only very sparingly (when alternatives would result in 
>>>>> more
>>>>> complicated code elsewhere). I think you want to avoid "return" here, too,
>>>>> and probably go even further and avoid the extra rcu_unlock_domain() as 
>>>>> well.
>>>>> That's easily possible to arrange for (taking the latter case as example):
>>>>>
>>>>>         /* Prevent unmapping when the subject domain has no 
>>>>> X86_EMU_USE_PIRQ */
>>>>>         if ( !is_hvm_domain(d) || has_pirq(d) )
>>>>>             ret = physdev_unmap_pirq(d, unmap.pirq);
>>>>>         else
>>>>>             ret = -EOPNOTSUPP;
>>>>>
>>>>>         rcu_unlock_domain(d);
>>>>>
>>>>> Personally I would even use a conditional operator here, but I believe
>>>>> others might dislike its use in situations like this one.
>>>>>
>>>>> The re-arrangement make a little more noticeable though that the comment
>>>>> isn't quite right either: PV domains necessarily have no
>>>>> X86_EMU_USE_PIRQ. Maybe "... has no notion of pIRQ"?
>>>>
>>>> Or just like below?
>>>>
>>>>         /*
>>>>          * Prevent unmapping when the subject hvm domain has no
>>>>          * X86_EMU_USE_PIRQ
>>>>          */
>>>>         if ( is_hvm_domain(d) && !has_pirq(d) )
>>>>             ret = -EOPNOTSUPP;
>>>>         else
>>>>             ret = physdev_unmap_pirq(d, unmap.pirq);
>>>
>>> No objection to the slightly changed comment. The code alternative you
>>> present is of course functionally identical, yet personally I prefer to
>>> have the "good" case on the "if" branch and the "bad" one following
>>> "else". I wouldn't insist, though.
>> OK, will change "good" case on the "if" branch.
>> Do I need to change "!is_hvm_domain(d)" to "is_pv_domain(d)" ?
>> And then have:
>>
>>         /* Only unmapping when the subject domain has a notion of PIRQ */
>>         if ( is_pv_domain(d) || has_pirq(d) )
>>             ret = physdev_unmap_pirq(d, unmap.pirq);
>>         else
>>             ret = -EOPNOTSUPP;
> 
> I for one would prefer if you kept using is_hvm_domain(), for being more
> precise in this situation.
OK, thanks. Will change in next version.

> 
> Jan

-- 
Best regards,
Jiqian Chen.