Re: [for-5.0 2/4] spapr: Improve handling of fdt buffer size

[Greg Kurz]

On Fri, 29 Nov 2019 16:33:54 +1100
David Gibson  wrote:

> Previously, spapr_build_fdt() constructed the device tree in a fixed
> buffer of size FDT_MAX_SIZE.  This is a bit inflexible, but more
> importantly it's awkward for the case where we use it during CAS.  In
> that case the guest firmware supplies a buffer and we have to
> awkwardly check that what we generated fits into it afterwards, after
> doing a lot of size checks during spapr_build_fdt().
> 
> Simplify this by having spapr_build_fdt() take a 'space' parameter.
> For the CAS case, we pass in the buffer size provided by SLOF, for the
> machine init case, we continue to pass FDT_MAX_SIZE.
> 
> Signed-off-by: David Gibson 
> ---

Reviewed-by: Greg Kurz 

>  hw/ppc/spapr.c | 33 +++--
>  1 file changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d9c9a2bcee..d34e317f48 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -917,7 +917,8 @@ static bool spapr_hotplugged_dev_before_cas(void)
>  return false;
>  }
>  
> -static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset);
> +static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
> + size_t space);
>  
>  int spapr_h_cas_compose_response(SpaprMachineState *spapr,
>   target_ulong addr, target_ulong size,
> @@ -930,24 +931,17 @@ int spapr_h_cas_compose_response(SpaprMachineState 
> *spapr,
>  return 1;
>  }
>  
> -if (size < sizeof(hdr) || size > FW_MAX_SIZE) {
> -error_report("SLOF provided an unexpected CAS buffer size "
> - TARGET_FMT_lu " (min: %zu, max: %u)",
> - size, sizeof(hdr), FW_MAX_SIZE);
> +if (size < sizeof(hdr)) {
> +error_report("SLOF provided insufficient CAS buffer "
> + TARGET_FMT_lu " (min: %zu)", size, sizeof(hdr));
>  exit(EXIT_FAILURE);
>  }
>  
>  size -= sizeof(hdr);
>  
> -fdt = spapr_build_fdt(spapr, false);
> +fdt = spapr_build_fdt(spapr, false, size);
>  _FDT((fdt_pack(fdt)));
>  
> -if (fdt_totalsize(fdt) + sizeof(hdr) > size) {
> -g_free(fdt);
> -trace_spapr_cas_failed(size);
> -return -1;
> -}
> -
>  cpu_physical_memory_write(addr, &hdr, sizeof(hdr));
>  cpu_physical_memory_write(addr + sizeof(hdr), fdt, fdt_totalsize(fdt));
>  trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
> @@ -1197,7 +1191,8 @@ static void spapr_dt_hypervisor(SpaprMachineState 
> *spapr, void *fdt)
>  }
>  }
>  
> -static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset)
> +static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
> + size_t space)
>  {
>  MachineState *machine = MACHINE(spapr);
>  MachineClass *mc = MACHINE_GET_CLASS(machine);
> @@ -1207,8 +1202,8 @@ static void *spapr_build_fdt(SpaprMachineState *spapr, 
> bool reset)
>  SpaprPhbState *phb;
>  char *buf;
>  
> -fdt = g_malloc0(FDT_MAX_SIZE);
> -_FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
> +fdt = g_malloc0(space);
> +_FDT((fdt_create_empty_tree(fdt, space)));
>  
>  /* Root node */
>  _FDT(fdt_setprop_string(fdt, 0, "device_type", "chrp"));
> @@ -1723,19 +1718,13 @@ static void spapr_machine_reset(MachineState *machine)
>   */
>  fdt_addr = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FDT_MAX_SIZE;
>  
> -fdt = spapr_build_fdt(spapr, true);
> +fdt = spapr_build_fdt(spapr, true, FDT_MAX_SIZE);
>  
>  rc = fdt_pack(fdt);
>  
>  /* Should only fail if we've built a corrupted tree */
>  assert(rc == 0);
>  
> -if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
> -error_report("FDT too big ! 0x%x bytes (max is 0x%x)",
> - fdt_totalsize(fdt), FDT_MAX_SIZE);
> -exit(1);
> -}
> -
>  /* Load the fdt */
>  qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
>  cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));

Re: [PATCH 09/21] hw/acpi: Fix latent legacy CPU plug error handling bug

[Igor Mammedov]

On Sat, 30 Nov 2019 20:42:28 +0100
Markus Armbruster  wrote:

> legacy_acpi_cpu_plug_cb() crashes when acpi_set_cpu_present_bit()
> fails and its @errp argument is null.  Messed up in commit cc43364de7
> "acpi/cpu-hotplug: introduce helper function to keep bit setting in
> one place".
> 
> The bug can't bite as no caller actually passes null, and
> acpi_set_cpu_present_bit() can't actually fail.  Fix it anyway.
> 
> Cc: "Michael S. Tsirkin" 
> Cc: Igor Mammedov 
> Signed-off-by: Markus Armbruster 

Reviewed-by: Igor Mammedov 

> ---
>  hw/acpi/cpu_hotplug.c | 10 +++---
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> index 3ac2045a95..9c3bcc84de 100644
> --- a/hw/acpi/cpu_hotplug.c
> +++ b/hw/acpi/cpu_hotplug.c
> @@ -55,8 +55,7 @@ static const MemoryRegionOps AcpiCpuHotplug_ops = {
>  },
>  };
>  
> -static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, CPUState *cpu,
> - Error **errp)
> +static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, CPUState *cpu)
>  {
>  CPUClass *k = CPU_GET_CLASS(cpu);
>  int64_t cpu_id;
> @@ -74,10 +73,7 @@ static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, 
> CPUState *cpu,
>  void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
>   AcpiCpuHotplug *g, DeviceState *dev, Error 
> **errp)
>  {
> -acpi_set_cpu_present_bit(g, CPU(dev), errp);
> -if (*errp != NULL) {
> -return;
> -}
> +acpi_set_cpu_present_bit(g, CPU(dev));
>  acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS);
>  }
>  
> @@ -92,7 +88,7 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, 
> Object *owner,
>  gpe_cpu->device = owner;
>  
>  CPU_FOREACH(cpu) {
> -acpi_set_cpu_present_bit(gpe_cpu, cpu, &error_abort);
> +acpi_set_cpu_present_bit(gpe_cpu, cpu);
>  }
>  }
>  

Re: [for-5.0 1/4] spapr: Don't trigger a CAS reboot for XICS/XIVE mode changeover

[Greg Kurz]

On Fri, 29 Nov 2019 16:33:53 +1100
David Gibson  wrote:

> PAPR allows the interrupt controller used on a POWER9 machine (XICS or
> XIVE) to be selected by the guest operating system, by using the
> ibm,client-architecture-support (CAS) feature negotiation call.
> 
> Currently, if the guest selects an interrupt controller different from the
> one selected at initial boot, this causes the system to be reset with the
> new model and the boot starts again.  This means we run through the SLOF
> boot process twice, as well as any other bootloader (e.g. grub) in use
> before the OS calls CAS.  This can be confusing and/or inconvenient for
> users.
> 
> Thanks to two fairly recent changes, we no longer need this reboot.  1) we
> now completely regenerate the device tree when CAS is called (meaning we
> don't need special case updates for all the device tree changes caused by
> the interrupt controller mode change),  2) we now have explicit code paths
> to activate and deactivate the different interrupt controllers, rather than
> just implicitly calling those at machine reset time.
> 
> We can therefore eliminate the reboot for changing irq mode, simply by
> putting a called to spapr_irq_update_active_intc() before we call

putting a call

> spapr_h_cas_compose_response() (which gives the updated device tree to the
> guest firmware and OS).
> 
> Signed-off-by: David Gibson 
> ---

Convenient indeed ! :)

Reviewed-by: Greg Kurz 

>  hw/ppc/spapr_hcall.c | 33 +
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 140f05c1c6..05a7ca275b 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1767,21 +1767,10 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu,
>  }
>  spapr->cas_pre_isa3_guest = !spapr_ovec_test(ov1_guest, OV1_PPC_3_00);
>  spapr_ovec_cleanup(ov1_guest);
> -if (!spapr->cas_reboot) {
> -/* If spapr_machine_reset() did not set up a HPT but one is necessary
> - * (because the guest isn't going to use radix) then set it up here. 
> */
> -if ((spapr->patb_entry & PATE1_GR) && !guest_radix) {
> -/* legacy hash or new hash: */
> -spapr_setup_hpt_and_vrma(spapr);
> -}
> -spapr->cas_reboot =
> -(spapr_h_cas_compose_response(spapr, args[1], args[2],
> -  ov5_updates) != 0);
> -}
>  
>  /*
> - * Ensure the guest asks for an interrupt mode we support; otherwise
> - * terminate the boot.
> + * Ensure the guest asks for an interrupt mode we support;
> + * otherwise terminate the boot.
>   */
>  if (guest_xive) {
>  if (!spapr->irq->xive) {
> @@ -1797,14 +1786,18 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu,
>  }
>  }
>  
> -/*
> - * Generate a machine reset when we have an update of the
> - * interrupt mode. Only required when the machine supports both
> - * modes.
> - */
> +spapr_irq_update_active_intc(spapr);
> +
>  if (!spapr->cas_reboot) {
> -spapr->cas_reboot = spapr_ovec_test(ov5_updates, OV5_XIVE_EXPLOIT)
> -&& spapr->irq->xics && spapr->irq->xive;
> +/* If spapr_machine_reset() did not set up a HPT but one is necessary
> + * (because the guest isn't going to use radix) then set it up here. 
> */
> +if ((spapr->patb_entry & PATE1_GR) && !guest_radix) {
> +/* legacy hash or new hash: */
> +spapr_setup_hpt_and_vrma(spapr);
> +}
> +spapr->cas_reboot =
> +(spapr_h_cas_compose_response(spapr, args[1], args[2],
> +  ov5_updates) != 0);
>  }
>  
>  spapr_ovec_cleanup(ov5_updates);

Re: [PATCH 08/21] exec: Fix latent file_ram_alloc() error handling bug

[Igor Mammedov]

On Sat, 30 Nov 2019 20:42:27 +0100
Markus Armbruster  wrote:

> When os_mem_prealloc() fails, file_ram_alloc() calls qemu_ram_munmap()
> and returns null.  Except it doesn't when its @errp argument is null,
> because it checks for failure with (errp && *errp).  Messed up in
> commit 056b68af77 "fix qemu exit on memory hotplug when allocation
> fails at prealloc time".
> 
> The bug can't bite as no caller actually passes null.  Fix it anyway.
> 
> Cc: Igor Mammedov 
> Signed-off-by: Markus Armbruster 

Reviewed-by: Igor Mammedov 

> ---
>  exec.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index ffdb518535..45695a5f2d 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1841,6 +1841,7 @@ static void *file_ram_alloc(RAMBlock *block,
>  bool truncate,
>  Error **errp)
>  {
> +Error *err = NULL;
>  MachineState *ms = MACHINE(qdev_get_machine());
>  void *area;
>  
> @@ -1898,8 +1899,9 @@ static void *file_ram_alloc(RAMBlock *block,
>  }
>  
>  if (mem_prealloc) {
> -os_mem_prealloc(fd, area, memory, ms->smp.cpus, errp);
> -if (errp && *errp) {
> +os_mem_prealloc(fd, area, memory, ms->smp.cpus, &err);
> +if (err) {
> +error_propagate(errp, err);
>  qemu_ram_munmap(fd, area, memory);
>  return NULL;
>  }

Re: [for-5.0 1/4] spapr,ppc: Simplify signature of kvmppc_rma_size()

[Cédric Le Goater]

On 29/11/2019 02:35, David Gibson wrote:
> This function calculates the maximum size of the RMA as implied by the
> host's page size of structure of the VRMA (there are a number of other
> constraints on the RMA size which will supersede this one in many
> circumstances).
> 
> The current interface takes the current RMA size estimate, and clamps it
> to the VRMA derived size.  The only current caller passes in an arguably
> wrong value (it will match the current RMA estimate in some but not all
> cases).
> 
> We want to fix that, but for now just keep concerns separated by having the
> KVM helper function just return the VRMA derived limit, and let the caller
> combine it with other constraints.  We call the new function
> kvmppc_vrma_limit() to more clearly indicate its limited responsibility.
> 
> The helper should only ever be called in the KVM enabled case, so replace
> its !CONFIG_KVM stub with an assert() rather than a dummy value.
> 
> Signed-off-by: David Gibson 

Reviewed-by: Cedric Le Goater 




> ---
>  hw/ppc/spapr.c   | 5 +++--
>  target/ppc/kvm.c | 5 ++---
>  target/ppc/kvm_ppc.h | 7 +++
>  3 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d9c9a2bcee..069bd04a8d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1635,8 +1635,9 @@ void spapr_setup_hpt_and_vrma(SpaprMachineState *spapr)
>  spapr_reallocate_hpt(spapr, hpt_shift, &error_fatal);
>  
>  if (spapr->vrma_adjust) {
> -spapr->rma_size = kvmppc_rma_size(spapr_node0_size(MACHINE(spapr)),
> -  spapr->htab_shift);
> +hwaddr vrma_limit = kvmppc_vrma_limit(spapr->htab_shift);
> +
> +spapr->rma_size = MIN(spapr_node0_size(MACHINE(spapr)), vrma_limit);
>  }
>  }
>  
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index c77f9848ec..09b3bd6443 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2101,7 +2101,7 @@ void kvmppc_hint_smt_possible(Error **errp)
>  
>  
>  #ifdef TARGET_PPC64
> -uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift)
> +uint64_t kvmppc_vrma_limit(unsigned int hash_shift)
>  {
>  struct kvm_ppc_smmu_info info;
>  long rampagesize, best_page_shift;
> @@ -2128,8 +2128,7 @@ uint64_t kvmppc_rma_size(uint64_t current_size, 
> unsigned int hash_shift)
>  }
>  }
>  
> -return MIN(current_size,
> -   1ULL << (best_page_shift + hash_shift - 7));
> +return 1ULL << (best_page_shift + hash_shift - 7));
>  }
>  #endif
>  
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 98bd7d5da6..4f0eec4c1b 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -45,7 +45,7 @@ void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t 
> page_shift,
>int *pfd, bool need_vfio);
>  int kvmppc_remove_spapr_tce(void *table, int pfd, uint32_t window_size);
>  int kvmppc_reset_htab(int shift_hint);
> -uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift);
> +uint64_t kvmppc_vrma_limit(unsigned int hash_shift);
>  bool kvmppc_has_cap_spapr_vfio(void);
>  #endif /* !CONFIG_USER_ONLY */
>  bool kvmppc_has_cap_epr(void);
> @@ -241,10 +241,9 @@ static inline int kvmppc_reset_htab(int shift_hint)
>  return 0;
>  }
>  
> -static inline uint64_t kvmppc_rma_size(uint64_t current_size,
> -   unsigned int hash_shift)
> +static inline uint64_t kvmppc_vrma_limit(unsigned int hash_shift)
>  {
> -return ram_size;
> +g_assert_not_reached();
>  }
>  
>  static inline bool kvmppc_hpt_needs_host_contiguous_pages(void)
> 

Re: [PATCH v37 05/17] target/avr: Add instruction translation - Arithmetic and Logic Instructions

[Michael Rolnik]

Aleksandar.

I could not find what happens if an instruction with unsupported registers
is executed. So, I am leaving this tiny core for later.

Regards,
Michael Rolnik

On Sun, Dec 1, 2019 at 1:11 AM Aleksandar Markovic <
aleksandar.m.m...@gmail.com> wrote:

>
>
> On Saturday, November 30, 2019, Michael Rolnik  wrote:
>
>> Hi Aleksandar.
>>
>> thanks for pointing that out I was not aware of that.
>> I can fix it.
>>
>>
> Hey, Michael,
>
> Please take alook at this:
>
> https://en.m.wikipedia.org/wiki/ATtiny_microcontroller_comparison_chart
>
> It looks that all cases of AVR 16-gpr-registers cores belong to the group 
> "avrtiny10",
> that actually you may want to add to your solution. Just a hint.
>
> Best regards,
> Aleksandar
>
>
>
> Regards,
>> Michael Rolnik
>>
>> On Sat, Nov 30, 2019 at 6:29 PM Aleksandar Markovic <
>> aleksandar.m.m...@gmail.com> wrote:
>>
>>>
>>>
>>> On Saturday, November 30, 2019, Aleksandar Markovic <
>>> aleksandar.m.m...@gmail.com> wrote:
>>>


 On Wednesday, November 27, 2019, Michael Rolnik 
 wrote:

 +
> +
> +/*
> + *  Performs the logical AND between the contents of register Rd and
> register
> + *  Rr and places the result in the destination register Rd.
> + */
> +static bool trans_AND(DisasContext *ctx, arg_AND *a)
> +{
> +TCGv Rd = cpu_r[a->rd];
> +TCGv Rr = cpu_r[a->rr];
> +TCGv R = tcg_temp_new_i32();
> +
> +/* op */
> +tcg_gen_and_tl(R, Rd, Rr); /* Rd = Rd and Rr */
> +
> +/* Vf */
> +tcg_gen_movi_tl(cpu_Vf, 0); /* Vf = 0 */
> +
> +/* Zf */
> +tcg_gen_setcondi_tl(TCG_COND_EQ, cpu_Zf, R, 0); /* Zf = R == 0 */
> +
> +gen_ZNSf(R);
> +
> +/* R */
> +tcg_gen_mov_tl(Rd, R);
> +
> +tcg_temp_free_i32(R);
> +
> +return true;
> +}
> +
> +
>

 According to specs:


 http://ww1.microchip.com/downloads/en/devicedoc/atmel-42505-8-bit-avr-microcontrollers-attiny102-attiny104_datasheet.pdf

 ... the chips in question have cores with 16 GPRs (that correspond to
 GPRs R16-R31 of more common AVR cores with 32 GPRs).


>>> There are more examples;
>>>
>>>
>>> http://ww1.microchip.com/downloads/en/DeviceDoc/atmel-8127-avr-8-bit-microcontroller-attiny4-attiny5-attiny9-attiny10_datasheet.pdf
>>>
>>> Also ATtiny20, ATtiny40.
>>>
>>> How do you handle such cores?

 I don't see here anything preventing usage of all 32 GPRs, resulting,
 of course, in an inaccurate emulation.

 Thanks,
 Aleksandar

>>>
>>
>> --
>> Best Regards,
>> Michael Rolnik
>>
>

-- 
Best Regards,
Michael Rolnik

Re: [PATCH 2/2] Add -mem-shared option

[Igor Mammedov]

On Fri, 29 Nov 2019 18:46:12 +0100
Paolo Bonzini  wrote:

> On 29/11/19 13:16, Igor Mammedov wrote:
> > As for "-m", I'd make it just an alias that translates
> >  -m/mem-path/mem-prealloc  
> 
> I think we should just deprecate -mem-path/-mem-prealloc in 5.0.  CCing
> Thomas as mister deprecation. :)

I'll add that to my series
 
[...]

Re: [PATCH v37 10/17] target/avr: Add instruction disassembly function

[Michael Rolnik]

Aleksandar.

If this code is going to be merge in 2019 I should modify al the
copyrights, right. or should I put 2020 in?

Regards,
Michael Rolnik

On Mon, Dec 2, 2019 at 2:28 AM Aleksandar Markovic <
aleksandar.m.m...@gmail.com> wrote:

>
>
> On Wednesday, November 27, 2019, Michael Rolnik  wrote:
>
>> Provide function disassembles executed instruction when `-d in_asm` is
>> provided
>>
>> Example:
>> `./avr-softmmu/qemu-system-avr -bios
>> free-rtos/Demo/AVR_ATMega2560_GCC/demo.elf -d in_asm` will produce
>> something like the following
>>
>> ```
>> ...
>> IN:
>> 0x014a:  CALL  0x3808
>>
>> IN: main
>> 0x3808:  CALL  0x4b4
>>
>> IN: vParTestInitialise
>> 0x04b4:  LDI   r24, 255
>> 0x04b6:  STS   r24, 0
>> 0x04b8:  MULS  r16, r20
>> 0x04ba:  OUT   $1, r24
>> 0x04bc:  LDS   r24, 0
>> 0x04be:  MULS  r16, r20
>> 0x04c0:  OUT   $2, r24
>> 0x04c2:  RET
>> ...
>> ```
>>
>> Signed-off-by: Michael Rolnik 
>> Suggested-by: Richard Henderson 
>> Suggested-by: Philippe Mathieu-Daudé 
>> Suggested-by: Aleksandar Markovic 
>> Reviewed-by: Philippe Mathieu-Daudé 
>> Tested-by: Philippe Mathieu-Daudé 
>> ---
>>  target/avr/cpu.h   |   1 +
>>  target/avr/cpu.c   |   2 +-
>>  target/avr/disas.c | 228 +
>>  target/avr/translate.c |  11 ++
>>  4 files changed, 241 insertions(+), 1 deletion(-)
>>  create mode 100644 target/avr/disas.c
>>
>> diff --git a/target/avr/cpu.h b/target/avr/cpu.h
>> index 9ea5260165..a3e615a1eb 100644
>> --- a/target/avr/cpu.h
>> +++ b/target/avr/cpu.h
>> @@ -157,6 +157,7 @@ bool avr_cpu_exec_interrupt(CPUState *cpu, int
>> int_req);
>>  hwaddr avr_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
>>  int avr_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
>>  int avr_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>> +int avr_print_insn(bfd_vma addr, disassemble_info *info);
>>
>>  static inline int avr_feature(CPUAVRState *env, int feature)
>>  {
>> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
>> index dae56d7845..52ec21dd16 100644
>> --- a/target/avr/cpu.c
>> +++ b/target/avr/cpu.c
>> @@ -83,7 +83,7 @@ static void avr_cpu_reset(CPUState *cs)
>>  static void avr_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
>>  {
>>  info->mach = bfd_arch_avr;
>> -info->print_insn = NULL;
>> +info->print_insn = avr_print_insn;
>>  }
>>
>>  static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
>> diff --git a/target/avr/disas.c b/target/avr/disas.c
>> new file mode 100644
>> index 00..a51ade7c2a
>> --- /dev/null
>> +++ b/target/avr/disas.c
>> @@ -0,0 +1,228 @@
>> +/*
>> + * AVR disassembler
>> + *
>> + * Copyright (c) 2018 Richard Henderson 
>
>
> Just a detail: since this file is created in 2019, the copyright year
> should be 2019 too.
>
> + * Copyright (c) 2019 Michael Rolnik 
>> + *
>> + * This program is free software: you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see .
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "disas/dis-asm.h"
>> +#include "qemu/bitops.h"
>> +#include "cpu.h"
>> +
>> +typedef struct {
>> +disassemble_info *info;
>> +uint16_t next_word;
>> +bool next_word_used;
>> +} DisasContext;
>> +
>> +static int to_regs_16_31_by_one(DisasContext *ctx, int indx)
>> +{
>> +return 16 + (indx % 16);
>> +}
>> +
>> +static int to_regs_16_23_by_one(DisasContext *ctx, int indx)
>> +{
>> +return 16 + (indx % 8);
>> +}
>> +static int to_regs_24_30_by_two(DisasContext *ctx, int indx)
>> +{
>> +return 24 + (indx % 4) * 2;
>> +}
>> +static int to_regs_00_30_by_two(DisasContext *ctx, int indx)
>> +{
>> +return (indx % 16) * 2;
>> +}
>> +
>> +static uint16_t next_word(DisasContext *ctx)
>> +{
>> +ctx->next_word_used = true;
>> +return ctx->next_word;
>> +}
>> +
>> +static int append_16(DisasContext *ctx, int x)
>> +{
>> +return x << 16 | next_word(ctx);
>> +}
>> +
>> +
>> +/* Include the auto-generated decoder.  */
>> +static bool decode_insn(DisasContext *ctx, uint16_t insn);
>> +#include "decode_insn.inc.c"
>> +
>> +#define output(mnemonic, format, ...) \
>> +(pctx->info->fprintf_func(pctx->info->stream, "%-9s " format, \
>> +mnemonic, ##__VA_ARGS__))
>> +
>> +int avr_print_insn(bfd_vma add

Re: [for-5.0 0/4] spapr: Improvements to CAS feature negotiation

[Cédric Le Goater]

On 29/11/2019 06:33, David Gibson wrote:
> This series contains several cleanups to the handling of the
> ibm,client-architecture-support firmware call used for boot time
> feature negotiation between the guest OS and the firmware &
> hypervisor.
> 
> Mostly it's just internal polish, but one significant user visible
> change is that we no longer generate an extra CAS reboot to switch
> between XICS and XIVE interrupt modes (by far the most common cause of
> CAS reboots in practice).


I love it. thanks for removing this extra reboot.

C. 


> 
> David Gibson (4):
>   spapr: Don't trigger a CAS reboot for XICS/XIVE mode changeover
>   spapr: Improve handling of fdt buffer size
>   spapr: Fold h_cas_compose_response() into
> h_client_architecture_support()
>   spapr: Simplify ovec diff
> 
>  hw/ppc/spapr.c  | 92 +++--
>  hw/ppc/spapr_hcall.c| 90 +---
>  hw/ppc/spapr_ovec.c | 30 
>  include/hw/ppc/spapr.h  |  4 +-
>  include/hw/ppc/spapr_ovec.h |  4 +-
>  5 files changed, 83 insertions(+), 137 deletions(-)
> 

Re: [for-5.0 3/4] spapr: Fold h_cas_compose_response() into h_client_architecture_support()

[Cédric Le Goater]

On 29/11/2019 06:33, David Gibson wrote:
> spapr_h_cas_compose_response() handles the last piece of the PAPR feature
> negotiation process invoked via the ibm,client-architecture-support OF
> call.  Its only caller is h_client_architecture_support() which handles
> most of the rest of that process.
> 
> I believe it was place in a separate file originally to handle some fiddly
> dependencies between functions, but mostly it's just confusing to have
> the CAS process split into two pieces like this.  Now that compose response
> is simplified (by just generating the whole device tree anew), it's cleaner
> to just fold it into h_client_architecture_support().
> 
> Signed-off-by: David Gibson 

Reviewed-by: Cedric Le Goater 

> ---
>  hw/ppc/spapr.c | 61 +-
>  hw/ppc/spapr_hcall.c   | 55 ++---
>  include/hw/ppc/spapr.h |  4 +--
>  3 files changed, 54 insertions(+), 66 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d34e317f48..5187f5b0a5 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -76,7 +76,6 @@
>  #include "hw/nmi.h"
>  #include "hw/intc/intc.h"
>  
> -#include "qemu/cutils.h"
>  #include "hw/ppc/spapr_cpu_core.h"
>  #include "hw/mem/memory-device.h"
>  #include "hw/ppc/spapr_tpm_proxy.h"
> @@ -897,63 +896,6 @@ out:
>  return ret;
>  }
>  
> -static bool spapr_hotplugged_dev_before_cas(void)
> -{
> -Object *drc_container, *obj;
> -ObjectProperty *prop;
> -ObjectPropertyIterator iter;
> -
> -drc_container = container_get(object_get_root(), "/dr-connector");
> -object_property_iter_init(&iter, drc_container);
> -while ((prop = object_property_iter_next(&iter))) {
> -if (!strstart(prop->type, "link<", NULL)) {
> -continue;
> -}
> -obj = object_property_get_link(drc_container, prop->name, NULL);
> -if (spapr_drc_needed(obj)) {
> -return true;
> -}
> -}
> -return false;
> -}
> -
> -static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
> - size_t space);
> -
> -int spapr_h_cas_compose_response(SpaprMachineState *spapr,
> - target_ulong addr, target_ulong size,
> - SpaprOptionVector *ov5_updates)
> -{
> -void *fdt;
> -SpaprDeviceTreeUpdateHeader hdr = { .version_id = 1 };
> -
> -if (spapr_hotplugged_dev_before_cas()) {
> -return 1;
> -}
> -
> -if (size < sizeof(hdr)) {
> -error_report("SLOF provided insufficient CAS buffer "
> - TARGET_FMT_lu " (min: %zu)", size, sizeof(hdr));
> -exit(EXIT_FAILURE);
> -}
> -
> -size -= sizeof(hdr);
> -
> -fdt = spapr_build_fdt(spapr, false, size);
> -_FDT((fdt_pack(fdt)));
> -
> -cpu_physical_memory_write(addr, &hdr, sizeof(hdr));
> -cpu_physical_memory_write(addr + sizeof(hdr), fdt, fdt_totalsize(fdt));
> -trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
> -
> -g_free(spapr->fdt_blob);
> -spapr->fdt_size = fdt_totalsize(fdt);
> -spapr->fdt_initial_size = spapr->fdt_size;
> -spapr->fdt_blob = fdt;
> -
> -return 0;
> -}
> -
>  static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
>  {
>  MachineState *ms = MACHINE(spapr);
> @@ -1191,8 +1133,7 @@ static void spapr_dt_hypervisor(SpaprMachineState 
> *spapr, void *fdt)
>  }
>  }
>  
> -static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
> - size_t space)
> +void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
>  {
>  MachineState *machine = MACHINE(spapr);
>  MachineClass *mc = MACHINE_GET_CLASS(machine);
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 05a7ca275b..0f19be794c 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1,4 +1,5 @@
>  #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>  #include "qapi/error.h"
>  #include "sysemu/hw_accel.h"
>  #include "sysemu/runstate.h"
> @@ -15,6 +16,7 @@
>  #include "cpu-models.h"
>  #include "trace.h"
>  #include "kvm_ppc.h"
> +#include "hw/ppc/fdt.h"
>  #include "hw/ppc/spapr_ovec.h"
>  #include "mmu-book3s-v3.h"
>  #include "hw/mem/memory-device.h"
> @@ -1638,6 +1640,26 @@ static uint32_t cas_check_pvr(SpaprMachineState 
> *spapr, PowerPCCPU *cpu,
>  return best_compat;
>  }
>  
> +static bool spapr_hotplugged_dev_before_cas(void)
> +{
> +Object *drc_container, *obj;
> +ObjectProperty *prop;
> +ObjectPropertyIterator iter;
> +
> +drc_container = container_get(object_get_root(), "/dr-connector");
> +object_property_iter_init(&iter, drc_container);
> +while ((prop = object_property_iter_next(&iter))) {
> +if (!strstart(prop->type, "link<", NULL)) {
> +continue;
> +}
> +obj = object_property_get_link(drc_container, prop->name, NULL);
> +if (spapr_drc_needed(o

Re: [for-5.0 2/4] spapr: Improve handling of fdt buffer size

[Cédric Le Goater]

On 29/11/2019 06:33, David Gibson wrote:
> Previously, spapr_build_fdt() constructed the device tree in a fixed
> buffer of size FDT_MAX_SIZE.  This is a bit inflexible, but more
> importantly it's awkward for the case where we use it during CAS.  In
> that case the guest firmware supplies a buffer and we have to
> awkwardly check that what we generated fits into it afterwards, after
> doing a lot of size checks during spapr_build_fdt().
> 
> Simplify this by having spapr_build_fdt() take a 'space' parameter.
> For the CAS case, we pass in the buffer size provided by SLOF, for the
> machine init case, we continue to pass FDT_MAX_SIZE.
> 
> Signed-off-by: David Gibson 

Reviewed-by: Cedric Le Goater 


> ---
>  hw/ppc/spapr.c | 33 +++--
>  1 file changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d9c9a2bcee..d34e317f48 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -917,7 +917,8 @@ static bool spapr_hotplugged_dev_before_cas(void)
>  return false;
>  }
>  
> -static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset);
> +static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
> + size_t space);
>  
>  int spapr_h_cas_compose_response(SpaprMachineState *spapr,
>   target_ulong addr, target_ulong size,
> @@ -930,24 +931,17 @@ int spapr_h_cas_compose_response(SpaprMachineState 
> *spapr,
>  return 1;
>  }
>  
> -if (size < sizeof(hdr) || size > FW_MAX_SIZE) {
> -error_report("SLOF provided an unexpected CAS buffer size "
> - TARGET_FMT_lu " (min: %zu, max: %u)",
> - size, sizeof(hdr), FW_MAX_SIZE);
> +if (size < sizeof(hdr)) {
> +error_report("SLOF provided insufficient CAS buffer "
> + TARGET_FMT_lu " (min: %zu)", size, sizeof(hdr));
>  exit(EXIT_FAILURE);
>  }
>  
>  size -= sizeof(hdr);
>  
> -fdt = spapr_build_fdt(spapr, false);
> +fdt = spapr_build_fdt(spapr, false, size);
>  _FDT((fdt_pack(fdt)));
>  
> -if (fdt_totalsize(fdt) + sizeof(hdr) > size) {
> -g_free(fdt);
> -trace_spapr_cas_failed(size);
> -return -1;
> -}
> -
>  cpu_physical_memory_write(addr, &hdr, sizeof(hdr));
>  cpu_physical_memory_write(addr + sizeof(hdr), fdt, fdt_totalsize(fdt));
>  trace_spapr_cas_continue(fdt_totalsize(fdt) + sizeof(hdr));
> @@ -1197,7 +1191,8 @@ static void spapr_dt_hypervisor(SpaprMachineState 
> *spapr, void *fdt)
>  }
>  }
>  
> -static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset)
> +static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset,
> + size_t space)
>  {
>  MachineState *machine = MACHINE(spapr);
>  MachineClass *mc = MACHINE_GET_CLASS(machine);
> @@ -1207,8 +1202,8 @@ static void *spapr_build_fdt(SpaprMachineState *spapr, 
> bool reset)
>  SpaprPhbState *phb;
>  char *buf;
>  
> -fdt = g_malloc0(FDT_MAX_SIZE);
> -_FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
> +fdt = g_malloc0(space);
> +_FDT((fdt_create_empty_tree(fdt, space)));
>  
>  /* Root node */
>  _FDT(fdt_setprop_string(fdt, 0, "device_type", "chrp"));
> @@ -1723,19 +1718,13 @@ static void spapr_machine_reset(MachineState *machine)
>   */
>  fdt_addr = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FDT_MAX_SIZE;
>  
> -fdt = spapr_build_fdt(spapr, true);
> +fdt = spapr_build_fdt(spapr, true, FDT_MAX_SIZE);
>  
>  rc = fdt_pack(fdt);
>  
>  /* Should only fail if we've built a corrupted tree */
>  assert(rc == 0);
>  
> -if (fdt_totalsize(fdt) > FDT_MAX_SIZE) {
> -error_report("FDT too big ! 0x%x bytes (max is 0x%x)",
> - fdt_totalsize(fdt), FDT_MAX_SIZE);
> -exit(1);
> -}
> -
>  /* Load the fdt */
>  qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
>  cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
> 

Re: [for-5.0 4/4] spapr: Simplify ovec diff

[Cédric Le Goater]

On 29/11/2019 06:33, David Gibson wrote:
> spapr_ovec_diff(ov, old, new) has somewhat complex semantics.  ov is set
> to those bits which are in new but not old, and it returns as a boolean
> whether or not there are any bits in old but not new.
> 
> It turns out that both callers only care about the second, not the first.
> This is basically equivalent to a bitmap subset operation, which is easier
> to understand and implement.  So replace spapr_ovec_diff() with
> spapr_ovec_subset().
> 
> Cc: Mike Roth 
> 
> Signed-off-by: David Gibson 


Reviewed-by: Cedric Le Goater 

> ---
>  hw/ppc/spapr.c  | 14 +++---
>  hw/ppc/spapr_hcall.c|  8 ++--
>  hw/ppc/spapr_ovec.c | 30 ++
>  include/hw/ppc/spapr_ovec.h |  4 +---
>  4 files changed, 16 insertions(+), 40 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5187f5b0a5..32e1cc1d3f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1840,8 +1840,6 @@ static bool spapr_ov5_cas_needed(void *opaque)
>  {
>  SpaprMachineState *spapr = opaque;
>  SpaprOptionVector *ov5_mask = spapr_ovec_new();
> -SpaprOptionVector *ov5_legacy = spapr_ovec_new();
> -SpaprOptionVector *ov5_removed = spapr_ovec_new();
>  bool cas_needed;
>  
>  /* Prior to the introduction of SpaprOptionVector, we had two option
> @@ -1873,17 +1871,11 @@ static bool spapr_ov5_cas_needed(void *opaque)
>  spapr_ovec_set(ov5_mask, OV5_DRCONF_MEMORY);
>  spapr_ovec_set(ov5_mask, OV5_DRMEM_V2);
>  
> -/* spapr_ovec_diff returns true if bits were removed. we avoid using
> - * the mask itself since in the future it's possible "legacy" bits may be
> - * removed via machine options, which could generate a false positive
> - * that breaks migration.
> - */
> -spapr_ovec_intersect(ov5_legacy, spapr->ov5, ov5_mask);
> -cas_needed = spapr_ovec_diff(ov5_removed, spapr->ov5, ov5_legacy);
> +/* We need extra information if we have any bits outside the mask
> + * defined above */
> +cas_needed = !spapr_ovec_subset(spapr->ov5, ov5_mask);
>  
>  spapr_ovec_cleanup(ov5_mask);
> -spapr_ovec_cleanup(ov5_legacy);
> -spapr_ovec_cleanup(ov5_removed);
>  
>  return cas_needed;
>  }
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 0f19be794c..f1799b1b70 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1671,7 +1671,7 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu,
>  target_ulong fdt_bufsize = args[2];
>  target_ulong ov_table;
>  uint32_t cas_pvr;
> -SpaprOptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> +SpaprOptionVector *ov1_guest, *ov5_guest, *ov5_cas_old;
>  bool guest_radix;
>  Error *local_err = NULL;
>  bool raw_mode_supported = false;
> @@ -1770,9 +1770,7 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu,
>  /* capabilities that have been added since CAS-generated guest reset.
>   * if capabilities have since been removed, generate another reset
>   */
> -ov5_updates = spapr_ovec_new();
> -spapr->cas_reboot = spapr_ovec_diff(ov5_updates,
> -ov5_cas_old, spapr->ov5_cas);
> +spapr->cas_reboot = !spapr_ovec_subset(ov5_cas_old, spapr->ov5_cas);
>  spapr_ovec_cleanup(ov5_cas_old);
>  /* Now that processing is finished, set the radix/hash bit for the
>   * guest if it requested a valid mode; otherwise terminate the boot. */
> @@ -1849,8 +1847,6 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu,
>  spapr->fdt_blob = fdt;
>  }
>  
> -spapr_ovec_cleanup(ov5_updates);
> -
>  if (spapr->cas_reboot) {
>  qemu_system_reset_request(SHUTDOWN_CAUSE_SUBSYSTEM_RESET);
>  }
> diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
> index 811fadf143..0ff6d1aeae 100644
> --- a/hw/ppc/spapr_ovec.c
> +++ b/hw/ppc/spapr_ovec.c
> @@ -76,31 +76,21 @@ void spapr_ovec_intersect(SpaprOptionVector *ov,
>  bitmap_and(ov->bitmap, ov1->bitmap, ov2->bitmap, OV_MAXBITS);
>  }
>  
> -/* returns true if options bits were removed, false otherwise */
> -bool spapr_ovec_diff(SpaprOptionVector *ov,
> - SpaprOptionVector *ov_old,
> - SpaprOptionVector *ov_new)
> +/* returns true if ov1 has a subset of bits in ov2 */
> +bool spapr_ovec_subset(SpaprOptionVector *ov1, SpaprOptionVector *ov2)
>  {
> -unsigned long *change_mask = bitmap_new(OV_MAXBITS);
> -unsigned long *removed_bits = bitmap_new(OV_MAXBITS);
> -bool bits_were_removed = false;
> +unsigned long *tmp = bitmap_new(OV_MAXBITS);
> +bool result;
>  
> -g_assert(ov);
> -g_assert(ov_old);
> -g_assert(ov_new);
> -
> -bitmap_xor(change_mask, ov_old->bitmap, ov_new->bitmap, OV_MAXBITS);
> -bitmap_and(ov->bitmap, ov_new->bitmap, change_mask, OV_MAXBITS);
> -bitmap_and(removed_bits, ov_ol

Re: [for-5.0 1/4] spapr: Don't trigger a CAS reboot for XICS/XIVE mode changeover

[Cédric Le Goater]

On 29/11/2019 06:33, David Gibson wrote:
> PAPR allows the interrupt controller used on a POWER9 machine (XICS or
> XIVE) to be selected by the guest operating system, by using the
> ibm,client-architecture-support (CAS) feature negotiation call.
> 
> Currently, if the guest selects an interrupt controller different from the
> one selected at initial boot, this causes the system to be reset with the
> new model and the boot starts again.  This means we run through the SLOF
> boot process twice, as well as any other bootloader (e.g. grub) in use
> before the OS calls CAS.  This can be confusing and/or inconvenient for
> users.
> 
> Thanks to two fairly recent changes, we no longer need this reboot.  1) we
> now completely regenerate the device tree when CAS is called (meaning we
> don't need special case updates for all the device tree changes caused by
> the interrupt controller mode change),  2) we now have explicit code paths
> to activate and deactivate the different interrupt controllers, rather than
> just implicitly calling those at machine reset time.
> 
> We can therefore eliminate the reboot for changing irq mode, simply by
> putting a called to spapr_irq_update_active_intc() before we call
> spapr_h_cas_compose_response() (which gives the updated device tree to the
> guest firmware and OS).
> 
> Signed-off-by: David Gibson 


Reviewed-by: Cedric Le Goater 

> ---
>  hw/ppc/spapr_hcall.c | 33 +
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 140f05c1c6..05a7ca275b 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1767,21 +1767,10 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu,
>  }
>  spapr->cas_pre_isa3_guest = !spapr_ovec_test(ov1_guest, OV1_PPC_3_00);
>  spapr_ovec_cleanup(ov1_guest);
> -if (!spapr->cas_reboot) {
> -/* If spapr_machine_reset() did not set up a HPT but one is necessary
> - * (because the guest isn't going to use radix) then set it up here. 
> */
> -if ((spapr->patb_entry & PATE1_GR) && !guest_radix) {
> -/* legacy hash or new hash: */
> -spapr_setup_hpt_and_vrma(spapr);
> -}
> -spapr->cas_reboot =
> -(spapr_h_cas_compose_response(spapr, args[1], args[2],
> -  ov5_updates) != 0);
> -}
>  
>  /*
> - * Ensure the guest asks for an interrupt mode we support; otherwise
> - * terminate the boot.
> + * Ensure the guest asks for an interrupt mode we support;
> + * otherwise terminate the boot.
>   */
>  if (guest_xive) {
>  if (!spapr->irq->xive) {
> @@ -1797,14 +1786,18 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu,
>  }
>  }
>  
> -/*
> - * Generate a machine reset when we have an update of the
> - * interrupt mode. Only required when the machine supports both
> - * modes.
> - */
> +spapr_irq_update_active_intc(spapr);
> +
>  if (!spapr->cas_reboot) {
> -spapr->cas_reboot = spapr_ovec_test(ov5_updates, OV5_XIVE_EXPLOIT)
> -&& spapr->irq->xics && spapr->irq->xive;
> +/* If spapr_machine_reset() did not set up a HPT but one is necessary
> + * (because the guest isn't going to use radix) then set it up here. 
> */
> +if ((spapr->patb_entry & PATE1_GR) && !guest_radix) {
> +/* legacy hash or new hash: */
> +spapr_setup_hpt_and_vrma(spapr);
> +}
> +spapr->cas_reboot =
> +(spapr_h_cas_compose_response(spapr, args[1], args[2],
> +  ov5_updates) != 0);
>  }
>  
>  spapr_ovec_cleanup(ov5_updates);
> 

Re: [PATCH] target/i386: Remove monitor from some CPU model

[Tao Xu]

I am so forry for sending this old version patch by mistake.

Please ignore this patch.

On 12/2/2019 2:28 PM, Xu, Tao3 wrote:

Add new version of Snowridge, Denverton, Opteron_G3, EPYC, and Dhyana
CPU model to remove MONITOR/MWAIT feature.

After QEMU/KVM use "-overcommit cpu-pm=on" to expose MONITOR/MWAIT
(commit id 6f131f13e68d648a8e4f083c667ab1acd88ce4cd), the MONITOR/MWAIT
feature in these CPU model is unused.

Signed-off-by: Tao Xu 
---
  target/i386/cpu.c | 58 +++
  1 file changed, 58 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index a624163ac2..7c5f1e8fe0 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2770,6 +2770,19 @@ static X86CPUDefinition builtin_x86_defs[] = {
  MSR_ARCH_CAP_RDCL_NO | MSR_ARCH_CAP_SKIP_L1DFL_VMENTRY,
  .xlevel = 0x8008,
  .model_id = "Intel Atom Processor (Denverton)",
+.versions = (X86CPUVersionDefinition[]) {
+{ .version = 1 },
+{
+.version = 2,
+.props = (PropValue[]) {
+{ "monitor", "off" },
+{ "model-id",
+  "Intel Atom Processor (Denverton, no MONITOR)" },
+{ /* end of list */ },
+},
+},
+{ /* end of list */ },
+},
  },
  {
  .name = "Snowridge",
@@ -2850,6 +2863,16 @@ static X86CPUDefinition builtin_x86_defs[] = {
  { /* end of list */ },
  },
  },
+{
+.version = 3,
+.props = (PropValue[]) {
+/* mpx was already removed by -v2 above */
+{ "monitor", "off" },
+{ "model-id",
+  "Intel Atom Processor (Snowridge, no MPX, no MONITOR)" },
+{ /* end of list */ },
+},
+},
  { /* end of list */ },
  },
  },
@@ -2961,6 +2984,19 @@ static X86CPUDefinition builtin_x86_defs[] = {
  CPUID_EXT3_ABM | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM,
  .xlevel = 0x8008,
  .model_id = "AMD Opteron 23xx (Gen 3 Class Opteron)",
+.versions = (X86CPUVersionDefinition[]) {
+{ .version = 1 },
+{
+.version = 2,
+.props = (PropValue[]) {
+{ "monitor", "off" },
+{ "model-id",
+  "AMD Opteron 23xx (Gen 3 Class Opteron, no MONITOR)" },
+{ /* end of list */ },
+},
+},
+{ /* end of list */ },
+},
  },
  {
  .name = "Opteron_G4",
@@ -3085,6 +3121,16 @@ static X86CPUDefinition builtin_x86_defs[] = {
  { /* end of list */ }
  }
  },
+{
+.version = 3,
+.props = (PropValue[]) {
+/* ibpb was already enabled by -v2 above */
+{ "monitor", "off" },
+{ "model-id",
+  "AMD EPYC Processor (with IBPB, no MONITOR)" },
+{ /* end of list */ },
+},
+},
  { /* end of list */ }
  }
  },
@@ -3137,6 +3183,18 @@ static X86CPUDefinition builtin_x86_defs[] = {
  .xlevel = 0x801E,
  .model_id = "Hygon Dhyana Processor",
  .cache_info = &epyc_cache_info,
+.versions = (X86CPUVersionDefinition[]) {
+{ .version = 1 },
+{
+.version = 2,
+.props = (PropValue[]) {
+{ "monitor", "off" },
+{ "model-id", "Hygon Dhyana Processor (no MONITOR)" },
+{ /* end of list */ },
+},
+},
+{ /* end of list */ },
+},
  },
  };
  

[PATCH RESEND 2/4] target/i386: Remove monitor from some CPU models

[Tao Xu]

Add new version of Snowridge, Denverton, Opteron_G3, EPYC, and Dhyana
CPU model to remove MONITOR/MWAIT feature.

After QEMU/KVM use "-overcommit cpu-pm=on" to expose MONITOR/MWAIT
(commit id 6f131f13e68d648a8e4f083c667ab1acd88ce4cd), the MONITOR/MWAIT
feature in these CPU model is unused.

Signed-off-by: Tao Xu 
---
 target/i386/cpu.c | 38 ++
 1 file changed, 38 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 06a3077f95..b09ac38409 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3621,6 +3621,14 @@ static X86CPUDefinition builtin_x86_defs[] = {
 { /* end of list */ },
 },
 },
+{
+.version = 3,
+.props = (PropValue[]) {
+/* mpx was already removed by -v2 above */
+{ "monitor", "off" },
+{ /* end of list */ },
+},
+},
 { /* end of list */ },
 },
 },
@@ -3732,6 +3740,17 @@ static X86CPUDefinition builtin_x86_defs[] = {
 CPUID_EXT3_ABM | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM,
 .xlevel = 0x8008,
 .model_id = "AMD Opteron 23xx (Gen 3 Class Opteron)",
+.versions = (X86CPUVersionDefinition[]) {
+{ .version = 1 },
+{
+.version = 2,
+.props = (PropValue[]) {
+{ "monitor", "off" },
+{ /* end of list */ },
+},
+},
+{ /* end of list */ },
+},
 },
 {
 .name = "Opteron_G4",
@@ -3856,6 +3875,14 @@ static X86CPUDefinition builtin_x86_defs[] = {
 { /* end of list */ }
 }
 },
+{
+.version = 3,
+.props = (PropValue[]) {
+/* ibpb was already enabled by -v2 above */
+{ "monitor", "off" },
+{ /* end of list */ },
+},
+},
 { /* end of list */ }
 }
 },
@@ -3908,6 +3935,17 @@ static X86CPUDefinition builtin_x86_defs[] = {
 .xlevel = 0x801E,
 .model_id = "Hygon Dhyana Processor",
 .cache_info = &epyc_cache_info,
+.versions = (X86CPUVersionDefinition[]) {
+{ .version = 1 },
+{
+.version = 2,
+.props = (PropValue[]) {
+{ "monitor", "off" },
+{ /* end of list */ },
+},
+},
+{ /* end of list */ },
+},
 },
 };
 
-- 
2.20.1

[PATCH RESEND 1/4] target/i386: Add Denverton-v2 (no MPX) CPU model

[Tao Xu]

Because MPX is being removed from the linux kernel, remove MPX feature
from Denverton.

Signed-off-by: Tao Xu 
---
 target/i386/cpu.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 69f518a21a..06a3077f95 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3482,6 +3482,18 @@ static X86CPUDefinition builtin_x86_defs[] = {
 .features[FEAT_VMX_VMFUNC] = MSR_VMX_VMFUNC_EPT_SWITCHING,
 .xlevel = 0x8008,
 .model_id = "Intel Atom Processor (Denverton)",
+.versions = (X86CPUVersionDefinition[]) {
+{ .version = 1 },
+{
+.version = 2,
+.props = (PropValue[]) {
+{ "monitor", "off" },
+{ "mpx", "off" },
+{ /* end of list */ },
+},
+},
+{ /* end of list */ },
+},
 },
 {
 .name = "Snowridge",
-- 
2.20.1

[PATCH RESEND 0/4] Add extra information to versioned CPU models

[Tao Xu]

This series of patches will remove MPX from Denverton, remove Remove
monitor from some CPU models. Add additional information for -cpu help
to indicate the changes in this version of CPU model.

The output is as follows:
x86_64-softmmu/qemu-system-x86_64 -cpu help | grep "\["
x86 Broadwell-v2  Intel Core Processor (Broadwell) [no TSX] 

x86 Broadwell-v3  Intel Core Processor (Broadwell) [IBRS]   

x86 Broadwell-v4  Intel Core Processor (Broadwell) [no TSX, IBRS]   

x86 Cascadelake-Server-v2  Intel Xeon Processor (Cascadelake) 
[ARCH_CAPABILITIES]
x86 Cascadelake-Server-v3  Intel Xeon Processor (Cascadelake) [no TSX]  
 
x86 Denverton-v2  Intel Atom Processor (Denverton) [no MPX, no MONITOR] 

x86 Dhyana-v2 Hygon Dhyana Processor [no MONITOR]   

x86 EPYC-v2   AMD EPYC Processor [IBPB] 

x86 EPYC-v3   AMD EPYC Processor [IBPB, no MONITOR] 

x86 Haswell-v2Intel Core Processor (Haswell) [no TSX]   

x86 Haswell-v3Intel Core Processor (Haswell) [IBRS] 

x86 Haswell-v4Intel Core Processor (Haswell) [no TSX, IBRS] 

x86 Icelake-Client-v2 Intel Core Processor (Icelake) [no TSX]   

x86 Icelake-Server-v2 Intel Xeon Processor (Icelake) [no TSX]   

x86 IvyBridge-v2  Intel Xeon E3-12xx v2 (Ivy Bridge) [IBRS] 

x86 Nehalem-v2Intel Core i7 9xx (Nehalem Class Core i7) [IBRS]  

x86 Opteron_G3-v2 AMD Opteron 23xx (Gen 3 Class Opteron) [no MONITOR]   

x86 SandyBridge-v2Intel Xeon E312xx (Sandy Bridge) [IBRS]   

x86 Skylake-Client-v2 Intel Core Processor (Skylake) [IBRS] 

x86 Skylake-Client-v3 Intel Core Processor (Skylake) [no TSX, IBRS] 

x86 Skylake-Server-v2 Intel Xeon Processor (Skylake) [IBRS] 

x86 Skylake-Server-v3 Intel Xeon Processor (Skylake) [no TSX, IBRS] 

x86 Snowridge-v2  Intel Atom Processor (SnowRidge) [no MPX] 

x86 Snowridge-v3  Intel Atom Processor (SnowRidge) [no MPX, no MONITOR] 

x86 Westmere-v2   Westmere E56xx/L56xx/X56xx (Nehalem-C) [IBRS]

Tao Xu (4):
  target/i386: Add Denverton-v2 (no MPX) CPU model
  target/i386: Remove monitor from some CPU models
  target/i386: Add new property note to versioned CPU models
  target/i386: Add notes for versioned CPU models

 target/i386/cpu.c | 112 +++---
 1 file changed, 85 insertions(+), 27 deletions(-)

-- 
2.20.1

[PATCH RESEND 3/4] target/i386: Add new property note to versioned CPU models

[Tao Xu]

Add additional information for -cpu help to indicate the changes in this
version of CPU model.

Suggested-by: Eduardo Habkost 
Signed-off-by: Tao Xu 
---
 target/i386/cpu.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b09ac38409..7b3bd6d4db 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1693,6 +1693,7 @@ typedef struct PropValue {
 typedef struct X86CPUVersionDefinition {
 X86CPUVersion version;
 const char *alias;
+const char *note;
 PropValue *props;
 } X86CPUVersionDefinition;
 
@@ -1723,6 +1724,7 @@ struct X86CPUModel {
 X86CPUDefinition *cpudef;
 /* CPU model version */
 X86CPUVersion version;
+const char *note;
 /*
  * If true, this is an alias CPU model.
  * This matters only for "-cpu help" and query-cpu-definitions
@@ -4788,6 +4790,7 @@ static void x86_cpu_list_entry(gpointer data, gpointer 
user_data)
 char *name = x86_cpu_class_get_model_name(cc);
 char *desc = g_strdup(cc->model_description);
 char *alias_of = x86_cpu_class_get_alias_of(cc);
+char *model_id = x86_cpu_class_get_model_id(cc);
 
 if (!desc && alias_of) {
 if (cc->model && cc->model->version == CPU_VERSION_AUTO) {
@@ -4796,14 +4799,18 @@ static void x86_cpu_list_entry(gpointer data, gpointer 
user_data)
 desc = g_strdup_printf("(alias of %s)", alias_of);
 }
 }
+if (!desc && cc->model && cc->model->note) {
+desc = g_strdup_printf("%s [%s]", model_id, cc->model->note);
+}
 if (!desc) {
-desc = x86_cpu_class_get_model_id(cc);
+desc = g_strdup_printf("%s", model_id);
 }
 
-qemu_printf("x86 %-20s  %-48s\n", name, desc);
+qemu_printf("x86 %-20s  %-58s\n", name, desc);
 g_free(name);
 g_free(desc);
 g_free(alias_of);
+g_free(model_id);
 }
 
 /* list available CPU models and flags */
@@ -5280,6 +5287,7 @@ static void x86_register_cpudef_types(X86CPUDefinition 
*def)
 X86CPUModel *m = g_new0(X86CPUModel, 1);
 m->cpudef = def;
 m->version = vdef->version;
+m->note = vdef->note;
 name = x86_cpu_versioned_model_name(def, vdef->version);
 x86_register_cpu_model_type(name, m);
 g_free(name);
-- 
2.20.1

[PATCH RESEND 4/4] target/i386: Add notes for versioned CPU models

[Tao Xu]

Add which features are added or removed in this version. Remove the
changed model-id in versioned CPU models.

Signed-off-by: Tao Xu 
---
 target/i386/cpu.c | 50 +++
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 7b3bd6d4db..c82fbfd02e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2281,10 +2281,9 @@ static X86CPUDefinition builtin_x86_defs[] = {
 {
 .version = 2,
 .alias = "Nehalem-IBRS",
+.note = "IBRS",
 .props = (PropValue[]) {
 { "spec-ctrl", "on" },
-{ "model-id",
-  "Intel Core i7 9xx (Nehalem Core i7, IBRS update)" },
 { /* end of list */ }
 }
 },
@@ -2362,10 +2361,9 @@ static X86CPUDefinition builtin_x86_defs[] = {
 {
 .version = 2,
 .alias = "Westmere-IBRS",
+.note = "IBRS",
 .props = (PropValue[]) {
 { "spec-ctrl", "on" },
-{ "model-id",
-  "Westmere E56xx/L56xx/X56xx (IBRS update)" },
 { /* end of list */ }
 }
 },
@@ -2448,10 +2446,9 @@ static X86CPUDefinition builtin_x86_defs[] = {
 {
 .version = 2,
 .alias = "SandyBridge-IBRS",
+.note = "IBRS",
 .props = (PropValue[]) {
 { "spec-ctrl", "on" },
-{ "model-id",
-  "Intel Xeon E312xx (Sandy Bridge, IBRS update)" },
 { /* end of list */ }
 }
 },
@@ -2540,10 +2537,9 @@ static X86CPUDefinition builtin_x86_defs[] = {
 {
 .version = 2,
 .alias = "IvyBridge-IBRS",
+.note = "IBRS",
 .props = (PropValue[]) {
 { "spec-ctrl", "on" },
-{ "model-id",
-  "Intel Xeon E3-12xx v2 (Ivy Bridge, IBRS)" },
 { /* end of list */ }
 }
 },
@@ -2637,17 +2633,18 @@ static X86CPUDefinition builtin_x86_defs[] = {
 {
 .version = 2,
 .alias = "Haswell-noTSX",
+.note = "no TSX",
 .props = (PropValue[]) {
 { "hle", "off" },
 { "rtm", "off" },
 { "stepping", "1" },
-{ "model-id", "Intel Core Processor (Haswell, no TSX)", },
 { /* end of list */ }
 },
 },
 {
 .version = 3,
 .alias = "Haswell-IBRS",
+.note = "IBRS",
 .props = (PropValue[]) {
 /* Restore TSX features removed by -v2 above */
 { "hle", "on" },
@@ -2658,21 +2655,18 @@ static X86CPUDefinition builtin_x86_defs[] = {
  */
 { "stepping", "4" },
 { "spec-ctrl", "on" },
-{ "model-id",
-  "Intel Core Processor (Haswell, IBRS)" },
 { /* end of list */ }
 }
 },
 {
 .version = 4,
 .alias = "Haswell-noTSX-IBRS",
+.note = "no TSX, IBRS",
 .props = (PropValue[]) {
 { "hle", "off" },
 { "rtm", "off" },
 /* spec-ctrl was already enabled by -v3 above */
 { "stepping", "1" },
-{ "model-id",
-  "Intel Core Processor (Haswell, no TSX, IBRS)" },
 { /* end of list */ }
 }
 },
@@ -2768,35 +2762,33 @@ static X86CPUDefinition builtin_x86_defs[] = {
 {
 .version = 2,
 .alias = "Broadwell-noTSX",
+.note = "no TSX",
 .props = (PropValue[]) {
 { "hle", "off" },
 { "rtm", "off" },
-{ "model-id", "Intel Core Processor (Broadwell, no TSX)", 
},
 { /* end of list */ }
 },
 },
 {
 .version = 3,
 .alias = "Broadwell-IBRS",
+.note = "IBRS",
 .props = (PropValue[]) {
 /* Restore TSX features removed by -v2 above */
 { "hle", "on" },
 { "rtm", "on" },
 { "spec-ctrl", "on" },
-{ "model-id",
-  "Intel Core Processor (Broadwell, IBRS)" },
 { /* end of list */ }
 }
 },
 {
 .v

Re: [PATCH v3 4/4] qom/object: Use common get/set uint helpers

[Alexey Kardashevskiy]

On 30/11/2019 04:46, Felipe Franciosi wrote:
> Several objects implemented their own uint property getters and setters,
> despite them being straightforward (without any checks/validations on
> the values themselves) and identical across objects. This makes use of
> an enhanced API for object_property_add_uintXX_ptr() which offers
> default setters.
> 
> Some of these setters used to update the value even if the type visit
> failed (eg. because the value being set overflowed over the given type).
> The new setter introduces a check for these errors, not updating the
> value if an error occurred. The error is propagated.
> 
> Signed-off-by: Felipe Franciosi 
> ---
>  hw/acpi/ich9.c   |  95 --
>  hw/isa/lpc_ich9.c|  12 +
>  hw/misc/edu.c|  13 ++
>  hw/pci-host/q35.c|  14 ++
>  hw/ppc/spapr.c   |  18 ++--
>  hw/vfio/pci-quirks.c |  20 +++-
>  memory.c |  15 +-
>  target/arm/cpu.c |  22 ++---
>  target/i386/sev.c| 106 ---
>  9 files changed, 40 insertions(+), 275 deletions(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 742fb78226..d9305be891 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -357,81 +357,6 @@ static void ich9_pm_set_cpu_hotplug_legacy(Object *obj, 
> bool value,
>  s->pm.cpu_hotplug_legacy = value;
>  }
>  
> -static void ich9_pm_get_disable_s3(Object *obj, Visitor *v, const char *name,
> -   void *opaque, Error **errp)
> -{
> -ICH9LPCPMRegs *pm = opaque;
> -uint8_t value = pm->disable_s3;
> -
> -visit_type_uint8(v, name, &value, errp);
> -}
> -
> -static void ich9_pm_set_disable_s3(Object *obj, Visitor *v, const char *name,
> -   void *opaque, Error **errp)
> -{
> -ICH9LPCPMRegs *pm = opaque;
> -Error *local_err = NULL;
> -uint8_t value;
> -
> -visit_type_uint8(v, name, &value, &local_err);
> -if (local_err) {
> -goto out;
> -}
> -pm->disable_s3 = value;
> -out:
> -error_propagate(errp, local_err);
> -}
> -
> -static void ich9_pm_get_disable_s4(Object *obj, Visitor *v, const char *name,
> -   void *opaque, Error **errp)
> -{
> -ICH9LPCPMRegs *pm = opaque;
> -uint8_t value = pm->disable_s4;
> -
> -visit_type_uint8(v, name, &value, errp);
> -}
> -
> -static void ich9_pm_set_disable_s4(Object *obj, Visitor *v, const char *name,
> -   void *opaque, Error **errp)
> -{
> -ICH9LPCPMRegs *pm = opaque;
> -Error *local_err = NULL;
> -uint8_t value;
> -
> -visit_type_uint8(v, name, &value, &local_err);
> -if (local_err) {
> -goto out;
> -}
> -pm->disable_s4 = value;
> -out:
> -error_propagate(errp, local_err);
> -}
> -
> -static void ich9_pm_get_s4_val(Object *obj, Visitor *v, const char *name,
> -   void *opaque, Error **errp)
> -{
> -ICH9LPCPMRegs *pm = opaque;
> -uint8_t value = pm->s4_val;
> -
> -visit_type_uint8(v, name, &value, errp);
> -}
> -
> -static void ich9_pm_set_s4_val(Object *obj, Visitor *v, const char *name,
> -   void *opaque, Error **errp)
> -{
> -ICH9LPCPMRegs *pm = opaque;
> -Error *local_err = NULL;
> -uint8_t value;
> -
> -visit_type_uint8(v, name, &value, &local_err);
> -if (local_err) {
> -goto out;
> -}
> -pm->s4_val = value;
> -out:
> -error_propagate(errp, local_err);
> -}
> -
>  static bool ich9_pm_get_enable_tco(Object *obj, Error **errp)
>  {
>  ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> @@ -468,18 +393,14 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs 
> *pm, Error **errp)
>   ich9_pm_get_cpu_hotplug_legacy,
>   ich9_pm_set_cpu_hotplug_legacy,
>   NULL);
> -object_property_add(obj, ACPI_PM_PROP_S3_DISABLED, "uint8",
> -ich9_pm_get_disable_s3,
> -ich9_pm_set_disable_s3,
> -NULL, pm, NULL);
> -object_property_add(obj, ACPI_PM_PROP_S4_DISABLED, "uint8",
> -ich9_pm_get_disable_s4,
> -ich9_pm_set_disable_s4,
> -NULL, pm, NULL);
> -object_property_add(obj, ACPI_PM_PROP_S4_VAL, "uint8",
> -ich9_pm_get_s4_val,
> -ich9_pm_set_s4_val,
> -NULL, pm, NULL);
> +object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S3_DISABLED,
> +  &pm->disable_s3, OBJ_PROP_FLAG_READWRITE,
> +  NULL);
> +object_property_add_uint8_ptr(obj, ACPI_PM_PROP_S4_DISABLED,
> +  &pm->disable_s4, OBJ_PROP_FLAG_READWRITE,
> +  NULL);
> +object_property_add_

[PATCH 4/4] target/i386: Add notes for versioned CPU models

[Tao Xu]

Add which features are added or removed in this version. Remove the
changed model-id in versioned CPU models.

Signed-off-by: Tao Xu 
---
 target/i386/cpu.c | 50 +++
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 7b3bd6d4db..c82fbfd02e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2281,10 +2281,9 @@ static X86CPUDefinition builtin_x86_defs[] = {
 {
 .version = 2,
 .alias = "Nehalem-IBRS",
+.note = "IBRS",
 .props = (PropValue[]) {
 { "spec-ctrl", "on" },
-{ "model-id",
-  "Intel Core i7 9xx (Nehalem Core i7, IBRS update)" },
 { /* end of list */ }
 }
 },
@@ -2362,10 +2361,9 @@ static X86CPUDefinition builtin_x86_defs[] = {
 {
 .version = 2,
 .alias = "Westmere-IBRS",
+.note = "IBRS",
 .props = (PropValue[]) {
 { "spec-ctrl", "on" },
-{ "model-id",
-  "Westmere E56xx/L56xx/X56xx (IBRS update)" },
 { /* end of list */ }
 }
 },
@@ -2448,10 +2446,9 @@ static X86CPUDefinition builtin_x86_defs[] = {
 {
 .version = 2,
 .alias = "SandyBridge-IBRS",
+.note = "IBRS",
 .props = (PropValue[]) {
 { "spec-ctrl", "on" },
-{ "model-id",
-  "Intel Xeon E312xx (Sandy Bridge, IBRS update)" },
 { /* end of list */ }
 }
 },
@@ -2540,10 +2537,9 @@ static X86CPUDefinition builtin_x86_defs[] = {
 {
 .version = 2,
 .alias = "IvyBridge-IBRS",
+.note = "IBRS",
 .props = (PropValue[]) {
 { "spec-ctrl", "on" },
-{ "model-id",
-  "Intel Xeon E3-12xx v2 (Ivy Bridge, IBRS)" },
 { /* end of list */ }
 }
 },
@@ -2637,17 +2633,18 @@ static X86CPUDefinition builtin_x86_defs[] = {
 {
 .version = 2,
 .alias = "Haswell-noTSX",
+.note = "no TSX",
 .props = (PropValue[]) {
 { "hle", "off" },
 { "rtm", "off" },
 { "stepping", "1" },
-{ "model-id", "Intel Core Processor (Haswell, no TSX)", },
 { /* end of list */ }
 },
 },
 {
 .version = 3,
 .alias = "Haswell-IBRS",
+.note = "IBRS",
 .props = (PropValue[]) {
 /* Restore TSX features removed by -v2 above */
 { "hle", "on" },
@@ -2658,21 +2655,18 @@ static X86CPUDefinition builtin_x86_defs[] = {
  */
 { "stepping", "4" },
 { "spec-ctrl", "on" },
-{ "model-id",
-  "Intel Core Processor (Haswell, IBRS)" },
 { /* end of list */ }
 }
 },
 {
 .version = 4,
 .alias = "Haswell-noTSX-IBRS",
+.note = "no TSX, IBRS",
 .props = (PropValue[]) {
 { "hle", "off" },
 { "rtm", "off" },
 /* spec-ctrl was already enabled by -v3 above */
 { "stepping", "1" },
-{ "model-id",
-  "Intel Core Processor (Haswell, no TSX, IBRS)" },
 { /* end of list */ }
 }
 },
@@ -2768,35 +2762,33 @@ static X86CPUDefinition builtin_x86_defs[] = {
 {
 .version = 2,
 .alias = "Broadwell-noTSX",
+.note = "no TSX",
 .props = (PropValue[]) {
 { "hle", "off" },
 { "rtm", "off" },
-{ "model-id", "Intel Core Processor (Broadwell, no TSX)", 
},
 { /* end of list */ }
 },
 },
 {
 .version = 3,
 .alias = "Broadwell-IBRS",
+.note = "IBRS",
 .props = (PropValue[]) {
 /* Restore TSX features removed by -v2 above */
 { "hle", "on" },
 { "rtm", "on" },
 { "spec-ctrl", "on" },
-{ "model-id",
-  "Intel Core Processor (Broadwell, IBRS)" },
 { /* end of list */ }
 }
 },
 {
 .v

[PATCH 2/4] target/i386: Remove monitor from some CPU models

[Tao Xu]

Add new version of Snowridge, Denverton, Opteron_G3, EPYC, and Dhyana
CPU model to remove MONITOR/MWAIT feature.

After QEMU/KVM use "-overcommit cpu-pm=on" to expose MONITOR/MWAIT
(commit id 6f131f13e68d648a8e4f083c667ab1acd88ce4cd), the MONITOR/MWAIT
feature in these CPU model is unused.

Signed-off-by: Tao Xu 
---
 target/i386/cpu.c | 38 ++
 1 file changed, 38 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 06a3077f95..b09ac38409 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3621,6 +3621,14 @@ static X86CPUDefinition builtin_x86_defs[] = {
 { /* end of list */ },
 },
 },
+{
+.version = 3,
+.props = (PropValue[]) {
+/* mpx was already removed by -v2 above */
+{ "monitor", "off" },
+{ /* end of list */ },
+},
+},
 { /* end of list */ },
 },
 },
@@ -3732,6 +3740,17 @@ static X86CPUDefinition builtin_x86_defs[] = {
 CPUID_EXT3_ABM | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM,
 .xlevel = 0x8008,
 .model_id = "AMD Opteron 23xx (Gen 3 Class Opteron)",
+.versions = (X86CPUVersionDefinition[]) {
+{ .version = 1 },
+{
+.version = 2,
+.props = (PropValue[]) {
+{ "monitor", "off" },
+{ /* end of list */ },
+},
+},
+{ /* end of list */ },
+},
 },
 {
 .name = "Opteron_G4",
@@ -3856,6 +3875,14 @@ static X86CPUDefinition builtin_x86_defs[] = {
 { /* end of list */ }
 }
 },
+{
+.version = 3,
+.props = (PropValue[]) {
+/* ibpb was already enabled by -v2 above */
+{ "monitor", "off" },
+{ /* end of list */ },
+},
+},
 { /* end of list */ }
 }
 },
@@ -3908,6 +3935,17 @@ static X86CPUDefinition builtin_x86_defs[] = {
 .xlevel = 0x801E,
 .model_id = "Hygon Dhyana Processor",
 .cache_info = &epyc_cache_info,
+.versions = (X86CPUVersionDefinition[]) {
+{ .version = 1 },
+{
+.version = 2,
+.props = (PropValue[]) {
+{ "monitor", "off" },
+{ /* end of list */ },
+},
+},
+{ /* end of list */ },
+},
 },
 };
 
-- 
2.20.1

[PATCH 1/4] target/i386: Add Denverton-v2 (no MPX) CPU model

[Tao Xu]

Because MPX is being removed from the linux kernel, remove MPX feature
from Denverton.

Signed-off-by: Tao Xu 
---
 target/i386/cpu.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 69f518a21a..06a3077f95 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3482,6 +3482,18 @@ static X86CPUDefinition builtin_x86_defs[] = {
 .features[FEAT_VMX_VMFUNC] = MSR_VMX_VMFUNC_EPT_SWITCHING,
 .xlevel = 0x8008,
 .model_id = "Intel Atom Processor (Denverton)",
+.versions = (X86CPUVersionDefinition[]) {
+{ .version = 1 },
+{
+.version = 2,
+.props = (PropValue[]) {
+{ "monitor", "off" },
+{ "mpx", "off" },
+{ /* end of list */ },
+},
+},
+{ /* end of list */ },
+},
 },
 {
 .name = "Snowridge",
-- 
2.20.1

[PATCH 3/4] target/i386: Add new property note to versioned CPU models

[Tao Xu]

Add additional information for -cpu help to indicate the changes in this
version of CPU model.

Suggested-by: Eduardo Habkost 
Signed-off-by: Tao Xu 
---
 target/i386/cpu.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b09ac38409..7b3bd6d4db 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1693,6 +1693,7 @@ typedef struct PropValue {
 typedef struct X86CPUVersionDefinition {
 X86CPUVersion version;
 const char *alias;
+const char *note;
 PropValue *props;
 } X86CPUVersionDefinition;
 
@@ -1723,6 +1724,7 @@ struct X86CPUModel {
 X86CPUDefinition *cpudef;
 /* CPU model version */
 X86CPUVersion version;
+const char *note;
 /*
  * If true, this is an alias CPU model.
  * This matters only for "-cpu help" and query-cpu-definitions
@@ -4788,6 +4790,7 @@ static void x86_cpu_list_entry(gpointer data, gpointer 
user_data)
 char *name = x86_cpu_class_get_model_name(cc);
 char *desc = g_strdup(cc->model_description);
 char *alias_of = x86_cpu_class_get_alias_of(cc);
+char *model_id = x86_cpu_class_get_model_id(cc);
 
 if (!desc && alias_of) {
 if (cc->model && cc->model->version == CPU_VERSION_AUTO) {
@@ -4796,14 +4799,18 @@ static void x86_cpu_list_entry(gpointer data, gpointer 
user_data)
 desc = g_strdup_printf("(alias of %s)", alias_of);
 }
 }
+if (!desc && cc->model && cc->model->note) {
+desc = g_strdup_printf("%s [%s]", model_id, cc->model->note);
+}
 if (!desc) {
-desc = x86_cpu_class_get_model_id(cc);
+desc = g_strdup_printf("%s", model_id);
 }
 
-qemu_printf("x86 %-20s  %-48s\n", name, desc);
+qemu_printf("x86 %-20s  %-58s\n", name, desc);
 g_free(name);
 g_free(desc);
 g_free(alias_of);
+g_free(model_id);
 }
 
 /* list available CPU models and flags */
@@ -5280,6 +5287,7 @@ static void x86_register_cpudef_types(X86CPUDefinition 
*def)
 X86CPUModel *m = g_new0(X86CPUModel, 1);
 m->cpudef = def;
 m->version = vdef->version;
+m->note = vdef->note;
 name = x86_cpu_versioned_model_name(def, vdef->version);
 x86_register_cpu_model_type(name, m);
 g_free(name);
-- 
2.20.1

Re: [RFC][PATCH 0/3] IVSHMEM version 2 device for QEMU

[Jan Kiszka]

On 27.11.19 18:19, Jan Kiszka wrote:

Hi Liang,

On 27.11.19 16:28, Liang Yan wrote:



On 11/11/19 7:57 AM, Jan Kiszka wrote:

To get the ball rolling after my presentation of the topic at KVM Forum
[1] and many fruitful discussions around it, this is a first concrete
code series. As discussed, I'm starting with the IVSHMEM implementation
of a QEMU device and server. It's RFC because, besides specification and
implementation details, there will still be some decisions needed about
how to integrate the new version best into the existing code bases.

If you want to play with this, the basic setup of the shared memory
device is described in patch 1 and 3. UIO driver and also the
virtio-ivshmem prototype can be found at

 
http://git.kiszka.org/?p=linux.git;a=shortlog;h=refs/heads/queues/ivshmem2 



Accessing the device via UIO is trivial enough. If you want to use it
for virtio, this is additionally to the description in patch 3 needed on
the virtio console backend side:

 modprobe uio_ivshmem
 echo "1af4 1110 1af4 1100 ffc003 ff" > 
/sys/bus/pci/drivers/uio_ivshmem/new_id

 linux/tools/virtio/virtio-ivshmem-console /dev/uio0

And for virtio block:

 echo "1af4 1110 1af4 1100 ffc002 ff" > 
/sys/bus/pci/drivers/uio_ivshmem/new_id
 linux/tools/virtio/virtio-ivshmem-console /dev/uio0 
/path/to/disk.img


After that, you can start the QEMU frontend instance with the
virtio-ivshmem driver installed which can use the new /dev/hvc* or
/dev/vda* as usual.

Any feedback welcome!


Hi, Jan,

I have been playing your code for last few weeks, mostly study and test,
of course. Really nice work. I have a few questions here:

First, qemu part looks good, I tried test between couple VMs, and device
could pop up correctly for all of them, but I had some problems when
trying load driver. For example, if set up two VMs, vm1 and vm2, start
ivshmem server as you suggested. vm1 could load uio_ivshmem and
virtio_ivshmem correctly, vm2 could load uio_ivshmem but could not show
up "/dev/uio0", virtio_ivshmem could not be loaded at all, these still
exist even I switch the load sequence of vm1 and vm2, and sometimes
reset "virtio_ivshmem" could crash both vm1 and vm2. Not quite sure this
is bug or "Ivshmem Mode" issue, but I went through ivshmem-server code,
did not related information.


If we are only talking about one ivshmem link and vm1 is the master, 
there is not role for virtio_ivshmem on it as backend. That is purely a 
frontend driver. Vice versa for vm2: If you want to use its ivshmem 
instance as virtio frontend, uio_ivshmem plays no role.


The "crash" is would be interesting to understand: Do you see kernel 
panics of the guests? Or are they stuck? Or are the QEMU instances 
stuck? Do you know that you can debug the guest kernels via gdb (and 
gdb-scripts of the kernel)?




I started some code work recently, such as fix code style issues and
some work based on above testing, however I know you are also working on
RFC V2, beside the protocol between server-client and client-client is
not finalized yet either, things may change, so much appreciate if you
could squeeze me into your develop schedule and share with me some
plans, :-)  Maybe I could send some pull request in your github repo?


I'm currently working on a refresh of the Jailhouse queue and the kernel 
patches to incorporate just two smaller changes:


  - use Siemens device ID
  - drop "features" register from ivshmem device

I have not yet touched the QEMU code for that so far, thus no conflict 
yet. I would wait for your patches then.


If it helps us to work on this together, I can push things to github as 
well. Will drop you a note when done. We should just present the outcome 
frequently as new series to the list.


I've updated my queues, mostly small changes, mostly to the kernel bits. 
Besides the already announced places, you can also find them as PR 
targets on


https://github.com/siemens/qemu/commits/wip/ivshmem2
https://github.com/siemens/linux/commits/queues/ivshmem2

To give the whole thing broader coverage, I will now also move forward 
and integrate the current state into Jailhouse - at the risk of having 
to rework the interface there once again. But there are a number of 
users already requiring the extended features (or even using them), plus 
this gives a nice test coverage of key components and properties.


Jan

--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

[PATCHv3] exynos4210_gic: Suppress gcc9 format-truncation warnings

[David Gibson]

exynos4210_gic_realize() prints the number of cpus into some temporary
buffers, but it only allows 3 bytes space for it.  That's plenty:
existing machines will only ever set this value to EXYNOS4210_NCPUS
(2).  But the compiler can't always figure that out, so some[*] gcc9
versions emit -Wformat-truncation warnings.

We can fix that by hinting the constraint to the compiler with a
suitably placed assert().

[*] The bizarre thing here, is that I've long gotten these warnings
compiling in a 32-bit x86 container as host - Fedora 30 with
gcc-9.2.1-1.fc30.i686 - but it compiles just fine on my normal
x86_64 host - Fedora 30 with and gcc-9.2.1-1.fc30.x86_64.

Signed-off-by: David Gibson 
Reviewed-by: Philippe Mathieu-Daudé 

Peter, up to you if you squeeze this in for qemu-4.2 or leave it until 5.0

Changes since v2:
 * Moved the assert outside the for loop using a trick suggested by
   Richard Henderson
Changes since v1:
 * Used an assert to hint the compiler, instead of increasing the
   buffer size.

---
 hw/intc/exynos4210_gic.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/intc/exynos4210_gic.c b/hw/intc/exynos4210_gic.c
index a1b699b6ba..ddd006aca6 100644
--- a/hw/intc/exynos4210_gic.c
+++ b/hw/intc/exynos4210_gic.c
@@ -293,6 +293,7 @@ static void exynos4210_gic_realize(DeviceState *dev, Error 
**errp)
 char cpu_alias_name[sizeof(cpu_prefix) + 3];
 char dist_alias_name[sizeof(cpu_prefix) + 3];
 SysBusDevice *gicbusdev;
+uint32_t n = s->num_cpu;
 uint32_t i;
 
 s->gic = qdev_create(NULL, "arm_gic");
@@ -313,7 +314,14 @@ static void exynos4210_gic_realize(DeviceState *dev, Error 
**errp)
 memory_region_init(&s->dist_container, obj, "exynos4210-dist-container",
 EXYNOS4210_EXT_GIC_DIST_REGION_SIZE);
 
-for (i = 0; i < s->num_cpu; i++) {
+/*
+ * This clues in gcc that our on-stack buffers do, in fact have
+ * enough room for the cpu numbers.  gcc 9.2.1 on 32-bit x86
+ * doesn't figure this out, otherwise and gives spurious warnings.
+ */
+assert(n <= EXYNOS4210_NCPUS);
+for (i = 0; i < n; i++) {
+
 /* Map CPU interface per SMP Core */
 sprintf(cpu_alias_name, "%s%x", cpu_prefix, i);
 memory_region_init_alias(&s->cpu_alias[i], obj,
-- 
2.23.0

Re: [PATCH v4 01/37] qdev: remove unused qdev_prop_int64

[Markus Armbruster]

Marc-André Lureau  writes:

> Signed-off-by: Marc-André Lureau 
> ---
>  hw/core/qdev-properties.c| 32 
>  include/hw/qdev-properties.h |  3 ---
>  2 files changed, 35 deletions(-)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index ac28890e5a..be4cb01f0b 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -409,31 +409,6 @@ static void set_uint64(Object *obj, Visitor *v, const 
> char *name,
>  visit_type_uint64(v, name, ptr, errp);
>  }
>  
> -static void get_int64(Object *obj, Visitor *v, const char *name,
> -  void *opaque, Error **errp)
> -{
> -DeviceState *dev = DEVICE(obj);
> -Property *prop = opaque;
> -int64_t *ptr = qdev_get_prop_ptr(dev, prop);
> -
> -visit_type_int64(v, name, ptr, errp);
> -}
> -
> -static void set_int64(Object *obj, Visitor *v, const char *name,
> -  void *opaque, Error **errp)
> -{
> -DeviceState *dev = DEVICE(obj);
> -Property *prop = opaque;
> -int64_t *ptr = qdev_get_prop_ptr(dev, prop);
> -
> -if (dev->realized) {
> -qdev_prop_set_after_realize(dev, name, errp);
> -return;
> -}
> -
> -visit_type_int64(v, name, ptr, errp);
> -}
> -
>  const PropertyInfo qdev_prop_uint64 = {
>  .name  = "uint64",
>  .get   = get_uint64,
> @@ -441,13 +416,6 @@ const PropertyInfo qdev_prop_uint64 = {
>  .set_default_value = set_default_value_uint,
>  };
>  
> -const PropertyInfo qdev_prop_int64 = {
> -.name  = "int64",
> -.get   = get_int64,
> -.set   = set_int64,
> -.set_default_value = set_default_value_int,
> -};
> -
>  /* --- string --- */
>  
>  static void release_string(Object *obj, const char *name, void *opaque)
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index c6a8cb5516..690ff07ae2 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -13,7 +13,6 @@ extern const PropertyInfo qdev_prop_uint16;
>  extern const PropertyInfo qdev_prop_uint32;
>  extern const PropertyInfo qdev_prop_int32;
>  extern const PropertyInfo qdev_prop_uint64;
> -extern const PropertyInfo qdev_prop_int64;
>  extern const PropertyInfo qdev_prop_size;
>  extern const PropertyInfo qdev_prop_string;
>  extern const PropertyInfo qdev_prop_chr;
> @@ -164,8 +163,6 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
>  DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_int32, int32_t)
>  #define DEFINE_PROP_UINT64(_n, _s, _f, _d)  \
>  DEFINE_PROP_UNSIGNED(_n, _s, _f, _d, qdev_prop_uint64, uint64_t)
> -#define DEFINE_PROP_INT64(_n, _s, _f, _d)  \
> -DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_int64, int64_t)
>  #define DEFINE_PROP_SIZE(_n, _s, _f, _d)   \
>  DEFINE_PROP_UNSIGNED(_n, _s, _f, _d, qdev_prop_size, uint64_t)
>  #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)   \

History of its use:

Author: Peter Xu 
Date:   Tue Jul 18 11:39:01 2017 +0800

qdev: provide DEFINE_PROP_INT64()

We have nearly all the stuff, but this one is missing. Add it in.

Am going to use this new helper for MigrationParameters fields, since
most of them are int64_t.

CC: Markus Armbruster 
CC: Eduardo Habkost 
CC: Marc-André Lureau 
CC: Peter Xu 
CC: Juan Quintela 
CC: Marcel Apfelbaum 
CC: Eric Blake 
Reviewed-by: Marc-André Lureau 
Reviewed-by: Marcel Apfelbaum 
Reviewed-by: Juan Quintela 
Reviewed-by: Eduardo Habkost 
Signed-off-by: Peter Xu 
Message-Id: <1500349150-13240-2-git-send-email-pet...@redhat.com>
Signed-off-by: Juan Quintela 

commit 89632fafdc64927647b6f45416307cd3d4c746db
Author: Peter Xu 
Date:   Tue Jul 18 11:39:02 2017 +0800

migration: export parameters to props

Export migration parameters to qdev properties. Then we can use, for
example:

  -global migration.x-cpu-throttle-initial=xxx

To specify migration parameters during init.

Prefix "x-" is appended for each parameter exported to show that this is
not a stable interface, and only for debugging/testing purpose.

Reviewed-by: Juan Quintela 
Signed-off-by: Peter Xu 
Message-Id: <1500349150-13240-3-git-send-email-pet...@redhat.com>
Signed-off-by: Juan Quintela 

[More commits adding uses...]

commit 741d4086c856320807a2575389d7c0505578270b
Author: Juan Quintela 
Date:   Fri Dec 1 13:08:38 2017 +0100

migration: Use proper types in json

We use int for everything (int64_t), and then we check that value is
between 0 and 255.  Change it to the valid types.

This change only happens for HMP.  QMP always use bytes and similar.

Signed-off-by: Juan Quintela 
Reviewed-by: Eric Blake 

[All uses 

Re: [PATCH 13/21] memory-device: Fix latent memory pre-plug error handling bugs

[Markus Armbruster]

David Hildenbrand  writes:

> On 30.11.19 20:42, Markus Armbruster wrote:
>> memory_device_get_free_addr() crashes when
>> memory_device_check_addable() fails and its @errp argument is null.
>> Messed up in commit 1b6d6af21b "pc-dimm: factor out capacity and slot
>> checks into MemoryDevice".
>> 
>> The bug can't bite as no caller actually passes null.  Fix it anyway.
>> 
>> Cc: David Hildenbrand 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  hw/mem/memory-device.c | 6 --
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>> index aef148c1d7..4bc9cf0917 100644
>> --- a/hw/mem/memory-device.c
>> +++ b/hw/mem/memory-device.c
>> @@ -99,6 +99,7 @@ static uint64_t memory_device_get_free_addr(MachineState 
>> *ms,
>>  uint64_t align, uint64_t size,
>>  Error **errp)
>>  {
>> +Error *err = NULL;
>>  GSList *list = NULL, *item;
>>  Range as, new = range_empty;
>>  
>> @@ -123,8 +124,9 @@ static uint64_t memory_device_get_free_addr(MachineState 
>> *ms,
>>  return 0;
>>  }
>>  
>> -memory_device_check_addable(ms, size, errp);
>> -if (*errp) {
>> +memory_device_check_addable(ms, size, &err);
>> +if (err) {
>> +error_propagate(errp, err);
>>  return 0;
>>  }
>
> I remember that some time ago, the best practice was to use "local_err",
> what is the latest state of that?

Hundreds of local Error * variables are named @local_err, and hundreds
more are named @err.

For what it's worth, the big comment in error.h uses @err, except in one
place where it needs two of them.

> I still disagree that these are BUGs or even latent BUGs. If somebody
> things these are BUGs and not cleanups, then we should probably have
> proper "Fixes: " tags instead.

Let's continue that discussion in the sub-thread where you first raised
this objection.

> Reviewed-by: David Hildenbrand 

Thanks!

Re: [PATCH 16/21] s390/cpu_modules: Fix latent realize() error handling bugs

[Markus Armbruster]

David Hildenbrand  writes:

> On 30.11.19 20:42, Markus Armbruster wrote:
>> get_max_cpu_model() crashes when kvm_s390_get_host_cpu_model() fails
>> and its @errp argument is null.
>> 
>> apply_cpu_model() crashes when kvm_s390_apply_cpu_model() fails and
>> its @errp argument is null.
>> 
>> s390_realize_cpu_model() crashes when get_max_cpu_model() or
>> check_compatibility() fail, and its @errp argument is null.
>> 
>> All three messed up in commit 80560137cf "s390x/cpumodel: check and
>> apply the CPU model".
>> 
>> The bugs can't bite as no caller actually passes null.  Fix them
>> anyway.
>> 
>
> Subject is wrong, should e.g., start with "s390x/cpumodels". (I am not
> aware of CPU modules :) )

Of course.

> [...]
>
> Same comment regarding "local_err" and "BUG".
>
> Reviewed-by: David Hildenbrand 

Thanks!

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[Markus Armbruster]

David Hildenbrand  writes:

> On 01.12.19 14:46, Aleksandar Markovic wrote:
>> 
>> 
>> On Saturday, November 30, 2019, David Hildenbrand > > wrote:
>> 
>> 
>> 
>> > Am 30.11.2019 um 20:42 schrieb Markus Armbruster
>> mailto:arm...@redhat.com>>:
>> >
>> > cpu_model_from_info() is a helper for
>> qmp_query_cpu_model_expansion(),
>> > qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
>> > crashes when the visitor or the QOM setter fails, and its @errp
>> > argument is null.  Messed up in commit 137974cea3 's390x/cpumodel:
>> > implement QMP interface "query-cpu-model-expansion"'.
>> >
>> > Its three callers have the same bug.  Messed up in commit 4e82ef0502
>> > 's390x/cpumodel: implement QMP interface "query-cpu-model-comparison"'
>> > and commit f1a47d08ef 's390x/cpumodel: implement QMP interface
>> > "query-cpu-model-baseline"'.
>> >
>> > The bugs can't bite as no caller actually passes null.  Fix them
>> > anyway.
>> 
>> https://en.m.wikipedia.org/wiki/Software_bug
>> 
>> 
>>   „ A software bug is an error, flaw or fault in a computer program
>> or system that causes it to produce an incorrect or unexpected
>> result, or to behave in unintended ways. „
>> 
>> Please make it clear in the descriptions that these are cleanups and
>> not bugfixes. It might be very confusing for people looking out for
>> real bugs.
>> 
>> 
>> 
>> Disclaimer: I am not entirely familiar with the code in question, so
>> take my opinion with reasonablereservation.
>> 
>> It looks that we here deal with latent bugs. As you probably know from
>> experience, a latent bugs, when they are activated with some ostensibly
>> unrelated code change, can be much more difficult to diagnose and fix
>> than regular bugs.
>
> "https://economictimes.indiatimes.com/definition/latent-bug
>
> "Definition: An uncovered or unidentified bug which exists in the system
> over a period of time is known as the Latent Bug. The bug may persist in
> the system in one or more versions of the software."
>
> AFAIK, a latent BUG can be triggered, it simply was never triggered.

First search hit.  Here's my second one:

Q: What are latent bugs?

A: These bugs do not cause problems today. However, they are lurking
just waiting to reveal themselves later.  The Ariane 5 rocket
failure was caused by a float->int conversion error that lay dormant
when previous rockets were slower; but the faster Ariane 5 triggered
the problem.  The Millennium bug is another example of a latent bug
that came to light when circumstances changed.  Latent bugs are much
harder to test using conventional testing techniques, and finding
them requires someone with foresight to ask.

http://www.geekinterview.com/question_details/36689

My point is: common usage of the term is not as clear-cut as your quote
makes it seem.

> Do you think the following code is buggy?
>
> static int get_val(int *ptr)
> {
>   return *ptr;
> }
>
> int main()
> {
>   int a = 0;
>
>   return get_val(&a);
> }
>
> I claim, no, although we could access a NULL pointer if ever reworked.
> There is no invalid system state possible.

get_val() is silent on how it wants to be used.  error.h is anything
but: it spells out how it wantes to be used in quite some detail.  In
particular:

 * Receive an error and pass it on to the caller:
 * Error *err = NULL;
 * foo(arg, &err);
 * if (err) {
 * handle the error...
 * error_propagate(errp, err);
 * }
 * where Error **errp is a parameter, by convention the last one.
 *
 * Do *not* "optimize" this to
 * foo(arg, errp);
 * if (*errp) { // WRONG!
 * handle the error...
 * }
 * because errp may be NULL!

My patch fixes this exact misuse of the interface.

>> In that light, this change is not a clean up. It is a fix of a latent
>> bugs, and Markus' aproach to treat it as a bug fix looks right to me. I
>> would just add a word "latent" or similar, which would even more
>> distance the patch from "cleanup" meaning.
>
> I agree iff there is some way to trigger it. Otherwise, to me it is a
> cleanup.If it's a BUG, it deserves proper Fixes tags and some
> description how it can be triggered.

Yes, a bug that can bite deserves a reproducer and a formal Fixes: tag.

The thing we're discussing (however we may want to call it) does not
have a reproducer, and I think we're in agreement that it doesn't need a
Fixes: tag.

However, my patch is not cleaning up something that's dirty, it's fixing
something that's unequivocally wrong: a violation of a stated interface
contract.

The violation happens to have no ill effects at this time due to the way
the violating code is being used.

I call that a "latent bug".  git-log has quite a few occurences of
"latent bug", by Richard Henderson, Daniel Berrangé, Paolo, ...

Re: [PATCH] configure: Use lld --image-base for --disable-pie user mode binaries

[Fangrui Song]

Thanks for reviewing this patch!

On 2019-12-01, Richard Henderson wrote:

On 11/27/19 6:36 PM, Fangrui Song wrote:

On 2019-11-20, Fangrui Song wrote:

On 2019-11-15, Fangrui Song wrote:

For lld, --image-base is the preferred way to set the base address.
lld does not actually implement -Ttext-segment, but treats it as an alias for
-Ttext. -Ttext-segment=0x6000 combined with --no-rosegment can
create a 1.6GB executable.

Fix the problem by using --image-base for lld. GNU ld and gold will
still get -Ttext-segment. Also delete the ld --verbose fallback introduced
in 2013, which is no longer relevant or correct (the default linker
script has changed).

Signed-off-by: Fangrui Song 
---
configure | 33 -
1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/configure b/configure
index 6099be1d84..2d45af0d09 100755
--- a/configure
+++ b/configure
@@ -6336,43 +6336,34 @@ fi

# Probe for the need for relocating the user-only binary.
if ( [ "$linux_user" = yes ] || [ "$bsd_user" = yes ] ) && [ "$pie" = no ];
then
-  textseg_addr=
+  image_base=
  case "$cpu" in
    arm | i386 | ppc* | s390* | sparc* | x86_64 | x32)
-  # ??? Rationale for choosing this address
-  textseg_addr=0x6000
+  # An arbitrary address that makes it unlikely to collide with user
+  # programs.


Please don't replace this ??? with an arbitrary rationale, which clearly
doesn't apply to all of these hosts.


In
https://lists.nongnu.org/archive/html/qemu-devel/2019-11/msg04646.html
it was suggested to move the comment around a bit.
I am not puzzled where and what I should say in the comment.
Can you (or other maintainers) kindly edit the comment for me?
I do not know enough about qemu to provide a good rationale here.


+  image_base=0x6000
  ;;
    mips)
  # A 256M aligned address, high in the address space, with enough
  # room for the code_gen_buffer above it before the stack.


This is the only one with a proper rationale.

That said, I'm not sure that the proper way to handle this issue with lld is to
drop this code entirely.


The patch changes a feature that lld does not support: -Ttext-segment,
to use --image-base instead.

Due to the prevalence of -z separate-code in GNU ld, -Ttext-segment is
no longer appropriate. I suggested that GNU linkers implement the
feature https://sourceware.org/bugzilla/show_bug.cgi?id=25207 .

What gets deleted is the sed script. As I explained in the commit
message, it is no longer relevant. It probably applies to an old GNU ld
that FreeBSD used. FreeBSD has switched to lld now.


The best way to handle the underlying issue -- address conflict between
interpreter and guest binary -- is PIE, for which this code is skipped.

After that, we go to some pain to choose a guest_base address that allows the
guest binary to load around the interpreter's reserved addresses.

So what's left that this messing about with link addresses buys us?


I agree that --enable-pie will be a better solution, but dropping the
support now will break at least FreeBSD. Its kernel supports running an
ET_DYN executable but it does not perform address randomization.
--disable-pie also appears to be used by ChromeOS developers who
reported https://bugs.llvm.org/show_bug.cgi?id=43997 . I can communicate
to them that migrating to --enable-pie is the way going forward.

Re: Network connection with COLO VM

[Daniel Cho]

Hi Zhang,

We use qemu-4.1.0 release on this case.

I think we need use block mirror to sync the disk to secondary node first,
then stop the primary VM and build COLO system.

In the stop moment, you need add some netfilter and chardev socket node for
COLO, maybe you need re-check this part.


Our test was already follow those step. Maybe I could describe the detail
of the test flow and issues.


Step 1:

Create primary VM without any netfilter and chardev for COLO, and using
other host ping primary VM continually.


Step 2:

Create secondary VM (the same device/drive with primary VM), and do block
mirror sync ( ping to primary VM normally )


Step 3:

After block mirror sync finish, add those netfilter and chardev to primary
VM and secondary VM for COLO ( *Can't* ping to primary VM but those packets
will be received later )


Step 4:

Start migrate primary VM to secondary VM, and primary VM & secondary VM are
running ( ping to primary VM works and receive those packets on step 3
status )




Between Step 3 to Step 4, it will take 10~20 seconds in our environment.

I could image this issue (delay reply packets) is because of setting COLO
proxy for temporary status,

but we thought 10~20 seconds might a little long. (If primary VM is already
doing some jobs, it might lose the data.)


Could we reduce those time? or those delay is depends on different VM?


Best Regard,

Daniel Cho.



Zhang, Chen  於 2019年11月30日 週六 上午2:04寫道：

>
>
>
>
> *From:* Daniel Cho 
> *Sent:* Friday, November 29, 2019 10:43 AM
> *To:* Zhang, Chen 
> *Cc:* Dr. David Alan Gilbert ; lukasstra...@web.de;
> qemu-devel@nongnu.org
> *Subject:* Re: Network connection with COLO VM
>
>
>
> Hi David,  Zhang,
>
>
>
> Thanks for replying my question.
>
> We know why will occur this issue.
>
> As you said, the COLO VM's network needs
>
> colo-proxy to control packets, so the guest's
>
> interface should set the filter to solve the problem.
>
>
>
> But we found another question, when we set the
>
> fault-tolerance feature to guest (primary VM is running,
>
> secondary VM is pausing), the guest's network would not
>
> responds any request for a while (in our environment
>
> about 20~30 secs) after secondary VM runs.
>
>
>
> Does it be a normal situation, or a known issue?
>
>
>
> Our test is creating primary VM for a while, then creating
>
> secondary VM to make it with COLO feature.
>
>
>
> Hi Daniel,
>
>
>
> Happy to hear you have solved ssh disconnection issue.
>
>
>
> Do you use Lukas’s patch on this case?
>
> I think we need use block mirror to sync the disk to secondary node first,
> then stop the primary VM and build COLO system.
>
> In the stop moment, you need add some netfilter and chardev socket node
> for COLO, maybe you need re-check this part.
>
>
>
> Best Regard,
>
> Daniel Cho
>
>
>
> Zhang, Chen  於 2019年11月28日 週四 上午9:26寫道：
>
>
>
> > -Original Message-
> > From: Dr. David Alan Gilbert 
> > Sent: Wednesday, November 27, 2019 6:51 PM
> > To: Daniel Cho ; Zhang, Chen
> > ; lukasstra...@web.de
> > Cc: qemu-devel@nongnu.org
> > Subject: Re: Network connection with COLO VM
> >
> > * Daniel Cho (daniel...@qnap.com) wrote:
> > > Hello everyone,
> > >
> > > Could we ssh to colo VM (means PVM & SVM are starting)?
> > >
> >
> > Lets cc in Zhang Chen and Lukas Straub.
>
> Thanks Dave.
>
> >
> > > SSH will connect to colo VM for a while, but it will disconnect with
> > > error
> > > *client_loop: send disconnect: Broken pipe*
> > >
> > > It seems to colo VM could not keep network session.
> > >
> > > Does it be a known issue?
> >
> > That sounds like the COLO proxy is getting upset; it's supposed to
> compare
> > packets sent by the primary and secondary and only send one to the
> outside
> > - you shouldn't be talking directly to the guest, but always via the
> proxy.  See
> > docs/colo-proxy.txt
> >
>
> Hi Daniel,
>
> I have try ssh to COLO guest with 8 hours, not occurred this issue.
> Please check your network/qemu configuration.
> But I found another problem maybe related this issue, if no network
> communication for a period of time(maybe 10min), the first message send to
> guest have a chance with delay(maybe 1-5 sec), I will try to fix it when I
> have time.
>
> Thanks
> Zhang Chen
>
> > Dave
> >
> > > Best Regard,
> > > Daniel Cho
> > --
> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
>
>

Re: [PATCH] virtio-input: fix memory leak in virtio_input_device_unrealize()

[Marc-André Lureau]

On Mon, Dec 2, 2019 at 5:20 AM  wrote:
>
> From: PanNengyuan 
>
> vdev->vq[i] is forgot to cleanup in
> virtio_input_device_unrealize, the memory leak stack is as bellow:
>
> Direct leak of 3584 byte(s) in 1 object(s) allocated from:
> #0 0x7f84a49f6560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
> #1 0x7f84a3b3e015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
> #2 0x559c0f0b33e7 in virtio_add_queue 
> /mnt/sdb/qemu-4.2.0-rc0/hw/virtio/virtio.c:2327
> #3 0x559c0f205c24 in virtio_input_device_realize 
> /mnt/sdb/qemu-4.2.0-rc0/hw/input/virtio-input.c:262
> #4 0x559c0f0b06a7 in virtio_device_realize 
> /mnt/sdb/qemu-4.2.0-rc0/hw/virtio/virtio.c:3504
> #5 0x559c0f1ba031 in device_set_realized  
> /mnt/sdb/qemu-4.2.0-rc0/hw/core/qdev.c:876
> #6 0x559c0f32cedd in property_set_bool 
> /mnt/sdb/qemu-4.2.0-rc0/qom/object.c:2080
> #7 0x559c0f3314ee in object_property_set_qobject 
> /mnt/sdb/qemu-4.2.0-rc0/qom/qom-qobject.c:26
>
> Direct leak of 3584 byte(s) in 1 object(s) allocated from:
> #0 0x7f84a49f6560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
> #1 0x7f84a3b3e015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
> #2 0x559c0f0b33e7 in virtio_add_queue 
> /mnt/sdb/qemu-4.2.0-rc0/hw/virtio/virtio.c:2327
> #3 0x559c0f205c3f in virtio_input_device_realize 
> /mnt/sdb/qemu-4.2.0-rc0/hw/input/virtio-input.c:263
> #4 0x559c0f0b06a7 in virtio_device_realize 
> /mnt/sdb/qemu-4.2.0-rc0/hw/virtio/virtio.c:3504
> #5 0x559c0f1ba031 in device_set_realized 
> /mnt/sdb/qemu-4.2.0-rc0/hw/core/qdev.c:876
> #6 0x559c0f32cedd in property_set_bool 
> /mnt/sdb/qemu-4.2.0-rc0/qom/object.c:2080
> #7 0x559c0f3314ee in object_property_set_qobject 
> /mnt/sdb/qemu-4.2.0-rc0/qom/qom-qobject.c:26
>
> Reported-by: Euler Robot 
> Signed-off-by: PanNengyuan 

This is already upstream:

commit 509ec36c1e4c559e90115a16403dea8d92dff335
Author: Marc-André Lureau 
Date:   Thu Nov 21 13:56:49 2019 +0400

virtio-input: fix memory leak on unrealize


> ---
>  hw/input/virtio-input.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/input/virtio-input.c b/hw/input/virtio-input.c
> index 51617a5..da94da4 100644
> --- a/hw/input/virtio-input.c
> +++ b/hw/input/virtio-input.c
> @@ -288,6 +288,9 @@ static void virtio_input_device_unrealize(DeviceState 
> *dev, Error **errp)
>  return;
>  }
>  }
> +
> +virtio_del_queue(vdev, 0);
> +virtio_del_queue(vdev, 1);
>  virtio_cleanup(vdev);
>  }
>
> --
> 2.7.2.windows.1
>
>
>


-- 
Marc-André Lureau

Re: [RFC net-next 08/18] tun: run offloaded XDP program in Tx path

[Jason Wang]

On 2019/12/2 上午5:54, David Miller wrote:

From: "Michael S. Tsirkin" 
Date: Sun, 1 Dec 2019 16:40:22 -0500


Right. But it is helpful to expose the supported functionality
to guest in some way, if nothing else then so that
guests can be moved between different hosts.

Also, we need a way to report this kind of event to guest
so it's possible to figure out what went wrong.

On the contrary, this is why it is of utmost importance that all
XDP implementations support the full suite of XDP facilities from
the very beginning.

This is why we keep giving people a hard time when they add support
only for some of the XDP return values and semantics.  Users will get
killed by this, and it makes XDP a poor technology to use because
behavior is not consistent across device types.

That's not acceptable and I'll push back on anything that continues
this trend.

If you can't HW offload it, kick it to software.



We can try to work out a solution for XDP_REDIRECT.

Thanks

Re: [RFC net-next 00/18] virtio_net XDP offload

[Jason Wang]

On 2019/12/2 上午12:54, David Ahern wrote:

On 11/27/19 10:18 PM, Jason Wang wrote:

We try to follow what NFP did by starting from a fraction of the whole
eBPF features. It would be very hard to have all eBPF features
implemented from the start.  It would be helpful to clarify what's the
minimal set of features that you want to have from the start.

Offloading guest programs needs to prevent a guest XDP program from
running bpf helpers that access host kernel data. e.g., bpf_fib_lookup



Right, so we probably need a new type of eBPF program on the host and 
filter out the unsupported helpers there.


Thanks

Re: [RFC net-next 07/18] tun: set offloaded xdp program

[Jason Wang]

On 2019/12/2 上午12:45, David Ahern wrote:

On 11/26/19 4:07 AM, Prashant Bhole wrote:

From: Jason Wang 

This patch introduces an ioctl way to set an offloaded XDP program
to tun driver. This ioctl will be used by qemu to offload XDP program
from virtio_net in the guest.


Seems like you need to set / reset the SOCK_XDP flag on tfile->sk since
this is an XDP program.

Also, why not add this program using netlink instead of ioctl? e.g., as
part of a generic XDP in the egress path like I am looking into for the
host side.



Maybe both, otherwise, qemu may need netlink as a dependency.

Thanks

Re: [RFC net-next 08/18] tun: run offloaded XDP program in Tx path

[Jason Wang]

On 2019/12/2 上午12:39, David Ahern wrote:

On 11/26/19 4:07 AM, Prashant Bhole wrote:

run offloaded XDP program as soon as packet is removed from the ptr
ring. Since this is XDP in Tx path, the traditional handling of
XDP actions XDP_TX/REDIRECT isn't valid. For this reason we call
do_xdp_generic_core instead of do_xdp_generic. do_xdp_generic_core
just runs the program and leaves the action handling to us.

What happens if an offloaded program returns XDP_REDIRECT?

Below you just drop the packet which is going to be a bad user
experience. A better user experience is to detect XDP return codes a
program uses, catch those that are not supported for this use case and
fail the install of the program.



Yes, this could be done in the guest.

Thanks

Re: [RFC net-next 07/18] tun: set offloaded xdp program

[Jason Wang]

On 2019/12/2 上午12:35, David Ahern wrote:

On 11/26/19 4:07 AM, Prashant Bhole wrote:

From: Jason Wang 

This patch introduces an ioctl way to set an offloaded XDP program
to tun driver. This ioctl will be used by qemu to offload XDP program
from virtio_net in the guest.

Signed-off-by: Jason Wang 
Signed-off-by: Prashant Bhole 
---
  drivers/net/tun.c   | 19 ++-
  include/uapi/linux/if_tun.h |  1 +
  2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index d078b4659897..ecb49101b0b5 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -241,6 +241,7 @@ struct tun_struct {
struct bpf_prog __rcu *xdp_prog;
struct tun_prog __rcu *steering_prog;
struct tun_prog __rcu *filter_prog;
+   struct tun_prog __rcu *offloaded_xdp_prog;

I have been looking into running XDP pograms in the TX path of a tap
device [1] where the program is installed and managed by a process in
the host. The code paths are the same as what you are doing with XDP
offload, so how about calling this xdp_prog_tx?

[1]
https://github.com/dsahern/linux/commit/f2303d05187c8a604cdb70b288338e9b1d1b0db6



I think it's fine, btw, except for the netlink part there should be no 
much difference.


Thanks

Re: [PATCH v2 11/14] target/arm: default SVE length to 64 bytes for linux-user

[Richard Henderson]

On 11/30/19 8:45 AM, Alex Bennée wrote:
> The Linux kernel chooses the default of 64 bytes for SVE registers on
> the basis that it is the largest size that won't grow the signal
> frame. When debugging larger sizes are also unwieldy in gdb as each
> zreg will take over a page of terminal to display.
> 
> The user can of course always specify a larger size with the
> sve-max-vq property on the command line:
> 
>   -cpu max,sve-max-vq=16
> 
> This should not make any difference to SVE enabled software as the SVE
> is of course vector length agnostic.
> 
> Signed-off-by: Alex Bennée 
> ---
>  target/arm/cpu64.c | 3 +++
>  1 file changed, 3 insertions(+)

6 is the largest size that doesn't grow the signal frame.
I imagine 4 was chosen because that's the only real hw atm.

> +/* Default sve-max-vq to a reasonable numer */
> +cpu->sve_max_vq = 4;

I also agree that we should match the kernel, but this is not the right way.
Changing max vq is not the same as changing the default vq.

You should change the value of env->vfp.zcr_el[1] in arm_cpu_reset(), and the
user can increase the length with prctl(2) as they would be able to on real
hardware that would have support for longer vector lengths.

Also, I don't think you should mix this up with gdb stuff.


r~

Re: [PATCH v6] ppc: well form kvmppc_hint_smt_possible error hint helper

[David Gibson]

On Wed, Nov 27, 2019 at 10:14:34PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Make kvmppc_hint_smt_possible hint append helper well formed:
> rename errp to errp_in, as it is IN-parameter here (which is unusual
> for errp), rename function to be kvmppc_error_append_*_hint.

I'm not entirely convinced by the errp_in name, since it's actually
both an in and out parameter.  Nonetheless, I've applied this to
ppc-for-5.0.


> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Reviewed-by: Marc-André Lureau 
> ---
> 
> v6: keep kvmppc_ function prefix
> add r-b by Marc-André
> 
>  target/ppc/kvm_ppc.h | 4 ++--
>  hw/ppc/spapr.c   | 2 +-
>  target/ppc/kvm.c | 6 +++---
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 98bd7d5da6..47b08a4030 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -28,7 +28,7 @@ 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_smt_threads(void);
> -void kvmppc_hint_smt_possible(Error **errp);
> +void kvmppc_error_append_smt_possible_hint(Error **errp_in);
>  int kvmppc_set_smt_threads(int smt);
>  int kvmppc_clear_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits);
>  int kvmppc_or_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits);
> @@ -164,7 +164,7 @@ static inline int kvmppc_smt_threads(void)
>  return 1;
>  }
>  
> -static inline void kvmppc_hint_smt_possible(Error **errp)
> +static inline void kvmppc_error_append_smt_possible_hint(Error **errp_in)
>  {
>  return;
>  }
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e076f6023c..1b87eb0ffd 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2564,7 +2564,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState 
> *spapr, Error **errp)
>" requires the use of VSMT mode %d.\n",
>smp_threads, kvm_smt, spapr->vsmt);
>  }
> -kvmppc_hint_smt_possible(&local_err);
> +kvmppc_error_append_smt_possible_hint(&local_err);
>  goto out;
>  }
>  }
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index c77f9848ec..7406d18945 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2076,7 +2076,7 @@ int kvmppc_set_smt_threads(int smt)
>  return ret;
>  }
>  
> -void kvmppc_hint_smt_possible(Error **errp)
> +void kvmppc_error_append_smt_possible_hint(Error **errp_in)
>  {
>  int i;
>  GString *g;
> @@ -2091,10 +2091,10 @@ void kvmppc_hint_smt_possible(Error **errp)
>  }
>  }
>  s = g_string_free(g, false);
> -error_append_hint(errp, "%s.\n", s);
> +error_append_hint(errp_in, "%s.\n", s);
>  g_free(s);
>  } else {
> -error_append_hint(errp,
> +error_append_hint(errp_in,
>"This KVM seems to be too old to support VSMT.\n");
>  }
>  }

-- 
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: [PATCH v2 08/14] gdbstub: extend GByteArray to read register helpers

[Richard Henderson]

On 11/30/19 8:45 AM, Alex Bennée wrote:
> -static int cpu_read_virt_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
> +static int cpu_read_virt_reg(CPUS390XState *env, GByteArray *mem_buf, int n)
>  {
>  switch (n) {
>  case S390_VIRT_CKC_REGNUM:
> @@ -296,9 +296,9 @@ static int cpu_write_virt_reg(CPUS390XState *env, uint8_t 
> *mem_buf, int n)
>  /* total number of registers in s390-gs.xml */
>  #define S390_NUM_GS_REGS 4
>  
> -static int cpu_read_gs_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
> +static int cpu_read_gs_reg(CPUS390XState *env, GByteArray *buf, int n)

Sometimes you're changing the name from mem_buf and sometimes not.  Perhaps
better not to change it anywhere, or change it everywhere first, without
changing the type.


r~

Re: [PATCH v2 06/14] target/arm: use gdb_get_reg helpers

[Richard Henderson]

On 11/30/19 8:45 AM, Alex Bennée wrote:
> This is cleaner than poking memory directly and will make later
> clean-ups easier.
> 
> Signed-off-by: Alex Bennée 
> 
> ---
> v2
>   - make sure we pass hi/lo correctly as quads are stored in LE order
> ---
>  target/arm/helper.c | 18 +++---
>  1 file changed, 7 insertions(+), 11 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [PATCH v2 05/14] gdbstub: add helper for 128 bit registers

[Richard Henderson]

On 11/30/19 8:45 AM, Alex Bennée wrote:
> Signed-off-by: Alex Bennée 
> 
> ---
> v2
>   - take care of endianess of the whole 128 bit word
> ---
>  include/exec/gdbstub.h | 13 +
>  1 file changed, 13 insertions(+)

Reviewed-by: Richard Henderson 

r~

Re: [PATCH v2 02/14] gdbstub: stop passing GDBState * around and use global

[Richard Henderson]

On 11/30/19 8:45 AM, Alex Bennée wrote:
> We only have one GDBState which should be allocated at the time we
> process any commands. This will make further clean-up a bit easier.
> 
> Signed-off-by: Alex Bennée 
> ---
>  gdbstub.c | 539 +++---
>  1 file changed, 267 insertions(+), 272 deletions(-)

Reviewed-by: Richard Henderson 

r~

Re: [PATCH v2 01/14] gdbstub: make GDBState static and have common init function

[Richard Henderson]

On 11/30/19 8:45 AM, Alex Bennée wrote:
> -static GDBState *gdbserver_state;
> +static GDBState gdbserver_state;
> +
> +static void init_gdbserver_state(void)
> +{
> +g_assert(!gdbserver_state.init);
> +memset(&gdbserver_state, 0, sizeof(GDBState));
> +gdbserver_state.init = true;
> +}

At no point does init transition from true to false, afaict.  Therefore the
memset is unnecessary, as the effect is had from the default
zero-initialization of the static variable.

Otherwise,
Reviewed-by: Richard Henderson 

r~

Re: [PATCH v7] Implement backend program convention command for vhost-user-blk

[Micky C]

Ping

On Mon, Nov 25, 2019 at 1:17 PM Micky Yun Chan(michiboo) <
chanmicky...@gmail.com> wrote:

> From: Micky Yun Chan 
>
> This patch is to add standard commands defined in
> docs/interop/vhost-user.rst
> For vhost-user-* program
>
> Signed-off-by: Micky Yun Chan (michiboo) 
> ---
>  contrib/vhost-user-blk/vhost-user-blk.c | 108 ++--
>  docs/interop/vhost-user.json|  31 +++
>  docs/interop/vhost-user.rst |  15 
>  3 files changed, 110 insertions(+), 44 deletions(-)
>
> diff --git a/contrib/vhost-user-blk/vhost-user-blk.c
> b/contrib/vhost-user-blk/vhost-user-blk.c
> index ae61034656..6fd91c7e99 100644
> --- a/contrib/vhost-user-blk/vhost-user-blk.c
> +++ b/contrib/vhost-user-blk/vhost-user-blk.c
> @@ -576,70 +576,90 @@ vub_new(char *blk_file)
>  return vdev_blk;
>  }
>
> +static int opt_fdnum = -1;
> +static char *opt_socket_path;
> +static char *opt_blk_file;
> +static gboolean opt_print_caps;
> +static gboolean opt_read_only;
> +
> +static GOptionEntry entries[] = {
> +{ "print-capabilities", 'c', 0, G_OPTION_ARG_NONE, &opt_print_caps,
> +  "Print capabilities", NULL },
> +{ "fd", 'f', 0, G_OPTION_ARG_INT, &opt_fdnum,
> +  "Use inherited fd socket", "FDNUM" },
> +{ "socket-path", 's', 0, G_OPTION_ARG_FILENAME, &opt_socket_path,
> +  "Use UNIX socket path", "PATH" },
> +{"blk-file", 'b', 0, G_OPTION_ARG_FILENAME, &opt_blk_file,
> + "block device or file path", "PATH"},
> +{ "read-only", 'r', 0, G_OPTION_ARG_NONE, &opt_read_only,
> +  "Enable read-only", NULL }
> +};
> +
>  int main(int argc, char **argv)
>  {
> -int opt;
> -char *unix_socket = NULL;
> -char *blk_file = NULL;
> -bool enable_ro = false;
>  int lsock = -1, csock = -1;
>  VubDev *vdev_blk = NULL;
> +GError *error = NULL;
> +GOptionContext *context;
>
> -while ((opt = getopt(argc, argv, "b:rs:h")) != -1) {
> -switch (opt) {
> -case 'b':
> -blk_file = g_strdup(optarg);
> -break;
> -case 's':
> -unix_socket = g_strdup(optarg);
> -break;
> -case 'r':
> -enable_ro = true;
> -break;
> -case 'h':
> -default:
> -printf("Usage: %s [ -b block device or file, -s UNIX domain
> socket"
> -   " | -r Enable read-only ] | [ -h ]\n", argv[0]);
> -return 0;
> +context = g_option_context_new(NULL);
> +g_option_context_add_main_entries(context, entries, NULL);
> +if (!g_option_context_parse(context, &argc, &argv, &error)) {
> +g_printerr("Option parsing failed: %s\n", error->message);
> +exit(EXIT_FAILURE);
> +}
> +if (opt_print_caps) {
> +g_print("{\n");
> +g_print("  \"type\": \"block\",\n");
> +g_print("  \"features\": [\n");
> +g_print("\"read-only\",\n");
> +g_print("\"blk-file\"\n");
> +g_print("  ]\n");
> +g_print("}\n");
> +exit(EXIT_SUCCESS);
> +}
> +
> +if (!opt_blk_file) {
> +g_print("%s\n", g_option_context_get_help(context, true, NULL));
> +exit(EXIT_FAILURE);
> +}
> +
> +if (opt_socket_path) {
> +lsock = unix_sock_new(opt_socket_path);
> +if (lsock < 0) {
> +exit(EXIT_FAILURE);
>  }
> +} else if (opt_fdnum < 0) {
> +g_print("%s\n", g_option_context_get_help(context, true, NULL));
> +exit(EXIT_FAILURE);
> +} else {
> +lsock = opt_fdnum;
>  }
>
> -if (!unix_socket || !blk_file) {
> -printf("Usage: %s [ -b block device or file, -s UNIX domain
> socket"
> -   " | -r Enable read-only ] | [ -h ]\n", argv[0]);
> -return -1;
> -}
> -
> -lsock = unix_sock_new(unix_socket);
> -if (lsock < 0) {
> -goto err;
> -}
> -
> -csock = accept(lsock, (void *)0, (void *)0);
> +csock = accept(lsock, NULL, NULL);
>  if (csock < 0) {
> -fprintf(stderr, "Accept error %s\n", strerror(errno));
> -goto err;
> +g_printerr("Accept error %s\n", strerror(errno));
> +exit(EXIT_FAILURE);
>  }
>
> -vdev_blk = vub_new(blk_file);
> +vdev_blk = vub_new(opt_blk_file);
>  if (!vdev_blk) {
> -goto err;
> +exit(EXIT_FAILURE);
>  }
> -if (enable_ro) {
> +if (opt_read_only) {
>  vdev_blk->enable_ro = true;
>  }
>
>  if (!vug_init(&vdev_blk->parent, VHOST_USER_BLK_MAX_QUEUES, csock,
>vub_panic_cb, &vub_iface)) {
> -fprintf(stderr, "Failed to initialized libvhost-user-glib\n");
> -goto err;
> +g_printerr("Failed to initialize libvhost-user-glib\n");
> +exit(EXIT_FAILURE);
>  }
>
>  g_main_loop_run(vdev_blk->loop);
> -
> +g_main_loop_unref(vdev_blk->loop);
> +g_option_context_free(context);
>  vug_deinit(&vdev_blk->parent);
> -
> -err:
>  vub_free(vdev_blk);
>  if

Re: [PATCH v1 5/5] linux-user: convert target_munmap debug to a tracepoint

[Richard Henderson]

On 11/28/19 7:46 PM, Alex Bennée wrote:
> Convert the final bit of DEBUG_MMAP to a tracepoint and remove the
> last remanents of the #ifdef hackery.
> 
> Signed-off-by: Alex Bennée 
> ---
>  linux-user/mmap.c   | 9 ++---
>  linux-user/trace-events | 1 +
>  2 files changed, 3 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson 


r~

Re: [PATCH v1 4/5] linux-user: log page table changes under -d page

[Richard Henderson]

On 11/28/19 7:46 PM, Alex Bennée wrote:
> +if (qemu_loglevel_mask(CPU_LOG_PAGE)) {
> +qemu_log_lock();
> +qemu_log("new page @ 0x"TARGET_ABI_FMT_lx" updates page map:\n", 
> start);
> +log_page_dump();
> +qemu_log_unlock();
> +}

Hmm.  The language used here asserts a change, when we don't actually know that
for a fact.  It *probably* adds a new page, but it could be overwriting an old
page.  Can you find a better wording here?


r~

[PATCH] virtio-input: fix memory leak in virtio_input_device_unrealize()

[pannengyuan]

From: PanNengyuan 

vdev->vq[i] is forgot to cleanup in
virtio_input_device_unrealize, the memory leak stack is as bellow:

Direct leak of 3584 byte(s) in 1 object(s) allocated from:
#0 0x7f84a49f6560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
#1 0x7f84a3b3e015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
#2 0x559c0f0b33e7 in virtio_add_queue 
/mnt/sdb/qemu-4.2.0-rc0/hw/virtio/virtio.c:2327
#3 0x559c0f205c24 in virtio_input_device_realize 
/mnt/sdb/qemu-4.2.0-rc0/hw/input/virtio-input.c:262
#4 0x559c0f0b06a7 in virtio_device_realize 
/mnt/sdb/qemu-4.2.0-rc0/hw/virtio/virtio.c:3504
#5 0x559c0f1ba031 in device_set_realized  
/mnt/sdb/qemu-4.2.0-rc0/hw/core/qdev.c:876
#6 0x559c0f32cedd in property_set_bool 
/mnt/sdb/qemu-4.2.0-rc0/qom/object.c:2080
#7 0x559c0f3314ee in object_property_set_qobject 
/mnt/sdb/qemu-4.2.0-rc0/qom/qom-qobject.c:26

Direct leak of 3584 byte(s) in 1 object(s) allocated from:
#0 0x7f84a49f6560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
#1 0x7f84a3b3e015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
#2 0x559c0f0b33e7 in virtio_add_queue 
/mnt/sdb/qemu-4.2.0-rc0/hw/virtio/virtio.c:2327
#3 0x559c0f205c3f in virtio_input_device_realize 
/mnt/sdb/qemu-4.2.0-rc0/hw/input/virtio-input.c:263
#4 0x559c0f0b06a7 in virtio_device_realize 
/mnt/sdb/qemu-4.2.0-rc0/hw/virtio/virtio.c:3504
#5 0x559c0f1ba031 in device_set_realized 
/mnt/sdb/qemu-4.2.0-rc0/hw/core/qdev.c:876
#6 0x559c0f32cedd in property_set_bool 
/mnt/sdb/qemu-4.2.0-rc0/qom/object.c:2080
#7 0x559c0f3314ee in object_property_set_qobject 
/mnt/sdb/qemu-4.2.0-rc0/qom/qom-qobject.c:26

Reported-by: Euler Robot 
Signed-off-by: PanNengyuan 
---
 hw/input/virtio-input.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/input/virtio-input.c b/hw/input/virtio-input.c
index 51617a5..da94da4 100644
--- a/hw/input/virtio-input.c
+++ b/hw/input/virtio-input.c
@@ -288,6 +288,9 @@ static void virtio_input_device_unrealize(DeviceState *dev, 
Error **errp)
 return;
 }
 }
+
+virtio_del_queue(vdev, 0);
+virtio_del_queue(vdev, 1);
 virtio_cleanup(vdev);
 }
 
-- 
2.7.2.windows.1

Re: [PATCH v1 3/5] linux-user: add target_mmap_complete tracepoint

[Richard Henderson]

On 11/28/19 7:46 PM, Alex Bennée wrote:
> For full details we also want to see where the mmaps end up.
> 
> Signed-off-by: Alex Bennée 
> ---
>  linux-user/mmap.c   | 2 +-
>  linux-user/trace-events | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson 


r~

Re: [PATCH v1 2/5] linux-user: convert target_mmap debug to tracepoint

[Richard Henderson]

On 11/28/19 7:46 PM, Alex Bennée wrote:
> +if (TRACE_TARGET_MMAP_ENABLED) {
> +char prot_str[4];
> +g_autoptr(GString) flag_str = g_string_new(NULL);
> +
> +pp_prot(&prot_str, prot);
> +
> +if (flags & MAP_FIXED) {
> +g_string_append(flag_str, "MAP_FIXED ");
> +}
> +if (flags & MAP_ANONYMOUS) {
> +g_string_append(flag_str, "MAP_ANON ");
> +}
> +
> +switch (flags & MAP_TYPE) {
>  case MAP_PRIVATE:
> -printf("MAP_PRIVATE ");
> +g_string_append(flag_str, "MAP_PRIVATE ");
>  break;
>  case MAP_SHARED:
> -printf("MAP_SHARED ");
> +g_string_append(flag_str, "MAP_SHARED ");
>  break;
>  default:
> -printf("[MAP_TYPE=0x%x] ", flags & MAP_TYPE);
> +g_string_append_printf(flag_str, "[MAP_TYPE=0x%x] ",
> +   flags & MAP_TYPE);
>  break;
>  }
> -printf("fd=%d offset=" TARGET_ABI_FMT_lx "\n", fd, offset);
> +trace_target_mmap(start, len, prot_str, flag_str->str, fd, offset);
>  }

I don't think that you need to re-create -strace output.
There are also quite a lot of MAP_* flags that are not
being printed, without any indication that they are left out.

Again, I think we should just print the hex value.


r~

Re: [PATCH v1 1/5] linux-user: convert target_mprotect debug to tracepoint

[Richard Henderson]

On 11/28/19 7:45 PM, Alex Bennée wrote:
> -#ifdef DEBUG_MMAP
> -printf("mprotect: start=0x" TARGET_ABI_FMT_lx
> -   "len=0x" TARGET_ABI_FMT_lx " prot=%c%c%c\n", start, len,
> -   prot & PROT_READ ? 'r' : '-',
> -   prot & PROT_WRITE ? 'w' : '-',
> -   prot & PROT_EXEC ? 'x' : '-');
> -#endif
> +if (TRACE_TARGET_MPROTECT_ENABLED) {
> +char prot_str[4];
> +prot_str[0] = prot & PROT_READ ? 'r' : '-';
> +prot_str[1] = prot & PROT_WRITE ? 'w' : '-';
> +prot_str[2] = prot & PROT_EXEC ? 'x' : '-';
> +prot_str[3] = 0;
> +trace_target_mprotect(start, len, prot_str);
> +}

There are other bits in prot other than these 3.
See especially my linux-user BTI patch set, and
know that the same sort of thing will be in the
as-yet undecided abi for the MTE extension.

Unless you have a good reason otherwise, I think
we should just print the numeric value in hex.


r~

Re: [PATCH v37 10/17] target/avr: Add instruction disassembly function

[Aleksandar Markovic]

On Wednesday, November 27, 2019, Michael Rolnik  wrote:

> Provide function disassembles executed instruction when `-d in_asm` is
> provided
>
> Example:
> `./avr-softmmu/qemu-system-avr -bios 
> free-rtos/Demo/AVR_ATMega2560_GCC/demo.elf
> -d in_asm` will produce something like the following
>
> ```
> ...
> IN:
> 0x014a:  CALL  0x3808
>
> IN: main
> 0x3808:  CALL  0x4b4
>
> IN: vParTestInitialise
> 0x04b4:  LDI   r24, 255
> 0x04b6:  STS   r24, 0
> 0x04b8:  MULS  r16, r20
> 0x04ba:  OUT   $1, r24
> 0x04bc:  LDS   r24, 0
> 0x04be:  MULS  r16, r20
> 0x04c0:  OUT   $2, r24
> 0x04c2:  RET
> ...
> ```
>
> Signed-off-by: Michael Rolnik 
> Suggested-by: Richard Henderson 
> Suggested-by: Philippe Mathieu-Daudé 
> Suggested-by: Aleksandar Markovic 
> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 
> ---
>  target/avr/cpu.h   |   1 +
>  target/avr/cpu.c   |   2 +-
>  target/avr/disas.c | 228 +
>  target/avr/translate.c |  11 ++
>  4 files changed, 241 insertions(+), 1 deletion(-)
>  create mode 100644 target/avr/disas.c
>
> diff --git a/target/avr/cpu.h b/target/avr/cpu.h
> index 9ea5260165..a3e615a1eb 100644
> --- a/target/avr/cpu.h
> +++ b/target/avr/cpu.h
> @@ -157,6 +157,7 @@ bool avr_cpu_exec_interrupt(CPUState *cpu, int
> int_req);
>  hwaddr avr_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
>  int avr_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
>  int avr_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
> +int avr_print_insn(bfd_vma addr, disassemble_info *info);
>
>  static inline int avr_feature(CPUAVRState *env, int feature)
>  {
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index dae56d7845..52ec21dd16 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -83,7 +83,7 @@ static void avr_cpu_reset(CPUState *cs)
>  static void avr_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
>  {
>  info->mach = bfd_arch_avr;
> -info->print_insn = NULL;
> +info->print_insn = avr_print_insn;
>  }
>
>  static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
> diff --git a/target/avr/disas.c b/target/avr/disas.c
> new file mode 100644
> index 00..a51ade7c2a
> --- /dev/null
> +++ b/target/avr/disas.c
> @@ -0,0 +1,228 @@
> +/*
> + * AVR disassembler
> + *
> + * Copyright (c) 2018 Richard Henderson 


Just a detail: since this file is created in 2019, the copyright year
should be 2019 too.

+ * Copyright (c) 2019 Michael Rolnik 
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#include "qemu/osdep.h"
> +#include "disas/dis-asm.h"
> +#include "qemu/bitops.h"
> +#include "cpu.h"
> +
> +typedef struct {
> +disassemble_info *info;
> +uint16_t next_word;
> +bool next_word_used;
> +} DisasContext;
> +
> +static int to_regs_16_31_by_one(DisasContext *ctx, int indx)
> +{
> +return 16 + (indx % 16);
> +}
> +
> +static int to_regs_16_23_by_one(DisasContext *ctx, int indx)
> +{
> +return 16 + (indx % 8);
> +}
> +static int to_regs_24_30_by_two(DisasContext *ctx, int indx)
> +{
> +return 24 + (indx % 4) * 2;
> +}
> +static int to_regs_00_30_by_two(DisasContext *ctx, int indx)
> +{
> +return (indx % 16) * 2;
> +}
> +
> +static uint16_t next_word(DisasContext *ctx)
> +{
> +ctx->next_word_used = true;
> +return ctx->next_word;
> +}
> +
> +static int append_16(DisasContext *ctx, int x)
> +{
> +return x << 16 | next_word(ctx);
> +}
> +
> +
> +/* Include the auto-generated decoder.  */
> +static bool decode_insn(DisasContext *ctx, uint16_t insn);
> +#include "decode_insn.inc.c"
> +
> +#define output(mnemonic, format, ...) \
> +(pctx->info->fprintf_func(pctx->info->stream, "%-9s " format, \
> +mnemonic, ##__VA_ARGS__))
> +
> +int avr_print_insn(bfd_vma addr, disassemble_info *info)
> +{
> +DisasContext ctx;
> +DisasContext *pctx = &ctx;
> +bfd_byte buffer[4];
> +uint16_t insn;
> +int status;
> +
> +ctx.info = info;
> +
> +status = info->read_memory_func(addr, buffer, 4, info);
> +if (status != 0) {
> +info->memory_error_func(status, addr, info);
> +return -1;
> +}
> +insn = bfd_getl16(buff

Re: [PATCH 4/5] mips: r4000: Renovate coding style

[Aleksandar Markovic]

On Monday, November 25, 2019, Filip Bozuta  wrote:

> The script checkpatch.pl located in scripts folder was
> used to detect all errors and warrnings in file:
> hw/mips/mips_r4k.c
>
> This mips r4000 machine file was edited and
> all the errors and warrings generated by the checkpatch.pl
> script were corrected and then the script was
> ran again to make sure there are no more errors and warnings.
>
> Signed-off-by: Filip Bozuta 
> ---
>  hw/mips/mips_r4k.c | 55 ++
> +++-
>  1 file changed, 34 insertions(+), 21 deletions(-)
>
> diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
> index 7002423..d638358 100644
> --- a/hw/mips/mips_r4k.c
> +++ b/hw/mips/mips_r4k.c
> @@ -6,7 +6,7 @@
>   * ISA memory at the 0x1000 (PHYS, 16Mb in size).
>   * All peripherial devices are attached to this "bus" with
>   * the standard PC ISA addresses.
> -*/
> + */
>
>  #include "qemu/osdep.h"
>  #include "qemu/units.h"
> @@ -54,17 +54,18 @@ static struct _loaderparams {
>  const char *initrd_filename;
>  } loaderparams;
>
> -static void mips_qemu_write (void *opaque, hwaddr addr,
> - uint64_t val, unsigned size)
> +static void mips_qemu_write(void *opaque, hwaddr addr,
> +uint64_t val, unsigned size)
>  {
> -if ((addr & 0x) == 0 && val == 42)
> +if ((addr & 0x) == 0 && val == 42) {
>  qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> -else if ((addr & 0x) == 4 && val == 42)
> +} else if ((addr & 0x) == 4 && val == 42) {
>  qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +}
>  }
>
> -static uint64_t mips_qemu_read (void *opaque, hwaddr addr,
> -unsigned size)
> +static uint64_t mips_qemu_read(void *opaque, hwaddr addr,
> +   unsigned size)
>  {
>  return 0;
>  }
> @@ -100,8 +101,9 @@ static int64_t load_kernel(void)
> (uint64_t *)&kernel_high, big_endian,
> EM_MIPS, 1, 0);
>  if (kernel_size >= 0) {
> -if ((entry & ~0x7fffULL) == 0x8000)
> +if ((entry & ~0x7fffULL) == 0x8000) {
>  entry = (int32_t)entry;
> +}
>  } else {
>  error_report("could not load kernel '%s': %s",
>   loaderparams.kernel_filename,
> @@ -113,9 +115,10 @@ static int64_t load_kernel(void)
>  initrd_size = 0;
>  initrd_offset = 0;
>  if (loaderparams.initrd_filename) {
> -initrd_size = get_image_size (loaderparams.initrd_filename);
> +initrd_size = get_image_size(loaderparams.initrd_filename);
>  if (initrd_size > 0) {
> -initrd_offset = (kernel_high + ~INITRD_PAGE_MASK) &
> INITRD_PAGE_MASK;
> +initrd_offset = (kernel_high + ~INITRD_PAGE_MASK) &
> +INITRD_PAGE_MASK;


"INITRD_PAGE_MASK" should be aligned vertically with  "(kernel_high".
Otherwise:

Reviewed-by: Aleksandar Markovic 



>  if (initrd_offset + initrd_size > ram_size) {
>  error_report("memory too small for initial ram disk '%s'",
>   loaderparams.initrd_filename);
> @@ -139,11 +142,13 @@ static int64_t load_kernel(void)
>  params_buf[1] = tswap32(0x12345678);
>
>  if (initrd_size > 0) {
> -snprintf((char *)params_buf + 8, 256, "rd_start=0x%" PRIx64 "
> rd_size=%" PRId64 " %s",
> +snprintf((char *)params_buf + 8, 256,
> + "rd_start=0x%" PRIx64 " rd_size=%" PRId64 " %s",
>   cpu_mips_phys_to_kseg0(NULL, initrd_offset),
>   initrd_size, loaderparams.kernel_cmdline);
>  } else {
> -snprintf((char *)params_buf + 8, 256, "%s",
> loaderparams.kernel_cmdline);
> +snprintf((char *)params_buf + 8, 256,
> +"%s", loaderparams.kernel_cmdline);
>  }
>
>  rom_add_blob_fixed("params", params_buf, params_size,
> @@ -207,15 +212,21 @@ void mips_r4k_init(MachineState *machine)
>
>  memory_region_add_subregion(address_space_mem, 0, ram);
>
> -memory_region_init_io(iomem, NULL, &mips_qemu_ops, NULL, "mips-qemu",
> 0x1);
> +memory_region_init_io(iomem, NULL, &mips_qemu_ops,
> +  NULL, "mips-qemu", 0x1);
> +
>  memory_region_add_subregion(address_space_mem, 0x1fbf, iomem);
>
> -/* Try to load a BIOS image. If this fails, we continue regardless,
> -   but initialize the hardware ourselves. When a kernel gets
> -   preloaded we also initialize the hardware, since the BIOS wasn't
> -   run. */
> -if (bios_name == NULL)
> +/*
> + * Try to load a BIOS image. If this fails, we continue regardless,
> + * but initialize the hardware ourselves. When a kernel gets
> + * preloaded we also initialize the hardware, since the BIOS wasn't
> + * run.
> + */
> +
> +if (bios_name == NULL) {
>  bios_name = 

Re: [PATCH 2/2] exec: drop tb_invalidate_phys_addr

[Richard Henderson]

On 11/27/19 10:06 PM, Max Filippov wrote:
> The only remaining user of tb_invalidate_phys_addr is target/xtensa
> instruction breakpoint code and it is better to use tb_flush there.
> 
> Drop tb_invalidate_phys_addr implementations and declarations.
> Use tb_flush in xtensa IBREAK helpers.
> 
> Signed-off-by: Max Filippov 
> ---
>  exec.c | 29 ++---
>  include/exec/exec-all.h|  3 ---
>  target/xtensa/dbg_helper.c | 19 +++
>  3 files changed, 5 insertions(+), 46 deletions(-)

Reviewed-by: Richard Henderson 

Though perhaps split in half, so the xtensa patch comes first.


r~

Re: [PATCH 5/5] mips: fulong 2e: Renovate coding style

[Aleksandar Markovic]

On Monday, November 25, 2019, Filip Bozuta  wrote:

> The script checkpatch.pl located in scripts folder was
> used to detect all errors and warrnings in files:
> hw/mips/mips_fulong2e.c
> hw/isa/vt82c686.c
> hw/pci-host/bonito.c
> include/hw/isa/vt82c686.h
>
> These mips Fulong 2E machine files were edited and
> all the errors and warrings generated by the checkpatch.pl
> script were corrected and then the script was
> ran again to make sure there are no more errors and warnings.
>
> Signed-off-by: Filip Bozuta 
> ---
>  hw/isa/vt82c686.c| 23 ++--
>  hw/pci-host/bonito.c | 60 +-
> --
>  2 files changed, 45 insertions(+), 38 deletions(-)
>
>
Excellent!

Reviewed-by: Aleksandar Markovic 



> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 616f67f..f828708 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -27,7 +27,7 @@
>  #include "qemu/timer.h"
>  #include "exec/address-spaces.h"
>
> -//#define DEBUG_VT82C686B
> +/* #define DEBUG_VT82C686B */
>
>  #ifdef DEBUG_VT82C686B
>  #define DPRINTF(fmt, ...) fprintf(stderr, "%s: " fmt, __func__,
> ##__VA_ARGS__)
> @@ -35,8 +35,7 @@
>  #define DPRINTF(fmt, ...)
>  #endif
>
> -typedef struct SuperIOConfig
> -{
> +typedef struct SuperIOConfig {
>  uint8_t config[0x100];
>  uint8_t index;
>  uint8_t data;
> @@ -102,7 +101,7 @@ static uint64_t superio_ioport_readb(void *opaque,
> hwaddr addr, unsigned size)
>  SuperIOConfig *superio_conf = opaque;
>
>  DPRINTF("superio_ioport_readb  address 0x%x\n", addr);
> -return (superio_conf->config[superio_conf->index]);
> +return superio_conf->config[superio_conf->index];
>  }
>
>  static const MemoryRegionOps superio_ops = {
> @@ -143,7 +142,7 @@ static void vt82c686b_isa_reset(DeviceState *dev)
>  }
>
>  /* write config pci function0 registers. PCI-ISA bridge */
> -static void vt82c686b_write_config(PCIDevice * d, uint32_t address,
> +static void vt82c686b_write_config(PCIDevice *d, uint32_t address,
> uint32_t val, int len)
>  {
>  VT82C686BState *vt686 = VT82C686B_DEVICE(d);
> @@ -365,7 +364,7 @@ static void vt82c686b_pm_realize(PCIDevice *dev, Error
> **errp)
>  pci_set_long(pci_conf + 0x48, 0x0001);
>
>  /* SMB ports:0xeee0~0xeeef */
> -s->smb_io_base =((s->smb_io_base & 0xfff0) + 0x0);
> +s->smb_io_base = ((s->smb_io_base & 0xfff0) + 0x0);
>  pci_conf[0x90] = s->smb_io_base | 1;
>  pci_conf[0x91] = s->smb_io_base >> 8;
>  pci_conf[0xd2] = 0x90;
> @@ -462,16 +461,18 @@ static void vt82c686b_realize(PCIDevice *d, Error
> **errp)
>
>  wmask = d->wmask;
>  for (i = 0x00; i < 0xff; i++) {
> -   if (i<=0x03 || (i>=0x08 && i<=0x3f)) {
> -   wmask[i] = 0x00;
> -   }
> +if (i <= 0x03 || (i >= 0x08 && i <= 0x3f)) {
> +wmask[i] = 0x00;
> +}
>  }
>
>  memory_region_init_io(&vt82c->superio, OBJECT(d), &superio_ops,
>&vt82c->superio_conf, "superio", 2);
>  memory_region_set_enabled(&vt82c->superio, false);
> -/* The floppy also uses 0x3f0 and 0x3f1.
> - * But we do not emulate a floppy, so just set it here. */
> +/*
> + * The floppy also uses 0x3f0 and 0x3f1.
> + * But we do not emulate a floppy, so just set it here.
> + */
>  memory_region_add_subregion(isa_bus->address_space_io, 0x3f0,
>  &vt82c->superio);
>  }
> diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c
> index ceee463..4692d41 100644
> --- a/hw/pci-host/bonito.c
> +++ b/hw/pci-host/bonito.c
> @@ -14,7 +14,8 @@
>   * fulong 2e mini pc has a bonito north bridge.
>   */
>
> -/* what is the meaning of devfn in qemu and IDSEL in bonito northbridge?
> +/*
> + * what is the meaning of devfn in qemu and IDSEL in bonito northbridge?
>   *
>   * devfn   pci_slot<<3  + funno
>   * one pci bus can have 32 devices and each device can have 8 functions.
> @@ -49,7 +50,7 @@
>  #include "sysemu/runstate.h"
>  #include "exec/address-spaces.h"
>
> -//#define DEBUG_BONITO
> +/* #define DEBUG_BONITO */
>
>  #ifdef DEBUG_BONITO
>  #define DPRINTF(fmt, ...) fprintf(stderr, "%s: " fmt, __func__,
> ##__VA_ARGS__)
> @@ -60,45 +61,45 @@
>  /* from linux soure code. include/asm-mips/mips-boards/bonito64.h*/
>  #define BONITO_BOOT_BASE0x1fc0
>  #define BONITO_BOOT_SIZE0x0010
> -#define BONITO_BOOT_TOP (BONITO_BOOT_BASE+BONITO_BOOT_SIZE-1)
> +#define BONITO_BOOT_TOP (BONITO_BOOT_BASE + BONITO_BOOT_SIZE - 1)
>  #define BONITO_FLASH_BASE   0x1c00
>  #define BONITO_FLASH_SIZE   0x0300
> -#define BONITO_FLASH_TOP(BONITO_FLASH_BASE+BONITO_FLASH_SIZE-1)
> +#define BONITO_FLASH_TOP(BONITO_FLASH_BASE + BONITO_FLASH_SIZE -
> 1)
>  #define BONITO_SOCKET_BASE  0x1f80
>  #define BONITO_SOCKET_SIZE  0x0040
> -#define BONITO_SOCKET_TOP   (BONITO_SOCKET_BASE+BONITO_SO

Re: [PATCH 1/2] exec: flush CPU TB cache in breakpoint_invalidate

[Richard Henderson]

On 11/27/19 10:06 PM, Max Filippov wrote:
> When a breakpoint is inserted at location for which there's currently no
> virtual to physical translation no action is taken on CPU TB cache. If a
> TB for that virtual address already exists but is not visible ATM the
> breakpoint won't be hit next time an instruction at that address will be
> executed.
> 
> Flush entire CPU TB cache in breakpoint_invalidate to force
> re-translation of all TBs for the breakpoint address.
> 
> This change fixes the following scenario:
> - linux user application is running
> - a breakpoint is inserted from QEMU gdbstub for a user address that is
>   not currently present in the target CPU TLB
> - an instruction at that address is executed, but the external debugger
>   doesn't get control.
> 
> Signed-off-by: Max Filippov 
> ---
> Changes RFC->v1:
> - do tb_flush in breakpoint_invalidate unconditionally

Unlike Paolo, I don't think this is a good idea.

If I was going to change anything here, I'd change this to not use
cpu_get_phys_page_attrs_debug but using the caching available from the actual
cputlb, using cc->tlb_fill() in probe mode -- something akin to probe_access(),
but not returning a host address, nor handling watchpoints nor notdirty.

This would help flushing too much by distinguishing different tbs for the same
virtual address mapping to a different physical address.


r~

Re: [RFC] exec: flush CPU TB cache when breakpoint address translation fails

[Richard Henderson]

On 11/26/19 10:26 PM, Max Filippov wrote:
> When a breakpoint is inserted at location for which there's currently no
> virtual to physical translation no action is taken on CPU TB cache. If a
> TB for that virtual address already exists but is not visible ATM the
> breakpoint won't be hit next time an instruction at that address will be
> executed.
> 
> Flush entire CPU TB cache when a breakpoint is set for a virtual address
> that cannot be translated to physical address.
> 
> This change fixes the following workflow:
> - linux user application is running
> - a breakpoint is inserted from QEMU gdbstub for a user address that is
>   not currently present in the target CPU TLB
> - an instruction at that address is executed, but the external debugger
>   doesn't get control.
> 
> Signed-off-by: Max Filippov 

As a short-term fix this works.  I'm willing to apply this to 5.0 with cc:
stable so that as a bug fix this gets pulled back to 4.2.1.  Paolo's request
for a comment is en pointe, so I'll wait for a version 2.

However, while running in system mode with a decent sized amount of guest ram,
I see 1-2M tbs live at once.  We don't really want to throw all of those away.

I think it would be better to put the tbs into some sort of data structure that
indexes the tbs by their virtual address.  I have no strong feelings about what
sort of data structure would be best, just something better than a linear
search of all of the tbs.  :-)


r~

Re: [PATCH] target/arm: Allow loading elf from aliased ROM regions

[Peter Maydell]

On Sun, 1 Dec 2019 at 22:50, Jean-Hugues Deschênes
 wrote:
> > My preference for fixing this properly is:
> >  * get Damien's three-phase-reset patchset into master
> >  * make the ROM blob loader write its data into ram
> >in phase 2 ('hold')
> > * make the arm CPU reset read the data in phase 3 ('exit')

> Makes perfect sense. Feel free to drop the patch.

Well, I'm still vaguely tracking this patch; we might want
a temporary-fix if it looks like the phased-reset approach
is going to take too long to get into master.

thanks
-- PMM

RE: [PATCH] target/arm: Allow loading elf from aliased ROM regions

[Jean-Hugues Deschênes]

> No. This is a reset ordering problem. The CPU reset happens before the 
> 'rom blob loader' reset, so at this point the rom data (usually an ELF file
> segment) has not been written into ram, and doing an 
> address_space_read() will just read zeroes. This is also why the aliasing
> issue happens at all -- the rom blob is at a particular address, but if the
> address we use here to try to read the data is an aliased variant of it
> then rom_ptr() does the wrong thing.
>
> My preference for fixing this properly is:
>  * get Damien's three-phase-reset patchset into master
>  * make the ROM blob loader write its data into ram
>in phase 2 ('hold')
> * make the arm CPU reset read the data in phase 3 ('exit')
>
> This last matches better what the hardware does -- the M-profile CPU
> reads the vector table in the first couple of clock cycles when it *leaves*
> reset, not while the CPU is being *held* in reset. This kind of thing is
> always awkward to model in an emulator, especially if you were hoping
> to also handle allowing the PC to be set from an ELF file entrypoint or
> by the user in the gdbstub on startup...

Makes perfect sense. Feel free to drop the patch.

Thanks,

Jh

Re: [PATCH] target/arm: Allow loading elf from aliased ROM regions

[Peter Maydell]

On Sun, 1 Dec 2019 at 20:13, Richard Henderson
 wrote:
>
> On 11/25/19 12:41 PM, Jean-Hugues Deschênes wrote:
> >  initial_msp = ldl_p(rom);
> >  initial_pc = ldl_p(rom + 4);
> >  } else {
> > -/* Address zero not covered by a ROM blob, or the ROM blob
> > - * is in non-modifiable memory and this is a second reset after
> > - * it got copied into memory. In the latter case, rom_ptr
> > - * will return a NULL pointer and we should use ldl_phys 
> > instead.
> > - */
> > -initial_msp = ldl_phys(s->as, vecbase);
> > -initial_pc = ldl_phys(s->as, vecbase + 4);
> > +/* See if the ROM blob is aliased somewhere */
> > +hwaddr len = 0, xlat = 0;
> > +MemoryRegion *mr = address_space_translate(s->as, vecbase, 
> > &xlat,
> > +&len, false, MEMTXATTRS_UNSPECIFIED);
> > +
> > +if (mr) {
> > +rom = rom_ptr(mr->addr + xlat, 8);
> > +} else {
> > +rom = NULL;
> > +}
> > +
> > +if (rom) {
> > +initial_msp = ldl_p(rom);
> > +initial_pc = ldl_p(rom + 4);
> > +} else {
> > +
> > +/*
> > + * Address zero not covered by a ROM blob, or the ROM blob
> > + * is in non-modifiable memory and this is a second reset 
> > after
> > + * it got copied into memory. In the latter case, rom_ptr
> > + * will return a NULL pointer and we should use ldl_phys
> > + * instead.
> > + */
> > +initial_msp = ldl_phys(s->as, vecbase);
> > +initial_pc = ldl_phys(s->as, vecbase + 4);
> > +}
> >  }
>
> Can this entire section, including the rom_ptr thing just above, be replaced
> with two address_space_read()?

No. This is a reset ordering problem. The CPU reset happens
before the 'rom blob loader' reset, so at this point the
rom data (usually an ELF file segment) has not been written
into ram, and doing an address_space_read() will just read
zeroes. This is also why the aliasing issue happens at all --
the rom blob is at a particular address, but if the address
we use here to try to read the data is an aliased variant
of it then rom_ptr() does the wrong thing.

My preference for fixing this properly is:
 * get Damien's three-phase-reset patchset into master
 * make the ROM blob loader write its data into ram
   in phase 2 ('hold')
 * make the arm CPU reset read the data in phase 3 ('exit')

This last matches better what the hardware does -- the
M-profile CPU reads the vector table in the first couple
of clock cycles when it *leaves* reset, not while the
CPU is being *held* in reset. This kind of thing is
always awkward to model in an emulator, especially if
you were hoping to also handle allowing the PC to be
set from an ELF file entrypoint or by the user in the
gdbstub on startup...

thanks
-- PMM

Re: [RFC net-next 08/18] tun: run offloaded XDP program in Tx path

[David Miller]

From: "Michael S. Tsirkin" 
Date: Sun, 1 Dec 2019 16:40:22 -0500

> Right. But it is helpful to expose the supported functionality
> to guest in some way, if nothing else then so that
> guests can be moved between different hosts.
> 
> Also, we need a way to report this kind of event to guest
> so it's possible to figure out what went wrong.

On the contrary, this is why it is of utmost importance that all
XDP implementations support the full suite of XDP facilities from
the very beginning.

This is why we keep giving people a hard time when they add support
only for some of the XDP return values and semantics.  Users will get
killed by this, and it makes XDP a poor technology to use because
behavior is not consistent across device types.

That's not acceptable and I'll push back on anything that continues
this trend.

If you can't HW offload it, kick it to software.

Re: [PATCH] configure: Use lld --image-base for --disable-pie user mode binaries

[Richard Henderson]

On 11/27/19 6:36 PM, Fangrui Song wrote:
> On 2019-11-20, Fangrui Song wrote:
>> On 2019-11-15, Fangrui Song wrote:
>>> For lld, --image-base is the preferred way to set the base address.
>>> lld does not actually implement -Ttext-segment, but treats it as an alias 
>>> for
>>> -Ttext. -Ttext-segment=0x6000 combined with --no-rosegment can
>>> create a 1.6GB executable.
>>>
>>> Fix the problem by using --image-base for lld. GNU ld and gold will
>>> still get -Ttext-segment. Also delete the ld --verbose fallback introduced
>>> in 2013, which is no longer relevant or correct (the default linker
>>> script has changed).
>>>
>>> Signed-off-by: Fangrui Song 
>>> ---
>>> configure | 33 -
>>> 1 file changed, 12 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/configure b/configure
>>> index 6099be1d84..2d45af0d09 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -6336,43 +6336,34 @@ fi
>>>
>>> # Probe for the need for relocating the user-only binary.
>>> if ( [ "$linux_user" = yes ] || [ "$bsd_user" = yes ] ) && [ "$pie" = no ];
>>> then
>>> -  textseg_addr=
>>> +  image_base=
>>>   case "$cpu" in
>>>     arm | i386 | ppc* | s390* | sparc* | x86_64 | x32)
>>> -  # ??? Rationale for choosing this address
>>> -  textseg_addr=0x6000
>>> +  # An arbitrary address that makes it unlikely to collide with user
>>> +  # programs.

Please don't replace this ??? with an arbitrary rationale, which clearly
doesn't apply to all of these hosts.

>>> +  image_base=0x6000
>>>   ;;
>>>     mips)
>>>   # A 256M aligned address, high in the address space, with enough
>>>   # room for the code_gen_buffer above it before the stack.

This is the only one with a proper rationale.

That said, I'm not sure that the proper way to handle this issue with lld is to
drop this code entirely.

The best way to handle the underlying issue -- address conflict between
interpreter and guest binary -- is PIE, for which this code is skipped.

After that, we go to some pain to choose a guest_base address that allows the
guest binary to load around the interpreter's reserved addresses.

So what's left that this messing about with link addresses buys us?


r~

Re: [RFC net-next 08/18] tun: run offloaded XDP program in Tx path

[Michael S. Tsirkin]

On Sun, Dec 01, 2019 at 12:56:21PM -0800, David Miller wrote:
> From: David Ahern 
> Date: Sun, 1 Dec 2019 09:39:54 -0700
> 
> > Below you just drop the packet which is going to be a bad user
> > experience. A better user experience is to detect XDP return codes a
> > program uses, catch those that are not supported for this use case and
> > fail the install of the program.
> 
> This is not universally possible.
> 
> Return codes can be calculated dynamically, come from maps potentially
> shared with other bpf programs, etc.
> 
> So unfortunately this suggestion is not tenable.

Right. But it is helpful to expose the supported functionality
to guest in some way, if nothing else then so that
guests can be moved between different hosts.

Also, we need a way to report this kind of event to guest
so it's possible to figure out what went wrong.

-- 
MST

Re: [RFC net-next 08/18] tun: run offloaded XDP program in Tx path

[David Miller]

From: David Ahern 
Date: Sun, 1 Dec 2019 09:39:54 -0700

> Below you just drop the packet which is going to be a bad user
> experience. A better user experience is to detect XDP return codes a
> program uses, catch those that are not supported for this use case and
> fail the install of the program.

This is not universally possible.

Return codes can be calculated dynamically, come from maps potentially
shared with other bpf programs, etc.

So unfortunately this suggestion is not tenable.

Re: [PATCH 2/5] mips: malta: Renovate coding style

[Philippe Mathieu-Daudé]

Hi Filip,

On 11/25/19 2:04 PM, Filip Bozuta wrote:

The script checkpatch.pl located in scripts folder was
used to detect all errors and warrnings in files:
 hw/mips/mips_malta.c
 hw/mips/gt64xxx_pci.c
 tests/acceptance/linux_ssh_mips_malta.py

All these mips malta machine files were edited and
all the errors and warrings generated by the checkpatch.pl
script were corrected and then the script was
ran again to make sure there are no more errors and warnings.

Signed-off-by: Filip Bozuta 
---
  hw/mips/mips_malta.c | 139 ---
  tests/acceptance/linux_ssh_mips_malta.py |   6 +-
  2 files changed, 75 insertions(+), 70 deletions(-)

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 92e9ca5..18eafac 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -137,7 +137,8 @@ static void malta_fpga_update_display(void *opaque)
   */
  
  #if defined(DEBUG)

-#  define logout(fmt, ...) fprintf(stderr, "MALTA\t%-24s" fmt, __func__, ## 
__VA_ARGS__)
+#  define logout(fmt, ...) \
+  fprintf(stderr, "MALTA\t%-24s" fmt, __func__, ## __VA_ARGS__)


This is deprecated code, if you look in the repository history, we 
usually don't touch it, except to get rid of it. The way to get rid of 
it is to replace the calls by trace events.



  #else
  #  define logout(fmt, ...) ((void)0)
  #endif
@@ -569,7 +570,7 @@ static MaltaFPGAState *malta_fpga_init(MemoryRegion 
*address_space,
  MaltaFPGAState *s;
  Chardev *chr;
  
-s = (MaltaFPGAState *)g_malloc0(sizeof(MaltaFPGAState));

+s = (MaltaFPGAState *)g_new0(sizeof(MaltaFPGAState));


This change doesn't even compile... Please compile your patches before 
posting them to the list.


You can find the prototype of this macro here:
https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#g-new0

  
  memory_region_init_io(&s->iomem, NULL, &malta_fpga_ops, s,

"malta-fpga", 0x10);
@@ -844,9 +845,9 @@ static void write_bootloader(uint8_t *base, int64_t 
run_addr,
  /* Small bootloader */
  p = (uint32_t *)base;
  
-stl_p(p++, 0x0800 |  /* j 0x1fc00580 */

+stl_p(p++, 0x0800 |   /* j 0x1fc00580 
*/
   ((run_addr + 0x580) & 0x0fff) >> 2);
-stl_p(p++, 0x);  /* nop */
+stl_p(p++, 0x);   /* nop */
  
  /* YAMON service vector */

  stl_p(base + 0x500, run_addr + 0x0580);  /* start: */
@@ -892,104 +893,106 @@ static void write_bootloader(uint8_t *base, int64_t 
run_addr,
  stl_p(p++, 0x34e7 | (loaderparams.ram_low_size & 0x));
  
  /* Load BAR registers as done by YAMON */

-stl_p(p++, 0x3c09b400);  /* lui t1, 
0xb400 */
+stl_p(p++, 0x3c09b400); /* lui t1, 0xb400 */
  
  #ifdef TARGET_WORDS_BIGENDIAN

-stl_p(p++, 0x3c08df00);  /* lui t0, 
0xdf00 */
+stl_p(p++, 0x3c08df00);  /* lui t0, 0xdf00 */
  #else
-stl_p(p++, 0x340800df);  /* ori t0, 
r0, 0x00df */
+stl_p(p++, 0x340800df);  /* ori t0, r0, 0x00df */
  #endif
-stl_p(p++, 0xad280068);  /* sw t0, 
0x0068(t1) */
+stl_p(p++, 0xad280068);  /* sw t0, 0x0068(t1) */
  
-stl_p(p++, 0x3c09bbe0);  /* lui t1, 0xbbe0 */

+stl_p(p++, 0x3c09bbe0);  /* lui t1, 0xbbe0 */
  
  #ifdef TARGET_WORDS_BIGENDIAN

-stl_p(p++, 0x3c08c000);  /* lui t0, 
0xc000 */
+stl_p(p++, 0x3c08c000);  /* lui t0, 0xc000 */
  #else
-stl_p(p++, 0x340800c0);  /* ori t0, 
r0, 0x00c0 */
+stl_p(p++, 0x340800c0);  /* ori t0, r0, 0x00c0 */
  #endif
-stl_p(p++, 0xad280048);  /* sw t0, 
0x0048(t1) */
+stl_p(p++, 0xad280048);  /* sw t0, 0x0048(t1) */
  #ifdef TARGET_WORDS_BIGENDIAN
-stl_p(p++, 0x3c084000);  /* lui t0, 
0x4000 */
+stl_p(p++, 0x3c084000);  /* lui t0, 0x4000 */
  #else
-stl_p(p++, 0x34080040);  /* ori t0, 
r0, 0x0040 */
+stl_p(p++, 0x34080040);  /* ori t0, r0, 0x0040 */
  #endif
-stl_p(p++, 0xad280050);  /* sw t0, 
0x0050(t1) */
+stl_p(p++, 0xad280050);  /* sw t0, 0x0050(t1) */
  
  #ifdef TARGET_WORDS_BIGENDIAN

-stl_p(p++, 0x3c088000);  /* lui t0, 
0x8000 */
+stl_p(p++, 0x3c088000);  /* lui t0, 0x8000 */
  #else
-stl_p(p++, 0x34080080);  /* ori t0, 
r

Re: [PATCH] target/arm: Allow loading elf from aliased ROM regions

[Richard Henderson]

On 11/25/19 12:41 PM, Jean-Hugues Deschênes wrote:
>  initial_msp = ldl_p(rom);
>  initial_pc = ldl_p(rom + 4);
>  } else {
> -/* Address zero not covered by a ROM blob, or the ROM blob
> - * is in non-modifiable memory and this is a second reset after
> - * it got copied into memory. In the latter case, rom_ptr
> - * will return a NULL pointer and we should use ldl_phys instead.
> - */
> -initial_msp = ldl_phys(s->as, vecbase);
> -initial_pc = ldl_phys(s->as, vecbase + 4);
> +/* See if the ROM blob is aliased somewhere */
> +hwaddr len = 0, xlat = 0;
> +MemoryRegion *mr = address_space_translate(s->as, vecbase, &xlat,
> +&len, false, MEMTXATTRS_UNSPECIFIED);
> +
> +if (mr) {
> +rom = rom_ptr(mr->addr + xlat, 8);
> +} else {
> +rom = NULL;
> +}
> +
> +if (rom) {
> +initial_msp = ldl_p(rom);
> +initial_pc = ldl_p(rom + 4);
> +} else {
> +
> +/*
> + * Address zero not covered by a ROM blob, or the ROM blob
> + * is in non-modifiable memory and this is a second reset 
> after
> + * it got copied into memory. In the latter case, rom_ptr
> + * will return a NULL pointer and we should use ldl_phys
> + * instead.
> + */
> +initial_msp = ldl_phys(s->as, vecbase);
> +initial_pc = ldl_phys(s->as, vecbase + 4);
> +}
>  }

Can this entire section, including the rom_ptr thing just above, be replaced
with two address_space_read()?


r~

Re: [PATCH v2 06/14] target/arm: use gdb_get_reg helpers

[Philippe Mathieu-Daudé]

On 11/30/19 9:45 AM, Alex Bennée wrote:

This is cleaner than poking memory directly and will make later
clean-ups easier.

Signed-off-by: Alex Bennée 

---
v2
   - make sure we pass hi/lo correctly as quads are stored in LE order
---
  target/arm/helper.c | 18 +++---
  1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0bf8f53d4b8..0ac950d6c71 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -105,21 +105,17 @@ static int aarch64_fpu_gdb_get_reg(CPUARMState *env, 
uint8_t *buf, int reg)
  {
  switch (reg) {
  case 0 ... 31:
-/* 128 bit FP register */
-{
-uint64_t *q = aa64_vfp_qreg(env, reg);
-stq_le_p(buf, q[0]);
-stq_le_p(buf + 8, q[1]);
-return 16;
-}
+{
+/* 128 bit FP register - quads are in LE order */


Oh, this was always wrong on BE :(

Reviewed-by: Philippe Mathieu-Daudé 


+uint64_t *q = aa64_vfp_qreg(env, reg);
+return gdb_get_reg128(buf, q[1], q[0]);
+}
  case 32:
  /* FPSR */
-stl_p(buf, vfp_get_fpsr(env));
-return 4;
+return gdb_get_reg32(buf, vfp_get_fpsr(env));
  case 33:
  /* FPCR */
-stl_p(buf, vfp_get_fpcr(env));
-return 4;
+return gdb_get_reg32(buf,vfp_get_fpcr(env));
  default:
  return 0;
  }

Re: [PATCH v2 05/14] gdbstub: add helper for 128 bit registers

[Philippe Mathieu-Daudé]

On 11/30/19 9:45 AM, Alex Bennée wrote:

Signed-off-by: Alex Bennée 

---
v2
   - take care of endianess of the whole 128 bit word
---
  include/exec/gdbstub.h | 13 +
  1 file changed, 13 insertions(+)

diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 08363969c14..59e366ba3af 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -102,6 +102,19 @@ static inline int gdb_get_reg64(uint8_t *mem_buf, uint64_t 
val)
  return 8;
  }
  
+static inline int gdb_get_reg128(uint8_t *mem_buf, uint64_t val_hi,

+ uint64_t val_lo)
+{
+#ifdef TARGET_WORDS_BIGENDIAN
+stq_p(mem_buf, val_hi);
+stq_p(mem_buf + 8, val_lo);
+#else
+stq_p(mem_buf, val_lo);
+stq_p(mem_buf + 8, val_hi);
+#endif
+return 16;
+}
+
  #if TARGET_LONG_BITS == 64
  #define gdb_get_regl(buf, val) gdb_get_reg64(buf, val)
  #define ldtul_p(addr) ldq_p(addr)



Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 04/26] qdev: move helper function to monitor/misc

[Philippe Mathieu-Daudé]

On 12/1/19 12:15 PM, Marc-André Lureau wrote:

Move the one-user function to the place it is being used.

Signed-off-by: Marc-André Lureau 
---
  hw/core/qdev.c | 26 --
  include/hw/qdev-core.h |  2 --
  monitor/misc.c | 26 ++
  3 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index c1a338bbe4..90eb01bc8e 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -760,32 +760,6 @@ void qdev_alias_all_properties(DeviceState *target, Object 
*source)
  } while (class != object_class_by_name(TYPE_DEVICE));
  }
  
-static int qdev_add_hotpluggable_device(Object *obj, void *opaque)

-{
-GSList **list = opaque;
-DeviceState *dev = (DeviceState *)object_dynamic_cast(OBJECT(obj),
-  TYPE_DEVICE);
-
-if (dev == NULL) {
-return 0;
-}
-
-if (dev->realized && object_property_get_bool(obj, "hotpluggable", NULL)) {
-*list = g_slist_append(*list, dev);
-}
-
-return 0;
-}
-
-GSList *qdev_build_hotpluggable_device_list(Object *peripheral)
-{
-GSList *list = NULL;
-
-object_child_foreach(peripheral, qdev_add_hotpluggable_device, &list);
-
-return list;
-}
-
  static bool device_get_realized(Object *obj, Error **errp)
  {
  DeviceState *dev = DEVICE(obj);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 1518495b1e..6b0e7b265d 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -457,8 +457,6 @@ extern bool qdev_hot_removed;
  
  char *qdev_get_dev_path(DeviceState *dev);
  
-GSList *qdev_build_hotpluggable_device_list(Object *peripheral);

-
  void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error **errp);
  
  void qbus_set_bus_hotplug_handler(BusState *bus, Error **errp);

diff --git a/monitor/misc.c b/monitor/misc.c
index 3baa15f3bf..36c85cecf3 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -1956,6 +1956,32 @@ void object_add_completion(ReadLineState *rs, int 
nb_args, const char *str)
  g_slist_free(list);
  }
  
+static int qdev_add_hotpluggable_device(Object *obj, void *opaque)

+{
+GSList **list = opaque;
+DeviceState *dev = (DeviceState *)object_dynamic_cast(OBJECT(obj),
+  TYPE_DEVICE);
+
+if (dev == NULL) {
+return 0;
+}
+
+if (dev->realized && object_property_get_bool(obj, "hotpluggable", NULL)) {
+*list = g_slist_append(*list, dev);
+}
+
+return 0;
+}
+
+static GSList *qdev_build_hotpluggable_device_list(Object *peripheral)
+{
+GSList *list = NULL;
+
+object_child_foreach(peripheral, qdev_add_hotpluggable_device, &list);
+
+return list;
+}
+
  static void peripheral_device_del_completion(ReadLineState *rs,
   const char *str, size_t len)
  {



Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 01/26] object: add extra sanity checks

[Philippe Mathieu-Daudé]

On 12/1/19 12:15 PM, Marc-André Lureau wrote:

Type system checked that children class_size >= parent class_size, but
not instances. Fix that.

Signed-off-by: Marc-André Lureau 
---
  qom/object.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/qom/object.c b/qom/object.c
index d51b57fba1..935491d334 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -303,6 +303,7 @@ static void type_initialize(TypeImpl *ti)
  int i;
  
  g_assert(parent->class_size <= ti->class_size);

+g_assert(parent->instance_size <= ti->instance_size);
  memcpy(ti->class, parent->class, parent->class_size);
  ti->class->interfaces = NULL;
  ti->class->properties = g_hash_table_new_full(



Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v6 00/37] Clean-ups: qom-ify serial and remove QDEV_PROP_PTR

[Aleksandar Markovic]

On Sunday, December 1, 2019, Aleksandar Markovic <
aleksandar.m.m...@gmail.com> wrote:

>
>
> On Sunday, December 1, 2019, Marc-André Lureau 
> wrote:
>
>> Hi Aleksandar
>>
>> On Sun, Dec 1, 2019 at 4:15 PM Aleksandar Markovic
>>  wrote:
>> >
>> >
>> >
>> > On Sunday, December 1, 2019, Marc-André Lureau <
>> marcandre.lur...@gmail.com> wrote:
>> >
>> >>
>> >> - "RFC: mips/cps: fix setting saar property"
>> >>
>> >> Perhaps I should have used FIX instead of RFC, because this should
>> >> actually be a real fix. However I could use someone help to exercise
>> >> the code path.
>> >>
>> >
>> > Marc-André, hi.
>> >
>> > There is a work in progress on fixing this. Can we in MIPS submit the
>> fix independently, since it involves some additional pieces of code that
>> are really deeply mips-specific? We acknowledge the bug, and want to
>> develop the real solution. Can you simply skip this RFC patch in your
>> series, since the issues will be handled separately in our patch, hopefully
>> soon after the merge window is open?
>> >
>> > For all other mips parts of your series, you have my "reviewed-by"s ,
>> in case I forgot to send them explicitely.
>> >
>>
>> This is a one-liner, and it is required to achieve the goal of the
>> series, to remove PROP_PTR.
>>
>> If you prefer, I can instead comment the line with a FIXME, since it
>> is apparently broken anyway?
>>
>> If you manage to get your fix merged earlier, then this patch can be
>> dropped. Else, is it a problem for the later fixes?
>>
>>
> OK, Marc-André,
>
> Please go ahead with this patch, so that the goal of the series is
> achieved, and we will later submitt a wider patch that will address the
> root cause. Just remove RFC from subject, everything else looks fine to me.
> You can add my "reviewed-by".
>
>
I mean, yes, you are right, it is broken, with or without the patch, so go
ahead, at least your series will fulfill its purpose, and I'll have enough
time to integrate the fix later on, without interfering each other.

Thanks for the series!


Yours, Aleksandar
>
>
>
>
>> thanks
>>
>>
>> --
>> Marc-André Lureau
>>
>

Re: [PATCH v6 00/37] Clean-ups: qom-ify serial and remove QDEV_PROP_PTR

[Aleksandar Markovic]

On Sunday, December 1, 2019, Marc-André Lureau 
wrote:

> Hi Aleksandar
>
> On Sun, Dec 1, 2019 at 4:15 PM Aleksandar Markovic
>  wrote:
> >
> >
> >
> > On Sunday, December 1, 2019, Marc-André Lureau <
> marcandre.lur...@gmail.com> wrote:
> >
> >>
> >> - "RFC: mips/cps: fix setting saar property"
> >>
> >> Perhaps I should have used FIX instead of RFC, because this should
> >> actually be a real fix. However I could use someone help to exercise
> >> the code path.
> >>
> >
> > Marc-André, hi.
> >
> > There is a work in progress on fixing this. Can we in MIPS submit the
> fix independently, since it involves some additional pieces of code that
> are really deeply mips-specific? We acknowledge the bug, and want to
> develop the real solution. Can you simply skip this RFC patch in your
> series, since the issues will be handled separately in our patch, hopefully
> soon after the merge window is open?
> >
> > For all other mips parts of your series, you have my "reviewed-by"s , in
> case I forgot to send them explicitely.
> >
>
> This is a one-liner, and it is required to achieve the goal of the
> series, to remove PROP_PTR.
>
> If you prefer, I can instead comment the line with a FIXME, since it
> is apparently broken anyway?
>
> If you manage to get your fix merged earlier, then this patch can be
> dropped. Else, is it a problem for the later fixes?
>
>
OK, Marc-André,

Please go ahead with this patch, so that the goal of the series is
achieved, and we will later submitt a wider patch that will address the
root cause. Just remove RFC from subject, everything else looks fine to me.
You can add my "reviewed-by".

Yours, Aleksandar




> thanks
>
>
> --
> Marc-André Lureau
>

Re: [PATCH 11/21] hw/ipmi: Fix latent realize() error handling bugs

[Corey Minyard]

On Sat, Nov 30, 2019 at 08:42:30PM +0100, Markus Armbruster wrote:
> isa_ipmi_bt_realize(), ipmi_isa_realize(), pci_ipmi_bt_realize(), and
> pci_ipmi_kcs_realize() crash when IPMIInterfaceClass method init()
> fails and their @errp argument is null.  First messed up in commit
> 0719029c47 "ipmi: Add an ISA KCS low-level interface", then imitated
> in commit a9b74079cb "ipmi: Add a BT low-level interface" and commit
> 12f983c6aa "ipmi: Add PCI IPMI interfaces".
> 
> The bug can't bite as no caller actually passes null, and none of the
> init() methods can actually fail.  Fix it anyway.

Well, whatever.  It looks correct and is better style.  I've added this
to my tree.

-corey

> 
> Cc: Corey Minyard 
> Signed-off-by: Markus Armbruster 
> ---
>  hw/ipmi/isa_ipmi_bt.c  | 7 +--
>  hw/ipmi/isa_ipmi_kcs.c | 7 +--
>  hw/ipmi/pci_ipmi_bt.c  | 6 --
>  hw/ipmi/pci_ipmi_kcs.c | 6 --
>  4 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
> index 9a87ffd3f0..9fba5ed383 100644
> --- a/hw/ipmi/isa_ipmi_bt.c
> +++ b/hw/ipmi/isa_ipmi_bt.c
> @@ -70,6 +70,7 @@ static void isa_ipmi_bt_lower_irq(IPMIBT *ib)
>  
>  static void isa_ipmi_bt_realize(DeviceState *dev, Error **errp)
>  {
> +Error *err = NULL;
>  ISADevice *isadev = ISA_DEVICE(dev);
>  ISAIPMIBTDevice *iib = ISA_IPMI_BT(dev);
>  IPMIInterface *ii = IPMI_INTERFACE(dev);
> @@ -85,9 +86,11 @@ static void isa_ipmi_bt_realize(DeviceState *dev, Error 
> **errp)
>  iib->bt.bmc->intf = ii;
>  iib->bt.opaque = iib;
>  
> -iic->init(ii, 0, errp);
> -if (*errp)
> +iic->init(ii, 0, &err);
> +if (err) {
> +error_propagate(errp, err);
>  return;
> +}
>  
>  if (iib->isairq > 0) {
>  isa_init_irq(isadev, &iib->irq, iib->isairq);
> diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
> index ca3ea36a3f..cc6bd817f2 100644
> --- a/hw/ipmi/isa_ipmi_kcs.c
> +++ b/hw/ipmi/isa_ipmi_kcs.c
> @@ -69,6 +69,7 @@ static void isa_ipmi_kcs_lower_irq(IPMIKCS *ik)
>  
>  static void ipmi_isa_realize(DeviceState *dev, Error **errp)
>  {
> +Error *err = NULL;
>  ISADevice *isadev = ISA_DEVICE(dev);
>  ISAIPMIKCSDevice *iik = ISA_IPMI_KCS(dev);
>  IPMIInterface *ii = IPMI_INTERFACE(dev);
> @@ -84,9 +85,11 @@ static void ipmi_isa_realize(DeviceState *dev, Error 
> **errp)
>  iik->kcs.bmc->intf = ii;
>  iik->kcs.opaque = iik;
>  
> -iic->init(ii, 0, errp);
> -if (*errp)
> +iic->init(ii, 0, &err);
> +if (err) {
> +error_propagate(errp, err);
>  return;
> +}
>  
>  if (iik->isairq > 0) {
>  isa_init_irq(isadev, &iik->irq, iik->isairq);
> diff --git a/hw/ipmi/pci_ipmi_bt.c b/hw/ipmi/pci_ipmi_bt.c
> index 6ed925a665..ba9cf016b5 100644
> --- a/hw/ipmi/pci_ipmi_bt.c
> +++ b/hw/ipmi/pci_ipmi_bt.c
> @@ -54,6 +54,7 @@ static void pci_ipmi_lower_irq(IPMIBT *ik)
>  
>  static void pci_ipmi_bt_realize(PCIDevice *pd, Error **errp)
>  {
> +Error *err = NULL;
>  PCIIPMIBTDevice *pik = PCI_IPMI_BT(pd);
>  IPMIInterface *ii = IPMI_INTERFACE(pd);
>  IPMIInterfaceClass *iic = IPMI_INTERFACE_GET_CLASS(ii);
> @@ -74,8 +75,9 @@ static void pci_ipmi_bt_realize(PCIDevice *pd, Error **errp)
>  pik->bt.raise_irq = pci_ipmi_raise_irq;
>  pik->bt.lower_irq = pci_ipmi_lower_irq;
>  
> -iic->init(ii, 8, errp);
> -if (*errp) {
> +iic->init(ii, 8, &err);
> +if (err) {
> +error_propagate(errp, err);
>  return;
>  }
>  pci_register_bar(pd, 0, PCI_BASE_ADDRESS_SPACE_IO, &pik->bt.io);
> diff --git a/hw/ipmi/pci_ipmi_kcs.c b/hw/ipmi/pci_ipmi_kcs.c
> index eeba63baa4..99f46152f4 100644
> --- a/hw/ipmi/pci_ipmi_kcs.c
> +++ b/hw/ipmi/pci_ipmi_kcs.c
> @@ -54,6 +54,7 @@ static void pci_ipmi_lower_irq(IPMIKCS *ik)
>  
>  static void pci_ipmi_kcs_realize(PCIDevice *pd, Error **errp)
>  {
> +Error *err = NULL;
>  PCIIPMIKCSDevice *pik = PCI_IPMI_KCS(pd);
>  IPMIInterface *ii = IPMI_INTERFACE(pd);
>  IPMIInterfaceClass *iic = IPMI_INTERFACE_GET_CLASS(ii);
> @@ -74,8 +75,9 @@ static void pci_ipmi_kcs_realize(PCIDevice *pd, Error 
> **errp)
>  pik->kcs.raise_irq = pci_ipmi_raise_irq;
>  pik->kcs.lower_irq = pci_ipmi_lower_irq;
>  
> -iic->init(ii, 8, errp);
> -if (*errp) {
> +iic->init(ii, 8, &err);
> +if (err) {
> +error_propagate(errp, err);
>  return;
>  }
>  pci_register_bar(pd, 0, PCI_BASE_ADDRESS_SPACE_IO, &pik->kcs.io);
> -- 
> 2.21.0
> 

Re: [PATCH v4 00/37] Clean-ups: qom-ify serial and remove QDEV_PROP_PTR

[Peter Maydell]

On Sun, 1 Dec 2019 at 17:27, Marc-André Lureau
 wrote:
>
> Hi
>
> On Sun, Dec 1, 2019 at 9:18 PM Peter Maydell  wrote:
> >
> > On Sun, 1 Dec 2019 at 10:19, Marc-André Lureau
> >  wrote:
> > >
> > > - "serial: register vmsd with DeviceClass"
> > >
> > > This is standard qdev-ification, however it breaks backward migration,
> > > but that's just how qdev_set_legacy_instance_id() works.
> >
> > I don't understand this part. Surely the whole point
> > of setting a legacy instance ID is exactly to preserve
> > migration compatibility? If it doesn't do that then what
> > does setting legacy ID value do?
> >
>
> It works in old->new direction only, because new code can match the
> legacy instance id.
>
> But when going from new->old, the legacy instance id is lost, as it
> uses new 0-based instance_id.

I still don't understand. My mental model of the situation is:

 * in the old (current) version of the code, the instance ID
   is some random thing resulting from what the old code does
 * in the new version of the code, we use qdev_set_legacy_instance_id,
   and so instead of using the ID you'd naturally get as a
   written-from-scratch qdev device, it uses the legacy value
   you pass in
 * thus the device/board in both old and new versions of QEMU
   uses the same value and migration in both directions works

I don't understand why we would ever be using a "new 0-based
instance_id" -- it seems to me that the whole point of setting
a legacy ID value is that we will use it always, and I don't
understand how the board code can know that it's going to be
the target of an old->new migration as opposed to being the
source of a new->old migration such that it can end up with
a different ID value in the latter case.

If qdev_set_legacy_instance_id() doesn't work the way I
think it does above, what *does* it do ?

thanks
-- PMM

Re: [PATCH 0/2] RFC: add -mem-shared option

[Paolo Bonzini]

On 01/12/19 16:40, Marc-André Lureau wrote:
>>> The original idea was to always support one NUMA node, so that you could
>>> do "-numa node,memdev=..." to specify a memory backend with -object.
>>> However, this is not possible anymore since
>>>
>>> if (!mc->cpu_index_to_instance_props ||
>>> !mc->get_default_cpu_node_id) {
>>> error_setg(errp, "NUMA is not supported by this machine-type");
>>> return;
>>> }
>>>
>>> has been added to hw/core/numa.c.
>>>
>>> Therefore, I think instead of -mem-shared we should add a "-m
>>> memdev=..." option.  This option:
>>>
>>> * would be mutually exclusive with both -mem-path
>>>
>>> * would be handled from allocate_system_memory_nonnuma.
>>>
>>> * could be mutually exclusive "-numa node", or could just be mutually
>>> exclusive with "-numa node,memdev=..." (the logical conclusion of that
>>> however would be an undeprecation of "-numa node,mem=...", so that has
>>> to be taken into account as well).
>> I completely agree we could do this.  I just think this misses
>> completely the point of this series, because usability of:
>>
>>   -object memory-backend-file,...,share=on,id=mem -m ...,memdev=mem
>>
>> is not much better than the usability of:
>>
>>   -object memory-backend-file,...,share=on,id=mem -numa node,memdev=mem
>>
> +1
> Perhaps when all RAM allocation will occur through memory-backend,
> "-mem-shared" could be simply an alias to "-global
> memory-backend.shared=on"

Yes, this is the point.  There are two parts in this series:

(1) allowing use of vhost-user on non-NUMA machines

(2) providing syntactic sugar for it

I have no problem with -mem-shared for (2), but it should just be
syntactic sugar for (1).  It's okay if -mem-shared is a global variable
rather than an alias for -global; the important part is not to add any
feature that is not available from the QOM-style command line options.

Paolo

Re: [PATCH v4 00/37] Clean-ups: qom-ify serial and remove QDEV_PROP_PTR

[Marc-André Lureau]

Hi

On Sun, Dec 1, 2019 at 9:18 PM Peter Maydell  wrote:
>
> On Sun, 1 Dec 2019 at 10:19, Marc-André Lureau
>  wrote:
> >
> > - "serial: register vmsd with DeviceClass"
> >
> > This is standard qdev-ification, however it breaks backward migration,
> > but that's just how qdev_set_legacy_instance_id() works.
>
> I don't understand this part. Surely the whole point
> of setting a legacy instance ID is exactly to preserve
> migration compatibility? If it doesn't do that then what
> does setting legacy ID value do?
>

It works in old->new direction only, because new code can match the
legacy instance id.

But when going from new->old, the legacy instance id is lost, as it
uses new 0-based instance_id.

This is just the way we have done so far, but I wish to be corrected
if I am wrong.

See the commit description for a bit more details.


-- 
Marc-André Lureau

Re: [PATCH v4 00/37] Clean-ups: qom-ify serial and remove QDEV_PROP_PTR

[Peter Maydell]

On Sun, 1 Dec 2019 at 10:19, Marc-André Lureau
 wrote:
>
> - "serial: register vmsd with DeviceClass"
>
> This is standard qdev-ification, however it breaks backward migration,
> but that's just how qdev_set_legacy_instance_id() works.

I don't understand this part. Surely the whole point
of setting a legacy instance ID is exactly to preserve
migration compatibility? If it doesn't do that then what
does setting legacy ID value do?

thanks
-- PMM

Re: [RFC net-next 00/18] virtio_net XDP offload

[David Ahern]

On 11/27/19 10:18 PM, Jason Wang wrote:
> We try to follow what NFP did by starting from a fraction of the whole
> eBPF features. It would be very hard to have all eBPF features
> implemented from the start.  It would be helpful to clarify what's the
> minimal set of features that you want to have from the start.

Offloading guest programs needs to prevent a guest XDP program from
running bpf helpers that access host kernel data. e.g., bpf_fib_lookup

Re: [RFC net-next 07/18] tun: set offloaded xdp program

[David Ahern]

On 11/26/19 4:07 AM, Prashant Bhole wrote:
> From: Jason Wang 
> 
> This patch introduces an ioctl way to set an offloaded XDP program
> to tun driver. This ioctl will be used by qemu to offload XDP program
> from virtio_net in the guest.
> 

Seems like you need to set / reset the SOCK_XDP flag on tfile->sk since
this is an XDP program.

Also, why not add this program using netlink instead of ioctl? e.g., as
part of a generic XDP in the egress path like I am looking into for the
host side.

Re: [RFC net-next 08/18] tun: run offloaded XDP program in Tx path

[David Ahern]

On 11/26/19 4:07 AM, Prashant Bhole wrote:
> run offloaded XDP program as soon as packet is removed from the ptr
> ring. Since this is XDP in Tx path, the traditional handling of
> XDP actions XDP_TX/REDIRECT isn't valid. For this reason we call
> do_xdp_generic_core instead of do_xdp_generic. do_xdp_generic_core
> just runs the program and leaves the action handling to us.

What happens if an offloaded program returns XDP_REDIRECT?

Below you just drop the packet which is going to be a bad user
experience. A better user experience is to detect XDP return codes a
program uses, catch those that are not supported for this use case and
fail the install of the program.

Re: [RFC net-next 07/18] tun: set offloaded xdp program

[David Ahern]

On 11/26/19 4:07 AM, Prashant Bhole wrote:
> From: Jason Wang 
> 
> This patch introduces an ioctl way to set an offloaded XDP program
> to tun driver. This ioctl will be used by qemu to offload XDP program
> from virtio_net in the guest.
> 
> Signed-off-by: Jason Wang 
> Signed-off-by: Prashant Bhole 
> ---
>  drivers/net/tun.c   | 19 ++-
>  include/uapi/linux/if_tun.h |  1 +
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index d078b4659897..ecb49101b0b5 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -241,6 +241,7 @@ struct tun_struct {
>   struct bpf_prog __rcu *xdp_prog;
>   struct tun_prog __rcu *steering_prog;
>   struct tun_prog __rcu *filter_prog;
> + struct tun_prog __rcu *offloaded_xdp_prog;

I have been looking into running XDP pograms in the TX path of a tap
device [1] where the program is installed and managed by a process in
the host. The code paths are the same as what you are doing with XDP
offload, so how about calling this xdp_prog_tx?

[1]
https://github.com/dsahern/linux/commit/f2303d05187c8a604cdb70b288338e9b1d1b0db6

Re: [PATCH 0/2] RFC: add -mem-shared option

[Marc-André Lureau]

Hi

On Sat, Nov 30, 2019 at 12:23 AM Eduardo Habkost  wrote:
>
> On Fri, Nov 29, 2019 at 10:31:36AM +0100, Paolo Bonzini wrote:
> > On 28/11/19 17:10, Eduardo Habkost wrote:
> > > On Thu, Nov 28, 2019 at 06:15:16PM +0400, Marc-André Lureau wrote:
> > >> Hi,
> > >>
> > >> Setting up shared memory for vhost-user is a bit complicated from
> > >> command line, as it requires NUMA setup such as: m 4G -object
> > >> memory-backend-file,id=mem,size=4G,mem-path=/dev/shm,share=on -numa
> > >> node,memdev=mem.
> > >>
> > >> Instead, I suggest to add a -mem-shared option for non-numa setups,
> > >> that will make the -mem-path or anonymouse memory shareable.
> > >
> > > Can we make this be a "-m" option?
> > >
> > > Or, even better: can we make "-m" options be automatically
> > > translated to memory-backend-* options somehow?
> > >
> >
> > The original idea was to always support one NUMA node, so that you could
> > do "-numa node,memdev=..." to specify a memory backend with -object.
> > However, this is not possible anymore since
> >
> > if (!mc->cpu_index_to_instance_props ||
> > !mc->get_default_cpu_node_id) {
> > error_setg(errp, "NUMA is not supported by this machine-type");
> > return;
> > }
> >
> > has been added to hw/core/numa.c.
> >
> > Therefore, I think instead of -mem-shared we should add a "-m
> > memdev=..." option.  This option:
> >
> > * would be mutually exclusive with both -mem-path
> >
> > * would be handled from allocate_system_memory_nonnuma.
> >
> > * could be mutually exclusive "-numa node", or could just be mutually
> > exclusive with "-numa node,memdev=..." (the logical conclusion of that
> > however would be an undeprecation of "-numa node,mem=...", so that has
> > to be taken into account as well).
>
> I completely agree we could do this.  I just think this misses
> completely the point of this series, because usability of:
>
>   -object memory-backend-file,...,share=on,id=mem -m ...,memdev=mem
>
> is not much better than the usability of:
>
>   -object memory-backend-file,...,share=on,id=mem -numa node,memdev=mem
>

+1
Perhaps when all RAM allocation will occur through memory-backend,
"-mem-shared" could be simply an alias to "-global
memory-backend.shared=on"


-- 
Marc-André Lureau

Re: [PATCH v6 00/37] Clean-ups: qom-ify serial and remove QDEV_PROP_PTR

[Marc-André Lureau]

Hi Aleksandar

On Sun, Dec 1, 2019 at 4:15 PM Aleksandar Markovic
 wrote:
>
>
>
> On Sunday, December 1, 2019, Marc-André Lureau  
> wrote:
>
>>
>> - "RFC: mips/cps: fix setting saar property"
>>
>> Perhaps I should have used FIX instead of RFC, because this should
>> actually be a real fix. However I could use someone help to exercise
>> the code path.
>>
>
> Marc-André, hi.
>
> There is a work in progress on fixing this. Can we in MIPS submit the fix 
> independently, since it involves some additional pieces of code that are 
> really deeply mips-specific? We acknowledge the bug, and want to develop the 
> real solution. Can you simply skip this RFC patch in your series, since the 
> issues will be handled separately in our patch, hopefully soon after the 
> merge window is open?
>
> For all other mips parts of your series, you have my "reviewed-by"s , in case 
> I forgot to send them explicitely.
>

This is a one-liner, and it is required to achieve the goal of the
series, to remove PROP_PTR.

If you prefer, I can instead comment the line with a FIXME, since it
is apparently broken anyway?

If you manage to get your fix merged earlier, then this patch can be
dropped. Else, is it a problem for the later fixes?

thanks


-- 
Marc-André Lureau

Re: [PATCH 00/21] Error handling fixes, may contain 4.2 material

[Michael S. Tsirkin]

On Sat, Nov 30, 2019 at 08:42:19PM +0100, Markus Armbruster wrote:
> PATCH 2-4 fix crash bugs.  Including them would be a no-brainer at
> -rc0.  But we're post -rc3, and even for crash bugs we require a
> certain likelihood of users getting bitten.
> 
> Jens, please assess impact of PATCH 2's crash bug.
> 
> Kevin, please do the same for PATCH 3.
> 
> Daniel, please do the same for PATCH 4.

virtio things:

Reviewed-by: Michael S. Tsirkin 

Jason do you want to pick these?

> The remainder is definitely not 4.2 material.
> 
> Cc: "Daniel P. Berrangé" 
> Cc: "Michael S. Tsirkin" 
> Cc: Christian Borntraeger 
> Cc: Corey Minyard 
> Cc: Cornelia Huck 
> Cc: David Hildenbrand 
> Cc: Halil Pasic 
> Cc: Igor Mammedov 
> Cc: Jens Freimann 
> Cc: Kevin Wolf 
> Cc: Michael Roth 
> Cc: Michael S. Tsirkin 
> Cc: Nishanth Aravamudan 
> Cc: Stefan Hajnoczi 
> 
> Markus Armbruster (21):
>   net/virtio: Drop useless n->primary_dev not null checks
>   net/virtio: Fix failover error handling crash bugs
>   block/file-posix: Fix laio_init() error handling crash bug
>   crypto: Fix certificate file error handling crash bug
>   crypto: Fix typo in QCryptoTLSSession's  comment
>   io: Fix Error usage in a comment 
>   tests: Clean up initialization of Error *err variables
>   exec: Fix latent file_ram_alloc() error handling bug
>   hw/acpi: Fix latent legacy CPU plug error handling bug
>   hw/core: Fix latent fit_load_fdt() error handling bug
>   hw/ipmi: Fix latent realize() error handling bugs
>   qga: Fix latent guest-get-fsinfo error handling bug
>   memory-device: Fix latent memory pre-plug error handling bugs
>   s390x/event-facility: Fix latent realize() error handling bug
>   s390x/cpu_models: Fix latent feature property error handling bugs
>   s390/cpu_modules: Fix latent realize() error handling bugs
>   s390x: Fix latent query-cpu-model-FOO error handling bugs
>   s390x: Fix latent query-cpu-definitions error handling bug
>   error: Clean up unusual names of Error * variables
>   hw/intc/s390: Simplify error handling in kvm_s390_flic_realize()
>   tests-blockjob: Use error_free_or_abort()
> 
>  include/crypto/tlssession.h |  2 +-
>  include/io/task.h   |  2 +-
>  block/file-posix.c  |  2 +-
>  crypto/tlscredsx509.c   |  2 +-
>  exec.c  |  6 +-
>  hw/acpi/cpu_hotplug.c   | 10 +--
>  hw/core/loader-fit.c| 15 ++---
>  hw/intc/s390_flic_kvm.c | 16 +++--
>  hw/ipmi/isa_ipmi_bt.c   |  7 ++-
>  hw/ipmi/isa_ipmi_kcs.c  |  7 ++-
>  hw/ipmi/pci_ipmi_bt.c   |  6 +-
>  hw/ipmi/pci_ipmi_kcs.c  |  6 +-
>  hw/mem/memory-device.c  |  6 +-
>  hw/net/virtio-net.c | 27 
>  hw/ppc/spapr_pci.c  | 16 ++---
>  hw/ppc/spapr_pci_nvlink2.c  | 10 +--
>  hw/s390x/event-facility.c   |  6 +-
>  qga/commands-posix.c|  6 +-
>  target/s390x/cpu_models.c   | 98 +
>  tests/test-blockjob.c   | 15 +++--
>  tests/test-qobject-output-visitor.c |  8 +--
>  tests/test-string-output-visitor.c  |  4 +-
>  22 files changed, 154 insertions(+), 123 deletions(-)
> 
> -- 
> 2.21.0

Re: [PATCH 16/21] s390/cpu_modules: Fix latent realize() error handling bugs

[David Hildenbrand]

On 30.11.19 20:42, Markus Armbruster wrote:
> get_max_cpu_model() crashes when kvm_s390_get_host_cpu_model() fails
> and its @errp argument is null.
> 
> apply_cpu_model() crashes when kvm_s390_apply_cpu_model() fails and
> its @errp argument is null.
> 
> s390_realize_cpu_model() crashes when get_max_cpu_model() or
> check_compatibility() fail, and its @errp argument is null.
> 
> All three messed up in commit 80560137cf "s390x/cpumodel: check and
> apply the CPU model".
> 
> The bugs can't bite as no caller actually passes null.  Fix them
> anyway.
> 

Subject is wrong, should e.g., start with "s390x/cpumodels". (I am not
aware of CPU modules :) )

[...]

Same comment regarding "local_err" and "BUG".

Reviewed-by: David Hildenbrand 

-- 
Thanks,

David / dhildenb

Re: [PATCH 13/21] memory-device: Fix latent memory pre-plug error handling bugs

[David Hildenbrand]

On 30.11.19 20:42, Markus Armbruster wrote:
> memory_device_get_free_addr() crashes when
> memory_device_check_addable() fails and its @errp argument is null.
> Messed up in commit 1b6d6af21b "pc-dimm: factor out capacity and slot
> checks into MemoryDevice".
> 
> The bug can't bite as no caller actually passes null.  Fix it anyway.
> 
> Cc: David Hildenbrand 
> Signed-off-by: Markus Armbruster 
> ---
>  hw/mem/memory-device.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index aef148c1d7..4bc9cf0917 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -99,6 +99,7 @@ static uint64_t memory_device_get_free_addr(MachineState 
> *ms,
>  uint64_t align, uint64_t size,
>  Error **errp)
>  {
> +Error *err = NULL;
>  GSList *list = NULL, *item;
>  Range as, new = range_empty;
>  
> @@ -123,8 +124,9 @@ static uint64_t memory_device_get_free_addr(MachineState 
> *ms,
>  return 0;
>  }
>  
> -memory_device_check_addable(ms, size, errp);
> -if (*errp) {
> +memory_device_check_addable(ms, size, &err);
> +if (err) {
> +error_propagate(errp, err);
>  return 0;
>  }

I remember that some time ago, the best practice was to use "local_err",
what is the latest state of that?

I still disagree that these are BUGs or even latent BUGs. If somebody
things these are BUGs and not cleanups, then we should probably have
proper "Fixes: " tags instead.

Reviewed-by: David Hildenbrand 


-- 
Thanks,

David / dhildenb

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[Aleksandar Markovic]

On Sunday, December 1, 2019, Aleksandar Markovic <
aleksandar.m.m...@gmail.com> wrote:

>
>
> On Sunday, December 1, 2019, Aleksandar Markovic <
> aleksandar.m.m...@gmail.com> wrote:
>
>>
>>
>> On Saturday, November 30, 2019, David Hildenbrand 
>> wrote:
>>
>>>
>>>
>>> > Am 30.11.2019 um 20:42 schrieb Markus Armbruster :
>>> >
>>> > cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(
>>> ),
>>> > qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
>>> > crashes when the visitor or the QOM setter fails, and its @errp
>>> > argument is null.  Messed up in commit 137974cea3 's390x/cpumodel:
>>> > implement QMP interface "query-cpu-model-expansion"'.
>>> >
>>> > Its three callers have the same bug.  Messed up in commit 4e82ef0502
>>> > 's390x/cpumodel: implement QMP interface "query-cpu-model-comparison"'
>>> > and commit f1a47d08ef 's390x/cpumodel: implement QMP interface
>>> > "query-cpu-model-baseline"'.
>>> >
>>> > The bugs can't bite as no caller actually passes null.  Fix them
>>> > anyway.
>>>
>>> https://en.m.wikipedia.org/wiki/Software_bug
>>>
>>>   „ A software bug is an error, flaw or fault in a computer program or
>>> system that causes it to produce an incorrect or unexpected result, or to
>>> behave in unintended ways. „
>>>
>>> Please make it clear in the descriptions that these are cleanups and not
>>> bugfixes. It might be very confusing for people looking out for real bugs.
>>
>>
>>>
>> Disclaimer: I am not entirely familiar with the code in question, so take
>> my opinion with reasonablereservation.
>>
>> It looks that we here deal with latent bugs. As you probably know from
>> experience, a latent bugs, when they are activated with some ostensibly
>> unrelated code change, can be much more difficult to diagnose and fix than
>> regular bugs.
>>
>>
> Oops, I didn't even realize that the patch title contains the word
> "latent". (I wrote the previous message without that knowledge. For some
> strange reason, my email client doesn't display email subject while
> replying.)
>
> In this case, I would suggest usage of phrase "latent bug" instead of
> "latent error" or so in the message title, to strenghten the point that
> this is not a cleanup.
>
>
Actually, the message title already does use "latent  bugs". So it is
fine - in my opinion.



> Yours, Aleksandar
>
>
>
>> In that light, this change is not a clean up. It is a fix of a latent
>> bugs, and Markus' aproach to treat it as a bug fix looks right to me. I
>> would just add a word "latent" or similar, which would even more distance
>> the patch from "cleanup" meaning.
>>
>> David, if I understand well, this patch fixes the commit done by you. I
>> definitely understand this is not a pleasant position, but we all
>> (definitelly including myself too) should learn to handle such situations
>> as gracefully as we can.
>>
>> Yours,
>> Aleksandar
>>
>>
>>
>>>
>>>
>>>
>>> Also, please change the terminology „messed up“ to „introduced in“ or
>>> similar.
>>>
>>> (applies to all s390x patches)
>>>
>>> Thanks.
>>
>>

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[David Hildenbrand]

On 01.12.19 14:46, Aleksandar Markovic wrote:
> 
> 
> On Saturday, November 30, 2019, David Hildenbrand  > wrote:
> 
> 
> 
> > Am 30.11.2019 um 20:42 schrieb Markus Armbruster
> mailto:arm...@redhat.com>>:
> >
> > cpu_model_from_info() is a helper for
> qmp_query_cpu_model_expansion(),
> > qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
> > crashes when the visitor or the QOM setter fails, and its @errp
> > argument is null.  Messed up in commit 137974cea3 's390x/cpumodel:
> > implement QMP interface "query-cpu-model-expansion"'.
> >
> > Its three callers have the same bug.  Messed up in commit 4e82ef0502
> > 's390x/cpumodel: implement QMP interface "query-cpu-model-comparison"'
> > and commit f1a47d08ef 's390x/cpumodel: implement QMP interface
> > "query-cpu-model-baseline"'.
> >
> > The bugs can't bite as no caller actually passes null.  Fix them
> > anyway.
> 
> https://en.m.wikipedia.org/wiki/Software_bug
> 
> 
>   „ A software bug is an error, flaw or fault in a computer program
> or system that causes it to produce an incorrect or unexpected
> result, or to behave in unintended ways. „
> 
> Please make it clear in the descriptions that these are cleanups and
> not bugfixes. It might be very confusing for people looking out for
> real bugs.
> 
> 
> 
> Disclaimer: I am not entirely familiar with the code in question, so
> take my opinion with reasonablereservation.
> 
> It looks that we here deal with latent bugs. As you probably know from
> experience, a latent bugs, when they are activated with some ostensibly
> unrelated code change, can be much more difficult to diagnose and fix
> than regular bugs.

"https://economictimes.indiatimes.com/definition/latent-bug

"Definition: An uncovered or unidentified bug which exists in the system
over a period of time is known as the Latent Bug. The bug may persist in
the system in one or more versions of the software."

AFAIK, a latent BUG can be triggered, it simply was never triggered.


Do you think the following code is buggy?

static int get_val(int *ptr)
{
return *ptr;
}

int main()
{
int a = 0;

return get_val(&a);
}

I claim, no, although we could access a NULL pointer if ever reworked.
There is no invalid system state possible.


> 
> In that light, this change is not a clean up. It is a fix of a latent
> bugs, and Markus' aproach to treat it as a bug fix looks right to me. I
> would just add a word "latent" or similar, which would even more
> distance the patch from "cleanup" meaning.

I agree iff there is some way to trigger it. Otherwise, to me it is a
cleanup.If it's a BUG, it deserves proper Fixes tags and some
description how it can be triggered.


-- 
Thanks,

David / dhildenb

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[Aleksandar Markovic]

On Sunday, December 1, 2019, Aleksandar Markovic <
aleksandar.m.m...@gmail.com> wrote:

>
>
> On Saturday, November 30, 2019, David Hildenbrand 
> wrote:
>
>>
>>
>> > Am 30.11.2019 um 20:42 schrieb Markus Armbruster :
>> >
>> > cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(),
>> > qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
>> > crashes when the visitor or the QOM setter fails, and its @errp
>> > argument is null.  Messed up in commit 137974cea3 's390x/cpumodel:
>> > implement QMP interface "query-cpu-model-expansion"'.
>> >
>> > Its three callers have the same bug.  Messed up in commit 4e82ef0502
>> > 's390x/cpumodel: implement QMP interface "query-cpu-model-comparison"'
>> > and commit f1a47d08ef 's390x/cpumodel: implement QMP interface
>> > "query-cpu-model-baseline"'.
>> >
>> > The bugs can't bite as no caller actually passes null.  Fix them
>> > anyway.
>>
>> https://en.m.wikipedia.org/wiki/Software_bug
>>
>>   „ A software bug is an error, flaw or fault in a computer program or
>> system that causes it to produce an incorrect or unexpected result, or to
>> behave in unintended ways. „
>>
>> Please make it clear in the descriptions that these are cleanups and not
>> bugfixes. It might be very confusing for people looking out for real bugs.
>
>
>>
> Disclaimer: I am not entirely familiar with the code in question, so take
> my opinion with reasonablereservation.
>
> It looks that we here deal with latent bugs. As you probably know from
> experience, a latent bugs, when they are activated with some ostensibly
> unrelated code change, can be much more difficult to diagnose and fix than
> regular bugs.
>
>
Oops, I didn't even realize that the patch title contains the word
"latent". (I wrote the previous message without that knowledge. For some
strange reason, my email client doesn't display email subject while
replying.)

In this case, I would suggest usage of phrase "latent bug" instead of
"latent error" or so in the message title, to strenghten the point that
this is not a cleanup.

Yours, Aleksandar



> In that light, this change is not a clean up. It is a fix of a latent
> bugs, and Markus' aproach to treat it as a bug fix looks right to me. I
> would just add a word "latent" or similar, which would even more distance
> the patch from "cleanup" meaning.
>
> David, if I understand well, this patch fixes the commit done by you. I
> definitely understand this is not a pleasant position, but we all
> (definitelly including myself too) should learn to handle such situations
> as gracefully as we can.
>
> Yours,
> Aleksandar
>
>
>
>>
>>
>>
>> Also, please change the terminology „messed up“ to „introduced in“ or
>> similar.
>>
>> (applies to all s390x patches)
>>
>> Thanks.
>
>

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[Aleksandar Markovic]

On Saturday, November 30, 2019, David Hildenbrand 
wrote:

>
>
> > Am 30.11.2019 um 20:42 schrieb Markus Armbruster :
> >
> > cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(),
> > qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
> > crashes when the visitor or the QOM setter fails, and its @errp
> > argument is null.  Messed up in commit 137974cea3 's390x/cpumodel:
> > implement QMP interface "query-cpu-model-expansion"'.
> >
> > Its three callers have the same bug.  Messed up in commit 4e82ef0502
> > 's390x/cpumodel: implement QMP interface "query-cpu-model-comparison"'
> > and commit f1a47d08ef 's390x/cpumodel: implement QMP interface
> > "query-cpu-model-baseline"'.
> >
> > The bugs can't bite as no caller actually passes null.  Fix them
> > anyway.
>
> https://en.m.wikipedia.org/wiki/Software_bug
>
>   „ A software bug is an error, flaw or fault in a computer program or
> system that causes it to produce an incorrect or unexpected result, or to
> behave in unintended ways. „
>
> Please make it clear in the descriptions that these are cleanups and not
> bugfixes. It might be very confusing for people looking out for real bugs.


>
Disclaimer: I am not entirely familiar with the code in question, so take
my opinion with reasonablereservation.

It looks that we here deal with latent bugs. As you probably know from
experience, a latent bugs, when they are activated with some ostensibly
unrelated code change, can be much more difficult to diagnose and fix than
regular bugs.

In that light, this change is not a clean up. It is a fix of a latent bugs,
and Markus' aproach to treat it as a bug fix looks right to me. I would
just add a word "latent" or similar, which would even more distance the
patch from "cleanup" meaning.

David, if I understand well, this patch fixes the commit done by you. I
definitely understand this is not a pleasant position, but we all
(definitelly including myself too) should learn to handle such situations
as gracefully as we can.

Yours,
Aleksandar



>
>
>
> Also, please change the terminology „messed up“ to „introduced in“ or
> similar.
>
> (applies to all s390x patches)
>
> Thanks.

Re: [PATCH v37 00/17] QEMU AVR 8 bit cores

[Aleksandar Markovic]

On Sunday, December 1, 2019, Aleksandar Markovic <
aleksandar.m.m...@gmail.com> wrote:

> Renaming devices such hw/char/avr_usart.c -> hw/char/atmel_usart.c
>> (similarly with the macros) would be enough Aleksandar?
>>
>> On Thursday, November 28, 2019, Michael Rolnik  wrote:
>
>> I will rename them.
>>
>
> AVR is the name of a microcontroller lineup, and Atmel is the name of the
> company that started producing them. Atmel was recently acquired by
> Microchip, so thw word Atmel now does not even exist in new specs.
>
> Taking this into account, I don't think renaming
>
> hw/char/avr_usart.c -> hw/char/atmel_usart.c
>
> is not appropriate.
>
>
I meant to say the renaming is not appropriate. Sorry for confusion.


>
>
>  Renaming macros, too. The current names are fine, for now.
>
> A separate but related naming question will show up later in future, when
> we, let's say, want to implement two different version of a peripheral
> (let's say USART), one as specified for older microcontrollers, and one for
> newer.
>
> But, OK, let's leave that for future.
>
> Regards,
> Aleksandar
>
>
>
>> On Thu, Nov 28, 2019 at 3:41 PM Aleksandar Markovic <
>> aleksandar.m.m...@gmail.com> wrote:
>>
>>>
>>>
>>> On Thursday, November 28, 2019, Philippe Mathieu-Daudé <
>>> phi...@redhat.com> wrote:
>>>
 On 11/28/19 2:25 PM, Michael Rolnik wrote:

> I don't see why you say that the peripherals are inside the chip,
> there is CPU within target/avr directory and then there are some
> peripherals in hw directory, CPU does not depend on them. what am I 
> missing?
>
> On Thu, Nov 28, 2019 at 3:22 PM Aleksandar Markovic <
> aleksandar.m.m...@gmail.com >
> wrote:
>
>
>
> On Thursday, November 28, 2019, Michael Rolnik  > wrote:
>
>
>
> On Wed, Nov 27, 2019 at 11:06 PM Aleksandar Markovic
>  > wrote:
>
> On Wed, Nov 27, 2019 at 6:53 PM Michael Rolnik
> mailto:mrol...@gmail.com>> wrote:
>  >
>  > This series of patches adds 8bit AVR cores to QEMU.
>  > All instruction, except BREAK/DES/SPM/SPMX, are
> implemented. Not fully tested yet.
>  > However I was able to execute simple code with
> functions.
> e.g fibonacci calculation.
>  > This series of patches include a non real, sample board.
>  > No fuses support yet. PC is set to 0 at reset.
>  >
>
> I have a couple of general remarks, so I am responding to
> the cover
> letter, not individual patches.
>
> 1) The licenses for Sarah devices differ than the rest -
> shouldn't all
> licenses be harmonized?
>
> Sarah,
> do you mind if use the same license I use for my code?
>
>
> 2) There is an architectural problem with peripherals. It
> is
> possible
> that they evolve over time, so, for example, USART could
> not
> be the
> same for older and newer CPUs (in principle, newer
> peripheral is
> expected to be o sort of "superset" of the older). How do
> you solve
> that problem? Right now, it may not looks serious to you,
> but if you
> don;t think about that right now, from the outset, soon the
> code will
> become so entangled, ti woudl be almost very difficult to
> fix it.
> Please think about that, how would you solve it, is there a
> way to
> pass the information on the currently emulated CPU to the
> code
> covering a peripheral, and provide a different behaviour?
>
> Hi Aleksandar,
>
> Please explain.
>
>
> My concern is about peripherals inside the chip, together with the
> core.
>
> If one models, let's say an external (in the sense, it is a
> separate
> chip) ADC (analog-to-digital converter), one looks at specs,
> implement what is resonable possible in QEMU, plug it in in one of
> machines thst contains it, and that's it. That ADC remains the
> same,
> of course, whatever the surrounding system is.
>
> In AVR case, I think we have a phenomenon likes of which we didn't
> see before (at least I don't know about). Number of AVR
> microcontrollers is very large, and both cores and peripherals
> evolved.
>
> For cores, you handle differences with all these AVR_FEATURE
> macros,
> and this seems to be working, no signif

Re: [PATCH v37 00/17] QEMU AVR 8 bit cores

[Aleksandar Markovic]

>
> Renaming devices such hw/char/avr_usart.c -> hw/char/atmel_usart.c
> (similarly with the macros) would be enough Aleksandar?
>
> On Thursday, November 28, 2019, Michael Rolnik  wrote:

> I will rename them.
>

AVR is the name of a microcontroller lineup, and Atmel is the name of the
company that started producing them. Atmel was recently acquired by
Microchip, so thw word Atmel now does not even exist in new specs.

Taking this into account, I don't think renaming

hw/char/avr_usart.c -> hw/char/atmel_usart.c

is not appropriate. Renaming macros, too. The current names are fine, for
now.

A separate but related naming question will show up later in future, when
we, let's say, want to implement two different version of a peripheral
(let's say USART), one as specified for older microcontrollers, and one for
newer.

But, OK, let's leave that for future.

Regards,
Aleksandar



> On Thu, Nov 28, 2019 at 3:41 PM Aleksandar Markovic <
> aleksandar.m.m...@gmail.com> wrote:
>
>>
>>
>> On Thursday, November 28, 2019, Philippe Mathieu-Daudé 
>> wrote:
>>
>>> On 11/28/19 2:25 PM, Michael Rolnik wrote:
>>>
 I don't see why you say that the peripherals are inside the chip, there
 is CPU within target/avr directory and then there are some peripherals in
 hw directory, CPU does not depend on them. what am I missing?

 On Thu, Nov 28, 2019 at 3:22 PM Aleksandar Markovic <
 aleksandar.m.m...@gmail.com >
 wrote:



 On Thursday, November 28, 2019, Michael Rolnik >>> > wrote:



 On Wed, Nov 27, 2019 at 11:06 PM Aleksandar Markovic
 >>> > wrote:

 On Wed, Nov 27, 2019 at 6:53 PM Michael Rolnik
 mailto:mrol...@gmail.com>> wrote:
  >
  > This series of patches adds 8bit AVR cores to QEMU.
  > All instruction, except BREAK/DES/SPM/SPMX, are
 implemented. Not fully tested yet.
  > However I was able to execute simple code with functions.
 e.g fibonacci calculation.
  > This series of patches include a non real, sample board.
  > No fuses support yet. PC is set to 0 at reset.
  >

 I have a couple of general remarks, so I am responding to
 the cover
 letter, not individual patches.

 1) The licenses for Sarah devices differ than the rest -
 shouldn't all
 licenses be harmonized?

 Sarah,
 do you mind if use the same license I use for my code?


 2) There is an architectural problem with peripherals. It is
 possible
 that they evolve over time, so, for example, USART could not
 be the
 same for older and newer CPUs (in principle, newer
 peripheral is
 expected to be o sort of "superset" of the older). How do
 you solve
 that problem? Right now, it may not looks serious to you,
 but if you
 don;t think about that right now, from the outset, soon the
 code will
 become so entangled, ti woudl be almost very difficult to
 fix it.
 Please think about that, how would you solve it, is there a
 way to
 pass the information on the currently emulated CPU to the
 code
 covering a peripheral, and provide a different behaviour?

 Hi Aleksandar,

 Please explain.


 My concern is about peripherals inside the chip, together with the
 core.

 If one models, let's say an external (in the sense, it is a separate
 chip) ADC (analog-to-digital converter), one looks at specs,
 implement what is resonable possible in QEMU, plug it in in one of
 machines thst contains it, and that's it. That ADC remains the same,
 of course, whatever the surrounding system is.

 In AVR case, I think we have a phenomenon likes of which we didn't
 see before (at least I don't know about). Number of AVR
 microcontrollers is very large, and both cores and peripherals
 evolved.

 For cores, you handle differences with all these AVR_FEATURE macros,
 and this seems to be working, no significant objection from my side,
 and btw that was not an easy task to execute, all admiration from
 me.

 But what about peripherals inside the chip? A peripheral with the
 same name and the same general area of functionality may be
 differently specified for microcontrollers from 2010 and 2018. By
 the difference I don

[PATCH v2 2/5] target/arm: Honor HCR_EL2.TID1 trapping requirements

[Marc Zyngier]

HCR_EL2.TID1 mandates that access from EL1 to REVIDR_EL1, AIDR_EL1
(and their 32bit equivalents) as well as TCMTR, TLBTR are trapped
to EL2. QEMU ignores it, making it harder for a hypervisor to
virtualize the HW (though to be fair, no known hypervisor actually
cares).

Do the right thing by trapping to EL2 if HCR_EL2.TID1 is set.

Reviewed-by: Edgar E. Iglesias 
Signed-off-by: Marc Zyngier 
---
 target/arm/helper.c | 36 
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 1e546096b8..93ecab27c0 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1973,6 +1973,26 @@ static uint64_t isr_read(CPUARMState *env, const 
ARMCPRegInfo *ri)
 return ret;
 }
 
+static CPAccessResult access_aa64_tid1(CPUARMState *env, const ARMCPRegInfo 
*ri,
+   bool isread)
+{
+if (arm_current_el(env) == 1 && (arm_hcr_el2_eff(env) & HCR_TID1)) {
+return CP_ACCESS_TRAP_EL2;
+}
+
+return CP_ACCESS_OK;
+}
+
+static CPAccessResult access_aa32_tid1(CPUARMState *env, const ARMCPRegInfo 
*ri,
+   bool isread)
+{
+if (arm_feature(env, ARM_FEATURE_V8)) {
+return access_aa64_tid1(env, ri, isread);
+}
+
+return CP_ACCESS_OK;
+}
+
 static const ARMCPRegInfo v7_cp_reginfo[] = {
 /* the old v6 WFI, UNPREDICTABLE in v7 but we choose to NOP */
 { .name = "NOP", .cp = 15, .crn = 7, .crm = 0, .opc1 = 0, .opc2 = 4,
@@ -2136,7 +2156,9 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
  */
 { .name = "AIDR", .state = ARM_CP_STATE_BOTH,
   .opc0 = 3, .opc1 = 1, .crn = 0, .crm = 0, .opc2 = 7,
-  .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
+  .access = PL1_R, .type = ARM_CP_CONST,
+  .accessfn = access_aa64_tid1,
+  .resetvalue = 0 },
 /* Auxiliary fault status registers: these also are IMPDEF, and we
  * choose to RAZ/WI for all cores.
  */
@@ -6732,7 +6754,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
   .access = PL1_R, .resetvalue = cpu->midr },
 { .name = "REVIDR_EL1", .state = ARM_CP_STATE_BOTH,
   .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 0, .opc2 = 6,
-  .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->revidr 
},
+  .access = PL1_R,
+  .accessfn = access_aa64_tid1,
+  .type = ARM_CP_CONST, .resetvalue = cpu->revidr },
 REGINFO_SENTINEL
 };
 ARMCPRegInfo id_cp_reginfo[] = {
@@ -6748,14 +6772,18 @@ void register_cp_regs_for_features(ARMCPU *cpu)
 /* TCMTR and TLBTR exist in v8 but have no 64-bit versions */
 { .name = "TCMTR",
   .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 2,
-  .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
+  .access = PL1_R,
+  .accessfn = access_aa32_tid1,
+  .type = ARM_CP_CONST, .resetvalue = 0 },
 REGINFO_SENTINEL
 };
 /* TLBTR is specific to VMSA */
 ARMCPRegInfo id_tlbtr_reginfo = {
   .name = "TLBTR",
   .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 3,
-  .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0,
+  .access = PL1_R,
+  .accessfn = access_aa32_tid1,
+  .type = ARM_CP_CONST, .resetvalue = 0,
 };
 /* MPUIR is specific to PMSA V6+ */
 ARMCPRegInfo id_mpuir_reginfo = {
-- 
2.20.1

[PATCH v2 4/5] target/arm: Handle AArch32 CP15 trapping via HSTR_EL2

[Marc Zyngier]

HSTR_EL2 offers a way to trap ranges of CP15 system register
accesses to EL2, and it looks like this register is completely
ignored by QEMU.

To avoid adding extra .accessfn filters all over the place (which
would have a direct performance impact), let's add a new TB flag
that gets set whenever HSTR_EL2 is non-zero and that QEMU translates
a context where this trap has a chance to apply, and only generate
the extra access check if the hypervisor is actively using this feature.

Tested with a hand-crafted KVM guest accessing CBAR.

Signed-off-by: Marc Zyngier 
---
 target/arm/cpu.h   |  2 ++
 target/arm/helper.c|  6 ++
 target/arm/op_helper.c | 21 +
 target/arm/translate.c |  3 ++-
 target/arm/translate.h |  2 ++
 5 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 83a809d4ba..cebb3511a5 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3215,6 +3215,8 @@ FIELD(TBFLAG_A32, NS, 6, 1)
 FIELD(TBFLAG_A32, VFPEN, 7, 1)  /* Partially cached, minus FPEXC. */
 FIELD(TBFLAG_A32, CONDEXEC, 8, 8)   /* Not cached. */
 FIELD(TBFLAG_A32, SCTLR_B, 16, 1)
+FIELD(TBFLAG_A32, HSTR_ACTIVE, 17, 1)
+
 /* For M profile only, set if FPCCR.LSPACT is set */
 FIELD(TBFLAG_A32, LSPACT, 18, 1)/* Not cached. */
 /* For M profile only, set if we must create a new FP context */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 93ecab27c0..0ba08d550a 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11283,6 +11283,12 @@ static uint32_t rebuild_hflags_a32(CPUARMState *env, 
int fp_el,
 if (arm_el_is_aa64(env, 1)) {
 flags = FIELD_DP32(flags, TBFLAG_A32, VFPEN, 1);
 }
+
+if (arm_current_el(env) < 2 && env->cp15.hstr_el2 &&
+(arm_hcr_el2_eff(env) & (HCR_E2H | HCR_TGE)) != (HCR_E2H | HCR_TGE)) {
+flags = FIELD_DP32(flags, TBFLAG_A32, HSTR_ACTIVE, 1);
+}
+
 return rebuild_hflags_common_32(env, fp_el, mmu_idx, flags);
 }
 
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index b529d6c1bf..681c5f3681 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -603,6 +603,26 @@ void HELPER(access_check_cp_reg)(CPUARMState *env, void 
*rip, uint32_t syndrome,
 raise_exception(env, EXCP_UDEF, syndrome, exception_target_el(env));
 }
 
+/* Check for an EL2 trap due to HSTR_EL2. We expect EL0 accesses
+ * to sysregs non accessible at EL0 to have UNDEF-ed already.
+ */
+if (!env->aarch64 && arm_current_el(env) < 2 && ri->cp == 15 &&
+(arm_hcr_el2_eff(env) & (HCR_E2H | HCR_TGE)) != (HCR_E2H | HCR_TGE)) {
+uint32_t mask = 1 << ri->crn;
+
+if (ri->type & ARM_CP_64BIT) {
+mask = 1 << ri->crm;
+}
+
+/* T4 and T14 are RES0 */
+mask &= ~((1 << 4) | (1 << 14));
+
+if (env->cp15.hstr_el2 & mask) {
+target_el = 2;
+goto exept;
+}
+}
+
 if (!ri->accessfn) {
 return;
 }
@@ -652,6 +672,7 @@ void HELPER(access_check_cp_reg)(CPUARMState *env, void 
*rip, uint32_t syndrome,
 g_assert_not_reached();
 }
 
+exept:
 raise_exception(env, EXCP_UDEF, syndrome, target_el);
 }
 
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 4d5d4bd888..f162be8434 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -6897,7 +6897,7 @@ static int disas_coproc_insn(DisasContext *s, uint32_t 
insn)
 return 1;
 }
 
-if (ri->accessfn ||
+if (s->hstr_active || ri->accessfn ||
 (arm_dc_feature(s, ARM_FEATURE_XSCALE) && cpnum < 14)) {
 /* Emit code to perform further access permissions checks at
  * runtime; this may result in an exception.
@@ -10843,6 +10843,7 @@ static void arm_tr_init_disas_context(DisasContextBase 
*dcbase, CPUState *cs)
!arm_el_is_aa64(env, 3);
 dc->thumb = FIELD_EX32(tb_flags, TBFLAG_A32, THUMB);
 dc->sctlr_b = FIELD_EX32(tb_flags, TBFLAG_A32, SCTLR_B);
+dc->hstr_active = FIELD_EX32(tb_flags, TBFLAG_A32, HSTR_ACTIVE);
 dc->be_data = FIELD_EX32(tb_flags, TBFLAG_ANY, BE_DATA) ? MO_BE : MO_LE;
 condexec = FIELD_EX32(tb_flags, TBFLAG_A32, CONDEXEC);
 dc->condexec_mask = (condexec & 0xf) << 1;
diff --git a/target/arm/translate.h b/target/arm/translate.h
index dd24f91f26..b837b7fcbf 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -77,6 +77,8 @@ typedef struct DisasContext {
 bool pauth_active;
 /* True with v8.5-BTI and SCTLR_ELx.BT* set.  */
 bool bt;
+/* True if any CP15 access is trapped by HSTR_EL2 */
+bool hstr_active;
 /*
  * >= 0, a copy of PSTATE.BTYPE, which will be 0 without v8.5-BTI.
  *  < 0, set by the current instruction.
-- 
2.20.1

[PATCH v2 3/5] target/arm: Handle trapping to EL2 of AArch32 VMRS instructions

[Marc Zyngier]

HCR_EL2.TID3 requires that AArch32 reads of MVFR[012] are trapped to
EL2, and HCR_EL2.TID0 does the same for reads of FPSID.
In order to handle this, introduce a new TCG helper function that
checks for these control bits before executing the VMRC instruction.

Tested with a hacked-up version of KVM/arm64 that sets the control
bits for 32bit guests.

Reviewed-by: Edgar E. Iglesias 
Signed-off-by: Marc Zyngier 
---
 target/arm/helper-a64.h|  2 ++
 target/arm/translate-vfp.inc.c | 18 +++---
 target/arm/vfp_helper.c| 29 +
 3 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/target/arm/helper-a64.h b/target/arm/helper-a64.h
index a915c1247f..0af44dc814 100644
--- a/target/arm/helper-a64.h
+++ b/target/arm/helper-a64.h
@@ -102,3 +102,5 @@ DEF_HELPER_FLAGS_3(autda, TCG_CALL_NO_WG, i64, env, i64, 
i64)
 DEF_HELPER_FLAGS_3(autdb, TCG_CALL_NO_WG, i64, env, i64, i64)
 DEF_HELPER_FLAGS_2(xpaci, TCG_CALL_NO_RWG_SE, i64, env, i64)
 DEF_HELPER_FLAGS_2(xpacd, TCG_CALL_NO_RWG_SE, i64, env, i64)
+
+DEF_HELPER_3(check_hcr_el2_trap, void, env, i32, i32)
diff --git a/target/arm/translate-vfp.inc.c b/target/arm/translate-vfp.inc.c
index 85c5ef897b..bf90ac0e5b 100644
--- a/target/arm/translate-vfp.inc.c
+++ b/target/arm/translate-vfp.inc.c
@@ -761,13 +761,25 @@ static bool trans_VMSR_VMRS(DisasContext *s, 
arg_VMSR_VMRS *a)
 if (a->l) {
 /* VMRS, move VFP special register to gp register */
 switch (a->reg) {
+case ARM_VFP_MVFR0:
+case ARM_VFP_MVFR1:
+case ARM_VFP_MVFR2:
 case ARM_VFP_FPSID:
+if (s->current_el == 1) {
+TCGv_i32 tcg_reg, tcg_rt;
+
+gen_set_condexec(s);
+gen_set_pc_im(s, s->pc_curr);
+tcg_reg = tcg_const_i32(a->reg);
+tcg_rt = tcg_const_i32(a->rt);
+gen_helper_check_hcr_el2_trap(cpu_env, tcg_rt, tcg_reg);
+tcg_temp_free_i32(tcg_reg);
+tcg_temp_free_i32(tcg_rt);
+}
+/* fall through */
 case ARM_VFP_FPEXC:
 case ARM_VFP_FPINST:
 case ARM_VFP_FPINST2:
-case ARM_VFP_MVFR0:
-case ARM_VFP_MVFR1:
-case ARM_VFP_MVFR2:
 tmp = load_cpu_field(vfp.xregs[a->reg]);
 break;
 case ARM_VFP_FPSCR:
diff --git a/target/arm/vfp_helper.c b/target/arm/vfp_helper.c
index 9710ef1c3e..0ae7d4f34a 100644
--- a/target/arm/vfp_helper.c
+++ b/target/arm/vfp_helper.c
@@ -1322,4 +1322,33 @@ float64 HELPER(frint64_d)(float64 f, void *fpst)
 return frint_d(f, fpst, 64);
 }
 
+void HELPER(check_hcr_el2_trap)(CPUARMState *env, uint32_t rt, uint32_t reg)
+{
+uint32_t syndrome;
+
+switch (reg) {
+case ARM_VFP_MVFR0:
+case ARM_VFP_MVFR1:
+case ARM_VFP_MVFR2:
+if (!(arm_hcr_el2_eff(env) & HCR_TID3)) {
+return;
+}
+break;
+case ARM_VFP_FPSID:
+if (!(arm_hcr_el2_eff(env) & HCR_TID0)) {
+return;
+}
+break;
+default:
+g_assert_not_reached();
+}
+
+syndrome = ((EC_FPIDTRAP << ARM_EL_EC_SHIFT)
+| ARM_EL_IL
+| (1 << 24) | (0xe << 20) | (7 << 14)
+| (reg << 10) | (rt << 5) | 1);
+
+raise_exception(env, EXCP_HYP_TRAP, syndrome, 2);
+}
+
 #endif
-- 
2.20.1

[PATCH v2 1/5] target/arm: Honor HCR_EL2.TID2 trapping requirements

[Marc Zyngier]

HCR_EL2.TID2 mandates that access from EL1 to CTR_EL0, CCSIDR_EL1,
CCSIDR2_EL1, CLIDR_EL1, CSSELR_EL1 are trapped to EL2, and QEMU
completely ignores it, making it impossible for hypervisors to
virtualize the cache hierarchy.

Do the right thing by trapping to EL2 if HCR_EL2.TID2 is set.

Signed-off-by: Marc Zyngier 
---
 target/arm/helper.c | 31 +++
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0bf8f53d4b..1e546096b8 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1910,6 +1910,17 @@ static void scr_write(CPUARMState *env, const 
ARMCPRegInfo *ri, uint64_t value)
 raw_write(env, ri, value);
 }
 
+static CPAccessResult access_aa64_tid2(CPUARMState *env,
+   const ARMCPRegInfo *ri,
+   bool isread)
+{
+if (arm_current_el(env) == 1 && (arm_hcr_el2_eff(env) & HCR_TID2)) {
+return CP_ACCESS_TRAP_EL2;
+}
+
+return CP_ACCESS_OK;
+}
+
 static uint64_t ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
 ARMCPU *cpu = env_archcpu(env);
@@ -2110,10 +2121,14 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
   .writefn = pmintenclr_write },
 { .name = "CCSIDR", .state = ARM_CP_STATE_BOTH,
   .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
-  .access = PL1_R, .readfn = ccsidr_read, .type = ARM_CP_NO_RAW },
+  .access = PL1_R,
+  .accessfn = access_aa64_tid2,
+  .readfn = ccsidr_read, .type = ARM_CP_NO_RAW },
 { .name = "CSSELR", .state = ARM_CP_STATE_BOTH,
   .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 2, .opc2 = 0,
-  .access = PL1_RW, .writefn = csselr_write, .resetvalue = 0,
+  .access = PL1_RW,
+  .accessfn = access_aa64_tid2,
+  .writefn = csselr_write, .resetvalue = 0,
   .bank_fieldoffsets = { offsetof(CPUARMState, cp15.csselr_s),
  offsetof(CPUARMState, cp15.csselr_ns) } },
 /* Auxiliary ID register: this actually has an IMPDEF value but for now
@@ -5204,6 +5219,11 @@ static CPAccessResult ctr_el0_access(CPUARMState *env, 
const ARMCPRegInfo *ri,
 if (arm_current_el(env) == 0 && !(env->cp15.sctlr_el[1] & SCTLR_UCT)) {
 return CP_ACCESS_TRAP;
 }
+
+if (arm_current_el(env) < 2 && arm_hcr_el2_eff(env) & HCR_TID2) {
+return CP_ACCESS_TRAP_EL2;
+}
+
 return CP_ACCESS_OK;
 }
 
@@ -6184,7 +6204,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
 ARMCPRegInfo clidr = {
 .name = "CLIDR", .state = ARM_CP_STATE_BOTH,
 .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 1,
-.access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->clidr
+.access = PL1_R, .type = ARM_CP_CONST,
+.accessfn = access_aa64_tid2,
+.resetvalue = cpu->clidr
 };
 define_one_arm_cp_reg(cpu, &clidr);
 define_arm_cp_regs(cpu, v7_cp_reginfo);
@@ -6717,7 +6739,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
 /* These are common to v8 and pre-v8 */
 { .name = "CTR",
   .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 1,
-  .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->ctr },
+  .access = PL1_R, .accessfn = ctr_el0_access,
+  .type = ARM_CP_CONST, .resetvalue = cpu->ctr },
 { .name = "CTR_EL0", .state = ARM_CP_STATE_AA64,
   .opc0 = 3, .opc1 = 3, .opc2 = 1, .crn = 0, .crm = 0,
   .access = PL0_R, .accessfn = ctr_el0_access,
-- 
2.20.1

[PATCH v2 5/5] target/arm: Add support for missing Jazelle system registers

[Marc Zyngier]

QEMU lacks the minimum Jazelle implementation that is required
by the architecture (everything is RAZ or RAZ/WI). Add it
together with the HCR_EL2.TID0 trapping that goes with it.

Signed-off-by: Marc Zyngier 
---
 target/arm/helper.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0ba08d550a..d6fc198a97 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6040,6 +6040,16 @@ static CPAccessResult access_aa32_tid3(CPUARMState *env, 
const ARMCPRegInfo *ri,
 return CP_ACCESS_OK;
 }
 
+static CPAccessResult access_jazelle(CPUARMState *env, const ARMCPRegInfo *ri,
+ bool isread)
+{
+if (arm_current_el(env) == 1 && (arm_hcr_el2_eff(env) & HCR_TID0)) {
+return CP_ACCESS_TRAP_EL2;
+}
+
+return CP_ACCESS_OK;
+}
+
 void register_cp_regs_for_features(ARMCPU *cpu)
 {
 /* Register all the coprocessor registers based on feature bits */
@@ -6057,6 +6067,23 @@ void register_cp_regs_for_features(ARMCPU *cpu)
 define_arm_cp_regs(cpu, not_v8_cp_reginfo);
 }
 
+if (cpu_isar_feature(jazelle, cpu)) {
+ARMCPRegInfo jazelle_regs[] = {
+{ .name = "JIDR",
+  .cp = 14, .crn = 0, .crm = 0, .opc1 = 7, .opc2 = 0,
+  .access = PL1_R, .accessfn = access_jazelle,
+  .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "JOSCR",
+  .cp = 14, .crn = 1, .crm = 0, .opc1 = 7, .opc2 = 0,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+{ .name = "JMCR",
+  .cp = 14, .crn = 2, .crm = 0, .opc1 = 7, .opc2 = 0,
+  .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+REGINFO_SENTINEL
+};
+
+define_arm_cp_regs(cpu, jazelle_regs);
+}
 if (arm_feature(env, ARM_FEATURE_V6)) {
 /* The ID registers all have impdef reset values */
 ARMCPRegInfo v6_idregs[] = {
-- 
2.20.1

[PATCH v2 0/5] target/arm: More EL2 trapping fixes

[Marc Zyngier]

Hi all,

This series is a follow-up on [1], which tried to address the
remaining missing HCR_EL2.TIDx traps. I've hopefully now adressed the
comments that Peter and Edgar raised.

I've also tried to tackle missing traps generated by HSTR_EL2, which
got completely ignored so far. Note that this results in the use of a
new TB bit, which I understand is a rare resource. I'd welcome
comments on how to handle it differently if at all possible.

Finally, and as a bonus non-feature, I've added support for the
missing Jazelle registers, giving me the opportunity to allow trapping
of JIDR to EL2 using HCR_EL2.TID0. Yay, Christmas! ;-)

I'm now going back to kernel stuff. I swear!

[1] https://patchew.org/QEMU/20191128161718.24361-1-...@kernel.org/

Marc Zyngier (5):
  target/arm: Honor HCR_EL2.TID2 trapping requirements
  target/arm: Honor HCR_EL2.TID1 trapping requirements
  target/arm: Handle trapping to EL2 of AArch32 VMRS instructions
  target/arm: Handle AArch32 CP15 trapping via HSTR_EL2
  target/arm: Add support for missing Jazelle system registers

 target/arm/cpu.h   |   2 +
 target/arm/helper-a64.h|   2 +
 target/arm/helper.c| 100 ++---
 target/arm/op_helper.c |  21 +++
 target/arm/translate-vfp.inc.c |  18 +-
 target/arm/translate.c |   3 +-
 target/arm/translate.h |   2 +
 target/arm/vfp_helper.c|  29 ++
 8 files changed, 165 insertions(+), 12 deletions(-)

-- 
2.20.1

Re: [PATCH v6 00/37] Clean-ups: qom-ify serial and remove QDEV_PROP_PTR

[Aleksandar Markovic]

On Sunday, December 1, 2019, Marc-André Lureau 
wrote:


> - "RFC: mips/cps: fix setting saar property"
>
> Perhaps I should have used FIX instead of RFC, because this should
> actually be a real fix. However I could use someone help to exercise
> the code path.
>
>
Marc-André, hi.

There is a work in progress on fixing this. Can we in MIPS submit the fix
independently, since it involves some additional pieces of code that are
really deeply mips-specific? We acknowledge the bug, and want to develop
the real solution. Can you simply skip this RFC patch in your series, since
the issues will be handled separately in our patch, hopefully soon after
the merge window is open?

For all other mips parts of your series, you have my "reviewed-by"s , in
case I forgot to send them explicitely.

Regards, Aleksandar




> - "
>