qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using


From: Michael S. Tsirkin
Subject: Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
Date: Thu, 17 Apr 2014 16:44:25 +0300

On Thu, Apr 17, 2014 at 02:29:13PM +0200, Greg Kurz wrote:
> On Thu, 17 Apr 2014 11:00:26 +0300
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Thu, Apr 17, 2014 at 08:54:12AM +0200, Greg Kurz wrote:
> > > On Wed, 16 Apr 2014 20:32:07 +0300
> > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > 
> > > > On Wed, Apr 16, 2014 at 05:42:22PM +0100, Peter Maydell wrote:
> > > > > On 16 April 2014 17:34, Michael S. Tsirkin <address@hidden> wrote:
> > > > > > so it looks like virtio is currently compiled per-target.
> > > > > > So why isn't it reasonable to keep it per-target for
> > > > > > purpose of this enhancement?
> > > > > > What am I missing?
> > > > > 
> > > > > "virtio" is more than one C file. Currently per-target:
> > > > > hw/virtio/virtio.c
> > > > > hw/virtio/virtio-balloon.c
> > > > > hw/scsi/virtio-scsi.c
> > > > > hw/block/virtio-blk.c
> > > > > hw/char/virtio-serial-bus.c
> > > > > hw/net/virtio-net.c
> > > > > 
> > > > > Currently built once:
> > > > > hw/char/virtio-console.c
> > > > > hw/virtio/virtio-bus.c
> > > > > hw/virtio/virtio-mmio.c
> > > > > hw/virtio/virtio-pci.c
> > > > > hw/virtio/virtio-rng.c
> > > > > 
> > > > > If we can move files from the first group to the second
> > > > > that's cool.
> > > > 
> > > > But doesn't have to be part of this series, yes?
> > > > I would say let's at least have virtio 1.0 out
> > > > and implemented in qemu and linux guests, then
> > > > we can start deprecate virtio 0.X in favor of it.
> > > > 
> > > > > Moving files from the second to the first is
> > > > > what I'd like to avoid...
> > > > > 
> > > > > thanks
> > > > > -- PMM
> > > > 
> > > > Absolutely.
> > > > 
> > > > Looks like we are in agreement after all.
> > > > 
> > > > So as far as I could see the only issue is with config access
> > > > e.g. in virtio-pci?
> > > > That one already has a branch around virtio_is_big_endian,
> > > > so it's not a problem, there's no need to optimize that.
> > > > 
> > > 
> > > It is because you haven't looked at the other patches... When
> > > the whole serie is applied, all virtio files use inlined helpers
> > > from virtio-access.h to access memory and fix endianness. That is
> > > why we'd rather not add target dependency here...
> > 
> > Right, so what I'm saying is very simple: there are two types of helpers:
> > - per target ones for guest memory access
> > - generic ones for config access
> > 
> > 
> > One way to implement generic ones is as out of line wrappers
> > for inline per target ones, another way is a separate implementation.
> > 
> 
> Then you want something like this ?
> 
> static inline void virtio_stl_p(VirtIODevice *vdev, void *ptr, uint32_t v)
> {
> #ifdef TARGET_BIENDIAN
>     if (virtio_is_big_endian(vdev)) {
>         stl_be_p(ptr, v);
>     } else {
>         stl_le_p(ptr, v);
>     }
> #else
>     stl_p(ptr, v);
> #endif
> }


I'm fine with this.
And I'm not against a runtime switch to get rid of per-target build of virtio.
I am merely asking for separatable patchsets, e.g. structured like this:

1. implement bi-endian support with no data path overhead for fixed endian 
targets
2. cleanup getting rid of per-target build adding minor data path overhead 

This way down the line if we see performance regressions it's easy to
revert and verify what causes it.

An alternative is accompanying patchset with performance benchmarking
results.

> > 
> > > This being said, if you have an insight to have branch-less code
> > > for fixed endian targets, and dedicated code for ambivalent endian
> > > targets, please send something.
> > > 
> > > Cheers.
> > > 
> > > --
> > > Greg
> > 
> > Sure but I'm a bit busy otherwise - if above hints are insufficient,
> > expect something by Tuesday.
> > 
> > 
> 
> -- 
> Gregory Kurz                                     address@hidden
>                                                  address@hidden
> Software Engineer @ IBM/Meiosys                  http://www.ibm.com
> Tel +33 (0)562 165 496
> 
> "Anarchy is about taking complete responsibility for yourself."
>         Alan Moore.
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]