[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH V2 14/25] x86/vvtd: Process interrupt remapping request



On Wed, Aug 09, 2017 at 04:34:15PM -0400, Lan Tianyu wrote:
> From: Chao Gao <chao.gao@xxxxxxxxx>
> 
> When a remapping interrupt request arrives, remapping hardware computes the
> interrupt_index per the algorithm described in VTD spec
> "Interrupt Remapping Table", interprets the IRTE and generates a remapped
> interrupt request.
> 
> This patch introduces viommu_handle_irq_request() to emulate the process how
> remapping hardware handles a remapping interrupt request.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> ---
>  xen/drivers/passthrough/vtd/iommu.h |  21 +++
>  xen/drivers/passthrough/vtd/vtd.h   |   6 +
>  xen/drivers/passthrough/vtd/vvtd.c  | 276 
> +++++++++++++++++++++++++++++++++++-
>  3 files changed, 302 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.h 
> b/xen/drivers/passthrough/vtd/iommu.h
> index 102b4f3..70e64cf 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -244,6 +244,21 @@
>  #define dma_frcd_source_id(c) (c & 0xffff)
>  #define dma_frcd_page_addr(d) (d & (((u64)-1) << 12)) /* low 64 bit */
>  
> +enum VTD_FAULT_TYPE
> +{
> +    /* Interrupt remapping transition faults */
> +    VTD_FR_IR_REQ_RSVD = 0x20,   /* One or more IR request reserved
> +                                  * fields set */
> +    VTD_FR_IR_INDEX_OVER = 0x21, /* Index value greater than max */
> +    VTD_FR_IR_ENTRY_P = 0x22,    /* Present (P) not set in IRTE */
> +    VTD_FR_IR_ROOT_INVAL = 0x23, /* IR Root table invalid */
> +    VTD_FR_IR_IRTE_RSVD = 0x24,  /* IRTE Rsvd field non-zero with
> +                                  * Present flag set */
> +    VTD_FR_IR_REQ_COMPAT = 0x25, /* Encountered compatible IR
> +                                  * request while disabled */
> +    VTD_FR_IR_SID_ERR = 0x26,    /* Invalid Source-ID */
> +};

Could you please align the values, like:

enum VTD_FAULT_TYPE
{
    /* Interrupt remapping transition faults */
    VTD_FR_IR_REQ_RSVD    = 0x20, /* One or more IR request reserved
                                   * fields set */
    VTD_FR_IR_INDEX_OVER  = 0x21, /* Index value greater than max */


> +
>  /*
>   * 0: Present
>   * 1-11: Reserved
> @@ -384,6 +399,12 @@ struct iremap_entry {
>  };
>  
>  /*
> + * When VT-d doesn't enable Extended Interrupt Mode. Hardware only interprets
> + * only 8-bits ([15:8]) of Destination-ID field in the IRTEs.

The above comment needs to be rewritten. My knowledge of VT-d is
limited, what is IRTE referring to? I thought I was referring to the
IO APIC redirection table registers, but the mask then doesn't make
sense.

> + */
> +#define IRTE_xAPIC_DEST_MASK 0xff00
> +
> +/*
>   * Posted-interrupt descriptor address is 64 bits with 64-byte aligned, only
>   * the upper 26 bits of lest significiant 32 bits is available.
>   */
> diff --git a/xen/drivers/passthrough/vtd/vtd.h 
> b/xen/drivers/passthrough/vtd/vtd.h
> index bb8889f..1032b46 100644
> --- a/xen/drivers/passthrough/vtd/vtd.h
> +++ b/xen/drivers/passthrough/vtd/vtd.h
> @@ -47,6 +47,8 @@ struct IO_APIC_route_remap_entry {
>      };
>  };
>  
> +#define IOAPIC_REMAP_ENTRY_INDEX(x) ((x.index_15 << 15) + x.index_0_14)

Could you use entry instead of x? And you should add parentheses
around it's usage in the macro.

> +
>  struct msi_msg_remap_entry {
>      union {
>          u32 val;
> @@ -65,4 +67,8 @@ struct msi_msg_remap_entry {
>      u32      data;           /* msi message data */
>  };
>  
> +#define MSI_REMAP_ENTRY_INDEX(x) ((x.address_lo.index_15 << 15) + \
> +                                  x.address_lo.index_0_14 + \
> +                                  (x.address_lo.SHV ? (uint16_t)x.data : 0))

Same here. And it would be clearer to place both macros together IMHO.

>  #endif // _VTD_H_
> diff --git a/xen/drivers/passthrough/vtd/vvtd.c 
> b/xen/drivers/passthrough/vtd/vvtd.c
> index 8e8dbe6..2bee352 100644
> --- a/xen/drivers/passthrough/vtd/vvtd.c
> +++ b/xen/drivers/passthrough/vtd/vvtd.c
> @@ -23,11 +23,16 @@
>  #include <xen/types.h>
>  #include <xen/viommu.h>
>  #include <xen/xmalloc.h>
> +#include <asm/apic.h>
>  #include <asm/current.h>
> +#include <asm/event.h>
>  #include <asm/hvm/domain.h>
> +#include <asm/io_apic.h>
>  #include <asm/page.h>
> +#include <asm/p2m.h>
>  
>  #include "iommu.h"
> +#include "vtd.h"
>  
>  struct hvm_hw_vvtd_regs {
>      uint8_t data[1024];
> @@ -38,6 +43,9 @@ struct hvm_hw_vvtd_regs {
>  #define VIOMMU_STATUS_IRQ_REMAPPING_ENABLED     (1 << 0)
>  #define VIOMMU_STATUS_DMA_REMAPPING_ENABLED     (1 << 1)
>  
> +#define vvtd_irq_remapping_enabled(vvtd) \
> +    (vvtd->status & VIOMMU_STATUS_IRQ_REMAPPING_ENABLED)
> +
>  struct vvtd {
>      /* VIOMMU_STATUS_XXX */
>      int status;
> @@ -120,6 +128,140 @@ static inline uint8_t vvtd_get_reg_byte(struct vvtd 
> *vtd, uint32_t reg)
>      vvtd_set_reg(vvtd, (reg) + 4, (val) >> 32); \
>  } while(0)
>  
> +static int map_guest_page(struct domain *d, uint64_t gfn, void **virt)

You can make this function return a void * and encode the error in
there. See IS_ERR_VALUE and ERR_PTR.

> +{
> +    struct page_info *p;
> +
> +    p = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
> +    if ( !p )
> +        return -EINVAL;
> +
> +    if ( !get_page_type(p, PGT_writable_page) )
> +    {
> +        put_page(p);
> +        return -EINVAL;
> +    }
> +
> +    *virt = __map_domain_page_global(p);
> +    if ( !*virt )
> +    {
> +        put_page_and_type(p);
> +        return -ENOMEM;
> +    }

Newline

> +    return 0;
> +}
> +
> +static void unmap_guest_page(void *virt)
> +{
> +    struct page_info *page;
> +
> +    if ( !virt )
> +        return;
> +
> +    virt = (void *)((unsigned long)virt & PAGE_MASK);

This should maybe be an ASSERT? Are you going to call unmap_guest_page
with something different than the return of map_guest_page?

> +    page = mfn_to_page(domain_page_map_to_mfn(virt));
> +
> +    unmap_domain_page_global(virt);
> +    put_page_and_type(page);
> +}
> +
> +static void vvtd_inj_irq(
> +    struct vlapic *target,
> +    uint8_t vector,
> +    uint8_t trig_mode,
> +    uint8_t delivery_mode)

Indentation.

> +{
> +    VVTD_DEBUG(VVTD_DBG_INFO, "dest=v%d, delivery_mode=%x vector=%d "
> +               "trig_mode=%d.",
> +               vlapic_vcpu(target)->vcpu_id, delivery_mode,
> +               vector, trig_mode);
> +
> +    ASSERT((delivery_mode == dest_Fixed) ||
> +           (delivery_mode == dest_LowestPrio));
> +
> +    vlapic_set_irq(target, vector, trig_mode);
> +}
> +
> +static int vvtd_delivery(
> +    struct domain *d, int vector,

uint8_t vector?

> +    uint32_t dest, uint8_t dest_mode,
> +    uint8_t delivery_mode, uint8_t trig_mode)

Indentation.

> +{
> +    struct vlapic *target;
> +    struct vcpu *v;
> +
> +    switch ( delivery_mode )
> +    {
> +    case dest_LowestPrio:
> +        target = vlapic_lowest_prio(d, NULL, 0, dest, dest_mode);
> +        if ( target != NULL )
> +        {
> +            vvtd_inj_irq(target, vector, trig_mode, delivery_mode);
> +            break;
> +        }
> +        VVTD_DEBUG(VVTD_DBG_INFO, "null round robin: vector=%02x\n", vector);
> +        break;
> +
> +    case dest_Fixed:
> +        for_each_vcpu ( d, v )
> +            if ( vlapic_match_dest(vcpu_vlapic(v), NULL, 0, dest,
> +                                   dest_mode) )
> +                vvtd_inj_irq(vcpu_vlapic(v), vector,
> +                             trig_mode, delivery_mode);
> +        break;
> +
> +    case dest_NMI:
> +        for_each_vcpu ( d, v )
> +            if ( vlapic_match_dest(vcpu_vlapic(v), NULL, 0, dest, dest_mode)
> +                 && !test_and_set_bool(v->nmi_pending) )
> +                vcpu_kick(v);
> +        break;
> +
> +    default:
> +        printk(XENLOG_G_WARNING
> +               "%pv: Unsupported VTD delivery mode %d for Dom%d\n",
> +               current, delivery_mode, d->domain_id);

gprintk? Or at least this should be rate-limited somehow, it seems
like the guest can trigger such behavior?

> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +static uint32_t irq_remapping_request_index(struct irq_remapping_request 
> *irq)

irq should be const.

> +{
> +    if ( irq->type == VIOMMU_REQUEST_IRQ_MSI )
> +    {
> +        struct msi_msg_remap_entry msi_msg = { { irq->msg.msi.addr }, 0,
> +                                               irq->msg.msi.data };

I would rather prefer that you use designated initializers, ie:

struct msi_msg_remap_entry msi_msg =
{
    .address_lo = { .val = irq->msg.msi.addr },
    .data = irq->msg.msi.data,
};

> +
> +        return MSI_REMAP_ENTRY_INDEX(msi_msg);
> +    }
> +    else if ( irq->type == VIOMMU_REQUEST_IRQ_APIC )
> +    {
> +        struct IO_APIC_route_remap_entry remap_rte = { { irq->msg.rte } };
> +
> +        return IOAPIC_REMAP_ENTRY_INDEX(remap_rte);
> +    }
> +    BUG();

ASSERT_UNREACHABLE();

And newline.

> +    return 0;
> +}
> +
> +static inline uint32_t irte_dest(struct vvtd *vvtd, uint32_t dest)

vvtd should be const.

> +{
> +    uint64_t irta;
> +
> +    vvtd_get_reg_quad(vvtd, DMAR_IRTA_REG, irta);
> +    /* In xAPIC mode, only 8-bits([15:8]) are valid */
> +    return DMA_IRTA_EIME(irta) ? dest : MASK_EXTR(dest, 
> IRTE_xAPIC_DEST_MASK);
> +}
> +
> +static int vvtd_record_fault(struct vvtd *vvtd,
> +                             struct irq_remapping_request *irq,
> +                             int reason)
> +{
> +    return 0;
> +}

I guess this is going to be expanded later in the series? Seems
pointless to introduce it now.

> +
>  static int vvtd_handle_gcmd_sirtp(struct vvtd *vvtd, uint32_t val)
>  {
>      uint64_t irta;
> @@ -259,6 +401,137 @@ static const struct hvm_mmio_ops vvtd_mmio_ops = {
>      .write = vvtd_write
>  };
>  
> +static bool ir_sid_valid(struct iremap_entry *irte, uint32_t source_id)
> +{
> +    return true;
> +}

Same here, it's pointless to introduce such helper now if it's a dummy
one anyway.

> +
> +/*
> + * 'record_fault' is a flag to indicate whether we need recording a fault
> + * and notifying guest when a fault happens during fetching vIRTE.
                                                      ^ fetch of
> + */
> +static int vvtd_get_entry(struct vvtd *vvtd,
> +                          struct irq_remapping_request *irq,

Seems like vvtd and irq could be const.

> +                          struct iremap_entry *dest,
> +                          bool record_fault)
> +{
> +    int ret;
> +    uint32_t entry = irq_remapping_request_index(irq);
> +    struct iremap_entry  *irte, *irt_page;
> +
> +    VVTD_DEBUG(VVTD_DBG_TRANS, "interpret a request with index %x", entry);
> +
> +    if ( entry > vvtd->irt_max_entry )
> +    {
> +        ret = VTD_FR_IR_INDEX_OVER;
> +        goto handle_fault;
> +    }
> +
> +    ret = map_guest_page(vvtd->domain, vvtd->irt + (entry >> 
> IREMAP_ENTRY_ORDER),
> +                         (void**)&irt_page);
> +    if ( ret )
> +    {
> +        ret = VTD_FR_IR_ROOT_INVAL;
> +        goto handle_fault;
> +    }
> +
> +    irte = irt_page + (entry % (1 << IREMAP_ENTRY_ORDER));
> +    dest->val = irte->val;
> +    if ( !qinval_present(*irte) )
> +    {
> +        ret = VTD_FR_IR_ENTRY_P;
> +        goto unmap_handle_fault;
> +    }
> +
> +    /* Check reserved bits */
> +    if ( (irte->remap.res_1 || irte->remap.res_2 || irte->remap.res_3 ||
> +          irte->remap.res_4) )
> +    {
> +        ret = VTD_FR_IR_IRTE_RSVD;
> +        goto unmap_handle_fault;
> +    }
> +
> +    if (!ir_sid_valid(irte, irq->source_id))
> +    {
> +        ret = VTD_FR_IR_SID_ERR;
> +        goto unmap_handle_fault;
> +    }
> +    unmap_guest_page(irt_page);

Newline.

> +    return 0;
> +
> + unmap_handle_fault:
> +    unmap_guest_page(irt_page);
> + handle_fault:
> +    if ( !record_fault )
> +        return ret;
> +
> +    switch ( ret )
> +    {
> +    case VTD_FR_IR_SID_ERR:
> +    case VTD_FR_IR_IRTE_RSVD:
> +    case VTD_FR_IR_ENTRY_P:
> +        if ( qinval_fault_disable(*irte) )
> +            break;
> +    /* fall through */
> +    case VTD_FR_IR_INDEX_OVER:
> +    case VTD_FR_IR_ROOT_INVAL:
> +        vvtd_record_fault(vvtd, irq, ret);
> +        break;
> +
> +    default:
> +        gdprintk(XENLOG_G_INFO, "Can't handle VT-d fault %x\n", ret);
> +    }
> +    return ret;

In order to avoid the usage of labels I would put the code in
handle_fault in a helper function and call it on error.

> +}
> +
> +static int vvtd_irq_request_sanity_check(struct vvtd *vvtd,
> +                                         struct irq_remapping_request *irq)
> +{
> +    if ( irq->type == VIOMMU_REQUEST_IRQ_APIC )
> +    {
> +        struct IO_APIC_route_remap_entry rte = { { irq->msg.rte } };
> +
> +        ASSERT(rte.format);
> +        return (!rte.reserved) ? 0 : VTD_FR_IR_REQ_RSVD;
> +    }
> +    else if ( irq->type == VIOMMU_REQUEST_IRQ_MSI )
> +    {
> +        struct msi_msg_remap_entry msi_msg = { { irq->msg.msi.addr } };
> +
> +        ASSERT(msi_msg.address_lo.format);
> +        return 0;
> +    }
> +    BUG();

ASSERT_UNREACHABLE(); and newline.

> +    return 0;
> +}
> +
> +static int vvtd_handle_irq_request(struct domain *d,
> +                                   struct irq_remapping_request *irq)
> +{
> +    struct iremap_entry irte;
> +    int ret;
> +    struct vvtd *vvtd = domain_vvtd(d);
> +
> +    if ( !vvtd || !vvtd_irq_remapping_enabled(vvtd) )
> +        return -EINVAL;

ENODEV.

> +
> +    ret = vvtd_irq_request_sanity_check(vvtd, irq);
> +    if ( ret )
> +    {
> +        vvtd_record_fault(vvtd, irq, ret);
> +        return ret;
> +    }
> +
> +    if ( !vvtd_get_entry(vvtd, irq, &irte, true) )

You are losing the return value of vvtd_get_entry here.

> +    {
> +        vvtd_delivery(vvtd->domain, irte.remap.vector,
> +                      irte_dest(vvtd, irte.remap.dst), irte.remap.dm,
> +                      irte.remap.dlm, irte.remap.tm);

You are losing the return value of vvtd_delivery.

> +        return 0;
> +    }

Newline.

> +    return -EFAULT;
> +}
> +

Roger.

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.