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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.09.2024 00:51, Stefano Stabellini wrote:
> On Thu, 5 Sep 2024, Jan Beulich wrote:
>> On 05.09.2024 08:45, Chen, Jiqian wrote:
>>> HI,
>>>
>>> On 2024/9/4 14:04, Jan Beulich wrote:
>>>> On 04.09.2024 03:43, Stefano Stabellini wrote:
>>>>> On Tue, 3 Sep 2024, Jan Beulich wrote:
>>>>>> On 03.09.2024 12:53, Chen, Jiqian wrote:
>>>>>>> On 2024/9/3 17:25, Jan Beulich wrote:
>>>>>>>> On 03.09.2024 09:58, Chen, Jiqian wrote:
>>>>>>>>> On 2024/9/3 15:42, Jan Beulich wrote:
>>>>>>>>>> On 03.09.2024 09:04, Jiqian Chen wrote:
>>>>>>>>>>> When dom0 is PVH type and passthrough a device to HVM domU, Qemu 
>>>>>>>>>>> code
>>>>>>>>>>> xen_pt_realize->xc_physdev_map_pirq and libxl code pci_add_dm_done->
>>>>>>>>>>> xc_physdev_map_pirq map a pirq for passthrough devices.
>>>>>>>>>>> In xc_physdev_map_pirq call stack, function hvm_physdev_op has a 
>>>>>>>>>>> check
>>>>>>>>>>> has_pirq(currd), but currd is PVH dom0, PVH has no X86_EMU_USE_PIRQ 
>>>>>>>>>>> flag,
>>>>>>>>>>> so it fails, PHYSDEVOP_map_pirq is not allowed for PVH dom0 in 
>>>>>>>>>>> current
>>>>>>>>>>> codes.
>>>>>>>>>>>
>>>>>>>>>>> But it is fine to map interrupts through pirq to a HVM domain whose
>>>>>>>>>>> XENFEAT_hvm_pirqs is not enabled. Because pirq field is used as a 
>>>>>>>>>>> way to
>>>>>>>>>>> reference interrupts and it is just the way for the device model to
>>>>>>>>>>> identify which interrupt should be mapped to which domain, however
>>>>>>>>>>> has_pirq() is just to check if HVM domains route interrupts from
>>>>>>>>>>> devices(emulated or passthrough) through event channel, so, the 
>>>>>>>>>>> has_pirq()
>>>>>>>>>>> check should not be applied to the PHYSDEVOP_map_pirq issued by 
>>>>>>>>>>> dom0.
>>>>>>>>>>>
>>>>>>>>>>> So, allow PHYSDEVOP_map_pirq when dom0 is PVH and also allow
>>>>>>>>>>> PHYSDEVOP_unmap_pirq for the removal device path to unmap pirq. 
>>>>>>>>>>> Then the
>>>>>>>>>>> interrupt of a passthrough device can be successfully mapped to 
>>>>>>>>>>> pirq for domU.
>>>>>>>>>>
>>>>>>>>>> As before: When you talk about just Dom0, ...
>>>>>>>>>>
>>>>>>>>>>> --- a/xen/arch/x86/hvm/hypercall.c
>>>>>>>>>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>>>>>>>>>> @@ -73,6 +73,8 @@ long hvm_physdev_op(int cmd, 
>>>>>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>>>>>      {
>>>>>>>>>>>      case PHYSDEVOP_map_pirq:
>>>>>>>>>>>      case PHYSDEVOP_unmap_pirq:
>>>>>>>>>>> +        break;
>>>>>>>>>>> +
>>>>>>>>>>>      case PHYSDEVOP_eoi:
>>>>>>>>>>>      case PHYSDEVOP_irq_status_query:
>>>>>>>>>>>      case PHYSDEVOP_get_free_pirq:
>>>>>>>>>>
>>>>>>>>>> ... that ought to match the code. IOW you've again lost why it is 
>>>>>>>>>> okay(ish)
>>>>>>>>>> (or even necessary) to also permit the op for non-Dom0 (e.g. a PVH 
>>>>>>>>>> stubdom).
>>>>>>>>>> Similarly imo Dom0 using this on itself wants discussing.
>>>>>>>>> Do you mean I need to talk about why permit this op for all HVM
>>>>>>>>
>>>>>>>> You don't need to invent reasons, but it needs making clear that wider 
>>>>>>>> than
>>>>>>>> necessary (for your purpose) exposure is at least not going to be a 
>>>>>>>> problem.
>>>>>>>>
>>>>>>>>> and  where can prevent PVH domain calling this for self-mapping, not 
>>>>>>>>> only dom0?
>>>>>>>>
>>>>>>>> Aiui use on itself is limited to Dom0, so only that would need 
>>>>>>>> clarifying
>>>>>>>> (along the lines of the above, i.e. that/why it is not a problem). For
>>>>>>>> has_pirq() domains use on oneself was already permitted before.
>>>>>>>
>>>>>>> Changed commit message to below. Please check and that will be great 
>>>>>>> helpful if you would show me how to modify it.
>>>>>>> {
>>>>>>> x86/pvh: Allow (un)map_pirq when dom0 is PVH
>>>>>>>
>>>>>>> Problem: when dom0 is PVH type and passthrough a device to HVM domU, 
>>>>>>> Qemu
>>>>>>> code xen_pt_realize->xc_physdev_map_pirq and libxl code 
>>>>>>> pci_add_dm_done->
>>>>>>> xc_physdev_map_pirq map a pirq for passthrough devices.
>>>>>>> In xc_physdev_map_pirq call stack, function hvm_physdev_op has a check
>>>>>>> has_pirq(currd), but currd is PVH dom0, PVH has no X86_EMU_USE_PIRQ 
>>>>>>> flag,
>>>>>>> so it fails, PHYSDEVOP_map_pirq is not allowed for PVH dom0 in current
>>>>>>> codes.
>>>>>>>
>>>>>>> To solve above problem, need to remove the chack has_pirq() for that
>>>>>>> situation(PHYSDEVOP_map_pirq is issued by dom0 for domUs). But without
>>>>>>> adding other restrictions, PHYSDEVOP_map_pirq will be allowed wider than
>>>>>>> what the problem need.
>>>>>>> So, clarify below:
>>>>>>>
>>>>>>> For HVM domUs whose XENFEAT_hvm_pirqs is not enabled,it is fine to map
>>>>>>> interrupts through pirq for them. Because pirq field is used as a way to
>>>>>>> reference interrupts and it is just the way for the device model to
>>>>>>> identify which interrupt should be mapped to which domain, however
>>>>>>> has_pirq() is just to check if HVM domains route interrupts from
>>>>>>> devices(emulated or passthrough) through event channel, so, remove
>>>>>>> has_pirq() check has no impact on HVM domUs.
>>>>>>>
>>>>>>> For PVH domUs that performs such an operation will fail at the check
>>>>>>> xsm_map_dedomain_pirq() in physdev_map-nirq().
>>>>>>>
>>>>>>> For PVH dom0, it uses vpci and doesn't use event channel, as above 
>>>>>>> talks,
>>>>>>> it also has no impact.
>>>>>>> }
>>>>>>
>>>>>> This is better than what you had before, and I don't really fancy re-
>>>>>> writing the description effectively from scratch. So let's just go from
>>>>>> there. As to the "excess" permission for the sub-ops: The main thing I'm
>>>>>> after is that it be clarified that we're not going to introduce any
>>>>>> security issues here. That requires auditing the code, and merely saying
>>>>>> "also has no impact" is a little too little for my taste. For Dom0 an
>>>>>> argument may be that it is overly powerful already anyway, even if for
>>>>>> PVH we're a little more strict than for PV (I think).
>>>>>
>>>>> Hi Jan, for clarity and to make this actionable, are you suggesting to
>>>>> clarify the commit message by adding wording around "Dom0 is overly
>>>>> powerful already anyway so it is OK so this is OK" ?
>>>>
>>>> Yes, perhaps with "deemed" added. 
>>> OK, for PVH dom0, I will change to " Dom0 is deemed overly powerful already 
>>> anyway, so it is OK "
>>
>> I don't mind the deemed as you add it, but the important place to add it
>> here is before "OK". I'm sorry, it didn't occur to me that after all the
>> prior discussion this earlier reply of mine could still be mis-interpreted.
>>
>>>> And text for DomU-s similarly extended, as the pointing at the XSM check 
>>>> is presently incomplete (stubdom-s can
>>>> pass that check, after all, as can de-priv qemu running in Dom0).
>>> So sorry, I know so little about this, I can't explain these situations, 
>>> could you tell me how to describe or help me write a paragraph?
>>
>> I'm afraid that in order to make (propose) such a change you need to be
>> able to explain why it is okay to expose functionality beyond where it's
>> presently exposed. It's not just writing a new paragraph that's needed
>> here. You first need to _check_ that what you do is okay. And once you've
>> done that checking, you then summarize that in writing.
>  
> 
> PHYSDEVOP_map_pirq ends up calling physdev_map_pirq which is protected
> by:
> 
>     ret = xsm_map_domain_pirq(XSM_DM_PRIV, d);
>     if ( ret )
>         return ret;
> 
> Dom0 having permission to do PHYSDEVOP_map_pirq even without has_pirq is
> fine. Device models are also OK because the code we are trying to enable
> is in fact part of the device model. If someone were to run an HVM
> stubdom they might need this patch as well.
> 
> If PHYSDEVOP_map_pirq is allowed, also PHYSDEVOP_unmap_pirq should be
> allowed.
> 
> Is this explanation OK?

This still solely focuses on why the functionality is wanted. There
continues to be nothing about the wider exposure actually being safe.

Jan