date:20180830

Signed-off-by: Peng Hao 
Reviewed-by: Eduardo Habkost 
---
 hw/timer/mc146818rtc.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 6f1f723..b19ef51 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -70,6 +70,7 @@ typedef struct RTCState {
 ISADevice parent_obj;
 
 MemoryRegion io;
+MemoryRegion coalesced_io;
 uint8_t cmos_data[128];
 uint8_t cmos_index;
 int32_t base_year;
@@ -990,6 +991,13 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
 memory_region_init_io(&s->io, OBJECT(s), &cmos_ops, s, "rtc", 2);
 isa_register_ioport(isadev, &s->io, base);
 
+/* register rtc 0x70 port as coalesced_pio */
+memory_region_set_flush_coalesced(&s->io);
+memory_region_init_io(&s->coalesced_io, OBJECT(s), &cmos_ops,
+  s, "rtc1", 1);
+isa_register_ioport(isadev, &s->coalesced_io, base);
+memory_region_add_coalescing(&s->coalesced_io, 0, 1);
+
 qdev_set_legacy_instance_id(dev, base, 3);
 qemu_register_reset(rtc_reset, s);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH V5 0/4] introduce coalesced pio support

Coalesced pio is base on coalesced mmio and can be used for some port
like rtc port, pci-host config port, virtio-pci config port and so on.

Specially in case of rtc as coalesced pio, some versions of windows guest
access rtc frequently because of rtc as system tick. guest access rtc like
this: write register index to 0x70, then write or read data from 0x71.
writing 0x70 port is just as index and do nothing else. So we can use
coalesced pio to handle this scene to reduce VM-EXIT time.

When it starts and closes the virtual machine, it will access pci-host config
port or virtio-pci config port frequently. So setting these port as
coalesced pio can reduce startup and shutdown time. In qemu I just realize
piixfx's pci-host, it is convenient for other pci-host type implementations.

without my patch, get the vm-exit time of accessing rtc 0x70 and piix 0xcf8
using perf tools: (guest OS : windows 7 64bit)
IO Port Access  Samples Samples%  Time%  Min Time  Max Time  Avg time
0x70:POUT86 30.99%74.59%   9us  29us10.75us (+- 3.41%)
0xcf8:POUT 1119 2.60% 2.12%   2.79us56.83us 3.41us (+- 2.23%)

with my patch
IO Port Access  Samples Samples%  Time%   Min Time  Max Time   Avg time
0x70:POUT   10632.02%29.47%0us  10us 1.57us (+- 7.38%)
0xcf8:POUT  10651.67% 0.28%   0.41us65.44us   0.66us (+- 10.55%)

These are just qemu's patches, another patches are for kernel.

Changes v4 --> v5:
   update kvm header, improve compatibility.

Changes v3 --> v4
   modify coalesced_mmio_{add|del} to coalesced_io_{add|del}
   delete unnecessary macro define

Peng Hao (4):
  target-i386: introduce coalesced_pio kvm header update
  target-i386: add coalesced_pio API
  target-i386: add rtc 0x70 port as coalesced_pio
  target-i386: add i440fx 0xcf8 port as coalesced_pio

 accel/kvm/kvm-all.c   | 61 ++-
 hw/pci-host/piix.c|  4 
 hw/timer/mc146818rtc.c|  8 +++
 include/exec/memory.h |  4 ++--
 linux-headers/linux/kvm.h | 11 +++--
 memory.c  |  4 ++--
 6 files changed, 80 insertions(+), 12 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH V5 2/4] target-i386:add coalesced_pio API

Signed-off-by: Peng Hao 
Reviewed-by: Eduardo Habkost 
---
 accel/kvm/kvm-all.c   | 57 +++
 include/exec/memory.h |  4 ++--
 memory.c  |  4 ++--
 3 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 4a3909d..11d8d78 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -78,6 +78,7 @@ struct KVMState
 int fd;
 int vmfd;
 int coalesced_mmio;
+int coalesced_pio;
 struct kvm_coalesced_mmio_ring *coalesced_mmio_ring;
 bool coalesced_flush_in_progress;
 int vcpu_events;
@@ -559,6 +560,45 @@ static void kvm_uncoalesce_mmio_region(MemoryListener 
*listener,
 }
 }
 
+static void kvm_coalesce_pio_add(MemoryListener *listener,
+MemoryRegionSection *section,
+hwaddr start, hwaddr size)
+{
+KVMState *s = kvm_state;
+
+if (s->coalesced_pio) {
+struct kvm_coalesced_mmio_zone zone;
+
+zone.addr = start;
+zone.size = size;
+zone.pio = 1;
+
+(void)kvm_vm_ioctl(s, KVM_REGISTER_COALESCED_MMIO, &zone);
+}
+}
+
+static void kvm_coalesce_pio_del(MemoryListener *listener,
+MemoryRegionSection *section,
+hwaddr start, hwaddr size)
+{
+KVMState *s = kvm_state;
+
+if (s->coalesced_pio) {
+struct kvm_coalesced_mmio_zone zone;
+
+zone.addr = start;
+zone.size = size;
+zone.pio = 1;
+
+(void)kvm_vm_ioctl(s, KVM_UNREGISTER_COALESCED_MMIO, &zone);
+ }
+}
+
+static MemoryListener kvm_coalesced_pio_listener = {
+.coalesced_io_add = kvm_coalesce_pio_add,
+.coalesced_io_del = kvm_coalesce_pio_del,
+};
+
 int kvm_check_extension(KVMState *s, unsigned int extension)
 {
 int ret;
@@ -1615,6 +1655,8 @@ static int kvm_init(MachineState *ms)
 }
 
 s->coalesced_mmio = kvm_check_extension(s, KVM_CAP_COALESCED_MMIO);
+s->coalesced_pio = s->coalesced_mmio &&
+   kvm_check_extension(s, KVM_CAP_COALESCED_PIO);
 
 #ifdef KVM_CAP_VCPU_EVENTS
 s->vcpu_events = kvm_check_extension(s, KVM_CAP_VCPU_EVENTS);
@@ -1687,13 +1729,15 @@ static int kvm_init(MachineState *ms)
 s->memory_listener.listener.eventfd_add = kvm_mem_ioeventfd_add;
 s->memory_listener.listener.eventfd_del = kvm_mem_ioeventfd_del;
 }
-s->memory_listener.listener.coalesced_mmio_add = kvm_coalesce_mmio_region;
-s->memory_listener.listener.coalesced_mmio_del = 
kvm_uncoalesce_mmio_region;
+s->memory_listener.listener.coalesced_io_add = kvm_coalesce_mmio_region;
+s->memory_listener.listener.coalesced_io_del = kvm_uncoalesce_mmio_region;
 
 kvm_memory_listener_register(s, &s->memory_listener,
  &address_space_memory, 0);
 memory_listener_register(&kvm_io_listener,
  &address_space_io);
+memory_listener_register(&kvm_coalesced_pio_listener,
+ &address_space_io);
 
 s->many_ioeventfds = kvm_check_many_ioeventfds();
 
@@ -1775,8 +1819,13 @@ void kvm_flush_coalesced_mmio_buffer(void)
 struct kvm_coalesced_mmio *ent;
 
 ent = &ring->coalesced_mmio[ring->first];
-
-cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
+if (ent->pio == 1) {
+address_space_rw(&address_space_io, ent->phys_addr,
+ MEMTXATTRS_UNSPECIFIED, ent->data,
+ ent->len, true);
+} else {
+cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
+}
 smp_wmb();
 ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
 }
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 448d41a..4600fa3 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -410,9 +410,9 @@ struct MemoryListener {
 bool match_data, uint64_t data, EventNotifier *e);
 void (*eventfd_del)(MemoryListener *listener, MemoryRegionSection *section,
 bool match_data, uint64_t data, EventNotifier *e);
-void (*coalesced_mmio_add)(MemoryListener *listener, MemoryRegionSection 
*section,
+void (*coalesced_io_add)(MemoryListener *listener, MemoryRegionSection 
*section,
hwaddr addr, hwaddr len);
-void (*coalesced_mmio_del)(MemoryListener *listener, MemoryRegionSection 
*section,
+void (*coalesced_io_del)(MemoryListener *listener, MemoryRegionSection 
*section,
hwaddr addr, hwaddr len);
 /* Lower = earlier (during add), later (during del) */
 unsigned priority;
diff --git a/memory.c b/memory.c
index e9cd446..8b0311e 100644
--- a/memory.c
+++ b/memory.c
@@ -2126,7 +2126,7 @@ static void 
memory_region_update_coalesced_range_as(MemoryRegion *

[Qemu-devel] [PATCH V5 1/4] target-i386: introduce coalesced_pio kvm header update

add coalesced_pio's struct and KVM_CAP_COALESCED_PIO header.

Signed-off-by: Peng Hao 
Reviewed-by: Eduardo Habkost 
---
 accel/kvm/kvm-all.c   |  4 ++--
 linux-headers/linux/kvm.h | 11 +--
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index eb7db92..4a3909d 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -536,7 +536,7 @@ static void kvm_coalesce_mmio_region(MemoryListener 
*listener,
 
 zone.addr = start;
 zone.size = size;
-zone.pad = 0;
+zone.pio = 0;
 
 (void)kvm_vm_ioctl(s, KVM_REGISTER_COALESCED_MMIO, &zone);
 }
@@ -553,7 +553,7 @@ static void kvm_uncoalesce_mmio_region(MemoryListener 
*listener,
 
 zone.addr = start;
 zone.size = size;
-zone.pad = 0;
+zone.pio = 0;
 
 (void)kvm_vm_ioctl(s, KVM_UNREGISTER_COALESCED_MMIO, &zone);
 }
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 98f389a..1cb0d41 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -420,13 +420,19 @@ struct kvm_run {
 struct kvm_coalesced_mmio_zone {
__u64 addr;
__u32 size;
-   __u32 pad;
+   union {
+   __u32 pad;
+   __u32 pio;
+   };
 };
 
 struct kvm_coalesced_mmio {
__u64 phys_addr;
__u32 len;
-   __u32 pad;
+   union {
+   __u32 pad;
+   __u32 pio;
+   };
__u8  data[8];
 };
 
@@ -949,6 +955,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_GET_MSR_FEATURES 153
 #define KVM_CAP_HYPERV_EVENTFD 154
 #define KVM_CAP_HYPERV_TLBFLUSH 155
+#define KVM_CAP_COALESCED_PIO 156
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
1.8.3.1

[Qemu-devel] [PATCH V5 4/4] target-i386: add i440fx 0xcf8 port as coalesced_pio

Signed-off-by: Peng Hao 
Reviewed-by: Eduardo Habkost 
---
 hw/pci-host/piix.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 0e60834..da73743 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -327,6 +327,10 @@ static void i440fx_pcihost_realize(DeviceState *dev, Error 
**errp)
 
 sysbus_add_io(sbd, 0xcfc, &s->data_mem);
 sysbus_init_ioports(sbd, 0xcfc, 4);
+
+/* register i440fx 0xcf8 port as coalesced pio */
+memory_region_set_flush_coalesced(&s->data_mem);
+memory_region_add_coalescing(&s->conf_mem, 0, 4);
 }
 
 static void i440fx_realize(PCIDevice *dev, Error **errp)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] vl:c: make sure that sockets are calculated correctly in '-smp X' case

On Wed, 29 Aug 2018 14:33:01 -0300
Eduardo Habkost  wrote:

> On Wed, Aug 29, 2018 at 04:32:01PM +0200, Igor Mammedov wrote:
> > commit
> >   (5cdc9b76e3 vl.c: Remove dead assignment)
> > removed sockets calculation when 'sockets' weren't provided on CLI
> > since there wasn't any users for it back then. Exiting checks
> > are neither reachable
> >} else if (sockets * cores * threads < cpus) {
> > or nor triggable
> >if (sockets * cores * threads > max_cpus)
> > so we weren't noticing wrong topology since then, since users
> > recalculate sockets adhoc on their own.
> > 
> > However with deprecation check it becomes noticable, for example
> >   -smp 2
> > will start printing warning:
> >   "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * 
> > threads (1) != maxcpus (2)"
> > calculating sockets if they weren't specified.
> > 
> > Fix it by returning back sockets calculation if it's omited on CLI.
> > 
> > Signed-off-by: Igor Mammedov 
> > ---
> >  vl.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/vl.c b/vl.c
> > index 7fd700e..333d638 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
> >  
> >  /* compute missing values, prefer sockets over cores over threads 
> > */
> >  if (cpus == 0 || sockets == 0) {
> > -sockets = sockets > 0 ? sockets : 1;
> >  cores = cores > 0 ? cores : 1;
> >  threads = threads > 0 ? threads : 1;
> >  if (cpus == 0) {
> > +sockets = sockets > 0 ? sockets : 1;
> >  cpus = cores * threads * sockets;
> > +} else {
> > +max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);  
> 
> We already have a "max_cpus = qemu_opt_get_number(...)" line in
> this function[1]:
> 
> /* compute missing values, prefer sockets over cores over threads */
> if (cpus == 0 || sockets == 0) {
> sockets = sockets > 0 ? sockets : 1;
> cores = cores > 0 ? cores : 1;
> threads = threads > 0 ? threads : 1;
> if (cpus == 0) {
> cpus = cores * threads * sockets;
> }
> } else if (cores == 0) {
> [...]
> } else if (threads == 0) {
> [...]
> } else if (sockets * cores * threads < cpus) {
> [...]
> }
> 
> max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); /* [1] */
> 
> /* [2] */
> 
> if (max_cpus < cpus) {
> error_report("maxcpus must be equal to or greater than smp");
> exit(1);
> }
> 
> Why not move the sockets == 0 check to [2]?
it won't work, since the rest 'else if' depends on sockets being non 0
which is ensured by (cpus == 0 || sockets == 0) branch, I can split this in 2
equivalent conditions. It's more code but it might be easier to read

diff --git a/vl.c b/vl.c
index 7fd700e..6320bdc 100644
--- a/vl.c
+++ b/vl.c
@@ -1208,14 +1208,18 @@ static void smp_parse(QemuOpts *opts)
 unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
 unsigned threads = qemu_opt_get_number(opts, "threads", 0);
 
+max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
 /* compute missing values, prefer sockets over cores over threads */
-if (cpus == 0 || sockets == 0) {
+if (cpus == 0) {
 sockets = sockets > 0 ? sockets : 1;
 cores = cores > 0 ? cores : 1;
 threads = threads > 0 ? threads : 1;
-if (cpus == 0) {
-cpus = cores * threads * sockets;
-}
+cpus = cores * threads * sockets;
+max_cpus = max_cpus > 0 ? max_cpus : cpus;
+} else if (sockets == 0) {
+cores = cores > 0 ? cores : 1;
+threads = threads > 0 ? threads : 1;
+sockets = max_cpus / (cores * threads);
 } else if (cores == 0) {
 threads = threads > 0 ? threads : 1;
 cores = cpus / (sockets * threads);
@@ -1231,8 +1235,6 @@ static void smp_parse(QemuOpts *opts)
 exit(1);
 }
 
-max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
-
 if (max_cpus < cpus) {
 error_report("maxcpus must be equal to or greater than smp");
 exit(1);


> 
> 
> > +sockets = !sockets ? max_cpus / (cores * threads) : 
> > sockets;  
> 
> The two patches in this thread make QEMU print a warning on a
> case that was never documented as invalid/deprecated: maxcpus now
> needs to be a multiple of cores*threads.
> 
> However, the error message doesn't make it obvious:
> 
>   $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3
>   qemu-system-x86_64: warning: Invalid CPU topology deprecated: sockets (2) * 
> cores (3) * threads (1) != maxcpus (7)
to me it look pretty obvious, fix one side of equation to make sure that VM
starts in the future.

> I thi

Re: [Qemu-devel] [PATCH] hostmem-memfd: honour share=on/off property

On Wed, 29 Aug 2018 14:36:00 +0200
Marc-André Lureau  wrote:

> Hi
> 
> On Wed, Aug 29, 2018 at 10:50 AM Igor Mammedov  wrote:
> >
> > On Tue, 28 Aug 2018 18:55:35 +0200
> > Marc-André Lureau  wrote:
> >  
> > > On Tue, Aug 28, 2018 at 6:54 PM Marc-André Lureau
> > >  wrote:  
> > > >
> > > > Hi
> > > >
> > > > On Tue, Aug 28, 2018 at 6:49 PM Igor Mammedov  
> > > > wrote:  
> > > > >
> > > > > On Tue, 28 Aug 2018 17:38:40 +0200
> > > > > Marc-André Lureau  wrote:
> > > > >  
> > > > > > The share=on/off property is used to modified mmap() MAP_SHARED
> > > > > > setting. Make it on by default for convenience and compatibility
> > > > > > reasons.  
> > > > > it would be nice to state reasons here.
> > > > >
> > > > > also flipping default to on is behavioral change when
> > > > > used with bind policy which could make backend initialization fail
> > > > > so CLI that used to work (if there are any users of this without 
> > > > > shared=on)
> > > > > won't work anymore.  
> > > >
> > > > memfd backend is passing shared=true to 
> > > > memory_region_init_ram_from_fd().
> > > >
> > > > The patch allows to tweak that (instead of ignoring the property),
> > > > while keeping the value to shared=true.  
> > >
> > > keeping the default value to shared=trrue.  
> > ok,
> > another question:
> >  does it make sense to allow "shared=false" at all?
> >  should we bail out if user asks for it?  
> 
> You may want to use memfd for convenience (no need to specify hugetlb
> mem-path for ex), without the share memory aspects.
ok, acked.

btw:
looking at memfd_backend_memory_alloc/file_backend_memory_alloc
both needlessly are are calling host_memory_backend_mr_inited()
which creates an illusion that alloc could be called multiple times
but it isn't, could you clean that up?

In addition a following error path needs fixing
memfd_backend_memory_alloc()
 if (fd == -1) {
  
return; 
 
}
it must set errp

> 
> (but the patch was mostly for completeness)
> 
> >  
> > >  
> > > >  
> > > > >  
> > > > > >
> > > > > > Signed-off-by: Marc-André Lureau 
> > > > > > ---
> > > > > >  backends/hostmem-memfd.c | 4 +++-
> > > > > >  qemu-options.hx  | 4 +++-
> > > > > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
> > > > > > index 1e20fe0ba8..abd52364db 100644
> > > > > > --- a/backends/hostmem-memfd.c
> > > > > > +++ b/backends/hostmem-memfd.c
> > > > > > @@ -59,7 +59,8 @@ memfd_backend_memory_alloc(HostMemoryBackend 
> > > > > > *backend, Error **errp)
> > > > > >
> > > > > >  name = object_get_canonical_path(OBJECT(backend));
> > > > > >  memory_region_init_ram_from_fd(&backend->mr, OBJECT(backend),
> > > > > > -   name, backend->size, true, fd, 
> > > > > > errp);
> > > > > > +   name, backend->size,
> > > > > > +   backend->share, fd, errp);
> > > > > >  g_free(name);
> > > > > >  }
> > > > > >
> > > > > > @@ -131,6 +132,7 @@ memfd_backend_instance_init(Object *obj)
> > > > > >
> > > > > >  /* default to sealed file */
> > > > > >  m->seal = true;
> > > > > > +MEMORY_BACKEND(m)->share = true;
> > > > > >  }
> > > > > >
> > > > > >  static void
> > > > > > diff --git a/qemu-options.hx b/qemu-options.hx
> > > > > > index f14e423b7c..25c02d72b8 100644
> > > > > > --- a/qemu-options.hx
> > > > > > +++ b/qemu-options.hx
> > > > > > @@ -4029,7 +4029,7 @@ Memory backend objects offer more control 
> > > > > > than the @option{-m} option that is
> > > > > >  traditionally used to define guest RAM. Please refer to
> > > > > >  @option{memory-backend-file} for a description of the options.
> > > > > >
> > > > > > -@item -object 
> > > > > > memory-backend-memfd,id=@var{id},merge=@var{on|off},dump=@var{on|off},prealloc=@var{on|off},size=@var{size},host-nodes=@var{host-nodes},policy=@var{default|preferred|bind|interleave},seal=@var{on|off},hugetlb=@var{on|off},hugetlbsize=@var{size}
> > > > > > +@item -object 
> > > > > > memory-backend-memfd,id=@var{id},merge=@var{on|off},dump=@var{on|off},share=@var{on|off},prealloc=@var{on|off},size=@var{size},host-nodes=@var{host-nodes},policy=@var{default|preferred|bind|interleave},seal=@var{on|off},hugetlb=@var{on|off},hugetlbsize=@var{size}
> > > > > >
> > > > > >  Creates an anonymous memory file backend object, which allows QEMU 
> > > > > > to
> > > > > >  share the memory with an external process (e.g. when using
> > > > > > @@ -4051,6 +4051,8 @@ with the @option{seal} option (requires at 
> > > > > > least Linux 4.16).
> > > > > >  Please refer to @option{memory-backend-file} for a description of 
> > > > > > the
> > > > > >  other options.
> > > > > >
> > > > > > +The @option{share} boolean option is @var{on} by default with 
> > > > > > me

Re: [Qemu-devel] [PATCH] hostmem-memfd: honour share=on/off property

On Tue, 28 Aug 2018 17:38:40 +0200
Marc-André Lureau  wrote:

> The share=on/off property is used to modified mmap() MAP_SHARED
> setting. Make it on by default for convenience and compatibility
> reasons.
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  backends/hostmem-memfd.c | 4 +++-
>  qemu-options.hx  | 4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/backends/hostmem-memfd.c b/backends/hostmem-memfd.c
> index 1e20fe0ba8..abd52364db 100644
> --- a/backends/hostmem-memfd.c
> +++ b/backends/hostmem-memfd.c
> @@ -59,7 +59,8 @@ memfd_backend_memory_alloc(HostMemoryBackend *backend, 
> Error **errp)
>  
>  name = object_get_canonical_path(OBJECT(backend));
>  memory_region_init_ram_from_fd(&backend->mr, OBJECT(backend),
> -   name, backend->size, true, fd, errp);
> +   name, backend->size,
> +   backend->share, fd, errp);
>  g_free(name);
>  }
>  
> @@ -131,6 +132,7 @@ memfd_backend_instance_init(Object *obj)
>  
>  /* default to sealed file */
>  m->seal = true;
> +MEMORY_BACKEND(m)->share = true;
>  }
>  
>  static void
> diff --git a/qemu-options.hx b/qemu-options.hx
> index f14e423b7c..25c02d72b8 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4029,7 +4029,7 @@ Memory backend objects offer more control than the 
> @option{-m} option that is
>  traditionally used to define guest RAM. Please refer to
>  @option{memory-backend-file} for a description of the options.
>  
> -@item -object 
> memory-backend-memfd,id=@var{id},merge=@var{on|off},dump=@var{on|off},prealloc=@var{on|off},size=@var{size},host-nodes=@var{host-nodes},policy=@var{default|preferred|bind|interleave},seal=@var{on|off},hugetlb=@var{on|off},hugetlbsize=@var{size}
> +@item -object 
> memory-backend-memfd,id=@var{id},merge=@var{on|off},dump=@var{on|off},share=@var{on|off},prealloc=@var{on|off},size=@var{size},host-nodes=@var{host-nodes},policy=@var{default|preferred|bind|interleave},seal=@var{on|off},hugetlb=@var{on|off},hugetlbsize=@var{size}
>  
>  Creates an anonymous memory file backend object, which allows QEMU to
>  share the memory with an external process (e.g. when using
> @@ -4051,6 +4051,8 @@ with the @option{seal} option (requires at least Linux 
> 4.16).
>  Please refer to @option{memory-backend-file} for a description of the
>  other options.
>  
> +The @option{share} boolean option is @var{on} by default with memfd.
> +
>  @item -object rng-random,id=@var{id},filename=@var{/dev/random}
>  
>  Creates a random number generator backend which obtains entropy from

Reviewed-by: Igor Mammedov

Re: [Qemu-devel] [PATCH v2 2/2] hw/arm: Add Arm Enterprise machine type

2018-08-30 Thread Leif Lindholm

On Thu, Aug 30, 2018 at 03:07:29PM +0800, Hongbo Zhang wrote:
> >> Yes, I am working on the v3, with main changes:
> >>  - machine name "sbsa-ref" (good name?)
> >>  - a separate file sbsa-ref.c
> >>  - don't touch the acpi c file, acpi will be supplied by uefi
> >
> > I agree with the above three bullets.
> >
> >>  - only necessary dt node, no other peripheral dt nodes
> >
> > I'm not sure what DT nodes are necessary. After discussing the
> > ACPI tables, and that firmware would supply them, I thought we
> > also agreed firmware would supply the DTB.
>
> Yes, firmware supply ACPI to load OS.
> But for DT, It is said that UEFI still needs some DT nodes, at least
> for memory size. I am trying to figure out which DT nodes are
> mandatory too, it is better we don't have any DT nodes left.
> 
> @Ard, @Leif, is there any possibility to remove all the DT nodes?
> On real hardware, how does UEFI find the memory size and CPU number?

Usually by asking some form of SCP/PMU.
So until we have rock-solid multi-cpu-instance (making terms up here,
there may be a proper name already?) support upstream, we need to take
shortcuts.

Using DT in the same way as we do for mach-virt when booting the OS
with ACPI seems the most logical choice to me.

The best alternative I can think of would be extending fw_cfg, but
that would then be lost effort once (if) we get to the abovementioned
situation, whereas the DT route comes for free.

/
Leif

Re: [Qemu-devel] [PATCH v2 2/2] hw/arm: Add Arm Enterprise machine type

2018-08-30 Thread Peter Maydell

On 30 August 2018 at 09:31, Leif Lindholm  wrote:
> On Thu, Aug 30, 2018 at 03:07:29PM +0800, Hongbo Zhang wrote:
>> @Ard, @Leif, is there any possibility to remove all the DT nodes?
>> On real hardware, how does UEFI find the memory size and CPU number?
>
> Usually by asking some form of SCP/PMU.
> So until we have rock-solid multi-cpu-instance (making terms up here,
> there may be a proper name already?) support upstream, we need to take
> shortcuts.

I would expect that you'd want a "faked-on-the-QEMU end" SCP
communications endpoint to start with anyway. We have one of
those for vexpress, for instance.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 01/20] memory-device: fix error message when hinted address is too small

2018-08-30 Thread Dr. David Alan Gilbert

* David Hildenbrand (da...@redhat.com) wrote:
> The "at" should actually be a "before".
> if (new_addr < address_space_start)
>  -> "can't add memory ... before... $address_space_start"
> 
> So it looks similar to the other check
> } else if ((new_addr + size) > address_space_end)
>  -> "can't add memory ... beyond..."
> 
> Signed-off-by: David Hildenbrand 
> ---
>  hw/mem/memory-device.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index 6de4f70bb4..efacbc2a7d 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -146,7 +146,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, 
> const uint64_t *hint,
>  new_addr = *hint;
>  if (new_addr < address_space_start) {
>  error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
> -   "] at 0x%" PRIx64, new_addr, size, 
> address_space_start);
> +   "] before 0x%" PRIx64, new_addr, size,
> +   address_space_start);

OK, so


Reviewed-by: Dr. David Alan Gilbert 

>  return 0;
>  } else if ((new_addr + size) > address_space_end) {

That comparison (that's already there) doesn't look wrap-safe?

Dave
>  error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64

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

Re: [Qemu-devel] [PATCH v2 02/20] memory-device: use memory device terminology in error messages

2018-08-30 Thread Dr. David Alan Gilbert

* David Hildenbrand (da...@redhat.com) wrote:
> While we rephrased most error messages, we missed these.
> 
> Signed-off-by: David Hildenbrand 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  hw/mem/memory-device.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index efacbc2a7d..0d9892b715 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -87,7 +87,7 @@ static void memory_device_check_addable(MachineState *ms, 
> uint64_t size,
>  memory_device_used_region_size(OBJECT(ms), &used_region_size);
>  if (used_region_size + size > ms->maxram_size - ms->ram_size) {
>  error_setg(errp, "not enough space, currently 0x%" PRIx64
> -   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
> +   " in use of total space for memory devices 0x" 
> RAM_ADDR_FMT,
> used_region_size, ms->maxram_size - ms->ram_size);
>  return;
>  }
> @@ -145,12 +145,12 @@ uint64_t memory_device_get_free_addr(MachineState *ms, 
> const uint64_t *hint,
>  if (hint) {
>  new_addr = *hint;
>  if (new_addr < address_space_start) {
> -error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
> +error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" 
> PRIx64
> "] before 0x%" PRIx64, new_addr, size,
> address_space_start);
>  return 0;
>  } else if ((new_addr + size) > address_space_end) {
> -error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
> +error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" 
> PRIx64
> "] beyond 0x%" PRIx64, new_addr, size,
> address_space_end);
>  return 0;
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] vl:c: make sure that sockets are calculated correctly in '-smp X' case

2018-08-30 Thread Andrew Jones

On Thu, Aug 30, 2018 at 09:58:51AM +0200, Igor Mammedov wrote:
> On Wed, 29 Aug 2018 14:33:01 -0300
> Eduardo Habkost  wrote:
> 
> > On Wed, Aug 29, 2018 at 04:32:01PM +0200, Igor Mammedov wrote:
> > > commit
> > >   (5cdc9b76e3 vl.c: Remove dead assignment)
> > > removed sockets calculation when 'sockets' weren't provided on CLI
> > > since there wasn't any users for it back then. Exiting checks
> > > are neither reachable
> > >} else if (sockets * cores * threads < cpus) {
> > > or nor triggable
> > >if (sockets * cores * threads > max_cpus)
> > > so we weren't noticing wrong topology since then, since users
> > > recalculate sockets adhoc on their own.
> > > 
> > > However with deprecation check it becomes noticable, for example
> > >   -smp 2
> > > will start printing warning:
> > >   "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * 
> > > threads (1) != maxcpus (2)"
> > > calculating sockets if they weren't specified.
> > > 
> > > Fix it by returning back sockets calculation if it's omited on CLI.
> > > 
> > > Signed-off-by: Igor Mammedov 
> > > ---
> > >  vl.c | 5 -
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/vl.c b/vl.c
> > > index 7fd700e..333d638 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
> > >  
> > >  /* compute missing values, prefer sockets over cores over 
> > > threads */
> > >  if (cpus == 0 || sockets == 0) {
> > > -sockets = sockets > 0 ? sockets : 1;
> > >  cores = cores > 0 ? cores : 1;
> > >  threads = threads > 0 ? threads : 1;
> > >  if (cpus == 0) {
> > > +sockets = sockets > 0 ? sockets : 1;
> > >  cpus = cores * threads * sockets;
> > > +} else {
> > > +max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);  
> > 
> > We already have a "max_cpus = qemu_opt_get_number(...)" line in
> > this function[1]:
> > 
> > /* compute missing values, prefer sockets over cores over threads */
> > if (cpus == 0 || sockets == 0) {
> > sockets = sockets > 0 ? sockets : 1;
> > cores = cores > 0 ? cores : 1;
> > threads = threads > 0 ? threads : 1;
> > if (cpus == 0) {
> > cpus = cores * threads * sockets;
> > }
> > } else if (cores == 0) {
> > [...]
> > } else if (threads == 0) {
> > [...]
> > } else if (sockets * cores * threads < cpus) {
> > [...]
> > }
> > 
> > max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); /* [1] */
> > 
> > /* [2] */
> > 
> > if (max_cpus < cpus) {
> > error_report("maxcpus must be equal to or greater than smp");
> > exit(1);
> > }
> > 
> > Why not move the sockets == 0 check to [2]?
> it won't work, since the rest 'else if' depends on sockets being non 0
> which is ensured by (cpus == 0 || sockets == 0) branch, I can split this in 2
> equivalent conditions. It's more code but it might be easier to read
> 
> diff --git a/vl.c b/vl.c
> index 7fd700e..6320bdc 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1208,14 +1208,18 @@ static void smp_parse(QemuOpts *opts)
>  unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
>  unsigned threads = qemu_opt_get_number(opts, "threads", 0);
>  
> +max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
>  /* compute missing values, prefer sockets over cores over threads */
> -if (cpus == 0 || sockets == 0) {
> +if (cpus == 0) {
>  sockets = sockets > 0 ? sockets : 1;
>  cores = cores > 0 ? cores : 1;
>  threads = threads > 0 ? threads : 1;
> -if (cpus == 0) {
> -cpus = cores * threads * sockets;
> -}
> +cpus = cores * threads * sockets;
> +max_cpus = max_cpus > 0 ? max_cpus : cpus;
> +} else if (sockets == 0) {
> +cores = cores > 0 ? cores : 1;
> +threads = threads > 0 ? threads : 1;
> +sockets = max_cpus / (cores * threads);
>  } else if (cores == 0) {
>  threads = threads > 0 ? threads : 1;
>  cores = cpus / (sockets * threads);
> @@ -1231,8 +1235,6 @@ static void smp_parse(QemuOpts *opts)
>  exit(1);
>  }
>  
> -max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
> -
>  if (max_cpus < cpus) {
>  error_report("maxcpus must be equal to or greater than smp");
>  exit(1);
>

I once made a cleanup similar to the above, but with more more line
removal. It doesn't look like I ever posted it, and I don't recall
how much I tested it, but here it is

diff --git a/vl.c b/vl.c
index 5ba06adf787b..ad4e389fde48 100644
--- a/vl.c
+++ b/vl.c
@@ -1208,30 +1208,22 @@ static void smp_parse(QemuOpts *opts)

Re: [Qemu-devel] [PATCH v2 2/2] hw/arm: Add Arm Enterprise machine type

2018-08-30 Thread Leif Lindholm

On Thu, Aug 30, 2018 at 09:39:33AM +0100, Peter Maydell wrote:
> On 30 August 2018 at 09:31, Leif Lindholm  wrote:
> > On Thu, Aug 30, 2018 at 03:07:29PM +0800, Hongbo Zhang wrote:
> >> @Ard, @Leif, is there any possibility to remove all the DT nodes?
> >> On real hardware, how does UEFI find the memory size and CPU number?
> >
> > Usually by asking some form of SCP/PMU.
> > So until we have rock-solid multi-cpu-instance (making terms up here,
> > there may be a proper name already?) support upstream, we need to take
> > shortcuts.
> 
> I would expect that you'd want a "faked-on-the-QEMU end" SCP
> communications endpoint to start with anyway. We have one of
> those for vexpress, for instance.

Ah, I wasn't aware we did!

In that case - sure, that is probably the more sensible solution.
Does it emulate SCMI?

/
Leif

Re: [Qemu-devel] [PATCH 7/9] block: Allow changing 'discard' on reopen

2018-08-30 Thread Alberto Garcia

On Wed 29 Aug 2018 01:39:10 PM CEST, Max Reitz wrote:
> On 2018-08-26 16:09, Alberto Garcia wrote:
>> 'discard' is one of the basic BlockdevOptions available for all
>> drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
>> the user cannot change it and doesn't get an error explaining that it
>> can't be changed.
>> 
>> This should update the discard setting, but does nothing:
>> 
>>(qemu) qemu-io virtio0 "reopen -o discard=on"
>> 
>> Since there's no reason why we shouldn't allow changing it and the
>> implementation is simple let's just do it.
>> 
>> Signed-off-by: Alberto Garcia 
>> ---
>>  block.c | 9 +
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/block.c b/block.c
>> index 804d557608..21f1eb9cd1 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3178,6 +3178,15 @@ int bdrv_reopen_prepare(BDRVReopenState 
>> *reopen_state, BlockReopenQueue *queue,
>>  
>>  update_flags_from_options(&reopen_state->flags, opts);
>>  
>> +value = qemu_opt_get(opts, "discard");
>> +if (value != NULL) {
>> +if (bdrv_parse_discard_flags(value, &reopen_state->flags) != 0) {
>> +error_setg(errp, "Invalid discard option");
>> +ret = -EINVAL;
>> +goto error;
>> +}
>> +}
>> +
>>  /* node-name and driver must be unchanged. Put them back into the 
>> QDict, so
>>   * that they are checked at the end of this function. */
>>  value = qemu_opt_get(opts, "node-name");
>
> Hm.  Why not just use the same "trick" here as in your last patch,
> i.e.  use qemu_opt_get_del() above and then qemu_opts_to_qdict() to
> return all unhandled options to the QDict?

Sure, I could do that and therefore forbid changing all of these
options. But as I said in the commit message, there's no reason why we
should not allow changing them.

Berto

Re: [Qemu-devel] [PATCH 7/9] block: Allow changing 'discard' on reopen

2018-08-30 Thread Alberto Garcia

On Wed 29 Aug 2018 01:39:10 PM CEST, Max Reitz wrote:
> On 2018-08-26 16:09, Alberto Garcia wrote:
>> 'discard' is one of the basic BlockdevOptions available for all
>> drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
>> the user cannot change it and doesn't get an error explaining that it
>> can't be changed.
>> 
>> This should update the discard setting, but does nothing:
>> 
>>(qemu) qemu-io virtio0 "reopen -o discard=on"
>> 
>> Since there's no reason why we shouldn't allow changing it and the
>> implementation is simple let's just do it.
>> 
>> Signed-off-by: Alberto Garcia 
>> ---
>>  block.c | 9 +
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/block.c b/block.c
>> index 804d557608..21f1eb9cd1 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3178,6 +3178,15 @@ int bdrv_reopen_prepare(BDRVReopenState 
>> *reopen_state, BlockReopenQueue *queue,
>>  
>>  update_flags_from_options(&reopen_state->flags, opts);
>>  
>> +value = qemu_opt_get(opts, "discard");
>> +if (value != NULL) {
>> +if (bdrv_parse_discard_flags(value, &reopen_state->flags) != 0) {
>> +error_setg(errp, "Invalid discard option");
>> +ret = -EINVAL;
>> +goto error;
>> +}
>> +}
>> +
>>  /* node-name and driver must be unchanged. Put them back into the 
>> QDict, so
>>   * that they are checked at the end of this function. */
>>  value = qemu_opt_get(opts, "node-name");
>
> Hm.  Why not just use the same "trick" here as in your last patch,
> i.e.  use qemu_opt_get_del() above and then qemu_opts_to_qdict() to
> return all unhandled options to the QDict?

Hm, I think I misunderstood you earlier. You propose that I use
qemu_opts_to_qdict() to catch all other bdrv_runtime_opts options not
processed in this function.

Yes, that sounds like a good idea.

Berto

[Qemu-devel] [Bug 1586613] Re: usb-hub can not be detached when detach usbdevice VM

*** This bug is a duplicate of bug 1586611 ***
https://bugs.launchpad.net/bugs/1586611

** This bug has been marked a duplicate of bug 1586611
   usb-hub can not be detached when detach usb  device from VM

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

Title:
  usb-hub can not be detached when detach usbdevice  VM

Status in QEMU:
  New

Bug description:
  I give a host usb device to guest in the way of passthrough,use "virsh 
attach-device" cmd. In guest os,use "lsusb" cmd I can see two devices have been 
added,one is usb device and the other is usb-hub（0409:55aa NEC Corp. Hub）.
  when I use "virsh detach-device" detach the usb device,in guest os the 
usb-hub was still exists.
  It can create a bad impression when operating the VM,for example,suspend and 
resume the VM,qemu would report that:

  2016-05-24T12:03:54.434369Z qemu-kvm: Unknown savevm section or
  instance ':00:01.2/2/usb-hub' 0

  2016-05-24T12:03:54.434742Z qemu-kvm: load of migration failed:
  Invalid argument

  From qemu's code,it can be sure that the usb-hub is generated by qemu,and the 
process of detaching usb-hub has already been executed,but failed.With adding 
print information,error as follows:
  libusbx: error [do_close] Device handle closed while transfer was still being 
processed, but the device is still connected as far as we know
  libusbx: warning [do_close] A cancellation for an in-flight transfer hasn't 
completed but closing the device handle

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

[Qemu-devel] [Bug 1586611] Re: usb-hub can not be detached when detach usb device from VM

Use a newer libvirt version which manages usb addressing and assigns usb
devices to usb ports.  This is required to make sure the physical device
tree is the same after vmsave/vmload or live migration.

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

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

Title:
  usb-hub can not be detached when detach usb  device from VM

Status in QEMU:
  Invalid

Bug description:
  I give a host usb device to guest in the way of using "virsh attach-device" 
cmd. In guest os,use "lsusb" cmd I can see two devices have been added,one is 
usb device and the other is usb-hub（0409:55aa NEC Corp. Hub）.
  when I use "virsh detach-device" detach the usb device,in guest os the 
usb-hub was still exists.
  It can create a bad impression when operating the VM,for example,suspend and 
resume the VM,qemu would report that:

  2016-05-24T12:03:54.434369Z qemu-kvm: Unknown savevm section or
  instance ':00:01.2/2/usb-hub' 0

  2016-05-24T12:03:54.434742Z qemu-kvm: load of migration failed:
  Invalid argument

  From qemu's code,it can be sure that the usb-hub is generated by
  qemu,but could not be seen in xml file of libvirt. So when executing
  the migration of VM or resuming VM after suspended,it report error
  like "Unknown savevm section or instance ':00:01.2/2/usb-hub' 0"

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

Re: [Qemu-devel] [PATCH] input-linux: toggle for lock keys

On Mon, Aug 27, 2018 at 01:49:22PM +, Ryan El Kochta wrote:

> I personally use the Scroll Lock key. On Linux, Scroll Lock (along
> with all the other lock keys) is easy to disable with an xmodmap
> command. On Windows (or, I'd assume, other guests), it requires third
> party software, which is why it's likely easier to just filter them
> out from QEMU.

Ok, disable with xmodmap is a reasonable approach for that key.
I think it would be a good idea to document that.

I'd suggest to add a single option, to configure alternative toggle
key(s), with just a few hard-coded variants which actually make sense.
Something like grab-toggle={two-shift,scroll-lock}.  Not sure whenever
we need the other lock keys.  scroll lock is probably the
least-frequently used key and therefore the best choice for the
purpose.

In case scroll-lock is selected we can automatically ignore it, without
ignore_scroll_lock=off option.

cheers,
  Gerd

[Qemu-devel] [Bug 1788665] Re: Low 2D graphics performance with Windows 10 (1803) VGA passthrough VM using "Spectre" protection

2018-08-30 Thread Heiko Sieger

Whew, after some hurdles I managed to install a Linux Mint 19 guest
(Ubuntu 18.04). After all updates, here the output:

$ dmesg | grep microcode
[0.036780] core: PEBS disabled due to CPU errata, please upgrade microcode

So the microcode in the guest is not loaded! But see below:

$ cat /proc/cpuinfo | grep flags
flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov 
pat pse36 clflush mmx fxsr sse sse2 ss ht syscall nx pdpe1gb rdtscp lm 
constant_tsc arch_perfmon rep_good nopl xtopology cpuid pni pclmulqdq vmx ssse3 
cx16 pcid sse4_1 sse4_2 x2apic popcnt tsc_deadline_timer aes xsave avx 
hypervisor lahf_lm cpuid_fault pti ssbd ibrs ibpb tpr_shadow vnmi flexpriority 
ept vpid tsc_adjust xsaveopt arat

Here is the qemu command I use for this Linux guest (it is almost
identical to the Windows 10 VM command):

qemu-system-x86_64 \
  -runas user \
  -monitor stdio \
  -serial none \
  -parallel none \
  -nodefaults \
  -nodefconfig \
  -name $vmname,process=$vmname \
  -machine q35,accel=kvm,kernel_irqchip=on \
-cpu 
host,kvm=off,hv_vendor_id=1234567890ab,hv_vapic,hv_time,hv_relaxed,hv_spinlocks=0x1fff
 \
  -smp 6,sockets=1,cores=3,threads=2 \
  -m 16G \
  -mem-path /dev/hugepages \
  -mem-prealloc \
  -balloon none \
  -rtc base=localtime,clock=host \
  -vga none \
  -nographic \
  -soundhw hda \
  -device vfio-pci,host=02:00.0,multifunction=on \
  -device vfio-pci,host=02:00.1 \
  -device vfio-pci,host=00:1a.0 \
  -device vfio-pci,host=08:00.0 \
  -drive if=pflash,format=raw,readonly,file=/usr/share/OVMF/OVMF_CODE.fd \
  -drive if=pflash,format=raw,file=/tmp/my_vars.fd \
  -boot order=c \
  -drive id=disk0,if=virtio,cache=none,format=raw,file=/home/user/win.img \
  -netdev type=tap,id=net0,ifname=vmtap0,vhost=on \
  -device virtio-net-pci,netdev=net0,mac=00:16:3e:00:01:01


Kernel: 4.15.0-33-generic x86_64

$ grep microcode /proc/cpuinfo
microcode   : 0x1
microcode   : 0x1
microcode   : 0x1
microcode   : 0x1
microcode   : 0x1
microcode   : 0x1

In essence:
The microcode is NOT loaded in the Linux VM. However, the following feature 
flags are listed: "pti ssbd ibrs ibpb". The "stibp" flag is missing.

Hope this helps.

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

Title:
  Low 2D graphics performance with Windows 10 (1803) VGA passthrough VM
  using "Spectre" protection

Status in QEMU:
  New

Bug description:
  Windows 10 (1803) VM using VGA passthrough via qemu script.

  After upgrading Windows 10 Pro VM to version 1803, or possibly after
  applying the March/April security updates from Microsoft, the VM would
  show low 2D graphics performance (sluggishness in 2D applications and
  low Passmark results).

  Turning off Spectre vulnerability protection in Windows remedies the
  issue.

  Expected behavior:
  qemu/kvm hypervisor to expose firmware capabilities of host to guest OS - see 
https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/CVE-2017-5715-and-hyper-v-vms

  Background:

  Starting in March or April Microsoft began to push driver updates in
  their updates / security updates. See https://support.microsoft.com
  /en-us/help/4073757/protect-your-windows-devices-against-spectre-
  meltdown

  One update concerns the Intel microcode - see
  https://support.microsoft.com/en-us/help/4100347. It is activated by
  default within Windows.

  Once the updates are applied within the Windows guest, 2D graphics
  performance drops significantly. Other performance benchmarks are not
  affected.

  A bare metal Windows installation does not display a performance loss
  after the update. See https://heiko-sieger.info/low-2d-graphics-
  benchmark-with-windows-10-1803-kvm-vm/

  Similar reports can be found here:
  
https://www.reddit.com/r/VFIO/comments/97unx4/passmark_lousy_2d_graphics_performance_on_windows/

  Hardware:

  6 core Intel Core i7-3930K (-MT-MCP-)

  Host OS:
  Linux Mint 19/Ubuntu 18.04
  Kernel: 4.15.0-32-generic x86_64
  Qemu: QEMU emulator version 2.11.1
  Intel microcode (host): 0x714
  dmesg | grep microcode
  [0.00] microcode: microcode updated early to revision 0x714, date = 
2018-05-08
  [2.810683] microcode: sig=0x206d7, pf=0x4, revision=0x714
  [2.813340] microcode: Microcode Update Driver: v2.2.

  Note: I manually updated the Intel microcode on the host from 0x713 to
  0x714. However, both microcode versions produce the same result in the
  Windows guest.

  Guest OS:
  Windows 10 Pro 64 bit, release 1803

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

[Qemu-devel] [PATCH] i386: clarify that the Q35 machine type implements a P35 chipset

2018-08-30 Thread Daniel P . Berrangé

The 'q35' machine type implements an Intel Series 3 chipset,
of which there are several variants:

  https://www.intel.com/Assets/PDF/datasheet/316966.pdf

The key difference between the 82P35 MCH ('p35', PCI device ID 0x29c0)
and 82Q35 GMCH ('q35', PCI device ID 0x29b0) variants is that the latter
has an integrated graphics adapter. QEMU does not implement integrated
graphics, so uses the PCI ID for the 82P35 chipset, despite calling the
machine type 'q35'. Thus we rename the PCI device ID constant to reflect
reality, to avoid confusing future developers. The new name more closely
matches what pci.ids reports it to be:

$ grep  P35 /usr/share/hwdata/pci.ids  | grep 29
29c0  82G33/G31/P35/P31 Express DRAM Controller
29c1  82G33/G31/P35/P31 Express PCI Express Root Port
29c4  82G33/G31/P35/P31 Express MEI Controller
29c5  82G33/G31/P35/P31 Express MEI Controller
29c6  82G33/G31/P35/P31 Express PT IDER Controller
29c7  82G33/G31/P35/P31 Express Serial KT Controller

$ grep  Q35 /usr/share/hwdata/pci.ids  | grep 29
29b0  82Q35 Express DRAM Controller
29b1  82Q35 Express PCI Express Root Port
29b2  82Q35 Express Integrated Graphics Controller
29b3  82Q35 Express Integrated Graphics Controller
29b4  82Q35 Express MEI Controller
29b5  82Q35 Express MEI Controller
29b6  82Q35 Express PT IDER Controller
29b7  82Q35 Express Serial KT Controller

Arguably the QEMU machine type should be named 'p35'. At this point in
time, however, it is not worth the churn for management applications &
documentation to worry about renaming it.

Signed-off-by: Daniel P. Berrangé 
---
 hw/pci-host/q35.c| 10 +-
 include/hw/pci/pci_ids.h |  2 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 02f9576588..0a056d6aea 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -618,7 +618,15 @@ static void mch_class_init(ObjectClass *klass, void *data)
 dc->desc = "Host bridge";
 dc->vmsd = &vmstate_mch;
 k->vendor_id = PCI_VENDOR_ID_INTEL;
-k->device_id = PCI_DEVICE_ID_INTEL_Q35_MCH;
+/*
+ * The 'q35' machine type implements an Intel Series 3 chipset,
+ * of which there are several variants. The key difference between
+ * the 82P35 MCH ('p35') and 82Q35 GMCH ('q35') variants is that
+ * the latter has an integrated graphics adapter. QEMU does not
+ * implement integrated graphics, so uses the PCI ID for the 82P35
+ * chipset.
+ */
+k->device_id = PCI_DEVICE_ID_INTEL_P35_MCH;
 k->revision = MCH_HOST_BRIDGE_REVISION_DEFAULT;
 k->class_id = PCI_CLASS_BRIDGE_HOST;
 /*
diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
index 63acc722a9..eeb33018ad 100644
--- a/include/hw/pci/pci_ids.h
+++ b/include/hw/pci/pci_ids.h
@@ -255,7 +255,7 @@
 #define PCI_DEVICE_ID_INTEL_82801I_EHCI2 0x293c
 #define PCI_DEVICE_ID_INTEL_82599_SFP_VF 0x10ed
 
-#define PCI_DEVICE_ID_INTEL_Q35_MCH  0x29c0
+#define PCI_DEVICE_ID_INTEL_P35_MCH  0x29c0
 
 #define PCI_VENDOR_ID_XEN0x5853
 #define PCI_DEVICE_ID_XEN_PLATFORM   0x0001
-- 
2.17.1

Re: [Qemu-devel] [PATCH] secondary-vga: unregister vram on unplug.

  Hi,

> > Thanks for the feedback, after closer inspection, the secondary-vga
> > refcount does, indeed, never reach 0.
> > 
> > I noticed the bug was not present in v2.12.0 and had been visible since
> > 93abfc88bd649de1933588bfc7175605331b3ea9
> > (https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg07547.html).
> > 
> > This patch causes the secondary-vga object to be referenced by its
> > subregions (mrs) which are themselves referenced by its mmio region
> > which is referenced by the device causing a reference loop.
> > We should probably break this loop upon exit, however, i am not sure
> > whether we should deletes the subregions or delete the mmio properly.
> 
> I'll take a look...

Ping, any results?

I'm wondering whenever we should just revert
93abfc88bd649de1933588bfc7175605331b3ea9.

Retested hotplug with 93abfc88bd649de1933588bfc7175605331b3ea9 reverted.
Works just fine with guest kernel loaded.  Doesn't work without guest
booted (grub prompt for example), but pci hotplug requires guest
cooperation so this isn't a bug in secondary-vga ...

cheers,
  Gerd

[Qemu-devel] [Bug 1788665] Re: Low 2D graphics performance with Windows 10 (1803) VGA passthrough VM using "Spectre" protection

2018-08-30 Thread Heiko Sieger

Downloaded and ran the spectre-meltdown-checker.sh

$ spectre-meltdown-checker.sh 
Spectre and Meltdown mitigation detection tool v0.39+

Checking for vulnerabilities on current system
Kernel is Linux 4.15.0-33-generic #36-Ubuntu SMP Wed Aug 15 16:00:05 UTC 2018 
x86_64
CPU is Intel(R) Core(TM) i7-3930K CPU @ 3.20GHz

Hardware check
* Hardware support (CPU microcode) for mitigation techniques
  * Indirect Branch Restricted Speculation (IBRS)
* SPEC_CTRL MSR is available:  YES 
* CPU indicates IBRS capability:  YES  (SPEC_CTRL feature bit)
  * Indirect Branch Prediction Barrier (IBPB)
* PRED_CMD MSR is available:  YES 
* CPU indicates IBPB capability:  YES  (SPEC_CTRL feature bit)
  * Single Thread Indirect Branch Predictors (STIBP)
* SPEC_CTRL MSR is available:  YES 
* CPU indicates STIBP capability:  NO 
  * Speculative Store Bypass Disable (SSBD)
* CPU indicates SSBD capability:  YES  (Intel SSBD)
  * L1 data cache invalidation
* FLUSH_CMD MSR is available:  YES 
  * Enhanced IBRS (IBRS_ALL)
* CPU indicates ARCH_CAPABILITIES MSR availability:  NO 
* ARCH_CAPABILITIES MSR advertises IBRS_ALL capability:  NO 
  * CPU explicitly indicates not being vulnerable to Meltdown (RDCL_NO):  NO 
  * CPU explicitly indicates not being vulnerable to Variant 4 (SSB_NO):  NO 
  * Hypervisor indicates host CPU might be vulnerable to RSB underflow (RSBA):  
NO 
  * CPU microcode is known to cause stability problems:  NO  (model 0x2d family 
0x6 stepping 0x7 ucode 0x1 cpuid 0x206d7)
  * CPU microcode is the latest known available version:  NO  (latest known 
version is 0x714 according to Intel Microcode Guidance, August 8 2018)
* CPU vulnerability to the speculative execution attack variants
  * Vulnerable to Variant 1:  YES 
  * Vulnerable to Variant 2:  YES 
  * Vulnerable to Variant 3:  YES 
  * Vulnerable to Variant 3a:  YES 
  * Vulnerable to Variant 4:  YES 
  * Vulnerable to Variant l1tf:  YES 

CVE-2017-5753 [bounds check bypass] aka 'Spectre Variant 1'
* Mitigated according to the /sys interface:  YES  (Mitigation: __user pointer 
sanitization)
* Kernel has array_index_mask_nospec:  YES  (1 occurrence(s) found of x86 64 
bits array_index_mask_nospec())
* Kernel has the Red Hat/Ubuntu patch:  NO 
* Kernel has mask_nospec64 (arm64):  NO 
> STATUS:  NOT VULNERABLE  (Mitigation: __user pointer sanitization)

CVE-2017-5715 [branch target injection] aka 'Spectre Variant 2'
* Mitigated according to the /sys interface:  YES  (Mitigation: Full generic 
retpoline, IBPB, IBRS_FW)
* Mitigation 1
  * Kernel is compiled with IBRS support:  YES 
* IBRS enabled and active:  YES  (for kernel and firmware code)
  * Kernel is compiled with IBPB support:  YES 
* IBPB enabled and active:  YES 
* Mitigation 2
  * Kernel has branch predictor hardening (arm):  NO 
  * Kernel compiled with retpoline option:  YES 
* Kernel compiled with a retpoline-aware compiler:  YES  (kernel reports 
full retpoline compilation)
> STATUS:  NOT VULNERABLE  (Full retpoline + IBPB are mitigating the 
> vulnerability)

CVE-2017-5754 [rogue data cache load] aka 'Meltdown' aka 'Variant 3'
* Mitigated according to the /sys interface:  YES  (Mitigation: PTI)
* Kernel supports Page Table Isolation (PTI):  YES 
  * PTI enabled and active:  YES 
  * Reduced performance impact of PTI:  YES  (CPU supports PCID, performance 
impact of PTI will be reduced)
* Running as a Xen PV DomU:  NO 
> STATUS:  NOT VULNERABLE  (Mitigation: PTI)

CVE-2018-3640 [rogue system register read] aka 'Variant 3a'
* CPU microcode mitigates the vulnerability:  YES 
> STATUS:  NOT VULNERABLE  (your CPU microcode mitigates the vulnerability)

CVE-2018-3639 [speculative store bypass] aka 'Variant 4'
* Mitigated according to the /sys interface:  YES  (Mitigation: Speculative 
Store Bypass disabled via prctl and seccomp)
* Kernel supports speculation store bypass:  YES  (found in /proc/self/status)
> STATUS:  NOT VULNERABLE  (Mitigation: Speculative Store Bypass disabled via 
> prctl and seccomp)

CVE-2018-3615/3620/3646 [L1 terminal fault] aka 'Foreshadow & Foreshadow-NG'
* Mitigated according to the /sys interface:  YES  (Mitigation: PTE Inversion; 
VMX: conditional cache flushes, SMT vulnerable)
> STATUS:  NOT VULNERABLE  (Mitigation: PTE Inversion; VMX: conditional cache 
> flushes, SMT vulnerable)


It shows that the microcode is not updated, and reports vulnerability.

If I understand correctly, the Linux VM should not install the
microcode, but report the microcode features of the host.

 * CPU indicates STIBP capability:  NO

Obviously stibp is not passed to the guest.

Is there any other/better way to pass the host cpu capabilities to the
VM?


** CVE added: https://cve.mitre.org/cgi-bin/cvename.cgi?name=2017-5753

** CVE added: https://cve.mitre.org/cgi-bin/cvename.cgi?name=2017-5754

** CVE added: https://cve.mitre.org/cgi-bin/cvename.cgi?name=2018-3615

** CVE added: https://cve.mitre.org/cgi-bin/cvename.cgi?name=2018-3639

** CV

[Qemu-devel] [Bug 1788665] Re: Low 2D graphics performance with Windows 10 (1803) VGA passthrough VM using "Spectre" protection

2018-08-30 Thread Daniel Berrange

> Whew, after some hurdles I managed to install a Linux Mint 19 guest (Ubuntu 
> 18.04). After all updates, here the output:
>
> $ dmesg | grep microcode
> [ 0.036780] core: PEBS disabled due to CPU errata, please upgrade microcode
>
> So the microcode in the guest is not loaded! But see below:

As mentioned earlier there is no concept of loading microcode in the
guest. Once it is loaded in the host it applies to everything running on
that host.

This message appears to be caused by fact that we do not expose the
current microcode version to the guest, so the guest always sees 0x1 as
the microcode version, causing this PEBS check to fail. I don't think
this is related to the meltdown/spectre fixes - you'd likely get that
error message even on older systems before meltdown/spectre

> $ cat /proc/cpuinfo | grep flags
> flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat 
> pse36 clflush mmx fxsr sse sse2 ss ht syscall nx pdpe1gb rdtscp lm 
> constant_tsc arch_perfmon rep_good nopl xtopology cpuid pni pclmulqdq vmx 
> ssse3 cx16 pcid sse4_1 sse4_2 x2apic popcnt tsc_deadline_timer aes xsave avx 
> hypervisor lahf_lm cpuid_fault pti ssbd ibrs ibpb tpr_shadow vnmi 
> flexpriority ept vpid tsc_adjust xsaveopt arat

This looks correct - the guest has seen the ssbd, ibrs, ibpb features as
required.

> The microcode is NOT loaded in the Linux VM. However, the following
feature flags are listed: "pti > ssbd ibrs ibpb". The "stibp" flag is
missing.

The "stipb" flag is not relevant to guests, it is only needed by the
hypervisor, so it is to be expected that the guest doesn't see it.

Everything seen from this Linux guest says that QEMU / KVM is doing the
right thing and the guest has the features needed to mitigate
Spectre/Meltdown.

The possibilities left are that either your Windows guest is lacking
software updates that could perhaps improve its performance, or that 2D
graphics really is that awful in combination with spectre/meltdown
fixes.

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

Title:
  Low 2D graphics performance with Windows 10 (1803) VGA passthrough VM
  using "Spectre" protection

Status in QEMU:
  New

Bug description:
  Windows 10 (1803) VM using VGA passthrough via qemu script.

  After upgrading Windows 10 Pro VM to version 1803, or possibly after
  applying the March/April security updates from Microsoft, the VM would
  show low 2D graphics performance (sluggishness in 2D applications and
  low Passmark results).

  Turning off Spectre vulnerability protection in Windows remedies the
  issue.

  Expected behavior:
  qemu/kvm hypervisor to expose firmware capabilities of host to guest OS - see 
https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/CVE-2017-5715-and-hyper-v-vms

  Background:

  Starting in March or April Microsoft began to push driver updates in
  their updates / security updates. See https://support.microsoft.com
  /en-us/help/4073757/protect-your-windows-devices-against-spectre-
  meltdown

  One update concerns the Intel microcode - see
  https://support.microsoft.com/en-us/help/4100347. It is activated by
  default within Windows.

  Once the updates are applied within the Windows guest, 2D graphics
  performance drops significantly. Other performance benchmarks are not
  affected.

  A bare metal Windows installation does not display a performance loss
  after the update. See https://heiko-sieger.info/low-2d-graphics-
  benchmark-with-windows-10-1803-kvm-vm/

  Similar reports can be found here:
  
https://www.reddit.com/r/VFIO/comments/97unx4/passmark_lousy_2d_graphics_performance_on_windows/

  Hardware:

  6 core Intel Core i7-3930K (-MT-MCP-)

  Host OS:
  Linux Mint 19/Ubuntu 18.04
  Kernel: 4.15.0-32-generic x86_64
  Qemu: QEMU emulator version 2.11.1
  Intel microcode (host): 0x714
  dmesg | grep microcode
  [0.00] microcode: microcode updated early to revision 0x714, date = 
2018-05-08
  [2.810683] microcode: sig=0x206d7, pf=0x4, revision=0x714
  [2.813340] microcode: Microcode Update Driver: v2.2.

  Note: I manually updated the Intel microcode on the host from 0x713 to
  0x714. However, both microcode versions produce the same result in the
  Windows guest.

  Guest OS:
  Windows 10 Pro 64 bit, release 1803

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

Re: [Qemu-devel] [PATCH v4 05/10] json-lexer: make it safe to call destroy multiple times

Marc-André Lureau  writes:

> We can easily avoid the burden of checking if the lexer was
> initialized prior to calling destroy by the caller, let's do it.
>
> This allows simplification in state tracking in the qmp-async RFC
> series, the patch "qmp: add QmpSession" can call qmp_session_destroy()
> multiple time, which in turns calls json_lexer_destroy().
>
> Signed-off-by: Marc-André Lureau 
> ---
>  qobject/json-lexer.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
> index e1745a3d95..39969047f4 100644
> --- a/qobject/json-lexer.c
> +++ b/qobject/json-lexer.c
> @@ -351,5 +351,8 @@ void json_lexer_flush(JSONLexer *lexer)
>  
>  void json_lexer_destroy(JSONLexer *lexer)
>  {
> -g_string_free(lexer->token, true);
> +if (lexer->token) {
> +g_string_free(lexer->token, true);
> +lexer->token = NULL;
> +}
>  }

Please stick this patch into the series its commit message refers to.

[Qemu-devel] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()

If VM has VCPUs plugged sparselly (for example a VM started with
3 VCPUs (cpu0, cpu1 and cpu2) and then cpu1 was hotunplugged so
only cpu0 and cpu2 are present), QGA will rise a error
  error: internal error: unable to execute QEMU agent command 'guest-get-vcpus':
  open("/sys/devices/system/cpu/cpu1/"): No such file or directory
when
  virsh vcpucount FOO --guest
is executed.
Fix it by ignoring non present CPUs when fetching CPUs status from sysfs.

Signed-off-by: Igor Mammedov 
---
 qga/commands-posix.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 37e8a2d..2929872 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2044,7 +2044,9 @@ static void transfer_vcpu(GuestLogicalProcessor *vcpu, 
bool sys2vcpu,
   vcpu->logical_id);
 dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
 if (dirfd == -1) {
-error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
+if (!(sys2vcpu && errno == ENOENT)) {
+error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
+}
 } else {
 static const char fn[] = "online";
 int fd;
-- 
2.7.4

[Qemu-devel] [PATCH v2 0/9] s390x: instruction flags and AFP registers for TCG

I wanted to add AFP-register control related checks for a long time.
However, doing these checks in each and every relevant handler is ugly.

As I will need similar checks for vector instructions (yes, I'm looking into
that but it might take some time), I decided to introduce per-instruction
flags, that allow to to check such "instruction properties" globally.
Tagging e.g. privileged instructions that way turns out quite nice.

Of course, while at it some fixes and cleanups.

v1 -> v2:
- added "s390x: move tcg_s390_program_interrupt() into TCG code and ..."
- Some checkpatch changes lead to some false positives in the last two
  patches ... to work around them, I dropped some unnecessary parantheses.

David Hildenbrand (9):
  s390x: move tcg_s390_program_interrupt() into TCG code and mark it
noreturn
  s390x/tcg: factor out and fix DATA exception injection
  s390x/tcg: store in the TB flags if AFP is enabled
  s390x/tcg: support flags for instructions
  s390x/tcg: add instruction flags for floating point instructions
  s390x/tcg: check for AFP-register, BFP and DFP data exceptions
  s390x/tcg: handle privileged instructions via flags
  s390x/tcg: fix FP register pair checks
  s390x/tcg: refactor specification checking

 target/s390x/cpu.h |   9 +
 target/s390x/excp_helper.c |  36 
 target/s390x/fpu_helper.c  |  13 +-
 target/s390x/helper.h  |   1 +
 target/s390x/insn-data.def | 395 +++--
 target/s390x/interrupt.c   |  15 +-
 target/s390x/kvm.c |   3 +-
 target/s390x/tcg-stub.c|  10 +
 target/s390x/tcg_s390x.h   |   4 +
 target/s390x/translate.c   | 197 --
 10 files changed, 343 insertions(+), 340 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 3/9] s390x/tcg: store in the TB flags if AFP is enabled

We exit the TB when changing the control registers, so just like PSW
bits, this should always be consistent for a TB.

Using the PSW bit semantic makes things a lot easier compared to
manually defining the spare, shifted bits.

Reviewed-by: Richard Henderson 
Signed-off-by: David Hildenbrand 
---
 target/s390x/cpu.h | 8 
 1 file changed, 8 insertions(+)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 5e50c3a303..8c2320e882 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -255,6 +255,7 @@ extern const struct VMStateDescription vmstate_s390_cpu;
 
 /* PSW defines */
 #undef PSW_MASK_PER
+#undef PSW_MASK_UNUSED_2
 #undef PSW_MASK_DAT
 #undef PSW_MASK_IO
 #undef PSW_MASK_EXT
@@ -273,6 +274,7 @@ extern const struct VMStateDescription vmstate_s390_cpu;
 #undef PSW_MASK_ESA_ADDR
 
 #define PSW_MASK_PER0x4000ULL
+#define PSW_MASK_UNUSED_2   0x2000ULL
 #define PSW_MASK_DAT0x0400ULL
 #define PSW_MASK_IO 0x0200ULL
 #define PSW_MASK_EXT0x0100ULL
@@ -318,6 +320,9 @@ extern const struct VMStateDescription vmstate_s390_cpu;
 #define FLAG_MASK_PSW   (FLAG_MASK_PER | FLAG_MASK_DAT | 
FLAG_MASK_PSTATE \
 | FLAG_MASK_ASC | FLAG_MASK_64 | FLAG_MASK_32)
 
+/* we'll use some unused PSW positions to store CR flags in tb flags */
+#define FLAG_MASK_AFP   (PSW_MASK_UNUSED_2 >> FLAG_MASK_PSW_SHIFT)
+
 /* Control register 0 bits */
 #define CR0_LOWPROT 0x1000ULL
 #define CR0_SECONDARY   0x0400ULL
@@ -364,6 +369,9 @@ static inline void cpu_get_tb_cpu_state(CPUS390XState* env, 
target_ulong *pc,
 *pc = env->psw.addr;
 *cs_base = env->ex_value;
 *flags = (env->psw.mask >> FLAG_MASK_PSW_SHIFT) & FLAG_MASK_PSW;
+if (env->cregs[0] & CR0_AFP) {
+*flags |= FLAG_MASK_AFP;
+}
 }
 
 /* PER bits from control register 9 */
-- 
2.17.1

[Qemu-devel] [PATCH v2 6/9] s390x/tcg: check for AFP-register, BFP and DFP data exceptions

With the annotated functions, we can now easily check this at a central
place.

DXC 1 is to be injected if an AFP register is used (for a HFP instruction)
when AFP is disabled.
DXC 2 is to be injected if a BFP instruction is used when AFP is
disabled.
DXC § is to be injected if a DFP instruction is used when AFP is
disabled.

Signed-off-by: David Hildenbrand 
---
 target/s390x/translate.c | 33 +
 1 file changed, 33 insertions(+)

diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 8322c81e90..a0c834ebb9 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -6055,6 +6055,11 @@ static const DisasInsn *extract_insn(CPUS390XState *env, 
DisasContext *s,
 return info;
 }
 
+static bool is_afp_reg(int reg)
+{
+return reg % 2 || reg > 6;
+}
+
 static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
 {
 const DisasInsn *insn;
@@ -6081,6 +6086,34 @@ static DisasJumpType translate_one(CPUS390XState *env, 
DisasContext *s)
 }
 #endif
 
+/* process flags */
+if (insn->flags) {
+/* if AFP is not enabled, instructions and registers are forbidden */
+if (!(s->base.tb->flags & FLAG_MASK_AFP)) {
+uint8_t dxc = 0;
+
+if ((insn->flags & IF_HFP1) && is_afp_reg(get_field(&f, r1))) {
+dxc = 1;
+}
+if ((insn->flags & IF_HFP2) && is_afp_reg(get_field(&f, r2))) {
+dxc = 1;
+}
+if ((insn->flags & IF_HFP3) && is_afp_reg(get_field(&f, r3))) {
+dxc = 1;
+}
+if (insn->flags & IF_BFP) {
+dxc = 2;
+}
+if (insn->flags & IF_DFP) {
+dxc = 3;
+}
+if (dxc) {
+gen_data_exception(dxc);
+return DISAS_NORETURN;
+}
+}
+}
+
 /* Check for insn specification exceptions.  */
 if (insn->spec) {
 int spec = insn->spec, excp = 0, r;
-- 
2.17.1

[Qemu-devel] [PATCH v2 8/9] s390x/tcg: fix FP register pair checks

Valid register pairs are 0/2, 1/3, 4/6, 5/7, 8/10, 9/11, 12/14, 13/15.

R1/R2 always selects the lower number, so the current checks are not
correct as e.g. 2/4 could be selected as a pair.

Signed-off-by: David Hildenbrand 
---
 target/s390x/translate.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 1ca6ef45a1..5cc65b0840 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -1110,7 +1110,7 @@ typedef struct {
 #define IF_HFP3 0x0004  /* r3 points at fp reg for HFP instructions */
 #define IF_BFP  0x0008  /* binary floating point instruction */
 #define IF_DFP  0x0010  /* decimal floating point instruction */
-#define IF_PRIV 0x0020  /* priviledged instruction */
+#define IF_PRIV 0x0020  /* privileged instruction */
 
 struct DisasInsn {
 unsigned opc:16;
@@ -5985,6 +5985,12 @@ static bool is_afp_reg(int reg)
 return reg % 2 || reg > 6;
 }
 
+static bool is_fp_pair(int reg)
+{
+/* 0,1,4,5,8,9,12,13: to exclude the others, check for single bit */
+return !(reg & 0x2);
+}
+
 static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
 {
 const DisasInsn *insn;
@@ -6013,7 +6019,7 @@ static DisasJumpType translate_one(CPUS390XState *env, 
DisasContext *s)
 
 /* process flags */
 if (insn->flags) {
-/* priviledged instruction */
+/* privileged instruction */
 if ((s->base.tb->flags & FLAG_MASK_PSTATE) && (insn->flags & IF_PRIV)) 
{
 gen_program_exception(s, PGM_PRIVILEGED);
 return DISAS_NORETURN;
@@ -6067,17 +6073,11 @@ static DisasJumpType translate_one(CPUS390XState *env, 
DisasContext *s)
 excp = PGM_SPECIFICATION;
 }
 }
-if (spec & SPEC_r1_f128) {
-r = get_field(&f, r1);
-if (r > 13) {
-excp = PGM_SPECIFICATION;
-}
+if (spec & SPEC_r1_f128 && !is_fp_pair(get_field(&f, r1))) {
+excp = PGM_SPECIFICATION;
 }
-if (spec & SPEC_r2_f128) {
-r = get_field(&f, r2);
-if (r > 13) {
-excp = PGM_SPECIFICATION;
-}
+if (spec & SPEC_r2_f128 && !is_fp_pair(get_field(&f, r2))) {
+excp = PGM_SPECIFICATION;
 }
 if (excp) {
 gen_program_exception(s, excp);
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/9] s390x/tcg: factor out and fix DATA exception injection

The DXC is to be stored in the low core, and only in the FPC in case AFP
is enabled in CR0. Stub is not required in current code, but this way
we never run into problems.

Signed-off-by: David Hildenbrand 
---
 target/s390x/cpu.h |  1 +
 target/s390x/excp_helper.c | 23 +++
 target/s390x/fpu_helper.c  | 13 +++--
 target/s390x/helper.h  |  1 +
 target/s390x/tcg-stub.c|  5 +
 target/s390x/tcg_s390x.h   |  2 ++
 target/s390x/translate.c   | 19 +--
 7 files changed, 44 insertions(+), 20 deletions(-)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 6f8861e554..5e50c3a303 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -322,6 +322,7 @@ extern const struct VMStateDescription vmstate_s390_cpu;
 #define CR0_LOWPROT 0x1000ULL
 #define CR0_SECONDARY   0x0400ULL
 #define CR0_EDAT0x0080ULL
+#define CR0_AFP 0x0004ULL
 #define CR0_EMERGENCY_SIGNAL_SC 0x4000ULL
 #define CR0_EXTERNAL_CALL_SC0x2000ULL
 #define CR0_CKC_SC  0x0800ULL
diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index 5dab3387c3..2b23105f41 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -21,6 +21,7 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "internal.h"
+#include "exec/helper-proto.h"
 #include "qemu/timer.h"
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
@@ -61,6 +62,28 @@ void QEMU_NORETURN tcg_s390_program_interrupt(CPUS390XState 
*env, uint32_t code,
 cpu_loop_exit(cs);
 }
 
+void QEMU_NORETURN tcg_s390_data_exception(CPUS390XState *env, uint32_t dxc,
+   uintptr_t ra)
+{
+g_assert(!(dxc & ~0xff));
+#if !defined(CONFIG_USER_ONLY)
+/* Store the DXC into the lowcore */
+stl_phys(CPU(s390_env_get_cpu(env))->as,
+ env->psa + offsetof(LowCore, data_exc_code), dxc);
+#endif
+
+/* Store the DXC into the FPC if AFP is enabled */
+if (env->cregs[0] & CR0_AFP) {
+env->fpc = (env->fpc & ~0xff00) | (dxc << 8);
+}
+tcg_s390_program_interrupt(env, PGM_DATA, ILEN_AUTO, ra);
+}
+
+void HELPER(data_exception)(CPUS390XState *env, uint32_t dxc)
+{
+tcg_s390_data_exception(env, dxc, GETPC());
+}
+
 #if defined(CONFIG_USER_ONLY)
 
 void s390_cpu_do_interrupt(CPUState *cs)
diff --git a/target/s390x/fpu_helper.c b/target/s390x/fpu_helper.c
index 5c5b451b3b..1b662d2520 100644
--- a/target/s390x/fpu_helper.c
+++ b/target/s390x/fpu_helper.c
@@ -21,6 +21,7 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "internal.h"
+#include "tcg_s390x.h"
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
 #include "exec/helper-proto.h"
@@ -40,14 +41,6 @@
  ? (mask / (from / to)) & to\
  : (mask & from) * (to / from))
 
-static void ieee_exception(CPUS390XState *env, uint32_t dxc, uintptr_t retaddr)
-{
-/* Install the DXC code.  */
-env->fpc = (env->fpc & ~0xff00) | (dxc << 8);
-/* Trap.  */
-s390_program_interrupt(env, PGM_DATA, ILEN_AUTO, retaddr);
-}
-
 /* Should be called after any operation that may raise IEEE exceptions.  */
 static void handle_exceptions(CPUS390XState *env, uintptr_t retaddr)
 {
@@ -75,7 +68,7 @@ static void handle_exceptions(CPUS390XState *env, uintptr_t 
retaddr)
 /* Send signals for enabled exceptions.  */
 s390_exc &= env->fpc >> 24;
 if (s390_exc) {
-ieee_exception(env, s390_exc, retaddr);
+tcg_s390_data_exception(env, s390_exc, retaddr);
 }
 }
 
@@ -773,6 +766,6 @@ void HELPER(sfas)(CPUS390XState *env, uint64_t val)
is also 1, a simulated-iee-exception trap occurs.  */
 s390_exc = (signalling >> 16) & (source >> 24);
 if (s390_exc) {
-ieee_exception(env, s390_exc | 3, GETPC());
+tcg_s390_data_exception(env, s390_exc | 3, GETPC());
 }
 }
diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 97c60ca7bc..018e9dd414 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -1,4 +1,5 @@
 DEF_HELPER_2(exception, noreturn, env, i32)
+DEF_HELPER_2(data_exception, noreturn, env, i32)
 DEF_HELPER_FLAGS_4(nc, TCG_CALL_NO_WG, i32, env, i32, i64, i64)
 DEF_HELPER_FLAGS_4(oc, TCG_CALL_NO_WG, i32, env, i32, i64, i64)
 DEF_HELPER_FLAGS_4(xc, TCG_CALL_NO_WG, i32, env, i32, i64, i64)
diff --git a/target/s390x/tcg-stub.c b/target/s390x/tcg-stub.c
index dc444fc867..32adb7276a 100644
--- a/target/s390x/tcg-stub.c
+++ b/target/s390x/tcg-stub.c
@@ -23,3 +23,8 @@ void QEMU_NORETURN tcg_s390_program_interrupt(CPUS390XState 
*env, uint32_t code,
 {
 g_assert_not_reached();
 }
+void QEMU_NORETURN tcg_s390_data_exception(CPUS390XState *env, uint32_t dxc,
+   uintptr_t ra)
+{
+g_assert_not_reached();
+}
diff --git a/target/s390x/tcg_s390x.h b/target/s390x/tcg_s390x.h
index d1fe01ef7e..ab2c4ba703 100644
--- a/target/s390x/tc

[Qemu-devel] [PATCH v2 4/9] s390x/tcg: support flags for instructions

Storing flags for instructions allows us to efficiently verify certain
properties at a central point. Examples might later be handling if
AFP is disabled in CR0, we are not in problem state, or if vector
instructions are disabled in CR0.

Signed-off-by: David Hildenbrand 
---
 target/s390x/insn-data.def |  3 +++
 target/s390x/translate.c   | 22 --
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 5c6f33ed9c..ff4a6ceaf5 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -3,6 +3,8 @@
  *
  *  C(OPC,NAME,FMT,   FAC, I1, I2, P, W, OP, CC)
  *  D(OPC,NAME,FMT,   FAC, I1, I2, P, W, OP, CC, DATA)
+ *  E(OPC,NAME,FMT,   FAC, I1, I2, P, W, OP, CC, DATA, FLAGS)
+ *  F(OPC,NAME,FMT,   FAC, I1, I2, P, W, OP, CC, FLAGS)
  *
  *  OPC  = (op << 8) | op2 where op is the major, op2 the minor opcode
  *  NAME = name of the opcode, used internally
@@ -15,6 +17,7 @@
  *  OP   = func op_xx does the bulk of the operation
  *  CC   = func cout_xx defines how cc should get set
  *  DATA = immediate argument to op_xx function
+ *  FLAGS = categorize the type of instruction (e.g. for advanced checks)
  *
  *  The helpers get called in order: I1, I2, P, OP, W, CC
  */
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index fa8468f0e1..e9cbeb2a1b 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -1114,6 +1114,7 @@ typedef struct {
 
 struct DisasInsn {
 unsigned opc:16;
+unsigned flags:16;
 DisasFormat fmt:8;
 unsigned fac:8;
 unsigned spec:8;
@@ -5796,17 +5797,24 @@ static void in2_insn(DisasContext *s, DisasFields *f, 
DisasOps *o)
search tree, rather than us having to post-process the table.  */
 
 #define C(OPC, NM, FT, FC, I1, I2, P, W, OP, CC) \
-D(OPC, NM, FT, FC, I1, I2, P, W, OP, CC, 0)
+E(OPC, NM, FT, FC, I1, I2, P, W, OP, CC, 0, 0)
 
-#define D(OPC, NM, FT, FC, I1, I2, P, W, OP, CC, D) insn_ ## NM,
+#define D(OPC, NM, FT, FC, I1, I2, P, W, OP, CC, D) \
+E(OPC, NM, FT, FC, I1, I2, P, W, OP, CC, D, 0)
+
+#define F(OPC, NM, FT, FC, I1, I2, P, W, OP, CC, FL) \
+E(OPC, NM, FT, FC, I1, I2, P, W, OP, CC, 0, FL)
+
+#define E(OPC, NM, FT, FC, I1, I2, P, W, OP, CC, D, FL) insn_ ## NM,
 
 enum DisasInsnEnum {
 #include "insn-data.def"
 };
 
-#undef D
-#define D(OPC, NM, FT, FC, I1, I2, P, W, OP, CC, D) {   \
+#undef E
+#define E(OPC, NM, FT, FC, I1, I2, P, W, OP, CC, D, FL) {   \
 .opc = OPC, \
+.flags = FL,\
 .fmt = FMT_##FT,\
 .fac = FAC_##FC,\
 .spec = SPEC_in1_##I1 | SPEC_in2_##I2 | SPEC_prep_##P | SPEC_wout_##W,  \
@@ -5877,8 +5885,8 @@ static const DisasInsn insn_info[] = {
 #include "insn-data.def"
 };
 
-#undef D
-#define D(OPC, NM, FT, FC, I1, I2, P, W, OP, CC, D) \
+#undef E
+#define E(OPC, NM, FT, FC, I1, I2, P, W, OP, CC, D, FL) \
 case OPC: return &insn_info[insn_ ## NM];
 
 static const DisasInsn *lookup_opc(uint16_t opc)
@@ -5890,6 +5898,8 @@ static const DisasInsn *lookup_opc(uint16_t opc)
 }
 }
 
+#undef F
+#undef E
 #undef D
 #undef C
 
-- 
2.17.1

[Qemu-devel] [PATCH v2 1/9] s390x: move tcg_s390_program_interrupt() into TCG code and mark it noreturn

Move it into TCG-only code and provide a stub. Turn it into noreturn.

As Richard noted, we currently don't log the psw.addr before restoring
the state, fix that by moving (duplicating) the qemu_log_mask in the
tcg/kvm handlers.

Signed-off-by: David Hildenbrand 
---
 target/s390x/excp_helper.c | 13 +
 target/s390x/interrupt.c   | 15 +--
 target/s390x/kvm.c |  3 ++-
 target/s390x/tcg-stub.c|  5 +
 target/s390x/tcg_s390x.h   |  2 ++
 5 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index f0ce60cff2..5dab3387c3 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -26,6 +26,7 @@
 #include "exec/cpu_ldst.h"
 #include "hw/s390x/ioinst.h"
 #include "exec/address-spaces.h"
+#include "tcg_s390x.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/sysemu.h"
 #include "hw/s390x/s390_flic.h"
@@ -48,6 +49,18 @@
 do { } while (0)
 #endif
 
+void QEMU_NORETURN tcg_s390_program_interrupt(CPUS390XState *env, uint32_t 
code,
+  int ilen, uintptr_t ra)
+{
+CPUState *cs = CPU(s390_env_get_cpu(env));
+
+cpu_restore_state(cs, ra, true);
+qemu_log_mask(CPU_LOG_INT, "program interrupt at %#" PRIx64 "\n",
+  env->psw.addr);
+trigger_pgm_exception(env, code, ilen);
+cpu_loop_exit(cs);
+}
+
 #if defined(CONFIG_USER_ONLY)
 
 void s390_cpu_do_interrupt(CPUState *cs)
diff --git a/target/s390x/interrupt.c b/target/s390x/interrupt.c
index 25cfb3eef8..a17eff5ebc 100644
--- a/target/s390x/interrupt.c
+++ b/target/s390x/interrupt.c
@@ -15,6 +15,7 @@
 #include "exec/exec-all.h"
 #include "sysemu/kvm.h"
 #include "hw/s390x/ioinst.h"
+#include "tcg_s390x.h"
 #if !defined(CONFIG_USER_ONLY)
 #include "hw/s390x/s390_flic.h"
 #endif
@@ -29,25 +30,11 @@ void trigger_pgm_exception(CPUS390XState *env, uint32_t 
code, uint32_t ilen)
 env->int_pgm_ilen = ilen;
 }
 
-static void tcg_s390_program_interrupt(CPUS390XState *env, uint32_t code,
-   int ilen, uintptr_t ra)
-{
-#ifdef CONFIG_TCG
-trigger_pgm_exception(env, code, ilen);
-cpu_loop_exit_restore(CPU(s390_env_get_cpu(env)), ra);
-#else
-g_assert_not_reached();
-#endif
-}
-
 void s390_program_interrupt(CPUS390XState *env, uint32_t code, int ilen,
 uintptr_t ra)
 {
 S390CPU *cpu = s390_env_get_cpu(env);
 
-qemu_log_mask(CPU_LOG_INT, "program interrupt at %#" PRIx64 "\n",
-  env->psw.addr);
-
 if (kvm_enabled()) {
 kvm_s390_program_interrupt(cpu, code);
 } else if (tcg_enabled()) {
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 348e8cc546..8ddbcf5b93 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1109,7 +1109,8 @@ void kvm_s390_program_interrupt(S390CPU *cpu, uint16_t 
code)
 .type = KVM_S390_PROGRAM_INT,
 .u.pgm.code = code,
 };
-
+qemu_log_mask(CPU_LOG_INT, "program interrupt at %#" PRIx64 "\n",
+  cpu->env.psw.addr);
 kvm_s390_vcpu_interrupt(cpu, &irq);
 }
 
diff --git a/target/s390x/tcg-stub.c b/target/s390x/tcg-stub.c
index c93501db0b..dc444fc867 100644
--- a/target/s390x/tcg-stub.c
+++ b/target/s390x/tcg-stub.c
@@ -18,3 +18,8 @@
 void tcg_s390_tod_updated(CPUState *cs, run_on_cpu_data opaque)
 {
 }
+void QEMU_NORETURN tcg_s390_program_interrupt(CPUS390XState *env, uint32_t 
code,
+  int ilen, uintptr_t ra)
+{
+g_assert_not_reached();
+}
diff --git a/target/s390x/tcg_s390x.h b/target/s390x/tcg_s390x.h
index 4e308aa0ce..d1fe01ef7e 100644
--- a/target/s390x/tcg_s390x.h
+++ b/target/s390x/tcg_s390x.h
@@ -14,5 +14,7 @@
 #define TCG_S390X_H
 
 void tcg_s390_tod_updated(CPUState *cs, run_on_cpu_data opaque);
+void QEMU_NORETURN tcg_s390_program_interrupt(CPUS390XState *env, uint32_t 
code,
+  int ilen, uintptr_t ra);
 
 #endif /* TCG_S390X_H */
-- 
2.17.1

[Qemu-devel] [PATCH v2 9/9] s390x/tcg: refactor specification checking

We can fit this nicely into less LOC, without harming readability.

Signed-off-by: David Hildenbrand 
---
 target/s390x/translate.c | 34 ++
 1 file changed, 6 insertions(+), 28 deletions(-)

diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 5cc65b0840..6c6db47a82 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -6053,34 +6053,12 @@ static DisasJumpType translate_one(CPUS390XState *env, 
DisasContext *s)
 
 /* Check for insn specification exceptions.  */
 if (insn->spec) {
-int spec = insn->spec, excp = 0, r;
-
-if (spec & SPEC_r1_even) {
-r = get_field(&f, r1);
-if (r & 1) {
-excp = PGM_SPECIFICATION;
-}
-}
-if (spec & SPEC_r2_even) {
-r = get_field(&f, r2);
-if (r & 1) {
-excp = PGM_SPECIFICATION;
-}
-}
-if (spec & SPEC_r3_even) {
-r = get_field(&f, r3);
-if (r & 1) {
-excp = PGM_SPECIFICATION;
-}
-}
-if (spec & SPEC_r1_f128 && !is_fp_pair(get_field(&f, r1))) {
-excp = PGM_SPECIFICATION;
-}
-if (spec & SPEC_r2_f128 && !is_fp_pair(get_field(&f, r2))) {
-excp = PGM_SPECIFICATION;
-}
-if (excp) {
-gen_program_exception(s, excp);
+if ((insn->spec & SPEC_r1_even && get_field(&f, r1) & 1) ||
+(insn->spec & SPEC_r2_even && get_field(&f, r2) & 1) ||
+(insn->spec & SPEC_r3_even && get_field(&f, r3) & 1) ||
+(insn->spec & SPEC_r1_f128 && !is_fp_pair(get_field(&f, r1))) ||
+(insn->spec & SPEC_r2_f128 && !is_fp_pair(get_field(&f, r2 {
+gen_program_exception(s, PGM_SPECIFICATION);
 return DISAS_NORETURN;
 }
 }
-- 
2.17.1

[Qemu-devel] [PATCH v2 5/9] s390x/tcg: add instruction flags for floating point instructions

These flags allow us to later on detect if a DATA program interrupt
is to be injected, and which DXC (1,2,3) is to be used.

Interestingly, some support FP instructions are considered as HFP
instructions (I assume simply because they were available very early).

Signed-off-by: David Hildenbrand 
---
 target/s390x/insn-data.def | 254 ++---
 target/s390x/translate.c   |   8 ++
 2 files changed, 135 insertions(+), 127 deletions(-)

diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index ff4a6ceaf5..506c3de1d7 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -32,11 +32,11 @@
 C(0xb9e8, AGRK,RRF_a, DO,  r2, r3, r1, 0, add, adds64)
 C(0xe308, AG,  RXY_a, Z,   r1, m2_64, r1, 0, add, adds64)
 C(0xe318, AGF, RXY_a, Z,   r1, m2_32s, r1, 0, add, adds64)
-C(0xb30a, AEBR,RRE,   Z,   e1, e2, new, e1, aeb, f32)
-C(0xb31a, ADBR,RRE,   Z,   f1_o, f2_o, f1, 0, adb, f64)
-C(0xb34a, AXBR,RRE,   Z,   0, x2_o, x1, 0, axb, f128)
-C(0xed0a, AEB, RXE,   Z,   e1, m2_32u, new, e1, aeb, f32)
-C(0xed1a, ADB, RXE,   Z,   f1_o, m2_64, f1, 0, adb, f64)
+F(0xb30a, AEBR,RRE,   Z,   e1, e2, new, e1, aeb, f32, IF_BFP)
+F(0xb31a, ADBR,RRE,   Z,   f1_o, f2_o, f1, 0, adb, f64, IF_BFP)
+F(0xb34a, AXBR,RRE,   Z,   0, x2_o, x1, 0, axb, f128, IF_BFP)
+F(0xed0a, AEB, RXE,   Z,   e1, m2_32u, new, e1, aeb, f32, IF_BFP)
+F(0xed1a, ADB, RXE,   Z,   f1_o, m2_64, f1, 0, adb, f64, IF_BFP)
 /* ADD HIGH */
 C(0xb9c8, AHHHR,   RRF_a, HW,  r2_sr32, r3_sr32, new, r1_32h, add, adds32)
 C(0xb9d8, AHHLR,   RRF_a, HW,  r2_sr32, r3, new, r1_32h, add, adds32)
@@ -151,7 +151,7 @@
 C(0xb241, CKSM,RRE,   Z,   r1_o, ra2, new, r1_32, cksm, 0)
 
 /* COPY SIGN */
-C(0xb372, CPSDR,   RRF_b, FPSSH, f3_o, f2_o, f1, 0, cps, 0)
+F(0xb372, CPSDR,   RRF_b, FPSSH, f3_o, f2_o, f1, 0, cps, 0, IF_HFP1 | 
IF_HFP2 | IF_HFP3)
 
 /* COMPARE */
 C(0x1900, CR,  RR_a,  Z,   r1_o, r2_o, 0, 0, 0, cmps32)
@@ -161,17 +161,17 @@
 C(0xb930, CGFR,RRE,   Z,   r1_o, r2_32s, 0, 0, 0, cmps64)
 C(0xe320, CG,  RXY_a, Z,   r1_o, m2_64, 0, 0, 0, cmps64)
 C(0xe330, CGF, RXY_a, Z,   r1_o, m2_32s, 0, 0, 0, cmps64)
-C(0xb309, CEBR,RRE,   Z,   e1, e2, 0, 0, ceb, 0)
-C(0xb319, CDBR,RRE,   Z,   f1_o, f2_o, 0, 0, cdb, 0)
-C(0xb349, CXBR,RRE,   Z,   x1_o, x2_o, 0, 0, cxb, 0)
-C(0xed09, CEB, RXE,   Z,   e1, m2_32u, 0, 0, ceb, 0)
-C(0xed19, CDB, RXE,   Z,   f1_o, m2_64, 0, 0, cdb, 0)
+F(0xb309, CEBR,RRE,   Z,   e1, e2, 0, 0, ceb, 0, IF_BFP)
+F(0xb319, CDBR,RRE,   Z,   f1_o, f2_o, 0, 0, cdb, 0, IF_BFP)
+F(0xb349, CXBR,RRE,   Z,   x1_o, x2_o, 0, 0, cxb, 0, IF_BFP)
+F(0xed09, CEB, RXE,   Z,   e1, m2_32u, 0, 0, ceb, 0, IF_BFP)
+F(0xed19, CDB, RXE,   Z,   f1_o, m2_64, 0, 0, cdb, 0, IF_BFP)
 /* COMPARE AND SIGNAL */
-C(0xb308, KEBR,RRE,   Z,   e1, e2, 0, 0, keb, 0)
-C(0xb318, KDBR,RRE,   Z,   f1_o, f2_o, 0, 0, kdb, 0)
-C(0xb348, KXBR,RRE,   Z,   x1_o, x2_o, 0, 0, kxb, 0)
-C(0xed08, KEB, RXE,   Z,   e1, m2_32u, 0, 0, keb, 0)
-C(0xed18, KDB, RXE,   Z,   f1_o, m2_64, 0, 0, kdb, 0)
+F(0xb308, KEBR,RRE,   Z,   e1, e2, 0, 0, keb, 0, IF_BFP)
+F(0xb318, KDBR,RRE,   Z,   f1_o, f2_o, 0, 0, kdb, 0, IF_BFP)
+F(0xb348, KXBR,RRE,   Z,   x1_o, x2_o, 0, 0, kxb, 0, IF_BFP)
+F(0xed08, KEB, RXE,   Z,   e1, m2_32u, 0, 0, keb, 0, IF_BFP)
+F(0xed18, KDB, RXE,   Z,   f1_o, m2_64, 0, 0, kdb, 0, IF_BFP)
 /* COMPARE IMMEDIATE */
 C(0xc20d, CFI, RIL_a, EI,  r1, i2, 0, 0, 0, cmps32)
 C(0xc20c, CGFI,RIL_a, EI,  r1, i2, 0, 0, 0, cmps64)
@@ -288,33 +288,33 @@
 C(0x4e00, CVD, RX_a,  Z,   r1_o, a2, 0, 0, cvd, 0)
 C(0xe326, CVDY,RXY_a, LD,  r1_o, a2, 0, 0, cvd, 0)
 /* CONVERT TO FIXED */
-C(0xb398, CFEBR,   RRF_e, Z,   0, e2, new, r1_32, cfeb, 0)
-C(0xb399, CFDBR,   RRF_e, Z,   0, f2_o, new, r1_32, cfdb, 0)
-C(0xb39a, CFXBR,   RRF_e, Z,   0, x2_o, new, r1_32, cfxb, 0)
-C(0xb3a8, CGEBR,   RRF_e, Z,   0, e2, r1, 0, cgeb, 0)
-C(0xb3a9, CGDBR,   RRF_e, Z,   0, f2_o, r1, 0, cgdb, 0)
-C(0xb3aa, CGXBR,   RRF_e, Z,   0, x2_o, r1, 0, cgxb, 0)
+F(0xb398, CFEBR,   RRF_e, Z,   0, e2, new, r1_32, cfeb, 0, IF_BFP)
+F(0xb399, CFDBR,   RRF_e, Z,   0, f2_o, new, r1_32, cfdb, 0, IF_BFP)
+F(0xb39a, CFXBR,   RRF_e, Z,   0, x2_o, new, r1_32, cfxb, 0, IF_BFP)
+F(0xb3a8, CGEBR,   RRF_e, Z,   0, e2, r1, 0, cgeb, 0, IF_BFP)
+F(0xb3a9, CGDBR,   RRF_e, Z,   0, f2_o, r1, 0, cgdb, 0, IF_BFP)
+F(0xb3aa, CGXBR,   RRF_e, Z,   0, x2_o, r1, 0, cgxb, 0, IF_BFP)
 /* CONVERT FROM FIXED */
-C(0xb394, CEFBR,   RRF_e, Z,   0, r2_32s, new, e1, cegb, 0)
-C(0xb395, CDFBR,   RRF_e, Z,   0, r2_32s, f1, 0, cdgb, 0)
-C(0xb396, CXFBR,   RRF_e, Z,   0, r2_32s, x1, 0, cxgb, 0)
-C(0xb3a4, CEGBR,   RRF_e, Z,   0, r2_o, new

Re: [Qemu-devel] [PATCH] vl:c: make sure that sockets are calculated correctly in '-smp X' case

On Thu, 30 Aug 2018 11:08:53 +0200
Andrew Jones  wrote:

> On Thu, Aug 30, 2018 at 09:58:51AM +0200, Igor Mammedov wrote:
> > On Wed, 29 Aug 2018 14:33:01 -0300
> > Eduardo Habkost  wrote:
> >   
> > > On Wed, Aug 29, 2018 at 04:32:01PM +0200, Igor Mammedov wrote:  
> > > > commit
> > > >   (5cdc9b76e3 vl.c: Remove dead assignment)
> > > > removed sockets calculation when 'sockets' weren't provided on CLI
> > > > since there wasn't any users for it back then. Exiting checks
> > > > are neither reachable
> > > >} else if (sockets * cores * threads < cpus) {
> > > > or nor triggable
> > > >if (sockets * cores * threads > max_cpus)
> > > > so we weren't noticing wrong topology since then, since users
> > > > recalculate sockets adhoc on their own.
> > > > 
> > > > However with deprecation check it becomes noticable, for example
> > > >   -smp 2
> > > > will start printing warning:
> > > >   "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * 
> > > > threads (1) != maxcpus (2)"
> > > > calculating sockets if they weren't specified.
> > > > 
> > > > Fix it by returning back sockets calculation if it's omited on CLI.
> > > > 
> > > > Signed-off-by: Igor Mammedov 
> > > > ---
> > > >  vl.c | 5 -
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/vl.c b/vl.c
> > > > index 7fd700e..333d638 100644
> > > > --- a/vl.c
> > > > +++ b/vl.c
> > > > @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
> > > >  
> > > >  /* compute missing values, prefer sockets over cores over 
> > > > threads */
> > > >  if (cpus == 0 || sockets == 0) {
> > > > -sockets = sockets > 0 ? sockets : 1;
> > > >  cores = cores > 0 ? cores : 1;
> > > >  threads = threads > 0 ? threads : 1;
> > > >  if (cpus == 0) {
> > > > +sockets = sockets > 0 ? sockets : 1;
> > > >  cpus = cores * threads * sockets;
> > > > +} else {
> > > > +max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); 
> > > >
> > > 
> > > We already have a "max_cpus = qemu_opt_get_number(...)" line in
> > > this function[1]:
> > > 
> > > /* compute missing values, prefer sockets over cores over threads 
> > > */
> > > if (cpus == 0 || sockets == 0) {
> > > sockets = sockets > 0 ? sockets : 1;
> > > cores = cores > 0 ? cores : 1;
> > > threads = threads > 0 ? threads : 1;
> > > if (cpus == 0) {
> > > cpus = cores * threads * sockets;
> > > }
> > > } else if (cores == 0) {
> > > [...]
> > > } else if (threads == 0) {
> > > [...]
> > > } else if (sockets * cores * threads < cpus) {
> > > [...]
> > > }
> > > 
> > > max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); /* [1] */
> > > 
> > > /* [2] */
> > > 
> > > if (max_cpus < cpus) {
> > > error_report("maxcpus must be equal to or greater than smp");
> > > exit(1);
> > > }
> > > 
> > > Why not move the sockets == 0 check to [2]?  
> > it won't work, since the rest 'else if' depends on sockets being non 0
> > which is ensured by (cpus == 0 || sockets == 0) branch, I can split this in 
> > 2
> > equivalent conditions. It's more code but it might be easier to read
> > 
> > diff --git a/vl.c b/vl.c
> > index 7fd700e..6320bdc 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1208,14 +1208,18 @@ static void smp_parse(QemuOpts *opts)
> >  unsigned cores   = qemu_opt_get_number(opts, "cores", 0);
> >  unsigned threads = qemu_opt_get_number(opts, "threads", 0);
> >  
> > +max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
> >  /* compute missing values, prefer sockets over cores over threads 
> > */
> > -if (cpus == 0 || sockets == 0) {
> > +if (cpus == 0) {
> >  sockets = sockets > 0 ? sockets : 1;
> >  cores = cores > 0 ? cores : 1;
> >  threads = threads > 0 ? threads : 1;
> > -if (cpus == 0) {
> > -cpus = cores * threads * sockets;
> > -}
> > +cpus = cores * threads * sockets;
> > +max_cpus = max_cpus > 0 ? max_cpus : cpus;
> > +} else if (sockets == 0) {
> > +cores = cores > 0 ? cores : 1;
> > +threads = threads > 0 ? threads : 1;
> > +sockets = max_cpus / (cores * threads);
> >  } else if (cores == 0) {
> >  threads = threads > 0 ? threads : 1;
> >  cores = cpus / (sockets * threads);
> > @@ -1231,8 +1235,6 @@ static void smp_parse(QemuOpts *opts)
> >  exit(1);
> >  }
> >  
> > -max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
> > -
> >  if (max_cpus < cpus) {
> >  error_report("maxcpus must be equal to or greater than smp");
> >  exit(1);

[Qemu-devel] [PATCH v2 7/9] s390x/tcg: handle privileged instructions via flags

Let's check this also at a central place.

Signed-off-by: David Hildenbrand 
---
 target/s390x/insn-data.def | 138 ++---
 target/s390x/translate.c   |  83 ++
 2 files changed, 76 insertions(+), 145 deletions(-)

diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 506c3de1d7..fe52d7b81c 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -961,126 +961,126 @@
 
 #ifndef CONFIG_USER_ONLY
 /* COMPARE AND SWAP AND PURGE */
-D(0xb250, CSP, RRE,   Z,   r1_32u, ra2, r1_P, 0, csp, 0, MO_TEUL)
-D(0xb98a, CSPG,RRE, DAT_ENH, r1_o, ra2, r1_P, 0, csp, 0, MO_TEQ)
+E(0xb250, CSP, RRE,   Z,   r1_32u, ra2, r1_P, 0, csp, 0, MO_TEUL, 
IF_PRIV)
+E(0xb98a, CSPG,RRE, DAT_ENH, r1_o, ra2, r1_P, 0, csp, 0, MO_TEQ, 
IF_PRIV)
 /* DIAGNOSE (KVM hypercall) */
-C(0x8300, DIAG,RSI,   Z,   0, 0, 0, 0, diag, 0)
+F(0x8300, DIAG,RSI,   Z,   0, 0, 0, 0, diag, 0, IF_PRIV)
 /* INSERT STORAGE KEY EXTENDED */
-C(0xb229, ISKE,RRE,   Z,   0, r2_o, new, r1_8, iske, 0)
+F(0xb229, ISKE,RRE,   Z,   0, r2_o, new, r1_8, iske, 0, IF_PRIV)
 /* INVALIDATE DAT TABLE ENTRY */
-C(0xb98e, IPDE,RRF_b, Z,   r1_o, r2_o, 0, 0, idte, 0)
+F(0xb98e, IPDE,RRF_b, Z,   r1_o, r2_o, 0, 0, idte, 0, IF_PRIV)
 /* INVALIDATE PAGE TABLE ENTRY */
-C(0xb221, IPTE,RRF_a, Z,   r1_o, r2_o, 0, 0, ipte, 0)
+F(0xb221, IPTE,RRF_a, Z,   r1_o, r2_o, 0, 0, ipte, 0, IF_PRIV)
 /* LOAD CONTROL */
-C(0xb700, LCTL,RS_a,  Z,   0, a2, 0, 0, lctl, 0)
-C(0xeb2f, LCTLG,   RSY_a, Z,   0, a2, 0, 0, lctlg, 0)
+F(0xb700, LCTL,RS_a,  Z,   0, a2, 0, 0, lctl, 0, IF_PRIV)
+F(0xeb2f, LCTLG,   RSY_a, Z,   0, a2, 0, 0, lctlg, 0, IF_PRIV)
 /* LOAD PROGRAM PARAMETER */
-C(0xb280, LPP, S,   LPP,   0, m2_64, 0, 0, lpp, 0)
+F(0xb280, LPP, S,   LPP,   0, m2_64, 0, 0, lpp, 0, IF_PRIV)
 /* LOAD PSW */
-C(0x8200, LPSW,S, Z,   0, a2, 0, 0, lpsw, 0)
+F(0x8200, LPSW,S, Z,   0, a2, 0, 0, lpsw, 0, IF_PRIV)
 /* LOAD PSW EXTENDED */
-C(0xb2b2, LPSWE,   S, Z,   0, a2, 0, 0, lpswe, 0)
+F(0xb2b2, LPSWE,   S, Z,   0, a2, 0, 0, lpswe, 0, IF_PRIV)
 /* LOAD REAL ADDRESS */
-C(0xb100, LRA, RX_a,  Z,   0, a2, r1, 0, lra, 0)
-C(0xe313, LRAY,RXY_a, LD,  0, a2, r1, 0, lra, 0)
-C(0xe303, LRAG,RXY_a, Z,   0, a2, r1, 0, lra, 0)
+F(0xb100, LRA, RX_a,  Z,   0, a2, r1, 0, lra, 0, IF_PRIV)
+F(0xe313, LRAY,RXY_a, LD,  0, a2, r1, 0, lra, 0, IF_PRIV)
+F(0xe303, LRAG,RXY_a, Z,   0, a2, r1, 0, lra, 0, IF_PRIV)
 /* LOAD USING REAL ADDRESS */
-C(0xb24b, LURA,RRE,   Z,   0, r2, new, r1_32, lura, 0)
-C(0xb905, LURAG,   RRE,   Z,   0, r2, r1, 0, lurag, 0)
+F(0xb24b, LURA,RRE,   Z,   0, r2, new, r1_32, lura, 0, IF_PRIV)
+F(0xb905, LURAG,   RRE,   Z,   0, r2, r1, 0, lurag, 0, IF_PRIV)
 /* MOVE TO PRIMARY */
-C(0xda00, MVCP,SS_d,  Z,   la1, a2, 0, 0, mvcp, 0)
+F(0xda00, MVCP,SS_d,  Z,   la1, a2, 0, 0, mvcp, 0, IF_PRIV)
 /* MOVE TO SECONDARY */
-C(0xdb00, MVCS,SS_d,  Z,   la1, a2, 0, 0, mvcs, 0)
+F(0xdb00, MVCS,SS_d,  Z,   la1, a2, 0, 0, mvcs, 0, IF_PRIV)
 /* PURGE TLB */
-C(0xb20d, PTLB,S, Z,   0, 0, 0, 0, ptlb, 0)
+F(0xb20d, PTLB,S, Z,   0, 0, 0, 0, ptlb, 0, IF_PRIV)
 /* RESET REFERENCE BIT EXTENDED */
-C(0xb22a, RRBE,RRE,   Z,   0, r2_o, 0, 0, rrbe, 0)
+F(0xb22a, RRBE,RRE,   Z,   0, r2_o, 0, 0, rrbe, 0, IF_PRIV)
 /* SERVICE CALL LOGICAL PROCESSOR (PV hypercall) */
-C(0xb220, SERVC,   RRE,   Z,   r1_o, r2_o, 0, 0, servc, 0)
+F(0xb220, SERVC,   RRE,   Z,   r1_o, r2_o, 0, 0, servc, 0, IF_PRIV)
 /* SET ADDRESS SPACE CONTROL FAST */
-C(0xb279, SACF,S, Z,   0, a2, 0, 0, sacf, 0)
+F(0xb279, SACF,S, Z,   0, a2, 0, 0, sacf, 0, IF_PRIV)
 /* SET CLOCK */
-C(0xb204, SCK, S, Z,   la2, 0, 0, 0, sck, 0)
+F(0xb204, SCK, S, Z,   la2, 0, 0, 0, sck, 0, IF_PRIV)
 /* SET CLOCK COMPARATOR */
-C(0xb206, SCKC,S, Z,   0, m2_64a, 0, 0, sckc, 0)
+F(0xb206, SCKC,S, Z,   0, m2_64a, 0, 0, sckc, 0, IF_PRIV)
 /* SET CLOCK PROGRAMMABLE FIELD */
-C(0x0107, SCKPF,   E, Z,   0, 0, 0, 0, sckpf, 0)
+F(0x0107, SCKPF,   E, Z,   0, 0, 0, 0, sckpf, 0, IF_PRIV)
 /* SET CPU TIMER */
-C(0xb208, SPT, S, Z,   0, m2_64a, 0, 0, spt, 0)
+F(0xb208, SPT, S, Z,   0, m2_64a, 0, 0, spt, 0, IF_PRIV)
 /* SET PREFIX */
-C(0xb210, SPX, S, Z,   0, m2_32ua, 0, 0, spx, 0)
+F(0xb210, SPX, S, Z,   0, m2_32ua, 0, 0, spx, 0, IF_PRIV)
 /* SET PSW KEY FROM ADDRESS */
-C(0xb20a, SPKA,S, Z,   0, a2, 0, 0, spka, 0)
+F(0xb20a, SPKA,S, Z,   0, a2, 0, 0, spka, 0, IF_PRIV)
 /* SET STORAGE KEY EXTENDED */
-C(0xb22b, SSKE,RRF_c, Z,   r1_o, r2_o, 0, 0, sske, 0)
+F(0xb22b, SSKE,RRF_c, Z,   r1_o, r2_o, 0, 0, sske, 0, IF_PRIV)
 /* SET SYSTEM M

Re: [Qemu-devel] [PATCH v3 0/8] Add limited MXU instruction support

2018-08-30 Thread Aleksandar Markovic

Hi, Craig,

> From: Craig Janeczek 
> Sent: Tuesday, August 28, 2018 3:00 PM
> 
> Subject: [PATCH v3 0/8] Add limited MXU instruction support
> 
> This patch set begins to add MXU instruction support for mips emulation.

Based on the info I have, I think a reasonable approach to integration of this 
series would be:

- Add this line in mips-defs.h

#define   ASE_MXU   0x0200

- In main switch, use this segment

if (ctx->insn_flags & ASE_MXU) {
decode_opc_special2_mxu(env, ctx);
} else {
decode_opc_special2_legacy(env, ctx);
}

That way, you would be able to add MXU code without specifying CPU that 
supports it. This will enable you to focus on MXU, which is a serieoous task 
anyway. Hopefully, a CPU will be added at some later date.

Thanks,
Aleksandar

[Qemu-devel] [PATCH 1/1] spapr: Correct reference count on spapr-cpu-core

2018-08-30 Thread Sam Bobroff

spapr_init_cpus() currently creates spapr-cpu-core objects via
object_new() and setting their realized property to true. This leaves
their reference count at two, because object_new() adds an initial
reference and the realization attaches them to a default parent object
which also increments the reference count.

This causes a problem if one of these cores is hot unplugged: no
delete event is generated for it because it's reference count doesn't
reach zero when it is detached from it's parent.

Correct this by adding a call to object_unref() in spapr_init_cpus().

Signed-off-by: Sam Bobroff 
---
 hw/ppc/spapr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0d032a1ad0..fbb00f0c56 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2502,6 +2502,8 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
 object_property_set_int(core, core_id, CPU_CORE_PROP_CORE_ID,
 &error_fatal);
 object_property_set_bool(core, true, "realized", &error_fatal);
+
+object_unref(core);
 }
 }
 }
-- 
2.16.1.74.g9b0b1f47b

Re: [Qemu-devel] [PATCH v4 06/10] tests: add qmp/object-add-without-props test

Marc-André Lureau  writes:

> test_object_add_without_props() tests a bug in qmp_object_add() we
> fixed in commit e64c75a975.  Sadly, we don't have systematic
> object-add tests.  This lone test can go into qmp-cmd-test for want of
> a better home.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  tests/qmp-cmd-test.c | 31 +++
>  1 file changed, 31 insertions(+)
>
> diff --git a/tests/qmp-cmd-test.c b/tests/qmp-cmd-test.c
> index c5b70df974..3ba8f68476 100644
> --- a/tests/qmp-cmd-test.c
> +++ b/tests/qmp-cmd-test.c
> @@ -19,6 +19,15 @@
>  
>  const char common_args[] = "-nodefaults -machine none";
>  
> +static const char *get_error_class(QDict *resp)
> +{
> +QDict *error = qdict_get_qdict(resp, "error");
> +const char *desc = qdict_get_try_str(error, "desc");
> +
> +g_assert(desc);
> +return error ? qdict_get_try_str(error, "class") : NULL;
> +}
> +
>  /* Query smoke tests */
>  
>  static int query_error_class(const char *cmd)

Copied from qmp-test.c.  It should be factored out instead.  Where to
put it?  libqtest.c isn't quite right, as the function could
theoretically be useful in unit tests as well, but I guess it would do
for now.

Asserting presence of "desc" makes little sense outside qmp-test.c
protocol tests, but it doesn't hurt, either.

Grep shows more possible users in tests/drive_del-test.c and
tests/test-qga.c.

> @@ -197,6 +206,24 @@ static void add_query_tests(QmpSchema *schema)
>  }
>  }
>  
> +static void test_object_add_without_props(void)
> +{
> +QTestState *qts;
> +QDict *ret;

qmp-test.c and qmp-cmd-test.c commonly use @resp for the response.

> +
> +qts = qtest_init(common_args);
> +
> +ret = qtest_qmp(qts,
> +"{'execute': 'object-add', 'arguments':"
> +" {'qom-type': 'memory-backend-ram', 'id': 'ram1' } }");
> +g_assert_nonnull(ret);

What's wrong with g_assert(!ret)?

> +
> +g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
> +
> +qobject_unref(ret);

Permit me to digress.

When you expect success, you check @resp like this:

ret = qdict_get_qdict(resp, "return");
... laborously check @ret against expectations ...

If you feel pedantically thorough, you can throw in

g_assert(!qdict_haskey(resp, "error");

When you expect failure, you check like this:

error = qdict_get_qdict(resp, "error");
class = qdict_get_try_str(error, "class");
g_assert_cmpstr(class, ==, "GenericError");

and perhaps

g_assert(!qdict_haskey(resp, "return");

get_error_class() saves a little typing in the failure case.  It's still
an awfully verbose overall, and the checking is full of holes more often
than not.  There's got to be a better way.

> +qtest_quit(qts);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  QmpSchema schema;
> @@ -206,6 +233,10 @@ int main(int argc, char *argv[])
>  
>  qmp_schema_init(&schema);
>  add_query_tests(&schema);
> +
> +qtest_add_func("qmp/object-add-without-props",
> +   test_object_add_without_props);
> +
>  ret = g_test_run();
>  
>  qmp_schema_cleanup(&schema);

May I have a TODO comment asking for coverage of generic object-add
failure modes?

Re: [Qemu-devel] [PATCH v4 07/10] tests: add qmp/qom-set-without-value test

Marc-André Lureau  writes:

> test_qom_set_without_value() is about a bug in infrastructure used by
> the QMP core, fixed in commit c489780203.  We covered the bug in
> infrastructure unit tests (commit bce3035a44).  I wrote that test
> earlier, to cover QMP level as well, the test could go into qmp-test.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  tests/qmp-test.c | 18 ++
>  1 file changed, 18 insertions(+)
>
> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
> index 4ae2245484..fdfe73b6d2 100644
> --- a/tests/qmp-test.c
> +++ b/tests/qmp-test.c
> @@ -348,6 +348,23 @@ static void test_qmp_preconfig(void)
>  qtest_quit(qs);
>  }
>  
> +static void test_qom_set_without_value(void)
> +{
> +QTestState *qts;
> +QDict *ret;
> +
> +qts = qtest_init(common_args);
> +
> +ret = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
> +" { 'path': '/machine', 'property': 'rtc-time' } }");
> +g_assert_nonnull(ret);
> +
> +g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
> +
> +qobject_unref(ret);
> +qtest_quit(qts);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  g_test_init(&argc, &argv, NULL);
> @@ -355,6 +372,7 @@ int main(int argc, char *argv[])
>  qtest_add_func("qmp/protocol", test_qmp_protocol);
>  qtest_add_func("qmp/oob", test_qmp_oob);
>  qtest_add_func("qmp/preconfig", test_qmp_preconfig);
> +qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value);
>  
>  return g_test_run();
>  }

What we're testing here is a missing 'any' parameter.  The test should
be named accordingly.  Perhaps:

   qtest_add_func("qmp/missing-any", test_missing_any);

I can't bring myself to care for this test.  There are countless ways to
run a command with invalid parameters, and this is just one.  The only
thing that sets it apart is it used to tickle a visitor bug we've long
fixed, complete with a unit test to prevent regressions.  But the cost
of carrying the test is pretty low, so if you really want it, then
taking it without further argument is probably cheaper even if it's as
redundant as I suspect it is.

Re: [Qemu-devel] [PATCH v4 07/10] tests: add qmp/qom-set-without-value test

Hi

On Thu, Aug 30, 2018 at 3:05 PM, Markus Armbruster  wrote:
> Marc-André Lureau  writes:
>
>> test_qom_set_without_value() is about a bug in infrastructure used by
>> the QMP core, fixed in commit c489780203.  We covered the bug in
>> infrastructure unit tests (commit bce3035a44).  I wrote that test
>> earlier, to cover QMP level as well, the test could go into qmp-test.
>>
>> Signed-off-by: Marc-André Lureau 
>> ---
>>  tests/qmp-test.c | 18 ++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
>> index 4ae2245484..fdfe73b6d2 100644
>> --- a/tests/qmp-test.c
>> +++ b/tests/qmp-test.c
>> @@ -348,6 +348,23 @@ static void test_qmp_preconfig(void)
>>  qtest_quit(qs);
>>  }
>>
>> +static void test_qom_set_without_value(void)
>> +{
>> +QTestState *qts;
>> +QDict *ret;
>> +
>> +qts = qtest_init(common_args);
>> +
>> +ret = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
>> +" { 'path': '/machine', 'property': 'rtc-time' } }");
>> +g_assert_nonnull(ret);
>> +
>> +g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
>> +
>> +qobject_unref(ret);
>> +qtest_quit(qts);
>> +}
>> +
>>  int main(int argc, char *argv[])
>>  {
>>  g_test_init(&argc, &argv, NULL);
>> @@ -355,6 +372,7 @@ int main(int argc, char *argv[])
>>  qtest_add_func("qmp/protocol", test_qmp_protocol);
>>  qtest_add_func("qmp/oob", test_qmp_oob);
>>  qtest_add_func("qmp/preconfig", test_qmp_preconfig);
>> +qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value);
>>
>>  return g_test_run();
>>  }
>
> What we're testing here is a missing 'any' parameter.  The test should
> be named accordingly.  Perhaps:
>
>qtest_add_func("qmp/missing-any", test_missing_any);
>
> I can't bring myself to care for this test.  There are countless ways to
> run a command with invalid parameters, and this is just one.  The only
> thing that sets it apart is it used to tickle a visitor bug we've long
> fixed, complete with a unit test to prevent regressions.  But the cost
> of carrying the test is pretty low, so if you really want it, then
> taking it without further argument is probably cheaper even if it's as
> redundant as I suspect it is.

Yes, I would rather have it than drop it. It covers the QMP level as well.

Re: [Qemu-devel] [PATCH v4 08/10] tests: add a qmp success-response test

Marc-André Lureau  writes:

> Verify the usage of this schema feature and the API behaviour.  This
> should be the only case where qmp_dispatch() returns NULL without
> error.

Scratch "without error".

> Signed-off-by: Marc-André Lureau 

With the commit message corrected:
Reviewed-by: Markus Armbruster

Re: [Qemu-devel] [PATCH v3 0/8] Add limited MXU instruction support

2018-08-30 Thread Janeczek, Craig via Qemu-devel

That seems like a good plan to separate these tasks. I will implement this in 
the next patchset. Thank you

-Original Message-
From: Aleksandar Markovic  
Sent: Thursday, August 30, 2018 8:40 AM
To: Janeczek, Craig ; qemu-devel@nongnu.org
Cc: aurel...@aurel32.net; Petar Jovanovic ; Richard 
Henderson 
Subject: Re: [PATCH v3 0/8] Add limited MXU instruction support

Hi, Craig,

> From: Craig Janeczek 
> Sent: Tuesday, August 28, 2018 3:00 PM
> 
> Subject: [PATCH v3 0/8] Add limited MXU instruction support
> 
> This patch set begins to add MXU instruction support for mips emulation.

Based on the info I have, I think a reasonable approach to integration of this 
series would be:

- Add this line in mips-defs.h

#define   ASE_MXU   0x0200

- In main switch, use this segment

if (ctx->insn_flags & ASE_MXU) {
decode_opc_special2_mxu(env, ctx);
} else {
decode_opc_special2_legacy(env, ctx);
}

That way, you would be able to add MXU code without specifying CPU that 
supports it. This will enable you to focus on MXU, which is a serieoous task 
anyway. Hopefully, a CPU will be added at some later date.

Thanks,
Aleksandar

Re: [Qemu-devel] [PATCH] char-pty: remove unnecessary #ifdef

2018-08-30 Thread Roman Bolshakov

On Wed, Aug 22, 2018 at 12:38:35PM +0200, Paolo Bonzini wrote:
> For some reason __APPLE__ was not checked in pty code.  However, the #ifdef
> is redundant: this file is already compiled only if CONFIG_POSIX, same as
> util/qemu-openpty.c which it uses.
> 

Hi Paolo,

Is it possible to apply the patch for master and 3.1 branch?

I'm trying to bring up qemu + libvirt on macOS. If it goes to 3.1, then
libvirt 4.7.0 and QEMU 3.1 are going to be a working solution for
virtualization on macOS.

Lack of pty devices prevents a user from using valid domain xml with
serial console and virsh console command.

--
Thank you,
Roman

Re: [Qemu-devel] [PATCH v2 2/2] hw/arm: Add Arm Enterprise machine type

2018-08-30 Thread Ard Biesheuvel

On 30 August 2018 at 12:02, Leif Lindholm  wrote:
> On Thu, Aug 30, 2018 at 09:39:33AM +0100, Peter Maydell wrote:
>> On 30 August 2018 at 09:31, Leif Lindholm  wrote:
>> > On Thu, Aug 30, 2018 at 03:07:29PM +0800, Hongbo Zhang wrote:
>> >> @Ard, @Leif, is there any possibility to remove all the DT nodes?
>> >> On real hardware, how does UEFI find the memory size and CPU number?
>> >
>> > Usually by asking some form of SCP/PMU.
>> > So until we have rock-solid multi-cpu-instance (making terms up here,
>> > there may be a proper name already?) support upstream, we need to take
>> > shortcuts.
>>
>> I would expect that you'd want a "faked-on-the-QEMU end" SCP
>> communications endpoint to start with anyway. We have one of
>> those for vexpress, for instance.
>
> Ah, I wasn't aware we did!
>
> In that case - sure, that is probably the more sensible solution.
> Does it emulate SCMI?
>

The purpose of the SBSA/SBBR qemu machine is prototyping the
interactions between rich firmware and the OS for things like RAS,
secure variable storage for UEFI or capsule update.

How exactly the firmware figures out how many CPUs and how much memory
we are running with is out of scope for this, and so I don't think
there is a need to build something from scratch here: DT will do just
fine, given that both EDK2 and ARM-TF have working DT code for these
things.

fw_cfg, on the other hand, is unsuitable for us. Generating ACPI
tables in a different way from actual bare metal is not a good idea,
given that things like RAS interact with ACPI as well, and fw_cfg is
also exposed to the OS in the default mach-virt configuration. So we
could tweak fw_cfg to be more suitable, but I think that does not
improve the usefulness of our prototype at all.

At some point, it would be nice if we could get UEFI to receive its
platform description from ARM-TF in a generic manner, and it would be
good for EDK2 and ARM-TF to align on this. So even if we decide that
SCMI is more suitable way to convey this information from QEMU to the
guest firmware, I would object to implementing the client side of that
in UEFI, given that it should be ARM-TF that provides this
information.

Re: [Qemu-devel] [PATCH v9 1/6] hw/i386: add pc-i440fx-3.1 & pc-q35-3.1

2018-08-30 Thread Stefan Berger


On 08/28/2018 10:05 AM, Marc-André Lureau wrote:

ping
On Fri, Aug 10, 2018 at 5:34 PM Marc-André Lureau
 wrote:

The following patch is going to add compatiblity parameters.

Signed-off-by: Marc-André Lureau 
---
  include/hw/compat.h  |  4 +++-
  include/hw/i386/pc.h |  5 -
  hw/i386/pc_piix.c| 13 -
  hw/i386/pc_q35.c | 12 +++-
  4 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/include/hw/compat.h b/include/hw/compat.h
index c08f4040bb..f8e596969a 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -1,7 +1,9 @@
  #ifndef HW_COMPAT_H
  #define HW_COMPAT_H

-#define HW_COMPAT_2_12 \
+#define HW_COMPAT_3_0

Should probably be 3_1 now?

+
+#define HW_COMPAT_2_12  \
  {\
  .driver   = "migration",\
  .property = "decompress-error-check",\
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 6894f37df1..09b0365a8e 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -294,7 +294,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
  int e820_get_num_entries(void);
  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);

-#define PC_COMPAT_2_12 \
+#define PC_COMPAT_3_0 \

Same here?

+HW_COMPAT_3_0
+
+#define PC_COMPAT_2_12  \
  HW_COMPAT_2_12 \
  {\
  .driver   = TYPE_X86_CPU,\
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index dc09466b3e..7db1fa88ef 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -428,13 +428,24 @@ static void pc_i440fx_machine_options(MachineClass *m)
  machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
  }

-static void pc_i440fx_3_0_machine_options(MachineClass *m)
+static void pc_i440fx_3_1_machine_options(MachineClass *m)
  {
  pc_i440fx_machine_options(m);
  m->alias = "pc";
  m->is_default = 1;
  }

+DEFINE_I440FX_MACHINE(v3_1, "pc-i440fx-3.1", NULL,
+  pc_i440fx_3_1_machine_options);
+
+static void pc_i440fx_3_0_machine_options(MachineClass *m)

should this be the 3_1  rather than the one above?

+{
+pc_i440fx_machine_options(m);
+m->is_default = 0;
+m->alias = NULL;
+SET_MACHINE_COMPAT(m, PC_COMPAT_3_0);
+}
+
  DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL,
pc_i440fx_3_0_machine_options);

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 532241e3f8..cb1980a088 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -311,12 +311,22 @@ static void pc_q35_machine_options(MachineClass *m)
  m->max_cpus = 288;
  }

-static void pc_q35_3_0_machine_options(MachineClass *m)
+static void pc_q35_3_1_machine_options(MachineClass *m)
  {
  pc_q35_machine_options(m);
  m->alias = "q35";
  }

+DEFINE_Q35_MACHINE(v3_1, "pc-q35-3.1", NULL,
+   pc_q35_3_1_machine_options);
+
+static void pc_q35_3_0_machine_options(MachineClass *m)
+{
+pc_q35_machine_options(m);
+m->alias = NULL;
+SET_MACHINE_COMPAT(m, PC_COMPAT_3_0);
+}
+
  DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL,
  pc_q35_3_0_machine_options);

--
2.18.0.547.g1d89318c48

Re: [Qemu-devel] [PATCH v2 2/2] hw/arm: Add Arm Enterprise machine type

2018-08-30 Thread Leif Lindholm

On Thu, Aug 30, 2018 at 03:29:02PM +0200, Ard Biesheuvel wrote:
> On 30 August 2018 at 12:02, Leif Lindholm  wrote:
> > On Thu, Aug 30, 2018 at 09:39:33AM +0100, Peter Maydell wrote:
> >> On 30 August 2018 at 09:31, Leif Lindholm  wrote:
> >> > On Thu, Aug 30, 2018 at 03:07:29PM +0800, Hongbo Zhang wrote:
> >> >> @Ard, @Leif, is there any possibility to remove all the DT nodes?
> >> >> On real hardware, how does UEFI find the memory size and CPU number?
> >> >
> >> > Usually by asking some form of SCP/PMU.
> >> > So until we have rock-solid multi-cpu-instance (making terms up here,
> >> > there may be a proper name already?) support upstream, we need to take
> >> > shortcuts.
> >>
> >> I would expect that you'd want a "faked-on-the-QEMU end" SCP
> >> communications endpoint to start with anyway. We have one of
> >> those for vexpress, for instance.
> >
> > Ah, I wasn't aware we did!
> >
> > In that case - sure, that is probably the more sensible solution.
> > Does it emulate SCMI?
> 
> The purpose of the SBSA/SBBR qemu machine is prototyping the
> interactions between rich firmware and the OS for things like RAS,
> secure variable storage for UEFI or capsule update.

I think it would be useful to at some point have also an open-source
SCP implementation and tie it into this platform (if feasible). I do
not however think it is important, and certainly nothing we need to
worry about for now.

> How exactly the firmware figures out how many CPUs and how much memory
> we are running with is out of scope for this, and so I don't think
> there is a need to build something from scratch here: DT will do just
> fine, given that both EDK2 and ARM-TF have working DT code for these
> things.

Agreed.

> fw_cfg, on the other hand, is unsuitable for us. Generating ACPI
> tables in a different way from actual bare metal is not a good idea,
> given that things like RAS interact with ACPI as well, and fw_cfg is
> also exposed to the OS in the default mach-virt configuration. So we
> could tweak fw_cfg to be more suitable, but I think that does not
> improve the usefulness of our prototype at all.

Indeed.

> At some point, it would be nice if we could get UEFI to receive its
> platform description from ARM-TF in a generic manner, and it would be
> good for EDK2 and ARM-TF to align on this. So even if we decide that
> SCMI is more suitable way to convey this information from QEMU to the
> guest firmware, I would object to implementing the client side of that
> in UEFI, given that it should be ARM-TF that provides this
> information.

Yes, but the modeled SCP is relevant for the ARM-TF port for this
platform. A "faked-on-the-QEMU end" SCP that communicates via SCMI (or
if that's been superseded, I may have lost track) allows for sharing
generic arm-tf code for retrieving this information with most hardware
platforms.

And that _would_ be nicer than having the lowest layers of the EDK2
port looking more like hardware than mach-virt.

This is of course also something that could be addressed at a later
date - I'm not sure how generic arm-tf is in this regard today.

/
Leif

Re: [Qemu-devel] [PATCH v9 1/6] hw/i386: add pc-i440fx-3.1 & pc-q35-3.1

Hi
On Thu, Aug 30, 2018 at 3:54 PM Stefan Berger
 wrote:
>
> On 08/28/2018 10:05 AM, Marc-André Lureau wrote:
> > ping
> > On Fri, Aug 10, 2018 at 5:34 PM Marc-André Lureau
> >  wrote:
> >> The following patch is going to add compatiblity parameters.
> >>
> >> Signed-off-by: Marc-André Lureau 
> >> ---
> >>   include/hw/compat.h  |  4 +++-
> >>   include/hw/i386/pc.h |  5 -
> >>   hw/i386/pc_piix.c| 13 -
> >>   hw/i386/pc_q35.c | 12 +++-
> >>   4 files changed, 30 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/include/hw/compat.h b/include/hw/compat.h
> >> index c08f4040bb..f8e596969a 100644
> >> --- a/include/hw/compat.h
> >> +++ b/include/hw/compat.h
> >> @@ -1,7 +1,9 @@
> >>   #ifndef HW_COMPAT_H
> >>   #define HW_COMPAT_H
> >>
> >> -#define HW_COMPAT_2_12 \
> >> +#define HW_COMPAT_3_0
> Should probably be 3_1 now?
> >> +
> >> +#define HW_COMPAT_2_12  \
> >>   {\
> >>   .driver   = "migration",\
> >>   .property = "decompress-error-check",\
> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >> index 6894f37df1..09b0365a8e 100644
> >> --- a/include/hw/i386/pc.h
> >> +++ b/include/hw/i386/pc.h
> >> @@ -294,7 +294,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
> >>   int e820_get_num_entries(void);
> >>   bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
> >>
> >> -#define PC_COMPAT_2_12 \
> >> +#define PC_COMPAT_3_0 \
> Same here?
> >> +HW_COMPAT_3_0
> >> +
> >> +#define PC_COMPAT_2_12  \
> >>   HW_COMPAT_2_12 \
> >>   {\
> >>   .driver   = TYPE_X86_CPU,\
> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >> index dc09466b3e..7db1fa88ef 100644
> >> --- a/hw/i386/pc_piix.c
> >> +++ b/hw/i386/pc_piix.c
> >> @@ -428,13 +428,24 @@ static void pc_i440fx_machine_options(MachineClass 
> >> *m)
> >>   machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
> >>   }
> >>
> >> -static void pc_i440fx_3_0_machine_options(MachineClass *m)
> >> +static void pc_i440fx_3_1_machine_options(MachineClass *m)
> >>   {
> >>   pc_i440fx_machine_options(m);
> >>   m->alias = "pc";
> >>   m->is_default = 1;
> >>   }
> >>
> >> +DEFINE_I440FX_MACHINE(v3_1, "pc-i440fx-3.1", NULL,
> >> +  pc_i440fx_3_1_machine_options);
> >> +
> >> +static void pc_i440fx_3_0_machine_options(MachineClass *m)
> should this be the 3_1  rather than the one above?

3_1 is just above :)

> >> +{
> >> +pc_i440fx_machine_options(m);
> >> +m->is_default = 0;
> >> +m->alias = NULL;
> >> +SET_MACHINE_COMPAT(m, PC_COMPAT_3_0);
> >> +}
> >> +
> >>   DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL,
> >> pc_i440fx_3_0_machine_options);
> >>
> >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> >> index 532241e3f8..cb1980a088 100644
> >> --- a/hw/i386/pc_q35.c
> >> +++ b/hw/i386/pc_q35.c
> >> @@ -311,12 +311,22 @@ static void pc_q35_machine_options(MachineClass *m)
> >>   m->max_cpus = 288;
> >>   }
> >>
> >> -static void pc_q35_3_0_machine_options(MachineClass *m)
> >> +static void pc_q35_3_1_machine_options(MachineClass *m)
> >>   {
> >>   pc_q35_machine_options(m);
> >>   m->alias = "q35";
> >>   }
> >>
> >> +DEFINE_Q35_MACHINE(v3_1, "pc-q35-3.1", NULL,
> >> +   pc_q35_3_1_machine_options);
> >> +
> >> +static void pc_q35_3_0_machine_options(MachineClass *m)
> >> +{
> >> +pc_q35_machine_options(m);
> >> +m->alias = NULL;
> >> +SET_MACHINE_COMPAT(m, PC_COMPAT_3_0);
> >> +}
> >> +
> >>   DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL,
> >>   pc_q35_3_0_machine_options);
> >>
> >> --
> >> 2.18.0.547.g1d89318c48
> >>
> >>
> >
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v4 09/10] qga: process_event() simplification

Marc-André Lureau  writes:

> Simplify the code around qmp_dispatch():
> - rely on qmp_dispatch/check_obj() for message checking
> - have a single send_response() point
> - constify send_response() argument
>
> It changes a couple of error messages:
>
> * When @req isn't a dictionary, from
> Invalid JSON syntax
>   to
> QMP input must be a JSON object
>
> * When @req lacks member "execute", from
> this feature or command is not currently supported
>   to
> QMP input lacks member 'execute'
>
> Signed-off-by: Marc-André Lureau 
> Reviewed-by: Michael Roth 

Reviewed-by: Markus Armbruster

Re: [Qemu-devel] [PATCH v6 03/13] qapi: remove error checks for event emission

Markus Armbruster  writes:

> Peter Xu  writes:
>
>> In the whole QAPI event emission code we're passing in an Error* object
>> along the whole stack.  That's never useful since it never fails after
>> all.  Remove that.
>
> This is the interesting part.  We'll see below why it can't fail.
>
>> Then, remove that parameter from all the callers to send an event.
>
> This is the mechanical part.  The callers pass &error_abort, except for
> one that passes NULL.
>
> The patch moves the &error_abort argument from the qapi_event_send_FOO()
> calls to the places where the argument is used.  No effect except for
> the caller that passes NULL.  That one now asserts "no error".
>
>> Suggested-by: Eric Blake 
>> Suggested-by: Markus Armbruster 
>> Signed-off-by: Peter Xu 
>> ---
>
> Patch quoted except for the parts that drop &error_abort from
> qapi_event_send_FOO() calls:
>
> [...]
>> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
>> index c2e11465f0..6d3cffd548 100644
>> --- a/docs/devel/qapi-code-gen.txt
>> +++ b/docs/devel/qapi-code-gen.txt

Missing:

   @@ -1340,7 +1340,7 @@ Example:
#include "example-qapi-types.h"


   -void qapi_event_send_my_event(Error **errp);
   +void qapi_event_send_my_event(void);

typedef enum example_QAPIEvent {
EXAMPLE_QAPI_EVENT_MY_EVENT = 0,

>> @@ -1356,10 +1356,9 @@ Example:
>>  $ cat qapi-generated/example-qapi-events.c
>>  [Uninteresting stuff omitted...]
>>  
>> -void qapi_event_send_my_event(Error **errp)
>> +void qapi_event_send_my_event(void)
>>  {
>>  QDict *qmp;
>> -Error *err = NULL;
>>  QMPEventFuncEmit emit;
>>  
>>  emit = qmp_event_get_func_emit();
>> @@ -1369,9 +1368,8 @@ Example:
>>  
>>  qmp = qmp_event_build_dict("MY_EVENT");
>>  
>> -emit(EXAMPLE_QAPI_EVENT_MY_EVENT, qmp, &err);
>> +emit(EXAMPLE_QAPI_EVENT_MY_EVENT, qmp);
>>  
>> -error_propagate(errp, err);
>>  qobject_unref(qmp);
>>  }
>>  
[...]

Re: [Qemu-devel] [PATCH] vl:c: make sure that sockets are calculated correctly in '-smp X' case

2018-08-30 Thread Eduardo Habkost

On Thu, Aug 30, 2018 at 09:58:51AM +0200, Igor Mammedov wrote:
> On Wed, 29 Aug 2018 14:33:01 -0300
> Eduardo Habkost  wrote:
> 
> > On Wed, Aug 29, 2018 at 04:32:01PM +0200, Igor Mammedov wrote:
> > > commit
> > >   (5cdc9b76e3 vl.c: Remove dead assignment)
> > > removed sockets calculation when 'sockets' weren't provided on CLI
> > > since there wasn't any users for it back then. Exiting checks
> > > are neither reachable
> > >} else if (sockets * cores * threads < cpus) {
> > > or nor triggable
> > >if (sockets * cores * threads > max_cpus)
> > > so we weren't noticing wrong topology since then, since users
> > > recalculate sockets adhoc on their own.
> > > 
> > > However with deprecation check it becomes noticable, for example
> > >   -smp 2
> > > will start printing warning:
> > >   "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * 
> > > threads (1) != maxcpus (2)"
> > > calculating sockets if they weren't specified.
> > > 
> > > Fix it by returning back sockets calculation if it's omited on CLI.
> > > 
> > > Signed-off-by: Igor Mammedov 
> > > ---
> > >  vl.c | 5 -
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/vl.c b/vl.c
> > > index 7fd700e..333d638 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
> > >  
> > >  /* compute missing values, prefer sockets over cores over 
> > > threads */
> > >  if (cpus == 0 || sockets == 0) {
> > > -sockets = sockets > 0 ? sockets : 1;
> > >  cores = cores > 0 ? cores : 1;
> > >  threads = threads > 0 ? threads : 1;
> > >  if (cpus == 0) {
> > > +sockets = sockets > 0 ? sockets : 1;
> > >  cpus = cores * threads * sockets;
> > > +} else {
> > > +max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);  
> > 
> > We already have a "max_cpus = qemu_opt_get_number(...)" line in
> > this function[1]:
> > 
> > /* compute missing values, prefer sockets over cores over threads */
> > if (cpus == 0 || sockets == 0) {
> > sockets = sockets > 0 ? sockets : 1;
> > cores = cores > 0 ? cores : 1;
> > threads = threads > 0 ? threads : 1;
> > if (cpus == 0) {
> > cpus = cores * threads * sockets;
> > }
> > } else if (cores == 0) {
> > [...]
> > } else if (threads == 0) {
> > [...]
> > } else if (sockets * cores * threads < cpus) {
> > [...]
> > }
> > 
> > max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus); /* [1] */
> > 
> > /* [2] */
> > 
> > if (max_cpus < cpus) {
> > error_report("maxcpus must be equal to or greater than smp");
> > exit(1);
> > }
> > 
> > Why not move the sockets == 0 check to [2]?
> it won't work, since the rest 'else if' depends on sockets being non 0
> which is ensured by (cpus == 0 || sockets == 0) branch, I can split this in 2
> equivalent conditions. It's more code but it might be easier to read

I like your original patch better.  I wasn't thinking about the
"cpus == 0 || sockets == 0" check, but about this line:

   sockets = !sockets ? max_cpus / (cores * threads) : sockets;

Just moving this line to [2] would be enough, wouldn't it?

> 
[...]
> > 
> > 
> > > +sockets = !sockets ? max_cpus / (cores * threads) : 
> > > sockets;  
> > 
> > The two patches in this thread make QEMU print a warning on a
> > case that was never documented as invalid/deprecated: maxcpus now
> > needs to be a multiple of cores*threads.
> > 
> > However, the error message doesn't make it obvious:
> > 
> >   $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3
> >   qemu-system-x86_64: warning: Invalid CPU topology deprecated: sockets (2) 
> > * cores (3) * threads (1) != maxcpus (7)
> to me it look pretty obvious, fix one side of equation to make sure that VM
> starts in the future.
> 
> > I think this would make more sense:
> > 
> >   $ ./x86_64-softmmu/qemu-system-x86_64 -smp 7,cores=3
> >   qemu-system-x86_64: warning: Invalid CPU topology deprecated: maxcpus (7) 
> > is not a multiple of cores (3) * threads (1)
> I think it will add not necessary warning/code, but if you really
> think we need it, I can add it to 'vl.c deprecate incorrect CPUs
> topology' on respin.

My main worry is about insufficient documentation: we need to
clarify that the s*c*t == maxcpus rule applies even if "sockets"
is omitted in the command-line, and that this indirectly requires
that maxcpus be a multiple of c*6.

That said, I won't mind if you keep the existing message in this
patch for code simplicity, as long as the documentation is
updated.

> 
> > 
> > 
> > >  }
> > >  } else if (cores == 0) {
> > >  threads = threads > 0 ? threads : 1;
> > > -- 
> > > 2.7.4
> > >   
> > 
> 

-- 
Eduar

Re: [Qemu-devel] [PATCH v4 00/10] monitor: various code simplification and fixes

Marc-André Lureau  writes:

> Hi,
>
> This series is a rebased subset of "[PATCH v3 00/38] RFC: monitor: add
> asynchronous command type".

PATCH 01-04 are core monitor work Peter will need to make progress.
Queued.
http://repo.or.cz/qemu/armbru.git/shortlog/refs/heads/monitor-next

PATCH 05 I'd like you to keep in your qmp-async series.

PATCH 06-07 could use a bit of work.

PATCH 08-10 are related to QGA.  Michael, would you like me to handle
them?

Re: [Qemu-devel] [PATCH 1/9] char.h: fix gtk-doc comment style

Marc-André Lureau  writes:

> Use the gtk-doc function comment style, as documented in:
> https://developer.gnome.org/gtk-doc-manual/stable/documenting_symbols.html.en

I'm no friend of this style, but these headers use it already, except
they get it wrong in places.  Your subject's "fix" expresses that, but
then your commit message body suggests a conversion to gtk-doc style.
Suggest to replace it by

  Fix up conformance to GTK-Doc function comment style, as documented in
  https://developer.gnome.org/gtk-doc-manual/stable/documenting_symbols.html.en

>
> Signed-off-by: Marc-André Lureau 
> ---
>  include/chardev/char-fe.h | 81 ++-
>  include/chardev/char.h| 61 +
>  2 files changed, 63 insertions(+), 79 deletions(-)
>
> diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
> index c67271f1ba..21071f1fb1 100644
> --- a/include/chardev/char-fe.h
> +++ b/include/chardev/char-fe.h
> @@ -20,7 +20,7 @@ struct CharBackend {
>  };
>  
>  /**
> - * @qemu_chr_fe_init:
> + * qemu_chr_fe_init:

Preexisting: fails to document parameters.  Leaving them undocumented is
acceptable for this style-fix patch.  More of the same below.

>   *
>   * Initializes a front end for the given CharBackend and
>   * Chardev. Call qemu_chr_fe_deinit() to remove the association and
> @@ -31,7 +31,7 @@ struct CharBackend {
>  bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp);
>  
>  /**
> - * @qemu_chr_fe_deinit:
> + * qemu_chr_fe_deinit:
>   * @b: a CharBackend
>   * @del: if true, delete the chardev backend
>  *
> @@ -42,9 +42,9 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error 
> **errp);
>  void qemu_chr_fe_deinit(CharBackend *b, bool del);
>  
>  /**
> - * @qemu_chr_fe_get_driver:
> + * qemu_chr_fe_get_driver:
>   *
> - * Returns the driver associated with a CharBackend or NULL if no
> + * Returns: the driver associated with a CharBackend or NULL if no
>   * associated Chardev.
>   * Note: avoid this function as the driver should never be accessed directly,
>   *   especially by the frontends that support chardevice hotswap.
> @@ -53,21 +53,21 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del);
>  Chardev *qemu_chr_fe_get_driver(CharBackend *be);
>  
>  /**
> - * @qemu_chr_fe_backend_connected:
> + * qemu_chr_fe_backend_connected:
>   *
> - * Returns true if there is a chardevice associated with @be.
> + * Returns: true if there is a chardevice associated with @be.
>   */
>  bool qemu_chr_fe_backend_connected(CharBackend *be);
>  
>  /**
> - * @qemu_chr_fe_backend_open:
> + * qemu_chr_fe_backend_open:
>   *
> - * Returns true if chardevice associated with @be is open.
> + * Returns: true if chardevice associated with @be is open.
>   */
>  bool qemu_chr_fe_backend_open(CharBackend *be);
>  
>  /**
> - * @qemu_chr_fe_set_handlers:
> + * qemu_chr_fe_set_handlers:
>   * @b: a CharBackend
>   * @fd_can_read: callback to get the amount of data the frontend may
>   *   receive
> @@ -95,7 +95,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
>bool set_open);
>  
>  /**
> - * @qemu_chr_fe_take_focus:
> + * qemu_chr_fe_take_focus:
>   *
>   * Take the focus (if the front end is muxed).
>   *
> @@ -104,14 +104,14 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
>  void qemu_chr_fe_take_focus(CharBackend *b);
>  
>  /**
> - * @qemu_chr_fe_accept_input:
> + * qemu_chr_fe_accept_input:
>   *
>   * Notify that the frontend is ready to receive data
>   */
>  void qemu_chr_fe_accept_input(CharBackend *be);
>  
>  /**
> - * @qemu_chr_fe_disconnect:
> + * qemu_chr_fe_disconnect:
>   *
>   * Close a fd accepted by character backend.
>   * Without associated Chardev, do nothing.
> @@ -119,7 +119,7 @@ void qemu_chr_fe_accept_input(CharBackend *be);
>  void qemu_chr_fe_disconnect(CharBackend *be);
>  
>  /**
> - * @qemu_chr_fe_wait_connected:
> + * qemu_chr_fe_wait_connected:
>   *
>   * Wait for characted backend to be connected, return < 0 on error or
>   * if no associated Chardev.

Preexisting: missing Returns:.  Again, leaving it that way is acceptable
for this style-fix patch.

> @@ -127,19 +127,18 @@ void qemu_chr_fe_disconnect(CharBackend *be);
>  int qemu_chr_fe_wait_connected(CharBackend *be, Error **errp);
>  
>  /**
> - * @qemu_chr_fe_set_echo:
> + * qemu_chr_fe_set_echo:
> + * @echo true to enable echo, false to disable echo

Colon after @echo, please.

>   *
>   * Ask the backend to override its normal echo setting.  This only really
>   * applies to the stdio backend and is used by the QMP server such that you
>   * can see what you type if you try to type QMP commands.
>   * Without associated Chardev, do nothing.
> - *
> - * @echo true to enable echo, false to disable echo
>   */
>  void qemu_chr_fe_set_echo(CharBackend *be, bool echo);
>  
>  /**
> - * @qemu_chr_fe_set_open:
> + * qemu_chr_fe_set_open:
>   *
>   * Set character frontend open status.  This is an indication that the
>

[Qemu-devel] [RFC 5/6] virtio-net: steering mode: Implement rss support

From: Sameeh Jubran 

Signed-off-by: Sameeh Jubran 
---
 hw/net/virtio-net.c | 122 
 1 file changed, 105 insertions(+), 17 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index e7c4ce6f66..4a52a6a1d0 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -972,41 +972,129 @@ static int virtio_net_handle_mq(VirtIONet *n, uint8_t 
cmd,
 return VIRTIO_NET_OK;
 }
 
-static int virtio_net_ctrl_steering_mode(VirtIONet *n, uint8_t cmd,
+
+static int virtio_net_ctrl_sm_rss(VirtIONet *n, uint32_t cmd,
 struct iovec *iov, unsigned int iov_cnt,
 struct iovec *iov_in, unsigned int iov_cnt_in,
-size_t *size_in)
+size_t *size_in)
+{
+size_t s;
+uint32_t supported_hash_function = 0;
+
+switch (cmd) {
+case VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS:
+supported_hash_function |= RSS_HASH_FUNCTION_TOEPLITZ;
+if (!size_in) {
+return VIRTIO_NET_ERR;
+}
+s = iov_from_buf(iov_in, iov_cnt_in, 0,
+&supported_hash_function,
+supported_hash_function);
+if (s != sizeof(n->supported_modes) ||
+!size_in) {
+return VIRTIO_NET_ERR;
+}
+*size_in = s;
+break;
+case VIRTIO_NET_SM_CTRL_RSS_SET:
+if (!n->rss_conf) {
+n->rss_conf = g_malloc0(
+sizeof(struct virtio_net_rss_conf));
+} else if (iov == NULL || iov_cnt == 0) {
+g_free(n->rss_conf->ptrs.hash_key);
+g_free(n->rss_conf->ptrs.indirection_table);
+g_free(n->rss_conf);
+return VIRTIO_NET_OK;
+}
+s = iov_to_buf(iov, iov_cnt, 0, n->rss_conf,
+sizeof(struct virtio_net_rss_conf) -
+sizeof(struct virtio_net_rss_conf_ptrs));
+
+if (s != sizeof(struct virtio_net_rss_conf) -
+sizeof(struct virtio_net_rss_conf_ptrs)) {
+return VIRTIO_NET_ERR;
+}
+n->rss_conf->ptrs.hash_key = g_malloc0(sizeof(uint8_t) *
+n->rss_conf->hash_key_length);
+s = iov_to_buf(iov, iov_cnt, 0, n->rss_conf->ptrs.hash_key,
+sizeof(uint8_t) * n->rss_conf->hash_key_length);
+if (s != sizeof(uint8_t) * n->rss_conf->hash_key_length) {
+g_free(n->rss_conf->ptrs.hash_key);
+return VIRTIO_NET_ERR;
+}
+n->rss_conf->ptrs.indirection_table
+= g_malloc0(sizeof(uint32_t) *
+n->rss_conf->indirection_table_length);
+s = iov_to_buf(iov, iov_cnt, 0,
+n->rss_conf->ptrs.indirection_table, sizeof(uint32_t) *
+n->rss_conf->indirection_table_length);
+if (s != sizeof(uint32_t) *
+n->rss_conf->indirection_table_length) {
+g_free(n->rss_conf->ptrs.hash_key);
+g_free(n->rss_conf->ptrs.indirection_table);
+return VIRTIO_NET_ERR;
+}
+/* do bpf magic */
+break;
+default:
+return VIRTIO_NET_ERR;
+}
+
+return VIRTIO_NET_OK;
+}
+
+static int virtio_net_ctrl_steering_mode(VirtIONet *n, uint8_t cmd,
+struct iovec *iov, unsigned int iov_cnt,
+struct iovec *iov_in, unsigned int iov_in_cnt,
+size_t *size_in)
 {
 size_t s;
 struct virtio_net_steering_mode sm;
+int status = 0;
+size_t size_in_cmd = 0;
 
 switch (cmd) {
 case VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES:
 if (!size_in) {
 return VIRTIO_NET_ERR;
 }
-  s = iov_from_buf(iov_in, iov_cnt_in, 0,
-  &n->supported_modes, sizeof(n->supported_modes));
+n->supported_modes.steering_modes |= STEERING_MODE_RSS |
+STEERING_MODE_AUTO;
+s = iov_from_buf(iov_in, iov_in_cnt, 0,
+&n->supported_modes,
+sizeof(n->supported_modes));
 if (s != sizeof(n->supported_modes) ||
-  !size_in) {
+!size_in) {
 return VIRTIO_NET_ERR;
 }
-  *size_in = s;
-  break;
+*size_in = s;
+ break;
 case VIRTIO_NET_CTRL_SM_CONTROL:
-s = iov_to_buf(iov, iov_cnt, 0, &sm, sizeof(sm) -
-sizeof(union command_data));
-if (s != sizeof(sm) - sizeof(union command_data)) {
+s = iov_to_buf(iov, iov_cnt, 0, &sm, sizeof(sm));
+if (s != sizeof(sm)) {
+return VIRTIO_NET_ERR;
+}
+iov_discard_front(&iov, &iov_cnt, sizeof(sm));
+/* TODO handle the case where we change mode, call the old */
+/* mode function with null ptrs  should do the trick of */
+/* freeing any resources */
+switch (sm.steering_mode) {
+case STEERING_MODE_AUTO:
+break;
+case STEERING_MODE_RS

[Qemu-devel] [PATCH] seccomp: check TSYNC host capability

Remove -sandbox option if the host is not capable of TSYNC, since the
sandbox will fail at setup time otherwise. This will help libvirt, for
ex, to figure out if -sandbox will work.

Signed-off-by: Marc-André Lureau 
---
 qemu-seccomp.c | 19 ++-
 vl.c   |  4 ++--
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index 4729eb107f..1baa5c69ed 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -282,7 +282,24 @@ static QemuOptsList qemu_sandbox_opts = {
 
 static void seccomp_register(void)
 {
-qemu_add_opts(&qemu_sandbox_opts);
+bool add = false;
+
+/* FIXME: use seccomp_api_get() >= 2 check when released */
+
+#if defined(SECCOMP_FILTER_FLAG_TSYNC)
+int check;
+
+/* check host TSYNC capability, it returns errno == ENOSYS if unavailable 
*/
+check = qemu_seccomp(SECCOMP_SET_MODE_FILTER,
+ SECCOMP_FILTER_FLAG_TSYNC, NULL);
+if (check < 0 && errno == EFAULT) {
+add = true;
+}
+#endif
+
+if (add) {
+qemu_add_opts(&qemu_sandbox_opts);
+}
 }
 opts_init(seccomp_register);
 #endif
diff --git a/vl.c b/vl.c
index 408bff9c1e..70c2449b86 100644
--- a/vl.c
+++ b/vl.c
@@ -4007,8 +4007,8 @@ int main(int argc, char **argv, char **envp)
 }
 
 #ifdef CONFIG_SECCOMP
-if (qemu_opts_foreach(qemu_find_opts("sandbox"),
-  parse_sandbox, NULL, NULL)) {
+olist = qemu_find_opts_err("sandbox", NULL);
+if (olist && qemu_opts_foreach(olist, parse_sandbox, NULL, NULL)) {
 exit(1);
 }
 #endif
-- 
2.19.0.rc0.48.gb9dfa238d5

[Qemu-devel] [RFC 0/6] Virtio-net: Support RSS

From: Sameeh Jubran 

This series implements the Steering Mode feature which was introduced on the
virtio-dev list a while ago, which can be found here:
* https://lists.oasis-open.org/archives/virtio-dev/201805/msg00024.html

The first three patches add some infrastructure support that is used in
the following three patches.

The ebpf filter doesn't fully work yet as I'm having an issue with the
verifier which needs to be fixed.

The patches still need some love as not all of the cases have been handled
yet most of the functionality has been implemented.

Please share your thoughts and comments so I'll move forward with
sending v1 along with a fully functioning ebpf code.

Sameeh Jubran (6):
  Add bpf support to qemu
  tap: Add support for bpf ioctls
  vhost-net: Expose vhost_net_get_fd
  virtio-net: implement steering mode feature
  virtio-net: steering mode: Implement rss support
  virtio-net: rss: Add bpf filter

 MAINTAINERS |5 +
 configure   |   44 +
 hw/net/rss_bpf_insns.h  | 3992 +++
 hw/net/rss_tap_bpf.h|   37 +
 hw/net/rss_tap_bpf_program.c|  172 ++
 hw/net/vhost_net.c  |2 +-
 hw/net/virtio-net.c |  250 +-
 include/hw/virtio/virtio-net.h  |5 +
 include/net/net.h   |3 +-
 include/standard-headers/linux/virtio_net.h |   55 +
 net/tap-bsd.c   |5 +
 net/tap-linux.c |   29 +-
 net/tap-linux.h |3 +-
 net/tap-solaris.c   |5 +
 net/tap-stub.c  |5 +
 net/tap.c   |8 +
 net/tap_int.h   |1 +
 qapi/net.json   |   11 +
 scripts/update-linux-headers.sh |8 +-
 19 files changed, 4627 insertions(+), 13 deletions(-)
 create mode 100644 hw/net/rss_bpf_insns.h
 create mode 100644 hw/net/rss_tap_bpf.h
 create mode 100644 hw/net/rss_tap_bpf_program.c

-- 
2.13.6

[Qemu-devel] [RFC 3/6] vhost-net: Expose vhost_net_get_fd

From: Sameeh Jubran 

Signed-off-by: Sameeh Jubran 
---
 hw/net/vhost_net.c | 2 +-
 include/hw/virtio/virtio-net.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index e037db63a3..c0bff725c9 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -129,7 +129,7 @@ uint64_t vhost_net_get_acked_features(VHostNetState *net)
 return net->dev.acked_features;
 }
 
-static int vhost_net_get_fd(NetClientState *backend)
+int vhost_net_get_fd(NetClientState *backend)
 {
 switch (backend->info->type) {
 case NET_CLIENT_DRIVER_TAP:
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 02484dc94c..a7b53edc96 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -107,4 +107,6 @@ typedef struct VirtIONet {
 void virtio_net_set_netclient_name(VirtIONet *n, const char *name,
const char *type);
 
+int vhost_net_get_fd(NetClientState *backend);
+
 #endif
-- 
2.13.6

[Qemu-devel] [RFC 4/6] virtio-net: implement steering mode feature

From: Sameeh Jubran 

Signed-off-by: Sameeh Jubran 
---
 hw/net/virtio-net.c | 65 +
 include/hw/virtio/virtio-net.h  |  3 ++
 include/standard-headers/linux/virtio_net.h | 55 
 3 files changed, 116 insertions(+), 7 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 90502fca7c..e7c4ce6f66 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -972,13 +972,53 @@ static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd,
 return VIRTIO_NET_OK;
 }
 
+static int virtio_net_ctrl_steering_mode(VirtIONet *n, uint8_t cmd,
+struct iovec *iov, unsigned int iov_cnt,
+struct iovec *iov_in, unsigned int iov_cnt_in,
+size_t *size_in)
+{
+size_t s;
+struct virtio_net_steering_mode sm;
+
+switch (cmd) {
+case VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES:
+if (!size_in) {
+return VIRTIO_NET_ERR;
+}
+  s = iov_from_buf(iov_in, iov_cnt_in, 0,
+  &n->supported_modes, sizeof(n->supported_modes));
+if (s != sizeof(n->supported_modes) ||
+  !size_in) {
+return VIRTIO_NET_ERR;
+}
+  *size_in = s;
+  break;
+case VIRTIO_NET_CTRL_SM_CONTROL:
+s = iov_to_buf(iov, iov_cnt, 0, &sm, sizeof(sm) -
+sizeof(union command_data));
+if (s != sizeof(sm) - sizeof(union command_data)) {
+return VIRTIO_NET_ERR;
+}
+/* switch (cmd)
+ {
+dafault:
+return VIRTIO_NET_ERR;
+ } */
+  break;
+default:
+return VIRTIO_NET_ERR;
+}
+
+return VIRTIO_NET_OK;
+}
+
 static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
 {
 VirtIONet *n = VIRTIO_NET(vdev);
 struct virtio_net_ctrl_hdr ctrl;
 virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
 VirtQueueElement *elem;
-size_t s;
+size_t s, elem_in_size = 0;
 struct iovec *iov, *iov2;
 unsigned int iov_cnt;
 
@@ -996,7 +1036,8 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, 
VirtQueue *vq)
 }
 
 iov_cnt = elem->out_num;
-iov2 = iov = g_memdup(elem->out_sg, sizeof(struct iovec) * 
elem->out_num);
+iov2 = iov = g_memdup(elem->out_sg, sizeof(struct iovec) *
+elem->out_num);
 s = iov_to_buf(iov, iov_cnt, 0, &ctrl, sizeof(ctrl));
 iov_discard_front(&iov, &iov_cnt, sizeof(ctrl));
 if (s != sizeof(ctrl)) {
@@ -1013,12 +1054,20 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, 
VirtQueue *vq)
 status = virtio_net_handle_mq(n, ctrl.cmd, iov, iov_cnt);
 } else if (ctrl.class == VIRTIO_NET_CTRL_GUEST_OFFLOADS) {
 status = virtio_net_handle_offloads(n, ctrl.cmd, iov, iov_cnt);
+} else if (ctrl.class == VIRTIO_NET_CTRL_STEERING_MODE) {
+size_t size_in = 0;
+status = virtio_net_ctrl_steering_mode(n, ctrl.cmd, iov, iov_cnt,
+   elem->in_sg, elem->in_num, &size_in);
+if (status == VIRTIO_NET_OK  && size_in > 0) {
+elem_in_size += size_in;
+}
 }
 
-s = iov_from_buf(elem->in_sg, elem->in_num, 0, &status, 
sizeof(status));
+s = iov_from_buf(elem->in_sg, elem->in_num, elem_in_size, &status,
+sizeof(status));
 assert(s == sizeof(status));
-
-virtqueue_push(vq, elem, sizeof(status));
+elem_in_size += s;
+virtqueue_push(vq, elem, elem_in_size);
 virtio_notify(vdev, vq);
 g_free(iov2);
 g_free(elem);
@@ -1375,10 +1424,10 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
n->guest_hdr_len, -1);
 if (out_num == VIRTQUEUE_MAX_SIZE) {
 goto drop;
-   }
+}
 out_num += 1;
 out_sg = sg2;
-   }
+  }
 }
 /*
  * If host wants to see the guest header as is, we can
@@ -1957,6 +2006,8 @@ static void virtio_net_device_realize(DeviceState *dev, 
Error **errp)
 n->host_features |= (1ULL << VIRTIO_NET_F_MTU);
 }
 
+n->host_features |= (1ULL << VIRTIO_NET_F_CTRL_STEERING_MODE);
+
 if (n->net_conf.duplex_str) {
 if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
 n->net_conf.duplex = DUPLEX_HALF;
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index a7b53edc96..809e85481c 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -102,6 +102,9 @@ typedef struct VirtIONet {
 int announce_counter;
 bool needs_vnet_hdr_swap;
 bool mtu_bypass_backend;
+struct virtio_net_steering_modes supported_modes;
+struct virtio_net_steering_modes current_mode;
+struct virtio_net_rss_conf *rss_conf;
 } V

[Qemu-devel] [RFC 1/6] Add bpf support to qemu

From: Sameeh Jubran 

This commit adds the bpf header provided by Linux to Qemu.

Signed-off-by: Sameeh Jubran 
---
 MAINTAINERS |  5 +
 configure   | 44 +
 scripts/update-linux-headers.sh |  8 ++--
 3 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0fb5f38f9f..bf2619239c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2126,6 +2126,11 @@ F: hw/rdma/*
 F: hw/rdma/vmw/*
 F: docs/pvrdma.txt
 
+BPF
+M: Sameeh Jubran 
+S: Maintained
+F: linux-headers/linux/bpf.h
+
 Build and test automation
 -
 Build and test automation
diff --git a/configure b/configure
index a8c4094c87..21edaf59aa 100755
--- a/configure
+++ b/configure
@@ -348,6 +348,7 @@ libattr=""
 xfs=""
 tcg="yes"
 membarrier=""
+bpf="no"
 vhost_net="no"
 vhost_crypto="no"
 vhost_scsi="no"
@@ -1173,6 +1174,10 @@ for opt do
   ;;
   --enable-membarrier) membarrier="yes"
   ;;
+  --disable-bpf) bpf="no"
+  ;;
+  --enable-bpf) bpf="yes"
+  ;;
   --disable-blobs) blobs="no"
   ;;
   --with-pkgversion=*) pkgversion="$optarg"
@@ -1593,6 +1598,7 @@ disabled with --disable-FEATURE, default is enabled if 
available:
   brlapi  BrlAPI (Braile)
   curlcurl connectivity
   membarrier  membarrier system call (for Linux 4.14+ or Windows)
+  bpf bpf system calls (for Linux 3.18+)
   fdt fdt device tree
   bluez   bluez stack connectivity
   kvm KVM acceleration support
@@ -5232,6 +5238,38 @@ else
 fi
 
 ##
+# check for usable bpf system call
+if test "$bpf" = "yes"; then
+have_bpf=no
+if test "$linux" = "yes" ; then
+cat > $TMPC << EOF
+#include 
+#include "linux/bpf.h"
+#include 
+#include 
+#include 
+int main(void) {
+union bpf_attr * attr = NULL;
+syscall(__NR_bpf, BPF_PROG_LOAD, attr, sizeof(attr));
+exit(0);
+}
+EOF
+bpf_include="-Iinclude/standard-headers/linux"
+bpf_cflags=""
+bpf_libs=""
+if compile_prog "$bpf_include" "$bpf_libs" ; then
+have_bpf=yes
+fi
+fi
+if test "$have_bpf" = "no"; then
+  feature_not_found "bpf" "libelf libs are not available or else \
+the bpf system call is not available"
+fi
+else
+bpf=no
+fi
+
+##
 # check if rtnetlink.h exists and is useful
 have_rtnetlink=no
 cat > $TMPC << EOF
@@ -5871,6 +5909,7 @@ echo "malloc trim support $malloc_trim"
 echo "RDMA support  $rdma"
 echo "fdt support   $fdt"
 echo "membarrier$membarrier"
+echo "bpf   $bpf"
 echo "preadv support$preadv"
 echo "fdatasync $fdatasync"
 echo "madvise   $madvise"
@@ -6365,6 +6404,11 @@ fi
 if test "$membarrier" = "yes" ; then
   echo "CONFIG_MEMBARRIER=y" >> $config_host_mak
 fi
+if test "$bpf" = "yes" ; then
+  echo "CONFIG_BPF=y" >> $config_host_mak
+  echo "LIBS_BPF=$bpf_libs" >> $config_host_mak
+  echo "CFLAGS_BPF=$bpf_cflags" >> $config_host_mak
+fi
 if test "$signalfd" = "yes" ; then
   echo "CONFIG_SIGNALFD=y" >> $config_host_mak
 fi
diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
index feb75390aa..57df8228af 100755
--- a/scripts/update-linux-headers.sh
+++ b/scripts/update-linux-headers.sh
@@ -35,6 +35,8 @@ cp_portable() {
 grep '#include' "$f" | grep -v -e 'linux/virtio' \
  -e 'linux/types' \
  -e 'stdint' \
+ -e 'stdio' \
+ -e 'stdbool' \
  -e 'linux/if_ether' \
  -e 'input-event-codes' \
  -e 'sys/' \
@@ -44,6 +46,7 @@ cp_portable() {
  -e 'linux/kernel' \
  -e 'linux/sysinfo' \
  -e 'asm-generic/kvm_para' \
+ -e 'linux/bpf' \
  > /dev/null
 then
 echo "Unexpected #include in input file $f".
@@ -58,7 +61,7 @@ cp_portable() {
 -e 's/__le\([0-9][0-9]*\)/uint\1_t/g' \
 -e 's/__be\([0-9][0-9]*\)/uint\1_t/g' \
 -e 's/"\(input-event-codes\.h\)"/"standard-headers\/linux\/\1"/' \
--e 's/]*\)>/"standard-headers\/linux\/\1"/' \
+-e 's/]*(?!bpf)\)>/"standard-headers\/linux\/\1"/' \
 -e 's/__bitwise//' \
 -e 's/__attribute__((packed))/QEMU_PACKED/' \
 -e 's/__inline__/inline/' \
@@ -126,7 +129,8 @@ done
 rm -rf "$output/linux-headers/linux"
 mkdir -p "$output/linux-headers/linux"
 for header in kvm.h vfio.h vfio_ccw.h vhost.h \
-  psci.h psp-sev.h userfaultfd.h; do
+  psci.h psp-sev.h userfaultfd.h  \
+  bpf.h; do

[Qemu-devel] [RFC 2/6] tap: Add support for bpf ioctls

From: Sameeh Jubran 

Starting from kernel v4.16 tun device supports TUNSETSTEERINGEBPF and
TUNSETFILTEREBPF.

Signed-off-by: Sameeh Jubran 
---
 include/net/net.h |  3 ++-
 net/tap-bsd.c |  5 +
 net/tap-linux.c   | 29 -
 net/tap-linux.h   |  3 ++-
 net/tap-solaris.c |  5 +
 net/tap-stub.c|  5 +
 net/tap.c |  8 
 net/tap_int.h |  1 +
 qapi/net.json | 11 +++
 9 files changed, 67 insertions(+), 3 deletions(-)

diff --git a/include/net/net.h b/include/net/net.h
index 1425960f76..e7d1baac10 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -39,7 +39,6 @@ typedef struct NICConf {
 DEFINE_PROP_MACADDR("mac",   _state, _conf.macaddr),\
 DEFINE_PROP_NETDEV("netdev", _state, _conf.peers)
 
-
 /* Net clients */
 
 typedef void (NetPoll)(NetClientState *, bool enable);
@@ -60,6 +59,7 @@ typedef int (SetVnetLE)(NetClientState *, bool);
 typedef int (SetVnetBE)(NetClientState *, bool);
 typedef struct SocketReadState SocketReadState;
 typedef void (SocketReadStateFinalize)(SocketReadState *rs);
+typedef int (SetBPFFilter)(NetClientState *, int, BPFType);
 
 typedef struct NetClientInfo {
 NetClientDriver type;
@@ -80,6 +80,7 @@ typedef struct NetClientInfo {
 SetVnetHdrLen *set_vnet_hdr_len;
 SetVnetLE *set_vnet_le;
 SetVnetBE *set_vnet_be;
+SetBPFFilter *set_bpf_filter;
 } NetClientInfo;
 
 struct NetClientState {
diff --git a/net/tap-bsd.c b/net/tap-bsd.c
index 6c9692263d..fccf17bad0 100644
--- a/net/tap-bsd.c
+++ b/net/tap-bsd.c
@@ -259,3 +259,8 @@ int tap_fd_get_ifname(int fd, char *ifname)
 {
 return -1;
 }
+
+int tap_fd_load_bpf(int fd, int bpf_fd, BPFType type)
+{
+return -1;
+}
diff --git a/net/tap-linux.c b/net/tap-linux.c
index 535b1ddb61..e8ee54f3b3 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -305,7 +305,8 @@ int tap_fd_get_ifname(int fd, char *ifname)
 {
 struct ifreq ifr;
 
-if (ioctl(fd, TUNGETIFF, &ifr) != 0) {
+if (ioctl(fd, TUNGETIFF, &ifr) != 0)
+{
 error_report("TUNGETIFF ioctl() failed: %s",
  strerror(errno));
 return -1;
@@ -314,3 +315,29 @@ int tap_fd_get_ifname(int fd, char *ifname)
 pstrcpy(ifname, sizeof(ifr.ifr_name), ifr.ifr_name);
 return 0;
 }
+
+
+int tap_fd_load_bpf(int fd, int bpf_fd, BPFType type)
+{
+int ioctl_num = 0;
+switch (type)
+{
+case BPF_TYPE_FILTER:
+ioctl_num = TUNSETFILTEREBPF;
+break;
+
+case BPF_TYPE_STEERING:
+ioctl_num = TUNSETSTEERINGEBPF;
+break;
+
+default:
+error_report("Unknown bpf_type");
+return -1;
+}
+
+if (ioctl(fd, ioctl_num, &bpf_fd) != 0) {
+error_report("#%d ioctl() failed: %s", ioctl_num, strerror(errno));
+return -1;
+}
+return 0;
+}
diff --git a/net/tap-linux.h b/net/tap-linux.h
index 2f36d100fc..7348169fc2 100644
--- a/net/tap-linux.h
+++ b/net/tap-linux.h
@@ -31,7 +31,8 @@
 #define TUNSETQUEUE  _IOW('T', 217, int)
 #define TUNSETVNETLE _IOW('T', 220, int)
 #define TUNSETVNETBE _IOW('T', 222, int)
-
+#define TUNSETSTEERINGEBPF _IOR('T', 224, int)
+#define TUNSETFILTEREBPF _IOR('T', 225, int)
 #endif
 
 /* TUNSETIFF ifr flags */
diff --git a/net/tap-solaris.c b/net/tap-solaris.c
index a2a92356c1..a5a6248c7d 100644
--- a/net/tap-solaris.c
+++ b/net/tap-solaris.c
@@ -254,3 +254,8 @@ int tap_fd_get_ifname(int fd, char *ifname)
 {
 return -1;
 }
+
+int tap_fd_load_bpf(int fd, int bpf_fd, BPFType type)
+{
+return -1;
+}
diff --git a/net/tap-stub.c b/net/tap-stub.c
index a9ab8f8293..d059a32435 100644
--- a/net/tap-stub.c
+++ b/net/tap-stub.c
@@ -85,3 +85,8 @@ int tap_fd_get_ifname(int fd, char *ifname)
 {
 return -1;
 }
+
+int tap_fd_load_bpf(int fd, int bpf_fd, BPFType type)
+{
+return -1;
+}
diff --git a/net/tap.c b/net/tap.c
index 2126f4882d..ee98fecd40 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -342,6 +342,13 @@ int tap_get_fd(NetClientState *nc)
 return s->fd;
 }
 
+static int tap_set_bpf_filter(NetClientState *nc, int bpf_fd, BPFType type)
+{
+TAPState *s = DO_UPCAST(TAPState, nc, nc);
+assert(nc->info->type == NET_CLIENT_DRIVER_TAP);
+return tap_fd_load_bpf(s->fd, bpf_fd, type);
+}
+
 /* fd support */
 
 static NetClientInfo net_tap_info = {
@@ -360,6 +367,7 @@ static NetClientInfo net_tap_info = {
 .set_vnet_hdr_len = tap_set_vnet_hdr_len,
 .set_vnet_le = tap_set_vnet_le,
 .set_vnet_be = tap_set_vnet_be,
+.set_bpf_filter = tap_set_bpf_filter,
 };
 
 static TAPState *net_tap_fd_init(NetClientState *peer,
diff --git a/net/tap_int.h b/net/tap_int.h
index 9f931d52d6..3e1603a88e 100644
--- a/net/tap_int.h
+++ b/net/tap_int.h
@@ -45,5 +45,6 @@ int tap_fd_set_vnet_be(int fd, int vnet_is_be);
 int tap_fd_enable(int fd);
 int tap_fd_disable(int fd);
 int tap_fd_get_ifname(int fd, char *ifname);
+int tap_fd_load_bpf(int fd, int bpf_fd, BPFType type);
 
 #endif /* NET_TAP_INT_H */
diff --git a/q

Re: [Qemu-devel] [PATCH 4/9] char: update the mux hanlders in class callback

Marc-André Lureau  writes:

> Instead of handling mux chardev in a special way in
> qemu_chr_fe_set_handlers(), we may use the chr_update_read_handler
> class callback instead.
>
> Signed-off-by: Marc-André Lureau 

In your subject, s/hanlders/handlers/.

Re: [Qemu-devel] [PATCH 2/9] chardev: mark the calls that allow an implicit mux monitor

Marc-André Lureau  writes:

> This is mostly for readability of the code. Let's make it clear which
> callers can create an implicit monitor when the chardev is muxed.
>
> This will also enforce a safer behaviour, as we don't really support
> creating monitor anywhere/anytime at the moment. Add an assert() to
> make sure the programmer explicitely wanted that behaviour.
>
> There are documented cases, such as: -serial/-parallel/-virtioconsole
> and to less extent -debugcon.
>
> Less obvious and questionable ones are -gdb, SLIRP -guestfwd and Xen
> console. Add a FIXME note for those, but keep the support for now.
>
> Other qemu_chr_new() callers either have a fixed parameter/filename
> string or do not need it, such as -qtest:
>
> * qtest.c: qtest_init()
>   Afaik, only used by tests/libqtest.c, without mux. I don't think we
>   support it outside of qemu testing: drop support for implicit mux
>   monitor (qemu_chr_new() call: no implicit mux now).
>
> * hw/
>   All with literal @filename argument that doesn't enable mux monitor.
>
> * tests/
>   All with @filename argument that doesn't enable mux monitor.
>
> On a related note, the list of monitor creation places:
>
> - the chardev creators listed above: all from command line (except
>   perhaps Xen console?)
>
> - -gdb & hmp gdbserver will create a "GDB monitor command" chardev
>   that is wired to an HMP monitor.
>
> - -mon command line option
>
> From this short study, I would like to think that a monitor may only
> be created in the main thread today, though I remain skeptical :)
>
> Signed-off-by: Marc-André Lureau 
> ---
>  include/chardev/char.h | 24 
>  chardev/char.c | 32 +---
>  gdbstub.c  |  6 +-
>  hw/char/xen_console.c  |  5 -
>  net/slirp.c|  5 -
>  vl.c   | 10 +-
>  6 files changed, 63 insertions(+), 19 deletions(-)
>
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 3e4fe6dad0..268daaa90b 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -105,14 +105,27 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
>   * @filename: the URI
>   *
>   * Create a new character backend from a URI.
> + * Do not implicitly initialize a monitor if the chardev is muxed.
>   *
>   * Returns: a new character backend
>   */
>  Chardev *qemu_chr_new(const char *label, const char *filename);
>  
>  /**
> - * qemu_chr_change:
> - * @opts: the new backend options
> + * qemu_chr_new_mux_mon:
> + * @label: the name of the backend
> + * @filename: the URI
> + *
> + * Create a new character backend from a URI.
> + * Implicitly initialize a monitor if the chardev is muxed.
> + *
> + * Returns: a new character backend
> + */
> +Chardev *qemu_chr_new_mux_mon(const char *label, const char *filename);
> +
> +/**
> +* qemu_chr_change:
> +* @opts: the new backend options
>   *
>   * Change an existing character backend
>   */
> @@ -129,6 +142,7 @@ void qemu_chr_cleanup(void);
>   * qemu_chr_new_noreplay:
>   * @label: the name of the backend
>   * @filename: the URI
> + * @with_mux_mon: if chardev is muxed, initialize a monitor
>   *
>   * Create a new character backend from a URI.
>   * Character device communications are not written
> @@ -136,7 +150,8 @@ void qemu_chr_cleanup(void);
>   *
>   * Returns: a new character backend
>   */
> -Chardev *qemu_chr_new_noreplay(const char *label, const char *filename);
> +Chardev *qemu_chr_new_noreplay(const char *label, const char *filename,
> +   bool with_mux_mon);
>  
>  /**
>   * qemu_chr_be_can_write:
> @@ -194,7 +209,8 @@ bool qemu_chr_has_feature(Chardev *chr,
>ChardevFeature feature);
>  void qemu_chr_set_feature(Chardev *chr,
>ChardevFeature feature);
> -QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
> +QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
> +bool with_mux_mon);
>  int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all);
>  #define qemu_chr_write_all(s, buf, len) qemu_chr_write(s, buf, len, true)
>  int qemu_chr_wait_connected(Chardev *chr, Error **errp);
> diff --git a/chardev/char.c b/chardev/char.c
> index 76d866e6fe..c1b89b6638 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -329,7 +329,8 @@ int qemu_chr_wait_connected(Chardev *chr, Error **errp)
>  return 0;
>  }
>  
> -QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
> +QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
> +bool with_mux_mon)
>  {
>  char host[65], port[33], width[8], height[8];
>  int pos;
> @@ -344,6 +345,10 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const 
> char *filename)
>  }
>  
>  if (strstart(filename, "mon:", &p)) {
> +if (!with_mux_mon) {
> +erro

Re: [Qemu-devel] [PATCH 5/9] char-fe: set_handlers() needs an associted chardev

Marc-André Lureau  writes:

> It is futile to call qemu_chr_fe_set_handlers() without an associated
> chardev, because the function is doing nothing in that case, not even
> reporting an error, it would likely be a programming error. Let's not
> handle that hypothetical case.
>
> (fwiw, I introduced the check in commit
> 94a40fc56036b5058b0b194d9e372a22e65ce7be, that was a mistake imho)
>
> Signed-off-by: Marc-André Lureau 

In your subject, s/associted/associated/.

[Qemu-devel] [PATCH v2 2/2] qemu-error: make use of {error, warn}_report_once_cond

2018-08-30 Thread Cornelia Huck

{error,warn}_report_once() are a special case of the new functions
and can simply switch to them.

Signed-off-by: Cornelia Huck 
---
 include/qemu/error-report.h | 34 ++
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index e415128ac4..918cb936d8 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -53,32 +53,26 @@ bool warn_report_once_cond(bool *printed, const char *fmt, 
...)
  * Similar to error_report(), except it prints the message just once.
  * Return true when it prints, false otherwise.
  */
-#define error_report_once(fmt, ...) \
-({  \
-static bool print_once_;\
-bool ret_print_once_ = !print_once_;\
-\
-if (!print_once_) { \
-print_once_ = true; \
-error_report(fmt, ##__VA_ARGS__);   \
-}   \
-unlikely(ret_print_once_);  \
+#define error_report_once(fmt, ...) \
+({  \
+static bool print_once_;\
+bool ret_print_once_ =  \
+error_report_once_cond(&print_once_,\
+   fmt, ##__VA_ARGS__); \
+unlikely(ret_print_once_);  \
 })
 
 /*
  * Similar to warn_report(), except it prints the message just once.
  * Return true when it prints, false otherwise.
  */
-#define warn_report_once(fmt, ...)  \
-({  \
-static bool print_once_;\
-bool ret_print_once_ = !print_once_;\
-\
-if (!print_once_) { \
-print_once_ = true; \
-warn_report(fmt, ##__VA_ARGS__);\
-}   \
-unlikely(ret_print_once_);  \
+#define warn_report_once(fmt, ...)  \
+({  \
+static bool print_once_;\
+bool ret_print_once_ =  \
+warn_report_once_cond(&print_once_, \
+  fmt, ##__VA_ARGS__);  \
+unlikely(ret_print_once_);  \
 })
 
 const char *error_get_progname(void);
-- 
2.14.4

[Qemu-devel] [PATCH v2 1/2] qemu-error: add {error, warn}_report_once_cond

2018-08-30 Thread Cornelia Huck

Add two functions to print an error/warning report once depending
on a passed-in condition variable and flip it if printed. This is
useful if you want to print a message not once-globally, but e.g.
once-per-device.

Inspired by warn_once() in hw/vfio/ccw.c, which has been replaced
with warn_report_once_cond().

Signed-off-by: Cornelia Huck 
---
 hw/vfio/ccw.c   | 18 +++--
 include/qemu/error-report.h |  5 +
 util/qemu-error.c   | 48 +
 3 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index e96bbdc78b..9246729a75 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -37,24 +37,12 @@ typedef struct VFIOCCWDevice {
 bool warned_orb_pfch;
 } VFIOCCWDevice;
 
-static inline void warn_once(bool *warned, const char *fmt, ...)
-{
-va_list ap;
-
-if (!warned || *warned) {
-return;
-}
-*warned = true;
-va_start(ap, fmt);
-warn_vreport(fmt, ap);
-va_end(ap);
-}
-
 static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch,
   const char *msg)
 {
-warn_once(&vcdev->warned_orb_pfch, "vfio-ccw (devno %x.%x.%04x): %s",
-  sch->cssid, sch->ssid, sch->devno, msg);
+warn_report_once_cond(&vcdev->warned_orb_pfch,
+  "vfio-ccw (devno %x.%x.%04x): %s",
+  sch->cssid, sch->ssid, sch->devno, msg);
 }
 
 static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 72fab2b031..e415128ac4 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -44,6 +44,11 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void warn_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void info_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 
+bool error_report_once_cond(bool *printed, const char *fmt, ...)
+GCC_FMT_ATTR(2, 3);
+bool warn_report_once_cond(bool *printed, const char *fmt, ...)
+GCC_FMT_ATTR(2, 3);
+
 /*
  * Similar to error_report(), except it prints the message just once.
  * Return true when it prints, false otherwise.
diff --git a/util/qemu-error.c b/util/qemu-error.c
index a25d3b94c6..b77e0bac4c 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -310,3 +310,51 @@ void info_report(const char *fmt, ...)
 vreport(REPORT_TYPE_INFO, fmt, ap);
 va_end(ap);
 }
+
+/*
+ * If *printed is false, print an error message to current monitor if we
+ * have one, else to stderr, and flip *printed to true.
+ * Returns false if message was not printed, else true.
+ * Format arguments like sprintf().  The resulting message should be
+ * a single phrase, with no newline or trailing punctuation.
+ * Prepend the current location and append a newline.
+ * It's wrong to call this in a QMP monitor.  Use error_setg() there.
+ */
+bool error_report_once_cond(bool *printed, const char *fmt, ...)
+{
+va_list ap;
+
+assert(printed);
+if (*printed) {
+return false;
+}
+*printed = true;
+va_start(ap, fmt);
+vreport(REPORT_TYPE_ERROR, fmt, ap);
+va_end(ap);
+return true;
+}
+
+/*
+ * If *printed is false, print a warning message to current monitor if we
+ * have one, else to stderr, and flip *printed to true.
+ * Returns false if message was not printed, else true.
+ * Format arguments like sprintf().  The resulting message should be
+ * a single phrase, with no newline or trailing punctuation.
+ * Prepend the current location and append a newline.
+ * It's wrong to call this in a QMP monitor.  Use error_setg() there.
+ */
+bool warn_report_once_cond(bool *printed, const char *fmt, ...)
+{
+va_list ap;
+
+assert(printed);
+if (*printed) {
+return false;
+}
+*printed = true;
+va_start(ap, fmt);
+vreport(REPORT_TYPE_WARNING, fmt, ap);
+va_end(ap);
+return true;
+}
-- 
2.14.4

[Qemu-devel] [PATCH v2 0/2] qemu-error: advanced report_once handling

2018-08-30 Thread Cornelia Huck

[Markus: I've decided to not include your R-b, as I did too many changes
to feel comfortable with that.]

[Also note that I'm about to disappear on vacation, so don't expect
quick responses. I just want to get it out before I forget about it.]

Based on previous discussions, I wanted to enhance the recently
merged report_once infrastucture with a way to print a message
once based on a variable instead of globally-once, similar to
what vfio-ccw uses today.

Not really tested, mainly wanted to get this out before my vacation
to get the ball rolling.

v1->v2:
- merged patch 3 (use the new function in vfio-ccw) into patch 1
- require a non-NULL 'printed' parameter
- have the functions return whether something was printed and slightly
  simplify the existing macros
- adapt macro formatting to suit my aesthetic sensibilities

Cornelia Huck (2):
  qemu-error: add {error,warn}_report_once_cond
  qemu-error: make use of {error,warn}_report_once_cond

 hw/vfio/ccw.c   | 18 +++--
 include/qemu/error-report.h | 39 ++--
 util/qemu-error.c   | 48 +
 3 files changed, 70 insertions(+), 35 deletions(-)

-- 
2.14.4

Re: [Qemu-devel] [RFC 2/6] tap: Add support for bpf ioctls

2018-08-30 Thread Eric Blake


On 08/30/2018 09:27 AM, Sameeh Jubran wrote:

From: Sameeh Jubran 

Starting from kernel v4.16 tun device supports TUNSETSTEERINGEBPF and
TUNSETFILTEREBPF.

Signed-off-by: Sameeh Jubran 
---



+++ b/qapi/net.json
@@ -692,3 +692,14 @@
  ##
  { 'event': 'NIC_RX_FILTER_CHANGED',
'data': { '*name': 'str', 'path': 'str' } }
+
+##
+# @BPFType:
+#
+# BPF programs types provided as an argument for tap bpf ioctls
+#
+# Since: 2.12


You missed 2.12 by a long shot; this should be 3.1.


+#
+##
+{ 'enum': 'BPFType',
+  'data': [ 'filter', 'steering' ] }


It might also be wise to document these two values in addition to the 
documentation of the enum as a whole.


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

Re: [Qemu-devel] [PATCH v4 06/10] tests: add qmp/object-add-without-props test

Hi
On Thu, Aug 30, 2018 at 3:01 PM Markus Armbruster  wrote:
>
> Marc-André Lureau  writes:
>
> > test_object_add_without_props() tests a bug in qmp_object_add() we
> > fixed in commit e64c75a975.  Sadly, we don't have systematic
> > object-add tests.  This lone test can go into qmp-cmd-test for want of
> > a better home.
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  tests/qmp-cmd-test.c | 31 +++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/tests/qmp-cmd-test.c b/tests/qmp-cmd-test.c
> > index c5b70df974..3ba8f68476 100644
> > --- a/tests/qmp-cmd-test.c
> > +++ b/tests/qmp-cmd-test.c
> > @@ -19,6 +19,15 @@
> >
> >  const char common_args[] = "-nodefaults -machine none";
> >
> > +static const char *get_error_class(QDict *resp)
> > +{
> > +QDict *error = qdict_get_qdict(resp, "error");
> > +const char *desc = qdict_get_try_str(error, "desc");
> > +
> > +g_assert(desc);
> > +return error ? qdict_get_try_str(error, "class") : NULL;
> > +}
> > +
> >  /* Query smoke tests */
> >
> >  static int query_error_class(const char *cmd)
>
> Copied from qmp-test.c.  It should be factored out instead.  Where to
> put it?  libqtest.c isn't quite right, as the function could
> theoretically be useful in unit tests as well, but I guess it would do
> for now.
>
> Asserting presence of "desc" makes little sense outside qmp-test.c
> protocol tests, but it doesn't hurt, either.
>
> Grep shows more possible users in tests/drive_del-test.c and
> tests/test-qga.c.

ok

>
> > @@ -197,6 +206,24 @@ static void add_query_tests(QmpSchema *schema)
> >  }
> >  }
> >
> > +static void test_object_add_without_props(void)
> > +{
> > +QTestState *qts;
> > +QDict *ret;
>
> qmp-test.c and qmp-cmd-test.c commonly use @resp for the response.

ok

>
> > +
> > +qts = qtest_init(common_args);
> > +
> > +ret = qtest_qmp(qts,
> > +"{'execute': 'object-add', 'arguments':"
> > +" {'qom-type': 'memory-backend-ram', 'id': 'ram1' } 
> > }");
> > +g_assert_nonnull(ret);
>
> What's wrong with g_assert(!ret)?

nothing wrong, but g_assert_nonnull is slightly more readable, both in
code and in error produced.

>
> > +
> > +g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
> > +
> > +qobject_unref(ret);
>
> Permit me to digress.
>
> When you expect success, you check @resp like this:
>
> ret = qdict_get_qdict(resp, "return");
> ... laborously check @ret against expectations ...
>
> If you feel pedantically thorough, you can throw in
>
> g_assert(!qdict_haskey(resp, "error");
>
> When you expect failure, you check like this:
>
> error = qdict_get_qdict(resp, "error");
> class = qdict_get_try_str(error, "class");
> g_assert_cmpstr(class, ==, "GenericError");
>
> and perhaps
>
> g_assert(!qdict_haskey(resp, "return");
>
> get_error_class() saves a little typing in the failure case.  It's still
> an awfully verbose overall, and the checking is full of holes more often
> than not.  There's got to be a better way.
>

what about?
/**
 * qmp_assert_error_class:
 * @rsp: QMP response to check for error
 * @class: an error class
 *
 * Assert the response has the given error class and discard @rsp.
 */
void qmp_assert_error_class(QDict *rsp, const char *class)
{
QDict *error = qdict_get_qdict(rsp, "error");

g_assert_cmpstr(qdict_get_try_str(error, "class"), ==, class);
g_assert_nonnull(qdict_get_try_str(error, "desc"));
g_assert_null(qdict_get(error, "return"));

qobject_unref(rsp);
}


> > +qtest_quit(qts);
> > +}
> > +
> >  int main(int argc, char *argv[])
> >  {
> >  QmpSchema schema;
> > @@ -206,6 +233,10 @@ int main(int argc, char *argv[])
> >
> >  qmp_schema_init(&schema);
> >  add_query_tests(&schema);
> > +
> > +qtest_add_func("qmp/object-add-without-props",
> > +   test_object_add_without_props);
> > +
> >  ret = g_test_run();
> >
> >  qmp_schema_cleanup(&schema);
>
> May I have a TODO comment asking for coverage of generic object-add
> failure modes?

You mean checking for other kind of failures? ok


-- 
Marc-André Lureau

[Qemu-devel] [PATCH 8/8] qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()

The commit for 0e4e4318eaa5 increments QCOW2_OL_MAX_BITNR but does not
add an array entry for QCOW2_OL_BITMAP_DIRECTORY_BITNR to metadata_ol_names[].
As a result, an array dereference of metadata_ol_names[8] in
qcow2_pre_write_overlap_check() could result in a read outside of the array 
bounds.

Fixes: 0e4e4318eaa5 ('qcow2: add overlap check for bitmap directory')

Cc: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Liam Merwick 
Reviewed-by: Darren Kenny 
Reviewed-by: Mark Kanda 
---
 block/qcow2-refcount.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3c539f02e5ec..6504e7421324 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2719,14 +2719,15 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, 
int ign, int64_t offset,
 }
 
 static const char *metadata_ol_names[] = {
-[QCOW2_OL_MAIN_HEADER_BITNR]= "qcow2_header",
-[QCOW2_OL_ACTIVE_L1_BITNR]  = "active L1 table",
-[QCOW2_OL_ACTIVE_L2_BITNR]  = "active L2 table",
-[QCOW2_OL_REFCOUNT_TABLE_BITNR] = "refcount table",
-[QCOW2_OL_REFCOUNT_BLOCK_BITNR] = "refcount block",
-[QCOW2_OL_SNAPSHOT_TABLE_BITNR] = "snapshot table",
-[QCOW2_OL_INACTIVE_L1_BITNR]= "inactive L1 table",
-[QCOW2_OL_INACTIVE_L2_BITNR]= "inactive L2 table",
+[QCOW2_OL_MAIN_HEADER_BITNR]= "qcow2_header",
+[QCOW2_OL_ACTIVE_L1_BITNR]  = "active L1 table",
+[QCOW2_OL_ACTIVE_L2_BITNR]  = "active L2 table",
+[QCOW2_OL_REFCOUNT_TABLE_BITNR] = "refcount table",
+[QCOW2_OL_REFCOUNT_BLOCK_BITNR] = "refcount block",
+[QCOW2_OL_SNAPSHOT_TABLE_BITNR] = "snapshot table",
+[QCOW2_OL_INACTIVE_L1_BITNR]= "inactive L1 table",
+[QCOW2_OL_INACTIVE_L2_BITNR]= "inactive L2 table",
+[QCOW2_OL_BITMAP_DIRECTORY_BITNR]   = "bitmap directory",
 };
 
 /*
-- 
1.8.3.1

[Qemu-devel] [PATCH 0/8] off-by-one and NULL pointer accesses detected by static analysis

Below are a number of fixes to some off-by-one, read outside array bounds, and
NULL pointer accesses detected by an internal Oracle static analysis tool 
(Parfait).
https://labs.oracle.com/pls/apex/f?p=labs:49:P49_PROJECT_ID:13

I have also included a patch to add a command-line option to configure to
select if AVX2 is used or not (keeping the existing behaviour by default).
My motivation was avoiding an issue with the static analysis tool but NetSpectre
was announced as I was working on this and I felt it may have more general uses.


Liam Merwick (8):
  configure: Provide option to explicitly disable AVX2
  job: Fix off-by-one accesses to JobSTT and JobVerbTable
  block: Null pointer dereference in blk_root_get_parent_desc()
  qemu-img: potential Null pointer deref in img_commit()
  block: Fix potential Null pointer dereferences in vvfat.c
  block: dump_qlist() may dereference a Null pointer
  io: file descriptor not initialized in qio_channel_command_new_spawn()
  qcow2: Read outside array bounds in qcow2_pre_write_overlap_check()

 block/block-backend.c|  2 +-
 block/qcow2-refcount.c   | 17 ---
 block/vvfat.c| 56 
 configure| 11 --
 include/qapi/qmp/qlist.h |  6 ++
 io/channel-command.c |  4 ++--
 job.c|  4 ++--
 qemu-img.c   |  3 +++
 8 files changed, 88 insertions(+), 15 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 7/8] io: file descriptor not initialized in qio_channel_command_new_spawn()

Incorrect checking of flags could result in uninitialized
file descriptor being used.

Signed-off-by: Liam Merwick 
Reviewed-by: Darren Kenny 
Reviewed-by: Mark Kanda 
---
 io/channel-command.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/io/channel-command.c b/io/channel-command.c
index 3e7eb17eff54..38deb687da21 100644
--- a/io/channel-command.c
+++ b/io/channel-command.c
@@ -59,10 +59,10 @@ qio_channel_command_new_spawn(const char *const argv[],
 
 flags = flags & O_ACCMODE;
 
-if (flags == O_RDONLY) {
+if ((flags & O_RDONLY) == O_RDONLY) {
 stdinnull = true;
 }
-if (flags == O_WRONLY) {
+if ((flags & O_WRONLY) == O_WRONLY) {
 stdoutnull = true;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/8] block: dump_qlist() may dereference a Null pointer

A NULL 'list' passed into function dump_qlist() isn't correctly
validated and can be passed to qlist_first() where it is dereferenced.

This could be resolved by checking if the list is NULL in dump_qlist()
and returning immediately. However, the general case can be handled by
adding a NULL arg check to to qlist_first() and qlist_next() and all
the callers to those functions handle that cleanly.

Signed-off-by: Liam Merwick 
Reviewed-by: Darren Kenny  
Reviewed-by: Mark Kanda 

---
 include/qapi/qmp/qlist.h | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
index 8d2c32ca2863..1ec716e2eb9e 100644
--- a/include/qapi/qmp/qlist.h
+++ b/include/qapi/qmp/qlist.h
@@ -58,11 +58,17 @@ void qlist_destroy_obj(QObject *obj);
 
 static inline const QListEntry *qlist_first(const QList *qlist)
 {
+if (!qlist) {
+return NULL;
+}
 return QTAILQ_FIRST(&qlist->head);
 }
 
 static inline const QListEntry *qlist_next(const QListEntry *entry)
 {
+if (!entry) {
+return NULL;
+}
 return QTAILQ_NEXT(entry, next);
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/8] block: Null pointer dereference in blk_root_get_parent_desc()

The dev_id returned by the call to blk_get_attached_dev_id() in
blk_root_get_parent_desc() can be NULL (an internal call to
object_get_canonical_path may have returned NULL) so it should
be checked before dereferencing.

Signed-off-by: Liam Merwick 
Reviewed-by: Darren Kenny 
Reviewed-by: Mark Kanda 
---
 block/block-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index fa120630be83..210eee75006a 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -136,7 +136,7 @@ static char *blk_root_get_parent_desc(BdrvChild *child)
 }
 
 dev_id = blk_get_attached_dev_id(blk);
-if (*dev_id) {
+if (dev_id && *dev_id) {
 return dev_id;
 } else {
 /* TODO Callback into the BB owner for something more detailed */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/5] nvme: PCI/e configuration from specification

2018-08-30 Thread Daniel Verkamp

Hi Shimi,

On Sun, Aug 26, 2018 at 2:50 PM Gersner  wrote:
>
> Hi Daniel,
> Thanks for taking a look. Comments are inline.
>
> Gersner.
>
> On Sun, Jul 15, 2018 at 9:21 AM Daniel Verkamp  wrote:
>>
>> On Fri, Jun 22, 2018 at 4:22 AM, Shimi Gersner  wrote:
>> > PCI/e configuration currently does not meets specifications.
>> >
>> > Patch includes various configuration changes to support specifications
>> > - BAR2 to return zero when read and CMD.IOSE is not set.
>> > - Expose NVME configuration through IO space (Optional).
>> > - PCI Power Management v1.2.
>> > - PCIe Function Level Reset.
>> > - Disable QEMUs default use of PCIe Link Status (DLLLA).
>> > - PCIe missing AOC compliance flag.
>> > - Mask PCIe End-to-End TLP as RO (Unspecified by specification).
>> [...]
>> >  n->num_namespaces = 1;
>> >  n->num_queues = 64;
>> > -n->reg_size = pow2ceil(0x1004 + 2 * (n->num_queues + 1) * 4);
>> > +n->reg_size = pow2ceil(0x2000 + 2 * (n->num_queues + 1) * 4);
>>
>> The existing math looks OK to me (maybe already 4 bytes larger than
>> necessary?). The controller registers should be 0x1000 bytes for the
>> fixed register set, then 2 * 4 bytes for each doorbell pair (+ 1 is
>> for the admin queue, and CAP.DSTRD is set to 0, so there is no extra
>> padding between queues). The result is also rounded up to a power of
>> two, so the result will typically be 8 KB.  What is the rationale for
>> this change?
>
> You are correct, this change shouldn't be here. It was made during tests 
> against the
> nvme compliance which failed here.

If the NVMe compliance test requires it, that is probably sufficient
reason to change it - although it would be interesting to hear their
rationale.

> The specification states that bits 0 to 13 are RO, which implicitly suggests
> that the minimal size of this BAR should be at least 16K as this is a standard
> way to determine the BAR size on PCI (AFAIK). On the contrary it doesn't
> fit very well with the memory mapped laid out on 3.1 Register Definition
> of the 1.3b nvme spec. Any idea?

You're right, the MLBAR section of the NVMe spec does seem to indicate
that up to bit 13 should be reserved/RO.  This is also probably a good
enough reason to make the change, as long as this reason is provided.

>
> Additionally it does look like it has an extra 4 bytes.
>
>>
>>
>> >  n->ns_size = bs_size / (uint64_t)n->num_namespaces;
>> >
>> >  n->namespaces = g_new0(NvmeNamespace, n->num_namespaces);
>> > @@ -1245,6 +1295,10 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
>> > **errp)
>> >  pci_register_bar(&n->parent_obj, 0,
>> >  PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
>> >  &n->iomem);
>> > +
>> > +// Expose the NVME memory through Address Space IO (Optional by spec)
>> > +pci_register_bar(&n->parent_obj, 2, PCI_BASE_ADDRESS_SPACE_IO, 
>> > &n->iomem);
>>
>> This looks like it will register the whole 4KB+ NVMe register set
>> (n->iomem) as an I/O space BAR, but this doesn't match the spec (see
>> NVMe 1.3c section 3.2, "Index/Data Pair registers"). The I/O BAR (if
>> implemented) should just contain two 4-byte registers, Index and Data,
>> that can be used to indirectly access the NVMe register set.  (Just
>> for curiosity, do you know of any software that uses this feature? It
>> could be useful for testing the implementation.)
>
> Very nice catch. We indeed totally miss-interpreted the specification here.
>
> We use the compliance suit for sanity, but it doesn't actually use this 
> feature,
> just verifies the configuration of the registers.
>
> We will go with rolling back this feature as this is optional.

This is probably the right move; I don't know of any real hardware
devices that implement it (and therefore I doubt any software will
require it).  However, it should not be too difficult to add an
implementation of this feature; if you are interested, take a look at
the ahci_idp_read/ahci_idp_write functions - the NVMe index/data pair
should be very similar.

> Question - I'm having hard time to determine from the specification - As
> this is optional, how device drivers determine if it is available? Does it
> simply the CMD.IOSE flag from the PCI? And if so, what is
> the standard way in QEMU to disable the flag completely?

Based on the wording in NVMe 1.3 section 3.2, it seems like the only
indication of support for Index/Data Pair is whether BAR2 is filled
out.  I believe PCI defines unused BARs to be all 0s, so software
should be able to use this to determine whether the feature is
provided by a particular NVMe PCIe device, and removing the
pci_register_bar() call should be enough to accomplish this from the
QEMU side.

Thanks,
-- Daniel

[Qemu-devel] [PATCH 2/8] job: Fix off-by-one accesses to JobSTT and JobVerbTable

In the array dereference of JobVerbTable[verb] in job_apply_verb()
the check of the index, verb, allows an overrun because an index
equal to the array size is permitted.

Similarly, in the array dereference of JobSTT[s0][s1] with index s1
in job_state_transition(), an off-by-one overrun is possible.

Signed-off-by: Liam Merwick 
Reviewed-by: Darren Kenny 
Reviewed-by: Mark Kanda 
---
 job.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/job.c b/job.c
index e36ebaafd81c..40320566f43b 100644
--- a/job.c
+++ b/job.c
@@ -166,7 +166,7 @@ bool job_is_internal(Job *job)
 static void job_state_transition(Job *job, JobStatus s1)
 {
 JobStatus s0 = job->status;
-assert(s1 >= 0 && s1 <= JOB_STATUS__MAX);
+assert(s1 >= 0 && s1 < JOB_STATUS__MAX);
 trace_job_state_transition(job, job->ret,
JobSTT[s0][s1] ? "allowed" : "disallowed",
JobStatus_str(s0), JobStatus_str(s1));
@@ -181,7 +181,7 @@ static void job_state_transition(Job *job, JobStatus s1)
 int job_apply_verb(Job *job, JobVerb verb, Error **errp)
 {
 JobStatus s0 = job->status;
-assert(verb >= 0 && verb <= JOB_VERB__MAX);
+assert(verb >= 0 && verb < JOB_VERB__MAX);
 trace_job_apply_verb(job, JobStatus_str(s0), JobVerb_str(verb),
  JobVerbTable[verb][s0] ? "allowed" : "prohibited");
 if (JobVerbTable[verb][s0]) {
-- 
1.8.3.1

[Qemu-devel] [Bug 1788665] Re: Low 2D graphics performance with Windows 10 (1803) VGA passthrough VM using "Spectre" protection

2018-08-30 Thread Heiko Sieger

>The possibilities left are that either your Windows guest is lacking
software updates that could perhaps improve its performance, or that 2D
graphics really is that awful in combination with spectre/meltdown
fixes.

Thanks Daniel. There are two problems with this explanation:

1. A native "bare metal" Windows 10 (1803) installation with all updates
applied does NOT have any 2D performance problems. See my attachment
(benchmarks) in the original post.

2. Both the Windows VM and the Linux VM do not see the microcode (version?),
and therefore do not know about the Spectre vulnerability mitigation in the
host kernel. However, the Microsoft document
https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/CVE-2017-5715-and-hyper-v-vms
suggests that Microsoft Hyper-V can be configured to expose new processor
capabilities to guest virtual machines, specifically the ones made available
through the microcode updates. Here a quote from the above Microsoft website:
"Firmware updates from your OEM may contain new processor capabilities that can
be used to protect against CVE-2017-5715 (IBRS, STIBP, IBPB). Once the
virtualization host's firmware has been updated, the hypervisor can make these
additional capabilities available to guest virtual machines after taking the
following steps."

The ideal behavior of qemu/kvm would be to expose the microcode
capabilities to the guest (I suppose this happens partially as seen by
the presence of the "pti ssbd ibrs ibpb" flags), but obviously
something is missing.

But the real important question is this:
In the above scenario, are the VMs protected from Spectre vulnerabilities,
simply by having the microcode installed in the host? Even when I disable the
Spectre protection inside the Windows VM?

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

Title:
Low 2D graphics performance with Windows 10 (1803) VGA passthrough VM
using "Spectre" protection

Status in QEMU:
New

Bug description:
Windows 10 (1803) VM using VGA passthrough via qemu script.

After upgrading Windows 10 Pro VM to version 1803, or possibly after
applying the March/April security updates from Microsoft, the VM would
show low 2D graphics performance (sluggishness in 2D applications and
low Passmark results).

Turning off Spectre vulnerability protection in Windows remedies the
issue.

Expected behavior:
qemu/kvm hypervisor to expose firmware capabilities of host to guest OS - see
https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/CVE-2017-5715-and-hyper-v-vms

Background:

Starting in March or April Microsoft began to push driver updates in
their updates / security updates. See https://support.microsoft.com
/en-us/help/4073757/protect-your-windows-devices-against-spectre-
meltdown

One update concerns the Intel microcode - see
https://support.microsoft.com/en-us/help/4100347. It is activated by
default within Windows.

Once the updates are applied within the Windows guest, 2D graphics
performance drops significantly. Other performance benchmarks are not
affected.

A bare metal Windows installation does not display a performance loss
after the update. See https://heiko-sieger.info/low-2d-graphics-
benchmark-with-windows-10-1803-kvm-vm/

Similar reports can be found here:

https://www.reddit.com/r/VFIO/comments/97unx4/passmark_lousy_2d_graphics_performance_on_windows/

Hardware:

6 core Intel Core i7-3930K (-MT-MCP-)

Host OS:
Linux Mint 19/Ubuntu 18.04
Kernel: 4.15.0-32-generic x86_64
Qemu: QEMU emulator version 2.11.1
Intel microcode (host): 0x714
dmesg | grep microcode
[0.00] microcode: microcode updated early to revision 0x714, date =
2018-05-08
[2.810683] microcode: sig=0x206d7, pf=0x4, revision=0x714
[2.813340] microcode: Microcode Update Driver: v2.2.

Note: I manually updated the Intel microcode on the host from 0x713 to
0x714. However, both microcode versions produce the same result in the
Windows guest.

Guest OS:
Windows 10 Pro 64 bit, release 1803

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

[Qemu-devel] [PATCH 5/8] block: Fix potential Null pointer dereferences in vvfat.c

The calls to bdrv_new_open_driver(), find_mapping_for_cluster(),
and array_get_next() may return NULL but it isn't always checked for
before dereferencing the value returned.

Signed-off-by: Liam Merwick 
Reviewed-by: Darren Kenny 
Reviewed-by: Mark Kanda 
---
 block/vvfat.c | 56 
 1 file changed, 56 insertions(+)

diff --git a/block/vvfat.c b/block/vvfat.c
index fc41841a5c3c..0f1f10a2f94b 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -448,6 +448,9 @@ static direntry_t *create_long_filename(BDRVVVFATState *s, 
const char *filename)
 
 for(i=0;idirectory));
+if (entry == NULL) {
+continue;
+}
 entry->attributes=0xf;
 entry->reserved[0]=0;
 entry->begin=0;
@@ -665,6 +668,9 @@ static inline void fat_set(BDRVVVFATState* s,unsigned int 
cluster,uint32_t value
 } else {
 int offset = (cluster*3/2);
 unsigned char* p = array_get(&(s->fat), offset);
+if (p == NULL) {
+return;
+}
 switch (cluster&1) {
 case 0:
 p[0] = value&0xff;
@@ -730,6 +736,9 @@ static inline direntry_t* 
create_short_and_long_name(BDRVVVFATState* s,
 
 if(is_dot) {
 entry=array_get_next(&(s->directory));
+if (entry == NULL) {
+return NULL;
+}
 memset(entry->name, 0x20, sizeof(entry->name));
 memcpy(entry->name,filename,strlen(filename));
 return entry;
@@ -844,6 +853,12 @@ static int read_directory(BDRVVVFATState* s, int 
mapping_index)
 /* create mapping for this file */
 if(!is_dot && !is_dotdot && (S_ISDIR(st.st_mode) || st.st_size)) {
 s->current_mapping = array_get_next(&(s->mapping));
+if (s->current_mapping == NULL) {
+fprintf(stderr, "Failed to create mapping for file\n");
+g_free(buffer);
+closedir(dir);
+return -2;
+}
 s->current_mapping->begin=0;
 s->current_mapping->end=st.st_size;
 /*
@@ -941,6 +956,9 @@ static int init_directories(BDRVVVFATState* s,
 /* add volume label */
 {
 direntry_t* entry=array_get_next(&(s->directory));
+if (entry == NULL) {
+return -1;
+}
 entry->attributes=0x28; /* archive | volume label */
 memcpy(entry->name, s->volume_label, sizeof(entry->name));
 }
@@ -953,6 +971,9 @@ static int init_directories(BDRVVVFATState* s,
 s->cluster_count=sector2cluster(s, s->sector_count);
 
 mapping = array_get_next(&(s->mapping));
+if (mapping == NULL) {
+return -1;
+}
 mapping->begin = 0;
 mapping->dir_index = 0;
 mapping->info.dir.parent_mapping_index = -1;
@@ -1630,6 +1651,9 @@ static void schedule_rename(BDRVVVFATState* s,
 uint32_t cluster, char* new_path)
 {
 commit_t* commit = array_get_next(&(s->commits));
+if (commit == NULL) {
+return;
+}
 commit->path = new_path;
 commit->param.rename.cluster = cluster;
 commit->action = ACTION_RENAME;
@@ -1639,6 +1663,9 @@ static void schedule_writeout(BDRVVVFATState* s,
 int dir_index, uint32_t modified_offset)
 {
 commit_t* commit = array_get_next(&(s->commits));
+if (commit == NULL) {
+return;
+}
 commit->path = NULL;
 commit->param.writeout.dir_index = dir_index;
 commit->param.writeout.modified_offset = modified_offset;
@@ -1649,6 +1676,9 @@ static void schedule_new_file(BDRVVVFATState* s,
 char* path, uint32_t first_cluster)
 {
 commit_t* commit = array_get_next(&(s->commits));
+if (commit == NULL) {
+return;
+}
 commit->path = path;
 commit->param.new_file.first_cluster = first_cluster;
 commit->action = ACTION_NEW_FILE;
@@ -1657,6 +1687,9 @@ static void schedule_new_file(BDRVVVFATState* s,
 static void schedule_mkdir(BDRVVVFATState* s, uint32_t cluster, char* path)
 {
 commit_t* commit = array_get_next(&(s->commits));
+if (commit == NULL) {
+return;
+}
 commit->path = path;
 commit->param.mkdir.cluster = cluster;
 commit->action = ACTION_MKDIR;
@@ -2261,6 +2294,9 @@ static mapping_t* insert_mapping(BDRVVVFATState* s,
 }
 if (index >= s->mapping.next || mapping->begin > begin) {
 mapping = array_insert(&(s->mapping), index, 1);
+if (mapping == NULL) {
+return NULL;
+}
 mapping->path = NULL;
 adjust_mapping_indices(s, index, +1);
 }
@@ -2428,6 +2464,9 @@ static int commit_direntries(BDRVVVFATState* s,
 direntry_t* direntry = array_get(&(s->directory), dir_index);
 uint32_t first_cluster = dir_index == 0 ? 0 : begin_of_direntry(direntry);
 mapping_t* mapping = find_mapping_for_cluster(s, first_cluster);
+if (mapping == NULL) {
+return -1;
+}
 
 int factor = 0x10 * s->sectors_per_cluster;
 int old_cluster_count, new_cluste

[Qemu-devel] [PATCH 4/8] qemu-img: potential Null pointer deref in img_commit()

The function block_job_get() may return NULL so before dereferencing
the 'job' pointer in img_commit() it should be checked.

Signed-off-by: Liam Merwick 
Reviewed-by: Darren Kenny 
Reviewed-by: Mark Kanda 
---
 qemu-img.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index b12f4cd19b0a..51fe09bd08ed 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1029,6 +1029,9 @@ static int img_commit(int argc, char **argv)
 }
 
 job = block_job_get("commit");
+if (job == NULL) {
+goto unref_backing;
+}
 run_block_job(job, &local_err);
 if (local_err) {
 goto unref_backing;
-- 
1.8.3.1

[Qemu-devel] [PATCH 1/8] configure: Provide option to explicitly disable AVX2

The configure script detects if the compiler has AVX2 support and
automatically sets avx2_opt="yes" which in turn defines CONFIG_AVX2_OPT.
There is no way of explicitly overriding this setting so this commit adds
two command-line options: --enable-avx2 and --disable-avx2.

The default behaviour, when no option is specified, is to maintain the
current behaviour and enable AVX2 if the compiler supports it.

Signed-off-by: Liam Merwick 
Reviewed-by: Darren Kenny 
Reviewed-by: Mark Kanda 
---
 configure | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 58862d2ae88a..8904a91715b3 100755
--- a/configure
+++ b/configure
@@ -427,7 +427,7 @@ usb_redir=""
 opengl=""
 opengl_dmabuf="no"
 cpuid_h="no"
-avx2_opt="no"
+avx2_opt=""
 zlib="yes"
 capstone=""
 lzo=""
@@ -1332,6 +1332,10 @@ for opt do
   ;;
   --disable-glusterfs) glusterfs="no"
   ;;
+  --disable-avx2) avx2_opt="no"
+  ;;
+  --enable-avx2) avx2_opt="yes"
+  ;;
   --enable-glusterfs) glusterfs="yes"
   ;;
   --disable-virtio-blk-data-plane|--enable-virtio-blk-data-plane)
@@ -1709,6 +1713,7 @@ disabled with --disable-FEATURE, default is enabled if 
available:
   libxml2 for Parallels image format
   tcmalloctcmalloc support
   jemallocjemalloc support
+  avx2AVX2 optimization support
   replication replication support
   vhost-vsock virtio sockets device support
   opengl  opengl support
@@ -5127,7 +5132,7 @@ fi
 # There is no point enabling this if cpuid.h is not usable,
 # since we won't be able to select the new routines.
 
-if test $cpuid_h = yes; then
+if test "$cpuid_h" = "yes" -a "$avx2_opt" != "no"; then
   cat > $TMPC << EOF
 #pragma GCC push_options
 #pragma GCC target("avx2")
@@ -5141,6 +5146,8 @@ int main(int argc, char *argv[]) { return bar(argv[0]); }
 EOF
   if compile_object "" ; then
 avx2_opt="yes"
+  else
+avx2_opt="no"
   fi
 fi
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 0/4] tests: add a few QMP-related tests

Hi,

The test patches from "[PATCH v4 00/10] monitor: various code
simplification and fixes" series.

v5:
- add "tests: add qmp_assert_error_class()" patch
- commit message tweaks, TODO

Marc-André Lureau (4):
  tests: add qmp_assert_error_class()
  tests: add qmp/object-add-without-props test
  tests: add qmp/qom-set-without-value test
  tests: add a qmp success-response test

 tests/libqtest.h|  9 +++
 tests/drive_del-test.c  |  5 +-
 tests/libqtest.c| 11 
 tests/qmp-cmd-test.c| 18 +
 tests/qmp-test.c| 87 +++--
 tests/test-qga.c|  9 +--
 tests/test-qmp-cmds.c   | 17 +
 tests/qapi-schema/qapi-schema-test.json |  2 +
 tests/qapi-schema/qapi-schema-test.out  |  2 +
 9 files changed, 99 insertions(+), 61 deletions(-)

-- 
2.19.0.rc0.48.gb9dfa238d5

Re: [Qemu-devel] [RFC V1] hw/pci/pci_example.c : Added a new pci device

2018-08-30 Thread Yoni Bettan





On 8/29/18 2:40 PM, Cornelia Huck wrote:

On Tue, 28 Aug 2018 10:24:26 +0300
Yoni Bettan  wrote:


Thanks you for your review!



 - this is a simple example of how to write a pci device that supports
   portio, mmio, irq and dma

Do you also plan to add example code for MSI(-X)?

[Not just asking because s390 only supports MSI-X. Honestly :)]


For now my main focus is to finish the virtIO device ASAP, so adding 
MSI-X support is in low priority.

But if it is a deal breaker for this patch to be accepted I will add it.
In both cases I have no problem to add MSI-X support later because the 
main purpose is to have an example device describing "the right way",
so, after you bring it to my attention in my opinion, MSI-X support 
should be added.

I will mention that subject on V2 and see what others think about it.



Signed-off-by: Yoni Bettan 
---
  hw/pci/Makefile.objs |   1 +
  hw/pci/pci_example.c | 309 +++
  2 files changed, 310 insertions(+)
  create mode 100644 hw/pci/pci_example.c


(...)


diff --git a/hw/pci/pci_example.c b/hw/pci/pci_example.c
new file mode 100644
index 00..326c9b7fa1
--- /dev/null
+++ b/hw/pci/pci_example.c
@@ -0,0 +1,309 @@
+#include "qemu/osdep.h"
+#include "hw/pci/pci.h"
+
+#define TYPE_PCI_EXAMPLE "pci-example"
+
+#define PCI_EXAMPLE(obj)  \
+OBJECT_CHECK(PCIExampleDevState, (obj), TYPE_PCI_EXAMPLE)
+
+
+#define ERROR -1

It's probably not a good idea to obfuscate the return code. Either
return -1, or a proper error code.


You are right, this will be repaired.
In addition another problem here is that a negative value is returned 
from functions that returns uint64_t so I will need to repair it anyway.



+#define BLOCK_SIZE 64
+#define EXAMPLE_MMIO_SIZE BLOCK_SIZE
+#define EXAMPLE_PIO_SIZE BLOCK_SIZE
+#define DMA_BUFF_SIZE 4096
+
+/*---*/
+/* PCI Struct*/
+/*---*/
+
+typedef struct PCIExampleDevState {
+/*< private >*/
+PCIDevice parent_obj;
+/*< public >*/
+
+/* memory region */
+MemoryRegion portio;
+MemoryRegion mmio;
+MemoryRegion irqio;
+MemoryRegion dmaio;
+
+/* data registers */
+uint64_t memData, ioData, dmaPhisicalBase;

I think we want this to be mem_data, io_data, and dma_physical_base
instead.


You are right, after reading 
https://github.com/portante/qemu/blob/master/CODING_STYLE I have notice 
that and this will be updated in V2.



+
+qemu_irq irq;
+/* for the driver to determine if this device caused the interrupt */
+uint64_t threwIrq;

Same here (threw_irq).


Same as above.



+
+} PCIExampleDevState;
+
+
+/*---*/
+/* Read/Write functions  */
+/*---*/
+
+/* do nothing because the mmio read is done from DMA buffer */
+static uint64_t pci_example_mmio_read(void *opaque, hwaddr addr, unsigned size)
+{
+return ERROR;
+}
+
+static void
+pci_example_mmio_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)

I think our preferred style is to keep return value and function name
on the same line and add line breaks in the parameter list, if needed.
(Some more occurrences further down.)


No problem, I prefer this style to.



+{
+PCIExampleDevState *pms = (PCIExampleDevState *)opaque;

peds? ped_state? (I'm not sure pms <-> PCIExampleDevState is obvious;
I'm not sure either whether my suggestions are better :)


This device originally was called PCIMulDevState since it just multiply 
a number, before submitting the patch I changed it to PCIExampleDevState 
but forgot to update the variables names :)

I will repair that.



+PCIDevice *pciDev = (PCIDevice *)opaque;

pci_dev is better, I think. Also, I'd probably refer to pms->parent_obj
(maybe call that one pci_device instead?) instead of casting.


I will update the name to fit to the coding style. About referencing to 
parent_obj I need to look again at the code to understand why I did it 
that way.



+
+if (size != 1) {
+return;
+}
+
+/* compute the result */
+pms->memData = val * 2;
+
+/* write the result directly to phisical memory */
+cpu_physical_memory_write(pms->dmaPhisicalBase, &pms->memData,
+DMA_BUFF_SIZE);

Indentation seems off.


How would you have done it otherwise ? this is a simple "Enter" when 80 
characters are reached.



+
+/* raise an IRQ to notify DMA has finished  */
+pms->threwIrq = 1;
+pci_irq_assert(pciDev);
+}

(...)


+/* do nothing because physical DMA buffer addres is onlyt set and don't need to
+ * be red */

s/addres/address/
s/onlyt/only/
s/don't/doesn't/
s/red/read/


Sorry for that, I didn't

[Qemu-devel] [PATCH v5 2/4] tests: add qmp/object-add-without-props test

test_object_add_without_props() tests a bug in qmp_object_add() we
fixed in commit e64c75a975.  Sadly, we don't have systematic
object-add tests.  This lone test can go into qmp-cmd-test for want of
a better home.

Signed-off-by: Marc-André Lureau 
---
 tests/qmp-cmd-test.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/tests/qmp-cmd-test.c b/tests/qmp-cmd-test.c
index c5b70df974..d12cac539c 100644
--- a/tests/qmp-cmd-test.c
+++ b/tests/qmp-cmd-test.c
@@ -197,6 +197,19 @@ static void add_query_tests(QmpSchema *schema)
 }
 }
 
+static void test_object_add_without_props(void)
+{
+QTestState *qts;
+QDict *resp;
+
+qts = qtest_init(common_args);
+resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
+" {'qom-type': 'memory-backend-ram', 'id': 'ram1' } }");
+g_assert_nonnull(resp);
+qmp_assert_error_class(resp, "GenericError");
+qtest_quit(qts);
+}
+
 int main(int argc, char *argv[])
 {
 QmpSchema schema;
@@ -206,6 +219,11 @@ int main(int argc, char *argv[])
 
 qmp_schema_init(&schema);
 add_query_tests(&schema);
+
+qtest_add_func("qmp/object-add-without-props",
+   test_object_add_without_props);
+/* TODO: add coverage of generic object-add failure modes */
+
 ret = g_test_run();
 
 qmp_schema_cleanup(&schema);
-- 
2.19.0.rc0.48.gb9dfa238d5

Re: [Qemu-devel] [PATCH v4 07/10] tests: add qmp/qom-set-without-value test

Hi

On Thu, Aug 30, 2018 at 3:05 PM Markus Armbruster  wrote:
>
> Marc-André Lureau  writes:
>
> > test_qom_set_without_value() is about a bug in infrastructure used by
> > the QMP core, fixed in commit c489780203.  We covered the bug in
> > infrastructure unit tests (commit bce3035a44).  I wrote that test
> > earlier, to cover QMP level as well, the test could go into qmp-test.
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  tests/qmp-test.c | 18 ++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/tests/qmp-test.c b/tests/qmp-test.c
> > index 4ae2245484..fdfe73b6d2 100644
> > --- a/tests/qmp-test.c
> > +++ b/tests/qmp-test.c
> > @@ -348,6 +348,23 @@ static void test_qmp_preconfig(void)
> >  qtest_quit(qs);
> >  }
> >
> > +static void test_qom_set_without_value(void)
> > +{
> > +QTestState *qts;
> > +QDict *ret;
> > +
> > +qts = qtest_init(common_args);
> > +
> > +ret = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
> > +" { 'path': '/machine', 'property': 'rtc-time' } }");
> > +g_assert_nonnull(ret);
> > +
> > +g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
> > +
> > +qobject_unref(ret);
> > +qtest_quit(qts);
> > +}
> > +
> >  int main(int argc, char *argv[])
> >  {
> >  g_test_init(&argc, &argv, NULL);
> > @@ -355,6 +372,7 @@ int main(int argc, char *argv[])
> >  qtest_add_func("qmp/protocol", test_qmp_protocol);
> >  qtest_add_func("qmp/oob", test_qmp_oob);
> >  qtest_add_func("qmp/preconfig", test_qmp_preconfig);
> > +qtest_add_func("qmp/qom-set-without-value", 
> > test_qom_set_without_value);
> >
> >  return g_test_run();
> >  }
>
> What we're testing here is a missing 'any' parameter.  The test should
> be named accordingly.  Perhaps:
>
>qtest_add_func("qmp/missing-any", test_missing_any);

Eh, the inner visitor fix was about an 'any' parameter missing.

But the test is more about about checking the behaviour of qom-set
without 'value' argument. I would not rename it based on the internal
bug that was fixed.

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] qga: ignore non present cpus when handling qmp_guest_get_vcpus()

2018-08-30 Thread Laszlo Ersek

+Drew

On 08/30/18 14:08, Igor Mammedov wrote:
> If VM has VCPUs plugged sparselly (for example a VM started with
> 3 VCPUs (cpu0, cpu1 and cpu2) and then cpu1 was hotunplugged so
> only cpu0 and cpu2 are present), QGA will rise a error
>   error: internal error: unable to execute QEMU agent command 
> 'guest-get-vcpus':
>   open("/sys/devices/system/cpu/cpu1/"): No such file or directory
> when
>   virsh vcpucount FOO --guest
> is executed.
> Fix it by ignoring non present CPUs when fetching CPUs status from sysfs.
> 
> Signed-off-by: Igor Mammedov 
> ---
>  qga/commands-posix.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 37e8a2d..2929872 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2044,7 +2044,9 @@ static void transfer_vcpu(GuestLogicalProcessor *vcpu, 
> bool sys2vcpu,
>vcpu->logical_id);
>  dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
>  if (dirfd == -1) {
> -error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
> +if (!(sys2vcpu && errno == ENOENT)) {
> +error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
> +}
>  } else {
>  static const char fn[] = "online";
>  int fd;
> 

Originally these guest agent commands (both getting and setting) were
meant to be used in the absence of real VCPU hot[un]plug, as a fallback
/ place-holder.

If the latter (= real VCPU hot(un)plug) works, then these guest agent
commands shouldn't be used at all.

Drew, do I remember correctly? ... The related RHBZ is
. (It's a private
one, and I'm not at liberty to open it up, so my apologies to non-RH folks.)

Anyway, given that "set" should be a subset of the "get" return value
(as documented in the command schema), if we fix up "get" to work with
sparse topologies, then "set" should work at once.

However... as far as I understand, this change will allow
qmp_guest_get_vcpus() to produce a GuestLogicalProcessor object for the
missing (hot-unplugged) VCPU, with the following contents:
- @logical-id: populated by the loop,
- @online: false (from g_malloc0()),
- @can-offline: present (from the loop), and false (from g_malloc0()).

The smaller problem with this might be that "online==false &&
can-offline==false" is nonsensical and has never been returned before. I
don't know how higher level apps will react.

The larger problem might be that a higher level app could simply copy
this output structure into the next "set" call unchanged, and then that
"set" call will fail.

I wonder if, instead of this patch, we should rework
qmp_guest_get_vcpus(), to silently skip processors for which this
dirpath ENOENT condition arises (i.e., return a shorter list of
GuestLogicalProcessor objects).

But, again, I wouldn't mix this guest agent command with real VCPU
hot(un)plug in the first place. The latter is much-much better, so if
it's available, use that exclusively?

Thanks,
Laszlo

[Qemu-devel] [PATCH] slirp: Propagate host TCP RST packet to the guest after socket disconnected

2018-08-30 Thread Gavin Grant via Qemu-devel

Commit 27d92ebc5ed1bb0b518d0ebc4c609182ad20a799 handled the case where the TCP
connection is abruptly closed via a RST packet, by checking for the ECONNRESET
errno. However it does not consider the case where the connection has been
half-closed by the host (FIN/ACK), then the host socket is disconnected. For
example, if the host application calls close() on the socket, then the
application exits.

In this case, the socket still exists due to the file descriptor in SLIRP, but
it is disconnected. recv() does not indicate an error since an orderly socket
close has previously occurred. The socket will then be stuck in FIN_WAIT_2,
until the peer sends FIN/ACK or a timeout occurs. Instead we can send a RST
to the peer and transition to the CLOSED state.

Signed-off-by: Gavin Grant 
---
 slirp/socket.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/slirp/socket.c b/slirp/socket.c
index 08fe98907d..543bd5feaf 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -204,12 +204,17 @@ soread(struct socket *so)
return 0;
else {
int err;
+   struct sockaddr addr;
socklen_t slen = sizeof err;
 
err = errno;
if (nn == 0) {
-   getsockopt(so->s, SOL_SOCKET, SO_ERROR,
-  &err, &slen);
+   if (getpeername(so->s, &addr, &slen) < 0) {
+   err = errno;
+   } else {
+   getsockopt(so->s, SOL_SOCKET, SO_ERROR,
+   &err, &slen);
+   }
}
 
DEBUG_MISC((dfd, " --- soread() disconnected, nn = %d, 
errno = %d-%s\n", nn, errno,strerror(errno)));
-- 
2.11.0

[Qemu-devel] [PATCH v5 3/4] tests: add qmp/qom-set-without-value test

test_qom_set_without_value() is about a bug in infrastructure used by
the QMP core, fixed in commit c489780203.  We covered the bug in
infrastructure unit tests (commit bce3035a44).  I wrote that test
earlier, to cover QMP level as well, the test could go into qmp-test.

Signed-off-by: Marc-André Lureau 
---
 tests/qmp-test.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index b3472281ae..2b923f02db 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -321,6 +321,19 @@ static void test_qmp_preconfig(void)
 qtest_quit(qs);
 }
 
+static void test_qom_set_without_value(void)
+{
+QTestState *qts;
+QDict *resp;
+
+qts = qtest_init(common_args);
+resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
+ " { 'path': '/machine', 'property': 'rtc-time' } }");
+g_assert_nonnull(resp);
+qmp_assert_error_class(resp, "GenericError");
+qtest_quit(qts);
+}
+
 int main(int argc, char *argv[])
 {
 g_test_init(&argc, &argv, NULL);
@@ -328,6 +341,7 @@ int main(int argc, char *argv[])
 qtest_add_func("qmp/protocol", test_qmp_protocol);
 qtest_add_func("qmp/oob", test_qmp_oob);
 qtest_add_func("qmp/preconfig", test_qmp_preconfig);
+qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value);
 
 return g_test_run();
 }
-- 
2.19.0.rc0.48.gb9dfa238d5

[Qemu-devel] [PATCH v5 1/4] tests: add qmp_assert_error_class()

This helper will simplify a bunch of code checking for QMP errors and
can be shared by various tests.  Note that test-qga does check for
error description as well, so don't replace the code there for now.

Signed-off-by: Marc-André Lureau 
---
 tests/libqtest.h   |  9 ++
 tests/drive_del-test.c |  5 +--
 tests/libqtest.c   | 11 +++
 tests/qmp-test.c   | 73 +-
 tests/test-qga.c   |  9 ++
 5 files changed, 46 insertions(+), 61 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index 36d5caecd4..ed88ff99d5 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -1004,4 +1004,13 @@ void qtest_qmp_device_del(const char *id);
  */
 bool qmp_rsp_is_err(QDict *rsp);
 
+/**
+ * qmp_assert_error_class:
+ * @rsp: QMP response to check for error
+ * @class: an error class
+ *
+ * Assert the response has the given error class and discard @rsp.
+ */
+void qmp_assert_error_class(QDict *rsp, const char *class);
+
 #endif
diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
index 673c10140f..4a1a0889c8 100644
--- a/tests/drive_del-test.c
+++ b/tests/drive_del-test.c
@@ -67,7 +67,6 @@ static void test_after_failed_device_add(void)
 {
 char driver[32];
 QDict *response;
-QDict *error;
 
 snprintf(driver, sizeof(driver), "virtio-blk-%s",
  qvirtio_get_dev_type());
@@ -83,9 +82,7 @@ static void test_after_failed_device_add(void)
"   'drive': 'drive0'"
"}}", driver);
 g_assert(response);
-error = qdict_get_qdict(response, "error");
-g_assert_cmpstr(qdict_get_try_str(error, "class"), ==, "GenericError");
-qobject_unref(response);
+qmp_assert_error_class(response, "GenericError");
 
 /* Delete the drive */
 drive_del();
diff --git a/tests/libqtest.c b/tests/libqtest.c
index d635c5bea0..2cd5736642 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -1194,3 +1194,14 @@ bool qmp_rsp_is_err(QDict *rsp)
 qobject_unref(rsp);
 return !!error;
 }
+
+void qmp_assert_error_class(QDict *rsp, const char *class)
+{
+QDict *error = qdict_get_qdict(rsp, "error");
+
+g_assert_cmpstr(qdict_get_try_str(error, "class"), ==, class);
+g_assert_nonnull(qdict_get_try_str(error, "desc"));
+g_assert(!qdict_haskey(rsp, "return"));
+
+qobject_unref(rsp);
+}
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 4ae2245484..b3472281ae 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -21,15 +21,6 @@
 
 const char common_args[] = "-nodefaults -machine none";
 
-static const char *get_error_class(QDict *resp)
-{
-QDict *error = qdict_get_qdict(resp, "error");
-const char *desc = qdict_get_try_str(error, "desc");
-
-g_assert(desc);
-return error ? qdict_get_try_str(error, "class") : NULL;
-}
-
 static void test_version(QObject *version)
 {
 Visitor *v;
@@ -42,15 +33,12 @@ static void test_version(QObject *version)
 visit_free(v);
 }
 
-static bool recovered(QTestState *qts)
+static void assert_recovered(QTestState *qts)
 {
 QDict *resp;
-bool ret;
 
 resp = qtest_qmp(qts, "{ 'execute': 'no-such-cmd' }");
-ret = !strcmp(get_error_class(resp), "CommandNotFound");
-qobject_unref(resp);
-return ret;
+qmp_assert_error_class(resp, "CommandNotFound");
 }
 
 static void test_malformed(QTestState *qts)
@@ -60,73 +48,61 @@ static void test_malformed(QTestState *qts)
 /* syntax error */
 qtest_qmp_send_raw(qts, "{]\n");
 resp = qtest_qmp_receive(qts);
-g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
-qobject_unref(resp);
-g_assert(recovered(qts));
+qmp_assert_error_class(resp, "GenericError");
+assert_recovered(qts);
 
 /* lexical error: impossible byte outside string */
 qtest_qmp_send_raw(qts, "{\xFF");
 resp = qtest_qmp_receive(qts);
-g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
-qobject_unref(resp);
-g_assert(recovered(qts));
+qmp_assert_error_class(resp, "GenericError");
+assert_recovered(qts);
 
 /* lexical error: funny control character outside string */
 qtest_qmp_send_raw(qts, "{\x01");
 resp = qtest_qmp_receive(qts);
-g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
-qobject_unref(resp);
-g_assert(recovered(qts));
+qmp_assert_error_class(resp, "GenericError");
+assert_recovered(qts);
 
 /* lexical error: impossible byte in string */
 qtest_qmp_send_raw(qts, "{'bad \xFF");
 resp = qtest_qmp_receive(qts);
-g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
-qobject_unref(resp);
-g_assert(recovered(qts));
+qmp_assert_error_class(resp, "GenericError");
+assert_recovered(qts);
 
 /* lexical error: control character in string */
 qtest_qmp_send_raw(qts, "{'execute': 'nonexistent', 'id':'\n");
 resp = qtest_qmp_receive(qts);
-g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
-qobject_unref(resp);
-

[Qemu-devel] [PATCH v5 4/4] tests: add a qmp success-response test

Verify the usage of this schema feature and the API behaviour.  This
should be the only case where qmp_dispatch() returns NULL.

Signed-off-by: Marc-André Lureau 
Reviewed-by: Markus Armbruster 
---
 tests/test-qmp-cmds.c   | 17 +
 tests/qapi-schema/qapi-schema-test.json |  2 ++
 tests/qapi-schema/qapi-schema-test.out  |  2 ++
 3 files changed, 21 insertions(+)

diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index ab414fa0c9..4ab2b6e5ce 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -32,6 +32,10 @@ void qmp_test_flags_command(Error **errp)
 {
 }
 
+void qmp_cmd_success_response(Error **errp)
+{
+}
+
 Empty2 *qmp_user_def_cmd0(Error **errp)
 {
 return g_new0(Empty2, 1);
@@ -153,6 +157,17 @@ static void test_dispatch_cmd_failure(void)
 qobject_unref(req);
 }
 
+static void test_dispatch_cmd_success_response(void)
+{
+QDict *req = qdict_new();
+QDict *resp;
+
+qdict_put_str(req, "execute", "cmd-success-response");
+resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false);
+g_assert_null(resp);
+qobject_unref(req);
+}
+
 static QObject *test_qmp_dispatch(QDict *req)
 {
 QDict *resp;
@@ -289,6 +304,8 @@ int main(int argc, char **argv)
 g_test_add_func("/qmp/dispatch_cmd", test_dispatch_cmd);
 g_test_add_func("/qmp/dispatch_cmd_failure", test_dispatch_cmd_failure);
 g_test_add_func("/qmp/dispatch_cmd_io", test_dispatch_cmd_io);
+g_test_add_func("/qmp/dispatch_cmd_success_response",
+test_dispatch_cmd_success_response);
 g_test_add_func("/qmp/dealloc_types", test_dealloc_types);
 g_test_add_func("/qmp/dealloc_partial", test_dealloc_partial);
 
diff --git a/tests/qapi-schema/qapi-schema-test.json 
b/tests/qapi-schema/qapi-schema-test.json
index 11aa4c8f8d..fb03163430 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -137,6 +137,8 @@
   'data': {'ud1a': 'UserDefOne', '*ud1b': 'UserDefOne'},
   'returns': 'UserDefTwo' }
 
+{ 'command': 'cmd-success-response', 'data': {}, 'success-response': false }
+
 # Returning a non-dictionary requires a name from the whitelist
 { 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' },
   'returns': 'int' }
diff --git a/tests/qapi-schema/qapi-schema-test.out 
b/tests/qapi-schema/qapi-schema-test.out
index 0da92455da..218ac7d556 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -156,6 +156,8 @@ object q_obj_user_def_cmd2-arg
 member ud1b: UserDefOne optional=True
 command user_def_cmd2 q_obj_user_def_cmd2-arg -> UserDefTwo
gen=True success_response=True boxed=False oob=False preconfig=False
+command cmd-success-response None -> None
+   gen=True success_response=False boxed=False oob=False preconfig=False
 object q_obj_guest-get-time-arg
 member a: int optional=False
 member b: int optional=True
-- 
2.19.0.rc0.48.gb9dfa238d5

[Qemu-devel] [PATCH] slirp: Propagate host TCP RST packet to the guest after socket disconnected

2018-08-30 Thread Gavin Grant via Qemu-devel

[PATCH] slirp: Propagate host TCP RST packet to the guest after socket 
disconnected

Please bear with me, since this is my first correspondence with the qemu-devel
mailing list (or any public devel mailing list for that matter!).

I believe I have witnessed a discrepancy between SLIRP TCP and normal UNIX/Linux
TCP behaviour relating to the sending of RST packet(s) after a socket has been
closed gracefully by a host client application, then the application
subsequently exits. When the host socket is closed gracefully, it is in a
"half-closed" state. i.e. The socket can no longer be read from, but it can be
written to and will wait in FIN_WAIT_1 & FIN_WAIT_2 states until the remote peer
sends a FIN. This is fine. But when the application exits, the host socket is no
longer connected. I would expect subsequent slirp_send() calls to return an 
EPIPE
error, causing RST packet(s) to be sent to the server, causing it to close its
socket.

However, unlike soread() in 27d92ebc5ed1bb0b518d0ebc4c609182ad20a799, sowrite()
does not call tcp_drop() for specific errno's, so the server socket will remain
open. sbappend() does not implement error handling, as it relies on errors to
be handled in soread().

My proposed patch attempts to address this by checking for closed & disconnected
socket status in soread(). Forgive me if my logic and proposal are foolish!

The application(s) that I observed this behaviour from were libiio test
programs (https://github.com/analogdevicesinc/libiio/tree/master/tests), which
connect to an iiod daemon process running on the server, to interface with the
Linux Industrial Input/Output (IIO) subsystem. I noticed that if iiod is running
on a QEMU server with SLIRP, then iiod does not notice when client applications
close their connection and exit. This can result in multiple threads open on the
server, with their sockets stuck in the CLOSE_WAIT state. Once several of these
threads exist, my QEMU system becomes extremely slow and unusable.

Re: [Qemu-devel] [PATCH] slirp: Propagate host TCP RST packet to the guest after socket disconnected

2018-08-30 Thread Samuel Thibault

Hello,

The principle seems sane, I'll have a look.

Thanks,
Samuel

Gavin Grant, le jeu. 30 août 2018 16:57:57 +0100, a ecrit:
> Commit 27d92ebc5ed1bb0b518d0ebc4c609182ad20a799 handled the case where the TCP
> connection is abruptly closed via a RST packet, by checking for the ECONNRESET
> errno. However it does not consider the case where the connection has been
> half-closed by the host (FIN/ACK), then the host socket is disconnected. For
> example, if the host application calls close() on the socket, then the
> application exits.
> 
> In this case, the socket still exists due to the file descriptor in SLIRP, but
> it is disconnected. recv() does not indicate an error since an orderly socket
> close has previously occurred. The socket will then be stuck in FIN_WAIT_2,
> until the peer sends FIN/ACK or a timeout occurs. Instead we can send a RST
> to the peer and transition to the CLOSED state.
> 
> Signed-off-by: Gavin Grant 
> ---
>  slirp/socket.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/slirp/socket.c b/slirp/socket.c
> index 08fe98907d..543bd5feaf 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -204,12 +204,17 @@ soread(struct socket *so)
>   return 0;
>   else {
>   int err;
> + struct sockaddr addr;
>   socklen_t slen = sizeof err;
>  
>   err = errno;
>   if (nn == 0) {
> - getsockopt(so->s, SOL_SOCKET, SO_ERROR,
> -&err, &slen);
> + if (getpeername(so->s, &addr, &slen) < 0) {
> + err = errno;
> + } else {
> + getsockopt(so->s, SOL_SOCKET, SO_ERROR,
> + &err, &slen);
> + }
>   }
>  
>   DEBUG_MISC((dfd, " --- soread() disconnected, nn = %d, 
> errno = %d-%s\n", nn, errno,strerror(errno)));
> -- 
> 2.11.0
> 

-- 
Samuel
***e trouve un .xls
***e passe un moment à se demander quelle version de xml c'est ça, le .xls
e: donc j'ai fait un file
 -+- #sos - on n'a pas forcément les mêmes références que tout le monde -+-

Re: [Qemu-devel] [PATCH 2/9] chardev: mark the calls that allow an implicit mux monitor

Hi
On Thu, Aug 30, 2018 at 4:58 PM Markus Armbruster  wrote:
>
> Marc-André Lureau  writes:
>
> > This is mostly for readability of the code. Let's make it clear which
> > callers can create an implicit monitor when the chardev is muxed.
> >
> > This will also enforce a safer behaviour, as we don't really support
> > creating monitor anywhere/anytime at the moment. Add an assert() to
> > make sure the programmer explicitely wanted that behaviour.
> >
> > There are documented cases, such as: -serial/-parallel/-virtioconsole
> > and to less extent -debugcon.
> >
> > Less obvious and questionable ones are -gdb, SLIRP -guestfwd and Xen
> > console. Add a FIXME note for those, but keep the support for now.
> >
> > Other qemu_chr_new() callers either have a fixed parameter/filename
> > string or do not need it, such as -qtest:
> >
> > * qtest.c: qtest_init()
> >   Afaik, only used by tests/libqtest.c, without mux. I don't think we
> >   support it outside of qemu testing: drop support for implicit mux
> >   monitor (qemu_chr_new() call: no implicit mux now).
> >
> > * hw/
> >   All with literal @filename argument that doesn't enable mux monitor.
> >
> > * tests/
> >   All with @filename argument that doesn't enable mux monitor.
> >
> > On a related note, the list of monitor creation places:
> >
> > - the chardev creators listed above: all from command line (except
> >   perhaps Xen console?)
> >
> > - -gdb & hmp gdbserver will create a "GDB monitor command" chardev
> >   that is wired to an HMP monitor.
> >
> > - -mon command line option
> >
> > From this short study, I would like to think that a monitor may only
> > be created in the main thread today, though I remain skeptical :)
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  include/chardev/char.h | 24 
> >  chardev/char.c | 32 +---
> >  gdbstub.c  |  6 +-
> >  hw/char/xen_console.c  |  5 -
> >  net/slirp.c|  5 -
> >  vl.c   | 10 +-
> >  6 files changed, 63 insertions(+), 19 deletions(-)
> >
> > diff --git a/include/chardev/char.h b/include/chardev/char.h
> > index 3e4fe6dad0..268daaa90b 100644
> > --- a/include/chardev/char.h
> > +++ b/include/chardev/char.h
> > @@ -105,14 +105,27 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
> >   * @filename: the URI
> >   *
> >   * Create a new character backend from a URI.
> > + * Do not implicitly initialize a monitor if the chardev is muxed.
> >   *
> >   * Returns: a new character backend
> >   */
> >  Chardev *qemu_chr_new(const char *label, const char *filename);
> >
> >  /**
> > - * qemu_chr_change:
> > - * @opts: the new backend options
> > + * qemu_chr_new_mux_mon:
> > + * @label: the name of the backend
> > + * @filename: the URI
> > + *
> > + * Create a new character backend from a URI.
> > + * Implicitly initialize a monitor if the chardev is muxed.
> > + *
> > + * Returns: a new character backend
> > + */
> > +Chardev *qemu_chr_new_mux_mon(const char *label, const char *filename);
> > +
> > +/**
> > +* qemu_chr_change:
> > +* @opts: the new backend options
> >   *
> >   * Change an existing character backend
> >   */
> > @@ -129,6 +142,7 @@ void qemu_chr_cleanup(void);
> >   * qemu_chr_new_noreplay:
> >   * @label: the name of the backend
> >   * @filename: the URI
> > + * @with_mux_mon: if chardev is muxed, initialize a monitor
> >   *
> >   * Create a new character backend from a URI.
> >   * Character device communications are not written
> > @@ -136,7 +150,8 @@ void qemu_chr_cleanup(void);
> >   *
> >   * Returns: a new character backend
> >   */
> > -Chardev *qemu_chr_new_noreplay(const char *label, const char *filename);
> > +Chardev *qemu_chr_new_noreplay(const char *label, const char *filename,
> > +   bool with_mux_mon);
> >
> >  /**
> >   * qemu_chr_be_can_write:
> > @@ -194,7 +209,8 @@ bool qemu_chr_has_feature(Chardev *chr,
> >ChardevFeature feature);
> >  void qemu_chr_set_feature(Chardev *chr,
> >ChardevFeature feature);
> > -QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
> > +QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
> > +bool with_mux_mon);
> >  int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool 
> > write_all);
> >  #define qemu_chr_write_all(s, buf, len) qemu_chr_write(s, buf, len, true)
> >  int qemu_chr_wait_connected(Chardev *chr, Error **errp);
> > diff --git a/chardev/char.c b/chardev/char.c
> > index 76d866e6fe..c1b89b6638 100644
> > --- a/chardev/char.c
> > +++ b/chardev/char.c
> > @@ -329,7 +329,8 @@ int qemu_chr_wait_connected(Chardev *chr, Error **errp)
> >  return 0;
> >  }
> >
> > -QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
> > +QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
> > +

Re: [Qemu-devel] [PATCH 7/8] io: file descriptor not initialized in qio_channel_command_new_spawn()

2018-08-30 Thread Eric Blake


On 08/30/2018 10:47 AM, Liam Merwick wrote:

Incorrect checking of flags could result in uninitialized
file descriptor being used.

Signed-off-by: Liam Merwick 
Reviewed-by: Darren Kenny 
Reviewed-by: Mark Kanda 
---
  io/channel-command.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/io/channel-command.c b/io/channel-command.c
index 3e7eb17eff54..38deb687da21 100644
--- a/io/channel-command.c
+++ b/io/channel-command.c
@@ -59,10 +59,10 @@ qio_channel_command_new_spawn(const char *const argv[],
  
  flags = flags & O_ACCMODE;
  
-if (flags == O_RDONLY) {

+if ((flags & O_RDONLY) == O_RDONLY) {


NACK.  O_RDONLY and O_WRONLY are subsets of O_ACCMODE, which we already 
masked out above.


On some systems, we have:
O_RDONLY == 0
O_WRONLY == 1
O_RDWR == 2

On other systems, we have:
O_RDONLY == 1
O_WRONLY == 2
O_RDWR == 3

Either way, if the user passed in O_RDWR, (flags & O_RDONLY) == O_RDONLY 
returns true, which is wrong.


O_ACCMODE was historically 0x3, although now that POSIX has O_EXEC and 
O_SEARCH (which can be the same bit pattern), some systems now make 
O_ACCMODE occupy 3 bits instead of 2.


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

Re: [Qemu-devel] [PATCH 4/9] char: update the mux hanlders in class callback

Hi

On Thu, Aug 30, 2018 at 4:58 PM Markus Armbruster  wrote:
>
> Marc-André Lureau  writes:
>
> > Instead of handling mux chardev in a special way in
> > qemu_chr_fe_set_handlers(), we may use the chr_update_read_handler
> > class callback instead.
> >
> > Signed-off-by: Marc-André Lureau 
>
> In your subject, s/hanlders/handlers/.

oops, fixed

does it get you r-b ?
thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 5/9] char-fe: set_handlers() needs an associted chardev