Re: [PATCH RFC] virtio-pci: new config layout: using memory BAR

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, May 28, 2013 at 01:53:35PM -0500, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Tue, May 28, 2013 at 12:15:16PM -0500, Anthony Liguori wrote:
> >> > @@ -455,6 +462,226 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr,
> >> >      }
> >> >  }
> >> >  
> >> > +static uint64_t virtio_pci_config_common_read(void *opaque, hwaddr addr,
> >> > +                                              unsigned size)
> >> > +{
> >> > +    VirtIOPCIProxy *proxy = opaque;
> >> > +    VirtIODevice *vdev = proxy->vdev;
> >> > +
> >> > +    uint64_t low = 0xffffffffull;
> >> > +
> >> > +    switch (addr) {
> >> > +    case offsetof(struct virtio_pci_common_cfg, device_feature_select):
> >> > +        return proxy->device_feature_select;
> >> 
> >> Oh dear no...  Please use defines like the rest of QEMU.
> >
> > Any good reason not to use offsetof?
> > I see about 138 examples in qemu.
> 
> There are exactly zero:
> 
> $ find . -name "*.c" -exec grep -l "case offset" {} \;
> $
> 
> > Anyway, that's the way Rusty wrote it in the kernel header -
> > I started with defines.
> > If you convince Rusty to switch I can switch too,
> 
> We have 300+ devices in QEMU that use #defines.  We're not using this
> kind of thing just because you want to copy code from the kernel.

Rusty, let's switch to defines?

> >> https://github.com/aliguori/qemu/commit/587c35c1a3fe90f6af0f97927047ef4d3182a659
> >> 
> >> And:
> >> 
> >> https://github.com/aliguori/qemu/commit/01ba80a23cf2eb1e15056f82b44b94ec381565cb
> >> 
> >> Which lets virtio-pci be subclassable and then remaps the config space to
> >> BAR2.
> >
> >
> > Interesting. Have the spec anywhere?
> 
> Not yet, but working on that.
> 
> > You are saying this is going to conflict because
> > of BAR2 usage, yes?
> 
> No, this whole thing is flexible.  I had to use BAR2 because BAR0 has to
> be the vram mapping.  It also had to be an MMIO bar.
> 
> The new layout might make it easier to implement a device like this.

Absolutely, you can put thing anywhere you like.

>  I
> shared it mainly because I wanted to show the subclassing idea vs. just
> tacking an option onto the existing virtio-pci code in QEMU.
> 
> Regards,
> 
> Anthony Liguori

I don't think a subclass will work for the new layout. This should be
completely transparent to users, just have more devices
work with more flexibility.

> > So let's only do this virtio-fb only for new layout, so we don't need
> > to maintain compatibility. In particular, we are working
> > on making memory BAR access fast for virtio devices
> > in a generic way. At the moment they are measureably slower
> > than PIO on x86.
> >
> >
> >> Haven't looked at the proposed new ring layout yet.
> >> 
> >> Regards,
> >
> > No new ring layout. It's new config layout.
> >
> >
> > -- 
> > MST
> > --
> > 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