Hi Julien,
On 08/06/2016 04:34 PM, Julien Grall wrote: > > > On 06/08/2016 14:45, Sergej Proskurin wrote: >> Hi Julien, > > Hello Sergej, > >> On 08/04/2016 04:04 PM, Julien Grall wrote: >>> On 01/08/16 18:10, Sergej Proskurin wrote: >>>> + return rc; >>>> + >>>> + hp2m = p2m_get_hostp2m(d); >>>> + ap2m = d->arch.altp2m_p2m[idx]; >>>> + >>>> + altp2m_lock(d); >>>> + >>>> + /* >>>> + * Flip mem_access_enabled to true when a permission is set, as >>>> to prevent >>>> + * allocating or inserting super-pages. >>>> + */ >>>> + ap2m->mem_access_enabled = true; >>> >>> Can you give more details about why you need this? >>> >> >> Similar to altp2m_set_mem_access, if we remap a page that is part of a >> super page in the hostp2m, we first map the superpage in form of 512 >> pages into the ap2m and then change only one page. So, we set >> mem_access_enabled to true to shatter the superpage on the ap2m side. > > mem_access_enabled should only be set when mem access is enabled and > nothing. > > I don't understand why you want to avoid superpage in the altp2m. If > you copy a host mapping is a superpage, then a altp2m mapping should > be a superpage. > > The code is able to cope with inserting a mapping in the middle of a > superpage without mem_access_enabled. > Alright, I will try it out in the next patch. Thank you. >> >>>> + >>>> + mfn = p2m_lookup_attr(ap2m, old_gfn, &p2mt, &level, NULL, NULL); >>>> + >>>> + /* Check whether the page needs to be reset. */ >>>> + if ( gfn_eq(new_gfn, INVALID_GFN) ) >>>> + { >>>> + /* If mfn is mapped by old_gpa, remove old_gpa from the >>>> altp2m table. */ >>>> + if ( !mfn_eq(mfn, INVALID_MFN) ) >>>> + { >>>> + rc = remove_altp2m_entry(d, ap2m, old_gpa, >>>> pfn_to_paddr(mfn_x(mfn)), level); >>> >>> remove_altp2m_entry should take a gfn and mfn in parameter and not an >>> address. The latter is a call for misusage of the API. >>> >> >> Ok. This will also remove the need for level_sizes/level_masks in the >> associated function. >> >>>> + if ( rc ) >>>> + { >>>> + rc = -EINVAL; >>>> + goto out; >>>> + } >>>> + } >>>> + >>>> + rc = 0; >>>> + goto out; >>>> + } >>>> + >>>> + /* Check host p2m if no valid entry in altp2m present. */ >>>> + if ( mfn_eq(mfn, INVALID_MFN) ) >>>> + { >>>> + mfn = p2m_lookup_attr(hp2m, old_gfn, &p2mt, &level, NULL, >>>> &xma); >>>> + if ( mfn_eq(mfn, INVALID_MFN) || (p2mt != p2m_ram_rw) ) >>> >>> Please add a comment to explain why the second check. >>> >> >> Ok, I will. It has the same reason as in patch #19: It is not sufficient >> so simply check for invalid MFN's as the type might be invalid. Also, >> the x86 implementation did not allow to remap a gfn to a shared page. > > Patch #19 has a different check which does not explain this one. (p2mt > != p2m_ram_rw) only guest read-write RAM can effectively be remapped > which is different than shared page cannot be remapped. > > BTW, ARM does not support shared page. > > This also lead to my question, why not allowing p2m_ram_ro? > I don't see a reason why not. Thank you. I will remove the check. >> >>>> + { >>>> + rc = -EINVAL; >>>> + goto out; >>>> + } >>>> + >>>> + /* If this is a superpage, copy that first. */ >>>> + if ( level != 3 ) >>>> + { >>>> + rc = modify_altp2m_entry(d, ap2m, old_gpa, >>>> pfn_to_paddr(mfn_x(mfn)), >>>> + level, p2mt, memaccess[xma]); >>>> + if ( rc ) >>>> + { >>>> + rc = -EINVAL; >>>> + goto out; >>>> + } >>>> + } >>>> + } >>>> + >>>> + mfn = p2m_lookup_attr(ap2m, new_gfn, &p2mt, &level, NULL, &xma); >>>> + >>>> + /* If new_gfn is not part of altp2m, get the mapping information >>>> from hp2m */ >>>> + if ( mfn_eq(mfn, INVALID_MFN) ) >>>> + mfn = p2m_lookup_attr(hp2m, new_gfn, &p2mt, &level, NULL, >>>> &xma); >>>> + >>>> + if ( mfn_eq(mfn, INVALID_MFN) || (p2mt != p2m_ram_rw) ) >>> >>> Please add a comment to explain why the second check. >>> >> >> Same reason as above. > > Then add a comment in the code. I will also remove this check. Best regards, ~Sergej _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel