diff mbox

[v2,10/17] virtio-scsi: use standard-headers

Message ID 1423999136-17320-11-git-send-email-mst@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin Feb. 15, 2015, 11:39 a.m. UTC
Drop duplicated code.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/virtio/virtio-scsi.h | 120 +++-------------------------------------
 hw/scsi/virtio-scsi.c           |   1 +
 2 files changed, 10 insertions(+), 111 deletions(-)

Comments

Paolo Bonzini Feb. 16, 2015, 9:30 a.m. UTC | #1
On 15/02/2015 12:39, Michael S. Tsirkin wrote:
> Drop duplicated code.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/virtio/virtio-scsi.h | 120 +++-------------------------------------
>  hw/scsi/virtio-scsi.c           |   1 +
>  2 files changed, 10 insertions(+), 111 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index bf17cc9..9bcda7e 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -14,6 +14,7 @@
>  #ifndef _QEMU_VIRTIO_SCSI_H
>  #define _QEMU_VIRTIO_SCSI_H
>  
> +#include "standard-headers/sys/virtio_scsi.h"

Why sys/?  It's linux/, let's keep it linux/.

Paolo
Michael S. Tsirkin Feb. 16, 2015, 11:36 a.m. UTC | #2
On Mon, Feb 16, 2015 at 10:30:24AM +0100, Paolo Bonzini wrote:
> 
> 
> On 15/02/2015 12:39, Michael S. Tsirkin wrote:
> > Drop duplicated code.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/hw/virtio/virtio-scsi.h | 120 +++-------------------------------------
> >  hw/scsi/virtio-scsi.c           |   1 +
> >  2 files changed, 10 insertions(+), 111 deletions(-)
> > 
> > diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> > index bf17cc9..9bcda7e 100644
> > --- a/include/hw/virtio/virtio-scsi.h
> > +++ b/include/hw/virtio/virtio-scsi.h
> > @@ -14,6 +14,7 @@
> >  #ifndef _QEMU_VIRTIO_SCSI_H
> >  #define _QEMU_VIRTIO_SCSI_H
> >  
> > +#include "standard-headers/sys/virtio_scsi.h"
> 
> Why sys/?  It's linux/, let's keep it linux/.
> 
> Paolo

Peter requested this change: he felt having portable
headers under linux/ is confusing:

http://mid.gmane.org/CAFEAcA8QFTWbBZSPjCKzEBBwWZojJL+LDKTPL_F=eCDpDHj=Zw@mail.gmail.com

Makes sense?
Peter Maydell Feb. 16, 2015, 11:45 a.m. UTC | #3
On 16 February 2015 at 11:36, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Feb 16, 2015 at 10:30:24AM +0100, Paolo Bonzini wrote:
>>
>>
>> On 15/02/2015 12:39, Michael S. Tsirkin wrote:
>> > Drop duplicated code.
>> >
>> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> > ---
>> >  include/hw/virtio/virtio-scsi.h | 120 +++-------------------------------------
>> >  hw/scsi/virtio-scsi.c           |   1 +
>> >  2 files changed, 10 insertions(+), 111 deletions(-)
>> >
>> > diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
>> > index bf17cc9..9bcda7e 100644
>> > --- a/include/hw/virtio/virtio-scsi.h
>> > +++ b/include/hw/virtio/virtio-scsi.h
>> > @@ -14,6 +14,7 @@
>> >  #ifndef _QEMU_VIRTIO_SCSI_H
>> >  #define _QEMU_VIRTIO_SCSI_H
>> >
>> > +#include "standard-headers/sys/virtio_scsi.h"
>>
>> Why sys/?  It's linux/, let's keep it linux/.
>>
>> Paolo
>
> Peter requested this change: he felt having portable
> headers under linux/ is confusing:

My point was that you don't want to be including
these headers via "linux/whatever.h";
#include "standard-headers/linux/whatever.h" would be
fine.

-- PMM
Paolo Bonzini Feb. 16, 2015, 2:27 p.m. UTC | #4
On 16/02/2015 12:45, Peter Maydell wrote:
> My point was that you don't want to be including
> these headers via "linux/whatever.h";
> #include "standard-headers/linux/whatever.h" would be
> fine.

I agree on both counts.

Paolo
Alexey Kardashevskiy March 11, 2015, 10:13 a.m. UTC | #5
Hi!

This particular patch broke virtio-scsi in SLOF (ppc64-server firmware), 
QEMU just exits:

Populating /pci@800000020000000/scsi@0
        SCSI: Looking for devices
qemu-system-ppc64: wrong size for virtio-scsi headers


This is how I run it:

-device virtio-scsi-pci,id=id3 \
-drive id=id4,if=none,file=virtimg/rhel7_24GB.qcow2 \
-device scsi-disk,id=id5,drive=id4

It is bigendian, kvm or tcg.


Any quick idea? Thanks :)



On 02/16/2015 10:36 PM, Michael S. Tsirkin wrote:
> On Mon, Feb 16, 2015 at 10:30:24AM +0100, Paolo Bonzini wrote:
>>
>>
>> On 15/02/2015 12:39, Michael S. Tsirkin wrote:
>>> Drop duplicated code.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>   include/hw/virtio/virtio-scsi.h | 120 +++-------------------------------------
>>>   hw/scsi/virtio-scsi.c           |   1 +
>>>   2 files changed, 10 insertions(+), 111 deletions(-)
>>>
>>> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
>>> index bf17cc9..9bcda7e 100644
>>> --- a/include/hw/virtio/virtio-scsi.h
>>> +++ b/include/hw/virtio/virtio-scsi.h
>>> @@ -14,6 +14,7 @@
>>>   #ifndef _QEMU_VIRTIO_SCSI_H
>>>   #define _QEMU_VIRTIO_SCSI_H
>>>
>>> +#include "standard-headers/sys/virtio_scsi.h"
>>
>> Why sys/?  It's linux/, let's keep it linux/.
>>
>> Paolo
>
> Peter requested this change: he felt having portable
> headers under linux/ is confusing:
>
> http://mid.gmane.org/CAFEAcA8QFTWbBZSPjCKzEBBwWZojJL+LDKTPL_F=eCDpDHj=Zw@mail.gmail.com
>
> Makes sense?
>
Alexey Kardashevskiy March 11, 2015, 10:57 a.m. UTC | #6
On 03/11/2015 09:13 PM, Alexey Kardashevskiy wrote:
> Hi!
>
> This particular patch broke virtio-scsi in SLOF (ppc64-server firmware),
> QEMU just exits:

Oops, there was v3 and Nikunj asked there. Ignore this.


>
> Populating /pci@800000020000000/scsi@0
>         SCSI: Looking for devices
> qemu-system-ppc64: wrong size for virtio-scsi headers
>
>
> This is how I run it:
>
> -device virtio-scsi-pci,id=id3 \
> -drive id=id4,if=none,file=virtimg/rhel7_24GB.qcow2 \
> -device scsi-disk,id=id5,drive=id4
>
> It is bigendian, kvm or tcg.
>
>
> Any quick idea? Thanks :)
>
>
>
> On 02/16/2015 10:36 PM, Michael S. Tsirkin wrote:
>> On Mon, Feb 16, 2015 at 10:30:24AM +0100, Paolo Bonzini wrote:
>>>
>>>
>>> On 15/02/2015 12:39, Michael S. Tsirkin wrote:
>>>> Drop duplicated code.
>>>>
>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>> ---
>>>>   include/hw/virtio/virtio-scsi.h | 120
>>>> +++-------------------------------------
>>>>   hw/scsi/virtio-scsi.c           |   1 +
>>>>   2 files changed, 10 insertions(+), 111 deletions(-)
>>>>
>>>> diff --git a/include/hw/virtio/virtio-scsi.h
>>>> b/include/hw/virtio/virtio-scsi.h
>>>> index bf17cc9..9bcda7e 100644
>>>> --- a/include/hw/virtio/virtio-scsi.h
>>>> +++ b/include/hw/virtio/virtio-scsi.h
>>>> @@ -14,6 +14,7 @@
>>>>   #ifndef _QEMU_VIRTIO_SCSI_H
>>>>   #define _QEMU_VIRTIO_SCSI_H
>>>>
>>>> +#include "standard-headers/sys/virtio_scsi.h"
>>>
>>> Why sys/?  It's linux/, let's keep it linux/.
>>>
>>> Paolo
>>
>> Peter requested this change: he felt having portable
>> headers under linux/ is confusing:
>>
>> http://mid.gmane.org/CAFEAcA8QFTWbBZSPjCKzEBBwWZojJL+LDKTPL_F=eCDpDHj=Zw@mail.gmail.com
>>
>>
>> Makes sense?
>>
>
>
Michael S. Tsirkin March 11, 2015, 12:04 p.m. UTC | #7
On Wed, Mar 11, 2015 at 09:13:18PM +1100, Alexey Kardashevskiy wrote:
> Hi!
> 
> This particular patch broke virtio-scsi in SLOF (ppc64-server firmware),
> QEMU just exits:
> 
> Populating /pci@800000020000000/scsi@0
>        SCSI: Looking for devices
> qemu-system-ppc64: wrong size for virtio-scsi headers
> 
> 
> This is how I run it:
> 
> -device virtio-scsi-pci,id=id3 \
> -drive id=id4,if=none,file=virtimg/rhel7_24GB.qcow2 \
> -device scsi-disk,id=id5,drive=id4
> 
> It is bigendian, kvm or tcg.
> 
> 
> Any quick idea? Thanks :)


Can you please try this patch:
mid.gmane.org/20150311101858-mutt-send-email-mst@redhat.com

Thanks!

> 
> 
> On 02/16/2015 10:36 PM, Michael S. Tsirkin wrote:
> >On Mon, Feb 16, 2015 at 10:30:24AM +0100, Paolo Bonzini wrote:
> >>
> >>
> >>On 15/02/2015 12:39, Michael S. Tsirkin wrote:
> >>>Drop duplicated code.
> >>>
> >>>Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>>---
> >>>  include/hw/virtio/virtio-scsi.h | 120 +++-------------------------------------
> >>>  hw/scsi/virtio-scsi.c           |   1 +
> >>>  2 files changed, 10 insertions(+), 111 deletions(-)
> >>>
> >>>diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> >>>index bf17cc9..9bcda7e 100644
> >>>--- a/include/hw/virtio/virtio-scsi.h
> >>>+++ b/include/hw/virtio/virtio-scsi.h
> >>>@@ -14,6 +14,7 @@
> >>>  #ifndef _QEMU_VIRTIO_SCSI_H
> >>>  #define _QEMU_VIRTIO_SCSI_H
> >>>
> >>>+#include "standard-headers/sys/virtio_scsi.h"
> >>
> >>Why sys/?  It's linux/, let's keep it linux/.
> >>
> >>Paolo
> >
> >Peter requested this change: he felt having portable
> >headers under linux/ is confusing:
> >
> >http://mid.gmane.org/CAFEAcA8QFTWbBZSPjCKzEBBwWZojJL+LDKTPL_F=eCDpDHj=Zw@mail.gmail.com
> >
> >Makes sense?
> >
> 
> 
> -- 
> Alexey
Alexey Kardashevskiy March 11, 2015, 12:52 p.m. UTC | #8
On 03/11/2015 11:04 PM, Michael S. Tsirkin wrote:
> On Wed, Mar 11, 2015 at 09:13:18PM +1100, Alexey Kardashevskiy wrote:
>> Hi!
>>
>> This particular patch broke virtio-scsi in SLOF (ppc64-server firmware),
>> QEMU just exits:
>>
>> Populating /pci@800000020000000/scsi@0
>>         SCSI: Looking for devices
>> qemu-system-ppc64: wrong size for virtio-scsi headers
>>
>>
>> This is how I run it:
>>
>> -device virtio-scsi-pci,id=id3 \
>> -drive id=id4,if=none,file=virtimg/rhel7_24GB.qcow2 \
>> -device scsi-disk,id=id5,drive=id4
>>
>> It is bigendian, kvm or tcg.
>>
>>
>> Any quick idea? Thanks :)
>
>
> Can you please try this patch:
> mid.gmane.org/20150311101858-mutt-send-email-mst@redhat.com


Tried, it helped. I also replied (the same) in the patch's mail thread.

>
> Thanks!
>
>>
>>
>> On 02/16/2015 10:36 PM, Michael S. Tsirkin wrote:
>>> On Mon, Feb 16, 2015 at 10:30:24AM +0100, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 15/02/2015 12:39, Michael S. Tsirkin wrote:
>>>>> Drop duplicated code.
>>>>>
>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> ---
>>>>>   include/hw/virtio/virtio-scsi.h | 120 +++-------------------------------------
>>>>>   hw/scsi/virtio-scsi.c           |   1 +
>>>>>   2 files changed, 10 insertions(+), 111 deletions(-)
>>>>>
>>>>> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
>>>>> index bf17cc9..9bcda7e 100644
>>>>> --- a/include/hw/virtio/virtio-scsi.h
>>>>> +++ b/include/hw/virtio/virtio-scsi.h
>>>>> @@ -14,6 +14,7 @@
>>>>>   #ifndef _QEMU_VIRTIO_SCSI_H
>>>>>   #define _QEMU_VIRTIO_SCSI_H
>>>>>
>>>>> +#include "standard-headers/sys/virtio_scsi.h"
>>>>
>>>> Why sys/?  It's linux/, let's keep it linux/.
>>>>
>>>> Paolo
>>>
>>> Peter requested this change: he felt having portable
>>> headers under linux/ is confusing:
>>>
>>> http://mid.gmane.org/CAFEAcA8QFTWbBZSPjCKzEBBwWZojJL+LDKTPL_F=eCDpDHj=Zw@mail.gmail.com
>>>
>>> Makes sense?
>>>
>>
>>
>> --
>> Alexey
diff mbox

Patch

diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index bf17cc9..9bcda7e 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -14,6 +14,7 @@ 
 #ifndef _QEMU_VIRTIO_SCSI_H
 #define _QEMU_VIRTIO_SCSI_H
 
+#include "standard-headers/sys/virtio_scsi.h"
 #include "hw/virtio/virtio.h"
 #include "hw/pci/pci.h"
 #include "hw/scsi/scsi.h"
@@ -28,15 +29,6 @@ 
 #define VIRTIO_SCSI(obj) \
         OBJECT_CHECK(VirtIOSCSI, (obj), TYPE_VIRTIO_SCSI)
 
-
-/* The ID for virtio_scsi */
-#define VIRTIO_ID_SCSI  8
-
-/* Feature Bits */
-#define VIRTIO_SCSI_F_INOUT                    0
-#define VIRTIO_SCSI_F_HOTPLUG                  1
-#define VIRTIO_SCSI_F_CHANGE                   2
-
 #define VIRTIO_SCSI_VQ_SIZE     128
 #define VIRTIO_SCSI_CDB_SIZE    32
 #define VIRTIO_SCSI_SENSE_SIZE  96
@@ -44,108 +36,14 @@ 
 #define VIRTIO_SCSI_MAX_TARGET  255
 #define VIRTIO_SCSI_MAX_LUN     16383
 
-/* Response codes */
-#define VIRTIO_SCSI_S_OK                       0
-#define VIRTIO_SCSI_S_OVERRUN                  1
-#define VIRTIO_SCSI_S_ABORTED                  2
-#define VIRTIO_SCSI_S_BAD_TARGET               3
-#define VIRTIO_SCSI_S_RESET                    4
-#define VIRTIO_SCSI_S_BUSY                     5
-#define VIRTIO_SCSI_S_TRANSPORT_FAILURE        6
-#define VIRTIO_SCSI_S_TARGET_FAILURE           7
-#define VIRTIO_SCSI_S_NEXUS_FAILURE            8
-#define VIRTIO_SCSI_S_FAILURE                  9
-#define VIRTIO_SCSI_S_FUNCTION_SUCCEEDED       10
-#define VIRTIO_SCSI_S_FUNCTION_REJECTED        11
-#define VIRTIO_SCSI_S_INCORRECT_LUN            12
-
-/* Controlq type codes.  */
-#define VIRTIO_SCSI_T_TMF                      0
-#define VIRTIO_SCSI_T_AN_QUERY                 1
-#define VIRTIO_SCSI_T_AN_SUBSCRIBE             2
-
-/* Valid TMF subtypes.  */
-#define VIRTIO_SCSI_T_TMF_ABORT_TASK           0
-#define VIRTIO_SCSI_T_TMF_ABORT_TASK_SET       1
-#define VIRTIO_SCSI_T_TMF_CLEAR_ACA            2
-#define VIRTIO_SCSI_T_TMF_CLEAR_TASK_SET       3
-#define VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET      4
-#define VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET   5
-#define VIRTIO_SCSI_T_TMF_QUERY_TASK           6
-#define VIRTIO_SCSI_T_TMF_QUERY_TASK_SET       7
-
-/* Events.  */
-#define VIRTIO_SCSI_T_EVENTS_MISSED            0x80000000
-#define VIRTIO_SCSI_T_NO_EVENT                 0
-#define VIRTIO_SCSI_T_TRANSPORT_RESET          1
-#define VIRTIO_SCSI_T_ASYNC_NOTIFY             2
-#define VIRTIO_SCSI_T_PARAM_CHANGE             3
-
-/* Reasons for transport reset event */
-#define VIRTIO_SCSI_EVT_RESET_HARD             0
-#define VIRTIO_SCSI_EVT_RESET_RESCAN           1
-#define VIRTIO_SCSI_EVT_RESET_REMOVED          2
-
-/* SCSI command request, followed by CDB and data-out */
-typedef struct {
-    uint8_t lun[8];              /* Logical Unit Number */
-    uint64_t tag;                /* Command identifier */
-    uint8_t task_attr;           /* Task attribute */
-    uint8_t prio;
-    uint8_t crn;
-} QEMU_PACKED VirtIOSCSICmdReq;
-
-/* Response, followed by sense data and data-in */
-typedef struct {
-    uint32_t sense_len;          /* Sense data length */
-    uint32_t resid;              /* Residual bytes in data buffer */
-    uint16_t status_qualifier;   /* Status qualifier */
-    uint8_t status;              /* Command completion status */
-    uint8_t response;            /* Response values */
-} QEMU_PACKED VirtIOSCSICmdResp;
-
-/* Task Management Request */
-typedef struct {
-    uint32_t type;
-    uint32_t subtype;
-    uint8_t lun[8];
-    uint64_t tag;
-} QEMU_PACKED VirtIOSCSICtrlTMFReq;
-
-typedef struct {
-    uint8_t response;
-} QEMU_PACKED VirtIOSCSICtrlTMFResp;
-
-/* Asynchronous notification query/subscription */
-typedef struct {
-    uint32_t type;
-    uint8_t lun[8];
-    uint32_t event_requested;
-} QEMU_PACKED VirtIOSCSICtrlANReq;
-
-typedef struct {
-    uint32_t event_actual;
-    uint8_t response;
-} QEMU_PACKED VirtIOSCSICtrlANResp;
-
-typedef struct {
-    uint32_t event;
-    uint8_t lun[8];
-    uint32_t reason;
-} QEMU_PACKED VirtIOSCSIEvent;
-
-typedef struct {
-    uint32_t num_queues;
-    uint32_t seg_max;
-    uint32_t max_sectors;
-    uint32_t cmd_per_lun;
-    uint32_t event_info_size;
-    uint32_t sense_size;
-    uint32_t cdb_size;
-    uint16_t max_channel;
-    uint16_t max_target;
-    uint32_t max_lun;
-} QEMU_PACKED VirtIOSCSIConfig;
+typedef struct virtio_scsi_cmd_req VirtIOSCSICmdReq;
+typedef struct virtio_scsi_cmd_resp VirtIOSCSICmdResp;
+typedef struct virtio_scsi_ctrl_tmf_req VirtIOSCSICtrlTMFReq;
+typedef struct virtio_scsi_ctrl_tmf_resp VirtIOSCSICtrlTMFResp;
+typedef struct virtio_scsi_ctrl_an_req VirtIOSCSICtrlANReq;
+typedef struct virtio_scsi_ctrl_an_resp VirtIOSCSICtrlANResp;
+typedef struct virtio_scsi_event VirtIOSCSIEvent;
+typedef struct virtio_scsi_config VirtIOSCSIConfig;
 
 struct VirtIOSCSIConf {
     uint32_t num_queues;
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 9e2c718..da4c215 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -13,6 +13,7 @@ 
  *
  */
 
+#include "standard-headers/sys/virtio_ids.h"
 #include "hw/virtio/virtio-scsi.h"
 #include "qemu/error-report.h"
 #include "qemu/iov.h"