Re: [Xen-devel] [PATCH v3 7/9] x86: Enable Supervisor Mode Access Prevention (SMAP) for Xen

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: Monday, April 28, 2014 5:48 PM
> To: Jan Beulich
> Cc: Wu, Feng; ian.campbell@xxxxxxxxxx; Dong, Eddie; Nakajima, Jun; Tian,
> Kevin; xen-devel@xxxxxxxxxxxxx
> Subject: Re: [PATCH v3 7/9] x86: Enable Supervisor Mode Access Prevention
> (SMAP) for Xen
> 
> On 28/04/14 10:40, Jan Beulich wrote:
> >>>> On 28.04.14 at 05:17, <feng.wu@xxxxxxxxx> 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.

Thanks Andrew, I thought it was a typo in your last review:)

> 
> > And similarly here then?
> >
> > Jan
> >
> 
> ~Andrew

Thanks,
Feng

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