Re: [Qemu-devel] [PATCH v2 1/6] Use IO port for qemu<->guest BIOS communication.

[Blue Swirl]

On 8/26/08, Gleb Natapov <address@hidden> wrote:
> On Mon, Aug 25, 2008 at 09:27:18PM +0300, Blue Swirl wrote:
>  > >  Blue Swirl: What do you think of switching sparc to use a structure more
>  > > like this?  I do prefer a key-value mechanism verses a blob.  Even with 
> pure
>  > > MMIO, the same results could be achieved by treating the MMIO region as
>  > > registers and using a selector.
>  >
>  > I could switch Sparc to something like this, if the goal is that it
>  > will be used by all targets.
>  >
>  > There should be a .h file which lists the keys and which can be
>  > included from C and asm, like current firmware_abi.h. I'd define an
>  > offset (0x8000?) for architecture-specific keys which need not be in
>  > the same .h file.
>
>
> Is the patch below what you mean? (not tested, but compiles)

Yes, but I'd still put the code from the .h file and pc.c to a new .c
file, only the keys and function prototypes to .h.

> +    int arch = key & FW_CFG_ARCH_LOCAL;
>  +
>  +    key &= (~FW_CFG_ARCH_LOCAL);
>  +
>  +    if (key >= FW_CFG_MAX_ENTRY)
>  +        return 0;

I was actually not thinking to use the arch part like this, but in
your solution, arch part storage is handled same way as others. Great!

> +static FWCfgState *bios_params;

Now that the _read and _write functions use the opaque argument, this
is not needed once allocated.

>  +static uint32_t bios_cfg_read(void *opaque, uint32_t addr)
>  +{
>
> +    FWCfgState *s = opaque;
>  +
>  +    return firmware_cfg_read(s);
>
> +}
>  +
>  +static void bios_cfg_write(void *opaque, uint32_t addr, uint32_t value)
>  +{
>
> +    FWCfgState *s = opaque;
>  +
>  +    firmware_cfg_select(s, (uint16_t)value);
>  +}
>  +

I'd put these to the new .c file.

One problem with this IO method is that it is not SMP safe, because of
the cur_entry selection and reading will not be atomic. Think of boot
CPU waking up all other CPUs with broadcast interrupt and then these
other CPUs all want to know the boot parameters at the same time. The
same could happen with warm reset. Directly mapped ROM doesn't have
this problem, but multiplexed ROM (or Sparc64 nvram used now) is also
unsafe.

I could think of a couple of solutions:
- different IO port for each CPU (not possible on PC)
- per-CPU cur_entry, the device "knows" the CPU index (not very beautiful)
- lock mechanism: before access, the CPU must win lock and only then
it can access the data. But IO ports don't support atomic
instructions? How about some kind of MAC-like protocol? It gets pretty
complex.

Are there other solutions?