On Mon, Jan 7, 2013 at 7:26 AM, Konrad Rzeszutek Wilk <konrad.w...@oracle.com> wrote: > On Fri, Jan 04, 2013 at 02:10:25PM -0800, Yinghai Lu wrote: >> On Fri, Jan 4, 2013 at 1:02 PM, Shuah Khan <shuahk...@gmail.com> wrote: >> > Pani'cing the system doesn't sound like a good option to me in this >> > case. This change to disable swiotlb is made for kdump. However, with >> > this change several system fail to boot, unless crashkernel_low=72M is >> > specified. >> >> this patchset is new feature to put second kdump kernel above 4G. >> >> > >> > I would the say the right approach to solve this would be to not >> > change the current pci_swiotlb_detect_override() behavior and treat >> > swiotlb =1 upon entry equivalent to swiotlb_force set. >> >> that will make intel system have to take crashkernel_low=72M too. >> otherwise intel system will get panic during swiotlb allocation. > > Two things: > > 1). You need to wrap the 'is_enough_..' in CONFIG_KEXEC, which means > that the function needs to go in a header file.
kdump is just one case, user still can boot the system with memmap=... to hit the panic. > 2). The check for 1MB is suspect. Why only 1MB? You mentioned it is > b/c of crashkernel_low=72M (which I am not seeing in v3.8 > kernel-parameters.txt? > Is that part of your mega-patchset?). Anyhow, there seems to be a > disconnect - > what if the user supplied crashkernel_low=27M? Perhaps the 'is_enough' > should also parse the bootparams to double-check that there is enough > low-mem space? But then if the kernel grows then 72M might not be enough > - > you might need 82M with 3.9. again, could with memmap= include or exclude case, so parse boot command line is not going to handle all the case. > > Perhaps a better way for this is to do: > 1). Change 'is_enough' to check only for 4MB. > 2). When booting as kexec, the SWIOTLB would only use 4MB instead of > 64MB? can not tell it is from kexec path. kexec just get boot loader type assigned. > > Or, we could also use the post-late SWIOTLB initialization similiary to > how it was > done on ia64. This would mean that the AMD VI code would just call the > .. something like this - NOT tested or even compile tested: > > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c > index c1c74e0..e7fa8f7 100644 > --- a/drivers/iommu/amd_iommu.c > +++ b/drivers/iommu/amd_iommu.c > @@ -3173,6 +3173,24 @@ int __init amd_iommu_init_dma_ops(void) > if (unhandled && max_pfn > MAX_DMA32_PFN) { > /* There are unhandled devices - initialize swiotlb for them > */ > swiotlb = 1; > + /* Late (so no bootmem allocator) usage and only if the early > SWIOTLB > + * hadn't been allocated (which can happen on kexec kernels > booted > + * above 4GB). */ > + if (!swiotlb_nr_tbl()) { > + int retry = 3; > + int mb_size = 64; > + int rc = 0; > +retry_me: > + if (retry < 0) > + panic("We tried setting %dMB for SWIOTLB but > got -ENOMEM", mb_size << 1); > + rc = swiotlb_late_init_with_default_size(mb_size * > (1<<20)); > + if (rc) { > + retry --; > + mb_size >> 1; > + goto retry_me; > + } > + dma_ops = &swiotlb_dma_ops; > + } > } > > amd_iommu_stats_init(); > > And then the early SWIOTLB initialization for 64MB can fail and we are still > OK. no, that is overkill. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/