Re: XenVif div by zero on Tx path after resume.

["Durrant, Paul" <xadimgnik@xxxxxxxxx>]

On 22/04/2022 15:17, Martin Harvey wrote:
> -----Original Message-----
> From: Durrant, Paul <xadimgnik@xxxxxxxxx>
> Sent: 22 April 2022 15:13
> To: Martin Harvey <martin.harvey@xxxxxxxxxx>; paul@xxxxxxx; 
> win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: XenVif div by zero on Tx path after resume.
>
>
> > ... and unfortunately it seems to have been broken for so long my git history 
> > won't tell me when :-(
> Oh dear. Possibly one of those "how did it ever work?" bugs.
>
> If you need testing resource / validation for any patches you'd like to throw 
> my way, I do have a v. good set of automates test cases which will repro the 
> bug (after about 12 hours of CPU grinding), and check that most obvious things 
> aren't broken.
>
> IRQL is such a nightmare: You can get it wrong in one place and the subtly 
> broken CPU state then dies in the next piece of code that needs some level of 
> atomicity.
OK, it was my misremembering of KeLowerIrql() semantics. The code looks 
to be doing the right thing.
SuspendTrigger() raises to DISPATCH.
It then calls SyncCapture() which schedules a DPC on all other 
processors which spins.
It then calls SyncDisableInterrupts() which instructs the worker DPCs to 
raise to HIGH, and then raises the current CPU to HIGH. There is a 
slightly subtle loop with a back-off to DISPATCH that waits for all CPUs 
to get to HIGH. The back-off is needed in case of mutual IPI. Once every 
CPU is at HIGH, they all disable interrupts.
Now the SCHEDOP is done to either suspend or migrate. When this returns 
we are resuming. There is a slight subtlety here. The SCHEDOP may 
'fail', which means we are doing a 'fast resume'. A fast resume is done 
on the source host. This means all backends are still there and thus we 
do not need to run the suspend callbacks.
SuspendTrigger() now calls SyncEnableInterrupts(). This runs the 'early' 
suspend callbacks on each CPU and drops all the CPUs back to DISPATCH. 
Hence interrupts are now possible, but normal threads are still 
prevented from being scheduled.
It then calls SyncRelease(). This runs the 'late' suspend callbacks and 
terminates the DPCs on other CPUs, thus allowing threads to be 
scheduled. It then lowers IRQL on the current CPU, which should mean a 
fall back to PASSIVE as SuspendTrigger() is only ever called from the 
dedicated thread (running FdoSuspend()).
So, there should be no way a thread can run before all 'late' callbacks 
have run unless there is a kernel call being made at the wrong IRQL or 
we need extra barriering. Hopefully the former could be caught in verifier.
  Paul