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

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

[Roger Pau Monne]

Changes in this version include a build fix for XSM and trapping 0xcf8 and 
the RTC ports for PVH instead of adding them to ioports_deny_access.

[PATCH v6 1/2] 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 v5:
 - Fix build with XSM (CONFIG_LATE_HWDOM).

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..a714549 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);
 
     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..b6b0034 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -42,6 +42,7 @@
 #include <xsm/xsm.h>
 #include <xen/trace.h>
 #include <xen/tmem.h>
+#include <asm/setup.h>
 
 /* Linux config option: propageted to domain0 */
 /* xen_processor_pmbits: xen control Cx, Px, ... */
@@ -219,6 +220,7 @@ static int late_hwdom_init(struct domain *d)
     rangeset_swap(d->iomem_caps, dom0->iomem_caps);
 #ifdef CONFIG_X86
     rangeset_swap(d->arch.ioport_caps, dom0->arch.ioport_caps);
+    setup_io_bitmap(d);
 #endif
 
     rcu_unlock_domain(dom0);
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

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

[Jan Beulich]

>>> On 11.05.15 at 16:57, <roger.pau@citrix.com> wrote:
> --- 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);

Is it indeed possible for is_hardware_domain() to be false for dom0
at this point?

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -42,6 +42,7 @@
>  #include <xsm/xsm.h>
>  #include <xen/trace.h>
>  #include <xen/tmem.h>
> +#include <asm/setup.h>
>  
>  /* Linux config option: propageted to domain0 */
>  /* xen_processor_pmbits: xen control Cx, Px, ... */
> @@ -219,6 +220,7 @@ static int late_hwdom_init(struct domain *d)
>      rangeset_swap(d->iomem_caps, dom0->iomem_caps);
>  #ifdef CONFIG_X86
>      rangeset_swap(d->arch.ioport_caps, dom0->arch.ioport_caps);
> +    setup_io_bitmap(d);
>  #endif

Considering that rangesets are getting swapped rather than
copied, I think you also need to reset Dom0's I/O bitmap here
to the ordinary, non-hardware domain one.

Jan

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

[Roger Pau Monné]

El 13/05/15 a les 11.53, Jan Beulich ha escrit:
>>>> On 11.05.15 at 16:57, <roger.pau@citrix.com> wrote:
>> --- 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);
> 
> Is it indeed possible for is_hardware_domain() to be false for dom0
> at this point?

No, I will remove this check in the next version.

>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -42,6 +42,7 @@
>>  #include <xsm/xsm.h>
>>  #include <xen/trace.h>
>>  #include <xen/tmem.h>
>> +#include <asm/setup.h>
>>  
>>  /* Linux config option: propageted to domain0 */
>>  /* xen_processor_pmbits: xen control Cx, Px, ... */
>> @@ -219,6 +220,7 @@ static int late_hwdom_init(struct domain *d)
>>      rangeset_swap(d->iomem_caps, dom0->iomem_caps);
>>  #ifdef CONFIG_X86
>>      rangeset_swap(d->arch.ioport_caps, dom0->arch.ioport_caps);
>> +    setup_io_bitmap(d);
>>  #endif
> 
> Considering that rangesets are getting swapped rather than
> copied, I think you also need to reset Dom0's I/O bitmap here
> to the ordinary, non-hardware domain one.

Yes. Would it be fine to memset it and just call setup_io_bitmap on it
again, or would you prefer to exchange it with the static one and free it?

Roger.

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

[Jan Beulich]

>>> On 14.05.15 at 17:27, <roger.pau@citrix.com> wrote:
> El 13/05/15 a les 11.53, Jan Beulich ha escrit:
>>>>> On 11.05.15 at 16:57, <roger.pau@citrix.com> wrote:
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -42,6 +42,7 @@
>>>  #include <xsm/xsm.h>
>>>  #include <xen/trace.h>
>>>  #include <xen/tmem.h>
>>> +#include <asm/setup.h>
>>>  
>>>  /* Linux config option: propageted to domain0 */
>>>  /* xen_processor_pmbits: xen control Cx, Px, ... */
>>> @@ -219,6 +220,7 @@ static int late_hwdom_init(struct domain *d)
>>>      rangeset_swap(d->iomem_caps, dom0->iomem_caps);
>>>  #ifdef CONFIG_X86
>>>      rangeset_swap(d->arch.ioport_caps, dom0->arch.ioport_caps);
>>> +    setup_io_bitmap(d);
>>>  #endif
>> 
>> Considering that rangesets are getting swapped rather than
>> copied, I think you also need to reset Dom0's I/O bitmap here
>> to the ordinary, non-hardware domain one.
> 
> Yes. Would it be fine to memset it and just call setup_io_bitmap on it
> again, or would you prefer to exchange it with the static one and free it?

Following how the rangesets are being treated, simply swapping
the two I/O bitmaps would seem to be the right approach here.

Jan

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

[Roger Pau Monné]

El 15/05/15 a les 8.36, Jan Beulich ha escrit:
>>>> On 14.05.15 at 17:27, <roger.pau@citrix.com> wrote:
>> El 13/05/15 a les 11.53, Jan Beulich ha escrit:
>>>>>> On 11.05.15 at 16:57, <roger.pau@citrix.com> wrote:
>>>> --- a/xen/common/domain.c
>>>> +++ b/xen/common/domain.c
>>>> @@ -42,6 +42,7 @@
>>>>  #include <xsm/xsm.h>
>>>>  #include <xen/trace.h>
>>>>  #include <xen/tmem.h>
>>>> +#include <asm/setup.h>
>>>>  
>>>>  /* Linux config option: propageted to domain0 */
>>>>  /* xen_processor_pmbits: xen control Cx, Px, ... */
>>>> @@ -219,6 +220,7 @@ static int late_hwdom_init(struct domain *d)
>>>>      rangeset_swap(d->iomem_caps, dom0->iomem_caps);
>>>>  #ifdef CONFIG_X86
>>>>      rangeset_swap(d->arch.ioport_caps, dom0->arch.ioport_caps);
>>>> +    setup_io_bitmap(d);
>>>>  #endif
>>>
>>> Considering that rangesets are getting swapped rather than
>>> copied, I think you also need to reset Dom0's I/O bitmap here
>>> to the ordinary, non-hardware domain one.
>>
>> Yes. Would it be fine to memset it and just call setup_io_bitmap on it
>> again, or would you prefer to exchange it with the static one and free it?
> 
> Following how the rangesets are being treated, simply swapping
> the two I/O bitmaps would seem to be the right approach here.

AFAICT this requires adding a new hook in hvm_function_table in order to
implement setting the io bitmap for SVM and VMX. I don't have a problem
with that, but it's going to need a separate patch.

Roger.

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

[Jan Beulich]

>>> On 15.05.15 at 09:34, <roger.pau@citrix.com> wrote:
> El 15/05/15 a les 8.36, Jan Beulich ha escrit:
>>>>> On 14.05.15 at 17:27, <roger.pau@citrix.com> wrote:
>>> El 13/05/15 a les 11.53, Jan Beulich ha escrit:
>>>>>>> On 11.05.15 at 16:57, <roger.pau@citrix.com> wrote:
>>>>> --- a/xen/common/domain.c
>>>>> +++ b/xen/common/domain.c
>>>>> @@ -42,6 +42,7 @@
>>>>>  #include <xsm/xsm.h>
>>>>>  #include <xen/trace.h>
>>>>>  #include <xen/tmem.h>
>>>>> +#include <asm/setup.h>
>>>>>  
>>>>>  /* Linux config option: propageted to domain0 */
>>>>>  /* xen_processor_pmbits: xen control Cx, Px, ... */
>>>>> @@ -219,6 +220,7 @@ static int late_hwdom_init(struct domain *d)
>>>>>      rangeset_swap(d->iomem_caps, dom0->iomem_caps);
>>>>>  #ifdef CONFIG_X86
>>>>>      rangeset_swap(d->arch.ioport_caps, dom0->arch.ioport_caps);
>>>>> +    setup_io_bitmap(d);
>>>>>  #endif
>>>>
>>>> Considering that rangesets are getting swapped rather than
>>>> copied, I think you also need to reset Dom0's I/O bitmap here
>>>> to the ordinary, non-hardware domain one.
>>>
>>> Yes. Would it be fine to memset it and just call setup_io_bitmap on it
>>> again, or would you prefer to exchange it with the static one and free it?
>> 
>> Following how the rangesets are being treated, simply swapping
>> the two I/O bitmaps would seem to be the right approach here.
> 
> AFAICT this requires adding a new hook in hvm_function_table in order to
> implement setting the io bitmap for SVM and VMX. I don't have a problem
> with that, but it's going to need a separate patch.

Right - if there is nothing like that currently, it'll need to be added.
Of course you may want to get Daniel de Graaf's (who originally
added this non-Dom0 hardware domain code, now Cc-ed) input on
the above outline approach before going that route...

Jan

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

[Daniel De Graaf]

On 05/15/2015 03:42 AM, Jan Beulich wrote:
>>>> On 15.05.15 at 09:34, <roger.pau@citrix.com> wrote:
>> El 15/05/15 a les 8.36, Jan Beulich ha escrit:
>>>>>> On 14.05.15 at 17:27, <roger.pau@citrix.com> wrote:
>>>> El 13/05/15 a les 11.53, Jan Beulich ha escrit:
>>>>>>>> On 11.05.15 at 16:57, <roger.pau@citrix.com> wrote:
>>>>>> --- a/xen/common/domain.c
>>>>>> +++ b/xen/common/domain.c
>>>>>> @@ -42,6 +42,7 @@
>>>>>>   #include <xsm/xsm.h>
>>>>>>   #include <xen/trace.h>
>>>>>>   #include <xen/tmem.h>
>>>>>> +#include <asm/setup.h>
>>>>>>
>>>>>>   /* Linux config option: propageted to domain0 */
>>>>>>   /* xen_processor_pmbits: xen control Cx, Px, ... */
>>>>>> @@ -219,6 +220,7 @@ static int late_hwdom_init(struct domain *d)
>>>>>>       rangeset_swap(d->iomem_caps, dom0->iomem_caps);
>>>>>>   #ifdef CONFIG_X86
>>>>>>       rangeset_swap(d->arch.ioport_caps, dom0->arch.ioport_caps);
>>>>>> +    setup_io_bitmap(d);
>>>>>>   #endif
>>>>>
>>>>> Considering that rangesets are getting swapped rather than
>>>>> copied, I think you also need to reset Dom0's I/O bitmap here
>>>>> to the ordinary, non-hardware domain one.
>>>>
>>>> Yes. Would it be fine to memset it and just call setup_io_bitmap on it
>>>> again, or would you prefer to exchange it with the static one and free it?
>>>
>>> Following how the rangesets are being treated, simply swapping
>>> the two I/O bitmaps would seem to be the right approach here.
>>
>> AFAICT this requires adding a new hook in hvm_function_table in order to
>> implement setting the io bitmap for SVM and VMX. I don't have a problem
>> with that, but it's going to need a separate patch.
>
> Right - if there is nothing like that currently, it'll need to be added.
> Of course you may want to get Daniel de Graaf's (who originally
> added this non-Dom0 hardware domain code, now Cc-ed) input on
> the above outline approach before going that route...

Currently, the only user of this code that I know of is a domain builder
which is a PV stub domain, which may impact a trivial swapping operation
because dom0->arch.hvm_domain will not be valid.  The primary reason why
the rangeset_swap calls are not rangeset_dup is because this stub domain
does not need access to the hardware after the creation of the hardware
domain.

I think it seems cleaner for the IO bitmap of domain 0 to also be cleared
after the hardware domain is created, but it's not really a requirement to
make things work.

-- 
Daniel De Graaf
National Security Agency

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

[Jan Beulich]

>>> On 15.05.15 at 22:09, <dgdegra@tycho.nsa.gov> wrote:
> On 05/15/2015 03:42 AM, Jan Beulich wrote:
>>>>> On 15.05.15 at 09:34, <roger.pau@citrix.com> wrote:
>>> El 15/05/15 a les 8.36, Jan Beulich ha escrit:
>>>>>>> On 14.05.15 at 17:27, <roger.pau@citrix.com> wrote:
>>>>> El 13/05/15 a les 11.53, Jan Beulich ha escrit:
>>>>>>>>> On 11.05.15 at 16:57, <roger.pau@citrix.com> wrote:
>>>>>>> --- a/xen/common/domain.c
>>>>>>> +++ b/xen/common/domain.c
>>>>>>> @@ -42,6 +42,7 @@
>>>>>>>   #include <xsm/xsm.h>
>>>>>>>   #include <xen/trace.h>
>>>>>>>   #include <xen/tmem.h>
>>>>>>> +#include <asm/setup.h>
>>>>>>>
>>>>>>>   /* Linux config option: propageted to domain0 */
>>>>>>>   /* xen_processor_pmbits: xen control Cx, Px, ... */
>>>>>>> @@ -219,6 +220,7 @@ static int late_hwdom_init(struct domain *d)
>>>>>>>       rangeset_swap(d->iomem_caps, dom0->iomem_caps);
>>>>>>>   #ifdef CONFIG_X86
>>>>>>>       rangeset_swap(d->arch.ioport_caps, dom0->arch.ioport_caps);
>>>>>>> +    setup_io_bitmap(d);
>>>>>>>   #endif
>>>>>>
>>>>>> Considering that rangesets are getting swapped rather than
>>>>>> copied, I think you also need to reset Dom0's I/O bitmap here
>>>>>> to the ordinary, non-hardware domain one.
>>>>>
>>>>> Yes. Would it be fine to memset it and just call setup_io_bitmap on it
>>>>> again, or would you prefer to exchange it with the static one and free it?
>>>>
>>>> Following how the rangesets are being treated, simply swapping
>>>> the two I/O bitmaps would seem to be the right approach here.
>>>
>>> AFAICT this requires adding a new hook in hvm_function_table in order to
>>> implement setting the io bitmap for SVM and VMX. I don't have a problem
>>> with that, but it's going to need a separate patch.
>>
>> Right - if there is nothing like that currently, it'll need to be added.
>> Of course you may want to get Daniel de Graaf's (who originally
>> added this non-Dom0 hardware domain code, now Cc-ed) input on
>> the above outline approach before going that route...
> 
> Currently, the only user of this code that I know of is a domain builder
> which is a PV stub domain, which may impact a trivial swapping operation
> because dom0->arch.hvm_domain will not be valid.  The primary reason why
> the rangeset_swap calls are not rangeset_dup is because this stub domain
> does not need access to the hardware after the creation of the hardware
> domain.
> 
> I think it seems cleaner for the IO bitmap of domain 0 to also be cleared
> after the hardware domain is created, but it's not really a requirement to
> make things work.

Hmm - I think the rangeset handling and the I/O bitmap handling ought
to be in sync (even if only to avoid future confusion/questions). And
yes, considering the case of one of the two being PV and the other PVH
is going to be necessary (even if right now only a PV stub domain builder
may exist).

Jan

[PATCH v6 2/2] xen/pvh: trap access to sensitive IO ports

[Roger Pau Monne]

This is needed so Xen can properly trap 4 byte accesses to 0xcf8 in order to
keep consistency with accesses to 0xcfc.

The access to RTC ports also needs to be trapped in order to keep
consistency, this includes RTC_PORT(0) and RTC_PORT(1) (0x70 and 0x71
respectively).

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v2:
 - Trap RTC ports.

Changes since v1:
 - Only trap on accesses to 0xcf8.
---
 xen/arch/x86/setup.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 3b9aee5..79593d9 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -49,6 +49,7 @@
 #include <xen/cpu.h>
 #include <asm/nmi.h>
 #include <asm/alternative.h>
+#include <asm/mc146818rtc.h>
 
 /* opt_nosmp: If true, secondary processors are ignored. */
 static bool_t __initdata opt_nosmp;
@@ -1535,6 +1536,16 @@ void __hwdom_init setup_io_bitmap(struct domain *d)
         rc = rangeset_report_ranges(d->arch.ioport_caps, 0, 0x10000,
                                     io_bitmap_cb, d);
         BUG_ON(rc);
+        /*
+         * NB: we need to trap accesses to 0xcf8 in order
+         * to intercept 4 byte accesses, that need to be
+         * handled by Xen in order to keep consistency.
+         * Access to 1 byte RTC ports also needs to be
+         * trapped in order to keep consistency.
+         */
+        __set_bit(0xcf8, d->arch.hvm_domain.io_bitmap);
+        __set_bit(RTC_PORT(0), d->arch.hvm_domain.io_bitmap);
+        __set_bit(RTC_PORT(1), d->arch.hvm_domain.io_bitmap);
     }
 }
 
-- 
1.9.5 (Apple Git-50.3)


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