Re: [PATCHv4 15/15] Pass boot device list to firmware.

[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?

> 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.