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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Aug 25, 2017 at 03:17:24PM +0800, Chao Gao wrote:
> 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@xxxxxxxxx>
> >> 
> >> 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@xxxxxxxxx>
> >> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> >> ---
> >
> >>      /* 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.

I will leave that to the maintainers. IMHO I wouldn't add inline
unless I see some figures that back up the supposed speed increase.

> >> +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.

So 'reason' is a bit position in this context? It needs to be unsigned
int then.

> >>  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.

Do you mean that with the code above all the possible fault types are
covered?

In which case it seems you want to add a ASSERT_UNREACHABLE to the
default case.

> >
> >> +        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). 

OK, so then it looks like you want an ASSERT above, and I'm not sure
whether you want to kill the domain. At the end if Xen ever gets here
it means there's a bug in the vvtd implementation. I think the proper
solution is to add the ASSERT above and simply return X86EMUL_OKAY
here.

Roger.

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