Re: [PATCH qemu] spapr: Fix typo in the patb_entry comment

[David Gibson]

On Thu, Feb 25, 2021 at 02:23:35PM +1100, Alexey Kardashevskiy wrote:
> There is no H_REGISTER_PROCESS_TABLE, it is H_REGISTER_PROC_TBL handler
> for which is still called h_register_process_table() though.
> 
> Signed-off-by: Alexey Kardashevskiy 

Applied to ppc-for-6.0.

In future, best to CC me directly, since I only occasionally peruse
the lists.

> ---
>  include/hw/ppc/spapr.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index ccbeeca1de84..8dceace4b442 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -168,7 +168,7 @@ struct SpaprMachineState {
>  SpaprResizeHpt resize_hpt;
>  void *htab;
>  uint32_t htab_shift;
> -uint64_t patb_entry; /* Process tbl registed in H_REGISTER_PROCESS_TABLE 
> */
> +uint64_t patb_entry; /* Process tbl registed in H_REGISTER_PROC_TBL */
>  SpaprPendingHpt *pending_hpt; /* in-progress resize */
>  
>  hwaddr rma_size;

-- 
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] hw/net: fsl_etsec: Tx padding length should exclude CRC

[Bin Meng]

Hi David,

On Mon, Mar 22, 2021 at 1:24 PM David Gibson
 wrote:
>
> On Mon, Mar 22, 2021 at 12:33:06PM +0800, Bin Meng wrote:
> > Hi David,
> >
> > On Mon, Mar 22, 2021 at 12:11 PM David Gibson
> >  wrote:
> > >
> > > On Tue, Mar 16, 2021 at 04:15:05PM +0800, Bin Meng wrote:
> > > > As the comment of tx_padding_and_crc() says: "Never add CRC in QEMU",
> > > > min_frame_len should excluce CRC, so it should be 60 instead of 64.
> > >
> > > Sorry, your reasoning still isn't clear to me.  If qemu is not adding
> > > the CRC, what is?
> >
> > No one is padding CRC in QEMU. QEMU network backends pass payload
> > without CRC in between.
>
> Ok, but the CRCs must be added if the packets are bridged onto a real
> device, yes?  Where does that happen?

I've never used it like that before. What's the command line to test that?

> >
> > > Will it always append a CRC after this padding is complete?
> >
> > No.
>
> If that's true, then won't the packets still be shorter than expected
> if we only pad to 60 bytes?

In QEMU packets are transmitted without CRC between network backends,
and when a NIC receives a packet, the minimum required payload length
is 60 bytes without a CRC.

Regards,
Bin

Re: [PATCH] hw/net: fsl_etsec: Tx padding length should exclude CRC

[David Gibson]

On Mon, Mar 22, 2021 at 12:33:06PM +0800, Bin Meng wrote:
> Hi David,
> 
> On Mon, Mar 22, 2021 at 12:11 PM David Gibson
>  wrote:
> >
> > On Tue, Mar 16, 2021 at 04:15:05PM +0800, Bin Meng wrote:
> > > As the comment of tx_padding_and_crc() says: "Never add CRC in QEMU",
> > > min_frame_len should excluce CRC, so it should be 60 instead of 64.
> >
> > Sorry, your reasoning still isn't clear to me.  If qemu is not adding
> > the CRC, what is?
> 
> No one is padding CRC in QEMU. QEMU network backends pass payload
> without CRC in between.

Ok, but the CRCs must be added if the packets are bridged onto a real
device, yes?  Where does that happen?
> 
> > Will it always append a CRC after this padding is complete?
> 
> No.

If that's true, then won't the packets still be shorter than expected
if we only pad to 60 bytes?

-- 
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 1/5] tests: Use the normal yank code instead of stubs in relevant tests

[Thomas Huth]

On 22/03/2021 00.31, Lukas Straub wrote:

Use the normal yank code instead of stubs in relevant tests to
increase coverage and to ensure that registering and unregistering
of yank instances and functions is done correctly.

Signed-off-by: Lukas Straub 
---
  tests/qtest/meson.build | 6 +++---
  tests/unit/meson.build  | 4 ++--
  2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 66ee9fbf45..40e1f495f7 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -234,9 +234,9 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c']
  qtests = {
'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
'cdrom-test': files('boot-sector.c'),
-  'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1,
+  'dbus-vmstate-test': ['migration-helpers.c', dbus_vmstate1, 
'../../monitor/yank.c'],
'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
-  'migration-test': files('migration-helpers.c'),
+  'migration-test': ['migration-helpers.c', io, '../../monitor/yank.c'],
'pxe-test': files('boot-sector.c'),
'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: 
false).sources()],
'tpm-crb-swtpm-test': [io, tpmemu_files],


Is this really necessary for the qtests? I can understand the change for the 
unit tests, but the qtests are separate programs where I could not imagine 
that they use the yank functions in any way?


 Thomas


PS: Please add a proper description about the yank feature to either that 
yank.c file or to include/qemu/yank.h ... I had a hard time to find out what 
this code is all about until I finally looked up your cover letter of the 
original series on the mailing list.

Re: [PATCH] spapr: Assert DIMM unplug state in spapr_memory_unplug()

[David Gibson]

On Sat, Mar 13, 2021 at 08:23:31AM +0100, Greg Kurz wrote:
> spapr_memory_unplug() is the last step of the hot unplug sequence.
> It is indirectly called by:
> 
>  spapr_lmb_release()
>   hotplug_handler_unplug()
> 
> and spapr_lmb_release() already buys us that DIMM unplug state is
> present : it gets restored with spapr_recover_pending_dimm_state()
> if missing.
> 
> g_assert() that spapr_pending_dimm_unplugs_find() cannot return NULL
> in spapr_memory_unplug() to make this clear and silence Coverity.
> 
> Fixes: Coverity CID 1450767
> Signed-off-by: Greg Kurz 

Applied to ppc-for-6.0, thanks.

> ---
>  hw/ppc/spapr.c |3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d56418ca2942..73a06df3b1b1 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3660,6 +3660,9 @@ static void spapr_memory_unplug(HotplugHandler 
> *hotplug_dev, DeviceState *dev)
>  SpaprMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
>  SpaprDimmState *ds = spapr_pending_dimm_unplugs_find(spapr, 
> PC_DIMM(dev));
>  
> +/* We really shouldn't get this far without anything to unplug */
> +g_assert(ds);
> +
>  pc_dimm_unplug(PC_DIMM(dev), MACHINE(hotplug_dev));
>  qdev_unrealize(dev);
>  spapr_pending_dimm_unplugs_remove(spapr, ds);
> 
> 

-- 
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 v4 14/17] hw/ppc/pnv_core: Update hflags after setting msr

[David Gibson]

On Mon, Mar 15, 2021 at 12:46:12PM -0600, Richard Henderson wrote:
> Signed-off-by: Richard Henderson 

Applied to ppc-for-6.0, thanks.

> ---
>  hw/ppc/pnv_core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index bd2bf2e044..8c2a15a0fb 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -29,6 +29,7 @@
>  #include "hw/ppc/pnv_xscom.h"
>  #include "hw/ppc/xics.h"
>  #include "hw/qdev-properties.h"
> +#include "helper_regs.h"
>  
>  static const char *pnv_core_cpu_typename(PnvCore *pc)
>  {
> @@ -55,8 +56,8 @@ static void pnv_core_cpu_reset(PnvCore *pc, PowerPCCPU *cpu)
>  env->gpr[3] = PNV_FDT_ADDR;
>  env->nip = 0x10;
>  env->msr |= MSR_HVB; /* Hypervisor mode */
> -
>  env->spr[SPR_HRMOR] = pc->hrmor;
> +hreg_compute_hflags(env);
>  
>  pcc->intc_reset(pc->chip, cpu);
>  }

-- 
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 v4 12/17] target/ppc: Remove MSR_SA and MSR_AP from hflags

[David Gibson]

On Mon, Mar 15, 2021 at 12:46:10PM -0600, Richard Henderson wrote:
> Nothing within the translator -- or anywhere else for that
> matter -- checks MSR_SA or MSR_AP on the 602.  This may be
> a mistake.  However, for the moment, we need not record these
> bits in hflags.
> 
> This allows us to simplify HFLAGS_VSX computation by moving
> it to overlap with MSR_VSX.

This leans into the requirement that certain hflags and msr bits line
up, which makes me nervous.  Apart from that

Reviewed-by: David Gibson 

> 
> Signed-off-by: Richard Henderson 
> ---
>  target/ppc/cpu.h | 4 +---
>  target/ppc/helper_regs.c | 7 +++
>  2 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 07a4331eec..23ff16c154 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -599,14 +599,12 @@ enum {
>  HFLAGS_DR = 4,   /* MSR_DR */
>  HFLAGS_IR = 5,   /* MSR_IR */
>  HFLAGS_SPE = 6,  /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR 
> */
> -HFLAGS_VSX = 7,  /* from MSR_VSX if cpu has VSX; avoid overlap w/ MSR_AP 
> */
>  HFLAGS_TM = 8,   /* computed from MSR_TM */
>  HFLAGS_BE = 9,   /* MSR_BE -- from elsewhere on embedded ppc */
>  HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */
>  HFLAGS_GTSE = 11, /* computed from SPR_LPCR[GTSE] */
>  HFLAGS_FP = 13,  /* MSR_FP */
> -HFLAGS_SA = 22,  /* MSR_SA */
> -HFLAGS_AP = 23,  /* MSR_AP */
> +HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
>  HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
>  };
>  
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 8479789e24..d62921c322 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -95,8 +95,7 @@ void hreg_compute_hflags(CPUPPCState *env)
>  
>  /* Some bits come straight across from MSR. */
>  msr_mask = ((1 << MSR_LE) | (1 << MSR_PR) |
> -(1 << MSR_DR) | (1 << MSR_IR) |
> -(1 << MSR_FP) | (1 << MSR_SA) | (1 << MSR_AP));
> +(1 << MSR_DR) | (1 << MSR_IR) | (1 << MSR_FP));
>  
>  if (ppc_flags & POWERPC_FLAG_HID0_LE) {
>  /*
> @@ -133,8 +132,8 @@ void hreg_compute_hflags(CPUPPCState *env)
>  if (ppc_flags & POWERPC_FLAG_VRE) {
>  msr_mask |= 1 << MSR_VR;
>  }
> -if ((ppc_flags & POWERPC_FLAG_VSX) && (msr & (1 << MSR_VSX))) {
> -hflags |= 1 << HFLAGS_VSX;
> +if (ppc_flags & POWERPC_FLAG_VSX) {
> +msr_mask |= 1 << MSR_VSX;
>  }
>  if ((ppc_flags & POWERPC_FLAG_TM) && (msr & (1ull << MSR_TM))) {
>  hflags |= 1 << HFLAGS_TM;

-- 
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 v4 11/17] target/ppc: Put LPCR[GTSE] in hflags

[David Gibson]

On Mon, Mar 15, 2021 at 12:46:09PM -0600, Richard Henderson wrote:
> Because this bit was not in hflags, the privilege check
> for tlb instructions was essentially random.
> Recompute hflags when storing to LPCR.
> 
> Signed-off-by: Richard Henderson 

Ouch.  Unlike the others which come from ancient strata of qemu code,
this one is pretty recent, and demonstrates that I don't really
understand how hflags and TCG code generation work.  Anyway,

Reviewed-by: David Gibson 

> ---
>  target/ppc/cpu.h | 1 +
>  target/ppc/helper_regs.c | 3 +++
>  target/ppc/mmu-hash64.c  | 3 +++
>  target/ppc/translate.c   | 2 +-
>  4 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 2abaf56869..07a4331eec 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -603,6 +603,7 @@ enum {
>  HFLAGS_TM = 8,   /* computed from MSR_TM */
>  HFLAGS_BE = 9,   /* MSR_BE -- from elsewhere on embedded ppc */
>  HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */
> +HFLAGS_GTSE = 11, /* computed from SPR_LPCR[GTSE] */
>  HFLAGS_FP = 13,  /* MSR_FP */
>  HFLAGS_SA = 22,  /* MSR_SA */
>  HFLAGS_AP = 23,  /* MSR_AP */
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index c735540333..8479789e24 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -139,6 +139,9 @@ void hreg_compute_hflags(CPUPPCState *env)
>  if ((ppc_flags & POWERPC_FLAG_TM) && (msr & (1ull << MSR_TM))) {
>  hflags |= 1 << HFLAGS_TM;
>  }
> +if (env->spr[SPR_LPCR] & LPCR_GTSE) {
> +hflags |= 1 << HFLAGS_GTSE;
> +}
>  
>  #ifndef CONFIG_USER_ONLY
>  if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 0fabc10302..d517a99832 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -30,6 +30,7 @@
>  #include "exec/log.h"
>  #include "hw/hw.h"
>  #include "mmu-book3s-v3.h"
> +#include "helper_regs.h"
>  
>  /* #define DEBUG_SLB */
>  
> @@ -1125,6 +1126,8 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
>  CPUPPCState *env = >env;
>  
>  env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
> +/* The gtse bit affects hflags */
> +hreg_compute_hflags(env);
>  }
>  
>  void helper_store_lpcr(CPUPPCState *env, target_ulong val)
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index d48c554290..5e629291d3 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7908,7 +7908,7 @@ static void ppc_tr_init_disas_context(DisasContextBase 
> *dcbase, CPUState *cs)
>  ctx->altivec_enabled = (hflags >> HFLAGS_VR) & 1;
>  ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
>  ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
> -ctx->gtse = !!(env->spr[SPR_LPCR] & LPCR_GTSE);
> +ctx->gtse = (hflags >> HFLAGS_GTSE) & 1;
>  
>  ctx->singlestep_enabled = 0;
>  if ((hflags >> HFLAGS_SE) & 1) {

-- 
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 v4 13/17] target/ppc: Remove env->immu_idx and env->dmmu_idx

[David Gibson]

On Mon, Mar 15, 2021 at 12:46:11PM -0600, Richard Henderson wrote:
> We weren't recording MSR_GS in hflags, which means that BookE
> memory accesses were essentially random vs Guest State.
> 
> Instead of adding this bit directly, record the completed mmu
> indexes instead.  This makes it obvious that we are recording
> exactly the information that we need.
> 
> This also means that we can stop directly recording MSR_IR.

What still uses MSR_DR, that you can't also drop it?

> 
> Signed-off-by: Richard Henderson 

Reviewed-by: David Gibson 

> ---
>  target/ppc/cpu.h | 12 --
>  target/ppc/helper_regs.h |  1 -
>  target/ppc/helper_regs.c | 88 
>  target/ppc/mem_helper.c  |  2 +-
>  target/ppc/translate.c   |  6 +--
>  5 files changed, 55 insertions(+), 54 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 23ff16c154..2f8d7fa13c 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -597,7 +597,6 @@ enum {
>  HFLAGS_64 = 2,   /* computed from MSR_CE and MSR_SF */
>  HFLAGS_PR = 3,   /* MSR_PR */
>  HFLAGS_DR = 4,   /* MSR_DR */
> -HFLAGS_IR = 5,   /* MSR_IR */
>  HFLAGS_SPE = 6,  /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR 
> */
>  HFLAGS_TM = 8,   /* computed from MSR_TM */
>  HFLAGS_BE = 9,   /* MSR_BE -- from elsewhere on embedded ppc */
> @@ -606,6 +605,9 @@ enum {
>  HFLAGS_FP = 13,  /* MSR_FP */
>  HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
>  HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
> +
> +HFLAGS_IMMU_IDX = 26, /* 26..28 -- the composite immu_idx */
> +HFLAGS_DMMU_IDX = 29, /* 29..31 -- the composite dmmu_idx */
>  };
>  
>  
> /*/
> @@ -1130,8 +1132,6 @@ struct CPUPPCState {
>  /* These resources are used only in TCG */
>  uint32_t hflags;
>  target_ulong hflags_compat_nmsr; /* for migration compatibility */
> -int immu_idx; /* precomputed MMU index to speed up insn accesses */
> -int dmmu_idx; /* precomputed MMU index to speed up data accesses */
>  
>  /* Power management */
>  int (*check_pow)(CPUPPCState *env);
> @@ -1367,7 +1367,11 @@ int ppc_dcr_write(ppc_dcr_t *dcr_env, int dcrn, 
> uint32_t val);
>  #define MMU_USER_IDX 0
>  static inline int cpu_mmu_index(CPUPPCState *env, bool ifetch)
>  {
> -return ifetch ? env->immu_idx : env->dmmu_idx;
> +#ifdef CONFIG_USER_ONLY
> +return MMU_USER_IDX;
> +#else
> +return (env->hflags >> (ifetch ? HFLAGS_IMMU_IDX : HFLAGS_DMMU_IDX)) & 7;
> +#endif
>  }
>  
>  /* Compatibility modes */
> diff --git a/target/ppc/helper_regs.h b/target/ppc/helper_regs.h
> index 4148a442b3..42f26870b9 100644
> --- a/target/ppc/helper_regs.h
> +++ b/target/ppc/helper_regs.h
> @@ -21,7 +21,6 @@
>  #define HELPER_REGS_H
>  
>  void hreg_swap_gpr_tgpr(CPUPPCState *env);
> -void hreg_compute_mem_idx(CPUPPCState *env);
>  void hreg_compute_hflags(CPUPPCState *env);
>  void cpu_interrupt_exittb(CPUState *cs);
>  int hreg_store_msr(CPUPPCState *env, target_ulong value, int alter_hv);
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index d62921c322..b28037ca24 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -43,49 +43,6 @@ void hreg_swap_gpr_tgpr(CPUPPCState *env)
>  env->tgpr[3] = tmp;
>  }
>  
> -void hreg_compute_mem_idx(CPUPPCState *env)
> -{
> -/*
> - * This is our encoding for server processors. The architecture
> - * specifies that there is no such thing as userspace with
> - * translation off, however it appears that MacOS does it and some
> - * 32-bit CPUs support it. Weird...
> - *
> - *   0 = Guest User space virtual mode
> - *   1 = Guest Kernel space virtual mode
> - *   2 = Guest User space real mode
> - *   3 = Guest Kernel space real mode
> - *   4 = HV User space virtual mode
> - *   5 = HV Kernel space virtual mode
> - *   6 = HV User space real mode
> - *   7 = HV Kernel space real mode
> - *
> - * For BookE, we need 8 MMU modes as follow:
> - *
> - *  0 = AS 0 HV User space
> - *  1 = AS 0 HV Kernel space
> - *  2 = AS 1 HV User space
> - *  3 = AS 1 HV Kernel space
> - *  4 = AS 0 Guest User space
> - *  5 = AS 0 Guest Kernel space
> - *  6 = AS 1 Guest User space
> - *  7 = AS 1 Guest Kernel space
> - */
> -if (env->mmu_model & POWERPC_MMU_BOOKE) {
> -env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1;
> -env->immu_idx += msr_is ? 2 : 0;
> -env->dmmu_idx += msr_ds ? 2 : 0;
> -env->immu_idx += msr_gs ? 4 : 0;
> -env->dmmu_idx += msr_gs ? 4 : 0;
> -} else {
> -env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1;
> -env->immu_idx += msr_ir ? 0 : 2;
> -env->dmmu_idx += msr_dr ? 0 : 2;
> -env->immu_idx += msr_hv ? 4 : 0;
> -env->dmmu_idx += msr_hv ? 4 : 0;
> -}

Re: [PATCH v4 15/17] hw/ppc/spapr_rtas: Update hflags after setting msr

[David Gibson]

On Mon, Mar 15, 2021 at 12:46:13PM -0600, Richard Henderson wrote:
> Signed-off-by: Richard Henderson 

Applied to ppc-for-6.0, thanks.

> ---
>  hw/ppc/spapr_rtas.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 8a79f9c628..6ec3e71757 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -51,6 +51,7 @@
>  #include "target/ppc/mmu-hash64.h"
>  #include "target/ppc/mmu-book3s-v3.h"
>  #include "migration/blocker.h"
> +#include "helper_regs.h"
>  
>  static void rtas_display_character(PowerPCCPU *cpu, SpaprMachineState *spapr,
> uint32_t token, uint32_t nargs,
> @@ -163,6 +164,7 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, 
> SpaprMachineState *spapr,
>  cpu_synchronize_state(CPU(newcpu));
>  
>  env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> +hreg_compute_hflags(env);
>  
>  /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
>  lpcr = env->spr[SPR_LPCR];

-- 
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: [PULL 0/8] Misc bugfixes for QEMU soft freeze

[Thomas Huth]

On 19/03/2021 15.39, Paolo Bonzini wrote:

The following changes since commit cf6b56d4f2107259f52413f979a1d474dad0c1e1:

   Merge remote-tracking branch 'remotes/philmd/tags/pflash-20210318' into 
staging (2021-03-18 23:04:41 +)

are available in the Git repository at:

   https://gitlab.com/bonzini/qemu.git tags/for-upstream

for you to fetch changes up to af05d7fa28010d4df9f5744514b16e71055d:

   tests/qtest: cleanup the testcase for bug 1878642 (2021-03-19 10:37:46 -0400)


* fixes for i386 TCG paging
* fixes for Hyper-V enlightenments
* avoid uninitialized variable warning


Paolo Bonzini (5):
   qom: use qemu_printf to print help for user-creatable objects


The qemu_printf patch that hit the master branch looks very funny. I think 
it should get reverted since that header file now gets included twice there?


 Thomas

Re: [PATCH] hw/net: fsl_etsec: Tx padding length should exclude CRC

[Bin Meng]

Hi David,

On Mon, Mar 22, 2021 at 12:11 PM David Gibson
 wrote:
>
> On Tue, Mar 16, 2021 at 04:15:05PM +0800, Bin Meng wrote:
> > As the comment of tx_padding_and_crc() says: "Never add CRC in QEMU",
> > min_frame_len should excluce CRC, so it should be 60 instead of 64.
>
> Sorry, your reasoning still isn't clear to me.  If qemu is not adding
> the CRC, what is?

No one is padding CRC in QEMU. QEMU network backends pass payload
without CRC in between.

> Will it always append a CRC after this padding is complete?

No.

Regards,
Bin

Re: [PATCH v4 10/17] target/ppc: Create helper_scv

[David Gibson]

On Mon, Mar 15, 2021 at 12:46:08PM -0600, Richard Henderson wrote:
> Perform the test against FSCR_SCV at runtime, in the helper.
> 
> This means we can remove the incorrect set against SCV in
> ppc_tr_init_disas_context and do not need to add an HFLAGS bit.
> 
> Signed-off-by: Richard Henderson 
> ---
>  target/ppc/helper.h  |  1 +
>  target/ppc/excp_helper.c |  9 +
>  target/ppc/translate.c   | 20 +++-
>  3 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 6a4dccf70c..513066d54d 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -13,6 +13,7 @@ DEF_HELPER_1(rfci, void, env)
>  DEF_HELPER_1(rfdi, void, env)
>  DEF_HELPER_1(rfmci, void, env)
>  #if defined(TARGET_PPC64)
> +DEF_HELPER_2(scv, noreturn, env, i32)
>  DEF_HELPER_2(pminsn, void, env, i32)
>  DEF_HELPER_1(rfid, void, env)
>  DEF_HELPER_1(rfscv, void, env)
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 85de7e6c90..5c95e0c103 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1130,6 +1130,15 @@ void helper_store_msr(CPUPPCState *env, target_ulong 
> val)
>  }
>  
>  #if defined(TARGET_PPC64)
> +void helper_scv(CPUPPCState *env, uint32_t lev)
> +{
> +if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
> +raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
> +} else {
> +raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
> +}
> +}
> +
>  void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
>  {
>  CPUState *cs;
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 7912495f28..d48c554290 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -173,7 +173,6 @@ struct DisasContext {
>  bool vsx_enabled;
>  bool spe_enabled;
>  bool tm_enabled;
> -bool scv_enabled;
>  bool gtse;
>  ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
>  int singlestep_enabled;
> @@ -4081,15 +4080,16 @@ static void gen_sc(DisasContext *ctx)
>  #if !defined(CONFIG_USER_ONLY)
>  static void gen_scv(DisasContext *ctx)
>  {
> -uint32_t lev;
> +uint32_t lev = (ctx->opcode >> 5) & 0x7F;
>  
> -if (unlikely(!ctx->scv_enabled)) {
> -gen_exception_err(ctx, POWERPC_EXCP_FU, FSCR_IC_SCV);
> -return;
> +/* Set the PC back to the faulting instruction. */
> +if (ctx->exception == POWERPC_EXCP_NONE) {
> +gen_update_nip(ctx, ctx->base.pc_next - 4);
>  }

I don't quite understand this.  Don't we need the NIP to be on the scv
instruction itself for the case where we get a facility unavailable
exception, but on the next instruction if we actually take the system
call?  This appears to be unconditional.

> +gen_helper_scv(cpu_env, tcg_constant_i32(lev));
>  
> -lev = (ctx->opcode >> 5) & 0x7F;
> -gen_exception_err(ctx, POWERPC_SYSCALL_VECTORED, lev);
> +/* This need not be exact, just not POWERPC_EXCP_NONE */
> +ctx->exception = POWERPC_SYSCALL_VECTORED;
>  }
>  #endif
>  #endif
> @@ -7907,12 +7907,6 @@ static void ppc_tr_init_disas_context(DisasContextBase 
> *dcbase, CPUState *cs)
>  ctx->spe_enabled = (hflags >> HFLAGS_SPE) & 1;
>  ctx->altivec_enabled = (hflags >> HFLAGS_VR) & 1;
>  ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
> -if ((env->flags & POWERPC_FLAG_SCV)
> -&& (env->spr[SPR_FSCR] & (1ull << FSCR_SCV))) {
> -ctx->scv_enabled = true;
> -} else {
> -ctx->scv_enabled = false;
> -}
>  ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
>  ctx->gtse = !!(env->spr[SPR_LPCR] & LPCR_GTSE);
>  

-- 
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 v4 05/17] target/ppc: Retain hflags_nmsr only for migration

[David Gibson]

On Mon, Mar 15, 2021 at 12:46:03PM -0600, Richard Henderson wrote:
> We have eliminated all normal uses of hflags_nmsr.  We need
> not even compute it except when we want to migrate.  Rename
> the field to emphasize this.
> 
> Remove the fixme comment for migrating access_type.  This value
> is only ever used with the current executing instruction, and
> is never live when the cpu is halted for migration.
> 
> Signed-off-by: Richard Henderson 

Applied to ppc-for-6.0, thanks.

> ---
>  target/ppc/cpu.h | 4 ++--
>  target/ppc/helper_regs.c | 2 --
>  target/ppc/machine.c | 9 ++---
>  3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 061d2eed1b..79c4033a42 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1105,8 +1105,8 @@ struct CPUPPCState {
>  #endif
>  
>  /* These resources are used only in QEMU core */
> -target_ulong hflags;  /* hflags is MSR & HFLAGS_MASK */
> -target_ulong hflags_nmsr; /* specific hflags, not coming from MSR */
> +target_ulong hflags;
> +target_ulong hflags_compat_nmsr; /* for migration compatibility */
>  int immu_idx; /* precomputed MMU index to speed up insn accesses */
>  int dmmu_idx; /* precomputed MMU index to speed up data accesses */
>  
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 95b9aca61f..a87e354ca2 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -104,8 +104,6 @@ void hreg_compute_hflags(CPUPPCState *env)
>   */
>  uint32_t le = extract32(env->spr[SPR_HID0], 3, 1);
>  env->hflags |= le << MSR_LE;
> -/* Retain for backward compatibility with migration. */
> -env->hflags_nmsr = le << MSR_LE;
>  }
>  }
>  
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index f6eeda9642..1f7a353c78 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -310,6 +310,10 @@ static int cpu_pre_save(void *opaque)
>  }
>  }
>  
> +/* Retain migration compatibility for pre 6.0 for 601 machines. */
> +env->hflags_compat_nmsr = (env->flags & POWERPC_FLAG_HID0_LE
> +   ? env->hflags & MSR_LE : 0);
> +
>  return 0;
>  }
>  
> @@ -829,9 +833,8 @@ const VMStateDescription vmstate_ppc_cpu = {
>  /* Supervisor mode architected state */
>  VMSTATE_UINTTL(env.msr, PowerPCCPU),
>  
> -/* Internal state */
> -VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
> -/* FIXME: access_type? */
> +/* Backward compatible internal state */
> +VMSTATE_UINTTL(env.hflags_compat_nmsr, PowerPCCPU),
>  
>  /* Sanity checking */
>  VMSTATE_UINTTL_TEST(mig_msr_mask, PowerPCCPU, cpu_pre_2_8_migration),

-- 
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 3/5] hw/pci-host/prep: Remove unuseful memory region mapping

[David Gibson]

On Fri, Mar 12, 2021 at 03:38:21PM -0500, Peter Xu wrote:
> On Fri, Mar 12, 2021 at 07:28:49PM +0100, Philippe Mathieu-Daudé wrote:
> > The pci_io_non_contiguous region is mapped on top of pci_io
> > with higher priority, but simply dispatch into this region
> > address space. Simplify by directly registering the former
> > region in place, and adapt the address space dispatch offsets.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé 
> > ---
> >  hw/pci-host/prep.c | 11 ---
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> > index 0a9162fba97..00a28c2d18c 100644
> > --- a/hw/pci-host/prep.c
> > +++ b/hw/pci-host/prep.c
> > @@ -159,8 +159,7 @@ static uint64_t raven_io_read(void *opaque, hwaddr addr,
> >  uint8_t buf[4];
> >  
> >  addr = raven_io_address(s, addr);
> > -address_space_read(>pci_io_as, addr + 0x8000,
> > -   MEMTXATTRS_UNSPECIFIED, buf, size);
> > +address_space_read(>pci_io_as, addr, MEMTXATTRS_UNSPECIFIED, buf, 
> > size);
> >  
> >  if (size == 1) {
> >  return buf[0];
> > @@ -191,8 +190,7 @@ static void raven_io_write(void *opaque, hwaddr addr,
> >  g_assert_not_reached();
> >  }
> >  
> > -address_space_write(>pci_io_as, addr + 0x8000,
> > -MEMTXATTRS_UNSPECIFIED, buf, size);
> > +address_space_write(>pci_io_as, addr, MEMTXATTRS_UNSPECIFIED, buf, 
> > size);
> 
> This changes access to s->pci_io_as, but below didn't change s->pci_io_as
> layout at all (below is about address_space_mem).  Is this intended?
> 
> >  }
> >  
> >  static const MemoryRegionOps raven_io_ops = {
> > @@ -294,9 +292,8 @@ static void raven_pcihost_initfn(Object *obj)
> >  address_space_init(>pci_io_as, >pci_io, "raven-io");
> >  
> >  /* CPU address space */
> > -memory_region_add_subregion(address_space_mem, 0x8000, >pci_io);
> > -memory_region_add_subregion_overlap(address_space_mem, 0x8000,
> > ->pci_io_non_contiguous, 1);
> > +memory_region_add_subregion(address_space_mem, 0x8000,
> > +>pci_io_non_contiguous);
> 
> I don't know any of this code at all... but it seems the two memory regions 
> are
> not identical in size:
> 
> memory_region_init(>pci_io, obj, "pci-io", 0x3f80);
> memory_region_init_io(>pci_io_non_contiguous, obj, _io_ops, s,
>   "pci-io-non-contiguous", 0x0080);
> 
> Then it seems the memory access dispatching to (0x0080, 0x3f80) would
> change too, from s->pci_io to nothing.  Raise this up too since I don't know
> either whether it's intended..

Right, it seems like this removes the mapping of s->pci_io entirely.

> 
> >  memory_region_add_subregion(address_space_mem, 0xc000, 
> > >pci_memory);
> >  pci_root_bus_new_inplace(>pci_bus, sizeof(s->pci_bus), DEVICE(obj), 
> > NULL,
> >   >pci_memory, >pci_io, 0, TYPE_PCI_BUS);
> 

-- 
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 v4 09/17] target/ppc: Put dbcr0 single-step bits into hflags

[David Gibson]

On Mon, Mar 15, 2021 at 12:46:07PM -0600, Richard Henderson wrote:
> Because these bits were not in hflags, the code generated
> for single-stepping on BookE was essentially random.
> Recompute hflags when storing to dbcr0.
> 
> Signed-off-by: Richard Henderson 

Reviewed-by: David Gibson 

> ---
>  target/ppc/helper_regs.c | 20 +++-
>  target/ppc/misc_helper.c |  3 +++
>  target/ppc/translate.c   | 11 ---
>  3 files changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 0a746bffd7..c735540333 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -107,11 +107,21 @@ void hreg_compute_hflags(CPUPPCState *env)
>  hflags |= le << MSR_LE;
>  }
>  
> -if (ppc_flags & POWERPC_FLAG_BE) {
> -msr_mask |= 1 << MSR_BE;
> -}
> -if (ppc_flags & POWERPC_FLAG_SE) {
> -msr_mask |= 1 << MSR_SE;
> +if (ppc_flags & POWERPC_FLAG_DE) {
> +target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0];
> +if (dbcr0 & DBCR0_ICMP) {
> +hflags |= 1 << HFLAGS_SE;
> +}
> +if (dbcr0 & DBCR0_BRT) {
> +hflags |= 1 << HFLAGS_BE;
> +}
> +} else {
> +if (ppc_flags & POWERPC_FLAG_BE) {
> +msr_mask |= 1 << MSR_BE;
> +}
> +if (ppc_flags & POWERPC_FLAG_SE) {
> +msr_mask |= 1 << MSR_SE;
> +}
>  }
>  
>  if (msr_is_64bit(env, msr)) {
> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> index b04b4d7c6e..a5ee1fd63c 100644
> --- a/target/ppc/misc_helper.c
> +++ b/target/ppc/misc_helper.c
> @@ -215,6 +215,9 @@ void helper_store_403_pbr(CPUPPCState *env, uint32_t num, 
> target_ulong value)
>  
>  void helper_store_40x_dbcr0(CPUPPCState *env, target_ulong val)
>  {
> +/* Bits 26 & 27 affect single-stepping */
> +hreg_compute_hflags(env);
> +/* Bits 28 & 29 affect reset or shutdown. */
>  store_40x_dbcr0(env, val);
>  }
>  
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index a85b890bb0..7912495f28 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7923,17 +7923,6 @@ static void ppc_tr_init_disas_context(DisasContextBase 
> *dcbase, CPUState *cs)
>  if ((hflags >> HFLAGS_BE) & 1) {
>  ctx->singlestep_enabled |= CPU_BRANCH_STEP;
>  }
> -if ((env->flags & POWERPC_FLAG_DE) && msr_de) {
> -ctx->singlestep_enabled = 0;
> -target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0];
> -if (dbcr0 & DBCR0_ICMP) {
> -ctx->singlestep_enabled |= CPU_SINGLE_STEP;
> -}
> -if (dbcr0 & DBCR0_BRT) {
> -ctx->singlestep_enabled |= CPU_BRANCH_STEP;
> -}
> -
> -}
>  if (unlikely(ctx->base.singlestep_enabled)) {
>  ctx->singlestep_enabled |= GDBSTUB_SINGLE_STEP;
>  }

-- 
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 v4 03/17] target/ppc: Properly sync cpu state with new msr in cpu_load_old

[David Gibson]

On Mon, Mar 15, 2021 at 12:46:01PM -0600, Richard Henderson wrote:
> Match cpu_post_load in using ppc_store_msr to set all of
> the cpu state implied by the value of msr.  Do not restore
> hflags or hflags_nmsr, as we recompute them in ppc_store_msr.
> 
> Signed-off-by: Richard Henderson 

Applied to ppc-for-6.0, thanks.

> ---
>  target/ppc/machine.c | 13 +++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 283db1d28a..87d7bffb86 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -21,6 +21,7 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int 
> version_id)
>  int32_t slb_nr;
>  #endif
>  target_ulong xer;
> +target_ulong msr;
>  
>  for (i = 0; i < 32; i++) {
>  qemu_get_betls(f, >gpr[i]);
> @@ -111,11 +112,19 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int 
> version_id)
>  qemu_get_betls(f, >ivpr_mask);
>  qemu_get_betls(f, >hreset_vector);
>  qemu_get_betls(f, >nip);
> -qemu_get_betls(f, >hflags);
> -qemu_get_betls(f, >hflags_nmsr);
> +qemu_get_sbetl(f); /* Discard unused hflags */
> +qemu_get_sbetl(f); /* Discard unused hflags_nmsr */
>  qemu_get_sbe32(f); /* Discard unused mmu_idx */
>  qemu_get_sbe32(f); /* Discard unused power_mode */
>  
> +/*
> + * Invalidate all supported msr bits except MSR_TGPR/MSR_HVB
> + * before restoring.  Note that this recomputes hflags and mem_idx.
> + */
> +msr = env->msr;
> +env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
> +ppc_store_msr(env, msr);
> +
>  /* Recompute mmu indices */
>  hreg_compute_mem_idx(env);
>  

-- 
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 4/5] hw/pci-host/prep: Do not directly map bus-master region onto main bus

[David Gibson]

On Fri, Mar 12, 2021 at 07:28:50PM +0100, Philippe Mathieu-Daudé wrote:
> The raven bus-master memory region is exposed as an AddressSpace.
> AddressSpaces root MemoryRegion must not be mapped into other
> MemoryRegion, therefore map the region using its alias.
> 
> Signed-off-by: Philippe Mathieu-Daudé 

Acked-by: David Gibson 

> ---
>  hw/pci-host/prep.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index 00a28c2d18c..6eaf9242bd8 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -295,8 +295,6 @@ static void raven_pcihost_initfn(Object *obj)
>  memory_region_add_subregion(address_space_mem, 0x8000,
>  >pci_io_non_contiguous);
>  memory_region_add_subregion(address_space_mem, 0xc000, 
> >pci_memory);
> -pci_root_bus_new_inplace(>pci_bus, sizeof(s->pci_bus), DEVICE(obj), 
> NULL,
> - >pci_memory, >pci_io, 0, TYPE_PCI_BUS);
>  
>  /* Bus master address space */
>  memory_region_init(>bm, obj, "bm-raven", 4 * GiB);
> @@ -308,6 +306,10 @@ static void raven_pcihost_initfn(Object *obj)
>  memory_region_add_subregion(>bm, 0 , >bm_pci_memory_alias);
>  memory_region_add_subregion(>bm, 0x8000, >bm_ram_alias);
>  address_space_init(>bm_as, >bm, "raven-bm");
> +
> +pci_root_bus_new_inplace(>pci_bus, sizeof(s->pci_bus), DEVICE(obj), 
> NULL,
> + >bm_pci_memory_alias, >pci_io, 0,
> + TYPE_PCI_BUS);
>  pci_setup_iommu(>pci_bus, raven_pcihost_set_iommu, s);
>  
>  h->bus = >pci_bus;

-- 
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 v4 06/17] target/ppc: Fix comment for MSR_FE{0,1}

[David Gibson]

On Mon, Mar 15, 2021 at 12:46:04PM -0600, Richard Henderson wrote:
> As per hreg_compute_hflags:
> 
>   We 'forget' FE0 & FE1: we'll never generate imprecise exceptions
> 
> remove the hflags marker from the respective comments.
> 
> Signed-off-by: Richard Henderson 

Applied to ppc-for-6.0, thanks.

> ---
>  target/ppc/cpu.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 79c4033a42..fd13489dce 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -322,13 +322,13 @@ typedef struct ppc_v3_pate_t {
>  #define MSR_PR   14 /* Problem state  hflags 
> */
>  #define MSR_FP   13 /* Floating point available   hflags 
> */
>  #define MSR_ME   12 /* Machine check interrupt enable
> */
> -#define MSR_FE0  11 /* Floating point exception mode 0hflags 
> */
> +#define MSR_FE0  11 /* Floating point exception mode 0   
> */
>  #define MSR_SE   10 /* Single-step trace enable x hflags 
> */
>  #define MSR_DWE  10 /* Debug wait enable on 405 x
> */
>  #define MSR_UBLE 10 /* User BTB lock enable on e500 x
> */
>  #define MSR_BE   9  /* Branch trace enable  x hflags 
> */
>  #define MSR_DE   9  /* Debug interrupts enable on embedded PowerPC  x
> */
> -#define MSR_FE1  8  /* Floating point exception mode 1hflags 
> */
> +#define MSR_FE1  8  /* Floating point exception mode 1   
> */
>  #define MSR_AL   7  /* AL bit on POWER   
> */
>  #define MSR_EP   6  /* Exception prefix on 601   
> */
>  #define MSR_IR   5  /* Instruction relocate  
> */

-- 
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] hw/net: fsl_etsec: Tx padding length should exclude CRC

[David Gibson]

On Tue, Mar 16, 2021 at 04:15:05PM +0800, Bin Meng wrote:
> As the comment of tx_padding_and_crc() says: "Never add CRC in QEMU",
> min_frame_len should excluce CRC, so it should be 60 instead of 64.

Sorry, your reasoning still isn't clear to me.  If qemu is not adding
the CRC, what is?  Will it always append a CRC after this padding is
complete?

> 
> Signed-off-by: Bin Meng 
> ---
> 
>  hw/net/fsl_etsec/rings.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index d6be0d7d18..8f08446415 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -259,7 +259,7 @@ static void process_tx_bd(eTSEC *etsec,
>  || etsec->regs[MACCFG2].value & MACCFG2_PADCRC) {
>  
>  /* Padding and CRC (Padding implies CRC) */
> -tx_padding_and_crc(etsec, 64);
> +tx_padding_and_crc(etsec, 60);
>  
>  } else if (etsec->first_bd.flags & BD_TX_TC
> || etsec->regs[MACCFG2].value & MACCFG2_CRC_EN) {

-- 
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 v4 08/17] target/ppc: Reduce env->hflags to uint32_t

[David Gibson]

On Mon, Mar 15, 2021 at 12:46:06PM -0600, Richard Henderson wrote:
> It will be stored in tb->flags, which is also uint32_t,
> so let's use the correct size.
> 
> Signed-off-by: Richard Henderson 

Reviewed-by: David Gibson 

> ---
>  target/ppc/cpu.h | 4 ++--
>  target/ppc/misc_helper.c | 2 +-
>  target/ppc/translate.c   | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 39f35b570c..2abaf56869 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1128,8 +1128,8 @@ struct CPUPPCState {
>  bool resume_as_sreset;
>  #endif
>  
> -/* These resources are used only in QEMU core */
> -target_ulong hflags;
> +/* These resources are used only in TCG */
> +uint32_t hflags;
>  target_ulong hflags_compat_nmsr; /* for migration compatibility */
>  int immu_idx; /* precomputed MMU index to speed up insn accesses */
>  int dmmu_idx; /* precomputed MMU index to speed up data accesses */
> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> index 63e3147eb4..b04b4d7c6e 100644
> --- a/target/ppc/misc_helper.c
> +++ b/target/ppc/misc_helper.c
> @@ -199,7 +199,7 @@ void helper_store_hid0_601(CPUPPCState *env, target_ulong 
> val)
>  if ((val ^ hid0) & 0x0008) {
>  /* Change current endianness */
>  hreg_compute_hflags(env);
> -qemu_log("%s: set endianness to %c => " TARGET_FMT_lx "\n", __func__,
> +qemu_log("%s: set endianness to %c => %08x\n", __func__,
>   val & 0x8 ? 'l' : 'b', env->hflags);
>  }
>  }
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index a9325a12e5..a85b890bb0 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7657,7 +7657,7 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int 
> flags)
>   env->nip, env->lr, env->ctr, cpu_read_xer(env),
>   cs->cpu_index);
>  qemu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx "  HF "
> - TARGET_FMT_lx " iidx %d didx %d\n",
> + "%08x iidx %d didx %d\n",
>   env->msr, env->spr[SPR_HID0],
>   env->hflags, env->immu_idx, env->dmmu_idx);
>  #if !defined(NO_TIMER_DUMP)

-- 
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 v4 02/17] target/ppc: Move 601 hflags adjustment to hreg_compute_hflags

[David Gibson]

On Mon, Mar 15, 2021 at 12:46:00PM -0600, Richard Henderson wrote:
> Keep all hflags computation in one place, as this will be
> especially important later.
> 
> Introduce a new POWERPC_FLAG_HID0_LE bit to indicate when
> LE should be taken from HID0.  This appears to be set if
> and only if POWERPC_FLAG_RTC_CLK is set, but we're not
> short of bits and having both names will avoid confusion.
> 
> Note that this was the only user of hflags_nmsr, so we can
> perform a straight assignment rather than mask and set.
> 
> Signed-off-by: Richard Henderson 

Applied to ppc-for-6.0, thanks.

> ---
>  target/ppc/cpu.h|  2 ++
>  target/ppc/helper_regs.c| 13 +++--
>  target/ppc/misc_helper.c|  8 +++-
>  target/ppc/translate_init.c.inc |  4 ++--
>  4 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index e73416da68..061d2eed1b 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -581,6 +581,8 @@ enum {
>  POWERPC_FLAG_TM   = 0x0010,
>  /* Has SCV (ISA 3.00)
> */
>  POWERPC_FLAG_SCV  = 0x0020,
> +/* Has HID0 for LE bit (601) 
> */
> +POWERPC_FLAG_HID0_LE  = 0x0040,
>  };
>  
>  
> /*/
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 5e18232b84..95b9aca61f 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -96,8 +96,17 @@ void hreg_compute_hflags(CPUPPCState *env)
>  hflags_mask |= (1ULL << MSR_CM) | (1ULL << MSR_SF) | MSR_HVB;
>  hreg_compute_mem_idx(env);
>  env->hflags = env->msr & hflags_mask;
> -/* Merge with hflags coming from other registers */
> -env->hflags |= env->hflags_nmsr;
> +
> +if (env->flags & POWERPC_FLAG_HID0_LE) {
> +/*
> + * Note that MSR_LE is not set in env->msr_mask for this cpu,
> + * and so will never be set in msr or hflags at this point.
> + */
> +uint32_t le = extract32(env->spr[SPR_HID0], 3, 1);
> +env->hflags |= le << MSR_LE;
> +/* Retain for backward compatibility with migration. */
> +env->hflags_nmsr = le << MSR_LE;
> +}
>  }
>  
>  void cpu_interrupt_exittb(CPUState *cs)
> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> index 5d6e0de396..63e3147eb4 100644
> --- a/target/ppc/misc_helper.c
> +++ b/target/ppc/misc_helper.c
> @@ -194,16 +194,14 @@ void helper_store_hid0_601(CPUPPCState *env, 
> target_ulong val)
>  target_ulong hid0;
>  
>  hid0 = env->spr[SPR_HID0];
> +env->spr[SPR_HID0] = (uint32_t)val;
> +
>  if ((val ^ hid0) & 0x0008) {
>  /* Change current endianness */
> -env->hflags &= ~(1 << MSR_LE);
> -env->hflags_nmsr &= ~(1 << MSR_LE);
> -env->hflags_nmsr |= (1 << MSR_LE) & (((val >> 3) & 1) << MSR_LE);
> -env->hflags |= env->hflags_nmsr;
> +hreg_compute_hflags(env);
>  qemu_log("%s: set endianness to %c => " TARGET_FMT_lx "\n", __func__,
>   val & 0x8 ? 'l' : 'b', env->hflags);
>  }
> -env->spr[SPR_HID0] = (uint32_t)val;
>  }
>  
>  void helper_store_403_pbr(CPUPPCState *env, uint32_t num, target_ulong value)
> diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
> index c03a7c4f52..049d76cfd1 100644
> --- a/target/ppc/translate_init.c.inc
> +++ b/target/ppc/translate_init.c.inc
> @@ -5441,7 +5441,7 @@ POWERPC_FAMILY(601)(ObjectClass *oc, void *data)
>  pcc->excp_model = POWERPC_EXCP_601;
>  pcc->bus_model = PPC_FLAGS_INPUT_6xx;
>  pcc->bfd_mach = bfd_mach_ppc_601;
> -pcc->flags = POWERPC_FLAG_SE | POWERPC_FLAG_RTC_CLK;
> +pcc->flags = POWERPC_FLAG_SE | POWERPC_FLAG_RTC_CLK | 
> POWERPC_FLAG_HID0_LE;
>  }
>  
>  #define POWERPC_MSRR_601v(0x1040ULL)
> @@ -5485,7 +5485,7 @@ POWERPC_FAMILY(601v)(ObjectClass *oc, void *data)
>  #endif
>  pcc->bus_model = PPC_FLAGS_INPUT_6xx;
>  pcc->bfd_mach = bfd_mach_ppc_601;
> -pcc->flags = POWERPC_FLAG_SE | POWERPC_FLAG_RTC_CLK;
> +pcc->flags = POWERPC_FLAG_SE | POWERPC_FLAG_RTC_CLK | 
> POWERPC_FLAG_HID0_LE;
>  }
>  
>  static void init_proc_602(CPUPPCState *env)

-- 
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 v4 01/17] target/ppc: Move helper_regs.h functions out-of-line

[David Gibson]

On Mon, Mar 15, 2021 at 12:45:59PM -0600, Richard Henderson wrote:
> Move the functions to a new file, helper_regs.c.
> 
> Note int_helper.c was relying on helper_regs.h to
> indirectly include qemu/log.h.
> 
> Signed-off-by: Richard Henderson 

Applied to ppc-for-6.0, thanks.

> ---
>  target/ppc/helper_regs.h | 184 ++--
>  target/ppc/helper_regs.c | 197 +++
>  target/ppc/int_helper.c  |   1 +
>  target/ppc/meson.build   |   1 +
>  4 files changed, 207 insertions(+), 176 deletions(-)
>  create mode 100644 target/ppc/helper_regs.c
> 
> diff --git a/target/ppc/helper_regs.h b/target/ppc/helper_regs.h
> index efcc903427..4148a442b3 100644
> --- a/target/ppc/helper_regs.h
> +++ b/target/ppc/helper_regs.h
> @@ -20,184 +20,16 @@
>  #ifndef HELPER_REGS_H
>  #define HELPER_REGS_H
>  
> -#include "qemu/main-loop.h"
> -#include "exec/exec-all.h"
> -#include "sysemu/kvm.h"
> +void hreg_swap_gpr_tgpr(CPUPPCState *env);
> +void hreg_compute_mem_idx(CPUPPCState *env);
> +void hreg_compute_hflags(CPUPPCState *env);
> +void cpu_interrupt_exittb(CPUState *cs);
> +int hreg_store_msr(CPUPPCState *env, target_ulong value, int alter_hv);
>  
> -/* Swap temporary saved registers with GPRs */
> -static inline void hreg_swap_gpr_tgpr(CPUPPCState *env)
> -{
> -target_ulong tmp;
> -
> -tmp = env->gpr[0];
> -env->gpr[0] = env->tgpr[0];
> -env->tgpr[0] = tmp;
> -tmp = env->gpr[1];
> -env->gpr[1] = env->tgpr[1];
> -env->tgpr[1] = tmp;
> -tmp = env->gpr[2];
> -env->gpr[2] = env->tgpr[2];
> -env->tgpr[2] = tmp;
> -tmp = env->gpr[3];
> -env->gpr[3] = env->tgpr[3];
> -env->tgpr[3] = tmp;
> -}
> -
> -static inline void hreg_compute_mem_idx(CPUPPCState *env)
> -{
> -/*
> - * This is our encoding for server processors. The architecture
> - * specifies that there is no such thing as userspace with
> - * translation off, however it appears that MacOS does it and some
> - * 32-bit CPUs support it. Weird...
> - *
> - *   0 = Guest User space virtual mode
> - *   1 = Guest Kernel space virtual mode
> - *   2 = Guest User space real mode
> - *   3 = Guest Kernel space real mode
> - *   4 = HV User space virtual mode
> - *   5 = HV Kernel space virtual mode
> - *   6 = HV User space real mode
> - *   7 = HV Kernel space real mode
> - *
> - * For BookE, we need 8 MMU modes as follow:
> - *
> - *  0 = AS 0 HV User space
> - *  1 = AS 0 HV Kernel space
> - *  2 = AS 1 HV User space
> - *  3 = AS 1 HV Kernel space
> - *  4 = AS 0 Guest User space
> - *  5 = AS 0 Guest Kernel space
> - *  6 = AS 1 Guest User space
> - *  7 = AS 1 Guest Kernel space
> - */
> -if (env->mmu_model & POWERPC_MMU_BOOKE) {
> -env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1;
> -env->immu_idx += msr_is ? 2 : 0;
> -env->dmmu_idx += msr_ds ? 2 : 0;
> -env->immu_idx += msr_gs ? 4 : 0;
> -env->dmmu_idx += msr_gs ? 4 : 0;
> -} else {
> -env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1;
> -env->immu_idx += msr_ir ? 0 : 2;
> -env->dmmu_idx += msr_dr ? 0 : 2;
> -env->immu_idx += msr_hv ? 4 : 0;
> -env->dmmu_idx += msr_hv ? 4 : 0;
> -}
> -}
> -
> -static inline void hreg_compute_hflags(CPUPPCState *env)
> -{
> -target_ulong hflags_mask;
> -
> -/* We 'forget' FE0 & FE1: we'll never generate imprecise exceptions */
> -hflags_mask = (1 << MSR_VR) | (1 << MSR_AP) | (1 << MSR_SA) |
> -(1 << MSR_PR) | (1 << MSR_FP) | (1 << MSR_SE) | (1 << MSR_BE) |
> -(1 << MSR_LE) | (1 << MSR_VSX) | (1 << MSR_IR) | (1 << MSR_DR);
> -hflags_mask |= (1ULL << MSR_CM) | (1ULL << MSR_SF) | MSR_HVB;
> -hreg_compute_mem_idx(env);
> -env->hflags = env->msr & hflags_mask;
> -/* Merge with hflags coming from other registers */
> -env->hflags |= env->hflags_nmsr;
> -}
> -
> -static inline void cpu_interrupt_exittb(CPUState *cs)
> -{
> -if (!kvm_enabled()) {
> -return;
> -}
> -
> -if (!qemu_mutex_iothread_locked()) {
> -qemu_mutex_lock_iothread();
> -cpu_interrupt(cs, CPU_INTERRUPT_EXITTB);
> -qemu_mutex_unlock_iothread();
> -} else {
> -cpu_interrupt(cs, CPU_INTERRUPT_EXITTB);
> -}
> -}
> -
> -static inline int hreg_store_msr(CPUPPCState *env, target_ulong value,
> - int alter_hv)
> -{
> -int excp;
> -#if !defined(CONFIG_USER_ONLY)
> -CPUState *cs = env_cpu(env);
> -#endif
> -
> -excp = 0;
> -value &= env->msr_mask;
> -#if !defined(CONFIG_USER_ONLY)
> -/* Neither mtmsr nor guest state can alter HV */
> -if (!alter_hv || !(env->msr & MSR_HVB)) {
> -value &= ~MSR_HVB;
> -value |= env->msr & MSR_HVB;
> -}
> -if (((value >> MSR_IR) & 1) != msr_ir ||
> -((value >> MSR_DR) & 1) != msr_dr) {
> -

Re: [PATCH v4 07/17] target/ppc: Disconnect hflags from MSR

[David Gibson]

On Mon, Mar 15, 2021 at 12:46:05PM -0600, Richard Henderson wrote:
> Copying flags directly from msr has drawbacks: (1) msr bits
> mean different things per cpu, (2) msr has 64 bits on 64 cpus
> while tb->flags has only 32 bits.
> 
> Create a enum to define these bits.  Document the origin of each bit.
> This fixes the truncation of env->hflags to tb->flags, because we no
> longer have hflags bits set above bit 31.
> 
> Most of the code in ppc_tr_init_disas_context is moved over to
> hreg_compute_hflags.  Some of it is simple extractions from msr,
> some requires examining other cpu flags.  Anything that is moved
> becomes a simple extract from hflags in ppc_tr_init_disas_context.
> 
> Several existing bugs are left in ppc_tr_init_disas_context, where
> additional changes are required -- to be addressed in future patches.
> 
> Remove a broken #if 0 block.
> 
> Reported-by: Ivan Warren 
> Signed-off-by: Richard Henderson 
> ---
>  target/ppc/cpu.h | 24 ++
>  target/ppc/helper_regs.c | 55 
>  target/ppc/translate.c   | 55 
>  3 files changed, 84 insertions(+), 50 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index fd13489dce..39f35b570c 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -585,6 +585,30 @@ enum {
>  POWERPC_FLAG_HID0_LE  = 0x0040,
>  };
>  
> +/*
> + * Bits for env->hflags.
> + *
> + * Most of these bits overlap with corresponding bits in MSR,
> + * but some come from other sources.  Be cautious when modifying.

Yeah.. I'm not sure "be cautious" is enough of a warning.  The exact
value of some but not all of these flags must equal that for the
corresponding MSR bits, which is terrifyingly subtle.

> + */
> +enum {
> +HFLAGS_LE = 0,   /* MSR_LE -- comes from elsewhere on 601 */
> +HFLAGS_HV = 1,   /* computed from MSR_HV and other state */
> +HFLAGS_64 = 2,   /* computed from MSR_CE and MSR_SF */
> +HFLAGS_PR = 3,   /* MSR_PR */
> +HFLAGS_DR = 4,   /* MSR_DR */
> +HFLAGS_IR = 5,   /* MSR_IR */
> +HFLAGS_SPE = 6,  /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR 
> */
> +HFLAGS_VSX = 7,  /* from MSR_VSX if cpu has VSX; avoid overlap w/ MSR_AP 
> */
> +HFLAGS_TM = 8,   /* computed from MSR_TM */
> +HFLAGS_BE = 9,   /* MSR_BE -- from elsewhere on embedded ppc */
> +HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */
> +HFLAGS_FP = 13,  /* MSR_FP */
> +HFLAGS_SA = 22,  /* MSR_SA */
> +HFLAGS_AP = 23,  /* MSR_AP */
> +HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
> +};
> +
>  
> /*/
>  /* Floating point status and control register
> */
>  #define FPSCR_DRN2   34 /* Decimal Floating-Point rounding control   
> */
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index a87e354ca2..0a746bffd7 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -18,6 +18,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "cpu.h"
>  #include "qemu/main-loop.h"
>  #include "exec/exec-all.h"
>  #include "sysemu/kvm.h"
> @@ -87,24 +88,56 @@ void hreg_compute_mem_idx(CPUPPCState *env)
>  
>  void hreg_compute_hflags(CPUPPCState *env)
>  {
> -target_ulong hflags_mask;
> +target_ulong msr = env->msr;
> +uint32_t ppc_flags = env->flags;
> +uint32_t hflags = 0;
> +uint32_t msr_mask;
>  
> -/* We 'forget' FE0 & FE1: we'll never generate imprecise exceptions */
> -hflags_mask = (1 << MSR_VR) | (1 << MSR_AP) | (1 << MSR_SA) |
> -(1 << MSR_PR) | (1 << MSR_FP) | (1 << MSR_SE) | (1 << MSR_BE) |
> -(1 << MSR_LE) | (1 << MSR_VSX) | (1 << MSR_IR) | (1 << MSR_DR);
> -hflags_mask |= (1ULL << MSR_CM) | (1ULL << MSR_SF) | MSR_HVB;
> -hreg_compute_mem_idx(env);
> -env->hflags = env->msr & hflags_mask;
> +/* Some bits come straight across from MSR. */
> +msr_mask = ((1 << MSR_LE) | (1 << MSR_PR) |
> +(1 << MSR_DR) | (1 << MSR_IR) |
> +(1 << MSR_FP) | (1 << MSR_SA) | (1 << MSR_AP));
>  
> -if (env->flags & POWERPC_FLAG_HID0_LE) {
> +if (ppc_flags & POWERPC_FLAG_HID0_LE) {
>  /*
>   * Note that MSR_LE is not set in env->msr_mask for this cpu,
> - * and so will never be set in msr or hflags at this point.
> + * and so will never be set in msr.
>   */
>  uint32_t le = extract32(env->spr[SPR_HID0], 3, 1);
> -env->hflags |= le << MSR_LE;
> +hflags |= le << MSR_LE;
>  }
> +
> +if (ppc_flags & POWERPC_FLAG_BE) {
> +msr_mask |= 1 << MSR_BE;
> +}
> +if (ppc_flags & POWERPC_FLAG_SE) {
> +msr_mask |= 1 << MSR_SE;
> +}
> +
> +if (msr_is_64bit(env, msr)) {
> +hflags |= 1 << HFLAGS_64;
> +}
> +if ((ppc_flags & POWERPC_FLAG_SPE) && (msr & (1 << MSR_SPE))) {

Re: [PATCH v4 04/17] target/ppc: Do not call hreg_compute_mem_idx after ppc_store_msr

[David Gibson]

On Mon, Mar 15, 2021 at 12:46:02PM -0600, Richard Henderson wrote:
> In ppc_store_msr we call hreg_compute_hflags, which itself
> calls hreg_compute_mem_idx.  Rely on ppc_store_msr to update
> everything required by the msr update.
> 
> Signed-off-by: Richard Henderson 

Applied to ppc-for-6.0.

> ---
>  target/ppc/machine.c | 7 +--
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 87d7bffb86..f6eeda9642 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -125,9 +125,6 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int 
> version_id)
>  env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
>  ppc_store_msr(env, msr);
>  
> -/* Recompute mmu indices */
> -hreg_compute_mem_idx(env);
> -
>  return 0;
>  }
>  
> @@ -418,14 +415,12 @@ static int cpu_post_load(void *opaque, int version_id)
>  
>  /*
>   * Invalidate all supported msr bits except MSR_TGPR/MSR_HVB
> - * before restoring
> + * before restoring.  Note that this recomputes hflags and mem_idx.
>   */
>  msr = env->msr;
>  env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
>  ppc_store_msr(env, msr);
>  
> -hreg_compute_mem_idx(env);
> -
>  return 0;
>  }
>  

-- 
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] hw/net: fsl_etsec: Tx padding length should exclude CRC

[David Gibson]

On Mon, Mar 22, 2021 at 09:29:12AM +0800, Bin Meng wrote:
> On Tue, Mar 16, 2021 at 4:15 PM Bin Meng  wrote:
> >
> > As the comment of tx_padding_and_crc() says: "Never add CRC in QEMU",
> > min_frame_len should excluce CRC, so it should be 60 instead of 64.
> >
> > Signed-off-by: Bin Meng 
> > ---
> >
> >  hw/net/fsl_etsec/rings.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> 
> Ping?

Sorry, I was away on holiday last week.

-- 
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: [RFC PATCH] i386: Add ratelimit for bus locks acquired in guest

[Chenyi Qiang]

On 3/19/2021 8:37 PM, Marcelo Tosatti wrote:

On Fri, Mar 19, 2021 at 10:59:20AM +0800, Chenyi Qiang wrote:

Hi Marcelo,

Thank you for your comment.

On 3/19/2021 1:32 AM, Marcelo Tosatti wrote:

On Wed, Mar 17, 2021 at 04:47:09PM +0800, Chenyi Qiang wrote:

Virtual Machines can exploit bus locks to degrade the performance of
system. To address this kind of performance DOS attack, bus lock VM exit
is introduced in KVM and it will report the bus locks detected in guest,
which can help userspace to enforce throttling policies.




The availability of bus lock VM exit can be detected through the
KVM_CAP_X86_BUS_LOCK_EXIT. The returned bitmap contains the potential
policies supported by KVM. The field KVM_BUS_LOCK_DETECTION_EXIT in
bitmap is the only supported strategy at present. It indicates that KVM
will exit to userspace to handle the bus locks.

This patch adds a ratelimit on the bus locks acquired in guest as a
mitigation policy.

Introduce a new field "bld" to record the limited speed of bus locks in
target VM. The user can specify it through the "bus-lock-detection"
as a machine property. In current implementation, the default value of
the speed is 0 per second, which means no restriction on the bus locks.

Ratelimit enforced in data transmission uses a time slice of 100ms to
get smooth output during regular operations in block jobs. As for
ratelimit on bus lock detection, simply set the ratelimit interval to 1s
and restrict the quota of bus lock occurrence to the value of "bld". A
potential alternative is to introduce the time slice as a property
which can help the user achieve more precise control.

The detail of Bus lock VM exit can be found in spec:
https://software.intel.com/content/www/us/en/develop/download/intel-architecture-instruction-set-extensions-programming-reference.html

Signed-off-by: Chenyi Qiang 
---
   hw/i386/x86.c |  6 ++
   include/hw/i386/x86.h |  7 +++
   target/i386/kvm/kvm.c | 44 +++
   3 files changed, 57 insertions(+)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 7865660e2c..a70a259e97 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1209,6 +1209,12 @@ static void x86_machine_initfn(Object *obj)
   x86ms->acpi = ON_OFF_AUTO_AUTO;
   x86ms->smp_dies = 1;
   x86ms->pci_irq_mask = ACPI_BUILD_PCI_IRQS;
+x86ms->bld = 0;
+
+object_property_add_uint64_ptr(obj, "bus-lock-detection",
+   >bld, OBJ_PROP_FLAG_READWRITE);
+object_property_set_description(obj, "bus-lock-detection",
+"Bus lock detection ratelimit");
   }
   static void x86_machine_class_init(ObjectClass *oc, void *data)
diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index 56080bd1fb..1f0ffbcfb9 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -72,6 +72,13 @@ struct X86MachineState {
* will be translated to MSI messages in the address space.
*/
   AddressSpace *ioapic_as;
+
+/*
+ * ratelimit enforced on detected bus locks, the default value
+ * is 0 per second
+ */
+uint64_t bld;
+RateLimit bld_limit;
   };
   #define X86_MACHINE_SMM  "smm"
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index c8d61daf68..724862137d 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -130,6 +130,8 @@ static bool has_msr_mcg_ext_ctl;
   static struct kvm_cpuid2 *cpuid_cache;
   static struct kvm_msr_list *kvm_feature_msrs;
+#define SLICE_TIME 10ULL /* ns */
+
   int kvm_has_pit_state2(void)
   {
   return has_pit_state2;
@@ -2267,6 +2269,27 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
   }
   }
+if (object_dynamic_cast(OBJECT(ms), TYPE_X86_MACHINE)) {
+X86MachineState *x86ms = X86_MACHINE(ms);
+
+if (x86ms->bld > 0) {
+ret = kvm_check_extension(s, KVM_CAP_X86_BUS_LOCK_EXIT);
+if (!(ret & KVM_BUS_LOCK_DETECTION_EXIT)) {
+error_report("kvm: bus lock detection unsupported");
+return -ENOTSUP;
+}
+ret = kvm_vm_enable_cap(s, KVM_CAP_X86_BUS_LOCK_EXIT, 0,
+KVM_BUS_LOCK_DETECTION_EXIT);
+if (ret < 0) {
+error_report("kvm: Failed to enable bus lock detection cap: 
%s",
+ strerror(-ret));
+return ret;
+}
+
+ratelimit_set_speed(>bld_limit, x86ms->bld, SLICE_TIME);
+}
+}
+
   return 0;
   }
@@ -4221,6 +4244,18 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
   }
   }
+static void kvm_rate_limit_on_bus_lock(void)
+{
+MachineState *ms = MACHINE(qdev_get_machine());
+X86MachineState *x86ms = X86_MACHINE(ms);
+
+uint64_t delay_ns = ratelimit_calculate_delay(>bld_limit, 1);
+
+if (delay_ns) {
+g_usleep(delay_ns / SCALE_US);
+}
+}


Hi,

Can't see a use-case where the throttling is very useful: 

Re: [RFC 1/1] target/riscv: add support of RNMI

[Frank Chang]

On Fri, Mar 19, 2021 at 9:29 PM Alistair Francis 
wrote:

> On Tue, Mar 9, 2021 at 2:30 AM  wrote:
> >
> > From: Frank Chang 
> >
> > Signed-off-by: Frank Chang 
>
> I had a quick look and this looks fine. I haven't compared it to the
> spec yet though.
>
> When you send the patch series do you mind splitting it up a bit more?
> It just makes it easier to review.
>
> Alistair
>

Sure, will do that.

Thanks,
Frank Chang


>
> > ---
> >  target/riscv/cpu.c| 40 +
> >  target/riscv/cpu.h| 16 -
> >  target/riscv/cpu_bits.h   | 19 ++
> >  target/riscv/cpu_helper.c | 47 +--
> >  target/riscv/csr.c| 59 +++
> >  target/riscv/helper.h |  1 +
> >  target/riscv/insn32.decode|  3 +
> >  .../riscv/insn_trans/trans_privileged.c.inc   | 13 
> >  target/riscv/op_helper.c  | 31 ++
> >  9 files changed, 224 insertions(+), 5 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index ddea8fbeeb3..07ea2105200 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -137,6 +137,14 @@ static void set_feature(CPURISCVState *env, int
> feature)
> >  env->features |= (1ULL << feature);
> >  }
> >
> > +static void set_rnmi_vectors(CPURISCVState *env, int irqvec, int
> excpvec)
> > +{
> > +#ifndef CONFIG_USER_ONLY
> > +env->rnmi_irqvec = irqvec;
> > +env->rnmi_excpvec = excpvec;
> > +#endif
> > +}
> > +
> >  static void set_resetvec(CPURISCVState *env, int resetvec)
> >  {
> >  #ifndef CONFIG_USER_ONLY
> > @@ -372,6 +380,23 @@ static void riscv_cpu_disas_set_info(CPUState *s,
> disassemble_info *info)
> >  }
> >  }
> >
> > +#ifndef CONFIG_USER_ONLY
> > +static void riscv_cpu_set_rnmi(void *opaque, int irq, int level)
> > +{
> > +RISCVCPU *cpu = opaque;
> > +CPURISCVState *env = >env;
> > +CPUState *cs = CPU(cpu);
> > +
> > +if (level) {
> > +env->nmip |= 1 << irq;
> > +cpu_interrupt(cs, CPU_INTERRUPT_RNMI);
> > +} else {
> > +env->nmip &= ~(1 << irq);
> > +cpu_reset_interrupt(cs, CPU_INTERRUPT_RNMI);
> > +}
> > +}
> > +#endif
> > +
> >  static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> >  {
> >  CPUState *cs = CPU(dev);
> > @@ -415,6 +440,16 @@ static void riscv_cpu_realize(DeviceState *dev,
> Error **errp)
> >
> >  set_resetvec(env, cpu->cfg.resetvec);
> >
> > +if (cpu->cfg.rnmi) {
> > +set_feature(env, RISCV_FEATURE_RNMI);
> > +set_rnmi_vectors(env, cpu->cfg.rnmi_irqvec,
> cpu->cfg.rnmi_excpvec);
> > +#ifndef CONFIG_USER_ONLY
> > +env->nmie = true;
> > +qdev_init_gpio_in_named(DEVICE(cpu), riscv_cpu_set_rnmi,
> > +"rnmi", TARGET_LONG_BITS);
> > +#endif
> > +}
> > +
> >  /* If only XLEN is set for misa, then set misa from properties */
> >  if (env->misa == RV32 || env->misa == RV64) {
> >  /* Do some ISA extension error checking */
> > @@ -554,6 +589,11 @@ static Property riscv_cpu_properties[] = {
> >  DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
> >  DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
> >  DEFINE_PROP_UINT64("resetvec", RISCVCPU, cfg.resetvec,
> DEFAULT_RSTVEC),
> > +DEFINE_PROP_BOOL("rnmi", RISCVCPU, cfg.rnmi, false),
> > +DEFINE_PROP_UINT64("rnmi_irqvec", RISCVCPU, cfg.rnmi_irqvec,
> > +   DEFAULT_RNMI_IRQVEC),
> > +DEFINE_PROP_UINT64("rnmi_excpvec", RISCVCPU, cfg.rnmi_excpvec,
> > +   DEFAULT_RNMI_EXCPVEC),
> >  DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 0edb2826a27..b9aa403dfec 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -80,7 +80,8 @@
> >  enum {
> >  RISCV_FEATURE_MMU,
> >  RISCV_FEATURE_PMP,
> > -RISCV_FEATURE_MISA
> > +RISCV_FEATURE_MISA,
> > +RISCV_FEATURE_RNMI,
> >  };
> >
> >  #define PRIV_VERSION_1_10_0 0x00011000
> > @@ -178,6 +179,16 @@ struct CPURISCVState {
> >  target_ulong mcause;
> >  target_ulong mtval;  /* since: priv-1.10.0 */
> >
> > +/* NMI */
> > +target_ulong mnscratch;
> > +target_ulong mnepc;
> > +target_ulong mncause; /* mncause without bit XLEN-1 set to 1 */
> > +target_ulong mnstatus;
> > +bool nmie;
> > +target_ulong nmip;
> > +target_ulong rnmi_irqvec;
> > +target_ulong rnmi_excpvec;
> > +
> >  /* Hypervisor CSRs */
> >  target_ulong hstatus;
> >  target_ulong hedeleg;
> > @@ -300,6 +311,9 @@ struct RISCVCPU {
> >  bool mmu;
> >  bool pmp;
> >  uint64_t resetvec;
> > +bool rnmi;
> > +uint64_t rnmi_irqvec;
> > +uint64_t rnmi_excpvec;
> >  } cfg;
> >  };
> >
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index 

Re: [RFC 0/1] target/riscv: add RNMI support

[Frank Chang]

On Fri, Mar 19, 2021 at 9:30 PM Alistair Francis 
wrote:

> On Tue, Mar 9, 2021 at 2:31 AM  wrote:
> >
> > From: Frank Chang 
> >
> > This patchset add suport of Resumable NMI (RNMI) in RISC-V.
> >
> > There are four new CSRs and one new instruction added to allow NMI to be
> > resumable in RISC-V, which are:
> >
> > =
> >   * mnscratch (0x350)
> >   * mnepc (0x351)
> >   * mncause   (0x352)
> >   * mnstatus  (0x353)
> > =
> >   * mnret: To return from RNMI interrupt/exception handler.
> > =
> >
> > RNMI also has higher priority than any other interrupts or exceptions
> > and cannot be disable by software.
> >
> > RNMI may be used to route to other devices such as Bus Error Unit or
> > Watchdog Timer in the future.
> >
> > The interrupt/exception trap handler addresses of RNMI are
> > implementation defined.
> >
> > The technical proposal of RNMI can be found:
> > https://lists.riscv.org/g/tech-privileged/message/421
> >
> > The port is available here:
> > https://github.com/sifive/qemu/tree/nmi-upstream-v1
> >
> > To enable RNMI feature, add cpu argument: 'rnmi=true' and specify
> > RNMI interrupt/exception handler addresses with 'rnmi_irqvec' and
> > 'rnmi_excpvec' arguments, e.g.
> > -cpu rv64,rnmi=true,rnmi_irqvec=0x2000,rnmi_excpvec=0x3000
>
> Can you include details about the software you are using and how I can
> test this?
>
> Alistair
>

Thanks, I'll include that in my next version patchset.

Frank Chang


>
> >
> > Frank Chang (1):
> >   target/riscv: add support of RNMI
> >
> >  target/riscv/cpu.c| 40 +
> >  target/riscv/cpu.h| 16 -
> >  target/riscv/cpu_bits.h   | 19 ++
> >  target/riscv/cpu_helper.c | 47 +--
> >  target/riscv/csr.c| 59 +++
> >  target/riscv/helper.h |  1 +
> >  target/riscv/insn32.decode|  3 +
> >  .../riscv/insn_trans/trans_privileged.c.inc   | 13 
> >  target/riscv/op_helper.c  | 31 ++
> >  9 files changed, 224 insertions(+), 5 deletions(-)
> >
> > --
> > 2.17.1
> >
> >
>

Re: [PATCH v2] target/ppc/kvm: Cache timebase frequency

[David Gibson]

On Wed, Mar 17, 2021 at 06:57:07PM +0100, Greg Kurz wrote:
> Each vCPU core exposes its timebase frequency in the DT. When running
> under KVM, this means parsing /proc/cpuinfo in order to get the timebase
> frequency of the host CPU.
> 
> The parsing appears to slow down the boot quite a bit with higher number
> of cores:
> 
> # of cores seconds spent in spapr_dt_cpus()
>   8  0.550122
>  16  1.342375
>  32  2.850316
>  64  5.922505
>  96  9.109224
> 128 12.245504
> 256 24.957236
> 384 37.389113
> 
> The timebase frequency of the host CPU is identical for all
> cores and it is an invariant for the VM lifetime. Cache it
> instead of doing the same expensive parsing again and again.
> 
> Rename kvmppc_get_tbfreq() to kvmppc_get_tbfreq_procfs() and
> rename the 'retval' variable to make it clear it is used as
> fallback only. Come up with a new version of kvmppc_get_tbfreq()
> that calls kvmppc_get_tbfreq_procfs() only once and keep the
> value in a static.
> 
> Zero is certainly not a valid value for the timebase frequency.
> Treat atoi() returning zero as another parsing error and return
> the fallback value instead. This allows kvmppc_get_tbfreq() to
> use zero as an indicator that kvmppc_get_tbfreq_procfs() hasn't
> been called yet.
> 
> With this patch applied:
> 
> 384 0.518382
> 
> Signed-off-by: Greg Kurz 

Applied to ppc-for-6.0, thanks.

> ---
> v2: - do the caching in a distinct function for clarity (Philippe)
> - rename 'retval' to 'tbfreq_fallback'
> - expand the changelog a bit
> ---
>  target/ppc/kvm.c |   25 +++--
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 298c1f882c67..104a308abb57 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1815,24 +1815,37 @@ static int read_cpuinfo(const char *field, char 
> *value, int len)
>  return ret;
>  }
>  
> -uint32_t kvmppc_get_tbfreq(void)
> +static uint32_t kvmppc_get_tbfreq_procfs(void)
>  {
>  char line[512];
>  char *ns;
> -uint32_t retval = NANOSECONDS_PER_SECOND;
> +uint32_t tbfreq_fallback = NANOSECONDS_PER_SECOND;
> +uint32_t tbfreq_procfs;
>  
>  if (read_cpuinfo("timebase", line, sizeof(line))) {
> -return retval;
> +return tbfreq_fallback;
>  }
>  
>  ns = strchr(line, ':');
>  if (!ns) {
> -return retval;
> +return tbfreq_fallback;
>  }
>  
> -ns++;
> +tbfreq_procfs = atoi(++ns);
> +
> +/* 0 is certainly not acceptable by the guest, return fallback value */
> +return tbfreq_procfs ? tbfreq_procfs : tbfreq_fallback;
> +}
> +
> +uint32_t kvmppc_get_tbfreq(void)
> +{
> +static uint32_t cached_tbfreq;
> +
> +if (!cached_tbfreq) {
> +cached_tbfreq = kvmppc_get_tbfreq_procfs();
> +}
>  
> -return atoi(ns);
> +return cached_tbfreq;
>  }
>  
>  bool kvmppc_get_host_serial(char **value)
> 
> 

-- 
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] hw/block: m25p80: Support fast read for SST flashes

[Bin Meng]

On Tue, Mar 16, 2021 at 9:39 AM Bin Meng  wrote:
>
> On Thu, Mar 11, 2021 at 4:18 PM Bin Meng  wrote:
> >
> > On Sat, Mar 6, 2021 at 2:01 PM Bin Meng  wrote:
> > >
> > > From: Bin Meng 
> > >
> > > Per SST25VF016B datasheet [1], SST flash requires a dummy byte after
> > > the address bytes. Note only SPI mode is supported by SST flashes.
> > >
> > > [1] http://ww1.microchip.com/downloads/en/devicedoc/s71271_04.pdf
> > >
> > > Signed-off-by: Bin Meng 
> > > Acked-by: Alistair Francis 
> > >
> > > ---
> > >
> > > Changes in v2:
> > > - rebase on qemu/master
> > >
> > >  hw/block/m25p80.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> >
> > Ping?
>
> Ping?

Ping?

Re: [PATCH] hw/net: fsl_etsec: Tx padding length should exclude CRC

[Bin Meng]

On Tue, Mar 16, 2021 at 4:15 PM Bin Meng  wrote:
>
> As the comment of tx_padding_and_crc() says: "Never add CRC in QEMU",
> min_frame_len should excluce CRC, so it should be 60 instead of 64.
>
> Signed-off-by: Bin Meng 
> ---
>
>  hw/net/fsl_etsec/rings.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Ping?

Re: [PATCH v5 00/12] net: Pad short frames for network backends

[Bin Meng]

On Wed, Mar 17, 2021 at 2:26 PM Bin Meng  wrote:
>
> The minimum Ethernet frame length is 60 bytes. For short frames with
> smaller length like ARP packets (only 42 bytes), on a real world NIC
> it can choose either padding its length to the minimum required 60
> bytes, or sending it out directly to the wire. Such behavior can be
> hardcoded or controled by a register bit. Similarly on the receive
> path, NICs can choose either dropping such short frames directly or
> handing them over to software to handle.
>
> On the other hand, for the network backends like SLiRP/TAP, they
> don't expose a way to control the short frame behavior. As of today
> they just send/receive data from/to the other end connected to them,
> which means any sized packet is acceptable. So they can send and
> receive short frames without any problem. It is observed that ARP
> packets sent from SLiRP/TAP are 42 bytes, and SLiRP/TAP just send
> these ARP packets to the other end which might be a NIC model that
> does not allow short frames to pass through.
>
> To provide better compatibility, for packets sent from QEMU network
> backends like SLiRP/TAP, we change to pad short frames before sending
> it out to the other end, if the other end does not forbid it via the
> nc->do_not_pad flag. This ensures a backend as an Ethernet sender
> does not violate the spec. But with this change, the behavior of
> dropping short frames from SLiRP/TAP interfaces in the NIC model
> cannot be emulated because it always receives a packet that is spec
> complaint. The capability of sending short frames from NIC models is
> still supported and short frames can still pass through SLiRP/TAP.
>
> This series should be able to fix the issue as reported with some
> NIC models before, that ARP requests get dropped, preventing the
> guest from becoming visible on the network. It was workarounded in
> these NIC models on the receive path, that when a short frame is
> received, it is padded up to 60 bytes.
>
> Changes in v5:
> - minor update on commit message
> - update the eth_pad_short_frame() comment
>

Ping?

[PATCH 3/5] chardev/char.c: Move object_property_try_add_child out of chardev_new

[Lukas Straub]

Move object_property_try_add_child out of chardev_new into it's
callers. This is a preparation for the next patches to fix yank
with the chardev-change case.

Signed-off-by: Lukas Straub 
---
 chardev/char.c | 42 --
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index 97cafd6849..1584865027 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -972,7 +972,9 @@ static Chardev *chardev_new(const char *id, const char 
*typename,

 qemu_char_open(chr, backend, _opened, _err);
 if (local_err) {
-goto end;
+error_propagate(errp, local_err);
+object_unref(obj);
+return NULL;
 }

 if (!chr->filename) {
@@ -982,22 +984,6 @@ static Chardev *chardev_new(const char *id, const char 
*typename,
 qemu_chr_be_event(chr, CHR_EVENT_OPENED);
 }

-if (id) {
-object_property_try_add_child(get_chardevs_root(), id, obj,
-  _err);
-if (local_err) {
-goto end;
-}
-object_unref(obj);
-}
-
-end:
-if (local_err) {
-error_propagate(errp, local_err);
-object_unref(obj);
-return NULL;
-}
-
 return chr;
 }

@@ -1006,6 +992,7 @@ Chardev *qemu_chardev_new(const char *id, const char 
*typename,
   GMainContext *gcontext,
   Error **errp)
 {
+Chardev *chr;
 g_autofree char *genid = NULL;

 if (!id) {
@@ -1013,7 +1000,19 @@ Chardev *qemu_chardev_new(const char *id, const char 
*typename,
 id = genid;
 }

-return chardev_new(id, typename, backend, gcontext, errp);
+chr = chardev_new(id, typename, backend, gcontext, errp);
+if (!chr) {
+return NULL;
+}
+
+if (!object_property_try_add_child(get_chardevs_root(), id, OBJECT(chr),
+   errp)) {
+object_unref(OBJECT(chr));
+return NULL;
+}
+object_unref(OBJECT(chr));
+
+return chr;
 }

 ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
@@ -1034,6 +1033,13 @@ ChardevReturn *qmp_chardev_add(const char *id, 
ChardevBackend *backend,
 return NULL;
 }

+if (!object_property_try_add_child(get_chardevs_root(), id, OBJECT(chr),
+   errp)) {
+object_unref(OBJECT(chr));
+return NULL;
+}
+object_unref(OBJECT(chr));
+
 ret = g_new0(ChardevReturn, 1);
 if (CHARDEV_IS_PTY(chr)) {
 ret->pty = g_strdup(chr->filename + 4);
--
2.30.2



pgpbdcww8fFeS.pgp
Description: OpenPGP digital signature

[PATCH 4/5] chardev/char.c: Always pass id to chardev_new

[Lukas Straub]

Always pass the id to chardev_new, since it is needed to register
the yank instance for the chardev. Also, after checking that
nothing calls chardev_new with id=NULL, assert() that id!=NULL.

This fixes a crash when using chardev-change to change a chardev
to chardev-socket, which attempts to register a yank instance.
This in turn tries to dereference the NULL-pointer.

Signed-off-by: Lukas Straub 
---
 chardev/char.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index 1584865027..ad416c0714 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -964,6 +964,7 @@ static Chardev *chardev_new(const char *id, const char 
*typename,
 bool be_opened = true;

 assert(g_str_has_prefix(typename, "chardev-"));
+assert(id);

 obj = object_new(typename);
 chr = CHARDEV(obj);
@@ -1092,12 +1093,11 @@ ChardevReturn *qmp_chardev_change(const char *id, 
ChardevBackend *backend,
 return NULL;
 }

-chr_new = chardev_new(NULL, object_class_get_name(OBJECT_CLASS(cc)),
+chr_new = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
   backend, chr->gcontext, errp);
 if (!chr_new) {
 return NULL;
 }
-chr_new->label = g_strdup(id);

 if (chr->be_open && !chr_new->be_open) {
 qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
--
2.30.2



pgpNcsDRPZBEt.pgp
Description: OpenPGP digital signature

[PATCH 1/5] tests: Use the normal yank code instead of stubs in relevant tests

[Lukas Straub]

Use the normal yank code instead of stubs in relevant tests to
increase coverage and to ensure that registering and unregistering
of yank instances and functions is done correctly.

Signed-off-by: Lukas Straub 
---
 tests/qtest/meson.build | 6 +++---
 tests/unit/meson.build  | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 66ee9fbf45..40e1f495f7 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -234,9 +234,9 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c']
 qtests = {
   'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
   'cdrom-test': files('boot-sector.c'),
-  'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1,
+  'dbus-vmstate-test': ['migration-helpers.c', dbus_vmstate1, 
'../../monitor/yank.c'],
   'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
-  'migration-test': files('migration-helpers.c'),
+  'migration-test': ['migration-helpers.c', io, '../../monitor/yank.c'],
   'pxe-test': files('boot-sector.c'),
   'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: 
false).sources()],
   'tpm-crb-swtpm-test': [io, tpmemu_files],
@@ -266,7 +266,7 @@ foreach dir : target_dirs
   endif
   qtest_env.set('G_TEST_DBUS_DAEMON', meson.source_root() / 
'tests/dbus-vmstate-daemon.sh')
   qtest_env.set('QTEST_QEMU_BINARY', './qemu-system-' + target_base)
-
+
   foreach test : target_qtests
 # Executables are shared across targets, declare them only the first time 
we
 # encounter them
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 4bfe4627ba..8ccf60af66 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -123,7 +123,7 @@ if have_system
 'test-util-sockets': ['socket-helpers.c'],
 'test-base64': [],
 'test-bufferiszero': [],
-'test-vmstate': [migration, io]
+'test-vmstate': [migration, io, '../../monitor/yank.c']
   }
   if 'CONFIG_INOTIFY1' in config_host
 tests += {'test-util-filemonitor': []}
@@ -135,7 +135,7 @@ if have_system
   if 'CONFIG_TSAN' not in config_host
 if 'CONFIG_POSIX' in config_host
 tests += {
-  'test-char': ['socket-helpers.c', qom, io, chardev]
+  'test-char': ['socket-helpers.c', qom, io, chardev, 
'../../monitor/yank.c']
 }
 endif

--
2.30.2



pgpOOytolxFIm.pgp
Description: OpenPGP digital signature

[PATCH 5/5] chardev: Fix yank with the chardev-change case

[Lukas Straub]

When changing from chardev-socket (which supports yank) to
chardev-socket again, it fails, because the new chardev attempts
to register a new yank instance. This in turn fails, as there
still is the yank instance from the current chardev. Also,
the old chardev shouldn't unregister the yank instance when it
is freed.

To fix this, now the new chardev only registers a yank instance if
the current chardev doesn't support yank and thus hasn't registered
one already. Also, when the old chardev is freed, it now only
unregisters the yank instance if the new chardev doesn't need it.

s->registered_yank is always true here, as chardev-change only works
on user-visible chardevs and those are guraranteed to register a
yank instance as they are initialized via
chardev_new()
 qemu_char_open()
  cc->open() (qmp_chardev_open_socket()).

Signed-off-by: Lukas Straub 
---
 chardev/char-socket.c  | 20 +---
 chardev/char.c | 32 +---
 include/chardev/char.h |  4 
 3 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index c8bced76b7..8186b6a205 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1119,7 +1119,13 @@ static void char_socket_finalize(Object *obj)
 }
 g_free(s->tls_authz);
 if (s->registered_yank) {
-yank_unregister_instance(CHARDEV_YANK_INSTANCE(chr->label));
+/*
+ * In the chardev-change special-case, we shouldn't unregister the yank
+ * instance, as it still may be needed.
+ */
+if (chr->yank_unregister_instance) {
+yank_unregister_instance(CHARDEV_YANK_INSTANCE(chr->label));
+}
 }

 qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
@@ -1421,8 +1427,14 @@ static void qmp_chardev_open_socket(Chardev *chr,
 qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
 }

-if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp)) {
-return;
+/*
+ * In the chardev-change special-case, we shouldn't register a new yank
+ * instance, as there already may be one.
+ */
+if (chr->yank_register_instance) {
+if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp)) {
+return;
+}
 }
 s->registered_yank = true;

@@ -1564,6 +1576,8 @@ static void char_socket_class_init(ObjectClass *oc, void 
*data)
 {
 ChardevClass *cc = CHARDEV_CLASS(oc);

+cc->supports_yank = true;
+
 cc->parse = qemu_chr_parse_socket;
 cc->open = qmp_chardev_open_socket;
 cc->chr_wait_connected = tcp_chr_wait_connected;
diff --git a/chardev/char.c b/chardev/char.c
index ad416c0714..245f8be201 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -39,6 +39,7 @@
 #include "qemu/option.h"
 #include "qemu/id.h"
 #include "qemu/coroutine.h"
+#include "qemu/yank.h"

 #include "chardev-internal.h"

@@ -266,6 +267,8 @@ static void char_init(Object *obj)
 {
 Chardev *chr = CHARDEV(obj);

+chr->yank_register_instance = true;
+chr->yank_unregister_instance = true;
 chr->logfd = -1;
 qemu_mutex_init(>chr_write_lock);

@@ -956,6 +959,7 @@ void qemu_chr_set_feature(Chardev *chr,
 static Chardev *chardev_new(const char *id, const char *typename,
 ChardevBackend *backend,
 GMainContext *gcontext,
+bool yank_register_instance,
 Error **errp)
 {
 Object *obj;
@@ -968,6 +972,7 @@ static Chardev *chardev_new(const char *id, const char 
*typename,

 obj = object_new(typename);
 chr = CHARDEV(obj);
+chr->yank_register_instance = yank_register_instance;
 chr->label = g_strdup(id);
 chr->gcontext = gcontext;

@@ -1001,7 +1006,7 @@ Chardev *qemu_chardev_new(const char *id, const char 
*typename,
 id = genid;
 }

-chr = chardev_new(id, typename, backend, gcontext, errp);
+chr = chardev_new(id, typename, backend, gcontext, true, errp);
 if (!chr) {
 return NULL;
 }
@@ -1029,7 +1034,7 @@ ChardevReturn *qmp_chardev_add(const char *id, 
ChardevBackend *backend,
 }

 chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
-  backend, NULL, errp);
+  backend, NULL, true, errp);
 if (!chr) {
 return NULL;
 }
@@ -1054,7 +1059,7 @@ ChardevReturn *qmp_chardev_change(const char *id, 
ChardevBackend *backend,
   Error **errp)
 {
 CharBackend *be;
-const ChardevClass *cc;
+const ChardevClass *cc, *cc_new;
 Chardev *chr, *chr_new;
 bool closed_sent = false;
 ChardevReturn *ret;
@@ -1088,13 +1093,20 @@ ChardevReturn *qmp_chardev_change(const char *id, 
ChardevBackend *backend,
 return NULL;
 }

-cc = char_get_class(ChardevBackendKind_str(backend->type), errp);
-if (!cc) {
+cc = CHARDEV_GET_CLASS(chr);
+cc_new = 

[PATCH 2/5] tests: Add tests for yank with the chardev-change

[Lukas Straub]

Add tests for yank with the chardev-change case.

Signed-off-by: Lukas Straub 
---
 MAINTAINERS|   1 +
 tests/unit/meson.build |   3 +-
 tests/unit/test-yank.c | 240 +
 3 files changed, 243 insertions(+), 1 deletion(-)
 create mode 100644 tests/unit/test-yank.c

diff --git a/MAINTAINERS b/MAINTAINERS
index aa024eed17..a8a7f0d1c2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2817,6 +2817,7 @@ F: monitor/yank.c
 F: stubs/yank.c
 F: include/qemu/yank.h
 F: qapi/yank.json
+F: tests/unit/test-yank.c

 COLO Framework
 M: zhanghailiang 
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 8ccf60af66..38e5dba920 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -123,7 +123,8 @@ if have_system
 'test-util-sockets': ['socket-helpers.c'],
 'test-base64': [],
 'test-bufferiszero': [],
-'test-vmstate': [migration, io, '../../monitor/yank.c']
+'test-vmstate': [migration, io, '../../monitor/yank.c'],
+'test-yank': ['socket-helpers.c', qom, io, chardev, '../../monitor/yank.c']
   }
   if 'CONFIG_INOTIFY1' in config_host
 tests += {'test-util-filemonitor': []}
diff --git a/tests/unit/test-yank.c b/tests/unit/test-yank.c
new file mode 100644
index 00..44f24c45a8
--- /dev/null
+++ b/tests/unit/test-yank.c
@@ -0,0 +1,240 @@
+#include "qemu/osdep.h"
+#include 
+
+#include "qemu/config-file.h"
+#include "qemu/module.h"
+#include "qemu/option.h"
+#include "qemu/sockets.h"
+#include "chardev/char-fe.h"
+#include "sysemu/sysemu.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-char.h"
+#include "qapi/qmp/qdict.h"
+#include "qom/qom-qobject.h"
+#include "io/channel-socket.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qapi-visit-sockets.h"
+#include "socket-helpers.h"
+#include "qapi/qapi-commands-yank.h"
+#include "qapi/qapi-types-yank.h"
+
+static int chardev_change(void *opaque)
+{
+return 0;
+}
+
+static bool is_yank_instance_registered(void)
+{
+YankInstanceList *list;
+bool ret;
+
+list = qmp_query_yank(_abort);
+
+ret = !!list;
+
+qapi_free_YankInstanceList(list);
+
+return ret;
+}
+
+static void char_change_to_yank_test(gconstpointer opaque)
+{
+SocketAddress *addr = (gpointer) opaque;
+Chardev *chr;
+CharBackend be;
+ChardevReturn *ret;
+QIOChannelSocket *ioc;
+
+/*
+ * Setup a listener socket and determine its address
+ * so we know the TCP port for the client later
+ */
+ioc = qio_channel_socket_new();
+g_assert_nonnull(ioc);
+qio_channel_socket_listen_sync(ioc, addr, 1, _abort);
+addr = qio_channel_socket_get_local_address(ioc, _abort);
+g_assert_nonnull(addr);
+
+ChardevBackend old_backend = { .type = CHARDEV_BACKEND_KIND_NULL };
+ChardevBackend new_backend = {
+.type = CHARDEV_BACKEND_KIND_SOCKET,
+.u.socket.data = &(ChardevSocket) {
+.addr = &(SocketAddressLegacy) {
+.type = SOCKET_ADDRESS_LEGACY_KIND_INET,
+.u.inet.data = >u.inet
+},
+.has_server = true,
+.server = false
+} };
+
+g_assert(!is_yank_instance_registered());
+
+ret = qmp_chardev_add("chardev", _backend, _abort);
+qapi_free_ChardevReturn(ret);
+chr = qemu_chr_find("chardev");
+g_assert_nonnull(chr);
+
+g_assert(!is_yank_instance_registered());
+
+qemu_chr_wait_connected(chr, _abort);
+qemu_chr_fe_init(, chr, _abort);
+/* allow chardev-change */
+qemu_chr_fe_set_handlers(, NULL, NULL,
+ NULL, chardev_change, NULL, NULL, true);
+
+ret = qmp_chardev_change("chardev", _backend, _abort);
+g_assert_nonnull(ret);
+g_assert(be.chr != chr);
+g_assert(is_yank_instance_registered());
+
+object_unparent(OBJECT(be.chr));
+object_unref(OBJECT(ioc));
+qapi_free_ChardevReturn(ret);
+}
+
+static void char_change_yank_to_yank_test(gconstpointer opaque)
+{
+SocketAddress *addr = (gpointer) opaque;
+Chardev *chr;
+CharBackend be;
+ChardevReturn *ret;
+QIOChannelSocket *ioc;
+
+/*
+ * Setup a listener socket and determine its address
+ * so we know the TCP port for the client later
+ */
+ioc = qio_channel_socket_new();
+g_assert_nonnull(ioc);
+qio_channel_socket_listen_sync(ioc, addr, 1, _abort);
+addr = qio_channel_socket_get_local_address(ioc, _abort);
+g_assert_nonnull(addr);
+
+ChardevBackend backend = {
+.type = CHARDEV_BACKEND_KIND_SOCKET,
+.u.socket.data = &(ChardevSocket) {
+.addr = &(SocketAddressLegacy) {
+.type = SOCKET_ADDRESS_LEGACY_KIND_INET,
+.u.inet.data = >u.inet
+},
+.has_server = true,
+.server = false
+} };
+
+g_assert(!is_yank_instance_registered());
+
+ret = qmp_chardev_add("chardev", , 

[PATCH 0/5] yank: Add chardev tests and fixes

[Lukas Straub]

Hello Everyone,
These patches increase test coverage for yank, add tests and fix bugs and
crashes in yank in combination with chardev-change.

Regards,
Lukas Straub

Based-on: <20210316135907.3646901-1-arm...@redhat.com>
([PATCH] yank: Avoid linking into executables that don't want it)

Alternatively:
https://github.com/Lukey3332/qemu.git yank_next

Lukas Straub (5):
  tests: Use the normal yank code instead of stubs in relevant tests
  tests: Add tests for yank with the chardev-change
  chardev/char.c: Move object_property_try_add_child out of chardev_new
  chardev/char.c: Always pass id to chardev_new
  chardev: Fix yank with the chardev-change case

 MAINTAINERS |   1 +
 chardev/char-socket.c   |  20 +++-
 chardev/char.c  |  74 -
 include/chardev/char.h  |   4 +
 tests/qtest/meson.build |   6 +-
 tests/unit/meson.build  |   5 +-
 tests/unit/test-yank.c  | 240 
 7 files changed, 317 insertions(+), 33 deletions(-)
 create mode 100644 tests/unit/test-yank.c

--
2.30.2


pgp_VGFo49Ze2.pgp
Description: OpenPGP digital signature

[PATCH] exec: Build page-varry-common.c with -fno-lto

[Richard Henderson]

In bbc17caf81f, we used an alias attribute to allow target_page
to be declared const, and yet be initialized late.

This fails when using LTO with several versions of gcc.
The compiler looks through the alias and decides that the const
variable is statically initialized to zero, then propagates that
zero to many uses of the variable.

This can be avoided by compiling one object file with -fno-lto.
In this way, any initializer cannot be seen, and the constant
propagation does not occur.

Since are certain to have this separate compilation unit, we can
drop the alias attribute as well.  We simply have differing
declarations for target_page in different compilation units.
Drop the use of init_target_page, and drop the configure detection
for CONFIG_ATTRIBUTE_ALIAS.

In order to change the compilation flags for a file with meson,
we must use a static_library.  This runs into specific_ss, where
we would need to create many static_library instances.

Fix this by splitting exec-page.c: the page-vary-common.c part is
compiled once as a static_library, while the page-vary.c part is
left in specific_ss in order to handle the target-specific value
of TARGET_PAGE_BITS_MIN.

Reported-by: Gavin Shan 
Signed-off-by: Richard Henderson 
---
 configure|  19 ---
 meson.build  |  18 ++-
 include/exec/cpu-all.h   |  15 ++
 include/exec/page-vary.h |  34 
 exec-vary.c  | 108 ---
 page-vary-common.c   |  54 
 page-vary.c  |  41 +++
 7 files changed, 150 insertions(+), 139 deletions(-)
 create mode 100644 include/exec/page-vary.h
 delete mode 100644 exec-vary.c
 create mode 100644 page-vary-common.c
 create mode 100644 page-vary.c

diff --git a/configure b/configure
index 847bc4d095..dbb873e09c 100755
--- a/configure
+++ b/configure
@@ -4889,21 +4889,6 @@ if  test "$plugins" = "yes" &&
   "for this purpose. You can't build with --static."
 fi
 
-
-# See if __attribute__((alias)) is supported.
-# This false for Xcode 9, but has been remedied for Xcode 10.
-# Unfortunately, travis uses Xcode 9 by default.
-
-attralias=no
-cat > $TMPC << EOF
-int x = 1;
-extern const int y __attribute__((alias("x")));
-int main(void) { return 0; }
-EOF
-if compile_prog "" "" ; then
-attralias=yes
-fi
-
 
 # check if getauxval is available.
 
@@ -5935,10 +5920,6 @@ if test "$atomic64" = "yes" ; then
   echo "CONFIG_ATOMIC64=y" >> $config_host_mak
 fi
 
-if test "$attralias" = "yes" ; then
-  echo "CONFIG_ATTRIBUTE_ALIAS=y" >> $config_host_mak
-fi
-
 if test "$getauxval" = "yes" ; then
   echo "CONFIG_GETAUXVAL=y" >> $config_host_mak
 fi
diff --git a/meson.build b/meson.build
index 5c85a15364..24e8897ba2 100644
--- a/meson.build
+++ b/meson.build
@@ -1933,7 +1933,6 @@ subdir('softmmu')
 
 common_ss.add(capstone)
 specific_ss.add(files('cpu.c', 'disas.c', 'gdbstub.c'), capstone)
-specific_ss.add(files('exec-vary.c'))
 specific_ss.add(when: 'CONFIG_TCG', if_true: files(
   'fpu/softfloat.c',
   'tcg/optimize.c',
@@ -1945,6 +1944,23 @@ specific_ss.add(when: 'CONFIG_TCG', if_true: files(
 ))
 specific_ss.add(when: 'CONFIG_TCG_INTERPRETER', if_true: files('tcg/tci.c'))
 
+# Work around a gcc bug/misfeature wherein constant propagation looks
+# through an alias:
+#   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99696
+# to guess that a const variable is always zero.  Without lto, this is
+# impossible, as the alias is restricted to page-vary-common.c.  Indeed,
+# without lto, not even the alias is required -- we simply use different
+# declarations in different compilation units.
+pagevary = files('page-vary-common.c')
+if get_option('b_lto')
+  pagevary = static_library('page-vary-common',
+sources: pagevary,
+c_args: ['-fno-lto'])
+  pagevary = declare_dependency(link_with: pagevary)
+endif
+common_ss.add(pagevary)
+specific_ss.add(files('page-vary.c'))
+
 subdir('backends')
 subdir('disas')
 subdir('migration')
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 76443eb11d..d76b0b9e02 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -215,22 +215,15 @@ static inline void stl_phys_notdirty(AddressSpace *as, 
hwaddr addr, uint32_t val
 /* page related stuff */
 
 #ifdef TARGET_PAGE_BITS_VARY
-typedef struct {
-bool decided;
-int bits;
-target_long mask;
-} TargetPageBits;
-#if defined(CONFIG_ATTRIBUTE_ALIAS) || !defined(IN_EXEC_VARY)
+# include "exec/page-vary.h"
 extern const TargetPageBits target_page;
-#else
-extern TargetPageBits target_page;
-#endif
 #ifdef CONFIG_DEBUG_TCG
 #define TARGET_PAGE_BITS   ({ assert(target_page.decided); target_page.bits; })
-#define TARGET_PAGE_MASK   ({ assert(target_page.decided); target_page.mask; })
+#define TARGET_PAGE_MASK   ({ assert(target_page.decided); \
+  

Re: [PATCH] configure: Improve alias attribute check

[Richard Henderson]

On 3/21/21 11:46 AM, Paolo Bonzini wrote:
HRM, what about biting the bullet and making exec-vary.c a C++ source?... Then 
instead of making it conditional an attribute((alias)), we make it conditional 
on having a C++ compiler.


Doesn't help.  The gcc bug I filed talks about c++, because that's the closest 
analogy.


But set_preferred_target_page_bits is called *much* later than a constructor. 
Though still before any use of the variable in question, for which we have an 
--enable-debug-tcg assertion.



r~

Re: [PATCH] configure: Improve alias attribute check

[Paolo Bonzini]

HRM, what about biting the bullet and making exec-vary.c a C++ source?...
Then instead of making it conditional an attribute((alias)), we make it
conditional on having a C++ compiler.

Making cpu-all.h compile as C++ would be complex, but we can isolate all
the required declarations in a separate header file that does not need
either osdep.h or glib-compat.h, and basically just have a global
constructor in exec-vary.cc that forwards to a function in exec.c.

Paolo

Il dom 21 mar 2021, 18:43 Paolo Bonzini  ha scritto:

>
>
> Il dom 21 mar 2021, 18:34 Richard Henderson 
> ha scritto:
>
>> On 3/21/21 10:50 AM, Paolo Bonzini wrote:
>> > Another workaround may be to avoid compiling exec-vary.c with
>> -flto.  I'm not
>> > sure that my meson fu is up to that.  Paolo?
>> >
>> > You would have to define a static library.
>>
>> Ok.  With an extra -fno-lto flag, or can I somehow remove -flto from the
>> library's cflags?  Or unset the meson b_lto variable?
>>
>
> -fno-lto should work, yes. b_lto tries to cater to other compilers, but we
> don't support anything but gcc-like drivers.
>
> > I have filed a gcc bug report:
>> >
>> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99696
>> >
>> > Hopefully someone can address that before gcc 11 gets released.  At
>> which
>> > point we need do nothing in qemu.  Aldy?
>>
>> So, I've reproduced the testcase failure with gcc 9.3 (ubuntu 20.04) as
>> well.
>> Which means that there are at least two releases for which this has not
>> worked.
>>
>> I think Gavin's runtime test is unnecessary.  We don't have to check the
>> runtime results, we can just [ "$lto" = true ], and we fairly well know
>> it'll fail.
>>
>
> Yeah, if anything the test can be used to re-enable attribute((alias))
> once we know there are some fixed compilers. (Though it's quite ugly to
> have worse compilation when cross-compiling).
>
> Paolo
>
>
>>
>> r~
>>
>>

Re: [PATCH] configure: Improve alias attribute check

[Paolo Bonzini]

Il dom 21 mar 2021, 18:34 Richard Henderson 
ha scritto:

> On 3/21/21 10:50 AM, Paolo Bonzini wrote:
> > Another workaround may be to avoid compiling exec-vary.c with
> -flto.  I'm not
> > sure that my meson fu is up to that.  Paolo?
> >
> > You would have to define a static library.
>
> Ok.  With an extra -fno-lto flag, or can I somehow remove -flto from the
> library's cflags?  Or unset the meson b_lto variable?
>

-fno-lto should work, yes. b_lto tries to cater to other compilers, but we
don't support anything but gcc-like drivers.

> I have filed a gcc bug report:
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99696
> >
> > Hopefully someone can address that before gcc 11 gets released.  At
> which
> > point we need do nothing in qemu.  Aldy?
>
> So, I've reproduced the testcase failure with gcc 9.3 (ubuntu 20.04) as
> well.
> Which means that there are at least two releases for which this has not
> worked.
>
> I think Gavin's runtime test is unnecessary.  We don't have to check the
> runtime results, we can just [ "$lto" = true ], and we fairly well know
> it'll fail.
>

Yeah, if anything the test can be used to re-enable attribute((alias)) once
we know there are some fixed compilers. (Though it's quite ugly to have
worse compilation when cross-compiling).

Paolo


>
> r~
>
>

Re: [PATCH] configure: Improve alias attribute check

[Richard Henderson]

On 3/21/21 10:50 AM, Paolo Bonzini wrote:

Another workaround may be to avoid compiling exec-vary.c with -flto.  I'm 
not
sure that my meson fu is up to that.  Paolo?

You would have to define a static library.


Ok.  With an extra -fno-lto flag, or can I somehow remove -flto from the 
library's cflags?  Or unset the meson b_lto variable?



I have filed a gcc bug report:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99696


Hopefully someone can address that before gcc 11 gets released.  At which
point we need do nothing in qemu.  Aldy?


Good point, I can give it a shot too just to see how rusty I am... That would 
be the best outcome, though we would have to check LLVM as well. If const 
doesn't work it would indeed be prudent to include Gavin's configure check.


So, I've reproduced the testcase failure with gcc 9.3 (ubuntu 20.04) as well. 
Which means that there are at least two releases for which this has not worked.


I think Gavin's runtime test is unnecessary.  We don't have to check the 
runtime results, we can just [ "$lto" = true ], and we fairly well know it'll fail.



r~

[PULL 5/5] FreeBSD: Upgrade to 12.2 release

[Thomas Huth]

From: Warner Losh 

FreeBSD 12.1 has reached end of life. Use 12.2 instead so that FreeBSD's
project's packages will work.  Update which timezone to pick. Work around a QEMU
bug that incorrectly raises an exception on a CRC32 instruction with the FPU
disabled.  The qemu bug is described here:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg784158.html

Signed-off-by: Warner Losh 
Message-Id: <20210307155654.993-2-...@bsdimp.com>
[thuth: Disable gnutls to work-around a problem with libtasn1]
Signed-off-by: Thomas Huth 
---
 tests/vm/freebsd | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tests/vm/freebsd b/tests/vm/freebsd
index 09f3ee6cb8..6e20e84322 100755
--- a/tests/vm/freebsd
+++ b/tests/vm/freebsd
@@ -20,12 +20,16 @@ import socket
 import subprocess
 import basevm
 
+FREEBSD_CONFIG = {
+'cpu'  : "max,sse4.2=off",
+}
+
 class FreeBSDVM(basevm.BaseVM):
 name = "freebsd"
 arch = "x86_64"
 
-link = 
"https://download.freebsd.org/ftp/releases/ISO-IMAGES/12.1/FreeBSD-12.1-RELEASE-amd64-disc1.iso.xz;
-csum = "7394c3f60a1e236e7bd3a05809cf43ae39a3b8e5d42d782004cf2f26b1cfcd88"
+link = 
"https://download.freebsd.org/ftp/releases/ISO-IMAGES/12.2/FreeBSD-12.2-RELEASE-amd64-disc1.iso.xz;
+csum = "a4530246cafbf1dd42a9bd3ea441ca9a78a6a0cd070278cbdf63f3a6f803ecae"
 size = "20G"
 pkgs = [
 # build tools
@@ -61,6 +65,8 @@ class FreeBSDVM(basevm.BaseVM):
 "zstd",
 ]
 
+# TODO: Enable gnutls again once FreeBSD's libtasn1 got fixed
+# See: https://gitlab.com/gnutls/libtasn1/-/merge_requests/71
 BUILD_SCRIPT = """
 set -e;
 rm -rf /home/qemu/qemu-test.*
@@ -68,7 +74,7 @@ class FreeBSDVM(basevm.BaseVM):
 mkdir src build; cd src;
 tar -xf /dev/vtbd1;
 cd ../build
-../src/configure --python=python3.7 {configure_opts};
+../src/configure --python=python3.7 --disable-gnutls {configure_opts};
 gmake --output-sync -j{jobs} {target} {verbose};
 """
 
@@ -125,7 +131,7 @@ class FreeBSDVM(basevm.BaseVM):
 self.console_wait_send("IPv6",  "n")
 self.console_wait_send("Resolver",  "\n")
 
-self.console_wait_send("Time Zone Selector","a\n")
+self.console_wait_send("Time Zone Selector","0\n")
 self.console_wait_send("Confirmation",  "y")
 self.console_wait_send("Time & Date",   "\n")
 self.console_wait_send("Time & Date",   "\n")
@@ -206,4 +212,4 @@ class FreeBSDVM(basevm.BaseVM):
 self.print_step("All done")
 
 if __name__ == "__main__":
-sys.exit(basevm.main(FreeBSDVM))
+sys.exit(basevm.main(FreeBSDVM, config=FREEBSD_CONFIG))
-- 
2.27.0

[PULL 3/5] configure: fix for SunOS based systems

[Thomas Huth]

From: David CARLIER 

local directive make the configure fails on these systems.

Signed-off-by: David Carlier 
Message-Id: 
Signed-off-by: Thomas Huth 
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 847bc4d095..61872096a8 100755
--- a/configure
+++ b/configure
@@ -111,7 +111,7 @@ error_exit() {
 do_compiler() {
 # Run the compiler, capturing its output to the log. First argument
 # is compiler binary to execute.
-local compiler="$1"
+compiler="$1"
 shift
 if test -n "$BASH_VERSION"; then eval '
 echo >>config.log "
-- 
2.27.0

[PULL 1/5] docs/devel/testing.rst: Fix references to unit tests

[Thomas Huth]

From: Wainer dos Santos Moschetta 

With the recent move of the unit tests to tests/unit directory some
instructions under the "Unit tests" section became imprecise, which
are fixed by this change.

Fixes: da668aa15b99 ("tests: Move unit tests into a separate directory")
Signed-off-by: Wainer dos Santos Moschetta 
Reviewed-by: Thomas Huth 
Reviewed-by: Willian Rampazzo 
Message-Id: <20210318174407.2299930-1-waine...@redhat.com>
Signed-off-by: Thomas Huth 
---
 docs/devel/testing.rst | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 1434a50cc4..1da4c4e4c4 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -34,17 +34,17 @@ If you are writing new code in QEMU, consider adding a unit 
test, especially
 for utility modules that are relatively stateless or have few dependencies. To
 add a new unit test:
 
-1. Create a new source file. For example, ``tests/foo-test.c``.
+1. Create a new source file. For example, ``tests/unit/foo-test.c``.
 
 2. Write the test. Normally you would include the header file which exports
the module API, then verify the interface behaves as expected from your
test. The test code should be organized with the glib testing framework.
Copying and modifying an existing test is usually a good idea.
 
-3. Add the test to ``tests/meson.build``. The unit tests are listed in a
+3. Add the test to ``tests/unit/meson.build``. The unit tests are listed in a
dictionary called ``tests``.  The values are any additional sources and
dependencies to be linked with the test.  For a simple test whose source
-   is in ``tests/foo-test.c``, it is enough to add an entry like::
+   is in ``tests/unit/foo-test.c``, it is enough to add an entry like::
 
  {
...
-- 
2.27.0

[PULL 2/5] tests/unit/test-block-iothread: fix maybe-uninitialized error on GCC 11

[Thomas Huth]

From: Emanuele Giuseppe Esposito 

When building qemu with GCC 11, test-block-iothread produces the following
warning:

../tests/unit/test-block-iothread.c:148:11: error: ‘buf’ may be used
uninitialized [-Werror=maybe-uninitialized]

This is caused by buf[512] left uninitialized and passed to
bdrv_save_vmstate() that expects a const uint8_t *, so the compiler
assumes it will be read and expects the parameter to be initialized.

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Paolo Bonzini 
Message-Id: <20210319112218.49609-1-eespo...@redhat.com>
Signed-off-by: Thomas Huth 
---
 tests/unit/test-block-iothread.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c
index 3f866a35c6..8cf172cb7a 100644
--- a/tests/unit/test-block-iothread.c
+++ b/tests/unit/test-block-iothread.c
@@ -89,7 +89,7 @@ static void test_sync_op_pread(BdrvChild *c)
 
 static void test_sync_op_pwrite(BdrvChild *c)
 {
-uint8_t buf[512];
+uint8_t buf[512] = { 0 };
 int ret;
 
 /* Success */
@@ -117,7 +117,7 @@ static void test_sync_op_blk_pread(BlockBackend *blk)
 
 static void test_sync_op_blk_pwrite(BlockBackend *blk)
 {
-uint8_t buf[512];
+uint8_t buf[512] = { 0 };
 int ret;
 
 /* Success */
@@ -141,7 +141,7 @@ static void test_sync_op_load_vmstate(BdrvChild *c)
 
 static void test_sync_op_save_vmstate(BdrvChild *c)
 {
-uint8_t buf[512];
+uint8_t buf[512] = { 0 };
 int ret;
 
 /* Error: Driver does not support snapshots */
-- 
2.27.0

[PULL 0/5] Unit test fixes + misc OS patches

[Thomas Huth]

 Hi Peter!

The following changes since commit 2e1293cbaac75e84f541f9acfa8e26749f4c3562:

  Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2021-03-16-v4' 
into staging (2021-03-19 16:40:00 +)

are available in the Git repository at:

  https://gitlab.com/thuth/qemu.git tags/pull-request-2021-03-21

for you to fetch changes up to 262fd27392128c180afc8f968d90d530574862f7:

  FreeBSD: Upgrade to 12.2 release (2021-03-20 06:33:17 +0100)


* Small fixes for the unit tests
* Compilation fixes for Illumos et al.
* Update the FreeBSD VM to 12.2


David CARLIER (2):
  configure: fix for SunOS based systems
  contrib: ivshmem client and server build fix for SunOS.

Emanuele Giuseppe Esposito (1):
  tests/unit/test-block-iothread: fix maybe-uninitialized error on GCC
11

Wainer dos Santos Moschetta (1):
  docs/devel/testing.rst: Fix references to unit tests

Warner Losh (1):
  FreeBSD: Upgrade to 12.2 release

 configure   |  2 +-
 contrib/ivshmem-client/ivshmem-client.c | 12 ++--
 contrib/ivshmem-server/ivshmem-server.c | 12 ++--
 docs/devel/testing.rst  |  6 +++---
 tests/unit/test-block-iothread.c|  6 +++---
 tests/vm/freebsd| 16 +++-
 6 files changed, 30 insertions(+), 24 deletions(-)

-- 
2.27.0

[PULL 4/5] contrib: ivshmem client and server build fix for SunOS.

[Thomas Huth]

From: David CARLIER 

sun is a macro on these systems, thus renaming the variables on the
client and server.

Signed-off-by: David Carlier 
Message-Id: 
Reviewed-by: Peter Maydell 
Signed-off-by: Thomas Huth 
---
 contrib/ivshmem-client/ivshmem-client.c | 12 ++--
 contrib/ivshmem-server/ivshmem-server.c | 12 ++--
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/contrib/ivshmem-client/ivshmem-client.c 
b/contrib/ivshmem-client/ivshmem-client.c
index b1274b236a..182c79d27c 100644
--- a/contrib/ivshmem-client/ivshmem-client.c
+++ b/contrib/ivshmem-client/ivshmem-client.c
@@ -178,7 +178,7 @@ ivshmem_client_init(IvshmemClient *client, const char 
*unix_sock_path,
 int
 ivshmem_client_connect(IvshmemClient *client)
 {
-struct sockaddr_un sun;
+struct sockaddr_un s_un;
 int fd, ret;
 int64_t tmp;
 
@@ -192,16 +192,16 @@ ivshmem_client_connect(IvshmemClient *client)
 return -1;
 }
 
-sun.sun_family = AF_UNIX;
-ret = snprintf(sun.sun_path, sizeof(sun.sun_path), "%s",
+s_un.sun_family = AF_UNIX;
+ret = snprintf(s_un.sun_path, sizeof(s_un.sun_path), "%s",
client->unix_sock_path);
-if (ret < 0 || ret >= sizeof(sun.sun_path)) {
+if (ret < 0 || ret >= sizeof(s_un.sun_path)) {
 IVSHMEM_CLIENT_DEBUG(client, "could not copy unix socket path\n");
 goto err_close;
 }
 
-if (connect(client->sock_fd, (struct sockaddr *), sizeof(sun)) < 0) {
-IVSHMEM_CLIENT_DEBUG(client, "cannot connect to %s: %s\n", 
sun.sun_path,
+if (connect(client->sock_fd, (struct sockaddr *)_un, sizeof(s_un)) < 0) {
+IVSHMEM_CLIENT_DEBUG(client, "cannot connect to %s: %s\n", 
s_un.sun_path,
  strerror(errno));
 goto err_close;
 }
diff --git a/contrib/ivshmem-server/ivshmem-server.c 
b/contrib/ivshmem-server/ivshmem-server.c
index 88daee812d..39a6ffdb5d 100644
--- a/contrib/ivshmem-server/ivshmem-server.c
+++ b/contrib/ivshmem-server/ivshmem-server.c
@@ -288,7 +288,7 @@ ivshmem_server_init(IvshmemServer *server, const char 
*unix_sock_path,
 int
 ivshmem_server_start(IvshmemServer *server)
 {
-struct sockaddr_un sun;
+struct sockaddr_un s_un;
 int shm_fd, sock_fd, ret;
 
 /* open shm file */
@@ -327,15 +327,15 @@ ivshmem_server_start(IvshmemServer *server)
 goto err_close_shm;
 }
 
-sun.sun_family = AF_UNIX;
-ret = snprintf(sun.sun_path, sizeof(sun.sun_path), "%s",
+s_un.sun_family = AF_UNIX;
+ret = snprintf(s_un.sun_path, sizeof(s_un.sun_path), "%s",
server->unix_sock_path);
-if (ret < 0 || ret >= sizeof(sun.sun_path)) {
+if (ret < 0 || ret >= sizeof(s_un.sun_path)) {
 IVSHMEM_SERVER_DEBUG(server, "could not copy unix socket path\n");
 goto err_close_sock;
 }
-if (bind(sock_fd, (struct sockaddr *), sizeof(sun)) < 0) {
-IVSHMEM_SERVER_DEBUG(server, "cannot connect to %s: %s\n", 
sun.sun_path,
+if (bind(sock_fd, (struct sockaddr *)_un, sizeof(s_un)) < 0) {
+IVSHMEM_SERVER_DEBUG(server, "cannot connect to %s: %s\n", 
s_un.sun_path,
  strerror(errno));
 goto err_close_sock;
 }
-- 
2.27.0

Re: [PATCH] configure: Improve alias attribute check

[Paolo Bonzini]

Il dom 21 mar 2021, 16:49 Richard Henderson 
ha scritto:

> What exact version of gcc are you guys using?  Something from rawhide that
> I can just install?
>

I am using Fedora 34. I upgraded just to test this bug and it seems stable
except that GNOME Shell extensions need an upgrade. However I haven't tried
building all of QEMU, only the test case.

So far I have failed to compile with gcc master with --enable-lto.  Lots of
> missing symbols reported at link time.  Therefore I've been unable to
> actually
> test what I intended to test.
>
> That said, I'm not hopeful that __attribute__((const)) will work.  I have
> an
> idea that early inlining will inline tiny function calls too soon, before
> the
> attribute has a change to DTRT during CSE.


Yeah that's at least plausible.

Another workaround may be to avoid compiling exec-vary.c with -flto.  I'm
> not
> sure that my meson fu is up to that.  Paolo?
>

You would have to define a static library.

I have filed a gcc bug report:
>
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99696
>
> Hopefully someone can address that before gcc 11 gets released.  At which
> point we need do nothing in qemu.  Aldy?


Good point, I can give it a shot too just to see how rusty I am... That
would be the best outcome, though we would have to check LLVM as well. If
const doesn't work it would indeed be prudent to include Gavin's configure
check.

Paolo


>
> r~
>
>

Re: [PATCH v6 10/11] hvf: arm: Add support for GICv3

[Alexander Graf]

On 28.01.21 17:40, Peter Maydell wrote:

On Wed, 20 Jan 2021 at 22:44, Alexander Graf  wrote:

We currently only support GICv2 emulation. To also support GICv3, we will
need to pass a few system registers into their respective handler functions.

This patch adds handling for all of the required system registers, so that
we can run with more than 8 vCPUs.

Signed-off-by: Alexander Graf 
Acked-by: Roman Bolshakov 

So, how much of the GICv3 does Hypervisor.framework expect
userspace to implement ?



All of it. There is absolutely 0 handling for anything GIC related in HVF.



Currently we have two GICv3 implementations:
  * hw/intc/arm_gicv3_kvm.c -- which is the stub device that
handles the KVM in-kernel GICv3
  * hw/intc/arm_gicv3.c -- which is the full-emulation device
that assumes that it is working with a TCG CPU

Support for HVF GICv3 needs either another one of these or
some serious refactoring of the full-emulation device so that
it doesn't assume that the CPU it's dealing with is a TCG one.
(I suspect the right design is to bite the bullet and make the
implementation follow the hardware in having "the GIC device proper"
and "the GIC CPU interface" separate from each other and communicating
via an API approximately equivalent to the GIC Stream Protocol
as described in the GICv3 architecture specification; but that's
a painful refactor and there might be some other approach less
invasive but still reasonably clean.)



Happy to hear good suggestions on how to do a less painful refactor. At 
the end of the day, while I agree that the arm_gicv3*.c code does rely 
on the CPU env that usually related to TCG, I don't see why we shouldn't 
reuse that same data structure to transmit CPU state...






  static uint64_t hvf_sysreg_read(CPUState *cpu, uint32_t reg)
  {
  ARMCPU *arm_cpu = ARM_CPU(cpu);
@@ -431,6 +491,39 @@ static uint64_t hvf_sysreg_read(CPUState *cpu, uint32_t 
reg)
  case SYSREG_PMCCNTR_EL0:
  val = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
  break;
+case SYSREG_ICC_AP0R0_EL1:
+case SYSREG_ICC_AP0R1_EL1:
+case SYSREG_ICC_AP0R2_EL1:
+case SYSREG_ICC_AP0R3_EL1:
+case SYSREG_ICC_AP1R0_EL1:
+case SYSREG_ICC_AP1R1_EL1:
+case SYSREG_ICC_AP1R2_EL1:
+case SYSREG_ICC_AP1R3_EL1:
+case SYSREG_ICC_ASGI1R_EL1:
+case SYSREG_ICC_BPR0_EL1:
+case SYSREG_ICC_BPR1_EL1:
+case SYSREG_ICC_DIR_EL1:
+case SYSREG_ICC_EOIR0_EL1:
+case SYSREG_ICC_EOIR1_EL1:
+case SYSREG_ICC_HPPIR0_EL1:
+case SYSREG_ICC_HPPIR1_EL1:
+case SYSREG_ICC_IAR0_EL1:
+case SYSREG_ICC_IAR1_EL1:
+case SYSREG_ICC_IGRPEN0_EL1:
+case SYSREG_ICC_IGRPEN1_EL1:
+case SYSREG_ICC_PMR_EL1:
+case SYSREG_ICC_SGI0R_EL1:
+case SYSREG_ICC_SGI1R_EL1:
+case SYSREG_ICC_SRE_EL1:
+val = hvf_sysreg_read_cp(cpu, reg);
+break;
+case SYSREG_ICC_CTLR_EL1:
+val = hvf_sysreg_read_cp(cpu, reg);
+
+/* AP0R registers above 0 don't trap, expose less PRIs to fit */
+val &= ~ICC_CTLR_EL1_PRIBITS_MASK;
+val |= 4 << ICC_CTLR_EL1_PRIBITS_SHIFT;
+break;

Pretty sure you don't want to be trying to squeeze even the
GICv3 cpuif implementation into this source file...


  default:
  DPRINTF("unhandled sysreg read %08x (op0=%d op1=%d op2=%d "
  "crn=%d crm=%d)", reg, (reg >> 20) & 0x3,
@@ -442,6 +535,24 @@ static uint64_t hvf_sysreg_read(CPUState *cpu, uint32_t 
reg)
  return val;
  }

+static void hvf_sysreg_write_cp(CPUState *cpu, uint32_t reg, uint64_t val)
+{
+ARMCPU *arm_cpu = ARM_CPU(cpu);
+CPUARMState *env = _cpu->env;
+const ARMCPRegInfo *ri;
+
+ri = get_arm_cp_reginfo(arm_cpu->cp_regs, hvf_reg2cp_reg(reg));
+
+if (ri) {
+if (ri->writefn) {
+ri->writefn(env, ri, val);
+} else {
+CPREG_FIELD64(env, ri) = val;
+}
+DPRINTF("vgic write to %s [val=%016llx]", ri->name, val);
+}
+}
+
  static void hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val)
  {
  ARMCPU *arm_cpu = ARM_CPU(cpu);
@@ -449,6 +560,36 @@ static void hvf_sysreg_write(CPUState *cpu, uint32_t reg, 
uint64_t val)
  switch (reg) {
  case SYSREG_CNTPCT_EL0:
  break;
+case SYSREG_ICC_AP0R0_EL1:
+case SYSREG_ICC_AP0R1_EL1:
+case SYSREG_ICC_AP0R2_EL1:
+case SYSREG_ICC_AP0R3_EL1:
+case SYSREG_ICC_AP1R0_EL1:
+case SYSREG_ICC_AP1R1_EL1:
+case SYSREG_ICC_AP1R2_EL1:
+case SYSREG_ICC_AP1R3_EL1:
+case SYSREG_ICC_ASGI1R_EL1:
+case SYSREG_ICC_BPR0_EL1:
+case SYSREG_ICC_BPR1_EL1:
+case SYSREG_ICC_CTLR_EL1:
+case SYSREG_ICC_DIR_EL1:
+case SYSREG_ICC_HPPIR0_EL1:
+case SYSREG_ICC_HPPIR1_EL1:
+case SYSREG_ICC_IAR0_EL1:
+case SYSREG_ICC_IAR1_EL1:
+case SYSREG_ICC_IGRPEN0_EL1:
+case SYSREG_ICC_IGRPEN1_EL1:
+case SYSREG_ICC_PMR_EL1:
+case SYSREG_ICC_SGI0R_EL1:
+case SYSREG_ICC_SGI1R_EL1:
+case SYSREG_ICC_SRE_EL1:
+

Re: [PATCH v6 09/11] arm/hvf: Add a WFI handler

[Alexander Graf]

On 10.02.21 23:17, Peter Maydell wrote:

On Wed, 10 Feb 2021 at 20:25, Peter Collingbourne  wrote:

On Thu, Jan 28, 2021 at 8:25 AM Peter Maydell  wrote:

On Wed, 20 Jan 2021 at 22:44, Alexander Graf  wrote:

+if (!seconds && nanos < 200) {
+break;
+}
+
+struct timespec ts = { seconds, nanos };
+hvf_wait_for_ipi(cpu, );
+}

Why doesn't the timer timeout manifest as an IPI ? (Put another way,
why is the timer interrupt special?)

Timer timeouts result in an IPI (via HV_EXIT_REASON_VTIMER_ACTIVATED)
if they become due while in hv_vcpu_run(). But at this point we are
not in hv_vcpu_run() (due to the aforementioned difference in wait
behavior between x86 and ARM) so we need to "manually" wait for the
timer to become due, re-enter the guest, let it exit with
HV_EXIT_REASON_VTIMER_ACTIVATED and then trigger the IPI.

But WFI doesn't just wait for a timer interrupt, it waits for
any interrupt. So it doesn't seem right that the timer interrupt
in particular is being handled specially here.



It waits for either an external interrupt (vcpu_kick() -> IPI -> signal 
-> pselect exits) or a vtimer (kept in the CPU thread, handled by hvf 
natively when vCPU is running, handled through the pselect timeout when 
in WFI mode).


In hvf on ARM, the vtimer is handled specially. It is owned by the 
kernel code when we're inside the CPU loop. We don't even get vtimer MSR 
exits. However when user space decides to not return to the kernel CPU 
loop, it needs to handle the vtimer expiry itself. We really only have 
that case on WFI.



Alex

Re: [PATCH] configure: Improve alias attribute check

[Richard Henderson]

On 3/20/21 4:33 PM, Richard Henderson wrote:

On 3/20/21 11:52 AM, Paolo Bonzini wrote:

+int main(void)
+{
+    return read_y();
+}


I think this should be "read_y() == 1 ? 0 : 1".


As a testcase returning 0 on success, yes.


I can reproduce it with -flto -O2 but not without -flto, do you agree?


Agreed.  Replicated with a random recent gcc 11 snapshot.
This is really annoying of lto.  It's clear something needs to change though.

Perhaps we can obtain the same optimization by wrapping reads of the page 
size in an inline __attribute__((const)) function.  Richard, what do you think?


I'll give it a shot and see what happens.


What exact version of gcc are you guys using?  Something from rawhide that I 
can just install?


So far I have failed to compile with gcc master with --enable-lto.  Lots of 
missing symbols reported at link time.  Therefore I've been unable to actually 
test what I intended to test.


That said, I'm not hopeful that __attribute__((const)) will work.  I have an 
idea that early inlining will inline tiny function calls too soon, before the 
attribute has a change to DTRT during CSE.  At which point we're left with bare 
variable references and we're exactly where we would be if we did nothing 
special at all.


Another workaround may be to avoid compiling exec-vary.c with -flto.  I'm not 
sure that my meson fu is up to that.  Paolo?


I have filed a gcc bug report:

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99696

Hopefully someone can address that before gcc 11 gets released.  At which point 
we need do nothing in qemu.  Aldy?



r~

[Bug 1920672] [NEW] Compilation fails silently

[Betim]

Public bug reported:

It compiles until the end and then just:
[6102/6103] Linking target qemu-system-alpha
[6103/6103] Linking target qemu-system-aarch64
make[1]: Leaving directory '/home/t/.cache/kiss/proc/32129/build/qemu/build'
make: *** [GNUmakefile:11: all] Error 2

Attached is the complete log including configure. I can't find why this
is happening maybe I have a wrong version of a required library?

Any ideas?

** Affects: qemu
 Importance: Undecided
 Status: New

** Attachment added: "make log"
   https://bugs.launchpad.net/bugs/1920672/+attachment/5478697/+files/qemu.log

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

Title:
  Compilation fails silently

Status in QEMU:
  New

Bug description:
  It compiles until the end and then just:
  [6102/6103] Linking target qemu-system-alpha
  [6103/6103] Linking target qemu-system-aarch64
  make[1]: Leaving directory '/home/t/.cache/kiss/proc/32129/build/qemu/build'
  make: *** [GNUmakefile:11: all] Error 2

  Attached is the complete log including configure. I can't find why
  this is happening maybe I have a wrong version of a required library?

  Any ideas?

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

Re: [PATCH v2] configure: Improve alias attribute check

[Paolo Bonzini]

On 21/03/21 01:17, Gavin Shan wrote:

It's still possible that the wrong value is returned from the alias
of variable even if the program can be compiled without issue. This
improves the check by executing the binary to check the result.

If alias attribute can't be working properly, the @target_page in
exec-vary.c will always return zeroes when we have the following gcc
version and the combination of options "-O2 -flto=auto".

   # gcc --version
   gcc (GCC) 11.0.0 20210210 (Red Hat 11.0.0-0)

This abstracts the code from exec-vary.c and use it as indicator to
enable gcc alias attribute or not. The gcc alias attribute is disabled
for the cross-compiling case as the compiled binary isn't executable.

Signed-off-by: Gavin Shan 


NACK, let's first try to figure out a way to preserve the optimization 
without using aliases.


Paolo

Re: Arm: VFP regression

[Peter Maydell]

On Sat, 20 Mar 2021 at 22:38, Adam Lackorzynski  wrote:
>
> Hi,
>
> I'm seeing a regression in Arm's vfp handling, giving an undefined
> instruction when reading mvfr1 in PL2/armv7 although the FPU is enabled.
> The following makes it work again for me, however this just looks like a
> band-aid. Thanks for taking a look.

Could you provide a test case, please (QEMU command line and
image/etc files needed to reproduce) ?

thanks
-- PMM

Re: [PATCH] tests/unit/test-block-iothread: fix maybe-uninitialized error on GCC 11

[Alberto Garcia]

On Fri 19 Mar 2021 12:22:18 PM CET, Emanuele Giuseppe Esposito 
 wrote:
> When building qemu with GCC 11, test-block-iothread produces the following
> warning:
>
> ../tests/unit/test-block-iothread.c:148:11: error: ‘buf’ may be used
> uninitialized [-Werror=maybe-uninitialized]
>
> This is caused by buf[512] left uninitialized and passed to
> bdrv_save_vmstate() that expects a const uint8_t *, so the compiler
> assumes it will be read and expects the parameter to be initialized.
>
> Signed-off-by: Emanuele Giuseppe Esposito 

Reviewed-by: Alberto Garcia 

Berto