[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC] x86: Add hack to disable "Fake HT" mode
On 15.11.2019 15:29, George Dunlap wrote: > On 11/15/19 2:18 PM, Jan Beulich wrote: >> On 15.11.2019 15:10, George Dunlap wrote: >>> On 11/15/19 2:06 PM, Andrew Cooper wrote: >>>> On 15/11/2019 14:04, George Dunlap wrote: >>>>>>> It's not entirely uncommon to (also) consider how the resulting >>>>>>> diff would look like when putting together a change. And besides >>>>>>> the review aspect, there's also the archeology one - "git blame" >>>>>>> yields much more helpful results when code doesn't get moved >>>>>>> around more than necessary. But yes, there's no very clear "this >>>>>>> is the better option" here. I've taken another look at the code >>>>>>> before your change though - everything is already nicely in one >>>>>>> place with Andrew's most recent code reorg. So I'm now having an >>>>>>> even harder time seeing why you want to move things around again. >>>>>> We don't. I've recommend twice now to have a single "else if" hunk >>>>>> which is nearly empty, and much more obviously a gross "make it work for >>>>>> 4.13" bodge. >>>>> The results are a tiny bit better, but not much really (see attached). >>>> >>>> What I meant was: >>>> >>>>> diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c >>>>> index 312c481f1e..bc088e45f0 100644 >>>>> --- a/tools/libxc/xc_cpuid_x86.c >>>>> +++ b/tools/libxc/xc_cpuid_x86.c >>>>> @@ -579,52 +579,71 @@ int xc_cpuid_apply_policy(xc_interface *xch, >>>>> uint32_t domid, >>>>> } >>>> >>>> else if ( getenv() ) >>>> { >>>> ... >>>> } >>>> >>>>> else >>>>> { >>>> >>>> With no delta to this block at all. >>> >>> Then we have to duplicate this code in both blocks: >>> >>> /* >>> * These settings are necessary to cause earlier >>> HVM_PARAM_NESTEDHVM / >>> * XEN_DOMCTL_disable_migrate settings to be reflected correctly in >>> * CPUID. Xen will discard these bits if configuration hasn't been >>> * set for the domain. >>> */ >>> p->extd.itsc = true; >>> p->basic.vmx = true; >>> p->extd.svm = true; >>> >>> I won't object if that's what you guys really want. >> >> Personally I think the duplication is less bad than the far >> heavier original code churn, but to be honest, especially with >> this intended to go away again soon anyway, I'd not be opposed >> at all to >> >> ... >> else if ( getenv() ) >> goto no_fake_ht; > > This isn't correct, because you do need to clear htt and cmp_legacy, as > well as zeroing out cores_per_package and threads_per_cache on Intel. > (At least, that's what XenServer's patch does, and it's the best tested.) > >> else >> { >> ... >> no_fake_ht: >> /* >> * These settings are necessary to cause earlier HVM_PARAM_NESTEDHVM >> / >> * XEN_DOMCTL_disable_migrate settings to be reflected correctly in >> * CPUID. Xen will discard these bits if configuration hasn't been >> * set for the domain. >> */ >> p->extd.itsc = true; >> p->basic.vmx = true; >> p->extd.svm = true; >> } >> >> (despite my general dislike of goto). > > Well I think gotos into other blocks is even worse. :-) > > I think the result is a lot nicer to review for sure. Trying to comment despite this having been an attachment: >--- a/tools/libxc/xc_cpuid_x86.c >+++ b/tools/libxc/xc_cpuid_x86.c >@@ -579,6 +579,26 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t >domid, > } > else > { >+ if ( getenv("XEN_LIBXC_DISABLE_FAKEHT") ) { The brace wants to move onto its own line. >+ p->basic.htt = false; I think everything below here indeed simply undoes what said old commit did, but I can't match up this one. And together with the question of whether instead leaving it alone, cmp_legacy then would have the same question raised. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |