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

On Wed, May 29, 2013 at 09:16:39AM -0500, Anthony Liguori
wrote:
> "Michael S. Tsirkin" <mst at redhat.com> writes:
> 
> > On Wed, May 29, 2013 at 07:52:37AM -0500, Anthony Liguori wrote:
> >> 1) C makes no guarantees about structure layout beyond the first
> >>    member.  Yes, if it's naturally aligned or has a packed
attribute,
> >>    GCC does the right thing.  But this isn't kernel land
anymore,
> >>    portability matters and there are more compilers than GCC.
> >
> > You expect a compiler to pad this structure:
> >
> > struct foo {
> > 	uint8_t a;
> > 	uint8_t b;
> > 	uint16_t c;
> > 	uint32_t d;
> > };
> >
> > I'm guessing any compiler that decides to waste memory in this way
> > will quickly get dropped by users and then we won't worry
> > about building QEMU with it.
> 
> There are other responses in the thread here and I don't really care to
> bikeshed on this issue.
Great. Let's make the bikeshed blue then?
> >> Well, given that virtio is widely deployed today, I would think
the 1.0
> >> standard should strictly reflect what's deployed today, no?
> >> Any new config layout would be 2.0 material, right?
> >
> > Not as it's currently planned. Devices can choose
> > to support a legacy layout in addition to the new one,
> > and if you look at the patch you will see that that
> > is exactly what it does.
> 
> Adding a new BAR most certainly requires bumping the revision ID or
> changing the device ID, no?
No, why would it?

If a device dropped BAR0, that would be a good reason
to bump revision ID.
We don't do this yet.
> Didn't we run into this problem with the virtio-win drivers with just
> the BAR size changing? 
Because they had a bug: they validated BAR0 size. AFAIK they don't care
what happens with other bars.
> >> Re: the new config layout, I don't think we would want to use
it for
> >> anything but new devices.  Forcing a guest driver change
> >
> > There's no forcing.
> > If you look at the patches closely, you will see that
> > we still support the old layout on BAR0.
> >
> >
> >> is a really big
> >> deal and I see no reason to do that unless there's a
compelling reason
> >> to.
> >
> > There are many a compelling reasons, and they are well known
> > limitations of virtio PCI:
> >
> > - PCI spec compliance (madates device operation with IO memory
> > disabled).
> 
> PCI express spec.  We are fully compliant with the PCI spec.  And
what's
> the user visible advantage of pointing an emulated virtio device behind
> a PCI-e bus verses a legacy PCI bus?
Native hotplug support.
> This is a very good example because if we have to disable BAR0, then
> it's an ABI breaker plan and simple.
Not we. The BIOS can disable IO BAR: it can do this already
but the device won't be functional.
> > - support 64 bit addressing
> 
> We currently support 44-bit addressing for the ring.  While I agree we
> need to bump it, there's no immediate problem with 44-bit addressing.
I heard developers (though not users) complaining.
> > - add more than 32 feature bits.
> > - individually disable queues.
> > - sanely support cross-endian systems.
> > - support very small (<1 PAGE) for virtio rings.
> > - support a separate page for each vq kick.
> > - make each device place config at flexible offset.
> 
> None of these things are holding us back today.
All of them do, to bigger or lesser degree.

> I'm not saying we shouldn't introduce a new device.  But adoption
of
> that device will be slow and realistically will be limited to new
> devices only.
> 
> We'll be supporting both devices for a very, very long time.
This is true for any new feature. What are you trying to say here?
We won't add new features to old config: for once, we have
run out of feature bits.
> Compatibility is the fundamental value that we provide.  We need to go
> out of our way to make sure that existing guests work and work as well
> as possible.
What are you trying to say?
There's nothing here that breaks compatibility.
Have you looked at the patch?

I'm wasting my time arguing on the mailing list, but
once I tear myself away from this occupation,
I intend to verify that I can run an old guest on
qemu with this patch without issues.
> Sticking virtio devices behind a PCI-e bus just for the hell of it
isn't
> a compelling reason to break existing guests.
> 
> Regards,
> 
> Anthony Liguori
That's why my patch does not break existing guests.

> 
> > Addressing any one of these would cause us to add a substantially new
> > way to operate virtio devices.
> >
> > And since it's a guest change anyway, it seemed like a
> > good time to do the new layout and fix everything in one go.
> >
> > And they are needed like yesterday.
> >
> >
> >> So we're stuck with the 1.0 config layout for a very long
time.
> >> 
> >> Regards,
> >> 
> >> Anthony Liguori
> >
> > Absolutely. This patch let us support both which will allow for
> > a gradual transition over the next 10 years or so.
> >
> >> > reason.  I suggest that's 2.0 material...
> >> >
> >> > Cheers,
> >> > Rusty.
> >> >
> >> > --
> >> > 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
> > --
> > 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

Il 29/05/2013 16:30, Michael S. Tsirkin ha scritto:
>> > There are other responses in the thread here and I don't
really care to
>> > bikeshed on this issue.
> Great. Let's make the bikeshed blue then?
Yes, let's make it blue, but please promise to check into Peter's
register API so we can remove the case offsetof.

I checked that it works on RHEL5, which is 4.1 and probably the oldest
compiler we care about (any other 4.1 lacks the __sync builtins;
upstream added them in 4.2).

Paolo

On Wed, May 29, 2013 at 04:32:31PM +0200, Paolo Bonzini
wrote:
> Il 29/05/2013 16:30, Michael S. Tsirkin ha scritto:
> >> > There are other responses in the thread here and I don't
really care to
> >> > bikeshed on this issue.
> > Great. Let's make the bikeshed blue then?
> 
> Yes, let's make it blue, but please promise to check into Peter's
> register API so we can remove the case offsetof.
I think it's overkill here.  virtio was designed to be easy to implement
with a simple switch statement.
> I checked that it works on RHEL5, which is 4.1 and probably the oldest
> compiler we care about (any other 4.1 lacks the __sync builtins;
> upstream added them in 4.2).
> 
> Paolo
Are you sure? Documentation says 4.0 ...

-- 
MST

"Michael S. Tsirkin" <mst at redhat.com> writes:
> On Wed, May 29, 2013 at 09:16:39AM -0500, Anthony Liguori wrote:
>> "Michael S. Tsirkin" <mst at redhat.com> writes:
>> > I'm guessing any compiler that decides to waste memory in this
way
>> > will quickly get dropped by users and then we won't worry
>> > about building QEMU with it.
>> 
>> There are other responses in the thread here and I don't really
care to
>> bikeshed on this issue.
>
> Great. Let's make the bikeshed blue then?
It's fun to argue about stuff like this and I certainly have an opinion,
but I honestly don't care all that much about the offsetof thing.
However...

>
>> >> Well, given that virtio is widely deployed today, I would
think the 1.0
>> >> standard should strictly reflect what's deployed today,
no?
>> >> Any new config layout would be 2.0 material, right?
>> >
>> > Not as it's currently planned. Devices can choose
>> > to support a legacy layout in addition to the new one,
>> > and if you look at the patch you will see that that
>> > is exactly what it does.
>> 
>> Adding a new BAR most certainly requires bumping the revision ID or
>> changing the device ID, no?
>
> No, why would it?
If we change the programming interface for a device in a way that is
incompatible, we are required to change the revision ID and/or device
ID.
> If a device dropped BAR0, that would be a good reason
> to bump revision ID.
> We don't do this yet.
But we have to drop BAR0 to put it behind a PCI express bus, right?

If that's the case, then device that's exposed on the PCI express bus
must use a different device ID and/or revision ID.

That means a new driver is needed in the guest.
>> Didn't we run into this problem with the virtio-win drivers with
just
>> the BAR size changing? 
>
> Because they had a bug: they validated BAR0 size. AFAIK they don't care
> what happens with other bars.
I think there's a grey area with respect to the assumptions a device can
make about the programming interface.

But very concretely, we cannot expose virtio-pci-net via PCI express
with BAR0 disabled because that will result in existing virtio-pci Linux
drivers breaking.
> Not we. The BIOS can disable IO BAR: it can do this already
> but the device won't be functional.
But the only way to expose the device over PCI express is to disable the
IO BAR, right?

Regards,

Anthony Liguori

On Wed, May 29, 2013 at 09:55:55AM -0500, Anthony Liguori
wrote:
> "Michael S. Tsirkin" <mst at redhat.com> writes:
> 
> > On Wed, May 29, 2013 at 09:16:39AM -0500, Anthony Liguori wrote:
> >> "Michael S. Tsirkin" <mst at redhat.com> writes:
> >> > I'm guessing any compiler that decides to waste memory in
this way
> >> > will quickly get dropped by users and then we won't worry
> >> > about building QEMU with it.
> >> 
> >> There are other responses in the thread here and I don't
really care to
> >> bikeshed on this issue.
> >
> > Great. Let's make the bikeshed blue then?
> 
> It's fun to argue about stuff like this and I certainly have an
opinion,
> but I honestly don't care all that much about the offsetof thing.
> However...
> 
> 
> >
> >> >> Well, given that virtio is widely deployed today, I would
think the 1.0
> >> >> standard should strictly reflect what's deployed
today, no?
> >> >> Any new config layout would be 2.0 material, right?
> >> >
> >> > Not as it's currently planned. Devices can choose
> >> > to support a legacy layout in addition to the new one,
> >> > and if you look at the patch you will see that that
> >> > is exactly what it does.
> >> 
> >> Adding a new BAR most certainly requires bumping the revision ID
or
> >> changing the device ID, no?
> >
> > No, why would it?
> 
> If we change the programming interface for a device in a way that is
> incompatible, we are required to change the revision ID and/or device
> ID.
It's compatible.

> > If a device dropped BAR0, that would be a good reason
> > to bump revision ID.
> > We don't do this yet.
> 
> But we have to drop BAR0 to put it behind a PCI express bus, right?
No, no.

The PCIe spec states that failure to allocate an I/O BAR should still
allow the device to function.

That's *guest* allocating an I/O BAR.

> If that's the case, then device that's exposed on the PCI express
bus
> must use a different device ID and/or revision ID.
> 
> That means a new driver is needed in the guest.
> 
> >> Didn't we run into this problem with the virtio-win drivers
with just
> >> the BAR size changing? 
> >
> > Because they had a bug: they validated BAR0 size. AFAIK they don't
care
> > what happens with other bars.
> 
> I think there's a grey area with respect to the assumptions a device
can
> make about the programming interface.
> 
> But very concretely, we cannot expose virtio-pci-net via PCI express
> with BAR0 disabled because that will result in existing virtio-pci Linux
> drivers breaking.
> 
> > Not we. The BIOS can disable IO BAR: it can do this already
> > but the device won't be functional.
> 
> But the only way to expose the device over PCI express is to disable the
> IO BAR, right?
> 
> Regards,
> 
> Anthony Liguori
No :)

On Wed, May 29, 2013 at 09:55:55AM -0500, Anthony Liguori
wrote:
> > Not we. The BIOS can disable IO BAR: it can do this already
> > but the device won't be functional.
> 
> But the only way to expose the device over PCI express is to disable the
> IO BAR, right?
I think this is the source of the misunderstanding:

	1.3.2.2.  PCI Express Endpoint Rules

	...

	PCI Express Endpoint must not depend on operating system
	allocation of I/O resources claimed through BAR(s).

The real meaning here is *not* that an Endpoint should not
have an I/O BAR. An Endpoint can have an I/O BAR,
and PCI Express spec supports I/O transactions.
The meaning is that an Endpoint should work
even if *the OS* decides to disable the I/O BAR.
Note: it's up to the guest. We support I/O
as a device with full compatibility for old guests,
but must be prepared to handle a guest which does not
enable I/O.

This likely means that as time goes on,
future OSes will start disabling I/O BARs,
relying on this not hurting functionality.

-- 
MST