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

[Eric Shelton <eshelton@xxxxxxxxx>]

On Mon, Sep 22, 2014 at 2:21 PM, Don Slutz <dslutz@xxxxxxxxxxx> wrote:
> On 09/17/14 00:30, Eric Shelton wrote:
>>
>> On 15.09.14 at 08:42, Jan Beulich wrote:
>> >>>> On 12.09.14 at 19:46, <dslutz@xxxxxxxxxxx
>> >>>> <mailto:dslutz@xxxxxxxxxxx>> wrote:
>> >> On 09/12/14 05:49, Jan Beulich wrote:
>> >>>>>> On 11.09.14 at 21:49, <andrew.cooper3@xxxxxxxxxx
>> >>>>>> <mailto:andrew.cooper3@xxxxxxxxxx>> wrote:
>> >>>>> +    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".
>> >>
>> >> I am totally confused.  I am happy to go with Jan's version.
>> >>
>> >> The confusion is that I get the same answer all the ways I try.
>> >
>> > Hmm - this ...
>> >
>> >>        ebx1 = 1000000000ull / APIC_BUS_CYCLE_NS / 1000ull;
>> >>        ebx2 = (1000000000ull / APIC_BUS_CYCLE_NS) / 1000ull;
>> >>        ebx3 = 1000000000ull / (APIC_BUS_CYCLE_NS * 1000ull);
>> >
>> > ... clearly indicates the contrary: You converted to mutiplication
>> >here, when the respective possibility of putting parentheses here
>> >would have been
>> >
>> > ebx3 = 1000000000ull / (APIC_BUS_CYCLE_NS / 1000ull);
>> >
>> >which I'm sure you agree won't produce the same result. But yes,
>> >the language implies parentheses this way
>> >
>> > ebx2 = (1000000000ull / APIC_BUS_CYCLE_NS) / 1000ull;
>> >
>> >so the original expression without them was correct, just
>> >slightly ambiguous.
>> >
>> >Jan
>>
>> A different approach for the bus frequency, taken in the patch I just
>> posted, is to provide the actual bus frequency, rather than a hardcoded
>> value.  Specifically, the value bus_freq in apic.c.
>>
>
> This is not the correct bus frequency for the vlapic code.
>
> I have a server where the host:
> ...
> (XEN) ..... host bus clock speed is 200.0108 MHz.
>
> But the guest sees:
> ...
> ..... host bus clock speed is 99.0999 MHz.
>
> (do to a bug in Linux, this is not displayed correctly, the real value is
> 99.999 MHz
> because "CONFIG_HZ=1000" not 100)
>
> I also see:
> ..... host bus clock speed is 100.0000 MHz.
>
>
> So reporting the 200.0108 to the guest would be wrong.
>
> (And on a different server I have:
>
> (XEN) ..... host bus clock speed is 100.0010 MHz.
>
> for the host; which is closer but still not right).
>
>    -Don Slutz

Thank you.  I see I overlooked this, and should have simply used the
vlapic frequency, which is essentially what was already done in the
veridian code.

- Eric

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