Linux Virtualization - May 2013 - [PATCH RFC] virtio-pci: new config layout: using memory BAR

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.

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,
but I prefer reusing same structures as linux even if
for now I've given up on reusing same headers.

> >From a QEMU pov, take a look at:
> 
>
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?
You are saying this is going to conflict because
of BAR2 usage, yes?

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

Il 28/05/2013 19:32, Michael S. Tsirkin ha scritto:
>>> > > +
>>> > > +    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'm not sure it's portable to use it in case labels.  IIRC, the
definition ((int)&(((T *)0)->field)) is not a valid C integer constant
expression.  Laszlo?
> I see about 138 examples in qemu.
Almost all of them are about fields in the host, not the guest, and none
of them are in case statements.

Paolo
> 
> 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,
> but I prefer reusing same structures as linux even if
> for now I've given up on reusing same headers.
> 
>

"Michael S. Tsirkin" <mst at 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.
>>
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.  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
> 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 at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, May 28, 2013 at 01:53:35PM -0500, Anthony Liguori
wrote:
> "Michael S. Tsirkin" <mst at 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 at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 05/28/13 19:43, Paolo Bonzini wrote:
> Il 28/05/2013 19:32, Michael S. Tsirkin ha scritto:
>>>>>> +
>>>>>> +    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'm not sure it's portable to use it in case labels.  IIRC, the
> definition ((int)&(((T *)0)->field)) is not a valid C integer
constant
> expression.  Laszlo?
As long as we use the offsetof() that comes with the C implementation
(ie. we don't hack it up ourselves), we should be safe; ISO C99
elegantly defines offsetof() in "7.17 Common definitions
<stddef.h>" p3
as

  The macros are [...]

    offsetof(type, member-designator)

  which expands to an integer constant expression that has type
  *size_t*, the value of which is the offset in bytes, to the structure
  member (designated by /member-designator/), from the beginning of its
  structure (designated by /type/). The type and member designator shall
  be such that given

    static type t;

  then the expression &(t.member-designator) evaluates to an address
  constant. (If the specified member is a bit-field, the behavior is
  undefined.)

("Address constant" is described in 6.6p9 but quoting even that here
doesn't seem necesary.)
>From 6.8.4.2p3, "The expression of each case label shall be an integer
constant expression [...]".

So,
(a) if we use the standard offsetof(),
(b) and you can also write, for example,

  static struct virtio_pci_common_cfg t;
  static void *p = &t.device_feature_select;

then the construct seems valid.

(Consistently with the above mention of UB if the specified member is a
bit-field: the address-of operator's constraints also forbid a bit-field
object as operand.)

Laszlo

Paolo Bonzini <pbonzini at redhat.com> writes:
> Il 28/05/2013 19:32, Michael S. Tsirkin ha scritto:
>>>> > > +
>>>> > > +    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'm not sure it's portable to use it in case labels.  IIRC, the
> definition ((int)&(((T *)0)->field)) is not a valid C integer
constant
> expression.  Laszlo?
It's defined to yield an integer constant expression in the ISO standard
(and I think ANSI too, though that's not at hand):

    7.19, para 3:

        ...offsetof(type, member-designator)
        which expands to an integer constant expression that has type
        size_t, ...

The real question is whether compilers qemu cares about meet the
standard (there's some evidence that older compilers fail this).  If
not, we'll have to define them as raw offsets...

Cheers,
Rusty.