Re: [PATCH 2/2] hvmloader: Allow the toolstack to choose WAET optimisations

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 27/11/13 12:56, Jan Beulich wrote:
>>>> On 27.11.13 at 13:14, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 27/11/13 10:03, Jan Beulich wrote:
>>>>>> On 26.11.13 at 21:39, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> @@ -196,8 +201,35 @@ static struct acpi_20_waet *construct_waet(void)
>>>>      memcpy(waet, &Waet, sizeof(*waet));
>>>>  
>>>>      waet->header.length = sizeof(*waet);
>>>> +
>>>> +    s = xenstore_read("platform/waet-rtc-noack", NULL);
>>>> +    if ( s )
>>>> +    {
>>>> +        if ( !strncmp(s, "1", 1) || !strncmp(s, "true", 4) )
>>>> +            waet->flags |= ACPI_WAET_RTC_NO_ACK;
>>>> +        else
>>>> +            waet->flags &= ~ACPI_WAET_RTC_NO_ACK;
>>>> +    }
>>>> +
>>>> +    s = xenstore_read("platform/waet-pm-reliable", NULL);
>>>> +    if ( s )
>>>> +    {
>>>> +        if ( !strncmp(s, "1", 1) || !strncmp(s, "true", 4) )
>>>> +            waet->flags |= ACPI_WAET_TIMER_ONE_READ;
>>>> +        else
>>>> +            waet->flags &= ~ACPI_WAET_TIMER_ONE_READ;
>>>> +    }
>>> Nothing in this series really allows these to be easily controlled on a
>>> per-domain basis - I would have expected a config file flag...
>>>
>>> And for this second one, as hinted at in your overview mail, I
>>> don't really see what purpose the controlling here serves: You
>>> don't consume the setting, i.e. the sole effect is that of
>>> controlling the ACPI table's flag value. Yet if we're fine with
>>> the guest doing single reads (of whatever), then we're surely
>>> fine too with it doing double reads. And hence the flag can
>>> easily be always set (as it was till now).
>>>
>>> If any second override was to be considered, I'd recommend
>>> on controlling the presence of the WAET as a whole.
>> The setting is consumed using
>>
>> +    /* Inform Xen which RTC mode has been chosen */
>> +    p.value = !!(waet->flags & ACPI_WAET_RTC_NO_ACK);
>> +    hypercall_hvm_op(HVMOP_set_param, &p);
> Note that I said "this second one", i.e. referring to
> ACPI_WAET_TIMER_ONE_READ.
>
> Jan
>

Ah apologies - I missed that.  I did bring that up in the covering
letter as to whether it was worth having an option for it in the first
place.

~Andrew