date:20211117

On 11/9/21 17:04, Cédric Le Goater wrote:
> On 11/9/21 16:51, Frederic Barrat wrote:
>>
>>
>> On 09/11/2021 15:50, Christophe Lombard wrote:
>>> The PCIe extended configuration space on the device is not currently
>>> accessible to the host. if by default,  it is still inaccessible for
>>> conventional for PCIe buses, add the current flag
>>> PCI_BUS_EXTENDED_CONFIG_SPACE on the root bus permits PCI-E extended
>>> config space access.
> 
> For the record, this is coming from an experiment of plugging a
> CXL device on a QEMU PowerNV POWER10 machine (baremetal). Only
> minor changes (64 bits ops) were required to get it working.

Since this note could be helpful when having future retrospective,
do you mind amending this note to the commit description?

> I wonder where are with the CXL models ?

IIRC Ben worked actively, asked help to the community but received
very few, basically because there is not enough man power IMHO.

Last thing I remember is Igor suggested a different design approach:
https://lore.kernel.org/qemu-devel/20210319180705.6ede9...@redhat.com/

>>> Signed-off-by: Christophe Lombard 
>>> ---
>>
>>
>> FWIW, looks good to me
>> Reviewed-by: Frederic Barrat 
> 
> 
> 
> Reviewed-by: Cédric Le Goater 
> 
> Thanks,
> 
> C.
> 
> 
>>
>>
>>
>>>   hw/pci-host/pnv_phb4.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
>>> index 5c375a9f28..40b793201a 100644
>>> --- a/hw/pci-host/pnv_phb4.c
>>> +++ b/hw/pci-host/pnv_phb4.c
>>> @@ -1205,6 +1205,7 @@ static void pnv_phb4_realize(DeviceState *dev,
>>> Error **errp)
>>>    &phb->pci_mmio, &phb->pci_io,
>>>    0, 4, TYPE_PNV_PHB4_ROOT_BUS);
>>>   pci_setup_iommu(pci->bus, pnv_phb4_dma_iommu, phb);
>>> +    pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>>>   /* Add a single Root port */
>>>   qdev_prop_set_uint8(DEVICE(&phb->root), "chassis", phb->chip_id);
>>>
> 
>

Re: [PATCH-for-7.0 v4 08/11] tests/unit/test-smp-parse: Add 'smp-without-dies-valid' machine type

Hi Yanan,

On 11/17/21 08:37, wangyanan (Y) wrote:
> On 2021/11/15 22:58, Philippe Mathieu-Daudé wrote:
>> Keep the common TYPE_MACHINE class initialization in
>> machine_base_class_init(), make it abstract, and move
>> the non-common code to a new class: "smp-without-dies-valid".
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>   tests/unit/test-smp-parse.c | 19 +++
>>   1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
>> index dfe7f1313b0..90a249fe8c8 100644
>> --- a/tests/unit/test-smp-parse.c
>> +++ b/tests/unit/test-smp-parse.c
>> @@ -478,13 +478,19 @@ static void machine_base_class_init(ObjectClass
>> *oc, void *data)
>>   {
>>   MachineClass *mc = MACHINE_CLASS(oc);
>>   +    mc->smp_props.prefer_sockets = true;
>> +
>> +    mc->name = g_strdup(SMP_MACHINE_NAME);
>> +}
>> +
>> +static void machine_without_dies_valid_class_init(ObjectClass *oc,
>> void *data)
>> +{
>> +    MachineClass *mc = MACHINE_CLASS(oc);
>> +
>>   mc->min_cpus = MIN_CPUS;
>>   mc->max_cpus = MAX_CPUS;
>>   -    mc->smp_props.prefer_sockets = true;
>>   mc->smp_props.dies_supported = false;
>> -
>> -    mc->name = g_strdup(SMP_MACHINE_NAME);
>>   }
>>     static void machine_without_dies_invalid_class_init(ObjectClass
>> *oc, void *data)
>> @@ -606,9 +612,14 @@ static const TypeInfo smp_machine_types[] = {
>>   {
>>   .name   = TYPE_MACHINE,
>>   .parent = TYPE_OBJECT,
>> +    .abstract   = true,
>>   .class_init = machine_base_class_init,
>>   .class_size = sizeof(MachineClass),
>>   .instance_size  = sizeof(MachineState),
>> +    }, {
>> +    .name   = MACHINE_TYPE_NAME("smp-without-dies-valid"),
>> +    .parent = TYPE_MACHINE,
>> +    .class_init = machine_without_dies_valid_class_init,
>>   }, {
>>   .name   =
>> MACHINE_TYPE_NAME("smp-without-dies-invalid"),
>>   .parent = TYPE_MACHINE,
>> @@ -629,7 +640,7 @@ int main(int argc, char *argv[])
>>   g_test_init(&argc, &argv, NULL);
>>     g_test_add_data_func("/test-smp-parse/generic/valid",
>> - TYPE_MACHINE,
>> + MACHINE_TYPE_NAME("smp-without-dies-valid"),
>>    test_generic_valid);
>>   g_test_add_data_func("/test-smp-parse/generic/invalid",
>>    MACHINE_TYPE_NAME("smp-without-dies-invalid"),
> After patch #7 and #8, we will have sub-tests as below. It looks nice,
> but it will
> probably be better to tweak "smp-without-dies-valid" to
> "smp-generic-valid",
> and "smp-without-dies-invalid" to "smp-generic-invalid", which will be more
> consistent with the corresponding sub-test name.
> 
> It's Ok now as we only have dies currently besides generic
> sockets/cores/threads,
> but "smp-without-dies-xxx" will become a bit confusing when new topology
> members are introduced and tested here.

OK I modified it and will respin once v6.2 is released.

Also test_with_dies() should be split in 2 tests: valid/invalid;
then smp_parse_test() should split tests further regarding the
socket preference. But I'll let that to you, I wanted to 1/ fix
our Windows CI and 2/ show you how to structure the tests.

Regards,

Phil.

Re: [PATCH v10 04/26] target/loongarch: Add fixed point arithmetic instruction translation

2021-11-17 Thread gaosong


Hi Richard,

On 2021/11/15 下午4:42, Richard Henderson wrote:

On 11/15/21 4:59 AM, gaosong wrote:

'The width of the immediate is a detail of the format'  means:

&fmt_rdrjimm rd  rj imm

@fmt_rdrjimm  .. imm:12  rj:5 rd:5 &fmt_rdrjimm
@fmt_rdrjimm14   imm:14  rj:5 rd:5 &fmt_rdrjimm
@fmt_rdrjimm16    .. imm:16  rj:5 rd:5 &fmt_rdrjimm

and we print in the disassembly, liks this

output_rdrjimm(DisasContext *ctx, arg_fmt_rdrjimm * a,  const char 
*mnemonic)

{
 output(ctx, mnemonic, "%s, %s, 0x%x", regnames[a->rd], 
regnames[a->rj], a->imm);

}

is that right?


Yes.

I'll note that regnames[] is defined in target/loongarch/cpu.c, which 
is not available when we want to use this disassembler for 
tcg/loongarch64/.  I think it would be easier to print this as


    "r%d", a->rd

so that you do not need to rely on the external strings.

I also think you should print signed numbers, "%d", because 0xfff8 
(truncated to 32 bits) is not really the correct representation of -8 
for a 64-bit operand.




1. We print sa in disassembly...
2. We use sa on gen_alsl_* not (sa2+1).
3. bytepick_w use the same print functions.
Is my understanding right?


Yes, that is the issue I am describing.

I see that  insns.decode format is not very consistent with other 
architectures, such ARM/RISCV


I'll correct it , like this:

# Fields
#
%sa2p1 15:2 !function=plus_1

#
# Argument sets
#
&r_i  rd imm
&rrr  rd rj rk
&rr_i rd rj imm
&rrr_sa rd rj rk sa

#
# Formats
#
@fmt_rrr   . rk:5 rj:5 rd:5 &rrr
@fmt_r_i20     ... imm:s20 rd:5 &r_i
@fmt_rr_i12    .. imm:s12 rj:5 rd:5 &rr_i
@fmt_rr_ui12    .. imm:12 rj:5 rd:5 &rr_i
@fmt_rr_i16    .. imm:s16 rj:5 rd:5 &rr_i
@fmt_rrr_sa2p1    ... .. rk:5 rj:5 rd:5 &rrr_sa  sa=%sa2p1

#
# Fixed point arithmetic operation instruction
#
add_w     0001 0 . . .    @fmt_rrr
add_d     0001 1 . . .    @fmt_rrr
sub_w     0001 00010 . . .    @fmt_rrr
sub_d     0001 00011 . . .    @fmt_rrr
slt   0001 00100 . . . @fmt_rrr
sltu  0001 00101 . . . @fmt_rrr
slti  001000  . .   
@fmt_rr_i12



and trans_xxx.c.inc

static bool gen_rrr(DisasContext *ctx, arg_rrr *a, ...) {}
static bool gen_rr_i12(DisasContext *ctx, arg_rr_i *a, ) {}
static bool gen_rrr_sa2p1(DisasContext *ctx, arg_rrr_sa *a, ...) {}
...

Richard, is that OK?

Thanks,
Song Gao

Re: [PATCH v4 3/9] linux-user/safe-syscall.inc.S: Move to common-user

On 11/16/21 22:03, Warner Losh wrote:
> On Tue, Nov 16, 2021 at 4:03 AM Richard Henderson
> mailto:richard.hender...@linaro.org>> wrote:
> 
> From: Warner Losh mailto:i...@bsdimp.com>>
> 
> Move all the safe_syscall.inc.S files to common-user. They are almost
> identical between linux-user and bsd-user to re-use.
> 
> Signed-off-by: Warner Losh mailto:i...@bsdimp.com>>
> Reviewed-by: Richard Henderson  >
> Message-Id: <2023045603.60391-4-...@bsdimp.com
> >
> Signed-off-by: Richard Henderson  >
> ---
>  meson.build                                                 | 1 +
>  {linux-user => common-user}/host/aarch64/hostdep.h          | 0
>  {linux-user => common-user}/host/arm/hostdep.h              | 0
>  {linux-user => common-user}/host/i386/hostdep.h             | 0
>  {linux-user => common-user}/host/ppc64/hostdep.h            | 0
>  {linux-user => common-user}/host/riscv/hostdep.h            | 0
>  {linux-user => common-user}/host/s390x/hostdep.h            | 0
>  {linux-user => common-user}/host/x86_64/hostdep.h           | 0
>  {linux-user => common-user}/host/aarch64/safe-syscall.inc.S | 0
>  {linux-user => common-user}/host/arm/safe-syscall.inc.S     | 0
>  {linux-user => common-user}/host/i386/safe-syscall.inc.S    | 0
>  {linux-user => common-user}/host/ppc64/safe-syscall.inc.S   | 0
>  {linux-user => common-user}/host/riscv/safe-syscall.inc.S   | 0
>  {linux-user => common-user}/host/s390x/safe-syscall.inc.S   | 0
>  {linux-user => common-user}/host/x86_64/safe-syscall.inc.S  | 0
>  15 files changed, 1 insertion(+)
>  rename {linux-user => common-user}/host/aarch64/hostdep.h (100%)
>  rename {linux-user => common-user}/host/arm/hostdep.h (100%)
>  rename {linux-user => common-user}/host/i386/hostdep.h (100%)
>  rename {linux-user => common-user}/host/ppc64/hostdep.h (100%)
>  rename {linux-user => common-user}/host/riscv/hostdep.h (100%)
>  rename {linux-user => common-user}/host/s390x/hostdep.h (100%)
>  rename {linux-user => common-user}/host/x86_64/hostdep.h (100%)
>  rename {linux-user => common-user}/host/aarch64/safe-syscall.inc.S
> (100%)
>  rename {linux-user => common-user}/host/arm/safe-syscall.inc.S (100%)
>  rename {linux-user => common-user}/host/i386/safe-syscall.inc.S (100%)
>  rename {linux-user => common-user}/host/ppc64/safe-syscall.inc.S (100%)
>  rename {linux-user => common-user}/host/riscv/safe-syscall.inc.S (100%)
>  rename {linux-user => common-user}/host/s390x/safe-syscall.inc.S (100%)
>  rename {linux-user => common-user}/host/x86_64/safe-syscall.inc.S
> (100%)
> 
> diff --git a/meson.build b/meson.build
> index ccc6cefc25..ec22cf05c1 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2878,6 +2878,7 @@ foreach target : target_dirs
>      if 'CONFIG_LINUX_USER' in config_target
>        base_dir = 'linux-user'
>        target_inc += include_directories('linux-user/host/' /
> config_host['ARCH'])
> +      target_inc += include_directories('common-user/host/' /
> config_host['ARCH'])
>      endif
>      if 'CONFIG_BSD_USER' in config_target
>        base_dir = 'bsd-user'
> 
> 
> I had to add this:
> 
> diff --git a/meson.build b/meson.build
> index 0a88bff8d2..349e7a988f 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2880,6 +2880,8 @@ foreach target : target_dirs
>      endif
>      if 'CONFIG_BSD_USER' in config_target
>        base_dir = 'bsd-user'
> +      target_inc += include_directories('bsd-user/host/' /
> config_host['ARCH'])
> +      target_inc += include_directories('common-user/host/' /
> config_host['ARCH'])
>        target_inc += include_directories('bsd-user/' / targetos)
>        dir = base_dir / abi
>        arch_srcs += files(dir / 'target_arch_cpu.c')
> 
> to get bsd-user compiling.

Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH 1/2] migration/colo: Optimize COLO start code path

2021-11-17 Thread Juan Quintela

"Zhang, Chen"  wrote:
>> -Original Message-
>> From: Juan Quintela 
>> Sent: Wednesday, November 17, 2021 12:28 AM
>> To: Zhang, Chen 
>> Cc: Hailiang Zhang ; Dr . David Alan
>> Gilbert ; qemu-dev 
>> Subject: Re: [PATCH 1/2] migration/colo: Optimize COLO start code path
>> 
>> Zhang Chen  wrote:
>> > There is no need to start COLO through MIGRATION_STATUS_ACTIVE.
>> 
>> Hi
>> 
>> I don't understand what you are trying to do.  In my reading, at least the
>> commit message is wrong:
>> 
>> void migrate_start_colo_process(MigrationState *s) {
>> ...
>> migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
>>   MIGRATION_STATUS_COLO);
>> ...
>> }
>> 
>> and
>> 
>> void *colo_process_incoming_thread(void *opaque) {
>> ...
>> migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
>>   MIGRATION_STATUS_COLO);
>> 
>> So colo starts with MIGRATION_STATUS_ACTIVE.
>
> Yes, this patch just optimized COLO primary code 
> path(migrate_start_colo_process()).
> We can see this patch removed the 
>  migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
>   MIGRATION_STATUS_COLO);
> In the migrate_start_colo_process().
>
> Current COLO status path:
>  MIGRATION_STATUS_XXX   --->   MIGRATION_STATUS_ACTIVE ---> 
> MIGRATION_STATUS_COLO ---> MIGRATION_STATUS_COMPLETED
>
> This patch try to remove redundant " MIGRATION_STATUS_ACTIVE " in COLO start. 
> MIGRATION_STATUS_XXX   ---> MIGRATION_STATUS_COLO ---> 
> MIGRATION_STATUS_COMPLETED
>
> Actually COLO primary code did nothing when running on 
> "MIGRATION_STATUS_ACTIVE".
> But for COLO secondary (void *colo_process_incoming_thread()), it shared some 
> code with normal migration. No need to do this.
>
> So, I will fix commit message to:
> Optimize COLO primary start path to:
> MIGRATION_STATUS_XXX   ---> MIGRATION_STATUS_COLO ---> 
> MIGRATION_STATUS_COMPLETED
> No need to start primary COLO through "MIGRATION_STATUS_ACTIVE".
>
> How about it?

Much better, thank.s

>> > Signed-off-by: Zhang Chen 
>> > ---
>> >  migration/colo.c  |  2 --
>> >  migration/migration.c | 18 +++---
>> >  2 files changed, 11 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/migration/colo.c b/migration/colo.c index
>> > 2415325262..ad1a4426b3 100644
>> > --- a/migration/colo.c
>> > +++ b/migration/colo.c
>> > @@ -667,8 +667,6 @@ void migrate_start_colo_process(MigrationState *s)
>> >  colo_checkpoint_notify, s);
>> >
>> >  qemu_sem_init(&s->colo_exit_sem, 0);
>> > -migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
>> > -  MIGRATION_STATUS_COLO);
>> >  colo_process_checkpoint(s);
>> >  qemu_mutex_lock_iothread();
>> >  }
>> > diff --git a/migration/migration.c b/migration/migration.c index
>> > abaf6f9e3d..4c8662a839 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -3222,7 +3222,10 @@ static void migration_completion(MigrationState
>> *s)
>> >  goto fail_invalidate;
>> >  }
>> >
>> > -if (!migrate_colo_enabled()) {
>> > +if (migrate_colo_enabled()) {
>> > +migrate_set_state(&s->state, current_active_state,
>> > +  MIGRATION_STATUS_COLO);
>> > +} else {
>> >  migrate_set_state(&s->state, current_active_state,
>> >MIGRATION_STATUS_COMPLETED);
>> >  }
>> 
>> This moves the setup to MIGRATION_STATUS_COLO to completion time
>> instead of the beggining of the process.  I have no clue why.  I guess you 
>> can
>> put a comment/commit message to say what you ar.e trynig to do.
>
> You are right, no need to setup here.
> I will remove this in next version.

Thanks.

>> > @@ -3607,12 +3610,7 @@ static void
>> migration_iteration_finish(MigrationState *s)
>> >  migration_calculate_complete(s);
>> >  runstate_set(RUN_STATE_POSTMIGRATE);
>> >  break;
>> > -
>> > -case MIGRATION_STATUS_ACTIVE:
>> > -/*
>> > - * We should really assert here, but since it's during
>> > - * migration, let's try to reduce the usage of assertions.
>> > - */
>> > +case MIGRATION_STATUS_COLO:
>> >  if (!migrate_colo_enabled()) {
>> >  error_report("%s: critical error: calling COLO code without "
>> >   "COLO enabled", __func__); @@ -3622,6
>> > +3620,12 @@ static void migration_iteration_finish(MigrationState *s)
>> >   * Fixme: we will run VM in COLO no matter its old running state.
>> >   * After exited COLO, we will keep running.
>> >   */
>> > + /* Fallthrough */
>> > +case MIGRATION_STATUS_ACTIVE:
>> > +/*
>> > + * We should really assert here, but since it's during
>> > + * migration, let's try to reduce the usage of assertions.
>> > + */
>> >  s->vm_was_running = true;
>> >  /* Fallthrough */
>> >  case MIGRATION_STATUS_FAILED:
>> 
>>

Re: [PATCH v4 7/9] linux-user: Remove HAVE_SAFE_SYSCALL and hostdep.h

On 11/16/21 12:02, Richard Henderson wrote:
> All supported hosts now define HAVE_SAFE_SYSCALL, so remove
> the ifdefs.  This leaves hostdep.h empty, so remove it.
> 
> Signed-off-by: Richard Henderson 
> ---
>  common-user/host/aarch64/hostdep.h | 18 --
>  common-user/host/arm/hostdep.h | 18 --
>  common-user/host/i386/hostdep.h| 18 --
>  common-user/host/mips/hostdep.h|  2 --
>  common-user/host/ppc64/hostdep.h   | 18 --
>  common-user/host/riscv/hostdep.h   | 14 --
>  common-user/host/s390x/hostdep.h   | 18 --
>  common-user/host/sparc64/hostdep.h |  2 --
>  common-user/host/x86_64/hostdep.h  | 18 --
>  linux-user/host/ia64/hostdep.h | 15 ---
>  linux-user/host/mips/hostdep.h | 15 ---
>  linux-user/host/ppc/hostdep.h  | 15 ---
>  linux-user/host/s390/hostdep.h | 15 ---
>  linux-user/host/sparc/hostdep.h| 15 ---
>  linux-user/host/sparc64/hostdep.h  | 15 ---
>  linux-user/host/x32/hostdep.h  | 15 ---
>  linux-user/safe-syscall.h  | 12 
>  linux-user/user-internals.h|  1 -
>  linux-user/signal.c|  2 --
>  linux-user/safe-syscall.S  |  3 ---
>  20 files changed, 249 deletions(-)
>  delete mode 100644 common-user/host/aarch64/hostdep.h
>  delete mode 100644 common-user/host/arm/hostdep.h
>  delete mode 100644 common-user/host/i386/hostdep.h
>  delete mode 100644 common-user/host/mips/hostdep.h
>  delete mode 100644 common-user/host/ppc64/hostdep.h
>  delete mode 100644 common-user/host/riscv/hostdep.h
>  delete mode 100644 common-user/host/s390x/hostdep.h
>  delete mode 100644 common-user/host/sparc64/hostdep.h
>  delete mode 100644 common-user/host/x86_64/hostdep.h
>  delete mode 100644 linux-user/host/ia64/hostdep.h
>  delete mode 100644 linux-user/host/mips/hostdep.h
>  delete mode 100644 linux-user/host/ppc/hostdep.h
>  delete mode 100644 linux-user/host/s390/hostdep.h
>  delete mode 100644 linux-user/host/sparc/hostdep.h
>  delete mode 100644 linux-user/host/sparc64/hostdep.h
>  delete mode 100644 linux-user/host/x32/hostdep.h

> diff --git a/linux-user/safe-syscall.h b/linux-user/safe-syscall.h
> -#ifdef HAVE_SAFE_SYSCALL
>  
>  /* The core part of this function is implemented in assembly. */
>  extern long safe_syscall_base(int *pending, int *errnop, long number, ...);
> @@ -137,15 +136,4 @@ extern char safe_syscall_end[];
>  safe_syscall_base(&((TaskState *)thread_cpu->opaque)->signal_pending, \
>&errno, __VA_ARGS__)
>  
> -#else
> -
> -/*
> - * Fallback for architectures which don't yet provide a safe-syscall assembly
> - * fragment; note that this is racy!
> - * This should go away when all host architectures have been updated.
> - */
> -#define safe_syscall syscall
> -
> -#endif

Good!

Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH] q35: turn off power_controller_present when acpi hotplug is enabled

2021-11-17 Thread Gerd Hoffmann

On Tue, Nov 16, 2021 at 08:26:41PM +0100, Igor Mammedov wrote:
> On Tue, 16 Nov 2021 10:04:33 +0100
> Gerd Hoffmann  wrote:
> 
> > Disable power control for pcie slots in case acpi hotplug is enabled
> > (6.2+ only for compatibility reasons).  This makes sure we don't get
> > unpleasant surprises with pci devices not being functional due to slot
> > power being turned off.
> > 
> > Signed-off-by: Gerd Hoffmann 
> > ---
> >  hw/i386/pc_q35.c | 13 ++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index e1e100316d93..869ca4c130f0 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -247,9 +247,16 @@ static void pc_q35_init(MachineState *machine)
> >   "x-keep-pci-slot-hpc",
> >   NULL);
> >  
> > -if (!keep_pci_slot_hpc && acpi_pcihp) {
> > -object_register_sugar_prop(TYPE_PCIE_SLOT, "x-native-hotplug",
> > -   "false", true);
> > +if (acpi_pcihp) {
> > +if (keep_pci_slot_hpc) {
> > +/* 6.2+ default: acpi-hotplug=on native-hotplug=on 
> > power-ctrl=off */
> > +object_register_sugar_prop(TYPE_PCIE_SLOT, COMPAT_PROP_PCP,
> > +   "false", true);
> 
> that will also turn off COMPAT_PROP_PCP on ports attached to PXBs,
> where ACPI hotplug is not used and native one is active.

Oh, wasn't aware of that detail.

> So question is if it's expected behavior?

Nope.  When native hotplug is used slot power control should be enabled.

Not sure how to handle that best though ...

take care,
  Gerd

Re: [RFC PATCH v2 09/30] target/loongarch: Add TLB instruction support


On 11/17/21 8:29 AM, yangxiaojuan wrote:

On 11/12/2021 02:14 AM, Richard Henderson wrote:

On 11/11/21 2:35 AM, Xiaojuan Yang wrote:

+static bool trans_tlbwr(DisasContext *ctx, arg_tlbwr *a)
+{
+gen_helper_check_plv(cpu_env);
+gen_helper_tlbwr(cpu_env);
+tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next + 4);
+ctx->base.is_jmp = DISAS_EXIT;
+return true;
+}


I think you can skip the EXIT if paging is disabled, which it usually will be 
in the software tlb handler.  You'd be able to tell with the mmu_idx being the 
one you use for paging disabled.


The paging disabled only enabled at the bios startup, we can get the phys 
address directly, tlbwr instruction will not be used when paging enabled.


Paging is also disabled during TLBRENTRY exception (see section 6.2.4 Hardware Exception 
Handling of TLB Refil Exception).  It is this routine that will usually use tlbwr most 
often (although the kernel at PRV 0 is not prevented from doing so).



+default:
+do_raise_exception(env, EXCP_INE, GETPC());


You can detect this during translation, and dispatch to the appropriate invtlb 
sub-function.


oh, sorry, I don't quiet understand this. detect during the translation sees 
more complicated.


It is not more complex at all.  Less complex, I would say.

static bool trans_invtlb(DisasContext *ctx, arg_invtlb *a)
{
TCGv rj = gpr_src(ctx, a->rj, EXT_NONE);
TCGv rk = gpr_src(ctx, a->rk, EXT_NONE);

if (check_plv(ctx)) {
return false;
}

switch (a->invop) {
case 0:
case 1:
gen_helper_invtlb_all(cpu_env);
break;
case 2:
gen_helper_invtlb_all_g(cpu_env, tcg_constant_i32(1));
break;
case 3:
gen_helper_invtlb_all_g(cpu_env, tcg_constant_i32(0));
break;
case 4:
gen_helper_invtlb_all_asid(cpu_env, rj);
break;
case 5:
gen_helper_invtlb_page_asid(cpu_env, rj, rk);
break;
case 6:
gen_helper_invtlb_page_asid_or_g(cpu_env, rj, rk);
break;
default:
return false;
}
ctx->base.is_jmp = DISAS_STOP;
return true;
}


r~

Re: [PATCH v4 8/9] common-user: Adjust system call return on FreeBSD

On 11/16/21 12:02, Richard Henderson wrote:
> From: Warner Losh 
> 
> FreeBSD system calls return positive errno.  On the 4 hosts for
> which we have support, error is indicated by the C bit set or clear.
> 
> Signed-off-by: Warner Losh 
> [rth: Rebase on new safe_syscall_base api; add #error check.]
> Signed-off-by: Richard Henderson 
> ---
>  common-user/host/aarch64/safe-syscall.inc.S | 12 +++-
>  common-user/host/arm/safe-syscall.inc.S | 11 +++

Can we split this in 2 patches?

>  common-user/host/i386/safe-syscall.inc.S| 10 ++
>  common-user/host/x86_64/safe-syscall.inc.S  | 10 ++
>  4 files changed, 42 insertions(+), 1 deletion(-)

RE: [PATCH] linux-user/hexagon: Use generic target_stat64 structure

2021-11-17 Thread Taylor Simpson



> -Original Message-
> From: Richard Henderson 
> Sent: Wednesday, November 17, 2021 1:18 AM
> To: Philippe Mathieu-Daudé ; qemu-devel@nongnu.org
> Cc: Laurent Vivier ; Taylor Simpson
> 
> Subject: Re: [PATCH] linux-user/hexagon: Use generic target_stat64
> structure
> 
> On 11/16/21 10:09 PM, Philippe Mathieu-Daudé wrote:
> > Linux Hexagon port doesn't define a specific 'struct stat'
> > but uses the generic one (see Linux commit 6103ec56c65c [*]
> > "asm-generic: add generic ABI headers" which predates the introduction
> > of the Hexagon port).
> >
> > Remove the target specific target_stat (which in fact is the
> > target_stat64 structure but uses incorrect target_long and ABI unsafe
> > long long types) and use the generic target_stat64 instead.
> >
> > [*]https://github.com/torvalds/linux/commit/6103ec56c65c3#diff-5f59b07
> > b38273b7d6a74193bc81a8cd18928c688276eae20cb10c569de3253ee
> >
> > Signed-off-by: Philippe Mathieu-Daudé
> > ---
> >   linux-user/syscall_defs.h | 28 ++--
> >   1 file changed, 2 insertions(+), 26 deletions(-)
> 
> Reviewed-by: Richard Henderson 

Reviewed-by: Taylor Simpson 
Tested-by: Taylor Simpson

Re: [PATCH v10 04/26] target/loongarch: Add fixed point arithmetic instruction translation


On 11/17/21 8:57 AM, gaosong wrote:
I see that  insns.decode format is not very consistent with other architectures, such 
ARM/RISCV


No.  I don't like how riscv has done it, though they have quite a few split fields, so 
perhaps they thought it looked weird.




#
# Argument sets
#
&r_i  rd imm
&rrr  rd rj rk
&rr_i rd rj imm
&rrr_sa rd rj rk sa

#
# Formats
#
@fmt_rrr   . rk:5 rj:5 rd:5 &rrr
@fmt_r_i20     ... imm:s20 rd:5 &r_i
@fmt_rr_i12    .. imm:s12 rj:5 rd:5 &rr_i
@fmt_rr_ui12    .. imm:12 rj:5 rd:5 &rr_i
@fmt_rr_i16    .. imm:s16 rj:5 rd:5 &rr_i
@fmt_rrr_sa2p1    ... .. rk:5 rj:5 rd:5 &rrr_sa  sa=%sa2p1

#
# Fixed point arithmetic operation instruction
#
add_w     0001 0 . . .    @fmt_rrr
add_d     0001 1 . . .    @fmt_rrr
sub_w     0001 00010 . . .    @fmt_rrr
sub_d     0001 00011 . . .    @fmt_rrr
slt   0001 00100 . . . @fmt_rrr
sltu  0001 00101 . . . @fmt_rrr
slti  001000  . .   @fmt_rr_i12


and trans_xxx.c.inc

static bool gen_rrr(DisasContext *ctx, arg_rrr *a, ...) {}
static bool gen_rr_i12(DisasContext *ctx, arg_rr_i *a, ) {}


gen_rr_i ?


static bool gen_rrr_sa2p1(DisasContext *ctx, arg_rrr_sa *a, ...) {}


gen_rrr_sa ?


Richard, is that OK?


Other than those two nits, this looks very clean.  Thanks,


r~

Re: [PULL 20/20] pcie: expire pending delete

2021-11-17 Thread Gerd Hoffmann

  Hi,

> >  dev->pending_deleted_event = true;
> > +dev->pending_deleted_expires_ms =
> > +qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 5000; /* 5 secs */
> 
> do we block migration if unplug was requested?

Not sure.

> (if not we might loose this state on destionatio, do we care about it?)

pending_deleted_event isn't transfered either, so it shouldn't be a
problem.  Worst case is that mgmt has to re-try sending device_del
on the migration target host.

take care,
  Gerd

Re: [PATCH v4 8/9] common-user: Adjust system call return on FreeBSD


On 11/17/21 9:23 AM, Philippe Mathieu-Daudé wrote:

On 11/16/21 12:02, Richard Henderson wrote:

From: Warner Losh 

FreeBSD system calls return positive errno.  On the 4 hosts for
which we have support, error is indicated by the C bit set or clear.

Signed-off-by: Warner Losh 
[rth: Rebase on new safe_syscall_base api; add #error check.]
Signed-off-by: Richard Henderson 
---
  common-user/host/aarch64/safe-syscall.inc.S | 12 +++-
  common-user/host/arm/safe-syscall.inc.S | 11 +++


Can we split this in 2 patches?


  common-user/host/i386/safe-syscall.inc.S| 10 ++
  common-user/host/x86_64/safe-syscall.inc.S  | 10 ++
  4 files changed, 42 insertions(+), 1 deletion(-)


Why 2?

They're small enough that I think having them all together is fine, but otherwise why 
wouldn't I split to 4?



r~

Re: [PATCH v4 8/9] common-user: Adjust system call return on FreeBSD

On 11/17/21 09:32, Richard Henderson wrote:
> On 11/17/21 9:23 AM, Philippe Mathieu-Daudé wrote:
>> On 11/16/21 12:02, Richard Henderson wrote:
>>> From: Warner Losh 
>>>
>>> FreeBSD system calls return positive errno.  On the 4 hosts for
>>> which we have support, error is indicated by the C bit set or clear.
>>>
>>> Signed-off-by: Warner Losh 
>>> [rth: Rebase on new safe_syscall_base api; add #error check.]
>>> Signed-off-by: Richard Henderson 
>>> ---
>>>   common-user/host/aarch64/safe-syscall.inc.S | 12 +++-
>>>   common-user/host/arm/safe-syscall.inc.S | 11 +++
>>
>> Can we split this in 2 patches?
>>
>>>   common-user/host/i386/safe-syscall.inc.S    | 10 ++
>>>   common-user/host/x86_64/safe-syscall.inc.S  | 10 ++
>>>   4 files changed, 42 insertions(+), 1 deletion(-)
> 
> Why 2?

Personal brain limitation, it is easier to me when I focus on
one base arch at a time. Previous mips/sparc64 changes are
in different patches.

> They're small enough that I think having them all together is fine, but
> otherwise why wouldn't I split to 4?

4 is even better for my brain, but I think I could force my brain
to focus in 1 hunk at a time in a single patch :)

Re: [PULL SUBSYSTEM qemu-pseries] pseries: Update SLOF firmware image


On 11/16/21 11:48 PM, Alexey Kardashevskiy wrote:
Yup. I am doing SLOF updates this way for ages after diifs became quite huge to make 
mailman barfing on the size, and the "subsystem" in the subj was the way to reduce the 
noise Peter had to respond to :)


btw should I be signing those? I am not now.


You could if you and Cedric want to do so (is it really Alexey sending the sub pr and not 
an impostor with access to the same github account?), but it will not leave a permanent 
record in the mainline history, because the merge object with the signature will be 
removed by any rebase.



r~

[PATCH V2] migration/colo: Optimize COLO primary node start code path

2021-11-17 Thread Zhang Chen

Optimize COLO primary start path from:
MIGRATION_STATUS_XXX --> MIGRATION_STATUS_ACTIVE --> MIGRATION_STATUS_COLO --> 
MIGRATION_STATUS_COMPLETED
To:
MIGRATION_STATUS_XXX --> MIGRATION_STATUS_COLO --> MIGRATION_STATUS_COMPLETED
No need to start primary COLO through "MIGRATION_STATUS_ACTIVE".

Signed-off-by: Zhang Chen 
---
 migration/colo.c  |  2 --
 migration/migration.c | 13 +++--
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index 2415325262..ad1a4426b3 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -667,8 +667,6 @@ void migrate_start_colo_process(MigrationState *s)
 colo_checkpoint_notify, s);
 
 qemu_sem_init(&s->colo_exit_sem, 0);
-migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
-  MIGRATION_STATUS_COLO);
 colo_process_checkpoint(s);
 qemu_mutex_lock_iothread();
 }
diff --git a/migration/migration.c b/migration/migration.c
index abaf6f9e3d..380fa05f12 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3607,12 +3607,7 @@ static void migration_iteration_finish(MigrationState *s)
 migration_calculate_complete(s);
 runstate_set(RUN_STATE_POSTMIGRATE);
 break;
-
-case MIGRATION_STATUS_ACTIVE:
-/*
- * We should really assert here, but since it's during
- * migration, let's try to reduce the usage of assertions.
- */
+case MIGRATION_STATUS_COLO:
 if (!migrate_colo_enabled()) {
 error_report("%s: critical error: calling COLO code without "
  "COLO enabled", __func__);
@@ -3622,6 +3617,12 @@ static void migration_iteration_finish(MigrationState *s)
  * Fixme: we will run VM in COLO no matter its old running state.
  * After exited COLO, we will keep running.
  */
+ /* Fallthrough */
+case MIGRATION_STATUS_ACTIVE:
+/*
+ * We should really assert here, but since it's during
+ * migration, let's try to reduce the usage of assertions.
+ */
 s->vm_was_running = true;
 /* Fallthrough */
 case MIGRATION_STATUS_FAILED:
-- 
2.25.1

Re: [PULL 0/5] Python patches


On 11/17/21 1:33 AM, John Snow wrote:

The following changes since commit 2b22e7540d6ab4efe82d442363e3fc900cea6584:

   Merge tag 'm68k-for-6.2-pull-request' of git://github.com/vivier/qemu-m68k 
into staging (2021-11-09 13:16:56 +0100)

are available in the Git repository at:

   https://gitlab.com/jsnow/qemu.git tags/python-pull-request

for you to fetch changes up to c398a241ec4138e0b995a0215dea84ca93b0384f:

   scripts/device-crash-test: hide tracebacks for QMP connect errors 
(2021-11-16 14:26:36 -0500)


Pull request



John Snow (5):
   python/aqmp: Fix disconnect during capabilities negotiation
   python/aqmp: fix ConnectError string method
   scripts/device-crash-test: simplify Exception handling
   scripts/device-crash-test: don't emit AQMP connection errors to stdout
   scripts/device-crash-test: hide tracebacks for QMP connect errors

  python/qemu/aqmp/protocol.py | 24 ++--
  scripts/device-crash-test| 33 +
  2 files changed, 43 insertions(+), 14 deletions(-)


Applied, thanks.

r~

Re: [RFC PATCH v2 08/30] target/loongarch: Add LoongArch CSR/IOCSR instruction

2021-11-17 Thread yangxiaojuan

Hi, Richard:

On 11/12/2021 01:43 AM, Richard Henderson wrote:
> On 11/11/21 2:35 AM, Xiaojuan Yang wrote:
>> This includes:
>> - CSRRD
>> - CSRWR
>> - CSRXCHG
>> - IOCSR{RD/WR}.{B/H/W/D}
> 
> I think IOCSR should be in a separate patch.
> It's completely unrelated to the other CSRs.
> 
>> +target_ulong helper_csr_rdq(CPULoongArchState *env, uint64_t csr)
>> +{
>> +int64_t v;
>> +
>> +switch (csr) {
>> +case LOONGARCH_CSR_PGD:
>> +if (env->CSR_TLBRERA & 0x1) {
>> +v = env->CSR_TLBRBADV;
>> +} else {
>> +v = env->CSR_BADV;
>> +}
>> +
>> +if ((v >> 63) & 0x1) {
>> +v = env->CSR_PGDH;
>> +} else {
>> +v = env->CSR_PGDL;
>> +}
>> +v = v & TARGET_PHYS_MASK;
> 
> This csr is defined to be GRLEN bits; I this mask looks wrong.
> 
>> +default:
>> +assert(0);
> 
> g_assert_not_reached.
> 
>> +switch (csr) {
>> +case LOONGARCH_CSR_ASID:
>> +old_v = env->CSR_ASID;
>> +env->CSR_ASID = val;
> 
> Mask the write to the field; you don't want to corrupt ASIDBITS, or the other 
> read-only bits.

Ok, all above have been modified.

> 
>> +case LOONGARCH_CSR_TCFG:
>> +old_v = env->CSR_TCFG;
>> +cpu_loongarch_store_stable_timer_config(env, val);
>> +break;
>> +case LOONGARCH_CSR_TINTCLR:
>> +old_v = 0;
>> +qemu_irq_lower(env->irq[IRQ_TIMER]);
> 
> The interrupt is not documented to clear on any write; only writes of 1 to 
> bit 0.

I think the manual has mentioned at 7.6.5 which says when 1 is written to this 
bit, the clock interrupt 
flag is cleared.

> 
>> +default:
>> +assert(0);
> 
> g_assert_not_reached.
> 
>> +}
>> +
>> +return old_v;
>> +}
>> +
>> +target_ulong helper_csr_xchgq(CPULoongArchState *env, target_ulong val,
>> +  target_ulong mask, uint64_t csr)
>> +{
>> +target_ulong tmp;
>> +target_ulong v = val & mask;
> 
> I think it would be less confusing to name the input parameter new_val, and 
> the local temporary old_val.
> 
>> +#define CASE_CSR_XCHGQ(csr) \
>> +case LOONGARCH_CSR_ ## csr: \
>> +{   \
>> +val = env->CSR_ ## csr; \
>> +env->CSR_ ## csr = (env->CSR_ ## csr) & (~mask);\
>> +env->CSR_ ## csr = (env->CSR_ ## csr) | v;  \
> 
>   old_val = env->CSR_##csr;
>   env->CSR_##csr = (old_val & ~mask) | (new_val & mask);
> 
> 
>> +switch (csr) {
>> +CASE_CSR_XCHGQ(CRMD)
> 
> I wonder if all of this would be better with a table of offsets, which could 
> be shared with the translator.
> 
> #define CSR_OFF(X)  [LOONGARCH_CSR_##X] = offsetof(CPUArchState, CSR_##X)
> 
> static const int csr_offsets[] = {
> CSR_OFF(CRMD),
> ...
> };
> 
> int cpu_csr_offset(unsigned csr_num)
> {
> if (csr_num < ARRAY_SIZE(csr_offsets)) {
> return csr_offsets[csr_num];
> }
> return 0;
> }
> 
> Which reduces this function to
> 
> unsigned csr_offset = cpu_csr_offset(csr_num);
> if (csr_offset == 0) {
> /* CSR is undefined: read as 0, write ignored. */
> return 0;
> }
> 
> uint64_t *csr = (void *)env + csr_offset;
> uint64_t old_val = *csr;
> 
> new_val = (new_val & mask) | (old_val & ~mask);
> 
> *csr = (old_val & ~mask) | (new_val & mask);
> 
> if (csr_num == LOONGARCH_CSR_TCFG) {
> cpu_loongarch_store_stable_timer_config(env, new_val);
> } else {
> *csr = new_val;
> }
> return old_val;
> 
>> +uint64_t helper_iocsr_read(CPULoongArchState *env, target_ulong r_addr,
>> +   uint32_t size)
>> +{
>> +LoongArchMachineState *lams = LOONGARCH_MACHINE(qdev_get_machine());
>> +int cpuid = env_cpu(env)->cpu_index;
>> +
>> +if (((r_addr & 0xff00) == 0x1000) || ((r_addr & 0xff00) == 0x1800)) {
>> +r_addr = r_addr + ((target_ulong)(cpuid & 0x3) << 8);
>> +}
> 
> This looks to be something that should be controlled by the address space 
> assigned to each cpu.
> 
>   But it's hard to tell.
> 
> Where is the documentation for this?  I didn't immediately find it in 3A5000 
> Technical Reference Manual, Chapter 10.

Yes, most iocsr instructions introduced on 3A5000 Technical Reference Manual, 
Chapter 10. 
Table 10-2, 10-3, 10-4, 10-5 and 11-10 lists per core iocsr

> 
>> +void helper_iocsr_write(CPULoongArchState *env, target_ulong w_addr,
>> +target_ulong val, uint32_t size)
>> +{
>> +LoongArchMachineState *lams = LOONGARCH_MACHINE(qdev_get_machine());
>> +int cpuid = env_cpu(env)->cpu_index;
>> +int mask, i;
>> +
>> +/*
>> + * For IPI send, Mail send, ANY send adjust addr and val
>> + * according to their real meaning
>> + */
>> +if (w_addr == 0x1040) { /* IPI send */
>> +cpuid = (val >> 16) & 0x3ff

Re: [PULL SUBSYSTEM qemu-pseries] pseries: Update SLOF firmware image


On 11/17/21 09:39, Richard Henderson wrote:

On 11/16/21 11:48 PM, Alexey Kardashevskiy wrote:

Yup. I am doing SLOF updates this way for ages after diifs became quite huge to make 
mailman barfing on the size, and the "subsystem" in the subj was the way to 
reduce the noise Peter had to respond to :)

btw should I be signing those? I am not now.


You could if you and Cedric want to do so (is it really Alexey sending the sub 
pr and not an impostor with access to the same github account?), but it will 
not leave a permanent record in the mainline history, because the merge object 
with the signature will be removed by any rebase.



Yes. I just noticed that the merge commit is lost :

2021-11-09 15:50 +0100 Christophe Lombard o [ppc-7.0] 
{origin/ppc-7.0} pci-host: Allow extended config space access for PowerNV PHB4 
model
2021-11-16 17:39 +0100 Cédric Le Goater   M─┐ Merge tag 
'qemu-slof-2022' of github.com:aik/qemu into ppc-7.0
2021-11-13 14:47 +1100 Alexey Kardashevskiy   │ o pseries: 
Update SLOF firmware image
2021-11-12 12:28 +0100 Richard Henderson  M─│─┐ Merge tag 
'pull-ppc-2022' of https://github.com/legoater/qemu into staging
2021-11-10 17:25 -0300 Daniel Henrique Barboza│ o─┘ 
{origin/ppc-for-upstream} {origin/ppc-next}  
ppc/mmu_helper.c: do not truncate 'ea
...

After rebase :

2021-11-09 15:50 +0100 Christophe Lombard o [ppc-7.0] 
pci-host: Allow extended config space access for PowerNV PHB4 model
2021-11-13 14:47 +1100 Alexey Kardashevskiy   o pseries: Update 
SLOF firmware image
2021-11-16 21:07 +0100 Richard Henderson  o Update version 
for v6.2.0-rc1 release
2021-11-16 18:55 +0100 Richard Henderson  M─┐ Merge tag 
'pull-nbd-2021-11-16' of https://repo.or.cz/qemu/ericb into staging
...

Alexey's commit appears in the list with only his SoB and we loose :

Merge tag 'qemu-slof-2022' of github.com:aik/qemu into ppc-7.0

* tag 'qemu-slof-2022' of github.com:aik/qemu:
  pseries: Update SLOF firmware image

Signed-off-by: Cédric Le Goater 

which I find interesting to identify where the SLOF blob is coming from.


Thanks,

C.

Re: [RFC PATCH v2 09/30] target/loongarch: Add TLB instruction support

2021-11-17 Thread yangxiaojuan




On 11/17/2021 04:22 PM, Richard Henderson wrote:
> On 11/17/21 8:29 AM, yangxiaojuan wrote:
>> On 11/12/2021 02:14 AM, Richard Henderson wrote:
>>> On 11/11/21 2:35 AM, Xiaojuan Yang wrote:
 +static bool trans_tlbwr(DisasContext *ctx, arg_tlbwr *a)
 +{
 +gen_helper_check_plv(cpu_env);
 +gen_helper_tlbwr(cpu_env);
 +tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next + 4);
 +ctx->base.is_jmp = DISAS_EXIT;
 +return true;
 +}
>>>
>>> I think you can skip the EXIT if paging is disabled, which it usually will 
>>> be in the software tlb handler.  You'd be able to tell with the mmu_idx 
>>> being the one you use for paging disabled.
>>
>> The paging disabled only enabled at the bios startup, we can get the phys 
>> address directly, tlbwr instruction will not be used when paging enabled.
> 
> Paging is also disabled during TLBRENTRY exception (see section 6.2.4 
> Hardware Exception Handling of TLB Refil Exception).  It is this routine that 
> will usually use tlbwr most often (although the kernel at PRV 0 is not 
> prevented from doing so).

Sorry, I forgot this situation.

> 
 +default:
 +do_raise_exception(env, EXCP_INE, GETPC());
>>>
>>> You can detect this during translation, and dispatch to the appropriate 
>>> invtlb sub-function.
>>>
>> oh, sorry, I don't quiet understand this. detect during the translation sees 
>> more complicated.
> 
> It is not more complex at all.  Less complex, I would say.
> 
> static bool trans_invtlb(DisasContext *ctx, arg_invtlb *a)
> {
> TCGv rj = gpr_src(ctx, a->rj, EXT_NONE);
> TCGv rk = gpr_src(ctx, a->rk, EXT_NONE);
> 
> if (check_plv(ctx)) {
> return false;
> }
> 
> switch (a->invop) {
> case 0:
> case 1:
> gen_helper_invtlb_all(cpu_env);
> break;
> case 2:
> gen_helper_invtlb_all_g(cpu_env, tcg_constant_i32(1));
> break;
> case 3:
> gen_helper_invtlb_all_g(cpu_env, tcg_constant_i32(0));
> break;
> case 4:
> gen_helper_invtlb_all_asid(cpu_env, rj);
> break;
> case 5:
> gen_helper_invtlb_page_asid(cpu_env, rj, rk);
> break;
> case 6:
> gen_helper_invtlb_page_asid_or_g(cpu_env, rj, rk);
> break;
> default:
> return false;
> }
> ctx->base.is_jmp = DISAS_STOP;
> return true;
> }
> 
Thank you. I get it.
> 
> r~

Re: [PATCH] pci-host: Allow extended config space access for PowerNV PHB4 model


On 11/17/21 08:59, Philippe Mathieu-Daudé wrote:

On 11/9/21 17:04, Cédric Le Goater wrote:

On 11/9/21 16:51, Frederic Barrat wrote:



On 09/11/2021 15:50, Christophe Lombard wrote:

The PCIe extended configuration space on the device is not currently
accessible to the host. if by default,  it is still inaccessible for
conventional for PCIe buses, add the current flag
PCI_BUS_EXTENDED_CONFIG_SPACE on the root bus permits PCI-E extended
config space access.


For the record, this is coming from an experiment of plugging a
CXL device on a QEMU PowerNV POWER10 machine (baremetal). Only
minor changes (64 bits ops) were required to get it working.


Since this note could be helpful when having future retrospective,
do you mind amending this note to the commit description?


Yes. I agree. Please do.
 

I wonder where we are with the CXL models ?


IIRC Ben worked actively, asked help to the community but received
very few, basically because there is not enough man power IMHO.

Last thing I remember is Igor suggested a different design approach:
https://lore.kernel.org/qemu-devel/20210319180705.6ede9...@redhat.com/


Well, the CXL Linux driver seemed to work fine on QEMU machines, Intel
and POWER.

Thanks for the info,

C.

Re: [PATCH v3 3/3] docs: rSTify the "SubmitAPatch" wiki


On 10/11/2021 15.49, Kashyap Chamarthy wrote:

- The original wiki is here[1]. I copied the wiki source[2] into a .wiki
   file, and used `pandoc` to convert it to rST:

 $> pandoc -f Mediawiki -t rst submitting-a-patch.wiki -o
submitting-a-patch.rst

- The only minor touch-ups I did was to fix URLs.  But 99%, it is a 1-1
   conversion.

   (An example of a "touch-up": under the section "Patch emails must
   include a Signed-off-by: line", I updated the "see SubmittingPatches
   1.12"  to "1.12) Sign your work")

- I have also converted a couple other related wiki pages (included in
   this patch series) that were hyperlinked within the SubmitAPatch page,
   or a page that it refers to:

   - SubmitAPullRequest: https://wiki.qemu.org/Contribute/SubmitAPullRequest
   - TrivialPatches: https://wiki.qemu.org/Contribute/TrivialPatches

- Over time, many people contributed to this wiki page; you can find all
   the authors in the wiki history[3].

[1] https://wiki.qemu.org/Contribute/SubmitAPatch
[2] http://wiki.qemu.org/index.php?title=Contribute/SubmitAPatch&action=edit
[3] http://wiki.qemu.org/index.php?title=Contribute/SubmitAPatch&action=history

Signed-off-by: Kashyap Chamarthy 
---
  docs/devel/index.rst  |   1 +
  docs/devel/submitting-a-patch.rst | 456 ++
  2 files changed, 457 insertions(+)
  create mode 100644 docs/devel/submitting-a-patch.rst

diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index 816eb7b7b0..c3cfa9e41f 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -47,3 +47,4 @@ modifying QEMU's source code.
 writing-qmp-commands
 trivial-patches
 submitting-a-pull-request
+   submitting-a-patch


I'd suggest to insert this before the pull-request entry, in case anybody 
reads the manual sequentially, it might be better to learn about the patch 
submission process first before reading about pull requests.

(I can fix this up when picking up the patch, no need to resend)


diff --git a/docs/devel/submitting-a-patch.rst 
b/docs/devel/submitting-a-patch.rst
new file mode 100644
index 00..c80dad47fa
--- /dev/null
+++ b/docs/devel/submitting-a-patch.rst

...

+Split up long patches
+~
+
+Split up longer patches into a patch series of logical code changes.
+Each change should compile and execute successfully. For instance, don't
+add a file to the makefile in patch one and then add the file itself in
+patch two. (This rule is here so that people can later use tools like
+```git bisect`` `__ without hitting


That hyperlink showed up in the rendered output. I'll fix it up by removing 
the "``" quotes.



+.. _write_a_meaningful_commit_message:
+
+Write a meaningful commit message
+~
+
+Commit messages should be meaningful and should stand on their own as a
+historical record of why the changes you applied were necessary or
+useful.
+
+QEMU follows the usual standard for git commit messages: the first line
+(which becomes the email subject line) is "subsystem: single line
+summary of change". Whether the "single line summary of change" starts
+with a capital is a matter of taste, but we prefer that the summary does
+not end in ".".


That ".". looks a little bit weird in the output ... maybe we should replace 
it with "does not end with a dot." ?



Look at ``git shortlog -30`` for an idea of sample
+subject lines. Then there is a blank line and a more detailed
+description of the patch, another blank and your Signed-off-by: line.
+Please do not use lines that are longer than 76 characters in your
+commit message (so that the text still shows up nicely with "git show"
+in a 80-columns terminal window).
+
+The body of the commit message is a good place to document why your
+change is important. Don't include comments like "This is a suggestion
+for fixing this bug" (they can go below the "---" line in the email so


That --- gets translated into a — character ... I'll replace the "---" with 
``---`` to fix it.


 Thomas

[PULL 1/2] target/riscv: machine: Sort the .subsections

2021-11-17 Thread Alistair Francis

From: Bin Meng 

Move the codes around so that the order of .subsections matches
the one they are referenced in vmstate_riscv_cpu.

Signed-off-by: Bin Meng 
Reviewed-by: Alistair Francis 
Message-id: 20211030030606.32297-1-bmeng...@gmail.com
Signed-off-by: Alistair Francis 
---
 target/riscv/machine.c | 92 +-
 1 file changed, 46 insertions(+), 46 deletions(-)

diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index 7b4c739564..ad8248ebfd 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -76,20 +76,50 @@ static bool hyper_needed(void *opaque)
 return riscv_has_ext(env, RVH);
 }
 
-static bool vector_needed(void *opaque)
-{
-RISCVCPU *cpu = opaque;
-CPURISCVState *env = &cpu->env;
+static const VMStateDescription vmstate_hyper = {
+.name = "cpu/hyper",
+.version_id = 1,
+.minimum_version_id = 1,
+.needed = hyper_needed,
+.fields = (VMStateField[]) {
+VMSTATE_UINTTL(env.hstatus, RISCVCPU),
+VMSTATE_UINTTL(env.hedeleg, RISCVCPU),
+VMSTATE_UINTTL(env.hideleg, RISCVCPU),
+VMSTATE_UINTTL(env.hcounteren, RISCVCPU),
+VMSTATE_UINTTL(env.htval, RISCVCPU),
+VMSTATE_UINTTL(env.htinst, RISCVCPU),
+VMSTATE_UINTTL(env.hgatp, RISCVCPU),
+VMSTATE_UINT64(env.htimedelta, RISCVCPU),
 
-return riscv_has_ext(env, RVV);
-}
+VMSTATE_UINT64(env.vsstatus, RISCVCPU),
+VMSTATE_UINTTL(env.vstvec, RISCVCPU),
+VMSTATE_UINTTL(env.vsscratch, RISCVCPU),
+VMSTATE_UINTTL(env.vsepc, RISCVCPU),
+VMSTATE_UINTTL(env.vscause, RISCVCPU),
+VMSTATE_UINTTL(env.vstval, RISCVCPU),
+VMSTATE_UINTTL(env.vsatp, RISCVCPU),
 
-static bool pointermasking_needed(void *opaque)
+VMSTATE_UINTTL(env.mtval2, RISCVCPU),
+VMSTATE_UINTTL(env.mtinst, RISCVCPU),
+
+VMSTATE_UINTTL(env.stvec_hs, RISCVCPU),
+VMSTATE_UINTTL(env.sscratch_hs, RISCVCPU),
+VMSTATE_UINTTL(env.sepc_hs, RISCVCPU),
+VMSTATE_UINTTL(env.scause_hs, RISCVCPU),
+VMSTATE_UINTTL(env.stval_hs, RISCVCPU),
+VMSTATE_UINTTL(env.satp_hs, RISCVCPU),
+VMSTATE_UINT64(env.mstatus_hs, RISCVCPU),
+
+VMSTATE_END_OF_LIST()
+}
+};
+
+static bool vector_needed(void *opaque)
 {
 RISCVCPU *cpu = opaque;
 CPURISCVState *env = &cpu->env;
 
-return riscv_has_ext(env, RVJ);
+return riscv_has_ext(env, RVV);
 }
 
 static const VMStateDescription vmstate_vector = {
@@ -108,6 +138,14 @@ static const VMStateDescription vmstate_vector = {
 }
 };
 
+static bool pointermasking_needed(void *opaque)
+{
+RISCVCPU *cpu = opaque;
+CPURISCVState *env = &cpu->env;
+
+return riscv_has_ext(env, RVJ);
+}
+
 static const VMStateDescription vmstate_pointermasking = {
 .name = "cpu/pointer_masking",
 .version_id = 1,
@@ -126,44 +164,6 @@ static const VMStateDescription vmstate_pointermasking = {
 }
 };
 
-static const VMStateDescription vmstate_hyper = {
-.name = "cpu/hyper",
-.version_id = 1,
-.minimum_version_id = 1,
-.needed = hyper_needed,
-.fields = (VMStateField[]) {
-VMSTATE_UINTTL(env.hstatus, RISCVCPU),
-VMSTATE_UINTTL(env.hedeleg, RISCVCPU),
-VMSTATE_UINTTL(env.hideleg, RISCVCPU),
-VMSTATE_UINTTL(env.hcounteren, RISCVCPU),
-VMSTATE_UINTTL(env.htval, RISCVCPU),
-VMSTATE_UINTTL(env.htinst, RISCVCPU),
-VMSTATE_UINTTL(env.hgatp, RISCVCPU),
-VMSTATE_UINT64(env.htimedelta, RISCVCPU),
-
-VMSTATE_UINT64(env.vsstatus, RISCVCPU),
-VMSTATE_UINTTL(env.vstvec, RISCVCPU),
-VMSTATE_UINTTL(env.vsscratch, RISCVCPU),
-VMSTATE_UINTTL(env.vsepc, RISCVCPU),
-VMSTATE_UINTTL(env.vscause, RISCVCPU),
-VMSTATE_UINTTL(env.vstval, RISCVCPU),
-VMSTATE_UINTTL(env.vsatp, RISCVCPU),
-
-VMSTATE_UINTTL(env.mtval2, RISCVCPU),
-VMSTATE_UINTTL(env.mtinst, RISCVCPU),
-
-VMSTATE_UINTTL(env.stvec_hs, RISCVCPU),
-VMSTATE_UINTTL(env.sscratch_hs, RISCVCPU),
-VMSTATE_UINTTL(env.sepc_hs, RISCVCPU),
-VMSTATE_UINTTL(env.scause_hs, RISCVCPU),
-VMSTATE_UINTTL(env.stval_hs, RISCVCPU),
-VMSTATE_UINTTL(env.satp_hs, RISCVCPU),
-VMSTATE_UINT64(env.mstatus_hs, RISCVCPU),
-
-VMSTATE_END_OF_LIST()
-}
-};
-
 const VMStateDescription vmstate_riscv_cpu = {
 .name = "cpu",
 .version_id = 3,
-- 
2.31.1

[PULL 0/2] riscv-to-apply queue

2021-11-17 Thread Alistair Francis

From: Alistair Francis 

The following changes since commit 8d5fcb1990bc64b62c0bc12121fe510940be5664:

  Merge tag 'python-pull-request' of https://gitlab.com/jsnow/qemu into staging 
(2021-11-17 07:41:08 +0100)

are available in the Git repository at:

  g...@github.com:alistair23/qemu.git tags/pull-riscv-to-apply-2027-1

for you to fetch changes up to c94c239496256f1f1cb589825d052c2f3e26ebf6:

  meson.build: Merge riscv32 and riscv64 cpu family (2021-11-17 19:18:22 +1000)


Sixth RISC-V PR for QEMU 6.2

 - Fix build for riscv hosts
 - Soft code alphabetically


Bin Meng (1):
  target/riscv: machine: Sort the .subsections

Richard Henderson (1):
  meson.build: Merge riscv32 and riscv64 cpu family

 meson.build|  6 
 target/riscv/machine.c | 92 +-
 2 files changed, 52 insertions(+), 46 deletions(-)

[PULL 2/2] meson.build: Merge riscv32 and riscv64 cpu family

2021-11-17 Thread Alistair Francis

From: Richard Henderson 

In ba0e73336200, we merged riscv32 and riscv64 in configure.
However, meson does not treat them the same.  We need to merge
them here as well.

Fixes: ba0e73336200
Signed-off-by: Richard Henderson 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Alistair Francis 
Message-id: 2026095042.335224-1-richard.hender...@linaro.org
Signed-off-by: Alistair Francis 
---
 meson.build | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/meson.build b/meson.build
index 36540e0794..e2d38a43e6 100644
--- a/meson.build
+++ b/meson.build
@@ -59,6 +59,12 @@ supported_cpus = ['ppc', 'ppc64', 's390x', 'riscv', 'x86', 
'x86_64',
   'arm', 'aarch64', 'mips', 'mips64', 'sparc', 'sparc64']
 
 cpu = host_machine.cpu_family()
+
+# Unify riscv* to a single family.
+if cpu in ['riscv32', 'riscv64']
+  cpu = 'riscv'
+endif
+
 targetos = host_machine.system()
 
 if cpu in ['x86', 'x86_64']
-- 
2.31.1

Re: [PATCH v10 04/26] target/loongarch: Add fixed point arithmetic instruction translation

2021-11-17 Thread gaosong


Hi Richard,

On 2021/11/17 下午4:28, Richard Henderson wrote:

On 11/17/21 8:57 AM, gaosong wrote:
I see that  insns.decode format is not very consistent with other 
architectures, such ARM/RISCV


No.  I don't like how riscv has done it, though they have quite a few 
split fields, so perhaps they thought it looked weird.




#
# Argument sets
#
&r_i  rd imm
&rrr  rd rj rk
&rr_i rd rj imm
&rrr_sa rd rj rk sa

#
# Formats
#
@fmt_rrr   . rk:5 rj:5 rd:5 &rrr
@fmt_r_i20     ... imm:s20 rd:5 &r_i
@fmt_rr_i12    .. imm:s12 rj:5 rd:5 &rr_i
@fmt_rr_ui12    .. imm:12 rj:5 rd:5 &rr_i
@fmt_rr_i16    .. imm:s16 rj:5 rd:5 &rr_i
@fmt_rrr_sa2p1    ... .. rk:5 rj:5 rd:5 &rrr_sa  
sa=%sa2p1


#
# Fixed point arithmetic operation instruction
#
add_w     0001 0 . . . @fmt_rrr
add_d     0001 1 . . . @fmt_rrr
sub_w     0001 00010 . . . @fmt_rrr
sub_d     0001 00011 . . . @fmt_rrr
slt   0001 00100 . . . @fmt_rrr
sltu  0001 00101 . . . @fmt_rrr
slti  001000  . .   
@fmt_rr_i12



and trans_xxx.c.inc

static bool gen_rrr(DisasContext *ctx, arg_rrr *a, ...) {}
static bool gen_rr_i12(DisasContext *ctx, arg_rr_i *a, ) {}


gen_rr_i ?


The code is not written completely,  like this:

gen_rr_i12:

@fmt_rr_i12    .. imm:s12 rj:5 rd:5 &rr_i
slti  001000  . . @fmt_rr_i12
sltui 001001  . . @fmt_rr_i12
...

gen_rr_ui12:

@fmt_rr_ui12    .. imm:12 rj:5 rd:5 &rr_i
andi  001101  . . @fmt_rr_ui12
ori   001110  . . @fmt_rr_ui12
xori  00  . . @fmt_rr_ui12
...

@fmt_rr_i12 and @fmt_rr_ui12 are two 'Formats',  but they use the same 
'Argument sets'(rr_i).




static bool gen_rrr_sa2p1(DisasContext *ctx, arg_rrr_sa *a, ...) {}


gen_rrr_sa ?


Likewise.

gen_rrr_sa2p1:

@fmt_rrr_sa2p1  ... .. rk:5 rj:5 rd:5   &fmt_rr_sa  
sa=%sa2p1
lsl_w     010 .. . . .@fmt_rrr_sa2p1
alsl_wu    011 .. . . .   @fmt_rrr_sa2p1
alsl_d    0010 110 .. . . .   @fmt_rrr_sa2p1
...

gen_rrr_sa2:
@fmt_rrr_sa2  ... sa:2 rk:5 rj:5 rd:5   &fmt_rr_sa
bytepick_w     100 .. . . .   @fmt_rrr_sa3
...

gen_rrr_sa3:
@fmt_rrr_sa3   .. sa:3 rk:5 rj:5 rd:5   &fmt_rr_sa
bytepick_d     11 ... . . .   @fmt_rrr_sa3
...


Richard, is that OK?


Other than those two nits, this looks very clean.  Thanks,


OK, I'll correct it on v11.

Thanks.
Song Gao

Re: [PULL 0/5] Python patches

2021-11-17 Thread Gerd Hoffmann

  Hi,

>   https://gitlab.com/jsnow/qemu.git tags/python-pull-request

What is the status of the plan to upload this to pypi eventually?

thanks,
  Gerd

Re: [PATCH v2 1/3] icount: preserve cflags when custom tb is about to execute



Pavel Dovgalyuk  writes:

> When debugging with the watchpoints, qemu may need to create
> TB with single instruction. This is achieved by setting cpu->cflags_next_tb.
> But when this block is about to execute, it may be interrupted by another
> thread. In this case cflags will be lost and next executed TB will not
> be the special one.
> This patch checks TB exit reason and restores cflags_next_tb to allow
> finding the interrupted block.

How about this alternative?

--8<---cut here---start->8---
accel/tcg: suppress IRQ check for special TBs

Generally when we set cpu->cflags_next_tb it is because we want to
carefully control the execution of the next TB. Currently there is a
race that causes cflags_next_tb to get ignored if an IRQ is processed
before we execute any actual instructions.

To avoid this we introduce a new compiler flag: CF_NOIRQ to suppress
this check in the generated code so we know we will definitely execute
the next block.

Signed-off-by: Alex Bennée 
Cc: Pavel Dovgalyuk 
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/245

3 files changed, 22 insertions(+), 3 deletions(-)
include/exec/exec-all.h   |  1 +
include/exec/gen-icount.h | 19 ---
accel/tcg/cpu-exec.c  |  5 +

modified   include/exec/exec-all.h
@@ -503,6 +503,7 @@ struct TranslationBlock {
 #define CF_USE_ICOUNT0x0002
 #define CF_INVALID   0x0004 /* TB is stale. Set with @jmp_lock held */
 #define CF_PARALLEL  0x0008 /* Generate code for a parallel context */
+#define CF_NOIRQ 0x0010 /* Generate an uninterruptible TB */
 #define CF_CLUSTER_MASK  0xff00 /* Top 8 bits are cluster ID */
 #define CF_CLUSTER_SHIFT 24
 
modified   include/exec/gen-icount.h
@@ -21,7 +21,6 @@ static inline void gen_tb_start(const TranslationBlock *tb)
 {
 TCGv_i32 count;
 
-tcg_ctx->exitreq_label = gen_new_label();
 if (tb_cflags(tb) & CF_USE_ICOUNT) {
 count = tcg_temp_local_new_i32();
 } else {
@@ -42,7 +41,19 @@ static inline void gen_tb_start(const TranslationBlock *tb)
 icount_start_insn = tcg_last_op();
 }
 
-tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, tcg_ctx->exitreq_label);
+/*
+ * Emit the check against icount_decr.u32 to see if we should exit
+ * unless we suppress the check with CF_NOIRQ. If we are using
+ * icount and have suppressed interruption the higher level code
+ * should have ensured we don't run more instructions than the
+ * budget.
+ */
+if (tb_cflags(tb) & CF_NOIRQ) {
+tcg_ctx->exitreq_label = NULL;
+} else {
+tcg_ctx->exitreq_label = gen_new_label();
+tcg_gen_brcondi_i32(TCG_COND_LT, count, 0, tcg_ctx->exitreq_label);
+}
 
 if (tb_cflags(tb) & CF_USE_ICOUNT) {
 tcg_gen_st16_i32(count, cpu_env,
@@ -74,7 +85,9 @@ static inline void gen_tb_end(const TranslationBlock *tb, int 
num_insns)
tcgv_i32_arg(tcg_constant_i32(num_insns)));
 }
 
-gen_set_label(tcg_ctx->exitreq_label);
+if (tcg_ctx->exitreq_label) {
+gen_set_label(tcg_ctx->exitreq_label);
+}
 tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);
 }
 
modified   accel/tcg/cpu-exec.c
@@ -954,11 +954,16 @@ int cpu_exec(CPUState *cpu)
  * after-access watchpoints.  Since this request should never
  * have CF_INVALID set, -1 is a convenient invalid value that
  * does not require tcg headers for cpu_common_reset.
+ *
+ * As we don't want this special TB being interrupted by
+ * some sort of asynchronous event we apply CF_NOIRQ to
+ * disable the usual event checking.
  */
 cflags = cpu->cflags_next_tb;
 if (cflags == -1) {
 cflags = curr_cflags(cpu);
 } else {
+cflags |= CF_NOIRQ;
 cpu->cflags_next_tb = -1;
 }
 
--8<---cut here---end--->8---

>
> Signed-off-by: Pavel Dovgalyuk 
> ---
>  accel/tcg/cpu-exec.c |   10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 2d14d02f6c..df12452b8f 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -846,6 +846,16 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, 
> TranslationBlock *tb,
>   * cpu_handle_interrupt.  cpu_handle_interrupt will also
>   * clear cpu->icount_decr.u16.high.
>   */
> +if (cpu->cflags_next_tb == -1
> +&& (!use_icount || !(tb->cflags & CF_USE_ICOUNT)
> +|| cpu_neg(cpu)->icount_decr.u16.low >= tb->icount)) {
> +/*
> + * icount is disabled or there are enough instructions
> + * in the budget, do not retranslate this block with
> + * different parameters.
> + */
> +cpu->cflags_next_tb = tb->cflags;
> +}
>

Re: [PATCH v2 6/7] target/riscv: cpu: Enable native debug feature on virt and sifive_u CPUs

2021-11-17 Thread Bin Meng

On Wed, Nov 17, 2021 at 8:58 AM Alistair Francis  wrote:
>
> On Sat, Oct 30, 2021 at 11:56 PM Bin Meng  wrote:
> >
> > Turn on native debug feature on virt and sifive_u CPUs.
>
> Is there a reason why it's only these 2 machines? Could this be
> enabled by default for all CPUs?
>

Yes, I think so. I only enabled these 2 as I did not check all
hardware specs like OpenTitan, etc.

Regards,
Bin

[RFC PATCH] tests/avocado: fix tcg_plugin mem access count test

When we cleaned up argument handling the test was missed.

Fixes: 5ae589faad ("tests/plugins/mem: introduce "track" arg and make args not 
positional")
Signed-off-by: Alex Bennée 
---
 tests/avocado/tcg_plugins.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/avocado/tcg_plugins.py b/tests/avocado/tcg_plugins.py
index 9ca1515c3b..642d2e49e3 100644
--- a/tests/avocado/tcg_plugins.py
+++ b/tests/avocado/tcg_plugins.py
@@ -131,7 +131,7 @@ def test_aarch64_virt_mem_icount(self):
  suffix=".log")
 
 self.run_vm(kernel_path, kernel_command_line,
-"tests/plugin/libmem.so,arg=both", plugin_log.name,
+"tests/plugin/libmem.so,inline=true,callback=true", 
plugin_log.name,
 console_pattern,
 args=('-icount', 'shift=1'))
 
-- 
2.30.2

Re: [PATCH v10 04/26] target/loongarch: Add fixed point arithmetic instruction translation


On 11/17/21 10:29 AM, gaosong wrote:

gen_rr_i ?


The code is not written completely,  like this:

gen_rr_i12:

@fmt_rr_i12    .. imm:s12 rj:5 rd:5 &rr_i
slti  001000  . . @fmt_rr_i12
sltui 001001  . . @fmt_rr_i12
...

gen_rr_ui12:

@fmt_rr_ui12    .. imm:12 rj:5 rd:5 &rr_i
andi  001101  . . @fmt_rr_ui12
ori   001110  . . @fmt_rr_ui12
xori  00  . . @fmt_rr_ui12
...

@fmt_rr_i12 and @fmt_rr_ui12 are two 'Formats',  but they use the same 
'Argument sets'(rr_i).


What I meant is that there would be a single gen_rr_i function handing the argument set 
rr_i; no need for two gen_rr_i* functions.



gen_rrr_sa2p1:

@fmt_rrr_sa2p1  ... .. rk:5 rj:5 rd:5   &fmt_rr_sa  
sa=%sa2p1
lsl_w     010 .. . . .@fmt_rrr_sa2p1
alsl_wu    011 .. . . .   @fmt_rrr_sa2p1
alsl_d    0010 110 .. . . .   @fmt_rrr_sa2p1
...

gen_rrr_sa2:
@fmt_rrr_sa2  ... sa:2 rk:5 rj:5 rd:5   &fmt_rr_sa
bytepick_w     100 .. . . .   @fmt_rrr_sa3
...

gen_rrr_sa3:
@fmt_rrr_sa3   .. sa:3 rk:5 rj:5 rd:5   &fmt_rr_sa
bytepick_d     11 ... . . .   @fmt_rrr_sa3
...


Likewise a single gen_rrr_sa function.


r~

Re: [PATCH v2 1/3] icount: preserve cflags when custom tb is about to execute


On 11/17/21 10:47 AM, Alex Bennée wrote:

-gen_set_label(tcg_ctx->exitreq_label);
+if (tcg_ctx->exitreq_label) {
+gen_set_label(tcg_ctx->exitreq_label);
+}
  tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);


The exit_tb is also not reachable, and should go in with the label.


  }
  
modified   accel/tcg/cpu-exec.c

@@ -954,11 +954,16 @@ int cpu_exec(CPUState *cpu)
   * after-access watchpoints.  Since this request should never
   * have CF_INVALID set, -1 is a convenient invalid value that
   * does not require tcg headers for cpu_common_reset.
+ *
+ * As we don't want this special TB being interrupted by
+ * some sort of asynchronous event we apply CF_NOIRQ to
+ * disable the usual event checking.
   */
  cflags = cpu->cflags_next_tb;
  if (cflags == -1) {
  cflags = curr_cflags(cpu);
  } else {
+cflags |= CF_NOIRQ;
  cpu->cflags_next_tb = -1;
  }


Still missing something to avoid cpu_handle_interrupt firing?


r~

Failing QEMU iotests




 Hi!

I think it has been working fine for me a couple of weeks ago,
but when I now run:

 make check SPEED=slow

I'm getting a couple of failing iotests... not sure whether
these are known issues already, so I thought I'd summarize them
here:

*** First one is 045 in raw mode: ***

 TEST   iotest-raw: 045 [fail]
QEMU  -- 
"/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-system-x86_64" 
-nodefaults -display none -accel qtest
QEMU_IMG  -- "/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-img"
QEMU_IO   -- "/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-io" 
--cache writeback --aio threads -f raw
QEMU_NBD  -- "/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-nbd"
IMGFMT-- raw
IMGPROTO  -- file
PLATFORM  -- Linux/x86_64 thuth 4.18.0-305.19.1.el8_4.x86_64
TEST_DIR  -- /home/thuth/tmp/qemu-build/tests/qemu-iotests/scratch
SOCK_DIR  -- /tmp/tmphlexdrlt
GDB_OPTIONS   --
VALGRIND_QEMU --
PRINT_QEMU_OUTPUT --

--- /home/thuth/devel/qemu/tests/qemu-iotests/045.out
+++ 045.out.bad
@@ -1,5 +1,77 @@
-...
+..EE.EE
+==
+ERROR: test_add_fd (__main__.TestSCMFd)
+--
+Traceback (most recent call last):
+  File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 148, in 
test_add_fd
+self._send_fd_by_SCM()
+  File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 144, in 
_send_fd_by_SCM
+ret = self.vm.send_fd_scm(file_path=image0)
+  File "/home/thuth/devel/qemu/python/qemu/machine/machine.py", line 229, in 
send_fd_scm
+self._qmp.send_fd_scm(fd)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/legacy.py", line 138, in 
send_fd_scm
+self._aqmp.send_fd_scm(fd)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/protocol.py", line 149, in 
_wrapper
+return func(proto, *args, **kwargs)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/qmp_client.py", line 644, in 
send_fd_scm
+sock = sock._sock  # pylint: disable=protected-access
+AttributeError: 'socket' object has no attribute '_sock'
+
+==
+ERROR: test_closefd (__main__.TestSCMFd)
+--
+Traceback (most recent call last):
+  File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 165, in 
test_closefd
+self._send_fd_by_SCM()
+  File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 144, in 
_send_fd_by_SCM
+ret = self.vm.send_fd_scm(file_path=image0)
+  File "/home/thuth/devel/qemu/python/qemu/machine/machine.py", line 229, in 
send_fd_scm
+self._qmp.send_fd_scm(fd)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/legacy.py", line 138, in 
send_fd_scm
+self._aqmp.send_fd_scm(fd)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/protocol.py", line 149, in 
_wrapper
+return func(proto, *args, **kwargs)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/qmp_client.py", line 644, in 
send_fd_scm
+sock = sock._sock  # pylint: disable=protected-access
+AttributeError: 'socket' object has no attribute '_sock'
+
+==
+ERROR: test_getfd (__main__.TestSCMFd)
+--
+Traceback (most recent call last):
+  File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 153, in test_getfd
+self._send_fd_by_SCM()
+  File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 144, in 
_send_fd_by_SCM
+ret = self.vm.send_fd_scm(file_path=image0)
+  File "/home/thuth/devel/qemu/python/qemu/machine/machine.py", line 229, in 
send_fd_scm
+self._qmp.send_fd_scm(fd)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/legacy.py", line 138, in 
send_fd_scm
+self._aqmp.send_fd_scm(fd)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/protocol.py", line 149, in 
_wrapper
+return func(proto, *args, **kwargs)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/qmp_client.py", line 644, in 
send_fd_scm
+sock = sock._sock  # pylint: disable=protected-access
+AttributeError: 'socket' object has no attribute '_sock'
+
+==
+ERROR: test_getfd_invalid_fdname (__main__.TestSCMFd)
+--
+Traceback (most recent call last):
+  File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 158, in 
test_getfd_invalid_fdname
+self._send_fd_by_SCM()
+  File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 144, in 
_send_fd_by_SCM
+ret = self.vm.send_fd_scm(file_path=image0)
+  File "/home/thuth/devel/qemu/python/qemu/machine/machine.py", line 229, in 
send_fd_scm
+self._qmp.send_fd_scm(fd)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/legacy.py", line 138, in 
send_fd_scm
+self._aqmp.send_fd_scm(fd)
+  File "/home/

Re: [PATCH v3 3/3] docs: rSTify the "SubmitAPatch" wiki

2021-11-17 Thread Kashyap Chamarthy

On Wed, Nov 17, 2021 at 10:08:52AM +0100, Thomas Huth wrote:
> On 10/11/2021 15.49, Kashyap Chamarthy wrote:

[...]

> >  writing-qmp-commands
> >  trivial-patches
> >  submitting-a-pull-request
> > +   submitting-a-patch
> 
> I'd suggest to insert this before the pull-request entry, in case anybody
> reads the manual sequentially, it might be better to learn about the patch
> submission process first before reading about pull requests.

I did notice it when looking at the rendered output, and still missed to
fix it; bad me.

> (I can fix this up when picking up the patch, no need to resend)

Thank you.

[...]

> > +Split up longer patches into a patch series of logical code changes.
> > +Each change should compile and execute successfully. For instance, don't
> > +add a file to the makefile in patch one and then add the file itself in
> > +patch two. (This rule is here so that people can later use tools like
> > +```git bisect`` `__ without hitting
> 
> That hyperlink showed up in the rendered output. I'll fix it up by removing
> the "``" quotes.

Oops, good catch.

[...]

> > +QEMU follows the usual standard for git commit messages: the first line
> > +(which becomes the email subject line) is "subsystem: single line
> > +summary of change". Whether the "single line summary of change" starts
> > +with a capital is a matter of taste, but we prefer that the summary does
> > +not end in ".".
> 
> That ".". looks a little bit weird in the output ... maybe we should replace
> it with "does not end with a dot." ?

Re-looking the output, yes it does look odd.  And yes, your amendment
is good.

[...]

> > +The body of the commit message is a good place to document why your
> > +change is important. Don't include comments like "This is a suggestion
> > +for fixing this bug" (they can go below the "---" line in the email so
> 
> That --- gets translated into a — character ... I'll replace the "---" with
> ``---`` to fix it.

Ah, when I locally ran `rst2html5 submitting-a-patch.rst
submitting-a-patch.html` it retained the "---", but when I built QEMU
(`configure --target-list=x86_64-softmmu --enable-docs`), Sphinx does
turn it into an em-dash (—), and missed to notice it.

Thanks for the careful review and submitting the PR.  I'm assuming I
don't need to respin a v4.
 

-- 
/kashyap

Re: [PATCH v2 1/3] icount: preserve cflags when custom tb is about to execute



Richard Henderson  writes:

> On 11/17/21 10:47 AM, Alex Bennée wrote:
>> -gen_set_label(tcg_ctx->exitreq_label);
>> +if (tcg_ctx->exitreq_label) {
>> +gen_set_label(tcg_ctx->exitreq_label);
>> +}
>>   tcg_gen_exit_tb(tb, TB_EXIT_REQUESTED);
>
> The exit_tb is also not reachable, and should go in with the label.

ok

>
>>   }
>>   modified   accel/tcg/cpu-exec.c
>> @@ -954,11 +954,16 @@ int cpu_exec(CPUState *cpu)
>>* after-access watchpoints.  Since this request should never
>>* have CF_INVALID set, -1 is a convenient invalid value that
>>* does not require tcg headers for cpu_common_reset.
>> + *
>> + * As we don't want this special TB being interrupted by
>> + * some sort of asynchronous event we apply CF_NOIRQ to
>> + * disable the usual event checking.
>>*/
>>   cflags = cpu->cflags_next_tb;
>>   if (cflags == -1) {
>>   cflags = curr_cflags(cpu);
>>   } else {
>> +cflags |= CF_NOIRQ;
>>   cpu->cflags_next_tb = -1;
>>   }
>
> Still missing something to avoid cpu_handle_interrupt firing?

Something as simple as:

--8<---cut here---start->8---
modified   accel/tcg/cpu-exec.c
@@ -721,6 +721,15 @@ static inline bool need_replay_interrupt(int 
interrupt_request)
 static inline bool cpu_handle_interrupt(CPUState *cpu,
 TranslationBlock **last_tb)
 {
+/*
+ * If we have special cflags lets not get distracted with IRQs. We
+ * shall exit the loop as soon as the next TB completes what it
+ * needs to do.
+ */
+if (cpu->cflags_next_tb != -1) {
+return false;
+}
+
 /* Clear the interrupt flag now since we're processing
  * cpu->interrupt_request and cpu->exit_request.
--8<---cut here---end--->8---

?

>
>
> r~


-- 
Alex Bennée

Re: [PATCH 1/2] block:hdev: support BLKSECDISCARD

2021-11-17 Thread Stefan Hajnoczi

On Tue, Nov 16, 2021 at 09:53:39PM -0800, Christoph Hellwig wrote:
> On Tue, Nov 16, 2021 at 10:58:30AM +, Stefan Hajnoczi wrote:
> > Question for Jens and Christoph:
> > 
> > Is there a way for userspace to detect whether a Linux block device
> > supports SECDISCARD?
> 
> I don't know of one.
> 
> > If not, then maybe a new sysfs attribute can be added:
> 
> This looks correct, but if we start looking into SECDISCARD seriously
> I'd like to split the max_sectors settings for it from discard as that
> is currently a bit of a mess.  If we then expose the secure erase max
> sectors in sysfs that would also be a good indicator.

That is probably better because userspace would the queue limits
information too. Thanks!

> What is the use case for exposing secure erase in qemu?  The whole
> concept for a LBA based secure erase is generally not a very smart
> idea for flash based media..

Yadong, please see Christoph's question above.

Stefan


signature.asc
Description: PGP signature

Re: Fwd: New Defects reported by Coverity Scan for QEMU


On 11/16/21 21:21, Luis Fernando Fujita Pires wrote:

From: Matheus K. Ferst 

Hi Cédric,

The only change was the helper name that is now uppercase, so nothing new
here. The underlying cause is that dfp_finalize_decimal64 only sets
dfp->vt.VsrD(1) and set_dfp64 receives a pointer to the complete struct.

But since set_dfp64 also only access VsrD(1), it shouldn't be a real
problem AFAICT. The same applies to CID 1465776~1465786 and
1465788~1465790.


Right. Coverity is probably reporting these as new just because the helper 
macros were re-written as part of the move to decodetree.
I believe these should be marked as false positives.

We *could* also wrap set_dfp{64,128} in new macros that would then reference 
only the appropriate parts of dfp, but, in this case, I don't think it's worth 
the trouble.


Thanks for the help on this,

C.

Re: [PATCH v2] dump-guest-memory: Use BQL to protect dump finalize process

2021-11-17 Thread Laszlo Ersek

On 11/16/21 14:54, Peter Xu wrote:
> When finalizing the dump-guest-memory with detached mode, we'll first set dump
> status to either FAIL or COMPLETE before doing the cleanup, however right 
> after
> the dump status change it's possible that another dump-guest-memory qmp 
> command
> is sent so both the main thread and dump thread (which is during cleanup) 
> could
> be accessing dump state in parallel.  That's racy.
> 
> Fix it by protecting the finalizing phase of dump-guest-memory using BQL as
> well, as qmp_dump_guest_memory() (which is the QEMU main thread) requires BQL.
> To do that, we expand the BQL from dump_cleanup() into dump_process(), so we
> will take the BQL longer than before.  The critical section must cover the
> status switch from ACTIVE->{FAIL|COMPLETE} so as to make sure there's no race
> any more.
> 
> We can also just introduce a specific mutex to serialize the dump process, but
> BQL should be enough for now so far, not to mention vm_start() in dump_cleanup
> will need BQL anyway, so maybe easier to just use the same mutex.
> 
> Reported-by: Laszlo Ersek 
> Reviewed-by: Marc-André Lureau 
> Signed-off-by: Peter Xu 
> ---
> v2:
> - Fix parallel spelling [Marc-Andre]
> - Add r-b for Marc-Andre
> - Mention that qmp_dump_guest_memory() is with BQL held [Laszlo]
> ---
>  dump/dump.c | 24 ++--
>  1 file changed, 18 insertions(+), 6 deletions(-)

Reviewed-by: Laszlo Ersek 

Thank you!
Laszlo

Re: [PATCH v2] dump-guest-memory: Use BQL to protect dump finalize process

2021-11-17 Thread Peter Xu

On Wed, Nov 17, 2021 at 11:43:42AM +0100, Laszlo Ersek wrote:
> On 11/16/21 14:54, Peter Xu wrote:
> > When finalizing the dump-guest-memory with detached mode, we'll first set 
> > dump
> > status to either FAIL or COMPLETE before doing the cleanup, however right 
> > after
> > the dump status change it's possible that another dump-guest-memory qmp 
> > command
> > is sent so both the main thread and dump thread (which is during cleanup) 
> > could
> > be accessing dump state in parallel.  That's racy.
> > 
> > Fix it by protecting the finalizing phase of dump-guest-memory using BQL as
> > well, as qmp_dump_guest_memory() (which is the QEMU main thread) requires 
> > BQL.
> > To do that, we expand the BQL from dump_cleanup() into dump_process(), so we
> > will take the BQL longer than before.  The critical section must cover the
> > status switch from ACTIVE->{FAIL|COMPLETE} so as to make sure there's no 
> > race
> > any more.
> > 
> > We can also just introduce a specific mutex to serialize the dump process, 
> > but
> > BQL should be enough for now so far, not to mention vm_start() in 
> > dump_cleanup
> > will need BQL anyway, so maybe easier to just use the same mutex.
> > 
> > Reported-by: Laszlo Ersek 
> > Reviewed-by: Marc-André Lureau 
> > Signed-off-by: Peter Xu 
> > ---
> > v2:
> > - Fix parallel spelling [Marc-Andre]
> > - Add r-b for Marc-Andre
> > - Mention that qmp_dump_guest_memory() is with BQL held [Laszlo]
> > ---
> >  dump/dump.c | 24 ++--
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> Reviewed-by: Laszlo Ersek 

Thanks (again) for the review (and spot/analyze the issue)!

-- 
Peter Xu

Re: [PATCH-for-7.0 v4 08/11] tests/unit/test-smp-parse: Add 'smp-without-dies-valid' machine type

2021-11-17 Thread wangyanan (Y)




On 2021/11/17 16:08, Philippe Mathieu-Daudé wrote:

Hi Yanan,

On 11/17/21 08:37, wangyanan (Y) wrote:

On 2021/11/15 22:58, Philippe Mathieu-Daudé wrote:

Keep the common TYPE_MACHINE class initialization in
machine_base_class_init(), make it abstract, and move
the non-common code to a new class: "smp-without-dies-valid".

Signed-off-by: Philippe Mathieu-Daudé 
---
   tests/unit/test-smp-parse.c | 19 +++
   1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index dfe7f1313b0..90a249fe8c8 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -478,13 +478,19 @@ static void machine_base_class_init(ObjectClass
*oc, void *data)
   {
   MachineClass *mc = MACHINE_CLASS(oc);
   +    mc->smp_props.prefer_sockets = true;
+
+    mc->name = g_strdup(SMP_MACHINE_NAME);
+}
+
+static void machine_without_dies_valid_class_init(ObjectClass *oc,
void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
   mc->min_cpus = MIN_CPUS;
   mc->max_cpus = MAX_CPUS;
   -    mc->smp_props.prefer_sockets = true;
   mc->smp_props.dies_supported = false;
-
-    mc->name = g_strdup(SMP_MACHINE_NAME);
   }
     static void machine_without_dies_invalid_class_init(ObjectClass
*oc, void *data)
@@ -606,9 +612,14 @@ static const TypeInfo smp_machine_types[] = {
   {
   .name   = TYPE_MACHINE,
   .parent = TYPE_OBJECT,
+    .abstract   = true,
   .class_init = machine_base_class_init,
   .class_size = sizeof(MachineClass),
   .instance_size  = sizeof(MachineState),
+    }, {
+    .name   = MACHINE_TYPE_NAME("smp-without-dies-valid"),
+    .parent = TYPE_MACHINE,
+    .class_init = machine_without_dies_valid_class_init,
   }, {
   .name   =
MACHINE_TYPE_NAME("smp-without-dies-invalid"),
   .parent = TYPE_MACHINE,
@@ -629,7 +640,7 @@ int main(int argc, char *argv[])
   g_test_init(&argc, &argv, NULL);
     g_test_add_data_func("/test-smp-parse/generic/valid",
- TYPE_MACHINE,
+ MACHINE_TYPE_NAME("smp-without-dies-valid"),
    test_generic_valid);
   g_test_add_data_func("/test-smp-parse/generic/invalid",
    MACHINE_TYPE_NAME("smp-without-dies-invalid"),

After patch #7 and #8, we will have sub-tests as below. It looks nice,
but it will
probably be better to tweak "smp-without-dies-valid" to
"smp-generic-valid",
and "smp-without-dies-invalid" to "smp-generic-invalid", which will be more
consistent with the corresponding sub-test name.

It's Ok now as we only have dies currently besides generic
sockets/cores/threads,
but "smp-without-dies-xxx" will become a bit confusing when new topology
members are introduced and tested here.

OK I modified it and will respin once v6.2 is released.

Also test_with_dies() should be split in 2 tests: valid/invalid;
then smp_parse_test() should split tests further regarding the
socket preference. But I'll let that to you,

Sure, I can do this in an appropriate time. But IMHO, I don't see a
strong necessity to split test_with_dies for now, as the valid/invalid
scenarios share the same class properties. We can probably keep it
as is until we have to split it.

As for socket preference, I can foresee code duplication if we split
all the tests into two parts just regarding the socket preference.
Isn't it clear enough to use current smp_parse_test() for each
test data sample? Do we have other concern here?

I wanted to 1/ fix
our Windows CI and 2/ show you how to structure the tests.

Understood. The test architecture is indeed improved a lot.
Thanks for your education. 😉

Thanks,
Yanan

Regards,

Phil.

.

Re: Failing QEMU iotests


On 17.11.21 11:07, Thomas Huth wrote:


 Hi!

I think it has been working fine for me a couple of weeks ago,
but when I now run:

 make check SPEED=slow

I'm getting a couple of failing iotests... not sure whether
these are known issues already, so I thought I'd summarize them
here:


Thanks!


*** First one is 045 in raw mode: ***

 TEST   iotest-raw: 045 [fail]
QEMU  -- 
"/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-system-x86_64" 
-nodefaults -display none -accel qtest
QEMU_IMG  -- 
"/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-img"
QEMU_IO   -- 
"/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-io" --cache 
writeback --aio threads -f raw
QEMU_NBD  -- 
"/home/thuth/tmp/qemu-build/tests/qemu-iotests/../../qemu-nbd"

IMGFMT    -- raw
IMGPROTO  -- file
PLATFORM  -- Linux/x86_64 thuth 4.18.0-305.19.1.el8_4.x86_64
TEST_DIR  -- /home/thuth/tmp/qemu-build/tests/qemu-iotests/scratch
SOCK_DIR  -- /tmp/tmphlexdrlt
GDB_OPTIONS   --
VALGRIND_QEMU --
PRINT_QEMU_OUTPUT --

--- /home/thuth/devel/qemu/tests/qemu-iotests/045.out
+++ 045.out.bad
@@ -1,5 +1,77 @@
-...
+..EE.EE
+==
+ERROR: test_add_fd (__main__.TestSCMFd)
+--
+Traceback (most recent call last):
+  File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 148, in 
test_add_fd

+    self._send_fd_by_SCM()
+  File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 144, in 
_send_fd_by_SCM

+    ret = self.vm.send_fd_scm(file_path=image0)
+  File "/home/thuth/devel/qemu/python/qemu/machine/machine.py", line 
229, in send_fd_scm

+    self._qmp.send_fd_scm(fd)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/legacy.py", line 138, 
in send_fd_scm

+    self._aqmp.send_fd_scm(fd)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/protocol.py", line 
149, in _wrapper

+    return func(proto, *args, **kwargs)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/qmp_client.py", line 
644, in send_fd_scm

+    sock = sock._sock  # pylint: disable=protected-access
+AttributeError: 'socket' object has no attribute '_sock'
+
+==
+ERROR: test_closefd (__main__.TestSCMFd)
+--
+Traceback (most recent call last):
+  File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 165, in 
test_closefd

+    self._send_fd_by_SCM()
+  File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 144, in 
_send_fd_by_SCM

+    ret = self.vm.send_fd_scm(file_path=image0)
+  File "/home/thuth/devel/qemu/python/qemu/machine/machine.py", line 
229, in send_fd_scm

+    self._qmp.send_fd_scm(fd)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/legacy.py", line 138, 
in send_fd_scm

+    self._aqmp.send_fd_scm(fd)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/protocol.py", line 
149, in _wrapper

+    return func(proto, *args, **kwargs)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/qmp_client.py", line 
644, in send_fd_scm

+    sock = sock._sock  # pylint: disable=protected-access
+AttributeError: 'socket' object has no attribute '_sock'
+
+==
+ERROR: test_getfd (__main__.TestSCMFd)
+--
+Traceback (most recent call last):
+  File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 153, in 
test_getfd

+    self._send_fd_by_SCM()
+  File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 144, in 
_send_fd_by_SCM

+    ret = self.vm.send_fd_scm(file_path=image0)
+  File "/home/thuth/devel/qemu/python/qemu/machine/machine.py", line 
229, in send_fd_scm

+    self._qmp.send_fd_scm(fd)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/legacy.py", line 138, 
in send_fd_scm

+    self._aqmp.send_fd_scm(fd)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/protocol.py", line 
149, in _wrapper

+    return func(proto, *args, **kwargs)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/qmp_client.py", line 
644, in send_fd_scm

+    sock = sock._sock  # pylint: disable=protected-access
+AttributeError: 'socket' object has no attribute '_sock'
+
+==
+ERROR: test_getfd_invalid_fdname (__main__.TestSCMFd)
+--
+Traceback (most recent call last):
+  File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 158, in 
test_getfd_invalid_fdname

+    self._send_fd_by_SCM()
+  File "/home/thuth/devel/qemu/tests/qemu-iotests/045", line 144, in 
_send_fd_by_SCM

+    ret = self.vm.send_fd_scm(file_path=image0)
+  File "/home/thuth/devel/qemu/python/qemu/machine/machine.py", line 
229, in send_fd_scm

+    self._qmp.send_fd_scm(fd)
+  File "/home/thuth/devel/qemu/python/qemu/aqmp/legacy.py"

Re: [PATCH] pmu: fix pmu vmstate subsection list


diff --git a/hw/misc/macio/pmu.c b/hw/misc/macio/pmu.c
index 4ad4f50e08c3..eb39c64694aa 100644
--- a/hw/misc/macio/pmu.c
+++ b/hw/misc/macio/pmu.c
@@ -718,6 +718,7 @@ static const VMStateDescription vmstate_pmu = {
  },
  .subsections = (const VMStateDescription * []) {
  &vmstate_pmu_adb,
+NULL
  }
  };
  


This fix is so obvious that I guess you could carry it through the
trivial tree IMHO.


I don't have anything queued for ppc yet but anyhow I can send a PR
at the end of the week if trivial doesn't.

Thanks,

C.

[RFC PATCH] plugins/meson.build: fix linker issue with weird paths

Signed-off-by: Alex Bennée 
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/712
---
 plugins/meson.build | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/plugins/meson.build b/plugins/meson.build
index aeb386ebae..b3de57853b 100644
--- a/plugins/meson.build
+++ b/plugins/meson.build
@@ -2,9 +2,9 @@ plugin_ldflags = []
 # Modules need more symbols than just those in plugins/qemu-plugins.symbols
 if not enable_modules
   if 'CONFIG_HAS_LD_DYNAMIC_LIST' in config_host
-plugin_ldflags = ['-Wl,--dynamic-list=' + (meson.project_build_root() / 
'qemu-plugins-ld.symbols')]
+plugin_ldflags = ['-Wl,--dynamic-list=qemu-plugins-ld.symbols']
   elif 'CONFIG_HAS_LD_EXPORTED_SYMBOLS_LIST' in config_host
-plugin_ldflags = ['-Wl,-exported_symbols_list,' + 
(meson.project_build_root() / 'qemu-plugins-ld64.symbols')]
+plugin_ldflags = ['-Wl,-exported_symbols_list,qemu-plugins-ld64.symbols']
   endif
 endif
 
-- 
2.30.2

Re: [PATCH v2 08/15] hw/nvme: Implement the Function Level Reset

2021-11-17 Thread Łukasz Gieryk

On Tue, Nov 16, 2021 at 01:28:19PM -0800, Keith Busch wrote:
> On Tue, Nov 16, 2021 at 04:34:39PM +0100, Łukasz Gieryk wrote:
> >  if (!pci_is_vf(&n->parent_obj) && n->params.sriov_max_vfs) {
> > -pcie_sriov_pf_disable_vfs(&n->parent_obj);
> > +if (rst != NVME_RESET_CONTROLLER) {
> > +pcie_sriov_pf_disable_vfs(&n->parent_obj);
> 
> Shouldn't this be 'if (rst == NVME_RESET_FUNCTION)'?

The NVMe Spec lists five possible reset types (triggers). According
to my understanding, only the Controller Reset doesn’t affect the VFs'
state, hence the '!='.

Re: [PATCH v2 1/3] icount: preserve cflags when custom tb is about to execute


On 11/17/21 11:29 AM, Alex Bennée wrote:

Still missing something to avoid cpu_handle_interrupt firing?


Something as simple as:

--8<---cut here---start->8---
modified   accel/tcg/cpu-exec.c
@@ -721,6 +721,15 @@ static inline bool need_replay_interrupt(int 
interrupt_request)
  static inline bool cpu_handle_interrupt(CPUState *cpu,
  TranslationBlock **last_tb)
  {
+/*
+ * If we have special cflags lets not get distracted with IRQs. We
+ * shall exit the loop as soon as the next TB completes what it
+ * needs to do.
+ */
+if (cpu->cflags_next_tb != -1) {
+return false;
+}
+
  /* Clear the interrupt flag now since we're processing
   * cpu->interrupt_request and cpu->exit_request.
--8<---cut here---end--->8---

?


Yeah, that should do it.


r~

Re: [PATCH v4 20/25] block_int-common.h: assertion in the callers of BlockDriver function pointers

2021-11-17 Thread Emanuele Giuseppe Esposito





On 15/11/2021 13:48, Hanna Reitz wrote:

On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Stefan Hajnoczi 
---
  block.c | 17 +
  1 file changed, 17 insertions(+)

diff --git a/block.c b/block.c
index 94bff5c757..40c4729b8d 100644
--- a/block.c
+++ b/block.c


[...]

@@ -2148,6 +2152,7 @@ static void bdrv_child_perm(BlockDriverState 
*bs, BlockDriverState *child_bs,

  uint64_t *nperm, uint64_t *nshared)
  {
  assert(bs->drv && bs->drv->bdrv_child_perm);
+    assert(qemu_in_main_thread());
  bs->drv->bdrv_child_perm(bs, c, role, reopen_queue,
   parent_perm, parent_shared,
   nperm, nshared);


(Should’ve noticed earlier, but only did now...)

First, this function is indirectly called by bdrv_refresh_perms(). I 
understand that all perm-related functions are classified as GS.


However, bdrv_co_invalidate_cache() invokes bdrv_refresh_perms. Being 
declared in block/coroutine.h, it’s an I/O function, so it mustn’t call 
such a GS function. BlockDriver.bdrv_co_invalidate_cache(), 
bdrv_invalidate_cache(), and blk_invalidate_cache() are also classified 
as I/O functions. Perhaps all of these functions should be classified as 
GS functions?  I believe their callers and their purpose would allow for 
this.


I think that the *_invalidate_cache functions are I/O.
First of all, test-block-iothread.c calls bdrv_invalidate_cache in 
test_sync_op_invalidate_cache, which is purposefully called in an 
iothread. So that hints that we want it as I/O.
(Small mistake I just noticed: blk_invalidate_cache has the BQL 
assertion even though it is rightly put in block-backend-io.h




Second, it’s called by bdrv_child_refresh_perms(), which is called by 
block_crypto_amend_options_generic_luks().  This function is called by 
block_crypto_co_amend_luks(), which is a BlockDriver.bdrv_co_amend 
implementation, which is classified as an I/O function.


Honestly, I don’t know how to fix that mess.  The best would be if we 
could make the perm functions thread-safe and classify them as I/O, but 
it seems to me like that’s impossible (I sure hope I’m wrong).  On the 
other hand, .bdrv_co_amend very much strikes me like a GS function, but 
it isn’t.  I’m afraid it must work on nodes that are not in the main 
context, and it launches a job, so AFAIU we absolutely cannot run it 
under the BQL.


It almost seems to me like we’d need a thread-safe variant of the perm 
functions that’s allowed to fail when it cannot guarantee thread safety 
or something.  Or perhaps I’m wrong and the perm functions can actually 
be classified as thread-safe and I/O, that’d be great…


I think that since we are currently only splitting and not taking care 
of the actual I/O thread safety, we can move the _perm functions in I/O, 
and add a nice TODO to double check their thread safety.


I mean, if they are not thread-safe after the split it means they are 
not thread safe also right now.


Emanuele

Re: [PULL 0/2] riscv-to-apply queue


On 11/17/21 10:20 AM, Alistair Francis wrote:

From: Alistair Francis 

The following changes since commit 8d5fcb1990bc64b62c0bc12121fe510940be5664:

   Merge tag 'python-pull-request' of https://gitlab.com/jsnow/qemu into 
staging (2021-11-17 07:41:08 +0100)

are available in the Git repository at:

   g...@github.com:alistair23/qemu.git tags/pull-riscv-to-apply-2027-1

for you to fetch changes up to c94c239496256f1f1cb589825d052c2f3e26ebf6:

   meson.build: Merge riscv32 and riscv64 cpu family (2021-11-17 19:18:22 +1000)


Sixth RISC-V PR for QEMU 6.2

  - Fix build for riscv hosts
  - Soft code alphabetically


Bin Meng (1):
   target/riscv: machine: Sort the .subsections

Richard Henderson (1):
   meson.build: Merge riscv32 and riscv64 cpu family

  meson.build|  6 
  target/riscv/machine.c | 92 +-
  2 files changed, 52 insertions(+), 46 deletions(-)



Applied, thanks.


r~

Re: [PULL SUBSYSTEM qemu-pseries] pseries: Update SLOF firmware image

On 11/17/21 09:49, Cédric Le Goater wrote:
> On 11/17/21 09:39, Richard Henderson wrote:
>> On 11/16/21 11:48 PM, Alexey Kardashevskiy wrote:
>>> Yup. I am doing SLOF updates this way for ages after diifs became
>>> quite huge to make mailman barfing on the size, and the "subsystem"
>>> in the subj was the way to reduce the noise Peter had to respond to :)
>>>
>>> btw should I be signing those? I am not now.
>>
>> You could if you and Cedric want to do so (is it really Alexey sending
>> the sub pr and not an impostor with access to the same github
>> account?), but it will not leave a permanent record in the mainline
>> history, because the merge object with the signature will be removed
>> by any rebase.
> 
> 
> Yes. I just noticed that the merge commit is lost :
> 
> 2021-11-09 15:50 +0100 Christophe Lombard o
> [ppc-7.0] {origin/ppc-7.0} pci-host: Allow extended config space access
> for PowerNV PHB4 model
> 2021-11-16 17:39 +0100 Cédric Le Goater   M─┐ Merge
> tag 'qemu-slof-2022' of github.com:aik/qemu into ppc-7.0
> 2021-11-13 14:47 +1100 Alexey Kardashevskiy   │ o
> pseries: Update SLOF firmware image
> 2021-11-12 12:28 +0100 Richard Henderson  M─│─┐
> Merge tag 'pull-ppc-2022' of https://github.com/legoater/qemu into
> staging
> 2021-11-10 17:25 -0300 Daniel Henrique Barboza    │ o─┘
> {origin/ppc-for-upstream} {origin/ppc-next} 
> ppc/mmu_helper.c: do not truncate 'ea
> ...
> 
> After rebase :
> 
> 2021-11-09 15:50 +0100 Christophe Lombard o
> [ppc-7.0] pci-host: Allow extended config space access for PowerNV PHB4
> model
> 2021-11-13 14:47 +1100 Alexey Kardashevskiy   o pseries:
> Update SLOF firmware image
> 2021-11-16 21:07 +0100 Richard Henderson  o Update
> version for v6.2.0-rc1 release
> 2021-11-16 18:55 +0100 Richard Henderson  M─┐ Merge
> tag 'pull-nbd-2021-11-16' of https://repo.or.cz/qemu/ericb into staging
> ...

No need to rebase if Alexey sends his pullreq once v6.2 is out.

> Alexey's commit appears in the list with only his SoB and we loose :
> 
>     Merge tag 'qemu-slof-2022' of github.com:aik/qemu into ppc-7.0
> 
>     * tag 'qemu-slof-2022' of github.com:aik/qemu:
>   pseries: Update SLOF firmware image
> 
>     Signed-off-by: Cédric Le Goater 
> 
> which I find interesting to identify where the SLOF blob is coming from.
> 
> 
> Thanks,
> 
> C.
>

[PATCH-for-6.2 v2 1/2] hw/nvme/ctrl: Fix buffer overrun in nvme_changed_nslist (CVE-2021-3947)

Both 'buf_len' and 'off' arguments are under guest control.
Since nvme_c2h() doesn't check out of boundary access, the
caller must check for eventual buffer overrun on 'trans_len'.

Cc: qemu-sta...@nongnu.org
Reported-by: Qiuhao Li 
Fixes: f432fdfa121 ("support changed namespace asynchronous event")
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/nvme/ctrl.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 6a571d18cfa..93a24464647 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4168,8 +4168,11 @@ static uint16_t nvme_changed_nslist(NvmeCtrl *n, uint8_t 
rae, uint32_t buf_len,
 int i = 0;
 uint32_t nsid;
 
-memset(nslist, 0x0, sizeof(nslist));
 trans_len = MIN(sizeof(nslist) - off, buf_len);
+if (trans_len >= sizeof(nslist)) {
+return NVME_INVALID_FIELD | NVME_DNR;
+}
+memset(nslist, 0x0, sizeof(nslist));
 
 while ((nsid = find_first_bit(n->changed_nsids, NVME_CHANGED_NSID_SIZE)) !=
 NVME_CHANGED_NSID_SIZE) {
-- 
2.31.1

[PATCH-for-6.2 v2 0/2] hw/nvme/ctrl: Fix buffer overrun (CVE-2021-3947)

Since v1:
- Do not add more buffer overflows in modify nvme_smart_info(),
  nvme_fw_log_info() and nvme_cmd_effects() (Klaus)
- Split nvme_error_info() change in another patch

Philippe Mathieu-Daudé (2):
  hw/nvme/ctrl: Fix buffer overrun in nvme_changed_nslist
(CVE-2021-3947)
  hw/nvme/ctrl: Prevent buffer overrun in nvme_error_info()

 hw/nvme/ctrl.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH-for-6.2 v2 2/2] hw/nvme/ctrl: Prevent buffer overrun in nvme_error_info()

Both 'buf_len' and 'off' arguments are under guest control.
Since nvme_c2h() doesn't check out of boundary access, the
caller must check for eventual buffer overrun on 'trans_len'.

Cc: qemu-sta...@nongnu.org
Fixes: 94a7897c41d ("add support for the get log page command")
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/nvme/ctrl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 93a24464647..7414f3b4dd1 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4146,7 +4146,8 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t rae, 
uint32_t buf_len,
 uint32_t trans_len;
 NvmeErrorLog errlog;
 
-if (off >= sizeof(errlog)) {
+trans_len = MIN(sizeof(errlog) - off, buf_len);
+if (trans_len >= sizeof(errlog)) {
 return NVME_INVALID_FIELD | NVME_DNR;
 }
 
@@ -4155,7 +4156,6 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t rae, 
uint32_t buf_len,
 }
 
 memset(&errlog, 0x0, sizeof(errlog));
-trans_len = MIN(sizeof(errlog) - off, buf_len);
 
 return nvme_c2h(n, (uint8_t *)&errlog, trans_len, req);
 }
-- 
2.31.1

Re: Failing QEMU iotests


On 17/11/2021 11.59, Hanna Reitz wrote:

On 17.11.21 11:07, Thomas Huth wrote:


 Hi!

I think it has been working fine for me a couple of weeks ago,
but when I now run:

 make check SPEED=slow

I'm getting a couple of failing iotests... not sure whether
these are known issues already, so I thought I'd summarize them
here:

...

--- /home/thuth/devel/qemu/tests/qemu-iotests/206.out
+++ 206.out.bad
@@ -99,55 +99,19 @@

 {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": 
{"driver": "qcow2", "encrypt": {"cipher-alg": "twofish-128", 
"cipher-mode": "ctr", "format": "luks", "hash-alg": "sha1", "iter-time": 
10, "ivgen-alg": "plain64", "ivgen-hash-alg": "md5", "key-secret": 
"keysec0"}, "file": {"driver": "file", "filename": 
"TEST_DIR/PID-t.qcow2"}, "size": 33554432}}}

 {"return": {}}
+Job failed: Unsupported cipher algorithm twofish-128 with ctr mode
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}

 image: TEST_IMG
 file format: IMGFMT
 virtual size: 32 MiB (33554432 bytes)
-encrypted: yes
 cluster_size: 65536
 Format specific information:
 compat: 1.1
 compression type: zlib
 lazy refcounts: false
 refcount bits: 16
-    encrypt:
-    ivgen alg: plain64
-    hash alg: sha1
-    cipher alg: twofish-128
-    uuid: ----
-    format: luks
-    cipher mode: ctr
-    slots:
-    [0]:
-    active: true
-    iters: XXX
-    key offset: 4096
-    stripes: 4000
-    [1]:
-    active: false
-    key offset: 69632
-    [2]:
-    active: false
-    key offset: 135168
-    [3]:
-    active: false
-    key offset: 200704
-    [4]:
-    active: false
-    key offset: 266240
-    [5]:
-    active: false
-    key offset: 331776
-    [6]:
-    active: false
-    key offset: 397312
-    [7]:
-    active: false
-    key offset: 462848
-    payload offset: 528384
-    master key iters: XXX
 corrupt: false
 extended l2: false


I doubt this worked a couple of weeks ago, but it’s definitely one that we 
should just get around to fixing. :/


Hm, maybe I've did the successful run on a different system last time ... I 
even slightly remember now having seen this before in the past on my current 
system, so yes, likely not something new.




+++ 297.out.bad
@@ -1,2 +1,21 @@
 === pylint ===
+* Module image-fleecing
+tests/image-fleecing:34:24: C0326: Exactly one space required after comma
+patterns = [('0x5d', '0', '64k'),
+    ^ (bad-whitespace)
+tests/image-fleecing:35:25: C0326: Exactly one space required after comma
+    ('0xd5', '1M',    '64k'),
+ ^ (bad-whitespace)
+tests/image-fleecing:36:26: C0326: Exactly one space required after comma
+    ('0xdc', '32M',   '64k'),
+  ^ (bad-whitespace)
+tests/image-fleecing:39:25: C0326: Exactly one space required after comma
+overwrite = [('0xab', '0', '64k'), # Full overwrite
+ ^ (bad-whitespace)
+tests/image-fleecing:48:32: C0326: Exactly one space required after comma
+remainder = [('0xd5', '0x108000',  '32k'), # Right-end of partial-left [1]
+    ^ (bad-whitespace)
+tests/image-fleecing:49:27: C0326: Exactly one space required after comma
+ ('0xdc', '32M',   '32k'), # Left-end of partial-right [2]
+   ^ (bad-whitespace)


This could be because your pylint is too old.  At least for the python/ 
tests we at least require 2.8.0 
(https://lists.nongnu.org/archive/html/qemu-block/2021-10/msg00768.html) and 
bad-whitespace was removed in 2.6.


Thanks, updating pylint fixed this problem, indeed!

But maybe the iotests should check the pylint version before using it?

 Thomas

Re: [PATCH v4 3/9] linux-user/safe-syscall.inc.S: Move to common-user


On 11/16/21 10:03 PM, Warner Losh wrote:

I had to add this:

diff --git a/meson.build b/meson.build
index 0a88bff8d2..349e7a988f 100644
--- a/meson.build
+++ b/meson.build
@@ -2880,6 +2880,8 @@ foreach target : target_dirs
      endif
      if 'CONFIG_BSD_USER' in config_target
        base_dir = 'bsd-user'
+      target_inc += include_directories('bsd-user/host/' / config_host['ARCH'])
+      target_inc += include_directories('common-user/host/' / 
config_host['ARCH'])


I get an error for adding bsd-user/host/ at this point, because bsd-user/host/arch does 
not yet exist.  But I can certainly add common-user/host/ now.



r~

Re: [PATCH v4 20/25] block_int-common.h: assertion in the callers of BlockDriver function pointers


On 17.11.21 12:33, Emanuele Giuseppe Esposito wrote:



On 15/11/2021 13:48, Hanna Reitz wrote:

On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Stefan Hajnoczi 
---
  block.c | 17 +
  1 file changed, 17 insertions(+)

diff --git a/block.c b/block.c
index 94bff5c757..40c4729b8d 100644
--- a/block.c
+++ b/block.c


[...]

@@ -2148,6 +2152,7 @@ static void bdrv_child_perm(BlockDriverState 
*bs, BlockDriverState *child_bs,

  uint64_t *nperm, uint64_t *nshared)
  {
  assert(bs->drv && bs->drv->bdrv_child_perm);
+    assert(qemu_in_main_thread());
  bs->drv->bdrv_child_perm(bs, c, role, reopen_queue,
   parent_perm, parent_shared,
   nperm, nshared);


(Should’ve noticed earlier, but only did now...)

First, this function is indirectly called by bdrv_refresh_perms(). I 
understand that all perm-related functions are classified as GS.


However, bdrv_co_invalidate_cache() invokes bdrv_refresh_perms. Being 
declared in block/coroutine.h, it’s an I/O function, so it mustn’t 
call such a GS function. BlockDriver.bdrv_co_invalidate_cache(), 
bdrv_invalidate_cache(), and blk_invalidate_cache() are also 
classified as I/O functions. Perhaps all of these functions should be 
classified as GS functions?  I believe their callers and their 
purpose would allow for this.


I think that the *_invalidate_cache functions are I/O.
First of all, test-block-iothread.c calls bdrv_invalidate_cache in 
test_sync_op_invalidate_cache, which is purposefully called in an 
iothread. So that hints that we want it as I/O.


Hm, OK, but bdrv_co_invalidate_cache() calls bdrv_refresh_perms(), which 
is a GS function, so that shouldn’t work, right?


(Small mistake I just noticed: blk_invalidate_cache has the BQL 
assertion even though it is rightly put in block-backend-io.h




Second, it’s called by bdrv_child_refresh_perms(), which is called by 
block_crypto_amend_options_generic_luks().  This function is called 
by block_crypto_co_amend_luks(), which is a BlockDriver.bdrv_co_amend 
implementation, which is classified as an I/O function.


Honestly, I don’t know how to fix that mess.  The best would be if we 
could make the perm functions thread-safe and classify them as I/O, 
but it seems to me like that’s impossible (I sure hope I’m wrong).  
On the other hand, .bdrv_co_amend very much strikes me like a GS 
function, but it isn’t.  I’m afraid it must work on nodes that are 
not in the main context, and it launches a job, so AFAIU we 
absolutely cannot run it under the BQL.


It almost seems to me like we’d need a thread-safe variant of the 
perm functions that’s allowed to fail when it cannot guarantee thread 
safety or something.  Or perhaps I’m wrong and the perm functions can 
actually be classified as thread-safe and I/O, that’d be great…


I think that since we are currently only splitting and not taking care 
of the actual I/O thread safety, we can move the _perm functions in 
I/O, and add a nice TODO to double check their thread safety.


:/

I would really, really like to avoid that unless it’s clear that we can 
make them thread-safe, or that there’s a way to take the BQL in I/O 
functions to call GS functions.  But the latter still wouldn’t make the 
perm functions I/O functions.  At most, I’d sort them under common 
functions.


I mean, if they are not thread-safe after the split it means they are 
not thread safe also right now.


Yes, sorry I wasn’t clear, I think there’s a pre-existing problem that 
your series only unveils.  I don’t know whether it has implications in 
practice yet.


Hanna

Re: [PATCH v3 2/4] s390x: kvm: topology: interception of PTF instruction

2021-11-17 Thread Pierre Morel





On 10/13/21 09:25, Thomas Huth wrote:

On 16/09/2021 15.50, Pierre Morel wrote:

When the host supports the CPU topology facility, the PTF
instruction with function code 2 is interpreted by the SIE,
provided that the userland hypervizor activates the interpretation
by using the KVM_CAP_S390_CPU_TOPOLOGY KVM extension.

The PTF instructions with function code 0 and 1 are intercepted
and must be emulated by the userland hypervizor.

Signed-off-by: Pierre Morel 
---
  hw/s390x/s390-virtio-ccw.c | 36 ++
  include/hw/s390x/s390-virtio-ccw.h |  6 +
  target/s390x/kvm/kvm.c | 15 +
  3 files changed, 57 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 61aeccb163..894f013139 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -404,6 +404,42 @@ static void 
s390_pv_prepare_reset(S390CcwMachineState *ms)

  s390_pv_prep_reset();
  }


Could you please add a comment in front of this function, with some 
explanations? If I've got that right, it's currently rather only a 
"dummy" function, rejecting FC 0 and 1, and FC 2 is always handled by 
the SIE, right?


I just saw I did not answer this question.

Yes function code 2 is handled by the SIE but it is not really a dummy 
function as without it PTF 0 or 1 would trigger a program check.


I will add a comment basically:

"We assume horizontal topology, the only one supported currently by 
Linux consequently we answer to function code 0 requesting horizontal 
polarization that it is already the current polarization and reject 
vertical polarization request without further explanation."



regards,
Pierre

--
Pierre Morel
IBM Lab Boeblingen

Re: [PULL 0/6] Misc patches for 6.2-rc2


On 11/17/21 11:19 AM, Thomas Huth wrote:

  Hi Richard!
  
The following changes since commit 8d5fcb1990bc64b62c0bc12121fe510940be5664:


   Merge tag 'python-pull-request' of https://gitlab.com/jsnow/qemu into 
staging (2021-11-17 07:41:08 +0100)

are available in the Git repository at:

   https://gitlab.com/thuth/qemu.git tags/pull-request-2021-11-17

for you to fetch changes up to d06f3bf922679d89815fde9eac9264ba44e37954:

   gitlab-ci/cirrus: Increase timeout to 80 minutes (2021-11-17 10:20:38 +0100)


* Remove some unused #defines in s390x code
* rSTify some of the development process pages from the Wiki
* Revert a useless patch in the device-crash-test script
* Bump timeout of the Cirrus-CI jobs to 80 minutes


Kashyap Chamarthy (3):
   docs: rSTify the "TrivialPatches" wiki
   docs: rSTify the "SubmitAPullRequest" wiki
   docs: rSTify the "SubmitAPatch" wiki

Thomas Huth (3):
   target/s390x/cpu.h: Remove unused SIGP_MODE defines
   Revert "device-crash-test: Ignore errors about a bus not being available"
   gitlab-ci/cirrus: Increase timeout to 80 minutes

  .gitlab-ci.d/cirrus.yml  |   1 +
  docs/devel/index.rst |   3 +
  docs/devel/submitting-a-patch.rst| 456 +++
  docs/devel/submitting-a-pull-request.rst |  76 ++
  docs/devel/trivial-patches.rst   |  50 
  scripts/device-crash-test|   1 -
  target/s390x/cpu.h   |   5 -
  7 files changed, 586 insertions(+), 6 deletions(-)
  create mode 100644 docs/devel/submitting-a-patch.rst
  create mode 100644 docs/devel/submitting-a-pull-request.rst
  create mode 100644 docs/devel/trivial-patches.rst



Applied, thanks.

r~

Re: [PATCH-for-6.2 v2 1/2] hw/nvme/ctrl: Fix buffer overrun in nvme_changed_nslist (CVE-2021-3947)

2021-11-17 Thread Klaus Jensen

On Nov 17 13:35, Philippe Mathieu-Daudé wrote:
> Both 'buf_len' and 'off' arguments are under guest control.
> Since nvme_c2h() doesn't check out of boundary access, the
> caller must check for eventual buffer overrun on 'trans_len'.
> 
> Cc: qemu-sta...@nongnu.org
> Reported-by: Qiuhao Li 
> Fixes: f432fdfa121 ("support changed namespace asynchronous event")
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/nvme/ctrl.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 6a571d18cfa..93a24464647 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -4168,8 +4168,11 @@ static uint16_t nvme_changed_nslist(NvmeCtrl *n, 
> uint8_t rae, uint32_t buf_len,
>  int i = 0;
>  uint32_t nsid;
>  
> -memset(nslist, 0x0, sizeof(nslist));
>  trans_len = MIN(sizeof(nslist) - off, buf_len);
> +if (trans_len >= sizeof(nslist)) {
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +memset(nslist, 0x0, sizeof(nslist));
>  
>  while ((nsid = find_first_bit(n->changed_nsids, NVME_CHANGED_NSID_SIZE)) 
> !=
>  NVME_CHANGED_NSID_SIZE) {

The issue I mentioned with off > sizeof(nslist). I'll send a fix that
just deals with it like all the other log pages.

There is probably a better way to do these checks, but I'm not sure how
right now.


signature.asc
Description: PGP signature

Re: [PATCH v4 20/25] block_int-common.h: assertion in the callers of BlockDriver function pointers

2021-11-17 Thread Emanuele Giuseppe Esposito





On 17/11/2021 13:51, Hanna Reitz wrote:

On 17.11.21 12:33, Emanuele Giuseppe Esposito wrote:



On 15/11/2021 13:48, Hanna Reitz wrote:

On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Stefan Hajnoczi 
---
  block.c | 17 +
  1 file changed, 17 insertions(+)

diff --git a/block.c b/block.c
index 94bff5c757..40c4729b8d 100644
--- a/block.c
+++ b/block.c


[...]

@@ -2148,6 +2152,7 @@ static void bdrv_child_perm(BlockDriverState 
*bs, BlockDriverState *child_bs,

  uint64_t *nperm, uint64_t *nshared)
  {
  assert(bs->drv && bs->drv->bdrv_child_perm);
+    assert(qemu_in_main_thread());
  bs->drv->bdrv_child_perm(bs, c, role, reopen_queue,
   parent_perm, parent_shared,
   nperm, nshared);


(Should’ve noticed earlier, but only did now...)

First, this function is indirectly called by bdrv_refresh_perms(). I 
understand that all perm-related functions are classified as GS.


However, bdrv_co_invalidate_cache() invokes bdrv_refresh_perms. Being 
declared in block/coroutine.h, it’s an I/O function, so it mustn’t 
call such a GS function. BlockDriver.bdrv_co_invalidate_cache(), 
bdrv_invalidate_cache(), and blk_invalidate_cache() are also 
classified as I/O functions. Perhaps all of these functions should be 
classified as GS functions?  I believe their callers and their 
purpose would allow for this.


I think that the *_invalidate_cache functions are I/O.
First of all, test-block-iothread.c calls bdrv_invalidate_cache in 
test_sync_op_invalidate_cache, which is purposefully called in an 
iothread. So that hints that we want it as I/O.


Hm, OK, but bdrv_co_invalidate_cache() calls bdrv_refresh_perms(), which 
is a GS function, so that shouldn’t work, right?


Ok let's take a step back for one moment: can you tell me why the perm 
functions should be GS?


On one side I see they are also used by I/O, as we can see above. On the 
other side, I kinda see that permission should only be modified under 
BQL. But I don't have any valid point to sustain that.
So I wonder if you have any specific and more valid reason to put them 
as GS.


Maybe clarifying this will help finding a clean solution to this problem.



(Small mistake I just noticed: blk_invalidate_cache has the BQL 
assertion even though it is rightly put in block-backend-io.h




Second, it’s called by bdrv_child_refresh_perms(), which is called by 
block_crypto_amend_options_generic_luks().  This function is called 
by block_crypto_co_amend_luks(), which is a BlockDriver.bdrv_co_amend 
implementation, which is classified as an I/O function.


Honestly, I don’t know how to fix that mess.  The best would be if we 
could make the perm functions thread-safe and classify them as I/O, 
but it seems to me like that’s impossible (I sure hope I’m wrong). On 
the other hand, .bdrv_co_amend very much strikes me like a GS 
function, but it isn’t.  I’m afraid it must work on nodes that are 
not in the main context, and it launches a job, so AFAIU we 
absolutely cannot run it under the BQL.


It almost seems to me like we’d need a thread-safe variant of the 
perm functions that’s allowed to fail when it cannot guarantee thread 
safety or something.  Or perhaps I’m wrong and the perm functions can 
actually be classified as thread-safe and I/O, that’d be great…


I think that since we are currently only splitting and not taking care 
of the actual I/O thread safety, we can move the _perm functions in 
I/O, and add a nice TODO to double check their thread safety.


:/

I would really, really like to avoid that unless it’s clear that we can 
make them thread-safe, or that there’s a way to take the BQL in I/O 
functions to call GS functions.  But the latter still wouldn’t make the 
perm functions I/O functions.  At most, I’d sort them under common 
functions.


I mean, if they are not thread-safe after the split it means they are 
not thread safe also right now.


Yes, sorry I wasn’t clear, I think there’s a pre-existing problem that 
your series only unveils.  I don’t know whether it has implications in 
practice yet.


Hanna

Re: [PATCH-for-6.2 v2 2/2] hw/nvme/ctrl: Prevent buffer overrun in nvme_error_info()

2021-11-17 Thread Klaus Jensen

On Nov 17 13:35, Philippe Mathieu-Daudé wrote:
> Both 'buf_len' and 'off' arguments are under guest control.
> Since nvme_c2h() doesn't check out of boundary access, the
> caller must check for eventual buffer overrun on 'trans_len'.
> 
> Cc: qemu-sta...@nongnu.org
> Fixes: 94a7897c41d ("add support for the get log page command")
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/nvme/ctrl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 93a24464647..7414f3b4dd1 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -4146,7 +4146,8 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t 
> rae, uint32_t buf_len,
>  uint32_t trans_len;
>  NvmeErrorLog errlog;
>  
> -if (off >= sizeof(errlog)) {
> +trans_len = MIN(sizeof(errlog) - off, buf_len);
> +if (trans_len >= sizeof(errlog)) {
>  return NVME_INVALID_FIELD | NVME_DNR;
>  }
>  
> @@ -4155,7 +4156,6 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t 
> rae, uint32_t buf_len,
>  }
>  
>  memset(&errlog, 0x0, sizeof(errlog));
> -trans_len = MIN(sizeof(errlog) - off, buf_len);
>  
>  return nvme_c2h(n, (uint8_t *)&errlog, trans_len, req);
>  }
> -- 
> 2.31.1
> 

I don't see any buffer overrun issue prior to this patch. However, there
is a functional bug since the offset is not added in the c2h.


signature.asc
Description: PGP signature

[PATCH for-6.2] hw/nvme: fix buffer overrun in nvme_changed_nslist (CVE-2021-3947)

2021-11-17 Thread Klaus Jensen

From: Klaus Jensen 

Fix missing offset verification.

Cc: qemu-sta...@nongnu.org
Cc: Philippe Mathieu-Daudé 
Reported-by: Qiuhao Li 
Fixes: f432fdfa121 ("support changed namespace asynchronous event")
Signed-off-by: Klaus Jensen 
---

Note: Since its so easy to mess this fix up, the log pages code could
probably use a refactor - there is a lot of duplicated code as well and
it's easy to miss a check like this. However, defer that for 7.0.

 hw/nvme/ctrl.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 6a571d18cfae..5f573c417b3d 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4168,6 +4168,11 @@ static uint16_t nvme_changed_nslist(NvmeCtrl *n, uint8_t 
rae, uint32_t buf_len,
 int i = 0;
 uint32_t nsid;
 
+if (off >= sizeof(nslist)) {
+trace_pci_nvme_err_invalid_log_page_offset(off, sizeof(nslist));
+return NVME_INVALID_FIELD | NVME_DNR;
+}
+
 memset(nslist, 0x0, sizeof(nslist));
 trans_len = MIN(sizeof(nslist) - off, buf_len);
 
-- 
2.34.0

Re: [PATCH v4 20/25] block_int-common.h: assertion in the callers of BlockDriver function pointers


On 17.11.21 14:09, Emanuele Giuseppe Esposito wrote:



On 17/11/2021 13:51, Hanna Reitz wrote:

On 17.11.21 12:33, Emanuele Giuseppe Esposito wrote:



On 15/11/2021 13:48, Hanna Reitz wrote:

On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Stefan Hajnoczi 
---
  block.c | 17 +
  1 file changed, 17 insertions(+)

diff --git a/block.c b/block.c
index 94bff5c757..40c4729b8d 100644
--- a/block.c
+++ b/block.c


[...]

@@ -2148,6 +2152,7 @@ static void bdrv_child_perm(BlockDriverState 
*bs, BlockDriverState *child_bs,

  uint64_t *nperm, uint64_t *nshared)
  {
  assert(bs->drv && bs->drv->bdrv_child_perm);
+    assert(qemu_in_main_thread());
  bs->drv->bdrv_child_perm(bs, c, role, reopen_queue,
   parent_perm, parent_shared,
   nperm, nshared);


(Should’ve noticed earlier, but only did now...)

First, this function is indirectly called by bdrv_refresh_perms(). 
I understand that all perm-related functions are classified as GS.


However, bdrv_co_invalidate_cache() invokes bdrv_refresh_perms. 
Being declared in block/coroutine.h, it’s an I/O function, so it 
mustn’t call such a GS function. 
BlockDriver.bdrv_co_invalidate_cache(), bdrv_invalidate_cache(), 
and blk_invalidate_cache() are also classified as I/O functions. 
Perhaps all of these functions should be classified as GS 
functions?  I believe their callers and their purpose would allow 
for this.


I think that the *_invalidate_cache functions are I/O.
First of all, test-block-iothread.c calls bdrv_invalidate_cache in 
test_sync_op_invalidate_cache, which is purposefully called in an 
iothread. So that hints that we want it as I/O.


Hm, OK, but bdrv_co_invalidate_cache() calls bdrv_refresh_perms(), 
which is a GS function, so that shouldn’t work, right?


Ok let's take a step back for one moment: can you tell me why the perm 
functions should be GS?


On one side I see they are also used by I/O, as we can see above. On 
the other side, I kinda see that permission should only be modified 
under BQL. But I don't have any valid point to sustain that.
So I wonder if you have any specific and more valid reason to put them 
as GS.


First I believe permissions to be part of the block graph state, and so 
global state.  But, well, that could be declared just a hunch.


Second permissions have transaction mechanisms – you try to update them 
on every node, if one fails, all are aborted, else all are committed.  
So this is by no means an atomic operation but quite drawn out.


The problem with this is that I/O operations rely on permissions, e.g. 
you’ll get assertion failures when trying to write but don’t have the 
WRITE permission.  So it definitely doesn’t seem like something to me 
that can be thread-safe in the sense of cooperating nicely with other 
I/O threads.


Perhaps it’d be fine to do permission updates while the relevant 
subgraph is drained (i.e. blocking all other I/O threads), but I kind of 
feel like the same could be said for all (other) GS operations.  Like, 
you could probably do all kinds of graph changes while all involved 
subgraphs are drained.


Hanna

Re: [PATCH V2] migration/colo: Optimize COLO primary node start code path

2021-11-17 Thread Juan Quintela

Zhang Chen  wrote:
> Optimize COLO primary start path from:
> MIGRATION_STATUS_XXX --> MIGRATION_STATUS_ACTIVE --> MIGRATION_STATUS_COLO 
> --> MIGRATION_STATUS_COMPLETED
> To:
> MIGRATION_STATUS_XXX --> MIGRATION_STATUS_COLO --> MIGRATION_STATUS_COMPLETED
> No need to start primary COLO through "MIGRATION_STATUS_ACTIVE".
>
> Signed-off-by: Zhang Chen 

Reviewed-by: Juan Quintela

Re: [PATCH v4 24/25] job.h: split function pointers in JobDriver

2021-11-17 Thread Emanuele Giuseppe Esposito





On 15/11/2021 16:11, Hanna Reitz wrote:

On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:

The job API will be handled separately in another serie.

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Stefan Hajnoczi 
---
  include/qemu/job.h | 16 
  1 file changed, 16 insertions(+)

diff --git a/include/qemu/job.h b/include/qemu/job.h
index 6e67b6977f..7e9e59f4b8 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -169,12 +169,21 @@ typedef struct Job {
   * Callbacks and other information about a Job driver.
   */
  struct JobDriver {
+
+    /* Fields initialized in struct definition and never changed. */


Like in patch 19, I’d prefer a slightly more verbose comment that I’d 
find more easily readable.



+
  /** Derived Job struct size */
  size_t instance_size;
  /** Enum describing the operation */
  JobType job_type;
+    /*
+ * Functions run without regard to the BQL and may run in any


s/and/that/?


+ * arbitrary thread. These functions do not need to be thread-safe
+ * because the caller ensures that are invoked from one thread at 
time.


s/that/they/ (or “that they”)

I believe .run() must be run in the job’s context, though.  Not sure if 
that’s necessary to note, but it isn’t really an arbitrary thread, and 
block jobs certainly require this (because they run in the block 
device’s context).  Or is that something that’s going to change with I/O 
threading?




What about moving .run() before the comment and add "Must be run in the 
job's context" to its comment description?


maybe also add the following assertion in job_co_entry (that calls 
job->run())?


assert(job->aio_context == qemu_get_current_aio_context());

Emanuele

Re: [PATCH v4 24/25] job.h: split function pointers in JobDriver


On 17.11.21 14:43, Emanuele Giuseppe Esposito wrote:



On 15/11/2021 16:11, Hanna Reitz wrote:

On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:

The job API will be handled separately in another serie.

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Stefan Hajnoczi 
---
  include/qemu/job.h | 16 
  1 file changed, 16 insertions(+)

diff --git a/include/qemu/job.h b/include/qemu/job.h
index 6e67b6977f..7e9e59f4b8 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -169,12 +169,21 @@ typedef struct Job {
   * Callbacks and other information about a Job driver.
   */
  struct JobDriver {
+
+    /* Fields initialized in struct definition and never changed. */


Like in patch 19, I’d prefer a slightly more verbose comment that I’d 
find more easily readable.



+
  /** Derived Job struct size */
  size_t instance_size;
  /** Enum describing the operation */
  JobType job_type;
+    /*
+ * Functions run without regard to the BQL and may run in any


s/and/that/?


+ * arbitrary thread. These functions do not need to be thread-safe
+ * because the caller ensures that are invoked from one thread 
at time.


s/that/they/ (or “that they”)

I believe .run() must be run in the job’s context, though.  Not sure 
if that’s necessary to note, but it isn’t really an arbitrary thread, 
and block jobs certainly require this (because they run in the block 
device’s context).  Or is that something that’s going to change with 
I/O threading?




What about moving .run() before the comment and add "Must be run in 
the job's context" to its comment description?


Sure, works for me.

maybe also add the following assertion in job_co_entry (that calls 
job->run())?


assert(job->aio_context == qemu_get_current_aio_context());


Sounds good!

Hanna

Re: [PATCH] gitlab-ci: Test compilation on Windows with MSYS2



Philippe Mathieu-Daudé  writes:

> On 11/16/21 08:05, Marc-André Lureau wrote:
>> Hi
>> 
>> On Mon, Nov 15, 2021 at 6:31 PM Philippe Mathieu-Daudé > > wrote:
>> 
>> On 11/15/21 15:06, Thomas Huth wrote:
>> > Gitlab also provides runners with Windows, we can use them to
>> > test compilation with MSYS2, in both, 64-bit and 32-bit.
>> >
>> > However, it takes quite a long time to set up the VM, so to
>> > stay in the 1h time frame, we can only compile and check one
>> > target here.
>> 
>> 
>> I wonder why gitlab does not offer the docker executor. On the
>> freedesktop gitlab instance, they have windows docker executor, which
>> speeds up the build time. Maybe we could also have our own Windows
>> runner for qemu?
>
> We could, foss.org provides the QEMU project with x86 VMs resources
> we are not using. What we miss is a sysadmin willing to setup &
> maintain a such runner.

I think we might also have Azure credits from MS, but the same issues
about admin and setup probably exist.

-- 
Alex Bennée

[PATCH v1 2/9] hw/arm/xlnx-versal: Connect Versal's PMC SLCR

Connect Versal's PMC SLCR (system-level control registers) model.

Signed-off-by: Francisco Iglesias 
---
 hw/arm/xlnx-versal.c | 18 ++
 include/hw/arm/xlnx-versal.h |  6 ++
 2 files changed, 24 insertions(+)

diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
index b2705b6925..08e250945f 100644
--- a/hw/arm/xlnx-versal.c
+++ b/hw/arm/xlnx-versal.c
@@ -369,6 +369,23 @@ static void versal_create_efuse(Versal *s, qemu_irq *pic)
 sysbus_connect_irq(SYS_BUS_DEVICE(ctrl), 0, pic[VERSAL_EFUSE_IRQ]);
 }
 
+static void versal_create_pmc_iou_slcr(Versal *s, qemu_irq *pic)
+{
+SysBusDevice *sbd;
+
+object_initialize_child(OBJECT(s), "versal-pmc-iou-slcr", &s->pmc.iou.slcr,
+TYPE_XILINX_VERSAL_PMC_IOU_SLCR);
+
+sbd = SYS_BUS_DEVICE(&s->pmc.iou.slcr);
+sysbus_realize(sbd, &error_fatal);
+
+memory_region_add_subregion(&s->mr_ps, MM_PMC_PMC_IOU_SLCR,
+sysbus_mmio_get_region(sbd, 0));
+
+sysbus_connect_irq(sbd, 0, pic[VERSAL_PMC_IOU_SLCR_IRQ]);
+}
+
+
 /* This takes the board allocated linear DDR memory and creates aliases
  * for each split DDR range/aperture on the Versal address map.
  */
@@ -459,6 +476,7 @@ static void versal_realize(DeviceState *dev, Error **errp)
 versal_create_xrams(s, pic);
 versal_create_bbram(s, pic);
 versal_create_efuse(s, pic);
+versal_create_pmc_iou_slcr(s, pic);
 versal_map_ddr(s);
 versal_unimp(s);
 
diff --git a/include/hw/arm/xlnx-versal.h b/include/hw/arm/xlnx-versal.h
index 895ba12c61..729c093dfc 100644
--- a/include/hw/arm/xlnx-versal.h
+++ b/include/hw/arm/xlnx-versal.h
@@ -26,6 +26,7 @@
 #include "hw/misc/xlnx-versal-xramc.h"
 #include "hw/nvram/xlnx-bbram.h"
 #include "hw/nvram/xlnx-versal-efuse.h"
+#include "hw/misc/xlnx-versal-pmc-iou-slcr.h"
 
 #define TYPE_XLNX_VERSAL "xlnx-versal"
 OBJECT_DECLARE_SIMPLE_TYPE(Versal, XLNX_VERSAL)
@@ -78,6 +79,7 @@ struct Versal {
 struct {
 struct {
 SDHCIState sd[XLNX_VERSAL_NR_SDS];
+XlnxVersalPmcIouSlcr slcr;
 } iou;
 
 XlnxZynqMPRTC rtc;
@@ -113,6 +115,7 @@ struct Versal {
 #define VERSAL_XRAM_IRQ_0  79
 #define VERSAL_BBRAM_APB_IRQ_0 121
 #define VERSAL_RTC_APB_ERR_IRQ 121
+#define VERSAL_PMC_IOU_SLCR_IRQ121
 #define VERSAL_SD0_IRQ_0   126
 #define VERSAL_EFUSE_IRQ   139
 #define VERSAL_RTC_ALARM_IRQ   142
@@ -178,6 +181,9 @@ struct Versal {
 #define MM_FPD_FPD_APU  0xfd5c
 #define MM_FPD_FPD_APU_SIZE 0x100
 
+#define MM_PMC_PMC_IOU_SLCR 0xf106
+#define MM_PMC_PMC_IOU_SLCR_SIZE0x1
+
 #define MM_PMC_SD0  0xf104U
 #define MM_PMC_SD0_SIZE 0x1
 #define MM_PMC_BBRAM_CTRL   0xf11f
-- 
2.11.0

[PATCH v1 1/9] hw/misc: Add a model of Versal's PMC SLCR

Add a model of Versal's PMC SLCR (system-level control registers).

Signed-off-by: Francisco Iglesias 
Signed-off-by: Edgar E. Iglesias 
---
 hw/misc/meson.build|5 +-
 hw/misc/xlnx-versal-pmc-iou-slcr.c | 1437 
 include/hw/misc/xlnx-versal-pmc-iou-slcr.h |   51 +
 3 files changed, 1492 insertions(+), 1 deletion(-)
 create mode 100644 hw/misc/xlnx-versal-pmc-iou-slcr.c
 create mode 100644 include/hw/misc/xlnx-versal-pmc-iou-slcr.h

diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 3f41a3a5b2..e82628a618 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -84,7 +84,10 @@ softmmu_ss.add(when: 'CONFIG_RASPI', if_true: files(
 ))
 softmmu_ss.add(when: 'CONFIG_SLAVIO', if_true: files('slavio_misc.c'))
 softmmu_ss.add(when: 'CONFIG_ZYNQ', if_true: files('zynq_slcr.c'))
-softmmu_ss.add(when: 'CONFIG_XLNX_VERSAL', if_true: 
files('xlnx-versal-xramc.c'))
+softmmu_ss.add(when: 'CONFIG_XLNX_VERSAL', if_true: files(
+  'xlnx-versal-xramc.c',
+  'xlnx-versal-pmc-iou-slcr.c',
+))
 softmmu_ss.add(when: 'CONFIG_STM32F2XX_SYSCFG', if_true: 
files('stm32f2xx_syscfg.c'))
 softmmu_ss.add(when: 'CONFIG_STM32F4XX_SYSCFG', if_true: 
files('stm32f4xx_syscfg.c'))
 softmmu_ss.add(when: 'CONFIG_STM32F4XX_EXTI', if_true: 
files('stm32f4xx_exti.c'))
diff --git a/hw/misc/xlnx-versal-pmc-iou-slcr.c 
b/hw/misc/xlnx-versal-pmc-iou-slcr.c
new file mode 100644
index 00..bd33017963
--- /dev/null
+++ b/hw/misc/xlnx-versal-pmc-iou-slcr.c
@@ -0,0 +1,1437 @@
+/*
+ * QEMU model of Versal's PMC IOU SLCR (system level control registers)
+ *
+ * Copyright (c) 2021 Xilinx Inc.
+ * Written by Edgar E. Iglesias 
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/sysbus.h"
+#include "hw/register.h"
+#include "hw/irq.h"
+#include "qemu/bitops.h"
+#include "qemu/log.h"
+#include "migration/vmstate.h"
+#include "hw/qdev-properties.h"
+#include "hw/misc/xlnx-versal-pmc-iou-slcr.h"
+
+#ifndef XILINX_VERSAL_PMC_IOU_SLCR_ERR_DEBUG
+#define XILINX_VERSAL_PMC_IOU_SLCR_ERR_DEBUG 0
+#endif
+
+REG32(MIO_PIN_0, 0x0)
+FIELD(MIO_PIN_0, L3_SEL, 7, 3)
+FIELD(MIO_PIN_0, L2_SEL, 5, 2)
+FIELD(MIO_PIN_0, L1_SEL, 3, 2)
+FIELD(MIO_PIN_0, L0_SEL, 1, 2)
+REG32(MIO_PIN_1, 0x4)
+FIELD(MIO_PIN_1, L3_SEL, 7, 3)
+FIELD(MIO_PIN_1, L2_SEL, 5, 2)
+FIELD(MIO_PIN_1, L1_SEL, 3, 2)
+FIELD(MIO_PIN_1, L0_SEL, 1, 2)
+REG32(MIO_PIN_2, 0x8)
+FIELD(MIO_PIN_2, L3_SEL, 7, 3)
+FIELD(MIO_PIN_2, L2_SEL, 5, 2)
+FIELD(MIO_PIN_2, L1_SEL, 3, 2)
+FIELD(MIO_PIN_2, L0_SEL, 1, 2)
+REG32(MIO_PIN_3, 0xc)
+FIELD(MIO_PIN_3, L3_SEL, 7, 3)
+FIELD(MIO_PIN_3, L2_SEL, 5, 2)
+FIELD(MIO_PIN_3, L1_SEL, 3, 2)
+FIELD(MIO_PIN_3, L0_SEL, 1, 2)
+REG32(MIO_PIN_4, 0x10)
+FIELD(MIO_PIN_4, L3_SEL, 7, 3)
+FIELD(MIO_PIN_4, L2_SEL, 5, 2)
+FIELD(MIO_PIN_4, L1_SEL, 3, 2)
+FIELD(MIO_PIN_4, L0_SEL, 1, 2)
+REG32(MIO_PIN_5, 0x14)
+FIELD(MIO_PIN_5, L3_SEL, 7, 3)
+FIELD(MIO_PIN_5, L2_SEL, 5, 2)
+FIELD(MIO_PIN_5, L1_SEL, 3, 2)
+FIELD(MIO_PIN_5, L0_SEL, 1, 2)
+REG32(MIO_PIN_6, 0x18)
+FIELD(MIO_PIN_6, L3_SEL, 7, 3)
+FIELD(MIO_PIN_6, L2_SEL, 5, 2)
+FIELD(MIO_PIN_6, L1_SEL, 3, 2)
+FIELD(MIO_PIN_6, L0_SEL, 1, 2)
+REG32(MIO_PIN_7, 0x1c)
+FIELD(MIO_PIN_7, L3_SEL, 7, 3)
+FIELD(MIO_PIN_7, L2_SEL, 5, 2)
+FIELD(MIO_PIN_7, L1_SEL, 3, 2)
+FIELD(MIO_PIN_7, L0_SEL, 1, 2)
+REG32(MIO_PIN_8, 0x20)
+FIELD(MIO_PIN_8, L3_SEL, 7, 3)
+FIELD(MIO_PIN_8, L2_SEL, 5, 2)
+FIELD(MIO_PIN_8, L1_SEL, 3, 2)
+FIELD(MIO_PIN_8, L0_SEL, 1, 2)
+REG32(MIO_PIN_9, 0x24)
+FIELD(MIO_PIN_9, L3_SEL, 7, 3)
+FIELD(MIO_PIN_9, L2_SEL, 5, 2)
+FIELD(MIO_PIN_9, L1_SEL, 3, 2)
+FIELD(MIO_PIN_9, L0_SEL, 1, 2)
+REG32(MIO_PIN_10, 0x28)
+FIELD(MIO_PIN_10, L3_SEL, 7, 3)
+FIELD(MIO_PIN_10, L2_SEL, 5, 2)
+FIELD(MIO_PIN_10, L1_SEL, 3, 2)
+FIELD(MIO_PIN_10, L0_SEL, 1, 2)

[PATCH v1 3/9] include/hw/dma/xlnx_csu_dma: Include ptimer.h and stream.h in the header

Include ptimer.h and stream.h in the header for being able to build and
reuse the DMA model (the first usage of StreamSink, StreamCanPushNotifyFn
and ptimer_state is in the header).

Signed-off-by: Francisco Iglesias 
---
 include/hw/dma/xlnx_csu_dma.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/hw/dma/xlnx_csu_dma.h b/include/hw/dma/xlnx_csu_dma.h
index 9e9dc551e9..8c39e46f58 100644
--- a/include/hw/dma/xlnx_csu_dma.h
+++ b/include/hw/dma/xlnx_csu_dma.h
@@ -21,6 +21,9 @@
 #ifndef XLNX_CSU_DMA_H
 #define XLNX_CSU_DMA_H
 
+#include "hw/ptimer.h"
+#include "hw/stream.h"
+
 #define TYPE_XLNX_CSU_DMA "xlnx.csu_dma"
 
 #define XLNX_CSU_DMA_R_MAX (0x2c / 4)
-- 
2.11.0

[PATCH v1 9/9] hw/arm/xlnx-versal-virt: Connect mt35xu01g flashes to the OSPI

Connect Micron Xccela mt35xu01g flashes to the OSPI flash memory
controller.

Signed-off-by: Francisco Iglesias 
---
 hw/arm/xlnx-versal-virt.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
index d2f55e29b6..f2f12a781e 100644
--- a/hw/arm/xlnx-versal-virt.c
+++ b/hw/arm/xlnx-versal-virt.c
@@ -25,6 +25,8 @@
 #define TYPE_XLNX_VERSAL_VIRT_MACHINE MACHINE_TYPE_NAME("xlnx-versal-virt")
 OBJECT_DECLARE_SIMPLE_TYPE(VersalVirt, XLNX_VERSAL_VIRT_MACHINE)
 
+#define XLNX_VERSAL_NUM_OSPI_FLASH 4
+
 struct VersalVirt {
 MachineState parent_obj;
 
@@ -690,6 +692,27 @@ static void versal_virt_init(MachineState *machine)
 exit(EXIT_FAILURE);
 }
 }
+
+for (i = 0; i < XLNX_VERSAL_NUM_OSPI_FLASH; i++) {
+BusState *spi_bus;
+DeviceState *flash_dev;
+qemu_irq cs_line;
+DriveInfo *dinfo = drive_get_next(IF_MTD);
+
+spi_bus = qdev_get_child_bus(DEVICE(&s->soc.pmc.iou.ospi), "spi0");
+
+flash_dev = qdev_new("mt35xu01g");
+if (dinfo) {
+qdev_prop_set_drive_err(flash_dev, "drive",
+blk_by_legacy_dinfo(dinfo), &error_fatal);
+}
+qdev_realize_and_unref(flash_dev, spi_bus, &error_fatal);
+
+cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
+
+sysbus_connect_irq(SYS_BUS_DEVICE(&s->soc.pmc.iou.ospi),
+   i + 1, cs_line);
+}
 }
 
 static void versal_virt_machine_instance_init(Object *obj)
-- 
2.11.0

[PATCH v1 4/9] hw/dma: Add the DMA control interface

Add an interface for controlling DMA models that are reused with other
models. This allows a controlling model to start transfers through the
DMA while reusing the DMA's handling of transfer state and completion
signaling.

Signed-off-by: Francisco Iglesias 
---
 hw/dma/dma-ctrl.c | 31 
 hw/dma/meson.build|  1 +
 include/hw/dma/dma-ctrl.h | 74 +++
 3 files changed, 106 insertions(+)
 create mode 100644 hw/dma/dma-ctrl.c
 create mode 100644 include/hw/dma/dma-ctrl.h

diff --git a/hw/dma/dma-ctrl.c b/hw/dma/dma-ctrl.c
new file mode 100644
index 00..4a9b68dac1
--- /dev/null
+++ b/hw/dma/dma-ctrl.c
@@ -0,0 +1,31 @@
+/*
+ * DMA control interface.
+ *
+ * Copyright (c) 2021 Xilinx Inc.
+ * Written by Francisco Iglesias 
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include "qemu/osdep.h"
+#include "exec/hwaddr.h"
+#include "hw/dma/dma-ctrl.h"
+
+void dma_ctrl_read_with_notify(DmaCtrl *dma_ctrl, hwaddr addr, uint32_t len,
+   DmaCtrlNotify *notify, bool start_dma)
+{
+DmaCtrlClass *dcc =  DMA_CTRL_GET_CLASS(dma_ctrl);
+dcc->read(dma_ctrl, addr, len, notify, start_dma);
+}
+
+static const TypeInfo dma_ctrl_info = {
+.name  = TYPE_DMA_CTRL,
+.parent= TYPE_INTERFACE,
+.class_size = sizeof(DmaCtrlClass),
+};
+
+static void dma_ctrl_register_types(void)
+{
+type_register_static(&dma_ctrl_info);
+}
+
+type_init(dma_ctrl_register_types)
diff --git a/hw/dma/meson.build b/hw/dma/meson.build
index f3f0661bc3..c0bc134046 100644
--- a/hw/dma/meson.build
+++ b/hw/dma/meson.build
@@ -14,3 +14,4 @@ softmmu_ss.add(when: 'CONFIG_PXA2XX', if_true: 
files('pxa2xx_dma.c'))
 softmmu_ss.add(when: 'CONFIG_RASPI', if_true: files('bcm2835_dma.c'))
 softmmu_ss.add(when: 'CONFIG_SIFIVE_PDMA', if_true: files('sifive_pdma.c'))
 softmmu_ss.add(when: 'CONFIG_XLNX_CSU_DMA', if_true: files('xlnx_csu_dma.c'))
+common_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('dma-ctrl.c'))
diff --git a/include/hw/dma/dma-ctrl.h b/include/hw/dma/dma-ctrl.h
new file mode 100644
index 00..498469395f
--- /dev/null
+++ b/include/hw/dma/dma-ctrl.h
@@ -0,0 +1,74 @@
+/*
+ * DMA control interface.
+ *
+ * Copyright (c) 2021 Xilinx Inc.
+ * Written by Francisco Iglesias 
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef HW_DMA_CTRL_H
+#define HW_DMA_CTRL_H
+
+#include "qemu-common.h"
+#include "hw/hw.h"
+#include "qom/object.h"
+
+#define TYPE_DMA_CTRL "dma-ctrl"
+
+#define DMA_CTRL_CLASS(klass) \
+ OBJECT_CLASS_CHECK(DmaCtrlClass, (klass), TYPE_DMA_CTRL)
+#define DMA_CTRL_GET_CLASS(obj) \
+OBJECT_GET_CLASS(DmaCtrlClass, (obj), TYPE_DMA_CTRL)
+#define DMA_CTRL(obj) \
+ INTERFACE_CHECK(DmaCtrl, (obj), TYPE_DMA_CTRL)
+
+typedef void (*dmactrl_notify_fn)(void *opaque);
+
+typedef struct DmaCtrlNotify {
+void *opaque;
+dmactrl_notify_fn cb;
+} DmaCtrlNotify;
+
+typedef struct DmaCtrl {
+Object Parent;
+} DmaCtrl;
+
+typedef struct DmaCtrlClass {
+InterfaceClass parent;
+
+/*
+ * read: Start a read transfer on the DMA implementing the DMA control
+ * interface
+ *
+ * @dma_ctrl: the DMA implementing this interface
+ * @addr: the address to read
+ * @len: the amount of bytes to read at 'addr'
+ * @notify: the structure containg a callback to call and opaque pointer
+ * to pass the callback when the transfer has been completed
+ * @start_dma: true for starting the DMA transfer and false for just
+ * refilling and proceding an already started transfer
+ */
+void (*read)(DmaCtrl *dma_ctrl, hwaddr addr, uint32_t len,
+ DmaCtrlNotify *notify, bool start_dma);
+} DmaCtrlClass;
+
+/*
+ * Start a read transfer on a DMA implementing the DMA control interface.
+ * The DMA will notify the caller that 'len' bytes have been read at 'addr'
+ * through the callback in the DmaCtrlNotify structure. For allowing refilling
+ * an already started transfer the DMA notifies the caller before considering
+ * the transfer done (e.g. before setting done flags, generating IRQs and
+ * modifying other relevant internal device state).
+ *
+ * @dma_ctrl: the DMA implementing this interface
+ * @addr: the address to read
+ * @len: the amount of bytes to read at 'addr'
+ * @notify: the structure containing a callback to call and opaque pointer
+ * to pass the callback when the transfer has been completed
+ * @start_dma: true for starting the DMA transfer and false for just
+ * refilling and proceding an already started transfer
+ */
+void dma_ctrl_read_with_notify(DmaCtrl *dma_ctrl, hwaddr addr, uint32_t len,
+   DmaCtrlNotify *notify, bool start_dma);
+
+#endif /* HW_DMA_CTRL_H */
-- 
2.11.0

[PATCH v1 5/9] hw/dma/xlnx_csu_dma: Implement the DMA control interface

Implement the DMA control interface for allowing control of DMA operations
from inside models that contain instances of (and reuse) the Xilinx CSU
DMA.

Signed-off-by: Francisco Iglesias 
---
 hw/dma/xlnx_csu_dma.c | 32 
 include/hw/dma/xlnx_csu_dma.h |  4 
 2 files changed, 36 insertions(+)

diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c
index 896bb3574d..9ed6e82225 100644
--- a/hw/dma/xlnx_csu_dma.c
+++ b/hw/dma/xlnx_csu_dma.c
@@ -277,6 +277,11 @@ static uint32_t xlnx_csu_dma_advance(XlnxCSUDMA *s, 
uint32_t len)
 s->regs[R_ADDR_MSB] = dst >> 32;
 }
 
+/* Notify dma-ctrl clients when the transfer has been completed */
+if (size == 0 && s->dma_ctrl_notify) {
+s->dma_ctrl_notify(s->dma_ctrl_opaque);
+}
+
 if (size == 0) {
 xlnx_csu_dma_done(s);
 }
@@ -472,6 +477,29 @@ static uint64_t addr_msb_pre_write(RegisterInfo *reg, 
uint64_t val)
 return val & R_ADDR_MSB_ADDR_MSB_MASK;
 }
 
+static void xlnx_csu_dma_dma_ctrl_read(DmaCtrl *dma_ctrl, hwaddr addr,
+ uint32_t len, DmaCtrlNotify *notify,
+ bool start_dma)
+{
+XlnxCSUDMA *s = XLNX_CSU_DMA(dma_ctrl);
+RegisterInfo *reg = &s->regs_info[R_SIZE];
+uint64_t we = MAKE_64BIT_MASK(0, 4 * 8);
+
+s->regs[R_ADDR] = addr;
+s->regs[R_ADDR_MSB] = (uint64_t)addr >> 32;
+
+if (notify) {
+s->dma_ctrl_notify = notify->cb;
+s->dma_ctrl_opaque = notify->opaque;
+}
+
+if (start_dma) {
+register_write(reg, len, we, object_get_typename(OBJECT(s)), false);
+} else {
+s->regs[R_SIZE] = len;
+}
+}
+
 static const RegisterAccessInfo *xlnx_csu_dma_regs_info[] = {
 #define DMACH_REGINFO(NAME, snd)  \
 (const RegisterAccessInfo []) {   \
@@ -696,6 +724,7 @@ static void xlnx_csu_dma_class_init(ObjectClass *klass, 
void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
 StreamSinkClass *ssc = STREAM_SINK_CLASS(klass);
+DmaCtrlClass *dcc = DMA_CTRL_CLASS(klass);
 
 dc->reset = xlnx_csu_dma_reset;
 dc->realize = xlnx_csu_dma_realize;
@@ -704,6 +733,8 @@ static void xlnx_csu_dma_class_init(ObjectClass *klass, 
void *data)
 
 ssc->push = xlnx_csu_dma_stream_push;
 ssc->can_push = xlnx_csu_dma_stream_can_push;
+
+dcc->read = xlnx_csu_dma_dma_ctrl_read;
 }
 
 static void xlnx_csu_dma_init(Object *obj)
@@ -731,6 +762,7 @@ static const TypeInfo xlnx_csu_dma_info = {
 .instance_init = xlnx_csu_dma_init,
 .interfaces = (InterfaceInfo[]) {
 { TYPE_STREAM_SINK },
+{ TYPE_DMA_CTRL },
 { }
 }
 };
diff --git a/include/hw/dma/xlnx_csu_dma.h b/include/hw/dma/xlnx_csu_dma.h
index 8c39e46f58..f7f086593c 100644
--- a/include/hw/dma/xlnx_csu_dma.h
+++ b/include/hw/dma/xlnx_csu_dma.h
@@ -23,6 +23,7 @@
 
 #include "hw/ptimer.h"
 #include "hw/stream.h"
+#include "hw/dma/dma-ctrl.h"
 
 #define TYPE_XLNX_CSU_DMA "xlnx.csu_dma"
 
@@ -45,6 +46,9 @@ typedef struct XlnxCSUDMA {
 StreamCanPushNotifyFn notify;
 void *notify_opaque;
 
+dmactrl_notify_fn dma_ctrl_notify;
+void *dma_ctrl_opaque;
+
 uint32_t regs[XLNX_CSU_DMA_R_MAX];
 RegisterInfo regs_info[XLNX_CSU_DMA_R_MAX];
 } XlnxCSUDMA;
-- 
2.11.0

[PATCH v1 8/9] hw/block/m25p80: Add support for Micron Xccela flash mt35xu01g

Add support for Micron Xccela flash mt35xu01g.

Signed-off-by: Francisco Iglesias 
---
 hw/block/m25p80.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index b77503dc84..c6bf3c6bfa 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -255,6 +255,8 @@ static const FlashPartInfo known_devices[] = {
 { INFO("n25q512a",0x20ba20,  0,  64 << 10, 1024, ER_4K) },
 { INFO("n25q512ax3",  0x20ba20,  0x1000,  64 << 10, 1024, ER_4K) },
 { INFO("mt25ql512ab", 0x20ba20, 0x1044, 64 << 10, 1024, ER_4K | ER_32K) },
+{ INFO_STACKED("mt35xu01g", 0x2c5b1b, 0x104100, 128 << 10, 1024,
+   ER_4K | ER_32K, 2) },
 { INFO_STACKED("n25q00",0x20ba21, 0x1000, 64 << 10, 2048, ER_4K, 4) },
 { INFO_STACKED("n25q00a",   0x20bb21, 0x1000, 64 << 10, 2048, ER_4K, 4) },
 { INFO_STACKED("mt25ql01g", 0x20ba21, 0x1040, 64 << 10, 2048, ER_4K, 2) },
-- 
2.11.0

[PATCH v1 7/9] hw/arm/xlnx-versal: Connect the OSPI flash memory controller model

Connect the OSPI flash memory controller model (including the source and
destination DMA).

Signed-off-by: Francisco Iglesias 
---
 hw/arm/xlnx-versal.c | 89 
 include/hw/arm/xlnx-versal.h | 18 +
 2 files changed, 107 insertions(+)

diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
index 08e250945f..f8e94a50fd 100644
--- a/hw/arm/xlnx-versal.c
+++ b/hw/arm/xlnx-versal.c
@@ -24,6 +24,7 @@
 
 #define XLNX_VERSAL_ACPU_TYPE ARM_CPU_TYPE_NAME("cortex-a72")
 #define GEM_REVISION0x40070106
+#define NUM_OSPI_IRQ_LINES 3
 
 static void versal_create_apu_cpus(Versal *s)
 {
@@ -385,6 +386,93 @@ static void versal_create_pmc_iou_slcr(Versal *s, qemu_irq 
*pic)
 sysbus_connect_irq(sbd, 0, pic[VERSAL_PMC_IOU_SLCR_IRQ]);
 }
 
+static void versal_create_ospi(Versal *s, qemu_irq *pic)
+{
+SysBusDevice *sbd;
+MemoryRegion *mr_dac;
+
+memory_region_init(&s->pmc.iou.lospi_mr, OBJECT(s),
+   "versal-lospi_mr" , MM_PMC_OSPI_DAC_SIZE);
+
+object_initialize_child(OBJECT(s), "versal-ospi", &s->pmc.iou.ospi,
+TYPE_XILINX_VERSAL_OSPI);
+
+mr_dac = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->pmc.iou.ospi), 1);
+memory_region_add_subregion(&s->pmc.iou.lospi_mr, 0x0, mr_dac);
+
+/* Create the OSPI destination DMA */
+object_initialize_child(OBJECT(s), "versal-ospi-dma-dst",
+&s->pmc.iou.ospi_dma_dst,
+TYPE_XLNX_CSU_DMA);
+
+object_property_set_link(OBJECT(&s->pmc.iou.ospi_dma_dst),
+"dma", OBJECT(get_system_memory()),
+ &error_abort);
+
+sbd = SYS_BUS_DEVICE(&s->pmc.iou.ospi_dma_dst);
+sysbus_realize(sbd, &error_fatal);
+
+memory_region_add_subregion(&s->mr_ps, MM_PMC_OSPI_DMA_DST,
+sysbus_mmio_get_region(sbd, 0));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(sbd), 0, pic[VERSAL_OSPI_IRQ]);
+
+/* Create the OSPI source DMA */
+object_initialize_child(OBJECT(s), "versal-ospi-dma-src",
+&s->pmc.iou.ospi_dma_src,
+TYPE_XLNX_CSU_DMA);
+
+object_property_set_bool(OBJECT(&s->pmc.iou.ospi_dma_src), "is-dst",
+ false, &error_abort);
+
+object_property_set_link(OBJECT(&s->pmc.iou.ospi_dma_src),
+"dma", OBJECT(mr_dac), &error_abort);
+
+object_property_set_link(OBJECT(&s->pmc.iou.ospi_dma_src),
+"stream-connected-dma",
+ OBJECT(&s->pmc.iou.ospi_dma_dst),
+ &error_abort);
+
+sbd = SYS_BUS_DEVICE(&s->pmc.iou.ospi_dma_src);
+sysbus_realize(sbd, &error_fatal);
+
+memory_region_add_subregion(&s->mr_ps, MM_PMC_OSPI_DMA_SRC,
+sysbus_mmio_get_region(sbd, 0));
+
+/* Create the OSPI */
+object_property_set_link(OBJECT(&s->pmc.iou.ospi), "dma-src",
+ OBJECT(&s->pmc.iou.ospi_dma_src), &error_abort);
+
+sbd = SYS_BUS_DEVICE(&s->pmc.iou.ospi);
+sysbus_realize(sbd, &error_fatal);
+
+memory_region_add_subregion(&s->mr_ps, MM_PMC_OSPI,
+sysbus_mmio_get_region(sbd, 0));
+
+memory_region_add_subregion(&s->mr_ps, MM_PMC_OSPI_DAC,
+&s->pmc.iou.lospi_mr);
+
+/* ospi_mux_sel */
+qdev_connect_gpio_out(DEVICE(&s->pmc.iou.slcr), 3,
+  qdev_get_gpio_in(DEVICE(&s->pmc.iou.ospi), 0));
+
+/* OSPI irq */
+object_initialize_child(OBJECT(s), "ospi-irq",
+&s->pmc.iou.ospi_irq, TYPE_OR_IRQ);
+object_property_set_int(OBJECT(&s->pmc.iou.ospi_irq),
+"num-lines", NUM_OSPI_IRQ_LINES, &error_fatal);
+qdev_realize(DEVICE(&s->pmc.iou.ospi_irq), NULL, &error_fatal);
+
+sysbus_connect_irq(SYS_BUS_DEVICE(&s->pmc.iou.ospi), 0,
+   qdev_get_gpio_in(DEVICE(&s->pmc.iou.ospi_irq), 0));
+sysbus_connect_irq(SYS_BUS_DEVICE(&s->pmc.iou.ospi_dma_src), 0,
+   qdev_get_gpio_in(DEVICE(&s->pmc.iou.ospi_irq), 1));
+sysbus_connect_irq(SYS_BUS_DEVICE(&s->pmc.iou.ospi_dma_dst), 0,
+   qdev_get_gpio_in(DEVICE(&s->pmc.iou.ospi_irq), 2));
+
+qdev_connect_gpio_out(DEVICE(&s->pmc.iou.ospi_irq), 0,
+  pic[VERSAL_OSPI_IRQ]);
+}
 
 /* This takes the board allocated linear DDR memory and creates aliases
  * for each split DDR range/aperture on the Versal address map.
@@ -477,6 +565,7 @@ static void versal_realize(DeviceState *dev, Error **errp)
 versal_create_bbram(s, pic);
 versal_create_efuse(s, pic);
 versal_create_pmc_iou_slcr(s, pic);
+versal_create_ospi(s, pic);
 versal_map_ddr(s);
 versal_unimp(s);
 
diff --git a/include/hw/arm/xlnx-versal.h b/include/hw/arm/xlnx-versal.h
index 729c093dfc..dae15db352 100644
--- a/include/hw/arm/xlnx-versal.h
+++ b/include/hw/arm/xlnx-versal.h
@@ -2

[PATCH v1 6/9] hw/ssi: Add a model of Xilinx Versal's OSPI flash memory controller

Add a model of Xilinx Versal's OSPI flash memory controller.

Signed-off-by: Francisco Iglesias 
---
 hw/ssi/meson.build|1 +
 hw/ssi/xlnx-versal-ospi.c | 1892 +
 include/hw/ssi/xlnx-versal-ospi.h |   86 ++
 3 files changed, 1979 insertions(+)
 create mode 100644 hw/ssi/xlnx-versal-ospi.c
 create mode 100644 include/hw/ssi/xlnx-versal-ospi.h

diff --git a/hw/ssi/meson.build b/hw/ssi/meson.build
index 3d6bc82ab1..0ded9cd092 100644
--- a/hw/ssi/meson.build
+++ b/hw/ssi/meson.build
@@ -7,5 +7,6 @@ softmmu_ss.add(when: 'CONFIG_SSI', if_true: files('ssi.c'))
 softmmu_ss.add(when: 'CONFIG_STM32F2XX_SPI', if_true: files('stm32f2xx_spi.c'))
 softmmu_ss.add(when: 'CONFIG_XILINX_SPI', if_true: files('xilinx_spi.c'))
 softmmu_ss.add(when: 'CONFIG_XILINX_SPIPS', if_true: files('xilinx_spips.c'))
+softmmu_ss.add(when: 'CONFIG_XLNX_VERSAL', if_true: 
files('xlnx-versal-ospi.c'))
 softmmu_ss.add(when: 'CONFIG_IMX', if_true: files('imx_spi.c'))
 softmmu_ss.add(when: 'CONFIG_OMAP', if_true: files('omap_spi.c'))
diff --git a/hw/ssi/xlnx-versal-ospi.c b/hw/ssi/xlnx-versal-ospi.c
new file mode 100644
index 00..c02fc143de
--- /dev/null
+++ b/hw/ssi/xlnx-versal-ospi.c
@@ -0,0 +1,1892 @@
+/*
+ * QEMU model of Xilinx Versal's OSPI controller.
+ *
+ * Copyright (c) 2021 Xilinx Inc.
+ * Written by Francisco Iglesias 
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#include "qemu/osdep.h"
+#include "hw/sysbus.h"
+#include "migration/vmstate.h"
+#include "hw/qdev-properties.h"
+#include "qemu/bitops.h"
+#include "qemu/log.h"
+#include "hw/irq.h"
+#include "hw/ssi/xlnx-versal-ospi.h"
+
+#ifndef XILINX_VERSAL_OSPI_ERR_DEBUG
+#define XILINX_VERSAL_OSPI_ERR_DEBUG 0
+#endif
+
+REG32(CONFIG_REG, 0x0)
+FIELD(CONFIG_REG, IDLE_FLD, 31, 1)
+FIELD(CONFIG_REG, DUAL_BYTE_OPCODE_EN_FLD, 30, 1)
+FIELD(CONFIG_REG, CRC_ENABLE_FLD, 29, 1)
+FIELD(CONFIG_REG, CONFIG_RESV2_FLD, 26, 3)
+FIELD(CONFIG_REG, PIPELINE_PHY_FLD, 25, 1)
+FIELD(CONFIG_REG, ENABLE_DTR_PROTOCOL_FLD, 24, 1)
+FIELD(CONFIG_REG, ENABLE_AHB_DECODER_FLD, 23, 1)
+FIELD(CONFIG_REG, MSTR_BAUD_DIV_FLD, 19, 4)
+FIELD(CONFIG_REG, ENTER_XIP_MODE_IMM_FLD, 18, 1)
+FIELD(CONFIG_REG, ENTER_XIP_MODE_FLD, 17, 1)
+FIELD(CONFIG_REG, ENB_AHB_ADDR_REMAP_FLD, 16, 1)
+FIELD(CONFIG_REG, ENB_DMA_IF_FLD, 15, 1)
+FIELD(CONFIG_REG, WR_PROT_FLASH_FLD, 14, 1)
+FIELD(CONFIG_REG, PERIPH_CS_LINES_FLD, 10, 4)
+FIELD(CONFIG_REG, PERIPH_SEL_DEC_FLD, 9, 1)
+FIELD(CONFIG_REG, ENB_LEGACY_IP_MODE_FLD, 8, 1)
+FIELD(CONFIG_REG, ENB_DIR_ACC_CTLR_FLD, 7, 1)
+FIELD(CONFIG_REG, RESET_CFG_FLD, 6, 1)
+FIELD(CONFIG_REG, RESET_PIN_FLD, 5, 1)
+FIELD(CONFIG_REG, HOLD_PIN_FLD, 4, 1)
+FIELD(CONFIG_REG, PHY_MODE_ENABLE_FLD, 3, 1)
+FIELD(CONFIG_REG, SEL_CLK_PHASE_FLD, 2, 1)
+FIELD(CONFIG_REG, SEL_CLK_POL_FLD, 1, 1)
+FIELD(CONFIG_REG, ENB_SPI_FLD, 0, 1)
+REG32(DEV_INSTR_RD_CONFIG_REG, 0x4)
+FIELD(DEV_INSTR_RD_CONFIG_REG, RD_INSTR_RESV5_FLD, 29, 3)
+FIELD(DEV_INSTR_RD_CONFIG_REG, DUMMY_RD_CLK_CYCLES_FLD, 24, 5)
+FIELD(DEV_INSTR_RD_CONFIG_REG, RD_INSTR_RESV4_FLD, 21, 3)
+FIELD(DEV_INSTR_RD_CONFIG_REG, MODE_BIT_ENABLE_FLD, 20, 1)
+FIELD(DEV_INSTR_RD_CONFIG_REG, RD_INSTR_RESV3_FLD, 18, 2)
+FIELD(DEV_INSTR_RD_CONFIG_REG, DATA_XFER_TYPE_EXT_MODE_FLD, 16, 2)
+FIELD(DEV_INSTR_RD_CONFIG_REG, RD_INSTR_RESV2_FLD, 14, 2)
+FIELD(DEV_INSTR_RD_CONFIG_REG, ADDR_XFER_TYPE_STD_MODE_FLD, 12, 2)
+FIELD(DEV_INSTR_RD_CONFIG_REG, PRED_DIS_FLD, 11, 1)
+FIELD(DEV_INSTR_RD_CONFIG_REG, DDR_EN_FLD, 10, 1)
+FIELD(DEV_INSTR_RD_CONFIG_REG, INSTR_TYPE_FLD, 8, 2)
+FIELD(DEV_INSTR_RD_CONFIG_REG, RD_OPCODE_NON_XIP_FLD, 0, 8)
+REG32(DEV_INSTR_WR_CONFIG_REG, 0x8)
+FIELD(DEV_INSTR_WR_CONFIG_REG, WR_INSTR_RESV4_FLD, 29, 3)
+FIELD(DEV_INSTR_WR_CONFIG_REG, DUMMY_WR_CLK_CYCLES_FLD, 24, 5)
+FIELD(DEV_INSTR_WR_CONFIG_REG, WR_INS

Re: [PATCH] tests/9pfs: use g_autofree where possible

2021-11-17 Thread Christian Schoenebeck

On Dienstag, 16. November 2021 17:40:08 CET Christian Schoenebeck wrote:
> Signed-off-by: Christian Schoenebeck 
> ---
>  tests/qtest/virtio-9p-test.c | 86 +++-
>  1 file changed, 25 insertions(+), 61 deletions(-)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 41fed41de1..11861aaf7d 100644

Queued on 9p.next, along with Greg's suggested additional hunk:
https://github.com/cschoenebeck/qemu/commits/9p.next

Thanks!

Best regards,
Christian Schoenebeck

[PATCH v1] chardev/wctable: don't free the instance in wctablet_chr_finalize

2021-11-17 Thread Daniil Tatianin

Object is supposed to be freed by invoking obj->free, and not
obj->instance_finalize. This would lead to use-after-free followed by
double free in object_unref/object_finalize.

Signed-off-by: Daniil Tatianin 
---
 chardev/wctablet.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/chardev/wctablet.c b/chardev/wctablet.c
index e9cb7ca710..fa3c9be04e 100644
--- a/chardev/wctablet.c
+++ b/chardev/wctablet.c
@@ -318,7 +318,6 @@ static void wctablet_chr_finalize(Object *obj)
 TabletChardev *tablet = WCTABLET_CHARDEV(obj);
 
 qemu_input_handler_unregister(tablet->hs);
-g_free(tablet);
 }
 
 static void wctablet_chr_open(Chardev *chr,
-- 
2.25.1

Re: [PATCH v2] hw/arm/virt: Expose empty NUMA nodes through ACPI

2021-11-17 Thread Jonathan Cameron

On Tue, 16 Nov 2021 12:11:29 +0100
David Hildenbrand  wrote:

> >>
> >> Examples include exposing HBM or PMEM to the VM. Just like on real HW,
> >> this memory is exposed via cpu-less, special nodes. In contrast to real
> >> HW, the memory is hotplugged later (I don't think HW supports hotplug
> >> like that yet, but it might just be a matter of time).  
> > 
> > I suppose some of that maybe covered by GENERIC_AFFINITY entries in SRAT
> > some by MEMORY entries. Or nodes created dynamically like with normal
> > hotplug memory.
> >   

The naming of the define is unhelpful.  GENERIC_AFFINITY here corresponds
to Generic Initiator Affinity.  So no good for memory. This is meant for
representation of accelerators / network cards etc so you can get the NUMA
characteristics for them accessing Memory in other nodes.

My understanding of 'traditional' memory hotplug is that typically the
PA into which memory is hotplugged is known at boot time whether or not
the memory is physically present.  As such, you present that in SRAT and rely
on the EFI memory map / other information sources to know the memory isn't
there.  When it is hotplugged later the address is looked up in SRAT to identify
the NUMA node.

That model is less useful for more flexible entities like virtio-mem or
indeed physical hardware such as CXL type 3 memory devices which typically
need their own nodes.

For the CXL type 3 option, currently proposal is to use the CXL table entries
representing Physical Address space regions to work out how many NUMA nodes
are needed and just create extra ones at boot.
https://lore.kernel.org/linux-cxl/163553711933.2509508.2203471175679990.st...@dwillia2-desk3.amr.corp.intel.com

It's a heuristic as we might need more nodes to represent things well kernel
side, but it's better than nothing and less effort that true dynamic node 
creation.
If you chase through the earlier versions of Alison's patch you will find some
discussion of that.

I wonder if virtio-mem should just grow a CDAT instance via a DOE?

That would make all this stuff discoverable via PCI config space rather than 
ACPI
CDAT is at:
https://uefi.org/sites/default/files/resources/Coherent%20Device%20Attribute%20Table_1.01.pdf
but the table access protocol over PCI DOE is currently in the CXL 2.0 spec
(nothing stops others using it though AFAIK).

However, then we'd actually need either dynamic node creation in the OS, or
some sort of reserved pool of extra nodes.  Long term it may be the most
flexible option.

Jonathan

> 
> I'm certainly no SRAT expert, but seems like under VMWare something
> similar can happen:
> 
> https://lkml.kernel.org/r/bae95f0c-faa7-40c6-a0d6-5049b1207...@vmware.com
> 
> "VM was powered on with 4 vCPUs (4 NUMA nodes) and 4GB memory.
> ACPI SRAT reports 128 possible CPUs and 128 possible NUMA nodes."
> 
> Note that that discussion is about hotplugging CPUs to memory-less,
> hotplugged nodes.
> 
> But there seems to be some way to expose possible NUMA nodes. Maybe
> that's via GENERIC_AFFINITY.
>

[PATCH v1 0/9] Xilinx Versal's PMC SLCR and OSPI support

Hi,

This series attempts to add support for Xilinx Versal's PMC SLCR
(system-level control registers) and OSPI flash memory controller to
Xilinx Versal virt machine.

The series start with adding a model of Versal's PMC SLCR and connecting
the model to the Versal virt machine. The series then adds a couple of
headers into the xlnx_csu_dma.h needed for building and reusing it later
with the OSPI. The series thereafter introduces a DMA control interface
and implements the interface in the xlnx_csu_dma for being able to reuse
and control the DMA with the OSPI controller. Thereafter a model of
Versal's OSPI controller is added and connected to the Versal virt
machine. The series then ends with adding initial support for the Micron
Xccelera mt35xu01g flash and flashes of this type are connected to the
OSPI in the Versal virt machine.

Best regards,
Francisco Iglesias

Francisco Iglesias (9):
  hw/misc: Add a model of Versal's PMC SLCR
  hw/arm/xlnx-versal: Connect Versal's PMC SLCR
  include/hw/dma/xlnx_csu_dma: Include ptimer.h and stream.h in the
header
  hw/dma: Add the DMA control interface
  hw/dma/xlnx_csu_dma: Implement the DMA control interface
  hw/ssi: Add a model of Xilinx Versal's OSPI flash memory controller
  hw/arm/xlnx-versal: Connect the OSPI flash memory controller model
  hw/block/m25p80: Add support for Micron Xccela flash mt35xu01g
  hw/arm/xlnx-versal-virt: Connect mt35xu01g flashes to the OSPI

 hw/arm/xlnx-versal-virt.c  |   23 +
 hw/arm/xlnx-versal.c   |  107 ++
 hw/block/m25p80.c  |2 +
 hw/dma/dma-ctrl.c  |   31 +
 hw/dma/meson.build |1 +
 hw/dma/xlnx_csu_dma.c  |   32 +
 hw/misc/meson.build|5 +-
 hw/misc/xlnx-versal-pmc-iou-slcr.c | 1437 +
 hw/ssi/meson.build |1 +
 hw/ssi/xlnx-versal-ospi.c  | 1892 
 include/hw/arm/xlnx-versal.h   |   24 +
 include/hw/dma/dma-ctrl.h  |   74 ++
 include/hw/dma/xlnx_csu_dma.h  |7 +
 include/hw/misc/xlnx-versal-pmc-iou-slcr.h |   51 +
 include/hw/ssi/xlnx-versal-ospi.h  |   86 ++
 15 files changed, 3772 insertions(+), 1 deletion(-)
 create mode 100644 hw/dma/dma-ctrl.c
 create mode 100644 hw/misc/xlnx-versal-pmc-iou-slcr.c
 create mode 100644 hw/ssi/xlnx-versal-ospi.c
 create mode 100644 include/hw/dma/dma-ctrl.h
 create mode 100644 include/hw/misc/xlnx-versal-pmc-iou-slcr.h
 create mode 100644 include/hw/ssi/xlnx-versal-ospi.h

-- 
2.11.0

Re: [PATCH v3 3/3] docs: rSTify the "SubmitAPatch" wiki


On 17/11/2021 11.25, Kashyap Chamarthy wrote:
...

+QEMU follows the usual standard for git commit messages: the first line
+(which becomes the email subject line) is "subsystem: single line
+summary of change". Whether the "single line summary of change" starts
+with a capital is a matter of taste, but we prefer that the summary does
+not end in ".".


That ".". looks a little bit weird in the output ... maybe we should replace
it with "does not end with a dot." ?


Re-looking the output, yes it does look odd.  And yes, your amendment
is good.


I haven't updated that one while picking up the patch - so we might want to 
fix it with an additional patch on top.



+The body of the commit message is a good place to document why your
+change is important. Don't include comments like "This is a suggestion
+for fixing this bug" (they can go below the "---" line in the email so


That --- gets translated into a — character ... I'll replace the "---" with
``---`` to fix it.


Ah, when I locally ran `rst2html5 submitting-a-patch.rst
submitting-a-patch.html` it retained the "---", but when I built QEMU
(`configure --target-list=x86_64-softmmu --enable-docs`), Sphinx does
turn it into an em-dash (—), and missed to notice it.

Thanks for the careful review and submitting the PR.  I'm assuming I
don't need to respin a v4.


Right, patches have been merged now.

Something I just noticed afterwards, after looking at the pages online: 
https://www.qemu.org/docs/master/devel/submitting-a-pull-request.html uses 
"Submit" in the heading, while 
https://www.qemu.org/docs/master/devel/submitting-a-patch.html uses 
"Submitting" ... looks a little bit inconsequent ... should we change it to 
use one form only? The Wiki used "submit", not "submitting", so maybe use 
that one?


 Thomas

Re: [PATCH] gitlab-ci: Test compilation on Windows with MSYS2

2021-11-17 Thread Daniel P . Berrangé

On Wed, Nov 17, 2021 at 01:52:57PM +, Alex Bennée wrote:
> 
> Philippe Mathieu-Daudé  writes:
> 
> > On 11/16/21 08:05, Marc-André Lureau wrote:
> >> Hi
> >> 
> >> On Mon, Nov 15, 2021 at 6:31 PM Philippe Mathieu-Daudé  >> > wrote:
> >> 
> >> On 11/15/21 15:06, Thomas Huth wrote:
> >> > Gitlab also provides runners with Windows, we can use them to
> >> > test compilation with MSYS2, in both, 64-bit and 32-bit.
> >> >
> >> > However, it takes quite a long time to set up the VM, so to
> >> > stay in the 1h time frame, we can only compile and check one
> >> > target here.
> >> 
> >> 
> >> I wonder why gitlab does not offer the docker executor. On the
> >> freedesktop gitlab instance, they have windows docker executor, which
> >> speeds up the build time. Maybe we could also have our own Windows
> >> runner for qemu?
> >
> > We could, foss.org provides the QEMU project with x86 VMs resources
> > we are not using. What we miss is a sysadmin willing to setup &
> > maintain a such runner.
> 
> I think we might also have Azure credits from MS, but the same issues
> about admin and setup probably exist.

I've never explored this in any way, but IIUC, Azure should have
ability to run *windows* containers. If that's possible, then we
could avoid the admin burden of a VM and just use throwaway windows
containers as we do for Linux.

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: Failing QEMU iotests

2021-11-17 Thread Daniel P . Berrangé

On Wed, Nov 17, 2021 at 01:50:12PM +0100, Thomas Huth wrote:
> On 17/11/2021 11.59, Hanna Reitz wrote:
> > On 17.11.21 11:07, Thomas Huth wrote:
> > > 
> > >  Hi!
> > > 
> > > I think it has been working fine for me a couple of weeks ago,
> > > but when I now run:
> > > 
> > >  make check SPEED=slow
> > > 
> > > I'm getting a couple of failing iotests... not sure whether
> > > these are known issues already, so I thought I'd summarize them
> > > here:
> ...
> > > --- /home/thuth/devel/qemu/tests/qemu-iotests/206.out
> > > +++ 206.out.bad
> > > @@ -99,55 +99,19 @@
> > > 
> > >  {"execute": "blockdev-create", "arguments": {"job-id": "job0",
> > > "options": {"driver": "qcow2", "encrypt": {"cipher-alg":
> > > "twofish-128", "cipher-mode": "ctr", "format": "luks", "hash-alg":
> > > "sha1", "iter-time": 10, "ivgen-alg": "plain64", "ivgen-hash-alg":
> > > "md5", "key-secret": "keysec0"}, "file": {"driver": "file",
> > > "filename": "TEST_DIR/PID-t.qcow2"}, "size": 33554432}}}
> > >  {"return": {}}
> > > +Job failed: Unsupported cipher algorithm twofish-128 with ctr mode
> > >  {"execute": "job-dismiss", "arguments": {"id": "job0"}}
> > >  {"return": {}}
> > > 
> > >  image: TEST_IMG
> > >  file format: IMGFMT
> > >  virtual size: 32 MiB (33554432 bytes)
> > > -encrypted: yes
> > >  cluster_size: 65536
> > >  Format specific information:
> > >  compat: 1.1
> > >  compression type: zlib
> > >  lazy refcounts: false
> > >  refcount bits: 16
> > > -    encrypt:
> > > -    ivgen alg: plain64
> > > -    hash alg: sha1
> > > -    cipher alg: twofish-128
> > > -    uuid: ----
> > > -    format: luks
> > > -    cipher mode: ctr
> > > -    slots:
> > > -    [0]:
> > > -    active: true
> > > -    iters: XXX
> > > -    key offset: 4096
> > > -    stripes: 4000
> > > -    [1]:
> > > -    active: false
> > > -    key offset: 69632
> > > -    [2]:
> > > -    active: false
> > > -    key offset: 135168
> > > -    [3]:
> > > -    active: false
> > > -    key offset: 200704
> > > -    [4]:
> > > -    active: false
> > > -    key offset: 266240
> > > -    [5]:
> > > -    active: false
> > > -    key offset: 331776
> > > -    [6]:
> > > -    active: false
> > > -    key offset: 397312
> > > -    [7]:
> > > -    active: false
> > > -    key offset: 462848
> > > -    payload offset: 528384
> > > -    master key iters: XXX
> > >  corrupt: false
> > >  extended l2: false
> > 
> > I doubt this worked a couple of weeks ago, but it’s definitely one that
> > we should just get around to fixing. :/
> 
> Hm, maybe I've did the successful run on a different system last time ... I
> even slightly remember now having seen this before in the past on my current
> system, so yes, likely not something new.

Triggered by me switching QEMU to prefer GNUTLS for crypto by default
in 6.1, as it doesn't bother to support obscure crypto algs that no
one uses in practice for LUKS.


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 v1] hw/i386/amd_iommu: clean up broken event logging

2021-11-17 Thread Daniil Tatianin

- Don't create evt buffer in every function where we want to log,
  instead make amdvi_log_event construct the buffer in-place using the
  arguments it was given.

- Correctly place address & info in the event buffer. Previously both
  would get shifted to incorrect bit offsets leading to evt containing
  uninitialized event type and address.

- Reduce buffer size from 32 to 16 bytes, which is the amount of bytes
  actually needed for event storage.

- Remove amdvi_encode_event & amdvi_setevent_bits, as they are no longer
  needed.

Signed-off-by: Daniil Tatianin 
---
 hw/i386/amd_iommu.c | 62 ++---
 1 file changed, 14 insertions(+), 48 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index fd75cae024..44b995be91 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -164,8 +164,14 @@ static void amdvi_generate_msi_interrupt(AMDVIState *s)
 }
 }
 
-static void amdvi_log_event(AMDVIState *s, uint64_t *evt)
+static void amdvi_log_event(AMDVIState *s, uint16_t devid,
+hwaddr addr, uint16_t info)
 {
+uint64_t evt[2] = {
+devid | ((uint64_t)info << 48),
+addr
+};
+
 /* event logging not enabled */
 if (!s->evtlog_enabled || amdvi_test_mask(s, AMDVI_MMIO_STATUS,
 AMDVI_MMIO_STATUS_EVT_OVF)) {
@@ -190,27 +196,6 @@ static void amdvi_log_event(AMDVIState *s, uint64_t *evt)
 amdvi_generate_msi_interrupt(s);
 }
 
-static void amdvi_setevent_bits(uint64_t *buffer, uint64_t value, int start,
-int length)
-{
-int index = start / 64, bitpos = start % 64;
-uint64_t mask = MAKE_64BIT_MASK(start, length);
-buffer[index] &= ~mask;
-buffer[index] |= (value << bitpos) & mask;
-}
-/*
- * AMDVi event structure
- *0:15   -> DeviceID
- *55:63  -> event type + miscellaneous info
- *63:127 -> related address
- */
-static void amdvi_encode_event(uint64_t *evt, uint16_t devid, uint64_t addr,
-   uint16_t info)
-{
-amdvi_setevent_bits(evt, devid, 0, 16);
-amdvi_setevent_bits(evt, info, 55, 8);
-amdvi_setevent_bits(evt, addr, 63, 64);
-}
 /* log an error encountered during a page walk
  *
  * @addr: virtual address in translation request
@@ -218,11 +203,8 @@ static void amdvi_encode_event(uint64_t *evt, uint16_t 
devid, uint64_t addr,
 static void amdvi_page_fault(AMDVIState *s, uint16_t devid,
  hwaddr addr, uint16_t info)
 {
-uint64_t evt[4];
-
 info |= AMDVI_EVENT_IOPF_I | AMDVI_EVENT_IOPF;
-amdvi_encode_event(evt, devid, addr, info);
-amdvi_log_event(s, evt);
+amdvi_log_event(s, devid, addr, info);
 pci_word_test_and_set_mask(s->pci.dev.config + PCI_STATUS,
 PCI_STATUS_SIG_TARGET_ABORT);
 }
@@ -232,14 +214,10 @@ static void amdvi_page_fault(AMDVIState *s, uint16_t 
devid,
  *  @info : error flags
  */
 static void amdvi_log_devtab_error(AMDVIState *s, uint16_t devid,
-   hwaddr devtab, uint16_t info)
+   hwaddr addr, uint16_t info)
 {
-uint64_t evt[4];
-
 info |= AMDVI_EVENT_DEV_TAB_HW_ERROR;
-
-amdvi_encode_event(evt, devid, devtab, info);
-amdvi_log_event(s, evt);
+amdvi_log_event(s, devid, addr, info);
 pci_word_test_and_set_mask(s->pci.dev.config + PCI_STATUS,
 PCI_STATUS_SIG_TARGET_ABORT);
 }
@@ -248,10 +226,7 @@ static void amdvi_log_devtab_error(AMDVIState *s, uint16_t 
devid,
  */
 static void amdvi_log_command_error(AMDVIState *s, hwaddr addr)
 {
-uint64_t evt[4], info = AMDVI_EVENT_COMMAND_HW_ERROR;
-
-amdvi_encode_event(evt, 0, addr, info);
-amdvi_log_event(s, evt);
+amdvi_log_event(s, 0, addr, AMDVI_EVENT_COMMAND_HW_ERROR);
 pci_word_test_and_set_mask(s->pci.dev.config + PCI_STATUS,
 PCI_STATUS_SIG_TARGET_ABORT);
 }
@@ -261,11 +236,8 @@ static void amdvi_log_command_error(AMDVIState *s, hwaddr 
addr)
 static void amdvi_log_illegalcom_error(AMDVIState *s, uint16_t info,
hwaddr addr)
 {
-uint64_t evt[4];
-
 info |= AMDVI_EVENT_ILLEGAL_COMMAND_ERROR;
-amdvi_encode_event(evt, 0, addr, info);
-amdvi_log_event(s, evt);
+amdvi_log_event(s, 0, addr, info);
 }
 /* log an error accessing device table
  *
@@ -276,11 +248,8 @@ static void amdvi_log_illegalcom_error(AMDVIState *s, 
uint16_t info,
 static void amdvi_log_illegaldevtab_error(AMDVIState *s, uint16_t devid,
   hwaddr addr, uint16_t info)
 {
-uint64_t evt[4];
-
 info |= AMDVI_EVENT_ILLEGAL_DEVTAB_ENTRY;
-amdvi_encode_event(evt, devid, addr, info);
-amdvi_log_event(s, evt);
+amdvi_log_event(s, devid, addr, info);
 }
 /* log an error accessing a PTE entry
  * @addr : address that couldn't be accessed
@@ -288,11 +257,8 @@ static void amdvi_log_illegaldevtab_error(AMDVIState *s, 
uint16_t devid,
 static void amdvi_log_pagetab_er

[RFC PATCH v3 3/5] qapi: Implement x-machine-init QMP command

From: Mirela Grujic 

The x-machine-init QMP command is available only if the -preconfig
option is used and the current machine initialization phase is
accel-created.

The command triggers QEMU to enter machine initialized phase and wait
for the QMP configuration. In the next commit, we will add the
possibility to create devices at this point. To exit the initialized
phase, use the existing x-exit-preconfig QMP command.

As part of preconfig mode, the command has the 'unstable' feature.

Signed-off-by: Mirela Grujic 
Signed-off-by: Damien Hedde 
---
Cc: Alistair Francis 

v3:
 + add 'unstable' feature
 + bump the target version to 7.0
 + fix the entrance check (and drop alistair ack-by). In previous
   version we were only checking we were not too early, we now check
   we are not too late too.
---
 qapi/machine.json | 27 +++
 softmmu/vl.c  | 19 +++
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 8e9a8afb1d..39c2397629 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1617,3 +1617,30 @@
 { 'command': 'query-machine-phase', 'returns': 'MachineInitPhaseStatus',
   'features' : ['unstable'],
   'allow-preconfig': true }
+
+##
+# @x-machine-init:
+#
+# Enter machine initialized phase
+#
+# Features:
+# @unstable: This command is part of the experimental preconfig mode.
+#
+# Since: 7.0
+#
+# Returns: nothing
+#
+# Notes: This command will trigger QEMU to execute initialization steps
+#that are required to enter the machine initialized phase. The command
+#is available only if the -preconfig command line option was passed and
+#if the machine is currently in the accel-created phase.
+#
+# Example:
+#
+# -> { "execute": "x-machine-init" }
+# <- { "return": {} }
+#
+##
+{ 'command': 'x-machine-init',
+  'features' : ['unstable'],
+  'allow-preconfig': true }
diff --git a/softmmu/vl.c b/softmmu/vl.c
index df19b911e6..a3bbe7b249 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -123,6 +123,7 @@
 #include "qapi/qapi-visit-qom.h"
 #include "qapi/qapi-commands-ui.h"
 #include "qapi/qmp/qdict.h"
+#include "qapi/qapi-commands-machine.h"
 #include "qapi/qmp/qerror.h"
 #include "sysemu/iothread.h"
 #include "qemu/guest-random.h"
@@ -2636,10 +2637,16 @@ static void qemu_init_displays(void)
 }
 }
 
-static void qemu_init_board(void)
+void qmp_x_machine_init(Error **errp)
 {
 MachineClass *machine_class = MACHINE_GET_CLASS(current_machine);
 
+if (phase_get() != MACHINE_INIT_PHASE_ACCEL_CREATED) {
+error_setg(errp, "The command is permitted only when "
+ "the machine is in accel-created phase");
+return;
+}
+
 if (machine_class->default_ram_id && current_machine->ram_size &&
 numa_uses_legacy_mem() && !current_machine->ram_memdev_id) {
 create_default_memdev(current_machine, mem_path);
@@ -2732,12 +2739,16 @@ static void qemu_machine_creation_done(void)
 
 void qmp_x_exit_preconfig(Error **errp)
 {
-if (phase_check(MACHINE_INIT_PHASE_INITIALIZED)) {
-error_setg(errp, "The command is permitted only before machine 
initialization");
+if (phase_check(MACHINE_INIT_PHASE_READY)) {
+error_setg(errp, "The command is permitted only before "
+ "the machine is ready");
 return;
 }
 
-qemu_init_board();
+if (!phase_check(MACHINE_INIT_PHASE_INITIALIZED)) {
+qmp_x_machine_init(errp);
+}
+
 qemu_create_cli_devices();
 qemu_machine_creation_done();
 
-- 
2.33.0

[RFC PATCH v3 0/5] QMP support for cold-plugging devices

Hi all,

This series adds support for cold-plugging devices using QMP
commands. It is a step towards machine configuration using QMP, but
it does not allow the user to add more devices than he could do with
the CLI options before.

Right now we can add a device using 2 ways:
+ giving "-device" CLI option. In that case the device is
cold-plugged: it is created before the machine becomes ready.
+ issuing device_add QMP command. In that case the device is
hot-plugged (the command can not be issued before the machine is
ready).

This series allows to issue device_add QMP to cold-plug a device
like we do with "-device" CLI option. All added QMP commands are
marked as 'unstable' in qapi as they are part of preconfig.
We achieve this by introducing a new 'x-machine-init' command to
stop the VM creation at a point where we can cold-plug devices.

We are aware of the ongoing discussion about preconfig future (see
[1]). This patchset includes no major modifications from the v2 (but
the scope is reduced) and "x-machine-init" simply stops the
configuration between qemu_board_init() and qemu_create_cli_devices()
function calls.

As a consequence, in the current state, the timeline is:
+ "x-machine-init" command
+ "device_add" cold-plug commands (no fw_cfg legacy order support)
+ "x-exit-preconfig" command will then trigger the following
+ "-soundhw" CLI options
+ "-fw_cfg" CLI options
+ usb devices creation
+ "-device" CLI options (with fw_cfg legacy order support)
+ some other devices creation (with fw_cfg legacy order support)

We don't know if the differences between -device/device_add are
acceptable or not. To reduce/remove them we could move the
"x-machine-init" stopping point. What do you think ?

Patches 1, 3 and 5 miss a review.

The series is organized as follow:

+ Patches 1 and 2 converts the MachinePhase enum to a qapi definition
and add the 'query-machine-phase'. It allows to introspect the
current machine phase during preconfig as we will now be able to
reach several machine phases using QMP.
+ Patch 3 adds the 'x-machine-init' QMP command to stop QEMU at
machine-initialized phase during preconfig.
+ Patch 4 allows issuing device_add QMP command during the
machine-initialized phase.
+ Patch 5 improves the doc about preconfig in consequence.

[1]:
https://lore.kernel.org/qemu-devel/b31f442d28920447690a6b8cee865bdbacde1283.1635160056.git.mpriv...@redhat.com

Thanks for your feedback.

---
Damien

v3:
+ extracted patches related to cold-plugging devices from the v2 rfc
+ updated for rebase (qapi 'unstable' feature addition and 7.0 version bump)
+ fix check for x-machine-init command (and drop Alistair's ack-by on
patch 4)
+ extracted only a bit of the doc patch
+ drop qdev_set_id patch because it was merged in Kevin's
"qdev: Add JSON -device" series which did a lot of cleanups in
device_add related functions:
https://lore.kernel.org/qemu-devel/20211008133442.141332-1-kw...@redhat.com

v2 was part of this rfc:
https://lore.kernel.org/qemu-devel/20210922161405.140018-1-damien.he...@greensocs.com

Damien Hedde (1):
docs/system: improve doc about preconfig

Mirela Grujic (4):
rename MachineInitPhase enum constants for QAPI compatibility
qapi: Implement query-machine-phase QMP command
qapi: Implement x-machine-init QMP command
qapi: Allow device_add to execute in machine initialized phase

--
2.33.0

[RFC PATCH v3 1/5] rename MachineInitPhase enum constants for QAPI compatibility

From: Mirela Grujic 

This commit is a preparation to switch to a QAPI definition
of the MachineInitPhase enum.

QAPI will generate enumeration constants prefixed with the
MACHINE_INIT_PHASE_, so rename values accordingly.

Signed-off-by: Mirela Grujic 
Signed-off-by: Damien Hedde 
---
 include/hw/qdev-core.h | 10 +-
 hw/core/machine-qmp-cmds.c |  2 +-
 hw/core/machine.c  |  6 +++---
 hw/core/qdev.c |  2 +-
 hw/pci/pci.c   |  2 +-
 hw/usb/core.c  |  2 +-
 hw/virtio/virtio-iommu.c   |  2 +-
 monitor/hmp.c  |  2 +-
 softmmu/qdev-monitor.c |  9 +
 softmmu/vl.c   |  6 +++---
 ui/console.c   |  3 ++-
 11 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 20d3066595..ef2d612d39 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -849,30 +849,30 @@ bool qdev_should_hide_device(const QDict *opts, bool 
from_json, Error **errp);
 
 typedef enum MachineInitPhase {
 /* current_machine is NULL.  */
-PHASE_NO_MACHINE,
+MACHINE_INIT_PHASE_NO_MACHINE,
 
 /* current_machine is not NULL, but current_machine->accel is NULL.  */
-PHASE_MACHINE_CREATED,
+MACHINE_INIT_PHASE_MACHINE_CREATED,
 
 /*
  * current_machine->accel is not NULL, but the machine properties have
  * not been validated and machine_class->init has not yet been called.
  */
-PHASE_ACCEL_CREATED,
+MACHINE_INIT_PHASE_ACCEL_CREATED,
 
 /*
  * machine_class->init has been called, thus creating any embedded
  * devices and validating machine properties.  Devices created at
  * this time are considered to be cold-plugged.
  */
-PHASE_MACHINE_INITIALIZED,
+MACHINE_INIT_PHASE_INITIALIZED,
 
 /*
  * QEMU is ready to start CPUs and devices created at this time
  * are considered to be hot-plugged.  The monitor is not restricted
  * to "preconfig" commands.
  */
-PHASE_MACHINE_READY,
+MACHINE_INIT_PHASE_READY,
 } MachineInitPhase;
 
 extern bool phase_check(MachineInitPhase phase);
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 4f4ab30f8c..ddbdc5212f 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -148,7 +148,7 @@ HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error 
**errp)
 
 void qmp_set_numa_node(NumaOptions *cmd, Error **errp)
 {
-if (phase_check(PHASE_MACHINE_INITIALIZED)) {
+if (phase_check(MACHINE_INIT_PHASE_INITIALIZED)) {
 error_setg(errp, "The command is permitted only before the machine has 
been created");
 return;
 }
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 26ec54e726..8560bb4c42 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1178,7 +1178,7 @@ void machine_run_board_init(MachineState *machine)
 
 accel_init_interfaces(ACCEL_GET_CLASS(machine->accelerator));
 machine_class->init(machine);
-phase_advance(PHASE_MACHINE_INITIALIZED);
+phase_advance(MACHINE_INIT_PHASE_INITIALIZED);
 }
 
 static NotifierList machine_init_done_notifiers =
@@ -1187,7 +1187,7 @@ static NotifierList machine_init_done_notifiers =
 void qemu_add_machine_init_done_notifier(Notifier *notify)
 {
 notifier_list_add(&machine_init_done_notifiers, notify);
-if (phase_check(PHASE_MACHINE_READY)) {
+if (phase_check(MACHINE_INIT_PHASE_READY)) {
 notify->notify(notify, NULL);
 }
 }
@@ -1210,7 +1210,7 @@ void qdev_machine_creation_done(void)
  * ok, initial machine setup is done, starting from now we can
  * only create hotpluggable devices
  */
-phase_advance(PHASE_MACHINE_READY);
+phase_advance(MACHINE_INIT_PHASE_READY);
 qdev_assert_realized_properly();
 
 /* TODO: once all bus devices are qdevified, this should be done
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 84f3019440..ccfd6f0dc4 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -674,7 +674,7 @@ static void device_initfn(Object *obj)
 {
 DeviceState *dev = DEVICE(obj);
 
-if (phase_check(PHASE_MACHINE_READY)) {
+if (phase_check(MACHINE_INIT_PHASE_READY)) {
 dev->hotplugged = 1;
 qdev_hot_added = true;
 }
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e5993c1ef5..f77d9e8d57 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1102,7 +1102,7 @@ static PCIDevice *do_pci_register_device(PCIDevice 
*pci_dev,
 address_space_init(&pci_dev->bus_master_as,
&pci_dev->bus_master_container_region, pci_dev->name);
 
-if (phase_check(PHASE_MACHINE_READY)) {
+if (phase_check(MACHINE_INIT_PHASE_READY)) {
 pci_init_bus_master(pci_dev);
 }
 pci_dev->irq_state = 0;
diff --git a/hw/usb/core.c b/hw/usb/core.c
index 975f76250a..7a9a81c4fe 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -97,7 +97,7 @@ void usb_wakeup(USBEndpoint *ep, unsigned int stream)
 USBDevice *dev = ep->dev;
 USBBus *bus = usb

[RFC PATCH v3 5/5] docs/system: improve doc about preconfig

Separate -S / -preconfig sections and improve a bit the preconfig part.

Signed-off-by: Damien Hedde 
Signed-off-by: Mirela Grujic 
---
 docs/system/managed-startup.rst | 20 +++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/docs/system/managed-startup.rst b/docs/system/managed-startup.rst
index 9bcf98ea79..e1cdbb79b1 100644
--- a/docs/system/managed-startup.rst
+++ b/docs/system/managed-startup.rst
@@ -1,6 +1,9 @@
 Managed start up options
 
 
+CPU Frezee start
+
+
 In system mode emulation, it's possible to create a VM in a paused
 state using the ``-S`` command line option. In this state the machine
 is completely initialized according to command line options and ready
@@ -20,7 +23,12 @@ allowing VM code to run.
 
 However, at the ``-S`` pause point, it's impossible to configure options
 that affect initial VM creation (like: ``-smp``/``-m``/``-numa`` ...) or
-cold plug devices. The experimental ``--preconfig`` command line option
+cold plug devices.
+
+Preconfig (experimental)
+
+
+The experimental ``--preconfig`` command line option
 allows pausing QEMU before the initial VM creation, in a "preconfig" state,
 where additional queries and configuration can be performed via QMP
 before moving on to the resulting configuration startup. In the
@@ -32,4 +40,14 @@ machine, including but not limited to:
 - ``query-qmp-schema``
 - ``query-commands``
 - ``query-status``
+- ``query-machine-phase``
+- ``x-machine-init``
 - ``x-exit-preconfig``
+
+Some commands make QEMU to progress along the VM creation procedure:
+
+- ``x-machine-init`` initializes the machine. Devices can be added only after
+  this command has been issued.
+
+- ``x-exit-preconfig`` leaves the preconfig state. It can be issued at any time
+  during preconfig and it will finish the VM creation.
-- 
2.33.0

[RFC PATCH v3 4/5] qapi: Allow device_add to execute in machine initialized phase

From: Mirela Grujic 

This commit allows to use the QMP command to add a cold-plugged
device like we can do with the CLI option -device.

Note: for device_add command in qdev.json adding the 'allow-preconfig'
option has no effect because the command appears to bypass QAPI (see
TODO at qapi/qdev.json:61). The option is added there solely to
document the intent.
For the same reason, the flags have to be explicitly set in
monitor_init_qmp_commands() when the device_add command is registered.

Signed-off-by: Mirela Grujic 
Signed-off-by: Damien Hedde 
Acked-by: Alistair Francis 
---
 qapi/qdev.json | 3 ++-
 monitor/misc.c | 2 +-
 softmmu/qdev-monitor.c | 6 ++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index 69656b14df..51c5b2fbcf 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -74,7 +74,8 @@
 { 'command': 'device_add',
   'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
   'gen': false, # so we can get the additional arguments
-  'features': ['json-cli'] }
+  'features': ['json-cli'],
+  'allow-preconfig': true }
 
 ##
 # @device_del:
diff --git a/monitor/misc.c b/monitor/misc.c
index a3a6e47844..fd56dd8f08 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -233,7 +233,7 @@ static void monitor_init_qmp_commands(void)
 qmp_init_marshal(&qmp_commands);
 
 qmp_register_command(&qmp_commands, "device_add",
- qmp_device_add, 0, 0);
+ qmp_device_add, QCO_ALLOW_PRECONFIG, 0);
 
 QTAILQ_INIT(&qmp_cap_negotiation_commands);
 qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 1d6a1c4716..06729145b7 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -842,6 +842,12 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, 
Error **errp)
 QemuOpts *opts;
 DeviceState *dev;
 
+if (!phase_check(MACHINE_INIT_PHASE_INITIALIZED)) {
+error_setg(errp, "The command is permitted only after "
+ "the machine is initialized");
+return;
+}
+
 opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, errp);
 if (!opts) {
 return;
-- 
2.33.0

[RFC PATCH v3 2/5] qapi: Implement query-machine-phase QMP command

From: Mirela Grujic 

The command returns current machine initialization phase.
>From now on, the MachineInitPhase enum is generated from the
QAPI schema.

Signed-off-by: Mirela Grujic 
Signed-off-by: Damien Hedde 
Reviewed-by: Philippe Mathieu-Daudé 
---

v3:
 + add 'unstable' feature
 + bump to version 7.0
---
 qapi/machine.json  | 60 ++
 include/hw/qdev-core.h | 30 ++-
 hw/core/machine-qmp-cmds.c |  9 ++
 hw/core/qdev.c |  5 
 4 files changed, 76 insertions(+), 28 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 067e3f5378..8e9a8afb1d 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1557,3 +1557,63 @@
 { 'command': 'x-query-usb',
   'returns': 'HumanReadableText',
   'features': [ 'unstable' ] }
+
+##
+# @MachineInitPhase:
+#
+# Enumeration of machine initialization phases.
+#
+# @no-machine: machine does not exist
+#
+# @machine-created: machine is created, but its accelerator is not
+#
+# @accel-created: accelerator is created, but the machine properties have not
+# been validated and machine initialization is not done yet
+#
+# @initialized: machine is initialized, thus creating any embedded devices and
+#   validating machine properties. Devices created at this time are
+#   considered to be cold-plugged.
+#
+# @ready: QEMU is ready to start CPUs and devices created at this time are
+# considered to be hot-plugged. The monitor is not restricted to
+# "preconfig" commands.
+#
+# Since: 7.0
+##
+{ 'enum': 'MachineInitPhase',
+  'data': [ 'no-machine', 'machine-created', 'accel-created', 'initialized',
+'ready' ] }
+
+##
+# @MachineInitPhaseStatus:
+#
+# Information about machine initialization phase
+#
+# @phase: the machine initialization phase
+#
+# Since: 7.0
+##
+{ 'struct': 'MachineInitPhaseStatus',
+  'data': { 'phase': 'MachineInitPhase' } }
+
+##
+# @query-machine-phase:
+#
+# Return machine initialization phase
+#
+# Features:
+# @unstable: This command is part of the experimental preconfig mode.
+#
+# Since: 7.0
+#
+# Returns: MachineInitPhaseStatus
+#
+# Example:
+#
+# -> { "execute": "query-machine-phase" }
+# <- { "return": { "phase": "initialized" } }
+#
+##
+{ 'command': 'query-machine-phase', 'returns': 'MachineInitPhaseStatus',
+  'features' : ['unstable'],
+  'allow-preconfig': true }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index ef2d612d39..ac856821ab 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -1,6 +1,7 @@
 #ifndef QDEV_CORE_H
 #define QDEV_CORE_H
 
+#include "qapi/qapi-types-machine.h"
 #include "qemu/queue.h"
 #include "qemu/bitmap.h"
 #include "qemu/rcu.h"
@@ -847,35 +848,8 @@ void device_listener_unregister(DeviceListener *listener);
  */
 bool qdev_should_hide_device(const QDict *opts, bool from_json, Error **errp);
 
-typedef enum MachineInitPhase {
-/* current_machine is NULL.  */
-MACHINE_INIT_PHASE_NO_MACHINE,
-
-/* current_machine is not NULL, but current_machine->accel is NULL.  */
-MACHINE_INIT_PHASE_MACHINE_CREATED,
-
-/*
- * current_machine->accel is not NULL, but the machine properties have
- * not been validated and machine_class->init has not yet been called.
- */
-MACHINE_INIT_PHASE_ACCEL_CREATED,
-
-/*
- * machine_class->init has been called, thus creating any embedded
- * devices and validating machine properties.  Devices created at
- * this time are considered to be cold-plugged.
- */
-MACHINE_INIT_PHASE_INITIALIZED,
-
-/*
- * QEMU is ready to start CPUs and devices created at this time
- * are considered to be hot-plugged.  The monitor is not restricted
- * to "preconfig" commands.
- */
-MACHINE_INIT_PHASE_READY,
-} MachineInitPhase;
-
 extern bool phase_check(MachineInitPhase phase);
 extern void phase_advance(MachineInitPhase phase);
+extern MachineInitPhase phase_get(void);
 
 #endif
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index ddbdc5212f..19f9da4c2d 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -244,3 +244,12 @@ HumanReadableText *qmp_x_query_numa(Error **errp)
  done:
 return human_readable_text_from_str(buf);
 }
+
+MachineInitPhaseStatus *qmp_query_machine_phase(Error **errp)
+{
+MachineInitPhaseStatus *status = g_malloc0(sizeof(*status));
+
+status->phase = phase_get();
+
+return status;
+}
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index ccfd6f0dc4..f22c050c89 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -921,6 +921,11 @@ void phase_advance(MachineInitPhase phase)
 machine_phase = phase;
 }
 
+MachineInitPhase phase_get(void)
+{
+return machine_phase;
+}
+
 static const TypeInfo device_type_info = {
 .name = TYPE_DEVICE,
 .parent = TYPE_OBJECT,
-- 
2.33.0

Re: [PATCH v1] chardev/wctable: don't free the instance in wctablet_chr_finalize

2021-11-17 Thread Marc-André Lureau

Hi

On Wed, Nov 17, 2021 at 6:25 PM Daniil Tatianin
 wrote:
>
> Object is supposed to be freed by invoking obj->free, and not
> obj->instance_finalize. This would lead to use-after-free followed by
> double free in object_unref/object_finalize.
>
> Signed-off-by: Daniil Tatianin 

Fixes: 378af96155d62 ("Add wctablet device")

Reviewed-by: Marc-André Lureau 

> ---
>  chardev/wctablet.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/chardev/wctablet.c b/chardev/wctablet.c
> index e9cb7ca710..fa3c9be04e 100644
> --- a/chardev/wctablet.c
> +++ b/chardev/wctablet.c
> @@ -318,7 +318,6 @@ static void wctablet_chr_finalize(Object *obj)
>  TabletChardev *tablet = WCTABLET_CHARDEV(obj);
>
>  qemu_input_handler_unregister(tablet->hs);
> -g_free(tablet);
>  }
>
>  static void wctablet_chr_open(Chardev *chr,
> --
> 2.25.1
>

[PATCH 0/2] iotests: Fix crypto algorithm failures

Hi,

iotests 149, 206, and 210 fail when qemu uses the gnutls crypto backend
(which is the default as of 8bd0931f6) because they try to use
algorithms that this backend does not support.

Have 206 and 210 use different algorithms instead (patch 1), and let 149
be skipped when it encounters an unsupported algorithm (patch 2).


Hanna Reitz (2):
  iotests: Use aes-128-cbc
  iotests/149: Skip on unsupported ciphers

 tests/qemu-iotests/149 | 23 ++-
 tests/qemu-iotests/206 |  4 ++--
 tests/qemu-iotests/206.out |  6 +++---
 tests/qemu-iotests/210 |  4 ++--
 tests/qemu-iotests/210.out |  6 +++---
 5 files changed, 28 insertions(+), 15 deletions(-)

-- 
2.33.1

[PATCH 1/2] iotests: Use aes-128-cbc

Our gnutls crypto backend (which is the default as of 8bd0931f6)
supports neither twofish-128 nor the CTR mode.  CBC and aes-128 are
supported by all of our backends (as far as I can tell), so use
aes-128-cbc in our iotests.

(We could also use e.g. aes-256-cbc, but the different key sizes would
lead to different key slot offsets and so change the reference output
more, which is why I went with aes-128.)

Signed-off-by: Hanna Reitz 
---
 tests/qemu-iotests/206 | 4 ++--
 tests/qemu-iotests/206.out | 6 +++---
 tests/qemu-iotests/210 | 4 ++--
 tests/qemu-iotests/210.out | 6 +++---
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
index c3cdad4ce4..10eff343f7 100755
--- a/tests/qemu-iotests/206
+++ b/tests/qemu-iotests/206
@@ -162,8 +162,8 @@ with iotests.FilePath('t.qcow2') as disk_path, \
  'encrypt': {
  'format': 'luks',
  'key-secret': 'keysec0',
- 'cipher-alg': 'twofish-128',
- 'cipher-mode': 'ctr',
+ 'cipher-alg': 'aes-128',
+ 'cipher-mode': 'cbc',
  'ivgen-alg': 'plain64',
  'ivgen-hash-alg': 'md5',
  'hash-alg': 'sha1',
diff --git a/tests/qemu-iotests/206.out b/tests/qemu-iotests/206.out
index 3593e8e9c2..80cd274223 100644
--- a/tests/qemu-iotests/206.out
+++ b/tests/qemu-iotests/206.out
@@ -97,7 +97,7 @@ Format specific information:
 
 === Successful image creation (encrypted) ===
 
-{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": 
{"driver": "qcow2", "encrypt": {"cipher-alg": "twofish-128", "cipher-mode": 
"ctr", "format": "luks", "hash-alg": "sha1", "iter-time": 10, "ivgen-alg": 
"plain64", "ivgen-hash-alg": "md5", "key-secret": "keysec0"}, "file": 
{"driver": "file", "filename": "TEST_DIR/PID-t.qcow2"}, "size": 33554432}}}
+{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": 
{"driver": "qcow2", "encrypt": {"cipher-alg": "aes-128", "cipher-mode": "cbc", 
"format": "luks", "hash-alg": "sha1", "iter-time": 10, "ivgen-alg": "plain64", 
"ivgen-hash-alg": "md5", "key-secret": "keysec0"}, "file": {"driver": "file", 
"filename": "TEST_DIR/PID-t.qcow2"}, "size": 33554432}}}
 {"return": {}}
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}
@@ -115,10 +115,10 @@ Format specific information:
 encrypt:
 ivgen alg: plain64
 hash alg: sha1
-cipher alg: twofish-128
+cipher alg: aes-128
 uuid: ----
 format: luks
-cipher mode: ctr
+cipher mode: cbc
 slots:
 [0]:
 active: true
diff --git a/tests/qemu-iotests/210 b/tests/qemu-iotests/210
index 5a62ed4dd1..a4dcc5fe59 100755
--- a/tests/qemu-iotests/210
+++ b/tests/qemu-iotests/210
@@ -83,8 +83,8 @@ with iotests.FilePath('t.luks') as disk_path, \
  },
  'size': size,
  'key-secret': 'keysec0',
- 'cipher-alg': 'twofish-128',
- 'cipher-mode': 'ctr',
+ 'cipher-alg': 'aes-128',
+ 'cipher-mode': 'cbc',
  'ivgen-alg': 'plain64',
  'ivgen-hash-alg': 'md5',
  'hash-alg': 'sha1',
diff --git a/tests/qemu-iotests/210.out b/tests/qemu-iotests/210.out
index 55c0844370..96d9f749dd 100644
--- a/tests/qemu-iotests/210.out
+++ b/tests/qemu-iotests/210.out
@@ -59,7 +59,7 @@ Format specific information:
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}
 
-{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": 
{"cipher-alg": "twofish-128", "cipher-mode": "ctr", "driver": "luks", "file": 
{"driver": "file", "filename": "TEST_DIR/PID-t.luks"}, "hash-alg": "sha1", 
"iter-time": 10, "ivgen-alg": "plain64", "ivgen-hash-alg": "md5", "key-secret": 
"keysec0", "size": 67108864}}}
+{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": 
{"cipher-alg": "aes-128", "cipher-mode": "cbc", "driver": "luks", "file": 
{"driver": "file", "filename": "TEST_DIR/PID-t.luks"}, "hash-alg": "sha1", 
"iter-time": 10, "ivgen-alg": "plain64", "ivgen-hash-alg": "md5", "key-secret": 
"keysec0", "size": 67108864}}}
 {"return": {}}
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}
@@ -71,9 +71,9 @@ encrypted: yes
 Format specific information:
 ivgen alg: plain64
 hash alg: sha1
-cipher alg: twofish-128
+cipher alg: aes-128
 uuid: ----
-cipher mode: ctr
+cipher mode: cbc
 slots:
 [0]:
 active: true
-- 
2.33.1

[PATCH 2/2] iotests/149: Skip on unsupported ciphers

Whenever qemu-img or qemu-io report that some cipher is unsupported,
skip the whole test, because that is probably because qemu has been
configured with the gnutls crypto backend.

We could taylor the algorithm list to what gnutls supports, but this is
a test that is run rather rarely anyway (because it requires
password-less sudo), and so it seems better and easier to skip it.  When
this test is intentionally run to check LUKS compatibility, it seems
better not to limit the algorithms but keep the list extensive.

Signed-off-by: Hanna Reitz 
---
 tests/qemu-iotests/149 | 23 ++-
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index 328fd05a4c..adcef86e88 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -230,6 +230,18 @@ def create_image(config, size_mb):
 fn.truncate(size_mb * 1024 * 1024)
 
 
+def check_cipher_support(output):
+"""Check the output of qemu-img or qemu-io for mention of the respective
+cipher algorithm being unsupported, and if so, skip this test.
+(Returns `output` for convenience.)"""
+
+if 'Unsupported cipher algorithm' in output:
+iotests.notrun('Unsupported cipher algorithm '
+   f'{config.cipher}-{config.keylen}-{config.mode}; '
+   'consider configuring qemu with a different crypto '
+   'backend')
+return output
+
 def qemu_img_create(config, size_mb):
 """Create and format a disk image with LUKS using qemu-img"""
 
@@ -253,7 +265,8 @@ def qemu_img_create(config, size_mb):
 "%dM" % size_mb]
 
 iotests.log("qemu-img " + " ".join(args), 
filters=[iotests.filter_test_dir])
-iotests.log(iotests.qemu_img_pipe(*args), 
filters=[iotests.filter_test_dir])
+iotests.log(check_cipher_support(iotests.qemu_img_pipe(*args)),
+filters=[iotests.filter_test_dir])
 
 def qemu_io_image_args(config, dev=False):
 """Get the args for access an image or device with qemu-io"""
@@ -279,8 +292,8 @@ def qemu_io_write_pattern(config, pattern, offset_mb, 
size_mb, dev=False):
 args = ["-c", "write -P 0x%x %dM %dM" % (pattern, offset_mb, size_mb)]
 args.extend(qemu_io_image_args(config, dev))
 iotests.log("qemu-io " + " ".join(args), filters=[iotests.filter_test_dir])
-iotests.log(iotests.qemu_io(*args), filters=[iotests.filter_test_dir,
- iotests.filter_qemu_io])
+iotests.log(check_cipher_support(iotests.qemu_io(*args)),
+filters=[iotests.filter_test_dir, iotests.filter_qemu_io])
 
 
 def qemu_io_read_pattern(config, pattern, offset_mb, size_mb, dev=False):
@@ -291,8 +304,8 @@ def qemu_io_read_pattern(config, pattern, offset_mb, 
size_mb, dev=False):
 args = ["-c", "read -P 0x%x %dM %dM" % (pattern, offset_mb, size_mb)]
 args.extend(qemu_io_image_args(config, dev))
 iotests.log("qemu-io " + " ".join(args), filters=[iotests.filter_test_dir])
-iotests.log(iotests.qemu_io(*args), filters=[iotests.filter_test_dir,
- iotests.filter_qemu_io])
+iotests.log(check_cipher_support(iotests.qemu_io(*args)),
+filters=[iotests.filter_test_dir, iotests.filter_qemu_io])
 
 
 def test_once(config, qemu_img=False):
-- 
2.33.1

Re: [PATCH 2/2] iotests/149: Skip on unsupported ciphers


On 17.11.21 16:01, Hanna Reitz wrote:

Whenever qemu-img or qemu-io report that some cipher is unsupported,
skip the whole test, because that is probably because qemu has been
configured with the gnutls crypto backend.

We could taylor the algorithm list to what gnutls supports, but this is
a test that is run rather rarely anyway (because it requires
password-less sudo), and so it seems better and easier to skip it.  When
this test is intentionally run to check LUKS compatibility, it seems
better not to limit the algorithms but keep the list extensive.

Signed-off-by: Hanna Reitz 
---
  tests/qemu-iotests/149 | 23 ++-
  1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index 328fd05a4c..adcef86e88 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -230,6 +230,18 @@ def create_image(config, size_mb):
  fn.truncate(size_mb * 1024 * 1024)
  
  
+def check_cipher_support(output):

+"""Check the output of qemu-img or qemu-io for mention of the respective
+cipher algorithm being unsupported, and if so, skip this test.
+(Returns `output` for convenience.)"""
+
+if 'Unsupported cipher algorithm' in output:
+iotests.notrun('Unsupported cipher algorithm '
+   f'{config.cipher}-{config.keylen}-{config.mode}; '


Oops.  Just when I sent this I realized that during refactoring (putting 
this code into its own function) I forgot to pass `config` as a parameter.


Didn’t notice that because...  It seems to work just fine despite 
`config` not being defined here?  Python will forever remain a black box 
for me...


Hanna


+   'consider configuring qemu with a different crypto '
+   'backend')
+return output
+

[PATCH v2 2/2] iotests/149: Skip on unsupported ciphers

Whenever qemu-img or qemu-io report that some cipher is unsupported,
skip the whole test, because that is probably because qemu has been
configured with the gnutls crypto backend.

We could taylor the algorithm list to what gnutls supports, but this is
a test that is run rather rarely anyway (because it requires
password-less sudo), and so it seems better and easier to skip it.  When
this test is intentionally run to check LUKS compatibility, it seems
better not to limit the algorithms but keep the list extensive.

Signed-off-by: Hanna Reitz 
---
 tests/qemu-iotests/149 | 23 ++-
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index 328fd05a4c..d49646ca60 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -230,6 +230,18 @@ def create_image(config, size_mb):
 fn.truncate(size_mb * 1024 * 1024)
 
 
+def check_cipher_support(config, output):
+"""Check the output of qemu-img or qemu-io for mention of the respective
+cipher algorithm being unsupported, and if so, skip this test.
+(Returns `output` for convenience.)"""
+
+if 'Unsupported cipher algorithm' in output:
+iotests.notrun('Unsupported cipher algorithm '
+   f'{config.cipher}-{config.keylen}-{config.mode}; '
+   'consider configuring qemu with a different crypto '
+   'backend')
+return output
+
 def qemu_img_create(config, size_mb):
 """Create and format a disk image with LUKS using qemu-img"""
 
@@ -253,7 +265,8 @@ def qemu_img_create(config, size_mb):
 "%dM" % size_mb]
 
 iotests.log("qemu-img " + " ".join(args), 
filters=[iotests.filter_test_dir])
-iotests.log(iotests.qemu_img_pipe(*args), 
filters=[iotests.filter_test_dir])
+iotests.log(check_cipher_support(config, iotests.qemu_img_pipe(*args)),
+filters=[iotests.filter_test_dir])
 
 def qemu_io_image_args(config, dev=False):
 """Get the args for access an image or device with qemu-io"""
@@ -279,8 +292,8 @@ def qemu_io_write_pattern(config, pattern, offset_mb, 
size_mb, dev=False):
 args = ["-c", "write -P 0x%x %dM %dM" % (pattern, offset_mb, size_mb)]
 args.extend(qemu_io_image_args(config, dev))
 iotests.log("qemu-io " + " ".join(args), filters=[iotests.filter_test_dir])
-iotests.log(iotests.qemu_io(*args), filters=[iotests.filter_test_dir,
- iotests.filter_qemu_io])
+iotests.log(check_cipher_support(config, iotests.qemu_io(*args)),
+filters=[iotests.filter_test_dir, iotests.filter_qemu_io])
 
 
 def qemu_io_read_pattern(config, pattern, offset_mb, size_mb, dev=False):
@@ -291,8 +304,8 @@ def qemu_io_read_pattern(config, pattern, offset_mb, 
size_mb, dev=False):
 args = ["-c", "read -P 0x%x %dM %dM" % (pattern, offset_mb, size_mb)]
 args.extend(qemu_io_image_args(config, dev))
 iotests.log("qemu-io " + " ".join(args), filters=[iotests.filter_test_dir])
-iotests.log(iotests.qemu_io(*args), filters=[iotests.filter_test_dir,
- iotests.filter_qemu_io])
+iotests.log(check_cipher_support(config, iotests.qemu_io(*args)),
+filters=[iotests.filter_test_dir, iotests.filter_qemu_io])
 
 
 def test_once(config, qemu_img=False):
-- 
2.33.1

[PATCH v2 0/2] iotests: Fix crypto algorithm failures

Hi,

iotests 149, 206, and 210 fail when qemu uses the gnutls crypto backend
(which is the default as of 8bd0931f6) because they try to use
algorithms that this backend does not support.

Have 206 and 210 use different algorithms instead (patch 1), and let 149
be skipped when it encounters an unsupported algorithm (patch 2).


v2:
- Fixed the `check_cipher_support()` function introduced in patch 2
  (forgot to pass `config`, though it worked even without, apparently
  because `config` is a global-scope variable)

- Also a good opportunity to CC Dan, who I forgot on v1


git-backport-diff against v1:

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/2:[] [--] 'iotests: Use aes-128-cbc'
002/2:[0008] [FC] 'iotests/149: Skip on unsupported ciphers'


Hanna Reitz (2):
  iotests: Use aes-128-cbc
  iotests/149: Skip on unsupported ciphers

 tests/qemu-iotests/149 | 23 ++-
 tests/qemu-iotests/206 |  4 ++--
 tests/qemu-iotests/206.out |  6 +++---
 tests/qemu-iotests/210 |  4 ++--
 tests/qemu-iotests/210.out |  6 +++---
 5 files changed, 28 insertions(+), 15 deletions(-)

-- 
2.33.1

[PATCH v2 1/2] iotests: Use aes-128-cbc