[Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size

[Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size

[Andrew Jones]

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

Re: [Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size

[Andrew Jones]

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

Re: [Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size

[Andrew Jones]

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

Re: [Qemu-devel] [PATCH] kvm-all: PAGE_SIZE should be real host page size

[Peter Maydell]

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