| Message ID | 1347000499-28701-5-git-send-email-nab@linux-iscsi.org |
|---|---|
| State | New |
| Headers | show |
Il 07/09/2012 08:48, Nicholas A. Bellinger ha scritto: > Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> > --- > hw/virtio-pci.c | 2 ++ > hw/virtio-scsi.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > hw/virtio-scsi.h | 1 + > 3 files changed, 52 insertions(+), 0 deletions(-) Please create a completely separate device vhost-scsi-pci instead (or virtio-scsi-tcm-pci, or something like that). It is used completely differently from virtio-scsi-pci, it does not make sense to conflate the two. Paolo
On Fri, 2012-09-07 at 18:00 +0200, Paolo Bonzini wrote: > Il 07/09/2012 08:48, Nicholas A. Bellinger ha scritto: > > Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > > Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > > Cc: Michael S. Tsirkin <mst@redhat.com> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> > > --- > > hw/virtio-pci.c | 2 ++ > > hw/virtio-scsi.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > > hw/virtio-scsi.h | 1 + > > 3 files changed, 52 insertions(+), 0 deletions(-) > > Please create a completely separate device vhost-scsi-pci instead (or > virtio-scsi-tcm-pci, or something like that). It is used completely > differently from virtio-scsi-pci, it does not make sense to conflate the > two. > Ok, I need to figure out what this will involve over the next days, and will likely have some more questions for you to get a standlone vhost-scsi-pci up and running. Also just curious (question for Anthony + QEMU folks), how long can we expect the QEMU 1.3 merge window to be open..? Thanks Paolo! --nab
Il 07/09/2012 21:23, Nicholas A. Bellinger ha scritto: >> > Please create a completely separate device vhost-scsi-pci instead (or >> > virtio-scsi-tcm-pci, or something like that). It is used completely >> > differently from virtio-scsi-pci, it does not make sense to conflate the >> > two. >> > > Ok, I need to figure out what this will involve over the next days, and > will likely have some more questions for you to get a standlone > vhost-scsi-pci up and running. > > Also just curious (question for Anthony + QEMU folks), how long can we > expect the QEMU 1.3 merge window to be open..? wiki.qemu.org/Planning/1.3 - no hurry, until November 15th. Paolo
On Fri, Sep 07, 2012 at 06:00:50PM +0200, Paolo Bonzini wrote: > Il 07/09/2012 08:48, Nicholas A. Bellinger ha scritto: > > Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > > Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > > Cc: Michael S. Tsirkin <mst@redhat.com> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> > > --- > > hw/virtio-pci.c | 2 ++ > > hw/virtio-scsi.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > > hw/virtio-scsi.h | 1 + > > 3 files changed, 52 insertions(+), 0 deletions(-) > > Please create a completely separate device vhost-scsi-pci instead (or > virtio-scsi-tcm-pci, or something like that). It is used completely > differently from virtio-scsi-pci, it does not make sense to conflate the > two. > > Paolo Ideally the name would say how it is different, not what backend it uses. Any good suggestions?
Il 09/09/2012 00:40, Michael S. Tsirkin ha scritto: > On Fri, Sep 07, 2012 at 06:00:50PM +0200, Paolo Bonzini wrote: >> Il 07/09/2012 08:48, Nicholas A. Bellinger ha scritto: >>> Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> >>> Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> >>> Cc: Michael S. Tsirkin <mst@redhat.com> >>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> >>> --- >>> hw/virtio-pci.c | 2 ++ >>> hw/virtio-scsi.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ >>> hw/virtio-scsi.h | 1 + >>> 3 files changed, 52 insertions(+), 0 deletions(-) >> >> Please create a completely separate device vhost-scsi-pci instead (or >> virtio-scsi-tcm-pci, or something like that). It is used completely >> differently from virtio-scsi-pci, it does not make sense to conflate the >> two. > > Ideally the name would say how it is different, not what backend it > uses. Any good suggestions? I chose the backend name because, ideally, there would be no other difference. QEMU _could_ implement all the goodies in vhost-scsi (such as reservations or ALUA), it just doesn't do that yet. Paolo
On Mon, Sep 10, 2012 at 08:16:54AM +0200, Paolo Bonzini wrote: > Il 09/09/2012 00:40, Michael S. Tsirkin ha scritto: > > On Fri, Sep 07, 2012 at 06:00:50PM +0200, Paolo Bonzini wrote: > >> Il 07/09/2012 08:48, Nicholas A. Bellinger ha scritto: > >>> Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > >>> Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> > >>> Cc: Michael S. Tsirkin <mst@redhat.com> > >>> Cc: Paolo Bonzini <pbonzini@redhat.com> > >>> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org> > >>> --- > >>> hw/virtio-pci.c | 2 ++ > >>> hw/virtio-scsi.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > >>> hw/virtio-scsi.h | 1 + > >>> 3 files changed, 52 insertions(+), 0 deletions(-) > >> > >> Please create a completely separate device vhost-scsi-pci instead (or > >> virtio-scsi-tcm-pci, or something like that). It is used completely > >> differently from virtio-scsi-pci, it does not make sense to conflate the > >> two. > > > > Ideally the name would say how it is different, not what backend it > > uses. Any good suggestions? > > I chose the backend name because, ideally, there would be no other > difference. QEMU _could_ implement all the goodies in vhost-scsi (such > as reservations or ALUA), it just doesn't do that yet. > > Paolo Then why do you say "It is used completely differently from virtio-scsi-pci"? Isn't it just a different backend? If yes then it should be a backend option, like it is for virtio-net.
Il 10/09/2012 08:24, Michael S. Tsirkin ha scritto: >> > I chose the backend name because, ideally, there would be no other >> > difference. QEMU _could_ implement all the goodies in vhost-scsi (such >> > as reservations or ALUA), it just doesn't do that yet. >> > >> > Paolo > Then why do you say "It is used completely differently from > virtio-scsi-pci"? It is configured differently (and I haven't seen a proposal yet for how to bridge the two), it does not interoperate, it has right now a different set of features. The "does not interoperate" bit is particularly important. Say QEMU were to implement persistent reservations (right now only a vhost-scsi feature). Then QEMU and vhost-scsi PR would not be interchangeable, a reservation made by QEMU would not be visible in vhost and vice versa. > Isn't it just a different backend? > > If yes then it should be a backend option, like it is > for virtio-net. You mean a -drive option? That would mean adding the logic to configure vhost-scsi to the QEMU block layer, that's a completely different project... Paolo
On Mon, Sep 10, 2012 at 08:42:15AM +0200, Paolo Bonzini wrote: > Il 10/09/2012 08:24, Michael S. Tsirkin ha scritto: > >> > I chose the backend name because, ideally, there would be no other > >> > difference. QEMU _could_ implement all the goodies in vhost-scsi (such > >> > as reservations or ALUA), it just doesn't do that yet. > >> > > >> > Paolo > > Then why do you say "It is used completely differently from > > virtio-scsi-pci"? > > It is configured differently (and I haven't seen a proposal yet for how > to bridge the two), it does not interoperate, it has right now a > different set of features. > > The "does not interoperate" bit is particularly important. Say QEMU > were to implement persistent reservations (right now only a vhost-scsi > feature). Then QEMU and vhost-scsi PR would not be interchangeable, a > reservation made by QEMU would not be visible in vhost and vice versa. So this is backend stuff, right? > > Isn't it just a different backend? > > > > If yes then it should be a backend option, like it is > > for virtio-net. > > You mean a -drive option? Yes. > That would mean adding the logic to configure > vhost-scsi to the QEMU block layer, that's a completely different project... > > Paolo This is an implementation detail. You can make it -drive option but still have all the actual logic outside block layer. All you need in block is option parsing code. Please take a look at how -net does this: we did *not* add all logic to qemu net layer.
On 09/10/2012 01:24 AM, Michael S. Tsirkin wrote: > On Mon, Sep 10, 2012 at 08:16:54AM +0200, Paolo Bonzini wrote: >> Il 09/09/2012 00:40, Michael S. Tsirkin ha scritto: >>> On Fri, Sep 07, 2012 at 06:00:50PM +0200, Paolo Bonzini wrote: >>>> Il 07/09/2012 08:48, Nicholas A. Bellinger ha scritto: >>>>> Cc: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com> >>>>> Cc: Zhi Yong Wu<wuzhy@linux.vnet.ibm.com> >>>>> Cc: Michael S. Tsirkin<mst@redhat.com> >>>>> Cc: Paolo Bonzini<pbonzini@redhat.com> >>>>> Signed-off-by: Nicholas Bellinger<nab@linux-iscsi.org> >>>>> --- >>>>> hw/virtio-pci.c | 2 ++ >>>>> hw/virtio-scsi.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ >>>>> hw/virtio-scsi.h | 1 + >>>>> 3 files changed, 52 insertions(+), 0 deletions(-) >>>> >>>> Please create a completely separate device vhost-scsi-pci instead (or >>>> virtio-scsi-tcm-pci, or something like that). It is used completely >>>> differently from virtio-scsi-pci, it does not make sense to conflate the >>>> two. >>> >>> Ideally the name would say how it is different, not what backend it >>> uses. Any good suggestions? >> >> I chose the backend name because, ideally, there would be no other >> difference. QEMU _could_ implement all the goodies in vhost-scsi (such >> as reservations or ALUA), it just doesn't do that yet. >> >> Paolo > > Then why do you say "It is used completely differently from > virtio-scsi-pci"? Isn't it just a different backend? > > If yes then it should be a backend option, like it is > for virtio-net. I don't mean to bike shed here so don't take this as a nack on making it a backend option, but in retrospect, the way we did vhost-net was a mistake even though I strongly advocated for it to be a backend option. The code to do it is really, really ugly. I think it would have made a lot more sense to just make it a device and then have it not use a netdev backend or any other kind of backend split. For instance: qemu -device vhost-net-pci,tapfd=X I know this breaks the model of separate backends and frontends but since vhost-net absolutely requires a tap fd, I think it's better in the long run to not abuse the netdev backend to prevent user confusion. Having a dedicated backend type that only has one possible option and can only be used by one device is a bit silly too. So I would be in favor of dropping/squashing 3/5 and radically simplifying how this was exposed to the user. I would just take qemu_vhost_scsi_opts and make them device properties. Regards, Anthony Liguori >
On Tue, Sep 11, 2012 at 08:46:34AM -0500, Anthony Liguori wrote: > On 09/10/2012 01:24 AM, Michael S. Tsirkin wrote: > >On Mon, Sep 10, 2012 at 08:16:54AM +0200, Paolo Bonzini wrote: > >>Il 09/09/2012 00:40, Michael S. Tsirkin ha scritto: > >>>On Fri, Sep 07, 2012 at 06:00:50PM +0200, Paolo Bonzini wrote: > >>>>Il 07/09/2012 08:48, Nicholas A. Bellinger ha scritto: > >>>>>Cc: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com> > >>>>>Cc: Zhi Yong Wu<wuzhy@linux.vnet.ibm.com> > >>>>>Cc: Michael S. Tsirkin<mst@redhat.com> > >>>>>Cc: Paolo Bonzini<pbonzini@redhat.com> > >>>>>Signed-off-by: Nicholas Bellinger<nab@linux-iscsi.org> > >>>>>--- > >>>>> hw/virtio-pci.c | 2 ++ > >>>>> hw/virtio-scsi.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > >>>>> hw/virtio-scsi.h | 1 + > >>>>> 3 files changed, 52 insertions(+), 0 deletions(-) > >>>> > >>>>Please create a completely separate device vhost-scsi-pci instead (or > >>>>virtio-scsi-tcm-pci, or something like that). It is used completely > >>>>differently from virtio-scsi-pci, it does not make sense to conflate the > >>>>two. > >>> > >>>Ideally the name would say how it is different, not what backend it > >>>uses. Any good suggestions? > >> > >>I chose the backend name because, ideally, there would be no other > >>difference. QEMU _could_ implement all the goodies in vhost-scsi (such > >>as reservations or ALUA), it just doesn't do that yet. > >> > >>Paolo > > > >Then why do you say "It is used completely differently from > >virtio-scsi-pci"? Isn't it just a different backend? > > > >If yes then it should be a backend option, like it is > >for virtio-net. > > I don't mean to bike shed here so don't take this as a nack on > making it a backend option, but in retrospect, the way we did > vhost-net was a mistake even though I strongly advocated for it to > be a backend option. > > The code to do it is really, really ugly. I think it would have > made a lot more sense to just make it a device and then have it not > use a netdev backend or any other kind of backend split. > > For instance: > > qemu -device vhost-net-pci,tapfd=X > > I know this breaks the model of separate backends and frontends but > since vhost-net absolutely requires a tap fd, I think it's better in > the long run to not abuse the netdev backend to prevent user > confusion. Having a dedicated backend type that only has one > possible option and can only be used by one device is a bit silly > too. > > So I would be in favor of dropping/squashing 3/5 and radically > simplifying how this was exposed to the user. > > I would just take qemu_vhost_scsi_opts and make them device properties. > > Regards, > > Anthony Liguori I'd like to clarify that I'm fine with either approach. Even a separate device is OK if this is what others want though I like it the least. > >
On Tue, 2012-09-11 at 18:07 +0300, Michael S. Tsirkin wrote: > On Tue, Sep 11, 2012 at 08:46:34AM -0500, Anthony Liguori wrote: > > On 09/10/2012 01:24 AM, Michael S. Tsirkin wrote: > > >On Mon, Sep 10, 2012 at 08:16:54AM +0200, Paolo Bonzini wrote: > > >>Il 09/09/2012 00:40, Michael S. Tsirkin ha scritto: > > >>>On Fri, Sep 07, 2012 at 06:00:50PM +0200, Paolo Bonzini wrote: <SNIP> > > >>>>Please create a completely separate device vhost-scsi-pci instead (or > > >>>>virtio-scsi-tcm-pci, or something like that). It is used completely > > >>>>differently from virtio-scsi-pci, it does not make sense to conflate the > > >>>>two. > > >>> > > >>>Ideally the name would say how it is different, not what backend it > > >>>uses. Any good suggestions? > > >> > > >>I chose the backend name because, ideally, there would be no other > > >>difference. QEMU _could_ implement all the goodies in vhost-scsi (such > > >>as reservations or ALUA), it just doesn't do that yet. > > >> > > >>Paolo > > > > > >Then why do you say "It is used completely differently from > > >virtio-scsi-pci"? Isn't it just a different backend? > > > > > >If yes then it should be a backend option, like it is > > >for virtio-net. > > > > I don't mean to bike shed here so don't take this as a nack on > > making it a backend option, but in retrospect, the way we did > > vhost-net was a mistake even though I strongly advocated for it to > > be a backend option. > > > > The code to do it is really, really ugly. I think it would have > > made a lot more sense to just make it a device and then have it not > > use a netdev backend or any other kind of backend split. > > > > For instance: > > > > qemu -device vhost-net-pci,tapfd=X > > > > I know this breaks the model of separate backends and frontends but > > since vhost-net absolutely requires a tap fd, I think it's better in > > the long run to not abuse the netdev backend to prevent user > > confusion. Having a dedicated backend type that only has one > > possible option and can only be used by one device is a bit silly > > too. > > > > So I would be in favor of dropping/squashing 3/5 and radically > > simplifying how this was exposed to the user. > > > > I would just take qemu_vhost_scsi_opts and make them device properties. > > > > Regards, > > > > Anthony Liguori > > I'd like to clarify that I'm fine with either approach. > Even a separate device is OK if this is what others want > though I like it the least. > Hi MST, Paolo & Co, I've been out the better part of the week with the flu, and am just now catching up on emails from the last days.. So to better understand the reasoning for adding an separate PCI device for vhost-scsi ahead of implementing the code changes, here are main points from folk's comments: *) Convert vhost-scsi into a separate standalone vhost-scsi-pci device - Lets userspace know that virtio-scsi + QEMU block and virtio-scsi + tcm_vhost do not track SCSI state (such as reservations + ALUA), and hence are not interchangeable during live-migration. - Reduces complexity of adding vhost-scsi related logic into existing virtio-scsi-pci code path. - Having backends with one possible option doesn’t make much sense. *) Keep vhost-scsi as a backend to virtio-scsi-pci - Reduces duplicated code amongst multiple virtio-scsi backends. - Follows the split for what existing vhost-net code already does. So that said, two quick questions for Paolo & Co.. For the standalone vhost-scsi-pci device case, can you give a brief idea as to what extent you'd like to see virtio-scsi.c code/defs duplicated and/or shared amongst a new vhost-scsi-pci device..? Also to help me along, can you give an example based on the current usage below how the QEMU command line arguments would change with a standalone vhost-scsi-pci device..? ./x86_64-softmmu/qemu-system-x86_64 -enable-kvm -smp 4 -m 2048 \ -hda /usr/src/qemu-vhost.git/debian_squeeze_amd64_standard-old.qcow2 \ -vhost-scsi id=vhost-scsi0,wwpn=naa.600140579ad21088,tpgt=1 \ -device virtio-scsi-pci,vhost-scsi=vhost-scsi0,event_idx=off Thank you! --nab
Il 14/09/2012 00:27, Nicholas A. Bellinger ha scritto: > *) Keep vhost-scsi as a backend to virtio-scsi-pci > > - Reduces duplicated code amongst multiple virtio-scsi backends. > > - Follows the split for what existing vhost-net code already does. > > So that said, two quick questions for Paolo & Co.. > > For the standalone vhost-scsi-pci device case, can you give a brief idea > as to what extent you'd like to see virtio-scsi.c code/defs duplicated > and/or shared amongst a new vhost-scsi-pci device..? Not much, in the end, would be shared; it could end up being just parts of virtio_scsi_init and virtio_scsi_exit, and virtio_scsi_get_config. Almost all the other code is to implement the SCSI bus interface, which you do not need. I don't remember if and how vhost handles configuration changes. If you need any struct in virtio-scsi.c, either move it to virtio-scsi.h or add the new device in the same file. > Also to help me along, can you give an example based on the current > usage below how the QEMU command line arguments would change with a > standalone vhost-scsi-pci device..? > > ./x86_64-softmmu/qemu-system-x86_64 -enable-kvm -smp 4 -m 2048 \ > -hda /usr/src/qemu-vhost.git/debian_squeeze_amd64_standard-old.qcow2 \ > -vhost-scsi id=vhost-scsi0,wwpn=naa.600140579ad21088,tpgt=1 \ > -device virtio-scsi-pci,vhost-scsi=vhost-scsi0,event_idx=off Two possibilities. Either simply do s/virtio-scsi-pci/vhost-scsi-pci/ or do ./x86_64-softmmu/qemu-system-x86_64 -enable-kvm -smp 4 -m 2048 \ -hda /usr/src/qemu-vhost.git/debian_squeeze_amd64_standard-old.qcow2 \ -device virtio-scsi-pci,wwpn=naa.600140579ad21088,tpgt=1,event_idx=off
On Fri, Sep 14, 2012 at 08:45:11AM +0200, Paolo Bonzini wrote: > Il 14/09/2012 00:27, Nicholas A. Bellinger ha scritto: > > *) Keep vhost-scsi as a backend to virtio-scsi-pci > > > > - Reduces duplicated code amongst multiple virtio-scsi backends. > > > > - Follows the split for what existing vhost-net code already does. > > > > So that said, two quick questions for Paolo & Co.. > > > > For the standalone vhost-scsi-pci device case, can you give a brief idea > > as to what extent you'd like to see virtio-scsi.c code/defs duplicated > > and/or shared amongst a new vhost-scsi-pci device..? > > Not much, in the end, would be shared; it could end up being just > parts of virtio_scsi_init and virtio_scsi_exit, and virtio_scsi_get_config. > > Almost all the other code is to implement the SCSI bus interface, > which you do not need. > > I don't remember if and how vhost handles configuration changes. All configuration changes are handled in userspace. > If you > need any struct in virtio-scsi.c, either move it to virtio-scsi.h or > add the new device in the same file. > > > Also to help me along, can you give an example based on the current > > usage below how the QEMU command line arguments would change with a > > standalone vhost-scsi-pci device..? > > > > ./x86_64-softmmu/qemu-system-x86_64 -enable-kvm -smp 4 -m 2048 \ > > -hda /usr/src/qemu-vhost.git/debian_squeeze_amd64_standard-old.qcow2 \ > > -vhost-scsi id=vhost-scsi0,wwpn=naa.600140579ad21088,tpgt=1 \ > > -device virtio-scsi-pci,vhost-scsi=vhost-scsi0,event_idx=off > > Two possibilities. Either simply do s/virtio-scsi-pci/vhost-scsi-pci/ or do > > ./x86_64-softmmu/qemu-system-x86_64 -enable-kvm -smp 4 -m 2048 \ > -hda /usr/src/qemu-vhost.git/debian_squeeze_amd64_standard-old.qcow2 \ > -device virtio-scsi-pci,wwpn=naa.600140579ad21088,tpgt=1,event_idx=off I think I like the second option better.
On Tue, Sep 11, 2012 at 08:46:34AM -0500, Anthony Liguori wrote: > On 09/10/2012 01:24 AM, Michael S. Tsirkin wrote: > >On Mon, Sep 10, 2012 at 08:16:54AM +0200, Paolo Bonzini wrote: > >>Il 09/09/2012 00:40, Michael S. Tsirkin ha scritto: > >>>On Fri, Sep 07, 2012 at 06:00:50PM +0200, Paolo Bonzini wrote: > >>>>Il 07/09/2012 08:48, Nicholas A. Bellinger ha scritto: > >>>>>Cc: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com> > >>>>>Cc: Zhi Yong Wu<wuzhy@linux.vnet.ibm.com> > >>>>>Cc: Michael S. Tsirkin<mst@redhat.com> > >>>>>Cc: Paolo Bonzini<pbonzini@redhat.com> > >>>>>Signed-off-by: Nicholas Bellinger<nab@linux-iscsi.org> > >>>>>--- > >>>>> hw/virtio-pci.c | 2 ++ > >>>>> hw/virtio-scsi.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > >>>>> hw/virtio-scsi.h | 1 + > >>>>> 3 files changed, 52 insertions(+), 0 deletions(-) > >>>> > >>>>Please create a completely separate device vhost-scsi-pci instead (or > >>>>virtio-scsi-tcm-pci, or something like that). It is used completely > >>>>differently from virtio-scsi-pci, it does not make sense to conflate the > >>>>two. > >>> > >>>Ideally the name would say how it is different, not what backend it > >>>uses. Any good suggestions? > >> > >>I chose the backend name because, ideally, there would be no other > >>difference. QEMU _could_ implement all the goodies in vhost-scsi (such > >>as reservations or ALUA), it just doesn't do that yet. > >> > >>Paolo > > > >Then why do you say "It is used completely differently from > >virtio-scsi-pci"? Isn't it just a different backend? > > > >If yes then it should be a backend option, like it is > >for virtio-net. > > I don't mean to bike shed here so don't take this as a nack on > making it a backend option, but in retrospect, the way we did > vhost-net was a mistake even though I strongly advocated for it to > be a backend option. > > The code to do it is really, really ugly. I think it would have > made a lot more sense to just make it a device and then have it not > use a netdev backend or any other kind of backend split. > > For instance: > > qemu -device vhost-net-pci,tapfd=X We'd have to duplicate all tap options such as upscript then, and educate users that vhost-net-pci is in fact same as virtio-net-pci just faster. They have enough trouble guessing "-net-pci" in virtio-net-pci. IMHO a simple -device virtio-net-pci,vhost=on would have been the right thing to do in retrospect. > since vhost-net absolutely requires a tap fd, I think it's better in > the long run to not abuse the netdev backend to prevent user > confusion. In practice adding an option (even if it was in the wrong place) did not result in user confusion. Also in practice, renaming "virtio" to virtio-net-pci etc did create confusion. > Having a dedicated backend type that only has one > possible option and can only be used by one device is a bit silly > too. By now we can just enable it by default. This was always the idea. > So I would be in favor of dropping/squashing 3/5 and radically > simplifying how this was exposed to the user. Let's just make sure we don't have implementation tail wagging the user interface dog :) > > I would just take qemu_vhost_scsi_opts and make them device properties. > > Regards, > > Anthony Liguori > > >
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index 125eded..8ec7cf1 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -22,6 +22,7 @@ #include "virtio-net.h" #include "virtio-serial.h" #include "virtio-scsi.h" +#include "vhost-scsi.h" #include "pci.h" #include "qemu-error.h" #include "msi.h" @@ -1036,6 +1037,7 @@ static void virtio_scsi_exit_pci(PCIDevice *pci_dev) } static Property virtio_scsi_properties[] = { + DEFINE_PROP_VHOST_SCSI("vhost-scsi", VirtIOPCIProxy, scsi.vhost_scsi), DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true), DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, DEV_NVECTORS_UNSPECIFIED), DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi), diff --git a/hw/virtio-scsi.c b/hw/virtio-scsi.c index 5f737ac..edda097 100644 --- a/hw/virtio-scsi.c +++ b/hw/virtio-scsi.c @@ -13,9 +13,13 @@ * */ +#include "qemu-common.h" +#include "qemu-error.h" +#include "vhost-scsi.h" #include "virtio-scsi.h" #include <hw/scsi.h> #include <hw/scsi-defs.h> +#include "vhost.h" #define VIRTIO_SCSI_VQ_SIZE 128 #define VIRTIO_SCSI_CDB_SIZE 32 @@ -144,6 +148,10 @@ typedef struct { uint32_t cdb_size; int resetting; bool events_dropped; + + bool vhost_started; + VHostSCSI *vhost_scsi; + VirtQueue *ctrl_vq; VirtQueue *event_vq; VirtQueue *cmd_vqs[0]; @@ -699,6 +707,38 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = { .load_request = virtio_scsi_load_request, }; +static bool virtio_scsi_started(VirtIOSCSI *s, uint8_t val) +{ + return (val & VIRTIO_CONFIG_S_DRIVER_OK) && s->vdev.vm_running; +} + +static void virtio_scsi_set_status(VirtIODevice *vdev, uint8_t val) +{ + VirtIOSCSI *s = (VirtIOSCSI *)vdev; + bool start = virtio_scsi_started(s, val); + + if (s->vhost_started == start) { + return; + } + + if (start) { + int ret; + + ret = vhost_scsi_start(s->vhost_scsi, vdev); + if (ret < 0) { + error_report("virtio-scsi: unable to start vhost: %s\n", + strerror(-ret)); + + /* There is no userspace virtio-scsi fallback so exit */ + exit(1); + } + } else { + vhost_scsi_stop(s->vhost_scsi, vdev); + } + + s->vhost_started = start; +} + VirtIODevice *virtio_scsi_init(DeviceState *dev, VirtIOSCSIConf *proxyconf) { VirtIOSCSI *s; @@ -712,12 +752,17 @@ VirtIODevice *virtio_scsi_init(DeviceState *dev, VirtIOSCSIConf *proxyconf) s->qdev = dev; s->conf = proxyconf; + s->vhost_started = false; + s->vhost_scsi = s->conf->vhost_scsi; /* TODO set up vdev function pointers */ s->vdev.get_config = virtio_scsi_get_config; s->vdev.set_config = virtio_scsi_set_config; s->vdev.get_features = virtio_scsi_get_features; s->vdev.reset = virtio_scsi_reset; + if (s->vhost_scsi) { + s->vdev.set_status = virtio_scsi_set_status; + } s->ctrl_vq = virtio_add_queue(&s->vdev, VIRTIO_SCSI_VQ_SIZE, virtio_scsi_handle_ctrl); @@ -743,5 +788,9 @@ void virtio_scsi_exit(VirtIODevice *vdev) { VirtIOSCSI *s = (VirtIOSCSI *)vdev; unregister_savevm(s->qdev, "virtio-scsi", s); + + /* This will stop vhost backend if appropriate. */ + virtio_scsi_set_status(vdev, 0); + virtio_cleanup(vdev); } diff --git a/hw/virtio-scsi.h b/hw/virtio-scsi.h index 4bc889d..74e9422 100644 --- a/hw/virtio-scsi.h +++ b/hw/virtio-scsi.h @@ -22,6 +22,7 @@ #define VIRTIO_SCSI_F_CHANGE 2 struct VirtIOSCSIConf { + VHostSCSI *vhost_scsi; uint32_t num_queues; uint32_t max_sectors; uint32_t cmd_per_lun;