Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[Cornelia Huck]

On Tue, 03 Dec 2019 08:49:58 +0100
Markus Armbruster  wrote:

> Cornelia Huck  writes:
> 
> > On Sat, 30 Nov 2019 20:42:36 +0100
> > Markus Armbruster  wrote:
> >
> > I don't really want to restart the discussion :), but what about:
> >  
> >> cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(),
> >> qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
> >> crashes when the visitor or the QOM setter fails, and its @errp
> >> argument is null.   
> >
> > "It would crash when the visitor or the QOM setter fails if its @errp
> > argument were NULL." ?
> >
> > (Hope I got my conditionals right...)  
> 
> I don't think this is an improvement.
> 
> The commit message matches a pattern "what's wrong, since when, impact,
> how is it fixed".  The pattern has become habit for me.  Its "what's
> wrong" part is strictly local.  The non-local argument comes in only
> when we assess impact.
> 
> Use of "would" in the what part's conditional signals the condition is
> unlikely.  True (it's actually impossible), but distracting (because it
> involves the non-local argument I'm not yet ready to make).

I think we'll have to agree to disagree here...

> 
> Let me try a different phrasing below.

...but also see below :)

> 
> >> Messed up in commit 137974cea3 's390x/cpumodel:  
> >
> > I agree that "Introduced" is a bit nicer than "Messed up".  
> 
> Works fine for me.  I didn't mean any disrespect --- I'd have to
> disrespect myself: the mess corrected by PATCH 10 is mine.
> 
> >> implement QMP interface "query-cpu-model-expansion"'.
> >> 
> >> Its three callers have the same bug.  Messed up in commit 4e82ef0502  
> 
> Feel free to call it "issue" rather than "bug".  I don't care, but David
> might.
> 
> >> 's390x/cpumodel: implement QMP interface "query-cpu-model-comparison"'
> >> and commit f1a47d08ef 's390x/cpumodel: implement QMP interface
> >> "query-cpu-model-baseline"'.  
> >
> > If we agree, I can tweak the various commit messages for the s390x
> > patches and apply them.  
> 
> Tweaking the non-s390x commit messages as well would be nicer, but
> requires a respin.
> 
> Let's try to craft a mutually agreeable commit message for this patch.
> Here's my attempt:
> 
> s390x: Fix query-cpu-model-FOO error API violations
> 
> cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(),
> qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
> dereferences @errp when the visitor or the QOM setter fails.  That's
> wrong; see the big comment in error.h.  Introduced in commit
> 137974cea3 's390x/cpumodel: implement QMP interface
> "query-cpu-model-expansion"'.
> 
> Its three callers have the same issue.  Introduced in commit
> 4e82ef0502 's390x/cpumodel: implement QMP interface
> "query-cpu-model-comparison"' and commit f1a47d08ef 's390x/cpumodel:
> implement QMP interface "query-cpu-model-baseline"'.
> 
> No caller actually passes null.  To fix, splice in a local Error *err,
> and error_propagate().
> 
> Cc: David Hildenbrand 
> Cc: Cornelia Huck 
> Signed-off-by: Markus Armbruster 

That sounds good to me.

> 
> Adapting it to other patches should be straightforward.

Ok, so how to proceed? I'm happy to tweak the commit messages for
s390x, but that is bound to get messy.

> 
> >> The bugs can't bite as no caller actually passes null.  Fix them
> >> anyway.
> >> 
> >> Cc: David Hildenbrand 
> >> Cc: Cornelia Huck 
> >> Signed-off-by: Markus Armbruster 
> >> ---
> >>  target/s390x/cpu_models.c | 43 ---
> >>  1 file changed, 27 insertions(+), 16 deletions(-)  
> >
> > David, I don't think you gave a R-b for that one yet?  

[PULL 1/1] hvf: correctly inject VMCS_INTR_T_HWINTR versus VMCS_INTR_T_SWINTR.

[Paolo Bonzini]

From: Cameron Esfahani 

Previous implementation in hvf_inject_interrupts() would always inject
VMCS_INTR_T_SWINTR even when VMCS_INTR_T_HWINTR was required.  Now
correctly determine when VMCS_INTR_T_HWINTR is appropriate versus
VMCS_INTR_T_SWINTR.

Make sure to clear ins_len and has_error_code when ins_len isn't
valid and error_code isn't set.

Signed-off-by: Cameron Esfahani 
Message-Id: 

Signed-off-by: Paolo Bonzini 
---
 target/i386/hvf/hvf.c|  4 +++-
 target/i386/hvf/x86hvf.c | 14 +-
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 784e67d77e..d72543dc31 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -637,6 +637,8 @@ static void hvf_store_events(CPUState *cpu, uint32_t 
ins_len, uint64_t idtvec_in
 env->exception_injected = 0;
 env->interrupt_injected = -1;
 env->nmi_injected = false;
+env->ins_len = 0;
+env->has_error_code = false;
 if (idtvec_info & VMCS_IDT_VEC_VALID) {
 switch (idtvec_info & VMCS_IDT_VEC_TYPE) {
 case VMCS_IDT_VEC_HWINTR:
@@ -659,7 +661,7 @@ static void hvf_store_events(CPUState *cpu, uint32_t 
ins_len, uint64_t idtvec_in
 (idtvec_info & VMCS_IDT_VEC_TYPE) == VMCS_IDT_VEC_SWINTR) {
 env->ins_len = ins_len;
 }
-if (idtvec_info & VMCS_INTR_DEL_ERRCODE) {
+if (idtvec_info & VMCS_IDT_VEC_ERRCODE_VALID) {
 env->has_error_code = true;
 env->error_code = rvmcs(cpu->hvf_fd, VMCS_IDT_VECTORING_ERROR);
 }
diff --git a/target/i386/hvf/x86hvf.c b/target/i386/hvf/x86hvf.c
index 1485b95776..edefe5319a 100644
--- a/target/i386/hvf/x86hvf.c
+++ b/target/i386/hvf/x86hvf.c
@@ -345,8 +345,6 @@ void vmx_clear_int_window_exiting(CPUState *cpu)
  ~VMCS_PRI_PROC_BASED_CTLS_INT_WINDOW_EXITING);
 }
 
-#define NMI_VEC 2
-
 bool hvf_inject_interrupts(CPUState *cpu_state)
 {
 X86CPU *x86cpu = X86_CPU(cpu_state);
@@ -357,7 +355,11 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
 bool have_event = true;
 if (env->interrupt_injected != -1) {
 vector = env->interrupt_injected;
-intr_type = VMCS_INTR_T_SWINTR;
+if (env->ins_len) {
+intr_type = VMCS_INTR_T_SWINTR;
+} else {
+intr_type = VMCS_INTR_T_HWINTR;
+}
 } else if (env->exception_nr != -1) {
 vector = env->exception_nr;
 if (vector == EXCP03_INT3 || vector == EXCP04_INTO) {
@@ -366,7 +368,7 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
 intr_type = VMCS_INTR_T_HWEXCEPTION;
 }
 } else if (env->nmi_injected) {
-vector = NMI_VEC;
+vector = EXCP02_NMI;
 intr_type = VMCS_INTR_T_NMI;
 } else {
 have_event = false;
@@ -390,6 +392,8 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
 if (env->has_error_code) {
 wvmcs(cpu_state->hvf_fd, VMCS_ENTRY_EXCEPTION_ERROR,
   env->error_code);
+/* Indicate that VMCS_ENTRY_EXCEPTION_ERROR is valid */
+info |= VMCS_INTR_DEL_ERRCODE;
 }
 /*printf("reinject  %lx err %d\n", info, err);*/
 wvmcs(cpu_state->hvf_fd, VMCS_ENTRY_INTR_INFO, info);
@@ -399,7 +403,7 @@ bool hvf_inject_interrupts(CPUState *cpu_state)
 if (cpu_state->interrupt_request & CPU_INTERRUPT_NMI) {
 if (!(env->hflags2 & HF2_NMI_MASK) && !(info & VMCS_INTR_VALID)) {
 cpu_state->interrupt_request &= ~CPU_INTERRUPT_NMI;
-info = VMCS_INTR_VALID | VMCS_INTR_T_NMI | NMI_VEC;
+info = VMCS_INTR_VALID | VMCS_INTR_T_NMI | EXCP02_NMI;
 wvmcs(cpu_state->hvf_fd, VMCS_ENTRY_INTR_INFO, info);
 } else {
 vmx_set_nmi_window_exiting(cpu_state);
-- 
2.21.0

[PULL 0/1] HVF fix QEMU 4.2-rc

[Paolo Bonzini]

The following changes since commit 39032981fa851d25fb27527f25f046fed800e585:

  Merge remote-tracking branch 'remotes/armbru/tags/pull-error-2019-12-02' into 
staging (2019-12-02 16:29:41 +)

are available in the Git repository at:

  git://github.com/bonzini/qemu.git tags/for-upstream

for you to fetch changes up to 64bef038e777208e4c35beae7f980fbd994b87eb:

  hvf: correctly inject VMCS_INTR_T_HWINTR versus VMCS_INTR_T_SWINTR. 
(2019-12-03 09:11:42 +0100)


* last HVF fix (Cameron)


Cameron Esfahani (1):
  hvf: correctly inject VMCS_INTR_T_HWINTR versus VMCS_INTR_T_SWINTR.

 target/i386/hvf/hvf.c|  4 +++-
 target/i386/hvf/x86hvf.c | 14 +-
 2 files changed, 12 insertions(+), 6 deletions(-)
-- 
2.21.0

Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver

[Geert Uytterhoeven]

Hi Harish,

On Tue, Dec 3, 2019 at 6:42 AM Harish Jenny K N
 wrote:
> > +static int gpio_aggregator_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct gpio_desc **descs;
> > + struct gpiochip_fwd *fwd;
> > + int i, n;
> > +
> > + n = gpiod_count(dev, NULL);
> > + if (n < 0)
> > + return n;
> > +
> > + descs = devm_kmalloc_array(dev, n, sizeof(*descs), GFP_KERNEL);
> > + if (!descs)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < n; i++) {
> > + descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
>
> can you please add this check as well as we need to return EPROBE_DEFER.
>
> if (desc[i] == ERR_PTR(-ENOENT))
> < return -EPROBE_DEFER;

So gpiod_get_index() nevers return -EPROBE_DEFER, but returns -ENOENT
instead?
How can a driver distinguish between "GPIO not found" and "gpiochip driver
not yet initialized"?
Worse, so the *_optional() variants will return NULL in both cases, too, so
the caller will always fall back to optional GPIO not present?

Or am I missing something?

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH] hw/ppc/prep: Remove the deprecated "prep" machine and the OpenHackware BIOS

[Thomas Huth]

On 03/12/2019 08.45, Philippe Mathieu-Daudé wrote:
> On 12/3/19 8:29 AM, Thomas Huth wrote:
>> It's been deprecated since QEMU v3.1. The 40p machine should be
>> used nowadays instead.
>>
>> Signed-off-by: Thomas Huth 
>> ---
>>   .gitmodules    |   3 -
>>   MAINTAINERS    |   1 -
>>   Makefile   |   2 +-
>>   docs/interop/firmware.json |   3 +-
>>   hw/ppc/ppc.c   |  18 --
>>   hw/ppc/prep.c  | 384 +
>>   include/hw/ppc/ppc.h   |   1 -
>>   pc-bios/README |   3 -
>>   pc-bios/ppc_rom.bin    | Bin 1048576 -> 0 bytes
>>   qemu-deprecated.texi   |   6 -
>>   qemu-doc.texi  |  15 +-
>>   roms/openhackware  |   1 -
>>   tests/boot-order-test.c    |  25 ---
>>   tests/cdrom-test.c |   2 +-
>>   tests/endianness-test.c    |   2 +-
>>   15 files changed, 10 insertions(+), 456 deletions(-)
>>   delete mode 100644 pc-bios/ppc_rom.bin
>>   delete mode 16 roms/openhackware
> [...]
>> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
>> index a725bce729..4a6218a516 100644
>> --- a/tests/boot-order-test.c
>> +++ b/tests/boot-order-test.c
>> @@ -108,30 +108,6 @@ static void test_pc_boot_order(void)
>>   test_boot_orders(NULL, read_boot_order_pc, test_cases_pc);
>>   }
>>   -static uint8_t read_m48t59(QTestState *qts, uint64_t addr, uint16_t
>> reg)
>> -{
>> -    qtest_writeb(qts, addr, reg & 0xff);
>> -    qtest_writeb(qts, addr + 1, reg >> 8);
>> -    return qtest_readb(qts, addr + 3);
>> -}
>> -
>> -static uint64_t read_boot_order_prep(QTestState *qts)
>> -{
>> -    return read_m48t59(qts, 0x8000 + 0x74, 0x34);
> 
> I'd rather keep this generic mmio-mapped ISA test.
> Maybe run it with the 40p machine?

I don't think that this is possible in an easy way here. On the prep
machine, the ISA bus is on a hard-coded MMIO address. On the 40p
machine, the ISA bus is behind a PCI-to-ISA bridge, thus the PCI part
needs to be set up first.

> Maybe we can rename this as read_boot_order_mm, and the previous
> read_boot_order_pc as read_boot_order_io.

I don't think it makes much sense. This was completely specific to the
"prep" machine, even the "40p" machine seems to prefer fw_cfg nowadays.
So let's simply remove this old stuff.

 Thomas

Re: [PATCH] monitor: Fix slow reading

[Yury Kotov]

02.12.2019, 23:50, "Markus Armbruster" :
> Yury Kotov  writes:
>
>>  Hi!
>>
>>  29.11.2019, 11:22, "Markus Armbruster" :
>>>  Yury Kotov  writes:
>>>
   The monitor_can_read (as a callback of qemu_chr_fe_set_handlers)
   should return size of buffer which monitor_qmp_read or monitor_read
   can process.
   Currently, monitor_can_read returns 1 as a result of logical not.
   Thus, for each QMP command, len(QMD) iterations of the main loop
   are required to handle a command.
   In fact, these both functions can process any buffer size.
   So, return 1024 as a reasonable size which is enough to process
   the most QMP commands, but not too big to block the main loop for
   a long time.

   Signed-off-by: Yury Kotov 
   ---
    monitor/monitor.c | 9 -
    1 file changed, 8 insertions(+), 1 deletion(-)

   diff --git a/monitor/monitor.c b/monitor/monitor.c
   index 12898b6448..cac3f39727 100644
   --- a/monitor/monitor.c
   +++ b/monitor/monitor.c
   @@ -50,6 +50,13 @@ typedef struct {
    int64_t rate; /* Minimum time (in ns) between two events */
    } MonitorQAPIEventConf;

   +/*
   + * The maximum buffer size which the monitor can process in one 
 iteration
   + * of the main loop. We don't want to block the loop for a long time
   + * because of JSON parser, so use a reasonable value.
   + */
   +#define MONITOR_READ_LEN_MAX 1024
   +
    /* Shared monitor I/O thread */
    IOThread *mon_iothread;

   @@ -498,7 +505,7 @@ int monitor_can_read(void *opaque)
    {
    Monitor *mon = opaque;

   - return !atomic_mb_read(&mon->suspend_cnt);
   + return atomic_mb_read(&mon->suspend_cnt) ? 0 : MONITOR_READ_LEN_MAX;
    }

    void monitor_list_append(Monitor *mon)
>>>
>>>  Prior attempt:
>>>  [PATCH 1/1] monitor: increase amount of data for monitor to read
>>>  Message-Id: <1493732857-10721-1-git-send-email-...@openvz.org>
>>>  https://lists.nongnu.org/archive/html/qemu-devel/2017-05/msg00206.html
>>>
>>>  Review concluded that it breaks HMP command migrate without -d. QMP is
>>>  probably okay. Sadly, no v2.
>>>
>>>  Next one:
>>>  Subject: [PATCH] monitor: increase amount of data for monitor to read
>>>  Message-Id: <20190610105906.28524-1-dplotni...@virtuozzo.com>
>>>  https://lists.nongnu.org/archive/html/qemu-devel/2019-06/msg01912.html
>>>
>>>  Same patch, with a second, suspicious-looking hunk thrown in. I didn't
>>>  make the connection to the prior attempt back then. I wrote "I think I
>>>  need to (re-)examine how QMP reads input, with special consideration to
>>>  its OOB feature."
>>>
>>>  This patch is a cleaner variation on the same theme. Its ramifications
>>>  are as unobvious as ever.
>>>
>>>  I figure the HMP situation is unchanged: not safe, although we could
>>>  probably make it safe if we wanted to (Daniel sketched how). My simpler
>>>  suggestion stands: separate f_can_read() callbacks for HMP and QMP
>>>  [PATCH 1], then change only the one for QMP [PATCH 2].
>>>
>>>  The QMP situation is also unchanged: we still need to think through how
>>>  this affects reading of QMP input, in particular OOB.
>>
>>  I've read the discussion around patches:
>>  "monitor: increase amount of data for monitor to read"
>>  and realized the problem.
>>
>>  It seems that my patch actually has some bugs with HMP and OOB
>>  because of suspend/resume.
>
> For HMP we're sure, for OOB we don't know.
>
>>  IIUC there are some approaches to fix them:
>>
>>  1) Input buffer
>>    1. Add input buffer for Monitor struct
>>    2. Handle commands from monitor_xxx_read callbacks one by one
>>    3. Schedule BH to handle remaining bytes in the buffer
>>
>>  2) Input buffer for suspend/resume
>>    1. Add input buffer for Monitor struct
>>    2. Handle multiple commands until monitor is not suspended
>>    3. If monitor suspended, put remaining data to the buffer
>>    4. Handle remaining data in the buffer when we get resume
>>
>>  We use QEMU 2.12 which doesn't have the full support of OOB and for which 
>> it's
>>  enough to fix HMP case by separating can_read callbacks. But those who use
>>  a newer version of QEMU can use OOB feature to improve HMP/QMP performance.
>
> OOB isn't for monitor performance, it's for monitor availability.
>
> QMP executes one command after the other. While a command executes, the
> monitor is effectively unavailable. This can be a problem. OOB
> execution lets you execute a few special commands right away, without
> waiting for prior commands to complete.
>
>>  So, I'm not sure there's a big sense in introducing some buffers.
>
> Reading byte-wise is pretty pathetic, but it works. I'm not sure how
> much performance buffers can gain us, and whether it's worth the
> necessary review effort. How QMP reads input is not trivial, thanks to
> OOB.
>
> Have you measured the improvement?

Honestly, I have a

Re: [PATCH] virtio-serial-bus: fix memory leak while attach virtio-serial-bus

[Laurent Vivier]

On 03/12/2019 01:53, pannengyuan wrote:
> 
> 
> On 2019/12/2 21:58, Laurent Vivier wrote:
>> On 02/12/2019 12:15, pannengy...@huawei.com wrote:
>>> From: PanNengyuan 
>>>
>>> ivqs/ovqs/c_ivq/c_ovq is forgot to cleanup in
>>> virtio_serial_device_unrealize, the memory leak stack is as bellow:
>>>
>>> Direct leak of 1290240 byte(s) in 180 object(s) allocated from:
>>> #0 0x7fc9bfc27560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
>>> #1 0x7fc9bed6f015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
>>> #2 0x5650e02b83e7 in virtio_add_queue 
>>> /mnt/sdb/qemu-4.2.0-rc0/hw/virtio/virtio.c:2327
>>> #3 0x5650e02847b5 in virtio_serial_device_realize 
>>> /mnt/sdb/qemu-4.2.0-rc0/hw/char/virtio-serial-bus.c:1089
>>> #4 0x5650e02b56a7 in virtio_device_realize 
>>> /mnt/sdb/qemu-4.2.0-rc0/hw/virtio/virtio.c:3504
>>> #5 0x5650e03bf031 in device_set_realized 
>>> /mnt/sdb/qemu-4.2.0-rc0/hw/core/qdev.c:876
>>> #6 0x5650e0531efd in property_set_bool 
>>> /mnt/sdb/qemu-4.2.0-rc0/qom/object.c:2080
>>> #7 0x5650e053650e in object_property_set_qobject 
>>> /mnt/sdb/qemu-4.2.0-rc0/qom/qom-qobject.c:26
>>> #8 0x5650e0533e14 in object_property_set_bool 
>>> /mnt/sdb/qemu-4.2.0-rc0/qom/object.c:1338
>>> #9 0x5650e04c0e37 in virtio_pci_realize 
>>> /mnt/sdb/qemu-4.2.0-rc0/hw/virtio/virtio-pci.c:1801
>>>
>>> Reported-by: Euler Robot 
>>> Signed-off-by: PanNengyuan 
>>> ---
>>>  hw/char/virtio-serial-bus.c | 6 ++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
>>> index 3325904..da9019a 100644
>>> --- a/hw/char/virtio-serial-bus.c
>>> +++ b/hw/char/virtio-serial-bus.c
>>> @@ -1126,9 +1126,15 @@ static void 
>>> virtio_serial_device_unrealize(DeviceState *dev, Error **errp)
>>>  {
>>>  VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>>  VirtIOSerial *vser = VIRTIO_SERIAL(dev);
>>> +int i;
>>>  
>>>  QLIST_REMOVE(vser, next);
>>>  
>>> +for (i = 0; i <= vser->bus.max_nr_ports; i++) {
>>> +virtio_del_queue(vdev, 2 * i);
>>> +virtio_del_queue(vdev, 2 * i + 1);
>>> +}
>>> +
>>
>> According to virtio_serial_device_realize() and the number of
>> virtio_add_queue(), I think you have more queues to delete:
>>
>>   4 + 2 * vser->bus.max_nr_ports
>>
>> (for vser->ivqs[0], vser->ovqs[0], vser->c_ivq, vser->c_ovq,
>> vser->ivqs[i], vser->ovqs[i]).
>>
>> Thanks,
>> Laurent
>>
>>
> Thanks, but I think the queues is correct, the queues in
> virtio_serial_device_realize is as follow:
> 
> // here is 2
> vser->ivqs[0] = virtio_add_queue(vdev, 128, handle_input);
> vser->ovqs[0] = virtio_add_queue(vdev, 128, handle_output);
> 
> // here is 2
> vser->c_ivq = virtio_add_queue(vdev, 32, control_in);
> vser->c_ovq = virtio_add_queue(vdev, 32, control_out);
> 
> // here 2 * (max_nr_ports - 1)  - i is from 1 to max_nr_ports - 1
> for (i = 1; i < vser->bus.max_nr_ports; i++) {
> vser->ivqs[i] = virtio_add_queue(vdev, 128, handle_input);
> vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output);
> }
> 
> so the total queues number is:  2 * (vser->bus.max_nr_ports + 1)
> 

Yes, you're right. A comment in the code would have helped or written
clearly like:

for (i = 0; i < 2 * (vser->bus.max_nr_ports + 1); i++) {
virtio_del_queue(vdev, i);
}

Thanks,
Laurent

Re: [PATCH 00/10] Add Allwinner H3 SoC and Orange Pi PC Machine

[Philippe Mathieu-Daudé]

On 12/2/19 10:09 PM, Niek Linnenbank wrote:

Dear QEMU developers,

Hereby I would like to contribute the following set of patches to QEMU
which add support for the Allwinner H3 System on Chip and the
Orange Pi PC machine. The following features and devices are supported:

  * SMP (Quad Core Cortex A7)
  * Generic Interrupt Controller configuration
  * SRAM mappings
  * Timer device (re-used from Allwinner A10)
  * UART
  * SD/MMC storage controller
  * EMAC ethernet connectivity
  * USB 2.0 interfaces
  * Clock Control Unit
  * System Control module
  * Security Identifier device


Awesome!


Functionality related to graphical output such as HDMI, GPU,
Display Engine and audio are not included. Recently released
mainline Linux kernels (4.19 up to latest master) and mainline U-Boot
are known to work. The SD/MMC code is tested using bonnie++ and
various tools such as fsck, dd and fdisk. The EMAC is verified with iperf3
using -netdev socket.

To build a Linux mainline kernel that can be booted by the Orange Pi PC
machine, simply configure the kernel using the sunxi_defconfig configuration:
  $ ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- make mrproper
  $ ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- make sunxi_defconfig

To be able to use USB storage, you need to manually enable the corresponding
configuration item. Start the kconfig configuration tool:
  $ ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- make menuconfig

Navigate to the following item, enable it and save your configuration:
  Device Drivers > USB support > USB Mass Storage support

Build the Linux kernel with:
  $ ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- make -j5

To boot the newly build linux kernel in QEMU with the Orange Pi PC machine, use:
  $ qemu-system-arm -M orangepi -m 512 -nic user -nographic \
  -kernel /path/to/linux/arch/arm/boot/zImage \
  -append 'console=ttyS0,115200' \
  -dtb /path/to/linux/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dtb

Note that this kernel does not have a root filesystem. You may provide it
with an official Orange Pi PC image [1] either as an SD card or as
USB mass storage. To boot using the Orange Pi PC Debian image on SD card,
simply add the -sd argument and provide the proper root= kernel parameter:
  $ qemu-system-arm -M orangepi -m 512 -nic user -nographic \
  -kernel /path/to/linux/arch/arm/boot/zImage \
  -append 'console=ttyS0,115200 root=/dev/mmcblk0p2' \
  -dtb /path/to/linux/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dtb \
  -sd OrangePi_pc_debian_stretch_server_linux5.3.5_v1.0.img

Alternatively, you can also choose to build and boot a recent buildroot [2]
using the orangepi_pc_defconfig or Armbian image [3] for Orange Pi PC.


Richard, trying the Armbian image from 
https://apt.armbian.com/pool/main/l/linux-4.20.7-sunxi/ I get:


$ arm-softmmu/qemu-system-arm -M orangepi -m 512 -nic user \
  -append 'console=ttyS0,115200' \
  -kernel boot/vmlinuz-4.20.7-sunxi \
  -dtb usr/lib/linux-image-dev-sunxi/sun8i-h3-orangepi-pc.dtb \
  -serial stdio -d unimp
Uncompressing Linux... done, booting the kernel.
rtc: unimplemented device write (size 4, value 0x16aa0001, offset 0x0)
rtc: unimplemented device read (size 4, offset 0x0)
rtc: unimplemented device read (size 4, offset 0x0)
rtc: unimplemented device read (size 4, offset 0x8)
qemu-system-arm: target/arm/helper.c:11359: cpu_get_tb_cpu_state: 
Assertion `flags == rebuild_hflags_internal(env)' failed.

Aborted (core dumped)

(gdb) bt
#0  0x7f6c1fa2ce35 in raise () at /lib64/libc.so.6
#1  0x7f6c1fa17895 in abort () at /lib64/libc.so.6
#2  0x7f6c1fa17769 in _nl_load_domain.cold () at /lib64/libc.so.6
#3  0x7f6c1fa25566 in annobin_assert.c_end () at /lib64/libc.so.6
#4  0x5590657e2685 in cpu_get_tb_cpu_state (env=0x5590686899b0, 
pc=0x7f6c07ffa718, cs_base=0x7f6c07ffa714, pflags=0x7f6c07ffa71c) at 
target/arm/helper.c:11359
#5  0x55906569f962 in tb_lookup__cpu_state (cpu=0x5590686808b0, 
pc=0x7f6c07ffa718, cs_base=0x7f6c07ffa714, flags=0x7f6c07ffa71c, 
cf_mask=524288) at include/exec/tb-lookup.h:28
#6  0x5590656a084c in tb_find (cpu=0x5590686808b0, last_tb=0x0, 
tb_exit=0, cf_mask=524288) at accel/tcg/cpu-exec.c:403
#7  0x5590656a114a in cpu_exec (cpu=0x5590686808b0) at 
accel/tcg/cpu-exec.c:730

#8  0x55906565f6af in tcg_cpu_exec (cpu=0x5590686808b0) at cpus.c:1473
#9  0x55906565ff05 in qemu_tcg_cpu_thread_fn (arg=0x5590686808b0) at 
cpus.c:1781
#10 0x559065d54aa6 in qemu_thread_start (args=0x5590687d8c20) at 
util/qemu-thread-posix.c:519

#11 0x7f6c1fbc54c0 in start_thread () at /lib64/libpthread.so.0
#12 0x7f6c1faf1553 in clone () at /lib64/libc.so.6

(gdb) p/x flags
$1 = 0x3360

(gdb) p/x *env
$2 = {regs = {0x0 , 0x40102448}, xregs = {0x0 32 times>}, pc = 0x0, pstate = 0x0, aarch64 = 0x0, hflags = 0x3360, 
uncached_cpsr = 0x1a, spsr = 0x0, banked_spsr = {0x0, 0x0, 0x0, 0x0, 
0x0, 0x0, 0x0, 0x0},
  banked_r13 = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, banked_r14 = 
{0x0, 0x0, 0x0, 0

Re: [PATCH] hw/ppc/prep: Remove the deprecated "prep" machine and the OpenHackware BIOS

[Philippe Mathieu-Daudé]

On 12/3/19 9:25 AM, Thomas Huth wrote:

On 03/12/2019 08.45, Philippe Mathieu-Daudé wrote:

On 12/3/19 8:29 AM, Thomas Huth wrote:

It's been deprecated since QEMU v3.1. The 40p machine should be
used nowadays instead.

Signed-off-by: Thomas Huth 
---
   .gitmodules    |   3 -
   MAINTAINERS    |   1 -
   Makefile   |   2 +-
   docs/interop/firmware.json |   3 +-
   hw/ppc/ppc.c   |  18 --
   hw/ppc/prep.c  | 384 +
   include/hw/ppc/ppc.h   |   1 -
   pc-bios/README |   3 -
   pc-bios/ppc_rom.bin    | Bin 1048576 -> 0 bytes
   qemu-deprecated.texi   |   6 -
   qemu-doc.texi  |  15 +-
   roms/openhackware  |   1 -
   tests/boot-order-test.c    |  25 ---
   tests/cdrom-test.c |   2 +-
   tests/endianness-test.c    |   2 +-
   15 files changed, 10 insertions(+), 456 deletions(-)
   delete mode 100644 pc-bios/ppc_rom.bin
   delete mode 16 roms/openhackware

[...]

diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index a725bce729..4a6218a516 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -108,30 +108,6 @@ static void test_pc_boot_order(void)
   test_boot_orders(NULL, read_boot_order_pc, test_cases_pc);
   }
   -static uint8_t read_m48t59(QTestState *qts, uint64_t addr, uint16_t
reg)
-{
-    qtest_writeb(qts, addr, reg & 0xff);
-    qtest_writeb(qts, addr + 1, reg >> 8);
-    return qtest_readb(qts, addr + 3);
-}
-
-static uint64_t read_boot_order_prep(QTestState *qts)
-{
-    return read_m48t59(qts, 0x8000 + 0x74, 0x34);


I'd rather keep this generic mmio-mapped ISA test.
Maybe run it with the 40p machine?


I don't think that this is possible in an easy way here. On the prep
machine, the ISA bus is on a hard-coded MMIO address. On the 40p
machine, the ISA bus is behind a PCI-to-ISA bridge, thus the PCI part
needs to be set up first.


The why ...


Maybe we can rename this as read_boot_order_mm, and the previous
read_boot_order_pc as read_boot_order_io.


I don't think it makes much sense. This was completely specific to the
"prep" machine, even the "40p" machine seems to prefer fw_cfg nowadays.
So let's simply remove this old stuff.

>
>> diff --git a/tests/endianness-test.c b/tests/endianness-test.c
>> index 58527952a5..2798802c63 100644
>> --- a/tests/endianness-test.c
>> +++ b/tests/endianness-test.c
>> @@ -35,7 +35,7 @@ static const TestCase test_cases[] = {
>>   { "mips64", "malta", 0x1000, .bswap = true },
>>   { "mips64el", "fulong2e", 0x1fd0 },
>>   { "ppc", "g3beige", 0xfe00, .bswap = true, .superio =
>> "i82378" },
>> -{ "ppc", "prep", 0x8000, .bswap = true },
>> +{ "ppc", "40p", 0x8000, .bswap = true },

... here you access the Super I/O behind the PCI bridge via MMIO?

>>   { "ppc", "bamboo", 0xe800, .bswap = true, .superio =
>> "i82378" },
>>   { "ppc64", "mac99", 0xf200, .bswap = true, .superio =
>> "i82378" },
>>   { "ppc64", "pseries", (1ULL << 45), .bswap = true, .superio =
>> "i82378" },
>>

Re: [PATCH 2/2] Add -mem-shared option

[Thomas Huth]

On 02/12/2019 22.00, Eduardo Habkost wrote:
> On Mon, Dec 02, 2019 at 08:39:48AM +0100, Igor Mammedov wrote:
>> On Fri, 29 Nov 2019 18:46:12 +0100
>> Paolo Bonzini  wrote:
>>
>>> On 29/11/19 13:16, Igor Mammedov wrote:
 As for "-m", I'd make it just an alias that translates
  -m/mem-path/mem-prealloc  
>>>
>>> I think we should just deprecate -mem-path/-mem-prealloc in 5.0.  CCing
>>> Thomas as mister deprecation. :)
>>
>> I'll add that to my series
> 
> Considering that the plan is to eventually reimplement those
> options as syntactic sugar for memory backend options (hopefully
> in less than 2 QEMU releases), what's the point of deprecating
> them?

Well, it depends on the "classification" [1] of the parameter...

Let's ask: What's the main purpose of the option?

Is it easier to use than the "full" option, and thus likely to be used
by a lot of people who run QEMU directly from the CLI? In that case it
should stay as "convenience option" and not be deprecated.

Or is the option merely there to give the upper layers like libvirt or
some few users and their scripts some more grace period to adapt their
code, but we all agree that the options are rather ugly and should
finally go away? Then it's rather a "legacy option" and the deprecation
process is the right way to go. Our QEMU interface is still way to
overcrowded, we should try to keep it as clean as possible.

 Thomas


[1] Using the terms from:
https://www.youtube.com/watch?v=Oscjpkns7tM&t=8m

Re: [PATCH 0/6] qapi: Module fixes and cleanups

[Markus Armbruster]

Markus Armbruster  writes:

> Kevin recently posted a minimally invasive fix for empty QAPI
> modules[*].  This is my attempt at a fix that also addresses the
> design weakness that led to the bug.

Queued for 5.0.

Re: [PATCH 00/10] Add Allwinner H3 SoC and Orange Pi PC Machine

[Philippe Mathieu-Daudé]

On 12/2/19 10:09 PM, Niek Linnenbank wrote:

Dear QEMU developers,

Hereby I would like to contribute the following set of patches to QEMU
which add support for the Allwinner H3 System on Chip and the
Orange Pi PC machine. The following features and devices are supported:

  * SMP (Quad Core Cortex A7)
  * Generic Interrupt Controller configuration
  * SRAM mappings
  * Timer device (re-used from Allwinner A10)
  * UART
  * SD/MMC storage controller
  * EMAC ethernet connectivity
  * USB 2.0 interfaces
  * Clock Control Unit
  * System Control module
  * Security Identifier device

Functionality related to graphical output such as HDMI, GPU,
Display Engine and audio are not included.


I'd love to see the OpenRISC AR100 core instantiated in this SoC.

Your contribution makes another good example of multi-arch/single-binary 
QEMU (here 4x ARM + 1x OpenRISC).

Re: [PATCH v3 0/5] hvf: stability fixes for HVF

[Paolo Bonzini]

On 03/12/19 00:55, Cameron Esfahani wrote:
> Changes in v3:
> - Change previous code which saved interrupt/exception type in
>   hvf_store_events() to inject later in hvf_inject_interrupts().
>   Now, hvf_inject_interrupts() will correctly determine when it's appropriate
>   to inject VMCS_INTR_T_HWINTR versus VMCS_INTR_T_SWINTR.  From feedback by
>   Paolo Bonzini to make code more similar to KVM model.

Queued, thanks.

Paolo

[Bug 1854910] [NEW] Support VHDX differencing images (ie images with backing)

[Mark Zealey]

Public bug reported:

The qemu vhdx driver does not support type 2 (differencing) vhdx images
(usually stored with file extension .avhdx). This means that any hyperv
images with snapshots are not supported by qemu-img. It would be great
to be able to convert to a new qcow image from a backing + set of
differencing images from hyperv, and/or convert an individual
differencing vhdx image to a qcow2 image with a backing file specified.

** Affects: qemu
 Importance: Undecided
 Status: New

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

Title:
  Support VHDX differencing images (ie images with backing)

Status in QEMU:
  New

Bug description:
  The qemu vhdx driver does not support type 2 (differencing) vhdx
  images (usually stored with file extension .avhdx). This means that
  any hyperv images with snapshots are not supported by qemu-img. It
  would be great to be able to convert to a new qcow image from a
  backing + set of differencing images from hyperv, and/or convert an
  individual differencing vhdx image to a qcow2 image with a backing
  file specified.

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

Re: Network connection with COLO VM

[Daniel Cho]

Hi Dave,

We could use the exist interface to add netfilter and chardev, it might not
have the problem you said.

However, the netfilter and chardev on the primary at the start, that means
we could not dynamic set COLO
feature to VM?

We try to change this chardev to prevent primary VM will stuck to wait
secondary VM.

-chardev socket,id=compare1,host=127.0.0.1,port=9004,server,wait \

to

-chardev socket,id=compare1,host=127.0.0.1,port=9004,server,nowait \

But it will make primary VM's network not works. (Can't get ip), until
starting connect with secondary VM.


Otherwise, the primary VM with netfileter / chardev and without netfilter /
chardev , they takes very different
booting time.
Without  netfilter / chardev : about 1 mins
With   netfilter / chardev : about 5 mins
Is this an issue?

Best regards,
Daniel Cho


Dr. David Alan Gilbert  於 2019年12月2日 週一 下午5:58寫道：

> * Daniel Cho (daniel...@qnap.com) wrote:
> > Hi Zhang,
> >
> > We use qemu-4.1.0 release on this case.
> >
> > I think we need use block mirror to sync the disk to secondary node
> first,
> > then stop the primary VM and build COLO system.
> >
> > In the stop moment, you need add some netfilter and chardev socket node
> for
> > COLO, maybe you need re-check this part.
> >
> >
> > Our test was already follow those step. Maybe I could describe the detail
> > of the test flow and issues.
> >
> >
> > Step 1:
> >
> > Create primary VM without any netfilter and chardev for COLO, and using
> > other host ping primary VM continually.
> >
> >
> > Step 2:
> >
> > Create secondary VM (the same device/drive with primary VM), and do block
> > mirror sync ( ping to primary VM normally )
> >
> >
> > Step 3:
> >
> > After block mirror sync finish, add those netfilter and chardev to
> primary
> > VM and secondary VM for COLO ( *Can't* ping to primary VM but those
> packets
> > will be received later )
> >
> >
> > Step 4:
> >
> > Start migrate primary VM to secondary VM, and primary VM & secondary VM
> are
> > running ( ping to primary VM works and receive those packets on step 3
> > status )
> >
> >
> >
> >
> > Between Step 3 to Step 4, it will take 10~20 seconds in our environment.
> >
> > I could image this issue (delay reply packets) is because of setting COLO
> > proxy for temporary status,
> >
> > but we thought 10~20 seconds might a little long. (If primary VM is
> already
> > doing some jobs, it might lose the data.)
> >
> >
> > Could we reduce those time? or those delay is depends on different VM?
>
> I think you need to set up the netfilter and chardev on the primary at
> the start;  the filter contains the state of the TCP connections working
> with the VM, so adding it later can't gain that state for existing
> connections.
>
> Dave
>
> >
> > Best Regard,
> >
> > Daniel Cho.
> >
> >
> >
> > Zhang, Chen  於 2019年11月30日 週六 上午2:04寫道：
> >
> > >
> > >
> > >
> > >
> > > *From:* Daniel Cho 
> > > *Sent:* Friday, November 29, 2019 10:43 AM
> > > *To:* Zhang, Chen 
> > > *Cc:* Dr. David Alan Gilbert ;
> lukasstra...@web.de;
> > > qemu-devel@nongnu.org
> > > *Subject:* Re: Network connection with COLO VM
> > >
> > >
> > >
> > > Hi David,  Zhang,
> > >
> > >
> > >
> > > Thanks for replying my question.
> > >
> > > We know why will occur this issue.
> > >
> > > As you said, the COLO VM's network needs
> > >
> > > colo-proxy to control packets, so the guest's
> > >
> > > interface should set the filter to solve the problem.
> > >
> > >
> > >
> > > But we found another question, when we set the
> > >
> > > fault-tolerance feature to guest (primary VM is running,
> > >
> > > secondary VM is pausing), the guest's network would not
> > >
> > > responds any request for a while (in our environment
> > >
> > > about 20~30 secs) after secondary VM runs.
> > >
> > >
> > >
> > > Does it be a normal situation, or a known issue?
> > >
> > >
> > >
> > > Our test is creating primary VM for a while, then creating
> > >
> > > secondary VM to make it with COLO feature.
> > >
> > >
> > >
> > > Hi Daniel,
> > >
> > >
> > >
> > > Happy to hear you have solved ssh disconnection issue.
> > >
> > >
> > >
> > > Do you use Lukas’s patch on this case?
> > >
> > > I think we need use block mirror to sync the disk to secondary node
> first,
> > > then stop the primary VM and build COLO system.
> > >
> > > In the stop moment, you need add some netfilter and chardev socket node
> > > for COLO, maybe you need re-check this part.
> > >
> > >
> > >
> > > Best Regard,
> > >
> > > Daniel Cho
> > >
> > >
> > >
> > > Zhang, Chen  於 2019年11月28日 週四 上午9:26寫道：
> > >
> > >
> > >
> > > > -Original Message-
> > > > From: Dr. David Alan Gilbert 
> > > > Sent: Wednesday, November 27, 2019 6:51 PM
> > > > To: Daniel Cho ; Zhang, Chen
> > > > ; lukasstra...@web.de
> > > > Cc: qemu-devel@nongnu.org
> > > > Subject: Re: Network connection with COLO VM
> > > >
> > > > * Daniel Cho (daniel...@qnap.com) wrote:
> > > > > Hello everyone,
> > > > >
> > > > > Could we ssh to colo VM (means PVM & SVM are star

Re: [PATCH 02/10] hw: arm: add Xunlong Orange Pi PC machine

[Philippe Mathieu-Daudé]

On 12/2/19 10:09 PM, Niek Linnenbank wrote:

The Xunlong Orange Pi PC is an Allwinner H3 System on Chip
based embedded computer with mainline support in both U-Boot
and Linux. The board comes with a Quad Core Cortex A7 @ 1.3GHz,
512MB RAM, 100Mbit ethernet, USB, SD/MMC, USB, HDMI and
various other I/O. This commit add support for the Xunlong
Orange Pi PC machine.

Signed-off-by: Niek Linnenbank 
---
  MAINTAINERS  |  1 +
  hw/arm/Makefile.objs |  2 +-
  hw/arm/orangepi.c| 90 
  3 files changed, 92 insertions(+), 1 deletion(-)
  create mode 100644 hw/arm/orangepi.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 29c9936037..42c913d6cb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -485,6 +485,7 @@ L: qemu-...@nongnu.org
  S: Maintained
  F: hw/*/allwinner-h3*
  F: include/hw/*/allwinner-h3*
+F: hw/arm/orangepi.c
  
  ARM PrimeCell and CMSDK devices

  M: Peter Maydell 
diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 956e496052..8d5ea453d5 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -34,7 +34,7 @@ obj-$(CONFIG_DIGIC) += digic.o
  obj-$(CONFIG_OMAP) += omap1.o omap2.o
  obj-$(CONFIG_STRONGARM) += strongarm.o
  obj-$(CONFIG_ALLWINNER_A10) += allwinner-a10.o cubieboard.o
-obj-$(CONFIG_ALLWINNER_H3) += allwinner-h3.o
+obj-$(CONFIG_ALLWINNER_H3) += allwinner-h3.o orangepi.o
  obj-$(CONFIG_RASPI) += bcm2835_peripherals.o bcm2836.o raspi.o
  obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
  obj-$(CONFIG_XLNX_ZYNQMP_ARM) += xlnx-zynqmp.o xlnx-zcu102.o
diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
new file mode 100644
index 00..5ef2735f81
--- /dev/null
+++ b/hw/arm/orangepi.c
@@ -0,0 +1,90 @@
+/*
+ * Orange Pi emulation
+ *
+ * Copyright (C) 2019 Niek Linnenbank 
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "exec/address-spaces.h"
+#include "qapi/error.h"
+#include "cpu.h"
+#include "hw/sysbus.h"
+#include "hw/boards.h"
+#include "hw/qdev-properties.h"
+#include "hw/arm/allwinner-h3.h"
+
+static struct arm_boot_info orangepi_binfo = {
+.loader_start = AW_H3_SDRAM_BASE,
+.board_id = -1,
+};
+
+typedef struct OrangePiState {
+AwH3State *h3;
+MemoryRegion sdram;
+} OrangePiState;
+
+static void orangepi_init(MachineState *machine)
+{
+OrangePiState *s = g_new(OrangePiState, 1);
+Error *err = NULL;
+


Here I'd add:

  if (strcmp(machine->cpu_type, ARM_CPU_TYPE_NAME("cortex-a7")) != 0) {
  error_report("This board can only be used with cortex-a7 CPU");
  exit(1);
  }


+s->h3 = AW_H3(object_new(TYPE_AW_H3));
+
+/* Setup timer properties */
+object_property_set_int(OBJECT(&s->h3->timer), 32768, "clk0-freq", &err);
+if (err != NULL) {
+error_reportf_err(err, "Couldn't set clk0 frequency: ");
+exit(1);
+}
+
+object_property_set_int(OBJECT(&s->h3->timer), 2400, "clk1-freq",
+&err);
+if (err != NULL) {
+error_reportf_err(err, "Couldn't set clk1 frequency: ");
+exit(1);
+}
+
+/* Mark H3 object realized */
+object_property_set_bool(OBJECT(s->h3), true, "realized", &err);


I'm not sure if that's correct but I'd simply use &error_abort here.


+if (err != NULL) {
+error_reportf_err(err, "Couldn't realize Allwinner H3: ");
+exit(1);
+}
+
+/* RAM */
+memory_region_allocate_system_memory(&s->sdram, NULL, "orangepi.ram",
+ machine->ram_size);


I'd only allow machine->ram_size == 1 * GiB here, since the onboard DRAM 
is not upgradable.



+memory_region_add_subregion(get_system_memory(), AW_H3_SDRAM_BASE,
+&s->sdram);
+
+/* Load target kernel */
+orangepi_binfo.ram_size = machine->ram_size;
+orangepi_binfo.nb_cpus  = AW_H3_NUM_CPUS;
+arm_load_kernel(ARM_CPU(first_cpu), machine, &orangepi_binfo);
+}
+
+static void orangepi_machine_init(MachineClass *mc)
+{
+mc->desc = "Orange Pi PC";
+mc->init = orangepi_init;
+mc->units_per_default_bus = 1;
+mc->min_cpus = AW_H3_NUM_CPUS;
+mc->max_cpus = AW_H3_NUM_CPUS;
+mc->default_cpus = AW_H3_NUM_CPUS;


   mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a7");


+mc->ignore_memory_transaction_failures = true;


You should not use this flag in new d

Re: [PATCH] hw/ppc/prep: Remove the deprecated "prep" machine and the OpenHackware BIOS

[Thomas Huth]

On 03/12/2019 09.51, Philippe Mathieu-Daudé wrote:
> On 12/3/19 9:25 AM, Thomas Huth wrote:
>> On 03/12/2019 08.45, Philippe Mathieu-Daudé wrote:
>>> On 12/3/19 8:29 AM, Thomas Huth wrote:
 It's been deprecated since QEMU v3.1. The 40p machine should be
 used nowadays instead.

 Signed-off-by: Thomas Huth 
 ---
    .gitmodules    |   3 -
    MAINTAINERS    |   1 -
    Makefile   |   2 +-
    docs/interop/firmware.json |   3 +-
    hw/ppc/ppc.c   |  18 --
    hw/ppc/prep.c  | 384
 +
    include/hw/ppc/ppc.h   |   1 -
    pc-bios/README |   3 -
    pc-bios/ppc_rom.bin    | Bin 1048576 -> 0 bytes
    qemu-deprecated.texi   |   6 -
    qemu-doc.texi  |  15 +-
    roms/openhackware  |   1 -
    tests/boot-order-test.c    |  25 ---
    tests/cdrom-test.c |   2 +-
    tests/endianness-test.c    |   2 +-
    15 files changed, 10 insertions(+), 456 deletions(-)
    delete mode 100644 pc-bios/ppc_rom.bin
    delete mode 16 roms/openhackware
>>> [...]
 diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
 index a725bce729..4a6218a516 100644
 --- a/tests/boot-order-test.c
 +++ b/tests/boot-order-test.c
 @@ -108,30 +108,6 @@ static void test_pc_boot_order(void)
    test_boot_orders(NULL, read_boot_order_pc, test_cases_pc);
    }
    -static uint8_t read_m48t59(QTestState *qts, uint64_t addr, uint16_t
 reg)
 -{
 -    qtest_writeb(qts, addr, reg & 0xff);
 -    qtest_writeb(qts, addr + 1, reg >> 8);
 -    return qtest_readb(qts, addr + 3);
 -}
 -
 -static uint64_t read_boot_order_prep(QTestState *qts)
 -{
 -    return read_m48t59(qts, 0x8000 + 0x74, 0x34);
>>>
>>> I'd rather keep this generic mmio-mapped ISA test.
>>> Maybe run it with the 40p machine?
>>
>> I don't think that this is possible in an easy way here. On the prep
>> machine, the ISA bus is on a hard-coded MMIO address. On the 40p
>> machine, the ISA bus is behind a PCI-to-ISA bridge, thus the PCI part
>> needs to be set up first.
> 
> The why ...

If you don't believe me, why don't you simply try to adapt the test on
your own to use the 40p machine instead?

>>> Maybe we can rename this as read_boot_order_mm, and the previous
>>> read_boot_order_pc as read_boot_order_io.
>>
>> I don't think it makes much sense. This was completely specific to the
>> "prep" machine, even the "40p" machine seems to prefer fw_cfg nowadays.
>> So let's simply remove this old stuff.
>>
>>> diff --git a/tests/endianness-test.c b/tests/endianness-test.c
>>> index 58527952a5..2798802c63 100644
>>> --- a/tests/endianness-test.c
>>> +++ b/tests/endianness-test.c
>>> @@ -35,7 +35,7 @@ static const TestCase test_cases[] = {
>>>   { "mips64", "malta", 0x1000, .bswap = true },
>>>   { "mips64el", "fulong2e", 0x1fd0 },
>>>   { "ppc", "g3beige", 0xfe00, .bswap = true, .superio =
>>> "i82378" },
>>> -    { "ppc", "prep", 0x8000, .bswap = true },
>>> +    { "ppc", "40p", 0x8000, .bswap = true },
> 
> ... here you access the Super I/O behind the PCI bridge via MMIO?

The difference is that this is an *arbitrary* address in I/O space
there. It's not an address of a certain PCI device like the m48t59
behind a PCI-bridge. As long as it's possible to write and read from
this address, the test is working. Both, the "prep" and the "40p"
machine have the "raven-pcihost" device at this address, so in this case
the switch from "40p" to "prep" was easily possible.

 Thomas

Re: [PATCH] hw/ppc/prep: Remove the deprecated "prep" machine and the OpenHackware BIOS

[Philippe Mathieu-Daudé]

On 12/3/19 10:15 AM, Thomas Huth wrote:

On 03/12/2019 09.51, Philippe Mathieu-Daudé wrote:

On 12/3/19 9:25 AM, Thomas Huth wrote:

On 03/12/2019 08.45, Philippe Mathieu-Daudé wrote:

On 12/3/19 8:29 AM, Thomas Huth wrote:

It's been deprecated since QEMU v3.1. The 40p machine should be
used nowadays instead.

Signed-off-by: Thomas Huth 
---
    .gitmodules    |   3 -
    MAINTAINERS    |   1 -
    Makefile   |   2 +-
    docs/interop/firmware.json |   3 +-
    hw/ppc/ppc.c   |  18 --
    hw/ppc/prep.c  | 384
+
    include/hw/ppc/ppc.h   |   1 -
    pc-bios/README |   3 -
    pc-bios/ppc_rom.bin    | Bin 1048576 -> 0 bytes
    qemu-deprecated.texi   |   6 -
    qemu-doc.texi  |  15 +-
    roms/openhackware  |   1 -
    tests/boot-order-test.c    |  25 ---
    tests/cdrom-test.c |   2 +-
    tests/endianness-test.c    |   2 +-
    15 files changed, 10 insertions(+), 456 deletions(-)
    delete mode 100644 pc-bios/ppc_rom.bin
    delete mode 16 roms/openhackware

[...]

diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index a725bce729..4a6218a516 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -108,30 +108,6 @@ static void test_pc_boot_order(void)
    test_boot_orders(NULL, read_boot_order_pc, test_cases_pc);
    }
    -static uint8_t read_m48t59(QTestState *qts, uint64_t addr, uint16_t
reg)
-{
-    qtest_writeb(qts, addr, reg & 0xff);
-    qtest_writeb(qts, addr + 1, reg >> 8);
-    return qtest_readb(qts, addr + 3);
-}
-
-static uint64_t read_boot_order_prep(QTestState *qts)
-{
-    return read_m48t59(qts, 0x8000 + 0x74, 0x34);


I'd rather keep this generic mmio-mapped ISA test.
Maybe run it with the 40p machine?


I don't think that this is possible in an easy way here. On the prep
machine, the ISA bus is on a hard-coded MMIO address. On the 40p
machine, the ISA bus is behind a PCI-to-ISA bridge, thus the PCI part
needs to be set up first.


The why ...




I meant "TheN why". The "..." were to continue the review comment below 
the endianness-test.c diff.



If you don't believe me, why don't you simply try to adapt the test on
your own to use the 40p machine instead?


I didn't meant to be rude, I'm sorry if you misunderstood me.


Maybe we can rename this as read_boot_order_mm, and the previous
read_boot_order_pc as read_boot_order_io.


I don't think it makes much sense. This was completely specific to the
"prep" machine, even the "40p" machine seems to prefer fw_cfg nowadays.
So let's simply remove this old stuff.


diff --git a/tests/endianness-test.c b/tests/endianness-test.c
index 58527952a5..2798802c63 100644
--- a/tests/endianness-test.c
+++ b/tests/endianness-test.c
@@ -35,7 +35,7 @@ static const TestCase test_cases[] = {
    { "mips64", "malta", 0x1000, .bswap = true },
    { "mips64el", "fulong2e", 0x1fd0 },
    { "ppc", "g3beige", 0xfe00, .bswap = true, .superio =
"i82378" },
-    { "ppc", "prep", 0x8000, .bswap = true },
+    { "ppc", "40p", 0x8000, .bswap = true },


... here you access the Super I/O behind the PCI bridge via MMIO?


The difference is that this is an *arbitrary* address in I/O space
there. It's not an address of a certain PCI device like the m48t59
behind a PCI-bridge. As long as it's possible to write and read from
this address, the test is working. Both, the "prep" and the "40p"
machine have the "raven-pcihost" device at this address, so in this case
the switch from "40p" to "prep" was easily possible.


Now I better understand what this test does, thanks.

Re: [PATCH 10/10] arm: allwinner-h3: add EMAC ethernet device

[KONRAD Frederic]

Le 12/2/19 à 10:09 PM, Niek Linnenbank a écrit :

The Allwinner H3 System on Chip includes an Ethernet MAC (EMAC)
which provides 10M/100M/1000M Ethernet connectivity. This commit
adds support for the Allwinner H3 EMAC, including emulation for
the following functionality:

  * DMA transfers
  * MII interface
  * Transmit CRC calculation

Signed-off-by: Niek Linnenbank 
---
  hw/arm/Kconfig |   1 +
  hw/arm/allwinner-h3.c  |  17 +
  hw/arm/orangepi.c  |   7 +
  hw/net/Kconfig |   3 +
  hw/net/Makefile.objs   |   1 +
  hw/net/allwinner-h3-emac.c | 786 +
  hw/net/trace-events|  10 +
  include/hw/arm/allwinner-h3.h  |   2 +
  include/hw/net/allwinner-h3-emac.h |  69 +++
  9 files changed, 896 insertions(+)
  create mode 100644 hw/net/allwinner-h3-emac.c
  create mode 100644 include/hw/net/allwinner-h3-emac.h

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index ebf8d2325f..551cff3442 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -294,6 +294,7 @@ config ALLWINNER_A10
  config ALLWINNER_H3
  bool
  select ALLWINNER_A10_PIT
+select ALLWINNER_H3_EMAC
  select SERIAL
  select ARM_TIMER
  select ARM_GIC
diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
index c2972caf88..274b8548c0 100644
--- a/hw/arm/allwinner-h3.c
+++ b/hw/arm/allwinner-h3.c
@@ -53,6 +53,9 @@ static void aw_h3_init(Object *obj)
  
  sysbus_init_child_obj(obj, "mmc0", &s->mmc0, sizeof(s->mmc0),

TYPE_AW_H3_SDHOST);
+
+sysbus_init_child_obj(obj, "emac", &s->emac, sizeof(s->emac),
+  TYPE_AW_H3_EMAC);
  }
  
  static void aw_h3_realize(DeviceState *dev, Error **errp)

@@ -237,6 +240,20 @@ static void aw_h3_realize(DeviceState *dev, Error **errp)
  return;
  }
  
+/* EMAC */

+if (nd_table[0].used) {
+qemu_check_nic_model(&nd_table[0], TYPE_AW_H3_EMAC);
+qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]);
+}
+object_property_set_bool(OBJECT(&s->emac), true, "realized", &err);
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
+sysbusdev = SYS_BUS_DEVICE(&s->emac);
+sysbus_mmio_map(sysbusdev, 0, AW_H3_EMAC_BASE);
+sysbus_connect_irq(sysbusdev, 0, s->irq[AW_H3_GIC_SPI_EMAC]);
+
  /* Universal Serial Bus */
  sysbus_create_simple(TYPE_AW_H3_EHCI, AW_H3_EHCI0_BASE,
   s->irq[AW_H3_GIC_SPI_EHCI0]);
diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
index dee3efaf08..8a61eb0e69 100644
--- a/hw/arm/orangepi.c
+++ b/hw/arm/orangepi.c
@@ -61,6 +61,13 @@ static void orangepi_init(MachineState *machine)
  exit(1);
  }
  
+/* Setup EMAC properties */

+object_property_set_int(OBJECT(&s->h3->emac), 1, "phy-addr", &err);
+if (err != NULL) {
+error_reportf_err(err, "Couldn't set phy address: ");
+exit(1);
+}
+
  /* Mark H3 object realized */
  object_property_set_bool(OBJECT(s->h3), true, "realized", &err);
  if (err != NULL) {
diff --git a/hw/net/Kconfig b/hw/net/Kconfig
index 3856417d42..36d3923992 100644
--- a/hw/net/Kconfig
+++ b/hw/net/Kconfig
@@ -74,6 +74,9 @@ config MIPSNET
  config ALLWINNER_EMAC
  bool
  
+config ALLWINNER_H3_EMAC

+bool
+
  config IMX_FEC
  bool
  
diff --git a/hw/net/Makefile.objs b/hw/net/Makefile.objs

index 7907d2c199..5548deb07a 100644
--- a/hw/net/Makefile.objs
+++ b/hw/net/Makefile.objs
@@ -23,6 +23,7 @@ common-obj-$(CONFIG_XGMAC) += xgmac.o
  common-obj-$(CONFIG_MIPSNET) += mipsnet.o
  common-obj-$(CONFIG_XILINX_AXI) += xilinx_axienet.o
  common-obj-$(CONFIG_ALLWINNER_EMAC) += allwinner_emac.o
+common-obj-$(CONFIG_ALLWINNER_H3_EMAC) += allwinner-h3-emac.o
  common-obj-$(CONFIG_IMX_FEC) += imx_fec.o
  
  common-obj-$(CONFIG_CADENCE) += cadence_gem.o

diff --git a/hw/net/allwinner-h3-emac.c b/hw/net/allwinner-h3-emac.c
new file mode 100644
index 00..37f6f44406
--- /dev/null
+++ b/hw/net/allwinner-h3-emac.c
@@ -0,0 +1,786 @@
+/*
+ * Allwinner H3 EMAC emulation
+ *
+ * Copyright (C) 2019 Niek Linnenbank 
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .
+ */
+
+#include "qemu/osdep.h"
+#include "hw/sysbus.h"
+#include "migration/vmstate.h"
+#include "net/net.h"
+#include "hw/irq.h"
+#include "hw/qdev-prope

Re: [PATCH v6] 9pfs: well form error hint helpers

[Greg Kurz]

On Mon, 2 Dec 2019 09:36:21 +
Vladimir Sementsov-Ogievskiy  wrote:

> 28.11.2019 1:37, Greg Kurz wrote:
> > On Wed, 27 Nov 2019 22:15:49 +0300
> > Vladimir Sementsov-Ogievskiy  wrote:
> > 
> >> Make error_append_security_model_hint and
> >> error_append_socket_sockfd_hint hint append helpers well formed:
> >> rename errp to errp_in, as it is IN-parameter here (which is unusual
> >> for errp).
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> >> Acked-by: Greg Kurz 
> >> ---
> >>
> >> v6: add Greg's a-b
> >>
> > 
> > I've already pushed that to my 9p-next branch:
> > 
> > https://github.com/gkurz/qemu/commits/9p-next
> 
> 
> Markus proposed to use Error *const *errp for such cases (among other
> things)
> 
> So, seems there would be v7, the current version is here:
> git://repo.or.cz/qemu/armbru.git branch error-prep
> 
> Discussion is here:
> https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg04633.html
> and here:
> https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg04636.html
> 

Ok, this makes sense. It's cleaner and it addresses your concern
with coccinelle, so I've dropped the patch from my 9p-next branch.

> > 
> > 
> >>   hw/9pfs/9p-local.c | 4 ++--
> >>   hw/9pfs/9p-proxy.c | 5 +++--
> >>   2 files changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> >> index 4708c0bd89..76fa1858b7 100644
> >> --- a/hw/9pfs/9p-local.c
> >> +++ b/hw/9pfs/9p-local.c
> >> @@ -1473,9 +1473,9 @@ static void local_cleanup(FsContext *ctx)
> >>   g_free(data);
> >>   }
> >>   
> >> -static void error_append_security_model_hint(Error **errp)
> >> +static void error_append_security_model_hint(Error **errp_in)
> >>   {
> >> -error_append_hint(errp, "Valid options are: security_model="
> >> +error_append_hint(errp_in, "Valid options are: security_model="
> >> "[passthrough|mapped-xattr|mapped-file|none]\n");
> >>   }
> >>   
> >> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> >> index 97ab9c58a5..9e29abc3ef 100644
> >> --- a/hw/9pfs/9p-proxy.c
> >> +++ b/hw/9pfs/9p-proxy.c
> >> @@ -1114,9 +1114,10 @@ static int connect_namedsocket(const char *path, 
> >> Error **errp)
> >>   return sockfd;
> >>   }
> >>   
> >> -static void error_append_socket_sockfd_hint(Error **errp)
> >> +static void error_append_socket_sockfd_hint(Error **errp_in)
> >>   {
> >> -error_append_hint(errp, "Either specify socket=/some/path where 
> >> /some/path"
> >> +error_append_hint(errp_in,
> >> +  "Either specify socket=/some/path where /some/path"
> >> " points to a listening AF_UNIX socket or 
> >> sock_fd=fd"
> >> " where fd is a file descriptor to a connected 
> >> AF_UNIX"
> >> " socket\n");
> > 
> 
> 

Re: [PATCH 17/21] s390x: Fix latent query-cpu-model-FOO error handling bugs

[David Hildenbrand]

>> s390x: Fix query-cpu-model-FOO error API violations
>>
>> cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(),
>> qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
>> dereferences @errp when the visitor or the QOM setter fails.  That's
>> wrong; see the big comment in error.h.  Introduced in commit
>> 137974cea3 's390x/cpumodel: implement QMP interface
>> "query-cpu-model-expansion"'.
>>
>> Its three callers have the same issue.  Introduced in commit
>> 4e82ef0502 's390x/cpumodel: implement QMP interface
>> "query-cpu-model-comparison"' and commit f1a47d08ef 's390x/cpumodel:
>> implement QMP interface "query-cpu-model-baseline"'.
>>
>> No caller actually passes null.  To fix, splice in a local Error *err,
>> and error_propagate().
>>
>> Cc: David Hildenbrand 
>> Cc: Cornelia Huck 
>> Signed-off-by: Markus Armbruster 
> 
> That sounds good to me.

Yes, something I would have enjoyed reading from the first sentence.

Reviewed-by: David Hildenbrand 

-- 
Thanks,

David / dhildenb

Re: [PATCH v37 00/17] QEMU AVR 8 bit cores

[Michael Rolnik]

Aleksandar.

enjoy your vacation.

Regards,
Michael Rolnik

On Tue, Dec 3, 2019 at 3:48 AM Aleksandar Markovic <
aleksandar.m.m...@gmail.com> wrote:

>
>
> On Tuesday, December 3, 2019, Aleksandar Markovic <
> aleksandar.m.m...@gmail.com> wrote:
>
>>
>>
>> On Tuesday, December 3, 2019, Aleksandar Markovic <
>> aleksandar.m.m...@gmail.com> wrote:
>>
>>>
>>>
>>> On Monday, December 2, 2019, Aleksandar Markovic <
>>> aleksandar.m.m...@gmail.com> wrote:
>>>


 On Monday, December 2, 2019, Michael Rolnik  wrote:

> how can I get this elf flags from within QEMU?
>
>>
>>
 In one of files from your "machine" patch, you have this snippet:

 +bytes_loaded = load_elf(
 +filename, NULL, NULL, NULL, NULL, NULL, NULL, 0, EM_NONE,
 0, 0);

 With this line you a kind of "blindly" load whatever you find in the
 file "filename". I think you need to modify load_elf() to fetch the
 information on what core the elf in question is compiled for. Additionally,
 you most likely have to check if the elf file is compiled for AVR at all.

 I don't know enough about AVR-specifics of ELF format, but I know that
 we in MIPS read successfuly some MIPS-specific things we need to know. Do
 some research for ELF format headrr content for AVR.

 This is really missing in your series, seriously.

 Please keep in mind that I don't have right now at hand any dev system,
 so all I said here is off of my head.

 You have to do some code digging.


>>> First, you need to update
>>>
>>> https://github.com/qemu/qemu/blob/master/include/elf.h
>>>
>>> with bits and pieces for AVR.
>>>
>>> In binutils file:
>>>
>>> https://github.com/bminor/binutils-gdb/blob/master/include/elf/common.h
>>>
>>> you will spot the line:
>>>
>>> #define EM_AVR 83 /* Atmel AVR 8-bit microcontroller */
>>>
>>> that is the value of e_machine field for AVR, which you need to insert
>>> in qemu's include/elf.h about at line 162.
>>>
>>> Then, in another binutils file:
>>>
>>> https://github.com/bminor/binutils-gdb/blob/master/include/elf/avr.h
>>>
>>> you find the lines:
>>>
>>> #define E_AVR_MACH_AVR1 1
>>> #define E_AVR_MACH_AVR2 2
>>> #define E_AVR_MACH_AVR25 25
>>> #define E_AVR_MACH_AVR3 3
>>> #define E_AVR_MACH_AVR31 31
>>> #define E_AVR_MACH_AVR35 35
>>> #define E_AVR_MACH_AVR4 4
>>> #define E_AVR_MACH_AVR5 5
>>> #define E_AVR_MACH_AVR51 51
>>> #define E_AVR_MACH_AVR6 6
>>> #define E_AVR_MACH_AVRTINY 100
>>> #define E_AVR_MACH_XMEGA1 101
>>> #define E_AVR_MACH_XMEGA2 102
>>> #define E_AVR_MACH_XMEGA3 103
>>> #define E_AVR_MACH_XMEGA4 104
>>> #define E_AVR_MACH_XMEGA5 105
>>> #define E_AVR_MACH_XMEGA6 106
>>> #define E_AVR_MACH_XMEGA7 107
>>>
>>> That you also need to insert in qemu's include/elf.h, probably at the
>>> end of tge foke or elsewhere.
>>>
>>> Perhaps something more you need to insert into that file, you'll see.
>>>
>>> Than, you need to modify the file where load_elf() resides with AVR
>>> support, take a look at other architectures' support, and adjust to what
>>> you need.
>>>
>>> I know it will be contrieved at times, but, personally, similar ELF
>>> support must be done for any upcoming platform. Only if there is some
>>> unsourmantable obstacle, that support can be omitted.
>>>
>>> I am on vacation next 10 days.
>>>
>>>
>> In the source of readelf utility:
>>
>>
>> static void
>> decode_AVR_machine_flags (unsigned e_flags, char buf[], size_t size)
>> {
>>   --size; /* Leave space for null terminator.  */
>>
>>   switch (e_flags & EF_AVR_MACH)
>> {
>> case E_AVR_MACH_AVR1:
>>   strncat (buf, ", avr:1", size);
>>   break;
>> case E_AVR_MACH_AVR2:
>>   strncat (buf, ", avr:2", size);
>>   break;
>> case E_AVR_MACH_AVR25:
>>   strncat (buf, ", avr:25", size);
>>   break;
>> case E_AVR_MACH_AVR3:
>>   strncat (buf, ", avr:3", size);
>>   break;
>> case E_AVR_MACH_AVR31:
>>   strncat (buf, ", avr:31", size);
>>   break;
>> case E_AVR_MACH_AVR35:
>>   strncat (buf, ", avr:35", size);
>>   break;
>> case E_AVR_MACH_AVR4:
>>   strncat (buf, ", avr:4", size);
>>   break;
>> case E_AVR_MACH_AVR5:
>>   strncat (buf, ", avr:5", size);
>>   break;
>> case E_AVR_MACH_AVR51:
>>   strncat (buf, ", avr:51", size);
>>   break;
>> case E_AVR_MACH_AVR6:
>>   strncat (buf, ", avr:6", size);
>>   break;
>> case E_AVR_MACH_AVRTINY:
>>   strncat (buf, ", avr:100", size);
>>   break;
>> case E_AVR_MACH_XMEGA1:
>>   strncat (buf, ", avr:101", size);
>>   break;
>> case E_AVR_MACH_XMEGA2:
>>   strncat (buf, ", avr:102", size);
>>   break;
>> case E_AVR_MACH_XMEGA3:
>>   strncat (buf, ", avr:103", size);
>>   break;
>> case E_AVR_MACH_XMEGA4:
>>   strncat (buf, ", avr:104", size);
>>   break;
>> case E_AVR_MACH_XMEGA5:
>>   strncat (buf, ", avr:105", s

Re: [PATCH v2 3/3] s390x: Fix cpu normal reset ri clearing

[Christian Borntraeger]

On 02.12.19 15:01, Janosch Frank wrote:
> As it turns out we need to clear the ri controls and PSW enablement
> bit to be architecture compliant.
> 
> Signed-off-by: Janosch Frank 
> ---
>  target/s390x/cpu.c | 5 +
>  target/s390x/cpu.h | 7 ++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 906285888e..c192e6b3b9 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -131,6 +131,11 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type 
> type)
>&env->fpu_status);
> /* fall through */

As this is a fall through, do we want to change the INITIAL RESET to only clear 
up to 
start_normal_reset_fields, e.g. something like this on top

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 829ce6ad5491..58ac721687a9 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -108,7 +108,7 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
 case S390_CPU_RESET_INITIAL:
 /* initial reset does not clear everything! */
 memset(&env->start_initial_reset_fields, 0,
-   offsetof(CPUS390XState, end_reset_fields) -
+   offsetof(CPUS390XState, start_normal_reset_fields) -
offsetof(CPUS390XState, start_initial_reset_fields));
 
 /* architectured initial value for Breaking-Event-Address register */


to avoid double memsetting. 


Other than this question
Reviewed-by: Christian Borntraeger 


>  case S390_CPU_RESET_NORMAL:
> +env->psw.mask &= ~PSW_MASK_RI;
> +memset(&env->start_normal_reset_fields, 0,
> +   offsetof(CPUS390XState, end_reset_fields) -
> +   offsetof(CPUS390XState, start_normal_reset_fields));
> +
>  env->pfault_token = -1UL;
>  env->bpbc = false;
>  break;
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index d5e18b096e..7f5fa1d35b 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -58,7 +58,6 @@ struct CPUS390XState {
>   */
>  uint64_t vregs[32][2] QEMU_ALIGNED(16);  /* vector registers */
>  uint32_t aregs[16];/* access registers */
> -uint8_t riccb[64]; /* runtime instrumentation control */
>  uint64_t gscb[4];  /* guarded storage control */
>  uint64_t etoken;   /* etoken */
>  uint64_t etoken_extension; /* etoken extension */
> @@ -114,6 +113,10 @@ struct CPUS390XState {
>  uint64_t gbea;
>  uint64_t pp;
>  
> +/* Fields up to this point are not cleared by normal CPU reset */
> +struct {} start_normal_reset_fields;
> +uint8_t riccb[64]; /* runtime instrumentation control */
> +
>  /* Fields up to this point are cleared by a CPU reset */
>  struct {} end_reset_fields;
>  
> @@ -252,6 +255,7 @@ extern const VMStateDescription vmstate_s390_cpu;
>  #undef PSW_SHIFT_ASC
>  #undef PSW_MASK_CC
>  #undef PSW_MASK_PM
> +#undef PSW_MASK_RI
>  #undef PSW_SHIFT_MASK_PM
>  #undef PSW_MASK_64
>  #undef PSW_MASK_32
> @@ -274,6 +278,7 @@ extern const VMStateDescription vmstate_s390_cpu;
>  #define PSW_MASK_CC 0x3000ULL
>  #define PSW_MASK_PM 0x0F00ULL
>  #define PSW_SHIFT_MASK_PM   40
> +#define PSW_MASK_RI 0x0080ULL
>  #define PSW_MASK_64 0x0001ULL
>  #define PSW_MASK_32 0x8000ULL
>  #define PSW_MASK_ESA_ADDR   0x7fffULL
> 

Re: [PATCH] virtiofs: Relax DAX window protection for ppc

[Dr. David Alan Gilbert]

* Fabiano Rosas (faro...@linux.ibm.com) wrote:
> When setting up the DAX window during the virtiofs driver probe inside
> the guest, the Linux arch-specific function arch_add_memory is called,
> which on ppc tries to do a cache flush [1] of the recently added
> memory. At this point the window is mmap'ed PROT_NONE by QEMU, which
> causes the following:
> 
> 
> [   10.136703] virtio_fs virtio0: Cache len: 0x8000 @ 0x2100
> [   10.165106] radix-mmu: Mapped 0xc0002100-0xc00021008000 with 
> 1.00 GiB pages
> error: kvm run failed Bad address
> NIP c0072350   LR c0072304 CTR 0100 XER 
> 2004 CPU#0
> MSR 82009033 HID0   HF 8000 iidx 3 didx 3
> TB   DECR 0
> GPR00 c0072304 c000fa383100 c1190300 
> GPR04 0001  c000fa383208 0080
> GPR08 c0002100 807f 0100 6874697720303030
> 
> 
> The problem is the same for the memory device removal path
> (e.g. during filesystem unmount).
> 
> Since powerpc expects the memory to be accessible during device
> addition/removal, this patch makes the DAX window readable at creation
> and after virtio-fs unmap.
> 
> 1 - 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fb5924fd
> 
> Signed-off-by: Fabiano Rosas 

Reviewed-by: Dr. David Alan Gilbert 

Thanks, I'll do a pull after the tree opens again.

Dave

> ---
>  hw/virtio/vhost-user-fs.c | 17 ++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index 455e97beea..92958797d0 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -24,6 +24,16 @@
>  #include "exec/address-spaces.h"
>  #include "trace.h"
>  
> +/*
> + * The powerpc kernel code expects the memory to be accessible during
> + * addition/removal.
> + */
> +#if defined(TARGET_PPC64) && defined(CONFIG_LINUX)
> +#define DAX_WINDOW_PROT PROT_READ
> +#else
> +#define DAX_WINDOW_PROT PROT_NONE
> +#endif
> +
>  uint64_t vhost_user_fs_slave_map(struct vhost_dev *dev, VhostUserFSSlaveMsg 
> *sm,
>   int fd)
>  {
> @@ -133,8 +143,8 @@ uint64_t vhost_user_fs_slave_unmap(struct vhost_dev *dev, 
> VhostUserFSSlaveMsg *s
>  continue;
>  }
>  
> -ptr = mmap(cache_host + sm->c_offset[i], sm->len[i],
> -PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
> +ptr = mmap(cache_host + sm->c_offset[i], sm->len[i], DAX_WINDOW_PROT,
> +   MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
>  if (ptr != (cache_host + sm->c_offset[i])) {
>  res = -errno;
>  fprintf(stderr, "%s: mmap failed (%s) [%d] %"
> @@ -485,8 +495,9 @@ static void vuf_device_realize(DeviceState *dev, Error 
> **errp)
>  
>  if (fs->conf.cache_size) {
>  /* Anonymous, private memory is not counted as overcommit */
> -cache_ptr = mmap(NULL, fs->conf.cache_size, PROT_NONE,
> +cache_ptr = mmap(NULL, fs->conf.cache_size, DAX_WINDOW_PROT,
>   MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> +
>  if (cache_ptr == MAP_FAILED) {
>  error_setg(errp, "Unable to mmap blank cache");
>  return;
> -- 
> 2.23.0
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH 1/3] iotests: Provide a function for checking the creation of huge files

[Alex Bennée]

Thomas Huth  writes:

> Some tests create huge (but sparse) files, and to be able to run those
> tests in certain limited environments (like CI containers), we have to
> check for the possibility to create such files first. Thus let's introduce
> a common function to check for large files, and replace the already
> existing checks in the iotests 005 and 220 with this function.
>
> Signed-off-by: Thomas Huth 

Reviewed-by: Alex Bennée 

> ---
>  tests/qemu-iotests/005   |  5 +
>  tests/qemu-iotests/220   |  6 ++
>  tests/qemu-iotests/common.rc | 10 ++
>  3 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/tests/qemu-iotests/005 b/tests/qemu-iotests/005
> index 58442762fe..b6d03ac37d 100755
> --- a/tests/qemu-iotests/005
> +++ b/tests/qemu-iotests/005
> @@ -59,10 +59,7 @@ fi
>  # Sanity check: For raw, we require a file system that permits the creation
>  # of a HUGE (but very sparse) file. Check we can create it before continuing.
>  if [ "$IMGFMT" = "raw" ]; then
> -if ! truncate --size=5T "$TEST_IMG"; then
> -_notrun "file system on $TEST_DIR does not support large enough 
> files"
> -fi
> -rm "$TEST_IMG"
> +_require_large_file 5T
>  fi
>  
>  echo
> diff --git a/tests/qemu-iotests/220 b/tests/qemu-iotests/220
> index 2d62c5dcac..15159270d3 100755
> --- a/tests/qemu-iotests/220
> +++ b/tests/qemu-iotests/220
> @@ -42,10 +42,8 @@ echo "== Creating huge file =="
>  
>  # Sanity check: We require a file system that permits the creation
>  # of a HUGE (but very sparse) file.  tmpfs works, ext4 does not.
> -if ! truncate --size=513T "$TEST_IMG"; then
> -_notrun "file system on $TEST_DIR does not support large enough files"
> -fi
> -rm "$TEST_IMG"
> +_require_large_file 513T
> +
>  IMGOPTS='cluster_size=2M,refcount_bits=1' _make_test_img 513T
>  
>  echo "== Populating refcounts =="
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 38e949cf69..91c0217e59 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -657,5 +657,15 @@ _require_devices()
>  done
>  }
>  
> +# Check that we have a file system that allows huge (but very sparse) files
> +#
> +_require_large_file()
> +{
> +if ! truncate --size="$1" "$TEST_IMG"; then
> +_notrun "file system on $TEST_DIR does not support large enough 
> files"
> +fi
> +rm "$TEST_IMG"
> +}
> +
>  # make sure this script returns success
>  true


-- 
Alex Bennée

Re: [PATCH 0/3] iotests: Check for the possibility to create large files

[Alex Bennée]

Thomas Huth  writes:

> Travis recently added the possibility to test on ppc64le, arm64 and s390x
> hosts, too. However, the containers are very restricted there and do not
> allow the creation of large files, so that the tests 060 and 079 are
> currently failing there. So let's add some proper checks to these tests
> first.

These look good to me, do you want them to go via my testing/next or are
Kevin and Max going to take it via their tree?

>
> Thomas Huth (3):
>   iotests: Provide a function for checking the creation of huge files
>   iotests: Skip test 060 if it is not possible to create large files
>   iotests: Skip test 079 if it is not possible to create large files
>
>  tests/qemu-iotests/005   |  5 +
>  tests/qemu-iotests/060   |  3 +++
>  tests/qemu-iotests/079   |  3 +++
>  tests/qemu-iotests/220   |  6 ++
>  tests/qemu-iotests/common.rc | 10 ++
>  5 files changed, 19 insertions(+), 8 deletions(-)


-- 
Alex Bennée

Re: [PATCH v2 3/3] s390x: Fix cpu normal reset ri clearing

[David Hildenbrand]

On 02.12.19 15:01, Janosch Frank wrote:
> As it turns out we need to clear the ri controls and PSW enablement
> bit to be architecture compliant.
> 
> Signed-off-by: Janosch Frank 
> ---
>  target/s390x/cpu.c | 5 +
>  target/s390x/cpu.h | 7 ++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 906285888e..c192e6b3b9 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -131,6 +131,11 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type 
> type)
>&env->fpu_status);
> /* fall through */
>  case S390_CPU_RESET_NORMAL:
> +env->psw.mask &= ~PSW_MASK_RI;
> +memset(&env->start_normal_reset_fields, 0,
> +   offsetof(CPUS390XState, end_reset_fields) -
> +   offsetof(CPUS390XState, start_normal_reset_fields));
> +
>  env->pfault_token = -1UL;
>  env->bpbc = false;
>  break;
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index d5e18b096e..7f5fa1d35b 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -58,7 +58,6 @@ struct CPUS390XState {
>   */
>  uint64_t vregs[32][2] QEMU_ALIGNED(16);  /* vector registers */
>  uint32_t aregs[16];/* access registers */
> -uint8_t riccb[64]; /* runtime instrumentation control */
>  uint64_t gscb[4];  /* guarded storage control */
>  uint64_t etoken;   /* etoken */
>  uint64_t etoken_extension; /* etoken extension */
> @@ -114,6 +113,10 @@ struct CPUS390XState {
>  uint64_t gbea;
>  uint64_t pp;
>  
> +/* Fields up to this point are not cleared by normal CPU reset */
> +struct {} start_normal_reset_fields;
> +uint8_t riccb[64]; /* runtime instrumentation control */
> +
>  /* Fields up to this point are cleared by a CPU reset */
>  struct {} end_reset_fields;
>  
> @@ -252,6 +255,7 @@ extern const VMStateDescription vmstate_s390_cpu;
>  #undef PSW_SHIFT_ASC
>  #undef PSW_MASK_CC
>  #undef PSW_MASK_PM
> +#undef PSW_MASK_RI
>  #undef PSW_SHIFT_MASK_PM
>  #undef PSW_MASK_64
>  #undef PSW_MASK_32
> @@ -274,6 +278,7 @@ extern const VMStateDescription vmstate_s390_cpu;
>  #define PSW_MASK_CC 0x3000ULL
>  #define PSW_MASK_PM 0x0F00ULL
>  #define PSW_SHIFT_MASK_PM   40
> +#define PSW_MASK_RI 0x0080ULL
>  #define PSW_MASK_64 0x0001ULL
>  #define PSW_MASK_32 0x8000ULL
>  #define PSW_MASK_ESA_ADDR   0x7fffULL
> 

I'm afraid I can't help because the documentation is confidential. It
does look sane to me, at least.

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 2/2] travis.yml: Run tcg tests with tci

[Alex Bennée]

Thomas Huth  writes:

> On 28/11/2019 22.06, Stefan Weil wrote:
>> Am 28.11.19 um 16:35 schrieb Thomas Huth:
>> 
>>> So far we only have compile coverage for tci. But since commit
>>> 2f160e0f9797c7522bfd0d09218d0c9340a5137c ("tci: Add implementation
>>> for INDEX_op_ld16u_i64") has been included now, we can also run the
>>> "tcg" and "qtest" tests with tci, so let's enable them in Travis now.
>>> Since we don't gain much additional test coverage by compiling all
>>> targets, and TCI is broken e.g. with the Sparc targets, we also limit
>> 
>> 
>> As far as I know it is broken with Sparc hosts (not Sparc targets).
>
> It was definitely hanging with sparc64-softmmu:
>
>  https://travis-ci.com/huth/qemu/jobs/261335163
>
> ... but since you've mentioned that it should work with the 32-bit
> sparc-softmmu, I'll give it another try with sparc-softmmu.
>
>>> the target list to a reasonable subset now (which should still get
>>> us test coverage by tests/boot-serial-test for example).
>>>
>>> Signed-off-by: Thomas Huth 
>>> ---
>>>  .travis.yml | 7 ---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/.travis.yml b/.travis.yml
>>> index c09b6a0014..de7559e777 100644
>>> --- a/.travis.yml
>>> +++ b/.travis.yml
>>> @@ -215,10 +215,11 @@ matrix:
>>>  - TEST_CMD=""
>>>  
>>>  
>>> -# We manually include builds which we disable "make check" for
>>> +# Check the TCG interpreter (TCI)
>>>  - env:
>>> -- CONFIG="--enable-debug --enable-tcg-interpreter"
>>> -- TEST_CMD=""
>>> +- CONFIG="--enable-debug --enable-tcg-interpreter 
>>> --disable-containers
>> 
>> 
>> You could also --disable-kvm. It should not be needed, and disabling it
>> might need less build resources.
>
> Good idea. KVM is not usable by default in Travis, so we should not
> accidentally use it for the tests that specify "accel=kvm:tcg", but in
> case that changes with a future version of the environment, we should
> maybe be prepared.

Makes sense, I'll wait for v3 before applying to me tree.

>
>>> +
>>> --target-list=alpha-softmmu,arm-softmmu,hppa-softmmu,m68k-softmmu,microblaze-softmmu,moxie-softmmu,ppc-softmmu,s390x-softmmu,x86_64-softmmu"
>>> +- TEST_CMD="make check-qtest check-tcg V=1"
>>>  
>>>  
>>>  # We don't need to exercise every backend with every front-end
>> 
>> 
>> Thank you for adding these tests.
>> 
>> Tested-by: Stefan Weil 
>
>  Thanks,
>   Thomas


-- 
Alex Bennée

Re: [PATCH] virtfs-proxy-helper: switch from libcap to libcap-ng

[Greg Kurz]

On Fri, 29 Nov 2019 13:59:37 +0100
Greg Kurz  wrote:

> On Fri, 29 Nov 2019 13:49:20 +0100
> Paolo Bonzini  wrote:
> 
> > On 29/11/19 13:32, Greg Kurz wrote:
> > > Nice. :)
> > > 
> > > Reviewed-by: Greg Kurz 
> > > 
> > > Paolo,
> > > 
> > > I can take this through my 9p tree if you want. Otherwise,
> > > 
> > > Acked-by: Greg Kurz 
> > 
> > Yes, please do it since it's self-contained.  You'd probably also test
> > it better than me. :)
> > 
> > Paolo
> > 
> 
> Ok I'll just do that then.
> 

And it happens to be missing an extra-change in Makefile :)

-fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap
+fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap-ng

I've fixed this up in my tree.

> Cheers,
> 
> --
> Greg

Re: [PATCH 1/2] usb-host: remove 'remote wakeup' flag from configuration descriptor

[Gerd Hoffmann]

  Hi,

> +/* If this is GET_DESCRIPTOR request for configuration descriptor,
> + * remove 'remote wakeup' flag from it to prevent idle power down
> + * in Windows guest */

scripts/checkpatch.pl complains about that, please fix (and also the
other checkpatch warnings).

> +if (s->suppress_remote_wake &&
> +udev->setup_buf[0] == USB_DIR_IN &&
> +udev->setup_buf[1] == USB_REQ_GET_DESCRIPTOR &&
> +udev->setup_buf[3] == USB_DT_CONFIG && udev->setup_buf[2] == 0 &&
> +xfer->actual_length > offsetof(struct libusb_config_descriptor, 
> bmAttributes) &&
> +(conf->bmAttributes & USB_CFG_ATT_WAKEUP)) {
> +struct libusb_device_descriptor desc;
> +libusb_get_device_descriptor(s->dev, &desc);
> +trace_usb_host_remote_wakeup_removed(desc.idVendor, 
> desc.idProduct);

Please use s->bus_num and s->addr to identify the device, like all the
other trace points do.  I don't think there is a need to log
desc.idVendor and desc.idProduct here.


Otherwise the patch looks fine.

cheers,
  Gerd

Re: [PATCH 00/15] s390x: Protected Virtualization support

[Daniel P . Berrangé]

On Fri, Nov 29, 2019 at 03:02:41PM +0100, Janosch Frank wrote:
> On 11/29/19 1:35 PM, Daniel P. Berrangé wrote:

> > Is there any way to prevent a guest from using protected mode even
> > if QEMU supports it ?  eg the mgmt app may want to be able to
> > guarantee that all VMs are migratable, so don't want a guest OS
> > secretly activating protected mode which blocks migration.
> 
> Not enabling facility 161 is enough.

Is this facility enabled by default in any scenario ?

What happens if the feature is enabled & QEMU is also
coinfigured to use huge pages or does not have memory
pinned into RAM, given that those features are said to
be incompatible ?

> 
> > 
> >> Such VMs are started like any other VM and run a short "normal" stub
> >> that will prepare some things and then requests to be protected.
> >>
> >> Most of the restrictions are memory related and might be lifted in the
> >> future:
> >> * No paging
> >> * No migration
> > 
> > Presumably QEMU is going to set a migration blocker when a guest
> > activates protected mode ?
> 
> Well, that's stuff I still need to figure out :)
> 
> > 
> >> * No huge page backings
> >> * No collaborative memory management

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

Re: [PATCH 2/2] usb-redir: remove 'remote wakeup' flag from configuration descriptor

[Gerd Hoffmann]

  Hi,

> +/*
> + * If this is GET_DESCRIPTOR request for configuration descriptor,
> + * remove 'remote wakeup' flag from it to prevent idle power down
> + * in Windows guest
> + */
> +if (dev->suppress_remote_wake &&
> +control_packet->requesttype == USB_DIR_IN &&
> +control_packet->request == USB_REQ_GET_DESCRIPTOR &&
> +control_packet->value == (USB_DT_CONFIG << 8) &&
> +control_packet->index == 0 &&
> +/* bmAttributes field of config descriptor */
> +len > 7 && (dev->dev.data_buf[7] & USB_CFG_ATT_WAKEUP)) {
> +DPRINTF("Removed remote wake %04X:%04X\n",
> +dev->device_info.vendor_id,
> +dev->device_info.product_id);
> +dev->dev.data_buf[7] &= ~USB_CFG_ATT_WAKEUP;
> +}

Hmm, not much opportunity to factor out stuff to share with usb-host.
Ok then.

I think checkpatch has complains for this too, otherwise it looks fine.

cheers,
  Gerd

Re: virtiofsd: Where should it live?

[Dr. David Alan Gilbert]

We seem to be coming to the conclusion something that:

  a) It should live in the qemu tree
  b) It shouldn't live under contrib
  c) We'll create a new top level, i.e. 'daemons'
  d) virtiofsd will be daemons/virtiofsd

Now, somethings I'm less clear on:
  e) What else would move into daemons?  It was suggested
that if we've got virtiofsd in there, then we should move
libvhost-user - which I understand, but then it's not a
'daemons'.
Are there any otehr daemons that should move?

  f) Should virtiofsd always be built (if the deps are installed)?

Dave

--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH v5 01/13] add device_legacy_reset function to prepare for reset api change

[Cornelia Huck]

On Fri, 18 Oct 2019 17:06:18 +0200
Damien Hedde  wrote:

> Provide a temporary device_legacy_reset function doing what
> device_reset does to prepare for the transition with Resettable
> API.
> 
> All occurrence of device_reset in the code tree are also replaced
> by device_legacy_reset.
> 
> The new resettable API has different prototype and semantics
> (resetting child buses as well as the specified device). Subsequent
> commits will make the changeover for each call site individually; once
> that is complete device_legacy_reset() will be removed.
> 
> Signed-off-by: Damien Hedde 
> Reviewed-by: Peter Maydell 
> Acked-by: David Gibson 
> ---
> Cc: Gerd Hoffmann 
> Cc: Paolo Bonzini 
> Cc: "Daniel P. Berrangé" 
> Cc: Eduardo Habkost 
> Cc: Richard Henderson 
> Cc: "Michael S. Tsirkin" 
> Cc: Marcel Apfelbaum 
> Cc: John Snow 
> Cc: "Cédric Le Goater" 
> Cc: Collin Walling 
> Cc: Cornelia Huck 
> Cc: David Hildenbrand 
> Cc: Halil Pasic 
> Cc: Christian Borntraeger 
> Cc: Dmitry Fleytman 
> Cc: Fam Zheng 
> ---
>  hw/audio/intel-hda.c | 2 +-
>  hw/core/qdev.c   | 6 +++---
>  hw/hyperv/hyperv.c   | 2 +-
>  hw/i386/pc.c | 2 +-
>  hw/ide/microdrive.c  | 8 
>  hw/intc/spapr_xive.c | 2 +-
>  hw/ppc/pnv_psi.c | 2 +-
>  hw/ppc/spapr_pci.c   | 2 +-
>  hw/ppc/spapr_vio.c   | 2 +-
>  hw/s390x/s390-pci-inst.c | 2 +-
>  hw/scsi/vmw_pvscsi.c | 2 +-
>  hw/sd/omap_mmc.c | 2 +-
>  hw/sd/pl181.c| 2 +-
>  include/hw/qdev-core.h   | 4 ++--
>  14 files changed, 20 insertions(+), 20 deletions(-)

Acked-by: Cornelia Huck 

Re: [PULL 0/1] HVF fix QEMU 4.2-rc

[Peter Maydell]

On Tue, 3 Dec 2019 at 08:16, Paolo Bonzini  wrote:
>
> The following changes since commit 39032981fa851d25fb27527f25f046fed800e585:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-error-2019-12-02' 
> into staging (2019-12-02 16:29:41 +)
>
> are available in the Git repository at:
>
>   git://github.com/bonzini/qemu.git tags/for-upstream
>
> for you to fetch changes up to 64bef038e777208e4c35beae7f980fbd994b87eb:
>
>   hvf: correctly inject VMCS_INTR_T_HWINTR versus VMCS_INTR_T_SWINTR. 
> (2019-12-03 09:11:42 +0100)
>
> 
> * last HVF fix (Cameron)
>
> 
> Cameron Esfahani (1):
>   hvf: correctly inject VMCS_INTR_T_HWINTR versus VMCS_INTR_T_SWINTR.
>
>  target/i386/hvf/hvf.c|  4 +++-
>  target/i386/hvf/x86hvf.c | 14 +-
>  2 files changed, 12 insertions(+), 6 deletions(-)


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.2
for any user-visible changes.

-- PMM

Re: virtiofsd: Where should it live?

[Peter Maydell]

On Tue, 3 Dec 2019 at 10:53, Dr. David Alan Gilbert  wrote:
>
> We seem to be coming to the conclusion something that:
>
>   a) It should live in the qemu tree
>   b) It shouldn't live under contrib
>   c) We'll create a new top level, i.e. 'daemons'
>   d) virtiofsd will be daemons/virtiofsd
>
> Now, somethings I'm less clear on:
>   e) What else would move into daemons?  It was suggested
> that if we've got virtiofsd in there, then we should move
> libvhost-user - which I understand, but then it's not a
> 'daemons'.
> Are there any otehr daemons that should move?

I like the idea of a new top level directory, but I think
'daemons' is a bit too specific -- for instance it seems to
me that qemu-img would be sensible to move out of the root,
and that's not a daemon.

thanks
-- PMM

Re: [PATCH v5 02/13] hw/core/qdev: add trace events to help with resettable transition

[Cornelia Huck]

On Fri, 18 Oct 2019 17:06:19 +0200
Damien Hedde  wrote:

> Adds trace events to reset procedure and when updating the parent
> bus of a device.
> 
> Signed-off-by: Damien Hedde 
> ---
>  hw/core/qdev.c   | 27 ---
>  hw/core/trace-events |  9 +
>  2 files changed, 33 insertions(+), 3 deletions(-)

Reviewed-by: Cornelia Huck 

Re: [PATCH v2 04/14] gdbstub: move mem_buf to GDBState and use GByteArray

[Damien Hedde]

On 11/30/19 9:45 AM, Alex Bennée wrote:
> This is in preparation for further re-factoring of the register API
> with the rest of the code. Theoretically the read register function
> could overwrite the MAX_PACKET_LENGTH buffer although currently all
> registers are well within the size range.
> 
> Signed-off-by: Alex Bennée 
> Reviewed-by: Richard Henderson 
> ---
>  gdbstub.c | 62 ++-
>  1 file changed, 38 insertions(+), 24 deletions(-)
> 
> @@ -2003,7 +2015,7 @@ static void handle_query_curr_tid(GdbCmdContext 
> *gdb_ctx, void *user_ctx)
>  cpu = get_first_cpu_in_process(process);
>  g_string_assign(gdbserver_state.str_buf, "QC");
>  gdb_append_thread_id(cpu, gdbserver_state.str_buf);
> -put_strbuf();;
> +put_strbuf();
Hi Alex,

The double ';' (and the two other occurrences below) is added by your
previous patch.

>  }
>  
>  static void handle_query_threads(GdbCmdContext *gdb_ctx, void *user_ctx)
> @@ -2015,7 +2027,7 @@ static void handle_query_threads(GdbCmdContext 
> *gdb_ctx, void *user_ctx)
>  
>  g_string_assign(gdbserver_state.str_buf, "m");
>  gdb_append_thread_id(gdbserver_state.query_cpu, gdbserver_state.str_buf);
> -put_strbuf();;
> +put_strbuf();
>  gdbserver_state.query_cpu = 
> gdb_next_attached_cpu(gdbserver_state.query_cpu);
>  }
>  
> @@ -2058,7 +2070,7 @@ static void handle_query_thread_extra(GdbCmdContext 
> *gdb_ctx, void *user_ctx)
>  }
>  trace_gdbstub_op_extra_info(rs->str);
>  memtohex(gdbserver_state.str_buf, (uint8_t *)rs->str, rs->len);
> -put_strbuf();;
> +put_strbuf();
>  }
>   

With the ";;" fix
Reviewed/Tested-by: Damien Hedde 

--
Damien

Re: [PATCH v5 02/22] target/arm: Add regime_has_2_ranges

[Peter Maydell]

On Fri, 11 Oct 2019 at 14:48, Richard Henderson
 wrote:
>
> A translation with 2 ranges has both positive and negative addresses.
> This is true for the EL1&0 and the as-yet unimplemented EL2&0 regimes.
>
> Signed-off-by: Richard Henderson 
> ---
>  target/arm/internals.h | 14 ++
>  target/arm/helper.c| 22 +-
>  target/arm/translate-a64.c |  3 +--
>  3 files changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index dcc5d6cca3..9486680b87 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -804,6 +804,20 @@ static inline void arm_call_el_change_hook(ARMCPU *cpu)
>  }
>  }
>
> +/* Return true if this address translation regime has two ranges.  */
> +static inline bool regime_has_2_ranges(ARMMMUIdx mmu_idx)
> +{
> +switch (mmu_idx) {
> +case ARMMMUIdx_S12NSE0:
> +case ARMMMUIdx_S12NSE1:
> +case ARMMMUIdx_S1NSE0:
> +case ARMMMUIdx_S1NSE1:
> +return true;

Don't S1SE0 and S1SE1 also need to be here?

> +default:
> +return false;
> +}
> +}
> +
>  /* Return true if this address translation regime is secure */
>  static inline bool regime_is_secure(CPUARMState *env, ARMMMUIdx mmu_idx)
>  {
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index b690eda136..f9dee51ede 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8774,15 +8774,8 @@ static int get_S1prot(CPUARMState *env, ARMMMUIdx 
> mmu_idx, bool is_aa64,
>  }
>
>  if (is_aa64) {
> -switch (regime_el(env, mmu_idx)) {
> -case 1:
> -if (!is_user) {
> -xn = pxn || (user_rw & PAGE_WRITE);
> -}
> -break;
> -case 2:
> -case 3:
> -break;
> +if (regime_has_2_ranges(mmu_idx) && !is_user) {
> +xn = pxn || (user_rw & PAGE_WRITE);
>  }

(I was sceptical that 'regime_has_2_ranges()' was the right condition
here, but the Arm ARM really does define it as "valid only when stage
1 of the translation regime can support two VA ranges".)

>  } else if (arm_feature(env, ARM_FEATURE_V7)) {
>  switch (regime_el(env, mmu_idx)) {
> @@ -9316,7 +9309,6 @@ ARMVAParameters aa64_va_parameters_both(CPUARMState 
> *env, uint64_t va,
>  ARMMMUIdx mmu_idx)
>  {
>  uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
> -uint32_t el = regime_el(env, mmu_idx);
>  bool tbi, tbid, epd, hpd, tcma, using16k, using64k;
>  int select, tsz;
>
> @@ -9326,7 +9318,7 @@ ARMVAParameters aa64_va_parameters_both(CPUARMState 
> *env, uint64_t va,
>   */
>  select = extract64(va, 55, 1);
>
> -if (el > 1) {
> +if (!regime_has_2_ranges(mmu_idx)) {
>  tsz = extract32(tcr, 0, 6);
>  using64k = extract32(tcr, 14, 1);
>  using16k = extract32(tcr, 15, 1);
> @@ -9486,10 +9478,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>  param = aa64_va_parameters(env, address, mmu_idx,
> access_type != MMU_INST_FETCH);
>  level = 0;
> -/* If we are in 64-bit EL2 or EL3 then there is no TTBR1, so mark it
> - * invalid.
> - */
> -ttbr1_valid = (el < 2);
> +ttbr1_valid = regime_has_2_ranges(mmu_idx);
>  addrsize = 64 - 8 * param.tbi;
>  inputsize = 64 - param.tsz;
>  } else {
> @@ -11095,8 +11084,7 @@ void cpu_get_tb_cpu_state(CPUARMState *env, 
> target_ulong *pc,
>  ARMVAParameters p0 = aa64_va_parameters_both(env, 0, stage1);
>  int tbii;
>
> -/* FIXME: ARMv8.1-VHE S2 translation regime.  */
> -if (regime_el(env, stage1) < 2) {
> +if (regime_has_2_ranges(mmu_idx)) {

Now that the rebuild_hflags patchset has landed this is in
rebuild_hflags_a64().

>  ARMVAParameters p1 = aa64_va_parameters_both(env, -1, 
> stage1);
>  tbid = (p1.tbi << 1) | p0.tbi;
>  tbii = tbid & ~((p1.tbid << 1) | p0.tbid);
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 51f3af9cd9..c85db69db4 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -175,8 +175,7 @@ static void gen_top_byte_ignore(DisasContext *s, TCGv_i64 
> dst,
>  if (tbi == 0) {
>  /* Load unmodified address */
>  tcg_gen_mov_i64(dst, src);
> -} else if (s->current_el >= 2) {
> -/* FIXME: ARMv8.1-VHE S2 translation regime.  */
> +} else if (!regime_has_2_ranges(s->mmu_idx)) {
>  /* Force tag byte to all zero */
>  tcg_gen_extract_i64(dst, src, 0, 56);
>  } else {

The comment above this function also needs updating to no longer
refer to "EL2 and EL3" vs "EL0 and EL1". (You might also remove
the use of the imperial 'We' in the last sentence in it ;-))

Otherwise
Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: virtiofsd: Where should it live?

[Dr. David Alan Gilbert]

* Peter Maydell (peter.mayd...@linaro.org) wrote:
> On Tue, 3 Dec 2019 at 10:53, Dr. David Alan Gilbert  
> wrote:
> >
> > We seem to be coming to the conclusion something that:
> >
> >   a) It should live in the qemu tree
> >   b) It shouldn't live under contrib
> >   c) We'll create a new top level, i.e. 'daemons'
> >   d) virtiofsd will be daemons/virtiofsd
> >
> > Now, somethings I'm less clear on:
> >   e) What else would move into daemons?  It was suggested
> > that if we've got virtiofsd in there, then we should move
> > libvhost-user - which I understand, but then it's not a
> > 'daemons'.
> > Are there any otehr daemons that should move?
> 
> I like the idea of a new top level directory, but I think
> 'daemons' is a bit too specific -- for instance it seems to
> me that qemu-img would be sensible to move out of the root,
> and that's not a daemon.

What would your preference be?

Thomas was suggesting 'tools'.

Dave

> thanks
> -- PMM
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH 1/2] exec: flush CPU TB cache in breakpoint_invalidate

[Alex Bennée]

Richard Henderson  writes:

> On 11/27/19 10:06 PM, Max Filippov wrote:
>> When a breakpoint is inserted at location for which there's currently no
>> virtual to physical translation no action is taken on CPU TB cache. If a
>> TB for that virtual address already exists but is not visible ATM the
>> breakpoint won't be hit next time an instruction at that address will be
>> executed.
>> 
>> Flush entire CPU TB cache in breakpoint_invalidate to force
>> re-translation of all TBs for the breakpoint address.
>> 
>> This change fixes the following scenario:
>> - linux user application is running
>> - a breakpoint is inserted from QEMU gdbstub for a user address that is
>>   not currently present in the target CPU TLB
>> - an instruction at that address is executed, but the external debugger
>>   doesn't get control.
>> 
>> Signed-off-by: Max Filippov 
>> ---
>> Changes RFC->v1:
>> - do tb_flush in breakpoint_invalidate unconditionally
>
> Unlike Paolo, I don't think this is a good idea.

We previously had a general tb_flush during the MTTCG implementation as
a temporary fix. It was changed back in 406bc339b0 and it would be nice
to minimise the flushing of code if we can. While most interactive users
aren't going to notice the temporary slow down it would suck for any
automated gdb scripting.

>
> If I was going to change anything here, I'd change this to not use
> cpu_get_phys_page_attrs_debug but using the caching available from the actual
> cputlb, using cc->tlb_fill() in probe mode -- something akin to 
> probe_access(),
> but not returning a host address, nor handling watchpoints nor notdirty.
>
> This would help flushing too much by distinguishing different tbs for the same
> virtual address mapping to a different physical address.
>
>
> r~


-- 
Alex Bennée

Re: [PATCH v37 10/17] target/avr: Add instruction disassembly function

[Philippe Mathieu-Daudé]

On 12/2/19 8:04 AM, Michael Rolnik wrote:

Aleksandar.

If this code is going to be merge in 2019 I should modify al the 
copyrights, right. or should I put 2020 in?


Usually the copyright date is when you first contributed your code to 
the world (here, the list). If a patch was on the list in 2018, even if 
you made modifications and repost it, (c) is 2018.


IOW, If your series gets merged in 2020, it will be merged as (c) 2019.

I'm not sure why Richard's (c) appears here, is target/avr/disas.c based 
on target/openrisc/disas.c? Then it looks correct to me, but IANAL.


On Mon, Dec 2, 2019 at 2:28 AM Aleksandar Markovic 
mailto:aleksandar.m.m...@gmail.com>> wrote:




On Wednesday, November 27, 2019, Michael Rolnik mailto:mrol...@gmail.com>> wrote:

Provide function disassembles executed instruction when `-d
in_asm` is
provided

Example:
`./avr-softmmu/qemu-system-avr -bios
free-rtos/Demo/AVR_ATMega2560_GCC/demo.elf -d in_asm` will
produce something like the following

```
     ...
     IN:
     0x014a:  CALL      0x3808

     IN: main
     0x3808:  CALL      0x4b4

     IN: vParTestInitialise
     0x04b4:  LDI       r24, 255
     0x04b6:  STS       r24, 0
     0x04b8:  MULS      r16, r20
     0x04ba:  OUT       $1, r24
     0x04bc:  LDS       r24, 0
     0x04be:  MULS      r16, r20
     0x04c0:  OUT       $2, r24
     0x04c2:  RET
     ...
```

Signed-off-by: Michael Rolnik mailto:mrol...@gmail.com>>
Suggested-by: Richard Henderson mailto:richard.hender...@linaro.org>>
Suggested-by: Philippe Mathieu-Daudé mailto:phi...@redhat.com>>
Suggested-by: Aleksandar Markovic mailto:aleksandar.m.m...@gmail.com>>
Reviewed-by: Philippe Mathieu-Daudé mailto:phi...@redhat.com>>
Tested-by: Philippe Mathieu-Daudé mailto:phi...@redhat.com>>
---
  target/avr/cpu.h       |   1 +
  target/avr/cpu.c       |   2 +-
  target/avr/disas.c     | 228
+
  target/avr/translate.c |  11 ++
  4 files changed, 241 insertions(+), 1 deletion(-)
  create mode 100644 target/avr/disas.c

diff --git a/target/avr/cpu.h b/target/avr/cpu.h
index 9ea5260165..a3e615a1eb 100644
--- a/target/avr/cpu.h
+++ b/target/avr/cpu.h
@@ -157,6 +157,7 @@ bool avr_cpu_exec_interrupt(CPUState *cpu,
int int_req);
  hwaddr avr_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
  int avr_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int
reg);
  int avr_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf,
int reg);
+int avr_print_insn(bfd_vma addr, disassemble_info *info);

  static inline int avr_feature(CPUAVRState *env, int feature)
  {
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index dae56d7845..52ec21dd16 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -83,7 +83,7 @@ static void avr_cpu_reset(CPUState *cs)
  static void avr_cpu_disas_set_info(CPUState *cpu,
disassemble_info *info)
  {
      info->mach = bfd_arch_avr;
-    info->print_insn = NULL;
+    info->print_insn = avr_print_insn;
  }

  static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
diff --git a/target/avr/disas.c b/target/avr/disas.c
new file mode 100644
index 00..a51ade7c2a
--- /dev/null
+++ b/target/avr/disas.c
@@ -0,0 +1,228 @@
+/*
+ * AVR disassembler
+ *
+ * Copyright (c) 2018 Richard Henderson mailto:r...@twiddle.net>>


Just a detail: since this file is created in 2019, the copyright
year should be 2019 too.

+ * Copyright (c) 2019 Michael Rolnik mailto:mrol...@gmail.com>>
+ *
+ * This program is free software: you can redistribute it
and/or modify
+ * it under the terms of the GNU General Public License as
published by
+ * the Free Software Foundation, either version 2 of the
License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
License
+ * along with this program.  If not, see
.
+ */

[...]

Re: [PATCH v2 05/15] stubs: add stubs for io_uring interface

[Stefan Hajnoczi]

On Mon, Nov 11, 2019 at 12:13:47PM +0100, Kevin Wolf wrote:
> Am 25.10.2019 um 18:04 hat Stefan Hajnoczi geschrieben:
> > From: Aarushi Mehta 
> > 
> > Signed-off-by: Aarushi Mehta 
> > Signed-off-by: Stefan Hajnoczi 
> 
> This commit message needs to answer at least where these stubs are
> actually used. Aren't all callers protected with #ifdef
> CONFIG_LINUX_IO_URING? (And if they aren't, why is abort() okay?)

Okay, I'll clarify in the commit description.

I didn't find a program that actually requires these stubs today, but in
theory they are required when:
1. A program links against util-obj-y but not block-obj-y (e.g.
   vhost-user-gpu)
AND
2. It uses util/async.o, which depends on luring_*() APIs

You can test this by adding a call to qemu_bh_new() into
vhost-user-gpu's main.c:

  ld: libqemuutil.a(async.o): in function `aio_ctx_finalize':
  qemu/util/async.c:281: undefined reference to `luring_detach_aio_context'
  ld: /home/stefanha/qemu/util/async.c:282: undefined reference to 
`luring_cleanup'
  ld: libqemuutil.a(async.o): in function `aio_setup_linux_io_uring':
  qemu/util/async.c:358: undefined reference to `luring_init'
  ld: /home/stefanha/qemu/util/async.c:363: undefined reference to 
`luring_attach_aio_context'

The program may have no intention of using io_uring, just the QEMU main
loop and BH, so there is an argument for avoiding linking block-obj-y*.
That's the purpose of the stubs and why abort() is okay.

* It's possible to argue against this and personally I'm not that
convinced by stubs for this scenario.  But io_uring.o should be
consistent with how things work today with linux-aio.o.  If you feel
strongly against having stubs then the linux-aio.o stubs should also be
removed (see commit c2b38b277a788).

Stefan


signature.asc
Description: PGP signature

Re: [PATCH v37 12/17] target/avr: Add example board configuration

[Philippe Mathieu-Daudé]

On 11/30/19 5:57 PM, Michael Rolnik wrote:

Hi Aleksandar.

I guess no testing would be possible.

My previous series of patches (about 2 years ago, before Sarah joined) 
did not contain any peripherals, there were only the CPU and the sample 
board, I used it to test instructions.
Somebody complained that there are no peripherals and that was where I 
stopped to care as I did (and still don't) want to model devices.


This is why I introduced Joaquin to your series, as he expressed 
interest in modeling the AVR devices :)


If he ever step in, you could be listed as maintainer of the CPU/TCG 
section, and he could take the HW section. We'll see...


I believed that others would join and add devices. then two years passed 
by, Sarah implemented the timer and the UART devices.

And here we are.

Regards,
Michael Rolnik

On Sat, Nov 30, 2019 at 12:49 PM Aleksandar Markovic 
mailto:aleksandar.m.m...@gmail.com>> wrote:




On Wednesday, November 27, 2019, Michael Rolnik mailto:mrol...@gmail.com>> wrote:

A simple board setup that configures an AVR CPU to run a given
firmware image.
This is all that's useful to implement without peripheral
emulation as AVR CPUs include a lot of on-board peripherals.

NOTE: this is not a real board 
NOTE: it's used for CPU testing

Signed-off-by: Michael Rolnik mailto:mrol...@gmail.com>>
Reviewed-by: Aleksandar Markovic mailto:amarko...@wavecomp.com>>
Nacked-by: Philippe Mathieu-Daudé mailto:phi...@redhat.com>>
---
  hw/avr/sample.c      | 282
+++
  hw/Kconfig           |   1 +
  hw/avr/Kconfig       |   6 +
  hw/avr/Makefile.objs |   1 +
  4 files changed, 290 insertions(+)
  create mode 100644 hw/avr/sample.c
  create mode 100644 hw/avr/Kconfig
  create mode 100644 hw/avr/Makefile.objs


Michael, hi.

I just need a clarification here:

- What will happen if this patch is removed? Would boot and Avocado
tests work? What else in general wouldn't work or be available? What
was, in fact, the ultimate motivation for you to insert this patch?


Avocado tests won't work (the FreeRTOS tests require the timer/uart).

The instruction-tests should work using 'qemu-avr-softmmu -M none -m 9' 
with optional '-cpu $CPU'.


By using '-m 9' the 'none' machine maps 9MB of RAM at base address 
0x., until 0x008f.. So the SRAM area from the Mega2560 MCU, 
which is normally mapped at 0x0080.0200, is accessible by the tests.



Thanks,
Aleksandar

diff --git a/hw/avr/sample.c b/hw/avr/sample.c
new file mode 100644
index 00..2295ec1b79
--- /dev/null
+++ b/hw/avr/sample.c
@@ -0,0 +1,282 @@
+/*
+ * QEMU AVR CPU
+ *
+ * Copyright (c) 2019 Michael Rolnik
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later
version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see
+ * 
+ */
+
+/*
+ *  NOTE:
+ *      This is not a real AVR board, this is an example!
+ *      The CPU is an approximation of an ATmega2560, but is
missing various
+ *      built-in peripherals.
+ *
+ *      This example board loads provided binary file into
flash memory and
+ *      executes it from 0x address in the code memory
space.
+ *
+ *      Currently used for AVR CPU validation
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu-common.h"
+#include "cpu.h"
+#include "hw/hw.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/qtest.h"
+#include "ui/console.h"
+#include "hw/boards.h"
+#include "hw/loader.h"
+#include "qemu/error-report.h"
+#include "exec/address-spaces.h"
+#include "include/hw/sysbus.h"
+#include "include/hw/char/avr_usart.h"
+#include "include/hw/timer/avr_timer16.h"
+#include "include/hw/misc/avr_mask.h"
+#include "elf.h"
+#include "hw/misc/unimp.h"
+
+#define SIZE_FLA

Re: [PATCH v5 03/13] hw/core: create Resettable QOM interface

[Cornelia Huck]

On Fri, 18 Oct 2019 17:06:20 +0200
Damien Hedde  wrote:

> This commit defines an interface allowing multi-phase reset. This aims
> to solve a problem of the actual single-phase reset (built in
> DeviceClass and BusClass): reset behavior is dependent on the order
> in which reset handlers are called. In particular doing external
> side-effect (like setting an qemu_irq) is problematic because receiving
> object may not be reset yet.
> 
> The Resettable interface divides the reset in 3 well defined phases.
> To reset an object tree, all 1st phases are executed then all 2nd then
> all 3rd. See the comments in include/hw/resettable.h for a more complete
> description. The interface defines 3 phases to let the future
> possibility of holding an object into reset for some time.
> 
> The qdev/qbus reset in DeviceClass and BusClass will be modified in
> following commits to use this interface. A mechanism is provided
> to allow executing a transitional reset handler in place of the 2nd
> phase which is executed in children-then-parent order inside a tree.
> This will allow to transition devices and buses smoothly while
> keeping the exact current qdev/qbus reset behavior for now.
> 
> Documentation will be added in a following commit.
> 
> Signed-off-by: Damien Hedde 
> ---
> 
> In this patch only a single reset type is supported, but the interface
> allows for more to be defined.
> 
> I had some thought about problems which may arise when having multiple
> reset types:
> 
> - reset type propagation. Right now we propagate the same reset type
>   to the children. I don't think it will work that with multiple
>   types.
>   For example, if we add pci_bus_reset type: a pci device will
>   implement the reset type but not its children (they may have
>   nothing to do with pci).
>   This can be solved by changing the child_foreach method rules.
>   We should say that child_foreach may change the type it
>   propagates to its children (on a children by children basis).
>   For example, the pci device may just propagate cold reset type
>   to its children.
>   For this we need to pass the type as parameter to child_foreach()
>   method.
> 
> - are all children concerned ? For a given reset type, some child
>   may not need to be reset. As above we can handle that with
>   child_foreach: an resettable object can propagate the reset only
>   to a partial set of its child.
>   For this we need to know the type when we release the reset,
>   that's why I added it to resettable_release_reset() even if it
>   is unused right now.
>   I've also added an opaque parameter to child_foreach. I think
>   we will need that to handle the change of parent because we
>   will need to test if a child is concerned by a reset type: the
>   opaque will allow to use a test callback and get some result.

What about an optional ->filter() callback? That would be invoked if
existing prior to calling the child_foreach callback and could be used
to exclude children from the reset for this round for all callbacks. Or
have it modify the reset type (like in your pci reset -> cold reset
example above), and completely skip it if the reset type has been
modified to a 'no reset' type?

> 
> - several reset types at the same time. I don't another solution
>   than saying we execute *enter* and *hold* phase for every reset
>   type. *exit* will still be executed once for all at the end.
>   It will be up for each object to cope with it if it handle
>   multiple reset types. For *enter* is trivial, calling it twice
>   in a row is no problem given that it should only reset internal
>   state. For *hold* there may be some complication.
> 
> - Obviously we will need to at least an interface class field to hold
>   the supported reset types by the class. Also the reset state will
>   need some modification.
> ---
>  Makefile.objs   |   1 +
>  hw/core/Makefile.objs   |   1 +
>  hw/core/resettable.c| 230 
>  hw/core/trace-events|  17 +++
>  include/hw/resettable.h | 199 ++
>  5 files changed, 448 insertions(+)
>  create mode 100644 hw/core/resettable.c
>  create mode 100644 include/hw/resettable.h

Re: virtiofsd: Where should it live?

[Daniel P . Berrangé]

On Tue, Dec 03, 2019 at 11:06:44AM +, Peter Maydell wrote:
> On Tue, 3 Dec 2019 at 10:53, Dr. David Alan Gilbert  
> wrote:
> >
> > We seem to be coming to the conclusion something that:
> >
> >   a) It should live in the qemu tree
> >   b) It shouldn't live under contrib
> >   c) We'll create a new top level, i.e. 'daemons'
> >   d) virtiofsd will be daemons/virtiofsd
> >
> > Now, somethings I'm less clear on:
> >   e) What else would move into daemons?  It was suggested
> > that if we've got virtiofsd in there, then we should move
> > libvhost-user - which I understand, but then it's not a
> > 'daemons'.
> > Are there any otehr daemons that should move?
> 
> I like the idea of a new top level directory, but I think
> 'daemons' is a bit too specific -- for instance it seems to
> me that qemu-img would be sensible to move out of the root,
> and that's not a daemon.

Do we really need an extra directory level ?

IIUC, the main point against having $GIT_ROOT/virtiofsd is that
the root of our repo is quite cluttered already.

Rather than trying to create a multi-level hierarchy which adds
a debate around naming, why not address the clutter by moving
*ALL* the .c/.h files out of the root so that we have a flatter
tree:

  $GITROOT
+- qemu-system
|   +- vl.c
|   +- ...most other files...
+- qemu-img
|   +- qemu-img.c
+- qemu-nbd
|   +- qemu-nbd.c
+- qemu-io
|   +- qemu-io.c
|   +- qemu-io-cmds.c
+- qemu-bridge-helper
|   ...
+- qemu-edid
+- qemu-keymap
+- qga  (already exists)

Then we can add virtiofsd and other programs at the root with no big
issue.

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

Re: [PATCH v5 06/13] hw/core/qdev: handle parent bus change regarding resettable

[Cornelia Huck]

On Fri, 29 Nov 2019 18:41:26 +
Peter Maydell  wrote:

> On Fri, 18 Oct 2019 at 16:07, Damien Hedde  wrote:
> >
> > In qdev_set_parent_bus(), when changing the parent bus of a
> > realized device, if the source and destination buses are not in the
> > same reset state, some adaptation are required. This patch adds  
> 
> "adaptations"
> 
> > needed call to resettable_change_parent() to make sure a device reset
> > state stays coherent with its parent bus.
> >
> > The addition is a no-op if:
> > 1. the device being parented is not realized.
> > 2. the device is realized, but both buses are not under reset.
> >
> > Case 2 means that as long as qdev_set_parent_bus() is called
> > during the machine realization procedure (which is before the
> > machine reset so nothing is in reset), it is a no op.
> >
> > There are 49 call sites of qdev_set_parent_bus(). All but one fall
> > into the no-op case:
> > + 28 calls related to virtio (in hw/{s390x,display,virtio}/
> >   {vhost,virtio}-xxx.c) to set a _vdev_/_vgpu_ composing device
> >   parent bus just before realizing the _vdev_/_vgpu_.
> > + hw/qdev.c: when creating a device in qdev_try_create()
> > + hw/sysbus.c: when initializing a device in the sysbus
> > + hw/display/virtio-gpu-pci.c: before realizing VirtIOGPUPCIBase/vgpu
> > + hw/display/virtio-vga.c: before realizing VirtIOVGABase/vgpu
> > + hw/i386/amd_iommu.c: before realizing AMDVIState/pci
> > + hw/misc/auxbus.c: when creating an AUXBus
> > + hw/misc/auxbus.c: when creating an AUXBus child
> > + hw/misc/macio/macio.c: when initializing a MACIOState child
> > + hw/misc/macio/macio.c: before realizing NewWorldMacIOState/pmu
> > + hw/misc/macio/macio.c: before realizing NewWorldMacIOState/cuda
> > + hw/pci-host/designware.c: before realizing DesignwarePCIEHost/root
> > + hw/pci-host/gpex.c: before realizing GPEXHost/root
> > + hw/pci-host/prep.c: when initializaing PREPPCIState/pci_dev
> > + hw/pci-host/q35.c: before realizing Q35PCIHost/mch
> > + hw/pci-host/versatile.c: when initializing PCIVPBState/pci_dev
> > + hw/pci-host/xilinx-pcie.c: before realizing XilinxPCIEHost/root
> > + hw/s390x/event-facility.c: when creating SCLPEventFacility/
> >  TYPE_SCLP_QUIESCE
> > + hw/s390x/event-facility.c: ditto with SCLPEventFacility/
> >  TYPE_SCLP_CPU_HOTPLUG
> > + hw/s390x/sclp.c: Not trivial because it is called on a SLCPDevice
> >   just after realizing it. Ok because at this point the destination
> >   bus (sysbus) is not in reset; the realize step is before the
> >   machine reset.
> > + hw/sd/core.c: Not OK. Used in sdbus_reparent_card(). See below.
> > + hw/ssi/ssi.c: Used to put spi slave on spi bus and connect the cs
> >   line in ssi_auto_connect_slave(). Ok because this function is only
> >   used in realize step in hw/ssi/aspeed_smc.ci, hw/ssi/imx_spi.c,
> >   hw/ssi/mss-spi.c, hw/ssi/xilinx_spi.c and hw/ssi/xilinx_spips.c.
> > + hw/xen/xen-legacy-backend.c: when creating a XenLegacyDevice device
> > + qdev-monitor.c: in device hotplug creation procedure before realize  
> 
> This is a really useful analysis to have in the commit message;
> thanks!
> 
> (Side note: I wonder if the sclp.c case could be reordered so
> it realizes the device after parenting it? Anyway, not something
> to worry about now.)

As far as I can see, that should work. This code is a bit weird anyway;
the problem is that we need the sysbus somewhere in there... I'm
wondering if that can be handled in a different way. But agreed, that
is something we can revisit later.

> 
> > Note that this commit alone will have no effect, right now there is no
> > use of resettable API to reset anything. So a bus will never be tagged
> > as in-reset by this same API.
> >
> > The one place where side-effect will occurs is in hw/sd/core.c in
> > sdbus_reparent_card(). This function is only used in the raspi machines,
> > including during the sysbus reset procedure. This case will be fixed by
> > a following commit before globally enabling resettable API for sysbus
> > reset.
> >
> > Signed-off-by: Damien Hedde   
> 
> Reviewed-by: Peter Maydell 
> 
> thanks
> -- PMM
> 

Re: [PATCH 1/1] tests/vm: Allow to set qemu-img path

[Alex Bennée]

Wainer dos Santos Moschetta  writes:

> By default VM build test use qemu-img from system's PATH to
> create the image disk. Due the lack of qemu-img on the system
> or the desire to simply use a version built with QEMU, it would
> be nice to allow one to set its path. So this patch makes that
> possible by reading the path to qemu-img from QEMU_IMG if set,
> otherwise it fallback to default behavior.
>
> Signed-off-by: Wainer dos Santos Moschetta 

Queued to testing/next, thanks.

> ---
>  docs/devel/testing.rst| 6 --
>  tests/vm/Makefile.include | 1 +
>  tests/vm/basevm.py| 5 +
>  tests/vm/centos   | 2 +-
>  tests/vm/fedora   | 4 +---
>  tests/vm/freebsd  | 3 +--
>  tests/vm/netbsd   | 3 +--
>  tests/vm/openbsd  | 3 +--
>  tests/vm/ubuntu.i386  | 2 +-
>  9 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> index 8e981e062d..9be6cd4410 100644
> --- a/docs/devel/testing.rst
> +++ b/docs/devel/testing.rst
> @@ -418,13 +418,15 @@ access, so they SHOULD NOT be exposed to external 
> interfaces if you are
>  concerned about attackers taking control of the guest and potentially
>  exploiting a QEMU security bug to compromise the host.
>  
> -QEMU binary
> 
> +QEMU binaries
> +-
>  
>  By default, qemu-system-x86_64 is searched in $PATH to run the guest. If 
> there
>  isn't one, or if it is older than 2.10, the test won't work. In this case,
>  provide the QEMU binary in env var: ``QEMU=/path/to/qemu-2.10+``.
>  
> +Likewise the path to qemu-img can be set in QEMU_IMG environment variable.
> +
>  Make jobs
>  -
>  
> diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
> index fea348e845..9e7c46a473 100644
> --- a/tests/vm/Makefile.include
> +++ b/tests/vm/Makefile.include
> @@ -34,6 +34,7 @@ vm-help vm-test:
>   @echo "DEBUG=1   - Enable verbose output on 
> host and interactive debugging"
>   @echo "V=1   - Enable verbose ouput on host 
> and guest commands"
>   @echo "QEMU=/path/to/qemu- Change path to QEMU binary"
> + @echo "QEMU_IMG=/path/to/qemu-img- Change path to qemu-img tool"
>  
>  vm-build-all: $(addprefix vm-build-, $(IMAGES))
>  
> diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
> index 91a9226026..d1efeb3646 100755
> --- a/tests/vm/basevm.py
> +++ b/tests/vm/basevm.py
> @@ -152,6 +152,11 @@ class BaseVM(object):
>  def build_image(self, img):
>  raise NotImplementedError
>  
> +def exec_qemu_img(self, *args):
> +cmd = [os.environ.get("QEMU_IMG", "qemu-img")]
> +cmd.extend(list(args))
> +subprocess.check_call(cmd)
> +
>  def add_source_dir(self, src_dir):
>  name = "data-" + 
> hashlib.sha1(src_dir.encode("utf-8")).hexdigest()[:5]
>  tarfile = os.path.join(self._tmpdir, name + ".tar")
> diff --git a/tests/vm/centos b/tests/vm/centos
> index 53976f1c4c..eac07dacd6 100755
> --- a/tests/vm/centos
> +++ b/tests/vm/centos
> @@ -68,7 +68,7 @@ class CentosVM(basevm.BaseVM):
>  sys.stderr.write("Extracting the image...\n")
>  subprocess.check_call(["ln", "-f", cimg, img_tmp + ".xz"])
>  subprocess.check_call(["xz", "--keep", "-dvf", img_tmp + ".xz"])
> -subprocess.check_call(["qemu-img", "resize", img_tmp, "50G"])
> +self.exec_qemu_img("resize", img_tmp, "50G")
>  self.boot(img_tmp, extra_args = ["-cdrom", 
> self._gen_cloud_init_iso()])
>  self.wait_ssh()
>  self.ssh_root_check("touch /etc/cloud/cloud-init.disabled")
> diff --git a/tests/vm/fedora b/tests/vm/fedora
> index 7fec1479fb..8e270fc0f0 100755
> --- a/tests/vm/fedora
> +++ b/tests/vm/fedora
> @@ -74,9 +74,7 @@ class FedoraVM(basevm.BaseVM):
>  
>  self.print_step("Preparing iso and disk image")
>  subprocess.check_call(["cp", "-f", cimg, iso])
> -subprocess.check_call(["qemu-img", "create", "-f", "qcow2",
> -   img_tmp, self.size])
> -
> +self.exec_qemu_img("create", "-f", "qcow2", img_tmp, self.size)
>  self.print_step("Booting installer")
>  self.boot(img_tmp, extra_args = [
>  "-bios", "pc-bios/bios-256k.bin",
> diff --git a/tests/vm/freebsd b/tests/vm/freebsd
> index 2a19461a90..1825cc5821 100755
> --- a/tests/vm/freebsd
> +++ b/tests/vm/freebsd
> @@ -82,8 +82,7 @@ class FreeBSDVM(basevm.BaseVM):
>  self.print_step("Preparing iso and disk image")
>  subprocess.check_call(["cp", "-f", cimg, iso_xz])
>  subprocess.check_call(["xz", "-dvf", iso_xz])
> -subprocess.check_call(["qemu-img", "create", "-f", "qcow2",
> -   img_tmp, self.size])
> +self.exec_qemu_img("create", "-f", "qcow2", img_tmp, self.size)
>  
>  self.print_step("Booting installer")
>  self.boot(img_tmp, extra_args = [
> 

Re: [PATCH v5 11/13] hw/s390x/ipl: replace deprecated qdev_reset_all registration

[Cornelia Huck]

On Fri, 18 Oct 2019 17:06:28 +0200
Damien Hedde  wrote:

> Replace deprecated qdev_reset_all by resettable_cold_reset_fn for
> the ipl registration in the main reset handlers.
> 
> This does not impact the behavior for the following reasons:
> + at this point resettable just call the old reset methods of devices
>   and buses in the same order than qdev/qbus.
> + resettable handlers registered with qemu_register_reset are
>   serialized; there is no interleaving.
> + eventual explicit calls to legacy reset API (device_reset or
>   qdev/qbus_reset) inside this reset handler will not be masked out
>   by resettable mechanism; they do not go through resettable api.
> 
> Signed-off-by: Damien Hedde 
> ---
> Cc: Cornelia Huck 
> Cc: qemu-s3...@nongnu.org
> Cc: Christian Borntraeger 
> Cc: Thomas Huth 
> ---
>  hw/s390x/ipl.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)

Reviewed-by: Cornelia Huck 

Re: [PATCH v5 00/13] Multi-phase reset mechanism

[Cornelia Huck]

On Fri, 18 Oct 2019 17:06:17 +0200
Damien Hedde  wrote:

> Hi all,
> 
> The purpose of this series is to split the current reset procedure
> into multiple phases. This will help to solve some ordering
> difficulties we have during reset. Previous version can be found here:
> https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg04359.html
> 
> This series adds resettable interface and transitions base Device and
> Bus classes (sysbus subclasses are ok too). It provides new reset
> functions but does not switch anymore the old functions
> (device_reset() and qdev/qbus_reset_all()) to resettable interface.
> These functions keep the exact same behavior as before.
> 
> The series also transition the main reset handlers registration which
> has no impact until devices and buses are transitioned.
> 
> I think this version is way better regarding the transition from the
> legacy to the resettable interface than the previous one.
> After this series, the plan is then to transition devices, buses and
> legacy reset call sites. Devices and buses have to be transitioned
> from mother class to daughter classes order but until the final
> (daughter) class is transitioned, old monolitic reset behavior will
> be kept for this class.

I have looked over this patchset a bit (with an eye to the s390 stuff).
Seems sane, although I currently don't have the resources to review
more in detail.

Re: [PATCH v5 03/22] target/arm: Add MTE system registers

[Peter Maydell]

On Fri, 11 Oct 2019 at 14:48, Richard Henderson
 wrote:
>
> This is TFSRE0_EL1, TFSR_EL1, TFSR_EL2, TFSR_EL3,
> RGSR_EL1, GCR_EL1, GMID_EL1, and PSTATE.TCO.
>
> Signed-off-by: Richard Henderson 
> ---
> v3: Add GMID; add access_mte.
> v4: Define only TCO at mte_insn_reg.
> ---
>  target/arm/cpu.h   |  3 ++
>  target/arm/internals.h |  6 
>  target/arm/helper.c| 73 ++
>  target/arm/translate-a64.c | 11 ++
>  4 files changed, 93 insertions(+)


> +{ .name = "GMID_EL1", .state = ARM_CP_STATE_AA64,
> +  .opc0 = 3, .opc1 = 1, .crn = 0, .crm = 0, .opc2 = 4,
> +  .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = GMID_EL1_BS },

This should trap if HCR_EL2.TID5 is 1 (since we're adding
support for the TID* ID reg trap bits now).

> +REGINFO_SENTINEL
> +};
> +
> +static const ARMCPRegInfo mte_tco_reginfo[] = {
> +{ .name = "TCO", .state = ARM_CP_STATE_AA64,
> +  .opc0 = 3, .opc1 = 3, .crn = 4, .crm = 2, .opc2 = 7,
> +  .type = ARM_CP_NO_RAW,
> +  .access = PL0_RW, .readfn = tco_read, .writefn = tco_write },
> +REGINFO_SENTINEL
> +};
>  #endif
>
>  static CPAccessResult access_predinv(CPUARMState *env, const ARMCPRegInfo 
> *ri,
> @@ -6881,6 +6948,12 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>  if (cpu_isar_feature(aa64_rndr, cpu)) {
>  define_arm_cp_regs(cpu, rndr_reginfo);
>  }

So, aa64_mte_insn_reg here is checking for ID_AA64PFR1_EL1 != 0
("instructions accessible at EL0 are implemented")
and aa64_mte is checking for >= 2 ("full implementation").
I think a couple of brief comments would clarify:

> +if (cpu_isar_feature(aa64_mte_insn_reg, cpu)) {
   /* EL0-visible MTE registers, present even for dummy
implementation */
> +define_arm_cp_regs(cpu, mte_tco_reginfo);
> +}
> +if (cpu_isar_feature(aa64_mte, cpu)) {
   /* MTE registers present for a full implementation */
> +define_arm_cp_regs(cpu, mte_reginfo);
> +}

(The other way to arrange this would be to have the 'real'
TCO regdef in mte_reginfo, and separately have "reginfo
if we only have the dummy visible-from-EL0-parts-only
which defines a constant 0 TCO" (and also make the MSR_i
code implement a RAZ/WI for this case, for consistency).
An implementation that allows the guest to toggle the PSTATE.TCO
bit to no visible effect is architecturally valid, though.)

>  #endif
>
>  /*
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index c85db69db4..62bdf50796 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -1611,6 +1611,17 @@ static void handle_msr_i(DisasContext *s, uint32_t 
> insn,
>  s->base.is_jmp = DISAS_UPDATE;
>  break;
>
> +case 0x1c: /* TCO */
> +if (!dc_isar_feature(aa64_mte_insn_reg, s)) {
> +goto do_unallocated;
> +}
> +if (crm & 1) {
> +set_pstate_bits(PSTATE_TCO);
> +} else {
> +clear_pstate_bits(PSTATE_TCO);
> +}
> +break;
> +
>  default:
>  do_unallocated:
>  unallocated_encoding(s);
> --
> 2.17.1

Otherwise
Reviewed-by: Peter Maydell 


thanks
-- PMM

Re: [PATCH] virtfs-proxy-helper: switch from libcap to libcap-ng

[Paolo Bonzini]

On 03/12/19 11:34, Greg Kurz wrote:
> On Fri, 29 Nov 2019 13:59:37 +0100
> Greg Kurz  wrote:
> 
>> On Fri, 29 Nov 2019 13:49:20 +0100
>> Paolo Bonzini  wrote:
>>
>>> On 29/11/19 13:32, Greg Kurz wrote:
 Nice. :)

 Reviewed-by: Greg Kurz 

 Paolo,

 I can take this through my 9p tree if you want. Otherwise,

 Acked-by: Greg Kurz 
>>>
>>> Yes, please do it since it's self-contained.  You'd probably also test
>>> it better than me. :)
>>>
>>> Paolo
>>>
>>
>> Ok I'll just do that then.
>>
> 
> And it happens to be missing an extra-change in Makefile :)
> 
> -fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap
> +fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap-ng

The new line is not needed, -lcap-ng should already be in LIBS via

LIBS+=-lz $(LIBS_TOOLS)

However, removing -lcap is certainly a good idea. :)

Paolo

Re: [PATCH for-5.0 v2 17/23] iotests: Use skip_if_unsupported decorator in 041

[Vladimir Sementsov-Ogievskiy]

11.11.2019 19:02, Max Reitz wrote:
> We can use this decorator above TestRepairQuorum.setUp() to skip all
> quorum tests with a single line.
> 
> Signed-off-by: Max Reitz

Reviewed-by: Vladimir Sementsov-Ogievskiy 

-- 
Best regards,
Vladimir

[PATCH 3/5] tests: fw_cfg: Use virt as default machine in fw_cfg-test.c

[Xiang Zheng]

The default machine type on aarch64 is not set which causes error in
qtest_init(). Here we use the "virt" machine as the default machine
type on aarch64.

Signed-off-by: Xiang Zheng 
---
 tests/fw_cfg-test.c | 65 +++--
 1 file changed, 51 insertions(+), 14 deletions(-)

diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
index 5a5342fa9d..a3a67d1099 100644
--- a/tests/fw_cfg-test.c
+++ b/tests/fw_cfg-test.c
@@ -23,13 +23,28 @@ static uint16_t max_cpus = 1;
 static uint64_t nb_nodes = 0;
 static uint16_t boot_menu = 0;
 
+static char *make_extra_args(const char *args)
+{
+const char *arch = qtest_get_arch();
+const char *machine_arg = NULL;
+
+if (strcmp(arch, "aarch64") == 0) {
+machine_arg = "-machine virt";
+} else {
+machine_arg = "";
+}
+
+return g_strdup_printf("%s %s", machine_arg, args);
+}
+
 static void test_fw_cfg_signature(void)
 {
 QFWCFG *fw_cfg;
 QTestState *s;
 char buf[5];
+char *cli = make_extra_args("");
 
-s = qtest_init("");
+s = qtest_init(cli);
 fw_cfg = fw_cfg_init(s);
 
 qfw_cfg_get(fw_cfg, FW_CFG_SIGNATURE, buf, 4);
@@ -38,6 +53,7 @@ static void test_fw_cfg_signature(void)
 g_assert_cmpstr(buf, ==, "QEMU");
 fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
+g_free(cli);
 }
 
 static void test_fw_cfg_id(void)
@@ -45,8 +61,9 @@ static void test_fw_cfg_id(void)
 QFWCFG *fw_cfg;
 QTestState *s;
 uint32_t id;
+char *cli = make_extra_args("");
 
-s = qtest_init("");
+s = qtest_init(cli);
 fw_cfg = fw_cfg_init(s);
 
 id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
@@ -54,6 +71,7 @@ static void test_fw_cfg_id(void)
  (id == 3));
 fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
+g_free(cli);
 }
 
 static void test_fw_cfg_uuid(void)
@@ -66,8 +84,9 @@ static void test_fw_cfg_uuid(void)
 0x46, 0x00, 0xcb, 0x32, 0x38, 0xec, 0x4b, 0x2f,
 0x8a, 0xcb, 0x81, 0xc6, 0xea, 0x54, 0xf2, 0xd8,
 };
+char *cli = make_extra_args("-uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8");
 
-s = qtest_init("-uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8");
+s = qtest_init(cli);
 fw_cfg = fw_cfg_init(s);
 
 qfw_cfg_get(fw_cfg, FW_CFG_UUID, buf, 16);
@@ -75,62 +94,70 @@ static void test_fw_cfg_uuid(void)
 
 fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
-
+g_free(cli);
 }
 
 static void test_fw_cfg_ram_size(void)
 {
 QFWCFG *fw_cfg;
 QTestState *s;
+char *cli = make_extra_args("");
 
-s = qtest_init("");
+s = qtest_init(cli);
 fw_cfg = fw_cfg_init(s);
 
 g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_RAM_SIZE), ==, ram_size);
 
 fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
+g_free(cli);
 }
 
 static void test_fw_cfg_nographic(void)
 {
 QFWCFG *fw_cfg;
 QTestState *s;
+char *cli = make_extra_args("");
 
-s = qtest_init("");
+s = qtest_init(cli);
 fw_cfg = fw_cfg_init(s);
 
 g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NOGRAPHIC), ==, 0);
 
 fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
+g_free(cli);
 }
 
 static void test_fw_cfg_nb_cpus(void)
 {
 QFWCFG *fw_cfg;
 QTestState *s;
+char *cli = make_extra_args("");
 
-s = qtest_init("");
+s = qtest_init(cli);
 fw_cfg = fw_cfg_init(s);
 
 g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NB_CPUS), ==, nb_cpus);
 
 fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
+g_free(cli);
 }
 
 static void test_fw_cfg_max_cpus(void)
 {
 QFWCFG *fw_cfg;
 QTestState *s;
+char *cli = make_extra_args("");
 
-s = qtest_init("");
+s = qtest_init(cli);
 fw_cfg = fw_cfg_init(s);
 
 g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_MAX_CPUS), ==, max_cpus);
 fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
+g_free(cli);
 }
 
 static void test_fw_cfg_numa(void)
@@ -139,8 +166,9 @@ static void test_fw_cfg_numa(void)
 QTestState *s;
 uint64_t *cpu_mask;
 uint64_t *node_mask;
+char *cli = make_extra_args("");
 
-s = qtest_init("");
+s = qtest_init(cli);
 fw_cfg = fw_cfg_init(s);
 
 g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_NUMA), ==, nb_nodes);
@@ -160,19 +188,22 @@ static void test_fw_cfg_numa(void)
 g_free(cpu_mask);
 fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
+g_free(cli);
 }
 
 static void test_fw_cfg_boot_menu(void)
 {
 QFWCFG *fw_cfg;
 QTestState *s;
+char *cli = make_extra_args("");
 
-s = qtest_init("");
+s = qtest_init(cli);
 fw_cfg = fw_cfg_init(s);
 
 g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_MENU), ==, boot_menu);
 fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
+g_free(cli);
 }
 
 static void test_fw_cfg_reboot_timeout(void)
@@ -181,8 +212,9 @@ static void test_fw_cfg_reboot_timeout(void)
 QTestState *s;
 uint32_t reboot_timeout = 0;
 size_t filesize;
+char *cli = make_extra_args("-boot reboot-timeout=15");
 
-s = qtest_init("-boot reboot-timeout=15");
+s = qtes

[PATCH 0/5] tests: Enable fw_cfg tests on AArch64

[Xiang Zheng]

There are quite a few tests disabled on AArch64 such as fw_cfg-tests.
This patch series fix some problems in test code and adapt it to
virt machine.

Xiang Zheng (5):
  tests: fw_cfg: Rename pc_fw_cfg_* to fw_cfg_*
  tests: fw_cfg: Support read/write of fw_cfg registers on aarch64
  tests: fw_cfg: Use virt as default machine in fw_cfg-test.c
  hw/arm/virt: Add FW_CFG_RAM_SIZE and FW_CFG_MAX_CPUS into fw_cfg
  tests: Enable fw_cfg test on aarch64

 hw/arm/virt.c|   3 ++
 tests/Makefile.include   |   1 +
 tests/fw_cfg-test.c  | 113 ++-
 tests/hd-geo-test.c  |   6 +--
 tests/libqos/fw_cfg.c|  17 +-
 tests/libqos/fw_cfg.h|  20 +--
 tests/libqos/malloc-pc.c |   4 +-
 7 files changed, 115 insertions(+), 49 deletions(-)

-- 
2.19.1

[PATCH 1/5] tests: fw_cfg: Rename pc_fw_cfg_* to fw_cfg_*

[Xiang Zheng]

Rename pc_fw_cfg_* to fw_cfg_* to make them common for other
architectures so that we can run fw_cfg tests on aarch64.

Signed-off-by: Xiang Zheng 
---
 tests/fw_cfg-test.c  | 48 
 tests/hd-geo-test.c  |  6 ++---
 tests/libqos/fw_cfg.h| 20 +
 tests/libqos/malloc-pc.c |  4 ++--
 4 files changed, 45 insertions(+), 33 deletions(-)

diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
index 5dc807ba23..5a5342fa9d 100644
--- a/tests/fw_cfg-test.c
+++ b/tests/fw_cfg-test.c
@@ -30,13 +30,13 @@ static void test_fw_cfg_signature(void)
 char buf[5];
 
 s = qtest_init("");
-fw_cfg = pc_fw_cfg_init(s);
+fw_cfg = fw_cfg_init(s);
 
 qfw_cfg_get(fw_cfg, FW_CFG_SIGNATURE, buf, 4);
 buf[4] = 0;
 
 g_assert_cmpstr(buf, ==, "QEMU");
-pc_fw_cfg_uninit(fw_cfg);
+fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
 }
 
@@ -47,12 +47,12 @@ static void test_fw_cfg_id(void)
 uint32_t id;
 
 s = qtest_init("");
-fw_cfg = pc_fw_cfg_init(s);
+fw_cfg = fw_cfg_init(s);
 
 id = qfw_cfg_get_u32(fw_cfg, FW_CFG_ID);
 g_assert((id == 1) ||
  (id == 3));
-pc_fw_cfg_uninit(fw_cfg);
+fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
 }
 
@@ -68,12 +68,12 @@ static void test_fw_cfg_uuid(void)
 };
 
 s = qtest_init("-uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8");
-fw_cfg = pc_fw_cfg_init(s);
+fw_cfg = fw_cfg_init(s);
 
 qfw_cfg_get(fw_cfg, FW_CFG_UUID, buf, 16);
 g_assert(memcmp(buf, uuid, sizeof(buf)) == 0);
 
-pc_fw_cfg_uninit(fw_cfg);
+fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
 
 }
@@ -84,11 +84,11 @@ static void test_fw_cfg_ram_size(void)
 QTestState *s;
 
 s = qtest_init("");
-fw_cfg = pc_fw_cfg_init(s);
+fw_cfg = fw_cfg_init(s);
 
 g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_RAM_SIZE), ==, ram_size);
 
-pc_fw_cfg_uninit(fw_cfg);
+fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
 }
 
@@ -98,11 +98,11 @@ static void test_fw_cfg_nographic(void)
 QTestState *s;
 
 s = qtest_init("");
-fw_cfg = pc_fw_cfg_init(s);
+fw_cfg = fw_cfg_init(s);
 
 g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NOGRAPHIC), ==, 0);
 
-pc_fw_cfg_uninit(fw_cfg);
+fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
 }
 
@@ -112,11 +112,11 @@ static void test_fw_cfg_nb_cpus(void)
 QTestState *s;
 
 s = qtest_init("");
-fw_cfg = pc_fw_cfg_init(s);
+fw_cfg = fw_cfg_init(s);
 
 g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_NB_CPUS), ==, nb_cpus);
 
-pc_fw_cfg_uninit(fw_cfg);
+fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
 }
 
@@ -126,10 +126,10 @@ static void test_fw_cfg_max_cpus(void)
 QTestState *s;
 
 s = qtest_init("");
-fw_cfg = pc_fw_cfg_init(s);
+fw_cfg = fw_cfg_init(s);
 
 g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_MAX_CPUS), ==, max_cpus);
-pc_fw_cfg_uninit(fw_cfg);
+fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
 }
 
@@ -141,7 +141,7 @@ static void test_fw_cfg_numa(void)
 uint64_t *node_mask;
 
 s = qtest_init("");
-fw_cfg = pc_fw_cfg_init(s);
+fw_cfg = fw_cfg_init(s);
 
 g_assert_cmpint(qfw_cfg_get_u64(fw_cfg, FW_CFG_NUMA), ==, nb_nodes);
 
@@ -158,7 +158,7 @@ static void test_fw_cfg_numa(void)
 
 g_free(node_mask);
 g_free(cpu_mask);
-pc_fw_cfg_uninit(fw_cfg);
+fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
 }
 
@@ -168,10 +168,10 @@ static void test_fw_cfg_boot_menu(void)
 QTestState *s;
 
 s = qtest_init("");
-fw_cfg = pc_fw_cfg_init(s);
+fw_cfg = fw_cfg_init(s);
 
 g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_MENU), ==, boot_menu);
-pc_fw_cfg_uninit(fw_cfg);
+fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
 }
 
@@ -183,14 +183,14 @@ static void test_fw_cfg_reboot_timeout(void)
 size_t filesize;
 
 s = qtest_init("-boot reboot-timeout=15");
-fw_cfg = pc_fw_cfg_init(s);
+fw_cfg = fw_cfg_init(s);
 
 filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-fail-wait",
 &reboot_timeout, sizeof(reboot_timeout));
 g_assert_cmpint(filesize, ==, sizeof(reboot_timeout));
 reboot_timeout = le32_to_cpu(reboot_timeout);
 g_assert_cmpint(reboot_timeout, ==, 15);
-pc_fw_cfg_uninit(fw_cfg);
+fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
 }
 
@@ -203,14 +203,14 @@ static void test_fw_cfg_no_reboot_timeout(void)
 
 /* Special value -1 means "don't reboot" */
 s = qtest_init("-boot reboot-timeout=-1");
-fw_cfg = pc_fw_cfg_init(s);
+fw_cfg = fw_cfg_init(s);
 
 filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-fail-wait",
 &reboot_timeout, sizeof(reboot_timeout));
 g_assert_cmpint(filesize, ==, sizeof(reboot_timeout));
 reboot_timeout = le32_to_cpu(reboot_timeout);
 g_assert_cmpint(reboot_timeout, ==, UINT32_MAX);
-pc_fw_cfg_uninit(fw_cfg);
+fw_cfg_uninit(fw_cfg);
 qtest_quit(s);
 }
 
@@ -222,14 +222,14 @@ static void test_fw_cfg_splas

[PATCH 4/5] hw/arm/virt: Add FW_CFG_RAM_SIZE and FW_CFG_MAX_CPUS into fw_cfg

[Xiang Zheng]

I'm not sure whether it's neccesary to add FW_CFG_RAM_SIZE and
FW_CFG_MAX_CPUS into fw_cfg on virt machine. This patch just makes
the fw_cfg-test happy.

Signed-off-by: Xiang Zheng 
---
 hw/arm/virt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d4bedc2607..26a4183775 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1084,6 +1084,9 @@ static FWCfgState *create_fw_cfg(const VirtMachineState 
*vms, AddressSpace *as)
 fw_cfg = fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
 fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)ms->smp.cpus);
 
+fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
+fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)ms->smp.max_cpus);
+
 nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
 qemu_fdt_add_subnode(vms->fdt, nodename);
 qemu_fdt_setprop_string(vms->fdt, nodename,
-- 
2.19.1

[PATCH 5/5] tests: Enable fw_cfg test on aarch64

[Xiang Zheng]

Now turn on the fw_cfg test for aarch64.

Signed-off-by: Xiang Zheng 
---
 tests/Makefile.include | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 8566f5f119..180e0ed2b7 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -276,6 +276,7 @@ check-qtest-aarch64-y += tests/arm-cpu-features$(EXESUF)
 check-qtest-aarch64-y += tests/numa-test$(EXESUF)
 check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
 check-qtest-aarch64-y += tests/migration-test$(EXESUF)
+check-qtest-aarch64-y += tests/fw_cfg-test$(EXESUF)
 # TODO: once aarch64 TCG is fixed on ARM 32 bit host, make test unconditional
 ifneq ($(ARCH),arm)
 check-qtest-aarch64-y += tests/bios-tables-test$(EXESUF)
-- 
2.19.1

Re: [PATCH 4/5] hw/arm/virt: Add FW_CFG_RAM_SIZE and FW_CFG_MAX_CPUS into fw_cfg

[Peter Maydell]

On Tue, 3 Dec 2019 at 12:29, Xiang Zheng  wrote:
>
> I'm not sure whether it's neccesary to add FW_CFG_RAM_SIZE and
> FW_CFG_MAX_CPUS into fw_cfg on virt machine. This patch just makes
> the fw_cfg-test happy.
>
> Signed-off-by: Xiang Zheng 
> ---
>  hw/arm/virt.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d4bedc2607..26a4183775 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1084,6 +1084,9 @@ static FWCfgState *create_fw_cfg(const VirtMachineState 
> *vms, AddressSpace *as)
>  fw_cfg = fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
>  fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)ms->smp.cpus);
>
> +fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
> +fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)ms->smp.max_cpus);
> +
>  nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
>  qemu_fdt_add_subnode(vms->fdt, nodename);
>  qemu_fdt_setprop_string(vms->fdt, nodename,
> --

Is there a spec anywhere that defines the meaning of these
FW_CFG entries ? docs/specs/fw_cfg.txt defines the
device interface but not what the 'standard' keys mean.
I'd prefer not to add them to the virt board without knowing
what they mean and why we have them.

thanks
-- PMM

[PATCH 2/5] tests: fw_cfg: Support read/write of fw_cfg registers on aarch64

[Xiang Zheng]

Refer to the fw_cfg registers locations of x86 and arm in
docs/specs/fw_cfg.txt, the test codes need to differ on the addresses
for read/write.

Besides, fix the endian problems in mm_fw_cfg_select().

Signed-off-by: Xiang Zheng 
---
 tests/libqos/fw_cfg.c | 17 +++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/tests/libqos/fw_cfg.c b/tests/libqos/fw_cfg.c
index 1f46258f96..c1518c5e81 100644
--- a/tests/libqos/fw_cfg.c
+++ b/tests/libqos/fw_cfg.c
@@ -57,7 +57,14 @@ uint64_t qfw_cfg_get_u64(QFWCFG *fw_cfg, uint16_t key)
 
 static void mm_fw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
 {
-qtest_writew(fw_cfg->qts, fw_cfg->base, key);
+const char *arch = qtest_get_arch();
+uint64_t offset = 0;
+
+if (!strcmp(arch, "aarch64")) {
+offset = 8;
+}
+
+qtest_writew(fw_cfg->qts, fw_cfg->base + offset, cpu_to_be16(key));
 }
 
 /*
@@ -108,9 +115,15 @@ static void mm_fw_cfg_read(QFWCFG *fw_cfg, void *data, 
size_t len)
 {
 uint8_t *ptr = data;
 int i;
+uint64_t offset = 2;
+const char *arch = qtest_get_arch();
+
+if (!strcmp(arch, "aarch64")) {
+offset = 0;
+}
 
 for (i = 0; i < len; i++) {
-ptr[i] = qtest_readb(fw_cfg->qts, fw_cfg->base + 2);
+ptr[i] = qtest_readb(fw_cfg->qts, fw_cfg->base + offset);
 }
 }
 
-- 
2.19.1

Re: [PATCH 1/5] tests: fw_cfg: Rename pc_fw_cfg_* to fw_cfg_*

[Peter Maydell]

On Tue, 3 Dec 2019 at 12:29, Xiang Zheng  wrote:
>
> Rename pc_fw_cfg_* to fw_cfg_* to make them common for other
> architectures so that we can run fw_cfg tests on aarch64.
>
> Signed-off-by: Xiang Zheng 

> -static inline QFWCFG *pc_fw_cfg_init(QTestState *qts)
> +static inline QFWCFG *fw_cfg_init(QTestState *qts)
>  {
> -return io_fw_cfg_init(qts, 0x510);
> +const char *arch = qtest_get_arch();
> +
> +if (!strcmp(arch, "aarch64")) {
> +return mm_fw_cfg_init(qts, 0x0902);
> +} else {
> +return io_fw_cfg_init(qts, 0x510);
> +}

Presence and address of the fw_cfg device depends
on the machine type, not the architecture, so is
it possible to write this so that it varies by
machine type, rather than by guest arch ?
There should also presumably be a fallback path
for "fw_cfg not present here", I suppose.

thanks
-- PMM

Re: [PATCH v2 03/14] gdbstub: move str_buf to GDBState and use GString

[Damien Hedde]

On 11/30/19 9:45 AM, Alex Bennée wrote:
> Rather than having a static buffer replace str_buf with a GString
> which we know can grow on demand. Convert the internal functions to
> take a GString instead of a char * and length.
> 
> Signed-off-by: Alex Bennée 
> Reviewed-by: Richard Henderson 
> 
> ---
> v2
>   - fix conflict from status gdbserver_state
>   - add put_strbuf helper
> ---
>  gdbstub.c | 195 +-
>  1 file changed, 90 insertions(+), 105 deletions(-)
> 
> @@ -667,25 +667,28 @@ static int put_packet(const char *buf)

Hi,

I did some tests with my target having lot of registers and was
wondering if we should add an assert there (or even better in
put_packet_binary()). Something like:
   /* FIXME: until bigger packets are supported */
   g_assert(strlen(buf) <= MAX_PACKET_LENGTH);

There is a memcpy() in put_packet_binary() that overflows
in that case. With this patch, read_all_registers() can for example
generate binary packet up to 2*MAX_PACKET_LENGTH.

>  return put_packet_binary(buf, strlen(buf), false);
>  }

Apart from this case which don't happen with in-tree targets, it works
fine. So,
Tested-by: Damien Hedde 

I'll work on the missing bits for bigger packet support I soon as I have
some spare time.

Regards,
--
Damien

Re: [PATCH v2 2/4] target/arm: Abstract the generic timer frequency

[Andrew Jeffery]

On Tue, 3 Dec 2019, at 16:39, Philippe Mathieu-Daudé wrote:
> On 12/3/19 5:14 AM, Andrew Jeffery wrote:
> > Prepare for SoCs such as the ASPEED AST2600 whose firmware configures
> > CNTFRQ to values significantly larger than the static 62.5MHz value
> > currently derived from GTIMER_SCALE. As the OS potentially derives its
> > timer periods from the CNTFRQ value the lack of support for running
> > QEMUTimers at the appropriate rate leads to sticky behaviour in the
> > guest.
> > 
> > Substitute the GTIMER_SCALE constant with use of a helper to derive the
> > period from gt_cntfrq stored in struct ARMCPU. Initially set gt_cntfrq
> > to the frequency associated with GTIMER_SCALE so current behaviour is
> > maintained.
> > 
> > Signed-off-by: Andrew Jeffery 
> > Reviewed-by: Richard Henderson 
> > ---
> >   target/arm/cpu.c|  2 ++
> >   target/arm/cpu.h| 10 ++
> >   target/arm/helper.c | 10 +++---
> >   3 files changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 7a4ac9339bf9..5698a74061bb 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -974,6 +974,8 @@ static void arm_cpu_initfn(Object *obj)
> >   if (tcg_enabled()) {
> >   cpu->psci_version = 2; /* TCG implements PSCI 0.2 */
> >   }
> > +
> > +cpu->gt_cntfrq = NANOSECONDS_PER_SECOND / GTIMER_SCALE;
> >   }
> >   
> >   static Property arm_cpu_reset_cbar_property =
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 83a809d4bac4..666c03871fdf 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -932,8 +932,18 @@ struct ARMCPU {
> >*/
> >   DECLARE_BITMAP(sve_vq_map, ARM_MAX_VQ);
> >   DECLARE_BITMAP(sve_vq_init, ARM_MAX_VQ);
> > +
> > +/* Generic timer counter frequency, in Hz */
> > +uint64_t gt_cntfrq;
> 
> You can also explicit the unit by calling it 'gt_cntfrq_hz'.

Fair call, I'll fix that.

> 
> >   };
> >   
> > +static inline unsigned int gt_cntfrq_period_ns(ARMCPU *cpu)
> > +{
> > +/* XXX: Could include qemu/timer.h to get NANOSECONDS_PER_SECOND? */
> 
> Why inline this call? I doubt there is a significant performance gain.

It wasn't so much performance. It started out as a macro for a simple 
calculation
because I didn't want to duplicate it across a number of places, then I wanted 
type
safety for the pointer so  I switched the macro in the header to an inline 
function. So
it is an evolution of the patch rather than something that came from an 
explicit goal
of e.g. performance.

Andrew

Re: [PATCH v2 3/4] target/arm: Prepare generic timer for per-platform CNTFRQ

[Andrew Jeffery]

On Tue, 3 Dec 2019, at 16:49, Philippe Mathieu-Daudé wrote:
> On 12/3/19 5:14 AM, Andrew Jeffery wrote:
> > The ASPEED AST2600 clocks the generic timer at the rate of HPLL. On
> > recent firmwares this is at 1125MHz, which is considerably quicker than
> > the assumed 62.5MHz of the current generic timer implementation. The
> > delta between the value as read from CNTFRQ and the true rate of the
> > underlying QEMUTimer leads to sticky behaviour in AST2600 guests.
> > 
> > Add a feature-gated property exposing CNTFRQ for ARM CPUs providing the
> > generic timer. This allows platforms to configure CNTFRQ (and the
> > associated QEMUTimer) to the appropriate frequency prior to starting the
> > guest.
> > 
> > As the platform can now determine the rate of CNTFRQ we're exposed to
> > limitations of QEMUTimer that didn't previously materialise: In the
> > course of emulation we need to arbitrarily and accurately convert
> > between guest ticks and time, but we're constrained by QEMUTimer's use
> > of an integer scaling factor. The effect is QEMUTimer cannot exactly
> > capture the period of frequencies that do not cleanly divide
> > NANOSECONDS_PER_SECOND for scaling ticks to time. As such, provide an
> > equally inaccurate scaling factor for scaling time to ticks so at least
> > a self-consistent inverse relationship holds.
> > 
> > Signed-off-by: Andrew Jeffery 
> > Reviewed-by: Richard Henderson 
> > ---
> >   target/arm/cpu.c| 43 +--
> >   target/arm/cpu.h| 18 ++
> >   target/arm/helper.c |  9 -
> >   3 files changed, 59 insertions(+), 11 deletions(-)
> > 
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 5698a74061bb..f186019a77fd 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -974,10 +974,12 @@ static void arm_cpu_initfn(Object *obj)
> >   if (tcg_enabled()) {
> >   cpu->psci_version = 2; /* TCG implements PSCI 0.2 */
> >   }
> > -
> > -cpu->gt_cntfrq = NANOSECONDS_PER_SECOND / GTIMER_SCALE;
> >   }
> >   
> > +static Property arm_cpu_gt_cntfrq_property =
> > +DEFINE_PROP_UINT64("cntfrq", ARMCPU, gt_cntfrq,
> > +   NANOSECONDS_PER_SECOND / GTIMER_SCALE);
> > +
> >   static Property arm_cpu_reset_cbar_property =
> >   DEFINE_PROP_UINT64("reset-cbar", ARMCPU, reset_cbar, 0);
> >   
> > @@ -1174,6 +1176,11 @@ void arm_cpu_post_init(Object *obj)
> >   
> >   qdev_property_add_static(DEVICE(obj), &arm_cpu_cfgend_property,
> >&error_abort);
> > +
> > +if (arm_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER)) {
> > +qdev_property_add_static(DEVICE(cpu), &arm_cpu_gt_cntfrq_property,
> > + &error_abort);
> > +}
> >   }
> >   
> >   static void arm_cpu_finalizefn(Object *obj)
> > @@ -1253,14 +1260,30 @@ static void arm_cpu_realizefn(DeviceState *dev, 
> > Error **errp)
> >   }
> >   }
> >   
> > -cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, 
> > GTIMER_SCALE,
> > -   arm_gt_ptimer_cb, cpu);
> > -cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, 
> > GTIMER_SCALE,
> > -   arm_gt_vtimer_cb, cpu);
> > -cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
> > -  arm_gt_htimer_cb, cpu);
> > -cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
> > -  arm_gt_stimer_cb, cpu);
> > +
> > +{
> > +uint64_t scale;
> 
> Apparently you have to use this odd indent due to the '#ifndef 
> CONFIG_USER_ONLY'. Well, acceptable.

It's the indent associated with the block scope for the scale variable to limit 
its lifetime
to where I needed it.

> 
> > +
> > +if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) {
> > +if (!cpu->gt_cntfrq) {
> > +error_setg(errp, "Invalid CNTFRQ: %"PRId64"Hz",
> > +   cpu->gt_cntfrq);
> > +return;
> > +}
> > +scale = gt_cntfrq_period_ns(cpu);
> > +} else {
> > +scale = GTIMER_SCALE;
> > +}
> > +
> > +cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
> > +   arm_gt_ptimer_cb, cpu);
> > +cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
> > +   arm_gt_vtimer_cb, cpu);
> > +cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
> > +  arm_gt_htimer_cb, cpu);
> > +cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
> > +  arm_gt_stimer_cb, cpu);
> > +}
> >   #endif
> >   
> >   cpu_exec_realizefn(cs, &local_e

Re: [PATCH for-5.0 v2 18/23] iotests: Add VM.assert_block_path()

[Vladimir Sementsov-Ogievskiy]

11.11.2019 19:02, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> ---
>   tests/qemu-iotests/iotests.py | 59 +++
>   1 file changed, 59 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index d34305ce69..3e03320ce3 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -681,6 +681,65 @@ class VM(qtest.QEMUQtestMachine):
>   
>   return fields.items() <= ret.items()
>   
> +"""
> +Check whether the node under the given path in the block graph is
> +@expected_node.
> +
> +@root is the node name of the node where the @path is rooted.
> +
> +@path is a string that consists of child names separated by
> +slashes.  It must begin with a slash.

Why do you need this slash? To stress that we are starting from root?
But root is not global, it's selected by previous argument, so for me the
path is more like relative than absolute..

> +
> +Examples for @root + @path:
> +  - root="qcow2-node", path="/backing/file"
> +  - root="quorum-node", path="/children.2/file"
> +
> +Hypothetically, @path could be empty, in which case it would point
> +to @root.  However, in practice this case is not useful and hence
> +not allowed.

1. path can't be empty, as accordingly to previous point, it must start with '/'
2. path can be '/', which does exactly what you don't allow, and I don't see,
where it is restricted in code

> +
> +@expected_node may be None.

Which means that, we assert existence of the path except its last element,
yes? Worth mention this behavior here.

> +
> +@graph may be None or the result of an x-debug-query-block-graph
> +call that has already been performed.
> +"""
> +def assert_block_path(self, root, path, expected_node, graph=None):
> +if graph is None:
> +graph = self.qmp('x-debug-query-block-graph')['return']
> +
> +iter_path = iter(path.split('/'))
> +
> +# Must start with a /
> +assert next(iter_path) == ''
> +
> +node = next((node for node in graph['nodes'] if node['name'] == 
> root),
> +None)
> +
> +for path_node in iter_path:
> +assert node is not None, 'Cannot follow path %s' % path
> +
> +try:
> +node_id = next(edge['child'] for edge in graph['edges'] \
> + if edge['parent'] == node['id'] 
> and
> +edge['name'] == path_node)
> +
> +node = next(node for node in graph['nodes'] \
> + if node['id'] == node_id)

this line cant fail. If it fail, it means a bug in x-debug-query-block-graph, 
so,
I'd prefer to move it out of try:except block.

> +except StopIteration:
> +node = None
> +
> +assert node is not None or expected_node is None, \
> +   'No node found under %s (but expected %s)' % \
> +   (path, expected_node)
> +
> +assert expected_node is not None or node is None, \
> +   'Found node %s under %s (but expected none)' % \
> +   (node['name'], path)
> +
> +if node is not None and expected_node is not None:

[1]
second part of condition already asserted by previous assertion

> +assert node['name'] == expected_node, \
> +   'Found node %s under %s (but expected %s)' % \
> +   (node['name'], path, expected_node)

IMHO, it would be easier to read like:

   if node is None:
   assert  expected_node is None, \
  'No node found under %s (but expected %s)' % \
  (path, expected_node)
   else:
   assert expected_node is not None, \
  'Found node %s under %s (but expected none)' % \
  (node['name'], path)

   assert node['name'] == expected_node, \
  'Found node %s under %s (but expected %s)' % \
  (node['name'], path, expected_node)

Or even just

   if node is None:
   assert expected_node is None, \
  'No node found under %s (but expected %s)' % \
  (path, expected_node)
   else:
   assert node['name'] == expected_node, \
  'Found node %s under %s (but expected %s)' % \
  (node['name'], path, expected_node)

(I've checked:
 >>> 'erger %s erg' % None
'erger None erg'

Also, %-style formatting is old, as I understand it's better always use 
.format()
)

>   
>   index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
>   
> 
-- 
Best regards,
Vladimir

Re: [PATCH 0/5] tests: Enable fw_cfg tests on AArch64

[Thomas Huth]

On 03/12/2019 13.27, Xiang Zheng wrote:
> There are quite a few tests disabled on AArch64 such as fw_cfg-tests.
> This patch series fix some problems in test code and adapt it to
> virt machine.
> 
> Xiang Zheng (5):
>   tests: fw_cfg: Rename pc_fw_cfg_* to fw_cfg_*
>   tests: fw_cfg: Support read/write of fw_cfg registers on aarch64
>   tests: fw_cfg: Use virt as default machine in fw_cfg-test.c
>   hw/arm/virt: Add FW_CFG_RAM_SIZE and FW_CFG_MAX_CPUS into fw_cfg
>   tests: Enable fw_cfg test on aarch64

 Hi,

this breaks "make check-qtest-ppc64":

  TESTcheck-qtest-ppc64: tests/boot-order-test
**
ERROR:tests/boot-order-test.c:40:test_a_boot_order: assertion failed
(actual == expected_boot): (0x == 0x0063)

Please make sure that "make check" continuous to work with all other
targets, too.

 Thanks,
  Thomas

Re: virtiofsd: Where should it live?

[Paolo Bonzini]

On 26/11/19 13:14, Dr. David Alan Gilbert wrote:
>> IOW, if we did decide we want it in QEMU, then instead of
>> '$GIT/contrib/virtiofsd', I'd prefer to see '$GIT/virtiofsd'.
>
> I'm not sure it deserves a new top level for such a specific tool.
> 

It could be in fsdev/virtiofsd, but I agree with Daniel that at this
point the QEMU build system introduces baggage that you may not want for
virtiofsd.

Paolo

Re: [PATCH] hw/ppc/prep: Remove the deprecated "prep" machine and the OpenHackware BIOS

[Paolo Bonzini]

On 03/12/19 10:15, Thomas Huth wrote:
 Maybe we can rename this as read_boot_order_mm, and the previous
 read_boot_order_pc as read_boot_order_io.
>>>
>>> I don't think it makes much sense. This was completely specific to the
>>> "prep" machine, even the "40p" machine seems to prefer fw_cfg nowadays.
>>> So let's simply remove this old stuff.
>>>
 diff --git a/tests/endianness-test.c b/tests/endianness-test.c
 index 58527952a5..2798802c63 100644
 --- a/tests/endianness-test.c
 +++ b/tests/endianness-test.c
 @@ -35,7 +35,7 @@ static const TestCase test_cases[] = {
    { "mips64", "malta", 0x1000, .bswap = true },
    { "mips64el", "fulong2e", 0x1fd0 },
    { "ppc", "g3beige", 0xfe00, .bswap = true, .superio =
 "i82378" },
 -    { "ppc", "prep", 0x8000, .bswap = true },
 +    { "ppc", "40p", 0x8000, .bswap = true },
>>
>> ... here you access the Super I/O behind the PCI bridge via MMIO?
> 
> The difference is that this is an *arbitrary* address in I/O space
> there.

No, it's the base address of the ISA space, to which the tests add the
address of the pc-testdev device.  It's not any different from the
0x8000 in boot-order-test.

That said, I think it's a sensible objection that boot order doesn't
come from m48t59 on 40p (does it not?).  The m48t59-test could also be
adjusted to test the 40p... the right way to do it would be to have an
ISA bridge driver in qgraph, but that's a topic for a separate series.

Thanks,


Paolo

> It's not an address of a certain PCI device like the m48t59
> behind a PCI-bridge. As long as it's possible to write and read from
> this address, the test is working. Both, the "prep" and the "40p"
> machine have the "raven-pcihost" device at this address, so in this case
> the switch from "40p" to "prep" was easily possible.
> 
>  Thomas
> 
> 

[PULL 2/2] seabios: update to pre-1.13 snapshot again

[Sam Eiderman]

Hi,

Maybe we should add:

CONFIG_HOST_BIOS_GEOMETRY=n

to rom/config.seabios-128k and recreate the 128k image?

Re: [PATCH v2 2/3] s390x: Add missing vcpu reset functions

[Janosch Frank]

On 12/2/19 4:46 PM, Cornelia Huck wrote:
> On Mon,  2 Dec 2019 09:01:45 -0500
> Janosch Frank  wrote:
> 
>> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach
>> for the initial reset, which was also called for the clear reset. To
> 
> s/which/and that/ ?

Ok

> 
>> be architecture compliant, we also need to clear local interrupts on a
>> normal reset.
>>
>> Because of this and the upcoming protvirt support we need to add
>> ioctls for the missing clear and normal resets.
>>
>> Signed-off-by: Janosch Frank 
>> ---
>>  target/s390x/cpu.c   | 14 --
>>  target/s390x/kvm-stub.c  | 10 +-
>>  target/s390x/kvm.c   | 42 
>>  target/s390x/kvm_s390x.h |  4 +++-
>>  4 files changed, 58 insertions(+), 12 deletions(-)
>>
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index 829ce6ad54..906285888e 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -139,8 +139,18 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type 
>> type)
>>  }
>>  
>>  /* Reset state inside the kernel that we cannot access yet from QEMU. */
> 
> Hm, why does this comment talk about 'yet'? Did we have any plans to
> change that?

You're asking the wrong person :)

> 
>> -if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) {
>> -kvm_s390_reset_vcpu(cpu);
>> +if (kvm_enabled()) {
>> +switch (type) {
>> +case S390_CPU_RESET_CLEAR:
>> +kvm_s390_reset_vcpu_clear(cpu);
>> +break;
>> +case S390_CPU_RESET_INITIAL:
>> +kvm_s390_reset_vcpu_initial(cpu);
>> +break;
>> +case S390_CPU_RESET_NORMAL:
>> +kvm_s390_reset_vcpu_normal(cpu);
>> +break;
> 
> Add a default case to catch errors?

Sure, just did

> 
>> +}
>>  }
>>  }
>>  
> 
> (...)
> 
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index ad6e38c876..7a2ec8b9f8 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -151,6 +151,7 @@ static int cap_s390_irq;
>>  static int cap_ri;
>>  static int cap_gs;
>>  static int cap_hpage_1m;
>> +static int cap_vcpu_resets;
>>  
>>  static int active_cmma;
>>  
>> @@ -342,6 +343,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>  cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
>>  cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
>>  cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
>> +cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
>>  
>>  if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>>  || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
>> @@ -403,20 +405,44 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
>>  return 0;
>>  }
>>  
>> -void kvm_s390_reset_vcpu(S390CPU *cpu)
>> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
>>  {
>>  CPUState *cs = CPU(cpu);
>>  
>> -/* The initial reset call is needed here to reset in-kernel
>> - * vcpu data that we can't access directly from QEMU
>> - * (i.e. with older kernels which don't support sync_regs/ONE_REG).
>> - * Before this ioctl cpu_synchronize_state() is called in common kvm
>> - * code (kvm-all) */
>> -if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
>> -error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
>> +/*
>> + * The reset call is needed here to reset in-kernel vcpu data that
>> + * we can't access directly from QEMU (i.e. with older kernels
>> + * which don't support sync_regs/ONE_REG).  Before this ioctl
> 
> Is the reference to sync_regs/ONE_REG still relevant? I'm a bit
> confused here, especially with regard to what we'll need for protected
> virt.

I just didn't want to move/remove stuff
Even with kvm_run we do a lot of stuff for the initial reset.

> 
>> + * cpu_synchronize_state() is called in common kvm code
>> + * (kvm-all).
>> + */
>> +if (kvm_vcpu_ioctl(cs, type)) {
>> +error_report("CPU reset failed on CPU %i", cs->cpu_index);
>>  }
>>  }
>>  
>> +void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
>> +{
>> +kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET);
>> +}
>> +
>> +void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
>> +{
>> +if (!cap_vcpu_resets) {
>> +kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET);
>> +} else {
>> +kvm_s390_reset_vcpu(cpu, KVM_S390_CLEAR_RESET);
>> +}
> 
> kvm_s390_reset_vcpu(cpu, cap_vcpu_resets ? KVM_S390_CLEAR_RESET : 
> KVM_S390_INITIAL_RESET);
> 
> One line, but maybe the conventional if is still better :)

I'd like to keep it as is.

> 
>> +}
>> +
>> +void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
>> +{
>> +if (!cap_vcpu_resets) {
>> +return;
>> +}
>> +kvm_s390_reset_vcpu(cpu, KVM_S390_NORMAL_RESET);
>> +}
>> +
>>  static int can_sync_regs(CPUState *cs, int regs)
>>  {
>>  return cap_sync_regs && (cs->kvm_run->kvm_valid_regs & regs) == regs;
>>

Re: virtiofsd: Where should it live?

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonz...@redhat.com) wrote:
> On 03/12/19 14:02, Dr. David Alan Gilbert wrote:
> >> It could be in fsdev/virtiofsd,
> > fsdev is currently all 9p stuff, so that would seem very confusing.
> 
> Move it to fsdev/9p?

Greg: Are you OK with us doing that, and then having fsdev/virtiofsd for
our side of things?

> >> but I agree with Daniel that at this
> >> point the QEMU build system introduces baggage that you may not want for
> >> virtiofsd.
> >
> > I've already got it wired up in contrib with qemu's build system
> > so that doesn't seem to be an issue.   The question is purely a 'where'.
> 
> Oh I agree it's not an insurmountable problem.  For a new project I may
> not want to deal with the complicated rules.mak stuff; however, if
> virtiofsd doesn't have to do anything complicated then it's your choice.

Fortunately we don't seem to have touched that.

Dave

> Paolo
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: virtiofsd: Where should it live?

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonz...@redhat.com) wrote:
> On 26/11/19 13:14, Dr. David Alan Gilbert wrote:
> >> IOW, if we did decide we want it in QEMU, then instead of
> >> '$GIT/contrib/virtiofsd', I'd prefer to see '$GIT/virtiofsd'.
> >
> > I'm not sure it deserves a new top level for such a specific tool.
> > 
> 
> It could be in fsdev/virtiofsd,

fsdev is currently all 9p stuff, so that would seem very confusing.

> but I agree with Daniel that at this
> point the QEMU build system introduces baggage that you may not want for
> virtiofsd.

I've already got it wired up in contrib with qemu's build system
so that doesn't seem to be an issue.   The question is purely a 'where'.

Dave

> Paolo
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Custom logic gates on user space emulation

[burak sarac]

Hello All,
 Currently I am studying qemu and I want to figure out how I can use
custom logic gates on user space emulation.  I am searching very basic
'hello world' kind of tutorial or some resources to i.e. adding left
or LOR : 1 | 0 = 1 but 0 | 1 = 0 to existing x86 arch
((https://en.wikibooks.org/wiki/X86_Assembly/Logic) ?). What I want to
try is run this extended x86 version with qemu user space emulation.
Do I need a custom toolchain also for this? I found this book:
https://subscription.packtpub.com/book/hardware_and_creative/9781783289455/1/ch01lvl1sec15/generating-a-custom-toolchain-become-an-expert

Sorry for my ignorance in case it is totally irrelevant and I would
appreciate any guidance! Or pseudo kind of road map for me!

Thank you & have a nice day
Burak

Re: [PATCH v2 2/2] travis.yml: Run tcg tests with tci

[Thomas Huth]

On 28/11/2019 22.33, Stefan Weil wrote:
> Am 28.11.19 um 22:06 schrieb Stefan Weil:
> 
>> Am 28.11.19 um 16:35 schrieb Thomas Huth:
>>
>>> So far we only have compile coverage for tci. But since commit
>>> 2f160e0f9797c7522bfd0d09218d0c9340a5137c ("tci: Add implementation
>>> for INDEX_op_ld16u_i64") has been included now, we can also run the
>>> "tcg" and "qtest" tests with tci, so let's enable them in Travis now.
>>> Since we don't gain much additional test coverage by compiling all
>>> targets, and TCI is broken e.g. with the Sparc targets, we also limit
>>
>> As far as I know it is broken with Sparc hosts (not Sparc targets).
>>
>> I tested without limiting the target list on an x86_64 host, and the
>> tests passed.
> 
> 
> Sorry, I have to correct myself: check-qtest-sparc64 fails. I'll examine
> that.

The 32-bit sparc target fails, too - when doing the prom-env-test:

https://travis-ci.com/huth/qemu/jobs/262572937#L7690

 Thomas

Re: virtiofsd: Where should it live?

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berra...@redhat.com) wrote:
> On Tue, Dec 03, 2019 at 11:06:44AM +, Peter Maydell wrote:
> > On Tue, 3 Dec 2019 at 10:53, Dr. David Alan Gilbert  
> > wrote:
> > >
> > > We seem to be coming to the conclusion something that:
> > >
> > >   a) It should live in the qemu tree
> > >   b) It shouldn't live under contrib
> > >   c) We'll create a new top level, i.e. 'daemons'
> > >   d) virtiofsd will be daemons/virtiofsd
> > >
> > > Now, somethings I'm less clear on:
> > >   e) What else would move into daemons?  It was suggested
> > > that if we've got virtiofsd in there, then we should move
> > > libvhost-user - which I understand, but then it's not a
> > > 'daemons'.
> > > Are there any otehr daemons that should move?
> > 
> > I like the idea of a new top level directory, but I think
> > 'daemons' is a bit too specific -- for instance it seems to
> > me that qemu-img would be sensible to move out of the root,
> > and that's not a daemon.
> 
> Do we really need an extra directory level ?
> 
> IIUC, the main point against having $GIT_ROOT/virtiofsd is that
> the root of our repo is quite cluttered already.
> 
> Rather than trying to create a multi-level hierarchy which adds
> a debate around naming, why not address the clutter by moving
> *ALL* the .c/.h files out of the root so that we have a flatter
> tree:
> 
>   $GITROOT
> +- qemu-system
> |   +- vl.c
> |   +- ...most other files...

This seems like a good idea anyway (are all these files -system not
user?)

> +- qemu-img
> |   +- qemu-img.c
> +- qemu-nbd
> |   +- qemu-nbd.c
> +- qemu-io
> |   +- qemu-io.c
> |   +- qemu-io-cmds.c
> +- qemu-bridge-helper
> |   ...
> +- qemu-edid
> +- qemu-keymap
> +- qga  (already exists)

I'm not seeing how having ~ one directory per file is helping; the
number of entries doesn't drop much.

> Then we can add virtiofsd and other programs at the root with no big
> issue.

Dave

> 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 :|
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH] hw/ppc/prep: Remove the deprecated "prep" machine and the OpenHackware BIOS

[Thomas Huth]

On 03/12/2019 14.04, Paolo Bonzini wrote:
> On 03/12/19 10:15, Thomas Huth wrote:
> Maybe we can rename this as read_boot_order_mm, and the previous
> read_boot_order_pc as read_boot_order_io.

 I don't think it makes much sense. This was completely specific to the
 "prep" machine, even the "40p" machine seems to prefer fw_cfg nowadays.
 So let's simply remove this old stuff.

> diff --git a/tests/endianness-test.c b/tests/endianness-test.c
> index 58527952a5..2798802c63 100644
> --- a/tests/endianness-test.c
> +++ b/tests/endianness-test.c
> @@ -35,7 +35,7 @@ static const TestCase test_cases[] = {
>    { "mips64", "malta", 0x1000, .bswap = true },
>    { "mips64el", "fulong2e", 0x1fd0 },
>    { "ppc", "g3beige", 0xfe00, .bswap = true, .superio =
> "i82378" },
> -    { "ppc", "prep", 0x8000, .bswap = true },
> +    { "ppc", "40p", 0x8000, .bswap = true },
>>>
>>> ... here you access the Super I/O behind the PCI bridge via MMIO?
>>
>> The difference is that this is an *arbitrary* address in I/O space
>> there.
> 
> No, it's the base address of the ISA space, to which the tests add the
> address of the pc-testdev device.  It's not any different from the
> 0x8000 in boot-order-test.

Hmm, interesting. Why is it not necessary to set up the BARs of the
PCI-to-ISA bridge in this case?

> That said, I think it's a sensible objection that boot order doesn't
> come from m48t59 on 40p (does it not?).
Right. I'm also not an expert here, but I think the OpenBIOS on 40p
rather uses fw_cfg instead.

 Thomas

Re: virtiofsd: Where should it live?

[Paolo Bonzini]

On 03/12/19 14:02, Dr. David Alan Gilbert wrote:
>> It could be in fsdev/virtiofsd,
> fsdev is currently all 9p stuff, so that would seem very confusing.

Move it to fsdev/9p?

>> but I agree with Daniel that at this
>> point the QEMU build system introduces baggage that you may not want for
>> virtiofsd.
>
> I've already got it wired up in contrib with qemu's build system
> so that doesn't seem to be an issue.   The question is purely a 'where'.

Oh I agree it's not an insurmountable problem.  For a new project I may
not want to deal with the complicated rules.mak stuff; however, if
virtiofsd doesn't have to do anything complicated then it's your choice.

Paolo

Re: Network connection with COLO VM

[Dr. David Alan Gilbert]

* Daniel Cho (daniel...@qnap.com) wrote:
> Hi Dave,
> 
> We could use the exist interface to add netfilter and chardev, it might not
> have the problem you said.
> 
> However, the netfilter and chardev on the primary at the start, that means
> we could not dynamic set COLO
> feature to VM?

I wasn't expecting that to be possible - I'd expect you to be able
to start in a state that looks the same as a primary+failed secondary;
but I'm not sure.

> We try to change this chardev to prevent primary VM will stuck to wait
> secondary VM.
> 
> -chardev socket,id=compare1,host=127.0.0.1,port=9004,server,wait \
> 
> to
> 
> -chardev socket,id=compare1,host=127.0.0.1,port=9004,server,nowait \
> 
> But it will make primary VM's network not works. (Can't get ip), until
> starting connect with secondary VM.

I'm not sure of the answer to this; I've not tried doing it - I'm not
sure anyone has!
But, the colo components do track the state of tcp connections, so I'm
expecting that they have to already exist to have the state of those
connections available for when you start the secondary.


> Otherwise, the primary VM with netfileter / chardev and without netfilter /
> chardev , they takes very different
> booting time.
> Without  netfilter / chardev : about 1 mins
> With   netfilter / chardev : about 5 mins
> Is this an issue?

that sounds like it needs investigating.

Dave

> Best regards,
> Daniel Cho
> 
> 
> Dr. David Alan Gilbert  於 2019年12月2日 週一 下午5:58寫道：
> 
> > * Daniel Cho (daniel...@qnap.com) wrote:
> > > Hi Zhang,
> > >
> > > We use qemu-4.1.0 release on this case.
> > >
> > > I think we need use block mirror to sync the disk to secondary node
> > first,
> > > then stop the primary VM and build COLO system.
> > >
> > > In the stop moment, you need add some netfilter and chardev socket node
> > for
> > > COLO, maybe you need re-check this part.
> > >
> > >
> > > Our test was already follow those step. Maybe I could describe the detail
> > > of the test flow and issues.
> > >
> > >
> > > Step 1:
> > >
> > > Create primary VM without any netfilter and chardev for COLO, and using
> > > other host ping primary VM continually.
> > >
> > >
> > > Step 2:
> > >
> > > Create secondary VM (the same device/drive with primary VM), and do block
> > > mirror sync ( ping to primary VM normally )
> > >
> > >
> > > Step 3:
> > >
> > > After block mirror sync finish, add those netfilter and chardev to
> > primary
> > > VM and secondary VM for COLO ( *Can't* ping to primary VM but those
> > packets
> > > will be received later )
> > >
> > >
> > > Step 4:
> > >
> > > Start migrate primary VM to secondary VM, and primary VM & secondary VM
> > are
> > > running ( ping to primary VM works and receive those packets on step 3
> > > status )
> > >
> > >
> > >
> > >
> > > Between Step 3 to Step 4, it will take 10~20 seconds in our environment.
> > >
> > > I could image this issue (delay reply packets) is because of setting COLO
> > > proxy for temporary status,
> > >
> > > but we thought 10~20 seconds might a little long. (If primary VM is
> > already
> > > doing some jobs, it might lose the data.)
> > >
> > >
> > > Could we reduce those time? or those delay is depends on different VM?
> >
> > I think you need to set up the netfilter and chardev on the primary at
> > the start;  the filter contains the state of the TCP connections working
> > with the VM, so adding it later can't gain that state for existing
> > connections.
> >
> > Dave
> >
> > >
> > > Best Regard,
> > >
> > > Daniel Cho.
> > >
> > >
> > >
> > > Zhang, Chen  於 2019年11月30日 週六 上午2:04寫道：
> > >
> > > >
> > > >
> > > >
> > > >
> > > > *From:* Daniel Cho 
> > > > *Sent:* Friday, November 29, 2019 10:43 AM
> > > > *To:* Zhang, Chen 
> > > > *Cc:* Dr. David Alan Gilbert ;
> > lukasstra...@web.de;
> > > > qemu-devel@nongnu.org
> > > > *Subject:* Re: Network connection with COLO VM
> > > >
> > > >
> > > >
> > > > Hi David,  Zhang,
> > > >
> > > >
> > > >
> > > > Thanks for replying my question.
> > > >
> > > > We know why will occur this issue.
> > > >
> > > > As you said, the COLO VM's network needs
> > > >
> > > > colo-proxy to control packets, so the guest's
> > > >
> > > > interface should set the filter to solve the problem.
> > > >
> > > >
> > > >
> > > > But we found another question, when we set the
> > > >
> > > > fault-tolerance feature to guest (primary VM is running,
> > > >
> > > > secondary VM is pausing), the guest's network would not
> > > >
> > > > responds any request for a while (in our environment
> > > >
> > > > about 20~30 secs) after secondary VM runs.
> > > >
> > > >
> > > >
> > > > Does it be a normal situation, or a known issue?
> > > >
> > > >
> > > >
> > > > Our test is creating primary VM for a while, then creating
> > > >
> > > > secondary VM to make it with COLO feature.
> > > >
> > > >
> > > >
> > > > Hi Daniel,
> > > >
> > > >
> > > >
> > > > Happy to hear you have solved ssh disconnection issue.
> > > >
> > > >
> > > >
> > > > Do you use Lukas’s pa

[PATCH v3 1/4] Header sync

[Janosch Frank]

Sync in the new vcpu resets.

Signed-off-by: Janosch Frank 
---
 linux-headers/linux/kvm.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 3d9b18f7f8..f9fc3f6dc0 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1000,6 +1000,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PMU_EVENT_FILTER 173
 #define KVM_CAP_ARM_IRQ_LINE_LAYOUT_2 174
 #define KVM_CAP_HYPERV_DIRECT_TLBFLUSH 175
+#define KVM_CAP_S390_VCPU_RESETS 180
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -1461,6 +1462,9 @@ struct kvm_enc_region {
 /* Available with KVM_CAP_ARM_SVE */
 #define KVM_ARM_VCPU_FINALIZE_IOW(KVMIO,  0xc2, int)
 
+#define KVM_S390_NORMAL_RESET_IO(KVMIO,  0xc3)
+#define KVM_S390_CLEAR_RESET _IO(KVMIO,  0xc4)
+
 /* Secure Encrypted Virtualization command */
 enum sev_cmd_id {
/* Guest initialization commands */
-- 
2.20.1

[PATCH v3 2/4] s390x: Add missing vcpu reset functions

[Janosch Frank]

Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach
for the initial reset, and that was also called for the clear
reset. To be architecture compliant, we also need to clear local
interrupts on a normal reset.

Because of this and the upcoming protvirt support we need to add
ioctls for the missing clear and normal resets.

Signed-off-by: Janosch Frank 
Reviewed-by: Thomas Huth 
Acked-by: David Hildenbrand 
---
 target/s390x/cpu.c   | 16 +--
 target/s390x/kvm-stub.c  | 10 +-
 target/s390x/kvm.c   | 42 
 target/s390x/kvm_s390x.h |  4 +++-
 4 files changed, 60 insertions(+), 12 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 829ce6ad54..4973365d6c 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -139,8 +139,20 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type 
type)
 }
 
 /* Reset state inside the kernel that we cannot access yet from QEMU. */
-if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) {
-kvm_s390_reset_vcpu(cpu);
+if (kvm_enabled()) {
+switch (type) {
+case S390_CPU_RESET_CLEAR:
+kvm_s390_reset_vcpu_clear(cpu);
+break;
+case S390_CPU_RESET_INITIAL:
+kvm_s390_reset_vcpu_initial(cpu);
+break;
+case S390_CPU_RESET_NORMAL:
+kvm_s390_reset_vcpu_normal(cpu);
+break;
+default:
+g_assert_not_reached();
+}
 }
 }
 
diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c
index 5152e2bdf1..c4cd497f85 100644
--- a/target/s390x/kvm-stub.c
+++ b/target/s390x/kvm-stub.c
@@ -83,7 +83,15 @@ void kvm_s390_cmma_reset(void)
 {
 }
 
-void kvm_s390_reset_vcpu(S390CPU *cpu)
+void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
+{
+}
+
+void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
+{
+}
+
+void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
 {
 }
 
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index ad6e38c876..f633472980 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -151,6 +151,7 @@ static int cap_s390_irq;
 static int cap_ri;
 static int cap_gs;
 static int cap_hpage_1m;
+static int cap_vcpu_resets;
 
 static int active_cmma;
 
@@ -342,6 +343,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
 cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
 cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
 cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
+cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
 
 if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
 || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
@@ -403,17 +405,41 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
 return 0;
 }
 
-void kvm_s390_reset_vcpu(S390CPU *cpu)
+static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
 {
 CPUState *cs = CPU(cpu);
 
-/* The initial reset call is needed here to reset in-kernel
- * vcpu data that we can't access directly from QEMU
- * (i.e. with older kernels which don't support sync_regs/ONE_REG).
- * Before this ioctl cpu_synchronize_state() is called in common kvm
- * code (kvm-all) */
-if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
-error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
+/*
+ * The reset call is needed here to reset in-kernel vcpu data that
+ * we can't access directly from QEMU (i.e. with older kernels
+ * which don't support sync_regs/ONE_REG).  Before this ioctl
+ * cpu_synchronize_state() is called in common kvm code
+ * (kvm-all).
+ */
+if (kvm_vcpu_ioctl(cs, type)) {
+error_report("CPU reset failed on CPU %i type %lx",
+ cs->cpu_index, type);
+}
+}
+
+void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
+{
+kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET);
+}
+
+void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
+{
+if (cap_vcpu_resets) {
+kvm_s390_reset_vcpu(cpu, KVM_S390_CLEAR_RESET);
+} else {
+kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET);
+}
+}
+
+void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
+{
+if (cap_vcpu_resets) {
+kvm_s390_reset_vcpu(cpu, KVM_S390_NORMAL_RESET);
 }
 }
 
diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
index caf985955b..0b21789796 100644
--- a/target/s390x/kvm_s390x.h
+++ b/target/s390x/kvm_s390x.h
@@ -34,7 +34,9 @@ int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, 
uint32_t sch,
 int vq, bool assign);
 int kvm_s390_cmma_active(void);
 void kvm_s390_cmma_reset(void);
-void kvm_s390_reset_vcpu(S390CPU *cpu);
+void kvm_s390_reset_vcpu_clear(S390CPU *cpu);
+void kvm_s390_reset_vcpu_normal(S390CPU *cpu);
+void kvm_s390_reset_vcpu_initial(S390CPU *cpu);
 int kvm_s390_set_mem_limit(uint64_t new_limit, uint64_t *hw_limit);
 void kvm_s390_set_max_pagesize(uint64_t pagesize, Error **errp);
 void

Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask

[Christian Borntraeger]

On 03.12.19 14:28, Janosch Frank wrote:
> We need to set the short psw indication bit in the reset psw, as it is
> a short psw.
> 
> fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the 
> guest")
> Signed-off-by: Janosch Frank 

We should also add 
commit 24bb1fa36ff7b25ee774dbe4a18830dc782b54bf (HEAD, github-cohuck/s390-next)
Author: Janosch Frank 
AuthorDate: Fri Nov 29 09:20:23 2019 -0500
Commit: Cornelia Huck 
CommitDate: Mon Dec 2 09:58:57 2019 +0100

s390x: Properly fetch and test the short psw on diag308 subc 0/1

or whatever the final commit id will be. While this patch is not "broken"
it exposes the bug.



> ---
>  pc-bios/s390-ccw/jump2ipl.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index 266f1502b9..da13c43cc0 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -12,11 +12,11 @@
>  #define KERN_IMAGE_START 0x01UL
>  #define PSW_MASK_64 0x0001ULL
>  #define PSW_MASK_32 0x8000ULL
> -#define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64)
> +#define PSW_MASK_SHORTPSW 0x0008ULL
> +#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_32 | PSW_MASK_64)
>  
>  typedef struct ResetInfo {
> -uint32_t ipl_mask;
> -uint32_t ipl_addr;
> +uint64_t ipl_psw;
>  uint32_t ipl_continue;
>  } ResetInfo;
>  
> @@ -50,7 +50,9 @@ void jump_to_IPL_code(uint64_t address)
>  ResetInfo *current = 0;
>  
>  save = *current;
> -current->ipl_addr = (uint32_t) (uint64_t) &jump_to_IPL_2;
> +
> +current->ipl_psw = (uint64_t) &jump_to_IPL_2;
> +current->ipl_psw |= RESET_PSW_MASK;
>  current->ipl_continue = address & 0x7fff;
>  
>  debug_print_int("set IPL addr to", current->ipl_continue);
> @@ -82,7 +84,7 @@ void jump_to_low_kernel(void)
>  }
>  
>  /* Trying to get PSW at zero address */
> -if (*((uint64_t *)0) & IPL_PSW_MASK) {
> +if (*((uint64_t *)0) & RESET_PSW_MASK) {
>  jump_to_IPL_code((*((uint64_t *)0)) & 0x7fff);
>  }
>  
> 

[PATCH v3 3/4] s390x: Fix cpu normal reset ri clearing

[Janosch Frank]

As it turns out we need to clear the ri controls and PSW enablement
bit to be architecture compliant.

Signed-off-by: Janosch Frank 
Reviewed-by: Christian Borntraeger 
---
 target/s390x/cpu.c | 7 ++-
 target/s390x/cpu.h | 7 ++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 4973365d6c..c282c36b69 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -108,7 +108,7 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
 case S390_CPU_RESET_INITIAL:
 /* initial reset does not clear everything! */
 memset(&env->start_initial_reset_fields, 0,
-   offsetof(CPUS390XState, end_reset_fields) -
+   offsetof(CPUS390XState, start_normal_reset_fields) -
offsetof(CPUS390XState, start_initial_reset_fields));
 
 /* architectured initial value for Breaking-Event-Address register */
@@ -131,6 +131,11 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type 
type)
   &env->fpu_status);
/* fall through */
 case S390_CPU_RESET_NORMAL:
+env->psw.mask &= ~PSW_MASK_RI;
+memset(&env->start_normal_reset_fields, 0,
+   offsetof(CPUS390XState, end_reset_fields) -
+   offsetof(CPUS390XState, start_normal_reset_fields));
+
 env->pfault_token = -1UL;
 env->bpbc = false;
 break;
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index d5e18b096e..7f5fa1d35b 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -58,7 +58,6 @@ struct CPUS390XState {
  */
 uint64_t vregs[32][2] QEMU_ALIGNED(16);  /* vector registers */
 uint32_t aregs[16];/* access registers */
-uint8_t riccb[64]; /* runtime instrumentation control */
 uint64_t gscb[4];  /* guarded storage control */
 uint64_t etoken;   /* etoken */
 uint64_t etoken_extension; /* etoken extension */
@@ -114,6 +113,10 @@ struct CPUS390XState {
 uint64_t gbea;
 uint64_t pp;
 
+/* Fields up to this point are not cleared by normal CPU reset */
+struct {} start_normal_reset_fields;
+uint8_t riccb[64]; /* runtime instrumentation control */
+
 /* Fields up to this point are cleared by a CPU reset */
 struct {} end_reset_fields;
 
@@ -252,6 +255,7 @@ extern const VMStateDescription vmstate_s390_cpu;
 #undef PSW_SHIFT_ASC
 #undef PSW_MASK_CC
 #undef PSW_MASK_PM
+#undef PSW_MASK_RI
 #undef PSW_SHIFT_MASK_PM
 #undef PSW_MASK_64
 #undef PSW_MASK_32
@@ -274,6 +278,7 @@ extern const VMStateDescription vmstate_s390_cpu;
 #define PSW_MASK_CC 0x3000ULL
 #define PSW_MASK_PM 0x0F00ULL
 #define PSW_SHIFT_MASK_PM   40
+#define PSW_MASK_RI 0x0080ULL
 #define PSW_MASK_64 0x0001ULL
 #define PSW_MASK_32 0x8000ULL
 #define PSW_MASK_ESA_ADDR   0x7fffULL
-- 
2.20.1

[PATCH v3 0/4] s390x: Increase architectural compliance

[Janosch Frank]

On a cpu reset normal, we need to clear local cpus. Unfortunately we
need a new API for that, since KVM only exposes one of the three
resets.

Also we need to clear the riccb and the PSW ri mask on a normal reset.
And we need to set the short psw bit indication in the bios when doing
a reset.

Patches are also in my cleanup branch.
v2 of the KVM patch for the resets will be posted soon.

Janosch Frank (4):
  Header sync
  s390x: Add missing vcpu reset functions
  s390x: Fix cpu normal reset ri clearing
  pc-bios/s390x: Fix reset psw mask

 linux-headers/linux/kvm.h   |  4 
 pc-bios/s390-ccw/jump2ipl.c | 12 ++-
 target/s390x/cpu.c  | 23 +---
 target/s390x/cpu.h  |  7 ++-
 target/s390x/kvm-stub.c | 10 -
 target/s390x/kvm.c  | 42 ++---
 target/s390x/kvm_s390x.h|  4 +++-
 7 files changed, 83 insertions(+), 19 deletions(-)

-- 
2.20.1

Re: [PATCH for-5.0 v2 19/23] iotests: Resolve TODOs in 041

[Vladimir Sementsov-Ogievskiy]

03.12.2019 16:32, Vladimir Sementsov-Ogievskiy wrote:
> 11.11.2019 19:02, Max Reitz wrote:
>> Signed-off-by: Max Reitz
> 
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> 


Oops, stop. Why do you remove line "self.vm.shutdown()" ?

-- 
Best regards,
Vladimir

[PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask

[Janosch Frank]

We need to set the short psw indication bit in the reset psw, as it is
a short psw.

fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the 
guest")
Signed-off-by: Janosch Frank 
---
 pc-bios/s390-ccw/jump2ipl.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
index 266f1502b9..da13c43cc0 100644
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -12,11 +12,11 @@
 #define KERN_IMAGE_START 0x01UL
 #define PSW_MASK_64 0x0001ULL
 #define PSW_MASK_32 0x8000ULL
-#define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64)
+#define PSW_MASK_SHORTPSW 0x0008ULL
+#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_32 | PSW_MASK_64)
 
 typedef struct ResetInfo {
-uint32_t ipl_mask;
-uint32_t ipl_addr;
+uint64_t ipl_psw;
 uint32_t ipl_continue;
 } ResetInfo;
 
@@ -50,7 +50,9 @@ void jump_to_IPL_code(uint64_t address)
 ResetInfo *current = 0;
 
 save = *current;
-current->ipl_addr = (uint32_t) (uint64_t) &jump_to_IPL_2;
+
+current->ipl_psw = (uint64_t) &jump_to_IPL_2;
+current->ipl_psw |= RESET_PSW_MASK;
 current->ipl_continue = address & 0x7fff;
 
 debug_print_int("set IPL addr to", current->ipl_continue);
@@ -82,7 +84,7 @@ void jump_to_low_kernel(void)
 }
 
 /* Trying to get PSW at zero address */
-if (*((uint64_t *)0) & IPL_PSW_MASK) {
+if (*((uint64_t *)0) & RESET_PSW_MASK) {
 jump_to_IPL_code((*((uint64_t *)0)) & 0x7fff);
 }
 
-- 
2.20.1

Re: [PATCH for-5.0 v2 19/23] iotests: Resolve TODOs in 041

[Vladimir Sementsov-Ogievskiy]

11.11.2019 19:02, Max Reitz wrote:
> Signed-off-by: Max Reitz


Reviewed-by: Vladimir Sementsov-Ogievskiy 

-- 
Best regards,
Vladimir

Re: [PATCH v5 04/22] target/arm: Add helper_mte_check{1,2,3}

[Peter Maydell]

On Fri, 11 Oct 2019 at 14:49, Richard Henderson
 wrote:
>
> Implements the rules of "PE generation of Checked and Unchecked
> accesses" which aren't already implied by TB_FLAGS_MTE_ACTIVE.
> Implements the rules of "PE handling of Tag Check Failure".
>
> Does not implement tag physical address space, so all operations
> reduce to unchecked so far.
>
> Signed-off-by: Richard Henderson 
> ---
> v2: Fix TFSR update.
> v3: Split helper_mte_check per {1,2} IAs; take tbi data from translate.
> v5: Split helper_mte_check3, the only one that needs a runtime check for tbi.
> ---
>  target/arm/helper-a64.h|   4 +
>  target/arm/mte_helper.c| 167 +
>  target/arm/translate-a64.c |  15 +++-
>  target/arm/Makefile.objs   |   1 +
>  4 files changed, 186 insertions(+), 1 deletion(-)
>  create mode 100644 target/arm/mte_helper.c
>
> diff --git a/target/arm/helper-a64.h b/target/arm/helper-a64.h
> index a915c1247f..a82e21f15a 100644
> --- a/target/arm/helper-a64.h
> +++ b/target/arm/helper-a64.h
> @@ -102,3 +102,7 @@ DEF_HELPER_FLAGS_3(autda, TCG_CALL_NO_WG, i64, env, i64, 
> i64)
>  DEF_HELPER_FLAGS_3(autdb, TCG_CALL_NO_WG, i64, env, i64, i64)
>  DEF_HELPER_FLAGS_2(xpaci, TCG_CALL_NO_RWG_SE, i64, env, i64)
>  DEF_HELPER_FLAGS_2(xpacd, TCG_CALL_NO_RWG_SE, i64, env, i64)
> +
> +DEF_HELPER_FLAGS_2(mte_check1, TCG_CALL_NO_WG, i64, env, i64)
> +DEF_HELPER_FLAGS_2(mte_check2, TCG_CALL_NO_WG, i64, env, i64)
> +DEF_HELPER_FLAGS_3(mte_check3, TCG_CALL_NO_WG, i64, env, i64, i32)
> diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
> new file mode 100644
> index 00..bbb90cbe86
> --- /dev/null
> +++ b/target/arm/mte_helper.c
> @@ -0,0 +1,167 @@
> +/*
> + * ARM v8.5-MemTag Operations
> + *
> + * Copyright (c) 2019 Linaro, Ltd.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> .
> + */
> +
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "internals.h"
> +#include "exec/exec-all.h"
> +#include "exec/cpu_ldst.h"
> +#include "exec/helper-proto.h"
> +
> +
> +static int get_allocation_tag(CPUARMState *env, uint64_t ptr, uintptr_t ra)
> +{
> +/* Tag storage not implemented.  */
> +return -1;
> +}
> +
> +static int allocation_tag_from_addr(uint64_t ptr)
> +{
> +ptr += 1ULL << 55;  /* carry ptr[55] into ptr[59:56].  */
> +return extract64(ptr, 56, 4);

What's the carry-bit-55 logic for? The pseudocode
AArch64.AllocationTagFromAddress just returns bits [59:56].

> +}
> +
> +/*
> + * Perform a checked access for MTE.
> + * On arrival, TBI is known to enabled, as is allocation_tag_access_enabled.

"to be"

> + */
> +static uint64_t do_mte_check(CPUARMState *env, uint64_t dirty_ptr,
> + uint64_t clean_ptr, uint32_t select,
> + uintptr_t ra)
> +{
> +ARMMMUIdx stage1 = arm_stage1_mmu_idx(env);
> +int ptr_tag, mem_tag;
> +
> +/*
> + * If TCMA is enabled, then physical tag 0 is unchecked.
> + * Note the rules in D6.8.1 are written with logical tags, where
> + * the corresponding physical tag rule is simpler: equal to 0.
> + * We will need the physical tag below anyway.
> + */

This reads a bit oddly, because (in the final version of the spec)
physical and logical tags are identical (AArch64.PhysicalTag()
just returns bits [59:56] of the vaddr).

> +ptr_tag = allocation_tag_from_addr(dirty_ptr);
> +if (ptr_tag == 0) {
> +ARMVAParameters p = aa64_va_parameters(env, dirty_ptr, stage1, true);
> +if (p.tcma) {
> +return clean_ptr;
> +}
> +}

I don't think this logic gets the "regime has two address ranges"
case correct. For a two-address-range translation regime (where
TCR_ELx has TCMA0 and TCMA1 bits, rather than just a single TCMA bit),
then the 'select' argument to this function needs to be involved,
because we should do a tag-unchecked access if:
 * addr[59:55]==0b0 (ie select == 0 and ptr_tag == 0)
   and TCR_ELx.TCMA0 is set
 * addr[59:55]==0b1 (ie select == 1 and ptr_tag == 0xf)
   and TCR_ELx.TCMA1 is set
(the pseudocode for this is in AArch64.AccessTagIsChecked(),
and the TCR_EL1.TCMA[01] bit definitions agree; the text in
D6.8.1 appears to be confused.)

> +
> +/*
> + * If an access is made to an address that does not provide tag
> + * storage, the result is 

Re: [PATCH] hw/ppc/prep: Remove the deprecated "prep" machine and the OpenHackware BIOS

[Paolo Bonzini]

On 03/12/19 14:16, Thomas Huth wrote:
> On 03/12/2019 14.04, Paolo Bonzini wrote:
>> On 03/12/19 10:15, Thomas Huth wrote:
>> Maybe we can rename this as read_boot_order_mm, and the previous
>> read_boot_order_pc as read_boot_order_io.
>
> I don't think it makes much sense. This was completely specific to the
> "prep" machine, even the "40p" machine seems to prefer fw_cfg nowadays.
> So let's simply remove this old stuff.
>
>> diff --git a/tests/endianness-test.c b/tests/endianness-test.c
>> index 58527952a5..2798802c63 100644
>> --- a/tests/endianness-test.c
>> +++ b/tests/endianness-test.c
>> @@ -35,7 +35,7 @@ static const TestCase test_cases[] = {
>>    { "mips64", "malta", 0x1000, .bswap = true },
>>    { "mips64el", "fulong2e", 0x1fd0 },
>>    { "ppc", "g3beige", 0xfe00, .bswap = true, .superio =
>> "i82378" },
>> -    { "ppc", "prep", 0x8000, .bswap = true },
>> +    { "ppc", "40p", 0x8000, .bswap = true },

 ... here you access the Super I/O behind the PCI bridge via MMIO?
>>>
>>> The difference is that this is an *arbitrary* address in I/O space
>>> there.
>>
>> No, it's the base address of the ISA space, to which the tests add the
>> address of the pc-testdev device.  It's not any different from the
>> 0x8000 in boot-order-test.
> 
> Hmm, interesting. Why is it not necessary to set up the BARs of the
> PCI-to-ISA bridge in this case?

I don't know.  Maybe the machine doesn't need it, since these are not
really BARs, since they are positive-decode addresses on the ISA bus.
It may also be an emulation inaccuracy in the pc87312 ISA bridge.

Paolo

>> That said, I think it's a sensible objection that boot order doesn't
>> come from m48t59 on 40p (does it not?).
> Right. I'm also not an expert here, but I think the OpenBIOS on 40p
> rather uses fw_cfg instead.
> 
>  Thomas
> 

Re: [Qemu-devel] [PATCH 3/4] tests/docker: Add test-acceptance runner

[Alex Bennée]

Cleber Rosa  writes:

> On Mon, Aug 19, 2019 at 01:18:26AM +0200, Philippe Mathieu-Daudé wrote:
>> Add a runner script to be able to run acceptance tests within
>> Docker images. We can now reproduce Travis CI builds locally (and
>> debug  them!).
>> 
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  tests/docker/test-acceptance | 21 +
>>  1 file changed, 21 insertions(+)
>>  create mode 100755 tests/docker/test-acceptance
>> 
>> diff --git a/tests/docker/test-acceptance b/tests/docker/test-acceptance
>> new file mode 100755
>> index 00..84edaa676c
>> --- /dev/null
>> +++ b/tests/docker/test-acceptance
>> @@ -0,0 +1,21 @@
>> +#!/bin/bash -e
>> +#
>> +# Compile default Travis-CI target and run Avocado acceptance tests
>> +#
>> +# Copyright (c) 2019 Red Hat Inc.
>> +#
>> +# Authors:
>> +#  Philippe Mathieu-Daudé 
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2
>> +# or (at your option) any later version. See the COPYING file in
>> +# the top-level directory.
>> +
>> +. common.rc
>> +
>> +cd "$BUILD_DIR"
>> +
>> +DEF_TARGET_LIST="x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu"
>> +TARGET_LIST=${TARGET_LIST:-$DEF_TARGET_LIST} \
>> +build_qemu
>> +check_qemu check-acceptance
>> -- 
>> 2.20.1
>> 
>> 
>
> I'm currently seeing test errors when running in a container:
>
>   MKDIR   /tmp/qemu-test/build/tests/results
>   AVOCADO tests/acceptance
> JOB ID : fe56cc0b2d1adbc0b5bb5828902e113d596edccf
> JOB LOG: 
> /tmp/qemu-test/build/tests/results/job-2019-08-19T22.13-fe56cc0/job.log
>  (01/27) 
> /tmp/qemu-test/src/tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_x86_64_pc:
>   ERROR: join() argument must be str or bytes, not 'NoneType' (0.05 s)
> Interrupting job (failfast).
> RESULTS: PASS 0 | ERROR 1 | FAIL 0 | SKIP 26 | WARN 0 | INTERRUPT 0 | 
> CANCEL 0
> JOB TIME   : 0.26 s
> /tmp/qemu-test/src/tests/Makefile.include:1158: recipe for target 
> 'check-acceptance' failed
> make: *** [check-acceptance] Error 9
>
> That being said, I'm not running it under docker, but under podman,
> although I'm not convinced yet that is the defining issue.  I'll try
> to identify what's going here.

Was there a conclusion to the discussion about this series?

-- 
Alex Bennée

Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver

[Harish Jenny K N]

On 03/12/19 1:47 PM, Geert Uytterhoeven wrote:
> Hi Harish,
>
> On Tue, Dec 3, 2019 at 6:42 AM Harish Jenny K N
>  wrote:
>>> +static int gpio_aggregator_probe(struct platform_device *pdev)
>>> +{
>>> + struct device *dev = &pdev->dev;
>>> + struct gpio_desc **descs;
>>> + struct gpiochip_fwd *fwd;
>>> + int i, n;
>>> +
>>> + n = gpiod_count(dev, NULL);
>>> + if (n < 0)
>>> + return n;
>>> +
>>> + descs = devm_kmalloc_array(dev, n, sizeof(*descs), GFP_KERNEL);
>>> + if (!descs)
>>> + return -ENOMEM;
>>> +
>>> + for (i = 0; i < n; i++) {
>>> + descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
>> can you please add this check as well as we need to return EPROBE_DEFER.
>>
>> if (desc[i] == ERR_PTR(-ENOENT))
>> < return -EPROBE_DEFER;
> So gpiod_get_index() nevers return -EPROBE_DEFER, but returns -ENOENT
> instead?
> How can a driver distinguish between "GPIO not found" and "gpiochip driver
> not yet initialized"?
> Worse, so the *_optional() variants will return NULL in both cases, too, so
> the caller will always fall back to optional GPIO not present?
>
> Or am I missing something?
>
> Gr{oetje,eeting}s,
>
> Geert


We had earlier tested our changes on 4.14 kernel and the explicit return of 
-EPROBE_DEFER was needed in the inverter driver.

probably the commit 6662ae6af82df10259a70c7569b4c12ea7f3ba93 ( gpiolib: Keep 
returning EPROBE_DEFER when we should)

has fixed the issue and now it returns -EPROBE_DEFER.  you can ignore this 
comment as of now. I will test and let you know if needed.


Thanks,

Harish

Re: [PATCH v3 7/7] MAINTAINERS: Add GPIO Aggregator/Repeater section

[Harish Jenny K N]

On 27/11/19 2:12 PM, Geert Uytterhoeven wrote:
> Add a maintainership section for the GPIO Aggregator/Repeater, covering
> documentation, Device Tree bindings, and driver source code.
>
> Signed-off-by: Geert Uytterhoeven 
> ---
> Harish: Do you want to be listed as maintainer, too?


Yes. please.

>
> v3:
>   - New.
> ---
>  MAINTAINERS | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e5949b6827b72f2b..0f12ebdaa8faa76b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7043,6 +7043,14 @@ S: Maintained
>  F:   Documentation/firmware-guide/acpi/gpio-properties.rst
>  F:   drivers/gpio/gpiolib-acpi.c
>  
> +GPIO AGGREGATOR/REPEATER
> +M:   Geert Uytterhoeven 
> +L:   linux-g...@vger.kernel.org
> +S:   Maintained
> +F:   Documentation/admin-guide/gpio/gpio-aggregator.rst
> +F:   Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
> +F:   drivers/gpio/gpio-aggregator.c
> +
>  GPIO IR Transmitter
>  M:   Sean Young 
>  L:   linux-me...@vger.kernel.org

Re: [RFC] QEMU Gating CI

[Alex Bennée]

Cleber Rosa  writes:

> RFC: QEMU Gating CI
> ===
>
> This RFC attempts to address most of the issues described in
> "Requirements/GatinCI"[1].  An also relevant write up is the "State of
> QEMU CI as we enter 4.0"[2].
>
> The general approach is one to minimize the infrastructure maintenance
> and development burden, leveraging as much as possible "other people's"
> infrastructure and code.  GitLab's CI/CD platform is the most relevant
> component dealt with here.
>
> Problem Statement
> -
>
> The following is copied verbatim from Peter Maydell's write up[1]:
>
> "A gating CI is a prerequisite to having a multi-maintainer model of
> merging. By having a common set of tests that are run prior to a merge
> you do not rely on who is currently doing merging duties having access
> to the current set of test machines."
>
> This is of a very simplified view of the problem that I'd like to break
> down even further into the following key points:
>
>  * Common set of tests
>  * Pre-merge ("prior to a merge")
>  * Access to the current set of test machines
>  * Multi-maintainer model
>
> Common set of tests
> ~~~
>
> Before we delve any further, let's make it clear that a "common set of
> tests" is really a "dynamic common set of tests".  My point is that a
> set of tests in QEMU may include or exclude different tests depending
> on the environment.
>
> The exact tests that will be executed may differ depending on the
> environment, including:
>
>  * Hardware
>  * Operating system
>  * Build configuration
>  * Environment variables
>
> In the "State of QEMU CI as we enter 4.0" Alex Bennée listed some of
> those "common set of tests":
>
>  * check

Check encompasses a subset of the other checks - currently:

 - check-unit
 - check-qtest
 - check-block

The thing that stops other groups of tests being included is generally
are they solid on all the various hw/os/config/env setups you describe.
For example check-tcg currently fails gloriously on non-x86 with docker
enabled as it tries to get all the cross compiler images working.

>  * check-tcg
>  * check-softfloat
>  * check-block
>  * check-acceptance
>
> While Peter mentions that most of his checks are limited to:
>
>  * check
>  * check-tcg
>
> Our current inability to quickly identify a faulty test from test
> execution results (and specially in remote environments), and act upon
> it (say quickly disable it on a given host platform), makes me believe
> that it's fair to start a gating CI implementation that uses this
> rather coarse granularity.
>
> Another benefit is a close or even a 1:1 relationship between a common
> test set and an entry in the CI configuration.  For instance, the
> "check" common test set would map to a "make check" command in a
> "script:" YAML entry.
>
> To exemplify my point, if one specific test run as part of "check-tcg"
> is found to be faulty on a specific job (say on a specific OS), the
> entire "check-tcg" test set may be disabled as a CI-level maintenance
> action.

This would in this example eliminate practically all emulation testing
apart from the very minimal boot-codes that get spun up by the various
qtest migration tests. And of course the longer a group of tests is
disabled the larger the window for additional regressions to get in.

It may be a reasonable approach but it's not without consequence.

> Of course a follow up action to deal with the specific test
> is required, probably in the form of a Launchpad bug and patches
> dealing with the issue, but without necessarily a CI related angle to
> it.
>
> If/when test result presentation and control mechanism evolve, we may
> feel confident and go into finer grained granularity.  For instance, a
> mechanism for disabling nothing but "tests/migration-test" on a given
> environment would be possible and desirable from a CI management
> level.

The migration tests have found regressions although the problem has
generally been they were intermittent failures and hard to reproduce
locally. The last one took a few weeks of grinding to reproduce and get
patches together.

> Pre-merge
> ~
>
> The natural way to have pre-merge CI jobs in GitLab is to send "Merge
> Requests"[3] (abbreviated as "MR" from now on).  In most projects, a
> MR comes from individual contributors, usually the authors of the
> changes themselves.  It's my understanding that the current maintainer
> model employed in QEMU will *not* change at this time, meaning that
> code contributions and reviews will continue to happen on the mailing
> list.  A maintainer then, having collected a number of patches, would
> submit a MR either in addition or in substitution to the Pull Requests
> sent to the mailing list.
>
> "Pipelines for Merged Results"[4] is a very important feature to
> support the multi-maintainer model, and looks in practice, similar to
> Peter's "staging" branch approach, with an "automatic refresh" of the
> target branch.  It can give a mai

Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver

[Harish Jenny K N]

> +static int gpio_aggregator_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct gpio_desc **descs;
> + struct gpiochip_fwd *fwd;
> + int i, n;
> +
> + n = gpiod_count(dev, NULL);
> + if (n < 0)
> + return n;
> +
> + descs = devm_kmalloc_array(dev, n, sizeof(*descs), GFP_KERNEL);
> + if (!descs)
> + return -ENOMEM;
> +
> + for (i = 0; i < n; i++) {
> + descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);

can you please add this check as well as we need to return EPROBE_DEFER.

if (desc[i] == ERR_PTR(-ENOENT))
< return -EPROBE_DEFER;


> + if (IS_ERR(descs[i]))
> + return PTR_ERR(descs[i]);
> + }
> +
> + fwd = gpiochip_fwd_create(dev, n, descs);
> + if (IS_ERR(fwd))
> + return PTR_ERR(fwd);
> +
> + platform_set_drvdata(pdev, fwd);
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id gpio_aggregator_dt_ids[] = {
> + { .compatible = "gpio-repeater" },
> + /*
> +  * Add GPIO-operated devices controlled from userspace below,
> +  * or use "driver_override" in sysfs
> +  */
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, gpio_aggregator_dt_ids);
> +#endif
> +
> +static struct platform_driver gpio_aggregator_driver = {
> + .probe = gpio_aggregator_probe,
> + .driver = {
> + .name = DRV_NAME,
> + .groups = gpio_aggregator_groups,
> + .of_match_table = of_match_ptr(gpio_aggregator_dt_ids),
> + },
> +};
> +
> +static int __init gpio_aggregator_init(void)
> +{
> + return platform_driver_register(&gpio_aggregator_driver);
> +}
> +module_init(gpio_aggregator_init);
> +
> +static void __exit gpio_aggregator_exit(void)
> +{
> + gpio_aggregator_remove_all();
> + platform_driver_unregister(&gpio_aggregator_driver);
> +}
> +module_exit(gpio_aggregator_exit);
> +
> +MODULE_AUTHOR("Geert Uytterhoeven ");
> +MODULE_DESCRIPTION("GPIO Aggregator/Repeater");
> +MODULE_LICENSE("GPL v2");

Re: [RFC] QEMU Gating CI

[Stefan Hajnoczi]

On Mon, Dec 02, 2019 at 01:12:54PM -0500, Cleber Rosa wrote:
> On Mon, Dec 02, 2019 at 05:00:18PM +, Stefan Hajnoczi wrote:
> > On Mon, Dec 02, 2019 at 09:05:52AM -0500, Cleber Rosa wrote:
> > > RFC: QEMU Gating CI
> > > ===
> > 
> > Excellent, thank you for your work on this!
> > 
> > > 
> > > This RFC attempts to address most of the issues described in
> > > "Requirements/GatinCI"[1].  An also relevant write up is the "State of
> > > QEMU CI as we enter 4.0"[2].
> > > 
> > > The general approach is one to minimize the infrastructure maintenance
> > > and development burden, leveraging as much as possible "other people's"
> > > infrastructure and code.  GitLab's CI/CD platform is the most relevant
> > > component dealt with here.
> > > 
> > > Problem Statement
> > > -
> > > 
> > > The following is copied verbatim from Peter Maydell's write up[1]:
> > > 
> > > "A gating CI is a prerequisite to having a multi-maintainer model of
> > > merging. By having a common set of tests that are run prior to a merge
> > > you do not rely on who is currently doing merging duties having access
> > > to the current set of test machines."
> > > 
> > > This is of a very simplified view of the problem that I'd like to break
> > > down even further into the following key points:
> > > 
> > >  * Common set of tests
> > >  * Pre-merge ("prior to a merge")
> > >  * Access to the current set of test machines
> > >  * Multi-maintainer model
> > > 
> > > Common set of tests
> > > ~~~
> > > 
> > > Before we delve any further, let's make it clear that a "common set of
> > > tests" is really a "dynamic common set of tests".  My point is that a
> > > set of tests in QEMU may include or exclude different tests depending
> > > on the environment.
> > > 
> > > The exact tests that will be executed may differ depending on the
> > > environment, including:
> > > 
> > >  * Hardware
> > >  * Operating system
> > >  * Build configuration
> > >  * Environment variables
> > > 
> > > In the "State of QEMU CI as we enter 4.0" Alex Bennée listed some of
> > > those "common set of tests":
> > > 
> > >  * check
> > >  * check-tcg
> > >  * check-softfloat
> > >  * check-block
> > >  * check-acceptance
> > > 
> > > While Peter mentions that most of his checks are limited to:
> > > 
> > >  * check
> > >  * check-tcg
> > > 
> > > Our current inability to quickly identify a faulty test from test
> > > execution results (and specially in remote environments), and act upon
> > > it (say quickly disable it on a given host platform), makes me believe
> > > that it's fair to start a gating CI implementation that uses this
> > > rather coarse granularity.
> > > 
> > > Another benefit is a close or even a 1:1 relationship between a common
> > > test set and an entry in the CI configuration.  For instance, the
> > > "check" common test set would map to a "make check" command in a
> > > "script:" YAML entry.
> > > 
> > > To exemplify my point, if one specific test run as part of "check-tcg"
> > > is found to be faulty on a specific job (say on a specific OS), the
> > > entire "check-tcg" test set may be disabled as a CI-level maintenance
> > > action.  Of course a follow up action to deal with the specific test
> > > is required, probably in the form of a Launchpad bug and patches
> > > dealing with the issue, but without necessarily a CI related angle to
> > > it.
> > 
> > I think this coarse level of granularity is unrealistic.  We cannot
> > disable 99 tests because of 1 known failure.  There must be a way of
> > disabling individual tests.  You don't need to implement it yourself,
> > but I think this needs to be solved by someone before a gating CI can be
> > put into use.
> >
> 
> IMO it should be realistic if you look at it from a "CI related
> angle".  The pull request could still be revised and disable a single
> test because of a known failure, but this would not be necessarily
> related to the CI.

That sounds fine, thanks.  I interpreted the text a little differently.
I agree this functionality doesn't need to present in order to move to
GitLab.

> 
> > It probably involves adding a "make EXCLUDE_TESTS=foo,bar check"
> > variable so that .gitlab-ci.yml can be modified to exclude specific
> > tests on certain OSes.
> >
> 
> I certainly acknowledge the issue, but I don't think this (and many
> other issues that will certainly come up) should be a blocker to the
> transition to GitLab.
> 
> > > 
> > > If/when test result presentation and control mechanism evolve, we may
> > > feel confident and go into finer grained granularity.  For instance, a
> > > mechanism for disabling nothing but "tests/migration-test" on a given
> > > environment would be possible and desirable from a CI management level.
> > > 
> > > Pre-merge
> > > ~
> > > 
> > > The natural way to have pre-merge CI jobs in GitLab is to send "Merge
> > > Requests"[3] (abbreviated as "MR" from now on).  In most projects, a
> > > MR comes

Re: [PATCH v5 05/22] target/arm: Suppress tag check for sp+offset

[Peter Maydell]

On Fri, 11 Oct 2019 at 14:49, Richard Henderson
 wrote:
>
> R0078 specifies that base register, or base register plus immediate
> offset, is unchecked when the base register is SP.

It looks like rule-numbers didn't make it into the final Arm ARM,
so I guess the reference here would just be to section D6.8.1 ?

Also, this phrasing is slightly ambiguous about whether the
"when base is SP" condition applies to both "base register"
and "base register + immediate", or just to the last of the two;
the correct reading is the latter of these (and the D6.8.1
Arm ARM text is in error; trust the pseudocode here).

We could perhaps say something like:

D6.8.1 specifies that accesses are tag-unchecked for loads and
stores (including exclusives, compare-and-swap, etc) whose addresses are:
 * base-register only, where the base register is SP
 * base-register plus immediate, where the base register is SP
   (not including reg+imm with writeback addressing forms)
and also that literal (pc-relative) loads are tag-unchecked.

> Signed-off-by: Richard Henderson 
> ---
> v2: Include writeback addresses as checked.

The load-literal case is implicitly tag-unchecked because
the address calculation doesn't go via clean_data_tbi(), right?

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH v3 4/7] dt-bindings: gpio: Add gpio-repeater bindings

[Harish Jenny K N]

On 27/11/19 2:12 PM, Geert Uytterhoeven wrote:
> Add Device Tree bindings for a GPIO repeater, with optional translation
> of physical signal properties.  This is useful for describing explicitly
> the presence of e.g. an inverter on a GPIO line, and was inspired by the
> non-YAML gpio-inverter bindings by Harish Jenny K N
> [1].
>
> Note that this is different from a GPIO Nexus Node[2], which cannot do
> physical signal property translation.
>
> While an inverter can be described implicitly by exchanging the
> GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags, this has its limitations.
> Each GPIO line has only a single GPIO_ACTIVE_* flag, but applies to both
> th provider and consumer sides:
>   1. The GPIO provider (controller) looks at the flags to know the
>  polarity, so it can translate between logical (active/not active)
>  and physical (high/low) signal levels.
>   2. While the signal polarity is usually fixed on the GPIO consumer
>  side (e.g. an LED is tied to either the supply voltage or GND),
>  it may be configurable on some devices, and both sides need to
>  agree.  Hence the GPIO_ACTIVE_* flag as seen by the consumer must
>  match the actual polarity.
>  There exists a similar issue with interrupt flags, where both the
>  interrupt controller and the device generating the interrupt need
>  to agree, which breaks in the presence of a physical inverter not
>  described in DT (see e.g. [3]).
>
> [1] "[PATCH V4 2/2] gpio: inverter: document the inverter bindings"
> 
> https://lore.kernel.org/linux-gpio/1561699236-18620-3-git-send-email-harish_kand...@mentor.com/
>
> [2] Devicetree Specification v0.3-rc2, Section 2.5
> 
> https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.3-rc2
>
> [3] "[PATCH] wlcore/wl18xx: Add invert-irq OF property for physically
> inverted IRQ"
> 
> https://lore.kernel.org/linux-renesas-soc/20190607172958.20745-1-ero...@de.adit-jv.com/
>
> Signed-off-by: Geert Uytterhoeven 
> ---
> v3:
>   - New.
> ---
>  .../bindings/gpio/gpio-repeater.yaml  | 53 +++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-repeater.yaml 
> b/Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
> new file mode 100644
> index ..efdee0c3be43f731
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-repeater.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/gpio-repeater.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GPIO Repeater
> +
> +maintainers:
> +  - Harish Jenny K N 
> +  - Geert Uytterhoeven 
> +
> +description:
> +  This represents a repeater for one or more GPIOs, possibly including 
> physical
> +  signal property translation (e.g. polarity inversion).
> +
> +properties:
> +  compatible:
> +const: gpio-repeater
> +
> +  "#gpio-cells":
> +const: 2
> +
> +  gpio-controller: true
> +
> +  gpios:
> +description:
> +  Phandle and specifier, one for each repeated GPIO.
> +
> +  gpio-line-names:
> +description:
> +  Strings defining the names of the GPIO lines going out of the GPIO
> +  controller.
> +
> +required:
> +  - compatible
> +  - "#gpio-cells"
> +  - gpio-controller
> +  - gpios
> +
> +additionalProperties: false
> +
> +examples:
> +  # Device node describing a polarity inverter for a single GPIO
> +  - |
> +#include 
> +
> +inverter: gpio-repeater {
> +compatible = "gpio-repeater";
> +#gpio-cells = <2>;
> +gpio-controller;
> +gpios = <&gpio 95 GPIO_ACTIVE_LOW>;
> +};


just a suggestion: giving a gpio-line-names in the example would look useful.

Re: [PATCH v5 06/22] target/arm: Implement the IRG instruction

[Peter Maydell]

On Fri, 11 Oct 2019 at 14:49, Richard Henderson
 wrote:
>
> Signed-off-by: Richard Henderson 
> ---
> v2: Update to 00eac5.
> Merge choose_random_nonexcluded_tag into helper_irg since
> that pseudo function no longer exists separately.
> ---
>  target/arm/helper-a64.h|  1 +
>  target/arm/mte_helper.c| 57 ++
>  target/arm/translate-a64.c |  7 +
>  3 files changed, 65 insertions(+)
>
> diff --git a/target/arm/helper-a64.h b/target/arm/helper-a64.h
> index a82e21f15a..6ff7f5b756 100644
> --- a/target/arm/helper-a64.h
> +++ b/target/arm/helper-a64.h
> @@ -106,3 +106,4 @@ DEF_HELPER_FLAGS_2(xpacd, TCG_CALL_NO_RWG_SE, i64, env, 
> i64)
>  DEF_HELPER_FLAGS_2(mte_check1, TCG_CALL_NO_WG, i64, env, i64)
>  DEF_HELPER_FLAGS_2(mte_check2, TCG_CALL_NO_WG, i64, env, i64)
>  DEF_HELPER_FLAGS_3(mte_check3, TCG_CALL_NO_WG, i64, env, i64, i32)
> +DEF_HELPER_FLAGS_3(irg, TCG_CALL_NO_RWG, i64, env, i64, i64)
> diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
> index bbb90cbe86..9848849a91 100644
> --- a/target/arm/mte_helper.c
> +++ b/target/arm/mte_helper.c
> @@ -37,6 +37,31 @@ static int allocation_tag_from_addr(uint64_t ptr)
>  return extract64(ptr, 56, 4);
>  }
>
> +static int choose_nonexcluded_tag(int tag, int offset, uint16_t exclude)
> +{
> +if (exclude == 0x) {
> +return 0;
> +}
> +if (offset == 0) {
> +while (exclude & (1 << tag)) {
> +tag = (tag + 1) & 15;
> +}
> +} else {
> +do {
> +do {
> +tag = (tag + 1) & 15;
> +} while (exclude & (1 << tag));
> +} while (--offset > 0);
> +}

I feel like this would be easier to review if it matched
the logic the pseudocode uses, though I think the end result
comes out the same.

> +return tag;
> +}
> +
> +static uint64_t address_with_allocation_tag(uint64_t ptr, int rtag)
> +{
> +rtag -= extract64(ptr, 55, 1);
> +return deposit64(ptr, 56, 4, rtag);

This doesn't match AArch64.AddressWithAllocationTag -- the
fiddling with bit 55 is unwanted.

> +}
> +
>  /*
>   * Perform a checked access for MTE.
>   * On arrival, TBI is known to enabled, as is allocation_tag_access_enabled.
> @@ -165,3 +190,35 @@ uint64_t HELPER(mte_check3)(CPUARMState *env, uint64_t 
> dirty_ptr, uint32_t tbi)
>  return dirty_ptr;
>  }
>  }
> +
> +uint64_t HELPER(irg)(CPUARMState *env, uint64_t rn, uint64_t rm)
> +{
> +int el = arm_current_el(env);
> +uint64_t sctlr = arm_sctlr(env, el);
> +int rtag = 0;
> +
> +if (allocation_tag_access_enabled(env, el, sctlr)) {
> +/*
> + * Our IMPDEF choice for GCR_EL1.RRND==1 is to behave as if
> + * GCR_EL1.RRND==0, always producing deterministic results.
> + */
> +uint16_t exclude = extract32(rm | env->cp15.gcr_el1, 0, 16);
> +int start = extract32(env->cp15.rgsr_el1, 0, 4);
> +int seed = extract32(env->cp15.rgsr_el1, 8, 16);
> +int offset, i;
> +
> +/* RandomTag */
> +for (i = offset = 0; i < 4; ++i) {
> +/* NextRandomTagBit */
> +int top = (extract32(seed, 5, 1) ^ extract32(seed, 3, 1) ^
> +   extract32(seed, 2, 1) ^ extract32(seed, 0, 1));
> +seed = (top << 15) | (seed >> 1);
> +offset |= top << i;
> +}
> +rtag = choose_nonexcluded_tag(start, offset, exclude);
> +
> +env->cp15.rgsr_el1 = rtag | (seed << 8);
> +}
> +
> +return address_with_allocation_tag(rn, rtag);
> +}
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 18d45fba87..83d253d67f 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -5156,6 +5156,13 @@ static void disas_data_proc_2src(DisasContext *s, 
> uint32_t insn)
>  case 3: /* SDIV */
>  handle_div(s, true, sf, rm, rn, rd);
>  break;
> +case 4: /* IRG */
> +if (sf == 0 || !dc_isar_feature(aa64_mte_insn_reg, s)) {
> +goto do_unallocated;
> +}
> +gen_helper_irg(cpu_reg_sp(s, rd), cpu_env,
> +   cpu_reg_sp(s, rn), cpu_reg(s, rm));

In the case of "we only have mte_insn_reg, not full MTE",
the allocation tag we insert into the address must always
be zero, so you could just special case this and emit code
inline to clear bits [59:56]. The code as it stands works
because we ensure that the guest can't set the SCTLR.*ATA*
bits. (That's a bit inconsistent with our approach to the
PSTATE.TCO bit, which we do allow a guest to toggle, but
the inconsistency is permitted by the architecture.) I'm
not sure whether "we only have the EL0 visible bits" is
going to be a common enough config to care about to
special-case.

thanks
-- PMM

Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver

[Eugeniu Rosca]

Hi Geert,

On Wed, Nov 27, 2019 at 09:42:51AM +0100, Geert Uytterhoeven wrote:
> +static int gpio_aggregator_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct gpio_desc **descs;
> + struct gpiochip_fwd *fwd;
> + int i, n;

FWIW/FTR, doing some blind creation and deletion of gpio aggregator
chips [1] on R-Car H3ULCB overnight, kmemleak reported once [2]. Not
sure this is something 100% reproducible.

[1] while true; do \
   echo e6055400.gpio 12,13 > 
/sys/bus/platform/drivers/gpio-aggregator/new_device; \
   echo gpio-aggregator.0 > 
/sys/bus/platform/drivers/gpio-aggregator/delete_device; \
   done 

[2] unreferenced object 0x0006d2c2e000 (size 128):
  comm "kworker/3:1", pid 55, jiffies 4294676978 (age 38546.676s)
  hex dump (first 32 bytes):
00 d9 d2 d3 06 00 ff ff 0c 00 e0 0f ff ff ff ff  
01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
  backtrace:
[] slab_post_alloc_hook+0x8c/0x94
[<6f419a4f>] __kmalloc+0x170/0x218
[<60d185ea>] kobj_map+0x78/0x1c0
[] cdev_add+0x68/0x94
[] cdev_device_add+0x74/0x90
[<497871d3>] gpiochip_setup_dev+0x84/0x1f0
[] gpiochip_add_data_with_key+0xbcc/0x11f0
[] devm_gpiochip_add_data+0x60/0xa8
[<442e34c1>] gpio_aggregator_probe+0x210/0x3c8
[<076e13fb>] platform_drv_probe+0x70/0xe4
[] really_probe+0x2d8/0x434
[] driver_probe_device+0x15c/0x16c
[] __device_attach_driver+0xdc/0x120
[] bus_for_each_drv+0x12c/0x154
[] __device_attach+0x148/0x1e0
[] device_initial_probe+0x24/0x30

-- 
Best Regards,
Eugeniu

Re: [PATCH v37 10/17] target/avr: Add instruction disassembly function

[Michael Rolnik]

Hi Philippe.

I copied Richard's file and modified it's content, that's why Richard is
there.

Regards,
Michael Rolnik

On Tue, Dec 3, 2019 at 1:18 PM Philippe Mathieu-Daudé 
wrote:

> On 12/2/19 8:04 AM, Michael Rolnik wrote:
> > Aleksandar.
> >
> > If this code is going to be merge in 2019 I should modify al the
> > copyrights, right. or should I put 2020 in?
>
> Usually the copyright date is when you first contributed your code to
> the world (here, the list). If a patch was on the list in 2018, even if
> you made modifications and repost it, (c) is 2018.
>
> IOW, If your series gets merged in 2020, it will be merged as (c) 2019.
>
> I'm not sure why Richard's (c) appears here, is target/avr/disas.c based
> on target/openrisc/disas.c? Then it looks correct to me, but IANAL.
>
> > On Mon, Dec 2, 2019 at 2:28 AM Aleksandar Markovic
> > mailto:aleksandar.m.m...@gmail.com>>
> wrote:
> >
> >
> >
> > On Wednesday, November 27, 2019, Michael Rolnik  > > wrote:
> >
> > Provide function disassembles executed instruction when `-d
> > in_asm` is
> > provided
> >
> > Example:
> > `./avr-softmmu/qemu-system-avr -bios
> > free-rtos/Demo/AVR_ATMega2560_GCC/demo.elf -d in_asm` will
> > produce something like the following
> >
> > ```
> >  ...
> >  IN:
> >  0x014a:  CALL  0x3808
> >
> >  IN: main
> >  0x3808:  CALL  0x4b4
> >
> >  IN: vParTestInitialise
> >  0x04b4:  LDI   r24, 255
> >  0x04b6:  STS   r24, 0
> >  0x04b8:  MULS  r16, r20
> >  0x04ba:  OUT   $1, r24
> >  0x04bc:  LDS   r24, 0
> >  0x04be:  MULS  r16, r20
> >  0x04c0:  OUT   $2, r24
> >  0x04c2:  RET
> >  ...
> > ```
> >
> > Signed-off-by: Michael Rolnik  > >
> > Suggested-by: Richard Henderson  > >
> > Suggested-by: Philippe Mathieu-Daudé  > >
> > Suggested-by: Aleksandar Markovic  > >
> > Reviewed-by: Philippe Mathieu-Daudé  > >
> > Tested-by: Philippe Mathieu-Daudé  > >
> > ---
> >   target/avr/cpu.h   |   1 +
> >   target/avr/cpu.c   |   2 +-
> >   target/avr/disas.c | 228
> > +
> >   target/avr/translate.c |  11 ++
> >   4 files changed, 241 insertions(+), 1 deletion(-)
> >   create mode 100644 target/avr/disas.c
> >
> > diff --git a/target/avr/cpu.h b/target/avr/cpu.h
> > index 9ea5260165..a3e615a1eb 100644
> > --- a/target/avr/cpu.h
> > +++ b/target/avr/cpu.h
> > @@ -157,6 +157,7 @@ bool avr_cpu_exec_interrupt(CPUState *cpu,
> > int int_req);
> >   hwaddr avr_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
> >   int avr_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int
> > reg);
> >   int avr_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf,
> > int reg);
> > +int avr_print_insn(bfd_vma addr, disassemble_info *info);
> >
> >   static inline int avr_feature(CPUAVRState *env, int feature)
> >   {
> > diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> > index dae56d7845..52ec21dd16 100644
> > --- a/target/avr/cpu.c
> > +++ b/target/avr/cpu.c
> > @@ -83,7 +83,7 @@ static void avr_cpu_reset(CPUState *cs)
> >   static void avr_cpu_disas_set_info(CPUState *cpu,
> > disassemble_info *info)
> >   {
> >   info->mach = bfd_arch_avr;
> > -info->print_insn = NULL;
> > +info->print_insn = avr_print_insn;
> >   }
> >
> >   static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
> > diff --git a/target/avr/disas.c b/target/avr/disas.c
> > new file mode 100644
> > index 00..a51ade7c2a
> > --- /dev/null
> > +++ b/target/avr/disas.c
> > @@ -0,0 +1,228 @@
> > +/*
> > + * AVR disassembler
> > + *
> > + * Copyright (c) 2018 Richard Henderson  > >
> >
> >
> > Just a detail: since this file is created in 2019, the copyright
> > year should be 2019 too.
> >
> > + * Copyright (c) 2019 Michael Rolnik  > >
> > + *
> > + * This program is free software: you can redistribute it
> > and/or modify
> > + * it under the terms of the GNU General Public License as
> > published by
> > + * the 

Re: [PATCH] virtio-serial-bus: fix memory leak while attach virtio-serial-bus

[Amit Shah]

On Tue, 2019-12-03 at 00:37 -0500, Michael S. Tsirkin wrote:
> On Tue, Dec 03, 2019 at 08:53:42AM +0800, pannengyuan wrote:
> > 
> > 
> > On 2019/12/2 21:58, Laurent Vivier wrote:
> > > On 02/12/2019 12:15, pannengy...@huawei.com wrote:
> > > > From: PanNengyuan 
> > > > 
> > > > ivqs/ovqs/c_ivq/c_ovq is forgot to cleanup in
> > > > virtio_serial_device_unrealize, the memory leak stack is as
> > > > bellow:
> > > > 
> > > > Direct leak of 1290240 byte(s) in 180 object(s) allocated from:
> > > > #0 0x7fc9bfc27560 in calloc
> > > > (/usr/lib64/libasan.so.3+0xc7560)
> > > > #1 0x7fc9bed6f015 in g_malloc0 (/usr/lib64/libglib-
> > > > 2.0.so.0+0x50015)
> > > > #2 0x5650e02b83e7 in virtio_add_queue /mnt/sdb/qemu-4.2.0-
> > > > rc0/hw/virtio/virtio.c:2327
> > > > #3 0x5650e02847b5 in virtio_serial_device_realize
> > > > /mnt/sdb/qemu-4.2.0-rc0/hw/char/virtio-serial-bus.c:1089
> > > > #4 0x5650e02b56a7 in virtio_device_realize /mnt/sdb/qemu-
> > > > 4.2.0-rc0/hw/virtio/virtio.c:3504
> > > > #5 0x5650e03bf031 in device_set_realized /mnt/sdb/qemu-
> > > > 4.2.0-rc0/hw/core/qdev.c:876
> > > > #6 0x5650e0531efd in property_set_bool /mnt/sdb/qemu-4.2.0-
> > > > rc0/qom/object.c:2080
> > > > #7 0x5650e053650e in object_property_set_qobject
> > > > /mnt/sdb/qemu-4.2.0-rc0/qom/qom-qobject.c:26
> > > > #8 0x5650e0533e14 in object_property_set_bool
> > > > /mnt/sdb/qemu-4.2.0-rc0/qom/object.c:1338
> > > > #9 0x5650e04c0e37 in virtio_pci_realize /mnt/sdb/qemu-
> > > > 4.2.0-rc0/hw/virtio/virtio-pci.c:1801
> > > > 
> > > > Reported-by: Euler Robot 
> > > > Signed-off-by: PanNengyuan 
> > > > ---
> > > >  hw/char/virtio-serial-bus.c | 6 ++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-
> > > > serial-bus.c
> > > > index 3325904..da9019a 100644
> > > > --- a/hw/char/virtio-serial-bus.c
> > > > +++ b/hw/char/virtio-serial-bus.c
> > > > @@ -1126,9 +1126,15 @@ static void
> > > > virtio_serial_device_unrealize(DeviceState *dev, Error **errp)
> > > >  {
> > > >  VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > > >  VirtIOSerial *vser = VIRTIO_SERIAL(dev);
> > > > +int i;
> > > >  
> > > >  QLIST_REMOVE(vser, next);
> > > >  
> > > > +for (i = 0; i <= vser->bus.max_nr_ports; i++) {
> > > > +virtio_del_queue(vdev, 2 * i);
> > > > +virtio_del_queue(vdev, 2 * i + 1);
> > > > +}
> > > > +
> > > 
> > > According to virtio_serial_device_realize() and the number of
> > > virtio_add_queue(), I think you have more queues to delete:
> > > 
> > >   4 + 2 * vser->bus.max_nr_ports
> > > 
> > > (for vser->ivqs[0], vser->ovqs[0], vser->c_ivq, vser->c_ovq,
> > > vser->ivqs[i], vser->ovqs[i]).
> > > 
> > > Thanks,
> > > Laurent
> > > 
> > > 
> > 
> > Thanks, but I think the queues is correct, the queues in
> > virtio_serial_device_realize is as follow:
> > 
> > // here is 2
> > vser->ivqs[0] = virtio_add_queue(vdev, 128, handle_input);
> > vser->ovqs[0] = virtio_add_queue(vdev, 128, handle_output);
> > 
> > // here is 2
> > vser->c_ivq = virtio_add_queue(vdev, 32, control_in);
> > vser->c_ovq = virtio_add_queue(vdev, 32, control_out);
> > 
> > // here 2 * (max_nr_ports - 1)  - i is from 1 to max_nr_ports -
> > 1
> > for (i = 1; i < vser->bus.max_nr_ports; i++) {
> > vser->ivqs[i] = virtio_add_queue(vdev, 128, handle_input);
> > vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output);
> > }
> > 
> > so the total queues number is:  2 * (vser->bus.max_nr_ports + 1)
> 
> Rather than worry about this, I posted a patch adding
> virtio_delete_queue.
> How about reusing that, and just using ivqs/ovqs pointers?

Nice, that's cleaner.

>