Re: [RFC PATCH] intel-iommu: Report interrupt remapping faults

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 5367 bytes --]

On Fri, 2023-03-10 at 15:57 -0500, Peter Xu wrote:
> On Fri, Mar 10, 2023 at 05:49:38PM +0000, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > +}
> > +
> > +#define log_irte_fault(f, sid, i) vtd_report_ir_fault(iommu, sid, f, i)
> 
> This one seems not used.

Oops yes, that was supposed to be temporary until I did the
search/replace.

> > +
> >  /* Handle Invalidation Queue Errors of queued invalidation interface error
> >   * conditions.
> >   */
> > @@ -3300,7 +3320,8 @@ static Property vtd_properties[] = {
> >  
> >  /* Read IRTE entry with specific index */
> >  static int vtd_irte_get(IntelIOMMUState *iommu, uint16_t index,
> > -                        VTD_IR_TableEntry *entry, uint16_t sid)
> > +                        VTD_IR_TableEntry *entry, uint16_t sid,
> > +                        bool do_fault)
> >  {
> >      static const uint16_t vtd_svt_mask[VTD_SQ_MAX] = \
> >          {0xffff, 0xfffb, 0xfff9, 0xfff8};
> > @@ -3311,6 +3332,9 @@ static int vtd_irte_get(IntelIOMMUState *iommu, uint16_t index,
> >      if (index >= iommu->intr_size) {
> >          error_report_once("%s: index too large: ind=0x%x",
> >                            __func__, index);
> > +        if (do_fault) {
> > +            vtd_report_ir_fault(iommu, sid, VTD_FR_IR_INDEX_OVER, index);
> > +        }
> 
> IIUC it's only the fault reason to report, then I am thinking if it's
> easier to let the caller taking care of that?
> 
> Though we'll need conditions for fault disabled, e.g....
> 
> >          return -VTD_FR_IR_INDEX_OVER;

Well, remember I want to kill that *return* of the VTD_FR_xxx error, so
although it looks like duplication now, it won't be.


> >      }
> >  
> > @@ -3319,17 +3343,33 @@ static int vtd_irte_get(IntelIOMMUState *iommu, uint16_t index,
> >                          entry, sizeof(*entry), MEMTXATTRS_UNSPECIFIED)) {
> >          error_report_once("%s: read failed: ind=0x%x addr=0x%" PRIx64,
> >                            __func__, index, addr);
> > +        if (do_fault) {
> > +            vtd_report_ir_fault(iommu, sid, VTD_FR_IR_ROOT_INVAL, index);
> > +        }
> >          return -VTD_FR_IR_ROOT_INVAL;
> >      }
> >  
> >      trace_vtd_ir_irte_get(index, le64_to_cpu(entry->data[1]),
> >                            le64_to_cpu(entry->data[0]));
> >  
> > +       /*
> > +        * The remaining potential fault conditions are "qualified" by the
> > +        * Fault Processing Disable bit in the IRTE. Even "not present".
> > +        * So just clear the do_fault flag if PFD is set, which will
> > +        * prevent faults being raised.
> > +        */
> > +       if (entry->irte.fault_disable) {
> > +               do_fault = false;
> > +    }
> > +
> >      if (!entry->irte.present) {
> >          error_report_once("%s: detected non-present IRTE "
> >                            "(index=%u, high=0x%" PRIx64 ", low=0x%" PRIx64 ")",
> >                            __func__, index, le64_to_cpu(entry->data[1]),
> >                            le64_to_cpu(entry->data[0]));
> > +        if (do_fault) {
> > +            vtd_report_ir_fault(iommu, sid, VTD_FR_IR_ENTRY_P, index);
> > +        }
> >          return -VTD_FR_IR_ENTRY_P;
> 
> ... here IIUC we can do:
> 
>   if (entry->irte.fault_disable)
>         return 0;
>   else
>         return -VTD_FR_IR_ENTRY_P;
>         
> Hence when error occurs we always keep the error report above, and let the
> caller report IR fault when <0.  It seems this will at least avoid plenty
> of same calls within vtd_irte_get().
> 
> I do see a few others outside vtd_irte_get().  In short, it'll be nice to
> avoid calling this same pattern in multiple places somehow.

I suppose we could pass the do_fault *into* vtd_report_ir_fault(). It's
a similar pattern to vtd_report_fault() which also takes an is_fpd_set
argument.

(Note: If I could tolerate just passing VTD_FRCD_IR_IDX(index) I think
I could actually just *call* the existing vtd_report_fault() without
having to touch any of the fault reporting functions at all. The bits
do all end up in the right place. It just seemed a bit icky.)

I did briefly ponder just returning the value and then letting the
caller do the report, but then we'd *also* have to make the return code
distinguish between "failed, but do not report a fault" and "succeeded,
and your translation is valid" cases. I figured I preferred it this
way.


> > --- a/hw/i386/intel_iommu_internal.h
> > +++ b/hw/i386/intel_iommu_internal.h
> > @@ -268,6 +268,7 @@
> >  #define VTD_FRCD_FI(val)        ((val) & ~0xfffULL)
> >  #define VTD_FRCD_PV(val)        (((val) & 0xffffULL) << 40)
> >  #define VTD_FRCD_PP(val)        (((val) & 0x1) << 31)
> > +#define VTD_FRCD_IR_IDX(val)    (((val) & 0xffffULL) << 48)


Think 'val' probably needs casting to a (uint64_t) before the shift there.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]