Re: [PATCH] replay: fix replay of the interrupts

[Claudio Fontana]

On 1/25/21 3:26 PM, Alex Bennée wrote:
> 
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> On 1/25/21 1:43 PM, Claudio Fontana wrote:
>>> On 1/25/21 1:12 PM, Alex Bennée wrote:
>>>>
>>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>>
>>>>> In general I agree, but != means that rr disabled returns true. In general
>>>>> it seems to me that rr disabled should work more or less the same as 
>>>>> record
>>>>> mode, because there is no replay log to provide the checkpoints.
>>>>
>>>> Is this not an argument to combine the mode and check into replay.h
>>>> inline helpers with some clear semantic documentation and the call sites
>>>> become self documenting?
>>>>
>>>> if (deadline == 0 && replay_recording_or_checkpoint())
>>>>
>>>> which also makes things easier to compile away if replay isn't there?
>>>
>>>
>>> Seems that the TCG build faces a similar issue to the issue I was facing 
>>> with the non-TCG build,
>>> before the non-TCG build got functional again (for x86).
>>>
>>> We solved the non-TCG build problem, by not compiling replay at all for 
>>> non-TCG, plus closing our nose and stubbing away what couldn't be 
>>> completely removed (yet).
>>>
>>> But the CONFIG_TCG build has the same legitimate requirement towards a 
>>> non-CONFIG_REPLAY build.
>>>
>>> ie, like we have tcg_enabled(), should we have replay_enabled()? Maybe it 
>>> could be reworked starting from replay_events_enabled()?
>>>
>>> And then when things are refactored properly for replay_enabled(), a 
>>> non-REPLAY TCG build can basically ignore all the inner workings of replay.
>>>
>>
>> I guess to summarize the above, should there be a CONFIG_REPLAY, dependent 
>> on CONFIG_TCG, by default on,
>> but which could be disabled with
>>
>> --disable-replay
>>
>> ?
> 
> I'm not sure - fundamentally having replay is one of those cool things
> you can do when you have TCG and I suspect there is less pressure to
> have a finely tuned TCG enabled build compared to our HW accelerated
> brethren using hypervisors. TCG plugins are a configure option purely
> because there is a small but non-marginal cost in having it enabled. I
> doubt replay figures that much if you are not using it.
> 
> That said if it makes it easier to make sure our abstractions are clean
> and the layering is good then maybe it is worth having a build that

I think that cleaning up these aspects would be beneficial in replay itself,
but maybe this could be done without forcing the --disable-replay option.

> allows us to check that. But if it's going to be super fiddly and
> involve large amounts of code motion I doubt the "win" is big enough for
> the effort.
> 
> Also I suspect the config option would be CONFIG_ICOUNT because replay
> is just one of the features on the spectrum of:
> 
>  deterministic icount -> record/replay -> reverse debugging
> 
> which all require the base support for icount.
> 

Right, icount_enabled() is already fundamentally able to check whether replay 
functionality is available or not.
Probably there is already some cleanup possible by applying this consistently.

Other clean up opportunities I see are in making consistent checks for whether 
the functionality is active, and consolidate all the spread state,
including (but not limited to):

replay_mode, events_enabled, replay_is_debugging, in_checkpoint, ... (into a 
single struct replay ?)

..and then I guess clean up the namespace from all the replay_ functions for 
which we need a gazillion stubs for non-TCG code..


Ciao,

Claudio