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

["Jan Beulich" <jbeulich@xxxxxxxx>]

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

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

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

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

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

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

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

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

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

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

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

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

This comment needs updating.

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

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

Jan


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