| Message ID | 20240911065832.1591273-3-Jiqian.Chen@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support device passthrough when dom0 is PVH on Xen | expand |
Hi Daniel, On 2024/9/11 14:58, Chen, Jiqian wrote: > Some domains are not aware of the pIRQ abstraction layer that maps > interrupt sources into Xen space interrupt numbers. pIRQs values are > only exposed to domains that have the option to route physical > interrupts over event channels. > > This creates issues for PCI-passthrough from a PVH domain, as some of > the passthrough related hypercalls use pIRQ as references to physical > interrupts on the system. One of such interfaces is > XEN_DOMCTL_irq_permission, used to grant or revoke access to > interrupts, takes a pIRQ as the reference to the interrupt to be > adjusted. > > Since PVH doesn't manage interrupts in terms of pIRQs, introduce a new > hypercall that allows setting interrupt permissions based on GSI value > rather than pIRQ. > > Note the GSI hypercall parameters is translated to an IRQ value (in > case there are ACPI overrides) before doing the checks. > > 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> > --- > CC: Daniel P . Smith <dpsmith@apertussolutions.com> > Remaining comment @Daniel P . Smith: > + ret = -EPERM; > + if ( !irq_access_permitted(currd, irq) || > + xsm_irq_permission(XSM_HOOK, d, irq, flags) ) > + break; > Is it okay to issue the XSM check using the translated value(irq), > not the one(gsi) that was originally passed into the hypercall? Could you please take a time to answer at this question? > --- > v13->v15 changes: > Change to use the commit message wrote by Roger. > Change the code comment from "Check all bits are zero except lowest bit" to "Check only valid bits are set". > Change the end return sentence of gsi_2_irq to "return irq ?: -EINVAL;" to preserve the error code from apic_pin_2_gsi_irq(). > > v12->v13 changes: > For struct xen_domctl_gsi_permission, rename "access_flag" to "flags", change its type from uint8_t to uint32_t, delete "pad", add XEN_DOMCTL_GSI_REVOKE and XEN_DOMCTL_GSI_GRANT macros. > Move "gsi > highest_gsi()" into function gsi_2_irq. > Modify parameter gsi in function gsi_2_irq and mp_find_ioapic to unsigned int type. > Delete unnecessary spaces and brackets around "~XEN_DOMCTL_GSI_ACTION_MASK". > Delete unnecessary goto statements and change to direct break. > Add description in commit message to explain how gsi to irq isconverted. > > v11->v12 changes: > Change nr_irqs_gsi to highest_gsi() to check gsi boundary, then need to remove "__init" of highest_gsi function. > Change the check of irq boundary from <0 to <=0, and remove unnecessary space. > Add #define XEN_DOMCTL_GSI_PERMISSION_MASK 1 to get lowest bit. > > v10->v11 changes: > Extracted from patch#5 of v10 into a separate patch. > Add non-zero judgment for other bits of allow_access. > Delete unnecessary judgment "if ( is_pv_domain(currd) || has_pirq(currd) )". > Change the error exit path identifier "out" to "gsi_permission_out". > Use ARRAY_SIZE() instead of open coed. > > v9->v10 changes: > Modified the commit message to further describe the purpose of adding XEN_DOMCTL_gsi_permission. > Added a check for all zeros in the padding field in XEN_DOMCTL_gsi_permission, and used currd instead of current->domain. > In the function gsi_2_irq, apic_pin_2_gsi_irq was used instead of the original new code, and error handling for irq0 was added. > Deleted the extra spaces in the upper and lower lines of the struct xen_domctl_gsi_permission definition. > > v8->v9 changes: > Change the commit message to describe more why we need this new hypercall. > Add comment above "if ( is_pv_domain(current->domain) || has_pirq(current->domain) )" to explain why we need this check. > Add gsi_2_irq to transform gsi to irq, instead of considering gsi == irq. > Add explicit padding to struct xen_domctl_gsi_permission. > > v5->v8 changes: > Nothing. > > v4->v5 changes: > New implementation to add new hypercall XEN_DOMCTL_gsi_permission to grant gsi. > --- > xen/arch/x86/domctl.c | 29 +++++++++++++++++++++++++++++ > xen/arch/x86/include/asm/io_apic.h | 2 ++ > xen/arch/x86/io_apic.c | 19 +++++++++++++++++++ > xen/arch/x86/mpparse.c | 7 +++---- > xen/include/public/domctl.h | 10 ++++++++++ > xen/xsm/flask/hooks.c | 1 + > 6 files changed, 64 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c > index 68b5b46d1a83..939b1de0ee59 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,34 @@ long arch_do_domctl( > break; > } > > + case XEN_DOMCTL_gsi_permission: > + { > + int irq; > + unsigned int gsi = domctl->u.gsi_permission.gsi; > + uint32_t flags = domctl->u.gsi_permission.flags; > + > + /* Check only valid bits are set */ > + ret = -EINVAL; > + if ( flags & ~XEN_DOMCTL_GSI_ACTION_MASK ) > + break; > + > + ret = irq = gsi_2_irq(gsi); > + if ( ret <= 0 ) > + break; > + > + ret = -EPERM; > + if ( !irq_access_permitted(currd, irq) || > + xsm_irq_permission(XSM_HOOK, d, irq, flags) ) > + break; > + > + if ( flags ) > + ret = irq_permit_access(d, irq); > + else > + ret = irq_deny_access(d, irq); > + > + 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..62456806c7af 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(unsigned int gsi); > +int gsi_2_irq(unsigned int gsi); > > #endif > diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c > index 772700584639..e40d2f7dbd75 100644 > --- a/xen/arch/x86/io_apic.c > +++ b/xen/arch/x86/io_apic.c > @@ -955,6 +955,25 @@ static int pin_2_irq(int idx, int apic, int pin) > return irq; > } > > +int gsi_2_irq(unsigned int gsi) > +{ > + int ioapic, irq; > + unsigned int pin; > + > + if ( gsi > highest_gsi() ) > + return -ERANGE; > + > + 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); > + > + return irq ?: -EINVAL; > +} > + > 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 306d8ed97a83..e13b83bbe9dd 100644 > --- a/xen/arch/x86/mpparse.c > +++ b/xen/arch/x86/mpparse.c > @@ -842,8 +842,7 @@ static struct mp_ioapic_routing { > } mp_ioapic_routing[MAX_IO_APICS]; > > > -static int mp_find_ioapic ( > - int gsi) > +int mp_find_ioapic(unsigned int gsi) > { > unsigned int i; > > @@ -854,7 +853,7 @@ static int mp_find_ioapic ( > return i; > } > > - printk(KERN_ERR "ERROR: Unable to locate IOAPIC for GSI %d\n", gsi); > + printk(KERN_ERR "ERROR: Unable to locate IOAPIC for GSI %u\n", gsi); > > return -1; > } > @@ -915,7 +914,7 @@ void __init mp_register_ioapic ( > return; > } > > -unsigned __init highest_gsi(void) > +unsigned highest_gsi(void) > { > unsigned x, res = 0; > for (x = 0; x < nr_ioapics; x++) > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index 2a49fe46ce25..e1028fc524cf 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -464,6 +464,14 @@ struct xen_domctl_irq_permission { > uint8_t pad[3]; > }; > > +/* XEN_DOMCTL_gsi_permission */ > +struct xen_domctl_gsi_permission { > + uint32_t gsi; > +#define XEN_DOMCTL_GSI_REVOKE 0 > +#define XEN_DOMCTL_GSI_GRANT 1 > +#define XEN_DOMCTL_GSI_ACTION_MASK 1 > + uint32_t flags; > +}; > > /* XEN_DOMCTL_iomem_permission */ > struct xen_domctl_iomem_permission { > @@ -1306,6 +1314,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 +1337,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 278ad38c2af3..dfa23738cd8a 100644 > --- a/xen/xsm/flask/hooks.c > +++ b/xen/xsm/flask/hooks.c > @@ -695,6 +695,7 @@ static int cf_check flask_domctl(struct domain *d, unsigned 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 > /*
Greetings! On 9/11/24 02:58, Jiqian Chen wrote: > Some domains are not aware of the pIRQ abstraction layer that maps > interrupt sources into Xen space interrupt numbers. pIRQs values are > only exposed to domains that have the option to route physical > interrupts over event channels. > > This creates issues for PCI-passthrough from a PVH domain, as some of > the passthrough related hypercalls use pIRQ as references to physical > interrupts on the system. One of such interfaces is > XEN_DOMCTL_irq_permission, used to grant or revoke access to > interrupts, takes a pIRQ as the reference to the interrupt to be > adjusted. > > Since PVH doesn't manage interrupts in terms of pIRQs, introduce a new > hypercall that allows setting interrupt permissions based on GSI value > rather than pIRQ. > > Note the GSI hypercall parameters is translated to an IRQ value (in > case there are ACPI overrides) before doing the checks. > > 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> > --- > CC: Daniel P . Smith <dpsmith@apertussolutions.com> > Remaining comment @Daniel P . Smith: > + ret = -EPERM; > + if ( !irq_access_permitted(currd, irq) || > + xsm_irq_permission(XSM_HOOK, d, irq, flags) ) > + break; > Is it okay to issue the XSM check using the translated value(irq), > not the one(gsi) that was originally passed into the hypercall? I don't see why this should be an issue. What is important is that labeling and access checks are in alignment. To my knowledge, it is expected that GSI are evaluated to their IRQ for determining the appropriate label. I have one code style comment below, but beyond that: Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com> > --- > v13->v15 changes: > Change to use the commit message wrote by Roger. > Change the code comment from "Check all bits are zero except lowest bit" to "Check only valid bits are set". > Change the end return sentence of gsi_2_irq to "return irq ?: -EINVAL;" to preserve the error code from apic_pin_2_gsi_irq(). > > v12->v13 changes: > For struct xen_domctl_gsi_permission, rename "access_flag" to "flags", change its type from uint8_t to uint32_t, delete "pad", add XEN_DOMCTL_GSI_REVOKE and XEN_DOMCTL_GSI_GRANT macros. > Move "gsi > highest_gsi()" into function gsi_2_irq. > Modify parameter gsi in function gsi_2_irq and mp_find_ioapic to unsigned int type. > Delete unnecessary spaces and brackets around "~XEN_DOMCTL_GSI_ACTION_MASK". > Delete unnecessary goto statements and change to direct break. > Add description in commit message to explain how gsi to irq isconverted. > > v11->v12 changes: > Change nr_irqs_gsi to highest_gsi() to check gsi boundary, then need to remove "__init" of highest_gsi function. > Change the check of irq boundary from <0 to <=0, and remove unnecessary space. > Add #define XEN_DOMCTL_GSI_PERMISSION_MASK 1 to get lowest bit. > > v10->v11 changes: > Extracted from patch#5 of v10 into a separate patch. > Add non-zero judgment for other bits of allow_access. > Delete unnecessary judgment "if ( is_pv_domain(currd) || has_pirq(currd) )". > Change the error exit path identifier "out" to "gsi_permission_out". > Use ARRAY_SIZE() instead of open coed. > > v9->v10 changes: > Modified the commit message to further describe the purpose of adding XEN_DOMCTL_gsi_permission. > Added a check for all zeros in the padding field in XEN_DOMCTL_gsi_permission, and used currd instead of current->domain. > In the function gsi_2_irq, apic_pin_2_gsi_irq was used instead of the original new code, and error handling for irq0 was added. > Deleted the extra spaces in the upper and lower lines of the struct xen_domctl_gsi_permission definition. > > v8->v9 changes: > Change the commit message to describe more why we need this new hypercall. > Add comment above "if ( is_pv_domain(current->domain) || has_pirq(current->domain) )" to explain why we need this check. > Add gsi_2_irq to transform gsi to irq, instead of considering gsi == irq. > Add explicit padding to struct xen_domctl_gsi_permission. > > v5->v8 changes: > Nothing. > > v4->v5 changes: > New implementation to add new hypercall XEN_DOMCTL_gsi_permission to grant gsi. > --- > xen/arch/x86/domctl.c | 29 +++++++++++++++++++++++++++++ > xen/arch/x86/include/asm/io_apic.h | 2 ++ > xen/arch/x86/io_apic.c | 19 +++++++++++++++++++ > xen/arch/x86/mpparse.c | 7 +++---- > xen/include/public/domctl.h | 10 ++++++++++ > xen/xsm/flask/hooks.c | 1 + > 6 files changed, 64 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c > index 68b5b46d1a83..939b1de0ee59 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,34 @@ long arch_do_domctl( > break; > } > > + case XEN_DOMCTL_gsi_permission: > + { > + int irq; > + unsigned int gsi = domctl->u.gsi_permission.gsi; > + uint32_t flags = domctl->u.gsi_permission.flags; > + > + /* Check only valid bits are set */ > + ret = -EINVAL; > + if ( flags & ~XEN_DOMCTL_GSI_ACTION_MASK ) > + break; > + > + ret = irq = gsi_2_irq(gsi); I was recently informed that a = b = c; form is not MISRA compliant. Since you just overwrite ret after the check, why not drop the assignment to ret and mae the next check against irq instead. > + if ( ret <= 0 ) > + break; > + > + ret = -EPERM; > + if ( !irq_access_permitted(currd, irq) || > + xsm_irq_permission(XSM_HOOK, d, irq, flags) ) > + break; > + > + if ( flags ) > + ret = irq_permit_access(d, irq); > + else > + ret = irq_deny_access(d, irq); > + > + 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..62456806c7af 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(unsigned int gsi); > +int gsi_2_irq(unsigned int gsi); > > #endif > diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c > index 772700584639..e40d2f7dbd75 100644 > --- a/xen/arch/x86/io_apic.c > +++ b/xen/arch/x86/io_apic.c > @@ -955,6 +955,25 @@ static int pin_2_irq(int idx, int apic, int pin) > return irq; > } > > +int gsi_2_irq(unsigned int gsi) > +{ > + int ioapic, irq; > + unsigned int pin; > + > + if ( gsi > highest_gsi() ) > + return -ERANGE; > + > + 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); > + > + return irq ?: -EINVAL; > +} > + > 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 306d8ed97a83..e13b83bbe9dd 100644 > --- a/xen/arch/x86/mpparse.c > +++ b/xen/arch/x86/mpparse.c > @@ -842,8 +842,7 @@ static struct mp_ioapic_routing { > } mp_ioapic_routing[MAX_IO_APICS]; > > > -static int mp_find_ioapic ( > - int gsi) > +int mp_find_ioapic(unsigned int gsi) > { > unsigned int i; > > @@ -854,7 +853,7 @@ static int mp_find_ioapic ( > return i; > } > > - printk(KERN_ERR "ERROR: Unable to locate IOAPIC for GSI %d\n", gsi); > + printk(KERN_ERR "ERROR: Unable to locate IOAPIC for GSI %u\n", gsi); > > return -1; > } > @@ -915,7 +914,7 @@ void __init mp_register_ioapic ( > return; > } > > -unsigned __init highest_gsi(void) > +unsigned highest_gsi(void) > { > unsigned x, res = 0; > for (x = 0; x < nr_ioapics; x++) > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index 2a49fe46ce25..e1028fc524cf 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -464,6 +464,14 @@ struct xen_domctl_irq_permission { > uint8_t pad[3]; > }; > > +/* XEN_DOMCTL_gsi_permission */ > +struct xen_domctl_gsi_permission { > + uint32_t gsi; > +#define XEN_DOMCTL_GSI_REVOKE 0 > +#define XEN_DOMCTL_GSI_GRANT 1 > +#define XEN_DOMCTL_GSI_ACTION_MASK 1 > + uint32_t flags; > +}; > > /* XEN_DOMCTL_iomem_permission */ > struct xen_domctl_iomem_permission { > @@ -1306,6 +1314,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 +1337,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 278ad38c2af3..dfa23738cd8a 100644 > --- a/xen/xsm/flask/hooks.c > +++ b/xen/xsm/flask/hooks.c > @@ -695,6 +695,7 @@ static int cf_check flask_domctl(struct domain *d, unsigned 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 > /*
On 12.09.2024 12:34, Daniel P. Smith wrote: > On 9/11/24 02:58, Jiqian Chen wrote: >> @@ -237,6 +238,34 @@ long arch_do_domctl( >> break; >> } >> >> + case XEN_DOMCTL_gsi_permission: >> + { >> + int irq; >> + unsigned int gsi = domctl->u.gsi_permission.gsi; >> + uint32_t flags = domctl->u.gsi_permission.flags; >> + >> + /* Check only valid bits are set */ >> + ret = -EINVAL; >> + if ( flags & ~XEN_DOMCTL_GSI_ACTION_MASK ) >> + break; >> + >> + ret = irq = gsi_2_irq(gsi); > > I was recently informed that a = b = c; form is not MISRA compliant. > Since you just overwrite ret after the check, why not drop the > assignment to ret and mae the next check against irq instead. The Misra concern is valid, yet the suggestion doesn't look to be quite matching what is needed. After all if we take ... >> + if ( ret <= 0 ) >> + break; ... the "break" path, "ret" needs to be set. Plus there's the problem of "ret" being zero when exiting the function indicates success, yet this is an error path (requiring ret < 0). So overall perhaps irq = gsi_2_irq(gsi); if ( irq <= 0 ) { ret = irq ?: -EACCES; break; } ? Jan
On 2024/9/12 18:51, Jan Beulich wrote: > On 12.09.2024 12:34, Daniel P. Smith wrote: >> On 9/11/24 02:58, Jiqian Chen wrote: >>> @@ -237,6 +238,34 @@ long arch_do_domctl( >>> break; >>> } >>> >>> + case XEN_DOMCTL_gsi_permission: >>> + { >>> + int irq; >>> + unsigned int gsi = domctl->u.gsi_permission.gsi; >>> + uint32_t flags = domctl->u.gsi_permission.flags; >>> + >>> + /* Check only valid bits are set */ >>> + ret = -EINVAL; >>> + if ( flags & ~XEN_DOMCTL_GSI_ACTION_MASK ) >>> + break; >>> + >>> + ret = irq = gsi_2_irq(gsi); >> >> I was recently informed that a = b = c; form is not MISRA compliant. >> Since you just overwrite ret after the check, why not drop the >> assignment to ret and mae the next check against irq instead. > > The Misra concern is valid, yet the suggestion doesn't look to be quite > matching what is needed. After all if we take ... > >>> + if ( ret <= 0 ) >>> + break; > > ... the "break" path, "ret" needs to be set. Plus there's the problem of > "ret" being zero when exiting the function indicates success, yet this > is an error path (requiring ret < 0). So overall perhaps > > irq = gsi_2_irq(gsi); > if ( irq <= 0 ) > { > ret = irq ?: -EACCES; > break; > } > > ? Yes, ret needs to be set. And since gsi_2_irq doesn't return 0(if irq is 0, gsi_2_irq returns -EINVAL). Maybe below is enough? irq = gsi_2_irq(gsi); if ( irq < 0 ) { ret = irq; break; } > > Jan
On 13.09.2024 04:38, Chen, Jiqian wrote: > On 2024/9/12 18:51, Jan Beulich wrote: >> On 12.09.2024 12:34, Daniel P. Smith wrote: >>> On 9/11/24 02:58, Jiqian Chen wrote: >>>> @@ -237,6 +238,34 @@ long arch_do_domctl( >>>> break; >>>> } >>>> >>>> + case XEN_DOMCTL_gsi_permission: >>>> + { >>>> + int irq; >>>> + unsigned int gsi = domctl->u.gsi_permission.gsi; >>>> + uint32_t flags = domctl->u.gsi_permission.flags; >>>> + >>>> + /* Check only valid bits are set */ >>>> + ret = -EINVAL; >>>> + if ( flags & ~XEN_DOMCTL_GSI_ACTION_MASK ) >>>> + break; >>>> + >>>> + ret = irq = gsi_2_irq(gsi); >>> >>> I was recently informed that a = b = c; form is not MISRA compliant. >>> Since you just overwrite ret after the check, why not drop the >>> assignment to ret and mae the next check against irq instead. >> >> The Misra concern is valid, yet the suggestion doesn't look to be quite >> matching what is needed. After all if we take ... >> >>>> + if ( ret <= 0 ) >>>> + break; >> >> ... the "break" path, "ret" needs to be set. Plus there's the problem of >> "ret" being zero when exiting the function indicates success, yet this >> is an error path (requiring ret < 0). So overall perhaps >> >> irq = gsi_2_irq(gsi); >> if ( irq <= 0 ) >> { >> ret = irq ?: -EACCES; >> break; >> } >> >> ? > > Yes, ret needs to be set. And since gsi_2_irq doesn't return 0(if irq is 0, gsi_2_irq returns -EINVAL). > Maybe below is enough? > irq = gsi_2_irq(gsi); > if ( irq < 0 ) > { > ret = irq; > break; > } My proposal was to cover that elsewhere we exclude IRQ0, and hence it would be good to be consistent here. Jan
On 2024/9/13 14:52, Jan Beulich wrote: > On 13.09.2024 04:38, Chen, Jiqian wrote: >> On 2024/9/12 18:51, Jan Beulich wrote: >>> On 12.09.2024 12:34, Daniel P. Smith wrote: >>>> On 9/11/24 02:58, Jiqian Chen wrote: >>>>> @@ -237,6 +238,34 @@ long arch_do_domctl( >>>>> break; >>>>> } >>>>> >>>>> + case XEN_DOMCTL_gsi_permission: >>>>> + { >>>>> + int irq; >>>>> + unsigned int gsi = domctl->u.gsi_permission.gsi; >>>>> + uint32_t flags = domctl->u.gsi_permission.flags; >>>>> + >>>>> + /* Check only valid bits are set */ >>>>> + ret = -EINVAL; >>>>> + if ( flags & ~XEN_DOMCTL_GSI_ACTION_MASK ) >>>>> + break; >>>>> + >>>>> + ret = irq = gsi_2_irq(gsi); >>>> >>>> I was recently informed that a = b = c; form is not MISRA compliant. >>>> Since you just overwrite ret after the check, why not drop the >>>> assignment to ret and mae the next check against irq instead. >>> >>> The Misra concern is valid, yet the suggestion doesn't look to be quite >>> matching what is needed. After all if we take ... >>> >>>>> + if ( ret <= 0 ) >>>>> + break; >>> >>> ... the "break" path, "ret" needs to be set. Plus there's the problem of >>> "ret" being zero when exiting the function indicates success, yet this >>> is an error path (requiring ret < 0). So overall perhaps >>> >>> irq = gsi_2_irq(gsi); >>> if ( irq <= 0 ) >>> { >>> ret = irq ?: -EACCES; >>> break; >>> } >>> >>> ? >> >> Yes, ret needs to be set. And since gsi_2_irq doesn't return 0(if irq is 0, gsi_2_irq returns -EINVAL). >> Maybe below is enough? >> irq = gsi_2_irq(gsi); >> if ( irq < 0 ) >> { >> ret = irq; >> break; >> } > > My proposal was to cover that elsewhere we exclude IRQ0, and hence > it would be good to be consistent here. Got it, I will use your proposal in next version. Thanks. > > Jan
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index 68b5b46d1a83..939b1de0ee59 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,34 @@ long arch_do_domctl( break; } + case XEN_DOMCTL_gsi_permission: + { + int irq; + unsigned int gsi = domctl->u.gsi_permission.gsi; + uint32_t flags = domctl->u.gsi_permission.flags; + + /* Check only valid bits are set */ + ret = -EINVAL; + if ( flags & ~XEN_DOMCTL_GSI_ACTION_MASK ) + break; + + ret = irq = gsi_2_irq(gsi); + if ( ret <= 0 ) + break; + + ret = -EPERM; + if ( !irq_access_permitted(currd, irq) || + xsm_irq_permission(XSM_HOOK, d, irq, flags) ) + break; + + if ( flags ) + ret = irq_permit_access(d, irq); + else + ret = irq_deny_access(d, irq); + + 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..62456806c7af 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(unsigned int gsi); +int gsi_2_irq(unsigned int gsi); #endif diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c index 772700584639..e40d2f7dbd75 100644 --- a/xen/arch/x86/io_apic.c +++ b/xen/arch/x86/io_apic.c @@ -955,6 +955,25 @@ static int pin_2_irq(int idx, int apic, int pin) return irq; } +int gsi_2_irq(unsigned int gsi) +{ + int ioapic, irq; + unsigned int pin; + + if ( gsi > highest_gsi() ) + return -ERANGE; + + 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); + + return irq ?: -EINVAL; +} + 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 306d8ed97a83..e13b83bbe9dd 100644 --- a/xen/arch/x86/mpparse.c +++ b/xen/arch/x86/mpparse.c @@ -842,8 +842,7 @@ static struct mp_ioapic_routing { } mp_ioapic_routing[MAX_IO_APICS]; -static int mp_find_ioapic ( - int gsi) +int mp_find_ioapic(unsigned int gsi) { unsigned int i; @@ -854,7 +853,7 @@ static int mp_find_ioapic ( return i; } - printk(KERN_ERR "ERROR: Unable to locate IOAPIC for GSI %d\n", gsi); + printk(KERN_ERR "ERROR: Unable to locate IOAPIC for GSI %u\n", gsi); return -1; } @@ -915,7 +914,7 @@ void __init mp_register_ioapic ( return; } -unsigned __init highest_gsi(void) +unsigned highest_gsi(void) { unsigned x, res = 0; for (x = 0; x < nr_ioapics; x++) diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 2a49fe46ce25..e1028fc524cf 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -464,6 +464,14 @@ struct xen_domctl_irq_permission { uint8_t pad[3]; }; +/* XEN_DOMCTL_gsi_permission */ +struct xen_domctl_gsi_permission { + uint32_t gsi; +#define XEN_DOMCTL_GSI_REVOKE 0 +#define XEN_DOMCTL_GSI_GRANT 1 +#define XEN_DOMCTL_GSI_ACTION_MASK 1 + uint32_t flags; +}; /* XEN_DOMCTL_iomem_permission */ struct xen_domctl_iomem_permission { @@ -1306,6 +1314,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 +1337,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 278ad38c2af3..dfa23738cd8a 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -695,6 +695,7 @@ static int cf_check flask_domctl(struct domain *d, unsigned 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 /*