[01/11] spapr_ovec: initial implementation of option vector helpers

PAPR guests advertise their capabilities to the platform by passing
an ibm,architecture-vec structure via an
ibm,client-architecture-support hcall as described by LoPAPR v11,
B.6.2.3. during early boot.

Using this information, the platform enables the capabilities it
supports, then encodes a subset of those enabled capabilities (the
5th option vector of the ibm,architecture-vec structure passed to
ibm,client-architecture-support) into the guest device tree via
"/chosen/ibm,architecture-vec-5".

The logical format of these these option vectors is a bit-vector,
where individual bits are addressed/documented based on the byte-wise
offset from the beginning of the bit-vector, followed by the bit-wise
index starting from the byte-wise offset. Thus the bits of each of
these bytes are stored in reverse order. Additionally, the first
byte of each option vector is encodes the length of the option vector,
so byte offsets begin at 1, and bit offset at 0.

This is not very intuitive for the purposes of mapping these bits to
a particular documented capability, so this patch introduces a set
of abstractions that encapsulate the work of parsing/encoding these
options vectors and testing for individual capabilities.

Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/Makefile.objs        |   2 +-
 hw/ppc/spapr_ovec.c         | 244 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr_ovec.h |  62 +++++++++++
 3 files changed, 307 insertions(+), 1 deletion(-)
 create mode 100644 hw/ppc/spapr_ovec.c
 create mode 100644 include/hw/ppc/spapr_ovec.h

On Wed, Oct 12, 2016 at 06:13:49PM -0500, Michael Roth wrote:
> PAPR guests advertise their capabilities to the platform by passing
> an ibm,architecture-vec structure via an
> ibm,client-architecture-support hcall as described by LoPAPR v11,
> B.6.2.3. during early boot.
> 
> Using this information, the platform enables the capabilities it
> supports, then encodes a subset of those enabled capabilities (the
> 5th option vector of the ibm,architecture-vec structure passed to
> ibm,client-architecture-support) into the guest device tree via
> "/chosen/ibm,architecture-vec-5".
> 
> The logical format of these these option vectors is a bit-vector,
> where individual bits are addressed/documented based on the byte-wise
> offset from the beginning of the bit-vector, followed by the bit-wise
> index starting from the byte-wise offset. Thus the bits of each of
> these bytes are stored in reverse order. Additionally, the first
> byte of each option vector is encodes the length of the option vector,
> so byte offsets begin at 1, and bit offset at 0.

Heh.. pity qemu doesn't use the ccan bitmap module
(http://ccodearchive.net/info/bitmap.html).  By design it always
stores the bitmaps in IBM bit number ordering, because that's most
obvious to a human reading a memory dump (for the purpose of bit
vectors - in most situations the IBM numbering is dumb).

> This is not very intuitive for the purposes of mapping these bits to
> a particular documented capability, so this patch introduces a set
> of abstractions that encapsulate the work of parsing/encoding these
> options vectors and testing for individual capabilities.
> 
> Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

A handful of small nits.

> ---
>  hw/ppc/Makefile.objs        |   2 +-
>  hw/ppc/spapr_ovec.c         | 244 ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr_ovec.h |  62 +++++++++++
>  3 files changed, 307 insertions(+), 1 deletion(-)
>  create mode 100644 hw/ppc/spapr_ovec.c
>  create mode 100644 include/hw/ppc/spapr_ovec.h
> 
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index 99a0d4e..2e0b0c9 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -4,7 +4,7 @@ obj-y += ppc.o ppc_booke.o fdt.o
>  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> -obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o
>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>  obj-y += spapr_pci_vfio.o
>  endif
> diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
> new file mode 100644
> index 0000000..ddc19f5
> --- /dev/null
> +++ b/hw/ppc/spapr_ovec.c
> @@ -0,0 +1,244 @@
> +/*
> + * QEMU SPAPR Architecture Option Vector Helper Functions
> + *
> + * Copyright IBM Corp. 2016
> + *
> + * Authors:
> + *  Bharata B Rao     <bharata@linux.vnet.ibm.com>
> + *  Michael Roth      <mdroth@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/ppc/spapr_ovec.h"
> +#include "qemu/bitmap.h"
> +#include "exec/address-spaces.h"
> +#include "qemu/error-report.h"
> +#include <libfdt.h>
> +
> +/* #define DEBUG_SPAPR_OVEC */
> +
> +#ifdef DEBUG_SPAPR_OVEC
> +#define DPRINTFN(fmt, ...) \
> +    do { fprintf(stderr, fmt "\n", ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTFN(fmt, ...) \
> +    do { } while (0)
> +#endif
> +
> +#define OV_MAXBYTES 256 /* not including length byte */
> +#define OV_MAXBITS (OV_MAXBYTES * BITS_PER_BYTE)
> +
> +/* we *could* work with bitmaps directly, but handling the bitmap privately
> + * allows us to more safely make assumptions about the bitmap size and
> + * simplify the calling code somewhat
> + */
> +struct sPAPROptionVector {
> +    unsigned long *bitmap;
> +};
> +
> +static sPAPROptionVector *spapr_ovec_from_bitmap(unsigned long *bitmap)
> +{
> +    sPAPROptionVector *ov;
> +
> +    g_assert(bitmap);
> +
> +    ov = g_new0(sPAPROptionVector, 1);
> +    ov->bitmap = bitmap;
> +
> +    return ov;
> +}
> +
> +sPAPROptionVector *spapr_ovec_new(void)
> +{
> +    return spapr_ovec_from_bitmap(bitmap_new(OV_MAXBITS));
> +}
> +
> +sPAPROptionVector *spapr_ovec_clone(sPAPROptionVector *ov_orig)
> +{
> +    sPAPROptionVector *ov;
> +
> +    g_assert(ov_orig);
> +
> +    ov = spapr_ovec_new();
> +    bitmap_copy(ov->bitmap, ov_orig->bitmap, OV_MAXBITS);
> +
> +    return ov;
> +}
> +
> +void spapr_ovec_intersect(sPAPROptionVector *ov,
> +                          sPAPROptionVector *ov1,
> +                          sPAPROptionVector *ov2)
> +{
> +    g_assert(ov);
> +    g_assert(ov1);
> +    g_assert(ov2);
> +
> +    bitmap_and(ov->bitmap, ov1->bitmap, ov2->bitmap, OV_MAXBITS);
> +}
> +
> +/* returns true if options bits were removed, false otherwise */
> +bool spapr_ovec_diff(sPAPROptionVector *ov,
> +                     sPAPROptionVector *ov_old,
> +                     sPAPROptionVector *ov_new)
> +{
> +    unsigned long *change_mask = bitmap_new(OV_MAXBITS);
> +    unsigned long *removed_bits = bitmap_new(OV_MAXBITS);
> +    bool bits_were_removed = false;
> +
> +    g_assert(ov);
> +    g_assert(ov_old);
> +    g_assert(ov_new);
> +
> +    bitmap_xor(change_mask, ov_old->bitmap, ov_new->bitmap, OV_MAXBITS);
> +    bitmap_and(ov->bitmap, ov_new->bitmap, change_mask, OV_MAXBITS);
> +    bitmap_and(removed_bits, ov_old->bitmap, change_mask, OV_MAXBITS);
> +
> +    if (!bitmap_empty(removed_bits, OV_MAXBITS)) {
> +        bits_were_removed = true;
> +    }
> +
> +    g_free(change_mask);
> +    g_free(removed_bits);
> +
> +    return bits_were_removed;
> +}
> +
> +void spapr_ovec_cleanup(sPAPROptionVector *ov)
> +{
> +    if (ov) {
> +        g_free(ov->bitmap);
> +        g_free(ov);
> +    }
> +}
> +
> +void spapr_ovec_set(sPAPROptionVector *ov, long bitnr)
> +{
> +    g_assert(ov);
> +    g_assert_cmpint(bitnr, <, OV_MAXBITS);
> +
> +    set_bit(bitnr, ov->bitmap);
> +}
> +
> +void spapr_ovec_clear(sPAPROptionVector *ov, long bitnr)
> +{
> +    g_assert(ov);
> +    g_assert_cmpint(bitnr, <, OV_MAXBITS);
> +
> +    clear_bit(bitnr, ov->bitmap);
> +}
> +
> +bool spapr_ovec_test(sPAPROptionVector *ov, long bitnr)
> +{
> +    g_assert(ov);
> +    g_assert_cmpint(bitnr, <, OV_MAXBITS);
> +
> +    return test_bit(bitnr, ov->bitmap) ? true : false;
> +}
> +
> +static void guest_byte_to_bitmap(uint8_t entry, unsigned long *bitmap,
> +                                 long bitmap_offset)
> +{
> +    int i;
> +
> +    for (i = 0; i < BITS_PER_BYTE; i++) {
> +        if (entry & (1 << (BITS_PER_BYTE - 1 - i))) {
> +            bitmap_set(bitmap, bitmap_offset + i, 1);
> +        }
> +    }
> +}
> +
> +static uint8_t guest_byte_from_bitmap(unsigned long *bitmap, long bitmap_offset)
> +{
> +    uint8_t entry = 0;
> +    int i;
> +
> +    for (i = 0; i < BITS_PER_BYTE; i++) {
> +        if (test_bit(bitmap_offset + i, bitmap)) {
> +            entry |= (1 << (BITS_PER_BYTE - 1 - i));
> +        }
> +    }
> +
> +    return entry;
> +}
> +
> +static target_ulong vector_addr(target_ulong table_addr, int vector)
> +{
> +    uint16_t vector_count, vector_len;
> +    int i;
> +
> +    vector_count = ldub_phys(&address_space_memory, table_addr) + 1;
> +    if (vector > vector_count) {
> +        return 0;
> +    }
> +    table_addr++; /* skip nr option vectors */
> +
> +    for (i = 0; i < vector - 1; i++) {
> +        vector_len = ldub_phys(&address_space_memory, table_addr) + 2;
> +        table_addr += vector_len;
> +    }
> +    return table_addr;
> +}
> +
> +sPAPROptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector)
> +{
> +    unsigned long *bitmap;
> +    target_ulong addr;
> +    uint16_t vector_len;
> +    int i;
> +
> +    g_assert(table_addr);
> +    g_assert_cmpint(vector, >=, 1); /* vector numbering starts at 1 */
> +
> +    addr = vector_addr(table_addr, vector);
> +    if (!addr) {
> +        /* specified vector isn't present */
> +        return NULL;
> +    }
> +
> +    vector_len = ldub_phys(&address_space_memory, addr++) + 1;

Here you use vector_len to be the number of bytes _not_ including the
length byte, but in other places you use the same name including the
length byte, which is a litle confusing.

> +    if (vector_len >= OV_MAXBYTES) {

Do you mean >= here, or >?  If so, what's wrong with vector_len ==
256, I thought that was explicitly permitted in the encoding?  If not,
then there's no need for the test since a byte load + 1 can't possibly
exceed 256 (you could have an assert if you want).

> +        error_report("guest option vector length %i exceeds max of %i",
> +                     vector_len, OV_MAXBYTES);
> +    }
> +    bitmap = bitmap_new(OV_MAXBITS);
> +
> +    for (i = 0; i < vector_len; i++) {
> +        uint8_t entry = ldub_phys(&address_space_memory, addr + i);
> +        if (entry) {
> +            DPRINTFN("read guest vector %2d, byte %3d / %3d: 0x%.2x",
> +                     vector, i + 1, vector_len, entry);
> +            guest_byte_to_bitmap(entry, bitmap, i * BITS_PER_BYTE);
> +        }
> +    }
> +
> +    return spapr_ovec_from_bitmap(bitmap);

This is the only caller of spapr_ovec_from_bitmap().  You could
equally well just use ovec_new() here and reach in to populate the
bitmap.  Means you don't need to expose spapr_ovec_from_bitmap() which
is only safe if the supplied bitmap is the right size.

> +}
> +
> +int spapr_ovec_populate_dt(void *fdt, int fdt_offset,
> +                           sPAPROptionVector *ov, const char *name)
> +{
> +    uint8_t vec[OV_MAXBYTES + 1];
> +    uint16_t vec_len;
> +    unsigned long lastbit;
> +    int i;
> +
> +    g_assert(ov);
> +
> +    lastbit = MIN(find_last_bit(ov->bitmap, OV_MAXBITS), OV_MAXBITS - 1);
> +    vec_len = lastbit / BITS_PER_BYTE + 2;

If no bits are set at all, find_last_bit() will return 2048, which
means you'll include a max size vector when you actually want a
minimum size vector.

> +    g_assert_cmpint(vec_len - 2, <=, UINT8_MAX);
> +    vec[0] = vec_len - 2; /* guest expects length encoded as n - 2 */
> +
> +    for (i = 1; i < vec_len; i++) {
> +        vec[i] = guest_byte_from_bitmap(ov->bitmap, (i - 1) * BITS_PER_BYTE);
> +        if (vec[i]) {
> +            DPRINTFN("encoding guest vector byte %3d / %3d: 0x%.2x",
> +                     i, vec_len, vec[i]);
> +        }
> +    }
> +
> +    return fdt_setprop(fdt, fdt_offset, name, vec, vec_len);
> +}
> diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
> new file mode 100644
> index 0000000..fba2d98
> --- /dev/null
> +++ b/include/hw/ppc/spapr_ovec.h
> @@ -0,0 +1,62 @@
> +/*
> + * QEMU SPAPR Option/Architecture Vector Definitions
> + *
> + * Each architecture option is organized/documented by the following
> + * in LoPAPR 1.1, Table 244:
> + *
> + *   <vector number>: the bit-vector in which the option is located
> + *   <vector byte>: the byte offset of the vector entry
> + *   <vector bit>: the bit offset within the vector entry
> + *
> + * where each vector entry can be one or more bytes.
> + *
> + * Firmware expects a somewhat literal encoding of this bit-vector
> + * structure, where each entry is stored in little-endian so that the
> + * byte ordering reflects that of the documentation, but where each bit
> + * offset is from "left-to-right" in the traditional representation of
> + * a byte value where the MSB is the left-most bit. Thus, each
> + * individual byte encodes the option bits in reverse order of the
> + * documented bit.
> + *
> + * These definitions/helpers attempt to abstract away this internal
> + * representation so that we can define/set/test for individual option
> + * bits using only the documented values. This is done mainly by relying
> + * on a bitmap to approximate the documented "bit-vector" structure and
> + * handling conversations to-from the internal representation under the
> + * covers.
> + *
> + * Copyright IBM Corp. 2016
> + *
> + * Authors:
> + *  Michael Roth      <mdroth@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#if !defined(__HW_SPAPR_OPTION_VECTORS_H__)
> +#define __HW_SPAPR_OPTION_VECTORS_H__
> +
> +#include "cpu.h"
> +
> +typedef struct sPAPROptionVector sPAPROptionVector;
> +
> +#define OV_BIT(byte, bit) ((byte - 1) * BITS_PER_BYTE + bit)
> +
> +/* interfaces */
> +sPAPROptionVector *spapr_ovec_new(void);
> +sPAPROptionVector *spapr_ovec_clone(sPAPROptionVector *ov_orig);
> +void spapr_ovec_intersect(sPAPROptionVector *ov,
> +                          sPAPROptionVector *ov1,
> +                          sPAPROptionVector *ov2);
> +bool spapr_ovec_diff(sPAPROptionVector *ov,
> +                     sPAPROptionVector *ov_old,
> +                     sPAPROptionVector *ov_new);
> +void spapr_ovec_cleanup(sPAPROptionVector *ov);
> +void spapr_ovec_set(sPAPROptionVector *ov, long bitnr);
> +void spapr_ovec_clear(sPAPROptionVector *ov, long bitnr);
> +bool spapr_ovec_test(sPAPROptionVector *ov, long bitnr);
> +sPAPROptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector);
> +int spapr_ovec_populate_dt(void *fdt, int fdt_offset,
> +                           sPAPROptionVector *ov, const char *name);
> +
> +#endif /* !defined (__HW_SPAPR_OPTION_VECTORS_H__) */

Quoting David Gibson (2016-10-13 21:39:19)
> On Wed, Oct 12, 2016 at 06:13:49PM -0500, Michael Roth wrote:
> > PAPR guests advertise their capabilities to the platform by passing
> > an ibm,architecture-vec structure via an
> > ibm,client-architecture-support hcall as described by LoPAPR v11,
> > B.6.2.3. during early boot.
> > 
> > Using this information, the platform enables the capabilities it
> > supports, then encodes a subset of those enabled capabilities (the
> > 5th option vector of the ibm,architecture-vec structure passed to
> > ibm,client-architecture-support) into the guest device tree via
> > "/chosen/ibm,architecture-vec-5".
> > 
> > The logical format of these these option vectors is a bit-vector,
> > where individual bits are addressed/documented based on the byte-wise
> > offset from the beginning of the bit-vector, followed by the bit-wise
> > index starting from the byte-wise offset. Thus the bits of each of
> > these bytes are stored in reverse order. Additionally, the first
> > byte of each option vector is encodes the length of the option vector,
> > so byte offsets begin at 1, and bit offset at 0.
> 
> Heh.. pity qemu doesn't use the ccan bitmap module
> (http://ccodearchive.net/info/bitmap.html).  By design it always
> stores the bitmaps in IBM bit number ordering, because that's most
> obvious to a human reading a memory dump (for the purpose of bit
> vectors - in most situations the IBM numbering is dumb).
> 
> > This is not very intuitive for the purposes of mapping these bits to
> > a particular documented capability, so this patch introduces a set
> > of abstractions that encapsulate the work of parsing/encoding these
> > options vectors and testing for individual capabilities.
> > 
> > Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> A handful of small nits.
> 
> > ---
> >  hw/ppc/Makefile.objs        |   2 +-
> >  hw/ppc/spapr_ovec.c         | 244 ++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr_ovec.h |  62 +++++++++++
> >  3 files changed, 307 insertions(+), 1 deletion(-)
> >  create mode 100644 hw/ppc/spapr_ovec.c
> >  create mode 100644 include/hw/ppc/spapr_ovec.h
> > 
> > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > index 99a0d4e..2e0b0c9 100644
> > --- a/hw/ppc/Makefile.objs
> > +++ b/hw/ppc/Makefile.objs
> > @@ -4,7 +4,7 @@ obj-y += ppc.o ppc_booke.o fdt.o
> >  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> > -obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o spapr_ovec.o
> >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> >  obj-y += spapr_pci_vfio.o
> >  endif
> > diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
> > new file mode 100644
> > index 0000000..ddc19f5
> > --- /dev/null
> > +++ b/hw/ppc/spapr_ovec.c
> > @@ -0,0 +1,244 @@
> > +/*
> > + * QEMU SPAPR Architecture Option Vector Helper Functions
> > + *
> > + * Copyright IBM Corp. 2016
> > + *
> > + * Authors:
> > + *  Bharata B Rao     <bharata@linux.vnet.ibm.com>
> > + *  Michael Roth      <mdroth@linux.vnet.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/ppc/spapr_ovec.h"
> > +#include "qemu/bitmap.h"
> > +#include "exec/address-spaces.h"
> > +#include "qemu/error-report.h"
> > +#include <libfdt.h>
> > +
> > +/* #define DEBUG_SPAPR_OVEC */
> > +
> > +#ifdef DEBUG_SPAPR_OVEC
> > +#define DPRINTFN(fmt, ...) \
> > +    do { fprintf(stderr, fmt "\n", ## __VA_ARGS__); } while (0)
> > +#else
> > +#define DPRINTFN(fmt, ...) \
> > +    do { } while (0)
> > +#endif
> > +
> > +#define OV_MAXBYTES 256 /* not including length byte */
> > +#define OV_MAXBITS (OV_MAXBYTES * BITS_PER_BYTE)
> > +
> > +/* we *could* work with bitmaps directly, but handling the bitmap privately
> > + * allows us to more safely make assumptions about the bitmap size and
> > + * simplify the calling code somewhat
> > + */
> > +struct sPAPROptionVector {
> > +    unsigned long *bitmap;
> > +};
> > +
> > +static sPAPROptionVector *spapr_ovec_from_bitmap(unsigned long *bitmap)
> > +{
> > +    sPAPROptionVector *ov;
> > +
> > +    g_assert(bitmap);
> > +
> > +    ov = g_new0(sPAPROptionVector, 1);
> > +    ov->bitmap = bitmap;
> > +
> > +    return ov;
> > +}
> > +
> > +sPAPROptionVector *spapr_ovec_new(void)
> > +{
> > +    return spapr_ovec_from_bitmap(bitmap_new(OV_MAXBITS));
> > +}
> > +
> > +sPAPROptionVector *spapr_ovec_clone(sPAPROptionVector *ov_orig)
> > +{
> > +    sPAPROptionVector *ov;
> > +
> > +    g_assert(ov_orig);
> > +
> > +    ov = spapr_ovec_new();
> > +    bitmap_copy(ov->bitmap, ov_orig->bitmap, OV_MAXBITS);
> > +
> > +    return ov;
> > +}
> > +
> > +void spapr_ovec_intersect(sPAPROptionVector *ov,
> > +                          sPAPROptionVector *ov1,
> > +                          sPAPROptionVector *ov2)
> > +{
> > +    g_assert(ov);
> > +    g_assert(ov1);
> > +    g_assert(ov2);
> > +
> > +    bitmap_and(ov->bitmap, ov1->bitmap, ov2->bitmap, OV_MAXBITS);
> > +}
> > +
> > +/* returns true if options bits were removed, false otherwise */
> > +bool spapr_ovec_diff(sPAPROptionVector *ov,
> > +                     sPAPROptionVector *ov_old,
> > +                     sPAPROptionVector *ov_new)
> > +{
> > +    unsigned long *change_mask = bitmap_new(OV_MAXBITS);
> > +    unsigned long *removed_bits = bitmap_new(OV_MAXBITS);
> > +    bool bits_were_removed = false;
> > +
> > +    g_assert(ov);
> > +    g_assert(ov_old);
> > +    g_assert(ov_new);
> > +
> > +    bitmap_xor(change_mask, ov_old->bitmap, ov_new->bitmap, OV_MAXBITS);
> > +    bitmap_and(ov->bitmap, ov_new->bitmap, change_mask, OV_MAXBITS);
> > +    bitmap_and(removed_bits, ov_old->bitmap, change_mask, OV_MAXBITS);
> > +
> > +    if (!bitmap_empty(removed_bits, OV_MAXBITS)) {
> > +        bits_were_removed = true;
> > +    }
> > +
> > +    g_free(change_mask);
> > +    g_free(removed_bits);
> > +
> > +    return bits_were_removed;
> > +}
> > +
> > +void spapr_ovec_cleanup(sPAPROptionVector *ov)
> > +{
> > +    if (ov) {
> > +        g_free(ov->bitmap);
> > +        g_free(ov);
> > +    }
> > +}
> > +
> > +void spapr_ovec_set(sPAPROptionVector *ov, long bitnr)
> > +{
> > +    g_assert(ov);
> > +    g_assert_cmpint(bitnr, <, OV_MAXBITS);
> > +
> > +    set_bit(bitnr, ov->bitmap);
> > +}
> > +
> > +void spapr_ovec_clear(sPAPROptionVector *ov, long bitnr)
> > +{
> > +    g_assert(ov);
> > +    g_assert_cmpint(bitnr, <, OV_MAXBITS);
> > +
> > +    clear_bit(bitnr, ov->bitmap);
> > +}
> > +
> > +bool spapr_ovec_test(sPAPROptionVector *ov, long bitnr)
> > +{
> > +    g_assert(ov);
> > +    g_assert_cmpint(bitnr, <, OV_MAXBITS);
> > +
> > +    return test_bit(bitnr, ov->bitmap) ? true : false;
> > +}
> > +
> > +static void guest_byte_to_bitmap(uint8_t entry, unsigned long *bitmap,
> > +                                 long bitmap_offset)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < BITS_PER_BYTE; i++) {
> > +        if (entry & (1 << (BITS_PER_BYTE - 1 - i))) {
> > +            bitmap_set(bitmap, bitmap_offset + i, 1);
> > +        }
> > +    }
> > +}
> > +
> > +static uint8_t guest_byte_from_bitmap(unsigned long *bitmap, long bitmap_offset)
> > +{
> > +    uint8_t entry = 0;
> > +    int i;
> > +
> > +    for (i = 0; i < BITS_PER_BYTE; i++) {
> > +        if (test_bit(bitmap_offset + i, bitmap)) {
> > +            entry |= (1 << (BITS_PER_BYTE - 1 - i));
> > +        }
> > +    }
> > +
> > +    return entry;
> > +}
> > +
> > +static target_ulong vector_addr(target_ulong table_addr, int vector)
> > +{
> > +    uint16_t vector_count, vector_len;
> > +    int i;
> > +
> > +    vector_count = ldub_phys(&address_space_memory, table_addr) + 1;
> > +    if (vector > vector_count) {
> > +        return 0;
> > +    }
> > +    table_addr++; /* skip nr option vectors */
> > +
> > +    for (i = 0; i < vector - 1; i++) {
> > +        vector_len = ldub_phys(&address_space_memory, table_addr) + 2;
> > +        table_addr += vector_len;
> > +    }
> > +    return table_addr;
> > +}
> > +
> > +sPAPROptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector)
> > +{
> > +    unsigned long *bitmap;
> > +    target_ulong addr;
> > +    uint16_t vector_len;
> > +    int i;
> > +
> > +    g_assert(table_addr);
> > +    g_assert_cmpint(vector, >=, 1); /* vector numbering starts at 1 */
> > +
> > +    addr = vector_addr(table_addr, vector);
> > +    if (!addr) {
> > +        /* specified vector isn't present */
> > +        return NULL;
> > +    }
> > +
> > +    vector_len = ldub_phys(&address_space_memory, addr++) + 1;
> 
> Here you use vector_len to be the number of bytes _not_ including the
> length byte, but in other places you use the same name including the
> length byte, which is a litle confusing.

True, the additional offset to account for the length byte should be added
to the table_addr in vector_addr() rather than the vector_len beforehand.
Will fix.

> 
> > +    if (vector_len >= OV_MAXBYTES) {
> 
> Do you mean >= here, or >?  If so, what's wrong with vector_len ==
> 256, I thought that was explicitly permitted in the encoding?  If not,

Yes, you're right, that should be vector_len > OV_MAXBYTES. I documented
this in OV_MAXBYTES define but still managed to screw it up.

> then there's no need for the test since a byte load + 1 can't possibly
> exceed 256 (you could have an assert if you want).

Good point, will switch it to an assert.

> 
> > +        error_report("guest option vector length %i exceeds max of %i",
> > +                     vector_len, OV_MAXBYTES);
> > +    }
> > +    bitmap = bitmap_new(OV_MAXBITS);
> > +
> > +    for (i = 0; i < vector_len; i++) {
> > +        uint8_t entry = ldub_phys(&address_space_memory, addr + i);
> > +        if (entry) {
> > +            DPRINTFN("read guest vector %2d, byte %3d / %3d: 0x%.2x",
> > +                     vector, i + 1, vector_len, entry);
> > +            guest_byte_to_bitmap(entry, bitmap, i * BITS_PER_BYTE);
> > +        }
> > +    }
> > +
> > +    return spapr_ovec_from_bitmap(bitmap);
> 
> This is the only caller of spapr_ovec_from_bitmap().  You could
> equally well just use ovec_new() here and reach in to populate the
> bitmap.  Means you don't need to expose spapr_ovec_from_bitmap() which
> is only safe if the supplied bitmap is the right size.

Yes, we're already using internal knowledge when sizing the bitmap.
spapr_ovec_from_bitmap() might still be safer if we could actually
assert the bitmap size, but since we can't it's probably not useful.
Will drop it.

> 
> > +}
> > +
> > +int spapr_ovec_populate_dt(void *fdt, int fdt_offset,
> > +                           sPAPROptionVector *ov, const char *name)
> > +{
> > +    uint8_t vec[OV_MAXBYTES + 1];
> > +    uint16_t vec_len;
> > +    unsigned long lastbit;
> > +    int i;
> > +
> > +    g_assert(ov);
> > +
> > +    lastbit = MIN(find_last_bit(ov->bitmap, OV_MAXBITS), OV_MAXBITS - 1);
> > +    vec_len = lastbit / BITS_PER_BYTE + 2;
> 
> If no bits are set at all, find_last_bit() will return 2048, which
> means you'll include a max size vector when you actually want a
> minimum size vector.

Ah, missed that. Will fix that and address the additional case of
inconsistent vector_len meaning here.

> 
> > +    g_assert_cmpint(vec_len - 2, <=, UINT8_MAX);
> > +    vec[0] = vec_len - 2; /* guest expects length encoded as n - 2 */
> > +
> > +    for (i = 1; i < vec_len; i++) {
> > +        vec[i] = guest_byte_from_bitmap(ov->bitmap, (i - 1) * BITS_PER_BYTE);
> > +        if (vec[i]) {
> > +            DPRINTFN("encoding guest vector byte %3d / %3d: 0x%.2x",
> > +                     i, vec_len, vec[i]);
> > +        }
> > +    }
> > +
> > +    return fdt_setprop(fdt, fdt_offset, name, vec, vec_len);
> > +}
> > diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
> > new file mode 100644
> > index 0000000..fba2d98
> > --- /dev/null
> > +++ b/include/hw/ppc/spapr_ovec.h
> > @@ -0,0 +1,62 @@
> > +/*
> > + * QEMU SPAPR Option/Architecture Vector Definitions
> > + *
> > + * Each architecture option is organized/documented by the following
> > + * in LoPAPR 1.1, Table 244:
> > + *
> > + *   <vector number>: the bit-vector in which the option is located
> > + *   <vector byte>: the byte offset of the vector entry
> > + *   <vector bit>: the bit offset within the vector entry
> > + *
> > + * where each vector entry can be one or more bytes.
> > + *
> > + * Firmware expects a somewhat literal encoding of this bit-vector
> > + * structure, where each entry is stored in little-endian so that the
> > + * byte ordering reflects that of the documentation, but where each bit
> > + * offset is from "left-to-right" in the traditional representation of
> > + * a byte value where the MSB is the left-most bit. Thus, each
> > + * individual byte encodes the option bits in reverse order of the
> > + * documented bit.
> > + *
> > + * These definitions/helpers attempt to abstract away this internal
> > + * representation so that we can define/set/test for individual option
> > + * bits using only the documented values. This is done mainly by relying
> > + * on a bitmap to approximate the documented "bit-vector" structure and
> > + * handling conversations to-from the internal representation under the
> > + * covers.
> > + *
> > + * Copyright IBM Corp. 2016
> > + *
> > + * Authors:
> > + *  Michael Roth      <mdroth@linux.vnet.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +#if !defined(__HW_SPAPR_OPTION_VECTORS_H__)
> > +#define __HW_SPAPR_OPTION_VECTORS_H__
> > +
> > +#include "cpu.h"
> > +
> > +typedef struct sPAPROptionVector sPAPROptionVector;
> > +
> > +#define OV_BIT(byte, bit) ((byte - 1) * BITS_PER_BYTE + bit)
> > +
> > +/* interfaces */
> > +sPAPROptionVector *spapr_ovec_new(void);
> > +sPAPROptionVector *spapr_ovec_clone(sPAPROptionVector *ov_orig);
> > +void spapr_ovec_intersect(sPAPROptionVector *ov,
> > +                          sPAPROptionVector *ov1,
> > +                          sPAPROptionVector *ov2);
> > +bool spapr_ovec_diff(sPAPROptionVector *ov,
> > +                     sPAPROptionVector *ov_old,
> > +                     sPAPROptionVector *ov_new);
> > +void spapr_ovec_cleanup(sPAPROptionVector *ov);
> > +void spapr_ovec_set(sPAPROptionVector *ov, long bitnr);
> > +void spapr_ovec_clear(sPAPROptionVector *ov, long bitnr);
> > +bool spapr_ovec_test(sPAPROptionVector *ov, long bitnr);
> > +sPAPROptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector);
> > +int spapr_ovec_populate_dt(void *fdt, int fdt_offset,
> > +                           sPAPROptionVector *ov, const char *name);
> > +
> > +#endif /* !defined (__HW_SPAPR_OPTION_VECTORS_H__) */
> 
> -- 
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>                                 | _way_ _around_!
> http://www.ozlabs.org/~dgibson