* [Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size @ 2015-11-10 0:23 Andrew Jones 2015-11-10 15:41 ` Paolo Bonzini 2015-11-10 16:29 ` Peter Maydell 0 siblings, 2 replies; 7+ messages in thread From: Andrew Jones @ 2015-11-10 0:23 UTC (permalink / raw) To: qemu-devel, kvm; +Cc: pbonzini Just noticed this while grepping TARGET_PAGE_SIZE for an unrelated reason. I didn't use qemu_real_host_page_size as kvm_set_phys_mem() does, because we'd need to make sure page_size_init() has run first. Signed-off-by: Andrew Jones <drjones@redhat.com> --- kvm-all.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/kvm-all.c b/kvm-all.c index 1bc12737723c3..de9ff5971fb3b 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -45,8 +45,10 @@ #include <sys/eventfd.h> #endif -/* KVM uses PAGE_SIZE in its definition of COALESCED_MMIO_MAX */ -#define PAGE_SIZE TARGET_PAGE_SIZE +/* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We + * need to use the real host PAGE_SIZE, as that's what KVM will use. + */ +#define PAGE_SIZE getpagesize() //#define DEBUG_KVM -- 2.4.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size 2015-11-10 0:23 [Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size Andrew Jones @ 2015-11-10 15:41 ` Paolo Bonzini 2015-11-10 15:57 ` Andrew Jones 2015-11-10 16:29 ` Peter Maydell 1 sibling, 1 reply; 7+ messages in thread From: Paolo Bonzini @ 2015-11-10 15:41 UTC (permalink / raw) To: Andrew Jones, qemu-devel, kvm On 10/11/2015 01:23, Andrew Jones wrote: > Just noticed this while grepping TARGET_PAGE_SIZE for an unrelated > reason. I didn't use qemu_real_host_page_size as kvm_set_phys_mem() > does, because we'd need to make sure page_size_init() has run first. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > kvm-all.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/kvm-all.c b/kvm-all.c > index 1bc12737723c3..de9ff5971fb3b 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -45,8 +45,10 @@ > #include <sys/eventfd.h> > #endif > > -/* KVM uses PAGE_SIZE in its definition of COALESCED_MMIO_MAX */ > -#define PAGE_SIZE TARGET_PAGE_SIZE > +/* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We > + * need to use the real host PAGE_SIZE, as that's what KVM will use. > + */ > +#define PAGE_SIZE getpagesize() > > //#define DEBUG_KVM > > Is this a bugfix or just a cleanup? If the former, on which targets? Paolo ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size 2015-11-10 15:41 ` Paolo Bonzini @ 2015-11-10 15:57 ` Andrew Jones 2015-11-10 16:15 ` Paolo Bonzini 0 siblings, 1 reply; 7+ messages in thread From: Andrew Jones @ 2015-11-10 15:57 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, kvm On Tue, Nov 10, 2015 at 04:41:16PM +0100, Paolo Bonzini wrote: > > > On 10/11/2015 01:23, Andrew Jones wrote: > > Just noticed this while grepping TARGET_PAGE_SIZE for an unrelated > > reason. I didn't use qemu_real_host_page_size as kvm_set_phys_mem() > > does, because we'd need to make sure page_size_init() has run first. > > > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > kvm-all.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/kvm-all.c b/kvm-all.c > > index 1bc12737723c3..de9ff5971fb3b 100644 > > --- a/kvm-all.c > > +++ b/kvm-all.c > > @@ -45,8 +45,10 @@ > > #include <sys/eventfd.h> > > #endif > > > > -/* KVM uses PAGE_SIZE in its definition of COALESCED_MMIO_MAX */ > > -#define PAGE_SIZE TARGET_PAGE_SIZE > > +/* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We > > + * need to use the real host PAGE_SIZE, as that's what KVM will use. > > + */ > > +#define PAGE_SIZE getpagesize() > > > > //#define DEBUG_KVM > > > > > > Is this a bugfix or just a cleanup? If the former, on which targets? It's a bugfix for any targets that have a TARGET_PAGE_SIZE != real-host-page-size. For example ARM has TARGET_PAGE_SIZE set to 1024, even when the host is using 4k or 64k pages. However, I didn't find this due to a bug, because on ARM I'm not using emulated devices that make use of the coalesced-mmio feature at this time. Thanks, drew > > Paolo > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size 2015-11-10 15:57 ` Andrew Jones @ 2015-11-10 16:15 ` Paolo Bonzini 0 siblings, 0 replies; 7+ messages in thread From: Paolo Bonzini @ 2015-11-10 16:15 UTC (permalink / raw) To: Andrew Jones; +Cc: qemu-devel, kvm On 10/11/2015 16:57, Andrew Jones wrote: > On Tue, Nov 10, 2015 at 04:41:16PM +0100, Paolo Bonzini wrote: >> >> >> On 10/11/2015 01:23, Andrew Jones wrote: >>> Just noticed this while grepping TARGET_PAGE_SIZE for an unrelated >>> reason. I didn't use qemu_real_host_page_size as kvm_set_phys_mem() >>> does, because we'd need to make sure page_size_init() has run first. >>> >>> Signed-off-by: Andrew Jones <drjones@redhat.com> >>> --- >>> kvm-all.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/kvm-all.c b/kvm-all.c >>> index 1bc12737723c3..de9ff5971fb3b 100644 >>> --- a/kvm-all.c >>> +++ b/kvm-all.c >>> @@ -45,8 +45,10 @@ >>> #include <sys/eventfd.h> >>> #endif >>> >>> -/* KVM uses PAGE_SIZE in its definition of COALESCED_MMIO_MAX */ >>> -#define PAGE_SIZE TARGET_PAGE_SIZE >>> +/* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We >>> + * need to use the real host PAGE_SIZE, as that's what KVM will use. >>> + */ >>> +#define PAGE_SIZE getpagesize() >>> >>> //#define DEBUG_KVM >>> >>> >> >> Is this a bugfix or just a cleanup? If the former, on which targets? > > It's a bugfix for any targets that have a TARGET_PAGE_SIZE != > real-host-page-size. For example ARM has TARGET_PAGE_SIZE set to 1024, > even when the host is using 4k or 64k pages. However, I didn't find this > due to a bug, because on ARM I'm not using emulated devices that make > use of the coalesced-mmio feature at this time. > > Thanks, > drew > >> >> Paolo >> -- >> To unsubscribe from this list: send the line "unsubscribe kvm" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html Ok, applied (2.6 only unless I gather more urgent patches). Paolo ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size 2015-11-10 0:23 [Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size Andrew Jones 2015-11-10 15:41 ` Paolo Bonzini @ 2015-11-10 16:29 ` Peter Maydell 2015-11-10 16:59 ` Andrew Jones 1 sibling, 1 reply; 7+ messages in thread From: Peter Maydell @ 2015-11-10 16:29 UTC (permalink / raw) To: Andrew Jones; +Cc: Paolo Bonzini, QEMU Developers, kvm-devel On 10 November 2015 at 00:23, Andrew Jones <drjones@redhat.com> wrote: > Just noticed this while grepping TARGET_PAGE_SIZE for an unrelated > reason. I didn't use qemu_real_host_page_size as kvm_set_phys_mem() > does, because we'd need to make sure page_size_init() has run first. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > kvm-all.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/kvm-all.c b/kvm-all.c > index 1bc12737723c3..de9ff5971fb3b 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -45,8 +45,10 @@ > #include <sys/eventfd.h> > #endif > > -/* KVM uses PAGE_SIZE in its definition of COALESCED_MMIO_MAX */ > -#define PAGE_SIZE TARGET_PAGE_SIZE > +/* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We > + * need to use the real host PAGE_SIZE, as that's what KVM will use. > + */ > +#define PAGE_SIZE getpagesize() Rather than defining PAGE_SIZE here (a confusing macro given we have several page sizes to deal with), why not just use getpagesize() in the one and only location where we currently use this macro? Also, you're guaranteed that page_size_init() has been run, because we call that from kvm_init(), and you can't call kvm_vcpu_init() before kvm_init(). thanks -- PMM ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size 2015-11-10 16:29 ` Peter Maydell @ 2015-11-10 16:59 ` Andrew Jones 2015-11-10 17:02 ` Peter Maydell 0 siblings, 1 reply; 7+ messages in thread From: Andrew Jones @ 2015-11-10 16:59 UTC (permalink / raw) To: Peter Maydell; +Cc: Paolo Bonzini, QEMU Developers, kvm-devel On Tue, Nov 10, 2015 at 04:29:31PM +0000, Peter Maydell wrote: > On 10 November 2015 at 00:23, Andrew Jones <drjones@redhat.com> wrote: > > Just noticed this while grepping TARGET_PAGE_SIZE for an unrelated > > reason. I didn't use qemu_real_host_page_size as kvm_set_phys_mem() > > does, because we'd need to make sure page_size_init() has run first. > > > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > kvm-all.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/kvm-all.c b/kvm-all.c > > index 1bc12737723c3..de9ff5971fb3b 100644 > > --- a/kvm-all.c > > +++ b/kvm-all.c > > @@ -45,8 +45,10 @@ > > #include <sys/eventfd.h> > > #endif > > > > -/* KVM uses PAGE_SIZE in its definition of COALESCED_MMIO_MAX */ > > -#define PAGE_SIZE TARGET_PAGE_SIZE > > +/* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We > > + * need to use the real host PAGE_SIZE, as that's what KVM will use. > > + */ > > +#define PAGE_SIZE getpagesize() > > Rather than defining PAGE_SIZE here (a confusing macro given > we have several page sizes to deal with), why not just use > getpagesize() in the one and only location where we currently > use this macro? The macro is used by kernel headers that we import and include in kvm-all.c. It's ugly, I agree, but that's how the this cookie crumbled. > > Also, you're guaranteed that page_size_init() has been run, because > we call that from kvm_init(), and you can't call kvm_vcpu_init() > before kvm_init(). True, but having that dependency seemed error prone to me. If we we some day changed when/if page_size_init is called then there could be an issue, or if somebody did something like kvm_init() { my_page_size = PAGE_SIZE; ... page_size_init(); ... use(my_page_size) } things would break. Thanks, drew ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size 2015-11-10 16:59 ` Andrew Jones @ 2015-11-10 17:02 ` Peter Maydell 0 siblings, 0 replies; 7+ messages in thread From: Peter Maydell @ 2015-11-10 17:02 UTC (permalink / raw) To: Andrew Jones; +Cc: Paolo Bonzini, QEMU Developers, kvm-devel On 10 November 2015 at 16:59, Andrew Jones <drjones@redhat.com> wrote: > On Tue, Nov 10, 2015 at 04:29:31PM +0000, Peter Maydell wrote: >> On 10 November 2015 at 00:23, Andrew Jones <drjones@redhat.com> wrote: >> > -/* KVM uses PAGE_SIZE in its definition of COALESCED_MMIO_MAX */ >> > -#define PAGE_SIZE TARGET_PAGE_SIZE >> > +/* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We >> > + * need to use the real host PAGE_SIZE, as that's what KVM will use. >> > + */ >> > +#define PAGE_SIZE getpagesize() >> >> Rather than defining PAGE_SIZE here (a confusing macro given >> we have several page sizes to deal with), why not just use >> getpagesize() in the one and only location where we currently >> use this macro? > > The macro is used by kernel headers that we import and include in > kvm-all.c. It's ugly, I agree, but that's how the this cookie crumbled. Oh, I see. That's pretty horrible. thanks -- PMM ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-11-10 17:03 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-11-10 0:23 [Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size Andrew Jones 2015-11-10 15:41 ` Paolo Bonzini 2015-11-10 15:57 ` Andrew Jones 2015-11-10 16:15 ` Paolo Bonzini 2015-11-10 16:29 ` Peter Maydell 2015-11-10 16:59 ` Andrew Jones 2015-11-10 17:02 ` Peter Maydell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).