Some type of domain don't have PIRQ, like PVH, when
passthrough a device to guest on PVH dom0, callstack
pci_add_dm_done->XEN_DOMCTL_irq_permission will failed
at domain_pirq_to_irq.

So, add a new hypercall to grant/revoke gsi permission
when dom0 is not PV or dom0 has not PIRQ flag.

Signed-off-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
 tools/include/xenctrl.h      |  5 +++
 tools/libs/ctrl/xc_domain.c  | 15 ++++++++
 tools/libs/light/libxl_pci.c | 72 ++++++++++++++++++++++++++++--------
 xen/arch/x86/domctl.c        | 31 ++++++++++++++++
 xen/include/public/domctl.h  |  9 +++++
 xen/xsm/flask/hooks.c        |  1 +
 6 files changed, 117 insertions(+), 16 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 841db41ad7e4..c21a79d74be3 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1382,6 +1382,11 @@ int xc_domain_irq_permission(xc_interface *xch,
                              uint32_t pirq,
                              bool allow_access);
 
+int xc_domain_gsi_permission(xc_interface *xch,
+                             uint32_t domid,
+                             uint32_t gsi,
+                             bool allow_access);
+
 int xc_domain_iomem_permission(xc_interface *xch,
                                uint32_t domid,
                                unsigned long first_mfn,
diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
index f2d9d14b4d9f..8540e84fda93 100644
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -1394,6 +1394,21 @@ int xc_domain_irq_permission(xc_interface *xch,
     return do_domctl(xch, &domctl);
 }
 
+int xc_domain_gsi_permission(xc_interface *xch,
+                             uint32_t domid,
+                             uint32_t gsi,
+                             bool allow_access)
+{
+    struct xen_domctl domctl = {
+        .cmd = XEN_DOMCTL_gsi_permission,
+        .domain = domid,
+        .u.gsi_permission.gsi = gsi,
+        .u.gsi_permission.allow_access = allow_access,
+    };
+
+    return do_domctl(xch, &domctl);
+}
+
 int xc_domain_iomem_permission(xc_interface *xch,
                                uint32_t domid,
                                unsigned long first_mfn,
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 7e44d4c3ae2b..1d1b81dd2844 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -1412,6 +1412,37 @@ static bool pci_supp_legacy_irq(void)
 #define PCI_SBDF(seg, bus, devfn) \
             ((((uint32_t)(seg)) << 16) | (PCI_DEVID(bus, devfn)))
 
+static int pci_device_set_gsi(libxl_ctx *ctx,
+                              libxl_domid domid,
+                              libxl_device_pci *pci,
+                              bool map,
+                              int *gsi_back)
+{
+    int r, gsi, pirq;
+    uint32_t sbdf;
+
+    sbdf = PCI_SBDF(pci->domain, pci->bus, (PCI_DEVFN(pci->dev, pci->func)));
+    r = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
+    *gsi_back = r;
+    if (r < 0)
+        return r;
+
+    gsi = r;
+    pirq = r;
+    if (map)
+        r = xc_physdev_map_pirq(ctx->xch, domid, gsi, &pirq);
+    else
+        r = xc_physdev_unmap_pirq(ctx->xch, domid, pirq);
+    if (r)
+        return r;
+
+    r = xc_domain_gsi_permission(ctx->xch, domid, gsi, map);
+    if (r && errno == EOPNOTSUPP)
+        r = xc_domain_irq_permission(ctx->xch, domid, gsi, map);
+
+    return r;
+}
+
 static void pci_add_dm_done(libxl__egc *egc,
                             pci_add_state *pas,
                             int rc)
@@ -1424,10 +1455,10 @@ static void pci_add_dm_done(libxl__egc *egc,
     unsigned long long start, end, flags, size;
     int irq, i;
     int r;
-    uint32_t sbdf;
     uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
     uint32_t domainid = domid;
     bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid);
+    int gsi;
 
     /* Convenience aliases */
     bool starting = pas->starting;
@@ -1485,6 +1516,19 @@ static void pci_add_dm_done(libxl__egc *egc,
     fclose(f);
     if (!pci_supp_legacy_irq())
         goto out_no_irq;
+
+    r = pci_device_set_gsi(ctx, domid, pci, 1, &gsi);
+    if (gsi >= 0) {
+        if (r < 0) {
+            rc = ERROR_FAIL;
+            LOGED(ERROR, domainid,
+                  "pci_device_set_gsi gsi=%d (error=%d)", gsi, errno);
+            goto out;
+        } else {
+            goto process_permissive;
+        }
+    }
+    /* if gsi < 0, keep using irq */
     sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
                                 pci->bus, pci->dev, pci->func);
     f = fopen(sysfs_path, "r");
@@ -1493,13 +1537,6 @@ static void pci_add_dm_done(libxl__egc *egc,
         goto out_no_irq;
     }
     if ((fscanf(f, "%u", &irq) == 1) && irq) {
-        sbdf = PCI_SBDF(pci->domain, pci->bus,
-                        (PCI_DEVFN(pci->dev, pci->func)));
-        r = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
-        /* if fail, keep using irq; if success, r is gsi, use gsi */
-        if (r != -1) {
-            irq = r;
-        }
         r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
         if (r < 0) {
             LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d (error=%d)",
@@ -1519,6 +1556,7 @@ static void pci_add_dm_done(libxl__egc *egc,
     }
     fclose(f);
 
+process_permissive:
     /* Don't restrict writes to the PCI config space from this VM */
     if (pci->permissive) {
         if ( sysfs_write_bdf(gc, SYSFS_PCIBACK_DRIVER"/permissive",
@@ -2186,10 +2224,10 @@ static void pci_remove_detached(libxl__egc *egc,
     int  irq = 0, i, stubdomid = 0;
     const char *sysfs_path;
     FILE *f;
-    uint32_t sbdf;
     uint32_t domainid = prs->domid;
     bool isstubdom;
     int r;
+    int gsi;
 
     /* Convenience aliases */
     libxl_device_pci *const pci = &prs->pci;
@@ -2245,6 +2283,15 @@ skip_bar:
     if (!pci_supp_legacy_irq())
         goto skip_legacy_irq;
 
+    r = pci_device_set_gsi(ctx, domid, pci, 0, &gsi);
+    if (gsi >= 0) {
+        if (r < 0) {
+            LOGED(ERROR, domainid,
+                  "pci_device_set_gsi gsi=%d (error=%d)", gsi, errno);
+        }
+        goto skip_legacy_irq;
+    }
+    /* if gsi < 0, keep using irq */
     sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
                            pci->bus, pci->dev, pci->func);
 
@@ -2255,13 +2302,6 @@ skip_bar:
     }
 
     if ((fscanf(f, "%u", &irq) == 1) && irq) {
-        sbdf = PCI_SBDF(pci->domain, pci->bus,
-                        (PCI_DEVFN(pci->dev, pci->func)));
-        r = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
-        /* if fail, keep using irq; if success, r is gsi, use gsi */
-        if (r != -1) {
-            irq = r;
-        }
         rc = xc_physdev_unmap_pirq(ctx->xch, domid, irq);
         if (rc < 0) {
             /*
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 9a72d57333e9..9b8a08b2a81d 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -237,6 +237,37 @@ long arch_do_domctl(
         break;
     }
 
+    case XEN_DOMCTL_gsi_permission:
+    {
+        unsigned int gsi = domctl->u.gsi_permission.gsi;
+        int allow = domctl->u.gsi_permission.allow_access;
+
+        if ( is_pv_domain(current->domain) || has_pirq(current->domain) )
+        {
+            ret = -EOPNOTSUPP;
+            break;
+        }
+
+        if ( gsi >= nr_irqs_gsi )
+        {
+            ret = -EINVAL;
+            break;
+        }
+
+        if ( !irq_access_permitted(current->domain, gsi) ||
+             xsm_irq_permission(XSM_HOOK, d, gsi, allow) )
+        {
+            ret = -EPERM;
+            break;
+        }
+
+        if ( allow )
+            ret = irq_permit_access(d, gsi);
+        else
+            ret = irq_deny_access(d, gsi);
+        break;
+    }
+
     case XEN_DOMCTL_getpageframeinfo3:
     {
         unsigned int num = domctl->u.getpageframeinfo3.num;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index a33f9ec32b08..47e95f9ee824 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -447,6 +447,13 @@ struct xen_domctl_irq_permission {
 };
 
 
+/* XEN_DOMCTL_gsi_permission */
+struct xen_domctl_gsi_permission {
+    uint32_t gsi;
+    uint8_t allow_access;    /* flag to specify enable/disable of x86 gsi access */
+};
+
+
 /* XEN_DOMCTL_iomem_permission */
 struct xen_domctl_iomem_permission {
     uint64_aligned_t first_mfn;/* first page (physical page number) in range */
@@ -1277,6 +1284,7 @@ struct xen_domctl {
 #define XEN_DOMCTL_vmtrace_op                    84
 #define XEN_DOMCTL_get_paging_mempool_size       85
 #define XEN_DOMCTL_set_paging_mempool_size       86
+#define XEN_DOMCTL_gsi_permission                87
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1299,6 +1307,7 @@ struct xen_domctl {
         struct xen_domctl_setdomainhandle   setdomainhandle;
         struct xen_domctl_setdebugging      setdebugging;
         struct xen_domctl_irq_permission    irq_permission;
+        struct xen_domctl_gsi_permission    gsi_permission;
         struct xen_domctl_iomem_permission  iomem_permission;
         struct xen_domctl_ioport_permission ioport_permission;
         struct xen_domctl_hypercall_init    hypercall_init;
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 5e88c71b8e22..a5b134c91101 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -685,6 +685,7 @@ static int cf_check flask_domctl(struct domain *d, int cmd)
     case XEN_DOMCTL_shadow_op:
     case XEN_DOMCTL_ioport_permission:
     case XEN_DOMCTL_ioport_mapping:
+    case XEN_DOMCTL_gsi_permission:
 #endif
 #ifdef CONFIG_HAS_PASSTHROUGH
     /*
-- 
2.34.1

On 16.05.2024 11:52, Jiqian Chen wrote:
> Some type of domain don't have PIRQ, like PVH, when
> passthrough a device to guest on PVH dom0, callstack
> pci_add_dm_done->XEN_DOMCTL_irq_permission will failed
> at domain_pirq_to_irq.
> 
> So, add a new hypercall to grant/revoke gsi permission
> when dom0 is not PV or dom0 has not PIRQ flag.

Honestly I find this hard to follow, and thus not really making clear why
no other existing mechanism could be used.

> Signed-off-by: Huang Rui <ray.huang@amd.com>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---

Below here in an RFC patch you typically would want to put specific items
you're seeking feedback on. Without that it's hard to tell why this is
marked RFC.

> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -237,6 +237,37 @@ long arch_do_domctl(
>          break;
>      }
>  
> +    case XEN_DOMCTL_gsi_permission:
> +    {
> +        unsigned int gsi = domctl->u.gsi_permission.gsi;
> +        int allow = domctl->u.gsi_permission.allow_access;

bool?

> +        if ( is_pv_domain(current->domain) || has_pirq(current->domain) )
> +        {
> +            ret = -EOPNOTSUPP;
> +            break;
> +        }

Such a restriction imo wants explaining in a comment.

> +        if ( gsi >= nr_irqs_gsi )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +
> +        if ( !irq_access_permitted(current->domain, gsi) ||

I.e. assuming IRQ == GSI? Is that a valid assumption when any number of
source overrides may be surfaced by ACPI?

> +             xsm_irq_permission(XSM_HOOK, d, gsi, allow) )

Here I'm pretty sure you can't very well re-use an existing hook, as the
value of interest is in a different numbering space, and a possible hook
function has no way of knowing which one it is. Daniel?

> +        {
> +            ret = -EPERM;
> +            break;
> +        }
> +
> +        if ( allow )
> +            ret = irq_permit_access(d, gsi);
> +        else
> +            ret = irq_deny_access(d, gsi);

As above I'm afraid you can't assume IRQ == GSI.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -447,6 +447,13 @@ struct xen_domctl_irq_permission {
>  };
>  
>  
> +/* XEN_DOMCTL_gsi_permission */
> +struct xen_domctl_gsi_permission {
> +    uint32_t gsi;
> +    uint8_t allow_access;    /* flag to specify enable/disable of x86 gsi access */
> +};

Explicit padding please, including a check that it's zero on input.

> +
> +
>  /* XEN_DOMCTL_iomem_permission */

No double blank lines please. In fact you will want to break the double blank
lines in leading context, inserting in the middle.

Jan

On 2024/5/16 22:01, Jan Beulich wrote:
> On 16.05.2024 11:52, Jiqian Chen wrote:
>> Some type of domain don't have PIRQ, like PVH, when
>> passthrough a device to guest on PVH dom0, callstack
>> pci_add_dm_done->XEN_DOMCTL_irq_permission will failed
>> at domain_pirq_to_irq.
>>
>> So, add a new hypercall to grant/revoke gsi permission
>> when dom0 is not PV or dom0 has not PIRQ flag.
> 
> Honestly I find this hard to follow, and thus not really making clear why
> no other existing mechanism could be used.
Sorry, I will describe more clearly in next version.

> 
>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> ---
> 
> Below here in an RFC patch you typically would want to put specific items
> you're seeking feedback on. Without that it's hard to tell why this is
> marked RFC.
OK, I will add " RFC: wait for the third patch on kernel side is accepted." in next version.

> 
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -237,6 +237,37 @@ long arch_do_domctl(
>>          break;
>>      }
>>  
>> +    case XEN_DOMCTL_gsi_permission:
>> +    {
>> +        unsigned int gsi = domctl->u.gsi_permission.gsi;
>> +        int allow = domctl->u.gsi_permission.allow_access;
> 
> bool?
Will change.

> 
>> +        if ( is_pv_domain(current->domain) || has_pirq(current->domain) )
>> +        {
>> +            ret = -EOPNOTSUPP;
>> +            break;
>> +        }
> 
> Such a restriction imo wants explaining in a comment.
Will add in next version.

> 
>> +        if ( gsi >= nr_irqs_gsi )
>> +        {
>> +            ret = -EINVAL;
>> +            break;
>> +        }
>> +
>> +        if ( !irq_access_permitted(current->domain, gsi) ||
> 
> I.e. assuming IRQ == GSI? Is that a valid assumption when any number of
> source overrides may be surfaced by ACPI?
All irqs smaller than nr_irqs_gsi are gsi, aren't they?

> 
>> +             xsm_irq_permission(XSM_HOOK, d, gsi, allow) )
> 
> Here I'm pretty sure you can't very well re-use an existing hook, as the
> value of interest is in a different numbering space, and a possible hook
> function has no way of knowing which one it is. Daniel?
> 
>> +        {
>> +            ret = -EPERM;
>> +            break;
>> +        }
>> +
>> +        if ( allow )
>> +            ret = irq_permit_access(d, gsi);
>> +        else
>> +            ret = irq_deny_access(d, gsi);
> 
> As above I'm afraid you can't assume IRQ == GSI.
> 
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -447,6 +447,13 @@ struct xen_domctl_irq_permission {
>>  };
>>  
>>  
>> +/* XEN_DOMCTL_gsi_permission */
>> +struct xen_domctl_gsi_permission {
>> +    uint32_t gsi;
>> +    uint8_t allow_access;    /* flag to specify enable/disable of x86 gsi access */
>> +};
> 
> Explicit padding please, including a check that it's zero on input.
Thanks, I will add in next version.

> 
>> +
>> +
>>  /* XEN_DOMCTL_iomem_permission */
> 
> No double blank lines please. In fact you will want to break the double blank
> lines in leading context, inserting in the middle.
Will remove one.
> 
> Jan

-- 
Best regards,
Jiqian Chen.

On 17.05.2024 12:45, Chen, Jiqian wrote:
> On 2024/5/16 22:01, Jan Beulich wrote:
>> On 16.05.2024 11:52, Jiqian Chen wrote:
>>> +        if ( gsi >= nr_irqs_gsi )
>>> +        {
>>> +            ret = -EINVAL;
>>> +            break;
>>> +        }
>>> +
>>> +        if ( !irq_access_permitted(current->domain, gsi) ||
>>
>> I.e. assuming IRQ == GSI? Is that a valid assumption when any number of
>> source overrides may be surfaced by ACPI?
> All irqs smaller than nr_irqs_gsi are gsi, aren't they?

They are, but there's not necessarily a 1:1 mapping.

Jan

On 2024/5/17 18:51, Jan Beulich wrote:
> On 17.05.2024 12:45, Chen, Jiqian wrote:
>> On 2024/5/16 22:01, Jan Beulich wrote:
>>> On 16.05.2024 11:52, Jiqian Chen wrote:
>>>> +        if ( gsi >= nr_irqs_gsi )
>>>> +        {
>>>> +            ret = -EINVAL;
>>>> +            break;
>>>> +        }
>>>> +
>>>> +        if ( !irq_access_permitted(current->domain, gsi) ||
>>>
>>> I.e. assuming IRQ == GSI? Is that a valid assumption when any number of
>>> source overrides may be surfaced by ACPI?
>> All irqs smaller than nr_irqs_gsi are gsi, aren't they?
> 
> They are, but there's not necessarily a 1:1 mapping.
Oh, so do I need to add a new gsi_caps to store granted gsi?
And Dom0 defaults to own all gsis permission?

> 
> Jan

-- 
Best regards,
Jiqian Chen.

On 17.05.2024 13:14, Chen, Jiqian wrote:
> On 2024/5/17 18:51, Jan Beulich wrote:
>> On 17.05.2024 12:45, Chen, Jiqian wrote:
>>> On 2024/5/16 22:01, Jan Beulich wrote:
>>>> On 16.05.2024 11:52, Jiqian Chen wrote:
>>>>> +        if ( gsi >= nr_irqs_gsi )
>>>>> +        {
>>>>> +            ret = -EINVAL;
>>>>> +            break;
>>>>> +        }
>>>>> +
>>>>> +        if ( !irq_access_permitted(current->domain, gsi) ||
>>>>
>>>> I.e. assuming IRQ == GSI? Is that a valid assumption when any number of
>>>> source overrides may be surfaced by ACPI?
>>> All irqs smaller than nr_irqs_gsi are gsi, aren't they?
>>
>> They are, but there's not necessarily a 1:1 mapping.
> Oh, so do I need to add a new gsi_caps to store granted gsi?

Probably not. You ought to be able to translate between GSI and IRQ,
and then continue to record in / check against IRQ permissions.

Jan

Hi,
On 2024/5/17 19:50, Jan Beulich wrote:
> On 17.05.2024 13:14, Chen, Jiqian wrote:
>> On 2024/5/17 18:51, Jan Beulich wrote:
>>> On 17.05.2024 12:45, Chen, Jiqian wrote:
>>>> On 2024/5/16 22:01, Jan Beulich wrote:
>>>>> On 16.05.2024 11:52, Jiqian Chen wrote:
>>>>>> +        if ( gsi >= nr_irqs_gsi )
>>>>>> +        {
>>>>>> +            ret = -EINVAL;
>>>>>> +            break;
>>>>>> +        }
>>>>>> +
>>>>>> +        if ( !irq_access_permitted(current->domain, gsi) ||
>>>>>
>>>>> I.e. assuming IRQ == GSI? Is that a valid assumption when any number of
>>>>> source overrides may be surfaced by ACPI?
>>>> All irqs smaller than nr_irqs_gsi are gsi, aren't they?
>>>
>>> They are, but there's not necessarily a 1:1 mapping.
>> Oh, so do I need to add a new gsi_caps to store granted gsi?
> 
> Probably not. You ought to be able to translate between GSI and IRQ,
> and then continue to record in / check against IRQ permissions.
But I found in function init_irq_data:
    for ( irq = 0; irq < nr_irqs_gsi; irq++ )
    {
        int rc;

        desc = irq_to_desc(irq);
        desc->irq = irq;

        rc = init_one_irq_desc(desc);
        if ( rc )
            return rc;
    }
Does it mean that when irq < nr_irqs_gsi, the gsi and irq is a 1:1 mapping?
What's more, when using PHYSDEVOP_setup_gsi, it calls mp_register_gsi,
and in mp_register_gsi, it uses " desc = irq_to_desc(gsi); " to get irq_desc directly.

Combining above, can we consider "gsi == irq" when irq < nr_irqs_gsi ?
> 
> Jan

-- 
Best regards,
Jiqian Chen.

On 29.05.2024 04:41, Chen, Jiqian wrote:
> Hi,
> On 2024/5/17 19:50, Jan Beulich wrote:
>> On 17.05.2024 13:14, Chen, Jiqian wrote:
>>> On 2024/5/17 18:51, Jan Beulich wrote:
>>>> On 17.05.2024 12:45, Chen, Jiqian wrote:
>>>>> On 2024/5/16 22:01, Jan Beulich wrote:
>>>>>> On 16.05.2024 11:52, Jiqian Chen wrote:
>>>>>>> +        if ( gsi >= nr_irqs_gsi )
>>>>>>> +        {
>>>>>>> +            ret = -EINVAL;
>>>>>>> +            break;
>>>>>>> +        }
>>>>>>> +
>>>>>>> +        if ( !irq_access_permitted(current->domain, gsi) ||
>>>>>>
>>>>>> I.e. assuming IRQ == GSI? Is that a valid assumption when any number of
>>>>>> source overrides may be surfaced by ACPI?
>>>>> All irqs smaller than nr_irqs_gsi are gsi, aren't they?
>>>>
>>>> They are, but there's not necessarily a 1:1 mapping.
>>> Oh, so do I need to add a new gsi_caps to store granted gsi?
>>
>> Probably not. You ought to be able to translate between GSI and IRQ,
>> and then continue to record in / check against IRQ permissions.
> But I found in function init_irq_data:
>     for ( irq = 0; irq < nr_irqs_gsi; irq++ )
>     {
>         int rc;
> 
>         desc = irq_to_desc(irq);
>         desc->irq = irq;
> 
>         rc = init_one_irq_desc(desc);
>         if ( rc )
>             return rc;
>     }
> Does it mean that when irq < nr_irqs_gsi, the gsi and irq is a 1:1 mapping?

No, as explained before. I also don't see how you would derive that from
the code above. "nr_irqs_gsi" describes what its name says: The number of
IRQs mapping to a (_some_) GSI. That's to tell them from the non-GSI (i.e.
mainly MSI) ones. There's no implication whatsoever on the IRQ <-> GSI
mapping.

> What's more, when using PHYSDEVOP_setup_gsi, it calls mp_register_gsi,
> and in mp_register_gsi, it uses " desc = irq_to_desc(gsi); " to get irq_desc directly.

Which may be wrong, while that wrong-ness may not have hit anyone in
practice (for reasons that would need working out).

> Combining above, can we consider "gsi == irq" when irq < nr_irqs_gsi ?

Again - no.

Jan

On 2024/5/29 14:31, Jan Beulich wrote:
> On 29.05.2024 04:41, Chen, Jiqian wrote:
>> Hi,
>> On 2024/5/17 19:50, Jan Beulich wrote:
>>> On 17.05.2024 13:14, Chen, Jiqian wrote:
>>>> On 2024/5/17 18:51, Jan Beulich wrote:
>>>>> On 17.05.2024 12:45, Chen, Jiqian wrote:
>>>>>> On 2024/5/16 22:01, Jan Beulich wrote:
>>>>>>> On 16.05.2024 11:52, Jiqian Chen wrote:
>>>>>>>> +        if ( gsi >= nr_irqs_gsi )
>>>>>>>> +        {
>>>>>>>> +            ret = -EINVAL;
>>>>>>>> +            break;
>>>>>>>> +        }
>>>>>>>> +
>>>>>>>> +        if ( !irq_access_permitted(current->domain, gsi) ||
>>>>>>>
>>>>>>> I.e. assuming IRQ == GSI? Is that a valid assumption when any number of
>>>>>>> source overrides may be surfaced by ACPI?
>>>>>> All irqs smaller than nr_irqs_gsi are gsi, aren't they?
>>>>>
>>>>> They are, but there's not necessarily a 1:1 mapping.
>>>> Oh, so do I need to add a new gsi_caps to store granted gsi?
>>>
>>> Probably not. You ought to be able to translate between GSI and IRQ,
>>> and then continue to record in / check against IRQ permissions.
>> But I found in function init_irq_data:
>>     for ( irq = 0; irq < nr_irqs_gsi; irq++ )
>>     {
>>         int rc;
>>
>>         desc = irq_to_desc(irq);
>>         desc->irq = irq;
>>
>>         rc = init_one_irq_desc(desc);
>>         if ( rc )
>>             return rc;
>>     }
>> Does it mean that when irq < nr_irqs_gsi, the gsi and irq is a 1:1 mapping?
> 
> No, as explained before. I also don't see how you would derive that from the code above.
Because here set desc->irq = irq, and it seems there is no other place to change this desc->irq, so, gsi 1 is considered to irq 1.

> "nr_irqs_gsi" describes what its name says: The number of
> IRQs mapping to a (_some_) GSI. That's to tell them from the non-GSI (i.e.
> mainly MSI) ones. There's no implication whatsoever on the IRQ <-> GSI
> mapping.
> 
>> What's more, when using PHYSDEVOP_setup_gsi, it calls mp_register_gsi,
>> and in mp_register_gsi, it uses " desc = irq_to_desc(gsi); " to get irq_desc directly.
> 
> Which may be wrong, while that wrong-ness may not have hit anyone in
> practice (for reasons that would need working out).
> 
>> Combining above, can we consider "gsi == irq" when irq < nr_irqs_gsi ?
> 
> Again - no.
Since you are certain that they are not equal, could you tell me where show they are not equal or where build their mappings,
so that I can know how to do a conversion gsi from irq.

> 
> Jan

-- 
Best regards,
Jiqian Chen.

On 29.05.2024 08:56, Chen, Jiqian wrote:
> On 2024/5/29 14:31, Jan Beulich wrote:
>> On 29.05.2024 04:41, Chen, Jiqian wrote:
>>> Hi,
>>> On 2024/5/17 19:50, Jan Beulich wrote:
>>>> On 17.05.2024 13:14, Chen, Jiqian wrote:
>>>>> On 2024/5/17 18:51, Jan Beulich wrote:
>>>>>> On 17.05.2024 12:45, Chen, Jiqian wrote:
>>>>>>> On 2024/5/16 22:01, Jan Beulich wrote:
>>>>>>>> On 16.05.2024 11:52, Jiqian Chen wrote:
>>>>>>>>> +        if ( gsi >= nr_irqs_gsi )
>>>>>>>>> +        {
>>>>>>>>> +            ret = -EINVAL;
>>>>>>>>> +            break;
>>>>>>>>> +        }
>>>>>>>>> +
>>>>>>>>> +        if ( !irq_access_permitted(current->domain, gsi) ||
>>>>>>>>
>>>>>>>> I.e. assuming IRQ == GSI? Is that a valid assumption when any number of
>>>>>>>> source overrides may be surfaced by ACPI?
>>>>>>> All irqs smaller than nr_irqs_gsi are gsi, aren't they?
>>>>>>
>>>>>> They are, but there's not necessarily a 1:1 mapping.
>>>>> Oh, so do I need to add a new gsi_caps to store granted gsi?
>>>>
>>>> Probably not. You ought to be able to translate between GSI and IRQ,
>>>> and then continue to record in / check against IRQ permissions.
>>> But I found in function init_irq_data:
>>>     for ( irq = 0; irq < nr_irqs_gsi; irq++ )
>>>     {
>>>         int rc;
>>>
>>>         desc = irq_to_desc(irq);
>>>         desc->irq = irq;
>>>
>>>         rc = init_one_irq_desc(desc);
>>>         if ( rc )
>>>             return rc;
>>>     }
>>> Does it mean that when irq < nr_irqs_gsi, the gsi and irq is a 1:1 mapping?
>>
>> No, as explained before. I also don't see how you would derive that from the code above.
> Because here set desc->irq = irq, and it seems there is no other place to change this desc->irq, so, gsi 1 is considered to irq 1.

What are you taking this from? The loop bound isn't nr_gsis, and the iteration
variable isn't in GSI space either; it's in IRQ numbering space. In this loop
we're merely leveraging that every GSI has a corresponding IRQ; there are no
assumptions made about the mapping between the two. Afaics at least.

>> "nr_irqs_gsi" describes what its name says: The number of
>> IRQs mapping to a (_some_) GSI. That's to tell them from the non-GSI (i.e.
>> mainly MSI) ones. There's no implication whatsoever on the IRQ <-> GSI
>> mapping.
>>
>>> What's more, when using PHYSDEVOP_setup_gsi, it calls mp_register_gsi,
>>> and in mp_register_gsi, it uses " desc = irq_to_desc(gsi); " to get irq_desc directly.
>>
>> Which may be wrong, while that wrong-ness may not have hit anyone in
>> practice (for reasons that would need working out).
>>
>>> Combining above, can we consider "gsi == irq" when irq < nr_irqs_gsi ?
>>
>> Again - no.
> Since you are certain that they are not equal, could you tell me where show they are not equal or where build their mappings,
> so that I can know how to do a conversion gsi from irq.

I did point you at the ACPI Interrupt Source Override structure before.
We're parsing those in acpi_parse_int_src_ovr(), to give you a place to
start going from.

Jan

On 2024/5/29 15:10, Jan Beulich wrote:
> On 29.05.2024 08:56, Chen, Jiqian wrote:
>> On 2024/5/29 14:31, Jan Beulich wrote:
>>> On 29.05.2024 04:41, Chen, Jiqian wrote:
>>>> Hi,
>>>> On 2024/5/17 19:50, Jan Beulich wrote:
>>>>> On 17.05.2024 13:14, Chen, Jiqian wrote:
>>>>>> On 2024/5/17 18:51, Jan Beulich wrote:
>>>>>>> On 17.05.2024 12:45, Chen, Jiqian wrote:
>>>>>>>> On 2024/5/16 22:01, Jan Beulich wrote:
>>>>>>>>> On 16.05.2024 11:52, Jiqian Chen wrote:
>>>>>>>>>> +        if ( gsi >= nr_irqs_gsi )
>>>>>>>>>> +        {
>>>>>>>>>> +            ret = -EINVAL;
>>>>>>>>>> +            break;
>>>>>>>>>> +        }
>>>>>>>>>> +
>>>>>>>>>> +        if ( !irq_access_permitted(current->domain, gsi) ||
>>>>>>>>>
>>>>>>>>> I.e. assuming IRQ == GSI? Is that a valid assumption when any number of
>>>>>>>>> source overrides may be surfaced by ACPI?
>>>>>>>> All irqs smaller than nr_irqs_gsi are gsi, aren't they?
>>>>>>>
>>>>>>> They are, but there's not necessarily a 1:1 mapping.
>>>>>> Oh, so do I need to add a new gsi_caps to store granted gsi?
>>>>>
>>>>> Probably not. You ought to be able to translate between GSI and IRQ,
>>>>> and then continue to record in / check against IRQ permissions.
>>>> But I found in function init_irq_data:
>>>>     for ( irq = 0; irq < nr_irqs_gsi; irq++ )
>>>>     {
>>>>         int rc;
>>>>
>>>>         desc = irq_to_desc(irq);
>>>>         desc->irq = irq;
>>>>
>>>>         rc = init_one_irq_desc(desc);
>>>>         if ( rc )
>>>>             return rc;
>>>>     }
>>>> Does it mean that when irq < nr_irqs_gsi, the gsi and irq is a 1:1 mapping?
>>>
>>> No, as explained before. I also don't see how you would derive that from the code above.
>> Because here set desc->irq = irq, and it seems there is no other place to change this desc->irq, so, gsi 1 is considered to irq 1.
> 
> What are you taking this from? The loop bound isn't nr_gsis, and the iteration
> variable isn't in GSI space either; it's in IRQ numbering space. In this loop
> we're merely leveraging that every GSI has a corresponding IRQ;
> there are no assumptions made about the mapping between the two. Afaics at least.
> 
>>> "nr_irqs_gsi" describes what its name says: The number of
>>> IRQs mapping to a (_some_) GSI. That's to tell them from the non-GSI (i.e.
>>> mainly MSI) ones. There's no implication whatsoever on the IRQ <-> GSI
>>> mapping.
>>>
>>>> What's more, when using PHYSDEVOP_setup_gsi, it calls mp_register_gsi,
>>>> and in mp_register_gsi, it uses " desc = irq_to_desc(gsi); " to get irq_desc directly.
>>>
>>> Which may be wrong, while that wrong-ness may not have hit anyone in
>>> practice (for reasons that would need working out).
>>>
>>>> Combining above, can we consider "gsi == irq" when irq < nr_irqs_gsi ?
>>>
>>> Again - no.
>> Since you are certain that they are not equal, could you tell me where show they are not equal or where build their mappings,
>> so that I can know how to do a conversion gsi from irq.
> 
> I did point you at the ACPI Interrupt Source Override structure before.
> We're parsing those in acpi_parse_int_src_ovr(), to give you a place to
> start going from.
Oh! I think I know.
If I want to transform gsi to irq, I need to do below:
	int irq, entry, ioapic, pin;

	ioapic = mp_find_ioapic(gsi);
	pin = gsi - mp_ioapic_routing[ioapic].gsi_base;
	entry = find_irq_entry(ioapic, pin, mp_INT);
	irq = pin_2_irq(entry, ioapic, pin);

Am I right?

> 
> Jan

-- 
Best regards,
Jiqian Chen.

On 29.05.2024 13:13, Chen, Jiqian wrote:
> On 2024/5/29 15:10, Jan Beulich wrote:
>> On 29.05.2024 08:56, Chen, Jiqian wrote:
>>> On 2024/5/29 14:31, Jan Beulich wrote:
>>>> On 29.05.2024 04:41, Chen, Jiqian wrote:
>>>>> But I found in function init_irq_data:
>>>>>     for ( irq = 0; irq < nr_irqs_gsi; irq++ )
>>>>>     {
>>>>>         int rc;
>>>>>
>>>>>         desc = irq_to_desc(irq);
>>>>>         desc->irq = irq;
>>>>>
>>>>>         rc = init_one_irq_desc(desc);
>>>>>         if ( rc )
>>>>>             return rc;
>>>>>     }
>>>>> Does it mean that when irq < nr_irqs_gsi, the gsi and irq is a 1:1 mapping?
>>>>
>>>> No, as explained before. I also don't see how you would derive that from the code above.
>>> Because here set desc->irq = irq, and it seems there is no other place to change this desc->irq, so, gsi 1 is considered to irq 1.
>>
>> What are you taking this from? The loop bound isn't nr_gsis, and the iteration
>> variable isn't in GSI space either; it's in IRQ numbering space. In this loop
>> we're merely leveraging that every GSI has a corresponding IRQ;
>> there are no assumptions made about the mapping between the two. Afaics at least.
>>
>>>> "nr_irqs_gsi" describes what its name says: The number of
>>>> IRQs mapping to a (_some_) GSI. That's to tell them from the non-GSI (i.e.
>>>> mainly MSI) ones. There's no implication whatsoever on the IRQ <-> GSI
>>>> mapping.
>>>>
>>>>> What's more, when using PHYSDEVOP_setup_gsi, it calls mp_register_gsi,
>>>>> and in mp_register_gsi, it uses " desc = irq_to_desc(gsi); " to get irq_desc directly.
>>>>
>>>> Which may be wrong, while that wrong-ness may not have hit anyone in
>>>> practice (for reasons that would need working out).
>>>>
>>>>> Combining above, can we consider "gsi == irq" when irq < nr_irqs_gsi ?
>>>>
>>>> Again - no.
>>> Since you are certain that they are not equal, could you tell me where show they are not equal or where build their mappings,
>>> so that I can know how to do a conversion gsi from irq.
>>
>> I did point you at the ACPI Interrupt Source Override structure before.
>> We're parsing those in acpi_parse_int_src_ovr(), to give you a place to
>> start going from.
> Oh! I think I know.
> If I want to transform gsi to irq, I need to do below:
> 	int irq, entry, ioapic, pin;
> 
> 	ioapic = mp_find_ioapic(gsi);
> 	pin = gsi - mp_ioapic_routing[ioapic].gsi_base;
> 	entry = find_irq_entry(ioapic, pin, mp_INT);
> 	irq = pin_2_irq(entry, ioapic, pin);
> 
> Am I right?

This looks plausible, yes.

Jan

On 2024/5/29 20:22, Jan Beulich wrote:
> On 29.05.2024 13:13, Chen, Jiqian wrote:
>> On 2024/5/29 15:10, Jan Beulich wrote:
>>> On 29.05.2024 08:56, Chen, Jiqian wrote:
>>>> On 2024/5/29 14:31, Jan Beulich wrote:
>>>>> On 29.05.2024 04:41, Chen, Jiqian wrote:
>>>>>> But I found in function init_irq_data:
>>>>>>     for ( irq = 0; irq < nr_irqs_gsi; irq++ )
>>>>>>     {
>>>>>>         int rc;
>>>>>>
>>>>>>         desc = irq_to_desc(irq);
>>>>>>         desc->irq = irq;
>>>>>>
>>>>>>         rc = init_one_irq_desc(desc);
>>>>>>         if ( rc )
>>>>>>             return rc;
>>>>>>     }
>>>>>> Does it mean that when irq < nr_irqs_gsi, the gsi and irq is a 1:1 mapping?
>>>>>
>>>>> No, as explained before. I also don't see how you would derive that from the code above.
>>>> Because here set desc->irq = irq, and it seems there is no other place to change this desc->irq, so, gsi 1 is considered to irq 1.
>>>
>>> What are you taking this from? The loop bound isn't nr_gsis, and the iteration
>>> variable isn't in GSI space either; it's in IRQ numbering space. In this loop
>>> we're merely leveraging that every GSI has a corresponding IRQ;
>>> there are no assumptions made about the mapping between the two. Afaics at least.
>>>
>>>>> "nr_irqs_gsi" describes what its name says: The number of
>>>>> IRQs mapping to a (_some_) GSI. That's to tell them from the non-GSI (i.e.
>>>>> mainly MSI) ones. There's no implication whatsoever on the IRQ <-> GSI
>>>>> mapping.
>>>>>
>>>>>> What's more, when using PHYSDEVOP_setup_gsi, it calls mp_register_gsi,
>>>>>> and in mp_register_gsi, it uses " desc = irq_to_desc(gsi); " to get irq_desc directly.
>>>>>
>>>>> Which may be wrong, while that wrong-ness may not have hit anyone in
>>>>> practice (for reasons that would need working out).
>>>>>
>>>>>> Combining above, can we consider "gsi == irq" when irq < nr_irqs_gsi ?
>>>>>
>>>>> Again - no.
>>>> Since you are certain that they are not equal, could you tell me where show they are not equal or where build their mappings,
>>>> so that I can know how to do a conversion gsi from irq.
>>>
>>> I did point you at the ACPI Interrupt Source Override structure before.
>>> We're parsing those in acpi_parse_int_src_ovr(), to give you a place to
>>> start going from.
>> Oh! I think I know.
>> If I want to transform gsi to irq, I need to do below:
>> 	int irq, entry, ioapic, pin;
>>
>> 	ioapic = mp_find_ioapic(gsi);
>> 	pin = gsi - mp_ioapic_routing[ioapic].gsi_base;
>> 	entry = find_irq_entry(ioapic, pin, mp_INT);
>> 	irq = pin_2_irq(entry, ioapic, pin);
>>
>> Am I right?
> 
> This looks plausible, yes.
I dump all mpc_config_intsrc of array mp_irqs, it shows:
(XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 0 dstapic 33 dstirq 2
(XEN) find_irq_entry type 3 irqtype 0 irqflag 15 srcbus 0 srcbusirq 9 dstapic 33 dstirq 9
(XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 1 dstapic 33 dstirq 1
(XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 3 dstapic 33 dstirq 3
(XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 4 dstapic 33 dstirq 4
(XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 5 dstapic 33 dstirq 5
(XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 6 dstapic 33 dstirq 6
(XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 7 dstapic 33 dstirq 7
(XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 8 dstapic 33 dstirq 8
(XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 10 dstapic 33 dstirq 10
(XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 11 dstapic 33 dstirq 11
(XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 12 dstapic 33 dstirq 12
(XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 13 dstapic 33 dstirq 13
(XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 14 dstapic 33 dstirq 14
(XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 15 dstapic 33 dstirq 15

It seems only Legacy irq and gsi[0:15] has a mapping in mp_irqs.
Other gsi can be considered 1:1 mapping with irq? Or are there other places reflect the mapping between irq and gsi?
And my code will be:
    case XEN_DOMCTL_gsi_permission:
    {
        unsigned int gsi = domctl->u.gsi_permission.gsi;
        int irq;
        bool allow = domctl->u.gsi_permission.allow_access;

        /*
         * gsi[0,15] is not 1:1 mapping to legacy irq[0,15], it need a
         * transformation. Other gsi is considered be 1:1 mapping to irq
         */
        if ( gsi < 16 )
            irq = gsi_2_irq(gsi);
        else
            irq = gsi;

        /*
         * If current domain is PV or it has PIRQ flag, it has a mapping
         * of gsi, pirq and irq, so it should use XEN_DOMCTL_irq_permission
         * to grant irq permission.
         */
        if ( is_pv_domain(current->domain) || has_pirq(current->domain) )
        {
            ret = -EOPNOTSUPP;
            break;
        }

        if ( gsi >= nr_irqs_gsi || irq < 0 )
        {
            ret = -EINVAL;
            break;
        }

        if ( !irq_access_permitted(current->domain, irq) ||
             xsm_irq_permission(XSM_HOOK, d, irq, allow) )
        {
            ret = -EPERM;
            break;
        }

        if ( allow )
            ret = irq_permit_access(d, irq);
        else
            ret = irq_deny_access(d, irq);
        break;
    }

Is above acceptable?

> 
> Jan

-- 
Best regards,
Jiqian Chen.

On 30.05.2024 13:19, Chen, Jiqian wrote:
> On 2024/5/29 20:22, Jan Beulich wrote:
>> On 29.05.2024 13:13, Chen, Jiqian wrote:
>>> On 2024/5/29 15:10, Jan Beulich wrote:
>>>> On 29.05.2024 08:56, Chen, Jiqian wrote:
>>>>> On 2024/5/29 14:31, Jan Beulich wrote:
>>>>>> On 29.05.2024 04:41, Chen, Jiqian wrote:
>>>>>>> But I found in function init_irq_data:
>>>>>>>     for ( irq = 0; irq < nr_irqs_gsi; irq++ )
>>>>>>>     {
>>>>>>>         int rc;
>>>>>>>
>>>>>>>         desc = irq_to_desc(irq);
>>>>>>>         desc->irq = irq;
>>>>>>>
>>>>>>>         rc = init_one_irq_desc(desc);
>>>>>>>         if ( rc )
>>>>>>>             return rc;
>>>>>>>     }
>>>>>>> Does it mean that when irq < nr_irqs_gsi, the gsi and irq is a 1:1 mapping?
>>>>>>
>>>>>> No, as explained before. I also don't see how you would derive that from the code above.
>>>>> Because here set desc->irq = irq, and it seems there is no other place to change this desc->irq, so, gsi 1 is considered to irq 1.
>>>>
>>>> What are you taking this from? The loop bound isn't nr_gsis, and the iteration
>>>> variable isn't in GSI space either; it's in IRQ numbering space. In this loop
>>>> we're merely leveraging that every GSI has a corresponding IRQ;
>>>> there are no assumptions made about the mapping between the two. Afaics at least.
>>>>
>>>>>> "nr_irqs_gsi" describes what its name says: The number of
>>>>>> IRQs mapping to a (_some_) GSI. That's to tell them from the non-GSI (i.e.
>>>>>> mainly MSI) ones. There's no implication whatsoever on the IRQ <-> GSI
>>>>>> mapping.
>>>>>>
>>>>>>> What's more, when using PHYSDEVOP_setup_gsi, it calls mp_register_gsi,
>>>>>>> and in mp_register_gsi, it uses " desc = irq_to_desc(gsi); " to get irq_desc directly.
>>>>>>
>>>>>> Which may be wrong, while that wrong-ness may not have hit anyone in
>>>>>> practice (for reasons that would need working out).
>>>>>>
>>>>>>> Combining above, can we consider "gsi == irq" when irq < nr_irqs_gsi ?
>>>>>>
>>>>>> Again - no.
>>>>> Since you are certain that they are not equal, could you tell me where show they are not equal or where build their mappings,
>>>>> so that I can know how to do a conversion gsi from irq.
>>>>
>>>> I did point you at the ACPI Interrupt Source Override structure before.
>>>> We're parsing those in acpi_parse_int_src_ovr(), to give you a place to
>>>> start going from.
>>> Oh! I think I know.
>>> If I want to transform gsi to irq, I need to do below:
>>> 	int irq, entry, ioapic, pin;
>>>
>>> 	ioapic = mp_find_ioapic(gsi);
>>> 	pin = gsi - mp_ioapic_routing[ioapic].gsi_base;
>>> 	entry = find_irq_entry(ioapic, pin, mp_INT);
>>> 	irq = pin_2_irq(entry, ioapic, pin);
>>>
>>> Am I right?
>>
>> This looks plausible, yes.
> I dump all mpc_config_intsrc of array mp_irqs, it shows:
> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 0 dstapic 33 dstirq 2
> (XEN) find_irq_entry type 3 irqtype 0 irqflag 15 srcbus 0 srcbusirq 9 dstapic 33 dstirq 9
> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 1 dstapic 33 dstirq 1
> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 3 dstapic 33 dstirq 3
> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 4 dstapic 33 dstirq 4
> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 5 dstapic 33 dstirq 5
> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 6 dstapic 33 dstirq 6
> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 7 dstapic 33 dstirq 7
> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 8 dstapic 33 dstirq 8
> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 10 dstapic 33 dstirq 10
> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 11 dstapic 33 dstirq 11
> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 12 dstapic 33 dstirq 12
> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 13 dstapic 33 dstirq 13
> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 14 dstapic 33 dstirq 14
> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 15 dstapic 33 dstirq 15
> 
> It seems only Legacy irq and gsi[0:15] has a mapping in mp_irqs.
> Other gsi can be considered 1:1 mapping with irq? Or are there other places reflect the mapping between irq and gsi?

It may be uncommon to have overrides for higher GSIs, but I don't think ACPI
disallows that.

Jan

On 2024/5/30 23:51, Jan Beulich wrote:
> On 30.05.2024 13:19, Chen, Jiqian wrote:
>> On 2024/5/29 20:22, Jan Beulich wrote:
>>> On 29.05.2024 13:13, Chen, Jiqian wrote:
>>>> On 2024/5/29 15:10, Jan Beulich wrote:
>>>>> On 29.05.2024 08:56, Chen, Jiqian wrote:
>>>>>> On 2024/5/29 14:31, Jan Beulich wrote:
>>>>>>> On 29.05.2024 04:41, Chen, Jiqian wrote:
>>>>>>>> But I found in function init_irq_data:
>>>>>>>>     for ( irq = 0; irq < nr_irqs_gsi; irq++ )
>>>>>>>>     {
>>>>>>>>         int rc;
>>>>>>>>
>>>>>>>>         desc = irq_to_desc(irq);
>>>>>>>>         desc->irq = irq;
>>>>>>>>
>>>>>>>>         rc = init_one_irq_desc(desc);
>>>>>>>>         if ( rc )
>>>>>>>>             return rc;
>>>>>>>>     }
>>>>>>>> Does it mean that when irq < nr_irqs_gsi, the gsi and irq is a 1:1 mapping?
>>>>>>>
>>>>>>> No, as explained before. I also don't see how you would derive that from the code above.
>>>>>> Because here set desc->irq = irq, and it seems there is no other place to change this desc->irq, so, gsi 1 is considered to irq 1.
>>>>>
>>>>> What are you taking this from? The loop bound isn't nr_gsis, and the iteration
>>>>> variable isn't in GSI space either; it's in IRQ numbering space. In this loop
>>>>> we're merely leveraging that every GSI has a corresponding IRQ;
>>>>> there are no assumptions made about the mapping between the two. Afaics at least.
>>>>>
>>>>>>> "nr_irqs_gsi" describes what its name says: The number of
>>>>>>> IRQs mapping to a (_some_) GSI. That's to tell them from the non-GSI (i.e.
>>>>>>> mainly MSI) ones. There's no implication whatsoever on the IRQ <-> GSI
>>>>>>> mapping.
>>>>>>>
>>>>>>>> What's more, when using PHYSDEVOP_setup_gsi, it calls mp_register_gsi,
>>>>>>>> and in mp_register_gsi, it uses " desc = irq_to_desc(gsi); " to get irq_desc directly.
>>>>>>>
>>>>>>> Which may be wrong, while that wrong-ness may not have hit anyone in
>>>>>>> practice (for reasons that would need working out).
>>>>>>>
>>>>>>>> Combining above, can we consider "gsi == irq" when irq < nr_irqs_gsi ?
>>>>>>>
>>>>>>> Again - no.
>>>>>> Since you are certain that they are not equal, could you tell me where show they are not equal or where build their mappings,
>>>>>> so that I can know how to do a conversion gsi from irq.
>>>>>
>>>>> I did point you at the ACPI Interrupt Source Override structure before.
>>>>> We're parsing those in acpi_parse_int_src_ovr(), to give you a place to
>>>>> start going from.
>>>> Oh! I think I know.
>>>> If I want to transform gsi to irq, I need to do below:
>>>> 	int irq, entry, ioapic, pin;
>>>>
>>>> 	ioapic = mp_find_ioapic(gsi);
>>>> 	pin = gsi - mp_ioapic_routing[ioapic].gsi_base;
>>>> 	entry = find_irq_entry(ioapic, pin, mp_INT);
>>>> 	irq = pin_2_irq(entry, ioapic, pin);
>>>>
>>>> Am I right?
>>>
>>> This looks plausible, yes.
>> I dump all mpc_config_intsrc of array mp_irqs, it shows:
>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 0 dstapic 33 dstirq 2
>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 15 srcbus 0 srcbusirq 9 dstapic 33 dstirq 9
>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 1 dstapic 33 dstirq 1
>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 3 dstapic 33 dstirq 3
>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 4 dstapic 33 dstirq 4
>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 5 dstapic 33 dstirq 5
>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 6 dstapic 33 dstirq 6
>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 7 dstapic 33 dstirq 7
>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 8 dstapic 33 dstirq 8
>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 10 dstapic 33 dstirq 10
>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 11 dstapic 33 dstirq 11
>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 12 dstapic 33 dstirq 12
>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 13 dstapic 33 dstirq 13
>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 14 dstapic 33 dstirq 14
>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 15 dstapic 33 dstirq 15
>>
>> It seems only Legacy irq and gsi[0:15] has a mapping in mp_irqs.
>> Other gsi can be considered 1:1 mapping with irq? Or are there other places reflect the mapping between irq and gsi?
> 
> It may be uncommon to have overrides for higher GSIs, but I don't think ACPI
> disallows that.
Do you suggest me to add overrides for higher GSIs into array mp_irqs?

> 
> Jan

-- 
Best regards,
Jiqian Chen.

On 04.06.2024 05:04, Chen, Jiqian wrote:
> On 2024/5/30 23:51, Jan Beulich wrote:
>> On 30.05.2024 13:19, Chen, Jiqian wrote:
>>> On 2024/5/29 20:22, Jan Beulich wrote:
>>>> On 29.05.2024 13:13, Chen, Jiqian wrote:
>>>>> On 2024/5/29 15:10, Jan Beulich wrote:
>>>>>> On 29.05.2024 08:56, Chen, Jiqian wrote:
>>>>>>> On 2024/5/29 14:31, Jan Beulich wrote:
>>>>>>>> On 29.05.2024 04:41, Chen, Jiqian wrote:
>>>>>>>>> But I found in function init_irq_data:
>>>>>>>>>     for ( irq = 0; irq < nr_irqs_gsi; irq++ )
>>>>>>>>>     {
>>>>>>>>>         int rc;
>>>>>>>>>
>>>>>>>>>         desc = irq_to_desc(irq);
>>>>>>>>>         desc->irq = irq;
>>>>>>>>>
>>>>>>>>>         rc = init_one_irq_desc(desc);
>>>>>>>>>         if ( rc )
>>>>>>>>>             return rc;
>>>>>>>>>     }
>>>>>>>>> Does it mean that when irq < nr_irqs_gsi, the gsi and irq is a 1:1 mapping?
>>>>>>>>
>>>>>>>> No, as explained before. I also don't see how you would derive that from the code above.
>>>>>>> Because here set desc->irq = irq, and it seems there is no other place to change this desc->irq, so, gsi 1 is considered to irq 1.
>>>>>>
>>>>>> What are you taking this from? The loop bound isn't nr_gsis, and the iteration
>>>>>> variable isn't in GSI space either; it's in IRQ numbering space. In this loop
>>>>>> we're merely leveraging that every GSI has a corresponding IRQ;
>>>>>> there are no assumptions made about the mapping between the two. Afaics at least.
>>>>>>
>>>>>>>> "nr_irqs_gsi" describes what its name says: The number of
>>>>>>>> IRQs mapping to a (_some_) GSI. That's to tell them from the non-GSI (i.e.
>>>>>>>> mainly MSI) ones. There's no implication whatsoever on the IRQ <-> GSI
>>>>>>>> mapping.
>>>>>>>>
>>>>>>>>> What's more, when using PHYSDEVOP_setup_gsi, it calls mp_register_gsi,
>>>>>>>>> and in mp_register_gsi, it uses " desc = irq_to_desc(gsi); " to get irq_desc directly.
>>>>>>>>
>>>>>>>> Which may be wrong, while that wrong-ness may not have hit anyone in
>>>>>>>> practice (for reasons that would need working out).
>>>>>>>>
>>>>>>>>> Combining above, can we consider "gsi == irq" when irq < nr_irqs_gsi ?
>>>>>>>>
>>>>>>>> Again - no.
>>>>>>> Since you are certain that they are not equal, could you tell me where show they are not equal or where build their mappings,
>>>>>>> so that I can know how to do a conversion gsi from irq.
>>>>>>
>>>>>> I did point you at the ACPI Interrupt Source Override structure before.
>>>>>> We're parsing those in acpi_parse_int_src_ovr(), to give you a place to
>>>>>> start going from.
>>>>> Oh! I think I know.
>>>>> If I want to transform gsi to irq, I need to do below:
>>>>> 	int irq, entry, ioapic, pin;
>>>>>
>>>>> 	ioapic = mp_find_ioapic(gsi);
>>>>> 	pin = gsi - mp_ioapic_routing[ioapic].gsi_base;
>>>>> 	entry = find_irq_entry(ioapic, pin, mp_INT);
>>>>> 	irq = pin_2_irq(entry, ioapic, pin);
>>>>>
>>>>> Am I right?
>>>>
>>>> This looks plausible, yes.
>>> I dump all mpc_config_intsrc of array mp_irqs, it shows:
>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 0 dstapic 33 dstirq 2
>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 15 srcbus 0 srcbusirq 9 dstapic 33 dstirq 9
>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 1 dstapic 33 dstirq 1
>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 3 dstapic 33 dstirq 3
>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 4 dstapic 33 dstirq 4
>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 5 dstapic 33 dstirq 5
>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 6 dstapic 33 dstirq 6
>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 7 dstapic 33 dstirq 7
>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 8 dstapic 33 dstirq 8
>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 10 dstapic 33 dstirq 10
>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 11 dstapic 33 dstirq 11
>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 12 dstapic 33 dstirq 12
>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 13 dstapic 33 dstirq 13
>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 14 dstapic 33 dstirq 14
>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 15 dstapic 33 dstirq 15
>>>
>>> It seems only Legacy irq and gsi[0:15] has a mapping in mp_irqs.
>>> Other gsi can be considered 1:1 mapping with irq? Or are there other places reflect the mapping between irq and gsi?
>>
>> It may be uncommon to have overrides for higher GSIs, but I don't think ACPI
>> disallows that.
> Do you suggest me to add overrides for higher GSIs into array mp_irqs?

Why "add"? That's what mp_override_legacy_irq() already does, isn't it?
Assuming of course any are surfaced at all by ACPI.

Jan

On 2024/6/4 13:55, Jan Beulich wrote:
> On 04.06.2024 05:04, Chen, Jiqian wrote:
>> On 2024/5/30 23:51, Jan Beulich wrote:
>>> On 30.05.2024 13:19, Chen, Jiqian wrote:
>>>> On 2024/5/29 20:22, Jan Beulich wrote:
>>>>> On 29.05.2024 13:13, Chen, Jiqian wrote:
>>>>>> On 2024/5/29 15:10, Jan Beulich wrote:
>>>>>>> On 29.05.2024 08:56, Chen, Jiqian wrote:
>>>>>>>> On 2024/5/29 14:31, Jan Beulich wrote:
>>>>>>>>> On 29.05.2024 04:41, Chen, Jiqian wrote:
>>>>>>>>>> But I found in function init_irq_data:
>>>>>>>>>>     for ( irq = 0; irq < nr_irqs_gsi; irq++ )
>>>>>>>>>>     {
>>>>>>>>>>         int rc;
>>>>>>>>>>
>>>>>>>>>>         desc = irq_to_desc(irq);
>>>>>>>>>>         desc->irq = irq;
>>>>>>>>>>
>>>>>>>>>>         rc = init_one_irq_desc(desc);
>>>>>>>>>>         if ( rc )
>>>>>>>>>>             return rc;
>>>>>>>>>>     }
>>>>>>>>>> Does it mean that when irq < nr_irqs_gsi, the gsi and irq is a 1:1 mapping?
>>>>>>>>>
>>>>>>>>> No, as explained before. I also don't see how you would derive that from the code above.
>>>>>>>> Because here set desc->irq = irq, and it seems there is no other place to change this desc->irq, so, gsi 1 is considered to irq 1.
>>>>>>>
>>>>>>> What are you taking this from? The loop bound isn't nr_gsis, and the iteration
>>>>>>> variable isn't in GSI space either; it's in IRQ numbering space. In this loop
>>>>>>> we're merely leveraging that every GSI has a corresponding IRQ;
>>>>>>> there are no assumptions made about the mapping between the two. Afaics at least.
>>>>>>>
>>>>>>>>> "nr_irqs_gsi" describes what its name says: The number of
>>>>>>>>> IRQs mapping to a (_some_) GSI. That's to tell them from the non-GSI (i.e.
>>>>>>>>> mainly MSI) ones. There's no implication whatsoever on the IRQ <-> GSI
>>>>>>>>> mapping.
>>>>>>>>>
>>>>>>>>>> What's more, when using PHYSDEVOP_setup_gsi, it calls mp_register_gsi,
>>>>>>>>>> and in mp_register_gsi, it uses " desc = irq_to_desc(gsi); " to get irq_desc directly.
>>>>>>>>>
>>>>>>>>> Which may be wrong, while that wrong-ness may not have hit anyone in
>>>>>>>>> practice (for reasons that would need working out).
>>>>>>>>>
>>>>>>>>>> Combining above, can we consider "gsi == irq" when irq < nr_irqs_gsi ?
>>>>>>>>>
>>>>>>>>> Again - no.
>>>>>>>> Since you are certain that they are not equal, could you tell me where show they are not equal or where build their mappings,
>>>>>>>> so that I can know how to do a conversion gsi from irq.
>>>>>>>
>>>>>>> I did point you at the ACPI Interrupt Source Override structure before.
>>>>>>> We're parsing those in acpi_parse_int_src_ovr(), to give you a place to
>>>>>>> start going from.
>>>>>> Oh! I think I know.
>>>>>> If I want to transform gsi to irq, I need to do below:
>>>>>> 	int irq, entry, ioapic, pin;
>>>>>>
>>>>>> 	ioapic = mp_find_ioapic(gsi);
>>>>>> 	pin = gsi - mp_ioapic_routing[ioapic].gsi_base;
>>>>>> 	entry = find_irq_entry(ioapic, pin, mp_INT);
>>>>>> 	irq = pin_2_irq(entry, ioapic, pin);
>>>>>>
>>>>>> Am I right?
>>>>>
>>>>> This looks plausible, yes.
>>>> I dump all mpc_config_intsrc of array mp_irqs, it shows:
>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 0 dstapic 33 dstirq 2
>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 15 srcbus 0 srcbusirq 9 dstapic 33 dstirq 9
>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 1 dstapic 33 dstirq 1
>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 3 dstapic 33 dstirq 3
>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 4 dstapic 33 dstirq 4
>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 5 dstapic 33 dstirq 5
>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 6 dstapic 33 dstirq 6
>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 7 dstapic 33 dstirq 7
>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 8 dstapic 33 dstirq 8
>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 10 dstapic 33 dstirq 10
>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 11 dstapic 33 dstirq 11
>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 12 dstapic 33 dstirq 12
>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 13 dstapic 33 dstirq 13
>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 14 dstapic 33 dstirq 14
>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 15 dstapic 33 dstirq 15
>>>>
>>>> It seems only Legacy irq and gsi[0:15] has a mapping in mp_irqs.
>>>> Other gsi can be considered 1:1 mapping with irq? Or are there other places reflect the mapping between irq and gsi?
>>>
>>> It may be uncommon to have overrides for higher GSIs, but I don't think ACPI
>>> disallows that.
>> Do you suggest me to add overrides for higher GSIs into array mp_irqs?
> 
> Why "add"? That's what mp_override_legacy_irq() already does, isn't it?
No. mp_override_legacy_irq only overrides for gsi < 16, but not for gsi >= 16(I dump all mappings from array mp_irqs).
In my environment, gsi of my dGPU is 24.
So, how do I process for gsi >= 16?

> Assuming of course any are surfaced at all by ACPI.
> 
> Jan

-- 
Best regards,
Jiqian Chen.

On 04.06.2024 08:01, Chen, Jiqian wrote:
> So, how do I process for gsi >= 16?

Oh, and to answer this as well: Isn't it as simple as treating
as 1:1 mapped any (valid) GSI you can't find in mp_irqs[]? You
only care about the mapping, not e.g. polarity or trigger mode
here, iirc.

Jan

On 04.06.2024 08:01, Chen, Jiqian wrote:
> On 2024/6/4 13:55, Jan Beulich wrote:
>> On 04.06.2024 05:04, Chen, Jiqian wrote:
>>> On 2024/5/30 23:51, Jan Beulich wrote:
>>>> On 30.05.2024 13:19, Chen, Jiqian wrote:
>>>>> I dump all mpc_config_intsrc of array mp_irqs, it shows:
>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 0 dstapic 33 dstirq 2
>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 15 srcbus 0 srcbusirq 9 dstapic 33 dstirq 9
>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 1 dstapic 33 dstirq 1
>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 3 dstapic 33 dstirq 3
>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 4 dstapic 33 dstirq 4
>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 5 dstapic 33 dstirq 5
>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 6 dstapic 33 dstirq 6
>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 7 dstapic 33 dstirq 7
>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 8 dstapic 33 dstirq 8
>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 10 dstapic 33 dstirq 10
>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 11 dstapic 33 dstirq 11
>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 12 dstapic 33 dstirq 12
>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 13 dstapic 33 dstirq 13
>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 14 dstapic 33 dstirq 14
>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 15 dstapic 33 dstirq 15
>>>>>
>>>>> It seems only Legacy irq and gsi[0:15] has a mapping in mp_irqs.
>>>>> Other gsi can be considered 1:1 mapping with irq? Or are there other places reflect the mapping between irq and gsi?
>>>>
>>>> It may be uncommon to have overrides for higher GSIs, but I don't think ACPI
>>>> disallows that.
>>> Do you suggest me to add overrides for higher GSIs into array mp_irqs?
>>
>> Why "add"? That's what mp_override_legacy_irq() already does, isn't it?
> No. mp_override_legacy_irq only overrides for gsi < 16, but not for gsi >= 16(I dump all mappings from array mp_irqs).

I assume you mean you observe so ...

> In my environment, gsi of my dGPU is 24.

... on one specific system? The function is invoked from
acpi_parse_int_src_ovr(), and I can't spot any restriction to
IRQs less than 16 there.

Jan

On 2024/6/4 14:12, Jan Beulich wrote:
> On 04.06.2024 08:01, Chen, Jiqian wrote:
>> On 2024/6/4 13:55, Jan Beulich wrote:
>>> On 04.06.2024 05:04, Chen, Jiqian wrote:
>>>> On 2024/5/30 23:51, Jan Beulich wrote:
>>>>> On 30.05.2024 13:19, Chen, Jiqian wrote:
>>>>>> I dump all mpc_config_intsrc of array mp_irqs, it shows:
>>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 0 dstapic 33 dstirq 2
>>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 15 srcbus 0 srcbusirq 9 dstapic 33 dstirq 9
>>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 1 dstapic 33 dstirq 1
>>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 3 dstapic 33 dstirq 3
>>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 4 dstapic 33 dstirq 4
>>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 5 dstapic 33 dstirq 5
>>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 6 dstapic 33 dstirq 6
>>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 7 dstapic 33 dstirq 7
>>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 8 dstapic 33 dstirq 8
>>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 10 dstapic 33 dstirq 10
>>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 11 dstapic 33 dstirq 11
>>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 12 dstapic 33 dstirq 12
>>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 13 dstapic 33 dstirq 13
>>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 14 dstapic 33 dstirq 14
>>>>>> (XEN) find_irq_entry type 3 irqtype 0 irqflag 0 srcbus 0 srcbusirq 15 dstapic 33 dstirq 15
>>>>>>
>>>>>> It seems only Legacy irq and gsi[0:15] has a mapping in mp_irqs.
>>>>>> Other gsi can be considered 1:1 mapping with irq? Or are there other places reflect the mapping between irq and gsi?
>>>>>
>>>>> It may be uncommon to have overrides for higher GSIs, but I don't think ACPI
>>>>> disallows that.
>>>> Do you suggest me to add overrides for higher GSIs into array mp_irqs?
>>>
>>> Why "add"? That's what mp_override_legacy_irq() already does, isn't it?
>> No. mp_override_legacy_irq only overrides for gsi < 16, but not for gsi >= 16(I dump all mappings from array mp_irqs).
> 
> I assume you mean you observe so ...
No, after starting xen pvh dom0, I dump all entries from mp_irqs.

> 
>> In my environment, gsi of my dGPU is 24.
> 
> ... on one specific system? The function is invoked from
> acpi_parse_int_src_ovr(), and I can't spot any restriction to
> IRQs less than 16 there.
I didn't see any restriction too, but from the dump results, there are only 16 entries, see previous email. 

> 
> Jan

-- 
Best regards,
Jiqian Chen.

On 04.06.2024 08:33, Chen, Jiqian wrote:
> On 2024/6/4 14:12, Jan Beulich wrote:
>> On 04.06.2024 08:01, Chen, Jiqian wrote:
>>> On 2024/6/4 13:55, Jan Beulich wrote:
>>>> On 04.06.2024 05:04, Chen, Jiqian wrote:
>>>>> On 2024/5/30 23:51, Jan Beulich wrote:
>>>>>> On 30.05.2024 13:19, Chen, Jiqian wrote:
>>>>>>> It seems only Legacy irq and gsi[0:15] has a mapping in mp_irqs.
>>>>>>> Other gsi can be considered 1:1 mapping with irq? Or are there other places reflect the mapping between irq and gsi?
>>>>>>
>>>>>> It may be uncommon to have overrides for higher GSIs, but I don't think ACPI
>>>>>> disallows that.
>>>>> Do you suggest me to add overrides for higher GSIs into array mp_irqs?
>>>>
>>>> Why "add"? That's what mp_override_legacy_irq() already does, isn't it?
>>> No. mp_override_legacy_irq only overrides for gsi < 16, but not for gsi >= 16(I dump all mappings from array mp_irqs).
>>
>> I assume you mean you observe so ...
> No, after starting xen pvh dom0, I dump all entries from mp_irqs.

IOW really your answer is "yes" ...

>>> In my environment, gsi of my dGPU is 24.
>>
>> ... on one specific system?

... to this question I raised. Whatever you dump on any number of
systems, there's always the chance that there's another system
where things are different.

>> The function is invoked from
>> acpi_parse_int_src_ovr(), and I can't spot any restriction to
>> IRQs less than 16 there.
> I didn't see any restriction too, but from the dump results, there are only 16 entries, see previous email. 

Hence why I tried to point out that going from observations on a
particular system isn't enough.

Jan

On 2024/6/4 14:36, Jan Beulich wrote:
> On 04.06.2024 08:33, Chen, Jiqian wrote:
>> On 2024/6/4 14:12, Jan Beulich wrote:
>>> On 04.06.2024 08:01, Chen, Jiqian wrote:
>>>> On 2024/6/4 13:55, Jan Beulich wrote:
>>>>> On 04.06.2024 05:04, Chen, Jiqian wrote:
>>>>>> On 2024/5/30 23:51, Jan Beulich wrote:
>>>>>>> On 30.05.2024 13:19, Chen, Jiqian wrote:
>>>>>>>> It seems only Legacy irq and gsi[0:15] has a mapping in mp_irqs.
>>>>>>>> Other gsi can be considered 1:1 mapping with irq? Or are there other places reflect the mapping between irq and gsi?
>>>>>>>
>>>>>>> It may be uncommon to have overrides for higher GSIs, but I don't think ACPI
>>>>>>> disallows that.
>>>>>> Do you suggest me to add overrides for higher GSIs into array mp_irqs?
>>>>>
>>>>> Why "add"? That's what mp_override_legacy_irq() already does, isn't it?
>>>> No. mp_override_legacy_irq only overrides for gsi < 16, but not for gsi >= 16(I dump all mappings from array mp_irqs).
>>>
>>> I assume you mean you observe so ...
>> No, after starting xen pvh dom0, I dump all entries from mp_irqs.
> 
> IOW really your answer is "yes" ...
> 
>>>> In my environment, gsi of my dGPU is 24.
>>>
>>> ... on one specific system?
> 
> ... to this question I raised. Whatever you dump on any number of
> systems, there's always the chance that there's another system
> where things are different.
> 
>>> The function is invoked from
>>> acpi_parse_int_src_ovr(), and I can't spot any restriction to
>>> IRQs less than 16 there.
>> I didn't see any restriction too, but from the dump results, there are only 16 entries, see previous email. 
> 
> Hence why I tried to point out that going from observations on a
> particular system isn't enough.
Anyway, I agree with you that I need to get mapping from mp_irqs.
I tried to get more debug information from my environment. And I attach them here, maybe you can find some problems.
acpi_parse_madt_ioapic_entries
	acpi_table_parse_madt(ACPI_MADT_TYPE_INTERRUPT_OVERRIDE, acpi_parse_int_src_ovr, MAX_IRQ_SOURCES);
		acpi_parse_int_src_ovr
			mp_override_legacy_irq
				only process two entries, irq 0 gsi 2 and irq 9 gsi 9
There are only two entries whose type is ACPI_MADT_TYPE_INTERRUPT_OVERRIDE in MADT table. Is it normal?
And
acpi_parse_madt_ioapic_entries
	mp_config_acpi_legacy_irqs
		process the other GSIs(< 16), so that the total number of mp_irqs is 16.

> 
> Jan

-- 
Best regards,
Jiqian Chen.

On 04.06.2024 10:18, Chen, Jiqian wrote:
> I tried to get more debug information from my environment. And I attach them here, maybe you can find some problems.
> acpi_parse_madt_ioapic_entries
> 	acpi_table_parse_madt(ACPI_MADT_TYPE_INTERRUPT_OVERRIDE, acpi_parse_int_src_ovr, MAX_IRQ_SOURCES);
> 		acpi_parse_int_src_ovr
> 			mp_override_legacy_irq
> 				only process two entries, irq 0 gsi 2 and irq 9 gsi 9
> There are only two entries whose type is ACPI_MADT_TYPE_INTERRUPT_OVERRIDE in MADT table. Is it normal?

Yes, that's what you'd typically see (or just one such entry).

Jan

On 2024/6/5 01:17, Jan Beulich wrote:
> On 04.06.2024 10:18, Chen, Jiqian wrote:
>> I tried to get more debug information from my environment. And I attach them here, maybe you can find some problems.
>> acpi_parse_madt_ioapic_entries
>> 	acpi_table_parse_madt(ACPI_MADT_TYPE_INTERRUPT_OVERRIDE, acpi_parse_int_src_ovr, MAX_IRQ_SOURCES);
>> 		acpi_parse_int_src_ovr
>> 			mp_override_legacy_irq
>> 				only process two entries, irq 0 gsi 2 and irq 9 gsi 9
>> There are only two entries whose type is ACPI_MADT_TYPE_INTERRUPT_OVERRIDE in MADT table. Is it normal?
> 
> Yes, that's what you'd typically see (or just one such entry).
Ok, let me conclude that acpi_parse_int_src_ovr get two entries from MADT table and add them into mp_irqs. They are [irq, gsi][0, 2] and [irq, gsi][9, 9].
Then in the following function mp_config_acpi_legacy_irqs initializes the 1:1 mapping of irq and gsi [0~15 except 2 and 9], and add them into mp_irqs.
But for high GSIs(>= 16), no mapping processing.
Right?

Is it that the Xen hypervisor lacks some handling of high GSIs?
For now, if hypervisor gets a high GSIs, it can't be transformed to irq, because there is no mapping between them.

> 
> Jan

-- 
Best regards,
Jiqian Chen.

On 05.06.2024 09:04, Chen, Jiqian wrote:
> On 2024/6/5 01:17, Jan Beulich wrote:
>> On 04.06.2024 10:18, Chen, Jiqian wrote:
>>> I tried to get more debug information from my environment. And I attach them here, maybe you can find some problems.
>>> acpi_parse_madt_ioapic_entries
>>> 	acpi_table_parse_madt(ACPI_MADT_TYPE_INTERRUPT_OVERRIDE, acpi_parse_int_src_ovr, MAX_IRQ_SOURCES);
>>> 		acpi_parse_int_src_ovr
>>> 			mp_override_legacy_irq
>>> 				only process two entries, irq 0 gsi 2 and irq 9 gsi 9
>>> There are only two entries whose type is ACPI_MADT_TYPE_INTERRUPT_OVERRIDE in MADT table. Is it normal?
>>
>> Yes, that's what you'd typically see (or just one such entry).
> Ok, let me conclude that acpi_parse_int_src_ovr get two entries from MADT table and add them into mp_irqs. They are [irq, gsi][0, 2] and [irq, gsi][9, 9].
> Then in the following function mp_config_acpi_legacy_irqs initializes the 1:1 mapping of irq and gsi [0~15 except 2 and 9], and add them into mp_irqs.
> But for high GSIs(>= 16), no mapping processing.
> Right?

On that specific system of yours - yes. In the general case high GSIs
may have entries, too.

> Is it that the Xen hypervisor lacks some handling of high GSIs?

I don't think so. Unless you can point out something?

> For now, if hypervisor gets a high GSIs, it can't be transformed to irq, because there is no mapping between them.

No, in the absence of a source override (note the word "override") the
default identity mapping applies.

Jan

On 2024/6/5 18:09, Jan Beulich wrote:
> On 05.06.2024 09:04, Chen, Jiqian wrote:
>> On 2024/6/5 01:17, Jan Beulich wrote:
>>> On 04.06.2024 10:18, Chen, Jiqian wrote:
>>>> I tried to get more debug information from my environment. And I attach them here, maybe you can find some problems.
>>>> acpi_parse_madt_ioapic_entries
>>>> 	acpi_table_parse_madt(ACPI_MADT_TYPE_INTERRUPT_OVERRIDE, acpi_parse_int_src_ovr, MAX_IRQ_SOURCES);
>>>> 		acpi_parse_int_src_ovr
>>>> 			mp_override_legacy_irq
>>>> 				only process two entries, irq 0 gsi 2 and irq 9 gsi 9
>>>> There are only two entries whose type is ACPI_MADT_TYPE_INTERRUPT_OVERRIDE in MADT table. Is it normal?
>>>
>>> Yes, that's what you'd typically see (or just one such entry).
>> Ok, let me conclude that acpi_parse_int_src_ovr get two entries from MADT table and add them into mp_irqs. They are [irq, gsi][0, 2] and [irq, gsi][9, 9].
>> Then in the following function mp_config_acpi_legacy_irqs initializes the 1:1 mapping of irq and gsi [0~15 except 2 and 9], and add them into mp_irqs.
>> But for high GSIs(>= 16), no mapping processing.
>> Right?
> 
> On that specific system of yours - yes. In the general case high GSIs
> may have entries, too.
> 
>> Is it that the Xen hypervisor lacks some handling of high GSIs?
> 
> I don't think so. Unless you can point out something?
Ok, so the implementation is still to get mapping from mp_irqs, I will change in next version.
Thank you.

> 
>> For now, if hypervisor gets a high GSIs, it can't be transformed to irq, because there is no mapping between them.
> 
> No, in the absence of a source override (note the word "override") the
> default identity mapping applies.
What is identity mapping? Like the mp_config_acpi_legacy_irqs does?

> 
> Jan

-- 
Best regards,
Jiqian Chen.

On 05.06.2024 12:22, Chen, Jiqian wrote:
> On 2024/6/5 18:09, Jan Beulich wrote:
>> On 05.06.2024 09:04, Chen, Jiqian wrote:
>>> For now, if hypervisor gets a high GSIs, it can't be transformed to irq, because there is no mapping between them.
>>
>> No, in the absence of a source override (note the word "override") the
>> default identity mapping applies.
> What is identity mapping?

GSI == IRQ

Jan