Re: [Xen-devel] [PATCH v3 6/7] xen/dom0: Drop iommu_hwdom_inclusive entirely

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.01.19 at 10:00, <andrew.cooper3@xxxxxxxxxx> wrote:
> This option is unique to x86 PV dom0's, but it is not sensible to have a
> catch-all which blindly maps all non-RAM regions into the IOMMU.
> 
> The map-reserved option remains, and covers all the buggy firmware issues that
> I am aware of.

The final part of this sentence is the main culprit, resulting in me
being uncertain about this move. I'm not outright opposed, since
I agree with the "is not sensible" part above. But there surely
was a reason why the option was introduced. May I suggest as
an alternative to take an intermediate step for at least one
release cycle, and attach a warning_add() to any use of this
option?

> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -172,10 +172,10 @@ static bool __hwdom_init hwdom_iommu_map(const struct 
> domain *d,
>      default:
>          if ( type & RAM_TYPE_RESERVED )
>          {
> -            if ( !iommu_hwdom_inclusive && !iommu_hwdom_reserved )
> +            if ( !iommu_hwdom_reserved )
>                  return false;
>          }
> -        else if ( is_hvm_domain(d) || !iommu_hwdom_inclusive || pfn > 
> max_pfn )
> +        else if ( is_hvm_domain(d) || pfn > max_pfn )
>              return false;
>      }

Removal of this option could be imagined taking an intermediate
step where the variable resolves to constant false. This suggests
to me that the first of these two changes is correct, but the
second is not. Instead the "else if()" ought to collapse to just
"else", which in turn would suggest to consider folding if()/else
into a single if().

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel