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

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

On 2024/9/3 14:09, Jan Beulich wrote:
> On 03.09.2024 06:01, Chen, Jiqian wrote:
>> On 2024/8/20 15:07, Jan Beulich wrote:
>>> On 20.08.2024 08:12, Chen, Jiqian wrote:
>>>> On 2024/8/19 17:08, Jan Beulich wrote:
>>>>> On 16.08.2024 13:08, Jiqian Chen wrote:
>>>>>> If run Xen with PVH dom0 and hvm domU, hvm will map a pirq for
>>>>>> a passthrough device by using gsi, see qemu code
>>>>>> xen_pt_realize->xc_physdev_map_pirq and libxl code
>>>>>> pci_add_dm_done->xc_physdev_map_pirq. Then xc_physdev_map_pirq
>>>>>> will call into Xen, but in hvm_physdev_op, PHYSDEVOP_map_pirq
>>>>>> is not allowed because currd is PVH dom0 and PVH has no
>>>>>> X86_EMU_USE_PIRQ flag, it will fail at has_pirq check.
>>>>>>
>>>>>> So, allow PHYSDEVOP_map_pirq when dom0 is PVH and also allow
>>>>>> iPHYSDEVOP_unmap_pirq for the removal device path to unmap pirq.
>>>>>> So that the interrupt of a passthrough device can be successfully
>>>>>> mapped to pirq for domU with a notion of PIRQ when dom0 is PVH.
>>>>>>
>>>>>> To exposing the functionality to wider than (presently) necessary
>>>>>> audience(like PVH domU), so it doesn't add any futher restrictions.
>>>>>
>>>>> The code change is fine, but I'm struggling with this sentence. I can't
>>>>> really derive what you're trying to say.
>>>> Ah, I wanted to explain why this path not add any further restrictions, 
>>>> then used your comments of last version.
>>>> How do I need to change this explanation?
>>>
>>> I think you want to take Roger's earlier comments (when he requested
>>> the relaxation) as basis to re-write (combine) both of the latter two
>>> paragraphs above (or maybe even all three of them). It's odd to first
>>> talk about Dom0, as if the operations were to be exposed just there,
>>> and only then add DomU-s.
>>
>> I tried to understand and summarize Roger's previous comments and changed 
>> commit message to the following. Do you think it is fine?
> 
> What are we talking about here? 
You had some concern about the description of commit message of this patch.
So I send a draft below to get your opinion.
If you forgot, I will directly send a new version(v14) later today.

> The patch was committed over a month ago?
Yes, I sent this v13 in Aug 16, and sorry to reply late.

> 
> Jan
> 
>> x86/pvh: Allow (un)map_pirq when dom0 is PVH
>>
>> 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.
>>
>> And the PVH domU which use vpci trying to issue a map_pirq will fail at the
>> xsm_map_domain_pirq() check in physdev_map_pirq() .
>>
>> 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.
>>
>>>
>>>>>> And there already are some senarios for domains without
>>>>>> X86_EMU_USE_PIRQ to use these functions.
>>>>>
>>>>> Are there? If so, pointing out an example may help.
>>>> If I understand correctly, Roger mentioned that PIRQs is disable by 
>>>> default for HVM guest("hvm_pirq=0") and passthrough device to guest.
>>>> In this scene, guest doesn't have PIRQs, but it still needs this hypercall.
>>>
>>> In which case please say so in order to be concrete, not vague.
>>>
>>> Jan
>>
> 

-- 
Best regards,
Jiqian Chen.