diff mbox series

[XEN,v11,5/8] x86/domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

Message ID 20240630123344.20623-6-Jiqian.Chen@amd.com (mailing list archive)
State Superseded
Headers show
Series Support device passthrough when dom0 is PVH on Xen | expand

Commit Message

Chen, Jiqian June 30, 2024, 12:33 p.m. UTC 
Some type of domain don't have PIRQs, like PVH, it doesn't do
PHYSDEVOP_map_pirq for each gsi. When passthrough a device
to guest base on PVH dom0, callstack
pci_add_dm_done->XEN_DOMCTL_irq_permission will fail at function
domain_pirq_to_irq, because PVH has no mapping of gsi, pirq and
irq on Xen side.
What's more, current hypercall XEN_DOMCTL_irq_permission requires
passing in pirq, it is not suitable for dom0 that doesn't have
PIRQs.

So, add a new hypercall XEN_DOMCTL_gsi_permission to grant the
permission of irq(translate from gsi) to dumU when dom0 has no
PIRQs.

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
 xen/arch/x86/domctl.c              | 33 ++++++++++++++++++++++++++++++
 xen/arch/x86/include/asm/io_apic.h |  2 ++
 xen/arch/x86/io_apic.c             | 17 +++++++++++++++
 xen/arch/x86/mpparse.c             |  3 +--
 xen/include/public/domctl.h        |  8 ++++++++
 xen/xsm/flask/hooks.c              |  1 +
 6 files changed, 62 insertions(+), 2 deletions(-)

Comments

Jan Beulich July 4, 2024, 1:33 p.m. UTC | #1 
On 30.06.2024 14:33, Jiqian Chen wrote:
> @@ -237,6 +238,38 @@ long arch_do_domctl(
>          break;
>      }
>  
> +    case XEN_DOMCTL_gsi_permission:
> +    {
> +        int irq;
> +        uint8_t mask = 1;
> +        unsigned int gsi = domctl->u.gsi_permission.gsi;
> +        bool allow = domctl->u.gsi_permission.allow_access;
> +
> +        /* Check all bits and pads are zero except lowest bit */
> +        ret = -EINVAL;
> +        if ( domctl->u.gsi_permission.allow_access & ( !mask ) )
> +            goto gsi_permission_out;

I'm pretty sure that if you had, as would have been expected, added a
#define to the public header for just the low bit you assign meaning to,
you wouldn't have caused yourself problems here. For one, the
initializer for "allow" will be easy to miss if meaning is assigned to
another of the bits. It sadly _still_ takes the full 8 bits and
converts those to a boolean. And then the check here won't work either.
I don't see what use the local variable "mask" is, but at the very
least I expect in place of ! you mean ~ really.

> +        for ( i = 0; i < ARRAY_SIZE(domctl->u.gsi_permission.pad); ++i )
> +            if ( domctl->u.gsi_permission.pad[i] )
> +                goto gsi_permission_out;
> +
> +        if ( gsi >= nr_irqs_gsi || ( irq = gsi_2_irq(gsi) ) < 0 )

nr_irqs_gsi is the upper bound on IRQs representing a GSI; as said before,
GSIs and IRQs are different number spaces, and hence you can't compare
gsi against nr_irqs_gsi. The (inclusive) upper bound on (valid) GSIs is
mp_ioapic_routing[nr_ioapics - 1].gsi_end, or the return value of
highest_gsi().

Also, style nit: Blanks belong immediately inside parentheses only for the
outer pair of control statements; no inner expressions should have them this
way.

Finally I'd like to ask that you use "<= 0", as we do in various places
elsewhere. IRQ0 is the timer interrupt; we never want to have that used by
any entity outside of Xen itself.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -464,6 +464,12 @@ struct xen_domctl_irq_permission {
>      uint8_t pad[3];
>  };
>  
> +/* 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 */

See above. It's not the field that serves as a flag for the purpose you
state, but just the low bit. You want to rename the field (flags?) and
#define a suitable constant.

Jan
Chen, Jiqian July 8, 2024, 2:28 a.m. UTC | #2 
On 2024/7/4 21:33, Jan Beulich wrote:
> On 30.06.2024 14:33, Jiqian Chen wrote:
>> @@ -237,6 +238,38 @@ long arch_do_domctl(
>>          break;
>>      }
>>  
>> +    case XEN_DOMCTL_gsi_permission:
>> +    {
>> +        int irq;
>> +        uint8_t mask = 1;
>> +        unsigned int gsi = domctl->u.gsi_permission.gsi;
>> +        bool allow = domctl->u.gsi_permission.allow_access;
>> +
>> +        /* Check all bits and pads are zero except lowest bit */
>> +        ret = -EINVAL;
>> +        if ( domctl->u.gsi_permission.allow_access & ( !mask ) )
>> +            goto gsi_permission_out;
> 
> I'm pretty sure that if you had, as would have been expected, added a
> #define to the public header for just the low bit you assign meaning to,
> you wouldn't have caused yourself problems here. For one, the
> initializer for "allow" will be easy to miss if meaning is assigned to
> another of the bits. It sadly _still_ takes the full 8 bits and
> converts those to a boolean. And then the check here won't work either.
> I don't see what use the local variable "mask" is, but at the very
> least I expect in place of ! you mean ~ really.
> 
>> +        for ( i = 0; i < ARRAY_SIZE(domctl->u.gsi_permission.pad); ++i )
>> +            if ( domctl->u.gsi_permission.pad[i] )
>> +                goto gsi_permission_out;
>> +
>> +        if ( gsi >= nr_irqs_gsi || ( irq = gsi_2_irq(gsi) ) < 0 )
> 
> nr_irqs_gsi is the upper bound on IRQs representing a GSI; as said before,
> GSIs and IRQs are different number spaces, and hence you can't compare
> gsi against nr_irqs_gsi. The (inclusive) upper bound on (valid) GSIs is
> mp_ioapic_routing[nr_ioapics - 1].gsi_end, or the return value of
> highest_gsi().
Will change to highest_gsi in next version.

> 
> Also, style nit: Blanks belong immediately inside parentheses only for the
> outer pair of control statements; no inner expressions should have them this
> way.
> 
> Finally I'd like to ask that you use "<= 0", as we do in various places
> elsewhere. IRQ0 is the timer interrupt; we never want to have that used by
> any entity outside of Xen itself.
> 
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -464,6 +464,12 @@ struct xen_domctl_irq_permission {
>>      uint8_t pad[3];
>>  };
>>  
>> +/* 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 */
> 
> See above. It's not the field that serves as a flag for the purpose you
> state, but just the low bit. You want to rename the field (flags?) and
> #define a suitable constant.

You mean?

struct xen_domctl_gsi_permission {
    uint32_t gsi;
#define GSI_PERMISSION_MASK    1
#define GSI_PERMISSION_DISABLE 0
#define GSI_PERMISSION_ENABLE  1
    uint8_t access_flag;    /* flag to specify enable/disable of x86 gsi access */
    uint8_t pad[3];
};

> 
> Jan
Jan Beulich July 8, 2024, 7:01 a.m. UTC | #3 
On 08.07.2024 04:28, Chen, Jiqian wrote:
> On 2024/7/4 21:33, Jan Beulich wrote:
>> On 30.06.2024 14:33, Jiqian Chen wrote:
>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -464,6 +464,12 @@ struct xen_domctl_irq_permission {
>>>      uint8_t pad[3];
>>>  };
>>>  
>>> +/* 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 */
>>
>> See above. It's not the field that serves as a flag for the purpose you
>> state, but just the low bit. You want to rename the field (flags?) and
>> #define a suitable constant.
> 
> You mean?
> 
> struct xen_domctl_gsi_permission {
>     uint32_t gsi;
> #define GSI_PERMISSION_MASK    1
> #define GSI_PERMISSION_DISABLE 0
> #define GSI_PERMISSION_ENABLE  1
>     uint8_t access_flag;    /* flag to specify enable/disable of x86 gsi access */
>     uint8_t pad[3];
> };

Something along these lines, yes. How far to go is a matter of taste; personally
I'd add just a single #define for now. One aspect is important though: The names
you chose are inappropriate. At the very least a XEN_ prefix is wanted, to
reduce the risk of possible name space collisions. Whether to actually use
XEN_DOMCTL_ is perhaps again a matter of taste - the header isn't consistent at
all in this regard.

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 9190e11faaa3..5f20febabbf2 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -36,6 +36,7 @@ 
 #include <asm/xstate.h>
 #include <asm/psr.h>
 #include <asm/cpu-policy.h>
+#include <asm/io_apic.h>
 
 static int update_domain_cpu_policy(struct domain *d,
                                     xen_domctl_cpu_policy_t *xdpc)
@@ -237,6 +238,38 @@  long arch_do_domctl(
         break;
     }
 
+    case XEN_DOMCTL_gsi_permission:
+    {
+        int irq;
+        uint8_t mask = 1;
+        unsigned int gsi = domctl->u.gsi_permission.gsi;
+        bool allow = domctl->u.gsi_permission.allow_access;
+
+        /* Check all bits and pads are zero except lowest bit */
+        ret = -EINVAL;
+        if ( domctl->u.gsi_permission.allow_access & ( !mask ) )
+            goto gsi_permission_out;
+        for ( i = 0; i < ARRAY_SIZE(domctl->u.gsi_permission.pad); ++i )
+            if ( domctl->u.gsi_permission.pad[i] )
+                goto gsi_permission_out;
+
+        if ( gsi >= nr_irqs_gsi || ( irq = gsi_2_irq(gsi) ) < 0 )
+            goto gsi_permission_out;
+
+        ret = -EPERM;
+        if ( !irq_access_permitted(currd, irq) ||
+             xsm_irq_permission(XSM_HOOK, d, irq, allow) )
+            goto gsi_permission_out;
+
+        if ( allow )
+            ret = irq_permit_access(d, irq);
+        else
+            ret = irq_deny_access(d, irq);
+
+    gsi_permission_out:
+        break;
+    }
+
     case XEN_DOMCTL_getpageframeinfo3:
     {
         unsigned int num = domctl->u.getpageframeinfo3.num;
diff --git a/xen/arch/x86/include/asm/io_apic.h b/xen/arch/x86/include/asm/io_apic.h
index 78268ea8f666..7e86d8337758 100644
--- a/xen/arch/x86/include/asm/io_apic.h
+++ b/xen/arch/x86/include/asm/io_apic.h
@@ -213,5 +213,7 @@  unsigned highest_gsi(void);
 
 int ioapic_guest_read( unsigned long physbase, unsigned int reg, u32 *pval);
 int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 val);
+int mp_find_ioapic(int gsi);
+int gsi_2_irq(int gsi);
 
 #endif
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index d73108558e09..d54283955a60 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -955,6 +955,23 @@  static int pin_2_irq(int idx, int apic, int pin)
     return irq;
 }
 
+int gsi_2_irq(int gsi)
+{
+    int ioapic, pin, irq;
+
+    ioapic = mp_find_ioapic(gsi);
+    if ( ioapic < 0 )
+        return -EINVAL;
+
+    pin = gsi - io_apic_gsi_base(ioapic);
+
+    irq = apic_pin_2_gsi_irq(ioapic, pin);
+    if ( irq <= 0 )
+        return -EINVAL;
+
+    return irq;
+}
+
 static inline int IO_APIC_irq_trigger(int irq)
 {
     int apic, idx, pin;
diff --git a/xen/arch/x86/mpparse.c b/xen/arch/x86/mpparse.c
index d8ccab2449c6..c95da0de5770 100644
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -841,8 +841,7 @@  static struct mp_ioapic_routing {
 } mp_ioapic_routing[MAX_IO_APICS];
 
 
-static int mp_find_ioapic (
-	int			gsi)
+int mp_find_ioapic(int gsi)
 {
 	unsigned int		i;
 
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 2a49fe46ce25..f7ae8b19d27d 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -464,6 +464,12 @@  struct xen_domctl_irq_permission {
     uint8_t pad[3];
 };
 
+/* 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 */
+    uint8_t pad[3];
+};
 
 /* XEN_DOMCTL_iomem_permission */
 struct xen_domctl_iomem_permission {
@@ -1306,6 +1312,7 @@  struct xen_domctl {
 #define XEN_DOMCTL_get_paging_mempool_size       85
 #define XEN_DOMCTL_set_paging_mempool_size       86
 #define XEN_DOMCTL_dt_overlay                    87
+#define XEN_DOMCTL_gsi_permission                88
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1328,6 +1335,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
     /*