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

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

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 "

> 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?

> 
> Jan

-- 
Best regards,
Jiqian Chen.