Re: [Xen-devel] [PATCH V2 2/6] iommu/arm: Add ability to handle deferred probing request

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 14/08/2019 20:25, Stefano Stabellini wrote:
> On Wed, 14 Aug 2019, Julien Grall wrote:
> I agree that we should enable all IOMMUs or none. I don't think we want
> to deal with partially initialized IOMMUs systems.
>
> Failure to enable all IOMMUs should lead to returning an error from the
> relevant function (arch_iommu_populate_page_table?) which should
The patch is:

|> start_xen()
|>   iommu_setup()
|>     iommu_hardware_setup()

> translate into Xen failing to boot and crashing. Which I think it is
> what you are suggesting, right?
That's correct. At the moment the return value of iommu_setup() is ignored. What 
I would like to suggest is something along the following lines:
rc = iommu_setup();
if ( iommu_enable && rc != -ENODEV )
  panic("Unable to setup IOMMUs");

> (I wouldn't call panic() inside the IOMMU specific initializer, because
> there might be iommu= command line options for Xen that specify a
> different desired outcome. I would deal with this case the same way we
> would deal with an error during initialization of a single IOMMU.)
I am not sure to understand this. If you have an half initialized IOMMU (note 
the "single" here!), then continuing is likely to make things much worst.
I don't advocate to put the panic() inside the IOMMU specific initializer (see 
above). But clearly, we should return an error no matter the content of 'iommu' 
command line if the user requested to enable the IOMMUs (if any). It wouldn't be 
right if the user can say "ignore IOMMU error" as most likely you will have 
unexpected error afterwards.
Cheers,

--
Julien Grall

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