Re: [PATCH v3 1/2] x86: restore pv_rtc_handler() invocation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.07.2020 12:31, Roger Pau Monné wrote:
> On Thu, Jul 16, 2020 at 12:06:14PM +0200, Jan Beulich wrote:
>> On 15.07.2020 16:51, Roger Pau Monné wrote:
>>> On Wed, Jul 15, 2020 at 03:51:17PM +0200, Jan Beulich wrote:
>>>> On 15.07.2020 15:32, Roger Pau Monné wrote:
>>>>> Feel free to change to ACCESS_ONCE or barrier if you think it's
>>>>> clearer.
>>>>
>>>> I did so (also on the writer side), not the least based on guessing
>>>> what Andrew would presumably have preferred.
>>>
>>> Thanks! Sorry I might be pedantic, but is the ACCESS_ONCE on the write
>>> side actually required? I'm not sure I see what ACCESS_ONCE protects
>>> against in handle_rtc_once.
>>
>> Well, this is all sort of a mess, I think. We have this mixture of
>> ACCESS_ONCE() and read_atomic() / write_atomic(), but I don't think
>> we use them consistently, and I'm not sure either is suitable to
>> deal with all (theoretical) corner cases.
>>
>> read_atomic() / write_atomic() guarantee a single insn to be used
>> to access a piece of data. I'm uncertain whether they also guarantee
>> single access (i.e. that the compiler won't replicate the asm()-s).
> 
> Yes, that would be my expectation from my reading of the manual, as
> it prevents gcc from: "move it out of loops or omit it on the
> assumption that the result from a previous call is still valid".
> 
>> The wording in gcc doc is pretty precise, but not quite enough imo
>> to be entirely certain.
> 
> I agree it's not that precise.
> 
>> ACCESS_ONCE() guarantees single access, but doesn't guarantee that
>> the compiler wouldn't split this single access into multiple insns.
>> (It's just, like elsewhere, that it would be pretty silly of it if
>> it did.)
>>
>> Yesterday, as said, I tried to in particular do what I expect/guess
>> Andrew would have wanted done. This is despite me not being entirely
>> convinced this is the right thing to do here, i.e. personally I
>> would have preferred read_atomic() / write_atomic(), as I think the
>> intention of what the gcc doc is saying is what we want (taking
>> into consideration both uses of "volatile" in these helpers).
> 
> Well, gcc states:
> 
> "Note that the compiler can move even volatile asm instructions
> relative to other code, including across jump instructions."
> 
> So I think we likely want to use {read/write}_atomic plus a compiler
> barrier? I'm not sure anyway how the read of pv_rtc_handler could be
> moved, but I guess I'm not that creative :).
> 
> AFAICT we require a write_atomic in handle_rtc_once in order to assure
> a single instruction is used (no barrier required), and then we
> require a read_atomic + a compiler barrier in rtc_guest_write in order
> to prevent the compiler from optimizing the accesses to 'hook' in any
> way? (that barrier might not be strictly required, as you say it's not
> fully clear whether 'asm volatile' doesn't provide the necessary
> protection here).

Yes, but I'd really like to wait for Andrew's input here before we
make any further adjustments. In any even what has gone in yesterday
is already better than what the original code was that needed
restoring.

Jan