Re: [Xen-devel] [PATCH] xen: Plumb an is_priv boolean into domain_create()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/07/18 10:57, Jan Beulich wrote:
>>>> On 02.07.18 at 11:44, <andrew.cooper3@xxxxxxxxxx> wrote:
>> The current mechanism of setting dom0->is_privileged after construction means
>> that the is_control_domain() predicate returns false during construction.
>>
>> In particular, this means that the CPUID Faulting special case in
>> init_domain_msr_policy() fails to take effect.  (In actual fact, faulting
>> support is advertised to dom0, but attempting to configure it is silently
>> ignored because of the dom0 special case in ctxt_switch_levelling().)
>>
>> This could be implemented using a flag in xen_domctl_createdomain, but using
>> an extra boolean parameter like this means that we can't accidentally allow
>> domain_create() to create a second dom0 due to parameter mis-auditing.
>>
>> While adjusting the setting of dom0->is_privileged, drop the redundant 
>> zeroing
>> of dom0->target.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Julien Grall <julien.grall@xxxxxxx>
>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
>>
>> Compile tested on ARM.  Functionally tested on x86.
>>
>> This ideally wants backporting, but will probably be prohibitive beyond 4.11
>>
>> Semi RFC because I'm about to fix the reason for needing the dom0 special 
>> case
>> for Faulting, and avoid the bug that way.  OTOH, is_control_domain() ought to
>> work sensibly.  Thoughts?
> I agree.
>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.  In terms of backports, it needs to go back to 4.10
(specifically because of c/s 4098b092), but backporting over my
reworking of the domain_create() interface might be interesting.

>
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -271,7 +271,7 @@ void __init arch_init_memory(void)
>>       * Hidden PCI devices will also be associated with this domain
>>       * (but be [partly] controlled by Dom0 nevertheless).
>>       */
>> -    dom_xen = domain_create(DOMID_XEN, NULL);
>> +    dom_xen = domain_create(DOMID_XEN, NULL, false);
> I wonder whether this would (perhaps not right in this patch) better
> pass true.

The overall users of is_control_domain() / is_privileged are:
 * CPUID Faulting special case for PV domains (to disappear eventually)
 * max_order() which affects various XENMEM_* hypercalls
 * Anything of type XSM_PRIV

None of these are relevant to the system domains, so for now I'd suggest
that we don't alter the behaviour here.  It is however certainly an
option if we need it in the future.

~Andrew

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