* [PATCH 1/7] ide: rename cmd_write to ctrl_write
2020-07-24 5:22 [PATCH 0/7] IDE: SRST and other fixes John Snow
@ 2020-07-24 5:22 ` John Snow
2020-07-24 6:16 ` Philippe Mathieu-Daudé
2020-07-24 5:22 ` [PATCH 2/7] ide: don't tamper with the device register John Snow
` (5 subsequent siblings)
6 siblings, 1 reply; 10+ messages in thread
From: John Snow @ 2020-07-24 5:22 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, Alexander Bulekov, John Snow, qemu-block, philmd
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 <jsnow@redhat.com>
---
include/hw/ide/internal.h | 9 +++++----
hw/ide/core.c | 12 ++++++------
hw/ide/ioport.c | 2 +-
hw/ide/macio.c | 2 +-
hw/ide/mmio.c | 8 ++++----
hw/ide/pci.c | 12 ++++++------
hw/ide/trace-events | 2 +-
7 files changed, 24 insertions(+), 23 deletions(-)
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 1a7869e85d..10ea6e1e23 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -57,8 +57,9 @@ typedef struct IDEDMAOps IDEDMAOps;
#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
@@ -564,7 +565,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);
}
}
@@ -603,7 +604,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);
diff --git a/hw/ide/core.c b/hw/ide/core.c
index d997a78e47..b472220d65 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2235,25 +2235,25 @@ uint32_t ide_status_read(void *opaque, uint32_t addr)
return ret;
}
-void ide_cmd_write(void *opaque, uint32_t addr, uint32_t val)
+void ide_ctrl_write(void *opaque, uint32_t addr, uint32_t val)
{
IDEBus *bus = opaque;
IDEState *s;
int i;
- trace_ide_cmd_write(addr, val, bus);
+ trace_ide_ctrl_write(addr, val, bus);
/* common for both drives */
- if (!(bus->cmd & IDE_CMD_RESET) &&
- (val & IDE_CMD_RESET)) {
+ if (!(bus->cmd & IDE_CTRL_RESET) &&
+ (val & IDE_CTRL_RESET)) {
/* reset low to high */
for(i = 0;i < 2; i++) {
s = &bus->ifs[i];
s->status = BUSY_STAT | SEEK_STAT;
s->error = 0x01;
}
- } else if ((bus->cmd & IDE_CMD_RESET) &&
- !(val & IDE_CMD_RESET)) {
+ } else if ((bus->cmd & IDE_CTRL_RESET) &&
+ !(val & IDE_CTRL_RESET)) {
/* high to low */
for(i = 0;i < 2; i++) {
s = &bus->ifs[i];
diff --git a/hw/ide/ioport.c b/hw/ide/ioport.c
index ab1f4e5d9c..b613ff3bba 100644
--- a/hw/ide/ioport.c
+++ b/hw/ide/ioport.c
@@ -46,7 +46,7 @@ static const MemoryRegionPortio ide_portio_list[] = {
};
static const MemoryRegionPortio ide_portio2_list[] = {
- { 0, 1, 1, .read = ide_status_read, .write = ide_cmd_write },
+ { 0, 1, 1, .read = ide_status_read, .write = ide_ctrl_write },
PORTIO_END_OF_LIST(),
};
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 62a599a075..b270a10163 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -329,7 +329,7 @@ static void pmac_ide_write(void *opaque, hwaddr addr, uint64_t val,
case 0x8:
case 0x16:
if (size == 1) {
- ide_cmd_write(&d->bus, 0, val);
+ ide_ctrl_write(&d->bus, 0, val);
}
break;
case 0x20:
diff --git a/hw/ide/mmio.c b/hw/ide/mmio.c
index d233bd8c01..80b8a9eb09 100644
--- a/hw/ide/mmio.c
+++ b/hw/ide/mmio.c
@@ -95,16 +95,16 @@ static uint64_t mmio_ide_status_read(void *opaque, hwaddr addr,
return ide_status_read(&s->bus, 0);
}
-static void mmio_ide_cmd_write(void *opaque, hwaddr addr,
- uint64_t val, unsigned size)
+static void mmio_ide_ctrl_write(void *opaque, hwaddr addr,
+ uint64_t val, unsigned size)
{
MMIOState *s = opaque;
- ide_cmd_write(&s->bus, 0, val);
+ ide_ctrl_write(&s->bus, 0, val);
}
static const MemoryRegionOps mmio_ide_cs_ops = {
.read = mmio_ide_status_read,
- .write = mmio_ide_cmd_write,
+ .write = mmio_ide_ctrl_write,
.endianness = DEVICE_LITTLE_ENDIAN,
};
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 5e85c4ad17..59726ae453 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -38,7 +38,7 @@
(IDE_RETRY_DMA | IDE_RETRY_PIO | \
IDE_RETRY_READ | IDE_RETRY_FLUSH)
-static uint64_t pci_ide_cmd_read(void *opaque, hwaddr addr, unsigned size)
+static uint64_t pci_ide_status_read(void *opaque, hwaddr addr, unsigned size)
{
IDEBus *bus = opaque;
@@ -48,20 +48,20 @@ static uint64_t pci_ide_cmd_read(void *opaque, hwaddr addr, unsigned size)
return ide_status_read(bus, addr + 2);
}
-static void pci_ide_cmd_write(void *opaque, hwaddr addr,
- uint64_t data, unsigned size)
+static void pci_ide_ctrl_write(void *opaque, hwaddr addr,
+ uint64_t data, unsigned size)
{
IDEBus *bus = opaque;
if (addr != 2 || size != 1) {
return;
}
- ide_cmd_write(bus, addr + 2, data);
+ ide_ctrl_write(bus, addr + 2, data);
}
const MemoryRegionOps pci_ide_cmd_le_ops = {
- .read = pci_ide_cmd_read,
- .write = pci_ide_cmd_write,
+ .read = pci_ide_status_read,
+ .write = pci_ide_ctrl_write,
.endianness = DEVICE_LITTLE_ENDIAN,
};
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index 2e4162629f..6e357685f9 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -5,7 +5,7 @@
ide_ioport_read(uint32_t addr, const char *reg, uint32_t val, void *bus, void *s) "IDE PIO rd @ 0x%"PRIx32" (%s); val 0x%02"PRIx32"; bus %p IDEState %p"
ide_ioport_write(uint32_t addr, const char *reg, uint32_t val, void *bus, void *s) "IDE PIO wr @ 0x%"PRIx32" (%s); val 0x%02"PRIx32"; bus %p IDEState %p"
ide_status_read(uint32_t addr, uint32_t val, void *bus, void *s) "IDE PIO rd @ 0x%"PRIx32" (Alt Status); val 0x%02"PRIx32"; bus %p; IDEState %p"
-ide_cmd_write(uint32_t addr, uint32_t val, void *bus) "IDE PIO wr @ 0x%"PRIx32" (Device Control); val 0x%02"PRIx32"; bus %p"
+ide_ctrl_write(uint32_t addr, uint32_t val, void *bus) "IDE PIO wr @ 0x%"PRIx32" (Device Control); val 0x%02"PRIx32"; bus %p"
# Warning: verbose
ide_data_readw(uint32_t addr, uint32_t val, void *bus, void *s) "IDE PIO rd @ 0x%"PRIx32" (Data: Word); val 0x%04"PRIx32"; bus %p; IDEState %p"
ide_data_writew(uint32_t addr, uint32_t val, void *bus, void *s) "IDE PIO wr @ 0x%"PRIx32" (Data: Word); val 0x%04"PRIx32"; bus %p; IDEState %p"
--
2.26.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 1/7] ide: rename cmd_write to ctrl_write
2020-07-24 5:22 ` [PATCH 1/7] ide: rename cmd_write to ctrl_write John Snow
@ 2020-07-24 6:16 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-07-24 6:16 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: kwolf, Alexander Bulekov, qemu-block
On 7/24/20 7:22 AM, John Snow wrote:
> 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 <jsnow@redhat.com>
> ---
> include/hw/ide/internal.h | 9 +++++----
> hw/ide/core.c | 12 ++++++------
> hw/ide/ioport.c | 2 +-
> hw/ide/macio.c | 2 +-
> hw/ide/mmio.c | 8 ++++----
> hw/ide/pci.c | 12 ++++++------
> hw/ide/trace-events | 2 +-
> 7 files changed, 24 insertions(+), 23 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/7] ide: don't tamper with the device register
2020-07-24 5:22 [PATCH 0/7] IDE: SRST and other fixes John Snow
2020-07-24 5:22 ` [PATCH 1/7] ide: rename cmd_write to ctrl_write John Snow
@ 2020-07-24 5:22 ` John Snow
2020-07-24 5:22 ` [PATCH 3/7] ide: model HOB correctly John Snow
` (4 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: John Snow @ 2020-07-24 5:22 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, Alexander Bulekov, John Snow, qemu-block, philmd
In real ISA operation, register writes go out to an entire bus channel
and all listening devices receive the write. The devices do not toggle
the DEV bit based on their own configuration, nor does the HBA
intermediate or tamper with that value.
The reality of the matter is that DEV0/DEV1 accordingly will react to
command register writes based on whether or not the device was selected.
This does not fix a known bug, but it makes the code slightly simpler
and more obvious.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index b472220d65..5cedebc408 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1297,8 +1297,8 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
break;
case ATA_IOPORT_WR_DEVICE_HEAD:
/* FIXME: HOB readback uses bit 7 */
- bus->ifs[0].select = (val & ~0x10) | 0xa0;
- bus->ifs[1].select = (val | 0x10) | 0xa0;
+ bus->ifs[0].select = val | 0xa0;
+ bus->ifs[1].select = val | 0xa0;
/* select drive */
bus->unit = (val >> 4) & 1;
break;
--
2.26.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 3/7] ide: model HOB correctly
2020-07-24 5:22 [PATCH 0/7] IDE: SRST and other fixes John Snow
2020-07-24 5:22 ` [PATCH 1/7] ide: rename cmd_write to ctrl_write John Snow
2020-07-24 5:22 ` [PATCH 2/7] ide: don't tamper with the device register John Snow
@ 2020-07-24 5:22 ` John Snow
2020-07-24 5:22 ` [PATCH 4/7] ide: reorder set/get sector functions John Snow
` (3 subsequent siblings)
6 siblings, 0 replies; 10+ messages in thread
From: John Snow @ 2020-07-24 5:22 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, Alexander Bulekov, John Snow, qemu-block, philmd
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 <jsnow@redhat.com>
---
include/hw/ide/internal.h | 1 +
hw/ide/core.c | 15 +++++++--------
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 10ea6e1e23..16d806e0cf 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -58,6 +58,7 @@ typedef struct IDEDMAOps IDEDMAOps;
#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
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 5cedebc408..a880b91b47 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;
--
2.26.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 4/7] ide: reorder set/get sector functions
2020-07-24 5:22 [PATCH 0/7] IDE: SRST and other fixes John Snow
` (2 preceding siblings ...)
2020-07-24 5:22 ` [PATCH 3/7] ide: model HOB correctly John Snow
@ 2020-07-24 5:22 ` John Snow
2020-07-24 6:17 ` Philippe Mathieu-Daudé
2020-07-24 5:22 ` [PATCH 5/7] ide: remove magic constants from the device register John Snow
` (2 subsequent siblings)
6 siblings, 1 reply; 10+ messages in thread
From: John Snow @ 2020-07-24 5:22 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, Alexander Bulekov, John Snow, qemu-block, philmd
Reorder these just a pinch to make them more obvious at a glance what
the addressing mode is.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/core.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index a880b91b47..f35864070b 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -587,21 +587,23 @@ int64_t ide_get_sector(IDEState *s)
{
int64_t sector_num;
if (s->select & 0x40) {
- /* lba */
- if (!s->lba48) {
- sector_num = ((s->select & 0x0f) << 24) | (s->hcyl << 16) |
- (s->lcyl << 8) | s->sector;
- } else {
+ if (s->lba48) {
sector_num = ((int64_t)s->hob_hcyl << 40) |
((int64_t) s->hob_lcyl << 32) |
((int64_t) s->hob_sector << 24) |
((int64_t) s->hcyl << 16) |
((int64_t) s->lcyl << 8) | s->sector;
+ } else {
+ /* LBA28 */
+ sector_num = ((s->select & 0x0f) << 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);
}
+
return sector_num;
}
@@ -609,20 +611,22 @@ void ide_set_sector(IDEState *s, int64_t sector_num)
{
unsigned int cyl, r;
if (s->select & 0x40) {
- if (!s->lba48) {
- s->select = (s->select & 0xf0) | (sector_num >> 24);
- s->hcyl = (sector_num >> 16);
- s->lcyl = (sector_num >> 8);
- s->sector = (sector_num);
- } else {
+ if (s->lba48) {
s->sector = sector_num;
s->lcyl = sector_num >> 8;
s->hcyl = sector_num >> 16;
s->hob_sector = sector_num >> 24;
s->hob_lcyl = sector_num >> 32;
s->hob_hcyl = sector_num >> 40;
+ } else {
+ /* LBA28 */
+ s->select = (s->select & 0xf0) | (sector_num >> 24);
+ s->hcyl = (sector_num >> 16);
+ s->lcyl = (sector_num >> 8);
+ s->sector = (sector_num);
}
} else {
+ /* CHS */
cyl = sector_num / (s->heads * s->sectors);
r = sector_num % (s->heads * s->sectors);
s->hcyl = cyl >> 8;
--
2.26.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 5/7] ide: remove magic constants from the device register
2020-07-24 5:22 [PATCH 0/7] IDE: SRST and other fixes John Snow
` (3 preceding siblings ...)
2020-07-24 5:22 ` [PATCH 4/7] ide: reorder set/get sector functions John Snow
@ 2020-07-24 5:22 ` John Snow
2020-07-24 5:22 ` [PATCH 6/7] ide: clear interrupt on command write John Snow
2020-07-24 5:23 ` [PATCH 7/7] ide: cancel pending callbacks on SRST John Snow
6 siblings, 0 replies; 10+ messages in thread
From: John Snow @ 2020-07-24 5:22 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, Alexander Bulekov, John Snow, qemu-block, philmd
(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 <jsnow@redhat.com>
---
include/hw/ide/internal.h | 11 +++++++++++
hw/ide/core.c | 26 ++++++++++++++------------
2 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 16d806e0cf..d5a6ba1056 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -29,6 +29,17 @@ typedef struct IDEDMAOps IDEDMAOps;
#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
diff --git a/hw/ide/core.c b/hw/ide/core.c
index f35864070b..5f4f004312 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;
--
2.26.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 6/7] ide: clear interrupt on command write
2020-07-24 5:22 [PATCH 0/7] IDE: SRST and other fixes John Snow
` (4 preceding siblings ...)
2020-07-24 5:22 ` [PATCH 5/7] ide: remove magic constants from the device register John Snow
@ 2020-07-24 5:22 ` John Snow
2020-07-24 5:23 ` [PATCH 7/7] ide: cancel pending callbacks on SRST John Snow
6 siblings, 0 replies; 10+ messages in thread
From: John Snow @ 2020-07-24 5:22 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, Alexander Bulekov, John Snow, qemu-block, philmd
Not known to fix any bug, but I couldn't help but notice that ATA
specifies that writing to this register should clear an interrupt.
ATA7: Section 5.3.3 (Command register - Effect)
ATA6: Section 7.4.4 (Command register - Effect)
ATA5: Section 7.4.4 (Command register - Effect)
ATA4: Section 7.4.4 (Command register - Effect)
ATA3: Section 5.2.2 (Command register)
Other editions: try searching for the phrase "Writing this register".
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 5f4f004312..e4c69a7fde 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1312,6 +1312,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
default:
case ATA_IOPORT_WR_COMMAND:
ide_clear_hob(bus);
+ qemu_irq_lower(bus->irq);
ide_exec_cmd(bus, val);
break;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 7/7] ide: cancel pending callbacks on SRST
2020-07-24 5:22 [PATCH 0/7] IDE: SRST and other fixes John Snow
` (5 preceding siblings ...)
2020-07-24 5:22 ` [PATCH 6/7] ide: clear interrupt on command write John Snow
@ 2020-07-24 5:23 ` John Snow
6 siblings, 0 replies; 10+ messages in thread
From: John Snow @ 2020-07-24 5:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, Alexander Bulekov, John Snow, qemu-block, philmd
The SRST implementation did not keep up with the rest of IDE; it is
possible to perform a weak reset on an IDE device to remove the BSY/DRQ
bits, and then issue writes to the control/device registers which can
cause chaos with the state machine.
Fix that by actually performing a real reset.
Reported-by: Alexander Bulekov <alxndr@bu.edu>
Fixes: https://bugs.launchpad.net/qemu/+bug/1878253
Fixes: https://bugs.launchpad.net/qemu/+bug/1887303
Fixes: https://bugs.launchpad.net/qemu/+bug/1887309
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/core.c | 58 +++++++++++++++++++++++++++++++++++----------------
1 file changed, 40 insertions(+), 18 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index e4c69a7fde..4da689abdf 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2241,6 +2241,37 @@ uint32_t ide_status_read(void *opaque, uint32_t addr)
return ret;
}
+static void ide_perform_srst(IDEState *s)
+{
+ s->status |= BUSY_STAT;
+
+ /* Halt PIO (Via register state); PIO BH remains scheduled. */
+ ide_transfer_halt(s);
+
+ /* Cancel DMA -- may drain block device and invoke callbacks */
+ ide_cancel_dma_sync(s);
+
+ /* Cancel PIO callback, reset registers/signature, etc */
+ ide_reset(s);
+
+ if (s->drive_kind == IDE_CD) {
+ /* ATAPI drives do not set READY or SEEK */
+ s->status = 0x00;
+ }
+}
+
+static void ide_bus_perform_srst(void *opaque)
+{
+ IDEBus *bus = opaque;
+ IDEState *s;
+ int i;
+
+ for (i = 0; i < 2; i++) {
+ s = &bus->ifs[i];
+ ide_perform_srst(s);
+ }
+}
+
void ide_ctrl_write(void *opaque, uint32_t addr, uint32_t val)
{
IDEBus *bus = opaque;
@@ -2249,26 +2280,17 @@ void ide_ctrl_write(void *opaque, uint32_t addr, uint32_t val)
trace_ide_ctrl_write(addr, val, bus);
- /* common for both drives */
- if (!(bus->cmd & IDE_CTRL_RESET) &&
- (val & IDE_CTRL_RESET)) {
- /* reset low to high */
- for(i = 0;i < 2; i++) {
+ /* Device0 and Device1 each have their own control register,
+ * but QEMU models it as just one register in the controller. */
+ if ((bus->cmd & IDE_CTRL_RESET) &&
+ !(val & IDE_CTRL_RESET)) {
+ /* SRST triggers on falling edge */
+ for (i = 0; i < 2; i++) {
s = &bus->ifs[i];
- s->status = BUSY_STAT | SEEK_STAT;
- s->error = 0x01;
- }
- } else if ((bus->cmd & IDE_CTRL_RESET) &&
- !(val & IDE_CTRL_RESET)) {
- /* high to low */
- for(i = 0;i < 2; i++) {
- s = &bus->ifs[i];
- if (s->drive_kind == IDE_CD)
- s->status = 0x00; /* NOTE: READY is _not_ set */
- else
- s->status = READY_STAT | SEEK_STAT;
- ide_set_signature(s);
+ s->status |= BUSY_STAT;
}
+ aio_bh_schedule_oneshot(qemu_get_aio_context(),
+ ide_bus_perform_srst, bus);
}
bus->cmd = val;
--
2.26.2
^ permalink raw reply related [flat|nested] 10+ messages in thread