Re: [Qemu-devel] [PATCH] AHCI read/write corruption with int13h

[John Snow]

On 07/30/2014 02:11 PM, Eniac Zhang wrote:
> The AHCI controller code in Qemu has a bug that it will use the
>   wrong LBA address when Seabios tries to access LBA>128GB
>   (aka 127.5GB limit 
> http://www.hardwaresecrets.com/printpage/Hard-Disk-Drives-Capacity-Limits/482).
>   When we needs to access the LBA>0xfffffff, 28bit LBA is not sufficient
>   thus AHCI code needs to convert that into an LBA48 command, but it
>   didn’t set all the flags correctly, so low level code ends up reading a 
> sector at different address.
>
> how to duplicate:
> turn off the workaround in ahci.c, leaving the debug logs in core.c,
>   compile your qemu-system-x86_64 and then run:
> ./ qemu-system-x86_64 -fda dos622.img -drive 
> if=none,file=./blank.qcow2,id=hdc,media=disk -device ide-hd,drive=hdc,bus=ide.0 
> -M q35 -m 256M -vnc :1 -boot a
>
> Blank.qcow2 is a 300GB virtual disk file I pre-created, you can leave it blank 
> cause what’s on disk doesn’t matter in this test.  dos622.img is the dos622 
> floppy image with debug.com and a batch file:
> a 100
> mov si, 0200
> mov ax, 4200
> mov dx, 0080
> int 13
> ret
>
> ; 0x3: length
> e 200 10 00 7f 00 00 00 00 50
> ; lba
> e 208 88 69 e2 11 00 00 00 00
>
> r ip
> 100
> g
> r
> d 5000:0
> q
>
> Connect vncviewer, once dos boot is completed, type debug<int1342.bat to try to 
> read a sector beyond 128GB using int13h.
> ---
>   hw/ide/ahci.c | 15 +++++++++++++++
>   hw/ide/core.c | 20 ++++++++++++++++++++
>   2 files changed, 35 insertions(+)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 604152a..3e86953 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -938,6 +938,21 @@ static int handle_cmd(AHCIState *s, int port, int slot)
>                * do, I simply assume non-used fields as reserved and OR 
> everything
>                * together, independent of the command.
>                */
> +             // enable lba and lba48 mode, otherwise the bit won't get set 
> until the command is completed, cause read/write corruption
> +             ide_state->lba48 = (cmd_fis[2] == WIN_READDMA_EXT
> +               || cmd_fis[2] == WIN_READ_EXT
> +               || cmd_fis[2] == WIN_READDMA_QUEUED_EXT
> +               || cmd_fis[2] == WIN_READ_NATIVE_MAX_EXT
> +               || cmd_fis[2] == WIN_MULTREAD_EXT
> +               || cmd_fis[2] == WIN_WRITE_EXT
> +               || cmd_fis[2] == WIN_WRITEDMA_EXT
> +               || cmd_fis[2] == WIN_WRITEDMA_QUEUED_EXT
> +               || cmd_fis[2] == WIN_SET_MAX_EXT
> +               || cmd_fis[2] == WIN_MULTWRITE_EXT
> +               || cmd_fis[2] == WIN_VERIFY_EXT
> +               || cmd_fis[2] == WIN_FLUSH_CACHE_EXT
> +               );
> +             ide_state->select |= 0x40;
>               ide_set_sector(ide_state, ((uint64_t)cmd_fis[10] << 40)
>                                       | ((uint64_t)cmd_fis[9] << 32)
>                                       /* This is used for LBA48 commands */

I finally did my research on what is going on here and I understand the 
problem now. LBA48, LBA28 and CHS commands all treat these registers 
slightly differently, so our usage of "ide_set_sector" is a problem 
because we are not providing enough information to the IDE core layer 
for it to properly convert the sector number we give it back to the 
proper registers. ide_set_sector cannot fill the registers properly
without other guiding information to help it choose between LBA48, LBA28 
and CHS modes of operation.

The approach you have taken is to adjust the register values (manually) 
by inspecting the CMD type and coercing ide_set_sector to behave.

I think we can probably do this a different way: ide_set_sector takes an 
LBA value and decomposes it into its constituent registers. The way we 
use it here is to combine registers manually, then decompose them again 
in ide_set_sector.

What we should do here instead is to set all of the component IDE 
registers directly from the incoming FIS packet and then we can skip 
both this part of the patch /and/ delete the call to ide_set_sector 
entirely, which should clean the code up a good deal.

Normally the FIS packet decomposition would be a function of a SATA 
compliance layer, but we don't have a SATA layer, so it falls on AHCI to 
do so.

Specifically, we should be able to set ide_state->select equal to 
cmd_fis[7] which will handle enabling LBA over CHS for us (provided the 
packet received from the guest is correct). We can also set the sector, 
lcyl, hcyl, hob_sector, hob_lcyl and hob_hcyl fields.

Then, the IDE core layer will handle most of the necessary 
back-and-forth conversions for us automatically.

Please let me know if you'd like to take over this fix for you or not; I 
am adjusting a lot of the AHCI code and I could adopt your patch into a 
forthcoming patchset fairly easily.

Thank you very much for reporting and fixing this; if you find issues 
with AHCI misbehaving in the future, please CC me on any future patches.

> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index db191a6..988a935 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -445,9 +445,23 @@ void ide_transfer_stop(IDEState *s)
>       s->status &= ~DRQ_STAT;
>   }
>
> +#define DEBUG_SECTOR 1
> +

Please don't submit patches that turn on debug statements invariably. 
You can just omit this definition if you feel the other prints are worth 
keeping.

> +#if DEBUG_SECTOR
> +#define DPRINTF(fmt, ...)                                       \
> +    do { printf("debug_sector: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...)
> +#endif
> +
> +/* Leave a chunk of memory at the top of RAM for the BIOS ACPI tables.  */
> +#define ACPI_DATA_SIZE       0x10000
> +#define BIOS_CFG_IOPORT 0x510

Did this get pulled in from elsewhere?

> +
>   int64_t ide_get_sector(IDEState *s)
>   {
>       int64_t sector_num;
> +
>       if (s->select & 0x40) {
>           /* lba */
>         if (!s->lba48) {
> @@ -464,12 +478,18 @@ int64_t ide_get_sector(IDEState *s)
>           sector_num = ((s->hcyl << 8) | s->lcyl) * s->heads * s->sectors +
>               (s->select & 0x0f) * s->sectors + (s->sector - 1);
>       }
> +#if DEBUG_SECTOR
> +    DPRINTF("get_sector: %lx\n", sector_num);
> +#endif
>       return sector_num;
>   }
>
>   void ide_set_sector(IDEState *s, int64_t sector_num)
>   {
>       unsigned int cyl, r;
> +#if DEBUG_SECTOR
> +    DPRINTF("set_sector: %lx\n", sector_num);
> +#endif
>       if (s->select & 0x40) {
>         if (!s->lba48) {
>               s->select = (s->select & 0xf0) | (sector_num >> 24);

Once we confirm the bug is fixed, most of these debug prints are perhaps 
no longer necessary.

Thanks!
--
—js