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 <d...@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.
smime.p7s
Description: S/MIME cryptographic signature
