| Message ID | 20240617090035.839640-6-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/6/17 17:00, Jiqian Chen wrote: > 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> > --- > RFC: it needs review and needs to wait for the corresponding third patch on linux kernel side to be merged. > --- > tools/include/xenctrl.h | 5 +++ > tools/libs/ctrl/xc_domain.c | 15 +++++++ > tools/libs/light/libxl_pci.c | 67 +++++++++++++++++++++++++++--- > xen/arch/x86/domctl.c | 43 +++++++++++++++++++ > 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 + > 9 files changed, 153 insertions(+), 8 deletions(-) > > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h > index a0381f74d24b..f3feb6848e25 100644 > --- a/tools/include/xenctrl.h > +++ b/tools/include/xenctrl.h > @@ -1382,6 +1382,11 @@ int xc_domain_irq_permission(xc_interface *xch, > uint32_t pirq, > bool allow_access); > > +int xc_domain_gsi_permission(xc_interface *xch, > + uint32_t domid, > + uint32_t gsi, > + bool allow_access); > + > int xc_domain_iomem_permission(xc_interface *xch, > uint32_t domid, > unsigned long first_mfn, > diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c > index f2d9d14b4d9f..8540e84fda93 100644 > --- a/tools/libs/ctrl/xc_domain.c > +++ b/tools/libs/ctrl/xc_domain.c > @@ -1394,6 +1394,21 @@ int xc_domain_irq_permission(xc_interface *xch, > return do_domctl(xch, &domctl); > } > > +int xc_domain_gsi_permission(xc_interface *xch, > + uint32_t domid, > + uint32_t gsi, > + bool allow_access) > +{ > + struct xen_domctl domctl = { > + .cmd = XEN_DOMCTL_gsi_permission, > + .domain = domid, > + .u.gsi_permission.gsi = gsi, > + .u.gsi_permission.allow_access = allow_access, > + }; > + > + return do_domctl(xch, &domctl); > +} > + > int xc_domain_iomem_permission(xc_interface *xch, > uint32_t domid, > unsigned long first_mfn, > diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c > index 376f91759ac6..f027f22c0028 100644 > --- a/tools/libs/light/libxl_pci.c > +++ b/tools/libs/light/libxl_pci.c > @@ -1431,6 +1431,9 @@ static void pci_add_dm_done(libxl__egc *egc, > uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED; > uint32_t domainid = domid; > bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid); > +#ifdef CONFIG_X86 > + xc_domaininfo_t info; > +#endif > > /* Convenience aliases */ > bool starting = pas->starting; > @@ -1516,14 +1519,39 @@ static void pci_add_dm_done(libxl__egc *egc, > rc = ERROR_FAIL; > goto out; > } > - r = xc_domain_irq_permission(ctx->xch, domid, irq, 1); > +#ifdef CONFIG_X86 > + /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission */ > + r = xc_domain_getinfo_single(ctx->xch, 0, &info); > if (r < 0) { > - LOGED(ERROR, domainid, > - "xc_domain_irq_permission irq=%d (error=%d)", irq, r); > + LOGED(ERROR, domainid, "getdomaininfo failed (error=%d)", errno); > fclose(f); > rc = ERROR_FAIL; > goto out; > } > + if (info.flags & XEN_DOMINF_hvm_guest && > + !(info.arch_config.emulation_flags & XEN_X86_EMU_USE_PIRQ) && > + gsi > 0) { > + r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1); > + if (r < 0) { > + LOGED(ERROR, domainid, > + "xc_domain_gsi_permission gsi=%d (error=%d)", gsi, errno); > + fclose(f); > + rc = ERROR_FAIL; > + goto out; > + } > + } > + else > +#endif > + { > + r = xc_domain_irq_permission(ctx->xch, domid, irq, 1); > + if (r < 0) { > + LOGED(ERROR, domainid, > + "xc_domain_irq_permission irq=%d (error=%d)", irq, errno); > + fclose(f); > + rc = ERROR_FAIL; > + goto out; > + } > + } > } > fclose(f); > > @@ -2200,6 +2228,10 @@ static void pci_remove_detached(libxl__egc *egc, > #endif > uint32_t domainid = prs->domid; > bool isstubdom; > +#ifdef CONFIG_X86 > + int r; > + xc_domaininfo_t info; > +#endif > > /* Convenience aliases */ > libxl_device_pci *const pci = &prs->pci; > @@ -2287,9 +2319,32 @@ skip_bar: > */ > LOGED(ERROR, domid, "xc_physdev_unmap_pirq irq=%d", irq); > } > - rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0); > - if (rc < 0) { > - LOGED(ERROR, domid, "xc_domain_irq_permission irq=%d", irq); > +#ifdef CONFIG_X86 > + /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission */ > + r = xc_domain_getinfo_single(ctx->xch, 0, &info); > + if (r < 0) { > + LOGED(ERROR, domid, "getdomaininfo failed (error=%d)", errno); > + fclose(f); > + rc = ERROR_FAIL; > + goto skip_legacy_irq; > + } > + if (info.flags & XEN_DOMINF_hvm_guest && > + !(info.arch_config.emulation_flags & XEN_X86_EMU_USE_PIRQ) && > + gsi > 0) { > + r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 0); > + if (r < 0) { > + LOGED(ERROR, domid, > + "xc_domain_gsi_permission gsi=%d (error=%d)", gsi, errno); > + rc = ERROR_FAIL; > + } > + } > + else > +#endif > + { > + rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0); > + if (rc < 0) { > + LOGED(ERROR, domid, "xc_domain_irq_permission irq=%d", irq); > + } > } > } > > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c > index 335aedf46d03..6b465bbc6ec0 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,48 @@ long arch_do_domctl( > break; > } > > + case XEN_DOMCTL_gsi_permission: > + { > + unsigned int gsi = domctl->u.gsi_permission.gsi; > + int irq; > + bool allow = domctl->u.gsi_permission.allow_access; > + > + /* Check all pads are zero */ > + ret = -EINVAL; > + for ( i = 0; > + i < sizeof(domctl->u.gsi_permission.pad) / > + sizeof(domctl->u.gsi_permission.pad[0]); > + ++i ) > + if ( domctl->u.gsi_permission.pad[i] ) > + goto out; > + > + /* > + * If current domain is PV or it has PIRQ flag, it has a mapping > + * of gsi, pirq and irq, so it should use XEN_DOMCTL_irq_permission > + * to grant irq permission. > + */ > + ret = -EOPNOTSUPP; > + if ( is_pv_domain(currd) || has_pirq(currd) ) > + goto out; > + > + ret = -EINVAL; > + if ( gsi >= nr_irqs_gsi || (irq = gsi_2_irq(gsi)) < 0 ) > + goto out; > + > + ret = -EPERM; > + if ( !irq_access_permitted(currd, irq) || > + xsm_irq_permission(XSM_HOOK, d, irq, allow) ) Copy the question of Jan from the previous version to here: Is it okay to issue the XSM check using the translated value, not the one that was originally passed into the hypercall? > + goto out; > + > + if ( allow ) > + ret = irq_permit_access(d, irq); > + else > + ret = irq_deny_access(d, irq); > + > + 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 b48a64246548..23845c8cb11f 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 > /*
On 17.06.2024 11:00, Jiqian Chen wrote: > 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> > --- > RFC: it needs review and needs to wait for the corresponding third patch on linux kernel side to be merged. > --- > tools/include/xenctrl.h | 5 +++ > tools/libs/ctrl/xc_domain.c | 15 +++++++ > tools/libs/light/libxl_pci.c | 67 +++++++++++++++++++++++++++--- > xen/arch/x86/domctl.c | 43 +++++++++++++++++++ > 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 + > 9 files changed, 153 insertions(+), 8 deletions(-) > > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h > index a0381f74d24b..f3feb6848e25 100644 > --- a/tools/include/xenctrl.h > +++ b/tools/include/xenctrl.h > @@ -1382,6 +1382,11 @@ int xc_domain_irq_permission(xc_interface *xch, > uint32_t pirq, > bool allow_access); > > +int xc_domain_gsi_permission(xc_interface *xch, > + uint32_t domid, > + uint32_t gsi, > + bool allow_access); > + > int xc_domain_iomem_permission(xc_interface *xch, > uint32_t domid, > unsigned long first_mfn, > diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c > index f2d9d14b4d9f..8540e84fda93 100644 > --- a/tools/libs/ctrl/xc_domain.c > +++ b/tools/libs/ctrl/xc_domain.c > @@ -1394,6 +1394,21 @@ int xc_domain_irq_permission(xc_interface *xch, > return do_domctl(xch, &domctl); > } > > +int xc_domain_gsi_permission(xc_interface *xch, > + uint32_t domid, > + uint32_t gsi, > + bool allow_access) > +{ > + struct xen_domctl domctl = { > + .cmd = XEN_DOMCTL_gsi_permission, > + .domain = domid, > + .u.gsi_permission.gsi = gsi, > + .u.gsi_permission.allow_access = allow_access, > + }; > + > + return do_domctl(xch, &domctl); > +} > + > int xc_domain_iomem_permission(xc_interface *xch, > uint32_t domid, > unsigned long first_mfn, > diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c > index 376f91759ac6..f027f22c0028 100644 > --- a/tools/libs/light/libxl_pci.c > +++ b/tools/libs/light/libxl_pci.c > @@ -1431,6 +1431,9 @@ static void pci_add_dm_done(libxl__egc *egc, > uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED; > uint32_t domainid = domid; > bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid); > +#ifdef CONFIG_X86 > + xc_domaininfo_t info; > +#endif > > /* Convenience aliases */ > bool starting = pas->starting; > @@ -1516,14 +1519,39 @@ static void pci_add_dm_done(libxl__egc *egc, > rc = ERROR_FAIL; > goto out; > } > - r = xc_domain_irq_permission(ctx->xch, domid, irq, 1); > +#ifdef CONFIG_X86 > + /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission */ > + r = xc_domain_getinfo_single(ctx->xch, 0, &info); Hard-coded 0 is imposing limitations. Ideally you would use DOMID_SELF, but I didn't check if that can be used with the underlying hypercall(s). Otherwise you want to pass the actual domid of the local domain here. > if (r < 0) { > - LOGED(ERROR, domainid, > - "xc_domain_irq_permission irq=%d (error=%d)", irq, r); > + LOGED(ERROR, domainid, "getdomaininfo failed (error=%d)", errno); > fclose(f); > rc = ERROR_FAIL; > goto out; > } > + if (info.flags & XEN_DOMINF_hvm_guest && You want to parenthesize the & here. > + !(info.arch_config.emulation_flags & XEN_X86_EMU_USE_PIRQ) && > + gsi > 0) { So if gsi < 0 failure of xc_domain_getinfo_single() would needlessly result in failure of this function? > --- 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,48 @@ long arch_do_domctl( > break; > } > > + case XEN_DOMCTL_gsi_permission: > + { > + unsigned int gsi = domctl->u.gsi_permission.gsi; > + int irq; > + bool allow = domctl->u.gsi_permission.allow_access; See my earlier comments on this conversion of 8 bits into just one. > + /* Check all pads are zero */ > + ret = -EINVAL; > + for ( i = 0; > + i < sizeof(domctl->u.gsi_permission.pad) / > + sizeof(domctl->u.gsi_permission.pad[0]); Please don't open-code ARRAY_SIZE(). > + ++i ) > + if ( domctl->u.gsi_permission.pad[i] ) > + goto out; > + > + /* > + * If current domain is PV or it has PIRQ flag, it has a mapping > + * of gsi, pirq and irq, so it should use XEN_DOMCTL_irq_permission > + * to grant irq permission. > + */ > + ret = -EOPNOTSUPP; > + if ( is_pv_domain(currd) || has_pirq(currd) ) > + goto out; I'm curious what other x86 maintainers think: I for one would not impose such an artificial restriction. > + ret = -EINVAL; > + if ( gsi >= nr_irqs_gsi || (irq = gsi_2_irq(gsi)) < 0 ) > + goto out; > + > + ret = -EPERM; > + if ( !irq_access_permitted(currd, irq) || > + xsm_irq_permission(XSM_HOOK, d, irq, allow) ) > + goto out; > + > + if ( allow ) > + ret = irq_permit_access(d, irq); > + else > + ret = irq_deny_access(d, irq); > + > + out: Please use a less generic name for such a label local to just one case block. However, with .. > + break; .. this being all that's done here: Why have a label in the first place? Jan
On 2024/6/17 23:32, Jan Beulich wrote: > On 17.06.2024 11:00, Jiqian Chen wrote: >> 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> >> --- >> RFC: it needs review and needs to wait for the corresponding third patch on linux kernel side to be merged. >> --- >> tools/include/xenctrl.h | 5 +++ >> tools/libs/ctrl/xc_domain.c | 15 +++++++ >> tools/libs/light/libxl_pci.c | 67 +++++++++++++++++++++++++++--- >> xen/arch/x86/domctl.c | 43 +++++++++++++++++++ >> 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 + >> 9 files changed, 153 insertions(+), 8 deletions(-) >> >> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h >> index a0381f74d24b..f3feb6848e25 100644 >> --- a/tools/include/xenctrl.h >> +++ b/tools/include/xenctrl.h >> @@ -1382,6 +1382,11 @@ int xc_domain_irq_permission(xc_interface *xch, >> uint32_t pirq, >> bool allow_access); >> >> +int xc_domain_gsi_permission(xc_interface *xch, >> + uint32_t domid, >> + uint32_t gsi, >> + bool allow_access); >> + >> int xc_domain_iomem_permission(xc_interface *xch, >> uint32_t domid, >> unsigned long first_mfn, >> diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c >> index f2d9d14b4d9f..8540e84fda93 100644 >> --- a/tools/libs/ctrl/xc_domain.c >> +++ b/tools/libs/ctrl/xc_domain.c >> @@ -1394,6 +1394,21 @@ int xc_domain_irq_permission(xc_interface *xch, >> return do_domctl(xch, &domctl); >> } >> >> +int xc_domain_gsi_permission(xc_interface *xch, >> + uint32_t domid, >> + uint32_t gsi, >> + bool allow_access) >> +{ >> + struct xen_domctl domctl = { >> + .cmd = XEN_DOMCTL_gsi_permission, >> + .domain = domid, >> + .u.gsi_permission.gsi = gsi, >> + .u.gsi_permission.allow_access = allow_access, >> + }; >> + >> + return do_domctl(xch, &domctl); >> +} >> + >> int xc_domain_iomem_permission(xc_interface *xch, >> uint32_t domid, >> unsigned long first_mfn, >> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c >> index 376f91759ac6..f027f22c0028 100644 >> --- a/tools/libs/light/libxl_pci.c >> +++ b/tools/libs/light/libxl_pci.c >> @@ -1431,6 +1431,9 @@ static void pci_add_dm_done(libxl__egc *egc, >> uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED; >> uint32_t domainid = domid; >> bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid); >> +#ifdef CONFIG_X86 >> + xc_domaininfo_t info; >> +#endif >> >> /* Convenience aliases */ >> bool starting = pas->starting; >> @@ -1516,14 +1519,39 @@ static void pci_add_dm_done(libxl__egc *egc, >> rc = ERROR_FAIL; >> goto out; >> } >> - r = xc_domain_irq_permission(ctx->xch, domid, irq, 1); >> +#ifdef CONFIG_X86 >> + /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission */ >> + r = xc_domain_getinfo_single(ctx->xch, 0, &info); > > Hard-coded 0 is imposing limitations. Ideally you would use DOMID_SELF, but > I didn't check if that can be used with the underlying hypercall(s). Otherwise > you want to pass the actual domid of the local domain here. But the action of granting permission is from dom0 to domU, what I need to get is the infomation of dom0, The actual domid here is domU's id I think, it is not useful. > >> if (r < 0) { >> - LOGED(ERROR, domainid, >> - "xc_domain_irq_permission irq=%d (error=%d)", irq, r); >> + LOGED(ERROR, domainid, "getdomaininfo failed (error=%d)", errno); >> fclose(f); >> rc = ERROR_FAIL; >> goto out; >> } >> + if (info.flags & XEN_DOMINF_hvm_guest && > > You want to parenthesize the & here. Will change in next version. > >> + !(info.arch_config.emulation_flags & XEN_X86_EMU_USE_PIRQ) && >> + gsi > 0) { > > So if gsi < 0 failure of xc_domain_getinfo_single() would needlessly result > in failure of this function? In next version, the judgment of gsi>0 will be placed in the next layer of if, and then the error will be handled. > >> --- 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,48 @@ long arch_do_domctl( >> break; >> } >> >> + case XEN_DOMCTL_gsi_permission: >> + { >> + unsigned int gsi = domctl->u.gsi_permission.gsi; >> + int irq; >> + bool allow = domctl->u.gsi_permission.allow_access; > > See my earlier comments on this conversion of 8 bits into just one. Do you mean that I need to check allow_access is >= 0? But allow_access is u8, it can't be negative. > >> + /* Check all pads are zero */ >> + ret = -EINVAL; >> + for ( i = 0; >> + i < sizeof(domctl->u.gsi_permission.pad) / >> + sizeof(domctl->u.gsi_permission.pad[0]); > > Please don't open-code ARRAY_SIZE(). Will change in next version. > >> + ++i ) >> + if ( domctl->u.gsi_permission.pad[i] ) >> + goto out; >> + >> + /* >> + * If current domain is PV or it has PIRQ flag, it has a mapping >> + * of gsi, pirq and irq, so it should use XEN_DOMCTL_irq_permission >> + * to grant irq permission. >> + */ >> + ret = -EOPNOTSUPP; >> + if ( is_pv_domain(currd) || has_pirq(currd) ) >> + goto out; > > I'm curious what other x86 maintainers think: I for one would not impose such > an artificial restriction. Emmm, so I need to remove this check. > >> + ret = -EINVAL; >> + if ( gsi >= nr_irqs_gsi || (irq = gsi_2_irq(gsi)) < 0 ) >> + goto out; >> + >> + ret = -EPERM; >> + if ( !irq_access_permitted(currd, irq) || >> + xsm_irq_permission(XSM_HOOK, d, irq, allow) ) >> + goto out; >> + >> + if ( allow ) >> + ret = irq_permit_access(d, irq); >> + else >> + ret = irq_deny_access(d, irq); >> + >> + out: > > Please use a less generic name for such a label local to just one case > block. However, with .. Ok, will change in next version. > >> + break; > > .. this being all that's done here: Why have a label in the first place? > > Jan
On 18.06.2024 10:23, Chen, Jiqian wrote: > On 2024/6/17 23:32, Jan Beulich wrote: >> On 17.06.2024 11:00, Jiqian Chen wrote: >>> @@ -1516,14 +1519,39 @@ static void pci_add_dm_done(libxl__egc *egc, >>> rc = ERROR_FAIL; >>> goto out; >>> } >>> - r = xc_domain_irq_permission(ctx->xch, domid, irq, 1); >>> +#ifdef CONFIG_X86 >>> + /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission */ >>> + r = xc_domain_getinfo_single(ctx->xch, 0, &info); >> >> Hard-coded 0 is imposing limitations. Ideally you would use DOMID_SELF, but >> I didn't check if that can be used with the underlying hypercall(s). Otherwise >> you want to pass the actual domid of the local domain here. > But the action of granting permission is from dom0 to domU, what I need to get is the infomation of dom0, > The actual domid here is domU's id I think, it is not useful. Note how I said DOMID_SELF and "local domain". There's no talk of using the DomU's domid. But what you apparently neglect is the fact that the hardware domain isn't necessarily Dom0 (see CONFIG_LATE_HWDOM in the hypervisor). While benign in most cases, this is relevant when it comes to referencing the hardware domain by domid. And it is the hardware domain which is going to drive the device re-assignment, as that domain is who's in possession of all the devices not yet assigned to any DomU. >>> @@ -237,6 +238,48 @@ long arch_do_domctl( >>> break; >>> } >>> >>> + case XEN_DOMCTL_gsi_permission: >>> + { >>> + unsigned int gsi = domctl->u.gsi_permission.gsi; >>> + int irq; >>> + bool allow = domctl->u.gsi_permission.allow_access; >> >> See my earlier comments on this conversion of 8 bits into just one. > Do you mean that I need to check allow_access is >= 0? > But allow_access is u8, it can't be negative. Right. What I can only re-iterate from earlier commenting is that you want to check for 0 or 1 (can be viewed as looking at just the low bit), rejecting everything else. It is only this way that down the road we could assign meaning to the other bits, without risking to break existing callers. That's the same as the requirement to check padding fields to be zero. Jan
On 2024/6/18 17:23, Jan Beulich wrote: > On 18.06.2024 10:23, Chen, Jiqian wrote: >> On 2024/6/17 23:32, Jan Beulich wrote: >>> On 17.06.2024 11:00, Jiqian Chen wrote: >>>> @@ -1516,14 +1519,39 @@ static void pci_add_dm_done(libxl__egc *egc, >>>> rc = ERROR_FAIL; >>>> goto out; >>>> } >>>> - r = xc_domain_irq_permission(ctx->xch, domid, irq, 1); >>>> +#ifdef CONFIG_X86 >>>> + /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission */ >>>> + r = xc_domain_getinfo_single(ctx->xch, 0, &info); >>> >>> Hard-coded 0 is imposing limitations. Ideally you would use DOMID_SELF, but >>> I didn't check if that can be used with the underlying hypercall(s). Otherwise From the commit 10ef7a91b5a8cb8c58903c60e2dd16ed490b3bcf, DOMID_SELF is not allowed for XEN_DOMCTL_getdomaininfo. And now XEN_DOMCTL_getdomaininfo gets domain through rcu_lock_domain_by_id. >>> you want to pass the actual domid of the local domain here. What is the local domain here? What is method for me to get its domid? >> But the action of granting permission is from dom0 to domU, what I need to get is the infomation of dom0, >> The actual domid here is domU's id I think, it is not useful. > > Note how I said DOMID_SELF and "local domain". There's no talk of using the > DomU's domid. But what you apparently neglect is the fact that the hardware > domain isn't necessarily Dom0 (see CONFIG_LATE_HWDOM in the hypervisor). > While benign in most cases, this is relevant when it comes to referencing > the hardware domain by domid. And it is the hardware domain which is going > to drive the device re-assignment, as that domain is who's in possession of > all the devices not yet assigned to any DomU. OK, I need to get the information of hardware domain here? > >>>> @@ -237,6 +238,48 @@ long arch_do_domctl( >>>> break; >>>> } >>>> >>>> + case XEN_DOMCTL_gsi_permission: >>>> + { >>>> + unsigned int gsi = domctl->u.gsi_permission.gsi; >>>> + int irq; >>>> + bool allow = domctl->u.gsi_permission.allow_access; >>> >>> See my earlier comments on this conversion of 8 bits into just one. >> Do you mean that I need to check allow_access is >= 0? >> But allow_access is u8, it can't be negative. > > Right. What I can only re-iterate from earlier commenting is that you > want to check for 0 or 1 (can be viewed as looking at just the low bit), > rejecting everything else. It is only this way that down the road we > could assign meaning to the other bits, without risking to break existing > callers. That's the same as the requirement to check padding fields to be > zero. OK, I will add check the other bit is zero except the lowest one bit. > > Jan
On 20.06.2024 11:40, Chen, Jiqian wrote: > On 2024/6/18 17:23, Jan Beulich wrote: >> On 18.06.2024 10:23, Chen, Jiqian wrote: >>> On 2024/6/17 23:32, Jan Beulich wrote: >>>> On 17.06.2024 11:00, Jiqian Chen wrote: >>>>> @@ -1516,14 +1519,39 @@ static void pci_add_dm_done(libxl__egc *egc, >>>>> rc = ERROR_FAIL; >>>>> goto out; >>>>> } >>>>> - r = xc_domain_irq_permission(ctx->xch, domid, irq, 1); >>>>> +#ifdef CONFIG_X86 >>>>> + /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission */ >>>>> + r = xc_domain_getinfo_single(ctx->xch, 0, &info); >>>> >>>> Hard-coded 0 is imposing limitations. Ideally you would use DOMID_SELF, but >>>> I didn't check if that can be used with the underlying hypercall(s). Otherwise > From the commit 10ef7a91b5a8cb8c58903c60e2dd16ed490b3bcf, DOMID_SELF is not allowed for XEN_DOMCTL_getdomaininfo. > And now XEN_DOMCTL_getdomaininfo gets domain through rcu_lock_domain_by_id. > >>>> you want to pass the actual domid of the local domain here. > What is the local domain here? The domain your code is running in. > What is method for me to get its domid? I hope there's an available function in one of the libraries to do that. But I wouldn't even know what to look for; that's a question to (primarily) Anthony then, who sadly continues to be our only tool stack maintainer. Alternatively we could maybe enable XEN_DOMCTL_getdomaininfo to permit DOMID_SELF. >>> But the action of granting permission is from dom0 to domU, what I need to get is the infomation of dom0, >>> The actual domid here is domU's id I think, it is not useful. >> >> Note how I said DOMID_SELF and "local domain". There's no talk of using the >> DomU's domid. But what you apparently neglect is the fact that the hardware >> domain isn't necessarily Dom0 (see CONFIG_LATE_HWDOM in the hypervisor). >> While benign in most cases, this is relevant when it comes to referencing >> the hardware domain by domid. And it is the hardware domain which is going >> to drive the device re-assignment, as that domain is who's in possession of >> all the devices not yet assigned to any DomU. > OK, I need to get the information of hardware domain here? Right, with (for this purpose) "hardware domain" == "local domain". Jan
On 2024/6/20 18:42, Jan Beulich wrote: > On 20.06.2024 11:40, Chen, Jiqian wrote: >> On 2024/6/18 17:23, Jan Beulich wrote: >>> On 18.06.2024 10:23, Chen, Jiqian wrote: >>>> On 2024/6/17 23:32, Jan Beulich wrote: >>>>> On 17.06.2024 11:00, Jiqian Chen wrote: >>>>>> @@ -1516,14 +1519,39 @@ static void pci_add_dm_done(libxl__egc *egc, >>>>>> rc = ERROR_FAIL; >>>>>> goto out; >>>>>> } >>>>>> - r = xc_domain_irq_permission(ctx->xch, domid, irq, 1); >>>>>> +#ifdef CONFIG_X86 >>>>>> + /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission */ >>>>>> + r = xc_domain_getinfo_single(ctx->xch, 0, &info); >>>>> >>>>> Hard-coded 0 is imposing limitations. Ideally you would use DOMID_SELF, but >>>>> I didn't check if that can be used with the underlying hypercall(s). Otherwise >> From the commit 10ef7a91b5a8cb8c58903c60e2dd16ed490b3bcf, DOMID_SELF is not allowed for XEN_DOMCTL_getdomaininfo. >> And now XEN_DOMCTL_getdomaininfo gets domain through rcu_lock_domain_by_id. >> >>>>> you want to pass the actual domid of the local domain here. >> What is the local domain here? > > The domain your code is running in. > >> What is method for me to get its domid? > > I hope there's an available function in one of the libraries to do that. I didn't find relate function. Hi Anthony, do you know? > But I wouldn't even know what to look for; that's a question to (primarily) > Anthony then, who sadly continues to be our only tool stack maintainer. > > Alternatively we could maybe enable XEN_DOMCTL_getdomaininfo to permit > DOMID_SELF. It didn't permit DOMID_SELF since below commit. Does it still have the same problem if permit DOMID_SELF? commit 10ef7a91b5a8cb8c58903c60e2dd16ed490b3bcf Author: kfraser@localhost.localdomain <kfraser@localhost.localdomain> Date: Tue Aug 14 09:56:46 2007 +0100 xen: Do not accept DOMID_SELF as input to DOMCTL_getdomaininfo. This was screwing up callers that loop on getdomaininfo(), if there was a domain with domid DOMID_FIRST_RESERVED-1 (== DOMID_SELF-1). They would see DOMID_SELF-1, then look up DOMID_SELF, which has domid 0 of course, and then start their domain-finding loop all over again! Found by Kouya Shimura <kouya@jp.fujitsu.com>. Thanks! Signed-off-by: Keir Fraser <keir@xensource.com> diff --git a/xen/common/domctl.c b/xen/common/domctl.c index 09a1e84d98e0..5d29667b7c3d 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -463,19 +463,13 @@ long do_domctl(XEN_GUEST_HANDLE(xen_domctl_t) u_domctl) case XEN_DOMCTL_getdomaininfo: { struct domain *d; - domid_t dom; - - dom = op->domain; - if ( dom == DOMID_SELF ) - dom = current->domain->domain_id; + domid_t dom = op->domain; rcu_read_lock(&domlist_read_lock); for_each_domain ( d ) - { if ( d->domain_id >= dom ) break; - } if ( d == NULL ) { > >>>> But the action of granting permission is from dom0 to domU, what I need to get is the infomation of dom0, >>>> The actual domid here is domU's id I think, it is not useful. >>> >>> Note how I said DOMID_SELF and "local domain". There's no talk of using the >>> DomU's domid. But what you apparently neglect is the fact that the hardware >>> domain isn't necessarily Dom0 (see CONFIG_LATE_HWDOM in the hypervisor). >>> While benign in most cases, this is relevant when it comes to referencing >>> the hardware domain by domid. And it is the hardware domain which is going >>> to drive the device re-assignment, as that domain is who's in possession of >>> all the devices not yet assigned to any DomU. >> OK, I need to get the information of hardware domain here? > > Right, with (for this purpose) "hardware domain" == "local domain". > > Jan
On 21.06.2024 10:20, Chen, Jiqian wrote: > On 2024/6/20 18:42, Jan Beulich wrote: >> On 20.06.2024 11:40, Chen, Jiqian wrote: >>> On 2024/6/18 17:23, Jan Beulich wrote: >>>> On 18.06.2024 10:23, Chen, Jiqian wrote: >>>>> On 2024/6/17 23:32, Jan Beulich wrote: >>>>>> On 17.06.2024 11:00, Jiqian Chen wrote: >>>>>>> @@ -1516,14 +1519,39 @@ static void pci_add_dm_done(libxl__egc *egc, >>>>>>> rc = ERROR_FAIL; >>>>>>> goto out; >>>>>>> } >>>>>>> - r = xc_domain_irq_permission(ctx->xch, domid, irq, 1); >>>>>>> +#ifdef CONFIG_X86 >>>>>>> + /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission */ >>>>>>> + r = xc_domain_getinfo_single(ctx->xch, 0, &info); >>>>>> >>>>>> Hard-coded 0 is imposing limitations. Ideally you would use DOMID_SELF, but >>>>>> I didn't check if that can be used with the underlying hypercall(s). Otherwise >>> From the commit 10ef7a91b5a8cb8c58903c60e2dd16ed490b3bcf, DOMID_SELF is not allowed for XEN_DOMCTL_getdomaininfo. >>> And now XEN_DOMCTL_getdomaininfo gets domain through rcu_lock_domain_by_id. >>> >>>>>> you want to pass the actual domid of the local domain here. >>> What is the local domain here? >> >> The domain your code is running in. >> >>> What is method for me to get its domid? >> >> I hope there's an available function in one of the libraries to do that. > I didn't find relate function. > Hi Anthony, do you know? > >> But I wouldn't even know what to look for; that's a question to (primarily) >> Anthony then, who sadly continues to be our only tool stack maintainer. >> >> Alternatively we could maybe enable XEN_DOMCTL_getdomaininfo to permit >> DOMID_SELF. > It didn't permit DOMID_SELF since below commit. Does it still have the same problem if permit DOMID_SELF? To answer this, all respective callers would need auditing. However, ... > commit 10ef7a91b5a8cb8c58903c60e2dd16ed490b3bcf > Author: kfraser@localhost.localdomain <kfraser@localhost.localdomain> > Date: Tue Aug 14 09:56:46 2007 +0100 > > xen: Do not accept DOMID_SELF as input to DOMCTL_getdomaininfo. > This was screwing up callers that loop on getdomaininfo(), if there > was a domain with domid DOMID_FIRST_RESERVED-1 (== DOMID_SELF-1). > They would see DOMID_SELF-1, then look up DOMID_SELF, which has domid > 0 of course, and then start their domain-finding loop all over again! > Found by Kouya Shimura <kouya@jp.fujitsu.com>. Thanks! > Signed-off-by: Keir Fraser <keir@xensource.com> ... I view this as a pretty odd justification for the change, when imo the bogus loops should instead have been adjusted. Jan > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index 09a1e84d98e0..5d29667b7c3d 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -463,19 +463,13 @@ long do_domctl(XEN_GUEST_HANDLE(xen_domctl_t) u_domctl) > case XEN_DOMCTL_getdomaininfo: > { > struct domain *d; > - domid_t dom; > - > - dom = op->domain; > - if ( dom == DOMID_SELF ) > - dom = current->domain->domain_id; > + domid_t dom = op->domain; > > rcu_read_lock(&domlist_read_lock); > > for_each_domain ( d ) > - { > if ( d->domain_id >= dom ) > break; > - } > > if ( d == NULL ) > {
On Fri, Jun 21, 2024 at 08:20:55AM +0000, Chen, Jiqian wrote: > On 2024/6/20 18:42, Jan Beulich wrote: > > On 20.06.2024 11:40, Chen, Jiqian wrote: > >> On 2024/6/18 17:23, Jan Beulich wrote: > >>> On 18.06.2024 10:23, Chen, Jiqian wrote: > >>>> On 2024/6/17 23:32, Jan Beulich wrote: > >>>>> On 17.06.2024 11:00, Jiqian Chen wrote: > >>>>>> @@ -1516,14 +1519,39 @@ static void pci_add_dm_done(libxl__egc *egc, > >>>>>> rc = ERROR_FAIL; > >>>>>> goto out; > >>>>>> } > >>>>>> - r = xc_domain_irq_permission(ctx->xch, domid, irq, 1); > >>>>>> +#ifdef CONFIG_X86 > >>>>>> + /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission */ > >>>>>> + r = xc_domain_getinfo_single(ctx->xch, 0, &info); > >>>>> > >>>>> Hard-coded 0 is imposing limitations. Ideally you would use DOMID_SELF, but > >>>>> I didn't check if that can be used with the underlying hypercall(s). Otherwise > >> From the commit 10ef7a91b5a8cb8c58903c60e2dd16ed490b3bcf, DOMID_SELF is not allowed for XEN_DOMCTL_getdomaininfo. > >> And now XEN_DOMCTL_getdomaininfo gets domain through rcu_lock_domain_by_id. > >> > >>>>> you want to pass the actual domid of the local domain here. > >> What is the local domain here? > > > > The domain your code is running in. > > > >> What is method for me to get its domid? > > > > I hope there's an available function in one of the libraries to do that. > I didn't find relate function. > Hi Anthony, do you know? Yes, I managed to find: LIBXL_TOOLSTACK_DOMID That's the value you can use instead of "0" do designate dom0. (That was harder than necessary to find.) Cheers,
On 2024/6/24 16:17, Jan Beulich wrote: > On 21.06.2024 10:20, Chen, Jiqian wrote: >> On 2024/6/20 18:42, Jan Beulich wrote: >>> On 20.06.2024 11:40, Chen, Jiqian wrote: >>>> On 2024/6/18 17:23, Jan Beulich wrote: >>>>> On 18.06.2024 10:23, Chen, Jiqian wrote: >>>>>> On 2024/6/17 23:32, Jan Beulich wrote: >>>>>>> On 17.06.2024 11:00, Jiqian Chen wrote: >>>>>>>> @@ -1516,14 +1519,39 @@ static void pci_add_dm_done(libxl__egc *egc, >>>>>>>> rc = ERROR_FAIL; >>>>>>>> goto out; >>>>>>>> } >>>>>>>> - r = xc_domain_irq_permission(ctx->xch, domid, irq, 1); >>>>>>>> +#ifdef CONFIG_X86 >>>>>>>> + /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission */ >>>>>>>> + r = xc_domain_getinfo_single(ctx->xch, 0, &info); >>>>>>> >>>>>>> Hard-coded 0 is imposing limitations. Ideally you would use DOMID_SELF, but >>>>>>> I didn't check if that can be used with the underlying hypercall(s). Otherwise >>>> From the commit 10ef7a91b5a8cb8c58903c60e2dd16ed490b3bcf, DOMID_SELF is not allowed for XEN_DOMCTL_getdomaininfo. >>>> And now XEN_DOMCTL_getdomaininfo gets domain through rcu_lock_domain_by_id. >>>> >>>>>>> you want to pass the actual domid of the local domain here. >>>> What is the local domain here? >>> >>> The domain your code is running in. >>> >>>> What is method for me to get its domid? >>> >>> I hope there's an available function in one of the libraries to do that. >> I didn't find relate function. >> Hi Anthony, do you know? >> >>> But I wouldn't even know what to look for; that's a question to (primarily) >>> Anthony then, who sadly continues to be our only tool stack maintainer. >>> >>> Alternatively we could maybe enable XEN_DOMCTL_getdomaininfo to permit >>> DOMID_SELF. >> It didn't permit DOMID_SELF since below commit. Does it still have the same problem if permit DOMID_SELF? > > To answer this, all respective callers would need auditing. However, ... > >> commit 10ef7a91b5a8cb8c58903c60e2dd16ed490b3bcf >> Author: kfraser@localhost.localdomain <kfraser@localhost.localdomain> >> Date: Tue Aug 14 09:56:46 2007 +0100 >> >> xen: Do not accept DOMID_SELF as input to DOMCTL_getdomaininfo. >> This was screwing up callers that loop on getdomaininfo(), if there >> was a domain with domid DOMID_FIRST_RESERVED-1 (== DOMID_SELF-1). >> They would see DOMID_SELF-1, then look up DOMID_SELF, which has domid >> 0 of course, and then start their domain-finding loop all over again! >> Found by Kouya Shimura <kouya@jp.fujitsu.com>. Thanks! >> Signed-off-by: Keir Fraser <keir@xensource.com> > > ... I view this as a pretty odd justification for the change, when imo the > bogus loops should instead have been adjusted. Yes, you are right. And Anthony suggested to use LIBXL_TOOLSTACK_DOMID to replace 0 domid. It seems there is no need to change hypercall DOMCTL_getdomaininfo for now? > > Jan > >> diff --git a/xen/common/domctl.c b/xen/common/domctl.c >> index 09a1e84d98e0..5d29667b7c3d 100644 >> --- a/xen/common/domctl.c >> +++ b/xen/common/domctl.c >> @@ -463,19 +463,13 @@ long do_domctl(XEN_GUEST_HANDLE(xen_domctl_t) u_domctl) >> case XEN_DOMCTL_getdomaininfo: >> { >> struct domain *d; >> - domid_t dom; >> - >> - dom = op->domain; >> - if ( dom == DOMID_SELF ) >> - dom = current->domain->domain_id; >> + domid_t dom = op->domain; >> >> rcu_read_lock(&domlist_read_lock); >> >> for_each_domain ( d ) >> - { >> if ( d->domain_id >= dom ) >> break; >> - } >> >> if ( d == NULL ) >> { >
On 2024/6/24 20:33, Anthony PERARD wrote: > On Fri, Jun 21, 2024 at 08:20:55AM +0000, Chen, Jiqian wrote: >> On 2024/6/20 18:42, Jan Beulich wrote: >>> On 20.06.2024 11:40, Chen, Jiqian wrote: >>>> On 2024/6/18 17:23, Jan Beulich wrote: >>>>> On 18.06.2024 10:23, Chen, Jiqian wrote: >>>>>> On 2024/6/17 23:32, Jan Beulich wrote: >>>>>>> On 17.06.2024 11:00, Jiqian Chen wrote: >>>>>>>> @@ -1516,14 +1519,39 @@ static void pci_add_dm_done(libxl__egc *egc, >>>>>>>> rc = ERROR_FAIL; >>>>>>>> goto out; >>>>>>>> } >>>>>>>> - r = xc_domain_irq_permission(ctx->xch, domid, irq, 1); >>>>>>>> +#ifdef CONFIG_X86 >>>>>>>> + /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission */ >>>>>>>> + r = xc_domain_getinfo_single(ctx->xch, 0, &info); >>>>>>> >>>>>>> Hard-coded 0 is imposing limitations. Ideally you would use DOMID_SELF, but >>>>>>> I didn't check if that can be used with the underlying hypercall(s). Otherwise >>>> From the commit 10ef7a91b5a8cb8c58903c60e2dd16ed490b3bcf, DOMID_SELF is not allowed for XEN_DOMCTL_getdomaininfo. >>>> And now XEN_DOMCTL_getdomaininfo gets domain through rcu_lock_domain_by_id. >>>> >>>>>>> you want to pass the actual domid of the local domain here. >>>> What is the local domain here? >>> >>> The domain your code is running in. >>> >>>> What is method for me to get its domid? >>> >>> I hope there's an available function in one of the libraries to do that. >> I didn't find relate function. >> Hi Anthony, do you know? > > Yes, I managed to find: > LIBXL_TOOLSTACK_DOMID > That's the value you can use instead of "0" do designate dom0. > (That was harder than necessary to find.) Thank you very much! I will use LIBXL_TOOLSTACK_DOMID in next version. > > Cheers, >
On 25.06.2024 09:44, Chen, Jiqian wrote: > On 2024/6/24 16:17, Jan Beulich wrote: >> On 21.06.2024 10:20, Chen, Jiqian wrote: >>> On 2024/6/20 18:42, Jan Beulich wrote: >>>> Alternatively we could maybe enable XEN_DOMCTL_getdomaininfo to permit >>>> DOMID_SELF. >>> It didn't permit DOMID_SELF since below commit. Does it still have the same problem if permit DOMID_SELF? >> >> To answer this, all respective callers would need auditing. However, ... >> >>> commit 10ef7a91b5a8cb8c58903c60e2dd16ed490b3bcf >>> Author: kfraser@localhost.localdomain <kfraser@localhost.localdomain> >>> Date: Tue Aug 14 09:56:46 2007 +0100 >>> >>> xen: Do not accept DOMID_SELF as input to DOMCTL_getdomaininfo. >>> This was screwing up callers that loop on getdomaininfo(), if there >>> was a domain with domid DOMID_FIRST_RESERVED-1 (== DOMID_SELF-1). >>> They would see DOMID_SELF-1, then look up DOMID_SELF, which has domid >>> 0 of course, and then start their domain-finding loop all over again! >>> Found by Kouya Shimura <kouya@jp.fujitsu.com>. Thanks! >>> Signed-off-by: Keir Fraser <keir@xensource.com> >> >> ... I view this as a pretty odd justification for the change, when imo the >> bogus loops should instead have been adjusted. > Yes, you are right. > And Anthony suggested to use LIBXL_TOOLSTACK_DOMID to replace 0 domid. > It seems there is no need to change hypercall DOMCTL_getdomaininfo for now? With Anthony's suggestion - indeed. Jan
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h index a0381f74d24b..f3feb6848e25 100644 --- a/tools/include/xenctrl.h +++ b/tools/include/xenctrl.h @@ -1382,6 +1382,11 @@ int xc_domain_irq_permission(xc_interface *xch, uint32_t pirq, bool allow_access); +int xc_domain_gsi_permission(xc_interface *xch, + uint32_t domid, + uint32_t gsi, + bool allow_access); + int xc_domain_iomem_permission(xc_interface *xch, uint32_t domid, unsigned long first_mfn, diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c index f2d9d14b4d9f..8540e84fda93 100644 --- a/tools/libs/ctrl/xc_domain.c +++ b/tools/libs/ctrl/xc_domain.c @@ -1394,6 +1394,21 @@ int xc_domain_irq_permission(xc_interface *xch, return do_domctl(xch, &domctl); } +int xc_domain_gsi_permission(xc_interface *xch, + uint32_t domid, + uint32_t gsi, + bool allow_access) +{ + struct xen_domctl domctl = { + .cmd = XEN_DOMCTL_gsi_permission, + .domain = domid, + .u.gsi_permission.gsi = gsi, + .u.gsi_permission.allow_access = allow_access, + }; + + return do_domctl(xch, &domctl); +} + int xc_domain_iomem_permission(xc_interface *xch, uint32_t domid, unsigned long first_mfn, diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c index 376f91759ac6..f027f22c0028 100644 --- a/tools/libs/light/libxl_pci.c +++ b/tools/libs/light/libxl_pci.c @@ -1431,6 +1431,9 @@ static void pci_add_dm_done(libxl__egc *egc, uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED; uint32_t domainid = domid; bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid); +#ifdef CONFIG_X86 + xc_domaininfo_t info; +#endif /* Convenience aliases */ bool starting = pas->starting; @@ -1516,14 +1519,39 @@ static void pci_add_dm_done(libxl__egc *egc, rc = ERROR_FAIL; goto out; } - r = xc_domain_irq_permission(ctx->xch, domid, irq, 1); +#ifdef CONFIG_X86 + /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission */ + r = xc_domain_getinfo_single(ctx->xch, 0, &info); if (r < 0) { - LOGED(ERROR, domainid, - "xc_domain_irq_permission irq=%d (error=%d)", irq, r); + LOGED(ERROR, domainid, "getdomaininfo failed (error=%d)", errno); fclose(f); rc = ERROR_FAIL; goto out; } + if (info.flags & XEN_DOMINF_hvm_guest && + !(info.arch_config.emulation_flags & XEN_X86_EMU_USE_PIRQ) && + gsi > 0) { + r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1); + if (r < 0) { + LOGED(ERROR, domainid, + "xc_domain_gsi_permission gsi=%d (error=%d)", gsi, errno); + fclose(f); + rc = ERROR_FAIL; + goto out; + } + } + else +#endif + { + r = xc_domain_irq_permission(ctx->xch, domid, irq, 1); + if (r < 0) { + LOGED(ERROR, domainid, + "xc_domain_irq_permission irq=%d (error=%d)", irq, errno); + fclose(f); + rc = ERROR_FAIL; + goto out; + } + } } fclose(f); @@ -2200,6 +2228,10 @@ static void pci_remove_detached(libxl__egc *egc, #endif uint32_t domainid = prs->domid; bool isstubdom; +#ifdef CONFIG_X86 + int r; + xc_domaininfo_t info; +#endif /* Convenience aliases */ libxl_device_pci *const pci = &prs->pci; @@ -2287,9 +2319,32 @@ skip_bar: */ LOGED(ERROR, domid, "xc_physdev_unmap_pirq irq=%d", irq); } - rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0); - if (rc < 0) { - LOGED(ERROR, domid, "xc_domain_irq_permission irq=%d", irq); +#ifdef CONFIG_X86 + /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission */ + r = xc_domain_getinfo_single(ctx->xch, 0, &info); + if (r < 0) { + LOGED(ERROR, domid, "getdomaininfo failed (error=%d)", errno); + fclose(f); + rc = ERROR_FAIL; + goto skip_legacy_irq; + } + if (info.flags & XEN_DOMINF_hvm_guest && + !(info.arch_config.emulation_flags & XEN_X86_EMU_USE_PIRQ) && + gsi > 0) { + r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 0); + if (r < 0) { + LOGED(ERROR, domid, + "xc_domain_gsi_permission gsi=%d (error=%d)", gsi, errno); + rc = ERROR_FAIL; + } + } + else +#endif + { + rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0); + if (rc < 0) { + LOGED(ERROR, domid, "xc_domain_irq_permission irq=%d", irq); + } } } diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index 335aedf46d03..6b465bbc6ec0 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,48 @@ long arch_do_domctl( break; } + case XEN_DOMCTL_gsi_permission: + { + unsigned int gsi = domctl->u.gsi_permission.gsi; + int irq; + bool allow = domctl->u.gsi_permission.allow_access; + + /* Check all pads are zero */ + ret = -EINVAL; + for ( i = 0; + i < sizeof(domctl->u.gsi_permission.pad) / + sizeof(domctl->u.gsi_permission.pad[0]); + ++i ) + if ( domctl->u.gsi_permission.pad[i] ) + goto out; + + /* + * If current domain is PV or it has PIRQ flag, it has a mapping + * of gsi, pirq and irq, so it should use XEN_DOMCTL_irq_permission + * to grant irq permission. + */ + ret = -EOPNOTSUPP; + if ( is_pv_domain(currd) || has_pirq(currd) ) + goto out; + + ret = -EINVAL; + if ( gsi >= nr_irqs_gsi || (irq = gsi_2_irq(gsi)) < 0 ) + goto out; + + ret = -EPERM; + if ( !irq_access_permitted(currd, irq) || + xsm_irq_permission(XSM_HOOK, d, irq, allow) ) + goto out; + + if ( allow ) + ret = irq_permit_access(d, irq); + else + ret = irq_deny_access(d, irq); + + 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 b48a64246548..23845c8cb11f 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 /*