Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Wed, Nov 16, 2011 at 10:17:39AM +0200, Sasha Levin wrote:
> On Wed, 2011-11-16 at 09:21 +0200, Michael S. Tsirkin wrote:
> > On Wed, Nov 16, 2011 at 10:28:52AM +1030, Rusty Russell wrote:
> > > On Fri, 11 Nov 2011 09:39:13 +0200, Sasha Levin
> > > wrote:
> > > > On Fri, Nov 11, 2011 at 6:24 AM, Rusty Russell
> > > > wrote:
> > > > > (2) There's no huge win in keeping the same layout. Let's make some
> > > > >cleanups. There are more users ahead of us then behind us (I
> > > > >hope!).
> > > >
> > > > Actually, if we already do cleanups, here are two more suggestions:
> > > >
> > > > 1. Make 64bit features a one big 64bit block, instead of having 32bits
> > > > in one place and 32 in another.
> > > > 2. Remove the reserved fields out of the config (the ones that were
> > > > caused by moving the ISR and the notifications out).
> > >
> > > Yes, those were exactly what I was thinking. I left it vague because
> > > there might be others you can see if we're prepared to abandon the
> > > current format.
> > >
> > > Cheers,
> > > Rusty.
> >
> > Yes but driver code doesn't get any cleaner by moving the fields.
> > And in fact, the legacy support makes the code messier.
> > What are the advantages?
> >
The advantages question is what should really balance out the overhead.
> What about splitting the parts which handle legacy code and new code?
Well, I considered that. Something along the lines of
#define VIRTIO_NEW_MSI_CONFIG_VECTOR18
And so on for all registers.
This seems to add a significant maintainance burden because of code
duplication. Note that, for example, vector programming is affected.
Multiply that by the number of guest OSes.
> It'll make it easier playing with the new spec more freely
I'm really worried about maintaing drivers long term.
Ease of experimentation is secondary for me.
> and will also
> make it easier removing legacy code in the future since you'll need to
> simply delete a chunk of code instead of removing legacy bits out of
> working code with a surgical knife.
It's unlikely to be a single chunk: we'd have structures and macros
which are separate. So at least 3 chunks.
Just for fun, here's what's involved in removing legacy map
support on top of my patch. As you see there are 4 chunks:
structure decl, map, unmap, and msix enable/disable.
And finding them was as simple as looking for legacy_map.
---
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index d242fcc..6c4d2faf 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -64,9 +64,6 @@ struct virtio_pci_device
/* Various IO mappings: used for resource tracking only. */
- /* Legacy BAR0: typically PIO. */
- void __iomem *legacy_map;
-
/* Mappings specified by device capabilities: typically in MMIO */
void __iomem *isr_map;
void __iomem *notify_map;
@@ -81,11 +78,7 @@ struct virtio_pci_device
static void virtio_pci_set_msix_enabled(struct virtio_pci_device *vp_dev, int
enabled)
{
vp_dev->msix_enabled = enabled;
- if (vp_dev->device_map)
- vp_dev->ioaddr_device = vp_dev->device_map;
- else
- vp_dev->ioaddr_device = vp_dev->legacy_map +
- VIRTIO_PCI_CONFIG(vp_dev);
+ vp_dev->ioaddr_device = vp_dev->device_map;
}
static void __iomem *virtio_pci_map_cfg(struct virtio_pci_device *vp_dev, u8
cap_id,
@@ -147,8 +140,6 @@ err:
static void virtio_pci_iounmap(struct virtio_pci_device *vp_dev)
{
- if (vp_dev->legacy_map)
- pci_iounmap(vp_dev->pci_dev, vp_dev->legacy_map);
if (vp_dev->isr_map)
pci_iounmap(vp_dev->pci_dev, vp_dev->isr_map);
if (vp_dev->notify_map)
@@ -176,36 +167,15 @@ static int virtio_pci_iomap(struct virtio_pci_device
*vp_dev)
if (!vp_dev->notify_map || !vp_dev->common_map ||
!vp_dev->device_map) {
- /*
-* If not all capabilities present, map legacy PIO.
-* Legacy access is at BAR 0. We never need to map
-* more than 256 bytes there, since legacy config space
-* used PIO which has this size limit.
-* */
- vp_dev->legacy_map = pci_iomap(vp_dev->pci_dev, 0, 256);
- if (!vp_dev->legacy_map) {
- dev_err(&vp_dev->vdev.dev, "Unable to map legacy PIO");
- goto err;
- }
+ dev_err(&vp_dev->vdev.dev, "Unable to map IO");
+ goto err;
}
- /* Prefer MMIO if available. If not, fallback to legacy PIO. */
- if (vp_dev->common_map)
- vp_dev->ioaddr = vp_dev->common_map;
- else
- vp_dev->ioaddr = vp_dev->legacy_map;
+ vp_dev->ioaddr = vp_dev->common_map;
- if (vp_dev->device_map)
- vp_dev->ioaddr_device = vp_dev->device_map;
- else
- vp_dev->io
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Wed, 2011-11-16 at 09:21 +0200, Michael S. Tsirkin wrote: > On Wed, Nov 16, 2011 at 10:28:52AM +1030, Rusty Russell wrote: > > On Fri, 11 Nov 2011 09:39:13 +0200, Sasha Levin > > wrote: > > > On Fri, Nov 11, 2011 at 6:24 AM, Rusty Russell > > > wrote: > > > > (2) There's no huge win in keeping the same layout. Let's make some > > > >cleanups. There are more users ahead of us then behind us (I > > > >hope!). > > > > > > Actually, if we already do cleanups, here are two more suggestions: > > > > > > 1. Make 64bit features a one big 64bit block, instead of having 32bits > > > in one place and 32 in another. > > > 2. Remove the reserved fields out of the config (the ones that were > > > caused by moving the ISR and the notifications out). > > > > Yes, those were exactly what I was thinking. I left it vague because > > there might be others you can see if we're prepared to abandon the > > current format. > > > > Cheers, > > Rusty. > > Yes but driver code doesn't get any cleaner by moving the fields. > And in fact, the legacy support makes the code messier. > What are the advantages? > What about splitting the parts which handle legacy code and new code? It'll make it easier playing with the new spec more freely and will also make it easier removing legacy code in the future since you'll need to simply delete a chunk of code instead of removing legacy bits out of working code with a surgical knife. -- Sasha. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Wed, Nov 16, 2011 at 10:28:52AM +1030, Rusty Russell wrote: > On Fri, 11 Nov 2011 09:39:13 +0200, Sasha Levin > wrote: > > On Fri, Nov 11, 2011 at 6:24 AM, Rusty Russell > > wrote: > > > (2) There's no huge win in keeping the same layout. Let's make some > > > cleanups. There are more users ahead of us then behind us (I > > > hope!). > > > > Actually, if we already do cleanups, here are two more suggestions: > > > > 1. Make 64bit features a one big 64bit block, instead of having 32bits > > in one place and 32 in another. > > 2. Remove the reserved fields out of the config (the ones that were > > caused by moving the ISR and the notifications out). > > Yes, those were exactly what I was thinking. I left it vague because > there might be others you can see if we're prepared to abandon the > current format. > > Cheers, > Rusty. Yes but driver code doesn't get any cleaner by moving the fields. And in fact, the legacy support makes the code messier. What are the advantages? -- MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Wed, Nov 16, 2011 at 10:28:05AM +1030, Rusty Russell wrote: > On Sun, 13 Nov 2011 17:14:27 +0200, "Michael S. Tsirkin" > wrote: > > On Fri, Nov 11, 2011 at 02:54:31PM +1030, Rusty Russell wrote: > > > Indeed, I'd like to see two changes to your proposal: > > > > > > (1) It should be all or nothing. If a driver can find the virtio header > > > capability, it should only use the capabilties. Otherwise, it > > > should fall back to legacy. > > > > Okay, but going forward, if we add more capabilities, we probably won't > > want to require them and fail to load if not there. That's really why I > > wanted to make the failover ignore any capability separately - to make > > this future proof. I'm not terribly fixated on this, it just seemed a > > bit more symmetrical to treat all capabilities in the same way. Hmm? > > Sure, a future capbility may not exist. But once a driver finds that > virtio header structure in the capability, it should *never* fall back > to the legacy area. ie. it can expect that Queue Notify, ISR Status and > Device Header all exist. > > ie. either use legacy mode, or use capabilities. Never both. > > > > > > Your draft suggests a mix is possible; > > > I prefer a clean failure (ie. one day don't present a BAR 0 *at > > > all*, so ancient drivers just fail to load.). > > > > Just to clarify, as written in draft this is possible with the current > > spec proposal. So I'm guessing there's some other motivation that you > > had in mind? > > At the moment you give a hybrid model where both are used. In five > years' time, that's going to be particularly ugly. > > > > > (2) There's no huge win in keeping the same layout. Let's make some > > > cleanups. > > > > About this last point - what cleanups do you have in mind? Just move > > some registers around? I guess we could put feature bits near each > > other, and move device status towards the end to avoid wasting 3 > > bytes. > > > The win seems minimal, but the change does make legacy hypervisor > > support in guests more cumbersome, as we need to spread coditional code > > around instead of localizing it in the initialization path. > > But the separation between "legacy" and "modern" will be sharper, making > it easier to excise the legacy portion later. > > And in five years' time, people implementing virtio will really thank us > that they can completely ignore the legacy header. OK, I get it I think. > > >There are more users ahead of us then behind us (I > > > hope!). > > > > In that case isn't it safe to assume we'll find some uses > > for the reserved registers? > > How would we tell? If we use a new capability struct for it, it's > obvious. Otherwise, you're going to need to steal more feature bits. Yes, exactly, if as you suggest, we disable legacy header when there is a capability - we can use reserved registers for other stuff. > Cheers, > Rusty. > PS. Sorry, was off sick for a few days. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Fri, 11 Nov 2011 09:39:13 +0200, Sasha Levin wrote: > On Fri, Nov 11, 2011 at 6:24 AM, Rusty Russell wrote: > > (2) There's no huge win in keeping the same layout. Let's make some > > cleanups. There are more users ahead of us then behind us (I > > hope!). > > Actually, if we already do cleanups, here are two more suggestions: > > 1. Make 64bit features a one big 64bit block, instead of having 32bits > in one place and 32 in another. > 2. Remove the reserved fields out of the config (the ones that were > caused by moving the ISR and the notifications out). Yes, those were exactly what I was thinking. I left it vague because there might be others you can see if we're prepared to abandon the current format. Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Sun, 13 Nov 2011 17:14:27 +0200, "Michael S. Tsirkin" wrote: > On Fri, Nov 11, 2011 at 02:54:31PM +1030, Rusty Russell wrote: > > Indeed, I'd like to see two changes to your proposal: > > > > (1) It should be all or nothing. If a driver can find the virtio header > > capability, it should only use the capabilties. Otherwise, it > > should fall back to legacy. > > Okay, but going forward, if we add more capabilities, we probably won't > want to require them and fail to load if not there. That's really why I > wanted to make the failover ignore any capability separately - to make > this future proof. I'm not terribly fixated on this, it just seemed a > bit more symmetrical to treat all capabilities in the same way. Hmm? Sure, a future capbility may not exist. But once a driver finds that virtio header structure in the capability, it should *never* fall back to the legacy area. ie. it can expect that Queue Notify, ISR Status and Device Header all exist. ie. either use legacy mode, or use capabilities. Never both. > > > Your draft suggests a mix is possible; > > I prefer a clean failure (ie. one day don't present a BAR 0 *at > > all*, so ancient drivers just fail to load.). > > Just to clarify, as written in draft this is possible with the current > spec proposal. So I'm guessing there's some other motivation that you > had in mind? At the moment you give a hybrid model where both are used. In five years' time, that's going to be particularly ugly. > > > (2) There's no huge win in keeping the same layout. Let's make some > > cleanups. > > About this last point - what cleanups do you have in mind? Just move > some registers around? I guess we could put feature bits near each > other, and move device status towards the end to avoid wasting 3 > bytes. > The win seems minimal, but the change does make legacy hypervisor > support in guests more cumbersome, as we need to spread coditional code > around instead of localizing it in the initialization path. But the separation between "legacy" and "modern" will be sharper, making it easier to excise the legacy portion later. And in five years' time, people implementing virtio will really thank us that they can completely ignore the legacy header. > >There are more users ahead of us then behind us (I > > hope!). > > In that case isn't it safe to assume we'll find some uses > for the reserved registers? How would we tell? If we use a new capability struct for it, it's obvious. Otherwise, you're going to need to steal more feature bits. Cheers, Rusty. PS. Sorry, was off sick for a few days. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Sun, Nov 13, 2011 at 05:14:27PM +0200, Michael S. Tsirkin wrote: > On Fri, Nov 11, 2011 at 02:54:31PM +1030, Rusty Russell wrote: > > On Wed, 09 Nov 2011 22:57:28 +0200, Sasha Levin > > wrote: > > > On Wed, 2011-11-09 at 22:52 +0200, Michael S. Tsirkin wrote: > > > > On Wed, Nov 09, 2011 at 10:24:47PM +0200, Sasha Levin wrote: > > > > > It'll be a bit harder deprecating it in the future. > > > > > > > > Harder than ... what ? > > > > > > Harder than allowing devices not to present it at all if new layout > > > config is used. Right now the simple implementation is to use MMIO for > > > config and device specific, and let it fallback to legacy for ISR and > > > notifications (and therefore, this is probably how everybody will > > > implement it), which means that when you do want to deprecate legacy, > > > there will be extra work to be done then, instead of doing it now. > > > > Indeed, I'd like to see two changes to your proposal: > > > > (1) It should be all or nothing. If a driver can find the virtio header > > capability, it should only use the capabilties. Otherwise, it > > should fall back to legacy. > > Okay, but going forward, if we add more capabilities, we probably won't > want to require them and fail to load if not there. That's really why I > wanted to make the failover ignore any capability separately - to make > this future proof. I'm not terribly fixated on this, it just seemed a > bit more symmetrical to treat all capabilities in the same way. Hmm? > > > Your draft suggests a mix is possible; > > I prefer a clean failure (ie. one day don't present a BAR 0 *at > > all*, so ancient drivers just fail to load.). > > Just to clarify, as written in draft this is possible with the current > spec proposal. So I'm guessing there's some other motivation that you > had in mind? > > > (2) There's no huge win in keeping the same layout. Let's make some > > cleanups. > > About this last point - what cleanups do you have in mind? Just move > some registers around? I guess we could put feature bits near each > other, and move device status towards the end to avoid wasting 3 bytes. > The win seems minimal, but the change does make legacy hypervisor > support in guests more cumbersome, as we need to spread coditional code > around instead of localizing it in the initialization path. > > >There are more users ahead of us then behind us (I > > hope!). > > In that case isn't it safe to assume we'll find some uses > for the reserved registers? > > > But I think this is the right direction! > > > > Thanks, > > Rusty. Note: waiting on response to the above before I tweak the spec again. -- MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Fri, Nov 11, 2011 at 02:54:31PM +1030, Rusty Russell wrote: > On Wed, 09 Nov 2011 22:57:28 +0200, Sasha Levin > wrote: > > On Wed, 2011-11-09 at 22:52 +0200, Michael S. Tsirkin wrote: > > > On Wed, Nov 09, 2011 at 10:24:47PM +0200, Sasha Levin wrote: > > > > It'll be a bit harder deprecating it in the future. > > > > > > Harder than ... what ? > > > > Harder than allowing devices not to present it at all if new layout > > config is used. Right now the simple implementation is to use MMIO for > > config and device specific, and let it fallback to legacy for ISR and > > notifications (and therefore, this is probably how everybody will > > implement it), which means that when you do want to deprecate legacy, > > there will be extra work to be done then, instead of doing it now. > > Indeed, I'd like to see two changes to your proposal: > > (1) It should be all or nothing. If a driver can find the virtio header > capability, it should only use the capabilties. Otherwise, it > should fall back to legacy. Okay, but going forward, if we add more capabilities, we probably won't want to require them and fail to load if not there. That's really why I wanted to make the failover ignore any capability separately - to make this future proof. I'm not terribly fixated on this, it just seemed a bit more symmetrical to treat all capabilities in the same way. Hmm? > Your draft suggests a mix is possible; > I prefer a clean failure (ie. one day don't present a BAR 0 *at > all*, so ancient drivers just fail to load.). Just to clarify, as written in draft this is possible with the current spec proposal. So I'm guessing there's some other motivation that you had in mind? > (2) There's no huge win in keeping the same layout. Let's make some > cleanups. About this last point - what cleanups do you have in mind? Just move some registers around? I guess we could put feature bits near each other, and move device status towards the end to avoid wasting 3 bytes. The win seems minimal, but the change does make legacy hypervisor support in guests more cumbersome, as we need to spread coditional code around instead of localizing it in the initialization path. >There are more users ahead of us then behind us (I > hope!). In that case isn't it safe to assume we'll find some uses for the reserved registers? > But I think this is the right direction! > > Thanks, > Rusty. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
> > Also, an unrelated questions: With PIO, requests were ordered, which > > means that if we wrote to the queue selector and then read from a > > queue register we would read the correct queue info. > > Is the same thing assured to us with MMIO? > > For real PCI, reads do not bypass writes in PCI. However this > is only true if both are MMIO or both PIO reads. > I don't think the ordering of MMIO versus PIO is guaranteed. > > On KVM, the kernel doesn't do anything to guarantee ordering. > So you get the natural ordering of the CPU. > > > If we write to a queue > > selector and immediately read from queue info would we be reading the > > right info, or is there the slight chance that it would get reordered > > and we would be reading queue info first and writing to the selector > > later? > > The thing to realize is that write to queue selector with KVM is in the > end performed by host. And reading queue info means that host will be > reading the queue selector. So this is a write followed by read > from the same address. AFAIK no CPUs can reorder such accesses, > so you get the right info. (As far as I understand all the complexity ;-) Memory mapped using ioremap()-like functions should preserve access order. In case of ARM architecture such memory region is defined (at the page tables level) as a "device memory" (contrary to "normal memory") and the processor will not try to be clever about it. I know next-to-nothing about x86, but I suppose similar idea applies. Cheers! Paweł -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Fri, Nov 11, 2011 at 09:39:13AM +0200, Sasha Levin wrote: > On Fri, Nov 11, 2011 at 6:24 AM, Rusty Russell wrote: > > On Wed, 09 Nov 2011 22:57:28 +0200, Sasha Levin > > wrote: > >> On Wed, 2011-11-09 at 22:52 +0200, Michael S. Tsirkin wrote: > >> > On Wed, Nov 09, 2011 at 10:24:47PM +0200, Sasha Levin wrote: > >> > > It'll be a bit harder deprecating it in the future. > >> > > >> > Harder than ... what ? > >> > >> Harder than allowing devices not to present it at all if new layout > >> config is used. Right now the simple implementation is to use MMIO for > >> config and device specific, and let it fallback to legacy for ISR and > >> notifications (and therefore, this is probably how everybody will > >> implement it), which means that when you do want to deprecate legacy, > >> there will be extra work to be done then, instead of doing it now. > > > > Indeed, I'd like to see two changes to your proposal: > > > > (1) It should be all or nothing. If a driver can find the virtio header > > capability, it should only use the capabilties. Otherwise, it > > should fall back to legacy. Your draft suggests a mix is possible; > > I prefer a clean failure (ie. one day don't present a BAR 0 *at > > all*, so ancient drivers just fail to load.). > > > > (2) There's no huge win in keeping the same layout. Let's make some > > cleanups. There are more users ahead of us then behind us (I > > hope!). > > Actually, if we already do cleanups, here are two more suggestions: > > 1. Make 64bit features a one big 64bit block, instead of having 32bits > in one place and 32 in another. > 2. Remove the reserved fields out of the config (the ones that were > caused by moving the ISR and the notifications out). > > > But I think this is the right direction! > > > > Thanks, > > Rusty. > > > > Also, an unrelated questions: With PIO, requests were ordered, which > means that if we wrote to the queue selector and then read from a > queue register we would read the correct queue info. > Is the same thing assured to us with MMIO? For real PCI, reads do not bypass writes in PCI. However this is only true if both are MMIO or both PIO reads. I don't think the ordering of MMIO versus PIO is guaranteed. On KVM, the kernel doesn't do anything to guarantee ordering. So you get the natural ordering of the CPU. > If we write to a queue > selector and immediately read from queue info would we be reading the > right info, or is there the slight chance that it would get reordered > and we would be reading queue info first and writing to the selector > later? The thing to realize is that write to queue selector with KVM is in the end performed by host. And reading queue info means that host will be reading the queue selector. So this is a write followed by read from the same address. AFAIK no CPUs can reorder such accesses, so you get the right info. -- MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Fri, Nov 11, 2011 at 02:54:31PM +1030, Rusty Russell wrote: > On Wed, 09 Nov 2011 22:57:28 +0200, Sasha Levin > wrote: > > On Wed, 2011-11-09 at 22:52 +0200, Michael S. Tsirkin wrote: > > > On Wed, Nov 09, 2011 at 10:24:47PM +0200, Sasha Levin wrote: > > > > It'll be a bit harder deprecating it in the future. > > > > > > Harder than ... what ? > > > > Harder than allowing devices not to present it at all if new layout > > config is used. Right now the simple implementation is to use MMIO for > > config and device specific, and let it fallback to legacy for ISR and > > notifications (and therefore, this is probably how everybody will > > implement it), which means that when you do want to deprecate legacy, > > there will be extra work to be done then, instead of doing it now. > > Indeed, I'd like to see two changes to your proposal: > > (1) It should be all or nothing. If a driver can find the virtio header > capability, it should only use the capabilties. Otherwise, it > should fall back to legacy. Your draft suggests a mix is possible; > I prefer a clean failure (ie. one day don't present a BAR 0 *at > all*, so ancient drivers just fail to load.). > (2) There's no huge win in keeping the same layout. Let's make some > cleanups. There are more users ahead of us then behind us (I > hope!). > But I think this is the right direction! > > Thanks, > Rusty. I'll do these changes, thanks! -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Fri, Nov 11, 2011 at 6:24 AM, Rusty Russell wrote: > On Wed, 09 Nov 2011 22:57:28 +0200, Sasha Levin > wrote: >> On Wed, 2011-11-09 at 22:52 +0200, Michael S. Tsirkin wrote: >> > On Wed, Nov 09, 2011 at 10:24:47PM +0200, Sasha Levin wrote: >> > > It'll be a bit harder deprecating it in the future. >> > >> > Harder than ... what ? >> >> Harder than allowing devices not to present it at all if new layout >> config is used. Right now the simple implementation is to use MMIO for >> config and device specific, and let it fallback to legacy for ISR and >> notifications (and therefore, this is probably how everybody will >> implement it), which means that when you do want to deprecate legacy, >> there will be extra work to be done then, instead of doing it now. > > Indeed, I'd like to see two changes to your proposal: > > (1) It should be all or nothing. If a driver can find the virtio header > capability, it should only use the capabilties. Otherwise, it > should fall back to legacy. Your draft suggests a mix is possible; > I prefer a clean failure (ie. one day don't present a BAR 0 *at > all*, so ancient drivers just fail to load.). > > (2) There's no huge win in keeping the same layout. Let's make some > cleanups. There are more users ahead of us then behind us (I > hope!). Actually, if we already do cleanups, here are two more suggestions: 1. Make 64bit features a one big 64bit block, instead of having 32bits in one place and 32 in another. 2. Remove the reserved fields out of the config (the ones that were caused by moving the ISR and the notifications out). > But I think this is the right direction! > > Thanks, > Rusty. > Also, an unrelated questions: With PIO, requests were ordered, which means that if we wrote to the queue selector and then read from a queue register we would read the correct queue info. Is the same thing assured to us with MMIO? If we write to a queue selector and immediately read from queue info would we be reading the right info, or is there the slight chance that it would get reordered and we would be reading queue info first and writing to the selector later? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Wed, 09 Nov 2011 22:57:28 +0200, Sasha Levin wrote: > On Wed, 2011-11-09 at 22:52 +0200, Michael S. Tsirkin wrote: > > On Wed, Nov 09, 2011 at 10:24:47PM +0200, Sasha Levin wrote: > > > It'll be a bit harder deprecating it in the future. > > > > Harder than ... what ? > > Harder than allowing devices not to present it at all if new layout > config is used. Right now the simple implementation is to use MMIO for > config and device specific, and let it fallback to legacy for ISR and > notifications (and therefore, this is probably how everybody will > implement it), which means that when you do want to deprecate legacy, > there will be extra work to be done then, instead of doing it now. Indeed, I'd like to see two changes to your proposal: (1) It should be all or nothing. If a driver can find the virtio header capability, it should only use the capabilties. Otherwise, it should fall back to legacy. Your draft suggests a mix is possible; I prefer a clean failure (ie. one day don't present a BAR 0 *at all*, so ancient drivers just fail to load.). (2) There's no huge win in keeping the same layout. Let's make some cleanups. There are more users ahead of us then behind us (I hope!). But I think this is the right direction! Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Wed, Nov 09, 2011 at 11:13:56PM +0200, Sasha Levin wrote: > On Wed, 2011-11-09 at 23:14 +0200, Michael S. Tsirkin wrote: > > On Wed, Nov 09, 2011 at 10:57:28PM +0200, Sasha Levin wrote: > > > On Wed, 2011-11-09 at 22:52 +0200, Michael S. Tsirkin wrote: > > > > On Wed, Nov 09, 2011 at 10:24:47PM +0200, Sasha Levin wrote: > > > > > On Wed, 2011-11-09 at 21:59 +0200, Michael S. Tsirkin wrote: > > > > > > > > > > [snip] > > > > > > > > > > > +\begin_layout Enumerate > > > > > > +Reset the device. > > > > > > + This is not required on initial start up. > > > > > > +\end_layout > > > > > > + > > > > > > +\begin_layout Enumerate > > > > > > +The ACKNOWLEDGE status bit is set: we have noticed the device. > > > > > > +\end_layout > > > > > > + > > > > > > +\begin_layout Enumerate > > > > > > +The DRIVER status bit is set: we know how to drive the device. > > > > > > +\end_layout > > > > > > + > > > > > > +\begin_layout Enumerate > > > > > > + > > > > > > +\change_inserted 1986246365 1320838089 > > > > > > +PCI capability list scan, detecting virtio configuration layout > > > > > > using Virtio > > > > > > + Structure PCI capabilities. > > > > > > > > > > Does the legacy space always gets mapped from BAR0? > > > > > > > > > > If yes, > > > > > > > > Yes and this is repeated in several places. Not clear? How can this > > > > be made clearer? > > > > > > Do you mean comments such as "For backwards compatibility, devices > > > should also present legacy configuration space in the first I/O region > > > of the PCI device"? What I understood from it is that the device should > > > have a legacy config in case it's used with an older guest, but I didn't > > > understand from it that the legacy config will be used even if new > > > layout is present. > > > > Yes, this is what I meant. New guest is required to use the new space > > and not the legacy one. So you dont need a legacy space for the at all. > > But practically, we'll need to support old guests for a long while. > > > > > > > It'll be a bit harder deprecating it in the future. > > > > > > > > Harder than ... what ? > > > > > > Harder than allowing devices not to present it at all if new layout > > > config is used. > > > > Yes, it's allowed if you know you have a new guest. It says > > explicitly that drivers are required to use new capabilities > > is they are there. > > > > > Right now the simple implementation is to use MMIO for > > > config and device specific, and let it fallback to legacy for ISR and > > > notifications (and therefore, this is probably how everybody will > > > implement it), which means that when you do want to deprecate legacy, > > > there will be extra work to be done then, instead of doing it now. > > > > If hypervisors don't implement the new layout then drivers will > > have to keep supporting the old one. I don't think we can do > > much about that. > > > > > > IMO there's no way to put legacy anywhere except the first BAR > > > > without breaking existing guests. > > > > > > It's not about where we put legacy, it's about how easy it is to drop > > > legacy entirely. > > > > We can only do this after all guests and hypervisors are updated. When > > they are, we can drop legacy from drivers and hypervisors, and > > I don't see a way to make it easier. > > Well, in that case, why does the PCI cap probing is #4 in the device > init list? Shouldn't we getting the layout and mapping it before we > write to the status byte? True, this is actually how it's done in the driver. Good catch, I'll correct the text, thanks. > -- > > Sasha. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Wed, 2011-11-09 at 23:14 +0200, Michael S. Tsirkin wrote: > On Wed, Nov 09, 2011 at 10:57:28PM +0200, Sasha Levin wrote: > > On Wed, 2011-11-09 at 22:52 +0200, Michael S. Tsirkin wrote: > > > On Wed, Nov 09, 2011 at 10:24:47PM +0200, Sasha Levin wrote: > > > > On Wed, 2011-11-09 at 21:59 +0200, Michael S. Tsirkin wrote: > > > > > > > > [snip] > > > > > > > > > +\begin_layout Enumerate > > > > > +Reset the device. > > > > > + This is not required on initial start up. > > > > > +\end_layout > > > > > + > > > > > +\begin_layout Enumerate > > > > > +The ACKNOWLEDGE status bit is set: we have noticed the device. > > > > > +\end_layout > > > > > + > > > > > +\begin_layout Enumerate > > > > > +The DRIVER status bit is set: we know how to drive the device. > > > > > +\end_layout > > > > > + > > > > > +\begin_layout Enumerate > > > > > + > > > > > +\change_inserted 1986246365 1320838089 > > > > > +PCI capability list scan, detecting virtio configuration layout > > > > > using Virtio > > > > > + Structure PCI capabilities. > > > > > > > > Does the legacy space always gets mapped from BAR0? > > > > > > > > If yes, > > > > > > Yes and this is repeated in several places. Not clear? How can this > > > be made clearer? > > > > Do you mean comments such as "For backwards compatibility, devices > > should also present legacy configuration space in the first I/O region > > of the PCI device"? What I understood from it is that the device should > > have a legacy config in case it's used with an older guest, but I didn't > > understand from it that the legacy config will be used even if new > > layout is present. > > Yes, this is what I meant. New guest is required to use the new space > and not the legacy one. So you dont need a legacy space for the at all. > But practically, we'll need to support old guests for a long while. > > > > > It'll be a bit harder deprecating it in the future. > > > > > > Harder than ... what ? > > > > Harder than allowing devices not to present it at all if new layout > > config is used. > > Yes, it's allowed if you know you have a new guest. It says > explicitly that drivers are required to use new capabilities > is they are there. > > > Right now the simple implementation is to use MMIO for > > config and device specific, and let it fallback to legacy for ISR and > > notifications (and therefore, this is probably how everybody will > > implement it), which means that when you do want to deprecate legacy, > > there will be extra work to be done then, instead of doing it now. > > If hypervisors don't implement the new layout then drivers will > have to keep supporting the old one. I don't think we can do > much about that. > > > > IMO there's no way to put legacy anywhere except the first BAR > > > without breaking existing guests. > > > > It's not about where we put legacy, it's about how easy it is to drop > > legacy entirely. > > We can only do this after all guests and hypervisors are updated. When > they are, we can drop legacy from drivers and hypervisors, and > I don't see a way to make it easier. Well, in that case, why does the PCI cap probing is #4 in the device init list? Shouldn't we getting the layout and mapping it before we write to the status byte? -- Sasha. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Wed, Nov 09, 2011 at 10:57:28PM +0200, Sasha Levin wrote: > On Wed, 2011-11-09 at 22:52 +0200, Michael S. Tsirkin wrote: > > On Wed, Nov 09, 2011 at 10:24:47PM +0200, Sasha Levin wrote: > > > On Wed, 2011-11-09 at 21:59 +0200, Michael S. Tsirkin wrote: > > > > > > [snip] > > > > > > > +\begin_layout Enumerate > > > > +Reset the device. > > > > + This is not required on initial start up. > > > > +\end_layout > > > > + > > > > +\begin_layout Enumerate > > > > +The ACKNOWLEDGE status bit is set: we have noticed the device. > > > > +\end_layout > > > > + > > > > +\begin_layout Enumerate > > > > +The DRIVER status bit is set: we know how to drive the device. > > > > +\end_layout > > > > + > > > > +\begin_layout Enumerate > > > > + > > > > +\change_inserted 1986246365 1320838089 > > > > +PCI capability list scan, detecting virtio configuration layout using > > > > Virtio > > > > + Structure PCI capabilities. > > > > > > Does the legacy space always gets mapped from BAR0? > > > > > > If yes, > > > > Yes and this is repeated in several places. Not clear? How can this > > be made clearer? > > Do you mean comments such as "For backwards compatibility, devices > should also present legacy configuration space in the first I/O region > of the PCI device"? What I understood from it is that the device should > have a legacy config in case it's used with an older guest, but I didn't > understand from it that the legacy config will be used even if new > layout is present. Yes, this is what I meant. New guest is required to use the new space and not the legacy one. So you dont need a legacy space for the at all. But practically, we'll need to support old guests for a long while. > > > It'll be a bit harder deprecating it in the future. > > > > Harder than ... what ? > > Harder than allowing devices not to present it at all if new layout > config is used. Yes, it's allowed if you know you have a new guest. It says explicitly that drivers are required to use new capabilities is they are there. > Right now the simple implementation is to use MMIO for > config and device specific, and let it fallback to legacy for ISR and > notifications (and therefore, this is probably how everybody will > implement it), which means that when you do want to deprecate legacy, > there will be extra work to be done then, instead of doing it now. If hypervisors don't implement the new layout then drivers will have to keep supporting the old one. I don't think we can do much about that. > > IMO there's no way to put legacy anywhere except the first BAR > > without breaking existing guests. > > It's not about where we put legacy, it's about how easy it is to drop > legacy entirely. We can only do this after all guests and hypervisors are updated. When they are, we can drop legacy from drivers and hypervisors, and I don't see a way to make it easier. > -- > > Sasha. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Wed, 2011-11-09 at 22:52 +0200, Michael S. Tsirkin wrote: > On Wed, Nov 09, 2011 at 10:24:47PM +0200, Sasha Levin wrote: > > On Wed, 2011-11-09 at 21:59 +0200, Michael S. Tsirkin wrote: > > > > [snip] > > > > > +\begin_layout Enumerate > > > +Reset the device. > > > + This is not required on initial start up. > > > +\end_layout > > > + > > > +\begin_layout Enumerate > > > +The ACKNOWLEDGE status bit is set: we have noticed the device. > > > +\end_layout > > > + > > > +\begin_layout Enumerate > > > +The DRIVER status bit is set: we know how to drive the device. > > > +\end_layout > > > + > > > +\begin_layout Enumerate > > > + > > > +\change_inserted 1986246365 1320838089 > > > +PCI capability list scan, detecting virtio configuration layout using > > > Virtio > > > + Structure PCI capabilities. > > > > Does the legacy space always gets mapped from BAR0? > > > > If yes, > > Yes and this is repeated in several places. Not clear? How can this > be made clearer? Do you mean comments such as "For backwards compatibility, devices should also present legacy configuration space in the first I/O region of the PCI device"? What I understood from it is that the device should have a legacy config in case it's used with an older guest, but I didn't understand from it that the legacy config will be used even if new layout is present. > > It'll be a bit harder deprecating it in the future. > > Harder than ... what ? Harder than allowing devices not to present it at all if new layout config is used. Right now the simple implementation is to use MMIO for config and device specific, and let it fallback to legacy for ISR and notifications (and therefore, this is probably how everybody will implement it), which means that when you do want to deprecate legacy, there will be extra work to be done then, instead of doing it now. > IMO there's no way to put legacy anywhere except the first BAR > without breaking existing guests. It's not about where we put legacy, it's about how easy it is to drop legacy entirely. -- Sasha. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Wed, Nov 09, 2011 at 10:24:47PM +0200, Sasha Levin wrote: > On Wed, 2011-11-09 at 21:59 +0200, Michael S. Tsirkin wrote: > > [snip] > > > +\begin_layout Enumerate > > +Reset the device. > > + This is not required on initial start up. > > +\end_layout > > + > > +\begin_layout Enumerate > > +The ACKNOWLEDGE status bit is set: we have noticed the device. > > +\end_layout > > + > > +\begin_layout Enumerate > > +The DRIVER status bit is set: we know how to drive the device. > > +\end_layout > > + > > +\begin_layout Enumerate > > + > > +\change_inserted 1986246365 1320838089 > > +PCI capability list scan, detecting virtio configuration layout using > > Virtio > > + Structure PCI capabilities. > > Does the legacy space always gets mapped from BAR0? > > If yes, Yes and this is repeated in several places. Not clear? How can this be made clearer? > It'll be a bit harder deprecating it in the future. Harder than ... what ? IMO there's no way to put legacy anywhere except the first BAR without breaking existing guests. > If not, this should actually be #1. Previous 3 bullets here were already > setting the status byte in the device config. > > [snip] > > > + > usebox="none"> > > +\begin_inset Text > > + > > +\begin_layout Plain Layout > > + > > +\change_inserted 1986246365 1320786225 > > +28 bytes > > This is the new virtio header min size, shouldn't it be 32? (20 + 4 > (msi-x) + 4 (guest feature) + 4 (device features)). Good catch, I'll correct this. > -- > > Sasha. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 RFC] virtio-spec: flexible configuration layout
On Wed, 2011-11-09 at 21:59 +0200, Michael S. Tsirkin wrote: [snip] > +\begin_layout Enumerate > +Reset the device. > + This is not required on initial start up. > +\end_layout > + > +\begin_layout Enumerate > +The ACKNOWLEDGE status bit is set: we have noticed the device. > +\end_layout > + > +\begin_layout Enumerate > +The DRIVER status bit is set: we know how to drive the device. > +\end_layout > + > +\begin_layout Enumerate > + > +\change_inserted 1986246365 1320838089 > +PCI capability list scan, detecting virtio configuration layout using Virtio > + Structure PCI capabilities. Does the legacy space always gets mapped from BAR0? If yes, It'll be a bit harder deprecating it in the future. If not, this should actually be #1. Previous 3 bullets here were already setting the status byte in the device config. [snip] > + usebox="none"> > +\begin_inset Text > + > +\begin_layout Plain Layout > + > +\change_inserted 1986246365 1320786225 > +28 bytes This is the new virtio header min size, shouldn't it be 32? (20 + 4 (msi-x) + 4 (guest feature) + 4 (device features)). -- Sasha. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
