On Sun, Oct 21, 2018 at 14:27:22 +0100, Richard Henderson wrote:
> On 10/19/18 2:05 AM, Emilio G. Cota wrote:
> > @@ -713,9 +713,9 @@ int hvf_vcpu_exec(CPUState *cpu)
> > switch (exit_reason) {
> > case EXIT_REASON_HLT: {
> > macvm_set_rip(cpu, rip + ins_len);
> > - if (!((cpu->interrupt_request & CPU_INTERRUPT_HARD) &&
> > + if (!((cpu_interrupt_request(cpu) & CPU_INTERRUPT_HARD) &&
> > (EFLAGS(env) & IF_MASK))
> > - && !(cpu->interrupt_request & CPU_INTERRUPT_NMI) &&
> > + && !(cpu_interrupt_request(cpu) & CPU_INTERRUPT_NMI) &&
> > !(idtvec_info & VMCS_IDT_VEC_VALID)) {
> > cpu_halted_set(cpu, 1);
> > ret = EXCP_HLT;
>
> Likewise wrt multiple calls.
>
> > @@ -400,7 +401,8 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
> > };
> > }
> >
> > - if (cpu_state->interrupt_request & CPU_INTERRUPT_NMI) {
> > + cpu_mutex_lock(cpu_state);
> > + if (cpu_interrupt_request(cpu_state) & CPU_INTERRUPT_NMI) {
> > if (!(env->hflags2 & HF2_NMI_MASK) && !(info & VMCS_INTR_VALID)) {
> > cpu_reset_interrupt(cpu_state, CPU_INTERRUPT_NMI);
> > info = VMCS_INTR_VALID | VMCS_INTR_T_NMI | NMI_VEC;
> > @@ -411,7 +413,7 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
> > }
> >
> > if (!(env->hflags & HF_INHIBIT_IRQ_MASK) &&
> > - (cpu_state->interrupt_request & CPU_INTERRUPT_HARD) &&
> > + (cpu_interrupt_request(cpu_state) & CPU_INTERRUPT_HARD) &&
> > (EFLAGS(env) & IF_MASK) && !(info & VMCS_INTR_VALID)) {
> > int line = cpu_get_pic_interrupt(&x86cpu->env);
> > cpu_reset_interrupt(cpu_state, CPU_INTERRUPT_HARD);
>
> Likewise.
>
> I think you need to be more careful about this in the conversions.
> Previously,
> the compiler would CSE these two loads; now you're taking a lock twice.
>
> Or in the second instance, once, since you explicitly take the lock around a
> big block. But I think that's papering over the fact that you make 4 calls
> when you should have made one, *and* not hold the lock across all that code.
Yes, I'm aware of this. For a first pass I wanted to make sure no updates
would be lost, e.g.
int interrupt_request = cpu_interrupt_request(cpu);
if (interrupt_request & FOO) {
do_foo(); /* sets cpu->interrupt_request | BAR */
}
if (interrupt_request & BAR) { /* wrongly misses BAR update */
do_bar();
}
I'll go through the entire patch to amend these.
Thanks,
E.
