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 Xenfirstname.lastname@example.org https://lists.xenproject.org/mailman/listinfo/xen-devel