Re: [PATCH-v2 0/6] vhost/scsi: Add T10 PI SGL passthrough support

2014-06-02 Thread Michael S. Tsirkin 
On Thu, May 22, 2014 at 02:26:16AM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 Hi MST, MKP, Paolo  Co,
 
 Here is the v2 patch series for adding T1O protection information (PI)
 SGL passthrough support between virtio-scsi LLD + vhost-scsi fabric
 endpoints.
 
 Following MST's recommendation, it includes the changes for using
 bytes intead of number of iovecs in virtio_scsi_cmd_req_pi along with
 the associated changes to virtio-scsi + vhost/scsi code.
 
 For vhost/scsi, this includes walking the leading iovec's length(s)
 to determine where protection payload ends, and real data payload 
 starts.  For virtio-scsi LLD code, this includes locating struct
 blk_integrity for using blk_rq_sectors + -tuple_size to calculate
 the total bytes for outgoing cmd_pi-pi_bytes[out,in] values.
 
 The full list of changes from last series include:
 
   - Use pi_bytesout + pi_bytesin instead of niovs in virtio-scsi PI
 header (mst + paolo)
   - Add prot_pto=1 in tcm_vhost_submission_work() when no PI buffer
 exists (nab)
   - Get rid of req + cdb pointer casts in vhost_scsi_handle_vq (mst)
   - Ensure that virtio_scsi_cmd_req_pi processing happens regardless
 of data_num in vhost_scsi_handle_vq (nab)
   - Pass TARGET_PROT_ALL into transport_init_session_tags() (nab)
   - Convert vhost_scsi_handle_vq to use memcpy_fromiovecend() (mst)
   - Convert vhost_scsi_handle_vq to use pi_bytesout + pi_bytesin (nab)
   - Convert virtio_scsi_init_hdr_pi() to use pi_bytesout + pi_bytesin
 (mst + paolo + nab)
   - Use blk_integrity-tuple_size to calculate pi bytes (nab)
 
 Please review for v3.16-rc1 code.
 
 Thanks!
 
 --nab

Acked-by: Michael S. Tsirkin m...@redhat.com


 Nicholas Bellinger (6):
   virtio-scsi.h: Add virtio_scsi_cmd_req_pi + VIRTIO_SCSI_F_T10_PI bits
   vhost/scsi: Move sanity check into vhost_scsi_map_iov_to_sgl
   vhost/scsi: Add preallocation of protection SGLs
   vhost/scsi: Add T10 PI IOV - SGL memory mapping logic
   vhost/scsi: Enable T10 PI IOV - SGL memory mapping
   virtio-scsi: Enable DIF/DIX modes in SCSI host LLD
 
  drivers/scsi/virtio_scsi.c  |   86 +---
  drivers/vhost/scsi.c|  305 
 +--
  include/linux/virtio_scsi.h |   15 ++-
  3 files changed, 292 insertions(+), 114 deletions(-)
 
 -- 
 1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-v2 0/6] vhost/scsi: Add T10 PI SGL passthrough support

2014-06-08 Thread Michael S. Tsirkin 
On Thu, May 22, 2014 at 02:26:16AM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 Hi MST, MKP, Paolo  Co,
 
 Here is the v2 patch series for adding T1O protection information (PI)
 SGL passthrough support between virtio-scsi LLD + vhost-scsi fabric
 endpoints.
 
 Following MST's recommendation, it includes the changes for using
 bytes intead of number of iovecs in virtio_scsi_cmd_req_pi along with
 the associated changes to virtio-scsi + vhost/scsi code.
 
 For vhost/scsi, this includes walking the leading iovec's length(s)
 to determine where protection payload ends, and real data payload 
 starts.  For virtio-scsi LLD code, this includes locating struct
 blk_integrity for using blk_rq_sectors + -tuple_size to calculate
 the total bytes for outgoing cmd_pi-pi_bytes[out,in] values.
 
 The full list of changes from last series include:
 
   - Use pi_bytesout + pi_bytesin instead of niovs in virtio-scsi PI
 header (mst + paolo)
   - Add prot_pto=1 in tcm_vhost_submission_work() when no PI buffer
 exists (nab)
   - Get rid of req + cdb pointer casts in vhost_scsi_handle_vq (mst)
   - Ensure that virtio_scsi_cmd_req_pi processing happens regardless
 of data_num in vhost_scsi_handle_vq (nab)
   - Pass TARGET_PROT_ALL into transport_init_session_tags() (nab)
   - Convert vhost_scsi_handle_vq to use memcpy_fromiovecend() (mst)
   - Convert vhost_scsi_handle_vq to use pi_bytesout + pi_bytesin (nab)
   - Convert virtio_scsi_init_hdr_pi() to use pi_bytesout + pi_bytesin
 (mst + paolo + nab)
   - Use blk_integrity-tuple_size to calculate pi bytes (nab)
 
 Please review for v3.16-rc1 code.
 
 Thanks!
 
 --nab


OK, finally went over this, looks good to me:

Acked-by: Michael S. Tsirkin m...@redhat.com

However, we really should stop making more changes
before fixing ANY_LAYOUT in this driver.

virtio 1.0 should be out soon and that makes ANY_LAYOUT
a required feature.



 Nicholas Bellinger (6):
   virtio-scsi.h: Add virtio_scsi_cmd_req_pi + VIRTIO_SCSI_F_T10_PI bits
   vhost/scsi: Move sanity check into vhost_scsi_map_iov_to_sgl
   vhost/scsi: Add preallocation of protection SGLs
   vhost/scsi: Add T10 PI IOV - SGL memory mapping logic
   vhost/scsi: Enable T10 PI IOV - SGL memory mapping
   virtio-scsi: Enable DIF/DIX modes in SCSI host LLD
 
  drivers/scsi/virtio_scsi.c  |   86 +---
  drivers/vhost/scsi.c|  305 
 +--
  include/linux/virtio_scsi.h |   15 ++-
  3 files changed, 292 insertions(+), 114 deletions(-)
 
 -- 
 1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-v2 1/6] virtio-scsi.h: Add virtio_scsi_cmd_req_pi + VIRTIO_SCSI_F_T10_PI bits

2014-06-09 Thread Michael S. Tsirkin 
On Thu, May 22, 2014 at 02:26:17AM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 This patch adds a virtio_scsi_cmd_req_pi header as recommened by
 Paolo that contains do_pi_niov + di_pi_niov elements used for
 signaling when protection information buffers are expected to
 preceed the data buffers.
 
 Also add new VIRTIO_SCSI_F_T10_PI feature bit to be used to signal
 host support.
 
 v4 changes:
- Use pi_bytesout + pi_bytesin instead of niovs (mst + paolo)
 
 Cc: Paolo Bonzini pbonz...@redhat.com
 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Martin K. Petersen martin.peter...@oracle.com
 Cc: Christoph Hellwig h...@lst.de
 Cc: Hannes Reinecke h...@suse.de
 Cc: Sagi Grimberg sa...@dev.mellanox.co.il
 Cc: H. Peter Anvin h...@zytor.com
 Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
 ---
  include/linux/virtio_scsi.h |   15 ++-
  1 file changed, 14 insertions(+), 1 deletion(-)
 
 diff --git a/include/linux/virtio_scsi.h b/include/linux/virtio_scsi.h
 index 4195b97..7344906 100644
 --- a/include/linux/virtio_scsi.h
 +++ b/include/linux/virtio_scsi.h
 @@ -35,11 +35,23 @@ struct virtio_scsi_cmd_req {
   u8 lun[8];  /* Logical Unit Number */
   u64 tag;/* Command identifier */
   u8 task_attr;   /* Task attribute */
 - u8 prio;
 + u8 prio;/* SAM command priority field */
   u8 crn;
   u8 cdb[VIRTIO_SCSI_CDB_SIZE];
  } __packed;
  
 +/* SCSI command request, followed by protection information */
 +struct virtio_scsi_cmd_req_pi {
 + u8 lun[8];  /* Logical Unit Number */
 + u64 tag;/* Command identifier */
 + u8 task_attr;   /* Task attribute */
 + u8 prio;/* SAM command priority field */
 + u8 crn;
 + u32 pi_bytesout;/* DataOUT PI Number of bytes */
 + u32 pi_bytesin; /* DataIN PI Number of bytes */
 + u8 cdb[VIRTIO_SCSI_CDB_SIZE];
 +} __packed;
 +
  /* Response, followed by sense data and data-in */
  struct virtio_scsi_cmd_resp {
   u32 sense_len;  /* Sense data length */
 @@ -97,6 +109,7 @@ struct virtio_scsi_config {
  #define VIRTIO_SCSI_F_INOUT0
  #define VIRTIO_SCSI_F_HOTPLUG  1
  #define VIRTIO_SCSI_F_CHANGE   2
 +#define VIRTIO_SCSI_F_T10_PI3


I'd like to add that it's strange to add a macro and
only use it in patch 5.
I believe this is one of the reasons bug in patch 5 went
unnoticed ...

  
  /* Response codes */
  #define VIRTIO_SCSI_S_OK   0
 -- 
 1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-v2 0/6] vhost/scsi: Add T10 PI SGL passthrough support

2014-06-09 Thread Michael S. Tsirkin 
On Thu, May 22, 2014 at 02:26:16AM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 Hi MST, MKP, Paolo  Co,
 
 Here is the v2 patch series for adding T1O protection information (PI)
 SGL passthrough support between virtio-scsi LLD + vhost-scsi fabric
 endpoints.
 
 Following MST's recommendation, it includes the changes for using
 bytes intead of number of iovecs in virtio_scsi_cmd_req_pi along with
 the associated changes to virtio-scsi + vhost/scsi code.
 
 For vhost/scsi, this includes walking the leading iovec's length(s)
 to determine where protection payload ends, and real data payload 
 starts.  For virtio-scsi LLD code, this includes locating struct
 blk_integrity for using blk_rq_sectors + -tuple_size to calculate
 the total bytes for outgoing cmd_pi-pi_bytes[out,in] values.
 
 The full list of changes from last series include:
 
   - Use pi_bytesout + pi_bytesin instead of niovs in virtio-scsi PI
 header (mst + paolo)
   - Add prot_pto=1 in tcm_vhost_submission_work() when no PI buffer
 exists (nab)
   - Get rid of req + cdb pointer casts in vhost_scsi_handle_vq (mst)
   - Ensure that virtio_scsi_cmd_req_pi processing happens regardless
 of data_num in vhost_scsi_handle_vq (nab)
   - Pass TARGET_PROT_ALL into transport_init_session_tags() (nab)
   - Convert vhost_scsi_handle_vq to use memcpy_fromiovecend() (mst)
   - Convert vhost_scsi_handle_vq to use pi_bytesout + pi_bytesin (nab)
   - Convert virtio_scsi_init_hdr_pi() to use pi_bytesout + pi_bytesin
 (mst + paolo + nab)
   - Use blk_integrity-tuple_size to calculate pi bytes (nab)
 
 Please review for v3.16-rc1 code.
 
 Thanks!
 
 --nab

OK since this conflicts with my vhost locking
patches, I have rebased this and parked at vhost-review
branch in my tree.

git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-review

Nicholas could you please take a look at the patches and confirm this is
OK ASAP?

If yes I will pack it all up and send to Linus.



 Nicholas Bellinger (6):
   virtio-scsi.h: Add virtio_scsi_cmd_req_pi + VIRTIO_SCSI_F_T10_PI bits
   vhost/scsi: Move sanity check into vhost_scsi_map_iov_to_sgl
   vhost/scsi: Add preallocation of protection SGLs
   vhost/scsi: Add T10 PI IOV - SGL memory mapping logic
   vhost/scsi: Enable T10 PI IOV - SGL memory mapping
   virtio-scsi: Enable DIF/DIX modes in SCSI host LLD
 
  drivers/scsi/virtio_scsi.c  |   86 +---
  drivers/vhost/scsi.c|  305 
 +--
  include/linux/virtio_scsi.h |   15 ++-
  3 files changed, 292 insertions(+), 114 deletions(-)
 
 -- 
 1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-v2 0/6] vhost/scsi: Add T10 PI SGL passthrough support

2014-06-10 Thread Michael S. Tsirkin 
On Tue, Jun 10, 2014 at 12:05:12AM -0700, Nicholas A. Bellinger wrote:
 On Mon, 2014-06-09 at 16:30 +0300, Michael S. Tsirkin wrote:
  On Thu, May 22, 2014 at 02:26:16AM +, Nicholas A. Bellinger wrote:
   From: Nicholas Bellinger n...@linux-iscsi.org
   
   Hi MST, MKP, Paolo  Co,
   
   Here is the v2 patch series for adding T1O protection information (PI)
   SGL passthrough support between virtio-scsi LLD + vhost-scsi fabric
   endpoints.
   
   Following MST's recommendation, it includes the changes for using
   bytes intead of number of iovecs in virtio_scsi_cmd_req_pi along with
   the associated changes to virtio-scsi + vhost/scsi code.
   
   For vhost/scsi, this includes walking the leading iovec's length(s)
   to determine where protection payload ends, and real data payload 
   starts.  For virtio-scsi LLD code, this includes locating struct
   blk_integrity for using blk_rq_sectors + -tuple_size to calculate
   the total bytes for outgoing cmd_pi-pi_bytes[out,in] values.
   
   The full list of changes from last series include:
   
 - Use pi_bytesout + pi_bytesin instead of niovs in virtio-scsi PI
   header (mst + paolo)
 - Add prot_pto=1 in tcm_vhost_submission_work() when no PI buffer
   exists (nab)
 - Get rid of req + cdb pointer casts in vhost_scsi_handle_vq (mst)
 - Ensure that virtio_scsi_cmd_req_pi processing happens regardless
   of data_num in vhost_scsi_handle_vq (nab)
 - Pass TARGET_PROT_ALL into transport_init_session_tags() (nab)
 - Convert vhost_scsi_handle_vq to use memcpy_fromiovecend() (mst)
 - Convert vhost_scsi_handle_vq to use pi_bytesout + pi_bytesin (nab)
 - Convert virtio_scsi_init_hdr_pi() to use pi_bytesout + pi_bytesin
   (mst + paolo + nab)
 - Use blk_integrity-tuple_size to calculate pi bytes (nab)
   
   Please review for v3.16-rc1 code.
   
   Thanks!
   
   --nab
  
  OK since this conflicts with my vhost locking
  patches, I have rebased this and parked at vhost-review
  branch in my tree.
  
  git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-review
  
  Nicholas could you please take a look at the patches and confirm this is
  OK ASAP?
  
  If yes I will pack it all up and send to Linus.
 
 From my experience, Linus prefers to fix this type of conflict on his
 own at merge time, instead of having subsystem maintainers rebase their
 for-next branches to include other's commits.

Cross-device type changes is exactly what vhost tree is there to allow
so I don't see a problem.

What Linus does not want is same patch in two trees.

So I see two options:
- I go ahead with my changes and you with yours and let Linus resolve
  the conflict.  This means bisect build will be broken since the
  breakage will likely not be noticed until after the merge.
- I pick up vhost scsi PI patches on my tree and you drop them from yours.

I prefer option 2 because it's cleaner wrt bisect.
But if you see a problem, pls let me know.

 Stephen (CC'ed) has included a fix in today's linux-next for the merge
 conflict here:
 
 https://lkml.org/lkml/2014/6/10/3
 
 Please confirm, as it will be a pointer to Linus within the
 target-pending/for-next PULL request.
 
 --nab


Yes but this does mean people trying to bisect will
hit build breakages, not nice.

-- 
MST
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-v2 0/6] vhost/scsi: Add T10 PI SGL passthrough support

2014-06-10 Thread Michael S. Tsirkin 
On Tue, Jun 10, 2014 at 09:52:17PM +1000, Stephen Rothwell wrote:
 Hi Michael,
 
 On Tue, 10 Jun 2014 12:42:54 +0300 Michael S. Tsirkin m...@redhat.com 
 wrote:
 
  So I see two options:
  - I go ahead with my changes and you with yours and let Linus resolve
the conflict.  This means bisect build will be broken since the
breakage will likely not be noticed until after the merge.
 
 Well, since the resolution is known, the one who submits their tree
 later should tell Linus (as suggested by Nicholas).  That is part of
 the point of the linux-next tree ... and therefore there would be no
 bisect problem.
 
   Stephen (CC'ed) has included a fix in today's linux-next for the merge
   conflict here:
   
   https://lkml.org/lkml/2014/6/10/3
   
   Please confirm, as it will be a pointer to Linus within the
   target-pending/for-next PULL request.
  
  Yes but this does mean people trying to bisect will
  hit build breakages, not nice.
 
 Not necessarily.
 
 -- 
 Cheers,
 Stephen Rothwells...@canb.auug.org.au


I don't see how that's possible.
Here's a point you might have missed.
Nicholas's patch isn't just introducing a merge conflict.
It is also buggy.
Replacing bit access with has_feature silently fixes the bug.

So if we want to avoid bisect breakage target tree will
have to be rebased.

And if doing that anyway, I don't see any reason not
to merge everything through the vhost tree, esp
since I already put the patches there. Less work for
everyone involved.

-- 
MST
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-v2 0/6] vhost/scsi: Add T10 PI SGL passthrough support

2014-06-10 Thread Michael S. Tsirkin 
On Tue, Jun 10, 2014 at 10:39:17AM -0700, Nicholas A. Bellinger wrote:
 On Tue, 2014-06-10 at 16:02 +0300, Michael S. Tsirkin wrote:
  On Tue, Jun 10, 2014 at 09:52:17PM +1000, Stephen Rothwell wrote:
   Hi Michael,
   
   On Tue, 10 Jun 2014 12:42:54 +0300 Michael S. Tsirkin m...@redhat.com 
   wrote:
   
So I see two options:
- I go ahead with my changes and you with yours and let Linus resolve
  the conflict.  This means bisect build will be broken since the
  breakage will likely not be noticed until after the merge.
   
   Well, since the resolution is known, the one who submits their tree
   later should tell Linus (as suggested by Nicholas).  That is part of
   the point of the linux-next tree ... and therefore there would be no
   bisect problem.
   
 Stephen (CC'ed) has included a fix in today's linux-next for the merge
 conflict here:
 
 https://lkml.org/lkml/2014/6/10/3
 
 Please confirm, as it will be a pointer to Linus within the
 target-pending/for-next PULL request.

Yes but this does mean people trying to bisect will
hit build breakages, not nice.
   
   Not necessarily.
   
   -- 
   Cheers,
   Stephen Rothwells...@canb.auug.org.au
  
  
  I don't see how that's possible.
  Here's a point you might have missed.
  Nicholas's patch isn't just introducing a merge conflict.
  It is also buggy.
  Replacing bit access with has_feature silently fixes the bug.
  
  So if we want to avoid bisect breakage target tree will
  have to be rebased.
  
  And if doing that anyway, I don't see any reason not
  to merge everything through the vhost tree, esp
  since I already put the patches there. Less work for
  everyone involved.
  
 
 The problem is with Sagi's recent changes wrt to including T10 PI bytes
 into expected data transfer length in target-core, you'll end up
 introducing a different bug into your tree..  ;)
 
 Why don't I simply add Stephen's patch to use vhost_has_feature() in
 target-pending/for-next, and we just make sure that the vhost PULL
 request goes out after target-pending..?
 
 --nab

Because that depends on vhost API changes :)

-- 
MST
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-v2 0/6] vhost/scsi: Add T10 PI SGL passthrough support

2014-06-10 Thread Michael S. Tsirkin 
On Tue, Jun 10, 2014 at 10:39:17AM -0700, Nicholas A. Bellinger wrote:
 On Tue, 2014-06-10 at 16:02 +0300, Michael S. Tsirkin wrote:
  On Tue, Jun 10, 2014 at 09:52:17PM +1000, Stephen Rothwell wrote:
   Hi Michael,
   
   On Tue, 10 Jun 2014 12:42:54 +0300 Michael S. Tsirkin m...@redhat.com 
   wrote:
   
So I see two options:
- I go ahead with my changes and you with yours and let Linus resolve
  the conflict.  This means bisect build will be broken since the
  breakage will likely not be noticed until after the merge.
   
   Well, since the resolution is known, the one who submits their tree
   later should tell Linus (as suggested by Nicholas).  That is part of
   the point of the linux-next tree ... and therefore there would be no
   bisect problem.
   
 Stephen (CC'ed) has included a fix in today's linux-next for the merge
 conflict here:
 
 https://lkml.org/lkml/2014/6/10/3
 
 Please confirm, as it will be a pointer to Linus within the
 target-pending/for-next PULL request.

Yes but this does mean people trying to bisect will
hit build breakages, not nice.
   
   Not necessarily.
   
   -- 
   Cheers,
   Stephen Rothwells...@canb.auug.org.au
  
  
  I don't see how that's possible.
  Here's a point you might have missed.
  Nicholas's patch isn't just introducing a merge conflict.
  It is also buggy.
  Replacing bit access with has_feature silently fixes the bug.
  
  So if we want to avoid bisect breakage target tree will
  have to be rebased.
  
  And if doing that anyway, I don't see any reason not
  to merge everything through the vhost tree, esp
  since I already put the patches there. Less work for
  everyone involved.
  
 
 The problem is with Sagi's recent changes wrt to including T10 PI bytes
 into expected data transfer length in target-core, you'll end up
 introducing a different bug into your tree..  ;)

I thought you wanted to fix it after -rc1 anyway?

 Why don't I simply add Stephen's patch to use vhost_has_feature() in
 target-pending/for-next, and we just make sure that the vhost PULL
 request goes out after target-pending..?
 
 --nab

I can drop the PI feature bit in rc1. You will apply Sagi's changes and
then enable the feature in rc2?
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-v2 0/6] vhost/scsi: Add T10 PI SGL passthrough support

2014-06-11 Thread Michael S. Tsirkin 
On Tue, Jun 10, 2014 at 02:20:28PM -0700, Nicholas A. Bellinger wrote:
 On Tue, 2014-06-10 at 13:56 -0700, Linus Torvalds wrote:
  On Tue, Jun 10, 2014 at 1:25 PM, Nicholas A. Bellinger
  n...@linux-iscsi.org wrote:
  
   That would work, or I can simply include a pointer to Stephen's patch in
   the target-pending PULL request after the vhost API changes are merged
   and Linus can apply himself..
  
  Yes. That way I'll include it in the merge, and everything should just work.
  
 
 nod, sounds good.
 
 MST, please drop the target related patches from your tree, and go ahead
 and send your PULL request now.
 
 --nab

ack.
Doing it right now.

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PULL] vhost: infrastructure changes for 3.16

2014-06-11 Thread Michael S. Tsirkin 
Hi Linus,
Please pull the following.
Please note this needs to be merged before merging
target-pending PULL which Nicholas will be sending
out shortly.

Thanks!

The following changes since commit 1860e379875dfe7271c649058aeddffe5afd9d0d:

  Linux 3.15 (2014-06-08 11:19:54 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus

for you to fetch changes up to 47283bef7ed356629467d1fac61687756e48f254:

  vhost: move memory pointer to VQs (2014-06-09 16:21:07 +0300)


vhost: infrastructure changes for 3.16

This reworks vhost core dropping unnecessary RCU uses in favor of VQ mutexes
which are used on fast path anyway.  This fixes worst-case latency for users
which change the memory mappings a lot.
Memory allocation for vhost-net now supports fallback on vmalloc (same as for
vhost-scsi) this makes it possible to create the device on systems where memory
is very fragmented, with slightly lower performance.

Signed-off-by: Michael S. Tsirkin m...@redhat.com


Michael S. Tsirkin (4):
  vhost-net: extend device allocation to vmalloc
  vhost: replace rcu with mutex
  vhost: move acked_features to VQs
  vhost: move memory pointer to VQs

 drivers/vhost/vhost.h | 19 --
 drivers/vhost/net.c   | 35 ---
 drivers/vhost/scsi.c  | 26 --
 drivers/vhost/test.c  | 11 +++---
 drivers/vhost/vhost.c | 97 ++-
 5 files changed, 101 insertions(+), 87 deletions(-)
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 24/25] virtio_scsi: drop scan callback

2014-10-12 Thread Michael S. Tsirkin 
Enable VQs early like we do for restore.
This makes it possible to drop the scan callback,
moving scanning into the probe function, and making
code simpler.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/scsi/virtio_scsi.c | 23 +++
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 327eba0..5f022ff 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -860,17 +860,6 @@ static void virtscsi_init_vq(struct virtio_scsi_vq 
*virtscsi_vq,
virtscsi_vq-vq = vq;
 }
 
-static void virtscsi_scan(struct virtio_device *vdev)
-{
-   struct Scsi_Host *shost = virtio_scsi_host(vdev);
-   struct virtio_scsi *vscsi = shost_priv(shost);
-
-   if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
-   virtscsi_kick_event_all(vscsi);
-
-   scsi_scan_host(shost);
-}
-
 static void virtscsi_remove_vqs(struct virtio_device *vdev)
 {
struct Scsi_Host *sh = virtio_scsi_host(vdev);
@@ -1007,10 +996,13 @@ static int virtscsi_probe(struct virtio_device *vdev)
err = scsi_add_host(shost, vdev-dev);
if (err)
goto scsi_add_host_failed;
-   /*
-* scsi_scan_host() happens in virtscsi_scan() via virtio_driver-scan()
-* after VIRTIO_CONFIG_S_DRIVER_OK has been set..
-*/
+
+   virtio_enable_vqs_early(vdev);
+
+   if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
+   virtscsi_kick_event_all(vscsi);
+
+   scsi_scan_host(shost);
return 0;
 
 scsi_add_host_failed:
@@ -1090,7 +1082,6 @@ static struct virtio_driver virtio_scsi_driver = {
.driver.owner = THIS_MODULE,
.id_table = id_table,
.probe = virtscsi_probe,
-   .scan = virtscsi_scan,
 #ifdef CONFIG_PM_SLEEP
.freeze = virtscsi_freeze,
.restore = virtscsi_restore,
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 25/25] virtio-rng: refactor probe error handling

2014-10-12 Thread Michael S. Tsirkin 
Code like
vi-vq = NULL;
kfree(vi)
does not make sense.

Clean it up, use goto error labels for cleanup.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/char/hw_random/virtio-rng.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/char/hw_random/virtio-rng.c 
b/drivers/char/hw_random/virtio-rng.c
index 132c9cc..72295ea 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -109,8 +109,8 @@ static int probe_common(struct virtio_device *vdev)
 
vi-index = index = ida_simple_get(rng_index_ida, 0, 0, GFP_KERNEL);
if (index  0) {
-   kfree(vi);
-   return index;
+   err = index;
+   goto err_ida;
}
sprintf(vi-name, virtio_rng.%d, index);
init_completion(vi-have_data);
@@ -128,13 +128,16 @@ static int probe_common(struct virtio_device *vdev)
vi-vq = virtio_find_single_vq(vdev, random_recv_done, input);
if (IS_ERR(vi-vq)) {
err = PTR_ERR(vi-vq);
-   vi-vq = NULL;
-   kfree(vi);
-   ida_simple_remove(rng_index_ida, index);
-   return err;
+   goto err_find;
}
 
return 0;
+
+err_find:
+   ida_simple_remove(rng_index_ida, index);
+err_ida:
+   kfree(vi);
+   return err;
 }
 
 static void remove_common(struct virtio_device *vdev)
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 20/25] virtio_net: enable VQs early on restore

2014-10-12 Thread Michael S. Tsirkin 
virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after restore returns, virtio net violated this
rule by using receive VQs within restore.

To fix, call virtio_enable_vqs_early before using VQs.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/net/virtio_net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3551417..6b6e136 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1912,6 +1912,8 @@ static int virtnet_restore(struct virtio_device *vdev)
if (err)
return err;
 
+   virtio_enable_vqs_early(vdev);
+
if (netif_running(vi-dev)) {
for (i = 0; i  vi-curr_queue_pairs; i++)
if (!try_fill_recv(vi-rq[i], GFP_KERNEL))
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 22/25] virtio_scsi: fix race on device removal

2014-10-12 Thread Michael S. Tsirkin 
We cancel event work on device removal, but an interrupt
could trigger immediately after this, and queue it
again.

To fix, set a flag.

Loosely based on patch by Paolo Bonzini

Signed-off-by: Paolo Bonzini pbonz...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/scsi/virtio_scsi.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 501838d..327eba0 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -110,6 +110,9 @@ struct virtio_scsi {
/* CPU hotplug notifier */
struct notifier_block nb;
 
+   /* Protected by event_vq lock */
+   bool stop_events;
+
struct virtio_scsi_vq ctrl_vq;
struct virtio_scsi_vq event_vq;
struct virtio_scsi_vq req_vqs[];
@@ -303,6 +306,11 @@ static void virtscsi_cancel_event_work(struct virtio_scsi 
*vscsi)
 {
int i;
 
+   /* Stop scheduling work before calling cancel_work_sync.  */
+   spin_lock_irq(vscsi-event_vq.vq_lock);
+   vscsi-stop_events = true;
+   spin_unlock_irq(vscsi-event_vq.vq_lock);
+
for (i = 0; i  VIRTIO_SCSI_EVENT_LEN; i++)
cancel_work_sync(vscsi-event_list[i].work);
 }
@@ -390,7 +398,8 @@ static void virtscsi_complete_event(struct virtio_scsi 
*vscsi, void *buf)
 {
struct virtio_scsi_event_node *event_node = buf;
 
-   queue_work(system_freezable_wq, event_node-work);
+   if (!vscsi-stop_events)
+   queue_work(system_freezable_wq, event_node-work);
 }
 
 static void virtscsi_event_done(struct virtqueue *vq)
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 21/25] virito_scsi: use freezable WQ for events

2014-10-12 Thread Michael S. Tsirkin 
From: Paolo Bonzini pbonz...@redhat.com

Michael S. Tsirkin noticed a race condition:
we reset device on freeze, but system WQ is still
running so it might try adding bufs to a VQ meanwhile.

To fix, switch to handling events from the freezable WQ.

Reported-by: Michael S. Tsirkin m...@redhat.com
Signed-off-by: Paolo Bonzini pbonz...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/scsi/virtio_scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 2b565b3..501838d 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -390,7 +390,7 @@ static void virtscsi_complete_event(struct virtio_scsi 
*vscsi, void *buf)
 {
struct virtio_scsi_event_node *event_node = buf;
 
-   schedule_work(event_node-work);
+   queue_work(system_freezable_wq, event_node-work);
 }
 
 static void virtscsi_event_done(struct virtqueue *vq)
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 15/25] virtio_net: fix use after free on allocation failure

2014-10-12 Thread Michael S. Tsirkin 
In the extremely unlikely event that driver initialization fails after
RX buffers are added, virtio net frees RX buffers while VQs are
still active, potentially causing device to use a freed buffer.

To fix, reset device first - same as we do on device removal.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/net/virtio_net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 430f3ae..3551417 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1830,6 +1830,8 @@ static int virtnet_probe(struct virtio_device *vdev)
return 0;
 
 free_recv_bufs:
+   vi-vdev-config-reset(vdev);
+
free_receive_bufs(vi);
unregister_netdev(dev);
 free_vqs:
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 18/25] virtio_scsi: enable VQs early on restore

2014-10-12 Thread Michael S. Tsirkin 
virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after restore returns, virtio scsi violated
this rule on restore by kicking event vq within restore.

To fix, call virtio_enable_vqs_early before using event queue.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/scsi/virtio_scsi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 0642ce3..2b565b3 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -1054,6 +1054,8 @@ static int virtscsi_restore(struct virtio_device *vdev)
return err;
}
 
+   virtio_enable_vqs_early(vdev);
+
if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
virtscsi_kick_event_all(vscsi);
 
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 16/25] virtio_scsi: move kick event out from virtscsi_init

2014-10-12 Thread Michael S. Tsirkin 
We currently kick event within virtscsi_init,
before host is fully initialized.

This can in theory confuse guest if device
consumes the buffers immediately.

To fix,  move virtscsi_kick_event_all out to scan/restore.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/scsi/virtio_scsi.c | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index eee1bc0..0642ce3 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -853,7 +853,11 @@ static void virtscsi_init_vq(struct virtio_scsi_vq 
*virtscsi_vq,
 
 static void virtscsi_scan(struct virtio_device *vdev)
 {
-   struct Scsi_Host *shost = (struct Scsi_Host *)vdev-priv;
+   struct Scsi_Host *shost = virtio_scsi_host(vdev);
+   struct virtio_scsi *vscsi = shost_priv(shost);
+
+   if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
+   virtscsi_kick_event_all(vscsi);
 
scsi_scan_host(shost);
 }
@@ -916,9 +920,6 @@ static int virtscsi_init(struct virtio_device *vdev,
virtscsi_config_set(vdev, cdb_size, VIRTIO_SCSI_CDB_SIZE);
virtscsi_config_set(vdev, sense_size, VIRTIO_SCSI_SENSE_SIZE);
 
-   if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
-   virtscsi_kick_event_all(vscsi);
-
err = 0;
 
 out:
@@ -1048,8 +1049,13 @@ static int virtscsi_restore(struct virtio_device *vdev)
return err;
 
err = register_hotcpu_notifier(vscsi-nb);
-   if (err)
+   if (err) {
vdev-config-del_vqs(vdev);
+   return err;
+   }
+
+   if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
+   virtscsi_kick_event_all(vscsi);
 
return err;
 }
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 09/25] virtio_net: minor cleanup

2014-10-12 Thread Michael S. Tsirkin 
goto done;
done:
return;
is ugly, it was put there to make diff review easier.
replace by open-coded return.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Acked-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/net/virtio_net.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 23e4a69..ef04d23 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1403,7 +1403,7 @@ static void virtnet_config_changed_work(struct 
work_struct *work)
 
if (virtio_cread_feature(vi-vdev, VIRTIO_NET_F_STATUS,
 struct virtio_net_config, status, v)  0)
-   goto done;
+   return;
 
if (v  VIRTIO_NET_S_ANNOUNCE) {
netdev_notify_peers(vi-dev);
@@ -1414,7 +1414,7 @@ static void virtnet_config_changed_work(struct 
work_struct *work)
v = VIRTIO_NET_S_LINK_UP;
 
if (vi-status == v)
-   goto done;
+   return;
 
vi-status = v;
 
@@ -1425,8 +1425,6 @@ static void virtnet_config_changed_work(struct 
work_struct *work)
netif_carrier_off(vi-dev);
netif_tx_stop_all_queues(vi-dev);
}
-done:
-   return;
 }
 
 static void virtnet_config_changed(struct virtio_device *vdev)
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 11/25] virtio_net: enable VQs early

2014-10-12 Thread Michael S. Tsirkin 
virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after probe returns, virtio net violated this
rule by using receive VQs within probe.

To fix, call virtio_enable_vqs_early before using VQs.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/net/virtio_net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ef04d23..430f3ae 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1792,6 +1792,8 @@ static int virtnet_probe(struct virtio_device *vdev)
goto free_vqs;
}
 
+   virtio_enable_vqs_early(vdev);
+
/* Last of all, set up some receive buffers. */
for (i = 0; i  vi-curr_queue_pairs; i++) {
try_fill_recv(vi-rq[i], GFP_KERNEL);
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 07/25] virtio_net: drop config_enable

2014-10-12 Thread Michael S. Tsirkin 
Now that virtio core ensures config changes don't arrive during probing,
drop config_enable flag in virtio net.
On removal, flush is now sufficient to guarantee that no change work is
queued.

This help simplify the driver, and will allow setting DRIVER_OK earlier
without losing config change notifications.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/net/virtio_net.c | 27 ---
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 59caa06..743fb04 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -123,9 +123,6 @@ struct virtnet_info {
/* Host can handle any s/g split between our header and packet data */
bool any_header_sg;
 
-   /* enable config space updates */
-   bool config_enable;
-
/* Active statistics */
struct virtnet_stats __percpu *stats;
 
@@ -1408,9 +1405,6 @@ static void virtnet_config_changed_work(struct 
work_struct *work)
u16 v;
 
mutex_lock(vi-config_lock);
-   if (!vi-config_enable)
-   goto done;
-
if (virtio_cread_feature(vi-vdev, VIRTIO_NET_F_STATUS,
 struct virtio_net_config, status, v)  0)
goto done;
@@ -1758,7 +1752,6 @@ static int virtnet_probe(struct virtio_device *vdev)
}
 
mutex_init(vi-config_lock);
-   vi-config_enable = true;
INIT_WORK(vi-config_work, virtnet_config_changed_work);
 
/* If we can receive ANY GSO packets, we must allocate large ones. */
@@ -1875,17 +1868,13 @@ static void virtnet_remove(struct virtio_device *vdev)
 
unregister_hotcpu_notifier(vi-nb);
 
-   /* Prevent config work handler from accessing the device. */
-   mutex_lock(vi-config_lock);
-   vi-config_enable = false;
-   mutex_unlock(vi-config_lock);
+   /* Make sure no work handler is accessing the device. */
+   flush_work(vi-config_work);
 
unregister_netdev(vi-dev);
 
remove_vq_common(vi);
 
-   flush_work(vi-config_work);
-
free_percpu(vi-stats);
free_netdev(vi-dev);
 }
@@ -1898,10 +1887,8 @@ static int virtnet_freeze(struct virtio_device *vdev)
 
unregister_hotcpu_notifier(vi-nb);
 
-   /* Prevent config work handler from accessing the device */
-   mutex_lock(vi-config_lock);
-   vi-config_enable = false;
-   mutex_unlock(vi-config_lock);
+   /* Make sure no work handler is accessing the device */
+   flush_work(vi-config_work);
 
netif_device_detach(vi-dev);
cancel_delayed_work_sync(vi-refill);
@@ -1916,8 +1903,6 @@ static int virtnet_freeze(struct virtio_device *vdev)
 
remove_vq_common(vi);
 
-   flush_work(vi-config_work);
-
return 0;
 }
 
@@ -1941,10 +1926,6 @@ static int virtnet_restore(struct virtio_device *vdev)
 
netif_device_attach(vi-dev);
 
-   mutex_lock(vi-config_lock);
-   vi-config_enable = true;
-   mutex_unlock(vi-config_lock);
-
rtnl_lock();
virtnet_set_queues(vi, vi-curr_queue_pairs);
rtnl_unlock();
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 14/25] 9p/trans_virtio: enable VQs early

2014-10-12 Thread Michael S. Tsirkin 
virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after probe returns, but virtio 9p device
adds self to channel list within probe, at which point VQ can be
used in violation of the spec.

To fix, call virtio_enable_vqs_early before using VQs.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 net/9p/trans_virtio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 6940d8f..766ba48 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -575,6 +575,8 @@ static int p9_virtio_probe(struct virtio_device *vdev)
/* Ceiling limit to avoid denial of service attacks */
chan-p9_max_pages = nr_free_buffer_pages()/4;
 
+   virtio_enable_vqs_early(vdev);
+
mutex_lock(virtio_9p_lock);
list_add_tail(chan-chan_list, virtio_chan_list);
mutex_unlock(virtio_9p_lock);
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 10/25] virtio: add API to enable VQs early

2014-10-12 Thread Michael S. Tsirkin 
virtio spec 0.9.X requires DRIVER_OK to be set before
VQs are used, but some drivers use VQs before probe
function returns.
Since DRIVER_OK is set after probe, this violates the spec.

Even though under virtio 1.0 transitional devices support this
behaviour, we want to make it possible for those early callers to become
spec compliant and eventually support non-transitional devices.

Add API for drivers to call before using VQs.

Sets DRIVER_OK internally.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 include/linux/virtio_config.h | 17 +
 1 file changed, 17 insertions(+)

diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index e8f8f71..e36403b 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -109,6 +109,23 @@ struct virtqueue *virtio_find_single_vq(struct 
virtio_device *vdev,
return vq;
 }
 
+/**
+ * virtio_enable_vqs_early - enable vq use in probe function
+ * @vdev: the device
+ *
+ * Driver must call this to use vqs in the probe function.
+ *
+ * Note: vqs are enabled automatically after probe returns.
+ */
+static inline
+void virtio_enable_vqs_early(struct virtio_device *dev)
+{
+   unsigned status = dev-config-get_status(dev);
+
+   BUG_ON(status  VIRTIO_CONFIG_S_DRIVER_OK);
+   dev-config-set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK);
+}
+
 static inline
 const char *virtio_bus_name(struct virtio_device *vdev)
 {
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 04/25] virtio: defer config changed notifications

2014-10-12 Thread Michael S. Tsirkin 
Defer config changed notifications that arrive during
probe/scan/freeze/restore.

This will allow drivers to set DRIVER_OK earlier, without worrying about
racing with config change interrupts.

This change will also benefit old hypervisors (before 2009)
that send interrupts without checking DRIVER_OK: previously,
the callback could race with driver-specific initialization.

This will also help simplify drivers.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 include/linux/virtio.h  |  6 ++
 drivers/virtio/virtio.c | 57 +
 2 files changed, 54 insertions(+), 9 deletions(-)

diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 8df7ba8..5636b11 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -79,6 +79,9 @@ bool virtqueue_is_broken(struct virtqueue *vq);
  * virtio_device - representation of a device using virtio
  * @index: unique position on the virtio bus
  * @failed: saved value for CONFIG_S_FAILED bit (for restore)
+ * @config_enabled: configuration change reporting enabled
+ * @config_changed: configuration change reported while disabled
+ * @config_lock: protects configuration change reporting
  * @dev: underlying device.
  * @id: the device type identification (used to match it with a driver).
  * @config: the configuration ops for this device.
@@ -90,6 +93,9 @@ bool virtqueue_is_broken(struct virtqueue *vq);
 struct virtio_device {
int index;
bool failed;
+   bool config_enabled;
+   bool config_changed;
+   spinlock_t config_lock;
struct device dev;
struct virtio_device_id id;
const struct virtio_config_ops *config;
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 8216b73..2536701 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -117,6 +117,42 @@ void virtio_check_driver_offered_feature(const struct 
virtio_device *vdev,
 }
 EXPORT_SYMBOL_GPL(virtio_check_driver_offered_feature);
 
+static void __virtio_config_changed(struct virtio_device *dev)
+{
+   struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
+
+   if (!dev-config_enabled)
+   dev-config_changed = true;
+   else if (drv  drv-config_changed)
+   drv-config_changed(dev);
+}
+
+void virtio_config_changed(struct virtio_device *dev)
+{
+   unsigned long flags;
+
+   spin_lock_irqsave(dev-config_lock, flags);
+   __virtio_config_changed(dev);
+   spin_unlock_irqrestore(dev-config_lock, flags);
+}
+EXPORT_SYMBOL_GPL(virtio_config_changed);
+
+static void virtio_config_disable(struct virtio_device *dev)
+{
+   spin_lock_irq(dev-config_lock);
+   dev-config_enabled = false;
+   spin_unlock_irq(dev-config_lock);
+}
+
+static void virtio_config_enable(struct virtio_device *dev)
+{
+   spin_lock_irq(dev-config_lock);
+   dev-config_enabled = true;
+   __virtio_config_changed(dev);
+   dev-config_changed = false;
+   spin_unlock_irq(dev-config_lock);
+}
+
 static int virtio_dev_probe(struct device *_d)
 {
int err, i;
@@ -153,6 +189,8 @@ static int virtio_dev_probe(struct device *_d)
add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
if (drv-scan)
drv-scan(dev);
+
+   virtio_config_enable(dev);
}
 
return err;
@@ -163,6 +201,8 @@ static int virtio_dev_remove(struct device *_d)
struct virtio_device *dev = dev_to_virtio(_d);
struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
 
+   virtio_config_disable(dev);
+
drv-remove(dev);
 
/* Driver should have reset device. */
@@ -211,6 +251,10 @@ int register_virtio_device(struct virtio_device *dev)
dev-index = err;
dev_set_name(dev-dev, virtio%u, dev-index);
 
+   spin_lock_init(dev-config_lock);
+   dev-config_enabled = false;
+   dev-config_changed = false;
+
/* We always start by resetting the device, in case a previous
 * driver messed it up.  This also tests that code path a little. */
dev-config-reset(dev);
@@ -239,20 +283,13 @@ void unregister_virtio_device(struct virtio_device *dev)
 }
 EXPORT_SYMBOL_GPL(unregister_virtio_device);
 
-void virtio_config_changed(struct virtio_device *dev)
-{
-   struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
-
-   if (drv  drv-config_changed)
-   drv-config_changed(dev);
-}
-EXPORT_SYMBOL_GPL(virtio_config_changed);
-
 #ifdef CONFIG_PM_SLEEP
 int virtio_device_freeze(struct virtio_device *dev)
 {
struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
 
+   virtio_config_disable(dev);
+
dev-failed = dev-config-get_status(dev)  VIRTIO_CONFIG_S_FAILED;
 
if (drv  drv-freeze)
@@ -297,6 +334,8 @@ int virtio_device_restore(struct virtio_device *dev)
/* Finally, tell the device we're all set

[PATCH v3 05/25] virtio_blk: drop config_enable

2014-10-12 Thread Michael S. Tsirkin 
Now that virtio core ensures config changes don't
arrive during probing, drop config_enable flag
in virtio blk.
On removal, flush is now sufficient to guarantee that
no change work is queued.

This help simplify the driver, and will allow
setting DRIVER_OK earlier without losing config
change notifications.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/block/virtio_blk.c | 23 ---
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 0a58140..91272f1a 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -44,9 +44,6 @@ struct virtio_blk
/* Lock for config space updates */
struct mutex config_lock;
 
-   /* enable config space updates */
-   bool config_enable;
-
/* What host tells us, plus 2 for header  tailer. */
unsigned int sg_elems;
 
@@ -348,8 +345,6 @@ static void virtblk_config_changed_work(struct work_struct 
*work)
u64 capacity, size;
 
mutex_lock(vblk-config_lock);
-   if (!vblk-config_enable)
-   goto done;
 
/* Host must always specify the capacity. */
virtio_cread(vdev, struct virtio_blk_config, capacity, capacity);
@@ -374,7 +369,7 @@ static void virtblk_config_changed_work(struct work_struct 
*work)
set_capacity(vblk-disk, capacity);
revalidate_disk(vblk-disk);
kobject_uevent_env(disk_to_dev(vblk-disk)-kobj, KOBJ_CHANGE, envp);
-done:
+
mutex_unlock(vblk-config_lock);
 }
 
@@ -609,7 +604,6 @@ static int virtblk_probe(struct virtio_device *vdev)
mutex_init(vblk-config_lock);
 
INIT_WORK(vblk-config_work, virtblk_config_changed_work);
-   vblk-config_enable = true;
 
err = init_vq(vblk);
if (err)
@@ -771,10 +765,8 @@ static void virtblk_remove(struct virtio_device *vdev)
int index = vblk-index;
int refc;
 
-   /* Prevent config work handler from accessing the device. */
-   mutex_lock(vblk-config_lock);
-   vblk-config_enable = false;
-   mutex_unlock(vblk-config_lock);
+   /* Make sure no work handler is accessing the device. */
+   flush_work(vblk-config_work);
 
del_gendisk(vblk-disk);
blk_cleanup_queue(vblk-disk-queue);
@@ -784,8 +776,6 @@ static void virtblk_remove(struct virtio_device *vdev)
/* Stop all the virtqueues. */
vdev-config-reset(vdev);
 
-   flush_work(vblk-config_work);
-
refc = atomic_read(disk_to_dev(vblk-disk)-kobj.kref.refcount);
put_disk(vblk-disk);
vdev-config-del_vqs(vdev);
@@ -805,11 +795,7 @@ static int virtblk_freeze(struct virtio_device *vdev)
/* Ensure we don't receive any more interrupts */
vdev-config-reset(vdev);
 
-   /* Prevent config work handler from accessing the device. */
-   mutex_lock(vblk-config_lock);
-   vblk-config_enable = false;
-   mutex_unlock(vblk-config_lock);
-
+   /* Make sure no work handler is accessing the device. */
flush_work(vblk-config_work);
 
blk_mq_stop_hw_queues(vblk-disk-queue);
@@ -823,7 +809,6 @@ static int virtblk_restore(struct virtio_device *vdev)
struct virtio_blk *vblk = vdev-priv;
int ret;
 
-   vblk-config_enable = true;
ret = init_vq(vdev-priv);
if (!ret)
blk_mq_start_stopped_hw_queues(vblk-disk-queue, true);
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 06/25] virtio-blk: drop config_mutex

2014-10-12 Thread Michael S. Tsirkin 
config_mutex served two purposes: prevent multiple concurrent config
change handlers, and synchronize access to config_enable flag.

Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1
workqueue: make all workqueues non-reentrant
all workqueues are non-reentrant, and config_enable
is now gone.

Get rid of the unnecessary lock.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/block/virtio_blk.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 91272f1a..89ba8d6 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -41,9 +41,6 @@ struct virtio_blk
/* Process context for config space updates */
struct work_struct config_work;
 
-   /* Lock for config space updates */
-   struct mutex config_lock;
-
/* What host tells us, plus 2 for header  tailer. */
unsigned int sg_elems;
 
@@ -344,8 +341,6 @@ static void virtblk_config_changed_work(struct work_struct 
*work)
char *envp[] = { RESIZE=1, NULL };
u64 capacity, size;
 
-   mutex_lock(vblk-config_lock);
-
/* Host must always specify the capacity. */
virtio_cread(vdev, struct virtio_blk_config, capacity, capacity);
 
@@ -369,8 +364,6 @@ static void virtblk_config_changed_work(struct work_struct 
*work)
set_capacity(vblk-disk, capacity);
revalidate_disk(vblk-disk);
kobject_uevent_env(disk_to_dev(vblk-disk)-kobj, KOBJ_CHANGE, envp);
-
-   mutex_unlock(vblk-config_lock);
 }
 
 static void virtblk_config_changed(struct virtio_device *vdev)
@@ -601,7 +594,6 @@ static int virtblk_probe(struct virtio_device *vdev)
 
vblk-vdev = vdev;
vblk-sg_elems = sg_elems;
-   mutex_init(vblk-config_lock);
 
INIT_WORK(vblk-config_work, virtblk_config_changed_work);
 
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 03/25] virtio-pci: move freeze/restore to virtio core

2014-10-12 Thread Michael S. Tsirkin 
This is in preparation to extending config changed event handling
in core.
Wrapping these in an API also seems to make for a cleaner code.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 include/linux/virtio.h  |  6 +
 drivers/virtio/virtio.c | 54 +
 drivers/virtio/virtio_pci.c | 54 ++---
 3 files changed, 62 insertions(+), 52 deletions(-)

diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 3c19bd3..8df7ba8 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -78,6 +78,7 @@ bool virtqueue_is_broken(struct virtqueue *vq);
 /**
  * virtio_device - representation of a device using virtio
  * @index: unique position on the virtio bus
+ * @failed: saved value for CONFIG_S_FAILED bit (for restore)
  * @dev: underlying device.
  * @id: the device type identification (used to match it with a driver).
  * @config: the configuration ops for this device.
@@ -88,6 +89,7 @@ bool virtqueue_is_broken(struct virtqueue *vq);
  */
 struct virtio_device {
int index;
+   bool failed;
struct device dev;
struct virtio_device_id id;
const struct virtio_config_ops *config;
@@ -109,6 +111,10 @@ void unregister_virtio_device(struct virtio_device *dev);
 void virtio_break_device(struct virtio_device *dev);
 
 void virtio_config_changed(struct virtio_device *dev);
+#ifdef CONFIG_PM_SLEEP
+int virtio_device_freeze(struct virtio_device *dev);
+int virtio_device_restore(struct virtio_device *dev);
+#endif
 
 /**
  * virtio_driver - operations for a virtio I/O driver
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 3980687..8216b73 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -248,6 +248,60 @@ void virtio_config_changed(struct virtio_device *dev)
 }
 EXPORT_SYMBOL_GPL(virtio_config_changed);
 
+#ifdef CONFIG_PM_SLEEP
+int virtio_device_freeze(struct virtio_device *dev)
+{
+   struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
+
+   dev-failed = dev-config-get_status(dev)  VIRTIO_CONFIG_S_FAILED;
+
+   if (drv  drv-freeze)
+   return drv-freeze(dev);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(virtio_device_freeze);
+
+int virtio_device_restore(struct virtio_device *dev)
+{
+   struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
+
+   /* We always start by resetting the device, in case a previous
+* driver messed it up. */
+   dev-config-reset(dev);
+
+   /* Acknowledge that we've seen the device. */
+   add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
+
+   /* Maybe driver failed before freeze.
+* Restore the failed status, for debugging. */
+   if (dev-failed)
+   add_status(dev, VIRTIO_CONFIG_S_FAILED);
+
+   if (!drv)
+   return 0;
+
+   /* We have a driver! */
+   add_status(dev, VIRTIO_CONFIG_S_DRIVER);
+
+   dev-config-finalize_features(dev);
+
+   if (drv-restore) {
+   int ret = drv-restore(dev);
+   if (ret) {
+   add_status(dev, VIRTIO_CONFIG_S_FAILED);
+   return ret;
+   }
+   }
+
+   /* Finally, tell the device we're all set */
+   add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(virtio_device_restore);
+#endif
+
 static int virtio_init(void)
 {
if (bus_register(virtio_bus) != 0)
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index f39f4e7..d34ebfa 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -57,9 +57,6 @@ struct virtio_pci_device
/* Vectors allocated, excluding per-vq vectors if any */
unsigned msix_used_vectors;
 
-   /* Status saved during hibernate/restore */
-   u8 saved_status;
-
/* Whether we have vector per vq */
bool per_vq_vectors;
 };
@@ -764,16 +761,9 @@ static int virtio_pci_freeze(struct device *dev)
 {
struct pci_dev *pci_dev = to_pci_dev(dev);
struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
-   struct virtio_driver *drv;
int ret;
 
-   drv = container_of(vp_dev-vdev.dev.driver,
-  struct virtio_driver, driver);
-
-   ret = 0;
-   vp_dev-saved_status = vp_get_status(vp_dev-vdev);
-   if (drv  drv-freeze)
-   ret = drv-freeze(vp_dev-vdev);
+   ret = virtio_device_freeze(vp_dev-vdev);
 
if (!ret)
pci_disable_device(pci_dev);
@@ -784,54 +774,14 @@ static int virtio_pci_restore(struct device *dev)
 {
struct pci_dev *pci_dev = to_pci_dev(dev);
struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
-   struct virtio_driver *drv;
-   unsigned status = 0;
int ret;
 
-   drv = container_of(vp_dev-vdev.dev.driver,
-  struct

[PATCH v3 08/25] virtio-net: drop config_mutex

2014-10-12 Thread Michael S. Tsirkin 
config_mutex served two purposes: prevent multiple concurrent config
change handlers, and synchronize access to config_enable flag.

Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1
workqueue: make all workqueues non-reentrant
all workqueues are non-reentrant, and config_enable
is now gone.

Get rid of the unnecessary lock.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/net/virtio_net.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 743fb04..23e4a69 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -132,9 +132,6 @@ struct virtnet_info {
/* Work struct for config space updates */
struct work_struct config_work;
 
-   /* Lock for config space updates */
-   struct mutex config_lock;
-
/* Does the affinity hint is set for virtqueues? */
bool affinity_hint_set;
 
@@ -1404,7 +1401,6 @@ static void virtnet_config_changed_work(struct 
work_struct *work)
container_of(work, struct virtnet_info, config_work);
u16 v;
 
-   mutex_lock(vi-config_lock);
if (virtio_cread_feature(vi-vdev, VIRTIO_NET_F_STATUS,
 struct virtio_net_config, status, v)  0)
goto done;
@@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct 
work_struct *work)
netif_tx_stop_all_queues(vi-dev);
}
 done:
-   mutex_unlock(vi-config_lock);
+   return;
 }
 
 static void virtnet_config_changed(struct virtio_device *vdev)
@@ -1751,7 +1747,6 @@ static int virtnet_probe(struct virtio_device *vdev)
u64_stats_init(virtnet_stats-rx_syncp);
}
 
-   mutex_init(vi-config_lock);
INIT_WORK(vi-config_work, virtnet_config_changed_work);
 
/* If we can receive ANY GSO packets, we must allocate large ones. */
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 00/25] virtio: fix spec compliance issues

2014-10-12 Thread Michael S. Tsirkin 
Rusty, please review this, and consider for this merge window.

This fixes the following virtio spec compliance issues:
1. on restore, drivers use device before setting ACKNOWLEDGE and DRIVER bits
2. on probe, drivers aren't prepared to handle config interrupts
   arriving before probe returns
3. on probe, drivers use device before DRIVER_OK it set

Note that 1 is a clear violation of virtio spec 0.9 and 1.0,
so I am proposing the fix for stable. OTOH 2 is against 1.0 rules but
is a known documented bug in many drivers, so let's fix it going
forward, but it does not seem to be worth it to backport
the changes.

An error handling bugfix for virtio-net and a fix for a
theoretical race condition in virtio scsi is also included.

2 is merely a theoretical race condition, but it seems important
to address to make sure that changes to address 3 do not introduce
instability.

I also included patch to drop scan callback use from virtio
scsi: using this callback creates asymmetry between probe
and resume, and we actually had to add code comments so
people can figure out the flow. Code flow becomes clearer
with the new API.

After this change, the only user of the scan callback is virtio rng.
It's possible to modify it trivially and then drop scan callback
from core, but I'm rather inclined to look at ways to
make some rng core changes so that we don't need to
have so many variables tracking device state.
So this is deferred for now.

Michael S. Tsirkin (24):
  virtio_pci: fix virtio spec compliance on restore
  virtio: unify config_changed handling
  virtio-pci: move freeze/restore to virtio core
  virtio: defer config changed notifications
  virtio_blk: drop config_enable
  virtio-blk: drop config_mutex
  virtio_net: drop config_enable
  virtio-net: drop config_mutex
  virtio_net: minor cleanup
  virtio: add API to enable VQs early
  virtio_net: enable VQs early
  virtio_blk: enable VQs early
  virtio_console: enable VQs early
  9p/trans_virtio: enable VQs early
  virtio_net: fix use after free on allocation failure
  virtio_scsi: move kick event out from virtscsi_init
  virtio_blk: enable VQs early on restore
  virtio_scsi: enable VQs early on restore
  virtio_console: enable VQs early on restore
  virtio_net: enable VQs early on restore
  virtio_scsi: fix race on device removal
  virtio_balloon: enable VQs early on restore
  virtio_scsi: drop scan callback
  virtio-rng: refactor probe error handling

Paolo Bonzini (1):
  virito_scsi: use freezable WQ for events

 include/linux/virtio.h  |  14 +
 include/linux/virtio_config.h   |  17 ++
 drivers/block/virtio_blk.c  |  40 --
 drivers/char/hw_random/virtio-rng.c |  15 +++---
 drivers/char/virtio_console.c   |   4 ++
 drivers/misc/mic/card/mic_virtio.c  |   6 +--
 drivers/net/virtio_net.c|  44 +---
 drivers/s390/kvm/kvm_virtio.c   |   9 +---
 drivers/s390/kvm/virtio_ccw.c   |   6 +--
 drivers/scsi/virtio_scsi.c  |  42 +--
 drivers/virtio/virtio.c | 102 
 drivers/virtio/virtio_balloon.c |   2 +
 drivers/virtio/virtio_mmio.c|   7 +--
 drivers/virtio/virtio_pci.c |  33 ++--
 net/9p/trans_virtio.c   |   2 +
 15 files changed, 206 insertions(+), 137 deletions(-)

-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 02/25] virtio: unify config_changed handling

2014-10-12 Thread Michael S. Tsirkin 
Replace duplicated code in all transports with a single wrapper in
virtio.c.

The only functional change is in virtio_mmio.c: if a buggy device sends
us an interrupt before driver is set, we previously returned IRQ_NONE,
now we return IRQ_HANDLED.

As this must not happen in practice, this does not look like a big deal.

See also commit 3fff0179e33cd7d0a688dab65700c46ad089e934
virtio-pci: do not oops on config change if driver not loaded.
for the original motivation behind the driver check.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 include/linux/virtio.h | 2 ++
 drivers/misc/mic/card/mic_virtio.c | 6 +-
 drivers/s390/kvm/kvm_virtio.c  | 9 +
 drivers/s390/kvm/virtio_ccw.c  | 6 +-
 drivers/virtio/virtio.c| 9 +
 drivers/virtio/virtio_mmio.c   | 7 ++-
 drivers/virtio/virtio_pci.c| 6 +-
 7 files changed, 17 insertions(+), 28 deletions(-)

diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index b46671e..3c19bd3 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -108,6 +108,8 @@ void unregister_virtio_device(struct virtio_device *dev);
 
 void virtio_break_device(struct virtio_device *dev);
 
+void virtio_config_changed(struct virtio_device *dev);
+
 /**
  * virtio_driver - operations for a virtio I/O driver
  * @driver: underlying device driver (populate name and owner).
diff --git a/drivers/misc/mic/card/mic_virtio.c 
b/drivers/misc/mic/card/mic_virtio.c
index f14b600..e647947 100644
--- a/drivers/misc/mic/card/mic_virtio.c
+++ b/drivers/misc/mic/card/mic_virtio.c
@@ -462,16 +462,12 @@ static void mic_handle_config_change(struct 
mic_device_desc __iomem *d,
struct mic_device_ctrl __iomem *dc
= (void __iomem *)d + mic_aligned_desc_size(d);
struct mic_vdev *mvdev = (struct mic_vdev *)ioread64(dc-vdev);
-   struct virtio_driver *drv;
 
if (ioread8(dc-config_change) != MIC_VIRTIO_PARAM_CONFIG_CHANGED)
return;
 
dev_dbg(mdrv-dev, %s %d\n, __func__, __LINE__);
-   drv = container_of(mvdev-vdev.dev.driver,
-   struct virtio_driver, driver);
-   if (drv-config_changed)
-   drv-config_changed(mvdev-vdev);
+   virtio_config_changed(mvdev-vdev);
iowrite8(1, dc-guest_ack);
 }
 
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index a134965..6431290 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -406,15 +406,8 @@ static void kvm_extint_handler(struct ext_code ext_code,
 
switch (param) {
case VIRTIO_PARAM_CONFIG_CHANGED:
-   {
-   struct virtio_driver *drv;
-   drv = container_of(vq-vdev-dev.driver,
-  struct virtio_driver, driver);
-   if (drv-config_changed)
-   drv-config_changed(vq-vdev);
-
+   virtio_config_changed(vq-vdev);
break;
-   }
case VIRTIO_PARAM_DEV_ADD:
schedule_work(hotplug_work);
break;
diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index d2c0b44..6cbe6ef 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -940,11 +940,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
vring_interrupt(0, vq);
}
if (test_bit(0, vcdev-indicators2)) {
-   drv = container_of(vcdev-vdev.dev.driver,
-  struct virtio_driver, driver);
-
-   if (drv  drv-config_changed)
-   drv-config_changed(vcdev-vdev);
+   virtio_config_changed(vcdev-vdev);
clear_bit(0, vcdev-indicators2);
}
 }
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index fed0ce1..3980687 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -239,6 +239,15 @@ void unregister_virtio_device(struct virtio_device *dev)
 }
 EXPORT_SYMBOL_GPL(unregister_virtio_device);
 
+void virtio_config_changed(struct virtio_device *dev)
+{
+   struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
+
+   if (drv  drv-config_changed)
+   drv-config_changed(dev);
+}
+EXPORT_SYMBOL_GPL(virtio_config_changed);
+
 static int virtio_init(void)
 {
if (bus_register(virtio_bus) != 0)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index c600ccf..ef9a165 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -234,8 +234,6 @@ static irqreturn_t vm_interrupt(int irq, void *opaque)
 {
struct virtio_mmio_device *vm_dev = opaque;
struct virtio_mmio_vq_info *info;
-   struct virtio_driver *vdrv = container_of(vm_dev-vdev.dev.driver,
-   struct virtio_driver, driver);
unsigned long status

[PATCH v3 01/25] virtio_pci: fix virtio spec compliance on restore

2014-10-12 Thread Michael S. Tsirkin 
On restore, virtio pci does the following:
+ set features
+ init vqs etc - device can be used at this point!
+ set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits

This is in violation of the virtio spec, which
requires the following order:
- ACKNOWLEDGE
- DRIVER
- init vqs
- DRIVER_OK

This behaviour will break with hypervisors that assume spec compliant
behaviour.  It seems like a good idea to have this patch applied to
stable branches to reduce the support butden for the hypervisors.

Cc: sta...@vger.kernel.org
Cc: Amit Shah amit.s...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/virtio/virtio_pci.c | 33 ++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 3d1463c..add40d0 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -789,6 +789,7 @@ static int virtio_pci_restore(struct device *dev)
struct pci_dev *pci_dev = to_pci_dev(dev);
struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
struct virtio_driver *drv;
+   unsigned status = 0;
int ret;
 
drv = container_of(vp_dev-vdev.dev.driver,
@@ -799,14 +800,40 @@ static int virtio_pci_restore(struct device *dev)
return ret;
 
pci_set_master(pci_dev);
+   /* We always start by resetting the device, in case a previous
+* driver messed it up. */
+   vp_reset(vp_dev-vdev);
+
+   /* Acknowledge that we've seen the device. */
+   status |= VIRTIO_CONFIG_S_ACKNOWLEDGE;
+   vp_set_status(vp_dev-vdev, status);
+
+   /* Maybe driver failed before freeze.
+* Restore the failed status, for debugging. */
+   status |= vp_dev-saved_status  VIRTIO_CONFIG_S_FAILED;
+   vp_set_status(vp_dev-vdev, status);
+
+   if (!drv)
+   return 0;
+
+   /* We have a driver! */
+   status |= VIRTIO_CONFIG_S_DRIVER;
+   vp_set_status(vp_dev-vdev, status);
+
vp_finalize_features(vp_dev-vdev);
 
-   if (drv  drv-restore)
+   if (drv-restore) {
ret = drv-restore(vp_dev-vdev);
+   if (ret) {
+   status |= VIRTIO_CONFIG_S_FAILED;
+   vp_set_status(vp_dev-vdev, status);
+   return ret;
+   }
+   }
 
/* Finally, tell the device we're all set */
-   if (!ret)
-   vp_set_status(vp_dev-vdev, vp_dev-saved_status);
+   status |= VIRTIO_CONFIG_S_DRIVER_OK;
+   vp_set_status(vp_dev-vdev, status);
 
return ret;
 }
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 10/25] virtio: add API to enable VQs early

2014-10-13 Thread Michael S. Tsirkin 
On Mon, Oct 13, 2014 at 05:22:39PM +1030, Rusty Russell wrote:
 Michael S. Tsirkin m...@redhat.com writes:
  virtio spec 0.9.X requires DRIVER_OK to be set before
  VQs are used, but some drivers use VQs before probe
  function returns.
  Since DRIVER_OK is set after probe, this violates the spec.
 
  Even though under virtio 1.0 transitional devices support this
  behaviour, we want to make it possible for those early callers to become
  spec compliant and eventually support non-transitional devices.
 
  Add API for drivers to call before using VQs.
 
  Sets DRIVER_OK internally.
 
  Signed-off-by: Michael S. Tsirkin m...@redhat.com
  Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
 
 OK, this all looks good, but I think we should do two things:
 
 1) rename virtio_enable_vqs_early() to virtio_device_ready().

I considered this, I was concerned that the usage isn't clear from name:
- this has to be called before VQs are used
- this is optional, happens automatically after probe

device_ready seems to imply driver is fully initialized,
if it was it could just call probe.

But it's easy enough to make the change, so I'll go ahead and
post V4. It will be up to you which one to apply then.

 2) Add a BUG_ON in the virtio_ring code to make sure device is ready
before any ops are called.
 
 Cheers,
 Rusty.

On kick? This seems too expensive, especially considering that
we don't track the status.

Anyway, can be a patch on top?

-- 
MST
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 00/25] virtio: fix spec compliance issues

2014-10-13 Thread Michael S. Tsirkin 
Changes from v4:
rename virtio_enable_vqs_early() to virtio_device_ready()
Note: Rusty requested we add a BUG_ON in the virtio_ring code.
This can be done by a separate patch on top.
Good for bisectability in case BUG_ON starts triggering :)

Rusty, please review this, and consider for this merge window.

This fixes the following virtio spec compliance issues:
1. on restore, drivers use device before setting ACKNOWLEDGE and DRIVER bits
2. on probe, drivers aren't prepared to handle config interrupts
   arriving before probe returns
3. on probe, drivers use device before DRIVER_OK it set

Note that 1 is a clear violation of virtio spec 0.9 and 1.0,
so I am proposing the fix for stable. OTOH 2 is against 1.0 rules but
is a known documented bug in many drivers, so let's fix it going
forward, but it does not seem to be worth it to backport
the changes.

An error handling bugfix for virtio-net and a fix for a
theoretical race condition in virtio scsi is also included.

2 is merely a theoretical race condition, but it seems important
to address to make sure that changes to address 3 do not introduce
instability.

I also included patch to drop scan callback use from virtio
scsi: using this callback creates asymmetry between probe
and resume, and we actually had to add code comments so
people can figure out the flow. Code flow becomes clearer
with the new API.

After this change, the only user of the scan callback is virtio rng.
It's possible to modify it trivially and then drop scan callback
from core, but I'm rather inclined to look at ways to
make some rng core changes so that we don't need to
have so many variables tracking device state.
So this is deferred for now.

Michael S. Tsirkin (24):
  virtio_pci: fix virtio spec compliance on restore
  virtio: unify config_changed handling
  virtio-pci: move freeze/restore to virtio core
  virtio: defer config changed notifications
  virtio_blk: drop config_enable
  virtio-blk: drop config_mutex
  virtio_net: drop config_enable
  virtio-net: drop config_mutex
  virtio_net: minor cleanup
  virtio: add API to enable VQs early
  virtio_net: enable VQs early
  virtio_blk: enable VQs early
  virtio_console: enable VQs early
  9p/trans_virtio: enable VQs early
  virtio_net: fix use after free on allocation failure
  virtio_scsi: move kick event out from virtscsi_init
  virtio_blk: enable VQs early on restore
  virtio_scsi: enable VQs early on restore
  virtio_console: enable VQs early on restore
  virtio_net: enable VQs early on restore
  virtio_scsi: fix race on device removal
  virtio_balloon: enable VQs early on restore
  virtio_scsi: drop scan callback
  virtio-rng: refactor probe error handling

Paolo Bonzini (1):
  virito_scsi: use freezable WQ for events

 include/linux/virtio.h  |  14 +
 include/linux/virtio_config.h   |  17 ++
 drivers/block/virtio_blk.c  |  40 --
 drivers/char/hw_random/virtio-rng.c |  15 +++---
 drivers/char/virtio_console.c   |   4 ++
 drivers/misc/mic/card/mic_virtio.c  |   6 +--
 drivers/net/virtio_net.c|  44 +---
 drivers/s390/kvm/kvm_virtio.c   |   9 +---
 drivers/s390/kvm/virtio_ccw.c   |   6 +--
 drivers/scsi/virtio_scsi.c  |  42 +--
 drivers/virtio/virtio.c | 102 
 drivers/virtio/virtio_balloon.c |   2 +
 drivers/virtio/virtio_mmio.c|   7 +--
 drivers/virtio/virtio_pci.c |  33 ++--
 net/9p/trans_virtio.c   |   2 +
 15 files changed, 206 insertions(+), 137 deletions(-)

-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 02/25] virtio: unify config_changed handling

2014-10-13 Thread Michael S. Tsirkin 
Replace duplicated code in all transports with a single wrapper in
virtio.c.

The only functional change is in virtio_mmio.c: if a buggy device sends
us an interrupt before driver is set, we previously returned IRQ_NONE,
now we return IRQ_HANDLED.

As this must not happen in practice, this does not look like a big deal.

See also commit 3fff0179e33cd7d0a688dab65700c46ad089e934
virtio-pci: do not oops on config change if driver not loaded.
for the original motivation behind the driver check.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 include/linux/virtio.h | 2 ++
 drivers/misc/mic/card/mic_virtio.c | 6 +-
 drivers/s390/kvm/kvm_virtio.c  | 9 +
 drivers/s390/kvm/virtio_ccw.c  | 6 +-
 drivers/virtio/virtio.c| 9 +
 drivers/virtio/virtio_mmio.c   | 7 ++-
 drivers/virtio/virtio_pci.c| 6 +-
 7 files changed, 17 insertions(+), 28 deletions(-)

diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index b46671e..3c19bd3 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -108,6 +108,8 @@ void unregister_virtio_device(struct virtio_device *dev);
 
 void virtio_break_device(struct virtio_device *dev);
 
+void virtio_config_changed(struct virtio_device *dev);
+
 /**
  * virtio_driver - operations for a virtio I/O driver
  * @driver: underlying device driver (populate name and owner).
diff --git a/drivers/misc/mic/card/mic_virtio.c 
b/drivers/misc/mic/card/mic_virtio.c
index f14b600..e647947 100644
--- a/drivers/misc/mic/card/mic_virtio.c
+++ b/drivers/misc/mic/card/mic_virtio.c
@@ -462,16 +462,12 @@ static void mic_handle_config_change(struct 
mic_device_desc __iomem *d,
struct mic_device_ctrl __iomem *dc
= (void __iomem *)d + mic_aligned_desc_size(d);
struct mic_vdev *mvdev = (struct mic_vdev *)ioread64(dc-vdev);
-   struct virtio_driver *drv;
 
if (ioread8(dc-config_change) != MIC_VIRTIO_PARAM_CONFIG_CHANGED)
return;
 
dev_dbg(mdrv-dev, %s %d\n, __func__, __LINE__);
-   drv = container_of(mvdev-vdev.dev.driver,
-   struct virtio_driver, driver);
-   if (drv-config_changed)
-   drv-config_changed(mvdev-vdev);
+   virtio_config_changed(mvdev-vdev);
iowrite8(1, dc-guest_ack);
 }
 
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index a134965..6431290 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -406,15 +406,8 @@ static void kvm_extint_handler(struct ext_code ext_code,
 
switch (param) {
case VIRTIO_PARAM_CONFIG_CHANGED:
-   {
-   struct virtio_driver *drv;
-   drv = container_of(vq-vdev-dev.driver,
-  struct virtio_driver, driver);
-   if (drv-config_changed)
-   drv-config_changed(vq-vdev);
-
+   virtio_config_changed(vq-vdev);
break;
-   }
case VIRTIO_PARAM_DEV_ADD:
schedule_work(hotplug_work);
break;
diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index d2c0b44..6cbe6ef 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -940,11 +940,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
vring_interrupt(0, vq);
}
if (test_bit(0, vcdev-indicators2)) {
-   drv = container_of(vcdev-vdev.dev.driver,
-  struct virtio_driver, driver);
-
-   if (drv  drv-config_changed)
-   drv-config_changed(vcdev-vdev);
+   virtio_config_changed(vcdev-vdev);
clear_bit(0, vcdev-indicators2);
}
 }
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index fed0ce1..3980687 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -239,6 +239,15 @@ void unregister_virtio_device(struct virtio_device *dev)
 }
 EXPORT_SYMBOL_GPL(unregister_virtio_device);
 
+void virtio_config_changed(struct virtio_device *dev)
+{
+   struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
+
+   if (drv  drv-config_changed)
+   drv-config_changed(dev);
+}
+EXPORT_SYMBOL_GPL(virtio_config_changed);
+
 static int virtio_init(void)
 {
if (bus_register(virtio_bus) != 0)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index c600ccf..ef9a165 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -234,8 +234,6 @@ static irqreturn_t vm_interrupt(int irq, void *opaque)
 {
struct virtio_mmio_device *vm_dev = opaque;
struct virtio_mmio_vq_info *info;
-   struct virtio_driver *vdrv = container_of(vm_dev-vdev.dev.driver,
-   struct virtio_driver, driver);
unsigned long status

[PATCH v4 01/25] virtio_pci: fix virtio spec compliance on restore

2014-10-13 Thread Michael S. Tsirkin 
On restore, virtio pci does the following:
+ set features
+ init vqs etc - device can be used at this point!
+ set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits

This is in violation of the virtio spec, which
requires the following order:
- ACKNOWLEDGE
- DRIVER
- init vqs
- DRIVER_OK

This behaviour will break with hypervisors that assume spec compliant
behaviour.  It seems like a good idea to have this patch applied to
stable branches to reduce the support butden for the hypervisors.

Cc: sta...@vger.kernel.org
Cc: Amit Shah amit.s...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/virtio/virtio_pci.c | 33 ++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 3d1463c..add40d0 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -789,6 +789,7 @@ static int virtio_pci_restore(struct device *dev)
struct pci_dev *pci_dev = to_pci_dev(dev);
struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
struct virtio_driver *drv;
+   unsigned status = 0;
int ret;
 
drv = container_of(vp_dev-vdev.dev.driver,
@@ -799,14 +800,40 @@ static int virtio_pci_restore(struct device *dev)
return ret;
 
pci_set_master(pci_dev);
+   /* We always start by resetting the device, in case a previous
+* driver messed it up. */
+   vp_reset(vp_dev-vdev);
+
+   /* Acknowledge that we've seen the device. */
+   status |= VIRTIO_CONFIG_S_ACKNOWLEDGE;
+   vp_set_status(vp_dev-vdev, status);
+
+   /* Maybe driver failed before freeze.
+* Restore the failed status, for debugging. */
+   status |= vp_dev-saved_status  VIRTIO_CONFIG_S_FAILED;
+   vp_set_status(vp_dev-vdev, status);
+
+   if (!drv)
+   return 0;
+
+   /* We have a driver! */
+   status |= VIRTIO_CONFIG_S_DRIVER;
+   vp_set_status(vp_dev-vdev, status);
+
vp_finalize_features(vp_dev-vdev);
 
-   if (drv  drv-restore)
+   if (drv-restore) {
ret = drv-restore(vp_dev-vdev);
+   if (ret) {
+   status |= VIRTIO_CONFIG_S_FAILED;
+   vp_set_status(vp_dev-vdev, status);
+   return ret;
+   }
+   }
 
/* Finally, tell the device we're all set */
-   if (!ret)
-   vp_set_status(vp_dev-vdev, vp_dev-saved_status);
+   status |= VIRTIO_CONFIG_S_DRIVER_OK;
+   vp_set_status(vp_dev-vdev, status);
 
return ret;
 }
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 03/25] virtio-pci: move freeze/restore to virtio core

2014-10-13 Thread Michael S. Tsirkin 
This is in preparation to extending config changed event handling
in core.
Wrapping these in an API also seems to make for a cleaner code.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 include/linux/virtio.h  |  6 +
 drivers/virtio/virtio.c | 54 +
 drivers/virtio/virtio_pci.c | 54 ++---
 3 files changed, 62 insertions(+), 52 deletions(-)

diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 3c19bd3..8df7ba8 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -78,6 +78,7 @@ bool virtqueue_is_broken(struct virtqueue *vq);
 /**
  * virtio_device - representation of a device using virtio
  * @index: unique position on the virtio bus
+ * @failed: saved value for CONFIG_S_FAILED bit (for restore)
  * @dev: underlying device.
  * @id: the device type identification (used to match it with a driver).
  * @config: the configuration ops for this device.
@@ -88,6 +89,7 @@ bool virtqueue_is_broken(struct virtqueue *vq);
  */
 struct virtio_device {
int index;
+   bool failed;
struct device dev;
struct virtio_device_id id;
const struct virtio_config_ops *config;
@@ -109,6 +111,10 @@ void unregister_virtio_device(struct virtio_device *dev);
 void virtio_break_device(struct virtio_device *dev);
 
 void virtio_config_changed(struct virtio_device *dev);
+#ifdef CONFIG_PM_SLEEP
+int virtio_device_freeze(struct virtio_device *dev);
+int virtio_device_restore(struct virtio_device *dev);
+#endif
 
 /**
  * virtio_driver - operations for a virtio I/O driver
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 3980687..8216b73 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -248,6 +248,60 @@ void virtio_config_changed(struct virtio_device *dev)
 }
 EXPORT_SYMBOL_GPL(virtio_config_changed);
 
+#ifdef CONFIG_PM_SLEEP
+int virtio_device_freeze(struct virtio_device *dev)
+{
+   struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
+
+   dev-failed = dev-config-get_status(dev)  VIRTIO_CONFIG_S_FAILED;
+
+   if (drv  drv-freeze)
+   return drv-freeze(dev);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(virtio_device_freeze);
+
+int virtio_device_restore(struct virtio_device *dev)
+{
+   struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
+
+   /* We always start by resetting the device, in case a previous
+* driver messed it up. */
+   dev-config-reset(dev);
+
+   /* Acknowledge that we've seen the device. */
+   add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
+
+   /* Maybe driver failed before freeze.
+* Restore the failed status, for debugging. */
+   if (dev-failed)
+   add_status(dev, VIRTIO_CONFIG_S_FAILED);
+
+   if (!drv)
+   return 0;
+
+   /* We have a driver! */
+   add_status(dev, VIRTIO_CONFIG_S_DRIVER);
+
+   dev-config-finalize_features(dev);
+
+   if (drv-restore) {
+   int ret = drv-restore(dev);
+   if (ret) {
+   add_status(dev, VIRTIO_CONFIG_S_FAILED);
+   return ret;
+   }
+   }
+
+   /* Finally, tell the device we're all set */
+   add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(virtio_device_restore);
+#endif
+
 static int virtio_init(void)
 {
if (bus_register(virtio_bus) != 0)
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index f39f4e7..d34ebfa 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -57,9 +57,6 @@ struct virtio_pci_device
/* Vectors allocated, excluding per-vq vectors if any */
unsigned msix_used_vectors;
 
-   /* Status saved during hibernate/restore */
-   u8 saved_status;
-
/* Whether we have vector per vq */
bool per_vq_vectors;
 };
@@ -764,16 +761,9 @@ static int virtio_pci_freeze(struct device *dev)
 {
struct pci_dev *pci_dev = to_pci_dev(dev);
struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
-   struct virtio_driver *drv;
int ret;
 
-   drv = container_of(vp_dev-vdev.dev.driver,
-  struct virtio_driver, driver);
-
-   ret = 0;
-   vp_dev-saved_status = vp_get_status(vp_dev-vdev);
-   if (drv  drv-freeze)
-   ret = drv-freeze(vp_dev-vdev);
+   ret = virtio_device_freeze(vp_dev-vdev);
 
if (!ret)
pci_disable_device(pci_dev);
@@ -784,54 +774,14 @@ static int virtio_pci_restore(struct device *dev)
 {
struct pci_dev *pci_dev = to_pci_dev(dev);
struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
-   struct virtio_driver *drv;
-   unsigned status = 0;
int ret;
 
-   drv = container_of(vp_dev-vdev.dev.driver,
-  struct

[PATCH v4 05/25] virtio_blk: drop config_enable

2014-10-13 Thread Michael S. Tsirkin 
Now that virtio core ensures config changes don't
arrive during probing, drop config_enable flag
in virtio blk.
On removal, flush is now sufficient to guarantee that
no change work is queued.

This help simplify the driver, and will allow
setting DRIVER_OK earlier without losing config
change notifications.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/block/virtio_blk.c | 23 ---
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 0a58140..91272f1a 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -44,9 +44,6 @@ struct virtio_blk
/* Lock for config space updates */
struct mutex config_lock;
 
-   /* enable config space updates */
-   bool config_enable;
-
/* What host tells us, plus 2 for header  tailer. */
unsigned int sg_elems;
 
@@ -348,8 +345,6 @@ static void virtblk_config_changed_work(struct work_struct 
*work)
u64 capacity, size;
 
mutex_lock(vblk-config_lock);
-   if (!vblk-config_enable)
-   goto done;
 
/* Host must always specify the capacity. */
virtio_cread(vdev, struct virtio_blk_config, capacity, capacity);
@@ -374,7 +369,7 @@ static void virtblk_config_changed_work(struct work_struct 
*work)
set_capacity(vblk-disk, capacity);
revalidate_disk(vblk-disk);
kobject_uevent_env(disk_to_dev(vblk-disk)-kobj, KOBJ_CHANGE, envp);
-done:
+
mutex_unlock(vblk-config_lock);
 }
 
@@ -609,7 +604,6 @@ static int virtblk_probe(struct virtio_device *vdev)
mutex_init(vblk-config_lock);
 
INIT_WORK(vblk-config_work, virtblk_config_changed_work);
-   vblk-config_enable = true;
 
err = init_vq(vblk);
if (err)
@@ -771,10 +765,8 @@ static void virtblk_remove(struct virtio_device *vdev)
int index = vblk-index;
int refc;
 
-   /* Prevent config work handler from accessing the device. */
-   mutex_lock(vblk-config_lock);
-   vblk-config_enable = false;
-   mutex_unlock(vblk-config_lock);
+   /* Make sure no work handler is accessing the device. */
+   flush_work(vblk-config_work);
 
del_gendisk(vblk-disk);
blk_cleanup_queue(vblk-disk-queue);
@@ -784,8 +776,6 @@ static void virtblk_remove(struct virtio_device *vdev)
/* Stop all the virtqueues. */
vdev-config-reset(vdev);
 
-   flush_work(vblk-config_work);
-
refc = atomic_read(disk_to_dev(vblk-disk)-kobj.kref.refcount);
put_disk(vblk-disk);
vdev-config-del_vqs(vdev);
@@ -805,11 +795,7 @@ static int virtblk_freeze(struct virtio_device *vdev)
/* Ensure we don't receive any more interrupts */
vdev-config-reset(vdev);
 
-   /* Prevent config work handler from accessing the device. */
-   mutex_lock(vblk-config_lock);
-   vblk-config_enable = false;
-   mutex_unlock(vblk-config_lock);
-
+   /* Make sure no work handler is accessing the device. */
flush_work(vblk-config_work);
 
blk_mq_stop_hw_queues(vblk-disk-queue);
@@ -823,7 +809,6 @@ static int virtblk_restore(struct virtio_device *vdev)
struct virtio_blk *vblk = vdev-priv;
int ret;
 
-   vblk-config_enable = true;
ret = init_vq(vdev-priv);
if (!ret)
blk_mq_start_stopped_hw_queues(vblk-disk-queue, true);
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 24/25] virtio_scsi: drop scan callback

2014-10-13 Thread Michael S. Tsirkin 
Enable VQs early like we do for restore.
This makes it possible to drop the scan callback,
moving scanning into the probe function, and making
code simpler.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/scsi/virtio_scsi.c | 23 +++
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 327eba0..5f022ff 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -860,17 +860,6 @@ static void virtscsi_init_vq(struct virtio_scsi_vq 
*virtscsi_vq,
virtscsi_vq-vq = vq;
 }
 
-static void virtscsi_scan(struct virtio_device *vdev)
-{
-   struct Scsi_Host *shost = virtio_scsi_host(vdev);
-   struct virtio_scsi *vscsi = shost_priv(shost);
-
-   if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
-   virtscsi_kick_event_all(vscsi);
-
-   scsi_scan_host(shost);
-}
-
 static void virtscsi_remove_vqs(struct virtio_device *vdev)
 {
struct Scsi_Host *sh = virtio_scsi_host(vdev);
@@ -1007,10 +996,13 @@ static int virtscsi_probe(struct virtio_device *vdev)
err = scsi_add_host(shost, vdev-dev);
if (err)
goto scsi_add_host_failed;
-   /*
-* scsi_scan_host() happens in virtscsi_scan() via virtio_driver-scan()
-* after VIRTIO_CONFIG_S_DRIVER_OK has been set..
-*/
+
+   virtio_device_ready(vdev);
+
+   if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
+   virtscsi_kick_event_all(vscsi);
+
+   scsi_scan_host(shost);
return 0;
 
 scsi_add_host_failed:
@@ -1090,7 +1082,6 @@ static struct virtio_driver virtio_scsi_driver = {
.driver.owner = THIS_MODULE,
.id_table = id_table,
.probe = virtscsi_probe,
-   .scan = virtscsi_scan,
 #ifdef CONFIG_PM_SLEEP
.freeze = virtscsi_freeze,
.restore = virtscsi_restore,
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 25/25] virtio-rng: refactor probe error handling

2014-10-13 Thread Michael S. Tsirkin 
Code like
vi-vq = NULL;
kfree(vi)
does not make sense.

Clean it up, use goto error labels for cleanup.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/char/hw_random/virtio-rng.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/char/hw_random/virtio-rng.c 
b/drivers/char/hw_random/virtio-rng.c
index 132c9cc..72295ea 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -109,8 +109,8 @@ static int probe_common(struct virtio_device *vdev)
 
vi-index = index = ida_simple_get(rng_index_ida, 0, 0, GFP_KERNEL);
if (index  0) {
-   kfree(vi);
-   return index;
+   err = index;
+   goto err_ida;
}
sprintf(vi-name, virtio_rng.%d, index);
init_completion(vi-have_data);
@@ -128,13 +128,16 @@ static int probe_common(struct virtio_device *vdev)
vi-vq = virtio_find_single_vq(vdev, random_recv_done, input);
if (IS_ERR(vi-vq)) {
err = PTR_ERR(vi-vq);
-   vi-vq = NULL;
-   kfree(vi);
-   ida_simple_remove(rng_index_ida, index);
-   return err;
+   goto err_find;
}
 
return 0;
+
+err_find:
+   ida_simple_remove(rng_index_ida, index);
+err_ida:
+   kfree(vi);
+   return err;
 }
 
 static void remove_common(struct virtio_device *vdev)
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 22/25] virtio_scsi: fix race on device removal

2014-10-13 Thread Michael S. Tsirkin 
We cancel event work on device removal, but an interrupt
could trigger immediately after this, and queue it
again.

To fix, set a flag.

Loosely based on patch by Paolo Bonzini

Signed-off-by: Paolo Bonzini pbonz...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/scsi/virtio_scsi.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 501838d..327eba0 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -110,6 +110,9 @@ struct virtio_scsi {
/* CPU hotplug notifier */
struct notifier_block nb;
 
+   /* Protected by event_vq lock */
+   bool stop_events;
+
struct virtio_scsi_vq ctrl_vq;
struct virtio_scsi_vq event_vq;
struct virtio_scsi_vq req_vqs[];
@@ -303,6 +306,11 @@ static void virtscsi_cancel_event_work(struct virtio_scsi 
*vscsi)
 {
int i;
 
+   /* Stop scheduling work before calling cancel_work_sync.  */
+   spin_lock_irq(vscsi-event_vq.vq_lock);
+   vscsi-stop_events = true;
+   spin_unlock_irq(vscsi-event_vq.vq_lock);
+
for (i = 0; i  VIRTIO_SCSI_EVENT_LEN; i++)
cancel_work_sync(vscsi-event_list[i].work);
 }
@@ -390,7 +398,8 @@ static void virtscsi_complete_event(struct virtio_scsi 
*vscsi, void *buf)
 {
struct virtio_scsi_event_node *event_node = buf;
 
-   queue_work(system_freezable_wq, event_node-work);
+   if (!vscsi-stop_events)
+   queue_work(system_freezable_wq, event_node-work);
 }
 
 static void virtscsi_event_done(struct virtqueue *vq)
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 21/25] virito_scsi: use freezable WQ for events

2014-10-13 Thread Michael S. Tsirkin 
From: Paolo Bonzini pbonz...@redhat.com

Michael S. Tsirkin noticed a race condition:
we reset device on freeze, but system WQ is still
running so it might try adding bufs to a VQ meanwhile.

To fix, switch to handling events from the freezable WQ.

Reported-by: Michael S. Tsirkin m...@redhat.com
Signed-off-by: Paolo Bonzini pbonz...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/scsi/virtio_scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 2b565b3..501838d 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -390,7 +390,7 @@ static void virtscsi_complete_event(struct virtio_scsi 
*vscsi, void *buf)
 {
struct virtio_scsi_event_node *event_node = buf;
 
-   schedule_work(event_node-work);
+   queue_work(system_freezable_wq, event_node-work);
 }
 
 static void virtscsi_event_done(struct virtqueue *vq)
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 23/25] virtio_balloon: enable VQs early on restore

2014-10-13 Thread Michael S. Tsirkin 
virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after resume returns, virtio balloon
violated this rule by adding bufs, which causes the VQ to be used
directly within restore.

To fix, call virtio_device_ready before using VQ.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/virtio/virtio_balloon.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 25ebe8e..9629fad 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -538,6 +538,8 @@ static int virtballoon_restore(struct virtio_device *vdev)
if (ret)
return ret;
 
+   virtio_device_ready(vdev);
+
fill_balloon(vb, towards_target(vb));
update_balloon_size(vb);
return 0;
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 14/25] 9p/trans_virtio: enable VQs early

2014-10-13 Thread Michael S. Tsirkin 
virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after probe returns, but virtio 9p device
adds self to channel list within probe, at which point VQ can be
used in violation of the spec.

To fix, call virtio_device_ready before using VQs.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 net/9p/trans_virtio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 6940d8f..766ba48 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -575,6 +575,8 @@ static int p9_virtio_probe(struct virtio_device *vdev)
/* Ceiling limit to avoid denial of service attacks */
chan-p9_max_pages = nr_free_buffer_pages()/4;
 
+   virtio_device_ready(vdev);
+
mutex_lock(virtio_9p_lock);
list_add_tail(chan-chan_list, virtio_chan_list);
mutex_unlock(virtio_9p_lock);
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 20/25] virtio_net: enable VQs early on restore

2014-10-13 Thread Michael S. Tsirkin 
virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after restore returns, virtio net violated this
rule by using receive VQs within restore.

To fix, call virtio_device_ready before using VQs.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/net/virtio_net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3551417..6b6e136 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1912,6 +1912,8 @@ static int virtnet_restore(struct virtio_device *vdev)
if (err)
return err;
 
+   virtio_device_ready(vdev);
+
if (netif_running(vi-dev)) {
for (i = 0; i  vi-curr_queue_pairs; i++)
if (!try_fill_recv(vi-rq[i], GFP_KERNEL))
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 18/25] virtio_scsi: enable VQs early on restore

2014-10-13 Thread Michael S. Tsirkin 
virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after restore returns, virtio scsi violated
this rule on restore by kicking event vq within restore.

To fix, call virtio_device_ready before using event queue.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/scsi/virtio_scsi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 0642ce3..2b565b3 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -1054,6 +1054,8 @@ static int virtscsi_restore(struct virtio_device *vdev)
return err;
}
 
+   virtio_device_ready(vdev);
+
if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
virtscsi_kick_event_all(vscsi);
 
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 16/25] virtio_scsi: move kick event out from virtscsi_init

2014-10-13 Thread Michael S. Tsirkin 
We currently kick event within virtscsi_init,
before host is fully initialized.

This can in theory confuse guest if device
consumes the buffers immediately.

To fix,  move virtscsi_kick_event_all out to scan/restore.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/scsi/virtio_scsi.c | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index eee1bc0..0642ce3 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -853,7 +853,11 @@ static void virtscsi_init_vq(struct virtio_scsi_vq 
*virtscsi_vq,
 
 static void virtscsi_scan(struct virtio_device *vdev)
 {
-   struct Scsi_Host *shost = (struct Scsi_Host *)vdev-priv;
+   struct Scsi_Host *shost = virtio_scsi_host(vdev);
+   struct virtio_scsi *vscsi = shost_priv(shost);
+
+   if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
+   virtscsi_kick_event_all(vscsi);
 
scsi_scan_host(shost);
 }
@@ -916,9 +920,6 @@ static int virtscsi_init(struct virtio_device *vdev,
virtscsi_config_set(vdev, cdb_size, VIRTIO_SCSI_CDB_SIZE);
virtscsi_config_set(vdev, sense_size, VIRTIO_SCSI_SENSE_SIZE);
 
-   if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
-   virtscsi_kick_event_all(vscsi);
-
err = 0;
 
 out:
@@ -1048,8 +1049,13 @@ static int virtscsi_restore(struct virtio_device *vdev)
return err;
 
err = register_hotcpu_notifier(vscsi-nb);
-   if (err)
+   if (err) {
vdev-config-del_vqs(vdev);
+   return err;
+   }
+
+   if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
+   virtscsi_kick_event_all(vscsi);
 
return err;
 }
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 17/25] virtio_blk: enable VQs early on restore

2014-10-13 Thread Michael S. Tsirkin 
virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after restore returns, virtio block violated
this rule on restore by restarting queues, which might in theory
cause the VQ to be used directly within restore.

To fix, call virtio_device_ready before using starting queues.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/block/virtio_blk.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 46b04bf..1c95af5 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -804,10 +804,13 @@ static int virtblk_restore(struct virtio_device *vdev)
int ret;
 
ret = init_vq(vdev-priv);
-   if (!ret)
-   blk_mq_start_stopped_hw_queues(vblk-disk-queue, true);
+   if (ret)
+   return ret;
+
+   virtio_device_ready(vdev);
 
-   return ret;
+   blk_mq_start_stopped_hw_queues(vblk-disk-queue, true);
+   return 0;
 }
 #endif
 
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 19/25] virtio_console: enable VQs early on restore

2014-10-13 Thread Michael S. Tsirkin 
virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after resume returns, virtio console violated this
rule by adding inbufs, which causes the VQ to be used directly within
restore.

To fix, call virtio_device_ready before using VQs.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/char/virtio_console.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 6ebe8f6..2ae843f 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -2184,6 +2184,8 @@ static int virtcons_restore(struct virtio_device *vdev)
if (ret)
return ret;
 
+   virtio_device_ready(portdev-vdev);
+
if (use_multiport(portdev))
fill_queue(portdev-c_ivq, portdev-c_ivq_lock);
 
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 13/25] virtio_console: enable VQs early

2014-10-13 Thread Michael S. Tsirkin 
virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after probe returns, virtio console violated this
rule by adding inbufs, which causes the VQ to be used directly within
probe.

To fix, call virtio_device_ready before using VQs.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/char/virtio_console.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index b585b47..6ebe8f6 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1449,6 +1449,8 @@ static int add_port(struct ports_device *portdev, u32 id)
spin_lock_init(port-outvq_lock);
init_waitqueue_head(port-waitqueue);
 
+   virtio_device_ready(portdev-vdev);
+
/* Fill the in_vq with buffers so the host can send us data. */
nr_added_bufs = fill_queue(port-in_vq, port-inbuf_lock);
if (!nr_added_bufs) {
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 12/25] virtio_blk: enable VQs early

2014-10-13 Thread Michael S. Tsirkin 
virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after probe returns, virtio block violated this
rule by calling add_disk, which causes the VQ to be used directly within
probe.

To fix, call virtio_device_ready before using VQs.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/block/virtio_blk.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 89ba8d6..46b04bf 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -719,6 +719,8 @@ static int virtblk_probe(struct virtio_device *vdev)
if (!err  opt_io_size)
blk_queue_io_opt(q, blk_size * opt_io_size);
 
+   virtio_device_ready(vdev);
+
add_disk(vblk-disk);
err = device_create_file(disk_to_dev(vblk-disk), dev_attr_serial);
if (err)
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 09/25] virtio_net: minor cleanup

2014-10-13 Thread Michael S. Tsirkin 
goto done;
done:
return;
is ugly, it was put there to make diff review easier.
replace by open-coded return.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Acked-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/net/virtio_net.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 23e4a69..ef04d23 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1403,7 +1403,7 @@ static void virtnet_config_changed_work(struct 
work_struct *work)
 
if (virtio_cread_feature(vi-vdev, VIRTIO_NET_F_STATUS,
 struct virtio_net_config, status, v)  0)
-   goto done;
+   return;
 
if (v  VIRTIO_NET_S_ANNOUNCE) {
netdev_notify_peers(vi-dev);
@@ -1414,7 +1414,7 @@ static void virtnet_config_changed_work(struct 
work_struct *work)
v = VIRTIO_NET_S_LINK_UP;
 
if (vi-status == v)
-   goto done;
+   return;
 
vi-status = v;
 
@@ -1425,8 +1425,6 @@ static void virtnet_config_changed_work(struct 
work_struct *work)
netif_carrier_off(vi-dev);
netif_tx_stop_all_queues(vi-dev);
}
-done:
-   return;
 }
 
 static void virtnet_config_changed(struct virtio_device *vdev)
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 08/25] virtio-net: drop config_mutex

2014-10-13 Thread Michael S. Tsirkin 
config_mutex served two purposes: prevent multiple concurrent config
change handlers, and synchronize access to config_enable flag.

Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1
workqueue: make all workqueues non-reentrant
all workqueues are non-reentrant, and config_enable
is now gone.

Get rid of the unnecessary lock.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/net/virtio_net.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 743fb04..23e4a69 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -132,9 +132,6 @@ struct virtnet_info {
/* Work struct for config space updates */
struct work_struct config_work;
 
-   /* Lock for config space updates */
-   struct mutex config_lock;
-
/* Does the affinity hint is set for virtqueues? */
bool affinity_hint_set;
 
@@ -1404,7 +1401,6 @@ static void virtnet_config_changed_work(struct 
work_struct *work)
container_of(work, struct virtnet_info, config_work);
u16 v;
 
-   mutex_lock(vi-config_lock);
if (virtio_cread_feature(vi-vdev, VIRTIO_NET_F_STATUS,
 struct virtio_net_config, status, v)  0)
goto done;
@@ -1430,7 +1426,7 @@ static void virtnet_config_changed_work(struct 
work_struct *work)
netif_tx_stop_all_queues(vi-dev);
}
 done:
-   mutex_unlock(vi-config_lock);
+   return;
 }
 
 static void virtnet_config_changed(struct virtio_device *vdev)
@@ -1751,7 +1747,6 @@ static int virtnet_probe(struct virtio_device *vdev)
u64_stats_init(virtnet_stats-rx_syncp);
}
 
-   mutex_init(vi-config_lock);
INIT_WORK(vi-config_work, virtnet_config_changed_work);
 
/* If we can receive ANY GSO packets, we must allocate large ones. */
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 11/25] virtio_net: enable VQs early

2014-10-13 Thread Michael S. Tsirkin 
virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after probe returns, virtio net violated this
rule by using receive VQs within probe.

To fix, call virtio_device_ready before using VQs.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/net/virtio_net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ef04d23..430f3ae 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1792,6 +1792,8 @@ static int virtnet_probe(struct virtio_device *vdev)
goto free_vqs;
}
 
+   virtio_device_ready(vdev);
+
/* Last of all, set up some receive buffers. */
for (i = 0; i  vi-curr_queue_pairs; i++) {
try_fill_recv(vi-rq[i], GFP_KERNEL);
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 06/25] virtio-blk: drop config_mutex

2014-10-13 Thread Michael S. Tsirkin 
config_mutex served two purposes: prevent multiple concurrent config
change handlers, and synchronize access to config_enable flag.

Since commit dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1
workqueue: make all workqueues non-reentrant
all workqueues are non-reentrant, and config_enable
is now gone.

Get rid of the unnecessary lock.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
---
 drivers/block/virtio_blk.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 91272f1a..89ba8d6 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -41,9 +41,6 @@ struct virtio_blk
/* Process context for config space updates */
struct work_struct config_work;
 
-   /* Lock for config space updates */
-   struct mutex config_lock;
-
/* What host tells us, plus 2 for header  tailer. */
unsigned int sg_elems;
 
@@ -344,8 +341,6 @@ static void virtblk_config_changed_work(struct work_struct 
*work)
char *envp[] = { RESIZE=1, NULL };
u64 capacity, size;
 
-   mutex_lock(vblk-config_lock);
-
/* Host must always specify the capacity. */
virtio_cread(vdev, struct virtio_blk_config, capacity, capacity);
 
@@ -369,8 +364,6 @@ static void virtblk_config_changed_work(struct work_struct 
*work)
set_capacity(vblk-disk, capacity);
revalidate_disk(vblk-disk);
kobject_uevent_env(disk_to_dev(vblk-disk)-kobj, KOBJ_CHANGE, envp);
-
-   mutex_unlock(vblk-config_lock);
 }
 
 static void virtblk_config_changed(struct virtio_device *vdev)
@@ -601,7 +594,6 @@ static int virtblk_probe(struct virtio_device *vdev)
 
vblk-vdev = vdev;
vblk-sg_elems = sg_elems;
-   mutex_init(vblk-config_lock);
 
INIT_WORK(vblk-config_work, virtblk_config_changed_work);
 
-- 
MST


--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 04/25] virtio: defer config changed notifications

2014-10-14 Thread Michael S. Tsirkin 
On Tue, Oct 14, 2014 at 11:01:12AM +1030, Rusty Russell wrote:
 Michael S. Tsirkin m...@redhat.com writes:
  Defer config changed notifications that arrive during
  probe/scan/freeze/restore.
 
  This will allow drivers to set DRIVER_OK earlier, without worrying about
  racing with config change interrupts.
 
  This change will also benefit old hypervisors (before 2009)
  that send interrupts without checking DRIVER_OK: previously,
  the callback could race with driver-specific initialization.
 
  This will also help simplify drivers.
 
 But AFAICT you never *read* dev-config_changed.
 
 You unconditionally trigger a config_changed event in
 virtio_config_enable().  That's a bit weird, but probably OK.
 
 How about the following change (on top of your patch).  I
 think the renaming is clearer, and note the added if() test in
 virtio_config_enable().
 
 If you approve, I'll fold it in.
 
 Cheers,
 Rusty.

Hi Rusty,
I'm okay with both changes.
You can fold it in if you prefer, or just make it a patch on top.

 diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
 index 2536701b098b..df598dd8c5c8 100644
 --- a/drivers/virtio/virtio.c
 +++ b/drivers/virtio/virtio.c
 @@ -122,7 +122,7 @@ static void __virtio_config_changed(struct virtio_device 
 *dev)
   struct virtio_driver *drv = drv_to_virtio(dev-dev.driver);
  
   if (!dev-config_enabled)
 - dev-config_changed = true;
 + dev-config_change_pending = true;
   else if (drv  drv-config_changed)
   drv-config_changed(dev);
  }
 @@ -148,8 +148,9 @@ static void virtio_config_enable(struct virtio_device 
 *dev)
  {
   spin_lock_irq(dev-config_lock);
   dev-config_enabled = true;
 - __virtio_config_changed(dev);
 - dev-config_changed = false;
 + if (dev-config_change_pending)
 + __virtio_config_changed(dev);
 + dev-config_change_pending = false;
   spin_unlock_irq(dev-config_lock);
  }
  
 @@ -253,7 +254,7 @@ int register_virtio_device(struct virtio_device *dev)
  
   spin_lock_init(dev-config_lock);
   dev-config_enabled = false;
 - dev-config_changed = false;
 + dev-config_change_pending = false;
  
   /* We always start by resetting the device, in case a previous
* driver messed it up.  This also tests that code path a little. */
 diff --git a/include/linux/virtio.h b/include/linux/virtio.h
 index 5636b119dc25..65261a7244fc 100644
 --- a/include/linux/virtio.h
 +++ b/include/linux/virtio.h
 @@ -80,7 +80,7 @@ bool virtqueue_is_broken(struct virtqueue *vq);
   * @index: unique position on the virtio bus
   * @failed: saved value for CONFIG_S_FAILED bit (for restore)
   * @config_enabled: configuration change reporting enabled
 - * @config_changed: configuration change reported while disabled
 + * @config_change_pending: configuration change reported while disabled
   * @config_lock: protects configuration change reporting
   * @dev: underlying device.
   * @id: the device type identification (used to match it with a driver).
 @@ -94,7 +94,7 @@ struct virtio_device {
   int index;
   bool failed;
   bool config_enabled;
 - bool config_changed;
 + bool config_change_pending;
   spinlock_t config_lock;
   struct device dev;
   struct virtio_device_id id;
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 13/25] virtio_console: enable VQs early

2014-10-20 Thread Michael S. Tsirkin 
On Mon, Oct 20, 2014 at 01:07:50PM +0100, Thomas Graf wrote:
 On 10/13/14 at 10:50am, Michael S. Tsirkin wrote:
  virtio spec requires drivers to set DRIVER_OK before using VQs.
  This is set automatically after probe returns, virtio console violated this
  rule by adding inbufs, which causes the VQ to be used directly within
  probe.
  
  To fix, call virtio_device_ready before using VQs.
  
  Signed-off-by: Michael S. Tsirkin m...@redhat.com
  ---
   drivers/char/virtio_console.c | 2 ++
   1 file changed, 2 insertions(+)
  
  diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
  index b585b47..6ebe8f6 100644
  --- a/drivers/char/virtio_console.c
  +++ b/drivers/char/virtio_console.c
  @@ -1449,6 +1449,8 @@ static int add_port(struct ports_device *portdev, u32 
  id)
  spin_lock_init(port-outvq_lock);
  init_waitqueue_head(port-waitqueue);
   
  +   virtio_device_ready(portdev-vdev);
  +
  /* Fill the in_vq with buffers so the host can send us data. */
  nr_added_bufs = fill_queue(port-in_vq, port-inbuf_lock);
  if (!nr_added_bufs) {

I see Cornelia sent a patch already.
I'd like to reproduce this though - could you send me
the command line please?


 Seems like probe and add_port() now both set VIRTIO_CONFIG_S_DRIVER_OK
 
 1.839658] kernel BUG at include/linux/virtio_config.h:125!
 [1.839995] invalid opcode:  [#1] SMP 
 [1.840169] Modules linked in: serio_raw virtio_balloon pcspkr virtio_net 
 virtio_console soundcore parport_pc floppy pvpanic parport i2c_piix4 nfsd 
 auth_rpcgss nfs_acl lockd grace sunrpc qxl drm_kms_helper ttm drm virtio_blk 
 i2c_core virtio_pci ata_generic virtio_ring virtio pata_acpi
 [1.840169] CPU: 2 PID: 266 Comm: kworker/2:2 Not tainted 3.17.0+ #1
 [1.840169] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
 [1.840169] Workqueue: events control_work_handler [virtio_console]
 [1.840169] task: 8800364bc840 ti: 88007849 task.ti: 
 88007849
 [1.840169] RIP: 0010:[a01d92c6]  [a01d92c6] 
 virtio_device_ready.part.12+0x4/0x6 [virtio_console]
 [1.840169] RSP: 0018:880078493c78  EFLAGS: 00010202
 [1.840169] RAX: 0007 RBX: 880036406200 RCX: 
 6e39
 [1.840169] RDX: c0b2 RSI:  RDI: 
 0001c0b2
 [1.840169] RBP: 880078493c78 R08: 81d2c6f8 R09: 
 
 [1.840169] R10: 0001 R11: 8800364bd000 R12: 
 880036618400
 [1.840169] R13: 0001 R14: 8800368c6800 R15: 
 880036406220
 [1.840169] FS:  () GS:88007fd0() 
 knlGS:
 [1.840169] CS:  0010 DS:  ES:  CR0: 8005003b
 [1.840169] CR2: 7f1c31c9 CR3: 01c14000 CR4: 
 06e0
 [1.840169] Stack:
 [1.840169]  880078493ce8 a01d886a 8801 
 810e20cd
 [1.840169]  880078493cb8 0282  
 87f90194
 [1.840169]  880078493ce8 88007ab1d4e0 880036618498 
 880036618410
 [1.840169] Call Trace:
 [1.840169]  [a01d886a] add_port+0x40a/0x440 [virtio_console]
 [1.840169]  [810e20cd] ? trace_hardirqs_on+0xd/0x10
 [1.840169]  [a01d8c6a] control_work_handler+0x3ca/0x420 
 [virtio_console]
 [1.840169]  [810b0e7b] ? process_one_work+0x15b/0x530
 [1.840169]  [810b0ef4] process_one_work+0x1d4/0x530
 [1.840169]  [810b0e7b] ? process_one_work+0x15b/0x530
 [1.840169]  [810b136b] worker_thread+0x11b/0x490
 [1.840169]  [810b1250] ? process_one_work+0x530/0x530
 [1.840169]  [810b67c3] kthread+0xf3/0x110
 [1.840169]  [81788f00] ? _raw_spin_unlock_irq+0x30/0x50
 [1.840169]  [810b66d0] ? kthread_create_on_node+0x240/0x240
 [1.840169]  [81789a7c] ret_from_fork+0x7c/0xb0
 [1.840169]  [810b66d0] ? kthread_create_on_node+0x240/0x240
 [1.840169] Code: ff 49 89 c4 4d 85 e4 0f 8f 25 ff ff ff eb ad 48 c7 c0 f4 
 ff ff ff e9 17 ff ff ff e8 f5 cd eb e0 90 55 48 89 e5 0f 0b 55 48 89 e5 0f 
 0b 55 48 89 e5 0f 0b 55 48 89 e5 e8 99 e2 ff ff 48 c7 c7 c0 
 [1.840169] RIP  [a01d92c6] virtio_device_ready.part.12+0x4/0x6 
 [virtio_console]
 [1.840169]  RSP 880078493c78
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 13/25] virtio_console: enable VQs early

2014-10-20 Thread Michael S. Tsirkin 
On Mon, Oct 20, 2014 at 04:10:16PM +0300, Michael S. Tsirkin wrote:
 On Mon, Oct 20, 2014 at 01:07:50PM +0100, Thomas Graf wrote:
  On 10/13/14 at 10:50am, Michael S. Tsirkin wrote:
   virtio spec requires drivers to set DRIVER_OK before using VQs.
   This is set automatically after probe returns, virtio console violated 
   this
   rule by adding inbufs, which causes the VQ to be used directly within
   probe.
   
   To fix, call virtio_device_ready before using VQs.
   
   Signed-off-by: Michael S. Tsirkin m...@redhat.com
   ---
drivers/char/virtio_console.c | 2 ++
1 file changed, 2 insertions(+)
   
   diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
   index b585b47..6ebe8f6 100644
   --- a/drivers/char/virtio_console.c
   +++ b/drivers/char/virtio_console.c
   @@ -1449,6 +1449,8 @@ static int add_port(struct ports_device *portdev, 
   u32 id)
 spin_lock_init(port-outvq_lock);
 init_waitqueue_head(port-waitqueue);

   + virtio_device_ready(portdev-vdev);
   +
 /* Fill the in_vq with buffers so the host can send us data. */
 nr_added_bufs = fill_queue(port-in_vq, port-inbuf_lock);
 if (!nr_added_bufs) {
 
 I see Cornelia sent a patch already.
 I'd like to reproduce this though - could you send me
 the command line please?

Nevermind, the trick is to add a port it seems:

-device virtio-serial -chardev socket,path=/tmp/c1,server,nowait,id=foo
-device virtserialport,chardev=foo,name=org.fedoraproject.port.0

works fine without -device virtserialport.

-- 
MST
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 13/25] virtio_console: enable VQs early

2014-10-20 Thread Michael S. Tsirkin 
On Mon, Oct 20, 2014 at 02:42:23PM +0200, Cornelia Huck wrote:
 On Mon, 20 Oct 2014 13:07:50 +0100
 Thomas Graf tg...@suug.ch wrote:
 
  On 10/13/14 at 10:50am, Michael S. Tsirkin wrote:
   virtio spec requires drivers to set DRIVER_OK before using VQs.
   This is set automatically after probe returns, virtio console violated 
   this
   rule by adding inbufs, which causes the VQ to be used directly within
   probe.
   
   To fix, call virtio_device_ready before using VQs.
   
   Signed-off-by: Michael S. Tsirkin m...@redhat.com
   ---
drivers/char/virtio_console.c | 2 ++
1 file changed, 2 insertions(+)
   
   diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
   index b585b47..6ebe8f6 100644
   --- a/drivers/char/virtio_console.c
   +++ b/drivers/char/virtio_console.c
   @@ -1449,6 +1449,8 @@ static int add_port(struct ports_device *portdev, 
   u32 id)
 spin_lock_init(port-outvq_lock);
 init_waitqueue_head(port-waitqueue);

   + virtio_device_ready(portdev-vdev);
   +
 /* Fill the in_vq with buffers so the host can send us data. */
 nr_added_bufs = fill_queue(port-in_vq, port-inbuf_lock);
 if (!nr_added_bufs) {
  
  Seems like probe and add_port() now both set VIRTIO_CONFIG_S_DRIVER_OK
 
 I think we need to set this in the probe function instead, otherwise we
 fail for multiqueue (which also wants to use the control queue early).
 
 Completely untested patch below; I can send this with proper s-o-b if
 it helps.
 
 diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
 index bfa6400..cf7a561 100644
 --- a/drivers/char/virtio_console.c
 +++ b/drivers/char/virtio_console.c
 @@ -1449,8 +1449,6 @@ static int add_port(struct ports_device *portdev, u32 
 id)
   spin_lock_init(port-outvq_lock);
   init_waitqueue_head(port-waitqueue);
  
 - virtio_device_ready(portdev-vdev);
 -
   /* Fill the in_vq with buffers so the host can send us data. */
   nr_added_bufs = fill_queue(port-in_vq, port-inbuf_lock);
   if (!nr_added_bufs) {
 @@ -2026,6 +2024,8 @@ static int virtcons_probe(struct virtio_device *vdev)
   spin_lock_init(portdev-ports_lock);
   INIT_LIST_HEAD(portdev-ports);
  
 + virtio_device_ready(portdev-vdev);
 +
   if (multiport) {
   unsigned int nr_added_bufs;
  

I wanted to set DRIVER_OK as late as possible, to both reduce
the chance it can fail after DRIVER_OK and to reduce  the risk of
introducing a regression since old qemu might only start sending
interrupts after DRIVER_OK is set.

So I wanted everything completely initialized before DRIVER_OK.

You patch makes sense to me since nothing can fail,
and we won't get interrupts before we add inbufs.

Reviewed-by: Michael S. Tsirkin m...@redhat.com

Testig will report shortly, pls send with sob line.

-- 
MST
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 13/25] virtio_console: enable VQs early

2014-10-20 Thread Michael S. Tsirkin 
On Mon, Oct 20, 2014 at 04:35:55PM +0300, Michael S. Tsirkin wrote:
 On Mon, Oct 20, 2014 at 02:42:23PM +0200, Cornelia Huck wrote:
  On Mon, 20 Oct 2014 13:07:50 +0100
  Thomas Graf tg...@suug.ch wrote:
  
   On 10/13/14 at 10:50am, Michael S. Tsirkin wrote:
virtio spec requires drivers to set DRIVER_OK before using VQs.
This is set automatically after probe returns, virtio console violated 
this
rule by adding inbufs, which causes the VQ to be used directly within
probe.

To fix, call virtio_device_ready before using VQs.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/char/virtio_console.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/virtio_console.c 
b/drivers/char/virtio_console.c
index b585b47..6ebe8f6 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1449,6 +1449,8 @@ static int add_port(struct ports_device *portdev, 
u32 id)
spin_lock_init(port-outvq_lock);
init_waitqueue_head(port-waitqueue);
 
+   virtio_device_ready(portdev-vdev);
+
/* Fill the in_vq with buffers so the host can send us data. */
nr_added_bufs = fill_queue(port-in_vq, port-inbuf_lock);
if (!nr_added_bufs) {
   
   Seems like probe and add_port() now both set VIRTIO_CONFIG_S_DRIVER_OK
  
  I think we need to set this in the probe function instead, otherwise we
  fail for multiqueue (which also wants to use the control queue early).
  
  Completely untested patch below; I can send this with proper s-o-b if
  it helps.
  
  diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
  index bfa6400..cf7a561 100644
  --- a/drivers/char/virtio_console.c
  +++ b/drivers/char/virtio_console.c
  @@ -1449,8 +1449,6 @@ static int add_port(struct ports_device *portdev, u32 
  id)
  spin_lock_init(port-outvq_lock);
  init_waitqueue_head(port-waitqueue);
   
  -   virtio_device_ready(portdev-vdev);
  -
  /* Fill the in_vq with buffers so the host can send us data. */
  nr_added_bufs = fill_queue(port-in_vq, port-inbuf_lock);
  if (!nr_added_bufs) {
  @@ -2026,6 +2024,8 @@ static int virtcons_probe(struct virtio_device *vdev)
  spin_lock_init(portdev-ports_lock);
  INIT_LIST_HEAD(portdev-ports);
   
  +   virtio_device_ready(portdev-vdev);
  +
  if (multiport) {
  unsigned int nr_added_bufs;
   
 
 I wanted to set DRIVER_OK as late as possible, to both reduce
 the chance it can fail after DRIVER_OK and to reduce  the risk of
 introducing a regression since old qemu might only start sending
 interrupts after DRIVER_OK is set.
 
 So I wanted everything completely initialized before DRIVER_OK.
 
 You patch makes sense to me since nothing can fail,
 and we won't get interrupts before we add inbufs.
 
 Reviewed-by: Michael S. Tsirkin m...@redhat.com
 
 Testig will report shortly, pls send with sob line.

Sure enough, this helps:

Tested-by: Michael S. Tsirkin m...@redhat.com
Acked-by: Michael S. Tsirkin m...@redhat.com

Pls repost as a top-level patch.

 -- 
 MST
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] vhost-scsi: Take configfs group dependency during VHOST_SCSI_SET_ENDPOINT

2014-10-21 Thread Michael S. Tsirkin 
On Tue, Oct 21, 2014 at 12:05:31PM -0700, Nicholas A. Bellinger wrote:
 Hey Paolo,
 
 On Thu, 2014-10-09 at 12:49 +0200, Paolo Bonzini wrote:
  Il 09/10/2014 10:49, Paolo Bonzini ha scritto:
   
   It does not happen if you close QEMU with SIGTERM, ctrl-c, or with the
   quit command, because no attempt is done to bring down the VM data
   structures (or free memory, or close file descriptors) in case of a
   fatal exit.  The kernel should do that for us.
  
  ... and in the case of vhost-scsi, doesn't it do that when
  vhost_scsi_release calls vhost_scsi_clear_endpoint?
  
 
 Thanks for the extra clarifications here.
 
 The SIGTERM, ctrl-c and quit command cases happen as you describe, and
 invoke vhost_scsi_release() - vhost_scsi_clear_endpoint() to drop the
 endpoint reference.
 
 AFAICT, it's the SIGKILL case that is problematic both with and without
 this patch.  With the patch, the configfs dependency on the vhost-scsi
 endpoint group is left in place, thus preventing the group (and
 underlying target_core_mod) from being removed until a
 VHOST_SCSI_CLEAR_ENDPOINT with the same wwpn is called to drop the
 original reference.
 
 Without the patch, the group can still be removed at any time, but any
 subsequent VHOST_SCSI_SET_ENDPOINT attempts with the original wwpn will
 fail after SIGKILL, because the original reference is still in place.
 
 So I held off on pushing this patch to -rc1 for the moment, but even
 with the above limitation preventing group shutdown after SIGKILL, I
 think it's still better to obtain the configfs dependency to prevent the
 removal of endpoints while there are active references.
 
 That said, I'm still unsure how to address the SIGKILL case, and what's
 the most sane way to drop dead references after it happens, and how
 vhost-scsi should be differentiating between dead and active references.
 
 Any ideas..?
 
 --nab

Need to use some other file (not sysfs), cleanup on release.

-- 
MST
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 10/25] virtio: add API to enable VQs early

2014-11-10 Thread Michael S. Tsirkin 
On Mon, Nov 10, 2014 at 04:45:09PM -0800, Andy Grover wrote:
 On 10/13/2014 12:50 AM, Michael S. Tsirkin wrote:
 virtio spec 0.9.X requires DRIVER_OK to be set before
 VQs are used, but some drivers use VQs before probe
 function returns.
 Since DRIVER_OK is set after probe, this violates the spec.
 
 Even though under virtio 1.0 transitional devices support this
 behaviour, we want to make it possible for those early callers to become
 spec compliant and eventually support non-transitional devices.
 
 Add API for drivers to call before using VQs.
 
 Sets DRIVER_OK internally.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com
 ---
   include/linux/virtio_config.h | 17 +
   1 file changed, 17 insertions(+)
 
 diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
 index e8f8f71..e36403b 100644
 --- a/include/linux/virtio_config.h
 +++ b/include/linux/virtio_config.h
 @@ -109,6 +109,23 @@ struct virtqueue *virtio_find_single_vq(struct 
 virtio_device *vdev,
  return vq;
   }
 
 +/**
 + * virtio_device_ready - enable vq use in probe function
 + * @vdev: the device
 + *
 + * Driver must call this to use vqs in the probe function.
 + *
 + * Note: vqs are enabled automatically after probe returns.
 + */
 +static inline
 +void virtio_device_ready(struct virtio_device *dev)
 +{
 +unsigned status = dev-config-get_status(dev);
 +
 +BUG_ON(status  VIRTIO_CONFIG_S_DRIVER_OK);
 +dev-config-set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK);
 +}
 
 Getting a BUG when booting via KVM, host Fedora 20, guest Fedora 20.
 
 my config is at:
 
 https://fedorapeople.org/~grover/config-20141110
 

The fix is here:
http://article.gmane.org/gmane.linux.kernel.virtualization/23324/raw

I'm surprised it's not merged yet.

Rusty, could you pick it up please?


 
 [0.828494] [ cut here ]
 [0.829039] kernel BUG at
 /home/agrover/git/kernel/include/linux/virtio_config.h:125!
 [0.831266] invalid opcode:  [#1] SMP DEBUG_PAGEALLOC
 [0.831266] Modules linked in:
 [0.831266] CPU: 1 PID: 30 Comm: kworker/1:1 Not tainted 3.18.0-rc4 #120
 [0.831266] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
 [0.831266] Workqueue: events control_work_handler
 [0.831266] task: 88003cd98000 ti: 88003cd94000 task.ti:
 88003cd94000
 [0.831266] RIP: 0010:[81445004]  [81445004]
 add_port+0x264/0x410
 [0.831266] RSP: :88003cd97c78  EFLAGS: 00010202
 [0.831266] RAX: 0007 RBX: 88003c58c400 RCX:
 0001
 [0.831266] RDX: c132 RSI: 81a955e9 RDI:
 0001c132
 [0.831266] RBP: 88003cd97cc8 R08:  R09:
 
 [0.831266] R10: 0001 R11:  R12:
 88003c58be00
 [0.831266] R13: 0001 R14: 8800395ca800 R15:
 88003c58c420
 [0.831266] FS:  () GS:88003fa0()
 knlGS:
 [0.831266] CS:  0010 DS:  ES:  CR0: 8005003b
 [0.831266] CR2:  CR3: 01c11000 CR4:
 06e0
 [0.831266] DR0:  DR1:  DR2:
 
 [0.831266] DR3:  DR6: fffe0ff0 DR7:
 0400
 [0.831266] Stack:
 [0.831266]  8801 0292 
 0001
 [0.831266]  88003cd97cc8 88003dfa8a20 88003c58beb8
 88003c58be10
 [0.831266]  8800395a2000  88003cd97d38
 8144531a
 [0.831266] Call Trace:
 [0.831266]  [8144531a] control_work_handler+0x16a/0x3c0
 [0.831266]  [8108b0c8] ? process_one_work+0x208/0x500
 [0.831266]  [8108b16c] process_one_work+0x2ac/0x500
 [0.831266]  [8108b0c8] ? process_one_work+0x208/0x500
 [0.831266]  [8108b68e] worker_thread+0x2ce/0x4e0
 [0.831266]  [8108b3c0] ? process_one_work+0x500/0x500
 [0.831266]  [81090b28] kthread+0xf8/0x100
 [0.831266]  [810bad7d] ? trace_hardirqs_on+0xd/0x10
 [0.831266]  [81090a30] ? kthread_stop+0x140/0x140
 [0.831266]  [816ea92c] ret_from_fork+0x7c/0xb0
 [0.831266]  [81090a30] ? kthread_stop+0x140/0x140
 [0.831266] Code: c7 c2 48 31 01 83 48 c7 c6 e9 55 a9 81 e8 55 b4 c6 ff
 4d 8b b4 24 58 01 00 00 49 8b 86 e8 04 00 00 4c 89 f7 ff 50 10 a8 04 74 0c
 0f 0b 66 2e 0f 1f 84 00 00 00 00 00 49 8b 96 e8 04 00 00 83 c8
 [0.831266] RIP  [81445004] add_port+0x264/0x410
 [0.831266]  RSP 88003cd97c78
 [0.878202] ---[ end trace f98fbb172cc7bbf4 ]---
 
 Thanks -- Andy
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 37/41] virtio_scsi: v1.0 support

2014-11-24 Thread Michael S. Tsirkin 
Note: for consistency, and to avoid sparse errors,
  convert all fields, even those no longer in use
  for virtio v1.0.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 include/linux/virtio_scsi.h | 32 +++-
 drivers/scsi/virtio_scsi.c  | 51 -
 2 files changed, 49 insertions(+), 34 deletions(-)

diff --git a/include/linux/virtio_scsi.h b/include/linux/virtio_scsi.h
index de429d1..af44864 100644
--- a/include/linux/virtio_scsi.h
+++ b/include/linux/virtio_scsi.h
@@ -27,13 +27,15 @@
 #ifndef _LINUX_VIRTIO_SCSI_H
 #define _LINUX_VIRTIO_SCSI_H
 
+#include linux/virtio_types.h
+
 #define VIRTIO_SCSI_CDB_SIZE   32
 #define VIRTIO_SCSI_SENSE_SIZE 96
 
 /* SCSI command request, followed by data-out */
 struct virtio_scsi_cmd_req {
u8 lun[8];  /* Logical Unit Number */
-   u64 tag;/* Command identifier */
+   __virtio64 tag; /* Command identifier */
u8 task_attr;   /* Task attribute */
u8 prio;/* SAM command priority field */
u8 crn;
@@ -43,20 +45,20 @@ struct virtio_scsi_cmd_req {
 /* SCSI command request, followed by protection information */
 struct virtio_scsi_cmd_req_pi {
u8 lun[8];  /* Logical Unit Number */
-   u64 tag;/* Command identifier */
+   __virtio64 tag; /* Command identifier */
u8 task_attr;   /* Task attribute */
u8 prio;/* SAM command priority field */
u8 crn;
-   u32 pi_bytesout;/* DataOUT PI Number of bytes */
-   u32 pi_bytesin; /* DataIN PI Number of bytes */
+   __virtio32 pi_bytesout; /* DataOUT PI Number of bytes */
+   __virtio32 pi_bytesin;  /* DataIN PI Number of bytes */
u8 cdb[VIRTIO_SCSI_CDB_SIZE];
 } __packed;
 
 /* Response, followed by sense data and data-in */
 struct virtio_scsi_cmd_resp {
-   u32 sense_len;  /* Sense data length */
-   u32 resid;  /* Residual bytes in data buffer */
-   u16 status_qualifier;   /* Status qualifier */
+   __virtio32 sense_len;   /* Sense data length */
+   __virtio32 resid;   /* Residual bytes in data buffer */
+   __virtio16 status_qualifier;/* Status qualifier */
u8 status;  /* Command completion status */
u8 response;/* Response values */
u8 sense[VIRTIO_SCSI_SENSE_SIZE];
@@ -64,10 +66,10 @@ struct virtio_scsi_cmd_resp {
 
 /* Task Management Request */
 struct virtio_scsi_ctrl_tmf_req {
-   u32 type;
-   u32 subtype;
+   __virtio32 type;
+   __virtio32 subtype;
u8 lun[8];
-   u64 tag;
+   __virtio64 tag;
 } __packed;
 
 struct virtio_scsi_ctrl_tmf_resp {
@@ -76,20 +78,20 @@ struct virtio_scsi_ctrl_tmf_resp {
 
 /* Asynchronous notification query/subscription */
 struct virtio_scsi_ctrl_an_req {
-   u32 type;
+   __virtio32 type;
u8 lun[8];
-   u32 event_requested;
+   __virtio32 event_requested;
 } __packed;
 
 struct virtio_scsi_ctrl_an_resp {
-   u32 event_actual;
+   __virtio32 event_actual;
u8 response;
 } __packed;
 
 struct virtio_scsi_event {
-   u32 event;
+   __virtio32 event;
u8 lun[8];
-   u32 reason;
+   __virtio32 reason;
 } __packed;
 
 struct virtio_scsi_config {
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index b83846f..c2779ea 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -158,7 +158,7 @@ static void virtscsi_complete_cmd(struct virtio_scsi 
*vscsi, void *buf)
sc, resp-response, resp-status, resp-sense_len);
 
sc-result = resp-status;
-   virtscsi_compute_resid(sc, resp-resid);
+   virtscsi_compute_resid(sc, __virtio32_to_cpu(vscsi-vdev, resp-resid));
switch (resp-response) {
case VIRTIO_SCSI_S_OK:
set_host_byte(sc, DID_OK);
@@ -196,10 +196,13 @@ static void virtscsi_complete_cmd(struct virtio_scsi 
*vscsi, void *buf)
break;
}
 
-   WARN_ON(resp-sense_len  VIRTIO_SCSI_SENSE_SIZE);
+   WARN_ON(__virtio32_to_cpu(vscsi-vdev, resp-sense_len) 
+   VIRTIO_SCSI_SENSE_SIZE);
if (sc-sense_buffer) {
memcpy(sc-sense_buffer, resp-sense,
-  min_t(u32, resp-sense_len, VIRTIO_SCSI_SENSE_SIZE));
+  min_t(u32,
+__virtio32_to_cpu(vscsi-vdev, resp-sense_len),
+VIRTIO_SCSI_SENSE_SIZE));
if (resp-sense_len)
set_driver_byte(sc, DRIVER_SENSE);
}
@@ -323,7 +326,7 @@ static void virtscsi_handle_transport_reset(struct 
virtio_scsi *vscsi,
unsigned int target = event-lun[1];
unsigned int lun = (event-lun[2]  8) | event-lun[3];
 
-   switch (event-reason) {
+   switch

[PATCH v4 38/42] virtio_scsi: v1.0 support

2014-11-25 Thread Michael S. Tsirkin 
Note: for consistency, and to avoid sparse errors,
  convert all fields, even those no longer in use
  for virtio v1.0.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 include/linux/virtio_scsi.h | 32 +++-
 drivers/scsi/virtio_scsi.c  | 51 -
 2 files changed, 49 insertions(+), 34 deletions(-)

diff --git a/include/linux/virtio_scsi.h b/include/linux/virtio_scsi.h
index de429d1..af44864 100644
--- a/include/linux/virtio_scsi.h
+++ b/include/linux/virtio_scsi.h
@@ -27,13 +27,15 @@
 #ifndef _LINUX_VIRTIO_SCSI_H
 #define _LINUX_VIRTIO_SCSI_H
 
+#include linux/virtio_types.h
+
 #define VIRTIO_SCSI_CDB_SIZE   32
 #define VIRTIO_SCSI_SENSE_SIZE 96
 
 /* SCSI command request, followed by data-out */
 struct virtio_scsi_cmd_req {
u8 lun[8];  /* Logical Unit Number */
-   u64 tag;/* Command identifier */
+   __virtio64 tag; /* Command identifier */
u8 task_attr;   /* Task attribute */
u8 prio;/* SAM command priority field */
u8 crn;
@@ -43,20 +45,20 @@ struct virtio_scsi_cmd_req {
 /* SCSI command request, followed by protection information */
 struct virtio_scsi_cmd_req_pi {
u8 lun[8];  /* Logical Unit Number */
-   u64 tag;/* Command identifier */
+   __virtio64 tag; /* Command identifier */
u8 task_attr;   /* Task attribute */
u8 prio;/* SAM command priority field */
u8 crn;
-   u32 pi_bytesout;/* DataOUT PI Number of bytes */
-   u32 pi_bytesin; /* DataIN PI Number of bytes */
+   __virtio32 pi_bytesout; /* DataOUT PI Number of bytes */
+   __virtio32 pi_bytesin;  /* DataIN PI Number of bytes */
u8 cdb[VIRTIO_SCSI_CDB_SIZE];
 } __packed;
 
 /* Response, followed by sense data and data-in */
 struct virtio_scsi_cmd_resp {
-   u32 sense_len;  /* Sense data length */
-   u32 resid;  /* Residual bytes in data buffer */
-   u16 status_qualifier;   /* Status qualifier */
+   __virtio32 sense_len;   /* Sense data length */
+   __virtio32 resid;   /* Residual bytes in data buffer */
+   __virtio16 status_qualifier;/* Status qualifier */
u8 status;  /* Command completion status */
u8 response;/* Response values */
u8 sense[VIRTIO_SCSI_SENSE_SIZE];
@@ -64,10 +66,10 @@ struct virtio_scsi_cmd_resp {
 
 /* Task Management Request */
 struct virtio_scsi_ctrl_tmf_req {
-   u32 type;
-   u32 subtype;
+   __virtio32 type;
+   __virtio32 subtype;
u8 lun[8];
-   u64 tag;
+   __virtio64 tag;
 } __packed;
 
 struct virtio_scsi_ctrl_tmf_resp {
@@ -76,20 +78,20 @@ struct virtio_scsi_ctrl_tmf_resp {
 
 /* Asynchronous notification query/subscription */
 struct virtio_scsi_ctrl_an_req {
-   u32 type;
+   __virtio32 type;
u8 lun[8];
-   u32 event_requested;
+   __virtio32 event_requested;
 } __packed;
 
 struct virtio_scsi_ctrl_an_resp {
-   u32 event_actual;
+   __virtio32 event_actual;
u8 response;
 } __packed;
 
 struct virtio_scsi_event {
-   u32 event;
+   __virtio32 event;
u8 lun[8];
-   u32 reason;
+   __virtio32 reason;
 } __packed;
 
 struct virtio_scsi_config {
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index b83846f..c2779ea 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -158,7 +158,7 @@ static void virtscsi_complete_cmd(struct virtio_scsi 
*vscsi, void *buf)
sc, resp-response, resp-status, resp-sense_len);
 
sc-result = resp-status;
-   virtscsi_compute_resid(sc, resp-resid);
+   virtscsi_compute_resid(sc, __virtio32_to_cpu(vscsi-vdev, resp-resid));
switch (resp-response) {
case VIRTIO_SCSI_S_OK:
set_host_byte(sc, DID_OK);
@@ -196,10 +196,13 @@ static void virtscsi_complete_cmd(struct virtio_scsi 
*vscsi, void *buf)
break;
}
 
-   WARN_ON(resp-sense_len  VIRTIO_SCSI_SENSE_SIZE);
+   WARN_ON(__virtio32_to_cpu(vscsi-vdev, resp-sense_len) 
+   VIRTIO_SCSI_SENSE_SIZE);
if (sc-sense_buffer) {
memcpy(sc-sense_buffer, resp-sense,
-  min_t(u32, resp-sense_len, VIRTIO_SCSI_SENSE_SIZE));
+  min_t(u32,
+__virtio32_to_cpu(vscsi-vdev, resp-sense_len),
+VIRTIO_SCSI_SENSE_SIZE));
if (resp-sense_len)
set_driver_byte(sc, DRIVER_SENSE);
}
@@ -323,7 +326,7 @@ static void virtscsi_handle_transport_reset(struct 
virtio_scsi *vscsi,
unsigned int target = event-lun[1];
unsigned int lun = (event-lun[2]  8) | event-lun[3];
 
-   switch (event-reason) {
+   switch

Re: [PATCH v4 38/42] virtio_scsi: v1.0 support

2014-11-26 Thread Michael S. Tsirkin 
On Wed, Nov 26, 2014 at 03:51:03PM +0100, Cornelia Huck wrote:
 On Tue, 25 Nov 2014 18:44:08 +0200
 Michael S. Tsirkin m...@redhat.com wrote:
 
  Note: for consistency, and to avoid sparse errors,
convert all fields, even those no longer in use
for virtio v1.0.
  
  Signed-off-by: Michael S. Tsirkin m...@redhat.com
  ---
   include/linux/virtio_scsi.h | 32 +++-
   drivers/scsi/virtio_scsi.c  | 51 
  -
   2 files changed, 49 insertions(+), 34 deletions(-)
  
 
  @@ -158,7 +158,7 @@ static void virtscsi_complete_cmd(struct virtio_scsi 
  *vscsi, void *buf)
  sc, resp-response, resp-status, resp-sense_len);
  
  sc-result = resp-status;
  -   virtscsi_compute_resid(sc, resp-resid);
  +   virtscsi_compute_resid(sc, __virtio32_to_cpu(vscsi-vdev, resp-resid));
 
 Confused. Don't you need the non-underscore versions if you pass in a
 vdev as first parameter?

Ouch, this built because vscsi-vdev is true.
thanks!

  switch (resp-response) {
  case VIRTIO_SCSI_S_OK:
  set_host_byte(sc, DID_OK);
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 41/45] virtio_scsi: v1.0 support

2014-11-27 Thread Michael S. Tsirkin 
Note: for consistency, and to avoid sparse errors,
  convert all fields, even those no longer in use
  for virtio v1.0.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Acked-by: Paolo Bonzini pbonz...@redhat.com
---
 include/linux/virtio_scsi.h | 32 +++-
 drivers/scsi/virtio_scsi.c  | 51 -
 2 files changed, 49 insertions(+), 34 deletions(-)

diff --git a/include/linux/virtio_scsi.h b/include/linux/virtio_scsi.h
index de429d1..af44864 100644
--- a/include/linux/virtio_scsi.h
+++ b/include/linux/virtio_scsi.h
@@ -27,13 +27,15 @@
 #ifndef _LINUX_VIRTIO_SCSI_H
 #define _LINUX_VIRTIO_SCSI_H
 
+#include linux/virtio_types.h
+
 #define VIRTIO_SCSI_CDB_SIZE   32
 #define VIRTIO_SCSI_SENSE_SIZE 96
 
 /* SCSI command request, followed by data-out */
 struct virtio_scsi_cmd_req {
u8 lun[8];  /* Logical Unit Number */
-   u64 tag;/* Command identifier */
+   __virtio64 tag; /* Command identifier */
u8 task_attr;   /* Task attribute */
u8 prio;/* SAM command priority field */
u8 crn;
@@ -43,20 +45,20 @@ struct virtio_scsi_cmd_req {
 /* SCSI command request, followed by protection information */
 struct virtio_scsi_cmd_req_pi {
u8 lun[8];  /* Logical Unit Number */
-   u64 tag;/* Command identifier */
+   __virtio64 tag; /* Command identifier */
u8 task_attr;   /* Task attribute */
u8 prio;/* SAM command priority field */
u8 crn;
-   u32 pi_bytesout;/* DataOUT PI Number of bytes */
-   u32 pi_bytesin; /* DataIN PI Number of bytes */
+   __virtio32 pi_bytesout; /* DataOUT PI Number of bytes */
+   __virtio32 pi_bytesin;  /* DataIN PI Number of bytes */
u8 cdb[VIRTIO_SCSI_CDB_SIZE];
 } __packed;
 
 /* Response, followed by sense data and data-in */
 struct virtio_scsi_cmd_resp {
-   u32 sense_len;  /* Sense data length */
-   u32 resid;  /* Residual bytes in data buffer */
-   u16 status_qualifier;   /* Status qualifier */
+   __virtio32 sense_len;   /* Sense data length */
+   __virtio32 resid;   /* Residual bytes in data buffer */
+   __virtio16 status_qualifier;/* Status qualifier */
u8 status;  /* Command completion status */
u8 response;/* Response values */
u8 sense[VIRTIO_SCSI_SENSE_SIZE];
@@ -64,10 +66,10 @@ struct virtio_scsi_cmd_resp {
 
 /* Task Management Request */
 struct virtio_scsi_ctrl_tmf_req {
-   u32 type;
-   u32 subtype;
+   __virtio32 type;
+   __virtio32 subtype;
u8 lun[8];
-   u64 tag;
+   __virtio64 tag;
 } __packed;
 
 struct virtio_scsi_ctrl_tmf_resp {
@@ -76,20 +78,20 @@ struct virtio_scsi_ctrl_tmf_resp {
 
 /* Asynchronous notification query/subscription */
 struct virtio_scsi_ctrl_an_req {
-   u32 type;
+   __virtio32 type;
u8 lun[8];
-   u32 event_requested;
+   __virtio32 event_requested;
 } __packed;
 
 struct virtio_scsi_ctrl_an_resp {
-   u32 event_actual;
+   __virtio32 event_actual;
u8 response;
 } __packed;
 
 struct virtio_scsi_event {
-   u32 event;
+   __virtio32 event;
u8 lun[8];
-   u32 reason;
+   __virtio32 reason;
 } __packed;
 
 struct virtio_scsi_config {
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index b83846f..d9ec806 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -158,7 +158,7 @@ static void virtscsi_complete_cmd(struct virtio_scsi 
*vscsi, void *buf)
sc, resp-response, resp-status, resp-sense_len);
 
sc-result = resp-status;
-   virtscsi_compute_resid(sc, resp-resid);
+   virtscsi_compute_resid(sc, virtio32_to_cpu(vscsi-vdev, resp-resid));
switch (resp-response) {
case VIRTIO_SCSI_S_OK:
set_host_byte(sc, DID_OK);
@@ -196,10 +196,13 @@ static void virtscsi_complete_cmd(struct virtio_scsi 
*vscsi, void *buf)
break;
}
 
-   WARN_ON(resp-sense_len  VIRTIO_SCSI_SENSE_SIZE);
+   WARN_ON(virtio32_to_cpu(vscsi-vdev, resp-sense_len) 
+   VIRTIO_SCSI_SENSE_SIZE);
if (sc-sense_buffer) {
memcpy(sc-sense_buffer, resp-sense,
-  min_t(u32, resp-sense_len, VIRTIO_SCSI_SENSE_SIZE));
+  min_t(u32,
+virtio32_to_cpu(vscsi-vdev, resp-sense_len),
+VIRTIO_SCSI_SENSE_SIZE));
if (resp-sense_len)
set_driver_byte(sc, DRIVER_SENSE);
}
@@ -323,7 +326,7 @@ static void virtscsi_handle_transport_reset(struct 
virtio_scsi *vscsi,
unsigned int target = event-lun[1];
unsigned int lun = (event-lun[2]  8) | event-lun[3];
 
-   switch (event

[PATCH v6 42/46] virtio_scsi: v1.0 support

2014-11-27 Thread Michael S. Tsirkin 
Note: for consistency, and to avoid sparse errors,
  convert all fields, even those no longer in use
  for virtio v1.0.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Acked-by: Paolo Bonzini pbonz...@redhat.com
---
 include/linux/virtio_scsi.h | 32 +++-
 drivers/scsi/virtio_scsi.c  | 51 -
 2 files changed, 49 insertions(+), 34 deletions(-)

diff --git a/include/linux/virtio_scsi.h b/include/linux/virtio_scsi.h
index de429d1..af44864 100644
--- a/include/linux/virtio_scsi.h
+++ b/include/linux/virtio_scsi.h
@@ -27,13 +27,15 @@
 #ifndef _LINUX_VIRTIO_SCSI_H
 #define _LINUX_VIRTIO_SCSI_H
 
+#include linux/virtio_types.h
+
 #define VIRTIO_SCSI_CDB_SIZE   32
 #define VIRTIO_SCSI_SENSE_SIZE 96
 
 /* SCSI command request, followed by data-out */
 struct virtio_scsi_cmd_req {
u8 lun[8];  /* Logical Unit Number */
-   u64 tag;/* Command identifier */
+   __virtio64 tag; /* Command identifier */
u8 task_attr;   /* Task attribute */
u8 prio;/* SAM command priority field */
u8 crn;
@@ -43,20 +45,20 @@ struct virtio_scsi_cmd_req {
 /* SCSI command request, followed by protection information */
 struct virtio_scsi_cmd_req_pi {
u8 lun[8];  /* Logical Unit Number */
-   u64 tag;/* Command identifier */
+   __virtio64 tag; /* Command identifier */
u8 task_attr;   /* Task attribute */
u8 prio;/* SAM command priority field */
u8 crn;
-   u32 pi_bytesout;/* DataOUT PI Number of bytes */
-   u32 pi_bytesin; /* DataIN PI Number of bytes */
+   __virtio32 pi_bytesout; /* DataOUT PI Number of bytes */
+   __virtio32 pi_bytesin;  /* DataIN PI Number of bytes */
u8 cdb[VIRTIO_SCSI_CDB_SIZE];
 } __packed;
 
 /* Response, followed by sense data and data-in */
 struct virtio_scsi_cmd_resp {
-   u32 sense_len;  /* Sense data length */
-   u32 resid;  /* Residual bytes in data buffer */
-   u16 status_qualifier;   /* Status qualifier */
+   __virtio32 sense_len;   /* Sense data length */
+   __virtio32 resid;   /* Residual bytes in data buffer */
+   __virtio16 status_qualifier;/* Status qualifier */
u8 status;  /* Command completion status */
u8 response;/* Response values */
u8 sense[VIRTIO_SCSI_SENSE_SIZE];
@@ -64,10 +66,10 @@ struct virtio_scsi_cmd_resp {
 
 /* Task Management Request */
 struct virtio_scsi_ctrl_tmf_req {
-   u32 type;
-   u32 subtype;
+   __virtio32 type;
+   __virtio32 subtype;
u8 lun[8];
-   u64 tag;
+   __virtio64 tag;
 } __packed;
 
 struct virtio_scsi_ctrl_tmf_resp {
@@ -76,20 +78,20 @@ struct virtio_scsi_ctrl_tmf_resp {
 
 /* Asynchronous notification query/subscription */
 struct virtio_scsi_ctrl_an_req {
-   u32 type;
+   __virtio32 type;
u8 lun[8];
-   u32 event_requested;
+   __virtio32 event_requested;
 } __packed;
 
 struct virtio_scsi_ctrl_an_resp {
-   u32 event_actual;
+   __virtio32 event_actual;
u8 response;
 } __packed;
 
 struct virtio_scsi_event {
-   u32 event;
+   __virtio32 event;
u8 lun[8];
-   u32 reason;
+   __virtio32 reason;
 } __packed;
 
 struct virtio_scsi_config {
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index b83846f..d9ec806 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -158,7 +158,7 @@ static void virtscsi_complete_cmd(struct virtio_scsi 
*vscsi, void *buf)
sc, resp-response, resp-status, resp-sense_len);
 
sc-result = resp-status;
-   virtscsi_compute_resid(sc, resp-resid);
+   virtscsi_compute_resid(sc, virtio32_to_cpu(vscsi-vdev, resp-resid));
switch (resp-response) {
case VIRTIO_SCSI_S_OK:
set_host_byte(sc, DID_OK);
@@ -196,10 +196,13 @@ static void virtscsi_complete_cmd(struct virtio_scsi 
*vscsi, void *buf)
break;
}
 
-   WARN_ON(resp-sense_len  VIRTIO_SCSI_SENSE_SIZE);
+   WARN_ON(virtio32_to_cpu(vscsi-vdev, resp-sense_len) 
+   VIRTIO_SCSI_SENSE_SIZE);
if (sc-sense_buffer) {
memcpy(sc-sense_buffer, resp-sense,
-  min_t(u32, resp-sense_len, VIRTIO_SCSI_SENSE_SIZE));
+  min_t(u32,
+virtio32_to_cpu(vscsi-vdev, resp-sense_len),
+VIRTIO_SCSI_SENSE_SIZE));
if (resp-sense_len)
set_driver_byte(sc, DRIVER_SENSE);
}
@@ -323,7 +326,7 @@ static void virtscsi_handle_transport_reset(struct 
virtio_scsi *vscsi,
unsigned int target = event-lun[1];
unsigned int lun = (event-lun[2]  8) | event-lun[3];
 
-   switch (event

[PATCH v7 42/46] virtio_scsi: v1.0 support

2014-11-30 Thread Michael S. Tsirkin 
Note: for consistency, and to avoid sparse errors,
  convert all fields, even those no longer in use
  for virtio v1.0.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Acked-by: Paolo Bonzini pbonz...@redhat.com
---
 include/linux/virtio_scsi.h | 32 +++-
 drivers/scsi/virtio_scsi.c  | 51 -
 2 files changed, 49 insertions(+), 34 deletions(-)

diff --git a/include/linux/virtio_scsi.h b/include/linux/virtio_scsi.h
index de429d1..af44864 100644
--- a/include/linux/virtio_scsi.h
+++ b/include/linux/virtio_scsi.h
@@ -27,13 +27,15 @@
 #ifndef _LINUX_VIRTIO_SCSI_H
 #define _LINUX_VIRTIO_SCSI_H
 
+#include linux/virtio_types.h
+
 #define VIRTIO_SCSI_CDB_SIZE   32
 #define VIRTIO_SCSI_SENSE_SIZE 96
 
 /* SCSI command request, followed by data-out */
 struct virtio_scsi_cmd_req {
u8 lun[8];  /* Logical Unit Number */
-   u64 tag;/* Command identifier */
+   __virtio64 tag; /* Command identifier */
u8 task_attr;   /* Task attribute */
u8 prio;/* SAM command priority field */
u8 crn;
@@ -43,20 +45,20 @@ struct virtio_scsi_cmd_req {
 /* SCSI command request, followed by protection information */
 struct virtio_scsi_cmd_req_pi {
u8 lun[8];  /* Logical Unit Number */
-   u64 tag;/* Command identifier */
+   __virtio64 tag; /* Command identifier */
u8 task_attr;   /* Task attribute */
u8 prio;/* SAM command priority field */
u8 crn;
-   u32 pi_bytesout;/* DataOUT PI Number of bytes */
-   u32 pi_bytesin; /* DataIN PI Number of bytes */
+   __virtio32 pi_bytesout; /* DataOUT PI Number of bytes */
+   __virtio32 pi_bytesin;  /* DataIN PI Number of bytes */
u8 cdb[VIRTIO_SCSI_CDB_SIZE];
 } __packed;
 
 /* Response, followed by sense data and data-in */
 struct virtio_scsi_cmd_resp {
-   u32 sense_len;  /* Sense data length */
-   u32 resid;  /* Residual bytes in data buffer */
-   u16 status_qualifier;   /* Status qualifier */
+   __virtio32 sense_len;   /* Sense data length */
+   __virtio32 resid;   /* Residual bytes in data buffer */
+   __virtio16 status_qualifier;/* Status qualifier */
u8 status;  /* Command completion status */
u8 response;/* Response values */
u8 sense[VIRTIO_SCSI_SENSE_SIZE];
@@ -64,10 +66,10 @@ struct virtio_scsi_cmd_resp {
 
 /* Task Management Request */
 struct virtio_scsi_ctrl_tmf_req {
-   u32 type;
-   u32 subtype;
+   __virtio32 type;
+   __virtio32 subtype;
u8 lun[8];
-   u64 tag;
+   __virtio64 tag;
 } __packed;
 
 struct virtio_scsi_ctrl_tmf_resp {
@@ -76,20 +78,20 @@ struct virtio_scsi_ctrl_tmf_resp {
 
 /* Asynchronous notification query/subscription */
 struct virtio_scsi_ctrl_an_req {
-   u32 type;
+   __virtio32 type;
u8 lun[8];
-   u32 event_requested;
+   __virtio32 event_requested;
 } __packed;
 
 struct virtio_scsi_ctrl_an_resp {
-   u32 event_actual;
+   __virtio32 event_actual;
u8 response;
 } __packed;
 
 struct virtio_scsi_event {
-   u32 event;
+   __virtio32 event;
u8 lun[8];
-   u32 reason;
+   __virtio32 reason;
 } __packed;
 
 struct virtio_scsi_config {
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index b83846f..d9ec806 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -158,7 +158,7 @@ static void virtscsi_complete_cmd(struct virtio_scsi 
*vscsi, void *buf)
sc, resp-response, resp-status, resp-sense_len);
 
sc-result = resp-status;
-   virtscsi_compute_resid(sc, resp-resid);
+   virtscsi_compute_resid(sc, virtio32_to_cpu(vscsi-vdev, resp-resid));
switch (resp-response) {
case VIRTIO_SCSI_S_OK:
set_host_byte(sc, DID_OK);
@@ -196,10 +196,13 @@ static void virtscsi_complete_cmd(struct virtio_scsi 
*vscsi, void *buf)
break;
}
 
-   WARN_ON(resp-sense_len  VIRTIO_SCSI_SENSE_SIZE);
+   WARN_ON(virtio32_to_cpu(vscsi-vdev, resp-sense_len) 
+   VIRTIO_SCSI_SENSE_SIZE);
if (sc-sense_buffer) {
memcpy(sc-sense_buffer, resp-sense,
-  min_t(u32, resp-sense_len, VIRTIO_SCSI_SENSE_SIZE));
+  min_t(u32,
+virtio32_to_cpu(vscsi-vdev, resp-sense_len),
+VIRTIO_SCSI_SENSE_SIZE));
if (resp-sense_len)
set_driver_byte(sc, DRIVER_SENSE);
}
@@ -323,7 +326,7 @@ static void virtscsi_handle_transport_reset(struct 
virtio_scsi *vscsi,
unsigned int target = event-lun[1];
unsigned int lun = (event-lun[2]  8) | event-lun[3];
 
-   switch (event

Re: [PATCH v7 42/46] virtio_scsi: v1.0 support

2014-12-01 Thread Michael S. Tsirkin 
On Mon, Dec 01, 2014 at 01:50:01PM +0100, Cornelia Huck wrote:
 On Sun, 30 Nov 2014 17:12:47 +0200
 Michael S. Tsirkin m...@redhat.com wrote:
 
  Note: for consistency, and to avoid sparse errors,
convert all fields, even those no longer in use
for virtio v1.0.
  
  Signed-off-by: Michael S. Tsirkin m...@redhat.com
  Acked-by: Paolo Bonzini pbonz...@redhat.com
  ---
   include/linux/virtio_scsi.h | 32 +++-
   drivers/scsi/virtio_scsi.c  | 51 
  -
   2 files changed, 49 insertions(+), 34 deletions(-)
  
 
  @@ -196,10 +196,13 @@ static void virtscsi_complete_cmd(struct virtio_scsi 
  *vscsi, void *buf)
  break;
  }
  
  -   WARN_ON(resp-sense_len  VIRTIO_SCSI_SENSE_SIZE);
  +   WARN_ON(virtio32_to_cpu(vscsi-vdev, resp-sense_len) 
  +   VIRTIO_SCSI_SENSE_SIZE);
 
 Introduce a local variable for this? Might make this statement and the
 min_t statement below easier to read.

I prefer 1:1 code conversions. We can do refactorings on top.
Since you mention this line as hard to read, I'll just make it  80
chars for now, and trivial line splits can come later.
OK?

  if (sc-sense_buffer) {
  memcpy(sc-sense_buffer, resp-sense,
  -  min_t(u32, resp-sense_len, VIRTIO_SCSI_SENSE_SIZE));
  +  min_t(u32,
  +virtio32_to_cpu(vscsi-vdev, resp-sense_len),
  +VIRTIO_SCSI_SENSE_SIZE));
  if (resp-sense_len)
  set_driver_byte(sc, DRIVER_SENSE);
  }
 
 Otherwise looks good to me.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 42/46] virtio_scsi: v1.0 support

2014-12-01 Thread Michael S. Tsirkin 
On Mon, Dec 01, 2014 at 02:53:05PM +0200, Michael S. Tsirkin wrote:
 On Mon, Dec 01, 2014 at 01:50:01PM +0100, Cornelia Huck wrote:
  On Sun, 30 Nov 2014 17:12:47 +0200
  Michael S. Tsirkin m...@redhat.com wrote:
  
   Note: for consistency, and to avoid sparse errors,
 convert all fields, even those no longer in use
 for virtio v1.0.
   
   Signed-off-by: Michael S. Tsirkin m...@redhat.com
   Acked-by: Paolo Bonzini pbonz...@redhat.com
   ---
include/linux/virtio_scsi.h | 32 +++-
drivers/scsi/virtio_scsi.c  | 51 
   -
2 files changed, 49 insertions(+), 34 deletions(-)
   
  
   @@ -196,10 +196,13 @@ static void virtscsi_complete_cmd(struct 
   virtio_scsi *vscsi, void *buf)
 break;
 }
   
   - WARN_ON(resp-sense_len  VIRTIO_SCSI_SENSE_SIZE);
   + WARN_ON(virtio32_to_cpu(vscsi-vdev, resp-sense_len) 
   + VIRTIO_SCSI_SENSE_SIZE);
  
  Introduce a local variable for this? Might make this statement and the
  min_t statement below easier to read.
 
 I prefer 1:1 code conversions. We can do refactorings on top.
 Since you mention this line as hard to read, I'll just make it  80
 chars for now, and trivial line splits can come later.
 OK?

Or maybe I'll keep it as is, since Paolo who wrote this code
is happy with it ..

 if (sc-sense_buffer) {
 memcpy(sc-sense_buffer, resp-sense,
   -min_t(u32, resp-sense_len, VIRTIO_SCSI_SENSE_SIZE));
   +min_t(u32,
   +  virtio32_to_cpu(vscsi-vdev, resp-sense_len),
   +  VIRTIO_SCSI_SENSE_SIZE));
 if (resp-sense_len)
 set_driver_byte(sc, DRIVER_SENSE);
 }
  
  Otherwise looks good to me.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v8 50/50] virtio: drop VIRTIO_F_VERSION_1 from drivers

2014-12-01 Thread Michael S. Tsirkin 
Core activates this bit automatically now,
drop it from drivers that set it explicitly.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
 drivers/block/virtio_blk.c| 1 -
 drivers/char/virtio_console.c | 1 -
 drivers/net/virtio_net.c  | 1 -
 drivers/scsi/virtio_scsi.c| 1 -
 4 files changed, 4 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 1f8b111..1fb9e09 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -836,7 +836,6 @@ static unsigned int features[] = {
VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
VIRTIO_BLK_F_TOPOLOGY,
VIRTIO_BLK_F_MQ,
-   VIRTIO_F_VERSION_1,
 };
 
 static struct virtio_driver virtio_blk = {
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 775c898..6cc832b 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -2130,7 +2130,6 @@ static struct virtio_device_id id_table[] = {
 static unsigned int features[] = {
VIRTIO_CONSOLE_F_SIZE,
VIRTIO_CONSOLE_F_MULTIPORT,
-   VIRTIO_F_VERSION_1,
 };
 
 static struct virtio_device_id rproc_serial_id_table[] = {
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 9ab3c50..b8bd719 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2004,7 +2004,6 @@ static unsigned int features[] = {
VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ,
VIRTIO_NET_F_CTRL_MAC_ADDR,
VIRTIO_F_ANY_LAYOUT,
-   VIRTIO_F_VERSION_1,
 };
 
 static struct virtio_driver virtio_net_driver = {
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index d9ec806..2308278 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -1085,7 +1085,6 @@ static unsigned int features[] = {
VIRTIO_SCSI_F_HOTPLUG,
VIRTIO_SCSI_F_CHANGE,
VIRTIO_SCSI_F_T10_PI,
-   VIRTIO_F_VERSION_1,
 };
 
 static struct virtio_driver virtio_scsi_driver = {
-- 
MST

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v8 42/50] virtio_scsi: v1.0 support

2014-12-01 Thread Michael S. Tsirkin 
Note: for consistency, and to avoid sparse errors,
  convert all fields, even those no longer in use
  for virtio v1.0.

Signed-off-by: Michael S. Tsirkin m...@redhat.com
Acked-by: Paolo Bonzini pbonz...@redhat.com
---
 include/linux/virtio_scsi.h | 32 +++-
 drivers/scsi/virtio_scsi.c  | 51 -
 2 files changed, 49 insertions(+), 34 deletions(-)

diff --git a/include/linux/virtio_scsi.h b/include/linux/virtio_scsi.h
index de429d1..af44864 100644
--- a/include/linux/virtio_scsi.h
+++ b/include/linux/virtio_scsi.h
@@ -27,13 +27,15 @@
 #ifndef _LINUX_VIRTIO_SCSI_H
 #define _LINUX_VIRTIO_SCSI_H
 
+#include linux/virtio_types.h
+
 #define VIRTIO_SCSI_CDB_SIZE   32
 #define VIRTIO_SCSI_SENSE_SIZE 96
 
 /* SCSI command request, followed by data-out */
 struct virtio_scsi_cmd_req {
u8 lun[8];  /* Logical Unit Number */
-   u64 tag;/* Command identifier */
+   __virtio64 tag; /* Command identifier */
u8 task_attr;   /* Task attribute */
u8 prio;/* SAM command priority field */
u8 crn;
@@ -43,20 +45,20 @@ struct virtio_scsi_cmd_req {
 /* SCSI command request, followed by protection information */
 struct virtio_scsi_cmd_req_pi {
u8 lun[8];  /* Logical Unit Number */
-   u64 tag;/* Command identifier */
+   __virtio64 tag; /* Command identifier */
u8 task_attr;   /* Task attribute */
u8 prio;/* SAM command priority field */
u8 crn;
-   u32 pi_bytesout;/* DataOUT PI Number of bytes */
-   u32 pi_bytesin; /* DataIN PI Number of bytes */
+   __virtio32 pi_bytesout; /* DataOUT PI Number of bytes */
+   __virtio32 pi_bytesin;  /* DataIN PI Number of bytes */
u8 cdb[VIRTIO_SCSI_CDB_SIZE];
 } __packed;
 
 /* Response, followed by sense data and data-in */
 struct virtio_scsi_cmd_resp {
-   u32 sense_len;  /* Sense data length */
-   u32 resid;  /* Residual bytes in data buffer */
-   u16 status_qualifier;   /* Status qualifier */
+   __virtio32 sense_len;   /* Sense data length */
+   __virtio32 resid;   /* Residual bytes in data buffer */
+   __virtio16 status_qualifier;/* Status qualifier */
u8 status;  /* Command completion status */
u8 response;/* Response values */
u8 sense[VIRTIO_SCSI_SENSE_SIZE];
@@ -64,10 +66,10 @@ struct virtio_scsi_cmd_resp {
 
 /* Task Management Request */
 struct virtio_scsi_ctrl_tmf_req {
-   u32 type;
-   u32 subtype;
+   __virtio32 type;
+   __virtio32 subtype;
u8 lun[8];
-   u64 tag;
+   __virtio64 tag;
 } __packed;
 
 struct virtio_scsi_ctrl_tmf_resp {
@@ -76,20 +78,20 @@ struct virtio_scsi_ctrl_tmf_resp {
 
 /* Asynchronous notification query/subscription */
 struct virtio_scsi_ctrl_an_req {
-   u32 type;
+   __virtio32 type;
u8 lun[8];
-   u32 event_requested;
+   __virtio32 event_requested;
 } __packed;
 
 struct virtio_scsi_ctrl_an_resp {
-   u32 event_actual;
+   __virtio32 event_actual;
u8 response;
 } __packed;
 
 struct virtio_scsi_event {
-   u32 event;
+   __virtio32 event;
u8 lun[8];
-   u32 reason;
+   __virtio32 reason;
 } __packed;
 
 struct virtio_scsi_config {
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index b83846f..d9ec806 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -158,7 +158,7 @@ static void virtscsi_complete_cmd(struct virtio_scsi 
*vscsi, void *buf)
sc, resp-response, resp-status, resp-sense_len);
 
sc-result = resp-status;
-   virtscsi_compute_resid(sc, resp-resid);
+   virtscsi_compute_resid(sc, virtio32_to_cpu(vscsi-vdev, resp-resid));
switch (resp-response) {
case VIRTIO_SCSI_S_OK:
set_host_byte(sc, DID_OK);
@@ -196,10 +196,13 @@ static void virtscsi_complete_cmd(struct virtio_scsi 
*vscsi, void *buf)
break;
}
 
-   WARN_ON(resp-sense_len  VIRTIO_SCSI_SENSE_SIZE);
+   WARN_ON(virtio32_to_cpu(vscsi-vdev, resp-sense_len) 
+   VIRTIO_SCSI_SENSE_SIZE);
if (sc-sense_buffer) {
memcpy(sc-sense_buffer, resp-sense,
-  min_t(u32, resp-sense_len, VIRTIO_SCSI_SENSE_SIZE));
+  min_t(u32,
+virtio32_to_cpu(vscsi-vdev, resp-sense_len),
+VIRTIO_SCSI_SENSE_SIZE));
if (resp-sense_len)
set_driver_byte(sc, DRIVER_SENSE);
}
@@ -323,7 +326,7 @@ static void virtscsi_handle_transport_reset(struct 
virtio_scsi *vscsi,
unsigned int target = event-lun[1];
unsigned int lun = (event-lun[2]  8) | event-lun[3];
 
-   switch (event

Re: [PATCH 4/4] tcm_vhost: Convert I/O path to use target_submit_cmd_map_mem

2012-10-02 Thread Michael S. Tsirkin 
On Tue, Oct 02, 2012 at 07:15:47AM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 This patch converts tcm_vhost to use target_submit_cmd_map_mem() for
 I/O submission and mapping of pre-allocated SGL memory from incoming
 virtio-scsi SGL memory - se_cmd descriptors.
 
 This includes removing the original open-coded fabric uses of target
 core callers to support transport_generic_map_mem_to_cmd() between
 target_setup_cmd_from_cdb() and transport_handle_cdb_direct() logic.
 
 It also includes adding a handful of new tcm_vhost_cmnd member +
 assignments in vhost_scsi_allocate_cmd() used from cmwq process
 context I/O submission within tcm_vhost_submission_work()
 
 Reported-by: Christoph Hellwig h...@lst.de
 Cc: Christoph Hellwig h...@lst.de
 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Paolo Bonzini pbonz...@redhat.com
 Cc: Stefan Hajnoczi stefa...@gmail.com
 Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
 ---
  drivers/vhost/tcm_vhost.c |   68 +---
  drivers/vhost/tcm_vhost.h |8 +
  2 files changed, 22 insertions(+), 54 deletions(-)
 
 diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
 index 89dc99b..2ca5f02 100644
 --- a/drivers/vhost/tcm_vhost.c
 +++ b/drivers/vhost/tcm_vhost.c
 @@ -415,10 +415,7 @@ static struct tcm_vhost_cmd *vhost_scsi_allocate_cmd(
  {
   struct tcm_vhost_cmd *tv_cmd;
   struct tcm_vhost_nexus *tv_nexus;
 - struct se_portal_group *se_tpg = tv_tpg-se_tpg;
   struct se_session *se_sess;
 - struct se_cmd *se_cmd;
 - int sam_task_attr;
  
   tv_nexus = tv_tpg-tpg_nexus;
   if (!tv_nexus) {
 @@ -434,23 +431,11 @@ static struct tcm_vhost_cmd *vhost_scsi_allocate_cmd(
   }
   INIT_LIST_HEAD(tv_cmd-tvc_completion_list);
   tv_cmd-tvc_tag = v_req-tag;
 + tv_cmd-tvc_task_attr = v_req-task_attr;
 + tv_cmd-tvc_exp_data_len = exp_data_len;
 + tv_cmd-tvc_data_direction = data_direction;
 + tv_cmd-tvc_nexus = tv_nexus;
  
 - se_cmd = tv_cmd-tvc_se_cmd;
 - /*
 -  * Locate the SAM Task Attr from virtio_scsi_cmd_req
 -  */
 - sam_task_attr = v_req-task_attr;
 - /*
 -  * Initialize struct se_cmd descriptor from TCM infrastructure
 -  */
 - transport_init_se_cmd(se_cmd, se_tpg-se_tpg_tfo, se_sess, exp_data_len,
 - data_direction, sam_task_attr,
 - tv_cmd-tvc_sense_buf[0]);
 -
 -#if 0/* FIXME: vhost_scsi_allocate_cmd() BIDI operation */
 - if (bidi)
 - se_cmd-se_cmd_flags |= SCF_BIDI;
 -#endif
   return tv_cmd;
  }
  
 @@ -549,37 +534,10 @@ static void tcm_vhost_submission_work(struct 
 work_struct *work)
  {
   struct tcm_vhost_cmd *tv_cmd =
   container_of(work, struct tcm_vhost_cmd, work);
 + struct tcm_vhost_nexus *tv_nexus;
   struct se_cmd *se_cmd = tv_cmd-tvc_se_cmd;
   struct scatterlist *sg_ptr, *sg_bidi_ptr = NULL;
   int rc, sg_no_bidi = 0;
 - /*
 -  * Locate the struct se_lun pointer based on v_req-lun, and
 -  * attach it to struct se_cmd
 -  */
 - rc = transport_lookup_cmd_lun(tv_cmd-tvc_se_cmd, tv_cmd-tvc_lun);
 - if (rc  0) {
 - pr_err(Failed to look up lun: %d\n, tv_cmd-tvc_lun);
 - transport_send_check_condition_and_sense(tv_cmd-tvc_se_cmd,
 - tv_cmd-tvc_se_cmd.scsi_sense_reason, 0);
 - transport_generic_free_cmd(se_cmd, 0);
 - return;
 - }
 -

IIUC tv_cmd can come from userspace so calling pr_error here
was a bug, it's good that it's fixed now.
And looking at target_submit_cmd_map_mem, it looks that
at least lun lookup error no longer triggers pr_error, right?
If yes it's good.

 - rc = target_setup_cmd_from_cdb(se_cmd, tv_cmd-tvc_cdb);
 - if (rc == -ENOMEM) {
 - transport_send_check_condition_and_sense(se_cmd,
 - TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE, 0);
 - transport_generic_free_cmd(se_cmd, 0);
 - return;
 - } else if (rc  0) {
 - if (se_cmd-se_cmd_flags  SCF_SCSI_RESERVATION_CONFLICT)
 - tcm_vhost_queue_status(se_cmd);
 - else
 - transport_send_check_condition_and_sense(se_cmd,
 - se_cmd-scsi_sense_reason, 0);
 - transport_generic_free_cmd(se_cmd, 0);
 - return;
 - }
  
   if (tv_cmd-tvc_sgl_count) {
   sg_ptr = tv_cmd-tvc_sgl;
 @@ -597,17 +555,19 @@ static void tcm_vhost_submission_work(struct 
 work_struct *work)
   } else {
   sg_ptr = NULL;
   }
 -
 - rc = transport_generic_map_mem_to_cmd(se_cmd, sg_ptr,
 - tv_cmd-tvc_sgl_count, sg_bidi_ptr,
 - sg_no_bidi);
 + tv_nexus = tv_cmd-tvc_nexus;
 +
 + rc = target_submit_cmd_map_mem(se_cmd, tv_nexus-tvn_se_sess

Re: [PATCH 0/4] target: Add target_submit_cmd_map_mem and convert tcm_loop+tcm_vhost

2012-10-02 Thread Michael S. Tsirkin 
On Tue, Oct 02, 2012 at 07:15:43AM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 Hi hch  Co,
 
 This series adds a new target_submit_cmd_map_mem() caller to accept
 pre-allocated SGL memory within the core generic I/O submission path.
 Patch #1 contains the core I/O changes, patch #2 + #4 includes the
 conversion of tcm_loop+tcm_vhost to use this new caller - drop their
 internal open-coded equivalents using transport_generic_map_mem_to_cmd().
 
 Patch #3 carries forward a work-around for tcm_loop w/ scsi-generic with
 user-space code that does not zero out it's READ payload buffer + ends
 up passing a payload filled with random data into target core's control CDB
 emulation.  Since we're not using bounce buffers any more for control
 CDB emulation in modern v3.x code, AFAICT tcm_loop still requires this
 extra bit to function properly with some legacy user-space code.
 
 Regardless, the main I/O path changes end up being very mechnical in
 nature for existing core and fabric code, and has been running as expected
 with fio small-block workloads last hours.
 
 Please review.
 
 Thanks Christoph!
 
 --nab

For the vhost part:

ACK series.

 Nicholas Bellinger (4):
   target: Add target_submit_cmd_map_mem for SGL fabric memory
 passthrough
   tcm_loop: Convert I/O path to use target_submit_cmd_map_mem
   target: Add TARGET_SCF_MAP_CLEAR_MEM work-around for tcm_loop
   tcm_vhost: Convert I/O path to use target_submit_cmd_map_mem
 
  drivers/target/loopback/tcm_loop.c |   62 ++-
  drivers/target/target_core_transport.c |  129 
 ++--
  drivers/vhost/tcm_vhost.c  |   68 -
  drivers/vhost/tcm_vhost.h  |8 ++
  include/target/target_core_base.h  |2 +
  include/target/target_core_fabric.h|3 +
  6 files changed, 141 insertions(+), 131 deletions(-)
 
 -- 
 1.7.2.5
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] virtio: add functions for piecewise addition of buffers

2012-12-18 Thread Michael S. Tsirkin 
Some comments without arguing about whether the performance
benefit is worth it.

On Tue, Dec 18, 2012 at 01:32:48PM +0100, Paolo Bonzini wrote:
 diff --git a/include/linux/virtio.h b/include/linux/virtio.h
 index cf8adb1..39d56c4 100644
 --- a/include/linux/virtio.h
 +++ b/include/linux/virtio.h
 @@ -7,6 +7,7 @@
  #include linux/spinlock.h
  #include linux/device.h
  #include linux/mod_devicetable.h
 +#include linux/dma-direction.h
  #include linux/gfp.h
  
  /**
 @@ -40,6 +41,26 @@ int virtqueue_add_buf(struct virtqueue *vq,
 void *data,
 gfp_t gfp);
  
 +struct virtqueue_buf {
 + struct virtqueue *vq;
 + struct vring_desc *indirect, *tail;

This is wrong: virtio.h does not include virito_ring.h,
and it shouldn't by design depend on it.

 + int head;
 +};
 +

Can't we track state internally to the virtqueue?
Exposing it seems to buy us nothing since you can't
call add_buf between start and end anyway.


 +int virtqueue_start_buf(struct virtqueue *_vq,
 + struct virtqueue_buf *buf,
 + void *data,
 + unsigned int count,
 + unsigned int count_sg,
 + gfp_t gfp);
 +
 +void virtqueue_add_sg(struct virtqueue_buf *buf,
 +   struct scatterlist sgl[],
 +   unsigned int count,
 +   enum dma_data_direction dir);
 +

And idea: in practice virtio scsi seems to always call sg_init_one, no?
So how about we pass in void* or something and avoid using sg and count?
This would make it useful for -net BTW.

 +void virtqueue_end_buf(struct virtqueue_buf *buf);
 +
  void virtqueue_kick(struct virtqueue *vq);
  
  bool virtqueue_kick_prepare(struct virtqueue *vq);
 -- 
 1.7.1
 
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/5] virtio-scsi: use functions for piecewise composition of buffers

2012-12-18 Thread Michael S. Tsirkin 
On Tue, Dec 18, 2012 at 01:32:49PM +0100, Paolo Bonzini wrote:
 Using the new virtio_scsi_add_sg function lets us simplify the queueing
 path.  In particular, all data protected by the tgt_lock is just gone
 (multiqueue will find a new use for the lock).

vq access still needs some protection: virtio is not reentrant
by itself. with tgt_lock gone what protects vq against
concurrent add_buf calls?

 The speedup is relatively small (2-4%) but it is worthwhile because of
 the code simplification---both in this patches and in the next ones.
 
 Signed-off-by: Wanlong Gao gaowanl...@cn.fujitsu.com
 Signed-off-by: Paolo Bonzini pbonz...@redhat.com
 ---
   v1-v2: new
 
  drivers/scsi/virtio_scsi.c |   94 +++
  1 files changed, 42 insertions(+), 52 deletions(-)
 
 diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
 index 74ab67a..2b93b6e 100644
 --- a/drivers/scsi/virtio_scsi.c
 +++ b/drivers/scsi/virtio_scsi.c
 @@ -59,11 +59,8 @@ struct virtio_scsi_vq {
  
  /* Per-target queue state */
  struct virtio_scsi_target_state {
 - /* Protects sg.  Lock hierarchy is tgt_lock - vq_lock.  */
 + /* Never held at the same time as vq_lock.  */
   spinlock_t tgt_lock;
 -
 - /* For sglist construction when adding commands to the virtqueue.  */
 - struct scatterlist sg[];
  };
  
  /* Driver instance state */
 @@ -351,57 +348,58 @@ static void virtscsi_event_done(struct virtqueue *vq)
   spin_unlock_irqrestore(vscsi-event_vq.vq_lock, flags);
  };
  
 -static void virtscsi_map_sgl(struct scatterlist *sg, unsigned int *p_idx,
 -  struct scsi_data_buffer *sdb)
 -{
 - struct sg_table *table = sdb-table;
 - struct scatterlist *sg_elem;
 - unsigned int idx = *p_idx;
 - int i;
 -
 - for_each_sg(table-sgl, sg_elem, table-nents, i)
 - sg[idx++] = *sg_elem;
 -
 - *p_idx = idx;
 -}
 -
  /**
 - * virtscsi_map_cmd - map a scsi_cmd to a virtqueue scatterlist
 + * virtscsi_add_cmd - add a virtio_scsi_cmd to a virtqueue
   * @vscsi: virtio_scsi state
   * @cmd  : command structure
 - * @out_num  : number of read-only elements
 - * @in_num   : number of write-only elements
   * @req_size : size of the request buffer
   * @resp_size: size of the response buffer
 - *
 - * Called with tgt_lock held.
 + * @gfp  : flags to use for memory allocations
   */
 -static void virtscsi_map_cmd(struct virtio_scsi_target_state *tgt,
 -  struct virtio_scsi_cmd *cmd,
 -  unsigned *out_num, unsigned *in_num,
 -  size_t req_size, size_t resp_size)
 +static int virtscsi_add_cmd(struct virtqueue *vq,
 + struct virtio_scsi_cmd *cmd,
 + size_t req_size, size_t resp_size, gfp_t gfp)
  {
   struct scsi_cmnd *sc = cmd-sc;
 - struct scatterlist *sg = tgt-sg;
 - unsigned int idx = 0;
 + struct scatterlist sg;
 + unsigned int count, count_sg;
 + struct sg_table *out, *in;
 + struct virtqueue_buf buf;
 + int ret;
 +
 + out = in = NULL;
 +
 + if (sc  sc-sc_data_direction != DMA_NONE) {
 + if (sc-sc_data_direction != DMA_FROM_DEVICE)
 + out = scsi_out(sc)-table;
 + if (sc-sc_data_direction != DMA_TO_DEVICE)
 + in = scsi_in(sc)-table;
 + }
 +
 + count_sg = 2 + (out ? 1 : 0)  + (in ? 1 : 0);
 + count= 2 + (out ? out-nents : 0) + (in ? in-nents : 0);
 + ret = virtqueue_start_buf(vq, buf, cmd, count, count_sg, gfp);
 + if (ret  0)
 + return ret;
  
   /* Request header.  */
 - sg_set_buf(sg[idx++], cmd-req, req_size);
 + sg_init_one(sg, cmd-req, req_size);
 + virtqueue_add_sg(buf, sg, 1, DMA_TO_DEVICE);
  
   /* Data-out buffer.  */
 - if (sc  sc-sc_data_direction != DMA_FROM_DEVICE)
 - virtscsi_map_sgl(sg, idx, scsi_out(sc));
 -
 - *out_num = idx;
 + if (out)
 + virtqueue_add_sg(buf, out-sgl, out-nents, DMA_TO_DEVICE);
  
   /* Response header.  */
 - sg_set_buf(sg[idx++], cmd-resp, resp_size);
 + sg_init_one(sg, cmd-resp, resp_size);
 + virtqueue_add_sg(buf, sg, 1, DMA_FROM_DEVICE);
  
   /* Data-in buffer */
 - if (sc  sc-sc_data_direction != DMA_TO_DEVICE)
 - virtscsi_map_sgl(sg, idx, scsi_in(sc));
 + if (in)
 + virtqueue_add_sg(buf, in-sgl, in-nents, DMA_FROM_DEVICE);
  
 - *in_num = idx - *out_num;
 + virtqueue_end_buf(buf);
 + return 0;
  }
  
  static int virtscsi_kick_cmd(struct virtio_scsi_target_state *tgt,
 @@ -409,25 +407,20 @@ static int virtscsi_kick_cmd(struct 
 virtio_scsi_target_state *tgt,
struct virtio_scsi_cmd *cmd,
size_t req_size, size_t resp_size, gfp_t gfp)
  {
 - unsigned int out_num, in_num;
   unsigned long flags;
 - int err;
 +

Re: [PATCH v2 0/5] Multiqueue virtio-scsi, and API for piecewise buffer submission

2012-12-18 Thread Michael S. Tsirkin 
On Tue, Dec 18, 2012 at 01:32:47PM +0100, Paolo Bonzini wrote:
 Hi all,
 
 this series adds multiqueue support to the virtio-scsi driver, based
 on Jason Wang's work on virtio-net.  It uses a simple queue steering
 algorithm that expects one queue per CPU.  LUNs in the same target always
 use the same queue (so that commands are not reordered); queue switching
 occurs when the request being queued is the only one for the target.
 Also based on Jason's patches, the virtqueue affinity is set so that
 each CPU is associated to one virtqueue.
 
 I tested the patches with fio, using up to 32 virtio-scsi disks backed
 by tmpfs on the host.  These numbers are with 1 LUN per target.
 
 FIO configuration
 -
 [global]
 rw=read
 bsrange=4k-64k
 ioengine=libaio
 direct=1
 iodepth=4
 loops=20
 
 overall bandwidth (MB/s)
 
 
 # of targetssingle-queuemulti-queue, 4 VCPUsmulti-queue, 8 VCPUs
 1  540   626 599
 2  795   965 925
 4  997  13761500
 8 1136  21302060
 161440  22692474
 241408  21792436
 321515  19782319
 
 (These numbers for single-queue are with 4 VCPUs, but the impact of adding
 more VCPUs is very limited).
 
 avg bandwidth per LUN (MB/s)
 
 
 # of targetssingle-queuemulti-queue, 4 VCPUsmulti-queue, 8 VCPUs
 1  540   626 599
 2  397   482 462
 4  249   344 375
 8  142   266 257
 16  90   141 154
 24  5890 101
 32  4761  72


Could you please try and measure host CPU utilization?
Without this data it is possible that your host
is undersubscribed and you are drinking up more host CPU.

Another thing to note is that ATM you might need to
test with idle=poll on host otherwise we have strange interaction
with power management where reducing the overhead
switches to lower power so gives you a worse IOPS.


 Patch 1 adds a new API to add functions for piecewise addition for buffers,
 which enables various simplifications in virtio-scsi (patches 2-3) and a
 small performance improvement of 2-6%.  Patches 4 and 5 add multiqueuing.
 
 I'm mostly looking for comments on the new API of patch 1 for inclusion
 into the 3.9 kernel.
 
 Thanks to Wao Ganlong for help rebasing and benchmarking these patches.
 
 Paolo Bonzini (5):
   virtio: add functions for piecewise addition of buffers
   virtio-scsi: use functions for piecewise composition of buffers
   virtio-scsi: redo allocation of target data
   virtio-scsi: pass struct virtio_scsi to virtqueue completion function
   virtio-scsi: introduce multiqueue support
 
  drivers/scsi/virtio_scsi.c   |  374 
 +-
  drivers/virtio/virtio_ring.c |  205 
  include/linux/virtio.h   |   21 +++
  3 files changed, 485 insertions(+), 115 deletions(-)
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] virtio-scsi: introduce multiqueue support

2012-12-18 Thread Michael S. Tsirkin 
On Tue, Dec 18, 2012 at 01:32:52PM +0100, Paolo Bonzini wrote:
 This patch adds queue steering to virtio-scsi.  When a target is sent
 multiple requests, we always drive them to the same queue so that FIFO
 processing order is kept.  However, if a target was idle, we can choose
 a queue arbitrarily.  In this case the queue is chosen according to the
 current VCPU, so the driver expects the number of request queues to be
 equal to the number of VCPUs.  This makes it easy and fast to select
 the queue, and also lets the driver optimize the IRQ affinity for the
 virtqueues (each virtqueue's affinity is set to the CPU that owns
 the queue).
 
 The speedup comes from improving cache locality and giving CPU affinity
 to the virtqueues, which is why this scheme was selected.  Assuming that
 the thread that is sending requests to the device is I/O-bound, it is
 likely to be sleeping at the time the ISR is executed, and thus executing
 the ISR on the same processor that sent the requests is cheap.
 
 However, the kernel will not execute the ISR on the best processor
 unless you explicitly set the affinity.  This is because in practice
 you will have many such I/O-bound processes and thus many otherwise
 idle processors.  Then the kernel will execute the ISR on a random
 processor, rather than the one that is sending requests to the device.
 
 The alternative to per-CPU virtqueues is per-target virtqueues.  To
 achieve the same locality, we could dynamically choose the virtqueue's
 affinity based on the CPU of the last task that sent a request.  This
 is less appealing because we do not set the affinity directly---we only
 provide a hint to the irqbalanced running in userspace.  Dynamically
 changing the affinity only works if the userspace applies the hint
 fast enough.
 
 Signed-off-by: Paolo Bonzini pbonz...@redhat.com
 ---
   v1-v2: improved comments and commit messages, added memory barriers
 
  drivers/scsi/virtio_scsi.c |  234 +--
  1 files changed, 201 insertions(+), 33 deletions(-)
 
 diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
 index 4f6c6a3..ca9d29d 100644
 --- a/drivers/scsi/virtio_scsi.c
 +++ b/drivers/scsi/virtio_scsi.c
 @@ -26,6 +26,7 @@
  
  #define VIRTIO_SCSI_MEMPOOL_SZ 64
  #define VIRTIO_SCSI_EVENT_LEN 8
 +#define VIRTIO_SCSI_VQ_BASE 2
  
  /* Command queue element */
  struct virtio_scsi_cmd {
 @@ -57,24 +58,57 @@ struct virtio_scsi_vq {
   struct virtqueue *vq;
  };
  
 -/* Per-target queue state */
 +/*
 + * Per-target queue state.
 + *
 + * This struct holds the data needed by the queue steering policy.  When a
 + * target is sent multiple requests, we need to drive them to the same queue 
 so
 + * that FIFO processing order is kept.  However, if a target was idle, we can
 + * choose a queue arbitrarily.  In this case the queue is chosen according to
 + * the current VCPU, so the driver expects the number of request queues to be
 + * equal to the number of VCPUs.  This makes it easy and fast to select the
 + * queue, and also lets the driver optimize the IRQ affinity for the 
 virtqueues
 + * (each virtqueue's affinity is set to the CPU that owns the queue).
 + *
 + * An interesting effect of this policy is that only writes to req_vq need to
 + * take the tgt_lock.  Read can be done outside the lock because:
 + *
 + * - writes of req_vq only occur when atomic_inc_return(tgt-reqs) returns 
 1.
 + *   In that case, no other CPU is reading req_vq: even if they were in
 + *   virtscsi_queuecommand_multi, they would be spinning on tgt_lock.
 + *
 + * - reads of req_vq only occur when the target is not idle (reqs != 0).
 + *   A CPU that enters virtscsi_queuecommand_multi will not modify req_vq.
 + *
 + * Similarly, decrements of reqs are never concurrent with writes of req_vq.
 + * Thus they can happen outside the tgt_lock, provided of course we make reqs
 + * an atomic_t.
 + */
  struct virtio_scsi_target_state {
 - /* Never held at the same time as vq_lock.  */
 + /* This spinlock ever held at the same time as vq_lock.  */
   spinlock_t tgt_lock;
 +
 + /* Count of outstanding requests.  */
 + atomic_t reqs;
 +
 + /* Currently active virtqueue for requests sent to this target.  */
 + struct virtio_scsi_vq *req_vq;
  };
  
  /* Driver instance state */
  struct virtio_scsi {
   struct virtio_device *vdev;
  
 - struct virtio_scsi_vq ctrl_vq;
 - struct virtio_scsi_vq event_vq;
 - struct virtio_scsi_vq req_vq;
 -
   /* Get some buffers ready for event vq */
   struct virtio_scsi_event_node event_list[VIRTIO_SCSI_EVENT_LEN];
  
   struct virtio_scsi_target_state *tgt;
 +
 + u32 num_queues;
 +
 + struct virtio_scsi_vq ctrl_vq;
 + struct virtio_scsi_vq event_vq;
 + struct virtio_scsi_vq req_vqs[];
  };
  
  static struct kmem_cache *virtscsi_cmd_cache;
 @@ -109,6 +143,7 @@ static void virtscsi_complete_cmd(struct virtio_scsi 
 *vscsi, void *buf)
   struct

Re: [PATCH v2 1/5] virtio: add functions for piecewise addition of buffers

2012-12-18 Thread Michael S. Tsirkin 
On Tue, Dec 18, 2012 at 02:43:51PM +0100, Paolo Bonzini wrote:
 Il 18/12/2012 14:36, Michael S. Tsirkin ha scritto:
  Some comments without arguing about whether the performance
  benefit is worth it.
  
  On Tue, Dec 18, 2012 at 01:32:48PM +0100, Paolo Bonzini wrote:
  diff --git a/include/linux/virtio.h b/include/linux/virtio.h
  index cf8adb1..39d56c4 100644
  --- a/include/linux/virtio.h
  +++ b/include/linux/virtio.h
  @@ -7,6 +7,7 @@
   #include linux/spinlock.h
   #include linux/device.h
   #include linux/mod_devicetable.h
  +#include linux/dma-direction.h
   #include linux/gfp.h
   
   /**
  @@ -40,6 +41,26 @@ int virtqueue_add_buf(struct virtqueue *vq,
   void *data,
   gfp_t gfp);
   
  +struct virtqueue_buf {
  +  struct virtqueue *vq;
  +  struct vring_desc *indirect, *tail;
  
  This is wrong: virtio.h does not include virito_ring.h,
  and it shouldn't by design depend on it.
  
  +  int head;
  +};
  +
  
  Can't we track state internally to the virtqueue?
  Exposing it seems to buy us nothing since you can't
  call add_buf between start and end anyway.
 
 I wanted to keep the state for these functions separate from the rest.
 I don't think it makes much sense to move it to struct virtqueue unless
 virtqueue_add_buf is converted to use the new API (doesn't make much
 sense, could even be a tad slower).

Why would it be slower?

 On the other hand moving it there would eliminate the dependency on
 virtio_ring.h.  Rusty, what do you think?
 
  +int virtqueue_start_buf(struct virtqueue *_vq,
  +  struct virtqueue_buf *buf,
  +  void *data,
  +  unsigned int count,
  +  unsigned int count_sg,
  +  gfp_t gfp);
  +
  +void virtqueue_add_sg(struct virtqueue_buf *buf,
  +struct scatterlist sgl[],
  +unsigned int count,
  +enum dma_data_direction dir);
  +
  
  And idea: in practice virtio scsi seems to always call sg_init_one, no?
  So how about we pass in void* or something and avoid using sg and count?
  This would make it useful for -net BTW.
 
 It also passes the scatterlist from the LLD.  It calls sg_init_one for
 the request/response headers.
 
 Paolo

Try adding a _single variant. You might see unrolling a loop
gives more of a benefit than this whole optimization.

  +void virtqueue_end_buf(struct virtqueue_buf *buf);
  +
   void virtqueue_kick(struct virtqueue *vq);
   
   bool virtqueue_kick_prepare(struct virtqueue *vq);
  -- 
  1.7.1
 
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] virtio-scsi: introduce multiqueue support

2012-12-18 Thread Michael S. Tsirkin 
On Tue, Dec 18, 2012 at 03:08:08PM +0100, Paolo Bonzini wrote:
 Il 18/12/2012 14:57, Michael S. Tsirkin ha scritto:
  -static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd 
  *sc)
  +static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
  +   struct virtio_scsi_target_state *tgt,
  +   struct scsi_cmnd *sc)
   {
  -  struct virtio_scsi *vscsi = shost_priv(sh);
  -  struct virtio_scsi_target_state *tgt = vscsi-tgt[sc-device-id];
 struct virtio_scsi_cmd *cmd;
  +  struct virtio_scsi_vq *req_vq;
 int ret;
   
 struct Scsi_Host *shost = virtio_scsi_host(vscsi-vdev);
  @@ -461,7 +533,8 @@ static int virtscsi_queuecommand(struct Scsi_Host *sh, 
  struct scsi_cmnd *sc)
 BUG_ON(sc-cmd_len  VIRTIO_SCSI_CDB_SIZE);
 memcpy(cmd-req.cmd.cdb, sc-cmnd, sc-cmd_len);
   
  -  if (virtscsi_kick_cmd(tgt, vscsi-req_vq, cmd,
  +  req_vq = ACCESS_ONCE(tgt-req_vq);
  
  This ACCESS_ONCE without a barrier looks strange to me.
  Can req_vq change? Needs a comment.
 
 Barriers are needed to order two things.  Here I don't have the second thing
 to order against, hence no barrier.
 
 Accessing req_vq lockless is safe, and there's a comment about it, but you
 still want ACCESS_ONCE to ensure the compiler doesn't play tricks.

That's just it.
Why don't you want compiler to play tricks?

ACCESS_ONCE is needed if the value can change
while you access it, this helps ensure
a consistent value is evalutated.

If it can you almost always need a barrier. If it doesn't
you don't need ACCESS_ONCE.

  It
 shouldn't be necessary, because the critical section of
 virtscsi_queuecommand_multi will already include the appropriate
 compiler barriers,

So if there's a barrier then pls add a comment saying where
it is.

 but it is actually clearer this way to me. :)

No barriers are needed I think because
when you queue command req is incremented to req_vq
can not change. But this also means ACCESS_ONCE
is not needed either.

  +  if (virtscsi_kick_cmd(tgt, req_vq, cmd,
   sizeof cmd-req.cmd, sizeof cmd-resp.cmd,
   GFP_ATOMIC) == 0)
 ret = 0;
  @@ -472,6 +545,48 @@ out:
 return ret;
   }
   
  +static int virtscsi_queuecommand_single(struct Scsi_Host *sh,
  +  struct scsi_cmnd *sc)
  +{
  +  struct virtio_scsi *vscsi = shost_priv(sh);
  +  struct virtio_scsi_target_state *tgt = vscsi-tgt[sc-device-id];
  +
  +  atomic_inc(tgt-reqs);
  
  And here we don't have barrier after atomic? Why? Needs a comment.
 
 Because we don't write req_vq, so there's no two writes to order.  Barrier
 against what?

Between atomic update and command. Once you queue command it
can complete and decrement reqs, if this happens before
increment reqs can become negative even.

  +  return virtscsi_queuecommand(vscsi, tgt, sc);
  +}
  +
  +static int virtscsi_queuecommand_multi(struct Scsi_Host *sh,
  + struct scsi_cmnd *sc)
  +{
  +  struct virtio_scsi *vscsi = shost_priv(sh);
  +  struct virtio_scsi_target_state *tgt = vscsi-tgt[sc-device-id];
  +  unsigned long flags;
  +  u32 queue_num;
  +
  +  /*
  +   * Using an atomic_t for tgt-reqs lets the virtqueue handler
  +   * decrement it without taking the spinlock.
  +   *
  +   * We still need a critical section to prevent concurrent submissions
  +   * from picking two different req_vqs.
  +   */
  +  spin_lock_irqsave(tgt-tgt_lock, flags);
  +  if (atomic_inc_return(tgt-reqs) == 1) {
  +  queue_num = smp_processor_id();
  +  while (unlikely(queue_num = vscsi-num_queues))
  +  queue_num -= vscsi-num_queues;
  +
  +  /*
  +   * Write reqs before writing req_vq, matching the
  +   * smp_read_barrier_depends() in virtscsi_req_done.
  +   */
  +  smp_wmb();
  +  tgt-req_vq = vscsi-req_vqs[queue_num];
  +  }
  +  spin_unlock_irqrestore(tgt-tgt_lock, flags);
  +  return virtscsi_queuecommand(vscsi, tgt, sc);
  +}
  +
   static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd 
  *cmd)
   {
 DECLARE_COMPLETION_ONSTACK(comp);
  @@ -541,12 +656,26 @@ static int virtscsi_abort(struct scsi_cmnd *sc)
 return virtscsi_tmf(vscsi, cmd);
   }
   
  -static struct scsi_host_template virtscsi_host_template = {
  +static struct scsi_host_template virtscsi_host_template_single = {
 .module = THIS_MODULE,
 .name = Virtio SCSI HBA,
 .proc_name = virtio_scsi,
  -  .queuecommand = virtscsi_queuecommand,
 .this_id = -1,
  +  .queuecommand = virtscsi_queuecommand_single,
  +  .eh_abort_handler = virtscsi_abort,
  +  .eh_device_reset_handler = virtscsi_device_reset,
  +
  +  .can_queue = 1024,
  +  .dma_boundary = UINT_MAX,
  +  .use_clustering = ENABLE_CLUSTERING,
  +};
  +
  +static struct scsi_host_template virtscsi_host_template_multi = {
  +  .module = THIS_MODULE,
  +  .name = Virtio SCSI HBA,
  +  .proc_name

Re: [PATCH v2 1/5] virtio: add functions for piecewise addition of buffers

2012-12-18 Thread Michael S. Tsirkin 
On Tue, Dec 18, 2012 at 03:32:15PM +0100, Paolo Bonzini wrote:
 Il 18/12/2012 14:59, Michael S. Tsirkin ha scritto:
  Can't we track state internally to the virtqueue? Exposing it
  seems to buy us nothing since you can't call add_buf between
  start and end anyway.
  
  I wanted to keep the state for these functions separate from the
  rest. I don't think it makes much sense to move it to struct
  virtqueue unless virtqueue_add_buf is converted to use the new API
  (doesn't make much sense, could even be a tad slower).
  
  Why would it be slower?
 
 virtqueue_add_buf could be slower if it used the new API.  That's
 because of the overhead of writing and reading from struct
 virtqueue_buf, instead of using variables in registers.

Yes but we'll get rid of virtqueue_buf.

  On the other hand moving it there would eliminate the dependency
  on virtio_ring.h.  Rusty, what do you think?
  
  And idea: in practice virtio scsi seems to always call
  sg_init_one, no? So how about we pass in void* or something and
  avoid using sg and count? This would make it useful for -net
  BTW.
  
  It also passes the scatterlist from the LLD.  It calls sg_init_one
  for the request/response headers.
  
  Try adding a _single variant. You might see unrolling a loop gives
  more of a benefit than this whole optimization.
 
 Makes sense, I'll try.  However, note that I *do* need the
 infrastructure in this patch because virtio-scsi could never use a
 hypothetical virtqueue_add_buf_single; requests always have at least 2
 buffers for the headers.
 
 However I could add virtqueue_add_sg_single and use it for those
 headers.

Right.

  The I/O buffer can keep using virtqueue_add_sg.
 
 Paolo
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] virtio-scsi: introduce multiqueue support

2012-12-18 Thread Michael S. Tsirkin 
On Tue, Dec 18, 2012 at 04:51:28PM +0100, Paolo Bonzini wrote:
 Il 18/12/2012 16:03, Michael S. Tsirkin ha scritto:
  On Tue, Dec 18, 2012 at 03:08:08PM +0100, Paolo Bonzini wrote:
  Il 18/12/2012 14:57, Michael S. Tsirkin ha scritto:
  -static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd 
  *sc)
  +static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
  + struct virtio_scsi_target_state *tgt,
  + struct scsi_cmnd *sc)
   {
  -struct virtio_scsi *vscsi = shost_priv(sh);
  -struct virtio_scsi_target_state *tgt = 
  vscsi-tgt[sc-device-id];
   struct virtio_scsi_cmd *cmd;
  +struct virtio_scsi_vq *req_vq;
   int ret;
   
   struct Scsi_Host *shost = virtio_scsi_host(vscsi-vdev);
  @@ -461,7 +533,8 @@ static int virtscsi_queuecommand(struct Scsi_Host 
  *sh, struct scsi_cmnd *sc)
   BUG_ON(sc-cmd_len  VIRTIO_SCSI_CDB_SIZE);
   memcpy(cmd-req.cmd.cdb, sc-cmnd, sc-cmd_len);
   
  -if (virtscsi_kick_cmd(tgt, vscsi-req_vq, cmd,
  +req_vq = ACCESS_ONCE(tgt-req_vq);
 
  This ACCESS_ONCE without a barrier looks strange to me.
  Can req_vq change? Needs a comment.
 
  Barriers are needed to order two things.  Here I don't have the second 
  thing
  to order against, hence no barrier.
 
  Accessing req_vq lockless is safe, and there's a comment about it, but you
  still want ACCESS_ONCE to ensure the compiler doesn't play tricks.
  
  That's just it.
  Why don't you want compiler to play tricks?
 
 Because I want the lockless access to occur exactly when I write it.

It doesn't occur when you write it. CPU can still move accesses
around. That's why you either need both ACCESS_ONCE and a barrier
or none.

 Otherwise I have one more thing to think about, i.e. what a crazy
 compiler writer could do with my code.  And having been on the other
 side of the trench, compiler writers can have *really* crazy ideas.
 
 Anyhow, I'll reorganize the code to move the ACCESS_ONCE closer to the
 write and make it clearer.
 
  +if (virtscsi_kick_cmd(tgt, req_vq, cmd,
 sizeof cmd-req.cmd, sizeof cmd-resp.cmd,
 GFP_ATOMIC) == 0)
   ret = 0;
  @@ -472,6 +545,48 @@ out:
   return ret;
   }
   
  +static int virtscsi_queuecommand_single(struct Scsi_Host *sh,
  +struct scsi_cmnd *sc)
  +{
  +struct virtio_scsi *vscsi = shost_priv(sh);
  +struct virtio_scsi_target_state *tgt = 
  vscsi-tgt[sc-device-id];
  +
  +atomic_inc(tgt-reqs);
 
  And here we don't have barrier after atomic? Why? Needs a comment.
 
  Because we don't write req_vq, so there's no two writes to order.  Barrier
  against what?
  
  Between atomic update and command. Once you queue command it
  can complete and decrement reqs, if this happens before
  increment reqs can become negative even.
 
 This is not a problem.  Please read Documentation/memory-barrier.txt:
 
The following also do _not_ imply memory barriers, and so may
require explicit memory barriers under some circumstances
(smp_mb__before_atomic_dec() for instance):
 
 atomic_add();
 atomic_sub();
 atomic_inc();
 atomic_dec();
 
If they're used for statistics generation, then they probably don't
need memory barriers, unless there's a coupling between statistical
data.
 
 This is the single-queue case, so it falls under this case.

Aha I missed it's single queue. Correct but please add a comment.

   /* Discover virtqueues and write information to configuration.  
  */
  -err = vdev-config-find_vqs(vdev, 3, vqs, callbacks, names);
  +err = vdev-config-find_vqs(vdev, num_vqs, vqs, callbacks, 
  names);
   if (err)
   return err;
   
  -virtscsi_init_vq(vscsi-ctrl_vq, vqs[0]);
  -virtscsi_init_vq(vscsi-event_vq, vqs[1]);
  -virtscsi_init_vq(vscsi-req_vq, vqs[2]);
  +virtscsi_init_vq(vscsi-ctrl_vq, vqs[0], false);
  +virtscsi_init_vq(vscsi-event_vq, vqs[1], false);
  +for (i = VIRTIO_SCSI_VQ_BASE; i  num_vqs; i++)
  +virtscsi_init_vq(vscsi-req_vqs[i - 
  VIRTIO_SCSI_VQ_BASE],
  + vqs[i], vscsi-num_queues  1);
 
  So affinity is true if 1 vq? I am guessing this is not
  going to do the right thing unless you have at least
  as many vqs as CPUs.
 
  Yes, and then you're not setting up the thing correctly.
  
  Why not just check instead of doing the wrong thing?
 
 The right thing could be to set the affinity with a stride, e.g. CPUs
 0-4 for virtqueue 0 and so on until CPUs 3-7 for virtqueue 3.
 
 Paolo

I think a simple #vqs == #cpus check would be kind of OK for
starters, otherwise let userspace set affinity.
Again need to think what happens with CPU hotplug.

  Isn't the same thing true

Re: [PATCH v2 0/5] Multiqueue virtio-scsi, and API for piecewise buffer submission

2012-12-19 Thread Michael S. Tsirkin 
On Wed, Dec 19, 2012 at 09:52:59AM +0100, Paolo Bonzini wrote:
 Il 18/12/2012 23:18, Rolf Eike Beer ha scritto:
  Paolo Bonzini wrote:
  Hi all,
 
  this series adds multiqueue support to the virtio-scsi driver, based
  on Jason Wang's work on virtio-net.  It uses a simple queue steering
  algorithm that expects one queue per CPU.  LUNs in the same target always
  use the same queue (so that commands are not reordered); queue switching
  occurs when the request being queued is the only one for the target.
  Also based on Jason's patches, the virtqueue affinity is set so that
  each CPU is associated to one virtqueue.
 
  I tested the patches with fio, using up to 32 virtio-scsi disks backed
  by tmpfs on the host.  These numbers are with 1 LUN per target.
 
  FIO configuration
  -
  [global]
  rw=read
  bsrange=4k-64k
  ioengine=libaio
  direct=1
  iodepth=4
  loops=20
 
  overall bandwidth (MB/s)
  
 
  # of targetssingle-queuemulti-queue, 4 VCPUsmulti-queue, 8 
  VCPUs
  1  540   626 599
  2  795   965 925
  4  997  13761500
  8 1136  21302060
  161440  22692474
  241408  21792436
  321515  19782319
 
  (These numbers for single-queue are with 4 VCPUs, but the impact of adding
  more VCPUs is very limited).
 
  avg bandwidth per LUN (MB/s)
  
 
  # of targetssingle-queuemulti-queue, 4 VCPUsmulti-queue, 8 
  VCPUs
  1  540   626 599
  2  397   482 462
  4  249   344 375
  8  142   266 257
  16  90   141 154
  24  5890 101
  32  4761  72
  
  Is there an explanation why 8x8 is slower then 4x8 in both cases?
 
 Regarding the in both cases part, it's because the second table has
 the same data as the first, but divided by the first column.
 
 In general, the strangenesses you find are probably within statistical
 noise or due to other effects such as host CPU utilization or contention
 on the big QEMU lock.
 
 Paolo
 

That's exactly what bothers me. If the IOPS divided by host CPU
goes down, then the win on lightly loaded host will become a regression
on a loaded host.

Need to measure that.

  8x1 and 8x2
  being slower than 4x1 and 4x2 is more or less expected, but 8x8 loses 
  against 
  4x8 while 8x4 wins against 4x4 and 8x16 against 4x16.
  
  Eike
  
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] virtio: add functions for piecewise addition of buffers

2012-12-19 Thread Michael S. Tsirkin 
On Wed, Dec 19, 2012 at 01:04:08PM +0100, Paolo Bonzini wrote:
 Il 19/12/2012 11:47, Stefan Hajnoczi ha scritto:
  On Tue, Dec 18, 2012 at 01:32:48PM +0100, Paolo Bonzini wrote:
  +/**
  + * virtqueue_start_buf - start building buffer for the other end
  + * @vq: the struct virtqueue we're talking about.
  + * @buf: a struct keeping the state of the buffer
  + * @data: the token identifying the buffer.
  + * @count: the number of buffers that will be added
  
  Perhaps count should be named count_bufs or num_bufs.
 
 Ok.
 
  + * @count_sg: the number of sg lists that will be added
  
  What is the purpose of count_sg?
 
 It is needed to decide whether to use an indirect or a direct buffer.
 The idea is to avoid a memory allocation if the driver is providing us
 with separate sg elements (under the assumption that they will be few).
 
 Originally I wanted to use a mix of direct and indirect buffer (direct
 if add_buf received a one-element scatterlist, otherwise indirect).  It
 would have had the same effect, without having to specify count_sg in
 advance.  The spec is not clear if that is allowed or not, but in the
 end they do not work with either QEMU or vhost, so I chose this
 alternative instead.
 
 Paolo

Hmm it should work with vhost.

 
  Stefan
  
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] virtio: add functions for piecewise addition of buffers

2012-12-19 Thread Michael S. Tsirkin 
On Wed, Dec 19, 2012 at 06:51:30PM +0200, Michael S. Tsirkin wrote:
 On Wed, Dec 19, 2012 at 01:04:08PM +0100, Paolo Bonzini wrote:
  Il 19/12/2012 11:47, Stefan Hajnoczi ha scritto:
   On Tue, Dec 18, 2012 at 01:32:48PM +0100, Paolo Bonzini wrote:
   +/**
   + * virtqueue_start_buf - start building buffer for the other end
   + * @vq: the struct virtqueue we're talking about.
   + * @buf: a struct keeping the state of the buffer
   + * @data: the token identifying the buffer.
   + * @count: the number of buffers that will be added
   
   Perhaps count should be named count_bufs or num_bufs.
  
  Ok.
  
   + * @count_sg: the number of sg lists that will be added
   
   What is the purpose of count_sg?
  
  It is needed to decide whether to use an indirect or a direct buffer.
  The idea is to avoid a memory allocation if the driver is providing us
  with separate sg elements (under the assumption that they will be few).
  
  Originally I wanted to use a mix of direct and indirect buffer (direct
  if add_buf received a one-element scatterlist, otherwise indirect).  It
  would have had the same effect, without having to specify count_sg in
  advance.  The spec is not clear if that is allowed or not, but in the
  end they do not work with either QEMU or vhost, so I chose this
  alternative instead.
  
  Paolo
 
 Hmm it should work with vhost.

BTW passing in num_in + num_out would be nicer than explicit direction:
closer to the current add_buf.

  
   Stefan
   
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] vhost/scsi: Fix incorrect usage of get_user_pages_fast write parameter

2013-10-28 Thread Michael S. Tsirkin 
On Fri, Oct 25, 2013 at 06:07:16PM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 This patch addresses a long-standing bug where the get_user_pages_fast()
 write parameter used for setting the underlying page table entry permission
 bits was incorrectly set to write=1 for data_direction=DMA_TO_DEVICE, and
 passed into get_user_pages_fast() via vhost_scsi_map_iov_to_sgl().
 
 However, this parameter is intended to signal WRITEs to pinned userspace
 PTEs for the virtio-scsi DMA_FROM_DEVICE - READ payload case, and *not*
 for the virtio-scsi DMA_TO_DEVICE - WRITE payload case.
 
 This bug would manifest itself as random process segmentation faults on
 KVM host after repeated vhost starts + stops and/or with lots of vhost
 endpoints + LUNs.
 
 Cc: Stefan Hajnoczi stefa...@redhat.com
 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Asias He as...@redhat.com
 Cc: sta...@vger.kernel.org # 3.6+
 Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org

Acked-by: Michael S. Tsirkin m...@redhat.com

 ---
  drivers/vhost/scsi.c |2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)
 
 diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
 index ce5221f..e663921 100644
 --- a/drivers/vhost/scsi.c
 +++ b/drivers/vhost/scsi.c
 @@ -1056,7 +1056,7 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct 
 vhost_virtqueue *vq)
   if (data_direction != DMA_NONE) {
   ret = vhost_scsi_map_iov_to_sgl(cmd,
   vq-iov[data_first], data_num,
 - data_direction == DMA_TO_DEVICE);
 + data_direction == DMA_FROM_DEVICE);
   if (unlikely(ret)) {
   vq_err(vq, Failed to map iov to sgl\n);
   goto err_free;
 -- 
 1.7.2.5
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

2014-04-07 Thread Michael S. Tsirkin 
On Sun, Apr 06, 2014 at 09:32:09PM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 This patch updates virtscsi_probe() to setup necessary Scsi_Host
 level protection resources. (currently hardcoded to 1)
 
 It changes virtscsi_add_cmd() to attach outgoing / incoming
 protection SGLs preceeding the data payload, and is using the
 new virtio_scsi_cmd_req_pi-d[oi],pi_niv field to signal
 to signal to vhost/scsi how many prot_sgs to expect.
 
 v3 changes:
   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
 
 v2 changes:
   - Make protection buffer come before data buffer (Paolo)
   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
 
 Cc: Paolo Bonzini pbonz...@redhat.com
 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Martin K. Petersen martin.peter...@oracle.com
 Cc: Christoph Hellwig h...@lst.de
 Cc: Hannes Reinecke h...@suse.de
 Cc: Sagi Grimberg sa...@dev.mellanox.co.il
 Cc: H. Peter Anvin h...@zytor.com
 Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org

OK but we need to document the new interface in the spec
(and incidentially, this will be useful to verify the assumptions
made here and on the host side).
Could you please submit this proposal to the OASIS Virtio TC
for inclusion into the next spec draft?
Ideally as a patch against the tex source, but a prose
description would do as well.
The TC meets on a bi-weekly basis, we should be able to ratify
this quickly.


 ---
  drivers/scsi/virtio_scsi.c |   78 
 ++--
  1 file changed, 60 insertions(+), 18 deletions(-)
 
 diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
 index 16bfd50..68d8d1b 100644
 --- a/drivers/scsi/virtio_scsi.c
 +++ b/drivers/scsi/virtio_scsi.c
 @@ -37,6 +37,7 @@ struct virtio_scsi_cmd {
   struct completion *comp;
   union {
   struct virtio_scsi_cmd_req   cmd;
 + struct virtio_scsi_cmd_req_picmd_pi;
   struct virtio_scsi_ctrl_tmf_req  tmf;
   struct virtio_scsi_ctrl_an_req   an;
   } req;
 @@ -440,7 +441,7 @@ static int virtscsi_add_cmd(struct virtqueue *vq,
   size_t req_size, size_t resp_size, gfp_t gfp)
  {
   struct scsi_cmnd *sc = cmd-sc;
 - struct scatterlist *sgs[4], req, resp;
 + struct scatterlist *sgs[6], req, resp;
   struct sg_table *out, *in;
   unsigned out_num = 0, in_num = 0;
  
 @@ -458,16 +459,24 @@ static int virtscsi_add_cmd(struct virtqueue *vq,
   sgs[out_num++] = req;
  
   /* Data-out buffer.  */
 - if (out)
 + if (out) {
 + /* Place WRITE protection SGLs before Data OUT payload */
 + if (scsi_prot_sg_count(sc))
 + sgs[out_num++] = scsi_prot_sglist(sc);
   sgs[out_num++] = out-sgl;
 + }
  
   /* Response header.  */
   sg_init_one(resp, cmd-resp, resp_size);
   sgs[out_num + in_num++] = resp;
  
   /* Data-in buffer */
 - if (in)
 + if (in) {
 + /* Place READ protection SGLs before Data IN payload */
 + if (scsi_prot_sg_count(sc))
 + sgs[out_num + in_num++] = scsi_prot_sglist(sc);
   sgs[out_num + in_num++] = in-sgl;
 + }
  
   return virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, gfp);
  }
 @@ -492,12 +501,36 @@ static int virtscsi_kick_cmd(struct virtio_scsi_vq *vq,
   return err;
  }
  
 +static void virtio_scsi_init_hdr(struct virtio_scsi_cmd_req *cmd,
 +  struct scsi_cmnd *sc)
 +{
 + cmd-lun[0] = 1;
 + cmd-lun[1] = sc-device-id;
 + cmd-lun[2] = (sc-device-lun  8) | 0x40;
 + cmd-lun[3] = sc-device-lun  0xff;
 + cmd-tag = (unsigned long)sc;
 + cmd-task_attr = VIRTIO_SCSI_S_SIMPLE;
 + cmd-prio = 0;
 + cmd-crn = 0;
 +}
 +
 +static void virtio_scsi_init_hdr_pi(struct virtio_scsi_cmd_req_pi *cmd_pi,
 + struct scsi_cmnd *sc)
 +{
 + virtio_scsi_init_hdr((struct virtio_scsi_cmd_req *)cmd_pi, sc);
 +
 + if (sc-sc_data_direction == DMA_TO_DEVICE)
 + cmd_pi-do_pi_niov = scsi_prot_sg_count(sc);
 + else if (sc-sc_data_direction == DMA_FROM_DEVICE)
 + cmd_pi-di_pi_niov = scsi_prot_sg_count(sc);
 +}
 +
  static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
struct virtio_scsi_vq *req_vq,
struct scsi_cmnd *sc)
  {
   struct virtio_scsi_cmd *cmd;
 - int ret;
 + int ret, req_size;
  
   struct Scsi_Host *shost = virtio_scsi_host(vscsi-vdev);
   BUG_ON(scsi_sg_count(sc)  shost-sg_tablesize);
 @@ -515,22 +548,20 @@ static int virtscsi_queuecommand(struct virtio_scsi 
 *vscsi,
  
   memset(cmd, 0, sizeof(*cmd));
   cmd-sc = sc;
 - cmd-req.cmd = (struct virtio_scsi_cmd_req){
 - .lun[0] = 1,
 - .lun[1] = sc-device-id,
 - .lun[2] = (sc-device-lun  8) | 0x40,
 - .lun

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

2014-04-07 Thread Michael S. Tsirkin 
On Mon, Apr 07, 2014 at 01:56:59AM -0700, Nicholas A. Bellinger wrote:
 On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
  On Sun, Apr 06, 2014 at 09:32:09PM +, Nicholas A. Bellinger wrote:
   From: Nicholas Bellinger n...@linux-iscsi.org
   
   This patch updates virtscsi_probe() to setup necessary Scsi_Host
   level protection resources. (currently hardcoded to 1)
   
   It changes virtscsi_add_cmd() to attach outgoing / incoming
   protection SGLs preceeding the data payload, and is using the
   new virtio_scsi_cmd_req_pi-d[oi],pi_niv field to signal
   to signal to vhost/scsi how many prot_sgs to expect.
   
   v3 changes:
 - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
   
   v2 changes:
 - Make protection buffer come before data buffer (Paolo)
 - Enable virtio_scsi_cmd_req_pi usage (Paolo)
   
   Cc: Paolo Bonzini pbonz...@redhat.com
   Cc: Michael S. Tsirkin m...@redhat.com
   Cc: Martin K. Petersen martin.peter...@oracle.com
   Cc: Christoph Hellwig h...@lst.de
   Cc: Hannes Reinecke h...@suse.de
   Cc: Sagi Grimberg sa...@dev.mellanox.co.il
   Cc: H. Peter Anvin h...@zytor.com
   Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
  
  OK but we need to document the new interface in the spec
  (and incidentially, this will be useful to verify the assumptions
  made here and on the host side).
  Could you please submit this proposal to the OASIS Virtio TC
  for inclusion into the next spec draft?
  Ideally as a patch against the tex source, but a prose
  description would do as well.
 
 Most certainly.  Please give me a bit to follow up on this, as the next
 couple of days are going to be hellishly busy..
 
  The TC meets on a bi-weekly basis, we should be able to ratify
  this quickly.
  
 
 Aside from that, please consider ACK'ing the vhost specific changes so
 these can make it into v3.15-rc1 code.
 
 --nab
 

Hmm but what if the TC wants to change the interface somewhat?
I guess we'll still be able to fix it after the merge window -
(or worst case, revert the change) is this what you are suggesting?

-- 
MST
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] virtio-scsi.h: Add virtio_scsi_cmd_req_pi + VIRTIO_SCSI_F_T10_PI bits

2014-04-07 Thread Michael S. Tsirkin 
On Sun, Apr 06, 2014 at 09:32:04PM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 This patch adds a virtio_scsi_cmd_req_pi header as recommened by
 Paolo that contains do_pi_niov + di_pi_niov elements used for
 signaling when protection information buffers are expected to
 preceed the data buffers.
 
 Also add new VIRTIO_SCSI_F_T10_PI feature bit to be used to signal
 host support.
 
 Cc: Paolo Bonzini pbonz...@redhat.com
 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Martin K. Petersen martin.peter...@oracle.com
 Cc: Christoph Hellwig h...@lst.de
 Cc: Hannes Reinecke h...@suse.de
 Cc: Sagi Grimberg sa...@dev.mellanox.co.il
 Cc: H. Peter Anvin h...@zytor.com
 Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
 ---
  include/linux/virtio_scsi.h |   15 ++-
  1 file changed, 14 insertions(+), 1 deletion(-)
 
 diff --git a/include/linux/virtio_scsi.h b/include/linux/virtio_scsi.h
 index 4195b97..590d249 100644
 --- a/include/linux/virtio_scsi.h
 +++ b/include/linux/virtio_scsi.h
 @@ -35,11 +35,23 @@ struct virtio_scsi_cmd_req {
   u8 lun[8];  /* Logical Unit Number */
   u64 tag;/* Command identifier */
   u8 task_attr;   /* Task attribute */
 - u8 prio;
 + u8 prio;/* SAM command priority field */
   u8 crn;
   u8 cdb[VIRTIO_SCSI_CDB_SIZE];
  } __packed;
  
 +/* SCSI command request, followed by protection information */
 +struct virtio_scsi_cmd_req_pi {
 + u8 lun[8];  /* Logical Unit Number */
 + u64 tag;/* Command identifier */
 + u8 task_attr;   /* Task attribute */
 + u8 prio;/* SAM command priority field */
 + u8 crn;

Hmm if we stick a reserved byte here, following fields will be
naturally aligned and we'd be able to get rid of __packed
instruction (which often causes gcc to generate worse code).

 + u16 do_pi_niov; /* DataOUT PI Number of iovecs */
 + u16 di_pi_niov; /* DataIN PI Number of iovecs */

So this looks like a somewhat problematic interface to me in that
it talks in terms of iovecs not bytes.
So this perpetuates the assumption that header is in a separate
iov from data (and protection is separate from data).
Arguably virtio doesn't work in terms of iovecs on the guest side so
this naming looks strange.
Further host side, get_vq_descs can in theory split a buffer to multiple
iovecs if it crosses the boundary of a memory region.

One solution is to use byte lengths here, but this does require
that vhost scsi gets rid of layout assumptions generally.
Not sure that's practical for -rc1.

Another is to do it like virtio-net does for RX, link two buffers
using the first one for data and the second one for protection.


 + u8 cdb[VIRTIO_SCSI_CDB_SIZE];
 +} __packed;
 +
  /* Response, followed by sense data and data-in */
  struct virtio_scsi_cmd_resp {
   u32 sense_len;  /* Sense data length */
 @@ -97,6 +109,7 @@ struct virtio_scsi_config {
  #define VIRTIO_SCSI_F_INOUT0
  #define VIRTIO_SCSI_F_HOTPLUG  1
  #define VIRTIO_SCSI_F_CHANGE   2
 +#define VIRTIO_SCSI_F_T10_PI3
  
  /* Response codes */
  #define VIRTIO_SCSI_S_OK   0
 -- 
 1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] virtio-scsi.h: Add virtio_scsi_cmd_req_pi + VIRTIO_SCSI_F_T10_PI bits

2014-04-09 Thread Michael S. Tsirkin 
On Tue, Apr 08, 2014 at 04:31:26PM -0400, Paolo Bonzini wrote:
 Il 07/04/2014 05:55, Michael S. Tsirkin ha scritto:
  + u16 do_pi_niov; /* DataOUT PI Number of iovecs */
  + u16 di_pi_niov; /* DataIN PI Number of iovecs */
 So this looks like a somewhat problematic interface to me in that
 it talks in terms of iovecs not bytes.
 So this perpetuates the assumption that header is in a separate
 iov from data (and protection is separate from data).
 Arguably virtio doesn't work in terms of iovecs on the guest side so
 this naming looks strange.
 Further host side, get_vq_descs can in theory split a buffer to multiple
 iovecs if it crosses the boundary of a memory region.
 
 One solution is to use byte lengths here, but this does require
 that vhost scsi gets rid of layout assumptions generally.
 Not sure that's practical for -rc1.
 
 Why does that require that vhost scsi gets rid of layout assumptions?
 
 The interface uses bytes instead of iovecs as the unit, and
 vhost-scsi can add the (temporary...) requirement that do_pi_nbytes
 and di_pi_nbytes comprise an integer number of iovecs.
 
 Paolo

That would be a reasonable intermediate step, but I'm worried there
are more assumptions lurking in tere.

In particular is it legal for PI to be in the same iov
entry as data? If not they really need to use a
separate buf entry.

-- 
MST
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

2014-05-07 Thread Michael S. Tsirkin 
On Mon, Apr 07, 2014 at 01:56:59AM -0700, Nicholas A. Bellinger wrote:
 On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
  On Sun, Apr 06, 2014 at 09:32:09PM +, Nicholas A. Bellinger wrote:
   From: Nicholas Bellinger n...@linux-iscsi.org
   
   This patch updates virtscsi_probe() to setup necessary Scsi_Host
   level protection resources. (currently hardcoded to 1)
   
   It changes virtscsi_add_cmd() to attach outgoing / incoming
   protection SGLs preceeding the data payload, and is using the
   new virtio_scsi_cmd_req_pi-d[oi],pi_niv field to signal
   to signal to vhost/scsi how many prot_sgs to expect.
   
   v3 changes:
 - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
   
   v2 changes:
 - Make protection buffer come before data buffer (Paolo)
 - Enable virtio_scsi_cmd_req_pi usage (Paolo)
   
   Cc: Paolo Bonzini pbonz...@redhat.com
   Cc: Michael S. Tsirkin m...@redhat.com
   Cc: Martin K. Petersen martin.peter...@oracle.com
   Cc: Christoph Hellwig h...@lst.de
   Cc: Hannes Reinecke h...@suse.de
   Cc: Sagi Grimberg sa...@dev.mellanox.co.il
   Cc: H. Peter Anvin h...@zytor.com
   Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
  
  OK but we need to document the new interface in the spec
  (and incidentially, this will be useful to verify the assumptions
  made here and on the host side).
  Could you please submit this proposal to the OASIS Virtio TC
  for inclusion into the next spec draft?
  Ideally as a patch against the tex source, but a prose
  description would do as well.
 
 Most certainly.  Please give me a bit to follow up on this, as the next
 couple of days are going to be hellishly busy..

Ping.
We really need to get this moving to have the interface reviewed for
the next merge window.


  The TC meets on a bi-weekly basis, we should be able to ratify
  this quickly.
  
 
 Aside from that, please consider ACK'ing the vhost specific changes so
 these can make it into v3.15-rc1 code.
 
 --nab
 
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

2014-05-19 Thread Michael S. Tsirkin 
On Mon, May 19, 2014 at 12:07:03PM -0700, Nicholas A. Bellinger wrote:
 On Wed, 2014-05-07 at 12:13 +0300, Michael S. Tsirkin wrote:
  On Mon, Apr 07, 2014 at 01:56:59AM -0700, Nicholas A. Bellinger wrote:
   On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
On Sun, Apr 06, 2014 at 09:32:09PM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 This patch updates virtscsi_probe() to setup necessary Scsi_Host
 level protection resources. (currently hardcoded to 1)
 
 It changes virtscsi_add_cmd() to attach outgoing / incoming
 protection SGLs preceeding the data payload, and is using the
 new virtio_scsi_cmd_req_pi-d[oi],pi_niv field to signal
 to signal to vhost/scsi how many prot_sgs to expect.
 
 v3 changes:
   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
 
 v2 changes:
   - Make protection buffer come before data buffer (Paolo)
   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
 
 Cc: Paolo Bonzini pbonz...@redhat.com
 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Martin K. Petersen martin.peter...@oracle.com
 Cc: Christoph Hellwig h...@lst.de
 Cc: Hannes Reinecke h...@suse.de
 Cc: Sagi Grimberg sa...@dev.mellanox.co.il
 Cc: H. Peter Anvin h...@zytor.com
 Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org

OK but we need to document the new interface in the spec
(and incidentially, this will be useful to verify the assumptions
made here and on the host side).
Could you please submit this proposal to the OASIS Virtio TC
for inclusion into the next spec draft?
Ideally as a patch against the tex source, but a prose
description would do as well.
   
   Most certainly.  Please give me a bit to follow up on this, as the next
   couple of days are going to be hellishly busy..
  
  Ping.
  We really need to get this moving to have the interface reviewed for
  the next merge window.
  
 
 Hi MST,
 
 So I've finally got some cycles to get back to this code, and wanted to
 verify the outstanding items you had previously raised:
 
 - Convert vhost-scsi to be independent of IOV layout using 
   memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi 
   operation..?)

Ideally yes.

 - Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
 - Convert virtio_scsi_cmd_req_pi to bytes field instead of number of 
   iovecs.
 - Ensure virtio_scsi_cmd_req_pi is naturally aligned

It turns out other virtio scsi commands aren't aligned?
If true we don't need to make an exception here.

 - Figure out why QEMU is not acking (any) vhost-scsi feature bits
 
 Is there anything else that you'd like to see for an initial merge, or
 other issues that need to be addressed with the above..?
 
 --nab

FYI Paolo sent a spec patch for the feature.
Have you seen it?

-- 
MST
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

2014-05-19 Thread Michael S. Tsirkin 
On Mon, May 19, 2014 at 01:54:50PM -0700, Nicholas A. Bellinger wrote:
 On Mon, 2014-05-19 at 22:15 +0300, Michael S. Tsirkin wrote:
  On Mon, May 19, 2014 at 12:07:03PM -0700, Nicholas A. Bellinger wrote:
   On Wed, 2014-05-07 at 12:13 +0300, Michael S. Tsirkin wrote:
On Mon, Apr 07, 2014 at 01:56:59AM -0700, Nicholas A. Bellinger wrote:
 On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
  On Sun, Apr 06, 2014 at 09:32:09PM +, Nicholas A. Bellinger 
  wrote:
   From: Nicholas Bellinger n...@linux-iscsi.org
   
   This patch updates virtscsi_probe() to setup necessary Scsi_Host
   level protection resources. (currently hardcoded to 1)
   
   It changes virtscsi_add_cmd() to attach outgoing / incoming
   protection SGLs preceeding the data payload, and is using the
   new virtio_scsi_cmd_req_pi-d[oi],pi_niv field to signal
   to signal to vhost/scsi how many prot_sgs to expect.
   
   v3 changes:
 - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header 
   (Paolo)
   
   v2 changes:
 - Make protection buffer come before data buffer (Paolo)
 - Enable virtio_scsi_cmd_req_pi usage (Paolo)
   
   Cc: Paolo Bonzini pbonz...@redhat.com
   Cc: Michael S. Tsirkin m...@redhat.com
   Cc: Martin K. Petersen martin.peter...@oracle.com
   Cc: Christoph Hellwig h...@lst.de
   Cc: Hannes Reinecke h...@suse.de
   Cc: Sagi Grimberg sa...@dev.mellanox.co.il
   Cc: H. Peter Anvin h...@zytor.com
   Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
  
  OK but we need to document the new interface in the spec
  (and incidentially, this will be useful to verify the assumptions
  made here and on the host side).
  Could you please submit this proposal to the OASIS Virtio TC
  for inclusion into the next spec draft?
  Ideally as a patch against the tex source, but a prose
  description would do as well.
 
 Most certainly.  Please give me a bit to follow up on this, as the 
 next
 couple of days are going to be hellishly busy..

Ping.
We really need to get this moving to have the interface reviewed for
the next merge window.

   
   Hi MST,
   
   So I've finally got some cycles to get back to this code, and wanted to
   verify the outstanding items you had previously raised:
   
   - Convert vhost-scsi to be independent of IOV layout using 
 memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi 
 operation..?)
  
  Ideally yes.
 
 Er, so changing vhost-scsi to be independent of IOV layout will have the
 side effect of breaking existing non PI virtio-scsi logic..?

Sorry I didn't make myself clear.
I merely think that all code should do memcpy_fromiovecend etc.
If done peoperly guests will not notice unless they test
VIRTIO_F_ANY_LAYOUT.



  
   - Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
   - Convert virtio_scsi_cmd_req_pi to bytes field instead of number of 
 iovecs.
   - Ensure virtio_scsi_cmd_req_pi is naturally aligned
  
  It turns out other virtio scsi commands aren't aligned?
 
 Correct, virtio_scsi_cmd_req is currently 51 bytes.
 
  If true we don't need to make an exception here.
 
 nod
 
  
   - Figure out why QEMU is not acking (any) vhost-scsi feature bits
   
   Is there anything else that you'd like to see for an initial merge, or
   other issues that need to be addressed with the above..?
   
   --nab
  
  FYI Paolo sent a spec patch for the feature.
  Have you seen it?
  
 
 Yep, looks fine.
 
 --nab
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-v2 1/6] virtio-scsi.h: Add virtio_scsi_cmd_req_pi + VIRTIO_SCSI_F_T10_PI bits

2014-05-22 Thread Michael S. Tsirkin 
On Thu, May 22, 2014 at 02:26:17AM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 This patch adds a virtio_scsi_cmd_req_pi header as recommened by
 Paolo that contains do_pi_niov + di_pi_niov elements used for
 signaling when protection information buffers are expected to
 preceed the data buffers.

Cc rusty.
Rusty could you please Ack merging this through Nicholas' tree
together with the vhost changes?


 Also add new VIRTIO_SCSI_F_T10_PI feature bit to be used to signal
 host support.
 
 v4 changes:

v2? Proably best to drop versioning info from commit log,
move it out to notes (after ---)

- Use pi_bytesout + pi_bytesin instead of niovs (mst + paolo)

Right, so maybe update the commit log above to match?
It gave me pause.

 Cc: Paolo Bonzini pbonz...@redhat.com
 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Martin K. Petersen martin.peter...@oracle.com
 Cc: Christoph Hellwig h...@lst.de
 Cc: Hannes Reinecke h...@suse.de
 Cc: Sagi Grimberg sa...@dev.mellanox.co.il
 Cc: H. Peter Anvin h...@zytor.com
 Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
 ---
  include/linux/virtio_scsi.h |   15 ++-
  1 file changed, 14 insertions(+), 1 deletion(-)
 
 diff --git a/include/linux/virtio_scsi.h b/include/linux/virtio_scsi.h
 index 4195b97..7344906 100644
 --- a/include/linux/virtio_scsi.h
 +++ b/include/linux/virtio_scsi.h
 @@ -35,11 +35,23 @@ struct virtio_scsi_cmd_req {
   u8 lun[8];  /* Logical Unit Number */
   u64 tag;/* Command identifier */
   u8 task_attr;   /* Task attribute */
 - u8 prio;
 + u8 prio;/* SAM command priority field */
   u8 crn;
   u8 cdb[VIRTIO_SCSI_CDB_SIZE];
  } __packed;
  
 +/* SCSI command request, followed by protection information */
 +struct virtio_scsi_cmd_req_pi {
 + u8 lun[8];  /* Logical Unit Number */
 + u64 tag;/* Command identifier */
 + u8 task_attr;   /* Task attribute */
 + u8 prio;/* SAM command priority field */
 + u8 crn;
 + u32 pi_bytesout;/* DataOUT PI Number of bytes */
 + u32 pi_bytesin; /* DataIN PI Number of bytes */
 + u8 cdb[VIRTIO_SCSI_CDB_SIZE];
 +} __packed;
 +
  /* Response, followed by sense data and data-in */
  struct virtio_scsi_cmd_resp {
   u32 sense_len;  /* Sense data length */
 @@ -97,6 +109,7 @@ struct virtio_scsi_config {
  #define VIRTIO_SCSI_F_INOUT0
  #define VIRTIO_SCSI_F_HOTPLUG  1
  #define VIRTIO_SCSI_F_CHANGE   2
 +#define VIRTIO_SCSI_F_T10_PI3

Could you pad this with spaces and not tabs so it aligns nicely
with stuff above it?

  
  /* Response codes */
  #define VIRTIO_SCSI_S_OK   0
 -- 
 1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH trivial] include linux/mutex.h from scsi_transport_iscsi.h

2007-07-25 Thread Michael S. Tsirkin 
scsi/scsi_transport_iscsi.h uses struct mutex, so while
linux/mutex.h seems to be pulled in indirectly
by one of the headers it includes, the right thing
is to include linux/mutex.h directly.

Signed-off-by: Michael S. Tsirkin [EMAIL PROTECTED]

---

diff --git a/include/scsi/scsi_transport_iscsi.h 
b/include/scsi/scsi_transport_iscsi.h
index 706c0cd..7530e98 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -24,6 +24,7 @@
 #define SCSI_TRANSPORT_ISCSI_H
 
 #include linux/device.h
+#include linux/mutex.h
 #include scsi/iscsi_if.h
 
 struct scsi_transport_template;


-- 
MST
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH trivial] include linux/mutex.h from scsi_transport_iscsi.h

2007-07-25 Thread Michael S. Tsirkin 
 Quoting Mike Christie [EMAIL PROTECTED]:
 Subject: Re: [PATCH trivial] include linux/mutex.h from scsi_transport_iscsi.h
 
 Michael S. Tsirkin wrote:
 scsi/scsi_transport_iscsi.h uses struct mutex, so while
 linux/mutex.h seems to be pulled in indirectly
 by one of the headers it includes, the right thing
 is to include linux/mutex.h directly.
 
 
 Is that part about always including the header directly right?

Think so. Analogous patches by me has been accepted in various
subsystems. See e.g. f8916c11a4dc4cb2367e9bee1788f4e0f1b4eabc.

 If so 
 then were you going to include list.h too,

Makes sense. I'll repost.

 and were you going to fix up 
 some of the other iscsi code?

Not at the moment.
The reason I noticed this is because I'm doing some other project.
I'll post patches for other files if/when I notice any issues.

-- 
MST
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH trivial v2] add includes to scsi_transport_iscsi.h

2007-07-26 Thread Michael S. Tsirkin 
scsi/scsi_transport_iscsi.h uses struct mutex and struct list_head,
so while linux/mutex.h and linux/list.h seem to be pulled in indirectly
by one of the headers it includes, the right thing
is to include linux/mutex.h and linus/list.h directly.

Signed-off-by: Michael S. Tsirkin [EMAIL PROTECTED]

---

Changelog:

Mike Christie pointed out that linux/list.h is missing too.

diff --git a/include/scsi/scsi_transport_iscsi.h 
b/include/scsi/scsi_transport_iscsi.h
index 706c0cd..7ff6199 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -24,6 +24,8 @@
 #define SCSI_TRANSPORT_ISCSI_H
 
 #include linux/device.h
+#include linux/list.h
+#include linux/mutex.h
 #include scsi/iscsi_if.h
 
 struct scsi_transport_template;

-- 
MST
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC-v2 0/4] tcm_vhost+cmwq fabric driver code for-3.6

2012-07-17 Thread Michael S. Tsirkin 
On Wed, Jul 11, 2012 at 09:15:00PM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 Hi folks,
 
 The following is a RFC-v2 series of tcm_vhost target fabric driver code
 currently in-flight for-3.6 mainline code.
 
 After last week's developments along with the help of some new folks, the
 changelog v1 - v2 so far looks like:
 
 *) Fix drivers/vhost/test.c to use VHOST_NET_FEATURES in patch #1 (Asias He)
 *) Fix tv_cmd completion - release SGL memory leak (nab)
 *) Fix sparse warnings for static variable usage (Fengguang Wu)
 *) Fix sparse warnings for min() typing + printk format specs (Fengguang Wu)
 *) Convert to cmwq submission for I/O dispatch (nab + hch)
 
 Also following Paolo's request, a patch for hw/virtio-scsi.c that sets
 scsi_host-max_target=0 that removes the need for virtio-scsi LLD to hardcode
 VirtIOSCSIConfig-max_id=1 in order to function with tcm_vhost.
 
 Note this series has been pushed into target-pending.git/for-next-merge, and
 should be getting picked up for tomorrow's linux-next build.
 
 Please let us know if you have any concerns and/or additional review feedback.
 
 Thank you!


It still seems not 100% clear whether this driver will have major
userspace using it. And if not, it would be very hard to support a driver
when recent userspace does not use it in the end.

I think a good idea for 3.6 would be to make it depend on CONFIG_STAGING.
Then we don't commit to an ABI.
For this, you can add a separate Kconfig and source it from 
drivers/staging/Kconfig.
Maybe it needs to be in a separate directory drivers/vhost/staging/Kconfig.


 Nicholas Bellinger (2):
   vhost: Add vhost_scsi specific defines
   tcm_vhost: Initial merge for vhost level target fabric driver
 
 Stefan Hajnoczi (2):
   vhost: Separate vhost-net features from vhost features
   vhost: make vhost work queue visible
 
  drivers/vhost/Kconfig |6 +
  drivers/vhost/Makefile|1 +
  drivers/vhost/net.c   |4 +-
  drivers/vhost/tcm_vhost.c | 1609 
 +
  drivers/vhost/tcm_vhost.h |   74 ++
  drivers/vhost/test.c  |4 +-
  drivers/vhost/vhost.c |5 +-
  drivers/vhost/vhost.h |6 +-
  include/linux/vhost.h |9 +
  9 files changed, 1710 insertions(+), 8 deletions(-)
  create mode 100644 drivers/vhost/tcm_vhost.c
  create mode 100644 drivers/vhost/tcm_vhost.h
 
 -- 
 1.7.2.5
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND 0/5] Add vhost-blk support

2012-07-17 Thread Michael S. Tsirkin 
On Fri, Jul 13, 2012 at 04:55:06PM +0800, Asias He wrote:
 
 Hi folks,
 
 [I am resending to fix the broken thread in the previous one.]
 
 This patchset adds vhost-blk support. vhost-blk is a in kernel virito-blk
 device accelerator. Compared to userspace virtio-blk implementation, vhost-blk
 gives about 5% to 15% performance improvement.

Same thing as tcm_host comment:

It seems not 100% clear whether this driver will have major
userspace using it. And if not, it would be very hard to support a
driver when recent userspace does not use it in the end.

I think a good idea for 3.6 would be to make it depend on
CONFIG_STAGING.  Then we don't commit to an ABI.  For this, you can add
a separate Kconfig and source it from drivers/staging/Kconfig.  Maybe it
needs to be in a separate directory drivers/vhost/staging/Kconfig.

I Cc'd the list of tcm_host in the hope that you can cooperate on this.

-- 
MST
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC-v2 0/4] tcm_vhost+cmwq fabric driver code for-3.6

2012-07-17 Thread Michael S. Tsirkin 
On Tue, Jul 17, 2012 at 01:55:42PM -0500, Anthony Liguori wrote:
 On 07/17/2012 10:05 AM, Michael S. Tsirkin wrote:
 On Wed, Jul 11, 2012 at 09:15:00PM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellingern...@linux-iscsi.org
 
 Hi folks,
 
 The following is a RFC-v2 series of tcm_vhost target fabric driver code
 currently in-flight for-3.6 mainline code.
 
 After last week's developments along with the help of some new folks, the
 changelog v1 -  v2 so far looks like:
 
 *) Fix drivers/vhost/test.c to use VHOST_NET_FEATURES in patch #1 (Asias He)
 *) Fix tv_cmd completion -  release SGL memory leak (nab)
 *) Fix sparse warnings for static variable usage (Fengguang Wu)
 *) Fix sparse warnings for min() typing + printk format specs (Fengguang Wu)
 *) Convert to cmwq submission for I/O dispatch (nab + hch)
 
 Also following Paolo's request, a patch for hw/virtio-scsi.c that sets
 scsi_host-max_target=0 that removes the need for virtio-scsi LLD to 
 hardcode
 VirtIOSCSIConfig-max_id=1 in order to function with tcm_vhost.
 
 Note this series has been pushed into target-pending.git/for-next-merge, and
 should be getting picked up for tomorrow's linux-next build.
 
 Please let us know if you have any concerns and/or additional review 
 feedback.
 
 Thank you!
 
 
 It still seems not 100% clear whether this driver will have major
 userspace using it. And if not, it would be very hard to support a driver
 when recent userspace does not use it in the end.
 
 I don't think this is a good reason to exclude something from the
 kernel. However, there are good reasons why this doesn't make sense
 for something like QEMU--specifically because we have a large number
 of features in our block layer that tcm_vhost would bypass.
 
 But perhaps it makes sense for something like native kvm tool.  And
 if it did go into the kernel, we would certainly support it in QEMU.
 
 But I do think the kernel should carefully consider whether it wants
 to support an interface like this.  This an extremely complicated
 ABI with a lot of subtle details around state and compatibility.
 
 Are you absolutely confident that you can support a userspace
 application that expects to get exactly the same response from all
 possible commands in 20 kernel versions from now?  Virtualization
 requires absolutely precise compatibility in terms of bugs and
 features.  This is probably not something the TCM stack has had to
 consider yet.
 
 I think a good idea for 3.6 would be to make it depend on CONFIG_STAGING.
 Then we don't commit to an ABI.
 
 I think this is a good idea.  Even if it goes in, a really clear
 policy would be needed wrt the userspace ABI.
 
 While tcm_vhost is probably more useful than vhost_blk, it's a much
 more complex ABI to maintain.
 
 Regards,
 
 Anthony Liguori

Maybe something like a whitelist of features will help?
Might even be a good idea to make it user controllable.

 For this, you can add a separate Kconfig and source it from 
 drivers/staging/Kconfig.
 Maybe it needs to be in a separate directory drivers/vhost/staging/Kconfig.
 
 
 Nicholas Bellinger (2):
vhost: Add vhost_scsi specific defines
tcm_vhost: Initial merge for vhost level target fabric driver
 
 Stefan Hajnoczi (2):
vhost: Separate vhost-net features from vhost features
vhost: make vhost work queue visible
 
   drivers/vhost/Kconfig |6 +
   drivers/vhost/Makefile|1 +
   drivers/vhost/net.c   |4 +-
   drivers/vhost/tcm_vhost.c | 1609 
  +
   drivers/vhost/tcm_vhost.h |   74 ++
   drivers/vhost/test.c  |4 +-
   drivers/vhost/vhost.c |5 +-
   drivers/vhost/vhost.h |6 +-
   include/linux/vhost.h |9 +
   9 files changed, 1710 insertions(+), 8 deletions(-)
   create mode 100644 drivers/vhost/tcm_vhost.c
   create mode 100644 drivers/vhost/tcm_vhost.h
 
 --
 1.7.2.5
 
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC-v2 0/4] tcm_vhost+cmwq fabric driver code for-3.6

2012-07-17 Thread Michael S. Tsirkin 
On Tue, Jul 17, 2012 at 02:17:22PM -0700, Nicholas A. Bellinger wrote:
 On Tue, 2012-07-17 at 18:05 +0300, Michael S. Tsirkin wrote:
  On Wed, Jul 11, 2012 at 09:15:00PM +, Nicholas A. Bellinger wrote:
   From: Nicholas Bellinger n...@linux-iscsi.org
   
   Hi folks,
   
   The following is a RFC-v2 series of tcm_vhost target fabric driver code
   currently in-flight for-3.6 mainline code.
   
   After last week's developments along with the help of some new folks, the
   changelog v1 - v2 so far looks like:
   
   *) Fix drivers/vhost/test.c to use VHOST_NET_FEATURES in patch #1 (Asias 
   He)
   *) Fix tv_cmd completion - release SGL memory leak (nab)
   *) Fix sparse warnings for static variable usage (Fengguang Wu)
   *) Fix sparse warnings for min() typing + printk format specs (Fengguang 
   Wu)
   *) Convert to cmwq submission for I/O dispatch (nab + hch)
   
   Also following Paolo's request, a patch for hw/virtio-scsi.c that sets
   scsi_host-max_target=0 that removes the need for virtio-scsi LLD to 
   hardcode
   VirtIOSCSIConfig-max_id=1 in order to function with tcm_vhost.
   
   Note this series has been pushed into target-pending.git/for-next-merge, 
   and
   should be getting picked up for tomorrow's linux-next build.
   
   Please let us know if you have any concerns and/or additional review 
   feedback.
   
   Thank you!
  
  
  It still seems not 100% clear whether this driver will have major
  userspace using it. And if not, it would be very hard to support a driver
  when recent userspace does not use it in the end.
  
 
 I'm happy to commit to working with QEMU + kvm-tool folks to get to a
 series that can (eventually) see vhost-scsi support merged into upstream
 userspace code.
 
 It took roughly 2 years to get the megasas HBA emulation from Dr. Hannes
 merged, but certainly vhost-scsi has alot less moving pieces and
 hopefully alot less controversial bits than the buffer - SGL
 conversion..  The key word being here 'hopefully'..  ;)
 
  I think a good idea for 3.6 would be to make it depend on CONFIG_STAGING.
  Then we don't commit to an ABI.
  For this, you can add a separate Kconfig and source it from 
  drivers/staging/Kconfig.
  Maybe it needs to be in a separate directory drivers/vhost/staging/Kconfig.
  
 
 So tcm_vhost has been marked as Experimental following virtio-scsi.
 
 Wrt to staging, I'd like to avoid mucking with staging because:
 
 *) The code has been posted for review
 *) The code has been converted to use the latest target-core primitives
 *) The code does not require cleanups between staging - merge
 *) The code has been stable the last 7 days since RFC-v2 with heavy 

staging is not just for code that needs cleanups.
It's for anything that does not guarantee ABI stability yet.
And I think it's a bit early to guarantee ABI stability - 7 days
is not all that long.

See for example Anthony's comments that raise exactly the ABI
issues.

 Also, tcm_vhost has been marked as Experimental following virtio-scsi.
 I'd much rather leave it at Experimental until we merge upstream
 userspace support.   If userspace support never ends up materializing,
 I'm fine with dropping it all together.

Once it's in kernel you never know who will use this driver.
Experimental does not mean driver can be dropped, staging does.

 However at this point given that there is a 3x performance gap between
 virtio-scsi-raw + virtio-scsi+tcm_vhost for random mixed small block
 I/O, and we still need the latter to do proper SCSI CDB passthrough for
 non TYPE_DISK devices I'm hoping that we can agree on userspace bits
 once tcm_vhost is merged.
 
 --nab

I do think upstream kernel would help you nail userspace issues too
but at this point it looks like either staging meterial or 3.6 is too
early.

-- 
MST
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  1   2   >