Re: [Xen-devel] Ping: Re: [PATCH 2/2] x86: drop cpu_has_sse{,2}

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.12.16 at 13:03, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 15/12/16 09:57, Jan Beulich wrote:
>>>>> On 09.12.16 at 16:27, <JBeulich@xxxxxxxx> wrote:
>>>>>> On 09.12.16 at 16:19, <JBeulich@xxxxxxxx> wrote:
>>>>>>> On 09.12.16 at 15:56, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> It is a layering violation to remove the host_has_* part of the check
>>>>> from the emulator.
>>>> This is from a very, very abstract perspective. The fact that
>>>> host_and_vcpu_*() and vcpu_*() are the same in the harness
>>>> should allow for them to be used interchangeably when only the
>>>> harness is affected.
>>> I should add that in a subsequent patch adding emulation of
>>> LDDQU (an SSE3 insn) this is going to be done by slightly
>>> extending the existing MOVDQ{U,A} etc block, i.e. using an
>>> SSE2 insn. If I was to follow your directions, I'd have to add
>>> separate host_has(sse2) and vcpu_has(sse3) checks. That's
>>> just going to become insane. I have just the vcpu check in
>>> there, and I really hope you won't force me to uglify that.
>> And one more which did stall. Meanwhile I've submitted patches
>> falling into the above category, which should help making a
>> judgment here.
> 
> I am sorry, but I remain unconvinced by these arguments.
> 
> The rule governing the use host_and_vcpu_*() is a) correct, and b) very
> clear when it comes to using instructions from certain classes as part
> of emulation.
> 
> In particular, a 32bit build of the emulator should not crash even if
> there is junk in the cpuid policy.

And it doesn't. Once again - the comment you refer to is relevant
for the hypervisor only (which its placement tries to make obvious).
For the harness, the host and vcpu checks are equivalent.

Did you look at some of those newer patches, to estimate how odd it
would be to add separate vcpu_must_have() and host_must_have()
checks? Given the (growing) size of the part of the code base, I think
we should always strive for as concise code as possible - no
unnecessary clutter or anything. Much of my recent code folding was
motivated by that, and I appreciate your recent recognition of further
opportunities in that area.

> This means either, we drop support for 32bit builds of the emulator, or
> retain the proper host_and_vcpu_*() checks.
> 
> 
> An alternative which comes to mind and hasn't been discussed yet, would
> be to formally require a 64bit cpu as a minimum requirement, even for
> 32bit builds.  In fact, I think we should have rather more written
> documentation anyway (possibly at the head of x86_emulate.c), including
> the instruction groups and behaviours we expect to be implemented, and
> which we expect to currently be missing.
> 
> If we had a formal statement of requiring a 64bit-capable CPU for
> running the emulator, I'd be happy to relax the checks based on the
> assumptions we are permitted to make.

Well, if I really can't convince you, I wouldn't see any issue with
documenting such a requirement (albeit it's not really clear where
that would best go, as putting it at the top of some of the files'
would imo make it likely to be overlooked by anyone not specifically
searching for any such statement).

Jan


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