| Message ID | d21f3b6d-d1c5-be00-6a1d-1944099b4bbc@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 23.05.17 at 15:40, <boris.ostrovsky@oracle.com> wrote: > And you haven't been able to reproduce this? I see this fail on two AMD > systems (different processor families). I didn't even have the time to try. > And this: > > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -560,7 +560,7 @@ int p2m_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, > { > if ( hap_enabled(d) ) > { > - unsigned long fn_mask = !mfn_eq(mfn, INVALID_MFN) ? > + unsigned long fn_mask = (!mfn_eq(mfn, INVALID_MFN) || 1)? > (gfn | mfn_x(mfn) | todo) : (gfn | todo); > > order = (!(fn_mask & ((1ul << PAGE_ORDER_1G) - 1)) && > > > makes the problem go away. Interesting. I took another look at p2m-pt.c, this time paying particular attention to INVALID_MFN uses. And right the first one may already provide a hint: Perhaps we now need L2 and L3 counterparts to p2m_l1e_from_pfn(). Further changes may then be needed to the splitting of large pages (in p2m_next_level()) depending on whether INVALID_MFN entries can make it there. Jan
On 05/23/2017 10:05 AM, Jan Beulich wrote: >>>> On 23.05.17 at 15:40, <boris.ostrovsky@oracle.com> wrote: >> And you haven't been able to reproduce this? I see this fail on two AMD >> systems (different processor families). > I didn't even have the time to try. > >> And this: >> >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -560,7 +560,7 @@ int p2m_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, >> { >> if ( hap_enabled(d) ) >> { >> - unsigned long fn_mask = !mfn_eq(mfn, INVALID_MFN) ? >> + unsigned long fn_mask = (!mfn_eq(mfn, INVALID_MFN) || 1)? >> (gfn | mfn_x(mfn) | todo) : (gfn | todo); >> >> order = (!(fn_mask & ((1ul << PAGE_ORDER_1G) - 1)) && >> >> >> makes the problem go away. > Interesting. I took another look at p2m-pt.c, this time paying > particular attention to INVALID_MFN uses. And right the first one > may already provide a hint: Perhaps we now need L2 and L3 > counterparts to p2m_l1e_from_pfn(). Defining p2m_l2e_from_pfn indeed helps a bit with HVM --- the guest now goes as far as loading balloon driver (when it crashes). > Further changes may then > be needed to the splitting of large pages (in p2m_next_level()) > depending on whether INVALID_MFN entries can make it there. Let me see what I can do here. -boris
On 05/23/2017 10:32 AM, Boris Ostrovsky wrote: > On 05/23/2017 10:05 AM, Jan Beulich wrote: >>>>> On 23.05.17 at 15:40, <boris.ostrovsky@oracle.com> wrote: >>> And you haven't been able to reproduce this? I see this fail on two AMD >>> systems (different processor families). >> I didn't even have the time to try. >> >>> And this: >>> >>> --- a/xen/arch/x86/mm/p2m.c >>> +++ b/xen/arch/x86/mm/p2m.c >>> @@ -560,7 +560,7 @@ int p2m_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, >>> { >>> if ( hap_enabled(d) ) >>> { >>> - unsigned long fn_mask = !mfn_eq(mfn, INVALID_MFN) ? >>> + unsigned long fn_mask = (!mfn_eq(mfn, INVALID_MFN) || 1)? >>> (gfn | mfn_x(mfn) | todo) : (gfn | todo); >>> >>> order = (!(fn_mask & ((1ul << PAGE_ORDER_1G) - 1)) && >>> >>> >>> makes the problem go away. >> Interesting. I took another look at p2m-pt.c, this time paying >> particular attention to INVALID_MFN uses. And right the first one >> may already provide a hint: Perhaps we now need L2 and L3 >> counterparts to p2m_l1e_from_pfn(). > Defining p2m_l2e_from_pfn indeed helps a bit with HVM --- the guest now > goes as far as loading balloon driver (when it crashes). > > >> Further changes may then >> be needed to the splitting of large pages (in p2m_next_level()) >> depending on whether INVALID_MFN entries can make it there. > Let me see what I can do here. TBH, I don't see what needs to be done in p2m_next_level(). mfn doesn't enter the calculation there. -boris
>>> On 24.05.17 at 00:25, <boris.ostrovsky@oracle.com> wrote: > On 05/23/2017 10:32 AM, Boris Ostrovsky wrote: >> On 05/23/2017 10:05 AM, Jan Beulich wrote: >>> Further changes may then >>> be needed to the splitting of large pages (in p2m_next_level()) >>> depending on whether INVALID_MFN entries can make it there. >> Let me see what I can do here. > > TBH, I don't see what needs to be done in p2m_next_level(). mfn doesn't > enter the calculation there. The relevant variables are named "pfn" there, and so far I'm only guessing there might be an issue (or actually I meanwhile thing it should only be a cosmetic one): What's being read out of the old PTE is being taken and incremented for each split PTE. That'll (in the 2Mb page case) result in 4k PTEs referencing MFNs 0xffffffffff and 0 ... 0x1fe. But generally this should be harmless, as these are non-present PTEs, and the frame numbers read back out of non-present PTEs should be of no interest to anyone. I'm pondering to convert the code to use mfn_add() (perhaps we should also have mfn_inc()), making the helper saturate. In any event I hope to find time later today to look into the issue myself. I'm kind of disappointed that there was no visible attempt from Igor so far to help the situation. Jan
On 24/05/2017 07:20, Jan Beulich wrote: > I'm kind of disappointed that there was no visible attempt > from Igor so far to help the situation. Igor is out of the office at the moment, and besides, this thread of problem is only 36h old at this point, which is a single working day in this timezone. ~Andrew
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index c6ec1a4..0051623 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -560,7 +560,7 @@ int p2m_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, { if ( hap_enabled(d) ) { - unsigned long fn_mask = !mfn_eq(mfn, INVALID_MFN) ? + unsigned long fn_mask = (!mfn_eq(mfn, INVALID_MFN) || 1)? (gfn | mfn_x(mfn) | todo) : (gfn | todo);