date:20210330

Re: [PATCH] docs: Add a QEMU Code of Conduct and Conflict Resolution Policy document

2021-03-30 Thread Thomas Huth

On 29/03/2021 22.59, Paolo Bonzini wrote:

Il lun 29 mar 2021, 20:33 Daniel P. Berrangé > ha scritto:

The obvious alternative is to import the contributor covenant

https://www.contributor-covenant.org/

The Contributor Covenant 1.x and 2.x are very different in that 2.x also 
includes conflict resolution. Unlike the code of conduct, the consequences 
of bad behavior are hard to generalize across multiple projects, so I would 
prefer anyway the 1.x version. The differences with the Django CoC aren't 
substantial.

Right. I also think we should use a code of conduct that allows us to keep 
the conflict resolution in a separate document.

Contributor Covenant 1.x is certainly an option, too, but it has IMHO 
already quite rigorous language ("Project maintainers have the [...] 
responsibility to remove, edit, or reject comments, commits, code, wiki 
edits ...", "Project maintainers who do not [...] enforce the Code of 
Conduct may be permanently removed from the project team."), which could 
either scare away people from taking maintainers responsibility or also 
could be used fire up arguments ("you are a maintainer, now according to the 
CoC you have to do this and that..."), which I'd rather like to avoid.
(well, as you know, I'm not a native English speaker, so I might also have 
gotten that tone wrong, but that's the impression that I had after reading 
that text as non-native speaker).

That's why I'd rather prefer the Django CoC instead.

However this does mean being more careful about the language in the "custom" 
documents such as the conflict resolution policy.

The second, it isn't a static document. It is being evolved over
time with new versions issued as understanding of problematic
situations evolves. We can choose to periodically update to stay
current with the broadly accepted norms.

This however has the same issues as the "or later" clause of the GPL (see 
the above example of 1.x vs 2.x for the Contributor Covenant). I don't think 
upgrade of the CoC should be automatic since there are no "compatibility" 
issues.

Agreed. We shouldn't auto-upgrade to a newer version of a CoC without 
reviewing the new clauses.

 > +If you are experiencing conflict, you should first address the perceived
 > +conflict directly with other involved parties, preferably through a
 > +real-time medium such as IRC. If this fails,

I agree with Daniel that this part should only be advisory. For example:

If you are experiencing conflict, please consider first addressing the 
perceived  conflict directly with other involved parties, preferably through 
a real-time medium such as IRC. If this fails or if you do not feel 
comfortable proceeding this way,...

Also this document doesn't mention anything about ensuring the
confidentiality/privacy for any complaints reported, which I
think is important to state explicitly.

Agreed, and also the part about keeping a record should be removed from the 
consequences part because it's a privacy regulation minefield.

Ok, thanks for the feedback, I'll try to incorporate it and send a v2.

 Thomas

Re: [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer

2021-03-30 Thread Mark Cave-Ayland


On 18/03/2021 18:13, Paolo Bonzini wrote:


On 18/03/21 00:02, Mark Cave-Ayland wrote:

Recently there have been a number of issues raised on Launchpad as a result of
fuzzing the am53c974 (ESP) device. I spent some time over the past couple of
days checking to see if anything had improved since my last patchset: from
what I can tell the issues are still present, but the cmdfifo related failures
now assert rather than corrupting memory.

This patchset applied to master passes my local tests using the qtest fuzz test
cases added by Alexander for the following Launchpad bugs:

   https://bugs.launchpad.net/qemu/+bug/1919035
   https://bugs.launchpad.net/qemu/+bug/1919036
   https://bugs.launchpad.net/qemu/+bug/1910723
   https://bugs.launchpad.net/qemu/+bug/1909247
I'm posting this now just before soft freeze since I see that some of the issues
have recently been allocated CVEs and so it could be argued that even though
they have existed for some time, it is worth fixing them for 6.0.

Signed-off-by: Mark Cave-Ayland 

v2:
- Add Alexander's R-B tag for patch 2 and Phil's R-B for patch 3
- Add patch 4 for additional testcase provided in Alexander's patch 1 comment
- Move current_req NULL checks forward in DMA functions (fixes ASAN bug reported
   at https://bugs.launchpad.net/qemu/+bug/1909247/comments/6) in patch 3
- Add qtest for am53c974 containing a basic set of regression tests using the
   automatic test cases generated by the fuzzer as requested by Paolo


Mark Cave-Ayland (6):
   esp: don't underflow cmdfifo if no message out/command data is present
   esp: don't overflow cmdfifo if TC is larger than the cmdfifo size
   esp: ensure cmdfifo is not empty and current_dev is non-NULL
   esp: don't underflow fifo when writing to the device
   esp: always check current_req is not NULL before use in DMA callbacks
   tests/qtest: add tests for am53c974 device

  hw/scsi/esp.c   |  73 +
  tests/qtest/am53c974-test.c | 122 
  tests/qtest/meson.build |   1 +
  3 files changed, 171 insertions(+), 25 deletions(-)
  create mode 100644 tests/qtest/am53c974-test.c



Queued, thanks.

Paolo


Hi Paolo,

I had a quick look at Alex's updated test cases and most of them are based on an 
incorrect assumption I made around the behaviour of fifo8_pop_buf(). Can you drop 
these for now, and I will submit a v3 shortly once I've given it a full run through 
my test images?



ATB,

Mark.

Re: [PATCH] ppc/spapr: Add support for implement support for H_SCM_HEALTH

2021-03-30 Thread Shivaprasad G Bhat


Hi Vaibhav,

Some comments inline..

On 3/29/21 9:52 PM, Vaibhav Jain wrote:

Add support for H_SCM_HEALTH hcall described at [1] for spapr
nvdimms. This enables guest to detect the 'unarmed' status of a
specific spapr nvdimm identified by its DRC and if its unarmed, mark
the region backed by the nvdimm as read-only.

The patch adds h_scm_health() to handle the H_SCM_HEALTH hcall which
returns two 64-bit bitmaps (health bitmap, health bitmap mask) derived
from 'struct nvdimm->unarmed' member.

Linux kernel side changes to enable handling of 'unarmed' nvdimms for
ppc64 are proposed at [2].

References:
[1] "Hypercall Op-codes (hcalls)"
 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/powerpc/papr_hcalls.rst

[2] "powerpc/papr_scm: Mark nvdimm as unarmed if needed during probe"
 
https://lore.kernel.org/linux-nvdimm/20210329113103.476760-1-vaib...@linux.ibm.com/

Signed-off-by: Vaibhav Jain 
---
  hw/ppc/spapr_nvdimm.c  | 30 ++
  include/hw/ppc/spapr.h |  4 ++--
  2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index b46c36917c..e38740036d 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -31,6 +31,13 @@
  #include "qemu/range.h"
  #include "hw/ppc/spapr_numa.h"
  
+/* DIMM health bitmap bitmap indicators */

+/* SCM device is unable to persist memory contents */
+#define PAPR_PMEM_UNARMED (1ULL << (63 - 0))
+
+/* Bits status indicators for health bitmap indicating unarmed dimm */
+#define PAPR_PMEM_UNARMED_MASK (PAPR_PMEM_UNARMED)
+
  bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm,
 uint64_t size, Error **errp)
  {
@@ -467,6 +474,28 @@ static target_ulong h_scm_unbind_all(PowerPCCPU *cpu, 
SpaprMachineState *spapr,
  return H_SUCCESS;
  }
  
+static target_ulong h_scm_health(PowerPCCPU *cpu, SpaprMachineState *spapr,

+ target_ulong opcode, target_ulong *args)
+{
+uint32_t drc_index = args[0];
+SpaprDrc *drc = spapr_drc_by_index(drc_index);
+NVDIMMDevice *nvdimm;
+
+if (drc && spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PMEM) {
+return H_PARAMETER;
+}
+



Please check if drc->dev is not NULL too. DRCs are created in advance

and drc->dev may not be assigned if the device is not plugged yet.



+nvdimm = NVDIMM(drc->dev);
+
+/* Check if the nvdimm is unarmed and send its status via health bitmaps */
+args[0] = nvdimm->unarmed ? PAPR_PMEM_UNARMED_MASK : 0;



Please use object_property_get_bool to fetch the unarmed value.



+
+/* health bitmap mask same as the health bitmap */
+args[1] = args[0];
+
+return H_SUCCESS;
+}
+
  static void spapr_scm_register_types(void)
  {


...


Thanks,

Shivaprasad

[PING] [PATCH] [NFC] Mark locally used symbols as static.

2021-03-30 Thread Yuri Gribov

Hi all,

This patch makes locally used symbols static to enable more compiler
optimizations on them. Some of the symbols turned out to not be used
at all so I marked them with ATTRIBUTE_UNUSED (as I wasn't sure if
they were ok to delete).

The symbols have been identified with a pet project of mine:
https://github.com/yugr/Localizer

Link to patch: 
https://patchew.org/QEMU/cajotw+5ddmsr8qjqxaa1oht79rpmjcrwkybuartynr_ngux...@mail.gmail.com/

>From 4e790fd06becfbbf6fb106ac52ae1e4515f1ac73 Mon Sep 17 00:00:00 2001
From: Yury Gribov 
Date: Sat, 20 Mar 2021 23:39:15 +0300
Subject: [PATCH] Mark locally used symbols as static.

Signed-off-by: Yury Gribov 
Acked-by: Max Filippov  (xtensa)
Acked-by: David Gibson  (ppc)
Reviewed-by: Stefan Hajnoczi  (tracetool)
Reviewed-by: Taylor Simpson  (hexagon)
---
 disas/alpha.c | 16 ++--
 disas/m68k.c  | 78 -
 disas/mips.c  | 14 ++--
 disas/nios2.c | 84 +--
 disas/ppc.c   | 26 +++---
 disas/riscv.c |  2 +-
 pc-bios/optionrom/linuxboot_dma.c |  4 +-
 scripts/tracetool/format/c.py |  2 +-
 target/hexagon/gen_dectree_import.c   |  2 +-
 target/hexagon/opcodes.c  |  2 +-
 target/i386/cpu.c |  2 +-
 target/s390x/cpu_models.c |  2 +-
 .../xtensa/core-dc232b/xtensa-modules.c.inc   |  2 +-
 .../xtensa/core-dc233c/xtensa-modules.c.inc   |  2 +-
 target/xtensa/core-de212/xtensa-modules.c.inc |  2 +-
 .../core-de233_fpu/xtensa-modules.c.inc   |  2 +-
 .../xtensa/core-dsp3400/xtensa-modules.c.inc  |  2 +-
 target/xtensa/core-fsf/xtensa-modules.c.inc   |  2 +-
 .../xtensa-modules.c.inc  |  2 +-
 .../core-test_kc705_be/xtensa-modules.c.inc   |  2 +-
 .../core-test_mmuhifi_c3/xtensa-modules.c.inc |  2 +-
 21 files changed, 125 insertions(+), 127 deletions(-)

diff --git a/disas/alpha.c b/disas/alpha.c
index 3db90fa665..361a4ed101 100644
--- a/disas/alpha.c
+++ b/disas/alpha.c
@@ -56,8 +56,8 @@ struct alpha_opcode
 /* The table itself is sorted by major opcode number, and is otherwise
in the order in which the disassembler should consider
instructions.  */
-extern const struct alpha_opcode alpha_opcodes[];
-extern const unsigned alpha_num_opcodes;
+static const struct alpha_opcode alpha_opcodes[];
+static const unsigned alpha_num_opcodes;

 /* Values defined for the flags field of a struct alpha_opcode.  */

@@ -137,8 +137,8 @@ struct alpha_operand
 /* Elements in the table are retrieved by indexing with values from
the operands field of the alpha_opcodes table.  */

-extern const struct alpha_operand alpha_operands[];
-extern const unsigned alpha_num_operands;
+static const struct alpha_operand alpha_operands[];
+static const unsigned alpha_num_operands;

 /* Values defined for the flags field of a struct alpha_operand.  */

@@ -293,7 +293,7 @@ static int extract_ev6hwjhint (unsigned, int *);
 
 /* The operands table  */

-const struct alpha_operand alpha_operands[] =
+static const struct alpha_operand alpha_operands[] =
 {
   /* The fields are bits, shift, insert, extract, flags */
   /* The zero index is used to indicate end-of-list */
@@ -424,7 +424,7 @@ const struct alpha_operand alpha_operands[] =
 insert_ev6hwjhint, extract_ev6hwjhint }
 };

-const unsigned alpha_num_operands =
sizeof(alpha_operands)/sizeof(*alpha_operands);
+static ATTRIBUTE_UNUSED const unsigned alpha_num_operands =
sizeof(alpha_operands)/sizeof(*alpha_operands);

 /* The RB field when it is the same as the RA field in the same insn.
This operand is marked fake.  The insertion function just copies
@@ -706,7 +706,7 @@ extract_ev6hwjhint(unsigned insn, int *invalid
ATTRIBUTE_UNUSED)
that were not assigned to a particular extension.
 */

-const struct alpha_opcode alpha_opcodes[] = {
+static const struct alpha_opcode alpha_opcodes[] = {
   { "halt",SPCD(0x00,0x), BASE, ARG_NONE },
   { "draina",  SPCD(0x00,0x0002), BASE, ARG_NONE },
   { "bpt", SPCD(0x00,0x0080), BASE, ARG_NONE },
@@ -1732,7 +1732,7 @@ const struct alpha_opcode alpha_opcodes[] = {
   { "bgt", BRA(0x3F), BASE, ARG_BRA },
 };

-const unsigned alpha_num_opcodes =
sizeof(alpha_opcodes)/sizeof(*alpha_opcodes);
+static ATTRIBUTE_UNUSED const unsigned alpha_num_opcodes =
sizeof(alpha_opcodes)/sizeof(*alpha_opcodes);

 /* OSF register names.  */

diff --git a/disas/m68k.c b/disas/m68k.c
index aefaecfbd6..903d5cfec4 100644
--- a/disas/m68k.c
+++ b/disas/m68k.c
@@ -95,29 +95,29 @@ struct floatformat

 /* floatformats for IEEE single and double, big and little endian.  */

-extern const struct floatformat floatformat_ieee_single_big;
-extern const struct floatformat floatformat_ieee_single_little;
-extern const struct floatformat

Re: [PATCH for-6.0 1/7] hw/block/nvme: fix pi constraint check

2021-03-30 Thread Klaus Jensen

On Mar 29 19:52, Gollu Appalanaidu wrote:
> On Wed, Mar 24, 2021 at 09:09:01PM +0100, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > Protection Information can only be enabled if there is at least 8 bytes
> > of metadata.
> > 
> > Signed-off-by: Klaus Jensen 
> > ---
> > hw/block/nvme-ns.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> > index 7f8d139a8663..ca04ee1bacfb 100644
> > --- a/hw/block/nvme-ns.c
> > +++ b/hw/block/nvme-ns.c
> > @@ -394,7 +394,7 @@ static int nvme_ns_check_constraints(NvmeNamespace *ns, 
> > Error **errp)
> > return -1;
> > }
> > 
> > -if (ns->params.pi && !ns->params.ms) {
> > +if (ns->params.pi && ns->params.ms < 8) {
> and also it is good check that "metadata size" is power of 2 or not?
> 

While I don't expect a lot of real-world devices having metadata sizes
that are not power of twos, there is no requirement in the spec for
that.

And the implementation here also does not require it :)


signature.asc
Description: PGP signature

Re: [PATCH] ppc/spapr: Add support for implement support for H_SCM_HEALTH

2021-03-30 Thread Vaibhav Jain

Hi Shiva,

Thanks for reviweing this patch. My responses inline below;


Shivaprasad G Bhat  writes:



>>   
>> +static target_ulong h_scm_health(PowerPCCPU *cpu, SpaprMachineState *spapr,
>> + target_ulong opcode, target_ulong *args)
>> +{
>> +uint32_t drc_index = args[0];
>> +SpaprDrc *drc = spapr_drc_by_index(drc_index);
>> +NVDIMMDevice *nvdimm;
>> +
>> +if (drc && spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PMEM) {
>> +return H_PARAMETER;
>> +}
>> +
>
>
> Please check if drc->dev is not NULL too. DRCs are created in advance
>
> and drc->dev may not be assigned if the device is not plugged yet.
>
>
Sure, will address that in v2

>> +nvdimm = NVDIMM(drc->dev);
>> +
>> +/* Check if the nvdimm is unarmed and send its status via health 
>> bitmaps */
>> +args[0] = nvdimm->unarmed ? PAPR_PMEM_UNARMED_MASK : 0;
>
>
> Please use object_property_get_bool to fetch the unarmed value.
>
>
Sure I will switch to object_property_get_bool in v2. However I see
nvdimm->unarmed being accessed in similar manner in
nvdimm_build_structure_memdev() which probably needs an update too.



-- 
Cheers
~ Vaibhav

[PATCH v2] docs: Add a QEMU Code of Conduct and Conflict Resolution Policy document

2021-03-30 Thread Thomas Huth

In an ideal world, we would all get along together very well, always be
polite and never end up in huge conflicts. And even if there are conflicts,
we would always handle each other fair and respectfully. Unfortunately,
this is not an ideal world and sometimes people forget how to interact with
each other in a professional and respectful way. Fortunately, this rarely
happens in the QEMU community, but still there are such rare cases, and
then it would be good to have a basic code of conduct document available
that can be shown to persons who are misbehaving. And if that does not help
yet, we should also have a conflict resolution policy ready that can be
applied in the worst case.

The Code of Conduct document is based on the Django Code of Conduct
(https://www.djangoproject.com/conduct/) and the conflict resolution
has been assembled by Paolo, based on the Drupal Conflict Resolution Policy
(https://www.drupal.org/conflict-resolution) and the Mozilla Consequence Ladder
(https://github.com/mozilla/diversity/blob/master/code-of-conduct-enforcement/consequence-ladder.md)

Signed-off-by: Thomas Huth 
---
 I've picked the Django Code of Conduct as a base, since it sounds rather
 friendly and still welcoming to me, but I'm open for other suggestions, too
 (but we should maybe pick one where the conflict resolution policy is
 separated from the CoC itself so that it can be better taylored to the
 requirements of the QEMU project)

 v2: Adjusted the wording in the conflict resolution document according to
 the suggestions from Daniel and Paolo

 docs/devel/code-of-conduct.rst | 85 ++
 docs/devel/conflict-resolution.rst | 78 +++
 docs/devel/index.rst   |  2 +
 3 files changed, 165 insertions(+)
 create mode 100644 docs/devel/code-of-conduct.rst
 create mode 100644 docs/devel/conflict-resolution.rst

diff --git a/docs/devel/code-of-conduct.rst b/docs/devel/code-of-conduct.rst
new file mode 100644
index 00..050dbd9e16
--- /dev/null
+++ b/docs/devel/code-of-conduct.rst
@@ -0,0 +1,85 @@
+Code of Conduct
+===
+
+Like the technical community as a whole, the QEMU community is made up of a
+mixture of professionals and volunteers from all over the world.
+Diversity is one of our huge strengths, but it can also lead to communication
+issues and unhappiness. To that end, we have a few ground rules that we ask
+people to adhere to. This code applies equally to founders, maintainers,
+contributors, mentors and those seeking help and guidance.
+
+This isn't an exhaustive list of things that you can't do. Rather, take it in
+the spirit in which it's intended - a guide to make it easier to enrich all of
+us and the technical communities in which we participate:
+
+* Be friendly and patient.
+
+* Be welcoming. We strive to be a community that welcomes and supports people
+  of all backgrounds and identities. This includes, but is not limited to
+  members of any race, ethnicity, culture, national origin, colour, immigration
+  status, social and economic class, educational level, sex, sexual 
orientation,
+  gender identity and expression, age, size, family status, political belief,
+  religion, and mental and physical ability.
+
+* Be considerate. Your work will be used by other people, and you in turn will
+  depend on the work of others. Any decision you take will affect users and
+  colleagues, and you should take those consequences into account when making
+  decisions. Remember that we're a world-wide community, so you might not be
+  communicating in someone else's primary language.
+
+* Be respectful. Not all of us will agree all the time, but disagreement is no
+  excuse for poor behavior and poor manners. We might all experience some
+  frustration now and then, but we cannot allow that frustration to turn into
+  a personal attack. It's important to remember that a community where people
+  feel uncomfortable or threatened is not a productive one. Members of the QEMU
+  community should be respectful when dealing with other members as well as
+  with people outside the QEMU community.
+
+* Be careful in the words that you choose. We are a community of professionals,
+  and we conduct ourselves professionally. Be kind to others. Do not insult or
+  put down other participants. Harassment and other exclusionary behavior
+  aren't acceptable. This includes, but is not limited to:
+
+  * Violent threats or language directed against another person.
+
+  * Discriminatory jokes and language.
+
+  * Posting sexually explicit or violent material.
+
+  * Posting (or threatening to post) other people's personally identifying
+information ("doxing").
+
+  * Personal insults, especially those using racist or sexist terms.
+
+  * Unwelcome sexual attention.
+
+  * Advocating for, or encouraging, any of the above behavior.
+
+  * Repeated harassment of others. In general, if someone asks you to stop,
+then stop.
+
+* When we disagree,

Re: [PATCH] docs: Add a QEMU Code of Conduct and Conflict Resolution Policy document

2021-03-30 Thread Daniel P . Berrangé

On Mon, Mar 29, 2021 at 10:59:23PM +0200, Paolo Bonzini wrote:
> Il lun 29 mar 2021, 20:33 Daniel P. Berrangé  ha
> scritto:
> 
> > The obvious alternative is to import the contributor covenant
> >
> >   https://www.contributor-covenant.org/
> 
> 
> The Contributor Covenant 1.x and 2.x are very different in that 2.x also
> includes conflict resolution. Unlike the code of conduct, the consequences
> of bad behavior are hard to generalize across multiple projects, so I would
> prefer anyway the 1.x version. The differences with the Django CoC aren't
> substantial.
> 
> However this does mean being more careful about the language in the
> "custom" documents such as the conflict resolution policy.
> 
> 
> The second, it isn't a static document. It is being evolved over
> > time with new versions issued as understanding of problematic
> > situations evolves. We can choose to periodically update to stay
> > current with the broadly accepted norms.
> >
> 
> This however has the same issues as the "or later" clause of the GPL (see
> the above example of 1.x vs 2.x for the Contributor Covenant). I don't
> think upgrade of the CoC should be automatic since there are no
> "compatibility" issues.

Note, I didn't say we should automatically upgrade - I said we can
choose to upgrade. 


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 v5 5/5] virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr

2021-03-30 Thread Luis Henriques

On Mon, Mar 29, 2021 at 03:51:51PM -0400, Vivek Goyal wrote:
> On Mon, Mar 29, 2021 at 04:35:57PM +0100, Luis Henriques wrote:
> > On Thu, Mar 25, 2021 at 11:38:52AM -0400, Vivek Goyal wrote:
> > > When posix access acls are set on a file, it can lead to adjusting file
> > > permissions (mode) as well. If caller does not have CAP_FSETID and it
> > > also does not have membership of owner group, this will lead to clearing
> > > SGID bit in mode.
> > > 
> > > Current fuse code is written in such a way that it expects file server
> > > to take care of chaning file mode (permission), if there is a need.
> > > Right now, host kernel does not clear SGID bit because virtiofsd is
> > > running as root and has CAP_FSETID. For host kernel to clear SGID,
> > > virtiofsd need to switch to gid of caller in guest and also drop
> > > CAP_FSETID (if caller did not have it to begin with).
> > > 
> > > If SGID needs to be cleared, client will set the flag
> > > FUSE_SETXATTR_ACL_KILL_SGID in setxattr request. In that case server
> > > should kill sgid.
> > > 
> > > Currently just switch to uid/gid of the caller and drop CAP_FSETID
> > > and that should do it.
> > > 
> > > This should fix the xfstest generic/375 test case.
> > > 
> > > We don't have to switch uid for this to work. That could be one 
> > > optimization
> > > that pass a parameter to lo_change_cred() to only switch gid and not uid.
> > > 
> > > Also this will not work whenever (if ever) we support idmapped mounts. In
> > > that case it is possible that uid/gid in request are 0/0 but still we
> > > need to clear SGID. So we will have to pick a non-root sgid and switch
> > > to that instead. That's an TODO item for future when idmapped mount
> > > support is introduced.
> > > 
> > > Reported-by: Luis Henriques 
> > > Signed-off-by: Vivek Goyal 
> > > ---
> > >  include/standard-headers/linux/fuse.h |  7 +
> > >  tools/virtiofsd/passthrough_ll.c  | 42 +--
> > >  2 files changed, 47 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/standard-headers/linux/fuse.h 
> > > b/include/standard-headers/linux/fuse.h
> > > index cc87ff27d0..4eb79399d4 100644
> > > --- a/include/standard-headers/linux/fuse.h
> > > +++ b/include/standard-headers/linux/fuse.h
> > > @@ -180,6 +180,7 @@
> > >   *  - add FUSE_HANDLE_KILLPRIV_V2, FUSE_WRITE_KILL_SUIDGID, 
> > > FATTR_KILL_SUIDGID
> > >   *  - add FUSE_OPEN_KILL_SUIDGID
> > >   *  - add FUSE_SETXATTR_V2
> > > + *  - add FUSE_SETXATTR_ACL_KILL_SGID
> > >   */
> > >  
> > >  #ifndef _LINUX_FUSE_H
> > > @@ -450,6 +451,12 @@ struct fuse_file_lock {
> > >   */
> > >  #define FUSE_OPEN_KILL_SUIDGID   (1 << 0)
> > >  
> > > +/**
> > > + * setxattr flags
> > > + * FUSE_SETXATTR_ACL_KILL_SGID: Clear SGID when system.posix_acl_access 
> > > is set
> > > + */
> > > +#define FUSE_SETXATTR_ACL_KILL_SGID(1 << 0)
> > > +
> > >  enum fuse_opcode {
> > >   FUSE_LOOKUP = 1,
> > >   FUSE_FORGET = 2,  /* no reply */
> > > diff --git a/tools/virtiofsd/passthrough_ll.c 
> > > b/tools/virtiofsd/passthrough_ll.c
> > > index 3f5c267604..8a48071d0b 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -175,7 +175,7 @@ struct lo_data {
> > >  int user_killpriv_v2, killpriv_v2;
> > >  /* If set, virtiofsd is responsible for setting umask during 
> > > creation */
> > >  bool change_umask;
> > > -int user_posix_acl;
> > > +int user_posix_acl, posix_acl;
> > >  };
> > >  
> > >  static const struct fuse_opt lo_opts[] = {
> > > @@ -716,8 +716,10 @@ static void lo_init(void *userdata, struct 
> > > fuse_conn_info *conn)
> > >   * in fuse_lowlevel.c
> > >   */
> > >  fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
> > > -conn->want |= FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK;
> > > +conn->want |= FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK |
> > > +  FUSE_CAP_SETXATTR_V2;
> > 
> > An annoying thing with this is that if we're using a kernel without
> > _V2 support the mount will still succeed.  But we'll see:
> > 
> > ls: cannot access '/mnt': Connection refused
> > 
> > and in the userspace:
> > 
> > fuse: error: filesystem requested capabilities 0x2000 that are not 
> > supported by kernel, aborting.
> > 
> > Maybe it would be worth to automatically disable acl support if this
> > happens (with an error message) but still allow the filesystem to be
> > used.
> 
> If user specific "-o posix_acl" then it is better to fail explicitly
> if posix_acl can't be enabled. If user did not specify anything, then
> it makes sense to automatically disable posix acl  and continue.
> 
> > Or, which is probably better, to handle the EPROTO error in the
> > kernel during mount.
> 
> This will have been idea but in fuse, init process handling happens
> asynchronously. That is mount returns to user space while init
> command might complete at a later point of time. So

[PATCH] target/xtensa: fix core import to meson.build

2021-03-30 Thread Max Filippov

import_core.sh was not updated to change meson.build when new xtensa
core is imported. Fix that.

Cc: qemu-sta...@nongnu.org # v5.2.0
Signed-off-by: Max Filippov 
---
 target/xtensa/import_core.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/xtensa/import_core.sh b/target/xtensa/import_core.sh
index c8626a8c02eb..f3404039cc20 100755
--- a/target/xtensa/import_core.sh
+++ b/target/xtensa/import_core.sh
@@ -66,5 +66,5 @@ static XtensaConfig $NAME __attribute__((unused)) = {
 REGISTER_CORE($NAME)
 EOF
 
-grep -q core-${NAME}.o "$BASE"/Makefile.objs || \
-echo "obj-y += core-${NAME}.o" >> "$BASE"/Makefile.objs
+grep -q core-${NAME}.c "$BASE"/meson.build || \
+echo "xtensa_ss.add(files('core-${NAME}.c'))" >> "$BASE"/meson.build
-- 
2.20.1

[PATCH] target/xtensa: make xtensa_modules static on import

2021-03-30 Thread Max Filippov

xtensa_modules variable defined in each xtensa-modules.c.inc is only
used locally by the including file. Make it static.

Signed-off-by: Max Filippov 
---
 target/xtensa/import_core.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/xtensa/import_core.sh b/target/xtensa/import_core.sh
index f3404039cc20..53d3c4d099bb 100755
--- a/target/xtensa/import_core.sh
+++ b/target/xtensa/import_core.sh
@@ -35,6 +35,7 @@ tar -xf "$OVERLAY" -O binutils/xtensa-modules.c | \
 -e '/^#include "ansidecl.h"/d' \
 -e '/^Slot_[a-zA-Z0-9_]\+_decode (const xtensa_insnbuf insn)/,/^}/s/^  
return 0;$/  return XTENSA_UNDEFINED;/' \
 -e 's/#include /#include "xtensa-isa.h"/' \
+-e 's/^\(xtensa_isa_internal xtensa_modules\)/static \1/' \
 > "$TARGET"/xtensa-modules.c.inc
 
 cat < "${TARGET}.c"
-- 
2.20.1

Re: [PATCH v7 4/4] tests: Add tests for yank with the chardev-change case

2021-03-30 Thread Marc-André Lureau

Hi Lukas,

On Mon, Mar 29, 2021 at 10:55 PM Lukas Straub  wrote:

> Add tests for yank with the chardev-change case.
>
> Signed-off-by: Lukas Straub 
> Reviewed-by: Marc-André Lureau 
> Tested-by: Li Zhang 
> ---
>  MAINTAINERS|   1 +
>  tests/unit/meson.build |   3 +-
>  tests/unit/test-yank.c | 227 +
>  3 files changed, 230 insertions(+), 1 deletion(-)
>  create mode 100644 tests/unit/test-yank.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 77259c031d..accb683a55 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2821,6 +2821,7 @@ M: Lukas Straub 
>  S: Odd fixes
>  F: util/yank.c
>  F: migration/yank_functions*
> +F: tests/unit/test-yank.c
>  F: include/qemu/yank.h
>  F: qapi/yank.json
>
> diff --git a/tests/unit/meson.build b/tests/unit/meson.build
> index 4bfe4627ba..b3bc2109da 100644
> --- a/tests/unit/meson.build
> +++ b/tests/unit/meson.build
> @@ -123,7 +123,8 @@ if have_system
>  'test-util-sockets': ['socket-helpers.c'],
>  'test-base64': [],
>  'test-bufferiszero': [],
> -'test-vmstate': [migration, io]
> +'test-vmstate': [migration, io],
> +'test-yank': ['socket-helpers.c', qom, io, chardev]
>}
>if 'CONFIG_INOTIFY1' in config_host
>  tests += {'test-util-filemonitor': []}
> diff --git a/tests/unit/test-yank.c b/tests/unit/test-yank.c
> new file mode 100644
> index 00..c46946b642
> --- /dev/null
> +++ b/tests/unit/test-yank.c
> @@ -0,0 +1,227 @@
> +/*
> + * Tests for QEMU yank feature
> + *
> + * Copyright (c) Lukas Straub 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include 
> +
> +#include "qemu/config-file.h"
> +#include "qemu/module.h"
> +#include "qemu/option.h"
> +#include "chardev/char-fe.h"
> +#include "sysemu/sysemu.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-commands-char.h"
> +#include "qapi/qapi-types-char.h"
> +#include "qapi/qapi-commands-yank.h"
> +#include "qapi/qapi-types-yank.h"
> +#include "io/channel-socket.h"
> +#include "io/net-listener.h"
> +#include "socket-helpers.h"
> +
> +typedef struct {
> +SocketAddress *addr;
> +bool old_yank;
> +bool new_yank;
> +bool fail;
> +} CharChangeTestConfig;
> +
> +static int chardev_change(void *opaque)
> +{
> +return 0;
> +}
> +
> +static bool is_yank_instance_registered(void)
> +{
> +YankInstanceList *list;
> +bool ret;
> +
> +list = qmp_query_yank(_abort);
> +
> +ret = !!list;
> +
> +qapi_free_YankInstanceList(list);
> +
> +return ret;
> +}
> +
> +static void char_change_test(gconstpointer opaque)
> +{
> +CharChangeTestConfig *conf = (gpointer) opaque;
> +SocketAddress *addr;
> +Chardev *chr;
> +CharBackend be;
> +ChardevReturn *ret;
> +QIOChannelSocket *ioc;
> +QIONetListener *listener;
> +
> +/*
> + * Setup a listener socket and determine its address
> + * so we know the TCP port for the client later
> + */
> +ioc = qio_channel_socket_new();
> +g_assert_nonnull(ioc);
> +qio_channel_socket_listen_sync(ioc, conf->addr, 1, _abort);
> +addr = qio_channel_socket_get_local_address(ioc, _abort);
> +g_assert_nonnull(addr);
> +listener = qio_net_listener_new();
> +g_assert_nonnull(listener);
> +qio_net_listener_add(listener, ioc);
>

The listener doesn't work, as there is no loop running. This works for me.
Please update the patch & resend. Thanks

diff --git a/tests/unit/test-yank.c b/tests/unit/test-yank.c
index 1596a3b98e..6e28648750 100644
--- a/tests/unit/test-yank.c
+++ b/tests/unit/test-yank.c
@@ -49,6 +49,16 @@ static bool is_yank_instance_registered(void)
 return ret;
 }

+static gpointer
+accept_thread(gpointer data)
+{
+QIOChannelSocket *ioc = data;
+
+qio_channel_socket_accept(ioc, _abort);
+
+return NULL;
+}
+
 static void char_change_test(gconstpointer opaque)
 {
 CharChangeTestConfig *conf = (gpointer) opaque;
@@ -57,6 +67,7 @@ static void char_change_test(gconstpointer opaque)
 CharBackend be;
 ChardevReturn *ret;
 QIOChannelSocket *ioc;
+QemuThread thread;

 /*
  * Setup a listener socket and determine its address
@@ -115,6 +126,11 @@ static void char_change_test(gconstpointer opaque)

 g_assert(!is_yank_instance_registered());

+if (conf->old_yank) {
+qemu_thread_create(, "accept", accept_thread,
+   ioc, QEMU_THREAD_JOINABLE);
+}
+
 ret = qmp_chardev_add("chardev", [conf->old_yank],
_abort);
 qapi_free_ChardevReturn(ret);
 chr = qemu_chr_find("chardev");
@@ -123,6 +139,10 @@ static void char_change_test(gconstpointer opaque)
 g_assert(is_yank_instance_registered() == conf->old_yank);

 qemu_chr_wait_connected(chr, _abort);
+if (conf->old_yank) {
+qemu_thread_join();
+}
+
 qemu_chr_fe_init(, chr, _abort);
 /* allow

[Bug 1090604] Re: RFE: Implement support for SMBIOS Type 41 structures

2021-03-30 Thread Vincent Bernat

I have sent a first patch around this:
https://lists.nongnu.org/archive/html/qemu-devel/2021-03/msg09391.html

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

Title:
  RFE: Implement support for SMBIOS Type 41 structures

Status in QEMU:
  In Progress

Bug description:
  This was originally filed in Fedora bugzilla:
  https://bugzilla.redhat.com/show_bug.cgi?id=669955

  """
  Please extend the existing support for SMBIOS in qemu to add a capability to 
provide "Onboard Devices Extended Information" (Type 41). Not only is this 
replacing one of the existing types, but it also provides a mapping between 
devices and physical system chassis locations. But there is no physical 
chassis! Right. However, this doesn't mean you don't want to tell the guest OS 
which virtual (e.g. network) interface is which. You can do that, if you 
implement this extension that is already going into real hardware, and likely 
other VMs too.

  See also page 117 of the v2.7 of the SMBIOS spec.

  FWIW, VMware ESX and Workstation expose their PCI NICs in the PCI IRQ Routing 
Table.  Kind of odd the first time you see it with biosdevname, as your NIC 
becomes pci3#1, but that's "correct" from a BIOS perspective. :-)
  """

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

Re: [PATCH 0/6] Add debug interface to kick/call on purpose

2021-03-30 Thread Dongli Zhang

On 3/28/21 8:56 PM, Jason Wang wrote:
> 
> 在 2021/3/27 上午5:16, Dongli Zhang 写道:
>> Hi Jason,
>>
>> On 3/26/21 12:24 AM, Jason Wang wrote:
>>> 在 2021/3/26 下午1:44, Dongli Zhang 写道:
 The virtio device/driver (e.g., vhost-scsi or vhost-net) may hang due to
 the loss of doorbell kick, e.g.,

 https://urldefense.com/v3/__https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01711.html__;!!GqivPVa7Brio!KS3pAU2cKjz4wgI4QSlE-YsJPhPG71nkE5_tGhaOf7mi_xvNxbvKkfn03rk5BNDLSEU$

 ... or due to the loss of IRQ, e.g., as fixed by linux kernel commit
 fe200ae48ef5 ("genirq: Mark polled irqs and defer the real handler").

 This patch introduces a new debug interface 'DeviceEvent' to DeviceClass
 to help narrow down if the issue is due to loss of irq/kick. So far the new
 interface handles only two events: 'call' and 'kick'. Any device (e.g.,
 virtio/vhost or VFIO) may implement the interface (e.g., via eventfd, MSI-X
 or legacy IRQ).

 The 'call' is to inject irq on purpose by admin for a specific device 
 (e.g.,
 vhost-scsi) from QEMU/host to VM, while the 'kick' is to kick the doorbell
 on purpose by admin at QEMU/host side for a specific device.

 This device can be used as a workaround if call/kick is lost due to
 virtualization software (e.g., kernel or QEMU) issue.

 We may also implement the interface for VFIO PCI, e.g., to write to
 VFIOPCIDevice->msi_vectors[i].interrupt will be able to inject IRQ to VM
 on purpose. This is considered future work once the virtio part is done.

 Below is from live crash analysis. Initially, the queue=2 has count=15 for
 'kick' eventfd_ctx. Suppose there is data in vring avail while there is no
 used available. We suspect this is because vhost-scsi was not notified by
 VM. In order to narrow down and analyze the issue, we use live crash to
 dump the current counter of eventfd for queue=2.

 crash> eventfd_ctx 8f67f6bbe700
 struct eventfd_ctx {
     kref = {
   refcount = {
     refs = {
   counter = 4
     }
   }
     },
     wqh = {
   lock = {
     {
   rlock = {
     raw_lock = {
   val = {
     counter = 0
   }
     }
   }
     }
   },
   head = {
     next = 0x8f841dc08e18,
     prev = 0x8f841dc08e18
   }
     },
     count = 15, ---> eventfd is 15 !!!
     flags = 526336
 }

 Now we kick the doorbell for vhost-scsi queue=2 on purpose for diagnostic
 with this interface.

 { "execute": "x-debug-device-event",
     "arguments": { "dev": "/machine/peripheral/vscsi0",
    "event": "kick", "queue": 2 } }

 The counter is increased to 16. Suppose the hang issue is resolved, it
 indicates something bad is in software that the 'kick' is lost.
>>> What do you mean by "software" here? And it looks to me you're testing 
>>> whether
>>> event_notifier_set() is called by virtio_queue_notify() here. If so, I'm not
>>> sure how much value could we gain from a dedicated debug interface like this
>>> consider there're a lot of exisinting general purpose debugging method like
>>> tracing or gdb. I'd say the path from virtio_queue_notify() to
>>> event_notifier_set() is only a very small fraction of the process of 
>>> virtqueue
>>> kick which is unlikey to be buggy. Consider usually the ioeventfd will be
>>> offloaded to KVM, it's more a chance that something is wrong in setuping
>>> ioeventfd instead of here. Irq is even more complicated.
>> Thank you very much!
>>
>> I am not testing whether event_notifier_set() is called by 
>> virtio_queue_notify().
>>
>> The 'software' indicates the data processing and event notification mechanism
>> involved with virtio/vhost PV driver frontend. E.g., while VM is waiting for 
>> an
>> extra IRQ, vhost side did not trigger IRQ, suppose vring_need_event()
>> erroneously returns false due to corrupted ring buffer status.
> 
> 
> So there could be several factors that may block the notification:
> 
> 1) eventfd bug (ioeventfd vs irqfd)
> 2) wrong virtqueue state (either driver or device)
> 3) missing barriers (either driver or device)
> 4) Qemu bug (irqchip and routing)
> ...

This is not only about whether notification is blocked.

It can also be used to help narrow down and understand if there is any
suspicious issue in blk-mq/scsi/netdev/napi code. The PV drivers are not only
drivers following virtio spec. It is closely related to many of other kernel
components.

Suppose IO was recovered after we inject an IRQ to vhost-scsi on purpose, we
will be able to analyze what may happen along the IO completion path starting
from when /where the IRQ is injected ... perhaps the root cause is not with
virtio but

Re: [PATCH] replay: fix recursive checkpoints

2021-03-30 Thread Pavel Dovgalyuk


On 29.03.2021 14:25, Alex Bennée wrote:


Pavel Dovgalyuk  writes:


Record/replay uses checkpoints to synchronize the execution
of the threads and timers. Hardware events such as BH are
processed at the checkpoints too.
Event processing can cause refreshing the virtual timers
and calling the icount-related functions, that also use checkpoints.
This patch prevents recursive processing of such checkpoints,
because they have their own records in the log and should be
processed later.

Signed-off-by: Pavel Dovgalyuk 
---
  replay/replay.c |   11 ++-
  1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/replay/replay.c b/replay/replay.c
index c806fec69a..6df2abc18c 100644
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -180,12 +180,13 @@ bool replay_checkpoint(ReplayCheckpoint checkpoint)
  }
  
  if (in_checkpoint) {

-/* If we are already in checkpoint, then there is no need
-   for additional synchronization.
+/*
 Recursion occurs when HW event modifies timers.
-   Timer modification may invoke the checkpoint and
-   proceed to recursion. */
-return true;
+   Prevent performing icount warp in this case and
+   wait for another invocation of the checkpoint.
+*/


nit: as you are updating the comment you might as well fix the style. It
would probably help with the diff as well.


+g_assert(replay_mode == REPLAY_MODE_PLAY);
+return false;
  }
  in_checkpoint = true;


The accompanying comments in replay.h are also confusing

 Returns 0 in PLAY mode if checkpoint was not found.
 Returns 1 in all other cases.

Which translated to actual bool results:

 Returns false in PLAY mode if checkpoint was not found
 Returns true in all other cases

Which implies the checkpoint is always found (or created?) which I'm not
even sure of while following the rest of the replay_checkpoint code
which has exit cases of:

 bool res = false; (default)
 replay_state.data_kind != EVENT_ASYNC;
 res = true; (when recording)

So is the following more correct?

/**
  * replay_checkpoint(checkpoint): save (in RECORD) or consume (in PLAY) 
checkpoint
  * @checkpoint: the checkpoint event
  *
  * In SAVE mode stores the checkpoint in the record and potentially
  * saves a number of events.
  *
  * In PLAY mode consumes checkpoint and any following EVENT_ASYNC events.
  *
  * Results: in SAVE mode always True
  *  in PLAY mode True unless checkpoint not found or recursively 
called.
  */



Almost true.
In PLAY returns True only if the checkpoint was found and all following 
async events matched and processed.
Otherwise returns false and non-processed events are postponed to be 
consumed later.


Pavel Dovgalyuk

Re: [PULL 00/10] For 6.0 patches

2021-03-30 Thread Marc-André Lureau

Hi

On Mon, Mar 29, 2021 at 9:54 PM Peter Maydell 
wrote:

> On Mon, 29 Mar 2021 at 17:30, Marc-André Lureau
>  wrote:
> >
> > Hi
> >
> > On Mon, Mar 29, 2021 at 7:56 PM Peter Maydell 
> wrote:
> >>
> >> On Mon, 29 Mar 2021 at 15:17, Marc-André Lureau
> >>  wrote:
> >> > ../docs/meson.build:30: WARNING: /usr/bin/sphinx-build-3:
> >> > Configuration error:
> >> > The Sphinx 'sphinx_rtd_theme' HTML theme was not found.
> >> >
> >> > ../docs/meson.build:32:6: ERROR: Problem encountered: Install a
> Python 3 version of python-sphinx and the readthedoc theme
> >>
> >>
> >> So why do you get that message, and I see the above? Older
> >> sphinx-build ?
> >
> >
> >
> > It's strange, it's like ModuleNotFoundError was not catched by the
> "except ImportError".
> >
> > What's the version of python?
>
> It's whatever's in the BSD VMs. I also saw the same error on the
>

I built successfully with  vm-build-openbsd, vm-build-freebsd, and
vm-build-netbsd. None have sphinx installed, thus simply print:
Program sphinx-build-3 sphinx-build found: NO

Am I missing something?

aarch64 CI machine, which has python 3.8.5 and sphinx-build 1.8.5.
> My guess is that it might be the sphinx-build version here. I vaguely
> recall that Sphinx is kind of picky about exceptions within the conf
> file but that there was a change in what it allowed at some point.
> It's possible we just can't do much with the old versions.
>

How do you run the build? Running make from an existing configured or build
state? If so, I have seen sphinx errors that don't stop the build (and
actually building the docs without sphinx-rtd). I don't know why this
happens, "regenerate"/reconfigure errors should stop the build.

It seems like a minor issue to me. A clean build will error correctly.

> I'm inclined to suggest we should postpone switching to the rtd theme
> until after the 6.0 release -- there isn't a strong need to get it
> in this release, is there ?
>
>
There is no hurry, but let's try to make some progress. If it's ready, I'll
let you decide if this is acceptable during freeze period or not.

Now I am not sure what should be fixed... I will try to find the cause of
the non-fatal error on incremental build.

thanks

-- 
Marc-André Lureau

[Bug 1862874] Re: java may stuck for a long time in system mode with "-cpu max"

2021-03-30 Thread David Hildenbrand

** Changed in: qemu
   Status: New => Confirmed

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

Title:
  java may stuck for a long time in system mode with "-cpu max"

Status in QEMU:
  Confirmed

Bug description:
  Bug Description:
  Run "java -version" in guest VM, java may stuck for a long time (several 
hours) and then recover.

  Steps to reproduce:
  1. Launch VM by attached simple script: launch.sh
  2. Execute "java -version" and then print "date" in a loop
  while :
  do
/home/bot/jdk/bin/java -version
date
  done
  3. A long time gap will be observed: may > 24 hours.

  Technical details:
  * host: x86_64 Linux 4.15.0-70-generic
  * qemu v4.2.0
  * java: tried two versions: openjdk-11-jre-headless or compiled java-13 
  * command-line: (See details in launch.sh)
  /home/bot/qemu/qemu-build/qemu-4.2.0/binaries/bin/qemu-system-x86_64 \
-drive "file=${img},format=qcow2" \
-drive "file=${user_data},format=raw" \
-cpu max \
-m 24G \
-serial mon:stdio \
-smp 8 \
-nographic \
  ;

  * Observed by java core dump generated by "kill -SIGSEGV" when java stucked:
  Different pthreads are blocked on their own condition variables:

Id   Target Id Frame
1Thread 0x7f48a041a080 (LWP 22470) __GI_raise (sig=sig@entry=6)
  at ../sysdeps/unix/sysv/linux/raise.c:51
2Thread 0x7f487197d700 (LWP 22473) 0x7f489f5c49f3 in 
futex_wait_cancelable (private=, expected=0, 
futex_word=0x7f48980197c0)
  at ../sysdeps/unix/sysv/linux/futex-internal.h:88
3Thread 0x7f4861b89700 (LWP 22483) 0x7f489f5c4ed9 in 
futex_reltimed_wait_cancelable (private=, 
reltime=0x7f4861b88960, expected=0,
  futex_word=0x7f489801b084)
  at ../sysdeps/unix/sysv/linux/futex-internal.h:142
4Thread 0x7f4861e8c700 (LWP 22480) 0x7f489f5c76d6 in 
futex_abstimed_wait_cancelable (private=0, abstime=0x0, expected=0, 
futex_word=0x7f48980107c0)
  at ../sysdeps/unix/sysv/linux/futex-internal.h:205
5Thread 0x7f4861c8a700 (LWP 22482) 0x7f489f5c4ed9 in 
futex_reltimed_wait_cancelable (private=, 
reltime=0x7f4861c89800, expected=0,
  futex_word=0x7f489801ed44)
  at ../sysdeps/unix/sysv/linux/futex-internal.h:142
6Thread 0x7f48a0418700 (LWP 22471) 0x7f4880b13200 in ?? ()
7Thread 0x7f48703ea700 (LWP 22478) 0x7f489f5c49f3 in 
futex_wait_cancelable (private=, expected=0, 
futex_word=0x7f489801dfc0)
  at ../sysdeps/unix/sysv/linux/futex-internal.h:88
8Thread 0x7f48702e9700 (LWP 22479) 0x7f489f5c49f3 in 
futex_wait_cancelable (private=, expected=0, 
futex_word=0x7f489838cd84)
  at ../sysdeps/unix/sysv/linux/futex-internal.h:88
9Thread 0x7f4870f71700 (LWP 22475) 0x7f489f5c49f3 in 
futex_wait_cancelable (private=, expected=0, 
futex_word=0x7f489801a300)
  at ../sysdeps/unix/sysv/linux/futex-internal.h:88
10   Thread 0x7f487187b700 (LWP 22474) 0x7f489f5c76d6 in 
futex_abstimed_wait_cancelable (private=0, abstime=0x0, expected=0, 
futex_word=0x7f48980cf770)
  at ../sysdeps/unix/sysv/linux/futex-internal.h:205
11   Thread 0x7f4871a7f700 (LWP 22472) 0x7f489f5c76d6 in 
futex_abstimed_wait_cancelable (private=0, abstime=0x0, expected=0, 
futex_word=0x7f489809ba30)
  at ../sysdeps/unix/sysv/linux/futex-internal.h:205
12   Thread 0x7f4861d8b700 (LWP 22481) 0x7f489f5c4ed9 in 
futex_reltimed_wait_cancelable (private=, 
reltime=0x7f4861d8a680, expected=0,
  futex_word=0x7f489801ed44)
  at ../sysdeps/unix/sysv/linux/futex-internal.h:142
13   Thread 0x7f48704ec700 (LWP 22477) 0x7f489f5c4ed9 in 
futex_reltimed_wait_cancelable (private=, 
reltime=0x7f48704eb910, expected=0,
  futex_word=0x7f489801d120)
  at ../sysdeps/unix/sysv/linux/futex-internal.h:142
14   Thread 0x7f4870e6f700 (LWP 22476) 0x7f489f5c4ed9 in 
futex_reltimed_wait_cancelable (private=, 
reltime=0x7f4870e6eb20, expected=0,
  futex_word=0x7f489828abd0)
  at ../sysdeps/unix/sysv/linux/futex-internal.h:142

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

[Bug 1920913] Re: Openjdk11+ fails to install on s390x

2021-03-30 Thread David Hildenbrand

Same BUG as https://bugs.launchpad.net/qemu/+bug/1862874

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

Title:
  Openjdk11+ fails to install on s390x

Status in QEMU:
  New

Bug description:
  While installing openjdk11 or higher from repo, it crashes while configuring 
ca-certificates-java.
  Although `java -version` passes, `jar -version` crashes. Detailed logs 
attached to this issue.

  ```
  # A fatal error has been detected by the Java Runtime Environment:
  #
  #  SIGILL (0x4) at pc=0x0040126f9980, pid=8425, tid=8430
  #
  # JRE version: OpenJDK Runtime Environment (11.0.10+9) (build 
11.0.10+9-Ubuntu-0ubuntu1.20.04)
  # Java VM: OpenJDK 64-Bit Server VM (11.0.10+9-Ubuntu-0ubuntu1.20.04, mixed 
mode, tiered, compressed oops, g1 gc, linux-s390x)
  # Problematic frame:
  # J 4 c1 java.lang.StringLatin1.hashCode([B)I java.base@11.0.10 (42 bytes) @ 
0x0040126f9980 [0x0040126f9980+0x]
  #
  # Core dump will be written. Default location: Core dumps may be processed 
with "/usr/share/apport/apport %p %s %c %d %P %E" (or dumping to //core.8425)
  #
  # An error report file with more information is saved as:
  # //hs_err_pid8425.log
  sed with "/usr/share/apport/apport %p %s %c %d %P %E" (or dumping to 
/root/core.10740)
  #
  # An error report file with more information is saved as:
  # /root/hs_err_pid10740.log
  ```

  Observed this on s390x/ubuntu as well as s390x/alpine when run on amd64 host.
  Please note, on native s390x, the installation is successful. Also this crash 
is not observed while installing openjdk-8-jdk.

  Qemu version: 5.2.0

  Please let me know if any more details are needed.

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

Re: [RFC 1/8] memory: Allow eventfd add/del without starting a transaction

2021-03-30 Thread Greg Kurz

On Mon, 29 Mar 2021 18:03:49 +0100
Stefan Hajnoczi  wrote:

> On Thu, Mar 25, 2021 at 04:07:28PM +0100, Greg Kurz wrote:
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 5728a681b27d..98ed552e001c 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -1848,13 +1848,25 @@ void 
> > memory_region_clear_flush_coalesced(MemoryRegion *mr);
> >   * @match_data: whether to match against @data, instead of just @addr
> >   * @data: the data to match against the guest write
> >   * @e: event notifier to be triggered when @addr, @size, and @data all 
> > match.
> > + * @transaction: whether to start a transaction for the change
> 
> "start" is unclear. Does it begin a transaction and return with the
> transaction unfinished? I think instead the function performs the
> eventfd addition within a transaction. It would be nice to clarify this.
> 

What about: 

 * @transaction: if true, the eventfd is added within a nested transaction,
 *   if false, it is up to the caller to ensure this is called
 *   within a transaction.

> >   **/
> > -void memory_region_add_eventfd(MemoryRegion *mr,
> > -   hwaddr addr,
> > -   unsigned size,
> > -   bool match_data,
> > -   uint64_t data,
> > -   EventNotifier *e);
> > +void memory_region_add_eventfd_full(MemoryRegion *mr,
> > +hwaddr addr,
> > +unsigned size,
> > +bool match_data,
> > +uint64_t data,
> > +EventNotifier *e,
> > +bool transaction);
> > +
> > +static inline void memory_region_add_eventfd(MemoryRegion *mr,
> > + hwaddr addr,
> > + unsigned size,
> > + bool match_data,
> > + uint64_t data,
> > + EventNotifier *e)
> > +{
> > +memory_region_add_eventfd_full(mr, addr, size, match_data, data, e, 
> > true);
> > +}
> >  
> >  /**
> >   * memory_region_del_eventfd: Cancel an eventfd.
> > @@ -1868,13 +1880,25 @@ void memory_region_add_eventfd(MemoryRegion *mr,
> >   * @match_data: whether to match against @data, instead of just @addr
> >   * @data: the data to match against the guest write
> >   * @e: event notifier to be triggered when @addr, @size, and @data all 
> > match.
> > + * @transaction: whether to start a transaction for the change
> 
> Same here.

and:

 * @transaction: if true, the eventfd is cancelled within a nested transaction,
 *   if false, it is up to the caller to ensure this is called
 *   within a transaction.

?


pgpx_Buep1Imv.pgp
Description: OpenPGP digital signature

Re: [PATCH v2] qapi: introduce 'query-cpu-model-cpuid' action

2021-03-30 Thread Valeriy Vdovin

On Tue, Mar 30, 2021 at 02:15:10AM +0200, Igor Mammedov wrote:
> On Thu, 25 Mar 2021 19:57:05 +0300
> Valeriy Vdovin  wrote:
> 
> > Introducing new qapi method 'query-cpu-model-cpuid'. This method can be 
> > used to
> > get virtualized cpu model info generated by QEMU during VM initialization in
> > the form of cpuid representation.
> > 
> > Diving into more details about virtual cpu generation: QEMU first parses 
> > '-cpu'
> > command line option. From there it takes the name of the model as the basis 
> > for
> > feature set of the new virtual cpu. After that it uses trailing '-cpu' 
> > options,
> > that state if additional cpu features should be present on the virtual cpu 
> > or
> > excluded from it (tokens '+'/'-' or '=on'/'=off').
> > After that QEMU checks if the host's cpu can actually support the derived
> > feature set and applies host limitations to it.
> > After this initialization procedure, virtual cpu has it's model and
> > vendor names, and a working feature set and is ready for identification
> > instructions such as CPUID.
> > 
> > Currently full output for this method is only supported for x86 cpus.
> > 
> > To learn exactly how virtual cpu is presented to the guest machine via CPUID
> > instruction, new qapi method can be used. By calling 'query-cpu-model-cpuid'
> > method, one can get a full listing of all CPUID leafs with subleafs which 
> > are
> > supported by the initialized virtual cpu.
> > 
> > Other than debug, the method is useful in cases when we would like to
> > utilize QEMU's virtual cpu initialization routines and put the retrieved
> > values into kernel CPUID overriding mechanics for more precise control
> > over how various processes perceive its underlying hardware with
> > container processes as a good example.
> 
> 
> existing 'query-cpu-definitions' does return feature bits that are actually
> supported by qemu/host combination, why do we need a second very simillar 
> interface?
> 
We've examined 'query-cpu-definitions' as well as 'query-cpu-model-expansion', 
which
is even a better fit for the job. But both methods just provide a list of cpu 
features,
while leaving CPUID generation out of their scope.
Here is an example output from 'query-cpu-model-expansion':

{
"return": {
  "model": {
 "name": "max",
 "props": {
   "vmx-entry-load-rtit-ctl": false,
   "phys-bits": 0,
   "core-id": -1,
   "svme-addr-chk": false,
   "xlevel": 2147483656,
   "cmov": true,
   "ia64": false,
   "ssb-no": false,
   "aes": false,
   "vmx-apicv-xapic": true,
   ...

However having this information we are only half-way there. We now need to 
somehow 
convert all this information into CPUID leaves, that we will be able to give 
out 
to the callers of 'cpuid' instruction. As we can see in the above listing, the
field type is not even a uniform list of cpu features. It's an unordered 
information
that matters for virtual cpu presentation each in it's own way.

To construct CPUID leaves from that, the application should have ALL the 
knowledge
about each type of the above properties list. This is the kind of code writing 
that
we naturally want to evade knowing that there is a perfect function that 
already does
that.
I'm talking about 'cpu_x86_cpuid' in QEMU sources. It already does the whole 
CPUID
response construction. Just looking at its listing the function seems to be 
pretty
complex. So writing the same logic will repeat the same complexity and all 
risks.
Also it's in a public domain, so it's guaranteed to be revisisted, improved and
bug-fixed often.
So utilizing this function is an easy choice, in fact almost no choice. All we 
need 
is an api, that can fetch results from this function, which is exactly what our 
new
QMP method does. The method is pretty straightforward, so there will be not 
much to 
maintain, compared to the effort that would need to be done to support future 
CPU
features.

> > 
> > Output format:
> > The core part of the returned JSON object can be described as a list of 
> > lists
> > with top level list contains leaf-level elements and the bottom level
> > containing subleafs, where 'leaf' is CPUID argument passed in EAX register 
> > and
> > 'subleaf' is a value passed to CPUID in ECX register for some specific
> > leafs, that support that. Each most basic CPUID result is passed in a
> > maximum of 4 registers EAX, EBX, ECX and EDX, with most leafs not utilizing
> > all 4 registers at once.
> > Also note that 'subleaf' is a kind of extension, used by only a couple of
> > leafs, while most of the leafs don't have this. Nevertheless, the output
> > data structure presents ALL leafs as having at least a single 'subleaf'.
> > This is done for data structure uniformity, so that it could be
> > processed in a more straightforward manner, in this case no one suffers
> > from such simplification.
> > 
> > Use example:
> > virsh qemu-monitor-command VM --pretty '{ "execute": 
> > "query-cpu-model-cpuid" }'
> >

[PATCH v3 1/3] Linux headers: update from 5.12-rc3

2021-03-30 Thread Ravi Bangoria

Update against Linux 5.12-rc3

Signed-off-by: Ravi Bangoria 
---
 include/standard-headers/drm/drm_fourcc.h | 23 -
 include/standard-headers/linux/input.h|  2 +-
 .../standard-headers/rdma/vmw_pvrdma-abi.h|  7 ++
 linux-headers/asm-generic/unistd.h|  4 +-
 linux-headers/asm-mips/unistd_n32.h   |  1 +
 linux-headers/asm-mips/unistd_n64.h   |  1 +
 linux-headers/asm-mips/unistd_o32.h   |  1 +
 linux-headers/asm-powerpc/kvm.h   |  2 +
 linux-headers/asm-powerpc/unistd_32.h |  1 +
 linux-headers/asm-powerpc/unistd_64.h |  1 +
 linux-headers/asm-s390/unistd_32.h|  1 +
 linux-headers/asm-s390/unistd_64.h|  1 +
 linux-headers/asm-x86/kvm.h   |  1 +
 linux-headers/asm-x86/unistd_32.h |  1 +
 linux-headers/asm-x86/unistd_64.h |  1 +
 linux-headers/asm-x86/unistd_x32.h|  1 +
 linux-headers/linux/kvm.h | 89 +++
 linux-headers/linux/vfio.h| 27 ++
 18 files changed, 161 insertions(+), 4 deletions(-)

diff --git a/include/standard-headers/drm/drm_fourcc.h 
b/include/standard-headers/drm/drm_fourcc.h
index c47e19810c..a61ae520c2 100644
--- a/include/standard-headers/drm/drm_fourcc.h
+++ b/include/standard-headers/drm/drm_fourcc.h
@@ -526,6 +526,25 @@ extern "C" {
  */
 #define I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS fourcc_mod_code(INTEL, 7)
 
+/*
+ * Intel Color Control Surface with Clear Color (CCS) for Gen-12 render
+ * compression.
+ *
+ * The main surface is Y-tiled and is at plane index 0 whereas CCS is linear
+ * and at index 1. The clear color is stored at index 2, and the pitch should
+ * be ignored. The clear color structure is 256 bits. The first 128 bits
+ * represents Raw Clear Color Red, Green, Blue and Alpha color each represented
+ * by 32 bits. The raw clear color is consumed by the 3d engine and generates
+ * the converted clear color of size 64 bits. The first 32 bits store the Lower
+ * Converted Clear Color value and the next 32 bits store the Higher Converted
+ * Clear Color value when applicable. The Converted Clear Color values are
+ * consumed by the DE. The last 64 bits are used to store Color Discard Enable
+ * and Depth Clear Value Valid which are ignored by the DE. A CCS cache line
+ * corresponds to an area of 4x1 tiles in the main surface. The main surface
+ * pitch is required to be a multiple of 4 tile widths.
+ */
+#define I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC fourcc_mod_code(INTEL, 8)
+
 /*
  * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
  *
@@ -1035,9 +1054,9 @@ drm_fourcc_canonicalize_nvidia_format_mod(uint64_t 
modifier)
  * Not all combinations are valid, and different SoCs may support different
  * combinations of layout and options.
  */
-#define __fourcc_mod_amlogic_layout_mask 0xf
+#define __fourcc_mod_amlogic_layout_mask 0xff
 #define __fourcc_mod_amlogic_options_shift 8
-#define __fourcc_mod_amlogic_options_mask 0xf
+#define __fourcc_mod_amlogic_options_mask 0xff
 
 #define DRM_FORMAT_MOD_AMLOGIC_FBC(__layout, __options) \
fourcc_mod_code(AMLOGIC, \
diff --git a/include/standard-headers/linux/input.h 
b/include/standard-headers/linux/input.h
index f89c986190..7822c24178 100644
--- a/include/standard-headers/linux/input.h
+++ b/include/standard-headers/linux/input.h
@@ -81,7 +81,7 @@ struct input_id {
  * in units per radian.
  * When INPUT_PROP_ACCELEROMETER is set the resolution changes.
  * The main axes (ABS_X, ABS_Y, ABS_Z) are then reported in
- * in units per g (units/g) and in units per degree per second
+ * units per g (units/g) and in units per degree per second
  * (units/deg/s) for rotational axes (ABS_RX, ABS_RY, ABS_RZ).
  */
 struct input_absinfo {
diff --git a/include/standard-headers/rdma/vmw_pvrdma-abi.h 
b/include/standard-headers/rdma/vmw_pvrdma-abi.h
index 0989426a3f..c30182a7ae 100644
--- a/include/standard-headers/rdma/vmw_pvrdma-abi.h
+++ b/include/standard-headers/rdma/vmw_pvrdma-abi.h
@@ -133,6 +133,13 @@ enum pvrdma_wc_flags {
PVRDMA_WC_FLAGS_MAX = PVRDMA_WC_WITH_NETWORK_HDR_TYPE,
 };
 
+enum pvrdma_network_type {
+   PVRDMA_NETWORK_IB,
+   PVRDMA_NETWORK_ROCE_V1 = PVRDMA_NETWORK_IB,
+   PVRDMA_NETWORK_IPV4,
+   PVRDMA_NETWORK_IPV6
+};
+
 struct pvrdma_alloc_ucontext_resp {
uint32_t qp_tab_size;
uint32_t reserved;
diff --git a/linux-headers/asm-generic/unistd.h 
b/linux-headers/asm-generic/unistd.h
index 7287529177..ce58cff99b 100644
--- a/linux-headers/asm-generic/unistd.h
+++ b/linux-headers/asm-generic/unistd.h
@@ -861,9 +861,11 @@ __SYSCALL(__NR_faccessat2, sys_faccessat2)
 __SYSCALL(__NR_process_madvise, sys_process_madvise)
 #define __NR_epoll_pwait2 441
 __SC_COMP(__NR_epoll_pwait2, sys_epoll_pwait2, compat_sys_epoll_pwait2)
+#define __NR_mount_setattr 442
+__SYSCALL(__NR_mount_setattr, sys_mount_setattr)
 
 #undef __NR_syscalls
-#define

Re: [PATCH v4 for-6.0? 0/3] qcow2: fix parallel rewrite and discard (rw-lock)

2021-03-30 Thread Vladimir Sementsov-Ogievskiy

30.03.2021 12:49, Max Reitz wrote:

On 25.03.21 20:12, Vladimir Sementsov-Ogievskiy wrote:

ping. Do we want it for 6.0?

I’d rather wait. I think the conclusion was that guests shouldn’t hit this
because they serialize discards?

I think, that we never had bugs, so we of course can wait.

There’s also something Kevin wrote on IRC a couple of weeks ago, for which I
had hoped he’d sent an email but I don’t think he did, so I’ll try to remember
and paraphrase as well as I can...

He basically asked whether it wouldn’t be conceptually simpler to take a
reference to some cluster in get_cluster_offset() and later release it with a
to-be-added put_cluster_offset().

He also noted that reading is problematic, too, because if you read a discarded
and reused cluster, this might result in an information leak (some guest
application might be able to read data it isn’t allowed to read); that’s why
making get_cluster_offset() the point of locking clusters against discarding
would be better.

Yes, I thought about read too, (RFCed in cover letter of [PATCH v5 0/6] qcow2:
fix parallel rewrite and discard (lockless))

This would probably work with both of your solutions. For the in-memory
solutions, you’d take a refcount to an actual cluster; in the CoRwLock
solution, you’d take that lock.

What do you think?

Hmm. What do you mean? Just rename my qcow2_inflight_writes_inc() and
qcow2_inflight_writes_dec() to get_cluster_offset()/put_cluster_offset(), to
make it more native to use for read operations as well?

Or to update any kind of "getting cluster offset" in the whole qcow2 driver to take a kind of
"dynamic reference count" by get_cluster_offset() and then call corresponding put() somewhere? In this
case I'm afraid it's a lot more work.. It would be also the problem that a lot of paths in qcow2 are not in
coroutine and don't even take s->lock when they actually should. This will also mean that we do same job as
normal qcow2 refcounts already do: no sense in keeping additional "dynamic refcount" for L2 table
cluster while reading it, as we already have non-zero qcow2 normal refcount for it..

--
Best regards,
Vladimir

Re: Serious doubts about Gitlab CI

2021-03-30 Thread Thomas Huth


On 30/03/2021 13.19, Daniel P. Berrangé wrote:

On Mon, Mar 29, 2021 at 03:10:36PM +0100, Stefan Hajnoczi wrote:

Hi,
I wanted to follow up with a summary of the CI jobs:

1. Containers & Containers Layer2 - ~3 minutes/job x 39 jobs
2. Builds - ~50 minutes/job x 61 jobs
3. Tests - ~12 minutes/job x 20 jobs
4. Deploy - 52 minutes x 1 job


I hope that 52 was just a typo ... ?


I think a challenges we have with our incremental approach is that
we're not really taking into account relative importance of the
different build scenarios, and often don't look at the big picture
of what the new job adds in terms of quality, compared to existing
jobs.

eg Consider we have

   build-system-alpine:
   build-system-ubuntu:
   build-system-debian:
   build-system-fedora:
   build-system-centos:
   build-system-opensuse:


I guess we could go through that list of jobs and remove the duplicated 
target CPUs, e.g. it should be enough to test x86_64-softmmu only once.



   build-trace-multi-user:
   build-trace-ftrace-system:
   build-trace-ust-system:

I'd question whether we really need any of those 'build-trace'
jobs. Instead, we could have build-system-ubuntu pass
--enable-trace-backends=log,simple,syslog, build-system-debian
pass --enable-trace-backends=ust and build-system-fedora
pass --enable-trace-backends=ftrace, etc.


I recently had the very same idea already:

 https://gitlab.com/qemu-project/qemu/-/commit/65aff82076a9bbfdf7

:-)


Another example, is that we test builds on centos7 with
three different combos of crypto backend settings. This was
to exercise bugs we've seen in old crypto packages in RHEL-7
but in reality, it is probably overkill, because downstream
RHEL-7 only cares about one specific combination.


Care to send a patch? Or shall we just wait one more months and then remove 
these jobs (since we won't support RHEL7 after QEMU 6.0 anymore)?



We don't really have a clearly defined plan to identify what
the most important things are in our testing coverage, so we
tend to accept anything without questioning its value add.
This really feeds back into the idea I've brought up many
times in the past, that we need to better define what we aim
to support in QEMU and its quality level, which will influence
what are the scenarios we care about testing.


But code that we have in the repository should get at least some basic test 
coverage, otherwise it bitrots soon ... so it's maybe rather the other old 
problem that we struggle with, that we should deprecate more code and remove 
it if nobody cares about it...



Traditionally ccache (https://ccache.dev/) was used to detect
recompilation of the same compiler input files. This is trickier to do
in GitLab CI since it would be necessary to share and update a cache,
potentially between untrusted users. Unfortunately this shifts the
bottleneck from CPU to network in a CI-as-a-Service environment since
the cached build output needs to be accessed by the linker on the CI
runner but is stored remotely.


Our docker containers install ccache already and I could have sworn
that we use that in gitlab, but now I'm not so sure. We're only
saving the "build/" directory as an artifact between jobs, and I'm
not sure that directory holds the ccache cache.


AFAIK we never really enabled ccache in the gitlab-CI, only in Travis.


This is as far as I've gotten with thinking about CI efficiency. Do you
think these optimizations are worth investigating or should we keep it
simple and just disable many builds by default?


ccache is a no-brainer and assuming it isn't already working with
our gitlab jobs, we must fix that asap.


I've found some nice instructions here:

https://gould.cx/ted/blog/2017/06/10/ccache-for-Gitlab-CI/

... and just kicked off a build with these modifications, let's see how it 
goes...



Aside from optimizing CI, we should consider whether there's more we
can do to optimize build process itself. We've done alot of work, but
there's still plenty of stuff we build multiple times, once for each
target. Perhaps there's scope for cutting this down in some manner ?


Right, I think we should also work more towards consolidating the QEMU 
binaries, to avoid that we have to always build sooo many target binaries 
again and again. E.g.:


- Do we still need to support 32-bit hosts? If not we could
  finally get rid of qemu-system-i386, qemu-system-ppc,
  qemu-system-arm, etc. and just provide the 64-bit variants

- Could we maybe somehow unify the targets that have both, big
  and little endian versions? Then we could merge e.g.
  qemu-system-microblaze and qemu-system-microblazeel etc.

- Or could we maybe even build a unified qemu-system binary that
  contains all target CPUs? ... that would also allow e.g.
  machines with a x86 main CPU and an ARM-based board management
  controller...


I'm unclear how many jobs in CI are build submodules, but if there's
more scope for using the pre-built distro packages that's going to
be beneficial in build time.

Re: Serious doubts about Gitlab CI

2021-03-30 Thread Philippe Mathieu-Daudé

On 3/30/21 1:55 PM, Thomas Huth wrote:
> On 30/03/2021 13.19, Daniel P. Berrangé wrote:
>> On Mon, Mar 29, 2021 at 03:10:36PM +0100, Stefan Hajnoczi wrote:

>>> Traditionally ccache (https://ccache.dev/) was used to detect
>>> recompilation of the same compiler input files. This is trickier to do
>>> in GitLab CI since it would be necessary to share and update a cache,
>>> potentially between untrusted users. Unfortunately this shifts the
>>> bottleneck from CPU to network in a CI-as-a-Service environment since
>>> the cached build output needs to be accessed by the linker on the CI
>>> runner but is stored remotely.
>>
>> Our docker containers install ccache already and I could have sworn
>> that we use that in gitlab, but now I'm not so sure. We're only
>> saving the "build/" directory as an artifact between jobs, and I'm
>> not sure that directory holds the ccache cache.
> 
> AFAIK we never really enabled ccache in the gitlab-CI, only in Travis.

Back then the Travis setup was simpler, and it took me 2 to 3 weeks
to get it right (probably spending 3 to 4h a day on it).

>>> This is as far as I've gotten with thinking about CI efficiency. Do you
>>> think these optimizations are worth investigating or should we keep it
>>> simple and just disable many builds by default?
>>
>> ccache is a no-brainer and assuming it isn't already working with
>> our gitlab jobs, we must fix that asap.
> 
> I've found some nice instructions here:
> 
> https://gould.cx/ted/blog/2017/06/10/ccache-for-Gitlab-CI/
> 
> ... and just kicked off a build with these modifications, let's see how
> it goes...

But we cross-build in Docker containers, so you need to mount the
cache dir in the container and set the CCACHE_DIR env var, isn't it?

Watch out about custom runners. If we do too many changes on the
free-tier runners, we'll never have the custom runner series integrated.

My 2 cents.

Regards,

Phil.

1 2 3 >

1 - 100 of 224 matches

Mail list logo