From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH v3 7/9] x86: Enable Supervisor Mode Access
 Prevention (SMAP) for Xen
Date: Mon, 28 Apr 2014 10:48:15 +0100
Message-ID: <535E23DF.3090903@citrix.com>
References: <1398655043-6162-1-git-send-email-feng.wu@intel.com>
	<535E3E45020000780000CC51@nat28.tlf.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <535E3E45020000780000CC51@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: kevin.tian@intel.com, Feng Wu <feng.wu@intel.com>, ian.campbell@citrix.com, eddie.dong@intel.com, xen-devel@lists.xen.org, jun.nakajima@intel.com
List-Id: xen-devel@lists.xenproject.org

On 28/04/14 10:40, Jan Beulich wrote:
>>>> On 28.04.14 at 05:17, <feng.wu@intel.com> wrote:
>> @@ -1394,6 +1398,15 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>                          bootstrap_map, cmdline) != 0)
>>          panic("Could not set up DOM0 guest OS");
>>  
>> +    /*
>> +     * Enable SMAP after constructing domain0, since there are lots of accesses to
>> +     * user pages in construct_dom0(), which is safe at the current stage.
>> +     */
>> +    if ( disable_smap )
>> +        setup_clear_cpu_cap(X86_FEATURE_SMAP);
> You should not have moved this part - this should happen before APs
> get brought up.
>
>> @@ -1379,8 +1399,14 @@ void do_page_fault(struct cpu_user_regs *regs)
>>  
>>      if ( unlikely(!guest_mode(regs)) )
>>      {
>> -        pf_type = spurious_page_fault(addr, error_code);
>> -        BUG_ON(pf_type == smep_fault);
>> +        pf_type = spurious_page_fault(addr, regs);
>> +        if ( (pf_type == smep_fault) || (pf_type == smap_fault) )
>> +        {
>> +            console_start_sync();
>> +            printk("Xen %s violation\n", (pf_type == smep_fault) ? "SMEP" : "SMAP");
> I know it's largely a matter of taste, but could I talk you into doing
>
>             printk("Xen SM%cP violation\n", (pf_type == smep_fault) ? 'E' : 'A');
>
> instead (unless others object)?

I concur.  Neither string is grepable.

>
>> @@ -1406,10 +1432,12 @@ void do_page_fault(struct cpu_user_regs *regs)
>>  
>>      if ( unlikely(current->domain->arch.suppress_spurious_page_faults) )
>>      {
>> -        pf_type = spurious_page_fault(addr, error_code);
>> -        if ( pf_type == smep_fault )
>> +        pf_type = spurious_page_fault(addr, regs);
>> +        if ( (pf_type == smep_fault) || (pf_type == smap_fault))
>>          {
>> -            gdprintk(XENLOG_ERR, "Fatal SMEP fault\n");
>> +            printk(XENLOG_G_ERR "%p fatal %s fault\n",
>> +                   current, (pf_type == smep_fault) ? "SMEP" : "SMAP");

That should be %pv rather than %p, which is an automagic format
parameter which will get you "d<domid>v<vcpuid>" formatted.

> And similarly here then?
>
> Jan
>

~Andrew