On Mon, Aug 13, 2012 at 3:24 PM, Pekka Enberg <[email protected]> wrote:
> On Thu, Aug 9, 2012 at 3:51 AM, Asias He <[email protected]> wrote:
>> This patch brings virito-scsi support to kvm tool.
>>
>> With the introduce of tcm_vhost (vhost-scsi)
>>
>> tcm_vhost: Initial merge for vhost level target fabric driver
>>
>> we can implement virito-scsi by simply having vhost-scsi to handle the
>> SCSI command.
>>
>> Howto use:
>> 1) Setup the tcm_vhost target through /sys/kernel/config
>>
>> [Stefan Hajnoczi, Thanks for the script to setup tcm_vhost]
>>
>> ** Setup wwpn and tpgt
>> $ wwpn="naa.0"
>> $ tpgt=/sys/kernel/config/target/vhost/$wwpn/tpgt_0
>> $ nexus=$tpgt/nexus
>> $ mkdir -p $tpgt
>> $ echo -n $wwpn > $nexus
>>
>> ** Setup lun using /dev/ram
>> $ n=0
>> $ lun=$tpgt/lun/lun_${n}
>> $ data=/sys/kernel/config/target/core/iblock_0/data_${n}
>> $ ram=/dev/ram${n}
>> $ mkdir -p $lun
>> $ mkdir -p $data
>> $ echo -n udev_path=${ram} > $data/control
>> $ echo -n 1 > $data/enable
>> $ ln -s $data $lun
>>
>> 2) Run kvm tool with the new disk option '-d scsi:$wwpn:$tpgt', e.g
>> $ lkvm run -k /boot/bzImage -d ~/img/sid.img -d scsi:naa.0:0
>>
>> Signed-off-by: Asias He <[email protected]>
>> Cc: Nicholas A. Bellinger <[email protected]>
>> Cc: Stefan Hajnoczi <[email protected]>
>> Cc: Paolo Bonzini <[email protected]>
>
> I've included some comments below but overall, looks good to me. Sasha?
>
>> ---
>> diff --git a/tools/kvm/include/kvm/disk-image.h
>> b/tools/kvm/include/kvm/disk-image.h
>> index 7ae17f8..54e4047 100644
>> --- a/tools/kvm/include/kvm/disk-image.h
>> +++ b/tools/kvm/include/kvm/disk-image.h
>> @@ -41,6 +41,8 @@ struct disk_image_operations {
>>
>> struct disk_image_params {
>> const char *filename;
>> + const char *wwpn;
>> + const char *tpgt;
>
> Maybe it's just me but "wwpn" and "tpgt" really could use a comment
> explaining what they are...
OK. They are terminology from linux target.
wwpn == world wide port number
tpgt == target port group tag
Nicholas, is this right?
>> bool readonly;
>> bool direct;
>> };
>> @@ -57,6 +59,8 @@ struct disk_image {
>> #ifdef CONFIG_HAS_AIO
>> io_context_t ctx;
>> #endif
>> + const char *wwpn;
>> + const char *tpgt;
>> };
>>
>> struct disk_image *disk_image__open(const char *filename, bool readonly,
>> bool direct);
>> diff --git a/tools/kvm/virtio/blk.c b/tools/kvm/virtio/blk.c
>> index 1fb969f..740442a 100644
>> --- a/tools/kvm/virtio/blk.c
>> +++ b/tools/kvm/virtio/blk.c
>> @@ -290,6 +290,8 @@ int virtio_blk__init(struct kvm *kvm)
>> int i, r = 0;
>>
>> for (i = 0; i < kvm->nr_disks; i++) {
>> + if (kvm->disks[i]->wwpn)
>> + continue;
>> r = virtio_blk__init_one(kvm, kvm->disks[i]);
>> if (r < 0)
>> goto cleanup;
>> diff --git a/tools/kvm/virtio/scsi.c b/tools/kvm/virtio/scsi.c
>> new file mode 100644
>> index 0000000..5bcb00c
>> --- /dev/null
>> +++ b/tools/kvm/virtio/scsi.c
>> @@ -0,0 +1,332 @@
>> +#include "kvm/virtio-scsi.h"
>> +#include "kvm/virtio-pci-dev.h"
>> +#include "kvm/disk-image.h"
>> +#include "kvm/kvm.h"
>> +#include "kvm/pci.h"
>> +#include "kvm/ioeventfd.h"
>> +#include "kvm/guest_compat.h"
>> +#include "kvm/virtio-pci.h"
>> +#include "kvm/virtio.h"
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/virtio_scsi.h>
>> +#include <linux/vhost.h>
>> +
>> +/*----------------------------------------------------*/
>> +/* TODO: Remove this when tcm_vhost goes upstream */
>> +#define TRANSPORT_IQN_LEN 224
>> +#define VHOST_SCSI_ABI_VERSION 0
>> +struct vhost_scsi_target {
>> + int abi_version;
>> + unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
>> + unsigned short vhost_tpgt;
>> +};
>> +/* VHOST_SCSI specific defines */
>> +#define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct
>> vhost_scsi_target)
>> +#define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct
>> vhost_scsi_target)
>> +#define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, struct
>> vhost_scsi_target)
>> +/*----------------------------------------------------*/
>
> You might as well move the above block into a small helper header file.
How about include/kvm/virtio-scsi.h ?
>> +
>> +
>> +#define VIRTIO_SCSI_QUEUE_SIZE 128
>> +#define NUM_VIRT_QUEUES 3
>> +
>> +static LIST_HEAD(sdevs);
>> +static int compat_id = -1;
>> +
>> +struct scsi_dev {
>> + struct virt_queue vqs[NUM_VIRT_QUEUES];
>> + struct virtio_scsi_config scsi_config;
>> + struct vhost_scsi_target target;
>> + u32 features;
>> + int vhost_fd;
>> + struct virtio_device vdev;
>> + struct list_head list;
>> + struct kvm *kvm;
>> +};
>> +
>> +static void set_config(struct kvm *kvm, void *dev, u8 data, u32 offset)
>> +{
>> + struct scsi_dev *sdev = dev;
>> +
>> + ((u8 *)(&sdev->scsi_config))[offset] = data;
>
> Can you introduce a helper function for this, please?
We have this similar code in other devices' get_config, get_config as well.
Let's clean them up after this patch? Also, I have plans to clean up
the vhost code.
>
>> +}
>> +
>> +static u8 get_config(struct kvm *kvm, void *dev, u32 offset)
>> +{
>> + struct scsi_dev *sdev = dev;
>> +
>> + return ((u8 *)(&sdev->scsi_config))[offset];
>
> Ditto.
>
>> +}
>> +
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Asias He
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
