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

[David Gibson <david@gibson.dropbear.id.au>]

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

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__) */

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]