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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 15/08/2019 10:29, Julien Grall wrote:
> 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.

I noticed there was already a panic() in iommu_setup() just in case the user
force the use of IOMMU but they were not initialized. I was half-tempted to set
iommu_force to true for Arm, but I think this is a different issue.

So here my take (not tested nor compiled).

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 2c5d1372c0..8f94f618b0 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -755,6 +755,7 @@ void __init start_xen(unsigned long boot_phys_offset,
         .max_grant_frames = gnttab_dom0_frames(),
         .max_maptrack_frames = opt_max_maptrack_frames,
     };
+    int rc;
 
     dcache_line_bytes = read_dcache_line_bytes();
 
@@ -890,7 +891,9 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     setup_virt_paging();
 
-    iommu_setup();
+    rc = iommu_setup();
+    if ( !iommu_enabled && rc != -ENODEV )
+        panic("Couldn't configure correctly all the IOMMUs.");
 
     do_initcalls();
 
diff --git a/xen/drivers/passthrough/arm/iommu.c 
b/xen/drivers/passthrough/arm/iommu.c
index 2135233736..f219de9ac3 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -51,6 +51,14 @@ int __init iommu_hardware_setup(void)
         rc = device_init(np, DEVICE_IOMMU, NULL);
         if ( !rc )
             num_iommus++;
+        /*
+         * Ignore the following error codes:
+         *   - EBADF: Indicate the current not is not an IOMMU
+         *   - ENODEV: The IOMMU is not present or cannot be used by
+         *     Xen.
+         */
+        else if ( rc != -EBADF && rc != -ENODEV )
+            return rc;
     }
 
     return ( num_iommus > 0 ) ? 0 : -ENODEV;




> 
> Cheers,
> 

-- 
Julien Grall

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