From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45595) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wz3hp-00073V-Vv for qemu-devel@nongnu.org; Mon, 23 Jun 2014 08:50:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wz3hi-0004JZ-Jl for qemu-devel@nongnu.org; Mon, 23 Jun 2014 08:50:41 -0400 Message-ID: <53A82297.2000706@suse.de> Date: Mon, 23 Jun 2014 14:50:31 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1403515671-23963-1-git-send-email-aik@ozlabs.ru> In-Reply-To: <1403515671-23963-1-git-send-email-aik@ozlabs.ru> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] spapr: Fix RTAS token numbers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy , qemu-devel@nongnu.org Cc: qemu-ppc@nongnu.org, Michael Roth , Nikunj A Dadhania On 23.06.14 11:27, Alexey Kardashevskiy wrote: > At the moment spapr_rtas_register() allocates a new token number for every > new RTAS callback so numbers are not fixed and depend on the number of > supported RTAS handlers and the exact order of spapr_rtas_register() calls. > These tokens are copied into the device tree and remain the same during > the guest lifetime. > > When we start another guest to receive a migration, it calls > spapr_rtas_register() as well. If the number of RTAS handlers or their > order is different in QEMU on source and destination sides, the "/rtas" > node in the device tree will differ. Since migration overwrites the device > tree (as it overwrites the entire RAM), the actual RTAS config on > the destination side gets broken. > > This defines global contant values for every RTAS token which QEMU > is using today. > > This changes spapr_rtas_register() to accept a token number instead of > allocating one. This changes all users of spapr_rtas_register(). > > This changes XICS-KVM not to cache tokens registered with KVM as they > constant now. > > This makes TOKEN_BASE global as RTAS_XXX use TOKEN_BASE as > a base. TOKEN_MAX is moved+renamed too to keep them together. > > This reserves token numbers for "os-term" handlers and PCI hotplug > which we are working on. > > Cc: Michael Roth > Cc: Nikunj A Dadhania > Signed-off-by: Alexey Kardashevskiy > --- > hw/intc/xics.c | 8 +++--- > hw/intc/xics_kvm.c | 22 ++++++---------- > hw/nvram/spapr_nvram.c | 4 +-- > hw/ppc/spapr_events.c | 3 ++- > hw/ppc/spapr_pci.c | 18 ++++++++----- > hw/ppc/spapr_rtas.c | 69 +++++++++++++++++++++++++------------------------- > hw/ppc/spapr_vio.c | 5 ++-- > include/hw/ppc/spapr.h | 40 ++++++++++++++++++++++++++++- > 8 files changed, 104 insertions(+), 65 deletions(-) > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index 76dd6f5..493a2a4 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -866,10 +866,10 @@ static void xics_realize(DeviceState *dev, Error **errp) > } > > /* Registration of global state belongs into realize */ > - spapr_rtas_register("ibm,set-xive", rtas_set_xive); > - spapr_rtas_register("ibm,get-xive", rtas_get_xive); > - spapr_rtas_register("ibm,int-off", rtas_int_off); > - spapr_rtas_register("ibm,int-on", rtas_int_on); > + spapr_rtas_register(RTAS_IBM_SET_XIVE, "ibm,set-xive", rtas_set_xive); > + spapr_rtas_register(RTAS_IBM_GET_XIVE, "ibm,get-xive", rtas_get_xive); > + spapr_rtas_register(RTAS_IBM_INT_OFF, "ibm,int-off", rtas_int_off); > + spapr_rtas_register(RTAS_IBM_INT_ON, "ibm,int-on", rtas_int_on); > > spapr_register_hypercall(H_CPPR, h_cppr); > spapr_register_hypercall(H_IPI, h_ipi); > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c > index 09476ae..4704c98 100644 > --- a/hw/intc/xics_kvm.c > +++ b/hw/intc/xics_kvm.c > @@ -38,10 +38,6 @@ > typedef struct KVMXICSState { > XICSState parent_obj; > > - uint32_t set_xive_token; > - uint32_t get_xive_token; > - uint32_t int_off_token; > - uint32_t int_on_token; > int kernel_xics_fd; > } KVMXICSState; > > @@ -392,32 +388,30 @@ static void xics_kvm_realize(DeviceState *dev, Error **errp) > goto fail; > } > > - icpkvm->set_xive_token = spapr_rtas_register("ibm,set-xive", rtas_dummy); > - icpkvm->get_xive_token = spapr_rtas_register("ibm,get-xive", rtas_dummy); > - icpkvm->int_off_token = spapr_rtas_register("ibm,int-off", rtas_dummy); > - icpkvm->int_on_token = spapr_rtas_register("ibm,int-on", rtas_dummy); > + spapr_rtas_register(RTAS_IBM_SET_XIVE, "ibm,set-xive", rtas_dummy); > + spapr_rtas_register(RTAS_IBM_GET_XIVE, "ibm,get-xive", rtas_dummy); > + spapr_rtas_register(RTAS_IBM_INT_OFF, "ibm,int-off", rtas_dummy); > + spapr_rtas_register(RTAS_IBM_INT_ON, "ibm,int-on", rtas_dummy); > > - rc = kvmppc_define_rtas_kernel_token(icpkvm->set_xive_token, > - "ibm,set-xive"); > + rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_SET_XIVE, "ibm,set-xive"); > if (rc < 0) { > error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,set-xive"); > goto fail; > } > > - rc = kvmppc_define_rtas_kernel_token(icpkvm->get_xive_token, > - "ibm,get-xive"); > + rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_GET_XIVE, "ibm,get-xive"); > if (rc < 0) { > error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,get-xive"); > goto fail; > } > > - rc = kvmppc_define_rtas_kernel_token(icpkvm->int_on_token, "ibm,int-on"); > + rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_ON, "ibm,int-on"); > if (rc < 0) { > error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-on"); > goto fail; > } > > - rc = kvmppc_define_rtas_kernel_token(icpkvm->int_off_token, "ibm,int-off"); > + rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_INT_OFF, "ibm,int-off"); > if (rc < 0) { > error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,int-off"); > goto fail; > diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c > index af49c46..6a72ef4 100644 > --- a/hw/nvram/spapr_nvram.c > +++ b/hw/nvram/spapr_nvram.c > @@ -153,8 +153,8 @@ static int spapr_nvram_init(VIOsPAPRDevice *dev) > return -1; > } > > - spapr_rtas_register("nvram-fetch", rtas_nvram_fetch); > - spapr_rtas_register("nvram-store", rtas_nvram_store); > + spapr_rtas_register(RTAS_NVRAM_FETCH, "nvram-fetch", rtas_nvram_fetch); > + spapr_rtas_register(RTAS_NVRAM_STORE, "nvram-store", rtas_nvram_store); > > return 0; > } > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index 16fa49e..5ce96a7 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -317,5 +317,6 @@ void spapr_events_init(sPAPREnvironment *spapr) > spapr->epow_irq = spapr_allocate_msi(0); > spapr->epow_notifier.notify = spapr_powerdown_req; > qemu_register_powerdown_notifier(&spapr->epow_notifier); > - spapr_rtas_register("check-exception", check_exception); > + spapr_rtas_register(RTAS_CHECK_EXCEPTION, "check-exception", > + check_exception); > } > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 988f8cb..8607c88 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -909,14 +909,20 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb, > > void spapr_pci_rtas_init(void) > { > - spapr_rtas_register("read-pci-config", rtas_read_pci_config); > - spapr_rtas_register("write-pci-config", rtas_write_pci_config); > - spapr_rtas_register("ibm,read-pci-config", rtas_ibm_read_pci_config); > - spapr_rtas_register("ibm,write-pci-config", rtas_ibm_write_pci_config); > + spapr_rtas_register(RTAS_READ_PCI_CONFIG, "read-pci-config", > + rtas_read_pci_config); > + spapr_rtas_register(RTAS_WRITE_PCI_CONFIG, "write-pci-config", > + rtas_write_pci_config); > + spapr_rtas_register(RTAS_IBM_READ_PCI_CONFIG, "ibm,read-pci-config", > + rtas_ibm_read_pci_config); > + spapr_rtas_register(RTAS_IBM_WRITE_PCI_CONFIG, "ibm,write-pci-config", > + rtas_ibm_write_pci_config); > if (msi_supported) { > - spapr_rtas_register("ibm,query-interrupt-source-number", > + spapr_rtas_register(RTAS_IBM_QUERY_INTERRUPT_SOURCE_NUMBER, > + "ibm,query-interrupt-source-number", > rtas_ibm_query_interrupt_source_number); > - spapr_rtas_register("ibm,change-msi", rtas_ibm_change_msi); > + spapr_rtas_register(RTAS_IBM_CHANGE_MSI, "ibm,change-msi", > + rtas_ibm_change_msi); > } > } > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index ea4a2b2..6746f61 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -35,9 +35,6 @@ > > #include > > -#define TOKEN_BASE 0x2000 > -#define TOKEN_MAX 0x100 > - > static void rtas_display_character(PowerPCCPU *cpu, sPAPREnvironment *spapr, > uint32_t token, uint32_t nargs, > target_ulong args, > @@ -270,17 +267,15 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu, > static struct rtas_call { > const char *name; > spapr_rtas_fn fn; > -} rtas_table[TOKEN_MAX]; > - > -static struct rtas_call *rtas_next = rtas_table; > +} rtas_table[RTAS_TOKEN_MAX]; > > target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr, > uint32_t token, uint32_t nargs, target_ulong args, > uint32_t nret, target_ulong rets) > { > - if ((token >= TOKEN_BASE) > - && ((token - TOKEN_BASE) < TOKEN_MAX)) { > - struct rtas_call *call = rtas_table + (token - TOKEN_BASE); > + if ((token >= RTAS_TOKEN_BASE) > + && ((token - RTAS_TOKEN_BASE) < RTAS_TOKEN_MAX)) { > + struct rtas_call *call = rtas_table + (token - RTAS_TOKEN_BASE); > > if (call->fn) { > call->fn(cpu, spapr, token, nargs, args, nret, rets); > @@ -302,23 +297,21 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr, > return H_PARAMETER; > } > > -int spapr_rtas_register(const char *name, spapr_rtas_fn fn) > +void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn) > { > - int i; > - > - for (i = 0; i < (rtas_next - rtas_table); i++) { > - if (strcmp(name, rtas_table[i].name) == 0) { > - fprintf(stderr, "RTAS call \"%s\" registered twice\n", name); > - exit(1); > - } > + if ((token < RTAS_TOKEN_BASE) || > + (token >= RTAS_TOKEN_BASE + RTAS_TOKEN_MAX)) { > + fprintf(stderr, "RTAS invalid token 0x%x\n", token); > + exit(1); > + } > + if (rtas_table[token].name) { > + fprintf(stderr, "RTAS call \"%s\" is registered already as 0x%x\n", > + rtas_table[token].name, token); > + exit(1); > } > > - assert(rtas_next < (rtas_table + TOKEN_MAX)); > - > - rtas_next->name = name; > - rtas_next->fn = fn; > - > - return (rtas_next++ - rtas_table) + TOKEN_BASE; > + rtas_table[token - RTAS_TOKEN_BASE].name = name; > + rtas_table[token - RTAS_TOKEN_BASE].fn = fn; > } > > int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr, > @@ -358,7 +351,7 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr, > return ret; > } > > - for (i = 0; i < TOKEN_MAX; i++) { > + for (i = 0; i < RTAS_TOKEN_MAX; i++) { > struct rtas_call *call = &rtas_table[i]; > > if (!call->name) { > @@ -366,7 +359,7 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr, > } > > ret = qemu_fdt_setprop_cell(fdt, "/rtas", call->name, > - i + TOKEN_BASE); > + i + RTAS_TOKEN_BASE); > if (ret < 0) { > fprintf(stderr, "Couldn't add rtas token for %s: %s\n", > call->name, fdt_strerror(ret)); > @@ -379,18 +372,24 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr, > > static void core_rtas_register_types(void) > { > - spapr_rtas_register("display-character", rtas_display_character); > - spapr_rtas_register("get-time-of-day", rtas_get_time_of_day); > - spapr_rtas_register("set-time-of-day", rtas_set_time_of_day); > - spapr_rtas_register("power-off", rtas_power_off); > - spapr_rtas_register("system-reboot", rtas_system_reboot); > - spapr_rtas_register("query-cpu-stopped-state", > + spapr_rtas_register(RTAS_DISPLAY_CHARACTER, "display-character", > + rtas_display_character); > + spapr_rtas_register(RTAS_GET_TIME_OF_DAY, "get-time-of-day", > + rtas_get_time_of_day); > + spapr_rtas_register(RTAS_SET_TIME_OF_DAY, "set-time-of-day", > + rtas_set_time_of_day); > + spapr_rtas_register(RTAS_POWER_OFF, "power-off", rtas_power_off); > + spapr_rtas_register(RTAS_SYSTEM_REBOOT, "system-reboot", > + rtas_system_reboot); > + spapr_rtas_register(RTAS_QUERY_CPU_STOPPED_STATE, "query-cpu-stopped-state", > rtas_query_cpu_stopped_state); > - spapr_rtas_register("start-cpu", rtas_start_cpu); > - spapr_rtas_register("stop-self", rtas_stop_self); > - spapr_rtas_register("ibm,get-system-parameter", > + spapr_rtas_register(RTAS_START_CPU, "start-cpu", rtas_start_cpu); > + spapr_rtas_register(RTAS_STOP_SELF, "stop-self", rtas_stop_self); > + spapr_rtas_register(RTAS_IBM_GET_SYSTEM_PARAMETER, > + "ibm,get-system-parameter", > rtas_ibm_get_system_parameter); > - spapr_rtas_register("ibm,set-system-parameter", > + spapr_rtas_register(RTAS_IBM_SET_SYSTEM_PARAMETER, > + "ibm,set-system-parameter", > rtas_ibm_set_system_parameter); > } > > diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c > index 04e16ae..a195fd1 100644 > --- a/hw/ppc/spapr_vio.c > +++ b/hw/ppc/spapr_vio.c > @@ -517,8 +517,9 @@ VIOsPAPRBus *spapr_vio_bus_init(void) > spapr_register_hypercall(H_ENABLE_CRQ, h_enable_crq); > > /* RTAS calls */ > - spapr_rtas_register("ibm,set-tce-bypass", rtas_set_tce_bypass); > - spapr_rtas_register("quiesce", rtas_quiesce); > + spapr_rtas_register(RTAS_IBM_SET_TCE_BYPASS, "ibm,set-tce-bypass", > + rtas_set_tce_bypass); > + spapr_rtas_register(RTAS_QUIESCE, "quiesce", rtas_quiesce); > > return bus; > } > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 08c301f..86bcdb3 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -358,6 +358,44 @@ static inline int spapr_allocate_lsi(int hint) > #define RTAS_OUT_NOT_SUPPORTED -3 > #define RTAS_OUT_NOT_AUTHORIZED -9002 > > +/* RTAS tokens */ > +#define RTAS_TOKEN_BASE 0x2000 > +#define RTAS_TOKEN_MAX 0x100 Now that you have a nice list of all RTAS token numbers... > + > +#define RTAS_DISPLAY_CHARACTER (RTAS_TOKEN_BASE + 0x00) > +#define RTAS_GET_TIME_OF_DAY (RTAS_TOKEN_BASE + 0x01) > +#define RTAS_SET_TIME_OF_DAY (RTAS_TOKEN_BASE + 0x02) > +#define RTAS_POWER_OFF (RTAS_TOKEN_BASE + 0x03) > +#define RTAS_SYSTEM_REBOOT (RTAS_TOKEN_BASE + 0x04) > +#define RTAS_QUERY_CPU_STOPPED_STATE (RTAS_TOKEN_BASE + 0x05) > +#define RTAS_START_CPU (RTAS_TOKEN_BASE + 0x06) > +#define RTAS_STOP_SELF (RTAS_TOKEN_BASE + 0x07) > +#define RTAS_IBM_GET_SYSTEM_PARAMETER (RTAS_TOKEN_BASE + 0x08) > +#define RTAS_IBM_SET_SYSTEM_PARAMETER (RTAS_TOKEN_BASE + 0x09) > +#define RTAS_IBM_SET_XIVE (RTAS_TOKEN_BASE + 0x0A) > +#define RTAS_IBM_GET_XIVE (RTAS_TOKEN_BASE + 0x0B) > +#define RTAS_IBM_INT_OFF (RTAS_TOKEN_BASE + 0x0C) > +#define RTAS_IBM_INT_ON (RTAS_TOKEN_BASE + 0x0D) > +#define RTAS_CHECK_EXCEPTION (RTAS_TOKEN_BASE + 0x0E) > +#define RTAS_EVENT_SCAN (RTAS_TOKEN_BASE + 0x0F) > +#define RTAS_IBM_SET_TCE_BYPASS (RTAS_TOKEN_BASE + 0x10) > +#define RTAS_QUIESCE (RTAS_TOKEN_BASE + 0x11) > +#define RTAS_NVRAM_FETCH (RTAS_TOKEN_BASE + 0x12) > +#define RTAS_NVRAM_STORE (RTAS_TOKEN_BASE + 0x13) > +#define RTAS_READ_PCI_CONFIG (RTAS_TOKEN_BASE + 0x14) > +#define RTAS_WRITE_PCI_CONFIG (RTAS_TOKEN_BASE + 0x15) > +#define RTAS_IBM_READ_PCI_CONFIG (RTAS_TOKEN_BASE + 0x16) > +#define RTAS_IBM_WRITE_PCI_CONFIG (RTAS_TOKEN_BASE + 0x17) > +#define RTAS_IBM_QUERY_INTERRUPT_SOURCE_NUMBER (RTAS_TOKEN_BASE + 0x18) > +#define RTAS_IBM_CHANGE_MSI (RTAS_TOKEN_BASE + 0x19) > +#define RTAS_SET_INDICATOR (RTAS_TOKEN_BASE + 0x1A) > +#define RTAS_SET_POWER_LEVEL (RTAS_TOKEN_BASE + 0x1B) > +#define RTAS_GET_POWER_LEVEL (RTAS_TOKEN_BASE + 0x1C) > +#define RTAS_GET_SENSOR_STATE (RTAS_TOKEN_BASE + 0x1D) > +#define RTAS_IBM_CONFIGURE_CONNECTOR (RTAS_TOKEN_BASE + 0x1E) > +#define RTAS_IBM_OS_TERM (RTAS_TOKEN_BASE + 0x1F) > +#define RTAS_IBM_EXTENDED_OS_TERM (RTAS_TOKEN_BASE + 0x20) ... you can just define RTAS_TOKEN_MAX as the last one. #define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x21) Then you can also do all the boundary checks on [RTAS_TOKEN_BASE..RTAS_TOKEN_MAX]. Apart from that tiny nit I like the patch. Alex > + > static inline uint64_t ppc64_phys_to_real(uint64_t addr) > { > return addr & ~0xF000000000000000ULL; > @@ -377,7 +415,7 @@ typedef void (*spapr_rtas_fn)(PowerPCCPU *cpu, sPAPREnvironment *spapr, > uint32_t token, > uint32_t nargs, target_ulong args, > uint32_t nret, target_ulong rets); > -int spapr_rtas_register(const char *name, spapr_rtas_fn fn); > +void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn); > target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPREnvironment *spapr, > uint32_t token, uint32_t nargs, target_ulong args, > uint32_t nret, target_ulong rets);