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

[Juergen Gross <jgross@suse.com>]

On 08/03/18 11:17, Jan Beulich wrote:
>>>> 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.

Yes.

> 
>> @@ -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?

I'll drop the conditional.

> 
>> +static int parse_xpti(const char *s)
> 
> __init

Aah, of course.

> 
>> +{
>> +    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.

Uuh, this was a copy-and-paste result from my alternative XPTI approach.
I'll just drop that value.

> 
>> +            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.

Okay.

> 
>> +void xpti_init(void)
> 
> __init

Yes.

> 
>> +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.

Okay, I'll drop that message.

> 
>> --- 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?

I'll have a look.


Juergen

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