* [Qemu-devel] [PATCH v3 1/9] ide: Move IDEDevice pointer from IDEBus to IDEState
2014-01-30 12:16 [Qemu-devel] [PATCH v3 0/9] Clean up IDE after completion of qdevification Markus Armbruster
@ 2014-01-30 12:16 ` Markus Armbruster
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 2/9] ide: Use IDEState member dev for "device connected" test Markus Armbruster
` (7 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2014-01-30 12:16 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, afaerber, stefanha, agraf
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/ide/core.c | 2 +-
hw/ide/internal.h | 3 +--
hw/ide/qdev.c | 12 +++---------
3 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 036cd4a..9087025 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -79,7 +79,7 @@ static void ide_identify(IDEState *s)
{
uint16_t *p;
unsigned int oldsize;
- IDEDevice *dev = s->unit ? s->bus->slave : s->bus->master;
+ IDEDevice *dev = s->dev;
if (s->identify_set) {
memcpy(s->io_buffer, s->identify_data, sizeof(s->identify_data));
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 0567a52..908d91d 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -341,6 +341,7 @@ enum ide_dma_cmd {
/* NOTE: IDEState represents in fact one drive */
struct IDEState {
IDEBus *bus;
+ IDEDevice *dev;
uint8_t unit;
/* ide config */
IDEDriveKind drive_kind;
@@ -447,8 +448,6 @@ struct IDEDMA {
struct IDEBus {
BusState qbus;
- IDEDevice *master;
- IDEDevice *slave;
IDEState ifs[2];
int bus_id;
int max_units;
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 18c4b7e..6ea1698 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -76,7 +76,7 @@ static int ide_qdev_init(DeviceState *qdev)
goto err;
}
if (dev->unit == -1) {
- dev->unit = bus->master ? 1 : 0;
+ dev->unit = bus->ifs[0].dev ? 1 : 0;
}
if (dev->unit >= bus->max_units) {
@@ -87,18 +87,12 @@ static int ide_qdev_init(DeviceState *qdev)
switch (dev->unit) {
case 0:
- if (bus->master) {
- error_report("IDE unit %d is in use", dev->unit);
- goto err;
- }
- bus->master = dev;
- break;
case 1:
- if (bus->slave) {
+ if (bus->ifs[dev->unit].dev) {
error_report("IDE unit %d is in use", dev->unit);
goto err;
}
- bus->slave = dev;
+ bus->ifs[dev->unit].dev = dev;
break;
default:
error_report("Invalid IDE unit %d", dev->unit);
--
1.8.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH v3 2/9] ide: Use IDEState member dev for "device connected" test
2014-01-30 12:16 [Qemu-devel] [PATCH v3 0/9] Clean up IDE after completion of qdevification Markus Armbruster
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 1/9] ide: Move IDEDevice pointer from IDEBus to IDEState Markus Armbruster
@ 2014-01-30 12:16 ` Markus Armbruster
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 3/9] ide: Don't block-align IDEState member smart_selftest_data Markus Armbruster
` (6 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2014-01-30 12:16 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, afaerber, stefanha, agraf
Testing dev is more obvious than testing bs, in my opinion.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/ide/ahci.c | 8 ++++----
hw/ide/core.c | 56 ++++++++++++++++++++++++++++-------------------------
hw/ide/microdrive.c | 2 +-
hw/ide/qdev.c | 2 +-
4 files changed, 36 insertions(+), 32 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index fbea9e8..eb6a6fe 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -85,7 +85,7 @@ static uint32_t ahci_port_read(AHCIState *s, int port, int offset)
val = pr->sig;
break;
case PORT_SCR_STAT:
- if (s->dev[port].port.ifs[0].bs) {
+ if (s->dev[port].port.ifs[0].dev) {
val = SATA_SCR_SSTATUS_DET_DEV_PRESENT_PHY_UP |
SATA_SCR_SSTATUS_SPD_GEN1 | SATA_SCR_SSTATUS_IPM_ACTIVE;
} else {
@@ -497,7 +497,7 @@ static void ahci_reset_port(AHCIState *s, int port)
d->init_d2h_sent = false;
ide_state = &s->dev[port].port.ifs[0];
- if (!ide_state->bs) {
+ if (!ide_state->dev) {
return;
}
@@ -523,7 +523,7 @@ static void ahci_reset_port(AHCIState *s, int port)
}
s->dev[port].port_state = STATE_RUN;
- if (!ide_state->bs) {
+ if (!ide_state->dev) {
s->dev[port].port_regs.sig = 0;
ide_state->status = SEEK_STAT | WRERR_STAT;
} else if (ide_state->drive_kind == IDE_CD) {
@@ -851,7 +851,7 @@ static int handle_cmd(AHCIState *s, int port, int slot)
/* The device we are working for */
ide_state = &s->dev[port].port.ifs[0];
- if (!ide_state->bs) {
+ if (!ide_state->dev) {
DPRINTF(port, "error: guest accessed unused port");
goto out;
}
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 9087025..dcfd92d 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -312,7 +312,7 @@ static void ide_set_signature(IDEState *s)
if (s->drive_kind == IDE_CD) {
s->lcyl = 0x14;
s->hcyl = 0xeb;
- } else if (s->bs) {
+ } else if (s->dev) {
s->lcyl = 0;
s->hcyl = 0;
} else {
@@ -818,7 +818,7 @@ static void ide_flush_cb(void *opaque, int ret)
void ide_flush_cache(IDEState *s)
{
- if (s->bs == NULL) {
+ if (!s->dev) {
ide_flush_cb(s, 0);
return;
}
@@ -1022,7 +1022,7 @@ static bool cmd_data_set_management(IDEState *s, uint8_t cmd)
{
switch (s->feature) {
case DSM_TRIM:
- if (s->bs) {
+ if (s->dev) {
ide_sector_start_dma(s, IDE_DMA_TRIM);
return false;
}
@@ -1035,7 +1035,7 @@ static bool cmd_data_set_management(IDEState *s, uint8_t cmd)
static bool cmd_identify(IDEState *s, uint8_t cmd)
{
- if (s->bs && s->drive_kind != IDE_CD) {
+ if (s->dev && s->drive_kind != IDE_CD) {
if (s->drive_kind != IDE_CFATA) {
ide_identify(s);
} else {
@@ -1085,7 +1085,7 @@ static bool cmd_read_multiple(IDEState *s, uint8_t cmd)
{
bool lba48 = (cmd == WIN_MULTREAD_EXT);
- if (!s->bs || !s->mult_sectors) {
+ if (!s->dev || !s->mult_sectors) {
ide_abort_command(s);
return true;
}
@@ -1101,7 +1101,7 @@ static bool cmd_write_multiple(IDEState *s, uint8_t cmd)
bool lba48 = (cmd == WIN_MULTWRITE_EXT);
int n;
- if (!s->bs || !s->mult_sectors) {
+ if (!s->dev || !s->mult_sectors) {
ide_abort_command(s);
return true;
}
@@ -1129,7 +1129,7 @@ static bool cmd_read_pio(IDEState *s, uint8_t cmd)
return true;
}
- if (!s->bs) {
+ if (!s->dev) {
ide_abort_command(s);
return true;
}
@@ -1145,7 +1145,7 @@ static bool cmd_write_pio(IDEState *s, uint8_t cmd)
{
bool lba48 = (cmd == WIN_WRITE_EXT);
- if (!s->bs) {
+ if (!s->dev) {
ide_abort_command(s);
return true;
}
@@ -1165,7 +1165,7 @@ static bool cmd_read_dma(IDEState *s, uint8_t cmd)
{
bool lba48 = (cmd == WIN_READDMA_EXT);
- if (!s->bs) {
+ if (!s->dev) {
ide_abort_command(s);
return true;
}
@@ -1180,7 +1180,7 @@ static bool cmd_write_dma(IDEState *s, uint8_t cmd)
{
bool lba48 = (cmd == WIN_WRITEDMA_EXT);
- if (!s->bs) {
+ if (!s->dev) {
ide_abort_command(s);
return true;
}
@@ -1231,7 +1231,7 @@ static bool cmd_set_features(IDEState *s, uint8_t cmd)
{
uint16_t *identify_data;
- if (!s->bs) {
+ if (!s->dev) {
ide_abort_command(s);
return true;
}
@@ -1710,8 +1710,9 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
#endif
s = idebus_active_if(bus);
/* ignore commands to non existent slave */
- if (s != bus->ifs && !s->bs)
+ if (s != bus->ifs && !s->dev) {
return;
+ }
/* Only DEVICE RESET is allowed while BSY or/and DRQ are set */
if ((s->status & (BUSY_STAT|DRQ_STAT)) && val != WIN_DEVICE_RESET)
@@ -1755,8 +1756,8 @@ uint32_t ide_ioport_read(void *opaque, uint32_t addr1)
ret = 0xff;
break;
case 1:
- if ((!bus->ifs[0].bs && !bus->ifs[1].bs) ||
- (s != bus->ifs && !s->bs))
+ if ((!bus->ifs[0].dev && !bus->ifs[1].dev) ||
+ (s != bus->ifs && !s->dev))
ret = 0;
else if (!hob)
ret = s->error;
@@ -1764,7 +1765,7 @@ uint32_t ide_ioport_read(void *opaque, uint32_t addr1)
ret = s->hob_feature;
break;
case 2:
- if (!bus->ifs[0].bs && !bus->ifs[1].bs)
+ if (!bus->ifs[0].dev && !bus->ifs[1].dev)
ret = 0;
else if (!hob)
ret = s->nsector & 0xff;
@@ -1772,7 +1773,7 @@ uint32_t ide_ioport_read(void *opaque, uint32_t addr1)
ret = s->hob_nsector;
break;
case 3:
- if (!bus->ifs[0].bs && !bus->ifs[1].bs)
+ if (!bus->ifs[0].dev && !bus->ifs[1].dev)
ret = 0;
else if (!hob)
ret = s->sector;
@@ -1780,7 +1781,7 @@ uint32_t ide_ioport_read(void *opaque, uint32_t addr1)
ret = s->hob_sector;
break;
case 4:
- if (!bus->ifs[0].bs && !bus->ifs[1].bs)
+ if (!bus->ifs[0].dev && !bus->ifs[1].dev)
ret = 0;
else if (!hob)
ret = s->lcyl;
@@ -1788,7 +1789,7 @@ uint32_t ide_ioport_read(void *opaque, uint32_t addr1)
ret = s->hob_lcyl;
break;
case 5:
- if (!bus->ifs[0].bs && !bus->ifs[1].bs)
+ if (!bus->ifs[0].dev && !bus->ifs[1].dev)
ret = 0;
else if (!hob)
ret = s->hcyl;
@@ -1796,18 +1797,20 @@ uint32_t ide_ioport_read(void *opaque, uint32_t addr1)
ret = s->hob_hcyl;
break;
case 6:
- if (!bus->ifs[0].bs && !bus->ifs[1].bs)
+ if (!bus->ifs[0].dev && !bus->ifs[1].dev) {
ret = 0;
- else
+ } else {
ret = s->select;
+ }
break;
default:
case 7:
- if ((!bus->ifs[0].bs && !bus->ifs[1].bs) ||
- (s != bus->ifs && !s->bs))
+ if ((!bus->ifs[0].dev && !bus->ifs[1].dev) ||
+ (s != bus->ifs && !s->dev)) {
ret = 0;
- else
+ } else {
ret = s->status;
+ }
qemu_irq_lower(bus->irq);
break;
}
@@ -1823,11 +1826,12 @@ uint32_t ide_status_read(void *opaque, uint32_t addr)
IDEState *s = idebus_active_if(bus);
int ret;
- if ((!bus->ifs[0].bs && !bus->ifs[1].bs) ||
- (s != bus->ifs && !s->bs))
+ if ((!bus->ifs[0].dev && !bus->ifs[1].dev) ||
+ (s != bus->ifs && !s->dev)) {
ret = 0;
- else
+ } else {
ret = s->status;
+ }
#ifdef DEBUG_IDE
printf("ide: read status addr=0x%x val=%02x\n", addr, ret);
#endif
diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
index 21d6495..29f0d64 100644
--- a/hw/ide/microdrive.c
+++ b/hw/ide/microdrive.c
@@ -247,7 +247,7 @@ static uint16_t md_common_read(PCMCIACardState *card, uint32_t at)
return ide_ioport_read(&s->bus, 0x1);
case 0xe: /* Alternate Status */
ifs = idebus_active_if(&s->bus);
- if (ifs->bs) {
+ if (ifs->dev) {
return ifs->status;
} else {
return 0;
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 6ea1698..7e8ddc2 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -120,7 +120,7 @@ int ide_get_geometry(BusState *bus, int unit,
{
IDEState *s = &DO_UPCAST(IDEBus, qbus, bus)->ifs[unit];
- if (s->drive_kind != IDE_HD || !s->bs) {
+ if (s->drive_kind != IDE_HD || !s->dev) {
return -1;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH v3 3/9] ide: Don't block-align IDEState member smart_selftest_data
2014-01-30 12:16 [Qemu-devel] [PATCH v3 0/9] Clean up IDE after completion of qdevification Markus Armbruster
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 1/9] ide: Move IDEDevice pointer from IDEBus to IDEState Markus Armbruster
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 2/9] ide: Use IDEState member dev for "device connected" test Markus Armbruster
@ 2014-01-30 12:16 ` Markus Armbruster
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 4/9] ide: Drop redundant IDEState member bs Markus Armbruster
` (5 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2014-01-30 12:16 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, afaerber, stefanha, agraf
All uses are simple array subscripts.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/ide/core.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index dcfd92d..6aaf6fa 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2164,8 +2164,7 @@ static void ide_init1(IDEBus *bus, int unit)
s->io_buffer = qemu_memalign(2048, s->io_buffer_total_len);
memset(s->io_buffer, 0, s->io_buffer_total_len);
- s->smart_selftest_data = qemu_blockalign(s->bs, 512);
- memset(s->smart_selftest_data, 0, 512);
+ s->smart_selftest_data = g_malloc0(512);
s->sector_write_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
ide_sector_write_timer_cb, s);
--
1.8.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH v3 4/9] ide: Drop redundant IDEState member bs
2014-01-30 12:16 [Qemu-devel] [PATCH v3 0/9] Clean up IDE after completion of qdevification Markus Armbruster
` (2 preceding siblings ...)
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 3/9] ide: Don't block-align IDEState member smart_selftest_data Markus Armbruster
@ 2014-01-30 12:16 ` Markus Armbruster
2014-02-03 14:54 ` Kevin Wolf
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 5/9] ide: Drop redundant IDEState geometry members Markus Armbruster
` (4 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2014-01-30 12:16 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, afaerber, stefanha, agraf
It's a copy of dev->conf.bs. The copy was needed for non-qdevified
controllers, which lacked dev.
Note how pci_piix3_xen_ide_unplug() cleared the copy. We'll get back
to that in the next few commits.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/ide/ahci.c | 11 +++++-----
hw/ide/atapi.c | 37 ++++++++++++++++++---------------
hw/ide/core.c | 62 ++++++++++++++++++++++++++++++-------------------------
hw/ide/internal.h | 3 +--
hw/ide/macio.c | 26 ++++++++++++-----------
hw/ide/piix.c | 4 ----
hw/ide/qdev.c | 2 +-
7 files changed, 76 insertions(+), 69 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index eb6a6fe..1afc37e 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -735,7 +735,7 @@ static void ncq_cb(void *opaque, int ret)
DPRINTF(ncq_tfs->drive->port_no, "NCQ transfer tag %d finished\n",
ncq_tfs->tag);
- bdrv_acct_done(ncq_tfs->drive->port.ifs[0].bs, &ncq_tfs->acct);
+ bdrv_acct_done(ncq_tfs->drive->port.ifs[0].dev->conf.bs, &ncq_tfs->acct);
qemu_sglist_destroy(&ncq_tfs->sglist);
ncq_tfs->used = 0;
}
@@ -746,6 +746,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
NCQFrame *ncq_fis = (NCQFrame*)cmd_fis;
uint8_t tag = ncq_fis->tag >> 3;
NCQTransferState *ncq_tfs = &s->dev[port].ncq_tfs[tag];
+ BlockDriverState *bs = ncq_tfs->drive->port.ifs[0].dev->conf.bs;
if (ncq_tfs->used) {
/* error - already in use */
@@ -785,9 +786,9 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
DPRINTF(port, "tag %d aio read %"PRId64"\n",
ncq_tfs->tag, ncq_tfs->lba);
- dma_acct_start(ncq_tfs->drive->port.ifs[0].bs, &ncq_tfs->acct,
+ dma_acct_start(bs, &ncq_tfs->acct,
&ncq_tfs->sglist, BDRV_ACCT_READ);
- ncq_tfs->aiocb = dma_bdrv_read(ncq_tfs->drive->port.ifs[0].bs,
+ ncq_tfs->aiocb = dma_bdrv_read(bs,
&ncq_tfs->sglist, ncq_tfs->lba,
ncq_cb, ncq_tfs);
break;
@@ -798,9 +799,9 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
DPRINTF(port, "tag %d aio write %"PRId64"\n",
ncq_tfs->tag, ncq_tfs->lba);
- dma_acct_start(ncq_tfs->drive->port.ifs[0].bs, &ncq_tfs->acct,
+ dma_acct_start(bs, &ncq_tfs->acct,
&ncq_tfs->sglist, BDRV_ACCT_WRITE);
- ncq_tfs->aiocb = dma_bdrv_write(ncq_tfs->drive->port.ifs[0].bs,
+ ncq_tfs->aiocb = dma_bdrv_write(bs,
&ncq_tfs->sglist, ncq_tfs->lba,
ncq_cb, ncq_tfs);
break;
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index f7d2009..3dc2de0 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -106,18 +106,19 @@ static void cd_data_to_raw(uint8_t *buf, int lba)
static int cd_read_sector(IDEState *s, int lba, uint8_t *buf, int sector_size)
{
+ BlockDriverState *bs = s->dev->conf.bs;
int ret;
switch(sector_size) {
case 2048:
- bdrv_acct_start(s->bs, &s->acct, 4 * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
- ret = bdrv_read(s->bs, (int64_t)lba << 2, buf, 4);
- bdrv_acct_done(s->bs, &s->acct);
+ bdrv_acct_start(bs, &s->acct, 4 * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
+ ret = bdrv_read(bs, (int64_t)lba << 2, buf, 4);
+ bdrv_acct_done(bs, &s->acct);
break;
case 2352:
- bdrv_acct_start(s->bs, &s->acct, 4 * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
- ret = bdrv_read(s->bs, (int64_t)lba << 2, buf + 16, 4);
- bdrv_acct_done(s->bs, &s->acct);
+ bdrv_acct_start(bs, &s->acct, 4 * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
+ ret = bdrv_read(bs, (int64_t)lba << 2, buf + 16, 4);
+ bdrv_acct_done(bs, &s->acct);
if (ret < 0)
return ret;
cd_data_to_raw(buf, lba);
@@ -253,7 +254,7 @@ static void ide_atapi_cmd_reply(IDEState *s, int size, int max_size)
s->io_buffer_index = 0;
if (s->atapi_dma) {
- bdrv_acct_start(s->bs, &s->acct, size, BDRV_ACCT_READ);
+ bdrv_acct_start(s->dev->conf.bs, &s->acct, size, BDRV_ACCT_READ);
s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
s->bus->dma->ops->start_dma(s->bus->dma, s,
ide_atapi_cmd_read_dma_cb);
@@ -349,13 +350,13 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
s->bus->dma->iov.iov_len = n * 4 * 512;
qemu_iovec_init_external(&s->bus->dma->qiov, &s->bus->dma->iov, 1);
- s->bus->dma->aiocb = bdrv_aio_readv(s->bs, (int64_t)s->lba << 2,
+ s->bus->dma->aiocb = bdrv_aio_readv(s->dev->conf.bs, (int64_t)s->lba << 2,
&s->bus->dma->qiov, n * 4,
ide_atapi_cmd_read_dma_cb, s);
return;
eot:
- bdrv_acct_done(s->bs, &s->acct);
+ bdrv_acct_done(s->dev->conf.bs, &s->acct);
s->bus->dma->ops->add_status(s->bus->dma, BM_STATUS_INT);
ide_set_inactive(s);
}
@@ -371,7 +372,8 @@ static void ide_atapi_cmd_read_dma(IDEState *s, int lba, int nb_sectors,
s->io_buffer_size = 0;
s->cd_sector_size = sector_size;
- bdrv_acct_start(s->bs, &s->acct, s->packet_transfer_size, BDRV_ACCT_READ);
+ bdrv_acct_start(s->dev->conf.bs, &s->acct, s->packet_transfer_size,
+ BDRV_ACCT_READ);
/* XXX: check if BUSY_STAT should be set */
s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
@@ -504,7 +506,7 @@ static unsigned int event_status_media(IDEState *s,
media_status = 0;
if (s->tray_open) {
media_status = MS_TRAY_OPEN;
- } else if (bdrv_is_inserted(s->bs)) {
+ } else if (bdrv_is_inserted(s->dev->conf.bs)) {
media_status = MS_MEDIA_PRESENT;
}
@@ -800,7 +802,7 @@ static void cmd_test_unit_ready(IDEState *s, uint8_t *buf)
static void cmd_prevent_allow_medium_removal(IDEState *s, uint8_t* buf)
{
s->tray_locked = buf[4] & 1;
- bdrv_lock_medium(s->bs, buf[4] & 1);
+ bdrv_lock_medium(s->dev->conf.bs, buf[4] & 1);
ide_atapi_cmd_ok(s);
}
@@ -884,14 +886,14 @@ static void cmd_start_stop_unit(IDEState *s, uint8_t* buf)
if (loej) {
if (!start && !s->tray_open && s->tray_locked) {
- sense = bdrv_is_inserted(s->bs)
+ sense = bdrv_is_inserted(s->dev->conf.bs)
? NOT_READY : ILLEGAL_REQUEST;
ide_atapi_cmd_error(s, sense, ASC_MEDIA_REMOVAL_PREVENTED);
return;
}
if (s->tray_open != !start) {
- bdrv_eject(s->bs, !start);
+ bdrv_eject(s->dev->conf.bs, !start);
s->tray_open = !start;
}
}
@@ -1094,6 +1096,7 @@ static const struct {
void ide_atapi_cmd(IDEState *s)
{
uint8_t *buf;
+ bool is_inserted;
buf = s->io_buffer;
#ifdef DEBUG_IDE_ATAPI
@@ -1124,8 +1127,9 @@ void ide_atapi_cmd(IDEState *s)
* GET_EVENT_STATUS_NOTIFICATION to detect such tray open/close
* states rely on this behavior.
*/
+ is_inserted = bdrv_is_inserted(s->dev->conf.bs);
if (!(atapi_cmd_table[s->io_buffer[0]].flags & ALLOW_UA) &&
- !s->tray_open && bdrv_is_inserted(s->bs) && s->cdrom_changed) {
+ !s->tray_open && is_inserted && s->cdrom_changed) {
if (s->cdrom_changed == 1) {
ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
@@ -1134,13 +1138,12 @@ void ide_atapi_cmd(IDEState *s)
ide_atapi_cmd_error(s, UNIT_ATTENTION, ASC_MEDIUM_MAY_HAVE_CHANGED);
s->cdrom_changed = 0;
}
-
return;
}
/* Report a Not Ready condition if appropriate for the command */
if ((atapi_cmd_table[s->io_buffer[0]].flags & CHECK_READY) &&
- (!media_present(s) || !bdrv_is_inserted(s->bs)))
+ (!media_present(s) || !is_inserted))
{
ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
return;
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 6aaf6fa..d220983 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -148,10 +148,11 @@ static void ide_identify(IDEState *s)
put_le16(p + 84, (1 << 14) | 0);
}
/* 14 = NOP supported, 5=WCACHE enabled, 0=SMART feature set enabled */
- if (bdrv_enable_write_cache(s->bs))
+ if (bdrv_enable_write_cache(s->dev->conf.bs)) {
put_le16(p + 85, (1 << 14) | (1 << 5) | 1);
- else
+ } else {
put_le16(p + 85, (1 << 14) | 1);
+ }
/* 13=flush_cache_ext,12=flush_cache,10=lba48 */
put_le16(p + 86, (1 << 13) | (1 <<12) | (1 << 10));
/* 14=set to 1, 8=has WWN, 1=SMART self test, 0=SMART error logging */
@@ -507,7 +508,7 @@ static void ide_sector_read_cb(void *opaque, int ret)
s->pio_aiocb = NULL;
s->status &= ~BUSY_STAT;
- bdrv_acct_done(s->bs, &s->acct);
+ bdrv_acct_done(s->dev->conf.bs, &s->acct);
if (ret != 0) {
if (ide_handle_rw_error(s, -ret, BM_STATUS_PIO_RETRY |
BM_STATUS_RETRY_READ)) {
@@ -531,6 +532,7 @@ static void ide_sector_read_cb(void *opaque, int ret)
void ide_sector_read(IDEState *s)
{
+ BlockDriverState *bs = s->dev->conf.bs;
int64_t sector_num;
int n;
@@ -558,8 +560,8 @@ void ide_sector_read(IDEState *s)
s->iov.iov_len = n * BDRV_SECTOR_SIZE;
qemu_iovec_init_external(&s->qiov, &s->iov, 1);
- bdrv_acct_start(s->bs, &s->acct, n * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
- s->pio_aiocb = bdrv_aio_readv(s->bs, sector_num, &s->qiov, n,
+ bdrv_acct_start(bs, &s->acct, n * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
+ s->pio_aiocb = bdrv_aio_readv(bs, sector_num, &s->qiov, n,
ide_sector_read_cb, s);
}
@@ -594,7 +596,8 @@ void ide_dma_error(IDEState *s)
static int ide_handle_rw_error(IDEState *s, int error, int op)
{
bool is_read = (op & BM_STATUS_RETRY_READ) != 0;
- BlockErrorAction action = bdrv_get_error_action(s->bs, is_read, error);
+ BlockErrorAction action = bdrv_get_error_action(s->dev->conf.bs,
+ is_read, error);
if (action == BDRV_ACTION_STOP) {
s->bus->dma->ops->set_unit(s->bus->dma, s->unit);
@@ -607,13 +610,14 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
ide_rw_error(s);
}
}
- bdrv_error_action(s->bs, action, is_read, error);
+ bdrv_error_action(s->dev->conf.bs, action, is_read, error);
return action != BDRV_ACTION_IGNORE;
}
void ide_dma_cb(void *opaque, int ret)
{
IDEState *s = opaque;
+ BlockDriverState *bs = s->dev->conf.bs;
int n;
int64_t sector_num;
bool stay_active = false;
@@ -673,15 +677,15 @@ void ide_dma_cb(void *opaque, int ret)
switch (s->dma_cmd) {
case IDE_DMA_READ:
- s->bus->dma->aiocb = dma_bdrv_read(s->bs, &s->sg, sector_num,
+ s->bus->dma->aiocb = dma_bdrv_read(bs, &s->sg, sector_num,
ide_dma_cb, s);
break;
case IDE_DMA_WRITE:
- s->bus->dma->aiocb = dma_bdrv_write(s->bs, &s->sg, sector_num,
+ s->bus->dma->aiocb = dma_bdrv_write(bs, &s->sg, sector_num,
ide_dma_cb, s);
break;
case IDE_DMA_TRIM:
- s->bus->dma->aiocb = dma_bdrv_io(s->bs, &s->sg, sector_num,
+ s->bus->dma->aiocb = dma_bdrv_io(bs, &s->sg, sector_num,
ide_issue_trim, ide_dma_cb, s,
DMA_DIRECTION_TO_DEVICE);
break;
@@ -690,7 +694,7 @@ void ide_dma_cb(void *opaque, int ret)
eot:
if (s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) {
- bdrv_acct_done(s->bs, &s->acct);
+ bdrv_acct_done(bs, &s->acct);
}
ide_set_inactive(s);
if (stay_active) {
@@ -707,12 +711,12 @@ static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
switch (dma_cmd) {
case IDE_DMA_READ:
- bdrv_acct_start(s->bs, &s->acct, s->nsector * BDRV_SECTOR_SIZE,
- BDRV_ACCT_READ);
+ bdrv_acct_start(s->dev->conf.bs, &s->acct,
+ s->nsector * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
break;
case IDE_DMA_WRITE:
- bdrv_acct_start(s->bs, &s->acct, s->nsector * BDRV_SECTOR_SIZE,
- BDRV_ACCT_WRITE);
+ bdrv_acct_start(s->dev->conf.bs, &s->acct,
+ s->nsector * BDRV_SECTOR_SIZE, BDRV_ACCT_WRITE);
break;
default:
break;
@@ -732,7 +736,7 @@ static void ide_sector_write_cb(void *opaque, int ret)
IDEState *s = opaque;
int n;
- bdrv_acct_done(s->bs, &s->acct);
+ bdrv_acct_done(s->dev->conf.bs, &s->acct);
s->pio_aiocb = NULL;
s->status &= ~BUSY_STAT;
@@ -777,6 +781,7 @@ static void ide_sector_write_cb(void *opaque, int ret)
void ide_sector_write(IDEState *s)
{
+ BlockDriverState *bs = s->dev->conf.bs;
int64_t sector_num;
int n;
@@ -794,8 +799,8 @@ void ide_sector_write(IDEState *s)
s->iov.iov_len = n * BDRV_SECTOR_SIZE;
qemu_iovec_init_external(&s->qiov, &s->iov, 1);
- bdrv_acct_start(s->bs, &s->acct, n * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
- s->pio_aiocb = bdrv_aio_writev(s->bs, sector_num, &s->qiov, n,
+ bdrv_acct_start(bs, &s->acct, n * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
+ s->pio_aiocb = bdrv_aio_writev(bs, sector_num, &s->qiov, n,
ide_sector_write_cb, s);
}
@@ -810,7 +815,7 @@ static void ide_flush_cb(void *opaque, int ret)
}
}
- bdrv_acct_done(s->bs, &s->acct);
+ bdrv_acct_done(s->dev->conf.bs, &s->acct);
s->status = READY_STAT | SEEK_STAT;
ide_async_cmd_done(s);
ide_set_irq(s->bus);
@@ -824,8 +829,8 @@ void ide_flush_cache(IDEState *s)
}
s->status |= BUSY_STAT;
- bdrv_acct_start(s->bs, &s->acct, 0, BDRV_ACCT_FLUSH);
- bdrv_aio_flush(s->bs, ide_flush_cb, s);
+ bdrv_acct_start(s->dev->conf.bs, &s->acct, 0, BDRV_ACCT_FLUSH);
+ bdrv_aio_flush(s->dev->conf.bs, ide_flush_cb, s);
}
static void ide_cfata_metadata_inquiry(IDEState *s)
@@ -888,7 +893,7 @@ static void ide_cd_change_cb(void *opaque, bool load)
uint64_t nb_sectors;
s->tray_open = !load;
- bdrv_get_geometry(s->bs, &nb_sectors);
+ bdrv_get_geometry(s->dev->conf.bs, &nb_sectors);
s->nb_sectors = nb_sectors;
/*
@@ -1239,12 +1244,12 @@ static bool cmd_set_features(IDEState *s, uint8_t cmd)
/* XXX: valid for CDROM ? */
switch (s->feature) {
case 0x02: /* write cache enable */
- bdrv_set_enable_write_cache(s->bs, true);
+ bdrv_set_enable_write_cache(s->dev->conf.bs, true);
identify_data = (uint16_t *)s->identify_data;
put_le16(identify_data + 85, (1 << 14) | (1 << 5) | 1);
return true;
case 0x82: /* write cache disable */
- bdrv_set_enable_write_cache(s->bs, false);
+ bdrv_set_enable_write_cache(s->dev->conf.bs, false);
identify_data = (uint16_t *)s->identify_data;
put_le16(identify_data + 85, (1 << 14) | 1);
ide_flush_cache(s);
@@ -2081,15 +2086,15 @@ static const BlockDevOps ide_cd_block_ops = {
.is_medium_locked = ide_cd_is_medium_locked,
};
-int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
+int ide_init_drive(IDEState *s, IDEDriveKind kind,
const char *version, const char *serial, const char *model,
uint64_t wwn,
uint32_t cylinders, uint32_t heads, uint32_t secs,
int chs_trans)
{
+ BlockDriverState *bs = s->dev->conf.bs;
uint64_t nb_sectors;
- s->bs = bs;
s->drive_kind = kind;
bdrv_get_geometry(bs, &nb_sectors);
@@ -2109,7 +2114,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
bdrv_set_dev_ops(bs, &ide_cd_block_ops, s);
bdrv_set_guest_block_size(bs, 2048);
} else {
- if (!bdrv_is_inserted(s->bs)) {
+ if (!bdrv_is_inserted(bs)) {
error_report("Device needs media, but drive is empty");
return -1;
}
@@ -2273,7 +2278,8 @@ static int ide_drive_post_load(void *opaque, int version_id)
IDEState *s = opaque;
if (s->identify_set) {
- bdrv_set_enable_write_cache(s->bs, !!(s->identify_data[85] & (1 << 5)));
+ bdrv_set_enable_write_cache(s->dev->conf.bs,
+ !!(s->identify_data[85] & (1 << 5)));
}
return 0;
}
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 908d91d..8f673f8 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -373,7 +373,6 @@ struct IDEState {
/* set for lba48 access */
uint8_t lba48;
- BlockDriverState *bs;
char version[9];
/* ATAPI specific */
struct unreported_events events;
@@ -546,7 +545,7 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr);
void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
uint32_t ide_data_readl(void *opaque, uint32_t addr);
-int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
+int ide_init_drive(IDEState *s, IDEDriveKind kind,
const char *version, const char *serial, const char *model,
uint64_t wwn,
uint32_t cylinders, uint32_t heads, uint32_t secs,
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index da94580..dfba4a5 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -134,7 +134,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
MACIO_DPRINTF("precopying unaligned %d bytes to %#" HWADDR_PRIx "\n",
unaligned, io->addr + io->len - unaligned);
- bdrv_read(s->bs, sector_num + nsector, io->remainder, 1);
+ bdrv_read(s->dev->conf.bs, sector_num + nsector, io->remainder, 1);
cpu_physical_memory_write(io->addr + io->len - unaligned,
io->remainder, unaligned);
@@ -164,14 +164,14 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
(s->lba << 2) + (s->io_buffer_index >> 9),
s->packet_transfer_size, s->dma_cmd);
- m->aiocb = dma_bdrv_read(s->bs, &s->sg,
+ m->aiocb = dma_bdrv_read(s->dev->conf.bs, &s->sg,
(int64_t)(s->lba << 2) + (s->io_buffer_index >> 9),
pmac_ide_atapi_transfer_cb, io);
return;
done:
MACIO_DPRINTF("done DMA\n");
- bdrv_acct_done(s->bs, &s->acct);
+ bdrv_acct_done(s->dev->conf.bs, &s->acct);
io->dma_end(opaque);
}
@@ -180,6 +180,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
DBDMA_io *io = opaque;
MACIOIDEState *m = io->opaque;
IDEState *s = idebus_active_if(&m->bus);
+ BlockDriverState *bs = s->dev->conf.bs;
int n = 0;
int64_t sector_num;
int unaligned;
@@ -229,7 +230,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
break;
case IDE_DMA_WRITE:
cpu_physical_memory_read(io->addr, p, remainder_len);
- bdrv_write(s->bs, sector_num - 1, io->remainder, 1);
+ bdrv_write(bs, sector_num - 1, io->remainder, 1);
break;
case IDE_DMA_TRIM:
break;
@@ -267,7 +268,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
switch (s->dma_cmd) {
case IDE_DMA_READ:
- bdrv_read(s->bs, sector_num + nsector, io->remainder, 1);
+ bdrv_read(bs, sector_num + nsector, io->remainder, 1);
cpu_physical_memory_write(io->addr + io->len - unaligned,
io->remainder, unaligned);
break;
@@ -306,15 +307,15 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
switch (s->dma_cmd) {
case IDE_DMA_READ:
- m->aiocb = dma_bdrv_read(s->bs, &s->sg, sector_num,
+ m->aiocb = dma_bdrv_read(bs, &s->sg, sector_num,
pmac_ide_transfer_cb, io);
break;
case IDE_DMA_WRITE:
- m->aiocb = dma_bdrv_write(s->bs, &s->sg, sector_num,
+ m->aiocb = dma_bdrv_write(bs, &s->sg, sector_num,
pmac_ide_transfer_cb, io);
break;
case IDE_DMA_TRIM:
- m->aiocb = dma_bdrv_io(s->bs, &s->sg, sector_num,
+ m->aiocb = dma_bdrv_io(bs, &s->sg, sector_num,
ide_issue_trim, pmac_ide_transfer_cb, io,
DMA_DIRECTION_TO_DEVICE);
break;
@@ -323,7 +324,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
done:
if (s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) {
- bdrv_acct_done(s->bs, &s->acct);
+ bdrv_acct_done(bs, &s->acct);
}
io->dma_end(io);
}
@@ -332,22 +333,23 @@ static void pmac_ide_transfer(DBDMA_io *io)
{
MACIOIDEState *m = io->opaque;
IDEState *s = idebus_active_if(&m->bus);
+ BlockDriverState *bs = s->dev->conf.bs;
MACIO_DPRINTF("\n");
s->io_buffer_size = 0;
if (s->drive_kind == IDE_CD) {
- bdrv_acct_start(s->bs, &s->acct, io->len, BDRV_ACCT_READ);
+ bdrv_acct_start(bs, &s->acct, io->len, BDRV_ACCT_READ);
pmac_ide_atapi_transfer_cb(io, 0);
return;
}
switch (s->dma_cmd) {
case IDE_DMA_READ:
- bdrv_acct_start(s->bs, &s->acct, io->len, BDRV_ACCT_READ);
+ bdrv_acct_start(bs, &s->acct, io->len, BDRV_ACCT_READ);
break;
case IDE_DMA_WRITE:
- bdrv_acct_start(s->bs, &s->acct, io->len, BDRV_ACCT_WRITE);
+ bdrv_acct_start(bs, &s->acct, io->len, BDRV_ACCT_WRITE);
break;
default:
break;
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 9b5960b..246e283 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -169,12 +169,9 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
static int pci_piix3_xen_ide_unplug(DeviceState *dev)
{
- PCIIDEState *pci_ide;
DriveInfo *di;
int i = 0;
- pci_ide = PCI_IDE(dev);
-
for (; i < 3; i++) {
di = drive_get_by_index(IF_IDE, i);
if (di != NULL && !di->media_cd) {
@@ -183,7 +180,6 @@ static int pci_piix3_xen_ide_unplug(DeviceState *dev)
bdrv_detach_dev(di->bdrv, ds);
}
bdrv_close(di->bdrv);
- pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
drive_put_ref(di);
}
}
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 7e8ddc2..f9a4d31 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -160,7 +160,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
return -1;
}
- if (ide_init_drive(s, dev->conf.bs, kind,
+ if (ide_init_drive(s, kind,
dev->version, dev->serial, dev->model, dev->wwn,
dev->conf.cyls, dev->conf.heads, dev->conf.secs,
dev->chs_trans) < 0) {
--
1.8.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH v3 4/9] ide: Drop redundant IDEState member bs
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 4/9] ide: Drop redundant IDEState member bs Markus Armbruster
@ 2014-02-03 14:54 ` Kevin Wolf
2014-02-05 14:07 ` Markus Armbruster
0 siblings, 1 reply; 18+ messages in thread
From: Kevin Wolf @ 2014-02-03 14:54 UTC (permalink / raw)
To: Markus Armbruster; +Cc: agraf, qemu-devel, stefanha, afaerber
Am 30.01.2014 um 13:16 hat Markus Armbruster geschrieben:
> It's a copy of dev->conf.bs. The copy was needed for non-qdevified
> controllers, which lacked dev.
>
> Note how pci_piix3_xen_ide_unplug() cleared the copy. We'll get back
> to that in the next few commits.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
So this pci_piix3_xen_ide_unplug() is what happens here:
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -169,12 +169,9 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
>
> static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> {
> - PCIIDEState *pci_ide;
> DriveInfo *di;
> int i = 0;
>
> - pci_ide = PCI_IDE(dev);
> -
> for (; i < 3; i++) {
> di = drive_get_by_index(IF_IDE, i);
> if (di != NULL && !di->media_cd) {
> @@ -183,7 +180,6 @@ static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> bdrv_detach_dev(di->bdrv, ds);
> }
> bdrv_close(di->bdrv);
> - pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
> drive_put_ref(di);
> }
> }
Probably I'm just missing the obvious, but it seems to me that the
copy was cleared here, while the original was left around. This was
no problem because the original was unused anyway after device
initialisation.
Now that the copy doesn't exist any more, we can't clear it, obviously,
but why don't we have to clear the original? Won't we still run the
"device is attached" code branches even though the device is really
unplugged?
Kevin
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH v3 4/9] ide: Drop redundant IDEState member bs
2014-02-03 14:54 ` Kevin Wolf
@ 2014-02-05 14:07 ` Markus Armbruster
2014-02-05 17:09 ` Stefano Stabellini
0 siblings, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2014-02-05 14:07 UTC (permalink / raw)
To: Kevin Wolf; +Cc: stefano.stabellini, afaerber, agraf, stefanha, qemu-devel
[Note cc: Stefano]
Kevin Wolf <kwolf@redhat.com> writes:
> Am 30.01.2014 um 13:16 hat Markus Armbruster geschrieben:
>> It's a copy of dev->conf.bs. The copy was needed for non-qdevified
>> controllers, which lacked dev.
>>
>> Note how pci_piix3_xen_ide_unplug() cleared the copy. We'll get back
>> to that in the next few commits.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> So this pci_piix3_xen_ide_unplug() is what happens here:
>
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -169,12 +169,9 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
>>
>> static int pci_piix3_xen_ide_unplug(DeviceState *dev)
>> {
>> - PCIIDEState *pci_ide;
>> DriveInfo *di;
>> int i = 0;
>>
>> - pci_ide = PCI_IDE(dev);
>> -
>> for (; i < 3; i++) {
>> di = drive_get_by_index(IF_IDE, i);
>> if (di != NULL && !di->media_cd) {
>> @@ -183,7 +180,6 @@ static int pci_piix3_xen_ide_unplug(DeviceState *dev)
>> bdrv_detach_dev(di->bdrv, ds);
>> }
>> bdrv_close(di->bdrv);
>> - pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
>> drive_put_ref(di);
>> }
>> }
>
> Probably I'm just missing the obvious, but it seems to me that the
> copy was cleared here, while the original was left around. This was
> no problem because the original was unused anyway after device
> initialisation.
>
> Now that the copy doesn't exist any more, we can't clear it, obviously,
> but why don't we have to clear the original? Won't we still run the
> "device is attached" code branches even though the device is really
> unplugged?
It's been a while since I wrote this. Almost 14 months, in fact.
No other IDE controller implements DeviceClass method unplug(). I can't
remember why the normal code to detach the backend (release_drive())
doesn't do here. Stefano, can you help?
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH v3 4/9] ide: Drop redundant IDEState member bs
2014-02-05 14:07 ` Markus Armbruster
@ 2014-02-05 17:09 ` Stefano Stabellini
2014-02-06 7:02 ` Markus Armbruster
0 siblings, 1 reply; 18+ messages in thread
From: Stefano Stabellini @ 2014-02-05 17:09 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, stefano.stabellini, agraf, qemu-devel, stefanha,
afaerber
On Wed, 5 Feb 2014, Markus Armbruster wrote:
> [Note cc: Stefano]
>
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 30.01.2014 um 13:16 hat Markus Armbruster geschrieben:
> >> It's a copy of dev->conf.bs. The copy was needed for non-qdevified
> >> controllers, which lacked dev.
> >>
> >> Note how pci_piix3_xen_ide_unplug() cleared the copy. We'll get back
> >> to that in the next few commits.
> >>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >
> > So this pci_piix3_xen_ide_unplug() is what happens here:
> >
> >> --- a/hw/ide/piix.c
> >> +++ b/hw/ide/piix.c
> >> @@ -169,12 +169,9 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
> >>
> >> static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> >> {
> >> - PCIIDEState *pci_ide;
> >> DriveInfo *di;
> >> int i = 0;
> >>
> >> - pci_ide = PCI_IDE(dev);
> >> -
> >> for (; i < 3; i++) {
> >> di = drive_get_by_index(IF_IDE, i);
> >> if (di != NULL && !di->media_cd) {
> >> @@ -183,7 +180,6 @@ static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> >> bdrv_detach_dev(di->bdrv, ds);
> >> }
> >> bdrv_close(di->bdrv);
> >> - pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
> >> drive_put_ref(di);
> >> }
> >> }
> >
> > Probably I'm just missing the obvious, but it seems to me that the
> > copy was cleared here, while the original was left around. This was
> > no problem because the original was unused anyway after device
> > initialisation.
> >
> > Now that the copy doesn't exist any more, we can't clear it, obviously,
> > but why don't we have to clear the original? Won't we still run the
> > "device is attached" code branches even though the device is really
> > unplugged?
>
> It's been a while since I wrote this. Almost 14 months, in fact.
>
> No other IDE controller implements DeviceClass method unplug(). I can't
> remember why the normal code to detach the backend (release_drive())
> doesn't do here. Stefano, can you help?
Too long to be able to remember the exact details :-/
However if you point me to a branch I can give it a try and tell you if
the unplug still works as it used to.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH v3 4/9] ide: Drop redundant IDEState member bs
2014-02-05 17:09 ` Stefano Stabellini
@ 2014-02-06 7:02 ` Markus Armbruster
2014-02-07 12:58 ` Stefano Stabellini
0 siblings, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2014-02-06 7:02 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Kevin Wolf, afaerber, agraf, stefanha, qemu-devel
Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
> On Wed, 5 Feb 2014, Markus Armbruster wrote:
>> [Note cc: Stefano]
>>
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > Am 30.01.2014 um 13:16 hat Markus Armbruster geschrieben:
>> >> It's a copy of dev->conf.bs. The copy was needed for non-qdevified
>> >> controllers, which lacked dev.
>> >>
>> >> Note how pci_piix3_xen_ide_unplug() cleared the copy. We'll get back
>> >> to that in the next few commits.
>> >>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >
>> > So this pci_piix3_xen_ide_unplug() is what happens here:
>> >
>> >> --- a/hw/ide/piix.c
>> >> +++ b/hw/ide/piix.c
>> >> @@ -169,12 +169,9 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
>> >>
>> >> static int pci_piix3_xen_ide_unplug(DeviceState *dev)
>> >> {
>> >> - PCIIDEState *pci_ide;
>> >> DriveInfo *di;
>> >> int i = 0;
>> >>
>> >> - pci_ide = PCI_IDE(dev);
>> >> -
>> >> for (; i < 3; i++) {
>> >> di = drive_get_by_index(IF_IDE, i);
>> >> if (di != NULL && !di->media_cd) {
>> >> @@ -183,7 +180,6 @@ static int pci_piix3_xen_ide_unplug(DeviceState *dev)
>> >> bdrv_detach_dev(di->bdrv, ds);
>> >> }
>> >> bdrv_close(di->bdrv);
>> >> - pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
>> >> drive_put_ref(di);
>> >> }
>> >> }
>> >
>> > Probably I'm just missing the obvious, but it seems to me that the
>> > copy was cleared here, while the original was left around. This was
>> > no problem because the original was unused anyway after device
>> > initialisation.
>> >
>> > Now that the copy doesn't exist any more, we can't clear it, obviously,
>> > but why don't we have to clear the original? Won't we still run the
>> > "device is attached" code branches even though the device is really
>> > unplugged?
>>
>> It's been a while since I wrote this. Almost 14 months, in fact.
>>
>> No other IDE controller implements DeviceClass method unplug(). I can't
>> remember why the normal code to detach the backend (release_drive())
>> doesn't do here. Stefano, can you help?
>
> Too long to be able to remember the exact details :-/
> However if you point me to a branch I can give it a try and tell you if
> the unplug still works as it used to.
Series trivially rebased to current master available at
git://repo.or.cz/qemu/armbru.git branch kill-non-qdev-ide
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH v3 4/9] ide: Drop redundant IDEState member bs
2014-02-06 7:02 ` Markus Armbruster
@ 2014-02-07 12:58 ` Stefano Stabellini
2014-02-11 14:41 ` Markus Armbruster
0 siblings, 1 reply; 18+ messages in thread
From: Stefano Stabellini @ 2014-02-07 12:58 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Stefano Stabellini, qemu-devel, agraf, stefanha,
afaerber
On Thu, 6 Feb 2014, Markus Armbruster wrote:
> Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
>
> > On Wed, 5 Feb 2014, Markus Armbruster wrote:
> >> [Note cc: Stefano]
> >>
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >>
> >> > Am 30.01.2014 um 13:16 hat Markus Armbruster geschrieben:
> >> >> It's a copy of dev->conf.bs. The copy was needed for non-qdevified
> >> >> controllers, which lacked dev.
> >> >>
> >> >> Note how pci_piix3_xen_ide_unplug() cleared the copy. We'll get back
> >> >> to that in the next few commits.
> >> >>
> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> >
> >> > So this pci_piix3_xen_ide_unplug() is what happens here:
> >> >
> >> >> --- a/hw/ide/piix.c
> >> >> +++ b/hw/ide/piix.c
> >> >> @@ -169,12 +169,9 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
> >> >>
> >> >> static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> >> >> {
> >> >> - PCIIDEState *pci_ide;
> >> >> DriveInfo *di;
> >> >> int i = 0;
> >> >>
> >> >> - pci_ide = PCI_IDE(dev);
> >> >> -
> >> >> for (; i < 3; i++) {
> >> >> di = drive_get_by_index(IF_IDE, i);
> >> >> if (di != NULL && !di->media_cd) {
> >> >> @@ -183,7 +180,6 @@ static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> >> >> bdrv_detach_dev(di->bdrv, ds);
> >> >> }
> >> >> bdrv_close(di->bdrv);
> >> >> - pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
> >> >> drive_put_ref(di);
> >> >> }
> >> >> }
> >> >
> >> > Probably I'm just missing the obvious, but it seems to me that the
> >> > copy was cleared here, while the original was left around. This was
> >> > no problem because the original was unused anyway after device
> >> > initialisation.
> >> >
> >> > Now that the copy doesn't exist any more, we can't clear it, obviously,
> >> > but why don't we have to clear the original? Won't we still run the
> >> > "device is attached" code branches even though the device is really
> >> > unplugged?
> >>
> >> It's been a while since I wrote this. Almost 14 months, in fact.
> >>
> >> No other IDE controller implements DeviceClass method unplug(). I can't
> >> remember why the normal code to detach the backend (release_drive())
> >> doesn't do here. Stefano, can you help?
> >
> > Too long to be able to remember the exact details :-/
> > However if you point me to a branch I can give it a try and tell you if
> > the unplug still works as it used to.
>
> Series trivially rebased to current master available at
> git://repo.or.cz/qemu/armbru.git branch kill-non-qdev-ide
Unfortunately it doesn't work: I can see both sda and xvda being present
after the system has booted.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH v3 4/9] ide: Drop redundant IDEState member bs
2014-02-07 12:58 ` Stefano Stabellini
@ 2014-02-11 14:41 ` Markus Armbruster
2014-02-12 9:16 ` Markus Armbruster
0 siblings, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2014-02-11 14:41 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Kevin Wolf, afaerber, qemu-devel, stefanha, agraf
Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
> On Thu, 6 Feb 2014, Markus Armbruster wrote:
>> Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
>>
>> > On Wed, 5 Feb 2014, Markus Armbruster wrote:
>> >> [Note cc: Stefano]
>> >>
>> >> Kevin Wolf <kwolf@redhat.com> writes:
>> >>
>> >> > Am 30.01.2014 um 13:16 hat Markus Armbruster geschrieben:
>> >> >> It's a copy of dev->conf.bs. The copy was needed for non-qdevified
>> >> >> controllers, which lacked dev.
>> >> >>
>> >> >> Note how pci_piix3_xen_ide_unplug() cleared the copy. We'll get back
>> >> >> to that in the next few commits.
>> >> >>
>> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> >
>> >> > So this pci_piix3_xen_ide_unplug() is what happens here:
>> >> >
>> >> >> --- a/hw/ide/piix.c
>> >> >> +++ b/hw/ide/piix.c
>> >> >> @@ -169,12 +169,9 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
>> >> >>
>> >> >> static int pci_piix3_xen_ide_unplug(DeviceState *dev)
>> >> >> {
>> >> >> - PCIIDEState *pci_ide;
>> >> >> DriveInfo *di;
>> >> >> int i = 0;
>> >> >>
>> >> >> - pci_ide = PCI_IDE(dev);
>> >> >> -
>> >> >> for (; i < 3; i++) {
>> >> >> di = drive_get_by_index(IF_IDE, i);
>> >> >> if (di != NULL && !di->media_cd) {
>> >> >> @@ -183,7 +180,6 @@ static int pci_piix3_xen_ide_unplug(DeviceState *dev)
>> >> >> bdrv_detach_dev(di->bdrv, ds);
>> >> >> }
>> >> >> bdrv_close(di->bdrv);
>> >> >> - pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
>> >> >> drive_put_ref(di);
>> >> >> }
>> >> >> }
>> >> >
>> >> > Probably I'm just missing the obvious, but it seems to me that the
>> >> > copy was cleared here, while the original was left around. This was
>> >> > no problem because the original was unused anyway after device
>> >> > initialisation.
>> >> >
>> >> > Now that the copy doesn't exist any more, we can't clear it, obviously,
>> >> > but why don't we have to clear the original? Won't we still run the
>> >> > "device is attached" code branches even though the device is really
>> >> > unplugged?
>> >>
>> >> It's been a while since I wrote this. Almost 14 months, in fact.
>> >>
>> >> No other IDE controller implements DeviceClass method unplug(). I can't
>> >> remember why the normal code to detach the backend (release_drive())
>> >> doesn't do here. Stefano, can you help?
>> >
>> > Too long to be able to remember the exact details :-/
>> > However if you point me to a branch I can give it a try and tell you if
>> > the unplug still works as it used to.
>>
>> Series trivially rebased to current master available at
>> git://repo.or.cz/qemu/armbru.git branch kill-non-qdev-ide
>
> Unfortunately it doesn't work: I can see both sda and xvda being present
> after the system has booted.
Thank you for testing. I'll try to clear the originals like Kevin
suggested. Hopefully, that'll pass your test.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH v3 4/9] ide: Drop redundant IDEState member bs
2014-02-11 14:41 ` Markus Armbruster
@ 2014-02-12 9:16 ` Markus Armbruster
2014-02-12 13:48 ` Stefano Stabellini
0 siblings, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2014-02-12 9:16 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Kevin Wolf, afaerber, qemu-devel, stefanha, agraf
Markus Armbruster <armbru@redhat.com> writes:
> Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
>
>> On Thu, 6 Feb 2014, Markus Armbruster wrote:
>>> Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
>>>
>>> > On Wed, 5 Feb 2014, Markus Armbruster wrote:
>>> >> [Note cc: Stefano]
>>> >>
>>> >> Kevin Wolf <kwolf@redhat.com> writes:
>>> >>
>>> >> > Am 30.01.2014 um 13:16 hat Markus Armbruster geschrieben:
>>> >> >> It's a copy of dev->conf.bs. The copy was needed for non-qdevified
>>> >> >> controllers, which lacked dev.
>>> >> >>
>>> >> >> Note how pci_piix3_xen_ide_unplug() cleared the copy. We'll get back
>>> >> >> to that in the next few commits.
>>> >> >>
>>> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> >> >
>>> >> > So this pci_piix3_xen_ide_unplug() is what happens here:
>>> >> >
>>> >> >> --- a/hw/ide/piix.c
>>> >> >> +++ b/hw/ide/piix.c
>>> >> >> @@ -169,12 +169,9 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
>>> >> >>
>>> >> >> static int pci_piix3_xen_ide_unplug(DeviceState *dev)
>>> >> >> {
>>> >> >> - PCIIDEState *pci_ide;
>>> >> >> DriveInfo *di;
>>> >> >> int i = 0;
>>> >> >>
>>> >> >> - pci_ide = PCI_IDE(dev);
>>> >> >> -
>>> >> >> for (; i < 3; i++) {
>>> >> >> di = drive_get_by_index(IF_IDE, i);
>>> >> >> if (di != NULL && !di->media_cd) {
>>> >> >> @@ -183,7 +180,6 @@ static int pci_piix3_xen_ide_unplug(DeviceState *dev)
>>> >> >> bdrv_detach_dev(di->bdrv, ds);
>>> >> >> }
>>> >> >> bdrv_close(di->bdrv);
>>> >> >> - pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
>>> >> >> drive_put_ref(di);
>>> >> >> }
>>> >> >> }
>>> >> >
>>> >> > Probably I'm just missing the obvious, but it seems to me that the
>>> >> > copy was cleared here, while the original was left around. This was
>>> >> > no problem because the original was unused anyway after device
>>> >> > initialisation.
>>> >> >
>>> >> > Now that the copy doesn't exist any more, we can't clear it, obviously,
>>> >> > but why don't we have to clear the original? Won't we still run the
>>> >> > "device is attached" code branches even though the device is really
>>> >> > unplugged?
>>> >>
>>> >> It's been a while since I wrote this. Almost 14 months, in fact.
>>> >>
>>> >> No other IDE controller implements DeviceClass method unplug(). I can't
>>> >> remember why the normal code to detach the backend (release_drive())
>>> >> doesn't do here. Stefano, can you help?
>>> >
>>> > Too long to be able to remember the exact details :-/
>>> > However if you point me to a branch I can give it a try and tell you if
>>> > the unplug still works as it used to.
>>>
>>> Series trivially rebased to current master available at
>>> git://repo.or.cz/qemu/armbru.git branch kill-non-qdev-ide
>>
>> Unfortunately it doesn't work: I can see both sda and xvda being present
>> after the system has booted.
>
> Thank you for testing. I'll try to clear the originals like Kevin
> suggested. Hopefully, that'll pass your test.
Stefano, I updated the branch, please retest it.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH v3 4/9] ide: Drop redundant IDEState member bs
2014-02-12 9:16 ` Markus Armbruster
@ 2014-02-12 13:48 ` Stefano Stabellini
0 siblings, 0 replies; 18+ messages in thread
From: Stefano Stabellini @ 2014-02-12 13:48 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Stefano Stabellini, agraf, qemu-devel, stefanha,
afaerber
On Wed, 12 Feb 2014, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
>
> > Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
> >
> >> On Thu, 6 Feb 2014, Markus Armbruster wrote:
> >>> Stefano Stabellini <stefano.stabellini@eu.citrix.com> writes:
> >>>
> >>> > On Wed, 5 Feb 2014, Markus Armbruster wrote:
> >>> >> [Note cc: Stefano]
> >>> >>
> >>> >> Kevin Wolf <kwolf@redhat.com> writes:
> >>> >>
> >>> >> > Am 30.01.2014 um 13:16 hat Markus Armbruster geschrieben:
> >>> >> >> It's a copy of dev->conf.bs. The copy was needed for non-qdevified
> >>> >> >> controllers, which lacked dev.
> >>> >> >>
> >>> >> >> Note how pci_piix3_xen_ide_unplug() cleared the copy. We'll get back
> >>> >> >> to that in the next few commits.
> >>> >> >>
> >>> >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >>> >> >
> >>> >> > So this pci_piix3_xen_ide_unplug() is what happens here:
> >>> >> >
> >>> >> >> --- a/hw/ide/piix.c
> >>> >> >> +++ b/hw/ide/piix.c
> >>> >> >> @@ -169,12 +169,9 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
> >>> >> >>
> >>> >> >> static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> >>> >> >> {
> >>> >> >> - PCIIDEState *pci_ide;
> >>> >> >> DriveInfo *di;
> >>> >> >> int i = 0;
> >>> >> >>
> >>> >> >> - pci_ide = PCI_IDE(dev);
> >>> >> >> -
> >>> >> >> for (; i < 3; i++) {
> >>> >> >> di = drive_get_by_index(IF_IDE, i);
> >>> >> >> if (di != NULL && !di->media_cd) {
> >>> >> >> @@ -183,7 +180,6 @@ static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> >>> >> >> bdrv_detach_dev(di->bdrv, ds);
> >>> >> >> }
> >>> >> >> bdrv_close(di->bdrv);
> >>> >> >> - pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
> >>> >> >> drive_put_ref(di);
> >>> >> >> }
> >>> >> >> }
> >>> >> >
> >>> >> > Probably I'm just missing the obvious, but it seems to me that the
> >>> >> > copy was cleared here, while the original was left around. This was
> >>> >> > no problem because the original was unused anyway after device
> >>> >> > initialisation.
> >>> >> >
> >>> >> > Now that the copy doesn't exist any more, we can't clear it, obviously,
> >>> >> > but why don't we have to clear the original? Won't we still run the
> >>> >> > "device is attached" code branches even though the device is really
> >>> >> > unplugged?
> >>> >>
> >>> >> It's been a while since I wrote this. Almost 14 months, in fact.
> >>> >>
> >>> >> No other IDE controller implements DeviceClass method unplug(). I can't
> >>> >> remember why the normal code to detach the backend (release_drive())
> >>> >> doesn't do here. Stefano, can you help?
> >>> >
> >>> > Too long to be able to remember the exact details :-/
> >>> > However if you point me to a branch I can give it a try and tell you if
> >>> > the unplug still works as it used to.
> >>>
> >>> Series trivially rebased to current master available at
> >>> git://repo.or.cz/qemu/armbru.git branch kill-non-qdev-ide
> >>
> >> Unfortunately it doesn't work: I can see both sda and xvda being present
> >> after the system has booted.
> >
> > Thank you for testing. I'll try to clear the originals like Kevin
> > suggested. Hopefully, that'll pass your test.
>
> Stefano, I updated the branch, please retest it.
QEMU exists with:
hw/ide/piix.c:183:pci_piix3_xen_ide_unplug: Object 0x7f172045b050 is not an instance of type ide-device
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v3 5/9] ide: Drop redundant IDEState geometry members
2014-01-30 12:16 [Qemu-devel] [PATCH v3 0/9] Clean up IDE after completion of qdevification Markus Armbruster
` (3 preceding siblings ...)
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 4/9] ide: Drop redundant IDEState member bs Markus Armbruster
@ 2014-01-30 12:16 ` Markus Armbruster
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 6/9] ide: Drop redundant IDEState member version Markus Armbruster
` (3 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2014-01-30 12:16 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, afaerber, stefanha, agraf
Members cylinders, heads, sectors, chs_trans are copies of
dev->conf.cyls, dev->conf.heads, dev->conf.secs, dev->chs_trans.
Copies were needed for non-qdevified controllers, which lacked dev.
Note that pci_piix3_xen_ide_unplug() did not clear the copies (it only
cleared the copy of bs). Begs the question whether stale data could
have been used after unplug. As far as I can tell, the copies were
used only when the copy of bs was non-null, thus no bug there.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/ide/core.c | 52 ++++++++++++++++++++++++----------------------------
hw/ide/internal.h | 5 +----
hw/ide/qdev.c | 12 +++++-------
3 files changed, 30 insertions(+), 39 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index d220983..93ccb6e 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -89,11 +89,11 @@ static void ide_identify(IDEState *s)
memset(s->io_buffer, 0, 512);
p = (uint16_t *)s->io_buffer;
put_le16(p + 0, 0x0040);
- put_le16(p + 1, s->cylinders);
- put_le16(p + 3, s->heads);
- put_le16(p + 4, 512 * s->sectors); /* XXX: retired, remove ? */
+ put_le16(p + 1, s->dev->conf.cyls);
+ put_le16(p + 3, s->dev->conf.heads);
+ put_le16(p + 4, 512 * s->dev->conf.secs); /* XXX: retired, remove ? */
put_le16(p + 5, 512); /* XXX: retired, remove ? */
- put_le16(p + 6, s->sectors);
+ put_le16(p + 6, s->dev->conf.secs);
padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
put_le16(p + 20, 3); /* XXX: retired, remove ? */
put_le16(p + 21, 512); /* cache size in sectors */
@@ -108,10 +108,10 @@ static void ide_identify(IDEState *s)
put_le16(p + 51, 0x200); /* PIO transfer cycle */
put_le16(p + 52, 0x200); /* DMA transfer cycle */
put_le16(p + 53, 1 | (1 << 1) | (1 << 2)); /* words 54-58,64-70,88 are valid */
- put_le16(p + 54, s->cylinders);
- put_le16(p + 55, s->heads);
- put_le16(p + 56, s->sectors);
- oldsize = s->cylinders * s->heads * s->sectors;
+ put_le16(p + 54, s->dev->conf.cyls);
+ put_le16(p + 55, s->dev->conf.heads);
+ put_le16(p + 56, s->dev->conf.secs);
+ oldsize = s->dev->conf.cyls * s->dev->conf.heads * s->dev->conf.secs;
put_le16(p + 57, oldsize);
put_le16(p + 58, oldsize >> 16);
if (s->mult_sectors)
@@ -249,12 +249,12 @@ static void ide_cfata_identify(IDEState *s)
memset(p, 0, sizeof(s->identify_data));
- cur_sec = s->cylinders * s->heads * s->sectors;
+ cur_sec = s->dev->conf.cyls * s->dev->conf.heads * s->dev->conf.secs;
put_le16(p + 0, 0x848a); /* CF Storage Card signature */
- put_le16(p + 1, s->cylinders); /* Default cylinders */
- put_le16(p + 3, s->heads); /* Default heads */
- put_le16(p + 6, s->sectors); /* Default sectors per track */
+ put_le16(p + 1, s->dev->conf.cyls); /* Default cylinders */
+ put_le16(p + 3, s->dev->conf.heads); /* Default heads */
+ put_le16(p + 6, s->dev->conf.secs); /* Default sectors per track */
put_le16(p + 7, s->nb_sectors >> 16); /* Sectors per card */
put_le16(p + 8, s->nb_sectors); /* Sectors per card */
padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
@@ -270,9 +270,9 @@ static void ide_cfata_identify(IDEState *s)
put_le16(p + 51, 0x0002); /* PIO cycle timing mode */
put_le16(p + 52, 0x0001); /* DMA cycle timing mode */
put_le16(p + 53, 0x0003); /* Translation params valid */
- put_le16(p + 54, s->cylinders); /* Current cylinders */
- put_le16(p + 55, s->heads); /* Current heads */
- put_le16(p + 56, s->sectors); /* Current sectors */
+ put_le16(p + 54, s->dev->conf.cyls); /* Current cylinders */
+ put_le16(p + 55, s->dev->conf.heads); /* Current heads */
+ put_le16(p + 56, s->dev->conf.secs); /* Current sectors */
put_le16(p + 57, cur_sec); /* Current capacity */
put_le16(p + 58, cur_sec >> 16); /* Current capacity */
if (s->mult_sectors) /* Multiple sector setting */
@@ -462,8 +462,10 @@ int64_t ide_get_sector(IDEState *s)
((int64_t) s->lcyl << 8) | s->sector;
}
} else {
- sector_num = ((s->hcyl << 8) | s->lcyl) * s->heads * s->sectors +
- (s->select & 0x0f) * s->sectors + (s->sector - 1);
+ sector_num =
+ (((s->hcyl << 8) | s->lcyl) * s->dev->conf.heads
+ + (s->select & 0x0f)) * s->dev->conf.secs
+ + (s->sector - 1);
}
return sector_num;
}
@@ -486,12 +488,12 @@ void ide_set_sector(IDEState *s, int64_t sector_num)
s->hob_hcyl = sector_num >> 40;
}
} else {
- cyl = sector_num / (s->heads * s->sectors);
- r = sector_num % (s->heads * s->sectors);
+ cyl = sector_num / (s->dev->conf.heads * s->dev->conf.secs);
+ r = sector_num % (s->dev->conf.heads * s->dev->conf.secs);
s->hcyl = cyl >> 8;
s->lcyl = cyl;
- s->select = (s->select & 0xf0) | ((r / s->sectors) & 0x0f);
- s->sector = (r % s->sectors) + 1;
+ s->select = (s->select & 0xf0) | ((r / s->dev->conf.secs) & 0x0f);
+ s->sector = (r % s->dev->conf.secs) + 1;
}
}
@@ -2088,9 +2090,7 @@ static const BlockDevOps ide_cd_block_ops = {
int ide_init_drive(IDEState *s, IDEDriveKind kind,
const char *version, const char *serial, const char *model,
- uint64_t wwn,
- uint32_t cylinders, uint32_t heads, uint32_t secs,
- int chs_trans)
+ uint64_t wwn)
{
BlockDriverState *bs = s->dev->conf.bs;
uint64_t nb_sectors;
@@ -2098,10 +2098,6 @@ int ide_init_drive(IDEState *s, IDEDriveKind kind,
s->drive_kind = kind;
bdrv_get_geometry(bs, &nb_sectors);
- s->cylinders = cylinders;
- s->heads = heads;
- s->sectors = secs;
- s->chs_trans = chs_trans;
s->nb_sectors = nb_sectors;
s->wwn = wwn;
/* The SMART values should be preserved across power cycles
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 8f673f8..7a39d44 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -345,7 +345,6 @@ struct IDEState {
uint8_t unit;
/* ide config */
IDEDriveKind drive_kind;
- int cylinders, heads, sectors, chs_trans;
int64_t nb_sectors;
int mult_sectors;
int identify_set;
@@ -547,9 +546,7 @@ uint32_t ide_data_readl(void *opaque, uint32_t addr);
int ide_init_drive(IDEState *s, IDEDriveKind kind,
const char *version, const char *serial, const char *model,
- uint64_t wwn,
- uint32_t cylinders, uint32_t heads, uint32_t secs,
- int chs_trans);
+ uint64_t wwn);
void ide_init2(IDEBus *bus, qemu_irq irq);
void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index f9a4d31..c233d66 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -124,15 +124,15 @@ int ide_get_geometry(BusState *bus, int unit,
return -1;
}
- *cyls = s->cylinders;
- *heads = s->heads;
- *secs = s->sectors;
+ *cyls = s->dev->conf.cyls;
+ *heads = s->dev->conf.heads;
+ *secs = s->dev->conf.secs;
return 0;
}
int ide_get_bios_chs_trans(BusState *bus, int unit)
{
- return DO_UPCAST(IDEBus, qbus, bus)->ifs[unit].chs_trans;
+ return DO_UPCAST(IDEBus, qbus, bus)->ifs[unit].dev->chs_trans;
}
/* --------------------------------- */
@@ -161,9 +161,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
}
if (ide_init_drive(s, kind,
- dev->version, dev->serial, dev->model, dev->wwn,
- dev->conf.cyls, dev->conf.heads, dev->conf.secs,
- dev->chs_trans) < 0) {
+ dev->version, dev->serial, dev->model, dev->wwn) < 0) {
return -1;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH v3 6/9] ide: Drop redundant IDEState member version
2014-01-30 12:16 [Qemu-devel] [PATCH v3 0/9] Clean up IDE after completion of qdevification Markus Armbruster
` (4 preceding siblings ...)
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 5/9] ide: Drop redundant IDEState geometry members Markus Armbruster
@ 2014-01-30 12:16 ` Markus Armbruster
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 7/9] ide: Drop redundant IDEState member drive_serial_str Markus Armbruster
` (2 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2014-01-30 12:16 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, afaerber, stefanha, agraf
It's a copy of dev->version. The copy was needed for non-qdevified
controllers, which lacked dev.
Note that pci_piix3_xen_ide_unplug() did not clear the copy (it only
cleared the copy of bs). Begs the question whether stale data could
have been used after unplug. As far as I can tell, the copy was used
only when the copy of bs was non-null, thus no bug there.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/ide/atapi.c | 2 +-
hw/ide/core.c | 14 ++++----------
hw/ide/internal.h | 3 +--
hw/ide/qdev.c | 9 +++++----
4 files changed, 11 insertions(+), 17 deletions(-)
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 3dc2de0..f20b3a6 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -634,7 +634,7 @@ static void cmd_inquiry(IDEState *s, uint8_t *buf)
buf[7] = 0; /* reserved */
padstr8(buf + 8, 8, "QEMU");
padstr8(buf + 16, 16, "QEMU DVD-ROM");
- padstr8(buf + 32, 4, s->version);
+ padstr8(buf + 32, 4, s->dev->version);
ide_atapi_cmd_reply(s, 36, max_len);
}
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 93ccb6e..564ad71 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -98,7 +98,7 @@ static void ide_identify(IDEState *s)
put_le16(p + 20, 3); /* XXX: retired, remove ? */
put_le16(p + 21, 512); /* cache size in sectors */
put_le16(p + 22, 4); /* ecc bytes */
- padstr((char *)(p + 23), s->version, 8); /* firmware version */
+ padstr((char *)(p + 23), s->dev->version, 8); /* firmware version */
padstr((char *)(p + 27), s->drive_model_str, 40); /* model */
#if MAX_MULT_SECTORS > 1
put_le16(p + 47, 0x8000 | MAX_MULT_SECTORS);
@@ -202,7 +202,7 @@ static void ide_atapi_identify(IDEState *s)
put_le16(p + 20, 3); /* buffer type */
put_le16(p + 21, 512); /* cache size in sectors */
put_le16(p + 22, 4); /* ecc bytes */
- padstr((char *)(p + 23), s->version, 8); /* firmware version */
+ padstr((char *)(p + 23), s->dev->version, 8); /* firmware version */
padstr((char *)(p + 27), s->drive_model_str, 40); /* model */
put_le16(p + 48, 1); /* dword I/O (XXX: should not be set on CDROM) */
#ifdef USE_DMA_CDROM
@@ -259,7 +259,7 @@ static void ide_cfata_identify(IDEState *s)
put_le16(p + 8, s->nb_sectors); /* Sectors per card */
padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
put_le16(p + 22, 0x0004); /* ECC bytes */
- padstr((char *) (p + 23), s->version, 8); /* Firmware Revision */
+ padstr((char *) (p + 23), s->dev->version, 8); /* Firmware Revision */
padstr((char *) (p + 27), s->drive_model_str, 40);/* Model number */
#if MAX_MULT_SECTORS > 1
put_le16(p + 47, 0x8000 | MAX_MULT_SECTORS);
@@ -2089,7 +2089,7 @@ static const BlockDevOps ide_cd_block_ops = {
};
int ide_init_drive(IDEState *s, IDEDriveKind kind,
- const char *version, const char *serial, const char *model,
+ const char *serial, const char *model,
uint64_t wwn)
{
BlockDriverState *bs = s->dev->conf.bs;
@@ -2141,12 +2141,6 @@ int ide_init_drive(IDEState *s, IDEDriveKind kind,
}
}
- if (version) {
- pstrcpy(s->version, sizeof(s->version), version);
- } else {
- pstrcpy(s->version, sizeof(s->version), qemu_get_version());
- }
-
ide_reset(s);
bdrv_iostatus_enable(bs);
return 0;
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 7a39d44..4c0fb8e 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -372,7 +372,6 @@ struct IDEState {
/* set for lba48 access */
uint8_t lba48;
- char version[9];
/* ATAPI specific */
struct unreported_events events;
uint8_t sense_key;
@@ -545,7 +544,7 @@ void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
uint32_t ide_data_readl(void *opaque, uint32_t addr);
int ide_init_drive(IDEState *s, IDEDriveKind kind,
- const char *version, const char *serial, const char *model,
+ const char *serial, const char *model,
uint64_t wwn);
void ide_init2(IDEBus *bus, qemu_irq irq);
void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index c233d66..0326360 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -160,14 +160,15 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
return -1;
}
+ if (!dev->version) {
+ dev->version = g_strdup(qemu_get_version());
+ }
+
if (ide_init_drive(s, kind,
- dev->version, dev->serial, dev->model, dev->wwn) < 0) {
+ dev->serial, dev->model, dev->wwn) < 0) {
return -1;
}
- if (!dev->version) {
- dev->version = g_strdup(s->version);
- }
if (!dev->serial) {
dev->serial = g_strdup(s->drive_serial_str);
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH v3 7/9] ide: Drop redundant IDEState member drive_serial_str
2014-01-30 12:16 [Qemu-devel] [PATCH v3 0/9] Clean up IDE after completion of qdevification Markus Armbruster
` (5 preceding siblings ...)
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 6/9] ide: Drop redundant IDEState member version Markus Armbruster
@ 2014-01-30 12:16 ` Markus Armbruster
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 8/9] ide: Drop redundant IDEState member model Markus Armbruster
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 9/9] ide: Drop redundant IDEState member wwn Markus Armbruster
8 siblings, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2014-01-30 12:16 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, afaerber, stefanha, agraf
It's a copy of dev->serial. The copy was needed for non-qdevified
controllers, which lacked dev.
Note that pci_piix3_xen_ide_unplug() did not clear the copy (it only
cleared the copy of bs). Begs the question whether stale data could
have been used after unplug. As far as I can tell, the copy was used
only when the copy of bs was non-null, thus no bug there.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/ide/core.c | 14 ++++----------
hw/ide/internal.h | 3 +--
hw/ide/qdev.c | 10 +++++-----
3 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 564ad71..c123a14 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -94,7 +94,7 @@ static void ide_identify(IDEState *s)
put_le16(p + 4, 512 * s->dev->conf.secs); /* XXX: retired, remove ? */
put_le16(p + 5, 512); /* XXX: retired, remove ? */
put_le16(p + 6, s->dev->conf.secs);
- padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
+ padstr((char *)(p + 10), s->dev->serial, 20); /* serial number */
put_le16(p + 20, 3); /* XXX: retired, remove ? */
put_le16(p + 21, 512); /* cache size in sectors */
put_le16(p + 22, 4); /* ecc bytes */
@@ -198,7 +198,7 @@ static void ide_atapi_identify(IDEState *s)
p = (uint16_t *)s->io_buffer;
/* Removable CDROM, 50us response, 12 byte packets */
put_le16(p + 0, (2 << 14) | (5 << 8) | (1 << 7) | (2 << 5) | (0 << 0));
- padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
+ padstr((char *)(p + 10), s->dev->serial, 20); /* serial number */
put_le16(p + 20, 3); /* buffer type */
put_le16(p + 21, 512); /* cache size in sectors */
put_le16(p + 22, 4); /* ecc bytes */
@@ -257,7 +257,7 @@ static void ide_cfata_identify(IDEState *s)
put_le16(p + 6, s->dev->conf.secs); /* Default sectors per track */
put_le16(p + 7, s->nb_sectors >> 16); /* Sectors per card */
put_le16(p + 8, s->nb_sectors); /* Sectors per card */
- padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
+ padstr((char *)(p + 10), s->dev->serial, 20); /* serial number */
put_le16(p + 22, 0x0004); /* ECC bytes */
padstr((char *) (p + 23), s->dev->version, 8); /* Firmware Revision */
padstr((char *) (p + 27), s->drive_model_str, 40);/* Model number */
@@ -2089,7 +2089,7 @@ static const BlockDevOps ide_cd_block_ops = {
};
int ide_init_drive(IDEState *s, IDEDriveKind kind,
- const char *serial, const char *model,
+ const char *model,
uint64_t wwn)
{
BlockDriverState *bs = s->dev->conf.bs;
@@ -2119,12 +2119,6 @@ int ide_init_drive(IDEState *s, IDEDriveKind kind,
return -1;
}
}
- if (serial) {
- pstrcpy(s->drive_serial_str, sizeof(s->drive_serial_str), serial);
- } else {
- snprintf(s->drive_serial_str, sizeof(s->drive_serial_str),
- "QM%05d", s->drive_serial);
- }
if (model) {
pstrcpy(s->drive_model_str, sizeof(s->drive_model_str), model);
} else {
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 4c0fb8e..f6b5b7a 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -350,7 +350,6 @@ struct IDEState {
int identify_set;
uint8_t identify_data[512];
int drive_serial;
- char drive_serial_str[21];
char drive_model_str[41];
uint64_t wwn;
/* ide regs */
@@ -544,7 +543,7 @@ void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
uint32_t ide_data_readl(void *opaque, uint32_t addr);
int ide_init_drive(IDEState *s, IDEDriveKind kind,
- const char *serial, const char *model,
+ const char *model,
uint64_t wwn);
void ide_init2(IDEBus *bus, qemu_irq irq);
void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 0326360..c1a4cb6 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -160,19 +160,19 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
return -1;
}
+ if (!dev->serial) {
+ dev->serial = g_strdup_printf("QM%05d", s->drive_serial);
+ }
+
if (!dev->version) {
dev->version = g_strdup(qemu_get_version());
}
if (ide_init_drive(s, kind,
- dev->serial, dev->model, dev->wwn) < 0) {
+ dev->model, dev->wwn) < 0) {
return -1;
}
- if (!dev->serial) {
- dev->serial = g_strdup(s->drive_serial_str);
- }
-
add_boot_device_path(dev->conf.bootindex, &dev->qdev,
dev->unit ? "/disk@1" : "/disk@0");
--
1.8.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH v3 8/9] ide: Drop redundant IDEState member model
2014-01-30 12:16 [Qemu-devel] [PATCH v3 0/9] Clean up IDE after completion of qdevification Markus Armbruster
` (6 preceding siblings ...)
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 7/9] ide: Drop redundant IDEState member drive_serial_str Markus Armbruster
@ 2014-01-30 12:16 ` Markus Armbruster
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 9/9] ide: Drop redundant IDEState member wwn Markus Armbruster
8 siblings, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2014-01-30 12:16 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, afaerber, stefanha, agraf
It's a copy of dev->serial. The copy was needed for non-qdevified
controllers, which lacked dev.
Note that pci_piix3_xen_ide_unplug() did not clear the copy (it only
cleared the copy of bs). Begs the question whether stale data could
have been used after unplug. As far as I can tell, the copy was used
only when the copy of bs was non-null, thus no bug there.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/ide/core.c | 22 +++-------------------
hw/ide/internal.h | 2 --
hw/ide/qdev.c | 16 +++++++++++++++-
3 files changed, 18 insertions(+), 22 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index c123a14..1c4522a 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -99,7 +99,7 @@ static void ide_identify(IDEState *s)
put_le16(p + 21, 512); /* cache size in sectors */
put_le16(p + 22, 4); /* ecc bytes */
padstr((char *)(p + 23), s->dev->version, 8); /* firmware version */
- padstr((char *)(p + 27), s->drive_model_str, 40); /* model */
+ padstr((char *)(p + 27), s->dev->model, 40); /* model */
#if MAX_MULT_SECTORS > 1
put_le16(p + 47, 0x8000 | MAX_MULT_SECTORS);
#endif
@@ -203,7 +203,7 @@ static void ide_atapi_identify(IDEState *s)
put_le16(p + 21, 512); /* cache size in sectors */
put_le16(p + 22, 4); /* ecc bytes */
padstr((char *)(p + 23), s->dev->version, 8); /* firmware version */
- padstr((char *)(p + 27), s->drive_model_str, 40); /* model */
+ padstr((char *)(p + 27), s->dev->model, 40); /* model */
put_le16(p + 48, 1); /* dword I/O (XXX: should not be set on CDROM) */
#ifdef USE_DMA_CDROM
put_le16(p + 49, 1 << 9 | 1 << 8); /* DMA and LBA supported */
@@ -260,7 +260,7 @@ static void ide_cfata_identify(IDEState *s)
padstr((char *)(p + 10), s->dev->serial, 20); /* serial number */
put_le16(p + 22, 0x0004); /* ECC bytes */
padstr((char *) (p + 23), s->dev->version, 8); /* Firmware Revision */
- padstr((char *) (p + 27), s->drive_model_str, 40);/* Model number */
+ padstr((char *) (p + 27), s->dev->model, 40); /* Model number */
#if MAX_MULT_SECTORS > 1
put_le16(p + 47, 0x8000 | MAX_MULT_SECTORS);
#else
@@ -2089,7 +2089,6 @@ static const BlockDevOps ide_cd_block_ops = {
};
int ide_init_drive(IDEState *s, IDEDriveKind kind,
- const char *model,
uint64_t wwn)
{
BlockDriverState *bs = s->dev->conf.bs;
@@ -2119,21 +2118,6 @@ int ide_init_drive(IDEState *s, IDEDriveKind kind,
return -1;
}
}
- if (model) {
- pstrcpy(s->drive_model_str, sizeof(s->drive_model_str), model);
- } else {
- switch (kind) {
- case IDE_CD:
- strcpy(s->drive_model_str, "QEMU DVD-ROM");
- break;
- case IDE_CFATA:
- strcpy(s->drive_model_str, "QEMU MICRODRIVE");
- break;
- default:
- strcpy(s->drive_model_str, "QEMU HARDDISK");
- break;
- }
- }
ide_reset(s);
bdrv_iostatus_enable(bs);
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index f6b5b7a..c4a5773 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -350,7 +350,6 @@ struct IDEState {
int identify_set;
uint8_t identify_data[512];
int drive_serial;
- char drive_model_str[41];
uint64_t wwn;
/* ide regs */
uint8_t feature;
@@ -543,7 +542,6 @@ void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
uint32_t ide_data_readl(void *opaque, uint32_t addr);
int ide_init_drive(IDEState *s, IDEDriveKind kind,
- const char *model,
uint64_t wwn);
void ide_init2(IDEBus *bus, qemu_irq irq);
void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index c1a4cb6..915bc27 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -168,8 +168,22 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
dev->version = g_strdup(qemu_get_version());
}
+ if (!dev->model) {
+ switch (kind) {
+ case IDE_CD:
+ dev->model = g_strdup("QEMU DVD-ROM");
+ break;
+ case IDE_CFATA:
+ dev->model = g_strdup("QEMU MICRODRIVE");
+ break;
+ default:
+ dev->model = g_strdup("QEMU HARDDISK");
+ break;
+ }
+ }
+
if (ide_init_drive(s, kind,
- dev->model, dev->wwn) < 0) {
+ dev->wwn) < 0) {
return -1;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH v3 9/9] ide: Drop redundant IDEState member wwn
2014-01-30 12:16 [Qemu-devel] [PATCH v3 0/9] Clean up IDE after completion of qdevification Markus Armbruster
` (7 preceding siblings ...)
2014-01-30 12:16 ` [Qemu-devel] [PATCH v3 8/9] ide: Drop redundant IDEState member model Markus Armbruster
@ 2014-01-30 12:16 ` Markus Armbruster
8 siblings, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2014-01-30 12:16 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, afaerber, stefanha, agraf
It's a copy of dev->wwn. The copy was needed for non-qdevified
controllers, which lacked dev.
Note that pci_piix3_xen_ide_unplug() did not clear the copy (it only
cleared the copy of bs). Begs the question whether stale data could
have been used after unplug. As far as I can tell, the copy was used
only when the copy of bs was non-null, thus no bug there.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/ide/core.c | 18 ++++++++----------
hw/ide/internal.h | 4 +---
hw/ide/qdev.c | 3 +--
3 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 1c4522a..98b6611 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -142,7 +142,7 @@ static void ide_identify(IDEState *s)
/* 13=flush_cache_ext,12=flush_cache,10=lba48 */
put_le16(p + 83, (1 << 14) | (1 << 13) | (1 <<12) | (1 << 10));
/* 14=set to 1, 8=has WWN, 1=SMART self test, 0=SMART error logging */
- if (s->wwn) {
+ if (s->dev->wwn) {
put_le16(p + 84, (1 << 14) | (1 << 8) | 0);
} else {
put_le16(p + 84, (1 << 14) | 0);
@@ -156,7 +156,7 @@ static void ide_identify(IDEState *s)
/* 13=flush_cache_ext,12=flush_cache,10=lba48 */
put_le16(p + 86, (1 << 13) | (1 <<12) | (1 << 10));
/* 14=set to 1, 8=has WWN, 1=SMART self test, 0=SMART error logging */
- if (s->wwn) {
+ if (s->dev->wwn) {
put_le16(p + 87, (1 << 14) | (1 << 8) | 0);
} else {
put_le16(p + 87, (1 << 14) | 0);
@@ -170,12 +170,12 @@ static void ide_identify(IDEState *s)
if (dev && dev->conf.physical_block_size)
put_le16(p + 106, 0x6000 | get_physical_block_exp(&dev->conf));
- if (s->wwn) {
+ if (s->dev->wwn) {
/* LE 16-bit words 111-108 contain 64-bit World Wide Name */
- put_le16(p + 108, s->wwn >> 48);
- put_le16(p + 109, s->wwn >> 32);
- put_le16(p + 110, s->wwn >> 16);
- put_le16(p + 111, s->wwn);
+ put_le16(p + 108, s->dev->wwn >> 48);
+ put_le16(p + 109, s->dev->wwn >> 32);
+ put_le16(p + 110, s->dev->wwn >> 16);
+ put_le16(p + 111, s->dev->wwn);
}
if (dev && dev->conf.discard_granularity) {
put_le16(p + 169, 1); /* TRIM support */
@@ -2088,8 +2088,7 @@ static const BlockDevOps ide_cd_block_ops = {
.is_medium_locked = ide_cd_is_medium_locked,
};
-int ide_init_drive(IDEState *s, IDEDriveKind kind,
- uint64_t wwn)
+int ide_init_drive(IDEState *s, IDEDriveKind kind)
{
BlockDriverState *bs = s->dev->conf.bs;
uint64_t nb_sectors;
@@ -2098,7 +2097,6 @@ int ide_init_drive(IDEState *s, IDEDriveKind kind,
bdrv_get_geometry(bs, &nb_sectors);
s->nb_sectors = nb_sectors;
- s->wwn = wwn;
/* The SMART values should be preserved across power cycles
but they aren't. */
s->smart_enabled = 1;
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index c4a5773..95ea75c 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -350,7 +350,6 @@ struct IDEState {
int identify_set;
uint8_t identify_data[512];
int drive_serial;
- uint64_t wwn;
/* ide regs */
uint8_t feature;
uint8_t error;
@@ -541,8 +540,7 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr);
void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
uint32_t ide_data_readl(void *opaque, uint32_t addr);
-int ide_init_drive(IDEState *s, IDEDriveKind kind,
- uint64_t wwn);
+int ide_init_drive(IDEState *s, IDEDriveKind kind);
void ide_init2(IDEBus *bus, qemu_irq irq);
void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 915bc27..17404b8 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -182,8 +182,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
}
}
- if (ide_init_drive(s, kind,
- dev->wwn) < 0) {
+ if (ide_init_drive(s, kind) < 0) {
return -1;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 18+ messages in thread