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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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

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

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


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