Re: [PATCH V2 23/25] x86/vvtd: Handle interrupt translation faults

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

On Wed, Aug 23, 2017 at 12:51:27PM +0100, Roger Pau Monné wrote:
>On Wed, Aug 09, 2017 at 04:34:24PM -0400, Lan Tianyu wrote:
>> From: Chao Gao <chao.gao@intel.com>
>> 
>> Interrupt translation faults are non-recoverable fault. When faults
>                                                   ^ 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
>  ^ chapter
>> 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>
>> ---
>
>>      /* Address range of remapping hardware register-set */
>>      uint64_t base_addr;
>>      uint64_t length;
>> @@ -97,6 +101,23 @@ static inline struct vvtd *vcpu_vvtd(struct vcpu *v)
>>      return domain_vvtd(v->domain);
>>  }
>>  
>> +static inline int vvtd_test_and_set_bit(struct vvtd *vvtd, uint32_t reg,
>> +                                        int nr)
>
>unsigned int for nr, and I'm not really sure the usefulness of this
>helpers. In any case inline should not be used and instead let the
>compiler optimize this.
>

I think compiler doesn't know the frequency of calling these function.
Explicitly make this function inline sometimes can avoid compiler
doesn't do this for some short and frequently used functions.

>> +static void vvtd_report_non_recoverable_fault(struct vvtd *vvtd, int reason)
>> +{
>> +    uint32_t fsts;
>> +
>> +    ASSERT(reason & DMA_FSTS_FAULTS);
>> +    fsts = vvtd_get_reg(vvtd, DMAR_FSTS_REG);
>> +    __vvtd_set_bit(vvtd, DMAR_FSTS_REG, reason);
>
>I don't understand this, is reason a bit position or a mask?
>
>DMA_FSTS_FAULTS seems to be a mask, that should be set into DMAR_FSTS_REG?

According VT-d spec 10.4.9, Each kind of fault is denoted by one bit in
DMAR_FSTS_REG.

>>  static int vvtd_record_fault(struct vvtd *vvtd,
>> -                             struct irq_remapping_request *irq,
>> +                             struct irq_remapping_request *request,
>>                               int reason)
>>  {
>> -    return 0;
>> +    struct vtd_fault_record_register frcd;
>> +    int frcd_idx;
>> +
>> +    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_BIT) )
>> +            return X86EMUL_OKAY;
>> +
>> +        /* No available Fault Record means Fault overflowed */
>> +        frcd_idx = vvtd_alloc_frcd(vvtd);
>> +        if ( frcd_idx == -1 )
>> +        {
>> +            vvtd_report_non_recoverable_fault(vvtd, DMA_FSTS_PFO_BIT);
>> +            return X86EMUL_OKAY;
>> +        }
>> +        memset(&frcd, 0, sizeof(frcd));
>> +        frcd.fields.FR = (u8)reason;
>> +        frcd.fields.FI = ((u64)irq_remapping_request_index(request)) << 36;
>> +        frcd.fields.SID = (u16)request->source_id;
>> +        frcd.fields.F = 1;
>> +        vvtd_commit_frcd(vvtd, frcd_idx, &frcd);
>> +        return X86EMUL_OKAY;
>> +
>> +    default:
>
>Other reasons are just ignored? Should this have an ASSERT_UNREACHABLE
>maybe?

It can have for all the faults are raised by vvtd. When vvtd generates a
new kinds of fault, the corresponding handler also should be added.

>
>> +        break;
>> +    }
>> +
>> +    gdprintk(XENLOG_ERR, "Can't handle vVTD Fault (reason 0x%x).", reason);
>> +    domain_crash(vvtd->domain);
>
>Oh, I see. Is it expected that such faults with unhandled reasons can
>be somehow generated by the domain itself?
>

No. Faults are generated by vvtd. We only add interrupt translation
faults. Other faults can be added when adding other features (e.g. DMA
remapping). 


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