Re: [Qemu-devel] [PATCH 01/11] spapr_ovec: initial implementation of option vector helpers

[Michael Roth]

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 <address@hidden>
> > Signed-off-by: Michael Roth <address@hidden>
> 
> 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     <address@hidden>
> > + *  Michael Roth      <address@hidden>
> > + *
> > + * 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      <address@hidden>
> > + *
> > + * 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