[PATCH v4] xen/pvh: use a custom IO bitmap for PVH hardware domains

[PATCH v4] xen/pvh: use a custom IO bitmap for PVH hardware domains

[Roger Pau Monne]

Since a PVH hardware domain has access to the physical hardware create a
custom more permissive IO bitmap. The permissions set on the bitmap are
populated based on the contents of the ioports rangeset.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Eddie Dong <eddie.dong@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
---
Changes since v3:
 - Add the RTC IO ports to the list of blocked ports.
 - Remove admin_io_okay since it's just a wrapper around
   ioports_access_permitted.

Changes since v2:
 - Add 0xcf8-0xcfb to the range of blocked (trapped) IO ports.
 - Use rangeset_report_ranges in order to iterate over the range of not
   trapped IO ports.
 - Allocate the Dom0 PVH IO bitmap with _xmalloc_array, which allows setting
   the alignment to PAGE_SIZE.
 - Tested with Linux PV/PVH using 3.18 and FreeBSD PVH HEAD.

Changes since v1:
 - Dynamically allocate PVH Dom0 IO bitmap if needed.
 - Drop cast from construct_vmcs when writing the IO bitmap.
 - Create a new function that sets up the bitmap before launching Dom0. This
   is needed because ns16550_endboot is called after construct_dom0.
---
 xen/arch/x86/domain_build.c      | 29 +++++++++++++++++++++++++++--
 xen/arch/x86/hvm/hvm.c           | 21 ++++++++++++++++++++-
 xen/arch/x86/hvm/svm/vmcb.c      |  3 ++-
 xen/arch/x86/hvm/vmx/vmcs.c      |  5 +++--
 xen/arch/x86/setup.c             |  2 ++
 xen/arch/x86/traps.c             | 27 ++++-----------------------
 xen/include/asm-x86/hvm/domain.h |  2 ++
 xen/include/asm-x86/setup.h      |  1 +
 8 files changed, 61 insertions(+), 29 deletions(-)

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 378e650..5f27074 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -1546,8 +1546,10 @@ int __init construct_dom0(
     /* ACPI PM Timer. */
     if ( pmtmr_ioport )
         rc |= ioports_deny_access(d, pmtmr_ioport, pmtmr_ioport + 3);
-    /* PCI configuration space (NB. 0xcf8 has special treatment). */
-    rc |= ioports_deny_access(d, 0xcfc, 0xcff);
+    /* PCI configuration space */
+    rc |= ioports_deny_access(d, 0xcf8, 0xcff);
+    /* RTC */
+    rc |= ioports_deny_access(d, 0x70, 0x71);
     /* Command-line I/O ranges. */
     process_dom0_ioports_disable(d);
 
@@ -1635,6 +1637,29 @@ out:
     return rc;
 }
 
+static int __init io_bitmap_cb(unsigned long s, unsigned long e, void *ctx)
+{
+    struct domain *d = ctx;
+    unsigned long i;
+
+    for ( i = s; i <= e; i++ )
+        __clear_bit(i, d->arch.hvm_domain.io_bitmap);
+
+    return 0;
+}
+
+void __init setup_io_bitmap(struct domain *d)
+{
+    int rc;
+
+    if ( is_pvh_domain(d) )
+    {
+        rc = rangeset_report_ranges(d->arch.ioport_caps, 0, 0x10000,
+                                    io_bitmap_cb, d);
+        BUG_ON(rc);
+    }
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index bfde380..c842ae0 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -78,9 +78,10 @@ integer_param("hvm_debug", opt_hvm_debug_level);
 
 struct hvm_function_table hvm_funcs __read_mostly;
 
+#define HVM_IOBITMAP_SIZE   (3*PAGE_SIZE/BYTES_PER_LONG)
 /* I/O permission bitmap is globally shared by all HVM guests. */
 unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
-    hvm_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];
+    hvm_io_bitmap[HVM_IOBITMAP_SIZE];
 
 /* Xen command-line option to enable HAP */
 static bool_t __initdata opt_hap_enabled = 1;
@@ -1484,6 +1485,22 @@ int hvm_domain_initialise(struct domain *d)
         goto fail1;
     d->arch.hvm_domain.io_handler->num_slot = 0;
 
+    /* Set the default IO Bitmap */
+    if ( is_hardware_domain(d) )
+    {
+        d->arch.hvm_domain.io_bitmap = _xmalloc_array(BYTES_PER_LONG, PAGE_SIZE,
+                                                      HVM_IOBITMAP_SIZE);
+        if ( d->arch.hvm_domain.io_bitmap == NULL )
+        {
+            rc = -ENOMEM;
+            goto fail1;
+        }
+        memset(d->arch.hvm_domain.io_bitmap, ~0,
+               HVM_IOBITMAP_SIZE * BYTES_PER_LONG);
+    }
+    else
+        d->arch.hvm_domain.io_bitmap = hvm_io_bitmap;
+
     if ( is_pvh_domain(d) )
     {
         register_portio_handler(d, 0, 0x10003, handle_pvh_io);
@@ -1519,6 +1536,8 @@ int hvm_domain_initialise(struct domain *d)
     stdvga_deinit(d);
     vioapic_deinit(d);
  fail1:
+    if ( is_hardware_domain(d) )
+        xfree(d->arch.hvm_domain.io_bitmap);
     xfree(d->arch.hvm_domain.io_handler);
     xfree(d->arch.hvm_domain.params);
  fail0:
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 21292bb..227a187 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -118,7 +118,8 @@ static int construct_vmcb(struct vcpu *v)
         svm_disable_intercept_for_msr(v, MSR_AMD64_LWP_CBADDR);
 
     vmcb->_msrpm_base_pa = (u64)virt_to_maddr(arch_svm->msrpm);
-    vmcb->_iopm_base_pa  = (u64)virt_to_maddr(hvm_io_bitmap);
+    vmcb->_iopm_base_pa  =
+        (u64)virt_to_maddr(v->domain->arch.hvm_domain.io_bitmap);
 
     /* Virtualise EFLAGS.IF and LAPIC TPR (CR8). */
     vmcb->_vintr.fields.intr_masking = 1;
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 63007a9..bd37f0e 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -986,8 +986,9 @@ static int construct_vmcs(struct vcpu *v)
     }
 
     /* I/O access bitmap. */
-    __vmwrite(IO_BITMAP_A, virt_to_maddr((char *)hvm_io_bitmap + 0));
-    __vmwrite(IO_BITMAP_B, virt_to_maddr((char *)hvm_io_bitmap + PAGE_SIZE));
+    __vmwrite(IO_BITMAP_A, virt_to_maddr(d->arch.hvm_domain.io_bitmap));
+    __vmwrite(IO_BITMAP_B, virt_to_maddr(d->arch.hvm_domain.io_bitmap +
+                                         PAGE_SIZE / BYTES_PER_LONG));
 
     if ( cpu_has_vmx_virtual_intr_delivery )
     {
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 2b9787a..339343f 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1446,6 +1446,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     dmi_end_boot();
 
+    setup_io_bitmap(dom0);
+
     system_state = SYS_STATE_active;
 
     domain_unpause_by_systemcontroller(dom0);
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 22cdfc4..8d2bbb2 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1753,25 +1753,6 @@ static int guest_io_okay(
     return 0;
 }
 
-/* Has the administrator granted sufficient permission for this I/O access? */
-static int admin_io_okay(
-    unsigned int port, unsigned int bytes,
-    struct vcpu *v, struct cpu_user_regs *regs)
-{
-    /*
-     * Port 0xcf8 (CONFIG_ADDRESS) is only visible for DWORD accesses.
-     * We never permit direct access to that register.
-     */
-    if ( (port == 0xcf8) && (bytes == 4) )
-        return 0;
-
-    /* We also never permit direct access to the RTC/CMOS registers. */
-    if ( ((port & ~1) == RTC_PORT(0)) )
-        return 0;
-
-    return ioports_access_permitted(v->domain, port, port + bytes - 1);
-}
-
 static int pci_cfg_ok(struct domain *d, int write, int size)
 {
     uint32_t machine_bdf;
@@ -1813,7 +1794,7 @@ uint32_t guest_io_read(
     uint32_t data = 0;
     unsigned int shift = 0;
 
-    if ( admin_io_okay(port, bytes, v, regs) )
+    if ( ioports_access_permitted(v->domain, port, port + bytes - 1) )
     {
         switch ( bytes )
         {
@@ -1877,7 +1858,7 @@ void guest_io_write(
     unsigned int port, unsigned int bytes, uint32_t data,
     struct vcpu *v, struct cpu_user_regs *regs)
 {
-    if ( admin_io_okay(port, bytes, v, regs) )
+    if ( ioports_access_permitted(v->domain, port, port + bytes - 1) )
     {
         switch ( bytes ) {
         case 1:
@@ -2228,7 +2209,7 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
     exec_in:
         if ( !guest_io_okay(port, op_bytes, v, regs) )
             goto fail;
-        if ( admin_io_okay(port, op_bytes, v, regs) )
+        if ( ioports_access_permitted(v->domain, port, port + op_bytes - 1) )
         {
             mark_regs_dirty(regs);
             io_emul(regs);            
@@ -2258,7 +2239,7 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
     exec_out:
         if ( !guest_io_okay(port, op_bytes, v, regs) )
             goto fail;
-        if ( admin_io_okay(port, op_bytes, v, regs) )
+        if ( ioports_access_permitted(v->domain, port, port + op_bytes - 1) )
         {
             mark_regs_dirty(regs);
             io_emul(regs);            
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 0702bf5..d002954 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -143,6 +143,8 @@ struct hvm_domain {
      */
     uint64_t sync_tsc;
 
+    unsigned long         *io_bitmap;
+
     union {
         struct vmx_domain vmx;
         struct svm_domain svm;
diff --git a/xen/include/asm-x86/setup.h b/xen/include/asm-x86/setup.h
index 08bc23a..381d9f8 100644
--- a/xen/include/asm-x86/setup.h
+++ b/xen/include/asm-x86/setup.h
@@ -32,6 +32,7 @@ int construct_dom0(
     module_t *initrd,
     void *(*bootstrap_map)(const module_t *),
     char *cmdline);
+void setup_io_bitmap(struct domain *d);
 
 unsigned long initial_images_nrpages(nodeid_t node);
 void discard_initial_images(void);
-- 
1.9.5 (Apple Git-50.3)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v4] xen/pvh: use a custom IO bitmap for PVH hardware domains

[Andrew Cooper]

On 30/04/15 11:42, Roger Pau Monne wrote:
> Since a PVH hardware domain has access to the physical hardware create a
> custom more permissive IO bitmap. The permissions set on the bitmap are
> populated based on the contents of the ioports rangeset.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Cc: Eddie Dong <eddie.dong@intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> ---
> Changes since v3:
>   - Add the RTC IO ports to the list of blocked ports.
>   - Remove admin_io_okay since it's just a wrapper around
>     ioports_access_permitted.
>
> Changes since v2:
>   - Add 0xcf8-0xcfb to the range of blocked (trapped) IO ports.
>   - Use rangeset_report_ranges in order to iterate over the range of not
>     trapped IO ports.
>   - Allocate the Dom0 PVH IO bitmap with _xmalloc_array, which allows setting
>     the alignment to PAGE_SIZE.
>   - Tested with Linux PV/PVH using 3.18 and FreeBSD PVH HEAD.
>
> Changes since v1:
>   - Dynamically allocate PVH Dom0 IO bitmap if needed.
>   - Drop cast from construct_vmcs when writing the IO bitmap.
>   - Create a new function that sets up the bitmap before launching Dom0. This
>     is needed because ns16550_endboot is called after construct_dom0.
> ---
>   xen/arch/x86/domain_build.c      | 29 +++++++++++++++++++++++++++--
>   xen/arch/x86/hvm/hvm.c           | 21 ++++++++++++++++++++-
>   xen/arch/x86/hvm/svm/vmcb.c      |  3 ++-
>   xen/arch/x86/hvm/vmx/vmcs.c      |  5 +++--
>   xen/arch/x86/setup.c             |  2 ++
>   xen/arch/x86/traps.c             | 27 ++++-----------------------
>   xen/include/asm-x86/hvm/domain.h |  2 ++
>   xen/include/asm-x86/setup.h      |  1 +
>   8 files changed, 61 insertions(+), 29 deletions(-)
>
> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> index 378e650..5f27074 100644
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -1546,8 +1546,10 @@ int __init construct_dom0(
>       /* ACPI PM Timer. */
>       if ( pmtmr_ioport )
>           rc |= ioports_deny_access(d, pmtmr_ioport, pmtmr_ioport + 3);
> -    /* PCI configuration space (NB. 0xcf8 has special treatment). */
> -    rc |= ioports_deny_access(d, 0xcfc, 0xcff);
> +    /* PCI configuration space */
> +    rc |= ioports_deny_access(d, 0xcf8, 0xcff);
> +    /* RTC */
> +    rc |= ioports_deny_access(d, 0x70, 0x71);
>       /* Command-line I/O ranges. */
>       process_dom0_ioports_disable(d);
>   
> @@ -1635,6 +1637,29 @@ out:
>       return rc;
>   }
>   
> +static int __init io_bitmap_cb(unsigned long s, unsigned long e, void *ctx)
> +{
> +    struct domain *d = ctx;
> +    unsigned long i;
> +
> +    for ( i = s; i <= e; i++ )
> +        __clear_bit(i, d->arch.hvm_domain.io_bitmap);
> +
> +    return 0;
> +}
> +
> +void __init setup_io_bitmap(struct domain *d)
> +{
> +    int rc;
> +
> +    if ( is_pvh_domain(d) )
> +    {
> +        rc = rangeset_report_ranges(d->arch.ioport_caps, 0, 0x10000,
> +                                    io_bitmap_cb, d);
> +        BUG_ON(rc);
> +    }
> +}
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index bfde380..c842ae0 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -78,9 +78,10 @@ integer_param("hvm_debug", opt_hvm_debug_level);
>   
>   struct hvm_function_table hvm_funcs __read_mostly;
>   
> +#define HVM_IOBITMAP_SIZE   (3*PAGE_SIZE/BYTES_PER_LONG)
>   /* I/O permission bitmap is globally shared by all HVM guests. */
>   unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
> -    hvm_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];
> +    hvm_io_bitmap[HVM_IOBITMAP_SIZE];
>   
>   /* Xen command-line option to enable HAP */
>   static bool_t __initdata opt_hap_enabled = 1;
> @@ -1484,6 +1485,22 @@ int hvm_domain_initialise(struct domain *d)
>           goto fail1;
>       d->arch.hvm_domain.io_handler->num_slot = 0;
>   
> +    /* Set the default IO Bitmap */
> +    if ( is_hardware_domain(d) )
> +    {
> +        d->arch.hvm_domain.io_bitmap = _xmalloc_array(BYTES_PER_LONG, PAGE_SIZE,
> +                                                      HVM_IOBITMAP_SIZE);
> +        if ( d->arch.hvm_domain.io_bitmap == NULL )
> +        {
> +            rc = -ENOMEM;
> +            goto fail1;
> +        }
> +        memset(d->arch.hvm_domain.io_bitmap, ~0,
> +               HVM_IOBITMAP_SIZE * BYTES_PER_LONG);
> +    }
> +    else
> +        d->arch.hvm_domain.io_bitmap = hvm_io_bitmap;
> +
>       if ( is_pvh_domain(d) )
>       {
>           register_portio_handler(d, 0, 0x10003, handle_pvh_io);
> @@ -1519,6 +1536,8 @@ int hvm_domain_initialise(struct domain *d)
>       stdvga_deinit(d);
>       vioapic_deinit(d);
>    fail1:
> +    if ( is_hardware_domain(d) )
> +        xfree(d->arch.hvm_domain.io_bitmap);
>       xfree(d->arch.hvm_domain.io_handler);
>       xfree(d->arch.hvm_domain.params);
>    fail0:
> diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
> index 21292bb..227a187 100644
> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -118,7 +118,8 @@ static int construct_vmcb(struct vcpu *v)
>           svm_disable_intercept_for_msr(v, MSR_AMD64_LWP_CBADDR);
>   
>       vmcb->_msrpm_base_pa = (u64)virt_to_maddr(arch_svm->msrpm);
> -    vmcb->_iopm_base_pa  = (u64)virt_to_maddr(hvm_io_bitmap);
> +    vmcb->_iopm_base_pa  =
> +        (u64)virt_to_maddr(v->domain->arch.hvm_domain.io_bitmap);
>   
>       /* Virtualise EFLAGS.IF and LAPIC TPR (CR8). */
>       vmcb->_vintr.fields.intr_masking = 1;
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 63007a9..bd37f0e 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -986,8 +986,9 @@ static int construct_vmcs(struct vcpu *v)
>       }
>   
>       /* I/O access bitmap. */
> -    __vmwrite(IO_BITMAP_A, virt_to_maddr((char *)hvm_io_bitmap + 0));
> -    __vmwrite(IO_BITMAP_B, virt_to_maddr((char *)hvm_io_bitmap + PAGE_SIZE));
> +    __vmwrite(IO_BITMAP_A, virt_to_maddr(d->arch.hvm_domain.io_bitmap));
> +    __vmwrite(IO_BITMAP_B, virt_to_maddr(d->arch.hvm_domain.io_bitmap +
> +                                         PAGE_SIZE / BYTES_PER_LONG));
>   
>       if ( cpu_has_vmx_virtual_intr_delivery )
>       {
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 2b9787a..339343f 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1446,6 +1446,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>   
>       dmi_end_boot();
>   
> +    setup_io_bitmap(dom0);
> +
>       system_state = SYS_STATE_active;
>   
>       domain_unpause_by_systemcontroller(dom0);
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 22cdfc4..8d2bbb2 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1753,25 +1753,6 @@ static int guest_io_okay(
>       return 0;
>   }
>   
> -/* Has the administrator granted sufficient permission for this I/O access? */
> -static int admin_io_okay(
> -    unsigned int port, unsigned int bytes,
> -    struct vcpu *v, struct cpu_user_regs *regs)
> -{
> -    /*
> -     * Port 0xcf8 (CONFIG_ADDRESS) is only visible for DWORD accesses.
> -     * We never permit direct access to that register.
> -     */
> -    if ( (port == 0xcf8) && (bytes == 4) )
> -        return 0;
> -
> -    /* We also never permit direct access to the RTC/CMOS registers. */
> -    if ( ((port & ~1) == RTC_PORT(0)) )
> -        return 0;
> -
> -    return ioports_access_permitted(v->domain, port, port + bytes - 1);
> -}
> -
>   static int pci_cfg_ok(struct domain *d, int write, int size)
>   {
>       uint32_t machine_bdf;
> @@ -1813,7 +1794,7 @@ uint32_t guest_io_read(
>       uint32_t data = 0;
>       unsigned int shift = 0;
>   
> -    if ( admin_io_okay(port, bytes, v, regs) )
> +    if ( ioports_access_permitted(v->domain, port, port + bytes - 1) )
>       {
>           switch ( bytes )
>           {
> @@ -1877,7 +1858,7 @@ void guest_io_write(
>       unsigned int port, unsigned int bytes, uint32_t data,
>       struct vcpu *v, struct cpu_user_regs *regs)
>   {
> -    if ( admin_io_okay(port, bytes, v, regs) )
> +    if ( ioports_access_permitted(v->domain, port, port + bytes - 1) )
>       {
>           switch ( bytes ) {
>           case 1:
> @@ -2228,7 +2209,7 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
>       exec_in:
>           if ( !guest_io_okay(port, op_bytes, v, regs) )
>               goto fail;
> -        if ( admin_io_okay(port, op_bytes, v, regs) )
> +        if ( ioports_access_permitted(v->domain, port, port + op_bytes - 1) )
>           {
>               mark_regs_dirty(regs);
>               io_emul(regs);
> @@ -2258,7 +2239,7 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
>       exec_out:
>           if ( !guest_io_okay(port, op_bytes, v, regs) )
>               goto fail;
> -        if ( admin_io_okay(port, op_bytes, v, regs) )
> +        if ( ioports_access_permitted(v->domain, port, port + op_bytes - 1) )
>           {
>               mark_regs_dirty(regs);
>               io_emul(regs);
> diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
> index 0702bf5..d002954 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -143,6 +143,8 @@ struct hvm_domain {
>        */
>       uint64_t sync_tsc;
>   
> +    unsigned long         *io_bitmap;
> +
>       union {
>           struct vmx_domain vmx;
>           struct svm_domain svm;
> diff --git a/xen/include/asm-x86/setup.h b/xen/include/asm-x86/setup.h
> index 08bc23a..381d9f8 100644
> --- a/xen/include/asm-x86/setup.h
> +++ b/xen/include/asm-x86/setup.h
> @@ -32,6 +32,7 @@ int construct_dom0(
>       module_t *initrd,
>       void *(*bootstrap_map)(const module_t *),
>       char *cmdline);
> +void setup_io_bitmap(struct domain *d);
>   
>   unsigned long initial_images_nrpages(nodeid_t node);
>   void discard_initial_images(void);


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v4] xen/pvh: use a custom IO bitmap for PVH hardware domains

[Jan Beulich]

>>> Roger Pau Monne <roger.pau@citrix.com> 04/30/15 12:42 PM >>>
>--- a/xen/arch/x86/domain_build.c
>+++ b/xen/arch/x86/domain_build.c
>@@ -1546,8 +1546,10 @@ int __init construct_dom0(
>/* ACPI PM Timer. */
>if ( pmtmr_ioport )
>rc |= ioports_deny_access(d, pmtmr_ioport, pmtmr_ioport + 3);
>-    /* PCI configuration space (NB. 0xcf8 has special treatment). */
>-    rc |= ioports_deny_access(d, 0xcfc, 0xcff);
>+    /* PCI configuration space */
>+    rc |= ioports_deny_access(d, 0xcf8, 0xcff);

This introduces a change in behavior, which we don't want: Previously accesses to
ports in this range other than a 4-byte access to CF8 were allowed, while now they
aren't.

>+    /* RTC */
>+    rc |= ioports_deny_access(d, 0x70, 0x71);

This one looks to be a valid replacement (and simplification) of the original code.
But please use RTC_PORT() here just like the being replaced code did.

>@@ -1635,6 +1637,29 @@ out:
>return rc;
>}
 >
>+static int __init io_bitmap_cb(unsigned long s, unsigned long e, void *ctx)
>+{
>+    struct domain *d = ctx;
>+    unsigned long i;

unsigned int (or the use of __clear_bit() below isn't really correct).

>+
>+    for ( i = s; i <= e; i++ )
>+        __clear_bit(i, d->arch.hvm_domain.io_bitmap);
>+
>+    return 0;
>+}
>+
>+void __init setup_io_bitmap(struct domain *d)
>+{
>+    int rc;
>+
>+    if ( is_pvh_domain(d) )

>From a conceptual standpoint this needs to be has_hvm_container_domain().

I also don't really see why these functions (with the used names) are being
placed in this file. In particular they would need to be called also when it's
other than Dom0 that acts as the hardware domain, and then they need to
be marked __hwdom_init and hence can't be placed in this file anymore.

>--- a/xen/arch/x86/hvm/hvm.c
>+++ b/xen/arch/x86/hvm/hvm.c
>@@ -78,9 +78,10 @@ integer_param("hvm_debug", opt_hvm_debug_level);
 >
>struct hvm_function_table hvm_funcs __read_mostly;
 >
>+#define HVM_IOBITMAP_SIZE   (3*PAGE_SIZE/BYTES_PER_LONG)

I think it would be even better if the define didn't consider the
representation as longs, but instead its users did where needed. Or if
not, its name needs to express that this isn't a size, but the number of
longs (e.g. HVM_IOBITMAP_LONGS).

>/* I/O permission bitmap is globally shared by all HVM guests. */

This comment needs updating.

>--- a/xen/arch/x86/hvm/svm/vmcb.c
>+++ b/xen/arch/x86/hvm/svm/vmcb.c
>@@ -118,7 +118,8 @@ static int construct_vmcb(struct vcpu *v)
>svm_disable_intercept_for_msr(v, MSR_AMD64_LWP_CBADDR);
 >
>vmcb->_msrpm_base_pa = (u64)virt_to_maddr(arch_svm->msrpm);
>-    vmcb->_iopm_base_pa  = (u64)virt_to_maddr(hvm_io_bitmap);
>+    vmcb->_iopm_base_pa  =
>+        (u64)virt_to_maddr(v->domain->arch.hvm_domain.io_bitmap);

Is the cast really needed here? If not, please drop it.

Jan