Re: [Qemu-devel] [PATCH] virtio-pci: replace byte swap hack

[Michael S. Tsirkin]

On Wed, Jan 09, 2013 at 08:39:08PM +0000, Blue Swirl wrote:
> On Mon, Jan 7, 2013 at 4:11 PM, Michael S. Tsirkin <address@hidden> wrote:
> > On Sun, Jan 06, 2013 at 08:04:39PM +0000, Blue Swirl wrote:
> >> On Sun, Jan 6, 2013 at 6:25 PM, Andreas Färber <address@hidden> wrote:
> >> > Am 06.01.2013 14:17, schrieb Alexander Graf:
> >> >>
> >> >> On 30.12.2012, at 13:55, Blue Swirl wrote:
> >> >>
> >> >>> Remove byte swaps by declaring the config space
> >> >>> as native endian.
> >> >>
> >> >> This is wrong. Virtio-pci config space is split into 2 regions. One 
> >> >> with native endianness, the other one with little endian.
> >> >
> >> > Can that MemoryRegion be split in two?
> >>
> >> Yes, but unfortunately the offset for the second region depends on if
> >> MSIX is enabled or not. PCI layer manages these bits without the
> >> device seeing any changes.
> >>
> >> This could be handled by introducing a callback at PCI layer to inform
> >> interested devices about changes to MSIX setup, or even generalized:
> >> inform devices about changes within any set of bits specified by the
> >> device.
> >
> > We already have a generic config_write callback and even use it in
> > virtio pci: virtio_write_config.  So you could simply do there:
> >
> >         if (region size != VIRTIO_PCI_CONFIG(dev)) {
> >                 resize regions
> >         }
> >
> > We would also have to resize to the default setup on
> > vm load and on vm reset.
> >
> > Overall not sure whether this would make the code cleaner or uglier.
> 
> I think it would be a net cleanup. Most of the ugliness comes from the
> poor device architecture.
> 
> There could be (unmeasurably) small performance gains since accesses
> to the two regions would be dispatched directly to the handlers. But
> if the MSIX mode bit is toggled very often compared to the accesses to
> config registers, it could actually cause some slow down due to
> adjustment to the offset with the memory API. How often does that
> happen, once per boot or more often? Are these registers accessed very
> often by the guests?

datapath accesses the memory a lot, while OTOH mode change happens once
per boot normally, so yes, in theory it's a minor optimization.  Likely
not measureable by itself but if others think it's cleaner to
structure code that way I sure won't object to a patch like that.

> >
> >> >
> >> > Andreas
> >> >
> >> > --
> >> > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> >> > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg