Re: [PATCH v1 5/8] virtio-ccw: Handle extra notification data

2024-03-11 Thread Eric Farman 
On Mon, 2024-03-04 at 14:46 -0500, Jonah Palmer wrote:
> > Add support to virtio-ccw devices for handling the extra data sent
> > from
> > the driver to the device when the VIRTIO_F_NOTIFICATION_DATA
> > transport
> > feature has been negotiated.
> > 
> > The extra data that's passed to the virtio-ccw device when this
> > feature
> > is enabled varies depending on the device's virtqueue layout.
> > 
> > That data passed to the virtio-ccw device is in the same format as
> > the
> > data passed to virtio-pci devices.
> > 
> > Acked-by: Thomas Huth 
> > Signed-off-by: Jonah Palmer 
> > ---
> >  hw/s390x/s390-virtio-ccw.c | 16 
> >  1 file changed, 12 insertions(+), 4 deletions(-)

Acked-by: Eric Farman 

(I see a v2 is coming for the ioeventfd side, but I was going through
this series today and thought that would affect the next patch rather
than this one.)

Re: [PATCH 1/6] system/cpus: rename qemu_mutex_lock_iothread() to qemu_bql_lock()

2023-11-30 Thread Eric Farman 
On Wed, 2023-11-29 at 16:26 -0500, Stefan Hajnoczi wrote:
> The Big QEMU Lock (BQL) has many names and they are confusing. The
> actual QemuMutex variable is called qemu_global_mutex but it's
> commonly
> referred to as the BQL in discussions and some code comments. The
> locking APIs, however, are called qemu_mutex_lock_iothread() and
> qemu_mutex_unlock_iothread().
> 
> The "iothread" name is historic and comes from when the main thread
> was
> split into into KVM vcpu threads and the "iothread" (now called the
> main
> loop thread). I have contributed to the confusion myself by
> introducing
> a separate --object iothread, a separate concept unrelated to the
> BQL.
> 
> The "iothread" name is no longer appropriate for the BQL. Rename the
> locking APIs to:
> - void qemu_bql_lock(void)
> - void qemu_bql_unlock(void)
> - bool qemu_bql_locked(void)
> 
> There are more APIs with "iothread" in their names. Subsequent
> patches
> will rename them. There are also comments and documentation that will
> be
> updated in later patches.
> 
> Signed-off-by: Stefan Hajnoczi 

Acked-by: Eric Farman

Re: [PATCH v2 16/19] hw/vfio/ccw: Replace DO_UPCAST(VFIOCCWDevice) by VFIO_CCW()

2023-02-13 Thread Eric Farman 
On Mon, 2023-02-13 at 17:10 +0100, Philippe Mathieu-Daudé wrote:
> On 13/2/23 16:51, Philippe Mathieu-Daudé wrote:
> > On 13/2/23 16:29, Eric Farman wrote:
> > > On Mon, 2023-02-13 at 08:08 +0100, Philippe Mathieu-Daudé wrote:
> > > > Use the VFIO_CCW() QOM type-checking macro to avoid
> > > > DO_UPCAST().
> > > > 
> > > > Signed-off-by: Philippe Mathieu-Daudé 
> > > > ---
> > > >   hw/vfio/ccw.c | 35 ---
> > > >   1 file changed, 16 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> > > > index 0354737666..a8aa5b48c4 100644
> > > > --- a/hw/vfio/ccw.c
> > > > +++ b/hw/vfio/ccw.c
> > > 
> > > ...snip...
> > > 
> > > > @@ -252,8 +248,8 @@ again:
> > > >   static void vfio_ccw_reset(DeviceState *dev)
> > > >   {
> > > >   CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj,
> > > > dev);
> > > 
> > > If I'm not mistaken, I believe that this (and (un)realize below)
> > > could
> > > be changed to:
> > > 
> > >     CcwDevice *ccw_dev = CCW_DEVICE(dev);
> > 
> > Even ...
> > 
> > > > -    S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj,
> > > > ccw_dev);
> > > > -    VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev,
> > > > cdev);
> > > > +    S390CCWDevice *cdev = S390_CCW_DEVICE(ccw_dev);
> > > > +    VFIOCCWDevice *vcdev = VFIO_CCW(cdev);
> > 
> >  VFIOCCWDevice *vcdev = VFIO_CCW(dev);

Ha, I didn't look to see if we cared about the intermediary ones, but
this is true here. (Realize cares a bit, but that's easy enough.)

> > 
> > But I somehow got scared to of removing too many casts...
> > 
> > Are these paths covered by a "make check-qtest" on a s390x host?
> 
> They are covered by the Avocado tests :)
> 
> $ avocado --show=app,console run -t arch:s390x tests/avocado
> 

Woo! Then I'm happy with the big squash then.

Reviewed-by: Eric Farman

Re: [PATCH v2 16/19] hw/vfio/ccw: Replace DO_UPCAST(VFIOCCWDevice) by VFIO_CCW()

2023-02-13 Thread Eric Farman 
On Mon, 2023-02-13 at 08:08 +0100, Philippe Mathieu-Daudé wrote:
> Use the VFIO_CCW() QOM type-checking macro to avoid DO_UPCAST().
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/vfio/ccw.c | 35 ---
>  1 file changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 0354737666..a8aa5b48c4 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c

...snip...

> @@ -252,8 +248,8 @@ again:
>  static void vfio_ccw_reset(DeviceState *dev)
>  {
>  CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev);

If I'm not mistaken, I believe that this (and (un)realize below) could
be changed to:

   CcwDevice *ccw_dev = CCW_DEVICE(dev);

> -    S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj,
> ccw_dev);
> -    VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
> +    S390CCWDevice *cdev = S390_CCW_DEVICE(ccw_dev);
> +    VFIOCCWDevice *vcdev = VFIO_CCW(cdev);
>  
>  ioctl(vcdev->vdev.fd, VFIO_DEVICE_RESET);
>  }

...snip...

> @@ -657,9 +654,9 @@ static void vfio_ccw_realize(DeviceState *dev,
> Error **errp)
>  {
>  VFIOGroup *group;
>  CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev);
> -    S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj,
> ccw_dev);
> -    VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
> +    S390CCWDevice *cdev = S390_CCW_DEVICE(ccw_dev);
>  S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev);
> +    VFIOCCWDevice *vcdev = VFIO_CCW(cdev);
>  Error *err = NULL;
>  
>  /* Call the class init function for subchannel. */
> @@ -729,9 +726,9 @@ out_err_propagate:
>  static void vfio_ccw_unrealize(DeviceState *dev)
>  {
>  CcwDevice *ccw_dev = DO_UPCAST(CcwDevice, parent_obj, dev);
> -    S390CCWDevice *cdev = DO_UPCAST(S390CCWDevice, parent_obj,
> ccw_dev);
> -    VFIOCCWDevice *vcdev = DO_UPCAST(VFIOCCWDevice, cdev, cdev);
> +    S390CCWDevice *cdev = S390_CCW_DEVICE(ccw_dev);
>  S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev);
> +    VFIOCCWDevice *vcdev = VFIO_CCW(cdev);
>  VFIOGroup *group = vcdev->vdev.group;
>  
>  vfio_ccw_unregister_irq_notifier(vcdev, VFIO_CCW_REQ_IRQ_INDEX);

Re: [PATCH v2 15/19] hw/s390x/event-facility: Replace DO_UPCAST(SCLPEvent) by SCLP_EVENT()

2023-02-13 Thread Eric Farman 
On Mon, 2023-02-13 at 08:08 +0100, Philippe Mathieu-Daudé wrote:
> Use the SCLP_EVENT() QOM type-checking macro to avoid DO_UPCAST().
> 
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Eric Farman 

> ---
>  hw/s390x/event-facility.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> index faa51aa4c7..6891e3cd73 100644
> --- a/hw/s390x/event-facility.c
> +++ b/hw/s390x/event-facility.c
> @@ -64,8 +64,7 @@ static bool event_pending(SCLPEventFacility *ef)
>  SCLPEventClass *event_class;
>  
>  QTAILQ_FOREACH(kid, >sbus.qbus.children, sibling) {
> -    DeviceState *qdev = kid->child;
> -    event = DO_UPCAST(SCLPEvent, qdev, qdev);
> +    event = SCLP_EVENT(kid->child);
>  event_class = SCLP_EVENT_GET_CLASS(event);
>  if (event->event_pending &&
>  event_class->get_send_mask() & ef->receive_mask) {

Re: [PATCH v2 14/19] hw/scsi/scsi-bus: Inline two uses of scsi_bus_from_device()

2023-02-13 Thread Eric Farman 
On Mon, 2023-02-13 at 08:08 +0100, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé 

Acked-by: Eric Farman 

> ---
>  hw/s390x/ipl.c | 7 ++-
>  hw/scsi/scsi-bus.c | 2 +-
>  include/hw/scsi/scsi.h | 5 -
>  3 files changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 8612684d48..4f7f4e60d6 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -366,11 +366,8 @@ static CcwDevice
> *s390_get_ccw_device(DeviceState *dev_st, int *devtype)
>  ccw_dev = CCW_DEVICE(vfio_ccw_dev);
>  tmp_dt = CCW_DEVTYPE_VFIO;
>  } else {
> -    SCSIDevice *sd = (SCSIDevice *)
> -    object_dynamic_cast(OBJECT(dev_st),
> -    TYPE_SCSI_DEVICE);
> -    if (sd) {
> -    SCSIBus *sbus = scsi_bus_from_device(sd);
> +    if (object_dynamic_cast(OBJECT(dev_st),
> TYPE_SCSI_DEVICE)) {
> +    SCSIBus *sbus =
> SCSI_BUS(qdev_get_parent_bus(dev_st));
>  VirtIODevice *vdev = (VirtIODevice *)
>  object_dynamic_cast(OBJECT(sbus->qbus.parent),
>  TYPE_VIRTIO_DEVICE);
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index c4525515ab..ee72b86b13 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -679,7 +679,7 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps
> *reqops, SCSIDevice *d,
>  uint32_t tag, uint32_t lun, void
> *hba_private)
>  {
>  SCSIRequest *req;
> -    SCSIBus *bus = scsi_bus_from_device(d);
> +    SCSIBus *bus = SCSI_BUS(qdev_get_parent_bus(DEVICE(d)));
>  BusState *qbus = BUS(bus);
>  const int memset_off = offsetof(SCSIRequest, sense)
>     + sizeof(req->sense);
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index eb558c145a..e3263dec0d 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -175,11 +175,6 @@ static inline void scsi_bus_init(SCSIBus *bus,
> size_t bus_size,
>  scsi_bus_init_named(bus, bus_size, host, info, NULL);
>  }
>  
> -static inline SCSIBus *scsi_bus_from_device(SCSIDevice *d)
> -{
> -    return SCSI_BUS(qdev_get_parent_bus(DEVICE(d)));
> -}
> -
>  SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend
> *blk,
>    int unit, bool removable, int
> bootindex,
>    bool share_rw,

Re: [Qemu-block] [PATCH v3 2/3] block: Fix target variable of BLKSECTGET ioctl

2017-02-01 Thread Eric Farman 



On 01/31/2017 07:25 PM, Max Reitz wrote:

On 20.01.2017 17:25, Eric Farman wrote:

Commit 6f6071745bd0 ("raw-posix: Fetch max sectors for host block device")
introduced a routine to call the kernel BLKSECTGET ioctl, which stores the
result back to user space.  However, the size of the data returned depends
on the routine handling the ioctl.  The (compat_)blkdev_ioctl returns a
short, while sg_ioctl returns an int.  Thus, on big-endian systems, we can
find ourselves accidentally shifting the result to a much larger value.
(On s390x, a short is 16 bits while an int is 32 bits.)

Also, the two ioctl handlers return values in different scales (block
returns sectors, while sg returns bytes), so some tweaking of the outputs
is required such that hdev_get_max_transfer_length returns a value in a
consistent set of units.

Signed-off-by: Eric Farman <far...@linux.vnet.ibm.com>
---
 block/file-posix.c| 17 ++---
 include/block/block.h |  1 +
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 28b47d9..9f83725 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -651,12 +651,15 @@ static void raw_reopen_abort(BDRVReopenState *state)
 state->opaque = NULL;
 }

-static int hdev_get_max_transfer_length(int fd)
+static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
 {
 #ifdef BLKSECTGET
-int max_sectors = 0;
-if (ioctl(fd, BLKSECTGET, _sectors) == 0) {
-return max_sectors;
+int max_bytes = 0;
+short max_sectors = 0;
+if (bs->sg && ioctl(fd, BLKSECTGET, _bytes) == 0) {
+return max_bytes;
+} else if (!bs->sg && ioctl(fd, BLKSECTGET, _sectors) == 0) {
+return max_sectors << BDRV_SECTOR_BITS;
 } else {
 return -errno;
 }
@@ -672,9 +675,9 @@ static void raw_refresh_limits(BlockDriverState *bs, Error 
**errp)

 if (!fstat(s->fd, )) {
 if (S_ISBLK(st.st_mode)) {
-int ret = hdev_get_max_transfer_length(s->fd);
-if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) {
-bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS);
+int ret = hdev_get_max_transfer_length(bs, s->fd);
+if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
+bs->bl.max_transfer = pow2floor(ret);
 }
 }
 }
diff --git a/include/block/block.h b/include/block/block.h
index 8b0dcda..4e81f20 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -116,6 +116,7 @@ typedef struct HDGeometry {

 #define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
  INT_MAX >> BDRV_SECTOR_BITS)
+#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)


I'd rather like it the other way round (i.e.

#define BDRV_REQUEST_MAX_BYTES MIN(SIZE_MAX, INT_MAX)
#define BDRV_REQUEST_MAX_SECTORS (BDRV_REQUEST_MAX_BYTES >>
BDRV_SECTOR_BITS)

), but I don't suppose I can interest you in a respin, can I? Would it
be OK with you if I changed you commit when I apply it?

Max



That's far cleaner looking, so absolutely go ahead.  Thank you!

 - Eric





 /*
  * Allocation status flags

Re: [Qemu-block] [Qemu-devel] [PATCH v3 0/3] scsi-generic and BLKSECTGET

2017-01-24 Thread Eric Farman 



On 01/24/2017 07:09 AM, Fam Zheng wrote:

On Tue, 01/24 12:23, Paolo Bonzini wrote:



On 22/01/2017 15:29, Fam Zheng wrote:

On Fri, 01/20 17:25, Eric Farman wrote:

Changes:
 v2->v3:
  - Move byte/sector conversions to patch 2 [Fam Zheng]
  - Rename "max_sectors" when holding a byte value [Fam Zheng]
 v1->v2:
  - Patch 3, make hdev_get_max_transfer_length return bytes [Fam Zheng]


Reviewed-by: Fam Zheng <f...@redhat.com>


Looks good, who's merging this? :)


All I know is not me :) Adding Kevin and Max who maintain block/file-posix.c.


Also not me :) But thank you for the review and consideration of this!

 - Eric

[Qemu-block] [PATCH v3 0/3] scsi-generic and BLKSECTGET

2017-01-20 Thread Eric Farman 
In the Linux kernel, I see two (three) places where the BLKSECTGET ioctl is
handled:

(1) block/(compat_)ioctl.c -- (compat_)blkdev_ioctl
(2) drivers/scsi/sg.c -- sg_ioctl

The former has been around forever[1], and returns a short value measured in
sectors.  A sector is generally assumed to be 512 bytes.

The latter has been around for slightly less than forever[2], and returns an
int that measures the value in bytes.  A change to return the block count
was brought up a few years ago[3] and nacked.

As a convenient example, if I use the blockdev tool to drive the ioctl to a
SCSI disk and its scsi-generic equivalent, I get different results:

  # lsscsi -g
  [0:0:8:1077166114]diskIBM  2107900  .217  /dev/sda /dev/sg0
  # blockdev --getmaxsect /dev/sda
  2560
  # blockdev --getmaxsect /dev/sg0
  20

Now, the value for /dev/sda looks "correct" to me.

  # cd /sys/devices/css0/0.0.0125/0.0.1f69/host0/rport-0\:0-8/
  # cd target0\:0\:8/0\:0\:8\:1077166114/
  # cat block/sda/queue/max_sectors_kb
  1280
  # cat block/sda/queue/hw_sector_size
  512

And the math checks out:

  max_sectors_kb * 1024 / hw_sector_size == getmaxsect
  -OR-
  1280 * 1024 / 512 = 2560

For /dev/sg0, it appears the answer is coming from the sg_ioctl result
which is already multiplied by the block size, and then looking at only the
upper half (short) of the returned big-endian fullword:

  (1280 * 1024 / 512) * 512 = 1310720 = x0014 => x0014 = 20

The reason for all this?  Well, QEMU recently added a BLKSECTGET ioctl
call[4] which we see during guest boot.  This code presumes the value is in
blocks/sectors, and converts it to bytes[5].  Not that this matters, because
the short/int discrepancy gives us "zero" on s390x.

Also, that code doesn't execute for scsi-generic devices, so the conversion
to bytes is correct, but I'd like to extend this code to interrogate
scsi-generic devices as well.  This is important because libvirt converts
a specified virtio-scsi device to its /dev/sgX address for the QEMU
commandline.

Changes:
 v2->v3:
  - Move byte/sector conversions to patch 2 [Fam Zheng]
  - Rename "max_sectors" when holding a byte value [Fam Zheng]
 v1->v2:
  - Patch 3, make hdev_get_max_transfer_length return bytes [Fam Zheng]

[1] The initial kernel git commit
[2] kernel commit 44ec95425c1d9dce6e4638c29e4362cfb44814e7
[3] https://lkml.org/lkml/2012/6/27/78
[4] qemu commit 6f6071745bd0366221f5a0160ed7d18d0e38b9f7
[5] qemu commit 5def6b80e1eca696c1fc6099e7f4d36729686402 

Eric Farman (3):
  hw/scsi: Fix debug message of cdb structure in scsi-generic
  block: Fix target variable of BLKSECTGET ioctl
  block: get max_transfer limit for char (scsi-generic) devices

 block/file-posix.c | 19 +++
 hw/scsi/scsi-generic.c |  5 +++--
 include/block/block.h  |  1 +
 3 files changed, 15 insertions(+), 10 deletions(-)

-- 
2.8.4

[Qemu-block] [PATCH v3 2/3] block: Fix target variable of BLKSECTGET ioctl

2017-01-20 Thread Eric Farman 
Commit 6f6071745bd0 ("raw-posix: Fetch max sectors for host block device")
introduced a routine to call the kernel BLKSECTGET ioctl, which stores the
result back to user space.  However, the size of the data returned depends
on the routine handling the ioctl.  The (compat_)blkdev_ioctl returns a
short, while sg_ioctl returns an int.  Thus, on big-endian systems, we can
find ourselves accidentally shifting the result to a much larger value.
(On s390x, a short is 16 bits while an int is 32 bits.)

Also, the two ioctl handlers return values in different scales (block
returns sectors, while sg returns bytes), so some tweaking of the outputs
is required such that hdev_get_max_transfer_length returns a value in a
consistent set of units.

Signed-off-by: Eric Farman <far...@linux.vnet.ibm.com>
---
 block/file-posix.c| 17 ++---
 include/block/block.h |  1 +
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 28b47d9..9f83725 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -651,12 +651,15 @@ static void raw_reopen_abort(BDRVReopenState *state)
 state->opaque = NULL;
 }
 
-static int hdev_get_max_transfer_length(int fd)
+static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
 {
 #ifdef BLKSECTGET
-int max_sectors = 0;
-if (ioctl(fd, BLKSECTGET, _sectors) == 0) {
-return max_sectors;
+int max_bytes = 0;
+short max_sectors = 0;
+if (bs->sg && ioctl(fd, BLKSECTGET, _bytes) == 0) {
+return max_bytes;
+} else if (!bs->sg && ioctl(fd, BLKSECTGET, _sectors) == 0) {
+return max_sectors << BDRV_SECTOR_BITS;
 } else {
 return -errno;
 }
@@ -672,9 +675,9 @@ static void raw_refresh_limits(BlockDriverState *bs, Error 
**errp)
 
 if (!fstat(s->fd, )) {
 if (S_ISBLK(st.st_mode)) {
-int ret = hdev_get_max_transfer_length(s->fd);
-if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) {
-bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS);
+int ret = hdev_get_max_transfer_length(bs, s->fd);
+if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
+bs->bl.max_transfer = pow2floor(ret);
 }
 }
 }
diff --git a/include/block/block.h b/include/block/block.h
index 8b0dcda..4e81f20 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -116,6 +116,7 @@ typedef struct HDGeometry {
 
 #define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
  INT_MAX >> BDRV_SECTOR_BITS)
+#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
 
 /*
  * Allocation status flags
-- 
2.8.4

[Qemu-block] [PATCH v3 1/3] hw/scsi: Fix debug message of cdb structure in scsi-generic

2017-01-20 Thread Eric Farman 
When running with debug enabled, the scsi-generic cdb that is
dumped skips byte 0 of the command, which is the opcode.  This
makes identifying which command is being issued/completed a
little difficult.  Example:

  0x00 0x00 0x01 0x00 0x00
  scsi-generic: scsi_read_data 0x0
  scsi-generic: Data ready tag=0x0 len=164
  scsi-generic: scsi_read_data 0x0
  scsi-generic: Command complete 0x0x10a42c60 tag=0x0 status=0

Improve this by adding a message prior to the loop, similar to
what exists for scsi-disk.  Clean up a few other messages to be
more explicit of what is being represented.  Example:

  scsi-generic: Command: data=0x12 0x00 0x00 0x01 0x00 0x00
  scsi-generic: scsi_read_data tag=0x0
  scsi-generic: Data ready tag=0x0 len=164
  scsi-generic: scsi_read_data tag=0x0
  scsi-generic: Command complete 0x0x10a452d0 tag=0x0 status=0

Signed-off-by: Eric Farman <far...@linux.vnet.ibm.com>
---
 hw/scsi/scsi-generic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 7a588a7..92f091a 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -246,7 +246,7 @@ static void scsi_read_data(SCSIRequest *req)
 SCSIDevice *s = r->req.dev;
 int ret;
 
-DPRINTF("scsi_read_data 0x%x\n", req->tag);
+DPRINTF("scsi_read_data tag=0x%x\n", req->tag);
 
 /* The request is used as the AIO opaque value, so add a ref.  */
 scsi_req_ref(>req);
@@ -294,7 +294,7 @@ static void scsi_write_data(SCSIRequest *req)
 SCSIDevice *s = r->req.dev;
 int ret;
 
-DPRINTF("scsi_write_data 0x%x\n", req->tag);
+DPRINTF("scsi_write_data tag=0x%x\n", req->tag);
 if (r->len == 0) {
 r->len = r->buflen;
 scsi_req_data(>req, r->len);
@@ -329,6 +329,7 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t 
*cmd)
 int ret;
 
 #ifdef DEBUG_SCSI
+DPRINTF("Command: data=0x%02x", cmd[0]);
 {
 int i;
 for (i = 1; i < r->req.cmd.len; i++) {
-- 
2.8.4

Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/3] block: get max_transfer limit for char (scsi-generic) devices

2017-01-20 Thread Eric Farman 



On 01/20/2017 04:53 AM, Fam Zheng wrote:

On Fri, 01/20 17:31, Fam Zheng wrote:

diff --git a/block/file-posix.c b/block/file-posix.c
index 2115155..94068ca 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -657,9 +657,11 @@ static int hdev_get_max_transfer_length(BlockDriverState 
*bs, int fd)
 int max_sectors = 0;
 short max_sectors_short = 0;
 if (bs->sg && ioctl(fd, BLKSECTGET, _sectors) == 0) {
+/* sg returns a value in bytes */
 return max_sectors;


The variable name is now misleading, maybe use "bytes" instead?


BTW patch 2 should already make the function return bytes consistently.  Doing
it here is meaningless code churn.


Excellent points.  Spinning v3 now.

 - Eric

[Qemu-block] [PATCH v2 0/3] scsi-generic and BLKSECTGET

2017-01-19 Thread Eric Farman 
In the Linux kernel, I see two (three) places where the BLKSECTGET ioctl is
handled:

(1) block/(compat_)ioctl.c -- (compat_)blkdev_ioctl
(2) drivers/scsi/sg.c -- sg_ioctl

The former has been around forever[1], and returns a short value measured in
sectors.  A sector is generally assumed to be 512 bytes.

The latter has been around for slightly less than forever[2], and returns an
int that measures the value in bytes.  A change to return the block count
was brought up a few years ago[3] and nacked.

As a convenient example, if I use the blockdev tool to drive the ioctl to a
SCSI disk and its scsi-generic equivalent, I get different results:

  # lsscsi -g
  [0:0:8:1077166114]diskIBM  2107900  .217  /dev/sda /dev/sg0
  # blockdev --getmaxsect /dev/sda
  2560
  # blockdev --getmaxsect /dev/sg0
  20

Now, the value for /dev/sda looks "correct" to me.

  # cd /sys/devices/css0/0.0.0125/0.0.1f69/host0/rport-0\:0-8/
  # cd target0\:0\:8/0\:0\:8\:1077166114/
  # cat block/sda/queue/max_sectors_kb
  1280
  # cat block/sda/queue/hw_sector_size
  512

And the math checks out:

  max_sectors_kb * 1024 / hw_sector_size == getmaxsect
  -OR-
  1280 * 1024 / 512 = 2560

For /dev/sg0, it appears the answer is coming from the sg_ioctl result
which is already multiplied by the block size, and then looking at only the
upper half (short) of the returned big-endian fullword:

  (1280 * 1024 / 512) * 512 = 1310720 = x0014 => x0014 = 20

The reason for all this?  Well, QEMU recently added a BLKSECTGET ioctl
call[4] which we see during guest boot.  This code presumes the value is in
blocks/sectors, and converts it to bytes[5].  Not that this matters, because
the short/int discrepancy gives us "zero" on s390x.

Also, that code doesn't execute for scsi-generic devices, so the conversion
to bytes is correct, but I'd like to extend this code to interrogate
scsi-generic devices as well.  This is important because libvirt converts
a specified virtio-scsi device to its /dev/sgX address for the QEMU
commandline.

So, do I have to code around the different field sizes (int vs short) as
well as scaling (bytes vs blocks)?  Obviously doable, but looking at the
resulting commits, I find myself feeling a little ill.

Changes:
 v1->v2:
  - Patch 3, make hdev_get_max_transfer_length return bytes [Fam Zheng]

[1] The initial kernel git commit
[2] kernel commit 44ec95425c1d9dce6e4638c29e4362cfb44814e7
[3] https://lkml.org/lkml/2012/6/27/78
[4] qemu commit 6f6071745bd0366221f5a0160ed7d18d0e38b9f7
[5] qemu commit 5def6b80e1eca696c1fc6099e7f4d36729686402 

Eric Farman (3):
  hw/scsi: Fix debug message of cdb structure in scsi-generic
  block: Fix target variable of BLKSECTGET ioctl
  block: get max_transfer limit for char (scsi-generic) devices

 block/file-posix.c | 17 +++--
 hw/scsi/scsi-generic.c |  5 +++--
 include/block/block.h  |  1 +
 3 files changed, 15 insertions(+), 8 deletions(-)

-- 
2.8.4

[Qemu-block] [PATCH v2 1/3] hw/scsi: Fix debug message of cdb structure in scsi-generic

2017-01-19 Thread Eric Farman 
When running with debug enabled, the scsi-generic cdb that is
dumped skips byte 0 of the command, which is the opcode.  This
makes identifying which command is being issued/completed a
little difficult.  Example:

  0x00 0x00 0x01 0x00 0x00
  scsi-generic: scsi_read_data 0x0
  scsi-generic: Data ready tag=0x0 len=164
  scsi-generic: scsi_read_data 0x0
  scsi-generic: Command complete 0x0x10a42c60 tag=0x0 status=0

Improve this by adding a message prior to the loop, similar to
what exists for scsi-disk.  Clean up a few other messages to be
more explicit of what is being represented.  Example:

  scsi-generic: Command: data=0x12 0x00 0x00 0x01 0x00 0x00
  scsi-generic: scsi_read_data tag=0x0
  scsi-generic: Data ready tag=0x0 len=164
  scsi-generic: scsi_read_data tag=0x0
  scsi-generic: Command complete 0x0x10a452d0 tag=0x0 status=0

Signed-off-by: Eric Farman <far...@linux.vnet.ibm.com>
---
 hw/scsi/scsi-generic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 7a588a7..92f091a 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -246,7 +246,7 @@ static void scsi_read_data(SCSIRequest *req)
 SCSIDevice *s = r->req.dev;
 int ret;
 
-DPRINTF("scsi_read_data 0x%x\n", req->tag);
+DPRINTF("scsi_read_data tag=0x%x\n", req->tag);
 
 /* The request is used as the AIO opaque value, so add a ref.  */
 scsi_req_ref(>req);
@@ -294,7 +294,7 @@ static void scsi_write_data(SCSIRequest *req)
 SCSIDevice *s = r->req.dev;
 int ret;
 
-DPRINTF("scsi_write_data 0x%x\n", req->tag);
+DPRINTF("scsi_write_data tag=0x%x\n", req->tag);
 if (r->len == 0) {
 r->len = r->buflen;
 scsi_req_data(>req, r->len);
@@ -329,6 +329,7 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t 
*cmd)
 int ret;
 
 #ifdef DEBUG_SCSI
+DPRINTF("Command: data=0x%02x", cmd[0]);
 {
 int i;
 for (i = 1; i < r->req.cmd.len; i++) {
-- 
2.8.4

[Qemu-block] [PATCH v2 3/3] block: get max_transfer limit for char (scsi-generic) devices

2017-01-19 Thread Eric Farman 
Commit 6f607174 introduced a routine to get the maximum number
of bytes for a single I/O transfer for block devices, however
scsi generic devices are character devices, not block.  Add
a condition for this, such that scsi generic devices can view
the same data.

Some tweaking of data is required, because the block and sg
ioctls return values in different scales (sectors versus
bytes).  So adjust hdev_get_max_transfer_length such that it
always returns a value in bytes.

Signed-off-by: Eric Farman <far...@linux.vnet.ibm.com>
---
 block/file-posix.c| 10 ++
 include/block/block.h |  1 +
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 2115155..94068ca 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -657,9 +657,11 @@ static int hdev_get_max_transfer_length(BlockDriverState 
*bs, int fd)
 int max_sectors = 0;
 short max_sectors_short = 0;
 if (bs->sg && ioctl(fd, BLKSECTGET, _sectors) == 0) {
+/* sg returns a value in bytes */
 return max_sectors;
 } else if (!bs->sg && ioctl(fd, BLKSECTGET, _sectors_short) == 0) {
-return max_sectors_short;
+/* block returns a value in sectors */
+return max_sectors_short << BDRV_SECTOR_BITS;
 } else {
 return -errno;
 }
@@ -674,10 +676,10 @@ static void raw_refresh_limits(BlockDriverState *bs, 
Error **errp)
 struct stat st;
 
 if (!fstat(s->fd, )) {
-if (S_ISBLK(st.st_mode)) {
+if (S_ISBLK(st.st_mode) || S_ISCHR(st.st_mode)) {
 int ret = hdev_get_max_transfer_length(bs, s->fd);
-if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) {
-bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS);
+if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
+bs->bl.max_transfer = pow2floor(ret);
 }
 }
 }
diff --git a/include/block/block.h b/include/block/block.h
index 8b0dcda..4e81f20 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -116,6 +116,7 @@ typedef struct HDGeometry {
 
 #define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
  INT_MAX >> BDRV_SECTOR_BITS)
+#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
 
 /*
  * Allocation status flags
-- 
2.8.4

[Qemu-block] [PATCH v2 2/3] block: Fix target variable of BLKSECTGET ioctl

2017-01-19 Thread Eric Farman 
Commit 6f607174 introduced a routine to call the kernel BLKSECTGET
ioctl, which stores the result back to user space.  However, the
size of the data returned depends on the routine handling the ioctl.
The (compat_)blkdev_ioctl returns a short, while sg_ioctl returns
an int.  Thus, on big-endian systems, we can find ourselves
accidentally shifting the result to a much larger value.
(On s390x, a short is 16 bits while an int is 32 bits.)

Signed-off-by: Eric Farman <far...@linux.vnet.ibm.com>
---
 block/file-posix.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 28b47d9..2115155 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -651,12 +651,15 @@ static void raw_reopen_abort(BDRVReopenState *state)
 state->opaque = NULL;
 }
 
-static int hdev_get_max_transfer_length(int fd)
+static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
 {
 #ifdef BLKSECTGET
 int max_sectors = 0;
-if (ioctl(fd, BLKSECTGET, _sectors) == 0) {
+short max_sectors_short = 0;
+if (bs->sg && ioctl(fd, BLKSECTGET, _sectors) == 0) {
 return max_sectors;
+} else if (!bs->sg && ioctl(fd, BLKSECTGET, _sectors_short) == 0) {
+return max_sectors_short;
 } else {
 return -errno;
 }
@@ -672,7 +675,7 @@ static void raw_refresh_limits(BlockDriverState *bs, Error 
**errp)
 
 if (!fstat(s->fd, )) {
 if (S_ISBLK(st.st_mode)) {
-int ret = hdev_get_max_transfer_length(s->fd);
+int ret = hdev_get_max_transfer_length(bs, s->fd);
 if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) {
 bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS);
 }
-- 
2.8.4

Re: [Qemu-block] [Qemu-devel] [PATCH 3/3] block: get max_transfer limit for char (scsi-generic) devices

2017-01-17 Thread Eric Farman 



On 01/17/2017 02:04 AM, Fam Zheng wrote:

On Mon, 01/16 22:12, Eric Farman wrote:

Commit 6f607174 introduced a routine to get the maximum number
of bytes for a single I/O transfer for block devices, however
scsi generic devices are character devices, not block.  Add
a condition for this, with slightly different logic because
the value is already in bytes, and need not be converted from
blocks as happens for block devices.

Signed-off-by: Eric Farman <far...@linux.vnet.ibm.com>
---
 block/file-posix.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 2115155..c0843c2 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -679,6 +679,13 @@ static void raw_refresh_limits(BlockDriverState *bs, Error 
**errp)
 if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) {
 bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS);
 }
+} else if (S_ISCHR(st.st_mode)) {
+/* sg returns transfer length in bytes already */
+int ret = hdev_get_max_transfer_length(bs, s->fd);
+if (ret > 0 &&
+(ret >> BDRV_SECTOR_BITS) <= BDRV_REQUEST_MAX_SECTORS) {
+bs->bl.max_transfer = pow2floor(ret);
+}


Please keep the sectors/bytes quirk in hdev_get_max_transfer_length and always
return bytes from there.


That's easy enough.  I'll allow a day or two before sending a v2, in 
case there's other considerations for the rats nest I've wandered into.


Thanks!

 - Eric

[Qemu-block] [RFC PATCH 0/3] scsi-generic and BLKSECTGET

2017-01-16 Thread Eric Farman 
(cc'ing linux-scsi for the cover-letter; patches only to QEMU lists.)

In the Linux kernel, I see two (three) places where the BLKSECTGET ioctl is
handled:

(1) block/(compat_)ioctl.c -- (compat_)blkdev_ioctl
(2) drivers/scsi/sg.c -- sg_ioctl

The former has been around forever[1], and returns a short value measured in
sectors.  A sector is generally assumed to be 512 bytes.

The latter has been around for slightly less than forever[2], and returns an
int that measures the value in bytes.  A change to return the block count
was brought up a few years ago[3] and nacked.

As a convenient example, if I use the blockdev tool to drive the ioctl to a
SCSI disk and its scsi-generic equivalent, I get different results:

  # lsscsi -g
  [0:0:8:1077166114]diskIBM  2107900  .217  /dev/sda /dev/sg0
  # blockdev --getmaxsect /dev/sda
  2560
  # blockdev --getmaxsect /dev/sg0
  20

Now, the value for /dev/sda looks "correct" to me.

  # cd /sys/devices/css0/0.0.0125/0.0.1f69/host0/rport-0\:0-8/
  # cd target0\:0\:8/0\:0\:8\:1077166114/
  # cat block/sda/queue/max_sectors_kb
  1280
  # cat block/sda/queue/hw_sector_size
  512

And the math checks out:

  max_sectors_kb * 1024 / hw_sector_size == getmaxsect
  -OR-
  1280 * 1024 / 512 = 2560

For /dev/sg0, it appears the answer is coming from the sg_ioctl result
which is already multiplied by the block size, and then looking at only the
upper half (short) of the returned big-endian fullword:

  (1280 * 1024 / 512) * 512 = 1310720 = x0014 => x0014 = 20

The reason for all this?  Well, QEMU recently added a BLKSECTGET ioctl
call[4] which we see during guest boot.  This code presumes the value is in
blocks/sectors, and converts it to bytes[5].  Not that this matters, because
the short/int discrepancy gives us "zero" on s390x.

Also, that code doesn't execute for scsi-generic devices, so the conversion
to bytes is correct, but I'd like to extend this code to interrogate
scsi-generic devices as well.  This is important because libvirt converts
a specified virtio-scsi device to its /dev/sgX address for the QEMU
commandline.

So, do I have to code around the different field sizes (int vs short) as
well as scaling (bytes vs blocks)?  Obviously doable, but looking at the
resulting commits, I find myself feeling a little ill.

[1] The initial kernel git commit
[2] kernel commit 44ec95425c1d9dce6e4638c29e4362cfb44814e7
[3] https://lkml.org/lkml/2012/6/27/78
[4] qemu commit 6f6071745bd0366221f5a0160ed7d18d0e38b9f7
[5] qemu commit 5def6b80e1eca696c1fc6099e7f4d36729686402

Eric Farman (3):
  hw/scsi: Fix debug message of cdb structure in scsi-generic
  block: Fix target variable of BLKSECTGET ioctl
  block: get max_transfer limit for char (scsi-generic) devices

 block/file-posix.c | 16 +---
 hw/scsi/scsi-generic.c |  5 +++--
 2 files changed, 16 insertions(+), 5 deletions(-)

-- 
2.8.4

[Qemu-block] [PATCH 3/3] block: get max_transfer limit for char (scsi-generic) devices

2017-01-16 Thread Eric Farman 
Commit 6f607174 introduced a routine to get the maximum number
of bytes for a single I/O transfer for block devices, however
scsi generic devices are character devices, not block.  Add
a condition for this, with slightly different logic because
the value is already in bytes, and need not be converted from
blocks as happens for block devices.

Signed-off-by: Eric Farman <far...@linux.vnet.ibm.com>
---
 block/file-posix.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 2115155..c0843c2 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -679,6 +679,13 @@ static void raw_refresh_limits(BlockDriverState *bs, Error 
**errp)
 if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) {
 bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS);
 }
+} else if (S_ISCHR(st.st_mode)) {
+/* sg returns transfer length in bytes already */
+int ret = hdev_get_max_transfer_length(bs, s->fd);
+if (ret > 0 &&
+(ret >> BDRV_SECTOR_BITS) <= BDRV_REQUEST_MAX_SECTORS) {
+bs->bl.max_transfer = pow2floor(ret);
+}
 }
 }
 
-- 
2.8.4

[Qemu-block] [PATCH 2/3] block: Fix target variable of BLKSECTGET ioctl

2017-01-16 Thread Eric Farman 
Commit 6f607174 introduced a routine to call the kernel BLKSECTGET
ioctl, which stores the result back to user space.  However, the
size of the data returned depends on the routine handling the ioctl.
The (compat_)blkdev_ioctl returns a short, while sg_ioctl returns
an int.  Thus, on big-endian systems, we can find ourselves
accidentally shifting the result to a much larger value.
(On s390x, a short is 16 bits while an int is 32 bits.)

Signed-off-by: Eric Farman <far...@linux.vnet.ibm.com>
---
 block/file-posix.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 28b47d9..2115155 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -651,12 +651,15 @@ static void raw_reopen_abort(BDRVReopenState *state)
 state->opaque = NULL;
 }
 
-static int hdev_get_max_transfer_length(int fd)
+static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
 {
 #ifdef BLKSECTGET
 int max_sectors = 0;
-if (ioctl(fd, BLKSECTGET, _sectors) == 0) {
+short max_sectors_short = 0;
+if (bs->sg && ioctl(fd, BLKSECTGET, _sectors) == 0) {
 return max_sectors;
+} else if (!bs->sg && ioctl(fd, BLKSECTGET, _sectors_short) == 0) {
+return max_sectors_short;
 } else {
 return -errno;
 }
@@ -672,7 +675,7 @@ static void raw_refresh_limits(BlockDriverState *bs, Error 
**errp)
 
 if (!fstat(s->fd, )) {
 if (S_ISBLK(st.st_mode)) {
-int ret = hdev_get_max_transfer_length(s->fd);
+int ret = hdev_get_max_transfer_length(bs, s->fd);
 if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) {
 bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS);
 }
-- 
2.8.4