Re: [Qemu-block] [Qemu-devel] [PATCH] ide: coverity touchups
On 07/13/2015 03:40 PM, Peter Maydell wrote: > On 13 July 2015 at 20:26, John Snow wrote: >> Just a handful of casts to quiet coverity up. >> >> s->ports should never exceed 32, but coverity doesn't know that. >> ncq_tfs->sector_count should also never exceed 64K. > > Personally I tend to mark that kind of thing as a false > positive in the coverity UI and move on... > > -- PMM > Either way; Paolo pinged me about the NCQ one so I figured I'd just do it. *shrug*
Re: [Qemu-block] [Qemu-devel] [PATCH] ide: coverity touchups
On 13 July 2015 at 20:26, John Snow wrote: > Just a handful of casts to quiet coverity up. > > s->ports should never exceed 32, but coverity doesn't know that. > ncq_tfs->sector_count should also never exceed 64K. Personally I tend to mark that kind of thing as a false positive in the coverity UI and move on... -- PMM
[Qemu-block] [PATCH] ide: coverity touchups
Just a handful of casts to quiet coverity up.
s->ports should never exceed 32, but coverity doesn't know that.
ncq_tfs->sector_count should also never exceed 64K.
Why not make Coverity a happy camper, though.
Signed-off-by: John Snow
---
hw/ide/ahci.c | 8
hw/ide/macio.c | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index bb6a92f..c45983c 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -356,7 +356,7 @@ static uint64_t ahci_mem_read_32(void *opaque, hwaddr addr)
DPRINTF(-1, "(addr 0x%08X), val 0x%08X\n", (unsigned) addr, val);
} else if ((addr >= AHCI_PORT_REGS_START_ADDR) &&
(addr < (AHCI_PORT_REGS_START_ADDR +
-(s->ports * AHCI_PORT_ADDR_OFFSET_LEN {
+(s->ports * (hwaddr)AHCI_PORT_ADDR_OFFSET_LEN {
val = ahci_port_read(s, (addr - AHCI_PORT_REGS_START_ADDR) >> 7,
addr & AHCI_PORT_ADDR_OFFSET_MASK);
}
@@ -433,7 +433,7 @@ static void ahci_mem_write(void *opaque, hwaddr addr,
}
} else if ((addr >= AHCI_PORT_REGS_START_ADDR) &&
(addr < (AHCI_PORT_REGS_START_ADDR +
-(s->ports * AHCI_PORT_ADDR_OFFSET_LEN {
+(s->ports * (hwaddr)AHCI_PORT_ADDR_OFFSET_LEN {
ahci_port_write(s, (addr - AHCI_PORT_REGS_START_ADDR) >> 7,
addr & AHCI_PORT_ADDR_OFFSET_MASK, val);
}
@@ -1571,8 +1571,8 @@ static int ahci_state_post_load(void *opaque, int
version_id)
return -1;
}
ahci_populate_sglist(ncq_tfs->drive, &ncq_tfs->sglist,
- ncq_tfs->cmdh, ncq_tfs->sector_count * 512,
- 0);
+ ncq_tfs->cmdh,
+ (int64_t)ncq_tfs->sector_count * 512, 0);
if (ncq_tfs->sector_count != ncq_tfs->sglist.size >> 9) {
return -1;
}
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index a55a479..340774b 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -253,7 +253,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int
ret)
}
/* Calculate current offset */
-offset = (int64_t)(s->lba << 11) + s->io_buffer_index;
+offset = ((int64_t)s->lba << 11) + s->io_buffer_index;
pmac_dma_read(s->blk, offset, io->len, pmac_ide_atapi_transfer_cb, io);
return;
--
2.1.0
Re: [Qemu-block] [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, Jul 13, 2015 at 03:20:59PM +0200, Cornelia Huck wrote:
> On Mon, 13 Jul 2015 15:36:00 +0300
> "Michael S. Tsirkin" wrote:
>
> > On Mon, Jul 13, 2015 at 02:30:24PM +0200, Cornelia Huck wrote:
> > > On Mon, 13 Jul 2015 15:22:52 +0300
> > > "Michael S. Tsirkin" wrote:
> > >
> > > > On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote:
> > > > > On Mon, 13 Jul 2015 11:56:51 +0200
> > > > > Kevin Wolf wrote:
> > > > >
> > > > > > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
> > > > > > >
> > > > > > >
> > > > > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > > > > > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> > > > > > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable
> > > > > > > >> it.
> > > > > > > >>
> > > > > > > >> Cc: Stefan Hajnoczi
> > > > > > > >> Cc: Kevin Wolf
> > > > > > > >> Cc: qemu-block@nongnu.org
> > > > > > > >> Signed-off-by: Jason Wang
> > > > > > > > Interesting, I noticed we have a field scsi - see
> > > > > > > > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > > > > > > > Author: Paolo Bonzini
> > > > > > > > Date: Fri Dec 23 15:39:03 2011 +0100
> > > > > > > >
> > > > > > > > virtio-blk: refuse SG_IO requests with scsi=off
> > > > > > > >
> > > > > > > > but it doesn't seem to be propagated to guest features in
> > > > > > > > any way.
> > > > > > > >
> > > > > > > > Maybe we should fix that, making that flag AutoOnOff?
> > > > > > >
> > > > > > > Looks ok but auto may need some compat work since default is true.
> > > > > > >
> > > > > > > > Then, if user explicitly requested scsi=on with a modern
> > > > > > > > interface then we can error out cleanly.
> > > > > > > >
> > > > > > > > Given scsi flag is currently ignored, I think
> > > > > > > > this can be a patch on top.
> > > > > > >
> > > > > > > Looks like virtio_blk_handle_scsi_req() check this:
> > > > > > >
> > > > > > > if (!blk->conf.scsi) {
> > > > > > > status = VIRTIO_BLK_S_UNSUPP;
> > > > > > > goto fail;
> > > > > > > }
> > > > > >
> > > > > > So we should be checking the same condition for the feature flag and
> > > > > > error out in the init function if we have a VERSION_1 device and
> > > > > > blk->conf.scsi is set.
> > > > >
> > > > > Hm, I wonder how this plays with transports that want to make the
> > > > > virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
> > > > > only want to offer VERSION_1 if the driver negotiated revision >= 1.
> > > > > I'd need to check for !scsi as well before I can add this feature bit
> > > > > then? Have the init function set a blocker for VERSION_1 so that the
> > > > > driver may only negotiate revision 0?
> > > >
> > > >
> > > > We already handle this, do we not?
> > > (...)
> > > > So guest that doesn't negotiate revision >= 1 never gets to see
> > > > VIRTIO_F_VERSION_1.
> > >
> > > Not my question :) I was wondering about scsi vs. virtio-1 devices. And
> > > as I basically only want to make the decision on whether to offer
> > > VERSION_1 when the guest negotiated a revision, I cannot fence scsi
> > > during init, no?
> >
> > No, I don't think there's a lot of value in offering scsi only to
> > old guests that don't negotiate revision >= 1.
> >
> > If user asked for virtio 1 support then that by proxy implies scsi
> > passthrough does not work, and it won't work for legacy
> > guests too.
>
> This would imply that any transitional device cannot offer scsi,
> doesn't it?
Yes, and that's because as written, transitional devices must set
ANY_LAYOUT, but that's incompatible with scsi.
> We have two layers interacting here: virtio-blk which may or may not
> offer scsi support, and the transport layer which may or may not offer
> VERSION_1 support. Failing scsi commands if VERSION_1 has been
> negotiated makes sense to me; but I don't want to disable scsi config a
> priori because the driver might negotiate VERSION_1. This would imply
> that virtio-blk over virtio-ccw would never offer scsi once we enable
> virtio-1 support, and it kind of defeats the purpose of a transitional
> device for me.
>
> (The other way round - fail negotiating revison 1 if the device was
> configured with scsi support - makes more sense to me.)
Re: [Qemu-block] [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On 13/07/2015 16:41, Cornelia Huck wrote: > On Mon, 13 Jul 2015 16:34:30 +0200 > Paolo Bonzini wrote: >> On 13/07/2015 15:20, Cornelia Huck wrote: >>> This would imply that any transitional device cannot offer scsi, >>> doesn't it? >>> >>> We have two layers interacting here: virtio-blk which may or may not >>> offer scsi support, and the transport layer which may or may not offer >>> VERSION_1 support. Failing scsi commands if VERSION_1 has been >>> negotiated makes sense to me; but I don't want to disable scsi config a >>> priori because the driver might negotiate VERSION_1. This would imply >>> that virtio-blk over virtio-ccw would never offer scsi once we enable >>> virtio-1 support, and it kind of defeats the purpose of a transitional >>> device for me. >>> >>> (The other way round - fail negotiating revison 1 if the device was >>> configured with scsi support - makes more sense to me.) >> >> For newer machine types, it would make sense to block VIRTIO_BLK_F_SCSI >> altogether if !blk->conf.scsi. Would that fix the problem for you too? > > This is probably a sensible approach, and it can be contained in > virtio-block, no? Yes, I think so. Paolo
Re: [Qemu-block] [PATCH 1/2] atapi: abort transfers with 0 byte limits
On 07/13/2015 06:18 AM, Kevin Wolf wrote: > Am 11.07.2015 um 03:05 hat John Snow geschrieben: >> We're supposed to abort on transfers like this, unless we fill >> Word 125 of our IDENTIFY data with a default transfer size, which >> we don't currently do. >> >> This is an ATA error, not a SCSI/ATAPI one. >> See ATA8-ACS3 sections 7.17.6.49 or 7.21.5. >> >> If we don't do this, QEMU will loop forever trying to transfer >> zero bytes, which isn't particularly useful. >> >> Signed-off-by: John Snow > > Of course, one could argue that for a clean separation between the ATA > core and SCSI/ATAPI, this check should really be done in cmd_packet and > ide_abort_command() could stay static. And in fact, I do think it's > worthwhile to try getting a better separation in the mid term. > > However, for a 2.4 fix, this patch might be the right thing to do. > > The interesting part is that the SCSI command is already partially > handled before the error is returned. The required assumption here is > that the guest doesn't actually see any intermediate state between > issuing the command and getting the abort, so it doesn't notice that we > already started the command successfully. I _think_ that assumption > holds true. > > Reviewed-by: Kevin Wolf > Yes, in theory it should be handled in cmd_packet, but it needs to know in advance all of the commands that obey the byte count limit and that information didn't seem easy to extract from the specification. This is indeed a hack, but it's effective for now. --js
Re: [Qemu-block] [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, 13 Jul 2015 16:34:30 +0200 Paolo Bonzini wrote: > > > On 13/07/2015 15:20, Cornelia Huck wrote: > > This would imply that any transitional device cannot offer scsi, > > doesn't it? > > > > We have two layers interacting here: virtio-blk which may or may not > > offer scsi support, and the transport layer which may or may not offer > > VERSION_1 support. Failing scsi commands if VERSION_1 has been > > negotiated makes sense to me; but I don't want to disable scsi config a > > priori because the driver might negotiate VERSION_1. This would imply > > that virtio-blk over virtio-ccw would never offer scsi once we enable > > virtio-1 support, and it kind of defeats the purpose of a transitional > > device for me. > > > > (The other way round - fail negotiating revison 1 if the device was > > configured with scsi support - makes more sense to me.) > > For newer machine types, it would make sense to block VIRTIO_BLK_F_SCSI > altogether if !blk->conf.scsi. Would that fix the problem for you too? This is probably a sensible approach, and it can be contained in virtio-block, no?
Re: [Qemu-block] [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On 13/07/2015 15:20, Cornelia Huck wrote: > This would imply that any transitional device cannot offer scsi, > doesn't it? > > We have two layers interacting here: virtio-blk which may or may not > offer scsi support, and the transport layer which may or may not offer > VERSION_1 support. Failing scsi commands if VERSION_1 has been > negotiated makes sense to me; but I don't want to disable scsi config a > priori because the driver might negotiate VERSION_1. This would imply > that virtio-blk over virtio-ccw would never offer scsi once we enable > virtio-1 support, and it kind of defeats the purpose of a transitional > device for me. > > (The other way round - fail negotiating revison 1 if the device was > configured with scsi support - makes more sense to me.) For newer machine types, it would make sense to block VIRTIO_BLK_F_SCSI altogether if !blk->conf.scsi. Would that fix the problem for you too? Paolo
Re: [Qemu-block] [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, 13 Jul 2015 15:36:00 +0300
"Michael S. Tsirkin" wrote:
> On Mon, Jul 13, 2015 at 02:30:24PM +0200, Cornelia Huck wrote:
> > On Mon, 13 Jul 2015 15:22:52 +0300
> > "Michael S. Tsirkin" wrote:
> >
> > > On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote:
> > > > On Mon, 13 Jul 2015 11:56:51 +0200
> > > > Kevin Wolf wrote:
> > > >
> > > > > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
> > > > > >
> > > > > >
> > > > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > > > > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> > > > > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> > > > > > >>
> > > > > > >> Cc: Stefan Hajnoczi
> > > > > > >> Cc: Kevin Wolf
> > > > > > >> Cc: qemu-block@nongnu.org
> > > > > > >> Signed-off-by: Jason Wang
> > > > > > > Interesting, I noticed we have a field scsi - see
> > > > > > > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > > > > > > Author: Paolo Bonzini
> > > > > > > Date: Fri Dec 23 15:39:03 2011 +0100
> > > > > > >
> > > > > > > virtio-blk: refuse SG_IO requests with scsi=off
> > > > > > >
> > > > > > > but it doesn't seem to be propagated to guest features in
> > > > > > > any way.
> > > > > > >
> > > > > > > Maybe we should fix that, making that flag AutoOnOff?
> > > > > >
> > > > > > Looks ok but auto may need some compat work since default is true.
> > > > > >
> > > > > > > Then, if user explicitly requested scsi=on with a modern
> > > > > > > interface then we can error out cleanly.
> > > > > > >
> > > > > > > Given scsi flag is currently ignored, I think
> > > > > > > this can be a patch on top.
> > > > > >
> > > > > > Looks like virtio_blk_handle_scsi_req() check this:
> > > > > >
> > > > > > if (!blk->conf.scsi) {
> > > > > > status = VIRTIO_BLK_S_UNSUPP;
> > > > > > goto fail;
> > > > > > }
> > > > >
> > > > > So we should be checking the same condition for the feature flag and
> > > > > error out in the init function if we have a VERSION_1 device and
> > > > > blk->conf.scsi is set.
> > > >
> > > > Hm, I wonder how this plays with transports that want to make the
> > > > virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
> > > > only want to offer VERSION_1 if the driver negotiated revision >= 1.
> > > > I'd need to check for !scsi as well before I can add this feature bit
> > > > then? Have the init function set a blocker for VERSION_1 so that the
> > > > driver may only negotiate revision 0?
> > >
> > >
> > > We already handle this, do we not?
> > (...)
> > > So guest that doesn't negotiate revision >= 1 never gets to see
> > > VIRTIO_F_VERSION_1.
> >
> > Not my question :) I was wondering about scsi vs. virtio-1 devices. And
> > as I basically only want to make the decision on whether to offer
> > VERSION_1 when the guest negotiated a revision, I cannot fence scsi
> > during init, no?
>
> No, I don't think there's a lot of value in offering scsi only to
> old guests that don't negotiate revision >= 1.
>
> If user asked for virtio 1 support then that by proxy implies scsi
> passthrough does not work, and it won't work for legacy
> guests too.
This would imply that any transitional device cannot offer scsi,
doesn't it?
We have two layers interacting here: virtio-blk which may or may not
offer scsi support, and the transport layer which may or may not offer
VERSION_1 support. Failing scsi commands if VERSION_1 has been
negotiated makes sense to me; but I don't want to disable scsi config a
priori because the driver might negotiate VERSION_1. This would imply
that virtio-blk over virtio-ccw would never offer scsi once we enable
virtio-1 support, and it kind of defeats the purpose of a transitional
device for me.
(The other way round - fail negotiating revison 1 if the device was
configured with scsi support - makes more sense to me.)
Re: [Qemu-block] [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, Jul 13, 2015 at 02:30:24PM +0200, Cornelia Huck wrote:
> On Mon, 13 Jul 2015 15:22:52 +0300
> "Michael S. Tsirkin" wrote:
>
> > On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote:
> > > On Mon, 13 Jul 2015 11:56:51 +0200
> > > Kevin Wolf wrote:
> > >
> > > > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
> > > > >
> > > > >
> > > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > > > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> > > > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> > > > > >>
> > > > > >> Cc: Stefan Hajnoczi
> > > > > >> Cc: Kevin Wolf
> > > > > >> Cc: qemu-block@nongnu.org
> > > > > >> Signed-off-by: Jason Wang
> > > > > > Interesting, I noticed we have a field scsi - see
> > > > > > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > > > > > Author: Paolo Bonzini
> > > > > > Date: Fri Dec 23 15:39:03 2011 +0100
> > > > > >
> > > > > > virtio-blk: refuse SG_IO requests with scsi=off
> > > > > >
> > > > > > but it doesn't seem to be propagated to guest features in
> > > > > > any way.
> > > > > >
> > > > > > Maybe we should fix that, making that flag AutoOnOff?
> > > > >
> > > > > Looks ok but auto may need some compat work since default is true.
> > > > >
> > > > > > Then, if user explicitly requested scsi=on with a modern
> > > > > > interface then we can error out cleanly.
> > > > > >
> > > > > > Given scsi flag is currently ignored, I think
> > > > > > this can be a patch on top.
> > > > >
> > > > > Looks like virtio_blk_handle_scsi_req() check this:
> > > > >
> > > > > if (!blk->conf.scsi) {
> > > > > status = VIRTIO_BLK_S_UNSUPP;
> > > > > goto fail;
> > > > > }
> > > >
> > > > So we should be checking the same condition for the feature flag and
> > > > error out in the init function if we have a VERSION_1 device and
> > > > blk->conf.scsi is set.
> > >
> > > Hm, I wonder how this plays with transports that want to make the
> > > virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
> > > only want to offer VERSION_1 if the driver negotiated revision >= 1.
> > > I'd need to check for !scsi as well before I can add this feature bit
> > > then? Have the init function set a blocker for VERSION_1 so that the
> > > driver may only negotiate revision 0?
> >
> >
> > We already handle this, do we not?
> (...)
> > So guest that doesn't negotiate revision >= 1 never gets to see
> > VIRTIO_F_VERSION_1.
>
> Not my question :) I was wondering about scsi vs. virtio-1 devices. And
> as I basically only want to make the decision on whether to offer
> VERSION_1 when the guest negotiated a revision, I cannot fence scsi
> during init, no?
No, I don't think there's a lot of value in offering scsi only to
old guests that don't negotiate revision >= 1.
If user asked for virtio 1 support then that by proxy implies scsi
passthrough does not work, and it won't work for legacy
guests too.
> >
> > Maybe we should go further and additionally all bits >= 32 if
> > VIRTIO_F_VERSION_1 is clear, but that can wait
> > and we have no bits like that in 2.4.
> >
> Spec says bits >= 32 are only valid if we have VERSION_1, doesn't it?
> Sounds sensible.
Re: [Qemu-block] [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, 13 Jul 2015 15:22:52 +0300
"Michael S. Tsirkin" wrote:
> On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote:
> > On Mon, 13 Jul 2015 11:56:51 +0200
> > Kevin Wolf wrote:
> >
> > > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
> > > >
> > > >
> > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> > > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> > > > >>
> > > > >> Cc: Stefan Hajnoczi
> > > > >> Cc: Kevin Wolf
> > > > >> Cc: qemu-block@nongnu.org
> > > > >> Signed-off-by: Jason Wang
> > > > > Interesting, I noticed we have a field scsi - see
> > > > > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > > > > Author: Paolo Bonzini
> > > > > Date: Fri Dec 23 15:39:03 2011 +0100
> > > > >
> > > > > virtio-blk: refuse SG_IO requests with scsi=off
> > > > >
> > > > > but it doesn't seem to be propagated to guest features in
> > > > > any way.
> > > > >
> > > > > Maybe we should fix that, making that flag AutoOnOff?
> > > >
> > > > Looks ok but auto may need some compat work since default is true.
> > > >
> > > > > Then, if user explicitly requested scsi=on with a modern
> > > > > interface then we can error out cleanly.
> > > > >
> > > > > Given scsi flag is currently ignored, I think
> > > > > this can be a patch on top.
> > > >
> > > > Looks like virtio_blk_handle_scsi_req() check this:
> > > >
> > > > if (!blk->conf.scsi) {
> > > > status = VIRTIO_BLK_S_UNSUPP;
> > > > goto fail;
> > > > }
> > >
> > > So we should be checking the same condition for the feature flag and
> > > error out in the init function if we have a VERSION_1 device and
> > > blk->conf.scsi is set.
> >
> > Hm, I wonder how this plays with transports that want to make the
> > virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
> > only want to offer VERSION_1 if the driver negotiated revision >= 1.
> > I'd need to check for !scsi as well before I can add this feature bit
> > then? Have the init function set a blocker for VERSION_1 so that the
> > driver may only negotiate revision 0?
>
>
> We already handle this, do we not?
(...)
> So guest that doesn't negotiate revision >= 1 never gets to see
> VIRTIO_F_VERSION_1.
Not my question :) I was wondering about scsi vs. virtio-1 devices. And
as I basically only want to make the decision on whether to offer
VERSION_1 when the guest negotiated a revision, I cannot fence scsi
during init, no?
>
> Maybe we should go further and additionally all bits >= 32 if
> VIRTIO_F_VERSION_1 is clear, but that can wait
> and we have no bits like that in 2.4.
>
Spec says bits >= 32 are only valid if we have VERSION_1, doesn't it?
Sounds sensible.
Re: [Qemu-block] [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote:
> On Mon, 13 Jul 2015 11:56:51 +0200
> Kevin Wolf wrote:
>
> > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
> > >
> > >
> > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> > > >>
> > > >> Cc: Stefan Hajnoczi
> > > >> Cc: Kevin Wolf
> > > >> Cc: qemu-block@nongnu.org
> > > >> Signed-off-by: Jason Wang
> > > > Interesting, I noticed we have a field scsi - see
> > > > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > > > Author: Paolo Bonzini
> > > > Date: Fri Dec 23 15:39:03 2011 +0100
> > > >
> > > > virtio-blk: refuse SG_IO requests with scsi=off
> > > >
> > > > but it doesn't seem to be propagated to guest features in
> > > > any way.
> > > >
> > > > Maybe we should fix that, making that flag AutoOnOff?
> > >
> > > Looks ok but auto may need some compat work since default is true.
> > >
> > > > Then, if user explicitly requested scsi=on with a modern
> > > > interface then we can error out cleanly.
> > > >
> > > > Given scsi flag is currently ignored, I think
> > > > this can be a patch on top.
> > >
> > > Looks like virtio_blk_handle_scsi_req() check this:
> > >
> > > if (!blk->conf.scsi) {
> > > status = VIRTIO_BLK_S_UNSUPP;
> > > goto fail;
> > > }
> >
> > So we should be checking the same condition for the feature flag and
> > error out in the init function if we have a VERSION_1 device and
> > blk->conf.scsi is set.
>
> Hm, I wonder how this plays with transports that want to make the
> virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
> only want to offer VERSION_1 if the driver negotiated revision >= 1.
> I'd need to check for !scsi as well before I can add this feature bit
> then? Have the init function set a blocker for VERSION_1 so that the
> driver may only negotiate revision 0?
We already handle this, do we not?
if (features.index == 0) {
features.features = (uint32_t)vdev->host_features;
} else if (features.index == 1) {
features.features = (uint32_t)(vdev->host_features >> 32);
/*
* Don't offer version 1 to the guest if it did not
* negotiate at least revision 1.
*/
if (dev->revision <= 0) {
features.features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
}
} else {
/* Return zeroes if the guest supports more feature bits. */
features.features = 0;
}
So guest that doesn't negotiate revision >= 1 never gets to see
VIRTIO_F_VERSION_1.
Maybe we should go further and additionally all bits >= 32 if
VIRTIO_F_VERSION_1 is clear, but that can wait
and we have no bits like that in 2.4.
--
MST
Re: [Qemu-block] [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, 13 Jul 2015 11:56:51 +0200
Kevin Wolf wrote:
> Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
> >
> >
> > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> > >>
> > >> Cc: Stefan Hajnoczi
> > >> Cc: Kevin Wolf
> > >> Cc: qemu-block@nongnu.org
> > >> Signed-off-by: Jason Wang
> > > Interesting, I noticed we have a field scsi - see
> > > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > > Author: Paolo Bonzini
> > > Date: Fri Dec 23 15:39:03 2011 +0100
> > >
> > > virtio-blk: refuse SG_IO requests with scsi=off
> > >
> > > but it doesn't seem to be propagated to guest features in
> > > any way.
> > >
> > > Maybe we should fix that, making that flag AutoOnOff?
> >
> > Looks ok but auto may need some compat work since default is true.
> >
> > > Then, if user explicitly requested scsi=on with a modern
> > > interface then we can error out cleanly.
> > >
> > > Given scsi flag is currently ignored, I think
> > > this can be a patch on top.
> >
> > Looks like virtio_blk_handle_scsi_req() check this:
> >
> > if (!blk->conf.scsi) {
> > status = VIRTIO_BLK_S_UNSUPP;
> > goto fail;
> > }
>
> So we should be checking the same condition for the feature flag and
> error out in the init function if we have a VERSION_1 device and
> blk->conf.scsi is set.
Hm, I wonder how this plays with transports that want to make the
virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically
only want to offer VERSION_1 if the driver negotiated revision >= 1.
I'd need to check for !scsi as well before I can add this feature bit
then? Have the init function set a blocker for VERSION_1 so that the
driver may only negotiate revision 0?
Re: [Qemu-block] [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, Jul 13, 2015 at 05:00:51PM +0800, Jason Wang wrote:
>
>
> On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> >>
> >> Cc: Stefan Hajnoczi
> >> Cc: Kevin Wolf
> >> Cc: qemu-block@nongnu.org
> >> Signed-off-by: Jason Wang
> > Interesting, I noticed we have a field scsi - see
> > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > Author: Paolo Bonzini
> > Date: Fri Dec 23 15:39:03 2011 +0100
> >
> > virtio-blk: refuse SG_IO requests with scsi=off
> >
> > but it doesn't seem to be propagated to guest features in
> > any way.
> >
> > Maybe we should fix that, making that flag AutoOnOff?
>
> Looks ok but auto may need some compat work since default is true.
Right. Auto would then mean "!modern".
> > Then, if user explicitly requested scsi=on with a modern
> > interface then we can error out cleanly.
> >
> > Given scsi flag is currently ignored, I think
> > this can be a patch on top.
>
> Looks like virtio_blk_handle_scsi_req() check this:
>
> if (!blk->conf.scsi) {
> status = VIRTIO_BLK_S_UNSUPP;
> goto fail;
> }
>
> >
> >> ---
> >> hw/block/virtio-blk.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> >> index 6aefda4..f30ad25 100644
> >> --- a/hw/block/virtio-blk.c
> >> +++ b/hw/block/virtio-blk.c
> >> @@ -730,7 +730,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice
> >> *vdev, uint64_t features)
> >> virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
> >> virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
> >> virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
> >> -virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> >> +if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1))
> >> +virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> >>
> >> if (s->conf.config_wce) {
> >> virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> >> --
> >> 2.1.4
Re: [Qemu-block] [PATCH 1/2] atapi: abort transfers with 0 byte limits
Am 11.07.2015 um 03:05 hat John Snow geschrieben: > We're supposed to abort on transfers like this, unless we fill > Word 125 of our IDENTIFY data with a default transfer size, which > we don't currently do. > > This is an ATA error, not a SCSI/ATAPI one. > See ATA8-ACS3 sections 7.17.6.49 or 7.21.5. > > If we don't do this, QEMU will loop forever trying to transfer > zero bytes, which isn't particularly useful. > > Signed-off-by: John Snow Of course, one could argue that for a clean separation between the ATA core and SCSI/ATAPI, this check should really be done in cmd_packet and ide_abort_command() could stay static. And in fact, I do think it's worthwhile to try getting a better separation in the mid term. However, for a 2.4 fix, this patch might be the right thing to do. The interesting part is that the SCSI command is already partially handled before the error is returned. The required assumption here is that the guest doesn't actually see any intermediate state between issuing the command and getting the abort, so it doesn't notice that we already started the command successfully. I _think_ that assumption holds true. Reviewed-by: Kevin Wolf
[Qemu-block] RFC chaning zlib windowBits for QCOW2
Hi, I was experimenting a little with optimized zlib versions (like zlib-ng) to optimize the speed of compressed qcow2 generation. I found out that chaning the windowBits from -12 to -15 alone increases compression speed by about 15% while lowering the compressed size by about 6%. As a test case I used a Debian 8.1 disk image. What is your opinion in chaning the windowBits? The cost is a higher memory usage. The usage changes from 272k -> 384k for compression and from 4k -> 32k for decompression. Peter
Re: [Qemu-block] [Qemu-devel] [PATCH RFC 4/4] aio-posix: Use epoll in aio_poll
On Fri, Jul 10, 2015 at 08:46:44AM +0800, Fam Zheng wrote:
> On Wed, 07/08 11:58, Stefan Hajnoczi wrote:
> > On Wed, Jul 08, 2015 at 09:01:27AM +0800, Fam Zheng wrote:
> > > On Tue, 07/07 16:08, Stefan Hajnoczi wrote:
> > > > > +#define EPOLL_BATCH 128
> > > > > +static bool aio_poll_epoll(AioContext *ctx, bool blocking)
> > > > > +{
> > > > > +AioHandler *node;
> > > > > +bool was_dispatching;
> > > > > +int i, ret;
> > > > > +bool progress;
> > > > > +int64_t timeout;
> > > > > +struct epoll_event events[EPOLL_BATCH];
> > > > > +
> > > > > +aio_context_acquire(ctx);
> > > > > +was_dispatching = ctx->dispatching;
> > > > > +progress = false;
> > > > > +
> > > > > +/* aio_notify can avoid the expensive event_notifier_set if
> > > > > + * everything (file descriptors, bottom halves, timers) will
> > > > > + * be re-evaluated before the next blocking poll(). This is
> > > > > + * already true when aio_poll is called with blocking == false;
> > > > > + * if blocking == true, it is only true after poll() returns.
> > > > > + *
> > > > > + * If we're in a nested event loop, ctx->dispatching might be
> > > > > true.
> > > > > + * In that case we can restore it just before returning, but we
> > > > > + * have to clear it now.
> > > > > + */
> > > > > +aio_set_dispatching(ctx, !blocking);
> > > > > +
> > > > > +ctx->walking_handlers++;
> > > > > +
> > > > > +timeout = blocking ? aio_compute_timeout(ctx) : 0;
> > > > > +
> > > > > +if (timeout > 0) {
> > > > > +timeout = DIV_ROUND_UP(timeout, 100);
> > > > > +}
> > > >
> > > > I think you already posted the timerfd code in an earlier series. Why
> > > > degrade to millisecond precision? It needs to be fixed up anyway if the
> > > > main loop uses aio_poll() in the future.
> > >
> > > Because of a little complication: timeout here is always -1 for iothread,
> > > and
> > > what is interesting is that -1 actually requires an explicit
> > >
> > > timerfd_settime(timerfd, flags, &(struct itimerspec){{0, 0}}, NULL)
> > >
> > > to disable timerfd for this aio_poll(), which costs somethings. Passing
> > > -1 to
> > > epoll_wait() without this doesn't work because the timerfd is already
> > > added to
> > > the epollfd and may have an unexpected timeout set before.
> > >
> > > Of course we can cache the state and optimize, but I've not reasoned
> > > about what
> > > if another thread happens to call aio_poll() when we're in epoll_wait(),
> > > for
> > > example when the first aio_poll() has a positive timeout but the second
> > > one has
> > > -1.
> >
> > I'm not sure I understand the threads scenario since aio_poll_epoll()
> > has a big aio_context_acquire()/release() region that protects it, but I
> > guess the nested aio_poll() case is similar. Care needs to be taken so
> > the extra timerfd state is consistent.
>
> Nested aio_poll() has no racing on timerfd because the outer aio_poll()'s
> epoll_wait() would have already returned at the point of the inner aio_poll().
>
> Threads are different with Paolo's "release AioContext around blocking
> aio_poll()".
Ah, I see!
> >
> > The optimization can be added later unless the timerfd_settime() syscall
> > is so expensive that it defeats the advantage of epoll().
>
> That's the plan, and must be done before it get used by main loop.
I'd rather we merge correct code than fast code which violates the API.
Let's do nanosecond precision now, as advertised by the function names,
and optimize timerfd later.
pgpKvHv0kFt3n.pgp
Description: PGP signature
Re: [Qemu-block] [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
>
>
> On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> >>
> >> Cc: Stefan Hajnoczi
> >> Cc: Kevin Wolf
> >> Cc: qemu-block@nongnu.org
> >> Signed-off-by: Jason Wang
> > Interesting, I noticed we have a field scsi - see
> > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > Author: Paolo Bonzini
> > Date: Fri Dec 23 15:39:03 2011 +0100
> >
> > virtio-blk: refuse SG_IO requests with scsi=off
> >
> > but it doesn't seem to be propagated to guest features in
> > any way.
> >
> > Maybe we should fix that, making that flag AutoOnOff?
>
> Looks ok but auto may need some compat work since default is true.
>
> > Then, if user explicitly requested scsi=on with a modern
> > interface then we can error out cleanly.
> >
> > Given scsi flag is currently ignored, I think
> > this can be a patch on top.
>
> Looks like virtio_blk_handle_scsi_req() check this:
>
> if (!blk->conf.scsi) {
> status = VIRTIO_BLK_S_UNSUPP;
> goto fail;
> }
So we should be checking the same condition for the feature flag and
error out in the init function if we have a VERSION_1 device and
blk->conf.scsi is set.
> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> >> index 6aefda4..f30ad25 100644
> >> --- a/hw/block/virtio-blk.c
> >> +++ b/hw/block/virtio-blk.c
> >> @@ -730,7 +730,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice
> >> *vdev, uint64_t features)
> >> virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
> >> virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
> >> virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
> >> -virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> >> +if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1))
> >> +virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
Coding style: Braces are needed here. (Same in the other patch you CCed
me on).
Kevin
Re: [Qemu-block] [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
>> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
>>
>> Cc: Stefan Hajnoczi
>> Cc: Kevin Wolf
>> Cc: qemu-block@nongnu.org
>> Signed-off-by: Jason Wang
> Interesting, I noticed we have a field scsi - see
> commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> Author: Paolo Bonzini
> Date: Fri Dec 23 15:39:03 2011 +0100
>
> virtio-blk: refuse SG_IO requests with scsi=off
>
> but it doesn't seem to be propagated to guest features in
> any way.
>
> Maybe we should fix that, making that flag AutoOnOff?
Looks ok but auto may need some compat work since default is true.
> Then, if user explicitly requested scsi=on with a modern
> interface then we can error out cleanly.
>
> Given scsi flag is currently ignored, I think
> this can be a patch on top.
Looks like virtio_blk_handle_scsi_req() check this:
if (!blk->conf.scsi) {
status = VIRTIO_BLK_S_UNSUPP;
goto fail;
}
>
>> ---
>> hw/block/virtio-blk.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> index 6aefda4..f30ad25 100644
>> --- a/hw/block/virtio-blk.c
>> +++ b/hw/block/virtio-blk.c
>> @@ -730,7 +730,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice
>> *vdev, uint64_t features)
>> virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
>> virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
>> virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
>> -virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
>> +if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1))
>> +virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
>>
>> if (s->conf.config_wce) {
>> virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
>> --
>> 2.1.4
Re: [Qemu-block] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device
On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
>
> Cc: Stefan Hajnoczi
> Cc: Kevin Wolf
> Cc: qemu-block@nongnu.org
> Signed-off-by: Jason Wang
Interesting, I noticed we have a field scsi - see
commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
Author: Paolo Bonzini
Date: Fri Dec 23 15:39:03 2011 +0100
virtio-blk: refuse SG_IO requests with scsi=off
but it doesn't seem to be propagated to guest features in
any way.
Maybe we should fix that, making that flag AutoOnOff?
Then, if user explicitly requested scsi=on with a modern
interface then we can error out cleanly.
Given scsi flag is currently ignored, I think
this can be a patch on top.
> ---
> hw/block/virtio-blk.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 6aefda4..f30ad25 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -730,7 +730,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice
> *vdev, uint64_t features)
> virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
> virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
> virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
> -virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> +if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1))
> +virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
>
> if (s->conf.config_wce) {
> virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> --
> 2.1.4
