Re: [PATCH V3 26/29] x86/vvtd: Handle interrupt translation faults

[Chao Gao <chao.gao@intel.com>]

On Thu, Oct 19, 2017 at 05:31:37PM +0100, Roger Pau Monné wrote:
>On Thu, Sep 21, 2017 at 11:02:07PM -0400, Lan Tianyu wrote:
>> From: Chao Gao <chao.gao@intel.com>
>> 
>> Interrupt translation faults are non-recoverable fault. When faults
>> are triggered, it needs to populate fault info to Fault Recording
>> Registers and inject vIOMMU msi interrupt to notify guest IOMMU driver
>> to deal with faults.
>> 
>> This patch emulates hardware's handling interrupt translation
>> faults (more information about the process can be found in VT-d spec,
>> chipter "Translation Faults", section "Non-Recoverable Fault
>> Reporting" and section "Non-Recoverable Logging").
>> Specifically, viommu_record_fault() records the fault information and
>> viommu_report_non_recoverable_fault() reports faults to software.
>> Currently, only Primary Fault Logging is supported and the Number of
>> Fault-recording Registers is 1.
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>>  xen/drivers/passthrough/vtd/iommu.h |  60 +++++++--
>>  xen/drivers/passthrough/vtd/vvtd.c  | 252 +++++++++++++++++++++++++++++++++++-
>>  2 files changed, 301 insertions(+), 11 deletions(-)
>> 
>> diff --git a/xen/drivers/passthrough/vtd/iommu.h b/xen/drivers/passthrough/vtd/iommu.h
>> index 790384f..e19b045 100644
>> --- a/xen/drivers/passthrough/vtd/iommu.h
>> +++ b/xen/drivers/passthrough/vtd/iommu.h
>> @@ -198,26 +198,66 @@
>>  #define DMA_CCMD_CAIG_MASK(x) (((u64)x) & ((u64) 0x3 << 59))
>>  
>>  /* FECTL_REG */
>> -#define DMA_FECTL_IM (((u64)1) << 31)
>> +#define DMA_FECTL_IM_SHIFT 31
>> +#define DMA_FECTL_IM (1U << DMA_FECTL_IM_SHIFT)
>> +#define DMA_FECTL_IP_SHIFT 30
>> +#define DMA_FECTL_IP (1U << DMA_FECTL_IP_SHIFT)
>
>Is it fine to change those from uint64_t to unsigned int?

Yes. The FECTL and FSTS are 32-bit registers.

>
>>  
>>  /* FSTS_REG */
>> -#define DMA_FSTS_PFO ((u64)1 << 0)
>> -#define DMA_FSTS_PPF ((u64)1 << 1)
>> -#define DMA_FSTS_AFO ((u64)1 << 2)
>> -#define DMA_FSTS_APF ((u64)1 << 3)
>> -#define DMA_FSTS_IQE ((u64)1 << 4)
>> -#define DMA_FSTS_ICE ((u64)1 << 5)
>> -#define DMA_FSTS_ITE ((u64)1 << 6)
>> -#define DMA_FSTS_FAULTS    DMA_FSTS_PFO | DMA_FSTS_PPF | DMA_FSTS_AFO | DMA_FSTS_APF | DMA_FSTS_IQE | DMA_FSTS_ICE | DMA_FSTS_ITE
>> +#define DMA_FSTS_PFO_SHIFT 0
>> +#define DMA_FSTS_PFO (1U << DMA_FSTS_PFO_SHIFT)
>> +#define DMA_FSTS_PPF_SHIFT 1
>> +#define DMA_FSTS_PPF (1U << DMA_FSTS_PPF_SHIFT)
>> +#define DMA_FSTS_AFO (1U << 2)
>> +#define DMA_FSTS_APF (1U << 3)
>> +#define DMA_FSTS_IQE (1U << 4)
>> +#define DMA_FSTS_ICE (1U << 5)
>> +#define DMA_FSTS_ITE (1U << 6)
>
>This seemingly non-functional changes should be done in a separate
>patch.

sure.

>> +static int vvtd_alloc_frcd(struct vvtd *vvtd)
>> +{
>> +    int prev;
>> +    uint64_t cap = vvtd_get_reg(vvtd, DMAR_CAP_REG);
>> +    unsigned int base = cap_fault_reg_offset(cap);
>> +
>> +    /* Set the F bit to indicate the FRCD is in use. */
>> +    if ( !vvtd_test_and_set_bit(vvtd,
>> +                                base + vvtd->status.fault_index * DMA_FRCD_LEN +
>> +                                DMA_FRCD3_OFFSET, DMA_FRCD_F_SHIFT) )
>> +    {
>> +        prev = vvtd->status.fault_index;
>> +        vvtd->status.fault_index = (prev + 1) % cap_num_fault_regs(cap);
>> +        return vvtd->status.fault_index;
>
>I would prefer that you return the index as an unsigned int parameter
>passed by reference rather than as the return value of the function,
>but that might not be the preference of others.

What are the pros and cons?

>> +static int vvtd_record_fault(struct vvtd *vvtd,
>> +                             struct arch_irq_remapping_request *request,
>> +                             int reason)
>> +{
>> +    struct vtd_fault_record_register frcd;
>> +    int fault_index;
>> +
>> +    switch(reason)
>> +    {
>> +    case VTD_FR_IR_REQ_RSVD:
>> +    case VTD_FR_IR_INDEX_OVER:
>> +    case VTD_FR_IR_ENTRY_P:
>> +    case VTD_FR_IR_ROOT_INVAL:
>> +    case VTD_FR_IR_IRTE_RSVD:
>> +    case VTD_FR_IR_REQ_COMPAT:
>> +    case VTD_FR_IR_SID_ERR:
>> +        if ( vvtd_test_bit(vvtd, DMAR_FSTS_REG, DMA_FSTS_PFO_SHIFT) )
>> +            return X86EMUL_OKAY;
>> +
>> +        /* No available Fault Record means Fault overflowed */
>> +        fault_index = vvtd_alloc_frcd(vvtd);
>> +        if ( fault_index == -1 )
>
>Erm, wouldn't vvtd_alloc_frcd return -ENOMEM in case of error? Ie: you
>should check if ( fault_index < 0 ).

It is a mistake.

Thanks
Chao


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