[Qemu-devel] [PATCH] ide: Check validity of logical block size

[Qemu-devel] [PATCH] ide: Check validity of logical block size

[Kevin Wolf]

Our IDE emulation can't handle logical block sizes other than 512. Check
for it.

The original assumption was that other values would silently be ignored
(which is bad enough), but it's not quite true: The physical block size
is exposed in IDENTIFY DEVICE as a multiple of the logical block size.
Setting a logical block size therefore also corrupts the physical block
size (4096/4096 doesn't silently downgrade to 4096/512, but 512/512).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/qdev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index b4f096e..1ebb58d 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -163,6 +163,11 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
         return -1;
     }
 
+    if (dev->conf.logical_block_size != 512) {
+        error_report("logical_block_size must be 512 for IDE");
+        return -1;
+    }
+
     blkconf_serial(&dev->conf, &dev->serial);
     if (kind != IDE_CD) {
         blkconf_geometry(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] ide: Check validity of logical block size

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Our IDE emulation can't handle logical block sizes other than 512. Check
> for it.

I guess this is in part due to the 512 literals sprinkled all over the
IDE code.

> The original assumption was that other values would silently be ignored
> (which is bad enough), but it's not quite true: The physical block size
> is exposed in IDENTIFY DEVICE as a multiple of the logical block size.
> Setting a logical block size therefore also corrupts the physical block
> size (4096/4096 doesn't silently downgrade to 4096/512, but 512/512).
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] ide: Check validity of logical block size

[Kevin Wolf]

Am 03.12.2014 um 15:37 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Our IDE emulation can't handle logical block sizes other than 512. Check
> > for it.
> 
> I guess this is in part due to the 512 literals sprinkled all over the
> IDE code.

Yes, this is one part. The other part is where the 512 literals are
missing and sector numbers are passed on to the block layer.

Kevin

Re: [Qemu-devel] [PATCH] ide: Check validity of logical block size

[John Snow]

On 12/03/2014 07:35 AM, Kevin Wolf wrote:
> Our IDE emulation can't handle logical block sizes other than 512. Check
> for it.
>
> The original assumption was that other values would silently be ignored
> (which is bad enough), but it's not quite true: The physical block size
> is exposed in IDENTIFY DEVICE as a multiple of the logical block size.
> Setting a logical block size therefore also corrupts the physical block
> size (4096/4096 doesn't silently downgrade to 4096/512, but 512/512).
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   hw/ide/qdev.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index b4f096e..1ebb58d 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -163,6 +163,11 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>           return -1;
>       }
>
> +    if (dev->conf.logical_block_size != 512) {
> +        error_report("logical_block_size must be 512 for IDE");
> +        return -1;
> +    }
> +
>       blkconf_serial(&dev->conf, &dev->serial);
>       if (kind != IDE_CD) {
>           blkconf_geometry(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err);
>

There are so many 512 literals in the IDE code that I actually assumed 
we already enforced this, or otherwise just always used 512 regardless.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH] ide: Check validity of logical block size

[Kevin Wolf]

Am 03.12.2014 um 13:35 hat Kevin Wolf geschrieben:
> Our IDE emulation can't handle logical block sizes other than 512. Check
> for it.
> 
> The original assumption was that other values would silently be ignored
> (which is bad enough), but it's not quite true: The physical block size
> is exposed in IDENTIFY DEVICE as a multiple of the logical block size.
> Setting a logical block size therefore also corrupts the physical block
> size (4096/4096 doesn't silently downgrade to 4096/512, but 512/512).
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Applied to block-next.

Kevin