Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

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

On 2023/12/15 06:49, Stefano Stabellini wrote:
> On Thu, 14 Dec 2023, Roger Pau Monné wrote:
>> On Thu, Dec 14, 2023 at 10:58:24AM +0100, Jan Beulich wrote:
>>> On 14.12.2023 10:55, Roger Pau Monné wrote:
>>>> On Thu, Dec 14, 2023 at 08:55:45AM +0000, Chen, Jiqian wrote:
>>>>> On 2023/12/13 15:03, Jan Beulich wrote:
>>>>>> On 13.12.2023 03:47, Chen, Jiqian wrote:
>>>>>>> On 2023/12/12 17:30, Jan Beulich wrote:
>>>>>>>> On 12.12.2023 07:49, Chen, Jiqian wrote:
>>>>>>>>> On 2023/12/11 23:31, Roger Pau Monné wrote:
>>>>>>>>>> On Mon, Dec 11, 2023 at 12:40:08AM +0800, Jiqian Chen wrote:
>>>>>>>>>>> --- a/xen/arch/x86/hvm/hypercall.c
>>>>>>>>>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>>>>>>>>>> @@ -72,8 +72,11 @@ long hvm_physdev_op(int cmd, 
>>>>>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>>>>>>>  
>>>>>>>>>>>      switch ( cmd )
>>>>>>>>>>>      {
>>>>>>>>>>> +    case PHYSDEVOP_setup_gsi:
>>>>>>>>>>
>>>>>>>>>> I think given the new approach on the Linux side patches, where
>>>>>>>>>> pciback will configure the interrupt, there's no need to expose
>>>>>>>>>> setup_gsi anymore?
>>>>>>>>> The latest patch(the second patch of v3 on kernel side) does 
>>>>>>>>> setup_gsi and map_pirq for passthrough device in pciback, so we need 
>>>>>>>>> this and below.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>      case PHYSDEVOP_map_pirq:
>>>>>>>>>>>      case PHYSDEVOP_unmap_pirq:
>>>>>>>>>>> +        if ( is_hardware_domain(currd) )
>>>>>>>>>>> +            break;
>>>>>>>>>>
>>>>>>>>>> Also Jan already pointed this out in v2: this hypercall needs to be
>>>>>>>>>> limited so a PVH dom0 cannot execute it against itself.  IOW: refuse
>>>>>>>>>> the hypercall if DOMID_SELF or the passed domid matches the current
>>>>>>>>>> domain domid.
>>>>>>>>> Yes, I remember Jan's suggestion, but since the latest patch(the 
>>>>>>>>> second patch of v3 on kernel side) has change the implementation, it 
>>>>>>>>> does setup_gsi and map_pirq for dom0 itself, so I didn't add the 
>>>>>>>>> DOMID_SELF check.
>>>>>>>>
>>>>>>>> And why exactly would it do specifically the map_pirq? (Even the 
>>>>>>>> setup_gsi
>>>>>>>> looks questionable to me, but there might be reasons there.)
>>>>>>> Map_pirq is to solve the check failure problem. (pci_add_dm_done-> 
>>>>>>> xc_domain_irq_permission-> XEN_DOMCTL_irq_permission-> 
>>>>>>> pirq_access_permitted->domain_pirq_to_irq->return irq is 0)
>>>>>>> Setup_gsi is because the gsi is never be unmasked, so the gsi is never 
>>>>>>> be registered( vioapic_hwdom_map_gsi-> mp_register_gsi is never be 
>>>>>>> called).
>>>>>>
>>>>>> And it was previously made pretty clear by Roger, I think, that doing a 
>>>>>> "map"
>>>>>> just for the purpose of granting permission is, well, at best a temporary
>>>>>> workaround in the early development phase. If there's presently no 
>>>>>> hypercall
>>>>>> to _only_ grant permission to IRQ, we need to add one.
>>>>> Could you please describe it in detail? Do you mean to add a new 
>>>>> hypercall to grant irq access for dom0 or domU?
>>>>> It seems XEN_DOMCTL_irq_permission is the hypercall to grant irq access 
>>>>> from dom0 to domU(see XEN_DOMCTL_irq_permission-> irq_permit_access). 
>>>>> There is no need to add hypercall to grant irq access.
>>>>> We failed here (XEN_DOMCTL_irq_permission-> 
>>>>> pirq_access_permitted->domain_pirq_to_irq->return irq is 0) is because 
>>>>> the PVH dom0 didn't use PIRQ, so we can't get irq from pirq if "current" 
>>>>> is PVH dom0.
>>>>
>>>> One way to bodge this would be to detect whether the caller of
>>>> XEN_DOMCTL_irq_permission is a PV or an HVM domain, and in case of HVM
>>>> assume the pirq field is a GSI.  I'm unsure however how that will work
>>>> with non-x86 architectures.
> 
> PIRQ is an x86-only concept. We have event channels but no PIRQs on ARM.
> I expect RISC-V will be the same.
> 
> 
>>>> It would  be better to introduce a new XEN_DOMCTL_gsi_permission, or
> 
> "GSI" is another x86-only concept.
> 
> So actually the best name was indeed XEN_DOMCTL_irq_permission, given
> that it is using the more arch-neutral "irq" terminology.
> 
> Perhaps it was always a mistake to pass PIRQs to
> XEN_DOMCTL_irq_permission and we should always have passed the real
> interrupt number (GSI on x86, SPI on ARM).
> 
> So your "bodge" is actually kind of OK in my opinion. Basically everyone
> else (x86 HVM/PVH, ARM, RISC-V, probably PPC too) will use
> XEN_DOMCTL_irq_permission with hardware interrupt numbers (GSIs, SPIs,
> etc.), the only special case is x86 PV. It is x86 PV the odd one.
> 
> Given that DOMCTL is an unstable interface anyway, I feel OK making
> changes to it, even better if backward compatible.
I try to understand your discussion about the modification of 
XEN_DOMCTL_irq_permission. At the xl level, gsi needs to be passed in instead 
of pirq, and then a judgment is added to XEN_DOMCTL_irq_permission, just like 
the implementation below?
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index d3507d13a029..f665d17afbf5 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -1486,6 +1486,7 @@ static void pci_add_dm_done(libxl__egc *egc,
         goto out_no_irq;
     }
     if ((fscanf(f, "%u", &irq) == 1) && irq) {
+        int gsi = irq;
         r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
         if (r < 0) {
             LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d (error=%d)",
@@ -1494,7 +1495,7 @@ static void pci_add_dm_done(libxl__egc *egc,
             rc = ERROR_FAIL;
             goto out;
         }
-        r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
+        r = xc_domain_irq_permission(ctx->xch, domid, gsi, 1);
         if (r < 0) {
             LOGED(ERROR, domainid,
                   "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index f5a71ee5f78d..782c4a7a70a4 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -658,7 +658,12 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
u_domctl)
             ret = -EINVAL;
             break;
         }
-        irq = pirq_access_permitted(current->domain, pirq);
+
+        if ( is_hvm_domain(current->domain) )
+            irq = pirq;
+        else
+            irq = pirq_access_permitted(current->domain, pirq);
+
         if ( !irq || xsm_irq_permission(XSM_HOOK, d, irq, allow) )
             ret = -EPERM;
         else if ( allow )

-- 
Best regards,
Jiqian Chen.