Re: Re: [PULL 11/73] cryptodev: Support query-stats QMP command

[zhenwei pi]

On 5/3/23 01:03, Peter Maydell wrote:

On Wed, 8 Mar 2023 at 01:11, Michael S. Tsirkin  wrote:


From: zhenwei pi 

Now we can use "query-stats" QMP command to query statistics of
crypto devices. (Originally this was designed to show statistics
by '{"execute": "query-cryptodev"}'. Daniel Berrangé suggested that
querying configuration info by "query-cryptodev", and querying
runtime performance info by "query-stats". This makes sense!)


Hi; Coverity points out (CID 1508074) that this change
introduces a memory leak:


+static int cryptodev_backend_stats_query(Object *obj, void *data)
+{



+entry = g_new0(StatsResult, 1);
+entry->provider = STATS_PROVIDER_CRYPTODEV;
+entry->qom_path = g_strdup(object_get_canonical_path(obj));


object_get_canonical_path() already returns allocated memory
that the caller should free with g_free(), so we should not
g_strdup() it (which then leaks that memory).


+entry->stats = stats_list;
+QAPI_LIST_PREPEND(*stats_results, entry);
+
+return 0;
+}


Would somebody like to send a patch?

thanks
-- PMM


Hi,

Thanks for pointing out this, I'll fix this later.

--
zhenwei pi

Re: [PULL 61/73] hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register

[Michael S. Tsirkin]

On Tue, May 02, 2023 at 09:32:34PM -0300, Leonardo Brás wrote:
> Hello Michael, Juan, Peter,
> 
> On Wed, 2023-04-26 at 09:19 +0200, Juan Quintela wrote:
> > "Michael S. Tsirkin"  wrote:
> > > On Tue, Apr 25, 2023 at 08:42:17PM -0400, Peter Xu wrote:
> > > > Hi, Michael, Jonathan,
> > > > 
> > > > On Tue, Mar 07, 2023 at 08:13:53PM -0500, Michael S. Tsirkin wrote:
> > > > This breaks the simplest migration from QEMU 8.0->7.2 binaries on all
> > > > machine types I think as long as the cap is present, e.g. the default
> > > > e1000e provided by the default q35 machine can already hit it with all
> > > > default cmdline:
> > > > 
> > > >   ./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]
> > > > 
> > > > 7.2 binary will have empty wmask for PCI_ERR_UNCOR_MASK, meanwhile I 
> > > > think
> > > > it can also see a non-zero value, then the migration will fail at:
> > > > 
> > > > vmstate_load :00:02.0/e1000e, e1000e
> > > >
> > > > qemu-7.2: get_pci_config_device: Bad config data: i=0x10a read: 40 
> > > > device: 0 cmask: ff wmask: 0 w1cmask:0
> > > > qemu-7.2: Failed to load PCIDevice:config   
> > > > qemu-7.2: Failed to load e1000e:parent_obj  
> > > >   
> > > > qemu-7.2: error while loading state for instance 0x0 of device 
> > > > ':00:02.0/e1000e'  
> > > > qemu-7.2: load of migration failed: Invalid argument
> > > > 
> > > > We probably at least want to have the default value to be still zero, 
> > > > and
> > > > we'd need to make sure it'll not be modified by the guest, iiuc.
> > > > 
> > > > Below oneliner works for me and makes the migration work again:
> > > > 
> > > > ===8<===
> > > > diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> > > > index 103667c368..563a37b79c 100644
> > > > --- a/hw/pci/pcie_aer.c
> > > > +++ b/hw/pci/pcie_aer.c
> > > > @@ -113,7 +113,7 @@ int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver, 
> > > > uint16_t offset,
> > > >  pci_set_long(dev->w1cmask + offset + PCI_ERR_UNCOR_STATUS,
> > > >   PCI_ERR_UNC_SUPPORTED);
> > > >  pci_set_long(dev->config + offset + PCI_ERR_UNCOR_MASK,
> > > > - PCI_ERR_UNC_MASK_DEFAULT);
> > > > + 0/*PCI_ERR_UNC_MASK_DEFAULT*/);
> > > >  pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_MASK,
> > > >   PCI_ERR_UNC_SUPPORTED);
> > > > ===8<===
> > > > 
> > > > Anyone could have a look on a solid solution from PCI side?
> > > > 
> > > > Copy Juan and Leonardo.
> > > > 
> > > > Thanks,
> > > 
> > > My bad, I forgot about this 臘.
> > > So we need a property and tweak it with compat machinery depending on
> > > machine type. Jonathan, can you work on this pls?
> > > Or I can revert for now to relieve the time pressure,
> > > redo the patch at your leasure.
> > 
> > I agree with Michael here, the best option is adding a new property.
> > 
> > Later, Juan.
> > 
> 
> I sent a patch implementing the suggested fix:
> https://lore.kernel.org/qemu-devel/20230503002701.854329-1-leob...@redhat.com/T/#u
> 
> Please let me know of anything to improve.
> 
> Best regards,
> Leo

Weird, didn't get it for some reason. Pulled it from lore now, thanks!

-- 
MST

[RFC PATCH 1/4] spapr: H_ENTER_NESTED should restore host XER ca field

[Nicholas Piggin]

Fix missing env->ca restore when going from L2 back to the host.

Fixes: 120f738a467 ("spapr: implement nested-hv capability for the virtual 
hypervisor")
Signed-off-by: Nicholas Piggin 
---
 hw/ppc/spapr_hcall.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index ec4def62f8..be225adaf6 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1785,6 +1785,7 @@ out_restore_l1:
 env->cfar = spapr_cpu->nested_host_state->cfar;
 env->xer = spapr_cpu->nested_host_state->xer;
 env->so = spapr_cpu->nested_host_state->so;
+env->ca = spapr_cpu->nested_host_state->ca;
 env->ov = spapr_cpu->nested_host_state->ov;
 env->ov32 = spapr_cpu->nested_host_state->ov32;
 env->ca32 = spapr_cpu->nested_host_state->ca32;
-- 
2.40.1

[RFC PATCH 4/4] spapr: Move spapr nested HV to a new file

[Nicholas Piggin]

Create spapr_nested.c for the nested HV implementation (modulo small
pieces in MMU and exception handling).

Signed-off-by: Nicholas Piggin 
---
 hw/ppc/meson.build |   1 +
 hw/ppc/spapr_hcall.c   | 429 +--
 hw/ppc/spapr_nested.c  | 496 +
 include/hw/ppc/spapr.h |  61 +
 4 files changed, 499 insertions(+), 488 deletions(-)
 create mode 100644 hw/ppc/spapr_nested.c

diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
index c927337da0..a313d4b964 100644
--- a/hw/ppc/meson.build
+++ b/hw/ppc/meson.build
@@ -15,6 +15,7 @@ ppc_ss.add(when: 'CONFIG_PSERIES', if_true: files(
   'spapr_vio.c',
   'spapr_events.c',
   'spapr_hcall.c',
+  'spapr_nested.c',
   'spapr_iommu.c',
   'spapr_rtas.c',
   'spapr_pci.c',
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 783a06ba98..dbdcbb0e9d 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1496,444 +1496,17 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, 
target_ulong opcode,
 }
 
 #ifdef CONFIG_TCG
-#define PRTS_MASK  0x1f
-
-static target_ulong h_set_ptbl(PowerPCCPU *cpu,
-   SpaprMachineState *spapr,
-   target_ulong opcode,
-   target_ulong *args)
-{
-target_ulong ptcr = args[0];
-
-if (!spapr_get_cap(spapr, SPAPR_CAP_NESTED_KVM_HV)) {
-return H_FUNCTION;
-}
-
-if ((ptcr & PRTS_MASK) + 12 - 4 > 12) {
-return H_PARAMETER;
-}
-
-spapr->nested_ptcr = ptcr; /* Save new partition table */
-
-return H_SUCCESS;
-}
-
-static target_ulong h_tlb_invalidate(PowerPCCPU *cpu,
- SpaprMachineState *spapr,
- target_ulong opcode,
- target_ulong *args)
-{
-/*
- * The spapr virtual hypervisor nested HV implementation retains no L2
- * translation state except for TLB. And the TLB is always invalidated
- * across L1<->L2 transitions, so nothing is required here.
- */
-
-return H_SUCCESS;
-}
-
-static target_ulong h_copy_tofrom_guest(PowerPCCPU *cpu,
-SpaprMachineState *spapr,
-target_ulong opcode,
-target_ulong *args)
-{
-/*
- * This HCALL is not required, L1 KVM will take a slow path and walk the
- * page tables manually to do the data copy.
- */
-return H_FUNCTION;
-}
-
-struct nested_ppc_state {
-uint64_t gpr[32];
-uint64_t lr;
-uint64_t ctr;
-uint64_t cfar;
-uint64_t msr;
-uint64_t nip;
-uint32_t cr;
-
-uint64_t xer;
-
-uint64_t lpcr;
-uint64_t lpidr;
-uint64_t pidr;
-uint64_t pcr;
-uint64_t dpdes;
-uint64_t hfscr;
-uint64_t srr0;
-uint64_t srr1;
-uint64_t sprg0;
-uint64_t sprg1;
-uint64_t sprg2;
-uint64_t sprg3;
-uint64_t ppr;
-
-int64_t tb_offset;
-};
-
-static void nested_save_state(struct nested_ppc_state *save, PowerPCCPU *cpu)
-{
-CPUPPCState *env = >env;
-uint32_t cr;
-int i;
-
-memcpy(save->gpr, env->gpr, sizeof(save->gpr));
-
-save->lr = env->lr;
-save->ctr = env->ctr;
-save->cfar = env->cfar;
-save->msr = env->msr;
-save->nip = env->nip;
-
-cr = 0;
-for (i = 0; i < 8; i++) {
-cr |= (env->crf[i] & 15) << (4 * (7 - i));
-}
-save->cr = cr;
-
-save->xer = cpu_read_xer(env);
-
-save->lpcr = env->spr[SPR_LPCR];
-save->lpidr = env->spr[SPR_LPIDR];
-save->pcr = env->spr[SPR_PCR];
-save->dpdes = env->spr[SPR_DPDES];
-save->hfscr = env->spr[SPR_HFSCR];
-save->srr0 = env->spr[SPR_SRR0];
-save->srr1 = env->spr[SPR_SRR1];
-save->sprg0 = env->spr[SPR_SPRG0];
-save->sprg1 = env->spr[SPR_SPRG1];
-save->sprg2 = env->spr[SPR_SPRG2];
-save->sprg3 = env->spr[SPR_SPRG3];
-save->pidr = env->spr[SPR_BOOKS_PID];
-save->ppr = env->spr[SPR_PPR];
-
-save->tb_offset = env->tb_env->tb_offset;
-}
-
-static void nested_load_state(PowerPCCPU *cpu, struct nested_ppc_state *load)
-{
-CPUState *cs = CPU(cpu);
-CPUPPCState *env = >env;
-uint32_t cr;
-int i;
-
-memcpy(env->gpr, load->gpr, sizeof(env->gpr));
-
-env->lr = load->lr;
-env->ctr = load->ctr;
-env->cfar = load->cfar;
-env->msr = load->msr;
-env->nip = load->nip;
-
-cr = load->cr;
-for (i = 7; i >= 0; i--) {
-env->crf[i] = cr & 15;
-cr >>= 4;
-}
-
-cpu_write_xer(env, load->xer);
-
-env->spr[SPR_LPCR] = load->lpcr;
-env->spr[SPR_LPIDR] = load->lpidr;
-env->spr[SPR_PCR] = load->pcr;
-env->spr[SPR_DPDES] = load->dpdes;
-env->spr[SPR_HFSCR] = load->hfscr;
-env->spr[SPR_SRR0] = load->srr0;
-env->spr[SPR_SRR1] = load->srr1;
-env->spr[SPR_SPRG0] = load->sprg0;
-env->spr[SPR_SPRG1] = load->sprg1;
-env->spr[SPR_SPRG2] = load->sprg2;
-

[RFC PATCH 3/4] spapr: load and store l2 state with helper functions

[Nicholas Piggin]

Arguably this is just shuffling around register accesses, but one nice
thing it does is allow the exit to save away the L2 state then switch
the environment to the L1 before copying L2 data back to the L1, which
logically flows more naturally and simplifies the error paths.

Signed-off-by: Nicholas Piggin 
---
 hw/ppc/spapr_hcall.c | 178 +--
 1 file changed, 85 insertions(+), 93 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 6679150ac7..783a06ba98 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1675,9 +1675,9 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
target_ulong *args)
 {
 PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
-CPUState *cs = CPU(cpu);
 CPUPPCState *env = >env;
 SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
+struct nested_ppc_state l2_state;
 target_ulong hv_ptr = args[0];
 target_ulong regs_ptr = args[1];
 target_ulong hdec, now = cpu_ppc_load_tbl(env);
@@ -1686,8 +1686,6 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
 struct kvmppc_hv_guest_state hv_state;
 struct kvmppc_pt_regs *regs;
 hwaddr len;
-uint64_t cr;
-int i;
 
 if (spapr->nested_ptcr == 0) {
 return H_NOT_AVAILABLE;
@@ -1713,6 +1711,10 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
 return H_PARAMETER;
 }
 
+if (hv_state.lpid == 0) {
+return H_PARAMETER;
+}
+
 spapr_cpu->nested_host_state = g_try_new(struct nested_ppc_state, 1);
 if (!spapr_cpu->nested_host_state) {
 return H_NO_MEM;
@@ -1731,51 +1733,49 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
 return H_P2;
 }
 
-len = sizeof(env->gpr);
+len = sizeof(l2_state.gpr);
 assert(len == sizeof(regs->gpr));
-memcpy(env->gpr, regs->gpr, len);
+memcpy(l2_state.gpr, regs->gpr, len);
 
-env->lr = regs->link;
-env->ctr = regs->ctr;
-cpu_write_xer(env, regs->xer);
-
-cr = regs->ccr;
-for (i = 7; i >= 0; i--) {
-env->crf[i] = cr & 15;
-cr >>= 4;
-}
-
-env->msr = regs->msr;
-env->nip = regs->nip;
+l2_state.lr = regs->link;
+l2_state.ctr = regs->ctr;
+l2_state.xer = regs->xer;
+l2_state.cr = regs->ccr;
+l2_state.msr = regs->msr;
+l2_state.nip = regs->nip;
 
 address_space_unmap(CPU(cpu)->as, regs, len, len, false);
 
-env->cfar = hv_state.cfar;
-
-assert(env->spr[SPR_LPIDR] == 0);
-env->spr[SPR_LPIDR] = hv_state.lpid;
+l2_state.cfar = hv_state.cfar;
+l2_state.lpidr = hv_state.lpid;
 
 lpcr_mask = LPCR_DPFD | LPCR_ILE | LPCR_AIL | LPCR_LD | LPCR_MER;
 lpcr = (env->spr[SPR_LPCR] & ~lpcr_mask) | (hv_state.lpcr & lpcr_mask);
 lpcr |= LPCR_HR | LPCR_UPRT | LPCR_GTSE | LPCR_HVICE | LPCR_HDICE;
 lpcr &= ~LPCR_LPES0;
-env->spr[SPR_LPCR] = lpcr & pcc->lpcr_mask;
+l2_state.lpcr = lpcr & pcc->lpcr_mask;
 
-env->spr[SPR_PCR] = hv_state.pcr;
+l2_state.pcr = hv_state.pcr;
 /* hv_state.amor is not used */
-env->spr[SPR_DPDES] = hv_state.dpdes;
-env->spr[SPR_HFSCR] = hv_state.hfscr;
-hdec = hv_state.hdec_expiry - now;
+l2_state.dpdes = hv_state.dpdes;
+l2_state.hfscr = hv_state.hfscr;
 /* TCG does not implement DAWR*, CIABR, PURR, SPURR, IC, VTB, HEIR SPRs*/
-env->spr[SPR_SRR0] = hv_state.srr0;
-env->spr[SPR_SRR1] = hv_state.srr1;
-env->spr[SPR_SPRG0] = hv_state.sprg[0];
-env->spr[SPR_SPRG1] = hv_state.sprg[1];
-env->spr[SPR_SPRG2] = hv_state.sprg[2];
-env->spr[SPR_SPRG3] = hv_state.sprg[3];
-env->spr[SPR_BOOKS_PID] = hv_state.pidr;
-env->spr[SPR_PPR] = hv_state.ppr;
+l2_state.srr0 = hv_state.srr0;
+l2_state.srr1 = hv_state.srr1;
+l2_state.sprg0 = hv_state.sprg[0];
+l2_state.sprg1 = hv_state.sprg[1];
+l2_state.sprg2 = hv_state.sprg[2];
+l2_state.sprg3 = hv_state.sprg[3];
+l2_state.pidr = hv_state.pidr;
+l2_state.ppr = hv_state.ppr;
+l2_state.tb_offset = env->tb_env->tb_offset + hv_state.tb_offset;
 
+/*
+ * Switch to the nested guest environment and start the "hdec" timer.
+ */
+nested_load_state(cpu, _state);
+
+hdec = hv_state.hdec_expiry - now;
 cpu_ppc_hdecr_init(env);
 cpu_ppc_store_hdecr(env, hdec);
 
@@ -1791,14 +1791,8 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
  * and it's not obviously worth a new data structure to do it.
  */
 
-env->tb_env->tb_offset += hv_state.tb_offset;
 spapr_cpu->in_nested = true;
 
-hreg_compute_hflags(env);
-ppc_maybe_interrupt(env);
-tlb_flush(cs);
-env->reserve_addr = -1; /* Reset the reservation */
-
 /*
  * The spapr hcall helper sets env->gpr[3] to the return value, but at
  * this point the L1 is not returning from the hcall but rather we
@@ -1812,51 +1806,69 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
 {
 CPUPPCState *env = >env;
 SpaprCpuState 

[RFC PATCH 2/4] spapr: Add a nested state struct

[Nicholas Piggin]

Rather than use a copy of CPUPPCState to store the host state while
the environment has been switched to the L2, use a new struct for
this purpose.

Have helper functions to save and load this host state.

Signed-off-by: Nicholas Piggin 
---
 hw/ppc/spapr_hcall.c| 164 
 include/hw/ppc/spapr_cpu_core.h |   5 +-
 2 files changed, 129 insertions(+), 40 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index be225adaf6..6679150ac7 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1544,6 +1544,126 @@ static target_ulong h_copy_tofrom_guest(PowerPCCPU *cpu,
 return H_FUNCTION;
 }
 
+struct nested_ppc_state {
+uint64_t gpr[32];
+uint64_t lr;
+uint64_t ctr;
+uint64_t cfar;
+uint64_t msr;
+uint64_t nip;
+uint32_t cr;
+
+uint64_t xer;
+
+uint64_t lpcr;
+uint64_t lpidr;
+uint64_t pidr;
+uint64_t pcr;
+uint64_t dpdes;
+uint64_t hfscr;
+uint64_t srr0;
+uint64_t srr1;
+uint64_t sprg0;
+uint64_t sprg1;
+uint64_t sprg2;
+uint64_t sprg3;
+uint64_t ppr;
+
+int64_t tb_offset;
+};
+
+static void nested_save_state(struct nested_ppc_state *save, PowerPCCPU *cpu)
+{
+CPUPPCState *env = >env;
+uint32_t cr;
+int i;
+
+memcpy(save->gpr, env->gpr, sizeof(save->gpr));
+
+save->lr = env->lr;
+save->ctr = env->ctr;
+save->cfar = env->cfar;
+save->msr = env->msr;
+save->nip = env->nip;
+
+cr = 0;
+for (i = 0; i < 8; i++) {
+cr |= (env->crf[i] & 15) << (4 * (7 - i));
+}
+save->cr = cr;
+
+save->xer = cpu_read_xer(env);
+
+save->lpcr = env->spr[SPR_LPCR];
+save->lpidr = env->spr[SPR_LPIDR];
+save->pcr = env->spr[SPR_PCR];
+save->dpdes = env->spr[SPR_DPDES];
+save->hfscr = env->spr[SPR_HFSCR];
+save->srr0 = env->spr[SPR_SRR0];
+save->srr1 = env->spr[SPR_SRR1];
+save->sprg0 = env->spr[SPR_SPRG0];
+save->sprg1 = env->spr[SPR_SPRG1];
+save->sprg2 = env->spr[SPR_SPRG2];
+save->sprg3 = env->spr[SPR_SPRG3];
+save->pidr = env->spr[SPR_BOOKS_PID];
+save->ppr = env->spr[SPR_PPR];
+
+save->tb_offset = env->tb_env->tb_offset;
+}
+
+static void nested_load_state(PowerPCCPU *cpu, struct nested_ppc_state *load)
+{
+CPUState *cs = CPU(cpu);
+CPUPPCState *env = >env;
+uint32_t cr;
+int i;
+
+memcpy(env->gpr, load->gpr, sizeof(env->gpr));
+
+env->lr = load->lr;
+env->ctr = load->ctr;
+env->cfar = load->cfar;
+env->msr = load->msr;
+env->nip = load->nip;
+
+cr = load->cr;
+for (i = 7; i >= 0; i--) {
+env->crf[i] = cr & 15;
+cr >>= 4;
+}
+
+cpu_write_xer(env, load->xer);
+
+env->spr[SPR_LPCR] = load->lpcr;
+env->spr[SPR_LPIDR] = load->lpidr;
+env->spr[SPR_PCR] = load->pcr;
+env->spr[SPR_DPDES] = load->dpdes;
+env->spr[SPR_HFSCR] = load->hfscr;
+env->spr[SPR_SRR0] = load->srr0;
+env->spr[SPR_SRR1] = load->srr1;
+env->spr[SPR_SPRG0] = load->sprg0;
+env->spr[SPR_SPRG1] = load->sprg1;
+env->spr[SPR_SPRG2] = load->sprg2;
+env->spr[SPR_SPRG3] = load->sprg3;
+env->spr[SPR_BOOKS_PID] = load->pidr;
+env->spr[SPR_PPR] = load->ppr;
+
+env->tb_env->tb_offset = load->tb_offset;
+
+/*
+ * MSR updated, compute hflags and possible interrupts.
+ */
+hreg_compute_hflags(env);
+ppc_maybe_interrupt(env);
+
+/*
+ * Nested HV does not tag TLB entries between L1 and L2, so must
+ * flush on transition.
+ */
+tlb_flush(cs);
+env->reserve_addr = -1; /* Reset the reservation */
+}
+
 /*
  * When this handler returns, the environment is switched to the L2 guest
  * and TCG begins running that. spapr_exit_nested() performs the switch from
@@ -1593,12 +1713,14 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
 return H_PARAMETER;
 }
 
-spapr_cpu->nested_host_state = g_try_new(CPUPPCState, 1);
+spapr_cpu->nested_host_state = g_try_new(struct nested_ppc_state, 1);
 if (!spapr_cpu->nested_host_state) {
 return H_NO_MEM;
 }
 
-memcpy(spapr_cpu->nested_host_state, env, sizeof(CPUPPCState));
+assert(env->spr[SPR_LPIDR] == 0);
+assert(env->spr[SPR_DPDES] == 0);
+nested_save_state(spapr_cpu->nested_host_state, cpu);
 
 len = sizeof(*regs);
 regs = address_space_map(CPU(cpu)->as, regs_ptr, , false,
@@ -1644,7 +1766,6 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
 env->spr[SPR_DPDES] = hv_state.dpdes;
 env->spr[SPR_HFSCR] = hv_state.hfscr;
 hdec = hv_state.hdec_expiry - now;
-spapr_cpu->nested_tb_offset = hv_state.tb_offset;
 /* TCG does not implement DAWR*, CIABR, PURR, SPURR, IC, VTB, HEIR SPRs*/
 env->spr[SPR_SRR0] = hv_state.srr0;
 env->spr[SPR_SRR1] = hv_state.srr1;
@@ -1670,7 +1791,7 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
  * and it's not obviously worth a new data structure to do it.
  */
 
-

[RFC PATCH 0/4] spapr: clean up nested hv

[Nicholas Piggin]

Something like this is the way I'd been wanting to refactor nested hv.
The state load/store functions and data is (somewhat) abstracted, and
the hcall interface remains in the hcall handlers.

If, hypothetically, you had a new flavour of nested enter hcall that had
some other way of specifying the L2 state to load, then you would
(hopefully) be able to extend and reuse the state struct and load/store
helpers.

Thanks,
Nick

Nicholas Piggin (4):
  spapr: H_ENTER_NESTED should restore host XER ca field
  spapr: Add a nested state struct
  spapr: load and store l2 state with helper functions
  spapr: Move spapr nested HV to a new file

 hw/ppc/meson.build  |   1 +
 hw/ppc/spapr_hcall.c| 348 +-
 hw/ppc/spapr_nested.c   | 496 
 include/hw/ppc/spapr.h  |  61 +---
 include/hw/ppc/spapr_cpu_core.h |   5 +-
 5 files changed, 502 insertions(+), 409 deletions(-)
 create mode 100644 hw/ppc/spapr_nested.c

-- 
2.40.1

Re: [PULL 61/73] hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register

[Leonardo Brás]

Hello Michael, Juan, Peter,

On Wed, 2023-04-26 at 09:19 +0200, Juan Quintela wrote:
> "Michael S. Tsirkin"  wrote:
> > On Tue, Apr 25, 2023 at 08:42:17PM -0400, Peter Xu wrote:
> > > Hi, Michael, Jonathan,
> > > 
> > > On Tue, Mar 07, 2023 at 08:13:53PM -0500, Michael S. Tsirkin wrote:
> > > This breaks the simplest migration from QEMU 8.0->7.2 binaries on all
> > > machine types I think as long as the cap is present, e.g. the default
> > > e1000e provided by the default q35 machine can already hit it with all
> > > default cmdline:
> > > 
> > >   ./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]
> > > 
> > > 7.2 binary will have empty wmask for PCI_ERR_UNCOR_MASK, meanwhile I think
> > > it can also see a non-zero value, then the migration will fail at:
> > > 
> > > vmstate_load :00:02.0/e1000e, e1000e  
> > >  
> > > qemu-7.2: get_pci_config_device: Bad config data: i=0x10a read: 40 
> > > device: 0 cmask: ff wmask: 0 w1cmask:0
> > > qemu-7.2: Failed to load PCIDevice:config   
> > > qemu-7.2: Failed to load e1000e:parent_obj
> > > 
> > > qemu-7.2: error while loading state for instance 0x0 of device 
> > > ':00:02.0/e1000e'  
> > > qemu-7.2: load of migration failed: Invalid argument
> > > 
> > > We probably at least want to have the default value to be still zero, and
> > > we'd need to make sure it'll not be modified by the guest, iiuc.
> > > 
> > > Below oneliner works for me and makes the migration work again:
> > > 
> > > ===8<===
> > > diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> > > index 103667c368..563a37b79c 100644
> > > --- a/hw/pci/pcie_aer.c
> > > +++ b/hw/pci/pcie_aer.c
> > > @@ -113,7 +113,7 @@ int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver, 
> > > uint16_t offset,
> > >  pci_set_long(dev->w1cmask + offset + PCI_ERR_UNCOR_STATUS,
> > >   PCI_ERR_UNC_SUPPORTED);
> > >  pci_set_long(dev->config + offset + PCI_ERR_UNCOR_MASK,
> > > - PCI_ERR_UNC_MASK_DEFAULT);
> > > + 0/*PCI_ERR_UNC_MASK_DEFAULT*/);
> > >  pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_MASK,
> > >   PCI_ERR_UNC_SUPPORTED);
> > > ===8<===
> > > 
> > > Anyone could have a look on a solid solution from PCI side?
> > > 
> > > Copy Juan and Leonardo.
> > > 
> > > Thanks,
> > 
> > My bad, I forgot about this 臘.
> > So we need a property and tweak it with compat machinery depending on
> > machine type. Jonathan, can you work on this pls?
> > Or I can revert for now to relieve the time pressure,
> > redo the patch at your leasure.
> 
> I agree with Michael here, the best option is adding a new property.
> 
> Later, Juan.
> 

I sent a patch implementing the suggested fix:
https://lore.kernel.org/qemu-devel/20230503002701.854329-1-leob...@redhat.com/T/#u

Please let me know of anything to improve.

Best regards,
Leo

[PATCH v1 1/1] hw/pci: Disable PCI_ERR_UNCOR_MASK register for machine type < 8.0

[Leonardo Bras]

Since it's implementation on v8.0.0-rc0, having the PCI_ERR_UNCOR_MASK
set for machine types < 8.0 will cause migration to fail if the target
QEMU version is < 8.0.0 :

qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10a read: 40 
device: 0 cmask: ff wmask: 0 w1cmask:0
qemu-system-x86_64: Failed to load PCIDevice:config
qemu-system-x86_64: Failed to load e1000e:parent_obj
qemu-system-x86_64: error while loading state for instance 0x0 of device 
':00:02.0/e1000e'
qemu-system-x86_64: load of migration failed: Invalid argument

The above test migrated a 7.2 machine type from QEMU master to QEMU 7.2.0,
with this cmdline:

./qemu-system-x86_64 -M pc-q35-7.2 [-incoming XXX]

In order to fix this, property x-pcie-err-unc-mask was introduced to
control when PCI_ERR_UNCOR_MASK is enabled. This property is enabled by
default, but is disabled if machine type <= 7.2.

Fixes: 010746ae1d ("hw/pci/aer: Implement PCI_ERR_UNCOR_MASK register")
Suggested-by: Michael S. Tsirkin 
Signed-off-by: Leonardo Bras 
---
 include/hw/pci/pci.h |  2 ++
 hw/core/machine.c|  1 +
 hw/pci/pci.c |  2 ++
 hw/pci/pcie_aer.c| 11 +++
 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 935b4b91b4..e6d0574a29 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -207,6 +207,8 @@ enum {
 QEMU_PCIE_EXTCAP_INIT = (1 << QEMU_PCIE_EXTCAP_INIT_BITNR),
 #define QEMU_PCIE_CXL_BITNR 10
 QEMU_PCIE_CAP_CXL = (1 << QEMU_PCIE_CXL_BITNR),
+#define QEMU_PCIE_ERR_UNC_MASK_BITNR 11
+QEMU_PCIE_ERR_UNC_MASK = (1 << QEMU_PCIE_ERR_UNC_MASK_BITNR),
 };
 
 typedef struct PCIINTxRoute {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 47a34841a5..07f763eb2e 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -48,6 +48,7 @@ GlobalProperty hw_compat_7_2[] = {
 { "e1000e", "migrate-timadj", "off" },
 { "virtio-mem", "x-early-migration", "false" },
 { "migration", "x-preempt-pre-7-2", "true" },
+{ TYPE_PCI_DEVICE, "x-pcie-err-unc-mask", "off" },
 };
 const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
 
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 8a87ccc8b0..5153ad63d6 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -79,6 +79,8 @@ static Property pci_props[] = {
 DEFINE_PROP_STRING("failover_pair_id", PCIDevice,
failover_pair_id),
 DEFINE_PROP_UINT32("acpi-index",  PCIDevice, acpi_index, 0),
+DEFINE_PROP_BIT("x-pcie-err-unc-mask", PCIDevice, cap_present,
+QEMU_PCIE_ERR_UNC_MASK_BITNR, true),
 DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index 103667c368..374d593ead 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -112,10 +112,13 @@ int pcie_aer_init(PCIDevice *dev, uint8_t cap_ver, 
uint16_t offset,
 
 pci_set_long(dev->w1cmask + offset + PCI_ERR_UNCOR_STATUS,
  PCI_ERR_UNC_SUPPORTED);
-pci_set_long(dev->config + offset + PCI_ERR_UNCOR_MASK,
- PCI_ERR_UNC_MASK_DEFAULT);
-pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_MASK,
- PCI_ERR_UNC_SUPPORTED);
+
+if (dev->cap_present & QEMU_PCIE_ERR_UNC_MASK) {
+pci_set_long(dev->config + offset + PCI_ERR_UNCOR_MASK,
+ PCI_ERR_UNC_MASK_DEFAULT);
+pci_set_long(dev->wmask + offset + PCI_ERR_UNCOR_MASK,
+ PCI_ERR_UNC_SUPPORTED);
+}
 
 pci_set_long(dev->config + offset + PCI_ERR_UNCOR_SEVER,
  PCI_ERR_UNC_SEVERITY_DEFAULT);
-- 
2.40.0

[PULL v3 04/10] xen-hvm: reorganize xen-hvm and move common function to xen-hvm-common

[Stefano Stabellini]

From: Stefano Stabellini 

This patch does following:
1. creates arch_handle_ioreq() and arch_xen_set_memory(). This is done in
preparation for moving most of xen-hvm code to an arch-neutral location,
move the x86-specific portion of xen_set_memory to arch_xen_set_memory.
Also, move handle_vmport_ioreq to arch_handle_ioreq.

2. Pure code movement: move common functions to hw/xen/xen-hvm-common.c
Extract common functionalities from hw/i386/xen/xen-hvm.c and move them to
hw/xen/xen-hvm-common.c. These common functions are useful for creating
an IOREQ server.

xen_hvm_init_pc() contains the architecture independent code for creating
and mapping a IOREQ server, connecting memory and IO listeners, initializing
a xen bus and registering backends. Moved this common xen code to a new
function xen_register_ioreq() which can be used by both x86 and ARM 
machines.

Following functions are moved to hw/xen/xen-hvm-common.c:
xen_vcpu_eport(), xen_vcpu_ioreq(), xen_ram_alloc(), xen_set_memory(),
xen_region_add(), xen_region_del(), xen_io_add(), xen_io_del(),
xen_device_realize(), xen_device_unrealize(),
cpu_get_ioreq_from_shared_memory(), cpu_get_ioreq(), do_inp(),
do_outp(), rw_phys_req_item(), read_phys_req_item(),
write_phys_req_item(), cpu_ioreq_pio(), cpu_ioreq_move(),
cpu_ioreq_config(), handle_ioreq(), handle_buffered_iopage(),
handle_buffered_io(), cpu_handle_ioreq(), xen_main_loop_prepare(),
xen_hvm_change_state_handler(), xen_exit_notifier(),
xen_map_ioreq_server(), destroy_hvm_domain() and
xen_shutdown_fatal_error()

3. Removed static type from below functions:
1. xen_region_add()
2. xen_region_del()
3. xen_io_add()
4. xen_io_del()
5. xen_device_realize()
6. xen_device_unrealize()
7. xen_hvm_change_state_handler()
8. cpu_ioreq_pio()
9. xen_exit_notifier()

4. Replace TARGET_PAGE_SIZE with XC_PAGE_SIZE to match the page side with Xen.

Signed-off-by: Vikram Garhwal 
Signed-off-by: Stefano Stabellini 
Reviewed-by: Stefano Stabellini 
Reviewed-by: Paul Durrant 
---
 hw/i386/xen/trace-events|   14 -
 hw/i386/xen/xen-hvm.c   | 1016 ++-
 hw/xen/meson.build  |5 +-
 hw/xen/trace-events |   14 +
 hw/xen/xen-hvm-common.c |  860 ++
 include/hw/i386/xen_arch_hvm.h  |   11 +
 include/hw/xen/arch_hvm.h   |3 +
 include/hw/xen/xen-hvm-common.h |   99 +++
 8 files changed, 1054 insertions(+), 968 deletions(-)
 create mode 100644 hw/xen/xen-hvm-common.c
 create mode 100644 include/hw/i386/xen_arch_hvm.h
 create mode 100644 include/hw/xen/arch_hvm.h
 create mode 100644 include/hw/xen/xen-hvm-common.h

diff --git a/hw/i386/xen/trace-events b/hw/i386/xen/trace-events
index a0c89d91c4..5d0a8d6dcf 100644
--- a/hw/i386/xen/trace-events
+++ b/hw/i386/xen/trace-events
@@ -7,17 +7,3 @@ xen_platform_log(char *s) "xen platform: %s"
 xen_pv_mmio_read(uint64_t addr) "WARNING: read from Xen PV Device MMIO space 
(address 0x%"PRIx64")"
 xen_pv_mmio_write(uint64_t addr) "WARNING: write to Xen PV Device MMIO space 
(address 0x%"PRIx64")"
 
-# xen-hvm.c
-xen_ram_alloc(unsigned long ram_addr, unsigned long size) "requested: 0x%lx, 
size 0x%lx"
-xen_client_set_memory(uint64_t start_addr, unsigned long size, bool log_dirty) 
"0x%"PRIx64" size 0x%lx, log_dirty %i"
-handle_ioreq(void *req, uint32_t type, uint32_t dir, uint32_t df, uint32_t 
data_is_ptr, uint64_t addr, uint64_t data, uint32_t count, uint32_t size) 
"I/O=%p type=%d dir=%d df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d 
size=%d"
-handle_ioreq_read(void *req, uint32_t type, uint32_t df, uint32_t data_is_ptr, 
uint64_t addr, uint64_t data, uint32_t count, uint32_t size) "I/O=%p read 
type=%d df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d size=%d"
-handle_ioreq_write(void *req, uint32_t type, uint32_t df, uint32_t 
data_is_ptr, uint64_t addr, uint64_t data, uint32_t count, uint32_t size) 
"I/O=%p write type=%d df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d 
size=%d"
-cpu_ioreq_pio(void *req, uint32_t dir, uint32_t df, uint32_t data_is_ptr, 
uint64_t addr, uint64_t data, uint32_t count, uint32_t size) "I/O=%p pio dir=%d 
df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d size=%d"
-cpu_ioreq_pio_read_reg(void *req, uint64_t data, uint64_t addr, uint32_t size) 
"I/O=%p pio read reg data=0x%"PRIx64" port=0x%"PRIx64" size=%d"
-cpu_ioreq_pio_write_reg(void *req, uint64_t data, uint64_t addr, uint32_t 
size) "I/O=%p pio write reg data=0x%"PRIx64" port=0x%"PRIx64" size=%d"
-cpu_ioreq_move(void *req, uint32_t dir, uint32_t df, uint32_t data_is_ptr, 
uint64_t addr, uint64_t data, uint32_t count, uint32_t size) "I/O=%p copy 
dir=%d df=%d ptr=%d port=0x%"PRIx64" data=0x%"PRIx64" count=%d size=%d"
-xen_map_resource_ioreq(uint32_t id, void *addr) "id: %u addr: %p"
-cpu_ioreq_config_read(void 

[PULL v3 05/10] include/hw/xen/xen_common: return error from xen_create_ioreq_server

[Stefano Stabellini]

From: Stefano Stabellini 

This is done to prepare for enabling xenpv support for ARM architecture.
On ARM it is possible to have a functioning xenpv machine with only the
PV backends and no IOREQ server. If the IOREQ server creation fails,
continue to the PV backends initialization.

Signed-off-by: Stefano Stabellini 
Signed-off-by: Vikram Garhwal 
Reviewed-by: Stefano Stabellini 
Reviewed-by: Paul Durrant 
---
 include/hw/xen/xen_native.h | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/hw/xen/xen_native.h b/include/hw/xen/xen_native.h
index 6bcc83baf9..8b01b071e5 100644
--- a/include/hw/xen/xen_native.h
+++ b/include/hw/xen/xen_native.h
@@ -433,9 +433,10 @@ static inline void xen_unmap_pcidev(domid_t dom,
 {
 }
 
-static inline void xen_create_ioreq_server(domid_t dom,
-   ioservid_t *ioservid)
+static inline int xen_create_ioreq_server(domid_t dom,
+  ioservid_t *ioservid)
 {
+return 0;
 }
 
 static inline void xen_destroy_ioreq_server(domid_t dom,
@@ -566,8 +567,8 @@ static inline void xen_unmap_pcidev(domid_t dom,
   PCI_FUNC(pci_dev->devfn));
 }
 
-static inline void xen_create_ioreq_server(domid_t dom,
-   ioservid_t *ioservid)
+static inline int xen_create_ioreq_server(domid_t dom,
+  ioservid_t *ioservid)
 {
 int rc = xendevicemodel_create_ioreq_server(xen_dmod, dom,
 HVM_IOREQSRV_BUFIOREQ_ATOMIC,
@@ -575,12 +576,14 @@ static inline void xen_create_ioreq_server(domid_t dom,
 
 if (rc == 0) {
 trace_xen_ioreq_server_create(*ioservid);
-return;
+return rc;
 }
 
 *ioservid = 0;
 use_default_ioreq_server = true;
 trace_xen_default_ioreq_server();
+
+return rc;
 }
 
 static inline void xen_destroy_ioreq_server(domid_t dom,
-- 
2.25.1

[PULL v3 03/10] hw/i386/xen/xen-hvm: move x86-specific fields out of XenIOState

[Stefano Stabellini]

From: Stefano Stabellini 

In preparation to moving most of xen-hvm code to an arch-neutral location, move:
- shared_vmport_page
- log_for_dirtybit
- dirty_bitmap
- suspend
- wakeup

out of XenIOState struct as these are only used on x86, especially the ones
related to dirty logging.
Updated XenIOState can be used for both aarch64 and x86.

Also, remove free_phys_offset as it was unused.

Signed-off-by: Stefano Stabellini 
Signed-off-by: Vikram Garhwal 
Reviewed-by: Paul Durrant 
Reviewed-by: Alex Bennée 
---
 hw/i386/xen/xen-hvm.c | 58 ---
 1 file changed, 27 insertions(+), 31 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 5403ac4b89..6be5a250a8 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -74,6 +74,7 @@ struct shared_vmport_iopage {
 };
 typedef struct shared_vmport_iopage shared_vmport_iopage_t;
 #endif
+static shared_vmport_iopage_t *shared_vmport_page;
 
 static inline uint32_t xen_vcpu_eport(shared_iopage_t *shared_page, int i)
 {
@@ -96,6 +97,11 @@ typedef struct XenPhysmap {
 } XenPhysmap;
 
 static QLIST_HEAD(, XenPhysmap) xen_physmap;
+static const XenPhysmap *log_for_dirtybit;
+/* Buffer used by xen_sync_dirty_bitmap */
+static unsigned long *dirty_bitmap;
+static Notifier suspend;
+static Notifier wakeup;
 
 typedef struct XenPciDevice {
 PCIDevice *pci_dev;
@@ -106,7 +112,6 @@ typedef struct XenPciDevice {
 typedef struct XenIOState {
 ioservid_t ioservid;
 shared_iopage_t *shared_page;
-shared_vmport_iopage_t *shared_vmport_page;
 buffered_iopage_t *buffered_io_page;
 xenforeignmemory_resource_handle *fres;
 QEMUTimer *buffered_io_timer;
@@ -126,14 +131,8 @@ typedef struct XenIOState {
 MemoryListener io_listener;
 QLIST_HEAD(, XenPciDevice) dev_list;
 DeviceListener device_listener;
-hwaddr free_phys_offset;
-const XenPhysmap *log_for_dirtybit;
-/* Buffer used by xen_sync_dirty_bitmap */
-unsigned long *dirty_bitmap;
 
 Notifier exit;
-Notifier suspend;
-Notifier wakeup;
 } XenIOState;
 
 /* Xen specific function for piix pci */
@@ -463,10 +462,10 @@ static int xen_remove_from_physmap(XenIOState *state,
 }
 
 QLIST_REMOVE(physmap, list);
-if (state->log_for_dirtybit == physmap) {
-state->log_for_dirtybit = NULL;
-g_free(state->dirty_bitmap);
-state->dirty_bitmap = NULL;
+if (log_for_dirtybit == physmap) {
+log_for_dirtybit = NULL;
+g_free(dirty_bitmap);
+dirty_bitmap = NULL;
 }
 g_free(physmap);
 
@@ -627,16 +626,16 @@ static void xen_sync_dirty_bitmap(XenIOState *state,
 return;
 }
 
-if (state->log_for_dirtybit == NULL) {
-state->log_for_dirtybit = physmap;
-state->dirty_bitmap = g_new(unsigned long, bitmap_size);
-} else if (state->log_for_dirtybit != physmap) {
+if (log_for_dirtybit == NULL) {
+log_for_dirtybit = physmap;
+dirty_bitmap = g_new(unsigned long, bitmap_size);
+} else if (log_for_dirtybit != physmap) {
 /* Only one range for dirty bitmap can be tracked. */
 return;
 }
 
 rc = xen_track_dirty_vram(xen_domid, start_addr >> TARGET_PAGE_BITS,
-  npages, state->dirty_bitmap);
+  npages, dirty_bitmap);
 if (rc < 0) {
 #ifndef ENODATA
 #define ENODATA  ENOENT
@@ -651,7 +650,7 @@ static void xen_sync_dirty_bitmap(XenIOState *state,
 }
 
 for (i = 0; i < bitmap_size; i++) {
-unsigned long map = state->dirty_bitmap[i];
+unsigned long map = dirty_bitmap[i];
 while (map != 0) {
 j = ctzl(map);
 map &= ~(1ul << j);
@@ -677,12 +676,10 @@ static void xen_log_start(MemoryListener *listener,
 static void xen_log_stop(MemoryListener *listener, MemoryRegionSection 
*section,
  int old, int new)
 {
-XenIOState *state = container_of(listener, XenIOState, memory_listener);
-
 if (old & ~new & (1 << DIRTY_MEMORY_VGA)) {
-state->log_for_dirtybit = NULL;
-g_free(state->dirty_bitmap);
-state->dirty_bitmap = NULL;
+log_for_dirtybit = NULL;
+g_free(dirty_bitmap);
+dirty_bitmap = NULL;
 /* Disable dirty bit tracking */
 xen_track_dirty_vram(xen_domid, 0, 0, NULL);
 }
@@ -1022,9 +1019,9 @@ static void handle_vmport_ioreq(XenIOState *state, 
ioreq_t *req)
 {
 vmware_regs_t *vmport_regs;
 
-assert(state->shared_vmport_page);
+assert(shared_vmport_page);
 vmport_regs =
->shared_vmport_page->vcpu_vmport_regs[state->send_vcpu];
+_vmport_page->vcpu_vmport_regs[state->send_vcpu];
 QEMU_BUILD_BUG_ON(sizeof(*req) < sizeof(*vmport_regs));
 
 current_cpu = state->cpu_by_vcpu_id[state->send_vcpu];
@@ -1472,7 +1469,6 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion 
**ram_memory)
 
 state->memory_listener = xen_memory_listener;
 

[PULL v3 08/10] meson.build: do not set have_xen_pci_passthrough for aarch64 targets

[Stefano Stabellini]

From: Stefano Stabellini 

have_xen_pci_passthrough is only used for Xen x86 VMs.

Signed-off-by: Stefano Stabellini 
Reviewed-by: Alex Bennée 
---
 meson.build | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/meson.build b/meson.build
index 77d42898c8..000ef06bfa 100644
--- a/meson.build
+++ b/meson.build
@@ -1471,6 +1471,8 @@ have_xen_pci_passthrough = 
get_option('xen_pci_passthrough') \
error_message: 'Xen PCI passthrough requested but Xen not enabled') 
\
   .require(targetos == 'linux',
error_message: 'Xen PCI passthrough not available on this 
platform') \
+  .require(cpu == 'x86'  or cpu == 'x86_64',
+   error_message: 'Xen PCI passthrough not available on this 
platform') \
   .allowed()
 
 
-- 
2.25.1

[PULL v3 09/10] hw/arm: introduce xenpvh machine

[Stefano Stabellini]

From: Vikram Garhwal 

Add a new machine xenpvh which creates a IOREQ server to register/connect with
Xen Hypervisor.

Optional: When CONFIG_TPM is enabled, it also creates a tpm-tis-device, adds a
TPM emulator and connects to swtpm running on host machine via chardev socket
and support TPM functionalities for a guest domain.

Extra command line for aarch64 xenpvh QEMU to connect to swtpm:
-chardev socket,id=chrtpm,path=/tmp/myvtpm2/swtpm-sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm \
-machine tpm-base-addr=0x0c00 \

swtpm implements a TPM software emulator(TPM 1.2 & TPM 2) built on libtpms and
provides access to TPM functionality over socket, chardev and CUSE interface.
Github repo: https://github.com/stefanberger/swtpm
Example for starting swtpm on host machine:
mkdir /tmp/vtpm2
swtpm socket --tpmstate dir=/tmp/vtpm2 \
--ctrl type=unixio,path=/tmp/vtpm2/swtpm-sock &

Signed-off-by: Vikram Garhwal 
Signed-off-by: Stefano Stabellini 
Reviewed-by: Stefano Stabellini 
---
 docs/system/arm/xenpvh.rst|  34 +++
 docs/system/target-arm.rst|   1 +
 hw/arm/meson.build|   2 +
 hw/arm/xen_arm.c  | 181 ++
 include/hw/arm/xen_arch_hvm.h |   9 ++
 include/hw/xen/arch_hvm.h |   2 +
 6 files changed, 229 insertions(+)
 create mode 100644 docs/system/arm/xenpvh.rst
 create mode 100644 hw/arm/xen_arm.c
 create mode 100644 include/hw/arm/xen_arch_hvm.h

diff --git a/docs/system/arm/xenpvh.rst b/docs/system/arm/xenpvh.rst
new file mode 100644
index 00..e1655c7ab8
--- /dev/null
+++ b/docs/system/arm/xenpvh.rst
@@ -0,0 +1,34 @@
+XENPVH (``xenpvh``)
+=
+This machine creates a IOREQ server to register/connect with Xen Hypervisor.
+
+When TPM is enabled, this machine also creates a tpm-tis-device at a user input
+tpm base address, adds a TPM emulator and connects to a swtpm application
+running on host machine via chardev socket. This enables xenpvh to support TPM
+functionalities for a guest domain.
+
+More information about TPM use and installing swtpm linux application can be
+found at: docs/specs/tpm.rst.
+
+Example for starting swtpm on host machine:
+.. code-block:: console
+
+mkdir /tmp/vtpm2
+swtpm socket --tpmstate dir=/tmp/vtpm2 \
+--ctrl type=unixio,path=/tmp/vtpm2/swtpm-sock &
+
+Sample QEMU xenpvh commands for running and connecting with Xen:
+.. code-block:: console
+
+qemu-system-aarch64 -xen-domid 1 \
+-chardev socket,id=libxl-cmd,path=qmp-libxl-1,server=on,wait=off \
+-mon chardev=libxl-cmd,mode=control \
+-chardev socket,id=libxenstat-cmd,path=qmp-libxenstat-1,server=on,wait=off 
\
+-mon chardev=libxenstat-cmd,mode=control \
+-xen-attach -name guest0 -vnc none -display none -nographic \
+-machine xenpvh -m 1301 \
+-chardev socket,id=chrtpm,path=tmp/vtpm2/swtpm-sock \
+-tpmdev emulator,id=tpm0,chardev=chrtpm -machine tpm-base-addr=0x0C00
+
+In above QEMU command, last two lines are for connecting xenpvh QEMU to swtpm
+via chardev socket.
diff --git a/docs/system/target-arm.rst b/docs/system/target-arm.rst
index 91ebc26c6d..af8d7c77d6 100644
--- a/docs/system/target-arm.rst
+++ b/docs/system/target-arm.rst
@@ -106,6 +106,7 @@ undocumented; you can get a complete list by running
arm/stm32
arm/virt
arm/xlnx-versal-virt
+   arm/xenpvh
 
 Emulated CPU architecture support
 =
diff --git a/hw/arm/meson.build b/hw/arm/meson.build
index b545ba0e4f..1b2a01a005 100644
--- a/hw/arm/meson.build
+++ b/hw/arm/meson.build
@@ -62,6 +62,8 @@ arm_ss.add(when: 'CONFIG_FSL_IMX7', if_true: 
files('fsl-imx7.c', 'mcimx7d-sabre.
 arm_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmuv3.c'))
 arm_ss.add(when: 'CONFIG_FSL_IMX6UL', if_true: files('fsl-imx6ul.c', 
'mcimx6ul-evk.c'))
 arm_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_soc.c'))
+arm_ss.add(when: 'CONFIG_XEN', if_true: files('xen_arm.c'))
+arm_ss.add_all(xen_ss)
 
 softmmu_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmu-common.c'))
 softmmu_ss.add(when: 'CONFIG_EXYNOS4', if_true: files('exynos4_boards.c'))
diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
new file mode 100644
index 00..19b1cb81ad
--- /dev/null
+++ b/hw/arm/xen_arm.c
@@ -0,0 +1,181 @@
+/*
+ * QEMU ARM Xen PVH Machine
+ *
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED 

[PULL v3 06/10] hw/xen/xen-hvm-common: skip ioreq creation on ioreq registration failure

[Stefano Stabellini]

From: Stefano Stabellini 

On ARM it is possible to have a functioning xenpv machine with only the
PV backends and no IOREQ server. If the IOREQ server creation fails continue
to the PV backends initialization.

Also, moved the IOREQ registration and mapping subroutine to new function
xen_do_ioreq_register().

Signed-off-by: Stefano Stabellini 
Signed-off-by: Vikram Garhwal 
Reviewed-by: Stefano Stabellini 
Reviewed-by: Paul Durrant 
---
 hw/xen/xen-hvm-common.c | 57 +++--
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
index a31b067404..cb82f4b83d 100644
--- a/hw/xen/xen-hvm-common.c
+++ b/hw/xen/xen-hvm-common.c
@@ -764,27 +764,12 @@ void xen_shutdown_fatal_error(const char *fmt, ...)
 qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);
 }
 
-void xen_register_ioreq(XenIOState *state, unsigned int max_cpus,
-MemoryListener xen_memory_listener)
+static void xen_do_ioreq_register(XenIOState *state,
+   unsigned int max_cpus,
+   MemoryListener xen_memory_listener)
 {
 int i, rc;
 
-setup_xen_backend_ops();
-
-state->xce_handle = qemu_xen_evtchn_open();
-if (state->xce_handle == NULL) {
-perror("xen: event channel open");
-goto err;
-}
-
-state->xenstore = xs_daemon_open();
-if (state->xenstore == NULL) {
-perror("xen: xenstore open");
-goto err;
-}
-
-xen_create_ioreq_server(xen_domid, >ioservid);
-
 state->exit.notify = xen_exit_notifier;
 qemu_add_exit_notifier(>exit);
 
@@ -849,12 +834,46 @@ void xen_register_ioreq(XenIOState *state, unsigned int 
max_cpus,
 QLIST_INIT(>dev_list);
 device_listener_register(>device_listener);
 
+return;
+
+err:
+error_report("xen hardware virtual machine initialisation failed");
+exit(1);
+}
+
+void xen_register_ioreq(XenIOState *state, unsigned int max_cpus,
+MemoryListener xen_memory_listener)
+{
+int rc;
+
+setup_xen_backend_ops();
+
+state->xce_handle = qemu_xen_evtchn_open();
+if (state->xce_handle == NULL) {
+perror("xen: event channel open");
+goto err;
+}
+
+state->xenstore = xs_daemon_open();
+if (state->xenstore == NULL) {
+perror("xen: xenstore open");
+goto err;
+}
+
+rc = xen_create_ioreq_server(xen_domid, >ioservid);
+if (!rc) {
+xen_do_ioreq_register(state, max_cpus, xen_memory_listener);
+} else {
+warn_report("xen: failed to create ioreq server");
+}
+
 xen_bus_init();
 
 xen_be_init();
 
 return;
+
 err:
-error_report("xen hardware virtual machine initialisation failed");
+error_report("xen hardware virtual machine backend registration failed");
 exit(1);
 }
-- 
2.25.1

[PULL v3 10/10] meson.build: enable xenpv machine build for ARM

[Stefano Stabellini]

From: Vikram Garhwal 

Add CONFIG_XEN for aarch64 device to support build for ARM targets.

Signed-off-by: Vikram Garhwal 
Signed-off-by: Stefano Stabellini 
Reviewed-by: Alex Bennée 
---
 meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 000ef06bfa..d51cd7d35c 100644
--- a/meson.build
+++ b/meson.build
@@ -135,7 +135,7 @@ endif
 if cpu in ['x86', 'x86_64', 'arm', 'aarch64']
   # i386 emulator provides xenpv machine type for multiple architectures
   accelerator_targets += {
-'CONFIG_XEN': ['i386-softmmu', 'x86_64-softmmu'],
+'CONFIG_XEN': ['i386-softmmu', 'x86_64-softmmu', 'aarch64-softmmu'],
   }
 endif
 if cpu in ['x86', 'x86_64']
-- 
2.25.1

[PULL v3 07/10] hw/xen/xen-hvm-common: Use g_new and error_report

[Stefano Stabellini]

From: Vikram Garhwal 

Replace g_malloc with g_new and perror with error_report.

Signed-off-by: Vikram Garhwal 
Reviewed-by: Stefano Stabellini 
Reviewed-by: Paul Durrant 
---
 hw/xen/xen-hvm-common.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
index cb82f4b83d..42339c96bd 100644
--- a/hw/xen/xen-hvm-common.c
+++ b/hw/xen/xen-hvm-common.c
@@ -33,7 +33,7 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, 
MemoryRegion *mr,
 trace_xen_ram_alloc(ram_addr, size);
 
 nr_pfn = size >> TARGET_PAGE_BITS;
-pfn_list = g_malloc(sizeof (*pfn_list) * nr_pfn);
+pfn_list = g_new(xen_pfn_t, nr_pfn);
 
 for (i = 0; i < nr_pfn; i++) {
 pfn_list[i] = (ram_addr >> TARGET_PAGE_BITS) + i;
@@ -730,7 +730,7 @@ void destroy_hvm_domain(bool reboot)
 return;
 }
 if (errno != ENOTTY /* old Xen */) {
-perror("xendevicemodel_shutdown failed");
+error_report("xendevicemodel_shutdown failed with error %d", 
errno);
 }
 /* well, try the old thing then */
 }
@@ -784,7 +784,7 @@ static void xen_do_ioreq_register(XenIOState *state,
 }
 
 /* Note: cpus is empty at this point in init */
-state->cpu_by_vcpu_id = g_malloc0(max_cpus * sizeof(CPUState *));
+state->cpu_by_vcpu_id = g_new0(CPUState *, max_cpus);
 
 rc = xen_set_ioreq_server_state(xen_domid, state->ioservid, true);
 if (rc < 0) {
@@ -793,7 +793,7 @@ static void xen_do_ioreq_register(XenIOState *state,
 goto err;
 }
 
-state->ioreq_local_port = g_malloc0(max_cpus * sizeof (evtchn_port_t));
+state->ioreq_local_port = g_new0(evtchn_port_t, max_cpus);
 
 /* FIXME: how about if we overflow the page here? */
 for (i = 0; i < max_cpus; i++) {
@@ -850,13 +850,13 @@ void xen_register_ioreq(XenIOState *state, unsigned int 
max_cpus,
 
 state->xce_handle = qemu_xen_evtchn_open();
 if (state->xce_handle == NULL) {
-perror("xen: event channel open");
+error_report("xen: event channel open failed with error %d", errno);
 goto err;
 }
 
 state->xenstore = xs_daemon_open();
 if (state->xenstore == NULL) {
-perror("xen: xenstore open");
+error_report("xen: xenstore open failed with error %d", errno);
 goto err;
 }
 
-- 
2.25.1

[PULL v3 01/10] hw/i386/xen/: move xen-mapcache.c to hw/xen/

[Stefano Stabellini]

From: Vikram Garhwal 

xen-mapcache.c contains common functions which can be used for enabling Xen on
aarch64 with IOREQ handling. Moving it out from hw/i386/xen to hw/xen to make it
accessible for both aarch64 and x86.

Signed-off-by: Vikram Garhwal 
Signed-off-by: Stefano Stabellini 
Reviewed-by: Paul Durrant 
---
 hw/i386/meson.build  | 1 +
 hw/i386/xen/meson.build  | 1 -
 hw/i386/xen/trace-events | 5 -
 hw/xen/meson.build   | 4 
 hw/xen/trace-events  | 5 +
 hw/{i386 => }/xen/xen-mapcache.c | 0
 6 files changed, 10 insertions(+), 6 deletions(-)
 rename hw/{i386 => }/xen/xen-mapcache.c (100%)

diff --git a/hw/i386/meson.build b/hw/i386/meson.build
index 213e2e82b3..cfdbfdcbcb 100644
--- a/hw/i386/meson.build
+++ b/hw/i386/meson.build
@@ -33,5 +33,6 @@ subdir('kvm')
 subdir('xen')
 
 i386_ss.add_all(xenpv_ss)
+i386_ss.add_all(xen_ss)
 
 hw_arch += {'i386': i386_ss}
diff --git a/hw/i386/xen/meson.build b/hw/i386/xen/meson.build
index 2e64a34e16..3dc4c4f106 100644
--- a/hw/i386/xen/meson.build
+++ b/hw/i386/xen/meson.build
@@ -1,6 +1,5 @@
 i386_ss.add(when: 'CONFIG_XEN', if_true: files(
   'xen-hvm.c',
-  'xen-mapcache.c',
   'xen_apic.c',
   'xen_pvdevice.c',
 ))
diff --git a/hw/i386/xen/trace-events b/hw/i386/xen/trace-events
index 5d6be61090..a0c89d91c4 100644
--- a/hw/i386/xen/trace-events
+++ b/hw/i386/xen/trace-events
@@ -21,8 +21,3 @@ xen_map_resource_ioreq(uint32_t id, void *addr) "id: %u addr: 
%p"
 cpu_ioreq_config_read(void *req, uint32_t sbdf, uint32_t reg, uint32_t size, 
uint32_t data) "I/O=%p sbdf=0x%x reg=%u size=%u data=0x%x"
 cpu_ioreq_config_write(void *req, uint32_t sbdf, uint32_t reg, uint32_t size, 
uint32_t data) "I/O=%p sbdf=0x%x reg=%u size=%u data=0x%x"
 
-# xen-mapcache.c
-xen_map_cache(uint64_t phys_addr) "want 0x%"PRIx64
-xen_remap_bucket(uint64_t index) "index 0x%"PRIx64
-xen_map_cache_return(void* ptr) "%p"
-
diff --git a/hw/xen/meson.build b/hw/xen/meson.build
index 19c6aabc7c..202752e557 100644
--- a/hw/xen/meson.build
+++ b/hw/xen/meson.build
@@ -26,3 +26,7 @@ else
 endif
 
 specific_ss.add_all(when: ['CONFIG_XEN', xen], if_true: xen_specific_ss)
+
+xen_ss = ss.source_set()
+
+xen_ss.add(when: 'CONFIG_XEN', if_true: files('xen-mapcache.c'))
diff --git a/hw/xen/trace-events b/hw/xen/trace-events
index 55c9e1df68..f977c7c8c6 100644
--- a/hw/xen/trace-events
+++ b/hw/xen/trace-events
@@ -41,3 +41,8 @@ xs_node_vprintf(char *path, char *value) "%s %s"
 xs_node_vscanf(char *path, char *value) "%s %s"
 xs_node_watch(char *path) "%s"
 xs_node_unwatch(char *path) "%s"
+
+# xen-mapcache.c
+xen_map_cache(uint64_t phys_addr) "want 0x%"PRIx64
+xen_remap_bucket(uint64_t index) "index 0x%"PRIx64
+xen_map_cache_return(void* ptr) "%p"
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
similarity index 100%
rename from hw/i386/xen/xen-mapcache.c
rename to hw/xen/xen-mapcache.c
-- 
2.25.1

[PULL v3 0/10] xenpvh3-tag

[Stefano Stabellini]

Hi Peter,

Vikram fixed the gitlab test problem, so now all the tests should
succeed. There were no changes to the QEMU code. I am resending the pull
request (I rebased it on staging, no conflicts.)

For reference this was the previous pull request:
https://marc.info/?l=qemu-devel=167641819725964

Cheers,

Stefano


The following changes since commit 4ebc33f3f3b656ebf62112daca6aa0f8019b4891:

  Merge tag 'pull-tcg-20230502-2' of https://gitlab.com/rth7680/qemu into 
staging (2023-05-02 21:18:45 +0100)

are available in the Git repository at:

  https://gitlab.com/sstabellini/qemu xenpvh3-tag

for you to fetch changes up to bc618c54318cbc2fcb9decf9d4c193cc336a0dbc:

  meson.build: enable xenpv machine build for ARM (2023-05-02 17:04:54 -0700)


Stefano Stabellini (5):
  hw/i386/xen/xen-hvm: move x86-specific fields out of XenIOState
  xen-hvm: reorganize xen-hvm and move common function to xen-hvm-common
  include/hw/xen/xen_common: return error from xen_create_ioreq_server
  hw/xen/xen-hvm-common: skip ioreq creation on ioreq registration failure
  meson.build: do not set have_xen_pci_passthrough for aarch64 targets

Vikram Garhwal (5):
  hw/i386/xen/: move xen-mapcache.c to hw/xen/
  hw/i386/xen: rearrange xen_hvm_init_pc
  hw/xen/xen-hvm-common: Use g_new and error_report
  hw/arm: introduce xenpvh machine
  meson.build: enable xenpv machine build for ARM

 docs/system/arm/xenpvh.rst   |   34 ++
 docs/system/target-arm.rst   |1 +
 hw/arm/meson.build   |2 +
 hw/arm/xen_arm.c |  181 +++
 hw/i386/meson.build  |1 +
 hw/i386/xen/meson.build  |1 -
 hw/i386/xen/trace-events |   19 -
 hw/i386/xen/xen-hvm.c| 1075 --
 hw/xen/meson.build   |7 +
 hw/xen/trace-events  |   19 +
 hw/xen/xen-hvm-common.c  |  879 +++
 hw/{i386 => }/xen/xen-mapcache.c |0
 include/hw/arm/xen_arch_hvm.h|9 +
 include/hw/i386/xen_arch_hvm.h   |   11 +
 include/hw/xen/arch_hvm.h|5 +
 include/hw/xen/xen-hvm-common.h  |   99 
 include/hw/xen/xen_native.h  |   13 +-
 meson.build  |4 +-
 18 files changed, 1350 insertions(+), 1010 deletions(-)
 create mode 100644 docs/system/arm/xenpvh.rst
 create mode 100644 hw/arm/xen_arm.c
 create mode 100644 hw/xen/xen-hvm-common.c
 rename hw/{i386 => }/xen/xen-mapcache.c (100%)
 create mode 100644 include/hw/arm/xen_arch_hvm.h
 create mode 100644 include/hw/i386/xen_arch_hvm.h
 create mode 100644 include/hw/xen/arch_hvm.h
 create mode 100644 include/hw/xen/xen-hvm-common.h

[PULL v3 02/10] hw/i386/xen: rearrange xen_hvm_init_pc

[Stefano Stabellini]

From: Vikram Garhwal 

In preparation to moving most of xen-hvm code to an arch-neutral location,
move non IOREQ references to:
- xen_get_vmport_regs_pfn
- xen_suspend_notifier
- xen_wakeup_notifier
- xen_ram_init

towards the end of the xen_hvm_init_pc() function.

This is done to keep the common ioreq functions in one place which will be
moved to new function in next patch in order to make it common to both x86 and
aarch64 machines.

Signed-off-by: Vikram Garhwal 
Signed-off-by: Stefano Stabellini 
Reviewed-by: Paul Durrant 
---
 hw/i386/xen/xen-hvm.c | 49 ++-
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 56641a550e..5403ac4b89 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1419,12 +1419,6 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion 
**ram_memory)
 state->exit.notify = xen_exit_notifier;
 qemu_add_exit_notifier(>exit);
 
-state->suspend.notify = xen_suspend_notifier;
-qemu_register_suspend_notifier(>suspend);
-
-state->wakeup.notify = xen_wakeup_notifier;
-qemu_register_wakeup_notifier(>wakeup);
-
 /*
  * Register wake-up support in QMP query-current-machine API
  */
@@ -1435,23 +1429,6 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion 
**ram_memory)
 goto err;
 }
 
-rc = xen_get_vmport_regs_pfn(xen_xc, xen_domid, _pfn);
-if (!rc) {
-DPRINTF("shared vmport page at pfn %lx\n", ioreq_pfn);
-state->shared_vmport_page =
-xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
- 1, _pfn, NULL);
-if (state->shared_vmport_page == NULL) {
-error_report("map shared vmport IO page returned error %d 
handle=%p",
- errno, xen_xc);
-goto err;
-}
-} else if (rc != -ENOSYS) {
-error_report("get vmport regs pfn returned error %d, rc=%d",
- errno, rc);
-goto err;
-}
-
 /* Note: cpus is empty at this point in init */
 state->cpu_by_vcpu_id = g_new0(CPUState *, max_cpus);
 
@@ -1490,7 +1467,6 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion 
**ram_memory)
 #else
 xen_map_cache_init(NULL, state);
 #endif
-xen_ram_init(pcms, ms->ram_size, ram_memory);
 
 qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
 
@@ -1511,6 +1487,31 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion 
**ram_memory)
 QLIST_INIT(_physmap);
 xen_read_physmap(state);
 
+state->suspend.notify = xen_suspend_notifier;
+qemu_register_suspend_notifier(>suspend);
+
+state->wakeup.notify = xen_wakeup_notifier;
+qemu_register_wakeup_notifier(>wakeup);
+
+rc = xen_get_vmport_regs_pfn(xen_xc, xen_domid, _pfn);
+if (!rc) {
+DPRINTF("shared vmport page at pfn %lx\n", ioreq_pfn);
+state->shared_vmport_page =
+xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
+ 1, _pfn, NULL);
+if (state->shared_vmport_page == NULL) {
+error_report("map shared vmport IO page returned error %d 
handle=%p",
+ errno, xen_xc);
+goto err;
+}
+} else if (rc != -ENOSYS) {
+error_report("get vmport regs pfn returned error %d, rc=%d",
+ errno, rc);
+goto err;
+}
+
+xen_ram_init(pcms, ms->ram_size, ram_memory);
+
 /* Disable ACPI build because Xen handles it */
 pcms->acpi_build_enabled = false;
 
-- 
2.25.1

Re: [PATCH 3/8] migration: Add precopy initial data loaded ACK functionality

[Peter Xu]

On Mon, May 01, 2023 at 05:01:36PM +0300, Avihai Horon wrote:
> Add the core functionality of precopy initial data, which allows the
> destination to ACK that initial data has been loaded and the source to
> wait for this ACK before completing the migration.
> 
> A new return path command MIG_RP_MSG_INITIAL_DATA_LOADED_ACK is added.
> It is sent by the destination after precopy initial data is loaded to
> ACK to the source that precopy initial data has been loaded.
> 
> In addition, two new SaveVMHandlers handlers are added:
> 1. is_initial_data_active which indicates whether precopy initial data
>is used for this migration user (i.e., SaveStateEntry).
> 2. initial_data_loaded which indicates whether precopy initial data has
>been loaded by this migration user.
> 
> Signed-off-by: Avihai Horon 
> ---
>  include/migration/register.h |  7 ++
>  migration/migration.h| 12 +++
>  migration/migration.c| 41 ++--
>  migration/savevm.c   | 39 ++
>  migration/trace-events   |  2 ++
>  5 files changed, 99 insertions(+), 2 deletions(-)
> 
> diff --git a/include/migration/register.h b/include/migration/register.h
> index 0a73f3883e..297bbe9f73 100644
> --- a/include/migration/register.h
> +++ b/include/migration/register.h
> @@ -77,6 +77,13 @@ typedef struct SaveVMHandlers {
>   * true if it's supported or false otherwise. Called both in src and 
> dest.
>   */
>  bool (*initial_data_advise)(void *opaque);
> +/*
> + * Checks if precopy initial data is active. If it's inactive,
> + * initial_data_loaded check is skipped.
> + */
> +bool (*is_initial_data_active)(void *opaque);
> +/* Checks if precopy initial data has been loaded in dest */
> +bool (*initial_data_loaded)(void *opaque);
>  } SaveVMHandlers;
>  
>  int register_savevm_live(const char *idstr,
> diff --git a/migration/migration.h b/migration/migration.h
> index 4f615e9dbc..d865c23d87 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -207,6 +207,11 @@ struct MigrationIncomingState {
>  
>  /* Indicates whether precopy initial data was enabled and should be used 
> */
>  bool initial_data_enabled;
> +/*
> + * Indicates whether an ACK that precopy initial data was loaded has been
> + * sent to source.
> + */
> +bool initial_data_loaded_ack_sent;
>  };
>  
>  MigrationIncomingState *migration_incoming_get_current(void);
> @@ -435,6 +440,12 @@ struct MigrationState {
>  
>  /* QEMU_VM_VMDESCRIPTION content filled for all non-iterable devices. */
>  JSONWriter *vmdesc;
> +
> +/*
> + * Indicates whether an ACK that precopy initial data was loaded in
> + * destination has been received.
> + */
> +bool initial_data_loaded_acked;
>  };
>  
>  void migrate_set_state(int *state, int old_state, int new_state);
> @@ -475,6 +486,7 @@ int 
> migrate_send_rp_message_req_pages(MigrationIncomingState *mis,
>  void migrate_send_rp_recv_bitmap(MigrationIncomingState *mis,
>   char *block_name);
>  void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
> +int migrate_send_rp_initial_data_loaded_ack(MigrationIncomingState *mis);
>  
>  void dirty_bitmap_mig_before_vm_start(void);
>  void dirty_bitmap_mig_cancel_outgoing(void);
> diff --git a/migration/migration.c b/migration/migration.c
> index 68cdf5b184..304cab2fa1 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -77,6 +77,11 @@ enum mig_rp_message_type {
>  MIG_RP_MSG_RECV_BITMAP,  /* send recved_bitmap back to source */
>  MIG_RP_MSG_RESUME_ACK,   /* tell source that we are ready to resume */
>  
> +MIG_RP_MSG_INITIAL_DATA_LOADED_ACK, /*
> + * Tell source precopy initial data 
> is
> + * loaded.
> + */
> +
>  MIG_RP_MSG_MAX
>  };
>  
> @@ -756,6 +761,12 @@ bool migration_has_all_channels(void)
>  return true;
>  }
>  
> +int migrate_send_rp_initial_data_loaded_ack(MigrationIncomingState *mis)
> +{
> +return migrate_send_rp_message(mis, MIG_RP_MSG_INITIAL_DATA_LOADED_ACK, 
> 0,
> +   NULL);
> +}
> +
>  /*
>   * Send a 'SHUT' message on the return channel with the given value
>   * to indicate that we've finished with the RP.  Non-0 value indicates
> @@ -1401,6 +1412,8 @@ void migrate_init(MigrationState *s)
>  s->vm_was_running = false;
>  s->iteration_initial_bytes = 0;
>  s->threshold_size = 0;
> +
> +s->initial_data_loaded_acked = false;
>  }
>  
>  int migrate_add_blocker_internal(Error *reason, Error **errp)
> @@ -1717,6 +1730,9 @@ static struct rp_cmd_args {
>  [MIG_RP_MSG_REQ_PAGES_ID]   = { .len = -1, .name = "REQ_PAGES_ID" },
>  [MIG_RP_MSG_RECV_BITMAP]= { .len = -1, .name = "RECV_BITMAP" },
>  

Re: [PATCH 2/8] migration: Add precopy initial data handshake

[Peter Xu]

(I've left high level comment in cover letter, but still some quick
comments I noticed when reading)

On Mon, May 01, 2023 at 05:01:35PM +0300, Avihai Horon wrote:
> Add precopy initial data handshake between source and destination upon
> migration setup. The purpose of the handshake is to notify the
> destination that precopy initial data is used and which migration users
> (i.e., SaveStateEntry) are going to use it.
> 
> The handshake is done in two levels. First, a general enable command is
> sent to notify the destination migration code that precopy initial data
> is used.
> Then, for each migration user in the source that supports precopy
> initial data, an enable command is sent to its counterpart in the
> destination:
> If both support it, precopy initial data will be used for them.
> If source doesn't support it, precopy initial data will not be used for
> them.
> If source supports it and destination doesn't, migration will be failed.
> 
> To implement it, a new migration command MIG_CMD_INITIAL_DATA_ENABLE is
> added, as well as a new SaveVMHandlers handler initial_data_advise.
> Calling the handler advises the migration user that precopy initial data
> is used and its return value indicates whether precopy initial data is
> supported by it.
> 
> Signed-off-by: Avihai Horon 
> ---
>  include/migration/register.h |   6 +++
>  migration/migration.h|   3 ++
>  migration/savevm.h   |   1 +
>  migration/migration.c|   4 ++
>  migration/savevm.c   | 102 +++
>  migration/trace-events   |   2 +
>  6 files changed, 118 insertions(+)
> 
> diff --git a/include/migration/register.h b/include/migration/register.h
> index a8dfd8fefd..0a73f3883e 100644
> --- a/include/migration/register.h
> +++ b/include/migration/register.h
> @@ -71,6 +71,12 @@ typedef struct SaveVMHandlers {
>  int (*load_cleanup)(void *opaque);
>  /* Called when postcopy migration wants to resume from failure */
>  int (*resume_prepare)(MigrationState *s, void *opaque);
> +
> +/*
> + * Advises that precopy initial data was requested to be enabled. Returns
> + * true if it's supported or false otherwise. Called both in src and 
> dest.
> + */
> +bool (*initial_data_advise)(void *opaque);
>  } SaveVMHandlers;
>  
>  int register_savevm_live(const char *idstr,
> diff --git a/migration/migration.h b/migration/migration.h
> index 3a918514e7..4f615e9dbc 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -204,6 +204,9 @@ struct MigrationIncomingState {
>   * contains valid information.
>   */
>  QemuMutex page_request_mutex;
> +
> +/* Indicates whether precopy initial data was enabled and should be used 
> */
> +bool initial_data_enabled;
>  };
>  
>  MigrationIncomingState *migration_incoming_get_current(void);
> diff --git a/migration/savevm.h b/migration/savevm.h
> index fb636735f0..d47ab4ad18 100644
> --- a/migration/savevm.h
> +++ b/migration/savevm.h
> @@ -58,6 +58,7 @@ void qemu_savevm_send_postcopy_ram_discard(QEMUFile *f, 
> const char *name,
> uint64_t *start_list,
> uint64_t *length_list);
>  void qemu_savevm_send_colo_enable(QEMUFile *f);
> +void qemu_savevm_send_initial_data_enable(MigrationState *ms, QEMUFile *f);
>  void qemu_savevm_live_state(QEMUFile *f);
>  int qemu_save_device_state(QEMUFile *f);
>  
> diff --git a/migration/migration.c b/migration/migration.c
> index abcadbb619..68cdf5b184 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2964,6 +2964,10 @@ static void *migration_thread(void *opaque)
>  qemu_savevm_send_colo_enable(s->to_dst_file);
>  }
>  
> +if (migrate_precopy_initial_data()) {
> +qemu_savevm_send_initial_data_enable(s, s->to_dst_file);
> +}
> +
>  qemu_savevm_state_setup(s->to_dst_file);
>  
>  qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
> diff --git a/migration/savevm.c b/migration/savevm.c
> index a9181b444b..2740defdf0 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -71,6 +71,13 @@
>  
>  const unsigned int postcopy_ram_discard_version;
>  
> +typedef struct {
> +uint8_t general_enable;
> +uint8_t reserved[7];
> +uint8_t idstr[256];
> +uint32_t instance_id;
> +} InitialDataInfo;
> +
>  /* Subcommands for QEMU_VM_COMMAND */
>  enum qemu_vm_cmd {
>  MIG_CMD_INVALID = 0,   /* Must be 0 */
> @@ -90,6 +97,8 @@ enum qemu_vm_cmd {
>  MIG_CMD_ENABLE_COLO,   /* Enable COLO */
>  MIG_CMD_POSTCOPY_RESUME,   /* resume postcopy on dest */
>  MIG_CMD_RECV_BITMAP,   /* Request for recved bitmap on dst */
> +
> +MIG_CMD_INITIAL_DATA_ENABLE, /* Enable precopy initial data in dest */
>  MIG_CMD_MAX
>  };
>  
> @@ -109,6 +118,8 @@ static struct mig_cmd_args {
>  [MIG_CMD_POSTCOPY_RESUME]  = { .len =  0, .name = "POSTCOPY_RESUME" },
>  [MIG_CMD_PACKAGED] 

Re: [PATCH 0/8] migration: Add precopy initial data capability and VFIO precopy support

[Peter Xu]

On Mon, May 01, 2023 at 05:01:33PM +0300, Avihai Horon wrote:
> Hello everyone,

Hi, Avihai,

> === Flow of operation ===
> 
> To use precopy initial data, the capability must be enabled in the
> source.
> 
> As this capability must be supported also in the destination, a
> handshake is performed during migration setup. The purpose of the
> handshake is to notify the destination that precopy initial data is used
> and to check if it's supported.
> 
> The handshake is done in two levels. First, a general handshake is done
> with the destination migration code to notify that precopy initial data
> is used. Then, for each migration user in the source that supports
> precopy initial data, a handshake is done with its counterpart in the
> destination:
> If both support it, precopy initial data will be used for them.
> If source doesn't support it, precopy initial data will not be used for
> them.
> If source supports it and destination doesn't, migration will be failed.
> 
> Assuming the handshake succeeded, migration starts to send precopy data
> and as part of it also the initial precopy data. Initial precopy data is
> just like any other precopy data and as such, migration code is not
> aware of it. Therefore, it's the responsibility of the migration users
> (such as VFIO devices) to notify their counterparts in the destination
> that their initial precopy data has been sent (for example, VFIO
> migration does it when its initial bytes reach zero).
> 
> In the destination, migration code will query each migration user that
> supports precopy initial data and check if its initial data has been
> loaded. If initial data has been loaded by all of them, an ACK will be
> sent to the source which will now be able to complete migration when
> appropriate.

I can understand why this is useful, what I'm not 100% sure is whether the
complexity is needed.  The idea seems to be that src never switchover
unless it receives a READY notification from dst.

I'm imaging below simplified and more general workflow, not sure whether it
could work for you:

  - Introduce a new cap "switchover-ready", it means whether there'll be a
ready event sent from dst -> src for "being ready for switchover"

  - When cap set, a new msg MIG_RP_MSG_SWITCHOVER_READY is defined and
handled on src showing that dest is ready for switchover. It'll be sent
only if dest is ready for the switchover

  - Introduce a field SaveVMHandlers.explicit_switchover_needed.  For each
special device like vfio that would like to participate in the decision
making, device can set its explicit_switchover_needed=1.  This field is
ignored if the new cap is not set.

  - Dst qemu: when new cap set, remember how many special devices are there
requesting explicit switchover (count of SaveVMHandlers that has the
bit set during load setup) as switch_over_pending=N.

  - Dst qemu: Once a device thinks its fine to switchover (probably in the
load_state() callback), it calls migration_notify_switchover_ready().
That decreases switch_over_pending and when it hits zero, one msg
MIG_RP_MSG_SWITCHOVER_READY will be sent to src.

Only until READY msg received on src could src switchover the precopy to
dst.

Then it only needs 1 more field in SaveVMHandlers rather than 3, and only 1
more msg (dst->src).

This is based on the fact that right now we always set caps on both qemus
so I suppose it already means either both have or don't have the feature
(even if one has, not setting the cap means disabled on both).

Would it work for this case and cleaner?

Thanks,

-- 
Peter Xu

Re: [RFC PATCH 1/4] pci: add handling of Enable bit in ATS Control Register

[Viktor Prutyanov]

On Wed, Apr 26, 2023 at 8:32 AM Jason Wang  wrote:
>
>
> 在 2023/4/24 19:21, Viktor Prutyanov 写道:
> > According to PCIe Address Translation Services specification 5.1.3.,
> > ATS Control Register has Enable bit to enable/disable ATS.
> > Add a new field for a trigger function which is called at the Enable
> > bit change, so that PCIe devices can handle ATS enable/disable.
> >
> > Signed-off-by: Viktor Prutyanov 
> > ---
> >   hw/pci/pci.c|  1 +
> >   hw/pci/pcie.c   | 21 +
> >   include/hw/pci/pci_device.h |  3 +++
> >   include/hw/pci/pcie.h   |  4 
> >   4 files changed, 29 insertions(+)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 208c16f450..79a47d2589 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1550,6 +1550,7 @@ void pci_default_write_config(PCIDevice *d, uint32_t 
> > addr, uint32_t val_in, int
> >   msi_write_config(d, addr, val_in, l);
> >   msix_write_config(d, addr, val_in, l);
> >   pcie_sriov_config_write(d, addr, val_in, l);
> > +pcie_ats_config_write(d, addr, val_in, l);
> >   }
> >
> >   /***/
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 924fdabd15..e0217161e5 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -1057,6 +1057,27 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset, 
> > bool aligned)
> >   pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f);
> >   }
> >
> > +void pcie_ats_config_write(PCIDevice *dev, uint32_t address, uint32_t val,
> > +   int len)
> > +{
> > +uint32_t off;
> > +uint16_t ats_cap = dev->exp.ats_cap;
> > +
> > +if (!ats_cap || address < ats_cap) {
> > +return;
> > +}
> > +off = address - ats_cap;
> > +if (off >= PCI_EXT_CAP_ATS_SIZEOF) {
> > +return;
> > +}
> > +
> > +if (range_covers_byte(off, len, PCI_ATS_CTRL + 1)) {
>
>
> Do we really need +1 here?

The Enable bit is the 15th in the ATS Control Register, so it is in a
byte next to PCI_ATS_CTRL. Although I'm not sure that this is the
best way to test this bit.

All other comments I tried to take into account in the v2.

Thanks,
Viktor Prutyanov

>
> The rest looks good.
>
> Thanks
>
>
> > +if (dev->ats_ctrl_trigger) {
> > +dev->ats_ctrl_trigger(dev, !!(val & PCI_ATS_CTRL_ENABLE));
> > +}
> > +}
> > +}
> > +
> >   /* ACS (Access Control Services) */
> >   void pcie_acs_init(PCIDevice *dev, uint16_t offset)
> >   {
> > diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h
> > index d3dd0f64b2..2bb1d68f3b 100644
> > --- a/include/hw/pci/pci_device.h
> > +++ b/include/hw/pci/pci_device.h
> > @@ -160,6 +160,9 @@ struct PCIDevice {
> >   /* ID of standby device in net_failover pair */
> >   char *failover_pair_id;
> >   uint32_t acpi_index;
> > +
> > +/* PCI ATS enable/disable trigger */
> > +void (*ats_ctrl_trigger)(PCIDevice *dev, bool enable);
> >   };
> >
> >   static inline int pci_intx(PCIDevice *pci_dev)
> > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> > index 798a262a0a..5f2dbd87cf 100644
> > --- a/include/hw/pci/pcie.h
> > +++ b/include/hw/pci/pcie.h
> > @@ -154,4 +154,8 @@ void pcie_cap_slot_unplug_cb(HotplugHandler 
> > *hotplug_dev, DeviceState *dev,
> >Error **errp);
> >   void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
> >DeviceState *dev, Error **errp);
> > +
> > +void pcie_ats_config_write(PCIDevice *dev, uint32_t address, uint32_t val,
> > +   int len);
> > +
> >   #endif /* QEMU_PCIE_H */
>

Re: [PATCH] migration: Attempt disk reactivation in more failure scenarios

[Peter Xu]

On Tue, May 02, 2023 at 03:52:12PM -0500, Eric Blake wrote:
> Commit fe904ea824 added a fail_inactivate label, which tries to
> reactivate disks on the source after a failure while s->state ==
> MIGRATION_STATUS_ACTIVE, but didn't actually use the label if
> qemu_savevm_state_complete_precopy() failed.  This failure to
> reactivate is also present in commit 6039dd5b1c (also covering the new
> s->state == MIGRATION_STATUS_DEVICE state) and 403d18ae (ensuring
> s->block_inactive is set more reliably).
> 
> Consolidate the two labels back into one - no matter HOW migration is
> failed, if there is any chance we can reach vm_start() after having
> attempted inactivation, it is essential that we have tried to restart
> disks before then.  This also makes the cleanup more like
> migrate_fd_cancel().
> 
> Suggested-by: Kevin Wolf 
> Signed-off-by: Eric Blake 

Acked-by: Peter Xu 

-- 
Peter Xu

[PATCH] block/export: call blk_set_dev_ops(blk, NULL, NULL)

[Stefan Hajnoczi]

Most export types install BlockDeviceOps pointers. It is easy to forget
to remove them because that happens automatically via the "drive" qdev
property in hw/ but not block/export/.

Put blk_set_dev_ops(blk, NULL, NULL) calls in the core export.c code so
the export types don't need to remember.

This fixes the nbd and vhost-user-blk export types.

Fixes: fd6afc501a01 ("nbd/server: Use drained block ops to quiesce the server")
Fixes: ca858a5fe94c ("vhost-user-blk-server: notify client about disk resize")
Signed-off-by: Stefan Hajnoczi 
---
 block/export/export.c| 2 ++
 block/export/vduse-blk.c | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/export/export.c b/block/export/export.c
index e3fee60611..62c7c22d45 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -192,6 +192,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error 
**errp)
 return exp;
 
 fail:
+blk_set_dev_ops(exp->blk, NULL, NULL);
 blk_unref(blk);
 aio_context_release(ctx);
 if (exp) {
@@ -219,6 +220,7 @@ static void blk_exp_delete_bh(void *opaque)
 assert(exp->refcount == 0);
 QLIST_REMOVE(exp, next);
 exp->drv->delete(exp);
+blk_set_dev_ops(exp->blk, NULL, NULL);
 blk_unref(exp->blk);
 qapi_event_send_block_export_deleted(exp->id);
 g_free(exp->id);
diff --git a/block/export/vduse-blk.c b/block/export/vduse-blk.c
index f7ae44e3ce..b53ef39da0 100644
--- a/block/export/vduse-blk.c
+++ b/block/export/vduse-blk.c
@@ -346,7 +346,6 @@ static void vduse_blk_exp_delete(BlockExport *exp)
 
 blk_remove_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach,
 vblk_exp);
-blk_set_dev_ops(exp->blk, NULL, NULL);
 ret = vduse_dev_destroy(vblk_exp->dev);
 if (ret != -EBUSY) {
 unlink(vblk_exp->recon_file);
-- 
2.40.1

Re: [PATCH v4 02/10] colo: make colo_checkpoint_notify static and provide simpler API

[Peter Xu]

On Fri, Apr 28, 2023 at 10:49:20PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> colo_checkpoint_notify() is mostly used in colo.c. Outside we use it
> once when x-checkpoint-delay migration parameter is set. So, let's
> simplify the external API to only that function - notify COLO that
> parameter was set. This make external API more robust and hides
> implementation details from external callers. Also this helps us to
> make COLO module optional in further patch (i.e. we are going to add
> possibility not build the COLO module).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Reviewed-by: Peter Xu 

-- 
Peter Xu

Re: [PATCH v4 10/10] migration: block incoming colo when capability is disabled

[Peter Xu]

On Fri, Apr 28, 2023 at 10:49:28PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We generally require same set of capabilities on source and target.
> Let's require x-colo capability to use COLO on target.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Reviewed-by: Peter Xu 

-- 
Peter Xu

Re: [PATCH v4 09/10] migration: disallow change capabilities in COLO state

[Peter Xu]

On Fri, Apr 28, 2023 at 10:49:27PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> COLO is not listed as running state in migrate_is_running(), so, it's
> theoretically possible to disable colo capability in COLO state and the
> unexpected error in migration_iteration_finish() is reachable.
> 
> Let's disallow that in qmp_migrate_set_capabilities. Than the error
> becomes absolutely unreachable: we can get into COLO state only with
> enabled capability and can't disable it while we are in COLO state. So
> substitute the error by simple assertion.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Reviewed-by: Peter Xu 

-- 
Peter Xu

Re: [PATCH v4 08/10] migration: process_incoming_migration_co(): move colo part to colo

[Peter Xu]

On Fri, Apr 28, 2023 at 10:49:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> +int coroutine_fn colo_incoming_co(void)
> +{
> +MigrationIncomingState *mis = migration_incoming_get_current();
> +Error *local_err = NULL;
> +QemuThread th;
> +
> +assert(!qemu_mutex_iothread_locked());

Is this assert reverted?  I assume it wants to guarantee BQL held rather
than not..

> +
> +if (!migration_incoming_colo_enabled()) {
> +return 0;
> +}
> +
> +/* Make sure all file formats throw away their mutable metadata */
> +bdrv_activate_all(_err);
> +if (local_err) {
> +error_report_err(local_err);
> +return -EINVAL;
> +}
> +
> +qemu_thread_create(, "COLO incoming", colo_process_incoming_thread,
> +   mis, QEMU_THREAD_JOINABLE);
> +
> +mis->colo_incoming_co = qemu_coroutine_self();
> +qemu_coroutine_yield();
> +mis->colo_incoming_co = NULL;
> +
> +qemu_mutex_unlock_iothread();
> +/* Wait checkpoint incoming thread exit before free resource */
> +qemu_thread_join();
> +qemu_mutex_lock_iothread();
> +
> +/* We hold the global iothread lock, so it is safe here */
> +colo_release_ram_cache();
> +
> +return 0;
> +}

-- 
Peter Xu

[PATCH] migration: Attempt disk reactivation in more failure scenarios

[Eric Blake]

Commit fe904ea824 added a fail_inactivate label, which tries to
reactivate disks on the source after a failure while s->state ==
MIGRATION_STATUS_ACTIVE, but didn't actually use the label if
qemu_savevm_state_complete_precopy() failed.  This failure to
reactivate is also present in commit 6039dd5b1c (also covering the new
s->state == MIGRATION_STATUS_DEVICE state) and 403d18ae (ensuring
s->block_inactive is set more reliably).

Consolidate the two labels back into one - no matter HOW migration is
failed, if there is any chance we can reach vm_start() after having
attempted inactivation, it is essential that we have tried to restart
disks before then.  This also makes the cleanup more like
migrate_fd_cancel().

Suggested-by: Kevin Wolf 
Signed-off-by: Eric Blake 
---
 migration/migration.c | 24 ++--
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index abcadbb619e..7f982bd2c80 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2299,6 +2299,11 @@ static void migration_completion(MigrationState *s)
 MIGRATION_STATUS_DEVICE);
 }
 if (ret >= 0) {
+/*
+ * Inactivate disks except in COLO, and track that we
+ * have done so in order to remember to reactivate
+ * them if migration fails or is cancelled.
+ */
 s->block_inactive = !migrate_colo();
 qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
 ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
@@ -2343,13 +2348,13 @@ static void migration_completion(MigrationState *s)
 rp_error = await_return_path_close_on_source(s);
 trace_migration_return_path_end_after(rp_error);
 if (rp_error) {
-goto fail_invalidate;
+goto fail;
 }
 }

 if (qemu_file_get_error(s->to_dst_file)) {
 trace_migration_completion_file_err();
-goto fail_invalidate;
+goto fail;
 }

 if (migrate_colo() && s->state == MIGRATION_STATUS_ACTIVE) {
@@ -2363,26 +2368,25 @@ static void migration_completion(MigrationState *s)

 return;

-fail_invalidate:
-/* If not doing postcopy, vm_start() will be called: let's regain
- * control on images.
- */
-if (s->state == MIGRATION_STATUS_ACTIVE ||
-s->state == MIGRATION_STATUS_DEVICE) {
+fail:
+if (s->block_inactive && (s->state == MIGRATION_STATUS_ACTIVE ||
+  s->state == MIGRATION_STATUS_DEVICE)) {
+/*
+ * If not doing postcopy, vm_start() will be called: let's
+ * regain control on images.
+ */
 Error *local_err = NULL;

 qemu_mutex_lock_iothread();
 bdrv_activate_all(_err);
 if (local_err) {
 error_report_err(local_err);
-s->block_inactive = true;
 } else {
 s->block_inactive = false;
 }
 qemu_mutex_unlock_iothread();
 }

-fail:
 migrate_set_state(>state, current_active_state,
   MIGRATION_STATUS_FAILED);
 }

base-commit: b5f47ba73b7c1457d2f18d71c00e1a91a76fe60b
-- 
2.40.1

Re: [PATCH v4 07/10] migration: split migration_incoming_co

[Peter Xu]

On Fri, Apr 28, 2023 at 10:49:25PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Originally, migration_incoming_co was introduced by
> 25d0c16f625feb3b6
>"migration: Switch to COLO process after finishing loadvm"
> to be able to enter from COLO code to one specific yield point, added
> by 25d0c16f625feb3b6.
> 
> Later in 923709896b1b0
>  "migration: poll the cm event for destination qemu"
> we reused this variable to wake the migration incoming coroutine from
> RDMA code.
> 
> That was doubtful idea. Entering coroutines is a very fragile thing:
> you should be absolutely sure which yield point you are going to enter.
> 
> I don't know how much is it safe to enter during qemu_loadvm_state()
> which I think what RDMA want to do. But for sure RDMA shouldn't enter
> the special COLO-related yield-point. As well, COLO code doesn't want
> to enter during qemu_loadvm_state(), it want to enter it's own specific
> yield-point.
> 
> As well, when in 8e48ac95865ac97d
>  "COLO: Add block replication into colo process" we added
> bdrv_invalidate_cache_all() call (now it's called activate_all())
> it became possible to enter the migration incoming coroutine during
> that call which is wrong too.
> 
> So, let't make these things separate and disjoint: loadvm_co for RDMA,
> non-NULL during qemu_loadvm_state(), and colo_incoming_co for COLO,
> non-NULL only around specific yield.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  migration/colo.c  | 4 ++--
>  migration/migration.c | 8 ++--
>  migration/migration.h | 9 -
>  3 files changed, 16 insertions(+), 5 deletions(-)

The idea looks right to me, but I really know mostly nothing on coroutines
and also rdma+colo..

Is the other ref in rdma.c (rdma_cm_poll_handler()) still missing?

-- 
Peter Xu

Re: [PATCH] ui: Fix pixel colour channel order for PNG screenshots

[BALATON Zoltan]

On Tue, 2 May 2023, Peter Maydell wrote:

When we take a PNG screenshot the ordering of the colour channels in
the data is not correct, resulting in the image having weird
colouring compared to the actual display.  (Specifically, on a
little-endian host the blue and red channels are swapped; on
big-endian everything is wrong.)

This happens because the pixman idea of the pixel data and the libpng
idea differ.  PIXMAN_a9r8g8b8 defines that pixels are 32-bit values,


Typo: should be PIXMAN_a8r8g8b8 not a9

Regards,
BALATON Zoltan


with A in bits 24-31, R in bits 16-23, G in bits 8-15 and B in bits
0-7.  This means that on little-endian systems the bytes in memory
are
  B G R A
and on big-endian systems they are
  A R G B

libpng, on the other hand, thinks of pixels as being a series of
values for each channel, so its format PNG_COLOR_TYPE_RGB_ALPHA
always wants bytes in the order
  R G B A

This isn't the same as the pixman order for either big or little
endian hosts.

The alpha channel is also unnecessary bulk in the output PNG file,
because there is no alpha information in a screenshot.

To handle the endianness issue, we already define in ui/qemu-pixman.h
various PIXMAN_BE_* and PIXMAN_LE_* values that give consistent
byte-order pixel channel formats.  So we can use PIXMAN_BE_r8g8b8 and
PNG_COLOR_TYPE_RGB, which both have an in-memory byte order of
   R G B
and 3 bytes per pixel.

(PPM format screenshots get this right; they already use the
PIXMAN_BE_r8g8b8 format.)

Cc: qemu-sta...@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1622
Fixes: 9a0a119a382867 ("Added parameter to take screenshot with screendump as 
PNG")
Signed-off-by: Peter Maydell 
---
Disclaimer: I don't have a BE system that I have convenient
graphics output from that I can use to test the screenshot
format there.
---
ui/console.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index 6e8a3cdc62d..e173731e205 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -311,7 +311,7 @@ static bool png_save(int fd, pixman_image_t *image, Error 
**errp)
png_struct *png_ptr;
png_info *info_ptr;
g_autoptr(pixman_image_t) linebuf =
-qemu_pixman_linebuf_create(PIXMAN_a8r8g8b8, width);
+qemu_pixman_linebuf_create(PIXMAN_BE_r8g8b8, width);
uint8_t *buf = (uint8_t *)pixman_image_get_data(linebuf);
FILE *f = fdopen(fd, "wb");
int y;
@@ -341,7 +341,7 @@ static bool png_save(int fd, pixman_image_t *image, Error 
**errp)
png_init_io(png_ptr, f);

png_set_IHDR(png_ptr, info_ptr, width, height, 8,
- PNG_COLOR_TYPE_RGB_ALPHA, PNG_INTERLACE_NONE,
+ PNG_COLOR_TYPE_RGB, PNG_INTERLACE_NONE,
 PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE);

png_write_info(png_ptr, info_ptr);

[PULL v2 03/12] qemu/bitops.h: Limit rotate amounts

[Richard Henderson]

From: Dickon Hood 

Rotates have been fixed up to only allow for reasonable rotate amounts
(ie, no rotates >7 on an 8b value etc.)  This fixes a problem with riscv
vector rotate instructions.

Signed-off-by: Dickon Hood 
Reviewed-by: Richard Henderson 
Message-Id: <20230428144757.57530-9-lawrence.hun...@codethink.co.uk>
[rth: Mask shifts in both directions.]
Signed-off-by: Richard Henderson 
---
 include/qemu/bitops.h | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
index 03213ce952..cb3526d1f4 100644
--- a/include/qemu/bitops.h
+++ b/include/qemu/bitops.h
@@ -218,7 +218,7 @@ static inline unsigned long find_first_zero_bit(const 
unsigned long *addr,
  */
 static inline uint8_t rol8(uint8_t word, unsigned int shift)
 {
-return (word << shift) | (word >> ((8 - shift) & 7));
+return (word << (shift & 7)) | (word >> (-shift & 7));
 }
 
 /**
@@ -228,7 +228,7 @@ static inline uint8_t rol8(uint8_t word, unsigned int shift)
  */
 static inline uint8_t ror8(uint8_t word, unsigned int shift)
 {
-return (word >> shift) | (word << ((8 - shift) & 7));
+return (word >> (shift & 7)) | (word << (-shift & 7));
 }
 
 /**
@@ -238,7 +238,7 @@ static inline uint8_t ror8(uint8_t word, unsigned int shift)
  */
 static inline uint16_t rol16(uint16_t word, unsigned int shift)
 {
-return (word << shift) | (word >> ((16 - shift) & 15));
+return (word << (shift & 15)) | (word >> (-shift & 15));
 }
 
 /**
@@ -248,7 +248,7 @@ static inline uint16_t rol16(uint16_t word, unsigned int 
shift)
  */
 static inline uint16_t ror16(uint16_t word, unsigned int shift)
 {
-return (word >> shift) | (word << ((16 - shift) & 15));
+return (word >> (shift & 15)) | (word << (-shift & 15));
 }
 
 /**
@@ -258,7 +258,7 @@ static inline uint16_t ror16(uint16_t word, unsigned int 
shift)
  */
 static inline uint32_t rol32(uint32_t word, unsigned int shift)
 {
-return (word << shift) | (word >> ((32 - shift) & 31));
+return (word << (shift & 31)) | (word >> (-shift & 31));
 }
 
 /**
@@ -268,7 +268,7 @@ static inline uint32_t rol32(uint32_t word, unsigned int 
shift)
  */
 static inline uint32_t ror32(uint32_t word, unsigned int shift)
 {
-return (word >> shift) | (word << ((32 - shift) & 31));
+return (word >> (shift & 31)) | (word << (-shift & 31));
 }
 
 /**
@@ -278,7 +278,7 @@ static inline uint32_t ror32(uint32_t word, unsigned int 
shift)
  */
 static inline uint64_t rol64(uint64_t word, unsigned int shift)
 {
-return (word << shift) | (word >> ((64 - shift) & 63));
+return (word << (shift & 63)) | (word >> (-shift & 63));
 }
 
 /**
@@ -288,7 +288,7 @@ static inline uint64_t rol64(uint64_t word, unsigned int 
shift)
  */
 static inline uint64_t ror64(uint64_t word, unsigned int shift)
 {
-return (word >> shift) | (word << ((64 - shift) & 63));
+return (word >> (shift & 63)) | (word << (-shift & 63));
 }
 
 /**
-- 
2.34.1

[PULL v2 00/12] tcg patch queue

[Richard Henderson]

The following changes since commit c586691e676214eb7edf6a468e84e7ce3b314d43:

  Merge tag 'pull-target-arm-20230502-2' of 
https://git.linaro.org/people/pmaydell/qemu-arm into staging (2023-05-02 
16:38:29 +0100)

are available in the Git repository at:

  https://gitlab.com/rth7680/qemu.git tags/pull-tcg-20230502-2

for you to fetch changes up to 129f1f9ee7df77d367d961b3c25353612d33cd83:

  tcg: Introduce tcg_out_movext2 (2023-05-02 13:05:45 -0700)


Misc tcg-related patch queue.

v2: Update bitops.h rotate patch.


Dickon Hood (1):
  qemu/bitops.h: Limit rotate amounts

Kiran Ostrolenk (1):
  qemu/host-utils.h: Add clz and ctz functions for lower-bit integers

Nazar Kazakov (2):
  tcg: Add tcg_gen_gvec_andcs
  tcg: Add tcg_gen_gvec_rotrs

Richard Henderson (7):
  softmmu: Tidy dirtylimit_dirty_ring_full_time
  qemu/int128: Re-shuffle Int128Alias members
  migration/xbzrle: Use __attribute__((target)) for avx512
  accel/tcg: Add cpu_ld*_code_mmu
  tcg/loongarch64: Conditionalize tcg_out_exts_i32_i64
  tcg/mips: Conditionalize tcg_out_exts_i32_i64
  tcg: Introduce tcg_out_movext2

Weiwei Li (1):
  accel/tcg: Uncache the host address for instruction fetch when tlb size < 
1

 meson.build  |  5 +--
 accel/tcg/tcg-runtime.h  |  1 +
 include/exec/cpu_ldst.h  |  9 ++
 include/qemu/bitops.h| 16 +-
 include/qemu/host-utils.h| 54 +++
 include/qemu/int128.h|  4 +--
 include/tcg/tcg-op-gvec.h|  4 +++
 accel/tcg/cputlb.c   | 53 ++
 accel/tcg/tcg-runtime-gvec.c | 11 +++
 accel/tcg/user-exec.c| 58 +
 migration/xbzrle.c   |  9 +++---
 softmmu/dirtylimit.c | 15 ++---
 tcg/tcg-op-gvec.c| 28 
 tcg/tcg.c| 69 +---
 tcg/arm/tcg-target.c.inc | 44 +++--
 tcg/i386/tcg-target.c.inc| 19 +--
 tcg/loongarch64/tcg-target.c.inc |  4 ++-
 tcg/mips/tcg-target.c.inc|  4 ++-
 18 files changed, 339 insertions(+), 68 deletions(-)

Re: [PATCH v2] migration: Handle block device inactivation failures better

[Eric Blake]

On Tue, May 02, 2023 at 11:54:12AM +0200, Kevin Wolf wrote:
> Hi Eric,
> 
> you asked me for a review downstream, but since you would have to bring
> back any problem to upstream anyway, let's discuss it here. For the
> start, let me state that (a) I don't fully understand why this patch
> fixes things and (b) I hate this function. More below.

Sadly, I also fall into (a) I don't know if this patch fully fixes
things, or if we're playing whack-a-mole and another bug is still
lurking, and (b) the control flow is indeed horrendous, where I'm also
not sure if I'm fully understanding how migration is supposed to be
working.

> 
> Am 14.04.2023 um 17:33 hat Eric Blake geschrieben:
> > Consider what happens when performing a migration between two host
> > machines connected to an NFS server serving multiple block devices to
> > the guest, when the NFS server becomes unavailable.  The migration
> > attempts to inactivate all block devices on the source (a necessary
> > step before the destination can take over); but if the NFS server is
> > non-responsive, the attempt to inactivate can itself fail.  When that
> > happens, the destination fails to get the migrated guest (good,
> > because the source wasn't able to flush everything properly):
> > 
> >   (qemu) qemu-kvm: load of migration failed: Input/output error
> > 
> > at which point, our only hope for the guest is for the source to take
> > back control.  With the current code base, the host outputs a message, but 
> > then appears to resume:
> > 
> >   (qemu) qemu-kvm: qemu_savevm_state_complete_precopy_non_iterable: 
> > bdrv_inactivate_all() failed (-1)
> > 
> >   (src qemu)info status
> >VM status: running
> > 
> > but a second migration attempt now asserts:
> > 
> >   (src qemu) qemu-kvm: ../block.c:6738: int 
> > bdrv_inactivate_recurse(BlockDriverState *): Assertion `!(bs->open_flags & 
> > BDRV_O_INACTIVE)' failed.
> > 
> > Whether the guest is recoverable on the source after the first failure
> > is debatable, but what we do not want is to have qemu itself fail due
> > to an assertion.  It looks like the problem is as follows:
> > 
> > In migration.c:migration_completion(), the source sets 'inactivate' to
> > true (since COLO is not enabled), then tries
> > savevm.c:qemu_savevm_state_complete_precopy() with a request to
> > inactivate block devices.  In turn, this calls
> > block.c:bdrv_inactivate_all(), which fails when flushing runs up
> > against the non-responsive NFS server.  With savevm failing, we are
> > now left in a state where some, but not all, of the block devices have
> > been inactivated; but migration_completion() then jumps to 'fail'
> > rather than 'fail_invalidate' and skips an attempt to reclaim those
> > those disks by calling bdrv_activate_all().  Even if we do attempt to
> > reclaim disks, we aren't taking note of failure there, either.
> 
> Why do we even jump to 'fail'? In other words, should 'fail_inactivate'
> really be called 'fail' and everything should jump there?
> 
> Greg added the 'fail_inactivate' label in fe904ea8242, but the commit
> message doesn't seem to tell why he left one goto. I see no reason why
> we wouldn't want to reactivate in this case, too. Maybe it's just for
> the colo case?

At the time of fe904ea8, all actions done by fail_invalidate: were
guarded by a mere
 if (s->state == MIGRATION_STATUS_ACTIVE)
Later, in 6039dd5b1c the guard was expanded to
 if (s->state == MIGRATION_STATUS_ACTIVE || s->state == MIGRATION_STATUS_DEVICE)

But reading the rest of the function (at either those prior commit
points, or at the present), I'm inclined to agree that all remaining
'goto fail' either happened at a point where s->state cannot match in
the first place (so consolidating labels is harmless), or is the ONE
place after we already checked s->state == MIGRATION_STATUS_ACTIVE or
just changed s->state to MIGRATION_STATUS_DEVICE, and our failure
could be from one of vm_stop_force_state(RUN_STATE_FINISH_MIGRATE)
(nothing to reactivate, as we haven't inactivated yet),
migration_maybe_pause() (where we change state, but still haven't
inactivated), or from qemu_savevm_state_complete_precopy() (where
'goto fail' does skip out on the reactivation, so consolidating the
labels would make sense to me, although at the time I wrote the patch,
I was too afraid to change the code that drastically).

Are we sure that bdrv_activate_all() is safe to call no matter what?
We already know that a request to inactivate when something is already
inactive asserts, but a request to activate something that is already
active is generally a no-op.  If so, I'm tending to agree with you
that having a single label where we ALWAYS attempt reactivation after
failure makes sense.

I'll post another patch along those lines.

> 
> > Thus, we have reached a state where the migration engine has forgotten
> > all state about whether a block device is inactive, because we did not
> > set s->block_inactive in enough places; so migration allows the 

Re: [PATCH v3 08/19] qemu/bitops.h: Limit rotate amounts

[Richard Henderson]

On 4/28/23 15:47, Lawrence Hunter wrote:

  static inline uint32_t ror32(uint32_t word, unsigned int shift)
  {
-return (word >> shift) | (word << ((32 - shift) & 31));
+shift &= 31;
+return (word >> shift) | (word << (32 - shift));


This is incorrect, because if shift == 0, you are now performing (word << 32).

I agree with your original intent though, to mask and accept any rotation.
I've changed these like so:

-return (word >> shift) | (word << ((32 - shift) & 31));
+return (word >> (shift & 31)) | (word << (-shift & 31));

which also eliminates the useless subtract from word-size-before-mask.


r~

Re: [PATCH v4 10/20] block: drain from main loop thread in bdrv_co_yield_to_drain()

[Stefan Hajnoczi]

On Tue, May 02, 2023 at 06:21:20PM +0200, Kevin Wolf wrote:
> Am 25.04.2023 um 19:27 hat Stefan Hajnoczi geschrieben:
> > For simplicity, always run BlockDevOps .drained_begin/end/poll()
> > callbacks in the main loop thread. This makes it easier to implement the
> > callbacks and avoids extra locks.
> > 
> > Move the function pointer declarations from the I/O Code section to the
> > Global State section in block-backend-common.h.
> > 
> > Signed-off-by: Stefan Hajnoczi 
> 
> If we're updating function pointers, we should probably update them in
> BdrvChildClass and BlockDriver, too.

I'll do that in the next revision.

> This means that a non-coroutine caller can't run in an iothread, not
> even the home iothread of the BlockDriverState. (I'm not sure if it was
> allowed previously. I don't think we're actually doing this, but in
> theory it could have worked.) Maybe put a GLOBAL_STATE_CODE() after
> handling the bdrv_co_yield_to_drain() case? Or would that look too odd?
> 
> IO_OR_GS_CODE();
> 
> if (qemu_in_coroutine()) {
> bdrv_co_yield_to_drain(bs, true, parent, poll);
> return;
> }
> 
> GLOBAL_STATE_CODE();

That looks good to me, it makes explicit that IO_OR_GS_CODE() only
applies until the end of the if statement.

Stefan


signature.asc
Description: PGP signature

Re: [PATCH v4 07/20] block/export: stop using is_external in vhost-user-blk server

[Stefan Hajnoczi]

On Tue, May 02, 2023 at 06:04:24PM +0200, Kevin Wolf wrote:
> Am 25.04.2023 um 19:27 hat Stefan Hajnoczi geschrieben:
> > vhost-user activity must be suspended during bdrv_drained_begin/end().
> > This prevents new requests from interfering with whatever is happening
> > in the drained section.
> > 
> > Previously this was done using aio_set_fd_handler()'s is_external
> > argument. In a multi-queue block layer world the aio_disable_external()
> > API cannot be used since multiple AioContext may be processing I/O, not
> > just one.
> > 
> > Switch to BlockDevOps->drained_begin/end() callbacks.
> > 
> > Signed-off-by: Stefan Hajnoczi 
> > ---
> >  block/export/vhost-user-blk-server.c | 43 ++--
> >  util/vhost-user-server.c | 10 +++
> >  2 files changed, 26 insertions(+), 27 deletions(-)
> > 
> > diff --git a/block/export/vhost-user-blk-server.c 
> > b/block/export/vhost-user-blk-server.c
> > index 092b86aae4..d20f69cd74 100644
> > --- a/block/export/vhost-user-blk-server.c
> > +++ b/block/export/vhost-user-blk-server.c
> > @@ -208,22 +208,6 @@ static const VuDevIface vu_blk_iface = {
> >  .process_msg   = vu_blk_process_msg,
> >  };
> >  
> > -static void blk_aio_attached(AioContext *ctx, void *opaque)
> > -{
> > -VuBlkExport *vexp = opaque;
> > -
> > -vexp->export.ctx = ctx;
> > -vhost_user_server_attach_aio_context(>vu_server, ctx);
> > -}
> > -
> > -static void blk_aio_detach(void *opaque)
> > -{
> > -VuBlkExport *vexp = opaque;
> > -
> > -vhost_user_server_detach_aio_context(>vu_server);
> > -vexp->export.ctx = NULL;
> > -}
> 
> So for changing the AioContext, we now rely on the fact that the node to
> be changed is always drained, so the drain callbacks implicitly cover
> this case, too?

Yes.

> >  static void
> >  vu_blk_initialize_config(BlockDriverState *bs,
> >   struct virtio_blk_config *config,
> > @@ -272,6 +256,25 @@ static void vu_blk_exp_resize(void *opaque)
> >  vu_config_change_msg(>vu_server.vu_dev);
> >  }
> >  
> > +/* Called with vexp->export.ctx acquired */
> > +static void vu_blk_drained_begin(void *opaque)
> > +{
> > +VuBlkExport *vexp = opaque;
> > +
> > +vhost_user_server_detach_aio_context(>vu_server);
> > +}
> 
> Compared to the old code, we're losing the vexp->export.ctx = NULL. This
> is correct at this point because after drained_begin we still keep
> processing requests until we arrive at a quiescent state.
> 
> However, if we detach the AioContext because we're deleting the
> iothread, won't we end up with a dangling pointer in vexp->export.ctx?
> Or can we be certain that nothing interesting happens before drained_end
> updates it with a new valid pointer again?

If you want I can add the detach() callback back again and set ctx to
NULL there?

Stefan


signature.asc
Description: PGP signature

Re: [PATCH v4 04/20] virtio-scsi: stop using aio_disable_external() during unplug

[Stefan Hajnoczi]

On Tue, May 02, 2023 at 03:19:52PM +0200, Kevin Wolf wrote:
> Am 01.05.2023 um 17:09 hat Stefan Hajnoczi geschrieben:
> > On Fri, Apr 28, 2023 at 04:22:55PM +0200, Kevin Wolf wrote:
> > > Am 25.04.2023 um 19:27 hat Stefan Hajnoczi geschrieben:
> > > > This patch is part of an effort to remove the aio_disable_external()
> > > > API because it does not fit in a multi-queue block layer world where
> > > > many AioContexts may be submitting requests to the same disk.
> > > > 
> > > > The SCSI emulation code is already in good shape to stop using
> > > > aio_disable_external(). It was only used by commit 9c5aad84da1c
> > > > ("virtio-scsi: fixed virtio_scsi_ctx_check failed when detaching scsi
> > > > disk") to ensure that virtio_scsi_hotunplug() works while the guest
> > > > driver is submitting I/O.
> > > > 
> > > > Ensure virtio_scsi_hotunplug() is safe as follows:
> > > > 
> > > > 1. qdev_simple_device_unplug_cb() -> qdev_unrealize() ->
> > > >device_set_realized() calls qatomic_set(>realized, false) so
> > > >that future scsi_device_get() calls return NULL because they exclude
> > > >SCSIDevices with realized=false.
> > > > 
> > > >That means virtio-scsi will reject new I/O requests to this
> > > >SCSIDevice with VIRTIO_SCSI_S_BAD_TARGET even while
> > > >virtio_scsi_hotunplug() is still executing. We are protected against
> > > >new requests!
> > > > 
> > > > 2. Add a call to scsi_device_purge_requests() from scsi_unrealize() so
> > > >that in-flight requests are cancelled synchronously. This ensures
> > > >that no in-flight requests remain once qdev_simple_device_unplug_cb()
> > > >returns.
> > > > 
> > > > Thanks to these two conditions we don't need aio_disable_external()
> > > > anymore.
> > > > 
> > > > Cc: Zhengui Li 
> > > > Reviewed-by: Paolo Bonzini 
> > > > Reviewed-by: Daniil Tatianin 
> > > > Signed-off-by: Stefan Hajnoczi 
> > > 
> > > qemu-iotests 040 starts failing for me after this patch, with what looks
> > > like a use-after-free error of some kind.
> > > 
> > > (gdb) bt
> > > #0  0x55b6e3e1f31c in job_type (job=0xe3e3e3e3e3e3e3e3) at 
> > > ../job.c:238
> > > #1  0x55b6e3e1cee5 in is_block_job (job=0xe3e3e3e3e3e3e3e3) at 
> > > ../blockjob.c:41
> > > #2  0x55b6e3e1ce7d in block_job_next_locked (bjob=0x55b6e72b7570) at 
> > > ../blockjob.c:54
> > > #3  0x55b6e3df6370 in blockdev_mark_auto_del (blk=0x55b6e74af0a0) at 
> > > ../blockdev.c:157
> > > #4  0x55b6e393e23b in scsi_qdev_unrealize (qdev=0x55b6e7c04d40) at 
> > > ../hw/scsi/scsi-bus.c:303
> > > #5  0x55b6e3db0d0e in device_set_realized (obj=0x55b6e7c04d40, 
> > > value=false, errp=0x55b6e497c918 ) at ../hw/core/qdev.c:599
> > > #6  0x55b6e3dba36e in property_set_bool (obj=0x55b6e7c04d40, 
> > > v=0x55b6e7d7f290, name=0x55b6e41bd6d8 "realized", opaque=0x55b6e7246d20, 
> > > errp=0x55b6e497c918 )
> > > at ../qom/object.c:2285
> > > #7  0x55b6e3db7e65 in object_property_set (obj=0x55b6e7c04d40, 
> > > name=0x55b6e41bd6d8 "realized", v=0x55b6e7d7f290, errp=0x55b6e497c918 
> > > ) at ../qom/object.c:1420
> > > #8  0x55b6e3dbd84a in object_property_set_qobject 
> > > (obj=0x55b6e7c04d40, name=0x55b6e41bd6d8 "realized", 
> > > value=0x55b6e74c1890, errp=0x55b6e497c918 )
> > > at ../qom/qom-qobject.c:28
> > > #9  0x55b6e3db8570 in object_property_set_bool (obj=0x55b6e7c04d40, 
> > > name=0x55b6e41bd6d8 "realized", value=false, errp=0x55b6e497c918 
> > > ) at ../qom/object.c:1489
> > > #10 0x55b6e3daf2b5 in qdev_unrealize (dev=0x55b6e7c04d40) at 
> > > ../hw/core/qdev.c:306
> > > #11 0x55b6e3db509d in qdev_simple_device_unplug_cb 
> > > (hotplug_dev=0x55b6e81c3630, dev=0x55b6e7c04d40, errp=0x7ffec5519200) at 
> > > ../hw/core/qdev-hotplug.c:72
> > > #12 0x55b6e3c520f9 in virtio_scsi_hotunplug 
> > > (hotplug_dev=0x55b6e81c3630, dev=0x55b6e7c04d40, errp=0x7ffec5519200) at 
> > > ../hw/scsi/virtio-scsi.c:1065
> > > #13 0x55b6e3db4dec in hotplug_handler_unplug 
> > > (plug_handler=0x55b6e81c3630, plugged_dev=0x55b6e7c04d40, 
> > > errp=0x7ffec5519200) at ../hw/core/hotplug.c:56
> > > #14 0x55b6e3a28f84 in qdev_unplug (dev=0x55b6e7c04d40, 
> > > errp=0x7ffec55192e0) at ../softmmu/qdev-monitor.c:935
> > > #15 0x55b6e3a290fa in qmp_device_del (id=0x55b6e74c1760 "scsi0", 
> > > errp=0x7ffec55192e0) at ../softmmu/qdev-monitor.c:955
> > > #16 0x55b6e3fb0a5f in qmp_marshal_device_del (args=0x7f61cc005eb0, 
> > > ret=0x7f61d5a8ae38, errp=0x7f61d5a8ae40) at qapi/qapi-commands-qdev.c:114
> > > #17 0x55b6e3fd52e1 in do_qmp_dispatch_bh (opaque=0x7f61d5a8ae08) at 
> > > ../qapi/qmp-dispatch.c:128
> > > #18 0x55b6e4007b9e in aio_bh_call (bh=0x55b6e7dea730) at 
> > > ../util/async.c:155
> > > #19 0x55b6e4007d2e in aio_bh_poll (ctx=0x55b6e72447c0) at 
> > > ../util/async.c:184
> > > #20 0x55b6e3fe3b45 in aio_dispatch (ctx=0x55b6e72447c0) at 
> > > ../util/aio-posix.c:421
> > > #21 0x55b6e4009544 in aio_ctx_dispatch 

Re: [PATCH v3 01/11] hw: arm: Add bananapi M2-Ultra and allwinner-r40 support

[Niek Linnenbank]

Hi Qianfan,

Sorry for my late response, I had a holiday in between.

On Tue, Apr 18, 2023 at 1:21 PM  wrote:

> From: qianfan Zhao 
>
> Allwinner R40 (sun8i) SoC features a Quad-Core Cortex-A7 ARM CPU,
> and a Mali400 MP2 GPU from ARM. It's also known as the Allwinner T3
> for In-Car Entertainment usage, A40i and A40pro are variants that
> differ in applicable temperatures range (industrial and military).
>
> Signed-off-by: qianfan Zhao 
> ---
>  configs/devices/arm-softmmu/default.mak |   1 +
>  hw/arm/Kconfig  |   9 +
>  hw/arm/allwinner-r40.c  | 418 
>  hw/arm/bananapi_m2u.c   | 129 
>  hw/arm/meson.build  |   1 +
>  include/hw/arm/allwinner-r40.h  | 110 +++
>  6 files changed, 668 insertions(+)
>  create mode 100644 hw/arm/allwinner-r40.c
>  create mode 100644 hw/arm/bananapi_m2u.c
>  create mode 100644 include/hw/arm/allwinner-r40.h
>
> diff --git a/configs/devices/arm-softmmu/default.mak
> b/configs/devices/arm-softmmu/default.mak
> index 1b49a7830c..76a43add23 100644
> --- a/configs/devices/arm-softmmu/default.mak
> +++ b/configs/devices/arm-softmmu/default.mak
> @@ -43,3 +43,4 @@ CONFIG_FSL_IMX6UL=y
>  CONFIG_SEMIHOSTING=y
>  CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
>  CONFIG_ALLWINNER_H3=y
> +CONFIG_ALLWINNER_R40=y
>

Looks like on latest master (c586691) this file has already been modified -
the patch doesn't apply anymore.

Could you please rebase your patch series in V4 to make them compatible
again?
After your rebase is done, I'll try to complete reviewing the remaining
patches.


> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index b5aed4aff5..9e14c3427e 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -344,6 +344,15 @@ config ALLWINNER_H3
>  select USB_EHCI_SYSBUS
>  select SD
>
> +config ALLWINNER_R40
> +bool
> +select ALLWINNER_A10_PIT
> +select SERIAL
> +select ARM_TIMER
> +select ARM_GIC
> +select UNIMP
> +select SD
> +
>  config RASPI
>  bool
>  select FRAMEBUFFER
> diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
> new file mode 100644
> index 00..b743d64253
> --- /dev/null
> +++ b/hw/arm/allwinner-r40.c
> @@ -0,0 +1,418 @@
> +/*
> + * Allwinner R40/A40i/T3 System on Chip emulation
> + *
> + * Copyright (C) 2023 qianfan Zhao 
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/error-report.h"
> +#include "qemu/bswap.h"
> +#include "qemu/module.h"
> +#include "qemu/units.h"
> +#include "hw/qdev-core.h"
> +#include "hw/sysbus.h"
> +#include "hw/char/serial.h"
> +#include "hw/misc/unimp.h"
> +#include "hw/usb/hcd-ehci.h"
> +#include "hw/loader.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/arm/allwinner-r40.h"
> +
> +/* Memory map */
> +const hwaddr allwinner_r40_memmap[] = {
> +[AW_R40_DEV_SRAM_A1]= 0x,
> +[AW_R40_DEV_SRAM_A2]= 0x4000,
> +[AW_R40_DEV_SRAM_A3]= 0x8000,
> +[AW_R40_DEV_SRAM_A4]= 0xb400,
> +[AW_R40_DEV_MMC0]   = 0x01c0f000,
> +[AW_R40_DEV_MMC1]   = 0x01c1,
> +[AW_R40_DEV_MMC2]   = 0x01c11000,
> +[AW_R40_DEV_MMC3]   = 0x01c12000,
> +[AW_R40_DEV_PIT]= 0x01c20c00,
> +[AW_R40_DEV_UART0]  = 0x01c28000,
> +[AW_R40_DEV_GIC_DIST]   = 0x01c81000,
> +[AW_R40_DEV_GIC_CPU]= 0x01c82000,
> +[AW_R40_DEV_GIC_HYP]= 0x01c84000,
> +[AW_R40_DEV_GIC_VCPU]   = 0x01c86000,
> +[AW_R40_DEV_SDRAM]  = 0x4000
> +};
> +
> +/* List of unimplemented devices */
> +struct AwR40Unimplemented {
> +const char *device_name;
> +hwaddr base;
> +hwaddr size;
> +};
> +
> +static struct AwR40Unimplemented r40_unimplemented[] = {
> +{ "d-engine",   0x0100, 4 * MiB },
> +{ "d-inter",0x0140, 128 * KiB },
> +{ "sram-c", 0x01c0, 4 * KiB },
> +{ "dma",0x01c02000, 4 * KiB },
> +{ "nfdc",   0x01c03000, 4 * KiB },
> +{ "ts", 0x01c04000, 4 * KiB },
> +{ "spi0",   0x01c05000, 4 * KiB },
> +{ "spi1",   0x01c06000, 4 * KiB },
> +{ "cs0",0x01c09000, 4 * KiB },
> +{ "keymem", 0x01c0a000, 4 * KiB },
> +{ "emac",   0x01c0b000, 4 * KiB },
> +{ "usb0-otg",   0x01c13000, 4 * 

Re: [PATCH] softfloat: Fix the incorrect computation in float32_exp2()

[Richard Henderson]

On 5/2/23 16:25, Shivaprasad G Bhat wrote:

The float32_exp2() is computing wrong exponent of 2.
For example, with the following set of values {0.1, 2.0, 2.0, -1.0},
the expected output would be {1.071773, 4.00, 4.00, 0.50}.
Instead, the function is computing {1.119102, 3.382044, 3.382044, -0.191022}

Looking at the code, the float32_exp2() attempts to do this

   2 3 4 5   n
   xx x x x x   x
  e  = 1 + --- + --- + --- + --- + --- + ... + --- + ...
1!2!3!4!5!  n!

But because of the 'typo'/bug it ends up doing

  xx x x x x   x
e  = 1 + --- + --- + --- + --- + --- + ... + --- + ...
   1!2!3!4!5!  n!

This is because instead of the xnp which holds the numerator,
parts_muladd is using the xp which is just 'x'. The commit '572c4d862ff2'
refactored this function, and it seems mistakenly using xp instead of xnp.

The patches fixes this possible typo.

Fixes: 572c4d862ff2 "softfloat: Convert float32_exp2 to FloatParts"
Partially-Resolves:https://gitlab.com/qemu-project/qemu/-/issues/1623
Reported-By: Luca Barbato (https://gitlab.com/lu-zero)
Signed-off-by: Shivaprasad G Bhat
Signed-off-by: Vaibhav Jain
---
  fpu/softfloat.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Whoops.  Good catch.

r~

Re: [PATCH v4 06/20] block/export: wait for vhost-user-blk requests when draining

[Stefan Hajnoczi]

On Tue, May 02, 2023 at 05:42:51PM +0200, Kevin Wolf wrote:
> Am 25.04.2023 um 19:27 hat Stefan Hajnoczi geschrieben:
> > Each vhost-user-blk request runs in a coroutine. When the BlockBackend
> > enters a drained section we need to enter a quiescent state. Currently
> > any in-flight requests race with bdrv_drained_begin() because it is
> > unaware of vhost-user-blk requests.
> > 
> > When blk_co_preadv/pwritev()/etc returns it wakes the
> > bdrv_drained_begin() thread but vhost-user-blk request processing has
> > not yet finished. The request coroutine continues executing while the
> > main loop thread thinks it is in a drained section.
> > 
> > One example where this is unsafe is for blk_set_aio_context() where
> > bdrv_drained_begin() is called before .aio_context_detached() and
> > .aio_context_attach(). If request coroutines are still running after
> > bdrv_drained_begin(), then the AioContext could change underneath them
> > and they race with new requests processed in the new AioContext. This
> > could lead to virtqueue corruption, for example.
> > 
> > (This example is theoretical, I came across this while reading the
> > code and have not tried to reproduce it.)
> > 
> > It's easy to make bdrv_drained_begin() wait for in-flight requests: add
> > a .drained_poll() callback that checks the VuServer's in-flight counter.
> > VuServer just needs an API that returns true when there are requests in
> > flight. The in-flight counter needs to be atomic.
> > 
> > Signed-off-by: Stefan Hajnoczi 
> > ---
> >  include/qemu/vhost-user-server.h |  4 +++-
> >  block/export/vhost-user-blk-server.c | 16 
> >  util/vhost-user-server.c | 14 ++
> >  3 files changed, 29 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/qemu/vhost-user-server.h 
> > b/include/qemu/vhost-user-server.h
> > index bc0ac9ddb6..b1c1cda886 100644
> > --- a/include/qemu/vhost-user-server.h
> > +++ b/include/qemu/vhost-user-server.h
> > @@ -40,8 +40,9 @@ typedef struct {
> >  int max_queues;
> >  const VuDevIface *vu_iface;
> >  
> > +unsigned int in_flight; /* atomic */
> > +
> >  /* Protected by ctx lock */
> > -unsigned int in_flight;
> >  bool wait_idle;
> >  VuDev vu_dev;
> >  QIOChannel *ioc; /* The I/O channel with the client */
> > @@ -62,6 +63,7 @@ void vhost_user_server_stop(VuServer *server);
> >  
> >  void vhost_user_server_inc_in_flight(VuServer *server);
> >  void vhost_user_server_dec_in_flight(VuServer *server);
> > +bool vhost_user_server_has_in_flight(VuServer *server);
> >  
> >  void vhost_user_server_attach_aio_context(VuServer *server, AioContext 
> > *ctx);
> >  void vhost_user_server_detach_aio_context(VuServer *server);
> > diff --git a/block/export/vhost-user-blk-server.c 
> > b/block/export/vhost-user-blk-server.c
> > index 841acb36e3..092b86aae4 100644
> > --- a/block/export/vhost-user-blk-server.c
> > +++ b/block/export/vhost-user-blk-server.c
> > @@ -272,7 +272,20 @@ static void vu_blk_exp_resize(void *opaque)
> >  vu_config_change_msg(>vu_server.vu_dev);
> >  }
> >  
> > +/*
> > + * Ensures that bdrv_drained_begin() waits until in-flight requests 
> > complete.
> > + *
> > + * Called with vexp->export.ctx acquired.
> > + */
> > +static bool vu_blk_drained_poll(void *opaque)
> > +{
> > +VuBlkExport *vexp = opaque;
> > +
> > +return vhost_user_server_has_in_flight(>vu_server);
> > +}
> > +
> >  static const BlockDevOps vu_blk_dev_ops = {
> > +.drained_poll  = vu_blk_drained_poll,
> >  .resize_cb = vu_blk_exp_resize,
> >  };
> 
> You're adding a new function pointer to an existing BlockDevOps...
> 
> > @@ -314,6 +327,7 @@ static int vu_blk_exp_create(BlockExport *exp, 
> > BlockExportOptions *opts,
> >  vu_blk_initialize_config(blk_bs(exp->blk), >blkcfg,
> >   logical_block_size, num_queues);
> >  
> > +blk_set_dev_ops(exp->blk, _blk_dev_ops, vexp);
> >  blk_add_aio_context_notifier(exp->blk, blk_aio_attached, 
> > blk_aio_detach,
> >   vexp);
> >  
> >  blk_set_dev_ops(exp->blk, _blk_dev_ops, vexp);
> 
> ..but still add a second blk_set_dev_ops(). Maybe a bad merge conflict
> resolution with commit ca858a5fe94?

Thanks, I probably didn't have ca858a5fe94 in my tree when writing this
code.

> > @@ -323,6 +337,7 @@ static int vu_blk_exp_create(BlockExport *exp, 
> > BlockExportOptions *opts,
> >   num_queues, _blk_iface, errp)) {
> >  blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
> >  blk_aio_detach, vexp);
> > +blk_set_dev_ops(exp->blk, NULL, NULL);
> >  g_free(vexp->handler.serial);
> >  return -EADDRNOTAVAIL;
> >  }
> > @@ -336,6 +351,7 @@ static void vu_blk_exp_delete(BlockExport *exp)
> >  
> >  blk_remove_aio_context_notifier(exp->blk, blk_aio_attached, 
> > blk_aio_detach,
> >

Re: [PATCH] tests/avocado/virtio-gpu: Fix the URLs of the test_virtio_vga_virgl test

[Marc-André Lureau]

On Tue, May 2, 2023 at 2:57 PM Thomas Huth  wrote:

> The URLs here are not valid anymore - looks like the assets got moved
> into the pub/archive/ subfolder instead.
>
> Signed-off-by: Thomas Huth 
>

Reviewed-by: Marc-André Lureau 


> ---
>  tests/avocado/virtio-gpu.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/avocado/virtio-gpu.py b/tests/avocado/virtio-gpu.py
> index 2a249a3a2c..e3b58fe799 100644
> --- a/tests/avocado/virtio-gpu.py
> +++ b/tests/avocado/virtio-gpu.py
> @@ -36,13 +36,13 @@ class VirtioGPUx86(QemuSystemTest):
>
>  KERNEL_COMMAND_LINE = "printk.time=0 console=ttyS0 rdinit=/bin/bash"
>  KERNEL_URL = (
> -"https://archives.fedoraproject.org/pub/fedora;
> +"https://archives.fedoraproject.org/pub/archive/fedora;
>  "/linux/releases/33/Everything/x86_64/os/images"
>  "/pxeboot/vmlinuz"
>  )
>  KERNEL_HASH = '1433cfe3f2ffaa44de4ecfb57ec25dc2399cdecf'
>  INITRD_URL = (
> -"https://archives.fedoraproject.org/pub/fedora;
> +"https://archives.fedoraproject.org/pub/archive/fedora;
>  "/linux/releases/33/Everything/x86_64/os/images"
>  "/pxeboot/initrd.img"
>  )
> --
> 2.31.1
>
>

Re: [PATCH] ui: Fix pixel colour channel order for PNG screenshots

[Marc-André Lureau]

On Tue, May 2, 2023 at 5:56 PM Peter Maydell 
wrote:

> When we take a PNG screenshot the ordering of the colour channels in
> the data is not correct, resulting in the image having weird
> colouring compared to the actual display.  (Specifically, on a
> little-endian host the blue and red channels are swapped; on
> big-endian everything is wrong.)
>
> This happens because the pixman idea of the pixel data and the libpng
> idea differ.  PIXMAN_a9r8g8b8 defines that pixels are 32-bit values,
> with A in bits 24-31, R in bits 16-23, G in bits 8-15 and B in bits
> 0-7.  This means that on little-endian systems the bytes in memory
> are
>B G R A
> and on big-endian systems they are
>A R G B
>
> libpng, on the other hand, thinks of pixels as being a series of
> values for each channel, so its format PNG_COLOR_TYPE_RGB_ALPHA
> always wants bytes in the order
>R G B A
>
> This isn't the same as the pixman order for either big or little
> endian hosts.
>
> The alpha channel is also unnecessary bulk in the output PNG file,
> because there is no alpha information in a screenshot.
>
> To handle the endianness issue, we already define in ui/qemu-pixman.h
> various PIXMAN_BE_* and PIXMAN_LE_* values that give consistent
> byte-order pixel channel formats.  So we can use PIXMAN_BE_r8g8b8 and
> PNG_COLOR_TYPE_RGB, which both have an in-memory byte order of
> R G B
> and 3 bytes per pixel.
>
> (PPM format screenshots get this right; they already use the
> PIXMAN_BE_r8g8b8 format.)
>
> Cc: qemu-sta...@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1622
> Fixes: 9a0a119a382867 ("Added parameter to take screenshot with screendump
> as PNG")
> Signed-off-by: Peter Maydell 
>

Reviewed-by: Marc-André Lureau 


> ---
> Disclaimer: I don't have a BE system that I have convenient
> graphics output from that I can use to test the screenshot
> format there.
> ---
>  ui/console.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ui/console.c b/ui/console.c
> index 6e8a3cdc62d..e173731e205 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -311,7 +311,7 @@ static bool png_save(int fd, pixman_image_t *image,
> Error **errp)
>  png_struct *png_ptr;
>  png_info *info_ptr;
>  g_autoptr(pixman_image_t) linebuf =
> -qemu_pixman_linebuf_create(PIXMAN_a8r8g8b8,
> width);
> +qemu_pixman_linebuf_create(PIXMAN_BE_r8g8b8, width);
>  uint8_t *buf = (uint8_t *)pixman_image_get_data(linebuf);
>  FILE *f = fdopen(fd, "wb");
>  int y;
> @@ -341,7 +341,7 @@ static bool png_save(int fd, pixman_image_t *image,
> Error **errp)
>  png_init_io(png_ptr, f);
>
>  png_set_IHDR(png_ptr, info_ptr, width, height, 8,
> - PNG_COLOR_TYPE_RGB_ALPHA, PNG_INTERLACE_NONE,
> + PNG_COLOR_TYPE_RGB, PNG_INTERLACE_NONE,
>   PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE);
>
>  png_write_info(png_ptr, info_ptr);
> --
> 2.34.1
>
>

Re: [PATCH v3 00/57] tcg: Improve atomicity support

[Richard Henderson]

On 5/2/23 17:11, Peter Maydell wrote:

On Tue, 25 Apr 2023 at 20:32, Richard Henderson
 wrote:


v1: 
https://lore.kernel.org/qemu-devel/20221118094754.242910-1-richard.hender...@linaro.org/
v2: 
https://lore.kernel.org/qemu-devel/20230216025739.1211680-1-richard.hender...@linaro.org/

Based-on: 20230424054105.1579315-1-richard.hender...@linaro.org
("[PATCH v3 00/57] tcg: Simplify calls to load/store helpers")

The main objective here is to support Arm FEAT_LSE2, which says that any
single memory access that does not cross a 16-byte boundary is atomic.
This is the MO_ATOM_WITHIN16 control.

While I'm touching all of this, a secondary objective is to handle the
atomicity of the IBM machines.  Both Power and s390x treat misaligned
accesses as atomic on the lsb of the pointer.  For instance, an 8-byte
access at ptr % 8 == 4 will appear as two atomic 4-byte accesses, and
ptr % 4 == 2 will appear as four 3-byte accesses.


I hope you mean "four 2-byte accesses", although I'm never
quite sure how odd s390x can get :-)


Heh.  Yes, of course.


r~

Re: [QEMU][PATCH v4 2/4] hw/net/can: Introduce Xilinx Versal CANFD controller

[Francisco Iglesias]

Hi Vikram,

A few comments below and some suggestions!


On [2023 Apr 24] Mon 23:34:31, Vikram Garhwal wrote:
> The Xilinx Versal CANFD controller is developed based on SocketCAN, QEMU CAN 
> bus
> implementation. Bus connection and socketCAN connection for each CAN module
> can be set through command lines.
> 
> Signed-off-by: Vikram Garhwal 
> ---
>  hw/net/can/meson.build |1 +
>  hw/net/can/trace-events|7 +
>  hw/net/can/xlnx-versal-canfd.c | 2115 
>  include/hw/net/xlnx-versal-canfd.h |   87 ++
>  4 files changed, 2210 insertions(+)
>  create mode 100644 hw/net/can/xlnx-versal-canfd.c
>  create mode 100644 include/hw/net/xlnx-versal-canfd.h
> 
> diff --git a/hw/net/can/meson.build b/hw/net/can/meson.build
> index 8fabbd9ee6..8d85201cb0 100644
> --- a/hw/net/can/meson.build
> +++ b/hw/net/can/meson.build
> @@ -5,3 +5,4 @@ softmmu_ss.add(when: 'CONFIG_CAN_PCI', if_true: 
> files('can_mioe3680_pci.c'))
>  softmmu_ss.add(when: 'CONFIG_CAN_CTUCANFD', if_true: files('ctucan_core.c'))
>  softmmu_ss.add(when: 'CONFIG_CAN_CTUCANFD_PCI', if_true: 
> files('ctucan_pci.c'))
>  softmmu_ss.add(when: 'CONFIG_XLNX_ZYNQMP', if_true: 
> files('xlnx-zynqmp-can.c'))
> +softmmu_ss.add(when: 'CONFIG_XLNX_VERSAL', if_true: 
> files('xlnx-versal-canfd.c'))
> diff --git a/hw/net/can/trace-events b/hw/net/can/trace-events
> index 8346a98ab5..de64ac1b31 100644
> --- a/hw/net/can/trace-events
> +++ b/hw/net/can/trace-events
> @@ -7,3 +7,10 @@ xlnx_can_filter_mask_pre_write(uint8_t filter_num, uint32_t 
> value) "Filter%d MAS
>  xlnx_can_tx_data(uint32_t id, uint8_t dlc, uint8_t db0, uint8_t db1, uint8_t 
> db2, uint8_t db3, uint8_t db4, uint8_t db5, uint8_t db6, uint8_t db7) "Frame: 
> ID: 0x%08x DLC: 0x%02x DATA: 0x%02x 0x%02x 0x%02x 0x%02x 0x%02x 0x%02x 0x%02x 
> 0x%02x"
>  xlnx_can_rx_data(uint32_t id, uint32_t dlc, uint8_t db0, uint8_t db1, 
> uint8_t db2, uint8_t db3, uint8_t db4, uint8_t db5, uint8_t db6, uint8_t db7) 
> "Frame: ID: 0x%08x DLC: 0x%02x DATA: 0x%02x 0x%02x 0x%02x 0x%02x 0x%02x 
> 0x%02x 0x%02x 0x%02x"
>  xlnx_can_rx_discard(uint32_t status) "Controller is not enabled for bus 
> communication. Status Register: 0x%08x"
> +
> +# xlnx-versal-canfd.c
> +xlnx_canfd_update_irq(char *path, uint32_t isr, uint32_t ier, uint32_t irq) 
> "%s: ISR: 0x%08x IER: 0x%08x IRQ: 0x%08x"
> +xlnx_canfd_rx_fifo_filter_reject(char *path, uint32_t id, uint8_t dlc) "%s: 
> Frame: ID: 0x%08x DLC: 0x%02x"
> +xlnx_canfd_rx_data(char *path, uint32_t id, uint8_t dlc, uint8_t flags) "%s: 
> Frame: ID: 0x%08x DLC: 0x%02x CANFD Flag: 0x%02x"
> +xlnx_canfd_tx_data(char *path, uint32_t id, uint8_t dlc, uint8_t flgas) "%s: 
> Frame: ID: 0x%08x DLC: 0x%02x CANFD Flag: 0x%02x"
> +xlnx_canfd_reset(char *path, uint32_t val) "%s: Resetting controller with 
> value = 0x%08x"
> diff --git a/hw/net/can/xlnx-versal-canfd.c b/hw/net/can/xlnx-versal-canfd.c
> new file mode 100644
> index 00..fb6fa54698
> --- /dev/null
> +++ b/hw/net/can/xlnx-versal-canfd.c
> @@ -0,0 +1,2115 @@
> +/*
> + * QEMU model of the Xilinx Versal CANFD device.
> + *
> + * This implementation is based on the following datasheet:
> + * https://docs.xilinx.com/v/u/2.0-English/pg223-canfd
> + *
> + * Copyright (c) 2022 AMD Inc.
> + *
> + * Written-by: Vikram Garhwal 
> + *
> + * Based on QEMU CANFD Device emulation implemented by Jin Yang, Deniz Eren 
> and
> + * Pavel Pisa
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/sysbus.h"
> +#include "hw/irq.h"
> +#include "hw/register.h"
> +#include "qapi/error.h"
> +#include "qemu/bitops.h"
> +#include "qemu/log.h"
> +#include "qemu/cutils.h"
> +#include "qemu/event_notifier.h"
> +#include "hw/qdev-properties.h"
> +#include "qom/object_interfaces.h"
> +#include "migration/vmstate.h"
> +#include "hw/net/xlnx-versal-canfd.h"
> +#include "trace.h"
> 

RE: [PATCH 3/9] target/Hexagon: Finish conversion to tcg_gen_qemu_{ld,st}_*

[Taylor Simpson]

> -Original Message-
> From: Richard Henderson 
> Sent: Tuesday, May 2, 2023 10:49 AM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Subject: Re: [PATCH 3/9] target/Hexagon: Finish conversion to
> tcg_gen_qemu_{ld,st}_*
> 
> On 5/2/23 16:27, Taylor Simpson wrote:
> >
> >
> >> -Original Message-
> >> From: Richard Henderson 
> >> Sent: Tuesday, May 2, 2023 8:58 AM
> >> To: qemu-devel@nongnu.org
> >> Cc: mrol...@gmail.com; edgar.igles...@gmail.com; Taylor Simpson
> >> ; a...@rev.ng; a...@rev.ng; laur...@vivier.eu;
> >> phi...@linaro.org; jiaxun.y...@flygoat.com; da...@redhat.com;
> >> i...@linux.ibm.com; th...@redhat.com; mark.cave-ayl...@ilande.co.uk;
> >> atar4q...@gmail.com; jcmvb...@gmail.com
> >> Subject: [PATCH 3/9] target/Hexagon: Finish conversion to
> >> tcg_gen_qemu_{ld,st}_*
> >>
> >> Convert away from the old interface with the implicit MemOp argument.
> >> Importantly, this removes some incorrect casts generated by
> >> idef-parser's gen_load().
> >>
> >> Signed-off-by: Richard Henderson 
> >> ---
> >>   target/hexagon/macros.h | 14 -
> >>   target/hexagon/genptr.c |  8 +++---
> >>   target/hexagon/idef-parser/parser-helpers.c | 28 +-
> >>   target/hexagon/translate.c  | 32 ++---
> >>   4 files changed, 40 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index
> >> 502c85ae35..244063b1d2 100644
> >> --- a/target/hexagon/genptr.c
> >> +++ b/target/hexagon/genptr.c
> >> @@ -320,14 +320,14 @@ void gen_set_byte_i64(int N, TCGv_i64 result,
> >> TCGv
> >> src)
> >>
> >>   static void gen_return(DisasContext *ctx, TCGv_i64 dst, TCGv src)
> >> @@ -
> >> 1019,7 +1019,7 @@ static void gen_vreg_load(DisasContext *ctx,
> >> intptr_t dstoff, TCGv src,
> >>   tcg_gen_andi_tl(src, src, ~((int32_t)sizeof(MMVector) - 1));
> >>   }
> >>   for (int i = 0; i < sizeof(MMVector) / 8; i++) {
> >> -tcg_gen_qemu_ld64(tmp, src, ctx->mem_idx);
> >> +tcg_gen_qemu_ld_i64(tmp, src, ctx->mem_idx, MO_TEUQ);
> >>   tcg_gen_addi_tl(src, src, 8);
> >>   tcg_gen_st_i64(tmp, cpu_env, dstoff + i * 8);
> >
> > Did you intend to leave the tcg_gen_st_i64 alone or should that be
> converted to tcg_gen_qemu_st64.
> >
> > There's a tcg_gen_st8_i64 in vec_to_qvec.  Does that need to be
> converted?
> 
> No, those are host stores to env, not guest stores to guest memory.
> Notice the lack of "qemu" in the function names.
> 
> > I'm curious if there's a better way to do a vector load (e.g., with
> tcg_gen_gvec_) than a loop that does 8 bytes at a time.
> 
> The best you can do at the moment is tcg_gen_qemu_ld_i128.
> But there's no gvec variant to load arbitrary vector lengths.

OK, thanks.

Also,
Tested-by: Taylor Simpson 

Re: [PATCH] block/blkio: add 'fd' option to virtio-blk-vhost-vdpa driver

[Stefan Hajnoczi]

On Tue, May 02, 2023 at 04:50:50PM +0200, Stefano Garzarella wrote:
> The virtio-blk-vhost-vdpa driver in libblkio 1.3.0 supports the new
> 'fd' property. Let's expose this to the user, so the management layer
> can pass the file descriptor of an already opened vhost-vdpa character
> device. This is useful especially when the device can only be accessed
> with certain privileges.
> 
> Signed-off-by: Stefano Garzarella 
> ---
> 
> Notes:
> As an alternative we could support passing `/dev/fdset/N` via 'path',
> always opening the path with qemu_open() and passing the fd to the
> libblkio driver.
> I preferred to add a new parameter though, because the code is
> simpler without changing how path works (alternatively we should check
> first if fd is supported by the driver or not).
> 
> What do you think?

I think the approach in this patch is fine.

> 
> Thanks,
> Stefano
> 
>  qapi/block-core.json |  6 +-
>  block/blkio.c| 45 +++-
>  2 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b57978957f..9f70777d49 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3841,10 +3841,14 @@
>  #
>  # @path: path to the vhost-vdpa character device.
>  #
> +# @fd: file descriptor of an already opened vhost-vdpa character device.
> +#  (Since 8.1)
> +#
>  # Since: 7.2
>  ##
>  { 'struct': 'BlockdevOptionsVirtioBlkVhostVdpa',
> -  'data': { 'path': 'str' },
> +  'data': { '*path': 'str',
> +'*fd': 'str' },
>'if': 'CONFIG_BLKIO' }
>  
>  ##
> diff --git a/block/blkio.c b/block/blkio.c
> index 0cdc99a729..98394b5745 100644
> --- a/block/blkio.c
> +++ b/block/blkio.c
> @@ -694,6 +694,49 @@ static int blkio_virtio_blk_common_open(BlockDriverState 
> *bs,
>  return 0;
>  }
>  
> +static int blkio_virtio_blk_vhost_vdpa_open(BlockDriverState *bs,
> +QDict *options, int flags, Error **errp)
> +{
> +const char *path = qdict_get_try_str(options, "path");
> +const char *fd_str = qdict_get_try_str(options, "fd");
> +BDRVBlkioState *s = bs->opaque;
> +int ret;
> +
> +if (path && fd_str) {
> +error_setg(errp, "'path' and 'fd' options are mutually exclusive");
> +return -EINVAL;
> +}
> +
> +if (!path && !fd_str) {
> +error_setg(errp, "none of 'path' or 'fd' options was specified");
> +return -EINVAL;
> +}
> +
> +if (path) {
> +ret = blkio_set_str(s->blkio, "path", path);
> +qdict_del(options, "path");
> +if (ret < 0) {
> +error_setg_errno(errp, -ret, "failed to set path: %s",
> + blkio_get_error_msg());
> +return ret;
> +}
> +} else {
> +ret = blkio_set_str(s->blkio, "fd", fd_str);

monitor_fd_param() is used by vhost-net, vhost-vsock, vhost-scsi, etc.

I think QEMU should parse the fd string and resolve it to a file
descriptor so the fd passing syntax matches the other vhost devices.


signature.asc
Description: PGP signature

[PATCH 2/3] target/openrisc: Set PC to cpu state on FPU exception

[Stafford Horne]

Store the PC to ensure the correct value can be read in the exception
handler.

Signed-off-by: Stafford Horne 
---
 target/openrisc/fpu_helper.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/target/openrisc/fpu_helper.c b/target/openrisc/fpu_helper.c
index f9e34fa2cc..1feebb9ac7 100644
--- a/target/openrisc/fpu_helper.c
+++ b/target/openrisc/fpu_helper.c
@@ -20,6 +20,7 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
+#include "exec/exec-all.h"
 #include "exec/helper-proto.h"
 #include "exception.h"
 #include "fpu/softfloat.h"
@@ -55,6 +56,9 @@ void HELPER(update_fpcsr)(CPUOpenRISCState *env)
 if (tmp) {
 env->fpcsr |= tmp;
 if (env->fpcsr & FPCSR_FPEE) {
+CPUState *cs = env_cpu(env);
+
+cpu_restore_state(cs, GETPC());
 helper_exception(env, EXCP_FPE);
 }
 }
-- 
2.39.1

[PATCH 1/3] target/openrisc: Allow fpcsr access in user mode

[Stafford Horne]

As per OpenRISC spec 1.4 FPCSR can be read and written in user mode.

Update mtspr and mfspr helpers to support this by moving the is_user
check into the helper.

There is a logic change here to no longer throw an illegal instruction
exception when executing mtspr/mfspr in user mode.  The illegal
instruction exception is not part of the spec, so this should be OK.

Link: 
https://raw.githubusercontent.com/openrisc/doc/master/openrisc-arch-1.4-rev0.pdf
Signed-off-by: Stafford Horne 
---
 target/openrisc/sys_helper.c | 45 +-
 target/openrisc/translate.c  | 72 
 2 files changed, 67 insertions(+), 50 deletions(-)

diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c
index ec145960e3..8a0259c710 100644
--- a/target/openrisc/sys_helper.c
+++ b/target/openrisc/sys_helper.c
@@ -29,17 +29,37 @@
 
 #define TO_SPR(group, number) (((group) << 11) + (number))
 
+static inline bool is_user(CPUOpenRISCState *env)
+{
+#ifdef CONFIG_USER_ONLY
+return true;
+#else
+return (env->sr & SR_SM) == 0;
+#endif
+}
+
 void HELPER(mtspr)(CPUOpenRISCState *env, target_ulong spr, target_ulong rb)
 {
-#ifndef CONFIG_USER_ONLY
 OpenRISCCPU *cpu = env_archcpu(env);
+#ifndef CONFIG_USER_ONLY
 CPUState *cs = env_cpu(env);
 target_ulong mr;
 int idx;
 #endif
 
+/* Handle user accessible SPRs first.  */
 switch (spr) {
+case TO_SPR(0, 20): /* FPCSR */
+cpu_set_fpcsr(env, rb);
+return;
+}
+
+if (is_user(env)) {
+raise_exception(cpu, EXCP_ILLEGAL);
+}
+
 #ifndef CONFIG_USER_ONLY
+switch (spr) {
 case TO_SPR(0, 11): /* EVBAR */
 env->evbar = rb;
 break;
@@ -187,12 +207,8 @@ void HELPER(mtspr)(CPUOpenRISCState *env, target_ulong 
spr, target_ulong rb)
 cpu_openrisc_timer_update(cpu);
 qemu_mutex_unlock_iothread();
 break;
-#endif
-
-case TO_SPR(0, 20): /* FPCSR */
-cpu_set_fpcsr(env, rb);
-break;
 }
+#endif
 }
 
 target_ulong HELPER(mfspr)(CPUOpenRISCState *env, target_ulong rd,
@@ -204,10 +220,22 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, 
target_ulong rd,
 OpenRISCCPU *cpu = env_archcpu(env);
 CPUState *cs = env_cpu(env);
 int idx;
+#else
+OpenRISCCPU *cpu = env_archcpu(env);
 #endif
 
+/* Handle user accessible SPRs first.  */
 switch (spr) {
+case TO_SPR(0, 20): /* FPCSR */
+return env->fpcsr;
+}
+
+if (is_user(env)) {
+raise_exception(cpu, EXCP_ILLEGAL);
+}
+
 #ifndef CONFIG_USER_ONLY
+switch (spr) {
 case TO_SPR(0, 0): /* VR */
 return env->vr;
 
@@ -324,11 +352,8 @@ target_ulong HELPER(mfspr)(CPUOpenRISCState *env, 
target_ulong rd,
 cpu_openrisc_count_update(cpu);
 qemu_mutex_unlock_iothread();
 return cpu_openrisc_count_get(cpu);
-#endif
-
-case TO_SPR(0, 20): /* FPCSR */
-return env->fpcsr;
 }
+#endif
 
 /* for rd is passed in, if rd unchanged, just keep it back.  */
 return rd;
diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
index 76e53c78d4..43ba0cc1ad 100644
--- a/target/openrisc/translate.c
+++ b/target/openrisc/translate.c
@@ -819,45 +819,12 @@ static bool trans_l_xori(DisasContext *dc, arg_rri *a)
 
 static bool trans_l_mfspr(DisasContext *dc, arg_l_mfspr *a)
 {
-check_r0_write(dc, a->d);
-
-if (is_user(dc)) {
-gen_illegal_exception(dc);
-} else {
-TCGv spr = tcg_temp_new();
-
-if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-gen_io_start();
-if (dc->delayed_branch) {
-tcg_gen_mov_tl(cpu_pc, jmp_pc);
-tcg_gen_discard_tl(jmp_pc);
-} else {
-tcg_gen_movi_tl(cpu_pc, dc->base.pc_next + 4);
-}
-dc->base.is_jmp = DISAS_EXIT;
-}
+TCGv spr = tcg_temp_new();
 
-tcg_gen_ori_tl(spr, cpu_R(dc, a->a), a->k);
-gen_helper_mfspr(cpu_R(dc, a->d), cpu_env, cpu_R(dc, a->d), spr);
-}
-return true;
-}
-
-static bool trans_l_mtspr(DisasContext *dc, arg_l_mtspr *a)
-{
-if (is_user(dc)) {
-gen_illegal_exception(dc);
-} else {
-TCGv spr;
+check_r0_write(dc, a->d);
 
-if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
-gen_io_start();
-}
-/* For SR, we will need to exit the TB to recognize the new
- * exception state.  For NPC, in theory this counts as a branch
- * (although the SPR only exists for use by an ICE).  Save all
- * of the cpu state first, allowing it to be overwritten.
- */
+if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
+gen_io_start();
 if (dc->delayed_branch) {
 tcg_gen_mov_tl(cpu_pc, jmp_pc);
 tcg_gen_discard_tl(jmp_pc);
@@ -865,11 +832,36 @@ static bool trans_l_mtspr(DisasContext *dc, arg_l_mtspr 
*a)
 tcg_gen_movi_tl(cpu_pc, dc->base.pc_next + 4);
  

[PATCH 3/3] target/openrisc: Setup FPU for detecting tininess before rounding

[Stafford Horne]

OpenRISC defines tininess to be detected before rounding.  Setup qemu to
obey this.

Signed-off-by: Stafford Horne 
---
 target/openrisc/cpu.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index 0ce4f796fa..cdbff26fb5 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -22,6 +22,7 @@
 #include "qemu/qemu-print.h"
 #include "cpu.h"
 #include "exec/exec-all.h"
+#include "fpu/softfloat-helpers.h"
 #include "tcg/tcg.h"
 
 static void openrisc_cpu_set_pc(CPUState *cs, vaddr value)
@@ -90,6 +91,10 @@ static void openrisc_cpu_reset_hold(Object *obj)
 s->exception_index = -1;
 cpu_set_fpcsr(>env, 0);
 
+set_default_nan_mode(1, >env.fp_status);
+set_float_detect_tininess(float_tininess_before_rounding,
+  >env.fp_status);
+
 #ifndef CONFIG_USER_ONLY
 cpu->env.picmr = 0x;
 cpu->env.picsr = 0x;
-- 
2.39.1

[PATCH 0/3] OpenRISC updates for user space FPU

[Stafford Horne]

This series adds support for the FPU related architecture changes defined in
architecture spec revision v1.4.

 - https://openrisc.io/revisions/r1.4

In summary the architecture changes are:

 - Change FPCSR SPR permissions to allow for reading and writing from user
   space.
 - Clarify that FPU underflow detection is done by detecting tininess before
   rounding.

Previous to this series FPCSR reads and writes from user-mode in QEMU would
throw an illegal argument exception.  The proper behavior should have been to
treat these operations as no-ops as the cpu implementations do.  As mentioned
series changes FPCSR read/write to follow the spec.

The series has been tested with the FPU support added in glibc test suite and
all math tests are passing.

Stafford Horne (3):
  target/openrisc: Allow fpcsr access in user mode
  target/openrisc: Set PC to cpu state on FPU exception
  target/openrisc: Setup FPU for detecting tininess before rounding

 target/openrisc/cpu.c|  5 +++
 target/openrisc/fpu_helper.c |  4 ++
 target/openrisc/sys_helper.c | 45 +-
 target/openrisc/translate.c  | 72 
 4 files changed, 76 insertions(+), 50 deletions(-)

-- 
2.39.1

Re: [PATCH v4 03/20] virtio-scsi: avoid race between unplug and transport event

[Stefan Hajnoczi]

On Tue, May 02, 2023 at 05:19:46PM +0200, Kevin Wolf wrote:
> Am 25.04.2023 um 19:26 hat Stefan Hajnoczi geschrieben:
> > Only report a transport reset event to the guest after the SCSIDevice
> > has been unrealized by qdev_simple_device_unplug_cb().
> > 
> > qdev_simple_device_unplug_cb() sets the SCSIDevice's qdev.realized field
> > to false so that scsi_device_find/get() no longer see it.
> > 
> > scsi_target_emulate_report_luns() also needs to be updated to filter out
> > SCSIDevices that are unrealized.
> > 
> > These changes ensure that the guest driver does not see the SCSIDevice
> > that's being unplugged if it responds very quickly to the transport
> > reset event.
> > 
> > Reviewed-by: Paolo Bonzini 
> > Reviewed-by: Michael S. Tsirkin 
> > Reviewed-by: Daniil Tatianin 
> > Signed-off-by: Stefan Hajnoczi 
> 
> > @@ -1082,6 +1073,15 @@ static void virtio_scsi_hotunplug(HotplugHandler 
> > *hotplug_dev, DeviceState *dev,
> >  blk_set_aio_context(sd->conf.blk, qemu_get_aio_context(), NULL);
> >  virtio_scsi_release(s);
> >  }
> > +
> > +if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) {
> > +virtio_scsi_acquire(s);
> > +virtio_scsi_push_event(s, sd,
> > +   VIRTIO_SCSI_T_TRANSPORT_RESET,
> > +   VIRTIO_SCSI_EVT_RESET_REMOVED);
> > +scsi_bus_set_ua(>bus, SENSE_CODE(REPORTED_LUNS_CHANGED));
> > +virtio_scsi_release(s);
> > +}
> >  }
> 
> s, sd and s->bus are all unrealized at this point, whereas before this
> patch they were still realized. I couldn't find any practical problem
> with it, but it made me nervous enough that I thought I should comment
> on it at least.
> 
> Should we maybe have documentation on these functions that says that
> they accept unrealized objects as their parameters?

s is the VirtIOSCSI controller, not the SCSIDevice that is being
unplugged. The VirtIOSCSI controller is still realized.

s->bus is the VirtIOSCSI controller's bus, it is still realized.

You are right that the SCSIDevice (sd) has been unrealized at this
point:
- sd->conf.blk is safe because qdev properties stay alive the
  Object is deleted, but I'm not sure we should rely on that.
- virti_scsi_push_event(.., sd, ...) is questionable because the LUN
  that's fetched from sd no longer belongs to the unplugged SCSIDevice.

How about I change the code to fetch sd->conf.blk and the LUN before
unplugging?

Stefan


signature.asc
Description: PGP signature

[PATCH 0/2] aio-posix: do not nest poll handlers

[Stefan Hajnoczi]

The following stack exhaustion was reported in
https://bugzilla.redhat.com/show_bug.cgi?id=2186181:

  ...
  #51 0x55884fca7451 aio_poll (qemu-kvm + 0x9d6451)
  #52 0x55884fab9cbd bdrv_poll_co (qemu-kvm + 0x7e8cbd)
  #53 0x55884fab654b blk_io_plug (qemu-kvm + 0x7e554b)
  #54 0x55884f927fef virtio_blk_handle_vq (qemu-kvm + 0x656fef)
  #55 0x55884f96d384 virtio_queue_host_notifier_aio_poll_ready (qemu-kvm + 
0x69c384)
  #56 0x55884fca671b aio_dispatch_handler (qemu-kvm + 0x9d571b)
  #57 0x55884fca7451 aio_poll (qemu-kvm + 0x9d6451)
  #58 0x55884fab9cbd bdrv_poll_co (qemu-kvm + 0x7e8cbd)
  #59 0x55884fab654b blk_io_plug (qemu-kvm + 0x7e554b)
  #60 0x55884f927fef virtio_blk_handle_vq (qemu-kvm + 0x656fef)
  #61 0x55884f96d384 virtio_queue_host_notifier_aio_poll_ready (qemu-kvm + 
0x69c384)
  #62 0x55884fca671b aio_dispatch_handler (qemu-kvm + 0x9d571b)
  #63 0x55884fca7451 aio_poll (qemu-kvm + 0x9d6451)
  ...

This happens because some block layer APIs in QEMU 8.0 run in coroutines in
order to take the graph rdlock. Existing virtqueue handler functions weren't
written with this in mind.

A simplified example of the problem is:

  void my_fd_handler(void *opaque)
  {
  do_something();
  event_notifier_test_and_clear(opaque);
  do_something_else();
  }

When do_something() calls aio_poll(), my_fd_handler() will be entered again
immediately because the fd is still readable and stack exhaustion will occur.

When do_something_else() calls aio_poll(), there is no stack exhaustion since
the event notifier has been cleared and the fd is not readable.

The actual bug is more involved. The handler in question is a poll handler, not
an fd handler, but the principle is the same.

I haven't been able to reproduce the bug, but I have included a test case that
demonstrates the problem.

Stefan Hajnoczi (2):
  aio-posix: do not nest poll handlers
  tested: add test for nested aio_poll() in poll handlers

 tests/unit/test-nested-aio-poll.c | 130 ++
 util/aio-posix.c  |  11 +++
 tests/unit/meson.build|   1 +
 3 files changed, 142 insertions(+)
 create mode 100644 tests/unit/test-nested-aio-poll.c

-- 
2.40.1

[PATCH 2/2] tested: add test for nested aio_poll() in poll handlers

[Stefan Hajnoczi]

Signed-off-by: Stefan Hajnoczi 
---
 tests/unit/test-nested-aio-poll.c | 130 ++
 tests/unit/meson.build|   1 +
 2 files changed, 131 insertions(+)
 create mode 100644 tests/unit/test-nested-aio-poll.c

diff --git a/tests/unit/test-nested-aio-poll.c 
b/tests/unit/test-nested-aio-poll.c
new file mode 100644
index 00..9bbe18b839
--- /dev/null
+++ b/tests/unit/test-nested-aio-poll.c
@@ -0,0 +1,130 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Test that poll handlers are not re-entrant in nested aio_poll()
+ *
+ * Copyright Red Hat
+ *
+ * Poll handlers are usually level-triggered. That means they continue firing
+ * until the condition is reset (e.g. a virtqueue becomes empty). If a poll
+ * handler calls nested aio_poll() before the condition is reset, then infinite
+ * recursion occurs.
+ *
+ * aio_poll() is supposed to prevent this by disabling poll handlers in nested
+ * aio_poll() calls. This test case checks that this is indeed what happens.
+ */
+#include "qemu/osdep.h"
+#include "block/aio.h"
+#include "qapi/error.h"
+
+typedef struct {
+AioContext *ctx;
+
+/* This is the EventNotifier that drives the test */
+EventNotifier poll_notifier;
+
+/* This EventNotifier is only used to wake aio_poll() */
+EventNotifier dummy_notifier;
+
+bool nested;
+} TestData;
+
+static void io_read(EventNotifier *notifier)
+{
+fprintf(stderr, "%s %p\n", __func__, notifier);
+event_notifier_test_and_clear(notifier);
+}
+
+static bool io_poll_true(void *opaque)
+{
+fprintf(stderr, "%s %p\n", __func__, opaque);
+return true;
+}
+
+static bool io_poll_false(void *opaque)
+{
+fprintf(stderr, "%s %p\n", __func__, opaque);
+return false;
+}
+
+static void io_poll_ready(EventNotifier *notifier)
+{
+TestData *td = container_of(notifier, TestData, poll_notifier);
+
+fprintf(stderr, "> %s\n", __func__);
+
+g_assert(!td->nested);
+td->nested = true;
+
+/* Wake the following nested aio_poll() call */
+event_notifier_set(>dummy_notifier);
+
+/* This nested event loop must not call io_poll()/io_poll_ready() */
+g_assert(aio_poll(td->ctx, true));
+
+td->nested = false;
+
+fprintf(stderr, "< %s\n", __func__);
+}
+
+/* dummy_notifier never triggers */
+static void io_poll_never_ready(EventNotifier *notifier)
+{
+g_assert_not_reached();
+}
+
+static void test(void)
+{
+TestData td = {
+.ctx = aio_context_new(_abort),
+};
+
+qemu_set_current_aio_context(td.ctx);
+
+/* Enable polling */
+aio_context_set_poll_params(td.ctx, 100, 2, 2, _abort);
+
+/*
+ * The GSource is unused but this has the side-effect of changing the fdmon
+ * that AioContext uses.
+ */
+aio_get_g_source(td.ctx);
+
+/* Make the event notifier active (set) right away */
+event_notifier_init(_notifier, 1);
+aio_set_event_notifier(td.ctx, _notifier, false,
+   io_read, io_poll_true, io_poll_ready);
+
+/* This event notifier will be used later */
+event_notifier_init(_notifier, 0);
+aio_set_event_notifier(td.ctx, _notifier, false,
+   io_read, io_poll_false, io_poll_never_ready);
+
+/* Consume aio_notify() */
+g_assert(!aio_poll(td.ctx, false));
+
+/*
+ * Run the io_read() handler. This has the side-effect of activating
+ * polling in future aio_poll() calls.
+ */
+g_assert(aio_poll(td.ctx, true));
+
+/* The second time around the io_poll()/io_poll_ready() handler runs */
+g_assert(aio_poll(td.ctx, true));
+
+/* Run io_poll()/io_poll_ready() one more time to show it keeps working */
+g_assert(aio_poll(td.ctx, true));
+
+aio_set_event_notifier(td.ctx, _notifier, false,
+   NULL, NULL, NULL);
+aio_set_event_notifier(td.ctx, _notifier, false, NULL, NULL, NULL);
+event_notifier_cleanup(_notifier);
+event_notifier_cleanup(_notifier);
+aio_context_unref(td.ctx);
+}
+
+int main(int argc, char **argv)
+{
+g_test_init(, , NULL);
+g_test_add_func("/nested-aio-poll", test);
+return g_test_run();
+}
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 3bc78d8660..a314f82baa 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -67,6 +67,7 @@ if have_block
 'test-coroutine': [testblock],
 'test-aio': [testblock],
 'test-aio-multithread': [testblock],
+'test-nested-aio-poll': [testblock],
 'test-throttle': [testblock],
 'test-thread-pool': [testblock],
 'test-hbitmap': [testblock],
-- 
2.40.1

[PATCH 1/2] aio-posix: do not nest poll handlers

[Stefan Hajnoczi]

QEMU's event loop supports nesting, which means that event handler
functions may themselves call aio_poll(). The condition that triggered a
handler must be reset before the nested aio_poll() call, otherwise the
same handler will be called and immediately re-enter aio_poll. This
leads to an infinite loop and stack exhaustion.

Poll handlers are especially prone to this issue, because they typically
reset their condition by finishing the processing of pending work.
Unfortunately it is during the processing of pending work that nested
aio_poll() calls typically occur and the condition has not yet been
reset.

Disable a poll handler during ->io_poll_ready() so that a nested
aio_poll() call cannot invoke ->io_poll_ready() again. As a result, the
disabled poll handler and its associated fd handler do not run during
the nested aio_poll(). Calling aio_set_fd_handler() from inside nested
aio_poll() could cause it to run again. If the fd handler is pending
inside nested aio_poll(), then it will also run again.

In theory fd handlers can be affected by the same issue, but they are
more likely to reset the condition before calling nested aio_poll().

This is a special case and it's somewhat complex, but I don't see a way
around it as long as nested aio_poll() is supported.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2186181
Fixes: c38270692593 ("block: Mark bdrv_co_io_(un)plug() and callers 
GRAPH_RDLOCK")
Cc: Kevin Wolf 
Cc: Emanuele Giuseppe Esposito 
Cc: Paolo Bonzini 
Signed-off-by: Stefan Hajnoczi 
---
 util/aio-posix.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/util/aio-posix.c b/util/aio-posix.c
index a8be940f76..34bc2a64d8 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -353,8 +353,19 @@ static bool aio_dispatch_handler(AioContext *ctx, 
AioHandler *node)
 poll_ready && revents == 0 &&
 aio_node_check(ctx, node->is_external) &&
 node->io_poll_ready) {
+/*
+ * Remove temporarily to avoid infinite loops when ->io_poll_ready()
+ * calls aio_poll() before clearing the condition that made the poll
+ * handler become ready.
+ */
+QLIST_SAFE_REMOVE(node, node_poll);
+
 node->io_poll_ready(node->opaque);
 
+if (!QLIST_IS_INSERTED(node, node_poll)) {
+QLIST_INSERT_HEAD(>poll_aio_handlers, node, node_poll);
+}
+
 /*
  * Return early since revents was zero. aio_notify() does not count as
  * progress.
-- 
2.40.1

Re: [PULL 0/3] Various fixes

[Richard Henderson]

On 5/2/23 11:18, Thomas Huth wrote:

  Hi Richard!

The following changes since commit 7c18f2d663521f1b31b821a13358ce38075eaf7d:

   Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging 
(2023-04-29 23:07:17 +0100)

are available in the Git repository at:

   https://gitlab.com/thuth/qemu.git tags/pull-request-2023-05-02

for you to fetch changes up to 7915bd06f25e1803778081161bf6fa10c42dc7cd:

   async: avoid use-after-free on re-entrancy guard (2023-05-02 10:03:26 +0200)


* Fix the failing FreeBSD job in our CI
* Run the tpm-tis-i2c-test only if TCG is enabled
* Fix a use-after-free problem in the new reentracy checking code


Alexander Bulekov (1):
   async: avoid use-after-free on re-entrancy guard

Fabiano Rosas (1):
   tests/qtest: Restrict tpm-tis-i2c-test to CONFIG_TCG

Thomas Huth (1):
   tests/qtest: Disable the spice test of readconfig-test on FreeBSD

  tests/qtest/readconfig-test.c |  6 +++---
  util/async.c  | 14 --
  tests/qtest/meson.build   |  3 ++-
  3 files changed, 13 insertions(+), 10 deletions(-)



Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/8.1 as 
appropriate.


r~

Re: [PULL v2 00/34] target-arm queue

[Richard Henderson]

On 5/2/23 15:49, Peter Maydell wrote:

v2 changes: dropped the patch that enables the new 'notcg' CI test:
it doesn't pass on our aarch64 runner because the CI runner doesn't
have access to /dev/kvm.

thanks
-- PMM

The following changes since commit 7c18f2d663521f1b31b821a13358ce38075eaf7d:

   Merge tag 'for-upstream' ofhttps://gitlab.com/bonzini/qemu  into staging 
(2023-04-29 23:07:17 +0100)

are available in the Git repository at:

   https://git.linaro.org/people/pmaydell/qemu-arm.git  
tags/pull-target-arm-20230502-2

for you to fetch changes up to a4ae17e5ec512862bf73e40dfbb1e7db71f2c1e7:

   hw/net/allwinner-sun8i-emac: Correctly byteswap descriptor fields 
(2023-05-02 15:47:41 +0100)


target-arm queue:
  * Support building Arm targets with CONFIG_TCG=no (ie KVM only)
  * hw/net: npcm7xx_emc: set MAC in register space
  * hw/arm/bcm2835_property: Implement "get command line" message
  * Deprecate the '-singlestep' command line option in favour of
'-one-insn-per-tb' and '-accel one-insn-per-tb=on'
  * Deprecate 'singlestep' member of QMP StatusInfo struct
  * docs/about/deprecated.rst: Add "since 7.1" tag to dtb-kaslr-seed deprecation
  * hw/net/msf2-emac: Don't modify descriptor in-place in emac_store_desc()
  * raspi, aspeed: Write bootloader code correctly on big-endian hosts
  * hw/intc/allwinner-a10-pic: Fix bug on big-endian hosts
  * Fix bug in A32 ERET on big-endian hosts that caused guest crash
  * hw/sd/allwinner-sdhost: Correctly byteswap descriptor fields
  * hw/net/allwinner-sun8i-emac: Correctly byteswap descriptor fields


Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/8.1 as 
appropriate.


r~

Re: [PATCH v4 02/10] colo: make colo_checkpoint_notify static and provide simpler API

[Juan Quintela]

Vladimir Sementsov-Ogievskiy  wrote:
> colo_checkpoint_notify() is mostly used in colo.c. Outside we use it
> once when x-checkpoint-delay migration parameter is set. So, let's
> simplify the external API to only that function - notify COLO that
> parameter was set. This make external API more robust and hides
> implementation details from external callers. Also this helps us to
> make COLO module optional in further patch (i.e. we are going to add
> possibility not build the COLO module).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Reviewed-by: Juan Quintela 

Re: [PATCH v4 06/10] migration: process_incoming_migration_co: simplify code flow around ret

[Juan Quintela]

Vladimir Sementsov-Ogievskiy  wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Reviewed-by: Juan Quintela 

Re: [PATCH v4 05/10] migration: drop colo_incoming_thread from MigrationIncomingState

[Juan Quintela]

Vladimir Sementsov-Ogievskiy  wrote:
> have_colo_incoming_thread variable is unused. colo_incoming_thread can
> be local.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Reviewed-by: Juan Quintela 

Re: [PATCH v20 03/21] target/s390x/cpu topology: handle STSI(15) and build the SYSIB

[Nina Schoetterl-Glausch]

On Tue, 2023-04-25 at 18:14 +0200, Pierre Morel wrote:
> On interception of STSI(15.1.x) the System Information Block
> (SYSIB) is built from the list of pre-ordered topology entries.
> 
> Signed-off-by: Pierre Morel 
> ---
>  MAINTAINERS |   1 +
>  include/hw/s390x/cpu-topology.h |  24 +++
>  include/hw/s390x/sclp.h |   1 +
>  target/s390x/cpu.h  |  72 
>  hw/s390x/cpu-topology.c |  13 +-
>  target/s390x/kvm/cpu_topology.c | 308 
>  target/s390x/kvm/kvm.c  |   5 +-
>  target/s390x/kvm/meson.build|   3 +-
>  8 files changed, 424 insertions(+), 3 deletions(-)
>  create mode 100644 target/s390x/kvm/cpu_topology.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bb7b34d0d8..de9052f753 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1659,6 +1659,7 @@ M: Pierre Morel 
>  S: Supported
>  F: include/hw/s390x/cpu-topology.h
>  F: hw/s390x/cpu-topology.c
> +F: target/s390x/kvm/cpu_topology.c
>  
>  X86 Machines
>  
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index af36f634e0..87bfeb631e 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -15,9 +15,33 @@
> 
[...]

> +typedef struct S390TopologyEntry {
> +QTAILQ_ENTRY(S390TopologyEntry) next;
> +s390_topology_id id;
> +uint64_t mask;
> +} S390TopologyEntry;
> +
>  typedef struct S390Topology {
>  uint8_t *cores_per_socket;
> +QTAILQ_HEAD(, S390TopologyEntry) list;

Since you recompute the list on every STSI, you no longer need this in here.
You can create it in insert_stsi_15_1_x.

>  CpuTopology *smp;
> +bool vertical_polarization;
>  } S390Topology;

[...]

> +/*
> + * Macro to check that the size of data after increment
> + * will not get bigger than the size of the SysIB.
> + */
> +#define SYSIB_GUARD(data, x) do {   \
> +data += x;  \
> +if (data  > sizeof(SysIB)) {\
^ two spaces

> +return 0;   \
> +}   \
> +} while (0)
> +

[...]

> +/**
> + * s390_topology_from_cpu:
> + * @cpu: The S390CPU
> + *
> + * Initialize the topology id from the CPU environment.
> + */
> +static s390_topology_id s390_topology_from_cpu(S390CPU *cpu)
> +{
> +s390_topology_id topology_id = {0};
> +
> +topology_id.drawer = cpu->env.drawer_id;
> +topology_id.book = cpu->env.book_id;
> +topology_id.socket = cpu->env.socket_id;
> +topology_id.origin = cpu->env.core_id / 64;
> +topology_id.type = S390_TOPOLOGY_CPU_IFL;
> +topology_id.dedicated = cpu->env.dedicated;
> +
> +if (s390_topology.vertical_polarization) {
> +/*
> + * Vertical polarization with dedicated CPU implies
> + * vertical high entitlement.
> + */

This has already been adjusted or rejected when the entitlement was set.

> +if (topology_id.dedicated) {
> +topology_id.entitlement = S390_CPU_ENTITLEMENT_HIGH;
> +} else {
> +topology_id.entitlement = cpu->env.entitlement;

You only need this assignment.
> +}
> +}
> +
> +return topology_id;

[...]

Re: [PATCH v11 00/13] target/arm: Allow CONFIG_TCG=n builds

[Fabiano Rosas]

Fabiano Rosas  writes:

> Peter Maydell  writes:
>
>> On Tue, 2 May 2023 at 15:51, Peter Maydell  wrote:
>>>
>>> On Tue, 2 May 2023 at 10:55, Peter Maydell  wrote:
>>> >
>>> > On Wed, 26 Apr 2023 at 19:00, Fabiano Rosas  wrote:
>>> > >
>>> > > Hi,
>>> > >
>>> > > Some minor changes:
>>> > >
>>> > > - new patch to move a test under CONFIG_TCG (broken on master);
>>> > > - new patch to document the unsupported CPU test (Philippe);
>>> > > - changed the test skip message when no KVM or TCG are present (Igor).
>>> >
>>> > Applied to target-arm.next; thanks for your persistence in
>>> > working through the many versions of this patchset.
>>>
>>> Update: I had to drop "gitlab-ci: Check building KVM-only aarch64 target"
>>> because it enables a CI job that fails on our aarch64 runner
>>> (because it wants to run tests using KVM but that machine
>>> isn't configured to allow the runner to use KVM).
>>
>> We fixed the runner config, but the CI still fails on that notcg
>> job because it is trying to run tests that explicitly use
>> '-accel tcg':
>> https://gitlab.com/qemu-project/qemu/-/jobs/4212850809#L3595
>>
>> Something is weird here, because we built without TCG support
>> on an aarch64 host but we still got qemu-system-i386
>> and qemu-system-x86_64 binaries, which then don't work
>> and cause the tests to fail...
>>
>
> Hmm, that's potentially due to Xen. Looks like we need more (!tcg &&
> !kvm) checks. Let me try to reproduce it.

Ah right, the test is skipped on my aarch64 host because I don't have
genisomage available. So what we need is this:

-- >8 --
From: Fabiano Rosas 
Date: Tue, 2 May 2023 13:42:14 -0300
Subject: [PATCH] fixup! tests/qtest: Fix tests when no KVM or TCG are present

---
 tests/qtest/cdrom-test.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
index 26a2400181..09655e6ff0 100644
--- a/tests/qtest/cdrom-test.c
+++ b/tests/qtest/cdrom-test.c
@@ -205,6 +205,11 @@ int main(int argc, char **argv)
 
 g_test_init(, , NULL);
 
+if (!qtest_has_accel("tcg") && !qtest_has_accel("kvm")) {
+g_test_skip("No KVM or TCG accelerator available");
+return 0;
+}
+
 if (exec_genisoimg(genisocheck)) {
 /* genisoimage not available - so can't run tests */
 return g_test_run();
-- 
2.35.3

Re: [PULL 05/27] hw/xen: Watches on XenStore transactions

[Peter Maydell]

On Tue, 7 Mar 2023 at 18:27, David Woodhouse  wrote:
>
> From: David Woodhouse 
>
> Firing watches on the nodes that still exist is relatively easy; just
> walk the tree and look at the nodes with refcount of one.
>
> Firing watches on *deleted* nodes is more fun. We add 'modified_in_tx'
> and 'deleted_in_tx' flags to each node. Nodes with those flags cannot
> be shared, as they will always be unique to the transaction in which
> they were created.
>
> When xs_node_walk would need to *create* a node as scaffolding and it
> encounters a deleted_in_tx node, it can resurrect it simply by clearing
> its deleted_in_tx flag. If that node originally had any *data*, they're
> gone, and the modified_in_tx flag will have been set when it was first
> deleted.
>
> We then attempt to send appropriate watches when the transaction is
> committed, properly delete the deleted_in_tx nodes, and remove the
> modified_in_tx flag from the others.
>
> Signed-off-by: David Woodhouse 
> Reviewed-by: Paul Durrant 

Hi; Coverity's "is there missing error handling?"
heuristic fired for a change in this code (CID 1508359):

>  static int transaction_commit(XenstoreImplState *s, XsTransaction *tx)
>  {
> +struct walk_op op;
> +XsNode **n;
> +
>  if (s->root_tx != tx->base_tx) {
>  return EAGAIN;
>  }
> @@ -720,10 +861,18 @@ static int transaction_commit(XenstoreImplState *s, 
> XsTransaction *tx)
>  s->root_tx = tx->tx_id;
>  s->nr_nodes = tx->nr_nodes;
>
> +init_walk_op(s, , XBT_NULL, tx->dom_id, "/", );

This is the only call to init_walk_op() which ignores its
return value. Intentional, or missing error handling?

> +op.deleted_in_tx = false;
> +op.mutating = true;
> +
>  /*
> - * XX: Walk the new root and fire watches on any node which has a
> + * Walk the new root and fire watches on any node which has a
>   * refcount of one (which is therefore unique to this transaction).
>   */
> +if (s->root->children) {
> +g_hash_table_foreach_remove(s->root->children, tx_commit_walk, );
> +}
> +
>  return 0;
>  }

thanks
-- PMM

Re: [PULL 11/73] cryptodev: Support query-stats QMP command

[Peter Maydell]

On Wed, 8 Mar 2023 at 01:11, Michael S. Tsirkin  wrote:
>
> From: zhenwei pi 
>
> Now we can use "query-stats" QMP command to query statistics of
> crypto devices. (Originally this was designed to show statistics
> by '{"execute": "query-cryptodev"}'. Daniel Berrangé suggested that
> querying configuration info by "query-cryptodev", and querying
> runtime performance info by "query-stats". This makes sense!)

Hi; Coverity points out (CID 1508074) that this change
introduces a memory leak:

> +static int cryptodev_backend_stats_query(Object *obj, void *data)
> +{

> +entry = g_new0(StatsResult, 1);
> +entry->provider = STATS_PROVIDER_CRYPTODEV;
> +entry->qom_path = g_strdup(object_get_canonical_path(obj));

object_get_canonical_path() already returns allocated memory
that the caller should free with g_free(), so we should not
g_strdup() it (which then leaks that memory).

> +entry->stats = stats_list;
> +QAPI_LIST_PREPEND(*stats_results, entry);
> +
> +return 0;
> +}

Would somebody like to send a patch?

thanks
-- PMM

Re: [PATCH v4 06/10] migration: process_incoming_migration_co: simplify code flow around ret

[Peter Xu]

On Fri, Apr 28, 2023 at 10:49:24PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Reviewed-by: Peter Xu 

-- 
Peter Xu

Re: [PATCH v4 05/10] migration: drop colo_incoming_thread from MigrationIncomingState

[Peter Xu]

On Fri, Apr 28, 2023 at 10:49:23PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> have_colo_incoming_thread variable is unused. colo_incoming_thread can
> be local.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Reviewed-by: Peter Xu 

-- 
Peter Xu

Re: [PATCH v4 03/10] build: move COLO under CONFIG_REPLICATION

[Peter Xu]

On Fri, Apr 28, 2023 at 10:49:21PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We don't allow to use x-colo capability when replication is not
> configured. So, no reason to build COLO when replication is disabled,
> it's unusable in this case.
> 
> Note also that the check in migrate_caps_check() is not the only
> restriction: some functions in migration/colo.c will just abort if
> called with not defined CONFIG_REPLICATION, for example:
> 
> migration_iteration_finish()
>case MIGRATION_STATUS_COLO:
>migrate_start_colo_process()
>colo_process_checkpoint()
>abort()
> 
> It could probably make sense to have possibility to enable COLO without
> REPLICATION, but this requires deeper audit of colo & replication code,
> which may be done later if needed.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Acked-by: Dr. David Alan Gilbert 
> ---
>  hmp-commands.hx|  2 ++
>  migration/colo.c   | 28 
>  migration/meson.build  |  6 --
>  migration/migration-hmp-cmds.c |  2 ++
>  migration/migration.c  |  6 ++
>  qapi/migration.json|  9 +---
>  stubs/colo.c   | 39 ++
>  stubs/meson.build  |  1 +
>  8 files changed, 60 insertions(+), 33 deletions(-)
>  create mode 100644 stubs/colo.c
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index bb85ee1d26..fbd0932232 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1035,6 +1035,7 @@ SRST
>migration (or once already in postcopy).
>  ERST
>  
> +#ifdef CONFIG_REPLICATION
>  {
>  .name   = "x_colo_lost_heartbeat",
>  .args_type  = "",
> @@ -1043,6 +1044,7 @@ ERST
>"a failover or takeover is needed.",
>  .cmd = hmp_x_colo_lost_heartbeat,
>  },
> +#endif
>  
>  SRST
>  ``x_colo_lost_heartbeat``
> diff --git a/migration/colo.c b/migration/colo.c
> index c9e0b909b9..6c7c313956 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -26,9 +26,7 @@
>  #include "qemu/rcu.h"
>  #include "migration/failover.h"
>  #include "migration/ram.h"
> -#ifdef CONFIG_REPLICATION
>  #include "block/replication.h"
> -#endif
>  #include "net/colo-compare.h"
>  #include "net/colo.h"
>  #include "block/block.h"
> @@ -86,7 +84,6 @@ void colo_checkpoint_delay_set(void)
>  static void secondary_vm_do_failover(void)
>  {
>  /* COLO needs enable block-replication */
> -#ifdef CONFIG_REPLICATION
>  int old_state;
>  MigrationIncomingState *mis = migration_incoming_get_current();
>  Error *local_err = NULL;
> @@ -151,14 +148,10 @@ static void secondary_vm_do_failover(void)
>  if (mis->migration_incoming_co) {
>  qemu_coroutine_enter(mis->migration_incoming_co);
>  }
> -#else
> -abort();
> -#endif
>  }
>  
>  static void primary_vm_do_failover(void)
>  {
> -#ifdef CONFIG_REPLICATION
>  MigrationState *s = migrate_get_current();
>  int old_state;
>  Error *local_err = NULL;
> @@ -199,9 +192,6 @@ static void primary_vm_do_failover(void)
>  
>  /* Notify COLO thread that failover work is finished */
>  qemu_sem_post(>colo_exit_sem);
> -#else
> -abort();
> -#endif
>  }
>  
>  COLOMode get_colo_mode(void)
> @@ -235,7 +225,6 @@ void colo_do_failover(void)
>  }
>  }
>  
> -#ifdef CONFIG_REPLICATION
>  void qmp_xen_set_replication(bool enable, bool primary,
>   bool has_failover, bool failover,
>   Error **errp)
> @@ -289,7 +278,6 @@ void qmp_xen_colo_do_checkpoint(Error **errp)
>  /* Notify all filters of all NIC to do checkpoint */
>  colo_notify_filters_event(COLO_EVENT_CHECKPOINT, errp);
>  }
> -#endif
>  
>  COLOStatus *qmp_query_colo_status(Error **errp)
>  {
> @@ -453,15 +441,11 @@ static int 
> colo_do_checkpoint_transaction(MigrationState *s,
>  }
>  qemu_mutex_lock_iothread();
>  
> -#ifdef CONFIG_REPLICATION
>  replication_do_checkpoint_all(_err);
>  if (local_err) {
>  qemu_mutex_unlock_iothread();
>  goto out;
>  }
> -#else
> -abort();
> -#endif
>  
>  colo_send_message(s->to_dst_file, COLO_MESSAGE_VMSTATE_SEND, _err);
>  if (local_err) {
> @@ -579,15 +563,11 @@ static void colo_process_checkpoint(MigrationState *s)
>  object_unref(OBJECT(bioc));
>  
>  qemu_mutex_lock_iothread();
> -#ifdef CONFIG_REPLICATION
>  replication_start_all(REPLICATION_MODE_PRIMARY, _err);
>  if (local_err) {
>  qemu_mutex_unlock_iothread();
>  goto out;
>  }
> -#else
> -abort();
> -#endif
>  
>  vm_start();
>  qemu_mutex_unlock_iothread();
> @@ -755,7 +735,6 @@ static void 
> colo_incoming_process_checkpoint(MigrationIncomingState *mis,
>  return;
>  }
>  
> -#ifdef CONFIG_REPLICATION
>  replication_get_error_all(_err);
>  if (local_err) {
>  error_propagate(errp, local_err);
> @@ 

Re: [PATCH v11 00/13] target/arm: Allow CONFIG_TCG=n builds

[Fabiano Rosas]

Peter Maydell  writes:

> On Tue, 2 May 2023 at 15:51, Peter Maydell  wrote:
>>
>> On Tue, 2 May 2023 at 10:55, Peter Maydell  wrote:
>> >
>> > On Wed, 26 Apr 2023 at 19:00, Fabiano Rosas  wrote:
>> > >
>> > > Hi,
>> > >
>> > > Some minor changes:
>> > >
>> > > - new patch to move a test under CONFIG_TCG (broken on master);
>> > > - new patch to document the unsupported CPU test (Philippe);
>> > > - changed the test skip message when no KVM or TCG are present (Igor).
>> >
>> > Applied to target-arm.next; thanks for your persistence in
>> > working through the many versions of this patchset.
>>
>> Update: I had to drop "gitlab-ci: Check building KVM-only aarch64 target"
>> because it enables a CI job that fails on our aarch64 runner
>> (because it wants to run tests using KVM but that machine
>> isn't configured to allow the runner to use KVM).
>
> We fixed the runner config, but the CI still fails on that notcg
> job because it is trying to run tests that explicitly use
> '-accel tcg':
> https://gitlab.com/qemu-project/qemu/-/jobs/4212850809#L3595
>
> Something is weird here, because we built without TCG support
> on an aarch64 host but we still got qemu-system-i386
> and qemu-system-x86_64 binaries, which then don't work
> and cause the tests to fail...
>

Hmm, that's potentially due to Xen. Looks like we need more (!tcg &&
!kvm) checks. Let me try to reproduce it.

Re: [PATCH v4 10/20] block: drain from main loop thread in bdrv_co_yield_to_drain()

[Kevin Wolf]

Am 25.04.2023 um 19:27 hat Stefan Hajnoczi geschrieben:
> For simplicity, always run BlockDevOps .drained_begin/end/poll()
> callbacks in the main loop thread. This makes it easier to implement the
> callbacks and avoids extra locks.
> 
> Move the function pointer declarations from the I/O Code section to the
> Global State section in block-backend-common.h.
> 
> Signed-off-by: Stefan Hajnoczi 

If we're updating function pointers, we should probably update them in
BdrvChildClass and BlockDriver, too.

This means that a non-coroutine caller can't run in an iothread, not
even the home iothread of the BlockDriverState. (I'm not sure if it was
allowed previously. I don't think we're actually doing this, but in
theory it could have worked.) Maybe put a GLOBAL_STATE_CODE() after
handling the bdrv_co_yield_to_drain() case? Or would that look too odd?

IO_OR_GS_CODE();

if (qemu_in_coroutine()) {
bdrv_co_yield_to_drain(bs, true, parent, poll);
return;
}

GLOBAL_STATE_CODE();

Kevin

Re: [PATCH v3 00/57] tcg: Improve atomicity support

[Peter Maydell]

On Tue, 25 Apr 2023 at 20:32, Richard Henderson
 wrote:
>
> v1: 
> https://lore.kernel.org/qemu-devel/20221118094754.242910-1-richard.hender...@linaro.org/
> v2: 
> https://lore.kernel.org/qemu-devel/20230216025739.1211680-1-richard.hender...@linaro.org/
>
> Based-on: 20230424054105.1579315-1-richard.hender...@linaro.org
> ("[PATCH v3 00/57] tcg: Simplify calls to load/store helpers")
>
> The main objective here is to support Arm FEAT_LSE2, which says that any
> single memory access that does not cross a 16-byte boundary is atomic.
> This is the MO_ATOM_WITHIN16 control.
>
> While I'm touching all of this, a secondary objective is to handle the
> atomicity of the IBM machines.  Both Power and s390x treat misaligned
> accesses as atomic on the lsb of the pointer.  For instance, an 8-byte
> access at ptr % 8 == 4 will appear as two atomic 4-byte accesses, and
> ptr % 4 == 2 will appear as four 3-byte accesses.

I hope you mean "four 2-byte accesses", although I'm never
quite sure how odd s390x can get :-)

-- PMM

[PATCH 03/16] target/alpha: Remove TARGET_ALIGNED_ONLY

[Richard Henderson]

Signed-off-by: Richard Henderson 
---
 configs/targets/alpha-linux-user.mak | 1 -
 configs/targets/alpha-softmmu.mak| 1 -
 2 files changed, 2 deletions(-)

diff --git a/configs/targets/alpha-linux-user.mak 
b/configs/targets/alpha-linux-user.mak
index 7e62fd796a..f7d3fb4afa 100644
--- a/configs/targets/alpha-linux-user.mak
+++ b/configs/targets/alpha-linux-user.mak
@@ -1,4 +1,3 @@
 TARGET_ARCH=alpha
 TARGET_SYSTBL_ABI=common
 TARGET_SYSTBL=syscall.tbl
-TARGET_ALIGNED_ONLY=y
diff --git a/configs/targets/alpha-softmmu.mak 
b/configs/targets/alpha-softmmu.mak
index e4b874a19e..9dbe160740 100644
--- a/configs/targets/alpha-softmmu.mak
+++ b/configs/targets/alpha-softmmu.mak
@@ -1,3 +1,2 @@
 TARGET_ARCH=alpha
-TARGET_ALIGNED_ONLY=y
 TARGET_SUPPORTS_MTTCG=y
-- 
2.34.1

[PATCH 01/16] target/alpha: Use MO_ALIGN for system UNALIGN()

[Richard Henderson]

Signed-off-by: Richard Henderson 
---
 target/alpha/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/alpha/translate.c b/target/alpha/translate.c
index 9d25e21164..ffbac1c114 100644
--- a/target/alpha/translate.c
+++ b/target/alpha/translate.c
@@ -72,7 +72,7 @@ struct DisasContext {
 #ifdef CONFIG_USER_ONLY
 #define UNALIGN(C)  (C)->unalign
 #else
-#define UNALIGN(C)  0
+#define UNALIGN(C)  MO_ALIGN
 #endif
 
 /* Target-specific return values from translate_one, indicating the
-- 
2.34.1

[PATCH 08/16] target/mips: Use MO_ALIGN instead of 0

[Richard Henderson]

The opposite of MO_UNALN is MO_ALIGN.

Signed-off-by: Richard Henderson 
---
 target/mips/tcg/nanomips_translate.c.inc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/mips/tcg/nanomips_translate.c.inc 
b/target/mips/tcg/nanomips_translate.c.inc
index b96dcd2ae9..a98dde0d2e 100644
--- a/target/mips/tcg/nanomips_translate.c.inc
+++ b/target/mips/tcg/nanomips_translate.c.inc
@@ -4305,7 +4305,7 @@ static int decode_nanomips_32_48_opc(CPUMIPSState *env, 
DisasContext *ctx)
 TCGv va = tcg_temp_new();
 TCGv t1 = tcg_temp_new();
 MemOp memop = (extract32(ctx->opcode, 8, 3)) ==
-  NM_P_LS_UAWM ? MO_UNALN : 0;
+  NM_P_LS_UAWM ? MO_UNALN : MO_ALIGN;
 
 count = (count == 0) ? 8 : count;
 while (counter != count) {
-- 
2.34.1

[PATCH 15/16] target/sparc: Remove TARGET_ALIGNED_ONLY

[Richard Henderson]

Signed-off-by: Richard Henderson 
---
 configs/targets/sparc-linux-user.mak   | 1 -
 configs/targets/sparc-softmmu.mak  | 1 -
 configs/targets/sparc32plus-linux-user.mak | 1 -
 configs/targets/sparc64-linux-user.mak | 1 -
 configs/targets/sparc64-softmmu.mak| 1 -
 5 files changed, 5 deletions(-)

diff --git a/configs/targets/sparc-linux-user.mak 
b/configs/targets/sparc-linux-user.mak
index 00e7bc1f07..abcfb8fc62 100644
--- a/configs/targets/sparc-linux-user.mak
+++ b/configs/targets/sparc-linux-user.mak
@@ -1,5 +1,4 @@
 TARGET_ARCH=sparc
 TARGET_SYSTBL_ABI=common,32
 TARGET_SYSTBL=syscall.tbl
-TARGET_ALIGNED_ONLY=y
 TARGET_BIG_ENDIAN=y
diff --git a/configs/targets/sparc-softmmu.mak 
b/configs/targets/sparc-softmmu.mak
index a849190f01..454eb35499 100644
--- a/configs/targets/sparc-softmmu.mak
+++ b/configs/targets/sparc-softmmu.mak
@@ -1,3 +1,2 @@
 TARGET_ARCH=sparc
-TARGET_ALIGNED_ONLY=y
 TARGET_BIG_ENDIAN=y
diff --git a/configs/targets/sparc32plus-linux-user.mak 
b/configs/targets/sparc32plus-linux-user.mak
index a65c0951a1..6cc8fa516b 100644
--- a/configs/targets/sparc32plus-linux-user.mak
+++ b/configs/targets/sparc32plus-linux-user.mak
@@ -4,5 +4,4 @@ TARGET_BASE_ARCH=sparc
 TARGET_ABI_DIR=sparc
 TARGET_SYSTBL_ABI=common,32
 TARGET_SYSTBL=syscall.tbl
-TARGET_ALIGNED_ONLY=y
 TARGET_BIG_ENDIAN=y
diff --git a/configs/targets/sparc64-linux-user.mak 
b/configs/targets/sparc64-linux-user.mak
index 20fcb93fa4..52f05ec000 100644
--- a/configs/targets/sparc64-linux-user.mak
+++ b/configs/targets/sparc64-linux-user.mak
@@ -3,5 +3,4 @@ TARGET_BASE_ARCH=sparc
 TARGET_ABI_DIR=sparc
 TARGET_SYSTBL_ABI=common,64
 TARGET_SYSTBL=syscall.tbl
-TARGET_ALIGNED_ONLY=y
 TARGET_BIG_ENDIAN=y
diff --git a/configs/targets/sparc64-softmmu.mak 
b/configs/targets/sparc64-softmmu.mak
index c626ac3eae..d3f8a3b710 100644
--- a/configs/targets/sparc64-softmmu.mak
+++ b/configs/targets/sparc64-softmmu.mak
@@ -1,4 +1,3 @@
 TARGET_ARCH=sparc64
 TARGET_BASE_ARCH=sparc
-TARGET_ALIGNED_ONLY=y
 TARGET_BIG_ENDIAN=y
-- 
2.34.1

[PATCH 09/16] target/mips: Remove TARGET_ALIGNED_ONLY

[Richard Henderson]

Signed-off-by: Richard Henderson 
---
 configs/targets/mips-linux-user.mak  | 1 -
 configs/targets/mips-softmmu.mak | 1 -
 configs/targets/mips64-linux-user.mak| 1 -
 configs/targets/mips64-softmmu.mak   | 1 -
 configs/targets/mips64el-linux-user.mak  | 1 -
 configs/targets/mips64el-softmmu.mak | 1 -
 configs/targets/mipsel-linux-user.mak| 1 -
 configs/targets/mipsel-softmmu.mak   | 1 -
 configs/targets/mipsn32-linux-user.mak   | 1 -
 configs/targets/mipsn32el-linux-user.mak | 1 -
 10 files changed, 10 deletions(-)

diff --git a/configs/targets/mips-linux-user.mak 
b/configs/targets/mips-linux-user.mak
index 71fa77d464..b4569a9893 100644
--- a/configs/targets/mips-linux-user.mak
+++ b/configs/targets/mips-linux-user.mak
@@ -2,5 +2,4 @@ TARGET_ARCH=mips
 TARGET_ABI_MIPSO32=y
 TARGET_SYSTBL_ABI=o32
 TARGET_SYSTBL=syscall_o32.tbl
-TARGET_ALIGNED_ONLY=y
 TARGET_BIG_ENDIAN=y
diff --git a/configs/targets/mips-softmmu.mak b/configs/targets/mips-softmmu.mak
index 7787a4d94c..d34b4083fc 100644
--- a/configs/targets/mips-softmmu.mak
+++ b/configs/targets/mips-softmmu.mak
@@ -1,4 +1,3 @@
 TARGET_ARCH=mips
-TARGET_ALIGNED_ONLY=y
 TARGET_BIG_ENDIAN=y
 TARGET_SUPPORTS_MTTCG=y
diff --git a/configs/targets/mips64-linux-user.mak 
b/configs/targets/mips64-linux-user.mak
index 5a4771f22d..d2ff509a11 100644
--- a/configs/targets/mips64-linux-user.mak
+++ b/configs/targets/mips64-linux-user.mak
@@ -3,5 +3,4 @@ TARGET_ABI_MIPSN64=y
 TARGET_BASE_ARCH=mips
 TARGET_SYSTBL_ABI=n64
 TARGET_SYSTBL=syscall_n64.tbl
-TARGET_ALIGNED_ONLY=y
 TARGET_BIG_ENDIAN=y
diff --git a/configs/targets/mips64-softmmu.mak 
b/configs/targets/mips64-softmmu.mak
index 568d66650c..12d9483bf0 100644
--- a/configs/targets/mips64-softmmu.mak
+++ b/configs/targets/mips64-softmmu.mak
@@ -1,4 +1,3 @@
 TARGET_ARCH=mips64
 TARGET_BASE_ARCH=mips
-TARGET_ALIGNED_ONLY=y
 TARGET_BIG_ENDIAN=y
diff --git a/configs/targets/mips64el-linux-user.mak 
b/configs/targets/mips64el-linux-user.mak
index f348f35997..f9efeec8ea 100644
--- a/configs/targets/mips64el-linux-user.mak
+++ b/configs/targets/mips64el-linux-user.mak
@@ -3,4 +3,3 @@ TARGET_ABI_MIPSN64=y
 TARGET_BASE_ARCH=mips
 TARGET_SYSTBL_ABI=n64
 TARGET_SYSTBL=syscall_n64.tbl
-TARGET_ALIGNED_ONLY=y
diff --git a/configs/targets/mips64el-softmmu.mak 
b/configs/targets/mips64el-softmmu.mak
index 5a52aa4b64..8d9ab3ddc4 100644
--- a/configs/targets/mips64el-softmmu.mak
+++ b/configs/targets/mips64el-softmmu.mak
@@ -1,4 +1,3 @@
 TARGET_ARCH=mips64
 TARGET_BASE_ARCH=mips
-TARGET_ALIGNED_ONLY=y
 TARGET_NEED_FDT=y
diff --git a/configs/targets/mipsel-linux-user.mak 
b/configs/targets/mipsel-linux-user.mak
index e23793070c..e8d7241d31 100644
--- a/configs/targets/mipsel-linux-user.mak
+++ b/configs/targets/mipsel-linux-user.mak
@@ -2,4 +2,3 @@ TARGET_ARCH=mips
 TARGET_ABI_MIPSO32=y
 TARGET_SYSTBL_ABI=o32
 TARGET_SYSTBL=syscall_o32.tbl
-TARGET_ALIGNED_ONLY=y
diff --git a/configs/targets/mipsel-softmmu.mak 
b/configs/targets/mipsel-softmmu.mak
index c7c41f4fb7..0829659fc2 100644
--- a/configs/targets/mipsel-softmmu.mak
+++ b/configs/targets/mipsel-softmmu.mak
@@ -1,3 +1,2 @@
 TARGET_ARCH=mips
-TARGET_ALIGNED_ONLY=y
 TARGET_SUPPORTS_MTTCG=y
diff --git a/configs/targets/mipsn32-linux-user.mak 
b/configs/targets/mipsn32-linux-user.mak
index 1e80b302fc..206095da64 100644
--- a/configs/targets/mipsn32-linux-user.mak
+++ b/configs/targets/mipsn32-linux-user.mak
@@ -4,5 +4,4 @@ TARGET_ABI32=y
 TARGET_BASE_ARCH=mips
 TARGET_SYSTBL_ABI=n32
 TARGET_SYSTBL=syscall_n32.tbl
-TARGET_ALIGNED_ONLY=y
 TARGET_BIG_ENDIAN=y
diff --git a/configs/targets/mipsn32el-linux-user.mak 
b/configs/targets/mipsn32el-linux-user.mak
index f31a9c394b..ca2a3ed753 100644
--- a/configs/targets/mipsn32el-linux-user.mak
+++ b/configs/targets/mipsn32el-linux-user.mak
@@ -4,4 +4,3 @@ TARGET_ABI32=y
 TARGET_BASE_ARCH=mips
 TARGET_SYSTBL_ABI=n32
 TARGET_SYSTBL=syscall_n32.tbl
-TARGET_ALIGNED_ONLY=y
-- 
2.34.1

[PATCH 00/16] tcg: Remove TARGET_ALIGNED_ONLY

[Richard Henderson]

Based-on: 20230502135741.1158035-1-richard.hender...@linaro.org
("[PATCH 0/9] tcg: Remove compatability helpers for qemu ld/st")

Add MO_ALIGN where required, so that we may remove TARGET_ALIGNED_ONLY.
This is required for building tcg once, because we cannot have multiple
definitions of MO_ALIGN and MO_UNALN.


r~


Richard Henderson (16):
  target/alpha: Use MO_ALIGN for system UNALIGN()
  target/alpha: Use MO_ALIGN where required
  target/alpha: Remove TARGET_ALIGNED_ONLY
  target/hppa: Use MO_ALIGN for system UNALIGN()
  target/hppa: Remove TARGET_ALIGNED_ONLY
  target/mips: Add MO_ALIGN to gen_llwp, gen_scwp
  target/mips: Add missing default_tcg_memop_mask
  target/mips: Use MO_ALIGN instead of 0
  target/mips: Remove TARGET_ALIGNED_ONLY
  target/nios2: Remove TARGET_ALIGNED_ONLY
  target/sh4: Use MO_ALIGN where required
  target/sh4: Remove TARGET_ALIGNED_ONLY
  target/sparc: Use MO_ALIGN where required
  target/sparc: Use cpu_ld*_code_mmu
  target/sparc: Remove TARGET_ALIGNED_ONLY
  tcg: Remove TARGET_ALIGNED_ONLY

 configs/targets/alpha-linux-user.mak   |   1 -
 configs/targets/alpha-softmmu.mak  |   1 -
 configs/targets/hppa-linux-user.mak|   1 -
 configs/targets/hppa-softmmu.mak   |   1 -
 configs/targets/mips-linux-user.mak|   1 -
 configs/targets/mips-softmmu.mak   |   1 -
 configs/targets/mips64-linux-user.mak  |   1 -
 configs/targets/mips64-softmmu.mak |   1 -
 configs/targets/mips64el-linux-user.mak|   1 -
 configs/targets/mips64el-softmmu.mak   |   1 -
 configs/targets/mipsel-linux-user.mak  |   1 -
 configs/targets/mipsel-softmmu.mak |   1 -
 configs/targets/mipsn32-linux-user.mak |   1 -
 configs/targets/mipsn32el-linux-user.mak   |   1 -
 configs/targets/nios2-softmmu.mak  |   1 -
 configs/targets/sh4-linux-user.mak |   1 -
 configs/targets/sh4-softmmu.mak|   1 -
 configs/targets/sh4eb-linux-user.mak   |   1 -
 configs/targets/sh4eb-softmmu.mak  |   1 -
 configs/targets/sparc-linux-user.mak   |   1 -
 configs/targets/sparc-softmmu.mak  |   1 -
 configs/targets/sparc32plus-linux-user.mak |   1 -
 configs/targets/sparc64-linux-user.mak |   1 -
 configs/targets/sparc64-softmmu.mak|   1 -
 include/exec/memop.h   |  13 +--
 include/exec/poison.h  |   1 -
 target/alpha/translate.c   |  38 
 target/hppa/translate.c|   2 +-
 target/mips/tcg/mxu_translate.c|   3 +-
 target/nios2/translate.c   |  10 ++
 target/sh4/translate.c | 102 +
 target/sparc/ldst_helper.c |  10 +-
 target/sparc/translate.c   |  66 ++---
 tcg/tcg.c  |   5 -
 target/mips/tcg/micromips_translate.c.inc  |  24 +++--
 target/mips/tcg/mips16e_translate.c.inc|  18 ++--
 target/mips/tcg/nanomips_translate.c.inc   |  32 +++
 37 files changed, 186 insertions(+), 162 deletions(-)

-- 
2.34.1

[PATCH 05/16] target/hppa: Remove TARGET_ALIGNED_ONLY

[Richard Henderson]

Signed-off-by: Richard Henderson 
---
 configs/targets/hppa-linux-user.mak | 1 -
 configs/targets/hppa-softmmu.mak| 1 -
 2 files changed, 2 deletions(-)

diff --git a/configs/targets/hppa-linux-user.mak 
b/configs/targets/hppa-linux-user.mak
index db873a8796..361ea39d71 100644
--- a/configs/targets/hppa-linux-user.mak
+++ b/configs/targets/hppa-linux-user.mak
@@ -1,5 +1,4 @@
 TARGET_ARCH=hppa
 TARGET_SYSTBL_ABI=common,32
 TARGET_SYSTBL=syscall.tbl
-TARGET_ALIGNED_ONLY=y
 TARGET_BIG_ENDIAN=y
diff --git a/configs/targets/hppa-softmmu.mak b/configs/targets/hppa-softmmu.mak
index 44f07b0332..a41662aa99 100644
--- a/configs/targets/hppa-softmmu.mak
+++ b/configs/targets/hppa-softmmu.mak
@@ -1,4 +1,3 @@
 TARGET_ARCH=hppa
-TARGET_ALIGNED_ONLY=y
 TARGET_BIG_ENDIAN=y
 TARGET_SUPPORTS_MTTCG=y
-- 
2.34.1

[PATCH 12/16] target/sh4: Remove TARGET_ALIGNED_ONLY

[Richard Henderson]

Signed-off-by: Richard Henderson 
---
 configs/targets/sh4-linux-user.mak   | 1 -
 configs/targets/sh4-softmmu.mak  | 1 -
 configs/targets/sh4eb-linux-user.mak | 1 -
 configs/targets/sh4eb-softmmu.mak| 1 -
 4 files changed, 4 deletions(-)

diff --git a/configs/targets/sh4-linux-user.mak 
b/configs/targets/sh4-linux-user.mak
index 0152d6621e..9908887566 100644
--- a/configs/targets/sh4-linux-user.mak
+++ b/configs/targets/sh4-linux-user.mak
@@ -1,5 +1,4 @@
 TARGET_ARCH=sh4
 TARGET_SYSTBL_ABI=common
 TARGET_SYSTBL=syscall.tbl
-TARGET_ALIGNED_ONLY=y
 TARGET_HAS_BFLT=y
diff --git a/configs/targets/sh4-softmmu.mak b/configs/targets/sh4-softmmu.mak
index 95896376c4..f9d62d91e4 100644
--- a/configs/targets/sh4-softmmu.mak
+++ b/configs/targets/sh4-softmmu.mak
@@ -1,2 +1 @@
 TARGET_ARCH=sh4
-TARGET_ALIGNED_ONLY=y
diff --git a/configs/targets/sh4eb-linux-user.mak 
b/configs/targets/sh4eb-linux-user.mak
index 6724165efe..9db6b3609c 100644
--- a/configs/targets/sh4eb-linux-user.mak
+++ b/configs/targets/sh4eb-linux-user.mak
@@ -1,6 +1,5 @@
 TARGET_ARCH=sh4
 TARGET_SYSTBL_ABI=common
 TARGET_SYSTBL=syscall.tbl
-TARGET_ALIGNED_ONLY=y
 TARGET_BIG_ENDIAN=y
 TARGET_HAS_BFLT=y
diff --git a/configs/targets/sh4eb-softmmu.mak 
b/configs/targets/sh4eb-softmmu.mak
index dc8b30bf7a..226b1fc698 100644
--- a/configs/targets/sh4eb-softmmu.mak
+++ b/configs/targets/sh4eb-softmmu.mak
@@ -1,3 +1,2 @@
 TARGET_ARCH=sh4
-TARGET_ALIGNED_ONLY=y
 TARGET_BIG_ENDIAN=y
-- 
2.34.1

[PATCH 06/16] target/mips: Add MO_ALIGN to gen_llwp, gen_scwp

[Richard Henderson]

These are atomic operations, so mark as requiring alignment.

Signed-off-by: Richard Henderson 
---
 target/mips/tcg/nanomips_translate.c.inc | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/mips/tcg/nanomips_translate.c.inc 
b/target/mips/tcg/nanomips_translate.c.inc
index 97b9572caa..e08343414c 100644
--- a/target/mips/tcg/nanomips_translate.c.inc
+++ b/target/mips/tcg/nanomips_translate.c.inc
@@ -998,7 +998,7 @@ static void gen_llwp(DisasContext *ctx, uint32_t base, 
int16_t offset,
 TCGv tmp2 = tcg_temp_new();
 
 gen_base_offset_addr(ctx, taddr, base, offset);
-tcg_gen_qemu_ld_i64(tval, taddr, ctx->mem_idx, MO_TEUQ);
+tcg_gen_qemu_ld_i64(tval, taddr, ctx->mem_idx, MO_TEUQ | MO_ALIGN);
 if (cpu_is_bigendian(ctx)) {
 tcg_gen_extr_i64_tl(tmp2, tmp1, tval);
 } else {
@@ -1039,7 +1039,8 @@ static void gen_scwp(DisasContext *ctx, uint32_t base, 
int16_t offset,
 
 tcg_gen_ld_i64(llval, cpu_env, offsetof(CPUMIPSState, llval_wp));
 tcg_gen_atomic_cmpxchg_i64(val, taddr, llval, tval,
-   eva ? MIPS_HFLAG_UM : ctx->mem_idx, MO_64);
+   eva ? MIPS_HFLAG_UM : ctx->mem_idx,
+   MO_64 | MO_ALIGN);
 if (reg1 != 0) {
 tcg_gen_movi_tl(cpu_gpr[reg1], 1);
 }
-- 
2.34.1

[PATCH 14/16] target/sparc: Use cpu_ld*_code_mmu

[Richard Henderson]

This passes on the memop as given as argument to
helper_ld_asi to the ultimate load primitive.

Signed-off-by: Richard Henderson 
---
 target/sparc/ldst_helper.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c
index a53580d9e4..7972d56a72 100644
--- a/target/sparc/ldst_helper.c
+++ b/target/sparc/ldst_helper.c
@@ -593,6 +593,7 @@ uint64_t helper_ld_asi(CPUSPARCState *env, target_ulong 
addr,
 #if defined(DEBUG_MXCC) || defined(DEBUG_ASI)
 uint32_t last_addr = addr;
 #endif
+MemOpIdx oi;
 
 do_check_align(env, addr, size - 1, GETPC());
 switch (asi) {
@@ -692,19 +693,20 @@ uint64_t helper_ld_asi(CPUSPARCState *env, target_ulong 
addr,
 case ASI_M_IODIAG:  /* Turbosparc IOTLB Diagnostic */
 break;
 case ASI_KERNELTXT: /* Supervisor code access */
+oi = make_memop_idx(memop, cpu_mmu_index(env, true));
 switch (size) {
 case 1:
-ret = cpu_ldub_code(env, addr);
+ret = cpu_ldb_code_mmu(env, addr, oi, GETPC());
 break;
 case 2:
-ret = cpu_lduw_code(env, addr);
+ret = cpu_ldw_code_mmu(env, addr, oi, GETPC());
 break;
 default:
 case 4:
-ret = cpu_ldl_code(env, addr);
+ret = cpu_ldl_code_mmu(env, addr, oi, GETPC());
 break;
 case 8:
-ret = cpu_ldq_code(env, addr);
+ret = cpu_ldq_code_mmu(env, addr, oi, GETPC());
 break;
 }
 break;
-- 
2.34.1

[PATCH 11/16] target/sh4: Use MO_ALIGN where required

[Richard Henderson]

Mark all memory operations that are not already marked with UNALIGN.

Signed-off-by: Richard Henderson 
---
 target/sh4/translate.c | 102 ++---
 1 file changed, 66 insertions(+), 36 deletions(-)

diff --git a/target/sh4/translate.c b/target/sh4/translate.c
index 6e40d5dd6a..0dedbb8210 100644
--- a/target/sh4/translate.c
+++ b/target/sh4/translate.c
@@ -527,13 +527,15 @@ static void _decode_opc(DisasContext * ctx)
 case 0x9000:   /* mov.w @(disp,PC),Rn */
{
 TCGv addr = tcg_constant_i32(ctx->base.pc_next + 4 + B7_0 * 2);
-tcg_gen_qemu_ld_i32(REG(B11_8), addr, ctx->memidx, MO_TESW);
+tcg_gen_qemu_ld_i32(REG(B11_8), addr, ctx->memidx,
+MO_TESW | MO_ALIGN);
}
return;
 case 0xd000:   /* mov.l @(disp,PC),Rn */
{
 TCGv addr = tcg_constant_i32((ctx->base.pc_next + 4 + B7_0 * 4) & 
~3);
-tcg_gen_qemu_ld_i32(REG(B11_8), addr, ctx->memidx, MO_TESL);
+tcg_gen_qemu_ld_i32(REG(B11_8), addr, ctx->memidx,
+MO_TESL | MO_ALIGN);
}
return;
 case 0x7000:   /* add #imm,Rn */
@@ -801,9 +803,11 @@ static void _decode_opc(DisasContext * ctx)
{
TCGv arg0, arg1;
arg0 = tcg_temp_new();
-tcg_gen_qemu_ld_i32(arg0, REG(B7_4), ctx->memidx, MO_TESL);
+tcg_gen_qemu_ld_i32(arg0, REG(B7_4), ctx->memidx,
+MO_TESL | MO_ALIGN);
arg1 = tcg_temp_new();
-tcg_gen_qemu_ld_i32(arg1, REG(B11_8), ctx->memidx, MO_TESL);
+tcg_gen_qemu_ld_i32(arg1, REG(B11_8), ctx->memidx,
+MO_TESL | MO_ALIGN);
 gen_helper_macl(cpu_env, arg0, arg1);
tcg_gen_addi_i32(REG(B7_4), REG(B7_4), 4);
tcg_gen_addi_i32(REG(B11_8), REG(B11_8), 4);
@@ -813,9 +817,11 @@ static void _decode_opc(DisasContext * ctx)
{
TCGv arg0, arg1;
arg0 = tcg_temp_new();
-tcg_gen_qemu_ld_i32(arg0, REG(B7_4), ctx->memidx, MO_TESL);
+tcg_gen_qemu_ld_i32(arg0, REG(B7_4), ctx->memidx,
+MO_TESL | MO_ALIGN);
arg1 = tcg_temp_new();
-tcg_gen_qemu_ld_i32(arg1, REG(B11_8), ctx->memidx, MO_TESL);
+tcg_gen_qemu_ld_i32(arg1, REG(B11_8), ctx->memidx,
+MO_TESL | MO_ALIGN);
 gen_helper_macw(cpu_env, arg0, arg1);
tcg_gen_addi_i32(REG(B11_8), REG(B11_8), 2);
tcg_gen_addi_i32(REG(B7_4), REG(B7_4), 2);
@@ -961,30 +967,36 @@ static void _decode_opc(DisasContext * ctx)
 if (ctx->tbflags & FPSCR_SZ) {
 TCGv_i64 fp = tcg_temp_new_i64();
 gen_load_fpr64(ctx, fp, XHACK(B7_4));
-tcg_gen_qemu_st_i64(fp, REG(B11_8), ctx->memidx, MO_TEUQ);
+tcg_gen_qemu_st_i64(fp, REG(B11_8), ctx->memidx,
+MO_TEUQ | MO_ALIGN);
} else {
-tcg_gen_qemu_st_i32(FREG(B7_4), REG(B11_8), ctx->memidx, MO_TEUL);
+tcg_gen_qemu_st_i32(FREG(B7_4), REG(B11_8), ctx->memidx,
+MO_TEUL | MO_ALIGN);
}
return;
 case 0xf008: /* fmov @Rm,{F,D,X}Rn - FPSCR: Nothing */
CHECK_FPU_ENABLED
 if (ctx->tbflags & FPSCR_SZ) {
 TCGv_i64 fp = tcg_temp_new_i64();
-tcg_gen_qemu_ld_i64(fp, REG(B7_4), ctx->memidx, MO_TEUQ);
+tcg_gen_qemu_ld_i64(fp, REG(B7_4), ctx->memidx,
+MO_TEUQ | MO_ALIGN);
 gen_store_fpr64(ctx, fp, XHACK(B11_8));
} else {
-tcg_gen_qemu_ld_i32(FREG(B11_8), REG(B7_4), ctx->memidx, MO_TEUL);
+tcg_gen_qemu_ld_i32(FREG(B11_8), REG(B7_4), ctx->memidx,
+MO_TEUL | MO_ALIGN);
}
return;
 case 0xf009: /* fmov @Rm+,{F,D,X}Rn - FPSCR: Nothing */
CHECK_FPU_ENABLED
 if (ctx->tbflags & FPSCR_SZ) {
 TCGv_i64 fp = tcg_temp_new_i64();
-tcg_gen_qemu_ld_i64(fp, REG(B7_4), ctx->memidx, MO_TEUQ);
+tcg_gen_qemu_ld_i64(fp, REG(B7_4), ctx->memidx,
+MO_TEUQ | MO_ALIGN);
 gen_store_fpr64(ctx, fp, XHACK(B11_8));
 tcg_gen_addi_i32(REG(B7_4), REG(B7_4), 8);
} else {
-tcg_gen_qemu_ld_i32(FREG(B11_8), REG(B7_4), ctx->memidx, MO_TEUL);
+tcg_gen_qemu_ld_i32(FREG(B11_8), REG(B7_4), ctx->memidx,
+MO_TEUL | MO_ALIGN);
tcg_gen_addi_i32(REG(B7_4), REG(B7_4), 4);
}
return;
@@ -996,10 +1008,12 @@ static void _decode_opc(DisasContext * ctx)
 TCGv_i64 fp = tcg_temp_new_i64();
 gen_load_fpr64(ctx, fp, XHACK(B7_4));
 tcg_gen_subi_i32(addr, REG(B11_8), 8);
-

[PATCH 13/16] target/sparc: Use MO_ALIGN where required

[Richard Henderson]

Signed-off-by: Richard Henderson 
---
 target/sparc/translate.c | 66 +---
 1 file changed, 34 insertions(+), 32 deletions(-)

diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index bc71e44e66..414e014b11 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -1899,7 +1899,7 @@ static void gen_swap(DisasContext *dc, TCGv dst, TCGv src,
  TCGv addr, int mmu_idx, MemOp memop)
 {
 gen_address_mask(dc, addr);
-tcg_gen_atomic_xchg_tl(dst, addr, src, mmu_idx, memop);
+tcg_gen_atomic_xchg_tl(dst, addr, src, mmu_idx, memop | MO_ALIGN);
 }
 
 static void gen_ldstub(DisasContext *dc, TCGv dst, TCGv addr, int mmu_idx)
@@ -2155,12 +2155,12 @@ static void gen_ld_asi(DisasContext *dc, TCGv dst, TCGv 
addr,
 break;
 case GET_ASI_DIRECT:
 gen_address_mask(dc, addr);
-tcg_gen_qemu_ld_tl(dst, addr, da.mem_idx, da.memop);
+tcg_gen_qemu_ld_tl(dst, addr, da.mem_idx, da.memop | MO_ALIGN);
 break;
 default:
 {
 TCGv_i32 r_asi = tcg_constant_i32(da.asi);
-TCGv_i32 r_mop = tcg_constant_i32(memop);
+TCGv_i32 r_mop = tcg_constant_i32(memop | MO_ALIGN);
 
 save_state(dc);
 #ifdef TARGET_SPARC64
@@ -2201,7 +2201,7 @@ static void gen_st_asi(DisasContext *dc, TCGv src, TCGv 
addr,
 /* fall through */
 case GET_ASI_DIRECT:
 gen_address_mask(dc, addr);
-tcg_gen_qemu_st_tl(src, addr, da.mem_idx, da.memop);
+tcg_gen_qemu_st_tl(src, addr, da.mem_idx, da.memop | MO_ALIGN);
 break;
 #if !defined(TARGET_SPARC64) && !defined(CONFIG_USER_ONLY)
 case GET_ASI_BCOPY:
@@ -2233,7 +2233,7 @@ static void gen_st_asi(DisasContext *dc, TCGv src, TCGv 
addr,
 default:
 {
 TCGv_i32 r_asi = tcg_constant_i32(da.asi);
-TCGv_i32 r_mop = tcg_constant_i32(memop & MO_SIZE);
+TCGv_i32 r_mop = tcg_constant_i32(memop | MO_ALIGN);
 
 save_state(dc);
 #ifdef TARGET_SPARC64
@@ -2283,7 +2283,7 @@ static void gen_cas_asi(DisasContext *dc, TCGv addr, TCGv 
cmpv,
 case GET_ASI_DIRECT:
 oldv = tcg_temp_new();
 tcg_gen_atomic_cmpxchg_tl(oldv, addr, cmpv, gen_load_gpr(dc, rd),
-  da.mem_idx, da.memop);
+  da.mem_idx, da.memop | MO_ALIGN);
 gen_store_gpr(dc, rd, oldv);
 break;
 default:
@@ -2347,7 +2347,7 @@ static void gen_ldf_asi(DisasContext *dc, TCGv addr,
 switch (size) {
 case 4:
 d32 = gen_dest_fpr_F(dc);
-tcg_gen_qemu_ld_i32(d32, addr, da.mem_idx, da.memop);
+tcg_gen_qemu_ld_i32(d32, addr, da.mem_idx, da.memop | MO_ALIGN);
 gen_store_fpr_F(dc, rd, d32);
 break;
 case 8:
@@ -2397,7 +2397,8 @@ static void gen_ldf_asi(DisasContext *dc, TCGv addr,
 /* Valid for lddfa only.  */
 if (size == 8) {
 gen_address_mask(dc, addr);
-tcg_gen_qemu_ld_i64(cpu_fpr[rd / 2], addr, da.mem_idx, da.memop);
+tcg_gen_qemu_ld_i64(cpu_fpr[rd / 2], addr, da.mem_idx,
+da.memop | MO_ALIGN);
 } else {
 gen_exception(dc, TT_ILL_INSN);
 }
@@ -2406,7 +2407,7 @@ static void gen_ldf_asi(DisasContext *dc, TCGv addr,
 default:
 {
 TCGv_i32 r_asi = tcg_constant_i32(da.asi);
-TCGv_i32 r_mop = tcg_constant_i32(da.memop);
+TCGv_i32 r_mop = tcg_constant_i32(da.memop | MO_ALIGN);
 
 save_state(dc);
 /* According to the table in the UA2011 manual, the only
@@ -2454,7 +2455,7 @@ static void gen_stf_asi(DisasContext *dc, TCGv addr,
 switch (size) {
 case 4:
 d32 = gen_load_fpr_F(dc, rd);
-tcg_gen_qemu_st_i32(d32, addr, da.mem_idx, da.memop);
+tcg_gen_qemu_st_i32(d32, addr, da.mem_idx, da.memop | MO_ALIGN);
 break;
 case 8:
 tcg_gen_qemu_st_i64(cpu_fpr[rd / 2], addr, da.mem_idx,
@@ -2506,7 +2507,8 @@ static void gen_stf_asi(DisasContext *dc, TCGv addr,
 /* Valid for stdfa only.  */
 if (size == 8) {
 gen_address_mask(dc, addr);
-tcg_gen_qemu_st_i64(cpu_fpr[rd / 2], addr, da.mem_idx, da.memop);
+tcg_gen_qemu_st_i64(cpu_fpr[rd / 2], addr, da.mem_idx,
+da.memop | MO_ALIGN);
 } else {
 gen_exception(dc, TT_ILL_INSN);
 }
@@ -2543,7 +2545,7 @@ static void gen_ldda_asi(DisasContext *dc, TCGv addr, int 
insn, int rd)
 TCGv_i64 tmp = tcg_temp_new_i64();
 
 gen_address_mask(dc, addr);
-tcg_gen_qemu_ld_i64(tmp, addr, da.mem_idx, da.memop);
+tcg_gen_qemu_ld_i64(tmp, addr, da.mem_idx, da.memop | MO_ALIGN);
 
 /* Note that LE ldda acts as if each 32-bit register
result is byte swapped.  

[PATCH 07/16] target/mips: Add missing default_tcg_memop_mask

[Richard Henderson]

Memory operations that are not already aligned, or otherwise
marked up, require addition of ctx->default_tcg_memop_mask.

Signed-off-by: Richard Henderson 
---
 target/mips/tcg/mxu_translate.c   |  3 ++-
 target/mips/tcg/micromips_translate.c.inc | 24 ++
 target/mips/tcg/mips16e_translate.c.inc   | 18 ++--
 target/mips/tcg/nanomips_translate.c.inc  | 25 +++
 4 files changed, 42 insertions(+), 28 deletions(-)

diff --git a/target/mips/tcg/mxu_translate.c b/target/mips/tcg/mxu_translate.c
index bdd20709c0..be038b5f07 100644
--- a/target/mips/tcg/mxu_translate.c
+++ b/target/mips/tcg/mxu_translate.c
@@ -831,7 +831,8 @@ static void gen_mxu_s32ldd_s32lddr(DisasContext *ctx)
 tcg_gen_ori_tl(t1, t1, 0xF000);
 }
 tcg_gen_add_tl(t1, t0, t1);
-tcg_gen_qemu_ld_tl(t1, t1, ctx->mem_idx, MO_TESL ^ (sel * MO_BSWAP));
+tcg_gen_qemu_ld_tl(t1, t1, ctx->mem_idx, (MO_TESL ^ (sel * MO_BSWAP)) |
+   ctx->default_tcg_memop_mask);
 
 gen_store_mxu_gpr(t1, XRa);
 }
diff --git a/target/mips/tcg/micromips_translate.c.inc 
b/target/mips/tcg/micromips_translate.c.inc
index e8b193aeda..211d102cf6 100644
--- a/target/mips/tcg/micromips_translate.c.inc
+++ b/target/mips/tcg/micromips_translate.c.inc
@@ -977,20 +977,24 @@ static void gen_ldst_pair(DisasContext *ctx, uint32_t 
opc, int rd,
 gen_reserved_instruction(ctx);
 return;
 }
-tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_TESL);
+tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_TESL |
+   ctx->default_tcg_memop_mask);
 gen_store_gpr(t1, rd);
 tcg_gen_movi_tl(t1, 4);
 gen_op_addr_add(ctx, t0, t0, t1);
-tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_TESL);
+tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_TESL |
+   ctx->default_tcg_memop_mask);
 gen_store_gpr(t1, rd + 1);
 break;
 case SWP:
 gen_load_gpr(t1, rd);
-tcg_gen_qemu_st_tl(t1, t0, ctx->mem_idx, MO_TEUL);
+tcg_gen_qemu_st_tl(t1, t0, ctx->mem_idx, MO_TEUL |
+   ctx->default_tcg_memop_mask);
 tcg_gen_movi_tl(t1, 4);
 gen_op_addr_add(ctx, t0, t0, t1);
 gen_load_gpr(t1, rd + 1);
-tcg_gen_qemu_st_tl(t1, t0, ctx->mem_idx, MO_TEUL);
+tcg_gen_qemu_st_tl(t1, t0, ctx->mem_idx, MO_TEUL |
+   ctx->default_tcg_memop_mask);
 break;
 #ifdef TARGET_MIPS64
 case LDP:
@@ -998,20 +1002,24 @@ static void gen_ldst_pair(DisasContext *ctx, uint32_t 
opc, int rd,
 gen_reserved_instruction(ctx);
 return;
 }
-tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_TEUQ);
+tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_TEUQ |
+   ctx->default_tcg_memop_mask);
 gen_store_gpr(t1, rd);
 tcg_gen_movi_tl(t1, 8);
 gen_op_addr_add(ctx, t0, t0, t1);
-tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_TEUQ);
+tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_TEUQ |
+   ctx->default_tcg_memop_mask);
 gen_store_gpr(t1, rd + 1);
 break;
 case SDP:
 gen_load_gpr(t1, rd);
-tcg_gen_qemu_st_tl(t1, t0, ctx->mem_idx, MO_TEUQ);
+tcg_gen_qemu_st_tl(t1, t0, ctx->mem_idx, MO_TEUQ |
+   ctx->default_tcg_memop_mask);
 tcg_gen_movi_tl(t1, 8);
 gen_op_addr_add(ctx, t0, t0, t1);
 gen_load_gpr(t1, rd + 1);
-tcg_gen_qemu_st_tl(t1, t0, ctx->mem_idx, MO_TEUQ);
+tcg_gen_qemu_st_tl(t1, t0, ctx->mem_idx, MO_TEUQ |
+   ctx->default_tcg_memop_mask);
 break;
 #endif
 }
diff --git a/target/mips/tcg/mips16e_translate.c.inc 
b/target/mips/tcg/mips16e_translate.c.inc
index 602f5f0c02..5cffe0e412 100644
--- a/target/mips/tcg/mips16e_translate.c.inc
+++ b/target/mips/tcg/mips16e_translate.c.inc
@@ -172,22 +172,26 @@ static void gen_mips16_save(DisasContext *ctx,
 case 4:
 gen_base_offset_addr(ctx, t0, 29, 12);
 gen_load_gpr(t1, 7);
-tcg_gen_qemu_st_tl(t1, t0, ctx->mem_idx, MO_TEUL);
+tcg_gen_qemu_st_tl(t1, t0, ctx->mem_idx, MO_TEUL |
+   ctx->default_tcg_memop_mask);
 /* Fall through */
 case 3:
 gen_base_offset_addr(ctx, t0, 29, 8);
 gen_load_gpr(t1, 6);
-tcg_gen_qemu_st_tl(t1, t0, ctx->mem_idx, MO_TEUL);
+tcg_gen_qemu_st_tl(t1, t0, ctx->mem_idx, MO_TEUL |
+   ctx->default_tcg_memop_mask);
 /* Fall through */
 case 2:
 gen_base_offset_addr(ctx, t0, 29, 4);
 gen_load_gpr(t1, 5);
-tcg_gen_qemu_st_tl(t1, t0, ctx->mem_idx, MO_TEUL);
+tcg_gen_qemu_st_tl(t1, t0, ctx->mem_idx, MO_TEUL |
+   ctx->default_tcg_memop_mask);
 /* Fall through */
 case 1:
 

[PATCH 16/16] tcg: Remove TARGET_ALIGNED_ONLY

[Richard Henderson]

All uses have now been expunged.

Signed-off-by: Richard Henderson 
---
 include/exec/memop.h  | 13 ++---
 include/exec/poison.h |  1 -
 tcg/tcg.c |  5 -
 3 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/include/exec/memop.h b/include/exec/memop.h
index 25d027434a..07f5f88188 100644
--- a/include/exec/memop.h
+++ b/include/exec/memop.h
@@ -47,8 +47,6 @@ typedef enum MemOp {
  * MO_UNALN accesses are never checked for alignment.
  * MO_ALIGN accesses will result in a call to the CPU's
  * do_unaligned_access hook if the guest address is not aligned.
- * The default depends on whether the target CPU defines
- * TARGET_ALIGNED_ONLY.
  *
  * Some architectures (e.g. ARMv8) need the address which is aligned
  * to a size more than the size of the memory access.
@@ -65,21 +63,14 @@ typedef enum MemOp {
  */
 MO_ASHIFT = 5,
 MO_AMASK = 0x7 << MO_ASHIFT,
-#ifdef NEED_CPU_H
-#ifdef TARGET_ALIGNED_ONLY
-MO_ALIGN = 0,
-MO_UNALN = MO_AMASK,
-#else
-MO_ALIGN = MO_AMASK,
-MO_UNALN = 0,
-#endif
-#endif
+MO_UNALN= 0,
 MO_ALIGN_2  = 1 << MO_ASHIFT,
 MO_ALIGN_4  = 2 << MO_ASHIFT,
 MO_ALIGN_8  = 3 << MO_ASHIFT,
 MO_ALIGN_16 = 4 << MO_ASHIFT,
 MO_ALIGN_32 = 5 << MO_ASHIFT,
 MO_ALIGN_64 = 6 << MO_ASHIFT,
+MO_ALIGN= MO_AMASK,
 
 /* Combinations of the above, for ease of use.  */
 MO_UB= MO_8,
diff --git a/include/exec/poison.h b/include/exec/poison.h
index 140daa4a85..256736e11a 100644
--- a/include/exec/poison.h
+++ b/include/exec/poison.h
@@ -35,7 +35,6 @@
 #pragma GCC poison TARGET_TRICORE
 #pragma GCC poison TARGET_XTENSA
 
-#pragma GCC poison TARGET_ALIGNED_ONLY
 #pragma GCC poison TARGET_HAS_BFLT
 #pragma GCC poison TARGET_NAME
 #pragma GCC poison TARGET_SUPPORTS_MTTCG
diff --git a/tcg/tcg.c b/tcg/tcg.c
index cfd3262a4a..2ce9dba25c 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2071,13 +2071,8 @@ static const char * const ldst_name[] =
 };
 
 static const char * const alignment_name[(MO_AMASK >> MO_ASHIFT) + 1] = {
-#ifdef TARGET_ALIGNED_ONLY
-[MO_UNALN >> MO_ASHIFT]= "un+",
-[MO_ALIGN >> MO_ASHIFT]= "",
-#else
 [MO_UNALN >> MO_ASHIFT]= "",
 [MO_ALIGN >> MO_ASHIFT]= "al+",
-#endif
 [MO_ALIGN_2 >> MO_ASHIFT]  = "al2+",
 [MO_ALIGN_4 >> MO_ASHIFT]  = "al4+",
 [MO_ALIGN_8 >> MO_ASHIFT]  = "al8+",
-- 
2.34.1

[PATCH 10/16] target/nios2: Remove TARGET_ALIGNED_ONLY

[Richard Henderson]

In gen_ldx/gen_stx, the only two locations for memory operations,
mark the operation as either aligned (softmmu) or unaligned
(user-only, as if emulated by the kernel).

Signed-off-by: Richard Henderson 
---
 configs/targets/nios2-softmmu.mak |  1 -
 target/nios2/translate.c  | 10 ++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/configs/targets/nios2-softmmu.mak 
b/configs/targets/nios2-softmmu.mak
index 5823fc02c8..c99ae3777e 100644
--- a/configs/targets/nios2-softmmu.mak
+++ b/configs/targets/nios2-softmmu.mak
@@ -1,3 +1,2 @@
 TARGET_ARCH=nios2
-TARGET_ALIGNED_ONLY=y
 TARGET_NEED_FDT=y
diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index 6610e22236..a548e16ed5 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -298,6 +298,11 @@ static void gen_ldx(DisasContext *dc, uint32_t code, 
uint32_t flags)
 TCGv data = dest_gpr(dc, instr.b);
 
 tcg_gen_addi_tl(addr, load_gpr(dc, instr.a), instr.imm16.s);
+#ifdef CONFIG_USER_ONLY
+flags |= MO_UNALN;
+#else
+flags |= MO_ALIGN;
+#endif
 tcg_gen_qemu_ld_tl(data, addr, dc->mem_idx, flags);
 }
 
@@ -309,6 +314,11 @@ static void gen_stx(DisasContext *dc, uint32_t code, 
uint32_t flags)
 
 TCGv addr = tcg_temp_new();
 tcg_gen_addi_tl(addr, load_gpr(dc, instr.a), instr.imm16.s);
+#ifdef CONFIG_USER_ONLY
+flags |= MO_UNALN;
+#else
+flags |= MO_ALIGN;
+#endif
 tcg_gen_qemu_st_tl(val, addr, dc->mem_idx, flags);
 }
 
-- 
2.34.1

[PATCH 02/16] target/alpha: Use MO_ALIGN where required

[Richard Henderson]

Mark all memory operations that are not already marked with UNALIGN.

Signed-off-by: Richard Henderson 
---
 target/alpha/translate.c | 36 
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/target/alpha/translate.c b/target/alpha/translate.c
index ffbac1c114..be8adb2526 100644
--- a/target/alpha/translate.c
+++ b/target/alpha/translate.c
@@ -2399,21 +2399,21 @@ static DisasJumpType translate_one(DisasContext *ctx, 
uint32_t insn)
 switch ((insn >> 12) & 0xF) {
 case 0x0:
 /* Longword physical access (hw_ldl/p) */
-tcg_gen_qemu_ld_i64(va, addr, MMU_PHYS_IDX, MO_LESL);
+tcg_gen_qemu_ld_i64(va, addr, MMU_PHYS_IDX, MO_LESL | 
MO_ALIGN);
 break;
 case 0x1:
 /* Quadword physical access (hw_ldq/p) */
-tcg_gen_qemu_ld_i64(va, addr, MMU_PHYS_IDX, MO_LEUQ);
+tcg_gen_qemu_ld_i64(va, addr, MMU_PHYS_IDX, MO_LEUQ | 
MO_ALIGN);
 break;
 case 0x2:
 /* Longword physical access with lock (hw_ldl_l/p) */
-tcg_gen_qemu_ld_i64(va, addr, MMU_PHYS_IDX, MO_LESL);
+tcg_gen_qemu_ld_i64(va, addr, MMU_PHYS_IDX, MO_LESL | 
MO_ALIGN);
 tcg_gen_mov_i64(cpu_lock_addr, addr);
 tcg_gen_mov_i64(cpu_lock_value, va);
 break;
 case 0x3:
 /* Quadword physical access with lock (hw_ldq_l/p) */
-tcg_gen_qemu_ld_i64(va, addr, MMU_PHYS_IDX, MO_LEUQ);
+tcg_gen_qemu_ld_i64(va, addr, MMU_PHYS_IDX, MO_LEUQ | 
MO_ALIGN);
 tcg_gen_mov_i64(cpu_lock_addr, addr);
 tcg_gen_mov_i64(cpu_lock_value, va);
 break;
@@ -2438,11 +2438,13 @@ static DisasJumpType translate_one(DisasContext *ctx, 
uint32_t insn)
 goto invalid_opc;
 case 0xA:
 /* Longword virtual access with protection check (hw_ldl/w) */
-tcg_gen_qemu_ld_i64(va, addr, MMU_KERNEL_IDX, MO_LESL);
+tcg_gen_qemu_ld_i64(va, addr, MMU_KERNEL_IDX,
+MO_LESL | MO_ALIGN);
 break;
 case 0xB:
 /* Quadword virtual access with protection check (hw_ldq/w) */
-tcg_gen_qemu_ld_i64(va, addr, MMU_KERNEL_IDX, MO_LEUQ);
+tcg_gen_qemu_ld_i64(va, addr, MMU_KERNEL_IDX,
+MO_LEUQ | MO_ALIGN);
 break;
 case 0xC:
 /* Longword virtual access with alt access mode (hw_ldl/a)*/
@@ -2453,12 +2455,14 @@ static DisasJumpType translate_one(DisasContext *ctx, 
uint32_t insn)
 case 0xE:
 /* Longword virtual access with alternate access mode and
protection checks (hw_ldl/wa) */
-tcg_gen_qemu_ld_i64(va, addr, MMU_USER_IDX, MO_LESL);
+tcg_gen_qemu_ld_i64(va, addr, MMU_USER_IDX,
+MO_LESL | MO_ALIGN);
 break;
 case 0xF:
 /* Quadword virtual access with alternate access mode and
protection checks (hw_ldq/wa) */
-tcg_gen_qemu_ld_i64(va, addr, MMU_USER_IDX, MO_LEUQ);
+tcg_gen_qemu_ld_i64(va, addr, MMU_USER_IDX,
+MO_LEUQ | MO_ALIGN);
 break;
 }
 break;
@@ -2659,7 +2663,7 @@ static DisasJumpType translate_one(DisasContext *ctx, 
uint32_t insn)
 vb = load_gpr(ctx, rb);
 tmp = tcg_temp_new();
 tcg_gen_addi_i64(tmp, vb, disp12);
-tcg_gen_qemu_st_i64(va, tmp, MMU_PHYS_IDX, MO_LESL);
+tcg_gen_qemu_st_i64(va, tmp, MMU_PHYS_IDX, MO_LESL | MO_ALIGN);
 break;
 case 0x1:
 /* Quadword physical access */
@@ -2667,17 +2671,17 @@ static DisasJumpType translate_one(DisasContext *ctx, 
uint32_t insn)
 vb = load_gpr(ctx, rb);
 tmp = tcg_temp_new();
 tcg_gen_addi_i64(tmp, vb, disp12);
-tcg_gen_qemu_st_i64(va, tmp, MMU_PHYS_IDX, MO_LEUQ);
+tcg_gen_qemu_st_i64(va, tmp, MMU_PHYS_IDX, MO_LEUQ | MO_ALIGN);
 break;
 case 0x2:
 /* Longword physical access with lock */
 ret = gen_store_conditional(ctx, ra, rb, disp12,
-MMU_PHYS_IDX, MO_LESL);
+MMU_PHYS_IDX, MO_LESL | MO_ALIGN);
 break;
 case 0x3:
 /* Quadword physical access with lock */
 ret = gen_store_conditional(ctx, ra, rb, disp12,
-MMU_PHYS_IDX, MO_LEUQ);
+

[PATCH 04/16] target/hppa: Use MO_ALIGN for system UNALIGN()

[Richard Henderson]

Signed-off-by: Richard Henderson 
---
 target/hppa/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 6a3154ebc6..59e4688bfa 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -271,7 +271,7 @@ typedef struct DisasContext {
 #ifdef CONFIG_USER_ONLY
 #define UNALIGN(C)  (C)->unalign
 #else
-#define UNALIGN(C)  0
+#define UNALIGN(C)  MO_ALIGN
 #endif
 
 /* Note that ssm/rsm instructions number PSW_W and PSW_E differently.  */
-- 
2.34.1

Re: [PATCH v11 00/13] target/arm: Allow CONFIG_TCG=n builds

[Peter Maydell]

On Tue, 2 May 2023 at 15:51, Peter Maydell  wrote:
>
> On Tue, 2 May 2023 at 10:55, Peter Maydell  wrote:
> >
> > On Wed, 26 Apr 2023 at 19:00, Fabiano Rosas  wrote:
> > >
> > > Hi,
> > >
> > > Some minor changes:
> > >
> > > - new patch to move a test under CONFIG_TCG (broken on master);
> > > - new patch to document the unsupported CPU test (Philippe);
> > > - changed the test skip message when no KVM or TCG are present (Igor).
> >
> > Applied to target-arm.next; thanks for your persistence in
> > working through the many versions of this patchset.
>
> Update: I had to drop "gitlab-ci: Check building KVM-only aarch64 target"
> because it enables a CI job that fails on our aarch64 runner
> (because it wants to run tests using KVM but that machine
> isn't configured to allow the runner to use KVM).

We fixed the runner config, but the CI still fails on that notcg
job because it is trying to run tests that explicitly use
'-accel tcg':
https://gitlab.com/qemu-project/qemu/-/jobs/4212850809#L3595

Something is weird here, because we built without TCG support
on an aarch64 host but we still got qemu-system-i386
and qemu-system-x86_64 binaries, which then don't work
and cause the tests to fail...

-- PMM

Re: [PATCH v4 07/20] block/export: stop using is_external in vhost-user-blk server

[Kevin Wolf]

Am 25.04.2023 um 19:27 hat Stefan Hajnoczi geschrieben:
> vhost-user activity must be suspended during bdrv_drained_begin/end().
> This prevents new requests from interfering with whatever is happening
> in the drained section.
> 
> Previously this was done using aio_set_fd_handler()'s is_external
> argument. In a multi-queue block layer world the aio_disable_external()
> API cannot be used since multiple AioContext may be processing I/O, not
> just one.
> 
> Switch to BlockDevOps->drained_begin/end() callbacks.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  block/export/vhost-user-blk-server.c | 43 ++--
>  util/vhost-user-server.c | 10 +++
>  2 files changed, 26 insertions(+), 27 deletions(-)
> 
> diff --git a/block/export/vhost-user-blk-server.c 
> b/block/export/vhost-user-blk-server.c
> index 092b86aae4..d20f69cd74 100644
> --- a/block/export/vhost-user-blk-server.c
> +++ b/block/export/vhost-user-blk-server.c
> @@ -208,22 +208,6 @@ static const VuDevIface vu_blk_iface = {
>  .process_msg   = vu_blk_process_msg,
>  };
>  
> -static void blk_aio_attached(AioContext *ctx, void *opaque)
> -{
> -VuBlkExport *vexp = opaque;
> -
> -vexp->export.ctx = ctx;
> -vhost_user_server_attach_aio_context(>vu_server, ctx);
> -}
> -
> -static void blk_aio_detach(void *opaque)
> -{
> -VuBlkExport *vexp = opaque;
> -
> -vhost_user_server_detach_aio_context(>vu_server);
> -vexp->export.ctx = NULL;
> -}

So for changing the AioContext, we now rely on the fact that the node to
be changed is always drained, so the drain callbacks implicitly cover
this case, too?

>  static void
>  vu_blk_initialize_config(BlockDriverState *bs,
>   struct virtio_blk_config *config,
> @@ -272,6 +256,25 @@ static void vu_blk_exp_resize(void *opaque)
>  vu_config_change_msg(>vu_server.vu_dev);
>  }
>  
> +/* Called with vexp->export.ctx acquired */
> +static void vu_blk_drained_begin(void *opaque)
> +{
> +VuBlkExport *vexp = opaque;
> +
> +vhost_user_server_detach_aio_context(>vu_server);
> +}

Compared to the old code, we're losing the vexp->export.ctx = NULL. This
is correct at this point because after drained_begin we still keep
processing requests until we arrive at a quiescent state.

However, if we detach the AioContext because we're deleting the
iothread, won't we end up with a dangling pointer in vexp->export.ctx?
Or can we be certain that nothing interesting happens before drained_end
updates it with a new valid pointer again?

Kevin

Re: proposed schedule for 8.1 release

[Richard Henderson]

On 5/2/23 15:44, Peter Maydell wrote:

Hi; I figured we could put some dates into the proposed 8.1
schedule. Here's a starter:

2023-07-11 Soft feature freeze
2023-07-18 Hard feature freeze. Tag rc0
2023-07-25 Tag rc1
2023-08-01 Tag rc2
2023-08-08 Tag rc3
2023-08-15 Release; or tag rc4 if needed
2023-08-22 Release if we needed an rc4

(dates picked largely to ensure that we don't slip into
September)

Richard, you're doing merges, so the most important
question is whether this works for you :-)


Looks reasonable.  I have some holiday scheduled between rc0 and rc1, but I'm sure we can 
work that out.



r~

RE: [PATCH 9/9] tcg: Remove compatability helpers for qemu ld/st

[Taylor Simpson]

> -Original Message-
> From: Richard Henderson 
> Sent: Tuesday, May 2, 2023 8:58 AM
> To: qemu-devel@nongnu.org
> Cc: mrol...@gmail.com; edgar.igles...@gmail.com; Taylor Simpson
> ; a...@rev.ng; a...@rev.ng; laur...@vivier.eu;
> phi...@linaro.org; jiaxun.y...@flygoat.com; da...@redhat.com;
> i...@linux.ibm.com; th...@redhat.com; mark.cave-ayl...@ilande.co.uk;
> atar4q...@gmail.com; jcmvb...@gmail.com
> Subject: [PATCH 9/9] tcg: Remove compatability helpers for qemu ld/st
> 
> Remove the old interfaces with the implicit MemOp argument.
> 
> Signed-off-by: Richard Henderson 
> ---
>  include/tcg/tcg-op.h | 55 
>  1 file changed, 55 deletions(-)
> 
> diff --git a/include/tcg/tcg-op.h b/include/tcg/tcg-op.h index
> dff17c7072..4401fa493c 100644
> --- a/include/tcg/tcg-op.h
> +++ b/include/tcg/tcg-op.h
> @@ -841,61 +841,6 @@ void tcg_gen_qemu_st_i64(TCGv_i64, TCGv,
> TCGArg, MemOp);  void tcg_gen_qemu_ld_i128(TCGv_i128, TCGv, TCGArg,
> MemOp);  void tcg_gen_qemu_st_i128(TCGv_i128, TCGv, TCGArg, MemOp);
> 
> -static inline void tcg_gen_qemu_ld8u(TCGv ret, TCGv addr, int mem_index)
> -{
> -tcg_gen_qemu_ld_tl(ret, addr, mem_index, MO_UB);
> -}
> -
> -static inline void tcg_gen_qemu_ld8s(TCGv ret, TCGv addr, int mem_index)
> -{
> -tcg_gen_qemu_ld_tl(ret, addr, mem_index, MO_SB);
> -}
> -
> -static inline void tcg_gen_qemu_ld16u(TCGv ret, TCGv addr, int
> mem_index) -{
> -tcg_gen_qemu_ld_tl(ret, addr, mem_index, MO_TEUW);
> -}
> -
> -static inline void tcg_gen_qemu_ld16s(TCGv ret, TCGv addr, int mem_index)
> -{
> -tcg_gen_qemu_ld_tl(ret, addr, mem_index, MO_TESW);
> -}
> -
> -static inline void tcg_gen_qemu_ld32u(TCGv ret, TCGv addr, int
> mem_index) -{
> -tcg_gen_qemu_ld_tl(ret, addr, mem_index, MO_TEUL);
> -}
> -
> -static inline void tcg_gen_qemu_ld32s(TCGv ret, TCGv addr, int mem_index)
> -{
> -tcg_gen_qemu_ld_tl(ret, addr, mem_index, MO_TESL);
> -}
> -
> -static inline void tcg_gen_qemu_ld64(TCGv_i64 ret, TCGv addr, int
> mem_index) -{
> -tcg_gen_qemu_ld_i64(ret, addr, mem_index, MO_TEUQ);
> -}
> -
> -static inline void tcg_gen_qemu_st8(TCGv arg, TCGv addr, int mem_index) -
> {
> -tcg_gen_qemu_st_tl(arg, addr, mem_index, MO_UB);
> -}
> -
> -static inline void tcg_gen_qemu_st16(TCGv arg, TCGv addr, int mem_index)
> -{
> -tcg_gen_qemu_st_tl(arg, addr, mem_index, MO_TEUW);
> -}
> -
> -static inline void tcg_gen_qemu_st32(TCGv arg, TCGv addr, int mem_index)
> -{
> -tcg_gen_qemu_st_tl(arg, addr, mem_index, MO_TEUL);
> -}
> -
> -static inline void tcg_gen_qemu_st64(TCGv_i64 arg, TCGv addr, int
> mem_index) -{
> -tcg_gen_qemu_st_i64(arg, addr, mem_index, MO_TEUQ);
> -}
> -
>  void tcg_gen_atomic_cmpxchg_i32(TCGv_i32, TCGv, TCGv_i32, TCGv_i32,
>  TCGArg, MemOp);  void
> tcg_gen_atomic_cmpxchg_i64(TCGv_i64, TCGv, TCGv_i64, TCGv_i64,


Is the intent that all loads use tcg_gen_qemu_ld_* and all stores use 
tcg_gen_qemu_st_*?

If so, there are other helpers to remove.  For example,
tcg_gen_ld32u_i64
tcg_gen_st8_i64

Thanks,
Taylor

Re: [PATCH 3/9] target/Hexagon: Finish conversion to tcg_gen_qemu_{ld,st}_*

[Richard Henderson]

On 5/2/23 16:27, Taylor Simpson wrote:




-Original Message-
From: Richard Henderson 
Sent: Tuesday, May 2, 2023 8:58 AM
To: qemu-devel@nongnu.org
Cc: mrol...@gmail.com; edgar.igles...@gmail.com; Taylor Simpson
; a...@rev.ng; a...@rev.ng; laur...@vivier.eu;
phi...@linaro.org; jiaxun.y...@flygoat.com; da...@redhat.com;
i...@linux.ibm.com; th...@redhat.com; mark.cave-ayl...@ilande.co.uk;
atar4q...@gmail.com; jcmvb...@gmail.com
Subject: [PATCH 3/9] target/Hexagon: Finish conversion to
tcg_gen_qemu_{ld,st}_*

Convert away from the old interface with the implicit MemOp argument.
Importantly, this removes some incorrect casts generated by idef-parser's
gen_load().

Signed-off-by: Richard Henderson 
---
  target/hexagon/macros.h | 14 -
  target/hexagon/genptr.c |  8 +++---
  target/hexagon/idef-parser/parser-helpers.c | 28 +-
  target/hexagon/translate.c  | 32 ++---
  4 files changed, 40 insertions(+), 42 deletions(-)

diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index
502c85ae35..244063b1d2 100644
--- a/target/hexagon/genptr.c
+++ b/target/hexagon/genptr.c
@@ -320,14 +320,14 @@ void gen_set_byte_i64(int N, TCGv_i64 result, TCGv
src)

  static void gen_return(DisasContext *ctx, TCGv_i64 dst, TCGv src) @@ -
1019,7 +1019,7 @@ static void gen_vreg_load(DisasContext *ctx, intptr_t
dstoff, TCGv src,
  tcg_gen_andi_tl(src, src, ~((int32_t)sizeof(MMVector) - 1));
  }
  for (int i = 0; i < sizeof(MMVector) / 8; i++) {
-tcg_gen_qemu_ld64(tmp, src, ctx->mem_idx);
+tcg_gen_qemu_ld_i64(tmp, src, ctx->mem_idx, MO_TEUQ);
  tcg_gen_addi_tl(src, src, 8);
  tcg_gen_st_i64(tmp, cpu_env, dstoff + i * 8);


Did you intend to leave the tcg_gen_st_i64 alone or should that be converted to 
tcg_gen_qemu_st64.

There's a tcg_gen_st8_i64 in vec_to_qvec.  Does that need to be converted?


No, those are host stores to env, not guest stores to guest memory.
Notice the lack of "qemu" in the function names.


I'm curious if there's a better way to do a vector load (e.g., with 
tcg_gen_gvec_) than a loop that does 8 bytes at a time.


The best you can do at the moment is tcg_gen_qemu_ld_i128.
But there's no gvec variant to load arbitrary vector lengths.


r~

Re: [PATCH] linux-user: Add /proc/cpuinfo handler for RISC-V

[Andreas Schwab]

On Mai 02 2023, Palmer Dabbelt wrote:

> On Tue, 02 May 2023 06:44:00 PDT (-0700), sch...@suse.de wrote:
>> Signed-off-by: Andreas Schwab 
>> ---
>>  linux-user/syscall.c | 30 --
>>  1 file changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 69f740ff98..c72456a34b 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -8231,7 +8231,8 @@ void target_exception_dump(CPUArchState *env, const 
>> char *fmt, int code)
>>  }
>>
>>  #if HOST_BIG_ENDIAN != TARGET_BIG_ENDIAN || \
>> -defined(TARGET_SPARC) || defined(TARGET_M68K) || defined(TARGET_HPPA)
>> +defined(TARGET_SPARC) || defined(TARGET_M68K) || defined(TARGET_HPPA) 
>> || \
>> +defined(TARGET_RISCV)
>>  static int is_proc(const char *filename, const char *entry)
>>  {
>>  return strcmp(filename, entry) == 0;
>> @@ -8309,6 +8310,31 @@ static int open_cpuinfo(CPUArchState *cpu_env, int fd)
>>  }
>>  #endif
>>
>> +#if defined(TARGET_RISCV)
>> +static int open_cpuinfo(CPUArchState *cpu_env, int fd)
>> +{
>> +int i, num_cpus;
>> +
>> +num_cpus = sysconf(_SC_NPROCESSORS_ONLN);
>> +for (i = 0; i < num_cpus; i++) {
>> +dprintf(fd, "processor\t: %d\n", i);
>> +dprintf(fd, "hart\t\t: %d\n", i);
>> +#if defined(TARGET_RISCV32)
>> +dprintf(fd, "isa\t\t: rv32imafdc\n");
>> +dprintf(fd, "mmu\t\t: sv32\n");
>> +#endif
>> +#if defined(TARGET_RISCV64)
>> +dprintf(fd, "isa\t\t: rv64imafdc\n");
>> +dprintf(fd, "mmu\t\t: sv57\n");
>
> Unless I'm misunderstanding something, we've got support for both non-sv57
> system (via sv* CPU properties)

The mmu type is not available in linux-user emulation.  This just
matches the default the system emulation would use.

> and non-GC systems (also via CPU properties).

None of the currently defined cpus are non-GC cpus (except sifive_e, but
that is not suitable for user-space anyway), and there doesn't appear to
be any properties defined for changing the supported extensions.

-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v4 06/20] block/export: wait for vhost-user-blk requests when draining

[Kevin Wolf]

Am 25.04.2023 um 19:27 hat Stefan Hajnoczi geschrieben:
> Each vhost-user-blk request runs in a coroutine. When the BlockBackend
> enters a drained section we need to enter a quiescent state. Currently
> any in-flight requests race with bdrv_drained_begin() because it is
> unaware of vhost-user-blk requests.
> 
> When blk_co_preadv/pwritev()/etc returns it wakes the
> bdrv_drained_begin() thread but vhost-user-blk request processing has
> not yet finished. The request coroutine continues executing while the
> main loop thread thinks it is in a drained section.
> 
> One example where this is unsafe is for blk_set_aio_context() where
> bdrv_drained_begin() is called before .aio_context_detached() and
> .aio_context_attach(). If request coroutines are still running after
> bdrv_drained_begin(), then the AioContext could change underneath them
> and they race with new requests processed in the new AioContext. This
> could lead to virtqueue corruption, for example.
> 
> (This example is theoretical, I came across this while reading the
> code and have not tried to reproduce it.)
> 
> It's easy to make bdrv_drained_begin() wait for in-flight requests: add
> a .drained_poll() callback that checks the VuServer's in-flight counter.
> VuServer just needs an API that returns true when there are requests in
> flight. The in-flight counter needs to be atomic.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  include/qemu/vhost-user-server.h |  4 +++-
>  block/export/vhost-user-blk-server.c | 16 
>  util/vhost-user-server.c | 14 ++
>  3 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/include/qemu/vhost-user-server.h 
> b/include/qemu/vhost-user-server.h
> index bc0ac9ddb6..b1c1cda886 100644
> --- a/include/qemu/vhost-user-server.h
> +++ b/include/qemu/vhost-user-server.h
> @@ -40,8 +40,9 @@ typedef struct {
>  int max_queues;
>  const VuDevIface *vu_iface;
>  
> +unsigned int in_flight; /* atomic */
> +
>  /* Protected by ctx lock */
> -unsigned int in_flight;
>  bool wait_idle;
>  VuDev vu_dev;
>  QIOChannel *ioc; /* The I/O channel with the client */
> @@ -62,6 +63,7 @@ void vhost_user_server_stop(VuServer *server);
>  
>  void vhost_user_server_inc_in_flight(VuServer *server);
>  void vhost_user_server_dec_in_flight(VuServer *server);
> +bool vhost_user_server_has_in_flight(VuServer *server);
>  
>  void vhost_user_server_attach_aio_context(VuServer *server, AioContext *ctx);
>  void vhost_user_server_detach_aio_context(VuServer *server);
> diff --git a/block/export/vhost-user-blk-server.c 
> b/block/export/vhost-user-blk-server.c
> index 841acb36e3..092b86aae4 100644
> --- a/block/export/vhost-user-blk-server.c
> +++ b/block/export/vhost-user-blk-server.c
> @@ -272,7 +272,20 @@ static void vu_blk_exp_resize(void *opaque)
>  vu_config_change_msg(>vu_server.vu_dev);
>  }
>  
> +/*
> + * Ensures that bdrv_drained_begin() waits until in-flight requests complete.
> + *
> + * Called with vexp->export.ctx acquired.
> + */
> +static bool vu_blk_drained_poll(void *opaque)
> +{
> +VuBlkExport *vexp = opaque;
> +
> +return vhost_user_server_has_in_flight(>vu_server);
> +}
> +
>  static const BlockDevOps vu_blk_dev_ops = {
> +.drained_poll  = vu_blk_drained_poll,
>  .resize_cb = vu_blk_exp_resize,
>  };

You're adding a new function pointer to an existing BlockDevOps...

> @@ -314,6 +327,7 @@ static int vu_blk_exp_create(BlockExport *exp, 
> BlockExportOptions *opts,
>  vu_blk_initialize_config(blk_bs(exp->blk), >blkcfg,
>   logical_block_size, num_queues);
>  
> +blk_set_dev_ops(exp->blk, _blk_dev_ops, vexp);
>  blk_add_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach,
>   vexp);
>  
>  blk_set_dev_ops(exp->blk, _blk_dev_ops, vexp);

..but still add a second blk_set_dev_ops(). Maybe a bad merge conflict
resolution with commit ca858a5fe94?

> @@ -323,6 +337,7 @@ static int vu_blk_exp_create(BlockExport *exp, 
> BlockExportOptions *opts,
>   num_queues, _blk_iface, errp)) {
>  blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
>  blk_aio_detach, vexp);
> +blk_set_dev_ops(exp->blk, NULL, NULL);
>  g_free(vexp->handler.serial);
>  return -EADDRNOTAVAIL;
>  }
> @@ -336,6 +351,7 @@ static void vu_blk_exp_delete(BlockExport *exp)
>  
>  blk_remove_aio_context_notifier(exp->blk, blk_aio_attached, 
> blk_aio_detach,
>  vexp);
> +blk_set_dev_ops(exp->blk, NULL, NULL);
>  g_free(vexp->handler.serial);
>  }

These two hunks are then probably already fixes for ca858a5fe94 and
should be a separate patch if so.

> diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
> index 1622f8cfb3..2e6b640050 100644
> --- a/util/vhost-user-server.c
> +++ b/util/vhost-user-server.c
> 

Re: [PATCH v2 00/10] Kconfig vs. default devices

[Alex Bennée]

Fabiano Rosas  writes:

> v2:
> Applying the feedback received, all small tweaks.
>
> Patch 6 still needs consensus on whether to apply the fix to Kconfig
> or elsewhere. Link to the previous version:
> https://lore.kernel.org/r/461ba038-31bf-49c4-758b-94ece36f1...@redhat.com
>
> changelog:
>
> - patch 1: moved isa-parallel to a build time check like the other
>patches;
> - patch 3: tweaked commit message;
> - patch 7: removed the default from XLNX_USB_SUBSYS.

I've queued the ARM tweaks to testing/next where I can add a test for it
in the end. I'll leave the x86 stuff for discussion of the more complete
solution that avoids hacky downstream patching.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

RE: [PATCH 3/9] target/Hexagon: Finish conversion to tcg_gen_qemu_{ld,st}_*

[Taylor Simpson]

> -Original Message-
> From: Richard Henderson 
> Sent: Tuesday, May 2, 2023 8:58 AM
> To: qemu-devel@nongnu.org
> Cc: mrol...@gmail.com; edgar.igles...@gmail.com; Taylor Simpson
> ; a...@rev.ng; a...@rev.ng; laur...@vivier.eu;
> phi...@linaro.org; jiaxun.y...@flygoat.com; da...@redhat.com;
> i...@linux.ibm.com; th...@redhat.com; mark.cave-ayl...@ilande.co.uk;
> atar4q...@gmail.com; jcmvb...@gmail.com
> Subject: [PATCH 3/9] target/Hexagon: Finish conversion to
> tcg_gen_qemu_{ld,st}_*
> 
> Convert away from the old interface with the implicit MemOp argument.
> Importantly, this removes some incorrect casts generated by idef-parser's
> gen_load().
> 
> Signed-off-by: Richard Henderson 
> ---
>  target/hexagon/macros.h | 14 -
>  target/hexagon/genptr.c |  8 +++---
>  target/hexagon/idef-parser/parser-helpers.c | 28 +-
>  target/hexagon/translate.c  | 32 ++---
>  4 files changed, 40 insertions(+), 42 deletions(-)
> 
> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index
> 502c85ae35..244063b1d2 100644
> --- a/target/hexagon/genptr.c
> +++ b/target/hexagon/genptr.c
> @@ -320,14 +320,14 @@ void gen_set_byte_i64(int N, TCGv_i64 result, TCGv
> src)
> 
>  static void gen_return(DisasContext *ctx, TCGv_i64 dst, TCGv src) @@ -
> 1019,7 +1019,7 @@ static void gen_vreg_load(DisasContext *ctx, intptr_t
> dstoff, TCGv src,
>  tcg_gen_andi_tl(src, src, ~((int32_t)sizeof(MMVector) - 1));
>  }
>  for (int i = 0; i < sizeof(MMVector) / 8; i++) {
> -tcg_gen_qemu_ld64(tmp, src, ctx->mem_idx);
> +tcg_gen_qemu_ld_i64(tmp, src, ctx->mem_idx, MO_TEUQ);
>  tcg_gen_addi_tl(src, src, 8);
>  tcg_gen_st_i64(tmp, cpu_env, dstoff + i * 8);

Did you intend to leave the tcg_gen_st_i64 alone or should that be converted to 
tcg_gen_qemu_st64.

There's a tcg_gen_st8_i64 in vec_to_qvec.  Does that need to be converted?

>  }

I'm curious if there's a better way to do a vector load (e.g., with 
tcg_gen_gvec_) than a loop that does 8 bytes at a time.

Otherwise,
Reviewed-by: Taylor Simpson 

[PATCH] softfloat: Fix the incorrect computation in float32_exp2()

[Shivaprasad G Bhat]

The float32_exp2() is computing wrong exponent of 2.
For example, with the following set of values {0.1, 2.0, 2.0, -1.0},
the expected output would be {1.071773, 4.00, 4.00, 0.50}.
Instead, the function is computing {1.119102, 3.382044, 3.382044, -0.191022}

Looking at the code, the float32_exp2() attempts to do this

  2 3 4 5   n
  xx x x x x   x
 e  = 1 + --- + --- + --- + --- + --- + ... + --- + ...
   1!2!3!4!5!  n!

But because of the 'typo'/bug it ends up doing

 xx x x x x   x
e  = 1 + --- + --- + --- + --- + --- + ... + --- + ...
  1!2!3!4!5!  n!

This is because instead of the xnp which holds the numerator,
parts_muladd is using the xp which is just 'x'. The commit '572c4d862ff2'
refactored this function, and it seems mistakenly using xp instead of xnp.

The patches fixes this possible typo.

Fixes: 572c4d862ff2 "softfloat: Convert float32_exp2 to FloatParts"
Partially-Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1623
Reported-By: Luca Barbato (https://gitlab.com/lu-zero)
Signed-off-by: Shivaprasad G Bhat 
Signed-off-by: Vaibhav Jain 
---
 fpu/softfloat.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index c7454c3eb1a..108f9cb224a 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -5135,7 +5135,7 @@ float32 float32_exp2(float32 a, float_status *status)
 float64_unpack_canonical(, float64_one, status);
 for (i = 0 ; i < 15 ; i++) {
 float64_unpack_canonical(, float32_exp2_coefficients[i], status);
-rp = *parts_muladd(, , , 0, status);
+rp = *parts_muladd(, , , 0, status);
 xnp = *parts_mul(, , status);
 }

Re: [PATCH v2 00/10] Kconfig vs. default devices

[Alex Bennée]

Thomas Huth  writes:

> On 08/02/2023 20.43, Philippe Mathieu-Daudé wrote:
>> On 8/2/23 20:26, Fabiano Rosas wrote:
>> 
>>> We currently have a situation where disabling a Kconfig might result
>>> in a runtime error when QEMU selects the corresponding device as a
>>> default value for an option. But first a disambiguation:
>>>
>>> Kconfig default::
>>>    a device "Foo" for which there's "config FOO default y" or "config X
>>>    imply FOO" in Kconfig.
>>>
>>> QEMU hardcoded default::
>>>    a fallback; a device "Foo" that is chosen in case no corresponding
>>>    option is given in the command line.
>>>
>>> The issue I'm trying to solve is that there is no link between the two
>>> "defaults" above, which means that when the user at build time
>>> de-selects a Kconfig default, either via configs/devices/*/*.mak or
>>> --without-default-devices, the subsequent invocation at runtime might
>>> continue to try to create the missing device due to QEMU defaults.
>> This will keep bitrotting if we don't cover such configs in our CI.
>> Why do you want to get this fixed BTW? I'm not sure there is a big
>> interest (as in "almost no users").
>> I tried to do that few years ago [*] and Thomas said:
>> "in our CI, we should test what users really need,
>>   and not each and every distantly possible combination."
>
> You're mis-quoting me here. That comment was made when we were talking
> about very arbitrary configs that likely nobody is going to use.
> Fabiano's series here is about the --without-default-devices configure
> option which everybody could add to their set of "configure" options
> easily.

Indeed - while trying to reduce the compile time I ran into this with a
plain --without-default-devices check. We also have in the meantime
introduced --with-devices-FOO so we can do minimal builds.

>
>  Thomas


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro