| Message ID | 20170831110518.10741-3-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/31/2017 06:05 AM, Daniel P. Berrange wrote: > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > block/crypto.c | 32 ++++++++++++++++---------------- > 1 file changed, 16 insertions(+), 16 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com>
Am 31.08.2017 um 13:05 hat Daniel P. Berrange geschrieben: > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > 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
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 <berrange@redhat.com> > > --- > > 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
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 <berrange@redhat.com> > > > --- > > > 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
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(&hd_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(&hd_qiov); - qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * 512); + qemu_iovec_add(&hd_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(&hd_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(&hd_qiov); - qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * 512); + qemu_iovec_add(&hd_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:
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- block/crypto.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)