Re: [Xen-devel] [PATCH RFC 26/31] xen/x86: Rework AMD masking MSR setup

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.01.16 at 14:59, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 22/01/16 11:13, Jan Beulich wrote:
>>>>> On 22.01.16 at 12:01, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 22/01/16 09:27, Jan Beulich wrote:
>>>>>>> On 16.12.15 at 22:24, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> +        expected_levelling_cap, levelling_caps,
>>>>> +        (expected_levelling_cap ^ levelling_caps) & levelling_caps);
>>>>> + printk(XENLOG_WARNING "Fam %#x, model %#x level %#x\n",
>>>>> +        c->x86, c->x86_model, c->cpuid_level);
>>>>> + printk(XENLOG_WARNING
>>>>> +        "If not running virtualised, please report a bug\n");
>>>> Well - you checked for running virtualized, so making it here when
>>>> running virtualized is already a bug (albeit not in the code here)?
>>> Why would it be a bug?  The virtualising environment might provide these
>>> MSRs, in which case we should use them.
>> Because you won't make it here when cpu_has_hypervisor.
> 
> Not all hypervisors advertise themselves with the hypervisor bit.  Some
> versions of HyperV and ESXi will panic if they see a hypervisor bit, so
> virtualisation software needs to hide the hypervisor bit to successfully
> visualise some software.

Well, but that's the bug I talk of: Not setting that CPUID bit is
not following the spec, and forcing it off to make some code work
in a virtualized environment isn't much better.

> The user reading Xen's error message is in a better position to judge
> when Xen is actually virtualised, because Xen is not able to say with
> certainty.

Okay, okay, bit that way then.

>>>> And then, how is this supposed to work? You only restore defaults,
>>>> but never write non-default values. Namely, nextd is an unused
>>>> function parameter ...
>>>>
>>>> Also I guess my comment about adding unused code needs
>>>> repeating here.
>>> Future patches build on this, both using the parameter, and not using
>>> the defaults.
>>>
>>> I am also certain that if I did two patches, the first putting in a void
>>> function, and the second changing it to a pointer, your review would ask
>>> me to turn it into this.
>> Well, I realize things will all make sense by the end of the series, but
>> honestly in some of the cases I'm not sure the split between patches
>> was well done.
> 
> If you can suggest a better ordering then I am all ears.

For example, move all the context switch logic into the patch
actually invoking the new hook. That still leaves more than
enough in the AMD and Intel rework patches.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel