[Qemu-devel] comments on: get page size in device init

[Qemu-devel] comments on: get page size in device init

[Michael S. Tsirkin]

>     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

[Qemu-devel] Re: comments on: get page size in device init

[Blue Swirl]

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.

[Qemu-devel] Re: comments on: get page size in device init

[Anthony Liguori]

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

[Qemu-devel] Re: comments on: get page size in device init

[Michael S. Tsirkin]

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

[Qemu-devel] Re: comments on: get page size in device init

[Blue Swirl]

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.

[Qemu-devel] Re: comments on: get page size in device init

[Michael S. Tsirkin]

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

[Qemu-devel] Re: comments on: get page size in device init

[Michael S. Tsirkin]

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

[Qemu-devel] Re: comments on: get page size in device init

[Blue Swirl]

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.

[Qemu-devel] Re: comments on: get page size in device init

[Anthony Liguori]

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