[Qemu-devel] 答复: Re: 答复: Re: [PATCH] vhost: fix a migration failedbecause ofvhost region merge
> > On Wed, Jul 19, 2017 at 03:24:27PM +0200, Igor Mammedov wrote:
> > > On Wed, 19 Jul 2017 12:46:13 +0100
> > > "Dr. David Alan Gilbert" wrote:
> > >
> > > > * Igor Mammedov (imamm...@redhat.com) wrote:
> > > > > On Wed, 19 Jul 2017 23:17:32 +0800
> > > > > Peng Hao wrote:
> > > > >
> > > > > > When a guest that has several hotplugged dimms is migrated, in
> > > > > > destination host it will fail to resume. Because vhost regions of
> > > > > > several dimms in source host are merged and in the restore stage
> > > > > > in destination host it computes whether more than vhost slot limit
> > > > > > before merging vhost regions of several dimms.
> > > > > could you provide a bit more detailed description of the problem
> > > > > including command line+used device_add commands on source and
> > > > > command line on destination?
> > > >
> > > > (ccing in Marc Andre and Maxime)
> > > >
> > > > Hmm, I'd like to understade the situation where you get merging between
> > > > RAMBlocks that complicates some stuff for postcopy.
> > > and probably inconsistent merging breaks vhost as well
> > >
> > > merging might happen if regions are adjacent or overlap
> > > but for that to happen merged regions must have equal
> > > distance between their GPA:HVA pairs, so that following
> > > translation would work:
> > >
> > > if gva in regionX[gva_start, len, hva_start]
> > >hva = hva_start + gva - gva_start
> > >
> > > while GVA of regions is under QEMU control and deterministic
> > > HVA is not, so in migration case merging might happen on source
> > > side but not on destination, resulting in different memory maps.
> > >
> > > Maybe Michael might know details why migration works in vhost usecase,
> > > but I don't see vhost sending any vmstate data.
> >
> > We aren't merging ramblocks at all.
> > When we are passing blocks A and B to vhost, if we see that
> >
> > hvaB=hvaA + lenA
> > gpaB=gpaA + lenA
> >
> > then we can improve performance a bit by passing a single
> > chunk to vhost: hvaA,gpaA,lena+lenB
> >
> >
> > so it does not affect migration normally.
> >
> > - I think it is like this:
> >
> > in source in destination:(restore)
> >
> > realize device 1 realize device 1
> >
> > realize device 2 realize dimm 0
> >
> > ... realize dimm1
> >
> >
> >
> > realize device n realize dimmx
> >
> >realize device m
> >
> > realize dimm0 .
> >
> > realize dimm1 .
> >
> > .. .
> >
> > realize dimmxrealize device n
> >
> >
> > In restore stage ,the sort of realizing device is different from starting
> > vm
> > because of adding dimms.
> >
> > So it may in some stage during restoring can't merge vhost regions.
> If you run over the number of regions supported by vhost on destination
> then you won't be able to start a VM there until you disable vhost.
some regions can not merge when just part of devices have realized.
when all devices are realized on destination, these regions can be merged
again
and the used slots can satisfy the vhost slot limit as on source.
in the restore stage the vm is not in running state, so don't compute if more
than
vhost slot limit when vm is not running. when a last device is realized,
all regions are merged to the slot number as on source. then the state of vm
changes to running.
it satisfies the vhost slot limit on source, so it should satify on
destination.
> > > > > >
> > > > > > Signed-off-by: Peng Hao
> > > > > > Signed-off-by: Wang Yechao
> > > > > > ---
> > > > > > hw/mem/pc-dimm.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > > > > > index ea67b46..bb0fa08 100644
> > > > > > --- a/hw/mem/pc-dimm.c
> > > > > > +++ b/hw/mem/pc-dimm.c
> > > > > > @@ -101,7 +101,7 @@ void pc_dimm_memory_plug
> > (DeviceState *dev, MemoryHotplugState *hpms,
> > > > > > goto out
> > > > > > }
> > > > > >
> > > > > > -if (!vhost_has_free_slot()) {
> > > > > > +if (!vhost_has_free_slot() && runstate_is_running()) {
> > > > > > error_setg(&local_err, "a used vhost backend has no free"
> > > > > > " memory slots left")
> > > > > > goto out
> > > >
> > > > Even this produces the wrong error message in this case,
> > > > it also makes me think if the existing code should undo a lot of
> > > > the object_property_set's that happen.
> > > >
> > > > Dave
> > > > >
> > > > >
> > > > --
> > > > Dr. David Alan Gilbert / dgilb..
Re: [Qemu-devel] [PATCH 1/2] [PATCH] hmp: dump ids includingsocket-id, core-id and so on for 'info registers'
>On Fri, 22 Jul 2017 03:38:55 -0400
>Yi Wang wrote:
>
>> This patch add output of CPUs' socket-id, core-id, thread-id and
>> apic-id for 'info registers', which can be used for querying other
>> hmp commands.
>>
>> Signed-off-by: Yi Wang
>> Signed-off-by: Yun Liu
>> ---
>> include/qom/cpu.h| 12
>> monitor.c| 1 +
>> qom/cpu.c| 10 ++
>> target/i386/cpu.c| 1 +
>> target/i386/cpu.h| 1 +
>> target/i386/helper.c | 13 +
>> 6 files changed, 38 insertions(+)
>>
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 25eefea..4717bd7 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -92,6 +92,7 @@ struct TranslationBlock
>> * CPUs can use the default implementation of this method. This method
>> should
>> * not be used by any callers other than the pre-1.0 virtio devices.
>> * @memory_rw_debug: Callback for GDB memory access.
>> + * @dump_ids: Callback for dumping ids.
>> * @dump_state: Callback for dumping state.
>> * @dump_statistics: Callback for dumping statistics.
>> * @get_arch_id: Callback for getting architecture-dependent CPU ID.
>> @@ -156,6 +157,7 @@ typedef struct CPUClass {
>> bool (*virtio_is_big_endian)(CPUState *cpu)
>> int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
>> uint8_t *buf, int len, bool is_write)
>> +void (*dump_ids)(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf)
>> void (*dump_state)(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
>> int flags)
>> GuestPanicInformation* (*get_crash_info)(CPUState *cpu)
>> @@ -518,6 +520,16 @@ enum CPUDumpFlags {
>> }
>>
>> /**
>> + * cpu_dump_ids:
>> + * @cpu: The CPU whose state is to be dumped.
>> + * @f: File to dump to.
>> + * @cpu_fprintf: Function to dump with.
>> + *
>> + * Dumps CPU socket-id, core-id, thread-id and apic-id.
>> + */
>> +void cpu_dump_ids(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf)
>> +
>> +/**
>> * cpu_dump_state:
>> * @cpu: The CPU whose state is to be dumped.
>> * @f: File to dump to.
>> diff --git a/monitor.c b/monitor.c
>> index 6d040e6..30f898c 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -1085,6 +1085,7 @@ static void hmp_info_registers(Monitor *mon, const
>> QDict *qdict)
>> if (all_cpus) {
>> CPU_FOREACH(cs) {
>> monitor_printf(mon, "\nCPU#%d\n", cs->cpu_index)
>> +cpu_dump_ids(cs, (FILE *)mon, monitor_fprintf)
>> cpu_dump_state(cs, (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU)
>> }
>> } else {
>> diff --git a/qom/cpu.c b/qom/cpu.c
>> index 4f38db0..a9df661 100644
>> --- a/qom/cpu.c
>> +++ b/qom/cpu.c
>> @@ -242,6 +242,16 @@ GuestPanicInformation *cpu_get_crash_info(CPUState *cpu)
>> return res
>> }
>>
>> +void cpu_dump_ids(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf)
>> +{
>> +CPUClass *cc = CPU_GET_CLASS(cpu)
>> +
>> +if (cc->dump_ids) {
>> +cpu_synchronize_state(cpu)
>> +cc->dump_ids(cpu, f, cpu_fprintf)
>> +}
>> +}
>> +
>> void cpu_dump_state(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
>> int flags)
>> {
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 0bbda76..0aa488f 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -4120,6 +4120,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc,
>> void *data)
>> cc->do_interrupt = x86_cpu_do_interrupt
>> cc->cpu_exec_interrupt = x86_cpu_exec_interrupt
>> #endif
>> +cc->dump_ids = x86_cpu_dump_ids
>> cc->dump_state = x86_cpu_dump_state
>> cc->get_crash_info = x86_cpu_get_crash_info
>> cc->set_pc = x86_cpu_set_pc
>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>> index 0518673..31ad681 100644
>> --- a/target/i386/cpu.h
>> +++ b/target/i386/cpu.h
>> @@ -1316,6 +1316,7 @@ int x86_cpu_write_elf32_qemunote(WriteCoreDumpFunction
>> f, CPUState *cpu,
>> void x86_cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list,
>> Error **errp)
>>
>> +void x86_cpu_dump_ids(CPUState *cs, FILE *f, fprintf_function cpu_fprintf)
>> void x86_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
>> int flags)
>>
>> diff --git a/target/i386/helper.c b/target/i386/helper.c
>> index f63eb3d..fd4f1af 100644
>> --- a/target/i386/helper.c
>> +++ b/target/i386/helper.c
>> @@ -408,6 +408,19 @@ void x86_cpu_dump_local_apic_state(CPUState *cs, FILE
>> *f,
>> #define DUMP_CODE_BYTES_TOTAL50
>> #define DUMP_CODE_BYTES_BACKWARD 20
>>
>> +void x86_cpu_dump_ids(CPUState *cs, FILE *f, fprintf_function cpu_fprintf)
>> +{
>
>
>> +X86CPU *cpu = X86_CPU(cs)
>> +APICCommonState *s = APIC_COMMON(cpu->apic_state)
>> +if (!s) {
>> +cpu_fprintf
Re: [Qemu-devel] [PATCH v6 8/8] tpm: Added support for TPM emulator
On Tue, 2017-07-18 at 05:08 -0700, Marc-André Lureau wrote: > Hi > > On Tue, Jul 18, 2017 at 1:49 AM, Amarnath Valluri > wrote: > > This change introduces a new TPM backend driver that can communicate with > > swtpm(software TPM emulator) using unix domain socket interface. > > > > Swtpm uses two unix sockets, one for plain TPM commands and responses, and > > one > > for out-of-band control messages. > > > > The swtpm and associated tools can be found here: > > https://github.com/stefanberger/swtpm > > > > The swtpm's control channel protocol specification can be found here: > > https://github.com/stefanberger/swtpm/wiki/Control-Channel-Specification > > I am afraid this isn't enough yet. > > > Usage: > > # setup TPM state directory > > mkdir /tmp/mytpm > > chown -R tss:root /tmp/mytpm > > /usr/bin/swtpm_setup --tpm-state /tmp/mytpm --createek > > > > # Ask qemu to use TPM emulator with given tpm state directory > > qemu-system-x86_64 \ > > [...] \ > > -tpmdev > > emulator,id=tpm0,tpmstatedir=/tmp/mytpm,logfile=/tmp/swtpm.log \ > > We should rather follow the vhost-user pattern: do not deal with > spawning the external swtpm/backend, just use chardev to connect to > it. At least you don't have to deal with process argument details, > management etc that may change version to version. I made the spawning completely optional and it is useful in some environments, especially makes life easier when multiple QEMU instances on a system. One can configure QEMU to connect to already running software emulator using Unix domain socket paths: -tpmdev emulator,id=tpm0,data-path=/path/,ctrl-path=/path - Amarnath
Re: [Qemu-devel] [PATCH 00/11] Make memory_region_init_ram() and friends handle migration
Hi Peter, On 07/07/2017 11:42 AM, Peter Maydell wrote: This patchset changes the memory region functions - memory_region_init_ram() - memory_region_init_rom() - memory_region_init_rom_device() to all automatically register the backing memory they allocate for migration using vmstate_register_ram(). Renamed functions - memory_region_init_ram_nomigrate() - memory_region_init_rom_nomigrate() - memory_region_init_rom_device_nomigrate() are provided which only do the MR init, for the oddball cases which want to manage migration of the backing memory themselves (and to avoid behavioural changes for callers which weren't managing correctly migration themselves...) [...] I'm seeing memleaks using the malta machine, they come from the smbus_eeprom_init() in hw/i2c/smbus_eeprom.c which does: uint8_t *eeprom_buf = g_malloc0(8 * 256); /* XXX: make this persistent */ Question for 2.11: is this device a candidate to use one of the function you enumerated? My guess is memory_region_init_rom_device() Question for 2.10: does that mean than the malta machine is not migratable? Thanks! Phil.
[Qemu-devel] [Bug 1300021] Re: after loadvm the system clock isn't current time
[Expired for QEMU because there has been no activity for 60 days.] ** Changed in: qemu Status: Incomplete => Expired -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1300021 Title: after loadvm the system clock isn't current time Status in QEMU: Expired Bug description: hi, when i load a snapshot of month ago using "loadvm name"command, the vm system time is past time,not recover current time. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1300021/+subscriptions
Re: [Qemu-devel] [PATCH v6 4/8] tpm-backend: Made few interface methods optional
On Tue, 2017-07-18 at 10:15 +, Marc-André Lureau wrote: > Hi > > On Tue, Jul 18, 2017 at 10:48 AM Amarnath Valluri > wrote: > > This allows backend implementations left optional interface > methods. > For mandatory methods assertion checks added. > > Took the opportunity to remove unused methods: > tpm_backend_get_type() > tpm_backend_get_desc() > > Signed-off-by: Amarnath Valluri > Reviewed-by: Marc-André Lureau > Reviewed-by: Stefan Berger > --- > backends/tpm.c | 36 > +--- > hw/tpm/tpm_passthrough.c | 23 +-- > include/sysemu/tpm_backend.h | 19 +-- > tpm.c| 2 +- > 4 files changed, 20 insertions(+), 60 deletions(-) > >LINKx86_64-softmmu/qemu-system-x86_64 > ../tpm.o: In function `qmp_query_tpm_inst': > /home/elmarco/src/qq/tpm.c:261: undefined reference to > `tpm_backend_get_type' > collect2: error: ld returned 1 exit status Thanks Marc, for pointing it out, i will this patch. - Amarnath
Re: [Qemu-devel] [PATCH v6 5/8] tmp backend: Add new api to read backend TpmInfo
On Tue, 2017-07-18 at 09:28 -0500, Eric Blake wrote:
> On 07/18/2017 05:39 AM, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Jul 18, 2017 at 1:49 AM, Amarnath Valluri
> > wrote:
> >> TPM configuration options are backend implementation details and shall not
> >> be
> >> part of base TPMBackend object, and these shall not be accessed directly
> >> outside
> >> of the class, hence added a new interface method, get_tpm_options() to
> >> TPMDriverOps., which shall be implemented by the derived classes to return
> >> configured tpm options.
> >>
> > One usually prefer to have the true case first.
> >
> >> +} else {
> >> +tpm_pt->ops->has_path = true;
> >> }
> >>
> >> +tpm_pt->ops->path = g_strdup(value);
> >
> > Interestingly, ops->path will be set even if ops->has_path = false. I
> > am not sure the visitors will handle that case properly (for visit or
> > dealloc etc). Could you set ops->has_path = true uncondtionnally?
>
> tmp_pt->opt->path is ignored if has_path is false; if it is assigned to
> malloc'd memory, then you leak that memory when freeing tpm_pt.
Yes, i agree there is memory leak here, i will fix it.
- Amarnath
[Qemu-devel] [Bug 1703506] Re: SMT not supported by QEMU on AMD Ryzen CPU
I tried disabling the CmpLegacy bit directly on /target/i386/cpu.c deleting the If statement on "case 0x8001:" or changing "*ecx |= 1 << 1;" to "*ecx |= 0 << 1;" But it didn't work, the VM still sees 8 physical cores. I believe the HTT bit should be enabled by default I tried changing it to "*edx |= 1 << 28;" in the If statement of "case 1:" just in case but it didn't matter. Anything else that I could try to hard-code for testing? -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1703506 Title: SMT not supported by QEMU on AMD Ryzen CPU Status in QEMU: New Bug description: HyperThreading/SMT is supported by AMD Ryzen CPUs but results in this message when setting the topology to threads=2: qemu-system-x86_64: AMD CPU doesn't support hyperthreading. Please configure -smp options properly. Checking in a Windows 10 guest reveals that SMT is not enabled, and from what I understand, QEMU converts the topology from threads to cores internally on AMD CPUs. This appears to cause performance problems in the guest perhaps because programs are assuming that these threads are actual cores. Software: Linux 4.12, qemu 2.9.0 host with KVM enabled, Windows 10 pro guest To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1703506/+subscriptions
Re: [Qemu-devel] [PATCH] pc: acpi: force FADT rev1 for old i440fx machine types
> On Jul 21, 2017, at 7:40 PM, Michael S. Tsirkin wrote: > > On Fri, Jul 21, 2017 at 12:10:48PM +0200, Igor Mammedov wrote: >> On Fri, 21 Jul 2017 10:49:55 +0100 >> "Daniel P. Berrange" wrote: >> >>> On Fri, Jul 21, 2017 at 11:32:11AM +0200, Igor Mammedov wrote: w2k used to boot on QEMU until we bumped revision of FADT to rev3 (commit 77af8a2b hw/i386: Use Rev3 FADT (ACPI 2.0) instead of Rev1 to improve guest OS support.) Considering that w2k is ancient and long time EOLed, leave default rev3 but make pc-i440fx-2.9 and older machine types to force rev1 so old setups won't break (w2k could boot). >>> >>> There needs to be a machine type property added to control this >>> feature. When provisioning new VMs, management apps need to be >>> able to set the property explicitly - having them rely on picking >>> particular machine type name+versions is not viable, because >>> downstream vendors replace the machine types with their own >>> names + versions. >> having property doesn't really help here and we don't do it for every >> compat tweak /ex: save_tsc_khz, linuxboot_dma_enabled/. >> >> Management would not benefit much from having property vs machine version >> as it would have to encode somewhere that for w2k it should set >> some machine property or pick a particular machine type. > > I think I'd disagree with that. If > users might need this for compatibility with some guests, > then it should be a property not just a machine type. > > But see below - I think we rushed it for the PC anyway. > >> Probably no one would worry about fixing virt-install or something >> else for the sake of w2k and if they are going to fix it >> it doesn't matter if they map machine type vs property. >> >> Also with new machine type deprecation policy we would be able >> easily to phase out rev1 support along with 2.9 machine, >> but if you expose property then removing it would break >> CLI not only for 2.9 but possible later machines if it's set there. >> >> So I'm against adding properties/CLI options for unless we have to in this >> case, >> and I'm not convinced that w2k deserves it. > > If I have to choose, I'd say Mac OSX is way less interesting than old > windows versions. Lots of people have software that will only run on old > windows and there's probably good money to be had running it on new > hardware in VMs. And PC machine is all about compatibility - we have Q35 > for new stuff. Besides OSX uses q35 anyway I think. > > So maybe the right thing to do is to > - switch default for PC back to rev 1 > - keep default for Q35 at rev 3 > > No machinetype hacks. I agree with your ideas.
Re: [Qemu-devel] [PATCH 08/11] qmp.py: Avoid "has_key" usage
On 07/20/2017 01:28 PM, Lukáš Doktor wrote:
The "has_key" is deprecated in favor of "__in__" operator.
Signed-off-by: Lukáš Doktor
Reviewed-by: Philippe Mathieu-Daudé
---
scripts/qmp/qmp.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 68f3420..a14b001 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -197,7 +197,7 @@ class QEMUMonitorProtocol(object):
Build and send a QMP command to the monitor, report errors when any
"""
ret = self.cmd(cmd, kwds)
-if ret.has_key('error'):
+if "error" in ret:
raise Exception(ret['error']['desc'])
return ret['return']
Re: [Qemu-devel] [PATCH 06/11] qmp.py: Couple of pylint/style fixes
Hi Lukáš,
Since comment/indent fixes and code changes are not related I'd rather
see this split in at least 2 patches.
On 07/20/2017 01:28 PM, Lukáš Doktor wrote:
No actual code changes, just a few pylint/style fixes and docstring
clarifications.
Signed-off-by: Lukáš Doktor
---
scripts/qmp/qmp.py | 37 -
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 62d3651..bb4d614 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -13,19 +13,30 @@ import errno
import socket
import sys
+
class QMPError(Exception):
pass
+
class QMPConnectError(QMPError):
pass
+
class QMPCapabilitiesError(QMPError):
pass
+
class QMPTimeoutError(QMPError):
pass
+
class QEMUMonitorProtocol:
+
+#: Socket's error class
+error = socket.error
+#: Socket's timeout
+timeout = socket.timeout
+
ok
def __init__(self, address, server=False, debug=False):
"""
Create a QEMUMonitorProtocol class.
@@ -42,6 +53,7 @@ class QEMUMonitorProtocol:
self.__address = address
self._debug = debug
self.__sock = self.__get_sock()
+self.__sockfile = None
if server:
self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
self.__sock.bind(self.__address)
@@ -56,7 +68,7 @@ class QEMUMonitorProtocol:
def __negotiate_capabilities(self):
greeting = self.__json_read()
-if greeting is None or not greeting.has_key('QMP'):
+if greeting is None or "QMP" not in greeting:
ok
raise QMPConnectError
# Greeting seems ok, negotiate capabilities
resp = self.cmd('qmp_capabilities')
@@ -78,8 +90,6 @@ class QEMUMonitorProtocol:
continue
return resp
-error = socket.error
-
def __get_events(self, wait=False):
"""
Check for new events in the stream and cache them in __events.
@@ -89,8 +99,8 @@ class QEMUMonitorProtocol:
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
"""
# Check for new events regardless and pull them into the cache:
@@ -175,7 +185,7 @@ class QEMUMonitorProtocol:
@param args: command arguments (dict)
@param id: command id (dict, list, string or int)
"""
-qmp_cmd = { 'execute': name }
+qmp_cmd = {'execute': name}
if args:
qmp_cmd['arguments'] = args
if id:
@@ -183,6 +193,9 @@ class QEMUMonitorProtocol:
return self.cmd_obj(qmp_cmd)
def command(self, cmd, **kwds):
+"""
+Build and send a QMP command to the monitor, report errors when any
I'm not native english speaker but I think "if any" sounds better.
+"""
ret = self.cmd(cmd, kwds)
if ret.has_key('error'):
raise Exception(ret['error']['desc'])
@@ -190,15 +203,15 @@ class QEMUMonitorProtocol:
def pull_event(self, wait=False):
"""
-Get and delete the first available QMP event.
+Get and pop the first available QMP event.
Both sentences seems unclear to me, regarding the function name... I
wonder if removing this comment makes this function clearer.
@param wait (bool): block until an event is available.
@param wait (float): If wait is a float, treat it as a timeout value.
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
@return The first available QMP event, or None.
"""
@@ -217,8 +230,8 @@ class QEMUMonitorProtocol:
@raise QMPTimeoutError: If a timeout float is provided and the timeout
period elapses.
-@raise QMPConnectError: If wait is True but no events could be
retrieved
-or if some other error occurred.
+@raise QMPConnectError: If wait is True but no events could be
+retrieved or if some other error occurred.
@return The list of available QMP events.
"""
@@ -235,8 +248,6 @@ class QEMUMonitorProtocol:
s
Re: [Qemu-devel] [PATCH 03/11] qemu.py: Use iteritems rather than keys()
On 07/20/2017 01:28 PM, Lukáš Doktor wrote: Let's avoid creating an in-memory list of keys and query for each value and use `iteritems` which is an iterator of key-value pairs. Signed-off-by: Lukáš Doktor Reviewed-by: Philippe Mathieu-Daudé --- scripts/qemu.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index 66fd863..7e95c25 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -185,11 +185,11 @@ class QEMUMachine(object): def qmp(self, cmd, conv_keys=True, **args): '''Invoke a QMP command and return the result dict''' qmp_args = dict() -for key in args.keys(): +for key, value in args.iteritems(): if conv_keys: -qmp_args[key.translate(self.underscore_to_dash)] = args[key] +qmp_args[key.translate(self.underscore_to_dash)] = value else: -qmp_args[key] = args[key] +qmp_args[key] = value return self._qmp.cmd(cmd, args=qmp_args)
Re: [Qemu-devel] [PATCH 02/11] qemu.py: Avoid dangerous arguments
On 07/20/2017 01:28 PM, Lukáš Doktor wrote:
The list object is mutable in python and potentially might modify other
object's arguments when used as default argument. Reproducer:
>>> vm1 = QEMUMachine("qemu")
>>> vm2 = QEMUMachine("qemu")
>>> vm1._wrapper.append("foo")
>>> print vm2._wrapper
['foo']
In this case the `args` is actually copied so it would be safe to keep
it, but it's not a good practice to keep it.
Signed-off-by: Lukáš Doktor
Reviewed-by: Philippe Mathieu-Daudé
---
scripts/qemu.py | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 191c916..66fd863 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -23,7 +23,7 @@ import qmp.qmp
class QEMUMachine(object):
'''A QEMU VM'''
-def __init__(self, binary, args=[], wrapper=[], name=None,
+def __init__(self, binary, args=None, wrapper=None, name=None,
test_dir="/var/tmp", monitor_address=None,
socket_scm_helper=None, debug=False):
'''
@@ -39,6 +39,10 @@ class QEMUMachine(object):
@param debug: enable debug mode (forwarded to QMP helper and such)
@note: Qemu process is not started until launch() is used.
'''
+if args is None:
+args = []
+if wrapper is None:
+wrapper = []
if name is None:
name = "qemu-%d" % os.getpid()
if monitor_address is None:
[Qemu-devel] [PULL for-2.10 1/2] xen: fix compilation on 32-bit hosts
From: Igor Druzhinin
Signed-off-by: Igor Druzhinin
Reviewed-by: Stefano Stabellini
---
hw/i386/xen/xen-mapcache.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index 2a1fbd1..bb1078c 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -527,7 +527,7 @@ static uint8_t *xen_replace_cache_entry_unlocked(hwaddr
old_phys_addr,
entry = entry->next;
}
if (!entry) {
-DPRINTF("Trying to update an entry for %lx " \
+DPRINTF("Trying to update an entry for "TARGET_FMT_plx \
"that is not in the mapcache!\n", old_phys_addr);
return NULL;
}
@@ -535,15 +535,16 @@ static uint8_t *xen_replace_cache_entry_unlocked(hwaddr
old_phys_addr,
address_index = new_phys_addr >> MCACHE_BUCKET_SHIFT;
address_offset = new_phys_addr & (MCACHE_BUCKET_SIZE - 1);
-fprintf(stderr, "Replacing a dummy mapcache entry for %lx with %lx\n",
-old_phys_addr, new_phys_addr);
+fprintf(stderr, "Replacing a dummy mapcache entry for "TARGET_FMT_plx \
+" with "TARGET_FMT_plx"\n", old_phys_addr, new_phys_addr);
xen_remap_bucket(entry, entry->vaddr_base,
cache_size, address_index, false);
if (!test_bits(address_offset >> XC_PAGE_SHIFT,
test_bit_size >> XC_PAGE_SHIFT,
entry->valid_mapping)) {
-DPRINTF("Unable to update a mapcache entry for %lx!\n", old_phys_addr);
+DPRINTF("Unable to update a mapcache entry for "TARGET_FMT_plx"!\n",
+old_phys_addr);
return NULL;
}
--
1.9.1
[Qemu-devel] [PULL for-2.10 2/2] xen-mapcache: Fix the bug when overlapping emulated DMA operations may cause inconsistency in guest memory mappings
From: Alexey G Under certain circumstances normal xen-mapcache functioning may be broken by guest's actions. This may lead to either QEMU performing exit() due to a caught bad pointer (and with QEMU process gone the guest domain simply appears hung afterwards) or actual use of the incorrect pointer inside QEMU address space -- a write to unmapped memory is possible. The bug is hard to reproduce on a i440 machine as multiple DMA sources are required (though it's possible in theory, using multiple emulated devices), but can be reproduced somewhat easily on a Q35 machine using an emulated AHCI controller -- each NCQ queue command slot may be used as an independent DMA source ex. using READ FPDMA QUEUED command, so a single storage device on the AHCI controller port will be enough to produce multiple DMAs (up to 32). The detailed description of the issue follows. Xen-mapcache provides an ability to map parts of a guest memory into QEMU's own address space to work with. There are two types of cache lookups: - translating a guest physical address into a pointer in QEMU's address space, mapping a part of guest domain memory if necessary (while trying to reduce a number of such (re)mappings to a minimum) - translating a QEMU's pointer back to its physical address in guest RAM These lookups are managed via two linked-lists of structures. MapCacheEntry is used for forward cache lookups, while MapCacheRev -- for reverse lookups. Every guest physical address is broken down into 2 parts: address_index = phys_addr >> MCACHE_BUCKET_SHIFT; address_offset = phys_addr & (MCACHE_BUCKET_SIZE - 1); MCACHE_BUCKET_SHIFT depends on a system (32/64) and is equal to 20 for a 64-bit system (which assumed for the further description). Basically, this means that we deal with 1 MB chunks and offsets within those 1 MB chunks. All mappings are created with 1MB-granularity, i.e. 1MB/2MB/3MB etc. Most DMA transfers typically are less than 1MB, however, if the transfer crosses any 1MB border(s) - than a nearest larger mapping size will be used, so ex. a 512-byte DMA transfer with the start address 700FFF80h will actually require a 2MB range. Current implementation assumes that MapCacheEntries are unique for a given address_index and size pair and that a single MapCacheEntry may be reused by multiple requests -- in this case the 'lock' field will be larger than 1. On other hand, each requested guest physical address (with 'lock' flag) is described by each own MapCacheRev. So there may be multiple MapCacheRev entries corresponding to a single MapCacheEntry. The xen-mapcache code uses MapCacheRev entries to retrieve the address_index & size pair which in turn used to find a related MapCacheEntry. The 'lock' field within a MapCacheEntry structure is actually a reference counter which shows a number of corresponding MapCacheRev entries. The bug lies in ability for the guest to indirectly manipulate with the xen-mapcache MapCacheEntries list via a special sequence of DMA operations, typically for storage devices. In order to trigger the bug, guest needs to issue DMA operations in specific order and timing. Although xen-mapcache is protected by the mutex lock -- this doesn't help in this case, as the bug is not due to a race condition. Suppose we have 3 DMA transfers, namely A, B and C, where - transfer A crosses 1MB border and thus uses a 2MB mapping - transfers B and C are normal transfers within 1MB range - and all 3 transfers belong to the same address_index In this case, if all these transfers are to be executed one-by-one (without overlaps), no special treatment necessary -- each transfer's mapping lock will be set and then cleared on unmap before starting the next transfer. The situation changes when DMA transfers overlap in time, ex. like this: |= transfer A (2MB) =| |= transfer B (1MB) =| |= transfer C (1MB) =| time ---> In this situation the following sequence of actions happens: 1. transfer A creates a mapping to 2MB area (lock=1) 2. transfer B (1MB) tries to find available mapping but cannot find one because transfer A is still in progress, and it has 2MB size + non-zero lock. So transfer B creates another mapping -- same address_index, but 1MB size. 3. transfer A completes, making 1st mapping entry available by setting its lock to 0 4. transfer C starts and tries to find available mapping entry and sees that 1st entry has lock=0, so it uses this entry but remaps the mapping to a 1MB size 5. transfer B completes and by this time - there are two locked entries in the MapCacheEntry list with the SAME values for both address_index and size - the entry for transfer B actually resides farther in list while transfer C's entry is first 6. xen_ram_addr_from_mapcache() for transfer B gets correct address_index and size pair from corresponding MapCacheRev entry, but then it starts looking for MapCacheEntry with
[Qemu-devel] [PULL for-2.10 0/2] please pull xen-20170721-tag
The following changes since commit 91939262ffcd3c85ea6a4793d3029326eea1d649: configure: Drop ancient Solaris 9 and earlier support (2017-07-21 15:04:05 +0100) are available in the git repository at: git://xenbits.xen.org/people/sstabellini/qemu-dm.git tags/xen-20170721-tag for you to fetch changes up to 7fb394ad8a7c4609cefa2136dec16cf65d028f40: xen-mapcache: Fix the bug when overlapping emulated DMA operations may cause inconsistency in guest memory mappings (2017-07-21 17:37:06 -0700) Xen 2017/07/21 Alexey G (1): xen-mapcache: Fix the bug when overlapping emulated DMA operations may cause inconsistency in guest memory mappings Igor Druzhinin (1): xen: fix compilation on 32-bit hosts hw/i386/xen/xen-mapcache.c | 22 -- 1 file changed, 16 insertions(+), 6 deletions(-)
Re: [Qemu-devel] [PATCH] vhost-user: fix watcher need be removed when vhost-user hotplug
On Fri, Jul 21, 2017 at 11:19:04AM +, Marc-André Lureau wrote:
> Hi
>
> On Fri, Jul 21, 2017 at 7:18 AM w00273186 wrote:
>
> From: Yunjian Wang
>
> "nc" is freed after hotplug vhost-user, but the watcher don't be removed.
> The QEMU crash when the watcher access the "nc" on socket disconnect.
>
>
>
> This is actually your 3rd iteration on the patch
>
> Could your describe your changes since:
> "[PATCH v2] vhost-user: fix watcher need be removed when vhost-user hotplug"
>
> Thanks
Yes but it's a 3-liner. That's way below the limit where you need
detailed change history. Does the patch make sense to you?
>
> Program received signal SIGSEGV, Segmentation fault.
> #0 object_get_class (obj=obj@entry=0x2) at qom/object.c:750
> #1 0x7f9bb4180da1 in qemu_chr_fe_disconnect (be=)
> at chardev/char-fe.c:372
> #2 0x7f9bb40d1100 in net_vhost_user_watch (chan=,
> cond=, opaque=) at net/vhost-user.c:188
> #3 0x7f9baf97f99a in g_main_context_dispatch () from /usr/lib64/
> libglib-2.0.so.0
> #4 0x7f9bb41d7ebc in glib_pollfds_poll () at util/main-loop.c:213
> #5 os_host_main_loop_wait (timeout=) at util/
> main-loop.c:261
> #6 main_loop_wait (nonblocking=nonblocking@entry=0) at util/
> main-loop.c:515
> #7 0x7f9bb3e266a7 in main_loop () at vl.c:1917
> #8 main (argc=, argv=, envp= out>) at vl.c:4786
>
> Signed-off-by: Yunjian Wang
> ---
> net/vhost-user.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 36f32a2..c23927c 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -151,6 +151,10 @@ static void vhost_user_cleanup(NetClientState *nc)
> s->vhost_net = NULL;
> }
> if (nc->queue_index == 0) {
> + if (s->watch) {
> + g_source_remove(s->watch);
> + s->watch = 0;
> + }
> qemu_chr_fe_deinit(&s->chr, true);
> }
>
> --
> 1.8.3.1
>
>
>
>
> --
> Marc-André Lureau
Re: [Qemu-devel] [PATCH v14 33/34] target/arm: Split out thumb_tr_translate_insn
On Fri, Jul 14, 2017 at 23:42:42 -1000, Richard Henderson wrote:
> We need not check for ARM vs Thumb state in order to dispatch
> disassembly of every instruction.
>
> Signed-off-by: Richard Henderson
> ---
> target/arm/translate.c | 134
> +++--
> 1 file changed, 86 insertions(+), 48 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index ebe1c1a..d7c3c10 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
(snip)
> +static void arm_post_translate_insn(CPUARMState *env, DisasContext *dc)
> +{
> +/* Translation stops when a conditional branch is encountered.
> + * Otherwise the subsequent code could get translated several times.
> + * Also stop translation when a page boundary is reached. This
> + * ensures prefetch aborts occur at the right place.
> + *
> + * We want to stop the TB if the next insn starts in a new page,
> + * or if it spans between this page and the next. This means that
> + * if we're looking at the last halfword in the page we need to
> + * see if it's a 16-bit Thumb insn (which will fit in this TB)
> + * or a 32-bit Thumb insn (which won't).
> + * This is to avoid generating a silly TB with a single 16-bit insn
> + * in it at the end of this page (which would execute correctly
> + * but isn't very efficient).
> + */
> +if (dc->base.is_jmp == DISAS_NEXT
> +&& (dc->pc >= dc->next_page_start
> +|| (dc->pc >= dc->next_page_start - 3
> +&& insn_crosses_page(env, dc {
> +dc->base.is_jmp = DISAS_TOO_MANY;
> }
>
> if (dc->condjmp && !dc->base.is_jmp) {
> gen_set_label(dc->condlabel);
> dc->condjmp = 0;
> }
> +dc->base.pc_next = dc->pc;
> +translator_loop_temp_check(&dc->base);
> +}
>
> -if (dc->base.is_jmp == DISAS_NEXT) {
> -/* Translation stops when a conditional branch is encountered.
> - * Otherwise the subsequent code could get translated several times.
> - * Also stop translation when a page boundary is reached. This
> - * ensures prefetch aborts occur at the right place. */
> -
> -if (dc->pc >= dc->next_page_start ||
> -(dc->pc >= dc->next_page_start - 3 &&
> - insn_crosses_page(env, dc))) {
> -/* We want to stop the TB if the next insn starts in a new page,
> - * or if it spans between this page and the next. This means that
> - * if we're looking at the last halfword in the page we need to
> - * see if it's a 16-bit Thumb insn (which will fit in this TB)
> - * or a 32-bit Thumb insn (which won't).
> - * This is to avoid generating a silly TB with a single 16-bit
> insn
> - * in it at the end of this page (which would execute correctly
> - * but isn't very efficient).
> - */
> -dc->base.is_jmp = DISAS_TOO_MANY;
Note that we've moved the above hunk ("if (dc->base.is_jmp == DISAS_NEXT)")
above the "if (dc->condjmp && !dc->base.is_jmp)" one; so when we now set
is_jmp to DISAS_TOO_MANY, dc->condjmp might be wrongly cleared by the
second hunk.
My review missed this, but luckily TCG asserts caught it while booting
debian-arm:
[ OK ] Started Increase datagram queue length.
systemd[1]: Started Increase datagram queue length.
[ OK ] Mounted POSIX Message Queue File System.
systemd[1]: Mounted POSIX Message Queue File System.
qemu-system-arm: /data/src/qemu/tcg/tcg-op.c:2584: tcg_gen_goto_tb: Assertion
`(tcg_ctx.goto_tb_issue_mask & (1 << idx)) == 0' failed.
Aborted (core dumped)
Keeping the original order fixes it.
Emilio
--8<--
---
target/arm/translate.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 77fe6a9..9cbace5 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -11979,6 +11979,11 @@ static bool arm_pre_translate_insn(DisasContext *dc)
static void arm_post_translate_insn(CPUARMState *env, DisasContext *dc)
{
+if (dc->condjmp && !dc->base.is_jmp) {
+gen_set_label(dc->condlabel);
+dc->condjmp = 0;
+}
+
/* Translation stops when a conditional branch is encountered.
* Otherwise the subsequent code could get translated several times.
* Also stop translation when a page boundary is reached. This
@@ -12000,10 +12005,6 @@ static void arm_post_translate_insn(CPUARMState *env,
DisasContext *dc)
dc->base.is_jmp = DISAS_TOO_MANY;
}
-if (dc->condjmp && !dc->base.is_jmp) {
-gen_set_label(dc->condlabel);
-dc->condjmp = 0;
-}
dc->base.pc_next = dc->pc;
translator_loop_temp_check(&dc->base);
}
--
2.7.4
Re: [Qemu-devel] [PULL for-2.10 6/7] xen/mapcache: introduce xen_replace_cache_entry()
On Fri, 21 Jul 2017, Igor Druzhinin wrote:
> On 21/07/17 14:50, Anthony PERARD wrote:
> > On Tue, Jul 18, 2017 at 03:22:41PM -0700, Stefano Stabellini wrote:
> > > From: Igor Druzhinin
> >
> > ...
> >
> > > +static uint8_t *xen_replace_cache_entry_unlocked(hwaddr old_phys_addr,
> > > + hwaddr new_phys_addr,
> > > + hwaddr size)
> > > +{
> > > +MapCacheEntry *entry;
> > > +hwaddr address_index, address_offset;
> > > +hwaddr test_bit_size, cache_size = size;
> > > +
> > > +address_index = old_phys_addr >> MCACHE_BUCKET_SHIFT;
> > > +address_offset = old_phys_addr & (MCACHE_BUCKET_SIZE - 1);
> > > +
> > > +assert(size);
> > > +/* test_bit_size is always a multiple of XC_PAGE_SIZE */
> > > +test_bit_size = size + (old_phys_addr & (XC_PAGE_SIZE - 1));
> > > +if (test_bit_size % XC_PAGE_SIZE) {
> > > +test_bit_size += XC_PAGE_SIZE - (test_bit_size % XC_PAGE_SIZE);
> > > +}
> > > +cache_size = size + address_offset;
> > > +if (cache_size % MCACHE_BUCKET_SIZE) {
> > > +cache_size += MCACHE_BUCKET_SIZE - (cache_size %
> > > MCACHE_BUCKET_SIZE);
> > > +}
> > > +
> > > +entry = &mapcache->entry[address_index % mapcache->nr_buckets];
> > > +while (entry && !(entry->paddr_index == address_index &&
> > > + entry->size == cache_size)) {
> > > +entry = entry->next;
> > > +}
> > > +if (!entry) {
> > > +DPRINTF("Trying to update an entry for %lx " \
> > > +"that is not in the mapcache!\n", old_phys_addr);
> > > +return NULL;
> > > +}
> > > +
> > > +address_index = new_phys_addr >> MCACHE_BUCKET_SHIFT;
> > > +address_offset = new_phys_addr & (MCACHE_BUCKET_SIZE - 1);
> > > +
> > > +fprintf(stderr, "Replacing a dummy mapcache entry for %lx with
> > > %lx\n",
> > > +old_phys_addr, new_phys_addr);
> >
> > Looks likes this does not build on 32bits.
> > in:
> > http://logs.test-lab.xenproject.org/osstest/logs/112041/build-i386/6.ts-xen-build.log
> >
> > /home/osstest/build.112041.build-i386/xen/tools/qemu-xen-dir/hw/i386/xen/xen-mapcache.c:
> > In function 'xen_replace_cache_entry_unlocked':
> > /home/osstest/build.112041.build-i386/xen/tools/qemu-xen-dir/hw/i386/xen/xen-mapcache.c:539:13:
> > error: format '%lx' expects argument of type 'long unsigned int', but
> > argument 3 has type 'hwaddr' [-Werror=format=]
> > old_phys_addr, new_phys_addr);
> > ^
> > /home/osstest/build.112041.build-i386/xen/tools/qemu-xen-dir/hw/i386/xen/xen-mapcache.c:539:13:
> > error: format '%lx' expects argument of type 'long unsigned int', but
> > argument 4 has type 'hwaddr' [-Werror=format=]
> > cc1: all warnings being treated as errors
> >CC i386-softmmu/target/i386/gdbstub.o
> > /home/osstest/build.112041.build-i386/xen/tools/qemu-xen-dir/rules.mak:66:
> > recipe for target 'hw/i386/xen/xen-mapcache.o' failed
> >
> > > +
> > > +xen_remap_bucket(entry, entry->vaddr_base,
> > > + cache_size, address_index, false);
> > > +if (!test_bits(address_offset >> XC_PAGE_SHIFT,
> > > +test_bit_size >> XC_PAGE_SHIFT,
> > > +entry->valid_mapping)) {
> > > +DPRINTF("Unable to update a mapcache entry for %lx!\n",
> > > old_phys_addr);
> > > +return NULL;
> > > +}
> > > +
> > > +return entry->vaddr_base + address_offset;
> > > +}
> > > +
> >
>
> Please, accept the attached patch to fix the issue.
The patch looks good to me. I'll send it upstream.
Re: [Qemu-devel] [PATCH v14 30/34] target/arm: [tcg] Port to generic translation framework
On Fri, Jul 21, 2017 at 19:02:58 -0400, Emilio G. Cota wrote: > On Fri, Jul 14, 2017 at 23:42:39 -1000, Richard Henderson wrote: > > From: Lluís Vilanova > > > > Signed-off-by: Lluís Vilanova > > Message-Id: <150002631325.22386.10348327185029496649.st...@frigg.lan> > > Signed-off-by: Richard Henderson > > --- > > target/arm/translate.h | 8 +--- > > target/arm/translate-a64.c | 107 > > --- > > target/arm/translate.c | 110 > > ++--- > > 3 files changed, 42 insertions(+), 183 deletions(-) > > (snip) > > diff --git a/target/arm/translate.c b/target/arm/translate.c > > index 4ea5f70..4b1230b 100644 > > --- a/target/arm/translate.c > > +++ b/target/arm/translate.c > > @@ -11897,7 +11897,9 @@ static void arm_tr_tb_start(DisasContextBase > > *dcbase, CPUState *cpu) > > TCGv_i32 tmp = tcg_temp_new_i32(); > > tcg_gen_movi_i32(tmp, 0); > > store_cpu_field(tmp, condexec_bits); > > +tcg_temp_free_i32(tmp); > > This seems unrelated to the patch. Perhaps a better place to add > this fix would be patch 20, with a mention to it in the commit log. > Or just keep it here, but mention it in the commit log. Silly me. This must go away, because store_cpu_field already frees the temp. E.
Re: [Qemu-devel] >256 Virtio-net-pci hotplug Devices
Hi Marcel On 21/07/2017 01:33, Marcel Apfelbaum wrote: On 20/07/2017 3:44, Kinsella, Ray wrote: That's strange. Please ensure the virtio devices are working in virtio 1.0 mode (disable-modern=0,disable-legacy=1). Let us know any problems you see. Not sure what yet, I will try scaling it with hotplugging tomorrow. Updates? I have managed to scale it to 128 devices. The kernel does complain about IO address space exhaustion. [ 83.697956] pci :80:00.0: BAR 13: no space for [io size 0x1000] [ 83.700958] pci :80:00.0: BAR 13: failed to assign [io size 0x1000] [ 83.701689] pci :80:00.1: BAR 13: no space for [io size 0x1000] [ 83.702378] pci :80:00.1: BAR 13: failed to assign [io size 0x1000] [ 83.703093] pci :80:00.2: BAR 13: no space for [io size 0x1000] I was surprised that I am running out of IO address space, as I am disabling legacy virtio. I assumed that this would remove the need for SeaBIOS to allocate the PCI Express Root Port IO address space. In any case - it doesn't stop the virtio-net device coming up and working as expected. [ 668.692081] virtio_net virtio103 enp141s0f4: renamed from eth101 [ 668.707114] virtio_net virtio130 enp144s0f7: renamed from eth128 [ 668.719795] virtio_net virtio129 enp144s0f6: renamed from eth127 I encountered some issues in vhost, due to open file exhaustion but resolved these with 'ulimit' in the usual way - burned alot of time on that today. When scaling up to 512 Virtio-net devices SeaBIOS appears to really slow down when configuring PCI Config space - haven't manage to get this to work yet. Not really. All you have to do is to add a property to the pxb-pci/pxb devices: pci_domain=x; then update the ACPI table to include the pxb domain. You also have to tweak a little the pxb-pcie/pxb devices to not share the bus numbers if pci_domain > 0. Thanks for information, will add to the list. Ray K \
Re: [Qemu-devel] 答复: Re: [PATCH] vhost: fix a migration failed because ofvhost region merge
On Thu, Jul 20, 2017 at 10:57:57AM +0800, peng.h...@zte.com.cn wrote:
> 原始邮件
> 发件人： ;
> 收件人： ;
> 抄送人： ; ;
> ;彭浩10096742;王业超10154425;
> ;
> 日期：2017年07月19日 23:53
> 主题：Re: [Qemu-devel] [PATCH] vhost: fix a migration failed because ofvhost
> region merge
>
>
> On Wed, Jul 19, 2017 at 03:24:27PM +0200, Igor Mammedov wrote:
> > On Wed, 19 Jul 2017 12:46:13 +0100
> > "Dr. David Alan Gilbert" wrote:
> >
> > > * Igor Mammedov (imamm...@redhat.com) wrote:
> > > > On Wed, 19 Jul 2017 23:17:32 +0800
> > > > Peng Hao wrote:
> > > >
> > > > > When a guest that has several hotplugged dimms is migrated, in
> > > > > destination host it will fail to resume. Because vhost regions of
> > > > > several dimms in source host are merged and in the restore stage
> > > > > in destination host it computes whether more than vhost slot limit
> > > > > before merging vhost regions of several dimms.
> > > > could you provide a bit more detailed description of the problem
> > > > including command line+used device_add commands on source and
> > > > command line on destination?
> > >
> > > (ccing in Marc Andre and Maxime)
> > >
> > > Hmm, I'd like to understade the situation where you get merging between
> > > RAMBlocks; that complicates some stuff for postcopy.
> > and probably inconsistent merging breaks vhost as well
> >
> > merging might happen if regions are adjacent or overlap
> > but for that to happen merged regions must have equal
> > distance between their GPA:HVA pairs, so that following
> > translation would work:
> >
> > if gva in regionX[gva_start, len, hva_start]
> >hva = hva_start + gva - gva_start
> >
> > while GVA of regions is under QEMU control and deterministic
> > HVA is not, so in migration case merging might happen on source
> > side but not on destination, resulting in different memory maps.
> >
> > Maybe Michael might know details why migration works in vhost usecase,
> > but I don't see vhost sending any vmstate data.
>
> We aren't merging ramblocks at all.
> When we are passing blocks A and B to vhost, if we see that
>
> hvaB=hvaA + lenA
> gpaB=gpaA + lenA
>
> then we can improve performance a bit by passing a single
> chunk to vhost: hvaA,gpaA,lena+lenB
>
>
> so it does not affect migration normally.
>
> - I think it is like this:
>
> in source; in destination:(restore)
>
> realize device 1 realize device 1
>
> realize device 2 realize dimm 0
>
> ... realize dimm1
>
>
>
> realize device n realize dimmx
>
>realize device m
>
> realize dimm0 .
>
> realize dimm1 .
>
> .. .
>
> realize dimmxrealize device n
>
>
> In restore stage ,the sort of realizing device is different from starting vm
> because of adding dimms.
>
> So it may in some stage during restoring can't merge vhost regions.
If you run over the number of regions supported by vhost on destination
then you won't be able to start a VM there until you disable vhost.
>
>
>
>
>
> >
> > >
> > > > >
> > > > > Signed-off-by: Peng Hao
> > > > > Signed-off-by: Wang Yechao
> > > > > ---
> > > > > hw/mem/pc-dimm.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > > > > index ea67b46..bb0fa08 100644
> > > > > --- a/hw/mem/pc-dimm.c
> > > > > +++ b/hw/mem/pc-dimm.c
> > > > > @@ -101,7 +101,7 @@ void pc_dimm_memory_plug
> (DeviceState *dev, MemoryHotplugState *hpms,
> > > > > goto out;
> > > > > }
> > > > >
> > > > > -if (!vhost_has_free_slot()) {
> > > > > +if (!vhost_has_free_slot() && runstate_is_running()) {
> > > > > error_setg(&local_err, "a used vhost backend has no free"
> > > > > " memory slots left");
> > > > > goto out;
> > >
> > > Even this produces the wrong error message in this case,
> > > it also makes me think if the existing code should undo a lot of
> > > the object_property_set's that happen.
> > >
> > > Dave
> > > >
> > > >
> > > --
> > > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
>
>
>
Re: [Qemu-devel] [PATCH] pc: acpi: force FADT rev1 for old i440fx machine types
On Fri, Jul 21, 2017 at 12:10:48PM +0200, Igor Mammedov wrote:
> On Fri, 21 Jul 2017 10:49:55 +0100
> "Daniel P. Berrange" wrote:
>
> > On Fri, Jul 21, 2017 at 11:32:11AM +0200, Igor Mammedov wrote:
> > > w2k used to boot on QEMU until we bumped revision of FADT to rev3
> > > (commit 77af8a2b hw/i386: Use Rev3 FADT (ACPI 2.0) instead of Rev1 to
> > > improve guest OS support.)
> > >
> > > Considering that w2k is ancient and long time EOLed, leave default
> > > rev3 but make pc-i440fx-2.9 and older machine types to force rev1
> > > so old setups won't break (w2k could boot).
> >
> > There needs to be a machine type property added to control this
> > feature. When provisioning new VMs, management apps need to be
> > able to set the property explicitly - having them rely on picking
> > particular machine type name+versions is not viable, because
> > downstream vendors replace the machine types with their own
> > names + versions.
> having property doesn't really help here and we don't do it for every
> compat tweak /ex: save_tsc_khz, linuxboot_dma_enabled/.
>
> Management would not benefit much from having property vs machine version
> as it would have to encode somewhere that for w2k it should set
> some machine property or pick a particular machine type.
I think I'd disagree with that. If
users might need this for compatibility with some guests,
then it should be a property not just a machine type.
But see below - I think we rushed it for the PC anyway.
> Probably no one would worry about fixing virt-install or something
> else for the sake of w2k and if they are going to fix it
> it doesn't matter if they map machine type vs property.
>
> Also with new machine type deprecation policy we would be able
> easily to phase out rev1 support along with 2.9 machine,
> but if you expose property then removing it would break
> CLI not only for 2.9 but possible later machines if it's set there.
>
> So I'm against adding properties/CLI options for unless we have to in this
> case,
> and I'm not convinced that w2k deserves it.
If I have to choose, I'd say Mac OSX is way less interesting than old
windows versions. Lots of people have software that will only run on old
windows and there's probably good money to be had running it on new
hardware in VMs. And PC machine is all about compatibility - we have Q35
for new stuff. Besides OSX uses q35 anyway I think.
So maybe the right thing to do is to
- switch default for PC back to rev 1
- keep default for Q35 at rev 3
No machinetype hacks.
> > >
> > > Signed-off-by: Igor Mammedov
> > > ---
> > > CC: Programmingkid
> > > CC: Phil Dennis-Jordan
> > > CC: "Michael S. Tsirkin"
> > >
> > > Only compile test since I don't have w2k to test with
> > >
> > > ---
> > > include/hw/i386/pc.h | 1 +
> > > hw/i386/acpi-build.c | 26 +++---
> > > hw/i386/pc_piix.c| 2 ++
> > > 3 files changed, 22 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index d80859b..d6f65dd 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -122,6 +122,7 @@ struct PCMachineClass {
> > > bool rsdp_in_ram;
> > > int legacy_acpi_table_size;
> > > unsigned acpi_data_size;
> > > +bool force_rev1_fadt;
> > >
> > > /* SMBIOS compat: */
> > > bool smbios_defaults;
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 6b7bade..227f9ad 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -272,7 +272,7 @@ build_facs(GArray *table_data, BIOSLinker *linker)
> > > }
> > >
> > > /* Load chipset information in FADT */
> > > -static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
> > > +static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm,
> > > bool rev1)
> > > {
> > > fadt->model = 1;
> > > fadt->reserved1 = 0;
> > > @@ -304,6 +304,9 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt,
> > > AcpiPmInfo *pm)
> > > fadt->flags |= cpu_to_le32(1 <<
> > > ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
> > > }
> > > fadt->century = RTC_CENTURY;
> > > +if (rev1) {
> > > +return;
> > > +}
> > >
> > > fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
> > > fadt->reset_value = 0xf;
> > > @@ -335,6 +338,7 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt,
> > > AcpiPmInfo *pm)
> > > /* FADT */
> > > static void
> > > build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
> > > + MachineState *machine,
> > > unsigned facs_tbl_offset, unsigned dsdt_tbl_offset,
> > > const char *oem_id, const char *oem_table_id)
> > > {
> > > @@ -342,6 +346,9 @@ build_fadt(GArray *table_data, BIOSLinker *linker,
> > > AcpiPmInfo *pm,
> > > unsigned fw_ctrl_offset = (char *)&fadt->firmware_ctrl -
> > > table_data->data;
> > > unsigned dsdt_entry_offset = (char *)&fadt->dsdt - ta
Re: [Qemu-devel] [PATCH v14 08/34] tcg: Add generic translation framework
On Fri, Jul 14, 2017 at 23:42:17 -1000, Richard Henderson wrote:
> From: Lluís Vilanova
>
> Signed-off-by: Lluís Vilanova
> Message-Id: <150002073981.22386.9870422422367410100.st...@frigg.lan>
> [rth: Moved max_insns adjustment from tb_start to init_disas_context.
> Removed pc_next return from translate_insn.
> Removed tcg_check_temp_count from generic loop.
> Moved gen_io_end to exactly match gen_io_start.
> Use qemu_log instead of error_report for temporary leaks.
> Moved TB size/icount assignments before disas_log.]
> Signed-off-by: Richard Henderson
(snip)
> +void translator_loop_temp_check(DisasContextBase *db)
> +{
> +if (tcg_check_temp_count()) {
> +qemu_log("warning: TCG temporary leaks before "
> + TARGET_FMT_lx "\n", db->pc_next);
> +}
> +}
I dislike that we have target code calling tcg_clear_temp_count()
and then calling translator_loop_temp_check().
I suggest we either:
- Add an inline wrapping tcg_clear_temp_count(), calling it something
like translator_loop_clear_temp_count()
- Just add a comment to this function, e.g.
'/* pairs with tcg_clear_temp_count() */'
Ignoring that and the update needed to the docs that Lluis mentioned,
Reviewed-by: Emilio G. Cota
E.
Re: [Qemu-devel] [PATCH v14 34/34] target/arm: Perform per-insn cross-page check only for Thumb
On Fri, Jul 14, 2017 at 23:42:43 -1000, Richard Henderson wrote: > ARM is a fixed-length ISA and we can compute the page crossing > condition exactly once during init_disas_context. > > Signed-off-by: Richard Henderson Reviewed-by: Emilio G. Cota E.
Re: [Qemu-devel] [PATCH v14 33/34] target/arm: Split out thumb_tr_translate_insn
On Fri, Jul 14, 2017 at 23:42:42 -1000, Richard Henderson wrote:
> We need not check for ARM vs Thumb state in order to dispatch
> disassembly of every instruction.
>
> Signed-off-by: Richard Henderson
> ---
> target/arm/translate.c | 134
> +++--
> 1 file changed, 86 insertions(+), 48 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index ebe1c1a..d7c3c10 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -11944,20 +11944,17 @@ static bool
> arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
> return true;
> }
>
> -static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
> +static bool arm_pre_translate_insn(DisasContext *dc)
> {
> -DisasContext *dc = container_of(dcbase, DisasContext, base);
> -CPUARMState *env = cpu->env_ptr;
> -
> #ifdef CONFIG_USER_ONLY
> -/* Intercept jump to the magic kernel page. */
> -if (dc->pc >= 0x) {
> -/* We always get here via a jump, so know we are not in a
> - conditional execution block. */
> -gen_exception_internal(EXCP_KERNEL_TRAP);
> -dc->base.is_jmp = DISAS_NORETURN;
> -return;
> -}
> +/* Intercept jump to the magic kernel page. */
> +if (dc->pc >= 0x) {
> +/* We always get here via a jump, so know we are not in a
> + conditional execution block. */
> +gen_exception_internal(EXCP_KERNEL_TRAP);
> +dc->base.is_jmp = DISAS_NORETURN;
> +return true;
> +}
See my comment in patch 24 about this.
Other than that, this is neat.
Reviewed-by: Emilio G. Cota
E.
Re: [Qemu-devel] [PATCH v14 24/34] target/arm: [tcg] Port to translate_insn
On Fri, Jul 14, 2017 at 23:42:33 -1000, Richard Henderson wrote:
> From: Lluís Vilanova
>
> Incrementally paves the way towards using the generic instruction translation
> loop.
>
> Signed-off-by: Lluís Vilanova
> Message-Id: <150002485863.22386.13949856269576226529.st...@frigg.lan>
> [rth: Adjust for translate_insn interface change.]
> Signed-off-by: Richard Henderson
(snip)
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -11842,6 +11842,8 @@ static int arm_tr_init_disas_context(DisasContextBase
> *dcbase,
> dc->is_ldex = false;
> dc->ss_same_el = false; /* Can't be true since EL_d must be AArch64 */
>
> +dc->next_page_start =
> +(dc->base.pc_first & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE;
>
> cpu_F0s = tcg_temp_new_i32();
> cpu_F1s = tcg_temp_new_i32();
> @@ -11935,14 +11937,93 @@ static bool
> arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
> return true;
> }
>
> +static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
> +{
> +DisasContext *dc = container_of(dcbase, DisasContext, base);
> +CPUARMState *env = cpu->env_ptr;
> +
> +#ifdef CONFIG_USER_ONLY
> +/* Intercept jump to the magic kernel page. */
> +if (dc->pc >= 0x) {
> +/* We always get here via a jump, so know we are not in a
> + conditional execution block. */
> +gen_exception_internal(EXCP_KERNEL_TRAP);
> +dc->base.is_jmp = DISAS_NORETURN;
> +return;
> +}
> +#endif
Nit: Indent this properly here to avoid the indent fix in patch 33.
E.
Re: [Qemu-devel] [PATCH v14 32/34] target/arm: Move ss check to init_disas_context
On Fri, Jul 14, 2017 at 23:42:41 -1000, Richard Henderson wrote: > We can check for single-step just once. > > Signed-off-by: Richard Henderson Reviewed-by: Emilio G. Cota E.
Re: [Qemu-devel] [PATCH v14 31/34] target/arm: [a64] Move page and ss checks to init_disas_context
On Fri, Jul 14, 2017 at 23:42:40 -1000, Richard Henderson wrote: > Since AArch64 uses a fixed-width ISA, we can pre-compute the number of > insns remaining on the page. Also, we can check for single-step once. > > Signed-off-by: Richard Henderson Reviewed-by: Emilio G. Cota Pity, if it wasn't for thumb, we could get rid of dc->next_page_start. E.
Re: [Qemu-devel] [PATCH v14 30/34] target/arm: [tcg] Port to generic translation framework
On Fri, Jul 14, 2017 at 23:42:39 -1000, Richard Henderson wrote: > From: Lluís Vilanova > > Signed-off-by: Lluís Vilanova > Message-Id: <150002631325.22386.10348327185029496649.st...@frigg.lan> > Signed-off-by: Richard Henderson > --- > target/arm/translate.h | 8 +--- > target/arm/translate-a64.c | 107 --- > target/arm/translate.c | 110 > ++--- > 3 files changed, 42 insertions(+), 183 deletions(-) (snip) > diff --git a/target/arm/translate.c b/target/arm/translate.c > index 4ea5f70..4b1230b 100644 > --- a/target/arm/translate.c > +++ b/target/arm/translate.c > @@ -11897,7 +11897,9 @@ static void arm_tr_tb_start(DisasContextBase *dcbase, > CPUState *cpu) > TCGv_i32 tmp = tcg_temp_new_i32(); > tcg_gen_movi_i32(tmp, 0); > store_cpu_field(tmp, condexec_bits); > +tcg_temp_free_i32(tmp); This seems unrelated to the patch. Perhaps a better place to add this fix would be patch 20, with a mention to it in the commit log. Or just keep it here, but mention it in the commit log. Otherwise, Reviewed-by: Emilio G. Cota
Re: [Qemu-devel] [PATCH v14 08/34] tcg: Add generic translation framework
Richard Henderson writes:
> From: Lluís Vilanova
> Signed-off-by: Lluís Vilanova
> Message-Id: <150002073981.22386.9870422422367410100.st...@frigg.lan>
> [rth: Moved max_insns adjustment from tb_start to init_disas_context.
> Removed pc_next return from translate_insn.
> Removed tcg_check_temp_count from generic loop.
> Moved gen_io_end to exactly match gen_io_start.
> Use qemu_log instead of error_report for temporary leaks.
> Moved TB size/icount assignments before disas_log.]
> Signed-off-by: Richard Henderson
> ---
> include/exec/translator.h | 101 +++
> accel/tcg/translator.c| 133
> ++
> accel/tcg/Makefile.objs | 1 +
> 3 files changed, 235 insertions(+)
> create mode 100644 accel/tcg/translator.c
> diff --git a/include/exec/translator.h b/include/exec/translator.h
> index b51b8f8..aa84376 100644
> --- a/include/exec/translator.h
> +++ b/include/exec/translator.h
> @@ -10,6 +10,19 @@
> #ifndef EXEC__TRANSLATOR_H
> #define EXEC__TRANSLATOR_H
> +/*
> + * Include this header from a target-specific file, and add a
> + *
> + * DisasContextBase base;
> + *
> + * member in your target-specific DisasContext.
> + */
> +
> +
> +#include "exec/exec-all.h"
> +#include "tcg/tcg.h"
> +
> +
> /**
> * DisasJumpType:
> * @DISAS_NEXT: Next instruction in program order.
> @@ -37,4 +50,92 @@ typedef enum DisasJumpType {
> DISAS_TARGET_11,
> } DisasJumpType;
> +/**
> + * DisasContextBase:
> + * @tb: Translation block for this disassembly.
> + * @pc_first: Address of first guest instruction in this TB.
> + * @pc_next: Address of next guest instruction in this TB (current during
> + * disassembly).
> + * @is_jmp: What instruction to disassemble next.
> + * @num_insns: Number of translated instructions (including current).
> + * @singlestep_enabled: "Hardware" single stepping enabled.
> + *
> + * Architecture-agnostic disassembly context.
> + */
> +typedef struct DisasContextBase {
> +TranslationBlock *tb;
> +target_ulong pc_first;
> +target_ulong pc_next;
> +DisasJumpType is_jmp;
> +unsigned int num_insns;
> +bool singlestep_enabled;
> +} DisasContextBase;
> +
> +/**
> + * TranslatorOps:
> + * @init_disas_context:
> + * Initialize the target-specific portions of DisasContext struct.
> + * The generic DisasContextBase has already been initialized.
> + * Return max_insns, modified as necessary by db->tb->flags.
> + *
> + * @tb_start:
> + * Emit any code required before the start of the main loop,
> + * after the generic gen_tb_start().
> + *
> + * @insn_start:
> + * Emit the tcg_gen_insn_start opcode.
> + *
> + * @breakpoint_check:
> + * When called, the breakpoint has already been checked to match the PC,
> + * but the target may decide the breakpoint missed the address
> + * (e.g., due to conditions encoded in their flags). Return true to
> + * indicate that the breakpoint did hit, in which case no more
> breakpoints
> + * are checked. If the breakpoint did hit, emit any code required to
> + * signal the exception, and set db->is_jmp as necessary to terminate
> + * the main loop.
> + *
> + * @translate_insn:
> + * Disassemble one instruction and set db->pc_next for the start
> + * of the following instruction. Set db->is_jmp as necessary to
> + * terminate the main loop.
> + *
> + * @tb_stop:
> + * Emit any opcodes required to exit the TB, based on db->is_jmp.
> + *
> + * @disas_log:
> + * Print instruction disassembly to log.
> + */
> +typedef struct TranslatorOps {
> +int (*init_disas_context)(DisasContextBase *db, CPUState *cpu,
> + int max_insns);
> +void (*tb_start)(DisasContextBase *db, CPUState *cpu);
> +void (*insn_start)(DisasContextBase *db, CPUState *cpu);
> +bool (*breakpoint_check)(DisasContextBase *db, CPUState *cpu,
> + const CPUBreakpoint *bp);
> +void (*translate_insn)(DisasContextBase *db, CPUState *cpu);
> +void (*tb_stop)(DisasContextBase *db, CPUState *cpu);
> +void (*disas_log)(const DisasContextBase *db, CPUState *cpu);
> +} TranslatorOps;
> +
> +/**
> + * translator_loop:
> + * @ops: Target-specific operations.
> + * @db: Disassembly context.
> + * @cpu: Target vCPU.
> + * @tb: Translation block.
> + *
> + * Generic translator loop.
> + *
> + * Translation will stop in the following cases (in order):
> + * - When et by #TranslatorOps::insn_start.
Seems untrue; there's a tcg_debug_assert, so this should now probably be
breakpoint_check() instead.
> + * - When set by #TranslatorOps::translate_insn.
> + * - When the TCG operation buffer is full.
> + * - When single-stepping is enabled (system-wide or on the current vCPU).
> + * - When too many instructions have been translated.
> + */
> +void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
> +
Re: [Qemu-devel] [PATCH v14 29/34] target/arm: [tcg, a64] Port to disas_log
On Fri, Jul 14, 2017 at 23:42:38 -1000, Richard Henderson wrote: > From: Lluís Vilanova > > Incrementally paves the way towards using the generic instruction translation > loop. > > Signed-off-by: Lluís Vilanova > Reviewed-by: Richard Henderson > Message-Id: <150002606914.22386.15524101311003685068.st...@frigg.lan> > [rth: Move tb->size computation and use that result.] > Signed-off-by: Richard Henderson Reviewed-by: Emilio G. Cota E.
Re: [Qemu-devel] [PATCH v14 27/34] target/arm: [tcg, a64] Port to tb_stop
On Fri, Jul 14, 2017 at 23:42:36 -1000, Richard Henderson wrote: > From: Lluís Vilanova > > Incrementally paves the way towards using the generic instruction translation > loop. > > Signed-off-by: Lluís Vilanova > Reviewed-by: Richard Henderson > Message-Id: <150002558503.22386.1149037590886263349.st...@frigg.lan> > Signed-off-by: Richard Henderson Reviewed-by: Emilio G. Cota E.
Re: [Qemu-devel] [PATCH v14 26/34] target/arm: [tcg] Port to tb_stop
On Fri, Jul 14, 2017 at 23:42:35 -1000, Richard Henderson wrote: > From: Lluís Vilanova > > Incrementally paves the way towards using the generic instruction translation > loop. > > Signed-off-by: Lluís Vilanova > Message-Id: <150002534291.22386.13499916738708680298.st...@frigg.lan> > Signed-off-by: Richard Henderson (snip) > @@ -12135,6 +12063,7 @@ void gen_intermediate_code(CPUState *cs, > TranslationBlock *tb) > default: > /* FIXME: Single stepping a WFI insn will not halt the CPU. */ > gen_singlestep_exception(dc); > +break; > case DISAS_NORETURN: > break; > } See my comment to patch 4. Otherwise, Reviewed-by: Emilio G. Cota E.
Re: [Qemu-devel] [PATCH v14 07/34] target/arm: Set is_jmp properly after single-stepping
Richard Henderson writes:
> We have generated an exception, so use DISAS_NORETURN.
Shouldn't this be folded into patch 4?
Thanks,
Lluis
> Signed-off-by: Richard Henderson
> ---
> target/arm/translate-a64.c | 1 +
> target/arm/translate.c | 4 +++-
> 2 files changed, 4 insertions(+), 1 deletion(-)
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 342ff7c..657684b 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -11285,6 +11285,7 @@ void gen_intermediate_code_a64(CPUState *cs,
> TranslationBlock *tb)
dc-> is_jmp = DISAS_UPDATE;
> } else {
> gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> +dc->is_jmp = DISAS_NORETURN;
> /* The address covered by the breakpoint must be
> included in [tb->pc, tb->pc + tb->size) in order
> to for it to be properly cleared -- thus we
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 2ae68ce..83e5491 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -11942,6 +11942,7 @@ void gen_intermediate_code(CPUState *cs,
> TranslationBlock *tb)
dc-> is_jmp = DISAS_UPDATE;
> } else {
> gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> +dc->is_jmp = DISAS_NORETURN;
> /* The address covered by the breakpoint must be
> included in [tb->pc, tb->pc + tb->size) in order
> to for it to be properly cleared -- thus we
> @@ -11986,7 +11987,8 @@ void gen_intermediate_code(CPUState *cs,
> TranslationBlock *tb)
> assert(num_insns == 1);
> gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0),
>default_exception_el(dc));
> -goto done_generating;
> +dc->is_jmp = DISAS_NORETURN;
> +break;
> }
> if (dc->thumb) {
> --
> 2.9.4
Re: [Qemu-devel] [PATCH v14 04/34] target/arm: Use DISAS_NORETURN
On Fri, Jul 14, 2017 at 23:42:13 -1000, Richard Henderson wrote: > Fold DISAS_EXC and DISAS_TB_JUMP into DISAS_NORETURN. > > In both cases all following code is dead. In the first > case because we have exited the TB via exception; in the > second case because we have exited the TB via goto_tb > and its associated machinery. > > Signed-off-by: Richard Henderson > --- > target/arm/translate.h | 8 ++-- > target/arm/translate-a64.c | 37 - > target/arm/translate.c | 15 --- > 3 files changed, 30 insertions(+), 30 deletions(-) > (snip) > diff --git a/target/arm/translate.c b/target/arm/translate.c > index e80cc35..fea76fb 100644 > --- a/target/arm/translate.c > +++ b/target/arm/translate.c (snip) > @@ -12081,6 +12081,8 @@ void gen_intermediate_code(CPUState *cs, > TranslationBlock *tb) > default: > /* FIXME: Single stepping a WFI insn will not halt the CPU. */ > gen_singlestep_exception(dc); > +case DISAS_NORETURN: > +break; Missing '/* fall-through */' above 'case DISAS_NORETURN'. Or just 'break' as we otherwise end up doing in patch 26. E.
Re: [Qemu-devel] [PATCH v14 03/34] target/i386: Use generic DISAS_* enumerators
Richard Henderson writes: > This target is not sophisticated in its use of cleanups at the > end of the translation loop. For the most part, any condition > that exits the TB is dealt with by emitting the exiting opcode > right then and there. Therefore the only is_jmp indicator that > is needed is DISAS_NORETURN. > For two stack segment modifying cases, we have not yet exited > the TB (therefore DISAS_NORETURN feels wrong), but intend to exit. > The caller of gen_movl_seg_T0 currently checks for any non-zero > value, therefore DISAS_TOO_MANY seems acceptable for that usage. > Signed-off-by: Richard Henderson > --- > target/i386/translate.c | 28 +++- > 1 file changed, 15 insertions(+), 13 deletions(-) > diff --git a/target/i386/translate.c b/target/i386/translate.c > index cab9e32..3ffbf1b 100644 > --- a/target/i386/translate.c > +++ b/target/i386/translate.c > @@ -31,6 +31,7 @@ > #include "trace-tcg.h" > #include "exec/log.h" > +#define DISAS_TOO_MANY 5 Why is this one not added as a generic define too (like DISAS_NORETURN in prev patch)? Thanks, Lluis
Re: [Qemu-devel] [PATCH v14 02/34] tcg: Add generic DISAS_NORETURN
Richard Henderson writes: > This will allow some amount of cleanup to happen before > switching the backends over to enum DisasJumpType. > Signed-off-by: Richard Henderson Reviewed-by: Lluís Vilanova > --- > include/exec/exec-all.h | 1 + > 1 file changed, 1 insertion(+) > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index 48d9d11..6760078 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -40,6 +40,7 @@ typedef ram_addr_t tb_page_addr_t; > #define DISAS_JUMP1 /* only pc was modified dynamically */ > #define DISAS_UPDATE 2 /* cpu state was modified dynamically */ > #define DISAS_TB_JUMP 3 /* only pc was modified statically */ > +#define DISAS_NORETURN 4 /* the tb has already been exited */ > #include "qemu/log.h" > -- > 2.9.4
Re: [Qemu-devel] [PATCH v14 25/34] target/arm: [tcg, a64] Port to translate_insn
On Fri, Jul 14, 2017 at 23:42:34 -1000, Richard Henderson wrote: > From: Lluís Vilanova > > Incrementally paves the way towards using the generic instruction translation > loop. > > Signed-off-by: Lluís Vilanova > Message-Id: <150002510079.22386.10164419868911710218.st...@frigg.lan> > [rth: Adjust for translate_insn interface change.] > Signed-off-by: Richard Henderson Reviewed-by: Emilio G. Cota E.
Re: [Qemu-devel] [PATCH v14 24/34] target/arm: [tcg] Port to translate_insn
On Fri, Jul 14, 2017 at 23:42:33 -1000, Richard Henderson wrote: > From: Lluís Vilanova > > Incrementally paves the way towards using the generic instruction translation > loop. > > Signed-off-by: Lluís Vilanova > Message-Id: <150002485863.22386.13949856269576226529.st...@frigg.lan> > [rth: Adjust for translate_insn interface change.] > Signed-off-by: Richard Henderson Reviewed-by: Emilio G. Cota E.
Re: [Qemu-devel] [PATCH v14 23/34] target/arm: [tcg, a64] Port to breakpoint_check
On Fri, Jul 14, 2017 at 23:42:32 -1000, Richard Henderson wrote: > From: Lluís Vilanova > > Incrementally paves the way towards using the generic instruction translation > loop. > > Signed-off-by: Lluís Vilanova > Reviewed-by: Richard Henderson > Message-Id: <150002461630.22386.14827196109258040543.st...@frigg.lan> > [rth: Use DISAS_TOO_MANY for "execute only one more" after bp.] > Signed-off-by: Richard Henderson Reviewed-by: Emilio G. Cota E.
Re: [Qemu-devel] [PATCH v14 10/34] target/i386: [tcg] Port to init_disas_context
On Fri, Jul 14, 2017 at 23:42:19 -1000, Richard Henderson wrote: > From: Lluís Vilanova > > Incrementally paves the way towards using the generic instruction translation > loop. > > Signed-off-by: Lluís Vilanova > Reviewed-by: Richard Henderson > Reviewed-by: Alex Benneé > Message-Id: <150002122448.22386.16854673576827449259.st...@frigg.lan> > [rth: Adjust for max_insns interface change.] > Signed-off-by: Richard Henderson Reviewed-by: Emilio G. Cota E.
Re: [Qemu-devel] [PATCH v4 42/43] tcg: introduce regions to split code_gen_buffer
On 07/20/2017 07:59 PM, Emilio G. Cota wrote: This is groundwork for supporting multiple TCG contexts. The naive solution here is to split code_gen_buffer statically among the TCG threads; this however results in poor utilization if translation needs are different across TCG threads. What we do here is to add an extra layer of indirection, assigning regions that act just like pages do in virtual memory allocation. (BTW if you are wondering about the chosen naming, I did not want to use blocks or pages because those are already heavily used in QEMU). We use a global lock to serialize allocations as well as statistics reporting (we now export the size of the used code_gen_buffer with tcg_code_size()). Note that for the allocator we could just use a counter and atomic_inc; however, that would complicate the gathering of tcg_code_size()-like stats. So given that the region operations are not a fast path, a lock seems the most reasonable choice. The effectiveness of this approach is clear after seeing some numbers. I used the bootup+shutdown of debian-arm with '-tb-size 80' as a benchmark. Note that I'm evaluating this after enabling per-thread TCG (which is done by a subsequent commit). * -smp 1, 1 region (entire buffer): qemu: flush code_size=83885014 nb_tbs=154739 avg_tb_size=357 qemu: flush code_size=83884902 nb_tbs=153136 avg_tb_size=363 qemu: flush code_size=83885014 nb_tbs=152777 avg_tb_size=364 qemu: flush code_size=83884950 nb_tbs=150057 avg_tb_size=373 qemu: flush code_size=83884998 nb_tbs=150234 avg_tb_size=373 qemu: flush code_size=83885014 nb_tbs=154009 avg_tb_size=360 qemu: flush code_size=83885014 nb_tbs=151007 avg_tb_size=370 qemu: flush code_size=83885014 nb_tbs=151816 avg_tb_size=367 That is, 8 flushes. * -smp 8, 32 regions (80/32 MB per region) [i.e. this patch]: qemu: flush code_size=76328008 nb_tbs=141040 avg_tb_size=356 qemu: flush code_size=75366534 nb_tbs=138000 avg_tb_size=361 qemu: flush code_size=76864546 nb_tbs=140653 avg_tb_size=361 qemu: flush code_size=76309084 nb_tbs=135945 avg_tb_size=375 qemu: flush code_size=74581856 nb_tbs=132909 avg_tb_size=375 qemu: flush code_size=73927256 nb_tbs=135616 avg_tb_size=360 qemu: flush code_size=78629426 nb_tbs=142896 avg_tb_size=365 qemu: flush code_size=76667052 nb_tbs=138508 avg_tb_size=368 Again, 8 flushes. Note how buffer utilization is not 100%, but it is close. Smaller region sizes would yield higher utilization, but we want region allocation to be rare (it acquires a lock), so we do not want to go too small. * -smp 8, static partitioning of 8 regions (10 MB per region): qemu: flush code_size=21936504 nb_tbs=40570 avg_tb_size=354 qemu: flush code_size=11472174 nb_tbs=20633 avg_tb_size=370 qemu: flush code_size=11603976 nb_tbs=21059 avg_tb_size=365 qemu: flush code_size=23254872 nb_tbs=41243 avg_tb_size=377 qemu: flush code_size=28289496 nb_tbs=52057 avg_tb_size=358 qemu: flush code_size=43605160 nb_tbs=78896 avg_tb_size=367 qemu: flush code_size=45166552 nb_tbs=82158 avg_tb_size=364 qemu: flush code_size=63289640 nb_tbs=116494 avg_tb_size=358 qemu: flush code_size=51389960 nb_tbs=93937 avg_tb_size=362 qemu: flush code_size=59665928 nb_tbs=107063 avg_tb_size=372 qemu: flush code_size=38380824 nb_tbs=68597 avg_tb_size=374 qemu: flush code_size=44884568 nb_tbs=79901 avg_tb_size=376 qemu: flush code_size=50782632 nb_tbs=90681 avg_tb_size=374 qemu: flush code_size=3984 nb_tbs=71433 avg_tb_size=372 qemu: flush code_size=64708840 nb_tbs=119052 avg_tb_size=359 qemu: flush code_size=49830008 nb_tbs=90992 avg_tb_size=362 qemu: flush code_size=68372408 nb_tbs=123442 avg_tb_size=368 qemu: flush code_size=3360 nb_tbs=59514 avg_tb_size=378 qemu: flush code_size=44748344 nb_tbs=80974 avg_tb_size=367 qemu: flush code_size=37104248 nb_tbs=67609 avg_tb_size=364 That is, 20 flushes. Note how a static partitioning approach uses the code buffer poorly, leading to many unnecessary flushes. Signed-off-by: Emilio G. Cota --- tcg/tcg.h | 6 ++ accel/tcg/translate-all.c | 63 + bsd-user/main.c | 1 + cpus.c| 12 +++ linux-user/main.c | 1 + tcg/tcg.c | 222 +- 6 files changed, 260 insertions(+), 45 deletions(-) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH v14 07/34] target/arm: Set is_jmp properly after single-stepping
On Fri, Jul 14, 2017 at 23:42:16 -1000, Richard Henderson wrote:
> We have generated an exception, so use DISAS_NORETURN.
>
> Signed-off-by: Richard Henderson
> ---
> target/arm/translate-a64.c | 1 +
> target/arm/translate.c | 4 +++-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index 342ff7c..657684b 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -11285,6 +11285,7 @@ void gen_intermediate_code_a64(CPUState *cs,
> TranslationBlock *tb)
> dc->is_jmp = DISAS_UPDATE;
> } else {
> gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> +dc->is_jmp = DISAS_NORETURN;
> /* The address covered by the breakpoint must be
> included in [tb->pc, tb->pc + tb->size) in order
> to for it to be properly cleared -- thus we
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 2ae68ce..83e5491 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -11942,6 +11942,7 @@ void gen_intermediate_code(CPUState *cs,
> TranslationBlock *tb)
> dc->is_jmp = DISAS_UPDATE;
> } else {
> gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> +dc->is_jmp = DISAS_NORETURN;
gen_exception_internal_insn already sets dc->is_jmp to DISAS_NORETURN,
as per patch 04/34:
@@ -304,7 +304,7 @@ static void gen_exception_internal_insn(DisasContext *s,
int offset, int excp)
{
gen_a64_set_pc_im(s->pc - offset);
gen_exception_internal(excp);
-s->is_jmp = DISAS_EXC;
+s->is_jmp = DISAS_NORETURN;
}
This applies to both arm and a64.
Why do we need to set is_jmp again, then?
E.
Re: [Qemu-devel] [PATCH v4 35/43] tcg: allocate optimizer temps with tcg_malloc
On 07/20/2017 07:59 PM, Emilio G. Cota wrote: Groundwork for supporting multiple TCG contexts. While at it, also allocate temps_used directly as a bitmap of the required size, instead of using a bitmap of TCG_MAX_TEMPS via TCGTempSet. Performance-wise we lose about 1.12% in a translation-heavy workload such as booting+shutting down debian-arm: Performance counter stats for 'taskset -c 0 arm-softmmu/qemu-system-arm \ -machine type=virt -nographic -smp 1 -m 4096 \ -netdev user,id=unet,hostfwd=tcp::-:22 \ -device virtio-net-device,netdev=unet \ -drive file=die-on-boot.qcow2,id=myblock,index=0,if=none \ -device virtio-blk-device,drive=myblock \ -kernel kernel.img -append console=ttyAMA0 root=/dev/vda1 \ -name arm,debug-threads=on -smp 1' (10 runs): exec time (s) Relative slowdown wrt original (%) --- original 20.213321616 0. tcg_malloc 20.441130078 1.1270214 TCGContext 20.477846517 1.3086662 g_malloc 20.780527895 2.8061013 The other two alternatives shown in the table are: - TCGContext: embed temps[TCG_MAX_TEMPS] and TCGTempSet used_temps in TCGContext. This is simple enough but it isn't faster than using tcg_malloc; moreover, it wastes memory. - g_malloc: allocate/deallocate both temps and used_temps every time tcg_optimize is executed. Suggested-by: Richard Henderson Signed-off-by: Emilio G. Cota --- tcg/optimize.c | 306 ++--- 1 file changed, 161 insertions(+), 145 deletions(-) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH] vhost: fix a migration failed because of vhost region merge
On Fri, Jul 21, 2017 at 04:41:58PM +0200, Igor Mammedov wrote:
> On Wed, 19 Jul 2017 18:52:56 +0300
> "Michael S. Tsirkin" wrote:
>
> > On Wed, Jul 19, 2017 at 03:24:27PM +0200, Igor Mammedov wrote:
> > > On Wed, 19 Jul 2017 12:46:13 +0100
> > > "Dr. David Alan Gilbert" wrote:
> > >
> > > > * Igor Mammedov (imamm...@redhat.com) wrote:
> > > > > On Wed, 19 Jul 2017 23:17:32 +0800
> > > > > Peng Hao wrote:
> > > > >
> > > > > > When a guest that has several hotplugged dimms is migrated, in
> > > > > > destination host it will fail to resume. Because vhost regions of
> > > > > > several dimms in source host are merged and in the restore stage
> > > > > > in destination host it computes whether more than vhost slot limit
> > > > > > before merging vhost regions of several dimms.
> > > > > could you provide a bit more detailed description of the problem
> > > > > including command line+used device_add commands on source and
> > > > > command line on destination?
> > > >
> > > > (ccing in Marc Andre and Maxime)
> > > >
> > > > Hmm, I'd like to understade the situation where you get merging between
> > > > RAMBlocks; that complicates some stuff for postcopy.
> > > and probably inconsistent merging breaks vhost as well
> > >
> > > merging might happen if regions are adjacent or overlap
> > > but for that to happen merged regions must have equal
> > > distance between their GPA:HVA pairs, so that following
> > > translation would work:
> > >
> > > if gva in regionX[gva_start, len, hva_start]
> > >hva = hva_start + gva - gva_start
> > >
> > > while GVA of regions is under QEMU control and deterministic
> > > HVA is not, so in migration case merging might happen on source
> > > side but not on destination, resulting in different memory maps.
> > >
> > > Maybe Michael might know details why migration works in vhost usecase,
> > > but I don't see vhost sending any vmstate data.
> >
> > We aren't merging ramblocks at all.
> > When we are passing blocks A and B to vhost, if we see that
> >
> > hvaB=hvaA + lenA
> > gpaB=gpaA + lenA
> >
> > then we can improve performance a bit by passing a single
> > chunk to vhost: hvaA,gpaA,lena+lenB
> kernel used to maintain flat array map for look up where
> such optimization could give some benefit which is negligible
> as in practice merging reduces array size only by ~5 entries.
>
> In addition kernel backend has been converted to interval tree
> as flat array doesn't scale, so merging doesn't really matters
> there anymore.
In my opinion not merging slots is an obvious waste - I
think there were patches that added a cache and that
showed some promise. cache will be more effective
if regions are bigger.
> If we can get rid of merging on QEMU side, resulting memory
> map will become of the same size regardless of the order
> in which entries are added or chancy random allocation
> that could allow region merging (i.e. size will become
> deterministic).
It seems somehow wrong to avoid doing (even minor) optimizations just to
make error handling simpler.
> Looking at vhost_user_set_mem_table() it sends actual number of
> entries to backend over the wire, so it shouldn't break backend
> if it were written right (i.e. uses msg.payload.memory.nregions
> instead of VHOST_MEMORY_MAX_NREGIONS from QEMU.), if it breaks
> then it's backend's fault and it should be fixed.
>
> Another thing that could break is too low limit
> VHOST_MEMORY_MAX_NREGIONS = 8
> and QEMU started with default options takes upto 7 entries in map
> unmerged, so any configuration that consumes additional slots won't
> start after upgrade. We could counter the most of issues by rising
> VHOST_MEMORY_MAX_NREGIONS limit and/or teaching vhost-user protocol
> to fetch limit from backend similar to vhost_kernel_memslots_limit().
I absolutely agree we should fix vhost-user to raise the slot
limit, along the lines you suggest. Care looking into it?
>
> > so it does not affect migration normally.
> >
> > >
> > > >
> > > > > >
> > > > > > Signed-off-by: Peng Hao
> > > > > > Signed-off-by: Wang Yechao
> > > > > > ---
> > > > > > hw/mem/pc-dimm.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > > > > > index ea67b46..bb0fa08 100644
> > > > > > --- a/hw/mem/pc-dimm.c
> > > > > > +++ b/hw/mem/pc-dimm.c
> > > > > > @@ -101,7 +101,7 @@ void pc_dimm_memory_plug(DeviceState *dev,
> > > > > > MemoryHotplugState *hpms,
> > > > > > goto out;
> > > > > > }
> > > > > >
> > > > > > -if (!vhost_has_free_slot()) {
> > > > > > +if (!vhost_has_free_slot() && runstate_is_running()) {
> > > > > > error_setg(&local_err, "a used vhost backend has no free"
> > > > > > " memory slots left");
> > > > > > goto out;
> > > >
> > > > Even this produces the wrong error message in this case,
> > > > it also makes me think if the
Re: [Qemu-devel] [PATCH v4 11/43] tcg: define CF_PARALLEL and use it for TB hashing along with CF_COUNT_MASK
On 07/20/2017 07:59 PM, Emilio G. Cota wrote: This will enable us to decouple code translation from the value of parallel_cpus at any given time. It will also help us minimize TB flushes when generating code via EXCP_ATOMIC. Note that the declaration of parallel_cpus is brought to exec-all.h to be able to define there the "curr_cflags" inline. Signed-off-by: Emilio G. Cota --- include/exec/exec-all.h | 20 +++- include/exec/tb-hash-xx.h | 9 ++--- include/exec/tb-hash.h| 4 ++-- include/exec/tb-lookup.h | 6 +++--- tcg/tcg.h | 1 - accel/tcg/cpu-exec.c | 45 +++-- accel/tcg/translate-all.c | 13 + exec.c| 2 +- tcg/tcg-runtime.c | 2 +- tests/qht-bench.c | 2 +- 10 files changed, 65 insertions(+), 39 deletions(-) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH v14 04/34] target/arm: Use DISAS_NORETURN
On Fri, Jul 14, 2017 at 23:42:13 -1000, Richard Henderson wrote: > Fold DISAS_EXC and DISAS_TB_JUMP into DISAS_NORETURN. > > In both cases all following code is dead. In the first > case because we have exited the TB via exception; in the > second case because we have exited the TB via goto_tb > and its associated machinery. > > Signed-off-by: Richard Henderson Reviewed-by: Emilio G. Cota E.
Re: [Qemu-devel] [PATCH v14 06/34] target/arm: Delay check for magic kernel page
On Fri, Jul 14, 2017 at 23:42:15 -1000, Richard Henderson wrote: > There's nothing magic about the exception that we generate in order > to execute the magic kernel page. We can and should allow gdb to > set a breakpoint at this location. > > Signed-off-by: Richard Henderson Reviewed-by: Emilio G. Cota E.
Re: [Qemu-devel] [PATCH v14 03/34] target/i386: Use generic DISAS_* enumerators
On Fri, Jul 14, 2017 at 23:42:12 -1000, Richard Henderson wrote: > This target is not sophisticated in its use of cleanups at the > end of the translation loop. For the most part, any condition > that exits the TB is dealt with by emitting the exiting opcode > right then and there. Therefore the only is_jmp indicator that > is needed is DISAS_NORETURN. > > For two stack segment modifying cases, we have not yet exited > the TB (therefore DISAS_NORETURN feels wrong), but intend to exit. > The caller of gen_movl_seg_T0 currently checks for any non-zero > value, therefore DISAS_TOO_MANY seems acceptable for that usage. > > Signed-off-by: Richard Henderson Reviewed-by: Emilio G. Cota E.
Re: [Qemu-devel] [PATCH v14 02/34] tcg: Add generic DISAS_NORETURN
On Fri, Jul 14, 2017 at 23:42:11 -1000, Richard Henderson wrote: > This will allow some amount of cleanup to happen before > switching the backends over to enum DisasJumpType. > > Signed-off-by: Richard Henderson Reviewed-by: Emilio G. Cota E.
Re: [Qemu-devel] [PATCH 4/9] tests: Clean up string interpolation into QMP input (simple cases)
On 07/21/2017 03:08 PM, Eric Blake wrote: >> 2. Support PRId64 and PRIu64, whatever their actual value may be. >> >>a. Support all possible values. This is what we've tried before. >> >>b. Support exactly the host's PRId64 and PRIu64 values. >> Preferences? >> >> I like 2b, but I'm not sure I'll like the code implementing it. One way >> to find out... I ended up implementing 2a in the C code, but 2b in the configure check to make sure our C code gets updated if we ever encounter any more oddballs. Of course, I don't have easy access to Mac OS, so I'm relying on others to test the patch for me; but once we get that patch going to our liking, I can rebase the rest of this series on top of it. https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg07039.html -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH] qobject: Accept "%"PRId64 in qobject_from_jsonf()
Commit 1792d7d0 was written because PRId64 expands to non-portable
crap for some libc, and we had testsuite failures on Mac OS as a
result. This in turn makes it difficult to rely on the obvious
conversions of 64-bit values into JSON, requiring things such as
casting int64_t to long long so we can use a reliable %lld and
still keep -Wformat happy. So now it's time to fix that.
We are already lexing %I64d (hello mingw); now expand the lexer
to parse %qd (hello Mac OS). In the process, we can drop one
state: IN_ESCAPE_I64 is redundant with IN_ESCAPE_LL. And fix a
comment that mistakenly omitted %u as a supported escape.
Next, tweak the parser to accept the exact spelling of PRId64
regardless of what it expands to (note that there are are now dead
branches in the 'if' chain for some platforms, like glibc; but all
branches are necessary on other platforms). We are at least safe
in that we are parsing the correct size 32-bit or a 64-bit quantity
on whatever branch we end up in. And of course, update the
testsuite for coverage of the feature.
Finally, update configure to barf on any libc that uses yet
another form of PRId64 that we have not yet coded for, so that we
can once again update json-lexer.c to cater to those quirks (my
guess? we might see %jd next). Yes, the only way I could find
to quickly do and still work when cross-compiling is to grep a
compiled binary; C does not make it easy to turn a string constant
into an integer constant, let along make preprocessor decisions;
and even parsing '$CC -E' output is fragile, since 64-bit glibc
pre-processes as "l" "d" rather than "ld". I'm assuming that
'strings' is portable enough during configure.
Signed-off-by: Eric Blake
---
configure | 21 +
qobject/json-lexer.c | 11 +++
qobject/json-parser.c | 10 ++
tests/check-qjson.c | 7 +++
4 files changed, 37 insertions(+), 12 deletions(-)
diff --git a/configure b/configure
index 6b52e19ee3..b810ff970d 100755
--- a/configure
+++ b/configure
@@ -3239,6 +3239,27 @@ for i in $glib_modules; do
fi
done
+# Sanity check that we can parse PRId64 and friends in json-lexer.c
+# (Sadly, the "easiest" way to do this is to grep the compiled binary)
+cat > $TMPC <
+int main(void) {
+static const char findme[] = "UnLiKeLyToClAsH_" PRId64;
+return !findme[0];
+}
+EOF
+if ! compile_prog "$CFLAGS" "$LIBS" ; then
+error_exit "can't determine value of PRId64"
+fi
+nl='
+'
+case $(strings $TMPE | grep ^UnLiKeLyToClAsH) in
+'' | *"$nl"* ) error_exit "can't determine value of PRId64" ;;
+*_ld | *_lld | *_I64d | *_qd) ;;
+*) error_exit "unexepected value of PRId64, please add %$(strings $TMPE |
+ sed -n s/^UnLiKeLyToClAsH_//p) support to json-lexer.c" ;;
+esac
+
# Sanity check that the current size_t matches the
# size that glib thinks it should be. This catches
# problems on multi-arch where people try to build
diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
index 980ba159d6..98b1ec8e35 100644
--- a/qobject/json-lexer.c
+++ b/qobject/json-lexer.c
@@ -31,7 +31,7 @@
*
* Extension for vararg handling in JSON construction:
*
- * %((l|ll|I64)?d|[ipsf])
+ * %(PRI[du]64|(l|ll)?[ud]|[ipsf])
*
*/
@@ -63,7 +63,6 @@ enum json_lexer_state {
IN_ESCAPE_LL,
IN_ESCAPE_I,
IN_ESCAPE_I6,
-IN_ESCAPE_I64,
IN_WHITESPACE,
IN_START,
};
@@ -236,13 +235,8 @@ static const uint8_t json_lexer[][256] = {
['u'] = JSON_ESCAPE,
},
-[IN_ESCAPE_I64] = {
-['d'] = JSON_ESCAPE,
-['u'] = JSON_ESCAPE,
-},
-
[IN_ESCAPE_I6] = {
-['4'] = IN_ESCAPE_I64,
+['4'] = IN_ESCAPE_LL,
},
[IN_ESCAPE_I] = {
@@ -257,6 +251,7 @@ static const uint8_t json_lexer[][256] = {
['u'] = JSON_ESCAPE,
['f'] = JSON_ESCAPE,
['l'] = IN_ESCAPE_L,
+['q'] = IN_ESCAPE_LL,
['I'] = IN_ESCAPE_I,
},
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 7a417f20cd..f01e97fc6b 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -470,15 +470,17 @@ static QObject *parse_escape(JSONParserContext *ctxt,
va_list *ap)
return QOBJECT(qnum_from_int(va_arg(*ap, int)));
} else if (!strcmp(token->str, "%ld")) {
return QOBJECT(qnum_from_int(va_arg(*ap, long)));
-} else if (!strcmp(token->str, "%lld") ||
- !strcmp(token->str, "%I64d")) {
+} else if (!strcmp(token->str, "%" PRId64)) {
+return QOBJECT(qnum_from_int(va_arg(*ap, int64_t)));
+} else if (!strcmp(token->str, "%lld")) {
return QOBJECT(qnum_from_int(va_arg(*ap, long long)));
} else if (!strcmp(token->str, "%u")) {
return QOBJECT(qnum_from_uint(va_arg(*ap, unsigned int)));
} else if (!strcmp(token->str, "%lu")) {
return QOBJECT(qnum_from_uint(va_arg(*ap, unsigned long)));
-} else if (!strcmp(token->str, "%llu") ||
- !strcmp(token->str, "%I64u")) {
+} else
Re: [Qemu-devel] [PATCH 4/9] tests: Clean up string interpolation into QMP input (simple cases)
On 07/21/2017 11:48 AM, Markus Armbruster wrote: > I forgot that PRId64 expands into non-standard crap on some systems. > > Options: > > 1. Outlaw use of PRI macros, limit integer length modifiers for >conversion specifiers "d" and "u" to "l" and "ll". When PRI macros >creep in, the build breaks on hosts where they expand to anything >else (hopefully, our CI will catch that). Interpolating int64_t >values become a bit awkward. > >Required work: fix my patches not to use PRId64, drop support for >length modifier "I64" from parse_escape(). Yuck. (But motivation for my earlier series to nuke qobject_from_jsonf()) > > 2. Support PRId64 and PRIu64, whatever their actual value may be. > >a. Support all possible values. This is what we've tried before. > Nasty: fails only at run-time on hosts with sufficiently creative > values. > > Required work: add support for length modifier "q", to unbreak > MacOS. > > Optional work: try to turn the run-time failure into a compile- > time failure, ideally in configure. Accepts garbage (although -Wformat detection will help avoid the worst of it). You can't check string equality in the preprocessor, and you can't do things like "case PRId64[0]: case 'l':" to force compilation errors due to duplicate labels, but we CAN limit ourselves to the preprocessor and grep that output to see what PRId64 expands to. I'll see if I can come up with a configure check. Still, it may be easier to just fail configure and tell people to report the bug on their odd libc, at which point we update json-lexer.c and configure, than it is to try and reuse configure results in json-lexer.c (since the PRId64 string is not a constant width, it gets hard to code an exact value into our lexer state machine, but easier to code every reported value). > >b. Support exactly the host's PRId64 and PRIu64 values. > > Work required: replace support of "I64d" and "I64u" by support of > PRId64 and PRIu64. Easy enough in parse_escape(), but not in > json-lexer.c. We could perhaps have the lexer accept the shortest > string that's followed by a conversion specifier as length > modifier, then reject invalid length modifiers in a semantic > action. Interesting idea. I'm playing with it... > >Optional work: try to simplify code that currently works around >unavailability of PRId64 and PRIu64. > > Preferences? > > I like 2b, but I'm not sure I'll like the code implementing it. One way > to find out... In the lexer, widen the state machine to accept up to three unknown characters between % and d. Hmm, there's also the possibility of int64_t being mapped to %jd, in addition to our known culprits of %ld, %lld, %I64d, and %qd. > >> Overall, I like the patch, but we need to fix the problems for the next >> round of this series. > > Yes. At this point, I think I'll spin the next round. But it's not longer a 2.10 priority, so it may be a few days. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] vhost: fix a migration failed because of vhost region merge
On Thu, Jul 20, 2017 at 06:22:15PM +0100, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (m...@redhat.com) wrote:
> > On Wed, Jul 19, 2017 at 03:24:27PM +0200, Igor Mammedov wrote:
> > > On Wed, 19 Jul 2017 12:46:13 +0100
> > > "Dr. David Alan Gilbert" wrote:
> > >
> > > > * Igor Mammedov (imamm...@redhat.com) wrote:
> > > > > On Wed, 19 Jul 2017 23:17:32 +0800
> > > > > Peng Hao wrote:
> > > > >
> > > > > > When a guest that has several hotplugged dimms is migrated, in
> > > > > > destination host it will fail to resume. Because vhost regions of
> > > > > > several dimms in source host are merged and in the restore stage
> > > > > > in destination host it computes whether more than vhost slot limit
> > > > > > before merging vhost regions of several dimms.
> > > > > could you provide a bit more detailed description of the problem
> > > > > including command line+used device_add commands on source and
> > > > > command line on destination?
> > > >
> > > > (ccing in Marc Andre and Maxime)
> > > >
> > > > Hmm, I'd like to understade the situation where you get merging between
> > > > RAMBlocks; that complicates some stuff for postcopy.
> > > and probably inconsistent merging breaks vhost as well
> > >
> > > merging might happen if regions are adjacent or overlap
> > > but for that to happen merged regions must have equal
> > > distance between their GPA:HVA pairs, so that following
> > > translation would work:
> > >
> > > if gva in regionX[gva_start, len, hva_start]
> > >hva = hva_start + gva - gva_start
> > >
> > > while GVA of regions is under QEMU control and deterministic
> > > HVA is not, so in migration case merging might happen on source
> > > side but not on destination, resulting in different memory maps.
> > >
> > > Maybe Michael might know details why migration works in vhost usecase,
> > > but I don't see vhost sending any vmstate data.
> >
> > We aren't merging ramblocks at all.
> > When we are passing blocks A and B to vhost, if we see that
> >
> > hvaB=hvaA + lenA
> > gpaB=gpaA + lenA
> >
> > then we can improve performance a bit by passing a single
> > chunk to vhost: hvaA,gpaA,lena+lenB
>
> OK, but that means that a region can incorporate multiple
> RAMBlocks though? Hmm that's not fun on postcopy.
>
> > so it does not affect migration normally.
>
> Well, why? What's required - if the region sizes/lengths/orders
> are different on the source and destination does it matter - if
> it does then that means we have a problem, since that heuristic
> is non-deterministic.
>
> Dave
It doesn't matter normally. But there's a limit
on number of regions that vhost can support.
pc_dimm_memory_plug tries to guess this number early
because it wants to fail hotplug requests gracefully.
See
commit 3fad87881e55aaff659408dcf25fa204f89a7896
Author: Igor Mammedov
Date: Tue Oct 6 10:37:28 2015 +0200
pc-dimm: add vhost slots limit check before commiting to hotplug
It might make sense to limit this to hotplug, though
runstate check seems like a wrong way to do this -
VM could be stopped e.g. through QMP at the time.
> >
> > >
> > > >
> > > > > >
> > > > > > Signed-off-by: Peng Hao
> > > > > > Signed-off-by: Wang Yechao
> > > > > > ---
> > > > > > hw/mem/pc-dimm.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > > > > > index ea67b46..bb0fa08 100644
> > > > > > --- a/hw/mem/pc-dimm.c
> > > > > > +++ b/hw/mem/pc-dimm.c
> > > > > > @@ -101,7 +101,7 @@ void pc_dimm_memory_plug(DeviceState *dev,
> > > > > > MemoryHotplugState *hpms,
> > > > > > goto out;
> > > > > > }
> > > > > >
> > > > > > -if (!vhost_has_free_slot()) {
> > > > > > +if (!vhost_has_free_slot() && runstate_is_running()) {
> > > > > > error_setg(&local_err, "a used vhost backend has no free"
> > > > > > " memory slots left");
> > > > > > goto out;
> > > >
> > > > Even this produces the wrong error message in this case,
> > > > it also makes me think if the existing code should undo a lot of
> > > > the object_property_set's that happen.
> > > >
> > > > Dave
> > > > >
> > > > >
> > > > --
> > > > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH 2/2] [PATCH] hmp: allow apic-id for "info lapic"
On Fri, Jul 21, 2017 at 03:38:56AM -0400, Yi Wang wrote:
> Add [apic-id] support for hmp command "info lapic", which is
> useful when debugging ipi and so on. Current behavior is not
> changed when the parameter isn't specified.
>
> Signed-off-by: Yi Wang
> Signed-off-by: Yun Liu
> ---
> hmp-commands-info.hx | 6 +++---
> target/i386/cpu.h | 10 ++
> target/i386/helper.c | 16
> target/i386/monitor.c | 9 -
> 4 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index d9df238..00623df 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -115,9 +115,9 @@ ETEXI
> #if defined(TARGET_I386)
> {
> .name = "lapic",
> -.args_type = "",
> -.params = "",
> -.help = "show local apic state",
> +.args_type = "apic-id:i?",
> +.params = "[apic-id]",
> +.help = "show local apic state (apic-id: local apic to read,
> default is 0)",
Default isn't 0: it's the APIC of the current CPU (the one
selected with the "cpu" command).
> .cmd= hmp_info_local_apic,
> },
> #endif
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 31ad681..4da2ca2 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1712,6 +1712,16 @@ void enable_compat_apic_id_mode(void);
> #define APIC_DEFAULT_ADDRESS 0xfee0
> #define APIC_SPACE_SIZE 0x10
>
> +/**
> + * x86_get_cpu_by_apic:
> + * @id: The apic-id of the specified CPU to obtain.
> + *
> + * Gets a CPU on which @id given of apic.
> + *
> + * Returns: The CPU or %NULL if there is no matching CPU.
> + */
> +CPUState *x86_get_cpu_by_apic(int id);
> +
> void x86_cpu_dump_local_apic_state(CPUState *cs, FILE *f,
> fprintf_function cpu_fprintf, int flags);
>
> diff --git a/target/i386/helper.c b/target/i386/helper.c
> index fd4f1af..6972833 100644
> --- a/target/i386/helper.c
> +++ b/target/i386/helper.c
> @@ -398,6 +398,22 @@ void x86_cpu_dump_local_apic_state(CPUState *cs, FILE *f,
> }
> cpu_fprintf(f, " PPR 0x%02x\n", apic_get_ppr(s));
> }
> +
> +CPUState *x86_get_cpu_by_apic(int id)
> +{
> +CPUState *cs;
> +
> +CPU_FOREACH(cs) {
> +X86CPU *cpu = X86_CPU(cs);
> +APICCommonState *s = APIC_COMMON(cpu->apic_state);
> +if (id == s->id) {
> +return cs;
> +}
> +}
> +
> +return NULL;
> +}
Similar logic already exists in qom/cpu.c:cpu_exists().
I suggest the following:
diff --git a/qom/cpu.c b/qom/cpu.c
index 4f38db0..e6210d5 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -34,7 +34,7 @@
CPUInterruptHandler cpu_interrupt_handler;
-bool cpu_exists(int64_t id)
+CPUState *cpu_by_arch_id(int64_t id)
{
CPUState *cpu;
@@ -42,10 +42,15 @@ bool cpu_exists(int64_t id)
CPUClass *cc = CPU_GET_CLASS(cpu);
if (cc->get_arch_id(cpu) == id) {
-return true;
+return cpu;
}
}
-return false;
+return NULL;
+}
+
+bool cpu_exists(int64_t id)
+{
+return !!cpu_by_arch_id(id);
}
CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
Signed-off-by: Eduardo Habkost
> +
> #else
> void x86_cpu_dump_local_apic_state(CPUState *cs, FILE *f,
> fprintf_function cpu_fprintf, int flags)
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index 77ead60..e911a57 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -632,7 +632,14 @@ const MonitorDef *target_monitor_defs(void)
>
> void hmp_info_local_apic(Monitor *mon, const QDict *qdict)
> {
> -CPUState *cs = mon_get_cpu();
> +CPUState *cs;
> +
> +if (qdict_haskey(qdict, "apic-id")) {
> +int id = qdict_get_try_int(qdict, "apic-id", 0);
On which cases do you want qdict_get_try_int() to return
def_value (0), here? The qdict_haskey() check will ensure the
key is present, and monitor_parse_arguments() will ensure it
will be an integer. Just qdict_get_int() seems safe here.
> +cs = x86_get_cpu_by_apic(id);
> +} else {
> +cs = mon_get_cpu();
> +}
>
> if (!cs) {
> monitor_printf(mon, "No CPU available\n");
> --
> 1.8.3.1
>
>
--
Eduardo
Re: [Qemu-devel] [PATCH] target/arm: fix TCG temp leak in aarch64 rev16
On Fri, Jul 21, 2017 at 14:16:46 +0100, Peter Maydell wrote: > On 21 July 2017 at 06:42, Emilio G. Cota wrote: > > const leak! patch below -- cut with `git am --scissors'. > > > > Emilio > > > > ---8<--- > > > > Signed-off-by: Emilio G. Cota > > --- > > Applied to target-arm.next, thanks -- but in future could you > send patches as proper patch emails, please (output of > git format-patch, sent as its own top level email, not a > reply to an existing thread) ? Otherwise the automated tools > don't recognise them as patches and it requires some tedious > manual application at this end. Will do, sorry about that. E.
Re: [Qemu-devel] [PATCH 2/3] Unified Datagram Socket Transport - GRE support
On 07/19/2017 12:32 PM, Anton Ivanov wrote:
>
>
> On 19/07/17 15:40, Eric Blake wrote:
>> On 07/18/2017 12:08 PM, anton.iva...@cambridgegreys.com wrote:
>>> From: Anton Ivanov
>>>
>>> This adds GRETAP support to the unified socket driver.
>>>
>>> +#
>>> +# @ipv6: force the use of ipv6
>> This doesn't quite match what we do with other sockets (where we have
>> both ipv4 and ipv6 booleans to allow IPv4-only, IPv6-only, or both). Is
>> this something where we can reuse InetSocketAddress instead of inventing
>> yet another way of doing things?
>>
>> Then again, it does match what NetdevL2TPv3Options did :(
>
> I just reviewed this again.
>
> I do not think we can today. This is the declaration:
>
> ##
> { 'struct': 'InetSocketAddressBase',
> 'data': {
> 'host': 'str',
> 'port': 'str' } }
>
> ##
>
> If I read this right port is mandatory, correct?
Okay, so it sounds like reusing InetSocket directly may not be possible.
But there's still the interface question of whether we want dual 'ipv4'
and 'ipv6' switches to allow finer-grain control over which (or both)
families to be used.
>
> We may be able to do it if the port portion if InetSocketAddress becomes
> optional. There is no such thing as port for the protocols which use the
> raw families.
We can always create a new QAPI type that expresses only the fields we
need; I don't think InetSocketAddress should be changed to have an
optional port just for your code additions.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] dirty page count problem
* Dr. David Alan Gilbert (dgilb...@redhat.com) wrote:
> Hi,
> Git bisect is pointing to your patch 084140bd49:
> exec: fix access to ram_list.dirty_memory when sync dirty bitmap
>
> trying to diagnose a bug I'm seeing; it looks like the dirty page count
> is wrong for some reason.
>
> Alex Bennée spotted a problem where the postcopy test would occasionally
> fail under very heavy load;attaching a debugger and it looks like
> the problem is we have a migration_dirty_page count stuck at 2;
> in the normal migration tests we don't spot this, because 2 pages is
> smaller than the threshold to end migration and so an extra 2 pages
> doesn't block it finishing. However, with a very
> small downtime setting (like we use in the postcopy test) and with
> very low bandwidth (as when Alex ran the test on a very heavily loaded
> machine) we end up never calling the bitmap sync again and never
> completing the iteration.
>
> I'm using the following addition to spot the problem:
I think I have the fix for this, or at least the reason; the alignment
check also needs fixing up, but the 08414 patch missed it.
Alex: Please test the fix included at the bottom
>
> diff --git a/migration/ram.c b/migration/ram.c
> index e75f1050e4..3ddf884952 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1350,6 +1350,13 @@ static int ram_find_and_save_block(RAMState *rs, bool
> last_stage)
> }
> } while (!pages && again);
>
> +if (!pages && !again && pss.complete_round && rs->migration_dirty_pages)
> +{
> +/* Should make this fail migration ? */
> +fprintf(stderr, "%s: no page found, yet dirty_pages=%"PRIu64"\n",
> +__func__, rs->migration_dirty_pages);
> +}
> +
> rs->last_seen_block = pss.block;
> rs->last_page = pss.page;
>
> (which I might add as a test to fail a migration)
>
> That test fails easily even on an unloaded machine:
> tests/postcopy-test
> /x86_64/postcopy: ram_find_and_save_block: no page found, yet dirty_pages=2
> ram_find_and_save_block: no page found, yet dirty_pages=2
> ram_find_and_save_block: no page found, yet dirty_pages=2
> OK
>
>
> I'll try and debug where our extra two pages are coming from.
From b2c86818eaaaf790246f21ae21fead903c1d64f0 Mon Sep 17 00:00:00 2001
From: "Dr. David Alan Gilbert"
Date: Fri, 21 Jul 2017 19:59:01 +0100
Subject: [PATCH] cpu_physical_memory_sync_dirty_bitmap: Fix alignment check
This code has an optimised, word aligned version, and a boring
unaligned version. Recently 084140bd498909 fixed a missing offset
addition from the core of both versions. However, the offset isn't
necessarily aligned and thus the choice between the two versions
needs fixing up to also include the offset.
DANGER: Friday evening, post-dinner lightly tested patch.
Symptom:
A few stuck unsent pages during migration; not normally noticed
unless under very low bandwidth.
Fixes: 084140bd498909
---
include/exec/ram_addr.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index c04f4f6..c802f7f 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -378,15 +378,16 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock
*rb,
{
ram_addr_t addr;
unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
+unsigned long word = BIT_WORD((start + rb->offset) >> TARGET_PAGE_BITS);
uint64_t num_dirty = 0;
unsigned long *dest = rb->bmap;
/* start address is aligned at the start of a word? */
-if (((page * BITS_PER_LONG) << TARGET_PAGE_BITS) == start) {
+if (((word * BITS_PER_LONG) << TARGET_PAGE_BITS) ==
+ (start + rb->offset)) {
int k;
int nr = BITS_TO_LONGS(length >> TARGET_PAGE_BITS);
unsigned long * const *src;
-unsigned long word = BIT_WORD((start + rb->offset) >>
TARGET_PAGE_BITS);
unsigned long idx = (word * BITS_PER_LONG) / DIRTY_MEMORY_BLOCK_SIZE;
unsigned long offset = BIT_WORD((word * BITS_PER_LONG) %
DIRTY_MEMORY_BLOCK_SIZE);
--
1.8.3.1
> Dave
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] Unified Datagram Socket Transport
Hi Jason, Hi Eric, hi list, I have gone through all comments and addressed everything to which I did not reply separately with clarifications. Before I resubmit I have a couple of architectural questions: 1. Is it OK in its current form: UDST client which cannot be instantiated and the others creating instances of it. I am aware that this does not quite match the current semantics, but this keeps the per-transport code to the minimum possible - init and (in the newest version) optional verify and form header functions. F.e. in the next submission raw will be init only and its data - nothing else. 2. I have updated the help, docs and the API. 3. I did not quite understand your comment on socket.c - what are you suggesting there - do you want to fold stream mode into a common backend? I do not think it is possible. I have tried to do surgery only on the datagram stuff. Also, socket.c is quite old and has a violations of current coding style and conventions. Should I fix those as a part of the submission or this can be a separate patch? A. On 21/07/17 04:55, Jason Wang wrote: On 2017年07月21日 03:12, anton.iva...@cambridgegreys.com wrote: Hi all, This addresses comments so far except Eric's suggestion to use InetSocketAddressBase. If I understand correctly its intended use, it will not be of help for protocols which have no port (raw sockets - GRE, L2TPv3, etc). It also includes a port of the original socket.c transport to the new UDST backend. The relevant code is ifdef-ed so there should be no effect on other systems. This looks sub-optimal. If you want to do this, I would rather suggest you just extend the socket dgram backend like what udst did now. I think that this is would be the appropriate place to stop in this iteration. I would prefer to have this polished, before I start looking at sendmmsg and bulk send or some of the more unpleasant encapsulations like geneve. Pay attention we're softfreeze now. So the feature is for 2.11, if it looks good, I can only queue it for 2.11. Btw, looks like not all comments of v1 were addressed. Thanks A. -- Anton R. Ivanov Cambridge Greys Limited, England and Wales company No 10273661 http://www.cambridgegreys.com/
Re: [Qemu-devel] [PATCH 10/11] qtest.py: Few pylint/style fixes
On Fri, Jul 21, 2017 at 08:57:34AM +0200, Lukáš Doktor wrote: > Dne 20.7.2017 v 20:42 Eduardo Habkost napsal(a): > > On Thu, Jul 20, 2017 at 06:28:14PM +0200, Lukáš Doktor wrote: > > [...] > >> @@ -83,8 +80,11 @@ class QEMUQtestMachine(qemu.QEMUMachine): > >> socket_scm_helper=None): > >> if name is None: > >> name = "qemu-%d" % os.getpid() > >> -super(QEMUQtestMachine, self).__init__(binary, args, name=name, > >> test_dir=test_dir, > >> - > >> socket_scm_helper=socket_scm_helper) > >> +scm_helper = socket_scm_helper > > > > Why is this necessary? > > > to avoid > 80 chars line. It should be optimized-out by the > python compiler so it should not slow down the execution. > Alternative solution is to use: > > super(QEMUQtestMachine, > self.__init__(...) > > which looks IMO uglier, but I can use that in v2, should that be your > preferred style. I think that would be better. The purpose of the extra variable isn't clear when reading the code, making it more confusing. -- Eduardo
Re: [Qemu-devel] [PATCH 3/3] Unified Datagram Socket Transport - raw support
[snip]
+"-netdev raw,id=str,ifname=ifname\n"
+"configure a network backend with ID 'str'
connected to\n"
+"an Ethernet interface named ifname via raw
socket.\n"
+"This backend does not change the interface
settings.\n"
+"Most interfaces will require being set into
promisc mode,\n"
+"as well having most offloads (TSO, etc) turned
off.\n"
+"Some virtual interfaces like tap support only
RX.\n"
Pay attention that qemu supports vnet header. So any reason to turn
off e.g TSO here?
I am not aware of any means to get extra info like checksums, etc show
up on raw socket read.
If you know a way to make them show up, this is worth investigating.
#endif
"-netdev
socket,id=str[,fd=h][,listen=[host]:port][,connect=host:port]\n"
"configure a network backend to connect to
another network\n"
@@ -2463,6 +2470,32 @@ qemu-system-i386 linux.img -net nic -net
gre,src=4.2.3.1,dst=1.2.3.4
@end example
+@item -netdev raw,id=@var{id},ifname=@var{ifname}
+@itemx -net raw[,vlan=@var{n}][,name=@var{name}],ifname=@var{ifname}
+Connect VLAN @var{n} directly to an Ethernet interface using raw
socket.
+
+This transport allows a VM to bypass most of the network stack which is
+extremely useful for tapping.
+
+@item ifname=@var{ifname}
+interface name (mandatory)
+
+@example
+# set up the interface - put it in promiscuous mode and turn off
offloads
+ifconfig eth0 up
+ifconfig eth0 promisc
+
+/sbin/ethtool -K eth0 gro off
+/sbin/ethtool -K eth0 tso off
+/sbin/ethtool -K eth0 gso off
+/sbin/ethtool -K eth0 tx off
Any reason to turn off tx here?
Yes - we already have it computed and we have written it as is as a
whole packet. You do not want it
re-computed as at least some adapters do silly things if you start
writing raw and the checksum already exists.
Once again, this one of the pros/cons of using tpacket vs recv/send
(with or without mmsg) on a raw socket.
recvm(m)sg/sendm(m)sg are brute force as far as offloads, but things
like scatter/gather work correctly so there are little copies.
Compared to that, tpacket will allow you some access to checksumming
which you can map onto checksum offload in a vNIC. As a payback for this
you end up copying in more cases than for send/recvmmsg and you pay
penalty for timestamping if you do not have a hardware timestamp source
in the NIC.
The other issue I always had with tpacket is that you "see" your own
packets so you have to manage a RX side BPF filter which removes those
so you do not see your own packets. That can get quite interesting if
you have a lot of MACs on a NIC (f.e. when there are multicast apps).
Not sure if this is still the case - it definitely was in mid 3.x Linux
kernels. If you use raw sendm(m)sg there is no issue - the packets are
not looped when writing to physical interfaces.
+
+# launch QEMU instance - if your network has reorder or is very
lossy add ,pincounter
+
+qemu-system-i386 linux.img -net nic -net raw,ifname=eth0
Can we switch to use -netdev here?
This is done in the new revisions.
Thanks
+
+@end example
+
@item -netdev
vde,id=@var{id}[,sock=@var{socketpath}][,port=@var{n}][,group=@var{groupname}][,mode=@var{octalmode}]
@itemx -net
vde[,vlan=@var{n}][,name=@var{name}][,sock=@var{socketpath}]
[,port=@var{n}][,group=@var{groupname}][,mode=@var{octalmode}]
Connect VLAN @var{n} to PORT @var{n} of a vde switch running on
host and
--
Anton R. Ivanov
Cambridge Greys Limited, England and Wales company No 10273661
http://www.cambridgegreys.com/
Re: [Qemu-devel] [PATCH 09/11] qmp.py: Avoid overriding a builtin object
On Fri, Jul 21, 2017 at 08:53:49AM +0200, Lukáš Doktor wrote:
> Dne 20.7.2017 v 20:38 Eduardo Habkost napsal(a):
> > On Thu, Jul 20, 2017 at 06:28:13PM +0200, Lukáš Doktor wrote:
> >> The "id" is a builtin method to get object's identity and should not be
> >> overridden. This might bring some issues in case someone was directly
> >> calling "cmd(..., id=id)" but I haven't found such usage on brief search
> >> for "cmd\(.*id=".
> >>
> >> Signed-off-by: Lukáš Doktor
> >> ---
> >> scripts/qmp/qmp.py | 8
> >> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
> >> index a14b001..c3e0206 100644
> >> --- a/scripts/qmp/qmp.py
> >> +++ b/scripts/qmp/qmp.py
> >> @@ -177,19 +177,19 @@ class QEMUMonitorProtocol(object):
> >> print >>sys.stderr, "QMP:<<< %s" % resp
> >> return resp
> >>
> >> -def cmd(self, name, args=None, id=None):
> >> +def cmd(self, name, args=None, cmd_id=None):
> >> """
> >> Build a QMP command and send it to the QMP Monitor.
> >>
> >> @param name: command name (string)
> >> @param args: command arguments (dict)
> >> -@param id: command id (dict, list, string or int)
> >> +@param cmd_id: command id (dict, list, string or int)
> >> """
> >> qmp_cmd = {'execute': name}
> >> if args:
> >> qmp_cmd['arguments'] = args
> >> -if id:
> >> -qmp_cmd['id'] = id
> >> +if cmd_id:
> >> +qmp_cmd['cmd_id'] = cmd_id
> >
> > The member sent through the monitor should still be called "id".
> > i.e.:
> >
> > qmp_cmd['id'] = cmd_id
> >
> Oups, sorry, automatic rename changed it and I forgot to fix
> this one back. I'll address this in v2. The main problem with
> this patch is it could break named arguments (`cmd(..., id=id)`
> calls) so I'm not sure it's worth including. But as mentioned
> in commit message I grepped full sources so we better fix this
> before someone starts using it.
I'm not convinced we need this patch, either. What exactly
breaks if we don't apply this patch and somebody tries to use
cmd(..., id=id)?
--
Eduardo
Re: [Qemu-devel] [PATCH 07/11] qmp.py: Use object-based class for QEMUMonitorProtocol
On Fri, Jul 21, 2017 at 08:50:22AM +0200, Lukáš Doktor wrote: > Dne 20.7.2017 v 20:35 Eduardo Habkost napsal(a): > > On Thu, Jul 20, 2017 at 06:28:11PM +0200, Lukáš Doktor wrote: > >> There is no need to define QEMUMonitorProtocol as old-style class. > >> > >> Signed-off-by: Lukáš Doktor > >> --- > >> scripts/qmp/qmp.py | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py > >> index bb4d614..68f3420 100644 > >> --- a/scripts/qmp/qmp.py > >> +++ b/scripts/qmp/qmp.py > >> @@ -30,7 +30,7 @@ class QMPTimeoutError(QMPError): > >> pass > >> > >> > >> -class QEMUMonitorProtocol: > >> +class QEMUMonitorProtocol(object): > > > > I don't fully understand the consequences of changing to > > new-style classes when using old-style SuperClass.__init__() > > calls in the __init__(). Should we change QMPShell.__init__() to > > use super()? > > > The consequence is it becomes a proper object with full object > model and less workarounds. It also consumes a bit more memory > but are the only available mode in py3. > > As for the old-style superclass, it works, but the correct > approach is to replace it with `super` call. I'll address that > in the v2 (I only checked for `.py` files but there are many > python sources in qemu tree without the proper extension. I > still need to get used to this.). Thanks for the explanation. Reviewed-by: Eduardo Habkost -- Eduardo
Re: [Qemu-devel] [PATCH 05/11] qemu.py: Use custom exceptions rather than Exception
On Fri, Jul 21, 2017 at 08:37:34AM +0200, Lukáš Doktor wrote:
> Dne 20.7.2017 v 20:27 Eduardo Habkost napsal(a):
> > On Thu, Jul 20, 2017 at 06:28:09PM +0200, Lukáš Doktor wrote:
> >> The naked Exception should not be widely used. It makes sense to be a
> >> bit more specific and use better-suited custom exceptions. As a benefit
> >> we can store the full reply in the exception in case someone needs it
> >> when catching the exception.
> >>
> >> Signed-off-by: Lukáš Doktor
> >> ---
> >> scripts/qemu.py | 17 +++--
> >> 1 file changed, 15 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/scripts/qemu.py b/scripts/qemu.py
> >> index 5948e19..2bd522f 100644
> >> --- a/scripts/qemu.py
> >> +++ b/scripts/qemu.py
> >> @@ -19,6 +19,19 @@ import subprocess
> >> import qmp.qmp
> >>
> >>
> >> +class MonitorResponseError(qmp.qmp.QMPError):
> >> +'''
> >> +Represents erroneous QMP monitor reply
> >> +'''
> >> +def __init__(self, reply):
> >> +try:
> >> +desc = reply["error"]["desc"]
> >> +except KeyError:
> >> +desc = reply
> >> +super(MonitorResponseError, self).__init__(desc)
> >> +self.reply = reply
> >
> > This would require every user of self.reply to first check if
> > it's a string or dictionary. All because of the "Monitor is
> > closed" case below:
> >
> I haven't used it for the `Monitor is closed` exception, so
> it's just to be able to store really broken reply safely.
> Anyway people can check whether the reply is a dict, or I can
> add `is_valid_reply` property which would check for
> `[error][desc]` presence (which is a bit more precise than just
> plain dict type checking).
Oops, I wasn't paying enough attention, sorry. self.reply is
actually always set to the response from the monitor.
If you are just trying a safe fallback for 'desc' if the response
broken, what about using repr(reply) or json.dumps(reply) if
reply['error']['desc'] isn't set?
>
> >> +
> >> +
> >> class QEMUMachine(object):
> >> '''A QEMU VM'''
> >>
> >> @@ -197,9 +210,9 @@ class QEMUMachine(object):
> >> '''
> >> reply = self.qmp(cmd, conv_keys, **args)
> >> if reply is None:
> >> -raise Exception("Monitor is closed")
> >> +raise qmp.qmp.QMPError("Monitor is closed")
> >
> > Should we really use the same exception type for this, if it's
> > not really a QMP monitor error reply, and won't even have a real
> > QMP reply in self.reply?
> >
> I wasn't sure but the same exception can be caused by other
> failures when obtaining replies so I think in case the monitor
> is closed we might expect the same exception. Would importing
> it in the top level of this module to become `qemu.QMPError`
> work for you better, or would you prefer IOError, RuntimeError
> or another custom exception?
I was not paying enough attention. QMPError sounds good to me.
Reviewed-by: Eduardo Habkost
>
> Lukáš
>
> >
> >> if "error" in reply:
> >> -raise Exception(reply["error"]["desc"])
> >> +raise MonitorResponseError(reply)
> >> return reply["return"]
> >>
> >> def get_qmp_event(self, wait=False):
> >> --
> >> 2.9.4
> >>
> >
>
>
--
Eduardo
[Qemu-devel] virtio-net-pci possible limitation on AArch64
Hi, I ran into an interesting limitation (or possibly bug) while trying to attach more than 3 (three) virtio-net-pci NICs to an AArch64 guest. I created 4 network with virsh (see below for xml defitions) - nothing fancy, only 2 of them use NAT. Attaching a virtio-net-pci NIC for each network works well, VMs can be spawned, all devices are properly detected by the guest OS. The first 3 guest interfaces work perfectly, however I can't get any traffic on the 4th one (in my case the public one). QEMU version: 2.9.0 Libvirt: tested with both 1.3.4 and 3.5.0, although I don't think libvirt is to blame here Host OS: Ubuntu Xenial 16.04.2 LTS Guest OS: Ubuntu Xenial Cloud Image (latest available) Reproducibility rate: 100% with below config VM is created with (also domain XML attached at the end of this mail): $ virt-install --name cfg01 --ram 4096 --vcpus 6 --cpu host-passthrough --accelerate \ --network network:pxe,model=virtio-net-pci \ --network network:mgmt,model=virtio-net-pci \ --network network:internal,model=virtio-net-pci \ --network network:public,model=virtio-net-pci \ --console pty --autostart --noreboot --noautoconsole --video=vga --noautoconsole [...] You might notice I had to disable legacy virtio in the domain XML, or the guest would complain about a missing F_... property and no guest network interfaces would be detected. For now, since we only need 4 guest interfaces, we switched one of them back to virtio-mmio, and everything is working as expected. Unfortunately I can't investigate this myself right now, but I can provide additional details if needed for someone else to look into it. BR, Alex --- Network definitions: --- pxe mgmt internal public --- Domain XML: --- cfg01 3f8415d1-963a-46aa-8a63-1c6558775b92 4194304 4194304 6 /machine hvm /usr/share/AAVMF/AAVMF_CODE.fd /var/lib/libvirt/qemu/nvram/cfg01_VARS.fd destroy restart restart /usr/bin/kvm libvirt-3f8415d1-963a-46aa-8a63-1c6558775b92 libvirt-3f8415d1-963a-46aa-8a63-1c6558775b92 +113:+115 +113:+115
Re: [Qemu-devel] [PULL for-2.10 6/7] xen/mapcache: introduce xen_replace_cache_entry()
On 21/07/17 14:50, Anthony PERARD wrote:
On Tue, Jul 18, 2017 at 03:22:41PM -0700, Stefano Stabellini wrote:
From: Igor Druzhinin
...
+static uint8_t *xen_replace_cache_entry_unlocked(hwaddr old_phys_addr,
+ hwaddr new_phys_addr,
+ hwaddr size)
+{
+MapCacheEntry *entry;
+hwaddr address_index, address_offset;
+hwaddr test_bit_size, cache_size = size;
+
+address_index = old_phys_addr >> MCACHE_BUCKET_SHIFT;
+address_offset = old_phys_addr & (MCACHE_BUCKET_SIZE - 1);
+
+assert(size);
+/* test_bit_size is always a multiple of XC_PAGE_SIZE */
+test_bit_size = size + (old_phys_addr & (XC_PAGE_SIZE - 1));
+if (test_bit_size % XC_PAGE_SIZE) {
+test_bit_size += XC_PAGE_SIZE - (test_bit_size % XC_PAGE_SIZE);
+}
+cache_size = size + address_offset;
+if (cache_size % MCACHE_BUCKET_SIZE) {
+cache_size += MCACHE_BUCKET_SIZE - (cache_size % MCACHE_BUCKET_SIZE);
+}
+
+entry = &mapcache->entry[address_index % mapcache->nr_buckets];
+while (entry && !(entry->paddr_index == address_index &&
+ entry->size == cache_size)) {
+entry = entry->next;
+}
+if (!entry) {
+DPRINTF("Trying to update an entry for %lx " \
+"that is not in the mapcache!\n", old_phys_addr);
+return NULL;
+}
+
+address_index = new_phys_addr >> MCACHE_BUCKET_SHIFT;
+address_offset = new_phys_addr & (MCACHE_BUCKET_SIZE - 1);
+
+fprintf(stderr, "Replacing a dummy mapcache entry for %lx with %lx\n",
+old_phys_addr, new_phys_addr);
Looks likes this does not build on 32bits.
in:
http://logs.test-lab.xenproject.org/osstest/logs/112041/build-i386/6.ts-xen-build.log
/home/osstest/build.112041.build-i386/xen/tools/qemu-xen-dir/hw/i386/xen/xen-mapcache.c:
In function 'xen_replace_cache_entry_unlocked':
/home/osstest/build.112041.build-i386/xen/tools/qemu-xen-dir/hw/i386/xen/xen-mapcache.c:539:13:
error: format '%lx' expects argument of type 'long unsigned int', but argument
3 has type 'hwaddr' [-Werror=format=]
old_phys_addr, new_phys_addr);
^
/home/osstest/build.112041.build-i386/xen/tools/qemu-xen-dir/hw/i386/xen/xen-mapcache.c:539:13:
error: format '%lx' expects argument of type 'long unsigned int', but argument
4 has type 'hwaddr' [-Werror=format=]
cc1: all warnings being treated as errors
CC i386-softmmu/target/i386/gdbstub.o
/home/osstest/build.112041.build-i386/xen/tools/qemu-xen-dir/rules.mak:66:
recipe for target 'hw/i386/xen/xen-mapcache.o' failed
+
+xen_remap_bucket(entry, entry->vaddr_base,
+ cache_size, address_index, false);
+if (!test_bits(address_offset >> XC_PAGE_SHIFT,
+test_bit_size >> XC_PAGE_SHIFT,
+entry->valid_mapping)) {
+DPRINTF("Unable to update a mapcache entry for %lx!\n", old_phys_addr);
+return NULL;
+}
+
+return entry->vaddr_base + address_offset;
+}
+
Please, accept the attached patch to fix the issue.
Igor
>From 69a3afa453e283e92ddfd76109b203a20a02524c Mon Sep 17 00:00:00 2001
From: Igor Druzhinin
Date: Fri, 21 Jul 2017 19:27:41 +0100
Subject: [PATCH] xen: fix compilation on 32-bit hosts
Signed-off-by: Igor Druzhinin
---
hw/i386/xen/xen-mapcache.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index 84cc4a2..540406a 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -529,7 +529,7 @@ static uint8_t *xen_replace_cache_entry_unlocked(hwaddr old_phys_addr,
entry = entry->next;
}
if (!entry) {
-DPRINTF("Trying to update an entry for %lx " \
+DPRINTF("Trying to update an entry for "TARGET_FMT_plx \
"that is not in the mapcache!\n", old_phys_addr);
return NULL;
}
@@ -537,15 +537,16 @@ static uint8_t *xen_replace_cache_entry_unlocked(hwaddr old_phys_addr,
address_index = new_phys_addr >> MCACHE_BUCKET_SHIFT;
address_offset = new_phys_addr & (MCACHE_BUCKET_SIZE - 1);
-fprintf(stderr, "Replacing a dummy mapcache entry for %lx with %lx\n",
-old_phys_addr, new_phys_addr);
+fprintf(stderr, "Replacing a dummy mapcache entry for "TARGET_FMT_plx \
+" with "TARGET_FMT_plx"\n", old_phys_addr, new_phys_addr);
xen_remap_bucket(entry, entry->vaddr_base,
cache_size, address_index, false);
if(!test_bits(address_offset >> XC_PAGE_SHIFT,
test_bit_size >> XC_PAGE_SHIFT,
entry->valid_mapping)) {
-DPRINTF("Unable to update a mapcache entry for %lx!\n", old_phys_addr);
+DPRINTF("Unable to update a mapcache entry for "TARGET_FMT_plx"!\n",
+old_phys_addr);
return NULL;
}
--
2.7.4
[Qemu-devel] [PATCH 2/2] qcow2: Fix sector calculation in qcow2_measure()
We used MAX() instead of the intended MIN() when computing how many
sectors to view in the current loop iteration of qcow2_measure(),
and passed in a value of INT_MAX sectors instead of our more usual
limit of BDRV_REQUEST_MAX_SECTORS (the latter avoids 32-bit overflow
on conversion to bytes). For small files, the bug is harmless:
bdrv_get_block_status_above() clamps its *pnum answer to the BDS
size, regardless of any insanely larger input request. However, for
any file at least 2T in size, we can very easily end up going into an
infinite loop (the maximum of 0x1 sectors and INT_MAX is a
64-bit quantity, which becomes 0 when assigned to int; once nb_sectors
is 0, we never make progress).
Signed-off-by: Eric Blake
---
block/qcow2.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 048df7e88b..d7c600b5a2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3670,8 +3670,8 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts,
BlockDriverState *in_bs,
for (sector_num = 0;
sector_num < ssize / BDRV_SECTOR_SIZE;
sector_num += pnum) {
-int nb_sectors = MAX(ssize / BDRV_SECTOR_SIZE - sector_num,
- INT_MAX);
+int nb_sectors = MIN(ssize / BDRV_SECTOR_SIZE - sector_num,
+ BDRV_REQUEST_MAX_SECTORS);
BlockDriverState *file;
int64_t ret;
--
2.13.3
[Qemu-devel] [PATCH 1/2] dirty-bitmap: Report BlockDirtyInfo.count in bytes, as documented
We've been documenting the value in bytes since its introduction
in commit b9a9b3a4 (v1.3), where it was actually reported in bytes.
Commit e4654d2 (v2.0) then removed things from block/qapi.c, in
preparation for a rewrite to a list of dirty sectors in the next
commit 21b5683 in block.c, but the new code mistakenly started
reporting in sectors.
Fixes: https://bugzilla.redhat.com/1441460
CC: qemu-sta...@nongnu.org
Signed-off-by: Eric Blake
Reviewed-by: John Snow
---
Too late for 2.9, since the regression has been unnoticed for
nine releases. But worth putting in 2.9.1 and 2.10.
v2-v4: no change
---
block/dirty-bitmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 543bddb9b5..30462d4f9a 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -461,7 +461,7 @@ BlockDirtyInfoList
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
-info->count = bdrv_get_dirty_count(bm);
+info->count = bdrv_get_dirty_count(bm) << BDRV_SECTOR_BITS;
info->granularity = bdrv_dirty_bitmap_granularity(bm);
info->has_name = !!bm->name;
info->name = g_strdup(bm->name);
--
2.13.3
[Qemu-devel] [PATCH for-2.10 0/2] Bug fixes from byte-based block status
Series 2-4 of my byte-based conversion missed soft freeze, so they are now 2.11 material. However, there are some bug fixes in those series that we should fix now in 2.10 (patch 1 from series two on dirty bitmaps, patch 2 extracted from "qcow2: Switch qcow2_measure() to byte-based iteration" from series three on block status). I don't know if it is worth enhancing iotest 178 to probe the size of a 2T image. The test is simple, and fast when patched: $ qemu-img create -f qcow2 -o cluster_size=2M huge 2T $ time ./qemu-img measure -O qcow2 -f qcow2 huge required size: 335806464 fully allocated size: 2199359062016 real0m0.021s user0m0.017s sys 0m0.004s but the inf-loop when unpatched is annoying; meanwhile, 'huge' only occupies 6 megabytes on disk, so it's not that invasive. Eric Blake (2): dirty-bitmap: Report BlockDirtyInfo.count in bytes, as documented qcow2: Fix sector calculation in qcow2_measure() block/dirty-bitmap.c | 2 +- block/qcow2.c| 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) -- 2.13.3
Re: [Qemu-devel] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support
On Fri, Jul 21, 2017 at 2:34 PM, Igor Mammedov wrote: > On Fri, 21 Jul 2017 10:23:38 +0100 > "Daniel P. Berrange" wrote: > >> On Fri, Jul 21, 2017 at 11:06:36AM +0200, Igor Mammedov wrote: >> > On Thu, 20 Jul 2017 21:29:33 +0200 >> > Phil Dennis-Jordan wrote: >> > >> > > On Thu, Jul 20, 2017 at 6:40 PM, Programmingkid >> > > wrote: >> > > > I noticed that Windows 2000 does not boot up in QEMU recently. After >> > > > bisecting the issue I found the offending commit: >> > w2k is very ancient (and long time EOLed), I can't even download it from >> > msdn to test >> > (oldest available is XP) >> > >> > do we really care about it? >> >> From a Red Hat, we don't care about it, because we're only targetting >> modern OS in RHEL, but from a QEMU community POV ability to run pretty >> much any guest OS you care to find is definitely in scope. > As far as someone is willing to maintain it and test it regularly, > otherwise it will beak someday anyway. > (I'm not really willing to do it as I don't have access to w2k and > interested in reducing maintainable code, but maybe someone would > like to step up, feel free to post patch to amend acpi maintaners) > > currently option 1 looks like the most compatible approach > but there is no way to predict if it will break some other OS > and it is not trivial to implement and maintain. > > CCing Laszlo, to get his opinion if option 1 is viable from > old/new OVMF standpoint (is it possible in 2.10 time frame?). I've not done a deep investigation yet, but I've put together a really quick prototype for the split RSDT/XSDT with 2 FADTs. I tested my existing WinXP x86, Win10 x64, and Ubuntu 16.04 x86-64 test images with SeaBIOS and they all worked. With OVMF, neither Windows 10 nor macOS would boot with this change - I don't currently know if that's OVMF's fault or if my prototype is broken. I don't have time right now to dig deeper into this, but hopefully I can look at it on Monday and also dig out a Win2000 disc then as well and test with that. The prototype patch is at https://github.com/pmj/qemu/tree/xsdt right now for anyone curious, or with more time on their hands to test it with Win2K and figure out why it's not working with OVMF. I'll try to do a proper RFC patch submission on Monday once I have a better handle on what's going on. I don't have any strong release policy opinions - I'll leave that to those with more experience. I'd be disappointed though if we had to entirely revert the Rev3 FADT patch for 2.10. >> > > Ouch. I reckon we have 2 options for fixing this: >> > > >> > > 1. Export two FADTs, one ACPI 1.0, one ACPI 2.0. The latter would need >> > > to be pointed to by an XSDT, which Qemu currently doesn't implement at >> > > all as far as I'm aware. Any ideas on how SeaBIOS or OVMF would handle >> > > this? Any likely other OS regressions? >> > > >> > > 2. Select FADT version with an option. This one is definitely safe, >> > > but adds yet another option. >> > the 3rd simpler option is: >> > force rev1 on old machine types (2.9 and older), >> > using machine compat machinery and use rev3 on newer machines >> >> That's not really a 3rd option - it is something that applies to >> both option 1 and 2. >> >> The original commit, and both these options involve changes are >> sensitive to guest ABI. So all machine types from 2.9 and earlier >> *must* be configured to stick with the ACPI 1.0 FADT only. > It's not per se ABI change, ABI is still the same but BIOS > supplied ACPI tables changed (nowdays they are generated by QEMU) > to new ones. Currently QEMU does not support versioned firmware, > i.e. each new release ships updated firmware and old machines > also use it. The same typically applies to ACPI tables. > > Alternative workaround, for w2k user (management), doesn't even > requires any fixes to qemu, he/she should just use old enough bios > with new QEMU (bios from 1.7 or 1.6 should work as it doesn't fetch > ACPI tables from QEMU). > > Another alternative is just keep just using old QEMU version with w2k, > the sources are available. > If user's figured out that he/she needs to force FADT(rev1) for w2k > not to crash (he is either able to bisect/compile QEMU or read this > thread at which point he/she should be able to his poison i.e. > compiling vs old bios) > > >> Whether we then have an option to turn on ACPI 2.0, or instead >> expose 1.0 and 2.0 at the same time, both must only happen on >> the 2.10 machine type (or newer if it misses this release). >> >> >> > > > commit 77af8a2b95b79699de650965d5228772743efe84 >> > > > Author: Phil Dennis-Jordan >> > > > Date: Wed Mar 15 19:20:26 2017 +1300 >> > > > >> > > > hw/i386: Use Rev3 FADT (ACPI 2.0) instead of Rev1 to improve guest >> > > > OS support. >> > > > >> > > > This updates the FADT generated for x86/64 machine types from >> > > > Revision 1 to 3. (Based on ACPI standard 2.0 instead of 1.0) The >> > > > intention is to expose the reset register information to guest >> >
Re: [Qemu-devel] [SeaBIOS] [RFC PATCH 0/2] Allow RedHat PCI bridges reserve more buses than necessary during init
On 21/07/2017 20:28, Kevin O'Connor wrote: On Fri, Jul 21, 2017 at 03:15:46PM +0300, Marcel Apfelbaum wrote: On 21/07/2017 13:04, Gerd Hoffmann wrote: I'd prefer to have a single vendor capability for all resource allocation hints provided by qemu. Sure, the capability looking something like: [flags: reserve-buses|reserve-IO|reserve-MEM|...] [extra-buses][IO-size][MEM-size] if reserve-buses -> use 'extra-buses' value and so on I don't have any objection to using a PCI capability, but I do wonder if fw_cfg would be a better fit. This information is purely qemu -> firmware, right? Hi Kevin, Right, but theoretically speaking a guest OS driver could also get the hint on hotplug, or simply because the OS chooses to re-assign resources on its own. A while ago we discussed the fw_cfg option, but Gerd preferred the vendor capability, and since the capability looked cleaner Aleksandr opted for it. He will send V2 soon together with the QEMU counterpart feature. Thanks, Marcel -Kevin
Re: [Qemu-devel] [PATCH 1/3] Unified Datagram Socket Transport
[snip] +NetUnifiedState *s = (NetUnifiedState *) us; +L2TPV3TunnelParams *p = (L2TPV3TunnelParams *) s->params; How about embedding NetUnifiedState into this structure and keep using NetL2TPV3State? Then: - 's' could be kept and lots of lines of changes could be saved here and l2tpv3_verify_header() - each transport could have their own type instead of using NET_CLIENT_DRIVER_L2TPV3 That means each of them having their own read/write functions in each transport, destroy functions, etc. I am trying to achieve exactly the opposite which across all transports should save more code. There should be nothing in a transport which leverages the common datagram processing backend except: 1. Init and parse arguments 2. Form Header 3. Verify Header All the rest can be common for a large family of datagram based transports - L2TPv3, GRE, RAW (both full interface and just pulling a specific vlan out of it), etc. It is trivial to do that for fixed size headers (as in the current patchset family). It is a bit more difficult to that for variable headers, but still datagram (GUE, Geneve, etc). These may also add 4 - I/O to control plane, but it remains to be seen if that is needed. This also makes any improvements to the backend - f.e. switching from send() to sendmmsg() automatically available for all transports. What cannot be done is to shoehorn into this stream based. I believe we have only one of those - the original socket.c in tcp mode and we can leave it to stay that way and switch only the datagram mode to a better backend. I am going through the other comments in the meantime to see if I missed something else and fixing the omissions. A. [snip] -- Anton R. Ivanov Cambridge Greys Limited, England and Wales company No 10273661 http://www.cambridgegreys.com/
Re: [Qemu-devel] [RFC PATCH for 2.11 12/23] target/arm/translate-a64.c: add FP16 FAGCT to AdvSIMD 3 Same
On 2017-07-21 14:58, Peter Maydell wrote: > On 21 July 2017 at 14:50, Alex Bennée wrote: > > Aurelien Jarno writes: > >> As said in another email, some architectures actually use more than one > >> float_status. We therefore need to implement a solution like the one > >> proposed by Richard. > > > > Ahh you mean more than one float_status for a given vCPU context? > > Yep. ARM's cpu state struct has > float_status fp_status; > float_status standard_fp_status; > > which we use to handle (1) operations which use the state > controlled by the FPSCR value and (2) operations which > ignore the FPSCR and use the "Standard FPSCR Value" (generally > Neon ops). More info in the comment in cpu.h... Indeed. This is also the case on: - i386: one float_status for the x87 FPU, one for MMX and one for SSE. - mips: one for the FPU and one for the MSA FP (SIMD operations). - ppc: one for the FPU instructions and one for the VEC instructions. Aurelien -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH] pc: acpi: force FADT rev1 for old i440fx machine types
> On Jul 21, 2017, at 5:32 AM, Igor Mammedov wrote:
>
> w2k used to boot on QEMU until we bumped revision of FADT to rev3
> (commit 77af8a2b hw/i386: Use Rev3 FADT (ACPI 2.0) instead of Rev1 to improve
> guest OS support.)
>
> Considering that w2k is ancient and long time EOLed, leave default
> rev3 but make pc-i440fx-2.9 and older machine types to force rev1
> so old setups won't break (w2k could boot).
>
> Signed-off-by: Igor Mammedov
> ---
> CC: Programmingkid
> CC: Phil Dennis-Jordan
> CC: "Michael S. Tsirkin"
>
> Only compile test since I don't have w2k to test with
>
> ---
> include/hw/i386/pc.h | 1 +
> hw/i386/acpi-build.c | 26 +++---
> hw/i386/pc_piix.c| 2 ++
> 3 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index d80859b..d6f65dd 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -122,6 +122,7 @@ struct PCMachineClass {
> bool rsdp_in_ram;
> int legacy_acpi_table_size;
> unsigned acpi_data_size;
> +bool force_rev1_fadt;
>
> /* SMBIOS compat: */
> bool smbios_defaults;
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 6b7bade..227f9ad 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -272,7 +272,7 @@ build_facs(GArray *table_data, BIOSLinker *linker)
> }
>
> /* Load chipset information in FADT */
> -static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
> +static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm, bool
> rev1)
> {
> fadt->model = 1;
> fadt->reserved1 = 0;
> @@ -304,6 +304,9 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt,
> AcpiPmInfo *pm)
> fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
> }
> fadt->century = RTC_CENTURY;
> +if (rev1) {
> +return;
> +}
>
> fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
> fadt->reset_value = 0xf;
> @@ -335,6 +338,7 @@ static void fadt_setup(AcpiFadtDescriptorRev3 *fadt,
> AcpiPmInfo *pm)
> /* FADT */
> static void
> build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
> + MachineState *machine,
>unsigned facs_tbl_offset, unsigned dsdt_tbl_offset,
>const char *oem_id, const char *oem_table_id)
> {
> @@ -342,6 +346,9 @@ build_fadt(GArray *table_data, BIOSLinker *linker,
> AcpiPmInfo *pm,
> unsigned fw_ctrl_offset = (char *)&fadt->firmware_ctrl - table_data->data;
> unsigned dsdt_entry_offset = (char *)&fadt->dsdt - table_data->data;
> unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - table_data->data;
> +PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> +int fadt_size = sizeof(*fadt);
> +int rev = 3;
>
> /* FACS address to be filled by Guest linker */
> bios_linker_loader_add_pointer(linker,
> @@ -349,16 +356,21 @@ build_fadt(GArray *table_data, BIOSLinker *linker,
> AcpiPmInfo *pm,
> ACPI_BUILD_TABLE_FILE, facs_tbl_offset);
>
> /* DSDT address to be filled by Guest linker */
> -fadt_setup(fadt, pm);
> +fadt_setup(fadt, pm, pcmc->force_rev1_fadt);
> bios_linker_loader_add_pointer(linker,
> ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
> ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> -bios_linker_loader_add_pointer(linker,
> -ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, sizeof(fadt->x_dsdt),
> -ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> +if (pcmc->force_rev1_fadt) {
> +rev = 1;
> +fadt_size = offsetof(typeof(*fadt), reset_register);
> +} else {
> +bios_linker_loader_add_pointer(linker,
> +ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, sizeof(fadt->x_dsdt),
> +ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> +}
>
> build_header(linker, table_data,
> - (void *)fadt, "FACP", sizeof(*fadt), 3, oem_id,
> oem_table_id);
> + (void *)fadt, "FACP", fadt_size, rev, oem_id, oem_table_id);
> }
>
> void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> @@ -2667,7 +2679,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState
> *machine)
> /* ACPI tables pointed to by RSDT */
> fadt = tables_blob->len;
> acpi_add_table(table_offsets, tables_blob);
> -build_fadt(tables_blob, tables->linker, &pm, facs, dsdt,
> +build_fadt(tables_blob, tables->linker, &pm, machine, facs, dsdt,
>slic_oem.id, slic_oem.table_id);
> aml_len += tables_blob->len - fadt;
>
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 11b4336..bc61332 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -449,9 +449,11 @@ DEFINE_I440FX_MACHINE(v2_10, "pc-i440fx-2.10", NULL,
>
> static void pc_i440fx_2_9_machine_options(MachineClass *m)
> {
> +PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> pc_i440fx_2_10_machine_options(m);
> m->is_default = 0;
> m->alias = NULL;
> +pcmc->for
Re: [Qemu-devel] [PATCH 2/2] file-posix: Do runtime check for ofd lock API
> From: Fam Zheng [mailto:f...@redhat.com] > Sent: Friday, 21 July 2017 3:21 > > It is reported that on Windows Subsystem for Linux, ofd operations fail > with -EINVAL. In other words, QEMU binary built with system headers that > exports F_OFD_SETLK doesn't necessarily run in an environment that > actually supports it: > > $ qemu-system-aarch64 ... -drive file=test.vhdx,if=none,id=hd0 \ > -device virtio-blk-pci,drive=hd0 > qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock > byte 100 > qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock > byte 100 > qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to lock > byte 100 > > Let's do a runtime check to cope with that. > > Reported-by: Andrew Baumann > Signed-off-by: Fam Zheng > --- > block/file-posix.c | 19 --- > 1 file changed, 8 insertions(+), 11 deletions(-) If it helps: Tested-By: Andrew Baumann Thanks! Andrew
Re: [Qemu-devel] [SeaBIOS] [RFC PATCH 0/2] Allow RedHat PCI bridges reserve more buses than necessary during init
On Fri, Jul 21, 2017 at 03:15:46PM +0300, Marcel Apfelbaum wrote: > On 21/07/2017 13:04, Gerd Hoffmann wrote: > > I'd prefer to have a single vendor capability for all resource > > allocation hints provided by qemu. > Sure, the capability looking something like: > >[flags: reserve-buses|reserve-IO|reserve-MEM|...] >[extra-buses][IO-size][MEM-size] > >if reserve-buses -> use 'extra-buses' value and so on I don't have any objection to using a PCI capability, but I do wonder if fw_cfg would be a better fit. This information is purely qemu -> firmware, right? -Kevin
[Qemu-devel] dirty page count problem
Hi,
Git bisect is pointing to your patch 084140bd49:
exec: fix access to ram_list.dirty_memory when sync dirty bitmap
trying to diagnose a bug I'm seeing; it looks like the dirty page count
is wrong for some reason.
Alex Bennée spotted a problem where the postcopy test would occasionally
fail under very heavy load;attaching a debugger and it looks like
the problem is we have a migration_dirty_page count stuck at 2;
in the normal migration tests we don't spot this, because 2 pages is
smaller than the threshold to end migration and so an extra 2 pages
doesn't block it finishing. However, with a very
small downtime setting (like we use in the postcopy test) and with
very low bandwidth (as when Alex ran the test on a very heavily loaded
machine) we end up never calling the bitmap sync again and never
completing the iteration.
I'm using the following addition to spot the problem:
diff --git a/migration/ram.c b/migration/ram.c
index e75f1050e4..3ddf884952 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1350,6 +1350,13 @@ static int ram_find_and_save_block(RAMState *rs, bool
last_stage)
}
} while (!pages && again);
+if (!pages && !again && pss.complete_round && rs->migration_dirty_pages)
+{
+/* Should make this fail migration ? */
+fprintf(stderr, "%s: no page found, yet dirty_pages=%"PRIu64"\n",
+__func__, rs->migration_dirty_pages);
+}
+
rs->last_seen_block = pss.block;
rs->last_page = pss.page;
(which I might add as a test to fail a migration)
That test fails easily even on an unloaded machine:
tests/postcopy-test
/x86_64/postcopy: ram_find_and_save_block: no page found, yet dirty_pages=2
ram_find_and_save_block: no page found, yet dirty_pages=2
ram_find_and_save_block: no page found, yet dirty_pages=2
OK
I'll try and debug where our extra two pages are coming from.
Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support
On Fri, 21 Jul 2017, Phil Dennis-Jordan wrote: On Fri, Jul 21, 2017 at 12:50 PM, BALATON Zoltan wrote: I don't know if this helps but I've found that this same commit also broke booting OS X on q35 with OVMF and Clover (some old versions I had and worked before this commit). See here: http://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg04306.html Maybe it's simpler to debug as these are open source but it's still not very clear why they fail because it starts to boot OS X but hangs during kernel init (and maybe Win2K fails to boot because of something else). I think I've seen this with OVMF 58f025afd and Clover 3354 (or around that, not completely sure about these versions). OVMF had 2 bugs that totally mucked up the ACPI tables if Qemu didn't supply its loader commands "just so", and modified ACPI 2.0-5.0 FADTs in such a way that they were no longer valid. These are fixed in 198a46d and 072060a, which are about a year newer than 58f025afd, so it's no surprise this combination doesn't work. I have no idea if Clover on top makes any difference. I've tried with 072060a and with that version OS X boots with same Clover so my problem seems to have been too old OVMF version and Clover does not make a difference here. So this also likely has nothing do with the Win2k problem discussed in this thread so you can disregard my post. Thank you, BALATON Zoltan
Re: [Qemu-devel] [PATCH 0/2] improve tracing
Vladimir Sementsov-Ogievskiy writes:
> Current trace system have a drawback: parameters of trace functions
> are calculated even if corresponding tracepoint is disabled. Also, it
> looks like trace function are not actually inlined by compiler (at
> least for me).
> Here is a fix proposal: move from function call to macros. Patch 02
> is an example, of how to reduce extra calculations with help of
> patch 01.
The tracing functions *were* inlined last time I checked, although things
changed quite a lot since then. Not sure that will make a lot of difference in
terms of overall performance (needs measuring).
As for arguments, each trace event has a define TRACE_{NAME}_ENABLED that you
can use for that purpose. If this is not explained in tracing.txt, that is a
documentation bug.
Thanks,
Lluis
Re: [Qemu-devel] [PATCH 6/9] tests/libqos/pci: Clean up string interpolation into QMP input
Eric Blake writes:
> On 07/21/2017 08:53 AM, Markus Armbruster wrote:
>> Leaving interpolation into JSON to qmp() is more robust than building
>> QMP input manually, as explained in the commit before previous.
>>
>> The case in qpci_plug_device_test() is a bit complicated: it
>> interpolates several JSON object members, not just a value. Clean it
>> up by passing them in as QDict rather than string, so we can leave
>> interpolation to qmp() here and to qobject_from_jsonf() in callers.
>>
>> Signed-off-by: Markus Armbruster
>> ---
>
>>
>> +response = qmp("{'execute': 'device_add', 'arguments': %p }", args);
>> +
>
> I like this; in fact, in my earlier series attempting to remove
> qobject_from_jsonf(), I heavily relied on %p being useful, to the point
> that I even added a helper function to make it easier (off-hand, it was
> something like qmp_execute(const char *command, QDict *arguments))
>
>> @@ -674,6 +676,7 @@ static void pci_hotplug(void)
>> if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>> qpci_unplug_acpi_device_test("drv1", PCI_SLOT_HP);
>> }
>> +
>> qtest_shutdown(qs);
>
> Spurious whitespace addition?
Is it spurious if the result looks better? ;)
There's a blank line after pci_test_start(), and that makes me want one
before the matching qtest_shutdown().
> Reviewed-by: Eric Blake
Thanks!
Re: [Qemu-devel] [PATCH 5/9] tests/libqos/usb: Clean up string interpolation into QMP input
Eric Blake writes:
> On 07/21/2017 08:53 AM, Markus Armbruster wrote:
>> Leaving interpolation into JSON to qmp() is more robust than building
>> QMP input manually, as explained in the previous commit.
>>
>> The case in usb_test_hotplug() slightly more complicated: it
>
> s/()/() is/
Will fix.
>> interpolates *into* JSON values. Clean it up by building the values
>> separately, so we can again leave interpolation to qmp().
>>
>> Signed-off-by: Markus Armbruster
>> ---
>> tests/libqos/usb.c | 30 ++
>> 1 file changed, 14 insertions(+), 16 deletions(-)
>>
>> diff --git a/tests/libqos/usb.c b/tests/libqos/usb.c
>> index 0cdfaec..f88d4a6 100644
>> --- a/tests/libqos/usb.c
>> +++ b/tests/libqos/usb.c
>> @@ -40,18 +40,20 @@ void uhci_port_test(struct qhc *hc, int port, uint16_t
>> expect)
>> void usb_test_hotplug(const char *hcd_id, const int port,
>>void (*port_check)(void))
>> {
>> +char id[32];
>> +char *bus;
>> QDict *response;
>> -char *cmd;
>>
>> -cmd = g_strdup_printf("{'execute': 'device_add',"
>> - " 'arguments': {"
>> - " 'driver': 'usb-tablet',"
>> - " 'port': '%d',"
>> - " 'bus': '%s.0',"
>> - " 'id': 'usbdev%d'"
>> - "}}", port, hcd_id, port);
>> -response = qmp(cmd);
>> -g_free(cmd);
>> +sprintf(id, "usbdev%d", port);
>
> I know this fits, but do any of our compilers issue dumb warnings about
> possible buffer overflow? I guess we'll deal with that if someone
> reports a problem.
We do similar things elsewhere, just grep for sprintf.
> Reviewed-by: Eric Blake
Thanks!
Re: [Qemu-devel] [PATCH 4/9] tests: Clean up string interpolation into QMP input (simple cases)
Eric Blake writes:
> On 07/21/2017 08:53 AM, Markus Armbruster wrote:
>> When you build QMP input manually like this
>>
>> cmd = g_strdup_printf("{ 'execute': 'migrate',"
>> "'arguments': { 'uri': '%s' } }",
>> uri);
>> rsp = qmp(cmd);
>> g_free(cmd);
>>
>> you're responsible for escaping the interpolated values for JSON. Not
>> done here, and therefore works only for sufficiently nice @uri. For
>> instance, if @uri contained a single "'", qobject_from_jsonv() would
>> fail, qmp_fd_sendv() would misinterpret the failure as empty input and
>> do nothing, and the test would hang waiting for a response that never
>> comes.
>>
>> Leaving interpolation into JSON to qmp() is more robust:
>>
>> rsp = qmp("{ 'execute': 'migrate', 'arguments': { 'uri': %s } }", uri);
>>
>> It's also more concise.
>>
>> Clean up the simple cases where we interpolate exactly a JSON value.
>>
>> Bonus: gets rid of non-literal format strings. A step towards
>> compile-time format string checking without triggering
>> -Wformat-nonliteral.
>>
>> Signed-off-by: Markus Armbruster
>
>> ---
>> +qmp_fd_send(fixture->fd,
>> +"\xff{'execute': 'guest-sync-delimited',"
>> +" 'arguments': {'id': %u } }",
>
> Ouch. Per 2/9:
> * json-lexer.c (only understands '%((l|ll|I64)?d|[ipsf])').
>
> That will need to be %d now. Or, more likely, we need to update the
> comments in json-lexer.c and in 2/9 (/me goes to read json-lexer.c...)
> /* escape */
> [IN_ESCAPE_LL] = {
> ['d'] = JSON_ESCAPE,
> ['u'] = JSON_ESCAPE,
> },
> ...
> [IN_ESCAPE] = {
> ['d'] = JSON_ESCAPE,
> ['i'] = JSON_ESCAPE,
> ['p'] = JSON_ESCAPE,
> ['s'] = JSON_ESCAPE,
> ['u'] = JSON_ESCAPE,
> ['f'] = JSON_ESCAPE,
> ['l'] = IN_ESCAPE_L,
> ['I'] = IN_ESCAPE_I,
>
> Aha, that 'd' should be '[du]' in all of the comments.
Yes. We really need %u, or else parse_escape() would call
qnum_from_int() instead of qnum_from_uint().
I think the doc fix for json-lexer.c can be squashed in. Keeping it
separate could simplify commit message writing, though. Your choice.
>> @@ -411,10 +408,10 @@ static void test_qga_file_ops(gconstpointer fix)
>>
>> enc = g_base64_encode(helloworld, sizeof(helloworld));
>> /* write */
>> -cmd = g_strdup_printf("{'execute': 'guest-file-write',"
>> - " 'arguments': { 'handle': %" PRId64 ","
>> - " 'buf-b64': '%s' } }", id, enc);
>> -ret = qmp_fd(fixture->fd, cmd);
>> +ret = qmp_fd(fixture->fd,
>> + "{'execute': 'guest-file-write',"
>> + " 'arguments': { 'handle': %" PRId64 ", 'buf-b64': %s } }",
>
> Ouch; you are reverting commit 1792d7d0. We tried hard to make
> json-lexer.c understand lots of different 64-bit format spellings, but
> we KNOW that we don't support MacOS (see commit 29a6731). We either
> need to beef up json-lexer.c to understand %qd, or get rid of JSON
> psuedo-strings, if we expect this to work; otherwise, you should use
> %lld and long long instead of PRId64 and uint64_t.
I forgot that PRId64 expands into non-standard crap on some systems.
Options:
1. Outlaw use of PRI macros, limit integer length modifiers for
conversion specifiers "d" and "u" to "l" and "ll". When PRI macros
creep in, the build breaks on hosts where they expand to anything
else (hopefully, our CI will catch that). Interpolating int64_t
values become a bit awkward.
Required work: fix my patches not to use PRId64, drop support for
length modifier "I64" from parse_escape().
2. Support PRId64 and PRIu64, whatever their actual value may be.
a. Support all possible values. This is what we've tried before.
Nasty: fails only at run-time on hosts with sufficiently creative
values.
Required work: add support for length modifier "q", to unbreak
MacOS.
Optional work: try to turn the run-time failure into a compile-
time failure, ideally in configure.
b. Support exactly the host's PRId64 and PRIu64 values.
Work required: replace support of "I64d" and "I64u" by support of
PRId64 and PRIu64. Easy enough in parse_escape(), but not in
json-lexer.c. We could perhaps have the lexer accept the shortest
string that's followed by a conversion specifier as length
modifier, then reject invalid length modifiers in a semantic
action.
Optional work: try to simplify code that currently works around
unavailability of PRId64 and PRIu64.
Preferences?
I like 2b, but I'm not sure I'll like the code implementing it. One way
to find out...
> Overall, I like the patch, but we need to fix the problems for the next
> round of this series.
Yes.
Re: [Qemu-devel] [PATCH 16/29] lm32: use QEMU_IS_ALIGNED macro
On 07/18/2017 11:37 AM, Thomas Huth wrote: On 18.07.2017 13:42, Michael Walle wrote: Am 2017-07-18 08:09, schrieb Philippe Mathieu-Daudé: Applied using the Coccinelle semantic patch scripts/coccinelle/use_osdep.cocci Signed-off-by: Philippe Mathieu-Daudé QEMU_IS_ALIGNED() sounds like it is used to check if a memory access is aligned. Although it does the same, the line in question is used for formatted output. I'm not sure if this macro should be used here. +1 I think we should not replace every usage of % blindly. It does really look wrong in this case here. Dropped, will wear my glasses next time ;)
Re: [Qemu-devel] [PATCH 10/29] net/rocker: use QEMU_IS_ALIGNED macro
On 07/18/2017 02:51 PM, Eric Blake wrote:
On 07/18/2017 01:09 AM, Philippe Mathieu-Daudé wrote:
Applied using the Coccinelle semantic patch scripts/coccinelle/use_osdep.cocci
Signed-off-by: Philippe Mathieu-Daudé
---
hw/net/rocker/rocker.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index 4f0f6d71e5..55228f2f52 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -1419,7 +1419,7 @@ static int pci_rocker_init(PCIDevice *dev)
desc_ring_set_consume(ring, cmd_consume, ROCKER_MSIX_VEC_CMD);
} else if (i == ROCKER_RING_EVENT) {
desc_ring_set_consume(ring, NULL, ROCKER_MSIX_VEC_EVENT);
-} else if (i % 2 == 0) {
+} else if (QEMU_IS_ALIGNED(i, 2)) {
desc_ring_set_consume(ring, tx_consume,
ROCKER_MSIX_VEC_TX((i - 2) / 2));
} else if (i % 2 == 1) {
Given the if chain, I think you don't want this one.
Indeed, dropped. Thanks for your review!
Re: [Qemu-devel] Improving QMP test coverage
On 07/21/2017 11:33 AM, Stefan Hajnoczi wrote: >> Output testing style delegates checking ouput to diff. I rather like it >> when text output is readily available. It is when testing QMP. A >> non-trivial example using this style could be useful, as discussing >> ideas tends to be more productive when they come with patches. > > Yes, I was considering how many of the Python iotests could be rewritten > comfortably in shell. It is nice when the test simply executes commands > and the output file shows the entire history of what happened. Great > for debugging. > > Stefan > I'd like to have a better understanding of the major pain points here. Although this can be seen as a matter of taste, style preferences and even religion, I guess it's safe to say that Python can scale better than shell. The upside of shell based tests is the "automatic" and complete logging, right? Running "bash -x /path/to/test.sh" will give much more *useful* information than "python -v /path/to/test.py" will, fact. I believe this has to do with how *generic* Python code is written, and how builtin functions and most of the standard Python libraries work as they do. Now, when writing code aimed at testing, making use of testing oriented libraries and tools, one would expect much more useful and readily available debug information. I'm biased, for sure, but that's what you get when you write basic tests using the Avocado libraries. For instance, when using process.run()[1] within a test, you can choose to see its command output quite easily with a command such as "avocado --show=avocado.test.stdout run test.py". Using other custom logging channels is also trivial (for instance for specific QMP communication)[2][3]. I wonder if such logging capabilities fill in the gap of what you describe as "[when the] output file shows the entire history of what happened". BTW, I'll defer the discussion of using an external tool to check the output and determine test success/failure, because it is IMO a complementary topic, and I believe I understand its use cases. Regards, - Cleber. [1] - http://avocado-framework.readthedocs.io/en/52.0/api/utils/avocado.utils.html#avocado.utils.process.run [2] - http://avocado-framework.readthedocs.io/en/52.0/WritingTests.html#advanced-logging-capabilities [3] - https://www.youtube.com/watch?v=htUbOsh8MZI -- Cleber Rosa [ Sr Software Engineer - Virtualization Team - Red Hat ] [ Avocado Test Framework - avocado-framework.github.io ] [ 7ABB 96EB 8B46 B94D 5E0F E9BB 657E 8D33 A5F2 09F3 ] signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [RFC] RFC on Backup tool
Ishani writes:
> Thanks for the review.
>
> - On Jul 17, 2017, at 12:48 PM, Fam Zheng f...@redhat.com wrote:
>
>> On Sun, 07/16 02:13, Ishani Chugh wrote:
>>> This is a Request For Comments patch for qemu backup tool. As an
>>> Outreachy intern, I am assigned to the project for creating a backup
>>> tool. qemu-backup will be a command-line tool for performing full and
>>> incremental disk backups on running VMs.
>>
>> Only full backup is implemented in this patch, is the plan to add incremental
>> backup on top? I'm curious because you have target file path set during
>> drive
>> add, but in the incremental case, it should be possible that each backup
>> creates
>> a new target file that chains up with earlier ones, so I think the target
>> file
>> should be an option for "backup" command as well.
>
> Yes. Incremental backup is to be added. I am still in learning phase with
> respect to incremental backups. I will modify the arguments and workflow
> accordingly.
>
>>> It is intended as a
>>> reference implementation for management stack and backup developers
>>> to see QEMU's backup features in action. The tool writes details of
>>> guest in a configuration file and the data is retrieved from the file
>>> while creating a backup. The usage is as follows:
>>> Add a guest
>>> python qemu-backup.py guest add --guest --qmp
>>> [--tcp]
>>>
>>> Add a drive for backup in a specified guest
>>> python qemu-backup.py drive add --guest --id
>>> [--target
>>> ]
>>>
>>> Create backup of the added drives:
>>> python qemu-backup.py backup --guest
>>>
>>> List all guest configs in configuration file:
>>> python qemu-backup.py guest list
>>
>> Please put these examples into the help output of the command, this way users
>> can read it as well.
>
> I have created a manpage documentation for the tool.
> http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg05241.html
> It is to be updated continuously as the development progresses.
>
>>>
>>> I will be obliged by any feedback.
>>
>> Thanks for doing this!
>>
>>>
>>> Signed-off-by: Ishani Chugh
>>> ---
>>> contrib/backup/qemu-backup.py | 244
>>> ++
>>> 1 file changed, 244 insertions(+)
>>> create mode 100644 contrib/backup/qemu-backup.py
>>>
>>> diff --git a/contrib/backup/qemu-backup.py b/contrib/backup/qemu-backup.py
>>> new file mode 100644
>>> index 000..9c3dc53
>>> --- /dev/null
>>> +++ b/contrib/backup/qemu-backup.py
>>> @@ -0,0 +1,244 @@
>>> +#!/usr/bin/python
>>> +# -*- coding: utf-8 -*-
>>
>> You need a copyright header here (the default choice for QEMU is GPLv2 but
>> there
>> is no strict restrictions for scripts). See examples in other *.py files.
>
> Thanks. Will update in next revision.
>
>>> +"""
>>> +This file is an implementation of backup tool
>>> +"""
>>> +from argparse import ArgumentParser
>>> +import os
>>> +import errno
>>> +from socket import error as socket_error
>>> +import configparser
>>
>> Python2 has ConfigParser while python3 has configparser. Please be specific
>> about the python compatibility level of this script - my system (Fedora) has
>> python2 as /usr/bin/python, so the shebang and your example command in the
>> commit message don't really work. "six" module can handle python 2/3
>> differentiations, or you can use '#!/usr/bin/env python2' to specify a python
>> version explicitly.
>
> The script is supposed to be both python2/3 compatible. I will take reference
> from Stefan's review and fix it in next revision.
>
>>> +import sys
>>> +sys.path.append('../../scripts/qmp')
>>
>> This expects the script to be invoked from its local directory? It's better
>> to
>> use the __file__ trick to allow arbitrary workdir:
>>
>> $ git grep __file__
>> scripts/device-crash-test:sys.path.append(os.path.join(os.path.dirname(__file__),
>> '..', 'scripts'))
>> scripts/qemu-gdb.py:sys.path.append(os.path.dirname(__file__))
>> tests/migration/guestperf/engine.py:sys.path.append(os.path.join(os.path.dirname(__file__),
>> '..', '..', '..', 'scripts'))
>> tests/qemu-iotests/iotests.py:sys.path.append(os.path.join(os.path.dirname(__file__),
>> '..', '..', 'scripts'))
>
> Thanks. Will fix it in next revision.
>
>>> +from qmp import QEMUMonitorProtocol
>>> +
>>> +
>>> +class BackupTool(object):
>>> +"""BackupTool Class"""
>>> +def __init__(self, config_file='backup.ini'):
>>
>> Is it better to put this in a more predictable place such as
>> "$HOME/.qemu-backup.ini" and/or make it a command line option?
>
> It is planned to be an optional command-line option, if not
> provided, the default location suggested should be taken.
Can it be $HOME/.config/qemu/backup.ini please as per:
https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html
Cheers,
--
Alex Bennée
Re: [Qemu-devel] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support
> On Jul 21, 2017, at 5:29 AM, qemu-devel-requ...@nongnu.org wrote: > > From: Igor Mammedov > > On Thu, 20 Jul 2017 21:29:33 +0200 > Phil Dennis-Jordan wrote: > >> On Thu, Jul 20, 2017 at 6:40 PM, Programmingkid >> wrote: >>> I noticed that Windows 2000 does not boot up in QEMU recently. After >>> bisecting the issue I found the offending commit: > w2k is very ancient (and long time EOLed), I can't even download it from msdn > to test > (oldest available is XP) If you really need to download Windows 2000, there may be other avenues you may want to try. > do we really care about it? Yes. It is a great operating system. Very light weight, fast, and stable. The perfect operating system to use in an emulator.
Re: [Qemu-devel] KVM "fake DAX" flushing interface - discussion
On Fri, Jul 21, 2017 at 09:29:15AM -0400, Pankaj Gupta wrote: > > > > A] Problems to solve: > > > -- > > > > > > 1] We are considering two approaches for 'fake DAX flushing interface'. > > > > > > 1.1] fake dax with NVDIMM flush hints & KVM async page fault > > > > > > - Existing interface. > > > > > > - The approach to use flush hint address is already nacked upstream. > > > > > > - Flush hint not queued interface for flushing. Applications might > > >avoid to use it. > > > > This doesn't contradicts the last point about async operation and vcpu > > control. KVM async page faults turn the Address Flush Hints write into > > an async operation so the guest can get other work done while waiting > > for completion. > > > > > > > > - Flush hint address traps from guest to host and do an entire fsync > > >on backing file which itself is costly. > > > > > > - Can be used to flush specific pages on host backing disk. We can > > >send data(pages information) equal to cache-line size(limitation) > > >and tell host to sync corresponding pages instead of entire disk > > >sync. > > > > Are you sure? Your previous point says only the entire device can be > > synced. The NVDIMM Adress Flush Hints interface does not involve > > address range information. > > Just syncing entire block device should be simple but costly. Using flush > hint address to write data which contains list/info of dirty pages to > flush requires more thought. This calls mmio write callback at Qemu side. > As per Intel (ACPI spec 6.1, Table 5-135) there is limit to max length > of data guest can write and is equal to cache line size. > > > > > > > > > - This will be an asynchronous operation and vCPU control is returned > > >quickly. > > > > > > > > > 1.2] Using additional para virt device in addition to pmem device(fake > > > dax > > > with device flush) > > > > Perhaps this can be exposed via ACPI as part of the NVDIMM standards > > instead of a separate KVM-only paravirt device. > > Same reason as above. If we decide on sending list of dirty pages there is > limit to send max size of data to host using flush hint address. I understand now: you are proposing to change the semantics of the Address Flush Hints interface. You want the value written to have meaning (the address range that needs to be flushed). Today the spec says: The content of the data is not relevant to the functioning of the flush hint mechanism. Maybe the NVDIMM folks can comment on this idea. signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] qemu-iotests: add a "how to" to ./README
On Fri, Jul 21, 2017 at 07:16:34AM -0500, Eric Blake wrote: > On 07/21/2017 04:34 AM, Stefan Hajnoczi wrote: > > There is not much getting started documentation for qemu-iotests. This > > patch explains how to create a new test and covers the overall testing > > approach. > > > > Cc: Ishani Chugh > > Signed-off-by: Stefan Hajnoczi > > --- > > > +3. Assign groups to the test > > + > > +Add your test to the ./group file. This file is the index of tests and > > assigns > > +them to functional groups like "rw" for read-write tests. Most tests > > belong to > > +the "rw" and "auto" groups. "auto" means the test runs when ./check is > > invoked > > +without a -g argument. > > + > > +Consider adding your test to the "quick" group if it executes quickly > > (<1s). > > We have several tests going up to 5s (and I have a patch pending to > remove two tests that took longer) - I think 1s is a bit on the short > end for still classifying a test as quick. I'm happy to accept any number blessed by Kevin. I do think that 1 second is a safe maximum and no one should get in trouble for adding a test that takes 1 second to the "quick" group. > > +This group is run by "make check-block" and is often included as part of > > build > > +tests in continuous integration systems. > > It would still be nice to have 'make check' run 'make check-block'... > but that's independent of this patch. Yes, there is a separate discussion about that on the list right now. Hopefully it will be added back. > > +Once you are happy with the test output it can be used as the golden master > > +with "mv .out.bad .out". Rerun the test to > > verify > > +that it passes. > > + > > +Congratulations, you've created a new test! > > Maybe a reminder to 'git add' the new files, then submit the patch? I thought about this too but decided not to get into the git and patch submission business. I carefully worded it to be about "creating" tests rather than "adding" them to qemu.git because I wanted to limit the scope of this README :). Stefan signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH 2/9] qtest: Document calling conventions
Eric Blake writes:
> On 07/21/2017 08:53 AM, Markus Armbruster wrote:
>> From: Eric Blake
>>
>> We have two flavors of vararg usage in qtest; make it clear that
>> qmp() has different semantics than hmp(), and let the compiler
>> enforce that hmp() is used correctly. However, qmp() (and friends)
>> only accept a subset of printf flags look-alikes (namely, those
>> that our JSON parser understands), and what is worse, qmp("true")
>> (the JSON keyword 'true') is different from qmp("%s", "true")
>> (the JSON string '"true"'), so marking those as printf-like would
>> produce more harm from bogus warnings than it helps (we may have
>> made a mistake in previously marking qobject_from_jsonf(), but
>> this patch is not addressing that).
>>
>> Signed-off-by: Eric Blake
>> Message-Id: <20170720214008.28494-5-ebl...@redhat.com>
>> Signed-off-by: Markus Armbruster
>
> As you pointed out on the other thread,
>
>> @@ -134,7 +137,7 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const char
>> *event);
>> /**
>> * qtest_hmp:
>> * @s: #QTestState instance to operate on.
>> - * @fmt...: HMP command to send to QEMU
>> + * @fmt...: HMP command to send to QEMU, formats arguments like vsprintf().
>> *
>> * Send HMP command to QEMU via QMP's human-monitor-command.
>> * QMP events are discarded.
>
> I accidentally killed the attribute here,
>
>> @@ -592,7 +598,7 @@ static inline QDict *qmp_eventwait_ref(const char *event)
>>
>> /**
>> * hmp:
>> - * @fmt...: HMP command to send to QEMU
>> + * @fmt...: HMP command to send to QEMU, formats arguments like vsprintf().
>> *
>> * Send HMP command to QEMU via QMP's human-monitor-command.
>> *
>
> and here.
Putting them back is easier than updating commit message 1/9.
Re: [Qemu-devel] [Qemu-block] [PATCH] block: check BlockDriverState object before dereference
On Mon, Jul 17, 2017 at 11:56:48AM -0400, John Snow wrote: > On 07/11/2017 01:08 PM, P J P wrote: > > From: Prasad J Pandit > > > > When processing ATA_CACHE_FLUSH ide controller command, > > BlockDriverState object could be null. Add check to avoid > > null pointer dereference. > > > > This happens through 0xE7, what QEMU calls WIN_CACHE_FLUSH and described > in ATA8 ACS3 as "FLUSH CACHE" > > The core of the matter here is that any ATA device associated with a > BlockBackend that has no media inserted will accept the command and call > blk_aio_flush, which will later fail because blk_aio_prwv assumes that > the BlockBackend it was given definitely has a BlockDriverState attached. > > The associated bdrv_inc_in_flight causes the null pointer dereference. > > It's not immediately clear to me what the right fix is here: > > (1) Should blk_ functions not assume they will have a BlockDriverState? > (i.e. should an attempted blk_flush on an empty blk just succeed > quietly, or is that inherently an error?) Hmm...BlockDriverState still has bdrv_is_inserted() even though BlockBackend->root can be NULL? CCing Markus in case he has thoughts on the BB/BDS split. I find it weird that block-backend.c calls bdrv_inc_in_flight() and then bdrv_co_flush() will call it again. Perhaps we need to do this so that blk_drain() works correctly but it's odd that they share the same counter variable. signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] qemu-iotests: Avoid unnecessary sleeps
On 07/21/2017 10:10 AM, Kevin Wolf wrote: > Test cases 030, 041 and 055 used to sleep for a second after calling > block-job-pause to make sure that the block job had time to actually > get into paused state. We can instead poll its status and use that one > second only as a timeout. > > The tests also slept a second for checking that the block jobs don't > make progress while being paused. Half a second is more than enough for > this. > > These changes reduce the total time for the three tests by 25 seconds on > my laptop (from 155 seconds to 130). Nice! > > Signed-off-by: Kevin Wolf > --- Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] Improving QMP test coverage
On Tue, Jul 18, 2017 at 06:24:19PM +0200, Markus Armbruster wrote: > Stefan Hajnoczi writes: > > > On Thu, Jul 13, 2017 at 05:28:52PM +0200, Markus Armbruster wrote: > >> What can we do to improve QMP testing? Sadly, I don't have the master > >> plan ready. I can tell people their new code needs to come with tests, > >> but that won't help much unless subsystem maintainers pick up the habit, > >> too. There are a few obvious tests to write for old code, such as a > >> generic test of query-like commands without arguments and without side > >> effects, similar to what test-hmp.c does for HMP command info (I hope to > >> get around to that one). But for much of the old code, we don't even > >> know where the test coverage holes are. > >> > >> Ideas anyone? > > > > It makes sense for maintainers to ask that new QMP commands come with > > comprehensive tests. > > > > For me the main question is how to begin? What is the easiest and > > preferred way to write QMP command test cases? > > > > Today the most common approach is a qtest test case that sends commands > > and verifies that the expected response is returned. This approach > > works but we could benefit from a discussion about the alternatives > > (e.g. qemu-iotests style shell scripts with output diffing). > > Output testing style delegates checking ouput to diff. I rather like it > when text output is readily available. It is when testing QMP. A > non-trivial example using this style could be useful, as discussing > ideas tends to be more productive when they come with patches. Yes, I was considering how many of the Python iotests could be rewritten comfortably in shell. It is nice when the test simply executes commands and the output file shows the entire history of what happened. Great for debugging. Stefan signature.asc Description: PGP signature
Re: [Qemu-devel] [RFC] RFC on Backup tool
On Tue, Jul 18, 2017 at 05:29:11PM -0400, John Snow wrote: > > > On 07/17/2017 03:37 PM, Ishani wrote: > > - On Jul 17, 2017, at 12:48 PM, Fam Zheng f...@redhat.com wrote: > >> On Sun, 07/16 02:13, Ishani Chugh wrote: > > [...] > > >> Only full backup is implemented in this patch, is the plan to add > >> incremental > >> backup on top? I'm curious because you have target file path set during > >> drive > >> add, but in the incremental case, it should be possible that each backup > >> creates > >> a new target file that chains up with earlier ones, so I think the target > >> file > >> should be an option for "backup" command as well. > > > > Yes. Incremental backup is to be added. I am still in learning phase with > > respect to incremental backups. I will modify the arguments and workflow > > accordingly. > > > > You may consider solidifying the backup target *pattern* during drive > add as an alternative, such as: > > .../path/to/backup/%VM%/%DRIVE%/%%-%mm%-%dd%.qcow2 > > Or some such scheme. Simple numerals work well, too: > > myvm/sda/incr.0.qcow2 > myvm/sda/incr.1.qcow2 > > Simple numerals offer the benefit that it is easier to reconstruct the > chain if you lose your metadata in the python script. > > Also consider that even for non-incremental backups, we want full > backups made subsequently to not, in general, overwrite the previous > full backup, so the TARGET is more of a "living entity" than a fixed > thing, even in the simple case. Patterns would be nice. You may find string.Template() useful: https://docs.python.org/2.7/library/string.html#template-strings https://docs.python.org/3/library/string.html#template-strings signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] block/vpc: fix uninitialised variable compiler warning
On 21 July 2017 at 09:21, Mark Cave-Ayland wrote: > Since commit cfc87e00 "block/vpc.c: Handle write failures in > get_image_offset()" older versions of gcc (in this case 4.7) incorrectly > warn that "ret" can be used uninitialised in vpc_co_pwritev(). > > Setting ret to 0 at the start of vpc_co_pwritev() prevents the warning > in gcc 4.7 and enables compilation with -Werror to succeed. > > Signed-off-by: Mark Cave-Ayland > --- > block/vpc.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/vpc.c b/block/vpc.c > index 10e6519..574879b 100644 > --- a/block/vpc.c > +++ b/block/vpc.c > @@ -649,7 +649,7 @@ vpc_co_pwritev(BlockDriverState *bs, uint64_t offset, > uint64_t bytes, > int64_t image_offset; > int64_t n_bytes; > int64_t bytes_done = 0; > -int ret; > +int ret = 0; > VHDFooter *footer = (VHDFooter *) s->footer_buf; > QEMUIOVector local_qiov; > Thanks; applied to master as a buildfix. -- PMM
Re: [Qemu-devel] [PATCH for-2.10 3/4] bsd-user/bsdload.c: Remove write-only id_change variable
On 21 July 2017 at 12:12, Thomas Huth wrote: > On 18.07.2017 18:26, Peter Maydell wrote: >> On OpenBSD the compiler complains: >> bsd-user/bsdload.c:54:17: warning: variable 'id_change' set but not used >> [-Wunused-but-set-variable] >> >> This is dead code that was originally copied from linux-user. >> We fixed this in linux-user in commit 331c23b5ca44293d1 in 2011; >> delete the useless code from bsd-user too. >> >> Signed-off-by: Peter Maydell >> --- > > Matches the commit from linux-user, and looks sane to me, so: > > Reviewed-by: Thomas Huth Thanks; applied to master. -- PMM
Re: [Qemu-devel] [PATCH] configure: Never use 'uname' to identify target OS
On 13 July 2017 at 16:15, Peter Maydell wrote: > For a very long time we have used 'uname -s' as our fallback if > we don't identify the target OS using a compiler #define. This > obviously doesn't work for cross-compilation, and we've had > a comment suggesting we fix this in configure for a long time. > Since we now have an exhaustive list of which OSes we can run > on (thanks to commit 898be3e0415 making an unrecognized OS > be a fatal error), we know which ones we're missing. > > Add check_define tests for the remaining OSes we support. The > defines checked are based on ones we already use in the codebase for > identifying the host OS (with the exception of GNU/kFreeBSD). > We can now set bogus_os immediately rather than doing it later. > > We leave the comment about uname being bad untouched, since > there is still a use of it for the fallback for unrecognized > host CPU type. > > Signed-off-by: Peter Maydell > --- Applied, thanks. -- PMM
Re: [Qemu-devel] [PATCH for-2.10 4/4] bsd-user/main.c: Fix unused variable warning
On 21 July 2017 at 13:23, Eric Blake wrote: > On 07/19/2017 03:19 AM, Peter Maydell wrote: >> On 18 July 2017 at 23:01, Eric Blake wrote: >>> On 07/18/2017 11:26 AM, Peter Maydell wrote: On OpenBSD the compiler warns: bsd-user/main.c:622:21: warning: variable 'sig' set but not used [-Wunused-but-set-variable] This is because a lot of the signal delivery code is #if-0'd out as unused. Reshuffle #ifdefs a bit to silence the warning. >>> >>> Why not just nuke the #if 0 code instead (we can always 'git revert' it >>> later as a starting point for someone that wants to implement it). >> >> I went for the minimal change, especially given we know >> that the freebsd folks have a big patchset on top of >> us fixing a lot of bsd-user code and it didn't seem worth >> giving them a more awkward rebase task. > > Perhaps worth mentioning in the commit message that keeping the #if 0 is > intentional, then. At any rate, > > Reviewed-by: Eric Blake Thanks; applied to master with an updated commit message. -- PMM
Re: [Qemu-devel] [PATCH] configure: Drop ancient Solaris 9 and earlier support
On 13 July 2017 at 15:21, Peter Maydell wrote: > Solaris 9 was released in 2002, its successor Solaris 10 was > released in 2005, and Solaris 9 was end-of-lifed in 2014. > Nobody has stepped forward to express interest in supporting > Solaris of any flavour, so removing support for the ancient > versions seems uncontroversial. > > In particular, this allows us to remove a use of 'uname' > in configure that won't work if you're cross-compiling. > > Signed-off-by: Peter Maydell > --- > Not a big thing, but it's a start on cleaning out some > of the untested and untestable cruft from configure... > Applied to master, thanks. -- PMM
