Re: [Xen-devel] [PATCH v5 03/11] x86/mmcfg: add handlers for the PVH Dom0 MMCFG areas

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne [mailto:roger.pau@xxxxxxxxxx]
> Sent: 14 August 2017 15:29
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: boris.ostrovsky@xxxxxxxxxx; konrad.wilk@xxxxxxxxxx; Roger Pau Monne
> <roger.pau@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Subject: [PATCH v5 03/11] x86/mmcfg: add handlers for the PVH Dom0
> MMCFG areas
> 
> Introduce a set of handlers for the accesses to the MMCFG areas. Those
> areas are setup based on the contents of the hardware MMCFG tables,
> and the list of handled MMCFG areas is stored inside of the hvm_domain
> struct.
> 
> The read/writes are forwarded to the generic vpci handlers once the
> address is decoded in order to obtain the device and register the
> guest is trying to access.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

> ---
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Paul Durrant <paul.durrant@xxxxxxxxxx>
> ---
> Changes since v4:
>  - Change the attribute of pvh_setup_mmcfg to __hwdom_init.
>  - Try to add as many MMCFG regions as possible, even if one fails to
>    add.
>  - Change some fields of the hvm_mmcfg struct: turn size into a
>    unsigned int, segment into uint16_t and bus into uint8_t.
>  - Convert some address parameters from unsigned long to paddr_t for
>    consistency.
>  - Make vpci_mmcfg_decode_addr return the decoded register in the
>    return of the function.
>  - Introduce a new macro to convert a MMCFG address into a BDF, and
>    use it in vpci_mmcfg_decode_addr to clarify the logic.
>  - In vpci_mmcfg_{read/write} unify the logic for 8B accesses and
>    smaller ones.
>  - Add the __hwdom_init attribute to register_vpci_mmcfg_handler.
>  - Test that reg + size doesn't cross a device boundary.
> 
> Changes since v3:
>  - Propagate changes from previous patches: drop xen_ prefix for vpci
>    functions, pass slot and func instead of devfn and fix the error
>    paths of the MMCFG handlers.
>  - s/ecam/mmcfg/.
>  - Move the destroy code to a separate function, so the hvm_mmcfg
>    struct can be private to hvm/io.c.
>  - Constify the return of vpci_mmcfg_find.
>  - Use d instead of v->domain in vpci_mmcfg_accept.
>  - Allow 8byte accesses to the mmcfg.
> 
> Changes since v1:
>  - Added locking.
> ---
>  xen/arch/x86/hvm/dom0_build.c    |  22 +++++
>  xen/arch/x86/hvm/hvm.c           |   3 +
>  xen/arch/x86/hvm/io.c            | 183
> ++++++++++++++++++++++++++++++++++++++-
>  xen/include/asm-x86/hvm/domain.h |   3 +
>  xen/include/asm-x86/hvm/io.h     |   7 ++
>  xen/include/asm-x86/pci.h        |   2 +
>  6 files changed, 219 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/dom0_build.c
> b/xen/arch/x86/hvm/dom0_build.c
> index 0e7d06be95..04a8682d33 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -38,6 +38,8 @@
>  #include <public/hvm/hvm_info_table.h>
>  #include <public/hvm/hvm_vcpu.h>
> 
> +#include "../x86_64/mmconfig.h"
> +
>  /*
>   * Have the TSS cover the ISA port range, which makes it
>   * - 104 bytes base structure
> @@ -1041,6 +1043,24 @@ static int __init pvh_setup_acpi(struct domain *d,
> paddr_t start_info)
>      return 0;
>  }
> 
> +static void __hwdom_init pvh_setup_mmcfg(struct domain *d)
> +{
> +    unsigned int i;
> +    int rc;
> +
> +    for ( i = 0; i < pci_mmcfg_config_num; i++ )
> +    {
> +        rc = register_vpci_mmcfg_handler(d, pci_mmcfg_config[i].address,
> +                                         
> pci_mmcfg_config[i].start_bus_number,
> +                                         pci_mmcfg_config[i].end_bus_number,
> +                                         pci_mmcfg_config[i].pci_segment);
> +        if ( rc )
> +            printk("Unable to setup MMCFG handler at %#lx for segment %u\n",
> +                   pci_mmcfg_config[i].address,
> +                   pci_mmcfg_config[i].pci_segment);
> +    }
> +}
> +
>  int __init dom0_construct_pvh(struct domain *d, const module_t *image,
>                                unsigned long image_headroom,
>                                module_t *initrd,
> @@ -1090,6 +1110,8 @@ int __init dom0_construct_pvh(struct domain *d,
> const module_t *image,
>          return rc;
>      }
> 
> +    pvh_setup_mmcfg(d);
> +
>      panic("Building a PVHv2 Dom0 is not yet supported.");
>      return 0;
>  }
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index cc73df8dc7..3168973820 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -583,6 +583,7 @@ int hvm_domain_initialise(struct domain *d, unsigned
> long domcr_flags,
>      spin_lock_init(&d->arch.hvm_domain.write_map.lock);
>      INIT_LIST_HEAD(&d->arch.hvm_domain.write_map.list);
>      INIT_LIST_HEAD(&d->arch.hvm_domain.g2m_ioport_list);
> +    INIT_LIST_HEAD(&d->arch.hvm_domain.mmcfg_regions);
> 
>      rc = create_perdomain_mapping(d, PERDOMAIN_VIRT_START, 0, NULL,
> NULL);
>      if ( rc )
> @@ -728,6 +729,8 @@ void hvm_domain_destroy(struct domain *d)
>          list_del(&ioport->list);
>          xfree(ioport);
>      }
> +
> +    destroy_vpci_mmcfg(&d->arch.hvm_domain.mmcfg_regions);
>  }
> 
>  static int hvm_save_tsc_adjust(struct domain *d, hvm_domain_context_t
> *h)
> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
> index c3b68eb257..2845dc5b48 100644
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -280,7 +280,7 @@ unsigned int hvm_pci_decode_addr(unsigned int cf8,
> unsigned int addr,
>  static bool vpci_access_allowed(unsigned int reg, unsigned int len)
>  {
>      /* Check access size. */
> -    if ( len != 1 && len != 2 && len != 4 )
> +    if ( len != 1 && len != 2 && len != 4 && len != 8 )
>          return false;
> 
>      /* Check that access is size aligned. */
> @@ -391,6 +391,187 @@ void register_vpci_portio_handler(struct domain
> *d)
>      handler->ops = &vpci_portio_ops;
>  }
> 
> +struct hvm_mmcfg {
> +    struct list_head next;
> +    paddr_t addr;
> +    unsigned int size;
> +    uint16_t segment;
> +    int8_t bus;
> +};
> +
> +/* Handlers to trap PCI MMCFG config accesses. */
> +static const struct hvm_mmcfg *vpci_mmcfg_find(struct domain *d,
> paddr_t addr)
> +{
> +    const struct hvm_mmcfg *mmcfg;
> +
> +    list_for_each_entry ( mmcfg, &d->arch.hvm_domain.mmcfg_regions,
> next )
> +        if ( addr >= mmcfg->addr && addr < mmcfg->addr + mmcfg->size )
> +            return mmcfg;
> +
> +    return NULL;
> +}
> +
> +static unsigned int vpci_mmcfg_decode_addr(const struct hvm_mmcfg
> *mmcfg,
> +                                           paddr_t addr, unsigned int *bus,
> +                                           unsigned int *slot,
> +                                           unsigned int *func)
> +{
> +    unsigned int bdf;
> +
> +    addr -= mmcfg->addr;
> +    bdf = MMCFG_BDF(addr);
> +    *bus = PCI_BUS(bdf) + mmcfg->bus;
> +    *slot = PCI_SLOT(bdf);
> +    *func = PCI_FUNC(bdf);
> +
> +    return addr & (PCI_CFG_SPACE_EXP_SIZE - 1);
> +}
> +
> +static int vpci_mmcfg_accept(struct vcpu *v, unsigned long addr)
> +{
> +    struct domain *d = v->domain;
> +    bool found;
> +
> +    vpci_rlock(d);
> +    found = vpci_mmcfg_find(d, addr);
> +    vpci_runlock(d);
> +
> +    return found;
> +}
> +
> +static int vpci_mmcfg_read(struct vcpu *v, unsigned long addr,
> +                           unsigned int len, unsigned long *data)
> +{
> +    struct domain *d = v->domain;
> +    const struct hvm_mmcfg *mmcfg;
> +    unsigned int bus, slot, func, reg;
> +
> +    *data = ~0ul;
> +
> +    vpci_rlock(d);
> +    mmcfg = vpci_mmcfg_find(d, addr);
> +    if ( !mmcfg )
> +    {
> +        vpci_runlock(d);
> +        return X86EMUL_OKAY;
> +    }
> +
> +    reg = vpci_mmcfg_decode_addr(mmcfg, addr, &bus, &slot, &func);
> +
> +    if ( !vpci_access_allowed(reg, len) ||
> +         (reg + len) > PCI_CFG_SPACE_EXP_SIZE )
> +    {
> +        vpci_runlock(d);
> +        return X86EMUL_OKAY;
> +    }
> +
> +    /*
> +     * According to the PCIe 3.1A specification:
> +     *  - Configuration Reads and Writes must usually be DWORD or smaller
> +     *    in size.
> +     *  - Because Root Complex implementations are not required to support
> +     *    accesses to a RCRB that cross DW boundaries [...] software
> +     *    should take care not to cause the generation of such accesses
> +     *    when accessing a RCRB unless the Root Complex will support the
> +     *    access.
> +     *  Xen however supports 8byte accesses by splitting them into two
> +     *  4byte accesses.
> +     */
> +    *data = vpci_read(mmcfg->segment, bus, slot, func, reg, min(4u, len));
> +    if ( len == 8 )
> +        *data |= (uint64_t)vpci_read(mmcfg->segment, bus, slot, func,
> +                                     reg + 4, 4) << 32;
> +    vpci_runlock(d);
> +
> +    return X86EMUL_OKAY;
> +}
> +
> +static int vpci_mmcfg_write(struct vcpu *v, unsigned long addr,
> +                            unsigned int len, unsigned long data)
> +{
> +    struct domain *d = v->domain;
> +    const struct hvm_mmcfg *mmcfg;
> +    unsigned int bus, slot, func, reg;
> +
> +    vpci_wlock(d);
> +    mmcfg = vpci_mmcfg_find(d, addr);
> +    if ( !mmcfg )
> +    {
> +        vpci_wunlock(d);
> +        return X86EMUL_OKAY;
> +    }
> +
> +    reg = vpci_mmcfg_decode_addr(mmcfg, addr, &bus, &slot, &func);
> +
> +    if ( !vpci_access_allowed(reg, len) ||
> +         (reg + len) > PCI_CFG_SPACE_EXP_SIZE )
> +    {
> +        vpci_wunlock(d);
> +        return X86EMUL_OKAY;
> +    }
> +
> +    vpci_write(mmcfg->segment, bus, slot, func, reg, min(4u, len), data);
> +    if ( len == 8 )
> +        vpci_write(mmcfg->segment, bus, slot, func, reg + 4, 4, data >> 32);
> +    vpci_wunlock(d);
> +
> +    return X86EMUL_OKAY;
> +}
> +
> +static const struct hvm_mmio_ops vpci_mmcfg_ops = {
> +    .check = vpci_mmcfg_accept,
> +    .read = vpci_mmcfg_read,
> +    .write = vpci_mmcfg_write,
> +};
> +
> +int __hwdom_init register_vpci_mmcfg_handler(struct domain *d, paddr_t
> addr,
> +                                             unsigned int start_bus,
> +                                             unsigned int end_bus,
> +                                             unsigned int seg)
> +{
> +    struct hvm_mmcfg *mmcfg;
> +
> +    ASSERT(is_hardware_domain(d));
> +
> +    vpci_wlock(d);
> +    if ( vpci_mmcfg_find(d, addr) )
> +    {
> +        vpci_wunlock(d);
> +        return -EEXIST;
> +    }
> +
> +    mmcfg = xmalloc(struct hvm_mmcfg);
> +    if ( !mmcfg )
> +    {
> +        vpci_wunlock(d);
> +        return -ENOMEM;
> +    }
> +
> +    if ( list_empty(&d->arch.hvm_domain.mmcfg_regions) )
> +        register_mmio_handler(d, &vpci_mmcfg_ops);
> +
> +    mmcfg->addr = addr + (start_bus << 20);
> +    mmcfg->bus = start_bus;
> +    mmcfg->segment = seg;
> +    mmcfg->size = (end_bus - start_bus + 1) << 20;
> +    list_add(&mmcfg->next, &d->arch.hvm_domain.mmcfg_regions);
> +    vpci_wunlock(d);
> +
> +    return 0;
> +}
> +
> +void destroy_vpci_mmcfg(struct list_head *domain_mmcfg)
> +{
> +    while ( !list_empty(domain_mmcfg) )
> +    {
> +        struct hvm_mmcfg *mmcfg = list_first_entry(domain_mmcfg,
> +                                                   struct hvm_mmcfg, next);
> +
> +        list_del(&mmcfg->next);
> +        xfree(mmcfg);
> +    }
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-
> x86/hvm/domain.h
> index 3a54d50606..e8dc01bc3e 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -187,6 +187,9 @@ struct hvm_domain {
>      /* Lock for the PCI emulation layer (vPCI). */
>      rwlock_t vpci_lock;
> 
> +    /* List of MMCFG regions trapped by Xen. */
> +    struct list_head mmcfg_regions;
> +
>      /* List of permanently write-mapped pages. */
>      struct {
>          spinlock_t lock;
> diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
> index 01322a2e21..837046026c 100644
> --- a/xen/include/asm-x86/hvm/io.h
> +++ b/xen/include/asm-x86/hvm/io.h
> @@ -163,6 +163,13 @@ void register_g2m_portio_handler(struct domain
> *d);
>  /* HVM port IO handler for PCI accesses. */
>  void register_vpci_portio_handler(struct domain *d);
> 
> +/* HVM MMIO handler for PCI MMCFG accesses. */
> +int register_vpci_mmcfg_handler(struct domain *d, paddr_t addr,
> +                                unsigned int start_bus, unsigned int end_bus,
> +                                unsigned int seg);
> +/* Destroy tracked MMCFG areas. */
> +void destroy_vpci_mmcfg(struct list_head *domain_mmcfg);
> +
>  #endif /* __ASM_X86_HVM_IO_H__ */
> 
> 
> diff --git a/xen/include/asm-x86/pci.h b/xen/include/asm-x86/pci.h
> index 36801d317b..ac16c8fd5d 100644
> --- a/xen/include/asm-x86/pci.h
> +++ b/xen/include/asm-x86/pci.h
> @@ -6,6 +6,8 @@
>  #define CF8_ADDR_HI(cf8) (  ((cf8) & 0x0f000000) >> 16)
>  #define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000))
> 
> +#define MMCFG_BDF(addr)  ( ((addr) & 0x0ffff000) >> 12)
> +
>  #define IS_SNB_GFX(id) (id == 0x01068086 || id == 0x01168086 \
>                          || id == 0x01268086 || id == 0x01028086 \
>                          || id == 0x01128086 || id == 0x01228086 \
> --
> 2.11.0 (Apple Git-81)

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