Re: [PATCH v4 01/16] xen: Add support for VMware cpuid leaves

["Jan Beulich" <JBeulich@suse.com>]

>>> On 11.09.14 at 21:49, <andrew.cooper3@citrix.com> wrote:
> I am not sure how "fixing things correctly in Xen" fairs against "libxl
> taking pain and possibly an API breakage because it previously exposed
> internal details which it shouldn't have done", but I would prefer that
> we didn't make the problem any harder to fix than it already is.
> 
> As a result, I am formally suggesting that this would be better done by
> adding a domain creation flag (although not being a maintainer, I
> realise my views in this matter don't strictly count for much).

The main reservation I have against this is that this, in the long run,
may result in a proliferation of VM creation flags. The slightly abused
HVM params at least make adding support of another kind of foreign
VMM emulation a pretty straightforward and isolated change.

>> +    case 0x10:
>> +        /* (Virtual) TSC frequency in kHz. */
>> +        *eax =  d->arch.tsc_khz;
>> +        /* (Virtual) Bus (local apic timer) frequency in kHz. */
>> +        *ebx = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
> 
> At least 1 pair of brackets please, especially as the placement of
> brackets affects the result of this particular calculation.

Or simply eliminate one of the divisions using
"1000000ull / APIC_BUS_CYCLE_NS".

>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -685,8 +685,12 @@ int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
>>                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
>>  {
>>      struct domain *d = current->domain;
>> -    /* Optionally shift out of the way of Viridian architectural leaves. */
>> -    uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
>> +    /*
>> +     * Optionally shift out of the way of Viridian or VMware
>> +     * architectural leaves.
>> +     */
>> +    uint32_t base = is_viridian_domain(d) | is_vmware_domain(d) ?
>> +        0x40000100 : 0x40000000;
> 
> Again, brackets please for binary operators.  (We have had one recent
> slipup because of the precedence of | and ?:)

I don't think parentheses are strictly required here - we're not very
consistent with when to disambiguate operator precedence by using
them when not strictly needed anyway, and I think people touching
hypervisor code can be expected to know the language specification
well enough.

Jan