| Message ID | 20200322161418.31606-8-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bunch of typesafe conversion | expand |
On 22.03.2020 17:14, julien@xen.org wrote: > From: Julien Grall <jgrall@amazon.com> > > Note that the code is now using cr3_to_mfn() to get the MFN. This is > slightly different as the top 12-bits will now be masked. And here I agree with the change. Hence it is even more so important that the patch introducing the new helper(s) first gets sorted. Should there be further patches in this series with this same interaction issue, I won't point it out again and may not respond at all if I see no other issues. Jan
On 26/03/2020 15:54, Jan Beulich wrote: > On 22.03.2020 17:14, julien@xen.org wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> Note that the code is now using cr3_to_mfn() to get the MFN. This is >> slightly different as the top 12-bits will now be masked. > > And here I agree with the change. Hence it is even more so important > that the patch introducing the new helper(s) first gets sorted. > Should there be further patches in this series with this same > interaction issue, I won't point it out again and may not respond at > all if I see no other issues. I will update the commit message explaining the reason of using cr3_to_mfn() and look at the other user. Cheers,
On 18/04/2020 12:01, Julien Grall wrote: > On 26/03/2020 15:54, Jan Beulich wrote: >> On 22.03.2020 17:14, julien@xen.org wrote: >>> From: Julien Grall <jgrall@amazon.com> >>> >>> Note that the code is now using cr3_to_mfn() to get the MFN. This is >>> slightly different as the top 12-bits will now be masked. >> >> And here I agree with the change. Hence it is even more so important >> that the patch introducing the new helper(s) first gets sorted. >> Should there be further patches in this series with this same >> interaction issue, I won't point it out again and may not respond at >> all if I see no other issues. > > I will update the commit message explaining the reason of using > cr3_to_mfn() and look at the other user. Looking at the code again, there are a few users that don't mask the top 12-bits. I am trying to understand why this has never been an issue so far. Wouldn't it break when bit 63 (no flush) is set? If so, maybe I should split the work from typesafe. Cheers,
On 18.04.2020 13:43, Julien Grall wrote: > On 18/04/2020 12:01, Julien Grall wrote: >> On 26/03/2020 15:54, Jan Beulich wrote: >>> On 22.03.2020 17:14, julien@xen.org wrote: >>>> From: Julien Grall <jgrall@amazon.com> >>>> >>>> Note that the code is now using cr3_to_mfn() to get the MFN. This is >>>> slightly different as the top 12-bits will now be masked. >>> >>> And here I agree with the change. Hence it is even more so important >>> that the patch introducing the new helper(s) first gets sorted. >>> Should there be further patches in this series with this same >>> interaction issue, I won't point it out again and may not respond at >>> all if I see no other issues. >> >> I will update the commit message explaining the reason of using cr3_to_mfn() and look at the other user. > > Looking at the code again, there are a few users that don't mask the top 12-bits. I am trying to understand why this has never been an issue so far. > > Wouldn't it break when bit 63 (no flush) is set? Yes; I guess those uses are case where bit 63 can't / won't be set. Just like the register, which doesn't store the bit, I think we avoid storing the bit set as well. But correctness of the non- masking variants can only be established by looking at every individual site. > If so, maybe I should split the work from typesafe. Maybe better indeed. Jan
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 04a3ebc0a2..4f524dc71e 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -1232,7 +1232,8 @@ enum pf_type { static enum pf_type __page_fault_type(unsigned long addr, const struct cpu_user_regs *regs) { - unsigned long mfn, cr3 = read_cr3(); + mfn_t mfn; + unsigned long cr3 = read_cr3(); l4_pgentry_t l4e, *l4t; l3_pgentry_t l3e, *l3t; l2_pgentry_t l2e, *l2t; @@ -1264,20 +1265,20 @@ static enum pf_type __page_fault_type(unsigned long addr, page_user = _PAGE_USER; - mfn = cr3 >> PAGE_SHIFT; + mfn = cr3_to_mfn(cr3); - l4t = map_domain_page(_mfn(mfn)); + l4t = map_domain_page(mfn); l4e = l4e_read_atomic(&l4t[l4_table_offset(addr)]); - mfn = l4e_get_pfn(l4e); + mfn = l4e_get_mfn(l4e); unmap_domain_page(l4t); if ( ((l4e_get_flags(l4e) & required_flags) != required_flags) || (l4e_get_flags(l4e) & disallowed_flags) ) return real_fault; page_user &= l4e_get_flags(l4e); - l3t = map_domain_page(_mfn(mfn)); + l3t = map_domain_page(mfn); l3e = l3e_read_atomic(&l3t[l3_table_offset(addr)]); - mfn = l3e_get_pfn(l3e); + mfn = l3e_get_mfn(l3e); unmap_domain_page(l3t); if ( ((l3e_get_flags(l3e) & required_flags) != required_flags) || (l3e_get_flags(l3e) & disallowed_flags) ) @@ -1286,9 +1287,9 @@ static enum pf_type __page_fault_type(unsigned long addr, if ( l3e_get_flags(l3e) & _PAGE_PSE ) goto leaf; - l2t = map_domain_page(_mfn(mfn)); + l2t = map_domain_page(mfn); l2e = l2e_read_atomic(&l2t[l2_table_offset(addr)]); - mfn = l2e_get_pfn(l2e); + mfn = l2e_get_mfn(l2e); unmap_domain_page(l2t); if ( ((l2e_get_flags(l2e) & required_flags) != required_flags) || (l2e_get_flags(l2e) & disallowed_flags) ) @@ -1297,9 +1298,9 @@ static enum pf_type __page_fault_type(unsigned long addr, if ( l2e_get_flags(l2e) & _PAGE_PSE ) goto leaf; - l1t = map_domain_page(_mfn(mfn)); + l1t = map_domain_page(mfn); l1e = l1e_read_atomic(&l1t[l1_table_offset(addr)]); - mfn = l1e_get_pfn(l1e); + mfn = l1e_get_mfn(l1e); unmap_domain_page(l1t); if ( ((l1e_get_flags(l1e) & required_flags) != required_flags) || (l1e_get_flags(l1e) & disallowed_flags) )