Re: [Xen-devel] [PATCH v2 2/2] x86/mmcfg/drhd: Move acpi_mmcfg_init() call before calling acpi_parse_dmar()
Ok, that make sense for me. Thanks for your detailed explanation, I will rewrite the patch next week. Sent from mobile 2018年8月17日 20:28于 Jan Beulich 写道： > > >>> On 17.08.18 at 09:01, wrote: > > pci_conf_read8() needs pci mmcfg mapping to work on multiple pci segments > > system such as HPE Superdome-Flex. > > > > Move acpi_mmcfg_init() call in acpi_boot_init() before calling > > acpi_parse_dmar() so that when pci_conf_read8() is called in > > acpi_parse_dev_scope(), we already have the mapping set up. > > > > acpi_mmcfg_init() only setup mmcfg mapping and has some global variables > > initialized so there is no hazard to move it ahead. > > I'm afraid this is a gross understatement. "Setup mappings" > alone is already putting such movement under question. Have > you checked explicitly that the initialization needed for this > setting up of mappings, if any, has actually occurred before > the new point the function gets called? > > In particular, mmio_ro_ranges looks to get set up only after > the call to acpi_boot_init(). I guess you didn't see a crash > solely because you also move the invocation across the call > to zap_low_mappings(). > > Similary pci_mmcfg_check_hostbridge() doesn't look all that > trivial. > > Please may I ask that you be quite a bit more careful here, > even more so when you've been warned already? > > > Meanwhile from its > > name, acpi_boot_init() is a proper place to call it. > > > > The alternative is moving the acpi_parse_dev_scope() call to a later point > > after acpi_mmcfg_init(). But acpi_parse_one_drhd(), acpi_parse_one_rmrr() > > and acpi_parse_one_atsr() all called acpi_parse_dev_scope() as their main > > job. Splitting those functions to two pieces looks less optimal and > > meaningless. > > Certainly not meaningless; I'm also not sure why you consider > the device scope parsing their main job. It's perhaps their > most involved part, but the fact alone that for the purposes > here we could probably get away without that part already > suggests to me that this is a secondary (yet necessary) aspect. > > Furthermore MMCFG will continue to not work this early (or > more precisely not at all until Dom0 boot has progressed far > enough) if the range(s) isn't/aren't marked reserved in E820. > It would be worthwhile saying half a sentence to this effect > in the description. > > Jan > > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] 回复: Re: 回复: [PATCH] x86/spec_ctrl: Fix several bugs in SPEC_CTRL_ENTRY_FROM_INTR_IST
2018年2月14日 20:21于 Andrew Cooper 写道： > > On 14/02/18 12:08, zhenzhong.duan wrote: > > > > > @@ -286,13 +286,13 @@ > > > setz %dl > > > and %dl, STACK_CPUINFO_FIELD(use_shadow_spec_ctrl)(%r14) > > Is it safe to remove the 'xor %edx, %edx' above? setz set whole byte 1 > > or 0. > > > > It is safe, but it is not a good idea. > > Using setz is an 8bit operation, which will suffer a register merge > stall in the pipeline as we know for certain at this point that the > upper bits of %edx are nonzero at this point. (An encoding which > allowed setz %eax would have been far more useful in 64bit code.) > Thanks for explain. But just curious does processor has ideas the whole register is zeroed and avoid a merge? thanks zduan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] 回复: [PATCH] x86/spec_ctrl: Fix several bugs in SPEC_CTRL_ENTRY_FROM_INTR_IST
2018年2月14日 19:10于 Andrew Cooper 写道： > > DO_OVERWRITE_RSB clobbers %rax, meaning in practice that that the > bti_ist_info > field gets zeroed. Older versions of this code had the DO_OVERWRITE_RSB > register selectable, so reintroduce this ability and use it to cause the > INTR_IST path to use %rdx instead. > > The use of %dl for the %cs.rpl check means that when an IST interrupt hits > Xen, we try to load 1 into the high 32 bits of MSR_SPEC_CTRL, suffering a #GP > fault instead. > > Also, drop an unused label which was a copy/paste mistake. > > Reported-by: Boris Ostrovsky > Reported-by: Zhenzhong Duan > Signed-off-by: Andrew Cooper > --- > CC: Jan Beulich > CC: Zhenzhong Duan > CC: Boris Ostrovsky > CC: Wei Liu > CC: Roger Pau Monné > --- > xen/include/asm-x86/spec_ctrl_asm.h | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/xen/include/asm-x86/spec_ctrl_asm.h > b/xen/include/asm-x86/spec_ctrl_asm.h > index 814f53d..1f78599 100644 > --- a/xen/include/asm-x86/spec_ctrl_asm.h > +++ b/xen/include/asm-x86/spec_ctrl_asm.h > @@ -79,10 +79,10 @@ > * - SPEC_CTRL_EXIT_TO_GUEST > */ > > -.macro DO_OVERWRITE_RSB > +.macro DO_OVERWRITE_RSB tmp=%rax > /* > * Requires nothing > - * Clobbers %rax, %rcx > + * Clobbers \tmp (%rax by default), %rcx > * > * Requires 256 bytes of stack space, but %rsp has no net change. Based on > * Google's performance numbers, the loop is unrolled to 16 iterations and > two > @@ -97,7 +97,7 @@ > * optimised with mov-elimination in modern cores. > */ > mov $16, %ecx /* 16 iterations, two calls per loop */ > - mov %rsp, %rax /* Store the current %rsp */ > + mov %rsp, \tmp /* Store the current %rsp */ > > .L\@_fill_rsb_loop: > > @@ -114,7 +114,7 @@ > > sub $1, %ecx > jnz .L\@_fill_rsb_loop > - mov %rax, %rsp /* Restore old %rsp */ > + mov \tmp, %rsp /* Restore old %rsp */ > .endm > > .macro DO_SPEC_CTRL_ENTRY_FROM_VMEXIT ibrs_val:req > @@ -274,7 +274,7 @@ > testb $BTI_IST_RSB, %al > jz .L\@_skip_rsb > > - DO_OVERWRITE_RSB > + DO_OVERWRITE_RSB tmp=%rdx /* Clobbers %ecx/%rdx */ > > .L\@_skip_rsb: > > @@ -286,13 +286,13 @@ > setz %dl > and %dl, STACK_CPUINFO_FIELD(use_shadow_spec_ctrl)(%r14) Is it safe to remove the 'xor %edx, %edx' above? setz set whole byte 1 or 0. --- thanks zduan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] 回复: Re: [PATCH] Choose retpoline only when it is safe to use
2018年2月6日 17:20于 Andrew Cooper 写道： > > On 06/02/2018 09:13, Zhenzhong Duan wrote: > > 在 2018/2/6 16:59, Andrew Cooper 写道: > >> On 06/02/2018 08:43, Zhenzhong Duan wrote: > >>> When ( ibrs && thunk == THUNK_DEFAULT && !retpoline_safe() ) is true, > >>> thunk is set to THUNK_JMP rather than THUNK_RETPOLINE. > >>> > >>> When (!ibrs && thunk == THUNK_DEFAULT && !retpoline_safe() ) is true, > >>> we should do the same. > >>> > >>> Signed-off-by: Zhenzhong Duan > >> Why? What improvement is this intended to give? > > No improvement, I just feel if retpoline isn't safe, THUNK_JMP is > > better and safer. > > Above first check is working that way. > > If your only two choices are unsafe repoline or plain jumps, then unsafe > repoline is far far far safer. > > Its unsafe properties only kick in on an RSB underflow, and an attacker > would have to do call-depths analysis of the running binary to identify > which rets to attempt to poison. > Thanks for explaining. So, for a retpoline safe processor, it just stop using RSB when it's empty to avoid underflow? Another question: if (opt_thunk == THUNK_DEFAULT && opt_ibrs == -1 && CONFIG_INDIRECT_THUNK && !cpu_has_lfence_dispatch && !retpoline_safe()) results in "thunk = THUNK_JMP" regardless of the value of "boot_cpu_has(X86_FEATURE_IBRSB)" Any reason not considering retpoline here if (!boot_cpu_has(X86_FEATURE_IBRSB)? thanks zduan___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
