From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCHv4 15/15] Pass boot device list to firmware. Date: Tue, 16 Nov 2010 16:11:12 +0200 Message-ID: <20101116141112.GS7948@redhat.com> References: <1289749181-12070-1-git-send-email-gleb@redhat.com> <1289749181-12070-16-git-send-email-gleb@redhat.com> <20101115084242.GG7948@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=koi8-r Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org, armbru@redhat.com, alex.williamson@redhat.com, mst@redhat.com, kevin@koconnor.net To: Blue Swirl Return-path: Received: from mx1.redhat.com ([209.132.183.28]:1564 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756272Ab0KPOLS convert rfc822-to-8bit (ORCPT ); Tue, 16 Nov 2010 09:11:18 -0500 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Nov 15, 2010 at 08:29:24PM +0000, Blue Swirl wrote: > 2010/11/15 Gleb Natapov : > > On Sun, Nov 14, 2010 at 10:50:13PM +0000, Blue Swirl wrote: > >> On Sun, Nov 14, 2010 at 3:39 PM, Gleb Natapov wr= ote: > >> > > >> > Signed-off-by: Gleb Natapov > >> > --- > >> > =9Ahw/fw_cfg.c | =9A 14 ++++++++++++++ > >> > =9Ahw/fw_cfg.h | =9A =9A4 +++- > >> > =9Asysemu.h =9A =9A| =9A =9A1 + > >> > =9Avl.c =9A =9A =9A =9A| =9A 51 ++++++++++++++++++++++++++++++++= +++++++++++++++++++ > >> > =9A4 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 { > >> > =9A =9A FWCfgFiles *files; > >> > =9A =9A uint16_t cur_entry; > >> > =9A =9A uint32_t cur_offset; > >> > + =9A =9ANotifier machine_ready; > >> > =9A}; > >> > > >> > =9Astatic void fw_cfg_write(FWCfgState *s, uint8_t value) > >> > @@ -315,6 +316,15 @@ int fw_cfg_add_file(FWCfgState *s, =9Aconst= char *filename, uint8_t *data, > >> > =9A =9A return 1; > >> > =9A} > >> > > >> > +static void fw_cfg_machine_ready(struct Notifier* n) > >> > +{ > >> > + =9A =9Auint32_t len; > >> > + =9A =9Achar *bootindex =3D get_boot_devices_list(&len); > >> > + > >> > + =9A =9Afw_cfg_add_bytes(container_of(n, FWCfgState, machine_re= ady), > >> > + =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A FW_CFG_BOOTINDEX, (uin= t8_t*)bootindex, len); > >> > +} > >> > + > >> > =9AFWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port= , > >> > =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A target_phys_addr= _t ctl_addr, target_phys_addr_t data_addr) > >> > =9A{ > >> > @@ -343,6 +353,10 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, = uint32_t data_port, > >> > =9A =9A fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, (uint16_t)max_cpus); > >> > =9A =9A fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu)= ; > >> > > >> > + > >> > + =9A =9As->machine_ready.notify =3D fw_cfg_machine_ready; > >> > + =9A =9Aqemu_add_machine_init_done_notifier(&s->machine_ready); > >> > + > >> > =9A =9A return s; > >> > =9A} > >> > > >> > 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 @@ > >> > > >> > =9A#define FW_CFG_FILE_FIRST =9A =9A =9A 0x20 > >> > =9A#define FW_CFG_FILE_SLOTS =9A =9A =9A 0x10 > >> > -#define FW_CFG_MAX_ENTRY =9A =9A =9A =9A(FW_CFG_FILE_FIRST+FW_C= =46G_FILE_SLOTS) > >> > +#define FW_CFG_FILE_LAST_SLOT =9A (FW_CFG_FILE_FIRST+FW_CFG_FIL= E_SLOTS) > >> > +#define FW_CFG_BOOTINDEX =9A =9A =9A =9A(FW_CFG_FILE_LAST_SLOT = + 1) > >> > +#define FW_CFG_MAX_ENTRY =9A =9A =9A =9AFW_CFG_BOOTINDEX > >> > >> This should be > >> #define FW_CFG_MAX_ENTRY =9A =9A =9A =9A(FW_CFG_BOOTINDEX + 1) > >> because the check is like this: > >> =9A =9A if ((key & FW_CFG_ENTRY_MASK) >=3D FW_CFG_MAX_ENTRY) { > >> =9A =9A =9A =9A s->cur_entry =3D 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 sp= ec > > it is allowed to abbreviate 1fe00000000 as 1fe,0? Spec allows to dr= op > > starting zeroes but TARGET_FMT_plx definition in targphys.h has 0 a= fter > > %. I can define another one without leading zeroes. Can you suggest > > a name? >=20 > 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 >=20 > 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. >=20 > >=9ATARGET_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 x= 86. > > "pci-ata" should be "ide" according to PCI binding spec :) >=20 > Yes, for example there is no ATA in the Ultra-5 tree but in UltraAX i= t exists: > /pci@1f,4000/ide@3/ata@0,0/cmdk@0,0 >=20 > > 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 =3D { > > > > =9Astatic SysBusDeviceInfo pbm_host_info =3D { > > =9A =9A .qdev.name =3D "pbm", > > + =9A =9A.qdev.fw_name =3D "pci", >=20 > Perhaps the FW path should use device class names if no name is speci= fied. 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. >=20 > I'll try Sparc32 to see how this fits there. -- Gleb.