| Message ID | 20240708114124.407797-5-Jiqian.Chen@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support device passthrough when dom0 is PVH on Xen | expand |
On 08.07.2024 13:41, Jiqian Chen wrote: > Some type of domains 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 to set the access of irq, it is not suitable for > dom0 that doesn't have PIRQs. > > So, add a new hypercall XEN_DOMCTL_gsi_permission to grant/deny > the permission of irq(translate from x86 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> > --- > 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, access_flag) ) > + goto gsi_permission_out; > Is it okay to issue the XSM check using the translated value, > not the one that was originally passed into the hypercall? As long as the answer to this is going to be "Yes": Reviewed-by: Jan Beulich <jbeulich@suse.com> Daniel, awaiting your input. Jan
On Mon, 8 Jul 2024, Jiqian Chen wrote: > Some type of domains 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 to set the access of irq, it is not suitable for > dom0 that doesn't have PIRQs. > > So, add a new hypercall XEN_DOMCTL_gsi_permission to grant/deny > the permission of irq(translate from x86 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> > --- > 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, access_flag) ) > + goto gsi_permission_out; > Is it okay to issue the XSM check using the translated value, > not the one that was originally passed into the hypercall? > --- > xen/arch/x86/domctl.c | 32 ++++++++++++++++++++++++++++++ > xen/arch/x86/include/asm/io_apic.h | 2 ++ > xen/arch/x86/io_apic.c | 17 ++++++++++++++++ > xen/arch/x86/mpparse.c | 5 ++--- > xen/include/public/domctl.h | 9 +++++++++ > xen/xsm/flask/hooks.c | 1 + > 6 files changed, 63 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c > index 9190e11faaa3..4e9e4c4cfed3 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,37 @@ long arch_do_domctl( > break; > } > > + case XEN_DOMCTL_gsi_permission: > + { > + int irq; > + unsigned int gsi = domctl->u.gsi_permission.gsi; > + uint8_t access_flag = domctl->u.gsi_permission.access_flag; > + > + /* Check all bits and pads are zero except lowest bit */ > + ret = -EINVAL; > + if ( access_flag & ( ~XEN_DOMCTL_GSI_PERMISSION_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 > highest_gsi() || (irq = gsi_2_irq(gsi)) <= 0 ) gsi is unsigned int but it is passed to gsi_2_irq which takes an int as parameter. If gsi >= INT32_MAX we have a problem. I think we should explicitly check for the possible overflow and return error in that case. > + goto gsi_permission_out; > + > + ret = -EPERM; > + if ( !irq_access_permitted(currd, irq) || > + xsm_irq_permission(XSM_HOOK, d, irq, access_flag) ) > + goto gsi_permission_out; > + > + if ( access_flag ) > + 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 d2a313c4ac72..5968c8055671 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..7786a3337760 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; > > @@ -914,7 +913,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..877e35ab1376 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -464,6 +464,13 @@ 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_PERMISSION_MASK 1 > + uint8_t access_flag; /* flag to specify enable/disable of x86 gsi access */ > + uint8_t pad[3]; > +}; > > /* XEN_DOMCTL_iomem_permission */ > struct xen_domctl_iomem_permission { > @@ -1306,6 +1313,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 +1336,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 2024/7/23 06:10, Stefano Stabellini wrote: > On Mon, 8 Jul 2024, Jiqian Chen wrote: >> Some type of domains 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 to set the access of irq, it is not suitable for >> dom0 that doesn't have PIRQs. >> >> So, add a new hypercall XEN_DOMCTL_gsi_permission to grant/deny >> the permission of irq(translate from x86 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> >> --- >> 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, access_flag) ) >> + goto gsi_permission_out; >> Is it okay to issue the XSM check using the translated value, >> not the one that was originally passed into the hypercall? >> --- >> xen/arch/x86/domctl.c | 32 ++++++++++++++++++++++++++++++ >> xen/arch/x86/include/asm/io_apic.h | 2 ++ >> xen/arch/x86/io_apic.c | 17 ++++++++++++++++ >> xen/arch/x86/mpparse.c | 5 ++--- >> xen/include/public/domctl.h | 9 +++++++++ >> xen/xsm/flask/hooks.c | 1 + >> 6 files changed, 63 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c >> index 9190e11faaa3..4e9e4c4cfed3 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,37 @@ long arch_do_domctl( >> break; >> } >> >> + case XEN_DOMCTL_gsi_permission: >> + { >> + int irq; >> + unsigned int gsi = domctl->u.gsi_permission.gsi; >> + uint8_t access_flag = domctl->u.gsi_permission.access_flag; >> + >> + /* Check all bits and pads are zero except lowest bit */ >> + ret = -EINVAL; >> + if ( access_flag & ( ~XEN_DOMCTL_GSI_PERMISSION_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 > highest_gsi() || (irq = gsi_2_irq(gsi)) <= 0 ) > > gsi is unsigned int but it is passed to gsi_2_irq which takes an int as > parameter. If gsi >= INT32_MAX we have a problem. I think we should > explicitly check for the possible overflow and return error in that > case. But here has checked "gsi > highest_gsi()", can highesi_gsi() return a gsi >= INT32_MAX? > > >> + goto gsi_permission_out; >> + >> + ret = -EPERM; >> + if ( !irq_access_permitted(currd, irq) || >> + xsm_irq_permission(XSM_HOOK, d, irq, access_flag) ) >> + goto gsi_permission_out; >> + >> + if ( access_flag ) >> + 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 d2a313c4ac72..5968c8055671 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..7786a3337760 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; >> >> @@ -914,7 +913,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..877e35ab1376 100644 >> --- a/xen/include/public/domctl.h >> +++ b/xen/include/public/domctl.h >> @@ -464,6 +464,13 @@ 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_PERMISSION_MASK 1 >> + uint8_t access_flag; /* flag to specify enable/disable of x86 gsi access */ >> + uint8_t pad[3]; >> +}; >> >> /* XEN_DOMCTL_iomem_permission */ >> struct xen_domctl_iomem_permission { >> @@ -1306,6 +1313,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 +1336,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 >>
Hi Daniel, On 2024/7/9 21:08, Jan Beulich wrote: > On 08.07.2024 13:41, Jiqian Chen wrote: >> Some type of domains 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 to set the access of irq, it is not suitable for >> dom0 that doesn't have PIRQs. >> >> So, add a new hypercall XEN_DOMCTL_gsi_permission to grant/deny >> the permission of irq(translate from x86 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> >> --- >> 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, access_flag) ) >> + goto gsi_permission_out; >> Is it okay to issue the XSM check using the translated value, >> not the one that was originally passed into the hypercall? Need your input. > > As long as the answer to this is going to be "Yes": > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Daniel, awaiting your input. > > Jan
On Fri, 26 Jul 2024, Chen, Jiqian wrote: > On 2024/7/23 06:10, Stefano Stabellini wrote: > > On Mon, 8 Jul 2024, Jiqian Chen wrote: > >> Some type of domains 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 to set the access of irq, it is not suitable for > >> dom0 that doesn't have PIRQs. > >> > >> So, add a new hypercall XEN_DOMCTL_gsi_permission to grant/deny > >> the permission of irq(translate from x86 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> > >> --- > >> 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, access_flag) ) > >> + goto gsi_permission_out; > >> Is it okay to issue the XSM check using the translated value, > >> not the one that was originally passed into the hypercall? > >> --- > >> xen/arch/x86/domctl.c | 32 ++++++++++++++++++++++++++++++ > >> xen/arch/x86/include/asm/io_apic.h | 2 ++ > >> xen/arch/x86/io_apic.c | 17 ++++++++++++++++ > >> xen/arch/x86/mpparse.c | 5 ++--- > >> xen/include/public/domctl.h | 9 +++++++++ > >> xen/xsm/flask/hooks.c | 1 + > >> 6 files changed, 63 insertions(+), 3 deletions(-) > >> > >> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c > >> index 9190e11faaa3..4e9e4c4cfed3 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,37 @@ long arch_do_domctl( > >> break; > >> } > >> > >> + case XEN_DOMCTL_gsi_permission: > >> + { > >> + int irq; > >> + unsigned int gsi = domctl->u.gsi_permission.gsi; > >> + uint8_t access_flag = domctl->u.gsi_permission.access_flag; > >> + > >> + /* Check all bits and pads are zero except lowest bit */ > >> + ret = -EINVAL; > >> + if ( access_flag & ( ~XEN_DOMCTL_GSI_PERMISSION_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 > highest_gsi() || (irq = gsi_2_irq(gsi)) <= 0 ) > > > > gsi is unsigned int but it is passed to gsi_2_irq which takes an int as > > parameter. If gsi >= INT32_MAX we have a problem. I think we should > > explicitly check for the possible overflow and return error in that > > case. > But here has checked "gsi > highest_gsi()", can highesi_gsi() return a gsi >= INT32_MAX? In practice it is impossible but in theory it could. But then I looked at the implementation of highest_gsi() and gsi_end actually a signed int. So I think this is OK: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
On Mon, Jul 08, 2024 at 07:41:21PM +0800, Jiqian Chen wrote: > Some type of domains 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 to set the access of irq, it is not suitable for > dom0 that doesn't have PIRQs. > > So, add a new hypercall XEN_DOMCTL_gsi_permission to grant/deny > the permission of irq(translate from x86 gsi) to dumU when dom0 ^ missing space, and s/translate/translated/ > 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> > --- > 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, access_flag) ) > + goto gsi_permission_out; > Is it okay to issue the XSM check using the translated value, > not the one that was originally passed into the hypercall? FWIW, I don't see the GSI -> IRQ translation much different from the pIRQ -> IRQ translation done by pirq_access_permitted(), which is also ahead of the xsm check. > --- > xen/arch/x86/domctl.c | 32 ++++++++++++++++++++++++++++++ > xen/arch/x86/include/asm/io_apic.h | 2 ++ > xen/arch/x86/io_apic.c | 17 ++++++++++++++++ > xen/arch/x86/mpparse.c | 5 ++--- > xen/include/public/domctl.h | 9 +++++++++ > xen/xsm/flask/hooks.c | 1 + > 6 files changed, 63 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c > index 9190e11faaa3..4e9e4c4cfed3 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,37 @@ long arch_do_domctl( > break; > } > > + case XEN_DOMCTL_gsi_permission: > + { > + int irq; > + unsigned int gsi = domctl->u.gsi_permission.gsi; > + uint8_t access_flag = domctl->u.gsi_permission.access_flag; > + > + /* Check all bits and pads are zero except lowest bit */ > + ret = -EINVAL; > + if ( access_flag & ( ~XEN_DOMCTL_GSI_PERMISSION_MASK ) ) ^ unneeded parentheses and spaces. > + 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 > highest_gsi() || (irq = gsi_2_irq(gsi)) <= 0 ) FWIW, I would place the gsi > highest_gsi() check inside gsi_2_irq(). There's no reason to open-code it here, and it could help other users of gsi_2_irq(). The error code could also be ERANGE here instead of EINVAL IMO. > + goto gsi_permission_out; > + > + ret = -EPERM; > + if ( !irq_access_permitted(currd, irq) || > + xsm_irq_permission(XSM_HOOK, d, irq, access_flag) ) > + goto gsi_permission_out; > + > + if ( access_flag ) > + ret = irq_permit_access(d, irq); > + else > + ret = irq_deny_access(d, irq); > + > + gsi_permission_out: > + break; Why do you need a label when it just contains a break? Instead of the goto gsi_permission_out just use break directly. > + } > + > 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 d2a313c4ac72..5968c8055671 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) unsigned int for gsi. > +{ > + int ioapic, pin, irq; pin would better be unsigned int also. > + > + 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..7786a3337760 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) If you are changing this, you might as well make the gsi parameter unsigned int. > { > unsigned int i; > > @@ -914,7 +913,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..877e35ab1376 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -464,6 +464,13 @@ 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_PERMISSION_MASK 1 IMO this would be better named GRANT or similar, maybe something like: /* Low bit used to signal grant/revoke action. */ #define XEN_DOMCTL_GSI_REVOKE 0 #define XEN_DOMCTL_GSI_GRANT 1 > + uint8_t access_flag; /* flag to specify enable/disable of x86 gsi access */ > + uint8_t pad[3]; We might as well declare the flags field as uint32_t and avoid the padding field. Thanks, Roger.
On 01.08.2024 13:06, Roger Pau Monné wrote: > On Mon, Jul 08, 2024 at 07:41:21PM +0800, Jiqian Chen wrote: >> Remaining comment @Daniel P . Smith: >> + ret = -EPERM; >> + if ( !irq_access_permitted(currd, irq) || >> + xsm_irq_permission(XSM_HOOK, d, irq, access_flag) ) >> + goto gsi_permission_out; >> Is it okay to issue the XSM check using the translated value, >> not the one that was originally passed into the hypercall? > > FWIW, I don't see the GSI -> IRQ translation much different from the > pIRQ -> IRQ translation done by pirq_access_permitted(), which is also > ahead of the xsm check. The question (which I raised originally) isn't an ordering one, but an auditing one: Is it okay to pass the XSM hook a value that isn't what was passed into the hypercall? And Daniel, please, can you finally take a moment to help here, in your role as XSM maintainer? Elsewhere you complained you weren't Cc-ed or asked; now that you were asked, you haven't responded for weeks if not months. Jan
On Thu, Aug 01, 2024 at 01:36:16PM +0200, Jan Beulich wrote: > On 01.08.2024 13:06, Roger Pau Monné wrote: > > On Mon, Jul 08, 2024 at 07:41:21PM +0800, Jiqian Chen wrote: > >> Remaining comment @Daniel P . Smith: > >> + ret = -EPERM; > >> + if ( !irq_access_permitted(currd, irq) || > >> + xsm_irq_permission(XSM_HOOK, d, irq, access_flag) ) > >> + goto gsi_permission_out; > >> Is it okay to issue the XSM check using the translated value, > >> not the one that was originally passed into the hypercall? > > > > FWIW, I don't see the GSI -> IRQ translation much different from the > > pIRQ -> IRQ translation done by pirq_access_permitted(), which is also > > ahead of the xsm check. > > The question (which I raised originally) isn't an ordering one, but an > auditing one: Is it okay to pass the XSM hook a value that isn't what > was passed into the hypercall? But that's also the case with the current XEN_DOMCTL_irq_permission implementation? As the hypercall parameter is a pIRQ, and the XSM check is done against the translated IRQ obtained from the pIRQ parameter. Not saying you question is not relevant, but we already have at least one very similar instance of doing the XSM check against a value derived from an hypercall parameter. Thanks, Roger.
On 01.08.2024 14:41, Roger Pau Monné wrote: > On Thu, Aug 01, 2024 at 01:36:16PM +0200, Jan Beulich wrote: >> On 01.08.2024 13:06, Roger Pau Monné wrote: >>> On Mon, Jul 08, 2024 at 07:41:21PM +0800, Jiqian Chen wrote: >>>> Remaining comment @Daniel P . Smith: >>>> + ret = -EPERM; >>>> + if ( !irq_access_permitted(currd, irq) || >>>> + xsm_irq_permission(XSM_HOOK, d, irq, access_flag) ) >>>> + goto gsi_permission_out; >>>> Is it okay to issue the XSM check using the translated value, >>>> not the one that was originally passed into the hypercall? >>> >>> FWIW, I don't see the GSI -> IRQ translation much different from the >>> pIRQ -> IRQ translation done by pirq_access_permitted(), which is also >>> ahead of the xsm check. >> >> The question (which I raised originally) isn't an ordering one, but an >> auditing one: Is it okay to pass the XSM hook a value that isn't what >> was passed into the hypercall? > > But that's also the case with the current XEN_DOMCTL_irq_permission > implementation? As the hypercall parameter is a pIRQ, and the XSM > check is done against the translated IRQ obtained from the pIRQ > parameter. In a way you're right, but in a way there's also a meaningful difference: There we translate between internal numbering spaces. Here we first translate a quantity in a numbering space superimposed onto us to an internal representation. Flask, otoh, in such a situation may prefer to see the external representation of the resource. Jan
On 2024/8/1 19:06, Roger Pau Monné wrote: > On Mon, Jul 08, 2024 at 07:41:21PM +0800, Jiqian Chen wrote: >> Some type of domains 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 to set the access of irq, it is not suitable for >> dom0 that doesn't have PIRQs. >> >> So, add a new hypercall XEN_DOMCTL_gsi_permission to grant/deny >> the permission of irq(translate from x86 gsi) to dumU when dom0 > ^ missing space, and s/translate/translated/ > >> 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> >> --- >> 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, access_flag) ) >> + goto gsi_permission_out; >> Is it okay to issue the XSM check using the translated value, >> not the one that was originally passed into the hypercall? > > FWIW, I don't see the GSI -> IRQ translation much different from the > pIRQ -> IRQ translation done by pirq_access_permitted(), which is also > ahead of the xsm check. > >> --- >> xen/arch/x86/domctl.c | 32 ++++++++++++++++++++++++++++++ >> xen/arch/x86/include/asm/io_apic.h | 2 ++ >> xen/arch/x86/io_apic.c | 17 ++++++++++++++++ >> xen/arch/x86/mpparse.c | 5 ++--- >> xen/include/public/domctl.h | 9 +++++++++ >> xen/xsm/flask/hooks.c | 1 + >> 6 files changed, 63 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c >> index 9190e11faaa3..4e9e4c4cfed3 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,37 @@ long arch_do_domctl( >> break; >> } >> >> + case XEN_DOMCTL_gsi_permission: >> + { >> + int irq; >> + unsigned int gsi = domctl->u.gsi_permission.gsi; >> + uint8_t access_flag = domctl->u.gsi_permission.access_flag; >> + >> + /* Check all bits and pads are zero except lowest bit */ >> + ret = -EINVAL; >> + if ( access_flag & ( ~XEN_DOMCTL_GSI_PERMISSION_MASK ) ) > ^ unneeded parentheses and spaces. >> + 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 > highest_gsi() || (irq = gsi_2_irq(gsi)) <= 0 ) > > FWIW, I would place the gsi > highest_gsi() check inside gsi_2_irq(). > There's no reason to open-code it here, and it could help other > users of gsi_2_irq(). The error code could also be ERANGE here > instead of EINVAL IMO. > >> + goto gsi_permission_out; >> + >> + ret = -EPERM; >> + if ( !irq_access_permitted(currd, irq) || >> + xsm_irq_permission(XSM_HOOK, d, irq, access_flag) ) >> + goto gsi_permission_out; >> + >> + if ( access_flag ) >> + ret = irq_permit_access(d, irq); >> + else >> + ret = irq_deny_access(d, irq); >> + >> + gsi_permission_out: >> + break; > > Why do you need a label when it just contains a break? Instead of the > goto gsi_permission_out just use break directly. > >> + } >> + >> 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 d2a313c4ac72..5968c8055671 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) > > unsigned int for gsi. > >> +{ >> + int ioapic, pin, irq; > > pin would better be unsigned int also. > >> + >> + 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..7786a3337760 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) > > If you are changing this, you might as well make the gsi parameter > unsigned int. Thanks, I will change codes according above comments in next version. > >> { >> unsigned int i; >> >> @@ -914,7 +913,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..877e35ab1376 100644 >> --- a/xen/include/public/domctl.h >> +++ b/xen/include/public/domctl.h >> @@ -464,6 +464,13 @@ 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_PERMISSION_MASK 1 > > IMO this would be better named GRANT or similar, maybe something like: > > /* Low bit used to signal grant/revoke action. */ > #define XEN_DOMCTL_GSI_REVOKE 0 > #define XEN_DOMCTL_GSI_GRANT 1 > >> + uint8_t access_flag; /* flag to specify enable/disable of x86 gsi access */ >> + uint8_t pad[3]; > > We might as well declare the flags field as uint32_t and avoid the > padding field. So, should this struct be like below? Then I just need to check whether everything except the lowest bit is 0. struct xen_domctl_gsi_permission { uint32_t gsi; /* Lowest bit used to signal grant/revoke action. */ #define XEN_DOMCTL_GSI_REVOKE 0 #define XEN_DOMCTL_GSI_GRANT 1 #define XEN_DOMCTL_GSI_PERMISSION_MASK 1 uint32_t access_flag; /* flag to specify enable/disable of x86 gsi access */ }; > > Thanks, Roger.
On 02.08.2024 05:10, Chen, Jiqian wrote: > On 2024/8/1 19:06, Roger Pau Monné wrote: >> On Mon, Jul 08, 2024 at 07:41:21PM +0800, Jiqian Chen wrote: >>> --- a/xen/include/public/domctl.h >>> +++ b/xen/include/public/domctl.h >>> @@ -464,6 +464,13 @@ 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_PERMISSION_MASK 1 >> >> IMO this would be better named GRANT or similar, maybe something like: >> >> /* Low bit used to signal grant/revoke action. */ >> #define XEN_DOMCTL_GSI_REVOKE 0 >> #define XEN_DOMCTL_GSI_GRANT 1 >> >>> + uint8_t access_flag; /* flag to specify enable/disable of x86 gsi access */ >>> + uint8_t pad[3]; >> >> We might as well declare the flags field as uint32_t and avoid the >> padding field. > So, should this struct be like below? Then I just need to check whether everything except the lowest bit is 0. > struct xen_domctl_gsi_permission { > uint32_t gsi; > /* Lowest bit used to signal grant/revoke action. */ > #define XEN_DOMCTL_GSI_REVOKE 0 > #define XEN_DOMCTL_GSI_GRANT 1 > #define XEN_DOMCTL_GSI_PERMISSION_MASK 1 > uint32_t access_flag; /* flag to specify enable/disable of x86 gsi access */ > }; Yet then why "access_flags"? You can't foresee what meaning the other bits may gain. That meaning may (and likely will) not be access related at all. Jan
On 2024/8/2 14:27, Jan Beulich wrote: > On 02.08.2024 05:10, Chen, Jiqian wrote: >> On 2024/8/1 19:06, Roger Pau Monné wrote: >>> On Mon, Jul 08, 2024 at 07:41:21PM +0800, Jiqian Chen wrote: >>>> --- a/xen/include/public/domctl.h >>>> +++ b/xen/include/public/domctl.h >>>> @@ -464,6 +464,13 @@ 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_PERMISSION_MASK 1 >>> >>> IMO this would be better named GRANT or similar, maybe something like: >>> >>> /* Low bit used to signal grant/revoke action. */ >>> #define XEN_DOMCTL_GSI_REVOKE 0 >>> #define XEN_DOMCTL_GSI_GRANT 1 >>> >>>> + uint8_t access_flag; /* flag to specify enable/disable of x86 gsi access */ >>>> + uint8_t pad[3]; >>> >>> We might as well declare the flags field as uint32_t and avoid the >>> padding field. >> So, should this struct be like below? Then I just need to check whether everything except the lowest bit is 0. >> struct xen_domctl_gsi_permission { >> uint32_t gsi; >> /* Lowest bit used to signal grant/revoke action. */ >> #define XEN_DOMCTL_GSI_REVOKE 0 >> #define XEN_DOMCTL_GSI_GRANT 1 >> #define XEN_DOMCTL_GSI_PERMISSION_MASK 1 >> uint32_t access_flag; /* flag to specify enable/disable of x86 gsi access */ >> }; > > Yet then why "access_flags"? You can't foresee what meaning the other bits may > gain. That meaning may (and likely will) not be access related at all. OK, just "uint32_t flags". > > Jan
On Fri, Aug 02, 2024 at 03:10:27AM +0000, Chen, Jiqian wrote: > On 2024/8/1 19:06, Roger Pau Monné wrote: > > We might as well declare the flags field as uint32_t and avoid the > > padding field. > So, should this struct be like below? Then I just need to check whether everything except the lowest bit is 0. > struct xen_domctl_gsi_permission { > uint32_t gsi; > /* Lowest bit used to signal grant/revoke action. */ > #define XEN_DOMCTL_GSI_REVOKE 0 > #define XEN_DOMCTL_GSI_GRANT 1 > #define XEN_DOMCTL_GSI_PERMISSION_MASK 1 Maybe ACTION_MASK rather than PERMISSION_MASK? > uint32_t access_flag; /* flag to specify enable/disable of x86 gsi access */ I would again be fine naming this just flags and the comment is likely to go stale quite soon if we add more flags. However given the simplicity of this hypercall I'm unsure whether any new flags could appear. Thanks, Roger.
On Mon, Jul 08, 2024 at 07:41:21PM +0800, Jiqian Chen wrote: > Some type of domains 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 to set the access of irq, it is not suitable for > dom0 that doesn't have PIRQs. > > So, add a new hypercall XEN_DOMCTL_gsi_permission to grant/deny > the permission of irq(translate from x86 gsi) to dumU when dom0 > has no PIRQs. I've been wondering about this, and if the hypercall is strictly to resolve GSIs into IRQs, isn't that the case that Xen identity maps GSI into the IRQ space, and hence no translation is required? Thanks, Roger.
On 2024/8/2 16:08, Roger Pau Monné wrote: > On Mon, Jul 08, 2024 at 07:41:21PM +0800, Jiqian Chen wrote: >> Some type of domains 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 to set the access of irq, it is not suitable for >> dom0 that doesn't have PIRQs. >> >> So, add a new hypercall XEN_DOMCTL_gsi_permission to grant/deny >> the permission of irq(translate from x86 gsi) to dumU when dom0 >> has no PIRQs. > > I've been wondering about this, and if the hypercall is strictly to > resolve GSIs into IRQs, isn't that the case that Xen identity maps GSI > into the IRQ space, and hence no translation is required? Yes, for gsis that has no entries in mp_irqs, xen do the identity maps. I will delete the words "translate .." > > Thanks, Roger.
On 02.08.2024 10:08, Roger Pau Monné wrote: > On Mon, Jul 08, 2024 at 07:41:21PM +0800, Jiqian Chen wrote: >> Some type of domains 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 to set the access of irq, it is not suitable for >> dom0 that doesn't have PIRQs. >> >> So, add a new hypercall XEN_DOMCTL_gsi_permission to grant/deny >> the permission of irq(translate from x86 gsi) to dumU when dom0 >> has no PIRQs. > > I've been wondering about this, and if the hypercall is strictly to > resolve GSIs into IRQs, isn't that the case that Xen identity maps GSI > into the IRQ space, and hence no translation is required? It was a long-winded discussion to clarify that in obscure cases translation is required: Whenever there's a source override in ACPI. Jan
On Fri, Aug 02, 2024 at 11:40:53AM +0200, Jan Beulich wrote: > On 02.08.2024 10:08, Roger Pau Monné wrote: > > On Mon, Jul 08, 2024 at 07:41:21PM +0800, Jiqian Chen wrote: > >> Some type of domains 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 to set the access of irq, it is not suitable for > >> dom0 that doesn't have PIRQs. > >> > >> So, add a new hypercall XEN_DOMCTL_gsi_permission to grant/deny > >> the permission of irq(translate from x86 gsi) to dumU when dom0 > >> has no PIRQs. > > > > I've been wondering about this, and if the hypercall is strictly to > > resolve GSIs into IRQs, isn't that the case that Xen identity maps GSI > > into the IRQ space, and hence no translation is required? > > It was a long-winded discussion to clarify that in obscure cases > translation is required: Whenever there's a source override in ACPI. Right, I see it's a bit convoluted to get the overrides, as those are indexed by IO-APIC pin, so we need to resolve the GSI -> (ioapic, pin) first and then check for any possible overrides. Might be helpful to mention in the commit description that the GSI to IRQ translation is done to account for ACPI overrides, as otherwise GSIs are identity mapped into IRQs. Thanks, Roger.
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index 9190e11faaa3..4e9e4c4cfed3 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,37 @@ long arch_do_domctl( break; } + case XEN_DOMCTL_gsi_permission: + { + int irq; + unsigned int gsi = domctl->u.gsi_permission.gsi; + uint8_t access_flag = domctl->u.gsi_permission.access_flag; + + /* Check all bits and pads are zero except lowest bit */ + ret = -EINVAL; + if ( access_flag & ( ~XEN_DOMCTL_GSI_PERMISSION_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 > highest_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, access_flag) ) + goto gsi_permission_out; + + if ( access_flag ) + 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 d2a313c4ac72..5968c8055671 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..7786a3337760 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; @@ -914,7 +913,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..877e35ab1376 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -464,6 +464,13 @@ 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_PERMISSION_MASK 1 + uint8_t access_flag; /* flag to specify enable/disable of x86 gsi access */ + uint8_t pad[3]; +}; /* XEN_DOMCTL_iomem_permission */ struct xen_domctl_iomem_permission { @@ -1306,6 +1313,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 +1336,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 /*