date:20220829

[PATCH v3 2/2] target/i386: Raise #GP on unaligned m128 accesses when required.

2022-08-29 Thread Ricky Zhou

Many instructions which load/store 128-bit values are supposed to
raise #GP when the memory operand isn't 16-byte aligned. This includes:
 - Instructions explicitly requiring memory alignment (Exceptions Type 1
   in the "AVX and SSE Instruction Exception Specification" section of
   the SDM)
 - Legacy SSE instructions that load/store 128-bit values (Exceptions
   Types 2 and 4).

This change sets MO_ALIGN_16 on 128-bit memory accesses that require
16-byte alignment. It adds cpu_record_sigbus and cpu_do_unaligned_access
hooks that simulate a #GP exception in qemu-user and qemu-system,
respectively.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/217
Reviewed-by: Richard Henderson 
Signed-off-by: Ricky Zhou 
---
 target/i386/tcg/excp_helper.c| 13 
 target/i386/tcg/helper-tcg.h | 28 ++---
 target/i386/tcg/sysemu/excp_helper.c |  8 +
 target/i386/tcg/tcg-cpu.c|  2 ++
 target/i386/tcg/translate.c  | 45 +---
 target/i386/tcg/user/excp_helper.c   |  7 +
 6 files changed, 74 insertions(+), 29 deletions(-)

diff --git a/target/i386/tcg/excp_helper.c b/target/i386/tcg/excp_helper.c
index c1ffa1c0ef..7c3c8dc7fe 100644
--- a/target/i386/tcg/excp_helper.c
+++ b/target/i386/tcg/excp_helper.c
@@ -140,3 +140,16 @@ G_NORETURN void raise_exception_ra(CPUX86State *env, int 
exception_index,
 {
 raise_interrupt2(env, exception_index, 0, 0, 0, retaddr);
 }
+
+G_NORETURN void handle_unaligned_access(CPUX86State *env, vaddr vaddr,
+MMUAccessType access_type,
+uintptr_t retaddr)
+{
+/*
+ * Unaligned accesses are currently only triggered by SSE/AVX
+ * instructions that impose alignment requirements on memory
+ * operands. These instructions raise #GP(0) upon accessing an
+ * unaligned address.
+ */
+raise_exception_ra(env, EXCP0D_GPF, retaddr);
+}
diff --git a/target/i386/tcg/helper-tcg.h b/target/i386/tcg/helper-tcg.h
index 34167e2e29..cd1723389a 100644
--- a/target/i386/tcg/helper-tcg.h
+++ b/target/i386/tcg/helper-tcg.h
@@ -42,17 +42,6 @@ void x86_cpu_do_interrupt(CPUState *cpu);
 bool x86_cpu_exec_interrupt(CPUState *cpu, int int_req);
 #endif
 
-/* helper.c */
-#ifdef CONFIG_USER_ONLY
-void x86_cpu_record_sigsegv(CPUState *cs, vaddr addr,
-MMUAccessType access_type,
-bool maperr, uintptr_t ra);
-#else
-bool x86_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
-  MMUAccessType access_type, int mmu_idx,
-  bool probe, uintptr_t retaddr);
-#endif
-
 void breakpoint_handler(CPUState *cs);
 
 /* n must be a constant to be efficient */
@@ -78,6 +67,23 @@ G_NORETURN void raise_exception_err_ra(CPUX86State *env, int 
exception_index,
int error_code, uintptr_t retaddr);
 G_NORETURN void raise_interrupt(CPUX86State *nenv, int intno, int is_int,
 int error_code, int next_eip_addend);
+G_NORETURN void handle_unaligned_access(CPUX86State *env, vaddr vaddr,
+MMUAccessType access_type,
+uintptr_t retaddr);
+#ifdef CONFIG_USER_ONLY
+void x86_cpu_record_sigsegv(CPUState *cs, vaddr addr,
+MMUAccessType access_type,
+bool maperr, uintptr_t ra);
+void x86_cpu_record_sigbus(CPUState *cs, vaddr addr,
+   MMUAccessType access_type, uintptr_t ra);
+#else
+bool x86_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
+  MMUAccessType access_type, int mmu_idx,
+  bool probe, uintptr_t retaddr);
+G_NORETURN void x86_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
+MMUAccessType access_type,
+int mmu_idx, uintptr_t retaddr);
+#endif
 
 /* cc_helper.c */
 extern const uint8_t parity_table[256];
diff --git a/target/i386/tcg/sysemu/excp_helper.c 
b/target/i386/tcg/sysemu/excp_helper.c
index 48feba7e75..796dc2a1f3 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -439,3 +439,11 @@ bool x86_cpu_tlb_fill(CPUState *cs, vaddr addr, int size,
 }
 return true;
 }
+
+G_NORETURN void x86_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
+MMUAccessType access_type,
+int mmu_idx, uintptr_t retaddr)
+{
+X86CPU *cpu = X86_CPU(cs);
+handle_unaligned_access(>env, vaddr, access_type, retaddr);
+}
diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
index 6fdfdf9598..d3c2b8fb49 100644
--- a/target/i386/tcg/tcg-cpu.c
+++ b/target/i386/tcg/tcg-cpu.c
@@ -75,10 +75,12 @@ static const struct TCGCPUOps x86_tcg_ops = {
 #ifdef CONFIG_USER_ONLY

[PATCH v3 1/2] target/i386: Read 8 bytes from cvttps2pi/cvtps2pi memory operands

2022-08-29 Thread Ricky Zhou

Before this change, emulation of cvttps2pi and cvtps2pi instructions
would read 16 bytes of memory instead of 8. The SDM states that
cvttps2pi takes a 64-bit memory location. The documentation for cvtps2pi
claims that it takes a a 128-bit memory location, but as with cvttps2pi,
the operand is written as xmm/m64. I double-checked on real hardware
that both of these instructions only read 8 bytes.

Reviewed-by: Richard Henderson 
Signed-off-by: Ricky Zhou 
---
 target/i386/tcg/translate.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index b7972f0ff5..3ba5f76156 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -3621,7 +3621,11 @@ static void gen_sse(CPUX86State *env, DisasContext *s, 
int b,
 if (mod != 3) {
 gen_lea_modrm(env, s, modrm);
 op2_offset = offsetof(CPUX86State,xmm_t0);
-gen_ldo_env_A0(s, op2_offset);
+if (b1) {
+gen_ldo_env_A0(s, op2_offset);
+} else {
+gen_ldq_env_A0(s, op2_offset);
+}
 } else {
 rm = (modrm & 7) | REX_B(s);
 op2_offset = offsetof(CPUX86State,xmm_regs[rm]);
-- 
2.37.2

[PATCH] hcd-ohci: Fix inconsistency when resetting ohci root hubs

2022-08-29 Thread Qiang Liu

I found an assertion failure in usb_cancel_packet() and posted my analysis in
https://gitlab.com/qemu-project/qemu/-/issues/1180. I think this issue is
because the inconsistency when resetting ohci root hubs.

There are two ways to reset ohci root hubs: 1) through HcRhPortStatus, 2)
through HcControl. However, when the packet's status is USB_PACKET_ASYNC,
resetting through HcRhPortStatus will complete the packet and thus resetting
through HcControl will fail. That is because IMO resetting through
HcRhPortStatus should first detach the port and then invoked usb_device_reset()
just like through HcControl. Therefore, I change usb_device_reset() to
usb_port_reset() where usb_detach() and usb_device_reset() are invoked
consequently.

Fixes: d28f4e2d8631 ("usb: kill USB_MSG_RESET")
Reported-by: Qiang Liu 
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1180
Signed-off-by: Qiang Liu 
---
 hw/usb/hcd-ohci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 895b29fb86..72df917834 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -1426,7 +1426,7 @@ static void ohci_port_set_status(OHCIState *ohci, int 
portnum, uint32_t val)
 
 if (ohci_port_set_if_connected(ohci, portnum, val & OHCI_PORT_PRS)) {
 trace_usb_ohci_port_reset(portnum);
-usb_device_reset(port->port.dev);
+usb_port_reset(>port);
 port->ctrl &= ~OHCI_PORT_PRS;
 /* ??? Should this also set OHCI_PORT_PESC.  */
 port->ctrl |= OHCI_PORT_PES | OHCI_PORT_PRSC;
-- 
2.25.1

Re: [PATCH v2 1/1] target/i386: Raise #GP on unaligned m128 accesses when required.

2022-08-29 Thread Richard Henderson


On 8/29/22 19:11, Ricky Zhou wrote:

Many instructions which load/store 128-bit values are supposed to
raise #GP when the memory operand isn't 16-byte aligned. This includes:
  - Instructions explicitly requiring memory alignment (Exceptions Type 1
in the "AVX and SSE Instruction Exception Specification" section of
the SDM)
  - Legacy SSE instructions that load/store 128-bit values (Exceptions
Types 2 and 4).

This change sets MO_ALIGN_16 on 128-bit memory accesses that require
16-byte alignment. It adds cpu_record_sigbus and cpu_do_unaligned_access
handlers that simulate a #GP exception in qemu-user and qemu-system,
respectively.





One minor behavior change apart from what is described above: Prior to
this change, emulation of cvttps2pi and cvtps2pi instructions would
incorrectly read 16 bytes of memory instead of 8. I double-checked on
real hardware that these instructions only read 8 bytes (and do not have
any address alignment requirements).


This should really be split out to a separate patch.



@@ -3621,7 +3629,11 @@ static void gen_sse(CPUX86State *env, DisasContext *s, 
int b,
  if (mod != 3) {
  gen_lea_modrm(env, s, modrm);
  op2_offset = offsetof(CPUX86State,xmm_t0);
-gen_ldo_env_A0(s, op2_offset);
+if ((b >> 8) & 1) {


Aka b1.

Otherwise,
Reviewed-by: Richard Henderson 


r~

[PATCH v2 0/1] target/i386: Raise #GP on unaligned m128 accesses when required.

2022-08-29 Thread Ricky Zhou

Thanks Richard for the detailed comments/code pointers! I've switched to
using MO_ALIGN_16 and implemented record_sigbus and do_unaligned_access
hooks to simulate #GP(0) as suggested. Given what was said about the low
likelihood of implementing #AC anytime soon, I have hardcoded #GP(0) in
these hooks for now rather than plumbing through an extra bit in MemOp.
Let me know if that seems reasonable, thanks!

Ricky Zhou (1):
  target/i386: Raise #GP on unaligned m128 accesses when required.

 target/i386/tcg/excp_helper.c| 13 
 target/i386/tcg/helper-tcg.h | 28 +---
 target/i386/tcg/sysemu/excp_helper.c |  8 +
 target/i386/tcg/tcg-cpu.c|  1 +
 target/i386/tcg/translate.c  | 49 ++--
 target/i386/tcg/user/excp_helper.c   |  7 
 6 files changed, 77 insertions(+), 29 deletions(-)

-- 
2.37.2

Re: [PATCH for-7.2 v4 15/21] qmp/hmp, device_tree.c: introduce 'info fdt' command

2022-08-29 Thread David Gibson

On Mon, Aug 29, 2022 at 07:00:55PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 8/29/22 00:34, David Gibson wrote:
> > On Fri, Aug 26, 2022 at 11:11:44AM -0300, Daniel Henrique Barboza wrote:
> > > Reading the FDT requires that the user saves the fdt_blob and then use
> > > 'dtc' to read the contents. Saving the file and using 'dtc' is a strong
> > > use case when we need to compare two FDTs, but it's a lot of steps if
> > > you want to do quick check on a certain node or property.
> > > 
> > > 'info fdt' retrieves FDT nodes (and properties, later on) and print it
> > > to the user. This can be used to check the FDT on running machines
> > > without having to save the blob and use 'dtc'.
> > > 
> > > The implementation is based on the premise that the machine thas a FDT
> > > created using libfdt and pointed by 'machine->fdt'. As long as this
> > > pre-requisite is met the machine should be able to support it.
> > > 
> > > For now we're going to add the required QMP/HMP boilerplate and the
> > > capability of printing the name of the properties of a given node. Next
> > > patches will extend 'info fdt' to be able to print nodes recursively,
> > > and then individual properties.
> > > 
> > > This command will always be executed in-band (i.e. holding BQL),
> > > avoiding potential race conditions with machines that might change the
> > > FDT during runtime (e.g. PowerPC 'pseries' machine).
> > > 
> > > 'info fdt' is not something that we expect to be used aside from 
> > > debugging,
> > > so we're implementing it in QMP as 'x-query-fdt'.
> > > 
> > > This is an example of 'info fdt' fetching the '/chosen' node of the
> > > pSeries machine:
> > > 
> > > (qemu) info fdt /chosen
> > > chosen {
> > >  ibm,architecture-vec-5;
> > >  rng-seed;
> > >  ibm,arch-vec-5-platform-support;
> > >  linux,pci-probe-only;
> > >  stdout-path;
> > >  linux,stdout-path;
> > >  qemu,graphic-depth;
> > >  qemu,graphic-height;
> > >  qemu,graphic-width;
> > > };
> > > 
> > > And the same node for the aarch64 'virt' machine:
> > > 
> > > (qemu) info fdt /chosen
> > > chosen {
> > >  stdout-path;
> > >  rng-seed;
> > >  kaslr-seed;
> > > };
> > 
> > So, I'm reasonably convinced allowing dumping the whole dtb from
> > qmp/hmp is useful.  I'm less convined that info fdt is worth the
> > additional complexity it incurs.  Note that as well as being able to
> > decompile a whole dtb using dtc, you can also extract and list
> > specific properties from a dtb blob using the 'fdtget' tool which is
> > part of the dtc tree.
> 
> What's your opinion on patch 21/21, where 'dumpdtb' can write a formatted
> FDT in a file with an extra option? That was possible because of the
> format helpers introduced for 'info fdt'. The idea is that since we're
> able to format a FDT in DTS format, we can also write the FDT in text
> format without relying on DTC to decode it.

Since it's mostly the same code, I think it's reasonable to throw in
if the info fdt stuff is there, but I don't think it's worth including
without that.  As a whole, I remain dubious that (info fdt + dumpdts)
is worth the complexity cost.

People with more practical experience debugging the embedded ARM
platforms might have a different opinion if they thing info fdt would
be really useful though.

> If we think that this 'dumpdtb' capability is worth having, I can respin
> the patches without 'info fdt' but adding these helpers to enable this
> 'dumpdtb' support. If not, then we can just remove patches 15-21 and
> be done with it.
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> > 
> > > 
> > > Cc: Dr. David Alan Gilbert 
> > > Acked-by: Dr. David Alan Gilbert 
> > > Signed-off-by: Daniel Henrique Barboza 
> > > ---
> > >   hmp-commands-info.hx | 13 ++
> > >   include/monitor/hmp.h|  1 +
> > >   include/sysemu/device_tree.h |  4 +++
> > >   monitor/hmp-cmds.c   | 13 ++
> > >   monitor/qmp-cmds.c   | 12 +
> > >   qapi/machine.json| 19 +++
> > >   softmmu/device_tree.c| 47 
> > >   7 files changed, 109 insertions(+)
> > > 
> > > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> > > index 188d9ece3b..743b48865d 100644
> > > --- a/hmp-commands-info.hx
> > > +++ b/hmp-commands-info.hx
> > > @@ -921,3 +921,16 @@ SRST
> > > ``stats``
> > >   Show runtime-collected statistics
> > >   ERST
> > > +
> > > +{
> > > +.name   = "fdt",
> > > +.args_type  = "nodepath:s",
> > > +.params = "nodepath",
> > > +.help   = "show firmware device tree node given its path",
> > > +.cmd= hmp_info_fdt,
> > > +},
> > > +
> > > +SRST
> > > +  ``info fdt``
> > > +Show a firmware device tree node given its path. Requires libfdt.
> > > +ERST
> > > diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> > > index d7f324da59..c0883dd1e3 100644
> > > ---

[PATCH v2 1/1] target/i386: Raise #GP on unaligned m128 accesses when required.

2022-08-29 Thread Ricky Zhou

Many instructions which load/store 128-bit values are supposed to
raise #GP when the memory operand isn't 16-byte aligned. This includes:
 - Instructions explicitly requiring memory alignment (Exceptions Type 1
   in the "AVX and SSE Instruction Exception Specification" section of
   the SDM)
 - Legacy SSE instructions that load/store 128-bit values (Exceptions
   Types 2 and 4).

This change sets MO_ALIGN_16 on 128-bit memory accesses that require
16-byte alignment. It adds cpu_record_sigbus and cpu_do_unaligned_access
handlers that simulate a #GP exception in qemu-user and qemu-system,
respectively.

One minor behavior change apart from what is described above: Prior to
this change, emulation of cvttps2pi and cvtps2pi instructions would
incorrectly read 16 bytes of memory instead of 8. I double-checked on
real hardware that these instructions only read 8 bytes (and do not have
any address alignment requirements).

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/217
Signed-off-by: Ricky Zhou 
---
 target/i386/tcg/excp_helper.c| 13 
 target/i386/tcg/helper-tcg.h | 28 +---
 target/i386/tcg/sysemu/excp_helper.c |  8 +
 target/i386/tcg/tcg-cpu.c|  2 ++
 target/i386/tcg/translate.c  | 49 ++--
 target/i386/tcg/user/excp_helper.c   |  7 
 6 files changed, 78 insertions(+), 29 deletions(-)

diff --git a/target/i386/tcg/excp_helper.c b/target/i386/tcg/excp_helper.c
index c1ffa1c0ef..7c3c8dc7fe 100644
--- a/target/i386/tcg/excp_helper.c
+++ b/target/i386/tcg/excp_helper.c
@@ -140,3 +140,16 @@ G_NORETURN void raise_exception_ra(CPUX86State *env, int 
exception_index,
 {
 raise_interrupt2(env, exception_index, 0, 0, 0, retaddr);
 }
+
+G_NORETURN void handle_unaligned_access(CPUX86State *env, vaddr vaddr,
+MMUAccessType access_type,
+uintptr_t retaddr)
+{
+/*
+ * Unaligned accesses are currently only triggered by SSE/AVX
+ * instructions that impose alignment requirements on memory
+ * operands. These instructions raise #GP(0) upon accessing an
+ * unaligned address.
+ */
+raise_exception_ra(env, EXCP0D_GPF, retaddr);
+}
diff --git a/target/i386/tcg/helper-tcg.h b/target/i386/tcg/helper-tcg.h
index 34167e2e29..cd1723389a 100644
--- a/target/i386/tcg/helper-tcg.h
+++ b/target/i386/tcg/helper-tcg.h
@@ -42,17 +42,6 @@ void x86_cpu_do_interrupt(CPUState *cpu);
 bool x86_cpu_exec_interrupt(CPUState *cpu, int int_req);
 #endif
 
-/* helper.c */
-#ifdef CONFIG_USER_ONLY
-void x86_cpu_record_sigsegv(CPUState *cs, vaddr addr,
-MMUAccessType access_type,
-bool maperr, uintptr_t ra);
-#else
-bool x86_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
-  MMUAccessType access_type, int mmu_idx,
-  bool probe, uintptr_t retaddr);
-#endif
-
 void breakpoint_handler(CPUState *cs);
 
 /* n must be a constant to be efficient */
@@ -78,6 +67,23 @@ G_NORETURN void raise_exception_err_ra(CPUX86State *env, int 
exception_index,
int error_code, uintptr_t retaddr);
 G_NORETURN void raise_interrupt(CPUX86State *nenv, int intno, int is_int,
 int error_code, int next_eip_addend);
+G_NORETURN void handle_unaligned_access(CPUX86State *env, vaddr vaddr,
+MMUAccessType access_type,
+uintptr_t retaddr);
+#ifdef CONFIG_USER_ONLY
+void x86_cpu_record_sigsegv(CPUState *cs, vaddr addr,
+MMUAccessType access_type,
+bool maperr, uintptr_t ra);
+void x86_cpu_record_sigbus(CPUState *cs, vaddr addr,
+   MMUAccessType access_type, uintptr_t ra);
+#else
+bool x86_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
+  MMUAccessType access_type, int mmu_idx,
+  bool probe, uintptr_t retaddr);
+G_NORETURN void x86_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
+MMUAccessType access_type,
+int mmu_idx, uintptr_t retaddr);
+#endif
 
 /* cc_helper.c */
 extern const uint8_t parity_table[256];
diff --git a/target/i386/tcg/sysemu/excp_helper.c 
b/target/i386/tcg/sysemu/excp_helper.c
index 48feba7e75..796dc2a1f3 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -439,3 +439,11 @@ bool x86_cpu_tlb_fill(CPUState *cs, vaddr addr, int size,
 }
 return true;
 }
+
+G_NORETURN void x86_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
+MMUAccessType access_type,
+int mmu_idx, uintptr_t retaddr)
+{
+X86CPU *cpu = X86_CPU(cs);
+handle_unaligned_access(>env, vaddr,

Re: [PATCH v1 15/25] Deprecate 32 bit big-endian MIPS

2022-08-29 Thread Huacai Chen

Reviewed-by: Huacai Chen 

On Tue, Aug 30, 2022 at 7:39 AM Philippe Mathieu-Daudé  wrote:
>
> Hi Alex,
>
> (+Aleksandar/Huacai)
>
> On 26/8/22 19:21, Alex Bennée wrote:
> > It's becoming harder to maintain a cross-compiler to test this host
> > architecture as the old stable Debian 10 ("Buster") moved into LTS
> > which supports fewer architectures. For now:
> >
> >- mark it's deprecation in the docs
> >- downgrade the containers to build TCG tests only
> >- drop the cross builds from our CI
> >
> > Users with an appropriate toolchain and user-space can still take
> > their chances building it.
> >
> > Signed-off-by: Alex Bennée 
> > ---
> >   docs/about/build-platforms.rst|  2 +-
> >   docs/about/deprecated.rst | 13 ++
> >   .gitlab-ci.d/container-cross.yml  |  1 -
> >   .gitlab-ci.d/crossbuilds.yml  | 14 ---
> >   tests/docker/Makefile.include |  5 +--
> >   .../dockerfiles/debian-mips-cross.docker  | 40 +--
> >   6 files changed, 27 insertions(+), 48 deletions(-)
> >
> > diff --git a/docs/about/build-platforms.rst b/docs/about/build-platforms.rst
> > index 26028756d0..1ca9144a7d 100644
> > --- a/docs/about/build-platforms.rst
> > +++ b/docs/about/build-platforms.rst
> > @@ -41,7 +41,7 @@ Those hosts are officially supported, with various 
> > accelerators:
> >- Accelerators
> >  * - Arm
> >- kvm (64 bit only), tcg, xen
> > -   * - MIPS
> > +   * - MIPS (LE only)
> >- kvm, tcg
> >  * - PPC
> >- kvm, tcg
> > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> > index 91b03115ee..22c2f4f4de 100644
> > --- a/docs/about/deprecated.rst
> > +++ b/docs/about/deprecated.rst
> > @@ -213,6 +213,19 @@ MIPS ``Trap-and-Emul`` KVM support (since 6.0)
> >   The MIPS ``Trap-and-Emul`` KVM host and guest support has been removed
> >   from Linux upstream kernel, declare it deprecated.
> >
> > +Host Architectures
> > +--
> > +
> > +BE MIPS (since 7.2)
> > +'''
> > +
> > +A Debian 10 ("Buster") moved into LTS the big endian 32 bit version of
> > +MIPS moved out of support making it hard to maintain our
> > +cross-compilation CI tests of the architecture. As we no longer have
> > +CI coverage support may bitrot away before the deprecation process
> > +completes. The little endian variants of MIPS (both 32 and 64 bit) are
> > +still a supported host architecture.
>
> For completeness we should update meson.build to consider
> host_machine.endian() and adapt this section:
>
>
>if not supported_cpus.contains(cpu)
>  message()
>  warning('SUPPORT FOR THIS HOST CPU WILL GO AWAY IN FUTURE RELEASES!')
>  message()
>  message('CPU host architecture ' + cpu + ' support is not currently
> maintained.')
>...
>
> This can be done later, and I might be able to do so in few weeks,
> so meanwhile (with Thomas comment addressed):
> Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH v8 0/7] Add support for zoned device

2022-08-29 Thread Sam Li

Stefan Hajnoczi  于2022年8月30日周二 03:44写道：
>
> On Fri, Aug 26, 2022 at 11:15:29PM +0800, Sam Li wrote:
> > Zoned Block Devices (ZBDs) devide the LBA space to block regions called 
> > zones
> > that are larger than the LBA size. It can only allow sequential writes, 
> > which
> > reduces write amplification in SSD, leading to higher throughput and 
> > increased
> > capacity. More details about ZBDs can be found at:
> >
> > https://zonedstorage.io/docs/introduction/zoned-storage
> >
> > The zoned device support aims to let guests (virtual machines) access zoned
> > storage devices on the host (hypervisor) through a virtio-blk device. This
> > involves extending QEMU's block layer and virtio-blk emulation code.  In its
> > current status, the virtio-blk device is not aware of ZBDs but the guest 
> > sees
> > host-managed drives as regular drive that will runs correctly under the most
> > common write workloads.
> >
> > This patch series extend the block layer APIs with the minimum set of zoned
> > commands that are necessary to support zoned devices. The commands are - 
> > Report
> > Zones, four zone operations and Zone Append (developing).
> >
> > It can be tested on a null_blk device using qemu-io or qemu-iotests. For
> > example, the command line for zone report using qemu-io is:
> > $ path/to/qemu-io --image-opts -n 
> > driver=zoned_host_device,filename=/dev/nullb0
> > -c "zrp offset nr_zones"
> >
> > v8:
> > - address review comments
> >   * solve patch conflicts and merge sysfs helper funcations into one patch
> >   * add cache.direct=on check in config
>
> Hi Sam,
> I have left a few comments.

That's great! Thanks for reviewing. I'll send a revision soon.

Sam

Re: [PATCH 1/5] Update version for v7.1.0-rc4 release

2022-08-29 Thread Dmitry Osipenko

On 8/29/22 18:40, Antonio Caggiano wrote:
> From: Richard Henderson 
> 
> Signed-off-by: Richard Henderson 
> ---
>  VERSION | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/VERSION b/VERSION
> index 1c944b9863..b8d5f3ebb6 100644
> --- a/VERSION
> +++ b/VERSION
> @@ -1 +1 @@
> -7.0.93
> +7.0.94

This patch shouldn't be here.

-- 
Best regards,
Dmitry

Re: [PATCH v1 13/25] gitlab-ci/custom-runners: Disable -static-pie for ubuntu-20.04-aarch64

2022-08-29 Thread Richard Henderson


On 8/29/22 16:16, Philippe Mathieu-Daudé wrote:
Shouldn't "--extra-cflags='-fno-pie -no-pie'" be handled by the configure script while 
processing the --disable-pie option?


I think configure just passes b_pie=off to meson, but yes, this could be improved -- 
there's definitely a disconnect somewhere.



r~

Re: [PATCH v1 15/25] Deprecate 32 bit big-endian MIPS

2022-08-29 Thread Philippe Mathieu-Daudé via


Hi Alex,

(+Aleksandar/Huacai)

On 26/8/22 19:21, Alex Bennée wrote:

It's becoming harder to maintain a cross-compiler to test this host
architecture as the old stable Debian 10 ("Buster") moved into LTS
which supports fewer architectures. For now:

   - mark it's deprecation in the docs
   - downgrade the containers to build TCG tests only
   - drop the cross builds from our CI

Users with an appropriate toolchain and user-space can still take
their chances building it.

Signed-off-by: Alex Bennée 
---
  docs/about/build-platforms.rst|  2 +-
  docs/about/deprecated.rst | 13 ++
  .gitlab-ci.d/container-cross.yml  |  1 -
  .gitlab-ci.d/crossbuilds.yml  | 14 ---
  tests/docker/Makefile.include |  5 +--
  .../dockerfiles/debian-mips-cross.docker  | 40 +--
  6 files changed, 27 insertions(+), 48 deletions(-)

diff --git a/docs/about/build-platforms.rst b/docs/about/build-platforms.rst
index 26028756d0..1ca9144a7d 100644
--- a/docs/about/build-platforms.rst
+++ b/docs/about/build-platforms.rst
@@ -41,7 +41,7 @@ Those hosts are officially supported, with various 
accelerators:
   - Accelerators
 * - Arm
   - kvm (64 bit only), tcg, xen
-   * - MIPS
+   * - MIPS (LE only)
   - kvm, tcg
 * - PPC
   - kvm, tcg
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 91b03115ee..22c2f4f4de 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -213,6 +213,19 @@ MIPS ``Trap-and-Emul`` KVM support (since 6.0)
  The MIPS ``Trap-and-Emul`` KVM host and guest support has been removed
  from Linux upstream kernel, declare it deprecated.
  
+Host Architectures

+--
+
+BE MIPS (since 7.2)
+'''
+
+A Debian 10 ("Buster") moved into LTS the big endian 32 bit version of
+MIPS moved out of support making it hard to maintain our
+cross-compilation CI tests of the architecture. As we no longer have
+CI coverage support may bitrot away before the deprecation process
+completes. The little endian variants of MIPS (both 32 and 64 bit) are
+still a supported host architecture.


For completeness we should update meson.build to consider 
host_machine.endian() and adapt this section:



  if not supported_cpus.contains(cpu)
message()
warning('SUPPORT FOR THIS HOST CPU WILL GO AWAY IN FUTURE RELEASES!')
message()
message('CPU host architecture ' + cpu + ' support is not currently 
maintained.')

  ...

This can be done later, and I might be able to do so in few weeks,
so meanwhile (with Thomas comment addressed):
Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH v1 13/25] gitlab-ci/custom-runners: Disable -static-pie for ubuntu-20.04-aarch64

2022-08-29 Thread Philippe Mathieu-Daudé via


On 26/8/22 19:21, Alex Bennée wrote:

From: Richard Henderson 

The project has reached the magic size at which we see

/usr/aarch64-linux-gnu/lib/libc.a(init-first.o): in function 
`__libc_init_first':
(.text+0x10): relocation truncated to fit: R_AARCH64_LD64_GOTPAGE_LO15 against \
symbol `__environ' defined in .bss section in 
/usr/aarch64-linux-gnu/lib/libc.a(environ.o)
/usr/bin/ld: (.text+0x10): warning: too many GOT entries for -fpic, please 
recompile with -fPIC

The bug has been reported upstream, but in the meantime there is
nothing we can do except build a non-pie executable.

Signed-off-by: Richard Henderson 
Signed-off-by: Alex Bennée 
Message-Id: <20220823210329.1969895-1-richard.hender...@linaro.org>
---
  .gitlab-ci.d/custom-runners/ubuntu-20.04-aarch64.yml | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/.gitlab-ci.d/custom-runners/ubuntu-20.04-aarch64.yml 
b/.gitlab-ci.d/custom-runners/ubuntu-20.04-aarch64.yml
index 3d878914e7..85a234801a 100644
--- a/.gitlab-ci.d/custom-runners/ubuntu-20.04-aarch64.yml
+++ b/.gitlab-ci.d/custom-runners/ubuntu-20.04-aarch64.yml
@@ -16,7 +16,9 @@ ubuntu-20.04-aarch64-all-linux-static:
   # --disable-glusterfs is needed because there's no static version of those 
libs in distro supplied packages
   - mkdir build
   - cd build
- - ../configure --enable-debug --static --disable-system --disable-glusterfs 
--disable-libssh
+ # Disable -static-pie due to build error with system libc:
+ # https://bugs.launchpad.net/ubuntu/+source/glibc/+bug/1987438
+ - ../configure --enable-debug --static --disable-system --disable-glusterfs 
--disable-libssh --disable-pie --extra-cflags='-fno-pie -no-pie'


Shouldn't "--extra-cflags='-fno-pie -no-pie'" be handled by the 
configure script while processing the --disable-pie option?

Re: [PATCH v1 12/25] tests/vm: Remove obsolete Fedora VM test

2022-08-29 Thread Philippe Mathieu-Daudé via


On 26/8/22 19:21, Alex Bennée wrote:

From: Thomas Huth 

It's still based on Fedora 30 - which is not supported anymore by QEMU
since years. Seems like nobody is using (and refreshing) this, and it's
easier to test this via a container anyway, so let's remove this now.

Signed-off-by: Thomas Huth 
Message-Id: <20220822175317.190551-1-th...@redhat.com>
Signed-off-by: Alex Bennée 
---
  tests/vm/Makefile.include |   3 +-
  tests/vm/fedora   | 190 --
  2 files changed, 1 insertion(+), 192 deletions(-)
  delete mode 100755 tests/vm/fedora


Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH] MAINTAINERS: Update Akihiko Odaki's email address

2022-08-29 Thread Philippe Mathieu-Daudé via


On 29/8/22 10:31, Akihiko Odaki wrote:

I am now employed by Daynix. Although my role as a reviewer of
macOS-related change is not very relevant to the employment, I decided
to use the company email address to avoid confusions from different
addresses.

Signed-off-by: Akihiko Odaki 
---
  MAINTAINERS | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH] tests/avocado/migration: Get find_free_port() from the ports

2022-08-29 Thread Philippe Mathieu-Daudé via


On 29/8/22 14:19, Thomas Huth wrote:

In upstream Avocado, the find_free_port() function is not available
from "network" anymore, but must be used via "ports", see:

  https://github.com/avocado-framework/avocado/commit/22fc98c6ff76cc55c48

To be able to update to a newer Avocado version later, let's use
the new way for accessing the find_free_port() function here.

Signed-off-by: Thomas Huth 
---
  tests/avocado/migration.py | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH 1/1] target/i386: Raise #GP on unaligned m128 accesses when required.

2022-08-29 Thread Richard Henderson

On 8/29/22 13:46, Ricky Zhou wrote:

Thanks for taking a look at this - did you see the bit in the cover
letter where I discuss doing this via alignment requirements on the
memory operation? My logic was that the memop alignment checks seem to
be more oriented towards triggering #AC exceptions (even though this is
not currently implemented),

I missed that in the cover. However... implementing #AC is pretty hypothetical. It's not
something that I've ever seen used, and not something that anyone has asked for.

One slightly more involved way to use alignment on the MemOp could be to
arrange to pass the problematic MemOp to do_unaligned_access and
helper_unaligned_{ld,st}. Then we could allow CPUs to handle
misalignment of different MemOps differently (e.g. raise #GP/SIGSEGV for
certain ops and #AC/SIGBUS for others). For this change to x86, we could
maybe get away with making MO_ALIGN_16 and above trigger #GP/SIGSEGV and
everything else trigger #AC/SIGBUS. If that's a little hacky, we could
instead add some dedicated bits to MemOp that distinguish different
types of unaligned accesses.

There's another related problem that actually has gotten a bug report in the past: when
the form of the address should raise #SS instead of #GP in system mode.

My initial thought was to record information about "the" memory access in the per-insn
unwind info, until I realized that there are insns with multiple memory operations
requiring different treatment. E.g. "push (%rax)", where the read might raise #GP and the
write might raise #SS. So I think we'd need to encode #GP vs #SS into the mmu_idx used
(e.g. in the lsb).

However, I don't think there are any similar situations of multiple memory types affecting
SSE, so #AC vs #GP could in fact be encoded into the per-insn unwind info.

As for SIGBUS vs SIGSEGV for SSE and user-only, you only need implement the
x86_cpu_ops.record_sigbus hook. C.f. the s390x version which raises PGM_SPECIFICATION ->
SIGILL for unaligned atomic operations.

Re: [PATCH for-7.2 v4 15/21] qmp/hmp, device_tree.c: introduce 'info fdt' command

2022-08-29 Thread Daniel Henrique Barboza





On 8/29/22 00:34, David Gibson wrote:

On Fri, Aug 26, 2022 at 11:11:44AM -0300, Daniel Henrique Barboza wrote:

Reading the FDT requires that the user saves the fdt_blob and then use
'dtc' to read the contents. Saving the file and using 'dtc' is a strong
use case when we need to compare two FDTs, but it's a lot of steps if
you want to do quick check on a certain node or property.

'info fdt' retrieves FDT nodes (and properties, later on) and print it
to the user. This can be used to check the FDT on running machines
without having to save the blob and use 'dtc'.

The implementation is based on the premise that the machine thas a FDT
created using libfdt and pointed by 'machine->fdt'. As long as this
pre-requisite is met the machine should be able to support it.

For now we're going to add the required QMP/HMP boilerplate and the
capability of printing the name of the properties of a given node. Next
patches will extend 'info fdt' to be able to print nodes recursively,
and then individual properties.

This command will always be executed in-band (i.e. holding BQL),
avoiding potential race conditions with machines that might change the
FDT during runtime (e.g. PowerPC 'pseries' machine).

'info fdt' is not something that we expect to be used aside from debugging,
so we're implementing it in QMP as 'x-query-fdt'.

This is an example of 'info fdt' fetching the '/chosen' node of the
pSeries machine:

(qemu) info fdt /chosen
chosen {
 ibm,architecture-vec-5;
 rng-seed;
 ibm,arch-vec-5-platform-support;
 linux,pci-probe-only;
 stdout-path;
 linux,stdout-path;
 qemu,graphic-depth;
 qemu,graphic-height;
 qemu,graphic-width;
};

And the same node for the aarch64 'virt' machine:

(qemu) info fdt /chosen
chosen {
 stdout-path;
 rng-seed;
 kaslr-seed;
};


So, I'm reasonably convinced allowing dumping the whole dtb from
qmp/hmp is useful.  I'm less convined that info fdt is worth the
additional complexity it incurs.  Note that as well as being able to
decompile a whole dtb using dtc, you can also extract and list
specific properties from a dtb blob using the 'fdtget' tool which is
part of the dtc tree.


What's your opinion on patch 21/21, where 'dumpdtb' can write a formatted
FDT in a file with an extra option? That was possible because of the
format helpers introduced for 'info fdt'. The idea is that since we're
able to format a FDT in DTS format, we can also write the FDT in text
format without relying on DTC to decode it.

If we think that this 'dumpdtb' capability is worth having, I can respin
the patches without 'info fdt' but adding these helpers to enable this
'dumpdtb' support. If not, then we can just remove patches 15-21 and
be done with it.


Thanks,


Daniel





Cc: Dr. David Alan Gilbert 
Acked-by: Dr. David Alan Gilbert 
Signed-off-by: Daniel Henrique Barboza 
---
  hmp-commands-info.hx | 13 ++
  include/monitor/hmp.h|  1 +
  include/sysemu/device_tree.h |  4 +++
  monitor/hmp-cmds.c   | 13 ++
  monitor/qmp-cmds.c   | 12 +
  qapi/machine.json| 19 +++
  softmmu/device_tree.c| 47 
  7 files changed, 109 insertions(+)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 188d9ece3b..743b48865d 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -921,3 +921,16 @@ SRST
``stats``
  Show runtime-collected statistics
  ERST
+
+{
+.name   = "fdt",
+.args_type  = "nodepath:s",
+.params = "nodepath",
+.help   = "show firmware device tree node given its path",
+.cmd= hmp_info_fdt,
+},
+
+SRST
+  ``info fdt``
+Show a firmware device tree node given its path. Requires libfdt.
+ERST
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index d7f324da59..c0883dd1e3 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -135,6 +135,7 @@ void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict 
*qdict);
  void hmp_cancel_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
  void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
  void hmp_dumpdtb(Monitor *mon, const QDict *qdict);
+void hmp_info_fdt(Monitor *mon, const QDict *qdict);
  void hmp_human_readable_text_helper(Monitor *mon,
  HumanReadableText *(*qmp_handler)(Error 
**));
  void hmp_info_stats(Monitor *mon, const QDict *qdict);
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index bf7684e4ed..057d13e397 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -14,6 +14,8 @@
  #ifndef DEVICE_TREE_H
  #define DEVICE_TREE_H
  
+#include "qapi/qapi-types-common.h"

+
  void *create_device_tree(int *sizep);
  void *load_device_tree(const char *filename_path, int *sizep);
  #ifdef CONFIG_LINUX
@@ -137,6 +139,8 @@ int qemu_fdt_add_path(void *fdt, const char *path);
  
  void

Re: [PATCH] ui/console: Get tab completion working again in the SDL monitor vc

2022-08-29 Thread Cal Peake

Hi Gerd,

Can you take a look at this and let me know what you think?

Thanks,
-Cal


On Thu, 11 Aug 2022, Cal Peake wrote:

> Define a QEMU special key constant for the tab key and add an entry for
> it in the qcode_to_keysym table. This allows tab completion to work again
> in the SDL monitor virtual console, which has been broken ever since the
> migration from SDL1 to SDL2.
> 
> Signed-off-by: Cal Peake 
> ---
>  include/ui/console.h | 1 +
>  ui/console.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/include/ui/console.h b/include/ui/console.h
> index c0520c694c..e400ee9fa7 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -70,6 +70,7 @@ void hmp_mouse_set(Monitor *mon, const QDict *qdict);
>  /* keysym is a unicode code except for special keys (see QEMU_KEY_xxx
> constants) */
>  #define QEMU_KEY_ESC1(c) ((c) | 0xe100)
> +#define QEMU_KEY_TAB0x0009
>  #define QEMU_KEY_BACKSPACE  0x007f
>  #define QEMU_KEY_UP QEMU_KEY_ESC1('A')
>  #define QEMU_KEY_DOWN   QEMU_KEY_ESC1('B')
> diff --git a/ui/console.c b/ui/console.c
> index e139f7115e..addaafba28 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1368,6 +1368,7 @@ static const int qcode_to_keysym[Q_KEY_CODE__MAX] = {
>  [Q_KEY_CODE_PGUP]   = QEMU_KEY_PAGEUP,
>  [Q_KEY_CODE_PGDN]   = QEMU_KEY_PAGEDOWN,
>  [Q_KEY_CODE_DELETE] = QEMU_KEY_DELETE,
> +[Q_KEY_CODE_TAB]= QEMU_KEY_TAB,
>  [Q_KEY_CODE_BACKSPACE] = QEMU_KEY_BACKSPACE,
>  };
>  
> -- 
> 2.35.3
> 
>

Re: [PATCH 1/1] target/i386: Raise #GP on unaligned m128 accesses when required.

2022-08-29 Thread Ricky Zhou

On Mon, Aug 29, 2022 at 9:45 AM Richard Henderson
 wrote:
>
> On 8/29/22 07:23, Ricky Zhou wrote:
> This trap should be raised via the memory operation:
> ...
> Only the first of the two loads/stores must be aligned, as the other is known 
> to be +8.
> You then must fill in the x86_tcg_ops.do_unaligned_access hook to raise #GP.
Thanks for taking a look at this - did you see the bit in the cover
letter where I discuss doing this via alignment requirements on the
memory operation? My logic was that the memop alignment checks seem to
be more oriented towards triggering #AC exceptions (even though this is
not currently implemented), since qemu-user's unaligned access handlers
(helper_unaligned_{ld,st}) already trigger SIGBUS as opposed to SIGSEGV.
I was concerned that implementing this via MO_ALIGN_16 would get in the
way of a hypothetical future implementation of the AC flag, since
do_unaligned_access would need to raise #AC instead of #GP for that.

One slightly more involved way to use alignment on the MemOp could be to
arrange to pass the problematic MemOp to do_unaligned_access and
helper_unaligned_{ld,st}. Then we could allow CPUs to handle
misalignment of different MemOps differently (e.g. raise #GP/SIGSEGV for
certain ops and #AC/SIGBUS for others). For this change to x86, we could
maybe get away with making MO_ALIGN_16 and above trigger #GP/SIGSEGV and
everything else trigger #AC/SIGBUS. If that's a little hacky, we could
instead add some dedicated bits to MemOp that distinguish different
types of unaligned accesses.

What do you think? Happy to implement whichever approach is preferred!

Thanks,
Ricky

Re: [PATCH v5 09/18] dump: Use a buffer for ELF section data and headers

2022-08-29 Thread Janis Schoetterl-Glausch

On Thu, 2022-08-11 at 12:11 +, Janosch Frank wrote:
> Currently we're writing the NULL section header if we overflow the
> physical header number in the ELF header. But in the future we'll add
> custom section headers AND section data.
> 
> To facilitate this we need to rearange section handling a bit. As with
> the other ELF headers we split the code into a prepare and a write
> step.
> 
> Signed-off-by: Janosch Frank 
> ---
>  dump/dump.c   | 83 +--
>  include/sysemu/dump.h |  2 ++
>  2 files changed, 58 insertions(+), 27 deletions(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index a905316fe5..0051c71d08 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -380,30 +380,57 @@ static void write_elf_phdr_note(DumpState *s, Error 
> **errp)
>  }
>  }
>  
> -static void write_elf_section(DumpState *s, int type, Error **errp)
> +static void prepare_elf_section_hdr_zero(DumpState *s)
>  {
> -Elf32_Shdr shdr32;
> -Elf64_Shdr shdr64;
> -int shdr_size;
> -void *shdr;
> +if (dump_is_64bit(s)) {
> +Elf64_Shdr *shdr64 = s->elf_section_hdrs;
> +
> +shdr64->sh_info = cpu_to_dump32(s, s->phdr_num);
> +} else {
> +Elf32_Shdr *shdr32 = s->elf_section_hdrs;
> +
> +shdr32->sh_info = cpu_to_dump32(s, s->phdr_num);
> +}
> +}
> +
> +static void prepare_elf_section_hdrs(DumpState *s)
> +{
> +size_t len, sizeof_shdr;
> +
> +/*
> + * Section ordering:
> + * - HDR zero
> + */
> +sizeof_shdr = dump_is_64bit(s) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
> +len = sizeof_shdr * s->shdr_num;
> +s->elf_section_hdrs = g_malloc0(len);

I'm not seeing this being freed.
> +
> +/*
> + * The first section header is ALWAYS a special initial section
> + * header.
> + *
> + * The header should be 0 with one exception being that if
> + * phdr_num is PN_XNUM then the sh_info field contains the real
> + * number of segment entries.
> + *
> + * As we zero allocate the buffer we will only need to modify
> + * sh_info for the PN_XNUM case.
> + */
> +if (s->phdr_num >= PN_XNUM) {
> +prepare_elf_section_hdr_zero(s);
> +}
> +}
> +
> +static void write_elf_section_headers(DumpState *s, Error **errp)

[...]

> @@ -579,6 +606,12 @@ static void dump_begin(DumpState *s, Error **errp)
>  return;
>  }
>  
> +/* write section headers to vmcore */
> +write_elf_section_headers(s, errp);
> +if (*errp) {
> +return;
> +}
> +
>  /* write PT_NOTE to vmcore */
>  write_elf_phdr_note(s, errp);
>  if (*errp) {
> @@ -591,14 +624,6 @@ static void dump_begin(DumpState *s, Error **errp)
>  return;
>  }
>  
> -/* write section to vmcore */
> -if (s->shdr_num) {
> -write_elf_section(s, 1, errp);
> -if (*errp) {
> -return;
> -}
> -}
> -

Here you change the order of the headers, but the elf header is only
fixed in patch 11.
I agree that this should be a separate patch, with an explanation on
why it's necessary. 
So basically squashed into patch 11, except I think the comment change
in that one should go into another patch.

>  /* write notes to vmcore */
>  write_elf_notes(s, errp);
>  }
> @@ -674,7 +699,11 @@ static void create_vmcore(DumpState *s, Error **errp)
>  return;
>  }
>  
> +/* Iterate over memory and dump it to file */
>  dump_iterate(s, errp);
> +if (*errp) {
> +return;
> +}
>  }
>  
>  static int write_start_flat_header(int fd)
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index b62513d87d..9995f65dc8 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -177,6 +177,8 @@ typedef struct DumpState {
>  int64_t filter_area_begin;  /* Start address of partial guest memory 
> area */
>  int64_t filter_area_length; /* Length of partial guest memory area */
>  
> +void *elf_section_hdrs; /* Pointer to section header buffer */
> +
>  uint8_t *note_buf;  /* buffer for notes */
>  size_t note_buf_offset; /* the writing place in note_buf */
>  uint32_t nr_cpus;   /* number of guest's cpu */

Re: [PATCH v5 04/18] dump: Rework get_start_block

2022-08-29 Thread Janis Schoetterl-Glausch

On Thu, 2022-08-11 at 12:10 +, Janosch Frank wrote:
> get_start_block() returns the start address of the first memory block
> or -1.
> 
> With the GuestPhysBlock iterator conversion we don't need to set the
> start address and can therefore remove that code and the "start"
> DumpState struct member. The only functionality left is the validation
> of the start block so it only makes sense to re-name the function to
> validate_start_block()

Nit, since you don't return an address anymore, I find retaining the -
1/0 return value instead of true/false weird.
> 
> Signed-off-by: Janosch Frank 
> Reviewed-by: Marc-André Lureau 
> Reviewed-by: Janis Schoetterl-Glausch 
> ---
>  dump/dump.c   | 20 ++--
>  include/sysemu/dump.h |  2 --
>  2 files changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index 340de5a1e7..e204912a89 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1500,30 +1500,22 @@ static void create_kdump_vmcore(DumpState *s, Error 
> **errp)
>  }
>  }
>  
> -static ram_addr_t get_start_block(DumpState *s)
> +static int validate_start_block(DumpState *s)
>  {
>  GuestPhysBlock *block;
>  
>  if (!s->has_filter) {
> -s->next_block = QTAILQ_FIRST(>guest_phys_blocks.head);
>  return 0;
>  }
>  
>  QTAILQ_FOREACH(block, >guest_phys_blocks.head, next) {
> +/* This block is out of the range */
>  if (block->target_start >= s->begin + s->length ||
>  block->target_end <= s->begin) {
> -/* This block is out of the range */
>  continue;
>  }
> -
> -s->next_block = block;
> -if (s->begin > block->target_start) {
> -s->start = s->begin - block->target_start;
> -} else {
> -s->start = 0;
> -}
> -return s->start;
> -}
> +return 0;
> +   }
>  
>  return -1;
>  }
> @@ -1670,8 +1662,8 @@ static void dump_init(DumpState *s, int fd, bool 
> has_format,
>  goto cleanup;
>  }
>  
> -s->start = get_start_block(s);
> -if (s->start == -1) {
> +/* Is the filter filtering everything? */
> +if (validate_start_block(s) == -1) {
>  error_setg(errp, QERR_INVALID_PARAMETER, "begin");
>  goto cleanup;
>  }
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index ffc2ea1072..7fce1d4af6 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -166,8 +166,6 @@ typedef struct DumpState {
>  hwaddr memory_offset;
>  int fd;
>  
> -GuestPhysBlock *next_block;
> -ram_addr_t start;
>  bool has_filter;
>  int64_t begin;
>  int64_t length;

Re: [PATCH v8 0/7] Add support for zoned device

2022-08-29 Thread Stefan Hajnoczi

On Fri, Aug 26, 2022 at 11:15:29PM +0800, Sam Li wrote:
> Zoned Block Devices (ZBDs) devide the LBA space to block regions called zones
> that are larger than the LBA size. It can only allow sequential writes, which
> reduces write amplification in SSD, leading to higher throughput and increased
> capacity. More details about ZBDs can be found at:
> 
> https://zonedstorage.io/docs/introduction/zoned-storage
> 
> The zoned device support aims to let guests (virtual machines) access zoned
> storage devices on the host (hypervisor) through a virtio-blk device. This
> involves extending QEMU's block layer and virtio-blk emulation code.  In its
> current status, the virtio-blk device is not aware of ZBDs but the guest sees
> host-managed drives as regular drive that will runs correctly under the most
> common write workloads.
> 
> This patch series extend the block layer APIs with the minimum set of zoned
> commands that are necessary to support zoned devices. The commands are - 
> Report
> Zones, four zone operations and Zone Append (developing).
> 
> It can be tested on a null_blk device using qemu-io or qemu-iotests. For
> example, the command line for zone report using qemu-io is:
> $ path/to/qemu-io --image-opts -n 
> driver=zoned_host_device,filename=/dev/nullb0
> -c "zrp offset nr_zones"
> 
> v8:
> - address review comments
>   * solve patch conflicts and merge sysfs helper funcations into one patch
>   * add cache.direct=on check in config

Hi Sam,
I have left a few comments.

Stefan


signature.asc
Description: PGP signature

Re: [PATCH v8 3/7] block: add block layer APIs resembling Linux ZonedBlockDevice ioctls

2022-08-29 Thread Stefan Hajnoczi

On Sat, Aug 27, 2022 at 12:17:04AM +0800, Sam Li wrote:
> +/*
> + * Send a zone_management command.
> + * op is the zone operation.
> + * offset is the starting zone specified as a sector offset.

Does "sector offset" mean "byte offset from the start of the device" or
does it mean in 512B sector units? For consistency this should be in
bytes.

> + * len is the maximum number of sectors the command should operate on. It
> + * should be aligned with the zone sector size.

Please use bytes for consistency with QEMU's block layer APIs.

> @@ -3022,6 +3183,118 @@ static void raw_account_discard(BDRVRawState *s, 
> uint64_t nbytes, int ret)
>  }
>  }
>  
> +/*
> + * zone report - Get a zone block device's information in the form
> + * of an array of zone descriptors.
> + *
> + * @param bs: passing zone block device file descriptor
> + * @param zones: an array of zone descriptors to hold zone
> + * information on reply
> + * @param offset: offset can be any byte within the zone size.

This isn't an offset within a zone, it's an offset within the entire
device, so I think "zone size" is confusing here.

> + * @param len: (not sure yet.

Please remove this and document nr_zones instead.

> + * @return 0 on success, -1 on failure
> + */
> +static int coroutine_fn raw_co_zone_report(BlockDriverState *bs, int64_t 
> offset,
> +   unsigned int *nr_zones,
> +   BlockZoneDescriptor *zones) {
> +#if defined(CONFIG_BLKZONED)
> +BDRVRawState *s = bs->opaque;
> +RawPosixAIOData acb;
> +
> +acb = (RawPosixAIOData) {
> +.bs = bs,
> +.aio_fildes = s->fd,
> +.aio_type   = QEMU_AIO_ZONE_REPORT,
> +.aio_offset = offset,
> +.zone_report= {
> +.nr_zones   = nr_zones,
> +.zones  = zones,
> +},
> +};
> +
> +return raw_thread_pool_submit(bs, handle_aiocb_zone_report, );
> +#else
> +return -ENOTSUP;
> +#endif
> +}
> +
> +/*
> + * zone management operations - Execute an operation on a zone
> + */
> +static int coroutine_fn raw_co_zone_mgmt(BlockDriverState *bs, BlockZoneOp 
> op,
> +int64_t offset, int64_t len) {
> +#if defined(CONFIG_BLKZONED)
> +BDRVRawState *s = bs->opaque;
> +RawPosixAIOData acb;
> +int64_t zone_sector, zone_sector_mask;
> +const char *ioctl_name;
> +unsigned long zone_op;
> +int ret;
> +
> +struct stat st;
> +if (fstat(s->fd, ) < 0) {
> +ret = -errno;
> +return ret;
> +}

st is not used and can be removed.

> +zone_sector = bs->bl.zone_sectors;
> +zone_sector_mask = zone_sector - 1;
> +if (offset & zone_sector_mask) {
> +error_report("sector offset %" PRId64 " is not aligned to zone size "
> + "%" PRId64 "", offset, zone_sector);
> +return -EINVAL;
> +}
> +
> +if (len & zone_sector_mask) {
> +error_report("number of sectors %" PRId64 " is not aligned to zone 
> size"
> +  " %" PRId64 "", len, zone_sector);
> +return -EINVAL;
> +}
> +
> +switch (op) {
> +case BLK_ZO_OPEN:
> +ioctl_name = "BLKOPENZONE";
> +zone_op = BLKOPENZONE;
> +break;
> +case BLK_ZO_CLOSE:
> +ioctl_name = "BLKCLOSEZONE";
> +zone_op = BLKCLOSEZONE;
> +break;
> +case BLK_ZO_FINISH:
> +ioctl_name = "BLKFINISHZONE";
> +zone_op = BLKFINISHZONE;
> +break;
> +case BLK_ZO_RESET:
> +ioctl_name = "BLKRESETZONE";
> +zone_op = BLKRESETZONE;
> +break;
> +default:
> +error_report("Invalid zone operation 0x%x", op);
> +return -EINVAL;
> +}
> +
> +acb = (RawPosixAIOData) {
> +.bs = bs,
> +.aio_fildes = s->fd,
> +.aio_type   = QEMU_AIO_ZONE_MGMT,
> +.aio_offset = offset,
> +.aio_nbytes = len,
> +.zone_mgmt  = {
> +.zone_op = zone_op,
> +},
> +};
> +
> +ret = raw_thread_pool_submit(bs, handle_aiocb_zone_mgmt, );
> +if (ret != 0) {
> +error_report("ioctl %s failed %d", ioctl_name, errno);
> +return -errno;

ret contains a negative errno value. The errno variable is not used by
raw_thread_pool_submit().

I suggest simplifying it to:

  return raw_thread_pool_submit(bs, handle_aiocb_zone_mgmt, );

That's what most of the other raw_thread_pool_submit() callers.


signature.asc
Description: PGP signature

Re: [PATCH v8 2/7] file-posix: introduce helper funcations for sysfs attributes

2022-08-29 Thread Stefan Hajnoczi

On Sat, Aug 27, 2022 at 12:11:21AM +0800, Sam Li wrote:

If you send another revision please fix the "funcations" typo in the
commit message.


signature.asc
Description: PGP signature

Re: [PATCH 4/9] hw/isa/vt82c686: QOM'ify via-ide creation

2022-08-29 Thread BB




Am 29. August 2022 19:04:06 MESZ schrieb BALATON Zoltan :
>On Mon, 29 Aug 2022, BB wrote:
>> Am 25. August 2022 01:18:56 MESZ schrieb BALATON Zoltan :
>>> On Thu, 25 Aug 2022, Bernhard Beschow wrote:
 On Wed, Aug 24, 2022 at 3:54 PM BALATON Zoltan  wrote:
> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>> The IDE function is closely tied to the ISA function (e.g. the IDE
>> interrupt routing happens there), so it makes sense that the IDE
>> function is instantiated within the southbridge itself. As a side effect,
>> duplicated code in the boards is resolved.
>> 
>> Signed-off-by: Bernhard Beschow 
>> ---
>> configs/devices/mips64el-softmmu/default.mak |  1 -
>> hw/isa/Kconfig   |  1 +
>> hw/isa/vt82c686.c| 18 ++
>> hw/mips/fuloong2e.c  |  3 ---
>> hw/ppc/Kconfig   |  1 -
>> hw/ppc/pegasos2.c|  4 
>> 6 files changed, 19 insertions(+), 9 deletions(-)
>> 
>> diff --git a/configs/devices/mips64el-softmmu/default.mak
> b/configs/devices/mips64el-softmmu/default.mak
>> index c610749ac1..d5188f7ea5 100644
>> --- a/configs/devices/mips64el-softmmu/default.mak
>> +++ b/configs/devices/mips64el-softmmu/default.mak
>> @@ -1,7 +1,6 @@
>> # Default configuration for mips64el-softmmu
>> 
>> include ../mips-softmmu/common.mak
>> -CONFIG_IDE_VIA=y
>> CONFIG_FULOONG=y
>> CONFIG_LOONGSON3V=y
>> CONFIG_ATI_VGA=y
>> diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
>> index d42143a991..20de7e9294 100644
>> --- a/hw/isa/Kconfig
>> +++ b/hw/isa/Kconfig
>> @@ -53,6 +53,7 @@ config VT82C686
>> select I8254
>> select I8257
>> select I8259
>> +select IDE_VIA
>> select MC146818RTC
>> select PARALLEL
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 5582c0b179..37d9ed635d 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -17,6 +17,7 @@
>> #include "hw/isa/vt82c686.h"
>> #include "hw/pci/pci.h"
>> #include "hw/qdev-properties.h"
>> +#include "hw/ide/pci.h"
>> #include "hw/isa/isa.h"
>> #include "hw/isa/superio.h"
>> #include "hw/intc/i8259.h"
>> @@ -544,6 +545,7 @@ struct ViaISAState {
>> qemu_irq cpu_intr;
>> qemu_irq *isa_irqs;
>> ViaSuperIOState via_sio;
>> +PCIIDEState ide;
>> };
>> 
>> static const VMStateDescription vmstate_via = {
>> @@ -556,10 +558,18 @@ static const VMStateDescription vmstate_via = {
>> }
>> };
>> 
>> +static void via_isa_init(Object *obj)
>> +{
>> +ViaISAState *s = VIA_ISA(obj);
>> +
>> +object_initialize_child(obj, "ide", >ide, "via-ide");
>> +}
>> +
>> static const TypeInfo via_isa_info = {
>> .name  = TYPE_VIA_ISA,
>> .parent= TYPE_PCI_DEVICE,
>> .instance_size = sizeof(ViaISAState),
>> +.instance_init = via_isa_init,
>> .abstract  = true,
>> .interfaces= (InterfaceInfo[]) {
>> { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>> @@ -583,6 +593,7 @@ static void via_isa_realize(PCIDevice *d, Error
> **errp)
>> {
>> ViaISAState *s = VIA_ISA(d);
>> DeviceState *dev = DEVICE(d);
>> +PCIBus *pci_bus = pci_get_bus(d);
>> qemu_irq *isa_irq;
>> ISABus *isa_bus;
>> int i;
>> @@ -607,6 +618,13 @@ static void via_isa_realize(PCIDevice *d, Error
> **errp)
>> if (!qdev_realize(DEVICE(>via_sio), BUS(isa_bus), errp)) {
>> return;
>> }
>> +
>> +/* Function 1: IDE */
>> +qdev_prop_set_int32(DEVICE(>ide), "addr", d->devfn + 1);
>> +if (!qdev_realize(DEVICE(>ide), BUS(pci_bus), errp)) {
>> +return;
>> +}
>> +pci_ide_create_devs(PCI_DEVICE(>ide));
> 
> I'm not sure about moving pci_ide_create_devs() here. This is usally
> called from board code and only piix4 seems to do this. Maybe that's wrong
> because if all IDE devices did this then one machine could not have more
> than one different ide devices (like having an on-board ide and adding a
> pci ide controoler with -device) so this probably belongs to the board
> code to add devices to its default ide controller only as this is machine
> specific. Unless I'm wrong in which case somebody will correct me.
> 
 
 Grepping the code it can be seen that it's always called right after
 creating the IDE controllers. The only notable exception is the "sii3112"
 device in the sam460ex board which is not emulated yet. Since the IDE
>>> 
>>> The problem with sii3112 is that it only has 2 channels becuase I did not 
>>> bother to implement more so pci_ide_create_devs() probably would not

Re: [PATCH 8/9] hw/isa/vt82c686: QOM'ify RTC creation

2022-08-29 Thread BB




Am 29. August 2022 19:50:10 MESZ schrieb BALATON Zoltan :
>On Mon, 29 Aug 2022, BB wrote:
>> Am 24. August 2022 01:23:14 MESZ schrieb BALATON Zoltan :
>>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
 On Tue, Aug 23, 2022 at 2:20 AM BALATON Zoltan  wrote:
> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>> Signed-off-by: Bernhard Beschow 
>> ---
>> hw/isa/vt82c686.c | 12 +++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 47f2fd2669..ee745d5d49 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -546,6 +546,7 @@ struct ViaISAState {
>> qemu_irq cpu_intr;
>> qemu_irq *isa_irqs;
>> ViaSuperIOState via_sio;
>> +RTCState rtc;
>> PCIIDEState ide;
>> UHCIState uhci[2];
>> ViaPMState pm;
>> @@ -567,6 +568,7 @@ static void via_isa_init(Object *obj)
>> {
>> ViaISAState *s = VIA_ISA(obj);
>> 
>> +object_initialize_child(obj, "rtc", >rtc, TYPE_MC146818_RTC);
>> object_initialize_child(obj, "ide", >ide, "via-ide");
>> object_initialize_child(obj, "uhci1", >uhci[0],
> "vt82c686b-usb-uhci");
>> object_initialize_child(obj, "uhci2", >uhci[1],
> "vt82c686b-usb-uhci");
>> @@ -615,7 +617,15 @@ static void via_isa_realize(PCIDevice *d, Error
> **errp)
>> isa_bus_irqs(isa_bus, s->isa_irqs);
>> i8254_pit_init(isa_bus, 0x40, 0, NULL);
>> i8257_dma_init(isa_bus, 0);
>> -mc146818_rtc_init(isa_bus, 2000, NULL);
>> +
>> +/* RTC */
>> +qdev_prop_set_int32(DEVICE(>rtc), "base_year", 2000);
>> +if (!qdev_realize(DEVICE(>rtc), BUS(isa_bus), errp)) {
>> +return;
>> +}
>> +object_property_add_alias(qdev_get_machine(), "rtc-time",
> OBJECT(>rtc),
>> +  "date");
>> +isa_connect_gpio_out(ISA_DEVICE(>rtc), 0, s->rtc.isairq);
>> 
>> for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
>> if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
>> 
> 
> This actually introduces code duplication as all other places except piix4
> seem to still use the init function (probably to ensure that the rtc-rime
> alias on the machine is properly set) so I'd keep this the same as
> everything else and drop this patch until this init function is removed
> from all other places as well.
> 
 
 Hi Zoltan,
 
 Thanks for the fast reply! Regarding code homogeneity and duplication I've
 made a similar argument for mc146818_rtc_init() in the past [1] and I've
 learnt that my patch went backwards. Incidentally, Peter mentioned vt686c
 as a candidate for the embed-the-device-struct style which - again
 incidentally - I've now done.
>>> 
>>> I've seen patches embedding devices recently but in this case it looked not 
>>> that simple because of the rtc-time alias.
>>> 
 The rtc-time alias is actually only used by a couple of PPC machines where
 Pegasos II is one of them. So the alias actually needs to be created only
 for these machines, and identifying the cases where it has to be preserved
 requires a lot of careful investigation. In the Pegasos II case this seems
 especially complicated since one needs to look through several layers of
 devices. During my work on the VT82xx south bridges I've gained some
 knowledge such that I'd like to make this simplifying contribution.
>>> 
>>> I've used it to implement the get-time-of-day rtas call with VOF in 
>>> pegasos2 because otherwise it would need to access internals of the RTC 
>>> model and/or duplicate some code. Here's the message discussing this:
>>> 
>>> https://lists.nongnu.org/archive/html/qemu-ppc/2021-10/msg00170.html
>>> 
>>> so this alias still seems to be the simplest way.
>>> 
>>> I think the primary function of this alias is not these ppc machines but 
>>> some QMP/HMP command or to make the guest time available from the monitor 
>>> or something like that so it's probably also used from there and therefore 
>>> all rtc probably should have it but I'm not sure about it.
>> 
>> Indeed, the alias seems to be a convenience for some QMP/HMP commands. 
>> AFAICS only the mc146818 sets the alias while it is probably not the only 
>> RTC modelled in QEMU. So I wonder why boards using another RTC don't need it 
>> and whether removing the alias constitutes a compatibility break.
>> 
 Our discussion makes me realize that the creation of the alias could now
 actually be moved to the Pegasos II board. This way, the Pegasos II board
 would both create and consume that alias, which seems to remove quite some
 cognitive load. Do you agree? Would moving the alias to the board work for
 you?
>>> 
>>> Yes I think that would be better. This way the vt82xx and piix4 would be 
>>> similar and the alias would also

Re: [PATCH 8/9] hw/isa/vt82c686: QOM'ify RTC creation

2022-08-29 Thread BALATON Zoltan


On Mon, 29 Aug 2022, BB wrote:

Am 24. August 2022 01:23:14 MESZ schrieb BALATON Zoltan :

On Tue, 23 Aug 2022, Bernhard Beschow wrote:

On Tue, Aug 23, 2022 at 2:20 AM BALATON Zoltan  wrote:

On Tue, 23 Aug 2022, Bernhard Beschow wrote:

Signed-off-by: Bernhard Beschow 
---
hw/isa/vt82c686.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 47f2fd2669..ee745d5d49 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -546,6 +546,7 @@ struct ViaISAState {
qemu_irq cpu_intr;
qemu_irq *isa_irqs;
ViaSuperIOState via_sio;
+RTCState rtc;
PCIIDEState ide;
UHCIState uhci[2];
ViaPMState pm;
@@ -567,6 +568,7 @@ static void via_isa_init(Object *obj)
{
ViaISAState *s = VIA_ISA(obj);

+object_initialize_child(obj, "rtc", >rtc, TYPE_MC146818_RTC);
object_initialize_child(obj, "ide", >ide, "via-ide");
object_initialize_child(obj, "uhci1", >uhci[0],

"vt82c686b-usb-uhci");

object_initialize_child(obj, "uhci2", >uhci[1],

"vt82c686b-usb-uhci");

@@ -615,7 +617,15 @@ static void via_isa_realize(PCIDevice *d, Error

**errp)

isa_bus_irqs(isa_bus, s->isa_irqs);
i8254_pit_init(isa_bus, 0x40, 0, NULL);
i8257_dma_init(isa_bus, 0);
-mc146818_rtc_init(isa_bus, 2000, NULL);
+
+/* RTC */
+qdev_prop_set_int32(DEVICE(>rtc), "base_year", 2000);
+if (!qdev_realize(DEVICE(>rtc), BUS(isa_bus), errp)) {
+return;
+}
+object_property_add_alias(qdev_get_machine(), "rtc-time",

OBJECT(>rtc),

+  "date");
+isa_connect_gpio_out(ISA_DEVICE(>rtc), 0, s->rtc.isairq);

for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {



This actually introduces code duplication as all other places except piix4
seem to still use the init function (probably to ensure that the rtc-rime
alias on the machine is properly set) so I'd keep this the same as
everything else and drop this patch until this init function is removed
from all other places as well.



Hi Zoltan,

Thanks for the fast reply! Regarding code homogeneity and duplication I've
made a similar argument for mc146818_rtc_init() in the past [1] and I've
learnt that my patch went backwards. Incidentally, Peter mentioned vt686c
as a candidate for the embed-the-device-struct style which - again
incidentally - I've now done.


I've seen patches embedding devices recently but in this case it looked not 
that simple because of the rtc-time alias.


The rtc-time alias is actually only used by a couple of PPC machines where
Pegasos II is one of them. So the alias actually needs to be created only
for these machines, and identifying the cases where it has to be preserved
requires a lot of careful investigation. In the Pegasos II case this seems
especially complicated since one needs to look through several layers of
devices. During my work on the VT82xx south bridges I've gained some
knowledge such that I'd like to make this simplifying contribution.


I've used it to implement the get-time-of-day rtas call with VOF in 
pegasos2 because otherwise it would need to access internals of the RTC 
model and/or duplicate some code. Here's the message discussing this:


https://lists.nongnu.org/archive/html/qemu-ppc/2021-10/msg00170.html

so this alias still seems to be the simplest way.

I think the primary function of this alias is not these ppc machines 
but some QMP/HMP command or to make the guest time available from the 
monitor or something like that so it's probably also used from there 
and therefore all rtc probably should have it but I'm not sure about 
it.


Indeed, the alias seems to be a convenience for some QMP/HMP commands. 
AFAICS only the mc146818 sets the alias while it is probably not the 
only RTC modelled in QEMU. So I wonder why boards using another RTC 
don't need it and whether removing the alias constitutes a compatibility 
break.



Our discussion makes me realize that the creation of the alias could now
actually be moved to the Pegasos II board. This way, the Pegasos II board
would both create and consume that alias, which seems to remove quite some
cognitive load. Do you agree? Would moving the alias to the board work for
you?


Yes I think that would be better. This way the vt82xx and piix4 would 
be similar and the alias would also be clear within the pegasos2 code 
and it also has the machine directly at that point so it's clearer that 
way.


All in all I wonder if we need to preserve the alias for the fuloong2e board?


I don't know. A quick investigation shows that it seems to be added by 
commit 654a36d857ff94 which suggests something may use it (or was intended 
to use it back then, but not sure if things have changed in the meantime). 
I don't think any management app cares about fuloong2e but if this should 
be a generic thing then all machine may need it. Then it's also mentioned 
in commit 29551fdcf4d99 that suggests one ought to

1 2 >

1 - 100 of 123 matches

Mail list logo