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.
And there already are some senarios for domains without
X86_EMU_USE_PIRQ to use these functions.

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
v12->v13 changes:
Removed the PHYSDEVOP_(un)map_pirq restriction check for pvh domU and added a corresponding description in the commit message.

v11->v12 changes:
Avoid using return, set error code instead when (un)map is not allowed.

v10->v11 changes:
Delete the judgment of "d==currd", so that we can prevent physdev_(un)map_pirq from being executed when domU has no pirq, instead of just preventing self-mapping.
And modify the description of the commit message accordingly.

v9->v10 changes:
Indent the comments above PHYSDEVOP_map_pirq according to the code style.

v8->v9 changes:
Add a comment above PHYSDEVOP_map_pirq to describe why need this hypercall.
Change "!is_pv_domain(d)" to "is_hvm_domain(d)", and "map.domid == DOMID_SELF" to "d == current->domian".

v7->v8 changes:
Add the domid check(domid == DOMID_SELF) to prevent self map when guest doesn't use pirq.
That check was missed in the previous version.

v6->v7 changes:
Nothing.

v5->v6 changes:
Nothing.

v4->v5 changes:
Move the check of self map_pirq to physdev.c, and change to check if the caller has PIRQ flag, and just break for PHYSDEVOP_(un)map_pirq in hvm_physdev_op.

v3->v4 changes:
add check to prevent PVH self map.

v2->v3 changes:
Du to changes in the implementation of the second patch on kernel side(that it will do setup_gsi and map_pirq when assigning a device to passthrough), add PHYSDEVOP_setup_gsi for PVH dom0, and we need to support self mapping.
---
 xen/arch/x86/hvm/hypercall.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c
index 68815b03eb25..0b7fc060b4e2 100644
--- 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:
-- 
2.34.1

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.

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

Jan

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?

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

> 
> Jan

-- 
Best regards,
Jiqian Chen.

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.

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

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?

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.

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? The patch was committed over a month
ago?

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
>

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.

On 03.09.2024 08:20, Chen, Jiqian wrote:
> 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.

I still don't get it. Yes, the patch could have done with a better
description, but ...

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

... it's simply too late now.

Jan