Re: [PATCH v2] linux/types.h: Restore the ability to disable sparse endianness checks

[Michael S. Tsirkin]

On Mon, Oct 16, 2017 at 10:26:33AM -0700, Bart Van Assche wrote:
> The purpose of patch "linux/types.h: enable endian checks for all
> sparse builds" was to encourage driver authors to annotate
> endianness correctly in their drivers. However, since that patch
> went upstream no endianness annotations in drivers have been fixed.

I doubt that's true. What's the basis for this claim?

> I think that this shows that the followed approach does not work,
> probably because several driver authors do not use sparse. For
> developers who are not the authors of these drivers it would take
> a very significant effort to make these drivers endianness clean.

I'm afraid I still don't see it.  For developers endian-ness is really
easy.  Look at hardware spec make sure code matches.  You can often do
without looking at the spec too, if a given field is always used with
cpu_to_le, mark it __le.  If you don't want to change driver code, you
don't really need to run sparse on it.

> Examples are drivers/scsi/qla2xxx and drivers/infiniband/hw/nes.

These seem to be actively maintained. So post a patch, maintainers
can look at the spec to help make sure annotations are right.

> Hence restore the ability to disable sparse endianness checks such
> that it becomes again easy to review other sparse diagnostics for
> people who want to analyze drivers they are not the author of.

What are these diagnostics that are important to analyze for people
unable to make even trivial driver changes? White-listing these
as opposed to black-listing endian-ness might be a better idea.

> References: commit 05de97003c77 ("linux/types.h: enable endian checks for all 
> sparse builds")
> Signed-off-by: Bart Van Assche <bart.vanass...@wdc.com>
> Cc: linux-scsi@vger.kernel.org
> Cc: linux-r...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Cc: Michael S. Tsirkin <m...@redhat.com>
> Cc: Christoph Hellwig <h...@infradead.org>
> Cc: Leon Romanovsky <l...@kernel.org>
> Cc: Andrew Morton <a...@linux-foundation.org>
> ---
> [v2]: Elaborated patch description
> 
>  include/uapi/linux/types.h | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/uapi/linux/types.h b/include/uapi/linux/types.h
> index 41e5914f0a8e..d3dcb0764c45 100644
> --- a/include/uapi/linux/types.h
> +++ b/include/uapi/linux/types.h
> @@ -23,7 +23,11 @@
>  #else
>  #define __bitwise__
>  #endif
> +#if !defined(__CHECK_ENDIAN__) || __CHECK_ENDIAN__ != 0
>  #define __bitwise __bitwise__
> +#else
> +#define __bitwise
> +#endif
>  
>  typedef __u16 __bitwise __le16;
>  typedef __u16 __bitwise __be16;
> -- 
> 2.14.2

Re: [PATCH] linux/types.h: Restore the ability to disable sparse endianness checks

[Michael S. Tsirkin]

On Mon, Oct 16, 2017 at 03:36:50PM +, Bart Van Assche wrote:
> On Mon, 2017-10-16 at 18:27 +0300, Michael S. Tsirkin wrote:
> > On Mon, Oct 16, 2017 at 01:57:35PM +, Bart Van Assche wrote:
> > > On Mon, 2017-10-16 at 16:34 +0300, Michael S. Tsirkin wrote:
> > > > I don't see how it'll help make things better. OTOH if the specific
> > > > drivers are tagged in the makefile, they can be gradually moved out to
> > > > staging or something to help trigger action.
> > > 
> > > Do you really want to move drivers like qla2xxx to staging? That driver is
> > > important to multiple enterprise distro's.
> > 
> > Frankly I'm surprised this one has sparse issues.
> > Really e.g. drivers/scsi/qla2xxx/qla_nvme.h is new from June 2017.
> > 
> > It's not some ancient piece of code that no one understands so
> > we are afraid to touch it.
> 
> Sorry if I wasn't clear enough but I wasn't referring to the qla2xxx NVMe
> code. I was referring to the qla2xxx FC initiator code. I think that code
> went upstream in January 2004. See also 
> https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/log/drivers/scsi/qla2xxx?ofs=100
> 
> Bart.

Right but qla_nvme also triggers these warnings. That's the problem with
disabling them tree-wide. To me it looks like the time we are spending
arguing about work-arounds would be better spent just fixing the
majority of the code. If a couple of places aren't clean and
need more thought, that's not a big deal.

-- 
MST

Re: [PATCH] linux/types.h: Restore the ability to disable sparse endianness checks

[Michael S. Tsirkin]

On Mon, Oct 16, 2017 at 01:57:35PM +, Bart Van Assche wrote:
> On Mon, 2017-10-16 at 16:34 +0300, Michael S. Tsirkin wrote:
> > I don't see how it'll help make things better. OTOH if the specific
> > drivers are tagged in the makefile, they can be gradually moved out to
> > staging or something to help trigger action.
> 
> Do you really want to move drivers like qla2xxx to staging? That driver is
> important to multiple enterprise distro's.
> 
> Bart.

Frankly I'm surprised this one has sparse issues.
Really e.g. drivers/scsi/qla2xxx/qla_nvme.h is new from June 2017.

It's not some ancient piece of code that no one understands so
we are afraid to touch it.

So if you care, why don't you just fix it up?  I suspect it's a question
of just tagging structure fields properly.

Here's a patch fixing up one of the files in that driver.

--->

qla2xxx: make qla_nvme sparse clean

Without looking into what it actually does, just add annotations so
sparse does not complain. Separately, the handle field in cmd_nvme which
isn't tagged as LE should be examined for endian-ness by someone who
understands this hardware.  If it actually needs to be native endian, a
comment explaining why would be a good idea.

Similarly for cur_dsd which seems to mix LE and native data.

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

diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c
index f3710a7..05ab549 100644
--- a/drivers/scsi/qla2xxx/qla_nvme.c
+++ b/drivers/scsi/qla2xxx/qla_nvme.c
@@ -442,7 +442,7 @@ static int qla2x00_start_nvme_mq(srb_t *sp)
req->ring_ptr++;
}
cont_pkt = (cont_a64_entry_t *)req->ring_ptr;
-   *((uint32_t *)(_pkt->entry_type)) =
+   *((__le32 *)(_pkt->entry_type)) =
cpu_to_le32(CONTINUE_A64_TYPE);
 
cur_dsd = (uint32_t *)cont_pkt->dseg_0_address;
@@ -450,9 +450,9 @@ static int qla2x00_start_nvme_mq(srb_t *sp)
}
 
sle_dma = sg_dma_address(sg);
-   *cur_dsd++ = cpu_to_le32(LSD(sle_dma));
-   *cur_dsd++ = cpu_to_le32(MSD(sle_dma));
-   *cur_dsd++ = cpu_to_le32(sg_dma_len(sg));
+   *(__le32 __force *)cur_dsd++ = cpu_to_le32(LSD(sle_dma));
+   *(__le32 __force *)cur_dsd++ = cpu_to_le32(MSD(sle_dma));
+   *(__le32 __force *)cur_dsd++ = cpu_to_le32(sg_dma_len(sg));
avail_dsds--;
}
 
diff --git a/drivers/scsi/qla2xxx/qla_nvme.h b/drivers/scsi/qla2xxx/qla_nvme.h
index dfe56f2..da58bd1 100644
--- a/drivers/scsi/qla2xxx/qla_nvme.h
+++ b/drivers/scsi/qla2xxx/qla_nvme.h
@@ -39,32 +39,32 @@ struct cmd_nvme {
uint8_t entry_status;   /* Entry Status. */
 
uint32_t handle;/* System handle. */
-   uint16_t nport_handle;  /* N_PORT handle. */
-   uint16_t timeout;   /* Command timeout. */
+   __le16 nport_handle;  /* N_PORT handle. */
+   __le16 timeout;   /* Command timeout. */
 
-   uint16_t dseg_count;/* Data segment count. */
-   uint16_t nvme_rsp_dsd_len;  /* NVMe RSP DSD length */
+   __le16 dseg_count;/* Data segment count. */
+   __le16 nvme_rsp_dsd_len;  /* NVMe RSP DSD length */
 
uint64_t rsvd;
 
-   uint16_t control_flags; /* Control Flags */
+   __le16 control_flags; /* Control Flags */
 #define CF_NVME_ENABLE  BIT_9
 #define CF_DIF_SEG_DESCR_ENABLE BIT_3
 #define CF_DATA_SEG_DESCR_ENABLEBIT_2
 #define CF_READ_DATABIT_1
 #define CF_WRITE_DATA   BIT_0
 
-   uint16_t nvme_cmnd_dseg_len; /* Data segment length. */
-   uint32_t nvme_cmnd_dseg_address[2];  /* Data segment address. */
-   uint32_t nvme_rsp_dseg_address[2];   /* Data segment address. */
+   __le16 nvme_cmnd_dseg_len; /* Data segment length. */
+   __le32 nvme_cmnd_dseg_address[2];  /* Data segment address. */
+   __le32 nvme_rsp_dseg_address[2];   /* Data segment address. */
 
-   uint32_t byte_count;/* Total byte count. */
+   __le32 byte_count;/* Total byte count. */
 
uint8_t port_id[3]; /* PortID of destination port. */
uint8_t vp_index;
 
-   uint32_t nvme_data_dseg_address[2];  /* Data segment address. */
-   uint32_t nvme_data_dseg_len; /* Data segment length. */
+   __le32 nvme_data_dseg_address[2];  /* Data segment address. */
+   __le32 nvme_data_dseg_len; /* Data segment length. */
 };
 
 #define PT_LS4_REQUEST 0x89/* Link Service pass-through IOCB (request) */
-- 
MST

Re: [RFC] apparently broken error recovery in vhost_scsi_iov_to_sgl()

[Michael S. Tsirkin]

On Sun, Sep 24, 2017 at 11:36:33PM +0100, Al Viro wrote:
> Suppose vhost_scsi_iov_to_sgl() got a two-iovec array, mapped
> e.g. 20 pages from the first one just fine and failed on the
> second.
> 
> static int
> vhost_scsi_iov_to_sgl(struct vhost_scsi_cmd *cmd, bool write,
>   struct iov_iter *iter,
>   struct scatterlist *sg, int sg_count)
> {
> size_t off = iter->iov_offset;
> int i, ret;
> 
> for (i = 0; i < iter->nr_segs; i++) {
> void __user *base = iter->iov[i].iov_base + off;
> size_t len = iter->iov[i].iov_len - off;
> 
> ret = vhost_scsi_map_to_sgl(cmd, base, len, sg, write);
> if (ret < 0) {
> for (i = 0; i < sg_count; i++) {
> struct page *page = sg_page([i]);
> if (page)
> put_page(page);
> }
> return ret;
> }
> sg += ret;
> off = 0;
> }
> return 0;
> }
> 
> What are we trying to drop in the if (ret < 0) in there?  In the case
> above we step into it on the second pass through the loop.  The first
> 20 entries of sg had been filled... and sg had been increased by 20,
> so whatever we find and feed to put_page(), it won't be those 20 pages.
> Moreover, the caller will reset cmd->tvc_{prot_,}sgl_count to zero,
> so vhost_scsi_release_cmd() won't find them either.
>
> Am I missing something subtle here, or should that thing be doing
> something like

Looks right to me. I think Nicholas wrote this, CC him.

> 
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 046f6d280af5..e47c5bc3ddca 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -688,6 +688,7 @@ vhost_scsi_iov_to_sgl(struct vhost_scsi_cmd *cmd, bool 
> write,
> struct scatterlist *sg, int sg_count)
>  {
>   size_t off = iter->iov_offset;
> + struct scatterlist *p = sg;
>   int i, ret;
>  
>   for (i = 0; i < iter->nr_segs; i++) {
> @@ -696,8 +697,8 @@ vhost_scsi_iov_to_sgl(struct vhost_scsi_cmd *cmd, bool 
> write,
>  
>   ret = vhost_scsi_map_to_sgl(cmd, base, len, sg, write);
>   if (ret < 0) {
> - for (i = 0; i < sg_count; i++) {
> - struct page *page = sg_page([i]);
> + while (p < sg) {
> + struct page *page = sg_page(p++);
>   if (page)
>   put_page(page);
>   }

Re: [PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.

[Michael S. Tsirkin]

On Fri, Aug 11, 2017 at 04:09:26PM +0200, Paolo Bonzini wrote:
> On 10/08/2017 23:41, Michael S. Tsirkin wrote:
> >>> Then we probably should fail probe if vq size is too small.
> >> What does this mean?
> > 
> > We must prevent driver from submitting s/g lists > vq size to device.
> 
> What is the rationale for the limit?

So the host knows what it needs to support.

>  It makes no sense if indirect
> descriptors are available, especially because...
> 
> > Either tell linux to avoid s/g lists that are too long, or
> > simply fail request if this happens, or refuse to attach driver to device.
> > 
> > Later option would look something like this within probe:
> > 
> > for (i = VIRTIO_SCSI_VQ_BASE; i < num_vqs; i++)
> > if (vqs[i]->num < MAX_SG_USED_BY_LINUX)
> > goto err;
> > 
> > 
> > I don't know what's MAX_SG_USED_BY_LINUX though.
> > 
> 
> ... both virtio-blk and virtio-scsi transmit their own value for the
> maximum sg list size (max_seg in virtio-scsi, seg_max in virtio-blk).
> 
> Paolo

No other device has it, and it seemed like a good idea to
limit it generally at the time.

we can fix the spec to relax the requirement for blk and scsi -
want to submit a proposal? Alternatively, add a generic field
for that.

For a quick fix, make sure vq size is >= max sg.

-- 
MST

Re: [PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.

[Michael S. Tsirkin]

On Thu, Aug 10, 2017 at 10:35:11PM +0100, Richard W.M. Jones wrote:
> On Fri, Aug 11, 2017 at 12:31:44AM +0300, Michael S. Tsirkin wrote:
> > Then we probably should fail probe if vq size is too small.
> 
> What does this mean?
> 
> Rich.

We must prevent driver from submitting s/g lists > vq size to device.


Either tell linux to avoid s/g lists that are too long, or
simply fail request if this happens, or refuse to attach driver to device.

Later option would look something like this within probe:

for (i = VIRTIO_SCSI_VQ_BASE; i < num_vqs; i++)
if (vqs[i]->num < MAX_SG_USED_BY_LINUX)
goto err;


I don't know what's MAX_SG_USED_BY_LINUX though.

> -- 
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-builder quickly builds VMs from scratch
> http://libguestfs.org/virt-builder.1.html

Re: [PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.

[Michael S. Tsirkin]

On Thu, Aug 10, 2017 at 10:30:38PM +0100, Richard W.M. Jones wrote:
> On Fri, Aug 11, 2017 at 12:21:16AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Aug 10, 2017 at 05:40:34PM +0100, Richard W.M. Jones wrote:
> > > If using indirect descriptors, you can make the total_sg as large as
> > > you want.
> > 
> > That would be a spec violation though, even if it happens to
> > work on current QEMU.
> > 
> > The spec says:
> > A driver MUST NOT create a descriptor chain longer than the Queue Size 
> > of the device.
> > 
> > What prompted this patch?
> > Do we ever encounter this situation?
> 
> This patch is needed because the following (2/2) patch will trigger
> that BUG_ON if I set virtqueue_size=64 or any smaller value.
> 
> The precise backtrace is below.
> 
> Rich.
> 
> [4.029510] [ cut here ]
> [4.030127] kernel BUG at drivers/virtio/virtio_ring.c:299!
> [4.030834] invalid opcode:  [#1] SMP
> [4.031340] Modules linked in: libcrc32c crc8 crc7 crc4 crc_itu_t 
> virtio_pci virtio_mmio virtio_input virtio_balloon virtio_scsi nd_pmem nd_btt 
> virtio_net virtio_crypto crypto_engine virtio_console virtio_rng virtio_blk 
> virtio_ring virtio nfit crc32_generic crct10dif_pclmul crc32c_intel 
> crc32_pclmul
> [4.034606] CPU: 0 PID: 1 Comm: init Not tainted 4.13.0-rc4+ #100
> [4.035354] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
> rel-1.10.2-0-g5f4c7b1-prebuilt.qemu-project.org 04/01/2014
> [4.036770] task: 9a859e243300 task.stack: a731c00cc000
> [4.037506] RIP: 0010:virtqueue_add_sgs+0x23d/0x460 [virtio_ring]
> [4.038250] RSP: :a731c00cf6e0 EFLAGS: 00010097
> [4.038898] RAX:  RBX: 0011 RCX: 
> dd0680646c40
> [4.039762] RDX:  RSI: a731c00cf7b8 RDI: 
> 9a85945c4d68
> [4.040634] RBP: a731c00cf748 R08: 9a85945c4a78 R09: 
> 01080020
> [4.041508] R10: a731c00cf788 R11: 9a859b3d3120 R12: 
> a731c00cf7d0
> [4.042382] R13: a731c00cf7d0 R14: 0001 R15: 
> 9a859b3f8200
> [4.043248] FS:  () GS:9a859e60() 
> knlGS:
> [4.044232] CS:  0010 DS:  ES:  CR0: 80050033
> [4.044942] CR2: 7fcff02e931c CR3: 1d23b000 CR4: 
> 003406f0
> [4.045815] DR0:  DR1:  DR2: 
> 
> [4.046684] DR3:  DR6: fffe0ff0 DR7: 
> 0400
> [4.047559] Call Trace:
> [4.047876]  virtscsi_add_cmd+0x1c9/0x280 [virtio_scsi]
> [4.048528]  virtscsi_kick_cmd+0x38/0x90 [virtio_scsi]
> [4.049161]  virtscsi_queuecommand+0x104/0x280 [virtio_scsi]
> [4.049875]  virtscsi_queuecommand_single+0x38/0x40 [virtio_scsi]
> [4.050628]  scsi_dispatch_cmd+0xf9/0x390
> [4.051128]  scsi_queue_rq+0x5e5/0x6f0
> [4.051602]  blk_mq_dispatch_rq_list+0x1ff/0x3c0
> [4.052175]  blk_mq_sched_dispatch_requests+0x199/0x1f0
> [4.052824]  __blk_mq_run_hw_queue+0x11d/0x1b0
> [4.053382]  __blk_mq_delay_run_hw_queue+0x8d/0xa0
> [4.053972]  blk_mq_run_hw_queue+0x14/0x20
> [4.054485]  blk_mq_sched_insert_requests+0x96/0x120
> [4.055095]  blk_mq_flush_plug_list+0x19d/0x410
> [4.055661]  blk_flush_plug_list+0xf9/0x2b0
> [4.056182]  blk_finish_plug+0x2c/0x40
> [4.056655]  __do_page_cache_readahead+0x32e/0x400
> [4.057261]  filemap_fault+0x2fb/0x890
> [4.057731]  ? filemap_fault+0x2fb/0x890
> [4.058220]  ? find_held_lock+0x3c/0xb0
> [4.058714]  ext4_filemap_fault+0x34/0x50
> [4.059212]  __do_fault+0x1e/0x110
> [4.059644]  __handle_mm_fault+0x6b2/0x1080
> [4.060167]  handle_mm_fault+0x178/0x350
> [4.060662]  __do_page_fault+0x26e/0x510
> [4.061152]  trace_do_page_fault+0x9d/0x290
> [4.061677]  do_async_page_fault+0x51/0xa0
> [4.062189]  async_page_fault+0x28/0x30
> [4.062667] RIP: 0033:0x7fcff030a24f
> [4.063113] RSP: 002b:7ffefc2ad078 EFLAGS: 00010206
> [4.063760] RAX: 7fcff02e931c RBX: 7fcff050f660 RCX: 
> 7fcff02e935c
> [4.064648] RDX: 0664 RSI:  RDI: 
> 7fcff02e931c
> [4.065519] RBP: 7ffefc2ad320 R08: 7fcff02e931c R09: 
> 00027000
> [4.066392] R10: 7fcff02e9980 R11: 0206 R12: 
> 7ffefc2ad0b0
> [4.067263] R13: 7ffefc2ad408 R14: 0002 R15: 
> 87f0
> [4.068135] Code: af 01 c7 45 c8 01 00 00 00 45 31 ed e9 9b fe ff ff 31 db 
> 48 83 7d d0 00 0f 85 3f fe ff ff 0f 0b 48 8b 7d b8 e8 e5 fd 22 de eb 8b <0f> 
> 0b 0f 0b 44

Re: [PATCH 1/2] virtio: Reduce BUG if total_sg > virtqueue size to WARN.

[Michael S. Tsirkin]

On Thu, Aug 10, 2017 at 05:40:34PM +0100, Richard W.M. Jones wrote:
> If using indirect descriptors, you can make the total_sg as large as
> you want.

That would be a spec violation though, even if it happens to
work on current QEMU.

The spec says:
A driver MUST NOT create a descriptor chain longer than the Queue Size 
of the device.

What prompted this patch?  Do we ever encounter this situation?

>  If not, BUG is too serious because the function later
> returns -ENOSPC.
> 
> Thanks Paolo Bonzini, Christoph Hellwig.
> 
> Signed-off-by: Richard W.M. Jones 
> ---
>  drivers/virtio/virtio_ring.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 5e1b548828e6..27cbc1eab868 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -296,7 +296,6 @@ static inline int virtqueue_add(struct virtqueue *_vq,
>   }
>  #endif
>  
> - BUG_ON(total_sg > vq->vring.num);
>   BUG_ON(total_sg == 0);
>  
>   head = vq->free_head;
> @@ -305,8 +304,10 @@ static inline int virtqueue_add(struct virtqueue *_vq,
>* buffers, then go indirect. FIXME: tune this threshold */
>   if (vq->indirect && total_sg > 1 && vq->vq.num_free)
>   desc = alloc_indirect(_vq, total_sg, gfp);
> - else
> + else {
>   desc = NULL;
> + WARN_ON_ONCE(total_sg > vq->vring.num && !vq->indirect);
> + }
>  
>   if (desc) {
>   /* Use a single buffer which doesn't continue */
> -- 
> 2.13.1

Re: [PATCH 1/6] virtio: wrap find_vqs

[Michael S. Tsirkin]

On Fri, Mar 31, 2017 at 12:04:55PM +0800, Jason Wang wrote:
> 
> 
> On 2017年03月30日 22:32, Michael S. Tsirkin wrote:
> > On Thu, Mar 30, 2017 at 02:00:08PM +0800, Jason Wang wrote:
> > > 
> > > On 2017年03月30日 04:48, Michael S. Tsirkin wrote:
> > > > We are going to add more parameters to find_vqs, let's wrap the call so
> > > > we don't need to tweak all drivers every time.
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin<m...@redhat.com>
> > > > ---
> > > A quick glance and it looks ok, but what the benefit of this series, is it
> > > required by other changes?
> > > 
> > > Thanks
> > Yes - to avoid touching all devices when doing the rest of
> > the patchset.
> 
> Maybe I'm not clear. I mean the benefit of this series not this single
> patch. I guess it may be used by you proposal that avoid reset when set XDP?

In particular, yes. It generally simplifies things significantly if
we can get the true buffer size back.

> If yes, do we really want to drop some packets after XDP is set?
> 
> Thanks

We would rather not drop packets. We could detect and copy them to make
XDP work.

-- 
MST

Re: [PATCH 1/6] virtio: wrap find_vqs

[Michael S. Tsirkin]

On Thu, Mar 30, 2017 at 02:00:08PM +0800, Jason Wang wrote:
> 
> 
> On 2017年03月30日 04:48, Michael S. Tsirkin wrote:
> > We are going to add more parameters to find_vqs, let's wrap the call so
> > we don't need to tweak all drivers every time.
> > 
> > Signed-off-by: Michael S. Tsirkin<m...@redhat.com>
> > ---
> 
> A quick glance and it looks ok, but what the benefit of this series, is it
> required by other changes?
> 
> Thanks

Yes - to avoid touching all devices when doing the rest of
the patchset.

[PATCH 1/6] virtio: wrap find_vqs

[Michael S. Tsirkin]

We are going to add more parameters to find_vqs, let's wrap the call so
we don't need to tweak all drivers every time.

Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
---
 drivers/block/virtio_blk.c | 3 +--
 drivers/char/virtio_console.c  | 6 +++---
 drivers/crypto/virtio/virtio_crypto_core.c | 3 +--
 drivers/gpu/drm/virtio/virtgpu_kms.c   | 3 +--
 drivers/net/caif/caif_virtio.c | 3 +--
 drivers/net/virtio_net.c   | 3 +--
 drivers/rpmsg/virtio_rpmsg_bus.c   | 2 +-
 drivers/scsi/virtio_scsi.c | 3 +--
 drivers/virtio/virtio_balloon.c| 3 +--
 drivers/virtio/virtio_input.c  | 3 +--
 include/linux/virtio_config.h  | 9 +
 net/vmw_vsock/virtio_transport.c   | 6 +++---
 12 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 1d4c9f8..c08c30c 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -455,8 +455,7 @@ static int init_vq(struct virtio_blk *vblk)
}
 
/* Discover virtqueues and write information to configuration.  */
-   err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names,
-   );
+   err = virtio_find_vqs(vdev, num_vqs, vqs, callbacks, names, );
if (err)
goto out;
 
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index e9b7e0b..5da4c8e 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1945,9 +1945,9 @@ static int init_vqs(struct ports_device *portdev)
}
}
/* Find the queues. */
-   err = portdev->vdev->config->find_vqs(portdev->vdev, nr_queues, vqs,
- io_callbacks,
- (const char **)io_names, NULL);
+   err = virtio_find_vqs(portdev->vdev, nr_queues, vqs,
+ io_callbacks,
+ (const char **)io_names, NULL);
if (err)
goto free;
 
diff --git a/drivers/crypto/virtio/virtio_crypto_core.c 
b/drivers/crypto/virtio/virtio_crypto_core.c
index 21472e4..a111cd72 100644
--- a/drivers/crypto/virtio/virtio_crypto_core.c
+++ b/drivers/crypto/virtio/virtio_crypto_core.c
@@ -119,8 +119,7 @@ static int virtcrypto_find_vqs(struct virtio_crypto *vi)
names[i] = vi->data_vq[i].name;
}
 
-   ret = vi->vdev->config->find_vqs(vi->vdev, total_vqs, vqs, callbacks,
-names, NULL);
+   ret = virtio_find_vqs(vi->vdev, total_vqs, vqs, callbacks, names, NULL);
if (ret)
goto err_find;
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c 
b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 4918668..1e1c90b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -175,8 +175,7 @@ int virtio_gpu_driver_load(struct drm_device *dev, unsigned 
long flags)
DRM_INFO("virgl 3d acceleration not supported by guest\n");
 #endif
 
-   ret = vgdev->vdev->config->find_vqs(vgdev->vdev, 2, vqs,
-   callbacks, names, NULL);
+   ret = virtio_find_vqs(vgdev->vdev, 2, vqs, callbacks, names, NULL);
if (ret) {
DRM_ERROR("failed to find virt queues\n");
goto err_vqs;
diff --git a/drivers/net/caif/caif_virtio.c b/drivers/net/caif/caif_virtio.c
index bc0eb47..6122768 100644
--- a/drivers/net/caif/caif_virtio.c
+++ b/drivers/net/caif/caif_virtio.c
@@ -679,8 +679,7 @@ static int cfv_probe(struct virtio_device *vdev)
goto err;
 
/* Get the TX virtio ring. This is a "guest side vring". */
-   err = vdev->config->find_vqs(vdev, 1, >vq_tx, _cbs, ,
-   NULL);
+   err = virtio_find_vqs(vdev, 1, >vq_tx, _cbs, , NULL);
if (err)
goto err;
 
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ea9890d..6802169 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2079,8 +2079,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
names[txq2vq(i)] = vi->sq[i].name;
}
 
-   ret = vi->vdev->config->find_vqs(vi->vdev, total_vqs, vqs, callbacks,
-names, NULL);
+   ret = virtio_find_vqs(vi->vdev, total_vqs, vqs, callbacks, names, NULL);
if (ret)
goto err_find;
 
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 5e66e08..f7cade0 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -869,7 +869,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
init_waitqueue_head(>sendq);
 

Re: [PATCH v2 2/2] virtio_scsi: Implement fc_host

[Michael S. Tsirkin]

On Thu, Jan 26, 2017 at 11:41:09AM +0800, Fam Zheng wrote:
> This implements the VIRTIO_SCSI_F_FC_HOST feature by reading the config
> fields and presenting them as sysfs fc_host attributes. The config
> change handler is added here because primary_active will toggle during
> migration.

Looks like there's active discussion on virtio tc mailing list.
It's ok to post patches meanwhile but best as RFC,
and repost after controversy is resolved.

> 
> Signed-off-by: Fam Zheng 
> ---
>  drivers/scsi/virtio_scsi.c | 60 
> +-
>  1 file changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index ec91bd0..1bb330c 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #define VIRTIO_SCSI_MEMPOOL_SZ 64
> @@ -795,6 +796,15 @@ static struct scsi_host_template 
> virtscsi_host_template_multi = {
>   .track_queue_depth = 1,
>  };
>  
> +static struct fc_function_template virtscsi_fc_template = {
> + .show_host_node_name = 1,
> + .show_host_port_name = 1,
> + .show_host_port_type = 1,
> + .show_host_port_state = 1,
> +};
> +
> +static struct scsi_transport_template *virtscsi_fc_transport_template;
> +
>  #define virtscsi_config_get(vdev, fld) \
>   ({ \
>   typeof(((struct virtio_scsi_config *)0)->fld) __val; \
> @@ -956,15 +966,42 @@ static int virtscsi_init(struct virtio_device *vdev,
>   return err;
>  }
>  
> +static void virtscsi_update_fc_host_attrs(struct virtio_device *vdev)
> +{
> + struct Scsi_Host *shost = vdev->priv;
> + u8 node_name[8], port_name[8];
> +
> + if (virtscsi_config_get(vdev, primary_active)) {
> + virtio_cread_bytes(vdev,
> + offsetof(struct virtio_scsi_config, primary_wwnn),
> + _name, 8);
> + virtio_cread_bytes(vdev,
> + offsetof(struct virtio_scsi_config, primary_wwpn),
> + _name, 8);
> + } else {
> + virtio_cread_bytes(vdev,
> + offsetof(struct virtio_scsi_config, secondary_wwnn),
> + _name, 8);
> + virtio_cread_bytes(vdev,
> + offsetof(struct virtio_scsi_config, secondary_wwpn),
> + _name, 8);
> + }

This is racy, isn't it? You need to wrap this in a generation check
otherwise read can race with primary_active changing.
And you might want a wrapper to virtio_cread_bytes that does not
include generation check.

> + fc_host_node_name(shost) = wwn_to_u64(node_name);
> + fc_host_port_name(shost) = wwn_to_u64(port_name);
> + fc_host_port_type(shost) = FC_PORTTYPE_NPORT;
> + fc_host_port_state(shost) = FC_PORTSTATE_ONLINE;
> +}
> +
>  static int virtscsi_probe(struct virtio_device *vdev)
>  {
> - struct Scsi_Host *shost;
> + struct Scsi_Host *shost = NULL;
>   struct virtio_scsi *vscsi;
>   int err;
>   u32 sg_elems, num_targets;
>   u32 cmd_per_lun;
>   u32 num_queues;
>   struct scsi_host_template *hostt;
> + bool fc_host_enabled;
>  
>   if (!vdev->config->get) {
>   dev_err(>dev, "%s failure: config access disabled\n",
> @@ -987,6 +1024,9 @@ static int virtscsi_probe(struct virtio_device *vdev)
>   if (!shost)
>   return -ENOMEM;
>  
> + fc_host_enabled = virtio_has_feature(vdev, VIRTIO_SCSI_F_FC_HOST);
> + if (fc_host_enabled)
> + shost->transportt = virtscsi_fc_transport_template;
>   sg_elems = virtscsi_config_get(vdev, seg_max) ?: 1;
>   shost->sg_tablesize = sg_elems;
>   vscsi = shost_priv(shost);
> @@ -1032,6 +1072,9 @@ static int virtscsi_probe(struct virtio_device *vdev)
>   if (err)
>   goto scsi_add_host_failed;
>  
> + if (fc_host_enabled)
> + virtscsi_update_fc_host_attrs(vdev);
> +
>   virtio_device_ready(vdev);
>  
>   if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
> @@ -1098,6 +1141,11 @@ static int virtscsi_restore(struct virtio_device *vdev)
>  }
>  #endif
>  
> +static void virtscsi_config_changed(struct virtio_device *vdev)
> +{

This is called unconditionally here, will access
invalid config fields if feature is off.
In fact this is wasting an MSIX vector when feature is not
negotiated. No easy way not to, but best document this
in a code comment.

> + virtscsi_update_fc_host_attrs(vdev);
> +}
> +
>  static struct virtio_device_id id_table[] = {
>   { VIRTIO_ID_SCSI, VIRTIO_DEV_ANY_ID },
>   { 0 },
> @@ -1109,6 +1157,7 @@ static unsigned int features[] = {
>  #ifdef CONFIG_BLK_DEV_INTEGRITY
>   VIRTIO_SCSI_F_T10_PI,
>  #endif
> + VIRTIO_SCSI_F_FC_HOST,
>  };
>  
>  static struct virtio_driver virtio_scsi_driver = {
> @@ -1123,12 +1172,20 @@ static struct virtio_driver virtio_scsi_driver = {
>  

Re: [PATCH v2 1/2] virtio_scsi: Add fc_host definitions

[Michael S. Tsirkin]

On Thu, Jan 26, 2017 at 11:41:08AM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng 
> ---

I pefer combining this with implementation,
hard to reason about interface alone.

>  include/uapi/linux/virtio_scsi.h | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/uapi/linux/virtio_scsi.h 
> b/include/uapi/linux/virtio_scsi.h
> index cc18ef8..a26fb31 100644
> --- a/include/uapi/linux/virtio_scsi.h
> +++ b/include/uapi/linux/virtio_scsi.h
> @@ -113,6 +113,11 @@ struct virtio_scsi_config {
>   __u16 max_channel;
>   __u16 max_target;
>   __u32 max_lun;
> + __u8  primary_wwpn[8];
> + __u8  primary_wwnn[8];
> + __u8  secondary_wwpn[8];
> + __u8  secondary_wwnn[8];
> + __u8  primary_active;

Is this in fact a binary value?
Also pls add padding to align on 8 byte boundary.

>  } __attribute__((packed));
>  
>  /* Feature Bits */
> @@ -120,6 +125,7 @@ struct virtio_scsi_config {
>  #define VIRTIO_SCSI_F_HOTPLUG  1
>  #define VIRTIO_SCSI_F_CHANGE   2
>  #define VIRTIO_SCSI_F_T10_PI   3
> +#define VIRTIO_SCSI_F_FC_HOST  4
>  
>  /* Response codes */
>  #define VIRTIO_SCSI_S_OK   0
> -- 
> 2.9.3
--
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 2/2] virtio_scsi: Implement fc_host

[Michael S. Tsirkin]

On Tue, Jan 17, 2017 at 10:05:00PM +0800, Fam Zheng wrote:
> On Tue, 01/17 14:17, Paolo Bonzini wrote:
> > 
> > 
> > On 16/01/2017 18:26, Fam Zheng wrote:
> > >> Is the endianness correct for big-endian host here?
> > >
> > > I think so. The fc_host sysfs uses u64 to represent port_name and 
> > > node_name,
> > > this patch does the same, so using virtio_* helpers for these fields 
> > > should
> > > handle the endianness correctly.
> > 
> > I was suspicious about it because they are defined as "u8 x[8]" in the
> > virtio_scsi_config struct.  So you would need to read with
> > virtio_cread_bytes and pass the result to wwn_to_u64.
> > 
> > For example, if you have 0x500123456789abcd this would be
> > 
> > 0x50 0x01 0x23 0x45 0x67 0x89 0xab 0cd
> > 
> > in virtio_scsi_config, and then virtio_cread64 would read it as a
> > little-endian u64, 0xcdab896745230150.  Maybe your QEMU patch is also
> > writing things as little-endian 64-bit integers, rather than 8-element
> > arrays of bytes?
> 
> Yes, they all used 64-bit integers in a "less surprising" endian. I think 
> there
> is an endianness conecpt to WWN, as in 0x500123456789abcd; and there is an
> native endianness to virtio, which is little-endian. If we use a "u8 x[8]" 
> type
> in the spec and want the WWN's MSB, namely the 0x50 stuff, to be the first 
> byte,
> is it worth to explicitly document that to avoid confusion?
> 
> Fam

Can't hurt, for sure.

--
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 0/8] enable endian checks for all sparse builds

[Michael S. Tsirkin]

This is just a reposting of the patch that enables endian checks, with addition
of trivial patches that drop __bitwise__ and __CHECK_ENDIAN__ everywhere.

I plan to include this in my pull request unless I hear otherwise.

Michael S. Tsirkin (8):
  linux/types.h: enable endian checks for all sparse builds
  tools: enable endian checks for all sparse builds
  Documentation/sparse: drop __bitwise__
  checkpatch: replace __bitwise__ with __bitwise
  linux: drop __bitwise__ everywhere
  Documentation/sparse: drop __CHECK_ENDIAN__
  fs/logfs: drop __CHECK_ENDIAN__
  Makefile: drop -D__CHECK_ENDIAN__ from cflags

 Documentation/translations/zh_CN/sparse.txt   |  7 +--
 arch/arm/plat-samsung/include/plat/gpio-cfg.h |  2 +-
 drivers/md/dm-cache-block-types.h |  6 +++---
 drivers/net/ethernet/sun/sunhme.h |  2 +-
 drivers/net/wireless/intel/iwlwifi/iwl-fw-file.h  |  4 ++--
 fs/logfs/logfs.h  |  4 +---
 include/linux/mmzone.h|  2 +-
 include/linux/serial_core.h   |  4 ++--
 include/linux/types.h |  4 ++--
 include/scsi/iscsi_proto.h|  2 +-
 include/target/target_core_base.h |  2 +-
 include/uapi/linux/types.h|  4 
 include/uapi/linux/virtio_types.h |  6 +++---
 net/ieee802154/6lowpan/6lowpan_i.h|  2 +-
 net/mac80211/ieee80211_i.h|  4 ++--
 tools/include/linux/types.h   |  4 
 Documentation/dev-tools/sparse.rst| 14 +-
 drivers/bluetooth/Makefile|  2 --
 drivers/net/can/Makefile  |  1 -
 drivers/net/ethernet/altera/Makefile  |  1 -
 drivers/net/ethernet/atheros/alx/Makefile |  1 -
 drivers/net/ethernet/freescale/Makefile   |  2 --
 drivers/net/wireless/ath/Makefile |  2 --
 drivers/net/wireless/ath/wil6210/Makefile |  2 --
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile |  2 --
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/Makefile |  1 -
 drivers/net/wireless/intel/iwlegacy/Makefile  |  2 --
 drivers/net/wireless/intel/iwlwifi/Makefile   |  2 +-
 drivers/net/wireless/intel/iwlwifi/dvm/Makefile   |  2 +-
 drivers/net/wireless/intel/iwlwifi/mvm/Makefile   |  2 +-
 drivers/net/wireless/intersil/orinoco/Makefile|  3 ---
 drivers/net/wireless/mediatek/mt7601u/Makefile|  2 --
 drivers/net/wireless/realtek/rtlwifi/Makefile |  2 --
 drivers/net/wireless/realtek/rtlwifi/btcoexist/Makefile   |  2 --
 drivers/net/wireless/realtek/rtlwifi/rtl8188ee/Makefile   |  2 --
 drivers/net/wireless/realtek/rtlwifi/rtl8192c/Makefile|  2 --
 drivers/net/wireless/realtek/rtlwifi/rtl8192ce/Makefile   |  2 --
 drivers/net/wireless/realtek/rtlwifi/rtl8192cu/Makefile   |  2 --
 drivers/net/wireless/realtek/rtlwifi/rtl8192de/Makefile   |  2 --
 drivers/net/wireless/realtek/rtlwifi/rtl8192ee/Makefile   |  2 --
 drivers/net/wireless/realtek/rtlwifi/rtl8192se/Makefile   |  2 --
 drivers/net/wireless/realtek/rtlwifi/rtl8723ae/Makefile   |  2 --
 drivers/net/wireless/realtek/rtlwifi/rtl8723be/Makefile   |  2 --
 drivers/net/wireless/realtek/rtlwifi/rtl8723com/Makefile  |  2 --
 drivers/net/wireless/realtek/rtlwifi/rtl8821ae/Makefile   |  2 --
 drivers/net/wireless/ti/wl1251/Makefile   |  2 --
 drivers/net/wireless/ti/wlcore/Makefile   |  2 --
 drivers/staging/rtl8188eu/Makefile|  2 +-
 drivers/staging/rtl8192e/Makefile |  2 --
 drivers/staging/rtl8192e/rtl8192e/Makefile|  2 --
 net/bluetooth/Makefile|  2 --
 net/ieee802154/Makefile   |  2 --
 net/mac80211/Makefile |  2 +-
 net/mac802154/Makefile|  2 --
 net/wireless/Makefile |  2 --
 scripts/checkpatch.pl |  4 ++--
 56 files changed, 30 insertions(+), 120 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 5/8] linux: drop __bitwise__ everywhere

[Michael S. Tsirkin]

__bitwise__ used to mean "yes, please enable sparse checks
unconditionally", but now that we dropped __CHECK_ENDIAN__
__bitwise is exactly the same.
There aren't many users, replace it by __bitwise everywhere.

Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
---
 arch/arm/plat-samsung/include/plat/gpio-cfg.h| 2 +-
 drivers/md/dm-cache-block-types.h| 6 +++---
 drivers/net/ethernet/sun/sunhme.h| 2 +-
 drivers/net/wireless/intel/iwlwifi/iwl-fw-file.h | 4 ++--
 include/linux/mmzone.h   | 2 +-
 include/linux/serial_core.h  | 4 ++--
 include/linux/types.h| 4 ++--
 include/scsi/iscsi_proto.h   | 2 +-
 include/target/target_core_base.h| 2 +-
 include/uapi/linux/virtio_types.h| 6 +++---
 net/ieee802154/6lowpan/6lowpan_i.h   | 2 +-
 net/mac80211/ieee80211_i.h   | 4 ++--
 12 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/arch/arm/plat-samsung/include/plat/gpio-cfg.h 
b/arch/arm/plat-samsung/include/plat/gpio-cfg.h
index 21391fa..e55d1f5 100644
--- a/arch/arm/plat-samsung/include/plat/gpio-cfg.h
+++ b/arch/arm/plat-samsung/include/plat/gpio-cfg.h
@@ -26,7 +26,7 @@
 
 #include 
 
-typedef unsigned int __bitwise__ samsung_gpio_pull_t;
+typedef unsigned int __bitwise samsung_gpio_pull_t;
 
 /* forward declaration if gpio-core.h hasn't been included */
 struct samsung_gpio_chip;
diff --git a/drivers/md/dm-cache-block-types.h 
b/drivers/md/dm-cache-block-types.h
index bed4ad4..389c9e8 100644
--- a/drivers/md/dm-cache-block-types.h
+++ b/drivers/md/dm-cache-block-types.h
@@ -17,9 +17,9 @@
  * discard bitset.
  */
 
-typedef dm_block_t __bitwise__ dm_oblock_t;
-typedef uint32_t __bitwise__ dm_cblock_t;
-typedef dm_block_t __bitwise__ dm_dblock_t;
+typedef dm_block_t __bitwise dm_oblock_t;
+typedef uint32_t __bitwise dm_cblock_t;
+typedef dm_block_t __bitwise dm_dblock_t;
 
 static inline dm_oblock_t to_oblock(dm_block_t b)
 {
diff --git a/drivers/net/ethernet/sun/sunhme.h 
b/drivers/net/ethernet/sun/sunhme.h
index f430765..4a8d5b1 100644
--- a/drivers/net/ethernet/sun/sunhme.h
+++ b/drivers/net/ethernet/sun/sunhme.h
@@ -302,7 +302,7 @@
  * Always write the address first before setting the ownership
  * bits to avoid races with the hardware scanning the ring.
  */
-typedef u32 __bitwise__ hme32;
+typedef u32 __bitwise hme32;
 
 struct happy_meal_rxd {
hme32 rx_flags;
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-fw-file.h 
b/drivers/net/wireless/intel/iwlwifi/iwl-fw-file.h
index 1ad0ec1..84813b5 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-fw-file.h
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-fw-file.h
@@ -228,7 +228,7 @@ enum iwl_ucode_tlv_flag {
IWL_UCODE_TLV_FLAGS_BCAST_FILTERING = BIT(29),
 };
 
-typedef unsigned int __bitwise__ iwl_ucode_tlv_api_t;
+typedef unsigned int __bitwise iwl_ucode_tlv_api_t;
 
 /**
  * enum iwl_ucode_tlv_api - ucode api
@@ -258,7 +258,7 @@ enum iwl_ucode_tlv_api {
 #endif
 };
 
-typedef unsigned int __bitwise__ iwl_ucode_tlv_capa_t;
+typedef unsigned int __bitwise iwl_ucode_tlv_capa_t;
 
 /**
  * enum iwl_ucode_tlv_capa - ucode capabilities
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 0f088f3..36d9896 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -246,7 +246,7 @@ struct lruvec {
 #define ISOLATE_UNEVICTABLE((__force isolate_mode_t)0x8)
 
 /* LRU Isolation modes. */
-typedef unsigned __bitwise__ isolate_mode_t;
+typedef unsigned __bitwise isolate_mode_t;
 
 enum zone_watermarks {
WMARK_MIN,
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 5d49488..5def8e8 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -111,8 +111,8 @@ struct uart_icount {
__u32   buf_overrun;
 };
 
-typedef unsigned int __bitwise__ upf_t;
-typedef unsigned int __bitwise__ upstat_t;
+typedef unsigned int __bitwise upf_t;
+typedef unsigned int __bitwise upstat_t;
 
 struct uart_port {
spinlock_t  lock;   /* port lock */
diff --git a/include/linux/types.h b/include/linux/types.h
index baf7183..d501ad3 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -154,8 +154,8 @@ typedef u64 dma_addr_t;
 typedef u32 dma_addr_t;
 #endif
 
-typedef unsigned __bitwise__ gfp_t;
-typedef unsigned __bitwise__ fmode_t;
+typedef unsigned __bitwise gfp_t;
+typedef unsigned __bitwise fmode_t;
 
 #ifdef CONFIG_PHYS_ADDR_T_64BIT
 typedef u64 phys_addr_t;
diff --git a/include/scsi/iscsi_proto.h b/include/scsi/iscsi_proto.h
index c1260d8..df156f1 100644
--- a/include/scsi/iscsi_proto.h
+++ b/include/scsi/iscsi_proto.h
@@ -74,7 +74,7 @@ static inline int iscsi_sna_gte(u32 n1, u32 n2)
 #define zero_data(p) {p[0]=0;p[1]=0;p[2]=0;}
 
 /* initiator tags; opaque for target */
-typedef uint32_t __bitwise__ itt_t;
+typedef uint32_t

Re: [PATCH] scsi/qla2xxx: label endian-ness for many fields

[Michael S. Tsirkin]

On Fri, Dec 09, 2016 at 09:49:28PM -0800, Joe Perches wrote:
> On Fri, 2016-12-09 at 22:45 +0200, Michael S. Tsirkin wrote:
> > This adds endian-ness labels for lots of qla structs.
> > Doing this cuts down number of sparse warnings from ~1700 to ~1400.
> > Will help find and resolve some of real issues down the road.
> > 
> > Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> > 
> > ---
> > 
> > Compile-tested only.
> > 
> > diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> > index 73b12e4..a4d3071 100644
> > --- a/drivers/scsi/qla2xxx/qla_def.h
> > +++ b/drivers/scsi/qla2xxx/qla_def.h
> > @@ -1159,28 +1159,28 @@ typedef struct {
> >  */
> > uint8_t  firmware_options[2];
> >  
> > -   uint16_t frame_payload_size;
> > -   uint16_t max_iocb_allocation;
> > -   uint16_t execution_throttle;
> > +   __le16 frame_payload_size;
> > +   __le16 max_iocb_allocation;
> > +   __le16 execution_throttle;
> 
> Shouldn't all these _not_ have the leading __?

[linux]$ git grep le32 include/linux/|grep -v _le32|grep -v le32_
[linux]$ 


> Perhaps the uint8_t uses should be converted to u8 as well.
> 
> [etc...]

Sure.  It's up to maintainers to clean up this driver though. I merely
posted this to show that proper tagging of endian-ness is not hard.

-- 
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] linux/types.h: enable endian checks for all sparse builds

[Michael S. Tsirkin]

On Fri, Dec 09, 2016 at 03:18:02PM +, Bart Van Assche wrote:
> On 12/08/16 22:40, Madhani, Himanshu wrote:
> > We’ll take a look and send patches to resolve these warnings.
> 
> Thanks!
> 
> Bart.
> 

Sounds good. I posted what I have so far so that you can
start from that.

-- 
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] scsi/qla2xxx: label endian-ness for many fields

[Michael S. Tsirkin]

This adds endian-ness labels for lots of qla structs.
Doing this cuts down number of sparse warnings from ~1700 to ~1400.
Will help find and resolve some of real issues down the road.

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

---

Compile-tested only.

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index 73b12e4..a4d3071 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -1159,28 +1159,28 @@ typedef struct {
 */
uint8_t  firmware_options[2];
 
-   uint16_t frame_payload_size;
-   uint16_t max_iocb_allocation;
-   uint16_t execution_throttle;
+   __le16 frame_payload_size;
+   __le16 max_iocb_allocation;
+   __le16 execution_throttle;
uint8_t  retry_count;
uint8_t  retry_delay;   /* unused */
uint8_t  port_name[WWN_SIZE];   /* Big endian. */
-   uint16_t hard_address;
+   __le16 hard_address;
uint8_t  inquiry_data;
uint8_t  login_timeout;
uint8_t  node_name[WWN_SIZE];   /* Big endian. */
 
-   uint16_t request_q_outpointer;
-   uint16_t response_q_inpointer;
-   uint16_t request_q_length;
-   uint16_t response_q_length;
-   uint32_t request_q_address[2];
-   uint32_t response_q_address[2];
+   __le16 request_q_outpointer;
+   __le16 response_q_inpointer;
+   __le16 request_q_length;
+   __le16 response_q_length;
+   __le32 request_q_address[2];
+   __le32 response_q_address[2];
 
-   uint16_t lun_enables;
+   __le16 lun_enables;
uint8_t  command_resource_count;
uint8_t  immediate_notify_resource_count;
-   uint16_t timeout;
+   __le16 timeout;
uint8_t  reserved_2[2];
 
/*
@@ -1238,48 +1238,48 @@ typedef struct {
 #define GLSO_USE_DID   BIT_3
 
 struct link_statistics {
-   uint32_t link_fail_cnt;
-   uint32_t loss_sync_cnt;
-   uint32_t loss_sig_cnt;
-   uint32_t prim_seq_err_cnt;
-   uint32_t inval_xmit_word_cnt;
-   uint32_t inval_crc_cnt;
-   uint32_t lip_cnt;
-   uint32_t link_up_cnt;
-   uint32_t link_down_loop_init_tmo;
-   uint32_t link_down_los;
-   uint32_t link_down_loss_rcv_clk;
-   uint32_t reserved0[5];
-   uint32_t port_cfg_chg;
-   uint32_t reserved1[11];
-   uint32_t rsp_q_full;
-   uint32_t atio_q_full;
-   uint32_t drop_ae;
-   uint32_t els_proto_err;
-   uint32_t reserved2;
-   uint32_t tx_frames;
-   uint32_t rx_frames;
-   uint32_t discarded_frames;
-   uint32_t dropped_frames;
-   uint32_t reserved3;
-   uint32_t nos_rcvd;
-   uint32_t reserved4[4];
-   uint32_t tx_prjt;
-   uint32_t rcv_exfail;
-   uint32_t rcv_abts;
-   uint32_t seq_frm_miss;
-   uint32_t corr_err;
-   uint32_t mb_rqst;
-   uint32_t nport_full;
-   uint32_t eofa;
-   uint32_t reserved5;
-   uint32_t fpm_recv_word_cnt_lo;
-   uint32_t fpm_recv_word_cnt_hi;
-   uint32_t fpm_disc_word_cnt_lo;
-   uint32_t fpm_disc_word_cnt_hi;
-   uint32_t fpm_xmit_word_cnt_lo;
-   uint32_t fpm_xmit_word_cnt_hi;
-   uint32_t reserved6[70];
+   __le32 link_fail_cnt;
+   __le32 loss_sync_cnt;
+   __le32 loss_sig_cnt;
+   __le32 prim_seq_err_cnt;
+   __le32 inval_xmit_word_cnt;
+   __le32 inval_crc_cnt;
+   __le32 lip_cnt;
+   __le32 link_up_cnt;
+   __le32 link_down_loop_init_tmo;
+   __le32 link_down_los;
+   __le32 link_down_loss_rcv_clk;
+   __le32 reserved0[5];
+   __le32 port_cfg_chg;
+   __le32 reserved1[11];
+   __le32 rsp_q_full;
+   __le32 atio_q_full;
+   __le32 drop_ae;
+   __le32 els_proto_err;
+   __le32 reserved2;
+   __le32 tx_frames;
+   __le32 rx_frames;
+   __le32 discarded_frames;
+   __le32 dropped_frames;
+   __le32 reserved3;
+   __le32 nos_rcvd;
+   __le32 reserved4[4];
+   __le32 tx_prjt;
+   __le32 rcv_exfail;
+   __le32 rcv_abts;
+   __le32 seq_frm_miss;
+   __le32 corr_err;
+   __le32 mb_rqst;
+   __le32 nport_full;
+   __le32 eofa;
+   __le32 reserved5;
+   __le32 fpm_recv_word_cnt_lo;
+   __le32 fpm_recv_word_cnt_hi;
+   __le32 fpm_disc_word_cnt_lo;
+   __le32 fpm_disc_word_cnt_hi;
+   __le32 fpm_xmit_word_cnt_lo;
+   __le32 fpm_xmit_word_cnt_hi;
+   __le32 reserved6[70];
 };
 
 /*
@@ -1330,13 +1330,13 @@ typedef struct {
 */
uint8_t  firmware_options[2];
 
-   uint16_t frame_payload_size;
-   uint16_t max_iocb_allocation;
-   uint16_t execution_throttle;
+   __le16 frame_payload_size;
+   __le16 max_iocb_allocation;
+   __le16 execution_throttle;
uint8_t  retry_count;
uint8_t  retry_delay;   /* unused */
uint8_t  port_name[WWN_SIZE];   /* Big endian. */
-   uint16_t hard_address;
+   

Re: [PATCH] linux/types.h: enable endian checks for all sparse builds

[Michael S. Tsirkin]

On Thu, Dec 08, 2016 at 06:38:11AM +, Bart Van Assche wrote:
> On 12/07/16 21:54, Michael S. Tsirkin wrote:
> > On Thu, Dec 08, 2016 at 05:21:47AM +, Bart Van Assche wrote:
> >> Additionally, there are notable exceptions to the rule that most drivers
> >> are endian-clean, e.g. drivers/scsi/qla2xxx. I would appreciate it if it
> >> would remain possible to check such drivers with sparse without enabling
> >> endianness checks. Have you considered to change #ifdef __CHECK_ENDIAN__
> >> into e.g. #ifndef __DONT_CHECK_ENDIAN__?
> >
> > The right thing is probably just to fix these, isn't it?
> > Until then, why not just ignore the warnings?
> 
> Neither option is realistic. With endian-checking enabled the qla2xxx 
> driver triggers so many warnings that it becomes a real challenge to 
> filter the non-endian warnings out manually:
> 
> $ for f in "" CF=-D__CHECK_ENDIAN__; do make M=drivers/scsi/qla2xxx C=2\
>  $f |  -c ': warning:'; done
> 4
> 752

You can always revert this patch in your tree, or whatever.  It does not
look like this will get fixed otherwise.

> If you think it would be easy to fix the endian warnings triggered by 
> the qla2xxx driver, you are welcome to try to fix these.
> 
> Bart.

Yea, this hardware was designed by someone who thought mixing
LE and BE all over the place is a good idea.
But who said it should be easy?

Maybe this change will be enough to motivate the maintainers.

Here's a minor buglet for you as a motivator:

if (ct_rsp->header.response !=
cpu_to_be16(CT_ACCEPT_RESPONSE)) {
ql_dbg(ql_dbg_disc + ql_dbg_buffer, vha, 0x2077,
"%s failed rejected request on port_id: 
%02x%02x%02x Compeltion status 0x%x, response 0x%x\n",
routine, vha->d_id.b.domain,
vha->d_id.b.area, vha->d_id.b.al_pa, 
comp_status, ct_rsp->header.response);


response is BE and isn't printed correctly.

another:

eiter->a.max_frame_size = cpu_to_be32(eiter->a.max_frame_size);
size += 4 + 4;

ql_dbg(ql_dbg_disc, vha, 0x20bc,
"Max_Frame_Size = %x.\n", eiter->a.max_frame_size);

printed too late, it's be by that time.

Here's another suspicious line

ctio24->u.status1.flags = (atio->u.isp24.attr << 9) |
cpu_to_le16(CTIO7_FLAGS_STATUS_MODE_1 |
CTIO7_FLAGS_TERMINATE);

shifting attr by 9 bits gives different results on BE and LE,
mixing it with le16 looks rather strange.

Another:

ha->flags.dport_enabled =
(mid_init_cb->init_cb.firmware_options_1 & BIT_7) != 0;

BIT_7 is native endian, firmware_options_1 is LE I think.



Look at qla27xx_find_valid_image as well.

if (pri_image_status.signature != QLA27XX_IMG_STATUS_SIGN)

qla27xx_image_status seems to be data coming from flash, but is
somehow native-endian? Maybe ...


lun = a->u.isp24.fcp_cmnd.lun;

I think lun here is in hardware format (le?), code treats it
as native.


Not to speak about interface abuse all over the place.
How about this:

uint32_t *
qla24xx_read_flash_data(scsi_qla_host_t *vha, uint32_t *dwptr, uint32_t
faddr,
uint32_t dwords) 
{
uint32_t i; 
struct qla_hw_data *ha = vha->hw;

/* Dword reads to flash. */
for (i = 0; i < dwords; i++, faddr++)
dwptr[i] = cpu_to_le32(qla24xx_read_flash_dword(ha,
flash_data_addr(ha, faddr)));

return dwptr;   
}

OK so we convert to LE ...

qla24xx_read_flash_data(vha, dcode, faddr, 4); 

risc_addr = be32_to_cpu(dcode[2]);
*srisc_addr = *srisc_addr == 0 ? risc_addr : *srisc_addr;
risc_size = be32_to_cpu(dcode[3]);

then happily assume it's BE.

And again, coming from flash, it's unlikely to actually be in the native
endian-ness as callers seem to assume. I'm guessing it's all BE.

I poked at it a bit and was able to cut down # of warnings
from 1700 to 1400 in an hour. Someone familiar with the code
should look at 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] linux/types.h: enable endian checks for all sparse builds

[Michael S. Tsirkin]

On Thu, Dec 08, 2016 at 05:21:47AM +, Bart Van Assche wrote:
> On 12/07/16 18:29, Michael S. Tsirkin wrote:
> > By now, linux is mostly endian-clean. Enabling endian-ness
> > checks for everyone produces about 200 new sparse warnings for me -
> > less than 10% over the 2000 sparse warnings already there.
> >
> > Not a big deal, OTOH enabling this helps people notice
> > they are introducing new bugs.
> >
> > So let's just drop __CHECK_ENDIAN__. Follow-up patches
> > can drop distinction between __bitwise and __bitwise__.
> 
> Hello Michael,
> 
> This patch makes a whole bunch of ccflags-y += -D__CHECK_ENDIAN__ 
> statements obsolete. Have you considered to remove these statements?

Absolutely. Just waiting for feedback on the idea.

> Additionally, there are notable exceptions to the rule that most drivers 
> are endian-clean, e.g. drivers/scsi/qla2xxx. I would appreciate it if it 
> would remain possible to check such drivers with sparse without enabling 
> endianness checks. Have you considered to change #ifdef __CHECK_ENDIAN__ 
> into e.g. #ifndef __DONT_CHECK_ENDIAN__?
> 
> Thanks,
> 
> Bart.

The right thing is probably just to fix these, isn't it?
Until then, why not just ignore the warnings?

-- 
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] linux/types.h: enable endian checks for all sparse builds

[Michael S. Tsirkin]

By now, linux is mostly endian-clean. Enabling endian-ness
checks for everyone produces about 200 new sparse warnings for me -
less than 10% over the 2000 sparse warnings already there.

Not a big deal, OTOH enabling this helps people notice
they are introducing new bugs.

So let's just drop __CHECK_ENDIAN__. Follow-up patches
can drop distinction between __bitwise and __bitwise__.

Cc: Linus Torvalds <torva...@linux-foundation.org>
Suggested-by: Christoph Hellwig <h...@infradead.org>
Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
---

Linus, could you ack this for upstream? If yes I'll
merge through my tree as a replacement for enabling
this just for virtio.

 include/uapi/linux/types.h | 4 
 1 file changed, 4 deletions(-)

diff --git a/include/uapi/linux/types.h b/include/uapi/linux/types.h
index acf0979..41e5914 100644
--- a/include/uapi/linux/types.h
+++ b/include/uapi/linux/types.h
@@ -23,11 +23,7 @@
 #else
 #define __bitwise__
 #endif
-#ifdef __CHECK_ENDIAN__
 #define __bitwise __bitwise__
-#else
-#define __bitwise
-#endif
 
 typedef __u16 __bitwise __le16;
 typedef __u16 __bitwise __be16;
-- 
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 10/10] virtio: enable endian checks for sparse builds

[Michael S. Tsirkin]

On Wed, Dec 07, 2016 at 07:25:51AM +0100, Johannes Berg wrote:
> On Tue, 2016-12-06 at 17:41 +0200, Michael S. Tsirkin wrote:
> 
> > It seems that there should be a better way to do it,
> > but this works too.
> 
> In some cases there might be:
> 
> > --- a/drivers/s390/virtio/Makefile
> > +++ b/drivers/s390/virtio/Makefile
> > @@ -6,6 +6,8 @@
> >  # it under the terms of the GNU General Public License (version 2
> > only)
> >  # as published by the Free Software Foundation.
> >  
> > +CFLAGS_virtio_ccw.o += -D__CHECK_ENDIAN__
> > +CFLAGS_kvm_virtio.o += -D__CHECK_ENDIAN__
> >  s390-virtio-objs := virtio_ccw.o
> >  ifdef CONFIG_S390_GUEST_OLD_TRANSPORT
> >  s390-virtio-objs += kvm_virtio.o
> 
> Here you could use
> 
> ccflags-y += -D__CHECK_ENDIAN__
> 
> for example, or even
> 
> subdir-ccflags-y += -D__CHECK_ENDIAN__
> 
> (in case any subdirs ever get added here)

Oh right. I forgot this directory only has virtio stuff.

> > --- a/drivers/vhost/Makefile
> > +++ b/drivers/vhost/Makefile
> > @@ -1,3 +1,4 @@
> > +ccflags-y := -D__CHECK_ENDIAN__
> 
> Looks like you did that here and in some other places though - so
> perhaps the s390 one was intentionally different?
> 
> > --- a/net/packet/Makefile
> > +++ b/net/packet/Makefile
> > @@ -2,6 +2,7 @@
> >  # Makefile for the packet AF.
> >  #
> >  
> > +ccflags-y := -D__CHECK_ENDIAN__
> 
> Technically this is slightly more than advertised, but I guess that
> still makes sense if it's clean now.
> 
> johannes
--
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 10/10] virtio: enable endian checks for sparse builds

[Michael S. Tsirkin]

__CHECK_ENDIAN__ isn't on by default presumably because
it triggers too many sparse warnings for correct code.
But virtio is now clean of these warnings, and
we want to keep it this way - enable this for
sparse builds.

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

It seems that there should be a better way to do it,
but this works too.

 drivers/block/Makefile  | 1 +
 drivers/char/Makefile   | 1 +
 drivers/char/hw_random/Makefile | 2 ++
 drivers/gpu/drm/virtio/Makefile | 1 +
 drivers/net/Makefile| 3 +++
 drivers/net/caif/Makefile   | 1 +
 drivers/rpmsg/Makefile  | 1 +
 drivers/s390/virtio/Makefile| 2 ++
 drivers/scsi/Makefile   | 1 +
 drivers/vhost/Makefile  | 1 +
 drivers/virtio/Makefile | 3 +++
 net/9p/Makefile | 1 +
 net/packet/Makefile | 1 +
 net/vmw_vsock/Makefile  | 2 ++
 14 files changed, 21 insertions(+)

diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index 1e9661e..597481c 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_BLK_DEV_OSD) += osdblk.o
 obj-$(CONFIG_BLK_DEV_UMEM) += umem.o
 obj-$(CONFIG_BLK_DEV_NBD)  += nbd.o
 obj-$(CONFIG_BLK_DEV_CRYPTOLOOP) += cryptoloop.o
+CFLAGS_virtio_blk.o += -D__CHECK_ENDIAN__
 obj-$(CONFIG_VIRTIO_BLK)   += virtio_blk.o
 
 obj-$(CONFIG_BLK_DEV_SX8)  += sx8.o
diff --git a/drivers/char/Makefile b/drivers/char/Makefile
index 6e6c244..a99467d 100644
--- a/drivers/char/Makefile
+++ b/drivers/char/Makefile
@@ -6,6 +6,7 @@ obj-y   += mem.o random.o
 obj-$(CONFIG_TTY_PRINTK)   += ttyprintk.o
 obj-y  += misc.o
 obj-$(CONFIG_ATARI_DSP56K) += dsp56k.o
+CFLAGS_virtio_console.o += -D__CHECK_ENDIAN__
 obj-$(CONFIG_VIRTIO_CONSOLE)   += virtio_console.o
 obj-$(CONFIG_RAW_DRIVER)   += raw.o
 obj-$(CONFIG_SGI_SNSC) += snsc.o snsc_event.o
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index 5f52b1e..a2b0931 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -17,6 +17,8 @@ obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
 obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
 obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
 obj-$(CONFIG_HW_RANDOM_PASEMI) += pasemi-rng.o
+CFLAGS_virtio_transport.o += -D__CHECK_ENDIAN__
+CFLAGS_virtio-rng.o += -D__CHECK_ENDIAN__
 obj-$(CONFIG_HW_RANDOM_VIRTIO) += virtio-rng.o
 obj-$(CONFIG_HW_RANDOM_TX4939) += tx4939-rng.o
 obj-$(CONFIG_HW_RANDOM_MXC_RNGA) += mxc-rnga.o
diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
index 3fb8eac..1162366 100644
--- a/drivers/gpu/drm/virtio/Makefile
+++ b/drivers/gpu/drm/virtio/Makefile
@@ -3,6 +3,7 @@
 # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
 
 ccflags-y := -Iinclude/drm
+ccflags-y += -D__CHECK_ENDIAN__
 
 virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_drm_bus.o virtgpu_gem.o \
virtgpu_fb.o virtgpu_display.o virtgpu_vq.o virtgpu_ttm.o \
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 7336cbd..3f587de 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_EQUALIZER) += eql.o
 obj-$(CONFIG_IFB) += ifb.o
 obj-$(CONFIG_MACSEC) += macsec.o
 obj-$(CONFIG_MACVLAN) += macvlan.o
+CFLAGS_macvtap.o += -D__CHECK_ENDIAN__
 obj-$(CONFIG_MACVTAP) += macvtap.o
 obj-$(CONFIG_MII) += mii.o
 obj-$(CONFIG_MDIO) += mdio.o
@@ -20,8 +21,10 @@ obj-$(CONFIG_NETCONSOLE) += netconsole.o
 obj-$(CONFIG_PHYLIB) += phy/
 obj-$(CONFIG_RIONET) += rionet.o
 obj-$(CONFIG_NET_TEAM) += team/
+CFLAGS_tun.o += -D__CHECK_ENDIAN__
 obj-$(CONFIG_TUN) += tun.o
 obj-$(CONFIG_VETH) += veth.o
+CFLAGS_virtio_net.o += -D__CHECK_ENDIAN__
 obj-$(CONFIG_VIRTIO_NET) += virtio_net.o
 obj-$(CONFIG_VXLAN) += vxlan.o
 obj-$(CONFIG_GENEVE) += geneve.o
diff --git a/drivers/net/caif/Makefile b/drivers/net/caif/Makefile
index 9bbd453..d1a922c 100644
--- a/drivers/net/caif/Makefile
+++ b/drivers/net/caif/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_CAIF_HSI) += caif_hsi.o
 
 # Virtio interface
 obj-$(CONFIG_CAIF_VIRTIO) += caif_virtio.o
+CFLAGS_caif_virtio.o += -D__CHECK_ENDIAN__
diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
index ae9c913..23c8b66 100644
--- a/drivers/rpmsg/Makefile
+++ b/drivers/rpmsg/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_RPMSG)+= rpmsg_core.o
 obj-$(CONFIG_RPMSG_QCOM_SMD)   += qcom_smd.o
 obj-$(CONFIG_RPMSG_VIRTIO) += virtio_rpmsg_bus.o
+CFLAGS_virtio_rpmsg_bus.o  += -D__CHECK_ENDIAN__
diff --git a/drivers/s390/virtio/Makefile b/drivers/s390/virtio/Makefile
index df40692..270ada5 100644
--- a/drivers/s390/virtio/Makefile
+++ b/drivers/s390/virtio/Makefile
@@ -6,6 +6,8 @@
 # it under the terms of the GNU General Public License (version 2 only)
 # as published by the Free Software Foundation.
 
+CFLAGS_virtio_ccw.o += -D__CHECK_ENDIAN__
+CFLAGS_kvm_virtio.o += -D__CHECK_ENDIAN__

[PATCH 00/10] virtio: sparse fixes

[Michael S. Tsirkin]

I run latest sparse from git on virtio drivers
(turns out the version I had was rather outdated).
This patchset fixes a couple of bugs this uncovered,
and adds some annotations to make it sparse-clean.
In particular, endian-ness is often tricky,
so this patchset enabled endian-ness checks for sparse
builds.

Michael S. Tsirkin (10):
  virtio_console: drop unused config fields
  drm/virtio: fix endianness in primary_plane_update
  drm/virtio: fix lock context imbalance
  drm/virtio: annotate virtio_gpu_queue_ctrl_buffer_locked
  vhost: make interval tree static inline
  vhost: add missing __user annotations
  vsock/virtio: add a missing __le annotation
  vsock/virtio: mark an internal function static
  vsock/virtio: fix src/dst cid format
  virtio: enable endian checks for sparse builds

 drivers/char/virtio_console.c   | 14 +++---
 drivers/gpu/drm/virtio/virtgpu_plane.c  |  4 ++--
 drivers/gpu/drm/virtio/virtgpu_vq.c |  6 +-
 drivers/vhost/vhost.c   | 12 ++--
 net/vmw_vsock/virtio_transport.c|  2 +-
 net/vmw_vsock/virtio_transport_common.c | 16 
 drivers/block/Makefile  |  1 +
 drivers/char/Makefile   |  1 +
 drivers/char/hw_random/Makefile |  2 ++
 drivers/gpu/drm/virtio/Makefile |  1 +
 drivers/net/Makefile|  3 +++
 drivers/net/caif/Makefile   |  1 +
 drivers/rpmsg/Makefile  |  1 +
 drivers/s390/virtio/Makefile|  2 ++
 drivers/scsi/Makefile   |  1 +
 drivers/vhost/Makefile  |  1 +
 drivers/virtio/Makefile |  3 +++
 net/9p/Makefile |  1 +
 net/packet/Makefile |  1 +
 net/vmw_vsock/Makefile  |  2 ++
 20 files changed, 50 insertions(+), 25 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

Re: DEFINE_IDA causing memory leaks? (was Re: [PATCH 1/2] virtio: fix memory leak of virtio ida cache layers)

[Michael S. Tsirkin]

On Thu, Sep 17, 2015 at 07:15:44AM -0700, James Bottomley wrote:
> On Thu, 2015-09-17 at 08:33 +0300, Michael S. Tsirkin wrote:
> > On Wed, Sep 16, 2015 at 07:29:17PM -0500, Suman Anna wrote:
> > > The virtio core uses a static ida named virtio_index_ida for
> > > assigning index numbers to virtio devices during registration.
> > > The ida core may allocate some internal idr cache layers and
> > > an ida bitmap upon any ida allocation, and all these layers are
> > > truely freed only upon the ida destruction. The virtio_index_ida
> > > is not destroyed at present, leading to a memory leak when using
> > > the virtio core as a module and atleast one virtio device is
> > > registered and unregistered.
> > > 
> > > Fix this by invoking ida_destroy() in the virtio core module
> > > exit.
> > > 
> > > Cc: "Michael S. Tsirkin" <m...@redhat.com>
> > > Signed-off-by: Suman Anna <s-a...@ti.com>
> > 
> > Interesting.
> > Will the same apply to e.g. sd_index_ida in drivers/scsi/sd.c
> > or iscsi_sess_ida in drivers/scsi/scsi_transport_iscsi.c?
> > 
> > If no, why not?
> > 
> > One doesn't generally expect to have to free global variables.
> > Maybe we should forbid DEFINE_IDA in modules?
> > 
> > James, could you comment on this please?
> 
> ida is Tejun's baby (cc'd).  However, it does look like without
> ida_destroy() you will leave a cached ida->bitmap dangling because we're
> trying to be a bit clever in ida_remove() so we cache the bitmap to
> relieve ida_pre_get() of the burden if we would otherwise free it.
> 
> I don't understand why you'd want to forbid DEFINE_IDA ... all it does
> is pre-initialise a usually static ida structure.  The initialised
> structure will have a NULL bitmap cache that's allocated in the first
> ida_pre_get() ... that all seems to work as expected and no different
> from a dynamically allocated struct ida.  Or are you thinking because
> ida_destory() doesn't set bitmap to NULL, it damages the reuse?  In
> which case I'm not sure there's much benefit to making it reusable, but
> I suppose we could by adding a memset into ida_destroy().
> 
> James

It's just unusual to have  a descructor without a constructor.
I bet more drivers misuse this AI because of this.

> > > ---
> > >  drivers/virtio/virtio.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > index b1877d73fa56..7062bb0975a5 100644
> > > --- a/drivers/virtio/virtio.c
> > > +++ b/drivers/virtio/virtio.c
> > > @@ -412,6 +412,7 @@ static int virtio_init(void)
> > >  static void __exit virtio_exit(void)
> > >  {
> > >   bus_unregister(_bus);
> > > + ida_destroy(_index_ida);
> > >  }
> > >  core_initcall(virtio_init);
> > >  module_exit(virtio_exit);
> > > -- 
> > > 2.5.0
> > --
> > 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
> > 
> 
> 
> 
--
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

DEFINE_IDA causing memory leaks? (was Re: [PATCH 1/2] virtio: fix memory leak of virtio ida cache layers)

[Michael S. Tsirkin]

On Wed, Sep 16, 2015 at 07:29:17PM -0500, Suman Anna wrote:
> The virtio core uses a static ida named virtio_index_ida for
> assigning index numbers to virtio devices during registration.
> The ida core may allocate some internal idr cache layers and
> an ida bitmap upon any ida allocation, and all these layers are
> truely freed only upon the ida destruction. The virtio_index_ida
> is not destroyed at present, leading to a memory leak when using
> the virtio core as a module and atleast one virtio device is
> registered and unregistered.
> 
> Fix this by invoking ida_destroy() in the virtio core module
> exit.
> 
> Cc: "Michael S. Tsirkin" <m...@redhat.com>
> Signed-off-by: Suman Anna <s-a...@ti.com>

Interesting.
Will the same apply to e.g. sd_index_ida in drivers/scsi/sd.c
or iscsi_sess_ida in drivers/scsi/scsi_transport_iscsi.c?

If no, why not?

One doesn't generally expect to have to free global variables.
Maybe we should forbid DEFINE_IDA in modules?

James, could you comment on this please?

> ---
>  drivers/virtio/virtio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index b1877d73fa56..7062bb0975a5 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -412,6 +412,7 @@ static int virtio_init(void)
>  static void __exit virtio_exit(void)
>  {
>   bus_unregister(_bus);
> + ida_destroy(_index_ida);
>  }
>  core_initcall(virtio_init);
>  module_exit(virtio_exit);
> -- 
> 2.5.0
--
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 61/86] scsi/qla1280: use uapi/linux/pci_ids.h directly

[Michael S. Tsirkin]

On Sun, Mar 29, 2015 at 05:03:36PM +0300, James Bottomley wrote:
 On Sun, 2015-03-29 at 15:42 +0200, Michael S. Tsirkin wrote:
  Header moved from linux/pci_ids.h to uapi/linux/pci_ids.h,
  use the new header directly so we can drop
  the wrapper in include/linux/pci_ids.h.
  
  Signed-off-by: Michael S. Tsirkin m...@redhat.com
  ---
   drivers/scsi/qla1280.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
  
  diff --git a/drivers/scsi/qla1280.c b/drivers/scsi/qla1280.c
  index c68a66e..b2ada21 100644
  --- a/drivers/scsi/qla1280.c
  +++ b/drivers/scsi/qla1280.c
  @@ -348,7 +348,7 @@
   #include linux/pci.h
   #include linux/proc_fs.h
   #include linux/stat.h
  -#include linux/pci_ids.h
  +#include uapi/linux/pci_ids.h
 
 This is bogus, isn't it?  There's a -Iuapi somewhere in the kernel
 compile line so the original include is still valid.  Zorro does this:
 zorro_ids.h is exclusively in uapi but the include is still
 
 #include linux/zorro_ids.h
 
 James
 


Hmm, that's true. I didn't know.  A bunch of files pull in headers from uapi
explicitly, so I assumed it's a good idea.  Do you think it's better to include
uapi files using short linux/x.h, or the full uapi/linux/x.h?  Linux has a
mix of both.

-- 
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 61/86] scsi/qla1280: use uapi/linux/pci_ids.h directly

[Michael S. Tsirkin]

Header moved from linux/pci_ids.h to uapi/linux/pci_ids.h,
use the new header directly so we can drop
the wrapper in include/linux/pci_ids.h.

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

diff --git a/drivers/scsi/qla1280.c b/drivers/scsi/qla1280.c
index c68a66e..b2ada21 100644
--- a/drivers/scsi/qla1280.c
+++ b/drivers/scsi/qla1280.c
@@ -348,7 +348,7 @@
 #include linux/pci.h
 #include linux/proc_fs.h
 #include linux/stat.h
-#include linux/pci_ids.h
+#include uapi/linux/pci_ids.h
 #include linux/interrupt.h
 #include linux/init.h
 #include linux/dma-mapping.h
-- 
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 60/86] scsi/arcmsr: use uapi/linux/pci_ids.h directly

[Michael S. Tsirkin]

Header moved from linux/pci_ids.h to uapi/linux/pci_ids.h,
use the new header directly so we can drop
the wrapper in include/linux/pci_ids.h.

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

diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
index 914c39f..ead6f31 100644
--- a/drivers/scsi/arcmsr/arcmsr_hba.c
+++ b/drivers/scsi/arcmsr/arcmsr_hba.c
@@ -47,7 +47,7 @@
 #include linux/module.h
 #include linux/reboot.h
 #include linux/spinlock.h
-#include linux/pci_ids.h
+#include uapi/linux/pci_ids.h
 #include linux/interrupt.h
 #include linux/moduleparam.h
 #include linux/errno.h
-- 
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 6/9] vhost/scsi: Set VIRTIO_F_ANY_LAYOUT + VIRTIO_F_VERSION_1 feature bits

[Michael S. Tsirkin]

On Wed, Feb 04, 2015 at 01:13:58AM -0800, Nicholas A. Bellinger wrote:
 On Tue, 2015-02-03 at 11:40 +0200, Michael S. Tsirkin wrote:
  On Tue, Feb 03, 2015 at 06:30:00AM +, Nicholas A. Bellinger wrote:
   From: Nicholas Bellinger n...@linux-iscsi.org
   
   Signal support of VIRTIO_F_ANY_LAYOUT + VIRTIO_F_VERSION_1 feature bits
   required for virtio-scsi 1.0 spec layout requirements.
   
   Cc: Michael S. Tsirkin m...@redhat.com
   Cc: Paolo Bonzini pbonz...@redhat.com
   Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
  
  Acked-by: Michael S. Tsirkin m...@redhat.com
  
  I'm hoping someone'll actually test this with a BE guest before
  we release this though.
  
 
 Yes please.  Currently waiting on an Power system to test against, but
 alas all of the OP systems are LE these days..
 
 What's left, s390..?  ;)
 
 --nab

Yes - Cc Conelia, I think she was interested in that.

-- 
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 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback

[Michael S. Tsirkin]

On Wed, Feb 04, 2015 at 01:40:25AM -0800, Nicholas A. Bellinger wrote:
   + /*
   +  * Any associated T10_PI bytes for the outgoing / incoming
   +  * payloads are included in calculation of exp_data_len here.
   +  */
   + if (out_size  req_size) {
   + data_direction = DMA_TO_DEVICE;
   + exp_data_len = out_size - req_size;
   + } else if (in_size  rsp_size) {
   + data_direction = DMA_FROM_DEVICE;
   + exp_data_len = in_size - rsp_size;
   + } else {
   + data_direction = DMA_NONE;
   + exp_data_len = 0;
   + }
  
  We must validate this doesn't cause exp_data_len to be negative.
  
 
 AFAICT, exp_data_len is always = 0 here.

What guarantees out_size  req_size and in_size  rsp_size,
respectively?

-- 
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 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback

[Michael S. Tsirkin]

On Wed, Feb 04, 2015 at 02:11:20AM -0800, Nicholas A. Bellinger wrote:
 On Tue, 2015-02-03 at 23:56 +, Al Viro wrote:
  On Tue, Feb 03, 2015 at 06:29:59AM +, Nicholas A. Bellinger wrote:
   +  * Copy over the virtio-scsi request header, which when
   +  * ANY_LAYOUT is enabled may span multiple iovecs, or a
   +  * single iovec may contain both the header + outgoing
   +  * WRITE payloads.
   +  *
   +  * copy_from_iter() is modifying the iovecs as copies over
   +  * req_size bytes into req, so the returned out_iter.iov[0]
   +  * will contain the correct start + offset of the outgoing
   +  * WRITE payload, if DMA_TO_DEVICE is set.
  
  It does no such thing.  What it does, though, is changing out_iter so
  that subsequent copy_from_iter() will return the data you want.  Note
  that out_iter.iov[0] will contain the correct _segment_ of that vector,
  with the data you want at out_iter.iov_offset bytes from the beginning
  of that segment.  .iov may come to point to subsequent segments and 
  .iov_offset
  keeps changing, but segments themselves are never changed.
 
 Yes, sorry.  Updating that comment to read:
 
  /*
   * Copy over the virtio-scsi request header, which for a
   * ANY_LAYOUT enabled guest may span multiple iovecs, or a
   * single iovec may contain both the header + outgoing
   * WRITE payloads.
   *
   * copy_from_iter() copies over req_size bytes, and sets up
   * out_iter.iov[0] + out_iter.iov_offset to contain the start
   * of the outgoing WRITE payload, if DMA_TO_DEVICE is set.
   */

I'm still confused wrt what this refers to.
You don't actually play with iovs directly anymore,
why bother explaining what happens to the underlying iov?
Can we just say
copy_from_iter will advance out_iter, so that it will point
at the start of the outgoing WRITE payload, if DMA_TO_DEVICE is
set.

Seems clearer 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-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback

[Michael S. Tsirkin]

On Wed, Feb 04, 2015 at 02:41:07AM -0800, Nicholas A. Bellinger wrote:
 On Wed, 2015-02-04 at 10:42 +0100, Michael S. Tsirkin wrote:
  On Wed, Feb 04, 2015 at 01:40:25AM -0800, Nicholas A. Bellinger wrote:
 + /*
 +  * Any associated T10_PI bytes for the outgoing / 
 incoming
 +  * payloads are included in calculation of exp_data_len 
 here.
 +  */
 + if (out_size  req_size) {
 + data_direction = DMA_TO_DEVICE;
 + exp_data_len = out_size - req_size;
 + } else if (in_size  rsp_size) {
 + data_direction = DMA_FROM_DEVICE;
 + exp_data_len = in_size - rsp_size;
 + } else {
 + data_direction = DMA_NONE;
 + exp_data_len = 0;
 + }

We must validate this doesn't cause exp_data_len to be negative.

   
   AFAICT, exp_data_len is always = 0 here.
  
  What guarantees out_size  req_size and in_size  rsp_size,
  respectively?
  
 
 Mmm, point taken.
 
 So moving this part after copy_from_iter() ensures that at least
 req_size bytes exists of out_size.  Making this change now.
 
 For in_size  rsp_size there is no guarantee, and falls back to
 data_direction = DMA_NONE + exp_data_len = 0;
 
 Is this what you had in mind..?
 
 --nab

Hmm what do you mean by there is no guarantee?
What will happen if in_size  rsp_size because guest
supplied an invalid descriptor?

-- 
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 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback

[Michael S. Tsirkin]

On Wed, Feb 04, 2015 at 02:55:12AM -0800, Nicholas A. Bellinger wrote:
 On Wed, 2015-02-04 at 02:41 -0800, Nicholas A. Bellinger wrote:
  On Wed, 2015-02-04 at 10:42 +0100, Michael S. Tsirkin wrote:
   On Wed, Feb 04, 2015 at 01:40:25AM -0800, Nicholas A. Bellinger wrote:
  +   /*
  +* Any associated T10_PI bytes for the outgoing / 
  incoming
  +* payloads are included in calculation of exp_data_len 
  here.
  +*/
  +   if (out_size  req_size) {
  +   data_direction = DMA_TO_DEVICE;
  +   exp_data_len = out_size - req_size;
  +   } else if (in_size  rsp_size) {
  +   data_direction = DMA_FROM_DEVICE;
  +   exp_data_len = in_size - rsp_size;
  +   } else {
  +   data_direction = DMA_NONE;
  +   exp_data_len = 0;
  +   }
 
 We must validate this doesn't cause exp_data_len to be negative.
 

AFAICT, exp_data_len is always = 0 here.
   
   What guarantees out_size  req_size and in_size  rsp_size,
   respectively?
   
  
  Mmm, point taken.
  
  So moving this part after copy_from_iter() ensures that at least
  req_size bytes exists of out_size.  Making this change now.
  
  For in_size  rsp_size there is no guarantee, and falls back to
  data_direction = DMA_NONE + exp_data_len = 0;
  
  Is this what you had in mind..?
  
 
 Something akin to:
 
 diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
 index f72fc56..daf10b7 100644
 --- a/drivers/vhost/scsi.c
 +++ b/drivers/vhost/scsi.c
 @@ -1047,30 +1047,12 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct 
 vhost_virtqueue *vq)
   target = v_req.lun[1];
   }
   /*
 -  * Determine data_direction by calculating the total outgoing
 -  * iovec sizes / incoming iovec sizes vs. virtio-scsi request /
 -  * response headers respectively.
 -  *
* FIXME: Not correct for BIDI operation
*/
   out_size = iov_length(vq-iov, out);
   in_size = iov_length(vq-iov[out], in);
  
   /*
 -  * Any associated T10_PI bytes for the outgoing / incoming
 -  * payloads are included in calculation of exp_data_len here.
 -  */
 - if (out_size  req_size) {
 - data_direction = DMA_TO_DEVICE;
 - exp_data_len = out_size - req_size;
 - } else if (in_size  rsp_size) {
 - data_direction = DMA_FROM_DEVICE;
 - exp_data_len = in_size - rsp_size;
 - } else {
 - data_direction = DMA_NONE;
 - exp_data_len = 0;
 - }
 - /*
* Copy over the virtio-scsi request header, which for a
* ANY_LAYOUT enabled guest may span multiple iovecs, or a
* single iovec may contain both the header + outgoing
 @@ -1102,8 +1084,9 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct 
 vhost_virtqueue *vq)
   continue;
   }
   /*
 -  * Determine start of T10_PI or data payload iovec based upon
 -  * data_direction.
 +  * Determine data_direction by calculating the total outgoing
 +  * iovec sizes + incoming iovec sizes vs. virtio-scsi request +
 +  * response headers respectively.
*
* For DMA_TO_DEVICE this is out_iter, which is already pointing
* to the right place.
 @@ -,16 +1094,27 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct 
 vhost_virtqueue *vq)
* For DMA_FROM_DEVICE, the iovec will be just past the end
* of the virtio-scsi response header in either the same
* or immediately following iovec.
 +  *
 +  * Any associated T10_PI bytes for the outgoing / incoming
 +  * payloads are included in calculation of exp_data_len here.
*/
   prot_bytes = 0;
  
 - if (data_direction == DMA_TO_DEVICE) {
 + if (out_size  req_size) {
 + data_direction = DMA_TO_DEVICE;
 + exp_data_len = out_size - req_size;
   data_iter = out_iter;
 - } else if (data_direction == DMA_FROM_DEVICE) {
 + } else if (in_size  rsp_size) {
 + data_direction = DMA_FROM_DEVICE;
 + exp_data_len = in_size - rsp_size;
 +
   iov_iter_init(in_iter, READ, vq-iov[out], in,
 rsp_size + exp_data_len);
   iov_iter_advance(in_iter, rsp_size);
   data_iter = in_iter;
 + } else

Re: [PATCH-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback

[Michael S. Tsirkin]

On Wed, Feb 04, 2015 at 02:41:07AM -0800, Nicholas A. Bellinger wrote:
 On Wed, 2015-02-04 at 10:42 +0100, Michael S. Tsirkin wrote:
  On Wed, Feb 04, 2015 at 01:40:25AM -0800, Nicholas A. Bellinger wrote:
 + /*
 +  * Any associated T10_PI bytes for the outgoing / 
 incoming
 +  * payloads are included in calculation of exp_data_len 
 here.
 +  */
 + if (out_size  req_size) {
 + data_direction = DMA_TO_DEVICE;
 + exp_data_len = out_size - req_size;
 + } else if (in_size  rsp_size) {
 + data_direction = DMA_FROM_DEVICE;
 + exp_data_len = in_size - rsp_size;
 + } else {
 + data_direction = DMA_NONE;
 + exp_data_len = 0;
 + }

We must validate this doesn't cause exp_data_len to be negative.

   
   AFAICT, exp_data_len is always = 0 here.
  
  What guarantees out_size  req_size and in_size  rsp_size,
  respectively?
  
 
 Mmm, point taken.
 
 So moving this part after copy_from_iter() ensures that at least
 req_size bytes exists of out_size.  Making this change now.
 
 For in_size  rsp_size there is no guarantee, and falls back to
 data_direction = DMA_NONE + exp_data_len = 0;
 
 Is this what you had in mind..?
 
 --nab

I don't see any problems with 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: [PATCH-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback

[Michael S. Tsirkin]

On Tue, Feb 03, 2015 at 11:56:16PM +, Al Viro wrote:
 On Tue, Feb 03, 2015 at 06:29:59AM +, Nicholas A. Bellinger wrote:
  +* Copy over the virtio-scsi request header, which when
  +* ANY_LAYOUT is enabled may span multiple iovecs, or a
  +* single iovec may contain both the header + outgoing
  +* WRITE payloads.
  +*
  +* copy_from_iter() is modifying the iovecs as copies over
  +* req_size bytes into req, so the returned out_iter.iov[0]
  +* will contain the correct start + offset of the outgoing
  +* WRITE payload, if DMA_TO_DEVICE is set.
 
 It does no such thing.  What it does, though, is changing out_iter so
 that subsequent copy_from_iter() will return the data you want.  Note
 that out_iter.iov[0] will contain the correct _segment_ of that vector,
 with the data you want at out_iter.iov_offset bytes from the beginning
 of that segment.  .iov may come to point to subsequent segments and 
 .iov_offset
 keeps changing, but segments themselves are never changed.
 
  +*/
  +   iov_iter_init(out_iter, READ, vq-iov[0], out,
 WRITE, please - as in this is
 the source of some write, we'll be copying _from_ it.  READ would be
 destination of some read, we'll be copying into it.
 
  +(data_direction == DMA_TO_DEVICE) ?
  + req_size + exp_data_len : req_size);
  +
  +   ret = copy_from_iter(req, req_size, out_iter);
 
 ...
 
  +   /*
  +* Determine start of T10_PI or data payload iovec in ANY_LAYOUT
  +* mode based upon data_direction.
  +*
  +* For DMA_TO_DEVICE, this is iov_out from copy_from_iter()
  +* with the already recalculated iov_base + iov_len.
 
 ITYM this is out_iter, which is already pointing to the right place
 
 AFAICS, the actual use is correct, it's just that the comments are confused.

Looks like it'd be nicer to pass iters around as much as possible,
try to reduce the amount of poking at the underlying iov.

--
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 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback

[Michael S. Tsirkin]

 +  * copy_from_iter() is modifying the iovecs as copies over
 +  * req_size bytes into req, so the returned out_iter.iov[0]
 +  * will contain the correct start + offset of the outgoing
 +  * WRITE payload, if DMA_TO_DEVICE is set.
 +  */

Does it, in fact, do this?
I thought so too, but Al Viro corrected me that it's
not supposed to.

-- 
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 1/9] vhost/scsi: Convert completion path to use copy_to_iser

[Michael S. Tsirkin]

On Tue, Feb 03, 2015 at 06:29:55AM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 Required for ANY_LAYOUT support when the incoming virtio-scsi response
 header + fixed size sense buffer payload may span more than a single
 iovec entry.
 
 This changes existing code to save cmd-tvc_resp_iov instead of the
 first single iovec base pointer from vq-iov[out].

Typo in subject: should be copy_to_iter.

 v3 changes:
   - Convert memcpy_toiovecend - copy_to_iser usage

This belongs after ---

 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Paolo Bonzini pbonz...@redhat.com
 Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
 ---
  drivers/vhost/scsi.c | 17 -
  1 file changed, 12 insertions(+), 5 deletions(-)
 
 diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
 index 01c01cb..1ad5b0f 100644
 --- a/drivers/vhost/scsi.c
 +++ b/drivers/vhost/scsi.c
 @@ -72,6 +72,8 @@ struct tcm_vhost_cmd {
   int tvc_vq_desc;
   /* virtio-scsi initiator task attribute */
   int tvc_task_attr;
 + /* virtio-scsi response incoming iovecs */
 + int tvc_in_iovs;
   /* virtio-scsi initiator data direction */
   enum dma_data_direction tvc_data_direction;
   /* Expected data transfer length from virtio-scsi header */
 @@ -87,8 +89,8 @@ struct tcm_vhost_cmd {
   struct scatterlist *tvc_sgl;
   struct scatterlist *tvc_prot_sgl;
   struct page **tvc_upages;
 - /* Pointer to response */
 - struct virtio_scsi_cmd_resp __user *tvc_resp;
 + /* Pointer to response header iovec */
 + struct iovec *tvc_resp_iov;
   /* Pointer to vhost_scsi for our device */
   struct vhost_scsi *tvc_vhost;
   /* Pointer to vhost_virtqueue for the cmd */
 @@ -682,6 +684,7 @@ static void vhost_scsi_complete_cmd_work(struct 
 vhost_work *work)
   struct tcm_vhost_cmd *cmd;
   struct llist_node *llnode;
   struct se_cmd *se_cmd;
 + struct iov_iter iov_iter;
   int ret, vq;
  
   bitmap_zero(signal, VHOST_SCSI_MAX_VQ);
 @@ -703,8 +706,11 @@ static void vhost_scsi_complete_cmd_work(struct 
 vhost_work *work)
se_cmd-scsi_sense_length);
   memcpy(v_rsp.sense, cmd-tvc_sense_buf,
  se_cmd-scsi_sense_length);
 - ret = copy_to_user(cmd-tvc_resp, v_rsp, sizeof(v_rsp));
 - if (likely(ret == 0)) {
 +
 + iov_iter_init(iov_iter, WRITE, cmd-tvc_resp_iov,
 +   cmd-tvc_in_iovs, sizeof(v_rsp));
 + ret = copy_to_iter(v_rsp, sizeof(v_rsp), iov_iter);
 + if (likely(ret == sizeof(v_rsp))) {
   struct vhost_scsi_virtqueue *q;
   vhost_add_used(cmd-tvc_vq, cmd-tvc_vq_desc, 0);
   q = container_of(cmd-tvc_vq, struct 
 vhost_scsi_virtqueue, vq);
 @@ -1159,7 +1165,8 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct 
 vhost_virtqueue *vq)
  
   cmd-tvc_vhost = vs;
   cmd-tvc_vq = vq;
 - cmd-tvc_resp = vq-iov[out].iov_base;
 + cmd-tvc_resp_iov = vq-iov[out];
 + cmd-tvc_in_iovs = in;
  
   pr_debug(vhost_scsi got command opcode: %#02x, lun: %d\n,
   cmd-tvc_cdb[0], cmd-tvc_lun);
 -- 
 1.9.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-v3 8/9] vhost/scsi: Drop left-over scsi_tcq.h include

[Michael S. Tsirkin]

On Tue, Feb 03, 2015 at 06:30:02AM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 With the recent removal of MSG_*_TAG defines in commit 68d81f40,
 vhost-scsi is now using TCM_*_TAG and doesn't depend upon host
 side scsi_tcq.h definitions anymore.
 
 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Paolo Bonzini pbonz...@redhat.com
 Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org

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

 ---
  drivers/vhost/scsi.c | 1 -
  1 file changed, 1 deletion(-)
 
 diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
 index 9af93d0..2b4b002 100644
 --- a/drivers/vhost/scsi.c
 +++ b/drivers/vhost/scsi.c
 @@ -38,7 +38,6 @@
  #include linux/miscdevice.h
  #include asm/unaligned.h
  #include scsi/scsi.h
 -#include scsi/scsi_tcq.h
  #include target/target_core_base.h
  #include target/target_core_fabric.h
  #include target/target_core_fabric_configfs.h
 -- 
 1.9.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-v3 9/9] vhost/scsi: Global tcm_vhost - vhost_scsi rename

[Michael S. Tsirkin]

On Tue, Feb 03, 2015 at 06:30:03AM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 There is a large amount of code that still references the original
 'tcm_vhost' naming conventions, instead of modern 'vhost_scsi'.
 
 Go ahead and do a global rename to make the usage consistent.
 
 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Paolo Bonzini pbonz...@redhat.com
 Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org

Yes, I've been wondering about that.

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

 ---
  drivers/vhost/scsi.c | 662 
 +--
  1 file changed, 331 insertions(+), 331 deletions(-)
 
 diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
 index 2b4b002..66f682c 100644
 --- a/drivers/vhost/scsi.c
 +++ b/drivers/vhost/scsi.c
 @@ -51,13 +51,13 @@
  
  #include vhost.h
  
 -#define TCM_VHOST_VERSION  v0.1
 -#define TCM_VHOST_NAMELEN 256
 -#define TCM_VHOST_MAX_CDB_SIZE 32
 -#define TCM_VHOST_DEFAULT_TAGS 256
 -#define TCM_VHOST_PREALLOC_SGLS 2048
 -#define TCM_VHOST_PREALLOC_UPAGES 2048
 -#define TCM_VHOST_PREALLOC_PROT_SGLS 512
 +#define VHOST_SCSI_VERSION  v0.1
 +#define VHOST_SCSI_NAMELEN 256
 +#define VHOST_SCSI_MAX_CDB_SIZE 32
 +#define VHOST_SCSI_DEFAULT_TAGS 256
 +#define VHOST_SCSI_PREALLOC_SGLS 2048
 +#define VHOST_SCSI_PREALLOC_UPAGES 2048
 +#define VHOST_SCSI_PREALLOC_PROT_SGLS 512
  
  struct vhost_scsi_inflight {
   /* Wait for the flush operation to finish */
 @@ -66,7 +66,7 @@ struct vhost_scsi_inflight {
   struct kref kref;
  };
  
 -struct tcm_vhost_cmd {
 +struct vhost_scsi_cmd {
   /* Descriptor from vhost_get_vq_desc() for virt_queue segment */
   int tvc_vq_desc;
   /* virtio-scsi initiator task attribute */
 @@ -82,7 +82,7 @@ struct tcm_vhost_cmd {
   /* The number of scatterlists associated with this cmd */
   u32 tvc_sgl_count;
   u32 tvc_prot_sgl_count;
 - /* Saved unpacked SCSI LUN for tcm_vhost_submission_work() */
 + /* Saved unpacked SCSI LUN for vhost_scsi_submission_work() */
   u32 tvc_lun;
   /* Pointer to the SGL formatted memory from virtio-scsi */
   struct scatterlist *tvc_sgl;
 @@ -95,13 +95,13 @@ struct tcm_vhost_cmd {
   /* Pointer to vhost_virtqueue for the cmd */
   struct vhost_virtqueue *tvc_vq;
   /* Pointer to vhost nexus memory */
 - struct tcm_vhost_nexus *tvc_nexus;
 + struct vhost_scsi_nexus *tvc_nexus;
   /* The TCM I/O descriptor that is accessed via container_of() */
   struct se_cmd tvc_se_cmd;
 - /* work item used for cmwq dispatch to tcm_vhost_submission_work() */
 + /* work item used for cmwq dispatch to vhost_scsi_submission_work() */
   struct work_struct work;
   /* Copy of the incoming SCSI command descriptor block (CDB) */
 - unsigned char tvc_cdb[TCM_VHOST_MAX_CDB_SIZE];
 + unsigned char tvc_cdb[VHOST_SCSI_MAX_CDB_SIZE];
   /* Sense buffer that will be mapped into outgoing status */
   unsigned char tvc_sense_buf[TRANSPORT_SENSE_BUFFER];
   /* Completed commands list, serviced from vhost worker thread */
 @@ -110,53 +110,53 @@ struct tcm_vhost_cmd {
   struct vhost_scsi_inflight *inflight;
  };
  
 -struct tcm_vhost_nexus {
 +struct vhost_scsi_nexus {
   /* Pointer to TCM session for I_T Nexus */
   struct se_session *tvn_se_sess;
  };
  
 -struct tcm_vhost_nacl {
 +struct vhost_scsi_nacl {
   /* Binary World Wide unique Port Name for Vhost Initiator port */
   u64 iport_wwpn;
   /* ASCII formatted WWPN for Sas Initiator port */
 - char iport_name[TCM_VHOST_NAMELEN];
 - /* Returned by tcm_vhost_make_nodeacl() */
 + char iport_name[VHOST_SCSI_NAMELEN];
 + /* Returned by vhost_scsi_make_nodeacl() */
   struct se_node_acl se_node_acl;
  };
  
 -struct tcm_vhost_tpg {
 +struct vhost_scsi_tpg {
   /* Vhost port target portal group tag for TCM */
   u16 tport_tpgt;
   /* Used to track number of TPG Port/Lun Links wrt to explict I_T Nexus 
 shutdown */
   int tv_tpg_port_count;
   /* Used for vhost_scsi device reference to tpg_nexus, protected by 
 tv_tpg_mutex */
   int tv_tpg_vhost_count;
 - /* list for tcm_vhost_list */
 + /* list for vhost_scsi_list */
   struct list_head tv_tpg_list;
   /* Used to protect access for tpg_nexus */
   struct mutex tv_tpg_mutex;
   /* Pointer to the TCM VHost I_T Nexus for this TPG endpoint */
 - struct tcm_vhost_nexus *tpg_nexus;
 - /* Pointer back to tcm_vhost_tport */
 - struct tcm_vhost_tport *tport;
 - /* Returned by tcm_vhost_make_tpg() */
 + struct vhost_scsi_nexus *tpg_nexus;
 + /* Pointer back to vhost_scsi_tport */
 + struct vhost_scsi_tport *tport;
 + /* Returned by vhost_scsi_make_tpg() */
   struct se_portal_group se_tpg;
   /* Pointer back to vhost_scsi, protected by tv_tpg_mutex */
   struct vhost_scsi *vhost_scsi;
  };
  
 -struct tcm_vhost_tport {
 +struct

Re: [PATCH-v3 5/9] vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback

[Michael S. Tsirkin]

On Tue, Feb 03, 2015 at 06:29:59AM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 This patch adds ANY_LAYOUT support with a new vqs[].vq.handle_kick()
 callback in vhost_scsi_handle_vqal().
 
 It calculates data_direction + exp_data_len for the new tcm_vhost_cmd
 descriptor by walking both outgoing + incoming iovecs using iov_iter,
 assuming the layout of outgoing request header + T10_PI + Data payload
 comes first.
 
 It also uses copy_from_iter() to copy leading virtio-scsi request header
 that may or may not include SCSI CDB, that returns a re-calculated iovec
 to start of T10_PI or Data SGL memory.
 
 v2 changes:
   - Fix up vhost_scsi_handle_vqal comments
   - Minor vhost_scsi_handle_vqal simplifications
   - Add missing minimum virtio-scsi response buffer size check
   - Fix pi_bytes* error message typo
   - Convert to use vhost_skip_iovec_bytes() common code
   - Add max_niov sanity checks vs. out + in offset into vq
 
 v3 changes:
   - Convert to copy_from_iter usage
   - Update vhost_scsi_handle_vqal comments for iov_iter usage
   - Convert prot_bytes offset to use iov_iter_advance
   - Drop max_niov usage in vhost_scsi_handle_vqal
   - Drop vhost_skip_iovec_bytes in favour of iov_iter
 
 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Paolo Bonzini pbonz...@redhat.com
 Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
 ---
  drivers/vhost/scsi.c | 260 
 +++
  1 file changed, 260 insertions(+)
 
 diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
 index 7dfff15..e1fe003 100644
 --- a/drivers/vhost/scsi.c
 +++ b/drivers/vhost/scsi.c
 @@ -1062,6 +1062,266 @@ vhost_scsi_send_bad_target(struct vhost_scsi *vs,
  }
  
  static void
 +vhost_scsi_handle_vqal(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 +{
 + struct tcm_vhost_tpg **vs_tpg, *tpg;
 + struct virtio_scsi_cmd_req v_req;
 + struct virtio_scsi_cmd_req_pi v_req_pi;
 + struct tcm_vhost_cmd *cmd;
 + struct iov_iter out_iter, in_iter, prot_iter, data_iter;
 + u64 tag;
 + u32 exp_data_len, data_direction;
 + unsigned out, in, i;
 + int head, ret, prot_bytes;
 + size_t req_size, rsp_size = sizeof(struct virtio_scsi_cmd_resp);
 + size_t out_size, in_size;
 + u16 lun;
 + u8 *target, *lunp, task_attr;
 + bool t10_pi = vhost_has_feature(vq, VIRTIO_SCSI_F_T10_PI);
 + void *req, *cdb;
 +
 + mutex_lock(vq-mutex);
 + /*
 +  * We can handle the vq only after the endpoint is setup by calling the
 +  * VHOST_SCSI_SET_ENDPOINT ioctl.
 +  */
 + vs_tpg = vq-private_data;
 + if (!vs_tpg)
 + goto out;
 +
 + vhost_disable_notify(vs-dev, vq);
 +
 + for (;;) {
 + head = vhost_get_vq_desc(vq, vq-iov,
 +  ARRAY_SIZE(vq-iov), out, in,
 +  NULL, NULL);
 + pr_debug(vhost_get_vq_desc: head: %d, out: %u in: %u\n,
 +  head, out, in);
 + /* On error, stop handling until the next kick. */
 + if (unlikely(head  0))
 + break;
 + /* Nothing new?  Wait for eventfd to tell us they refilled. */
 + if (head == vq-num) {
 + if (unlikely(vhost_enable_notify(vs-dev, vq))) {
 + vhost_disable_notify(vs-dev, vq);
 + continue;
 + }
 + break;
 + }
 + /*
 +  * Check for a sane response buffer so we can report early
 +  * errors back to the guest.
 +  */
 + if (unlikely(vq-iov[out].iov_len  rsp_size)) {
 + vq_err(vq, Expecting at least virtio_scsi_cmd_resp
 +  size, got %zu bytes\n, vq-iov[out].iov_len);
 + break;
 + }
 + /*
 +  * Setup pointers and values based upon different virtio-scsi
 +  * request header if T10_PI is enabled in KVM guest.
 +  */
 + if (t10_pi) {
 + req = v_req_pi;
 + req_size = sizeof(v_req_pi);
 + lunp = v_req_pi.lun[0];
 + target = v_req_pi.lun[1];
 + } else {
 + req = v_req;
 + req_size = sizeof(v_req);
 + lunp = v_req.lun[0];
 + target = v_req.lun[1];
 + }
 + /*
 +  * Determine data_direction for ANY_LAYOUT

It's not just for ANY_LAYOUT though, is it?
After patchset is fully applied this will run for
all guests?

 by calculating the
 +  * total outgoing iovec sizes / incoming iovec sizes vs.
 +  * virtio-scsi request / response headers respectively.
 +  *
 +  * FIXME: Not correct for BIDI operation

Re: [PATCH-v3 0/9] vhost/scsi: Add ANY_LAYOUT + VERSION_1 support

[Michael S. Tsirkin]

On Tue, Feb 03, 2015 at 06:29:54AM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 Hi MST, Paolo, Al  Co,
 
 Here is -v3 for adding vhost/scsi ANY_LAYOUT + VERSION_1 host feature
 bit support.
 
 It adds a new vhost_virtqueue -handle_kick() callback to determine the
 start of protection and data payloads iovecs past starting virtio-scsi
 request and response headers, based upon data_direction using iov_iter
 primitives.
 
 It assumes request/CDB and response/sense_buffer headers may span more
 than a single iovec using mm/iov_iter.c logic.
 
 It also allows virtio-scsi headers + T10_PI + Data SGL payloads to span
 the same iovec when pinning user-space memory via get_user_pages_fast()
 code.  (Not tested yet)
 
 Based upon Al  HCH's feedback, the patch series has been converted to
 use copy_*_iter() for virtio-scsi header copy.  Also, patch #4 has been
 updated to use iov_iter_npages() for sgl_count, and patch #5 updated to
 use iov_iter_advance() for calculating prot_bytes offset to the start
 of data_iter.
 
 v3 changelog:
   - Convert memcpy_toiovecend - copy_to_iter usage
   - Update vhost_scsi_mapal + friends to use iov_iter
   - Move iov_iter sanity checks into vhost_scsi_calc_sgls
   - Convert vhost_scsi_calc_sgls() to iov_iter_npages()
   - Convert to vhost_scsi_handle_vqal to copy_from_iter usage
   - Update vhost_scsi_handle_vqal comments for iov_iter usage
   - Convert prot_bytes offset to use iov_iter_advance
   - Drop max_niov usage in vhost_scsi_handle_vqal
   - Drop vhost_skip_iovec_bytes in favour of iov_iter
 
 Note the one part that has been left unchanged is vhost_scsi_map_to_sgl()
 into get_user_pages_fast(), for which existing iov_iter_get_pages() code
 will need to allow for a callback to perform the associated scatterlists
 setup from **pages for protection + data payloads.

I'm not sure it has to be a callback: maybe just add
struct scatterlist *sg parameter and use it if set.

 It's functioning against v3.19-rc1 virtio-scsi LLD in T10_PI mode using
 TYPE-1 DIF with ANY_LAYOUT - VERSION_1 guest feature bits enabled, using
 the layout following existing convention with protection/data SGL payloads
 residing within seperate iovecs.
 
 Also included in patch #9 is an over-due change to rename code in scsi.c
 to line up with modern vhost_scsi naming convention.
 
 Please review.
 
 Thank you,
 
 Nicholas Bellinger (9):
   vhost/scsi: Convert completion path to use copy_to_iser
   vhost/scsi: Fix incorrect early vhost_scsi_handle_vq failures
   vhost/scsi: Change vhost_scsi_map_to_sgl to accept iov ptr + len
   vhost/scsi: Add ANY_LAYOUT iov - sgl mapping prerequisites
   vhost/scsi: Add ANY_LAYOUT vhost_virtqueue callback
   vhost/scsi: Set VIRTIO_F_ANY_LAYOUT + VIRTIO_F_VERSION_1 feature bits
   vhost/scsi: Drop legacy pre virtio v1.0 !ANY_LAYOUT logic
   vhost/scsi: Drop left-over scsi_tcq.h include
   vhost/scsi: Global tcm_vhost - vhost_scsi rename
 
  drivers/vhost/scsi.c | 1073 
 ++
  1 file changed, 549 insertions(+), 524 deletions(-)
 
 -- 
 1.9.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-v3 4/9] vhost/scsi: Add ANY_LAYOUT iov - sgl mapping prerequisites

[Michael S. Tsirkin]

On Tue, Feb 03, 2015 at 06:29:58AM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 This patch adds ANY_LAYOUT prerequisites logic for accepting a set of
 protection + data payloads via iov_iter.  Also includes helpers for
 calcuating SGLs + invoking vhost_scsi_map_to_sgl() with a known number
 of iovecs.
 
 Required by ANY_LAYOUT processing when struct iovec may be offset into
 the first outgoing virtio-scsi request header.
 
 v2 changes:
   - Clear -tvc_sgl_count for vhost_scsi_mapal failure
   - Make vhost_scsi_mapal + vhost_scsi_calc_sgls accept max_niov
   - Minor cleanups
 
 v3 changes:
   - Update vhost_scsi_mapal + friends to use iov_iter
   - Move iov_iter sanity checks into vhost_scsi_calc_sgls
   - Convert vhost_scsi_calc_sgls() to iov_iter_npages()

I guess if this goes through your tree, you can drop these
when applying, but it's best not to assume this, and put
changelog after ---.

 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Paolo Bonzini pbonz...@redhat.com
 Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
 ---
  drivers/vhost/scsi.c | 93 
 
  1 file changed, 93 insertions(+)
 
 diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
 index c3b12b3..7dfff15 100644
 --- a/drivers/vhost/scsi.c
 +++ b/drivers/vhost/scsi.c
 @@ -914,6 +914,99 @@ vhost_scsi_map_iov_to_prot(struct tcm_vhost_cmd *cmd,
   return 0;
  }
  
 +static int
 +vhost_scsi_calc_sgls(struct iov_iter *iter, size_t bytes, int max_sgls)
 +{
 + int sgl_count = 0;
 +
 + if (!iter || !iter-iov) {
 + pr_err(%s: iter-iov is NULL, but expected bytes: %zu
 + present\n, __func__, bytes);
 + return -EINVAL;
 + }
 +
 + sgl_count = iov_iter_npages(iter, 0x);
 + if (sgl_count  max_sgls) {
 + pr_err(%s: requested sgl_count: %d exceeds pre-allocated
 + max_sgls: %d\n, __func__, sgl_count, max_sgls);
 + return -EINVAL;
 + }
 + return sgl_count;
 +}
 +
 +static int
 +vhost_scsi_iov_to_sgl(struct tcm_vhost_cmd *cmd, bool write,
 +   struct iov_iter *iter, struct scatterlist *sg,
 +   int sg_count)
 +{
 + size_t off = iter-iov_offset;
 + int i, ret;
 +
 + for (i = 0; i  iter-nr_segs; i++) {
 + void __user *base = iter-iov[i].iov_base + off;
 + size_t len = iter-iov[i].iov_len - off;
 +
 + ret = vhost_scsi_map_to_sgl(cmd, base, len, sg, write);
 + if (ret  0) {
 + for (i = 0; i  sg_count; i++) {
 + struct page *page = sg_page(sg[i]);
 + if (page)
 + put_page(page);
 + }
 + return ret;
 + }
 + sg += ret;
 + off = 0;
 + }
 + return 0;
 +}
 +
 +static int
 +vhost_scsi_mapal(struct tcm_vhost_cmd *cmd,
 +  size_t prot_bytes, struct iov_iter *prot_iter,
 +  size_t data_bytes, struct iov_iter *data_iter)
 +{
 + int sgl_count, ret;
 + bool write = (cmd-tvc_data_direction == DMA_FROM_DEVICE);
 +
 + if (prot_bytes) {
 + sgl_count = vhost_scsi_calc_sgls(prot_iter, prot_bytes,
 +  TCM_VHOST_PREALLOC_PROT_SGLS);
 + if (sgl_count  0)
 + return sgl_count;
 +
 + sg_init_table(cmd-tvc_prot_sgl, sgl_count);
 + cmd-tvc_prot_sgl_count = sgl_count;
 + pr_debug(%s prot_sg %p prot_sgl_count %u\n, __func__,
 +  cmd-tvc_prot_sgl, cmd-tvc_prot_sgl_count);
 +
 + ret = vhost_scsi_iov_to_sgl(cmd, write, prot_iter,
 + cmd-tvc_prot_sgl,
 + cmd-tvc_prot_sgl_count);
 + if (ret  0) {
 + cmd-tvc_prot_sgl_count = 0;
 + return ret;
 + }
 + }
 + sgl_count = vhost_scsi_calc_sgls(data_iter, data_bytes,
 +  TCM_VHOST_PREALLOC_SGLS);
 + if (sgl_count  0)
 + return sgl_count;
 +
 + sg_init_table(cmd-tvc_sgl, sgl_count);
 + cmd-tvc_sgl_count = sgl_count;
 + pr_debug(%s data_sg %p data_sgl_count %u\n, __func__,
 +   cmd-tvc_sgl, cmd-tvc_sgl_count);
 +
 + ret = vhost_scsi_iov_to_sgl(cmd, write, data_iter,
 + cmd-tvc_sgl, cmd-tvc_sgl_count);
 + if (ret  0) {
 + cmd-tvc_sgl_count = 0;
 + return ret;
 + }
 + return 0;
 +}
 +
  static void tcm_vhost_submission_work(struct work_struct *work)
  {
   struct tcm_vhost_cmd *cmd =
 -- 
 1.9.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

Re: [PATCH-v3 7/9] vhost/scsi: Drop legacy pre virtio v1.0 !ANY_LAYOUT logic

[Michael S. Tsirkin]

On Tue, Feb 03, 2015 at 06:30:01AM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 With the new ANY_LAYOUT logic in place for vhost_scsi_handle_vqal(),
 there is no longer a reason to keep around the legacy code with
 !ANY_LAYOUT assumptions.
 
 Go ahead and drop the pre virtio 1.0 logic in vhost_scsi_handle_vq()
 and associated helpers.

Will probably be easier to review if you smash this with patch 5,
this way we see both old and new code side by side.

Also, let's rename _vqal to _vq in this patch?

 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Paolo Bonzini pbonz...@redhat.com
 Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
 ---
  drivers/vhost/scsi.c | 340 
 +--
  1 file changed, 1 insertion(+), 339 deletions(-)
 
 diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
 index c25fdd7..9af93d0 100644
 --- a/drivers/vhost/scsi.c
 +++ b/drivers/vhost/scsi.c
 @@ -830,93 +830,6 @@ out:
  }
  
  static int
 -vhost_scsi_map_iov_to_sgl(struct tcm_vhost_cmd *cmd,
 -   struct iovec *iov,
 -   int niov,
 -   bool write)
 -{
 - struct scatterlist *sg = cmd-tvc_sgl;
 - unsigned int sgl_count = 0;
 - int ret, i;
 -
 - for (i = 0; i  niov; i++)
 - sgl_count += iov_num_pages(iov[i].iov_base, iov[i].iov_len);
 -
 - if (sgl_count  TCM_VHOST_PREALLOC_SGLS) {
 - pr_err(vhost_scsi_map_iov_to_sgl() sgl_count: %u greater than
 -  preallocated TCM_VHOST_PREALLOC_SGLS: %u\n,
 - sgl_count, TCM_VHOST_PREALLOC_SGLS);
 - return -ENOBUFS;
 - }
 -
 - pr_debug(%s sg %p sgl_count %u\n, __func__, sg, sgl_count);
 - sg_init_table(sg, sgl_count);
 - cmd-tvc_sgl_count = sgl_count;
 -
 - pr_debug(Mapping iovec %p for %u pages\n, iov[0], sgl_count);
 -
 - for (i = 0; i  niov; i++) {
 - ret = vhost_scsi_map_to_sgl(cmd, iov[i].iov_base, 
 iov[i].iov_len,
 - sg, write);
 - if (ret  0) {
 - for (i = 0; i  cmd-tvc_sgl_count; i++) {
 - struct page *page = sg_page(cmd-tvc_sgl[i]);
 - if (page)
 - put_page(page);
 - }
 - cmd-tvc_sgl_count = 0;
 - return ret;
 - }
 - sg += ret;
 - sgl_count -= ret;
 - }
 - return 0;
 -}
 -
 -static int
 -vhost_scsi_map_iov_to_prot(struct tcm_vhost_cmd *cmd,
 -struct iovec *iov,
 -int niov,
 -bool write)
 -{
 - struct scatterlist *prot_sg = cmd-tvc_prot_sgl;
 - unsigned int prot_sgl_count = 0;
 - int ret, i;
 -
 - for (i = 0; i  niov; i++)
 - prot_sgl_count += iov_num_pages(iov[i].iov_base, 
 iov[i].iov_len);
 -
 - if (prot_sgl_count  TCM_VHOST_PREALLOC_PROT_SGLS) {
 - pr_err(vhost_scsi_map_iov_to_prot() sgl_count: %u greater than
 -  preallocated TCM_VHOST_PREALLOC_PROT_SGLS: %u\n,
 - prot_sgl_count, TCM_VHOST_PREALLOC_PROT_SGLS);
 - return -ENOBUFS;
 - }
 -
 - pr_debug(%s prot_sg %p prot_sgl_count %u\n, __func__,
 -  prot_sg, prot_sgl_count);
 - sg_init_table(prot_sg, prot_sgl_count);
 - cmd-tvc_prot_sgl_count = prot_sgl_count;
 -
 - for (i = 0; i  niov; i++) {
 - ret = vhost_scsi_map_to_sgl(cmd, iov[i].iov_base, 
 iov[i].iov_len,
 - prot_sg, write);
 - if (ret  0) {
 - for (i = 0; i  cmd-tvc_prot_sgl_count; i++) {
 - struct page *page = 
 sg_page(cmd-tvc_prot_sgl[i]);
 - if (page)
 - put_page(page);
 - }
 - cmd-tvc_prot_sgl_count = 0;
 - return ret;
 - }
 - prot_sg += ret;
 - prot_sgl_count -= ret;
 - }
 - return 0;
 -}
 -
 -static int
  vhost_scsi_calc_sgls(struct iov_iter *iter, size_t bytes, int max_sgls)
  {
   int sgl_count = 0;
 @@ -1323,254 +1236,6 @@ out:
   mutex_unlock(vq-mutex);
  }
  
 -static void
 -vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 -{
 - struct tcm_vhost_tpg **vs_tpg;
 - struct virtio_scsi_cmd_req v_req;
 - struct virtio_scsi_cmd_req_pi v_req_pi;
 - struct tcm_vhost_tpg *tpg;
 - struct tcm_vhost_cmd *cmd;
 - u64 tag;
 - u32 exp_data_len, data_first, data_num, data_direction, prot_first;
 - unsigned out, in, i;
 - int head, ret, data_niov, prot_niov, prot_bytes;
 - size_t req_size;
 - u16 lun;
 - u8 *target, *lunp, task_attr;
 - bool hdr_pi;
 - void *req, *cdb;
 -
 - mutex_lock

Re: [PATCH-v3 6/9] vhost/scsi: Set VIRTIO_F_ANY_LAYOUT + VIRTIO_F_VERSION_1 feature bits

[Michael S. Tsirkin]

On Tue, Feb 03, 2015 at 06:30:00AM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 Signal support of VIRTIO_F_ANY_LAYOUT + VIRTIO_F_VERSION_1 feature bits
 required for virtio-scsi 1.0 spec layout requirements.
 
 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Paolo Bonzini pbonz...@redhat.com
 Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org

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

I'm hoping someone'll actually test this with a BE guest before
we release this though.

 ---
  drivers/vhost/scsi.c | 9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
 index e1fe003..c25fdd7 100644
 --- a/drivers/vhost/scsi.c
 +++ b/drivers/vhost/scsi.c
 @@ -173,7 +173,9 @@ enum {
  /* Note: can't set VIRTIO_F_VERSION_1 yet, since that implies ANY_LAYOUT. */
  enum {
   VHOST_SCSI_FEATURES = VHOST_FEATURES | (1ULL  VIRTIO_SCSI_F_HOTPLUG) |
 -(1ULL  VIRTIO_SCSI_F_T10_PI)
 +(1ULL  VIRTIO_SCSI_F_T10_PI) |
 +(1ULL  VIRTIO_F_ANY_LAYOUT) |
 +(1ULL  VIRTIO_F_VERSION_1)
  };
  
  #define VHOST_SCSI_MAX_TARGET256
 @@ -1626,7 +1628,10 @@ static void vhost_scsi_handle_kick(struct vhost_work 
 *work)
   poll.work);
   struct vhost_scsi *vs = container_of(vq-dev, struct vhost_scsi, dev);
  
 - vhost_scsi_handle_vq(vs, vq);
 + if (vhost_has_feature(vq, VIRTIO_F_ANY_LAYOUT))
 + vhost_scsi_handle_vqal(vs, vq);
 + else
 + vhost_scsi_handle_vq(vs, vq);
  }
  
  static void vhost_scsi_flush_vq(struct vhost_scsi *vs, int index)
 -- 
 1.9.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 4/8] vhost/scsi: Change vhost_scsi_map_to_sgl to accept iov ptr + len

[Michael S. Tsirkin]

On Fri, Jan 30, 2015 at 08:12:28AM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 This patch changes vhost_scsi_map_to_sgl() parameters to accept virtio
 iovec ptr + len when determing pages_nr.
 
 This is currently done with iov_num_pages() - PAGE_ALIGN, so allow
 the same parameters as well.
 
 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Paolo Bonzini pbonz...@redhat.com
 Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org
 ---
  drivers/vhost/scsi.c | 37 +++--
  1 file changed, 15 insertions(+), 22 deletions(-)
 
 diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
 index 9c5ac23..049e603 100644
 --- a/drivers/vhost/scsi.c
 +++ b/drivers/vhost/scsi.c
 @@ -220,10 +220,10 @@ static struct workqueue_struct *tcm_vhost_workqueue;
  static DEFINE_MUTEX(tcm_vhost_mutex);
  static LIST_HEAD(tcm_vhost_list);
  
 -static int iov_num_pages(struct iovec *iov)
 +static int iov_num_pages(void __user *iov_base, size_t iov_len)
  {
 - return (PAGE_ALIGN((unsigned long)iov-iov_base + iov-iov_len) -
 -((unsigned long)iov-iov_base  PAGE_MASK))  PAGE_SHIFT;
 + return (PAGE_ALIGN((unsigned long)iov_base + iov_len) -
 +((unsigned long)iov_base  PAGE_MASK))  PAGE_SHIFT;
  }
  
  static void tcm_vhost_done_inflight(struct kref *kref)
 @@ -777,25 +777,18 @@ vhost_scsi_get_tag(struct vhost_virtqueue *vq, struct 
 tcm_vhost_tpg *tpg,
   * Returns the number of scatterlist entries used or -errno on error.
   */
  static int
 -vhost_scsi_map_to_sgl(struct tcm_vhost_cmd *tv_cmd,
 +vhost_scsi_map_to_sgl(struct tcm_vhost_cmd *cmd,
 +   void __user *ptr,
 +   size_t len,
 struct scatterlist *sgl,
 -   unsigned int sgl_count,
 -   struct iovec *iov,
 -   struct page **pages,
 bool write)
  {
 - unsigned int npages = 0, pages_nr, offset, nbytes;
 + unsigned int npages = 0, offset, nbytes;
 + unsigned int pages_nr = iov_num_pages(ptr, len);
   struct scatterlist *sg = sgl;
 - void __user *ptr = iov-iov_base;
 - size_t len = iov-iov_len;
 + struct page **pages = cmd-tvc_upages;
   int ret, i;
  
 - pages_nr = iov_num_pages(iov);
 - if (pages_nr  sgl_count) {
 - pr_err(vhost_scsi_map_to_sgl() pages_nr: %u greater than
 - sgl_count: %u\n, pages_nr, sgl_count);
 - return -ENOBUFS;
 - }
   if (pages_nr  TCM_VHOST_PREALLOC_UPAGES) {
   pr_err(vhost_scsi_map_to_sgl() pages_nr: %u greater than
   preallocated TCM_VHOST_PREALLOC_UPAGES: %u\n,
 @@ -840,7 +833,7 @@ vhost_scsi_map_iov_to_sgl(struct tcm_vhost_cmd *cmd,
   int ret, i;
  
   for (i = 0; i  niov; i++)
 - sgl_count += iov_num_pages(iov[i]);
 + sgl_count += iov_num_pages(iov[i].iov_base, iov[i].iov_len);
  

A helper function for this loop seems in order as well?


   if (sgl_count  TCM_VHOST_PREALLOC_SGLS) {
   pr_err(vhost_scsi_map_iov_to_sgl() sgl_count: %u greater than
 @@ -856,8 +849,8 @@ vhost_scsi_map_iov_to_sgl(struct tcm_vhost_cmd *cmd,
   pr_debug(Mapping iovec %p for %u pages\n, iov[0], sgl_count);
  
   for (i = 0; i  niov; i++) {
 - ret = vhost_scsi_map_to_sgl(cmd, sg, sgl_count, iov[i],
 - cmd-tvc_upages, write);
 + ret = vhost_scsi_map_to_sgl(cmd, iov[i].iov_base, 
 iov[i].iov_len,
 + sg, write);
   if (ret  0) {
   for (i = 0; i  cmd-tvc_sgl_count; i++) {
   struct page *page = sg_page(cmd-tvc_sgl[i]);
 @@ -884,7 +877,7 @@ vhost_scsi_map_iov_to_prot(struct tcm_vhost_cmd *cmd,
   int ret, i;
  
   for (i = 0; i  niov; i++)
 - prot_sgl_count += iov_num_pages(iov[i]);
 + prot_sgl_count += iov_num_pages(iov[i].iov_base, 
 iov[i].iov_len);
  
   if (prot_sgl_count  TCM_VHOST_PREALLOC_PROT_SGLS) {
   pr_err(vhost_scsi_map_iov_to_prot() sgl_count: %u greater than
 @@ -899,8 +892,8 @@ vhost_scsi_map_iov_to_prot(struct tcm_vhost_cmd *cmd,
   cmd-tvc_prot_sgl_count = prot_sgl_count;
  
   for (i = 0; i  niov; i++) {
 - ret = vhost_scsi_map_to_sgl(cmd, prot_sg, prot_sgl_count, 
 iov[i],
 - cmd-tvc_upages, write);
 + ret = vhost_scsi_map_to_sgl(cmd, iov[i].iov_base, 
 iov[i].iov_len,
 + prot_sg, write);
   if (ret  0) {
   for (i = 0; i  cmd-tvc_prot_sgl_count; i++) {
   struct page *page = 
 sg_page(cmd-tvc_prot_sgl[i]);
 -- 
 1.9.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

[PATCH v3 06/16] virtio/scsi: verify device has config space

[Michael S. Tsirkin]

Some devices might not implement config space access
(e.g. remoteproc used not to - before 3.9).
virtio/scsi needs config space access so make it
fail gracefully if not there.

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

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index c52bb5d..f164f24 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -950,6 +950,12 @@ static int virtscsi_probe(struct virtio_device *vdev)
u32 num_queues;
struct scsi_host_template *hostt;
 
+   if (!vdev-config-get) {
+   dev_err(vdev-dev, %s failure: config access disabled\n,
+   __func__);
+   return -EINVAL;
+   }
+
/* We need to know how many queues before we allocate. */
num_queues = virtscsi_config_get(vdev, num_queues) ? : 1;
 
-- 
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 5/6] virtio/scsi: verify device has config space

[Michael S. Tsirkin]

Some devices might not implement config space access
(e.g. remoteproc used not to - before 3.9).
virtio/scsi needs config space access so make it
fail gracefully if not there.

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

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index c52bb5d..f164f24 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -950,6 +950,12 @@ static int virtscsi_probe(struct virtio_device *vdev)
u32 num_queues;
struct scsi_host_template *hostt;
 
+   if (!vdev-config-get) {
+   dev_err(vdev-dev, %s failure: config access disabled\n,
+   __func__);
+   return -EINVAL;
+   }
+
/* We need to know how many queues before we allocate. */
num_queues = virtscsi_config_get(vdev, num_queues) ? : 1;
 
-- 
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] virtio-scsi: Fix the race condition in virtscsi_handle_event

[Michael S. Tsirkin]

On Tue, Jan 06, 2015 at 03:33:36PM +0800, Fam Zheng wrote:
 There is a race condition in virtscsi_handle_event, when many device
 hotplug/unplug events flush in quickly.
 
 The scsi_remove_device in virtscsi_handle_transport_reset may trigger
 the BUG_ON in scsi_target_reap, because the state is altered behind it,
 probably by scsi_scan_host of another event. I'm able to reproduce it by
 repeatedly plugging and unplugging a scsi disk with the same lun number.
 
 To make is safe, a single thread workqueue local to the module is added
 which runs the scan work. With this change, the panic goes away.
 
 Signed-off-by: Fam Zheng f...@redhat.com
 ---
 
 v2: Use a single threaded workqueue instead of mutex to serialize work
 (Venkatesh)
 ---
  drivers/scsi/virtio_scsi.c | 14 +-
  1 file changed, 13 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
 index c52bb5d..71b0091 100644
 --- a/drivers/scsi/virtio_scsi.c
 +++ b/drivers/scsi/virtio_scsi.c
 @@ -120,6 +120,8 @@ struct virtio_scsi {
  
  static struct kmem_cache *virtscsi_cmd_cache;
  static mempool_t *virtscsi_cmd_pool;
 +static struct workqueue_struct *virtscsi_scan_wq;
 +
  

Don't add two empty lines please.

  static inline struct Scsi_Host *virtio_scsi_host(struct virtio_device *vdev)
  {
 @@ -404,7 +406,7 @@ static void virtscsi_complete_event(struct virtio_scsi 
 *vscsi, void *buf)
   struct virtio_scsi_event_node *event_node = buf;
  
   if (!vscsi-stop_events)
 - queue_work(system_freezable_wq, event_node-work);
 + queue_work(virtscsi_scan_wq, event_node-work);
  }
  
  static void virtscsi_event_done(struct virtqueue *vq)
 @@ -1119,6 +1121,12 @@ static int __init init(void)
   pr_err(mempool_create() for virtscsi_cmd_pool failed\n);
   goto error;
   }
 + virtscsi_scan_wq = create_singlethread_workqueue(virtscsi-scan);
 + if (!virtscsi_scan_wq) {
 + pr_err(create_singlethread_workqueue() for virtscsi_scan_wq 
 failed\n);
 + goto error;
 + }
 +
   ret = register_virtio_driver(virtio_scsi_driver);
   if (ret  0)
   goto error;
 @@ -1126,6 +1134,9 @@ static int __init init(void)
   return 0;
  
  error:
 + if (virtscsi_scan_wq) {
 + destroy_workqueue(virtscsi_scan_wq);
 + }
   if (virtscsi_cmd_pool) {
   mempool_destroy(virtscsi_cmd_pool);
   virtscsi_cmd_pool = NULL;
 @@ -1142,6 +1153,7 @@ static void __exit fini(void)
   unregister_virtio_driver(virtio_scsi_driver);
   mempool_destroy(virtscsi_cmd_pool);
   kmem_cache_destroy(virtscsi_cmd_cache);
 + destroy_workqueue(virtscsi_scan_wq);


Please destroy in reverse order of initialization:
between unregister_virtio_driver and mempool_destroy.

  }
  module_init(init);
  module_exit(fini);
 -- 
 1.9.3
--
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] virtio-scsi: Fix the race condition in virtscsi_handle_event

[Michael S. Tsirkin]

On Tue, Jan 06, 2015 at 09:25:05PM +0800, Fam Zheng wrote:
 There is a race condition in virtscsi_handle_event, when many device
 hotplug/unplug events flush in quickly.
 
 The scsi_remove_device in virtscsi_handle_transport_reset may trigger
 the BUG_ON in scsi_target_reap, because the state is altered behind it,
 probably by scsi_scan_host of another event. I'm able to reproduce it by
 repeatedly plugging and unplugging a scsi disk with the same lun number.
 
 To fix this, a single thread workqueue (local to the module) is added,
 which makes the scan work serialized.

All wqs are serialized:

Note that the flag WQ_NON_REENTRANT no longer exists as all workqueues
are now non-reentrant - any work item is guaranteed to be executed by
at most one worker system-wide at any given time.


 With this change, the panic goes
 away.

I think the commit log is confusing.
serialization can not be an issue.

At a guess, what happens is two events are processed out of order,
so unplug bypasses another event, causing use after free errors.

With this in mind, your patch will indeed fix the issue, but
a better fix would be to call alloc_ordered_workqueue.

 Signed-off-by: Fam Zheng f...@redhat.com
 ---
 
 v3: Fix spacing and destroy order. (MST)
 ---
  drivers/scsi/virtio_scsi.c | 14 +-
  1 file changed, 13 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
 index c52bb5d..144bb73 100644
 --- a/drivers/scsi/virtio_scsi.c
 +++ b/drivers/scsi/virtio_scsi.c
 @@ -120,6 +120,7 @@ struct virtio_scsi {
  
  static struct kmem_cache *virtscsi_cmd_cache;
  static mempool_t *virtscsi_cmd_pool;
 +static struct workqueue_struct *virtscsi_scan_wq;
  
  static inline struct Scsi_Host *virtio_scsi_host(struct virtio_device *vdev)
  {
 @@ -404,7 +405,7 @@ static void virtscsi_complete_event(struct virtio_scsi 
 *vscsi, void *buf)
   struct virtio_scsi_event_node *event_node = buf;
  
   if (!vscsi-stop_events)
 - queue_work(system_freezable_wq, event_node-work);
 + queue_work(virtscsi_scan_wq, event_node-work);

This means wq isn't freezable anymore.

  }
  
  static void virtscsi_event_done(struct virtqueue *vq)
 @@ -1119,6 +1120,13 @@ static int __init init(void)
   pr_err(mempool_create() for virtscsi_cmd_pool failed\n);
   goto error;
   }
 +
 + virtscsi_scan_wq = create_singlethread_workqueue(virtscsi-scan);
 + if (!virtscsi_scan_wq) {
 + pr_err(create_singlethread_workqueue() for virtscsi_scan_wq 
 failed\n);
 + goto error;
 + }
 +

Documentation/workqueue.txt says:
alloc_workqueue() allocates a wq.  The original create_*workqueue()
functions are deprecated and scheduled for removal.  alloc_workqueue()
takes three arguments - @name, @flags and @max_active.


   ret = register_virtio_driver(virtio_scsi_driver);
   if (ret  0)
   goto error;
 @@ -1126,6 +1134,9 @@ static int __init init(void)
   return 0;
  
  error:
 + if (virtscsi_scan_wq) {
 + destroy_workqueue(virtscsi_scan_wq);
 + }

Single line, shouldn't use {} here.

   if (virtscsi_cmd_pool) {
   mempool_destroy(virtscsi_cmd_pool);
   virtscsi_cmd_pool = NULL;
 @@ -1140,6 +1151,7 @@ error:
  static void __exit fini(void)
  {
   unregister_virtio_driver(virtio_scsi_driver);
 + destroy_workqueue(virtscsi_scan_wq);
   mempool_destroy(virtscsi_cmd_pool);
   kmem_cache_destroy(virtscsi_cmd_cache);
  }
 -- 
 1.9.3
--
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: Add missing virtio-scsi - TCM attribute conversion

[Michael S. Tsirkin]

On Tue, Jan 06, 2015 at 08:18:05PM +, Nicholas A. Bellinger wrote:
 From: Nicholas Bellinger n...@linux-iscsi.org
 
 While looking at hch's recent conversion to drop the MSG_*_TAG
 definitions, I noticed a long standing bug in vhost-scsi where
 the VIRTIO_SCSI_S_* attribute definitions where incorrectly
 being passed directly into target_submit_cmd_map_sgls().
 
 This patch adds the missing virtio-scsi to TCM/SAM task attribute
 conversion.
 
 Cc: Christoph Hellwig h...@lst.de
 Cc: Michael S. Tsirkin m...@redhat.com
 Cc: Paolo Bonzini pbonz...@redhat.com
 Signed-off-by: Nicholas Bellinger n...@linux-iscsi.org

Cc stable as well?

 ---
  drivers/vhost/scsi.c | 24 +---
  1 file changed, 21 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
 index 01c01cb..d695b16 100644
 --- a/drivers/vhost/scsi.c
 +++ b/drivers/vhost/scsi.c
 @@ -911,6 +911,23 @@ vhost_scsi_map_iov_to_prot(struct tcm_vhost_cmd *cmd,
   return 0;
  }
  
 +static int vhost_scsi_to_tcm_attr(int attr)
 +{
 + switch (attr) {
 + case VIRTIO_SCSI_S_SIMPLE:
 + return TCM_SIMPLE_TAG;
 + case VIRTIO_SCSI_S_ORDERED:
 + return TCM_ORDERED_TAG;
 + case VIRTIO_SCSI_S_HEAD:
 + return TCM_HEAD_TAG;
 + case VIRTIO_SCSI_S_ACA:
 + return TCM_ACA_TAG;
 + default:
 + break;
 + }
 + return TCM_SIMPLE_TAG;
 +}
 +
  static void tcm_vhost_submission_work(struct work_struct *work)
  {
   struct tcm_vhost_cmd *cmd =
 @@ -936,9 +953,10 @@ static void tcm_vhost_submission_work(struct work_struct 
 *work)
   rc = target_submit_cmd_map_sgls(se_cmd, tv_nexus-tvn_se_sess,
   cmd-tvc_cdb, cmd-tvc_sense_buf[0],
   cmd-tvc_lun, cmd-tvc_exp_data_len,
 - cmd-tvc_task_attr, cmd-tvc_data_direction,
 - TARGET_SCF_ACK_KREF, sg_ptr, cmd-tvc_sgl_count,
 - NULL, 0, sg_prot_ptr, cmd-tvc_prot_sgl_count);
 + vhost_scsi_to_tcm_attr(cmd-tvc_task_attr),
 + cmd-tvc_data_direction, TARGET_SCF_ACK_KREF,
 + sg_ptr, cmd-tvc_sgl_count, NULL, 0, sg_prot_ptr,
 + cmd-tvc_prot_sgl_count);
   if (rc  0) {
   transport_send_check_condition_and_sense(se_cmd,
   TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE, 0);
 -- 
 1.9.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] virtio-scsi: Fix the race condition in virtscsi_handle_event

[Michael S. Tsirkin]

On Tue, Jan 06, 2015 at 12:10:59AM +0200, Michael S. Tsirkin wrote:
 On Mon, Jan 05, 2015 at 11:48:47AM -0800, Venkatesh Srinivas wrote:
  On Sun, Jan 4, 2015 at 10:04 PM, Fam Zheng f...@redhat.com wrote:
  
  There is a race condition in virtscsi_handle_event, when many device
  hotplug/unplug events flush in quickly.
  
  The scsi_remove_device in virtscsi_handle_transport_reset may trigger
  the BUG_ON in scsi_target_reap, because the state is altered behind it,
  probably by scsi_scan_host of another event. I'm able to reproduce it by
  repeatedly plugging and unplugging a scsi disk with the same lun number.
  
  To make is safe, the mutex added in struct virtio_scsi is held in
  virtscsi_handle_event, so that all the events are processed in a
  synchronized way. With this lock, the panic goes away.
  
  Signed-off-by: Fam Zheng f...@redhat.com
  ---
   drivers/scsi/virtio_scsi.c | 6 ++
   1 file changed, 6 insertions(+)
  
  diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
  index c52bb5d..7f194d4 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;
  
  +       /* Protect the hotplug/unplug event handling */
  +       struct mutex scan_lock;
  +
          /* Protected by event_vq lock */
          bool stop_events;
  
  @@ -377,6 +380,7 @@ static void virtscsi_handle_event(struct work_struct
  *work)
          struct virtio_scsi *vscsi = event_node-vscsi;
          struct virtio_scsi_event *event = event_node-event;
  
  +       mutex_lock(vscsi-scan_lock);
          if (event-event 
              cpu_to_virtio32(vscsi-vdev, VIRTIO_SCSI_T_EVENTS_MISSED)) {
                  event-event = ~cpu_to_virtio32(vscsi-vdev,
  @@ -397,6 +401,7 @@ static void virtscsi_handle_event(struct work_struct
  *work)
                  pr_err(Unsupport virtio scsi event %x\n, 
  event-event);
          }
          virtscsi_kick_event(vscsi, event_node);
  +       mutex_unlock(vscsi-scan_lock);
   }
  
   static void virtscsi_complete_event(struct virtio_scsi *vscsi, void 
  *buf)
  @@ -894,6 +899,7 @@ static int virtscsi_init(struct virtio_device *vdev,
          const char **names;
          struct virtqueue **vqs;
  
  +       mutex_init(vscsi-scan_lock);
          num_vqs = vscsi-num_queues + VIRTIO_SCSI_VQ_BASE;
          vqs = kmalloc(num_vqs * sizeof(struct virtqueue *), GFP_KERNEL);
          callbacks = kmalloc(num_vqs * sizeof(vq_callback_t *), 
  GFP_KERNEL);
  --
  1.9.3
  
  
  Nice find.
  
  This fix does have the effect of serializing all event handling via 
  scan_lock;
  perhaps you want to instead create a singlethreaded workqueue in virtio_scsi
  and queue handle_event there, rather than waiting on scan_lock on the system
  workqueue?
 
 Or use the system single-threaded wq.


I was sure we have one, but apparently not :(

Pls ignore the comment, sorry about the noise.

 
  Reviewed-by: Venkatesh Srinivas venkate...@google.com
  
  -- vs;
--
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] virtio-scsi: Fix the race condition in virtscsi_handle_event

[Michael S. Tsirkin]

On Mon, Jan 05, 2015 at 11:48:47AM -0800, Venkatesh Srinivas wrote:
 On Sun, Jan 4, 2015 at 10:04 PM, Fam Zheng f...@redhat.com wrote:
 
 There is a race condition in virtscsi_handle_event, when many device
 hotplug/unplug events flush in quickly.
 
 The scsi_remove_device in virtscsi_handle_transport_reset may trigger
 the BUG_ON in scsi_target_reap, because the state is altered behind it,
 probably by scsi_scan_host of another event. I'm able to reproduce it by
 repeatedly plugging and unplugging a scsi disk with the same lun number.
 
 To make is safe, the mutex added in struct virtio_scsi is held in
 virtscsi_handle_event, so that all the events are processed in a
 synchronized way. With this lock, the panic goes away.
 
 Signed-off-by: Fam Zheng f...@redhat.com
 ---
  drivers/scsi/virtio_scsi.c | 6 ++
  1 file changed, 6 insertions(+)
 
 diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
 index c52bb5d..7f194d4 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;
 
 +       /* Protect the hotplug/unplug event handling */
 +       struct mutex scan_lock;
 +
         /* Protected by event_vq lock */
         bool stop_events;
 
 @@ -377,6 +380,7 @@ static void virtscsi_handle_event(struct work_struct
 *work)
         struct virtio_scsi *vscsi = event_node-vscsi;
         struct virtio_scsi_event *event = event_node-event;
 
 +       mutex_lock(vscsi-scan_lock);
         if (event-event 
             cpu_to_virtio32(vscsi-vdev, VIRTIO_SCSI_T_EVENTS_MISSED)) {
                 event-event = ~cpu_to_virtio32(vscsi-vdev,
 @@ -397,6 +401,7 @@ static void virtscsi_handle_event(struct work_struct
 *work)
                 pr_err(Unsupport virtio scsi event %x\n, event-event);
         }
         virtscsi_kick_event(vscsi, event_node);
 +       mutex_unlock(vscsi-scan_lock);
  }
 
  static void virtscsi_complete_event(struct virtio_scsi *vscsi, void *buf)
 @@ -894,6 +899,7 @@ static int virtscsi_init(struct virtio_device *vdev,
         const char **names;
         struct virtqueue **vqs;
 
 +       mutex_init(vscsi-scan_lock);
         num_vqs = vscsi-num_queues + VIRTIO_SCSI_VQ_BASE;
         vqs = kmalloc(num_vqs * sizeof(struct virtqueue *), GFP_KERNEL);
         callbacks = kmalloc(num_vqs * sizeof(vq_callback_t *), 
 GFP_KERNEL);
 --
 1.9.3
 
 
 Nice find.
 
 This fix does have the effect of serializing all event handling via scan_lock;
 perhaps you want to instead create a singlethreaded workqueue in virtio_scsi
 and queue handle_event there, rather than waiting on scan_lock on the system
 workqueue?

Or use the system single-threaded wq.


 Reviewed-by: Venkatesh Srinivas venkate...@google.com
 
 -- vs;
--
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

[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

[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

[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

[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

[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 v5 41/45] virtio_scsi: v1.0 support

[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

[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 v4 38/42] virtio_scsi: v1.0 support

[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 v4 38/42] virtio_scsi: v1.0 support

[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 v3 37/41] virtio_scsi: v1.0 support

[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 10/25] virtio: add API to enable VQs early

[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

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

[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 13/25] virtio_console: enable VQs early

[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

[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

[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

[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 v4 04/25] virtio: defer config changed notifications

[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 v3 10/25] virtio: add API to enable VQs early

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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 24/25] virtio_scsi: drop scan callback

[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

[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

[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

[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

[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

[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