Re: [PATCH v2 3/6] xen/x86: support per-domain flag for xpti

["Jan Beulich" <JBeulich@suse.com>]

>>> On 02.03.18 at 09:14, <jgross@suse.com> wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -510,15 +510,19 @@ void make_cr3(struct vcpu *v, mfn_t mfn)
>  
>  void write_ptbase(struct vcpu *v)
>  {
> -    if ( this_cpu(root_pgt) && is_pv_vcpu(v) && !is_pv_32bit_vcpu(v) )
> +    if ( is_pv_vcpu(v) && v->domain->arch.pv_domain.xpti )
>      {
>          get_cpu_info()->root_pgt_changed = true;
> +        get_cpu_info()->pv_cr3 = __pa(this_cpu(root_pgt));
>          asm volatile ( "mov %0, %%cr3" : : "r" (v->arch.cr3) : "memory" );
>      }
>      else
>      {
>          get_cpu_info()->root_pgt_changed = false;
> +        /* Make sure to clear xen_cr3 before pv_cr3; write_cr3() serializes. */
> +        get_cpu_info()->xen_cr3 = 0;
>          write_cr3(v->arch.cr3);
> +        get_cpu_info()->pv_cr3 = 0;
>      }
>  }

I think you want to latch the return value of get_cpu_info() into a
local variable now.

> @@ -707,6 +708,9 @@ int __init dom0_construct_pv(struct domain *d,
>              cpu = p->processor;
>      }
>  
> +    if ( !is_pv_32bit_domain(d) )
> +        xpti_domain_init(d);

Perhaps better to omit the conditional here? Or otherwise use the
"compat32" local variable?

> +static int parse_xpti(const char *s)

__init

> +{
> +    int rc = 0;
> +
> +    switch ( parse_bool(s, NULL) )
> +    {
> +    case 0:
> +        opt_xpti = XPTI_OFF;
> +        break;
> +    case 1:
> +        opt_xpti = XPTI_ON;
> +        break;
> +    default:
> +        if ( !strcmp(s, "default") )

This wants to also be mentioned in the command line doc.

> +            opt_xpti = XPTI_DEFAULT;
> +        else if ( !strcmp(s, "nodom0") )
> +            opt_xpti = XPTI_NODOM0;
> +        else
> +            rc = -EINVAL;
> +        break;
> +    }
> +
> +    return rc;
> +}
> +
> +custom_param("xpti", parse_xpti);

Please omit the blank line above here.

> +void xpti_init(void)

__init

> +void xpti_domain_init(struct domain *d)
> +{
> +    if ( !is_pv_domain(d) || is_pv_32bit_domain(d) )
> +        return;
> +
> +    switch ( opt_xpti )
> +    {
> +    case XPTI_OFF:
> +        d->arch.pv_domain.xpti = false;
> +        break;
> +    case XPTI_ON:
> +        d->arch.pv_domain.xpti = true;
> +        break;
> +    case XPTI_NODOM0:
> +        d->arch.pv_domain.xpti = d->domain_id != 0 &&
> +                                 d->domain_id != hardware_domid;
> +        break;
> +    default:
> +        ASSERT_UNREACHABLE();
> +        break;
> +    }
> +
> +    if ( d->arch.pv_domain.xpti )
> +        printk("Enabling Xen Pagetable protection (XPTI) for Domain %d\n",
> +               d->domain_id);

Please don't, even less so without XENLOG_G_*. And if you really,
really want this at, say, XENLOG_G_DEBUG, then Dom%d please.

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -257,6 +257,9 @@ struct pv_domain
>      struct mapcache_domain mapcache;
>  
>      struct cpuidmasks *cpuidmasks;
> +
> +    /* XPTI active? */
> +    bool xpti;
>  };

Is there really no 1 byte slot available elsewhere in the structure?
Like between nr_l4_pages and mapcache?

Jan


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