Re: intel-iommu: Report interrupt remapping faults, fix return value

[Peter Xu]

On Tue, Jul 25, 2023 at 11:01:16AM +0100, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> A generic X86IOMMUClass->int_remap function should not return VT-d
> specific values; fix it to return 0 if the interrupt was successfully
> translated or -EINVAL if not.
> 
> The VTD_FR_IR_xxx values are supposed to be used to actually raise
> faults through the fault reporting mechanism, so do that instead for
> the case where the IRQ is actually being injected.
> 
> There is more work to be done here, as pretranslations for the KVM IRQ
> routing table can't fault; an untranslatable IRQ should be handled in
> userspace and the fault raised only when the IRQ actually happens (if
> indeed the IRTE is still not valid at that time). But we can work on
> that later; we can at least raise faults for the direct case.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

Mostly good to me, only still a few trivial comments..

[...]

> @@ -3320,17 +3342,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);
> -        return -VTD_FR_IR_ROOT_INVAL;
> +        if (do_fault) {
> +            vtd_report_ir_fault(iommu, sid, VTD_FR_IR_ROOT_INVAL, index);
> +        }
> +        return false;
>      }
>  
>      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;

Wrong indent on these lines..

> +    }
> +
>      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;

Forgot to change retval?

>      }

Thanks,

-- 
Peter Xu