All of lore.kernel.org
 help / color / mirror / Atom feed
* [Xen-devel] [PATCH] x86/shim: Short circuit control/hardware checks in PV_SHIM_EXCLUSIVE builds
@ 2020-01-03 20:07 Andrew Cooper
  2020-01-05 16:30 ` Wei Liu
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Andrew Cooper @ 2020-01-03 20:07 UTC (permalink / raw)
  To: Xen-devel
  Cc: Andrew Cooper, Sergey Dyasli, Wei Liu, Jan Beulich, Roger Pau Monné

The net diffstat is:
  add/remove: 0/13 grow/shrink: 25/129 up/down: 6297/-20469 (-14172)

With the following objects/functions removed entirely:
  iommu_hwdom_none                               1       -      -1
  hwdom_max_order                                4       -      -4
  extra_hwdom_irqs                               4       -      -4
  ctldom_max_order                               4       -      -4
  acpi_c1e_quirk                                43       -     -43
  hvm_pirq_eoi                                  62       -     -62
  max_order                                     94       -     -94
  conring_puts                                 104       -    -104
  propagate_node                               119       -    -119
  mmio_ro_emulate_ops                          224       -    -224
  mmcfg_intercept_ops                          224       -    -224
  pci_cfg_ok                                   295       -    -295
  p2m_lock                                     546       -    -546

And the following reduced to stubs:
  arch_iommu_hwdom_init                        852       2    -850
  p2m_add_foreign                              880      16    -864

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/include/xen/sched.h | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index a2accd90f6..cc942a3621 100644
--- 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);
+}
 
 #define VM_ASSIST(d, t) (test_bit(VMASST_TYPE_ ## t, &(d)->vm_assist))
 
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Xen-devel] [PATCH] x86/shim: Short circuit control/hardware checks in PV_SHIM_EXCLUSIVE builds
  2020-01-03 20:07 [Xen-devel] [PATCH] x86/shim: Short circuit control/hardware checks in PV_SHIM_EXCLUSIVE builds Andrew Cooper
@ 2020-01-05 16:30 ` Wei Liu
  2020-01-06 10:13 ` Jan Beulich
  2020-01-07  9:08 ` Sergey Dyasli
  2 siblings, 0 replies; 6+ messages in thread
From: Wei Liu @ 2020-01-05 16:30 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Xen-devel, Sergey Dyasli, Wei Liu, Jan Beulich, Roger Pau Monné

On Fri, Jan 03, 2020 at 08:07:42PM +0000, Andrew Cooper wrote:
> The net diffstat is:
>   add/remove: 0/13 grow/shrink: 25/129 up/down: 6297/-20469 (-14172)
> 
> With the following objects/functions removed entirely:
>   iommu_hwdom_none                               1       -      -1
>   hwdom_max_order                                4       -      -4
>   extra_hwdom_irqs                               4       -      -4
>   ctldom_max_order                               4       -      -4
>   acpi_c1e_quirk                                43       -     -43
>   hvm_pirq_eoi                                  62       -     -62
>   max_order                                     94       -     -94
>   conring_puts                                 104       -    -104
>   propagate_node                               119       -    -119
>   mmio_ro_emulate_ops                          224       -    -224
>   mmcfg_intercept_ops                          224       -    -224
>   pci_cfg_ok                                   295       -    -295
>   p2m_lock                                     546       -    -546
> 
> And the following reduced to stubs:
>   arch_iommu_hwdom_init                        852       2    -850
>   p2m_add_foreign                              880      16    -864
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wl@xen.org>

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Xen-devel] [PATCH] x86/shim: Short circuit control/hardware checks in PV_SHIM_EXCLUSIVE builds
  2020-01-03 20:07 [Xen-devel] [PATCH] x86/shim: Short circuit control/hardware checks in PV_SHIM_EXCLUSIVE builds Andrew Cooper
  2020-01-05 16:30 ` Wei Liu
@ 2020-01-06 10:13 ` Jan Beulich
  2020-01-06 17:16   ` Andrew Cooper
  2020-01-07  9:08 ` Sergey Dyasli
  2 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2020-01-06 10:13 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Sergey Dyasli, Wei Liu, Roger Pau Monné

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

Jan

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Xen-devel] [PATCH] x86/shim: Short circuit control/hardware checks in PV_SHIM_EXCLUSIVE builds
  2020-01-06 10:13 ` Jan Beulich
@ 2020-01-06 17:16   ` Andrew Cooper
  2020-01-07  9:05     ` Jan Beulich
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2020-01-06 17:16 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Xen-devel, Sergey Dyasli, Wei Liu, Roger Pau Monné

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.

~Andrew

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Xen-devel] [PATCH] x86/shim: Short circuit control/hardware checks in PV_SHIM_EXCLUSIVE builds
  2020-01-06 17:16   ` Andrew Cooper
@ 2020-01-07  9:05     ` Jan Beulich
  0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2020-01-07  9:05 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Sergey Dyasli, Xen-devel, Wei Liu, Roger Pau Monné

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Xen-devel] [PATCH] x86/shim: Short circuit control/hardware checks in PV_SHIM_EXCLUSIVE builds
  2020-01-03 20:07 [Xen-devel] [PATCH] x86/shim: Short circuit control/hardware checks in PV_SHIM_EXCLUSIVE builds Andrew Cooper
  2020-01-05 16:30 ` Wei Liu
  2020-01-06 10:13 ` Jan Beulich
@ 2020-01-07  9:08 ` Sergey Dyasli
  2 siblings, 0 replies; 6+ messages in thread
From: Sergey Dyasli @ 2020-01-07  9:08 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: sergey.dyasli@citrix.com >> Sergey Dyasli, Wei Liu,
	Jan Beulich, Roger Pau Monné

On 03/01/2020 20:07, Andrew Cooper wrote:
> The net diffstat is:
>   add/remove: 0/13 grow/shrink: 25/129 up/down: 6297/-20469 (-14172)
> 
> With the following objects/functions removed entirely:
>   iommu_hwdom_none                               1       -      -1
>   hwdom_max_order                                4       -      -4
>   extra_hwdom_irqs                               4       -      -4
>   ctldom_max_order                               4       -      -4
>   acpi_c1e_quirk                                43       -     -43
>   hvm_pirq_eoi                                  62       -     -62
>   max_order                                     94       -     -94
>   conring_puts                                 104       -    -104
>   propagate_node                               119       -    -119
>   mmio_ro_emulate_ops                          224       -    -224
>   mmcfg_intercept_ops                          224       -    -224
>   pci_cfg_ok                                   295       -    -295
>   p2m_lock                                     546       -    -546
> 
> And the following reduced to stubs:
>   arch_iommu_hwdom_init                        852       2    -850
>   p2m_add_foreign                              880      16    -864
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I tested this patch some time ago on a private branch, so

	Tested-by: Sergey Dyasli <sergey.dyasli@citrix.com>

Thanks,
Sergey

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-01-07  9:09 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-03 20:07 [Xen-devel] [PATCH] x86/shim: Short circuit control/hardware checks in PV_SHIM_EXCLUSIVE builds Andrew Cooper
2020-01-05 16:30 ` Wei Liu
2020-01-06 10:13 ` Jan Beulich
2020-01-06 17:16   ` Andrew Cooper
2020-01-07  9:05     ` Jan Beulich
2020-01-07  9:08 ` Sergey Dyasli

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.