* [Qemu-devel] comments on: get page size in device init
@ 2009-09-23 12:58 Michael S. Tsirkin
2009-09-23 17:07 ` [Qemu-devel] " Blue Swirl
0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2009-09-23 12:58 UTC (permalink / raw)
To: qemu-devel, anthony, Blue Swirl
> Compile msix only once
>
> Get page size in device init.
>
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
What was the motivation for the page size change?
It seems the only user passes TARGET_PAGE_SIZE anyway,
using a constant seems clearer and probably generates
less code. No?
Did I miss this patch on qemu-devel? Generally, it's nice to have
patches posted to list to give people a chance to comment, before they
are pushed to the public tree. Right?
Finally, the 2 changes seem unrelated: why was it a good idea to bundle
them in one commit?
Thanks,
--
MST
^ permalink raw reply [flat|nested] 9+ messages in thread* [Qemu-devel] Re: comments on: get page size in device init 2009-09-23 12:58 [Qemu-devel] comments on: get page size in device init Michael S. Tsirkin @ 2009-09-23 17:07 ` Blue Swirl 2009-09-23 18:02 ` Anthony Liguori 0 siblings, 1 reply; 9+ messages in thread From: Blue Swirl @ 2009-09-23 17:07 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Wed, Sep 23, 2009 at 3:58 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> Compile msix only once >> >> Get page size in device init. >> >> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> > > What was the motivation for the page size change? "Compile msix only once" > It seems the only user passes TARGET_PAGE_SIZE anyway, > using a constant seems clearer and probably generates > less code. No? Yes, but then the code would depend on TARGET_PAGE_SIZE, making it impossible to compile the code only once. Moreover, devices in general (with the exception of maybe virtio and Xen devices and horrible abominations like vmmouse/vmport) should have no knowledge about the CPU. TARGET_PAGE_SIZE is a parameter of the CPU, no device should need to use it. Therefore this commit is actually a cleanup. > Did I miss this patch on qemu-devel? Generally, it's nice to have > patches posted to list to give people a chance to comment, before they > are pushed to the public tree. Right? No, you did not miss anything. I was not nice, sorry about that. > Finally, the 2 changes seem unrelated: why was it a good idea to bundle > them in one commit? Which 2 changes are you referring to? I think all changes are necessary. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] Re: comments on: get page size in device init 2009-09-23 17:07 ` [Qemu-devel] " Blue Swirl @ 2009-09-23 18:02 ` Anthony Liguori 2009-09-23 18:40 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Anthony Liguori @ 2009-09-23 18:02 UTC (permalink / raw) To: Blue Swirl; +Cc: qemu-devel, Michael S. Tsirkin Blue Swirl wrote: > On Wed, Sep 23, 2009 at 3:58 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > >>> Compile msix only once >>> >>> Get page size in device init. >>> >>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> >>> >> What was the motivation for the page size change? >> > > "Compile msix only once" > > >> It seems the only user passes TARGET_PAGE_SIZE anyway, >> using a constant seems clearer and probably generates >> less code. No? >> > > Yes, but then the code would depend on TARGET_PAGE_SIZE, making it > impossible to compile the code only once. > We could probably get away with doing #define TARGET_PAGE_SIZE target_get_page_size() And take care of a big chunk of this without passing page size parameters around. Regards, Anthony Liguori ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] Re: comments on: get page size in device init 2009-09-23 18:02 ` Anthony Liguori @ 2009-09-23 18:40 ` Michael S. Tsirkin 2009-09-23 19:03 ` Blue Swirl 0 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2009-09-23 18:40 UTC (permalink / raw) To: Anthony Liguori; +Cc: Blue Swirl, qemu-devel On Wed, Sep 23, 2009 at 01:02:56PM -0500, Anthony Liguori wrote: > Blue Swirl wrote: >> On Wed, Sep 23, 2009 at 3:58 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> >>>> Compile msix only once >>>> >>>> Get page size in device init. >>>> >>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> >>>> >>> What was the motivation for the page size change? >>> >> >> "Compile msix only once" >> >> >>> It seems the only user passes TARGET_PAGE_SIZE anyway, >>> using a constant seems clearer and probably generates >>> less code. No? >>> >> >> Yes, but then the code would depend on TARGET_PAGE_SIZE, making it >> impossible to compile the code only once. >> > > We could probably get away with doing > > #define TARGET_PAGE_SIZE target_get_page_size() > > And take care of a big chunk of this without passing page size > parameters around. Sounds good. > Regards, > > Anthony Liguori ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] Re: comments on: get page size in device init 2009-09-23 18:40 ` Michael S. Tsirkin @ 2009-09-23 19:03 ` Blue Swirl 2009-09-23 19:13 ` Michael S. Tsirkin ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Blue Swirl @ 2009-09-23 19:03 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Wed, Sep 23, 2009 at 9:40 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Wed, Sep 23, 2009 at 01:02:56PM -0500, Anthony Liguori wrote: >> Blue Swirl wrote: >>> On Wed, Sep 23, 2009 at 3:58 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >>> >>>>> Compile msix only once >>>>> >>>>> Get page size in device init. >>>>> >>>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> >>>>> >>>> What was the motivation for the page size change? >>>> >>> >>> "Compile msix only once" >>> >>> >>>> It seems the only user passes TARGET_PAGE_SIZE anyway, >>>> using a constant seems clearer and probably generates >>>> less code. No? >>>> >>> >>> Yes, but then the code would depend on TARGET_PAGE_SIZE, making it >>> impossible to compile the code only once. >>> >> >> We could probably get away with doing >> >> #define TARGET_PAGE_SIZE target_get_page_size() >> >> And take care of a big chunk of this without passing page size >> parameters around. > > > Sounds good. That would work and target_get_page_size() together with get_ram_size() would also handle the virtio case nicely, except for the if (!kvm_enabled() || kvm_has_sync_mmu()) part. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] Re: comments on: get page size in device init 2009-09-23 19:03 ` Blue Swirl @ 2009-09-23 19:13 ` Michael S. Tsirkin 2009-09-23 19:59 ` Blue Swirl 2009-09-23 19:19 ` Michael S. Tsirkin 2009-09-23 20:35 ` Anthony Liguori 2 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2009-09-23 19:13 UTC (permalink / raw) To: Blue Swirl; +Cc: qemu-devel On Wed, Sep 23, 2009 at 10:03:26PM +0300, Blue Swirl wrote: > On Wed, Sep 23, 2009 at 9:40 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Sep 23, 2009 at 01:02:56PM -0500, Anthony Liguori wrote: > >> Blue Swirl wrote: > >>> On Wed, Sep 23, 2009 at 3:58 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > >>> > >>>>> Compile msix only once > >>>>> > >>>>> Get page size in device init. > >>>>> > >>>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> > >>>>> > >>>> What was the motivation for the page size change? > >>>> > >>> > >>> "Compile msix only once" > >>> > >>> > >>>> It seems the only user passes TARGET_PAGE_SIZE anyway, > >>>> using a constant seems clearer and probably generates > >>>> less code. No? > >>>> > >>> > >>> Yes, but then the code would depend on TARGET_PAGE_SIZE, making it > >>> impossible to compile the code only once. > >>> > >> > >> We could probably get away with doing > >> > >> #define TARGET_PAGE_SIZE target_get_page_size() > >> > >> And take care of a big chunk of this without passing page size > >> parameters around. > > > > > > Sounds good. > > That would work and target_get_page_size() together with > get_ram_size() would also handle the virtio case nicely, except for > the if (!kvm_enabled() || kvm_has_sync_mmu()) part. Where's get_ram_size needed? -- MST ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] Re: comments on: get page size in device init 2009-09-23 19:13 ` Michael S. Tsirkin @ 2009-09-23 19:59 ` Blue Swirl 0 siblings, 0 replies; 9+ messages in thread From: Blue Swirl @ 2009-09-23 19:59 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Wed, Sep 23, 2009 at 10:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Wed, Sep 23, 2009 at 10:03:26PM +0300, Blue Swirl wrote: >> On Wed, Sep 23, 2009 at 9:40 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > On Wed, Sep 23, 2009 at 01:02:56PM -0500, Anthony Liguori wrote: >> >> Blue Swirl wrote: >> >>> On Wed, Sep 23, 2009 at 3:58 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> >>> >> >>>>> Compile msix only once >> >>>>> >> >>>>> Get page size in device init. >> >>>>> >> >>>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> >> >>>>> >> >>>> What was the motivation for the page size change? >> >>>> >> >>> >> >>> "Compile msix only once" >> >>> >> >>> >> >>>> It seems the only user passes TARGET_PAGE_SIZE anyway, >> >>>> using a constant seems clearer and probably generates >> >>>> less code. No? >> >>>> >> >>> >> >>> Yes, but then the code would depend on TARGET_PAGE_SIZE, making it >> >>> impossible to compile the code only once. >> >>> >> >> >> >> We could probably get away with doing >> >> >> >> #define TARGET_PAGE_SIZE target_get_page_size() >> >> >> >> And take care of a big chunk of this without passing page size >> >> parameters around. >> > >> > >> > Sounds good. >> >> That would work and target_get_page_size() together with >> get_ram_size() would also handle the virtio case nicely, except for >> the if (!kvm_enabled() || kvm_has_sync_mmu()) part. > > Where's get_ram_size needed? In virtio-balloon. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] Re: comments on: get page size in device init 2009-09-23 19:03 ` Blue Swirl 2009-09-23 19:13 ` Michael S. Tsirkin @ 2009-09-23 19:19 ` Michael S. Tsirkin 2009-09-23 20:35 ` Anthony Liguori 2 siblings, 0 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2009-09-23 19:19 UTC (permalink / raw) To: Blue Swirl; +Cc: qemu-devel On Wed, Sep 23, 2009 at 10:03:26PM +0300, Blue Swirl wrote: > On Wed, Sep 23, 2009 at 9:40 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Sep 23, 2009 at 01:02:56PM -0500, Anthony Liguori wrote: > >> Blue Swirl wrote: > >>> On Wed, Sep 23, 2009 at 3:58 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > >>> > >>>>> Compile msix only once > >>>>> > >>>>> Get page size in device init. > >>>>> > >>>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> > >>>>> > >>>> What was the motivation for the page size change? > >>>> > >>> > >>> "Compile msix only once" > >>> > >>> > >>>> It seems the only user passes TARGET_PAGE_SIZE anyway, > >>>> using a constant seems clearer and probably generates > >>>> less code. No? > >>>> > >>> > >>> Yes, but then the code would depend on TARGET_PAGE_SIZE, making it > >>> impossible to compile the code only once. > >>> > >> > >> We could probably get away with doing > >> > >> #define TARGET_PAGE_SIZE target_get_page_size() > >> > >> And take care of a big chunk of this without passing page size > >> parameters around. > > > > > > Sounds good. > > That would work and target_get_page_size() together with > get_ram_size() would also handle the virtio case nicely, except for > the if (!kvm_enabled() || kvm_has_sync_mmu()) part. OK, I did and just sent the msix part, virtio can come on top. -- MST ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] Re: comments on: get page size in device init 2009-09-23 19:03 ` Blue Swirl 2009-09-23 19:13 ` Michael S. Tsirkin 2009-09-23 19:19 ` Michael S. Tsirkin @ 2009-09-23 20:35 ` Anthony Liguori 2 siblings, 0 replies; 9+ messages in thread From: Anthony Liguori @ 2009-09-23 20:35 UTC (permalink / raw) To: Blue Swirl; +Cc: qemu-devel, Michael S. Tsirkin Blue Swirl wrote: > That would work and target_get_page_size() together with > get_ram_size() would also handle the virtio case nicely, except for > the if (!kvm_enabled() || kvm_has_sync_mmu()) part. > We probably want to introduce a qemu_zero_memory_page() that includes that check. We have the same basic check during memory loading too. Regards, Anthony Liguori ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-09-23 20:36 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-09-23 12:58 [Qemu-devel] comments on: get page size in device init Michael S. Tsirkin 2009-09-23 17:07 ` [Qemu-devel] " Blue Swirl 2009-09-23 18:02 ` Anthony Liguori 2009-09-23 18:40 ` Michael S. Tsirkin 2009-09-23 19:03 ` Blue Swirl 2009-09-23 19:13 ` Michael S. Tsirkin 2009-09-23 19:59 ` Blue Swirl 2009-09-23 19:19 ` Michael S. Tsirkin 2009-09-23 20:35 ` Anthony Liguori
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).