If we run Xen with PVH dom0 and hvm domU, hvm will map a pirq for
a passthrough device by using gsi, see xen_pt_realize->xc_physdev_map_pirq
and 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, I think we may need to allow PHYSDEVOP_map_pirq when currd is dom0 (at
present dom0 is PVH).

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---
 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 6ad5b4d5f1..f9c4a2243a 100644
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -74,6 +74,8 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     {
     case PHYSDEVOP_map_pirq:
     case PHYSDEVOP_unmap_pirq:
+        if (is_hardware_domain(currd))
+            break;
     case PHYSDEVOP_eoi:
     case PHYSDEVOP_irq_status_query:
     case PHYSDEVOP_get_free_pirq:
-- 
2.34.1

On 24.11.2023 11:41, Jiqian Chen wrote:
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -74,6 +74,8 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>      {
>      case PHYSDEVOP_map_pirq:
>      case PHYSDEVOP_unmap_pirq:
> +        if (is_hardware_domain(currd))
> +            break;
>      case PHYSDEVOP_eoi:
>      case PHYSDEVOP_irq_status_query:
>      case PHYSDEVOP_get_free_pirq:

If you wouldn't go the route suggested by Roger, I think you will need
to deny self-mapping requests here.

Also note that both here and in patch 1 you will want to adjust a number
of style violations.

Jan

On 2023/11/28 23:14, Jan Beulich wrote:
> On 24.11.2023 11:41, Jiqian Chen wrote:
>> --- a/xen/arch/x86/hvm/hypercall.c
>> +++ b/xen/arch/x86/hvm/hypercall.c
>> @@ -74,6 +74,8 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>      {
>>      case PHYSDEVOP_map_pirq:
>>      case PHYSDEVOP_unmap_pirq:
>> +        if (is_hardware_domain(currd))
>> +            break;
>>      case PHYSDEVOP_eoi:
>>      case PHYSDEVOP_irq_status_query:
>>      case PHYSDEVOP_get_free_pirq:
> 
> If you wouldn't go the route suggested by Roger, I think you will need
> to deny self-mapping requests here.
Do you mean below?
if (arg.domid == DOMID_SELF)
	return;

> 
> Also note that both here and in patch 1 you will want to adjust a number
> of style violations.
Could you please descript in detail? This will greatly assist me in making modifications in the next version. Thank you!

> 
> Jan

-- 
Best regards,
Jiqian Chen.

On 30.11.2023 07:44, Chen, Jiqian wrote:
> On 2023/11/28 23:14, Jan Beulich wrote:
>> On 24.11.2023 11:41, Jiqian Chen wrote:
>>> --- a/xen/arch/x86/hvm/hypercall.c
>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>> @@ -74,6 +74,8 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>      {
>>>      case PHYSDEVOP_map_pirq:
>>>      case PHYSDEVOP_unmap_pirq:
>>> +        if (is_hardware_domain(currd))
>>> +            break;
>>>      case PHYSDEVOP_eoi:
>>>      case PHYSDEVOP_irq_status_query:
>>>      case PHYSDEVOP_get_free_pirq:
>>
>> If you wouldn't go the route suggested by Roger, I think you will need
>> to deny self-mapping requests here.
> Do you mean below?
> if (arg.domid == DOMID_SELF)
> 	return;

That's part of it, yes. You'd also need to check for the actual domain ID of
the caller domain.

>> Also note that both here and in patch 1 you will want to adjust a number
>> of style violations.
> Could you please descript in detail? This will greatly assist me in making modifications in the next version. Thank you!

Well, in the code above you're missing blanks inside the if(). Please see
./CODING_STYLE.

Jan

On 2023/11/30 17:00, Jan Beulich wrote:
> On 30.11.2023 07:44, Chen, Jiqian wrote:
>> On 2023/11/28 23:14, Jan Beulich wrote:
>>> On 24.11.2023 11:41, Jiqian Chen wrote:
>>>> --- a/xen/arch/x86/hvm/hypercall.c
>>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>>> @@ -74,6 +74,8 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>      {
>>>>      case PHYSDEVOP_map_pirq:
>>>>      case PHYSDEVOP_unmap_pirq:
>>>> +        if (is_hardware_domain(currd))
>>>> +            break;
>>>>      case PHYSDEVOP_eoi:
>>>>      case PHYSDEVOP_irq_status_query:
>>>>      case PHYSDEVOP_get_free_pirq:
>>>
>>> If you wouldn't go the route suggested by Roger, I think you will need
>>> to deny self-mapping requests here.
>> Do you mean below?
>> if (arg.domid == DOMID_SELF)
>> 	return;
> 
> That's part of it, yes. You'd also need to check for the actual domain ID of
> the caller domain.
I will add more check in next version.

> 
>>> Also note that both here and in patch 1 you will want to adjust a number
>>> of style violations.
>> Could you please descript in detail? This will greatly assist me in making modifications in the next version. Thank you!
> 
> Well, in the code above you're missing blanks inside the if(). Please see
> ./CODING_STYLE.
Thank you very much! I will check and modify all my patches to meet the Xen code style in next version.

> 
> Jan

-- 
Best regards,
Jiqian Chen.

Hi Jan Beulich,

On 2023/11/30 14:44, Chen, Jiqian wrote:
> 
> On 2023/11/28 23:14, Jan Beulich wrote:
>> On 24.11.2023 11:41, Jiqian Chen wrote:
>>> --- a/xen/arch/x86/hvm/hypercall.c
>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>> @@ -74,6 +74,8 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>      {
>>>      case PHYSDEVOP_map_pirq:
>>>      case PHYSDEVOP_unmap_pirq:
>>> +        if (is_hardware_domain(currd))
>>> +            break;
>>>      case PHYSDEVOP_eoi:
>>>      case PHYSDEVOP_irq_status_query:
>>>      case PHYSDEVOP_get_free_pirq:
>>
>> If you wouldn't go the route suggested by Roger, I think you will need
>> to deny self-mapping requests here.
> Do you mean below?
> if (arg.domid == DOMID_SELF)
> 	return;
> 
>>
>> Also note that both here and in patch 1 you will want to adjust a number
>> of style violations.
> Could you please descript in detail? This will greatly assist me in making modifications in the next version. Thank you!
Oh! Do you mean there are many code style problems that not satisfiable for CODING_STYLE of Xen in my codes?
Thank Xenia for reminder.

> 
>>
>> Jan
> 

-- 
Best regards,
Jiqian Chen.

On Fri, Nov 24, 2023 at 06:41:35PM +0800, Jiqian Chen wrote:
> If we run Xen with PVH dom0 and hvm domU, hvm will map a pirq for
> a passthrough device by using gsi, see xen_pt_realize->xc_physdev_map_pirq
> and 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, I think we may need to allow PHYSDEVOP_map_pirq when currd is dom0 (at

And PHYSDEVOP_unmap_pirq also?

> present dom0 is PVH).

IMO it would be better to implement a new set of DMOP hypercalls that
handle the setup of interrupts from physical devices that are assigned
to a guest.  That should allow us to get rid of the awkward PHYSDEVOP
+ XEN_DOMCTL_{,un}bind_pt_irq hypercall interface, which currently
prevents QEMU from being hypervisor version agnostic (since the domctl
interface is not stable).

I understand this might be too much to ask for, but something to take
into account.

Thanks, Roger.

On 2023/11/28 22:17, Roger Pau Monné wrote:
> On Fri, Nov 24, 2023 at 06:41:35PM +0800, Jiqian Chen wrote:
>> If we run Xen with PVH dom0 and hvm domU, hvm will map a pirq for
>> a passthrough device by using gsi, see xen_pt_realize->xc_physdev_map_pirq
>> and 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, I think we may need to allow PHYSDEVOP_map_pirq when currd is dom0 (at
> 
> And PHYSDEVOP_unmap_pirq also?
Yes, in the failed path, PHYSDEVOP_unmap_pirq will be called. I will add some descriptions about it into the commit message.

> 
>> present dom0 is PVH).
> 
> IMO it would be better to implement a new set of DMOP hypercalls that
> handle the setup of interrupts from physical devices that are assigned
> to a guest.  That should allow us to get rid of the awkward PHYSDEVOP
> + XEN_DOMCTL_{,un}bind_pt_irq hypercall interface, which currently
> prevents QEMU from being hypervisor version agnostic (since the domctl
> interface is not stable).
> 
> I understand this might be too much to ask for, but something to take
> into account.
Yes, that will be a complex project. I think current change can meet the needs. We can take DMOP into account in the future. Thank you.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.

On Thu, Nov 30, 2023 at 06:32:00AM +0000, Chen, Jiqian wrote:
> 
> On 2023/11/28 22:17, Roger Pau Monné wrote:
> > On Fri, Nov 24, 2023 at 06:41:35PM +0800, Jiqian Chen wrote:
> >> If we run Xen with PVH dom0 and hvm domU, hvm will map a pirq for
> >> a passthrough device by using gsi, see xen_pt_realize->xc_physdev_map_pirq
> >> and 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, I think we may need to allow PHYSDEVOP_map_pirq when currd is dom0 (at
> > 
> > And PHYSDEVOP_unmap_pirq also?
> Yes, in the failed path, PHYSDEVOP_unmap_pirq will be called. I will add some descriptions about it into the commit message.
> 
> > 
> >> present dom0 is PVH).
> > 
> > IMO it would be better to implement a new set of DMOP hypercalls that
> > handle the setup of interrupts from physical devices that are assigned
> > to a guest.  That should allow us to get rid of the awkward PHYSDEVOP
> > + XEN_DOMCTL_{,un}bind_pt_irq hypercall interface, which currently
> > prevents QEMU from being hypervisor version agnostic (since the domctl
> > interface is not stable).
> > 
> > I understand this might be too much to ask for, but something to take
> > into account.
> Yes, that will be a complex project. I think current change can meet the needs. We can take DMOP into account in the future. Thank you.

The issue with this approach is that we always do things in a rush and
cut corners, and then never pay back the debt.  Anyway, I'm not going
to block this, and I'm not blaming you.

Sadly this is just focused on getting something working in the short
term rather than thinking long term in a maintainable interface.

Regards, Roger.