Re: [PATCH 09/14] tests/qapi-schema: Improve union discriminator coverage

[Markus Armbruster]

Eric Blake  writes:

> On Thu, Mar 16, 2023 at 08:13:20AM +0100, Markus Armbruster wrote:
>> A union's 'discriminator' must name a one of the common members.
>
> s/ a//

Yes.

>> QAPISchemaVariants.check() looks it up by its c_name(), then checks
>> the name matches exactly (because c_name() is not injective).
>> 
>> Tests union-base-empty and union-invalid-discriminator both cover the
>> case where lookup fails.  Repurpose the latter to cover the case where
>> it succeeds and the name check fails.
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  tests/qapi-schema/union-invalid-discriminator.err  | 2 +-
>>  tests/qapi-schema/union-invalid-discriminator.json | 4 ++--
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>> 
>
> Reviewed-by: Eric Blake 
>
> (- vs. _ is subtle, especially since I purposefully case-map them to
> one another whenever I can...)

Abusing the clash checking machinery to look up the the tag member is
kind of hacky, and it's why we have this odd case to cover.

Thanks!

Re: [PATCH 08/14] qapi: Fix to reject 'data': 'mumble' in struct

[Markus Armbruster]

Eric Blake  writes:

> On Thu, Mar 16, 2023 at 08:13:19AM +0100, Markus Armbruster wrote:
>> A struct's 'data' must be a JSON object defining the struct's members.
>> The QAPI code generator incorrectly accepts a JSON string instead, and
>> then crashes in QAPISchema._make_members() called from
>> ._def_struct_type().
>> 
>> Fix to reject it, and add a test case.
>
> Nice catch; I see why the split into three functions earlier on
> foreshadowed some subtle bug fixes to come.
>
>> +++ b/scripts/qapi/expr.py
>> @@ -354,14 +354,14 @@ def check_type_name_or_array(value: Optional[object],
>> source)
>>  
>>  
>> -def check_type_name_or_implicit(value: Optional[object],
>> -info: QAPISourceInfo, source: str,
>> -parent_name: Optional[str]) -> None:
>> +def check_type_implicit(value: Optional[object],
>> +info: QAPISourceInfo, source: str,
>> +parent_name: Optional[str]) -> None:
>
> At first I thought this was a straight rename...
>
>>  """
>>  Normalize and validate an optional implicit struct type.
>>  
>> -Accept ``None``, ``str``, or a ``dict`` defining an implicit
>> -struct type.  The latter is normalized in place.
>> +Accept ``None`` or a ``dict`` defining an implicit struct type.
>> +The latter is normalized in place.
>>  
>>  :param value: The value to check.
>>  :param info: QAPI schema source file information.
>> @@ -377,9 +377,6 @@ def check_type_name_or_implicit(value: Optional[object],
>>  if value is None:
>>  return
>>  
>> -if isinstance(value, str):
>> -return
>> -
>>  if not isinstance(value, dict):
>>  raise QAPISemError(info,
>> "%s should be an object or type name" % source)
>> @@ -401,6 +398,15 @@ def check_type_name_or_implicit(value: Optional[object],
>>  check_type_name_or_array(arg['type'], info, key_source)
>>  
>>  
>> +def check_type_name_or_implicit(value: Optional[object],
>> +info: QAPISourceInfo, source: str,
>> +parent_name: Optional[str]) -> None:
>> +if value is None or isinstance(value, str):
>
> ...until I got here and saw that you kept the original name, and added
> a new helper.  Worth calling out the new function name
> check_type_implicit() in the commit message?  It would have spared me
> a minute.

Can do.

> As earlier, you lost the doc comment.  I'll leave it to your
> discretion if it is important to copy one back in.

I didn't duplicate the doc string, which means it moves from
check_type_name_or_implicit() to check_type_implicit(), where the actual
meat is.

John, you added the doc string in commit a48653638fa (qapi/expr.py: Add
docstrings).  Do you have an opinion?

>> +return
>> +
>> +check_type_implicit(value, info, source, parent_name)
>> +
>> +
>>  def check_features(features: Optional[object],
>> info: QAPISourceInfo) -> None:
>>  """
>> @@ -486,7 +492,7 @@ def check_struct(expr: QAPIExpression) -> None:
>>  name = cast(str, expr['struct'])  # Checked in check_exprs
>>  members = expr['data']
>>  
>> -check_type_name_or_implicit(members, expr.info, "'data'", name)
>> +check_type_implicit(members, expr.info, "'data'", name)
>>  check_type_name(expr.get('base'), expr.info, "'base'")
>>
>
> Reviewed-by: Eric Blake 

Thanks!

Re: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor

[Akihiko Odaki]

On 2023/03/17 5:27, Sriram Yagnaraman wrote:



-Original Message-
From: qemu-devel-bounces+sriram.yagnaraman=est.t...@nongnu.org
 On Behalf
Of Akihiko Odaki
Sent: Thursday, 16 March 2023 16:57
Cc: qemu-devel@nongnu.org; Jason Wang ; Dmitry
Fleytman ; quint...@redhat.com; Philippe
Mathieu-Daudé ; Akihiko Odaki

Subject: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor

The current implementation of igb uses only part of a advanced Tx context
descriptor because it misses some features and sniffs the trait of the packet
instead of respecting the packet type specified in the descriptor. However, we
will certainly need the entire Tx context descriptor when we update igb to
respect these ignored fields. Save the entire Tx context descriptor to prepare
for such a change.

Signed-off-by: Akihiko Odaki 
---
V1 -> V2: Bump igb-tx version

  hw/net/igb.c  | 10 ++
  hw/net/igb_core.c | 17 ++---  hw/net/igb_core.h |  3 +--
  3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/hw/net/igb.c b/hw/net/igb.c index c6d753df87..f9ec82fc28 100644
--- a/hw/net/igb.c
+++ b/hw/net/igb.c
@@ -504,11 +504,13 @@ static int igb_post_load(void *opaque, int
version_id)

  static const VMStateDescription igb_vmstate_tx = {
  .name = "igb-tx",
-.version_id = 1,
-.minimum_version_id = 1,
+.version_id = 2,
+.minimum_version_id = 2,
  .fields = (VMStateField[]) {
-VMSTATE_UINT16(vlan, struct igb_tx),
-VMSTATE_UINT16(mss, struct igb_tx),
+VMSTATE_UINT32(ctx.vlan_macip_lens, struct igb_tx),
+VMSTATE_UINT32(ctx.seqnum_seed, struct igb_tx),
+VMSTATE_UINT32(ctx.type_tucmd_mlhl, struct igb_tx),
+VMSTATE_UINT32(ctx.mss_l4len_idx, struct igb_tx),
  VMSTATE_BOOL(tse, struct igb_tx),
  VMSTATE_BOOL(ixsm, struct igb_tx),
  VMSTATE_BOOL(txsm, struct igb_tx), diff --git a/hw/net/igb_core.c
b/hw/net/igb_core.c index a7c7bfdc75..304f5d849f 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -390,7 +390,8 @@ static bool
  igb_setup_tx_offloads(IGBCore *core, struct igb_tx *tx)  {
  if (tx->tse) {
-if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, tx->mss)) {
+uint32_t mss = tx->ctx.mss_l4len_idx >> 16;
+if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, mss)) {
  return false;
  }

@@ -550,8 +551,10 @@ igb_process_tx_desc(IGBCore *core,
 E1000_ADVTXD_DTYP_CTXT) {
  /* advanced transmit context descriptor */
  tx_ctx_desc = (struct e1000_adv_tx_context_desc *)tx_desc;
-tx->vlan = le32_to_cpu(tx_ctx_desc->vlan_macip_lens) >> 16;
-tx->mss = le32_to_cpu(tx_ctx_desc->mss_l4len_idx) >> 16;
+tx->ctx.vlan_macip_lens = 
le32_to_cpu(tx_ctx_desc->vlan_macip_lens);
+tx->ctx.seqnum_seed = le32_to_cpu(tx_ctx_desc->seqnum_seed);
+tx->ctx.type_tucmd_mlhl = le32_to_cpu(tx_ctx_desc-

type_tucmd_mlhl);

+tx->ctx.mss_l4len_idx =
+ le32_to_cpu(tx_ctx_desc->mss_l4len_idx);


Wouldn't it be better to parse the context into all the required fields like 
vlan, mss, etc., already when handling the context descriptor, instead of 
parsing it for every data descriptor later?
Also, in my yet to be merged patch [1] which handles VLAN insertion for VMDq I 
use the vlan field in multiple places, so it would be better to have the vlan 
value readily available.
[1]: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg00393.html


If there is a better representation of the entire context descriptor we 
may use it as an internal use, but I think it is good enough for the 
purpose, too.


For patch [1], I think it is better to gather the logic to derive the 
VID into one place instead of cluttering several places with the 
relevant code. Concretely, igb_tx_insert_vlan() can decide whether to 
read the VID from the context descriptor or from VMIR, or not to read 
(in case VLAN insertion is disabled).


Regards,
Akihiko Odaki




  return;
  } else {
  /* unknown descriptor type */ @@ -575,8 +578,9 @@
igb_process_tx_desc(IGBCore *core,
  if (cmd_type_len & E1000_TXD_CMD_EOP) {
  if (!tx->skip_cp && net_tx_pkt_parse(tx->tx_pkt)) {
  if (cmd_type_len & E1000_TXD_CMD_VLE) {
-net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, tx->vlan,
-core->mac[VET] & 0x);
+uint16_t vlan = tx->ctx.vlan_macip_lens >> 16;
+uint16_t vet = core->mac[VET] & 0x;
+net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, vlan, vet);
  }
  if (igb_tx_pkt_send(core, tx, queue_index)) {
  igb_on_tx_done_update_stats(core, tx->tx_pkt); @@ -4024,8
+4028,7 @@ static void igb_reset(IGBCore *core, bool sw)
  for (i = 0; i < ARRAY_SIZE(core->tx); i++) {
  tx = >tx[i];
  net_tx_pkt_reset(tx->tx_pkt);
-

Re: [PATCH 06/14] qapi: Simplify code a bit after previous commit

[Markus Armbruster]

Eric Blake  writes:

> On Thu, Mar 16, 2023 at 08:13:17AM +0100, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster 
>
> Looks like 'previous commit' in the subject line actually means 4/14
> (two commits ago); a victim of rebasing, I'm sure.

Hmm, actually both commits matter.

The first hunk simplifies check_type_name() by contracting its two
conditionals.  It is enabled by the previous commit, which removed the
code between the two conditionals.

The second hunk simplifies check_type_name_or_array() the same way, but
that one has had nothing in between since PATCH 04.

I'll change the title to "after previous commits".

> Reviewed-by: Eric Blake 

Thanks!

Re: [PATCH 04/14] qapi: Split up check_type()

[Markus Armbruster]

Eric Blake  writes:

> On Thu, Mar 16, 2023 at 08:13:15AM +0100, Markus Armbruster wrote:
>> check_type() can check type names, arrays, and implicit struct types.
>> Callers pass flags to select from this menu.  This makes the function
>> somewhat hard to read.  Moreover, a few minor bugs are hiding in
>> there, as we'll see shortly.
>> 
>> Split it into check_type_name(), check_type_name_or_implicit().  Each
>
> You omitted check_type_name_or_array() in this summary

Oops!

>> of them is a copy of the original specialized to a certain set of
>> flags.
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  scripts/qapi/expr.py | 116 +--
>>  1 file changed, 67 insertions(+), 49 deletions(-)
>
>> 
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index 59bdd86024..bc04bf34c2 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -333,62 +333,74 @@ def normalize_members(members: object) -> None:
>>  members[key] = {'type': arg}
>>  
>>  
>> -def check_type(value: Optional[object],
>> -   info: QAPISourceInfo,
>> -   source: str,
>> -   allow_array: bool = False,
>> -   allow_dict: Union[bool, str] = False) -> None:
>
> There are few enough callers to see that they do indeed have exactly
> one of (nearly) three call patterns.
>
>> -"""
>> -Normalize and validate the QAPI type of ``value``.
>> -
>> -Python types of ``str`` or ``None`` are always allowed.
>> -
>> -:param value: The value to check.
>> -:param info: QAPI schema source file information.
>> -:param source: Error string describing this ``value``.
>> -:param allow_array:
>> -Allow a ``List[str]`` of length 1, which indicates an array of
>> -the type named by the list element.
>> -:param allow_dict:
>> -Allow a dict.  Its members can be struct type members or union
>> -branches.  When the value of ``allow_dict`` is in pragma
>> -``member-name-exceptions``, the dict's keys may violate the
>> -member naming rules.  The dict members are normalized in place.
>> -
>> -:raise QAPISemError: When ``value`` fails validation.
>> -:return: None, ``value`` is normalized in-place as needed.
>> -"""
>> +def check_type_name(value: Optional[object],
>> +info: QAPISourceInfo, source: str) -> None:
>
> check_type_name() replaces callers that relied on the default for
> allow_array and allow_dict

Yes.

>> +if value is None:
>
> Loses out on the documentation.  Not sure how much that matters to
> you?

You mean the doc string?

I could copy and specialize it along with the code, but the new function
is so simple...  not sure it's worth explaining.

>> +return
>> +
>> +if isinstance(value, str):
>> +return
>> +
>> +if isinstance(value, list):
>> +raise QAPISemError(info, "%s cannot be an array" % source)
>> +
>> +raise QAPISemError(info, "%s should be a type name" % source)
>> +
>> +
>> +def check_type_name_or_array(value: Optional[object],
>> + info: QAPISourceInfo, source: str) -> None:
>
> check_type_name_or_array() replaces all callers that passed
> allow_array=True.

Yes.

>>  if value is None:
>
> Another copy without documentation.

Same doubts.

>>  return
>>  
>> -# Type name
>>  if isinstance(value, str):
>>  return
>>  
>> -# Array type
>>  if isinstance(value, list):
>> -if not allow_array:
>> -raise QAPISemError(info, "%s cannot be an array" % source)
>>  if len(value) != 1 or not isinstance(value[0], str):
>>  raise QAPISemError(info,
>> "%s: array type must contain single type 
>> name" %
>> source)
>>  return
>>  
>> -# Anonymous type
>> +raise QAPISemError(info,
>> +   "%s should be a type name" % source)
>>  
>> -if not allow_dict:
>> -raise QAPISemError(info, "%s should be a type name" % source)
>> +
>> +def check_type_name_or_implicit(value: Optional[object],
>> +info: QAPISourceInfo, source: str,
>> +parent_name: Optional[str]) -> None:
>
> And check_type_name_or_implicit replaces all callers that passed
> allow_dict=str, where str is now the parent_name.

Yes.

>(Wow, that was an
> odd overload of the parameter name - I like the split version better).

It was less bad than what it replaced :)

Commit 638c4af9310 (qapi: Clean up member name case checking)

> ...
>> @@ -560,10 +572,13 @@ def check_command(expr: QAPIExpression) -> None:
>>  rets = expr.get('returns')
>>  boxed = expr.get('boxed', False)
>>  
>> -if boxed and args is None:
>> -raise QAPISemError(expr.info, "'boxed': true requires 'data'")
>> -check_type(args, 

RE: [PATCH v5 04/16] hw/9pfs: Implement Windows specific xxxdir() APIs

[Shi, Guohuai]

> -Original Message-
> From: Shi, Guohuai
> Sent: Friday, March 17, 2023 01:28
> To: Christian Schoenebeck ; Greg Kurz
> ; qemu-devel@nongnu.org
> Cc: Meng, Bin 
> Subject: RE: [PATCH v5 04/16] hw/9pfs: Implement Windows specific xxxdir()
> APIs
> 
> 
> 
> > -Original Message-
> > From: Christian Schoenebeck 
> > Sent: Thursday, March 16, 2023 19:05
> > To: Greg Kurz ; qemu-devel@nongnu.org
> > Cc: Meng, Bin ; Shi, Guohuai
> > 
> > Subject: Re: [PATCH v5 04/16] hw/9pfs: Implement Windows specific
> > xxxdir() APIs
> >
> > CAUTION: This email comes from a non Wind River email account!
> > Do not click links or open attachments unless you recognize the sender
> > and know the content is safe.
> >
> > On Wednesday, March 15, 2023 8:05:34 PM CET Shi, Guohuai wrote:
> > >
> > > > -Original Message-
> > > > From: Christian Schoenebeck 
> > > > Sent: Wednesday, March 15, 2023 00:06
> > > > To: Greg Kurz ; qemu-devel@nongnu.org
> > > > Cc: Shi, Guohuai ; Meng, Bin
> > > > 
> > > > Subject: Re: [PATCH v5 04/16] hw/9pfs: Implement Windows specific
> > > > xxxdir() APIs
> > > >
> > > > CAUTION: This email comes from a non Wind River email account!
> > > > Do not click links or open attachments unless you recognize the
> > > > sender and know the content is safe.
> > > >
> > > > On Monday, February 20, 2023 11:08:03 AM CET Bin Meng wrote:
> > > > > From: Guohuai Shi 
> > > > >
> > > > > This commit implements Windows specific xxxdir() APIs for safety
> > > > > directory access.
> > > >
> > > > That comment is seriously too short for this patch.
> > > >
> > > > 1. You should describe the behaviour implementation that you have
> > > > chosen and why you have chosen it.
> > > >
> > > > 2. Like already said in the previous version of the patch, you
> > > > should place a link to the discussion we had on this issue.
> > > >
> > > > > Signed-off-by: Guohuai Shi 
> > > > > Signed-off-by: Bin Meng 
> > > > > ---
> > > > >
> > > > >  hw/9pfs/9p-util.h   |   6 +
> > > > >  hw/9pfs/9p-util-win32.c | 443
> > > > > 
> > > > >  2 files changed, 449 insertions(+)
> > > > >
> > > > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h index
> > > > > 0f159fb4ce..c1c251fbd1 100644
> > > > > --- a/hw/9pfs/9p-util.h
> > > > > +++ b/hw/9pfs/9p-util.h
> > > > > @@ -141,6 +141,12 @@ int unlinkat_win32(int dirfd, const char
> > > > > *pathname, int flags);  int statfs_win32(const char *root_path,
> > > > > struct statfs *stbuf);  int openat_dir(int dirfd, const char
> > > > > *name);  int openat_file(int dirfd, const char *name, int flags,
> > > > > mode_t mode);
> > > > > +DIR *opendir_win32(const char *full_file_name); int
> > > > > +closedir_win32(DIR *pDir); struct dirent *readdir_win32(DIR
> > > > > +*pDir); void rewinddir_win32(DIR *pDir); void seekdir_win32(DIR
> > > > > +*pDir, long pos); long telldir_win32(DIR *pDir);
> > > > >  #endif
> > > > >
> > > > >  static inline void close_preserve_errno(int fd) diff --git
> > > > > a/hw/9pfs/9p-util-win32.c b/hw/9pfs/9p-util-win32.c index
> > > > > a99d579a06..e9408f3c45 100644
> > > > > --- a/hw/9pfs/9p-util-win32.c
> > > > > +++ b/hw/9pfs/9p-util-win32.c
> > > > > @@ -37,6 +37,16 @@
> > > > >   *Windows does not support opendir, the directory fd is created 
> > > > > by
> > > > >   *CreateFile and convert to fd by _open_osfhandle(). Keep the fd
> > open
> > > > will
> > > > >   *lock and protect the directory (can not be modified or 
> > > > > replaced)
> > > > > + *
> > > > > + * 5. Neither Windows native APIs, nor MinGW provide a POSIX
> > > > > + compatible
> > > > API for
> > > > > + *acquiring directory entries in a safe way. Calling those APIs
> > > > (native
> > > > > + *_findfirst() and _findnext() or MinGW's readdir(), seekdir() 
> > > > > and
> > > > > + *telldir()) directly can lead to an inconsistent state if
> > directory
> > > > is
> > > > > + *modified in between, e.g. the same directory appearing more
> than
> > > > once
> > > > > + *in output, or directories not appearing at all in output even
> > though
> > > > they
> > > > > + *were neither newly created nor deleted. POSIX does not define
> > what
> > > > happens
> > > > > + *with deleted or newly created directories in between, but it
> > > > guarantees a
> > > > > + *consistent state.
> > > > >   */
> > > > >
> > > > >  #include "qemu/osdep.h"
> > > > > @@ -51,6 +61,25 @@
> > > > >
> > > > >  #define V9FS_MAGIC  0x53465039  /* string "9PFS" */
> > > > >
> > > > > +/*
> > > > > + * MinGW and Windows does not provide a safe way to seek
> > > > > +directory while other
> > > > > + * thread is modifying the same directory.
> > > > > + *
> > > > > + * This structure is used to store sorted file id and ensure
> > > > > +directory seek
> > > > > + * consistency.
> > > > > + */
> > > > > +struct dir_win32 {
> > > > > +struct dirent dd_dir;
> > > > > +uint32_t offset;
> > > > > +uint32_t 

Re: [PATCH v7 0/4] Add zoned storage emulation to virtio-blk driver

[Sam Li]

Stefan Hajnoczi  于2023年3月17日周五 03:46写道：
>
> On Fri, Mar 10, 2023 at 06:54:27PM +0800, Sam Li wrote:
> > This patch adds zoned storage emulation to the virtio-blk driver.
> >
> > The patch implements the virtio-blk ZBD support standardization that is
> > recently accepted by virtio-spec. The link to related commit is at
> >
> > https://github.com/oasis-tcs/virtio-spec/commit/b4e8efa0fa6c8d844328090ad15db65af8d7d981
> >
> > The Linux zoned device code that implemented by Dmitry Fomichev has been
> > released at the latest Linux version v6.3-rc1.
> >
> > Aside: adding zoned=on alike options to virtio-blk device will be
> > considered as following-ups in future.
> >
> > v6:
> > - update headers to v6.3-rc1
>
> Hi Sam,
> I had some minor comments but overall this looks good. Looking forward
> to merging it soon!

That's great to hear. I'll address them in the next revision. Please
let me know if any further issues arise.

Thanks,
Sam

Re: [PATCH for-8.1 v2 25/26] target/riscv: rework write_misa()

[liweiwei]

On 2023/3/16 04:37, Daniel Henrique Barboza wrote:



On 3/15/23 02:25, liweiwei wrote:


On 2023/3/15 00:49, Daniel Henrique Barboza wrote:

write_misa() must use as much common logic as possible. We want to open
code just the bits that are exclusive to the CSR write operation and 
TCG

internals.

Rewrite write_misa() to work as follows:

- supress RVC right after verifying that we're not updating RVG;

- mask the write using misa_ext_mask to avoid enabling unsupported
   extensions;

- emulate the steps done by realize(): validate the candidate misa_ext
   val, then validate the configuration with the candidate misa_ext 
val,

   and finally commit the changes to cpu->cfg.

If any of the validation steps fails simply ignore the write operation.

Let's keep write_misa() as experimental for now until this logic gains
enough mileage.

Signed-off-by: Daniel Henrique Barboza 
---
  target/riscv/cpu.c | 12 +---
  target/riscv/cpu.h |  6 ++
  target/riscv/csr.c | 47 
+-

  3 files changed, 32 insertions(+), 33 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 5bd92e1cda..4789a7b70d 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1027,9 +1027,8 @@ static void 
riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)

  }
-static void riscv_cpu_validate_misa_ext(CPURISCVState *env,
-    uint32_t misa_ext,
-    Error **errp)
+void riscv_cpu_validate_misa_ext(CPURISCVState *env, uint32_t 
misa_ext,

+ Error **errp)
  {
  Error *local_err = NULL;
@@ -1134,9 +1133,8 @@ static void 
riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)

   * candidate misa_ext value. No changes in env->misa_ext
   * are made.
   */
-static void riscv_cpu_validate_extensions(RISCVCPU *cpu,
-  uint32_t misa_ext,
-  Error **errp)
+void riscv_cpu_validate_extensions(RISCVCPU *cpu, uint32_t misa_ext,
+   Error **errp)
  {
  if (cpu->cfg.epmp && !cpu->cfg.pmp) {
  /*
@@ -1227,7 +1225,7 @@ static void 
riscv_cpu_validate_extensions(RISCVCPU *cpu,

  }
  }
-static void riscv_cpu_commit_cpu_cfg(RISCVCPU *cpu)
+void riscv_cpu_commit_cpu_cfg(RISCVCPU *cpu)
  {
  if (cpu->cfg.ext_zk) {
  cpu->cfg.ext_zkn = true;
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index dbb4df9df0..ca2ba6a647 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -593,6 +593,12 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr 
address, int size,

  char *riscv_isa_string(RISCVCPU *cpu);
  void riscv_cpu_list(void);
+void riscv_cpu_validate_misa_ext(CPURISCVState *env, uint32_t 
misa_ext,

+ Error **errp);
+void riscv_cpu_validate_extensions(RISCVCPU *cpu, uint32_t misa_ext,
+   Error **errp);
+void riscv_cpu_commit_cpu_cfg(RISCVCPU *cpu);
+
  #define cpu_list riscv_cpu_list
  #define cpu_mmu_index riscv_cpu_mmu_index
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 918d442ebd..6f26e7dbcd 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1343,6 +1343,9 @@ static RISCVException read_misa(CPURISCVState 
*env, int csrno,

  static RISCVException write_misa(CPURISCVState *env, int csrno,
   target_ulong val)
  {
+    RISCVCPU *cpu = env_archcpu(env);
+    Error *local_err = NULL;
+
  if (!riscv_cpu_cfg(env)->misa_w) {
  /* drop write to misa */
  return RISCV_EXCP_NONE;
@@ -1353,47 +1356,39 @@ static RISCVException 
write_misa(CPURISCVState *env, int csrno,

  return RISCV_EXCP_NONE;
  }
-    /* 'I' or 'E' must be present */
-    if (!(val & (RVI | RVE))) {
-    /* It is not, drop write to misa */
-    return RISCV_EXCP_NONE;
-    }
-
-    /* 'E' excludes all other extensions */
-    if (val & RVE) {
-    /*
- * when we support 'E' we can do "val = RVE;" however
- * for now we just drop writes if 'E' is present.
- */
-    return RISCV_EXCP_NONE;
-    }
-
  /*
- * misa.MXL writes are not supported by QEMU.
- * Drop writes to those bits.
+ * Suppress 'C' if next instruction is not aligned
+ * TODO: this should check next_pc
   */
+    if ((val & RVC) && (GETPC() & ~3) != 0) {
+    val &= ~RVC;
+    }
  /* Mask extensions that are not supported by this hart */
  val &= env->misa_ext_mask;
-    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
-    if ((val & RVD) && !(val & RVF)) {
-    val &= ~RVD;
+    /* If nothing changed, do nothing. */
+    if (val == env->misa_ext) {
+    return RISCV_EXCP_NONE;
  }
  /*
- * Suppress 'C' if next instruction is not aligned
- * TODO: this should check next_pc
+ * This flow is similar to what riscv_cpu_realize() does,
+ * with the 

Question about TCG liveness_pass_1

[LIU Zhiwei]

Hi Richard,

When I read the tcg code, I find a corner case which may be a bug in 
liveness_pass_1.


I see all TEMP_TBs or global temps are set to TS_DEAD | TS_MEM when 
enter liveness_pass_1. Think about the  sequence.



1)Write_global_temp_0 // 0->TS_DEAD, but not recorded in arg_life

2)INDEX_op_qemu_st   //trigger an exception here.

3)Ref_global_temp_0   // TS_DEAD->0

4)Write_global_temp_0 // TS_DEAD | TS_MEM -> TS_DEAD

As 1) will not write to memory, its register will be reused by the 3).  
I think it may miss a write to global_temp_0 when enter an exception.



Best Regards,
Zhiwei

Re: [PATCH 14/14] qapi: Require boxed for conditional command and event arguments

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:25AM +0100, Markus Armbruster wrote:
> The C code generator fails to honor 'if' conditions of command and
> event arguments.
>
...
> 
> Conditional arguments work fine with 'boxed': true, simply because
> complex types with conditional members work fine.  Not worth breaking.
> 
> Reject conditional arguments unless boxed.

Yay - matches my earlier suggestion at how to avoid #if in the middle
of a parameter list.

> 
> Move the tests cases covering unboxed conditional arguments out of
> tests/qapi-schema/qapi-schema-test.json.  Cover boxed conditional
> arguments there instead.
> 
> Signed-off-by: Markus Armbruster 
> ---

A big end to the series, but I'm glad we got here.

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 13/14] qapi: Fix code generated for optional conditional struct member

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:24AM +0100, Markus Armbruster wrote:
> The generated member visit neglects to emit #if around a conditional
> struct member's has_ variable.  For instance,
> tests/qapi-schema/qapi-schema-test.json generates
> 
> #if defined(TEST_IF_STRUCT)
> bool visit_type_TestIfStruct_members(Visitor *v, TestIfStruct *obj, Error 
> **errp)
> {
> --->  bool has_baz = !!obj->baz;
> 
...
> 
> Won't compile when TEST_IF_STRUCT is defined and TEST_IF_STRUCT_MEMBER
> isn't.
> 
> Fix that the obvious way:
> 
> #if defined(TEST_IF_STRUCT_MEMBER)
>   bool has_baz = !!obj->baz;
> #endif /* defined(TEST_IF_STRUCT_MEMBER) */
> 
> Fixes: 44ea9d9be33c (qapi: Start to elide redundant has_FOO in generated C)
> Signed-off-by: Marc-André Lureau 
> Signed-off-by: Markus Armbruster 
> ---
>  scripts/qapi/visit.py | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 11/14] tests/qapi-schema: Clean up positive test for conditionals

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:22AM +0100, Markus Armbruster wrote:
> Union TestIfUnion is conditional on macros TEST_IF_UNION and
> TEST_IF_STRUCT.  It uses TestIfEnum, which is conditional on macro
> TEST_IF_ENUM.  If TEST_IF_ENUM and TEST_IF_STRUCT are defined, but
> TEST_IF_ENUM isn't, the generated code won't compile.

s/ENUM/UNION/ in one of these two uses in this sentence.

> 
> Command test-if-cmd is conditional an macros TEST_IF_CMD and
> TEST_IF_STRUCT, and uses TestIfEnum.  Similar issue.
> 
> Event TEST_IF_EVENT is conditional an macros TEST_IF_EVT and
> TEST_IF_STRUCT, and uses TestIfEnum.  Similar issue.
> 
> Replace the uses of TestIfEnum in the latter two by str.
> 
> TestIfUnion is now TestIfEnum's only user.  Change TestIfEnum's
> condition to TEST_IF_UNION.

Fair enough, once the commit message doesn't confuse me in the first
paragraph ;)

> 
> Signed-off-by: Markus Armbruster 
> ---
>  tests/qapi-schema/qapi-schema-test.json | 6 +++---
>  tests/qapi-schema/qapi-schema-test.out  | 8 +++-
>  2 files changed, 6 insertions(+), 8 deletions(-)
>

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 09/14] tests/qapi-schema: Improve union discriminator coverage

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:20AM +0100, Markus Armbruster wrote:
> A union's 'discriminator' must name a one of the common members.

s/ a//

> QAPISchemaVariants.check() looks it up by its c_name(), then checks
> the name matches exactly (because c_name() is not injective).
> 
> Tests union-base-empty and union-invalid-discriminator both cover the
> case where lookup fails.  Repurpose the latter to cover the case where
> it succeeds and the name check fails.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  tests/qapi-schema/union-invalid-discriminator.err  | 2 +-
>  tests/qapi-schema/union-invalid-discriminator.json | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake 

(- vs. _ is subtle, especially since I purposefully case-map them to
one another whenever I can...)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 08/14] qapi: Fix to reject 'data': 'mumble' in struct

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:19AM +0100, Markus Armbruster wrote:
> A struct's 'data' must be a JSON object defining the struct's members.
> The QAPI code generator incorrectly accepts a JSON string instead, and
> then crashes in QAPISchema._make_members() called from
> ._def_struct_type().
> 
> Fix to reject it, and add a test case.

Nice catch; I see why the split into three functions earlier on
foreshadowed some subtle bug fixes to come.

> +++ b/scripts/qapi/expr.py
> @@ -354,14 +354,14 @@ def check_type_name_or_array(value: Optional[object],
> source)
>  
>  
> -def check_type_name_or_implicit(value: Optional[object],
> -info: QAPISourceInfo, source: str,
> -parent_name: Optional[str]) -> None:
> +def check_type_implicit(value: Optional[object],
> +info: QAPISourceInfo, source: str,
> +parent_name: Optional[str]) -> None:

At first I thought this was a straight rename...

>  """
>  Normalize and validate an optional implicit struct type.
>  
> -Accept ``None``, ``str``, or a ``dict`` defining an implicit
> -struct type.  The latter is normalized in place.
> +Accept ``None`` or a ``dict`` defining an implicit struct type.
> +The latter is normalized in place.
>  
>  :param value: The value to check.
>  :param info: QAPI schema source file information.
> @@ -377,9 +377,6 @@ def check_type_name_or_implicit(value: Optional[object],
>  if value is None:
>  return
>  
> -if isinstance(value, str):
> -return
> -
>  if not isinstance(value, dict):
>  raise QAPISemError(info,
> "%s should be an object or type name" % source)
> @@ -401,6 +398,15 @@ def check_type_name_or_implicit(value: Optional[object],
>  check_type_name_or_array(arg['type'], info, key_source)
>  
>  
> +def check_type_name_or_implicit(value: Optional[object],
> +info: QAPISourceInfo, source: str,
> +parent_name: Optional[str]) -> None:
> +if value is None or isinstance(value, str):

...until I got here and saw that you kept the original name, and added
a new helper.  Worth calling out the new function name
check_type_implicit() in the commit message?  It would have spared me
a minute.

As earlier, you lost the doc comment.  I'll leave it to your
discretion if it is important to copy one back in.

> +return
> +
> +check_type_implicit(value, info, source, parent_name)
> +
> +
>  def check_features(features: Optional[object],
> info: QAPISourceInfo) -> None:
>  """
> @@ -486,7 +492,7 @@ def check_struct(expr: QAPIExpression) -> None:
>  name = cast(str, expr['struct'])  # Checked in check_exprs
>  members = expr['data']
>  
> -check_type_name_or_implicit(members, expr.info, "'data'", name)
> +check_type_implicit(members, expr.info, "'data'", name)
>  check_type_name(expr.get('base'), expr.info, "'base'")
>

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 07/14] qapi: Fix error message when type name or array is expected

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:18AM +0100, Markus Armbruster wrote:
> We incorrectly report "FOO should be a type name" when it could also
> be an array.  Fix that.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  scripts/qapi/expr.py| 15 +++
>  tests/qapi-schema/event-nest-struct.err |  2 +-
>  tests/qapi-schema/nested-struct-data.err|  2 +-
>  tests/qapi-schema/returns-dict.err  |  2 +-
>  tests/qapi-schema/struct-member-invalid.err |  2 +-
>  5 files changed, 11 insertions(+), 12 deletions(-)
>

Doesn't change the set of schemas accepted, but does make it easier to
understand when a schema is rejected.

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 06/14] qapi: Simplify code a bit after previous commit

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:17AM +0100, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster 

Looks like 'previous commit' in the subject line actually means 4/14
(two commits ago); a victim of rebasing, I'm sure.

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 04/14] qapi: Split up check_type()

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:15AM +0100, Markus Armbruster wrote:
> check_type() can check type names, arrays, and implicit struct types.
> Callers pass flags to select from this menu.  This makes the function
> somewhat hard to read.  Moreover, a few minor bugs are hiding in
> there, as we'll see shortly.
> 
> Split it into check_type_name(), check_type_name_or_implicit().  Each

You omitted check_type_name_or_array() in this summary

> of them is a copy of the original specialized to a certain set of
> flags.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  scripts/qapi/expr.py | 116 +--
>  1 file changed, 67 insertions(+), 49 deletions(-)

> 
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 59bdd86024..bc04bf34c2 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -333,62 +333,74 @@ def normalize_members(members: object) -> None:
>  members[key] = {'type': arg}
>  
>  
> -def check_type(value: Optional[object],
> -   info: QAPISourceInfo,
> -   source: str,
> -   allow_array: bool = False,
> -   allow_dict: Union[bool, str] = False) -> None:

There are few enough callers to see that they do indeed have exactly
one of (nearly) three call patterns.

> -"""
> -Normalize and validate the QAPI type of ``value``.
> -
> -Python types of ``str`` or ``None`` are always allowed.
> -
> -:param value: The value to check.
> -:param info: QAPI schema source file information.
> -:param source: Error string describing this ``value``.
> -:param allow_array:
> -Allow a ``List[str]`` of length 1, which indicates an array of
> -the type named by the list element.
> -:param allow_dict:
> -Allow a dict.  Its members can be struct type members or union
> -branches.  When the value of ``allow_dict`` is in pragma
> -``member-name-exceptions``, the dict's keys may violate the
> -member naming rules.  The dict members are normalized in place.
> -
> -:raise QAPISemError: When ``value`` fails validation.
> -:return: None, ``value`` is normalized in-place as needed.
> -"""
> +def check_type_name(value: Optional[object],
> +info: QAPISourceInfo, source: str) -> None:

check_type_name() replaces callers that relied on the default for
allow_array and allow_dict

> +if value is None:

Loses out on the documentation.  Not sure how much that matters to
you?

> +return
> +
> +if isinstance(value, str):
> +return
> +
> +if isinstance(value, list):
> +raise QAPISemError(info, "%s cannot be an array" % source)
> +
> +raise QAPISemError(info, "%s should be a type name" % source)
> +
> +
> +def check_type_name_or_array(value: Optional[object],
> + info: QAPISourceInfo, source: str) -> None:

check_type_name_or_array() replaces all callers that passed
allow_array=True.

>  if value is None:

Another copy without documentation.

>  return
>  
> -# Type name
>  if isinstance(value, str):
>  return
>  
> -# Array type
>  if isinstance(value, list):
> -if not allow_array:
> -raise QAPISemError(info, "%s cannot be an array" % source)
>  if len(value) != 1 or not isinstance(value[0], str):
>  raise QAPISemError(info,
> "%s: array type must contain single type 
> name" %
> source)
>  return
>  
> -# Anonymous type
> +raise QAPISemError(info,
> +   "%s should be a type name" % source)
>  
> -if not allow_dict:
> -raise QAPISemError(info, "%s should be a type name" % source)
> +
> +def check_type_name_or_implicit(value: Optional[object],
> +info: QAPISourceInfo, source: str,
> +parent_name: Optional[str]) -> None:

And check_type_name_or_implicit replaces all callers that passed
allow_dict=str, where str is now the parent_name.  (Wow, that was an
odd overload of the parameter name - I like the split version better).

...
> @@ -560,10 +572,13 @@ def check_command(expr: QAPIExpression) -> None:
>  rets = expr.get('returns')
>  boxed = expr.get('boxed', False)
>  
> -if boxed and args is None:
> -raise QAPISemError(expr.info, "'boxed': true requires 'data'")
> -check_type(args, expr.info, "'data'", allow_dict=not boxed)
> -check_type(rets, expr.info, "'returns'", allow_array=True)
> +if boxed:
> +if args is None:
> +raise QAPISemError(expr.info, "'boxed': true requires 'data'")
> +check_type_name(args, expr.info, "'data'")
> +else:
> +check_type_name_or_implicit(args, expr.info, "'data'", None)

And this use of allow_dict was the weirdest, where it really does fit
better as calls into two separate functions.

With the fixed 

Re: [PATCH 03/14] qapi: Clean up after removal of simple unions

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:14AM +0100, Markus Armbruster wrote:
> Commit 4e99f4b12c0 (qapi: Drop simple unions) missed a bit of code
> dealing with simple union branches.  Drop it.
> 
> Signed-off-by: Markus Armbruster 
> ---

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[PATCH v2] virtio: refresh vring region cache after updating a virtqueue size

[Carlos López]

When a virtqueue size is changed by the guest via
virtio_queue_set_num(), its region cache is not automatically updated.
If the size was increased, this could lead to accessing the cache out
of bounds. For example, in vring_get_used_event():

static inline uint16_t vring_get_used_event(VirtQueue *vq)
{
return vring_avail_ring(vq, vq->vring.num);
}

static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
{
VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
hwaddr pa = offsetof(VRingAvail, ring[i]);

if (!caches) {
return 0;
}

return virtio_lduw_phys_cached(vq->vdev, >avail, pa);
}

vq->vring.num will be greater than caches->avail.len, which will
trigger a failed assertion down the call path of
virtio_lduw_phys_cached().

Fix this by calling virtio_init_region_cache() after
virtio_queue_set_num() if we are not already calling
virtio_queue_set_rings(). In the legacy path this is already done by
virtio_queue_update_rings().

Signed-off-by: Carlos López 
---
v2: use virtio_init_region_cache() instead of
virtio_queue_update_rings() in the path for modern devices.

 hw/s390x/virtio-ccw.c  | 1 +
 hw/virtio/virtio-mmio.c| 1 +
 hw/virtio/virtio-pci.c | 1 +
 hw/virtio/virtio.c | 2 +-
 include/hw/virtio/virtio.h | 1 +
 5 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index e33e5207ab..f44de1a8c1 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -237,6 +237,7 @@ static int virtio_ccw_set_vqs(SubchDev *sch, VqInfoBlock 
*info,
 return -EINVAL;
 }
 virtio_queue_set_num(vdev, index, num);
+virtio_init_region_cache(vdev, index);
 } else if (virtio_queue_get_num(vdev, index) > num) {
 /* Fail if we don't have a big enough queue. */
 return -EINVAL;
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 23ba625eb6..c2c6d85475 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -354,6 +354,7 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, 
uint64_t value,
 if (proxy->legacy) {
 virtio_queue_update_rings(vdev, vdev->queue_sel);
 } else {
+virtio_init_region_cache(vdev, vdev->queue_sel);
 proxy->vqs[vdev->queue_sel].num = value;
 }
 break;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 247325c193..02fb84a8fa 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1554,6 +1554,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr 
addr,
 proxy->vqs[vdev->queue_sel].num = val;
 virtio_queue_set_num(vdev, vdev->queue_sel,
  proxy->vqs[vdev->queue_sel].num);
+virtio_init_region_cache(vdev, vdev->queue_sel);
 break;
 case VIRTIO_PCI_COMMON_Q_MSIX:
 vector = virtio_queue_vector(vdev, vdev->queue_sel);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 98c4819fcc..272d930721 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -226,7 +226,7 @@ static void virtio_virtqueue_reset_region_cache(struct 
VirtQueue *vq)
 }
 }
 
-static void virtio_init_region_cache(VirtIODevice *vdev, int n)
+void virtio_init_region_cache(VirtIODevice *vdev, int n)
 {
 VirtQueue *vq = >vq[n];
 VRingMemoryRegionCaches *old = vq->vring.caches;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 77c6c55929..fed5fff049 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -309,6 +309,7 @@ int virtio_get_num_queues(VirtIODevice *vdev);
 void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc,
 hwaddr avail, hwaddr used);
 void virtio_queue_update_rings(VirtIODevice *vdev, int n);
+void virtio_init_region_cache(VirtIODevice *vdev, int n);
 void virtio_queue_set_align(VirtIODevice *vdev, int n, int align);
 void virtio_queue_notify(VirtIODevice *vdev, int n);
 uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
-- 
2.35.3

[PATCH v2 4/4] tests/avocado: Add reboot tests to Cubieboard

[Strahinja Jankovic]

Cubieboard tests end with comment "reboot not functioning; omit test".
Fix this so reboot is done at the end of each test.

Signed-off-by: Strahinja Jankovic 

Reviewed-by: Niek Linnenbank 
Tested-by: Niek Linnenbank 
---
 tests/avocado/boot_linux_console.py | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/tests/avocado/boot_linux_console.py 
b/tests/avocado/boot_linux_console.py
index 574609bf43..c0675809e6 100644
--- a/tests/avocado/boot_linux_console.py
+++ b/tests/avocado/boot_linux_console.py
@@ -581,7 +581,10 @@ def test_arm_cubieboard_initrd(self):
 'Allwinner sun4i/sun5i')
 exec_command_and_wait_for_pattern(self, 'cat /proc/iomem',
 'system-control@1c0')
-# cubieboard's reboot is not functioning; omit reboot test.
+exec_command_and_wait_for_pattern(self, 'reboot',
+'reboot: Restarting system')
+# Wait for VM to shut down gracefully
+self.vm.wait()
 
 def test_arm_cubieboard_sata(self):
 """
@@ -625,7 +628,10 @@ def test_arm_cubieboard_sata(self):
 'Allwinner sun4i/sun5i')
 exec_command_and_wait_for_pattern(self, 'cat /proc/partitions',
 'sda')
-# cubieboard's reboot is not functioning; omit reboot test.
+exec_command_and_wait_for_pattern(self, 'reboot',
+'reboot: Restarting system')
+# Wait for VM to shut down gracefully
+self.vm.wait()
 
 @skipUnless(os.getenv('AVOCADO_ALLOW_LARGE_STORAGE'), 'storage limited')
 def test_arm_cubieboard_openwrt_22_03_2(self):
@@ -672,7 +678,10 @@ def test_arm_cubieboard_openwrt_22_03_2(self):
 
 exec_command_and_wait_for_pattern(self, 'cat /proc/cpuinfo',
 'Allwinner sun4i/sun5i')
-# cubieboard's reboot is not functioning; omit reboot test.
+exec_command_and_wait_for_pattern(self, 'reboot',
+'reboot: Restarting system')
+# Wait for VM to shut down gracefully
+self.vm.wait()
 
 @skipUnless(os.getenv('AVOCADO_TIMEOUT_EXPECTED'), 'Test might timeout')
 def test_arm_quanta_gsj(self):
-- 
2.30.2

[PATCH v2 3/4] hw/arm: Add WDT to Allwinner-H3 and Orangepi-PC

[Strahinja Jankovic]

This patch adds WDT to Allwinner-H3 and Orangepi-PC.
WDT is added as an overlay to the Timer module memory area.

Signed-off-by: Strahinja Jankovic 

Reviewed-by: Niek Linnenbank 
---
 docs/system/arm/orangepi.rst  | 1 +
 hw/arm/Kconfig| 1 +
 hw/arm/allwinner-h3.c | 8 
 include/hw/arm/allwinner-h3.h | 5 -
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/docs/system/arm/orangepi.rst b/docs/system/arm/orangepi.rst
index e5973600a1..9afa54213b 100644
--- a/docs/system/arm/orangepi.rst
+++ b/docs/system/arm/orangepi.rst
@@ -26,6 +26,7 @@ The Orange Pi PC machine supports the following devices:
  * System Control module
  * Security Identifier device
  * TWI (I2C)
+ * Watchdog timer
 
 Limitations
 """
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index ec15248536..7d916f5450 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -337,6 +337,7 @@ config ALLWINNER_H3
 select ALLWINNER_A10_PIT
 select ALLWINNER_SUN8I_EMAC
 select ALLWINNER_I2C
+select ALLWINNER_WDT
 select SERIAL
 select ARM_TIMER
 select ARM_GIC
diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
index 69d0ad6f50..f05afddf7e 100644
--- a/hw/arm/allwinner-h3.c
+++ b/hw/arm/allwinner-h3.c
@@ -49,6 +49,7 @@ const hwaddr allwinner_h3_memmap[] = {
 [AW_H3_DEV_OHCI3]  = 0x01c1d400,
 [AW_H3_DEV_CCU]= 0x01c2,
 [AW_H3_DEV_PIT]= 0x01c20c00,
+[AW_H3_DEV_WDT]= 0x01c20ca0,
 [AW_H3_DEV_UART0]  = 0x01c28000,
 [AW_H3_DEV_UART1]  = 0x01c28400,
 [AW_H3_DEV_UART2]  = 0x01c28800,
@@ -234,6 +235,8 @@ static void allwinner_h3_init(Object *obj)
 object_initialize_child(obj, "twi1",  >i2c1,  TYPE_AW_I2C_SUN6I);
 object_initialize_child(obj, "twi2",  >i2c2,  TYPE_AW_I2C_SUN6I);
 object_initialize_child(obj, "r_twi", >r_twi, TYPE_AW_I2C_SUN6I);
+
+object_initialize_child(obj, "wdt", >wdt, TYPE_AW_WDT_SUN6I);
 }
 
 static void allwinner_h3_realize(DeviceState *dev, Error **errp)
@@ -453,6 +456,11 @@ static void allwinner_h3_realize(DeviceState *dev, Error 
**errp)
 sysbus_connect_irq(SYS_BUS_DEVICE(>r_twi), 0,
qdev_get_gpio_in(DEVICE(>gic), AW_H3_GIC_SPI_R_TWI));
 
+/* WDT */
+sysbus_realize(SYS_BUS_DEVICE(>wdt), _fatal);
+sysbus_mmio_map_overlap(SYS_BUS_DEVICE(>wdt), 0,
+s->memmap[AW_H3_DEV_WDT], 1);
+
 /* Unimplemented devices */
 for (i = 0; i < ARRAY_SIZE(unimplemented); i++) {
 create_unimplemented_device(unimplemented[i].device_name,
diff --git a/include/hw/arm/allwinner-h3.h b/include/hw/arm/allwinner-h3.h
index 59e0f822d2..f15d6d7cc7 100644
--- a/include/hw/arm/allwinner-h3.h
+++ b/include/hw/arm/allwinner-h3.h
@@ -48,6 +48,7 @@
 #include "hw/net/allwinner-sun8i-emac.h"
 #include "hw/rtc/allwinner-rtc.h"
 #include "hw/i2c/allwinner-i2c.h"
+#include "hw/watchdog/allwinner-wdt.h"
 #include "target/arm/cpu.h"
 #include "sysemu/block-backend.h"
 
@@ -96,7 +97,8 @@ enum {
 AW_H3_DEV_RTC,
 AW_H3_DEV_CPUCFG,
 AW_H3_DEV_R_TWI,
-AW_H3_DEV_SDRAM
+AW_H3_DEV_SDRAM,
+AW_H3_DEV_WDT
 };
 
 /** Total number of CPU cores in the H3 SoC */
@@ -141,6 +143,7 @@ struct AwH3State {
 AWI2CState r_twi;
 AwSun8iEmacState emac;
 AwRtcState rtc;
+AwWdtState wdt;
 GICState gic;
 MemoryRegion sram_a1;
 MemoryRegion sram_a2;
-- 
2.30.2

[PATCH v2 2/4] hw/arm: Add WDT to Allwinner-A10 and Cubieboard

[Strahinja Jankovic]

This patch adds WDT to Allwinner-A10 and Cubieboard.
WDT is added as an overlay to the Timer module memory map.

Signed-off-by: Strahinja Jankovic 

Reviewed-by: Niek Linnenbank 
---
 docs/system/arm/cubieboard.rst | 1 +
 hw/arm/Kconfig | 1 +
 hw/arm/allwinner-a10.c | 7 +++
 include/hw/arm/allwinner-a10.h | 2 ++
 4 files changed, 11 insertions(+)

diff --git a/docs/system/arm/cubieboard.rst b/docs/system/arm/cubieboard.rst
index 8d485f5435..58c4a2d3ea 100644
--- a/docs/system/arm/cubieboard.rst
+++ b/docs/system/arm/cubieboard.rst
@@ -15,3 +15,4 @@ Emulated devices:
 - USB controller
 - SATA controller
 - TWI (I2C) controller
+- Watchdog timer
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index b5aed4aff5..ec15248536 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -325,6 +325,7 @@ config ALLWINNER_A10
 select ALLWINNER_A10_PIC
 select ALLWINNER_A10_CCM
 select ALLWINNER_A10_DRAMC
+select ALLWINNER_WDT
 select ALLWINNER_EMAC
 select ALLWINNER_I2C
 select AXP209_PMU
diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
index b7ca795c71..b0ea3f7f66 100644
--- a/hw/arm/allwinner-a10.c
+++ b/hw/arm/allwinner-a10.c
@@ -38,6 +38,7 @@
 #define AW_A10_EHCI_BASE0x01c14000
 #define AW_A10_OHCI_BASE0x01c14400
 #define AW_A10_SATA_BASE0x01c18000
+#define AW_A10_WDT_BASE 0x01c20c90
 #define AW_A10_RTC_BASE 0x01c20d00
 #define AW_A10_I2C0_BASE0x01c2ac00
 
@@ -92,6 +93,8 @@ static void aw_a10_init(Object *obj)
 object_initialize_child(obj, "mmc0", >mmc0, TYPE_AW_SDHOST_SUN4I);
 
 object_initialize_child(obj, "rtc", >rtc, TYPE_AW_RTC_SUN4I);
+
+object_initialize_child(obj, "wdt", >wdt, TYPE_AW_WDT_SUN4I);
 }
 
 static void aw_a10_realize(DeviceState *dev, Error **errp)
@@ -203,6 +206,10 @@ static void aw_a10_realize(DeviceState *dev, Error **errp)
 sysbus_realize(SYS_BUS_DEVICE(>i2c0), _fatal);
 sysbus_mmio_map(SYS_BUS_DEVICE(>i2c0), 0, AW_A10_I2C0_BASE);
 sysbus_connect_irq(SYS_BUS_DEVICE(>i2c0), 0, qdev_get_gpio_in(dev, 7));
+
+/* WDT */
+sysbus_realize(SYS_BUS_DEVICE(>wdt), _fatal);
+sysbus_mmio_map_overlap(SYS_BUS_DEVICE(>wdt), 0, AW_A10_WDT_BASE, 1);
 }
 
 static void aw_a10_class_init(ObjectClass *oc, void *data)
diff --git a/include/hw/arm/allwinner-a10.h b/include/hw/arm/allwinner-a10.h
index 095afb225d..cd1465c613 100644
--- a/include/hw/arm/allwinner-a10.h
+++ b/include/hw/arm/allwinner-a10.h
@@ -13,6 +13,7 @@
 #include "hw/misc/allwinner-a10-ccm.h"
 #include "hw/misc/allwinner-a10-dramc.h"
 #include "hw/i2c/allwinner-i2c.h"
+#include "hw/watchdog/allwinner-wdt.h"
 #include "sysemu/block-backend.h"
 
 #include "target/arm/cpu.h"
@@ -41,6 +42,7 @@ struct AwA10State {
 AwSdHostState mmc0;
 AWI2CState i2c0;
 AwRtcState rtc;
+AwWdtState wdt;
 MemoryRegion sram_a;
 EHCISysBusState ehci[AW_A10_NUM_USB];
 OHCISysBusState ohci[AW_A10_NUM_USB];
-- 
2.30.2

[PATCH v2 1/4] hw/watchdog: Allwinner WDT emulation for system reset

[Strahinja Jankovic]

This patch adds basic support for Allwinner WDT.
Both sun4i and sun6i variants are supported.
However, interrupt generation is not supported, so WDT can be used only to 
trigger system reset.

Signed-off-by: Strahinja Jankovic 

---
 hw/watchdog/Kconfig |   4 +
 hw/watchdog/allwinner-wdt.c | 416 
 hw/watchdog/meson.build |   1 +
 hw/watchdog/trace-events|   7 +
 include/hw/watchdog/allwinner-wdt.h | 123 
 5 files changed, 551 insertions(+)
 create mode 100644 hw/watchdog/allwinner-wdt.c
 create mode 100644 include/hw/watchdog/allwinner-wdt.h

diff --git a/hw/watchdog/Kconfig b/hw/watchdog/Kconfig
index 66e1d029e3..861fd00334 100644
--- a/hw/watchdog/Kconfig
+++ b/hw/watchdog/Kconfig
@@ -20,3 +20,7 @@ config WDT_IMX2
 
 config WDT_SBSA
 bool
+
+config ALLWINNER_WDT
+bool
+select PTIMER
diff --git a/hw/watchdog/allwinner-wdt.c b/hw/watchdog/allwinner-wdt.c
new file mode 100644
index 00..45a4a36ba7
--- /dev/null
+++ b/hw/watchdog/allwinner-wdt.c
@@ -0,0 +1,416 @@
+/*
+ * Allwinner Watchdog emulation
+ *
+ * Copyright (C) 2023 Strahinja Jankovic 
+ *
+ *  This file is derived from Allwinner RTC,
+ *  by Niek Linnenbank.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/units.h"
+#include "qemu/module.h"
+#include "trace.h"
+#include "hw/sysbus.h"
+#include "hw/registerfields.h"
+#include "hw/watchdog/allwinner-wdt.h"
+#include "sysemu/watchdog.h"
+#include "migration/vmstate.h"
+
+/* WDT registers */
+enum {
+REG_IRQ_EN = 0, /* Watchdog interrupt enable */
+REG_IRQ_STA,/* Watchdog interrupt status */
+REG_CTRL,   /* Watchdog control register */
+REG_CFG,/* Watchdog configuration register */
+REG_MODE,   /* Watchdog mode register */
+};
+
+/* Universal WDT register flags */
+#define WDT_RESTART_MASK(1 << 0)
+#define WDT_EN_MASK (1 << 0)
+
+/* sun4i specific WDT register flags */
+#define RST_EN_SUN4I_MASK   (1 << 1)
+#define INTV_VALUE_SUN4I_SHIFT  (3)
+#define INTV_VALUE_SUN4I_MASK   (0xfu << INTV_VALUE_SUN4I_SHIFT)
+
+/* sun6i specific WDT register flags */
+#define RST_EN_SUN6I_MASK   (1 << 0)
+#define KEY_FIELD_SUN6I_SHIFT   (1)
+#define KEY_FIELD_SUN6I_MASK(0xfffu << KEY_FIELD_SUN6I_SHIFT)
+#define KEY_FIELD_SUN6I (0xA57u)
+#define INTV_VALUE_SUN6I_SHIFT  (4)
+#define INTV_VALUE_SUN6I_MASK   (0xfu << INTV_VALUE_SUN6I_SHIFT)
+
+/* Map of INTV_VALUE to 0.5s units. */
+static const uint8_t allwinner_wdt_count_map[] = {
+1,
+2,
+4,
+6,
+8,
+10,
+12,
+16,
+20,
+24,
+28,
+32
+};
+
+/* WDT sun4i register map (offset to name) */
+const uint8_t allwinner_wdt_sun4i_regmap[] = {
+[0x] = REG_CTRL,
+[0x0004] = REG_MODE,
+};
+
+/* WDT sun6i register map (offset to name) */
+const uint8_t allwinner_wdt_sun6i_regmap[] = {
+[0x] = REG_IRQ_EN,
+[0x0004] = REG_IRQ_STA,
+[0x0010] = REG_CTRL,
+[0x0014] = REG_CFG,
+[0x0018] = REG_MODE,
+};
+
+static bool allwinner_wdt_sun4i_read(AwWdtState *s, uint32_t offset)
+{
+/* no sun4i specific registers currently implemented */
+return false;
+}
+
+static bool allwinner_wdt_sun4i_write(AwWdtState *s, uint32_t offset,
+  uint32_t data)
+{
+/* no sun4i specific registers currently implemented */
+return false;
+}
+
+static bool allwinner_wdt_sun4i_can_reset_system(AwWdtState *s)
+{
+if (s->regs[REG_MODE] & RST_EN_SUN6I_MASK) {
+return true;
+} else {
+return false;
+}
+}
+
+static bool allwinner_wdt_sun4i_is_key_valid(AwWdtState *s, uint32_t val)
+{
+/* sun4i has no key */
+return true;
+}
+
+static uint8_t allwinner_wdt_sun4i_get_intv_value(AwWdtState *s)
+{
+return ((s->regs[REG_MODE] & INTV_VALUE_SUN4I_MASK) >>
+INTV_VALUE_SUN4I_SHIFT);
+}
+
+static bool allwinner_wdt_sun6i_read(AwWdtState *s, uint32_t offset)
+{
+const AwWdtClass *c = AW_WDT_GET_CLASS(s);
+
+switch (c->regmap[offset]) {
+case REG_IRQ_EN:
+case REG_IRQ_STA:
+case REG_CFG:
+return true;
+default:
+break;
+}
+return false;
+}
+
+static bool allwinner_wdt_sun6i_write(AwWdtState *s, uint32_t offset,
+ 

[PATCH v2 0/4] Basic Allwinner WDT emulation

[Strahinja Jankovic]

This patch set introduces basic emulation of Allwinner WDT.
Since WDT in both A10 and H3 is part of Timer module, the WDT
functionality is added as an overlay in the memory map.

The focus was to enable reboot functionality, so WDT interrupt handling
is not covered in this patch set.

With these patches the `reboot` command can be used for both Cubieboard
and Orangepi-PC in order to restart the system.

Also, Cubieboard avocado tests have been improved to include reboot
steps as well.

v2:
- Cleaned up WDT implementation (changes only in patch 01/04)
- Removed unnecessary checks - instead of changing enum to start from 1,
  removed if (!c->regmap[offset]) since it was conflicting enum values
- Reorganized comments

Strahinja Jankovic (4):
  hw/watchdog: Allwinner WDT emulation for system reset
  hw/arm: Add WDT to Allwinner-A10 and Cubieboard
  hw/arm: Add WDT to Allwinner-H3 and Orangepi-PC
  tests/avocado: Add reboot tests to Cubieboard

 docs/system/arm/cubieboard.rst  |   1 +
 docs/system/arm/orangepi.rst|   1 +
 hw/arm/Kconfig  |   2 +
 hw/arm/allwinner-a10.c  |   7 +
 hw/arm/allwinner-h3.c   |   8 +
 hw/watchdog/Kconfig |   4 +
 hw/watchdog/allwinner-wdt.c | 416 
 hw/watchdog/meson.build |   1 +
 hw/watchdog/trace-events|   7 +
 include/hw/arm/allwinner-a10.h  |   2 +
 include/hw/arm/allwinner-h3.h   |   5 +-
 include/hw/watchdog/allwinner-wdt.h | 123 
 tests/avocado/boot_linux_console.py |  15 +-
 13 files changed, 588 insertions(+), 4 deletions(-)
 create mode 100644 hw/watchdog/allwinner-wdt.c
 create mode 100644 include/hw/watchdog/allwinner-wdt.h

-- 
2.30.2

[PATCH] hw/usb/imx: Fix out of bounds access in imx_usbphy_read()

[Guenter Roeck]

The i.MX USB Phy driver does not check register ranges, resulting in out of
bounds accesses if an attempt is made to access non-existing PHY registers.
Add range check and conditionally report bad accesses to fix the problem.

While at it, also conditionally log attempted writes to non-existing or
read-only registers.

Reported-by: Qiang Liu 
Link: https://gitlab.com/qemu-project/qemu/-/issues/1408
Fixes: 0701a5efa015 ("hw/usb: Add basic i.MX USB Phy support")
Signed-off-by: Guenter Roeck 
---
 hw/usb/imx-usb-phy.c | 19 +--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/hw/usb/imx-usb-phy.c b/hw/usb/imx-usb-phy.c
index 5d7a549e34..1a97b36a11 100644
--- a/hw/usb/imx-usb-phy.c
+++ b/hw/usb/imx-usb-phy.c
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 #include "hw/usb/imx-usb-phy.h"
 #include "migration/vmstate.h"
+#include "qemu/log.h"
 #include "qemu/module.h"
 
 static const VMStateDescription vmstate_imx_usbphy = {
@@ -90,7 +91,15 @@ static uint64_t imx_usbphy_read(void *opaque, hwaddr offset, 
unsigned size)
 value = s->usbphy[index - 3];
 break;
 default:
-value = s->usbphy[index];
+if (index < USBPHY_MAX) {
+value = s->usbphy[index];
+} else {
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: Read from non-existing USB PHY register 0x%"
+  HWADDR_PRIx "\n",
+  __func__, offset);
+value = 0;
+}
 break;
 }
 return (uint64_t)value;
@@ -168,7 +177,13 @@ static void imx_usbphy_write(void *opaque, hwaddr offset, 
uint64_t value,
 s->usbphy[index - 3] ^= value;
 break;
 default:
-/* Other registers are read-only */
+/* Other registers are read-only or do not exist */
+qemu_log_mask(LOG_GUEST_ERROR,
+  "%s: Write to %s USB PHY register 0x%"
+  HWADDR_PRIx "\n",
+  __func__,
+  index >= USBPHY_MAX ? "non-existing" : "read-only",
+  offset);
 break;
 }
 }
-- 
2.39.2

[PATCH docs_and_memory_encryption 1/1] initial commit

[~titilola]

From: Titilola 

---
 README.md | Bin 0 -> 30 bytes
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 README.md

diff --git a/README.md b/README.md
new file mode 100644
index 
..1f2ce0dcfc24c1ca3291260835280a1b6ac24e37
GIT binary patch
literal 30
hcmezW`Gfn*Voox-5Vz{|kJ004%3266xZ

literal 0
HcmV?d1

-- 
2.34.7

[PATCH docs_and_memory_encryption 0/1] An attempt to convert txt file to rst

[~titilola]

==
PULL 16/26
==

:Subject: docs: add firmware feature flags
:Author: Gerd Hoffmann 
:Date: 2022-10-13 6:52 UTC

Description
===

This patch adds information about firmware feature flags.

Changes
===

- Add information about firmware feature flags to documentation.

Doc Updates
===

- Update documentation with information about firmware feature flags.

File Changes


- No file changes.

==
PATCH v2
==

:Subject: docs: add firmware feature flags
:Author: Gerd Hoffmann 
:Date: 2022-09-30 13:32 UTC

Description
===

This patch adds information about firmware feature flags.

Changes
===

- Add information about firmware feature flags to documentation.

Doc Updates
===

- Update documentation with information about firmware feature flags.

File Changes


- No file changes.

==
Re: [PATCH]
==

:Subject: docs: add firmware feature flags
:Author: Kashyap Chamarthy 
:Date: 2022-09-30 9:34 UTC

Description
===

This is a response to a previous patch that added information about
firmware feature flags.

Comment
===

The patch looks good. Reviewed-by: Kashyap Chamarthy


==
PATCH
==

:Subject: docs: add firmware feature flags
:Author: Gerd Hoffmann 
:Date: 2022-09-30 9:18 UTC

Description
===

This patch adds information about firmware feature flags.

Changes
===

- Add information about firmware feature flags to documentation.

Doc Updates
===

- Update documentation with information about firmware feature flags.

File Changes


- No file changes.

==
Re: [PATCH v2]
==

:Subject: docs: Add measurement calculation details to amd-memory-
encryption.txt
:Author: Daniel P. Berrangé 
:Date: 2022-02-16 19:01 UTC

Comment
===

The patch looks good. Reviewed-by: Daniel P. Berrangé


==
Re: [PATCH v2]
==

:Subject: docs: Add measurement calculation details to amd-memory-
encryption.txt
:Author: Dov Murik 
:Date: 2022-02-15 6:52 UTC

Comment
===

The patch looks good. Reviewed-by: Dov Murik 

==
PULL 41/42
==

:Subject: docs: rstfy confidential guest documentation
:Author: Cédric Le Goater 
:Date: 2022-02-10 13:00 UTC

Description
===

This pull request updates confidential guest documentation.

Changes
===

- Update confidential guest documentation to use rst format.

Doc Updates
===

- Update confidential guest documentation to use rst format.

File Changes


- No file changes.

==
PULL 00/42
==

:Subject: ppc queue
:Author: Cédric Le Goater 
:Date: 2022-02-10 12:59 UTC

Description
===

This pull request includes ppc queue changes.

Changes
===

- Various changes to ppc queue.

File Changes


- Various file changes.

.. raw:: html

   
   
   Re: [PATCH] docs: rstfy confidential guest
documentation
   [PATCH] docs: rstfy confidential guest
documentation
   
   

.. _id1:

Re: [PATCH] docs: rstfy confidential guest documentation
**

On 2/4/22 11:12 AM, Cornelia Huck wrote:
> This patch series replaces the SGX-related documentation for the
confidential
> guest feature with rst-based documentation.

Thanks for doing this, Cornelia! The new documentation looks much
better than the old one.

Reviewed-by: Daniel Henrique Barboza 

.. _id2:

[PATCH] docs: rstfy confidential guest documentation
*

This patch series replaces the SGX-related documentation for the
confidential
guest feature with rst-based documentation.

Signed-off-by: Cornelia Huck 
---
 docs/confidential-guest.rst | 2416

 1 file changed, 2416 insertions(+)
 create mode 100644 docs/confidential-guest.rst

10. [PATCH] docs: rstfy confidential guest documentation
~~~

This patch series replaces the SGX-related documentation for the
confidential
guest feature with rst-based documentation.

Signed-off-by: Cornelia Huck 
---
 docs/confidential-guest.rst | 2416

 1 file changed, 2416 insertions(+)
 create mode 100644 docs/confidential-guest.rst

11. Re: [RFC PATCH v2 06/44] hw/i386: Introduce kvm-type for TDX guest
On Mon, Jan 10, 2022 at 12:00:21PM +, Xiaoyao Li wrote:

On 1/7/2022 10:06 PM, Daniel P. Berrangé wrote:

On Fri, Jan 07, 2022 at 02:58:57PM +0800, Xiaoyao Li wrote:

According to https://github.com/intel/KVMGT-
Kernel/blob/master/src/kvmgt.c,
there is no such kvm-type as "tdx", so I think the following document
may
need to be updated.

Thanks for the pointer. Yes, we'll need to update that.

Reviewed-by: Daniel P. Berrangé berra...@redhat.com

Regards,
Daniel

Re: [RFC PATCH v2 06/44] hw/i386: Introduce kvm-type for TDX guest

On Mon, Jan 10, 2022 at 12:00:21PM +, Xiaoyao Li wrote:
> On 1/7/2022 10:06 PM, Daniel P. Berrangé wrote:
> > On Fri, Jan 07, 2022 at 02:58:57PM +0800, Xiaoyao 

Re: [PATCH 01/14] qapi: Fix error message format regression

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:12AM +0100, Markus Armbruster wrote:
> Commit 52a474180ae3 changed reporting of errors connected to a source
> location without mentioning it in the commit message.  For instance,
> 
> $ python scripts/qapi-gen.py tests/qapi-schema/unknown-escape.json
> tests/qapi-schema/unknown-escape.json:3:21: unknown escape \x
> 
> became
> 
> scripts/qapi-gen.py: tests/qapi-schema/unknown-escape.json:3:21: unknown 
> escape \x
> 
> This is not how compilers report such errors, and Emacs doesn't
> recognize the format.  Revert this change.
> 
> Fixes: 52a474180ae3 (qapi-gen: Separate arg-parsing from generation)
> Signed-off-by: Markus Armbruster 
> ---
>  scripts/qapi/main.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[PATCH v3] tests/tcg/x86_64: add cross-modifying code test

[Ilya Leoshkevich]

commit f025692c992c ("accel/tcg: Clear PAGE_WRITE before translation")
fixed cross-modifying code handling, but did not add a test. The
changed code was further improved recently [1], and I was not sure
whether these modifications were safe (spoiler: they were fine).

Add a test to make sure there are no regressions.

[1] https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg00034.html

Signed-off-by: Ilya Leoshkevich 
---

v2: https://patchew.org/QEMU/20220905154944.1284289-1-...@linux.ibm.com/
v2 -> v3: Resend with a trivial rebase.

v1: https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg00455.html
v1 -> v2: Fix tweaking the flags (Alex).
  Keep the custom build rule for now.

 tests/tcg/x86_64/Makefile.target|  4 ++
 tests/tcg/x86_64/cross-modifying-code.c | 80 +
 2 files changed, 84 insertions(+)
 create mode 100644 tests/tcg/x86_64/cross-modifying-code.c

diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target
index e64aab1b81c..331b0b1fcc5 100644
--- a/tests/tcg/x86_64/Makefile.target
+++ b/tests/tcg/x86_64/Makefile.target
@@ -13,6 +13,7 @@ X86_64_TESTS += vsyscall
 X86_64_TESTS += noexec
 X86_64_TESTS += cmpxchg
 X86_64_TESTS += adox
+X86_64_TESTS += cross-modifying-code
 TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64
 else
 TESTS=$(MULTIARCH_TESTS)
@@ -29,3 +30,6 @@ test-x86_64: test-i386.c test-i386.h test-i386-shift.h 
test-i386-muldiv.h
 
 %: $(SRC_PATH)/tests/tcg/x86_64/%.c
$(CC) $(CFLAGS) $< -o $@ $(LDFLAGS)
+
+cross-modifying-code: CFLAGS+=-pthread
+cross-modifying-code: LDFLAGS+=-pthread
diff --git a/tests/tcg/x86_64/cross-modifying-code.c 
b/tests/tcg/x86_64/cross-modifying-code.c
new file mode 100644
index 000..2704df6061c
--- /dev/null
+++ b/tests/tcg/x86_64/cross-modifying-code.c
@@ -0,0 +1,80 @@
+/*
+ * Test patching code, running in one thread, from another thread.
+ *
+ * Intel SDM calls this "cross-modifying code" and recommends a special
+ * sequence, which requires both threads to cooperate.
+ *
+ * Linux kernel uses a different sequence that does not require cooperation and
+ * involves patching the first byte with int3.
+ *
+ * Finally, there is user-mode software out there that simply uses atomics, and
+ * that seems to be good enough in practice. Test that QEMU has no problems
+ * with this as well.
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+void add1_or_nop(long *x);
+asm(".pushsection .rwx,\"awx\",@progbits\n"
+".globl add1_or_nop\n"
+/* addq $0x1,(%rdi) */
+"add1_or_nop: .byte 0x48, 0x83, 0x07, 0x01\n"
+"ret\n"
+".popsection\n");
+
+#define THREAD_WAIT 0
+#define THREAD_PATCH 1
+#define THREAD_STOP 2
+
+static void *thread_func(void *arg)
+{
+int val = 0x0026748d; /* nop */
+
+while (true) {
+switch (__atomic_load_n((int *)arg, __ATOMIC_SEQ_CST)) {
+case THREAD_WAIT:
+break;
+case THREAD_PATCH:
+val = __atomic_exchange_n((int *)_or_nop, val,
+  __ATOMIC_SEQ_CST);
+break;
+case THREAD_STOP:
+return NULL;
+default:
+assert(false);
+__builtin_unreachable();
+}
+}
+}
+
+#define INITIAL 42
+#define COUNT 100
+
+int main(void)
+{
+int command = THREAD_WAIT;
+pthread_t thread;
+long x = 0;
+int err;
+int i;
+
+err = pthread_create(, NULL, _func, );
+assert(err == 0);
+
+__atomic_store_n(, THREAD_PATCH, __ATOMIC_SEQ_CST);
+for (i = 0; i < COUNT; i++) {
+add1_or_nop();
+}
+__atomic_store_n(, THREAD_STOP, __ATOMIC_SEQ_CST);
+
+err = pthread_join(thread, NULL);
+assert(err == 0);
+
+assert(x >= INITIAL);
+assert(x <= INITIAL + COUNT);
+
+return EXIT_SUCCESS;
+}
-- 
2.39.2

[PATCH v3 0/2] Fix EXECUTE of relative long instructions

[Ilya Leoshkevich]

v2: https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg04499.html
v2 -> v3: Make mem static (Nina).
  Initialize cc with cr (Nina).
  Drop long casts (Nina).
  Move mask assignment outside of asm.
  Use "a" constraints instead of "r" where necessary.
  Drop unnecessary earlyclobbers.

v1: https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg04316.html
v1 -> v2: Address the middle of an array in the test (Richard).
  Rebase - not 100% trivial, so not carrying Reviewed-bys.

Hi,

This series fixes EXECUTE of instructions like LARL, LGLR, etc.
Currently the address calculation uses EXECUTE's address as a base,
while it should be using that of the target instruction.
Patch 1 fixes the issue, patch 2 adds a test.

Best regards,
Ilya

Ilya Leoshkevich (2):
  target/s390x: Fix EXECUTE of relative long instructions
  tests/tcg/s390x: Add ex-relative-long.c

 target/s390x/cpu.h |   1 +
 target/s390x/tcg/mem_helper.c  |   1 +
 target/s390x/tcg/translate.c   |  13 ++-
 tests/tcg/s390x/Makefile.target|   1 +
 tests/tcg/s390x/ex-relative-long.c | 156 +
 5 files changed, 171 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/s390x/ex-relative-long.c

-- 
2.39.2

[PATCH v3 2/2] tests/tcg/s390x: Add ex-relative-long.c

[Ilya Leoshkevich]

Test EXECUTE and EXECUTE RELATIVE LONG with relative long instructions
as targets.

Reviewed-by: Richard Henderson 
Reviewed-by: Nina Schoetterl-Glausch 
Signed-off-by: Ilya Leoshkevich 
---
 tests/tcg/s390x/Makefile.target|   1 +
 tests/tcg/s390x/ex-relative-long.c | 156 +
 2 files changed, 157 insertions(+)
 create mode 100644 tests/tcg/s390x/ex-relative-long.c

diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index cf93b966862..90bc48227db 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -29,6 +29,7 @@ TESTS+=clst
 TESTS+=long-double
 TESTS+=cdsg
 TESTS+=chrl
+TESTS+=ex-relative-long
 
 cdsg: CFLAGS+=-pthread
 cdsg: LDFLAGS+=-pthread
diff --git a/tests/tcg/s390x/ex-relative-long.c 
b/tests/tcg/s390x/ex-relative-long.c
new file mode 100644
index 000..21fbef62585
--- /dev/null
+++ b/tests/tcg/s390x/ex-relative-long.c
@@ -0,0 +1,156 @@
+/* Check EXECUTE with relative long instructions as targets. */
+#include 
+#include 
+
+struct test {
+const char *name;
+long (*func)(long reg, long *cc);
+long exp_reg;
+long exp_mem;
+long exp_cc;
+};
+
+/*
+ * Each test sets the MEM_IDXth element of the mem array to MEM and uses a
+ * single relative long instruction on it. The other elements remain zero.
+ * This is in order to prevent stumbling upon MEM in random memory in case
+ * there is an off-by-a-small-value bug.
+ *
+ * Note that while gcc supports the ZL constraint for relative long operands,
+ * clang doesn't, so the assembly code accesses mem[MEM_IDX] using MEM_ASM.
+ */
+static long mem[0x1000];
+#define MEM_IDX 0x800
+#define MEM_ASM "mem+0x800*8"
+
+/* Initial %r2 value. */
+#define REG 0x1234567887654321
+
+/* Initial mem[MEM_IDX] value. */
+#define MEM 0xfedcba9889abcdef
+
+/* Initial cc value. */
+#define CC 0
+
+/* Relative long instructions and their expected effects. */
+#define FOR_EACH_INSN(F)   
\
+F(cgfrl,  REG, MEM,2)  
\
+F(cghrl,  REG, MEM,2)  
\
+F(cgrl,   REG, MEM,2)  
\
+F(chrl,   REG, MEM,1)  
\
+F(clgfrl, REG, MEM,2)  
\
+F(clghrl, REG, MEM,2)  
\
+F(clgrl,  REG, MEM,1)  
\
+F(clhrl,  REG, MEM,2)  
\
+F(clrl,   REG, MEM,1)  
\
+F(crl,REG, MEM,1)  
\
+F(larl,   (long)[MEM_IDX], MEM,CC) 
\
+F(lgfrl,  0xfedcba98,  MEM,CC) 
\
+F(lghrl,  0xfedc,  MEM,CC) 
\
+F(lgrl,   MEM, MEM,CC) 
\
+F(lhrl,   0x12345678fedc,  MEM,CC) 
\
+F(llghrl, 0xfedc,  MEM,CC) 
\
+F(llhrl,  0x12345678fedc,  MEM,CC) 
\
+F(lrl,0x12345678fedcba98,  MEM,CC) 
\
+F(stgrl,  REG, REG,CC) 
\
+F(sthrl,  REG, 0x4321ba9889abcdef, CC) 
\
+F(strl,   REG, 0x8765432189abcdef, CC)
+
+/* Test functions. */
+#define DEFINE_EX_TEST(insn, exp_reg, exp_mem, exp_cc) 
\
+static long test_ex_ ## insn(long reg, long *cc)   
\
+{  
\
+register long r2 asm("r2");
\
+char mask = 0x20;  /* make target use %r2 */   
\
+long pm, target;   
\
+   
\
+r2 = reg;  
\
+asm("larl %[target],0f\n"  
\
+"cr %%r0,%%r0\n"  /* initial cc */ 
\
+"ex %[mask],0(%[target])\n"
\
+"jg 1f\n"  
\
+"0: " #insn " %%r0," MEM_ASM "\n"  
\
+"1: ipm %[pm]\n"   
\
+: [target] "=" (target), [r2] "+r" (r2), [pm] "=r" (pm) 

[PATCH v3 1/2] target/s390x: Fix EXECUTE of relative long instructions

[Ilya Leoshkevich]

The code uses the wrong base for relative addressing: it should use the
target instruction address and not the EXECUTE's address.

Fix by storing the target instruction address in the new CPUS390XState
member and loading it from the code generated by gen_ri2().

Reported-by: Nina Schoetterl-Glausch 
Reviewed-by: Richard Henderson 
Reviewed-by: David Hildenbrand 
Signed-off-by: Ilya Leoshkevich 
---
 target/s390x/cpu.h|  1 +
 target/s390x/tcg/mem_helper.c |  1 +
 target/s390x/tcg/translate.c  | 13 -
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7d6d01325b2..8aaf8dd5a3b 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -87,6 +87,7 @@ struct CPUArchState {
 uint64_t cc_vr;
 
 uint64_t ex_value;
+uint64_t ex_target;
 
 uint64_t __excp_addr;
 uint64_t psa;
diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index 6835c26dda4..00afae2b640 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -2530,6 +2530,7 @@ void HELPER(ex)(CPUS390XState *env, uint32_t ilen, 
uint64_t r1, uint64_t addr)
that ex_value is non-zero, which flags that we are in a state
that requires such execution.  */
 env->ex_value = insn | ilen;
+env->ex_target = addr;
 }
 
 uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t dest, uint64_t src,
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 14c3896d529..e938d8538f8 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -5747,7 +5747,18 @@ static void in2_a2(DisasContext *s, DisasOps *o)
 
 static TCGv gen_ri2(DisasContext *s)
 {
-return tcg_constant_i64(s->base.pc_next + (int64_t)get_field(s, i2) * 2);
+int64_t delta = (int64_t)get_field(s, i2) * 2;
+TCGv ri2;
+
+if (unlikely(s->ex_value)) {
+ri2 = tcg_temp_new_i64();
+tcg_gen_ld_i64(ri2, cpu_env, offsetof(CPUS390XState, ex_target));
+tcg_gen_addi_i64(ri2, ri2, delta);
+} else {
+ri2 = tcg_constant_i64(s->base.pc_next + delta);
+}
+
+return ri2;
 }
 
 static void in2_ri2(DisasContext *s, DisasOps *o)
-- 
2.39.2

Re: [PATCH v2 2/2] tests/tcg/s390x: Add ex-relative-long.c

[Ilya Leoshkevich]

On Thu, 2023-03-16 at 18:50 +0100, Nina Schoetterl-Glausch wrote:
> On Wed, 2023-03-15 at 01:11 +0100, Ilya Leoshkevich wrote:
> > > Test EXECUTE and EXECUTE RELATIVE LONG with relative long
> > > instructions
> > > as targets.
> > > 
> > > Signed-off-by: Ilya Leoshkevich 
> 
> Reviewed-by: Nina Schoetterl-Glausch 
> 
> Some comments below.
> 
> > > ---
> > >  tests/tcg/s390x/Makefile.target    |   1 +
> > >  tests/tcg/s390x/ex-relative-long.c | 159
> > > +
> > >  2 files changed, 160 insertions(+)
> > >  create mode 100644 tests/tcg/s390x/ex-relative-long.c
> > > 
> > > diff --git a/tests/tcg/s390x/Makefile.target
> > > b/tests/tcg/s390x/Makefile.target
> > > index cf93b966862..90bc48227db 100644
> > > --- a/tests/tcg/s390x/Makefile.target
> > > +++ b/tests/tcg/s390x/Makefile.target
> > > @@ -29,6 +29,7 @@ TESTS+=clst
> > >  TESTS+=long-double
> > >  TESTS+=cdsg
> > >  TESTS+=chrl
> > > +TESTS+=ex-relative-long
> > >  
> > >  cdsg: CFLAGS+=-pthread
> > >  cdsg: LDFLAGS+=-pthread
> > > diff --git a/tests/tcg/s390x/ex-relative-long.c
> > > b/tests/tcg/s390x/ex-relative-long.c
> > > new file mode 100644
> > > index 000..4caa8c1b962
> > > --- /dev/null
> > > +++ b/tests/tcg/s390x/ex-relative-long.c
> > > @@ -0,0 +1,159 @@
> > > +/* Check EXECUTE with relative long instructions as targets. */
> > > +#include 
> > > +#include 
> > > +
> > > +struct test {
> > > +    const char *name;
> > > +    long (*func)(long reg, long *cc);
> > > +    long exp_reg;
> > > +    long exp_mem;
> > > +    long exp_cc;
> > > +};
> > > +
> > > +/*
> > > + * Each test sets the MEM_IDXth element of the mem array to MEM
> > > and uses a
> > > + * single relative long instruction on it. The other elements
> > > remain zero.
> > > + * This is in order to prevent stumbling upon MEM in random
> > > memory in case
> > > + * there is an off-by-a-small-value bug.
> > > + *
> > > + * Note that while gcc supports the ZL constraint for relative
> > > long operands,
> > > + * clang doesn't, so the assembly code accesses mem[MEM_IDX]
> > > using MEM_ASM.
> > > + */
> > > +long mem[0x1000];
> 
> This could be static, no?

I was worried that mem would become inaccessible from the asm block,
but apparently it still works if I make mem static.

> > > +#define MEM_IDX 0x800
> > > +#define MEM_ASM "mem+0x800*8"
> > > +
> > > +/* Initial %r2 value. */
> > > +#define REG 0x1234567887654321
> > > +
> > > +/* Initial mem[MEM_IDX] value. */
> > > +#define MEM 0xfedcba9889abcdef
> > > +
> > > +/* Initial cc value. */
> > > +#define CC 0
> > > +
> > > +/* Relative long instructions and their expected effects. */
> > > +#define
> > > FOR_EACH_INSN(F) 
> > >   \
> 
> You could define some macros and then calculate a bunch of values in
> the table, i.e.
> #define SL(v) ((long)(v))
> #define UL(v) ((unsigned long)(v))
> #define SI(v, i) ((int)(v >> ((1 - i) * 32)))
> #define UI(v, i) ((unsigned int)(v >> ((1 - i) * 32)))
> #define SH(v, i) ((short)(v >> ((3 - i) * 16)))
> #define UH(v, i) ((unsigned short)(v >> ((3 - i) * 16)))
> #define CMP(f, s) ((f) == (s) ? 0 : ((f) < (s) ? 1 : 2 ))
> 
> F(cgfrl,  REG, MEM,    CMP(SL(REG),
> SI(MEM, 0))
> 
> But everything checks out, so no need.
> 
> > > +    F(cgfrl,  REG, MEM,   
> > > 2)  \
> > > +    F(cghrl,  REG, MEM,   
> > > 2)  \
> > > +    F(cgrl,   REG, MEM,   
> > > 2)  \
> > > +    F(chrl,   REG, MEM,   
> > > 1)  \
> > > +    F(clgfrl, REG, MEM,   
> > > 2)  \
> > > +    F(clghrl, REG, MEM,   
> > > 2)  \
> > > +    F(clgrl,  REG, MEM,   
> > > 1)  \
> > > +    F(clhrl,  REG, MEM,   
> > > 2)  \
> > > +    F(clrl,   REG, MEM,   
> > > 1)  \
> > > +    F(crl,    REG, MEM,   
> > > 1)  \
> > > +    F(larl,   (long)[MEM_IDX], MEM,   
> > > CC) \
> > > +    F(lgfrl,  0xfedcba98,  MEM,   
> > > CC) \
> > > +    F(lghrl,  0xfedc,  MEM,   
> > > CC) \
> > > +    F(lgrl,   MEM, MEM,   
> > > CC) \
> > > +    F(lhrl,   0x12345678fedc,  MEM,   
> > > CC) \
> > > +    F(llghrl, 0xfedc,  MEM,   
> > > CC) \
> > > +    F(llhrl,  0x12345678fedc,  MEM,   
> > > CC) \
> > > +    F(lrl,    0x12345678fedcba98,  MEM,   
> > > CC) \
> > > 

RE: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor

[Sriram Yagnaraman]

> -Original Message-
> From: qemu-devel-bounces+sriram.yagnaraman=est.t...@nongnu.org
>  On Behalf
> Of Akihiko Odaki
> Sent: Thursday, 16 March 2023 16:57
> Cc: qemu-devel@nongnu.org; Jason Wang ; Dmitry
> Fleytman ; quint...@redhat.com; Philippe
> Mathieu-Daudé ; Akihiko Odaki
> 
> Subject: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor
> 
> The current implementation of igb uses only part of a advanced Tx context
> descriptor because it misses some features and sniffs the trait of the packet
> instead of respecting the packet type specified in the descriptor. However, we
> will certainly need the entire Tx context descriptor when we update igb to
> respect these ignored fields. Save the entire Tx context descriptor to prepare
> for such a change.
> 
> Signed-off-by: Akihiko Odaki 
> ---
> V1 -> V2: Bump igb-tx version
> 
>  hw/net/igb.c  | 10 ++
>  hw/net/igb_core.c | 17 ++---  hw/net/igb_core.h |  3 +--
>  3 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/net/igb.c b/hw/net/igb.c index c6d753df87..f9ec82fc28 100644
> --- a/hw/net/igb.c
> +++ b/hw/net/igb.c
> @@ -504,11 +504,13 @@ static int igb_post_load(void *opaque, int
> version_id)
> 
>  static const VMStateDescription igb_vmstate_tx = {
>  .name = "igb-tx",
> -.version_id = 1,
> -.minimum_version_id = 1,
> +.version_id = 2,
> +.minimum_version_id = 2,
>  .fields = (VMStateField[]) {
> -VMSTATE_UINT16(vlan, struct igb_tx),
> -VMSTATE_UINT16(mss, struct igb_tx),
> +VMSTATE_UINT32(ctx.vlan_macip_lens, struct igb_tx),
> +VMSTATE_UINT32(ctx.seqnum_seed, struct igb_tx),
> +VMSTATE_UINT32(ctx.type_tucmd_mlhl, struct igb_tx),
> +VMSTATE_UINT32(ctx.mss_l4len_idx, struct igb_tx),
>  VMSTATE_BOOL(tse, struct igb_tx),
>  VMSTATE_BOOL(ixsm, struct igb_tx),
>  VMSTATE_BOOL(txsm, struct igb_tx), diff --git a/hw/net/igb_core.c
> b/hw/net/igb_core.c index a7c7bfdc75..304f5d849f 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -390,7 +390,8 @@ static bool
>  igb_setup_tx_offloads(IGBCore *core, struct igb_tx *tx)  {
>  if (tx->tse) {
> -if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, tx->mss)) {
> +uint32_t mss = tx->ctx.mss_l4len_idx >> 16;
> +if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, mss)) {
>  return false;
>  }
> 
> @@ -550,8 +551,10 @@ igb_process_tx_desc(IGBCore *core,
> E1000_ADVTXD_DTYP_CTXT) {
>  /* advanced transmit context descriptor */
>  tx_ctx_desc = (struct e1000_adv_tx_context_desc *)tx_desc;
> -tx->vlan = le32_to_cpu(tx_ctx_desc->vlan_macip_lens) >> 16;
> -tx->mss = le32_to_cpu(tx_ctx_desc->mss_l4len_idx) >> 16;
> +tx->ctx.vlan_macip_lens = 
> le32_to_cpu(tx_ctx_desc->vlan_macip_lens);
> +tx->ctx.seqnum_seed = le32_to_cpu(tx_ctx_desc->seqnum_seed);
> +tx->ctx.type_tucmd_mlhl = le32_to_cpu(tx_ctx_desc-
> >type_tucmd_mlhl);
> +tx->ctx.mss_l4len_idx =
> + le32_to_cpu(tx_ctx_desc->mss_l4len_idx);

Wouldn't it be better to parse the context into all the required fields like 
vlan, mss, etc., already when handling the context descriptor, instead of 
parsing it for every data descriptor later?
Also, in my yet to be merged patch [1] which handles VLAN insertion for VMDq I 
use the vlan field in multiple places, so it would be better to have the vlan 
value readily available. 
[1]: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg00393.html

>  return;
>  } else {
>  /* unknown descriptor type */ @@ -575,8 +578,9 @@
> igb_process_tx_desc(IGBCore *core,
>  if (cmd_type_len & E1000_TXD_CMD_EOP) {
>  if (!tx->skip_cp && net_tx_pkt_parse(tx->tx_pkt)) {
>  if (cmd_type_len & E1000_TXD_CMD_VLE) {
> -net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, tx->vlan,
> -core->mac[VET] & 0x);
> +uint16_t vlan = tx->ctx.vlan_macip_lens >> 16;
> +uint16_t vet = core->mac[VET] & 0x;
> +net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, vlan, vet);
>  }
>  if (igb_tx_pkt_send(core, tx, queue_index)) {
>  igb_on_tx_done_update_stats(core, tx->tx_pkt); @@ -4024,8
> +4028,7 @@ static void igb_reset(IGBCore *core, bool sw)
>  for (i = 0; i < ARRAY_SIZE(core->tx); i++) {
>  tx = >tx[i];
>  net_tx_pkt_reset(tx->tx_pkt);
> -tx->vlan = 0;
> -tx->mss = 0;
> +memset(>ctx, 0, sizeof(tx->ctx));
>  tx->tse = false;
>  tx->ixsm = false;
>  tx->txsm = false;
> diff --git a/hw/net/igb_core.h b/hw/net/igb_core.h index
> 814c1e264b..3483edc655 100644
> --- a/hw/net/igb_core.h
> +++ b/hw/net/igb_core.h
> @@ -72,8 +72,7 @@ struct IGBCore {
>  QEMUTimer *autoneg_timer;
> 
>  struct 

[PULL 0/2] target/xtensa TCG test updates

[Max Filippov]

Hi Peter,

please pull the following updates for the target/xtensa TCG tests.

The following changes since commit 27a03171d02ee0de8de4e2d3bed241795d672859:

  Merge tag 'pull-tcg-20230313' of https://gitlab.com/rth7680/qemu into staging 
(2023-03-14 10:09:15 +)

are available in the Git repository at:

  https://github.com/OSLL/qemu-xtensa.git tags/20230316-xtensa

for you to fetch changes up to 51139fb3e7b05dd7daeca8f00748678ce9e087e5:

  tests/tcg/xtensa: allow testing big-endian cores (2023-03-15 05:08:04 -0700)


target/xtensa updates for v8.0:

- enable testing big-endian xtensa cores


Max Filippov (2):
  tests/tcg/xtensa: add linker.ld to CLEANFILES
  tests/tcg/xtensa: allow testing big-endian cores

 MAINTAINERS| 1 +
 tests/tcg/xtensa/Makefile.softmmu-target   | 5 +++--
 tests/tcg/xtensaeb/Makefile.softmmu-target | 5 +
 3 files changed, 9 insertions(+), 2 deletions(-)
 create mode 100644 tests/tcg/xtensaeb/Makefile.softmmu-target

-- 
Thanks.
-- Max

Re: [PATCH v6 1/4] file-posix: add tracking of the zone write pointers

[Stefan Hajnoczi]

On Fri, Mar 10, 2023 at 06:31:03PM +0800, Sam Li wrote:
> @@ -2338,9 +2424,15 @@ static int coroutine_fn raw_co_prw(BlockDriverState 
> *bs, uint64_t offset,
>  {
>  BDRVRawState *s = bs->opaque;
>  RawPosixAIOData acb;
> +int ret;
>  
>  if (fd_open(bs) < 0)
>  return -EIO;
> +#if defined(CONFIG_BLKZONED)
> +if (bs->bl.wps) {
> +qemu_co_mutex_lock(>bl.wps->colock);
> +}
> +#endif

Is the lock only needed by QEMU_AIO_WRITE requests? If yes, can we skip
it for other request types to avoid serializing those requests?


signature.asc
Description: PGP signature

Re: [PATCH v7 0/4] Add zoned storage emulation to virtio-blk driver

[Stefan Hajnoczi]

On Fri, Mar 10, 2023 at 06:54:27PM +0800, Sam Li wrote:
> This patch adds zoned storage emulation to the virtio-blk driver.
> 
> The patch implements the virtio-blk ZBD support standardization that is
> recently accepted by virtio-spec. The link to related commit is at
> 
> https://github.com/oasis-tcs/virtio-spec/commit/b4e8efa0fa6c8d844328090ad15db65af8d7d981
> 
> The Linux zoned device code that implemented by Dmitry Fomichev has been
> released at the latest Linux version v6.3-rc1.
> 
> Aside: adding zoned=on alike options to virtio-blk device will be
> considered as following-ups in future.
> 
> v6:
> - update headers to v6.3-rc1

Hi Sam,
I had some minor comments but overall this looks good. Looking forward
to merging it soon!

Thanks,
Stefan


signature.asc
Description: PGP signature

Re: [PATCH v6 2/4] block: introduce zone append write for zoned devices

[Stefan Hajnoczi]

On Fri, Mar 10, 2023 at 06:31:04PM +0800, Sam Li wrote:
> A zone append command is a write operation that specifies the first
> logical block of a zone as the write position. When writing to a zoned
> block device using zone append, the byte offset of writes is pointing
> to the write pointer of that zone. Upon completion the device will
> respond with the position the data has been written in the zone.
> 
> Signed-off-by: Sam Li 
> ---
>  block/block-backend.c | 60 +++
>  block/file-posix.c| 54 +---
>  block/io.c| 21 +++
>  block/io_uring.c  |  4 +++
>  block/linux-aio.c |  3 ++
>  block/raw-format.c|  8 +
>  include/block/block-io.h  |  4 +++
>  include/block/block_int-common.h  |  5 +++
>  include/block/raw-aio.h   |  4 ++-
>  include/sysemu/block-backend-io.h |  9 +
>  10 files changed, 166 insertions(+), 6 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index f70b08e3f6..28e8f5d778 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1888,6 +1888,45 @@ BlockAIOCB *blk_aio_zone_mgmt(BlockBackend *blk, 
> BlockZoneOp op,
>  return >common;
>  }
>  
> +static void coroutine_fn blk_aio_zone_append_entry(void *opaque)
> +{
> +BlkAioEmAIOCB *acb = opaque;
> +BlkRwCo *rwco = >rwco;
> +
> +rwco->ret = blk_co_zone_append(rwco->blk, >bytes,
> +   rwco->iobuf, rwco->flags);
> +blk_aio_complete(acb);
> +}
> +
> +BlockAIOCB *blk_aio_zone_append(BlockBackend *blk, int64_t *offset,
> +QEMUIOVector *qiov, BdrvRequestFlags flags,
> +BlockCompletionFunc *cb, void *opaque) {
> +BlkAioEmAIOCB *acb;
> +Coroutine *co;
> +IO_CODE();
> +
> +blk_inc_in_flight(blk);
> +acb = blk_aio_get(_aio_em_aiocb_info, blk, cb, opaque);
> +acb->rwco = (BlkRwCo) {
> +.blk= blk,
> +.ret= NOT_DONE,
> +.flags  = flags,
> +.iobuf  = qiov,
> +};
> +acb->bytes = *offset;
> +acb->has_returned = false;
> +
> +co = qemu_coroutine_create(blk_aio_zone_append_entry, acb);
> +aio_co_enter(blk_get_aio_context(blk), co);
> +acb->has_returned = true;
> +if (acb->rwco.ret != NOT_DONE) {
> +replay_bh_schedule_oneshot_event(blk_get_aio_context(blk),
> + blk_aio_complete_bh, acb);
> +}
> +
> +return >common;
> +}

How is the resulting offset value communicated back to the caller? I
see offset being read (dereferenced) but there is no write (assignment).
Maybe this function should pass through acb->bytes = (int64_t)offset
instead so that blk_co_zone_append() can modify the offset?


signature.asc
Description: PGP signature

Re: [PATCH v7 3/4] block: add accounting for zone append operation

[Stefan Hajnoczi]

On Fri, Mar 10, 2023 at 06:54:30PM +0800, Sam Li wrote:
> Taking account of the new zone append write operation for zoned devices,
> BLOCK_ACCT_APPEND enum is introduced as other I/O request type (read,
> write, flush).

Can it be called BLOCK_ACCT_ZONE_APPEND so it's clear that this
operation is specific to zoned devices? I think people might not make
the connection if they just see "append" and think that regular devices
support this operation.

> 
> Signed-off-by: Sam Li 
> ---
>  block/qapi-sysemu.c| 11 
>  block/qapi.c   | 15 ++
>  hw/block/virtio-blk.c  |  4 +++
>  include/block/accounting.h |  1 +
>  qapi/block-core.json   | 56 ++
>  qapi/block.json|  4 +++
>  6 files changed, 80 insertions(+), 11 deletions(-)
> 
> diff --git a/block/qapi-sysemu.c b/block/qapi-sysemu.c
> index 7bd7554150..f7e56dfeb2 100644
> --- a/block/qapi-sysemu.c
> +++ b/block/qapi-sysemu.c
> @@ -517,6 +517,7 @@ void qmp_block_latency_histogram_set(
>  bool has_boundaries, uint64List *boundaries,
>  bool has_boundaries_read, uint64List *boundaries_read,
>  bool has_boundaries_write, uint64List *boundaries_write,
> +bool has_boundaries_append, uint64List *boundaries_append,
>  bool has_boundaries_flush, uint64List *boundaries_flush,
>  Error **errp)
>  {
> @@ -557,6 +558,16 @@ void qmp_block_latency_histogram_set(
>  }
>  }
>  
> +if (has_boundaries || has_boundaries_append) {
> +ret = block_latency_histogram_set(
> +stats, BLOCK_ACCT_APPEND,
> +has_boundaries_append ? boundaries_append : boundaries);
> +if (ret) {
> +error_setg(errp, "Device '%s' set append write boundaries fail", 
> id);
> +return;
> +}
> +}
> +
>  if (has_boundaries || has_boundaries_flush) {
>  ret = block_latency_histogram_set(
>  stats, BLOCK_ACCT_FLUSH,
> diff --git a/block/qapi.c b/block/qapi.c
> index c84147849d..d4be8ad72e 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -533,27 +533,33 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, 
> BlockBackend *blk)
>  
>  ds->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ];
>  ds->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE];
> +ds->zap_bytes = stats->nr_bytes[BLOCK_ACCT_APPEND];

"zone_append_bytes" would be clearer. For a moment I thought "zap" is a
new operation. Since "zap" isn't used anywhere else, let's not introduce
a new name here.

>  ds->unmap_bytes = stats->nr_bytes[BLOCK_ACCT_UNMAP];
>  ds->rd_operations = stats->nr_ops[BLOCK_ACCT_READ];
>  ds->wr_operations = stats->nr_ops[BLOCK_ACCT_WRITE];
> +ds->zap_operations = stats->nr_ops[BLOCK_ACCT_APPEND];
>  ds->unmap_operations = stats->nr_ops[BLOCK_ACCT_UNMAP];
>  
>  ds->failed_rd_operations = stats->failed_ops[BLOCK_ACCT_READ];
>  ds->failed_wr_operations = stats->failed_ops[BLOCK_ACCT_WRITE];
> +ds->failed_zap_operations = stats->failed_ops[BLOCK_ACCT_APPEND];
>  ds->failed_flush_operations = stats->failed_ops[BLOCK_ACCT_FLUSH];
>  ds->failed_unmap_operations = stats->failed_ops[BLOCK_ACCT_UNMAP];
>  
>  ds->invalid_rd_operations = stats->invalid_ops[BLOCK_ACCT_READ];
>  ds->invalid_wr_operations = stats->invalid_ops[BLOCK_ACCT_WRITE];
> +ds->invalid_zap_operations = stats->invalid_ops[BLOCK_ACCT_APPEND];
>  ds->invalid_flush_operations =
>  stats->invalid_ops[BLOCK_ACCT_FLUSH];
>  ds->invalid_unmap_operations = stats->invalid_ops[BLOCK_ACCT_UNMAP];
>  
>  ds->rd_merged = stats->merged[BLOCK_ACCT_READ];
>  ds->wr_merged = stats->merged[BLOCK_ACCT_WRITE];
> +ds->zap_merged = stats->merged[BLOCK_ACCT_APPEND];
>  ds->unmap_merged = stats->merged[BLOCK_ACCT_UNMAP];
>  ds->flush_operations = stats->nr_ops[BLOCK_ACCT_FLUSH];
>  ds->wr_total_time_ns = stats->total_time_ns[BLOCK_ACCT_WRITE];
> +ds->zap_total_time_ns = stats->total_time_ns[BLOCK_ACCT_APPEND];
>  ds->rd_total_time_ns = stats->total_time_ns[BLOCK_ACCT_READ];
>  ds->flush_total_time_ns = stats->total_time_ns[BLOCK_ACCT_FLUSH];
>  ds->unmap_total_time_ns = stats->total_time_ns[BLOCK_ACCT_UNMAP];
> @@ -571,6 +577,7 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, 
> BlockBackend *blk)
>  
>  TimedAverage *rd = >latency[BLOCK_ACCT_READ];
>  TimedAverage *wr = >latency[BLOCK_ACCT_WRITE];
> +TimedAverage *zap = >latency[BLOCK_ACCT_APPEND];
>  TimedAverage *fl = >latency[BLOCK_ACCT_FLUSH];
>  
>  dev_stats->interval_length = ts->interval_length;
> @@ -583,6 +590,10 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, 
> BlockBackend *blk)
>  dev_stats->max_wr_latency_ns = timed_average_max(wr);
>  dev_stats->avg_wr_latency_ns = timed_average_avg(wr);
>  
> +dev_stats->min_zap_latency_ns = timed_average_min(zap);
> +dev_stats->max_zap_latency_ns = 

Re: [PATCH v7 2/4] virtio-blk: add zoned storage emulation for zoned devices

[Stefan Hajnoczi]

On Fri, Mar 10, 2023 at 06:54:29PM +0800, Sam Li wrote:
> This patch extends virtio-blk emulation to handle zoned device commands
> by calling the new block layer APIs to perform zoned device I/O on
> behalf of the guest. It supports Report Zone, four zone oparations (open,
> close, finish, reset), and Append Zone.
> 
> The VIRTIO_BLK_F_ZONED feature bit will only be set if the host does
> support zoned block devices. Regular block devices(conventional zones)
> will not be set.
> 
> The guest os can use blktests, fio to test those commands on zoned devices.
> Furthermore, using zonefs to test zone append write is also supported.
> 
> Signed-off-by: Sam Li 
> ---
>  hw/block/virtio-blk-common.c |   2 +
>  hw/block/virtio-blk.c| 394 +++
>  2 files changed, 396 insertions(+)
> 
> diff --git a/hw/block/virtio-blk-common.c b/hw/block/virtio-blk-common.c
> index ac52d7c176..e2f8e2f6da 100644
> --- a/hw/block/virtio-blk-common.c
> +++ b/hw/block/virtio-blk-common.c
> @@ -29,6 +29,8 @@ static const VirtIOFeature feature_sizes[] = {
>   .end = endof(struct virtio_blk_config, discard_sector_alignment)},
>  {.flags = 1ULL << VIRTIO_BLK_F_WRITE_ZEROES,
>   .end = endof(struct virtio_blk_config, write_zeroes_may_unmap)},
> +{.flags = 1ULL << VIRTIO_BLK_F_ZONED,
> + .end = endof(struct virtio_blk_config, zoned)},
>  {}
>  };
>  
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index cefca93b31..4ded625732 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -17,6 +17,7 @@
>  #include "qemu/module.h"
>  #include "qemu/error-report.h"
>  #include "qemu/main-loop.h"
> +#include "block/block_int.h"
>  #include "trace.h"
>  #include "hw/block/block.h"
>  #include "hw/qdev-properties.h"
> @@ -601,6 +602,341 @@ err:
>  return err_status;
>  }
>  
> +typedef struct ZoneCmdData {
> +VirtIOBlockReq *req;
> +struct iovec *in_iov;
> +unsigned in_num;
> +union {
> +struct {
> +unsigned int nr_zones;
> +BlockZoneDescriptor *zones;
> +} zone_report_data;
> +struct {
> +int64_t offset;
> +} zone_append_data;
> +};
> +} ZoneCmdData;
> +
> +/*
> + * check zoned_request: error checking before issuing requests. If all checks
> + * passed, return true.
> + * append: true if only zone append requests issued.
> + */
> +static bool check_zoned_request(VirtIOBlock *s, int64_t offset, int64_t len,
> + bool append, uint8_t *status) {
> +BlockDriverState *bs = blk_bs(s->blk);
> +int index;
> +
> +if (!virtio_has_feature(s->host_features, VIRTIO_BLK_F_ZONED)) {
> +*status = VIRTIO_BLK_S_UNSUPP;
> +return false;
> +}
> +
> +if (offset < 0 || len < 0 || len > (bs->total_sectors << 
> BDRV_SECTOR_BITS)
> +|| offset > (bs->total_sectors << BDRV_SECTOR_BITS) - len) {
> +*status = VIRTIO_BLK_S_ZONE_INVALID_CMD;
> +return false;
> +}
> +
> +if (append) {
> +if (bs->bl.write_granularity) {
> +if ((offset % bs->bl.write_granularity) != 0) {
> +*status = VIRTIO_BLK_S_ZONE_UNALIGNED_WP;
> +return false;
> +}
> +}
> +
> +index = offset / bs->bl.zone_size;
> +if (BDRV_ZT_IS_CONV(bs->bl.wps->wp[index])) {
> +*status = VIRTIO_BLK_S_ZONE_INVALID_CMD;
> +return false;
> +}
> +
> +if (len / 512 > bs->bl.max_append_sectors) {
> +if (bs->bl.max_append_sectors == 0) {
> +*status = VIRTIO_BLK_S_UNSUPP;
> +} else {
> +*status = VIRTIO_BLK_S_ZONE_INVALID_CMD;
> +}
> +return false;
> +}
> +}
> +return true;
> +}
> +
> +static void virtio_blk_zone_report_complete(void *opaque, int ret)
> +{
> +ZoneCmdData *data = opaque;
> +VirtIOBlockReq *req = data->req;
> +VirtIOBlock *s = req->dev;
> +VirtIODevice *vdev = VIRTIO_DEVICE(req->dev);
> +struct iovec *in_iov = data->in_iov;
> +unsigned in_num = data->in_num;
> +int64_t zrp_size, n, j = 0;
> +int64_t nz = data->zone_report_data.nr_zones;
> +int8_t err_status = VIRTIO_BLK_S_OK;
> +
> +if (ret) {
> +err_status = VIRTIO_BLK_S_ZONE_INVALID_CMD;
> +goto out;
> +}
> +
> +struct virtio_blk_zone_report zrp_hdr = (struct virtio_blk_zone_report) {
> +.nr_zones = cpu_to_le64(nz),

Indentation is off. QEMU uses 4-space indentation.

> +};
> +zrp_size = sizeof(struct virtio_blk_zone_report)
> +   + sizeof(struct virtio_blk_zone_descriptor) * nz;
> +n = iov_from_buf(in_iov, in_num, 0, _hdr, sizeof(zrp_hdr));
> +if (n != sizeof(zrp_hdr)) {
> +virtio_error(vdev, "Driver provided input buffer that is too 
> small!");
> +err_status = VIRTIO_BLK_S_ZONE_INVALID_CMD;
> +goto out;
> +}
> +
> +for (size_t i = 

Re: [PATCH v6 3/4] qemu-iotests: test zone append operation

[Stefan Hajnoczi]

On Fri, Mar 10, 2023 at 06:31:05PM +0800, Sam Li wrote:
> This tests is mainly a helper to indicate append writes in block layer
> behaves as expected.
> 
> Signed-off-by: Sam Li 
> ---
>  qemu-io-cmds.c | 65 ++
>  tests/qemu-iotests/tests/zoned.out |  7 
>  tests/qemu-iotests/tests/zoned.sh  |  9 +
>  3 files changed, 81 insertions(+)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index f35ea627d7..4159f41ab9 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -1874,6 +1874,70 @@ static const cmdinfo_t zone_reset_cmd = {
>  .oneline = "reset a zone write pointer in zone block device",
>  };
>  
> +static int do_aio_zone_append(BlockBackend *blk, QEMUIOVector *qiov,
> +  int64_t *offset, int flags, int *total)
> +{
> +int async_ret = NOT_DONE;
> +
> +blk_aio_zone_append(blk, offset, qiov, flags, aio_rw_done, _ret);
> +while (async_ret == NOT_DONE) {
> +main_loop_wait(false);
> +}
> +
> +*total = qiov->size;
> +return async_ret < 0 ? async_ret : 1;
> +}
> +
> +static int zone_append_f(BlockBackend *blk, int argc, char **argv)
> +{
> +int ret;
> +int flags = 0;
> +int total = 0;
> +int64_t offset;
> +char *buf;
> +int nr_iov;
> +int pattern = 0xcd;
> +QEMUIOVector qiov;
> +
> +if (optind > argc - 2) {
> +return -EINVAL;
> +}
> +optind++;
> +offset = cvtnum(argv[optind]);
> +if (offset < 0) {
> +print_cvtnum_err(offset, argv[optind]);
> +return offset;
> +}
> +optind++;
> +nr_iov = argc - optind;
> +buf = create_iovec(blk, , [optind], nr_iov, pattern,
> +   flags & BDRV_REQ_REGISTERED_BUF);
> +if (buf == NULL) {
> +return -EINVAL;
> +}
> +ret = do_aio_zone_append(blk, , , flags, );
> +if (ret < 0) {
> +printf("zone append failed: %s\n", strerror(-ret));
> +goto out;
> +}

How about a -p option that prints the value of offset after the
operation completes? That way the test case can check that
blk_aio_zone_append() produces the right offset value.

(The tests should also check zone_report output, but they should verify
that offset is correctly updated by zone_append too.)

> +
> +out:
> +qemu_io_free(blk, buf, qiov.size,
> + flags & BDRV_REQ_REGISTERED_BUF);
> +qemu_iovec_destroy();
> +return ret;
> +}
> +
> +static const cmdinfo_t zone_append_cmd = {
> +.name = "zone_append",
> +.altname = "zap",
> +.cfunc = zone_append_f,
> +.argmin = 3,
> +.argmax = 3,
> +.args = "offset len [len..]",
> +.oneline = "append write a number of bytes at a specified offset",
> +};
> +
>  static int truncate_f(BlockBackend *blk, int argc, char **argv);
>  static const cmdinfo_t truncate_cmd = {
>  .name   = "truncate",
> @@ -2672,6 +2736,7 @@ static void __attribute((constructor)) 
> init_qemuio_commands(void)
>  qemuio_add_command(_close_cmd);
>  qemuio_add_command(_finish_cmd);
>  qemuio_add_command(_reset_cmd);
> +qemuio_add_command(_append_cmd);
>  qemuio_add_command(_cmd);
>  qemuio_add_command(_cmd);
>  qemuio_add_command(_cmd);
> diff --git a/tests/qemu-iotests/tests/zoned.out 
> b/tests/qemu-iotests/tests/zoned.out
> index 0c8f96deb9..b3b139b4ec 100644
> --- a/tests/qemu-iotests/tests/zoned.out
> +++ b/tests/qemu-iotests/tests/zoned.out
> @@ -50,4 +50,11 @@ start: 0x8, len 0x8, cap 0x8, wptr 0x10, 
> zcond:14, [type: 2]
>  (5) resetting the second zone
>  After resetting a zone:
>  start: 0x8, len 0x8, cap 0x8, wptr 0x8, zcond:1, [type: 2]
> +
> +
> +(6) append write
> +After appending the first zone:
> +start: 0x0, len 0x8, cap 0x8, wptr 0x18, zcond:2, [type: 2]
> +After appending the second zone:
> +start: 0x8, len 0x8, cap 0x8, wptr 0x80018, zcond:2, [type: 2]
>  *** done
> diff --git a/tests/qemu-iotests/tests/zoned.sh 
> b/tests/qemu-iotests/tests/zoned.sh
> index 9d7c15dde6..6c3ded6c4c 100755
> --- a/tests/qemu-iotests/tests/zoned.sh
> +++ b/tests/qemu-iotests/tests/zoned.sh
> @@ -79,6 +79,15 @@ echo "(5) resetting the second zone"
>  sudo $QEMU_IO $IMG -c "zrs 268435456 268435456"
>  echo "After resetting a zone:"
>  sudo $QEMU_IO $IMG -c "zrp 268435456 1"
> +echo
> +echo
> +echo "(6) append write" # physical block size of the device is 4096
> +sudo $QEMU_IO $IMG -c "zap 0 0x1000 0x2000"
> +echo "After appending the first zone:"
> +sudo $QEMU_IO $IMG -c "zrp 0 1"
> +sudo $QEMU_IO $IMG -c "zap 268435456 0x1000 0x2000"
> +echo "After appending the second zone:"
> +sudo $QEMU_IO $IMG -c "zrp 268435456 1"
>  
>  # success, all done
>  echo "*** done"
> -- 
> 2.39.2
> 


signature.asc
Description: PGP signature

Call failed: MCTP Endpoint did not respond: Qemu CXL switch with mctp-1.0

[Maverickk 78]

Hi

 I am trying mctp & mctpd with aspeed +buildroot(master) + linux v6.2
with Qemu 7.2.


I have added necessary FMAPI related patches into QEMU to support CLX
switch emulation

RFC-1-2-misc-i2c_mctp_cxl_fmapi-Initial-device-emulation.diff

RFC-2-3-hw-i2c-add-mctp-core.diff

RFC-4-4-hw-misc-add-a-toy-i2c-echo-device.diff

RFC-2-2-arm-virt-Add-aspeed-i2c-controller-and-MCTP-EP-to-enable-MCTP-testing.diff

RFC-3-3-hw-nvme-add-nvme-management-interface-model.diff


Executed following mctp commands to setup the binding,

mctp link set mctpi2c15 up

mctp addr add 50 dev mctpi2c15

mctp link set mctpi2c15 net 11

systemctl restart mctpd.service

busctl call xyz.openbmc_project.MCTP /xyz/openbmc_project/mctp
au.com.CodeConstruct.MCTP AssignEndpoint say mctpi2c15 1 0x4d


 The above busctl configuration is reaching fmapi patch and sets up
the endpoint id but then mctpd fails with log after timeout.

Call failed: MCTP Endpoint did not respond

Any clue what's going on?


Regards
Raghu

Re: [PATCH v16 0/8] Add support for zoned device

[Stefan Hajnoczi]

On Fri, Mar 10, 2023 at 06:23:55PM +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.
> 
> There has been a debate on whethre introducing new zoned_host_device 
> BlockDriver
> specifically for zoned devices. In the end, it's been decided to stick to
> existing host_device BlockDriver interface by only adding new zoned operations
> inside it. The benefit of that is to avoid further changes - one example is
> command line syntax - to the applications like Libvirt using QEMU zoned
> emulation.
> 
> It can be tested on a null_blk device using qemu-io or qemu-iotests. For
> example, to test zone report using qemu-io:
> $ path/to/qemu-io --image-opts -n driver=host_device,filename=/dev/nullb0
> -c "zrp offset nr_zones"
> 
> v16:
> - update zoned_host device name to host_device [Stefan]
> - fix probing zoned device blocksizes [Stefan]
> - Use empty fields instead of changing struct size of BlkRwCo [Kevin, Stefan]
> 
> v15:
> - drop zoned_host_device BlockDriver
> - add zoned device option to host_device driver instead of introducing a new
>   zoned_host_device BlockDriver [Stefan]
> 
> v14:
> - address Stefan's comments of probing block sizes
> 
> v13:
> - add some tracing points for new zone APIs [Dmitry]
> - change error handling in zone_mgmt [Damien, Stefan]
> 
> v12:
> - address review comments
>   * drop BLK_ZO_RESET_ALL bit [Damien]
>   * fix error messages, style, and typos[Damien, Hannes]
> 
> v11:
> - address review comments
>   * fix possible BLKZONED config compiling warnings [Stefan]
>   * fix capacity field compiling warnings on older kernel [Stefan,Damien]
> 
> v10:
> - address review comments
>   * deal with the last small zone case in zone_mgmt operations [Damien]
>   * handle the capacity field outdated in old kernel(before 5.9) [Damien]
>   * use byte unit in block layer to be consistent with QEMU [Eric]
>   * fix coding style related problems [Stefan]
> 
> v9:
> - address review comments
>   * specify units of zone commands requests [Stefan]
>   * fix some error handling in file-posix [Stefan]
>   * introduce zoned_host_devcie in the commit message [Markus]
> 
> v8:
> - address review comments
>   * solve patch conflicts and merge sysfs helper funcations into one patch
>   * add cache.direct=on check in config
> 
> v7:
> - address review comments
>   * modify sysfs attribute helper funcations
>   * move the input validation and error checking into raw_co_zone_* function
>   * fix checks in config
> 
> v6:
> - drop virtio-blk emulation changes
> - address Stefan's review comments
>   * fix CONFIG_BLKZONED configs in related functions
>   * replace reading fd by g_file_get_contents() in get_sysfs_str_val()
>   * rewrite documentation for zoned storage
> 
> v5:
> - add zoned storage emulation to virtio-blk device
> - add documentation for zoned storage
> - address review comments
>   * fix qemu-iotests
>   * fix check to block layer
>   * modify interfaces of sysfs helper functions
>   * rename zoned device structs according to QEMU styles
>   * reorder patches
> 
> v4:
> - add virtio-blk headers for zoned device
> - add configurations for zoned host device
> - add zone operations for raw-format
> - address review comments
>   * fix memory leak bug in zone_report
>   * add checks to block layers
>   * fix qemu-iotests format
>   * fix sysfs helper functions
> 
> v3:
> - add helper functions to get sysfs attributes
> - address review comments
>   * fix zone report bugs
>   * fix the qemu-io code path
>   * use thread pool to avoid blocking ioctl() calls
> 
> v2:
> - add qemu-io sub-commands
> - address review comments
>   * modify interfaces of APIs
> 
> v1:
> - add block layer APIs resembling Linux ZoneBlockDevice ioctls
> 
> Sam Li (8):
>   include: add zoned device structs
>   file-posix: introduce helper functions for sysfs attributes
>   block: add block layer APIs resembling Linux ZonedBlockDevice ioctls
>   raw-format: add zone operations to pass through requests
>   config: add check to block layer
>   qemu-iotests: test new zone operations
>   

Re: [PATCH v2 2/2] tests/tcg/s390x: Add ex-relative-long.c

[Nina Schoetterl-Glausch]

On Wed, 2023-03-15 at 01:11 +0100, Ilya Leoshkevich wrote:
> > Test EXECUTE and EXECUTE RELATIVE LONG with relative long instructions
> > as targets.
> > 
> > Signed-off-by: Ilya Leoshkevich 

Reviewed-by: Nina Schoetterl-Glausch 

Some comments below.

> > ---
> >  tests/tcg/s390x/Makefile.target|   1 +
> >  tests/tcg/s390x/ex-relative-long.c | 159 +
> >  2 files changed, 160 insertions(+)
> >  create mode 100644 tests/tcg/s390x/ex-relative-long.c
> > 
> > diff --git a/tests/tcg/s390x/Makefile.target 
> > b/tests/tcg/s390x/Makefile.target
> > index cf93b966862..90bc48227db 100644
> > --- a/tests/tcg/s390x/Makefile.target
> > +++ b/tests/tcg/s390x/Makefile.target
> > @@ -29,6 +29,7 @@ TESTS+=clst
> >  TESTS+=long-double
> >  TESTS+=cdsg
> >  TESTS+=chrl
> > +TESTS+=ex-relative-long
> >  
> >  cdsg: CFLAGS+=-pthread
> >  cdsg: LDFLAGS+=-pthread
> > diff --git a/tests/tcg/s390x/ex-relative-long.c 
> > b/tests/tcg/s390x/ex-relative-long.c
> > new file mode 100644
> > index 000..4caa8c1b962
> > --- /dev/null
> > +++ b/tests/tcg/s390x/ex-relative-long.c
> > @@ -0,0 +1,159 @@
> > +/* Check EXECUTE with relative long instructions as targets. */
> > +#include 
> > +#include 
> > +
> > +struct test {
> > +const char *name;
> > +long (*func)(long reg, long *cc);
> > +long exp_reg;
> > +long exp_mem;
> > +long exp_cc;
> > +};
> > +
> > +/*
> > + * Each test sets the MEM_IDXth element of the mem array to MEM and uses a
> > + * single relative long instruction on it. The other elements remain zero.
> > + * This is in order to prevent stumbling upon MEM in random memory in case
> > + * there is an off-by-a-small-value bug.
> > + *
> > + * Note that while gcc supports the ZL constraint for relative long 
> > operands,
> > + * clang doesn't, so the assembly code accesses mem[MEM_IDX] using MEM_ASM.
> > + */
> > +long mem[0x1000];

This could be static, no?

> > +#define MEM_IDX 0x800
> > +#define MEM_ASM "mem+0x800*8"
> > +
> > +/* Initial %r2 value. */
> > +#define REG 0x1234567887654321
> > +
> > +/* Initial mem[MEM_IDX] value. */
> > +#define MEM 0xfedcba9889abcdef
> > +
> > +/* Initial cc value. */
> > +#define CC 0
> > +
> > +/* Relative long instructions and their expected effects. */
> > +#define FOR_EACH_INSN(F)   
> > \

You could define some macros and then calculate a bunch of values in the table, 
i.e.
#define SL(v) ((long)(v))
#define UL(v) ((unsigned long)(v))
#define SI(v, i) ((int)(v >> ((1 - i) * 32)))
#define UI(v, i) ((unsigned int)(v >> ((1 - i) * 32)))
#define SH(v, i) ((short)(v >> ((3 - i) * 16)))
#define UH(v, i) ((unsigned short)(v >> ((3 - i) * 16)))
#define CMP(f, s) ((f) == (s) ? 0 : ((f) < (s) ? 1 : 2 ))

F(cgfrl,  REG, MEM,CMP(SL(REG), SI(MEM, 0))

But everything checks out, so no need.

> > +F(cgfrl,  REG, MEM,2)  
> > \
> > +F(cghrl,  REG, MEM,2)  
> > \
> > +F(cgrl,   REG, MEM,2)  
> > \
> > +F(chrl,   REG, MEM,1)  
> > \
> > +F(clgfrl, REG, MEM,2)  
> > \
> > +F(clghrl, REG, MEM,2)  
> > \
> > +F(clgrl,  REG, MEM,1)  
> > \
> > +F(clhrl,  REG, MEM,2)  
> > \
> > +F(clrl,   REG, MEM,1)  
> > \
> > +F(crl,REG, MEM,1)  
> > \
> > +F(larl,   (long)[MEM_IDX], MEM,CC) 
> > \
> > +F(lgfrl,  0xfedcba98,  MEM,CC) 
> > \
> > +F(lghrl,  0xfedc,  MEM,CC) 
> > \
> > +F(lgrl,   MEM, MEM,CC) 
> > \
> > +F(lhrl,   0x12345678fedc,  MEM,CC) 
> > \
> > +F(llghrl, 0xfedc,  MEM,CC) 
> > \
> > +F(llhrl,  0x12345678fedc,  MEM,CC) 
> > \
> > +F(lrl,0x12345678fedcba98,  MEM,CC) 
> > \
> > +F(stgrl,  REG, REG,CC) 
> > \
> > +F(sthrl,  REG, 0x4321ba9889abcdef, CC) 
> > \
> > +F(strl,   REG, 0x8765432189abcdef, CC)
> > +
> > +/* Test functions. */
> > +#define DEFINE_EX_TEST(insn, exp_reg, exp_mem, exp_cc) 
> > \
> > +static long test_ex_ ## insn(long reg, long *cc)   
> > \
> > +

Re: [RFC 1/2] block: Split padded I/O vectors exceeding IOV_MAX

[Vladimir Sementsov-Ogievskiy]

On 15.03.23 15:13, Hanna Czenczek wrote:

When processing vectored guest requests that are not aligned to the
storage request alignment, we pad them by adding head and/or tail
buffers for a read-modify-write cycle.

The guest can submit I/O vectors up to IOV_MAX (1024) in length, but
with this padding, the vector can exceed that limit.  As of
4c002cef0e9abe7135d7916c51abce47f7fc1ee2 ("util/iov: make
qemu_iovec_init_extended() honest"), we refuse to pad vectors beyond the
limit, instead returning an error to the guest.

To the guest, this appears as a random I/O error.  We should not return
an I/O error to the guest when it issued a perfectly valid request.

Before 4c002cef0e9abe7135d7916c51abce47f7fc1ee2, we just made the vector
longer than IOV_MAX, which generally seems to work (because the guest
assumes a smaller alignment than we really have, file-posix's
raw_co_prw() will generally see bdrv_qiov_is_aligned() return false, and
so emulate the request, so that the IOV_MAX does not matter).  However,
that does not seem exactly great.

I see two ways to fix this problem:
1. We split such long requests into two requests.
2. We join some elements of the vector into new buffers to make it
shorter.

I am wary of (1), because it seems like it may have unintended side
effects.

(2) on the other hand seems relatively simple to implement, with
hopefully few side effects, so this patch does that.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2141964
Signed-off-by: Hanna Czenczek 
---
  block/io.c | 139 ++---
  util/iov.c |   4 --
  2 files changed, 133 insertions(+), 10 deletions(-)

diff --git a/block/io.c b/block/io.c
index 8974d46941..ee226d23d6 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1435,6 +1435,12 @@ out:
   * @merge_reads is true for small requests,
   * if @buf_len == @head + bytes + @tail. In this case it is possible that both
   * head and tail exist but @buf_len == align and @tail_buf == @buf.
+ *
+ * @write is true for write requests, false for read requests.
+ *
+ * If padding makes the vector too long (exceeding IOV_MAX), then we need to
+ * merge existing vector elements into a single one.  @collapse_buf acts as the
+ * bounce buffer in such cases.
   */
  typedef struct BdrvRequestPadding {
  uint8_t *buf;
@@ -1443,11 +1449,17 @@ typedef struct BdrvRequestPadding {
  size_t head;
  size_t tail;
  bool merge_reads;
+bool write;
  QEMUIOVector local_qiov;
+
+uint8_t *collapse_buf;
+size_t collapse_len;
+QEMUIOVector collapsed_qiov;
  } BdrvRequestPadding;
  
  static bool bdrv_init_padding(BlockDriverState *bs,

int64_t offset, int64_t bytes,
+  bool write,
BdrvRequestPadding *pad)
  {
  int64_t align = bs->bl.request_alignment;
@@ -1479,9 +1491,101 @@ static bool bdrv_init_padding(BlockDriverState *bs,
  pad->tail_buf = pad->buf + pad->buf_len - align;
  }
  
+pad->write = write;

+
  return true;
  }
  
+/*

+ * If padding has made the IOV (`pad->local_qiov`) too long (more than IOV_MAX
+ * elements), collapse some elements into a single one so that it adheres to 
the
+ * IOV_MAX limit again.
+ *
+ * If collapsing, `pad->collapse_buf` will be used as a bounce buffer of length
+ * `pad->collapse_len`.  `pad->collapsed_qiov` will contain the previous 
entries
+ * (before collapsing), so that bdrv_padding_destroy() can copy the bounce
+ * buffer content back for read requests.
+ *
+ * Note that we will not touch the padding head or tail entries here.  We 
cannot
+ * move them to a bounce buffer, because for RMWs, both head and tail expect to
+ * be in an aligned buffer with scratch space after (head) or before (tail) to
+ * perform the read into (because the whole buffer must be aligned, but head's
+ * and tail's lengths naturally cannot be aligned, because they provide padding
+ * for unaligned requests).  A collapsed bounce buffer for multiple IOV 
elements
+ * cannot provide such scratch space.
+ *
+ * Therefore, this function collapses the first IOV elements after the
+ * (potential) head element.
+ */
+static void bdrv_padding_collapse(BdrvRequestPadding *pad, BlockDriverState 
*bs)
+{
+int surplus_count, collapse_count;
+struct iovec *collapse_iovs;
+QEMUIOVector collapse_qiov;
+size_t move_count;
+
+surplus_count = pad->local_qiov.niov - IOV_MAX;
+/* Not exceeding the limit?  Nothing to collapse. */
+if (surplus_count <= 0) {
+return;
+}
+
+/*
+ * Only head and tail can have lead to the number of entries exceeding
+ * IOV_MAX, so we can exceed it by the head and tail at most
+ */
+assert(surplus_count <= !!pad->head + !!pad->tail);
+
+/*
+ * We merge (collapse) `surplus_count` entries into the first entry that is
+ * not padding, i.e. we merge `surplus_count + 1` entries into entry 0 if
+ * there is no 

RE: [PATCH v5 04/16] hw/9pfs: Implement Windows specific xxxdir() APIs

[Shi, Guohuai]

> -Original Message-
> From: Christian Schoenebeck 
> Sent: Thursday, March 16, 2023 19:05
> To: Greg Kurz ; qemu-devel@nongnu.org
> Cc: Meng, Bin ; Shi, Guohuai
> 
> Subject: Re: [PATCH v5 04/16] hw/9pfs: Implement Windows specific xxxdir()
> APIs
> 
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and
> know the content is safe.
> 
> On Wednesday, March 15, 2023 8:05:34 PM CET Shi, Guohuai wrote:
> >
> > > -Original Message-
> > > From: Christian Schoenebeck 
> > > Sent: Wednesday, March 15, 2023 00:06
> > > To: Greg Kurz ; qemu-devel@nongnu.org
> > > Cc: Shi, Guohuai ; Meng, Bin
> > > 
> > > Subject: Re: [PATCH v5 04/16] hw/9pfs: Implement Windows specific
> > > xxxdir() APIs
> > >
> > > CAUTION: This email comes from a non Wind River email account!
> > > Do not click links or open attachments unless you recognize the
> > > sender and know the content is safe.
> > >
> > > On Monday, February 20, 2023 11:08:03 AM CET Bin Meng wrote:
> > > > From: Guohuai Shi 
> > > >
> > > > This commit implements Windows specific xxxdir() APIs for safety
> > > > directory access.
> > >
> > > That comment is seriously too short for this patch.
> > >
> > > 1. You should describe the behaviour implementation that you have
> > > chosen and why you have chosen it.
> > >
> > > 2. Like already said in the previous version of the patch, you
> > > should place a link to the discussion we had on this issue.
> > >
> > > > Signed-off-by: Guohuai Shi 
> > > > Signed-off-by: Bin Meng 
> > > > ---
> > > >
> > > >  hw/9pfs/9p-util.h   |   6 +
> > > >  hw/9pfs/9p-util-win32.c | 443
> > > > 
> > > >  2 files changed, 449 insertions(+)
> > > >
> > > > diff --git a/hw/9pfs/9p-util.h b/hw/9pfs/9p-util.h index
> > > > 0f159fb4ce..c1c251fbd1 100644
> > > > --- a/hw/9pfs/9p-util.h
> > > > +++ b/hw/9pfs/9p-util.h
> > > > @@ -141,6 +141,12 @@ int unlinkat_win32(int dirfd, const char
> > > > *pathname, int flags);  int statfs_win32(const char *root_path,
> > > > struct statfs *stbuf);  int openat_dir(int dirfd, const char
> > > > *name);  int openat_file(int dirfd, const char *name, int flags,
> > > > mode_t mode);
> > > > +DIR *opendir_win32(const char *full_file_name); int
> > > > +closedir_win32(DIR *pDir); struct dirent *readdir_win32(DIR
> > > > +*pDir); void rewinddir_win32(DIR *pDir); void seekdir_win32(DIR
> > > > +*pDir, long pos); long telldir_win32(DIR *pDir);
> > > >  #endif
> > > >
> > > >  static inline void close_preserve_errno(int fd) diff --git
> > > > a/hw/9pfs/9p-util-win32.c b/hw/9pfs/9p-util-win32.c index
> > > > a99d579a06..e9408f3c45 100644
> > > > --- a/hw/9pfs/9p-util-win32.c
> > > > +++ b/hw/9pfs/9p-util-win32.c
> > > > @@ -37,6 +37,16 @@
> > > >   *Windows does not support opendir, the directory fd is created by
> > > >   *CreateFile and convert to fd by _open_osfhandle(). Keep the fd
> open
> > > will
> > > >   *lock and protect the directory (can not be modified or replaced)
> > > > + *
> > > > + * 5. Neither Windows native APIs, nor MinGW provide a POSIX
> > > > + compatible
> > > API for
> > > > + *acquiring directory entries in a safe way. Calling those APIs
> > > (native
> > > > + *_findfirst() and _findnext() or MinGW's readdir(), seekdir() and
> > > > + *telldir()) directly can lead to an inconsistent state if
> directory
> > > is
> > > > + *modified in between, e.g. the same directory appearing more than
> > > once
> > > > + *in output, or directories not appearing at all in output even
> though
> > > they
> > > > + *were neither newly created nor deleted. POSIX does not define
> what
> > > happens
> > > > + *with deleted or newly created directories in between, but it
> > > guarantees a
> > > > + *consistent state.
> > > >   */
> > > >
> > > >  #include "qemu/osdep.h"
> > > > @@ -51,6 +61,25 @@
> > > >
> > > >  #define V9FS_MAGIC  0x53465039  /* string "9PFS" */
> > > >
> > > > +/*
> > > > + * MinGW and Windows does not provide a safe way to seek
> > > > +directory while other
> > > > + * thread is modifying the same directory.
> > > > + *
> > > > + * This structure is used to store sorted file id and ensure
> > > > +directory seek
> > > > + * consistency.
> > > > + */
> > > > +struct dir_win32 {
> > > > +struct dirent dd_dir;
> > > > +uint32_t offset;
> > > > +uint32_t total_entries;
> > > > +HANDLE hDir;
> > > > +uint32_t dir_name_len;
> > > > +uint64_t dot_id;
> > > > +uint64_t dot_dot_id;
> > > > +uint64_t *file_id_list;
> > > > +char dd_name[1];
> > > > +};
> > > > +
> > > >  /*
> > > >   * win32_error_to_posix - convert Win32 error to POSIX error number
> > > >   *
> > > > @@ -977,3 +1006,417 @@ int qemu_mknodat(int dirfd, const char
> > > > *filename,
> > > mode_t mode, dev_t dev)
> > > >  errno = ENOTSUP;
> > > >  return -1;
> > > >  }
> > > > +
> > > > +static int 

[PATCH v3 1/2] target/s390x: Fix R[NOX]SBG with T=1

[Ilya Leoshkevich]

RXSBG usage in the "filetests" test from the wasmtime testsuite makes
tcg_reg_alloc_op() attempt to temp_load() a TEMP_VAL_DEAD temporary,
causing an assertion failure:

0x01000a70:  ec14 b040 3057  rxsbg%r1, %r4, 0xb0, 0x40, 0x30

OP after optimization and liveness analysis:
  01000a70 0004 0006
 rotl_i64 tmp2,r4,$0x30   dead: 1 2  pref=0x
 and_i64 tmp2,tmp2,$0x8000dead: 1  pref=0x
[xor_i64 tmp3,tmp3,tmp2   dead: 1 2  pref=0x]
 and_i64 cc_dst,tmp3,$0x8000  sync: 0  dead: 0 1 2  pref=0x
 mov_i64 psw_addr,$0x1000a76  sync: 0  dead: 0 1  pref=0x
 mov_i32 cc_op,$0x6   sync: 0  dead: 0 1  pref=0x
 call lookup_tb_ptr,$0x6,$1,tmp8,env  dead: 1  pref=none
 goto_ptr tmp8dead: 0
 set_label $L0
 exit_tb $0x7fffe809d183

../tcg/tcg.c:3865: tcg fatal error

The reason is that tmp3 does not have an initial value, which confuses
the register allocator. This also affects the correctness of the
results.

Fix by assigning R1 to it.

Exposed by commit e2e641fa3d5 ("tcg: Change default temp lifetime to
TEMP_TB").

Fixes: d6c6372e186e ("target-s390: Implement R[NOX]SBG")
Reviewed-by: David Hildenbrand 
Signed-off-by: Ilya Leoshkevich 
---
 target/s390x/tcg/translate.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 14c3896d529..0fb36e04be8 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -3695,11 +3695,15 @@ static DisasJumpType op_rosbg(DisasContext *s, DisasOps 
*o)
 int i3 = get_field(s, i3);
 int i4 = get_field(s, i4);
 int i5 = get_field(s, i5);
+TCGv_i64 orig_out;
 uint64_t mask;
 
 /* If this is a test-only form, arrange to discard the result.  */
 if (i3 & 0x80) {
+tcg_debug_assert(o->out != NULL);
+orig_out = o->out;
 o->out = tcg_temp_new_i64();
+tcg_gen_mov_i64(o->out, orig_out);
 }
 
 i3 &= 63;
-- 
2.39.2

[PATCH v3 2/2] tests/tcg/s390x: Add rxsbg.c

[Ilya Leoshkevich]

Add a small test for RXSBG with T=1 to prevent regressions.

Signed-off-by: Ilya Leoshkevich 
---
 tests/tcg/s390x/Makefile.target |  3 +++
 tests/tcg/s390x/rxsbg.c | 46 +
 2 files changed, 49 insertions(+)
 create mode 100644 tests/tcg/s390x/rxsbg.c

diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index cf93b966862..3c940ac952e 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -29,10 +29,13 @@ TESTS+=clst
 TESTS+=long-double
 TESTS+=cdsg
 TESTS+=chrl
+TESTS+=rxsbg
 
 cdsg: CFLAGS+=-pthread
 cdsg: LDFLAGS+=-pthread
 
+rxsbg: CFLAGS+=-O2
+
 Z13_TESTS=vistr
 $(Z13_TESTS): CFLAGS+=-march=z13 -O2
 TESTS+=$(Z13_TESTS)
diff --git a/tests/tcg/s390x/rxsbg.c b/tests/tcg/s390x/rxsbg.c
new file mode 100644
index 000..4b155db304e
--- /dev/null
+++ b/tests/tcg/s390x/rxsbg.c
@@ -0,0 +1,46 @@
+/*
+ * Test the RXSBG instruction.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include 
+#include 
+
+static inline __attribute__((__always_inline__)) void
+rxsbg(unsigned long *r1, unsigned long r2, int i3, int i4, int i5, int *cc)
+{
+asm("rxsbg %[r1],%[r2],%[i3],%[i4],%[i5]\n"
+"ipm %[cc]"
+: [r1] "+r" (*r1), [cc] "=r" (*cc)
+: [r2] "r" (r2) , [i3] "i" (i3) , [i4] "i" (i4) , [i5] "i" (i5)
+: "cc");
+*cc = (*cc >> 28) & 3;
+}
+
+void test_cc0(void)
+{
+unsigned long r1 = 6;
+int cc;
+
+rxsbg(, 3, 61 | 0x80, 62, 1, );
+assert(r1 == 6);
+assert(cc == 0);
+}
+
+void test_cc1(void)
+{
+unsigned long r1 = 2;
+int cc;
+
+rxsbg(, 3, 61 | 0x80, 62, 1, );
+assert(r1 == 2);
+assert(cc == 1);
+}
+
+int main(void)
+{
+test_cc0();
+test_cc1();
+
+return EXIT_SUCCESS;
+}
-- 
2.39.2

[PATCH v3 0/2] target/s390x: Fix R[NOX]SBG with T=1

[Ilya Leoshkevich]

v2: https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg04699.html
v2 -> v3: Assert that o->out != NULL, mention the commit that exposed
  the problem (Philippe).

v1: https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg04493.html
v1 -> v2: Work around a clang issue (Thomas).
  Add cc=0 test, use more human-friendly constants.

Hi,

This series fixes ROTATE THEN  SELECTED BITS when
test-results control is on. The problem is the incorrect translation,
which confuses the register allocator.

Patch 1 is the fix, patch 2 adds a test.

Best regards,
Ilya

Ilya Leoshkevich (2):
  target/s390x: Fix R[NOX]SBG with T=1
  tests/tcg/s390x: Add rxsbg.c

 target/s390x/tcg/translate.c|  4 +++
 tests/tcg/s390x/Makefile.target |  3 +++
 tests/tcg/s390x/rxsbg.c | 46 +
 3 files changed, 53 insertions(+)
 create mode 100644 tests/tcg/s390x/rxsbg.c

-- 
2.39.2

Re: [PATCH v2 1/2] target/s390x: Fix R[NOX]SBG with T=1

[Ilya Leoshkevich]

On Thu, 2023-03-16 at 09:41 +0100, Philippe Mathieu-Daudé wrote:
> On 16/3/23 00:56, Ilya Leoshkevich wrote:
> > RXSBG usage in the "filetests" test from the wasmtime testsuite
> > makes
> > tcg_reg_alloc_op() attempt to temp_load() a TEMP_VAL_DEAD
> > temporary,
> > causing an assertion failure:
> > 
> >  0x01000a70:  ec14 b040 3057  rxsbg    %r1, %r4, 0xb0, 0x40,
> > 0x30
> > 
> >  OP after optimization and liveness analysis:
> >    01000a70 0004 0006
> >   rotl_i64 tmp2,r4,$0x30   dead: 1 2 
> > pref=0x
> >   and_i64 tmp2,tmp2,$0x8000    dead: 1  pref=0x
> >  [xor_i64 tmp3,tmp3,tmp2   dead: 1 2 
> > pref=0x]
> >   and_i64 cc_dst,tmp3,$0x8000  sync: 0  dead: 0 1
> > 2  pref=0x
> >   mov_i64 psw_addr,$0x1000a76  sync: 0  dead: 0 1 
> > pref=0x
> >   mov_i32 cc_op,$0x6   sync: 0  dead: 0 1 
> > pref=0x
> >   call lookup_tb_ptr,$0x6,$1,tmp8,env  dead: 1  pref=none
> >   goto_ptr tmp8    dead: 0
> >   set_label $L0
> >   exit_tb $0x7fffe809d183
> > 
> >  ../tcg/tcg.c:3865: tcg fatal error
> > 
> > The reason is that tmp3 does not have an initial value, which
> > confuses
> > the register allocator. This also affects the correctness of the
> > results.
> > 
> > Fix by assigning R1 to it.
> > 
> > Fixes: d6c6372e186e ("target-s390: Implement R[NOX]SBG")
> 
> Exposed by 3ac6f91bca..dd161de75f?

Bisect points to:

commit e2e641fa3d5e730f128562d6901dcc729c9bf8a0
Author: Richard Henderson 
Date:   Sun Jan 29 14:09:00 2023 -1000

tcg: Change default temp lifetime to TEMP_TB

I will mention this.

> 3ac6f91bca target/s390x: Drop tcg_temp_free from translate.c
> dd161de75f target/s390x: Remove g_out, g_out2, g_in1, g_in2
> 
> > Reviewed-by: David Hildenbrand 
> > Signed-off-by: Ilya Leoshkevich 
> > ---
> >   target/s390x/tcg/translate.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/target/s390x/tcg/translate.c
> > b/target/s390x/tcg/translate.c
> > index 14c3896d529..6dd2f41ad08 100644
> > --- a/target/s390x/tcg/translate.c
> > +++ b/target/s390x/tcg/translate.c
> > @@ -3696,10 +3696,13 @@ static DisasJumpType op_rosbg(DisasContext
> > *s, DisasOps *o)
> >   int i4 = get_field(s, i4);
> >   int i5 = get_field(s, i5);
> >   uint64_t mask;
> > +    TCGv_i64 tmp;
> >   
> >   /* If this is a test-only form, arrange to discard the
> > result.  */
> >   if (i3 & 0x80) {
> 
>    tcg_debug_assert(o->out != NULL); ?

Ok, I will add this.

> 
> > +    tmp = o->out;
> >   o->out = tcg_temp_new_i64();
> > +    tcg_gen_mov_i64(o->out, tmp);
> 
> Something bugs me with this pattern but I can't say why yet :(

Please let me know once you come up with something.
I will do s/tmp/orig_out/ send a v3 in the meantime.

> >   }
> >   
> >   i3 &= 63;

Re: [PATCH v3 1/5] hw/usb: Add basic i.MX USB Phy support

[Guenter Roeck]

On Thu, Mar 16, 2023 at 02:51:23PM +, Peter Maydell wrote:
> On Thu, 16 Mar 2023 at 14:12, Guenter Roeck  wrote:
> >
> > Hi Peter,
> >
> > On 3/16/23 06:41, Peter Maydell wrote:
> > > On Fri, 13 Mar 2020 at 01:45, Guenter Roeck  wrote:
> > >>
> > >> Add basic USB PHY support as implemented in i.MX23, i.MX28, i.MX6,
> > >> and i.MX7 SoCs.
> > >>
> > >> The only support really needed - at least to boot Linux - is support
> > >> for soft reset, which needs to reset various registers to their initial
> > >> value. Otherwise, just record register values.
> > >>
> > >> Reviewed-by: Peter Maydell 
> > >> Signed-off-by: Guenter Roeck 
> > >
> > > Hi Guenter; we've had a fuzzer report that this device model
> > > accesses off the end of the usbphy[] array:
> > > https://gitlab.com/qemu-project/qemu/-/issues/1408
> > >
> >
> > Good catch. And an obvious bug, sorry.
> 
> 
> >
> > > Do you know what the device is supposed to do with these
> > > off-the-end acceses? We could either reduce the memory region
> > > size or bounds check and RAZ/WI the out-of-range accesses.
> > >
> >
> > I have no idea what the real hardware would do. The datasheets (at
> > least the ones I checked) don't say, only that the region size is 4k.
> > I would suggest a bounds check, ignore out-of-bounds writes (maybe
> > with a log message), and return 0 for reads (which I think is what
> > you suggest with RAZ/WI).
> >
> > Want me to send a patch ?
> 
> If you have the time, that would be great. I expect you're
> better set up to test it than I am...
> 

I prepared a patch. Currently testing.

Guenter

Re: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor

[Juan Quintela]

Akihiko Odaki  wrote:
> The current implementation of igb uses only part of a advanced Tx
> context descriptor because it misses some features and sniffs the trait
> of the packet instead of respecting the packet type specified in the
> descriptor. However, we will certainly need the entire Tx context
> descriptor when we update igb to respect these ignored fields. Save the
> entire Tx context descriptor to prepare for such a change.
>
> Signed-off-by: Akihiko Odaki 

Reviewed-by: Juan Quintela 

Re: [PATCH v2 09/32] include/exec: fix kerneldoc definition

[Peter Maydell]

On Wed, 15 Mar 2023 at 17:49, Alex Bennée  wrote:
>
> The kerneldoc processor complains about the mismatched variable name.
> Fix it.
>
> Message-Id: <20230310103123.2118519-11-alex.ben...@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé 
> Signed-off-by: Alex Bennée 

Note that Laurent has picked up a different variant of this
fix into the -trivial tree...

-- PMM

Re: [PATCH v2 0/3] contrib/elf2dmp: Windows Server 2022 support

[Viktor Prutyanov]

> Hi,
> 
> For now, elf2dmp is unable to convert ELF-dump to DMP-dump made of
> Windows Server 2022 guest. This patch series fixes it.
> 
> v1: improve code-style fix
> v2: don't remove data directory entry RVA print and DOS header size check
> 
> Viktor Prutyanov (3):
> contrib/elf2dmp: fix code style
> contrib/elf2dmp: move PE dir search to pe_get_data_dir_entry
> contrib/elf2dmp: add PE name check and Windows Server 2022 support
> 
> contrib/elf2dmp/addrspace.c | 1 +
> contrib/elf2dmp/main.c | 108 ++---
> contrib/elf2dmp/pe.h | 115 
> 3 files changed, 140 insertions(+), 84 deletions(-)
> 
> --
> 2.35.1

Hi Peter,

As we discussed, I would like to ask you to pick up this series.

Thanks,
Viktor Prutyanov

[PATCH v4 08/12] target/s390x: Handle CLRL and CLGFRL with non-aligned addresses

[Ilya Leoshkevich]

Use MO_ALIGN and let do_unaligned_access() generate a specification
exception.

Reported-by: Nina Schoetterl-Glausch 
Suggested-by: Nina Schoetterl-Glausch 
Reviewed-by: Richard Henderson 
Signed-off-by: Ilya Leoshkevich 
---
 target/s390x/tcg/translate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index d3b8126d8c6..c67f8440db8 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -5866,7 +5866,8 @@ static void in2_mri2_32s(DisasContext *s, DisasOps *o)
 static void in2_mri2_32u(DisasContext *s, DisasOps *o)
 {
 o->in2 = tcg_temp_new_i64();
-tcg_gen_qemu_ld32u(o->in2, gen_ri2(s), get_mem_index(s));
+tcg_gen_qemu_ld_tl(o->in2, gen_ri2(s), get_mem_index(s),
+   MO_TEUL | MO_ALIGN);
 }
 #define SPEC_in2_mri2_32u 0
 
-- 
2.39.2

[PATCH v4 05/12] target/s390x: Handle LLGFRL from non-aligned addresses

[Ilya Leoshkevich]

Use MO_ALIGN and let do_unaligned_access() generate a specification
exception.

Reported-by: Nina Schoetterl-Glausch 
Suggested-by: Nina Schoetterl-Glausch 
Reviewed-by: Richard Henderson 
Signed-off-by: Ilya Leoshkevich 
---
 target/s390x/tcg/insn-data.h.inc | 6 +++---
 target/s390x/tcg/translate.c | 3 ++-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/s390x/tcg/insn-data.h.inc b/target/s390x/tcg/insn-data.h.inc
index 5aff4c0873a..3abd2dbedd5 100644
--- a/target/s390x/tcg/insn-data.h.inc
+++ b/target/s390x/tcg/insn-data.h.inc
@@ -502,16 +502,16 @@
 C(0xc405, LHRL,RIL_b, GIE, 0, ri2, new, r1_32, ld16s, 0)
 C(0xc404, LGHRL,   RIL_b, GIE, 0, ri2, r1, 0, ld16s, 0)
 /* LOAD HIGH */
-C(0xe3ca, LFH, RXY_a, HW,  0, a2, new, r1_32h, ld32u, 0)
+D(0xe3ca, LFH, RXY_a, HW,  0, a2, new, r1_32h, ld32u, 0, 0)
 /* LOAG HIGH AND TRAP */
 C(0xe3c8, LFHAT,   RXY_a, LAT, 0, m2_32u, r1, 0, lfhat, 0)
 /* LOAD LOGICAL */
 C(0xb916, LLGFR,   RRE,   Z,   0, r2_32u, 0, r1, mov2, 0)
-C(0xe316, LLGF,RXY_a, Z,   0, a2, r1, 0, ld32u, 0)
+D(0xe316, LLGF,RXY_a, Z,   0, a2, r1, 0, ld32u, 0, 0)
 /* LOAD LOGICAL AND TRAP */
 C(0xe39d, LLGFAT,  RXY_a, LAT, 0, a2, r1, 0, llgfat, 0)
 /* LOAD LOGICAL RELATIVE LONG */
-C(0xc40e, LLGFRL,  RIL_b, GIE, 0, ri2, r1, 0, ld32u, 0)
+D(0xc40e, LLGFRL,  RIL_b, GIE, 0, ri2, r1, 0, ld32u, 0, MO_ALIGN)
 /* LOAD LOGICAL CHARACTER */
 C(0xb994, LLCR,RRE,   EI,  0, r2_8u, 0, r1_32, mov2, 0)
 C(0xb984, LLGCR,   RRE,   EI,  0, r2_8u, 0, r1, mov2, 0)
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index f89e1ce353b..1f459f0f2bd 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -2777,7 +2777,8 @@ static DisasJumpType op_ld32s(DisasContext *s, DisasOps 
*o)
 
 static DisasJumpType op_ld32u(DisasContext *s, DisasOps *o)
 {
-tcg_gen_qemu_ld32u(o->out, o->in2, get_mem_index(s));
+tcg_gen_qemu_ld_tl(o->out, o->in2, get_mem_index(s),
+   MO_TEUL | s->insn->data);
 return DISAS_NEXT;
 }
 
-- 
2.39.2

[PATCH v4 07/12] target/s390x: Handle CGRL and CLGRL with non-aligned addresses

[Ilya Leoshkevich]

Use MO_ALIGN and let do_unaligned_access() generate a specification
exception.

Reported-by: Nina Schoetterl-Glausch 
Suggested-by: Nina Schoetterl-Glausch 
Reviewed-by: Richard Henderson 
Signed-off-by: Ilya Leoshkevich 
---
 target/s390x/tcg/translate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 983bb4edc39..d3b8126d8c6 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -5873,7 +5873,8 @@ static void in2_mri2_32u(DisasContext *s, DisasOps *o)
 static void in2_mri2_64(DisasContext *s, DisasOps *o)
 {
 o->in2 = tcg_temp_new_i64();
-tcg_gen_qemu_ld64(o->in2, gen_ri2(s), get_mem_index(s));
+tcg_gen_qemu_ld_i64(o->in2, gen_ri2(s), get_mem_index(s),
+MO_TEUQ | MO_ALIGN);
 }
 #define SPEC_in2_mri2_64 0
 
-- 
2.39.2

[PATCH v4 06/12] target/s390x: Handle CRL and CGFRL with non-aligned addresses

[Ilya Leoshkevich]

Use MO_ALIGN and let do_unaligned_access() generate a specification
exception.

Reported-by: Nina Schoetterl-Glausch 
Suggested-by: Nina Schoetterl-Glausch 
Reviewed-by: Richard Henderson 
Signed-off-by: Ilya Leoshkevich 
---
 target/s390x/tcg/translate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 1f459f0f2bd..983bb4edc39 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -5858,7 +5858,8 @@ static void in2_mri2_16u(DisasContext *s, DisasOps *o)
 static void in2_mri2_32s(DisasContext *s, DisasOps *o)
 {
 o->in2 = tcg_temp_new_i64();
-tcg_gen_qemu_ld32s(o->in2, gen_ri2(s), get_mem_index(s));
+tcg_gen_qemu_ld_tl(o->in2, gen_ri2(s), get_mem_index(s),
+   MO_TESL | MO_ALIGN);
 }
 #define SPEC_in2_mri2_32s 0
 
-- 
2.39.2

[PATCH v4 11/12] target/s390x: Update do_unaligned_access() comment

[Ilya Leoshkevich]

Relative long instructions now depend on do_unaligned_access() too.

Reviewed-by: Richard Henderson 
Signed-off-by: Ilya Leoshkevich 
---
 target/s390x/tcg/excp_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c
index bc767f04438..cafdef77234 100644
--- a/target/s390x/tcg/excp_helper.c
+++ b/target/s390x/tcg/excp_helper.c
@@ -85,8 +85,8 @@ void HELPER(data_exception)(CPUS390XState *env, uint32_t dxc)
 
 /*
  * Unaligned accesses are only diagnosed with MO_ALIGN.  At the moment,
- * this is only for the atomic operations, for which we want to raise a
- * specification exception.
+ * this is only for the atomic and relative long operations, for which we want
+ * to raise a specification exception.
  */
 static G_NORETURN
 void do_unaligned_access(CPUState *cs, uintptr_t retaddr)
-- 
2.39.2

[PATCH v4 00/12] target/s390x: Handle unaligned accesses

[Ilya Leoshkevich]

v3: https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg04687.html
v3 -> v4: Get rid of the preprocessor magic in the new tests (Thomas).

v2: https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg04231.html
v2 -> v3: Fix clang build (Thomas).

v1: https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg03821.html
v1 -> v2: Use MO_ALIGN (Richard).

Patches that need review:
- [PATCH 12/12] tests/tcg/s390x: Test unaligned accesses

Hi,

This series makes accessing unaligned addresses with branching, LPSWE,
EXECUTE and relative long instructions fail with a specification
exception instead of succeeding.

Patches 1-10 are fixes, patch 11 adjusts a comment to reflect a change
done by fixes, patch 12 adds a number of softmmu and user tests.

Best regards,
Ilya

Ilya Leoshkevich (12):
  target/s390x: Handle branching to odd addresses
  target/s390x: Handle EXECUTE of odd addresses
  target/s390x: Handle LGRL from non-aligned addresses
  target/s390x: Handle LRL and LGFRL from non-aligned addresses
  target/s390x: Handle LLGFRL from non-aligned addresses
  target/s390x: Handle CRL and CGFRL with non-aligned addresses
  target/s390x: Handle CGRL and CLGRL with non-aligned addresses
  target/s390x: Handle CLRL and CLGFRL with non-aligned addresses
  target/s390x: Handle STRL to non-aligned addresses
  target/s390x: Handle STGRL to non-aligned addresses
  target/s390x: Update do_unaligned_access() comment
  tests/tcg/s390x: Test unaligned accesses

 target/s390x/cpu.h  |  9 
 target/s390x/tcg/excp_helper.c  |  4 +-
 target/s390x/tcg/insn-data.h.inc| 46 ++---
 target/s390x/tcg/mem_helper.c   | 12 +-
 target/s390x/tcg/translate.c| 24 +++
 tests/tcg/s390x/Makefile.softmmu-target | 15 +--
 tests/tcg/s390x/Makefile.target |  8 
 tests/tcg/s390x/br-odd.S| 16 +++
 tests/tcg/s390x/cgrl-unaligned.S| 16 +++
 tests/tcg/s390x/clrl-unaligned.S| 16 +++
 tests/tcg/s390x/crl-unaligned.S | 16 +++
 tests/tcg/s390x/ex-odd.S| 17 
 tests/tcg/s390x/lgrl-unaligned.S| 16 +++
 tests/tcg/s390x/llgfrl-unaligned.S  | 16 +++
 tests/tcg/s390x/lpswe-unaligned.S   | 18 
 tests/tcg/s390x/lrl-unaligned.S | 16 +++
 tests/tcg/s390x/pgm-specification-softmmu.S | 40 ++
 tests/tcg/s390x/pgm-specification-user.c| 37 +
 tests/tcg/s390x/pgm-specification.mak   | 15 +++
 tests/tcg/s390x/softmmu.ld  | 20 +
 tests/tcg/s390x/stgrl-unaligned.S   | 16 +++
 tests/tcg/s390x/strl-unaligned.S| 16 +++
 22 files changed, 371 insertions(+), 38 deletions(-)
 create mode 100644 tests/tcg/s390x/br-odd.S
 create mode 100644 tests/tcg/s390x/cgrl-unaligned.S
 create mode 100644 tests/tcg/s390x/clrl-unaligned.S
 create mode 100644 tests/tcg/s390x/crl-unaligned.S
 create mode 100644 tests/tcg/s390x/ex-odd.S
 create mode 100644 tests/tcg/s390x/lgrl-unaligned.S
 create mode 100644 tests/tcg/s390x/llgfrl-unaligned.S
 create mode 100644 tests/tcg/s390x/lpswe-unaligned.S
 create mode 100644 tests/tcg/s390x/lrl-unaligned.S
 create mode 100644 tests/tcg/s390x/pgm-specification-softmmu.S
 create mode 100644 tests/tcg/s390x/pgm-specification-user.c
 create mode 100644 tests/tcg/s390x/pgm-specification.mak
 create mode 100644 tests/tcg/s390x/softmmu.ld
 create mode 100644 tests/tcg/s390x/stgrl-unaligned.S
 create mode 100644 tests/tcg/s390x/strl-unaligned.S

-- 
2.39.2

[PATCH v4 04/12] target/s390x: Handle LRL and LGFRL from non-aligned addresses

[Ilya Leoshkevich]

Use MO_ALIGN and let do_unaligned_access() generate a specification
exception.

Reported-by: Nina Schoetterl-Glausch 
Suggested-by: Nina Schoetterl-Glausch 
Reviewed-by: Richard Henderson 
Signed-off-by: Ilya Leoshkevich 
---
 target/s390x/tcg/insn-data.h.inc | 14 +++---
 target/s390x/tcg/translate.c |  3 ++-
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/target/s390x/tcg/insn-data.h.inc b/target/s390x/tcg/insn-data.h.inc
index d439d803509..5aff4c0873a 100644
--- a/target/s390x/tcg/insn-data.h.inc
+++ b/target/s390x/tcg/insn-data.h.inc
@@ -410,12 +410,12 @@
 
 /* LOAD */
 C(0x1800, LR,  RR_a,  Z,   0, r2_o, 0, cond_r1r2_32, mov2, 0)
-C(0x5800, L,   RX_a,  Z,   0, a2, new, r1_32, ld32s, 0)
-C(0xe358, LY,  RXY_a, LD,  0, a2, new, r1_32, ld32s, 0)
+D(0x5800, L,   RX_a,  Z,   0, a2, new, r1_32, ld32s, 0, 0)
+D(0xe358, LY,  RXY_a, LD,  0, a2, new, r1_32, ld32s, 0, 0)
 C(0xb904, LGR, RRE,   Z,   0, r2_o, 0, r1, mov2, 0)
 C(0xb914, LGFR,RRE,   Z,   0, r2_32s, 0, r1, mov2, 0)
 D(0xe304, LG,  RXY_a, Z,   0, a2, r1, 0, ld64, 0, 0)
-C(0xe314, LGF, RXY_a, Z,   0, a2, r1, 0, ld32s, 0)
+D(0xe314, LGF, RXY_a, Z,   0, a2, r1, 0, ld32s, 0, 0)
 F(0x2800, LDR, RR_a,  Z,   0, f2, 0, f1, mov2, 0, IF_AFP1 | IF_AFP2)
 F(0x6800, LD,  RX_a,  Z,   0, m2_64, 0, f1, mov2, 0, IF_AFP1)
 F(0xed65, LDY, RXY_a, LD,  0, m2_64, 0, f1, mov2, 0, IF_AFP1)
@@ -426,9 +426,9 @@
 /* LOAD IMMEDIATE */
 C(0xc001, LGFI,RIL_a, EI,  0, i2, 0, r1, mov2, 0)
 /* LOAD RELATIVE LONG */
-C(0xc40d, LRL, RIL_b, GIE, 0, ri2, new, r1_32, ld32s, 0)
+D(0xc40d, LRL, RIL_b, GIE, 0, ri2, new, r1_32, ld32s, 0, MO_ALIGN)
 D(0xc408, LGRL,RIL_b, GIE, 0, ri2, r1, 0, ld64, 0, MO_ALIGN)
-C(0xc40c, LGFRL,   RIL_b, GIE, 0, ri2, r1, 0, ld32s, 0)
+D(0xc40c, LGFRL,   RIL_b, GIE, 0, ri2, r1, 0, ld32s, 0, MO_ALIGN)
 /* LOAD ADDRESS */
 C(0x4100, LA,  RX_a,  Z,   0, a2, 0, r1, mov2, 0)
 C(0xe371, LAY, RXY_a, LD,  0, a2, 0, r1, mov2, 0)
@@ -456,9 +456,9 @@
 C(0x1200, LTR, RR_a,  Z,   0, r2_o, 0, cond_r1r2_32, mov2, s32)
 C(0xb902, LTGR,RRE,   Z,   0, r2_o, 0, r1, mov2, s64)
 C(0xb912, LTGFR,   RRE,   Z,   0, r2_32s, 0, r1, mov2, s64)
-C(0xe312, LT,  RXY_a, EI,  0, a2, new, r1_32, ld32s, s64)
+D(0xe312, LT,  RXY_a, EI,  0, a2, new, r1_32, ld32s, s64, 0)
 D(0xe302, LTG, RXY_a, EI,  0, a2, r1, 0, ld64, s64, 0)
-C(0xe332, LTGF,RXY_a, GIE, 0, a2, r1, 0, ld32s, s64)
+D(0xe332, LTGF,RXY_a, GIE, 0, a2, r1, 0, ld32s, s64, 0)
 F(0xb302, LTEBR,   RRE,   Z,   0, e2, 0, cond_e1e2, mov2, f32, IF_BFP)
 F(0xb312, LTDBR,   RRE,   Z,   0, f2, 0, f1, mov2, f64, IF_BFP)
 F(0xb342, LTXBR,   RRE,   Z,   x2h, x2l, 0, x1_P, movx, f128, IF_BFP)
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index a40289512da..f89e1ce353b 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -2770,7 +2770,8 @@ static DisasJumpType op_ld16u(DisasContext *s, DisasOps 
*o)
 
 static DisasJumpType op_ld32s(DisasContext *s, DisasOps *o)
 {
-tcg_gen_qemu_ld32s(o->out, o->in2, get_mem_index(s));
+tcg_gen_qemu_ld_tl(o->out, o->in2, get_mem_index(s),
+   MO_TESL | s->insn->data);
 return DISAS_NEXT;
 }
 
-- 
2.39.2

[PATCH v4 09/12] target/s390x: Handle STRL to non-aligned addresses

[Ilya Leoshkevich]

Use MO_ALIGN and let do_unaligned_access() generate a specification
exception.

Reported-by: Nina Schoetterl-Glausch 
Suggested-by: Nina Schoetterl-Glausch 
Reviewed-by: Richard Henderson 
Signed-off-by: Ilya Leoshkevich 
---
 target/s390x/tcg/insn-data.h.inc | 12 ++--
 target/s390x/tcg/translate.c |  3 ++-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/target/s390x/tcg/insn-data.h.inc b/target/s390x/tcg/insn-data.h.inc
index 3abd2dbedd5..30c02b3fcd6 100644
--- a/target/s390x/tcg/insn-data.h.inc
+++ b/target/s390x/tcg/insn-data.h.inc
@@ -840,15 +840,15 @@
 F(0xed15, SQDB,RXE,   Z,   0, m2_64, new, f1, sqdb, 0, IF_BFP)
 
 /* STORE */
-C(0x5000, ST,  RX_a,  Z,   r1_o, a2, 0, 0, st32, 0)
-C(0xe350, STY, RXY_a, LD,  r1_o, a2, 0, 0, st32, 0)
+D(0x5000, ST,  RX_a,  Z,   r1_o, a2, 0, 0, st32, 0, 0)
+D(0xe350, STY, RXY_a, LD,  r1_o, a2, 0, 0, st32, 0, 0)
 C(0xe324, STG, RXY_a, Z,   r1_o, a2, 0, 0, st64, 0)
 F(0x6000, STD, RX_a,  Z,   f1, a2, 0, 0, st64, 0, IF_AFP1)
 F(0xed67, STDY,RXY_a, LD,  f1, a2, 0, 0, st64, 0, IF_AFP1)
-F(0x7000, STE, RX_a,  Z,   e1, a2, 0, 0, st32, 0, IF_AFP1)
-F(0xed66, STEY,RXY_a, LD,  e1, a2, 0, 0, st32, 0, IF_AFP1)
+E(0x7000, STE, RX_a,  Z,   e1, a2, 0, 0, st32, 0, 0, IF_AFP1)
+E(0xed66, STEY,RXY_a, LD,  e1, a2, 0, 0, st32, 0, 0, IF_AFP1)
 /* STORE RELATIVE LONG */
-C(0xc40f, STRL,RIL_b, GIE, r1_o, ri2, 0, 0, st32, 0)
+D(0xc40f, STRL,RIL_b, GIE, r1_o, ri2, 0, 0, st32, 0, MO_ALIGN)
 C(0xc40b, STGRL,   RIL_b, GIE, r1_o, ri2, 0, 0, st64, 0)
 /* STORE CHARACTER */
 C(0x4200, STC, RX_a,  Z,   r1_o, a2, 0, 0, st8, 0)
@@ -867,7 +867,7 @@
 /* STORE HALFWORD RELATIVE LONG */
 C(0xc407, STHRL,   RIL_b, GIE, r1_o, ri2, 0, 0, st16, 0)
 /* STORE HIGH */
-C(0xe3cb, STFH,RXY_a, HW,  r1_sr32, a2, 0, 0, st32, 0)
+D(0xe3cb, STFH,RXY_a, HW,  r1_sr32, a2, 0, 0, st32, 0, 0)
 /* STORE ON CONDITION */
 D(0xebf3, STOC,RSY_b, LOC, 0, 0, 0, 0, soc, 0, 0)
 D(0xebe3, STOCG,   RSY_b, LOC, 0, 0, 0, 0, soc, 0, 1)
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index c67f8440db8..8fd21425dba 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -4370,7 +4370,8 @@ static DisasJumpType op_st16(DisasContext *s, DisasOps *o)
 
 static DisasJumpType op_st32(DisasContext *s, DisasOps *o)
 {
-tcg_gen_qemu_st32(o->in1, o->in2, get_mem_index(s));
+tcg_gen_qemu_st_tl(o->in1, o->in2, get_mem_index(s),
+   MO_TEUL | s->insn->data);
 return DISAS_NEXT;
 }
 
-- 
2.39.2

[PATCH v4 02/12] target/s390x: Handle EXECUTE of odd addresses

[Ilya Leoshkevich]

Generate a specification exception in the helper before trying to fetch
the instruction.

Reported-by: Nina Schoetterl-Glausch 
Reviewed-by: Richard Henderson 
Signed-off-by: Ilya Leoshkevich 
---
 target/s390x/tcg/mem_helper.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index 6835c26dda4..9d1c4bb7374 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -2468,8 +2468,16 @@ void HELPER(stpq_parallel)(CPUS390XState *env, uint64_t 
addr,
 */
 void HELPER(ex)(CPUS390XState *env, uint32_t ilen, uint64_t r1, uint64_t addr)
 {
-uint64_t insn = cpu_lduw_code(env, addr);
-uint8_t opc = insn >> 8;
+uint64_t insn;
+uint8_t opc;
+
+/* EXECUTE targets must be at even addresses.  */
+if (addr & 1) {
+tcg_s390_program_interrupt(env, PGM_SPECIFICATION, GETPC());
+}
+
+insn = cpu_lduw_code(env, addr);
+opc = insn >> 8;
 
 /* Or in the contents of R1[56:63].  */
 insn |= r1 & 0xff;
-- 
2.39.2

[PATCH v4 03/12] target/s390x: Handle LGRL from non-aligned addresses

[Ilya Leoshkevich]

Use MO_ALIGN and let do_unaligned_access() generate a specification
exception.

Reported-by: Nina Schoetterl-Glausch 
Suggested-by: Nina Schoetterl-Glausch 
Reviewed-by: Richard Henderson 
Signed-off-by: Ilya Leoshkevich 
---
 target/s390x/tcg/insn-data.h.inc | 6 +++---
 target/s390x/tcg/translate.c | 3 ++-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/s390x/tcg/insn-data.h.inc b/target/s390x/tcg/insn-data.h.inc
index 6fe8ca51437..d439d803509 100644
--- a/target/s390x/tcg/insn-data.h.inc
+++ b/target/s390x/tcg/insn-data.h.inc
@@ -414,7 +414,7 @@
 C(0xe358, LY,  RXY_a, LD,  0, a2, new, r1_32, ld32s, 0)
 C(0xb904, LGR, RRE,   Z,   0, r2_o, 0, r1, mov2, 0)
 C(0xb914, LGFR,RRE,   Z,   0, r2_32s, 0, r1, mov2, 0)
-C(0xe304, LG,  RXY_a, Z,   0, a2, r1, 0, ld64, 0)
+D(0xe304, LG,  RXY_a, Z,   0, a2, r1, 0, ld64, 0, 0)
 C(0xe314, LGF, RXY_a, Z,   0, a2, r1, 0, ld32s, 0)
 F(0x2800, LDR, RR_a,  Z,   0, f2, 0, f1, mov2, 0, IF_AFP1 | IF_AFP2)
 F(0x6800, LD,  RX_a,  Z,   0, m2_64, 0, f1, mov2, 0, IF_AFP1)
@@ -427,7 +427,7 @@
 C(0xc001, LGFI,RIL_a, EI,  0, i2, 0, r1, mov2, 0)
 /* LOAD RELATIVE LONG */
 C(0xc40d, LRL, RIL_b, GIE, 0, ri2, new, r1_32, ld32s, 0)
-C(0xc408, LGRL,RIL_b, GIE, 0, ri2, r1, 0, ld64, 0)
+D(0xc408, LGRL,RIL_b, GIE, 0, ri2, r1, 0, ld64, 0, MO_ALIGN)
 C(0xc40c, LGFRL,   RIL_b, GIE, 0, ri2, r1, 0, ld32s, 0)
 /* LOAD ADDRESS */
 C(0x4100, LA,  RX_a,  Z,   0, a2, 0, r1, mov2, 0)
@@ -457,7 +457,7 @@
 C(0xb902, LTGR,RRE,   Z,   0, r2_o, 0, r1, mov2, s64)
 C(0xb912, LTGFR,   RRE,   Z,   0, r2_32s, 0, r1, mov2, s64)
 C(0xe312, LT,  RXY_a, EI,  0, a2, new, r1_32, ld32s, s64)
-C(0xe302, LTG, RXY_a, EI,  0, a2, r1, 0, ld64, s64)
+D(0xe302, LTG, RXY_a, EI,  0, a2, r1, 0, ld64, s64, 0)
 C(0xe332, LTGF,RXY_a, GIE, 0, a2, r1, 0, ld32s, s64)
 F(0xb302, LTEBR,   RRE,   Z,   0, e2, 0, cond_e1e2, mov2, f32, IF_BFP)
 F(0xb312, LTDBR,   RRE,   Z,   0, f2, 0, f1, mov2, f64, IF_BFP)
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 14c3896d529..a40289512da 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -2782,7 +2782,8 @@ static DisasJumpType op_ld32u(DisasContext *s, DisasOps 
*o)
 
 static DisasJumpType op_ld64(DisasContext *s, DisasOps *o)
 {
-tcg_gen_qemu_ld64(o->out, o->in2, get_mem_index(s));
+tcg_gen_qemu_ld_i64(o->out, o->in2, get_mem_index(s),
+MO_TEUQ | s->insn->data);
 return DISAS_NEXT;
 }
 
-- 
2.39.2

[PATCH v4 10/12] target/s390x: Handle STGRL to non-aligned addresses

[Ilya Leoshkevich]

Use MO_ALIGN and let do_unaligned_access() generate a specification
exception.

Reported-by: Nina Schoetterl-Glausch 
Suggested-by: Nina Schoetterl-Glausch 
Reviewed-by: Richard Henderson 
Signed-off-by: Ilya Leoshkevich 
---
 target/s390x/tcg/insn-data.h.inc | 8 
 target/s390x/tcg/translate.c | 3 ++-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/target/s390x/tcg/insn-data.h.inc b/target/s390x/tcg/insn-data.h.inc
index 30c02b3fcd6..597d968b0e8 100644
--- a/target/s390x/tcg/insn-data.h.inc
+++ b/target/s390x/tcg/insn-data.h.inc
@@ -842,14 +842,14 @@
 /* STORE */
 D(0x5000, ST,  RX_a,  Z,   r1_o, a2, 0, 0, st32, 0, 0)
 D(0xe350, STY, RXY_a, LD,  r1_o, a2, 0, 0, st32, 0, 0)
-C(0xe324, STG, RXY_a, Z,   r1_o, a2, 0, 0, st64, 0)
-F(0x6000, STD, RX_a,  Z,   f1, a2, 0, 0, st64, 0, IF_AFP1)
-F(0xed67, STDY,RXY_a, LD,  f1, a2, 0, 0, st64, 0, IF_AFP1)
+D(0xe324, STG, RXY_a, Z,   r1_o, a2, 0, 0, st64, 0, 0)
+E(0x6000, STD, RX_a,  Z,   f1, a2, 0, 0, st64, 0, 0, IF_AFP1)
+E(0xed67, STDY,RXY_a, LD,  f1, a2, 0, 0, st64, 0, 0, IF_AFP1)
 E(0x7000, STE, RX_a,  Z,   e1, a2, 0, 0, st32, 0, 0, IF_AFP1)
 E(0xed66, STEY,RXY_a, LD,  e1, a2, 0, 0, st32, 0, 0, IF_AFP1)
 /* STORE RELATIVE LONG */
 D(0xc40f, STRL,RIL_b, GIE, r1_o, ri2, 0, 0, st32, 0, MO_ALIGN)
-C(0xc40b, STGRL,   RIL_b, GIE, r1_o, ri2, 0, 0, st64, 0)
+D(0xc40b, STGRL,   RIL_b, GIE, r1_o, ri2, 0, 0, st64, 0, MO_ALIGN)
 /* STORE CHARACTER */
 C(0x4200, STC, RX_a,  Z,   r1_o, a2, 0, 0, st8, 0)
 C(0xe372, STCY,RXY_a, LD,  r1_o, a2, 0, 0, st8, 0)
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 8fd21425dba..7626692df22 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -4377,7 +4377,8 @@ static DisasJumpType op_st32(DisasContext *s, DisasOps *o)
 
 static DisasJumpType op_st64(DisasContext *s, DisasOps *o)
 {
-tcg_gen_qemu_st64(o->in1, o->in2, get_mem_index(s));
+tcg_gen_qemu_st_i64(o->in1, o->in2, get_mem_index(s),
+MO_TEUQ | s->insn->data);
 return DISAS_NEXT;
 }
 
-- 
2.39.2

[PATCH v4 12/12] tests/tcg/s390x: Test unaligned accesses

[Ilya Leoshkevich]

Add a number of small test that check whether accessing unaligned
addresses in various ways leads to a specification exception.

Run these test both in softmmu and user configurations; expect a PGM
in one case and SIGILL in the other.

Signed-off-by: Ilya Leoshkevich 
---
 tests/tcg/s390x/Makefile.softmmu-target | 15 ++--
 tests/tcg/s390x/Makefile.target |  8 +
 tests/tcg/s390x/br-odd.S| 16 +
 tests/tcg/s390x/cgrl-unaligned.S| 16 +
 tests/tcg/s390x/clrl-unaligned.S| 16 +
 tests/tcg/s390x/crl-unaligned.S | 16 +
 tests/tcg/s390x/ex-odd.S| 17 +
 tests/tcg/s390x/lgrl-unaligned.S| 16 +
 tests/tcg/s390x/llgfrl-unaligned.S  | 16 +
 tests/tcg/s390x/lpswe-unaligned.S   | 18 ++
 tests/tcg/s390x/lrl-unaligned.S | 16 +
 tests/tcg/s390x/pgm-specification-softmmu.S | 40 +
 tests/tcg/s390x/pgm-specification-user.c| 37 +++
 tests/tcg/s390x/pgm-specification.mak   | 15 
 tests/tcg/s390x/softmmu.ld  | 20 +++
 tests/tcg/s390x/stgrl-unaligned.S   | 16 +
 tests/tcg/s390x/strl-unaligned.S| 16 +
 17 files changed, 311 insertions(+), 3 deletions(-)
 create mode 100644 tests/tcg/s390x/br-odd.S
 create mode 100644 tests/tcg/s390x/cgrl-unaligned.S
 create mode 100644 tests/tcg/s390x/clrl-unaligned.S
 create mode 100644 tests/tcg/s390x/crl-unaligned.S
 create mode 100644 tests/tcg/s390x/ex-odd.S
 create mode 100644 tests/tcg/s390x/lgrl-unaligned.S
 create mode 100644 tests/tcg/s390x/llgfrl-unaligned.S
 create mode 100644 tests/tcg/s390x/lpswe-unaligned.S
 create mode 100644 tests/tcg/s390x/lrl-unaligned.S
 create mode 100644 tests/tcg/s390x/pgm-specification-softmmu.S
 create mode 100644 tests/tcg/s390x/pgm-specification-user.c
 create mode 100644 tests/tcg/s390x/pgm-specification.mak
 create mode 100644 tests/tcg/s390x/softmmu.ld
 create mode 100644 tests/tcg/s390x/stgrl-unaligned.S
 create mode 100644 tests/tcg/s390x/strl-unaligned.S

diff --git a/tests/tcg/s390x/Makefile.softmmu-target 
b/tests/tcg/s390x/Makefile.softmmu-target
index 725b6c598db..6d8bf299b28 100644
--- a/tests/tcg/s390x/Makefile.softmmu-target
+++ b/tests/tcg/s390x/Makefile.softmmu-target
@@ -1,11 +1,20 @@
 S390X_SRC=$(SRC_PATH)/tests/tcg/s390x
 VPATH+=$(S390X_SRC)
 QEMU_OPTS=-action panic=exit-failure -kernel
+LINK_SCRIPT=$(S390X_SRC)/softmmu.ld
+LDFLAGS=-nostdlib -static -Wl,-T$(LINK_SCRIPT)
 
-%: %.S
-   $(CC) -march=z13 -m64 -nostdlib -static -Wl,-Ttext=0 \
-   -Wl,--build-id=none $< -o $@
+%.o: %.S
+   $(CC) -march=z13 -m64 -c $< -o $@
+
+%: %.o $(LINK_SCRIPT)
+   $(CC) $< -o $@ $(LDFLAGS)
 
 TESTS += unaligned-lowcore
 TESTS += bal
 TESTS += sam
+
+include $(S390X_SRC)/pgm-specification.mak
+$(PGM_SPECIFICATION_TESTS): pgm-specification-softmmu.o
+$(PGM_SPECIFICATION_TESTS): LDFLAGS+=pgm-specification-softmmu.o
+TESTS += $(PGM_SPECIFICATION_TESTS)
diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index cf93b966862..1002ab79886 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -2,6 +2,9 @@ S390X_SRC=$(SRC_PATH)/tests/tcg/s390x
 VPATH+=$(S390X_SRC)
 CFLAGS+=-march=zEC12 -m64
 
+%.o: %.c
+   $(CC) $(CFLAGS) $(EXTRA_CFLAGS) -c $< -o $@
+
 config-cc.mak: Makefile
$(quiet-@)( \
$(call cc-option,-march=z14, CROSS_CC_HAS_Z14); \
@@ -33,6 +36,11 @@ TESTS+=chrl
 cdsg: CFLAGS+=-pthread
 cdsg: LDFLAGS+=-pthread
 
+include $(S390X_SRC)/pgm-specification.mak
+$(PGM_SPECIFICATION_TESTS): pgm-specification-user.o
+$(PGM_SPECIFICATION_TESTS): LDFLAGS+=pgm-specification-user.o
+TESTS += $(PGM_SPECIFICATION_TESTS)
+
 Z13_TESTS=vistr
 $(Z13_TESTS): CFLAGS+=-march=z13 -O2
 TESTS+=$(Z13_TESTS)
diff --git a/tests/tcg/s390x/br-odd.S b/tests/tcg/s390x/br-odd.S
new file mode 100644
index 000..2fae47a9e34
--- /dev/null
+++ b/tests/tcg/s390x/br-odd.S
@@ -0,0 +1,16 @@
+/*
+ * Test BRanching to a non-mapped odd address.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+.globl test
+test:
+lgrl %r1,odd_addr
+br %r1
+
+.align 8
+odd_addr:
+.quad 0x
+.globl expected_old_psw
+expected_old_psw:
+.quad 0x18000,0x
diff --git a/tests/tcg/s390x/cgrl-unaligned.S b/tests/tcg/s390x/cgrl-unaligned.S
new file mode 100644
index 000..164d68f2e64
--- /dev/null
+++ b/tests/tcg/s390x/cgrl-unaligned.S
@@ -0,0 +1,16 @@
+/*
+ * Test CGRL with a non-doubleword aligned address.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+.globl test
+test:
+cgrl %r1,unaligned
+
+.align 8
+.globl expected_old_psw
+expected_old_psw:
+.quad 0x18000,test
+.long 0
+unaligned:
+.quad 0
diff --git a/tests/tcg/s390x/clrl-unaligned.S b/tests/tcg/s390x/clrl-unaligned.S
new file 

[PATCH v4 01/12] target/s390x: Handle branching to odd addresses

[Ilya Leoshkevich]

Let branching happen and try to generate a new translation block with
an odd address. Generate a specification exception in
cpu_get_tb_cpu_state().

Reported-by: Harold Grovesteen 
Reported-by: Nina Schoetterl-Glausch 
Reviewed-by: Richard Henderson 
Signed-off-by: Ilya Leoshkevich 
---
 target/s390x/cpu.h | 9 +
 1 file changed, 9 insertions(+)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7d6d01325b2..0a76e96e078 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -29,6 +29,7 @@
 #include "cpu_models.h"
 #include "exec/cpu-defs.h"
 #include "qemu/cpu-float.h"
+#include "tcg/tcg_s390x.h"
 
 #define ELF_MACHINE_UNAME "S390X"
 
@@ -381,6 +382,14 @@ static inline int cpu_mmu_index(CPUS390XState *env, bool 
ifetch)
 static inline void cpu_get_tb_cpu_state(CPUS390XState* env, target_ulong *pc,
 target_ulong *cs_base, uint32_t *flags)
 {
+if (env->psw.addr & 1) {
+/*
+ * Instructions must be at even addresses.
+ * This needs to be checked before address translation.
+ */
+env->int_pgm_ilen = 2; /* see s390_cpu_tlb_fill() */
+tcg_s390_program_interrupt(env, PGM_SPECIFICATION, 0);
+}
 *pc = env->psw.addr;
 *cs_base = env->ex_value;
 *flags = (env->psw.mask >> FLAG_MASK_PSW_SHIFT) & FLAG_MASK_PSW;
-- 
2.39.2

Re: [PATCH v2 02/32] tests/docker: all add DOCKER_BUILDKIT to RUNC environment

[Richard Henderson]

On 3/15/23 10:43, Alex Bennée wrote:

It seems we also need to pass DOCKER_BUILDKIT as an argument to docker
itself to get the full benefit of caching.

Signed-off-by: Alex Bennée 
Suggested-by: Fabiano Rosas 
---
  tests/docker/Makefile.include | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Tested-by: Richard Henderson 

r~



diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 54ed77f671..9401525325 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -39,7 +39,7 @@ docker-qemu-src: $(DOCKER_SRC_COPY)
  # General rule for building docker images.
  docker-image-%: $(DOCKER_FILES_DIR)/%.docker
  $(call quiet-command, \
-   $(RUNC) build   \
+   DOCKER_BUILDKIT=1 $(RUNC) build \
$(if $V,,--quiet)   \
$(if $(NOCACHE),--no-cache, \
$(if $(DOCKER_REGISTRY),--cache-from 
$(DOCKER_REGISTRY)/qemu/$*)) \

Re: [PATCH for 8.0 v2] memory: Prevent recursive memory access

[Akihiko Odaki]

On 2023/03/17 1:15, Alexander Bulekov wrote:

On 230316 2124, Akihiko Odaki wrote:

A guest may request ask a memory-mapped device to perform DMA. If the
address specified for DMA is the device performing DMA, it will create
recursion. It is very unlikely that device implementations are prepared
for such an abnormal access, which can result in unpredictable behavior.

In particular, such a recursion breaks e1000e, a network device. If
the device is configured to write the received packet to the register
to trigger receiving, it triggers re-entry to the Rx logic of e1000e.
This causes use-after-free since the Rx logic is not re-entrant.

As there should be no valid reason to perform recursive memory access,
check for recursion before accessing memory-mapped device.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1543
Signed-off-by: Akihiko Odaki 


Hi Akihiko,
I think the spirit of this is similar to the fix I proposed here:
https://lore.kernel.org/qemu-devel/20230313082417.827484-1-alx...@bu.edu/

My version also addresses the following case, which we have found
instances of:
Device Foo Bottom Half -> DMA write to Device Foo Memory Region

That said, the patch is held up on some corner cases and it seems it
will not make it into 8.0. I guess we can add #1543 to the list of
issues in https://gitlab.com/qemu-project/qemu/-/issues/556


The e1000e bug is certainly covered by your fix. It is nice that it also 
covers the case of DMA from bottom half. I hope it will land soon in the 
next version.


Regards,
Akihiko Odaki



Thanks
-Alex


---
  softmmu/memory.c | 79 +---
  1 file changed, 62 insertions(+), 17 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 4699ba55ec..19c60ee1f0 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -50,6 +50,10 @@ static QTAILQ_HEAD(, AddressSpace) address_spaces
  
  static GHashTable *flat_views;
  
+static const Object **accessed_region_owners;

+static size_t accessed_region_owners_capacity;
+static size_t accessed_region_owners_num;
+
  typedef struct AddrRange AddrRange;
  
  /*

@@ -1394,6 +1398,16 @@ bool memory_region_access_valid(MemoryRegion *mr,
  return false;
  }
  
+for (size_t i = 0; i < accessed_region_owners_num; i++) {

+if (accessed_region_owners[i] == mr->owner) {
+qemu_log_mask(LOG_GUEST_ERROR, "Invalid %s at addr 0x%" HWADDR_PRIX
+  ", size %u, region '%s', reason: recursive access\n",
+  is_write ? "write" : "read",
+  addr, size, memory_region_name(mr));
+return false;
+}
+}
+
  /* Treat zero as compatibility all valid */
  if (!mr->ops->valid.max_access_size) {
  return true;
@@ -1413,6 +1427,34 @@ bool memory_region_access_valid(MemoryRegion *mr,
  return true;
  }
  
+static bool memory_region_access_start(MemoryRegion *mr,

+   hwaddr addr,
+   unsigned size,
+   bool is_write,
+   MemTxAttrs attrs)
+{
+if (!memory_region_access_valid(mr, addr, size, is_write, attrs)) {
+return false;
+}
+
+accessed_region_owners_num++;
+if (accessed_region_owners_num > accessed_region_owners_capacity) {
+accessed_region_owners_capacity = accessed_region_owners_num;
+accessed_region_owners = g_realloc_n(accessed_region_owners,
+ accessed_region_owners_capacity,
+ sizeof(*accessed_region_owners));
+}
+
+accessed_region_owners[accessed_region_owners_num - 1] = mr->owner;
+
+return true;
+}
+
+static void memory_region_access_end(void)
+{
+accessed_region_owners_num--;
+}
+
  static MemTxResult memory_region_dispatch_read1(MemoryRegion *mr,
  hwaddr addr,
  uint64_t *pval,
@@ -1450,12 +1492,13 @@ MemTxResult memory_region_dispatch_read(MemoryRegion 
*mr,
 mr->alias_offset + addr,
 pval, op, attrs);
  }
-if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
+if (!memory_region_access_start(mr, addr, size, false, attrs)) {
  *pval = unassigned_mem_read(mr, addr, size);
  return MEMTX_DECODE_ERROR;
  }
  
  r = memory_region_dispatch_read1(mr, addr, pval, size, attrs);

+memory_region_access_end();
  adjust_endianness(mr, pval, op);
  return r;
  }
@@ -1493,13 +1536,14 @@ MemTxResult memory_region_dispatch_write(MemoryRegion 
*mr,
   MemTxAttrs attrs)
  {
  unsigned size = memop_size(op);
+MemTxResult result;
  
  if (mr->alias) {

  return 

[PATCH for 8.0 v3] memory: Prevent recursive memory access

[Akihiko Odaki]

A guest may request ask a memory-mapped device to perform DMA. If the
address specified for DMA is the device performing DMA, it will create
recursion. It is very unlikely that device implementations are prepared
for such an abnormal access, which can result in unpredictable behavior.

In particular, such a recursion breaks e1000e, a network device. If
the device is configured to write the received packet to the register
to trigger receiving, it triggers re-entry to the Rx logic of e1000e.
This causes use-after-free since the Rx logic is not re-entrant.

As there should be no valid reason to perform recursive memory access,
check for recursion before accessing memory-mapped device.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1543
Signed-off-by: Akihiko Odaki 
---
V1 -> V2: Marked the variable thread-local. Introduced linked list.

 softmmu/memory.c | 81 ++--
 1 file changed, 64 insertions(+), 17 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 4699ba55ec..6be33a9e3e 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -61,6 +61,15 @@ struct AddrRange {
 Int128 size;
 };
 
+typedef struct AccessedRegion AccessedRegion;
+
+struct AccessedRegion {
+const Object *owner;
+const AccessedRegion *next;
+};
+
+static __thread const AccessedRegion *accessed_region;
+
 static AddrRange addrrange_make(Int128 start, Int128 size)
 {
 return (AddrRange) { start, size };
@@ -1394,6 +1403,16 @@ bool memory_region_access_valid(MemoryRegion *mr,
 return false;
 }
 
+for (const AccessedRegion *ar = accessed_region; ar; ar = ar->next) {
+if (ar->owner == mr->owner) {
+qemu_log_mask(LOG_GUEST_ERROR, "Invalid %s at addr 0x%" HWADDR_PRIX
+  ", size %u, region '%s', reason: recursive access\n",
+  is_write ? "write" : "read",
+  addr, size, memory_region_name(mr));
+return false;
+}
+}
+
 /* Treat zero as compatibility all valid */
 if (!mr->ops->valid.max_access_size) {
 return true;
@@ -1413,6 +1432,29 @@ bool memory_region_access_valid(MemoryRegion *mr,
 return true;
 }
 
+static bool memory_region_access_start(MemoryRegion *mr,
+   hwaddr addr,
+   unsigned size,
+   bool is_write,
+   MemTxAttrs attrs,
+   AccessedRegion *ar)
+{
+if (!memory_region_access_valid(mr, addr, size, is_write, attrs)) {
+return false;
+}
+
+ar->owner = mr->owner;
+ar->next = accessed_region;
+accessed_region = ar->next;
+
+return true;
+}
+
+static void memory_region_access_end(void)
+{
+accessed_region = accessed_region->next;
+}
+
 static MemTxResult memory_region_dispatch_read1(MemoryRegion *mr,
 hwaddr addr,
 uint64_t *pval,
@@ -1443,6 +1485,7 @@ MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
 MemTxAttrs attrs)
 {
 unsigned size = memop_size(op);
+AccessedRegion ar;
 MemTxResult r;
 
 if (mr->alias) {
@@ -1450,12 +1493,13 @@ MemTxResult memory_region_dispatch_read(MemoryRegion 
*mr,
mr->alias_offset + addr,
pval, op, attrs);
 }
-if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
+if (!memory_region_access_start(mr, addr, size, false, attrs, )) {
 *pval = unassigned_mem_read(mr, addr, size);
 return MEMTX_DECODE_ERROR;
 }
 
 r = memory_region_dispatch_read1(mr, addr, pval, size, attrs);
+memory_region_access_end();
 adjust_endianness(mr, pval, op);
 return r;
 }
@@ -1493,13 +1537,15 @@ MemTxResult memory_region_dispatch_write(MemoryRegion 
*mr,
  MemTxAttrs attrs)
 {
 unsigned size = memop_size(op);
+AccessedRegion ar;
+MemTxResult result;
 
 if (mr->alias) {
 return memory_region_dispatch_write(mr->alias,
 mr->alias_offset + addr,
 data, op, attrs);
 }
-if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
+if (!memory_region_access_start(mr, addr, size, true, attrs, )) {
 unassigned_mem_write(mr, addr, data, size);
 return MEMTX_DECODE_ERROR;
 }
@@ -1508,23 +1554,24 @@ MemTxResult memory_region_dispatch_write(MemoryRegion 
*mr,
 
 if ((!kvm_eventfds_enabled()) &&
 memory_region_dispatch_write_eventfds(mr, addr, data, size, attrs)) {
-return MEMTX_OK;
-}
-
-if (mr->ops->write) {
-return access_with_adjusted_size(addr, , size,
-   

Re: [PATCH for 8.0 v2] memory: Prevent recursive memory access

[Alexander Bulekov]

On 230316 2124, Akihiko Odaki wrote:
> A guest may request ask a memory-mapped device to perform DMA. If the
> address specified for DMA is the device performing DMA, it will create
> recursion. It is very unlikely that device implementations are prepared
> for such an abnormal access, which can result in unpredictable behavior.
> 
> In particular, such a recursion breaks e1000e, a network device. If
> the device is configured to write the received packet to the register
> to trigger receiving, it triggers re-entry to the Rx logic of e1000e.
> This causes use-after-free since the Rx logic is not re-entrant.
> 
> As there should be no valid reason to perform recursive memory access,
> check for recursion before accessing memory-mapped device.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1543
> Signed-off-by: Akihiko Odaki 

Hi Akihiko,
I think the spirit of this is similar to the fix I proposed here:
https://lore.kernel.org/qemu-devel/20230313082417.827484-1-alx...@bu.edu/

My version also addresses the following case, which we have found
instances of:
Device Foo Bottom Half -> DMA write to Device Foo Memory Region

That said, the patch is held up on some corner cases and it seems it
will not make it into 8.0. I guess we can add #1543 to the list of
issues in https://gitlab.com/qemu-project/qemu/-/issues/556

Thanks
-Alex

> ---
>  softmmu/memory.c | 79 +---
>  1 file changed, 62 insertions(+), 17 deletions(-)
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 4699ba55ec..19c60ee1f0 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -50,6 +50,10 @@ static QTAILQ_HEAD(, AddressSpace) address_spaces
>  
>  static GHashTable *flat_views;
>  
> +static const Object **accessed_region_owners;
> +static size_t accessed_region_owners_capacity;
> +static size_t accessed_region_owners_num;
> +
>  typedef struct AddrRange AddrRange;
>  
>  /*
> @@ -1394,6 +1398,16 @@ bool memory_region_access_valid(MemoryRegion *mr,
>  return false;
>  }
>  
> +for (size_t i = 0; i < accessed_region_owners_num; i++) {
> +if (accessed_region_owners[i] == mr->owner) {
> +qemu_log_mask(LOG_GUEST_ERROR, "Invalid %s at addr 0x%" 
> HWADDR_PRIX
> +  ", size %u, region '%s', reason: recursive 
> access\n",
> +  is_write ? "write" : "read",
> +  addr, size, memory_region_name(mr));
> +return false;
> +}
> +}
> +
>  /* Treat zero as compatibility all valid */
>  if (!mr->ops->valid.max_access_size) {
>  return true;
> @@ -1413,6 +1427,34 @@ bool memory_region_access_valid(MemoryRegion *mr,
>  return true;
>  }
>  
> +static bool memory_region_access_start(MemoryRegion *mr,
> +   hwaddr addr,
> +   unsigned size,
> +   bool is_write,
> +   MemTxAttrs attrs)
> +{
> +if (!memory_region_access_valid(mr, addr, size, is_write, attrs)) {
> +return false;
> +}
> +
> +accessed_region_owners_num++;
> +if (accessed_region_owners_num > accessed_region_owners_capacity) {
> +accessed_region_owners_capacity = accessed_region_owners_num;
> +accessed_region_owners = g_realloc_n(accessed_region_owners,
> + accessed_region_owners_capacity,
> + 
> sizeof(*accessed_region_owners));
> +}
> +
> +accessed_region_owners[accessed_region_owners_num - 1] = mr->owner;
> +
> +return true;
> +}
> +
> +static void memory_region_access_end(void)
> +{
> +accessed_region_owners_num--;
> +}
> +
>  static MemTxResult memory_region_dispatch_read1(MemoryRegion *mr,
>  hwaddr addr,
>  uint64_t *pval,
> @@ -1450,12 +1492,13 @@ MemTxResult memory_region_dispatch_read(MemoryRegion 
> *mr,
> mr->alias_offset + addr,
> pval, op, attrs);
>  }
> -if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
> +if (!memory_region_access_start(mr, addr, size, false, attrs)) {
>  *pval = unassigned_mem_read(mr, addr, size);
>  return MEMTX_DECODE_ERROR;
>  }
>  
>  r = memory_region_dispatch_read1(mr, addr, pval, size, attrs);
> +memory_region_access_end();
>  adjust_endianness(mr, pval, op);
>  return r;
>  }
> @@ -1493,13 +1536,14 @@ MemTxResult memory_region_dispatch_write(MemoryRegion 
> *mr,
>   MemTxAttrs attrs)
>  {
>  unsigned size = memop_size(op);
> +MemTxResult result;
>  
>  if (mr->alias) {
>  return memory_region_dispatch_write(mr->alias,
>  

Re: [PATCH v2 09/32] include/exec: fix kerneldoc definition

[Richard Henderson]

On 3/15/23 10:43, Alex Bennée wrote:

The kerneldoc processor complains about the mismatched variable name.
Fix it.

Message-Id:<20230310103123.2118519-11-alex.ben...@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Alex Bennée
---
  include/exec/memory.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v2 07/32] tests/tcg: add some help output for running individual tests

[Richard Henderson]

On 3/15/23 10:43, Alex Bennée wrote:

So you can do:

   cd tests/tcg/aarch64-linux-user
   make -f ../Makefile.target help

To see the list of tests. You can then run each one individually.

Signed-off-by: Alex Bennée
---
  tests/tcg/Makefile.target | 7 +++
  1 file changed, 7 insertions(+)


Acked-by: Richard Henderson 

r~

Re: [PATCH v2 06/32] include/qemu: add documentation for memory callbacks

[Richard Henderson]

On 3/15/23 10:43, Alex Bennée wrote:

Some API documentation was missed, rectify that.

Fixes:https://gitlab.com/qemu-project/qemu/-/issues/1497
Signed-off-by: Alex Bennée
---
  include/qemu/qemu-plugin.h | 47 ++
  1 file changed, 43 insertions(+), 4 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor

[Philippe Mathieu-Daudé]

On 16/3/23 16:57, Akihiko Odaki wrote:

The current implementation of igb uses only part of a advanced Tx
context descriptor because it misses some features and sniffs the trait
of the packet instead of respecting the packet type specified in the
descriptor. However, we will certainly need the entire Tx context
descriptor when we update igb to respect these ignored fields. Save the
entire Tx context descriptor to prepare for such a change.

Signed-off-by: Akihiko Odaki 
---
V1 -> V2: Bump igb-tx version

  hw/net/igb.c  | 10 ++
  hw/net/igb_core.c | 17 ++---
  hw/net/igb_core.h |  3 +--
  3 files changed, 17 insertions(+), 13 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

[PATCH for 8.0 v2] igb: Save the entire Tx context descriptor

[Akihiko Odaki]

The current implementation of igb uses only part of a advanced Tx
context descriptor because it misses some features and sniffs the trait
of the packet instead of respecting the packet type specified in the
descriptor. However, we will certainly need the entire Tx context
descriptor when we update igb to respect these ignored fields. Save the
entire Tx context descriptor to prepare for such a change.

Signed-off-by: Akihiko Odaki 
---
V1 -> V2: Bump igb-tx version

 hw/net/igb.c  | 10 ++
 hw/net/igb_core.c | 17 ++---
 hw/net/igb_core.h |  3 +--
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/hw/net/igb.c b/hw/net/igb.c
index c6d753df87..f9ec82fc28 100644
--- a/hw/net/igb.c
+++ b/hw/net/igb.c
@@ -504,11 +504,13 @@ static int igb_post_load(void *opaque, int version_id)
 
 static const VMStateDescription igb_vmstate_tx = {
 .name = "igb-tx",
-.version_id = 1,
-.minimum_version_id = 1,
+.version_id = 2,
+.minimum_version_id = 2,
 .fields = (VMStateField[]) {
-VMSTATE_UINT16(vlan, struct igb_tx),
-VMSTATE_UINT16(mss, struct igb_tx),
+VMSTATE_UINT32(ctx.vlan_macip_lens, struct igb_tx),
+VMSTATE_UINT32(ctx.seqnum_seed, struct igb_tx),
+VMSTATE_UINT32(ctx.type_tucmd_mlhl, struct igb_tx),
+VMSTATE_UINT32(ctx.mss_l4len_idx, struct igb_tx),
 VMSTATE_BOOL(tse, struct igb_tx),
 VMSTATE_BOOL(ixsm, struct igb_tx),
 VMSTATE_BOOL(txsm, struct igb_tx),
diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
index a7c7bfdc75..304f5d849f 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -390,7 +390,8 @@ static bool
 igb_setup_tx_offloads(IGBCore *core, struct igb_tx *tx)
 {
 if (tx->tse) {
-if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, tx->mss)) {
+uint32_t mss = tx->ctx.mss_l4len_idx >> 16;
+if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, mss)) {
 return false;
 }
 
@@ -550,8 +551,10 @@ igb_process_tx_desc(IGBCore *core,
E1000_ADVTXD_DTYP_CTXT) {
 /* advanced transmit context descriptor */
 tx_ctx_desc = (struct e1000_adv_tx_context_desc *)tx_desc;
-tx->vlan = le32_to_cpu(tx_ctx_desc->vlan_macip_lens) >> 16;
-tx->mss = le32_to_cpu(tx_ctx_desc->mss_l4len_idx) >> 16;
+tx->ctx.vlan_macip_lens = 
le32_to_cpu(tx_ctx_desc->vlan_macip_lens);
+tx->ctx.seqnum_seed = le32_to_cpu(tx_ctx_desc->seqnum_seed);
+tx->ctx.type_tucmd_mlhl = 
le32_to_cpu(tx_ctx_desc->type_tucmd_mlhl);
+tx->ctx.mss_l4len_idx = le32_to_cpu(tx_ctx_desc->mss_l4len_idx);
 return;
 } else {
 /* unknown descriptor type */
@@ -575,8 +578,9 @@ igb_process_tx_desc(IGBCore *core,
 if (cmd_type_len & E1000_TXD_CMD_EOP) {
 if (!tx->skip_cp && net_tx_pkt_parse(tx->tx_pkt)) {
 if (cmd_type_len & E1000_TXD_CMD_VLE) {
-net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, tx->vlan,
-core->mac[VET] & 0x);
+uint16_t vlan = tx->ctx.vlan_macip_lens >> 16;
+uint16_t vet = core->mac[VET] & 0x;
+net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, vlan, vet);
 }
 if (igb_tx_pkt_send(core, tx, queue_index)) {
 igb_on_tx_done_update_stats(core, tx->tx_pkt);
@@ -4024,8 +4028,7 @@ static void igb_reset(IGBCore *core, bool sw)
 for (i = 0; i < ARRAY_SIZE(core->tx); i++) {
 tx = >tx[i];
 net_tx_pkt_reset(tx->tx_pkt);
-tx->vlan = 0;
-tx->mss = 0;
+memset(>ctx, 0, sizeof(tx->ctx));
 tx->tse = false;
 tx->ixsm = false;
 tx->txsm = false;
diff --git a/hw/net/igb_core.h b/hw/net/igb_core.h
index 814c1e264b..3483edc655 100644
--- a/hw/net/igb_core.h
+++ b/hw/net/igb_core.h
@@ -72,8 +72,7 @@ struct IGBCore {
 QEMUTimer *autoneg_timer;
 
 struct igb_tx {
-uint16_t vlan;  /* VLAN Tag */
-uint16_t mss;   /* Maximum Segment Size */
+struct e1000_adv_tx_context_desc ctx;
 bool tse;   /* TCP/UDP Segmentation Enable */
 bool ixsm;  /* Insert IP Checksum */
 bool txsm;  /* Insert TCP/UDP Checksum */
-- 
2.39.2

Re: dropping 32-bit host support

[Daniel P . Berrangé]

On Thu, Mar 16, 2023 at 04:01:06PM +0300, Andrew Randrianasulu wrote:
> Well, this language about "market" and "investment"  not just figures of
> the speech, sadly? Because paid developers work on  areas they paid to
> develop, by boss with big bucks.

This is FUD.

Many QEMU maintainers are employeed, but that does not mean that their
boss gets to dictate what the QEMU community does. The company has its
priorities but this cannot be forced onto the community. Changes have
to be made through tradeoffs and consensus building across all active
maintainers.

To put it another way, responsible open source maintainers/contributors
wear two hats.

With their corporate hat on they have tasks to work on that are directly
important to their employer in the short term. They can make a case for
why these contributions are beneficial, but there's never a guarantee
the community will agree / accept it.

With their community hat on they look at, and work on, what is important
for the health of the community in general. This can sometimes be contrary
to what the employer would otherwise like to see. Wise companies accept
this tradeoff, because the long term health of the community is ultimately
important to them too.

QEMU is fortunate to have many responsible maintainers who balance the
demands of their employer vs the community on an ongoing basis.

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[PATCH v2 2/3] vhost: Remove vhost_backend_can_merge() callback

[David Hildenbrand]

Checking whether the memory regions are equal is sufficient: if they are
equal, then most certainly the contained fd is equal.

The whole vhost-user memslot handling is suboptimal and overly
complicated. We shouldn't have to lookup a RAM memory regions we got
notified about in vhost_user_get_mr_data() using a host pointer. But that
requires a bigger rework -- especially an alternative vhost_set_mem_table()
backend call that simply consumes MemoryRegionSections.

For now, let's just drop vhost_backend_can_merge().

Acked-by: Stefan Hajnoczi 
Reviewed-by: Igor Mammedov 
Signed-off-by: David Hildenbrand 
---
 hw/virtio/vhost-user.c| 14 --
 hw/virtio/vhost-vdpa.c|  1 -
 hw/virtio/vhost.c |  6 +-
 include/hw/virtio/vhost-backend.h |  4 
 4 files changed, 1 insertion(+), 24 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 0c3e2702b1..831375a967 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2195,19 +2195,6 @@ static int vhost_user_migration_done(struct vhost_dev 
*dev, char* mac_addr)
 return -ENOTSUP;
 }
 
-static bool vhost_user_can_merge(struct vhost_dev *dev,
- uint64_t start1, uint64_t size1,
- uint64_t start2, uint64_t size2)
-{
-ram_addr_t offset;
-int mfd, rfd;
-
-(void)vhost_user_get_mr_data(start1, , );
-(void)vhost_user_get_mr_data(start2, , );
-
-return mfd == rfd;
-}
-
 static int vhost_user_net_set_mtu(struct vhost_dev *dev, uint16_t mtu)
 {
 VhostUserMsg msg;
@@ -2704,7 +2691,6 @@ const VhostOps user_ops = {
 .vhost_set_vring_enable = vhost_user_set_vring_enable,
 .vhost_requires_shm_log = vhost_user_requires_shm_log,
 .vhost_migration_done = vhost_user_migration_done,
-.vhost_backend_can_merge = vhost_user_can_merge,
 .vhost_net_set_mtu = vhost_user_net_set_mtu,
 .vhost_set_iotlb_callback = vhost_user_set_iotlb_callback,
 .vhost_send_device_iotlb_msg = vhost_user_send_device_iotlb_msg,
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index bc6bad23d5..38d98528e7 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1355,7 +1355,6 @@ const VhostOps vdpa_ops = {
 .vhost_set_config = vhost_vdpa_set_config,
 .vhost_requires_shm_log = NULL,
 .vhost_migration_done = NULL,
-.vhost_backend_can_merge = NULL,
 .vhost_net_set_mtu = NULL,
 .vhost_set_iotlb_callback = NULL,
 .vhost_send_device_iotlb_msg = NULL,
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 912cc56603..8706d189ec 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -729,11 +729,7 @@ static void vhost_region_add_section(struct vhost_dev *dev,
 size_t offset = mrs_gpa - prev_gpa_start;
 
 if (prev_host_start + offset == mrs_host &&
-section->mr == prev_sec->mr &&
-(!dev->vhost_ops->vhost_backend_can_merge ||
- dev->vhost_ops->vhost_backend_can_merge(dev,
-mrs_host, mrs_size,
-prev_host_start, prev_size))) {
+section->mr == prev_sec->mr) {
 uint64_t max_end = MAX(prev_host_end, mrs_host + mrs_size);
 need_add = false;
 prev_sec->offset_within_address_space =
diff --git a/include/hw/virtio/vhost-backend.h 
b/include/hw/virtio/vhost-backend.h
index 2349a4a7d2..f3ba7b676b 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -86,9 +86,6 @@ typedef int (*vhost_set_vring_enable_op)(struct vhost_dev 
*dev,
 typedef bool (*vhost_requires_shm_log_op)(struct vhost_dev *dev);
 typedef int (*vhost_migration_done_op)(struct vhost_dev *dev,
char *mac_addr);
-typedef bool (*vhost_backend_can_merge_op)(struct vhost_dev *dev,
-   uint64_t start1, uint64_t size1,
-   uint64_t start2, uint64_t size2);
 typedef int (*vhost_vsock_set_guest_cid_op)(struct vhost_dev *dev,
 uint64_t guest_cid);
 typedef int (*vhost_vsock_set_running_op)(struct vhost_dev *dev, int start);
@@ -163,7 +160,6 @@ typedef struct VhostOps {
 vhost_set_vring_enable_op vhost_set_vring_enable;
 vhost_requires_shm_log_op vhost_requires_shm_log;
 vhost_migration_done_op vhost_migration_done;
-vhost_backend_can_merge_op vhost_backend_can_merge;
 vhost_vsock_set_guest_cid_op vhost_vsock_set_guest_cid;
 vhost_vsock_set_running_op vhost_vsock_set_running;
 vhost_set_iotlb_callback_op vhost_set_iotlb_callback;
-- 
2.39.2

[PATCH v2 3/3] softmmu/physmem: Fixup qemu_ram_block_from_host() documentation

[David Hildenbrand]

Let's fixup the documentation (e.g., removing traces of the ram_addr_t
parameter that no longer exists) and move it to the header file while at
it.

Suggested-by: Igor Mammedov 
Signed-off-by: David Hildenbrand 
---
 include/exec/cpu-common.h | 15 +++
 softmmu/physmem.c | 17 -
 2 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 6feaa40ca7..edef5bee21 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -75,6 +75,21 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length);
 ram_addr_t qemu_ram_addr_from_host(void *ptr);
 ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr);
 RAMBlock *qemu_ram_block_by_name(const char *name);
+
+/*
+ * Translates a host ptr back to a RAMBlock and an offset in that RAMBlock.
+ *
+ * @ptr: The host pointer to transalte.
+ * @round_offset: Whether to round the result offset down to a target page
+ * @offset: Will be set to the offset within the returned RAMBlock.
+ *
+ * Returns: RAMBlock (or NULL if not found)
+ *
+ * By the time this function returns, the returned pointer is not protected
+ * by RCU anymore.  If the caller is not within an RCU critical section and
+ * does not hold the iothread lock, it must have other means of protecting the
+ * pointer, such as a reference to the memory region that owns the RAMBlock.
+ */
 RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
ram_addr_t *offset);
 ram_addr_t qemu_ram_block_host_offset(RAMBlock *rb, void *host);
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index fb412a56e1..36b33786fd 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -2169,23 +2169,6 @@ ram_addr_t qemu_ram_block_host_offset(RAMBlock *rb, void 
*host)
 return res;
 }
 
-/*
- * Translates a host ptr back to a RAMBlock, a ram_addr and an offset
- * in that RAMBlock.
- *
- * ptr: Host pointer to look up
- * round_offset: If true round the result offset down to a page boundary
- * *ram_addr: set to result ram_addr
- * *offset: set to result offset within the RAMBlock
- *
- * Returns: RAMBlock (or NULL if not found)
- *
- * By the time this function returns, the returned pointer is not protected
- * by RCU anymore.  If the caller is not within an RCU critical section and
- * does not hold the iothread lock, it must have other means of protecting the
- * pointer, such as a reference to the region that includes the incoming
- * ram_addr_t.
- */
 RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
ram_addr_t *offset)
 {
-- 
2.39.2

[PATCH v2 0/3] vhost: memslot handling improvements

[David Hildenbrand]

Following up on my previous work to make virtio-mem consume multiple
memslots dynamically [1] that requires precise accounting between used vs.
reserved memslots, I realized that vhost makes this extra hard by
filtering out some memory region sections (so they don't consume a
memslot) in the vhost-user case, which messes up the whole memslot
accounting.

This series fixes what I found to be broken and prepares for more work on
[1]. Further, it cleanes up the merge checks that I consider unnecessary.

[1] https://lkml.kernel.org/r/20211027124531.57561-8-da...@redhat.com

Cc: "Michael S. Tsirkin" 
Cc: Stefan Hajnoczi 
Cc: Dr. David Alan Gilbert 
Cc: Igor Mammedov 

v1 -> v2:
- "vhost: Rework memslot filtering and fix "used_memslot" tracking"
-- New approach: keep filtering, but make filtering less generic and
   track separately. This should keep any existing setups working.
- "softmmu/physmem: Fixup qemu_ram_block_from_host() documentation"
-- As requested by Igor

David Hildenbrand (3):
  vhost: Rework memslot filtering and fix "used_memslot" tracking
  vhost: Remove vhost_backend_can_merge() callback
  softmmu/physmem: Fixup qemu_ram_block_from_host() documentation

 hw/virtio/vhost-user.c| 21 ++-
 hw/virtio/vhost-vdpa.c|  1 -
 hw/virtio/vhost.c | 62 ---
 include/exec/cpu-common.h | 15 
 include/hw/virtio/vhost-backend.h |  9 +
 softmmu/physmem.c | 17 -
 6 files changed, 68 insertions(+), 57 deletions(-)

-- 
2.39.2

[PATCH v2 1/3] vhost: Rework memslot filtering and fix "used_memslot" tracking

[David Hildenbrand]

Having multiple vhost devices, some filtering out fd-less memslots and
some not, can mess up the "used_memslot" accounting. Consequently our
"free memslot" checks become unreliable and we might run out of free
memslots at runtime later.

An example sequence which can trigger a potential issue that involves
different vhost backends (vhost-kernel and vhost-user) and hotplugged
memory devices can be found at [1].

Let's make the filtering mechanism less generic and distinguish between
backends that support private memslots (without a fd) and ones that only
support shared memslots (with a fd). Track the used_memslots for both
cases separately and use the corresponding value when required.

Note: Most probably we should filter out MAP_PRIVATE fd-based RAM regions
(for example, via memory-backend-memfd,...,shared=off or as default with
 memory-backend-file) as well. When not using MAP_SHARED, it might not work
as expected. Add a TODO for now.

[1] https://lkml.kernel.org/r/fad9136f-08d3-3fd9-71a1-502069c00...@redhat.com

Fixes: 988a27754bbb ("vhost: allow backends to filter memory sections")
Cc: Tiwei Bie 
Signed-off-by: David Hildenbrand 
---
 hw/virtio/vhost-user.c|  7 ++--
 hw/virtio/vhost.c | 56 ++-
 include/hw/virtio/vhost-backend.h |  5 ++-
 3 files changed, 52 insertions(+), 16 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index e5285df4ba..0c3e2702b1 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2453,10 +2453,9 @@ vhost_user_crypto_close_session(struct vhost_dev *dev, 
uint64_t session_id)
 return 0;
 }
 
-static bool vhost_user_mem_section_filter(struct vhost_dev *dev,
-  MemoryRegionSection *section)
+static bool vhost_user_no_private_memslots(struct vhost_dev *dev)
 {
-return memory_region_get_fd(section->mr) >= 0;
+return true;
 }
 
 static int vhost_user_get_inflight_fd(struct vhost_dev *dev,
@@ -2686,6 +2685,7 @@ const VhostOps user_ops = {
 .vhost_backend_init = vhost_user_backend_init,
 .vhost_backend_cleanup = vhost_user_backend_cleanup,
 .vhost_backend_memslots_limit = vhost_user_memslots_limit,
+.vhost_backend_no_private_memslots = vhost_user_no_private_memslots,
 .vhost_set_log_base = vhost_user_set_log_base,
 .vhost_set_mem_table = vhost_user_set_mem_table,
 .vhost_set_vring_addr = vhost_user_set_vring_addr,
@@ -2712,7 +2712,6 @@ const VhostOps user_ops = {
 .vhost_set_config = vhost_user_set_config,
 .vhost_crypto_create_session = vhost_user_crypto_create_session,
 .vhost_crypto_close_session = vhost_user_crypto_close_session,
-.vhost_backend_mem_section_filter = vhost_user_mem_section_filter,
 .vhost_get_inflight_fd = vhost_user_get_inflight_fd,
 .vhost_set_inflight_fd = vhost_user_set_inflight_fd,
 .vhost_dev_start = vhost_user_dev_start,
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index a266396576..912cc56603 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -46,20 +46,33 @@
 static struct vhost_log *vhost_log;
 static struct vhost_log *vhost_log_shm;
 
+/* Memslots used by backends that support private memslots (without an fd). */
 static unsigned int used_memslots;
+
+/* Memslots used by backends that only support shared memslots (with an fd). */
+static unsigned int used_shared_memslots;
+
 static QLIST_HEAD(, vhost_dev) vhost_devices =
 QLIST_HEAD_INITIALIZER(vhost_devices);
 
 bool vhost_has_free_slot(void)
 {
-unsigned int slots_limit = ~0U;
+unsigned int free = UINT_MAX;
 struct vhost_dev *hdev;
 
 QLIST_FOREACH(hdev, _devices, entry) {
 unsigned int r = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
-slots_limit = MIN(slots_limit, r);
+unsigned int cur_free;
+
+if (hdev->vhost_ops->vhost_backend_no_private_memslots &&
+hdev->vhost_ops->vhost_backend_no_private_memslots(hdev)) {
+cur_free = r - used_shared_memslots;
+} else {
+cur_free = r - used_memslots;
+}
+free = MIN(free, cur_free);
 }
-return slots_limit > used_memslots;
+return free > 1;
 }
 
 static void vhost_dev_sync_region(struct vhost_dev *dev,
@@ -475,8 +488,7 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
  * vhost_section: identify sections needed for vhost access
  *
  * We only care about RAM sections here (where virtqueue and guest
- * internals accessed by virtio might live). If we find one we still
- * allow the backend to potentially filter it out of our list.
+ * internals accessed by virtio might live).
  */
 static bool vhost_section(struct vhost_dev *dev, MemoryRegionSection *section)
 {
@@ -503,8 +515,16 @@ static bool vhost_section(struct vhost_dev *dev, 
MemoryRegionSection *section)
 return false;
 }
 
-if 

Re: dropping 32-bit host support

[Andrew Randrianasulu]

чт, 16 мар. 2023 г., 18:21 Warner Losh :

>
>
> On Thu, Mar 16, 2023 at 7:33 AM Thomas Huth  wrote:
>
>> If you'd followed the QEMU project, you'd know that there are very
>> helpful
>> people around, from all kind of companies, Linaro guys who help with
>> reviewing and merging non-ARM patches, Red Hatters who help with BSD
>
> and Haiku patches, etc.
>>
>
> Without this help, bsd-user would be dead. As it is, it is struggling with
> its own
> resource issues, but the kind help I've received from the QEMU project has
> motivated me to keep going in upstreaming what our fork has, as well as
> working to make the code better.
>
> I'll only add that FreeBSD's efforts to improve its CI story was derailed
> for two
> years by people like this, so it makes me happy to see lines being drawn
> in this thread.
>

Yeah, this. Just it seems we are ended up on different sides of said line.
But this is ok.


They aren't unreasonable, and look to me to be in the best
> interest of the QEMU project. You can't make everybody happy all the time.
> And while it's good to try sometimes, other times it bogs down real
> efforts to
> make things better. This is one of those times.
>
> Warner
>

Re: dropping 32-bit host support

[Andrew Randrianasulu]

чт, 16 мар. 2023 г., 16:32 Thomas Huth :

> On 16/03/2023 14.01, Andrew Randrianasulu wrote:
> ...
> > Well, this language about "market" and "investment"  not just figures of
> the
> > speech, sadly? Because paid developers work on  areas they paid to
> develop,
> > by boss with big bucks.
>
> Sorry for getting more explicit now, but: Can you please stop making such
> aggressive assertions which are obviously wrong and where you apparently
> have no clue about about?
>

I usually read much more than I write, thank you very much.



> If you'd followed the QEMU project, you'd know that there are very helpful
> people around, from all kind of companies, Linaro guys who help with
> reviewing and merging non-ARM patches, Red Hatters who help with BSD and
> Haiku patches, etc.
>
> Anyway, if you're not happy with the way the project is evolving, then
> start
> contributing instead of grumbling.
>


Is there any point to contributing to project that happily will told you to
.go smoke in a corner?

>
>   Thomas
>
>

Re: [PATCH] docs/sphinx/kerneldoc.py: Honour --enable-werror

[Peter Maydell]

On Thu, 16 Mar 2023 at 15:16, Laurent Vivier  wrote:
>
> Le 16/03/2023 à 14:42, Peter Maydell a écrit :
> >
> >
> > On Thu, 16 Mar 2023 at 13:40, Laurent Vivier  > > wrote:
> >  >
> >  > Le 14/03/2023 à 12:44, Peter Maydell a écrit :
> >  > > Currently, the kerneldoc Sphinx plugin doesn't honour the
> >  > > --enable-werror configure option, so its warnings are never fatal.
> >  > > This is because although we do pass sphinx-build the -W switch, the
> >  > > warnings from kerneldoc are produced by the scripts/kernel-doc script
> >  > > directly and don't go through Sphinx's "emit a warning" function.
> >  > >
> >  > > When --enable-werror is in effect, pass sphinx-build an extra
> >  > > argument -Dkerneldoc_werror=1.  The kerneldoc plugin can then use
> >  > > this to determine whether it should be passing the kernel-doc script
> >  > > -Werror.
> >  > >
> >  > > We do this because there is no documented mechanism for
> >  > > a Sphinx plugin to determine whether sphinx-build was
> >  > > passed -W or not; if one is provided then we can switch to
> >  > > that at a later date:
> >  > > https://github.com/sphinx-doc/sphinx/issues/11239
> > 
> >  > >
> >  > > Signed-off-by: Peter Maydell  > >
> >  > > ---
> >  > > NB: we need to land the fix for the current outstanding
> >  > > warning before this one can go in...
> >  > > 
> > https://lore.kernel.org/qemu-devel/20230310103123.2118519-11-alex.ben...@linaro.org/
> > 
> >  > > ---
> >  > >   docs/meson.build | 2 +-
> >  > >   docs/sphinx/kerneldoc.py | 5 +
> >  > >   2 files changed, 6 insertions(+), 1 deletion(-)
> >
> >  > I've applied it to my trivial-patches branch,
> >  > but if you want to apply it via some doc or misc branches, let me know.
> >
> > Trivial is fine, but make sure you've put in the fix for
> > the outstanding warning first :-)
>
> I didn't take this one but:
>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg949558.html

Yeah, that one has the same effect.

-- PMM

Re: dropping 32-bit host support

[Warner Losh]

On Thu, Mar 16, 2023 at 7:33 AM Thomas Huth  wrote:

> If you'd followed the QEMU project, you'd know that there are very helpful
> people around, from all kind of companies, Linaro guys who help with
> reviewing and merging non-ARM patches, Red Hatters who help with BSD

and Haiku patches, etc.
>

Without this help, bsd-user would be dead. As it is, it is struggling with
its own
resource issues, but the kind help I've received from the QEMU project has
motivated me to keep going in upstreaming what our fork has, as well as
working to make the code better.

I'll only add that FreeBSD's efforts to improve its CI story was derailed
for two
years by people like this, so it makes me happy to see lines being drawn
in this thread. They aren't unreasonable, and look to me to be in the best
interest of the QEMU project. You can't make everybody happy all the time.
And while it's good to try sometimes, other times it bogs down real efforts
to
make things better. This is one of those times.

Warner

Re: [PATCH] docs/sphinx/kerneldoc.py: Honour --enable-werror

[Laurent Vivier]

Le 16/03/2023 à 14:42, Peter Maydell a écrit :



On Thu, 16 Mar 2023 at 13:40, Laurent Vivier mailto:laur...@vivier.eu>> wrote:
 >
 > Le 14/03/2023 à 12:44, Peter Maydell a écrit :
 > > Currently, the kerneldoc Sphinx plugin doesn't honour the
 > > --enable-werror configure option, so its warnings are never fatal.
 > > This is because although we do pass sphinx-build the -W switch, the
 > > warnings from kerneldoc are produced by the scripts/kernel-doc script
 > > directly and don't go through Sphinx's "emit a warning" function.
 > >
 > > When --enable-werror is in effect, pass sphinx-build an extra
 > > argument -Dkerneldoc_werror=1.  The kerneldoc plugin can then use
 > > this to determine whether it should be passing the kernel-doc script
 > > -Werror.
 > >
 > > We do this because there is no documented mechanism for
 > > a Sphinx plugin to determine whether sphinx-build was
 > > passed -W or not; if one is provided then we can switch to
 > > that at a later date:
 > > https://github.com/sphinx-doc/sphinx/issues/11239 


 > >
 > > Signed-off-by: Peter Maydell mailto:peter.mayd...@linaro.org>>
 > > ---
 > > NB: we need to land the fix for the current outstanding
 > > warning before this one can go in...
 > > https://lore.kernel.org/qemu-devel/20230310103123.2118519-11-alex.ben...@linaro.org/ 


 > > ---
 > >   docs/meson.build         | 2 +-
 > >   docs/sphinx/kerneldoc.py | 5 +
 > >   2 files changed, 6 insertions(+), 1 deletion(-)

 > I've applied it to my trivial-patches branch,
 > but if you want to apply it via some doc or misc branches, let me know.

Trivial is fine, but make sure you've put in the fix for
the outstanding warning first :-)


I didn't take this one but:

https://www.mail-archive.com/qemu-devel@nongnu.org/msg949558.html

Is it ok?

Thanks,
Laurent

[PULL 1/7] migration: Wait on preempt channel in preempt thread

[Juan Quintela]

From: Peter Xu 

QEMU main thread will wait until dest preempt channel established during
processing the LISTEN command (within the whole postcopy PACKAGED data), by
waiting on the semaphore postcopy_qemufile_dst_done.

That's racy, because it's possible that the dest QEMU main thread hasn't
yet accept()ed the new connection when processing the LISTEN event.  The
sem_wait() will yield the main thread without being able to run anything
else including the accept() of the new socket, which can cause deadlock
within the main thread.

To avoid the race, move the "wait channel" from main thread to the preempt
thread right at the start.

Reported-by: Peter Maydell 
Fixes: 5655aab079 ("migration: Postpone postcopy preempt channel to be after 
main")
Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Juan Quintela 
Signed-off-by: Peter Xu 
Signed-off-by: Juan Quintela 
---
 migration/postcopy-ram.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index f54f44d899..41c0713650 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1197,11 +1197,6 @@ int postcopy_ram_incoming_setup(MigrationIncomingState 
*mis)
 }
 
 if (migrate_postcopy_preempt()) {
-/*
- * The preempt channel is established in asynchronous way.  Wait
- * for its completion.
- */
-qemu_sem_wait(>postcopy_qemufile_dst_done);
 /*
  * This thread needs to be created after the temp pages because
  * it'll fetch RAM_CHANNEL_POSTCOPY PostcopyTmpPage immediately.
@@ -1668,6 +1663,12 @@ void *postcopy_preempt_thread(void *opaque)
 
 qemu_sem_post(>thread_sync_sem);
 
+/*
+ * The preempt channel is established in asynchronous way.  Wait
+ * for its completion.
+ */
+qemu_sem_wait(>postcopy_qemufile_dst_done);
+
 /* Sending RAM_SAVE_FLAG_EOS to terminate this thread */
 qemu_mutex_lock(>postcopy_prio_thread_mutex);
 while (1) {
-- 
2.39.2

[PULL 4/7] migration/xbzrle: fix out-of-bounds write with axv512

[Juan Quintela]

From: Matheus Tavares Bernardino 

xbzrle_encode_buffer_avx512() checks for overflows too scarcely in its
outer loop, causing out-of-bounds writes:

$ ../configure --target-list=aarch64-softmmu --enable-sanitizers 
--enable-avx512bw
$ make tests/unit/test-xbzrle && ./tests/unit/test-xbzrle

==5518==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6210b100 
at pc 0x561109a7714d bp 0x7ffed712a440 sp 0x7ffed712a430
WRITE of size 1 at 0x6210b100 thread T0
#0 0x561109a7714c in uleb128_encode_small ../util/cutils.c:831
#1 0x561109b67f6a in xbzrle_encode_buffer_avx512 ../migration/xbzrle.c:275
#2 0x5611099a7428 in test_encode_decode_overflow 
../tests/unit/test-xbzrle.c:153
#3 0x7fb2fb65a58d  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d)
#4 0x7fb2fb65a333  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a333)
#5 0x7fb2fb65aa79 in g_test_run_suite 
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7aa79)
#6 0x7fb2fb65aa94 in g_test_run 
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7aa94)
#7 0x5611099a3a23 in main ../tests/unit/test-xbzrle.c:218
#8 0x7fb2fa78c082 in __libc_start_main 
(/lib/x86_64-linux-gnu/libc.so.6+0x24082)
#9 0x5611099a608d in _start (/qemu/build/tests/unit/test-xbzrle+0x28408d)

0x6210b100 is located 0 bytes to the right of 4096-byte region 
[0x6210a100,0x6210b100)
allocated by thread T0 here:
#0 0x7fb2fb823a06 in __interceptor_calloc 
../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153
#1 0x7fb2fb637ef0 in g_malloc0 
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57ef0)

Fix that by performing the overflow check in the inner loop, instead.

Signed-off-by: Matheus Tavares Bernardino 
Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
---
 migration/xbzrle.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/migration/xbzrle.c b/migration/xbzrle.c
index 21b92d4eae..c6f8b20917 100644
--- a/migration/xbzrle.c
+++ b/migration/xbzrle.c
@@ -197,10 +197,6 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t 
*new_buf, int slen,
 __m512i r = _mm512_set1_epi32(0);
 
 while (count512s) {
-if (d + 2 > dlen) {
-return -1;
-}
-
 int bytes_to_check = 64;
 uint64_t mask = 0x;
 if (count512s == 1) {
@@ -216,6 +212,9 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t 
*new_buf, int slen,
 
 bool is_same = (comp & 0x1);
 while (bytes_to_check) {
+if (d + 2 > dlen) {
+return -1;
+}
 if (is_same) {
 if (nzrun_len) {
 d += uleb128_encode_small(dst + d, nzrun_len);
-- 
2.39.2

[PULL 7/7] migration: fix populate_vfio_info

[Juan Quintela]

From: Steve Sistare 

Include CONFIG_DEVICES so that populate_vfio_info is instantiated for
CONFIG_VFIO.  Without it, the 'info migrate' command never returns
info about vfio.

Fixes: 43bd0bf30f ("migration: Move populate_vfio_info() into a separate file")
Signed-off-by: Steve Sistare 
Reviewed-by: Marc-André Lureau 
Reviewed-by: Thomas Huth 
Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
---
 migration/target.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/target.c b/migration/target.c
index 907ebf0a0a..00ca007f97 100644
--- a/migration/target.c
+++ b/migration/target.c
@@ -8,6 +8,7 @@
 #include "qemu/osdep.h"
 #include "qapi/qapi-types-migration.h"
 #include "migration.h"
+#include CONFIG_DEVICES
 
 #ifdef CONFIG_VFIO
 #include "hw/vfio/vfio-common.h"
@@ -17,7 +18,6 @@ void populate_vfio_info(MigrationInfo *info)
 {
 #ifdef CONFIG_VFIO
 if (vfio_mig_active()) {
-info->has_vfio = true;
 info->vfio = g_malloc0(sizeof(*info->vfio));
 info->vfio->transferred = vfio_mig_bytes_transferred();
 }
-- 
2.39.2

[PULL 5/7] migration/rdma: Remove deprecated variable rdma_return_path

[Juan Quintela]

From: Li Zhijian 

It's no longer needed since commit
44bcfd45e98 ("migration/rdma: destination: create the return patch after the 
first accept")

Signed-off-by: Li Zhijian 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
---
 migration/rdma.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 9d70e9885b..df646be35e 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -4111,7 +4111,7 @@ static void rdma_accept_incoming_migration(void *opaque)
 void rdma_start_incoming_migration(const char *host_port, Error **errp)
 {
 int ret;
-RDMAContext *rdma, *rdma_return_path = NULL;
+RDMAContext *rdma;
 Error *local_err = NULL;
 
 trace_rdma_start_incoming_migration();
@@ -4157,7 +4157,6 @@ err:
 g_free(rdma->host_port);
 }
 g_free(rdma);
-g_free(rdma_return_path);
 }
 
 void rdma_start_outgoing_migration(void *opaque,
-- 
2.39.2

[PULL 3/7] migration/xbzrle: use ctz64 to avoid undefined result

[Juan Quintela]

From: Matheus Tavares Bernardino 

__builtin_ctzll() produces undefined results when the argument is 0.
This can be seen through test-xbzrle, which produces the following
warning:

../migration/xbzrle.c:265: runtime error: passing zero to ctz(), which is not a 
valid argument

Replace __builtin_ctzll() with our ctz64() wrapper which properly
handles 0.

Signed-off-by: Matheus Tavares Bernardino 
Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
---
 migration/xbzrle.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/migration/xbzrle.c b/migration/xbzrle.c
index 05366e86c0..21b92d4eae 100644
--- a/migration/xbzrle.c
+++ b/migration/xbzrle.c
@@ -12,6 +12,7 @@
  */
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
+#include "qemu/host-utils.h"
 #include "xbzrle.h"
 
 /*
@@ -233,7 +234,7 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t 
*new_buf, int slen,
 break;
 }
 never_same = false;
-num = __builtin_ctzll(~comp);
+num = ctz64(~comp);
 num = (num < bytes_to_check) ? num : bytes_to_check;
 zrun_len += num;
 bytes_to_check -= num;
@@ -262,7 +263,7 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t 
*new_buf, int slen,
 nzrun_len += 64;
 break;
 }
-num = __builtin_ctzll(comp);
+num = ctz64(comp);
 num = (num < bytes_to_check) ? num : bytes_to_check;
 nzrun_len += num;
 bytes_to_check -= num;
-- 
2.39.2

[PULL 2/7] migration/rdma: Fix return-path case

[Juan Quintela]

From: "Dr. David Alan Gilbert" 

The RDMA code has return-path handling code, but it's only enabled
if postcopy is enabled; if the 'return-path' migration capability
is enabled, the return path is NOT setup but the core migration
code still tries to use it and breaks.

Enable the RDMA return path if either postcopy or the return-path
capability is enabled.

bz: https://bugzilla.redhat.com/show_bug.cgi?id=2063615

Signed-off-by: Dr. David Alan Gilbert 
Reviewed-by: Juan Quintela 
Reviewed-by: Li Zhijian 
Signed-off-by: Juan Quintela 
---
 migration/rdma.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 288eadc2d2..9d70e9885b 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3373,7 +3373,8 @@ static int qemu_rdma_accept(RDMAContext *rdma)
  * initialize the RDMAContext for return path for postcopy after first
  * connection request reached.
  */
-if (migrate_postcopy() && !rdma->is_return_path) {
+if ((migrate_postcopy() || migrate_use_return_path())
+&& !rdma->is_return_path) {
 rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL);
 if (rdma_return_path == NULL) {
 rdma_ack_cm_event(cm_event);
@@ -3455,7 +3456,8 @@ static int qemu_rdma_accept(RDMAContext *rdma)
 }
 
 /* Accept the second connection request for return path */
-if (migrate_postcopy() && !rdma->is_return_path) {
+if ((migrate_postcopy() || migrate_use_return_path())
+&& !rdma->is_return_path) {
 qemu_set_fd_handler(rdma->channel->fd, rdma_accept_incoming_migration,
 NULL,
 (void *)(intptr_t)rdma->return_path);
@@ -4192,7 +4194,7 @@ void rdma_start_outgoing_migration(void *opaque,
 }
 
 /* RDMA postcopy need a separate queue pair for return path */
-if (migrate_postcopy()) {
+if (migrate_postcopy() || migrate_use_return_path()) {
 rdma_return_path = qemu_rdma_data_init(host_port, errp);
 
 if (rdma_return_path == NULL) {
-- 
2.39.2

[PULL 6/7] migration/multifd: correct multifd_send_thread to trace the flags

[Juan Quintela]

From: Wei Wang 

The p->flags could be updated via the send_prepare callback, e.g. OR-ed
with MULTIFD_FLAG_ZLIB via zlib_send_prepare. Assign p->flags to the
local "flags" before the send_prepare callback could only get partial of
p->flags. Fix it by moving the assignment of p->flags to the local flags
after the callback, so that the correct flags can be traced.

Fixes: ab7cbb0b9a3b ("multifd: Make no compression operations into its own 
structure")
Signed-off-by: Wei Wang 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
---
 migration/multifd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 5e85c3ea9b..cbc0dfe39b 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -677,7 +677,7 @@ static void *multifd_send_thread(void *opaque)
 
 if (p->pending_job) {
 uint64_t packet_num = p->packet_num;
-uint32_t flags = p->flags;
+uint32_t flags;
 p->normal_num = 0;
 
 if (use_zero_copy_send) {
@@ -699,6 +699,7 @@ static void *multifd_send_thread(void *opaque)
 }
 }
 multifd_send_fill_packet(p);
+flags = p->flags;
 p->flags = 0;
 p->num_packets++;
 p->total_normal_pages += p->normal_num;
-- 
2.39.2

[PULL 0/7] Migration 20230316 patches

[Juan Quintela]

The following changes since commit 9636e513255362c4a329e3e5fb2c97dab3c5ce47:

  Merge tag 'misc-next-pull-request' of https://gitlab.com/berrange/qemu into 
staging (2023-03-15 17:20:04 +)

are available in the Git repository at:

  https://gitlab.com/juan.quintela/qemu.git tags/migration-20230316-pull-request

for you to fetch changes up to fa76c854ae837328187bef41d80af5d1ad36681f:

  migration: fix populate_vfio_info (2023-03-16 16:07:07 +0100)


Migration Pull request

Hi

This is just fixes for migration.
- Fix rdma (dave)
- Remove unused variable (Zhijian)
- Fix AVX512 and XBZRLE (Matheus)
- Fix migration preempt (Peter)
- Fix populate_vfio_info (Steve)
- Fix multifd send trace (Wei)

Please apply.

Later, Juan.



Dr. David Alan Gilbert (1):
  migration/rdma: Fix return-path case

Li Zhijian (1):
  migration/rdma: Remove deprecated variable rdma_return_path

Matheus Tavares Bernardino (2):
  migration/xbzrle: use ctz64 to avoid undefined result
  migration/xbzrle: fix out-of-bounds write with axv512

Peter Xu (1):
  migration: Wait on preempt channel in preempt thread

Steve Sistare (1):
  migration: fix populate_vfio_info

Wei Wang (1):
  migration/multifd: correct multifd_send_thread to trace the flags

 migration/multifd.c  |  3 ++-
 migration/postcopy-ram.c | 11 ++-
 migration/rdma.c | 11 ++-
 migration/target.c   |  2 +-
 migration/xbzrle.c   | 12 ++--
 5 files changed, 21 insertions(+), 18 deletions(-)

-- 
2.39.2

Re: [PATCH v3 2/3] qapi: Do not generate empty enum

[Markus Armbruster]

Daniel P. Berrangé  writes:

> On Thu, Mar 16, 2023 at 01:31:04PM +0100, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé  writes:
>> 
>> > Per the C++ standard, empty enum are ill-formed. Do not generate

The C standard.  The C++ standard doesn't apply here :)

>> > them in order to avoid:
>> >
>> >   In file included from qga/qga-qapi-emit-events.c:14:
>> >   qga/qga-qapi-emit-events.h:20:1: error: empty enum is invalid
>> >  20 | } qga_QAPIEvent;
>> > | ^
>> >
>> > Reported-by: Markus Armbruster 
>> > Signed-off-by: Philippe Mathieu-Daudé 
>> 
>> Two failures in "make check-qapi-schema" (which is run by "make check"):
>> 
>> 1. Positive test case qapi-schema-test
>> 
>> --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/qapi-schema-test.out
>> +++ 
>> @@ -19,7 +19,6 @@
>>  member enum2: EnumOne optional=True
>>  member enum3: EnumOne optional=False
>>  member enum4: EnumOne optional=True
>> -enum MyEnum
>>  object Empty1
>>  object Empty2
>>  base Empty1
>> 
>>You forgot to update expected test output.  No big deal.
>> 
>> 2. Negative test case union-empty
>> 
>> --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/union-empty.err
>> +++ 
>> @@ -1,2 +1,2 @@
>> -union-empty.json: In union 'Union':
>> -union-empty.json:4: union has no branches
>> +union-empty.json: In struct 'Base':
>> +union-empty.json:3: member 'type' uses unknown type 'Empty'
>> stderr:
>> qapi-schema-test FAIL
>> union-empty FAIL
>> 
>>The error message regresses.
>> 
>>I can see two ways to fix this:
>> 
>>(A) You can't just drop empty enumeration types on the floor.  To not
>>generate code for them, you need to skip them wherever we
>>generate code for enumeration types.
>> 
>>(B) Outlaw empty enumeration types.
>> 
>> I recommend to give (B) a try, it's likely simpler.
>
> Possible trap-door with (B), if we have any enums where *every*
> member is conditionalized on a CONFIG_XXX rule, there might be
> certain build scenarios where an enum suddenly becomes empty.

True.  Scratch the idea.

Trap-door also applies to (A): we can still end up with empty enums.

(C) Always emit a dummy member.  This is actually what we do now:

typedef enum OnOffAuto {
ON_OFF_AUTO_AUTO = 1,
ON_OFF_AUTO_ON = 2,
ON_OFF_AUTO_OFF = 3,
ON_OFF_AUTO__MAX,   <--- the dummy
} OnOffAuto;

But the next patch changes it to

typedef enum OnOffAuto {
ON_OFF_AUTO_AUTO,
ON_OFF_AUTO_ON,
ON_OFF_AUTO_OFF,
#define ON_OFF_AUTO__MAX 3
} OnOffAuto;

Two problems, actually.

One, we lose the dummy.  We could add one back like

typedef enum OnOffAuto {
ON_OFF_AUTO__DUMMY = 0,
ON_OFF_AUTO_AUTO = 0,
ON_OFF_AUTO_ON,
ON_OFF_AUTO_OFF,
#define ON_OFF_AUTO__MAX 3
} OnOffAuto;

But all of this falls apart with conditional members!

Example 1 (taken from qapi/block-core.json):

{ 'enum': 'BlockdevAioOptions',
  'data': [ 'threads', 'native',
{ 'name': 'io_uring', 'if': 'CONFIG_LINUX_IO_URING' } ] }

Generates now:

typedef enum BlockdevAioOptions {
BLOCKDEV_AIO_OPTIONS_THREADS,
BLOCKDEV_AIO_OPTIONS_NATIVE,
#if defined(CONFIG_LINUX_IO_URING)
BLOCKDEV_AIO_OPTIONS_IO_URING,
#endif /* defined(CONFIG_LINUX_IO_URING) */
BLOCKDEV_AIO_OPTIONS__MAX,
} BlockdevAioOptions;

BLOCKDEV_AIO_OPTIONS__MAX is 3 if defined(CONFIG_LINUX_IO_URING), else
2.

After the next patch:

typedef enum BlockdevAioOptions {
BLOCKDEV_AIO_OPTIONS_THREADS,
BLOCKDEV_AIO_OPTIONS_NATIVE,
#if defined(CONFIG_LINUX_IO_URING)
BLOCKDEV_AIO_OPTIONS_IO_URING,
#endif /* defined(CONFIG_LINUX_IO_URING) */
#define BLOCKDEV_AIO_OPTIONS__MAX 3
} BlockdevAioOptions;

Now it's always 3.

Example 2 (same with members reordered):

{ 'enum': 'BlockdevAioOptions',
  'data': [ { 'name': 'io_uring', 'if': 'CONFIG_LINUX_IO_URING' },
'threads', 'native' ] }

Same problem for __MAX, additional problem for __DUMMY:

typedef enum BlockdevAioOptions {
BLOCKDEV_AIO_OPTIONS__DUMMY = 0,
#if defined(CONFIG_LINUX_IO_URING)
BLOCKDEV_AIO_OPTIONS_IO_URING = 0,
#endif /* defined(CONFIG_LINUX_IO_URING) */
BLOCKDEV_AIO_OPTIONS_THREADS,
BLOCKDEV_AIO_OPTIONS_NATIVE,
#define BLOCKDEV_AIO_OPTIONS__MAX 3
} BlockdevAioOptions;

If CONFIG_LINUX_IO_URING is off, the enum starts at 1 instead of 0.

Arrays indexed by the enum start with a hole.  Code using them is
probably not prepared for holes.

*Sigh*

Re: [PATCH v3 1/5] hw/usb: Add basic i.MX USB Phy support

[Peter Maydell]

On Thu, 16 Mar 2023 at 14:12, Guenter Roeck  wrote:
>
> Hi Peter,
>
> On 3/16/23 06:41, Peter Maydell wrote:
> > On Fri, 13 Mar 2020 at 01:45, Guenter Roeck  wrote:
> >>
> >> Add basic USB PHY support as implemented in i.MX23, i.MX28, i.MX6,
> >> and i.MX7 SoCs.
> >>
> >> The only support really needed - at least to boot Linux - is support
> >> for soft reset, which needs to reset various registers to their initial
> >> value. Otherwise, just record register values.
> >>
> >> Reviewed-by: Peter Maydell 
> >> Signed-off-by: Guenter Roeck 
> >
> > Hi Guenter; we've had a fuzzer report that this device model
> > accesses off the end of the usbphy[] array:
> > https://gitlab.com/qemu-project/qemu/-/issues/1408
> >
>
> Good catch. And an obvious bug, sorry.


>
> > Do you know what the device is supposed to do with these
> > off-the-end acceses? We could either reduce the memory region
> > size or bounds check and RAZ/WI the out-of-range accesses.
> >
>
> I have no idea what the real hardware would do. The datasheets (at
> least the ones I checked) don't say, only that the region size is 4k.
> I would suggest a bounds check, ignore out-of-bounds writes (maybe
> with a log message), and return 0 for reads (which I think is what
> you suggest with RAZ/WI).
>
> Want me to send a patch ?

If you have the time, that would be great. I expect you're
better set up to test it than I am...

thanks
-- PMM

Re: [PATCH for 8.0] igb: Save the entire Tx context descriptor

[Juan Quintela]

Philippe Mathieu-Daudé  wrote:
> On 16/3/23 13:40, Akihiko Odaki wrote:
>> On 2023/03/16 21:36, Philippe Mathieu-Daudé wrote:
>>> On 16/3/23 13:28, Akihiko Odaki wrote:
 The current implementation of igb uses only part of a advanced Tx
 context descriptor because it misses some features and sniffs the trait
 of the packet instead of respecting the packet type specified in the
 descriptor. However, we will certainly need the entire Tx context
 descriptor when we update igb to respect these ignored fields. Save the
 entire Tx context descriptor to prepare for such a change.

 Signed-off-by: Akihiko Odaki 
 ---
   hw/net/igb.c  |  6 --
   hw/net/igb_core.c | 17 ++---
   hw/net/igb_core.h |  3 +--
   3 files changed, 15 insertions(+), 11 deletions(-)

 diff --git a/hw/net/igb.c b/hw/net/igb.c
 index 0792626322..50239a7cb1 100644
 --- a/hw/net/igb.c
 +++ b/hw/net/igb.c
 @@ -499,8 +499,10 @@ static const VMStateDescription igb_vmstate_tx = {
   .version_id = 1,
>>>
>>> Don't we need to increment the vmstate version? See
>>> https://qemu-project.gitlab.io/qemu/devel/migration.html#versions
>> This device is added only a week ago so it shouldn't need version
>> bump. That is also why I tagged this change "for 8.0".
>
> Well it is cheaper than dealing with partially backported commits...
> Also could be a better example for future developers IMHO. My 2 cents.

You can't have everything O:-)

I would just bump the version and not do the "dance" where you can
migrate from v1 and v2.  I.e. don't add tests at all.

This way bisect will fail with the correct message.

Later, Juan.

Re: [PATCH v3 2/3] qapi: Do not generate empty enum

[Daniel P . Berrangé]

On Thu, Mar 16, 2023 at 03:39:59PM +0100, Juan Quintela wrote:
> Daniel P. Berrangé  wrote:
> > On Thu, Mar 16, 2023 at 01:31:04PM +0100, Markus Armbruster wrote:
> >> Philippe Mathieu-Daudé  writes:
> >> 
> >> > Per the C++ standard, empty enum are ill-formed. Do not generate
> >> > them in order to avoid:
> >> >
> >> >   In file included from qga/qga-qapi-emit-events.c:14:
> >> >   qga/qga-qapi-emit-events.h:20:1: error: empty enum is invalid
> >> >  20 | } qga_QAPIEvent;
> >> > | ^
> >> >
> >> > Reported-by: Markus Armbruster 
> >> > Signed-off-by: Philippe Mathieu-Daudé 
> >> 
> >> Two failures in "make check-qapi-schema" (which is run by "make check"):
> >> 
> >> 1. Positive test case qapi-schema-test
> >> 
> >> --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/qapi-schema-test.out
> >> +++ 
> >> @@ -19,7 +19,6 @@
> >>  member enum2: EnumOne optional=True
> >>  member enum3: EnumOne optional=False
> >>  member enum4: EnumOne optional=True
> >> -enum MyEnum
> >>  object Empty1
> >>  object Empty2
> >>  base Empty1
> >> 
> >>You forgot to update expected test output.  No big deal.
> >> 
> >> 2. Negative test case union-empty
> >> 
> >> --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/union-empty.err
> >> +++ 
> >> @@ -1,2 +1,2 @@
> >> -union-empty.json: In union 'Union':
> >> -union-empty.json:4: union has no branches
> >> +union-empty.json: In struct 'Base':
> >> +union-empty.json:3: member 'type' uses unknown type 'Empty'
> >> stderr:
> >> qapi-schema-test FAIL
> >> union-empty FAIL
> >> 
> >>The error message regresses.
> >> 
> >>I can see two ways to fix this:
> >> 
> >>(A) You can't just drop empty enumeration types on the floor.  To not
> >>generate code for them, you need to skip them wherever we
> >>generate code for enumeration types.
> >> 
> >>(B) Outlaw empty enumeration types.
> >> 
> >> I recommend to give (B) a try, it's likely simpler.
> >
> > Possible trap-door with (B), if we have any enums where *every*
> > member is conditionalized on a CONFIG_XXX rule, there might be
> > certain build scenarios where an enum suddenly becomes empty.
> 
> Do we have an example for this?
> Because it looks really weird.  I would expect that the "container" unit
> of that enumeration is #ifdef out of compilation somehow.

I'm not sure if such an example physically exists. I know the  audio
code gets close, with all but 2 options conditional:

{ 'enum': 'AudiodevDriver',
  'data': [ 'none',
{ 'name': 'alsa', 'if': 'CONFIG_AUDIO_ALSA' },
{ 'name': 'coreaudio', 'if': 'CONFIG_AUDIO_COREAUDIO' },
{ 'name': 'dbus', 'if': 'CONFIG_DBUS_DISPLAY' },
{ 'name': 'dsound', 'if': 'CONFIG_AUDIO_DSOUND' },
{ 'name': 'jack', 'if': 'CONFIG_AUDIO_JACK' },
{ 'name': 'oss', 'if': 'CONFIG_AUDIO_OSS' },
{ 'name': 'pa', 'if': 'CONFIG_AUDIO_PA' },
{ 'name': 'sdl', 'if': 'CONFIG_AUDIO_SDL' },
{ 'name': 'sndio', 'if': 'CONFIG_AUDIO_SNDIO' },
{ 'name': 'spice', 'if': 'CONFIG_SPICE' },
'wav' ] }

Just wanted to warn that we shouldn't assume empty enums can't
exist, because it would be quite easy to add 2 extra conditionals
to this audio example, and the enum wouldn't appear empty at a
glance, but none the less could be empty in some compile scenarios

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v3 2/3] qapi: Do not generate empty enum

[Juan Quintela]

Daniel P. Berrangé  wrote:
> On Thu, Mar 16, 2023 at 01:31:04PM +0100, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé  writes:
>> 
>> > Per the C++ standard, empty enum are ill-formed. Do not generate
>> > them in order to avoid:
>> >
>> >   In file included from qga/qga-qapi-emit-events.c:14:
>> >   qga/qga-qapi-emit-events.h:20:1: error: empty enum is invalid
>> >  20 | } qga_QAPIEvent;
>> > | ^
>> >
>> > Reported-by: Markus Armbruster 
>> > Signed-off-by: Philippe Mathieu-Daudé 
>> 
>> Two failures in "make check-qapi-schema" (which is run by "make check"):
>> 
>> 1. Positive test case qapi-schema-test
>> 
>> --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/qapi-schema-test.out
>> +++ 
>> @@ -19,7 +19,6 @@
>>  member enum2: EnumOne optional=True
>>  member enum3: EnumOne optional=False
>>  member enum4: EnumOne optional=True
>> -enum MyEnum
>>  object Empty1
>>  object Empty2
>>  base Empty1
>> 
>>You forgot to update expected test output.  No big deal.
>> 
>> 2. Negative test case union-empty
>> 
>> --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/union-empty.err
>> +++ 
>> @@ -1,2 +1,2 @@
>> -union-empty.json: In union 'Union':
>> -union-empty.json:4: union has no branches
>> +union-empty.json: In struct 'Base':
>> +union-empty.json:3: member 'type' uses unknown type 'Empty'
>> stderr:
>> qapi-schema-test FAIL
>> union-empty FAIL
>> 
>>The error message regresses.
>> 
>>I can see two ways to fix this:
>> 
>>(A) You can't just drop empty enumeration types on the floor.  To not
>>generate code for them, you need to skip them wherever we
>>generate code for enumeration types.
>> 
>>(B) Outlaw empty enumeration types.
>> 
>> I recommend to give (B) a try, it's likely simpler.
>
> Possible trap-door with (B), if we have any enums where *every*
> member is conditionalized on a CONFIG_XXX rule, there might be
> certain build scenarios where an enum suddenly becomes empty.

Do we have an example for this?
Because it looks really weird.  I would expect that the "container" unit
of that enumeration is #ifdef out of compilation somehow.

Later, Juan.

Re: [PATCH V2 01/20] migration: fix populate_vfio_info

[Juan Quintela]

Steve Sistare  wrote:
> Include CONFIG_DEVICES so that populate_vfio_info is instantiated for
> CONFIG_VFIO.  Without it, the 'info migrate' command never returns
> info about vfio.
>
> Fixes: 43bd0bf30f ("migration: Move populate_vfio_info() into a separate 
> file")
> Signed-off-by: Steve Sistare 
> Reviewed-by: Marc-André Lureau 

Reviewed-by: Juan Quintela 

queued.