Re: [PATCH v2 06/19] x86/pv: Implement pv_inject_{event, page_fault, hw_exception}()

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>, Xen-devel <xen-devel@lists.xen.org>
Subject: Re: [PATCH v2 06/19] x86/pv: Implement pv_inject_{event, page_fault, hw_exception}()
Date: Tue, 29 Nov 2016 16:50:52 +0000	[thread overview]
Message-ID: <227bbeb2-4117-bef6-2683-8a3b5cb02c89@citrix.com> (raw)
In-Reply-To: <583DB41102000078001235A2@prv-mh.provo.novell.com>

On 29/11/16 16:00, Jan Beulich wrote:
>>>> On 28.11.16 at 12:13, <andrew.cooper3@citrix.com> wrote:
>> The existing propagate_page_fault() is fairly similar to
>> pv_inject_page_fault(), although it has a return value.  Only a single caller
>> makes use of the return value, and non-NULL is only returned if the passed cr2
>> is non-canonical.  Opencode this single case in
>> handle_gdt_ldt_mapping_fault(), allowing propagate_page_fault() to become
>> void.
> I can only say that back then it was quite intentional to not open
> code this in the caller, no matter that it was (and still is) just one.

Is that an objection to me making this change?

>
>>      if ( unlikely(null_trap_bounce(v, tb)) )
>> -        gprintk(XENLOG_WARNING,
>> -                "Unhandled %s fault/trap [#%d, ec=%04x]\n",
>> -                trapstr(trapnr), trapnr, regs->error_code);
>> +    {
>> +        if ( vector == TRAP_page_fault )
>> +        {
>> +            printk("%pv: unhandled page fault (ec=%04X)\n", v, error_code);
>> +            show_page_walk(event->cr2);
>> +
>> +            if ( unlikely(error_code & PFEC_reserved_bit) )
>> +                reserved_bit_page_fault(event->cr2, regs);
> I think you want to move the show_page_walk() into an else here,
> to avoid logging two of them. But then again - why do you move
> this behind the null_trap_bounce() check? It had been logged
> independently in the original code, and for (imo) a good reason
> (reserved bit faults are always a sign of a hypervisor problem
> after all).

TBH, I found it odd that it was in propagate_page_fault() to start with.

It is the kind of thing which should be in the pagefault handler itself,
not in the reinjection-to-pv-guests code.

Would moving it into fixup_page_fault() be ok?  It should probably go
between the hap/shadow and memadd checks, as shadow guests can have
reserved bits set.

>
>> +        }
>> +        else
>> +            gprintk(XENLOG_WARNING,
>> +                    "Unhandled %s fault/trap [#%d, ec=%04x]\n",
>> +                    trapstr(vector), vector, error_code);
> Which tells us that we need to finally get our log level handling
> straightened, to avoid inconsistencies like the one here comparing
> with the code a few lines up.

Yes.  I chose not to unpick that swamp right now, but if
reserved_bit_page_fault() gets moved out, this bit can be simplified to
this single gprintk().

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel