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

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

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.
}

> 
> Jan

-- 
Best regards,
Jiqian Chen.