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.
