[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 2/2] xen/domain: rewrite emulation_flags_ok()
On Wed, Jun 04, 2025 at 12:43:22PM +0200, Jan Beulich wrote: > 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. Sure, no problem, I can address that. Thanks! > > > + }, > > + > > + /* 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. Ack. > > Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |