Re: [Qemu-devel] [PATCH 1/3] iothread: provide helpers for internal use

[Peter Xu]

On Mon, Oct 02, 2017 at 07:18:04PM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi  writes:
> 
> > On Fri, Sep 22, 2017 at 04:56:10PM +0800, Peter Xu wrote:
> >> IOThread is a general framework that contains IO loop environment and a
> >> real thread behind.  It's also good to be used internally inside qemu.
> >> Provide some helpers for it to create iothreads to be used internally.
> >> 
> >> Signed-off-by: Peter Xu 
> >> ---
> >>  include/sysemu/iothread.h |  8 
> >>  iothread.c| 21 +
> >>  2 files changed, 29 insertions(+)
> >> 
> >> diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
> >> index d2985b3..b07663f 100644
> >> --- a/include/sysemu/iothread.h
> >> +++ b/include/sysemu/iothread.h
> >> @@ -46,4 +46,12 @@ AioContext *iothread_get_aio_context(IOThread 
> >> *iothread);
> >>  void iothread_stop_all(void);
> >>  GMainContext *iothread_get_g_main_context(IOThread *iothread);
> >>  
> >> +/*
> >> + * Helpers used to allocate iothreads for internal use.  These
> >> + * iothreads will not be seen by monitor clients when query using
> >> + * "query-iothreads".
> >> + */
> >> +IOThread *iothread_create(const char *id, Error **errp);
> >> +void iothread_destroy(IOThread *iothread);
> >> +
> >>  #endif /* IOTHREAD_H */
> >> diff --git a/iothread.c b/iothread.c
> >> index 44c8944..74e400c 100644
> >> --- a/iothread.c
> >> +++ b/iothread.c
> >> @@ -354,3 +354,24 @@ GMainContext *iothread_get_g_main_context(IOThread 
> >> *iothread)
> >>  
> >>  return iothread->worker_context;
> >>  }
> >> +
> >> +static Object *iothread_get_internal_parent(void)
> >> +{
> >> +return container_get(object_get_root(), "/internal-iothreads");
> >> +}
> >
> > Markus, please advise on the following QMP API design issue.
> 
> Sorry for the slow response.  Is my advice still needed, or are you
> good?

I see that the patches are in master now (latest version, not this
one), and the consensus should be that internally used iothreads won't
affect results of query-iothreads.  So I think we are good now.  Thanks,

-- 
Peter Xu

[Qemu-devel] [Bug 1722074] Re: warning: host doesn't support requested feature: CPUID.01H:ECX.vmx

[htmldevelo...@gmail.com]

** Description changed:

- I encountered the bug today:
+ 
+ I encountered the bug today (when using qemu to boot up images - which used 
to work on my Intel CPU box):
  
  warning: host doesn't support requested feature: CPUID.01H:ECX.vmx
+ 
+ The bug is a show-stopper - I completely cannot load my kernel images at
+ all.
  
  My Ubuntu have this version of QEMU installed:
  
  qemu-system-x86_64 --version
  
  QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.16), Copyright
  (c) 2003-2008 Fabrice Bellard
  
- And PC is a AMD Ryzen7 CPU built, and since it is not Intel CPU:
- 
+ And PC is a AMD Ryzen7 CPU built, and this is the first time I am using
+ it to load QEMU images.   My host information:
  
  cat /proc/cpuinfo |more
  
  processor : 0
  vendor_id : AuthenticAMD
  cpu family: 23
  model : 1
  model name: AMD Ryzen 7 1700X Eight-Core Processor
  stepping  : 1
  microcode : 0x800110e
  cpu MHz   : 2200.000
  cache size: 512 KB
  physical id   : 0
  siblings  : 16
  core id   : 0
  cpu cores : 8
  apicid: 0
  initial apicid: 0
  fpu   : yes
  fpu_exception : yes
  cpuid level   : 13
  wp: yes
- flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov 
+ flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov
  pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext fxsr_opt pdpe1gb 
rdtscp
-  lm constant_tsc rep_good nopl nonstop_tsc extd_apicid aperfmperf pni 
pclmulqdq 
+  lm constant_tsc rep_good nopl nonstop_tsc extd_apicid aperfmperf pni 
pclmulqdq
  monitor ssse3 fma cx16 sse4_1 sse4_2 movbe popcnt aes xsave avx f16c rdrand 
lahf
  _lm cmp_legacy svm extapic cr8_legacy abm sse4a misalignsse 3dnowprefetch 
osvw s
- kinit wdt tce topoext perfctr_core perfctr_nb bpext perfctr_l2 mwaitx 
hw_pstate 
- vmmcall fsgsbase bmi1 avx2 smep bmi2 rdseed adx smap clflushopt sha_ni 
xsaveopt 
+ kinit wdt tce topoext perfctr_core perfctr_nb bpext perfctr_l2 mwaitx 
hw_pstate
+ vmmcall fsgsbase bmi1 avx2 smep bmi2 rdseed adx smap clflushopt sha_ni 
xsaveopt
  xsavec xgetbv1 xsaves clzero irperf arat npt lbrv svm_lock nrip_save 
tsc_scale v
- mcb_clean flushbyasid decodeassists pausefilter pfthreshold avic 
overflow_recov 
+ mcb_clean flushbyasid decodeassists pausefilter pfthreshold avic 
overflow_recov
  succor smca
  bugs  : fxsave_leak sysret_ss_attrs null_seg
  bogomips  : 6787.24
  TLB size  : 2560 4K pages
  clflush size  : 64
  cache_alignment   : 64
  address sizes : 48 bits physical, 48 bits virtual
  power management: ts ttp tm hwpstate eff_freq_ro [13] [14]
  
  processor : 1
  vendor_id : AuthenticAMD
  cpu family: 23
  model : 1
  model name: AMD Ryzen 7 1700X Eight-Core Processor
  stepping  : 1
  microcode : 0x800110e
  cpu MHz   : 2200.000
  cache size: 512 KB
  
  From other places, it can be seen that this is an AMD CPU issue:
  
  https://www.virtualmin.com/node/52227
  
  not sure?
+ 
+ The bug will also affect the host negatively:  it will completely go
+ into a hung mode - the entire host becomes completely unsable.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1722074

Title:
  warning: host doesn't support requested feature: CPUID.01H:ECX.vmx

Status in QEMU:
  New

Bug description:
  
  I encountered the bug today (when using qemu to boot up images - which used 
to work on my Intel CPU box):

  warning: host doesn't support requested feature: CPUID.01H:ECX.vmx

  The bug is a show-stopper - I completely cannot load my kernel images
  at all.

  My Ubuntu have this version of QEMU installed:

  qemu-system-x86_64 --version

  QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.16),
  Copyright (c) 2003-2008 Fabrice Bellard

  And PC is a AMD Ryzen7 CPU built, and this is the first time I am
  using it to load QEMU images.   My host information:

  cat /proc/cpuinfo |more

  processor : 0
  vendor_id : AuthenticAMD
  cpu family: 23
  model : 1
  model name: AMD Ryzen 7 1700X Eight-Core Processor
  stepping  : 1
  microcode : 0x800110e
  cpu MHz   : 2200.000
  cache size: 512 KB
  physical id   : 0
  siblings  : 16
  core id   : 0
  cpu cores : 8
  apicid: 0
  initial apicid: 0
  fpu   : yes
  fpu_exception : yes
  cpuid level   : 13
  wp: yes
  flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov
  pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext fxsr_opt pdpe1gb 
rdtscp
   lm constant_tsc rep_good nopl nonstop_tsc extd_apicid aperfmperf pni 
pclmulqdq
  monitor ssse3 fma cx16 sse4_1 sse4_2 movbe popcnt aes xsave avx f16c rdrand 
lahf
  _lm cmp_legacy svm extapic cr8_legacy abm sse4a misalignsse 3dnowprefetch 
osvw 

Re: [Qemu-devel] [PATCH v5 4/6] target/ppc: Handle NMI guest exit

[Aravinda Prasad]

On Wednesday 04 October 2017 06:59 AM, David Gibson wrote:
> On Thu, Sep 28, 2017 at 04:08:10PM +0530, Aravinda Prasad wrote:
>> Memory error such as bit flips that cannot be corrected
>> by hardware are passed on to the kernel for handling.
>> If the memory address in error belongs to guest then
>> the guest kernel is responsible for taking suitable action.
>> Patch [1] enhances KVM to exit guest with exit reason
>> set to KVM_EXIT_NMI in such cases.
>>
>> This patch handles KVM_EXIT_NMI exit. If the guest OS
>> has registered the machine check handling routine by
>> calling "ibm,nmi-register", then the handler builds
>> the error log and invokes the registered handler else
>> invokes the handler at 0x200.
>>
>> Note that FWNMI handles synchronous machine check exceptions
>> triggered by the hardware and hence we do not extend
>> such support to the "nmi" command available in the QEMU
>> monitor. Hence, "nmi" command from the monitor will
>> always go through 0x200 vector.
>>
>> [1] https://www.spinics.net/lists/kvm-ppc/msg12637.html
>>  (e20bbd3d and related commits)
> 
> What does happen on KVM if an asynchronous machine check exception
> occurs while in the guest?  Or under PowerVM for that matter.

AFAIK asynchronous errors take a different path in KVM as it can happen
in a different process context.

> 
>>
>> Signed-off-by: Aravinda Prasad 
>> Signed-off-by: Mahesh Salgaonkar 
>> ---
>>  hw/ppc/spapr.c |4 +++
>>  hw/ppc/spapr_events.c  |   62 
>> 
>>  include/hw/ppc/spapr.h |6 +
>>  target/ppc/kvm.c   |   62 
>> 
>>  target/ppc/kvm_ppc.h   |   14 +++
>>  5 files changed, 148 insertions(+)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index d568ea6..7780434 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -2453,6 +2453,10 @@ static void ppc_spapr_init(MachineState *machine)
>>  error_report("Could not get size of LPAR rtas '%s'", filename);
>>  exit(1);
>>  }
>> +
>> +/* Resize blob to accommodate error log. */
>> +spapr->rtas_size = spapr_get_rtas_size();
>> +
>>  spapr->rtas_blob = g_malloc(spapr->rtas_size);
>>  if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
>>  error_report("Could not load LPAR rtas '%s'", filename);
>> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
>> index e377fc7..ac93a7b 100644
>> --- a/hw/ppc/spapr_events.c
>> +++ b/hw/ppc/spapr_events.c
>> @@ -41,6 +41,7 @@
>>  #include "qemu/bcd.h"
>>  #include "hw/ppc/spapr_ovec.h"
>>  #include 
>> +#include 
>>  
>>  #define RTAS_LOG_VERSION_MASK   0xff00
>>  #define   RTAS_LOG_VERSION_60x0600
>> @@ -174,6 +175,22 @@ struct epow_extended_log {
>>  struct rtas_event_log_v6_epow epow;
>>  } QEMU_PACKED;
>>  
>> +/*
>> + * Data format in RTAS Blob
>> + *
>> + * This structure contains error information related to Machine
>> + * Check exception. This is filled up and copied to rtas blob
>> + * upon machine check exception. The address of rtas blob is
>> + * passed on to OS registered machine check notification
>> + * routines upon machine check exception.
>> + */
>> +struct rtas_event_log_mce {
>> +target_ulong r3;
>> +struct rtas_error_log rtas_error_log;
>> +unsigned char   buffer[1];  /* Start of extended log */
> 
> I believe we allow C99 extensions in qemu, so you can use buffer[], a
> C99 flexible array member, rather than the length 1 hack.

ok.

> 
>> +} QEMU_PACKED;
>> +
>> +
>>  union drc_identifier {
>>  uint32_t index;
>>  uint32_t count;
>> @@ -623,6 +640,51 @@ void 
>> spapr_hotplug_req_remove_by_count_indexed(sPAPRDRConnectorType drc_type,
>>  RTAS_LOG_V6_HP_ACTION_REMOVE, drc_type, 
>> _id);
>>  }
>>  
>> +ssize_t spapr_get_rtas_size(void)
>> +{
>> +return RTAS_ERRLOG_OFFSET + sizeof(struct rtas_event_log_mce);
> 
> Erm.. because of the definition of rtas_event_log_mce, this only
> allows for 1 byte of extended log buffer.  That doesn't seem right.

This is directly taken from the kernel's RTAS log (struct rtas_error_log
in arch/powerpc/include/asm/rtas.h). I am not sure why they use 1 byte
extended log buffer.

> 
>> +}
>> +
>> +target_ulong spapr_mce_req_event(target_ulong r3, hwaddr rtas_addr,
>> + uint16_t flags, bool err_type, bool le)
> 
> err_tpe isn't a very informative name for a boolean.  'uncorrectable'
> would be better.  Although, didn't you say only uncorrectable errors
> are directed to the guest, so does this have any purpose anyway?

Right now only uncorrectable errors, but can be enhanced later to pass
on correctable errors to inform the guest.

> 
>> +{
>> +struct rtas_event_log_mce mc_log;
>> +uint32_t summary;
>> +
>> +/* Set error log fields */
>> +mc_log.r3 = r3;
>> +

[Qemu-devel] [Bug 1722074] [NEW] warning: host doesn't support requested feature: CPUID.01H:ECX.vmx

[htmldevelo...@gmail.com]

Public bug reported:

I encountered the bug today:

warning: host doesn't support requested feature: CPUID.01H:ECX.vmx

My Ubuntu have this version of QEMU installed:

qemu-system-x86_64 --version

QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.16), Copyright
(c) 2003-2008 Fabrice Bellard

And PC is a AMD Ryzen7 CPU built, and since it is not Intel CPU:


cat /proc/cpuinfo |more

processor   : 0
vendor_id   : AuthenticAMD
cpu family  : 23
model   : 1
model name  : AMD Ryzen 7 1700X Eight-Core Processor
stepping: 1
microcode   : 0x800110e
cpu MHz : 2200.000
cache size  : 512 KB
physical id : 0
siblings: 16
core id : 0
cpu cores   : 8
apicid  : 0
initial apicid  : 0
fpu : yes
fpu_exception   : yes
cpuid level : 13
wp  : yes
flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov 
pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext fxsr_opt pdpe1gb rdtscp
 lm constant_tsc rep_good nopl nonstop_tsc extd_apicid aperfmperf pni pclmulqdq 
monitor ssse3 fma cx16 sse4_1 sse4_2 movbe popcnt aes xsave avx f16c rdrand lahf
_lm cmp_legacy svm extapic cr8_legacy abm sse4a misalignsse 3dnowprefetch osvw s
kinit wdt tce topoext perfctr_core perfctr_nb bpext perfctr_l2 mwaitx hw_pstate 
vmmcall fsgsbase bmi1 avx2 smep bmi2 rdseed adx smap clflushopt sha_ni xsaveopt 
xsavec xgetbv1 xsaves clzero irperf arat npt lbrv svm_lock nrip_save tsc_scale v
mcb_clean flushbyasid decodeassists pausefilter pfthreshold avic overflow_recov 
succor smca
bugs: fxsave_leak sysret_ss_attrs null_seg
bogomips: 6787.24
TLB size: 2560 4K pages
clflush size: 64
cache_alignment : 64
address sizes   : 48 bits physical, 48 bits virtual
power management: ts ttp tm hwpstate eff_freq_ro [13] [14]

processor   : 1
vendor_id   : AuthenticAMD
cpu family  : 23
model   : 1
model name  : AMD Ryzen 7 1700X Eight-Core Processor
stepping: 1
microcode   : 0x800110e
cpu MHz : 2200.000
cache size  : 512 KB

>From other places, it can be seen that this is an AMD CPU issue:

https://www.virtualmin.com/node/52227

not sure?

** Affects: qemu
 Importance: Undecided
 Status: New

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1722074

Title:
  warning: host doesn't support requested feature: CPUID.01H:ECX.vmx

Status in QEMU:
  New

Bug description:
  I encountered the bug today:

  warning: host doesn't support requested feature: CPUID.01H:ECX.vmx

  My Ubuntu have this version of QEMU installed:

  qemu-system-x86_64 --version

  QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.16),
  Copyright (c) 2003-2008 Fabrice Bellard

  And PC is a AMD Ryzen7 CPU built, and since it is not Intel CPU:

  
  cat /proc/cpuinfo |more

  processor : 0
  vendor_id : AuthenticAMD
  cpu family: 23
  model : 1
  model name: AMD Ryzen 7 1700X Eight-Core Processor
  stepping  : 1
  microcode : 0x800110e
  cpu MHz   : 2200.000
  cache size: 512 KB
  physical id   : 0
  siblings  : 16
  core id   : 0
  cpu cores : 8
  apicid: 0
  initial apicid: 0
  fpu   : yes
  fpu_exception : yes
  cpuid level   : 13
  wp: yes
  flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov 
  pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext fxsr_opt pdpe1gb 
rdtscp
   lm constant_tsc rep_good nopl nonstop_tsc extd_apicid aperfmperf pni 
pclmulqdq 
  monitor ssse3 fma cx16 sse4_1 sse4_2 movbe popcnt aes xsave avx f16c rdrand 
lahf
  _lm cmp_legacy svm extapic cr8_legacy abm sse4a misalignsse 3dnowprefetch 
osvw s
  kinit wdt tce topoext perfctr_core perfctr_nb bpext perfctr_l2 mwaitx 
hw_pstate 
  vmmcall fsgsbase bmi1 avx2 smep bmi2 rdseed adx smap clflushopt sha_ni 
xsaveopt 
  xsavec xgetbv1 xsaves clzero irperf arat npt lbrv svm_lock nrip_save 
tsc_scale v
  mcb_clean flushbyasid decodeassists pausefilter pfthreshold avic 
overflow_recov 
  succor smca
  bugs  : fxsave_leak sysret_ss_attrs null_seg
  bogomips  : 6787.24
  TLB size  : 2560 4K pages
  clflush size  : 64
  cache_alignment   : 64
  address sizes : 48 bits physical, 48 bits virtual
  power management: ts ttp tm hwpstate eff_freq_ro [13] [14]

  processor : 1
  vendor_id : AuthenticAMD
  cpu family: 23
  model : 1
  model name: AMD Ryzen 7 1700X Eight-Core Processor
  stepping  : 1
  microcode : 0x800110e
  cpu MHz   : 2200.000
  cache size: 512 KB

  From other places, it can be seen that this is an AMD CPU issue:

  https://www.virtualmin.com/node/52227

  not sure?

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1722074/+subscriptions

Re: [Qemu-devel] [PATCH v5 6/6] migration: Block migration while handling machine check

[Aravinda Prasad]

On Wednesday 04 October 2017 07:09 AM, David Gibson wrote:
> On Thu, Sep 28, 2017 at 04:08:31PM +0530, Aravinda Prasad wrote:
>> Block VM migration requests until the machine check
>> error handling is complete as (i) these errors are
>> specific to the source hardware and is irrelevant on
>> the target hardware, (ii) these errors cause data
>> corruption and should be handled before migration.
>>
>> Signed-off-by: Aravinda Prasad 
>> ---
>>  hw/ppc/spapr_rtas.c|3 +++
>>  include/hw/ppc/spapr.h |2 ++
>>  target/ppc/kvm.c   |   17 +
>>  3 files changed, 22 insertions(+)
>>
>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>> index d017a67..17f6567 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -47,6 +47,7 @@
>>  #include "trace.h"
>>  #include "hw/ppc/fdt.h"
>>  #include "kvm_ppc.h"
>> +#include "migration/blocker.h"
>>  
>>  static void rtas_display_character(PowerPCCPU *cpu, sPAPRMachineState 
>> *spapr,
>> uint32_t token, uint32_t nargs,
>> @@ -390,6 +391,8 @@ static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu,
>>  spapr->mc_status = -1;
>>  qemu_cond_signal(>mc_delivery_cond);
>>  rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>> +migrate_del_blocker(spapr->migration_blocker);
>> +error_free(spapr->migration_blocker);
>>  }
>>  }
>>  
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index a75e9cf..0890a44 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -7,6 +7,7 @@
>>  #include "hw/ppc/spapr_drc.h"
>>  #include "hw/mem/pc-dimm.h"
>>  #include "hw/ppc/spapr_ovec.h"
>> +#include "qapi/error.h"
>>  
>>  struct VIOsPAPRBus;
>>  struct sPAPRPHBState;
>> @@ -136,6 +137,7 @@ struct sPAPRMachineState {
>>  MemoryHotplugState hotplug_memory;
>>  
>>  const char *icp_type;
>> +Error *migration_blocker;
> 
> This isn't a good name, because it's _specifically_ the fwnmi as a
> migration blocker - trying to put another migration blocker in here
> would break horribly, because nmi-interlock would clear it regardless.

Will rename it.

> 
>>  };
>>  
>>  #define H_SUCCESS 0
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index 59b3322..58de7ea 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -52,6 +52,7 @@
>>  #endif
>>  #include "elf.h"
>>  #include "sysemu/kvm_int.h"
>> +#include "migration/blocker.h"
>>  
>>  //#define DEBUG_KVM
>>  
>> @@ -2770,10 +2771,26 @@ int kvm_handle_nmi(PowerPCCPU *cpu, struct kvm_run 
>> *run)
>>  sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>  PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>>  target_ulong msr = 0;
>> +Error *local_err = NULL;
>> +int ret;
>>  bool type, le;
>>  
>>  cpu_synchronize_state(CPU(cpu));
>>  
>> +error_setg(>migration_blocker,
>> +"Live migration not supported during machine check error 
>> handling");
> 
> In fact, there's no real reason to generate the error here.  The
> error's always the same so you could just create it at startup as a
> global and just add/remove it to the block list.

ok.

> 
>> +ret = migrate_add_blocker(spapr->migration_blocker, _err);
>> +if (ret < 0) {
>> +/*
>> + * We don't want to abort and let the migration to continue. In a
>> + * rare case, the machine check handler will run on the target
>> + * hardware. Though this is not preferable, it is better than 
>> aborting
> 
> Why is it not preferable?  I mean it's an edge case, but AFAICT it's
> still the correct behaviour.

Will remove that line in the comment.

> 
>> + * the migration or killing the VM.
>> + */
>> +error_free(spapr->migration_blocker);
>> +fprintf(stderr, "Warning: Machine check during VM migration\n");
> 
> Use error_report(), not fprintf().

ok.

Regards,
Aravinda

> 
>> +}
>> +
>>  /*
>>   * Properly set bits in MSR before we invoke the handler.
>>   * SRR0/1, DAR and DSISR are properly set by KVM
>>
> 

-- 
Regards,
Aravinda

Re: [Qemu-devel] [PATCH v5 5/6] ppc: spapr: Enable FWNMI capability

[Aravinda Prasad]

On Wednesday 04 October 2017 07:04 AM, David Gibson wrote:
> On Thu, Sep 28, 2017 at 04:08:21PM +0530, Aravinda Prasad wrote:
>> Enable the KVM capability KVM_CAP_PPC_FWNMI so that
>> the KVM causes guest exit with NMI as exit reason
>> when it encounters a machine check exception on the
>> address belonging to a guest. Without this capability
>> enabled, KVM redirects machine check exceptions to
>> guest's 0x200 vector.
>>
>> Signed-off-by: Aravinda Prasad 
>> ---
>>  hw/ppc/spapr_rtas.c  |   15 +++
>>  target/ppc/kvm.c |   13 +
>>  target/ppc/kvm_ppc.h |6 ++
>>  3 files changed, 34 insertions(+)
>>
>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>> index 08e9a5e..d017a67 100644
>> --- a/hw/ppc/spapr_rtas.c
>> +++ b/hw/ppc/spapr_rtas.c
>> @@ -46,6 +46,7 @@
>>  #include "qemu/cutils.h"
>>  #include "trace.h"
>>  #include "hw/ppc/fdt.h"
>> +#include "kvm_ppc.h"
>>  
>>  static void rtas_display_character(PowerPCCPU *cpu, sPAPRMachineState 
>> *spapr,
>> uint32_t token, uint32_t nargs,
>> @@ -354,6 +355,20 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
>>target_ulong args,
>>uint32_t nret, target_ulong rets)
>>  {
>> +int ret;
>> +
>> +ret = kvmppc_fwnmi_enable(cpu);
> 
> If you're enabling it here, doesn't that mean you need to disable it
> on reset?

We don't have a way in KVM to disable it once enabled.

> 
>> +if (ret == 1) {
>> +rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
>> +return;
>> +}
>> +
>> +if (ret < 0) {
>> +rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
>> +return;
>> +}
>> +
>>  spapr->guest_machine_check_addr = rtas_ld(args, 1);
>>  rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>>  }
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index 7e4ce02..59b3322 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -92,6 +92,7 @@ static int cap_mmu_radix;
>>  static int cap_mmu_hash_v3;
>>  static int cap_resize_hpt;
>>  static int cap_ppc_pvr_compat;
>> +static int cap_fwnmi;
>>  
>>  static uint32_t debug_inst_opcode;
>>  
>> @@ -150,6 +151,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>  cap_mmu_radix = kvm_vm_check_extension(s, KVM_CAP_PPC_MMU_RADIX);
>>  cap_mmu_hash_v3 = kvm_vm_check_extension(s, KVM_CAP_PPC_MMU_HASH_V3);
>>  cap_resize_hpt = kvm_vm_check_extension(s, KVM_CAP_SPAPR_RESIZE_HPT);
>> +cap_fwnmi = kvm_check_extension(s, KVM_CAP_PPC_FWNMI);
>>  /*
>>   * Note: setting it to false because there is not such capability
>>   * in KVM at this moment.
>> @@ -2142,6 +2144,17 @@ void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int 
>> mpic_proxy)
>>  }
>>  }
>>  
>> +int kvmppc_fwnmi_enable(PowerPCCPU *cpu)
>> +{
>> +CPUState *cs = CPU(cpu);
>> +
>> +if (!cap_fwnmi) {
>> +return 1;
>> +}
>> +
>> +return kvm_vcpu_enable_cap(cs, KVM_CAP_PPC_FWNMI, 0);
> 
> Yeah, this is no good.  It means migration from a host that's fwnmi
> capable to one that isn't will be subtly broken.  Instead you need to
> make fwnmi capability a machine property.  If the property is
> requested and the host kernel doesn't support it, you need to outright
> fail, rather than try to fall back.

Sure.

> 
>> +}
>> +
>>  int kvmppc_smt_threads(void)
>>  {
>>  return cap_ppc_smt ? cap_ppc_smt : 1;
>> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
>> index 0139dae..55b6df2 100644
>> --- a/target/ppc/kvm_ppc.h
>> +++ b/target/ppc/kvm_ppc.h
>> @@ -28,6 +28,7 @@ void kvmppc_enable_clear_ref_mod_hcalls(void);
>>  void kvmppc_set_papr(PowerPCCPU *cpu);
>>  int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr);
>>  void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy);
>> +int kvmppc_fwnmi_enable(PowerPCCPU *cpu);
>>  int kvmppc_smt_threads(void);
>>  void kvmppc_hint_smt_possible(Error **errp);
>>  int kvmppc_set_smt_threads(int smt);
>> @@ -157,6 +158,11 @@ static inline void kvmppc_set_mpic_proxy(PowerPCCPU 
>> *cpu, int mpic_proxy)
>>  {
>>  }
>>  
>> +int kvmppc_fwnmi_enable(PowerPCCPU *cpu)
>> +{
>> +return 1;
> 
> Likewise, this should be available, not banned, on TCG.  I think there
> are existing problems with TCG<->KVM migration, but there's no
> inherent reason they shouldn't work, so we don't want to introduce
> extra reasons they don't.
> 
> Even if TCG will never generate fwnmis (for now), it should allow the
> guest to register for them.

Should this be then changed later only when TCG generates fwnmis?
Because we don't want to set spapr->guest_machine_check_addr for TCG in
rtas_ibm_nmi_register(). If set then all machine checks are redirected
to this address which is not desirable for TCG as we still want machine
checks to be passed via 0x200.

Regards,
Aravinda

> 
>> +}
>> +
>>  static inline int kvmppc_smt_threads(void)
>>  {
>>  return 1;
>>
> 

-- 
Regards,
Aravinda

[Qemu-devel] [PATCH v2 1/2] spapr: introduce SPAPR_MACHINE_NOASSERT()

[Greg Kurz]

A spapr-cpu-core device can only be plugged into a pseries machine. This
is checked in spapr_cpu_core_realize() with object_dynamic_cast() instead
of the usual SPAPR_MACHINE() macro because we don't want QEMU to abort.

This patch moves the gory details to a SPAPR_MACHINE_NOASSERT() macro
that acts like the SPAPR_MACHINE() one, except it returns NULL instead
of aborting if its argument doesn't point to a pseries machine type.

This is done for two reasons:
- it makes the code nicer
- it may be used by other pseries-specific devices like PHBs for example

Signed-off-by: Greg Kurz 
---
 hw/ppc/spapr_cpu_core.c |7 +++
 include/hw/ppc/spapr.h  |3 +++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 37beb56e8b18..5fde07614218 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -199,7 +199,7 @@ error:
 
 static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
 {
-sPAPRMachineState *spapr;
+sPAPRMachineState *spapr = SPAPR_MACHINE_NOASSERT(qdev_get_machine());
 sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
 sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
 CPUCore *cc = CPU_CORE(OBJECT(dev));
@@ -209,9 +209,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error 
**errp)
 void *obj;
 int i, j;
 
-spapr = (sPAPRMachineState *) qdev_get_machine();
-if (!object_dynamic_cast((Object *) spapr, TYPE_SPAPR_MACHINE)) {
-error_setg(errp, "spapr-cpu-core needs a pseries machine");
+if (!spapr) {
+error_setg(errp, TYPE_SPAPR_CPU_CORE " needs a pseries machine");
 return;
 }
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index c1b365f56431..4933da8083df 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -43,6 +43,9 @@ typedef struct sPAPRMachineClass sPAPRMachineClass;
 #define SPAPR_MACHINE_CLASS(klass) \
 OBJECT_CLASS_CHECK(sPAPRMachineClass, klass, TYPE_SPAPR_MACHINE)
 
+#define SPAPR_MACHINE_NOASSERT(obj) \
+((sPAPRMachineState *) object_dynamic_cast(obj, TYPE_SPAPR_MACHINE))
+
 typedef enum {
 SPAPR_RESIZE_HPT_DEFAULT = 0,
 SPAPR_RESIZE_HPT_DISABLED,

[Qemu-devel] [PATCH v2 2/2] spapr_pci: fail gracefully with non-pseries machine types

[Greg Kurz]

QEMU currently crashes when the user tries to add a spapr-pci-host-bridge
on a non-pseries machine:

$ qemu-system-ppc64 -M ppce500 -device spapr-pci-host-bridge,index=1
hw/ppc/spapr_pci.c:1535:spapr_phb_realize:
Object 0x1003dacae60 is not an instance of type spapr-machine
Aborted (core dumped)

The same thing happens with the deprecated but still available child type
spapr-pci-vfio-host-bridge.

Fix both by using the SPAPR_MACHINE_NOASSERT() macro in papr_phb_realize().

Reviewed-by: Daniel Henrique Barboza 
Signed-off-by: Greg Kurz 
---
v2: - use new SPAPR_MACHINE_NOASSERT() macro
---
 hw/ppc/spapr_pci.c |7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 5049ced4e8b4..4d6a91ebb9c2 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1507,7 +1507,7 @@ static void spapr_pci_unplug_request(HotplugHandler 
*plug_handler,
 
 static void spapr_phb_realize(DeviceState *dev, Error **errp)
 {
-sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+sPAPRMachineState *spapr = SPAPR_MACHINE_NOASSERT(qdev_get_machine());
 SysBusDevice *s = SYS_BUS_DEVICE(dev);
 sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
 PCIHostState *phb = PCI_HOST_BRIDGE(s);
@@ -1519,6 +1519,11 @@ static void spapr_phb_realize(DeviceState *dev, Error 
**errp)
 const unsigned windows_supported =
 sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
 
+if (!spapr) {
+error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries 
machine");
+return;
+}
+
 if (sphb->index != (uint32_t)-1) {
 sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
 Error *local_err = NULL;

Re: [Qemu-devel] [Qemu-ppc] How to debug crash in TCG code?

[BALATON Zoltan]

Hello,

I'm still trying to debug a crash that I don't understand why is
happening and how to debug it further. Any hints are apreciated. I've 
described the problem previously in

http://lists.nongnu.org/archive/html/qemu-ppc/2017-08/msg00249.html
but I've dug deeper now, here's what I've found.

I'm running with -accel tcg,thread=single -singlestep so it does not seem 
to be a multi threading problem. The seg fault happens when U-Boot running 
on the emulated embedded PPC SoC accesses a PCI device but after several 
successful iteration of the same access so it seems to be some internal 
inconsistency that arrises while QEMU is running, not something generally 
broken. It is also dependent on code run by the client or QEMU, so e.g. it 
disappears when I enable -d in_asm or use a different U-Boot version. I'm 
not sure what exactly causes the bug to happen or disappear but I can 
reproduce it reliably with certain images at the same place. It always 
happens during io_readx or io_writex. Here's the info I could get with a 
debugger with some additional debug and trace messages enabled:


Here's the PCI device detected by U-Boot and BARs mapped:
The device is implemended here:
http://lists.nongnu.org/archive/html/qemu-ppc/2017-08/msg00246.html

4744@1507472117.454656:pci_cfg_read sii3112 01:0 @0xe -> 0x0
tlb_set_page_with_attrs: vaddr=0ffb0800 paddr=0x0ffb0800 prot=7 idx=1
tlb_set_page_with_attrs: vaddr=0ff73c00 paddr=0x0ff73c00 prot=7 idx=1
4744@1507472117.454676:pci_cfg_read sii3112 01:0 @0x0 -> 0x1095
4744@1507472117.454766:pci_cfg_read sii3112 01:0 @0x2 -> 0x3112
4744@1507472117.454775:pci_cfg_read sii3112 01:0 @0xa -> 0x104
tlb_set_page_with_attrs: vaddr=0ff74000 paddr=0x0ff74000 prot=7 idx=1
4744@1507472117.454794:pci_cfg_read sii3112 01:0 @0xa -> 0x104
4744@1507472117.454905:pci_cfg_read sii3112 01:0 @0x4 -> 0x0
4744@1507472117.454916:pci_cfg_write sii3112 01:0 @0x10 <- 0x
4744@1507472117.454927:pci_cfg_read sii3112 01:0 @0x10 -> 0xfff9
4744@1507472117.456524:pci_cfg_write sii3112 01:0 @0x10 <- 0x1000
tlb_set_page_with_attrs: vaddr=0ff74800 paddr=0x0ff74800 prot=7 idx=1
4744@1507472117.456823:pci_cfg_write sii3112 01:0 @0x14 <- 0x
4744@1507472117.456833:pci_cfg_read sii3112 01:0 @0x14 -> 0xfffd
4744@1507472117.456844:pci_cfg_write sii3112 01:0 @0x14 <- 0x1008
4744@1507472117.456852:pci_cfg_write sii3112 01:0 @0x18 <- 0x
4744@1507472117.456859:pci_cfg_read sii3112 01:0 @0x18 -> 0xfff9
4744@1507472117.456869:pci_cfg_write sii3112 01:0 @0x18 <- 0x1010
4744@1507472117.456878:pci_cfg_write sii3112 01:0 @0x1c <- 0x
4744@1507472117.456885:pci_cfg_read sii3112 01:0 @0x1c -> 0xfffd
4744@1507472117.456895:pci_cfg_write sii3112 01:0 @0x1c <- 0x1018
4744@1507472117.456903:pci_cfg_write sii3112 01:0 @0x20 <- 0x
4744@1507472117.456911:pci_cfg_read sii3112 01:0 @0x20 -> 0xfff1
4744@1507472117.456920:pci_cfg_write sii3112 01:0 @0x20 <- 0x1020
4744@1507472117.456929:pci_cfg_write sii3112 01:0 @0x24 <- 0x
4744@1507472117.456936:pci_cfg_read sii3112 01:0 @0x24 -> 0xfe00
4744@1507472117.457173:pci_cfg_write sii3112 01:0 @0x24 <- 0x8000
4744@1507472117.457208:pci_cfg_write sii3112 01:0 @0x4 <- 0x7
4744@1507472117.457213:pci_update_mappings_add d=0x55aca57d3ce0 00:01.0 
0,0x1000+0x8
tlb_flush_nocheck: (count: 23)
4744@1507472117.457488:pci_update_mappings_add d=0x55aca57d3ce0 00:01.0 
1,0x1008+0x4
tlb_flush_nocheck: (count: 24)
4744@1507472117.457769:pci_update_mappings_add d=0x55aca57d3ce0 00:01.0 
2,0x1010+0x8
tlb_flush_nocheck: (count: 25)
4744@1507472117.458048:pci_update_mappings_add d=0x55aca57d3ce0 00:01.0 
3,0x1018+0x4
tlb_flush_nocheck: (count: 26)
4744@1507472117.458324:pci_update_mappings_add d=0x55aca57d3ce0 00:01.0 
4,0x1020+0x10
tlb_flush_nocheck: (count: 27)
4744@1507472117.458604:pci_update_mappings_add d=0x55aca57d3ce0 00:01.0 
5,0x8000+0x200

(BAR0-4 are alias memory regions to BAR5 so all access should go to BAR5 
at the end even if U-Boot accesses it through BAR0-4)

Here U-Boot starts using the device:

tlb_set_page_with_attrs: vaddr=dec0 paddr=0x000c0ec0 prot=3 idx=1
4744@1507472121.253966:pci_cfg_read ppc4xx-host-bridge 00:0 @0xe -> 0x0
4744@1507472121.253982:pci_cfg_read ppc4xx-host-bridge 00:0 @0x0 -> 0x1014
4744@1507472121.253992:pci_cfg_read ppc4xx-host-bridge 00:0 @0x2 -> 0x27f
4744@1507472121.254005:pci_cfg_read sii3112 01:0 @0xe -> 0x0
4744@1507472121.254015:pci_cfg_read sii3112 01:0 @0x0 -> 0x1095
4744@1507472121.254024:pci_cfg_read sii3112 01:0 @0x2 -> 0x3112
tlb_set_page_with_attrs: vaddr=0800 paddr=0x0800 prot=7 idx=1
tlb_set_page_with_attrs: vaddr=0ff6f000 paddr=0x0ff6f000 prot=7 idx=1
tlb_set_page_with_attrs: vaddr=0ff74000 paddr=0x0ff74000 prot=7 idx=1
tlb_set_page_with_attrs: vaddr=0ff73c00 paddr=0x0ff73c00 prot=7 idx=1
tlb_set_page_with_attrs: vaddr=0ffb0c00 paddr=0x0ffb0c00 prot=7 idx=1

Re: [Qemu-devel] [PATCH v3 0/8] Add the ZynqMP PMU and IPI

[Edgar E. Iglesias]

On Wed, Sep 20, 2017 at 03:01:31PM -0700, Alistair Francis wrote:
> 
> This series adds the ZynqMP Power Management Unit (PMU) machine with basic
> functionality.
> 
> The machine only has the
>  - CPU
>  - Memory
>  - Interrupt controller
>  - IPI device
> 
> connected, but that is enough to run some of the ROM and firmware
> code on the machine
> 
> The series also adds the IPI device and connects it to the ZynqMP ARM
> side and the ZynqMP PMU. These IPI devices don't connect between the ARM
> and MicroBlaze instances though.
> 
> v3:
>  - Add the interrupt controller
>  - Replace some of the error_fatals with errp
>  - Fix the PMU CPU name

Hi Alistair,


Sorry for the super long delay...

I think this mostly looks good but I was wondering if we really need
to have a board specific (zcu102) PMU?

Best regards,
Edgar



> 
> 
> 
> Alistair Francis (8):
>   xlnx-zynqmp-pmu: Initial commit of the ZynqMP PMU
>   xlnx-zynqmp-pmu: Add the CPU and memory
>   aarch64-softmmu.mak: Use an ARM specific config
>   xlnx-pmu-iomod-intc: Add the PMU Interrupt controller
>   xlnx-zynqmp-pmu: Connect the PMU interrupt controller
>   xlnx-zynqmp-ipi: Initial version of the Xilinx IPI device
>   xlnx-zynqmp-pmu: Connect the IPI device to the PMU
>   xlnx-zynqmp: Connect the IPI device to the ZynqMP SoC
> 
>  default-configs/aarch64-softmmu.mak|   1 +
>  default-configs/microblaze-softmmu.mak |   1 +
>  hw/arm/Makefile.objs   |   2 +-
>  hw/arm/xlnx-zynqmp.c   |  14 +
>  hw/display/Makefile.objs   |   2 +-
>  hw/dma/Makefile.objs   |   2 +-
>  hw/intc/Makefile.objs  |   2 +
>  hw/intc/xlnx-pmu-iomod-intc.c  | 554 
> +
>  hw/intc/xlnx-zynqmp-ipi.c  | 377 ++
>  hw/microblaze/Makefile.objs|   1 +
>  hw/microblaze/xlnx-zynqmp-pmu.c| 200 
>  include/hw/arm/xlnx-zynqmp.h   |   2 +
>  include/hw/intc/xlnx-pmu-iomod-intc.h  |  58 
>  include/hw/intc/xlnx-zynqmp-ipi.h  |  57 
>  14 files changed, 1270 insertions(+), 3 deletions(-)
>  create mode 100644 hw/intc/xlnx-pmu-iomod-intc.c
>  create mode 100644 hw/intc/xlnx-zynqmp-ipi.c
>  create mode 100644 hw/microblaze/xlnx-zynqmp-pmu.c
>  create mode 100644 include/hw/intc/xlnx-pmu-iomod-intc.h
>  create mode 100644 include/hw/intc/xlnx-zynqmp-ipi.h
> 
> -- 
> 2.11.0
> 

Re: [Qemu-devel] [PATCH v2 1/2] spapr: introduce SPAPR_MACHINE_NOASSERT()

[Peter Maydell]

On 8 October 2017 at 18:17, Greg Kurz  wrote:
> A spapr-cpu-core device can only be plugged into a pseries machine. This
> is checked in spapr_cpu_core_realize() with object_dynamic_cast() instead
> of the usual SPAPR_MACHINE() macro because we don't want QEMU to abort.
>
> This patch moves the gory details to a SPAPR_MACHINE_NOASSERT() macro
> that acts like the SPAPR_MACHINE() one, except it returns NULL instead
> of aborting if its argument doesn't point to a pseries machine type.
>
> This is done for two reasons:
> - it makes the code nicer
> - it may be used by other pseries-specific devices like PHBs for example
>
> Signed-off-by: Greg Kurz 
> ---
>  hw/ppc/spapr_cpu_core.c |7 +++
>  include/hw/ppc/spapr.h  |3 +++
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 37beb56e8b18..5fde07614218 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -199,7 +199,7 @@ error:
>
>  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>  {
> -sPAPRMachineState *spapr;
> +sPAPRMachineState *spapr = SPAPR_MACHINE_NOASSERT(qdev_get_machine());
>  sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
>  sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
>  CPUCore *cc = CPU_CORE(OBJECT(dev));
> @@ -209,9 +209,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, 
> Error **errp)
>  void *obj;
>  int i, j;
>
> -spapr = (sPAPRMachineState *) qdev_get_machine();
> -if (!object_dynamic_cast((Object *) spapr, TYPE_SPAPR_MACHINE)) {
> -error_setg(errp, "spapr-cpu-core needs a pseries machine");
> +if (!spapr) {
> +error_setg(errp, TYPE_SPAPR_CPU_CORE " needs a pseries machine");
>  return;
>  }
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index c1b365f56431..4933da8083df 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -43,6 +43,9 @@ typedef struct sPAPRMachineClass sPAPRMachineClass;
>  #define SPAPR_MACHINE_CLASS(klass) \
>  OBJECT_CLASS_CHECK(sPAPRMachineClass, klass, TYPE_SPAPR_MACHINE)
>
> +#define SPAPR_MACHINE_NOASSERT(obj) \
> +((sPAPRMachineState *) object_dynamic_cast(obj, TYPE_SPAPR_MACHINE))
> +

I don't think this is a great idea. Doing things with pointers
that might not be of the right type should be obvious, not hidden
under macros. An opencoded call to object_dynamic_cast is how the
rest of the codebase does this; it's a bit of a weird way
to write "isinstance()" but there you go. If we want to
improve the way we write this sort of thing we should do
so as a general improvement to the QOM APIs and conventions,
not just a single thing in SPAPR code.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 36/88] SLIRP: use g_new() family of functions

[Samuel Thibault]

Hello,

Philippe Mathieu-Daudé, on ven. 06 oct. 2017 20:49:31 -0300, wrote:
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -869,7 +869,7 @@ net_init_slirp_configs(const StringList *fwd, int flags)
>  while (fwd) {
>  struct slirp_config_str *config;
>  
> -config = g_malloc0(sizeof(*config));
> +config = g_new0(struct slirp_config_str, 1);

It doesn't really seem like an improvement to me in such case: we don't
have overflow issues here, and using sizeof(*config) makes the line
clearly correct, while expliciting the struct behind means we need to
make sure of the coherency.

> diff --git a/slirp/socket.c b/slirp/socket.c
> index cb7b5b608d..2eccb68c2e 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -48,7 +48,7 @@ socreate(Slirp *slirp)
>  {
>struct socket *so;
>  
> -  so = (struct socket *)malloc(sizeof(struct socket));
> +  so = g_new(struct socket, 1);

Similarly, here I'd rather write g_malloc(sizeof(*so));

Samuel

Re: [Qemu-devel] [PATCH v2 1/2] spapr: introduce SPAPR_MACHINE_NOASSERT()

[David Gibson]

On Sun, Oct 08, 2017 at 09:05:37PM +0100, Peter Maydell wrote:
> On 8 October 2017 at 18:17, Greg Kurz  wrote:
> > A spapr-cpu-core device can only be plugged into a pseries machine. This
> > is checked in spapr_cpu_core_realize() with object_dynamic_cast() instead
> > of the usual SPAPR_MACHINE() macro because we don't want QEMU to abort.
> >
> > This patch moves the gory details to a SPAPR_MACHINE_NOASSERT() macro
> > that acts like the SPAPR_MACHINE() one, except it returns NULL instead
> > of aborting if its argument doesn't point to a pseries machine type.
> >
> > This is done for two reasons:
> > - it makes the code nicer
> > - it may be used by other pseries-specific devices like PHBs for example
> >
> > Signed-off-by: Greg Kurz 
> > ---
> >  hw/ppc/spapr_cpu_core.c |7 +++
> >  include/hw/ppc/spapr.h  |3 +++
> >  2 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 37beb56e8b18..5fde07614218 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -199,7 +199,7 @@ error:
> >
> >  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >  {
> > -sPAPRMachineState *spapr;
> > +sPAPRMachineState *spapr = SPAPR_MACHINE_NOASSERT(qdev_get_machine());
> >  sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> >  sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
> >  CPUCore *cc = CPU_CORE(OBJECT(dev));
> > @@ -209,9 +209,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, 
> > Error **errp)
> >  void *obj;
> >  int i, j;
> >
> > -spapr = (sPAPRMachineState *) qdev_get_machine();
> > -if (!object_dynamic_cast((Object *) spapr, TYPE_SPAPR_MACHINE)) {
> > -error_setg(errp, "spapr-cpu-core needs a pseries machine");
> > +if (!spapr) {
> > +error_setg(errp, TYPE_SPAPR_CPU_CORE " needs a pseries machine");
> >  return;
> >  }
> >
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index c1b365f56431..4933da8083df 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -43,6 +43,9 @@ typedef struct sPAPRMachineClass sPAPRMachineClass;
> >  #define SPAPR_MACHINE_CLASS(klass) \
> >  OBJECT_CLASS_CHECK(sPAPRMachineClass, klass, TYPE_SPAPR_MACHINE)
> >
> > +#define SPAPR_MACHINE_NOASSERT(obj) \
> > +((sPAPRMachineState *) object_dynamic_cast(obj, TYPE_SPAPR_MACHINE))
> > +
> 
> I don't think this is a great idea. Doing things with pointers
> that might not be of the right type should be obvious, not hidden
> under macros. An opencoded call to object_dynamic_cast is how the
> rest of the codebase does this; it's a bit of a weird way
> to write "isinstance()" but there you go. If we want to
> improve the way we write this sort of thing we should do
> so as a general improvement to the QOM APIs and conventions,
> not just a single thing in SPAPR code.

Yeah, I tend to agree.  Sorry, the original advice I gave on not using
object_dynamic_cast() directly was bogus - I didn't think it through
clearly.

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v5 5/6] ppc: spapr: Enable FWNMI capability

[David Gibson]

On Sun, Oct 08, 2017 at 01:56:06PM +0530, Aravinda Prasad wrote:
> 
> 
> On Wednesday 04 October 2017 07:04 AM, David Gibson wrote:
> > On Thu, Sep 28, 2017 at 04:08:21PM +0530, Aravinda Prasad wrote:
> >> Enable the KVM capability KVM_CAP_PPC_FWNMI so that
> >> the KVM causes guest exit with NMI as exit reason
> >> when it encounters a machine check exception on the
> >> address belonging to a guest. Without this capability
> >> enabled, KVM redirects machine check exceptions to
> >> guest's 0x200 vector.
> >>
> >> Signed-off-by: Aravinda Prasad 
> >> ---
> >>  hw/ppc/spapr_rtas.c  |   15 +++
> >>  target/ppc/kvm.c |   13 +
> >>  target/ppc/kvm_ppc.h |6 ++
> >>  3 files changed, 34 insertions(+)
> >>
> >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> >> index 08e9a5e..d017a67 100644
> >> --- a/hw/ppc/spapr_rtas.c
> >> +++ b/hw/ppc/spapr_rtas.c
> >> @@ -46,6 +46,7 @@
> >>  #include "qemu/cutils.h"
> >>  #include "trace.h"
> >>  #include "hw/ppc/fdt.h"
> >> +#include "kvm_ppc.h"
> >>  
> >>  static void rtas_display_character(PowerPCCPU *cpu, sPAPRMachineState 
> >> *spapr,
> >> uint32_t token, uint32_t nargs,
> >> @@ -354,6 +355,20 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
> >>target_ulong args,
> >>uint32_t nret, target_ulong rets)
> >>  {
> >> +int ret;
> >> +
> >> +ret = kvmppc_fwnmi_enable(cpu);
> > 
> > If you're enabling it here, doesn't that mean you need to disable it
> > on reset?
> 
> We don't have a way in KVM to disable it once enabled.

Hm, ok.  Is it possible to simulate "old style" handling in qemu, even
if KVM is in the new mode?  I guess you can catch the NMI exit from
KVM, but redirect it back to the 0x200 vector in the guest, yes?

If that's so, you might as well try to enable this in the kernel at
machine init time, rather than waiting until nmi-register is called.
That way the user gets an error early if the kernel can't handle it
on a VM that's supposed to support it.

> 
> > 
> >> +if (ret == 1) {
> >> +rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
> >> +return;
> >> +}
> >> +
> >> +if (ret < 0) {
> >> +rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> >> +return;
> >> +}
> >> +
> >>  spapr->guest_machine_check_addr = rtas_ld(args, 1);
> >>  rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> >>  }
> >> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> >> index 7e4ce02..59b3322 100644
> >> --- a/target/ppc/kvm.c
> >> +++ b/target/ppc/kvm.c
> >> @@ -92,6 +92,7 @@ static int cap_mmu_radix;
> >>  static int cap_mmu_hash_v3;
> >>  static int cap_resize_hpt;
> >>  static int cap_ppc_pvr_compat;
> >> +static int cap_fwnmi;
> >>  
> >>  static uint32_t debug_inst_opcode;
> >>  
> >> @@ -150,6 +151,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> >>  cap_mmu_radix = kvm_vm_check_extension(s, KVM_CAP_PPC_MMU_RADIX);
> >>  cap_mmu_hash_v3 = kvm_vm_check_extension(s, KVM_CAP_PPC_MMU_HASH_V3);
> >>  cap_resize_hpt = kvm_vm_check_extension(s, KVM_CAP_SPAPR_RESIZE_HPT);
> >> +cap_fwnmi = kvm_check_extension(s, KVM_CAP_PPC_FWNMI);
> >>  /*
> >>   * Note: setting it to false because there is not such capability
> >>   * in KVM at this moment.
> >> @@ -2142,6 +2144,17 @@ void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int 
> >> mpic_proxy)
> >>  }
> >>  }
> >>  
> >> +int kvmppc_fwnmi_enable(PowerPCCPU *cpu)
> >> +{
> >> +CPUState *cs = CPU(cpu);
> >> +
> >> +if (!cap_fwnmi) {
> >> +return 1;
> >> +}
> >> +
> >> +return kvm_vcpu_enable_cap(cs, KVM_CAP_PPC_FWNMI, 0);
> > 
> > Yeah, this is no good.  It means migration from a host that's fwnmi
> > capable to one that isn't will be subtly broken.  Instead you need to
> > make fwnmi capability a machine property.  If the property is
> > requested and the host kernel doesn't support it, you need to outright
> > fail, rather than try to fall back.
> 
> Sure.
> 
> > 
> >> +}
> >> +
> >>  int kvmppc_smt_threads(void)
> >>  {
> >>  return cap_ppc_smt ? cap_ppc_smt : 1;
> >> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> >> index 0139dae..55b6df2 100644
> >> --- a/target/ppc/kvm_ppc.h
> >> +++ b/target/ppc/kvm_ppc.h
> >> @@ -28,6 +28,7 @@ void kvmppc_enable_clear_ref_mod_hcalls(void);
> >>  void kvmppc_set_papr(PowerPCCPU *cpu);
> >>  int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr);
> >>  void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy);
> >> +int kvmppc_fwnmi_enable(PowerPCCPU *cpu);
> >>  int kvmppc_smt_threads(void);
> >>  void kvmppc_hint_smt_possible(Error **errp);
> >>  int kvmppc_set_smt_threads(int smt);
> >> @@ -157,6 +158,11 @@ static inline void kvmppc_set_mpic_proxy(PowerPCCPU 
> >> *cpu, int mpic_proxy)
> >>  {
> >>  }
> >>  
> >> +int kvmppc_fwnmi_enable(PowerPCCPU *cpu)
> >> +{
> >> +return 1;
> > 
> > Likewise, this 

Re: [Qemu-devel] [PATCH v5 4/6] target/ppc: Handle NMI guest exit

[David Gibson]

On Sun, Oct 08, 2017 at 02:29:26PM +0530, Aravinda Prasad wrote:
> 
> 
> On Wednesday 04 October 2017 06:59 AM, David Gibson wrote:
> > On Thu, Sep 28, 2017 at 04:08:10PM +0530, Aravinda Prasad wrote:
> >> Memory error such as bit flips that cannot be corrected
> >> by hardware are passed on to the kernel for handling.
> >> If the memory address in error belongs to guest then
> >> the guest kernel is responsible for taking suitable action.
> >> Patch [1] enhances KVM to exit guest with exit reason
> >> set to KVM_EXIT_NMI in such cases.
> >>
> >> This patch handles KVM_EXIT_NMI exit. If the guest OS
> >> has registered the machine check handling routine by
> >> calling "ibm,nmi-register", then the handler builds
> >> the error log and invokes the registered handler else
> >> invokes the handler at 0x200.
> >>
> >> Note that FWNMI handles synchronous machine check exceptions
> >> triggered by the hardware and hence we do not extend
> >> such support to the "nmi" command available in the QEMU
> >> monitor. Hence, "nmi" command from the monitor will
> >> always go through 0x200 vector.
> >>
> >> [1] https://www.spinics.net/lists/kvm-ppc/msg12637.html
> >>(e20bbd3d and related commits)
> > 
> > What does happen on KVM if an asynchronous machine check exception
> > occurs while in the guest?  Or under PowerVM for that matter.
> 
> AFAIK asynchronous errors take a different path in KVM as it can happen
> in a different process context.

Well, obviously, I'm wondering what impact it will have on the guest,
one way or another.

[snip]
> >> +ssize_t spapr_get_rtas_size(void)
> >> +{
> >> +return RTAS_ERRLOG_OFFSET + sizeof(struct rtas_event_log_mce);
> > 
> > Erm.. because of the definition of rtas_event_log_mce, this only
> > allows for 1 byte of extended log buffer.  That doesn't seem right.
> 
> This is directly taken from the kernel's RTAS log (struct rtas_error_log
> in arch/powerpc/include/asm/rtas.h). I am not sure why they use 1 byte
> extended log buffer.

I think you'd better find out, then.

[snip]
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index 28b6e2e..a75e9cf 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -556,6 +556,9 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, 
> >> target_ulong opcode,
> >>  #define DIAGNOSTICS_RUN_MODE_IMMEDIATE 2
> >>  #define DIAGNOSTICS_RUN_MODE_PERIODIC  3
> >>  
> >> +/* Offset from rtas-base where error log is placed */
> >> +#define RTAS_ERRLOG_OFFSET   0x200
> > 
> > Is there any particular rationale for this offset?  Our actual RTAS
> > code is 20 bytes, much smaller than this.
> 
> Just to ensure some space if in case RTAS code needs to be extended in
> future.

Hm, but IIUC, we control both sides here.  qemu puts the log into the
RTAS buffer at a particular offset, and qemu tells the guest where to
find it at a particular offset within the RTAS buffer.

So, if we need to extend the RTAS code (unlikely) we can increase our
offset, and the guest will be none the wiser.

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2 2/6] aspeed: add an I2C RTC device to all machines

[Andrew Jeffery]

On Wed, 2017-09-20 at 09:01 +0200, Cédric Le Goater wrote:
> The AST2500 EVB does not have an RTC but we can pretend that one is
> plugged on the I2C bus header.
> 
> The romulus and witherspoon boards expects an Epson RX8900 I2C RTC but
> a ds1338 is good enough for the basic features we need.
> 
> Signed-off-by: Cédric Le Goater 

Reviewed-by: Andrew Jeffery 

> ---
>  hw/arm/aspeed.c | 19 +++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 81f522f711ae..362b683e9021 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -89,6 +89,7 @@ enum {
>  
>  static void palmetto_bmc_i2c_init(AspeedBoardState *bmc);
>  static void ast2500_evb_i2c_init(AspeedBoardState *bmc);
> +static void romulus_bmc_i2c_init(AspeedBoardState *bmc);
>  static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc);
>  
>  static const AspeedBoardConfig aspeed_boards[] = {
> @@ -114,6 +115,7 @@ static const AspeedBoardConfig aspeed_boards[] = {
>  .fmc_model = "n25q256a",
>  .spi_model = "mx66l1g45g",
>  .num_cs= 2,
> +.i2c_init  = romulus_bmc_i2c_init,
>  },
>  [WITHERSPOON_BMC]  = {
>  .soc_name  = "ast2500-a1",
> @@ -298,6 +300,10 @@ static void ast2500_evb_i2c_init(AspeedBoardState *bmc)
>  
>  /* The AST2500 EVB expects a LM75 but a TMP105 is compatible */
>  i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 7), "tmp105", 
> 0x4d);
> +
> +/* The AST2500 EVB does not have an RTC. Let's pretend that one is
> + * plugged on the I2C bus header */
> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 11), "ds1338", 
> 0x32);
>  }
>  
>  static void ast2500_evb_init(MachineState *machine)
> @@ -325,6 +331,15 @@ static const TypeInfo ast2500_evb_type = {
>  .class_init = ast2500_evb_class_init,
>  };
>  
> +static void romulus_bmc_i2c_init(AspeedBoardState *bmc)
> +{
> +AspeedSoCState *soc = >soc;
> +
> +/* The romulus board expects Epson RX8900 I2C RTC but a ds1338 is
> + * good enough */
> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 11), "ds1338", 
> 0x32);
> +}
> +
>  static void romulus_bmc_init(MachineState *machine)
>  {
>  aspeed_board_init(machine, _boards[ROMULUS_BMC]);
> @@ -358,6 +373,10 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState 
> *bmc)
>  i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 5), "tmp423", 
> 0x4c);
>  
>  i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 9), "tmp105", 
> 0x4a);
> +
> +/* The witherspoon board expects Epson RX8900 I2C RTC but a ds1338 is
> + * good enough */
> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 11), "ds1338", 
> 0x32);
>  }
>  
>  static void witherspoon_bmc_init(MachineState *machine)

signature.asc
Description: This is a digitally signed message part

Re: [Qemu-devel] [PATCH v2 4/6] aspeed: Add EEPROM I2C devices

[Andrew Jeffery]

On Wed, 2017-09-20 at 09:01 +0200, Cédric Le Goater wrote:
> The Aspeed boards have at least one EEPROM to hold the Vital Product
> Data (VPD).
> 
> Signed-off-by: Cédric Le Goater 

Reviewed-by: Andrew Jeffery 

> ---
> 
> Changes since v1:
> 
>  - fixed palmetto EEPROM size
>  - used smbus_eeprom_init_one()
> 
> hw/arm/aspeed.c | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 362b683e9021..462f8008cff5 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -17,6 +17,7 @@
>  #include "hw/arm/arm.h"
>  #include "hw/arm/aspeed_soc.h"
>  #include "hw/boards.h"
> +#include "hw/i2c/smbus.h"
>  #include "qemu/log.h"
>  #include "sysemu/block-backend.h"
>  #include "sysemu/blockdev.h"
> @@ -255,11 +256,15 @@ static void palmetto_bmc_i2c_init(AspeedBoardState *bmc)
>  {
>  AspeedSoCState *soc = >soc;
>  DeviceState *dev;
> +uint8_t *eeprom_buf = g_malloc0(32 * 1024);
>  
>  /* The palmetto platform expects a ds3231 RTC but a ds1338 is
>   * enough to provide basic RTC features. Alarms will be missing */
>  i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 0), "ds1338", 
> 0x68);
>  
> +smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(>i2c), 0), 0x50,
> +  eeprom_buf);
> +
>  /* add a TMP423 temperature sensor */
>  dev = i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 2),
> "tmp423", 0x4c);
> @@ -297,6 +302,10 @@ static const TypeInfo palmetto_bmc_type = {
>  static void ast2500_evb_i2c_init(AspeedBoardState *bmc)
>  {
>  AspeedSoCState *soc = >soc;
> +uint8_t *eeprom_buf = g_malloc0(8 * 1024);
> +
> +smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(>i2c), 3), 0x50,
> +  eeprom_buf);
>  
>  /* The AST2500 EVB expects a LM75 but a TMP105 is compatible */
>  i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 7), "tmp105", 
> 0x4d);
> @@ -368,6 +377,7 @@ static const TypeInfo romulus_bmc_type = {
>  static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
>  {
>  AspeedSoCState *soc = >soc;
> +uint8_t *eeprom_buf = g_malloc0(8 * 1024);
>  
>  i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 4), "tmp423", 
> 0x4c);
>  i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 5), "tmp423", 
> 0x4c);
> @@ -377,6 +387,9 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState 
> *bmc)
>  /* The witherspoon board expects Epson RX8900 I2C RTC but a ds1338 is
>   * good enough */
>  i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 11), "ds1338", 
> 0x32);
> +
> +smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(>i2c), 11), 0x51,
> +  eeprom_buf);
>  }
>  
>  static void witherspoon_bmc_init(MachineState *machine)

signature.asc
Description: This is a digitally signed message part

Re: [Qemu-devel] [PATCH v2 6/6] aspeed: add the pc9552 chips to the witherspoon machine

[Andrew Jeffery]

On Wed, 2017-09-20 at 09:01 +0200, Cédric Le Goater wrote:
> The pca9552 LED blinkers on the Witherspoon machine are used for leds
> but also as GPIOs to control fans and GPUs.
> 
> Signed-off-by: Cédric Le Goater 

Reviewed-by: Andrew Jeffery 

> ---
>  hw/arm/aspeed.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 462f8008cff5..2ae46d71bff7 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -379,6 +379,8 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState 
> *bmc)
>  AspeedSoCState *soc = >soc;
>  uint8_t *eeprom_buf = g_malloc0(8 * 1024);
>  
> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 3), "pca9552", 
> 0x60);
> +
>  i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 4), "tmp423", 
> 0x4c);
>  i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 5), "tmp423", 
> 0x4c);
>  
> @@ -390,6 +392,8 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState 
> *bmc)
>  
>  smbus_eeprom_init_one(aspeed_i2c_get_bus(DEVICE(>i2c), 11), 0x51,
>    eeprom_buf);
> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 11), "pca9552",
> + 0x60);
>  }
>  
>  static void witherspoon_bmc_init(MachineState *machine)

signature.asc
Description: This is a digitally signed message part

Re: [Qemu-devel] [RFC PATCH qemu] virtio-pci: Replace modern_as with direct access to modern_bar

[Alexey Kardashevskiy]

On 06/10/17 23:58, Paolo Bonzini wrote:
> On 03/10/2017 04:11, Alexey Kardashevskiy wrote:
 What is best to do if MR is not found (and is it possible in practice)?
 assert or redirect to unassigned memory?
>>> Do nothing at all, I think.
>> Repost without asserts?
> 
> Yes, I guess so.

easy, done :)

> 
>>> I haven't thought much about the endian switches, but it looks good
>>> apart from that.
>> What is next? Michael?
> 
> Yes, I was going to ping him about this patch too.
> 
> Paolo


-- 
Alexey

Re: [Qemu-devel] [PATCH v2] watchdog/aspeed: fix variable type to store reload value

[Andrew Jeffery]

On Wed, 2017-09-20 at 08:49 +0200, Cédric Le Goater wrote:
> Initially from Anton D. Kachalov"  but the SoB was
> missing.
> 
> Signed-off-by: Cédric Le Goater 
> [clg: change commit log and subject
>   replace UL suffix by ULL ]
> Signed-off-by: Cédric Le Goater 

Acked-by: Andrew Jeffery 

> ---
>  hw/watchdog/wdt_aspeed.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index 22bce364d7b5..95f6ad186d72 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -100,13 +100,13 @@ static uint64_t aspeed_wdt_read(void *opaque, hwaddr 
> offset, unsigned size)
>  
>  static void aspeed_wdt_reload(AspeedWDTState *s, bool pclk)
>  {
> -uint32_t reload;
> +uint64_t reload;
>  
>  if (pclk) {
>  reload = muldiv64(s->regs[WDT_RELOAD_VALUE], NANOSECONDS_PER_SECOND,
>    s->pclk_freq);
>  } else {
> -reload = s->regs[WDT_RELOAD_VALUE] * 1000;
> +reload = s->regs[WDT_RELOAD_VALUE] * 1000ULL;
>  }
>  
>  if (aspeed_wdt_is_enabled(s)) {

signature.asc
Description: This is a digitally signed message part

Re: [Qemu-devel] [PATCH v2 1/6] aspeed: add support for the witherspoon-bmc board

[Andrew Jeffery]

On Wed, 2017-09-20 at 09:01 +0200, Cédric Le Goater wrote:
> The Witherspoon boards are OpenPOWER system hosting POWER9 Processors.
> Let's add support for their BMC including a couple of I2C devices as
> found on real HW.
> 
> Signed-off-by: Cédric Le Goater 
> ---
>  hw/arm/aspeed.c | 49 +
>  1 file changed, 49 insertions(+)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index ab895ad490af..81f522f711ae 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -46,6 +46,7 @@ enum {
>  PALMETTO_BMC,
>  AST2500_EVB,
>  ROMULUS_BMC,
> +WITHERSPOON_BMC,
>  };
>  
>  /* Palmetto hardware value: 0x120CE416 */
> @@ -83,8 +84,12 @@ enum {
>  SCU_AST2500_HW_STRAP_ACPI_ENABLE |  \
>  SCU_HW_STRAP_SPI_MODE(SCU_HW_STRAP_SPI_MASTER))
>  
> +/* Witherspoon hardware value: 0xF10AD216 (but use romulus definition) */
> +#define WITHERSPOON_BMC_HW_STRAP1 ROMULUS_BMC_HW_STRAP1
> +
>  static void palmetto_bmc_i2c_init(AspeedBoardState *bmc);
>  static void ast2500_evb_i2c_init(AspeedBoardState *bmc);
> +static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc);
>  
>  static const AspeedBoardConfig aspeed_boards[] = {
>  [PALMETTO_BMC] = {
> @@ -110,6 +115,14 @@ static const AspeedBoardConfig aspeed_boards[] = {
>  .spi_model = "mx66l1g45g",
>  .num_cs= 2,
>  },
> +[WITHERSPOON_BMC]  = {
> +.soc_name  = "ast2500-a1",
> +.hw_strap1 = WITHERSPOON_BMC_HW_STRAP1,
> +.fmc_model = "mx25l25635e",
> +.spi_model = "mx66l1g45g",
> +.num_cs= 2,
> +.i2c_init  = witherspoon_bmc_i2c_init,
> +},
>  };
>  
>  #define FIRMWARE_ADDR 0x0
> @@ -337,11 +350,47 @@ static const TypeInfo romulus_bmc_type = {
>  .class_init = romulus_bmc_class_init,
>  };
>  
> +static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
> +{
> +AspeedSoCState *soc = >soc;
> +
> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 4), "tmp423", 
> 0x4c);
> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 5), "tmp423", 
> 0x4c);
> +
> +i2c_create_slave(aspeed_i2c_get_bus(DEVICE(>i2c), 9), "tmp105", 
> 0x4a);

Looks like I need to track down newer versions of the schematics I have.

> +}
> +
> +static void witherspoon_bmc_init(MachineState *machine)
> +{
> +aspeed_board_init(machine, _boards[WITHERSPOON_BMC]);
> +}
> +
> +static void witherspoon_bmc_class_init(ObjectClass *oc, void *data)
> +{
> +MachineClass *mc = MACHINE_CLASS(oc);
> +
> +mc->desc = "OpenPOWER Witherspoon BMC (ARM1176)";
> +mc->init = witherspoon_bmc_init;
> +mc->max_cpus = 1;
> +mc->no_sdcard = 1;
> +mc->no_floppy = 1;
> +mc->no_cdrom = 1;
> +mc->no_parallel = 1;
> +mc->ignore_memory_transaction_failures = true;

Aside from the issue with the above as pointed out by Peter,

Reviewed-by: Andrew Jeffery 

> +}
> +
> +static const TypeInfo witherspoon_bmc_type = {
> +.name = MACHINE_TYPE_NAME("witherspoon-bmc"),
> +.parent = TYPE_MACHINE,
> +.class_init = witherspoon_bmc_class_init,
> +};
> +
>  static void aspeed_machine_init(void)
>  {
>  type_register_static(_bmc_type);
>  type_register_static(_evb_type);
>  type_register_static(_bmc_type);
> +type_register_static(_bmc_type);
>  }
>  
>  type_init(aspeed_machine_init)

signature.asc
Description: This is a digitally signed message part

Re: [Qemu-devel] [PATCH v2 3/6] smbus: add a smbus_eeprom_init_one() routine

[Philippe Mathieu-Daudé]

On 09/20/2017 04:01 AM, Cédric Le Goater wrote:
> This is an helper routine to add a single EEPROM on an I2C bus. It can
> be directly used by smbus_eeprom_init() which adds a certain number of
> EEPROMs on mips and x86 machines.
> 
> Signed-off-by: Cédric Le Goater 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/i2c/smbus_eeprom.c  | 16 +++-
>  include/hw/i2c/smbus.h |  1 +
>  2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index b13ec0fe7a2a..2d24a4cd59bf 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -140,6 +140,16 @@ static void smbus_eeprom_register_types(void)
>  
>  type_init(smbus_eeprom_register_types)
>  
> +void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t 
> *eeprom_buf)
> +{
> +DeviceState *dev;
> +
> +dev = qdev_create((BusState *) smbus, "smbus-eeprom");
> +qdev_prop_set_uint8(dev, "address", address);
> +qdev_prop_set_ptr(dev, "data", eeprom_buf);
> +qdev_init_nofail(dev);
> +}
> +
>  void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
> const uint8_t *eeprom_spd, int eeprom_spd_size)
>  {
> @@ -150,10 +160,6 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
>  }
>  
>  for (i = 0; i < nb_eeprom; i++) {
> -DeviceState *eeprom;
> -eeprom = qdev_create((BusState *)smbus, "smbus-eeprom");
> -qdev_prop_set_uint8(eeprom, "address", 0x50 + i);
> -qdev_prop_set_ptr(eeprom, "data", eeprom_buf + (i * 256));
> -qdev_init_nofail(eeprom);
> +smbus_eeprom_init_one(smbus, 0x50 + i, eeprom_buf + (i * 256));
>  }
>  }
> diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
> index 544bbc19574f..666cdeb04c07 100644
> --- a/include/hw/i2c/smbus.h
> +++ b/include/hw/i2c/smbus.h
> @@ -77,6 +77,7 @@ int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t 
> command, uint8_t *data);
>  int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t 
> *data,
>int len);
>  
> +void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t 
> *eeprom_buf);
>  void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
> const uint8_t *eeprom_spd, int size);
>  
> 

[Qemu-devel] [PATCH qemu v2] virtio-pci: Replace modern_as with direct access to modern_bar

[Alexey Kardashevskiy]

The modern bar is accessed now via yet another address space created just
for that purpose and it does not really need FlatView and dispatch tree
as it has a single memory region so it is just a waste of memory. Things
get even worse when there are dozens or hundreds of virtio-pci devices -
since these address spaces are global, changing any of them triggers
rebuilding all address spaces.

This replaces indirect accesses to the modern BAR with a simple lookup
and direct calls to memory_region_dispatch_read/write.

This is expected to save lots of memory at boot time after applying:
[Qemu-devel] [PULL 00/32] Misc changes for 2017-09-22

Signed-off-by: Alexey Kardashevskiy 
---
Changes:
v2:
* replaced assert(mr) with a silent return
---
 hw/virtio/virtio-pci.h | 17 +++-
 hw/virtio/virtio-pci.c | 75 +-
 2 files changed, 54 insertions(+), 38 deletions(-)

diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 69f5959623..12d3a90686 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -155,15 +155,18 @@ typedef struct VirtIOPCIQueue {
 struct VirtIOPCIProxy {
 PCIDevice pci_dev;
 MemoryRegion bar;
-VirtIOPCIRegion common;
-VirtIOPCIRegion isr;
-VirtIOPCIRegion device;
-VirtIOPCIRegion notify;
-VirtIOPCIRegion notify_pio;
+union {
+struct {
+VirtIOPCIRegion common;
+VirtIOPCIRegion isr;
+VirtIOPCIRegion device;
+VirtIOPCIRegion notify;
+VirtIOPCIRegion notify_pio;
+};
+VirtIOPCIRegion regs[5];
+};
 MemoryRegion modern_bar;
 MemoryRegion io_bar;
-MemoryRegion modern_cfg;
-AddressSpace modern_as;
 uint32_t legacy_io_bar_idx;
 uint32_t msix_bar_idx;
 uint32_t modern_io_bar_idx;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 8b0d6b69cd..ce14d5ff20 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -545,6 +545,24 @@ static const MemoryRegionOps virtio_pci_config_ops = {
 .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+static MemoryRegion *virtio_address_space_lookup(VirtIOPCIProxy *proxy,
+ hwaddr *off, int len)
+{
+int i;
+VirtIOPCIRegion *reg;
+
+for (i = 0; i < ARRAY_SIZE(proxy->regs); ++i) {
+reg = >regs[i];
+if (*off >= reg->offset &&
+*off + len <= reg->offset + reg->size) {
+*off -= reg->offset;
+return >mr;
+}
+}
+
+return NULL;
+}
+
 /* Below are generic functions to do memcpy from/to an address space,
  * without byteswaps, with input validation.
  *
@@ -558,63 +576,72 @@ static const MemoryRegionOps virtio_pci_config_ops = {
  * Note: host pointer must be aligned.
  */
 static
-void virtio_address_space_write(AddressSpace *as, hwaddr addr,
+void virtio_address_space_write(VirtIOPCIProxy *proxy, hwaddr addr,
 const uint8_t *buf, int len)
 {
-uint32_t val;
+uint64_t val;
+MemoryRegion *mr;
 
 /* address_space_* APIs assume an aligned address.
  * As address is under guest control, handle illegal values.
  */
 addr &= ~(len - 1);
 
+mr = virtio_address_space_lookup(proxy, , len);
+if (!mr) {
+return;
+}
+
 /* Make sure caller aligned buf properly */
 assert(!(((uintptr_t)buf) & (len - 1)));
 
 switch (len) {
 case 1:
 val = pci_get_byte(buf);
-address_space_stb(as, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
 break;
 case 2:
-val = pci_get_word(buf);
-address_space_stw_le(as, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
+val = cpu_to_le16(pci_get_word(buf));
 break;
 case 4:
-val = pci_get_long(buf);
-address_space_stl_le(as, addr, val, MEMTXATTRS_UNSPECIFIED, NULL);
+val = cpu_to_le32(pci_get_long(buf));
 break;
 default:
 /* As length is under guest control, handle illegal values. */
-break;
+return;
 }
+memory_region_dispatch_write(mr, addr, val, len, MEMTXATTRS_UNSPECIFIED);
 }
 
 static void
-virtio_address_space_read(AddressSpace *as, hwaddr addr, uint8_t *buf, int len)
+virtio_address_space_read(VirtIOPCIProxy *proxy, hwaddr addr,
+  uint8_t *buf, int len)
 {
-uint32_t val;
+uint64_t val;
+MemoryRegion *mr;
 
 /* address_space_* APIs assume an aligned address.
  * As address is under guest control, handle illegal values.
  */
 addr &= ~(len - 1);
 
+mr = virtio_address_space_lookup(proxy, , len);
+if (!mr) {
+return;
+}
+
 /* Make sure caller aligned buf properly */
 assert(!(((uintptr_t)buf) & (len - 1)));
 
+memory_region_dispatch_read(mr, addr, , len, MEMTXATTRS_UNSPECIFIED);
 switch (len) {
 case 1:
-val = address_space_ldub(as, addr, MEMTXATTRS_UNSPECIFIED, NULL);
 

Re: [Qemu-devel] [PATCH v2 1/2] exec: add page_mask for flatview_do_translate

[Peter Xu]

On Fri, Oct 06, 2017 at 03:03:50PM +0200, Maxime Coquelin wrote:
> 
> 
> On 10/06/2017 02:48 PM, Paolo Bonzini wrote:
> >On 06/10/2017 14:46, Maxime Coquelin wrote:
>    addr = ((iotlb.translated_addr & ~iotlb.addr_mask)
>    | (addr & iotlb.addr_mask));
> -    *plen = MIN(*plen, (addr | iotlb.addr_mask) - addr + 1);
> +    page_mask = iotlb.addr_mask;
> >>>
> >>>Should this be "page_mask &= iotlb.addr_mask"?
> >>>
> >>>If you have multiple IOMMUs on top of each other (yeah, I know...) I
> >>>think the smallest size should win.  This is also consistent with the
> >>>MIN in the line below.
> >>
> >>I agree, but changin to "page_mask &= iotlb.addr_mask" will not be
> >>enough, we also have to change the init value. Else we will always end
> >>up with 0xfff.
> >>
> >>Maybe we could do as plen was handled before, i.e. setting page_mask
> >>init value to (hwaddr)(-1), and after the loop set it to
> >>~TARGET_PAGE_MASK if it hasn't been changed.
> >>
> >>Does that sound reasonable?
> >
> >True that, in fact it makes sense for the "IOTLB entry" to represent all
> >of memory if there's no IOMMU at all.
> 
> Indeed, that makes sense as no iommu means identity mapping. It would
> moreover improve performance, as the vhost backend will only have a
> single IOTLB entry in its cache.
> 
> Maybe it is better to wait for Peter to understand the reason he limited
> it to the target page size?

Sorry, just came back from a long holiday.

I was trying to use 4K as default to be safe (but yes the mask was not
correct, thanks for fixing that!), to make sure the translated range
covered by the IOMMUTLBEntry will always be safe to access (I thought
that was how IOTLB was defined, but I may be wrong).  Using (-1) is
good especially from performance POV as long as the caller knows the
real memory boundary, but I'm not sure whether it'll break the IOTLB
scemantic somehow.

If we want to make it -1 for transparent mappings, maybe worth
commenting it in definition of IOMMUTLBEntry.page_mask?

(Btw, thanks again for moving these patches forward; I tried to, but I
 failed :)

-- 
Peter Xu

Re: [Qemu-devel] What is the status of the QEMU sound rework?

[Markus Armbruster]

Copying Gerd...

Zir Blazer  writes:

> I'm a VGA Passthrough user that uses a QEMU VM to fully replace a Windows 
> native install as the main OS.
>
>
> Currently, one of the biggest hazzles for many users is that sound is hard to 
> get working properly with the emulated AC'97 and Intel HDA Sound Cards due to 
> crackle or latency issues (Latency issues heavily affects recording 
> scenarios, like voice chat applications in a VM). An example here, through 
> there are far more: 
> https://www.reddit.com/r/VFIO/comments/746t4h/getting_rid_of_audio_crackling_once_and_for_all/
>
>
> Some people managed to get sound working at acceptable levels by meddling 
> with the environmental variables, but most of the solutions come from a 
> trial-and-error esoteric procedure instead of structured troubleshooting, as 
> each user has to figure out which settings makes the emulated Sound Cards 
> work decently in his system. For those that failed getting the desired 
> results with emulation, they have to throw Hardware at the issue by using PCI 
> Passthrough of a Sound Card, or an USB Sound Card via USB Passthrough (Or 
> even have it plugged to an USB Controller passed to the VM via PCI 
> Passthrough), or using the passthroughed Video Card HDMI with a Monitor with 
> speakers, just to solve the sound issues. The problem is that this 
> complicates this type of setups a lot, and it makes impossible a typical use 
> case that would be that all the VMs output the sound to the host, which has 
> control of the Motherboard integrated Sound Card, and it mixes it to use a 
> single set of speakers for the entire system.
>
>
>
> I have been googling around and found that at several times, there were ideas 
> and proposal, and even some code, that were intended to overhaul QEMU sound 
> system. Examples includes:
>
> GSoC 2011 Xen PV Audio, a paravirtualized Sound Card 
> https://wiki.xenproject.org/wiki/Archived/GSoc_2011_Ideas
> GSoC 2015 QEMU rework of the sound environmental variables 
> https://wiki.qemu.org/Google_Summer_of_Code_2015#QEMU_audio_backend
> GSoC 2017 QEMU rework a patch series that did precisely that 
> https://wiki.qemu.org/Google_Summer_of_Code_2017#QEMU_audio_backend
> https://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg02451.html
>
>
>
> Basically, what happened with all the previous ideas and proposals? Did any 
> of that work ever get into upstream QEMU?
> Is anyone tinkering with the idea of making a paravirtualized Sound Card and 
> its associated Drivers for the mainstream OSes to try to fix once and for all 
> the sound issues?

Re: [Qemu-devel] [RFC v2 24/33] migration: synchronize dirty bitmap for resume

[Peter Xu]

On Mon, Oct 02, 2017 at 12:04:46PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > On Fri, Sep 22, 2017 at 12:33:19PM +0100, Dr. David Alan Gilbert wrote:
> > > * Peter Xu (pet...@redhat.com) wrote:
> > > > This patch implements the first part of core RAM resume logic for
> > > > postcopy. ram_resume_prepare() is provided for the work.
> > > > 
> > > > When the migration is interrupted by network failure, the dirty bitmap
> > > > on the source side will be meaningless, because even the dirty bit is
> > > > cleared, it is still possible that the sent page was lost along the way
> > > > to destination. Here instead of continue the migration with the old
> > > > dirty bitmap on source, we ask the destination side to send back its
> > > > received bitmap, then invert it to be our initial dirty bitmap.
> > > > 
> > > > The source side send thread will issue the MIG_CMD_RECV_BITMAP requests,
> > > > once per ramblock, to ask for the received bitmap. On destination side,
> > > > MIG_RP_MSG_RECV_BITMAP will be issued, along with the requested bitmap.
> > > > Data will be received on the return-path thread of source, and the main
> > > > migration thread will be notified when all the ramblock bitmaps are
> > > > synchronized.
> > > > 
> > > > Signed-off-by: Peter Xu 
> > > > ---
> > > >  migration/migration.c  |  4 +++
> > > >  migration/migration.h  |  1 +
> > > >  migration/ram.c| 67 
> > > > ++
> > > >  migration/trace-events |  4 +++
> > > >  4 files changed, 76 insertions(+)
> > > > 
> > > > diff --git a/migration/migration.c b/migration/migration.c
> > > > index 19b7f3a5..19aed72 100644
> > > > --- a/migration/migration.c
> > > > +++ b/migration/migration.c
> > > > @@ -2605,6 +2605,8 @@ static void migration_instance_finalize(Object 
> > > > *obj)
> > > >  
> > > >  g_free(params->tls_hostname);
> > > >  g_free(params->tls_creds);
> > > > +
> > > > +qemu_sem_destroy(>rp_state.rp_sem);
> > > >  }
> > > >  
> > > >  static void migration_instance_init(Object *obj)
> > > > @@ -2629,6 +2631,8 @@ static void migration_instance_init(Object *obj)
> > > >  params->has_downtime_limit = true;
> > > >  params->has_x_checkpoint_delay = true;
> > > >  params->has_block_incremental = true;
> > > > +
> > > > +qemu_sem_init(>rp_state.rp_sem, 1);
> > > >  }
> > > >  
> > > >  /*
> > > > diff --git a/migration/migration.h b/migration/migration.h
> > > > index a3a0582..d041369 100644
> > > > --- a/migration/migration.h
> > > > +++ b/migration/migration.h
> > > > @@ -107,6 +107,7 @@ struct MigrationState
> > > >  QEMUFile *from_dst_file;
> > > >  QemuThreadrp_thread;
> > > >  bool  error;
> > > > +QemuSemaphore rp_sem;
> > > >  } rp_state;
> > > >  
> > > >  double mbps;
> > > > diff --git a/migration/ram.c b/migration/ram.c
> > > > index 5d938e3..afabcf5 100644
> > > > --- a/migration/ram.c
> > > > +++ b/migration/ram.c
> > > > @@ -47,6 +47,7 @@
> > > >  #include "exec/target_page.h"
> > > >  #include "qemu/rcu_queue.h"
> > > >  #include "migration/colo.h"
> > > > +#include "savevm.h"
> > > >  
> > > >  /***/
> > > >  /* ram save/restore */
> > > > @@ -295,6 +296,8 @@ struct RAMState {
> > > >  RAMBlock *last_req_rb;
> > > >  /* Queue of outstanding page requests from the destination */
> > > >  QemuMutex src_page_req_mutex;
> > > > +/* Ramblock counts to sync dirty bitmap. Only used for recovery */
> > > > +int ramblock_to_sync;
> > > >  QSIMPLEQ_HEAD(src_page_requests, RAMSrcPageRequest) 
> > > > src_page_requests;
> > > >  };
> > > >  typedef struct RAMState RAMState;
> > > > @@ -2770,6 +2773,56 @@ static int ram_load(QEMUFile *f, void *opaque, 
> > > > int version_id)
> > > >  return ret;
> > > >  }
> > > >  
> > > > +/* Sync all the dirty bitmap with destination VM.  */
> > > > +static int ram_dirty_bitmap_sync_all(MigrationState *s, RAMState *rs)
> > > > +{
> > > > +RAMBlock *block;
> > > > +QEMUFile *file = s->to_dst_file;
> > > > +int ramblock_count = 0;
> > > > +
> > > > +trace_ram_dirty_bitmap_sync_start();
> > > > +
> > > > +/*
> > > > + * We do this in such order:
> > > > + *
> > > > + * 1. calculate block count
> > > > + * 2. fill in the count to N
> > > > + * 3. send MIG_CMD_RECV_BITMAP requests
> > > > + * 4. wait on the semaphore until N -> 0
> > > > + */
> > > > +
> > > > +RAMBLOCK_FOREACH(block) {
> > > > +ramblock_count++;
> > > > +}
> > > > +
> > > > +atomic_set(>ramblock_to_sync, ramblock_count);
> > > > +RAMBLOCK_FOREACH(block) {
> > > > +qemu_savevm_send_recv_bitmap(file, block->idstr);
> > > > +}
> > > > +
> > > > +trace_ram_dirty_bitmap_sync_wait();
> > > 
> > > Please include the RAMBlock name in the trace, so if it hangs we can
> > > see 

Re: [Qemu-devel] [PATCH 18/23] ppc: pnv: use generic cpu_model parsing

[Igor Mammedov]

On Fri, 6 Oct 2017 22:25:03 +1100
David Gibson  wrote:

> On Fri, Oct 06, 2017 at 11:30:54AM +0200, Igor Mammedov wrote:
> > On Fri, 6 Oct 2017 19:34:19 +1100
> > David Gibson  wrote:
> >   
> > > On Thu, Oct 05, 2017 at 06:24:45PM +0200, Igor Mammedov wrote:  
> > > > use common cpu_model prasing in vl.c and set default cpu_model
> > > > using generic MachineClass::default_cpu_type.
> > > > 
> > > > Beside of switching to generic infrastructure it solves several
> > > > issues.
> > > > 
> > > >  * ppc_cpu_class_by_name() is used to deal with lower/upper case
> > > >and alias translations into actual cpu type, which fixes
> > > > '-M powernv -cpu power8' and '-M powernv -cpu power9_v1.0'
> > > >usecases which error out with:
> > > > 'invalid CPU model 'FOO' for powernv machine'
> > > >  * allows to switch to lower-case typenames in pnv chip/core name
> > > >(by convention typnames should be lower-case)
> > > >  * replace aliased names /power8, power9, .../ with exact cpu model
> > > >names (i.e. typenames should be stable but aliases might decide to
> > > >point to other cpu model withi family or changed by kvm). It will
> > > >also help to simplify pnv_chip/core code and get rid of dependency
> > > >on cpu_model parsing.
> > > > 
> > > > Signed-off-by: Igor Mammedov 
> > > > ---
> > > >  include/hw/ppc/pnv.h |  8 
> > > >  hw/ppc/pnv.c | 22 ++
> > > >  hw/ppc/pnv_core.c|  2 +-
> > > >  3 files changed, 15 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> > > > index 9c5437d..2525f7f 100644
> > > > --- a/include/hw/ppc/pnv.h
> > > > +++ b/include/hw/ppc/pnv.h
> > > > @@ -80,19 +80,19 @@ typedef struct PnvChipClass {
> > > >  uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
> > > >  } PnvChipClass;
> > > >  
> > > > -#define TYPE_PNV_CHIP_POWER8E TYPE_PNV_CHIP "-POWER8E"
> > > > +#define TYPE_PNV_CHIP_POWER8E TYPE_PNV_CHIP "-power8e_v2.1"
> > > >  #define PNV_CHIP_POWER8E(obj) \
> > > >  OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP_POWER8E)
> > > >  
> > > > -#define TYPE_PNV_CHIP_POWER8 TYPE_PNV_CHIP "-POWER8"
> > > > +#define TYPE_PNV_CHIP_POWER8 TYPE_PNV_CHIP "-power8_v2.0"
> > > >  #define PNV_CHIP_POWER8(obj) \
> > > >  OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP_POWER8)
> > > >  
> > > > -#define TYPE_PNV_CHIP_POWER8NVL TYPE_PNV_CHIP "-POWER8NVL"
> > > > +#define TYPE_PNV_CHIP_POWER8NVL TYPE_PNV_CHIP "-power8nvl_v1.0"
> > > >  #define PNV_CHIP_POWER8NVL(obj) \
> > > >  OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP_POWER8NVL)
> > > >  
> > > > -#define TYPE_PNV_CHIP_POWER9 TYPE_PNV_CHIP "-POWER9"
> > > > +#define TYPE_PNV_CHIP_POWER9 TYPE_PNV_CHIP "-power9_v1.0"
> > > 
> > > Uh.. we really should add a DD2 power9 before we make this change.
> > > Making a DD1.0 (read, buggy as hell) chip the default is not
> > > sensible.  Especially since we don't implement the various DD1 bugs
> > > and differences in qemu.  
> > I guess pnv owner will have to it,
> > I can't help here /me uses whatever is in code right now/  
> 
> I just committed a patch to ppc-for-2.11 that adds POWER9 v2.0 to the
> code (and makes it the default).  Sorry, this will probably require a
> rebase of your stuff.
Do you have a pointer to the patch or even better ppc staging tree to rebase on?

Re: [Qemu-devel] [PATCH 7/8] os-posix: Provide new -runasid option

[Markus Armbruster]

Ian Jackson  writes:

> This allows the caller to specify a uid and gid to use, even if there
> is no corresponding password entry.  This will be useful in certain
> Xen configurations.
>
> Signed-off-by: Ian Jackson 
[...]
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9f6e2ad..34a5329 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3968,6 +3968,18 @@ Immediately before starting guest execution, drop root 
> privileges, switching
>  to the specified user.
>  ETEXI
>  
> +#ifndef _WIN32
> +DEF("runasid", HAS_ARG, QEMU_OPTION_runasid, \
> +"-runasid uid.gid change to numeric uid and gid just before starting 
> the VM\n",
> +QEMU_ARCH_ALL)
> +#endif
> +STEXI
> +@item -runasid @var{uid}.@var{gid}
> +@findex -runasid
> +Immediately before starting guest execution, drop root privileges, switching
> +to the specified uid and gid.
> +ETEXI
> +
>  DEF("prom-env", HAS_ARG, QEMU_OPTION_prom_env,
>  "-prom-env variable=value\n"
>  "set OpenBIOS nvram variables\n",

The last thing the QEMU command line needs is more exotic options.  Are
you sure we need a new one here?  Can we make existing -runas serve?
Precedence: Coreutils[*].  Pseudo-code:

if argument is a decimal number starting with '+':
user ID
else if argument is a valid user name:
user name
else if argument is a valid user ID:
user ID
else:
error

[*] 
https://www.gnu.org/software/coreutils/manual/html_node/Disambiguating-names-and-IDs.html

Re: [Qemu-devel] [PATCH v2 1/2] spapr: introduce SPAPR_MACHINE_NOASSERT()

[Greg Kurz]

On Mon, 9 Oct 2017 10:26:38 +1100
David Gibson  wrote:

> On Sun, Oct 08, 2017 at 09:05:37PM +0100, Peter Maydell wrote:
> > On 8 October 2017 at 18:17, Greg Kurz  wrote:  
> > > A spapr-cpu-core device can only be plugged into a pseries machine. This
> > > is checked in spapr_cpu_core_realize() with object_dynamic_cast() instead
> > > of the usual SPAPR_MACHINE() macro because we don't want QEMU to abort.
> > >
> > > This patch moves the gory details to a SPAPR_MACHINE_NOASSERT() macro
> > > that acts like the SPAPR_MACHINE() one, except it returns NULL instead
> > > of aborting if its argument doesn't point to a pseries machine type.
> > >
> > > This is done for two reasons:
> > > - it makes the code nicer
> > > - it may be used by other pseries-specific devices like PHBs for example
> > >
> > > Signed-off-by: Greg Kurz 
> > > ---
> > >  hw/ppc/spapr_cpu_core.c |7 +++
> > >  include/hw/ppc/spapr.h  |3 +++
> > >  2 files changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index 37beb56e8b18..5fde07614218 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -199,7 +199,7 @@ error:
> > >
> > >  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > >  {
> > > -sPAPRMachineState *spapr;
> > > +sPAPRMachineState *spapr = 
> > > SPAPR_MACHINE_NOASSERT(qdev_get_machine());
> > >  sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > >  sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
> > >  CPUCore *cc = CPU_CORE(OBJECT(dev));
> > > @@ -209,9 +209,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, 
> > > Error **errp)
> > >  void *obj;
> > >  int i, j;
> > >
> > > -spapr = (sPAPRMachineState *) qdev_get_machine();
> > > -if (!object_dynamic_cast((Object *) spapr, TYPE_SPAPR_MACHINE)) {
> > > -error_setg(errp, "spapr-cpu-core needs a pseries machine");
> > > +if (!spapr) {
> > > +error_setg(errp, TYPE_SPAPR_CPU_CORE " needs a pseries machine");
> > >  return;
> > >  }
> > >
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index c1b365f56431..4933da8083df 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -43,6 +43,9 @@ typedef struct sPAPRMachineClass sPAPRMachineClass;
> > >  #define SPAPR_MACHINE_CLASS(klass) \
> > >  OBJECT_CLASS_CHECK(sPAPRMachineClass, klass, TYPE_SPAPR_MACHINE)
> > >
> > > +#define SPAPR_MACHINE_NOASSERT(obj) \
> > > +((sPAPRMachineState *) object_dynamic_cast(obj, TYPE_SPAPR_MACHINE))
> > > +  
> > 
> > I don't think this is a great idea. Doing things with pointers
> > that might not be of the right type should be obvious, not hidden
> > under macros. An opencoded call to object_dynamic_cast is how the
> > rest of the codebase does this; it's a bit of a weird way
> > to write "isinstance()" but there you go. If we want to
> > improve the way we write this sort of thing we should do
> > so as a general improvement to the QOM APIs and conventions,
> > not just a single thing in SPAPR code.  
> 

Ok, I agree.

> Yeah, I tend to agree.  Sorry, the original advice I gave on not using
> object_dynamic_cast() directly was bogus - I didn't think it through
> clearly.
> 

Then maybe you can apply the previous version or do you have another
suggestion ?


pgpv5iAiT6ZZV.pgp
Description: OpenPGP digital signature