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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.07.2024 10:51, Roger Pau Monné wrote:
> On Wed, Jul 31, 2024 at 10:40:46AM +0200, Jan Beulich wrote:
>> On 31.07.2024 10:24, Roger Pau Monné wrote:
>>> On Wed, Jul 31, 2024 at 09:58:28AM +0200, Jan Beulich wrote:
>>>> On 31.07.2024 09:50, Roger Pau Monné wrote:
>>>>> On Mon, Jul 08, 2024 at 07:41:19PM +0800, Jiqian Chen wrote:
>>>>>> --- a/xen/arch/x86/physdev.c
>>>>>> +++ b/xen/arch/x86/physdev.c
>>>>>> @@ -323,7 +323,11 @@ ret_t do_physdev_op(int cmd, 
>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>          if ( !d )
>>>>>>              break;
>>>>>>  
>>>>>> -        ret = physdev_map_pirq(d, map.type, &map.index, &map.pirq, 
>>>>>> &msi);
>>>>>> +        /* Only mapping when the subject domain has a notion of PIRQ */
>>>>>> +        if ( !is_hvm_domain(d) || has_pirq(d) )
>>>>>
>>>>> I'm afraid this is not true.  It's fine to map interrupts to HVM
>>>>> domains that don't have XENFEAT_hvm_pirqs enabled.  has_pirq() simply
>>>>> allow HVM domains to route interrupts from devices (either emulated or
>>>>> passed through) over event channels.
>>>>>
>>>>> It might have worked in the past (when using a version of Xen < 4.19)
>>>>> because XENFEAT_hvm_pirqs was enabled by default for HVM guests.
>>>>>
>>>>> physdev_map_pirq() will work fine when used against domains that don't
>>>>> have XENFEAT_hvm_pirqs enabled, and it needs to be kept this way.
>>>>>
>>>>> I think you want to allow PHYSDEVOP_{,un}map_pirq for HVM domains, but
>>>>> keep the code in do_physdev_op() as-is.  You will have to check
>>>>> whether the current paths in do_physdev_op() are not making
>>>>> assumptions about XENFEAT_hvm_pirqs being enabled when the calling
>>>>> domain is of HVM type.  I don't think that's the case, but better
>>>>> check.
>>>>
>>>> Yet the goal is to disallow mapping into PVH domains. The use of
>>>> has_pirq() was aiming at that. If that predicate can't be used (anymore)
>>>> for this purpose, which one is appropriate now?
>>>
>>> Why do you want to add such restriction now, when it's not currently
>>> present?
>>>
>>> It was already the case that a PV dom0 could issue
>>> PHYSDEVOP_{,un}map_pirq operations against a PVH domU, whatever the
>>> result of such operation be.
>>
>> Because (a) that was wrong and (b) we'd suddenly permit a PVH DomU to
>> issue such for itself.
> 
> Regarding (b) a PVH domU issuing such operations would fail at the
> xsm_map_domain_pirq() check in physdev_map_pirq().

Hmm, yes, fair point.

> I agree with (a), but I don't think enabling PVH dom0 usage of the
> hypercalls should be gated on this.  As said a PV dom0 is already
> capable of issuing PHYSDEVOP_{,un}map_pirq operations against a PVH
> domU.

Okay, I can accept that as an intermediate position. We ought to deny
such requests at some point though for PVH domains, the latest in the
course of making vPCI work there.

Jan