Do not translate INVALID_GFN as l2 guest gfn into l1 guest gfn. Pass correct pfec for translation into l1 guest gfn. Found with Hyper-V. Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> CC: Tim Deegan <tim@xen.org> -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85689 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Hi, At 17:37 +0200 on 31 Jul (1343756240), Christoph Egger wrote:> Do not translate INVALID_GFN as l2 guest gfn into l1 guest gfn.Why not? l2 gfns don''t have any special meaning that we can dictate from inside Xen.> Pass correct pfec for translation into l1 guest gfn.This seems like a good idea, but probably should happen for all entries, not just INVALID_GFN ones -- we shouldn''t be returning a PFEC to the guest that comes from translations outside his control. How about this: diff -r fdd4b7b36959 xen/arch/x86/mm/p2m.c --- a/xen/arch/x86/mm/p2m.c Thu Aug 02 12:04:31 2012 +0100 +++ b/xen/arch/x86/mm/p2m.c Thu Aug 02 12:17:48 2012 +0100 @@ -1581,6 +1581,7 @@ unsigned long paging_gva_to_gfn(struct v unsigned long gfn; struct p2m_domain *p2m; const struct paging_mode *mode; + uint32_t pfec_21 = *pfec; uint64_t ncr3 = nhvm_vcpu_hostcr3(v); /* translate l2 guest va into l2 guest gfn */ @@ -1590,7 +1591,7 @@ unsigned long paging_gva_to_gfn(struct v /* translate l2 guest gfn into l1 guest gfn */ return hostmode->p2m_ga_to_gfn(v, hostp2m, ncr3, - gfn << PAGE_SHIFT, pfec, NULL); + gfn << PAGE_SHIFT, &pfec_21, NULL); } return hostmode->gva_to_gfn(v, hostp2m, va, pfec); Cheers, Tim.> Found with Hyper-V. > > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> > CC: Tim Deegan <tim@xen.org> > > -- > ---to satisfy European Law for business letters: > Advanced Micro Devices GmbH > Einsteinring 24, 85689 Dornach b. Muenchen > Geschaeftsfuehrer: Alberto Bozzo > Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen > Registergericht Muenchen, HRB Nr. 43632Content-Description: xen_p2m.diff> diff -r 8330198c3240 xen/arch/x86/mm/p2m.c > --- a/xen/arch/x86/mm/p2m.c Fri Jul 27 12:24:03 2012 +0200 > +++ b/xen/arch/x86/mm/p2m.c Tue Jul 31 16:49:54 2012 +0200 > @@ -1582,12 +1582,19 @@ unsigned long paging_gva_to_gfn(struct v > struct p2m_domain *p2m; > const struct paging_mode *mode; > uint64_t ncr3 = nhvm_vcpu_hostcr3(v); > + uint32_t pfec1 = *pfec; > > /* translate l2 guest va into l2 guest gfn */ > p2m = p2m_get_nestedp2m(v, ncr3); > mode = paging_get_nestedmode(v); > gfn = mode->gva_to_gfn(v, p2m, va, pfec); > > + /* if l1 guest maps its mmio pages into the > + * l2 guest then we see this case here. */ > + if (gfn == INVALID_GFN) > + return INVALID_GFN; > + *pfec = pfec1; > + > /* translate l2 guest gfn into l1 guest gfn */ > return hostmode->p2m_ga_to_gfn(v, hostp2m, ncr3, > gfn << PAGE_SHIFT, pfec, NULL);> _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On 08/02/12 13:19, Tim Deegan wrote:> Hi, > > At 17:37 +0200 on 31 Jul (1343756240), Christoph Egger wrote: >> Do not translate INVALID_GFN as l2 guest gfn into l1 guest gfn. > > Why not? l2 gfns don''t have any special meaning that we can > dictate from inside Xen. > >> Pass correct pfec for translation into l1 guest gfn. > > This seems like a good idea, but probably should happen for all > entries, not just INVALID_GFN ones -- we shouldn''t be returning a PFEC > to the guest that comes from translations outside his control. > > How about this: > > diff -r fdd4b7b36959 xen/arch/x86/mm/p2m.c > --- a/xen/arch/x86/mm/p2m.c Thu Aug 02 12:04:31 2012 +0100 > +++ b/xen/arch/x86/mm/p2m.c Thu Aug 02 12:17:48 2012 +0100 > @@ -1581,6 +1581,7 @@ unsigned long paging_gva_to_gfn(struct v > unsigned long gfn; > struct p2m_domain *p2m; > const struct paging_mode *mode; > + uint32_t pfec_21 = *pfec; > uint64_t ncr3 = nhvm_vcpu_hostcr3(v); > > /* translate l2 guest va into l2 guest gfn */ > @@ -1590,7 +1591,7 @@ unsigned long paging_gva_to_gfn(struct v > > /* translate l2 guest gfn into l1 guest gfn */ > return hostmode->p2m_ga_to_gfn(v, hostp2m, ncr3, > - gfn << PAGE_SHIFT, pfec, NULL); > + gfn << PAGE_SHIFT, &pfec_21, NULL);The caller will see the return value of pfec and not from pfec_21. If this is what the caller expects then this is fine with me. Christoph> } > > return hostmode->gva_to_gfn(v, hostp2m, va, pfec); > > Cheers, > > Tim. > >> Found with Hyper-V. >> >> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> >> CC: Tim Deegan <tim@xen.org> >> >> -- >> ---to satisfy European Law for business letters: >> Advanced Micro Devices GmbH >> Einsteinring 24, 85689 Dornach b. Muenchen >> Geschaeftsfuehrer: Alberto Bozzo >> Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen >> Registergericht Muenchen, HRB Nr. 43632 > > Content-Description: xen_p2m.diff >> diff -r 8330198c3240 xen/arch/x86/mm/p2m.c >> --- a/xen/arch/x86/mm/p2m.c Fri Jul 27 12:24:03 2012 +0200 >> +++ b/xen/arch/x86/mm/p2m.c Tue Jul 31 16:49:54 2012 +0200 >> @@ -1582,12 +1582,19 @@ unsigned long paging_gva_to_gfn(struct v >> struct p2m_domain *p2m; >> const struct paging_mode *mode; >> uint64_t ncr3 = nhvm_vcpu_hostcr3(v); >> + uint32_t pfec1 = *pfec; >> >> /* translate l2 guest va into l2 guest gfn */ >> p2m = p2m_get_nestedp2m(v, ncr3); >> mode = paging_get_nestedmode(v); >> gfn = mode->gva_to_gfn(v, p2m, va, pfec); >> >> + /* if l1 guest maps its mmio pages into the >> + * l2 guest then we see this case here. */ >> + if (gfn == INVALID_GFN) >> + return INVALID_GFN; >> + *pfec = pfec1; >> + >> /* translate l2 guest gfn into l1 guest gfn */ >> return hostmode->p2m_ga_to_gfn(v, hostp2m, ncr3, >> gfn << PAGE_SHIFT, pfec, NULL); > >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel > >-- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85689 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632
At 13:28 +0200 on 02 Aug (1343914136), Christoph Egger wrote:> On 08/02/12 13:19, Tim Deegan wrote: > > > Hi, > > > > At 17:37 +0200 on 31 Jul (1343756240), Christoph Egger wrote: > >> Do not translate INVALID_GFN as l2 guest gfn into l1 guest gfn. > > > > Why not? l2 gfns don''t have any special meaning that we can > > dictate from inside Xen. > > > >> Pass correct pfec for translation into l1 guest gfn. > > > > This seems like a good idea, but probably should happen for all > > entries, not just INVALID_GFN ones -- we shouldn''t be returning a PFEC > > to the guest that comes from translations outside his control. > > > > How about this: > > > > diff -r fdd4b7b36959 xen/arch/x86/mm/p2m.c > > --- a/xen/arch/x86/mm/p2m.c Thu Aug 02 12:04:31 2012 +0100 > > +++ b/xen/arch/x86/mm/p2m.c Thu Aug 02 12:17:48 2012 +0100 > > @@ -1581,6 +1581,7 @@ unsigned long paging_gva_to_gfn(struct v > > unsigned long gfn; > > struct p2m_domain *p2m; > > const struct paging_mode *mode; > > + uint32_t pfec_21 = *pfec; > > uint64_t ncr3 = nhvm_vcpu_hostcr3(v); > > > > /* translate l2 guest va into l2 guest gfn */ > > @@ -1590,7 +1591,7 @@ unsigned long paging_gva_to_gfn(struct v > > > > /* translate l2 guest gfn into l1 guest gfn */ > > return hostmode->p2m_ga_to_gfn(v, hostp2m, ncr3, > > - gfn << PAGE_SHIFT, pfec, NULL); > > + gfn << PAGE_SHIFT, &pfec_21, NULL); > > > The caller will see the return value of pfec and not from pfec_21. > If this is what the caller expects then this is fine with me.Yes, I think that is what the caller expects -- the error code is made up from the pagetable walk rather than from the p2m table. Can I take that as an ack? And more importantly, does it fix the Hyper-V problem you encountered? Cheers, Tim
On 08/02/12 13:35, Tim Deegan wrote:> At 13:28 +0200 on 02 Aug (1343914136), Christoph Egger wrote: >> On 08/02/12 13:19, Tim Deegan wrote: >> >>> Hi, >>> >>> At 17:37 +0200 on 31 Jul (1343756240), Christoph Egger wrote: >>>> Do not translate INVALID_GFN as l2 guest gfn into l1 guest gfn. >>> >>> Why not? l2 gfns don''t have any special meaning that we can >>> dictate from inside Xen. >>> >>>> Pass correct pfec for translation into l1 guest gfn. >>> >>> This seems like a good idea, but probably should happen for all >>> entries, not just INVALID_GFN ones -- we shouldn''t be returning a PFEC >>> to the guest that comes from translations outside his control. >>> >>> How about this: >>> >>> diff -r fdd4b7b36959 xen/arch/x86/mm/p2m.c >>> --- a/xen/arch/x86/mm/p2m.c Thu Aug 02 12:04:31 2012 +0100 >>> +++ b/xen/arch/x86/mm/p2m.c Thu Aug 02 12:17:48 2012 +0100 >>> @@ -1581,6 +1581,7 @@ unsigned long paging_gva_to_gfn(struct v >>> unsigned long gfn; >>> struct p2m_domain *p2m; >>> const struct paging_mode *mode; >>> + uint32_t pfec_21 = *pfec; >>> uint64_t ncr3 = nhvm_vcpu_hostcr3(v); >>> >>> /* translate l2 guest va into l2 guest gfn */ >>> @@ -1590,7 +1591,7 @@ unsigned long paging_gva_to_gfn(struct v >>> >>> /* translate l2 guest gfn into l1 guest gfn */ >>> return hostmode->p2m_ga_to_gfn(v, hostp2m, ncr3, >>> - gfn << PAGE_SHIFT, pfec, NULL); >>> + gfn << PAGE_SHIFT, &pfec_21, NULL); >> >> >> The caller will see the return value of pfec and not from pfec_21. >> If this is what the caller expects then this is fine with me. > > Yes, I think that is what the caller expects -- the error code is made > up from the pagetable walk rather than from the p2m table. > > Can I take that as an ack?Yes.> And more importantly, does it fix the Hyper-V problem you encountered?The one you mean is covered with the other patch. But I found this with Hyper-V when doing MMIO accesses. Christoph -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85689 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632
At 14:14 +0200 on 02 Aug (1343916891), Christoph Egger wrote:> > Yes, I think that is what the caller expects -- the error code is made > > up from the pagetable walk rather than from the p2m table. > > > > Can I take that as an ack? > > Yes.Thanks; I''ve applied it. Tim.