Re: [Xen-devel] [PATCH] x86/shim: Short circuit control/hardware checks in PV_SHIM_EXCLUSIVE builds

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.01.2020 18:16, Andrew Cooper wrote:
> On 06/01/2020 10:13, Jan Beulich wrote:
>> On 03.01.2020 21:07, Andrew Cooper wrote:
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -963,10 +963,22 @@ void watchdog_domain_destroy(struct domain *d);
>>>   *    (that is, this would not be suitable for a driver domain)
>>>   *  - There is never a reason to deny the hardware domain access to this
>>>   */
>>> -#define is_hardware_domain(_d) evaluate_nospec((_d) == hardware_domain)
>>> +static always_inline bool is_hardware_domain(const struct domain *d)
>>> +{
>>> +    if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
>>> +        return false;
>>> +
>>> +    return evaluate_nospec(d == hardware_domain);
>>> +}
>>>  
>>>  /* This check is for functionality specific to a control domain */
>>> -#define is_control_domain(_d) evaluate_nospec((_d)->is_privileged)
>>> +static always_inline bool is_control_domain(const struct domain *d)
>>> +{
>>> +    if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
>>> +        return false;
>>> +
>>> +    return evaluate_nospec(d->is_privileged);
>>> +}
>> Besides its intended purpose this also fixes (but only for the
>> shim-exclusive case) an interaction issue with LATE_HWDOM: If in
>> shim mode the "hardware_dom=1" command line option was used,
>> misbehavior would result afaict.
> 
> Perhaps, but there are plenty of other ways to break things using the
> shims command line.
> 
> Remember that the shim command line is not under user control at all.
> 
>> Therefore I think this wants
>> amending with adjustments to also make the !PV_SHIM_EXCLUSIVE
>> case work correctly (read: ignore that command line option). I
>> guess additionally LATE_HWDOM should also depend on
>> !PV_SHIM_EXCLUSIVE (and/or vice versa).
> 
> No - such a bugfix should be a separate change, because it is not
> related to this patch.
> 
> Fixing it would require extra x86 #ifdef-ary in common code.  While this
> is doable, there is also work in progress from the OpenXT folk to
> completely overhaul how that mechanism works (which will in practice
> remove the command line parameter).
> 
> Given both of these aspects, I'm tempted to leave it as-is for now.

Okay, yet may I ask that you mention the partial bug fix in the
description?

Jan

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