| Message ID | 1447115022-4142-1-git-send-email-drjones@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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
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(-)