diff mbox

[RFC,03/12] xen/x86: allow the emulated APICs to be enbled for the hardware domain

Message ID 1469809747-11176-4-git-send-email-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Roger Pau Monné July 29, 2016, 4:28 p.m. UTC
Allow the used of both the emulated local APIC and IO APIC for the hardware
domain.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/domain.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

Comments

Andrew Cooper July 29, 2016, 5:50 p.m. UTC | #1
On 29/07/16 17:28, Roger Pau Monne wrote:
>          if ( is_hardware_domain(d) )
> -            config->emulation_flags |= XEN_X86_EMU_PIT;
> -        if ( config->emulation_flags != 0 &&
> -             (config->emulation_flags !=
> -              (is_hvm_domain(d) ? XEN_X86_EMU_ALL : XEN_X86_EMU_PIT)) )
> +            emflags |= XEN_X86_EMU_PIT;
> +
> +        if ( (is_hardware_domain(d) ?
> +              (is_hvm_domain(d) && emflags !=
> +              (XEN_X86_EMU_PIT|XEN_X86_EMU_LAPIC|XEN_X86_EMU_IOAPIC)) :
> +              (emflags && (is_hvm_domain(d) ? (emflags != XEN_X86_EMU_ALL) :
> +                                              (emflags != XEN_X86_EMU_PIT)))) )

This is getting very complicated.

It is rather important (security wise) and not a hotpath.  Could I
please request that you move this logic to a helper such as:

static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)

and make this more readable.  Any decent compiler will inline and
simplify it appropriately.

~Andrew
Roger Pau Monné Aug. 1, 2016, 11:23 a.m. UTC | #2
On Fri, Jul 29, 2016 at 06:50:48PM +0100, Andrew Cooper wrote:
> On 29/07/16 17:28, Roger Pau Monne wrote:
> >          if ( is_hardware_domain(d) )
> > -            config->emulation_flags |= XEN_X86_EMU_PIT;
> > -        if ( config->emulation_flags != 0 &&
> > -             (config->emulation_flags !=
> > -              (is_hvm_domain(d) ? XEN_X86_EMU_ALL : XEN_X86_EMU_PIT)) )
> > +            emflags |= XEN_X86_EMU_PIT;
> > +
> > +        if ( (is_hardware_domain(d) ?
> > +              (is_hvm_domain(d) && emflags !=
> > +              (XEN_X86_EMU_PIT|XEN_X86_EMU_LAPIC|XEN_X86_EMU_IOAPIC)) :
> > +              (emflags && (is_hvm_domain(d) ? (emflags != XEN_X86_EMU_ALL) :
> > +                                              (emflags != XEN_X86_EMU_PIT)))) )
> 
> This is getting very complicated.
> 
> It is rather important (security wise) and not a hotpath.  Could I
> please request that you move this logic to a helper such as:
> 
> static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
> 
> and make this more readable.  Any decent compiler will inline and
> simplify it appropriately.

Thanks, I've now moved it to a helper as suggested:

static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
{

    if ( is_hvm_domain(d) )
    {
        if ( is_hardware_domain(d) &&
             emflags != (XEN_X86_EMU_PIT|XEN_X86_EMU_LAPIC|XEN_X86_EMU_IOAPIC))
            return false;
        if ( !is_hardware_domain(d) &&
             emflags != XEN_X86_EMU_ALL && emflags != 0 )
            return false;
    }
    else
    {
        /* PV or classic PVH. */
        if ( is_hardware_domain(d) && emflags != XEN_X86_EMU_PIT )
            return false;
        if ( !is_hardware_domain(d) && emflags != 0 )
            return false;
    }
    return true;
}

Roger.
diff mbox

Patch

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 1133ea2..4d47228 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -545,25 +545,31 @@  int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     }
     else
     {
-        if ( (config->emulation_flags & ~XEN_X86_EMU_ALL) != 0 )
+        uint32_t emflags = config->emulation_flags;
+
+        if ( (emflags & ~XEN_X86_EMU_ALL) != 0 )
         {
             printk(XENLOG_G_ERR "d%d: Invalid emulation bitmap: %#x\n",
-                   d->domain_id, config->emulation_flags);
+                   d->domain_id, emflags);
             return -EINVAL;
         }
+
         if ( is_hardware_domain(d) )
-            config->emulation_flags |= XEN_X86_EMU_PIT;
-        if ( config->emulation_flags != 0 &&
-             (config->emulation_flags !=
-              (is_hvm_domain(d) ? XEN_X86_EMU_ALL : XEN_X86_EMU_PIT)) )
+            emflags |= XEN_X86_EMU_PIT;
+
+        if ( (is_hardware_domain(d) ?
+              (is_hvm_domain(d) && emflags !=
+              (XEN_X86_EMU_PIT|XEN_X86_EMU_LAPIC|XEN_X86_EMU_IOAPIC)) :
+              (emflags && (is_hvm_domain(d) ? (emflags != XEN_X86_EMU_ALL) :
+                                              (emflags != XEN_X86_EMU_PIT)))) )
         {
             printk(XENLOG_G_ERR "d%d: Xen does not allow %s domain creation "
                    "with the current selection of emulators: %#x\n",
                    d->domain_id, is_hvm_domain(d) ? "HVM" : "PV",
-                   config->emulation_flags);
+                   emflags);
             return -EOPNOTSUPP;
         }
-        d->arch.emulation_flags = config->emulation_flags;
+        d->arch.emulation_flags = emflags;
     }
 
     if ( has_hvm_container_domain(d) )