Message ID | 20210427130201.2142-1-andrew.cooper3@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86/pv: Drop HYPERVISOR_COMPAT_VIRT_START() | expand |
On 27.04.2021 15:02, Andrew Cooper wrote: > This is pure obfuscation (in particular, hiding the two locations where the > variable gets set), and is longer than the code it replaces. Obfuscation - not just; see below. > Also fix MACH2PHYS_COMPAT_VIRT_START to be expressed as a 1-parameter macro, > which is how it is used. The current construct only works because > HYPERVISOR_COMPAT_VIRT_START was also a 1-parameter macro. I don't mind this getting changed, but I don't think there's any "fixing" involved here. Omitting macro parameters from macros forwarding to other macros (or functions) is well defined and imo not a problem at all. In fact, if at the end of all expansions a macro evaluates to a function, it may be necessary to do so in case covering not just function invocations is intended, but also taking of its address. > --- a/xen/arch/x86/pv/descriptor-tables.c > +++ b/xen/arch/x86/pv/descriptor-tables.c > @@ -235,7 +235,7 @@ static bool check_descriptor(const struct domain *dom, seg_desc_t *d) > if ( b & _SEGMENT_G ) > limit <<= 12; > > - if ( (base == 0) && (limit > HYPERVISOR_COMPAT_VIRT_START(dom)) ) > + if ( (base == 0) && (limit > dom->arch.hv_compat_vstart) ) I expect this (and a few more instances) to fail to build when !PV32. It was the purpose of ... > --- a/xen/include/asm-x86/config.h > +++ b/xen/include/asm-x86/config.h > @@ -254,21 +254,16 @@ extern unsigned char boot_edid_info[128]; > > /* This is not a fixed value, just a lower limit. */ > #define __HYPERVISOR_COMPAT_VIRT_START 0xF5800000 > -#define HYPERVISOR_COMPAT_VIRT_START(d) ((d)->arch.hv_compat_vstart) > - > -#else /* !CONFIG_PV32 */ > - > -#define HYPERVISOR_COMPAT_VIRT_START(d) ((void)(d), 0) ... this to allow things to build in the absence of the actual struct member. Jan
On 27/04/2021 14:18, Jan Beulich wrote: > On 27.04.2021 15:02, Andrew Cooper wrote: >> This is pure obfuscation (in particular, hiding the two locations where the >> variable gets set), and is longer than the code it replaces. > Obfuscation - not just; see below. > >> Also fix MACH2PHYS_COMPAT_VIRT_START to be expressed as a 1-parameter macro, >> which is how it is used. The current construct only works because >> HYPERVISOR_COMPAT_VIRT_START was also a 1-parameter macro. > I don't mind this getting changed, but I don't think there's any > "fixing" involved here. Omitting macro parameters from macros > forwarding to other macros (or functions) is well defined and imo > not a problem at all. In fact, if at the end of all expansions a > macro evaluates to a function, it may be necessary to do so in case > covering not just function invocations is intended, but also taking > of its address. It might be well formed, but it doesn't help at all with trying to follow what the code does. > >> --- a/xen/arch/x86/pv/descriptor-tables.c >> +++ b/xen/arch/x86/pv/descriptor-tables.c >> @@ -235,7 +235,7 @@ static bool check_descriptor(const struct domain *dom, seg_desc_t *d) >> if ( b & _SEGMENT_G ) >> limit <<= 12; >> >> - if ( (base == 0) && (limit > HYPERVISOR_COMPAT_VIRT_START(dom)) ) >> + if ( (base == 0) && (limit > dom->arch.hv_compat_vstart) ) > I expect this (and a few more instances) to fail to build when !PV32. > It was the purpose of ... > >> --- a/xen/include/asm-x86/config.h >> +++ b/xen/include/asm-x86/config.h >> @@ -254,21 +254,16 @@ extern unsigned char boot_edid_info[128]; >> >> /* This is not a fixed value, just a lower limit. */ >> #define __HYPERVISOR_COMPAT_VIRT_START 0xF5800000 >> -#define HYPERVISOR_COMPAT_VIRT_START(d) ((d)->arch.hv_compat_vstart) >> - >> -#else /* !CONFIG_PV32 */ >> - >> -#define HYPERVISOR_COMPAT_VIRT_START(d) ((void)(d), 0) > ... this to allow things to build in the absence of the actual struct > member. Hmm - I really should have got this change in earlier, shouldn't I... For this example you've pointed out, feeding 0 into the limit calculation is not a correct thing to do in the slightest. ~Andrew
On 27.04.2021 20:05, Andrew Cooper wrote: > On 27/04/2021 14:18, Jan Beulich wrote: >> On 27.04.2021 15:02, Andrew Cooper wrote: >>> --- a/xen/include/asm-x86/config.h >>> +++ b/xen/include/asm-x86/config.h >>> @@ -254,21 +254,16 @@ extern unsigned char boot_edid_info[128]; >>> >>> /* This is not a fixed value, just a lower limit. */ >>> #define __HYPERVISOR_COMPAT_VIRT_START 0xF5800000 >>> -#define HYPERVISOR_COMPAT_VIRT_START(d) ((d)->arch.hv_compat_vstart) >>> - >>> -#else /* !CONFIG_PV32 */ >>> - >>> -#define HYPERVISOR_COMPAT_VIRT_START(d) ((void)(d), 0) >> ... this to allow things to build in the absence of the actual struct >> member. > > Hmm - I really should have got this change in earlier, shouldn't I... > > For this example you've pointed out, feeding 0 into the limit > calculation is not a correct thing to do in the slightest. Does it actually get fed into any such calculation? From looking around yesterday as well as from when I made that change over half a year ago I seem to recall that all potentially problematic uses are already suitably guarded. Jan
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 4dc27f798e..5d8b864718 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -791,7 +791,7 @@ int arch_domain_create(struct domain *d, d->arch.emulation_flags = emflags; #ifdef CONFIG_PV32 - HYPERVISOR_COMPAT_VIRT_START(d) = + d->arch.hv_compat_vstart = is_pv_domain(d) ? __HYPERVISOR_COMPAT_VIRT_START : ~0u; #endif diff --git a/xen/arch/x86/pv/descriptor-tables.c b/xen/arch/x86/pv/descriptor-tables.c index 5e84704400..0d22759820 100644 --- a/xen/arch/x86/pv/descriptor-tables.c +++ b/xen/arch/x86/pv/descriptor-tables.c @@ -235,7 +235,7 @@ static bool check_descriptor(const struct domain *dom, seg_desc_t *d) if ( b & _SEGMENT_G ) limit <<= 12; - if ( (base == 0) && (limit > HYPERVISOR_COMPAT_VIRT_START(dom)) ) + if ( (base == 0) && (limit > dom->arch.hv_compat_vstart) ) { a |= 0x0000ffff; b |= 0x000f0000; diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c index e0801a9e6d..5e70422678 100644 --- a/xen/arch/x86/pv/dom0_build.c +++ b/xen/arch/x86/pv/dom0_build.c @@ -432,7 +432,7 @@ int __init dom0_construct_pv(struct domain *d, printk("Dom0 expects too high a hypervisor start address\n"); return -ERANGE; } - HYPERVISOR_COMPAT_VIRT_START(d) = + d->arch.hv_compat_vstart = max_t(unsigned int, m2p_compat_vstart, value); } @@ -603,7 +603,7 @@ int __init dom0_construct_pv(struct domain *d, /* Overlap with Xen protected area? */ if ( compat - ? v_end > HYPERVISOR_COMPAT_VIRT_START(d) + ? v_end > d->arch.hv_compat_vstart : (v_start < HYPERVISOR_VIRT_END) && (v_end > HYPERVISOR_VIRT_START) ) { printk("DOM0 image overlaps with Xen private area.\n"); diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c index c41ce847b3..a51c2b52d9 100644 --- a/xen/arch/x86/x86_64/mm.c +++ b/xen/arch/x86/x86_64/mm.c @@ -1029,7 +1029,7 @@ int pagefault_by_memadd(unsigned long addr, struct cpu_user_regs *regs) struct domain *d = current->domain; return mem_hotplug && guest_mode(regs) && is_pv_32bit_domain(d) && - (addr >= HYPERVISOR_COMPAT_VIRT_START(d)) && + (addr >= d->arch.hv_compat_vstart) && (addr < MACH2PHYS_COMPAT_VIRT_END); } @@ -1048,7 +1048,7 @@ int handle_memadd_fault(unsigned long addr, struct cpu_user_regs *regs) if (!is_pv_32bit_domain(d)) return 0; - if ( (addr < HYPERVISOR_COMPAT_VIRT_START(d)) || + if ( (addr < d->arch.hv_compat_vstart) || (addr >= MACH2PHYS_COMPAT_VIRT_END) ) return 0; diff --git a/xen/common/compat/kernel.c b/xen/common/compat/kernel.c index 804b919bdc..57845a6c07 100644 --- a/xen/common/compat/kernel.c +++ b/xen/common/compat/kernel.c @@ -27,7 +27,7 @@ CHECK_TYPE(capabilities_info); #define xen_platform_parameters compat_platform_parameters #define xen_platform_parameters_t compat_platform_parameters_t #undef HYPERVISOR_VIRT_START -#define HYPERVISOR_VIRT_START HYPERVISOR_COMPAT_VIRT_START(current->domain) +#define HYPERVISOR_VIRT_START current->domain->arch.hv_compat_vstart #define xen_changeset_info compat_changeset_info #define xen_changeset_info_t compat_changeset_info_t diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h index 883c2ef0df..130db90b5c 100644 --- a/xen/include/asm-x86/config.h +++ b/xen/include/asm-x86/config.h @@ -254,21 +254,16 @@ extern unsigned char boot_edid_info[128]; /* This is not a fixed value, just a lower limit. */ #define __HYPERVISOR_COMPAT_VIRT_START 0xF5800000 -#define HYPERVISOR_COMPAT_VIRT_START(d) ((d)->arch.hv_compat_vstart) - -#else /* !CONFIG_PV32 */ - -#define HYPERVISOR_COMPAT_VIRT_START(d) ((void)(d), 0) #endif /* CONFIG_PV32 */ -#define MACH2PHYS_COMPAT_VIRT_START HYPERVISOR_COMPAT_VIRT_START +#define MACH2PHYS_COMPAT_VIRT_START(d) (d)->arch.hv_compat_vstart #define MACH2PHYS_COMPAT_VIRT_END 0xFFE00000 #define MACH2PHYS_COMPAT_NR_ENTRIES(d) \ ((MACH2PHYS_COMPAT_VIRT_END-MACH2PHYS_COMPAT_VIRT_START(d))>>2) #define COMPAT_L2_PAGETABLE_FIRST_XEN_SLOT(d) \ - l2_table_offset(HYPERVISOR_COMPAT_VIRT_START(d)) + l2_table_offset((d)->arch.hv_compat_vstart) #define COMPAT_L2_PAGETABLE_LAST_XEN_SLOT l2_table_offset(~0U) #define COMPAT_L2_PAGETABLE_XEN_SLOTS(d) \ (COMPAT_L2_PAGETABLE_LAST_XEN_SLOT - COMPAT_L2_PAGETABLE_FIRST_XEN_SLOT(d) + 1) diff --git a/xen/include/asm-x86/x86_64/uaccess.h b/xen/include/asm-x86/x86_64/uaccess.h index ba79f950fb..8c7df060d5 100644 --- a/xen/include/asm-x86/x86_64/uaccess.h +++ b/xen/include/asm-x86/x86_64/uaccess.h @@ -55,7 +55,7 @@ extern void *xlat_malloc(unsigned long *xlat_page_current, size_t size); access_ok(addr, (count) * (size))) #define __compat_addr_ok(d, addr) \ - ((unsigned long)(addr) < HYPERVISOR_COMPAT_VIRT_START(d)) + ((unsigned long)(addr) < (d)->arch.hv_compat_vstart) #define __compat_access_ok(d, addr, size) \ __compat_addr_ok(d, (unsigned long)(addr) + ((size) ? (size) - 1 : 0))
This is pure obfuscation (in particular, hiding the two locations where the variable gets set), and is longer than the code it replaces. Also fix MACH2PHYS_COMPAT_VIRT_START to be expressed as a 1-parameter macro, which is how it is used. The current construct only works because HYPERVISOR_COMPAT_VIRT_START was also a 1-parameter macro. No functional change, but does create easier-to-follow logic. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> --- xen/arch/x86/domain.c | 2 +- xen/arch/x86/pv/descriptor-tables.c | 2 +- xen/arch/x86/pv/dom0_build.c | 4 ++-- xen/arch/x86/x86_64/mm.c | 4 ++-- xen/common/compat/kernel.c | 2 +- xen/include/asm-x86/config.h | 9 ++------- xen/include/asm-x86/x86_64/uaccess.h | 2 +- 7 files changed, 10 insertions(+), 15 deletions(-)