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

[PATCH RFC] 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.

Also add the IO ports of the serial console used by Xen to the list of not
accessible IO ports.

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>
---
 xen/arch/x86/domain_build.c      | 10 ++++++++++
 xen/arch/x86/hvm/hvm.c           | 11 +++++++++++
 xen/arch/x86/hvm/svm/vmcb.c      |  3 ++-
 xen/arch/x86/hvm/vmx/vmcs.c      |  6 ++++--
 xen/arch/x86/hvm/vmx/vmx.c       |  1 +
 xen/drivers/char/ns16550.c       | 10 ++++++++++
 xen/include/asm-x86/hvm/domain.h |  2 ++
 xen/include/asm-x86/hvm/hvm.h    |  1 +
 xen/include/xen/serial.h         |  4 ++++
 9 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index e5c845c..d0365fe 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -22,6 +22,7 @@
 #include <xen/compat.h>
 #include <xen/libelf.h>
 #include <xen/pfn.h>
+#include <xen/serial.h>
 #include <asm/regs.h>
 #include <asm/system.h>
 #include <asm/io.h>
@@ -1541,6 +1542,11 @@ int __init construct_dom0(
     rc |= ioports_deny_access(d, 0x40, 0x43);
     /* PIT Channel 2 / PC Speaker Control. */
     rc |= ioports_deny_access(d, 0x61, 0x61);
+    /* Serial console. */
+    if ( uart_ioport1 != 0 )
+        rc |= ioports_deny_access(d, uart_ioport1, uart_ioport1 + 7);
+    if ( uart_ioport2 != 0 )
+        rc |= ioports_deny_access(d, uart_ioport2, uart_ioport2 + 7);
     /* ACPI PM Timer. */
     if ( pmtmr_ioport )
         rc |= ioports_deny_access(d, pmtmr_ioport, pmtmr_ioport + 3);
@@ -1618,6 +1624,10 @@ int __init construct_dom0(
 
         pvh_map_all_iomem(d, nr_pages);
         pvh_setup_e820(d, nr_pages);
+
+        for ( i = 0; i < 0x10000; i++ )
+            if ( ioports_access_permitted(d, i, i) )
+                __clear_bit(i, hvm_hw_io_bitmap);
     }
 
     if ( d->domain_id == hardware_domid )
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 3ff87c6..6de89b2 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -82,6 +82,10 @@ struct hvm_function_table hvm_funcs __read_mostly;
 unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
     hvm_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];
 
+/* I/O permission bitmap for HVM hardware domain */
+unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
+    hvm_hw_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];
+
 /* Xen command-line option to enable HAP */
 static bool_t __initdata opt_hap_enabled = 1;
 boolean_param("hap", opt_hap_enabled);
@@ -162,6 +166,7 @@ static int __init hvm_enable(void)
      * often used for I/O delays, but the vmexits simply slow things down).
      */
     memset(hvm_io_bitmap, ~0, sizeof(hvm_io_bitmap));
+    memset(hvm_hw_io_bitmap, ~0, sizeof(hvm_hw_io_bitmap));
     if ( hvm_port80_allowed )
         __clear_bit(0x80, hvm_io_bitmap);
     __clear_bit(0xed, hvm_io_bitmap);
@@ -1484,6 +1489,12 @@ 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 = hvm_hw_io_bitmap;
+    else
+        d->arch.hvm_domain.io_bitmap = hvm_io_bitmap;
+
     if ( is_pvh_domain(d) )
     {
         register_portio_handler(d, 0, 0x10003, handle_pvh_io);
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 21292bb..eb2176b 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -72,6 +72,7 @@ static int construct_vmcb(struct vcpu *v)
 {
     struct arch_svm_struct *arch_svm = &v->arch.hvm_svm;
     struct vmcb_struct *vmcb = arch_svm->vmcb;
+    struct domain *d = v->domain;
 
     vmcb->_general1_intercepts = 
         GENERAL1_INTERCEPT_INTR        | GENERAL1_INTERCEPT_NMI         |
@@ -118,7 +119,7 @@ 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(d->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 d614638..3080979 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -986,8 +986,10 @@ 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((char *)d->arch.hvm_domain.io_bitmap + 0));
+    __vmwrite(IO_BITMAP_B,
+              virt_to_maddr((char *)d->arch.hvm_domain.io_bitmap + PAGE_SIZE));
 
     if ( cpu_has_vmx_virtual_intr_delivery )
     {
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 2ac1492..d5ccdce 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3118,6 +3118,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
             uint16_t port = (exit_qualification >> 16) & 0xFFFF;
             int bytes = (exit_qualification & 0x07) + 1;
             int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;
+
             if ( handle_pio(port, bytes, dir) )
                 update_guest_eip(); /* Safe: IN, OUT */
         }
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index d443880..b382e72 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -44,6 +44,9 @@ static char __initdata opt_com2[30] = "";
 string_param("com1", opt_com1);
 string_param("com2", opt_com2);
 
+unsigned int uart_ioport1 = 0;
+unsigned int uart_ioport2 = 0;
+
 static struct ns16550 {
     int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
     u64 io_base;   /* I/O port or memory-mapped I/O address. */
@@ -1121,6 +1124,13 @@ void __init ns16550_init(int index, struct ns16550_defaults *defaults)
     uart->reg_shift = 0;
 
     ns16550_parse_port_config(uart, (index == 0) ? opt_com1 : opt_com2);
+    if ( uart->baud != 0 && uart->io_base < 0x10000 )
+    {
+        if ( index == 0 )
+            uart_ioport1 = uart->io_base;
+        else
+            uart_ioport2 = uart->io_base;
+    }
 }
 
 #ifdef HAS_DEVICE_TREE
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/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 0dc909b..55e432e 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -213,6 +213,7 @@ extern struct hvm_function_table hvm_funcs;
 extern bool_t hvm_enabled;
 extern bool_t cpu_has_lmsl;
 extern s8 hvm_port80_allowed;
+extern unsigned long hvm_hw_io_bitmap[];
 
 extern const struct hvm_function_table *start_svm(void);
 extern const struct hvm_function_table *start_vmx(void);
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index 71e6ade..bc5fb06 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -24,6 +24,10 @@ void serial_set_rx_handler(int handle, serial_rx_fn fn);
 /* Number of characters we buffer for an interrupt-driven transmitter. */
 extern unsigned int serial_txbufsz;
 
+/* UART IO port */
+extern unsigned int uart_ioport1;
+extern unsigned int uart_ioport2;
+
 struct uart_driver;
 
 enum serial_port_state {
-- 
1.9.5 (Apple Git-50.3)


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

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

[Andrew Cooper]

On 08/04/15 13:57, 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.
>
> Also add the IO ports of the serial console used by Xen to the list of not
> accessible IO ports.

Thankyou for looking into this.  I think it is the correct general
direction, but I do have some questions/thoughts about this area.

I know that the current implementation is that dom0 is whitelisted and
can play with everything, but is this actually the best API? 
Conceptually, a better approach would be for dom0 to start with no
permissions, and explicitly request access (After all, PV and PVH
domains are expected to know exactly what they are doing under Xen). 
This has an extra advantage in that dom0 can't accidentally grant
permissions for resources it doens't know about to domUs.

Instead of adding to a growing blacklist in contruct_dom0, it might be a
better to maintain a global rangeset (or few) for resources which are
used by Xen and not permitted to be used by any other domains.  This
would allow the ioports_deny_access()/etc calls to move into the correct
drivers, instead of having to extern things like the uart ports.  It is
also far more likely to be kept up to date.  (On that note, we could
probably do with an audit of the currently denied resources.  I highly
doubt there is a PIT driver which could function with access to only
some of the ports).

In addition, some specific review...

>
> 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>
> ---
>  xen/arch/x86/domain_build.c      | 10 ++++++++++
>  xen/arch/x86/hvm/hvm.c           | 11 +++++++++++
>  xen/arch/x86/hvm/svm/vmcb.c      |  3 ++-
>  xen/arch/x86/hvm/vmx/vmcs.c      |  6 ++++--
>  xen/arch/x86/hvm/vmx/vmx.c       |  1 +
>  xen/drivers/char/ns16550.c       | 10 ++++++++++
>  xen/include/asm-x86/hvm/domain.h |  2 ++
>  xen/include/asm-x86/hvm/hvm.h    |  1 +
>  xen/include/xen/serial.h         |  4 ++++
>  9 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> index e5c845c..d0365fe 100644
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -22,6 +22,7 @@
>  #include <xen/compat.h>
>  #include <xen/libelf.h>
>  #include <xen/pfn.h>
> +#include <xen/serial.h>
>  #include <asm/regs.h>
>  #include <asm/system.h>
>  #include <asm/io.h>
> @@ -1541,6 +1542,11 @@ int __init construct_dom0(
>      rc |= ioports_deny_access(d, 0x40, 0x43);
>      /* PIT Channel 2 / PC Speaker Control. */
>      rc |= ioports_deny_access(d, 0x61, 0x61);
> +    /* Serial console. */
> +    if ( uart_ioport1 != 0 )
> +        rc |= ioports_deny_access(d, uart_ioport1, uart_ioport1 + 7);
> +    if ( uart_ioport2 != 0 )
> +        rc |= ioports_deny_access(d, uart_ioport2, uart_ioport2 + 7);
>      /* ACPI PM Timer. */
>      if ( pmtmr_ioport )
>          rc |= ioports_deny_access(d, pmtmr_ioport, pmtmr_ioport + 3);
> @@ -1618,6 +1624,10 @@ int __init construct_dom0(
>  
>          pvh_map_all_iomem(d, nr_pages);
>          pvh_setup_e820(d, nr_pages);
> +
> +        for ( i = 0; i < 0x10000; i++ )
> +            if ( ioports_access_permitted(d, i, i) )
> +                __clear_bit(i, hvm_hw_io_bitmap);

(There is surely a more efficient way of doing this?  If there isn't,
there probably should be)

There is also a boundary issue between VT-x and SVM.

For VT-x, the IO bitmap is 2 pages.  For SVM, it is 2 pages and 3 bits. 
I suspect the difference is to do with the handling of a 4byte write to
port 0xffff.  I think you might need to check "i < 0x10003" instead.

>      }
>  
>      if ( d->domain_id == hardware_domid )
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 3ff87c6..6de89b2 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -82,6 +82,10 @@ struct hvm_function_table hvm_funcs __read_mostly;
>  unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
>      hvm_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];
>  
> +/* I/O permission bitmap for HVM hardware domain */
> +unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
> +    hvm_hw_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];
> +
>  /* Xen command-line option to enable HAP */
>  static bool_t __initdata opt_hap_enabled = 1;
>  boolean_param("hap", opt_hap_enabled);
> @@ -162,6 +166,7 @@ static int __init hvm_enable(void)
>       * often used for I/O delays, but the vmexits simply slow things down).
>       */
>      memset(hvm_io_bitmap, ~0, sizeof(hvm_io_bitmap));
> +    memset(hvm_hw_io_bitmap, ~0, sizeof(hvm_hw_io_bitmap));
>      if ( hvm_port80_allowed )
>          __clear_bit(0x80, hvm_io_bitmap);
>      __clear_bit(0xed, hvm_io_bitmap);
> @@ -1484,6 +1489,12 @@ 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 = hvm_hw_io_bitmap;
> +    else
> +        d->arch.hvm_domain.io_bitmap = hvm_io_bitmap;

You will need to augment late_hwdom_init() as well, or you will break
anyone making us of "hardware_dom=$N"

~Andrew

> +
>      if ( is_pvh_domain(d) )
>      {
>          register_portio_handler(d, 0, 0x10003, handle_pvh_io);
> diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
> index 21292bb..eb2176b 100644
> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -72,6 +72,7 @@ static int construct_vmcb(struct vcpu *v)
>  {
>      struct arch_svm_struct *arch_svm = &v->arch.hvm_svm;
>      struct vmcb_struct *vmcb = arch_svm->vmcb;
> +    struct domain *d = v->domain;
>  
>      vmcb->_general1_intercepts = 
>          GENERAL1_INTERCEPT_INTR        | GENERAL1_INTERCEPT_NMI         |
> @@ -118,7 +119,7 @@ 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(d->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 d614638..3080979 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -986,8 +986,10 @@ 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((char *)d->arch.hvm_domain.io_bitmap + 0));
> +    __vmwrite(IO_BITMAP_B,
> +              virt_to_maddr((char *)d->arch.hvm_domain.io_bitmap + PAGE_SIZE));
>  
>      if ( cpu_has_vmx_virtual_intr_delivery )
>      {
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 2ac1492..d5ccdce 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3118,6 +3118,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>              uint16_t port = (exit_qualification >> 16) & 0xFFFF;
>              int bytes = (exit_qualification & 0x07) + 1;
>              int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;
> +
>              if ( handle_pio(port, bytes, dir) )
>                  update_guest_eip(); /* Safe: IN, OUT */
>          }
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index d443880..b382e72 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -44,6 +44,9 @@ static char __initdata opt_com2[30] = "";
>  string_param("com1", opt_com1);
>  string_param("com2", opt_com2);
>  
> +unsigned int uart_ioport1 = 0;
> +unsigned int uart_ioport2 = 0;
> +
>  static struct ns16550 {
>      int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
>      u64 io_base;   /* I/O port or memory-mapped I/O address. */
> @@ -1121,6 +1124,13 @@ void __init ns16550_init(int index, struct ns16550_defaults *defaults)
>      uart->reg_shift = 0;
>  
>      ns16550_parse_port_config(uart, (index == 0) ? opt_com1 : opt_com2);
> +    if ( uart->baud != 0 && uart->io_base < 0x10000 )
> +    {
> +        if ( index == 0 )
> +            uart_ioport1 = uart->io_base;
> +        else
> +            uart_ioport2 = uart->io_base;
> +    }
>  }
>  
>  #ifdef HAS_DEVICE_TREE
> 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/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 0dc909b..55e432e 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -213,6 +213,7 @@ extern struct hvm_function_table hvm_funcs;
>  extern bool_t hvm_enabled;
>  extern bool_t cpu_has_lmsl;
>  extern s8 hvm_port80_allowed;
> +extern unsigned long hvm_hw_io_bitmap[];
>  
>  extern const struct hvm_function_table *start_svm(void);
>  extern const struct hvm_function_table *start_vmx(void);
> diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
> index 71e6ade..bc5fb06 100644
> --- a/xen/include/xen/serial.h
> +++ b/xen/include/xen/serial.h
> @@ -24,6 +24,10 @@ void serial_set_rx_handler(int handle, serial_rx_fn fn);
>  /* Number of characters we buffer for an interrupt-driven transmitter. */
>  extern unsigned int serial_txbufsz;
>  
> +/* UART IO port */
> +extern unsigned int uart_ioport1;
> +extern unsigned int uart_ioport2;
> +
>  struct uart_driver;
>  
>  enum serial_port_state {


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

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

[Tian, Kevin]

> From: Roger Pau Monne [mailto:roger.pau@citrix.com]
> Sent: Wednesday, April 08, 2015 8:57 PM
> 
> 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.
> 
> Also add the IO ports of the serial console used by Xen to the list of not
> accessible IO ports.
> 
> 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>
> ---
>  xen/arch/x86/domain_build.c      | 10 ++++++++++
>  xen/arch/x86/hvm/hvm.c           | 11 +++++++++++
>  xen/arch/x86/hvm/svm/vmcb.c      |  3 ++-
>  xen/arch/x86/hvm/vmx/vmcs.c      |  6 ++++--
>  xen/arch/x86/hvm/vmx/vmx.c       |  1 +
>  xen/drivers/char/ns16550.c       | 10 ++++++++++
>  xen/include/asm-x86/hvm/domain.h |  2 ++
>  xen/include/asm-x86/hvm/hvm.h    |  1 +
>  xen/include/xen/serial.h         |  4 ++++
>  9 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> index e5c845c..d0365fe 100644
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -22,6 +22,7 @@
>  #include <xen/compat.h>
>  #include <xen/libelf.h>
>  #include <xen/pfn.h>
> +#include <xen/serial.h>
>  #include <asm/regs.h>
>  #include <asm/system.h>
>  #include <asm/io.h>
> @@ -1541,6 +1542,11 @@ int __init construct_dom0(
>      rc |= ioports_deny_access(d, 0x40, 0x43);
>      /* PIT Channel 2 / PC Speaker Control. */
>      rc |= ioports_deny_access(d, 0x61, 0x61);
> +    /* Serial console. */
> +    if ( uart_ioport1 != 0 )
> +        rc |= ioports_deny_access(d, uart_ioport1, uart_ioport1 + 7);
> +    if ( uart_ioport2 != 0 )
> +        rc |= ioports_deny_access(d, uart_ioport2, uart_ioport2 + 7);

would this be better in actual serial driver?

>      /* ACPI PM Timer. */
>      if ( pmtmr_ioport )
>          rc |= ioports_deny_access(d, pmtmr_ioport, pmtmr_ioport + 3);
> @@ -1618,6 +1624,10 @@ int __init construct_dom0(
> 
>          pvh_map_all_iomem(d, nr_pages);
>          pvh_setup_e820(d, nr_pages);
> +
> +        for ( i = 0; i < 0x10000; i++ )
> +            if ( ioports_access_permitted(d, i, i) )
> +                __clear_bit(i, hvm_hw_io_bitmap);
>      }
> 
>      if ( d->domain_id == hardware_domid )
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 3ff87c6..6de89b2 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -82,6 +82,10 @@ struct hvm_function_table hvm_funcs __read_mostly;
>  unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
>      hvm_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];
> 
> +/* I/O permission bitmap for HVM hardware domain */
> +unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
> +    hvm_hw_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];
> +
>  /* Xen command-line option to enable HAP */
>  static bool_t __initdata opt_hap_enabled = 1;
>  boolean_param("hap", opt_hap_enabled);
> @@ -162,6 +166,7 @@ static int __init hvm_enable(void)
>       * often used for I/O delays, but the vmexits simply slow things down).
>       */
>      memset(hvm_io_bitmap, ~0, sizeof(hvm_io_bitmap));
> +    memset(hvm_hw_io_bitmap, ~0, sizeof(hvm_hw_io_bitmap));
>      if ( hvm_port80_allowed )
>          __clear_bit(0x80, hvm_io_bitmap);
>      __clear_bit(0xed, hvm_io_bitmap);
> @@ -1484,6 +1489,12 @@ 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 = hvm_hw_io_bitmap;
> +    else
> +        d->arch.hvm_domain.io_bitmap = hvm_io_bitmap;
> +

if we want to support multiple PVH hardware domains w/ different
permissions, using global array is not correct here.

>      if ( is_pvh_domain(d) )
>      {
>          register_portio_handler(d, 0, 0x10003, handle_pvh_io);
> diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
> index 21292bb..eb2176b 100644
> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -72,6 +72,7 @@ static int construct_vmcb(struct vcpu *v)
>  {
>      struct arch_svm_struct *arch_svm = &v->arch.hvm_svm;
>      struct vmcb_struct *vmcb = arch_svm->vmcb;
> +    struct domain *d = v->domain;
> 
>      vmcb->_general1_intercepts =
>          GENERAL1_INTERCEPT_INTR        |
> GENERAL1_INTERCEPT_NMI         |
> @@ -118,7 +119,7 @@ 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(d->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 d614638..3080979 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -986,8 +986,10 @@ 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((char *)d->arch.hvm_domain.io_bitmap +
> 0));
> +    __vmwrite(IO_BITMAP_B,
> +              virt_to_maddr((char *)d->arch.hvm_domain.io_bitmap +
> PAGE_SIZE));
> 
>      if ( cpu_has_vmx_virtual_intr_delivery )
>      {
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 2ac1492..d5ccdce 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3118,6 +3118,7 @@ void vmx_vmexit_handler(struct cpu_user_regs
> *regs)
>              uint16_t port = (exit_qualification >> 16) & 0xFFFF;
>              int bytes = (exit_qualification & 0x07) + 1;
>              int dir = (exit_qualification & 0x08) ? IOREQ_READ :
> IOREQ_WRITE;
> +
>              if ( handle_pio(port, bytes, dir) )
>                  update_guest_eip(); /* Safe: IN, OUT */
>          }
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index d443880..b382e72 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -44,6 +44,9 @@ static char __initdata opt_com2[30] = "";
>  string_param("com1", opt_com1);
>  string_param("com2", opt_com2);
> 
> +unsigned int uart_ioport1 = 0;
> +unsigned int uart_ioport2 = 0;
> +
>  static struct ns16550 {
>      int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
>      u64 io_base;   /* I/O port or memory-mapped I/O address. */
> @@ -1121,6 +1124,13 @@ void __init ns16550_init(int index, struct
> ns16550_defaults *defaults)
>      uart->reg_shift = 0;
> 
>      ns16550_parse_port_config(uart, (index == 0) ? opt_com1 : opt_com2);
> +    if ( uart->baud != 0 && uart->io_base < 0x10000 )
> +    {
> +        if ( index == 0 )
> +            uart_ioport1 = uart->io_base;
> +        else
> +            uart_ioport2 = uart->io_base;
> +    }
>  }
> 
>  #ifdef HAS_DEVICE_TREE
> 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/hvm/hvm.h
> b/xen/include/asm-x86/hvm/hvm.h
> index 0dc909b..55e432e 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -213,6 +213,7 @@ extern struct hvm_function_table hvm_funcs;
>  extern bool_t hvm_enabled;
>  extern bool_t cpu_has_lmsl;
>  extern s8 hvm_port80_allowed;
> +extern unsigned long hvm_hw_io_bitmap[];
> 
>  extern const struct hvm_function_table *start_svm(void);
>  extern const struct hvm_function_table *start_vmx(void);
> diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
> index 71e6ade..bc5fb06 100644
> --- a/xen/include/xen/serial.h
> +++ b/xen/include/xen/serial.h
> @@ -24,6 +24,10 @@ void serial_set_rx_handler(int handle, serial_rx_fn fn);
>  /* Number of characters we buffer for an interrupt-driven transmitter. */
>  extern unsigned int serial_txbufsz;
> 
> +/* UART IO port */
> +extern unsigned int uart_ioport1;
> +extern unsigned int uart_ioport2;
> +
>  struct uart_driver;
> 
>  enum serial_port_state {
> --
> 1.9.5 (Apple Git-50.3)

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

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

[Roger Pau Monné]

El 10/04/15 a les 3.43, Tian, Kevin ha escrit:
>> From: Roger Pau Monne [mailto:roger.pau@citrix.com]
[...]
>> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
>> index e5c845c..d0365fe 100644
>> --- a/xen/arch/x86/domain_build.c
>> +++ b/xen/arch/x86/domain_build.c
>> @@ -22,6 +22,7 @@
>>  #include <xen/compat.h>
>>  #include <xen/libelf.h>
>>  #include <xen/pfn.h>
>> +#include <xen/serial.h>
>>  #include <asm/regs.h>
>>  #include <asm/system.h>
>>  #include <asm/io.h>
>> @@ -1541,6 +1542,11 @@ int __init construct_dom0(
>>      rc |= ioports_deny_access(d, 0x40, 0x43);
>>      /* PIT Channel 2 / PC Speaker Control. */
>>      rc |= ioports_deny_access(d, 0x61, 0x61);
>> +    /* Serial console. */
>> +    if ( uart_ioport1 != 0 )
>> +        rc |= ioports_deny_access(d, uart_ioport1, uart_ioport1 + 7);
>> +    if ( uart_ioport2 != 0 )
>> +        rc |= ioports_deny_access(d, uart_ioport2, uart_ioport2 + 7);
> 
> would this be better in actual serial driver?

Definitely, I'm doing it this way because at the point were the uart is
set the initial domain struct has not even been created, much less
passed to the uart driver code.

I will rework this to be more similar to mmio_ro_ranges, which can be
set from driver code directly.

[...]

>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -82,6 +82,10 @@ struct hvm_function_table hvm_funcs __read_mostly;
>>  unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
>>      hvm_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];
>>
>> +/* I/O permission bitmap for HVM hardware domain */
>> +unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
>> +    hvm_hw_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];
>> +
>>  /* Xen command-line option to enable HAP */
>>  static bool_t __initdata opt_hap_enabled = 1;
>>  boolean_param("hap", opt_hap_enabled);
>> @@ -162,6 +166,7 @@ static int __init hvm_enable(void)
>>       * often used for I/O delays, but the vmexits simply slow things down).
>>       */
>>      memset(hvm_io_bitmap, ~0, sizeof(hvm_io_bitmap));
>> +    memset(hvm_hw_io_bitmap, ~0, sizeof(hvm_hw_io_bitmap));
>>      if ( hvm_port80_allowed )
>>          __clear_bit(0x80, hvm_io_bitmap);
>>      __clear_bit(0xed, hvm_io_bitmap);
>> @@ -1484,6 +1489,12 @@ 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 = hvm_hw_io_bitmap;
>> +    else
>> +        d->arch.hvm_domain.io_bitmap = hvm_io_bitmap;
>> +
> 
> if we want to support multiple PVH hardware domains w/ different
> permissions, using global array is not correct here.

Isn't there supposed to be only one hardware domain that has access to
the full hardware?

Roger.

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

[Jan Beulich]

>>> On 10.04.15 at 03:43, <kevin.tian@intel.com> wrote:
>>  From: Roger Pau Monne [mailto:roger.pau@citrix.com]
>> Sent: Wednesday, April 08, 2015 8:57 PM
>> @@ -1484,6 +1489,12 @@ 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 = hvm_hw_io_bitmap;
>> +    else
>> +        d->arch.hvm_domain.io_bitmap = hvm_io_bitmap;
>> +
> 
> if we want to support multiple PVH hardware domains w/ different
> permissions, using global array is not correct here.

There's no such thing like multiple hardware domains. There can
only ever (theoretically that is) be multiple control domains.

Jan

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

[Jan Beulich]

>>> On 08.04.15 at 14:57, <roger.pau@citrix.com> 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.
> 
> Also add the IO ports of the serial console used by Xen to the list of not
> accessible IO ports.

So why is what ns16550_endboot() does not sufficient?

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -82,6 +82,10 @@ struct hvm_function_table hvm_funcs __read_mostly;
>  unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
>      hvm_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];
>  
> +/* I/O permission bitmap for HVM hardware domain */
> +unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
> +    hvm_hw_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];

This should be allocated memory, as it's needed only in the PVH
Dom0 case.

> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -72,6 +72,7 @@ static int construct_vmcb(struct vcpu *v)
>  {
>      struct arch_svm_struct *arch_svm = &v->arch.hvm_svm;
>      struct vmcb_struct *vmcb = arch_svm->vmcb;
> +    struct domain *d = v->domain;

I don't see the value of this variable.

> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -986,8 +986,10 @@ 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((char *)d->arch.hvm_domain.io_bitmap + 0));
> +    __vmwrite(IO_BITMAP_B,
> +              virt_to_maddr((char *)d->arch.hvm_domain.io_bitmap + PAGE_SIZE));

Please drop the bogus cast and pointless addition of zero from the
first of these. I'd prefer it to be done without cast on the second one
too.

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3118,6 +3118,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>              uint16_t port = (exit_qualification >> 16) & 0xFFFF;
>              int bytes = (exit_qualification & 0x07) + 1;
>              int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;
> +
>              if ( handle_pio(port, bytes, dir) )
>                  update_guest_eip(); /* Safe: IN, OUT */
>          }

This is a valid style correction, but being the only change to this file
it doesn't belong here.

> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -213,6 +213,7 @@ extern struct hvm_function_table hvm_funcs;
>  extern bool_t hvm_enabled;
>  extern bool_t cpu_has_lmsl;
>  extern s8 hvm_port80_allowed;
> +extern unsigned long hvm_hw_io_bitmap[];

This should be declared next to hvm_io_bitmap[] (I don't mind if
you move its declaration here).

Jan

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

[Jan Beulich]

>>> On 09.04.15 at 12:55, <andrew.cooper3@citrix.com> wrote:
> On 08/04/15 13:57, 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.
>>
>> Also add the IO ports of the serial console used by Xen to the list of not
>> accessible IO ports.
> 
> Thankyou for looking into this.  I think it is the correct general
> direction, but I do have some questions/thoughts about this area.
> 
> I know that the current implementation is that dom0 is whitelisted and
> can play with everything, but is this actually the best API? 
> Conceptually, a better approach would be for dom0 to start with no
> permissions, and explicitly request access (After all, PV and PVH
> domains are expected to know exactly what they are doing under Xen). 

I don't think this would work too well with the AML interpreter. And
it certainly doesn't make sense to have Dom0 request access blindly
for every port an IN or OUT is done against, as that would effectively
be no different from Dom0 being granted access to everything minus
a black list as we do now.

>> @@ -1618,6 +1624,10 @@ int __init construct_dom0(
>>  
>>          pvh_map_all_iomem(d, nr_pages);
>>          pvh_setup_e820(d, nr_pages);
>> +
>> +        for ( i = 0; i < 0x10000; i++ )
>> +            if ( ioports_access_permitted(d, i, i) )
>> +                __clear_bit(i, hvm_hw_io_bitmap);
> 
> (There is surely a more efficient way of doing this?  If there isn't,
> there probably should be)
> 
> There is also a boundary issue between VT-x and SVM.
> 
> For VT-x, the IO bitmap is 2 pages.  For SVM, it is 2 pages and 3 bits. 
> I suspect the difference is to do with the handling of a 4byte write to
> port 0xffff.  I think you might need to check "i < 0x10003" instead.

I don't think we should ever grant access to ports 0x10000...0x10003,
i.e. not clearing those bits seems quite fine to me.

Jan

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

[Roger Pau Monné]

El 08/04/15 a les 14.57, Roger Pau Monne ha escrit:
> 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.
> 
> Also add the IO ports of the serial console used by Xen to the list of not
> accessible IO ports.

I have one question about the current IO port handling for PVH guests
(DomU and Dom0). There's some code right now in vmx_vmexit_handler
(EXIT_REASON_IO_INSTRUCTION) that's kind PVH specific:

if ( exit_qualification & 0x10 )
{
    /* INS, OUTS */
    if ( unlikely(is_pvh_vcpu(v)) /* PVH fixme */ ||
         !handle_mmio() )
        hvm_inject_hw_exception(TRAP_gp_fault, 0);
}
else
{
    /* IN, OUT */
    uint16_t port = (exit_qualification >> 16) & 0xFFFF;
    int bytes = (exit_qualification & 0x07) + 1;
    int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;

    if ( handle_pio(port, bytes, dir) )
        update_guest_eip(); /* Safe: IN, OUT */
}

Is there any need for DomUs to access the IO ports? I know that FreeBSD
will poke at some of them during boot to scan for devices, but I'm not
sure if we could just make them noops in the PVH case and simply return
garbage.

Also, once this is set the PVH Specification document should be updated
to reflect what can guests expect when poking at IO ports.

Roger.

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

[Andrew Cooper]

On 14/04/15 11:01, Roger Pau Monné wrote:
> El 08/04/15 a les 14.57, Roger Pau Monne ha escrit:
>> 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.
>>
>> Also add the IO ports of the serial console used by Xen to the list of not
>> accessible IO ports.
> I have one question about the current IO port handling for PVH guests
> (DomU and Dom0). There's some code right now in vmx_vmexit_handler
> (EXIT_REASON_IO_INSTRUCTION) that's kind PVH specific:
>
> if ( exit_qualification & 0x10 )
> {
>     /* INS, OUTS */
>     if ( unlikely(is_pvh_vcpu(v)) /* PVH fixme */ ||
>          !handle_mmio() )
>         hvm_inject_hw_exception(TRAP_gp_fault, 0);
> }
> else
> {
>     /* IN, OUT */
>     uint16_t port = (exit_qualification >> 16) & 0xFFFF;
>     int bytes = (exit_qualification & 0x07) + 1;
>     int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;
>
>     if ( handle_pio(port, bytes, dir) )
>         update_guest_eip(); /* Safe: IN, OUT */
> }
>
> Is there any need for DomUs to access the IO ports?

In the case of PCI passthrough, the guest may need to use a devices IO BARs.

However, PCI passthrough and PVH is still a very open question, so
making a change here isn't really breaking anything.

> I know that FreeBSD
> will poke at some of them during boot to scan for devices, but I'm not
> sure if we could just make them noops in the PVH case and simply return
> garbage.

If anything, ~0 is what should be returned to match real hardware.

~Andrew

>
> Also, once this is set the PVH Specification document should be updated
> to reflect what can guests expect when poking at IO ports.
>
> Roger.


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

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

[Jan Beulich]

>>> On 14.04.15 at 12:01, <roger.pau@citrix.com> wrote:
> I have one question about the current IO port handling for PVH guests
> (DomU and Dom0). There's some code right now in vmx_vmexit_handler
> (EXIT_REASON_IO_INSTRUCTION) that's kind PVH specific:
> 
> if ( exit_qualification & 0x10 )
> {
>     /* INS, OUTS */
>     if ( unlikely(is_pvh_vcpu(v)) /* PVH fixme */ ||
>          !handle_mmio() )
>         hvm_inject_hw_exception(TRAP_gp_fault, 0);
> }
> else
> {
>     /* IN, OUT */
>     uint16_t port = (exit_qualification >> 16) & 0xFFFF;
>     int bytes = (exit_qualification & 0x07) + 1;
>     int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;
> 
>     if ( handle_pio(port, bytes, dir) )
>         update_guest_eip(); /* Safe: IN, OUT */
> }
> 
> Is there any need for DomUs to access the IO ports? I know that FreeBSD
> will poke at some of them during boot to scan for devices, but I'm not
> sure if we could just make them noops in the PVH case and simply return
> garbage.

The PVH special case quoted above is there only to prevent
reaching handle_mmio(). The non-string "else" path was enabled
solely on the basis that it was possible to be made work without
much extra effort. And while (without pass-through) it shouldn't
be needed for PVH DomU-s, it also should do no harm.

Jan

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

[Roger Pau Monné]

Hello,

El 14/04/15 a les 12.31, Jan Beulich ha escrit:
>>>> On 14.04.15 at 12:01, <roger.pau@citrix.com> wrote:
>> I have one question about the current IO port handling for PVH guests
>> (DomU and Dom0). There's some code right now in vmx_vmexit_handler
>> (EXIT_REASON_IO_INSTRUCTION) that's kind PVH specific:
>>
>> if ( exit_qualification & 0x10 )
>> {
>>     /* INS, OUTS */
>>     if ( unlikely(is_pvh_vcpu(v)) /* PVH fixme */ ||
>>          !handle_mmio() )
>>         hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> }
>> else
>> {
>>     /* IN, OUT */
>>     uint16_t port = (exit_qualification >> 16) & 0xFFFF;
>>     int bytes = (exit_qualification & 0x07) + 1;
>>     int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;
>>
>>     if ( handle_pio(port, bytes, dir) )
>>         update_guest_eip(); /* Safe: IN, OUT */
>> }
>>
>> Is there any need for DomUs to access the IO ports? I know that FreeBSD
>> will poke at some of them during boot to scan for devices, but I'm not
>> sure if we could just make them noops in the PVH case and simply return
>> garbage.
> 
> The PVH special case quoted above is there only to prevent
> reaching handle_mmio(). 

Injecting a GP seems like a little bit too much for trapped INS/OUTS, I
would rather just drop/ignore them if possible.

> The non-string "else" path was enabled
> solely on the basis that it was possible to be made work without
> much extra effort. And while (without pass-through) it shouldn't
> be needed for PVH DomU-s, it also should do no harm.

Ack.

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

[Jan Beulich]

>>> On 14.04.15 at 12:45, <roger.pau@citrix.com> wrote:
> Hello,
> 
> El 14/04/15 a les 12.31, Jan Beulich ha escrit:
>>>>> On 14.04.15 at 12:01, <roger.pau@citrix.com> wrote:
>>> I have one question about the current IO port handling for PVH guests
>>> (DomU and Dom0). There's some code right now in vmx_vmexit_handler
>>> (EXIT_REASON_IO_INSTRUCTION) that's kind PVH specific:
>>>
>>> if ( exit_qualification & 0x10 )
>>> {
>>>     /* INS, OUTS */
>>>     if ( unlikely(is_pvh_vcpu(v)) /* PVH fixme */ ||
>>>          !handle_mmio() )
>>>         hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>> }
>>> else
>>> {
>>>     /* IN, OUT */
>>>     uint16_t port = (exit_qualification >> 16) & 0xFFFF;
>>>     int bytes = (exit_qualification & 0x07) + 1;
>>>     int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;
>>>
>>>     if ( handle_pio(port, bytes, dir) )
>>>         update_guest_eip(); /* Safe: IN, OUT */
>>> }
>>>
>>> Is there any need for DomUs to access the IO ports? I know that FreeBSD
>>> will poke at some of them during boot to scan for devices, but I'm not
>>> sure if we could just make them noops in the PVH case and simply return
>>> garbage.
>> 
>> The PVH special case quoted above is there only to prevent
>> reaching handle_mmio(). 
> 
> Injecting a GP seems like a little bit too much for trapped INS/OUTS, I
> would rather just drop/ignore them if possible.

Dropping them means (silent) data corruption, whereas #GP is a
clear sign that something went wrong. In the end the case will
need to be properly handled anyway, and learning about this
becoming an immediate need will be better through a plainly
crashed domain than through one that (perhaps quite a long
period of time later) died in some obscure way.

Jan