[PATCH v2 15/26] xen/domctl: wrap xsm_{irq_permission,iomem_permission} with CONFIG_MGMT_HYPERCALLS

Penny Zheng posted 26 patches 5 months ago
There is a newer version of this series
[PATCH v2 15/26] xen/domctl: wrap xsm_{irq_permission,iomem_permission} with CONFIG_MGMT_HYPERCALLS
Posted by Penny Zheng 5 months ago
The following functions are invoked only under
XEN_DOMCTL_{irq_permission,iomem_permission} domctl-op, and shall be wrapped
with CONFIG_MGMT_HYPERCALLS:
- xsm_irq_permission
- xsm_iomem_permission

Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v1 -> v2:
- adapt to changes of "unify DOMCTL to MGMT_HYPERCALLS"
---
 xen/include/xsm/xsm.h | 10 ++++++++++
 xen/xsm/dummy.c       |  2 ++
 xen/xsm/flask/hooks.c |  4 ++++
 3 files changed, 16 insertions(+)

diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 4d332ceca2..1fcd945336 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -113,9 +113,11 @@ struct xsm_ops {
     int (*unmap_domain_irq)(struct domain *d, int irq, const void *data);
     int (*bind_pt_irq)(struct domain *d, struct xen_domctl_bind_pt_irq *bind);
     int (*unbind_pt_irq)(struct domain *d, struct xen_domctl_bind_pt_irq *bind);
+#ifdef CONFIG_MGMT_HYPERCALLS
     int (*irq_permission)(struct domain *d, int pirq, uint8_t allow);
     int (*iomem_permission)(struct domain *d, uint64_t s, uint64_t e,
                             uint8_t allow);
+#endif
     int (*iomem_mapping)(struct domain *d, uint64_t s, uint64_t e,
                          uint8_t allow);
     int (*pci_config_permission)(struct domain *d, uint32_t machine_bdf,
@@ -508,13 +510,21 @@ static inline int xsm_unbind_pt_irq(
 static inline int xsm_irq_permission(
     xsm_default_t def, struct domain *d, int pirq, uint8_t allow)
 {
+#ifdef CONFIG_MGMT_HYPERCALLS
     return alternative_call(xsm_ops.irq_permission, d, pirq, allow);
+#else
+    return -EOPNOTSUPP;
+#endif
 }
 
 static inline int xsm_iomem_permission(
     xsm_default_t def, struct domain *d, uint64_t s, uint64_t e, uint8_t allow)
 {
+#ifdef CONFIG_MGMT_HYPERCALLS
     return alternative_call(xsm_ops.iomem_permission, d, s, e, allow);
+#else
+    return -EOPNOTSUPP;
+#endif
 }
 
 static inline int xsm_iomem_mapping(
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 2c878999a3..b216894579 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -73,8 +73,10 @@ static const struct xsm_ops __initconst_cf_clobber dummy_ops = {
     .unmap_domain_irq              = xsm_unmap_domain_irq,
     .bind_pt_irq                   = xsm_bind_pt_irq,
     .unbind_pt_irq                 = xsm_unbind_pt_irq,
+#ifdef CONFIG_MGMT_HYPERCALLS
     .irq_permission                = xsm_irq_permission,
     .iomem_permission              = xsm_iomem_permission,
+#endif
     .iomem_mapping                 = xsm_iomem_mapping,
     .pci_config_permission         = xsm_pci_config_permission,
     .get_vnumainfo                 = xsm_get_vnumainfo,
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index e8a4deb2ea..198053be77 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1111,12 +1111,14 @@ static int cf_check flask_unbind_pt_irq(
     return current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__REMOVE);
 }
 
+#ifdef CONFIG_MGMT_HYPERCALLS
 static int cf_check flask_irq_permission(
     struct domain *d, int pirq, uint8_t access)
 {
     /* the PIRQ number is not useful; real IRQ is checked during mapping */
     return current_has_perm(d, SECCLASS_RESOURCE, resource_to_perm(access));
 }
+#endif /* CONFIG_MGMT_HYPERCALLS */
 
 struct iomem_has_perm_data {
     uint32_t ssid;
@@ -1943,8 +1945,10 @@ static const struct xsm_ops __initconst_cf_clobber flask_ops = {
     .unmap_domain_irq = flask_unmap_domain_irq,
     .bind_pt_irq = flask_bind_pt_irq,
     .unbind_pt_irq = flask_unbind_pt_irq,
+#ifdef CONFIG_MGMT_HYPERCALLS
     .irq_permission = flask_irq_permission,
     .iomem_permission = flask_iomem_permission,
+#endif
     .iomem_mapping = flask_iomem_mapping,
     .pci_config_permission = flask_pci_config_permission,
 
-- 
2.34.1
Re: [PATCH v2 15/26] xen/domctl: wrap xsm_{irq_permission,iomem_permission} with CONFIG_MGMT_HYPERCALLS
Posted by Jan Beulich 5 months ago
On 10.09.2025 09:38, Penny Zheng wrote:
> @@ -508,13 +510,21 @@ static inline int xsm_unbind_pt_irq(
>  static inline int xsm_irq_permission(
>      xsm_default_t def, struct domain *d, int pirq, uint8_t allow)
>  {
> +#ifdef CONFIG_MGMT_HYPERCALLS
>      return alternative_call(xsm_ops.irq_permission, d, pirq, allow);
> +#else
> +    return -EOPNOTSUPP;
> +#endif
>  }
>  
>  static inline int xsm_iomem_permission(
>      xsm_default_t def, struct domain *d, uint64_t s, uint64_t e, uint8_t allow)
>  {
> +#ifdef CONFIG_MGMT_HYPERCALLS
>      return alternative_call(xsm_ops.iomem_permission, d, s, e, allow);
> +#else
> +    return -EOPNOTSUPP;
> +#endif
>  }

Along the lines of Stefano's comment - why would these inline functions stay
around? Them returning an error in the MGMT_HYPERCALLS=n case is actually a
problem: For xsm_iomem_permission() it's only a conceptual one, but for
xsm_irq_permission() you break x86's handling of XEN_DOMCTL_gsi_permission.
I would have added "transiently", but from the titles of later patches I
can't spot where to expect that one to be taken care of.

> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -1111,12 +1111,14 @@ static int cf_check flask_unbind_pt_irq(
>      return current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__REMOVE);
>  }
>  
> +#ifdef CONFIG_MGMT_HYPERCALLS
>  static int cf_check flask_irq_permission(
>      struct domain *d, int pirq, uint8_t access)
>  {
>      /* the PIRQ number is not useful; real IRQ is checked during mapping */
>      return current_has_perm(d, SECCLASS_RESOURCE, resource_to_perm(access));
>  }
> +#endif /* CONFIG_MGMT_HYPERCALLS */
>  
>  struct iomem_has_perm_data {
>      uint32_t ssid;
> @@ -1943,8 +1945,10 @@ static const struct xsm_ops __initconst_cf_clobber flask_ops = {
>      .unmap_domain_irq = flask_unmap_domain_irq,
>      .bind_pt_irq = flask_bind_pt_irq,
>      .unbind_pt_irq = flask_unbind_pt_irq,
> +#ifdef CONFIG_MGMT_HYPERCALLS
>      .irq_permission = flask_irq_permission,
>      .iomem_permission = flask_iomem_permission,
> +#endif
>      .iomem_mapping = flask_iomem_mapping,
>      .pci_config_permission = flask_pci_config_permission,
>  

It's odd that flask_iomem_permission() remains as a function, but for the
moment that looks to be necessary, as it's (oddly enough) called from
flask_iomem_mapping(). However, for that one I again can't drive from
titles of subsequent patches where it would be taken care of.

Daniel - is this layering actually helpful? Can't we either drop
flask_iomem_mapping() (with the benefit of a cf_check disappearing), or
have it do directly what it wants done, rather than calling the other
hook function?

Having reached the bottom of the patch - what about xsm/dummy.h?

Jan
RE: [PATCH v2 15/26] xen/domctl: wrap xsm_{irq_permission,iomem_permission} with CONFIG_MGMT_HYPERCALLS
Posted by Penny, Zheng 4 months, 2 weeks ago
[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, September 11, 2025 7:02 PM
> To: Penny, Zheng <penny.zheng@amd.com>; Daniel P. Smith
> <dpsmith@apertussolutions.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v2 15/26] xen/domctl: wrap
> xsm_{irq_permission,iomem_permission} with CONFIG_MGMT_HYPERCALLS
>
> On 10.09.2025 09:38, Penny Zheng wrote:
> > --- a/xen/xsm/flask/hooks.c
> > +++ b/xen/xsm/flask/hooks.c
> > @@ -1111,12 +1111,14 @@ static int cf_check flask_unbind_pt_irq(
> >      return current_has_perm(d, SECCLASS_RESOURCE,
> RESOURCE__REMOVE);
> > }
> >
> > +#ifdef CONFIG_MGMT_HYPERCALLS
> >  static int cf_check flask_irq_permission(
> >      struct domain *d, int pirq, uint8_t access)  {
> >      /* the PIRQ number is not useful; real IRQ is checked during mapping */
> >      return current_has_perm(d, SECCLASS_RESOURCE,
> > resource_to_perm(access));  }
> > +#endif /* CONFIG_MGMT_HYPERCALLS */
> >
> >  struct iomem_has_perm_data {
> >      uint32_t ssid;
> > @@ -1943,8 +1945,10 @@ static const struct xsm_ops __initconst_cf_clobber
> flask_ops = {
> >      .unmap_domain_irq = flask_unmap_domain_irq,
> >      .bind_pt_irq = flask_bind_pt_irq,
> >      .unbind_pt_irq = flask_unbind_pt_irq,
> > +#ifdef CONFIG_MGMT_HYPERCALLS
> >      .irq_permission = flask_irq_permission,
> >      .iomem_permission = flask_iomem_permission,
> > +#endif
> >      .iomem_mapping = flask_iomem_mapping,
> >      .pci_config_permission = flask_pci_config_permission,
> >
>
> It's odd that flask_iomem_permission() remains as a function, but for the moment
> that looks to be necessary, as it's (oddly enough) called from
> flask_iomem_mapping(). However, for that one I again can't drive from titles of
> subsequent patches where it would be taken care of.
>
> Daniel - is this layering actually helpful? Can't we either drop
> flask_iomem_mapping() (with the benefit of a cf_check disappearing), or have it do
> directly what it wants done, rather than calling the other hook function?
>

If with no explicit worries, I'll create a new commit in next serie to remove redundant xsm_iomem_mapping(). Then here, we only shall take care of  xsm_irq_permission()

> Having reached the bottom of the patch - what about xsm/dummy.h?
>
> Jan
Re: [PATCH v2 15/26] xen/domctl: wrap xsm_{irq_permission,iomem_permission} with CONFIG_MGMT_HYPERCALLS
Posted by Stefano Stabellini 5 months ago
On Wed, 10 Sep 2025, Penny Zheng wrote:
> The following functions are invoked only under
> XEN_DOMCTL_{irq_permission,iomem_permission} domctl-op, and shall be wrapped
> with CONFIG_MGMT_HYPERCALLS:
> - xsm_irq_permission
> - xsm_iomem_permission
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
> ---
> v1 -> v2:
> - adapt to changes of "unify DOMCTL to MGMT_HYPERCALLS"
> ---
>  xen/include/xsm/xsm.h | 10 ++++++++++
>  xen/xsm/dummy.c       |  2 ++
>  xen/xsm/flask/hooks.c |  4 ++++
>  3 files changed, 16 insertions(+)

there is no change to domctl.c ?


> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index 4d332ceca2..1fcd945336 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -113,9 +113,11 @@ struct xsm_ops {
>      int (*unmap_domain_irq)(struct domain *d, int irq, const void *data);
>      int (*bind_pt_irq)(struct domain *d, struct xen_domctl_bind_pt_irq *bind);
>      int (*unbind_pt_irq)(struct domain *d, struct xen_domctl_bind_pt_irq *bind);
> +#ifdef CONFIG_MGMT_HYPERCALLS
>      int (*irq_permission)(struct domain *d, int pirq, uint8_t allow);
>      int (*iomem_permission)(struct domain *d, uint64_t s, uint64_t e,
>                              uint8_t allow);
> +#endif
>      int (*iomem_mapping)(struct domain *d, uint64_t s, uint64_t e,
>                           uint8_t allow);
>      int (*pci_config_permission)(struct domain *d, uint32_t machine_bdf,
> @@ -508,13 +510,21 @@ static inline int xsm_unbind_pt_irq(
>  static inline int xsm_irq_permission(
>      xsm_default_t def, struct domain *d, int pirq, uint8_t allow)
>  {
> +#ifdef CONFIG_MGMT_HYPERCALLS
>      return alternative_call(xsm_ops.irq_permission, d, pirq, allow);
> +#else
> +    return -EOPNOTSUPP;
> +#endif
>  }
>  
>  static inline int xsm_iomem_permission(
>      xsm_default_t def, struct domain *d, uint64_t s, uint64_t e, uint8_t allow)
>  {
> +#ifdef CONFIG_MGMT_HYPERCALLS
>      return alternative_call(xsm_ops.iomem_permission, d, s, e, allow);
> +#else
> +    return -EOPNOTSUPP;
> +#endif
>  }
>  
>  static inline int xsm_iomem_mapping(
> diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
> index 2c878999a3..b216894579 100644
> --- a/xen/xsm/dummy.c
> +++ b/xen/xsm/dummy.c
> @@ -73,8 +73,10 @@ static const struct xsm_ops __initconst_cf_clobber dummy_ops = {
>      .unmap_domain_irq              = xsm_unmap_domain_irq,
>      .bind_pt_irq                   = xsm_bind_pt_irq,
>      .unbind_pt_irq                 = xsm_unbind_pt_irq,
> +#ifdef CONFIG_MGMT_HYPERCALLS
>      .irq_permission                = xsm_irq_permission,
>      .iomem_permission              = xsm_iomem_permission,
> +#endif
>      .iomem_mapping                 = xsm_iomem_mapping,
>      .pci_config_permission         = xsm_pci_config_permission,
>      .get_vnumainfo                 = xsm_get_vnumainfo,
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index e8a4deb2ea..198053be77 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -1111,12 +1111,14 @@ static int cf_check flask_unbind_pt_irq(
>      return current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__REMOVE);
>  }
>  
> +#ifdef CONFIG_MGMT_HYPERCALLS
>  static int cf_check flask_irq_permission(
>      struct domain *d, int pirq, uint8_t access)
>  {
>      /* the PIRQ number is not useful; real IRQ is checked during mapping */
>      return current_has_perm(d, SECCLASS_RESOURCE, resource_to_perm(access));
>  }
> +#endif /* CONFIG_MGMT_HYPERCALLS */
>  
>  struct iomem_has_perm_data {
>      uint32_t ssid;
> @@ -1943,8 +1945,10 @@ static const struct xsm_ops __initconst_cf_clobber flask_ops = {
>      .unmap_domain_irq = flask_unmap_domain_irq,
>      .bind_pt_irq = flask_bind_pt_irq,
>      .unbind_pt_irq = flask_unbind_pt_irq,
> +#ifdef CONFIG_MGMT_HYPERCALLS
>      .irq_permission = flask_irq_permission,
>      .iomem_permission = flask_iomem_permission,
> +#endif
>      .iomem_mapping = flask_iomem_mapping,
>      .pci_config_permission = flask_pci_config_permission,
>  
> -- 
> 2.34.1
> 
>