* [Qemu-devel] [PATCHv4 01/15] Introduce fw_name field to DeviceInfo structure.
2010-11-14 15:39 [Qemu-devel] [PATCHv4 00/15] boot order specification Gleb Natapov
@ 2010-11-14 15:39 ` Gleb Natapov
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 02/15] Introduce new BusInfo callback get_fw_dev_path Gleb Natapov
` (13 subsequent siblings)
14 siblings, 0 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-14 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kvm, mst, armbru, blauwirbel, alex.williamson, kevin
Add "fw_name" to DeviceInfo to use in device path building. In
contrast to "name" "fw_name" should refer to functionality device
provides instead of particular device model like "name" does.
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
hw/fdc.c | 1 +
hw/ide/isa.c | 1 +
hw/ide/qdev.c | 1 +
hw/isa-bus.c | 1 +
hw/lance.c | 1 +
hw/piix_pci.c | 1 +
hw/qdev.h | 6 ++++++
hw/usb-hub.c | 1 +
hw/usb-net.c | 1 +
hw/virtio-pci.c | 1 +
10 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/hw/fdc.c b/hw/fdc.c
index c159dcb..a467c4b 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -2040,6 +2040,7 @@ static const VMStateDescription vmstate_isa_fdc ={
static ISADeviceInfo isa_fdc_info = {
.init = isabus_fdc_init1,
.qdev.name = "isa-fdc",
+ .qdev.fw_name = "fdc",
.qdev.size = sizeof(FDCtrlISABus),
.qdev.no_user = 1,
.qdev.vmsd = &vmstate_isa_fdc,
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index 6b57e0d..9856435 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -98,6 +98,7 @@ ISADevice *isa_ide_init(int iobase, int iobase2, int isairq,
static ISADeviceInfo isa_ide_info = {
.qdev.name = "isa-ide",
+ .qdev.fw_name = "ide",
.qdev.size = sizeof(ISAIDEState),
.init = isa_ide_initfn,
.qdev.reset = isa_ide_reset,
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 0808760..6d27b60 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -134,6 +134,7 @@ static int ide_drive_initfn(IDEDevice *dev)
static IDEDeviceInfo ide_drive_info = {
.qdev.name = "ide-drive",
+ .qdev.fw_name = "drive",
.qdev.size = sizeof(IDEDrive),
.init = ide_drive_initfn,
.qdev.props = (Property[]) {
diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index 4e306de..26036e0 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -153,6 +153,7 @@ static int isabus_bridge_init(SysBusDevice *dev)
static SysBusDeviceInfo isabus_bridge_info = {
.init = isabus_bridge_init,
.qdev.name = "isabus-bridge",
+ .qdev.fw_name = "isa",
.qdev.size = sizeof(SysBusDevice),
.qdev.no_user = 1,
};
diff --git a/hw/lance.c b/hw/lance.c
index dc12144..1a3bb1a 100644
--- a/hw/lance.c
+++ b/hw/lance.c
@@ -141,6 +141,7 @@ static void lance_reset(DeviceState *dev)
static SysBusDeviceInfo lance_info = {
.init = lance_init,
.qdev.name = "lance",
+ .qdev.fw_name = "ethernet",
.qdev.size = sizeof(SysBusPCNetState),
.qdev.reset = lance_reset,
.qdev.vmsd = &vmstate_lance,
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index b5589b9..38f9d9e 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -365,6 +365,7 @@ static PCIDeviceInfo i440fx_info[] = {
static SysBusDeviceInfo i440fx_pcihost_info = {
.init = i440fx_pcihost_initfn,
.qdev.name = "i440FX-pcihost",
+ .qdev.fw_name = "pci",
.qdev.size = sizeof(I440FXState),
.qdev.no_user = 1,
};
diff --git a/hw/qdev.h b/hw/qdev.h
index 579328a..9f90efe 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -139,6 +139,7 @@ typedef void (*qdev_resetfn)(DeviceState *dev);
struct DeviceInfo {
const char *name;
+ const char *fw_name;
const char *alias;
const char *desc;
size_t size;
@@ -288,6 +289,11 @@ void qdev_prop_set_defaults(DeviceState *dev, Property *props);
void qdev_prop_register_global_list(GlobalProperty *props);
void qdev_prop_set_globals(DeviceState *dev);
+static inline const char *qdev_fw_name(DeviceState *dev)
+{
+ return dev->info->fw_name ? : dev->info->alias ? : dev->info->name;
+}
+
/* This is a nasty hack to allow passing a NULL bus to qdev_create. */
extern struct BusInfo system_bus_info;
diff --git a/hw/usb-hub.c b/hw/usb-hub.c
index 2a1edfc..8e3a96b 100644
--- a/hw/usb-hub.c
+++ b/hw/usb-hub.c
@@ -545,6 +545,7 @@ static int usb_hub_initfn(USBDevice *dev)
static struct USBDeviceInfo hub_info = {
.product_desc = "QEMU USB Hub",
.qdev.name = "usb-hub",
+ .qdev.fw_name = "hub",
.qdev.size = sizeof(USBHubState),
.init = usb_hub_initfn,
.handle_packet = usb_hub_handle_packet,
diff --git a/hw/usb-net.c b/hw/usb-net.c
index 70f9263..2287ee1 100644
--- a/hw/usb-net.c
+++ b/hw/usb-net.c
@@ -1496,6 +1496,7 @@ static USBDevice *usb_net_init(const char *cmdline)
static struct USBDeviceInfo net_info = {
.product_desc = "QEMU USB Network Interface",
.qdev.name = "usb-net",
+ .qdev.fw_name = "network",
.qdev.size = sizeof(USBNetState),
.init = usb_net_initfn,
.handle_packet = usb_generic_handle_packet,
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 729917d..be2c92f 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -697,6 +697,7 @@ static int virtio_9p_init_pci(PCIDevice *pci_dev)
static PCIDeviceInfo virtio_info[] = {
{
.qdev.name = "virtio-blk-pci",
+ .qdev.alias = "virtio-blk",
.qdev.size = sizeof(VirtIOPCIProxy),
.init = virtio_blk_init_pci,
.exit = virtio_blk_exit_pci,
--
1.7.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCHv4 02/15] Introduce new BusInfo callback get_fw_dev_path.
2010-11-14 15:39 [Qemu-devel] [PATCHv4 00/15] boot order specification Gleb Natapov
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 01/15] Introduce fw_name field to DeviceInfo structure Gleb Natapov
@ 2010-11-14 15:39 ` Gleb Natapov
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 03/15] Keep track of ISA ports ISA device is using in qdev Gleb Natapov
` (12 subsequent siblings)
14 siblings, 0 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-14 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kvm, mst, armbru, blauwirbel, alex.williamson, kevin
New get_fw_dev_path callback will be used for build device path usable
by firmware in contrast to qdev qemu internal device path.
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
hw/qdev.h | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/hw/qdev.h b/hw/qdev.h
index 9f90efe..dc669b3 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -49,12 +49,14 @@ struct DeviceState {
typedef void (*bus_dev_printfn)(Monitor *mon, DeviceState *dev, int indent);
typedef char *(*bus_get_dev_path)(DeviceState *dev);
+typedef char *(*bus_get_fw_dev_path)(DeviceState *dev);
struct BusInfo {
const char *name;
size_t size;
bus_dev_printfn print_dev;
bus_get_dev_path get_dev_path;
+ bus_get_fw_dev_path get_fw_dev_path;
Property *props;
};
--
1.7.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCHv4 03/15] Keep track of ISA ports ISA device is using in qdev.
2010-11-14 15:39 [Qemu-devel] [PATCHv4 00/15] boot order specification Gleb Natapov
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 01/15] Introduce fw_name field to DeviceInfo structure Gleb Natapov
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 02/15] Introduce new BusInfo callback get_fw_dev_path Gleb Natapov
@ 2010-11-14 15:39 ` Gleb Natapov
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 04/15] Add get_fw_dev_path callback to ISA bus " Gleb Natapov
` (11 subsequent siblings)
14 siblings, 0 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-14 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kvm, mst, armbru, blauwirbel, alex.williamson, kevin
Store all io ports used by device in ISADevice structure.
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
hw/cs4231a.c | 1 +
hw/fdc.c | 3 +++
hw/gus.c | 4 ++++
hw/ide/isa.c | 2 ++
hw/isa-bus.c | 25 +++++++++++++++++++++++++
hw/isa.h | 4 ++++
hw/m48t59.c | 1 +
hw/mc146818rtc.c | 1 +
hw/ne2000-isa.c | 3 +++
hw/parallel.c | 5 +++++
hw/pckbd.c | 3 +++
hw/sb16.c | 4 ++++
hw/serial.c | 1 +
13 files changed, 57 insertions(+), 0 deletions(-)
diff --git a/hw/cs4231a.c b/hw/cs4231a.c
index 4d5ce5c..598f032 100644
--- a/hw/cs4231a.c
+++ b/hw/cs4231a.c
@@ -645,6 +645,7 @@ static int cs4231a_initfn (ISADevice *dev)
isa_init_irq (dev, &s->pic, s->irq);
for (i = 0; i < 4; i++) {
+ isa_init_ioport(dev, i);
register_ioport_write (s->port + i, 1, 1, cs_write, s);
register_ioport_read (s->port + i, 1, 1, cs_read, s);
}
diff --git a/hw/fdc.c b/hw/fdc.c
index a467c4b..5ab754b 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -1983,6 +1983,9 @@ static int isabus_fdc_init1(ISADevice *dev)
&fdctrl_write_port, fdctrl);
register_ioport_write(iobase + 0x07, 1, 1,
&fdctrl_write_port, fdctrl);
+ isa_init_ioport_range(dev, iobase + 1, 5);
+ isa_init_ioport(dev, iobase + 7);
+
isa_init_irq(&isa->busdev, &fdctrl->irq, isairq);
fdctrl->dma_chann = dma_chann;
diff --git a/hw/gus.c b/hw/gus.c
index e9016d8..ff9e7c7 100644
--- a/hw/gus.c
+++ b/hw/gus.c
@@ -264,20 +264,24 @@ static int gus_initfn (ISADevice *dev)
register_ioport_write (s->port, 1, 1, gus_writeb, s);
register_ioport_write (s->port, 1, 2, gus_writew, s);
+ isa_init_ioport_range(dev, s->port, 2);
register_ioport_read ((s->port + 0x100) & 0xf00, 1, 1, gus_readb, s);
register_ioport_read ((s->port + 0x100) & 0xf00, 1, 2, gus_readw, s);
+ isa_init_ioport_range(dev, (s->port + 0x100) & 0xf00, 2);
register_ioport_write (s->port + 6, 10, 1, gus_writeb, s);
register_ioport_write (s->port + 6, 10, 2, gus_writew, s);
register_ioport_read (s->port + 6, 10, 1, gus_readb, s);
register_ioport_read (s->port + 6, 10, 2, gus_readw, s);
+ isa_init_ioport_range(dev, s->port + 6, 10);
register_ioport_write (s->port + 0x100, 8, 1, gus_writeb, s);
register_ioport_write (s->port + 0x100, 8, 2, gus_writew, s);
register_ioport_read (s->port + 0x100, 8, 1, gus_readb, s);
register_ioport_read (s->port + 0x100, 8, 2, gus_readw, s);
+ isa_init_ioport_range(dev, s->port + 0x100, 8);
DMA_register_channel (s->emu.gusdma, GUS_read_DMA, s);
s->emu.himemaddr = s->himem;
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index 9856435..4206afd 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -70,6 +70,8 @@ static int isa_ide_initfn(ISADevice *dev)
ide_bus_new(&s->bus, &s->dev.qdev);
ide_init_ioport(&s->bus, s->iobase, s->iobase2);
isa_init_irq(dev, &s->irq, s->isairq);
+ isa_init_ioport_range(dev, s->iobase, 8);
+ isa_init_ioport(dev, s->iobase2);
ide_init2(&s->bus, s->irq);
vmstate_register(&dev->qdev, 0, &vmstate_ide_isa, s);
return 0;
diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index 26036e0..c0ac7e9 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -92,6 +92,31 @@ void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq)
dev->nirqs++;
}
+static void isa_init_ioport_one(ISADevice *dev, uint16_t ioport)
+{
+ assert(dev->nioports < ARRAY_SIZE(dev->ioports));
+ dev->ioports[dev->nioports++] = ioport;
+}
+
+static int isa_cmp_ports(const void *p1, const void *p2)
+{
+ return *(uint16_t*)p1 - *(uint16_t*)p2;
+}
+
+void isa_init_ioport_range(ISADevice *dev, uint16_t start, uint16_t length)
+{
+ int i;
+ for (i = start; i < start + length; i++) {
+ isa_init_ioport_one(dev, i);
+ }
+ qsort(dev->ioports, dev->nioports, sizeof(dev->ioports[0]), isa_cmp_ports);
+}
+
+void isa_init_ioport(ISADevice *dev, uint16_t ioport)
+{
+ isa_init_ioport_range(dev, ioport, 1);
+}
+
static int isa_qdev_init(DeviceState *qdev, DeviceInfo *base)
{
ISADevice *dev = DO_UPCAST(ISADevice, qdev, qdev);
diff --git a/hw/isa.h b/hw/isa.h
index aaf0272..4794b76 100644
--- a/hw/isa.h
+++ b/hw/isa.h
@@ -14,6 +14,8 @@ struct ISADevice {
DeviceState qdev;
uint32_t isairq[2];
int nirqs;
+ uint16_t ioports[32];
+ int nioports;
};
typedef int (*isa_qdev_initfn)(ISADevice *dev);
@@ -26,6 +28,8 @@ ISABus *isa_bus_new(DeviceState *dev);
void isa_bus_irqs(qemu_irq *irqs);
qemu_irq isa_reserve_irq(int isairq);
void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq);
+void isa_init_ioport(ISADevice *dev, uint16_t ioport);
+void isa_init_ioport_range(ISADevice *dev, uint16_t start, uint16_t length);
void isa_qdev_register(ISADeviceInfo *info);
ISADevice *isa_create(const char *name);
ISADevice *isa_create_simple(const char *name);
diff --git a/hw/m48t59.c b/hw/m48t59.c
index c7492a6..75a94e1 100644
--- a/hw/m48t59.c
+++ b/hw/m48t59.c
@@ -680,6 +680,7 @@ M48t59State *m48t59_init_isa(uint32_t io_base, uint16_t size, int type)
if (io_base != 0) {
register_ioport_read(io_base, 0x04, 1, NVRAM_readb, s);
register_ioport_write(io_base, 0x04, 1, NVRAM_writeb, s);
+ isa_init_ioport_range(dev, io_base, 4);
}
return s;
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 2b91fa8..6466aff 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -605,6 +605,7 @@ static int rtc_initfn(ISADevice *dev)
register_ioport_write(base, 2, 1, cmos_ioport_write, s);
register_ioport_read(base, 2, 1, cmos_ioport_read, s);
+ isa_init_ioport_range(dev, base, 2);
qdev_set_legacy_instance_id(&dev->qdev, base, 2);
qemu_register_reset(rtc_reset, s);
diff --git a/hw/ne2000-isa.c b/hw/ne2000-isa.c
index 03a5a1f..3ff0d89 100644
--- a/hw/ne2000-isa.c
+++ b/hw/ne2000-isa.c
@@ -68,14 +68,17 @@ static int isa_ne2000_initfn(ISADevice *dev)
register_ioport_write(isa->iobase, 16, 1, ne2000_ioport_write, s);
register_ioport_read(isa->iobase, 16, 1, ne2000_ioport_read, s);
+ isa_init_ioport_range(dev, isa->iobase, 16);
register_ioport_write(isa->iobase + 0x10, 1, 1, ne2000_asic_ioport_write, s);
register_ioport_read(isa->iobase + 0x10, 1, 1, ne2000_asic_ioport_read, s);
register_ioport_write(isa->iobase + 0x10, 2, 2, ne2000_asic_ioport_write, s);
register_ioport_read(isa->iobase + 0x10, 2, 2, ne2000_asic_ioport_read, s);
+ isa_init_ioport_range(dev, isa->iobase + 0x10, 2);
register_ioport_write(isa->iobase + 0x1f, 1, 1, ne2000_reset_ioport_write, s);
register_ioport_read(isa->iobase + 0x1f, 1, 1, ne2000_reset_ioport_read, s);
+ isa_init_ioport(dev, isa->iobase + 0x1f);
isa_init_irq(dev, &s->irq, isa->isairq);
diff --git a/hw/parallel.c b/hw/parallel.c
index 6b11672..6270b53 100644
--- a/hw/parallel.c
+++ b/hw/parallel.c
@@ -481,16 +481,21 @@ static int parallel_isa_initfn(ISADevice *dev)
if (s->hw_driver) {
register_ioport_write(base, 8, 1, parallel_ioport_write_hw, s);
register_ioport_read(base, 8, 1, parallel_ioport_read_hw, s);
+ isa_init_ioport_range(dev, base, 8);
+
register_ioport_write(base+4, 1, 2, parallel_ioport_eppdata_write_hw2, s);
register_ioport_read(base+4, 1, 2, parallel_ioport_eppdata_read_hw2, s);
register_ioport_write(base+4, 1, 4, parallel_ioport_eppdata_write_hw4, s);
register_ioport_read(base+4, 1, 4, parallel_ioport_eppdata_read_hw4, s);
+ isa_init_ioport(dev, base+4);
register_ioport_write(base+0x400, 8, 1, parallel_ioport_ecp_write, s);
register_ioport_read(base+0x400, 8, 1, parallel_ioport_ecp_read, s);
+ isa_init_ioport_range(dev, base+0x400, 8);
}
else {
register_ioport_write(base, 8, 1, parallel_ioport_write_sw, s);
register_ioport_read(base, 8, 1, parallel_ioport_read_sw, s);
+ isa_init_ioport_range(dev, base, 8);
}
return 0;
}
diff --git a/hw/pckbd.c b/hw/pckbd.c
index 6e4e406..e736505 100644
--- a/hw/pckbd.c
+++ b/hw/pckbd.c
@@ -484,10 +484,13 @@ static int i8042_initfn(ISADevice *dev)
register_ioport_read(0x60, 1, 1, kbd_read_data, s);
register_ioport_write(0x60, 1, 1, kbd_write_data, s);
+ isa_init_ioport(dev, 0x60);
register_ioport_read(0x64, 1, 1, kbd_read_status, s);
register_ioport_write(0x64, 1, 1, kbd_write_command, s);
+ isa_init_ioport(dev, 0x64);
register_ioport_read(0x92, 1, 1, ioport92_read, s);
register_ioport_write(0x92, 1, 1, ioport92_write, s);
+ isa_init_ioport(dev, 0x92);
s->kbd = ps2_kbd_init(kbd_update_kbd_irq, s);
s->mouse = ps2_mouse_init(kbd_update_aux_irq, s);
diff --git a/hw/sb16.c b/hw/sb16.c
index 78590a7..c9d37ad 100644
--- a/hw/sb16.c
+++ b/hw/sb16.c
@@ -1368,16 +1368,20 @@ static int sb16_initfn (ISADevice *dev)
for (i = 0; i < ARRAY_SIZE (dsp_write_ports); i++) {
register_ioport_write (s->port + dsp_write_ports[i], 1, 1, dsp_write, s);
+ isa_init_ioport(dev, s->port + dsp_write_ports[i]);
}
for (i = 0; i < ARRAY_SIZE (dsp_read_ports); i++) {
register_ioport_read (s->port + dsp_read_ports[i], 1, 1, dsp_read, s);
+ isa_init_ioport(dev, s->port + dsp_read_ports[i]);
}
register_ioport_write (s->port + 0x4, 1, 1, mixer_write_indexb, s);
register_ioport_write (s->port + 0x4, 1, 2, mixer_write_indexw, s);
+ isa_init_ioport(dev, s->port + 0x4);
register_ioport_read (s->port + 0x5, 1, 1, mixer_read, s);
register_ioport_write (s->port + 0x5, 1, 1, mixer_write_datab, s);
+ isa_init_ioport(dev, s->port + 0x5);
DMA_register_channel (s->hdma, SB_read_DMA, s);
DMA_register_channel (s->dma, SB_read_DMA, s);
diff --git a/hw/serial.c b/hw/serial.c
index 9ebc452..20902ae 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -778,6 +778,7 @@ static int serial_isa_initfn(ISADevice *dev)
register_ioport_write(isa->iobase, 8, 1, serial_ioport_write, s);
register_ioport_read(isa->iobase, 8, 1, serial_ioport_read, s);
+ isa_init_ioport_range(dev, isa->iobase, 8);
return 0;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCHv4 04/15] Add get_fw_dev_path callback to ISA bus in qdev.
2010-11-14 15:39 [Qemu-devel] [PATCHv4 00/15] boot order specification Gleb Natapov
` (2 preceding siblings ...)
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 03/15] Keep track of ISA ports ISA device is using in qdev Gleb Natapov
@ 2010-11-14 15:39 ` Gleb Natapov
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 05/15] Store IDE bus id in IDEBus structure for easy access Gleb Natapov
` (10 subsequent siblings)
14 siblings, 0 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-14 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kvm, mst, armbru, blauwirbel, alex.williamson, kevin
Use device ioports to create unique device path.
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
hw/isa-bus.c | 16 ++++++++++++++++
1 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index c0ac7e9..c423c1b 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -31,11 +31,13 @@ static ISABus *isabus;
target_phys_addr_t isa_mem_base = 0;
static void isabus_dev_print(Monitor *mon, DeviceState *dev, int indent);
+static char *isabus_get_fw_dev_path(DeviceState *dev);
static struct BusInfo isa_bus_info = {
.name = "ISA",
.size = sizeof(ISABus),
.print_dev = isabus_dev_print,
+ .get_fw_dev_path = isabus_get_fw_dev_path,
};
ISABus *isa_bus_new(DeviceState *dev)
@@ -188,4 +190,18 @@ static void isabus_register_devices(void)
sysbus_register_withprop(&isabus_bridge_info);
}
+static char *isabus_get_fw_dev_path(DeviceState *dev)
+{
+ ISADevice *d = (ISADevice*)dev;
+ char path[40];
+ int off;
+
+ off = snprintf(path, sizeof(path), "%s", qdev_fw_name(dev));
+ if (d->nioports) {
+ snprintf(path + off, sizeof(path) - off, "@%04x", d->ioports[0]);
+ }
+
+ return strdup(path);
+}
+
device_init(isabus_register_devices)
--
1.7.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCHv4 05/15] Store IDE bus id in IDEBus structure for easy access.
2010-11-14 15:39 [Qemu-devel] [PATCHv4 00/15] boot order specification Gleb Natapov
` (3 preceding siblings ...)
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 04/15] Add get_fw_dev_path callback to ISA bus " Gleb Natapov
@ 2010-11-14 15:39 ` Gleb Natapov
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 06/15] Add get_fw_dev_path callback to IDE bus Gleb Natapov
` (9 subsequent siblings)
14 siblings, 0 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-14 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kvm, mst, armbru, blauwirbel, alex.williamson, kevin
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
hw/ide/cmd646.c | 4 ++--
hw/ide/internal.h | 3 ++-
hw/ide/isa.c | 2 +-
hw/ide/piix.c | 4 ++--
hw/ide/qdev.c | 3 ++-
hw/ide/via.c | 4 ++--
6 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index ff80dd5..b2cbdbc 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -257,8 +257,8 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1
irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
- ide_bus_new(&d->bus[0], &d->dev.qdev);
- ide_bus_new(&d->bus[1], &d->dev.qdev);
+ ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
+ ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
ide_init2(&d->bus[0], irq[0]);
ide_init2(&d->bus[1], irq[1]);
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index d652e06..c0a1abc 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -448,6 +448,7 @@ struct IDEBus {
IDEDevice *slave;
BMDMAState *bmdma;
IDEState ifs[2];
+ int bus_id;
uint8_t unit;
uint8_t cmd;
qemu_irq irq;
@@ -565,7 +566,7 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
void ide_init_ioport(IDEBus *bus, int iobase, int iobase2);
/* hw/ide/qdev.c */
-void ide_bus_new(IDEBus *idebus, DeviceState *dev);
+void ide_bus_new(IDEBus *idebus, DeviceState *dev, int bus_id);
IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
#endif /* HW_IDE_INTERNAL_H */
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index 4206afd..8c59c5a 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -67,7 +67,7 @@ static int isa_ide_initfn(ISADevice *dev)
{
ISAIDEState *s = DO_UPCAST(ISAIDEState, dev, dev);
- ide_bus_new(&s->bus, &s->dev.qdev);
+ ide_bus_new(&s->bus, &s->dev.qdev, 0);
ide_init_ioport(&s->bus, s->iobase, s->iobase2);
isa_init_irq(dev, &s->irq, s->isairq);
isa_init_ioport_range(dev, s->iobase, 8);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 07483e8..d0b04a3 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -129,8 +129,8 @@ static int pci_piix_ide_initfn(PCIIDEState *d)
vmstate_register(&d->dev.qdev, 0, &vmstate_ide_pci, d);
- ide_bus_new(&d->bus[0], &d->dev.qdev);
- ide_bus_new(&d->bus[1], &d->dev.qdev);
+ ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
+ ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
ide_init_ioport(&d->bus[1], 0x170, 0x376);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 6d27b60..88ff657 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -29,9 +29,10 @@ static struct BusInfo ide_bus_info = {
.size = sizeof(IDEBus),
};
-void ide_bus_new(IDEBus *idebus, DeviceState *dev)
+void ide_bus_new(IDEBus *idebus, DeviceState *dev, int bus_id)
{
qbus_create_inplace(&idebus->qbus, &ide_bus_info, dev, NULL);
+ idebus->bus_id = bus_id;
}
static int ide_qdev_init(DeviceState *qdev, DeviceInfo *base)
diff --git a/hw/ide/via.c b/hw/ide/via.c
index b2c7cad..cc48b2b 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -158,8 +158,8 @@ static int vt82c686b_ide_initfn(PCIDevice *dev)
vmstate_register(&dev->qdev, 0, &vmstate_ide_pci, d);
- ide_bus_new(&d->bus[0], &d->dev.qdev);
- ide_bus_new(&d->bus[1], &d->dev.qdev);
+ ide_bus_new(&d->bus[0], &d->dev.qdev, 0);
+ ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
ide_init2(&d->bus[0], isa_reserve_irq(14));
ide_init2(&d->bus[1], isa_reserve_irq(15));
ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
--
1.7.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCHv4 06/15] Add get_fw_dev_path callback to IDE bus.
2010-11-14 15:39 [Qemu-devel] [PATCHv4 00/15] boot order specification Gleb Natapov
` (4 preceding siblings ...)
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 05/15] Store IDE bus id in IDEBus structure for easy access Gleb Natapov
@ 2010-11-14 15:39 ` Gleb Natapov
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 07/15] Add get_dev_path callback for system bus Gleb Natapov
` (8 subsequent siblings)
14 siblings, 0 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-14 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kvm, mst, armbru, blauwirbel, alex.williamson, kevin
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
hw/ide/qdev.c | 13 +++++++++++++
1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 88ff657..01a181b 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -24,9 +24,12 @@
/* --------------------------------- */
+static char *idebus_get_fw_dev_path(DeviceState *dev);
+
static struct BusInfo ide_bus_info = {
.name = "IDE",
.size = sizeof(IDEBus),
+ .get_fw_dev_path = idebus_get_fw_dev_path,
};
void ide_bus_new(IDEBus *idebus, DeviceState *dev, int bus_id)
@@ -35,6 +38,16 @@ void ide_bus_new(IDEBus *idebus, DeviceState *dev, int bus_id)
idebus->bus_id = bus_id;
}
+static char *idebus_get_fw_dev_path(DeviceState *dev)
+{
+ char path[30];
+
+ snprintf(path, sizeof(path), "%s@%d", qdev_fw_name(dev),
+ ((IDEBus*)dev->parent_bus)->bus_id);
+
+ return strdup(path);
+}
+
static int ide_qdev_init(DeviceState *qdev, DeviceInfo *base)
{
IDEDevice *dev = DO_UPCAST(IDEDevice, qdev, qdev);
--
1.7.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCHv4 07/15] Add get_dev_path callback for system bus.
2010-11-14 15:39 [Qemu-devel] [PATCHv4 00/15] boot order specification Gleb Natapov
` (5 preceding siblings ...)
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 06/15] Add get_fw_dev_path callback to IDE bus Gleb Natapov
@ 2010-11-14 15:39 ` Gleb Natapov
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 08/15] Add get_fw_dev_path callback for pci bus Gleb Natapov
` (7 subsequent siblings)
14 siblings, 0 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-14 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kvm, mst, armbru, blauwirbel, alex.williamson, kevin
Prints out mmio or pio used to access child device.
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
hw/pci_host.c | 2 ++
hw/sysbus.c | 30 ++++++++++++++++++++++++++++++
hw/sysbus.h | 4 ++++
3 files changed, 36 insertions(+), 0 deletions(-)
diff --git a/hw/pci_host.c b/hw/pci_host.c
index bc5b771..28d45bf 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -197,6 +197,7 @@ void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s)
{
pci_host_init(s);
register_ioport_simple(&s->conf_noswap_handler, ioport, 4, 4);
+ sysbus_init_ioports(&s->busdev, ioport, 4);
}
int pci_host_data_register_mmio(PCIHostState *s, int swap)
@@ -215,4 +216,5 @@ void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s)
register_ioport_simple(&s->data_noswap_handler, ioport, 4, 1);
register_ioport_simple(&s->data_noswap_handler, ioport, 4, 2);
register_ioport_simple(&s->data_noswap_handler, ioport, 4, 4);
+ sysbus_init_ioports(&s->busdev, ioport, 4);
}
diff --git a/hw/sysbus.c b/hw/sysbus.c
index d817721..1583bd8 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -22,11 +22,13 @@
#include "monitor.h"
static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent);
+static char *sysbus_get_fw_dev_path(DeviceState *dev);
struct BusInfo system_bus_info = {
.name = "System",
.size = sizeof(BusState),
.print_dev = sysbus_dev_print,
+ .get_fw_dev_path = sysbus_get_fw_dev_path,
};
void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq)
@@ -106,6 +108,16 @@ void sysbus_init_mmio_cb(SysBusDevice *dev, target_phys_addr_t size,
dev->mmio[n].cb = cb;
}
+void sysbus_init_ioports(SysBusDevice *dev, pio_addr_t ioport, pio_addr_t size)
+{
+ pio_addr_t i;
+
+ for (i = 0; i < size; i++) {
+ assert(dev->num_pio < QDEV_MAX_PIO);
+ dev->pio[dev->num_pio++] = ioport++;
+ }
+}
+
static int sysbus_device_init(DeviceState *dev, DeviceInfo *base)
{
SysBusDeviceInfo *info = container_of(base, SysBusDeviceInfo, qdev);
@@ -171,3 +183,21 @@ static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
indent, "", s->mmio[i].addr, s->mmio[i].size);
}
}
+
+static char *sysbus_get_fw_dev_path(DeviceState *dev)
+{
+ SysBusDevice *s = sysbus_from_qdev(dev);
+ char path[40];
+ int off;
+
+ off = snprintf(path, sizeof(path), "%s", qdev_fw_name(dev));
+
+ if (s->num_mmio) {
+ snprintf(path + off, sizeof(path) - off, "@"TARGET_FMT_plx,
+ s->mmio[0].addr);
+ } else if (s->num_pio) {
+ snprintf(path + off, sizeof(path) - off, "@i%04x", s->pio[0]);
+ }
+
+ return strdup(path);
+}
diff --git a/hw/sysbus.h b/hw/sysbus.h
index 5980901..e9eb618 100644
--- a/hw/sysbus.h
+++ b/hw/sysbus.h
@@ -6,6 +6,7 @@
#include "qdev.h"
#define QDEV_MAX_MMIO 32
+#define QDEV_MAX_PIO 32
#define QDEV_MAX_IRQ 256
typedef struct SysBusDevice SysBusDevice;
@@ -23,6 +24,8 @@ struct SysBusDevice {
mmio_mapfunc cb;
ram_addr_t iofunc;
} mmio[QDEV_MAX_MMIO];
+ int num_pio;
+ pio_addr_t pio[QDEV_MAX_PIO];
};
typedef int (*sysbus_initfn)(SysBusDevice *dev);
@@ -45,6 +48,7 @@ void sysbus_init_mmio_cb(SysBusDevice *dev, target_phys_addr_t size,
mmio_mapfunc cb);
void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p);
void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice *target);
+void sysbus_init_ioports(SysBusDevice *dev, pio_addr_t ioport, pio_addr_t size);
void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq);
--
1.7.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCHv4 08/15] Add get_fw_dev_path callback for pci bus.
2010-11-14 15:39 [Qemu-devel] [PATCHv4 00/15] boot order specification Gleb Natapov
` (6 preceding siblings ...)
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 07/15] Add get_dev_path callback for system bus Gleb Natapov
@ 2010-11-14 15:39 ` Gleb Natapov
2010-11-14 18:27 ` [Qemu-devel] " Michael S. Tsirkin
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 09/15] Record which USBDevice USBPort belongs too Gleb Natapov
` (6 subsequent siblings)
14 siblings, 1 reply; 55+ messages in thread
From: Gleb Natapov @ 2010-11-14 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kvm, mst, armbru, blauwirbel, alex.williamson, kevin
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
hw/pci.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++-------------
1 files changed, 85 insertions(+), 23 deletions(-)
diff --git a/hw/pci.c b/hw/pci.c
index 962886e..114b435 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -43,12 +43,14 @@
static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
static char *pcibus_get_dev_path(DeviceState *dev);
+static char *pcibus_get_fw_dev_path(DeviceState *dev);
struct BusInfo pci_bus_info = {
.name = "PCI",
.size = sizeof(PCIBus),
.print_dev = pcibus_dev_print,
.get_dev_path = pcibus_get_dev_path,
+ .get_fw_dev_path = pcibus_get_fw_dev_path,
.props = (Property[]) {
DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
@@ -1061,45 +1063,63 @@ void pci_msi_notify(PCIDevice *dev, unsigned int vector)
typedef struct {
uint16_t class;
const char *desc;
+ const char *fw_name;
+ uint16_t fw_ign_bits;
} pci_class_desc;
static const pci_class_desc pci_class_descriptions[] =
{
- { 0x0100, "SCSI controller"},
- { 0x0101, "IDE controller"},
- { 0x0102, "Floppy controller"},
- { 0x0103, "IPI controller"},
- { 0x0104, "RAID controller"},
+ { 0x0001, "VGA controller", "display"},
+ { 0x0100, "SCSI controller", "scsi"},
+ { 0x0101, "IDE controller", "ide"},
+ { 0x0102, "Floppy controller", "fdc"},
+ { 0x0103, "IPI controller", "ipi"},
+ { 0x0104, "RAID controller", "raid"},
{ 0x0106, "SATA controller"},
{ 0x0107, "SAS controller"},
{ 0x0180, "Storage controller"},
- { 0x0200, "Ethernet controller"},
- { 0x0201, "Token Ring controller"},
- { 0x0202, "FDDI controller"},
- { 0x0203, "ATM controller"},
+ { 0x0200, "Ethernet controller", "ethernet"},
+ { 0x0201, "Token Ring controller", "token-ring"},
+ { 0x0202, "FDDI controller", "fddi"},
+ { 0x0203, "ATM controller", "atm"},
{ 0x0280, "Network controller"},
- { 0x0300, "VGA controller"},
+ { 0x0300, "VGA controller", "display", 0x00ff},
{ 0x0301, "XGA controller"},
{ 0x0302, "3D controller"},
{ 0x0380, "Display controller"},
- { 0x0400, "Video controller"},
- { 0x0401, "Audio controller"},
+ { 0x0400, "Video controller", "video"},
+ { 0x0401, "Audio controller", "sound"},
{ 0x0402, "Phone"},
{ 0x0480, "Multimedia controller"},
- { 0x0500, "RAM controller"},
- { 0x0501, "Flash controller"},
+ { 0x0500, "RAM controller", "memory"},
+ { 0x0501, "Flash controller", "flash"},
{ 0x0580, "Memory controller"},
- { 0x0600, "Host bridge"},
- { 0x0601, "ISA bridge"},
- { 0x0602, "EISA bridge"},
- { 0x0603, "MC bridge"},
- { 0x0604, "PCI bridge"},
- { 0x0605, "PCMCIA bridge"},
- { 0x0606, "NUBUS bridge"},
- { 0x0607, "CARDBUS bridge"},
+ { 0x0600, "Host bridge", "host"},
+ { 0x0601, "ISA bridge", "isa"},
+ { 0x0602, "EISA bridge", "eisa"},
+ { 0x0603, "MC bridge", "mca"},
+ { 0x0604, "PCI bridge", "pci"},
+ { 0x0605, "PCMCIA bridge", "pcmcia"},
+ { 0x0606, "NUBUS bridge", "nubus"},
+ { 0x0607, "CARDBUS bridge", "cardbus"},
{ 0x0608, "RACEWAY bridge"},
{ 0x0680, "Bridge"},
- { 0x0c03, "USB controller"},
+ { 0x0700, "Serial port", "serial"},
+ { 0x0701, "Parallel port", "parallel"},
+ { 0x0800, "Interrupt controller", "interrupt-controller"},
+ { 0x0801, "DMA controller", "dma-controller"},
+ { 0x0802, "Timer", "timer"},
+ { 0x0803, "RTC", "rtc"},
+ { 0x0900, "Keyboard", "keyboard"},
+ { 0x0901, "Pen", "pen"},
+ { 0x0902, "Mouse", "mouse"},
+ { 0x0A00, "Dock station", "dock", 0x00ff},
+ { 0x0B00, "i386 cpu", "cpu", 0x00ff},
+ { 0x0c00, "Fireware contorller", "fireware"},
+ { 0x0c01, "Access bus controller", "access-bus"},
+ { 0x0c02, "SSA controller", "ssa"},
+ { 0x0c03, "USB controller", "usb"},
+ { 0x0c04, "Fibre channel controller", "fibre-channel"},
{ 0, NULL}
};
@@ -1825,6 +1845,48 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
}
}
+static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len)
+{
+ PCIDevice *d = (PCIDevice *)dev;
+ const char *name = NULL;
+ const pci_class_desc *desc = pci_class_descriptions;
+ int class = pci_get_word(d->config + PCI_CLASS_DEVICE);
+
+ while (desc->desc &&
+ (class & ~desc->fw_ign_bits) !=
+ (desc->class & ~desc->fw_ign_bits)) {
+ desc++;
+ }
+
+ if (desc->desc) {
+ name = desc->fw_name;
+ }
+
+ if (name) {
+ pstrcpy(buf, len, name);
+ } else {
+ snprintf(buf, len, "pci%04x,%04x",
+ pci_get_word(d->config + PCI_VENDOR_ID),
+ pci_get_word(d->config + PCI_DEVICE_ID));
+ }
+
+ return buf;
+}
+
+static char *pcibus_get_fw_dev_path(DeviceState *dev)
+{
+ PCIDevice *d = (PCIDevice *)dev;
+ char path[50], name[33];
+ int off;
+
+ off = snprintf(path, sizeof(path), "%s@%x",
+ pci_dev_fw_name(dev, name, sizeof name),
+ PCI_SLOT(d->devfn));
+ if (PCI_FUNC(d->devfn))
+ snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
+ return strdup(path);
+}
+
static char *pcibus_get_dev_path(DeviceState *dev)
{
PCIDevice *d = (PCIDevice *)dev;
--
1.7.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 08/15] Add get_fw_dev_path callback for pci bus.
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 08/15] Add get_fw_dev_path callback for pci bus Gleb Natapov
@ 2010-11-14 18:27 ` Michael S. Tsirkin
2010-11-14 18:42 ` Gleb Natapov
0 siblings, 1 reply; 55+ messages in thread
From: Michael S. Tsirkin @ 2010-11-14 18:27 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, qemu-devel, armbru, blauwirbel, alex.williamson, kevin
On Sun, Nov 14, 2010 at 05:39:34PM +0200, Gleb Natapov wrote:
>
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
> hw/pci.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++-------------
> 1 files changed, 85 insertions(+), 23 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index 962886e..114b435 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -43,12 +43,14 @@
>
> static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
> static char *pcibus_get_dev_path(DeviceState *dev);
> +static char *pcibus_get_fw_dev_path(DeviceState *dev);
>
> struct BusInfo pci_bus_info = {
> .name = "PCI",
> .size = sizeof(PCIBus),
> .print_dev = pcibus_dev_print,
> .get_dev_path = pcibus_get_dev_path,
> + .get_fw_dev_path = pcibus_get_fw_dev_path,
> .props = (Property[]) {
> DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> @@ -1061,45 +1063,63 @@ void pci_msi_notify(PCIDevice *dev, unsigned int vector)
> typedef struct {
> uint16_t class;
> const char *desc;
> + const char *fw_name;
> + uint16_t fw_ign_bits;
> } pci_class_desc;
>
> static const pci_class_desc pci_class_descriptions[] =
> {
> - { 0x0100, "SCSI controller"},
> - { 0x0101, "IDE controller"},
> - { 0x0102, "Floppy controller"},
> - { 0x0103, "IPI controller"},
> - { 0x0104, "RAID controller"},
> + { 0x0001, "VGA controller", "display"},
> + { 0x0100, "SCSI controller", "scsi"},
> + { 0x0101, "IDE controller", "ide"},
> + { 0x0102, "Floppy controller", "fdc"},
> + { 0x0103, "IPI controller", "ipi"},
> + { 0x0104, "RAID controller", "raid"},
> { 0x0106, "SATA controller"},
> { 0x0107, "SAS controller"},
> { 0x0180, "Storage controller"},
> - { 0x0200, "Ethernet controller"},
> - { 0x0201, "Token Ring controller"},
> - { 0x0202, "FDDI controller"},
> - { 0x0203, "ATM controller"},
> + { 0x0200, "Ethernet controller", "ethernet"},
> + { 0x0201, "Token Ring controller", "token-ring"},
> + { 0x0202, "FDDI controller", "fddi"},
> + { 0x0203, "ATM controller", "atm"},
> { 0x0280, "Network controller"},
> - { 0x0300, "VGA controller"},
> + { 0x0300, "VGA controller", "display", 0x00ff},
> { 0x0301, "XGA controller"},
> { 0x0302, "3D controller"},
> { 0x0380, "Display controller"},
> - { 0x0400, "Video controller"},
> - { 0x0401, "Audio controller"},
> + { 0x0400, "Video controller", "video"},
> + { 0x0401, "Audio controller", "sound"},
> { 0x0402, "Phone"},
> { 0x0480, "Multimedia controller"},
> - { 0x0500, "RAM controller"},
> - { 0x0501, "Flash controller"},
> + { 0x0500, "RAM controller", "memory"},
> + { 0x0501, "Flash controller", "flash"},
> { 0x0580, "Memory controller"},
> - { 0x0600, "Host bridge"},
> - { 0x0601, "ISA bridge"},
> - { 0x0602, "EISA bridge"},
> - { 0x0603, "MC bridge"},
> - { 0x0604, "PCI bridge"},
> - { 0x0605, "PCMCIA bridge"},
> - { 0x0606, "NUBUS bridge"},
> - { 0x0607, "CARDBUS bridge"},
> + { 0x0600, "Host bridge", "host"},
> + { 0x0601, "ISA bridge", "isa"},
> + { 0x0602, "EISA bridge", "eisa"},
> + { 0x0603, "MC bridge", "mca"},
> + { 0x0604, "PCI bridge", "pci"},
> + { 0x0605, "PCMCIA bridge", "pcmcia"},
> + { 0x0606, "NUBUS bridge", "nubus"},
> + { 0x0607, "CARDBUS bridge", "cardbus"},
> { 0x0608, "RACEWAY bridge"},
> { 0x0680, "Bridge"},
> - { 0x0c03, "USB controller"},
> + { 0x0700, "Serial port", "serial"},
> + { 0x0701, "Parallel port", "parallel"},
> + { 0x0800, "Interrupt controller", "interrupt-controller"},
> + { 0x0801, "DMA controller", "dma-controller"},
> + { 0x0802, "Timer", "timer"},
> + { 0x0803, "RTC", "rtc"},
> + { 0x0900, "Keyboard", "keyboard"},
> + { 0x0901, "Pen", "pen"},
> + { 0x0902, "Mouse", "mouse"},
> + { 0x0A00, "Dock station", "dock", 0x00ff},
> + { 0x0B00, "i386 cpu", "cpu", 0x00ff},
> + { 0x0c00, "Fireware contorller", "fireware"},
> + { 0x0c01, "Access bus controller", "access-bus"},
> + { 0x0c02, "SSA controller", "ssa"},
> + { 0x0c03, "USB controller", "usb"},
> + { 0x0c04, "Fibre channel controller", "fibre-channel"},
> { 0, NULL}
> };
>
> @@ -1825,6 +1845,48 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent)
> }
> }
>
> +static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len)
> +{
> + PCIDevice *d = (PCIDevice *)dev;
> + const char *name = NULL;
> + const pci_class_desc *desc = pci_class_descriptions;
> + int class = pci_get_word(d->config + PCI_CLASS_DEVICE);
> +
> + while (desc->desc &&
> + (class & ~desc->fw_ign_bits) !=
> + (desc->class & ~desc->fw_ign_bits)) {
> + desc++;
> + }
> +
> + if (desc->desc) {
> + name = desc->fw_name;
> + }
> +
> + if (name) {
> + pstrcpy(buf, len, name);
> + } else {
> + snprintf(buf, len, "pci%04x,%04x",
> + pci_get_word(d->config + PCI_VENDOR_ID),
> + pci_get_word(d->config + PCI_DEVICE_ID));
> + }
> +
> + return buf;
> +}
> +
> +static char *pcibus_get_fw_dev_path(DeviceState *dev)
> +{
> + PCIDevice *d = (PCIDevice *)dev;
> + char path[50], name[33];
> + int off;
> +
> + off = snprintf(path, sizeof(path), "%s@%x",
> + pci_dev_fw_name(dev, name, sizeof name),
> + PCI_SLOT(d->devfn));
> + if (PCI_FUNC(d->devfn))
> + snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
Can we *always* specify a slot number in the name?
If yes I think we should, because I think we saw that the short form is
ambiguous: if there's a device at a slot != 0, openfirmware will
to match that against the name.
Right?
> + return strdup(path);
> +}
> +
> static char *pcibus_get_dev_path(DeviceState *dev)
> {
> PCIDevice *d = (PCIDevice *)dev;
> --
> 1.7.1
^ permalink raw reply [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 08/15] Add get_fw_dev_path callback for pci bus.
2010-11-14 18:27 ` [Qemu-devel] " Michael S. Tsirkin
@ 2010-11-14 18:42 ` Gleb Natapov
0 siblings, 0 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-14 18:42 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, qemu-devel, armbru, blauwirbel, alex.williamson, kevin
On Sun, Nov 14, 2010 at 08:27:20PM +0200, Michael S. Tsirkin wrote:
> On Sun, Nov 14, 2010 at 05:39:34PM +0200, Gleb Natapov wrote:
> > +static char *pcibus_get_fw_dev_path(DeviceState *dev)
> > +{
> > + PCIDevice *d = (PCIDevice *)dev;
> > + char path[50], name[33];
> > + int off;
> > +
> > + off = snprintf(path, sizeof(path), "%s@%x",
> > + pci_dev_fw_name(dev, name, sizeof name),
> > + PCI_SLOT(d->devfn));
> > + if (PCI_FUNC(d->devfn))
> > + snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
>
> Can we *always* specify a slot number in the name?
> If yes I think we should, because I think we saw that the short form is
> ambiguous: if there's a device at a slot != 0, openfirmware will
> to match that against the name.
You accidentally something here.
> Right?
>
I do not see what is ambiguous here. Can you explain?
--
Gleb.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] [PATCHv4 09/15] Record which USBDevice USBPort belongs too.
2010-11-14 15:39 [Qemu-devel] [PATCHv4 00/15] boot order specification Gleb Natapov
` (7 preceding siblings ...)
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 08/15] Add get_fw_dev_path callback for pci bus Gleb Natapov
@ 2010-11-14 15:39 ` Gleb Natapov
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 10/15] Add get_dev_path callback for usb bus Gleb Natapov
` (5 subsequent siblings)
14 siblings, 0 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-14 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kvm, mst, armbru, blauwirbel, alex.williamson, kevin
Ports on root hub will have NULL here. This is needed to reconstruct
path from device to its root hub to build device path.
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
hw/usb-bus.c | 3 ++-
hw/usb-hub.c | 2 +-
hw/usb-musb.c | 2 +-
hw/usb-ohci.c | 2 +-
hw/usb-uhci.c | 2 +-
hw/usb.h | 3 ++-
6 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index b692503..256b881 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -110,11 +110,12 @@ USBDevice *usb_create_simple(USBBus *bus, const char *name)
}
void usb_register_port(USBBus *bus, USBPort *port, void *opaque, int index,
- usb_attachfn attach)
+ USBDevice *pdev, usb_attachfn attach)
{
port->opaque = opaque;
port->index = index;
port->attach = attach;
+ port->pdev = pdev;
QTAILQ_INSERT_TAIL(&bus->free, port, next);
bus->nfree++;
}
diff --git a/hw/usb-hub.c b/hw/usb-hub.c
index 8e3a96b..8a3f829 100644
--- a/hw/usb-hub.c
+++ b/hw/usb-hub.c
@@ -535,7 +535,7 @@ static int usb_hub_initfn(USBDevice *dev)
for (i = 0; i < s->nb_ports; i++) {
port = &s->ports[i];
usb_register_port(usb_bus_from_device(dev),
- &port->port, s, i, usb_hub_attach);
+ &port->port, s, i, &s->dev, usb_hub_attach);
port->wPortStatus = PORT_STAT_POWER;
port->wPortChange = 0;
}
diff --git a/hw/usb-musb.c b/hw/usb-musb.c
index 7f15842..9efe7a6 100644
--- a/hw/usb-musb.c
+++ b/hw/usb-musb.c
@@ -343,7 +343,7 @@ struct MUSBState {
}
usb_bus_new(&s->bus, NULL /* FIXME */);
- usb_register_port(&s->bus, &s->port, s, 0, musb_attach);
+ usb_register_port(&s->bus, &s->port, s, 0, NULL, musb_attach);
return s;
}
diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
index c60fd8d..59604cf 100644
--- a/hw/usb-ohci.c
+++ b/hw/usb-ohci.c
@@ -1705,7 +1705,7 @@ static void usb_ohci_init(OHCIState *ohci, DeviceState *dev,
usb_bus_new(&ohci->bus, dev);
ohci->num_ports = num_ports;
for (i = 0; i < num_ports; i++) {
- usb_register_port(&ohci->bus, &ohci->rhport[i].port, ohci, i, ohci_attach);
+ usb_register_port(&ohci->bus, &ohci->rhport[i].port, ohci, i, NULL, ohci_attach);
}
ohci->async_td = 0;
diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c
index 1d83400..b9b822f 100644
--- a/hw/usb-uhci.c
+++ b/hw/usb-uhci.c
@@ -1115,7 +1115,7 @@ static int usb_uhci_common_initfn(UHCIState *s)
usb_bus_new(&s->bus, &s->dev.qdev);
for(i = 0; i < NB_PORTS; i++) {
- usb_register_port(&s->bus, &s->ports[i].port, s, i, uhci_attach);
+ usb_register_port(&s->bus, &s->ports[i].port, s, i, NULL, uhci_attach);
}
s->frame_timer = qemu_new_timer(vm_clock, uhci_frame_timer, s);
s->expire_time = qemu_get_clock(vm_clock) +
diff --git a/hw/usb.h b/hw/usb.h
index 00d2802..0b32d77 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -203,6 +203,7 @@ struct USBPort {
USBDevice *dev;
usb_attachfn attach;
void *opaque;
+ USBDevice *pdev;
int index; /* internal port index, may be used with the opaque */
QTAILQ_ENTRY(USBPort) next;
};
@@ -312,7 +313,7 @@ USBDevice *usb_create(USBBus *bus, const char *name);
USBDevice *usb_create_simple(USBBus *bus, const char *name);
USBDevice *usbdevice_create(const char *cmdline);
void usb_register_port(USBBus *bus, USBPort *port, void *opaque, int index,
- usb_attachfn attach);
+ USBDevice *pdev, usb_attachfn attach);
void usb_unregister_port(USBBus *bus, USBPort *port);
int usb_device_attach(USBDevice *dev);
int usb_device_detach(USBDevice *dev);
--
1.7.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCHv4 10/15] Add get_dev_path callback for usb bus.
2010-11-14 15:39 [Qemu-devel] [PATCHv4 00/15] boot order specification Gleb Natapov
` (8 preceding siblings ...)
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 09/15] Record which USBDevice USBPort belongs too Gleb Natapov
@ 2010-11-14 15:39 ` Gleb Natapov
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 11/15] Add bootindex parameter to net/block/fd device Gleb Natapov
` (4 subsequent siblings)
14 siblings, 0 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-14 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kvm, mst, armbru, blauwirbel, alex.williamson, kevin
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
hw/usb-bus.c | 42 ++++++++++++++++++++++++++++++++++++++++++
1 files changed, 42 insertions(+), 0 deletions(-)
diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index 256b881..8b4583c 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -5,11 +5,13 @@
#include "monitor.h"
static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
+static char *usbbus_get_fw_dev_path(DeviceState *dev);
static struct BusInfo usb_bus_info = {
.name = "USB",
.size = sizeof(USBBus),
.print_dev = usb_bus_dev_print,
+ .get_fw_dev_path = usbbus_get_fw_dev_path,
};
static int next_usb_bus = 0;
static QTAILQ_HEAD(, USBBus) busses = QTAILQ_HEAD_INITIALIZER(busses);
@@ -307,3 +309,43 @@ USBDevice *usbdevice_create(const char *cmdline)
}
return usb->usbdevice_init(params);
}
+
+static int usbbus_get_fw_dev_path_helper(USBDevice *d, USBBus *bus, char *p,
+ int len)
+{
+ int l = 0;
+ USBPort *port;
+
+ QTAILQ_FOREACH(port, &bus->used, next) {
+ if (port->dev == d) {
+ if (port->pdev) {
+ l = usbbus_get_fw_dev_path_helper(port->pdev, bus, p, len);
+ }
+ l += snprintf(p + l, len - l, "%s@%x/", qdev_fw_name(&d->qdev),
+ port->index);
+ break;
+ }
+ }
+
+ return l;
+}
+
+static char *usbbus_get_fw_dev_path(DeviceState *dev)
+{
+ USBDevice *d = (USBDevice*)dev;
+ USBBus *bus = usb_bus_from_device(d);
+ char path[100];
+ int l;
+
+ assert(d->attached != 0);
+
+ l = usbbus_get_fw_dev_path_helper(d, bus, path, sizeof(path));
+
+ if (l == 0) {
+ abort();
+ }
+
+ path[l-1] = '\0';
+
+ return strdup(path);
+}
--
1.7.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCHv4 11/15] Add bootindex parameter to net/block/fd device
2010-11-14 15:39 [Qemu-devel] [PATCHv4 00/15] boot order specification Gleb Natapov
` (9 preceding siblings ...)
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 10/15] Add get_dev_path callback for usb bus Gleb Natapov
@ 2010-11-14 15:39 ` Gleb Natapov
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 12/15] Change fw_cfg_add_file() to get full file path as a parameter Gleb Natapov
` (3 subsequent siblings)
14 siblings, 0 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-14 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kvm, mst, armbru, blauwirbel, alex.williamson, kevin
If bootindex is specified on command line a string that describes device
in firmware readable way is added into sorted list. Later this list will
be passed into firmware to control boot order.
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
block_int.h | 4 +++-
hw/e1000.c | 4 ++++
hw/eepro100.c | 3 +++
hw/fdc.c | 8 ++++++++
hw/ide/qdev.c | 5 +++++
hw/ne2000.c | 3 +++
hw/pcnet.c | 4 ++++
hw/qdev.c | 32 ++++++++++++++++++++++++++++++++
hw/qdev.h | 1 +
hw/rtl8139.c | 4 ++++
hw/usb-net.c | 2 ++
hw/virtio-blk.c | 2 ++
hw/virtio-net.c | 2 ++
net.h | 4 +++-
sysemu.h | 2 ++
vl.c | 40 ++++++++++++++++++++++++++++++++++++++++
16 files changed, 118 insertions(+), 2 deletions(-)
diff --git a/block_int.h b/block_int.h
index 3c3adb5..0a0e47d 100644
--- a/block_int.h
+++ b/block_int.h
@@ -227,6 +227,7 @@ typedef struct BlockConf {
uint16_t logical_block_size;
uint16_t min_io_size;
uint32_t opt_io_size;
+ int32_t bootindex;
} BlockConf;
static inline unsigned int get_physical_block_exp(BlockConf *conf)
@@ -249,6 +250,7 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
DEFINE_PROP_UINT16("physical_block_size", _state, \
_conf.physical_block_size, 512), \
DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0), \
- DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0)
+ DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0), \
+ DEFINE_PROP_INT32("bootindex", _state, _conf.bootindex, -1) \
#endif /* BLOCK_INT_H */
diff --git a/hw/e1000.c b/hw/e1000.c
index 532efdc..053f33e 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -30,6 +30,7 @@
#include "net.h"
#include "net/checksum.h"
#include "loader.h"
+#include "sysemu.h"
#include "e1000_hw.h"
@@ -1148,6 +1149,9 @@ static int pci_e1000_init(PCIDevice *pci_dev)
d->dev.qdev.info->name, d->dev.qdev.id, d);
qemu_format_nic_info_str(&d->nic->nc, macaddr);
+
+ add_boot_device_path(d->conf.bootindex, &pci_dev->qdev, "ethernet-phy@0");
+
return 0;
}
diff --git a/hw/eepro100.c b/hw/eepro100.c
index 41d792a..80adac6 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -46,6 +46,7 @@
#include "pci.h"
#include "net.h"
#include "eeprom93xx.h"
+#include "sysemu.h"
#define KiB 1024
@@ -1907,6 +1908,8 @@ static int e100_nic_init(PCIDevice *pci_dev)
s->vmstate->name = s->nic->nc.model;
vmstate_register(&pci_dev->qdev, -1, s->vmstate, s);
+ add_boot_device_path(s->conf.bootindex, &pci_dev->qdev, "ethernet-phy@0");
+
return 0;
}
diff --git a/hw/fdc.c b/hw/fdc.c
index 5ab754b..7b1349f 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -35,6 +35,7 @@
#include "sysbus.h"
#include "qdev-addr.h"
#include "blockdev.h"
+#include "sysemu.h"
/********************************************************/
/* debug Floppy devices */
@@ -523,6 +524,8 @@ typedef struct FDCtrlSysBus {
typedef struct FDCtrlISABus {
ISADevice busdev;
struct FDCtrl state;
+ int32_t bootindexA;
+ int32_t bootindexB;
} FDCtrlISABus;
static uint32_t fdctrl_read (void *opaque, uint32_t reg)
@@ -1992,6 +1995,9 @@ static int isabus_fdc_init1(ISADevice *dev)
qdev_set_legacy_instance_id(&dev->qdev, iobase, 2);
ret = fdctrl_init_common(fdctrl);
+ add_boot_device_path(isa->bootindexA, &dev->qdev, "floppy@0");
+ add_boot_device_path(isa->bootindexB, &dev->qdev, "floppy@1");
+
return ret;
}
@@ -2051,6 +2057,8 @@ static ISADeviceInfo isa_fdc_info = {
.qdev.props = (Property[]) {
DEFINE_PROP_DRIVE("driveA", FDCtrlISABus, state.drives[0].bs),
DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.drives[1].bs),
+ DEFINE_PROP_INT32("bootindexA", FDCtrlISABus, bootindexA, -1),
+ DEFINE_PROP_INT32("bootindexB", FDCtrlISABus, bootindexB, -1),
DEFINE_PROP_END_OF_LIST(),
},
};
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 01a181b..69a00e2 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -21,6 +21,7 @@
#include "qemu-error.h"
#include <hw/ide/internal.h>
#include "blockdev.h"
+#include "sysemu.h"
/* --------------------------------- */
@@ -143,6 +144,10 @@ static int ide_drive_initfn(IDEDevice *dev)
if (!dev->serial) {
dev->serial = qemu_strdup(s->drive_serial_str);
}
+
+ add_boot_device_path(dev->conf.bootindex, &dev->qdev,
+ dev->unit ? "disk@1" : "disk@0");
+
return 0;
}
diff --git a/hw/ne2000.c b/hw/ne2000.c
index 126e7cf..f4bbac2 100644
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -26,6 +26,7 @@
#include "net.h"
#include "ne2000.h"
#include "loader.h"
+#include "sysemu.h"
/* debug NE2000 card */
//#define DEBUG_NE2000
@@ -746,6 +747,8 @@ static int pci_ne2000_init(PCIDevice *pci_dev)
}
}
+ add_boot_device_path(s->c.bootindex, &pci_dev->qdev, "ethernet-phy@0");
+
return 0;
}
diff --git a/hw/pcnet.c b/hw/pcnet.c
index b52935a..a988f4f 100644
--- a/hw/pcnet.c
+++ b/hw/pcnet.c
@@ -40,6 +40,7 @@
#include "loader.h"
#include "qemu-timer.h"
#include "qemu_socket.h"
+#include "sysemu.h"
#include "pcnet.h"
@@ -1898,6 +1899,9 @@ int pcnet_common_init(DeviceState *dev, PCNetState *s, NetClientInfo *info)
qemu_macaddr_default_if_unset(&s->conf.macaddr);
s->nic = qemu_new_nic(info, &s->conf, dev->info->name, dev->id, s);
qemu_format_nic_info_str(&s->nic->nc, s->conf.macaddr.a);
+
+ add_boot_device_path(s->conf.bootindex, dev, "ethernet-phy@0");
+
return 0;
}
diff --git a/hw/qdev.c b/hw/qdev.c
index 35858cb..cc9bf43 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -820,3 +820,35 @@ int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
}
return qdev_unplug(dev);
}
+
+static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
+{
+ int l = 0;
+
+ if (dev && dev->parent_bus) {
+ char *d;
+ l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
+ if (dev->parent_bus->info->get_fw_dev_path) {
+ d = dev->parent_bus->info->get_fw_dev_path(dev);
+ l += snprintf(p + l, size - l, "%s", d);
+ qemu_free(d);
+ } else {
+ l += snprintf(p + l, size - l, "%s", dev->info->name);
+ }
+ }
+ l += snprintf(p + l , size - l, "/");
+
+ return l;
+}
+
+char* qdev_get_fw_dev_path(DeviceState *dev)
+{
+ char path[128];
+ int l;
+
+ l = qdev_get_fw_dev_path_helper(dev, path, 128);
+
+ path[l-1] = '\0';
+
+ return strdup(path);
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index dc669b3..13f3597 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -296,6 +296,7 @@ static inline const char *qdev_fw_name(DeviceState *dev)
return dev->info->fw_name ? : dev->info->alias ? : dev->info->name;
}
+char *qdev_get_fw_dev_path(DeviceState *dev);
/* This is a nasty hack to allow passing a NULL bus to qdev_create. */
extern struct BusInfo system_bus_info;
diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index d92981d..e44ffb8 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -52,6 +52,7 @@
#include "qemu-timer.h"
#include "net.h"
#include "loader.h"
+#include "sysemu.h"
/* debug RTL8139 card */
//#define DEBUG_RTL8139 1
@@ -3387,6 +3388,9 @@ static int pci_rtl8139_init(PCIDevice *dev)
s->TimerExpire = 0;
s->timer = qemu_new_timer(vm_clock, rtl8139_timer, s);
rtl8139_set_next_tctr_time(s, qemu_get_clock(vm_clock));
+
+ add_boot_device_path(s->conf.bootindex, &dev->qdev, "ethernet-phy@0");
+
return 0;
}
diff --git a/hw/usb-net.c b/hw/usb-net.c
index 2287ee1..dac20a7 100644
--- a/hw/usb-net.c
+++ b/hw/usb-net.c
@@ -27,6 +27,7 @@
#include "usb.h"
#include "net.h"
#include "qemu-queue.h"
+#include "sysemu.h"
/*#define TRAFFIC_DEBUG*/
/* Thanks to NetChip Technologies for donating this product ID.
@@ -1463,6 +1464,7 @@ static int usb_net_initfn(USBDevice *dev)
s->conf.macaddr.a[4],
s->conf.macaddr.a[5]);
+ add_boot_device_path(s->conf.bootindex, &dev->qdev, "ethernet@0");
return 0;
}
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 49528a9..ba11e29 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -548,6 +548,8 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
bdrv_set_removable(s->bs, 0);
s->bs->buffer_alignment = conf->logical_block_size;
+ add_boot_device_path(conf->bootindex, dev, "disk@0");
+
return &s->vdev;
}
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 7e1688c..a2ba40b 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -1018,6 +1018,8 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
virtio_net_save, virtio_net_load, n);
n->vmstate = qemu_add_vm_change_state_handler(virtio_net_vmstate_change, n);
+ add_boot_device_path(conf->bootindex, dev, "ethernet-phy@0");
+
return &n->vdev;
}
diff --git a/net.h b/net.h
index 44c31a9..6ceca50 100644
--- a/net.h
+++ b/net.h
@@ -17,12 +17,14 @@ typedef struct NICConf {
MACAddr macaddr;
VLANState *vlan;
VLANClientState *peer;
+ int32_t bootindex;
} NICConf;
#define DEFINE_NIC_PROPERTIES(_state, _conf) \
DEFINE_PROP_MACADDR("mac", _state, _conf.macaddr), \
DEFINE_PROP_VLAN("vlan", _state, _conf.vlan), \
- DEFINE_PROP_NETDEV("netdev", _state, _conf.peer)
+ DEFINE_PROP_NETDEV("netdev", _state, _conf.peer), \
+ DEFINE_PROP_INT32("bootindex", _state, _conf.bootindex, -1)
/* VLANs support */
diff --git a/sysemu.h b/sysemu.h
index b81a70e..fe9a582 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -188,4 +188,6 @@ void rtc_change_mon_event(struct tm *tm);
void register_devices(void);
+void add_boot_device_path(int32_t bootindex, DeviceState *dev,
+ const char *suffix);
#endif
diff --git a/vl.c b/vl.c
index c58583d..4c159f0 100644
--- a/vl.c
+++ b/vl.c
@@ -228,6 +228,17 @@ unsigned int nb_prom_envs = 0;
const char *prom_envs[MAX_PROM_ENVS];
int boot_menu;
+typedef struct FWBootEntry FWBootEntry;
+
+struct FWBootEntry {
+ QTAILQ_ENTRY(FWBootEntry) link;
+ int32_t bootindex;
+ DeviceState *dev;
+ char *suffix;
+};
+
+QTAILQ_HEAD(, FWBootEntry) fw_boot_order = QTAILQ_HEAD_INITIALIZER(fw_boot_order);
+
int nb_numa_nodes;
uint64_t node_mem[MAX_NODES];
uint64_t node_cpumask[MAX_NODES];
@@ -692,6 +703,35 @@ static void restore_boot_devices(void *opaque)
qemu_free(standard_boot_devices);
}
+void add_boot_device_path(int32_t bootindex, DeviceState *dev,
+ const char *suffix)
+{
+ FWBootEntry *node, *i;
+
+ if (bootindex < 0) {
+ return;
+ }
+
+ assert(dev != NULL || suffix != NULL);
+
+ node = qemu_mallocz(sizeof(FWBootEntry));
+ node->bootindex = bootindex;
+ node->suffix = strdup(suffix);
+ node->dev = dev;
+
+ QTAILQ_FOREACH(i, &fw_boot_order, link) {
+ if (i->bootindex == bootindex) {
+ fprintf(stderr, "Two devices with same boot index %d\n", bootindex);
+ exit(1);
+ } else if (i->bootindex < bootindex) {
+ continue;
+ }
+ QTAILQ_INSERT_BEFORE(i, node, link);
+ return;
+ }
+ QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
+}
+
static void numa_add(const char *optarg)
{
char option[128];
--
1.7.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCHv4 12/15] Change fw_cfg_add_file() to get full file path as a parameter.
2010-11-14 15:39 [Qemu-devel] [PATCHv4 00/15] boot order specification Gleb Natapov
` (10 preceding siblings ...)
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 11/15] Add bootindex parameter to net/block/fd device Gleb Natapov
@ 2010-11-14 15:39 ` Gleb Natapov
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 13/15] Add bootindex for option roms Gleb Natapov
` (2 subsequent siblings)
14 siblings, 0 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-14 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kvm, mst, armbru, blauwirbel, alex.williamson, kevin
Change fw_cfg_add_file() to get full file path as a parameter instead
of building one internally. Two reasons for that. First caller may need
to know how file is named. Second this moves policy of file naming out
from fw_cfg. Platform may want to use more then two levels of
directories for instance.
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
hw/fw_cfg.c | 16 ++++------------
hw/fw_cfg.h | 4 ++--
hw/loader.c | 16 ++++++++++++++--
3 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index 72866ae..7b9434f 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -277,10 +277,9 @@ int fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
return 1;
}
-int fw_cfg_add_file(FWCfgState *s, const char *dir, const char *filename,
- uint8_t *data, uint32_t len)
+int fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
+ uint32_t len)
{
- const char *basename;
int i, index;
if (!s->files) {
@@ -297,15 +296,8 @@ int fw_cfg_add_file(FWCfgState *s, const char *dir, const char *filename,
fw_cfg_add_bytes(s, FW_CFG_FILE_FIRST + index, data, len);
- basename = strrchr(filename, '/');
- if (basename) {
- basename++;
- } else {
- basename = filename;
- }
-
- snprintf(s->files->f[index].name, sizeof(s->files->f[index].name),
- "%s/%s", dir, basename);
+ pstrcpy(s->files->f[index].name, sizeof(s->files->f[index].name),
+ filename);
for (i = 0; i < index; i++) {
if (strcmp(s->files->f[index].name, s->files->f[i].name) == 0) {
FW_CFG_DPRINTF("%s: skip duplicate: %s\n", __FUNCTION__,
diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
index 4d13a4f..856bf91 100644
--- a/hw/fw_cfg.h
+++ b/hw/fw_cfg.h
@@ -60,8 +60,8 @@ int fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value);
int fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
int fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
void *callback_opaque, uint8_t *data, size_t len);
-int fw_cfg_add_file(FWCfgState *s, const char *dir, const char *filename,
- uint8_t *data, uint32_t len);
+int fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
+ uint32_t len);
FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
target_phys_addr_t crl_addr, target_phys_addr_t data_addr);
diff --git a/hw/loader.c b/hw/loader.c
index 49ac1fa..1e98326 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -592,8 +592,20 @@ int rom_add_file(const char *file, const char *fw_dir,
}
close(fd);
rom_insert(rom);
- if (rom->fw_file && fw_cfg)
- fw_cfg_add_file(fw_cfg, rom->fw_dir, rom->fw_file, rom->data, rom->romsize);
+ if (rom->fw_file && fw_cfg) {
+ const char *basename;
+ char fw_file_name[56];
+
+ basename = strrchr(rom->fw_file, '/');
+ if (basename) {
+ basename++;
+ } else {
+ basename = rom->fw_file;
+ }
+ snprintf(fw_file_name, sizeof(fw_file_name), "%s/%s", rom->fw_dir,
+ basename);
+ fw_cfg_add_file(fw_cfg, fw_file_name, rom->data, rom->romsize);
+ }
return 0;
err:
--
1.7.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCHv4 13/15] Add bootindex for option roms.
2010-11-14 15:39 [Qemu-devel] [PATCHv4 00/15] boot order specification Gleb Natapov
` (11 preceding siblings ...)
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 12/15] Change fw_cfg_add_file() to get full file path as a parameter Gleb Natapov
@ 2010-11-14 15:39 ` Gleb Natapov
2010-11-14 21:33 ` [Qemu-devel] " Blue Swirl
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 14/15] Add notifier that will be called when machine is fully created Gleb Natapov
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 15/15] Pass boot device list to firmware Gleb Natapov
14 siblings, 1 reply; 55+ messages in thread
From: Gleb Natapov @ 2010-11-14 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kvm, mst, armbru, blauwirbel, alex.williamson, kevin
Extend -option-rom command to have additional parameter ,bootindex=.
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
hw/loader.c | 16 +++++++++++-----
hw/loader.h | 8 ++++----
hw/multiboot.c | 3 ++-
hw/ne2000.c | 2 +-
hw/nseries.c | 2 +-
hw/pc.c | 7 ++++---
hw/pci.c | 2 +-
hw/pcnet.c | 2 +-
qemu-config.c | 17 +++++++++++++++++
sysemu.h | 6 +++++-
vl.c | 11 +++++++++--
11 files changed, 56 insertions(+), 20 deletions(-)
diff --git a/hw/loader.c b/hw/loader.c
index 1e98326..eb198f6 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -107,7 +107,7 @@ int load_image_targphys(const char *filename,
size = get_image_size(filename);
if (size > 0)
- rom_add_file_fixed(filename, addr);
+ rom_add_file_fixed(filename, addr, -1);
return size;
}
@@ -557,10 +557,11 @@ static void rom_insert(Rom *rom)
}
int rom_add_file(const char *file, const char *fw_dir,
- target_phys_addr_t addr)
+ target_phys_addr_t addr, int32_t bootindex)
{
Rom *rom;
int rc, fd = -1;
+ char devpath[100];
rom = qemu_mallocz(sizeof(*rom));
rom->name = qemu_strdup(file);
@@ -605,7 +606,12 @@ int rom_add_file(const char *file, const char *fw_dir,
snprintf(fw_file_name, sizeof(fw_file_name), "%s/%s", rom->fw_dir,
basename);
fw_cfg_add_file(fw_cfg, fw_file_name, rom->data, rom->romsize);
+ snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
+ } else {
+ snprintf(devpath, sizeof(devpath), "/rom@" TARGET_FMT_plx, addr);
}
+
+ add_boot_device_path(bootindex, NULL, devpath);
return 0;
err:
@@ -635,12 +641,12 @@ int rom_add_blob(const char *name, const void *blob, size_t len,
int rom_add_vga(const char *file)
{
- return rom_add_file(file, "vgaroms", 0);
+ return rom_add_file(file, "vgaroms", 0, -1);
}
-int rom_add_option(const char *file)
+int rom_add_option(const char *file, int32_t bootindex)
{
- return rom_add_file(file, "genroms", 0);
+ return rom_add_file(file, "genroms", 0, bootindex);
}
static void rom_reset(void *unused)
diff --git a/hw/loader.h b/hw/loader.h
index 1f82fc5..fc6bdff 100644
--- a/hw/loader.h
+++ b/hw/loader.h
@@ -22,7 +22,7 @@ void pstrcpy_targphys(const char *name,
int rom_add_file(const char *file, const char *fw_dir,
- target_phys_addr_t addr);
+ target_phys_addr_t addr, int32_t bootindex);
int rom_add_blob(const char *name, const void *blob, size_t len,
target_phys_addr_t addr);
int rom_load_all(void);
@@ -31,8 +31,8 @@ int rom_copy(uint8_t *dest, target_phys_addr_t addr, size_t size);
void *rom_ptr(target_phys_addr_t addr);
void do_info_roms(Monitor *mon);
-#define rom_add_file_fixed(_f, _a) \
- rom_add_file(_f, NULL, _a)
+#define rom_add_file_fixed(_f, _a, _i) \
+ rom_add_file(_f, NULL, _a, _i)
#define rom_add_blob_fixed(_f, _b, _l, _a) \
rom_add_blob(_f, _b, _l, _a)
@@ -43,6 +43,6 @@ void do_info_roms(Monitor *mon);
#define PC_ROM_SIZE (PC_ROM_MAX - PC_ROM_MIN_VGA)
int rom_add_vga(const char *file);
-int rom_add_option(const char *file);
+int rom_add_option(const char *file, int32_t bootindex);
#endif
diff --git a/hw/multiboot.c b/hw/multiboot.c
index f9097a2..b438019 100644
--- a/hw/multiboot.c
+++ b/hw/multiboot.c
@@ -325,7 +325,8 @@ int load_multiboot(void *fw_cfg,
fw_cfg_add_bytes(fw_cfg, FW_CFG_INITRD_DATA, mb_bootinfo_data,
sizeof(bootinfo));
- option_rom[nb_option_roms] = "multiboot.bin";
+ option_rom[nb_option_roms].name = "multiboot.bin";
+ option_rom[nb_option_roms].bootindex = 0;
nb_option_roms++;
return 1; /* yes, we are multiboot */
diff --git a/hw/ne2000.c b/hw/ne2000.c
index f4bbac2..67e0cb0 100644
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -742,7 +742,7 @@ static int pci_ne2000_init(PCIDevice *pci_dev)
if (!pci_dev->qdev.hotplugged) {
static int loaded = 0;
if (!loaded) {
- rom_add_option("pxe-ne2k_pci.bin");
+ rom_add_option("pxe-ne2k_pci.bin", -1);
loaded = 1;
}
}
diff --git a/hw/nseries.c b/hw/nseries.c
index 04a028d..39780ef 100644
--- a/hw/nseries.c
+++ b/hw/nseries.c
@@ -1326,7 +1326,7 @@ static void n8x0_init(ram_addr_t ram_size, const char *boot_device,
qemu_register_reset(n8x0_boot_init, s);
}
- if (option_rom[0] && (boot_device[0] == 'n' || !kernel_filename)) {
+ if (option_rom[0].name && (boot_device[0] == 'n' || !kernel_filename)) {
int rom_size;
uint8_t nolo_tags[0x10000];
/* No, wait, better start at the ROM. */
diff --git a/hw/pc.c b/hw/pc.c
index 69b13bf..5111a76 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -733,7 +733,8 @@ static void load_linux(void *fw_cfg,
fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size);
fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA, setup, setup_size);
- option_rom[nb_option_roms] = "linuxboot.bin";
+ option_rom[nb_option_roms].name = "linuxboot.bin";
+ option_rom[nb_option_roms].bootindex = 0;
nb_option_roms++;
}
@@ -937,7 +938,7 @@ void pc_memory_init(ram_addr_t ram_size,
goto bios_error;
}
bios_offset = qemu_ram_alloc(NULL, "pc.bios", bios_size);
- ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size));
+ ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
if (ret != 0) {
bios_error:
fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
@@ -969,7 +970,7 @@ void pc_memory_init(ram_addr_t ram_size,
}
for (i = 0; i < nb_option_roms; i++) {
- rom_add_option(option_rom[i]);
+ rom_add_option(option_rom[i].name, option_rom[i].bootindex);
}
}
diff --git a/hw/pci.c b/hw/pci.c
index 114b435..49e9298 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1701,7 +1701,7 @@ static int pci_add_option_rom(PCIDevice *pdev)
if (class == 0x0300) {
rom_add_vga(pdev->romfile);
} else {
- rom_add_option(pdev->romfile);
+ rom_add_option(pdev->romfile, -1);
}
return 0;
}
diff --git a/hw/pcnet.c b/hw/pcnet.c
index a988f4f..38e6591 100644
--- a/hw/pcnet.c
+++ b/hw/pcnet.c
@@ -2014,7 +2014,7 @@ static int pci_pcnet_init(PCIDevice *pci_dev)
if (!pci_dev->qdev.hotplugged) {
static int loaded = 0;
if (!loaded) {
- rom_add_option("pxe-pcnet.bin");
+ rom_add_option("pxe-pcnet.bin", -1);
loaded = 1;
}
}
diff --git a/qemu-config.c b/qemu-config.c
index 52f18be..965fa46 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -429,6 +429,22 @@ QemuOptsList qemu_spice_opts = {
},
};
+QemuOptsList qemu_option_rom_opts = {
+ .name = "option-rom",
+ .implied_opt_name = "romfile",
+ .head = QTAILQ_HEAD_INITIALIZER(qemu_option_rom_opts.head),
+ .desc = {
+ {
+ .name = "bootindex",
+ .type = QEMU_OPT_NUMBER,
+ }, {
+ .name = "romfile",
+ .type = QEMU_OPT_STRING,
+ },
+ { /* end if list */ }
+ },
+};
+
static QemuOptsList *vm_config_groups[32] = {
&qemu_drive_opts,
&qemu_chardev_opts,
@@ -442,6 +458,7 @@ static QemuOptsList *vm_config_groups[32] = {
#ifdef CONFIG_SIMPLE_TRACE
&qemu_trace_opts,
#endif
+ &qemu_option_rom_opts,
NULL,
};
diff --git a/sysemu.h b/sysemu.h
index fe9a582..48f8eee 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -139,7 +139,11 @@ extern uint64_t node_mem[MAX_NODES];
extern uint64_t node_cpumask[MAX_NODES];
#define MAX_OPTION_ROMS 16
-extern const char *option_rom[MAX_OPTION_ROMS];
+typedef struct QEMUOptionRom {
+ const char *name;
+ int32_t bootindex;
+} QEMUOptionRom;
+extern QEMUOptionRom option_rom[MAX_OPTION_ROMS];
extern int nb_option_roms;
#define MAX_PROM_ENVS 128
diff --git a/vl.c b/vl.c
index 4c159f0..e8ada75 100644
--- a/vl.c
+++ b/vl.c
@@ -217,7 +217,7 @@ int cursor_hide = 1;
int graphic_rotate = 0;
uint8_t irq0override = 1;
const char *watchdog;
-const char *option_rom[MAX_OPTION_ROMS];
+QEMUOptionRom option_rom[MAX_OPTION_ROMS];
int nb_option_roms;
int semihosting_enabled = 0;
int old_param = 0;
@@ -2516,7 +2516,14 @@ int main(int argc, char **argv, char **envp)
fprintf(stderr, "Too many option ROMs\n");
exit(1);
}
- option_rom[nb_option_roms] = optarg;
+ opts = qemu_opts_parse(qemu_find_opts("option-rom"), optarg, 1);
+ option_rom[nb_option_roms].name = qemu_opt_get(opts, "romfile");
+ option_rom[nb_option_roms].bootindex =
+ qemu_opt_get_number(opts, "bootindex", -1);
+ if (!option_rom[nb_option_roms].name) {
+ fprintf(stderr, "Option ROM file is not specified\n");
+ exit(1);
+ }
nb_option_roms++;
break;
case QEMU_OPTION_semihosting:
--
1.7.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 13/15] Add bootindex for option roms.
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 13/15] Add bootindex for option roms Gleb Natapov
@ 2010-11-14 21:33 ` Blue Swirl
2010-11-15 10:18 ` Gleb Natapov
0 siblings, 1 reply; 55+ messages in thread
From: Blue Swirl @ 2010-11-14 21:33 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, mst, qemu-devel, armbru, alex.williamson, kevin
On Sun, Nov 14, 2010 at 3:39 PM, Gleb Natapov <gleb@redhat.com> wrote:
> Extend -option-rom command to have additional parameter ,bootindex=.
This patch is broken:
CC arm-softmmu/palm.o
/src/qemu/hw/palm.c: In function 'palmte_init':
/src/qemu/hw/palm.c:237: error: incompatible type for argument 1 of
'get_image_size'
/src/qemu/hw/palm.c:245: error: incompatible type for argument 1 of
'load_image_targphys'
cc1: warnings being treated as errors
/src/qemu/hw/palm.c:250: error: format '%s' expects type 'char *', but
argument 4 has type 'QEMUOptionRom'
CC arm-softmmu/nseries.o
/src/qemu/hw/nseries.c: In function 'n8x0_init':
/src/qemu/hw/nseries.c:1346: error: incompatible type for argument 1
of 'load_image_targphys'
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 13/15] Add bootindex for option roms.
2010-11-14 21:33 ` [Qemu-devel] " Blue Swirl
@ 2010-11-15 10:18 ` Gleb Natapov
0 siblings, 0 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-15 10:18 UTC (permalink / raw)
To: Blue Swirl; +Cc: kvm, mst, qemu-devel, armbru, alex.williamson, kevin
On Sun, Nov 14, 2010 at 09:33:06PM +0000, Blue Swirl wrote:
> On Sun, Nov 14, 2010 at 3:39 PM, Gleb Natapov <gleb@redhat.com> wrote:
> > Extend -option-rom command to have additional parameter ,bootindex=.
>
> This patch is broken:
> CC arm-softmmu/palm.o
> /src/qemu/hw/palm.c: In function 'palmte_init':
> /src/qemu/hw/palm.c:237: error: incompatible type for argument 1 of
> 'get_image_size'
> /src/qemu/hw/palm.c:245: error: incompatible type for argument 1 of
> 'load_image_targphys'
> cc1: warnings being treated as errors
> /src/qemu/hw/palm.c:250: error: format '%s' expects type 'char *', but
> argument 4 has type 'QEMUOptionRom'
> CC arm-softmmu/nseries.o
> /src/qemu/hw/nseries.c: In function 'n8x0_init':
> /src/qemu/hw/nseries.c:1346: error: incompatible type for argument 1
> of 'load_image_targphys'
Fixed in new version.
--
Gleb.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] [PATCHv4 14/15] Add notifier that will be called when machine is fully created.
2010-11-14 15:39 [Qemu-devel] [PATCHv4 00/15] boot order specification Gleb Natapov
` (12 preceding siblings ...)
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 13/15] Add bootindex for option roms Gleb Natapov
@ 2010-11-14 15:39 ` Gleb Natapov
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 15/15] Pass boot device list to firmware Gleb Natapov
14 siblings, 0 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-14 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kvm, mst, armbru, blauwirbel, alex.williamson, kevin
Action that depends on fully initialized device model should register
with this notifier chain.
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
sysemu.h | 2 ++
vl.c | 15 +++++++++++++++
2 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/sysemu.h b/sysemu.h
index 48f8eee..c42f33a 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -60,6 +60,8 @@ void qemu_system_reset(void);
void qemu_add_exit_notifier(Notifier *notify);
void qemu_remove_exit_notifier(Notifier *notify);
+void qemu_add_machine_init_done_notifier(Notifier *notify);
+
void do_savevm(Monitor *mon, const QDict *qdict);
int load_vmstate(const char *name);
void do_delvm(Monitor *mon, const QDict *qdict);
diff --git a/vl.c b/vl.c
index e8ada75..918d988 100644
--- a/vl.c
+++ b/vl.c
@@ -253,6 +253,9 @@ static void *boot_set_opaque;
static NotifierList exit_notifiers =
NOTIFIER_LIST_INITIALIZER(exit_notifiers);
+static NotifierList machine_init_done_notifiers =
+ NOTIFIER_LIST_INITIALIZER(machine_init_done_notifiers);
+
int kvm_allowed = 0;
uint32_t xen_domid;
enum xen_mode xen_mode = XEN_EMULATE;
@@ -1778,6 +1781,16 @@ static void qemu_run_exit_notifiers(void)
notifier_list_notify(&exit_notifiers);
}
+void qemu_add_machine_init_done_notifier(Notifier *notify)
+{
+ notifier_list_add(&machine_init_done_notifiers, notify);
+}
+
+static void qemu_run_machine_init_done_notifiers(void)
+{
+ notifier_list_notify(&machine_init_done_notifiers);
+}
+
static const QEMUOption *lookup_opt(int argc, char **argv,
const char **poptarg, int *poptind)
{
@@ -3023,6 +3036,8 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
+ qemu_run_machine_init_done_notifiers();
+
qemu_system_reset();
if (loadvm) {
if (load_vmstate(loadvm) < 0) {
--
1.7.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-14 15:39 [Qemu-devel] [PATCHv4 00/15] boot order specification Gleb Natapov
` (13 preceding siblings ...)
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 14/15] Add notifier that will be called when machine is fully created Gleb Natapov
@ 2010-11-14 15:39 ` Gleb Natapov
2010-11-14 18:41 ` [Qemu-devel] " Michael S. Tsirkin
` (3 more replies)
14 siblings, 4 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-14 15:39 UTC (permalink / raw)
To: qemu-devel; +Cc: kvm, mst, armbru, blauwirbel, alex.williamson, kevin
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
hw/fw_cfg.c | 14 ++++++++++++++
hw/fw_cfg.h | 4 +++-
sysemu.h | 1 +
vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 69 insertions(+), 1 deletions(-)
diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index 7b9434f..f6a67db 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -53,6 +53,7 @@ struct FWCfgState {
FWCfgFiles *files;
uint16_t cur_entry;
uint32_t cur_offset;
+ Notifier machine_ready;
};
static void fw_cfg_write(FWCfgState *s, uint8_t value)
@@ -315,6 +316,15 @@ int fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
return 1;
}
+static void fw_cfg_machine_ready(struct Notifier* n)
+{
+ uint32_t len;
+ char *bootindex = get_boot_devices_list(&len);
+
+ fw_cfg_add_bytes(container_of(n, FWCfgState, machine_ready),
+ FW_CFG_BOOTINDEX, (uint8_t*)bootindex, len);
+}
+
FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
target_phys_addr_t ctl_addr, target_phys_addr_t data_addr)
{
@@ -343,6 +353,10 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
+
+ s->machine_ready.notify = fw_cfg_machine_ready;
+ qemu_add_machine_init_done_notifier(&s->machine_ready);
+
return s;
}
diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
index 856bf91..4d61410 100644
--- a/hw/fw_cfg.h
+++ b/hw/fw_cfg.h
@@ -30,7 +30,9 @@
#define FW_CFG_FILE_FIRST 0x20
#define FW_CFG_FILE_SLOTS 0x10
-#define FW_CFG_MAX_ENTRY (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
+#define FW_CFG_FILE_LAST_SLOT (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
+#define FW_CFG_BOOTINDEX (FW_CFG_FILE_LAST_SLOT + 1)
+#define FW_CFG_MAX_ENTRY FW_CFG_BOOTINDEX
#define FW_CFG_WRITE_CHANNEL 0x4000
#define FW_CFG_ARCH_LOCAL 0x8000
diff --git a/sysemu.h b/sysemu.h
index c42f33a..38a20a3 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -196,4 +196,5 @@ void register_devices(void);
void add_boot_device_path(int32_t bootindex, DeviceState *dev,
const char *suffix);
+char *get_boot_devices_list(uint32_t *size);
#endif
diff --git a/vl.c b/vl.c
index 918d988..cca1e76 100644
--- a/vl.c
+++ b/vl.c
@@ -735,6 +735,57 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev,
QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
}
+/*
+ * This function returns device list as an array in a below format:
+ * +-----+-----+---------------+-----+---------------+--
+ * | n | l1 | devpath1 | l2 | devpath2 | ...
+ * +-----+-----+---------------+-----+---------------+--
+ * where:
+ * n - a number of devise pathes (one byte)
+ * l - length of following device path string (one byte)
+ * devpath - non-null terminated string of length l representing
+ * one device path
+ */
+char *get_boot_devices_list(uint32_t *size)
+{
+ FWBootEntry *i;
+ uint32_t total = 1, c = 0;
+ char *list = qemu_malloc(1);
+
+ QTAILQ_FOREACH(i, &fw_boot_order, link) {
+ char *devpath = NULL, *bootpath;
+ int len;
+
+ if (i->dev) {
+ devpath = qdev_get_fw_dev_path(i->dev);
+ assert(devpath);
+ }
+
+ if (i->suffix && devpath) {
+ bootpath = qemu_malloc(strlen(devpath) + strlen(i->suffix) + 2);
+ sprintf(bootpath, "%s/%s", devpath, i->suffix);
+ qemu_free(devpath);
+ } else if (devpath) {
+ bootpath = devpath;
+ } else {
+ bootpath = strdup(i->suffix);
+ }
+
+ len = strlen(bootpath);
+ list = qemu_realloc(list, total + len + 1);
+ list[total++] = len;
+ memcpy(&list[total], bootpath, len);
+ total += len;
+ c++;
+ qemu_free(bootpath);
+ }
+
+ list[0] = c;
+ *size = total;
+
+ return list;
+}
+
static void numa_add(const char *optarg)
{
char option[128];
--
1.7.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 15/15] Pass boot device list to firmware Gleb Natapov
@ 2010-11-14 18:41 ` Michael S. Tsirkin
2010-11-14 18:52 ` Gleb Natapov
2010-11-14 20:49 ` Blue Swirl
` (2 subsequent siblings)
3 siblings, 1 reply; 55+ messages in thread
From: Michael S. Tsirkin @ 2010-11-14 18:41 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, qemu-devel, armbru, blauwirbel, alex.williamson, kevin
On Sun, Nov 14, 2010 at 05:39:41PM +0200, Gleb Natapov wrote:
>
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
> hw/fw_cfg.c | 14 ++++++++++++++
> hw/fw_cfg.h | 4 +++-
> sysemu.h | 1 +
> vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 69 insertions(+), 1 deletions(-)
>
> diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
> index 7b9434f..f6a67db 100644
> --- a/hw/fw_cfg.c
> +++ b/hw/fw_cfg.c
> @@ -53,6 +53,7 @@ struct FWCfgState {
> FWCfgFiles *files;
> uint16_t cur_entry;
> uint32_t cur_offset;
> + Notifier machine_ready;
> };
>
> static void fw_cfg_write(FWCfgState *s, uint8_t value)
> @@ -315,6 +316,15 @@ int fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
> return 1;
> }
>
> +static void fw_cfg_machine_ready(struct Notifier* n)
> +{
> + uint32_t len;
> + char *bootindex = get_boot_devices_list(&len);
> +
> + fw_cfg_add_bytes(container_of(n, FWCfgState, machine_ready),
> + FW_CFG_BOOTINDEX, (uint8_t*)bootindex, len);
> +}
> +
> FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
> target_phys_addr_t ctl_addr, target_phys_addr_t data_addr)
> {
> @@ -343,6 +353,10 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
> fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
> fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
>
> +
> + s->machine_ready.notify = fw_cfg_machine_ready;
> + qemu_add_machine_init_done_notifier(&s->machine_ready);
> +
> return s;
> }
>
> diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
> index 856bf91..4d61410 100644
> --- a/hw/fw_cfg.h
> +++ b/hw/fw_cfg.h
> @@ -30,7 +30,9 @@
>
> #define FW_CFG_FILE_FIRST 0x20
> #define FW_CFG_FILE_SLOTS 0x10
> -#define FW_CFG_MAX_ENTRY (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
> +#define FW_CFG_FILE_LAST_SLOT (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
> +#define FW_CFG_BOOTINDEX (FW_CFG_FILE_LAST_SLOT + 1)
> +#define FW_CFG_MAX_ENTRY FW_CFG_BOOTINDEX
>
> #define FW_CFG_WRITE_CHANNEL 0x4000
> #define FW_CFG_ARCH_LOCAL 0x8000
> diff --git a/sysemu.h b/sysemu.h
> index c42f33a..38a20a3 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -196,4 +196,5 @@ void register_devices(void);
>
> void add_boot_device_path(int32_t bootindex, DeviceState *dev,
> const char *suffix);
> +char *get_boot_devices_list(uint32_t *size);
> #endif
> diff --git a/vl.c b/vl.c
> index 918d988..cca1e76 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -735,6 +735,57 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev,
> QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
> }
>
> +/*
> + * This function returns device list as an array in a below format:
> + * +-----+-----+---------------+-----+---------------+--
> + * | n | l1 | devpath1 | l2 | devpath2 | ...
> + * +-----+-----+---------------+-----+---------------+--
No one will ever want > 256 devices? Let's make it 4 byte or something.
> + * where:
> + * n - a number of devise pathes (one byte)
> + * l - length of following device path string (one byte)
Might not fit: with pci we can have 256 nested buses.
How about simply null-terminating each path?
> + * devpath - non-null terminated string of length l representing
> + * one device path
> + */
Document return value + parameters as well?
> +char *get_boot_devices_list(uint32_t *size)
> +{
> + FWBootEntry *i;
> + uint32_t total = 1, c = 0;
> + char *list = qemu_malloc(1);
> +
> + QTAILQ_FOREACH(i, &fw_boot_order, link) {
> + char *devpath = NULL, *bootpath;
> + int len;
> +
> + if (i->dev) {
> + devpath = qdev_get_fw_dev_path(i->dev);
> + assert(devpath);
> + }
> +
> + if (i->suffix && devpath) {
> + bootpath = qemu_malloc(strlen(devpath) + strlen(i->suffix) + 2);
> + sprintf(bootpath, "%s/%s", devpath, i->suffix);
> + qemu_free(devpath);
devpath is allocated with strdup, not qemu_malloc,
so I guess it should be freed with free?
Alternatively, let's add qemu_strdup
Might be a good idea: fix error handling here and elsewhere.
> + } else if (devpath) {
> + bootpath = devpath;
> + } else {
> + bootpath = strdup(i->suffix);
> + }
assert(bootpath).
> +
> + len = strlen(bootpath);
> + list = qemu_realloc(list, total + len + 1);
> + list[total++] = len;
> + memcpy(&list[total], bootpath, len);
> + total += len;
> + c++;
> + qemu_free(bootpath);
Man, is this tricky.
> + }
> +
> + list[0] = c;
> + *size = total;
> +
> + return list;
> +}
> +
> static void numa_add(const char *optarg)
> {
> char option[128];
> --
> 1.7.1
^ permalink raw reply [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-14 18:41 ` [Qemu-devel] " Michael S. Tsirkin
@ 2010-11-14 18:52 ` Gleb Natapov
2010-11-14 20:56 ` Michael S. Tsirkin
2010-11-14 20:57 ` Michael S. Tsirkin
0 siblings, 2 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-14 18:52 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, qemu-devel, armbru, blauwirbel, alex.williamson, kevin
On Sun, Nov 14, 2010 at 08:41:37PM +0200, Michael S. Tsirkin wrote:
> On Sun, Nov 14, 2010 at 05:39:41PM +0200, Gleb Natapov wrote:
> >
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > ---
> > hw/fw_cfg.c | 14 ++++++++++++++
> > hw/fw_cfg.h | 4 +++-
> > sysemu.h | 1 +
> > vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 69 insertions(+), 1 deletions(-)
> >
> > diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
> > index 7b9434f..f6a67db 100644
> > --- a/hw/fw_cfg.c
> > +++ b/hw/fw_cfg.c
> > @@ -53,6 +53,7 @@ struct FWCfgState {
> > FWCfgFiles *files;
> > uint16_t cur_entry;
> > uint32_t cur_offset;
> > + Notifier machine_ready;
> > };
> >
> > static void fw_cfg_write(FWCfgState *s, uint8_t value)
> > @@ -315,6 +316,15 @@ int fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
> > return 1;
> > }
> >
> > +static void fw_cfg_machine_ready(struct Notifier* n)
> > +{
> > + uint32_t len;
> > + char *bootindex = get_boot_devices_list(&len);
> > +
> > + fw_cfg_add_bytes(container_of(n, FWCfgState, machine_ready),
> > + FW_CFG_BOOTINDEX, (uint8_t*)bootindex, len);
> > +}
> > +
> > FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
> > target_phys_addr_t ctl_addr, target_phys_addr_t data_addr)
> > {
> > @@ -343,6 +353,10 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
> > fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
> > fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
> >
> > +
> > + s->machine_ready.notify = fw_cfg_machine_ready;
> > + qemu_add_machine_init_done_notifier(&s->machine_ready);
> > +
> > return s;
> > }
> >
> > diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
> > index 856bf91..4d61410 100644
> > --- a/hw/fw_cfg.h
> > +++ b/hw/fw_cfg.h
> > @@ -30,7 +30,9 @@
> >
> > #define FW_CFG_FILE_FIRST 0x20
> > #define FW_CFG_FILE_SLOTS 0x10
> > -#define FW_CFG_MAX_ENTRY (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
> > +#define FW_CFG_FILE_LAST_SLOT (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
> > +#define FW_CFG_BOOTINDEX (FW_CFG_FILE_LAST_SLOT + 1)
> > +#define FW_CFG_MAX_ENTRY FW_CFG_BOOTINDEX
> >
> > #define FW_CFG_WRITE_CHANNEL 0x4000
> > #define FW_CFG_ARCH_LOCAL 0x8000
> > diff --git a/sysemu.h b/sysemu.h
> > index c42f33a..38a20a3 100644
> > --- a/sysemu.h
> > +++ b/sysemu.h
> > @@ -196,4 +196,5 @@ void register_devices(void);
> >
> > void add_boot_device_path(int32_t bootindex, DeviceState *dev,
> > const char *suffix);
> > +char *get_boot_devices_list(uint32_t *size);
> > #endif
> > diff --git a/vl.c b/vl.c
> > index 918d988..cca1e76 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -735,6 +735,57 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev,
> > QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
> > }
> >
> > +/*
> > + * This function returns device list as an array in a below format:
> > + * +-----+-----+---------------+-----+---------------+--
> > + * | n | l1 | devpath1 | l2 | devpath2 | ...
> > + * +-----+-----+---------------+-----+---------------+--
>
> No one will ever want > 256 devices? Let's make it 4 byte or something.
>
More then 256 _boot_ devices. I think this is more then reasonable.
> > + * where:
> > + * n - a number of devise pathes (one byte)
> > + * l - length of following device path string (one byte)
>
> Might not fit: with pci we can have 256 nested buses.
Theoretically. But will it practically happen?
> How about simply null-terminating each path?
>
Will be harder for Seabios. I can use more then one byte for length, but
I tried to avoid endianess handling.
> > + * devpath - non-null terminated string of length l representing
> > + * one device path
> > + */
>
> Document return value + parameters as well?
>
Return value is documented above :)
> > +char *get_boot_devices_list(uint32_t *size)
> > +{
> > + FWBootEntry *i;
> > + uint32_t total = 1, c = 0;
> > + char *list = qemu_malloc(1);
> > +
> > + QTAILQ_FOREACH(i, &fw_boot_order, link) {
> > + char *devpath = NULL, *bootpath;
> > + int len;
> > +
> > + if (i->dev) {
> > + devpath = qdev_get_fw_dev_path(i->dev);
> > + assert(devpath);
> > + }
> > +
> > + if (i->suffix && devpath) {
> > + bootpath = qemu_malloc(strlen(devpath) + strlen(i->suffix) + 2);
> > + sprintf(bootpath, "%s/%s", devpath, i->suffix);
> > + qemu_free(devpath);
>
> devpath is allocated with strdup, not qemu_malloc,
> so I guess it should be freed with free?
There is code that do it like this all over qemu.
> Alternatively, let's add qemu_strdup
> Might be a good idea: fix error handling here and elsewhere.
>
> > + } else if (devpath) {
> > + bootpath = devpath;
> > + } else {
> > + bootpath = strdup(i->suffix);
> > + }
>
> assert(bootpath).
>
OK
> > +
> > + len = strlen(bootpath);
> > + list = qemu_realloc(list, total + len + 1);
> > + list[total++] = len;
> > + memcpy(&list[total], bootpath, len);
> > + total += len;
> > + c++;
> > + qemu_free(bootpath);
>
> Man, is this tricky.
>
Nah, not at all.
--
Gleb.
^ permalink raw reply [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-14 18:52 ` Gleb Natapov
@ 2010-11-14 20:56 ` Michael S. Tsirkin
2010-11-14 20:57 ` Michael S. Tsirkin
1 sibling, 0 replies; 55+ messages in thread
From: Michael S. Tsirkin @ 2010-11-14 20:56 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, qemu-devel, armbru, blauwirbel, alex.williamson, kevin
On Sun, Nov 14, 2010 at 08:52:37PM +0200, Gleb Natapov wrote:
> On Sun, Nov 14, 2010 at 08:41:37PM +0200, Michael S. Tsirkin wrote:
> > On Sun, Nov 14, 2010 at 05:39:41PM +0200, Gleb Natapov wrote:
> > >
> > > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > > ---
> > > hw/fw_cfg.c | 14 ++++++++++++++
> > > hw/fw_cfg.h | 4 +++-
> > > sysemu.h | 1 +
> > > vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 69 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
> > > index 7b9434f..f6a67db 100644
> > > --- a/hw/fw_cfg.c
> > > +++ b/hw/fw_cfg.c
> > > @@ -53,6 +53,7 @@ struct FWCfgState {
> > > FWCfgFiles *files;
> > > uint16_t cur_entry;
> > > uint32_t cur_offset;
> > > + Notifier machine_ready;
> > > };
> > >
> > > static void fw_cfg_write(FWCfgState *s, uint8_t value)
> > > @@ -315,6 +316,15 @@ int fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
> > > return 1;
> > > }
> > >
> > > +static void fw_cfg_machine_ready(struct Notifier* n)
> > > +{
> > > + uint32_t len;
> > > + char *bootindex = get_boot_devices_list(&len);
> > > +
> > > + fw_cfg_add_bytes(container_of(n, FWCfgState, machine_ready),
> > > + FW_CFG_BOOTINDEX, (uint8_t*)bootindex, len);
> > > +}
> > > +
> > > FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
> > > target_phys_addr_t ctl_addr, target_phys_addr_t data_addr)
> > > {
> > > @@ -343,6 +353,10 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
> > > fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
> > > fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
> > >
> > > +
> > > + s->machine_ready.notify = fw_cfg_machine_ready;
> > > + qemu_add_machine_init_done_notifier(&s->machine_ready);
> > > +
> > > return s;
> > > }
> > >
> > > diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
> > > index 856bf91..4d61410 100644
> > > --- a/hw/fw_cfg.h
> > > +++ b/hw/fw_cfg.h
> > > @@ -30,7 +30,9 @@
> > >
> > > #define FW_CFG_FILE_FIRST 0x20
> > > #define FW_CFG_FILE_SLOTS 0x10
> > > -#define FW_CFG_MAX_ENTRY (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
> > > +#define FW_CFG_FILE_LAST_SLOT (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
> > > +#define FW_CFG_BOOTINDEX (FW_CFG_FILE_LAST_SLOT + 1)
> > > +#define FW_CFG_MAX_ENTRY FW_CFG_BOOTINDEX
> > >
> > > #define FW_CFG_WRITE_CHANNEL 0x4000
> > > #define FW_CFG_ARCH_LOCAL 0x8000
> > > diff --git a/sysemu.h b/sysemu.h
> > > index c42f33a..38a20a3 100644
> > > --- a/sysemu.h
> > > +++ b/sysemu.h
> > > @@ -196,4 +196,5 @@ void register_devices(void);
> > >
> > > void add_boot_device_path(int32_t bootindex, DeviceState *dev,
> > > const char *suffix);
> > > +char *get_boot_devices_list(uint32_t *size);
> > > #endif
> > > diff --git a/vl.c b/vl.c
> > > index 918d988..cca1e76 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -735,6 +735,57 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev,
> > > QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
> > > }
> > >
> > > +/*
> > > + * This function returns device list as an array in a below format:
> > > + * +-----+-----+---------------+-----+---------------+--
> > > + * | n | l1 | devpath1 | l2 | devpath2 | ...
> > > + * +-----+-----+---------------+-----+---------------+--
> >
> > No one will ever want > 256 devices? Let's make it 4 byte or something.
> >
> More then 256 _boot_ devices. I think this is more then reasonable.
>
> > > + * where:
> > > + * n - a number of devise pathes (one byte)
> > > + * l - length of following device path string (one byte)
> >
> > Might not fit: with pci we can have 256 nested buses.
> Theoretically. But will it practically happen?
Why not? It's easy to specify this on qemu command line.
You do nothing to detect this and gracefully fail either, do you?
--
MST
^ permalink raw reply [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-14 18:52 ` Gleb Natapov
2010-11-14 20:56 ` Michael S. Tsirkin
@ 2010-11-14 20:57 ` Michael S. Tsirkin
1 sibling, 0 replies; 55+ messages in thread
From: Michael S. Tsirkin @ 2010-11-14 20:57 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, qemu-devel, armbru, blauwirbel, alex.williamson, kevin
On Sun, Nov 14, 2010 at 08:52:37PM +0200, Gleb Natapov wrote:
> > > +
> > > + len = strlen(bootpath);
> > > + list = qemu_realloc(list, total + len + 1);
> > > + list[total++] = len;
> > > + memcpy(&list[total], bootpath, len);
> > > + total += len;
> > > + c++;
> > > + qemu_free(bootpath);
> >
> > Man, is this tricky.
> >
> Nah, not at all.
I think it will be easier if we don't try to do this in one pass.
1. pass: calculate total length and # of devices
2. allocate
2. pass fill in
> --
> Gleb.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 15/15] Pass boot device list to firmware Gleb Natapov
2010-11-14 18:41 ` [Qemu-devel] " Michael S. Tsirkin
@ 2010-11-14 20:49 ` Blue Swirl
2010-11-14 20:54 ` Michael S. Tsirkin
2010-11-14 22:50 ` Blue Swirl
2010-11-15 3:40 ` Kevin O'Connor
3 siblings, 1 reply; 55+ messages in thread
From: Blue Swirl @ 2010-11-14 20:49 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, mst, qemu-devel, armbru, alex.williamson, kevin
On Sun, Nov 14, 2010 at 3:39 PM, Gleb Natapov <gleb@redhat.com> wrote:
>
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
> hw/fw_cfg.c | 14 ++++++++++++++
> hw/fw_cfg.h | 4 +++-
> sysemu.h | 1 +
> vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 69 insertions(+), 1 deletions(-)
>
> diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
> index 7b9434f..f6a67db 100644
> --- a/hw/fw_cfg.c
> +++ b/hw/fw_cfg.c
> @@ -53,6 +53,7 @@ struct FWCfgState {
> FWCfgFiles *files;
> uint16_t cur_entry;
> uint32_t cur_offset;
> + Notifier machine_ready;
> };
>
> static void fw_cfg_write(FWCfgState *s, uint8_t value)
> @@ -315,6 +316,15 @@ int fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
> return 1;
> }
>
> +static void fw_cfg_machine_ready(struct Notifier* n)
> +{
> + uint32_t len;
> + char *bootindex = get_boot_devices_list(&len);
> +
> + fw_cfg_add_bytes(container_of(n, FWCfgState, machine_ready),
> + FW_CFG_BOOTINDEX, (uint8_t*)bootindex, len);
I started to implement this to OpenBIOS but I noticed a small issue.
First the first byte must be read to determine length. Then the read
routine will be called again to read the correct amount of bytes. This
would work, but since there is no shortage of IDs, I'd prefer a system
where one ID is used to query the length and another ID is used to
read the data, without the length byte. This is similar how command
line, initrd etc. are handled.
This would have the advantage that since fw_cfg uses little endian
format, the length value would easily scale to for example 64 bits to
support terabytes of boot device lists. ;-)
^ permalink raw reply [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-14 20:49 ` Blue Swirl
@ 2010-11-14 20:54 ` Michael S. Tsirkin
2010-11-14 21:13 ` Blue Swirl
0 siblings, 1 reply; 55+ messages in thread
From: Michael S. Tsirkin @ 2010-11-14 20:54 UTC (permalink / raw)
To: Blue Swirl; +Cc: kvm, Gleb Natapov, armbru, qemu-devel, alex.williamson, kevin
On Sun, Nov 14, 2010 at 08:49:59PM +0000, Blue Swirl wrote:
> On Sun, Nov 14, 2010 at 3:39 PM, Gleb Natapov <gleb@redhat.com> wrote:
> >
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > ---
> > hw/fw_cfg.c | 14 ++++++++++++++
> > hw/fw_cfg.h | 4 +++-
> > sysemu.h | 1 +
> > vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 69 insertions(+), 1 deletions(-)
> >
> > diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
> > index 7b9434f..f6a67db 100644
> > --- a/hw/fw_cfg.c
> > +++ b/hw/fw_cfg.c
> > @@ -53,6 +53,7 @@ struct FWCfgState {
> > FWCfgFiles *files;
> > uint16_t cur_entry;
> > uint32_t cur_offset;
> > + Notifier machine_ready;
> > };
> >
> > static void fw_cfg_write(FWCfgState *s, uint8_t value)
> > @@ -315,6 +316,15 @@ int fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
> > return 1;
> > }
> >
> > +static void fw_cfg_machine_ready(struct Notifier* n)
> > +{
> > + uint32_t len;
> > + char *bootindex = get_boot_devices_list(&len);
> > +
> > + fw_cfg_add_bytes(container_of(n, FWCfgState, machine_ready),
> > + FW_CFG_BOOTINDEX, (uint8_t*)bootindex, len);
>
> I started to implement this to OpenBIOS but I noticed a small issue.
> First the first byte must be read to determine length. Then the read
> routine will be called again to read the correct amount of bytes. This
> would work, but since there is no shortage of IDs, I'd prefer a system
> where one ID is used to query the length and another ID is used to
> read the data, without the length byte. This is similar how command
> line, initrd etc. are handled.
>
> This would have the advantage that since fw_cfg uses little endian
> format, the length value would easily scale to for example 64 bits to
> support terabytes of boot device lists. ;-)
Yea. Let's just print # of devices as a property, in ASCII.
No endian-ness, no nothing.
Also - can we just NULL-terminate each ID?
^ permalink raw reply [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-14 20:54 ` Michael S. Tsirkin
@ 2010-11-14 21:13 ` Blue Swirl
2010-11-14 21:45 ` Michael S. Tsirkin
0 siblings, 1 reply; 55+ messages in thread
From: Blue Swirl @ 2010-11-14 21:13 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, Gleb Natapov, armbru, qemu-devel, alex.williamson, kevin
On Sun, Nov 14, 2010 at 8:54 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Nov 14, 2010 at 08:49:59PM +0000, Blue Swirl wrote:
>> On Sun, Nov 14, 2010 at 3:39 PM, Gleb Natapov <gleb@redhat.com> wrote:
>> >
>> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
>> > ---
>> > hw/fw_cfg.c | 14 ++++++++++++++
>> > hw/fw_cfg.h | 4 +++-
>> > sysemu.h | 1 +
>> > vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> > 4 files changed, 69 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
>> > index 7b9434f..f6a67db 100644
>> > --- a/hw/fw_cfg.c
>> > +++ b/hw/fw_cfg.c
>> > @@ -53,6 +53,7 @@ struct FWCfgState {
>> > FWCfgFiles *files;
>> > uint16_t cur_entry;
>> > uint32_t cur_offset;
>> > + Notifier machine_ready;
>> > };
>> >
>> > static void fw_cfg_write(FWCfgState *s, uint8_t value)
>> > @@ -315,6 +316,15 @@ int fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
>> > return 1;
>> > }
>> >
>> > +static void fw_cfg_machine_ready(struct Notifier* n)
>> > +{
>> > + uint32_t len;
>> > + char *bootindex = get_boot_devices_list(&len);
>> > +
>> > + fw_cfg_add_bytes(container_of(n, FWCfgState, machine_ready),
>> > + FW_CFG_BOOTINDEX, (uint8_t*)bootindex, len);
>>
>> I started to implement this to OpenBIOS but I noticed a small issue.
>> First the first byte must be read to determine length. Then the read
>> routine will be called again to read the correct amount of bytes. This
>> would work, but since there is no shortage of IDs, I'd prefer a system
>> where one ID is used to query the length and another ID is used to
>> read the data, without the length byte. This is similar how command
>> line, initrd etc. are handled.
>>
>> This would have the advantage that since fw_cfg uses little endian
>> format, the length value would easily scale to for example 64 bits to
>> support terabytes of boot device lists. ;-)
>
> Yea. Let's just print # of devices as a property, in ASCII.
> No endian-ness, no nothing.
> Also - can we just NULL-terminate each ID?
No, we should use LE numbers like other IDs. To be more specific, this
is what I meant (instead of FW_CFG_BOOTINDEX):
FW_CFG_BOOTINDEX_LEN: get LE integer length of the boot device data.
FW_CFG_BOOTINDEX_DATA: get the boot device data as NUL terminated C
strings, all strings back-to-back. The reader can determine number of
strings.
^ permalink raw reply [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-14 21:13 ` Blue Swirl
@ 2010-11-14 21:45 ` Michael S. Tsirkin
0 siblings, 0 replies; 55+ messages in thread
From: Michael S. Tsirkin @ 2010-11-14 21:45 UTC (permalink / raw)
To: Blue Swirl; +Cc: kvm, Gleb Natapov, armbru, qemu-devel, alex.williamson, kevin
On Sun, Nov 14, 2010 at 09:13:03PM +0000, Blue Swirl wrote:
> On Sun, Nov 14, 2010 at 8:54 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Nov 14, 2010 at 08:49:59PM +0000, Blue Swirl wrote:
> >> On Sun, Nov 14, 2010 at 3:39 PM, Gleb Natapov <gleb@redhat.com> wrote:
> >> >
> >> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> >> > ---
> >> > hw/fw_cfg.c | 14 ++++++++++++++
> >> > hw/fw_cfg.h | 4 +++-
> >> > sysemu.h | 1 +
> >> > vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >> > 4 files changed, 69 insertions(+), 1 deletions(-)
> >> >
> >> > diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
> >> > index 7b9434f..f6a67db 100644
> >> > --- a/hw/fw_cfg.c
> >> > +++ b/hw/fw_cfg.c
> >> > @@ -53,6 +53,7 @@ struct FWCfgState {
> >> > FWCfgFiles *files;
> >> > uint16_t cur_entry;
> >> > uint32_t cur_offset;
> >> > + Notifier machine_ready;
> >> > };
> >> >
> >> > static void fw_cfg_write(FWCfgState *s, uint8_t value)
> >> > @@ -315,6 +316,15 @@ int fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
> >> > return 1;
> >> > }
> >> >
> >> > +static void fw_cfg_machine_ready(struct Notifier* n)
> >> > +{
> >> > + uint32_t len;
> >> > + char *bootindex = get_boot_devices_list(&len);
> >> > +
> >> > + fw_cfg_add_bytes(container_of(n, FWCfgState, machine_ready),
> >> > + FW_CFG_BOOTINDEX, (uint8_t*)bootindex, len);
> >>
> >> I started to implement this to OpenBIOS but I noticed a small issue.
> >> First the first byte must be read to determine length. Then the read
> >> routine will be called again to read the correct amount of bytes. This
> >> would work, but since there is no shortage of IDs, I'd prefer a system
> >> where one ID is used to query the length and another ID is used to
> >> read the data, without the length byte. This is similar how command
> >> line, initrd etc. are handled.
> >>
> >> This would have the advantage that since fw_cfg uses little endian
> >> format, the length value would easily scale to for example 64 bits to
> >> support terabytes of boot device lists. ;-)
> >
> > Yea. Let's just print # of devices as a property, in ASCII.
> > No endian-ness, no nothing.
> > Also - can we just NULL-terminate each ID?
>
> No, we should use LE numbers like other IDs. To be more specific, this
> is what I meant (instead of FW_CFG_BOOTINDEX):
> FW_CFG_BOOTINDEX_LEN: get LE integer length of the boot device data.
> FW_CFG_BOOTINDEX_DATA: get the boot device data as NUL terminated C
> strings, all strings back-to-back. The reader can determine number of
> strings.
Sounds good.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 15/15] Pass boot device list to firmware Gleb Natapov
2010-11-14 18:41 ` [Qemu-devel] " Michael S. Tsirkin
2010-11-14 20:49 ` Blue Swirl
@ 2010-11-14 22:50 ` Blue Swirl
2010-11-15 8:42 ` Gleb Natapov
2010-11-15 3:40 ` Kevin O'Connor
3 siblings, 1 reply; 55+ messages in thread
From: Blue Swirl @ 2010-11-14 22:50 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, mst, qemu-devel, armbru, alex.williamson, kevin
On Sun, Nov 14, 2010 at 3:39 PM, Gleb Natapov <gleb@redhat.com> wrote:
>
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
> hw/fw_cfg.c | 14 ++++++++++++++
> hw/fw_cfg.h | 4 +++-
> sysemu.h | 1 +
> vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 69 insertions(+), 1 deletions(-)
>
> diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
> index 7b9434f..f6a67db 100644
> --- a/hw/fw_cfg.c
> +++ b/hw/fw_cfg.c
> @@ -53,6 +53,7 @@ struct FWCfgState {
> FWCfgFiles *files;
> uint16_t cur_entry;
> uint32_t cur_offset;
> + Notifier machine_ready;
> };
>
> static void fw_cfg_write(FWCfgState *s, uint8_t value)
> @@ -315,6 +316,15 @@ int fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
> return 1;
> }
>
> +static void fw_cfg_machine_ready(struct Notifier* n)
> +{
> + uint32_t len;
> + char *bootindex = get_boot_devices_list(&len);
> +
> + fw_cfg_add_bytes(container_of(n, FWCfgState, machine_ready),
> + FW_CFG_BOOTINDEX, (uint8_t*)bootindex, len);
> +}
> +
> FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
> target_phys_addr_t ctl_addr, target_phys_addr_t data_addr)
> {
> @@ -343,6 +353,10 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
> fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
> fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
>
> +
> + s->machine_ready.notify = fw_cfg_machine_ready;
> + qemu_add_machine_init_done_notifier(&s->machine_ready);
> +
> return s;
> }
>
> diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
> index 856bf91..4d61410 100644
> --- a/hw/fw_cfg.h
> +++ b/hw/fw_cfg.h
> @@ -30,7 +30,9 @@
>
> #define FW_CFG_FILE_FIRST 0x20
> #define FW_CFG_FILE_SLOTS 0x10
> -#define FW_CFG_MAX_ENTRY (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
> +#define FW_CFG_FILE_LAST_SLOT (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
> +#define FW_CFG_BOOTINDEX (FW_CFG_FILE_LAST_SLOT + 1)
> +#define FW_CFG_MAX_ENTRY FW_CFG_BOOTINDEX
This should be
#define FW_CFG_MAX_ENTRY (FW_CFG_BOOTINDEX + 1)
because the check is like this:
if ((key & FW_CFG_ENTRY_MASK) >= FW_CFG_MAX_ENTRY) {
s->cur_entry = FW_CFG_INVALID;
With that change, I got the bootindex passed to OpenBIOS:
OpenBIOS for Sparc64
Configuration device id QEMU version 1 machine id 0
kernel cmdline
CPUs: 1 x SUNW,UltraSPARC-IIi
UUID: 00000000-0000-0000-0000-000000000000
bootindex num_strings 1
bootindex /pbm@000001fe00000000/ide@5/drive@1/disk@0
The device path does not match exactly, but it's close:
/pci@1fe,0/pci-ata@5/ide1@600/disk@0
^ permalink raw reply [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-14 22:50 ` Blue Swirl
@ 2010-11-15 8:42 ` Gleb Natapov
2010-11-15 20:29 ` Blue Swirl
0 siblings, 1 reply; 55+ messages in thread
From: Gleb Natapov @ 2010-11-15 8:42 UTC (permalink / raw)
To: Blue Swirl; +Cc: kvm, mst, qemu-devel, armbru, alex.williamson, kevin
On Sun, Nov 14, 2010 at 10:50:13PM +0000, Blue Swirl wrote:
> On Sun, Nov 14, 2010 at 3:39 PM, Gleb Natapov <gleb@redhat.com> wrote:
> >
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > ---
> > hw/fw_cfg.c | 14 ++++++++++++++
> > hw/fw_cfg.h | 4 +++-
> > sysemu.h | 1 +
> > vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 69 insertions(+), 1 deletions(-)
> >
> > diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
> > index 7b9434f..f6a67db 100644
> > --- a/hw/fw_cfg.c
> > +++ b/hw/fw_cfg.c
> > @@ -53,6 +53,7 @@ struct FWCfgState {
> > FWCfgFiles *files;
> > uint16_t cur_entry;
> > uint32_t cur_offset;
> > + Notifier machine_ready;
> > };
> >
> > static void fw_cfg_write(FWCfgState *s, uint8_t value)
> > @@ -315,6 +316,15 @@ int fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
> > return 1;
> > }
> >
> > +static void fw_cfg_machine_ready(struct Notifier* n)
> > +{
> > + uint32_t len;
> > + char *bootindex = get_boot_devices_list(&len);
> > +
> > + fw_cfg_add_bytes(container_of(n, FWCfgState, machine_ready),
> > + FW_CFG_BOOTINDEX, (uint8_t*)bootindex, len);
> > +}
> > +
> > FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
> > target_phys_addr_t ctl_addr, target_phys_addr_t data_addr)
> > {
> > @@ -343,6 +353,10 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
> > fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
> > fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
> >
> > +
> > + s->machine_ready.notify = fw_cfg_machine_ready;
> > + qemu_add_machine_init_done_notifier(&s->machine_ready);
> > +
> > return s;
> > }
> >
> > diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
> > index 856bf91..4d61410 100644
> > --- a/hw/fw_cfg.h
> > +++ b/hw/fw_cfg.h
> > @@ -30,7 +30,9 @@
> >
> > #define FW_CFG_FILE_FIRST 0x20
> > #define FW_CFG_FILE_SLOTS 0x10
> > -#define FW_CFG_MAX_ENTRY (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
> > +#define FW_CFG_FILE_LAST_SLOT (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
> > +#define FW_CFG_BOOTINDEX (FW_CFG_FILE_LAST_SLOT + 1)
> > +#define FW_CFG_MAX_ENTRY FW_CFG_BOOTINDEX
>
> This should be
> #define FW_CFG_MAX_ENTRY (FW_CFG_BOOTINDEX + 1)
> because the check is like this:
> if ((key & FW_CFG_ENTRY_MASK) >= FW_CFG_MAX_ENTRY) {
> s->cur_entry = FW_CFG_INVALID;
>
Yeah, will fix.
> With that change, I got the bootindex passed to OpenBIOS:
> OpenBIOS for Sparc64
> Configuration device id QEMU version 1 machine id 0
> kernel cmdline
> CPUs: 1 x SUNW,UltraSPARC-IIi
> UUID: 00000000-0000-0000-0000-000000000000
> bootindex num_strings 1
> bootindex /pbm@000001fe00000000/ide@5/drive@1/disk@0
>
> The device path does not match exactly, but it's close:
> /pci@1fe,0/pci-ata@5/ide1@600/disk@0
pbm->pci should be solvable by the patch at the end. Were in the spec
it is allowed to abbreviate 1fe00000000 as 1fe,0? Spec allows to drop
starting zeroes but TARGET_FMT_plx definition in targphys.h has 0 after
%. I can define another one without leading zeroes. Can you suggest
a name? TARGET_FMT_lx is poisoned. As of ATA there is no open firmware
binding spec for ATA, so everyone does what he pleases. I based my
implementation on what open firmware showing when running on qemu x86.
"pci-ata" should be "ide" according to PCI binding spec :)
diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index c619112..643aa49 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -453,6 +453,7 @@ static PCIDeviceInfo pbm_pci_host_info = {
static SysBusDeviceInfo pbm_host_info = {
.qdev.name = "pbm",
+ .qdev.fw_name = "pci",
.qdev.size = sizeof(APBState),
.qdev.reset = pci_pbm_reset,
.init = pci_pbm_init_device,
--
Gleb.
^ permalink raw reply related [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-15 8:42 ` Gleb Natapov
@ 2010-11-15 20:29 ` Blue Swirl
2010-11-16 14:11 ` Gleb Natapov
0 siblings, 1 reply; 55+ messages in thread
From: Blue Swirl @ 2010-11-15 20:29 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, mst, qemu-devel, armbru, alex.williamson, kevin
2010/11/15 Gleb Natapov <gleb@redhat.com>:
> On Sun, Nov 14, 2010 at 10:50:13PM +0000, Blue Swirl wrote:
>> On Sun, Nov 14, 2010 at 3:39 PM, Gleb Natapov <gleb@redhat.com> wrote:
>> >
>> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
>> > ---
>> > hw/fw_cfg.c | 14 ++++++++++++++
>> > hw/fw_cfg.h | 4 +++-
>> > sysemu.h | 1 +
>> > vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> > 4 files changed, 69 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
>> > index 7b9434f..f6a67db 100644
>> > --- a/hw/fw_cfg.c
>> > +++ b/hw/fw_cfg.c
>> > @@ -53,6 +53,7 @@ struct FWCfgState {
>> > FWCfgFiles *files;
>> > uint16_t cur_entry;
>> > uint32_t cur_offset;
>> > + Notifier machine_ready;
>> > };
>> >
>> > static void fw_cfg_write(FWCfgState *s, uint8_t value)
>> > @@ -315,6 +316,15 @@ int fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
>> > return 1;
>> > }
>> >
>> > +static void fw_cfg_machine_ready(struct Notifier* n)
>> > +{
>> > + uint32_t len;
>> > + char *bootindex = get_boot_devices_list(&len);
>> > +
>> > + fw_cfg_add_bytes(container_of(n, FWCfgState, machine_ready),
>> > + FW_CFG_BOOTINDEX, (uint8_t*)bootindex, len);
>> > +}
>> > +
>> > FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
>> > target_phys_addr_t ctl_addr, target_phys_addr_t data_addr)
>> > {
>> > @@ -343,6 +353,10 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
>> > fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
>> > fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
>> >
>> > +
>> > + s->machine_ready.notify = fw_cfg_machine_ready;
>> > + qemu_add_machine_init_done_notifier(&s->machine_ready);
>> > +
>> > return s;
>> > }
>> >
>> > diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
>> > index 856bf91..4d61410 100644
>> > --- a/hw/fw_cfg.h
>> > +++ b/hw/fw_cfg.h
>> > @@ -30,7 +30,9 @@
>> >
>> > #define FW_CFG_FILE_FIRST 0x20
>> > #define FW_CFG_FILE_SLOTS 0x10
>> > -#define FW_CFG_MAX_ENTRY (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
>> > +#define FW_CFG_FILE_LAST_SLOT (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
>> > +#define FW_CFG_BOOTINDEX (FW_CFG_FILE_LAST_SLOT + 1)
>> > +#define FW_CFG_MAX_ENTRY FW_CFG_BOOTINDEX
>>
>> This should be
>> #define FW_CFG_MAX_ENTRY (FW_CFG_BOOTINDEX + 1)
>> because the check is like this:
>> if ((key & FW_CFG_ENTRY_MASK) >= FW_CFG_MAX_ENTRY) {
>> s->cur_entry = FW_CFG_INVALID;
>>
> Yeah, will fix.
>
>> With that change, I got the bootindex passed to OpenBIOS:
>> OpenBIOS for Sparc64
>> Configuration device id QEMU version 1 machine id 0
>> kernel cmdline
>> CPUs: 1 x SUNW,UltraSPARC-IIi
>> UUID: 00000000-0000-0000-0000-000000000000
>> bootindex num_strings 1
>> bootindex /pbm@000001fe00000000/ide@5/drive@1/disk@0
>>
>> The device path does not match exactly, but it's close:
>> /pci@1fe,0/pci-ata@5/ide1@600/disk@0
>
> pbm->pci should be solvable by the patch at the end. Were in the spec
> it is allowed to abbreviate 1fe00000000 as 1fe,0? Spec allows to drop
> starting zeroes but TARGET_FMT_plx definition in targphys.h has 0 after
> %. I can define another one without leading zeroes. Can you suggest
> a name?
I think OpenBIOS for Sparc64 is not correct here, so it may be a bad
reference architecture. OBP on a real Ultra-5 used a path like this:
/pci@1f,0/pci@1,1/ide@3/disk@0,0
pci@1f,0 specifies the PCI host bridge at UPA bus port ID of 0x1f.
pci@1,1 specifies a PCI-PCI bridge.
> TARGET_FMT_lx is poisoned. As of ATA there is no open firmware
> binding spec for ATA, so everyone does what he pleases. I based my
> implementation on what open firmware showing when running on qemu x86.
> "pci-ata" should be "ide" according to PCI binding spec :)
Yes, for example there is no ATA in the Ultra-5 tree but in UltraAX it exists:
/pci@1f,4000/ide@3/ata@0,0/cmdk@0,0
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index c619112..643aa49 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -453,6 +453,7 @@ static PCIDeviceInfo pbm_pci_host_info = {
>
> static SysBusDeviceInfo pbm_host_info = {
> .qdev.name = "pbm",
> + .qdev.fw_name = "pci",
Perhaps the FW path should use device class names if no name is specified.
I'll try Sparc32 to see how this fits there.
^ permalink raw reply [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-15 20:29 ` Blue Swirl
@ 2010-11-16 14:11 ` Gleb Natapov
2010-11-16 18:30 ` Blue Swirl
0 siblings, 1 reply; 55+ messages in thread
From: Gleb Natapov @ 2010-11-16 14:11 UTC (permalink / raw)
To: Blue Swirl; +Cc: kvm, mst, qemu-devel, armbru, alex.williamson, kevin
On Mon, Nov 15, 2010 at 08:29:24PM +0000, Blue Swirl wrote:
> 2010/11/15 Gleb Natapov <gleb@redhat.com>:
> > On Sun, Nov 14, 2010 at 10:50:13PM +0000, Blue Swirl wrote:
> >> On Sun, Nov 14, 2010 at 3:39 PM, Gleb Natapov <gleb@redhat.com> wrote:
> >> >
> >> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> >> > ---
> >> > hw/fw_cfg.c | 14 ++++++++++++++
> >> > hw/fw_cfg.h | 4 +++-
> >> > sysemu.h | 1 +
> >> > vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >> > 4 files changed, 69 insertions(+), 1 deletions(-)
> >> >
> >> > diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
> >> > index 7b9434f..f6a67db 100644
> >> > --- a/hw/fw_cfg.c
> >> > +++ b/hw/fw_cfg.c
> >> > @@ -53,6 +53,7 @@ struct FWCfgState {
> >> > FWCfgFiles *files;
> >> > uint16_t cur_entry;
> >> > uint32_t cur_offset;
> >> > + Notifier machine_ready;
> >> > };
> >> >
> >> > static void fw_cfg_write(FWCfgState *s, uint8_t value)
> >> > @@ -315,6 +316,15 @@ int fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
> >> > return 1;
> >> > }
> >> >
> >> > +static void fw_cfg_machine_ready(struct Notifier* n)
> >> > +{
> >> > + uint32_t len;
> >> > + char *bootindex = get_boot_devices_list(&len);
> >> > +
> >> > + fw_cfg_add_bytes(container_of(n, FWCfgState, machine_ready),
> >> > + FW_CFG_BOOTINDEX, (uint8_t*)bootindex, len);
> >> > +}
> >> > +
> >> > FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
> >> > target_phys_addr_t ctl_addr, target_phys_addr_t data_addr)
> >> > {
> >> > @@ -343,6 +353,10 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
> >> > fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
> >> > fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
> >> >
> >> > +
> >> > + s->machine_ready.notify = fw_cfg_machine_ready;
> >> > + qemu_add_machine_init_done_notifier(&s->machine_ready);
> >> > +
> >> > return s;
> >> > }
> >> >
> >> > diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
> >> > index 856bf91..4d61410 100644
> >> > --- a/hw/fw_cfg.h
> >> > +++ b/hw/fw_cfg.h
> >> > @@ -30,7 +30,9 @@
> >> >
> >> > #define FW_CFG_FILE_FIRST 0x20
> >> > #define FW_CFG_FILE_SLOTS 0x10
> >> > -#define FW_CFG_MAX_ENTRY (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
> >> > +#define FW_CFG_FILE_LAST_SLOT (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
> >> > +#define FW_CFG_BOOTINDEX (FW_CFG_FILE_LAST_SLOT + 1)
> >> > +#define FW_CFG_MAX_ENTRY FW_CFG_BOOTINDEX
> >>
> >> This should be
> >> #define FW_CFG_MAX_ENTRY (FW_CFG_BOOTINDEX + 1)
> >> because the check is like this:
> >> if ((key & FW_CFG_ENTRY_MASK) >= FW_CFG_MAX_ENTRY) {
> >> s->cur_entry = FW_CFG_INVALID;
> >>
> > Yeah, will fix.
> >
> >> With that change, I got the bootindex passed to OpenBIOS:
> >> OpenBIOS for Sparc64
> >> Configuration device id QEMU version 1 machine id 0
> >> kernel cmdline
> >> CPUs: 1 x SUNW,UltraSPARC-IIi
> >> UUID: 00000000-0000-0000-0000-000000000000
> >> bootindex num_strings 1
> >> bootindex /pbm@000001fe00000000/ide@5/drive@1/disk@0
> >>
> >> The device path does not match exactly, but it's close:
> >> /pci@1fe,0/pci-ata@5/ide1@600/disk@0
> >
> > pbm->pci should be solvable by the patch at the end. Were in the spec
> > it is allowed to abbreviate 1fe00000000 as 1fe,0? Spec allows to drop
> > starting zeroes but TARGET_FMT_plx definition in targphys.h has 0 after
> > %. I can define another one without leading zeroes. Can you suggest
> > a name?
>
> I think OpenBIOS for Sparc64 is not correct here, so it may be a bad
> reference architecture. OBP on a real Ultra-5 used a path like this:
> /pci@1f,0/pci@1,1/ide@3/disk@0,0
>
> pci@1f,0 specifies the PCI host bridge at UPA bus port ID of 0x1f.
According to device name qemu creates pci controller is memory mapped
at address 1fe00000000 and by looking at the code I can see that this
is indeed the case. How is UPA naming works?
> pci@1,1 specifies a PCI-PCI bridge.
>
> > TARGET_FMT_lx is poisoned. As of ATA there is no open firmware
> > binding spec for ATA, so everyone does what he pleases. I based my
> > implementation on what open firmware showing when running on qemu x86.
> > "pci-ata" should be "ide" according to PCI binding spec :)
>
> Yes, for example there is no ATA in the Ultra-5 tree but in UltraAX it exists:
> /pci@1f,4000/ide@3/ata@0,0/cmdk@0,0
>
> > diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> > index c619112..643aa49 100644
> > --- a/hw/apb_pci.c
> > +++ b/hw/apb_pci.c
> > @@ -453,6 +453,7 @@ static PCIDeviceInfo pbm_pci_host_info = {
> >
> > static SysBusDeviceInfo pbm_host_info = {
> > .qdev.name = "pbm",
> > + .qdev.fw_name = "pci",
>
> Perhaps the FW path should use device class names if no name is specified.
What do you mean by "device class name". We can do something like this:
if (dev->child_bus.lh_first)
return dev->child_bus.lh_first->info->name;
i.e if there is child bus use its bus name as fw name. This will make
all pci devices to have "pci" as fw name automatically. The problem is
that theoretically same device can provide different buses.
>
> I'll try Sparc32 to see how this fits there.
--
Gleb.
^ permalink raw reply [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-16 14:11 ` Gleb Natapov
@ 2010-11-16 18:30 ` Blue Swirl
2010-11-16 19:02 ` Gleb Natapov
0 siblings, 1 reply; 55+ messages in thread
From: Blue Swirl @ 2010-11-16 18:30 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, mst, qemu-devel, armbru, alex.williamson, kevin
2010/11/16 Gleb Natapov <gleb@redhat.com>:
> On Mon, Nov 15, 2010 at 08:29:24PM +0000, Blue Swirl wrote:
>> 2010/11/15 Gleb Natapov <gleb@redhat.com>:
>> > On Sun, Nov 14, 2010 at 10:50:13PM +0000, Blue Swirl wrote:
>> >> On Sun, Nov 14, 2010 at 3:39 PM, Gleb Natapov <gleb@redhat.com> wrote:
>> >> >
>> >> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
>> >> > ---
>> >> > hw/fw_cfg.c | 14 ++++++++++++++
>> >> > hw/fw_cfg.h | 4 +++-
>> >> > sysemu.h | 1 +
>> >> > vl.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> >> > 4 files changed, 69 insertions(+), 1 deletions(-)
>> >> >
>> >> > diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
>> >> > index 7b9434f..f6a67db 100644
>> >> > --- a/hw/fw_cfg.c
>> >> > +++ b/hw/fw_cfg.c
>> >> > @@ -53,6 +53,7 @@ struct FWCfgState {
>> >> > FWCfgFiles *files;
>> >> > uint16_t cur_entry;
>> >> > uint32_t cur_offset;
>> >> > + Notifier machine_ready;
>> >> > };
>> >> >
>> >> > static void fw_cfg_write(FWCfgState *s, uint8_t value)
>> >> > @@ -315,6 +316,15 @@ int fw_cfg_add_file(FWCfgState *s, const char *filename, uint8_t *data,
>> >> > return 1;
>> >> > }
>> >> >
>> >> > +static void fw_cfg_machine_ready(struct Notifier* n)
>> >> > +{
>> >> > + uint32_t len;
>> >> > + char *bootindex = get_boot_devices_list(&len);
>> >> > +
>> >> > + fw_cfg_add_bytes(container_of(n, FWCfgState, machine_ready),
>> >> > + FW_CFG_BOOTINDEX, (uint8_t*)bootindex, len);
>> >> > +}
>> >> > +
>> >> > FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
>> >> > target_phys_addr_t ctl_addr, target_phys_addr_t data_addr)
>> >> > {
>> >> > @@ -343,6 +353,10 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
>> >> > fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
>> >> > fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
>> >> >
>> >> > +
>> >> > + s->machine_ready.notify = fw_cfg_machine_ready;
>> >> > + qemu_add_machine_init_done_notifier(&s->machine_ready);
>> >> > +
>> >> > return s;
>> >> > }
>> >> >
>> >> > diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
>> >> > index 856bf91..4d61410 100644
>> >> > --- a/hw/fw_cfg.h
>> >> > +++ b/hw/fw_cfg.h
>> >> > @@ -30,7 +30,9 @@
>> >> >
>> >> > #define FW_CFG_FILE_FIRST 0x20
>> >> > #define FW_CFG_FILE_SLOTS 0x10
>> >> > -#define FW_CFG_MAX_ENTRY (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
>> >> > +#define FW_CFG_FILE_LAST_SLOT (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
>> >> > +#define FW_CFG_BOOTINDEX (FW_CFG_FILE_LAST_SLOT + 1)
>> >> > +#define FW_CFG_MAX_ENTRY FW_CFG_BOOTINDEX
>> >>
>> >> This should be
>> >> #define FW_CFG_MAX_ENTRY (FW_CFG_BOOTINDEX + 1)
>> >> because the check is like this:
>> >> if ((key & FW_CFG_ENTRY_MASK) >= FW_CFG_MAX_ENTRY) {
>> >> s->cur_entry = FW_CFG_INVALID;
>> >>
>> > Yeah, will fix.
>> >
>> >> With that change, I got the bootindex passed to OpenBIOS:
>> >> OpenBIOS for Sparc64
>> >> Configuration device id QEMU version 1 machine id 0
>> >> kernel cmdline
>> >> CPUs: 1 x SUNW,UltraSPARC-IIi
>> >> UUID: 00000000-0000-0000-0000-000000000000
>> >> bootindex num_strings 1
>> >> bootindex /pbm@000001fe00000000/ide@5/drive@1/disk@0
>> >>
>> >> The device path does not match exactly, but it's close:
>> >> /pci@1fe,0/pci-ata@5/ide1@600/disk@0
>> >
>> > pbm->pci should be solvable by the patch at the end. Were in the spec
>> > it is allowed to abbreviate 1fe00000000 as 1fe,0? Spec allows to drop
>> > starting zeroes but TARGET_FMT_plx definition in targphys.h has 0 after
>> > %. I can define another one without leading zeroes. Can you suggest
>> > a name?
>>
>> I think OpenBIOS for Sparc64 is not correct here, so it may be a bad
>> reference architecture. OBP on a real Ultra-5 used a path like this:
>> /pci@1f,0/pci@1,1/ide@3/disk@0,0
>>
>> pci@1f,0 specifies the PCI host bridge at UPA bus port ID of 0x1f.
> According to device name qemu creates pci controller is memory mapped
> at address 1fe00000000 and by looking at the code I can see that this
> is indeed the case. How is UPA naming works?
No idea.
>> pci@1,1 specifies a PCI-PCI bridge.
>>
>> > TARGET_FMT_lx is poisoned. As of ATA there is no open firmware
>> > binding spec for ATA, so everyone does what he pleases. I based my
>> > implementation on what open firmware showing when running on qemu x86.
>> > "pci-ata" should be "ide" according to PCI binding spec :)
>>
>> Yes, for example there is no ATA in the Ultra-5 tree but in UltraAX it exists:
>> /pci@1f,4000/ide@3/ata@0,0/cmdk@0,0
>>
>> > diff --git a/hw/apb_pci.c b/hw/apb_pci.c
>> > index c619112..643aa49 100644
>> > --- a/hw/apb_pci.c
>> > +++ b/hw/apb_pci.c
>> > @@ -453,6 +453,7 @@ static PCIDeviceInfo pbm_pci_host_info = {
>> >
>> > static SysBusDeviceInfo pbm_host_info = {
>> > .qdev.name = "pbm",
>> > + .qdev.fw_name = "pci",
>>
>> Perhaps the FW path should use device class names if no name is specified.
> What do you mean by "device class name". We can do something like this:
> if (dev->child_bus.lh_first)
> return dev->child_bus.lh_first->info->name;
>
> i.e if there is child bus use its bus name as fw name. This will make
> all pci devices to have "pci" as fw name automatically. The problem is
> that theoretically same device can provide different buses.
I meant PCI class name, like "display" for display controllers,
"network" for NICs etc.
>> I'll try Sparc32 to see how this fits there.
Except bootindex is not implemented for SCSI.
^ permalink raw reply [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-16 18:30 ` Blue Swirl
@ 2010-11-16 19:02 ` Gleb Natapov
2010-11-17 21:54 ` Blue Swirl
0 siblings, 1 reply; 55+ messages in thread
From: Gleb Natapov @ 2010-11-16 19:02 UTC (permalink / raw)
To: Blue Swirl; +Cc: kvm, mst, qemu-devel, armbru, alex.williamson, kevin
On Tue, Nov 16, 2010 at 06:30:19PM +0000, Blue Swirl wrote:
> >> Perhaps the FW path should use device class names if no name is specified.
> > What do you mean by "device class name". We can do something like this:
> > if (dev->child_bus.lh_first)
> > return dev->child_bus.lh_first->info->name;
> >
> > i.e if there is child bus use its bus name as fw name. This will make
> > all pci devices to have "pci" as fw name automatically. The problem is
> > that theoretically same device can provide different buses.
>
> I meant PCI class name, like "display" for display controllers,
> "network" for NICs etc.
>
That is what my pci bus related patch is doing already.
> >> I'll try Sparc32 to see how this fits there.
>
> Except bootindex is not implemented for SCSI.
Will look into adding it.
--
Gleb.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-16 19:02 ` Gleb Natapov
@ 2010-11-17 21:54 ` Blue Swirl
2010-11-18 10:18 ` Gleb Natapov
0 siblings, 1 reply; 55+ messages in thread
From: Blue Swirl @ 2010-11-17 21:54 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, mst, qemu-devel, armbru, alex.williamson, kevin
2010/11/16 Gleb Natapov <gleb@redhat.com>:
> On Tue, Nov 16, 2010 at 06:30:19PM +0000, Blue Swirl wrote:
>> >> Perhaps the FW path should use device class names if no name is specified.
>> > What do you mean by "device class name". We can do something like this:
>> > if (dev->child_bus.lh_first)
>> > return dev->child_bus.lh_first->info->name;
>> >
>> > i.e if there is child bus use its bus name as fw name. This will make
>> > all pci devices to have "pci" as fw name automatically. The problem is
>> > that theoretically same device can provide different buses.
>>
>> I meant PCI class name, like "display" for display controllers,
>> "network" for NICs etc.
>>
> That is what my pci bus related patch is doing already.
>
>> >> I'll try Sparc32 to see how this fits there.
>>
>> Except bootindex is not implemented for SCSI.
> Will look into adding it.
Thanks. The bootindex on Sparc32 looks like this:
bootindex /esp@0000000078800000/disk@1,0
/ethernet@ffffffffffffffff/ethernet-phy@0
I don't think I got Lance setup right.
OF paths for the devices would be:
/iommu@0,10000000/sbus@0,10001000/espdma@5,8400000/esp@5,8800000/sd@1,0
/iommu@0,10000000/sbus@0,10001000/ledma@5,8400010/le@5,8c00000
The logic for ESP is that ESP (registers at 0x78800000, slot offset
0x880000) is handled by the DMA controller (registers at 0x78400000,
slot offset 0x840000), they are in a SBus slot #5, and SBus (registers
at 0x10001000) is in turn handled by IOMMU (registers at 0x10000000).
Lance should be handled the same way.
This hierarchy is partly known by QEMU because DMA accesses use this
flow, but not otherwise. There is no concept of SBus slots, DMA talks
to IOMMU directly. Though in this case both ESP, Lance and their DMA
controllers are on board devices in a MACIO chip. It may be possible
to add the hierarchy information at each stage.
It should also be possible for BIOS to determine the device just from
the physical address if we ignored OF compatibility.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-17 21:54 ` Blue Swirl
@ 2010-11-18 10:18 ` Gleb Natapov
2010-11-18 11:38 ` Michael S. Tsirkin
0 siblings, 1 reply; 55+ messages in thread
From: Gleb Natapov @ 2010-11-18 10:18 UTC (permalink / raw)
To: Blue Swirl; +Cc: kvm, mst, qemu-devel, armbru, alex.williamson, kevin
On Wed, Nov 17, 2010 at 09:54:27PM +0000, Blue Swirl wrote:
> 2010/11/16 Gleb Natapov <gleb@redhat.com>:
> > On Tue, Nov 16, 2010 at 06:30:19PM +0000, Blue Swirl wrote:
> >> >> Perhaps the FW path should use device class names if no name is specified.
> >> > What do you mean by "device class name". We can do something like this:
> >> > if (dev->child_bus.lh_first)
> >> > return dev->child_bus.lh_first->info->name;
> >> >
> >> > i.e if there is child bus use its bus name as fw name. This will make
> >> > all pci devices to have "pci" as fw name automatically. The problem is
> >> > that theoretically same device can provide different buses.
> >>
> >> I meant PCI class name, like "display" for display controllers,
> >> "network" for NICs etc.
> >>
> > That is what my pci bus related patch is doing already.
> >
> >> >> I'll try Sparc32 to see how this fits there.
> >>
> >> Except bootindex is not implemented for SCSI.
> > Will look into adding it.
>
> Thanks. The bootindex on Sparc32 looks like this:
> bootindex /esp@0000000078800000/disk@1,0
> /ethernet@ffffffffffffffff/ethernet-phy@0
>
For arches other then x86 there is a lot of work left to be done :)
For starter exotic sparc buses should get their own get_fw_dev_path()
implementation.
> I don't think I got Lance setup right.
>
> OF paths for the devices would be:
> /iommu@0,10000000/sbus@0,10001000/espdma@5,8400000/esp@5,8800000/sd@1,0
> /iommu@0,10000000/sbus@0,10001000/ledma@5,8400010/le@5,8c00000
If qdev hierarchy does not correspond to real HW there is no much we can
do expect for fixing qdev.
>
> The logic for ESP is that ESP (registers at 0x78800000, slot offset
> 0x880000) is handled by the DMA controller (registers at 0x78400000,
> slot offset 0x840000), they are in a SBus slot #5, and SBus (registers
> at 0x10001000) is in turn handled by IOMMU (registers at 0x10000000).
> Lance should be handled the same way.
>
> This hierarchy is partly known by QEMU because DMA accesses use this
> flow, but not otherwise. There is no concept of SBus slots, DMA talks
> to IOMMU directly. Though in this case both ESP, Lance and their DMA
> controllers are on board devices in a MACIO chip. It may be possible
> to add the hierarchy information at each stage.
>
> It should also be possible for BIOS to determine the device just from
> the physical address if we ignored OF compatibility.
It would be nice to be OF compatible at least at some level. Of course OF
spec is not strict enough to have two different implementations produce
exactly same device path that can be compared by strcpy. Can we apply
the series now? At least for x86 it provides useful paths and work can
be continue for other arches by interested parties.
--
Gleb.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-18 10:18 ` Gleb Natapov
@ 2010-11-18 11:38 ` Michael S. Tsirkin
2010-11-18 11:45 ` Gleb Natapov
0 siblings, 1 reply; 55+ messages in thread
From: Michael S. Tsirkin @ 2010-11-18 11:38 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, qemu-devel, armbru, Blue Swirl, alex.williamson, kevin
On Thu, Nov 18, 2010 at 12:18:27PM +0200, Gleb Natapov wrote:
> On Wed, Nov 17, 2010 at 09:54:27PM +0000, Blue Swirl wrote:
> > 2010/11/16 Gleb Natapov <gleb@redhat.com>:
> > > On Tue, Nov 16, 2010 at 06:30:19PM +0000, Blue Swirl wrote:
> > >> >> Perhaps the FW path should use device class names if no name is specified.
> > >> > What do you mean by "device class name". We can do something like this:
> > >> > if (dev->child_bus.lh_first)
> > >> > return dev->child_bus.lh_first->info->name;
> > >> >
> > >> > i.e if there is child bus use its bus name as fw name. This will make
> > >> > all pci devices to have "pci" as fw name automatically. The problem is
> > >> > that theoretically same device can provide different buses.
> > >>
> > >> I meant PCI class name, like "display" for display controllers,
> > >> "network" for NICs etc.
> > >>
> > > That is what my pci bus related patch is doing already.
> > >
> > >> >> I'll try Sparc32 to see how this fits there.
> > >>
> > >> Except bootindex is not implemented for SCSI.
> > > Will look into adding it.
> >
> > Thanks. The bootindex on Sparc32 looks like this:
> > bootindex /esp@0000000078800000/disk@1,0
> > /ethernet@ffffffffffffffff/ethernet-phy@0
> >
> For arches other then x86 there is a lot of work left to be done :)
> For starter exotic sparc buses should get their own get_fw_dev_path()
> implementation.
>
> > I don't think I got Lance setup right.
> >
> > OF paths for the devices would be:
> > /iommu@0,10000000/sbus@0,10001000/espdma@5,8400000/esp@5,8800000/sd@1,0
> > /iommu@0,10000000/sbus@0,10001000/ledma@5,8400010/le@5,8c00000
> If qdev hierarchy does not correspond to real HW there is no much we can
> do expect for fixing qdev.
That's bad. This raises a concern: if these paths expose qdev
internals, any attempt to fix this will break migration.
> >
> > The logic for ESP is that ESP (registers at 0x78800000, slot offset
> > 0x880000) is handled by the DMA controller (registers at 0x78400000,
> > slot offset 0x840000), they are in a SBus slot #5, and SBus (registers
> > at 0x10001000) is in turn handled by IOMMU (registers at 0x10000000).
> > Lance should be handled the same way.
> >
> > This hierarchy is partly known by QEMU because DMA accesses use this
> > flow, but not otherwise. There is no concept of SBus slots, DMA talks
> > to IOMMU directly. Though in this case both ESP, Lance and their DMA
> > controllers are on board devices in a MACIO chip. It may be possible
> > to add the hierarchy information at each stage.
> >
> > It should also be possible for BIOS to determine the device just from
> > the physical address if we ignored OF compatibility.
> It would be nice to be OF compatible at least at some level. Of course OF
> spec is not strict enough to have two different implementations produce
> exactly same device path that can be compared by strcpy. Can we apply
> the series now? At least for x86 it provides useful paths and work can
> be continue for other arches by interested parties.
>
> --
> Gleb.
Something I only now realized is that we commit
to never changing the paths for any architecture
that supports migration.
--
MST
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-18 11:38 ` Michael S. Tsirkin
@ 2010-11-18 11:45 ` Gleb Natapov
2010-11-18 11:52 ` Michael S. Tsirkin
0 siblings, 1 reply; 55+ messages in thread
From: Gleb Natapov @ 2010-11-18 11:45 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, qemu-devel, armbru, Blue Swirl, alex.williamson, kevin
On Thu, Nov 18, 2010 at 01:38:31PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 18, 2010 at 12:18:27PM +0200, Gleb Natapov wrote:
> > On Wed, Nov 17, 2010 at 09:54:27PM +0000, Blue Swirl wrote:
> > > 2010/11/16 Gleb Natapov <gleb@redhat.com>:
> > > > On Tue, Nov 16, 2010 at 06:30:19PM +0000, Blue Swirl wrote:
> > > >> >> Perhaps the FW path should use device class names if no name is specified.
> > > >> > What do you mean by "device class name". We can do something like this:
> > > >> > if (dev->child_bus.lh_first)
> > > >> > return dev->child_bus.lh_first->info->name;
> > > >> >
> > > >> > i.e if there is child bus use its bus name as fw name. This will make
> > > >> > all pci devices to have "pci" as fw name automatically. The problem is
> > > >> > that theoretically same device can provide different buses.
> > > >>
> > > >> I meant PCI class name, like "display" for display controllers,
> > > >> "network" for NICs etc.
> > > >>
> > > > That is what my pci bus related patch is doing already.
> > > >
> > > >> >> I'll try Sparc32 to see how this fits there.
> > > >>
> > > >> Except bootindex is not implemented for SCSI.
> > > > Will look into adding it.
> > >
> > > Thanks. The bootindex on Sparc32 looks like this:
> > > bootindex /esp@0000000078800000/disk@1,0
> > > /ethernet@ffffffffffffffff/ethernet-phy@0
> > >
> > For arches other then x86 there is a lot of work left to be done :)
> > For starter exotic sparc buses should get their own get_fw_dev_path()
> > implementation.
> >
> > > I don't think I got Lance setup right.
> > >
> > > OF paths for the devices would be:
> > > /iommu@0,10000000/sbus@0,10001000/espdma@5,8400000/esp@5,8800000/sd@1,0
> > > /iommu@0,10000000/sbus@0,10001000/ledma@5,8400010/le@5,8c00000
> > If qdev hierarchy does not correspond to real HW there is no much we can
> > do expect for fixing qdev.
>
> That's bad. This raises a concern: if these paths expose qdev
> internals, any attempt to fix this will break migration.
>
The path expose internal HW hierarchy. It is designed to do so. Qdev
designed to do the same: describe HW hierarchy. If qdev fails to do so it
is broken. I do not see connection to migration at all since the path is
not used in migration code.
> > >
> > > The logic for ESP is that ESP (registers at 0x78800000, slot offset
> > > 0x880000) is handled by the DMA controller (registers at 0x78400000,
> > > slot offset 0x840000), they are in a SBus slot #5, and SBus (registers
> > > at 0x10001000) is in turn handled by IOMMU (registers at 0x10000000).
> > > Lance should be handled the same way.
> > >
> > > This hierarchy is partly known by QEMU because DMA accesses use this
> > > flow, but not otherwise. There is no concept of SBus slots, DMA talks
> > > to IOMMU directly. Though in this case both ESP, Lance and their DMA
> > > controllers are on board devices in a MACIO chip. It may be possible
> > > to add the hierarchy information at each stage.
> > >
> > > It should also be possible for BIOS to determine the device just from
> > > the physical address if we ignored OF compatibility.
> > It would be nice to be OF compatible at least at some level. Of course OF
> > spec is not strict enough to have two different implementations produce
> > exactly same device path that can be compared by strcpy. Can we apply
> > the series now? At least for x86 it provides useful paths and work can
> > be continue for other arches by interested parties.
> >
> > --
> > Gleb.
>
> Something I only now realized is that we commit
> to never changing the paths for any architecture
> that supports migration.
>
No connection to migration whatsoever.
--
Gleb.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-18 11:45 ` Gleb Natapov
@ 2010-11-18 11:52 ` Michael S. Tsirkin
2010-11-18 12:16 ` Gleb Natapov
0 siblings, 1 reply; 55+ messages in thread
From: Michael S. Tsirkin @ 2010-11-18 11:52 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, qemu-devel, armbru, Blue Swirl, alex.williamson, kevin
On Thu, Nov 18, 2010 at 01:45:04PM +0200, Gleb Natapov wrote:
> On Thu, Nov 18, 2010 at 01:38:31PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 18, 2010 at 12:18:27PM +0200, Gleb Natapov wrote:
> > > On Wed, Nov 17, 2010 at 09:54:27PM +0000, Blue Swirl wrote:
> > > > 2010/11/16 Gleb Natapov <gleb@redhat.com>:
> > > > > On Tue, Nov 16, 2010 at 06:30:19PM +0000, Blue Swirl wrote:
> > > > >> >> Perhaps the FW path should use device class names if no name is specified.
> > > > >> > What do you mean by "device class name". We can do something like this:
> > > > >> > if (dev->child_bus.lh_first)
> > > > >> > return dev->child_bus.lh_first->info->name;
> > > > >> >
> > > > >> > i.e if there is child bus use its bus name as fw name. This will make
> > > > >> > all pci devices to have "pci" as fw name automatically. The problem is
> > > > >> > that theoretically same device can provide different buses.
> > > > >>
> > > > >> I meant PCI class name, like "display" for display controllers,
> > > > >> "network" for NICs etc.
> > > > >>
> > > > > That is what my pci bus related patch is doing already.
> > > > >
> > > > >> >> I'll try Sparc32 to see how this fits there.
> > > > >>
> > > > >> Except bootindex is not implemented for SCSI.
> > > > > Will look into adding it.
> > > >
> > > > Thanks. The bootindex on Sparc32 looks like this:
> > > > bootindex /esp@0000000078800000/disk@1,0
> > > > /ethernet@ffffffffffffffff/ethernet-phy@0
> > > >
> > > For arches other then x86 there is a lot of work left to be done :)
> > > For starter exotic sparc buses should get their own get_fw_dev_path()
> > > implementation.
> > >
> > > > I don't think I got Lance setup right.
> > > >
> > > > OF paths for the devices would be:
> > > > /iommu@0,10000000/sbus@0,10001000/espdma@5,8400000/esp@5,8800000/sd@1,0
> > > > /iommu@0,10000000/sbus@0,10001000/ledma@5,8400010/le@5,8c00000
> > > If qdev hierarchy does not correspond to real HW there is no much we can
> > > do expect for fixing qdev.
> >
> > That's bad. This raises a concern: if these paths expose qdev
> > internals, any attempt to fix this will break migration.
> >
> The path expose internal HW hierarchy. It is designed to do so. Qdev
> designed to do the same: describe HW hierarchy. If qdev fails to do so it
> is broken.
Yes. But since you use qdev to build up the path, a broken
qdev will give you a broken path.
> I do not see connection to migration at all since the path is
> not used in migration code.
The connection is that if we pass the list with path 1 which you define
as broken to BIOS, then migrate to a machine with an updated qemu
which has a correct path, BIOS won't be able to complete the boot.
Right? Same in reverse direction.
As solution could be a fuzzy matching
of paths that wiull let us recover.
> > > >
> > > > The logic for ESP is that ESP (registers at 0x78800000, slot offset
> > > > 0x880000) is handled by the DMA controller (registers at 0x78400000,
> > > > slot offset 0x840000), they are in a SBus slot #5, and SBus (registers
> > > > at 0x10001000) is in turn handled by IOMMU (registers at 0x10000000).
> > > > Lance should be handled the same way.
> > > >
> > > > This hierarchy is partly known by QEMU because DMA accesses use this
> > > > flow, but not otherwise. There is no concept of SBus slots, DMA talks
> > > > to IOMMU directly. Though in this case both ESP, Lance and their DMA
> > > > controllers are on board devices in a MACIO chip. It may be possible
> > > > to add the hierarchy information at each stage.
> > > >
> > > > It should also be possible for BIOS to determine the device just from
> > > > the physical address if we ignored OF compatibility.
> > > It would be nice to be OF compatible at least at some level. Of course OF
> > > spec is not strict enough to have two different implementations produce
> > > exactly same device path that can be compared by strcpy. Can we apply
> > > the series now? At least for x86 it provides useful paths and work can
> > > be continue for other arches by interested parties.
> > >
> > > --
> > > Gleb.
> >
> > Something I only now realized is that we commit
> > to never changing the paths for any architecture
> > that supports migration.
> >
> No connection to migration whatsoever.
It just seems silly to use different paths for the same thing.
Besides the connection above, I was hoping to use these paths
for section names in migration. If we can't guarantee they are
stable, we'll have to roll our own, and if we do this,
with stability guarantees required for migration format,
maybe use it for other things like BIOS as well?
> --
> Gleb.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-18 11:52 ` Michael S. Tsirkin
@ 2010-11-18 12:16 ` Gleb Natapov
2010-11-18 12:23 ` Michael S. Tsirkin
0 siblings, 1 reply; 55+ messages in thread
From: Gleb Natapov @ 2010-11-18 12:16 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, qemu-devel, armbru, Blue Swirl, alex.williamson, kevin
On Thu, Nov 18, 2010 at 01:52:30PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 18, 2010 at 01:45:04PM +0200, Gleb Natapov wrote:
> > On Thu, Nov 18, 2010 at 01:38:31PM +0200, Michael S. Tsirkin wrote:
> > > On Thu, Nov 18, 2010 at 12:18:27PM +0200, Gleb Natapov wrote:
> > > > On Wed, Nov 17, 2010 at 09:54:27PM +0000, Blue Swirl wrote:
> > > > > 2010/11/16 Gleb Natapov <gleb@redhat.com>:
> > > > > > On Tue, Nov 16, 2010 at 06:30:19PM +0000, Blue Swirl wrote:
> > > > > >> >> Perhaps the FW path should use device class names if no name is specified.
> > > > > >> > What do you mean by "device class name". We can do something like this:
> > > > > >> > if (dev->child_bus.lh_first)
> > > > > >> > return dev->child_bus.lh_first->info->name;
> > > > > >> >
> > > > > >> > i.e if there is child bus use its bus name as fw name. This will make
> > > > > >> > all pci devices to have "pci" as fw name automatically. The problem is
> > > > > >> > that theoretically same device can provide different buses.
> > > > > >>
> > > > > >> I meant PCI class name, like "display" for display controllers,
> > > > > >> "network" for NICs etc.
> > > > > >>
> > > > > > That is what my pci bus related patch is doing already.
> > > > > >
> > > > > >> >> I'll try Sparc32 to see how this fits there.
> > > > > >>
> > > > > >> Except bootindex is not implemented for SCSI.
> > > > > > Will look into adding it.
> > > > >
> > > > > Thanks. The bootindex on Sparc32 looks like this:
> > > > > bootindex /esp@0000000078800000/disk@1,0
> > > > > /ethernet@ffffffffffffffff/ethernet-phy@0
> > > > >
> > > > For arches other then x86 there is a lot of work left to be done :)
> > > > For starter exotic sparc buses should get their own get_fw_dev_path()
> > > > implementation.
> > > >
> > > > > I don't think I got Lance setup right.
> > > > >
> > > > > OF paths for the devices would be:
> > > > > /iommu@0,10000000/sbus@0,10001000/espdma@5,8400000/esp@5,8800000/sd@1,0
> > > > > /iommu@0,10000000/sbus@0,10001000/ledma@5,8400010/le@5,8c00000
> > > > If qdev hierarchy does not correspond to real HW there is no much we can
> > > > do expect for fixing qdev.
> > >
> > > That's bad. This raises a concern: if these paths expose qdev
> > > internals, any attempt to fix this will break migration.
> > >
> > The path expose internal HW hierarchy. It is designed to do so. Qdev
> > designed to do the same: describe HW hierarchy. If qdev fails to do so it
> > is broken.
>
> Yes. But since you use qdev to build up the path, a broken
> qdev will give you a broken path.
>
Qdev bug. Fix it like any other bug. The nice is that when you compare
device path produced by qdev and real HW you can see when qdev is wrong.
> > I do not see connection to migration at all since the path is
> > not used in migration code.
>
> The connection is that if we pass the list with path 1 which you define
> as broken to BIOS, then migrate to a machine with an updated qemu
> which has a correct path, BIOS won't be able to complete the boot.
You solve it like you solve all such issue with -M machine type.
But the problem exists only if migration happens in a short window
between start of the boot process and BIOS reading boot order string.
After reboot new qemu should have new BIOS.
> Right? Same in reverse direction.
Reverse direction is not and never was supported.
> As solution could be a fuzzy matching
> of paths that wiull let us recover.
>
Firmware can try its best of course, but nothing is guarantied.
> > > > >
> > > > > The logic for ESP is that ESP (registers at 0x78800000, slot offset
> > > > > 0x880000) is handled by the DMA controller (registers at 0x78400000,
> > > > > slot offset 0x840000), they are in a SBus slot #5, and SBus (registers
> > > > > at 0x10001000) is in turn handled by IOMMU (registers at 0x10000000).
> > > > > Lance should be handled the same way.
> > > > >
> > > > > This hierarchy is partly known by QEMU because DMA accesses use this
> > > > > flow, but not otherwise. There is no concept of SBus slots, DMA talks
> > > > > to IOMMU directly. Though in this case both ESP, Lance and their DMA
> > > > > controllers are on board devices in a MACIO chip. It may be possible
> > > > > to add the hierarchy information at each stage.
> > > > >
> > > > > It should also be possible for BIOS to determine the device just from
> > > > > the physical address if we ignored OF compatibility.
> > > > It would be nice to be OF compatible at least at some level. Of course OF
> > > > spec is not strict enough to have two different implementations produce
> > > > exactly same device path that can be compared by strcpy. Can we apply
> > > > the series now? At least for x86 it provides useful paths and work can
> > > > be continue for other arches by interested parties.
> > > >
> > > > --
> > > > Gleb.
> > >
> > > Something I only now realized is that we commit
> > > to never changing the paths for any architecture
> > > that supports migration.
> > >
> > No connection to migration whatsoever.
>
> It just seems silly to use different paths for the same thing.
>
> Besides the connection above, I was hoping to use these paths
> for section names in migration. If we can't guarantee they are
> stable, we'll have to roll our own, and if we do this,
> with stability guarantees required for migration format,
> maybe use it for other things like BIOS as well?
>
It doesn't matter what do you use for migration purposes as long as it
depend on qdev hierarchy it will have problem when qdev hierarchy
changes and if it doesn't you can't produce unique names reliably.
--
Gleb.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-18 12:16 ` Gleb Natapov
@ 2010-11-18 12:23 ` Michael S. Tsirkin
2010-11-18 12:37 ` Gleb Natapov
0 siblings, 1 reply; 55+ messages in thread
From: Michael S. Tsirkin @ 2010-11-18 12:23 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, qemu-devel, armbru, Blue Swirl, alex.williamson, kevin
On Thu, Nov 18, 2010 at 02:16:05PM +0200, Gleb Natapov wrote:
> On Thu, Nov 18, 2010 at 01:52:30PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 18, 2010 at 01:45:04PM +0200, Gleb Natapov wrote:
> > > On Thu, Nov 18, 2010 at 01:38:31PM +0200, Michael S. Tsirkin wrote:
> > > > On Thu, Nov 18, 2010 at 12:18:27PM +0200, Gleb Natapov wrote:
> > > > > On Wed, Nov 17, 2010 at 09:54:27PM +0000, Blue Swirl wrote:
> > > > > > 2010/11/16 Gleb Natapov <gleb@redhat.com>:
> > > > > > > On Tue, Nov 16, 2010 at 06:30:19PM +0000, Blue Swirl wrote:
> > > > > > >> >> Perhaps the FW path should use device class names if no name is specified.
> > > > > > >> > What do you mean by "device class name". We can do something like this:
> > > > > > >> > if (dev->child_bus.lh_first)
> > > > > > >> > return dev->child_bus.lh_first->info->name;
> > > > > > >> >
> > > > > > >> > i.e if there is child bus use its bus name as fw name. This will make
> > > > > > >> > all pci devices to have "pci" as fw name automatically. The problem is
> > > > > > >> > that theoretically same device can provide different buses.
> > > > > > >>
> > > > > > >> I meant PCI class name, like "display" for display controllers,
> > > > > > >> "network" for NICs etc.
> > > > > > >>
> > > > > > > That is what my pci bus related patch is doing already.
> > > > > > >
> > > > > > >> >> I'll try Sparc32 to see how this fits there.
> > > > > > >>
> > > > > > >> Except bootindex is not implemented for SCSI.
> > > > > > > Will look into adding it.
> > > > > >
> > > > > > Thanks. The bootindex on Sparc32 looks like this:
> > > > > > bootindex /esp@0000000078800000/disk@1,0
> > > > > > /ethernet@ffffffffffffffff/ethernet-phy@0
> > > > > >
> > > > > For arches other then x86 there is a lot of work left to be done :)
> > > > > For starter exotic sparc buses should get their own get_fw_dev_path()
> > > > > implementation.
> > > > >
> > > > > > I don't think I got Lance setup right.
> > > > > >
> > > > > > OF paths for the devices would be:
> > > > > > /iommu@0,10000000/sbus@0,10001000/espdma@5,8400000/esp@5,8800000/sd@1,0
> > > > > > /iommu@0,10000000/sbus@0,10001000/ledma@5,8400010/le@5,8c00000
> > > > > If qdev hierarchy does not correspond to real HW there is no much we can
> > > > > do expect for fixing qdev.
> > > >
> > > > That's bad. This raises a concern: if these paths expose qdev
> > > > internals, any attempt to fix this will break migration.
> > > >
> > > The path expose internal HW hierarchy. It is designed to do so. Qdev
> > > designed to do the same: describe HW hierarchy. If qdev fails to do so it
> > > is broken.
> >
> > Yes. But since you use qdev to build up the path, a broken
> > qdev will give you a broken path.
> >
> Qdev bug. Fix it like any other bug. The nice is that when you compare
> device path produced by qdev and real HW you can see when qdev is wrong.
>
> > > I do not see connection to migration at all since the path is
> > > not used in migration code.
> >
> > The connection is that if we pass the list with path 1 which you define
> > as broken to BIOS, then migrate to a machine with an updated qemu
> > which has a correct path, BIOS won't be able to complete the boot.
> You solve it like you solve all such issue with -M machine type.
So that's unavoidable if we think paths are correct.
But if we know they are wrong, we are better off
correcting them first IMO.
> But the problem exists only if migration happens in a short window
> between start of the boot process and BIOS reading boot order string.
> After reboot new qemu should have new BIOS.
That makes it even more nasty, doesn't it?
> > Right? Same in reverse direction.
> Reverse direction is not and never was supported.
>
> > As solution could be a fuzzy matching
> > of paths that wiull let us recover.
> >
> Firmware can try its best of course, but nothing is guarantied.
No I mean qemu could do matching fuzzily.
This way if we get a path from the old BIOS we can
survive.
> > > > > >
> > > > > > The logic for ESP is that ESP (registers at 0x78800000, slot offset
> > > > > > 0x880000) is handled by the DMA controller (registers at 0x78400000,
> > > > > > slot offset 0x840000), they are in a SBus slot #5, and SBus (registers
> > > > > > at 0x10001000) is in turn handled by IOMMU (registers at 0x10000000).
> > > > > > Lance should be handled the same way.
> > > > > >
> > > > > > This hierarchy is partly known by QEMU because DMA accesses use this
> > > > > > flow, but not otherwise. There is no concept of SBus slots, DMA talks
> > > > > > to IOMMU directly. Though in this case both ESP, Lance and their DMA
> > > > > > controllers are on board devices in a MACIO chip. It may be possible
> > > > > > to add the hierarchy information at each stage.
> > > > > >
> > > > > > It should also be possible for BIOS to determine the device just from
> > > > > > the physical address if we ignored OF compatibility.
> > > > > It would be nice to be OF compatible at least at some level. Of course OF
> > > > > spec is not strict enough to have two different implementations produce
> > > > > exactly same device path that can be compared by strcpy. Can we apply
> > > > > the series now? At least for x86 it provides useful paths and work can
> > > > > be continue for other arches by interested parties.
> > > > >
> > > > > --
> > > > > Gleb.
> > > >
> > > > Something I only now realized is that we commit
> > > > to never changing the paths for any architecture
> > > > that supports migration.
> > > >
> > > No connection to migration whatsoever.
> >
> > It just seems silly to use different paths for the same thing.
> >
> > Besides the connection above, I was hoping to use these paths
> > for section names in migration. If we can't guarantee they are
> > stable, we'll have to roll our own, and if we do this,
> > with stability guarantees required for migration format,
> > maybe use it for other things like BIOS as well?
> >
> It doesn't matter what do you use for migration purposes as long as it
> depend on qdev hierarchy it will have problem when qdev hierarchy
> changes and if it doesn't you can't produce unique names reliably.
>
> --
> Gleb.
We can, it's not like OF is the only way to enumerate.
We could have driver-specific paths for example, exactly like we
currently have. I.e. paths don't have to be globally unique because
each driver has it's own domain.
It seems cleaner to use an existing spec but we must figure out
how it will not become a support issue.
--
MST
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-18 12:23 ` Michael S. Tsirkin
@ 2010-11-18 12:37 ` Gleb Natapov
2010-11-18 13:12 ` Michael S. Tsirkin
0 siblings, 1 reply; 55+ messages in thread
From: Gleb Natapov @ 2010-11-18 12:37 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, qemu-devel, armbru, Blue Swirl, alex.williamson, kevin
On Thu, Nov 18, 2010 at 02:23:20PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 18, 2010 at 02:16:05PM +0200, Gleb Natapov wrote:
> > On Thu, Nov 18, 2010 at 01:52:30PM +0200, Michael S. Tsirkin wrote:
> > > On Thu, Nov 18, 2010 at 01:45:04PM +0200, Gleb Natapov wrote:
> > > > On Thu, Nov 18, 2010 at 01:38:31PM +0200, Michael S. Tsirkin wrote:
> > > > > On Thu, Nov 18, 2010 at 12:18:27PM +0200, Gleb Natapov wrote:
> > > > > > On Wed, Nov 17, 2010 at 09:54:27PM +0000, Blue Swirl wrote:
> > > > > > > 2010/11/16 Gleb Natapov <gleb@redhat.com>:
> > > > > > > > On Tue, Nov 16, 2010 at 06:30:19PM +0000, Blue Swirl wrote:
> > > > > > > >> >> Perhaps the FW path should use device class names if no name is specified.
> > > > > > > >> > What do you mean by "device class name". We can do something like this:
> > > > > > > >> > if (dev->child_bus.lh_first)
> > > > > > > >> > return dev->child_bus.lh_first->info->name;
> > > > > > > >> >
> > > > > > > >> > i.e if there is child bus use its bus name as fw name. This will make
> > > > > > > >> > all pci devices to have "pci" as fw name automatically. The problem is
> > > > > > > >> > that theoretically same device can provide different buses.
> > > > > > > >>
> > > > > > > >> I meant PCI class name, like "display" for display controllers,
> > > > > > > >> "network" for NICs etc.
> > > > > > > >>
> > > > > > > > That is what my pci bus related patch is doing already.
> > > > > > > >
> > > > > > > >> >> I'll try Sparc32 to see how this fits there.
> > > > > > > >>
> > > > > > > >> Except bootindex is not implemented for SCSI.
> > > > > > > > Will look into adding it.
> > > > > > >
> > > > > > > Thanks. The bootindex on Sparc32 looks like this:
> > > > > > > bootindex /esp@0000000078800000/disk@1,0
> > > > > > > /ethernet@ffffffffffffffff/ethernet-phy@0
> > > > > > >
> > > > > > For arches other then x86 there is a lot of work left to be done :)
> > > > > > For starter exotic sparc buses should get their own get_fw_dev_path()
> > > > > > implementation.
> > > > > >
> > > > > > > I don't think I got Lance setup right.
> > > > > > >
> > > > > > > OF paths for the devices would be:
> > > > > > > /iommu@0,10000000/sbus@0,10001000/espdma@5,8400000/esp@5,8800000/sd@1,0
> > > > > > > /iommu@0,10000000/sbus@0,10001000/ledma@5,8400010/le@5,8c00000
> > > > > > If qdev hierarchy does not correspond to real HW there is no much we can
> > > > > > do expect for fixing qdev.
> > > > >
> > > > > That's bad. This raises a concern: if these paths expose qdev
> > > > > internals, any attempt to fix this will break migration.
> > > > >
> > > > The path expose internal HW hierarchy. It is designed to do so. Qdev
> > > > designed to do the same: describe HW hierarchy. If qdev fails to do so it
> > > > is broken.
> > >
> > > Yes. But since you use qdev to build up the path, a broken
> > > qdev will give you a broken path.
> > >
> > Qdev bug. Fix it like any other bug. The nice is that when you compare
> > device path produced by qdev and real HW you can see when qdev is wrong.
> >
> > > > I do not see connection to migration at all since the path is
> > > > not used in migration code.
> > >
> > > The connection is that if we pass the list with path 1 which you define
> > > as broken to BIOS, then migrate to a machine with an updated qemu
> > > which has a correct path, BIOS won't be able to complete the boot.
> > You solve it like you solve all such issue with -M machine type.
>
> So that's unavoidable if we think paths are correct.
> But if we know they are wrong, we are better off
> correcting them first IMO.
>
They are correct for x86. My patch set does not even tries to cover all
HW. If sparc want to use them to it better be fixed. Or if there is enough
info in the path to determine device it may choose to use it as is.
> > But the problem exists only if migration happens in a short window
> > between start of the boot process and BIOS reading boot order string.
> > After reboot new qemu should have new BIOS.
>
> That makes it even more nasty, doesn't it?
No.
>
> > > Right? Same in reverse direction.
> > Reverse direction is not and never was supported.
> >
> > > As solution could be a fuzzy matching
> > > of paths that wiull let us recover.
> > >
> > Firmware can try its best of course, but nothing is guarantied.
>
> No I mean qemu could do matching fuzzily.
> This way if we get a path from the old BIOS we can
> survive.
Qemu does not take paths from BIOS so I don't know what are you talking
about here.
>
> > > > > > >
> > > > > > > The logic for ESP is that ESP (registers at 0x78800000, slot offset
> > > > > > > 0x880000) is handled by the DMA controller (registers at 0x78400000,
> > > > > > > slot offset 0x840000), they are in a SBus slot #5, and SBus (registers
> > > > > > > at 0x10001000) is in turn handled by IOMMU (registers at 0x10000000).
> > > > > > > Lance should be handled the same way.
> > > > > > >
> > > > > > > This hierarchy is partly known by QEMU because DMA accesses use this
> > > > > > > flow, but not otherwise. There is no concept of SBus slots, DMA talks
> > > > > > > to IOMMU directly. Though in this case both ESP, Lance and their DMA
> > > > > > > controllers are on board devices in a MACIO chip. It may be possible
> > > > > > > to add the hierarchy information at each stage.
> > > > > > >
> > > > > > > It should also be possible for BIOS to determine the device just from
> > > > > > > the physical address if we ignored OF compatibility.
> > > > > > It would be nice to be OF compatible at least at some level. Of course OF
> > > > > > spec is not strict enough to have two different implementations produce
> > > > > > exactly same device path that can be compared by strcpy. Can we apply
> > > > > > the series now? At least for x86 it provides useful paths and work can
> > > > > > be continue for other arches by interested parties.
> > > > > >
> > > > > > --
> > > > > > Gleb.
> > > > >
> > > > > Something I only now realized is that we commit
> > > > > to never changing the paths for any architecture
> > > > > that supports migration.
> > > > >
> > > > No connection to migration whatsoever.
> > >
> > > It just seems silly to use different paths for the same thing.
> > >
> > > Besides the connection above, I was hoping to use these paths
> > > for section names in migration. If we can't guarantee they are
> > > stable, we'll have to roll our own, and if we do this,
> > > with stability guarantees required for migration format,
> > > maybe use it for other things like BIOS as well?
> > >
> > It doesn't matter what do you use for migration purposes as long as it
> > depend on qdev hierarchy it will have problem when qdev hierarchy
> > changes and if it doesn't you can't produce unique names reliably.
> >
> > --
> > Gleb.
>
> We can, it's not like OF is the only way to enumerate.
> We could have driver-specific paths for example, exactly like we
> currently have. I.e. paths don't have to be globally unique because
> each driver has it's own domain.
>
What we have currently is not even close to be correct. It happens to
work since it is implemented only for one bus type and we can have only
one of this bus right now. And of course it is not suitable for passing
boot path to FW since it does not describe path to device.
> It seems cleaner to use an existing spec but we must figure out
> how it will not become a support issue.
>
If you think you can figure out how to describe device path (or even
give globally unique name to device) and to not depend on internal qdev
implementation go ahead and do that for migration.
--
Gleb.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-18 12:37 ` Gleb Natapov
@ 2010-11-18 13:12 ` Michael S. Tsirkin
2010-11-18 13:16 ` Gleb Natapov
0 siblings, 1 reply; 55+ messages in thread
From: Michael S. Tsirkin @ 2010-11-18 13:12 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, qemu-devel, armbru, Blue Swirl, alex.williamson, kevin
On Thu, Nov 18, 2010 at 02:37:08PM +0200, Gleb Natapov wrote:
> > So that's unavoidable if we think paths are correct.
> > But if we know they are wrong, we are better off
> > correcting them first IMO.
> >
> They are correct for x86. My patch set does not even tries to cover all
> HW. If sparc want to use them to it better be fixed. Or if there is enough
> info in the path to determine device it may choose to use it as is.
Fair enough I guess.
> > > But the problem exists only if migration happens in a short window
> > > between start of the boot process and BIOS reading boot order string.
> > > After reboot new qemu should have new BIOS.
> >
> > That makes it even more nasty, doesn't it?
> No.
Nasty as in hard to reproduce.
--
MST
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-18 13:12 ` Michael S. Tsirkin
@ 2010-11-18 13:16 ` Gleb Natapov
0 siblings, 0 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-18 13:16 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, qemu-devel, armbru, Blue Swirl, alex.williamson, kevin
On Thu, Nov 18, 2010 at 03:12:02PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 18, 2010 at 02:37:08PM +0200, Gleb Natapov wrote:
> > > So that's unavoidable if we think paths are correct.
> > > But if we know they are wrong, we are better off
> > > correcting them first IMO.
> > >
> > They are correct for x86. My patch set does not even tries to cover all
> > HW. If sparc want to use them to it better be fixed. Or if there is enough
> > info in the path to determine device it may choose to use it as is.
>
> Fair enough I guess.
>
> > > > But the problem exists only if migration happens in a short window
> > > > between start of the boot process and BIOS reading boot order string.
> > > > After reboot new qemu should have new BIOS.
> > >
> > > That makes it even more nasty, doesn't it?
> > No.
>
> Nasty as in hard to reproduce.
>
It is very easy to reproduce if you know what you are looking for :).
Just stick sleep() in correct place in the BIOS.
--
Gleb.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-14 15:39 ` [Qemu-devel] [PATCHv4 15/15] Pass boot device list to firmware Gleb Natapov
` (2 preceding siblings ...)
2010-11-14 22:50 ` Blue Swirl
@ 2010-11-15 3:40 ` Kevin O'Connor
2010-11-15 7:40 ` Gleb Natapov
3 siblings, 1 reply; 55+ messages in thread
From: Kevin O'Connor @ 2010-11-15 3:40 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, mst, qemu-devel, armbru, blauwirbel, alex.williamson
On Sun, Nov 14, 2010 at 05:39:41PM +0200, Gleb Natapov wrote:
> +/*
> + * This function returns device list as an array in a below format:
> + * +-----+-----+---------------+-----+---------------+--
> + * | n | l1 | devpath1 | l2 | devpath2 | ...
> + * +-----+-----+---------------+-----+---------------+--
> + * where:
> + * n - a number of devise pathes (one byte)
> + * l - length of following device path string (one byte)
> + * devpath - non-null terminated string of length l representing
> + * one device path
> + */
Why not just return a newline separated list that is null terminated?
-Kevin
^ permalink raw reply [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-15 3:40 ` Kevin O'Connor
@ 2010-11-15 7:40 ` Gleb Natapov
2010-11-15 7:53 ` Michael S. Tsirkin
2010-11-15 13:26 ` Kevin O'Connor
0 siblings, 2 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-15 7:40 UTC (permalink / raw)
To: Kevin O'Connor
Cc: kvm, mst, qemu-devel, armbru, blauwirbel, alex.williamson
On Sun, Nov 14, 2010 at 10:40:33PM -0500, Kevin O'Connor wrote:
> On Sun, Nov 14, 2010 at 05:39:41PM +0200, Gleb Natapov wrote:
> > +/*
> > + * This function returns device list as an array in a below format:
> > + * +-----+-----+---------------+-----+---------------+--
> > + * | n | l1 | devpath1 | l2 | devpath2 | ...
> > + * +-----+-----+---------------+-----+---------------+--
> > + * where:
> > + * n - a number of devise pathes (one byte)
> > + * l - length of following device path string (one byte)
> > + * devpath - non-null terminated string of length l representing
> > + * one device path
> > + */
>
> Why not just return a newline separated list that is null terminated?
>
Doing it like this will needlessly complicate firmware side. How do you
know how much memory to allocate before reading device list? Doing it
like Blue suggest (have BOOTINDEX_LEN and BOOTINDEX_STRING) solves this.
To create nice array from bootindex string you firmware will still have
to do additional pass on it though. With format like above the code
would look like that:
qemu_cfg_read(&n, 1);
arr = alloc(n);
for (i=0; i<n; i++) {
qemu_cfg_read(&l, 1);
arr[i] = zalloc(l+1);
qemu_cfg_read(arr[i], l);
}
--
Gleb.
^ permalink raw reply [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-15 7:40 ` Gleb Natapov
@ 2010-11-15 7:53 ` Michael S. Tsirkin
2010-11-15 8:09 ` Gleb Natapov
2010-11-15 13:26 ` Kevin O'Connor
1 sibling, 1 reply; 55+ messages in thread
From: Michael S. Tsirkin @ 2010-11-15 7:53 UTC (permalink / raw)
To: Gleb Natapov
Cc: kvm, qemu-devel, armbru, blauwirbel, alex.williamson,
Kevin O'Connor
On Mon, Nov 15, 2010 at 09:40:08AM +0200, Gleb Natapov wrote:
> On Sun, Nov 14, 2010 at 10:40:33PM -0500, Kevin O'Connor wrote:
> > On Sun, Nov 14, 2010 at 05:39:41PM +0200, Gleb Natapov wrote:
> > > +/*
> > > + * This function returns device list as an array in a below format:
> > > + * +-----+-----+---------------+-----+---------------+--
> > > + * | n | l1 | devpath1 | l2 | devpath2 | ...
> > > + * +-----+-----+---------------+-----+---------------+--
> > > + * where:
> > > + * n - a number of devise pathes (one byte)
> > > + * l - length of following device path string (one byte)
> > > + * devpath - non-null terminated string of length l representing
> > > + * one device path
> > > + */
> >
> > Why not just return a newline separated list that is null terminated?
> >
> Doing it like this will needlessly complicate firmware side. How do you
> know how much memory to allocate before reading device list?
Do a memory scan, count newlines until you reach 0?
> Doing it
> like Blue suggest (have BOOTINDEX_LEN and BOOTINDEX_STRING) solves this.
> To create nice array from bootindex string you firmware will still have
> to do additional pass on it though.
Why is this a problem? Pass over memory is cheap, isn't it?
> With format like above the code
> would look like that:
>
> qemu_cfg_read(&n, 1);
> arr = alloc(n);
> for (i=0; i<n; i++) {
> qemu_cfg_read(&l, 1);
> arr[i] = zalloc(l+1);
> qemu_cfg_read(arr[i], l);
> }
>
>
> --
> Gleb.
At this point I don't care about format.
But I would like one without 1-byte-length limitations,
just so we can cover whatever pci can through at us.
--
MST
^ permalink raw reply [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-15 7:53 ` Michael S. Tsirkin
@ 2010-11-15 8:09 ` Gleb Natapov
0 siblings, 0 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-15 8:09 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, qemu-devel, armbru, blauwirbel, alex.williamson,
Kevin O'Connor
On Mon, Nov 15, 2010 at 09:53:50AM +0200, Michael S. Tsirkin wrote:
> On Mon, Nov 15, 2010 at 09:40:08AM +0200, Gleb Natapov wrote:
> > On Sun, Nov 14, 2010 at 10:40:33PM -0500, Kevin O'Connor wrote:
> > > On Sun, Nov 14, 2010 at 05:39:41PM +0200, Gleb Natapov wrote:
> > > > +/*
> > > > + * This function returns device list as an array in a below format:
> > > > + * +-----+-----+---------------+-----+---------------+--
> > > > + * | n | l1 | devpath1 | l2 | devpath2 | ...
> > > > + * +-----+-----+---------------+-----+---------------+--
> > > > + * where:
> > > > + * n - a number of devise pathes (one byte)
> > > > + * l - length of following device path string (one byte)
> > > > + * devpath - non-null terminated string of length l representing
> > > > + * one device path
> > > > + */
> > >
> > > Why not just return a newline separated list that is null terminated?
> > >
> > Doing it like this will needlessly complicate firmware side. How do you
> > know how much memory to allocate before reading device list?
>
> Do a memory scan, count newlines until you reach 0?
>
To do memory scan you need to read it into memory first. To read it into
memory you need to know how much memory to allocate to know how much
memory to allocate you meed to do memory scan... Notice pattern here :)
Of course you can scan IO space too discarding everything you read first
time, but why introduce broken interface in the first place?
> > Doing it
> > like Blue suggest (have BOOTINDEX_LEN and BOOTINDEX_STRING) solves this.
> > To create nice array from bootindex string you firmware will still have
> > to do additional pass on it though.
>
> Why is this a problem? Pass over memory is cheap, isn't it?
>
More code, each line of code potentially introduce bug. But I will go with
Blue suggestion anyway since we already use it for other things.
> > With format like above the code
> > would look like that:
> >
> > qemu_cfg_read(&n, 1);
> > arr = alloc(n);
> > for (i=0; i<n; i++) {
> > qemu_cfg_read(&l, 1);
> > arr[i] = zalloc(l+1);
> > qemu_cfg_read(arr[i], l);
> > }
> >
> >
> > --
> > Gleb.
>
>
> At this point I don't care about format.
I do.
> But I would like one without 1-byte-length limitations,
> just so we can cover whatever pci can through at us.
>
I agree. 1-byte for one device string may be to limiting. It is still
more then 15 PCI bridges on a PC and if you have your pci bus go that
deep you are doing something very wrong. But according to spec device
name can be 32 byte long and device address may be 64bit physical
address and that makes length of one device element to be 50 byte.
--
Gleb.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-15 7:40 ` Gleb Natapov
2010-11-15 7:53 ` Michael S. Tsirkin
@ 2010-11-15 13:26 ` Kevin O'Connor
2010-11-15 13:36 ` Gleb Natapov
1 sibling, 1 reply; 55+ messages in thread
From: Kevin O'Connor @ 2010-11-15 13:26 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, mst, qemu-devel, armbru, blauwirbel, alex.williamson
On Mon, Nov 15, 2010 at 09:40:08AM +0200, Gleb Natapov wrote:
> On Sun, Nov 14, 2010 at 10:40:33PM -0500, Kevin O'Connor wrote:
> > Why not just return a newline separated list that is null terminated?
> >
> Doing it like this will needlessly complicate firmware side. How do you
> know how much memory to allocate before reading device list?
My preference would be for the size to be exposed via the
QEMU_CFG_FILE_DIR selector. (My preference would be for all objects
in fw_cfg to have entries in QEMU_CFG_FILE_DIR describing their size
in a reliable manner.)
-Kevin
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-15 13:26 ` Kevin O'Connor
@ 2010-11-15 13:36 ` Gleb Natapov
2010-11-15 13:46 ` Gleb Natapov
2010-11-16 2:52 ` Kevin O'Connor
0 siblings, 2 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-15 13:36 UTC (permalink / raw)
To: Kevin O'Connor
Cc: kvm, mst, qemu-devel, armbru, blauwirbel, alex.williamson
On Mon, Nov 15, 2010 at 08:26:35AM -0500, Kevin O'Connor wrote:
> On Mon, Nov 15, 2010 at 09:40:08AM +0200, Gleb Natapov wrote:
> > On Sun, Nov 14, 2010 at 10:40:33PM -0500, Kevin O'Connor wrote:
> > > Why not just return a newline separated list that is null terminated?
> > >
> > Doing it like this will needlessly complicate firmware side. How do you
> > know how much memory to allocate before reading device list?
>
> My preference would be for the size to be exposed via the
> QEMU_CFG_FILE_DIR selector. (My preference would be for all objects
> in fw_cfg to have entries in QEMU_CFG_FILE_DIR describing their size
> in a reliable manner.)
>
Will interface suggested by Blue will be good for you? The one with two
fw_cfg ids. BOOTINDEX_LEN for len and BOOTINDEX_DATA for device list. I
already changed my implementation to this one. Using FILE_DIR requires
us to generate synthetic name. Hmm BTW I do not see proper endianness
handling in FILE_DIR.
--
Gleb.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-15 13:36 ` Gleb Natapov
@ 2010-11-15 13:46 ` Gleb Natapov
2010-11-16 2:52 ` Kevin O'Connor
1 sibling, 0 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-15 13:46 UTC (permalink / raw)
To: Kevin O'Connor
Cc: kvm, mst, qemu-devel, armbru, blauwirbel, alex.williamson
On Mon, Nov 15, 2010 at 03:36:25PM +0200, Gleb Natapov wrote:
> Hmm BTW I do not see proper endianness
> handling in FILE_DIR.
>
That's just me. Everything it OK there with endianness.
--
Gleb.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-15 13:36 ` Gleb Natapov
2010-11-15 13:46 ` Gleb Natapov
@ 2010-11-16 2:52 ` Kevin O'Connor
2010-11-16 7:22 ` Gleb Natapov
1 sibling, 1 reply; 55+ messages in thread
From: Kevin O'Connor @ 2010-11-16 2:52 UTC (permalink / raw)
To: Gleb Natapov; +Cc: kvm, mst, qemu-devel, armbru, blauwirbel, alex.williamson
On Mon, Nov 15, 2010 at 03:36:25PM +0200, Gleb Natapov wrote:
> On Mon, Nov 15, 2010 at 08:26:35AM -0500, Kevin O'Connor wrote:
> > On Mon, Nov 15, 2010 at 09:40:08AM +0200, Gleb Natapov wrote:
> > > On Sun, Nov 14, 2010 at 10:40:33PM -0500, Kevin O'Connor wrote:
> > > > Why not just return a newline separated list that is null terminated?
> > > >
> > > Doing it like this will needlessly complicate firmware side. How do you
> > > know how much memory to allocate before reading device list?
> >
> > My preference would be for the size to be exposed via the
> > QEMU_CFG_FILE_DIR selector. (My preference would be for all objects
> > in fw_cfg to have entries in QEMU_CFG_FILE_DIR describing their size
> > in a reliable manner.)
> >
> Will interface suggested by Blue will be good for you? The one with two
> fw_cfg ids. BOOTINDEX_LEN for len and BOOTINDEX_DATA for device list. I
I dislike how different fw_cfg objects pass the length in different
ways (eg, QEMU_CFG_E820_TABLE passes length as first 4 bytes). This
is a common problem - I'd prefer if we could adopt one uniform way of
passing length. I think QEMU_CFG_FILE_DIR solves this problem well.
I also have an ulterior motive here. If the boot order is exposed as
a newline separated list via an entry in QEMU_CFG_FILE_DIR, then this
becomes free for coreboot users as well. (On coreboot, the boot order
could be placed in a "file" in flash with no change to the seabios
code.)
-Kevin
^ permalink raw reply [flat|nested] 55+ messages in thread
* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-16 2:52 ` Kevin O'Connor
@ 2010-11-16 7:22 ` Gleb Natapov
2010-11-16 13:49 ` Kevin O'Connor
2010-11-16 18:19 ` Blue Swirl
0 siblings, 2 replies; 55+ messages in thread
From: Gleb Natapov @ 2010-11-16 7:22 UTC (permalink / raw)
To: Kevin O'Connor
Cc: kvm, mst, qemu-devel, armbru, blauwirbel, alex.williamson
On Mon, Nov 15, 2010 at 09:52:19PM -0500, Kevin O'Connor wrote:
> On Mon, Nov 15, 2010 at 03:36:25PM +0200, Gleb Natapov wrote:
> > On Mon, Nov 15, 2010 at 08:26:35AM -0500, Kevin O'Connor wrote:
> > > On Mon, Nov 15, 2010 at 09:40:08AM +0200, Gleb Natapov wrote:
> > > > On Sun, Nov 14, 2010 at 10:40:33PM -0500, Kevin O'Connor wrote:
> > > > > Why not just return a newline separated list that is null terminated?
> > > > >
> > > > Doing it like this will needlessly complicate firmware side. How do you
> > > > know how much memory to allocate before reading device list?
> > >
> > > My preference would be for the size to be exposed via the
> > > QEMU_CFG_FILE_DIR selector. (My preference would be for all objects
> > > in fw_cfg to have entries in QEMU_CFG_FILE_DIR describing their size
> > > in a reliable manner.)
> > >
> > Will interface suggested by Blue will be good for you? The one with two
> > fw_cfg ids. BOOTINDEX_LEN for len and BOOTINDEX_DATA for device list. I
>
> I dislike how different fw_cfg objects pass the length in different
> ways (eg, QEMU_CFG_E820_TABLE passes length as first 4 bytes). This
> is a common problem - I'd prefer if we could adopt one uniform way of
> passing length. I think QEMU_CFG_FILE_DIR solves this problem well.
>
Looking at available fw cfg option I see that _SIZE _DATA is also a
common pattern. The problem with QEMU_CFG_FILE_DIR is that we have very
little available slots right now. If we a going to require everything to
use it we better grow number of available slots considerably now while
it is easily done (no option defined above file slots yet).
I personally do not have preferences one way or the other. Blue are you
OK with using QEMU_CFG_FILE_DIR?
> I also have an ulterior motive here. If the boot order is exposed as
> a newline separated list via an entry in QEMU_CFG_FILE_DIR, then this
> becomes free for coreboot users as well. (On coreboot, the boot order
> could be placed in a "file" in flash with no change to the seabios
> code.)
>
You can define get_boot_order() function and implement it differently
for qemu and coreboot. For coreboot it will be one linear. Just call
cbfs_copyfile("bootorder"). BTW why newline separation is important?
--
Gleb.
^ permalink raw reply [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-16 7:22 ` Gleb Natapov
@ 2010-11-16 13:49 ` Kevin O'Connor
2010-11-16 18:19 ` Blue Swirl
1 sibling, 0 replies; 55+ messages in thread
From: Kevin O'Connor @ 2010-11-16 13:49 UTC (permalink / raw)
To: Gleb Natapov; +Cc: qemu-devel, kvm
On Tue, Nov 16, 2010 at 09:22:45AM +0200, Gleb Natapov wrote:
> On Mon, Nov 15, 2010 at 09:52:19PM -0500, Kevin O'Connor wrote:
> > I also have an ulterior motive here. If the boot order is exposed as
> > a newline separated list via an entry in QEMU_CFG_FILE_DIR, then this
> > becomes free for coreboot users as well. (On coreboot, the boot order
> > could be placed in a "file" in flash with no change to the seabios
> > code.)
> >
> You can define get_boot_order() function and implement it differently
> for qemu and coreboot. For coreboot it will be one linear. Just call
> cbfs_copyfile("bootorder"). BTW why newline separation is important?
Sure, but it'd be nice to just use romfile_copy("bootorder").
Using newline separated just makes it easier for users to vi and/or
cat the file.
-Kevin
^ permalink raw reply [flat|nested] 55+ messages in thread* [Qemu-devel] Re: [PATCHv4 15/15] Pass boot device list to firmware.
2010-11-16 7:22 ` Gleb Natapov
2010-11-16 13:49 ` Kevin O'Connor
@ 2010-11-16 18:19 ` Blue Swirl
1 sibling, 0 replies; 55+ messages in thread
From: Blue Swirl @ 2010-11-16 18:19 UTC (permalink / raw)
To: Gleb Natapov
Cc: kvm, mst, qemu-devel, armbru, alex.williamson, Kevin O'Connor
On Tue, Nov 16, 2010 at 7:22 AM, Gleb Natapov <gleb@redhat.com> wrote:
> On Mon, Nov 15, 2010 at 09:52:19PM -0500, Kevin O'Connor wrote:
>> On Mon, Nov 15, 2010 at 03:36:25PM +0200, Gleb Natapov wrote:
>> > On Mon, Nov 15, 2010 at 08:26:35AM -0500, Kevin O'Connor wrote:
>> > > On Mon, Nov 15, 2010 at 09:40:08AM +0200, Gleb Natapov wrote:
>> > > > On Sun, Nov 14, 2010 at 10:40:33PM -0500, Kevin O'Connor wrote:
>> > > > > Why not just return a newline separated list that is null terminated?
>> > > > >
>> > > > Doing it like this will needlessly complicate firmware side. How do you
>> > > > know how much memory to allocate before reading device list?
>> > >
>> > > My preference would be for the size to be exposed via the
>> > > QEMU_CFG_FILE_DIR selector. (My preference would be for all objects
>> > > in fw_cfg to have entries in QEMU_CFG_FILE_DIR describing their size
>> > > in a reliable manner.)
>> > >
>> > Will interface suggested by Blue will be good for you? The one with two
>> > fw_cfg ids. BOOTINDEX_LEN for len and BOOTINDEX_DATA for device list. I
>>
>> I dislike how different fw_cfg objects pass the length in different
>> ways (eg, QEMU_CFG_E820_TABLE passes length as first 4 bytes). This
>> is a common problem - I'd prefer if we could adopt one uniform way of
>> passing length. I think QEMU_CFG_FILE_DIR solves this problem well.
>>
> Looking at available fw cfg option I see that _SIZE _DATA is also a
> common pattern. The problem with QEMU_CFG_FILE_DIR is that we have very
> little available slots right now. If we a going to require everything to
> use it we better grow number of available slots considerably now while
> it is easily done (no option defined above file slots yet).
FW_CFG_FILE_DIR seems to be a bit poorly designed. Maybe we should
deprecate it and design a more scalable model. There are also string
variables passed to BIOS (-prom-env for Sparc/PPC) which could then
use this new model instead of NVRAM.
> I personally do not have preferences one way or the other. Blue are you
> OK with using QEMU_CFG_FILE_DIR?
That would also work.
>> I also have an ulterior motive here. If the boot order is exposed as
>> a newline separated list via an entry in QEMU_CFG_FILE_DIR, then this
>> becomes free for coreboot users as well. (On coreboot, the boot order
>> could be placed in a "file" in flash with no change to the seabios
>> code.)
>>
> You can define get_boot_order() function and implement it differently
> for qemu and coreboot. For coreboot it will be one linear. Just call
> cbfs_copyfile("bootorder"). BTW why newline separation is important?
Newline and zero are both OK since neither can appear inside a valid boot path.
^ permalink raw reply [flat|nested] 55+ messages in thread