Re: [Xen-devel] [PATCH v2 3/6] xen/x86: support per-domain flag for xpti
On 08/03/18 13:49, Jan Beulich wrote:
On 08.03.18 at 12:30, wrote:
>> On 08/03/18 11:17, Jan Beulich wrote:
>> On 02.03.18 at 09:14, wrote:
+static int parse_xpti(const char *s)
+{
+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.
>
> I'm not sure that's the best route (and I did intentionally not ask for
> you doing so): In cases where you can't edit the pre-built command
> line options (like e.g. those read by xen.efi from the config file), it
> is quite useful to be able to override what may be there back to the
> default.
Okay, then I'll document it.
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 3/6] xen/x86: support per-domain flag for xpti
>>> On 08.03.18 at 12:30, wrote:
> On 08/03/18 11:17, Jan Beulich wrote:
> On 02.03.18 at 09:14, wrote:
>>> +static int parse_xpti(const char *s)
>>> +{
>>> +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.
I'm not sure that's the best route (and I did intentionally not ask for
you doing so): In cases where you can't edit the pre-built command
line options (like e.g. those read by xen.efi from the config file), it
is quite useful to be able to override what may be there back to the
default.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 3/6] xen/x86: support per-domain flag for xpti
On 08/03/18 11:17, Jan Beulich wrote:
On 02.03.18 at 09:14, 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
Re: [Xen-devel] [PATCH v2 3/6] xen/x86: support per-domain flag for xpti
>>> On 02.03.18 at 09:14, 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
