Re: [PATCH v4 11/28] x86/vvtd: Process interrupt remapping request

From: Chao Gao <chao.gao@intel.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Lan Tianyu <tianyu.lan@intel.com>,
	Kevin Tian <kevin.tian@intel.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	xen-devel@lists.xen.org, Jan Beulich <jbeulich@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH v4 11/28] x86/vvtd: Process interrupt remapping request
Date: Sun, 11 Feb 2018 13:31:41 +0800	[thread overview]
Message-ID: <20180211053141.GB124683@skl-4s-chao.sh.intel.com> (raw)
In-Reply-To: <20180209174417.gnjgyunfqthlmknw@MacBook-Pro-de-Roger.local>

On Fri, Feb 09, 2018 at 05:44:17PM +0000, Roger Pau Monné wrote:
>On Fri, Nov 17, 2017 at 02:22:18PM +0800, Chao Gao wrote:
>> 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. This patch
>> also introduces a counter inflight_intr, which is used to count the number
>> of interrupt are being handled. The reason why we should have this
>> counter is VT-d hardware should drain in-flight interrups before setting
>> flags to show that some operations are completed. These operations
>> include enabling interrupt remapping and performing a kind of invalidation
>> requests. In vvtd, we also try to drain in-flight interrupts by waiting
>> the inflight_intr is decreased to 0.
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> 
>> ---
>> v4:
>>  - use "#define" to define interrupt remapping transition faults
>>  rather than using an enum
>>  - use switch-case rather than if-else in irq_remapping_request_index()
>>  and vvtd_irq_request_sanity_check()
>>  - introduce a counter inflight_intr
>> 
>> v3:
>>  - Encode map_guest_page()'s error into void* to avoid using another parameter
>> ---
>>  xen/drivers/passthrough/vtd/iommu.h |  15 +++
>>  xen/drivers/passthrough/vtd/vvtd.c  | 219 ++++++++++++++++++++++++++++++++++++
>>  2 files changed, 234 insertions(+)
>> 
>> diff --git a/xen/drivers/passthrough/vtd/iommu.h b/xen/drivers/passthrough/vtd/iommu.h
>> index 9c59aeb..82edd2a 100644
>> --- a/xen/drivers/passthrough/vtd/iommu.h
>> +++ b/xen/drivers/passthrough/vtd/iommu.h
>> @@ -216,6 +216,15 @@
>>  #define dma_frcd_source_id(c) (c & 0xffff)
>>  #define dma_frcd_page_addr(d) (d & (((u64)-1) << 12)) /* low 64 bit */
>>  
>> +/* Interrupt remapping transition faults */
>> +#define VTD_FR_IR_REQ_RSVD      0x20
>> +#define VTD_FR_IR_INDEX_OVER    0x21
>> +#define VTD_FR_IR_ENTRY_P       0x22
>> +#define VTD_FR_IR_ROOT_INVAL    0x23
>> +#define VTD_FR_IR_IRTE_RSVD     0x24
>> +#define VTD_FR_IR_REQ_COMPAT    0x25
>> +#define VTD_FR_IR_SID_ERR       0x26
>> +
>>  /*
>>   * 0: Present
>>   * 1-11: Reserved
>> @@ -356,6 +365,12 @@ struct iremap_entry {
>>  };
>>  
>>  /*
>> + * When VT-d doesn't enable extended interrupt mode, hardware interprets
>> + * 8-bits ([15:8]) of Destination-ID field in the IRTEs.
>> + */
>> +#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/vvtd.c b/xen/drivers/passthrough/vtd/vvtd.c
>> index 06e522a..927e715 100644
>> --- a/xen/drivers/passthrough/vtd/vvtd.c
>> +++ b/xen/drivers/passthrough/vtd/vvtd.c
>> @@ -22,11 +22,15 @@
>>  #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/io_apic.h>
>>  #include <asm/hvm/domain.h>
>>  #include <asm/p2m.h>
>>  
>>  #include "iommu.h"
>> +#include "vtd.h"
>>  
>>  /* Supported capabilities by vvtd */
>>  #define VVTD_MAX_CAPS VIOMMU_CAP_IRQ_REMAPPING
>> @@ -52,6 +56,8 @@ struct vvtd {
>>      uint64_t base_addr;
>>      /* Point back to the owner domain */
>>      struct domain *domain;
>> +    /* # of in-flight interrupts */
>> +    atomic_t inflight_intr;
>>  
>>      struct hvm_hw_vvtd hw;
>>      void *irt_base;
>> @@ -181,6 +187,109 @@ static void unmap_guest_pages(void *va, uint32_t nr)
>>          put_page_and_type(mfn_to_page(mfn[i]));
>>  }
>>  
>> +static int vvtd_delivery(struct domain *d, uint8_t vector,
>> +                         uint32_t dest, bool dest_mode,
>> +                         uint8_t delivery_mode, uint8_t trig_mode)
>> +{
>> +    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_debug("d%d: dest=v%d dlm=%x vector=%d trig_mode=%d\n",
>> +                       vlapic_domain(target)->domain_id,
>> +                       vlapic_vcpu(target)->vcpu_id,
>> +                       delivery_mode, vector, trig_mode);
>> +            vlapic_set_irq(target, vector, trig_mode);
>> +            break;
>> +        }
>> +        vvtd_debug("d%d: null round robin: vector=%02x\n",
>> +                   d->domain_id, vector);
>> +        break;
>> +
>> +    case dest_Fixed:
>> +        for_each_vcpu ( d, v )
>> +            if ( vlapic_match_dest(vcpu_vlapic(v), NULL, 0, dest, dest_mode) )
>> +            {
>> +                vvtd_debug("d%d: dest=v%d dlm=%x vector=%d trig_mode=%d\n",
>> +                           v->domain->domain_id, v->vcpu_id,
>> +                           delivery_mode, vector, trig_mode);
>> +                vlapic_set_irq(vcpu_vlapic(v), vector, trig_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);
>
>Doing this loops here seems quite bad from a preformance PoV,
>specially taking into account that this code is going to be used with
>> 128 vCPUs.

Maybe. But i prefer to not do optimization at this early stage.

>
>> +        break;
>> +
>> +    default:
>> +        gdprintk(XENLOG_WARNING, "Unsupported VTD delivery mode %d\n",
>> +                 delivery_mode);
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +/* Computing the IRTE index for a given interrupt request. When success, return
>> + * 0 and set index to reference the corresponding IRTE. Otherwise, return < 0,
>> + * i.e. -1 when the irq request isn't an remapping format.
>> + */
>> +static int irq_remapping_request_index(
>> +    const struct arch_irq_remapping_request *irq, uint32_t *index)
>> +{
>> +    switch ( irq->type )
>> +    {
>> +    case VIOMMU_REQUEST_IRQ_MSI:
>> +    {
>> +        struct msi_msg_remap_entry msi_msg =
>> +        {
>> +            .address_lo = { .val = irq->msg.msi.addr },
>
>Can't you just use .address_lo.val = irq->...

Will do.

>
>> +            .data = irq->msg.msi.data,
>> +        };
>> +
>> +        if ( !msi_msg.address_lo.format )
>> +            return -1;
>
>In all the other functions you already return some kind of meaningful
>error code, please do so here also.

Ok.

>
>> +
>> +        *index = (msi_msg.address_lo.index_15 << 15) +
>> +                msi_msg.address_lo.index_0_14;
>> +        if ( msi_msg.address_lo.SHV )
>> +            *index += (uint16_t)msi_msg.data;
>> +        break;
>> +    }
>> +
>> +    case VIOMMU_REQUEST_IRQ_APIC:
>> +    {
>> +        struct IO_APIC_route_remap_entry remap_rte = { .val = irq->msg.rte };
>> +
>> +        if ( !remap_rte.format )
>> +            return -1;
>> +
>> +        *index = (remap_rte.index_15 << 15) + remap_rte.index_0_14;
>> +        break;
>> +    }
>> +
>> +    default:
>> +        ASSERT_UNREACHABLE();
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static inline uint32_t irte_dest(struct vvtd *vvtd, uint32_t dest)
>> +{
>> +    /* In xAPIC mode, only 8-bits([15:8]) are valid */
>> +    return vvtd->hw.eim_enabled ? dest
>> +                                : MASK_EXTR(dest, IRTE_xAPIC_DEST_MASK);
>> +}
>> +
>>  static void write_gcmd_ire(struct vvtd *vvtd, uint32_t val)
>>  {
>>      bool set = val & DMA_GCMD_IRE;
>> @@ -323,6 +432,115 @@ static const struct hvm_mmio_ops vvtd_mmio_ops = {
>>      .write = vvtd_write
>>  };
>>  
>> +static void vvtd_handle_fault(struct vvtd *vvtd,
>> +                              const struct arch_irq_remapping_request *irq,
>> +                              struct iremap_entry *irte,
>> +                              unsigned int fault)
>> +{
>> +    switch ( fault )
>> +    {
>> +    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_REQ_RSVD:
>> +    case VTD_FR_IR_INDEX_OVER:
>> +    case VTD_FR_IR_ROOT_INVAL:
>> +        /* TODO: handle fault (e.g. record and report this fault to VM */
>> +        break;
>> +
>> +    default:
>> +        vvtd_debug("d%d can't handle VT-d fault %x\n", vvtd->domain->domain_id,
>> +                   fault);
>> +    }
>> +    return;
>> +}
>> +
>> +static bool vvtd_irq_request_sanity_check(const struct vvtd *vvtd,
>> +                                   const struct arch_irq_remapping_request *irq)
>> +{
>> +    switch ( irq->type )
>> +    {
>> +    case VIOMMU_REQUEST_IRQ_APIC:
>> +    {
>> +        struct IO_APIC_route_remap_entry rte = { .val = irq->msg.rte };
>> +
>> +        return !rte.reserved;
>> +    }
>> +
>> +    case VIOMMU_REQUEST_IRQ_MSI:
>> +        return true;
>> +    }
>> +
>> +    ASSERT_UNREACHABLE();
>> +    return false;
>> +}
>> +
>> +static int vvtd_get_entry(struct vvtd *vvtd,
>> +                          const struct arch_irq_remapping_request *irq,
>> +                          struct iremap_entry *dest)
>const for both vvtd and dest?

constify vvtd is ok. 'dest' is used to store the entry corresponding to the request.
So 'dest' cannot be const.

>
>> +{
>> +    uint32_t entry;
>> +    struct iremap_entry irte;
>> +    int ret = irq_remapping_request_index(irq, &entry);
>> +
>> +    ASSERT(!ret);
>> +
>> +    vvtd_debug("d%d: interpret a request with index %x\n",
>> +               vvtd->domain->domain_id, entry);
>> +
>> +    if ( !vvtd_irq_request_sanity_check(vvtd, irq) )
>> +        return VTD_FR_IR_REQ_RSVD;
>> +    else if ( entry > vvtd->hw.irt_max_entry )
>> +        return VTD_FR_IR_INDEX_OVER;
>> +    else if ( !vvtd->irt_base )
>
>No need for the 'else', since you are already using return.
>
>> +        return VTD_FR_IR_ROOT_INVAL;
>> +
>> +    irte = ((struct iremap_entry*)vvtd->irt_base)[entry];
>> +
>> +    if ( !qinval_present(irte) )
>> +        ret = VTD_FR_IR_ENTRY_P;
>> +    else if ( (irte.remap.res_1 || irte.remap.res_2 || irte.remap.res_3 ||
>> +               irte.remap.res_4) )
>> +        ret = VTD_FR_IR_IRTE_RSVD;
>> +
>> +    /* FIXME: We don't check against the source ID */
>> +
>> +    dest->val = irte.val;
>> +
>> +    return ret;
>> +}
>> +
>> +static int vvtd_handle_irq_request(const struct domain *d,
>
>constifying domain here is not the best practice IMHO. In the function
>you are actually modifying vvtd, which is fine because it's a pointer
>but it's conceptually inside of domain.

Ok.

>
>> +                                   const struct arch_irq_remapping_request *irq)
>> +{
>> +    struct iremap_entry irte;
>> +    int ret;
>> +    struct vvtd *vvtd = domain_vvtd(d);
>> +
>> +    if ( !vvtd || !vvtd->hw.intremap_enabled )
>> +        return -ENODEV;
>> +
>> +    atomic_inc(&vvtd->inflight_intr);
>> +    ret = vvtd_get_entry(vvtd, irq, &irte);
>> +    if ( ret )
>> +    {
>> +        vvtd_handle_fault(vvtd, irq, &irte, ret);
>> +        goto out;
>> +    }
>> +
>> +    ret = vvtd_delivery(vvtd->domain, irte.remap.vector,
>> +                        irte_dest(vvtd, irte.remap.dst),
>> +                        irte.remap.dm, irte.remap.dlm,
>> +                        irte.remap.tm);
>> +
>> + out:
>> +    atomic_dec(&vvtd->inflight_intr);
>
>So inflight_intr seem to be quite pointless, you only use it in this
>function and it's never read AFAICT.

I will introduce this field when it is first used.

Thanks
Chao

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel