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

["Blue Swirl" <blauwirbel@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1686 bytes --]

On 8/31/08, Gleb Natapov <gleb@qumranet.com> wrote:
> On Sun, Aug 31, 2008 at 03:45:36PM +0300, Blue Swirl wrote:
>  > >  > I made an updated version of the patch 1/6, with explicit little
>  > >  > endian conversions. I'm not very happy with that. Another way would be
>  > >  > to add functions just to put different size numbers into device and
>  > >  > they would hide the conversion.
>  > >  >
>  > >
>  > > So what approach we should go with? We can go with the second one (add
>  > >  functions for each type) and extend interface as needed.
>  >
>  > This version adds the functions, now the interface is much better. I
>  > updated the other patches too.
>
>
> > +static int fw_cfg_select(FWCfgState *s, uint16_t key)
>  > +{
>  > +    int ret;
>  > +
>  > +    s->cur_offset = 0;
>  > +    if ((key & ~FW_CFG_ARCH_LOCAL) >= FW_CFG_MAX_ENTRY) {
>
> > +        s->cur_entry = 0;
>  Here we select valid entry on incorrect input. Perhaps set 0xffff here
>  and return 0 on read if cur_entry == 0xffff ?
>
>
>  > +static uint8_t fw_cfg_read(FWCfgState *s)
>  > +{
>  > +    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
>
> > +    FWCfgEntry *e = &s->entries[arch][s->cur_entry];
>  This should be:
>
>         FWCfgEntry *e = &s->entries[arch][s->cur_entry & ~FW_CFG_ARCH_LOCAL];
>
>  > +    uint8_t ret;
>  > +
>
> > +    if (!e || !e->data || s->cur_offset >= e->len)
>  Like this:
>
>        if (s->cur_entry == 0xffff || !e->data || s->cur_offset >= e->len)
>
>  > +        ret = 0;
>  > +    else
>  > +        ret = e->data[s->cur_offset++];
>  > +
>  > +    FW_CFG_DPRINTF("read %d\n", ret);
>  > +
>  > +    return ret;
>  > +}
>  > +

Right, here's a fixed version.

[-- Attachment #2: new_fw_abi.diff --]
[-- Type: plain/text, Size: 11963 bytes --]