[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 2/2] xen/domain: rewrite emulation_flags_ok()
On 02.06.2025 21:17, dmkhn@xxxxxxxxx wrote: > From: Denis Mukhin <dmukhin@xxxxxxxx> > > Rewrite emulation_flags_ok() to simplify future modifications. > > No functional change intended. > > Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx> > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx> > Reviewed-by: Teddy Astie <teddy.astie@xxxxxxxxxx> > --- > Changes since v4: > - updated commentaries > - added Teddy's R-b, kept Stefano's R-b > --- > xen/arch/x86/domain.c | 91 ++++++++++++++++++++++++++++++++++--------- > 1 file changed, 73 insertions(+), 18 deletions(-) Given this diffstat, I wonder what the other x86 maintainers think about this. > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -743,32 +743,87 @@ int arch_sanitise_domain_config(struct > xen_domctl_createdomain *config) > return 0; > } > > +/* > + * Verify that the domain's emulation flags resolve to a supported > configuration. > + * > + * This ensures we only allow a known, safe subset of emulation combinations > + * (for both functionality and security). Arbitrary mixes are likely to cause > + * errors (e.g., null pointer dereferences). > + * > + * NB: use the internal X86_EMU_XXX symbols, not the public XEN_X86_EMU_XXX > + * symbols. > + */ > static bool emulation_flags_ok(const struct domain *d, uint32_t emflags) > { > + enum { > + CAP_PV = BIT(0, U), > + CAP_HVM = BIT(1, U), > + CAP_HWDOM = BIT(2, U), > + CAP_DOMU = BIT(3, U), > + }; > + static const struct { > + unsigned int caps; > + uint32_t min; > + uint32_t opt; > + } configs[] = { > +#ifdef CONFIG_PV > + /* PV */ > + { > + .caps = CAP_PV | CAP_DOMU, > + .min = 0, > + .opt = 0, Why the latter two initializers? Imo adding ones which say nothing else than what's the default is only enlarging code without much real benefit. > + }, > + > + /* PV dom0 */ > + { > + .caps = CAP_PV | CAP_HWDOM, > + .min = X86_EMU_PIT, > + .opt = 0, > + }, > +#endif /* #ifdef CONFIG_PV */ > + > +#ifdef CONFIG_HVM > + /* PVH dom0 */ > + { > + .caps = CAP_HVM | CAP_HWDOM, > + .min = X86_EMU_LAPIC | X86_EMU_IOAPIC | X86_EMU_VPCI, > + .opt = 0, > + }, > + > + /* HVM domU */ > + { > + .caps = CAP_HVM | CAP_DOMU, > + .min = X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ), > + /* HVM PIRQ feature is user-selectable. */ > + .opt = X86_EMU_USE_PIRQ, > + }, > + > + /* PVH domU */ > + { > + .caps = CAP_HVM | CAP_DOMU, > + .min = X86_EMU_LAPIC, > + .opt = 0, > + }, > +#endif /* #ifdef CONFIG_HVM */ > + }; > + unsigned int i, caps = is_hardware_domain(d) ? CAP_HWDOM : CAP_DOMU; > + > + if ( is_pv_domain(d) ) > + caps |= CAP_PV; > + else if ( is_hvm_domain(d) ) > + caps |= CAP_HVM; There's no 3rd case, so this could be expressed with plain "else", and hence also with a conditional operator, and hence could also be right in the initializer. How far to go with those transformations I'm not sure; personally I'd go all the way, but I'd be okay-ish with just the first of the steps. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |