* Jeremy Kerr <j...@ozlabs.org> [2019-10-03 15:07:24]: > Hi Vasant, > > > > OK. How about we just don't do that? > > > > Yes. Hostboot will fix that. It will make sure that HBRT is loaded > > into regular memory. > > Super. > > > > It sounds like we're just trying to work around an invalid > > > representation of the mappings. > > > > Its not workaround. Its additional check. > > The issue is that you've added a check for stuff that the kernel doesn't > (and shouldn't) know about, and assumed that the kernel knows better > than the device tree. It may be the correct thing to do in this case, > but we can't guarantee that it's always correct.
Good point on the policy ownership. > For example, what if there is a future HBRT range that is fine to go > into NVRAM? With this change, that's not possible. The current topic is who owns setting up the ATT bits for that piece of memory. It is the kernel today. Kernel decides to set this up as normal memory or I/O memory and sets the bits in page table entry. > Or, what if there's a range of address-space that isn't backed by system > RAM (say, some MMIO-mapped hardware) that we want to expose to a future > HBRT implementation? This change will block that. > > The kernel doesn't know what is and is not valid for a HBRT mapping, so > it has no reason to override what's specified in the device tree. We've > designed this so that the kernel provides the mechanism for mapping > pages, and not the policy of which pages can be mapped. The features altered are cache inhibit and guarding which affects ability to fetch instructions. If we allow HBRT to reside in an I/O memory, the we need to tell kernel that it is ok to allow caching and instruction execution in that region and accordingly change the ATT bits. This patch does not block a working function, but actually makes debugging a failed case easier. The failing scenario without this check is such that HBRT cannot fetch from the region of memory and loops in minor page faults doing nothing. As Vasant mentioned hostboot team will add code to relocate the HBRT to the right place. Addressing your concern, if we end up allowing HBRT in non system-RAM area, we need to add some more flags in device tree to instruct the driver to force change the page protection bits as page_prot = pgprot_cached(page_prot); It does not make sense to keep the region cache inhibited and just clear the Guard bit (ATT=0b10 - non-idempotent I/O) so we should force to normal memory ATT=0b00. In summary, this check does not block any working function today. We fail to execute HBRT code after we successfully mmap() the memory anyway. When we need to move firmware components to other types of memory, we should do a future patch to indicate in device tree that this is non system-RAM and kernel should change PTE permissions and then setup the mmap(). Or HBRT really has a use for NVRAM in which case we explicitly need to indicate (via device-tree) the required attribute for this mapping. --Vaidy
