On 06.09.2021 20:07, Andrew Cooper wrote:
> On 06/09/2021 16:17, Jan Beulich wrote:
>> On 06.09.2021 14:00, Jane Malalane wrote:
>>> --- a/xen/arch/x86/cpu/amd.c
>>> +++ b/xen/arch/x86/cpu/amd.c
>>> @@ -681,6 +681,19 @@ void amd_init_lfence(struct cpuinfo_x86 *c)
>>> c->x86_capability);
>>> }
>>>
>>> +void detect_zen2_null_seg_behaviour(void)
>> This can in principle be marked __init.
>>
>>> +{
>>> + uint64_t base;
>>> +
>>> + wrmsrl(MSR_FS_BASE, 1);
>>> + asm volatile ( "mov %0, %%fs" :: "rm" (0) );
>> While I don't strictly mind the "m" part of the constraint to remain
>> there (in the hope for compilers actually to support this), iirc it's
>> not useful to have when the value is a constant: Last time I checked,
>> the compiler would not instantiate an anonymous (stack) variable to
>> fulfill this constraint (as can be seen when dropping the "r" part of
>> the constraint).
>
> This is "rm" because it is what we use elsewhere in Xen for selectors,
> and because it is the correct constraints based on the legal instruction
> encodings.
grep-ing for "%%[defgs]s" reveals:
efi_arch_post_exit_boot(), svm_ctxt_switch_to(), and
do_set_segment_base() all use just "r". This grep has not produced
any use of "rm". What are you talking about?
> If you want to work around what you perceive to be bugs in compilers
> then submit a independent change yourself.
I don't perceive this as a bug; perhaps a desirable feature. I also
did start my response with "While I don't strictly mind the "m"
part ..." - was this not careful enough to indicate I'm not going
to insist on the change, but I'd prefer it to be made?
>>> @@ -731,6 +744,11 @@ static void init_amd(struct cpuinfo_x86 *c)
>>> else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */
>>> amd_init_lfence(c);
>>>
>>> + /* Probe for NSCB on Zen2 CPUs when not virtualised */
>>> + if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data &&
>>> + c->x86 == 0x17 && c->x86_model >= 30 && c->x86_model <= 0x5f)
>> DYM 0x30 here?
>
> 0x30, although it turns out that some of the mobile Zen2 CPUs exceed
> 0x60 in terms of model number.
>
> As Zen3 changes the family number to 0x19, I'd just drop the upper bound.
Minor note: Even if it didn't, the !cpu_has_nscb would also be enough
to avoid the probing there.
>> Or 0x1e? In any event 0x5f should be accompanied by
>> another hex constant. And it would also help if in the description
>> you said where these bounds
>
> From talking to people at AMD.
>
>> as well as ...
>>
>>> --- a/xen/arch/x86/cpu/hygon.c
>>> +++ b/xen/arch/x86/cpu/hygon.c
>>> @@ -34,6 +34,11 @@ static void init_hygon(struct cpuinfo_x86 *c)
>>>
>>> amd_init_lfence(c);
>>>
>>> + /* Probe for NSCB on Zen2 CPUs when not virtualised */
>>> + if (!cpu_has_hypervisor && !cpu_has_nscb && c == &boot_cpu_data &&
>>> + c->x86 == 0x18 && c->x86_model >= 4)
>> ... this one come from.
>
> From talking to people at Hygon.
Fair enough, but imo this wants mentioning in the description.
Jan
