Re: [PATCH 1/1] x86/vtd: Mask DMAR faults for IGD devices

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.04.2020 15:36, Brendan Kerrigan wrote:
> The Intel graphics device records DMAR faults regardless
> of the Fault Process Disable bit being set.

I can't seem to be able to find a place where we would set FPD.
Why do we need the workaround then, or what am I missing?

> When this fault
> occurs, enable the Interrupt Mask (IM) bit in the Fault
> Event Control (FECTL) register to prevent the continued
> recording of the fault.

This should mention the underlying erratum in one way or another.

> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -41,6 +41,8 @@
>  #include "vtd.h"
>  #include "../ats.h"
>  
> +#define IS_IGD(seg, id) (0 == seg && 0 == PCI_BUS(id) && 2 == PCI_SLOT(id) 
> && 0 == PCI_FUNC(id))
> +
>  struct mapped_rmrr {
>      struct list_head list;
>      u64 base, end;
> @@ -872,6 +874,13 @@ static int iommu_page_fault_do_one(struct vtd_iommu 
> *iommu, int type,
>      printk(XENLOG_G_WARNING VTDPREFIX "%s: reason %02x - %s\n",
>             kind, fault_reason, reason);
>  
> +    if ( DMA_REMAP == fault_type && type && IS_IGD(seg, source_id) ) {

Using existing infrastructure would be at least some improvement, in
particular to avoid this workaround triggering on unaffected
hardware (including such where there's something else at 0000:00:02.0).
See e.g. is_igd_drhd(), and note that most uses of it are currently
further qualified to limit what hardware quirk workarounds get applied
to. In any event you'll want to clarify in the description that this
won't ever be done on hardware not having this issue, at least as long
as this isn't obvious from the code.

Also - why the "type" part of the conditional? The erratum text
doesn't talk about only DMA reads being affected.

Finally, style-wise the brace belongs on its own line.

> +        u32 fectl = dmar_readl(iommu->reg, DMAR_FECTL_REG);
> +        fectl |= DMA_FECTL_IM;

While this is the recommended workaround, I don't see you undoing
the masking anywhere later, and I'm also unconvinced this should be
done upon seeing the first fault. One option might be to do so on the
the first fault when FPD is set for the offending device. Or else,
following the title, it might want/need doing unconditionally at
initialization time.

> +        dmar_writel(iommu->reg, DMAR_FECTL_REG, fectl);
> +        printk(XENLOG_G_WARNING VTDPREFIX "Disabling DMAR faults for IGD\n");

If, as suggested above, IM will get cleared again at some point,
this printk() should imo not be issued more than once.

Jan