Re: [Xen-devel] [PATCH] x86/shadow: Drop incorrect diagnostic when shadowing TSS.RSP0

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.04.19 at 21:09, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -3305,8 +3305,9 @@ static int sh_page_fault(struct vcpu *v,
>      {
>          /*
>           * If we are in the middle of injecting an exception or interrupt 
> then
> -         * we should not emulate: it is not the instruction at %eip that 
> caused
> -         * the fault. Furthermore it is almost certainly the case the handler
> +         * we should not emulate: the fault is a side effect of the processor
> +         * trying to push an exception frame onto a stack which has yet to be
> +         * shadowed.  Furthermore it is almost certainly the case the handler
>           * stack is currently considered to be a page table, so we should
>           * unshadow the faulting page before exiting.
>           */

Your addition to me looks to contradict the part of the comment you
leave in place: You say "which has yet to be shadowed", while the
pre-existing text says "it is almost certainly the case the handler
stack is currently considered to be a page table", which to me means
it _is_ already shadowed (and in fact should not be).

In your addition, do you perhaps mean the page tables covering the
stack which have yet to be shadowed?

> @@ -3319,9 +3320,6 @@ static int sh_page_fault(struct vcpu *v,
>                  v->arch.paging.last_write_emul_ok = 0;
>              }
>  #endif
> -            gdprintk(XENLOG_DEBUG, "write to pagetable during event "
> -                     "injection: cr2=%#lx, mfn=%#lx\n",
> -                     va, mfn_x(gmfn));
>              sh_remove_shadows(d, gmfn, 0 /* thorough */, 1 /* must succeed 
> */);

The "almost certainly" above makes me wonder whether it wouldn't
be better to leave the message in place, but put it under some
conditional. A perhaps too simplistic initial thought of mine was to
issue it in case sh_remove_shadows() ended up crashing the
guest. Considering sh_mfn_is_a_page_table(gmfn)'s return value
might be another option.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel