Re: [PATCH for-4.19???] x86/physdev: replace physdev_{,un}map_pirq() checking against DOMID_SELF

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/06/2024 12:45 pm, Jan Beulich wrote:
> On 12.06.2024 13:05, Andrew Cooper wrote:
>> On 12/06/2024 9:44 am, Jan Beulich wrote:
>>> It's hardly ever correct to check for just DOMID_SELF, as guests have
>>> ways to figure out their domain IDs and hence could instead use those as
>>> inputs to respective hypercalls. Note, however, that for ordinary DomU-s
>>> the adjustment is relaxing things rather than tightening them, since
>>> - as a result of XSA-237 - the respective XSM checks would have rejected
>>> self (un)mapping attempts for other than the control domain.
>>>
>>> Since in physdev_map_pirq() handling overall is a little easier this
>>> way, move obtaining of the domain pointer into the caller. Doing the
>>> same for physdev_unmap_pirq() is just to keep both consistent in this
>>> regard. For both this has the advantage that it is now provable (by the
>>> build not failing) that there are no DOMID_SELF checks left (and none
>>> could easily be re-added).
>>>
>>> Fixes: 0b469cd68708 ("Interrupt remapping to PIRQs in HVM guests")
>>> Fixes: 9e1a3415b773 ("x86: fixes after emuirq changes")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> I think it is right to perform the domid lookup in do_physdev_op() and
>> pass d down into physdev_{un,}map_pirq().
>>
>> But I don't see what this has to do with the build failing.  You're not
>> undef-ing DOMID_SELF, so I don't see what kind of provability you've added.
> I'm talking of provability for the two functions in question. Not
> globally of course.

I'd suggest simply dropping the sentence.

I don't think it adds anything (both functions are trivially short) to
the overall message, and the "build breaking" part in particular is at
odds with the change.

>>> --- a/xen/arch/x86/physdev.c
>>> +++ b/xen/arch/x86/physdev.c
>>> @@ -184,6 +170,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
>>>  
>>>      switch ( cmd )
>>>      {
>>> +        struct domain *d;
>>> +
>> Please don't introduce any more of these.
>>
>> We've discussed several times about wanting to start using trivial
>> autovar init support, and every one of these additions is going to need
>> reverting.
>>
>> In this case, there's literally no difference having it at function scope.
> Will do; sorry, habits.

Thanks.

With that, and preferably the adjusted commit message, Reviewed-by:
Andrew Cooper <andrew.cooper3@citrix.com>