From 98d98912238d9f4f4c41bda0a3d944d0cff934ce Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 24 Jul 2020 01:22:54 -0400 Subject: ide: rename cmd_write to ctrl_write It's the Control register, part of the Control block -- Command is misleading here. Rename all related functions and constants. Signed-off-by: John Snow Reviewed-by: Philippe Mathieu-Daudé --- include/hw/ide/internal.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'include') diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h index 8a95ad8c4d..a23bb2f348 100644 --- a/include/hw/ide/internal.h +++ b/include/hw/ide/internal.h @@ -57,8 +57,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(IDEBus, IDE_BUS) #define REL 0x04 #define TAG_MASK 0xf8 -#define IDE_CMD_RESET 0x04 -#define IDE_CMD_DISABLE_IRQ 0x02 +/* Bits of Device Control register */ +#define IDE_CTRL_RESET 0x04 +#define IDE_CTRL_DISABLE_IRQ 0x02 /* ACS-2 T13/2015-D Table B.2 Command codes */ #define WIN_NOP 0x00 @@ -559,7 +560,7 @@ static inline IDEState *idebus_active_if(IDEBus *bus) static inline void ide_set_irq(IDEBus *bus) { - if (!(bus->cmd & IDE_CMD_DISABLE_IRQ)) { + if (!(bus->cmd & IDE_CTRL_DISABLE_IRQ)) { qemu_irq_raise(bus->irq); } } @@ -598,7 +599,7 @@ void ide_atapi_io_error(IDEState *s, int ret); void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val); uint32_t ide_ioport_read(void *opaque, uint32_t addr1); uint32_t ide_status_read(void *opaque, uint32_t addr); -void ide_cmd_write(void *opaque, uint32_t addr, uint32_t val); +void ide_ctrl_write(void *opaque, uint32_t addr, uint32_t val); void ide_data_writew(void *opaque, uint32_t addr, uint32_t val); uint32_t ide_data_readw(void *opaque, uint32_t addr); void ide_data_writel(void *opaque, uint32_t addr, uint32_t val); -- cgit v1.2.3-55-g7522 From be8c9423dec7bd0a0af7f57ecbbcb2718db72555 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 24 Jul 2020 01:22:56 -0400 Subject: ide: model HOB correctly I have been staring at this FIXME for years and I never knew what it meant. I finally stumbled across it! When writing to the command registers, the old value is shifted into a HOB copy of the register and the new value is written into the primary register. When reading registers, the value retrieved is dependent on the HOB bit in the CONTROL register. By setting bit 7 (0x80) in CONTROL, any register read will, if it has one, yield the HOB value for that register instead. Our code has a problem: We were using bit 7 of the DEVICE register to model this. We use bus->cmd roughly as the control register already, as it stores the value from ide_ctrl_write. Lastly, all command register writes reset the HOB, so fix that, too. Signed-off-by: John Snow --- hw/ide/core.c | 15 +++++++-------- include/hw/ide/internal.h | 1 + 2 files changed, 8 insertions(+), 8 deletions(-) (limited to 'include') diff --git a/hw/ide/core.c b/hw/ide/core.c index 29dc5dc4b4..6ececa5dfe 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -1215,8 +1215,7 @@ static void ide_cmd_lba48_transform(IDEState *s, int lba48) static void ide_clear_hob(IDEBus *bus) { /* any write clears HOB high bit of device control register */ - bus->ifs[0].select &= ~(1 << 7); - bus->ifs[1].select &= ~(1 << 7); + bus->cmd &= ~(IDE_CTRL_HOB); } /* IOport [W]rite [R]egisters */ @@ -1256,12 +1255,14 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val) return; } + /* NOTE: Device0 and Device1 both receive incoming register writes. + * (They're on the same bus! They have to!) */ + switch (reg_num) { case 0: break; case ATA_IOPORT_WR_FEATURES: ide_clear_hob(bus); - /* NOTE: data is written to the two drives */ bus->ifs[0].hob_feature = bus->ifs[0].feature; bus->ifs[1].hob_feature = bus->ifs[1].feature; bus->ifs[0].feature = val; @@ -1296,7 +1297,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val) bus->ifs[1].hcyl = val; break; case ATA_IOPORT_WR_DEVICE_HEAD: - /* FIXME: HOB readback uses bit 7 */ + ide_clear_hob(bus); bus->ifs[0].select = val | 0xa0; bus->ifs[1].select = val | 0xa0; /* select drive */ @@ -1304,7 +1305,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val) break; default: case ATA_IOPORT_WR_COMMAND: - /* command */ + ide_clear_hob(bus); ide_exec_cmd(bus, val); break; } @@ -2142,9 +2143,7 @@ uint32_t ide_ioport_read(void *opaque, uint32_t addr) int ret, hob; reg_num = addr & 7; - /* FIXME: HOB readback uses bit 7, but it's always set right now */ - //hob = s->select & (1 << 7); - hob = 0; + hob = bus->cmd & (IDE_CTRL_HOB); switch (reg_num) { case ATA_IOPORT_RR_DATA: ret = 0xff; diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h index a23bb2f348..5b7b0e47e6 100644 --- a/include/hw/ide/internal.h +++ b/include/hw/ide/internal.h @@ -58,6 +58,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(IDEBus, IDE_BUS) #define TAG_MASK 0xf8 /* Bits of Device Control register */ +#define IDE_CTRL_HOB 0x80 #define IDE_CTRL_RESET 0x04 #define IDE_CTRL_DISABLE_IRQ 0x02 -- cgit v1.2.3-55-g7522 From 0c7515e1c47372ae5d53f2e281b2ccd425ebbcc6 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 24 Jul 2020 01:22:58 -0400 Subject: ide: remove magic constants from the device register (In QEMU, we call this the "select" register.) My memory isn't good enough to memorize what these magic runes do. Label them to prevent mixups from happening in the future. Side note: I assume it's safe to always set 0xA0 even though ATA2 claims these bits are reserved, because ATA3 immediately reinstated that these bits should be always on. ATA4 and subsequent specs only claim that the fields are obsolete, so I assume it's safe to leave these set and that it should work with the widest array of guests. Signed-off-by: John Snow --- hw/ide/core.c | 26 ++++++++++++++------------ include/hw/ide/internal.h | 11 +++++++++++ 2 files changed, 25 insertions(+), 12 deletions(-) (limited to 'include') diff --git a/hw/ide/core.c b/hw/ide/core.c index afff355e5c..8a55352e4b 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -367,7 +367,7 @@ fill_buffer: static void ide_set_signature(IDEState *s) { - s->select &= 0xf0; /* clear head */ + s->select &= ~(ATA_DEV_HS); /* clear head */ /* put signature */ s->nsector = 1; s->sector = 1; @@ -586,7 +586,7 @@ void ide_transfer_stop(IDEState *s) int64_t ide_get_sector(IDEState *s) { int64_t sector_num; - if (s->select & 0x40) { + if (s->select & (ATA_DEV_LBA)) { if (s->lba48) { sector_num = ((int64_t)s->hob_hcyl << 40) | ((int64_t) s->hob_lcyl << 32) | @@ -595,13 +595,13 @@ int64_t ide_get_sector(IDEState *s) ((int64_t) s->lcyl << 8) | s->sector; } else { /* LBA28 */ - sector_num = ((s->select & 0x0f) << 24) | (s->hcyl << 16) | - (s->lcyl << 8) | s->sector; + sector_num = ((s->select & (ATA_DEV_LBA_MSB)) << 24) | + (s->hcyl << 16) | (s->lcyl << 8) | s->sector; } } else { /* CHS */ sector_num = ((s->hcyl << 8) | s->lcyl) * s->heads * s->sectors + - (s->select & 0x0f) * s->sectors + (s->sector - 1); + (s->select & (ATA_DEV_HS)) * s->sectors + (s->sector - 1); } return sector_num; @@ -610,7 +610,7 @@ int64_t ide_get_sector(IDEState *s) void ide_set_sector(IDEState *s, int64_t sector_num) { unsigned int cyl, r; - if (s->select & 0x40) { + if (s->select & (ATA_DEV_LBA)) { if (s->lba48) { s->sector = sector_num; s->lcyl = sector_num >> 8; @@ -620,7 +620,8 @@ void ide_set_sector(IDEState *s, int64_t sector_num) s->hob_hcyl = sector_num >> 40; } else { /* LBA28 */ - s->select = (s->select & 0xf0) | (sector_num >> 24); + s->select = (s->select & ~(ATA_DEV_LBA_MSB)) | + ((sector_num >> 24) & (ATA_DEV_LBA_MSB)); s->hcyl = (sector_num >> 16); s->lcyl = (sector_num >> 8); s->sector = (sector_num); @@ -631,7 +632,8 @@ void ide_set_sector(IDEState *s, int64_t sector_num) r = sector_num % (s->heads * s->sectors); s->hcyl = cyl >> 8; s->lcyl = cyl; - s->select = (s->select & 0xf0) | ((r / s->sectors) & 0x0f); + s->select = (s->select & ~(ATA_DEV_HS)) | + ((r / s->sectors) & (ATA_DEV_HS)); s->sector = (r % s->sectors) + 1; } } @@ -1302,10 +1304,10 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val) break; case ATA_IOPORT_WR_DEVICE_HEAD: ide_clear_hob(bus); - bus->ifs[0].select = val | 0xa0; - bus->ifs[1].select = val | 0xa0; + bus->ifs[0].select = val | (ATA_DEV_ALWAYS_ON); + bus->ifs[1].select = val | (ATA_DEV_ALWAYS_ON); /* select drive */ - bus->unit = (val >> 4) & 1; + bus->unit = (val & (ATA_DEV_SELECT)) ? 1 : 0; break; default: case ATA_IOPORT_WR_COMMAND: @@ -1343,7 +1345,7 @@ static void ide_reset(IDEState *s) s->hob_lcyl = 0; s->hob_hcyl = 0; - s->select = 0xa0; + s->select = (ATA_DEV_ALWAYS_ON); s->status = READY_STAT | SEEK_STAT; s->lba48 = 0; diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h index 5b7b0e47e6..2d09162eeb 100644 --- a/include/hw/ide/internal.h +++ b/include/hw/ide/internal.h @@ -29,6 +29,17 @@ OBJECT_DECLARE_SIMPLE_TYPE(IDEBus, IDE_BUS) #define MAX_IDE_DEVS 2 +/* Device/Head ("select") Register */ +#define ATA_DEV_SELECT 0x10 +/* ATA1,3: Defined as '1'. + * ATA2: Reserved. + * ATA3-7: obsolete. */ +#define ATA_DEV_ALWAYS_ON 0xA0 +#define ATA_DEV_LBA 0x40 +#define ATA_DEV_LBA_MSB 0x0F /* LBA 24:27 */ +#define ATA_DEV_HS 0x0F /* HS 3:0 */ + + /* Bits of HD_STATUS */ #define ERR_STAT 0x01 #define INDEX_STAT 0x02 -- cgit v1.2.3-55-g7522