* Re: [xen-unstable test] 13539: regressions - FAIL
2012-08-03 7:48 ` Ian Campbell
@ 2012-08-03 7:57 ` Ian Campbell
2012-08-03 7:59 ` Jan Beulich
2012-08-03 8:34 ` Tim Deegan
2 siblings, 0 replies; 12+ messages in thread
From: Ian Campbell @ 2012-08-03 7:57 UTC (permalink / raw)
To: Ian Jackson
Cc: Keir Fraser, Christoph Egger, xen-devel@lists.xensource.com,
Tim (Xen.org), Jan Beulich
(adding Keir & Jan in case Christoph & Tim aren't around)
On Fri, 2012-08-03 at 08:48 +0100, Ian Campbell wrote:
> On Fri, 2012-08-03 at 06:12 +0100, Ian Campbell wrote:
> > On Fri, 2012-08-03 at 00:41 +0100, xen.org wrote:
> > > flight 13539 xen-unstable real [real]
> > > http://www.chiark.greenend.org.uk/~xensrcts/logs/13539/
> > >
> > > Regressions :-(
> > >
> > > Tests which did not succeed and are blocking,
> > > including tests which could not be run:
> > > build-i386-oldkern 4 xen-build fail REGR. vs. 13536
> > > build-i386 4 xen-build fail REGR. vs. 13536
>
> 8<----------------------------------
>
> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1343980045 -3600
> # Node ID 23fdca3adb3346090ea8b65b77cad7d279cf9daf
> # Parent 95a4ab632ac25ce0ec6a245dcc46ad57d3c7030f
> nestedhvm: fix nested page fault build error on 32-bit
>
> cc1: warnings being treated as errors
> hvm.c: In function ‘hvm_hap_nested_page_fault’:
> hvm.c:1282: error: passing argument 2 of ‘nestedhvm_hap_nested_page_fault’ from incompatible pointer type /local/scratch/ianc/devel/xen-unstable.hg/xen/include/asm/hvm/nestedhvm.h:55: note: expected ‘paddr_t *’ but argument is of type ‘long unsigned int *’
>
> hvm_hap_nested_page_fault takes an unsigned long gpa and passes &gpa
> to nestedhvm_hap_nested_page_fault which takes a paddr_t *. Since both
> of the callers of hvm_hap_nested_page_fault (svm_do_nested_pgfault and
> ept_handle_violation) actually have the gpa which they pass to
> hvm_hap_nested_page_fault as a paddr_t I think it makes sense to
> change the argument to hvm_hap_nested_page_fault.
>
> The other user of gpa in hvm_hap_nested_page_fault is a call to
> p2m_mem_access_check, which currently also takes a paddr_t gpa but I
> think a paddr_t is appropriate there too.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>
> diff -r 95a4ab632ac2 -r 23fdca3adb33 xen/arch/x86/hvm/hvm.c
> --- a/xen/arch/x86/hvm/hvm.c Fri Aug 03 08:43:10 2012 +0100
> +++ b/xen/arch/x86/hvm/hvm.c Fri Aug 03 08:47:25 2012 +0100
> @@ -1242,7 +1242,7 @@ void hvm_inject_page_fault(int errcode,
> hvm_inject_trap(&trap);
> }
>
> -int hvm_hap_nested_page_fault(unsigned long gpa,
> +int hvm_hap_nested_page_fault(paddr_t gpa,
> bool_t gla_valid,
> unsigned long gla,
> bool_t access_r,
> diff -r 95a4ab632ac2 -r 23fdca3adb33 xen/arch/x86/mm/p2m.c
> --- a/xen/arch/x86/mm/p2m.c Fri Aug 03 08:43:10 2012 +0100
> +++ b/xen/arch/x86/mm/p2m.c Fri Aug 03 08:47:25 2012 +0100
> @@ -1233,7 +1233,7 @@ void p2m_mem_paging_resume(struct domain
> }
> }
>
> -bool_t p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned long gla,
> +bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long gla,
> bool_t access_r, bool_t access_w, bool_t access_x,
> mem_event_request_t **req_ptr)
> {
> diff -r 95a4ab632ac2 -r 23fdca3adb33 xen/include/asm-x86/hvm/hvm.h
> --- a/xen/include/asm-x86/hvm/hvm.h Fri Aug 03 08:43:10 2012 +0100
> +++ b/xen/include/asm-x86/hvm/hvm.h Fri Aug 03 08:47:25 2012 +0100
> @@ -433,7 +433,7 @@ static inline void hvm_set_info_guest(st
>
> int hvm_debug_op(struct vcpu *v, int32_t op);
>
> -int hvm_hap_nested_page_fault(unsigned long gpa,
> +int hvm_hap_nested_page_fault(paddr_t gpa,
> bool_t gla_valid, unsigned long gla,
> bool_t access_r,
> bool_t access_w,
> diff -r 95a4ab632ac2 -r 23fdca3adb33 xen/include/asm-x86/p2m.h
> --- a/xen/include/asm-x86/p2m.h Fri Aug 03 08:43:10 2012 +0100
> +++ b/xen/include/asm-x86/p2m.h Fri Aug 03 08:47:25 2012 +0100
> @@ -589,7 +589,7 @@ static inline void p2m_mem_paging_popula
> * been promoted with no underlying vcpu pause. If the req_ptr has been populated,
> * then the caller must put the event in the ring (once having released get_gfn*
> * locks -- caller must also xfree the request. */
> -bool_t p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned long gla,
> +bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long gla,
> bool_t access_r, bool_t access_w, bool_t access_x,
> mem_event_request_t **req_ptr);
> /* Resumes the running of the VCPU, restarting the last instruction */
> @@ -606,7 +606,7 @@ int p2m_get_mem_access(struct domain *d,
> hvmmem_access_t *access);
>
> #else
> -static inline bool_t p2m_mem_access_check(unsigned long gpa, bool_t gla_valid,
> +static inline bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid,
> unsigned long gla, bool_t access_r,
> bool_t access_w, bool_t access_x,
> mem_event_request_t **req_ptr)
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [xen-unstable test] 13539: regressions - FAIL
2012-08-03 7:48 ` Ian Campbell
2012-08-03 7:57 ` Ian Campbell
@ 2012-08-03 7:59 ` Jan Beulich
2012-08-03 8:00 ` Ian Campbell
2012-08-03 9:11 ` Christoph Egger
2012-08-03 8:34 ` Tim Deegan
2 siblings, 2 replies; 12+ messages in thread
From: Jan Beulich @ 2012-08-03 7:59 UTC (permalink / raw)
To: Ian Campbell
Cc: Tim (Xen.org), Christoph Egger, xen-devel@lists.xensource.com,
Ian Jackson
>>> On 03.08.12 at 09:48, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Fri, 2012-08-03 at 06:12 +0100, Ian Campbell wrote:
>> On Fri, 2012-08-03 at 00:41 +0100, xen.org wrote:
>> > flight 13539 xen-unstable real [real]
>> > http://www.chiark.greenend.org.uk/~xensrcts/logs/13539/
>> >
>> > Regressions :-(
>> >
>> > Tests which did not succeed and are blocking,
>> > including tests which could not be run:
>> > build-i386-oldkern 4 xen-build fail REGR. vs.
> 13536
>> > build-i386 4 xen-build fail REGR. vs.
> 13536
>
> 8<----------------------------------
>
> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1343980045 -3600
> # Node ID 23fdca3adb3346090ea8b65b77cad7d279cf9daf
> # Parent 95a4ab632ac25ce0ec6a245dcc46ad57d3c7030f
> nestedhvm: fix nested page fault build error on 32-bit
>
> cc1: warnings being treated as errors
> hvm.c: In function ‘hvm_hap_nested_page_fault’:
> hvm.c:1282: error: passing argument 2 of
> ‘nestedhvm_hap_nested_page_fault’ from incompatible pointer type
> /local/scratch/ianc/devel/xen-unstable.hg/xen/include/asm/hvm/nestedhvm.h:55:
> note: expected ‘paddr_t *’ but argument is of type ‘long unsigned int *’
>
> hvm_hap_nested_page_fault takes an unsigned long gpa and passes &gpa
> to nestedhvm_hap_nested_page_fault which takes a paddr_t *. Since both
> of the callers of hvm_hap_nested_page_fault (svm_do_nested_pgfault and
> ept_handle_violation) actually have the gpa which they pass to
> hvm_hap_nested_page_fault as a paddr_t I think it makes sense to
> change the argument to hvm_hap_nested_page_fault.
And that's even outside of the current build failure - it just
can't have worked for >4Gb guests on the 32-bit hypervisor.
> The other user of gpa in hvm_hap_nested_page_fault is a call to
> p2m_mem_access_check, which currently also takes a paddr_t gpa but I
> think a paddr_t is appropriate there too.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
> diff -r 95a4ab632ac2 -r 23fdca3adb33 xen/arch/x86/hvm/hvm.c
> --- a/xen/arch/x86/hvm/hvm.c Fri Aug 03 08:43:10 2012 +0100
> +++ b/xen/arch/x86/hvm/hvm.c Fri Aug 03 08:47:25 2012 +0100
> @@ -1242,7 +1242,7 @@ void hvm_inject_page_fault(int errcode,
> hvm_inject_trap(&trap);
> }
>
> -int hvm_hap_nested_page_fault(unsigned long gpa,
> +int hvm_hap_nested_page_fault(paddr_t gpa,
> bool_t gla_valid,
> unsigned long gla,
> bool_t access_r,
> diff -r 95a4ab632ac2 -r 23fdca3adb33 xen/arch/x86/mm/p2m.c
> --- a/xen/arch/x86/mm/p2m.c Fri Aug 03 08:43:10 2012 +0100
> +++ b/xen/arch/x86/mm/p2m.c Fri Aug 03 08:47:25 2012 +0100
> @@ -1233,7 +1233,7 @@ void p2m_mem_paging_resume(struct domain
> }
> }
>
> -bool_t p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned
> long gla,
> +bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long
> gla,
> bool_t access_r, bool_t access_w, bool_t
> access_x,
> mem_event_request_t **req_ptr)
> {
> diff -r 95a4ab632ac2 -r 23fdca3adb33 xen/include/asm-x86/hvm/hvm.h
> --- a/xen/include/asm-x86/hvm/hvm.h Fri Aug 03 08:43:10 2012 +0100
> +++ b/xen/include/asm-x86/hvm/hvm.h Fri Aug 03 08:47:25 2012 +0100
> @@ -433,7 +433,7 @@ static inline void hvm_set_info_guest(st
>
> int hvm_debug_op(struct vcpu *v, int32_t op);
>
> -int hvm_hap_nested_page_fault(unsigned long gpa,
> +int hvm_hap_nested_page_fault(paddr_t gpa,
> bool_t gla_valid, unsigned long gla,
> bool_t access_r,
> bool_t access_w,
> diff -r 95a4ab632ac2 -r 23fdca3adb33 xen/include/asm-x86/p2m.h
> --- a/xen/include/asm-x86/p2m.h Fri Aug 03 08:43:10 2012 +0100
> +++ b/xen/include/asm-x86/p2m.h Fri Aug 03 08:47:25 2012 +0100
> @@ -589,7 +589,7 @@ static inline void p2m_mem_paging_popula
> * been promoted with no underlying vcpu pause. If the req_ptr has been
> populated,
> * then the caller must put the event in the ring (once having released
> get_gfn*
> * locks -- caller must also xfree the request. */
> -bool_t p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned
> long gla,
> +bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long
> gla,
> bool_t access_r, bool_t access_w, bool_t
> access_x,
> mem_event_request_t **req_ptr);
> /* Resumes the running of the VCPU, restarting the last instruction */
> @@ -606,7 +606,7 @@ int p2m_get_mem_access(struct domain *d,
> hvmmem_access_t *access);
>
> #else
> -static inline bool_t p2m_mem_access_check(unsigned long gpa, bool_t
> gla_valid,
> +static inline bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid,
> unsigned long gla, bool_t access_r,
>
> bool_t access_w, bool_t access_x,
> mem_event_request_t **req_ptr)
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [xen-unstable test] 13539: regressions - FAIL
2012-08-03 7:59 ` Jan Beulich
@ 2012-08-03 8:00 ` Ian Campbell
2012-08-03 8:44 ` Jan Beulich
2012-08-03 9:11 ` Christoph Egger
1 sibling, 1 reply; 12+ messages in thread
From: Ian Campbell @ 2012-08-03 8:00 UTC (permalink / raw)
To: Jan Beulich
Cc: Tim (Xen.org), Christoph Egger, xen-devel@lists.xensource.com,
Ian Jackson
On Fri, 2012-08-03 at 08:59 +0100, Jan Beulich wrote:
> >>> On 03.08.12 at 09:48, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Fri, 2012-08-03 at 06:12 +0100, Ian Campbell wrote:
> >> On Fri, 2012-08-03 at 00:41 +0100, xen.org wrote:
> >> > flight 13539 xen-unstable real [real]
> >> > http://www.chiark.greenend.org.uk/~xensrcts/logs/13539/
> >> >
> >> > Regressions :-(
> >> >
> >> > Tests which did not succeed and are blocking,
> >> > including tests which could not be run:
> >> > build-i386-oldkern 4 xen-build fail REGR. vs.
> > 13536
> >> > build-i386 4 xen-build fail REGR. vs.
> > 13536
> >
> > 8<----------------------------------
> >
> > # HG changeset patch
> > # User Ian Campbell <ian.campbell@citrix.com>
> > # Date 1343980045 -3600
> > # Node ID 23fdca3adb3346090ea8b65b77cad7d279cf9daf
> > # Parent 95a4ab632ac25ce0ec6a245dcc46ad57d3c7030f
> > nestedhvm: fix nested page fault build error on 32-bit
> >
> > cc1: warnings being treated as errors
> > hvm.c: In function ‘hvm_hap_nested_page_fault’:
> > hvm.c:1282: error: passing argument 2 of
> > ‘nestedhvm_hap_nested_page_fault’ from incompatible pointer type
> > /local/scratch/ianc/devel/xen-unstable.hg/xen/include/asm/hvm/nestedhvm.h:55:
> > note: expected ‘paddr_t *’ but argument is of type ‘long unsigned int *’
> >
> > hvm_hap_nested_page_fault takes an unsigned long gpa and passes &gpa
> > to nestedhvm_hap_nested_page_fault which takes a paddr_t *. Since both
> > of the callers of hvm_hap_nested_page_fault (svm_do_nested_pgfault and
> > ept_handle_violation) actually have the gpa which they pass to
> > hvm_hap_nested_page_fault as a paddr_t I think it makes sense to
> > change the argument to hvm_hap_nested_page_fault.
>
> And that's even outside of the current build failure - it just
> can't have worked for >4Gb guests on the 32-bit hypervisor.
Right. I must admit I was surprised to find that nestedhvm was a feature
of 32 bit at all, I had expect the fix to be changing some sort of stub
function...
>
> > The other user of gpa in hvm_hap_nested_page_fault is a call to
> > p2m_mem_access_check, which currently also takes a paddr_t gpa but I
> > think a paddr_t is appropriate there too.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> > diff -r 95a4ab632ac2 -r 23fdca3adb33 xen/arch/x86/hvm/hvm.c
> > --- a/xen/arch/x86/hvm/hvm.c Fri Aug 03 08:43:10 2012 +0100
> > +++ b/xen/arch/x86/hvm/hvm.c Fri Aug 03 08:47:25 2012 +0100
> > @@ -1242,7 +1242,7 @@ void hvm_inject_page_fault(int errcode,
> > hvm_inject_trap(&trap);
> > }
> >
> > -int hvm_hap_nested_page_fault(unsigned long gpa,
> > +int hvm_hap_nested_page_fault(paddr_t gpa,
> > bool_t gla_valid,
> > unsigned long gla,
> > bool_t access_r,
> > diff -r 95a4ab632ac2 -r 23fdca3adb33 xen/arch/x86/mm/p2m.c
> > --- a/xen/arch/x86/mm/p2m.c Fri Aug 03 08:43:10 2012 +0100
> > +++ b/xen/arch/x86/mm/p2m.c Fri Aug 03 08:47:25 2012 +0100
> > @@ -1233,7 +1233,7 @@ void p2m_mem_paging_resume(struct domain
> > }
> > }
> >
> > -bool_t p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned
> > long gla,
> > +bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long
> > gla,
> > bool_t access_r, bool_t access_w, bool_t
> > access_x,
> > mem_event_request_t **req_ptr)
> > {
> > diff -r 95a4ab632ac2 -r 23fdca3adb33 xen/include/asm-x86/hvm/hvm.h
> > --- a/xen/include/asm-x86/hvm/hvm.h Fri Aug 03 08:43:10 2012 +0100
> > +++ b/xen/include/asm-x86/hvm/hvm.h Fri Aug 03 08:47:25 2012 +0100
> > @@ -433,7 +433,7 @@ static inline void hvm_set_info_guest(st
> >
> > int hvm_debug_op(struct vcpu *v, int32_t op);
> >
> > -int hvm_hap_nested_page_fault(unsigned long gpa,
> > +int hvm_hap_nested_page_fault(paddr_t gpa,
> > bool_t gla_valid, unsigned long gla,
> > bool_t access_r,
> > bool_t access_w,
> > diff -r 95a4ab632ac2 -r 23fdca3adb33 xen/include/asm-x86/p2m.h
> > --- a/xen/include/asm-x86/p2m.h Fri Aug 03 08:43:10 2012 +0100
> > +++ b/xen/include/asm-x86/p2m.h Fri Aug 03 08:47:25 2012 +0100
> > @@ -589,7 +589,7 @@ static inline void p2m_mem_paging_popula
> > * been promoted with no underlying vcpu pause. If the req_ptr has been
> > populated,
> > * then the caller must put the event in the ring (once having released
> > get_gfn*
> > * locks -- caller must also xfree the request. */
> > -bool_t p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned
> > long gla,
> > +bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long
> > gla,
> > bool_t access_r, bool_t access_w, bool_t
> > access_x,
> > mem_event_request_t **req_ptr);
> > /* Resumes the running of the VCPU, restarting the last instruction */
> > @@ -606,7 +606,7 @@ int p2m_get_mem_access(struct domain *d,
> > hvmmem_access_t *access);
> >
> > #else
> > -static inline bool_t p2m_mem_access_check(unsigned long gpa, bool_t
> > gla_valid,
> > +static inline bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid,
> > unsigned long gla, bool_t access_r,
> >
> > bool_t access_w, bool_t access_x,
> > mem_event_request_t **req_ptr)
> >
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [xen-unstable test] 13539: regressions - FAIL
2012-08-03 8:00 ` Ian Campbell
@ 2012-08-03 8:44 ` Jan Beulich
0 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2012-08-03 8:44 UTC (permalink / raw)
To: Ian Campbell
Cc: Tim (Xen.org), Christoph Egger, xen-devel@lists.xensource.com,
Ian Jackson
>>> On 03.08.12 at 10:00, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Fri, 2012-08-03 at 08:59 +0100, Jan Beulich wrote:
>> >>> On 03.08.12 at 09:48, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Fri, 2012-08-03 at 06:12 +0100, Ian Campbell wrote:
>> >> On Fri, 2012-08-03 at 00:41 +0100, xen.org wrote:
>> >> > flight 13539 xen-unstable real [real]
>> >> > http://www.chiark.greenend.org.uk/~xensrcts/logs/13539/
>> >> >
>> >> > Regressions :-(
>> >> >
>> >> > Tests which did not succeed and are blocking,
>> >> > including tests which could not be run:
>> >> > build-i386-oldkern 4 xen-build fail REGR. vs.
>> > 13536
>> >> > build-i386 4 xen-build fail REGR. vs.
>> > 13536
>> >
>> > 8<----------------------------------
>> >
>> > # HG changeset patch
>> > # User Ian Campbell <ian.campbell@citrix.com>
>> > # Date 1343980045 -3600
>> > # Node ID 23fdca3adb3346090ea8b65b77cad7d279cf9daf
>> > # Parent 95a4ab632ac25ce0ec6a245dcc46ad57d3c7030f
>> > nestedhvm: fix nested page fault build error on 32-bit
>> >
>> > cc1: warnings being treated as errors
>> > hvm.c: In function ‘hvm_hap_nested_page_fault’:
>> > hvm.c:1282: error: passing argument 2 of
>> > ‘nestedhvm_hap_nested_page_fault’ from incompatible pointer type
>> >
> /local/scratch/ianc/devel/xen-unstable.hg/xen/include/asm/hvm/nestedhvm.h:55:
>
>> > note: expected ‘paddr_t *’ but argument is of type ‘long unsigned int *’
>> >
>> > hvm_hap_nested_page_fault takes an unsigned long gpa and passes &gpa
>> > to nestedhvm_hap_nested_page_fault which takes a paddr_t *. Since both
>> > of the callers of hvm_hap_nested_page_fault (svm_do_nested_pgfault and
>> > ept_handle_violation) actually have the gpa which they pass to
>> > hvm_hap_nested_page_fault as a paddr_t I think it makes sense to
>> > change the argument to hvm_hap_nested_page_fault.
>>
>> And that's even outside of the current build failure - it just
>> can't have worked for >4Gb guests on the 32-bit hypervisor.
>
> Right. I must admit I was surprised to find that nestedhvm was a feature
> of 32 bit at all, I had expect the fix to be changing some sort of stub
> function...
So did I first think. But the function that needed changing really
isn't nested-HVM only, so the fix was required in any case (just
that without the exposing c/s, the problem would likely have been
found a lot later).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [xen-unstable test] 13539: regressions - FAIL
2012-08-03 7:59 ` Jan Beulich
2012-08-03 8:00 ` Ian Campbell
@ 2012-08-03 9:11 ` Christoph Egger
1 sibling, 0 replies; 12+ messages in thread
From: Christoph Egger @ 2012-08-03 9:11 UTC (permalink / raw)
To: Jan Beulich
Cc: Tim (Xen.org), xen-devel@lists.xensource.com, Ian Jackson,
Ian Campbell
On 08/03/12 09:59, Jan Beulich wrote:
>>>> On 03.08.12 at 09:48, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> On Fri, 2012-08-03 at 06:12 +0100, Ian Campbell wrote:
>>> On Fri, 2012-08-03 at 00:41 +0100, xen.org wrote:
>>>> flight 13539 xen-unstable real [real]
>>>> http://www.chiark.greenend.org.uk/~xensrcts/logs/13539/
>>>>
>>>> Regressions :-(
>>>>
>>>> Tests which did not succeed and are blocking,
>>>> including tests which could not be run:
>>>> build-i386-oldkern 4 xen-build fail REGR. vs.
>> 13536
>>>> build-i386 4 xen-build fail REGR. vs.
>> 13536
>>
>> 8<----------------------------------
>>
>> # HG changeset patch
>> # User Ian Campbell <ian.campbell@citrix.com>
>> # Date 1343980045 -3600
>> # Node ID 23fdca3adb3346090ea8b65b77cad7d279cf9daf
>> # Parent 95a4ab632ac25ce0ec6a245dcc46ad57d3c7030f
>> nestedhvm: fix nested page fault build error on 32-bit
>>
>> cc1: warnings being treated as errors
>> hvm.c: In function ‘hvm_hap_nested_page_fault’:
>> hvm.c:1282: error: passing argument 2 of
>> ‘nestedhvm_hap_nested_page_fault’ from incompatible pointer type
>> /local/scratch/ianc/devel/xen-unstable.hg/xen/include/asm/hvm/nestedhvm.h:55:
>> note: expected ‘paddr_t *’ but argument is of type ‘long unsigned int *’
>>
>> hvm_hap_nested_page_fault takes an unsigned long gpa and passes &gpa
>> to nestedhvm_hap_nested_page_fault which takes a paddr_t *. Since both
>> of the callers of hvm_hap_nested_page_fault (svm_do_nested_pgfault and
>> ept_handle_violation) actually have the gpa which they pass to
>> hvm_hap_nested_page_fault as a paddr_t I think it makes sense to
>> change the argument to hvm_hap_nested_page_fault.
>
> And that's even outside of the current build failure - it just
> can't have worked for >4Gb guests on the 32-bit hypervisor.
>
>> The other user of gpa in hvm_hap_nested_page_fault is a call to
>> p2m_mem_access_check, which currently also takes a paddr_t gpa but I
>> think a paddr_t is appropriate there too.
>>
>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>
> Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Christoph Egger <Christoph.Egger@amd.com>
>
>> diff -r 95a4ab632ac2 -r 23fdca3adb33 xen/arch/x86/hvm/hvm.c
>> --- a/xen/arch/x86/hvm/hvm.c Fri Aug 03 08:43:10 2012 +0100
>> +++ b/xen/arch/x86/hvm/hvm.c Fri Aug 03 08:47:25 2012 +0100
>> @@ -1242,7 +1242,7 @@ void hvm_inject_page_fault(int errcode,
>> hvm_inject_trap(&trap);
>> }
>>
>> -int hvm_hap_nested_page_fault(unsigned long gpa,
>> +int hvm_hap_nested_page_fault(paddr_t gpa,
>> bool_t gla_valid,
>> unsigned long gla,
>> bool_t access_r,
>> diff -r 95a4ab632ac2 -r 23fdca3adb33 xen/arch/x86/mm/p2m.c
>> --- a/xen/arch/x86/mm/p2m.c Fri Aug 03 08:43:10 2012 +0100
>> +++ b/xen/arch/x86/mm/p2m.c Fri Aug 03 08:47:25 2012 +0100
>> @@ -1233,7 +1233,7 @@ void p2m_mem_paging_resume(struct domain
>> }
>> }
>>
>> -bool_t p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned
>> long gla,
>> +bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long
>> gla,
>> bool_t access_r, bool_t access_w, bool_t
>> access_x,
>> mem_event_request_t **req_ptr)
>> {
>> diff -r 95a4ab632ac2 -r 23fdca3adb33 xen/include/asm-x86/hvm/hvm.h
>> --- a/xen/include/asm-x86/hvm/hvm.h Fri Aug 03 08:43:10 2012 +0100
>> +++ b/xen/include/asm-x86/hvm/hvm.h Fri Aug 03 08:47:25 2012 +0100
>> @@ -433,7 +433,7 @@ static inline void hvm_set_info_guest(st
>>
>> int hvm_debug_op(struct vcpu *v, int32_t op);
>>
>> -int hvm_hap_nested_page_fault(unsigned long gpa,
>> +int hvm_hap_nested_page_fault(paddr_t gpa,
>> bool_t gla_valid, unsigned long gla,
>> bool_t access_r,
>> bool_t access_w,
>> diff -r 95a4ab632ac2 -r 23fdca3adb33 xen/include/asm-x86/p2m.h
>> --- a/xen/include/asm-x86/p2m.h Fri Aug 03 08:43:10 2012 +0100
>> +++ b/xen/include/asm-x86/p2m.h Fri Aug 03 08:47:25 2012 +0100
>> @@ -589,7 +589,7 @@ static inline void p2m_mem_paging_popula
>> * been promoted with no underlying vcpu pause. If the req_ptr has been
>> populated,
>> * then the caller must put the event in the ring (once having released
>> get_gfn*
>> * locks -- caller must also xfree the request. */
>> -bool_t p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned
>> long gla,
>> +bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long
>> gla,
>> bool_t access_r, bool_t access_w, bool_t
>> access_x,
>> mem_event_request_t **req_ptr);
>> /* Resumes the running of the VCPU, restarting the last instruction */
>> @@ -606,7 +606,7 @@ int p2m_get_mem_access(struct domain *d,
>> hvmmem_access_t *access);
>>
>> #else
>> -static inline bool_t p2m_mem_access_check(unsigned long gpa, bool_t
>> gla_valid,
>> +static inline bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid,
>> unsigned long gla, bool_t access_r,
>>
>> bool_t access_w, bool_t access_x,
>> mem_event_request_t **req_ptr)
>>
>>
>>
>> _______________________________________________
>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [xen-unstable test] 13539: regressions - FAIL
2012-08-03 7:48 ` Ian Campbell
2012-08-03 7:57 ` Ian Campbell
2012-08-03 7:59 ` Jan Beulich
@ 2012-08-03 8:34 ` Tim Deegan
2012-08-03 8:37 ` Ian Campbell
2 siblings, 1 reply; 12+ messages in thread
From: Tim Deegan @ 2012-08-03 8:34 UTC (permalink / raw)
To: Ian Campbell
Cc: keir, Christoph Egger, xen-devel@lists.xensource.com, Ian Jackson
At 08:48 +0100 on 03 Aug (1343983712), Ian Campbell wrote:
> nestedhvm: fix nested page fault build error on 32-bit
>
> cc1: warnings being treated as errors
> hvm.c: In function ???hvm_hap_nested_page_fault???:
> hvm.c:1282: error: passing argument 2 of ???nestedhvm_hap_nested_page_fault??? from incompatible pointer type /local/scratch/ianc/devel/xen-unstable.hg/xen/include/asm/hvm/nestedhvm.h:55: note: expected ???paddr_t *??? but argument is of type ???long unsigned int *???
>
> hvm_hap_nested_page_fault takes an unsigned long gpa and passes &gpa
> to nestedhvm_hap_nested_page_fault which takes a paddr_t *. Since both
> of the callers of hvm_hap_nested_page_fault (svm_do_nested_pgfault and
> ept_handle_violation) actually have the gpa which they pass to
> hvm_hap_nested_page_fault as a paddr_t I think it makes sense to
> change the argument to hvm_hap_nested_page_fault.
>
> The other user of gpa in hvm_hap_nested_page_fault is a call to
> p2m_mem_access_check, which currently also takes a paddr_t gpa but I
> think a paddr_t is appropriate there too.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>
I think this is a candidate for backporting, too. As Jan points out,
this is a HAP bug on 32-bit with >4G guests.
Tim.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [xen-unstable test] 13539: regressions - FAIL
2012-08-03 8:34 ` Tim Deegan
@ 2012-08-03 8:37 ` Ian Campbell
2012-08-03 8:39 ` Tim Deegan
0 siblings, 1 reply; 12+ messages in thread
From: Ian Campbell @ 2012-08-03 8:37 UTC (permalink / raw)
To: Tim Deegan
Cc: Keir (Xen.org), Christoph Egger, xen-devel@lists.xensource.com,
Ian Jackson
On Fri, 2012-08-03 at 09:34 +0100, Tim Deegan wrote:
> At 08:48 +0100 on 03 Aug (1343983712), Ian Campbell wrote:
> > nestedhvm: fix nested page fault build error on 32-bit
> >
> > cc1: warnings being treated as errors
> > hvm.c: In function ???hvm_hap_nested_page_fault???:
> > hvm.c:1282: error: passing argument 2 of ???nestedhvm_hap_nested_page_fault??? from incompatible pointer type /local/scratch/ianc/devel/xen-unstable.hg/xen/include/asm/hvm/nestedhvm.h:55: note: expected ???paddr_t *??? but argument is of type ???long unsigned int *???
> >
> > hvm_hap_nested_page_fault takes an unsigned long gpa and passes &gpa
> > to nestedhvm_hap_nested_page_fault which takes a paddr_t *. Since both
> > of the callers of hvm_hap_nested_page_fault (svm_do_nested_pgfault and
> > ept_handle_violation) actually have the gpa which they pass to
> > hvm_hap_nested_page_fault as a paddr_t I think it makes sense to
> > change the argument to hvm_hap_nested_page_fault.
> >
> > The other user of gpa in hvm_hap_nested_page_fault is a call to
> > p2m_mem_access_check, which currently also takes a paddr_t gpa but I
> > think a paddr_t is appropriate there too.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>
> Acked-by: Tim Deegan <tim@xen.org>
Is one of you or Jan going to apply or shall I? (I'm doing a tools
commit sweep right now)
> I think this is a candidate for backporting, too. As Jan points out,
> this is a HAP bug on 32-bit with >4G guests.
>
> Tim.
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [xen-unstable test] 13539: regressions - FAIL
2012-08-03 8:37 ` Ian Campbell
@ 2012-08-03 8:39 ` Tim Deegan
2012-08-03 8:55 ` Ian Campbell
0 siblings, 1 reply; 12+ messages in thread
From: Tim Deegan @ 2012-08-03 8:39 UTC (permalink / raw)
To: Ian Campbell
Cc: Keir (Xen.org), Christoph Egger, xen-devel@lists.xensource.com,
Ian Jackson
At 09:37 +0100 on 03 Aug (1343986678), Ian Campbell wrote:
> On Fri, 2012-08-03 at 09:34 +0100, Tim Deegan wrote:
> > At 08:48 +0100 on 03 Aug (1343983712), Ian Campbell wrote:
> > > nestedhvm: fix nested page fault build error on 32-bit
> > >
> > > cc1: warnings being treated as errors
> > > hvm.c: In function ???hvm_hap_nested_page_fault???:
> > > hvm.c:1282: error: passing argument 2 of ???nestedhvm_hap_nested_page_fault??? from incompatible pointer type /local/scratch/ianc/devel/xen-unstable.hg/xen/include/asm/hvm/nestedhvm.h:55: note: expected ???paddr_t *??? but argument is of type ???long unsigned int *???
> > >
> > > hvm_hap_nested_page_fault takes an unsigned long gpa and passes &gpa
> > > to nestedhvm_hap_nested_page_fault which takes a paddr_t *. Since both
> > > of the callers of hvm_hap_nested_page_fault (svm_do_nested_pgfault and
> > > ept_handle_violation) actually have the gpa which they pass to
> > > hvm_hap_nested_page_fault as a paddr_t I think it makes sense to
> > > change the argument to hvm_hap_nested_page_fault.
> > >
> > > The other user of gpa in hvm_hap_nested_page_fault is a call to
> > > p2m_mem_access_check, which currently also takes a paddr_t gpa but I
> > > think a paddr_t is appropriate there too.
> > >
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >
> > Acked-by: Tim Deegan <tim@xen.org>
>
> Is one of you or Jan going to apply or shall I? (I'm doing a tools
> commit sweep right now)
If you're already applying things, please do apply up this one too.
Tim.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [xen-unstable test] 13539: regressions - FAIL
2012-08-03 8:39 ` Tim Deegan
@ 2012-08-03 8:55 ` Ian Campbell
0 siblings, 0 replies; 12+ messages in thread
From: Ian Campbell @ 2012-08-03 8:55 UTC (permalink / raw)
To: Tim Deegan
Cc: Keir (Xen.org), Christoph Egger, xen-devel@lists.xensource.com,
Ian Jackson
On Fri, 2012-08-03 at 09:39 +0100, Tim Deegan wrote:
> At 09:37 +0100 on 03 Aug (1343986678), Ian Campbell wrote:
> > On Fri, 2012-08-03 at 09:34 +0100, Tim Deegan wrote:
> > > At 08:48 +0100 on 03 Aug (1343983712), Ian Campbell wrote:
> > > > nestedhvm: fix nested page fault build error on 32-bit
> > > >
> > > > cc1: warnings being treated as errors
> > > > hvm.c: In function ???hvm_hap_nested_page_fault???:
> > > > hvm.c:1282: error: passing argument 2 of ???nestedhvm_hap_nested_page_fault??? from incompatible pointer type /local/scratch/ianc/devel/xen-unstable.hg/xen/include/asm/hvm/nestedhvm.h:55: note: expected ???paddr_t *??? but argument is of type ???long unsigned int *???
> > > >
> > > > hvm_hap_nested_page_fault takes an unsigned long gpa and passes &gpa
> > > > to nestedhvm_hap_nested_page_fault which takes a paddr_t *. Since both
> > > > of the callers of hvm_hap_nested_page_fault (svm_do_nested_pgfault and
> > > > ept_handle_violation) actually have the gpa which they pass to
> > > > hvm_hap_nested_page_fault as a paddr_t I think it makes sense to
> > > > change the argument to hvm_hap_nested_page_fault.
> > > >
> > > > The other user of gpa in hvm_hap_nested_page_fault is a call to
> > > > p2m_mem_access_check, which currently also takes a paddr_t gpa but I
> > > > think a paddr_t is appropriate there too.
> > > >
> > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > >
> > > Acked-by: Tim Deegan <tim@xen.org>
> >
> > Is one of you or Jan going to apply or shall I? (I'm doing a tools
> > commit sweep right now)
>
> If you're already applying things, please do apply up this one too.
Done.
I added an extra sentence to the commit log:
Jan points out that this is also an issue for >4GB guests on the
32
bit hypervisor.
>
> Tim.
^ permalink raw reply [flat|nested] 12+ messages in thread