Re: [Qemu-devel] [PATCH 02/11] spapr_hcall: use spapr_ovec_* interfaces for CAS options

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

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

On Wed, Oct 12, 2016 at 06:13:50PM -0500, Michael Roth wrote:
> Currently we access individual bytes of an option vector via
> ldub_phys() to test for the presence of a particular capability
> within that byte. Currently this is only done for the "dynamic
> reconfiguration memory" capability bit. If that bit is present,
> we pass a boolean value to spapr_h_cas_compose_response()
> to generate a modified device tree segment with the additional
> properties required to enable this functionality.
> 
> As more capability bits are added, will would need to modify the
> code to add additional option vector accesses and extend the
> param list for spapr_h_cas_compose_response() to include similar
> boolean values for these parameters.
> 
> Avoid this by switching to spapr_ovec_* helpers so we can do all
> the parsing in one shot and then test for these additional bits
> within spapr_h_cas_compose_response() directly.
> 
> Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr.c              | 10 ++++++--
>  hw/ppc/spapr_hcall.c        | 56 ++++++++++++---------------------------------
>  include/hw/ppc/spapr.h      |  5 +++-
>  include/hw/ppc/spapr_ovec.h |  3 +++
>  4 files changed, 30 insertions(+), 44 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 03e3803..934d6b2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -856,7 +856,7 @@ out:
>  
>  int spapr_h_cas_compose_response(sPAPRMachineState *spapr,
>                                   target_ulong addr, target_ulong size,
> -                                 bool cpu_update, bool memory_update)
> +                                 bool cpu_update)
>  {
>      void *fdt, *fdt_skel;
>      sPAPRDeviceTreeUpdateHeader hdr = { .version_id = 1 };
> @@ -880,7 +880,8 @@ int spapr_h_cas_compose_response(sPAPRMachineState *spapr,
>      }
>  
>      /* Generate ibm,dynamic-reconfiguration-memory node if required */
> -    if (memory_update && smc->dr_lmb_enabled) {
> +    if (spapr_ovec_test(spapr->ov5_cas, OV5_DRCONF_MEMORY)) {
> +        g_assert(smc->dr_lmb_enabled);
>          _FDT((spapr_populate_drconf_memory(spapr, fdt)));
>      }
>  
> @@ -1769,7 +1770,12 @@ static void ppc_spapr_init(MachineState *machine)
>                                     DIV_ROUND_UP(max_cpus * smt, smp_threads),
>                                     XICS_IRQS_SPAPR, &error_fatal);
>  
> +    /* Set up containers for ibm,client-set-architecture negotiated options */
> +    spapr->ov5 = spapr_ovec_new();
> +    spapr->ov5_cas = spapr_ovec_new();
> +
>      if (smc->dr_lmb_enabled) {
> +        spapr_ovec_set(spapr->ov5, OV5_DRCONF_MEMORY);
>          spapr_validate_node_memory(machine, &error_fatal);
>      }
>  
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index c5e7e8c..f1d081b 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -11,6 +11,7 @@
>  #include "trace.h"
>  #include "sysemu/kvm.h"
>  #include "kvm_ppc.h"
> +#include "hw/ppc/spapr_ovec.h"
>  
>  struct SPRSyncState {
>      int spr;
> @@ -880,32 +881,6 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      return ret;
>  }
>  
> -/*
> - * Return the offset to the requested option vector @vector in the
> - * option vector table @table.
> - */
> -static target_ulong cas_get_option_vector(int vector, target_ulong table)
> -{
> -    int i;
> -    char nr_vectors, nr_entries;
> -
> -    if (!table) {
> -        return 0;
> -    }
> -
> -    nr_vectors = (ldl_phys(&address_space_memory, table) >> 24) + 1;
> -    if (!vector || vector > nr_vectors) {
> -        return 0;
> -    }
> -    table++; /* skip nr option vectors */
> -
> -    for (i = 0; i < vector - 1; i++) {
> -        nr_entries = ldl_phys(&address_space_memory, table) >> 24;
> -        table += nr_entries + 2;
> -    }
> -    return table;
> -}
> -
>  typedef struct {
>      uint32_t cpu_version;
>      Error *err;
> @@ -961,23 +936,21 @@ static void cas_handle_compat_cpu(PowerPCCPUClass *pcc, uint32_t pvr,
>      }
>  }
>  
> -#define OV5_DRCONF_MEMORY 0x20
> -
>  static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
>                                                    sPAPRMachineState *spapr,
>                                                    target_ulong opcode,
>                                                    target_ulong *args)
>  {
>      target_ulong list = ppc64_phys_to_real(args[0]);
> -    target_ulong ov_table, ov5;
> +    target_ulong ov_table;
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu_);
>      CPUState *cs;
> -    bool cpu_match = false, cpu_update = true, memory_update = false;
> +    bool cpu_match = false, cpu_update = true;
>      unsigned old_cpu_version = cpu_->cpu_version;
>      unsigned compat_lvl = 0, cpu_version = 0;
>      unsigned max_lvl = get_compat_level(cpu_->max_compat);
>      int counter;
> -    char ov5_byte2;
> +    sPAPROptionVector *ov5_guest;
>  
>      /* Parse PVR list */
>      for (counter = 0; counter < 512; ++counter) {
> @@ -1033,19 +1006,20 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
>      /* For the future use: here @ov_table points to the first option vector */
>      ov_table = list;
>  
> -    ov5 = cas_get_option_vector(5, ov_table);
> -    if (!ov5) {
> -        return H_SUCCESS;
> -    }
> +    ov5_guest = spapr_ovec_parse_vector(ov_table, 5);
>  
> -    /* @list now points to OV 5 */
> -    ov5_byte2 = ldub_phys(&address_space_memory, ov5 + 2);
> -    if (ov5_byte2 & OV5_DRCONF_MEMORY) {
> -        memory_update = true;
> -    }
> +    /* NOTE: there are actually a number of ov5 bits where input from the
> +     * guest is always zero, and the platform/QEMU enables them independently
> +     * of guest input. To model these properly we'd want some sort of mask,
> +     * but since they only currently apply to memory migration as defined
> +     * by LoPAPR 1.1, 14.5.4.8, which QEMU doesn't implement, we don't need
> +     * to worry about this.
> +     */
> +    spapr_ovec_intersect(spapr->ov5_cas, spapr->ov5, ov5_guest);
> +    spapr_ovec_cleanup(ov5_guest);
>  
>      if (spapr_h_cas_compose_response(spapr, args[1], args[2],
> -                                     cpu_update, memory_update)) {
> +                                     cpu_update)) {
>          qemu_system_reset_request();
>      }
>  
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 39dadaa..6c20d28 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -6,6 +6,7 @@
>  #include "hw/ppc/xics.h"
>  #include "hw/ppc/spapr_drc.h"
>  #include "hw/mem/pc-dimm.h"
> +#include "hw/ppc/spapr_ovec.h"
>  
>  struct VIOsPAPRBus;
>  struct sPAPRPHBState;
> @@ -66,6 +67,8 @@ struct sPAPRMachineState {
>      uint64_t rtc_offset; /* Now used only during incoming migration */
>      struct PPCTimebase tb;
>      bool has_graphics;
> +    sPAPROptionVector *ov5;
> +    sPAPROptionVector *ov5_cas;
>  
>      uint32_t check_exception_irq;
>      Notifier epow_notifier;
> @@ -577,7 +580,7 @@ void spapr_events_init(sPAPRMachineState *sm);
>  void spapr_events_fdt_skel(void *fdt, uint32_t epow_irq);
>  int spapr_h_cas_compose_response(sPAPRMachineState *sm,
>                                   target_ulong addr, target_ulong size,
> -                                 bool cpu_update, bool memory_update);
> +                                 bool cpu_update);
>  sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn);
>  void spapr_tce_table_enable(sPAPRTCETable *tcet,
>                              uint32_t page_shift, uint64_t bus_offset,
> diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
> index fba2d98..09afd59 100644
> --- a/include/hw/ppc/spapr_ovec.h
> +++ b/include/hw/ppc/spapr_ovec.h
> @@ -42,6 +42,9 @@ typedef struct sPAPROptionVector sPAPROptionVector;
>  
>  #define OV_BIT(byte, bit) ((byte - 1) * BITS_PER_BYTE + bit)
>  
> +/* option vector 5 */
> +#define OV5_DRCONF_MEMORY       OV_BIT(2, 2)
> +
>  /* interfaces */
>  sPAPROptionVector *spapr_ovec_new(void);
>  sPAPROptionVector *spapr_ovec_clone(sPAPROptionVector *ov_orig);

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