Re: [RFC XEN PATCH v5 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

2024-02-25 Thread Chen, Jiqian 
On 2024/2/23 23:59, Anthony PERARD wrote:
> On Thu, Feb 22, 2024 at 07:22:45AM +, Chen, Jiqian wrote:
>> On 2024/2/21 23:03, Anthony PERARD wrote:
>>> On Fri, Jan 12, 2024 at 02:13:17PM +0800, Jiqian Chen wrote:
 diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
 index a1c6e82631e9..4136a860a048 100644
 --- a/tools/libs/light/libxl_pci.c
 +++ b/tools/libs/light/libxl_pci.c
 @@ -1421,6 +1421,8 @@ static void pci_add_dm_done(libxl__egc *egc,
  uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
  uint32_t domainid = domid;
  bool isstubdom = libxl_is_stubdom(ctx, domid, );
 +int gsi;
 +bool has_gsi = true;
>>>
>>> Why is this function has "gsi" variable, and "gsi = irq" but the next
>>> function pci_remove_detached() does only have "irq" variable?
>> Because in pci_add_dm_done, it calls " r = xc_physdev_map_pirq(ctx->xch, 
>> domid, irq, ); ", it passes the pointer of irq in, and then irq will be 
>> changed, so I need to use gsi to save the origin value.
> 
> I'm sorry but unconditional "gsi = irq" looks very wrong, that line
Make sense, I should add a condition and some comments here. Maybe:
/* if has gsi sysfs node, then fscanf() read from the gsi sysfs and 
store it in irq variable, we need to save the original gsi value, because irq 
will be changed in xc_physdev_map_pirq */
if (has_gsi)
gsi = irq;

> needs to be removed or changed, otherwise we have two functions doing the
> same thing just after that (xc_domain_irq_permission and
> xc_domain_gsi_permission). Instead, my guess is that the
> arguments of xc_physdev_map_pirq() needs to be changes. What does
> contain `irq` after the map_pirq() call? A "pirq" of some kind? Maybe
Yes, pirq.

> xc_physdev_map_pirq(ctx->xch, domid, irq, ) would make things more
> clear about what's going on.
> 
> 
> And BTW, maybe renaming the variable "has_gsi" to "is_gsi" might make
> things a bit clearer as well, as in: "the variable 'irq' $is_gsi",
> instead of a variable that have a meaning of "the system $has_gsi"
> without necessarily meaning that the code is using it.
Gsi is a new sysfs node added by my kernel patch, and it is still in discussion 
with PCI Maintainer.
And for compatible with old version of kernel that didn't has gsi sysfs node, 
we still need to use irq here.
So, I named this variable to has_gsi. Is it clearer that changing it to 
has_gsi_sysfs_node?
Maybe I need to add some comments in code to make the usage of gsi clear.

> 
> Maybe using (abusing?) the variable name "irq" might be a bit wrong now?
> What does "GSI" stand for anyway? And about "PIRQ"? This is just some
GSI is x86 specific concept, it is related to IOAPIC-PIN. PIRQ is used to route 
interrupts in Xen.

> question to figure out if there's potentially a better name for the
> variables names.
> 
> Thanks,
> 

-- 
Best regards,
Jiqian Chen.

Re: [RFC XEN PATCH v5 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

2024-02-23 Thread Anthony PERARD 
On Thu, Feb 22, 2024 at 07:22:45AM +, Chen, Jiqian wrote:
> On 2024/2/21 23:03, Anthony PERARD wrote:
> > On Fri, Jan 12, 2024 at 02:13:17PM +0800, Jiqian Chen wrote:
> >> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> >> index a1c6e82631e9..4136a860a048 100644
> >> --- a/tools/libs/light/libxl_pci.c
> >> +++ b/tools/libs/light/libxl_pci.c
> >> @@ -1421,6 +1421,8 @@ static void pci_add_dm_done(libxl__egc *egc,
> >>  uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
> >>  uint32_t domainid = domid;
> >>  bool isstubdom = libxl_is_stubdom(ctx, domid, );
> >> +int gsi;
> >> +bool has_gsi = true;
> > 
> > Why is this function has "gsi" variable, and "gsi = irq" but the next
> > function pci_remove_detached() does only have "irq" variable?
> Because in pci_add_dm_done, it calls " r = xc_physdev_map_pirq(ctx->xch, 
> domid, irq, ); ", it passes the pointer of irq in, and then irq will be 
> changed, so I need to use gsi to save the origin value.

I'm sorry but unconditional "gsi = irq" looks very wrong, that line
needs to be removed or changed, otherwise we have two functions doing the
same thing just after that (xc_domain_irq_permission and
xc_domain_gsi_permission). Instead, my guess is that the
arguments of xc_physdev_map_pirq() needs to be changes. What does
contain `irq` after the map_pirq() call? A "pirq" of some kind? Maybe
xc_physdev_map_pirq(ctx->xch, domid, irq, ) would make things more
clear about what's going on.


And BTW, maybe renaming the variable "has_gsi" to "is_gsi" might make
things a bit clearer as well, as in: "the variable 'irq' $is_gsi",
instead of a variable that have a meaning of "the system $has_gsi"
without necessarily meaning that the code is using it.

Maybe using (abusing?) the variable name "irq" might be a bit wrong now?
What does "GSI" stand for anyway? And about "PIRQ"? This is just some
question to figure out if there's potentially a better name for the
variables names.

Thanks,

-- 
Anthony PERARD

Re: [RFC XEN PATCH v5 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

2024-02-21 Thread Chen, Jiqian 
Hi Anthony,

On 2024/2/21 23:03, Anthony PERARD wrote:
> On Fri, Jan 12, 2024 at 02:13:17PM +0800, Jiqian Chen wrote:
>> diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
>> index f2d9d14b4d9f..448ba2c59ae1 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, );
>>  }
>>  
>> +int xc_domain_gsi_permission(xc_interface *xch,
>> + uint32_t domid,
>> + uint32_t gsi,
>> + bool allow_access)
>> +{
>> +struct xen_domctl domctl = {};
>> +
>> +domctl.cmd = XEN_DOMCTL_gsi_permission;
>> +domctl.domain = domid;
>> +domctl.u.gsi_permission.gsi = gsi;
>> +domctl.u.gsi_permission.allow_access = allow_access;
> 
> This could be written as:
> struct xen_domctl domctl = {
> .cmd = XEN_DOMCTL_gsi_permission,
> .domain = domid,
> .u.gsi_permission.gsi = gsi,
> .u.gsi_permission.allow_access = allow_access,
> };
> 
That's fine, I will change to this in next version.

> but your change is fine too.
> 
> 
>> +
>> +return do_domctl(xch, );
>> +}
>> +
>>  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 a1c6e82631e9..4136a860a048 100644
>> --- a/tools/libs/light/libxl_pci.c
>> +++ b/tools/libs/light/libxl_pci.c
>> @@ -1421,6 +1421,8 @@ static void pci_add_dm_done(libxl__egc *egc,
>>  uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
>>  uint32_t domainid = domid;
>>  bool isstubdom = libxl_is_stubdom(ctx, domid, );
>> +int gsi;
>> +bool has_gsi = true;
> 
> Why is this function has "gsi" variable, and "gsi = irq" but the next
> function pci_remove_detached() does only have "irq" variable?
Because in pci_add_dm_done, it calls " r = xc_physdev_map_pirq(ctx->xch, domid, 
irq, ); ", it passes the pointer of irq in, and then irq will be changed, 
so I need to use gsi to save the origin value.

> 
> So, gsi can only be positive integer, right? Could we forgo `has_gsi` and
> just set "gsi = -1" when there's no gsi?
No, we can't forgo 'has_gsi', because we need to set gsi = irq to save the 
original val.

> 
>>  /* Convenience aliases */
>>  bool starting = pas->starting;
>> @@ -1482,6 +1484,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>>  pci->bus, pci->dev, pci->func);
>>  
>>  if ( access(sysfs_path, F_OK) != 0 ) {
>> +has_gsi = false;
>>  if ( errno == ENOENT )
>>  sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", 
>> pci->domain,
>>  pci->bus, pci->dev, pci->func);
>> @@ -1497,6 +1500,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>>  goto out_no_irq;
>>  }
>>  if ((fscanf(f, "%u", ) == 1) && irq) {
>> +gsi = irq;
> 
> Why do you do this, that that mean that `gsi` and `irq` are the same? Or
> does `irq` happen to sometime contain a `gsi` value?
Above reason.

> 
>>  r = xc_physdev_map_pirq(ctx->xch, domid, irq, );
>>  if (r < 0) {
>>  LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d (error=%d)",
>> @@ -1505,7 +1509,10 @@ static void pci_add_dm_done(libxl__egc *egc,
>>  rc = ERROR_FAIL;
>>  goto out;
>>  }
>> -r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
>> +if ( has_gsi )
>> +r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1);
>> +if ( !has_gsi || r == -EOPNOTSUPP )
>> +r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
>>  if (r < 0) {
>>  LOGED(ERROR, domainid,
>>"xc_domain_irq_permission irq=%d (error=%d)", irq, r);
> 
> Looks like this error message could be wrong, I think we want to know if
> which call between xc_domain_irq_permission() and
> xc_domain_gsi_permission() has failed.
You are right, I will separate them in next version.

> 
>> @@ -2185,6 +2192,7 @@ static void pci_remove_detached(libxl__egc *egc,
>>  FILE *f;
>>  uint32_t domainid = prs->domid;
>>  bool isstubdom;
>> +bool has_gsi = true;
>>  
>>  /* Convenience aliases */
>>  libxl_device_pci *const pci = >pci;
>> @@ -2244,6 +2252,7 @@ skip_bar:
>> pci->bus, pci->dev, pci->func);
>>  
>>  if ( access(sysfs_path, F_OK) != 0 ) {
>> +has_gsi = false;
>>  if ( errno == ENOENT )
>>  sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", 
>> pci->domain,
>>  pci->bus, pci->dev, pci->func);
>> @@ -2270,7 +2279,10 @@ skip_bar:
>>   */
>>  LOGED(ERROR, domid, "xc_physdev_unmap_pirq irq=%d", irq);
>>  }
>> -rc =

Re: [RFC XEN PATCH v5 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

2024-02-21 Thread Anthony PERARD 
On Fri, Jan 12, 2024 at 02:13:17PM +0800, Jiqian Chen wrote:
> diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
> index f2d9d14b4d9f..448ba2c59ae1 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, );
>  }
>  
> +int xc_domain_gsi_permission(xc_interface *xch,
> + uint32_t domid,
> + uint32_t gsi,
> + bool allow_access)
> +{
> +struct xen_domctl domctl = {};
> +
> +domctl.cmd = XEN_DOMCTL_gsi_permission;
> +domctl.domain = domid;
> +domctl.u.gsi_permission.gsi = gsi;
> +domctl.u.gsi_permission.allow_access = allow_access;

This could be written as:
struct xen_domctl domctl = {
.cmd = XEN_DOMCTL_gsi_permission,
.domain = domid,
.u.gsi_permission.gsi = gsi,
.u.gsi_permission.allow_access = allow_access,
};

but your change is fine too.


> +
> +return do_domctl(xch, );
> +}
> +
>  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 a1c6e82631e9..4136a860a048 100644
> --- a/tools/libs/light/libxl_pci.c
> +++ b/tools/libs/light/libxl_pci.c
> @@ -1421,6 +1421,8 @@ static void pci_add_dm_done(libxl__egc *egc,
>  uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
>  uint32_t domainid = domid;
>  bool isstubdom = libxl_is_stubdom(ctx, domid, );
> +int gsi;
> +bool has_gsi = true;

Why is this function has "gsi" variable, and "gsi = irq" but the next
function pci_remove_detached() does only have "irq" variable?

So, gsi can only be positive integer, right? Could we forgo `has_gsi` and
just set "gsi = -1" when there's no gsi?

>  /* Convenience aliases */
>  bool starting = pas->starting;
> @@ -1482,6 +1484,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>  pci->bus, pci->dev, pci->func);
>  
>  if ( access(sysfs_path, F_OK) != 0 ) {
> +has_gsi = false;
>  if ( errno == ENOENT )
>  sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", 
> pci->domain,
>  pci->bus, pci->dev, pci->func);
> @@ -1497,6 +1500,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>  goto out_no_irq;
>  }
>  if ((fscanf(f, "%u", ) == 1) && irq) {
> +gsi = irq;

Why do you do this, that that mean that `gsi` and `irq` are the same? Or
does `irq` happen to sometime contain a `gsi` value?

>  r = xc_physdev_map_pirq(ctx->xch, domid, irq, );
>  if (r < 0) {
>  LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d (error=%d)",
> @@ -1505,7 +1509,10 @@ static void pci_add_dm_done(libxl__egc *egc,
>  rc = ERROR_FAIL;
>  goto out;
>  }
> -r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
> +if ( has_gsi )
> +r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1);
> +if ( !has_gsi || r == -EOPNOTSUPP )
> +r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
>  if (r < 0) {
>  LOGED(ERROR, domainid,
>"xc_domain_irq_permission irq=%d (error=%d)", irq, r);

Looks like this error message could be wrong, I think we want to know if
which call between xc_domain_irq_permission() and
xc_domain_gsi_permission() has failed.

> @@ -2185,6 +2192,7 @@ static void pci_remove_detached(libxl__egc *egc,
>  FILE *f;
>  uint32_t domainid = prs->domid;
>  bool isstubdom;
> +bool has_gsi = true;
>  
>  /* Convenience aliases */
>  libxl_device_pci *const pci = >pci;
> @@ -2244,6 +2252,7 @@ skip_bar:
> pci->bus, pci->dev, pci->func);
>  
>  if ( access(sysfs_path, F_OK) != 0 ) {
> +has_gsi = false;
>  if ( errno == ENOENT )
>  sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", 
> pci->domain,
>  pci->bus, pci->dev, pci->func);
> @@ -2270,7 +2279,10 @@ skip_bar:
>   */
>  LOGED(ERROR, domid, "xc_physdev_unmap_pirq irq=%d", irq);
>  }
> -rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
> +if ( has_gsi )
> +rc = xc_domain_gsi_permission(ctx->xch, domid, irq, 0);

Why do you use the xc_domain_gsi_permission() with "irq" here, but not
in pci_add_dm_done() ?

> +if ( !has_gsi || rc == -EOPNOTSUPP )
> +rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
>  if (rc < 0) {
>  LOGED(ERROR, domid, "xc_domain_irq_permission irq=%d", irq);
>  }


These changes to libxl are quite confusing, there's a mixed up between
"gsi" and "irq", it's hard to follow.

Thanks,

-- 
Anthony PERARD

[RFC XEN PATCH v5 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

2024-01-11 Thread Jiqian Chen 
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.

Co-developed-by: Huang Rui 
Signed-off-by: Jiqian Chen 
---
 tools/include/xenctrl.h  |  5 +
 tools/libs/ctrl/xc_domain.c  | 15 +++
 tools/libs/light/libxl_pci.c | 16 ++--
 xen/arch/x86/domctl.c| 31 +++
 xen/include/public/domctl.h  |  9 +
 xen/xsm/flask/hooks.c|  1 +
 6 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 2ef8b4e05422..519c860a00d5 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..448ba2c59ae1 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, );
 }
 
+int xc_domain_gsi_permission(xc_interface *xch,
+ uint32_t domid,
+ uint32_t gsi,
+ bool allow_access)
+{
+struct xen_domctl domctl = {};
+
+domctl.cmd = XEN_DOMCTL_gsi_permission;
+domctl.domain = domid;
+domctl.u.gsi_permission.gsi = gsi;
+domctl.u.gsi_permission.allow_access = allow_access;
+
+return do_domctl(xch, );
+}
+
 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 a1c6e82631e9..4136a860a048 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -1421,6 +1421,8 @@ static void pci_add_dm_done(libxl__egc *egc,
 uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
 uint32_t domainid = domid;
 bool isstubdom = libxl_is_stubdom(ctx, domid, );
+int gsi;
+bool has_gsi = true;
 
 /* Convenience aliases */
 bool starting = pas->starting;
@@ -1482,6 +1484,7 @@ static void pci_add_dm_done(libxl__egc *egc,
 pci->bus, pci->dev, pci->func);
 
 if ( access(sysfs_path, F_OK) != 0 ) {
+has_gsi = false;
 if ( errno == ENOENT )
 sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
 pci->bus, pci->dev, pci->func);
@@ -1497,6 +1500,7 @@ static void pci_add_dm_done(libxl__egc *egc,
 goto out_no_irq;
 }
 if ((fscanf(f, "%u", ) == 1) && irq) {
+gsi = irq;
 r = xc_physdev_map_pirq(ctx->xch, domid, irq, );
 if (r < 0) {
 LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d (error=%d)",
@@ -1505,7 +1509,10 @@ static void pci_add_dm_done(libxl__egc *egc,
 rc = ERROR_FAIL;
 goto out;
 }
-r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
+if ( has_gsi )
+r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1);
+if ( !has_gsi || r == -EOPNOTSUPP )
+r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
 if (r < 0) {
 LOGED(ERROR, domainid,
   "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
@@ -2185,6 +2192,7 @@ static void pci_remove_detached(libxl__egc *egc,
 FILE *f;
 uint32_t domainid = prs->domid;
 bool isstubdom;
+bool has_gsi = true;
 
 /* Convenience aliases */
 libxl_device_pci *const pci = >pci;
@@ -2244,6 +2252,7 @@ skip_bar:
pci->bus, pci->dev, pci->func);
 
 if ( access(sysfs_path, F_OK) != 0 ) {
+has_gsi = false;
 if ( errno == ENOENT )
 sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
 pci->bus, pci->dev, pci->func);
@@ -2270,7 +2279,10 @@ skip_bar:
  */
 LOGED(ERROR, domid, "xc_physdev_unmap_pirq irq=%d", irq);
 }
-rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
+if ( has_gsi )
+rc = xc_domain_gsi_permission(ctx->xch, domid, irq, 0);
+if ( !has_gsi || rc == -EOPNOTSUPP )
+rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
 if (rc < 0) {