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

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

[Roger Pau Monne]

This version include splitting changes into separate patches, specially 
those that also affect PV guests.

[PATCH v5 1/3] 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 v4:
 - Split changes also affecting PV to a separate patch.
 - Use int with __clear_bit.
 - Drop pointless cast in vmcb.c.
 - Make HVM_IOBITMAP_SIZE contain the size of the io bitmap pages in bytes.
 - Make setup_io_bitmap a hardware domain specific function, and allow it to
   work with late hw domain init.

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/hvm/hvm.c           | 24 ++++++++++++++++++++++--
 xen/arch/x86/hvm/svm/vmcb.c      |  2 +-
 xen/arch/x86/hvm/vmx/vmcs.c      |  5 +++--
 xen/arch/x86/setup.c             | 29 +++++++++++++++++++++++++++++
 xen/common/domain.c              |  2 ++
 xen/include/asm-x86/hvm/domain.h |  2 ++
 xen/include/asm-x86/setup.h      |  1 +
 7 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 3a09439..ea052c4 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -77,9 +77,13 @@ integer_param("hvm_debug", opt_hvm_debug_level);
 
 struct hvm_function_table hvm_funcs __read_mostly;
 
-/* I/O permission bitmap is globally shared by all HVM guests. */
+#define HVM_IOBITMAP_SIZE   (3*PAGE_SIZE)
+/*
+ * The I/O permission bitmap is globally shared by all HVM guests except
+ * the hardware domain that has a more permissive IO bitmap.
+ */
 unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
-    hvm_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];
+    hvm_io_bitmap[HVM_IOBITMAP_SIZE/BYTES_PER_LONG];
 
 /* Xen command-line option to enable HAP */
 static bool_t __initdata opt_hap_enabled = 1;
@@ -1461,6 +1465,20 @@ 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(HVM_IOBITMAP_SIZE, PAGE_SIZE);
+        if ( d->arch.hvm_domain.io_bitmap == NULL )
+        {
+            rc = -ENOMEM;
+            goto fail1;
+        }
+        memset(d->arch.hvm_domain.io_bitmap, ~0, HVM_IOBITMAP_SIZE);
+    }
+    else
+        d->arch.hvm_domain.io_bitmap = hvm_io_bitmap;
+
     if ( is_pvh_domain(d) )
     {
         register_portio_handler(d, 0, 0x10003, handle_pvh_io);
@@ -1496,6 +1514,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..10afd44 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -118,7 +118,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 = 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 3123706..f62aa90 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1032,8 +1032,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..3b9aee5 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1446,6 +1446,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     dmi_end_boot();
 
+    if ( is_hardware_domain(dom0) )
+        setup_io_bitmap(dom0);
+
     system_state = SYS_STATE_active;
 
     domain_unpause_by_systemcontroller(dom0);
@@ -1509,6 +1512,32 @@ int __hwdom_init xen_in_range(unsigned long mfn)
     return 0;
 }
 
+static int __hwdom_init io_bitmap_cb(unsigned long s, unsigned long e,
+                                     void *ctx)
+{
+    struct domain *d = ctx;
+    int i;
+
+    ASSERT(s <= INT_MAX && e <= INT_MAX);
+    for ( i = s; i <= e; i++ )
+        __clear_bit(i, d->arch.hvm_domain.io_bitmap);
+
+    return 0;
+}
+
+void __hwdom_init setup_io_bitmap(struct domain *d)
+{
+    int rc;
+
+    ASSERT(is_hardware_domain(d));
+    if ( has_hvm_container_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/common/domain.c b/xen/common/domain.c
index 6803c4d..8e0a7cd 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -225,6 +225,8 @@ static int late_hwdom_init(struct domain *d)
 
     iommu_hwdom_init(d);
 
+    setup_io_bitmap(d);
+
     return rv;
 #else
     return 0;
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 0f8b19a..e250791 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -141,6 +141,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

[PATCH v5 2/3] xen: add the RTC io space to the blocked access list

[Roger Pau Monne]

Previously this was done ad-hoc in admin_io_okay.

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_build.c | 3 +++
 xen/arch/x86/traps.c        | 4 ----
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 03e4bfe..2a23746 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -37,6 +37,7 @@
 #include <asm/io_apic.h>
 #include <asm/hap.h>
 #include <asm/hpet.h>
+#include <asm/mc146818rtc.h>
 
 #include <public/version.h>
 
@@ -1548,6 +1549,8 @@ int __init construct_dom0(
         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);
+    /* Never permit direct access to the RTC/CMOS registers. */
+    rc |= ioports_deny_access(d, RTC_PORT(0), RTC_PORT(1));
     /* Command-line I/O ranges. */
     process_dom0_ioports_disable(d);
 
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 22cdfc4..0b0c5e9 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1765,10 +1765,6 @@ static int admin_io_okay(
     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);
 }
 
-- 
1.9.5 (Apple Git-50.3)


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

[PATCH v5 3/3] xen: block access to IO port 0xcf9

[Roger Pau Monne]

This port is used by PM1a and should not be accessed directly by Dom0. This
also premits trapping 2 and 4 byte accesses to 0xcf8, which need to be
handled by the hypervisor.

Also, since admin_io_okay is now a wrapper around ioports_access_permitted
remove it.

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_build.c |  2 ++
 xen/arch/x86/traps.c        | 23 ++++-------------------
 2 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 2a23746..ecc872d 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -1551,6 +1551,8 @@ int __init construct_dom0(
     rc |= ioports_deny_access(d, 0xcfc, 0xcff);
     /* Never permit direct access to the RTC/CMOS registers. */
     rc |= ioports_deny_access(d, RTC_PORT(0), RTC_PORT(1));
+    /* PM1a */
+    rc |= ioports_deny_access(d, 0xcf9, 0xcf9);
     /* Command-line I/O ranges. */
     process_dom0_ioports_disable(d);
 
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 0b0c5e9..8d2bbb2 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1753,21 +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;
-
-    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;
@@ -1809,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 )
         {
@@ -1873,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:
@@ -2224,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);            
@@ -2254,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);            
-- 
1.9.5 (Apple Git-50.3)


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

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

[Jan Beulich]

>>> On 07.05.15 at 16:54, <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.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one minor remark:

> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1032,8 +1032,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));

Why not

    __vmwrite(IO_BITMAP_B, virt_to_maddr(d->arch.hvm_domain.io_bitmap) + PAGE_SIZE);

? (If you agree I can change this while committing.)

Jan

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

Re: [PATCH v5 2/3] xen: add the RTC io space to the blocked access list

[Jan Beulich]

>>> On 07.05.15 at 16:54, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -37,6 +37,7 @@
>  #include <asm/io_apic.h>
>  #include <asm/hap.h>
>  #include <asm/hpet.h>
> +#include <asm/mc146818rtc.h>
>  
>  #include <public/version.h>
>  
> @@ -1548,6 +1549,8 @@ int __init construct_dom0(
>          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);
> +    /* Never permit direct access to the RTC/CMOS registers. */
> +    rc |= ioports_deny_access(d, RTC_PORT(0), RTC_PORT(1));
>      /* Command-line I/O ranges. */
>      process_dom0_ioports_disable(d);

This indeed is a direct replacement of what we had before (and
hence I'm considering the patch okay as is). I nevertheless wonder
how useful it is: Frequently the RTC/CMOS can also be accessed
via ports 72 and 73 (and then all 256 bytes of CMOS), and often
address decoding is even more lax (e.g. aliasing the whole 70...77
range to 70/71 or 70...73).

Jan

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

[Roger Pau Monné]

El 07/05/15 a les 17.10, Jan Beulich ha escrit:
>>>> On 07.05.15 at 16:54, <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.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one minor remark:

Thanks.

>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -1032,8 +1032,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));
> 
> Why not
> 
>     __vmwrite(IO_BITMAP_B, virt_to_maddr(d->arch.hvm_domain.io_bitmap) + PAGE_SIZE);
> 
> ? (If you agree I can change this while committing.)

Sure, please go ahead.

Roger.


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

Re: [PATCH v5 3/3] xen: block access to IO port 0xcf9

[Jan Beulich]

>>> On 07.05.15 at 16:54, <roger.pau@citrix.com> wrote:
> This port is used by PM1a and should not be accessed directly by Dom0.

I don't think this is unconditionally PM1a - that should be read out
of the FADT if at all. I also don't think port CF9 universally serves
as the port to do reboots. I.e. I don't think this should be done
unconditionally.

> This
> also premits trapping 2 and 4 byte accesses to 0xcf8, which need to be
> handled by the hypervisor.

Only 4-byte ones need to be handled in the hypervisor, and you're
not adding any code forcing 2-byte ones to be allowed through. I.e.

> Also, since admin_io_okay is now a wrapper around ioports_access_permitted
> remove it.

... this should not be the final result afaict.

Jan

Re: [PATCH v5 3/3] xen: block access to IO port 0xcf9

[Roger Pau Monné]

Hello,

El 07/05/15 a les 17.22, Jan Beulich ha escrit:
>>>> On 07.05.15 at 16:54, <roger.pau@citrix.com> wrote:
>> This port is used by PM1a and should not be accessed directly by Dom0.
> 
> I don't think this is unconditionally PM1a - that should be read out
> of the FADT if at all. I also don't think port CF9 universally serves
> as the port to do reboots. I.e. I don't think this should be done
> unconditionally.
> 
>> This
>> also premits trapping 2 and 4 byte accesses to 0xcf8, which need to be
>> handled by the hypervisor.
> 
> Only 4-byte ones need to be handled in the hypervisor, and you're
> not adding any code forcing 2-byte ones to be allowed through. I.e.
> 
>> Also, since admin_io_okay is now a wrapper around ioports_access_permitted
>> remove it.
> 
> ... this should not be the final result afaict.

Thanks for the comments. IMHO the best way to deal with this is to not
add anything in the 0xcf8-0xcfb to ioports_deny_access, leaving
admin_io_okay as-is. Then in the PVH io bitmap blocking access to
0xcf8-0xcfb in order to trap accesses to that range. Does that sound
suitable?

Roger.

Re: [PATCH v5 3/3] xen: block access to IO port 0xcf9

[Jan Beulich]

>>> On 07.05.15 at 17:57, <roger.pau@citrix.com> wrote:
> Thanks for the comments. IMHO the best way to deal with this is to not
> add anything in the 0xcf8-0xcfb to ioports_deny_access, leaving
> admin_io_okay as-is. Then in the PVH io bitmap blocking access to
> 0xcf8-0xcfb in order to trap accesses to that range. Does that sound
> suitable?

Provided other than 4-byte accesses to that port range get handled
correctly (read: the same as PV) for PVH, sure.

Jan

Re: [PATCH v5 2/3] xen: add the RTC io space to the blocked access list

[Jan Beulich]

>>> On 07.05.15 at 16:54, <roger.pau@citrix.com> wrote:
> @@ -1548,6 +1549,8 @@ int __init construct_dom0(
>          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);
> +    /* Never permit direct access to the RTC/CMOS registers. */
> +    rc |= ioports_deny_access(d, RTC_PORT(0), RTC_PORT(1));

Looks like I prematurely committed this: Did you test that Dom0
still can access CMOS/RTC with that change? I ask because the
ioports_access_permitted() checks in guest_io_{read,write}()
now ought to fail for Dom0... (Apart from that on second thought
it looks wrong also conceptionally - we don't want to deny Dom0
access to these ports, we just don't want it to access them
directly.)

Jan

Re: [PATCH v5 2/3] xen: add the RTC io space to the blocked access list

[Roger Pau Monné]

El 08/05/15 a les 13.50, Jan Beulich ha escrit:
>>>> On 07.05.15 at 16:54, <roger.pau@citrix.com> wrote:
>> @@ -1548,6 +1549,8 @@ int __init construct_dom0(
>>          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);
>> +    /* Never permit direct access to the RTC/CMOS registers. */
>> +    rc |= ioports_deny_access(d, RTC_PORT(0), RTC_PORT(1));
> 
> Looks like I prematurely committed this: Did you test that Dom0
> still can access CMOS/RTC with that change? I ask because the
> ioports_access_permitted() checks in guest_io_{read,write}()
> now ought to fail for Dom0... (Apart from that on second thought
> it looks wrong also conceptionally - we don't want to deny Dom0
> access to these ports, we just don't want it to access them
> directly.)

Yes, it looks like this needs to be reverted. I think we need to do
something similar to what I've done with 0xcf8; don't add the ports to
ioports_deny_access and just trap them for PVH in setup_io_bitmap.

Roger.

Re: [PATCH v5 2/3] xen: add the RTC io space to the blocked access list

[Jan Beulich]

>>> On 08.05.15 at 13:55, <roger.pau@citrix.com> wrote:
> El 08/05/15 a les 13.50, Jan Beulich ha escrit:
>>>>> On 07.05.15 at 16:54, <roger.pau@citrix.com> wrote:
>>> @@ -1548,6 +1549,8 @@ int __init construct_dom0(
>>>          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);
>>> +    /* Never permit direct access to the RTC/CMOS registers. */
>>> +    rc |= ioports_deny_access(d, RTC_PORT(0), RTC_PORT(1));
>> 
>> Looks like I prematurely committed this: Did you test that Dom0
>> still can access CMOS/RTC with that change? I ask because the
>> ioports_access_permitted() checks in guest_io_{read,write}()
>> now ought to fail for Dom0... (Apart from that on second thought
>> it looks wrong also conceptionally - we don't want to deny Dom0
>> access to these ports, we just don't want it to access them
>> directly.)
> 
> Yes, it looks like this needs to be reverted. I think we need to do
> something similar to what I've done with 0xcf8; don't add the ports to
> ioports_deny_access and just trap them for PVH in setup_io_bitmap.

Right. Hence I'll ditch that v2 patch, awaiting a v3 doing both.

Jan

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

[Tian, Kevin]

> From: Roger Pau Monne [mailto:roger.pau@citrix.com]
> Sent: Thursday, May 07, 2015 10:54 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.
> 
> 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>

Acked-by: Kevin Tian <kevin.tian@intel.com>

Thanks
Kevin

> ---
> Changes since v4:
>  - Split changes also affecting PV to a separate patch.
>  - Use int with __clear_bit.
>  - Drop pointless cast in vmcb.c.
>  - Make HVM_IOBITMAP_SIZE contain the size of the io bitmap pages in bytes.
>  - Make setup_io_bitmap a hardware domain specific function, and allow it to
>    work with late hw domain init.
> 
> 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/hvm/hvm.c           | 24 ++++++++++++++++++++++--
>  xen/arch/x86/hvm/svm/vmcb.c      |  2 +-
>  xen/arch/x86/hvm/vmx/vmcs.c      |  5 +++--
>  xen/arch/x86/setup.c             | 29
> +++++++++++++++++++++++++++++
>  xen/common/domain.c              |  2 ++
>  xen/include/asm-x86/hvm/domain.h |  2 ++
>  xen/include/asm-x86/setup.h      |  1 +
>  7 files changed, 60 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 3a09439..ea052c4 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -77,9 +77,13 @@ integer_param("hvm_debug", opt_hvm_debug_level);
> 
>  struct hvm_function_table hvm_funcs __read_mostly;
> 
> -/* I/O permission bitmap is globally shared by all HVM guests. */
> +#define HVM_IOBITMAP_SIZE   (3*PAGE_SIZE)
> +/*
> + * The I/O permission bitmap is globally shared by all HVM guests except
> + * the hardware domain that has a more permissive IO bitmap.
> + */
>  unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
> -    hvm_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];
> +    hvm_io_bitmap[HVM_IOBITMAP_SIZE/BYTES_PER_LONG];
> 
>  /* Xen command-line option to enable HAP */
>  static bool_t __initdata opt_hap_enabled = 1;
> @@ -1461,6 +1465,20 @@ 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(HVM_IOBITMAP_SIZE,
> PAGE_SIZE);
> +        if ( d->arch.hvm_domain.io_bitmap == NULL )
> +        {
> +            rc = -ENOMEM;
> +            goto fail1;
> +        }
> +        memset(d->arch.hvm_domain.io_bitmap, ~0,
> HVM_IOBITMAP_SIZE);
> +    }
> +    else
> +        d->arch.hvm_domain.io_bitmap = hvm_io_bitmap;
> +
>      if ( is_pvh_domain(d) )
>      {
>          register_portio_handler(d, 0, 0x10003, handle_pvh_io);
> @@ -1496,6 +1514,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..10afd44 100644
> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -118,7 +118,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 =
> 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 3123706..f62aa90 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1032,8 +1032,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..3b9aee5 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1446,6 +1446,9 @@ void __init noreturn __start_xen(unsigned long
> mbi_p)
> 
>      dmi_end_boot();
> 
> +    if ( is_hardware_domain(dom0) )
> +        setup_io_bitmap(dom0);
> +
>      system_state = SYS_STATE_active;
> 
>      domain_unpause_by_systemcontroller(dom0);
> @@ -1509,6 +1512,32 @@ int __hwdom_init xen_in_range(unsigned long
> mfn)
>      return 0;
>  }
> 
> +static int __hwdom_init io_bitmap_cb(unsigned long s, unsigned long e,
> +                                     void *ctx)
> +{
> +    struct domain *d = ctx;
> +    int i;
> +
> +    ASSERT(s <= INT_MAX && e <= INT_MAX);
> +    for ( i = s; i <= e; i++ )
> +        __clear_bit(i, d->arch.hvm_domain.io_bitmap);
> +
> +    return 0;
> +}
> +
> +void __hwdom_init setup_io_bitmap(struct domain *d)
> +{
> +    int rc;
> +
> +    ASSERT(is_hardware_domain(d));
> +    if ( has_hvm_container_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/common/domain.c b/xen/common/domain.c
> index 6803c4d..8e0a7cd 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -225,6 +225,8 @@ static int late_hwdom_init(struct domain *d)
> 
>      iommu_hwdom_init(d);
> 
> +    setup_io_bitmap(d);
> +
>      return rv;
>  #else
>      return 0;
> diff --git a/xen/include/asm-x86/hvm/domain.h
> b/xen/include/asm-x86/hvm/domain.h
> index 0f8b19a..e250791 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -141,6 +141,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