Re: [PATCH 0/4] ui/console: multihead: fix crash, simplify logic

[Mark Cave-Ayland]

On 30/09/2023 22:28, Laszlo Ersek wrote:


On 9/29/23 09:57, Mark Cave-Ayland wrote:

On 26/09/2023 09:00, Marc-André Lureau wrote:


Hi Laszlo

On Mon, Sep 25, 2023 at 7:36 PM Laszlo Ersek  wrote:

Has this been queued by someone? Both Gerd and Marc-André are "odd
fixers", so I'm not sure who should be sending a PR with these patches
(and I don't see a pending PULL at

with these patch subjects included).


I have the series in my "ui" branch. I was waiting for a few more
patches to be accumulated. But if someone else takes this first, I'll
drop them.


Does this series fix the "../ui/console.c:818: dpy_get_ui_info:
Assertion `dpy_ui_info_supported(con)' failed." assert() on startup when
using gtk? It would be good to get this fixed in git master soon, as it
has been broken for a couple of weeks now, and -display sdl has issues
tracking the mouse correctly on my laptop here :(


... probably not; I've never seen that issue. Can you provide a reproducer?


The environment is a standard Debian bookworm install building QEMU git master with 
QEMU gtk support. The only difference I can think of is that I do all my QEMU builds 
as a separate user, and then export the display to my current user desktop i.e.


As my current user:
  $ xhost +

As my QEMU build user:
  $ export DISPLAY=:1
  $ ./build/qemu-system-sparc
  qemu-system-sparc: ../ui/console.c:818: dpy_get_ui_info: Assertion
 `dpy_ui_info_supported(con)' failed.
  Aborted (core dumped)


Also, it should be bisectable (over Marc-André's 52-part series I guess).


Indeed. I've just run git bisect and it returns the following:

a92e7bb4cad57cc5c8817fb18fb25650507b69f8 is the first bad commit
commit a92e7bb4cad57cc5c8817fb18fb25650507b69f8
Author: Marc-André Lureau 
Date:   Tue Sep 12 10:13:01 2023 +0400

ui: add precondition for dpy_get_ui_info()

Ensure that it only get called when dpy_ui_info_supported(). The
function should always return a result. There should be a non-null
console or active_console.

Modify the argument to be const as well.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Albert Esteve 

 include/ui/console.h | 2 +-
 ui/console.c | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)


ATB,

Mark.

Re: [PATCH] hw/display/ramfb: plug slight guest-triggerable leak on mode setting

[Laszlo Ersek]

On 10/1/23 00:14, Laszlo Ersek wrote:
> On 9/29/23 13:17, Marc-André Lureau wrote:
>> Hi
>>
>> On Wed, Sep 27, 2023 at 7:46 PM Laszlo Ersek  wrote:
>>>
>>> On 9/19/23 15:19, Laszlo Ersek wrote:
 The fw_cfg DMA write callback in ramfb prepares a new display surface in
 QEMU; this new surface is put to use ("swapped in") upon the next display
 update. At that time, the old surface (if any) is released.

 If the guest triggers the fw_cfg DMA write callback at least twice between
 two adjacent display updates, then the second callback (and further such
 callbacks) will leak the previously prepared (but not yet swapped in)
 display surface.

 The issue can be shown by:

 (1) starting QEMU with "-trace displaysurface_free", and

 (2) running the following program in the guest UEFI shell:

> #include// ShellAppMain()
> #include  // gBS
> #include   // 
> EFI_GRAPHICS_OUTPUT_PROTOCOL
>
> INTN
> EFIAPI
> ShellAppMain (
>   IN UINTN   Argc,
>   IN CHAR16  **Argv
>   )
> {
>   EFI_STATUSStatus;
>   VOID  *Interface;
>   EFI_GRAPHICS_OUTPUT_PROTOCOL  *Gop;
>   UINT32Mode;
>
>   Status = gBS->LocateProtocol (
>   &gEfiGraphicsOutputProtocolGuid,
>   NULL,
>   &Interface
>   );
>   if (EFI_ERROR (Status)) {
> return 1;
>   }
>
>   Gop = Interface;
>
>   Mode = 1;
>   for ( ; ;) {
> Status = Gop->SetMode (Gop, Mode);
> if (EFI_ERROR (Status)) {
>   break;
> }
>
> Mode = 1 - Mode;
>   }
>
>   return 1;
> }

 The symptom is then that:

 - only one trace message appears periodically,

 - the time between adjacent messages keeps increasing -- implying that
   some list structure (containing the leaked resources) keeps growing,

 - the "surface" pointer is ever different.

> 18566@1695127471.449586:displaysurface_free surface=0x7f2fcc09a7c0
> 18566@1695127471.529559:displaysurface_free surface=0x7f2fcc9dac10
> 18566@1695127471.659812:displaysurface_free surface=0x7f2fcc441dd0
> 18566@1695127471.839669:displaysurface_free surface=0x7f2fcc0363d0
> 18566@1695127472.069674:displaysurface_free surface=0x7f2fcc413a80
> 18566@1695127472.349580:displaysurface_free surface=0x7f2fcc09cd00
> 18566@1695127472.679783:displaysurface_free surface=0x7f2fcc1395f0
> 18566@1695127473.059848:displaysurface_free surface=0x7f2fcc1cae50
> 18566@1695127473.489724:displaysurface_free surface=0x7f2fcc42fc50
> 18566@1695127473.969791:displaysurface_free surface=0x7f2fcc45dcc0
> 18566@1695127474.499708:displaysurface_free surface=0x7f2fcc70b9d0
> 18566@1695127475.079769:displaysurface_free surface=0x7f2fcc82acc0
> 18566@1695127475.709941:displaysurface_free surface=0x7f2fcc369c00
> 18566@1695127476.389619:displaysurface_free surface=0x7f2fcc32b910
> 18566@1695127477.119772:displaysurface_free surface=0x7f2fcc0d5a20
> 18566@1695127477.899517:displaysurface_free surface=0x7f2fcc086c40
> 18566@1695127478.729962:displaysurface_free surface=0x7f2fccc72020
> 18566@1695127479.609839:displaysurface_free surface=0x7f2fcc185160
> 18566@1695127480.539688:displaysurface_free surface=0x7f2fcc23a7e0
> 18566@1695127481.519759:displaysurface_free surface=0x7f2fcc3ec870
> 18566@1695127482.549930:displaysurface_free surface=0x7f2fcc634960
> 18566@1695127483.629661:displaysurface_free surface=0x7f2fcc26b140
> 18566@1695127484.759987:displaysurface_free surface=0x7f2fcc321700
> 18566@1695127485.940289:displaysurface_free surface=0x7f2fccaad100

 We figured this wasn't a CVE-worthy problem, as only small amounts of
 memory were leaked (the framebuffer itself is mapped from guest RAM, QEMU
 only allocates administrative structures), plus libvirt restricts QEMU
 memory footprint anyway, thus the guest can only DoS itself.

 Plug the leak, by releasing the last prepared (not yet swapped in) display
 surface, if any, in the fw_cfg DMA write callback.

 Regarding the "reproducer", with the fix in place, the log is flooded with
 trace messages (one per fw_cfg write), *and* the trace message alternates
 between just two "surface" pointer values (i.e., nothing is leaked, the
 allocator flip-flops between two objects in effect).

 This issue appears to date back to the introducion of ramfb (995b30179bdc,
 "hw/display: add ramfb, a simple boot framebuffer living in guest ram",
 2018-06-18).

 Cc: Gerd Hoffmann  (maintainer:ramfb)
 Cc: qemu-sta...@nongnu.org
 Fixes: 995b30179bdc
 Signed-off-by: Laszlo Ersek 
 ---
  hw/display/ramfb.c | 1 +
  1 file changed, 1 inser

[PATCH v2 1/2] elf2dmp: limit print length for sign_rsds

[Viktor Prutyanov]

String sign_rsds isn't terminated, so the print length must be limited.

Fixes: Coverity CID 1521598
Signed-off-by: Viktor Prutyanov 
---
 contrib/elf2dmp/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
index 5db163bdbe..6de5c9808e 100644
--- a/contrib/elf2dmp/main.c
+++ b/contrib/elf2dmp/main.c
@@ -478,7 +478,7 @@ static bool pe_check_pdb_name(uint64_t base, void 
*start_addr,
 }
 
 if (memcmp(&rsds->Signature, sign_rsds, sizeof(sign_rsds))) {
-eprintf("CodeView signature is \'%.4s\', \'%s\' expected\n",
+eprintf("CodeView signature is \'%.4s\', \'%.4s\' expected\n",
 rsds->Signature, sign_rsds);
 return false;
 }
-- 
2.21.0

[PATCH v2 0/2] elf2dmp: fixes of code analysis warnings

[Viktor Prutyanov]

This series tries to fix Coverity warnings.

v2: fix commit authorship, add CIDs

Viktor Prutyanov (2):
  elf2dmp: limit print length for sign_rsds
  elf2dmp: check array bounds in pdb_get_file_size

 contrib/elf2dmp/main.c |  2 +-
 contrib/elf2dmp/pdb.c  | 13 +
 2 files changed, 10 insertions(+), 5 deletions(-)

-- 
2.21.0

[PATCH v2 2/2] elf2dmp: check array bounds in pdb_get_file_size

[Viktor Prutyanov]

Index in file_size array must be checked against num_files, because the
entries we are looking for may be absent in the PDB.

Fixes: Coverity CID 1521597
Signed-off-by: Viktor Prutyanov 
---
 contrib/elf2dmp/pdb.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/contrib/elf2dmp/pdb.c b/contrib/elf2dmp/pdb.c
index 6ca5086f02..8e3c18c82f 100644
--- a/contrib/elf2dmp/pdb.c
+++ b/contrib/elf2dmp/pdb.c
@@ -25,6 +25,10 @@
 
 static uint32_t pdb_get_file_size(const struct pdb_reader *r, unsigned idx)
 {
+if (idx >= r->ds.toc->num_files) {
+return 0;
+}
+
 return r->ds.toc->file_size[idx];
 }
 
@@ -159,16 +163,17 @@ static void *pdb_ds_read_file(struct pdb_reader* r, 
uint32_t file_number)
 
 static int pdb_init_segments(struct pdb_reader *r)
 {
-char *segs;
 unsigned stream_idx = r->segments;
 
-segs = pdb_ds_read_file(r, stream_idx);
-if (!segs) {
+r->segs = pdb_ds_read_file(r, stream_idx);
+if (!r->segs) {
 return 1;
 }
 
-r->segs = segs;
 r->segs_size = pdb_get_file_size(r, stream_idx);
+if (!r->segs_size) {
+return 1;
+}
 
 return 0;
 }
-- 
2.21.0

[PATCH 1/2] elf2dmp: limit print length for sign_rsds

[Viktor Prutyanov]

From: Viktor Prutyanov 

String sign_rsds isn't terminated, so the print length must be limited.

Signed-off-by: Viktor Prutyanov 
---
 contrib/elf2dmp/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
index 5db163bdbe..6de5c9808e 100644
--- a/contrib/elf2dmp/main.c
+++ b/contrib/elf2dmp/main.c
@@ -478,7 +478,7 @@ static bool pe_check_pdb_name(uint64_t base, void 
*start_addr,
 }
 
 if (memcmp(&rsds->Signature, sign_rsds, sizeof(sign_rsds))) {
-eprintf("CodeView signature is \'%.4s\', \'%s\' expected\n",
+eprintf("CodeView signature is \'%.4s\', \'%.4s\' expected\n",
 rsds->Signature, sign_rsds);
 return false;
 }
-- 
2.21.0

[PATCH 0/2] elf2dmp: fixes of code analysis warnings

[Viktor Prutyanov]

This series is dedicated to fixes of Coverity warnings.

Viktor Prutyanov (2):
  elf2dmp: limit print length for sign_rsds
  elf2dmp: check array bounds in pdb_get_file_size

 contrib/elf2dmp/main.c |  2 +-
 contrib/elf2dmp/pdb.c  | 13 +
 2 files changed, 10 insertions(+), 5 deletions(-)

-- 
2.21.0

[PATCH 2/2] elf2dmp: check array bounds in pdb_get_file_size

[Viktor Prutyanov]

Index in file_size array must be checked against num_files, because the
entries we are looking for may be absent in the PDB.

Signed-off-by: Viktor Prutyanov 
---
 contrib/elf2dmp/pdb.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/contrib/elf2dmp/pdb.c b/contrib/elf2dmp/pdb.c
index 6ca5086f02..8e3c18c82f 100644
--- a/contrib/elf2dmp/pdb.c
+++ b/contrib/elf2dmp/pdb.c
@@ -25,6 +25,10 @@
 
 static uint32_t pdb_get_file_size(const struct pdb_reader *r, unsigned idx)
 {
+if (idx >= r->ds.toc->num_files) {
+return 0;
+}
+
 return r->ds.toc->file_size[idx];
 }
 
@@ -159,16 +163,17 @@ static void *pdb_ds_read_file(struct pdb_reader* r, 
uint32_t file_number)
 
 static int pdb_init_segments(struct pdb_reader *r)
 {
-char *segs;
 unsigned stream_idx = r->segments;
 
-segs = pdb_ds_read_file(r, stream_idx);
-if (!segs) {
+r->segs = pdb_ds_read_file(r, stream_idx);
+if (!r->segs) {
 return 1;
 }
 
-r->segs = segs;
 r->segs_size = pdb_get_file_size(r, stream_idx);
+if (!r->segs_size) {
+return 1;
+}
 
 return 0;
 }
-- 
2.21.0

Re: [PATCH] hw/display/ramfb: plug slight guest-triggerable leak on mode setting

[Laszlo Ersek]

On 9/29/23 13:17, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Sep 27, 2023 at 7:46 PM Laszlo Ersek  wrote:
>>
>> On 9/19/23 15:19, Laszlo Ersek wrote:
>>> The fw_cfg DMA write callback in ramfb prepares a new display surface in
>>> QEMU; this new surface is put to use ("swapped in") upon the next display
>>> update. At that time, the old surface (if any) is released.
>>>
>>> If the guest triggers the fw_cfg DMA write callback at least twice between
>>> two adjacent display updates, then the second callback (and further such
>>> callbacks) will leak the previously prepared (but not yet swapped in)
>>> display surface.
>>>
>>> The issue can be shown by:
>>>
>>> (1) starting QEMU with "-trace displaysurface_free", and
>>>
>>> (2) running the following program in the guest UEFI shell:
>>>
 #include// ShellAppMain()
 #include  // gBS
 #include   // 
 EFI_GRAPHICS_OUTPUT_PROTOCOL

 INTN
 EFIAPI
 ShellAppMain (
   IN UINTN   Argc,
   IN CHAR16  **Argv
   )
 {
   EFI_STATUSStatus;
   VOID  *Interface;
   EFI_GRAPHICS_OUTPUT_PROTOCOL  *Gop;
   UINT32Mode;

   Status = gBS->LocateProtocol (
   &gEfiGraphicsOutputProtocolGuid,
   NULL,
   &Interface
   );
   if (EFI_ERROR (Status)) {
 return 1;
   }

   Gop = Interface;

   Mode = 1;
   for ( ; ;) {
 Status = Gop->SetMode (Gop, Mode);
 if (EFI_ERROR (Status)) {
   break;
 }

 Mode = 1 - Mode;
   }

   return 1;
 }
>>>
>>> The symptom is then that:
>>>
>>> - only one trace message appears periodically,
>>>
>>> - the time between adjacent messages keeps increasing -- implying that
>>>   some list structure (containing the leaked resources) keeps growing,
>>>
>>> - the "surface" pointer is ever different.
>>>
 18566@1695127471.449586:displaysurface_free surface=0x7f2fcc09a7c0
 18566@1695127471.529559:displaysurface_free surface=0x7f2fcc9dac10
 18566@1695127471.659812:displaysurface_free surface=0x7f2fcc441dd0
 18566@1695127471.839669:displaysurface_free surface=0x7f2fcc0363d0
 18566@1695127472.069674:displaysurface_free surface=0x7f2fcc413a80
 18566@1695127472.349580:displaysurface_free surface=0x7f2fcc09cd00
 18566@1695127472.679783:displaysurface_free surface=0x7f2fcc1395f0
 18566@1695127473.059848:displaysurface_free surface=0x7f2fcc1cae50
 18566@1695127473.489724:displaysurface_free surface=0x7f2fcc42fc50
 18566@1695127473.969791:displaysurface_free surface=0x7f2fcc45dcc0
 18566@1695127474.499708:displaysurface_free surface=0x7f2fcc70b9d0
 18566@1695127475.079769:displaysurface_free surface=0x7f2fcc82acc0
 18566@1695127475.709941:displaysurface_free surface=0x7f2fcc369c00
 18566@1695127476.389619:displaysurface_free surface=0x7f2fcc32b910
 18566@1695127477.119772:displaysurface_free surface=0x7f2fcc0d5a20
 18566@1695127477.899517:displaysurface_free surface=0x7f2fcc086c40
 18566@1695127478.729962:displaysurface_free surface=0x7f2fccc72020
 18566@1695127479.609839:displaysurface_free surface=0x7f2fcc185160
 18566@1695127480.539688:displaysurface_free surface=0x7f2fcc23a7e0
 18566@1695127481.519759:displaysurface_free surface=0x7f2fcc3ec870
 18566@1695127482.549930:displaysurface_free surface=0x7f2fcc634960
 18566@1695127483.629661:displaysurface_free surface=0x7f2fcc26b140
 18566@1695127484.759987:displaysurface_free surface=0x7f2fcc321700
 18566@1695127485.940289:displaysurface_free surface=0x7f2fccaad100
>>>
>>> We figured this wasn't a CVE-worthy problem, as only small amounts of
>>> memory were leaked (the framebuffer itself is mapped from guest RAM, QEMU
>>> only allocates administrative structures), plus libvirt restricts QEMU
>>> memory footprint anyway, thus the guest can only DoS itself.
>>>
>>> Plug the leak, by releasing the last prepared (not yet swapped in) display
>>> surface, if any, in the fw_cfg DMA write callback.
>>>
>>> Regarding the "reproducer", with the fix in place, the log is flooded with
>>> trace messages (one per fw_cfg write), *and* the trace message alternates
>>> between just two "surface" pointer values (i.e., nothing is leaked, the
>>> allocator flip-flops between two objects in effect).
>>>
>>> This issue appears to date back to the introducion of ramfb (995b30179bdc,
>>> "hw/display: add ramfb, a simple boot framebuffer living in guest ram",
>>> 2018-06-18).
>>>
>>> Cc: Gerd Hoffmann  (maintainer:ramfb)
>>> Cc: qemu-sta...@nongnu.org
>>> Fixes: 995b30179bdc
>>> Signed-off-by: Laszlo Ersek 
>>> ---
>>>  hw/display/ramfb.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
>>> index 79b9754a5820..c2b002d53480 100644
>>> --- a/hw/display/ramfb.c
>>> +++ b/hw/dis

Re: [PATCH 0/4] ui/console: multihead: fix crash, simplify logic

[Laszlo Ersek]

On 9/29/23 09:57, Mark Cave-Ayland wrote:
> On 26/09/2023 09:00, Marc-André Lureau wrote:
> 
>> Hi Laszlo
>>
>> On Mon, Sep 25, 2023 at 7:36 PM Laszlo Ersek  wrote:
>>> Has this been queued by someone? Both Gerd and Marc-André are "odd
>>> fixers", so I'm not sure who should be sending a PR with these patches
>>> (and I don't see a pending PULL at
>>> 
>>> with these patch subjects included).
>>
>> I have the series in my "ui" branch. I was waiting for a few more
>> patches to be accumulated. But if someone else takes this first, I'll
>> drop them.
> 
> Does this series fix the "../ui/console.c:818: dpy_get_ui_info:
> Assertion `dpy_ui_info_supported(con)' failed." assert() on startup when
> using gtk? It would be good to get this fixed in git master soon, as it
> has been broken for a couple of weeks now, and -display sdl has issues
> tracking the mouse correctly on my laptop here :(

... probably not; I've never seen that issue. Can you provide a reproducer?

Also, it should be bisectable (over Marc-André's 52-part series I guess).

Laszlo

> 
> 
> ATB,
> 
> Mark.
> 

[PATCH] linux-user/hppa: Fix struct target_sigcontext layout

[Richard Henderson]

Use abi_ullong not uint64_t so that the alignment of the field
and therefore the layout of the struct is correct.

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

diff --git a/linux-user/hppa/signal.c b/linux-user/hppa/signal.c
index bda6e54655..ec5f5412d1 100644
--- a/linux-user/hppa/signal.c
+++ b/linux-user/hppa/signal.c
@@ -25,7 +25,7 @@
 struct target_sigcontext {
 abi_ulong sc_flags;
 abi_ulong sc_gr[32];
-uint64_t sc_fr[32];
+abi_ullong sc_fr[32];
 abi_ulong sc_iasq[2];
 abi_ulong sc_iaoq[2];
 abi_ulong sc_sar;
-- 
2.34.1

[PATCH v2 1/1] qemu-img: do not erase destination file in qemu-img dd command

[Mike Maslenkin]

Add a check that destination file exists and do not call bdrv_create for
this case.

Currently `qemu-img dd` command destroys content of destination file.
Effectively this means that parameters (geometry) of destination image
file are changing. This can be undesirable behavior for user especially
if format of destination image does not support resizing.

Steps to reproduce:
  1. Create empty disk image with some non default size.
   `qemu-img  create -f qcow2 $DEST_IMG 3T`
 Remember that `qemu-img info $DEST_IMG` returns:
   virtual size: 3 TiB (3298534883328 bytes)
   disk size: 240 KiB
   cluster_size: 65536
  2. Run `qemu-img dd -O qcow2 of=$DEST_IMG if=$SRC_IMG bs=1M count=100`
  3. Check `qemu-img info $DEST_IMG` output:
   virtual size: 100 MiB (104857600 bytes)
   disk size: 112 MiB
   cluster_size: 65536

Parameters of $DEST_IMG were changed. Actually `qemu-img dd` has created
a new disk based on current default geometry for particular format.
For example for "parallels" format default BAT for 256GB disk is written
to empty file prior writing disk image data.

With this patch virtual disk metadata and geometry of a destination image
are preserved. As another visible change of `qemu-img dd` behavior is that
if destination image is less than source it can finish with error (similar
to "dd" utility):
  qemu-img: error while writing to output image file: Input/output error

Signed-off-by: Mike Maslenkin 
---
  diff from v1: removed additional fprintf call leaved in patch by accident
---
 qemu-img.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index a48edb71015c..1a83c14212fb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -5150,13 +5150,15 @@ static int img_dd(int argc, char **argv)
 size - in.bsz * in.offset, &error_abort);
 }
 
-ret = bdrv_create(drv, out.filename, opts, &local_err);
-if (ret < 0) {
-error_reportf_err(local_err,
-  "%s: error while creating output image: ",
-  out.filename);
-ret = -1;
-goto out;
+if (!g_file_test(out.filename, G_FILE_TEST_EXISTS)) {
+ret = bdrv_create(drv, out.filename, opts, &local_err);
+if (ret < 0) {
+error_reportf_err(local_err,
+   "%s: error while creating output image: ",
+   out.filename);
+ret = -1;
+goto out;
+}
 }
 
 /* TODO, we can't honour --image-opts for the target,
-- 
2.32.0 (Apple Git-132)

Re: On integrating LoongArch EDK2 firmware into QEMU build process

[WANG Xuerui]

On 3/31/23 08:54, maobibo wrote:

Xuerui,

Thanks for your mail, it is a good suggestion. Now we are planing to move 
LoongArch uefi bios from edk2-platform to edk2 repo, so that uefi bios 
supporting LoongArch can be auto compiled and uploaded to qemu repo. Only that 
process is somwhat slow since lacking of hands, however we are doing this.


Pinging: a few months have passed, and it seems this work is stalled? 
Given the LoongArch Linux KVM support is about to land in v6.7, it may 
be time to prepare the firmware and QEMU side of things, so users would 
no longer have to manually acquire the firmware blobs whenever they fire 
up their VMs.




Regards
Bibo, Mao

在 2023/3/30 22:06, WANG Xuerui 写道:

Hi,

Recently there are reportedly increased general interest in trying out 
LoongArch on top of QEMU, among both end users and organizations; and the EDK2 
firmware port is fully upstreamed since the stable202211 version, and a build 
suitable for QEMU is already possible with Platform/Loongson/LoongArchQemuPkg 
in edk2-platforms. I think providing pre-built LoongArch firmware would make it 
much easier to dabble in system emulation, helping those users. (They currently 
have to pull a blob from yangxiaojuan/qemu-binary, and remember to pair certain 
version of QEMU with certain revision of the firmware blob. I'm also one of the 
users who can't remember which version to use, but I can always build my own; 
imagine the difficulty an end user would face!)

So I tried to add a LoongArch build to the list stored in roms/, but discovered 
that edk2-platforms seems not included, because all other platforms' EDK2 
packages are directly under the main edk2 repo.

The question is: is integrating a platform package from edk2-platforms okay 
under the current build system, so we can arrange to provide edk2-platforms 
also as a submodule and go ahead? Or do we (the LoongArch firmware community) 
have to change the code organization to make necessary parts available in the 
main edk2 repo?

CC-ing target/loongarch maintainers from Loongson too as you may have more 
information.

[PATCH 1/1] qemu-img: do not erase destination file in qemu-img dd command

[Mike Maslenkin]

Add a check that destination file exists and do not call bdrv_create for
this case.

Currently `qemu-img dd` command destroys content of destination file.
Effectively this means that parameters (geometry) of destination image
file are changing. This can be undesirable behavior for user especially
if format of destination image does not support resizing.

Steps to reproduce:
  1. Create empty disk image with some non default size.
   `qemu-img  create -f qcow2 $DEST_IMG 3T`
 Remember that `qemu-img info $DEST_IMG` returns:
   virtual size: 3 TiB (3298534883328 bytes)
   disk size: 240 KiB
   cluster_size: 65536
  2. Run `qemu-img dd -O qcow2 of=$DEST_IMG if=$SRC_IMG bs=1M count=100`
  3. Check `qemu-img info $DEST_IMG` output:
   virtual size: 100 MiB (104857600 bytes)
   disk size: 112 MiB
   cluster_size: 65536

Parameters of $DEST_IMG were changed. Actually `qemu-img dd` has created
a new disk based on current default geometry for particular format.
For example for "parallels" format default BAT for 256GB disk is written
to empty file prior writing disk image data.

With this patch virtual disk metadata and geometry of a destination image
are preserved. As another visible change of `qemu-img dd` behavior is that
if destination image is less than source it can finish with error (similar
to "dd" utility):
  qemu-img: error while writing to output image file: Input/output error

Signed-off-by: Mike Maslenkin 
---
 qemu-img.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index a48edb71015c..1a83c14212fb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -5150,13 +5150,16 @@ static int img_dd(int argc, char **argv)
 size - in.bsz * in.offset, &error_abort);
 }
 
-ret = bdrv_create(drv, out.filename, opts, &local_err);
-if (ret < 0) {
-error_reportf_err(local_err,
-  "%s: error while creating output image: ",
-  out.filename);
-ret = -1;
-goto out;
+if (!g_file_test(out.filename, G_FILE_TEST_EXISTS)) {
+ret = bdrv_create(drv, out.filename, opts, &local_err);
+fprintf (stderr, "Recreating image file\n");
+if (ret < 0) {
+error_reportf_err(local_err,
+   "%s: error while creating output image: ",
+   out.filename);
+ret = -1;
+goto out;
+}
 }
 
 /* TODO, we can't honour --image-opts for the target,
-- 
2.32.0 (Apple Git-132)

Re: [PATCH 0/7] tcg/loongarch64: Improvements for 128-bit load/store

[WANG Xuerui]

On 9/30/23 10:13, Richard Henderson wrote:

Ping.

r~

On 9/16/23 15:01, Richard Henderson wrote:

For tcg generated code, use new registers with load so that we never
overlap the input address, so that we can simplify address build for
64-bit user-only.

For tcg out-of-line code, implement the host/ headers to for atomic 
128-bit

load and store, reducing the cases for which we must raise EXCP_ATOMIC.


r~

Based-on: 20230916171223.521545-1-richard.hender...@linaro.org
("[PULL v2 00/39] tcg patch queue")

Richard Henderson (7):
   tcg: Add C_N2_I1
   tcg/loongarch64: Use C_N2_I1 for INDEX_op_qemu_ld_a*_i128
   util: Add cpuinfo for loongarch64
   tcg/loongarch64: Use cpuinfo.h
   host/include/loongarch64: Add atomic16 load and store
   accel/tcg: Remove redundant case in store_atom_16
   accel/tcg: Fix condition for store_atom_insert_al16

  .../include/loongarch64/host/atomic128-ldst.h | 52 +++
  host/include/loongarch64/host/cpuinfo.h   | 21 
  .../loongarch64/host/load-extract-al16-al8.h  | 39 ++
  .../loongarch64/host/store-insert-al16.h  | 12 +
  tcg/loongarch64/tcg-target-con-set.h  |  2 +-
  tcg/loongarch64/tcg-target.h  |  8 +--
  accel/tcg/cputlb.c    |  2 +-
  tcg/tcg.c |  5 ++
  util/cpuinfo-loongarch.c  | 35 +
  accel/tcg/ldst_atomicity.c.inc    | 14 ++---
  tcg/loongarch64/tcg-target.c.inc  | 25 +
  util/meson.build  |  2 +
  12 files changed, 189 insertions(+), 28 deletions(-)
  create mode 100644 host/include/loongarch64/host/atomic128-ldst.h
  create mode 100644 host/include/loongarch64/host/cpuinfo.h
  create mode 100644 
host/include/loongarch64/host/load-extract-al16-al8.h

  create mode 100644 host/include/loongarch64/host/store-insert-al16.h
  create mode 100644 util/cpuinfo-loongarch.c


Sorry for the delay; I've skimmed through the series and tested on 
Loongson 3C5000L hardware, so


Reviewed-by: WANG Xuerui 

[PATCH] build: Remove --enable-gprof

[Richard Henderson]

This build option has been deprecated since 8.0.
Remove all CONFIG_GPROF code that depends on that,
including one errant check using TARGET_GPROF.

Signed-off-by: Richard Henderson 
---
 docs/about/deprecated.rst  | 14 --
 meson.build| 12 
 bsd-user/bsd-proc.h|  3 ---
 bsd-user/signal.c  |  5 -
 linux-user/exit.c  |  6 --
 linux-user/signal.c|  5 -
 meson_options.txt  |  3 ---
 scripts/meson-buildoptions.sh  |  3 ---
 tests/qemu-iotests/meson.build |  2 +-
 9 files changed, 1 insertion(+), 52 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 8f3fef97bd..54be9e21a4 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -20,20 +20,6 @@ they were first deprecated in the 2.10.0 release.
 What follows is a list of all features currently marked as
 deprecated.
 
-Build options
--
-
-``gprof`` builds (since 8.0)
-
-
-The ``--enable-gprof`` configure setting relies on compiler
-instrumentation to gather its data which can distort the generated
-profile. As other non-instrumenting tools are available that give a
-more holistic view of the system with non-instrumented binaries we are
-deprecating the build option and no longer defend it in CI. The
-``--enable-gcov`` build option remains for analysis test case
-coverage.
-
 System emulator command line arguments
 --
 
diff --git a/meson.build b/meson.build
index 5139db2ff7..726d690e27 100644
--- a/meson.build
+++ b/meson.build
@@ -254,11 +254,6 @@ if host_arch == 'i386' and not cc.links('''
   qemu_common_flags = ['-march=i486'] + qemu_common_flags
 endif
 
-if get_option('gprof')
-  qemu_common_flags += ['-p']
-  qemu_ldflags += ['-p']
-endif
-
 if get_option('prefer_static')
   qemu_ldflags += get_option('b_pie') ? '-static-pie' : '-static'
 endif
@@ -2214,7 +2209,6 @@ config_host_data.set('CONFIG_DEBUG_GRAPH_LOCK', 
get_option('debug_graph_lock'))
 config_host_data.set('CONFIG_DEBUG_MUTEX', get_option('debug_mutex'))
 config_host_data.set('CONFIG_DEBUG_STACK_USAGE', 
get_option('debug_stack_usage'))
 config_host_data.set('CONFIG_DEBUG_TCG', get_option('debug_tcg'))
-config_host_data.set('CONFIG_GPROF', get_option('gprof'))
 config_host_data.set('CONFIG_LIVE_BLOCK_MIGRATION', 
get_option('live_block_migration').allowed())
 config_host_data.set('CONFIG_QOM_CAST_DEBUG', get_option('qom_cast_debug'))
 config_host_data.set('CONFIG_REPLICATION', get_option('replication').allowed())
@@ -4122,12 +4116,6 @@ summary_info += {'memory allocator':  
get_option('malloc')}
 summary_info += {'avx2 optimization': config_host_data.get('CONFIG_AVX2_OPT')}
 summary_info += {'avx512bw optimization': 
config_host_data.get('CONFIG_AVX512BW_OPT')}
 summary_info += {'avx512f optimization': 
config_host_data.get('CONFIG_AVX512F_OPT')}
-if get_option('gprof')
-  gprof_info = 'YES (deprecated)'
-else
-  gprof_info = get_option('gprof')
-endif
-summary_info += {'gprof': gprof_info}
 summary_info += {'gcov':  get_option('b_coverage')}
 summary_info += {'thread sanitizer':  get_option('tsan')}
 summary_info += {'CFI support':   get_option('cfi')}
diff --git a/bsd-user/bsd-proc.h b/bsd-user/bsd-proc.h
index a1061bffb8..0e1d461c4c 100644
--- a/bsd-user/bsd-proc.h
+++ b/bsd-user/bsd-proc.h
@@ -25,9 +25,6 @@
 /* exit(2) */
 static inline abi_long do_bsd_exit(void *cpu_env, abi_long arg1)
 {
-#ifdef TARGET_GPROF
-_mcleanup();
-#endif
 gdb_exit(arg1);
 qemu_plugin_user_exit();
 _exit(arg1);
diff --git a/bsd-user/signal.c b/bsd-user/signal.c
index b6beab659e..0b13455f4a 100644
--- a/bsd-user/signal.c
+++ b/bsd-user/signal.c
@@ -848,11 +848,6 @@ void signal_init(void)
 act.sa_flags = SA_SIGINFO;
 
 for (i = 1; i <= TARGET_NSIG; i++) {
-#ifdef CONFIG_GPROF
-if (i == TARGET_SIGPROF) {
-continue;
-}
-#endif
 host_sig = target_to_host_signal(i);
 sigaction(host_sig, NULL, &oact);
 if (oact.sa_sigaction == (void *)SIG_IGN) {
diff --git a/linux-user/exit.c b/linux-user/exit.c
index 3017d28a3c..50266314e0 100644
--- a/linux-user/exit.c
+++ b/linux-user/exit.c
@@ -22,9 +22,6 @@
 #include "qemu.h"
 #include "user-internals.h"
 #include "qemu/plugin.h"
-#ifdef CONFIG_GPROF
-#include 
-#endif
 
 #ifdef CONFIG_GCOV
 extern void __gcov_dump(void);
@@ -32,9 +29,6 @@ extern void __gcov_dump(void);
 
 void preexit_cleanup(CPUArchState *env, int code)
 {
-#ifdef CONFIG_GPROF
-_mcleanup();
-#endif
 #ifdef CONFIG_GCOV
 __gcov_dump();
 #endif
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 748a98f3e5..920db8978f 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -588,11 +588,6 @@ void signal_init(void)
 act.sa_flags = SA_SIGINFO;
 act.sa_sigaction = host_signal_handler;
 for(i = 1; i <= TARGET_NSIG; i++) {
-#ifdef CONFIG_

Re: [PATCH v4 16/18] virtio-mem: Expose device memory dynamically via multiple memslots if enabled

[Maciej S. Szmigiero]

On 26.09.2023 20:57, David Hildenbrand wrote:

Having large virtio-mem devices that only expose little memory to a VM
is currently a problem: we map the whole sparse memory region into the
guest using a single memslot, resulting in one gigantic memslot in KVM.
KVM allocates metadata for the whole memslot, which can result in quite
some memory waste.

Assuming we have a 1 TiB virtio-mem device and only expose little (e.g.,
1 GiB) memory, we would create a single 1 TiB memslot and KVM has to
allocate metadata for that 1 TiB memslot: on x86, this implies allocating
a significant amount of memory for metadata:

(1) RMAP: 8 bytes per 4 KiB, 8 bytes per 2 MiB, 8 bytes per 1 GiB
 -> For 1 TiB: 2147483648 + 4194304 + 8192 = ~ 2 GiB (0.2 %)

 With the TDP MMU (cat /sys/module/kvm/parameters/tdp_mmu) this gets
 allocated lazily when required for nested VMs
(2) gfn_track: 2 bytes per 4 KiB
 -> For 1 TiB: 536870912 = ~512 MiB (0.05 %)
(3) lpage_info: 4 bytes per 2 MiB, 4 bytes per 1 GiB
 -> For 1 TiB: 2097152 + 4096 = ~2 MiB (0.0002 %)
(4) 2x dirty bitmaps for tracking: 2x 1 bit per 4 KiB page
 -> For 1 TiB: 536870912 = 64 MiB (0.006 %)

So we primarily care about (1) and (2). The bad thing is, that the
memory consumption *doubles* once SMM is enabled, because we create the
memslot once for !SMM and once for SMM.

Having a 1 TiB memslot without the TDP MMU consumes around:
* With SMM: 5 GiB
* Without SMM: 2.5 GiB
Having a 1 TiB memslot with the TDP MMU consumes around:
* With SMM: 1 GiB
* Without SMM: 512 MiB

... and that's really something we want to optimize, to be able to just
start a VM with small boot memory (e.g., 4 GiB) and a virtio-mem device
that can grow very large (e.g., 1 TiB).

Consequently, using multiple memslots and only mapping the memslots we
really need can significantly reduce memory waste and speed up
memslot-related operations. Let's expose the sparse RAM memory region using
multiple memslots, mapping only the memslots we currently need into our
device memory region container.

The feature can be enabled using "dynamic-memslots=on" and requires
"unplugged-inaccessible=on", which is nowadays the default.

Once enabled, we'll auto-detect the number of memslots to use based on the
memslot limit provided by the core. We'll use at most 1 memslot per
gigabyte. Note that our global limit of memslots accross all memory devices
is currently set to 256: even with multiple large virtio-mem devices,
we'd still have a sane limit on the number of memslots used.

The default is to not dynamically map memslot for now
("dynamic-memslots=off"). The optimization must be enabled manually,
because some vhost setups (e.g., hotplug of vhost-user devices) might be
problematic until we support more memslots especially in vhost-user backends.

Note that "dynamic-memslots=on" is just a hint that multiple memslots
*may* be used for internal optimizations, not that multiple memslots
*must* be used. The actual number of memslots that are used is an
internal detail: for example, once memslot metadata is no longer an
issue, we could simply stop optimizing for that. Migration source and
destination can differ on the setting of "dynamic-memslots".

Signed-off-by: David Hildenbrand 
---


The changes seem reasonable, so:
Reviewed-by: Maciej S. Szmigiero 

Thanks,
Maciej

Re: [PATCH v4 15/18] virtio-mem: Update state to match bitmap as soon as it's been migrated

[Maciej S. Szmigiero]

On 26.09.2023 20:57, David Hildenbrand wrote:

It's cleaner and future-proof to just have other state that depends on the
bitmap state to be updated as soon as possible when restoring the bitmap.

So factor out informing RamDiscardListener into a functon and call it in
case of early migration right after we restored the bitmap.

Signed-off-by: David Hildenbrand 
---
  hw/virtio/virtio-mem.c | 26 +-
  1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 0b0e6c5090..0cf47df9cf 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -984,9 +984,8 @@ static int virtio_mem_restore_unplugged(VirtIOMEM *vmem)
 virtio_mem_discard_range_cb);
  }
  
-static int virtio_mem_post_load(void *opaque, int version_id)

+static int virtio_mem_post_load_bitmap(VirtIOMEM *vmem)
  {
-VirtIOMEM *vmem = VIRTIO_MEM(opaque);
  RamDiscardListener *rdl;
  int ret;
  
@@ -1001,6 +1000,20 @@ static int virtio_mem_post_load(void *opaque, int version_id)

  return ret;
  }
  }
+return 0;
+}
+
+static int virtio_mem_post_load(void *opaque, int version_id)
+{
+VirtIOMEM *vmem = VIRTIO_MEM(opaque);
+int ret;
+
+if (!vmem->early_migration) {
+ret = virtio_mem_post_load_bitmap(vmem);
+if (ret) {
+return ret;
+}
+}
  
  /*

   * If shared RAM is migrated using the file content and not using QEMU,
@@ -1043,7 +1056,7 @@ static int virtio_mem_post_load_early(void *opaque, int 
version_id)
  int ret;
  
  if (!vmem->prealloc) {

-return 0;
+goto post_load_bitmap;
  }
  
  /*

@@ -1051,7 +1064,7 @@ static int virtio_mem_post_load_early(void *opaque, int 
version_id)
   * don't mess with preallocation and postcopy.
   */
  if (migrate_ram_is_ignored(rb)) {
-return 0;
+goto post_load_bitmap;
  }
  
  /*

@@ -1084,7 +1097,10 @@ static int virtio_mem_post_load_early(void *opaque, int 
version_id)
  return -EBUSY;
  }
  }
-return 0;
+
+post_load_bitmap:
+/* Finally, update any other state to be consistent with the new bitmap. */
+return virtio_mem_post_load_bitmap(vmem);
  }
  
  typedef struct VirtIOMEMMigSanityChecks {



Reviewed-by: Maciej S. Szmigiero 

Thanks,
Maciej

Re: [PATCH] target/loongarch: fix ASXE flag conflict

[Richard Henderson]

On 9/30/23 04:28, Jiajie Chen wrote:

HW_FLAGS_EUEN_ASXE acccidentally conflicts with HW_FLAGS_CRMD_PG,
enabling LASX instructions even when CSR_EUEN.ASXE=0.

Closes: https://gitlab.com/qemu-project/qemu/-/issues/1907
Signed-off-by: Jiajie Chen 
---
  target/loongarch/cpu.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
index f125a8e49b..79ad79a289 100644
--- a/target/loongarch/cpu.h
+++ b/target/loongarch/cpu.h
@@ -462,7 +462,7 @@ static inline void set_pc(CPULoongArchState *env, uint64_t 
value)
  #define HW_FLAGS_CRMD_PGR_CSR_CRMD_PG_MASK   /* 0x10 */
  #define HW_FLAGS_EUEN_FPE   0x04
  #define HW_FLAGS_EUEN_SXE   0x08
-#define HW_FLAGS_EUEN_ASXE  0x10
+#define HW_FLAGS_EUEN_ASXE  0x40
  #define HW_FLAGS_VA32   0x20
  
  static inline void cpu_get_tb_cpu_state(CPULoongArchState *env, vaddr *pc,


Better to put all defines in bit order, otherwise it will be easy to make the same mistake 
again.


With that,
Reviewed-by: Richard Henderson 


r~

Re: [PATCH] target/sh4: fix crashes on signal delivery

[Yoshinori Sato]

On Fri, 29 Sep 2023 01:42:08 +0900,
Mikulas Patocka wrote:
> 
> sh4 uses gUSA (general UserSpace Atomicity) to provide atomicity on CPUs
> that don't have atomic instructions. A gUSA region that adds 1 to an
> atomic variable stored in @R2 looks like this:
> 
>   4004b6:   03 c7   mova4004c4 ,r0
>   4004b8:   f3 61   mov r15,r1
>   4004ba:   09 00   nop
>   4004bc:   fa ef   mov #-6,r15
>   4004be:   22 63   mov.l   @r2,r3
>   4004c0:   01 73   add #1,r3
>   4004c2:   32 22   mov.l   r3,@r2
>   4004c4:   13 6f   mov r1,r15
> 
> R0 contains a pointer to the end of the gUSA region
> R1 contains the saved stack pointer
> R15 contains negative length of the gUSA region
> 
> When this region is interrupted by a signal, the kernel detects if
> R15 >= -128U. If yes, the kernel rolls back PC to the beginning of the
> region and restores SP by copying R1 to R15.
> 
> The problem happens if we are interrupted by a signal at address 4004c4.
> R15 still holds the value -6, but the atomic value was already written by
> an instruction at address 4004c2. In this situation we can't undo the
> gUSA. The function unwind_gusa does nothing, the signal handler attempts
> to push a signal frame to the address -6 and crashes.
> 
> This patch fixes it, so that if we are interrupted at the last instruction 
> in a gUSA region, we copy R1 to R15 to restore the correct stack pointer 
> and avoid crashing.
> 
> There's another bug: if we are interrupted in a delay slot, we save the
> address of the instruction in the delay slot. We must save the address of
> the previous instruction.
> 
> Signed-off-by: Mikulas Patocka 
> Cc: qemu-sta...@nongnu.org

Reviewed-by: Yoshinori Sato 

> ---
>  linux-user/sh4/signal.c |8 
>  1 file changed, 8 insertions(+)
> 
> Index: qemu/linux-user/sh4/signal.c
> ===
> --- qemu.orig/linux-user/sh4/signal.c 2023-09-27 19:02:41.0 +0200
> +++ qemu/linux-user/sh4/signal.c  2023-09-27 19:55:13.0 +0200
> @@ -104,6 +104,14 @@ static void unwind_gusa(CPUSH4State *reg
>  
>  /* Reset the SP to the saved version in R1.  */
>  regs->gregs[15] = regs->gregs[1];
> +} else if (regs->gregs[15] >= -128u && regs->pc == regs->gregs[0]) {
> +/* If we are on the last instruction of a gUSA region, we must reset
> +   the SP, otherwise we would be pushing the signal context to
> +   invalid memory.  */
> +regs->gregs[15] = regs->gregs[1];
> +} else if (regs->flags & TB_FLAG_DELAY_SLOT) {
> +/* If we are in a delay slot, push the previous instruction.  */
> +regs->pc -= 2;
>  }
>  }
>  
> 

-- 
Yosinori Sato

Re: [PATCH v7 01/12] nbd/server: Support a request payload

[Vladimir Sementsov-Ogievskiy]

On 25.09.23 22:22, Eric Blake wrote:

Upcoming additions to support NBD 64-bit effect lengths allow for the
possibility to distinguish between payload length (capped at 32M) and
effect length (64 bits, although we generally assume 63 bits because
of off_t limitations).  Without that extension, only the NBD_CMD_WRITE
request has a payload; but with the extension, it makes sense to allow
at least NBD_CMD_BLOCK_STATUS to have both a payload and effect length
in a future patch (where the payload is a limited-size struct that in
turn gives the real effect length as well as a subset of known ids for
which status is requested).  Other future NBD commands may also have a
request payload, so the 64-bit extension introduces a new
NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header
length is a payload length or an effect length, rather than
hard-coding the decision based on the command.

According to the spec, a client should never send a command with a
payload without the negotiation phase proving such extension is
available.  So in the unlikely event the bit is set or cleared
incorrectly, the client is already at fault; if the client then
provides the payload, we can gracefully consume it off the wire and
fail the command with NBD_EINVAL (subsequent checks for magic numbers
ensure we are still in sync), while if the client fails to send
payload we block waiting for it (basically deadlocking our connection
to the bad client, but not negatively impacting our ability to service
other clients, so not a security risk).  Note that we do not support
the payload version of BLOCK_STATUS yet.

Signed-off-by: Eric Blake 
---

v7: another try at better logic [Vladimir]

v5: retitled from v4 13/24, rewrite on top of previous patch's switch
statement [Vladimir]

v4: less indentation on several 'if's [Vladimir]
---
  nbd/server.c | 37 +
  nbd/trace-events |  1 +
  2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 7a6f95071f8..1eabcfc908d 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2322,9 +2322,11 @@ static int coroutine_fn 
nbd_co_receive_request(NBDRequestData *req,
 Error **errp)
  {
  NBDClient *client = req->client;
+bool extended_with_payload;
  bool check_length = false;
  bool check_rofs = false;
  bool allocate_buffer = false;
+bool payload_okay = false;
  unsigned payload_len = 0;
  int valid_flags = NBD_CMD_FLAG_FUA;
  int ret;
@@ -2338,6 +2340,13 @@ static int coroutine_fn 
nbd_co_receive_request(NBDRequestData *req,

  trace_nbd_co_receive_request_decode_type(request->cookie, request->type,
   nbd_cmd_lookup(request->type));
+extended_with_payload = client->mode >= NBD_MODE_EXTENDED &&
+request->flags & NBD_CMD_FLAG_PAYLOAD_LEN;
+if (extended_with_payload) {
+payload_len = request->len;
+check_length = true;
+}
+
  switch (request->type) {
  case NBD_CMD_DISC:
  /* Special case: we're going to disconnect without a reply,
@@ -2354,6 +2363,15 @@ static int coroutine_fn 
nbd_co_receive_request(NBDRequestData *req,
  break;

  case NBD_CMD_WRITE:
+if (client->mode >= NBD_MODE_EXTENDED) {
+if (!extended_with_payload) {
+/* The client is noncompliant. Trace it, but proceed. */
+trace_nbd_co_receive_ext_payload_compliance(request->from,
+request->len);
+}
+valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN;
+}
+payload_okay = true;
  payload_len = request->len;
  check_length = true;
  allocate_buffer = true;
@@ -2395,6 +2413,14 @@ static int coroutine_fn 
nbd_co_receive_request(NBDRequestData *req,
 request->len, NBD_MAX_BUFFER_SIZE);
  return -EINVAL;
  }
+if (payload_len && !payload_okay) {
+/*
+ * For now, we don't support payloads on other commands; but
+ * we can keep the connection alive by ignoring the payload.
+ */
+assert(request->type != NBD_CMD_WRITE);
+request->len = 0;


So, for sure, after this we go to

if (requests->flags & ~valid_flags)... and return -EINVAL.

Why we need to set request->len to 0? Just to not return "operation past EOF" instead of 
"unsupported flags", if len is too big? Maybe, that worth a comment.

Anyway, I now see nothing wrong in it, so:
Reviewed-by: Vladimir Sementsov-Ogievskiy 


+}
  if (allocate_buffer) {
  /* READ, WRITE */
  req->data = blk_try_blockalign(client->exp->common.blk,
@@ -2405,10 +2431,13 @@ static int coroutine_fn 
nbd_co_receive_request(NBDRequestData *req,
  }
  }
  if (payload_len) {
-/* WRITE */
-assert(req->data);
-ret = nbd_read(client->ioc, req->data

[PATCH 4/5] hw/m68k/next-cube: Remove unused NEXTDMA_EN code

[Thomas Huth]

The network code should reside in a separate file, so remove the
related handlers from next-cube.c.

Signed-off-by: Thomas Huth 
---
 hw/m68k/next-cube.c | 42 --
 1 file changed, 42 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 50f2cd0c61..d9a0dca07f 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -589,8 +589,6 @@ static const MemoryRegionOps scr_ops = {
 
 #define NEXTDMA_SCSI(x)  (0x10 + x)
 #define NEXTDMA_FD(x)(0x10 + x)
-#define NEXTDMA_ENTX(x)  (0x110 + x)
-#define NEXTDMA_ENRX(x)  (0x150 + x)
 #define NEXTDMA_CSR  0x0
 #define NEXTDMA_NEXT 0x4000
 #define NEXTDMA_LIMIT0x4004
@@ -605,37 +603,6 @@ static void dma_writel(void *opaque, hwaddr addr, uint64_t 
value,
 NeXTState *next_state = NEXT_MACHINE(opaque);
 
 switch (addr) {
-case NEXTDMA_ENRX(NEXTDMA_CSR):
-if (value & DMA_DEV2M) {
-next_state->dma[NEXTDMA_ENRX].csr |= DMA_DEV2M;
-}
-
-if (value & DMA_SETENABLE) {
-/* DPRINTF("SCSI DMA ENABLE\n"); */
-next_state->dma[NEXTDMA_ENRX].csr |= DMA_ENABLE;
-}
-if (value & DMA_SETSUPDATE) {
-next_state->dma[NEXTDMA_ENRX].csr |= DMA_SUPDATE;
-}
-if (value & DMA_CLRCOMPLETE) {
-next_state->dma[NEXTDMA_ENRX].csr &= ~DMA_COMPLETE;
-}
-
-if (value & DMA_RESET) {
-next_state->dma[NEXTDMA_ENRX].csr &= ~(DMA_COMPLETE | DMA_SUPDATE |
-  DMA_ENABLE | DMA_DEV2M);
-}
-/* DPRINTF("RXCSR \tWrite: %x\n",value); */
-break;
-case NEXTDMA_ENRX(NEXTDMA_NEXT_INIT):
-next_state->dma[NEXTDMA_ENRX].next_initbuf = value;
-break;
-case NEXTDMA_ENRX(NEXTDMA_NEXT):
-next_state->dma[NEXTDMA_ENRX].next = value;
-break;
-case NEXTDMA_ENRX(NEXTDMA_LIMIT):
-next_state->dma[NEXTDMA_ENRX].limit = value;
-break;
 case NEXTDMA_SCSI(NEXTDMA_CSR):
 if (value & DMA_DEV2M) {
 next_state->dma[NEXTDMA_SCSI].csr |= DMA_DEV2M;
@@ -692,15 +659,6 @@ static uint64_t dma_readl(void *opaque, hwaddr addr, 
unsigned int size)
 case NEXTDMA_SCSI(NEXTDMA_CSR):
 DPRINTF("SCSI DMA CSR READ\n");
 return next_state->dma[NEXTDMA_SCSI].csr;
-case NEXTDMA_ENRX(NEXTDMA_CSR):
-return next_state->dma[NEXTDMA_ENRX].csr;
-case NEXTDMA_ENRX(NEXTDMA_NEXT_INIT):
-return next_state->dma[NEXTDMA_ENRX].next_initbuf;
-case NEXTDMA_ENRX(NEXTDMA_NEXT):
-return next_state->dma[NEXTDMA_ENRX].next;
-case NEXTDMA_ENRX(NEXTDMA_LIMIT):
-return next_state->dma[NEXTDMA_ENRX].limit;
-
 case NEXTDMA_SCSI(NEXTDMA_NEXT):
 return next_state->dma[NEXTDMA_SCSI].next;
 case NEXTDMA_SCSI(NEXTDMA_NEXT_INIT):
-- 
2.41.0

[PATCH 2/5] hw/scsi/esp: Work around problem with PIO data read for the NeXT-Cube machine

[Thomas Huth]

The NeXT-Cube bios uses this mode in its selftest, and without
decreasing the amount of bytes in the fifo here, the selftest
fails.

Signed-off-by: Thomas Huth 
---
 hw/scsi/esp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index e52188d022..0d54efe826 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -961,7 +961,7 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
 (s->rregs[ESP_RSTAT] & STAT_PIO_MASK) == 0) {
 /* Data out.  */
 qemu_log_mask(LOG_UNIMP, "esp: PIO data read not implemented\n");
-s->rregs[ESP_FIFO] = 0;
+s->rregs[ESP_FIFO] = esp_fifo_pop(&s->fifo);
 } else {
 if ((s->rregs[ESP_RSTAT] & 0x7) == STAT_DI) {
 if (s->ti_size) {
-- 
2.41.0

[PATCH 5/5] m68k: Add NeXTcube network controller

[Thomas Huth]

The device is based on Bryce's code from GSoC 2011 that can be found here:

 https://github.com/blanham/qemu-NeXT/blob/next-cube/hw/next-net.c

The network boot unfortunately does not work yet (the firmware
successfully can send out packets, but still have problems receiving
the answer of the BOOTP server), but the code is also required to
avoid that the firmware hangs or crashes during its selftest, so it
makes sense to include this in the current shape already.

Signed-off-by: Thomas Huth 
---
 hw/m68k/next-cube.c |  32 ++-
 hw/net/meson.build  |   1 +
 hw/net/next-net.c   | 538 
 include/hw/m68k/next-cube.h |  10 +
 4 files changed, 579 insertions(+), 2 deletions(-)
 create mode 100644 hw/net/next-net.c

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index d9a0dca07f..bd12007a9f 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -28,6 +28,7 @@
 #include "ui/console.h"
 #include "target/m68k/cpu.h"
 #include "migration/vmstate.h"
+#include "net/net.h"
 
 /* #define DEBUG_NEXT */
 #ifdef DEBUG_NEXT
@@ -908,6 +909,32 @@ static void next_escc_init(DeviceState *pcdev)
 sysbus_mmio_map(s, 0, 0x2118000);
 }
 
+static void nextnet_init(DeviceState *pcdev)
+{
+DeviceState *dev;
+SysBusDevice *sbd;
+NICInfo *ni = &nd_table[0];
+int i;
+
+dev = qdev_new(TYPE_NEXT_NET);
+if (ni->used) {
+qemu_check_nic_model(ni, TYPE_NEXT_NET);
+qdev_set_nic_properties(dev, ni);
+}
+sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+
+sbd = SYS_BUS_DEVICE(dev);
+sysbus_mmio_map(sbd, 0, 0x02000110);
+sysbus_mmio_map(sbd, 1, 0x02004100);
+sysbus_mmio_map(sbd, 2, 0x02004310);
+sysbus_mmio_map(sbd, 3, 0x02106000);
+
+/* Set up TX/RX and DMA irqs */
+for (i = 0; i < NEXTNET_NUM_IRQS; i++) {
+sysbus_connect_irq(sbd, i, qdev_get_gpio_in(pcdev, NEXTNET_TX_I_DMA 
+i));
+}
+}
+
 static void next_pc_reset(DeviceState *dev)
 {
 NeXTPC *s = NEXT_PC(dev);
@@ -1081,14 +1108,15 @@ static void next_cube_init(MachineState *machine)
 /* Serial */
 next_escc_init(pcdev);
 
-/* TODO: */
-/* Network */
 /* SCSI */
 next_scsi_init(pcdev, cpu);
 
 /* DMA */
 memory_region_init_io(dmamem, NULL, &dma_ops, machine, "next.dma", 0x5000);
 memory_region_add_subregion(sysmem, 0x0200, dmamem);
+
+/* Network */
+nextnet_init(pcdev);
 }
 
 static void next_machine_class_init(ObjectClass *oc, void *data)
diff --git a/hw/net/meson.build b/hw/net/meson.build
index 2632634df3..8812d62f34 100644
--- a/hw/net/meson.build
+++ b/hw/net/meson.build
@@ -42,6 +42,7 @@ system_ss.add(when: 'CONFIG_NPCM7XX', if_true: 
files('npcm7xx_emc.c'))
 
 system_ss.add(when: 'CONFIG_ETRAXFS', if_true: files('etraxfs_eth.c'))
 system_ss.add(when: 'CONFIG_COLDFIRE', if_true: files('mcf_fec.c'))
+system_ss.add(when: 'CONFIG_NEXTCUBE', if_true: files('next-net.c'))
 specific_ss.add(when: 'CONFIG_PSERIES', if_true: files('spapr_llan.c'))
 system_ss.add(when: 'CONFIG_XILINX_ETHLITE', if_true: 
files('xilinx_ethlite.c'))
 
diff --git a/hw/net/next-net.c b/hw/net/next-net.c
new file mode 100644
index 00..67a7bf580b
--- /dev/null
+++ b/hw/net/next-net.c
@@ -0,0 +1,538 @@
+/*
+ * QEMU NeXT Network (MB8795) emulation
+ *
+ * Copyright (c) 2011 Bryce Lanham
+ *
+ * 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 "exec/address-spaces.h"
+#include "sysemu/sysemu.h"
+#include "hw/irq.h"
+#include "hw/m68k/next-cube.h"
+#include "hw/sysbus.h"
+#include "hw/qdev-properties.h"
+#include "net/net.h"
+
+/* debug NeXT ethernet */
+// #define DEBUG_NET
+
+#ifdef DEBUG_NET
+#define DPRINTF(fmt, ...) \
+do { printf("NET: " fmt , ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...) do { } while (0)
+#endif
+
+/* names could be better */
+typedef struct NextDMA {
+uint32_t csr;
+uint32_t savedbase;

[PATCH 1/5] hw/m68k/next-cube: Mirror BIOS to address 0

[Thomas Huth]

The ROM is also available at address 0, so add a proper mirror
for this address.

Signed-off-by: Thomas Huth 
---
 hw/m68k/next-cube.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 5d244b3b95..4ab9a5ec98 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -945,6 +945,7 @@ static void next_cube_init(MachineState *machine)
 M68kCPU *cpu;
 CPUM68KState *env;
 MemoryRegion *rom = g_new(MemoryRegion, 1);
+MemoryRegion *rom2 = g_new(MemoryRegion, 1);
 MemoryRegion *dmamem = g_new(MemoryRegion, 1);
 MemoryRegion *bmapm1 = g_new(MemoryRegion, 1);
 MemoryRegion *bmapm2 = g_new(MemoryRegion, 1);
@@ -998,9 +999,10 @@ static void next_cube_init(MachineState *machine)
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x0200e000);
 
 /* Load ROM here */
-/* still not sure if the rom should also be mapped at 0x0*/
 memory_region_init_rom(rom, NULL, "next.rom", 0x2, &error_fatal);
 memory_region_add_subregion(sysmem, 0x0100, rom);
+memory_region_init_alias(rom2, NULL, "next.rom2", rom, 0x0, 0x2);
+memory_region_add_subregion(sysmem, 0x0, rom2);
 if (load_image_targphys(bios_name, 0x0100, 0x2) < 8) {
 if (!qtest_enabled()) {
 error_report("Failed to load firmware '%s'.", bios_name);
-- 
2.41.0

[PATCH 3/5] m68k: Instantiate the ESP SCSI controller for the NeXTcube machine

[Thomas Huth]

The NeXTcube uses a NCR 53C90 SCSI interface for its disks, so we should
be able to use the ESP controller from QEMU here. The code here has been
basically taken from Bryce Lanham's GSoC 2011 contribution, except for
the next_scsi_init() function which has been rewritte as a replacement
for the esp_init() function (that has been removed quite a while ago).

Note that SCSI is not working yet. The ESP code likely needs some more
fixes first and there still might be some bugs left in they way we wire
it up for the NeXT-Cube machine.

Signed-off-by: Thomas Huth 
Signed-off-by: Mark Cave-Ayland 
---
 hw/m68k/next-cube.c | 116 +---
 1 file changed, 109 insertions(+), 7 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 4ab9a5ec98..50f2cd0c61 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -90,10 +90,13 @@ struct NeXTPC {
 
 uint32_t scr1;
 uint32_t scr2;
-uint8_t scsi_csr_1;
-uint8_t scsi_csr_2;
 uint32_t int_mask;
 uint32_t int_status;
+uint8_t scsi_csr_1;
+uint8_t scsi_csr_2;
+
+qemu_irq scsi_reset;
+qemu_irq scsi_dma;
 
 NextRtc rtc;
 };
@@ -466,7 +469,7 @@ static void scr_writeb(NeXTPC *s, hwaddr addr, uint32_t 
value)
 DPRINTF("SCSICSR FIFO Flush\n");
 /* will have to add another irq to the esp if this is needed */
 /* esp_puflush_fifo(esp_g); */
-/* qemu_irq_pulse(s->scsi_dma); */
+qemu_irq_pulse(s->scsi_dma);
 }
 
 if (value & SCSICSR_ENABLE) {
@@ -486,9 +489,9 @@ static void scr_writeb(NeXTPC *s, hwaddr addr, uint32_t 
value)
 if (value & SCSICSR_RESET) {
 DPRINTF("SCSICSR Reset\n");
 /* I think this should set DMADIR. CPUDMA and INTMASK to 0 */
-/* qemu_irq_raise(s->scsi_reset); */
-/* s->scsi_csr_1 &= ~(SCSICSR_INTMASK |0x80|0x1); */
-
+qemu_irq_raise(s->scsi_reset);
+s->scsi_csr_1 &= ~(SCSICSR_INTMASK |0x80|0x1);
+qemu_irq_lower(s->scsi_reset);
 }
 if (value & SCSICSR_DMADIR) {
 DPRINTF("SCSICSR DMAdir\n");
@@ -496,10 +499,11 @@ static void scr_writeb(NeXTPC *s, hwaddr addr, uint32_t 
value)
 if (value & SCSICSR_CPUDMA) {
 DPRINTF("SCSICSR CPUDMA\n");
 /* qemu_irq_raise(s->scsi_dma); */
-
 s->int_status |= 0x400;
 } else {
+/* fprintf(stderr,"SCSICSR CPUDMA disabled\n"); */
 s->int_status &= ~(0x400);
+/* qemu_irq_lower(s->scsi_dma); */
 }
 if (value & SCSICSR_INTMASK) {
 DPRINTF("SCSICSR INTMASK\n");
@@ -828,6 +832,102 @@ static void next_irq(void *opaque, int number, int level)
 }
 }
 
+static void nextdma_write(void *opaque, uint8_t *buf, int size, int type)
+{
+uint32_t base_addr;
+int irq = 0;
+uint8_t align = 16;
+NeXTState *next_state = NEXT_MACHINE(qdev_get_machine());
+
+if (type == NEXTDMA_ENRX || type == NEXTDMA_ENTX) {
+align = 32;
+}
+/* Most DMA is supposedly 16 byte aligned */
+if ((size % align) != 0) {
+size -= size % align;
+size += align;
+}
+
+/*
+ * prom sets the dma start using initbuf while the bootloader uses next
+ * so we check to see if initbuf is 0
+ */
+if (next_state->dma[type].next_initbuf == 0) {
+base_addr = next_state->dma[type].next;
+} else {
+base_addr = next_state->dma[type].next_initbuf;
+}
+
+cpu_physical_memory_write(base_addr, buf, size);
+
+next_state->dma[type].next_initbuf = 0;
+
+/* saved limit is checked to calculate packet size
+by both the rom and netbsd */
+next_state->dma[type].saved_limit = (next_state->dma[type].next + size);
+next_state->dma[type].saved_next  = (next_state->dma[type].next);
+
+/*32 bytes under savedbase seems to be some kind of register
+of which the purpose is unknown as of yet*/
+//stl_phys(s->rx_dma.base-32,0x);
+
+if (!(next_state->dma[type].csr & DMA_SUPDATE)) {
+next_state->dma[type].next  = next_state->dma[type].start;
+next_state->dma[type].limit = next_state->dma[type].stop;
+}
+
+/* Set dma registers and raise an irq */
+next_state->dma[type].csr |= DMA_COMPLETE; /* DON'T CHANGE THIS! */
+
+switch (type) {
+case NEXTDMA_SCSI:
+irq = NEXT_SCSI_DMA_I;
+break;
+}
+
+next_irq(opaque, irq, 1);
+next_irq(opaque, irq, 0);
+}
+
+static void nextscsi_read(void *opaque, uint8_t *buf, int len)
+{
+DPRINTF("SCSI READ: %x\n", len);
+abort();
+}
+
+static void nextscsi_write(void *opaque, uint8_t *buf, int size)
+{
+DPRINTF("SCSI WRITE: %i\n", size);
+nextdma_write(opaque, buf, size, NEXTDMA_SCSI);
+}
+
+static void next_scsi_init(DeviceState *pcdev, M68kCPU *cpu)
+{
+struct NeXTPC *next_pc = NEXT_PC(pcdev);
+DeviceState *dev;
+SysBusDevice *sysbusdev;
+ 

Re: [PATCH v7 12/12] nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS

[Vladimir Sementsov-Ogievskiy]

On 25.09.23 22:22, Eric Blake wrote:

Allow a client to request a subset of negotiated meta contexts.  For
example, a client may ask to use a single connection to learn about
both block status and dirty bitmaps, but where the dirty bitmap
queries only need to be performed on a subset of the disk; forcing the
server to compute that information on block status queries in the rest
of the disk is wasted effort (both at the server, and on the amount of
traffic sent over the wire to be parsed and ignored by the client).

Qemu as an NBD client never requests to use more than one meta
context, so it has no need to use block status payloads.  Testing this
instead requires support from libnbd, which CAN access multiple meta
contexts in parallel from a single NBD connection; an interop test
submitted to the libnbd project at the same time as this patch
demonstrates the feature working, as well as testing some corner cases
(for example, when the payload length is longer than the export
length), although other corner cases (like passing the same id
duplicated) requires a protocol fuzzer because libnbd is not wired up
to break the protocol that badly.

This also includes tweaks to 'qemu-nbd --list' to show when a server
is advertising the capability, and to the testsuite to reflect the
addition to that output.

Of note: qemu will always advertise the new feature bit during
NBD_OPT_INFO if extended headers have alreay been negotiated
(regardless of whether any NBD_OPT_SET_META_CONTEXT negotiation has
occurred); but for NBD_OPT_GO, qemu only advertises the feature if
block status is also enabled (that is, if the client does not
negotiate any contexts, then NBD_CMD_BLOCK_STATUS cannot be used, so
the feature is not advertised).

Signed-off-by: Eric Blake 
---



[..]



+/*
+ * nbd_co_block_status_payload_read
+ * Called when a client wants a subset of negotiated contexts via a
+ * BLOCK_STATUS payload.  Check the payload for valid length and
+ * contents.  On success, return 0 with request updated to effective
+ * length.  If request was invalid but all payload consumed, return 0
+ * with request->len and request->contexts->count set to 0 (which will
+ * trigger an appropriate NBD_EINVAL response later on).  Return
+ * negative errno if the payload was not fully consumed.
+ */
+static int
+nbd_co_block_status_payload_read(NBDClient *client, NBDRequest *request,
+ Error **errp)


[..]


+payload_len > (sizeof(NBDBlockStatusPayload) +
+   sizeof(id) * client->contexts.count)) {
+goto skip;
+}
+
+buf = g_malloc(payload_len);
+if (nbd_read(client->ioc, buf, payload_len,
+ "CMD_BLOCK_STATUS data", errp) < 0) {
+return -EIO;
+}
+trace_nbd_co_receive_request_payload_received(request->cookie,
+  payload_len);
+request->contexts->bitmaps = g_new0(bool, nr_bitmaps);
+count = (payload_len - sizeof(NBDBlockStatusPayload)) / sizeof(id);
+payload_len = 0;
+
+for (i = 0; i < count; i++) {
+id = ldl_be_p(buf + sizeof(NBDBlockStatusPayload) + sizeof(id) * i);
+if (id == NBD_META_ID_BASE_ALLOCATION) {
+if (request->contexts->base_allocation) {
+goto skip;
+}


should we also check that base_allocation is negotiated?


+request->contexts->base_allocation = true;
+} else if (id == NBD_META_ID_ALLOCATION_DEPTH) {
+if (request->contexts->allocation_depth) {
+goto skip;
+}


same here


+request->contexts->allocation_depth = true;
+} else {
+int idx = id - NBD_META_ID_DIRTY_BITMAP;
+


I think, we also should check that idx >=0 after this operation.


+if (idx > nr_bitmaps || request->contexts->bitmaps[idx]) {
+goto skip;
+}
+request->contexts->bitmaps[idx] = true;
+}
+}
+
+request->len = ldq_be_p(buf);
+request->contexts->count = count;
+return 0;
+
+ skip:
+trace_nbd_co_receive_block_status_payload_compliance(request->from,
+ request->len);
+request->len = request->contexts->count = 0;
+return nbd_drop(client->ioc, payload_len, errp);
+}
+


[..]


diff --git a/nbd/trace-events b/nbd/trace-events
index 8f4e20ee9f2..ac186c19ec0 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -70,6 +70,7 @@ nbd_co_send_chunk_read(uint64_t cookie, uint64_t offset, void 
*data, uint64_t si
  nbd_co_send_chunk_read_hole(uint64_t cookie, uint64_t offset, uint64_t size) "Send structured read 
hole reply: cookie = %" PRIu64 ", offset = %" PRIu64 ", len = %" PRIu64
  nbd_co_send_extents(uint64_t cookie, unsigned int extents, uint32_t id, uint64_t length, int last) 
"Send block status reply: cookie = %" PRIu64 ", extents = %u, context = %d (extents cover 
%" PRIu64 " bytes, last chunk = %d)"
  nbd_co_

[PATCH 0/5] m68k: Instantiate ESP and next-net in the next-cube machine

[Thomas Huth]

Mark Cave-Ayland recently asked me about the ESP patches for the
next-cube machine that I once posted a long time ago, but never
got it merged (since Mark is currently working on improving the
ESP device). With his help, I dusted off the ESP patch, but we
had to discover that the firmware also wants to see the NIC
device in the MMIO region, otherwise it often crashes or hangs.
So here's now the current set of patches for the next-cube machine.
Both, the ESP SCSI controller and the NIC do not properly work
yet, but at least the next-cube firmware gets much further now
during it selftest, so I think this is a good base for future
work and experiments.

Thomas Huth (5):
  hw/m68k/next-cube: Mirror BIOS to address 0
  hw/scsi/esp: Work around problem with PIO data read for the NeXT-Cube
machine
  m68k: Instantiate the ESP SCSI controller for the NeXTcube machine
  hw/m68k/next-cube: Remove unused NEXTDMA_EN code
  m68k: Add NeXTcube network controller

 hw/m68k/next-cube.c | 194 +
 hw/net/meson.build  |   1 +
 hw/net/next-net.c   | 538 
 hw/scsi/esp.c   |   2 +-
 include/hw/m68k/next-cube.h |  10 +
 5 files changed, 692 insertions(+), 53 deletions(-)
 create mode 100644 hw/net/next-net.c

-- 
2.41.0

Re: [PATCH 4/7] tcg/loongarch64: Use cpuinfo.h

[Jiajie Chen]

On 2023/9/17 06:01, Richard Henderson wrote:

Signed-off-by: Richard Henderson 
---
  tcg/loongarch64/tcg-target.h | 8 
  tcg/loongarch64/tcg-target.c.inc | 8 +---
  2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/tcg/loongarch64/tcg-target.h b/tcg/loongarch64/tcg-target.h
index 03017672f6..1bea15b02e 100644
--- a/tcg/loongarch64/tcg-target.h
+++ b/tcg/loongarch64/tcg-target.h
@@ -29,6 +29,8 @@
  #ifndef LOONGARCH_TCG_TARGET_H
  #define LOONGARCH_TCG_TARGET_H
  
+#include "host/cpuinfo.h"

+
  #define TCG_TARGET_INSN_UNIT_SIZE 4
  #define TCG_TARGET_NB_REGS 64
  
@@ -85,8 +87,6 @@ typedef enum {

  TCG_VEC_TMP0 = TCG_REG_V23,
  } TCGReg;
  
-extern bool use_lsx_instructions;

-
  /* used for function call generation */
  #define TCG_REG_CALL_STACK  TCG_REG_SP
  #define TCG_TARGET_STACK_ALIGN  16
@@ -171,10 +171,10 @@ extern bool use_lsx_instructions;
  #define TCG_TARGET_HAS_muluh_i641
  #define TCG_TARGET_HAS_mulsh_i641
  
-#define TCG_TARGET_HAS_qemu_ldst_i128   use_lsx_instructions

+#define TCG_TARGET_HAS_qemu_ldst_i128   (cpuinfo & CPUINFO_LSX)
  
  #define TCG_TARGET_HAS_v64  0

-#define TCG_TARGET_HAS_v128 use_lsx_instructions
+#define TCG_TARGET_HAS_v128 (cpuinfo & CPUINFO_LSX)
  #define TCG_TARGET_HAS_v256 0
  
  #define TCG_TARGET_HAS_not_vec  1

diff --git a/tcg/loongarch64/tcg-target.c.inc b/tcg/loongarch64/tcg-target.c.inc
index 40074c46b8..52f2c26ce1 100644
--- a/tcg/loongarch64/tcg-target.c.inc
+++ b/tcg/loongarch64/tcg-target.c.inc
@@ -32,8 +32,6 @@
  #include "../tcg-ldst.c.inc"
  #include 
  
-bool use_lsx_instructions;

-
  #ifdef CONFIG_DEBUG_TCG
  static const char * const tcg_target_reg_names[TCG_TARGET_NB_REGS] = {
  "zero",
@@ -2316,10 +2314,6 @@ static void tcg_target_init(TCGContext *s)
  exit(EXIT_FAILURE);
  }
  
-if (hwcap & HWCAP_LOONGARCH_LSX) {

-use_lsx_instructions = 1;
-}
-
  tcg_target_available_regs[TCG_TYPE_I32] = ALL_GENERAL_REGS;
  tcg_target_available_regs[TCG_TYPE_I64] = ALL_GENERAL_REGS;
  
@@ -2335,7 +2329,7 @@ static void tcg_target_init(TCGContext *s)

  tcg_regset_reset_reg(tcg_target_call_clobber_regs, TCG_REG_S8);
  tcg_regset_reset_reg(tcg_target_call_clobber_regs, TCG_REG_S9);
  
-if (use_lsx_instructions) {

+if (cpuinfo & CPUINFO_LSX) {
  tcg_target_available_regs[TCG_TYPE_V128] = ALL_VECTOR_REGS;
  tcg_regset_reset_reg(tcg_target_call_clobber_regs, TCG_REG_V24);
  tcg_regset_reset_reg(tcg_target_call_clobber_regs, TCG_REG_V25);



Reviewed-by: Jiajie Chen 

Re: [PATCH 2/7] tcg/loongarch64: Use C_N2_I1 for INDEX_op_qemu_ld_a*_i128

[Jiajie Chen]

On 2023/9/17 06:01, Richard Henderson wrote:

Use new registers for the output, so that we never overlap
the input address, which could happen for user-only.
This avoids a "tmp = addr + 0" in that case.

Signed-off-by: Richard Henderson 
---
  tcg/loongarch64/tcg-target-con-set.h |  2 +-
  tcg/loongarch64/tcg-target.c.inc | 17 +++--
  2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/tcg/loongarch64/tcg-target-con-set.h 
b/tcg/loongarch64/tcg-target-con-set.h
index 77d62e38e7..cae6c2aad6 100644
--- a/tcg/loongarch64/tcg-target-con-set.h
+++ b/tcg/loongarch64/tcg-target-con-set.h
@@ -38,4 +38,4 @@ C_O1_I2(w, w, wM)
  C_O1_I2(w, w, wA)
  C_O1_I3(w, w, w, w)
  C_O1_I4(r, rZ, rJ, rZ, rZ)
-C_O2_I1(r, r, r)
+C_N2_I1(r, r, r)
diff --git a/tcg/loongarch64/tcg-target.c.inc b/tcg/loongarch64/tcg-target.c.inc
index b701df50db..40074c46b8 100644
--- a/tcg/loongarch64/tcg-target.c.inc
+++ b/tcg/loongarch64/tcg-target.c.inc
@@ -1105,13 +1105,18 @@ static void tcg_out_qemu_ldst_i128(TCGContext *s, 
TCGReg data_lo, TCGReg data_hi
  }
  } else {
  /* Otherwise use a pair of LD/ST. */
-tcg_out_opc_add_d(s, TCG_REG_TMP0, h.base, h.index);
+TCGReg base = h.base;
+if (h.index != TCG_REG_ZERO) {
+base = TCG_REG_TMP0;
+tcg_out_opc_add_d(s, base, h.base, h.index);
+}
  if (is_ld) {
-tcg_out_opc_ld_d(s, data_lo, TCG_REG_TMP0, 0);
-tcg_out_opc_ld_d(s, data_hi, TCG_REG_TMP0, 8);
+tcg_debug_assert(base != data_lo);
+tcg_out_opc_ld_d(s, data_lo, base, 0);
+tcg_out_opc_ld_d(s, data_hi, base, 8);
  } else {
-tcg_out_opc_st_d(s, data_lo, TCG_REG_TMP0, 0);
-tcg_out_opc_st_d(s, data_hi, TCG_REG_TMP0, 8);
+tcg_out_opc_st_d(s, data_lo, base, 0);
+tcg_out_opc_st_d(s, data_hi, base, 8);
  }
  }
  
@@ -2049,7 +2054,7 @@ static TCGConstraintSetIndex tcg_target_op_def(TCGOpcode op)
  
  case INDEX_op_qemu_ld_a32_i128:

  case INDEX_op_qemu_ld_a64_i128:
-return C_O2_I1(r, r, r);
+return C_N2_I1(r, r, r);
  
  case INDEX_op_qemu_st_a32_i128:

  case INDEX_op_qemu_st_a64_i128:



Reviewed-by: Jiajie Chen 

Re: [PATCH 3/7] util: Add cpuinfo for loongarch64

[Jiajie Chen]

On 2023/9/17 06:01, Richard Henderson wrote:

Signed-off-by: Richard Henderson 
---
  host/include/loongarch64/host/cpuinfo.h | 21 +++
  util/cpuinfo-loongarch.c| 35 +
  util/meson.build|  2 ++
  3 files changed, 58 insertions(+)
  create mode 100644 host/include/loongarch64/host/cpuinfo.h
  create mode 100644 util/cpuinfo-loongarch.c

diff --git a/host/include/loongarch64/host/cpuinfo.h 
b/host/include/loongarch64/host/cpuinfo.h
new file mode 100644
index 00..fab664a10b
--- /dev/null
+++ b/host/include/loongarch64/host/cpuinfo.h
@@ -0,0 +1,21 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Host specific cpu identification for LoongArch
+ */
+
+#ifndef HOST_CPUINFO_H
+#define HOST_CPUINFO_H
+
+#define CPUINFO_ALWAYS  (1u << 0)  /* so cpuinfo is nonzero */
+#define CPUINFO_LSX (1u << 1)
+
+/* Initialized with a constructor. */
+extern unsigned cpuinfo;
+
+/*
+ * We cannot rely on constructor ordering, so other constructors must
+ * use the function interface rather than the variable above.
+ */
+unsigned cpuinfo_init(void);
+
+#endif /* HOST_CPUINFO_H */
diff --git a/util/cpuinfo-loongarch.c b/util/cpuinfo-loongarch.c
new file mode 100644
index 00..08b6d7460c
--- /dev/null
+++ b/util/cpuinfo-loongarch.c
@@ -0,0 +1,35 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Host specific cpu identification for LoongArch.
+ */
+
+#include "qemu/osdep.h"
+#include "host/cpuinfo.h"
+
+#ifdef CONFIG_GETAUXVAL
+# include 
+#else
+# include "elf.h"
+#endif
+#include 
+
+unsigned cpuinfo;
+
+/* Called both as constructor and (possibly) via other constructors. */
+unsigned __attribute__((constructor)) cpuinfo_init(void)
+{
+unsigned info = cpuinfo;
+unsigned long hwcap;
+
+if (info) {
+return info;
+}
+
+hwcap = qemu_getauxval(AT_HWCAP);
+
+info = CPUINFO_ALWAYS;
+info |= (hwcap & HWCAP_LOONGARCH_LSX ? CPUINFO_LSX : 0);
+
+cpuinfo = info;
+return info;
+}
diff --git a/util/meson.build b/util/meson.build
index c4827fd70a..b136f02aa0 100644
--- a/util/meson.build
+++ b/util/meson.build
@@ -112,6 +112,8 @@ if cpu == 'aarch64'
util_ss.add(files('cpuinfo-aarch64.c'))
  elif cpu in ['x86', 'x86_64']
util_ss.add(files('cpuinfo-i386.c'))
+elif cpu == 'loongarch64'
+  util_ss.add(files('cpuinfo-loongarch.c'))
  elif cpu in ['ppc', 'ppc64']
util_ss.add(files('cpuinfo-ppc.c'))
  endif



Reviewed-by: Jiajie Chen 

Re: [PATCH 1/7] tcg: Add C_N2_I1

[Jiajie Chen]

On 2023/9/17 06:01, Richard Henderson wrote:

Constraint with two outputs, both in new registers.

Signed-off-by: Richard Henderson 
---
  tcg/tcg.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index 604fa9bf3e..fdbf79689a 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -644,6 +644,7 @@ static void tcg_out_movext3(TCGContext *s, const 
TCGMovExtend *i1,
  #define C_O1_I4(O1, I1, I2, I3, I4) C_PFX5(c_o1_i4_, O1, I1, I2, I3, I4),
  
  #define C_N1_I2(O1, I1, I2) C_PFX3(c_n1_i2_, O1, I1, I2),

+#define C_N2_I1(O1, O2, I1) C_PFX3(c_n2_i1_, O1, O2, I1),
  
  #define C_O2_I1(O1, O2, I1) C_PFX3(c_o2_i1_, O1, O2, I1),

  #define C_O2_I2(O1, O2, I1, I2) C_PFX4(c_o2_i2_, O1, O2, I1, I2),
@@ -666,6 +667,7 @@ static TCGConstraintSetIndex tcg_target_op_def(TCGOpcode);
  #undef C_O1_I3
  #undef C_O1_I4
  #undef C_N1_I2
+#undef C_N2_I1
  #undef C_O2_I1
  #undef C_O2_I2
  #undef C_O2_I3
@@ -685,6 +687,7 @@ static TCGConstraintSetIndex tcg_target_op_def(TCGOpcode);
  #define C_O1_I4(O1, I1, I2, I3, I4) { .args_ct_str = { #O1, #I1, #I2, 
#I3, #I4 } },
  
  #define C_N1_I2(O1, I1, I2) { .args_ct_str = { "&" #O1, #I1, #I2 } },

+#define C_N2_I1(O1, O2, I1) { .args_ct_str = { "&" #O1, "&" #O2, 
#I1 } },
  
  #define C_O2_I1(O1, O2, I1) { .args_ct_str = { #O1, #O2, #I1 } },

  #define C_O2_I2(O1, O2, I1, I2) { .args_ct_str = { #O1, #O2, #I1, #I2 
} },
@@ -706,6 +709,7 @@ static const TCGTargetOpDef constraint_sets[] = {
  #undef C_O1_I3
  #undef C_O1_I4
  #undef C_N1_I2
+#undef C_N2_I1
  #undef C_O2_I1
  #undef C_O2_I2
  #undef C_O2_I3
@@ -725,6 +729,7 @@ static const TCGTargetOpDef constraint_sets[] = {
  #define C_O1_I4(O1, I1, I2, I3, I4) C_PFX5(c_o1_i4_, O1, I1, I2, I3, I4)
  
  #define C_N1_I2(O1, I1, I2) C_PFX3(c_n1_i2_, O1, I1, I2)

+#define C_N2_I1(O1, O2, I1) C_PFX3(c_n2_i1_, O1, O2, I1)
  
  #define C_O2_I1(O1, O2, I1) C_PFX3(c_o2_i1_, O1, O2, I1)

  #define C_O2_I2(O1, O2, I1, I2) C_PFX4(c_o2_i2_, O1, O2, I1, I2)



Reviewed-by: Jiajie Chen 

[PATCH] target/loongarch: fix ASXE flag conflict

[Jiajie Chen]

HW_FLAGS_EUEN_ASXE acccidentally conflicts with HW_FLAGS_CRMD_PG,
enabling LASX instructions even when CSR_EUEN.ASXE=0.

Closes: https://gitlab.com/qemu-project/qemu/-/issues/1907
Signed-off-by: Jiajie Chen 
---
 target/loongarch/cpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
index f125a8e49b..79ad79a289 100644
--- a/target/loongarch/cpu.h
+++ b/target/loongarch/cpu.h
@@ -462,7 +462,7 @@ static inline void set_pc(CPULoongArchState *env, uint64_t 
value)
 #define HW_FLAGS_CRMD_PGR_CSR_CRMD_PG_MASK   /* 0x10 */
 #define HW_FLAGS_EUEN_FPE   0x04
 #define HW_FLAGS_EUEN_SXE   0x08
-#define HW_FLAGS_EUEN_ASXE  0x10
+#define HW_FLAGS_EUEN_ASXE  0x40
 #define HW_FLAGS_VA32   0x20
 
 static inline void cpu_get_tb_cpu_state(CPULoongArchState *env, vaddr *pc,
-- 
2.41.0

Re: [PATCH v3 05/20] q800: add IOSB subsystem

[BALATON Zoltan]

On Fri, 29 Sep 2023, Mark Cave-Ayland wrote:

It is needed because it defines the BIOSConfig area.

Co-developed-by: Laurent Vivier 
Signed-off-by: Mark Cave-Ayland 
---
MAINTAINERS|   2 +
hw/m68k/Kconfig|   1 +
hw/m68k/q800.c |   9 +++
hw/misc/Kconfig|   3 +
hw/misc/iosb.c | 133 +
hw/misc/meson.build|   1 +
hw/misc/trace-events   |   4 ++
include/hw/m68k/q800.h |   2 +
include/hw/misc/iosb.h |  25 
9 files changed, 180 insertions(+)
create mode 100644 hw/misc/iosb.c
create mode 100644 include/hw/misc/iosb.h

diff --git a/MAINTAINERS b/MAINTAINERS
index ae212542fa..8f5a51b351 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1229,6 +1229,7 @@ F: hw/nubus/*
F: hw/display/macfb.c
F: hw/block/swim.c
F: hw/misc/djmemc.c
+F: hw/misc/iosb.c
F: hw/m68k/bootinfo.h
F: include/standard-headers/asm-m68k/bootinfo.h
F: include/standard-headers/asm-m68k/bootinfo-mac.h
@@ -1239,6 +1240,7 @@ F: include/hw/block/swim.h
F: include/hw/m68k/q800.h
F: include/hw/m68k/q800-glue.h
F: include/hw/misc/djmemc.h
+F: include/hw/misc/iosb.h

virt
M: Laurent Vivier 
diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig
index 330cfdfa2d..64fa70a0db 100644
--- a/hw/m68k/Kconfig
+++ b/hw/m68k/Kconfig
@@ -24,6 +24,7 @@ config Q800
select DP8393X
select OR_IRQ
select DJMEMC
+select IOSB

config M68K_VIRT
bool
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index ac8509ba6f..081b95e9cf 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -41,6 +41,7 @@
#include "hw/m68k/q800-glue.h"
#include "hw/misc/mac_via.h"
#include "hw/misc/djmemc.h"
+#include "hw/misc/iosb.h"
#include "hw/input/adb.h"
#include "hw/nubus/mac-nubus-bridge.h"
#include "hw/display/macfb.h"
@@ -71,6 +72,7 @@
#define ESP_BASE  (IO_BASE + 0x1)
#define ESP_PDMA  (IO_BASE + 0x10100)
#define ASC_BASE  (IO_BASE + 0x14000)
+#define IOSB_BASE (IO_BASE + 0x18000)
#define SWIM_BASE (IO_BASE + 0x1E000)

#define SONIC_PROM_SIZE   0x1000
@@ -296,6 +298,13 @@ static void q800_machine_init(MachineState *machine)
memory_region_add_subregion(&m->macio, DJMEMC_BASE - IO_BASE,
sysbus_mmio_get_region(sysbus, 0));

+/* IOSB subsystem */
+object_initialize_child(OBJECT(machine), "iosb", &m->iosb, TYPE_IOSB);
+sysbus = SYS_BUS_DEVICE(&m->iosb);
+sysbus_realize_and_unref(sysbus, &error_fatal);
+memory_region_add_subregion(&m->macio, IOSB_BASE - IO_BASE,
+sysbus_mmio_get_region(sysbus, 0));
+
/* VIA 1 */
object_initialize_child(OBJECT(machine), "via1", &m->via1,
TYPE_MOS6522_Q800_VIA1);
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index cb7857e3ed..858277bb60 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -189,4 +189,7 @@ config AXP2XX_PMU
config DJMEMC
bool

+config IOSB
+bool
+
source macio/Kconfig
diff --git a/hw/misc/iosb.c b/hw/misc/iosb.c
new file mode 100644
index 00..e7e9dcca47
--- /dev/null
+++ b/hw/misc/iosb.c
@@ -0,0 +1,133 @@
+/*
+ * QEMU IOSB emulation
+ *
+ * Copyright (c) 2019 Laurent Vivier
+ * Copyright (c) 2022 Mark Cave-Ayland
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "migration/vmstate.h"
+#include "hw/sysbus.h"
+#include "hw/misc/iosb.h"
+#include "trace.h"
+
+#define IOSB_SIZE  0x2000
+
+#define IOSB_CONFIG0x0
+#define IOSB_CONFIG2   0x100
+#define IOSB_SONIC_SCSI0x200
+#define IOSB_REVISION  0x300
+#define IOSB_SCSI_RESID0x400
+#define IOSB_BRIGHTNESS0x500
+#define IOSB_TIMEOUT   0x600
+
+
+static uint64_t iosb_read(void *opaque, hwaddr addr,
+  unsigned size)
+{
+IOSBState *s = IOSB(opaque);


QOM cast here and in write function aren't necessary because these will be 
called with the object passed to memory_region_init_io in the init 
function where it's already cast so you can be sure here it's the right 
type and thus assign directly without a cast. This avoids some asserts on 
accessing the device which probably only matters for devices accessed very 
frequently but I think that's a good convention to follow for all devices.


Independent of this comment:

Reviewed-by: BALATON Zoltan 

Regards,
BALATON Zoltan


+uint64_t val = 0;
+
+switch (addr) {
+case IOSB_CONFIG:
+case IOSB_CONFIG2:
+case IOSB_SONIC_SCSI:
+case IOSB_REVISION:
+case IOSB_SCSI_RESID:
+case IOSB_BRIGHTNESS:
+case IOSB_TIMEOUT:
+val = s->regs[addr >> 8];
+break;
+default:
+qemu_log_mask(LOG_UNIMP, "IOSB: unimplemented read addr=0x%"PRIx64
+ " val=0x%"PRIx64 " size=%d\n",
+ addr, val, size);
+}
+
+trace_iosb_read(addr, val, size);
+return val;
+}
+
+static void iosb_write(void *opaque, hwaddr addr, uint64_t val,
+   

Re: [PATCH v11 6/9] gfxstream + rutabaga: add initial support for gfxstream

[Thomas Huth]

On 29/09/2023 17.06, Bernhard Beschow wrote:



Am 21. September 2023 23:44:42 UTC schrieb Gurchetan Singh 
:

On Tue, Sep 19, 2023 at 3:07 PM Akihiko Odaki 
wrote:


On 2023/09/20 3:36, Bernhard Beschow wrote:



Am 15. September 2023 02:38:02 UTC schrieb Gurchetan Singh <

gurchetansi...@chromium.org>:

On Thu, Sep 14, 2023 at 12:23 AM Bernhard Beschow 

wrote:

...

First, sorry for responding after such a long time. I've been busy

with

work and I'm doing QEMU in my free time.

In iteration 1 I've raised the topic on capset_names [1] and I

haven't

seen it answered properly. Perhaps I need to rephrase a bit so here

we

go:

capset_names seems to be colon-separated list of bit options managed

by

rutabaga. This introduces yet another way of options handling. There

have

been talks about harmonizing options handling in QEMU since

apparently

it

is considered too complex [2,3].




Why not pass the "capset" as a bitfield like capset_mask and have

QEMU

create "capset" from QOM properties?


IIUC these flags could come from virtio_gpu.h which is already

present in

the QEMU tree. This would not inly shortcut the dependency on

rutabaga

here

but would also be more idiomatic QEMU (since it makes the options

more

introspectable by internal machinery).




Of course the bitfield approach would require modifications in QEMU
whenever rutabaga gains new features. However, I figure that in the

long

term rutabaga will be quite feature complete such that the benefits

of

idiomatic QEMU handling will outweigh the decoupling of the projects.

What do you think?



I think what you're suggesting is something like -device
virtio-gpu-rutabaga,capset_mask=0x10100 [40, which would be
gfxstream_vulkan + cross_domain]?


I was thinking more along the lines of
`virtio-gpu-rutabaga,gfxstream_vulkan=on,cross_domain=on` where
gfxstream_vulkan and cross_domain are boolean QOM properties. This

would

make for a human-readable format which follows QEMU style.



We actually did consider something like that when adding the
--context-types flag [with crosvm], but there was a desire for a
human-readable format rather than numbers [even if they are in the
virtio-gpu spec].

Additionally, there are quite a few context types that people are

playing

around with [gfxstream-gles, gfxstream-composer] that are launchable

and

aren't in the spec just yet.


Right, QEMU had to be modified for this kind of experimentation. I
considered this in my last paragraph and figured that in the long run

QEMU

*may* prefer more idiomatic option handling since it tries hard to not
break its command line interface. I'm just pointing this out -- the
decision is ultimately up to the community.

Why not have dedicated QEMU development branches for experimentation?
Wouldn't upstreaming new features into QEMU be a good motivation to

get the

missing pieces into the spec, once they are mature?





Also, a key feature we want to explicitly **not** turn on all

available

context-types and let the user decide.


How would you prevent that with the current colon-separated approach?
Splitting capset_mask in multiple parameters is just a different
syntactical representation of the same thing.


That'll allow guest Mesa in
particular to do its magic in its loader.  So one may run Zink + ANV

with

ioctl forwarding, or Iris + ioctl forwarding and compare performance

with

the same guest image.

And another thing is one needs some knowledge of the host system to

choose

the right context type.  You wouldn't do Zink + ANV ioctl forwarding

on

MacOS.  So I think the task of choosing the right context type will

fall

to

projects that depend on QEMU (such as Android Emulator) which have

some

knowledge of the host environment.

We actually have a graphics detector somewhere that calls VK/OpenGL

before

launching the VM and sets the right options.  Plan is to port into
gfxstream, maybe we could use that.


You could bail out in QEMU if rutabaga_calculate_capset_mask() detects
conflicting combinations, no?



So given the desire for human readable formats, being portable across

VMMs

(crosvm, qemu, rust-vmm??) and experimentation, the string -> capset

mask

conversion was put in the rutabaga API.  So I wouldn't change it for

those

reasons.


What do you mean by being portable across VMMs?



Having the API inside rutabaga is (mildly) useful when multiple VMMs

have

the need to translate from a human-readable format to flags digestible

by

rutabaga.



https://android.googlesource.com/device/google/cuttlefish/+/refs/heads/main/host/libs/vm_manager/qemu_manager.cpp#452




https://android.googlesource.com/device/google/cuttlefish/+/refs/heads/main/host/libs/vm_manager/crosvm_manager.cpp#353




https://chromium.googlesource.com/chromiumos/platform2/+/refs/heads/main/vm_tools/concierge/vm_builder.cc#505


For these crosvm/qemu launchers, I imagine capset names will be plumbed

all

the way through eventually (launch_cvd
--gpu_context=gfxstream-

Re: [PULL 38/56] hw/acpi: changes towards enabling -Wshadow=local

[Markus Armbruster]

Ani Sinha  writes:

>> On 29-Sep-2023, at 2:20 PM, Markus Armbruster  wrote:
>> 
>> From: Ani Sinha 
>> 
>> Code changes in acpi that addresses all compiler complaints coming from 
>> enabling
>> -Wshadow flags. Enabling -Wshadow catches cases of local variables shadowing
>> other local variables or parameters. These makes the code confusing and/or 
>> adds
>> bugs that are difficult to catch.  See also
>> 
>>Subject: Help wanted for enabling -Wshadow=local
>>Message-Id: <87r0mqlf9x@pond.sub.org>
>>https://lore.kernel.org/qemu-devel/87r0mqlf9x@pond.sub.org
>> 
>> The code is tested to build with and without the flag turned on.
>> 
>> CC: Markus Armbruster 
>> CC: Philippe Mathieu-Daude 
>> CC: m...@redhat.com
>> CC: imamm...@redhat.com
>> Signed-off-by: Ani Sinha 
>> Message-ID: <20230922124203.127110-1-anisi...@redhat.com>
>> Reviewed-by: Michael S. Tsirkin 
>> [Commit message tweaked]
>> Signed-off-by: Markus Armbruster 
>
> Thanks!
> Are you not going to pick up "hw/i386: changes towards enabling 
> -Wshadow=local” ?

Your conversation with Michael looked unfinished to me, so I didn't
include your patch in my pull request.  I did add it to shadow-next.