Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X
On Sun, 2 Oct 2011 11:09:00 +0200, "Michael S. Tsirkin" wrote: > On Mon, Sep 19, 2011 at 05:19:49PM +0930, Rusty Russell wrote: > > On Mon, 19 Sep 2011 09:01:50 +0300, "Michael S. Tsirkin" > > wrote: > > > On Mon, Sep 19, 2011 at 01:05:17PM +0930, Rusty Russell wrote: > > > > On Sat, 20 Aug 2011 23:00:44 +0300, "Michael S. Tsirkin" > > > > wrote: > > > > > On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote: > > > > > > Maybe this is better solved by copying the way it was done in PCI > > > > > > itself > > > > > > with capability linked list? > > > > > > > > > > There are any number of ways to lay out the structure. I went for > > > > > what > > > > > seemed a simplest one. For MSI-X the train has left the station. We > > > > > can probably still tweak where the high 32 bit features > > > > > for 64 bit features are. No idea if it's worth it. > > > > > > > > Sorry, this has been in the back of my mind. I think it's a good idea; > > > > can we use the capability linked list for pre-device specific stuff from > > > > now on? > > > > > > > > Thanks, > > > > Rusty. > > > > > > Do we even want capability bits then? > > > We can give each capability an ack flag ... > > > > We could have, and if I'd known PCI when I designed virtio I might have. > > > > But it's not easy now to map structure offsets to that scheme, and we > > can't really force such a change on the non-PCI users. So I'd say we > > should only do it for the non-device-specific options. ie. we'll still > > have the MSI-X case move the device-specific config, but we'll use a > > linked list from now on, eg. for the next 32 features bits... > > > > Thoughts? > > Rusty. > > So I thought about this some more. It probably makes sense to > stop adding info in the io space, and start adding new stuff in > memory space which is much less contrained. > > As we need to keep compatibility, and also because memory access is > much slower with kvm so we prefer io for datapath, we could have the > first X bytes still mirrored in io space. > > MSIX is there, and it's pointed to from config space, > so we could just put our stuff at offset 0. > > Or if we wanted a lot of flexibility, we could add pointers, in config > space, to device specific and non device specific regions in memory > space. > > Makes sense? I've cc'd David Gibson who has noted in the past that I/O space on PPC is icky. I think a linked list for future virtio-pci extensions makes sense (not device-specific stuff, that's great as a simple structure). I think mirroring everything in mem space makes sense, too. 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: [PATCH] virtio-net: Read MAC only after initializing MSI-X
On Wed, Sep 28, 2011 at 09:30:51PM +0300, Sasha Levin wrote: > On Mon, 2011-09-19 at 17:19 +0930, Rusty Russell wrote: > > On Mon, 19 Sep 2011 09:01:50 +0300, "Michael S. Tsirkin" > > wrote: > > > On Mon, Sep 19, 2011 at 01:05:17PM +0930, Rusty Russell wrote: > > > > On Sat, 20 Aug 2011 23:00:44 +0300, "Michael S. Tsirkin" > > > > wrote: > > > > > On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote: > > > > > > Maybe this is better solved by copying the way it was done in PCI > > > > > > itself > > > > > > with capability linked list? > > > > > > > > > > There are any number of ways to lay out the structure. I went for > > > > > what > > > > > seemed a simplest one. For MSI-X the train has left the station. We > > > > > can probably still tweak where the high 32 bit features > > > > > for 64 bit features are. No idea if it's worth it. > > > > > > > > Sorry, this has been in the back of my mind. I think it's a good idea; > > > > can we use the capability linked list for pre-device specific stuff from > > > > now on? > > > > > > > > Thanks, > > > > Rusty. > > > > > > Do we even want capability bits then? > > > We can give each capability an ack flag ... > > > > We could have, and if I'd known PCI when I designed virtio I might have. > > > > But it's not easy now to map structure offsets to that scheme, and we > > can't really force such a change on the non-PCI users. So I'd say we > > should only do it for the non-device-specific options. ie. we'll still > > have the MSI-X case move the device-specific config, but we'll use a > > linked list from now on, eg. for the next 32 features bits... > > > > Thoughts? > > Rusty. > > What if we create a capability list but place it in the virtio-pci > config space instead of the PCI space? Pls note that virtio-pci config space is io so it is very constrained, we do need to pack it densely. If we want to add a lot of stuff there we probably should move it to memory space. It's slower than io on kvm, but most uses of it aren't on data path. > It'll work fine with non-PCI users and would leave MSI-X as the only > thing that changes offsets (and we could probably deprecate and remove > it at some point in the future). > > -- > > 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: [PATCH] virtio-net: Read MAC only after initializing MSI-X
On Mon, Sep 19, 2011 at 05:19:49PM +0930, Rusty Russell wrote: > On Mon, 19 Sep 2011 09:01:50 +0300, "Michael S. Tsirkin" > wrote: > > On Mon, Sep 19, 2011 at 01:05:17PM +0930, Rusty Russell wrote: > > > On Sat, 20 Aug 2011 23:00:44 +0300, "Michael S. Tsirkin" > > > wrote: > > > > On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote: > > > > > Maybe this is better solved by copying the way it was done in PCI > > > > > itself > > > > > with capability linked list? > > > > > > > > There are any number of ways to lay out the structure. I went for what > > > > seemed a simplest one. For MSI-X the train has left the station. We > > > > can probably still tweak where the high 32 bit features > > > > for 64 bit features are. No idea if it's worth it. > > > > > > Sorry, this has been in the back of my mind. I think it's a good idea; > > > can we use the capability linked list for pre-device specific stuff from > > > now on? > > > > > > Thanks, > > > Rusty. > > > > Do we even want capability bits then? > > We can give each capability an ack flag ... > > We could have, and if I'd known PCI when I designed virtio I might have. > > But it's not easy now to map structure offsets to that scheme, and we > can't really force such a change on the non-PCI users. So I'd say we > should only do it for the non-device-specific options. ie. we'll still > have the MSI-X case move the device-specific config, but we'll use a > linked list from now on, eg. for the next 32 features bits... > > Thoughts? > Rusty. So I thought about this some more. It probably makes sense to stop adding info in the io space, and start adding new stuff in memory space which is much less contrained. As we need to keep compatibility, and also because memory access is much slower with kvm so we prefer io for datapath, we could have the first X bytes still mirrored in io space. MSIX is there, and it's pointed to from config space, so we could just put our stuff at offset 0. Or if we wanted a lot of flexibility, we could add pointers, in config space, to device specific and non device specific regions in memory space. Makes sense? -- 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: [PATCH] virtio-net: Read MAC only after initializing MSI-X
On Mon, 2011-09-19 at 17:19 +0930, Rusty Russell wrote: > On Mon, 19 Sep 2011 09:01:50 +0300, "Michael S. Tsirkin" > wrote: > > On Mon, Sep 19, 2011 at 01:05:17PM +0930, Rusty Russell wrote: > > > On Sat, 20 Aug 2011 23:00:44 +0300, "Michael S. Tsirkin" > > > wrote: > > > > On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote: > > > > > Maybe this is better solved by copying the way it was done in PCI > > > > > itself > > > > > with capability linked list? > > > > > > > > There are any number of ways to lay out the structure. I went for what > > > > seemed a simplest one. For MSI-X the train has left the station. We > > > > can probably still tweak where the high 32 bit features > > > > for 64 bit features are. No idea if it's worth it. > > > > > > Sorry, this has been in the back of my mind. I think it's a good idea; > > > can we use the capability linked list for pre-device specific stuff from > > > now on? > > > > > > Thanks, > > > Rusty. > > > > Do we even want capability bits then? > > We can give each capability an ack flag ... > > We could have, and if I'd known PCI when I designed virtio I might have. > > But it's not easy now to map structure offsets to that scheme, and we > can't really force such a change on the non-PCI users. So I'd say we > should only do it for the non-device-specific options. ie. we'll still > have the MSI-X case move the device-specific config, but we'll use a > linked list from now on, eg. for the next 32 features bits... > > Thoughts? > Rusty. What if we create a capability list but place it in the virtio-pci config space instead of the PCI space? It'll work fine with non-PCI users and would leave MSI-X as the only thing that changes offsets (and we could probably deprecate and remove it at some point in the future). -- 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: [PATCH] virtio-net: Read MAC only after initializing MSI-X
On Mon, 19 Sep 2011 09:01:50 +0300, "Michael S. Tsirkin" wrote: > On Mon, Sep 19, 2011 at 01:05:17PM +0930, Rusty Russell wrote: > > On Sat, 20 Aug 2011 23:00:44 +0300, "Michael S. Tsirkin" > > wrote: > > > On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote: > > > > Maybe this is better solved by copying the way it was done in PCI itself > > > > with capability linked list? > > > > > > There are any number of ways to lay out the structure. I went for what > > > seemed a simplest one. For MSI-X the train has left the station. We > > > can probably still tweak where the high 32 bit features > > > for 64 bit features are. No idea if it's worth it. > > > > Sorry, this has been in the back of my mind. I think it's a good idea; > > can we use the capability linked list for pre-device specific stuff from > > now on? > > > > Thanks, > > Rusty. > > Do we even want capability bits then? > We can give each capability an ack flag ... We could have, and if I'd known PCI when I designed virtio I might have. But it's not easy now to map structure offsets to that scheme, and we can't really force such a change on the non-PCI users. So I'd say we should only do it for the non-device-specific options. ie. we'll still have the MSI-X case move the device-specific config, but we'll use a linked list from now on, eg. for the next 32 features bits... Thoughts? 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: [PATCH] virtio-net: Read MAC only after initializing MSI-X
On Mon, Sep 19, 2011 at 01:05:17PM +0930, Rusty Russell wrote: > On Sat, 20 Aug 2011 23:00:44 +0300, "Michael S. Tsirkin" > wrote: > > On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote: > > > Maybe this is better solved by copying the way it was done in PCI itself > > > with capability linked list? > > > > There are any number of ways to lay out the structure. I went for what > > seemed a simplest one. For MSI-X the train has left the station. We > > can probably still tweak where the high 32 bit features > > for 64 bit features are. No idea if it's worth it. > > Sorry, this has been in the back of my mind. I think it's a good idea; > can we use the capability linked list for pre-device specific stuff from > now on? > > Thanks, > Rusty. Do we even want capability bits then? We can give each capability an ack flag ... -- 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: [PATCH] virtio-net: Read MAC only after initializing MSI-X
On Sat, 20 Aug 2011 23:00:44 +0300, "Michael S. Tsirkin" wrote: > On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote: > > Maybe this is better solved by copying the way it was done in PCI itself > > with capability linked list? > > There are any number of ways to lay out the structure. I went for what > seemed a simplest one. For MSI-X the train has left the station. We > can probably still tweak where the high 32 bit features > for 64 bit features are. No idea if it's worth it. Sorry, this has been in the back of my mind. I think it's a good idea; can we use the capability linked list for pre-device specific stuff from now on? 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: [PATCH] virtio-net: Read MAC only after initializing MSI-X
I'm wondering if we can switch to using a linked list 'capabilities' structure similar to whats being done with PCI capabilities. Here are the pros and the cons as I see them: Pros: * Simpler code - currently before each access to the virtio config space we have to check whether MSI-X is on and whether the device has 64bit features. This isn't necessarily slow, but it complicates the code. * Future proof - code will be a mess once 5-6 features can change the config space. * 'Concept reuse' - using same concepts in virtio-pci as the ones used in PCI ('PCI Capabilities list') would make it easier to understand, and would implement the same method to use optional features as in the layer above. Cons: * MSI-X is already moving the config space, we'll need to keep supporting it for a while, but it would mean that it's the only thing we need to keep backwards compatible. * 64bit features also move config space, but they're brand new in the spec and aren't implemented in the kernel yet - I doubt anyone implemented it anywhere else yet. On Mon, 2011-08-22 at 11:36 +0300, Michael S. Tsirkin wrote: > On Mon, Aug 22, 2011 at 09:54:50AM +0930, Rusty Russell wrote: > > On Fri, 19 Aug 2011 18:23:35 +0300, "Michael S. Tsirkin" > > wrote: > > > On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote: > > > > The MAC of a virtio-net device is located at the first field of the > > > > device > > > > specific header. This header is located at offset 20 if the device > > > > doesn't > > > > support MSI-X or offset 24 if it does. > > > > > > > > Current code in virtnet_probe() used to probe the MAC before checking > > > > for > > > > MSI-X, which means that the read was always made from offset 20 > > > > regardless > > > > of whether MSI-X in enabled or not. > > > > > > > > This patch moves the MAC probe to after the detection of whether MSI-X > > > > is > > > > enabled. This way the MAC will be read from offset 24 if the device > > > > indeed > > > > supports MSI-X. > > > > > > > > Cc: Rusty Russell > > > > Cc: Michael S. Tsirkin > > > > Cc: virtualizat...@lists.linux-foundation.org > > > > Cc: net...@vger.kernel.org > > > > Cc: kvm@vger.kernel.org > > > > Signed-off-by: Sasha Levin > > > > > > I am not sure I see a bug in virtio: the config pace layout simply > > > changes as msix is enabled and disabled (and if you look at the latest > > > draft, also on whether 64 bit features are enabled). > > > It doesn't depend on msix capability being present in device. > > > > > > The spec seems to be explicit enough: > > > If MSI-X is enabled for the device, two additional fields immediately > > > follow this header. > > > > > > So I'm guessing the bug is in kvm tools which assume > > > same layout for when msix is enabled and disabled. > > > qemu-kvm seems to do the right thing so the device > > > seems to get the correct mac. > > > > So, the config space moves once MSI-X is enabled? In which case, it > > should say "ONCE MSI-X is enabled..." > > > > Thanks, > > Rusty. > > Yes. Or maybe 'WHEN' - since if MSI-X is disabled again, it moves back. > Let's update the spec to make it clearer? > -- 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: [PATCH] virtio-net: Read MAC only after initializing MSI-X
On Mon, 22 Aug 2011 11:36:13 +0300, "Michael S. Tsirkin" wrote: > Yes. Or maybe 'WHEN' - since if MSI-X is disabled again, it moves back. > Let's update the spec to make it clearer? I left the language as is, but added a footnote at the end of the sentence: If MSI-X is enabled for the device, two additional fields immediately follow this header: [footnote: ie. once you enable MSI-X on the device, the other fields move. If you turn it off again, they move back!] 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: [PATCH] virtio-net: Read MAC only after initializing MSI-X
On Mon, Aug 22, 2011 at 09:54:50AM +0930, Rusty Russell wrote: > On Fri, 19 Aug 2011 18:23:35 +0300, "Michael S. Tsirkin" > wrote: > > On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote: > > > The MAC of a virtio-net device is located at the first field of the device > > > specific header. This header is located at offset 20 if the device doesn't > > > support MSI-X or offset 24 if it does. > > > > > > Current code in virtnet_probe() used to probe the MAC before checking for > > > MSI-X, which means that the read was always made from offset 20 regardless > > > of whether MSI-X in enabled or not. > > > > > > This patch moves the MAC probe to after the detection of whether MSI-X is > > > enabled. This way the MAC will be read from offset 24 if the device indeed > > > supports MSI-X. > > > > > > Cc: Rusty Russell > > > Cc: Michael S. Tsirkin > > > Cc: virtualizat...@lists.linux-foundation.org > > > Cc: net...@vger.kernel.org > > > Cc: kvm@vger.kernel.org > > > Signed-off-by: Sasha Levin > > > > I am not sure I see a bug in virtio: the config pace layout simply > > changes as msix is enabled and disabled (and if you look at the latest > > draft, also on whether 64 bit features are enabled). > > It doesn't depend on msix capability being present in device. > > > > The spec seems to be explicit enough: > > If MSI-X is enabled for the device, two additional fields immediately > > follow this header. > > > > So I'm guessing the bug is in kvm tools which assume > > same layout for when msix is enabled and disabled. > > qemu-kvm seems to do the right thing so the device > > seems to get the correct mac. > > So, the config space moves once MSI-X is enabled? In which case, it > should say "ONCE MSI-X is enabled..." > > Thanks, > Rusty. Yes. Or maybe 'WHEN' - since if MSI-X is disabled again, it moves back. Let's update the spec to make it clearer? -- 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: [PATCH] virtio-net: Read MAC only after initializing MSI-X
On Fri, 19 Aug 2011 18:23:35 +0300, "Michael S. Tsirkin" wrote: > On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote: > > The MAC of a virtio-net device is located at the first field of the device > > specific header. This header is located at offset 20 if the device doesn't > > support MSI-X or offset 24 if it does. > > > > Current code in virtnet_probe() used to probe the MAC before checking for > > MSI-X, which means that the read was always made from offset 20 regardless > > of whether MSI-X in enabled or not. > > > > This patch moves the MAC probe to after the detection of whether MSI-X is > > enabled. This way the MAC will be read from offset 24 if the device indeed > > supports MSI-X. > > > > Cc: Rusty Russell > > Cc: Michael S. Tsirkin > > Cc: virtualizat...@lists.linux-foundation.org > > Cc: net...@vger.kernel.org > > Cc: kvm@vger.kernel.org > > Signed-off-by: Sasha Levin > > I am not sure I see a bug in virtio: the config pace layout simply > changes as msix is enabled and disabled (and if you look at the latest > draft, also on whether 64 bit features are enabled). > It doesn't depend on msix capability being present in device. > > The spec seems to be explicit enough: > If MSI-X is enabled for the device, two additional fields immediately > follow this header. > > So I'm guessing the bug is in kvm tools which assume > same layout for when msix is enabled and disabled. > qemu-kvm seems to do the right thing so the device > seems to get the correct mac. So, the config space moves once MSI-X is enabled? In which case, it should say "ONCE MSI-X is enabled..." 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: [PATCH] virtio-net: Read MAC only after initializing MSI-X
On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote: > On Fri, 2011-08-19 at 18:23 +0300, Michael S. Tsirkin wrote: > > On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote: > > > The MAC of a virtio-net device is located at the first field of the device > > > specific header. This header is located at offset 20 if the device doesn't > > > support MSI-X or offset 24 if it does. > > > > > > Current code in virtnet_probe() used to probe the MAC before checking for > > > MSI-X, which means that the read was always made from offset 20 regardless > > > of whether MSI-X in enabled or not. > > > > > > This patch moves the MAC probe to after the detection of whether MSI-X is > > > enabled. This way the MAC will be read from offset 24 if the device indeed > > > supports MSI-X. > > > > > > Cc: Rusty Russell > > > Cc: Michael S. Tsirkin > > > Cc: virtualizat...@lists.linux-foundation.org > > > Cc: net...@vger.kernel.org > > > Cc: kvm@vger.kernel.org > > > Signed-off-by: Sasha Levin > > > > I am not sure I see a bug in virtio: the config pace layout simply > > changes as msix is enabled and disabled (and if you look at the latest > > draft, also on whether 64 bit features are enabled). > > It doesn't depend on msix capability being present in device. > > > > The spec seems to be explicit enough: > > If MSI-X is enabled for the device, two additional fields immediately > > follow this header. > > > > So I'm guessing the bug is in kvm tools which assume > > same layout for when msix is enabled and disabled. > > qemu-kvm seems to do the right thing so the device > > seems to get the correct mac. > > We assumed that PCI config space has a static layout like most other > devices. Having a behavior of "First bit 20 does something, but after > enabling MSI-X it does something completely different" sounds strange. The layout is always virtio header followed by device specific header. We started with a small header so when more data was added, we could not extend the header unconditionally. We can't change that behaviour for MSI-X now, guests and hosts rely on it. > > I'm wondering why offsets of the config structure change during run time > and are not statically defined when the device is started. That's because of backwards compatibility with old guests. When we know the guest is new, we expose new layout, but old guests must see old layout. > It's not like VIRTIO_F_FEATURES_HI can be disabled after it was enabled, Yes it can, e.g. at guest reset. Generally features can be tweaked any way guest likes until status is set to OK. > or MSI-X can be simply disabled during run time. Not sure what you mean by 'run time'. Guest can reset or disable the device, change any parameters, then re-enable. > Maybe this is better solved by copying the way it was done in PCI itself > with capability linked list? > > -- > > Sasha. There are any number of ways to lay out the structure. I went for what seemed a simplest one. For MSI-X the train has left the station. We can probably still tweak where the high 32 bit features for 64 bit features are. No idea if it's worth it. -- 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: [PATCH] virtio-net: Read MAC only after initializing MSI-X
On Fri, 2011-08-19 at 18:23 +0300, Michael S. Tsirkin wrote: > On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote: > > The MAC of a virtio-net device is located at the first field of the device > > specific header. This header is located at offset 20 if the device doesn't > > support MSI-X or offset 24 if it does. > > > > Current code in virtnet_probe() used to probe the MAC before checking for > > MSI-X, which means that the read was always made from offset 20 regardless > > of whether MSI-X in enabled or not. > > > > This patch moves the MAC probe to after the detection of whether MSI-X is > > enabled. This way the MAC will be read from offset 24 if the device indeed > > supports MSI-X. > > > > Cc: Rusty Russell > > Cc: Michael S. Tsirkin > > Cc: virtualizat...@lists.linux-foundation.org > > Cc: net...@vger.kernel.org > > Cc: kvm@vger.kernel.org > > Signed-off-by: Sasha Levin > > I am not sure I see a bug in virtio: the config pace layout simply > changes as msix is enabled and disabled (and if you look at the latest > draft, also on whether 64 bit features are enabled). > It doesn't depend on msix capability being present in device. > > The spec seems to be explicit enough: > If MSI-X is enabled for the device, two additional fields immediately > follow this header. > > So I'm guessing the bug is in kvm tools which assume > same layout for when msix is enabled and disabled. > qemu-kvm seems to do the right thing so the device > seems to get the correct mac. We assumed that PCI config space has a static layout like most other devices. Having a behavior of "First bit 20 does something, but after enabling MSI-X it does something completely different" sounds strange. I'm wondering why offsets of the config structure change during run time and are not statically defined when the device is started. It's not like VIRTIO_F_FEATURES_HI can be disabled after it was enabled, or MSI-X can be simply disabled during run time. Maybe this is better solved by copying the way it was done in PCI itself with capability linked list? -- 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: [PATCH] virtio-net: Read MAC only after initializing MSI-X
On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote: > The MAC of a virtio-net device is located at the first field of the device > specific header. This header is located at offset 20 if the device doesn't > support MSI-X or offset 24 if it does. > > Current code in virtnet_probe() used to probe the MAC before checking for > MSI-X, which means that the read was always made from offset 20 regardless > of whether MSI-X in enabled or not. > > This patch moves the MAC probe to after the detection of whether MSI-X is > enabled. This way the MAC will be read from offset 24 if the device indeed > supports MSI-X. > > Cc: Rusty Russell > Cc: Michael S. Tsirkin > Cc: virtualizat...@lists.linux-foundation.org > Cc: net...@vger.kernel.org > Cc: kvm@vger.kernel.org > Signed-off-by: Sasha Levin I am not sure I see a bug in virtio: the config pace layout simply changes as msix is enabled and disabled (and if you look at the latest draft, also on whether 64 bit features are enabled). It doesn't depend on msix capability being present in device. The spec seems to be explicit enough: If MSI-X is enabled for the device, two additional fields immediately follow this header. So I'm guessing the bug is in kvm tools which assume same layout for when msix is enabled and disabled. qemu-kvm seems to do the right thing so the device seems to get the correct mac. > --- > drivers/net/virtio_net.c | 16 > 1 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 0c7321c..55ccf96 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -981,14 +981,6 @@ static int virtnet_probe(struct virtio_device *vdev) > /* (!csum && gso) case will be fixed by register_netdev() */ > } > > - /* Configuration may specify what MAC to use. Otherwise random. */ > - if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) { > - vdev->config->get(vdev, > - offsetof(struct virtio_net_config, mac), > - dev->dev_addr, dev->addr_len); > - } else > - random_ether_addr(dev->dev_addr); > - > /* Set up our device-specific information */ > vi = netdev_priv(dev); > netif_napi_add(dev, &vi->napi, virtnet_poll, napi_weight); > @@ -1032,6 +1024,14 @@ static int virtnet_probe(struct virtio_device *vdev) > dev->features |= NETIF_F_HW_VLAN_FILTER; > } > > + /* Configuration may specify what MAC to use. Otherwise random. */ > + if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) { > + vdev->config->get(vdev, > + offsetof(struct virtio_net_config, mac), > + dev->dev_addr, dev->addr_len); > + } else > + random_ether_addr(dev->dev_addr); > + > err = register_netdev(dev); > if (err) { > pr_debug("virtio_net: registering device failed\n"); > -- > 1.7.6 -- 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: [PATCH] virtio-net: Read MAC only after initializing MSI-X
On Mon, 2011-08-15 at 09:55 +0930, Rusty Russell wrote: > On Sun, 14 Aug 2011 16:57:32 +0300, Sasha Levin > wrote: > > On Sun, 2011-08-14 at 12:23 +0930, Rusty Russell wrote: > > > On Sat, 13 Aug 2011 11:51:01 +0300, Sasha Levin > > > wrote: > > > > The MAC of a virtio-net device is located at the first field of the > > > > device > > > > specific header. This header is located at offset 20 if the device > > > > doesn't > > > > support MSI-X or offset 24 if it does. > > > > > > Erk. This means, in general, we have to do virtio_find_single_vq or > > > config->find_vqs before we examine any config options. > > > > > > Look at virtio_blk, which has the same error. > > > > > > Solutions in order of best to worst: > > > (1) Enable MSI-X before calling device probe. This means reserving two > > > vectors in virtio_pci_probe to ensure we *can* do this, I think. > > > Michael? > > > > Do you mean reserving the vectors even before we probed the device for > > MSI-X support? Wouldn't we need 3 vectors then? (config, input, output). > > We want three, but *need* two: see vp_find_vqs(). Also, the generic > code doesn't know how many virtqueues we have on the device. We can just pci_enable_msix() and see if we can get 2 vectors, if we can - we assume the device has msix on, right? -- 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: [PATCH] virtio-net: Read MAC only after initializing MSI-X
On Sun, 14 Aug 2011 16:57:32 +0300, Sasha Levin wrote: > On Sun, 2011-08-14 at 12:23 +0930, Rusty Russell wrote: > > On Sat, 13 Aug 2011 11:51:01 +0300, Sasha Levin > > wrote: > > > The MAC of a virtio-net device is located at the first field of the device > > > specific header. This header is located at offset 20 if the device doesn't > > > support MSI-X or offset 24 if it does. > > > > Erk. This means, in general, we have to do virtio_find_single_vq or > > config->find_vqs before we examine any config options. > > > > Look at virtio_blk, which has the same error. > > > > Solutions in order of best to worst: > > (1) Enable MSI-X before calling device probe. This means reserving two > > vectors in virtio_pci_probe to ensure we *can* do this, I think. > > Michael? > > Do you mean reserving the vectors even before we probed the device for > MSI-X support? Wouldn't we need 3 vectors then? (config, input, output). We want three, but *need* two: see vp_find_vqs(). Also, the generic code doesn't know how many virtqueues we have on the device. > > (2) Ensure ordering of "find_vqs then access config space" statically. This > > probably means handing the vqs array to virtio_config_val, so noone > > can call it before they have their virtqueues. > > Just noticed that only virtio-blk uses virtio_config_val(), while the > others are still doing 'if(virtio_has_feature()) vdev->config->get()', > I'll send patches to fix that regardless of what we end up doing here. Thanks. > Did you want to pass the vq array to virtio_config_val() just to check > that they were already found? Not if we fix is using method #1... 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: [PATCH] virtio-net: Read MAC only after initializing MSI-X
On Sun, 2011-08-14 at 12:23 +0930, Rusty Russell wrote: > On Sat, 13 Aug 2011 11:51:01 +0300, Sasha Levin > wrote: > > The MAC of a virtio-net device is located at the first field of the device > > specific header. This header is located at offset 20 if the device doesn't > > support MSI-X or offset 24 if it does. > > Erk. This means, in general, we have to do virtio_find_single_vq or > config->find_vqs before we examine any config options. > > Look at virtio_blk, which has the same error. > > Solutions in order of best to worst: > (1) Enable MSI-X before calling device probe. This means reserving two > vectors in virtio_pci_probe to ensure we *can* do this, I think. Michael? Do you mean reserving the vectors even before we probed the device for MSI-X support? Wouldn't we need 3 vectors then? (config, input, output). > (2) Ensure ordering of "find_vqs then access config space" statically. This > probably means handing the vqs array to virtio_config_val, so noone > can call it before they have their virtqueues. Just noticed that only virtio-blk uses virtio_config_val(), while the others are still doing 'if(virtio_has_feature()) vdev->config->get()', I'll send patches to fix that regardless of what we end up doing here. Did you want to pass the vq array to virtio_config_val() just to check that they were already found? > (3) Ensure ordering dynamically, ie. BUG_ON() if they haven't done > find_vqs when they call the config accessors. > > If (1) is too invasive for -stable, then we should rearrange the drivers > in separate patches (and cc: -stable), then fix it properly. > > Good catch Sasha! > > Cheers, > Rusty. -- 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: [PATCH] virtio-net: Read MAC only after initializing MSI-X
On Sat, 13 Aug 2011 11:51:01 +0300, Sasha Levin wrote: > The MAC of a virtio-net device is located at the first field of the device > specific header. This header is located at offset 20 if the device doesn't > support MSI-X or offset 24 if it does. Erk. This means, in general, we have to do virtio_find_single_vq or config->find_vqs before we examine any config options. Look at virtio_blk, which has the same error. Solutions in order of best to worst: (1) Enable MSI-X before calling device probe. This means reserving two vectors in virtio_pci_probe to ensure we *can* do this, I think. Michael? (2) Ensure ordering of "find_vqs then access config space" statically. This probably means handing the vqs array to virtio_config_val, so noone can call it before they have their virtqueues. (3) Ensure ordering dynamically, ie. BUG_ON() if they haven't done find_vqs when they call the config accessors. If (1) is too invasive for -stable, then we should rearrange the drivers in separate patches (and cc: -stable), then fix it properly. Good catch Sasha! 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