Re: [Qemu-block] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver

2017-09-05 Thread Kevin Wolf 
Am 05.09.2017 um 12:05 hat Daniel P. Berrange geschrieben:
> On Tue, Sep 05, 2017 at 11:52:15AM +0200, Kevin Wolf wrote:
> > Am 31.08.2017 um 13:05 hat Daniel P. Berrange geschrieben:
> > > Signed-off-by: Daniel P. Berrange 
> > > ---
> > >  block/crypto.c | 32 
> > >  1 file changed, 16 insertions(+), 16 deletions(-)
> > 
> > I'm actually not sure about this one. Anything that is left after
> > patch 3 is probably not the arbitrary unit that qemu uses internally
> > for some interfaces, but the unit in which data is encrypted.
> > 
> > Basically, if just for fun we ever changed the unit of things like
> > bdrv_write() from 512 to 4096, then everything that needs to or at least
> > can be changed to use 4096 is BDRV_SECTOR_SIZE. But everything that
> > needs to stay on 512 (like I suspect most of the occurrences in the
> > crypto driver) is a different constant really (QCRYPTO_SECTOR_SIZE?).
> 
> Yeah for sure LUKSv1 & legacy qcow2 encryption need to stay 512
> forever, so if BDRV_SECTOR_SIZE were to change that would be a
> problem.
> 
> I wrote this on the assumption that we would never change BDRV_SECTOR_SIZE
> though. If we did need different sector sizes in the block layer I figured
> it would surely end up being a dynamic property per disk, rather than just
> changing the compile time constant. So from that POV I thought it ok to
> use BDRV_SECTOR_SIZE.

Yes, I agree that we'll never BDRV_SECTOR_SIZE in practice. I just use
this as a way to check whether this is really BDRV_SECTOR_SIZE, or
whether we have two different quantities that just happen to be both
512.

For example, QDICT_BUCKET_MAX is also 512, but that doesn't make it
right to use here, even though it works and is unlikely to change.

What we may actually want to do at some point (won't be in the near
future, though) is removing BDRV_SECTOR_SIZE because it refers to an
arbitrary unit in APIs and we're in the process of making everything
byte-based instead. This is another way to check whether some value is
BDRV_SECTOR_SIZE: If it will remain even after removing APIs with
sector-based parameters, it's probably something different.

> Perhaps I should instead add a qcrypto_block_get_sector_size() API though
> and use that, so we can fetch the sector size per encryption scheme in
> case we ever get a scheme using a non-512 sector size for encryption.

If you have a use for it, sure. Otherwise, I'd just suggest introducing
another constant for now.

Kevin

Re: [Qemu-block] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver

2017-09-05 Thread Daniel P. Berrange 
On Tue, Sep 05, 2017 at 11:52:15AM +0200, Kevin Wolf wrote:
> Am 31.08.2017 um 13:05 hat Daniel P. Berrange geschrieben:
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  block/crypto.c | 32 
> >  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> I'm actually not sure about this one. Anything that is left after
> patch 3 is probably not the arbitrary unit that qemu uses internally
> for some interfaces, but the unit in which data is encrypted.
> 
> Basically, if just for fun we ever changed the unit of things like
> bdrv_write() from 512 to 4096, then everything that needs to or at least
> can be changed to use 4096 is BDRV_SECTOR_SIZE. But everything that
> needs to stay on 512 (like I suspect most of the occurrences in the
> crypto driver) is a different constant really (QCRYPTO_SECTOR_SIZE?).

Yeah for sure LUKSv1 & legacy qcow2 encryption need to stay 512
forever, so if BDRV_SECTOR_SIZE were to change that would be a
problem.

I wrote this on the assumption that we would never change BDRV_SECTOR_SIZE
though. If we did need different sector sizes in the block layer I figured
it would surely end up being a dynamic property per disk, rather than just
changing the compile time constant. So from that POV I thought it ok to
use BDRV_SECTOR_SIZE.

Perhaps I should instead add a qcrypto_block_get_sector_size() API though
and use that, so we can fetch the sector size per encryption scheme in
case we ever get a scheme using a non-512 sector size for encryption.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver

2017-09-05 Thread Kevin Wolf 
Am 31.08.2017 um 13:05 hat Daniel P. Berrange geschrieben:
> Signed-off-by: Daniel P. Berrange 
> ---
>  block/crypto.c | 32 
>  1 file changed, 16 insertions(+), 16 deletions(-)

I'm actually not sure about this one. Anything that is left after
patch 3 is probably not the arbitrary unit that qemu uses internally
for some interfaces, but the unit in which data is encrypted.

Basically, if just for fun we ever changed the unit of things like
bdrv_write() from 512 to 4096, then everything that needs to or at least
can be changed to use 4096 is BDRV_SECTOR_SIZE. But everything that
needs to stay on 512 (like I suspect most of the occurrences in the
crypto driver) is a different constant really (QCRYPTO_SECTOR_SIZE?).

Kevin

Re: [Qemu-block] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver

2017-08-31 Thread Eric Blake 
On 08/31/2017 06:05 AM, Daniel P. Berrange wrote:
> Signed-off-by: Daniel P. Berrange 
> ---
>  block/crypto.c | 32 
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver

2017-08-31 Thread Daniel P. Berrange 
Signed-off-by: Daniel P. Berrange 
---
 block/crypto.c | 32 
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index cc8afe0e0d..e63f094379 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -392,16 +392,16 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t 
sector_num,
 QEMUIOVector hd_qiov;
 int ret = 0;
 size_t payload_offset =
-qcrypto_block_get_payload_offset(crypto->block) / 512;
+qcrypto_block_get_payload_offset(crypto->block) / BDRV_SECTOR_SIZE;
 
 qemu_iovec_init(_qiov, qiov->niov);
 
 /* Bounce buffer because we don't wish to expose cipher text
  * in qiov which points to guest memory.
  */
-cipher_data =
-qemu_try_blockalign(bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * 512,
-  qiov->size));
+cipher_data = qemu_try_blockalign(
+bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * BDRV_SECTOR_SIZE,
+  qiov->size));
 if (cipher_data == NULL) {
 ret = -ENOMEM;
 goto cleanup;
@@ -415,7 +415,7 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t 
sector_num,
 }
 
 qemu_iovec_reset(_qiov);
-qemu_iovec_add(_qiov, cipher_data, cur_nr_sectors * 512);
+qemu_iovec_add(_qiov, cipher_data, cur_nr_sectors * 
BDRV_SECTOR_SIZE);
 
 ret = bdrv_co_readv(bs->file,
 payload_offset + sector_num,
@@ -426,18 +426,18 @@ block_crypto_co_readv(BlockDriverState *bs, int64_t 
sector_num,
 
 if (qcrypto_block_decrypt(crypto->block,
   sector_num,
-  cipher_data, cur_nr_sectors * 512,
+  cipher_data, cur_nr_sectors * 
BDRV_SECTOR_SIZE,
   NULL) < 0) {
 ret = -EIO;
 goto cleanup;
 }
 
 qemu_iovec_from_buf(qiov, bytes_done,
-cipher_data, cur_nr_sectors * 512);
+cipher_data, cur_nr_sectors * BDRV_SECTOR_SIZE);
 
 remaining_sectors -= cur_nr_sectors;
 sector_num += cur_nr_sectors;
-bytes_done += cur_nr_sectors * 512;
+bytes_done += cur_nr_sectors * BDRV_SECTOR_SIZE;
 }
 
  cleanup:
@@ -459,16 +459,16 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t 
sector_num,
 QEMUIOVector hd_qiov;
 int ret = 0;
 size_t payload_offset =
-qcrypto_block_get_payload_offset(crypto->block) / 512;
+qcrypto_block_get_payload_offset(crypto->block) / BDRV_SECTOR_SIZE;
 
 qemu_iovec_init(_qiov, qiov->niov);
 
 /* Bounce buffer because we're not permitted to touch
  * contents of qiov - it points to guest memory.
  */
-cipher_data =
-qemu_try_blockalign(bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * 512,
-  qiov->size));
+cipher_data = qemu_try_blockalign(
+bs->file->bs, MIN(BLOCK_CRYPTO_MAX_SECTORS * BDRV_SECTOR_SIZE,
+  qiov->size));
 if (cipher_data == NULL) {
 ret = -ENOMEM;
 goto cleanup;
@@ -482,18 +482,18 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t 
sector_num,
 }
 
 qemu_iovec_to_buf(qiov, bytes_done,
-  cipher_data, cur_nr_sectors * 512);
+  cipher_data, cur_nr_sectors * BDRV_SECTOR_SIZE);
 
 if (qcrypto_block_encrypt(crypto->block,
   sector_num,
-  cipher_data, cur_nr_sectors * 512,
+  cipher_data, cur_nr_sectors * 
BDRV_SECTOR_SIZE,
   NULL) < 0) {
 ret = -EIO;
 goto cleanup;
 }
 
 qemu_iovec_reset(_qiov);
-qemu_iovec_add(_qiov, cipher_data, cur_nr_sectors * 512);
+qemu_iovec_add(_qiov, cipher_data, cur_nr_sectors * 
BDRV_SECTOR_SIZE);
 
 ret = bdrv_co_writev(bs->file,
  payload_offset + sector_num,
@@ -504,7 +504,7 @@ block_crypto_co_writev(BlockDriverState *bs, int64_t 
sector_num,
 
 remaining_sectors -= cur_nr_sectors;
 sector_num += cur_nr_sectors;
-bytes_done += cur_nr_sectors * 512;
+bytes_done += cur_nr_sectors * BDRV_SECTOR_SIZE;
 }
 
  cleanup:
-- 
2.13.5