Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Wed, 16 Apr 2014 20:32:07 +0300 "Michael S. Tsirkin" wrote: > On Wed, Apr 16, 2014 at 05:42:22PM +0100, Peter Maydell wrote: > > On 16 April 2014 17:34, Michael S. Tsirkin wrote: > > > so it looks like virtio is currently compiled per-target. > > > So why isn't it reasonable to keep it per-target for > > > purpose of this enhancement? > > > What am I missing? > > > > "virtio" is more than one C file. Currently per-target: > > hw/virtio/virtio.c > > hw/virtio/virtio-balloon.c > > hw/scsi/virtio-scsi.c > > hw/block/virtio-blk.c > > hw/char/virtio-serial-bus.c > > hw/net/virtio-net.c > > > > Currently built once: > > hw/char/virtio-console.c > > hw/virtio/virtio-bus.c > > hw/virtio/virtio-mmio.c > > hw/virtio/virtio-pci.c > > hw/virtio/virtio-rng.c > > > > If we can move files from the first group to the second > > that's cool. > > But doesn't have to be part of this series, yes? > I would say let's at least have virtio 1.0 out > and implemented in qemu and linux guests, then > we can start deprecate virtio 0.X in favor of it. > > > Moving files from the second to the first is > > what I'd like to avoid... > > > > thanks > > -- PMM > > Absolutely. > > Looks like we are in agreement after all. > > So as far as I could see the only issue is with config access > e.g. in virtio-pci? > That one already has a branch around virtio_is_big_endian, > so it's not a problem, there's no need to optimize that. > It is because you haven't looked at the other patches... When the whole serie is applied, all virtio files use inlined helpers from virtio-access.h to access memory and fix endianness. That is why we'd rather not add target dependency here... This being said, if you have an insight to have branch-less code for fixed endian targets, and dedicated code for ambivalent endian targets, please send something. Cheers. -- Greg
Re: [Qemu-devel] [PATCH v1 0/3] Introduce qemu_get_boot_opts()
Peter Crosthwaite writes: > On Wed, Apr 16, 2014 at 6:05 PM, Markus Armbruster wrote: >> Peter Crosthwaite writes: >> >>> Hi Markus, >>> >>> This series introduces qemu_get_boot_opts(), in much the same way as >>> was done for qemu_get_machine_opts(). >>> >>> As usual, I have out-of-scope and out-of-tree usages :) But P3 does >>> clean up the one existing instance of the long-and-awkward form of >>> this query and makes it consistent with an immediately surrounding >>> qemu_get_machine_opts(). >> >> I doubt this is worthwhile on its own as it stands. >> >> However, you missed the two uses of "boot-opts" in hw/nvram/fw_cfg.c. >> Since these uses are currently wrong the same way as the the uses of >> "machine" fixed in commit 36ad0e9 were, covering them could strengthen >> your case quite a bit, >> > > Yeh I noticed it, andI thought that "get the first list element" code > was weird. And I decided to let sleeing dogs lie. But are you saying > its wrong and we can just simplify to: > > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -125,18 +125,16 @@ static void fw_cfg_bootsplash(FWCfgState *s) > const char *temp; > > /* get user configuration */ > -QemuOptsList *plist = qemu_find_opts("boot-opts"); > -QemuOpts *opts = QTAILQ_FIRST(&plist->head); > +QemuOpts *opts = qemu_get_boot_opts(); > > ? Happy to make this change. Also drop the if (opts != NULL). Suggest to pattern the commit message after commit 36ad0e9, or at least reference it.
Re: [Qemu-devel] [PATCH v1 1/3] qdev: Expose the qdev id string as a prop
Andreas Färber writes: > Am 16.04.2014 08:42, schrieb Markus Armbruster: >> Peter Crosthwaite writes: >> >>> On Wed, Apr 16, 2014 at 2:16 AM, Andreas Färber wrote: Am 15.04.2014 04:21, schrieb Peter Crosthwaite: > So clients can set the top level id string. > > Signed-off-by: Peter Crosthwaite Anthony had nack'ed Paolo's attempt to generalize the qdev id to QOM, so I'm not sure if we should really do this even if just on device level. The id= is used to construct the canonical path, and we can't change that through a qdev setter. >> >> Let me try to paraphrase to make sure I got you: The canonical QOM path >> is fixed at creation time. Setting an ID dynamically would destroy the >> relationship between ID and canonical QOM path. Correct? > > Kind of. > > If we use, say, qemu-system-x86_64 -device nec-usb-xhci, then it ends up > as /machine/peripheral-anon/device[0] currently. No problem. > > If we instead use qemu-system-x86_64 -device nec-usb-xhci,id=foo, then > it becomes /machine/peripheral/foo, with "foo" being the name of the > child<> property named after the id= value. > Not only can there be just one canonical path, each object can only have > just one parent. So either /machine/peripheral/foo remains when renamed > to ID bar, or we need to destroy .../foo and then re-create .../bar. I'm > not aware of any precedent for the latter, so it would require some care > to not let the object die for lack of references in the middle of the > operation, for instance, or to survive unparenting at all due to bad > programmer assumptions. Moving a live device around doesn't strike me as a good idea. > While at QMP level link<> properties are treated as string paths, at QOM > level they are represented as C pointers and the canonical path is > reconstructed in the property getter. I.e., link<>s would not break if > the canonical path changes, but a QMP user may get confused. Indeed. Thanks!
Re: [Qemu-devel] Turning off default storage devices?
Andy Lutomirski writes: > On Mon, Apr 14, 2014 at 1:15 AM, Markus Armbruster wrote: >> Peter Crosthwaite writes: >> >>> Hi Andy, >>> >>> On Thu, Apr 10, 2014 at 5:55 AM, Andy Lutomirski >>> wrote: Currently, -M q35 boots linux quite a bit slower than the default machine type. This seems to be because it takes a few hundred ms to determine that there's nothing attached to the AHCI controller. In virtio setups, there will probably never be anything attached to the AHCI controller. Would it be possible to add something like -machine default_storage=off to turn off default storage devices? This could include the AHCI on q35 and the cdrom and such on pc. There's precedent: -machine usb=off turns off the default USB controllers, which is great for setups that use xhci. >>> >>> Is there a more generic solution to your problem? Can you implement >>> command line device removal in a non specific way and avoid having to >>> invent AHCI or even "storage" specific arguments. You could >>> considering bringing the xhci use case you mentioned under the same >>> umbrella. >> >> USB has always been off by default, at least for the boards I'm familiar >> with, due to the USB emulation's non-trivial CPU use. >> >> There's no such thing as a Q35 board without USB in the physical world. >> Can't stop us from making a virtual one, of course. >> >> Likewise, there's no such thing as a Q35 board without AHCI in the >> physical world, and again that can't stop us from making a virtual one. >> >> The difference to USB is that our q35 machines have always had AHCI even >> with -nodefaults. You seem to propose adding a switch to disable AHCI, >> yet leave it enabled with -nodefaults. >> >> -nodefaults should give you a board with all the optional components >> suppressed. > > Will this break libvirt, which may expect -nodefaults to still come > with an IDE bus? Possibly. >> On the one hand, I'd rather not add exceptions to -nodefaults "give me >> the board with all its optional components suppressed" semantics. >> >> On the other hand, a few hundred ms are a long time. > > That's why I proposed a new option. Yes, it's ugly :/ Does real hardware take that long, too? Or is the delay some virtualization artifact?
Re: [Qemu-devel] [PATCH 2/7] megasas: Enable MSI-X support
On 04/16/2014 07:52 PM, Michael S. Tsirkin wrote: > On Wed, Apr 16, 2014 at 07:47:37PM +0200, Andreas Färber wrote: >> Am 16.04.2014 19:40, schrieb Michael S. Tsirkin: >>> On Wed, Apr 16, 2014 at 06:48:08PM +0200, Andreas Färber wrote: Am 16.04.2014 18:32, schrieb Alexander Graf: > > On 16.04.14 16:44, Hannes Reinecke wrote: >> MSI-X support has been fixed in qemu, so we can enable it again. >> >> Signed-off-by: Hannes Reinecke >> --- >> hw/scsi/megasas.c | 19 ++- >> 1 file changed, 6 insertions(+), 13 deletions(-) >> >> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c >> index 1781525..df45286 100644 >> --- a/hw/scsi/megasas.c >> +++ b/hw/scsi/megasas.c >> @@ -2084,7 +2084,8 @@ static const VMStateDescription vmstate_megasas = { >> .minimum_version_id = 0, >> .minimum_version_id_old = 0, >> .fields = (VMStateField[]) { >> -VMSTATE_PCI_DEVICE(parent_obj, MegasasState), >> +VMSTATE_PCIE_DEVICE(parent_obj, MegasasState), >> +VMSTATE_MSIX(parent_obj, MegasasState), > > This requires a version change for vmstate, no? The PCI -> PCIE change yes. mst might have objections for bumping an x86 PCI device? >>> >>> Yes. >>> It should be possible to make this conditional on having a pcie >>> capability, and disable pcie capability for old pc versions. >> >> I did have a patch merging their vmstate_ structs based on some test in >> the series I ping'ed, unfortunately the difference was mostly config >> space size, so not immediately obvious how to set via compat_props, but >> likely solvable somehow. > > Check pci_is_express ? > The MSIX addition should be safe AFAICT since old versions would not enable MSI-X. >>> >>> Yes but I don't see a code like this - where's code in PC >>> disabling msix for old versions? >> >> The default value for use_msix (use-msix?) in v2 is already false, so no >> need to set it to false again in compat_props? >> >> Andreas > > Ah, I didn't notice that. Sure, if it's off by default there's > no need for compat code. > And the net result is ... what? Do I need to change the code? Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
Re: [Qemu-devel] [PATCH 13/35] pc: initialize memory hotplug address space
On Wed, Apr 16, 2014 at 04:23:44PM +0200, Igor Mammedov wrote: > On Wed, 16 Apr 2014 16:59:25 +0800 > Hu Tao wrote: > > > On Fri, Apr 04, 2014 at 03:36:38PM +0200, Igor Mammedov wrote: > > > initialize and map hotplug memory address space container > > > into guest's RAM address space. > > > > > > Signed-off-by: Igor Mammedov > > > --- > > > hw/i386/pc.c | 19 +-- > > > include/hw/i386/pc.h | 10 ++ > > > 2 files changed, 27 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > index 32b4003..69e4225 100644 > > > --- a/hw/i386/pc.c > > > +++ b/hw/i386/pc.c > > > @@ -1171,6 +1171,9 @@ FWCfgState *pc_memory_init(MemoryRegion > > > *system_memory, > > > MemoryRegion *ram, *option_rom_mr; > > > MemoryRegion *ram_below_4g, *ram_above_4g; > > > FWCfgState *fw_cfg; > > > +ram_addr_t ram_size = below_4g_mem_size + above_4g_mem_size; > > > +MachineState *machine = MACHINE(qdev_get_machine()); > > > +PCMachineState *pcms = PC_MACHINE(machine); > > > > > > linux_boot = (kernel_filename != NULL); > > > > > > @@ -1179,8 +1182,7 @@ FWCfgState *pc_memory_init(MemoryRegion > > > *system_memory, > > > * with older qemus that used qemu_ram_alloc(). > > > */ > > > ram = g_malloc(sizeof(*ram)); > > > -memory_region_init_ram(ram, NULL, "pc.ram", > > > - below_4g_mem_size + above_4g_mem_size); > > > +memory_region_init_ram(ram, NULL, "pc.ram", ram_size); > > > vmstate_register_ram_global(ram); > > > *ram_memory = ram; > > > ram_below_4g = g_malloc(sizeof(*ram_below_4g)); > > > @@ -1197,6 +1199,19 @@ FWCfgState *pc_memory_init(MemoryRegion > > > *system_memory, > > > e820_add_entry(0x1ULL, above_4g_mem_size, E820_RAM); > > > } > > > > > > +/* initialize hotplug memory address space */ > > > +if (ram_size < machine->init_args.maxram_size) { > > > +ram_addr_t hotplug_mem_size = > > > +machine->init_args.maxram_size - ram_size; > > > + > > > +pcms->hotplug_memory_base = > > > +ROUND_UP(0x1ULL + above_4g_mem_size, 1ULL << 30); > > > > -m maxmem should be limited otherwise hotplug_memory_base + maxmem can > > overflow(in dimm_get_free_addr()). > If overflow happens than dimm_get_free_addr() will return error, > if you look its end. > > dimm_get_free_addr() { > ... > if (new_start < address_space_start) { > error_setg(... > > > > that of cause doesn't mean that maxmem limit shouldn't be set and verified > with proper error reporting in case of one. Yeah. With command line -object memory-ram,id=foo,size=512M -m 512M,slots=4,maxmem=17179869183G It reports an error "can't add memory [0x1:0x2000] beyond 0xa000" when hotplugging memory-ram foo but shouldn't. > Is there any suggestion as to what max supported RAM amount should be? max ram shouldn't exceed unused memory range above 4g (UINT64_MAX - 0x1 - above_4g_mem_size), that is: maxram_size - ram_size <= UINT64_MAX - 0x1 - above_4g_mem_size Following patch does the check: diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 1329a50..f55e8c6 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -68,6 +68,20 @@ static bool smbios_type1_defaults = true; static bool gigabyte_align = true; static bool has_reserved_memory = true; +ram_addr_t get_above_4g_mem_size(ram_addr_t ram_size) +{ +ram_addr_t above_4g_mem_size; + +if (ram_size >= 0xe000) { +ram_addr_t lowmem = gigabyte_align ? 0xc000 : 0xe000; +above_4g_mem_size = ram_size - lowmem; +} else { +above_4g_mem_size = 0; +} + +return above_4g_mem_size; +} + /* PC hardware initialisation */ static void pc_init1(QEMUMachineInitArgs *args, int pci_enabled, diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 88efdaa..b03fb64 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -273,6 +273,7 @@ void pc_system_firmware_init(MemoryRegion *rom_memory, /* pvpanic.c */ uint16_t pvpanic_port(void); +ram_addr_t get_above_4g_mem_size(ram_addr_t ram_size); /* e820 types */ #define E820_RAM1 diff --git a/vl.c b/vl.c index 8f7b04e..095068e 100644 --- a/vl.c +++ b/vl.c @@ -3378,6 +3378,14 @@ int main(int argc, char **argv, char **envp) exit(EXIT_FAILURE); } +ram_addr_t above_4g_mem = get_above_4g_mem_size(ram_size); +if (sz - ram_size > UINT64_MAX - 0x1 - above_4g_mem) { +fprintf(stderr, "qemu: invalid -m option value: maxmem " +"(%" PRIu64 ") out of range.\n", sz); +fprintf(stderr, "maxmem: %" PRIu64 "\n", UINT64_MAX - 0x1 - above_4g_mem + ram_size); +exit(EXIT_FAILURE); +} + s
[Qemu-devel] About SIG_IPI handler
Hi, all I’m trying to figure out how do_savevm works in QEMU. But there is one thing has bothered me quite a lot. I found that vm_stop invoke qemu_cpu_kick_thread to send SIG_IPI to a vcpu thread, and I have understand that in TCG mode, the cpu_signal() function will be invoked as the SIG_IPI handler. But I don’t know what happens in KVM mode. Actually I can’t find the signal handler function. I only find a function named dummy_signal, and it doesn't do anything. Thanks a lot! -- - Shiru Ren Department of Computer Science School of EECS Peking University Beijing, China
Re: [Qemu-devel] About SIG_IPI handler
On 2014-04-17 07:46, Shiru Ren wrote: > Hi, all > > I’m trying to figure out how do_savevm works in QEMU. But there is one > thing has bothered me quite a lot. I found that vm_stop invoke > qemu_cpu_kick_thread to send SIG_IPI to a vcpu thread, and I have > understand that in TCG mode, the cpu_signal() function will be invoked as > the SIG_IPI handler. But I don’t know what happens in KVM mode. Actually I > can’t find the signal handler function. I only find a function named > dummy_signal, and it doesn't do anything. This signal is handled synchronously in KVM mode, see qemu_kvm_eat_signals in cpus.c. Jan signature.asc Description: OpenPGP digital signature
[Qemu-devel] [Bug 1257352] Re: kvm hangs occasionally when switching out of the qemu console
A-ha, the reason is that this only triggers if the qemu window is focused. Running the script while focusing does reproduce (and do other weird things). So perhaps this is happening in sdl_grab_start(), which exits early if the app is not focused? -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1257352 Title: kvm hangs occasionally when switching out of the qemu console Status in QEMU: New Status in “qemu” package in Ubuntu: Confirmed Bug description: To recreate (although this does *NOT* fail most of the time alas): 1) press "ctrl-alt-2" to switch to the qemu console. 2) type say "sendkey ctrl-alt-f1" 3) press "ctrl-alt-1". Expected outcome: Switch to tty1 in the VM. Actual outcome: No switch to tty1 in the VM. and qemu console unresponsive to any keyboard input. Rather a vague problem description I'm afraid but this has happened to me 3 times recently. No crash and no excessive CPU is observed. I'll grab an strace when it happens again and attach... ProblemType: Bug DistroRelease: Ubuntu 14.04 Package: qemu-system-x86 1.6.0+dfsg-2ubuntu4 ProcVersionSignature: Ubuntu 3.12.0-4.12-generic 3.12.1 Uname: Linux 3.12.0-4-generic i686 NonfreeKernelModules: nvidia ApportVersion: 2.12.7-0ubuntu1 Architecture: i386 CurrentDesktop: Unity Date: Tue Dec 3 15:41:40 2013 InstallationDate: Installed on 2010-10-21 (1139 days ago) InstallationMedia: Ubuntu 10.10 "Maverick Meerkat" - Release i386 (20101007) SourcePackage: qemu UpgradeStatus: Upgraded to trusty on 2013-11-01 (31 days ago) To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1257352/+subscriptions
[Qemu-devel] [Bug 1257352] Re: kvm hangs occasionally when switching out of the qemu console
Maddeningly, I've not yet been able to reproduce this by doing for i in `seq 1 100`; do xdotool search --name qemu keydown ctrl+alt+2 xdotool search --name qemu keyup ctrl+alt+2 xdotool search --name qemu keydown ctrl+alt+1 xdotool search --name qemu keyup ctrl+alt+1 done -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1257352 Title: kvm hangs occasionally when switching out of the qemu console Status in QEMU: New Status in “qemu” package in Ubuntu: Confirmed Bug description: To recreate (although this does *NOT* fail most of the time alas): 1) press "ctrl-alt-2" to switch to the qemu console. 2) type say "sendkey ctrl-alt-f1" 3) press "ctrl-alt-1". Expected outcome: Switch to tty1 in the VM. Actual outcome: No switch to tty1 in the VM. and qemu console unresponsive to any keyboard input. Rather a vague problem description I'm afraid but this has happened to me 3 times recently. No crash and no excessive CPU is observed. I'll grab an strace when it happens again and attach... ProblemType: Bug DistroRelease: Ubuntu 14.04 Package: qemu-system-x86 1.6.0+dfsg-2ubuntu4 ProcVersionSignature: Ubuntu 3.12.0-4.12-generic 3.12.1 Uname: Linux 3.12.0-4-generic i686 NonfreeKernelModules: nvidia ApportVersion: 2.12.7-0ubuntu1 Architecture: i386 CurrentDesktop: Unity Date: Tue Dec 3 15:41:40 2013 InstallationDate: Installed on 2010-10-21 (1139 days ago) InstallationMedia: Ubuntu 10.10 "Maverick Meerkat" - Release i386 (20101007) SourcePackage: qemu UpgradeStatus: Upgraded to trusty on 2013-11-01 (31 days ago) To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1257352/+subscriptions
[Qemu-devel] [PATCH] vmdk: Fix %d and %lld to PRI* in format strings
Signed-off-by: Fam Zheng --- block/vmdk.c | 13 +++-- 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index b69988d..938a183 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -640,7 +640,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, if (le32_to_cpu(header.version) > 3) { char buf[64]; -snprintf(buf, sizeof(buf), "VMDK version %d", +snprintf(buf, sizeof(buf), "VMDK version %" PRId32, le32_to_cpu(header.version)); error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, bs->device_name, "vmdk", buf); @@ -671,8 +671,9 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, } if (bdrv_getlength(file) < le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE) { -error_setg(errp, "File truncated, expecting at least %lld bytes", - le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE); +error_setg(errp, "File truncated, expecting at least %" PRId64 " bytes", + (int64_t)(le64_to_cpu(header.grain_offset) + * BDRV_SECTOR_SIZE)); return -EINVAL; } @@ -1720,7 +1721,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options, "\n" "ddb.virtualHWVersion = \"%d\"\n" "ddb.geometry.cylinders = \"%" PRId64 "\"\n" -"ddb.geometry.heads = \"%d\"\n" +"ddb.geometry.heads = \"%" PRIu32 "\"\n" "ddb.geometry.sectors = \"63\"\n" "ddb.adapterType = \"%s\"\n"; @@ -1780,9 +1781,9 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options, strcmp(fmt, "twoGbMaxExtentFlat")); compress = !strcmp(fmt, "streamOptimized"); if (flat) { -desc_extent_line = "RW %lld FLAT \"%s\" 0\n"; +desc_extent_line = "RW %" PRId64 " FLAT \"%s\" 0\n"; } else { -desc_extent_line = "RW %lld SPARSE \"%s\"\n"; +desc_extent_line = "RW %" PRId64 " SPARSE \"%s\"\n"; } if (flat && backing_file) { error_setg(errp, "Flat image can't have backing file"); -- 1.9.2
Re: [Qemu-devel] [PATCH target-arm v1 1/1] arm: translate.c: Fix smlald Instruction
On Wed, Apr 16, 2014 at 1:27 AM, Peter Maydell wrote: > On 4 April 2014 03:19, Peter Crosthwaite wrote: >> The smlald (and probably smlsld) instruction was doing incorrect sign >> extensions of the operands amongst 64bit result calculation. The >> instruction psuedo-code is: >> >> operand2 = if m_swap then ROR(R[m],16) else R[m]; >> product1 = SInt(R[n]<15:0>) * SInt(operand2<15:0>); >> product2 = SInt(R[n]<31:16>) * SInt(operand2<31:16>); >> result = product1 + product2 + SInt(R[dHi]:R[dLo]); >> R[dHi] = result<63:32>; >> R[dLo] = result<31:0>; >> >> The result calculation should be done in 64 bit arithmetic, and hence >> product1 and product2 should be sign extended to 64b before calculation. >> >> The current implementation was adding product1 and product2 together >> then sign-extending the intermediate result leading to false negatives. >> >> E.G. if product1 = product2 = 0x400, their sum = 0x8000, which >> will be incorrectly interpreted as -ve on sign extension. >> >> We fix by doing the 64b extensions on both product1 and product2 before >> any addition/subtraction happens. > > Yes, this looks right. You also fix that we were > possibly incorrectly setting the Q saturation flag for > SMLSLD, which the ARM ARM specifically says is not set: > can you mention that in the commit message too? > Yes. I've taken what you have said there near verbatim to commit message (s/you/we). > Interestingly, my random-instruction-set testing doesn't > notice this bug: it must just never manage to hit a pair > of input values which trigger it. > >> Reported-by: Christina Smith >> Signed-off-by: Peter Crosthwaite >> --- >> >> target-arm/translate.c | 33 ++--- >> 1 file changed, 22 insertions(+), 11 deletions(-) >> >> diff --git a/target-arm/translate.c b/target-arm/translate.c >> index 56e3b4b..5a62b54 100644 >> --- a/target-arm/translate.c >> +++ b/target-arm/translate.c >> @@ -7278,6 +7278,7 @@ static void disas_arm_insn(CPUARMState * env, >> DisasContext *s) >> TCGv_i32 tmp3; >> TCGv_i32 addr; >> TCGv_i64 tmp64; >> +TCGv_i64 tmp64_2; > > Can you declare this more locally to where it's used, > please? > Done. >> >> insn = arm_ldl_code(env, s->pc, s->bswap_code); >> s->pc += 4; >> @@ -8328,26 +8329,36 @@ static void disas_arm_insn(CPUARMState * env, >> DisasContext *s) >> if (insn & (1 << 5)) >> gen_swap_half(tmp2); >> gen_smul_dual(tmp, tmp2); >> -if (insn & (1 << 6)) { >> -/* This subtraction cannot overflow. */ >> -tcg_gen_sub_i32(tmp, tmp, tmp2); >> -} else { >> -/* This addition cannot overflow 32 bits; >> - * however it may overflow considered as a >> signed >> - * operation, in which case we must set the Q >> flag. >> - */ >> -gen_helper_add_setq(tmp, cpu_env, tmp, tmp2); >> -} >> -tcg_temp_free_i32(tmp2); >> if (insn & (1 << 22)) { >> /* smlald, smlsld */ >> tmp64 = tcg_temp_new_i64(); >> +tmp64_2 = tcg_temp_new_i64(); >> tcg_gen_ext_i32_i64(tmp64, tmp); >> +tcg_gen_ext_i32_i64(tmp64_2, tmp2); >> tcg_temp_free_i32(tmp); >> +tcg_temp_free_i32(tmp2); >> +if (insn & (1 << 6)) { >> +tcg_gen_sub_i64(tmp64, tmp64, tmp64_2); >> +} else { >> +tcg_gen_add_i64(tmp64, tmp64, tmp64_2); >> +} >> +tcg_temp_free_i64(tmp64_2); >> gen_addq(s, tmp64, rd, rn); >> gen_storeq_reg(s, rd, rn, tmp64); >> tcg_temp_free_i64(tmp64); >> } else { >> +if (insn & (1 << 6)) { >> +/* This subtraction cannot overflow. */ >> +tcg_gen_sub_i32(tmp, tmp, tmp2); >> +} else { >> +/* This addition cannot overflow 32 bits; >> + * however it may overflow considered as a >> + * signed operation, in which case we must >> set >> + * the Q flag. >> + */ >> +gen_helper_add_setq(tmp, cpu_env, tmp, >> tmp2); >> +} >> +tcg_temp_free_i32(tmp2); >>
[Qemu-devel] [PATCH target-arm v2 1/1] arm: translate.c: Fix smlald Instruction
The smlald (and probably smlsld) instruction was doing incorrect sign extensions of the operands amongst 64bit result calculation. The instruction psuedo-code is: operand2 = if m_swap then ROR(R[m],16) else R[m]; product1 = SInt(R[n]<15:0>) * SInt(operand2<15:0>); product2 = SInt(R[n]<31:16>) * SInt(operand2<31:16>); result = product1 + product2 + SInt(R[dHi]:R[dLo]); R[dHi] = result<63:32>; R[dLo] = result<31:0>; The result calculation should be done in 64 bit arithmetic, and hence product1 and product2 should be sign extended to 64b before calculation. The current implementation was adding product1 and product2 together then sign-extending the intermediate result leading to false negatives. E.G. if product1 = product2 = 0x400, their sum = 0x8000, which will be incorrectly interpreted as -ve on sign extension. We fix by doing the 64b extensions on both product1 and product2 before any addition/subtraction happens. We also fix where we were possibly incorrectly setting the Q saturation flag for SMLSLD, which the ARM ARM specifically says is not set. Reported-by: Christina Smith Signed-off-by: Peter Crosthwaite Reviewed-by: Peter Maydell --- changed since v1 (PMM review): Added commit msg note about Q flag correction Localised tmp64_2 declaration Fixed "else" comment location target-arm/translate.c | 34 +++--- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/target-arm/translate.c b/target-arm/translate.c index 56e3b4b..0335f10 100644 --- a/target-arm/translate.c +++ b/target-arm/translate.c @@ -8328,27 +8328,39 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s) if (insn & (1 << 5)) gen_swap_half(tmp2); gen_smul_dual(tmp, tmp2); -if (insn & (1 << 6)) { -/* This subtraction cannot overflow. */ -tcg_gen_sub_i32(tmp, tmp, tmp2); -} else { -/* This addition cannot overflow 32 bits; - * however it may overflow considered as a signed - * operation, in which case we must set the Q flag. - */ -gen_helper_add_setq(tmp, cpu_env, tmp, tmp2); -} -tcg_temp_free_i32(tmp2); if (insn & (1 << 22)) { /* smlald, smlsld */ +TCGv_i64 tmp64_2; + tmp64 = tcg_temp_new_i64(); +tmp64_2 = tcg_temp_new_i64(); tcg_gen_ext_i32_i64(tmp64, tmp); +tcg_gen_ext_i32_i64(tmp64_2, tmp2); tcg_temp_free_i32(tmp); +tcg_temp_free_i32(tmp2); +if (insn & (1 << 6)) { +tcg_gen_sub_i64(tmp64, tmp64, tmp64_2); +} else { +tcg_gen_add_i64(tmp64, tmp64, tmp64_2); +} +tcg_temp_free_i64(tmp64_2); gen_addq(s, tmp64, rd, rn); gen_storeq_reg(s, rd, rn, tmp64); tcg_temp_free_i64(tmp64); } else { /* smuad, smusd, smlad, smlsd */ +if (insn & (1 << 6)) { +/* This subtraction cannot overflow. */ +tcg_gen_sub_i32(tmp, tmp, tmp2); +} else { +/* This addition cannot overflow 32 bits; + * however it may overflow considered as a + * signed operation, in which case we must set + * the Q flag. + */ +gen_helper_add_setq(tmp, cpu_env, tmp, tmp2); +} +tcg_temp_free_i32(tmp2); if (rd != 15) { tmp2 = load_reg(s, rd); -- 1.9.2.1.g06c4abd
Re: [Qemu-devel] [PATCH 1/2] QEMU: PPC: specify PVRs for all e500 cores
> -Original Message- > From: Alexander Graf [mailto:ag...@suse.de] > Sent: Monday, April 14, 2014 6:01 AM > To: Yoder Stuart-B08248 > Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org > Subject: Re: [PATCH 1/2] QEMU: PPC: specify PVRs for all e500 cores > > > On 14.02.14 20:22, Stuart Yoder wrote: > > From: Stuart Yoder > > > > Signed-off-by: Stuart Yoder > > --- > > target-ppc/cpu-models.c | 64 > +-- > > target-ppc/cpu-models.h | 30 -- > > 2 files changed, 90 insertions(+), 4 deletions(-) > > > > diff --git a/target-ppc/cpu-models.c b/target-ppc/cpu-models.c > > index f6c9b3a..9cf603b 100644 > > --- a/target-ppc/cpu-models.c > > +++ b/target-ppc/cpu-models.c > > @@ -677,19 +677,79 @@ > > "PowerPC e500 v2.0 core") > > POWERPC_DEF("e500v2_v10",CPU_POWERPC_e500v2_v10, > e500v2, > > "PowerPC e500v2 v1.0 core") > > +POWERPC_DEF("e500v2_v11",CPU_POWERPC_e500v2_v11, > e500v2, > > +"PowerPC e500v2 v1.1 core") > > POWERPC_DEF("e500v2_v20",CPU_POWERPC_e500v2_v20, > e500v2, > > "PowerPC e500v2 v2.0 core") > > POWERPC_DEF("e500v2_v21",CPU_POWERPC_e500v2_v21, > e500v2, > > "PowerPC e500v2 v2.1 core") > > POWERPC_DEF("e500v2_v22",CPU_POWERPC_e500v2_v22, > e500v2, > > "PowerPC e500v2 v2.2 core") > > +POWERPC_DEF("e500v2_v23",CPU_POWERPC_e500v2_v23, > e500v2, > > +"PowerPC e500v2 v2.3 core") > > POWERPC_DEF("e500v2_v30",CPU_POWERPC_e500v2_v30, > e500v2, > > "PowerPC e500v2 v3.0 core") > > +POWERPC_DEF("e500v2_v31",CPU_POWERPC_e500v2_v31, > e500v2, > > +"PowerPC e500v2 v3.1 core") > > +POWERPC_DEF("e500v2_v1040", CPU_POWERPC_e500v2_v1040, > e500v2, > > +"PowerPC e500v2 v104.0 core") > > +POWERPC_DEF("e500v2_v1050", CPU_POWERPC_e500v2_v1050, > e500v2, > > +"PowerPC e500v2 v105.0 core") > > +POWERPC_DEF("e500v2_v1051", CPU_POWERPC_e500v2_v1051, > e500v2, > > +"PowerPC e500v2 v105.1 core") > > +POWERPC_DEF("e500v2_v1151", CPU_POWERPC_e500v2_v1151, > e500v2, > > +"PowerPC e500v2 v115.1 core") > > +POWERPC_DEF("e500v2_v1152", CPU_POWERPC_e500v2_v1152, > e500v2, > > +"PowerPC e500v2 v115.2 core") > > +POWERPC_DEF("e500v2_v2050", CPU_POWERPC_e500v2_v2050, > e500v2, > > +"PowerPC e500v2 v205.0 core") > > +POWERPC_DEF("e500v2_v2051", CPU_POWERPC_e500v2_v2051, > e500v2, > > +"PowerPC e500v2 v205.1 core") > > +POWERPC_DEF("e500v2_v2151", CPU_POWERPC_e500v2_v2151, > e500v2, > > +"PowerPC e500v2 v215.1 core") > > +POWERPC_DEF("e500v2_v2152", CPU_POWERPC_e500v2_v2152, > e500v2, > > +"PowerPC e500v2 v215.2 core") > > +POWERPC_DEF("e500v2_v2251", CPU_POWERPC_e500v2_v2251, > e500v2, > > +"PowerPC e500v2 v225.1 core") > > + > > +/* e500mc family */ > > POWERPC_DEF_SVR("e500mc", "e500mc", > > -CPU_POWERPC_e500mc, POWERPC_SVR_E500, > e500mc) > > +CPU_POWERPC_e500mc_v20, POWERPC_SVR_E500, > e500mc) > > How about we use aliases instead like the other CPU types? :) > > > +POWERPC_DEF_SVR("e500mc_v10", "PowerPC e500mc v1.0 core", > > +CPU_POWERPC_e500mc_v10, POWERPC_SVR_E500, > e500mc) > > +POWERPC_DEF_SVR("e500mc_v21", "PowerPC e500mc v2.1 core", > > +CPU_POWERPC_e500mc_v21, POWERPC_SVR_E500, > e500mc) > > +POWERPC_DEF_SVR("e500mc_v22", "PowerPC e500mc v2.2 core", > > +CPU_POWERPC_e500mc_v22, POWERPC_SVR_E500, > e500mc) > > +POWERPC_DEF_SVR("e500mc_v23", "PowerPC e500mc v2.3 core", > > +CPU_POWERPC_e500mc_v23, POWERPC_SVR_E500, > e500mc) > > +POWERPC_DEF_SVR("e500mc_v1030", "PowerPC e500mc v103.0 core", > > +CPU_POWERPC_e500mc_v1030, POWERPC_SVR_E500, > e500mc) > > +POWERPC_DEF_SVR("e500mc_v30", "PowerPC e500mc v3.0 core", > > +CPU_POWERPC_e500mc_v30, POWERPC_SVR_E500, > e500mc) > > +POWERPC_DEF_SVR("e500mc_v31", "PowerPC e500mc v3.1 core", > > +CPU_POWERPC_e500mc_v31, POWERPC_SVR_E500, > e500mc) > > +POWERPC_DEF_SVR("e500mc_v32", "PowerPC e500mc v3.2 core", > > +CPU_POWERPC_e500mc_v32, POWERPC_SVR_E500, > e500mc) > > + > > #ifdef TARGET_PPC64 > > +/* e5500 family */ > > POWERPC_DEF_SVR("e5500", "e5500", > > -CPU_POWERPC_e5500,POWERPC_SVR_E500, > e5500) > > Same here Can you explain you mean? what kind of alias? Thanks, Stuart
Re: [Qemu-devel] [PATCH v4 0/4] Generalise FIFO to more integer types
Hi Peter, On Tue, Apr 15, 2014 at 1:17 PM, Peter Crosthwaite wrote: > > There is a utility helper for dealing with 8 bit fifos. This should be > applicable to other integer widths as well. These two patches > generalise this FIFO to work for 16, 32 and 64 bit ints. > > changed since v3: > Initialised buffer_size (P2) (Beniamino review) > changed since v2: > Glueified hot paths to increase performance. > Addressed various minor review comments. > Added PL022 as example client (Markus Review). > changed since v1: > Rebased to include fifo buffer functionality. > > > Peter Crosthwaite (4): > util/fifo: Generalise naming scheme > util/fifo: Generalise for common integer widths > ssi: pl022: Send debug info to stderr > ssi: pl022: Convert to use FIFO > This ones all reviewed so I'm now wondering what Queue to go via. With patches 3+4 patching ARM code specifically and the P1 change pattern having most of its impact in Zynq land, should it go via target-arm? Or would you prefer a FIFO API specific PULL from me? Regards, Peter > hw/char/serial.c| 30 +++ > hw/net/allwinner_emac.c | 72 +++ > hw/ssi/pl022.c | 103 +- > hw/ssi/xilinx_spi.c | 42 - > hw/ssi/xilinx_spips.c | 66 +++--- > include/hw/char/serial.h| 6 +- > include/hw/net/allwinner_emac.h | 6 +- > include/qemu/fifo.h | 180 + > include/qemu/fifo8.h| 160 - > util/Makefile.objs | 2 +- > util/fifo.c | 191 > > util/fifo8.c| 126 -- > 12 files changed, 526 insertions(+), 458 deletions(-) > create mode 100644 include/qemu/fifo.h > delete mode 100644 include/qemu/fifo8.h > create mode 100644 util/fifo.c > delete mode 100644 util/fifo8.c > > -- > 1.9.2.1.g06c4abd >
[Qemu-devel] [Bug 1257352] Re: kvm hangs occasionally when switching out of the qemu console
Sadly a bisect pointed to the unlikely commit 7a239e46. Upstream git head is still affected. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1257352 Title: kvm hangs occasionally when switching out of the qemu console Status in QEMU: New Status in “qemu” package in Ubuntu: Confirmed Bug description: To recreate (although this does *NOT* fail most of the time alas): 1) press "ctrl-alt-2" to switch to the qemu console. 2) type say "sendkey ctrl-alt-f1" 3) press "ctrl-alt-1". Expected outcome: Switch to tty1 in the VM. Actual outcome: No switch to tty1 in the VM. and qemu console unresponsive to any keyboard input. Rather a vague problem description I'm afraid but this has happened to me 3 times recently. No crash and no excessive CPU is observed. I'll grab an strace when it happens again and attach... ProblemType: Bug DistroRelease: Ubuntu 14.04 Package: qemu-system-x86 1.6.0+dfsg-2ubuntu4 ProcVersionSignature: Ubuntu 3.12.0-4.12-generic 3.12.1 Uname: Linux 3.12.0-4-generic i686 NonfreeKernelModules: nvidia ApportVersion: 2.12.7-0ubuntu1 Architecture: i386 CurrentDesktop: Unity Date: Tue Dec 3 15:41:40 2013 InstallationDate: Installed on 2010-10-21 (1139 days ago) InstallationMedia: Ubuntu 10.10 "Maverick Meerkat" - Release i386 (20101007) SourcePackage: qemu UpgradeStatus: Upgraded to trusty on 2013-11-01 (31 days ago) To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1257352/+subscriptions
Re: [Qemu-devel] [PATCH v1 0/3] Introduce qemu_get_boot_opts()
On Wed, Apr 16, 2014 at 6:05 PM, Markus Armbruster wrote: > Peter Crosthwaite writes: > >> Hi Markus, >> >> This series introduces qemu_get_boot_opts(), in much the same way as >> was done for qemu_get_machine_opts(). >> >> As usual, I have out-of-scope and out-of-tree usages :) But P3 does >> clean up the one existing instance of the long-and-awkward form of >> this query and makes it consistent with an immediately surrounding >> qemu_get_machine_opts(). > > I doubt this is worthwhile on its own as it stands. > > However, you missed the two uses of "boot-opts" in hw/nvram/fw_cfg.c. > Since these uses are currently wrong the same way as the the uses of > "machine" fixed in commit 36ad0e9 were, covering them could strengthen > your case quite a bit, > Yeh I noticed it, andI thought that "get the first list element" code was weird. And I decided to let sleeing dogs lie. But are you saying its wrong and we can just simplify to: --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -125,18 +125,16 @@ static void fw_cfg_bootsplash(FWCfgState *s) const char *temp; /* get user configuration */ -QemuOptsList *plist = qemu_find_opts("boot-opts"); -QemuOpts *opts = QTAILQ_FIRST(&plist->head); +QemuOpts *opts = qemu_get_boot_opts(); ? Happy to make this change. Regards, Peter
Re: [Qemu-devel] [questions] host panic happened when kvm guest accessthe memory which was provided by host remap_pfn_range page to qemu
> Hi, all > > I provide host's memory to guest by remap_pfn_range host page to qemu, and > when guest access the page, host paniced. > I missed to set vma->vm_pgoff, vma->vm_pgoff = virt_to_phys(test_mem) >> PAGE_SHIFT; > Any ideas? > > Thanks, > Zhang Haoyu
Re: [Qemu-devel] [PATCH v1 1/3] qdev: Expose the qdev id string as a prop
On Thu, Apr 17, 2014 at 3:20 AM, Andreas Färber wrote: > Am 15.04.2014 23:39, schrieb Peter Crosthwaite: >> On Wed, Apr 16, 2014 at 2:16 AM, Andreas Färber wrote: >>> Am 15.04.2014 04:21, schrieb Peter Crosthwaite: So clients can set the top level id string. Signed-off-by: Peter Crosthwaite >>> >>> Anthony had nack'ed Paolo's attempt to generalize the qdev id to QOM, so >>> I'm not sure if we should really do this even if just on device level. >>> The id= is used to construct the canonical path, and we can't change >>> that through a qdev setter. >> >> How can we change it? The problem I am trying to solve, is getting >> meaningful device instance names instead of the anonymous defaults. >> This includes in the canonical path. I am completely open to proposals >> :) > > Possibly this is just a mixup, see below. On yesterday's KVM call > "instance names" referred to VMState names and instance_ids and > qdev/qbus paths. Then there's the device IDs that this patch was > touching. And there's the canonical QOM composition tree paths, that for > user-created devices may include the device ID. > >> Using a dynamic property might allow us to >>> unparent while keeping a reference and then add it as a child<> again, >>> but that still feels awkward. >> >> Eww. >> >> Having it as a getter only seem more >>> secure and predictable. >>> >> >> Sure, once it's committed to the canonical path it definitely needs a >> read-only semantic. > >> Shouldn't be hugely different to the >> writable-until-realize semantic of qdev props though? > > It is to some degree. The canonical paths get set up as part of or right > after instance_init. Only legacy devices without canonical path get a > path as part of realize, but that needs to be fixed for recursive > realization: Devices need to be accessible somewhere in the tree to be > user-modified via qom-set, and they will need to be in the tree to be > realized. > >>> Another issue is bus naming. If supplied NULL, the bus will be based on >>> ID. If the ID changes, bus naming will be inconsistent. >>> >> >> Or rather - what the user intends. If the board has 2 USB busses named >> "foo" and "bar", then those should be both the canonical path and bus >> names. Rather than the homogenised default names "usb0", "usb1". > > Let's not discuss this here again, it's being discussed between ppc and > libvirt already. If the device supplies a hardcoded bus name then (while > it may not be unique) we don't have to care in the context of IDs here. > The interesting case is if we create a device with id=foo, and the bus > uses NULL as indicated above, then it will get foo.0, foo.1, etc. unless > I've missed something. IDs are unique, so if you have only one bus per > device you get foo.0 and bar.0; if a device author chooses not to use > NULL as bus name and rolls their own then we can't help them on the > DeviceState level. > >>> Why would clients set the ID after having chosen a different ID >> >> I'm lost here - what is the mechanism by which you can validly set >> per-instance IDs? > > Sorry, maybe I don't fully get your question... > > Either the user specifies ,id=foo on command line or HMP, or she > doesn't. The corresponding mechanism in config files is [device "foo"], > see docs/q35-chipset.cfg for an example. > > qdev paths used for VMState currently still depend on a bus, which Alex > or Alexey wanted to look into fixing. By default -1 is supplied by > DeviceState for registering dc->vmsd, which can be overridden by > inlining the corresponding qdev.c code or more conveniently - Alex' > suggestion yesterday - by moving that default value of -1 into an > overrideable instance field. > > As for canonical QOM paths, it's the job of machines and devices to set > up those - unless a peripheral device is added by the user, in which > case see previous explanations by Markus and me. > > You might remember my big MPCore refactoring? That was supposed to be > the building block for you to create a proper SoC container device for > Zynq and thereby constructing meaningful canonical paths. I.e., a > ZedBoard or MicroZed or Parallella board instantiates the SoC as > /machine/zynq, the Zynq SoC makes available /machine/zynq/cortex or > whatever (-> PMM; recursively ./scu etc.) plus memory-controller (this > series), uart and whatever Zynq peripherals. I am aware of this and has been on my radar for quite some time. You can't expect me to > dictate paths for each model, that's no generic task; you need to take > care of naming your favorite device models yourself so that *no* > /machine/unassigned/device[n] remains. This may involve, such as in > Anthony's case of i440fx or in case of function-driven Exynos/Zynq/... > code, refactoring parts of devices into child<> devices for aggregation > rather than just assigning names to existing devices on /machine. > > BTW that's also why - e.g. in the Xilinx context where you discussed > inlining with Edgar - having qdev-style void cr
Re: [Qemu-devel] [PATCH v4 2/6] blockjob: Introduce block_job_complete_sync()
On Wed, 04/16 23:43, Max Reitz wrote: > On 16.04.2014 03:48, Fam Zheng wrote: > >On Sat, 04/12 20:57, Max Reitz wrote: > >>Implement block_job_complete_sync() by doing the exact same thing as > >>block_job_cancel_sync() does, only with calling block_job_complete() > >>instead of block_job_cancel(). > >> > >>Signed-off-by: Max Reitz > >>--- > >> blockjob.c | 24 ++-- > >> include/block/blockjob.h | 15 +++ > >> 2 files changed, 37 insertions(+), 2 deletions(-) > >> > >>diff --git a/blockjob.c b/blockjob.c > >>index b3ce14c..d12f3ea 100644 > >>--- a/blockjob.c > >>+++ b/blockjob.c > >>@@ -165,7 +165,9 @@ static void block_job_cancel_cb(void *opaque, int ret) > >> data->cb(data->opaque, ret); > >> } > >>-int block_job_cancel_sync(BlockJob *job) > >>+static int block_job_finish_sync(BlockJob *job, > >>+ void (*finish)(BlockJob *, Error **errp), > >>+ Error **errp) > >> { > >> struct BlockCancelData data; > >> BlockDriverState *bs = job->bs; > >>@@ -181,13 +183,31 @@ int block_job_cancel_sync(BlockJob *job) > >> data.ret = -EINPROGRESS; > >> job->cb = block_job_cancel_cb; > >> job->opaque = &data; > >>-block_job_cancel(job); > >>+finish(job, errp); > >> while (data.ret == -EINPROGRESS) { > >> qemu_aio_wait(); > >> } > >> return (data.cancelled && data.ret == 0) ? -ECANCELED : data.ret; > >> } > >>+/* A wrapper around block_job_cancel() taking an Error ** parameter so it > >>may be > >>+ * used with block_job_finish_sync() without the need for (rather nasty) > >>+ * function pointer casts there. */ > >Why not just change the prototype of block_job_cancel to have this error > >pointer? > > Well, there are external callers of block_job_cancel() which I would have to > change and I don't really like calling a function with NULL for the Error ** > parameter if it has one; even though this version of block_job_cancel > wouldn't actually make use of that parameter. > > So, what I'd end up doing is implementing error handling for > block_job_cancel() in these external callers without any actual need for it, > which seems to me like more effort than implementing this wrapper. > Furthermore, I personally like having this wrapper directly above > block_job_cancel_sync() as it explains nicely why the latter may give NULL > for the Error ** parameter of block_job_finish_sync(). Thanks for explanation! I was in doubt about different parameter lists of block_job_complete_sync and block_job_cancel_sync (only one has Error **). But it is not a big problem. > > I'll change it, though, if you insist. ;-) Not strongly here, your decision. > >>+static void block_job_cancel_err(BlockJob *job, Error **errp) > >>+{ > >>+block_job_cancel(job); > >>+} > >>+ > >>+int block_job_cancel_sync(BlockJob *job) > >>+{ > >>+return block_job_finish_sync(job, &block_job_cancel_err, NULL); > >>+} > >>+ > >>+int block_job_complete_sync(BlockJob *job, Error **errp) > >>+{ > >>+return block_job_finish_sync(job, &block_job_complete, errp); > >>+} > >>+ > >> void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns) > >> { > >> assert(job->busy); > >>diff --git a/include/block/blockjob.h b/include/block/blockjob.h > >>index d76de62..626ea42 100644 > >>--- a/include/block/blockjob.h > >>+++ b/include/block/blockjob.h > >>@@ -253,6 +253,21 @@ bool block_job_is_paused(BlockJob *job); > >> int block_job_cancel_sync(BlockJob *job); > >> /** > >>+ * block_job_complete_sync: > >>+ * @job: The job to be completed. > >>+ * @errp: Error object which may be set by block_job_complete(); this is > >>not > >>+ *necessarily set on every error, the job return value has to be > >>+ *checked as well. > >>+ * > >>+ * Synchronously complete the job. The completion callback is called > >>before the > >>+ * function returns, unless it is NULL (which is permissible when using > >>this > >>+ * function). > >>+ * > >>+ * Returns the return value from the job. > >>+ */ > >>+int block_job_complete_sync(BlockJob *job, Error **errp); > >>+ > >>+/** > >> * block_job_iostatus_reset: > >> * @job: The job whose I/O status should be reset. > >> * > >>-- > >>1.9.2 > >> >
Re: [Qemu-devel] disk cache question and drawback of cache
On Wed, 04/16 22:07, longguang.yue wrote: > hi,all > libvirt supply "default", "none", "writethrough", "writeback", "directsync", > "unsafe" disk cache options > > > 1. > as for qemu, how qemu uses those options? and what are the differences? > better tell me where are the codes corresponding to those options? Please see the explanations in qemu-doc: http://wiki.qemu.org/download/qemu-doc.html > 2. > after a long time of running , it seems that vms will slow down? do you > know why? Slow in doing what exactly? Disk? Network? CPU? Or memory access? Could you be specific how you tested it? Could you show some data? Thanks, Fam
Re: [Qemu-devel] [PATCH 08/35] qdev: hotplug for buss-less devices
On Fri, Apr 4, 2014 at 11:36 PM, Igor Mammedov wrote: > Adds get_hotplug_handler() method to machine, and > makes bus-less device to use it during hotplug > as a means to discover hotplug handler controller. > Returned controller is used to permorm a hotplug > action. > > Signed-off-by: Igor Mammedov > --- > hw/core/qdev.c | 13 + > include/hw/boards.h | 8 > 2 files changed, 21 insertions(+) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 60f9df1..50bb8f5 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -34,6 +34,7 @@ > #include "qapi/qmp/qjson.h" > #include "monitor/monitor.h" > #include "hw/hotplug.h" > +#include "hw/boards.h" > > int qdev_hotplug = 0; > static bool qdev_hot_added = false; > @@ -761,6 +762,18 @@ static void device_set_realized(Object *obj, bool value, > Error **err) > local_err == NULL) { > hotplug_handler_plug(dev->parent_bus->hotplug_handler, > dev, &local_err); > +} else if (local_err == NULL && > + object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) { This doesn't look right - you are relying on global state to implement hotplug. Later in the series you then need to do RTTI at the machine level to properly determine if hotplug is really appropriate then do some machine specific attachment. This idea of "we dont need a bus just hotplug to the machine itself" doesnt seem generic at all. If you need to define hotplug socket-side functionality, then that seems to me to be a bus level problem - and you should use a bus - probably SYSBUS or an extension thereof. Then your hotplug work can be generalized to sysbus and a range of devices rather than us having independent competing "embedded vs PC" hotplug implementations. How do you implement hotplugging to multiple completely independent DIMM slots? (i.e. two slots at completely different places in the bus heir-achy). Regarding DIMM, I think it is a bus. I'm not sure if it actually needs its own class yet (TBH I haven't gone line-line on this series looking for DIMMisms). It is ultimately a memory mapped bus if anything I think it should be: .name = TYPE_DIMM_DEVICE, .parent = TYPE_SYSBUS_DEVICE, .name = TYPE_DIMM_SLOT, .parent = TYPE_SYSTEM_BUS, It is true that we still need to remove the global stateness from sysbus itself (there are a few threads on list on the issue) before this can really be nice. Regards, Peter > +HotplugHandler *hotplug_ctrl; > +MachineState *machine = MACHINE(qdev_get_machine()); > +MachineClass *mc = MACHINE_GET_CLASS(machine); > + > +if (mc->get_hotplug_handler) { > +hotplug_ctrl = mc->get_hotplug_handler(machine, dev); > +if (hotplug_ctrl) { > +hotplug_handler_plug(hotplug_ctrl, dev, &local_err); > +} > +} > } > > if (qdev_get_vmsd(dev) && local_err == NULL) { > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 3567190..67750b5 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -73,6 +73,11 @@ extern MachineState *current_machine; > /** > * MachineClass: > * @qemu_machine: #QEMUMachine > + * @get_hotplug_handler: this function is called during bus-less > + *device hotplug. If defined it returns pointer to an instance > + *of HotplugHandler object, which handles hotplug operation > + *for a given @dev. It may return NULL if @dev doesn't require > + *any actions to be performed by hotplug handler. > */ > struct MachineClass { > /*< private >*/ > @@ -80,6 +85,9 @@ struct MachineClass { > /*< public >*/ > > QEMUMachine *qemu_machine; > + > +HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > + DeviceState *dev); > }; > > /** > -- > 1.9.0 > >
Re: [Qemu-devel] Change of TEXT_OFFSET for multi_v7_defconfig
On Wed, 16 Apr 2014, Christopher Covington wrote: > On 04/16/2014 03:14 PM, Nicolas Pitre wrote: > > On Wed, 16 Apr 2014, Christopher Covington wrote: > > > >> It seems to me that if external/uncompressed image loaders could query the > >> text offset in a straightforward manner, variance between images could be > >> easily dealt with. Would reading it out of ELF metadata be a reasonable > >> mechanism? Could the ELF format vmlinux be a suitable general replacement > >> for > >> the raw Image? > > > > The ELF image only has virtual addresses in it. And the virtual address > > of the kernel may be changed independently of TEXT_OFFSET with > > CONFIG_VMSPLIT_*. > > Do you know why this is the case? The ELF format is capable of storing > physical addresses as mentioned below. We don't know at build time what the physical address of the kernel will be. Back when the kernel could boot on a single SOC family then the physical address was hardcoded in the kernel binary. And with some special configs (running a XIP kernel is the most obvious example) it is still the case. But in general we don't want to hardcode any physical address in order to boot the same kernel binary on as many different machines as possible. Therefore the physical address is determined at run time in most cases and having it in the ELF file would be meaningless. The virtual address for the kernel, including TEXT_OFFSET, is determined at build time. And that's what the ELF file carries. > What I meant to ask about was variance from one kernel version and build to > the next, given a single platform. Platform-to-platform variation can probably > be abstracted where needed by the scripts controlling the external load. Right. The platform script must know where physical RAM is and load the kernel accordingly. In case of zImage you need to load it anywhere in the first 128MB of RAM. In the uncompressed Image it is at TEXT_OFFSET from start of physical RAM. > In any case, CONFIG_VMSPLIT_* that you mentioned above would be an > example where it would vary in an inconvenient manner, so this > approach wouldn't be an improvement. Well. This config option changes the virtual address the kernel is going to set up for itself. And that is going to be reflected in the ELF file as well. That part is pretty machine independent and can be obtained easily with tools like debuggers. you pretty much need the ELF file if you want to have symbolic debugging anyway. So no issue there. > >> Or could we patch up the linker script to set zero-based ELF load > >> memory addresses (LMAs) [4] so that the physical addresses are almost > >> right, > >> you just might have to add a system-specific RAM offset, perhaps pulled > >> out of > > I don't think I made this very clear, but adding the offset would happen at > load/run-time, controlled by JTAG scripts or simulator equivalent. Sure. To load and debug the code before the MMU is turned on, you need to know the physical address of RAM (the kernel binary doesn't carry it) and the value of TEXT_OFFSET (this is hardcoded in the kernel binary at build time). > > If you really really want to get at the TEXT_OFFSET value in the > > uncompressed image, the simplest way would be: > > > > diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S > > index f8c08839ed..de84d0635a 100644 > > --- a/arch/arm/kernel/head.S > > +++ b/arch/arm/kernel/head.S > > @@ -78,6 +78,11 @@ > > > > __HEAD > > ENTRY(stext) > > + > > + b 1f > > + .word TEXT_OFFSET @ located at a 4-byte offset in Image > > +1: > > + > > ARM_BE8(setendbe )@ ensure we are in BE8 mode > > > > THUMB(adr r9, BSYM(1f)) @ Kernel is always entered in > > ARM. > > > > This way the first word for Image would always be 0xea00 and the > > second one would be TEXT_OFFSET. No other kernel Image binaries ever > > had 0xea00 as their first word so that also let you validate whether > > or not the TEXT_OFFSET value is there. > > Thank you for the suggestion. This approach also came to mind, but it would > require new documentation and tooling in the JTAG scripts or simulator > equivalent. That's another aspect of the ELF-based approaches that I > like--hopefully existing documentation and tool support could be reused. The above is useful for loading the raw uncompressed Image without carrying the full ELF baggage. With the ELF file we could simply store a symbol which value would be TEXT_OFFSET. But the physical start of RAM has to come from outside the kernel image. But whatever you do, new documentation and tooling is required anyway if you want to automate this into your debugging script. Nicolas
Re: [Qemu-devel] [PATCH v4 4/6] qemu-img: Enable progress output for commit
On 16.04.2014 23:48, Max Reitz wrote: On 16.04.2014 17:00, Kevin Wolf wrote: Am 12.04.2014 um 20:57 hat Max Reitz geschrieben: Implement progress output for the commit command by querying the progress of the block job. Signed-off-by: Max Reitz --- qemu-img-cmds.hx | 4 ++-- qemu-img.c | 44 ++-- qemu-img.texi| 2 +- 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index d029609..8bc55cd 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -22,9 +22,9 @@ STEXI ETEXI DEF("commit", img_commit, -"commit [-q] [-f fmt] [-t cache] filename") +"commit [-q] [-f fmt] [-t cache] [-p] filename") STEXI -@item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename} +@item commit [-q] [-f @var{fmt}] [-t @var{cache}] [-p] @var{filename} ETEXI DEF("compare", img_compare, diff --git a/qemu-img.c b/qemu-img.c index 9fe6384..50d7519 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -686,6 +686,9 @@ fail: struct CommonBlockJobCBInfo { Error **errp; bool done; +/* If the blockjob works on several sectors at once, this field has to + * contain that granularity in bytes */ +uint64_t granularity; }; static void common_block_job_cb(void *opaque, int ret) @@ -702,12 +705,34 @@ static void common_block_job_cb(void *opaque, int ret) static void run_block_job(BlockJob *job, struct CommonBlockJobCBInfo *cbi) { BlockJobInfo *info; +uint64_t mod_offset = 0; do { qemu_aio_wait(); info = block_job_query(job); +if (info->offset) { +if (!mod_offset) { +/* Some block jobs (at least "commit") will only work on a + * subset of the image file and therefore basically skip many + * sectors at the start (processing them apparently + * instantaneously). These sectors should be ignored when + * calculating the progress. + * If the blockjob processes several sectors at once, the first + * progress report is likely to come after one such sector group + * (whose size is cbi->granularity), therefore subtract it from + * the current offset in order to obtain a more probable + * starting offset. */ +mod_offset = info->offset > cbi->granularity + ? info->offset - cbi->granularity + : 0; granularity != buf_size. And you still can't distinguish whether the first chunk was skipped or copied. In the v2 review I suggested changing mirror_run() to update s->common.len. There you wouldn't have to make any assumptions, but would know exactly where the bitmap initialisation is done. And it would improve traditional block job progress output, too. Am I missing anything that makes this approach harder than it seems? I just don't like it, somehow, that's all. But I'll see what I can do. Okay, now I have a better reason than "Meh, I don't like it" (which is always a very bad reason, of course), being the following: As mirror_run() is actually made for mirroring from an active block device, some sectors may be marked dirty during the block job which the initialization loop did not consider dirty. This will not occur in the case of qemu-img commit, obviously, as the block device is not really used. However, mirror_run() isn't made for use by qemu-img only. If new sectors are marked dirty during the block job's operation, we'd have to increase the length of the block job live which seems very crude to me. Of course, I'd love to be proven wrong, as I do see that the above solution (taking the granularity into account) is pretty crude as well. Max
Re: [Qemu-devel] Change of TEXT_OFFSET for multi_v7_defconfig
On Wed, Apr 16, 2014 at 10:36:11PM +0100, Peter Maydell wrote: > On 16 April 2014 22:08, Christopher Covington wrote: > > On 04/16/2014 03:14 PM, Nicolas Pitre wrote: > >> But both QEMU and the boot-wrapper should deal with zImage. That's the > >> only image format with documented load offset is guaranteed not to > >> change i.e. it can be loaded at about any offset as zImage knows how to > >> relocate itself as needed. There is nowhere a guarantee that > >> TEXT_OFFSET can't change. > > > > QEMU definitely does support zImage and I believe it's promoted as the main > > boot method. > > Yes; we also support uImage. The code nominally handling Image > actually currently loads at 0x1, so the set of people who actually > try to use it is obviously not very large :-) For ARM, that means precisely zero users without modification of that. We've never supported an offset of 0x1 in mainline kernels. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.
Re: [Qemu-devel] Change of TEXT_OFFSET for multi_v7_defconfig
On Wed, Apr 16, 2014 at 05:08:35PM -0400, Christopher Covington wrote: > What I meant to ask about was variance from one kernel version and build to > the next, given a single platform. Platform-to-platform variation can probably > be abstracted where needed by the scripts controlling the external load. In > any case, CONFIG_VMSPLIT_* that you mentioned above would be an example where > it would vary in an inconvenient manner, so this approach wouldn't be an > improvement. No it wouldn't. CONFIG_VMSPLIT_* has nothing to do with where the kernel is loaded in physical memory. That's all about how the kernel sets up the page tables, and where the kernel eventually expects to run in the virtual address space. As far as the debugger goes, it still loads the kernel at the exact same address irrespective of what CONFIG_VMSPLIT_* setting is chosen. The issue here is with arm-soc's single zImage project sucking in existing platforms where there's a requirement to keep the first N kB of memory free from use - eg, because a boot loader likes to scribble over it, or it's in use by a DSP processor, or something similar. Once one of those platforms is merged and enabled in the single zImage, the offset into RAM that the kernel must be loaded has to change to avoid clashing on those platforms. So, there really isn't one single Kconfig option you can point at to tell what physical RAM offset the kernel needs to be loaded at... it depends what platforms are enabled in the kernel you're trying to boot. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.
Re: [Qemu-devel] qemu builds on arm hosts
On 16 April 2014 04:26, New B wrote: > I hit these errors: > > 1- a pragma #error induced error in tcg/aarch64/tcg-target.c stating that > "USE_DIRECT_JUMP required for aarch64”, line 1105. > 2- a link failure: > Undefined symbols for architecture x86_64: > "_print_insn_i386", referenced from: > _disas in disas.o > ld: symbol(s) not found for architecture x86_64 > clang: error: linker command failed with exit code 1 (use -v to see > invocation) > make[1]: *** [qemu-system-arm] Error 1 > make: *** [subdir-arm-softmmu] Error 2 > > I am not sure why there is a mention of “architecture x86_64” if I am > supposed to be building for aarch64. You've clearly managed to break your build environment somehow (wrong arguments to configure?) because that is trying to use the x86 linker, from the looks of things. Is this a cross-build? In any case, this should all work so your problems are in your build environment or configure arguments, I think. You might like to try one of the 2.0 release candidates, just to check it's not something that's only wrong on the 1.7.x branch. thanks -- PMM
Re: [Qemu-devel] [PATCH 3/3] block: Catch integer overflow in bdrv_rw_co()
On 16.04.2014 15:08, Kevin Wolf wrote: Insanely large requests could cause an integer overflow in bdrv_rw_co() while converting sectors to bytes. This patch catches the problem and returns an error (if we hadn't overflown the integer here, bdrv_check_byte_request() would have rejected the request, so we're not breaking anything that was supposed to work before). We actually do have a test case that triggers behaviour where we accidentally let such a request pass, so that it would return success, but read 0 bytes instead of the requested 4 GB. It fails now like it should. If the vdi block driver wants to be able to deal with huge images, it can't read the whole block bitmap at once into memory like it does today, but needs to use a metadata cache like qcow2 does. Signed-off-by: Kevin Wolf --- block.c| 4 tests/qemu-iotests/084.out | 5 + 2 files changed, 5 insertions(+), 4 deletions(-) Maybe we should add some comment to test 084, as I can easily understand someone getting confused as why such a test now catching something practically unrelated to VDI there. Or we should just leave the output as it is, making the test deliberately fail. The fix itself is correct, though. Requests exceeding INT_MAX bytes aren't supported in other places as well (right now, discards come to my mind), so it should be fine to reject them here as well. Max
Re: [Qemu-devel] Are Qemu builds supported for arm host?
On 15 April 2014 22:47, New B wrote: > I keep getting this error: "must include QEMU headers” emitted from > tcg/tcg-op.h. I have no idea how come this is happening. > > BTW, the same setup I have (build env + 1.7.1-stable qemu sources) to > successfully build a functional qemu-system-arm for x86-64 host. Yes, it works fine (and an ARM system is one of the ones I regularly do build tests on before commiting things). It works in exactly the same way as a build on an x86 Linux system, so if you're running into problems then you've messed up the build somehow or your build environment is broken. thanks -- PMM
Re: [Qemu-devel] [PATCH 2/3] block: Limit size to INT_MAX in bdrv_check_byte_request()
On 16.04.2014 15:08, Kevin Wolf wrote: Commit 8f4754ed intended to protect against integer overflow bugs in block drivers by making sure that a single request that is passed to drivers is no longer than INT_MAX bytes. However, meanwhile there are some callers that don't use that code path any more but call bdrv_check_byte_request() directy, so let's add a check there as well. Signed-off-by: Kevin Wolf --- block.c | 4 1 file changed, 4 insertions(+) Reviewed-by: Max Reitz
Re: [Qemu-devel] [RFC v2 4/6] vfio: Add initial IRQ support in QEMU platform device
On Wed, 16 Apr 2014 15:29:35 +0200 Eric Auger wrote: > On 04/11/2014 03:34 AM, Kim Phillips wrote: > > On Wed, 9 Apr 2014 16:33:07 +0100 > > Eric Auger wrote: > >> @@ -108,12 +108,13 @@ static const MemMapEntry a15memmap[] = { > >> /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that > >> size */ > >> /* 0x1000 .. 0x4000 reserved for PCI */ > >> [VIRT_MEM] = { 0x4000, 1ULL * 1024 * 1024 * 1024 }, > >> -[VIRT_ETHERNET] = { 0xfff51000, 0x1000 }, > >> +[VIRT_ETHERNET] = { 0xfff41000, 0x1000 }, > > > > this change isn't explained (the device is at physical 0xfff51000, > > not 0xfff41000), and looks like it belongs in the first patch of the > > series anyway. Note: see e.g., commit f5fdcd6e5 "hw/arm: Add > > 'virt' platform" for an example of how to re-compose commit message > > text for patches that undergo a change of author. > > > Hi Kim, Hi Eric, > I acknowledge the change is not justified in the context of IRQ support > introduction. I will remove it. > I changed this because the address used in the prior patch was > misleading to me, as I reported in one comment. Indeed 0xFFF51000 is the > host physical address of the device but here we specify the guest > physical address which in general does not relate at all with the host > physical address. I see there's churn in other threads in this area...good. > >> +char *name; > >> +uint32_t mmap_timeout; /* mmap timeout value in ms */ > >> +VFIORegion regions[PLATFORM_NUM_REGIONS]; > > > > this is a regression over the last version of the third patch I sent > > you; 'regions' should remain dynamically allocated - here, they're > > being fixed. > OK I did that way to reuse > vfio_irq_eoi(container_of(region, VFIODevice, regions[region->nr])) > in vfio_region_write/read. > > But this definitively can be improved. it has to, in order to support a variable number of regions. Maybe use the VFIODevice itself as the opaque pointer instead of 'fd' in struct VFIORegion? > >> +/* TODO: fix this number of regions, > >> + * currently a single region is handled > >> + */ > > > > please do :) > Can't we image to have a separate patch for this story of number of > regions, overall? well I'd expect the next revision of this series to blend better with the the way it was done in "vfio: add vfio-platform support" (the existing 3rd patch), i.e., add interrupt support without breaking support for a variable number of regions. > >> +static void vfio_irq_eoi(VFIODevice *vdev) > >> +{ > >> + > >> +VFIOINTp *intp; > >> +bool eoi_done = false; > >> + > >> +QLIST_FOREACH(intp, &vdev->intp_list, next) { > >> +if (intp->pending) { > >> +if (eoi_done) { > >> +DPRINTF("several IRQ pending simultaneously: \ > >> + this is not a supported case yet\n"); > >> +} > > > > If the thing to do in this case is not remap MMIO to the 'fast > > path', then we should do that. Otherwise, I think I'd protect this > > with DEBUG in the meantime.. > I did not understand that comment As it stands, that code is only DPRINTF'ing in the irq->pending case, and it's adding a linked list to the VFIOINT struct which otherwise is equal to that of the vfio-pci implementation. All we need to do in the irq pending case is *not* switch back to the 'fast path' (direct MMIO access, ie., still use vfio_region_{read,write} ()), right? In which case, all we may need is an interrupt pending counter in the VFIODevice struct, that this code should check. Looking at the code again, I noticed vfio_disable_irqindex(), vfio_unmask_int{x,p}() are almost verbatim copies - can they be merged into common.c? I also noticed the platform code doesn't have a vfio_mask_intx() equivalent but can't tell if it needs it ATM. > BR > > Eric Thanks Eric, Kim
Re: [Qemu-devel] [PATCH 1/3] block: Fix nb_sectors check in bdrv_check_byte_request()
On 16.04.2014 15:08, Kevin Wolf wrote: nb_sectors is signed, check for negative values. Signed-off-by: Kevin Wolf --- block.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Okay, why not. Reviewed-by: Max Reitz
Re: [Qemu-devel] [PATCH] block: Check bdrv_getlength() return value in bdrv_make_zero()
On 16.04.2014 15:09, Kevin Wolf wrote: Signed-off-by: Kevin Wolf --- block.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) Reviewed-by: Max Reitz
Re: [Qemu-devel] [PATCH v4 3/6] qemu-img: Implement commit like QMP
On 16.04.2014 16:40, Kevin Wolf wrote: Am 12.04.2014 um 20:57 hat Max Reitz geschrieben: qemu-img should use QMP commands whenever possible in order to ensure feature completeness of both online and offline image operations. As qemu-img itself has no access to QMP (since this would basically require just everything being linked into qemu-img), imitate QMP's implementation of block-commit by using commit_active_start() and then waiting for the block job to finish. This new implementation does not empty the snapshot image, as opposed to the old implementation using bdrv_commit(). However, as QMP's block-commit apparently never did this and as qcow2 (which is probably qemu's standard image format) does not even implement the required function (bdrv_make_empty()), it does not seem necessary. Signed-off-by: Max Reitz --- block/Makefile.objs | 2 +- qemu-img.c | 86 + 2 files changed, 68 insertions(+), 20 deletions(-) diff --git a/block/Makefile.objs b/block/Makefile.objs index fd88c03..2c37e80 100644 --- a/block/Makefile.objs +++ b/block/Makefile.objs @@ -9,6 +9,7 @@ block-obj-y += snapshot.o qapi.o block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o block-obj-$(CONFIG_POSIX) += raw-posix.o block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o +block-obj-y += mirror.o ifeq ($(CONFIG_POSIX),y) block-obj-y += nbd.o nbd-client.o sheepdog.o @@ -22,7 +23,6 @@ endif common-obj-y += stream.o common-obj-y += commit.o -common-obj-y += mirror.o common-obj-y += backup.o iscsi.o-cflags := $(LIBISCSI_CFLAGS) diff --git a/qemu-img.c b/qemu-img.c index 8455994..9fe6384 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -30,6 +30,7 @@ #include "qemu/osdep.h" #include "sysemu/sysemu.h" #include "block/block_int.h" +#include "block/blockjob.h" #include "block/qapi.h" #include @@ -682,12 +683,49 @@ fail: return ret; } +struct CommonBlockJobCBInfo { +Error **errp; +bool done; Looks unused (set, but never read). Right, it's an artifact from an earlier version and in-between work. +}; + +static void common_block_job_cb(void *opaque, int ret) +{ +struct CommonBlockJobCBInfo *cbi = opaque; + +if (ret < 0) { +error_setg_errno(cbi->errp, -ret, "Block job failed"); +} In practice, I guess this will give us rather bad error messages. Perhaps we need to replace 'int ret' with 'Error *errp' for block job callbacks in a followup. Probably, yes. + +cbi->done = true; +} + +static void run_block_job(BlockJob *job, struct CommonBlockJobCBInfo *cbi) +{ +BlockJobInfo *info; + +do { +qemu_aio_wait(); + +info = block_job_query(job); Where does info get freed? That, indeed, is a good question. I'll fix it, thanks. + +if (!info->busy && info->offset < info->len) { +block_job_resume(job); +} +} while (info->offset < info->len); + +block_job_complete_sync(job, cbi->errp); +} + +/* Same as in block.c */ +#define COMMIT_BUF_SECTORS 2048 [...] +commit_active_start(bs, base_bs, 0, COMMIT_BUF_SECTORS << BDRV_SECTOR_BITS, +BLOCKDEV_ON_ERROR_REPORT, common_block_job_cb, &cbi, +&local_err); Though bdrv_commit() uses it for a different purpose: There it's the buffer size that is used for committing. A single request can never be larger than this value, but depending on bdrv_is_allocated() it can be smaller. So the granularity for the decision whether to copy data is still the granularity of bdrv_is_allocated(), i.e. one cluster. For the mirror block job, the decision is taken on the granularity that you specify. This should be the same as for bdrv_commit(), i.e. the default that you get when you specify 0. mirror_start_job() also has a buf_size parameter, which is however not exposed by commit_active_start(). This is where COMMIT_BUF_SECTORS would be right. Hm, interesting. I remembered trying this and it being pretty slow (test 20 and some other test images). However, I can't reproduce it any longer. I guess that means I can drop patch 1 of this series, too. Max
Re: [Qemu-devel] [PATCH v4 4/6] qemu-img: Enable progress output for commit
On 16.04.2014 17:00, Kevin Wolf wrote: Am 12.04.2014 um 20:57 hat Max Reitz geschrieben: Implement progress output for the commit command by querying the progress of the block job. Signed-off-by: Max Reitz --- qemu-img-cmds.hx | 4 ++-- qemu-img.c | 44 ++-- qemu-img.texi| 2 +- 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index d029609..8bc55cd 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -22,9 +22,9 @@ STEXI ETEXI DEF("commit", img_commit, -"commit [-q] [-f fmt] [-t cache] filename") +"commit [-q] [-f fmt] [-t cache] [-p] filename") STEXI -@item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename} +@item commit [-q] [-f @var{fmt}] [-t @var{cache}] [-p] @var{filename} ETEXI DEF("compare", img_compare, diff --git a/qemu-img.c b/qemu-img.c index 9fe6384..50d7519 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -686,6 +686,9 @@ fail: struct CommonBlockJobCBInfo { Error **errp; bool done; +/* If the blockjob works on several sectors at once, this field has to + * contain that granularity in bytes */ +uint64_t granularity; }; static void common_block_job_cb(void *opaque, int ret) @@ -702,12 +705,34 @@ static void common_block_job_cb(void *opaque, int ret) static void run_block_job(BlockJob *job, struct CommonBlockJobCBInfo *cbi) { BlockJobInfo *info; +uint64_t mod_offset = 0; do { qemu_aio_wait(); info = block_job_query(job); +if (info->offset) { +if (!mod_offset) { +/* Some block jobs (at least "commit") will only work on a + * subset of the image file and therefore basically skip many + * sectors at the start (processing them apparently + * instantaneously). These sectors should be ignored when + * calculating the progress. + * If the blockjob processes several sectors at once, the first + * progress report is likely to come after one such sector group + * (whose size is cbi->granularity), therefore subtract it from + * the current offset in order to obtain a more probable + * starting offset. */ +mod_offset = info->offset > cbi->granularity + ? info->offset - cbi->granularity + : 0; granularity != buf_size. And you still can't distinguish whether the first chunk was skipped or copied. In the v2 review I suggested changing mirror_run() to update s->common.len. There you wouldn't have to make any assumptions, but would know exactly where the bitmap initialisation is done. And it would improve traditional block job progress output, too. Am I missing anything that makes this approach harder than it seems? I just don't like it, somehow, that's all. But I'll see what I can do. Max
Re: [Qemu-devel] [PATCH v4 2/6] blockjob: Introduce block_job_complete_sync()
On 16.04.2014 16:41, Kevin Wolf wrote: Am 12.04.2014 um 20:57 hat Max Reitz geschrieben: Implement block_job_complete_sync() by doing the exact same thing as block_job_cancel_sync() does, only with calling block_job_complete() instead of block_job_cancel(). Signed-off-by: Max Reitz --- blockjob.c | 24 ++-- include/block/blockjob.h | 15 +++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/blockjob.c b/blockjob.c index b3ce14c..d12f3ea 100644 --- a/blockjob.c +++ b/blockjob.c @@ -165,7 +165,9 @@ static void block_job_cancel_cb(void *opaque, int ret) data->cb(data->opaque, ret); } -int block_job_cancel_sync(BlockJob *job) +static int block_job_finish_sync(BlockJob *job, + void (*finish)(BlockJob *, Error **errp), + Error **errp) { struct BlockCancelData data; BlockDriverState *bs = job->bs; @@ -181,13 +183,31 @@ int block_job_cancel_sync(BlockJob *job) data.ret = -EINPROGRESS; job->cb = block_job_cancel_cb; Rename this as block_job_finish_cb? Yes, that makes sense. Max
Re: [Qemu-devel] [PATCH v4 2/6] blockjob: Introduce block_job_complete_sync()
On 16.04.2014 03:48, Fam Zheng wrote: On Sat, 04/12 20:57, Max Reitz wrote: Implement block_job_complete_sync() by doing the exact same thing as block_job_cancel_sync() does, only with calling block_job_complete() instead of block_job_cancel(). Signed-off-by: Max Reitz --- blockjob.c | 24 ++-- include/block/blockjob.h | 15 +++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/blockjob.c b/blockjob.c index b3ce14c..d12f3ea 100644 --- a/blockjob.c +++ b/blockjob.c @@ -165,7 +165,9 @@ static void block_job_cancel_cb(void *opaque, int ret) data->cb(data->opaque, ret); } -int block_job_cancel_sync(BlockJob *job) +static int block_job_finish_sync(BlockJob *job, + void (*finish)(BlockJob *, Error **errp), + Error **errp) { struct BlockCancelData data; BlockDriverState *bs = job->bs; @@ -181,13 +183,31 @@ int block_job_cancel_sync(BlockJob *job) data.ret = -EINPROGRESS; job->cb = block_job_cancel_cb; job->opaque = &data; -block_job_cancel(job); +finish(job, errp); while (data.ret == -EINPROGRESS) { qemu_aio_wait(); } return (data.cancelled && data.ret == 0) ? -ECANCELED : data.ret; } +/* A wrapper around block_job_cancel() taking an Error ** parameter so it may be + * used with block_job_finish_sync() without the need for (rather nasty) + * function pointer casts there. */ Why not just change the prototype of block_job_cancel to have this error pointer? Well, there are external callers of block_job_cancel() which I would have to change and I don't really like calling a function with NULL for the Error ** parameter if it has one; even though this version of block_job_cancel wouldn't actually make use of that parameter. So, what I'd end up doing is implementing error handling for block_job_cancel() in these external callers without any actual need for it, which seems to me like more effort than implementing this wrapper. Furthermore, I personally like having this wrapper directly above block_job_cancel_sync() as it explains nicely why the latter may give NULL for the Error ** parameter of block_job_finish_sync(). I'll change it, though, if you insist. ;-) Max Thanks, Fam +static void block_job_cancel_err(BlockJob *job, Error **errp) +{ +block_job_cancel(job); +} + +int block_job_cancel_sync(BlockJob *job) +{ +return block_job_finish_sync(job, &block_job_cancel_err, NULL); +} + +int block_job_complete_sync(BlockJob *job, Error **errp) +{ +return block_job_finish_sync(job, &block_job_complete, errp); +} + void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns) { assert(job->busy); diff --git a/include/block/blockjob.h b/include/block/blockjob.h index d76de62..626ea42 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -253,6 +253,21 @@ bool block_job_is_paused(BlockJob *job); int block_job_cancel_sync(BlockJob *job); /** + * block_job_complete_sync: + * @job: The job to be completed. + * @errp: Error object which may be set by block_job_complete(); this is not + *necessarily set on every error, the job return value has to be + *checked as well. + * + * Synchronously complete the job. The completion callback is called before the + * function returns, unless it is NULL (which is permissible when using this + * function). + * + * Returns the return value from the job. + */ +int block_job_complete_sync(BlockJob *job, Error **errp); + +/** * block_job_iostatus_reset: * @job: The job whose I/O status should be reset. * -- 1.9.2
Re: [Qemu-devel] Change of TEXT_OFFSET for multi_v7_defconfig
On 16 April 2014 22:08, Christopher Covington wrote: > On 04/16/2014 03:14 PM, Nicolas Pitre wrote: >> But both QEMU and the boot-wrapper should deal with zImage. That's the >> only image format with documented load offset is guaranteed not to >> change i.e. it can be loaded at about any offset as zImage knows how to >> relocate itself as needed. There is nowhere a guarantee that >> TEXT_OFFSET can't change. > > QEMU definitely does support zImage and I believe it's promoted as the main > boot method. Yes; we also support uImage. The code nominally handling Image actually currently loads at 0x1, so the set of people who actually try to use it is obviously not very large :-) thanks -- PMM
Re: [Qemu-devel] [PATCH v3] block: Handle error of bdrv_getlength in bdrv_create_dirty_bitmap
On 16.04.2014 03:34, Fam Zheng wrote: bdrv_getlength could fail, check the return value before using it. Return NULL and set errno if it fails. Callers are updated to handle the error case. Signed-off-by: Fam Zheng --- v3: Fix errno (Max) Signed-off-by: Fam Zheng --- block-migration.c | 30 ++ block.c | 11 +-- block/mirror.c| 5 - include/block/block.h | 3 ++- 4 files changed, 41 insertions(+), 8 deletions(-) Reviewed-by: Max Reitz
Re: [Qemu-devel] [PATCH] Unnecessary comma.
Oh, my mistake. It's really semicolon, not comma :) Should I resend the patch with correct name? 16.04.2014 21:58, Peter Maydell пишет: On 16 April 2014 18:32, Stefan Weil wrote: Am 16.04.2014 15:43, schrieb Igor Ryzhov: Signed-off-by: Igor Ryzhov --- net/net.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/net.c b/net/net.c index e3ef1e4..60a07f1 100644 --- a/net/net.c +++ b/net/net.c @@ -473,7 +473,7 @@ ssize_t qemu_deliver_packet(NetClientState *sender, if (ret == 0) { nc->receive_disabled = 1; -}; +} return ret; } Reviewed-by: Stefan Weil CC'ing qemu-trivial PS. The "." at the end of your subject line is unnecessary, too. :-) "net/net.c: Remove unnecessary semicolon" would probably be better (since it lets people know which area the patch is affecting and also gets the name of the punctuation mark right ;-)). thanks -- PMM
Re: [Qemu-devel] Change of TEXT_OFFSET for multi_v7_defconfig
Hi Nicolas, Thanks for your response. On 04/16/2014 03:14 PM, Nicolas Pitre wrote: > On Wed, 16 Apr 2014, Christopher Covington wrote: > >> On 04/15/2014 06:44 AM, Daniel Thompson wrote: >>> Hi Folks >>> >>> I've just been rebasing some of my development branches against v3.15rc1 >>> and observed some boot regressions due to TEXT_OFFSET changing from >>> 0x8000 to 0x208000. >>> >>> Now the boot regression turned out to be fault in the JTAG boot tools I >>> was using (it had internally hardcoded to TEXT_OFFSET to 0x8000 when >>> calculating what physical load address to use). I've fixed the JTAG >>> loader and my own boards now boots fine. >> >> Your tools are not alone in being affected by this change. QEMU is >> considering >> changing their hard-coded value to 0x8000 [1], which I was eager to see until >> being reminded of this (that patch would still be an improvement, but not >> enough for users of new multiplatform kernels). >> >> The boot-wrapper [2] (the default bootloader for ARM's proprietary models >> which could potentially be used on other systems) is also affected. > > Why would QEMU and the ARM boot-wrap-per care about the kernel > TEXT_OFFSET value? The simulators I'm familiar with all have the equivalent of a built-in JTAG debugger, capable of peeking at and poking memory, servicing Angel semihosting calls, and so on. Knowledge of the TEXT_OFFSET is required for loading non-self-uncompressing images for the same reasons as when using JTAG on real hardware, as I understand it. > I may understand the desire to boot a plain uncompressed Image over JTAG > and in this case you are amongst a very small group of people doing so > and therefore should be knowing what you're doing. In other words this > is a minor inconvenient to a few people. I didn't mean to imply that there is a large user base for this style of loading, just that an approach that works across multiple tools would be nice if change is warranted. > But both QEMU and the boot-wrapper should deal with zImage. That's the > only image format with documented load offset is guaranteed not to > change i.e. it can be loaded at about any offset as zImage knows how to > relocate itself as needed. There is nowhere a guarantee that > TEXT_OFFSET can't change. QEMU definitely does support zImage and I believe it's promoted as the main boot method. I would expect the bootwrapper to work with zImages, as its (in the non-semihosting case) basically just packing the kernel, device tree and initramfs up into an ELF file that's loaded into memory by a simulator's built-in JTAG-like loader. > And if you think booting zImage on ARM models is too slow, then simply > try out CONFIG_KERNEL_LZO. Thanks for the tip. >> My current thinking is that even if we temporarily removed variance (the >> jumping about) by maybe building every image with the maximum offset that any >> image could have, there would still be variance between images built before >> and after that change, and maybe also when some new platform gets added with >> an even higher offset. So if there's going to be variance, could we maybe >> make >> it no longer a problem? > > There is already no problem with zImage. > >> It seems to me that if external/uncompressed image loaders could query the >> text offset in a straightforward manner, variance between images could be >> easily dealt with. Would reading it out of ELF metadata be a reasonable >> mechanism? Could the ELF format vmlinux be a suitable general replacement for >> the raw Image? > > The ELF image only has virtual addresses in it. And the virtual address > of the kernel may be changed independently of TEXT_OFFSET with > CONFIG_VMSPLIT_*. Do you know why this is the case? The ELF format is capable of storing physical addresses as mentioned below. >> Now at least with my current .config, the vmlinux only has virtual addresses. >> Documentation/arm/Booting says the MMU has to be off at boot time so this >> still might not be the ideal input for image loaders. Tools could hard-code >> the phsyical-to-virtual offset instead of the TEXT_OFFSET. Is that less >> likely >> to vary? > > Physical offset does vary from one platform to another, so this > particular physical-to-virtual offset is actually determined at run time > and the kernel runtime patched during early boot -- see __fixup_pv_table > in arch/arm/kernel/head.S. What I meant to ask about was variance from one kernel version and build to the next, given a single platform. Platform-to-platform variation can probably be abstracted where needed by the scripts controlling the external load. In any case, CONFIG_VMSPLIT_* that you mentioned above would be an example where it would vary in an inconvenient manner, so this approach wouldn't be an improvement. >> Or could we patch up the linker script to set zero-based ELF load >> memory addresses (LMAs) [4] so that the physical addresses are almost right, >> you just might have to add a system-speci
Re: [Qemu-devel] [QEMU v6 PATCH 00/17] SMBIOS: build full tables in QEMU
OK, so I have the "legacy" (field-by-field, types 0 and 1 only) code back in, right next to the new aggregate-smbios-table-plus-entrypoint code, tested and apparently working fine. Before I get carried away with "git rebase", do we still want to go through the whole patch sequence of generating aggregate tables identical to what SeaBIOS would have (including e.g. type 20), then drop type 20 and upgrade to smbios spec v2.8, etc ? Since we're keeping the legacy code, I can add in the new code (only for machine types >= 2.1) directly as smbios spec v2.8 compliant (mainly without adding in generation for type 20, then ripping it right back out again). This would result in a smaller, cleaner patch set. Any objections ? On Wed, Apr 16, 2014 at 08:41:51AM +0200, Gerd Hoffmann wrote: > Global variable, simliar to smbios_type1_defaults. machine type init > code will set it accordingly. smbios code just does "if > (smbios_generate_tables) { ... }" (or however we name this). OK, so right now I'm parsing the "version" argument to what is currently smbios_set_type1_defaults() (and will become smbios_set_defaults() after patching). This gets passed in by pc_piix.c or pc_q35.c from args->machine->name, and comes in turn from the various machine definitions in pc_[q35|piix].c. I noticed some of the older QEMUMachine structures have a hw_version member set, but the latest I could find being set explicitly is 1.0, and only in pc_piix.c. On current (2.0) piix and q35 machines, the hw_version field is NULL, is that a bug or a feature ? If the latter (feature), is there a better way to find out which side of "2.1" I'm currently on than by scraping it out of args->machine->name ? Thanks, --Gabriel
Re: [Qemu-devel] [PATCH v4 2/4] util/fifo: Generalise for common integer widths
On 04/15/14 13:26, Beniamino Galvani wrote: On Mon, Apr 14, 2014 at 08:18:56PM -0700, Peter Crosthwaite wrote: Add support for 16, 32 and 64 bit width FIFOs. The push and pop functions are replicated to accept all four different integer types. The element width of the FIFO is set at creation time. The backing storage for all element types is still uint8_t regardless of element width so some save-load logic is needed to handle endianness issue WRT VMSD. Signed-off-by: Peter Crosthwaite --- changed since v3: Initialised buffer_size properly (Beniamino review) changed since v2: replicated (and glueified) the push/pop functions (Don Slutz review). Fix "each each" typo (Beniamino review). Done use "Case(n):" (Beniamino review). hw/char/serial.c| 4 +- hw/net/allwinner_emac.c | 6 +-- hw/ssi/xilinx_spi.c | 4 +- hw/ssi/xilinx_spips.c | 4 +- include/qemu/fifo.h | 33 ++--- util/fifo.c | 121 +--- 6 files changed, 128 insertions(+), 44 deletions(-) Looks good to me, Reviewed-by: Beniamino Galvani Also: Reviewed-by: Don Slutz
Re: [Qemu-devel] [PATCH v2] qmp: object-add: Validate class before creating object
On 04/16/2014 01:39 PM, Eduardo Habkost wrote: > Currently it is very easy to crash QEMU by issuing an object-add command > using an abstract class or a class that doesn't support > TYPE_USER_CREATABLE as parameter. > > Example: with the following QMP command: > > (QEMU) object-add qom-type=cpu id=foo > > QEMU aborts at: > > ERROR:qom/object.c:335:object_initialize_with_type: assertion failed: > (type->abstract == false) > > This patch moves the check for TYPE_USER_CREATABLE before object_new(), > and adds a check to prevent the code from trying to instantiate abstract > classes. > > Signed-off-by: Eduardo Habkost > --- > Changes v2: > * Change ordering: first check for TYPE_USER_CREATABLE and then check >if class is abstract. This makes the ordering of checks closer to >what's already done on device_add. Changes look fine to me, re-tested to verify. Reviewed-by: Matthew Rosato Tested-by: Matthew Rosato > --- > qmp.c | 21 ++--- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/qmp.c b/qmp.c > index 87a28f7..9a93ab1 100644 > --- a/qmp.c > +++ b/qmp.c > @@ -540,14 +540,27 @@ void object_add(const char *type, const char *id, const > QDict *qdict, > Visitor *v, Error **errp) > { > Object *obj; > +ObjectClass *klass; > const QDictEntry *e; > Error *local_err = NULL; > > -if (!object_class_by_name(type)) { > +klass = object_class_by_name(type); > +if (!klass) { > error_setg(errp, "invalid class name"); > return; > } > > +if (!object_class_dynamic_cast(klass, TYPE_USER_CREATABLE)) { > +error_setg(errp, "object type '%s' isn't supported by object-add", > + type); > +return; > +} > + > +if (object_class_is_abstract(klass)) { > +error_setg(errp, "object type '%s' is abstract", type); > +return; > +} > + > obj = object_new(type); > if (qdict) { > for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) { > @@ -558,12 +571,6 @@ void object_add(const char *type, const char *id, const > QDict *qdict, > } > } > > -if (!object_dynamic_cast(obj, TYPE_USER_CREATABLE)) { > -error_setg(&local_err, "object type '%s' isn't supported by > object-add", > - type); > -goto out; > -} > - > user_creatable_complete(obj, &local_err); > if (local_err) { > goto out; >
Re: [Qemu-devel] Change of TEXT_OFFSET for multi_v7_defconfig
On Wed, 16 Apr 2014, Christopher Covington wrote: > On 04/15/2014 06:44 AM, Daniel Thompson wrote: > > Hi Folks > > > > I've just been rebasing some of my development branches against v3.15rc1 > > and observed some boot regressions due to TEXT_OFFSET changing from > > 0x8000 to 0x208000. > > > > Now the boot regression turned out to be fault in the JTAG boot tools I > > was using (it had internally hardcoded to TEXT_OFFSET to 0x8000 when > > calculating what physical load address to use). I've fixed the JTAG > > loader and my own boards now boots fine. > > Your tools are not alone in being affected by this change. QEMU is considering > changing their hard-coded value to 0x8000 [1], which I was eager to see until > being reminded of this (that patch would still be an improvement, but not > enough for users of new multiplatform kernels). > > The boot-wrapper [2] (the default bootloader for ARM's proprietary models > which could potentially be used on other systems) is also affected. Why would QEMU and the ARM boot-wrap-per care about the kernel TEXT_OFFSET value? I may understand the desire to boot a plain uncompressed Image over JTAG and in this case you are amongst a very small group of people doing so and therefore should be knowing what you're doing. In other words this is a minor inconvenient to a few people. But both QEMU and the boot-wrapper should deal with zImage. That's the only image format with documented load offset is guaranteed not to change i.e. it can be loaded at about any offset as zImage knows how to relocate itself as needed. There is nowhere a guarantee that TEXT_OFFSET can't change. And if you think booting zImage on ARM models is too slow, then simply try out CONFIG_KERNEL_LZO. > My current thinking is that even if we temporarily removed variance (the > jumping about) by maybe building every image with the maximum offset that any > image could have, there would still be variance between images built before > and after that change, and maybe also when some new platform gets added with > an even higher offset. So if there's going to be variance, could we maybe make > it no longer a problem? There is already no problem with zImage. > It seems to me that if external/uncompressed image loaders could query the > text offset in a straightforward manner, variance between images could be > easily dealt with. Would reading it out of ELF metadata be a reasonable > mechanism? Could the ELF format vmlinux be a suitable general replacement for > the raw Image? The ELF image only has virtual addresses in it. And the virtual address of the kernel may be changed independently of TEXT_OFFSET with CONFIG_VMSPLIT_*. > Now at least with my current .config, the vmlinux only has virtual addresses. > Documentation/arm/Booting says the MMU has to be off at boot time so this > still might not be the ideal input for image loaders. Tools could hard-code > the phsyical-to-virtual offset instead of the TEXT_OFFSET. Is that less likely > to vary? Physical offset does vary from one platform to another, so this particular physical-to-virtual offset is actually determined at run time and the kernel runtime patched during early boot -- see __fixup_pv_table in arch/arm/kernel/head.S. > Or could we patch up the linker script to set zero-based ELF load > memory addresses (LMAs) [4] so that the physical addresses are almost right, > you just might have to add a system-specific RAM offset, perhaps pulled out of > the device tree? If that won't work, we could generate some kind of > vmlinux-phys with physical addresses. The latter two options might also > simplify external debugging before __turn_mmu_on(). I like the sound of the > LMA approach best, assuming it doesn't break existing stuff (I notice a few AT > directives in vmlinux.lds.S). Some of this might transfer to arm64 as well. > What do you all think? If you really really want to get at the TEXT_OFFSET value in the uncompressed image, the simplest way would be: diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S index f8c08839ed..de84d0635a 100644 --- a/arch/arm/kernel/head.S +++ b/arch/arm/kernel/head.S @@ -78,6 +78,11 @@ __HEAD ENTRY(stext) + + b 1f + .word TEXT_OFFSET @ located at a 4-byte offset in Image +1: + ARM_BE8(setendbe )@ ensure we are in BE8 mode THUMB(adr r9, BSYM(1f)) @ Kernel is always entered in ARM. This way the first word for Image would always be 0xea00 and the second one would be TEXT_OFFSET. No other kernel Image binaries ever had 0xea00 as their first word so that also let you validate whether or not the TEXT_OFFSET value is there. Nicolas
Re: [Qemu-devel] [PATCH v24 00/31] replace QEMUOptionParameter with QemuOpts
On Thu, Apr 10, 2014 at 11:20:46AM +0800, Chunyan Liu wrote: > 2014-04-08 9:12 GMT+08:00 Leandro Dorileo : > > > On Thu, Apr 03, 2014 at 05:54:18PM +0800, Chunyan Liu wrote: > > > This patch series is to replace QEMUOptionParameter with QemuOpts, so > > that only > > > one Qemu Option structure is kept in QEMU code. > > > > > > --- > > > Changes to v23: > > > * Improve conversion functions, make .assigned info not lost. > > > * Update qcow2.c amend_option, keep checking 'assigned'. > > > * Improve qemu_opt_get_*_del, after get option, delete all settings > > > to this option (since in qemu_opt_set, if set option many times, > > > there will be many opts in the list for the same option). > > > * Some other fixes for qemu-iotests > > > * Other fixes to v23 comments > > > > > > Chunyan Liu (31): > > > QemuOpts: move find_desc_by_name ahead for later calling > > > QemuOpts: add def_value_str to QemuOptDesc > > > qapi: output def_value_str when query command line options > > > QemuOpts: change opt->name|str from (const char *) to (char *) > > > QemuOpts: move qemu_opt_del ahead for later calling > > > QemuOpts: add qemu_opt_get_*_del functions for replace work > > > QemuOpts: add qemu_opts_print_help to replace print_option_help > > > QemuOpts: add conversion between QEMUOptionParameter to QemuOpts > > > QemuOpts: add qemu_opts_append to replace append_option_parameters > > > QemuOpts: check NULL input for qemu_opts_del > > > qemu_opts_print: change fprintf stderr to printf > > > change block layer to support both QemuOpts and QEMUOptionParamter > > > vvfat.c: handle cross_driver's create_options and create_opts > > > cow.c: replace QEMUOptionParameter with QemuOpts > > > gluster.c: replace QEMUOptionParameter with QemuOpts > > > iscsi.c: replace QEMUOptionParameter with QemuOpts > > > qcow.c: replace QEMUOptionParameter with QemuOpts > > > qcow2.c: replace QEMUOptionParameter with QemuOpts > > > qed.c: replace QEMUOptionParameter with QemuOpts > > > raw-posix.c: replace QEMUOptionParameter with QemuOpts > > > raw-win32.c: replace QEMUOptionParameter with QemuOpts > > > raw_bsd.c: replace QEMUOptionParameter with QemuOpts > > > rbd.c: replace QEMUOptionParameter with QemuOpts > > > sheepdog.c: replace QEMUOptionParameter with QemuOpts > > > ssh.c: replace QEMUOptionParameter with QemuOpts > > > vdi.c: replace QEMUOptionParameter with QemuOpts > > > vhdx.c: replace QEMUOptionParameter with QemuOpts > > > vmdk.c: replace QEMUOptionParameter with QemuOpts > > > vpc.c: replace QEMUOptionParameter with QemuOpts > > > cleanup QEMUOptionParameter > > > QemuOpts: cleanup tmp 'allocated' member from QemuOptsList > > > > > > block/nfs.c is missing conversion. Have you tested your own patches? > > A simple git am && config && make is enough to break the build. > > > > Another case that's caused because "libnfs supportno" and nfs.c is not > compiled. I'll update and check if there is other files missing. But is > there > any configure option that could enable all drivers? Not that I'm aware of. > For those changed backend drivers above, of course have been tested > "make && tests/qemu-iotests/check". Good... -- Dorileo > > Chunyan > > > > --- > > Leandro Dorileo > > > > > > > > > > block.c | 96 > > > block/cow.c | 52 ++-- > > > block/gluster.c | 73 +++--- > > > block/iscsi.c | 32 ++- > > > block/qcow.c | 72 +++--- > > > block/qcow2.c | 264 +++-- > > > block/qed.c | 112 - > > > block/qed.h | 3 +- > > > block/raw-posix.c | 55 ++--- > > > block/raw-win32.c | 38 +-- > > > block/raw_bsd.c | 25 +- > > > block/rbd.c | 61 +++-- > > > block/sheepdog.c | 102 > > > block/ssh.c | 30 ++- > > > block/vdi.c | 71 +++--- > > > block/vhdx.c | 97 > > > block/vhdx.h | 1 + > > > block/vmdk.c | 121 +- > > > block/vpc.c | 60 ++--- > > > block/vvfat.c | 11 +- > > > include/block/block.h | 7 +- > > > include/block/block_int.h | 9 +- > > > include/qemu/option.h | 53 + > > > include/qemu/option_int.h | 4 +- > > > qapi-schema.json | 6 +- > > > qapi/opts-visitor.c | 10 +- > > > qemu-img.c| 89 --- > > > qmp-commands.hx | 2 + > > > util/qemu-config.c| 4 + > > > util/qemu-option.c| 587 > > -- > > > 30 files changed, 1029 insertions(+), 1118 deletions(-) > > > > > > -- > > > 1.7.12.4 > > > > > > >
Re: [Qemu-devel] [PATCH v24 02/31] QemuOpts: add def_value_str to QemuOptDesc
On Thu, Apr 10, 2014 at 11:36:05AM +0800, Chunyan Liu wrote: > 2014-04-08 9:31 GMT+08:00 Leandro Dorileo : > > > On Thu, Apr 03, 2014 at 05:54:20PM +0800, Chunyan Liu wrote: > > > Add def_value_str (default value) to QemuOptDesc, to replace function of > > the > > > default value in QEMUOptionParameter. > > > > > > Improve qemu_opts_get_* functions: if find opt, return opt->str; > > otherwise, > > > if desc->def_value_str is set, return desc->def_value_str; otherwise, > > return > > > input defval. > > > > > > Improve qemu_opts_print: if option is set, print opt->str; otherwise, if > > > desc->def_value_str is set, also print it. > > > > > > Reviewed-by: Eric Blake > > > Signed-off-by: Dong Xu Wang > > > Signed-off-by: Chunyan Liu > > > --- > > > include/qemu/option.h | 3 ++- > > > util/qemu-option.c| 56 > > ++- > > > 2 files changed, 49 insertions(+), 10 deletions(-) > > > > > > diff --git a/include/qemu/option.h b/include/qemu/option.h > > > index 8c0ac34..c3b0a91 100644 > > > --- a/include/qemu/option.h > > > +++ b/include/qemu/option.h > > > @@ -99,6 +99,7 @@ typedef struct QemuOptDesc { > > > const char *name; > > > enum QemuOptType type; > > > const char *help; > > > +const char *def_value_str; > > > } QemuOptDesc; > > > > > > I still think you don't need to define the default value as a string to > > later > > parse it, see what I mean > > https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg04273.html > > > > But I think we can live with it. See bellow, if you fix the issue you can > > add my Reviewed-by. > > > > > > > > > > struct QemuOptsList { > > > @@ -156,7 +157,7 @@ QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict > > *qdict); > > > void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp); > > > > > > typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque); > > > -int qemu_opts_print(QemuOpts *opts, void *dummy); > > > +void qemu_opts_print(QemuOpts *opts); > > > int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void > > *opaque, > > >int abort_on_failure); > > > > > > diff --git a/util/qemu-option.c b/util/qemu-option.c > > > index e6d10bc..d5e80da 100644 > > > --- a/util/qemu-option.c > > > +++ b/util/qemu-option.c > > > @@ -570,6 +570,13 @@ static QemuOpt *qemu_opt_find(QemuOpts *opts, const > > char *name) > > > const char *qemu_opt_get(QemuOpts *opts, const char *name) > > > { > > > QemuOpt *opt = qemu_opt_find(opts, name); > > > + > > > +if (!opt) { > > > +const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, > > name); > > > +if (desc && desc->def_value_str) { > > > +return desc->def_value_str; > > > +} > > > +} > > > return opt ? opt->str : NULL; > > > } > > > > > > @@ -589,8 +596,13 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char > > *name, bool defval) > > > { > > > QemuOpt *opt = qemu_opt_find(opts, name); > > > > > > -if (opt == NULL) > > > +if (opt == NULL) { > > > +const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, > > name); > > > +if (desc && desc->def_value_str) { > > > +parse_option_bool(name, desc->def_value_str, &defval, > > &error_abort); > > > +} > > > return defval; > > > +} > > > assert(opt->desc && opt->desc->type == QEMU_OPT_BOOL); > > > return opt->value.boolean; > > > } > > > @@ -599,8 +611,14 @@ uint64_t qemu_opt_get_number(QemuOpts *opts, const > > char *name, uint64_t defval) > > > { > > > QemuOpt *opt = qemu_opt_find(opts, name); > > > > > > -if (opt == NULL) > > > +if (opt == NULL) { > > > +const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, > > name); > > > +if (desc && desc->def_value_str) { > > > +parse_option_number(name, desc->def_value_str, &defval, > > > +&error_abort); > > > +} > > > return defval; > > > +} > > > assert(opt->desc && opt->desc->type == QEMU_OPT_NUMBER); > > > return opt->value.uint; > > > } > > > @@ -609,8 +627,13 @@ uint64_t qemu_opt_get_size(QemuOpts *opts, const > > char *name, uint64_t defval) > > > { > > > QemuOpt *opt = qemu_opt_find(opts, name); > > > > > > -if (opt == NULL) > > > +if (opt == NULL) { > > > +const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, > > name); > > > +if (desc && desc->def_value_str) { > > > +parse_option_size(name, desc->def_value_str, &defval, > > &error_abort); > > > +} > > > return defval; > > > +} > > > assert(opt->desc && opt->desc->type == QEMU_OPT_SIZE); > > > return opt->value.uint; > > > } > > > @@ -895,17 +918,32 @@ void qemu_opts_del(QemuOpts *opts) > > > g_free(opts); > > > } > > > > > > -int qemu_opts_print(QemuOpts *opts, void *dummy) > > > +void qemu_opts_print(QemuOpts *opts) > > > { >
Re: [Qemu-devel] QEMU APIC version ID bug?
APIC ver register is 1 but *IO*APIC ver register is 3... sorry for the confusion/noise, qemu looks fine neil On 4/16/14 10:16 AM, Neil McGill wrote: Seems there is a bug in qemu where the APIC version is being checked as value 3. However, it should be 1. static uint32_t apic_mem_readl(void *opaque, hwaddr addr) { ... switch(index) { case 0x03: /* version */ val = 0x11 | ((APIC_LVT_NB - 1) << 16); /* version 0x11 */ break; Comparing to KVM, it has the correct value: static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic, unsigned long addr, unsigned long length) { unsigned long result = 0; switch (ioapic->ioregsel) { case IOAPIC_REG_VERSION: result = IOAPIC_NUM_PINS - 1) & 0xff) << 16) | (IOAPIC_VERSION_ID & 0xff)); break; ./virt/kvm/ioapic.h:#define IOAPIC_REG_VERSION 0x01 I hit this as we have on OS that was checking for the number of IRQ pins and we were falling into the default case. Is this a known issue? What's the best way to submit a patch/get this committed if it is an accepted bug? tx neil
Re: [Qemu-devel] [PATCH RFC qom-next for-next v2 6/6] pci: Move VMSTATE_MSIX() into vmstate_pci_device
On Wed, Apr 16, 2014 at 04:22:32PM +0200, Andreas Färber wrote: > Am 02.09.2013 13:31, schrieb Michael S. Tsirkin: > > On Mon, Jul 29, 2013 at 02:27:01AM +0200, Andreas Färber wrote: > >> Use it conditional on msix_present() and drop msix_{save,load}() calls > >> following pci_device_{save,load}(). > >> > >> This reorders the msix_save() and msix_unuse_all_vectors() calls for > >> virtio-pci, but they seem independent of each other. > > > > No, that's a bug. msix_unuse_all_vectors clears pending state > > if any, it will lose state if called before load. > > Michael, you were just saying no here, now MegaSAS is forgetting to add > appropriate VMState. How do you envision solving that bug? Can we move > msix_unuse_all_vectors() to common code or something? I agree it's a good idea to find a way to fix this. I'll look into this, thanks for the reminder. > FTR, Stefan had requested on IRC that vmxnet state not be changed > incompatibly. What we discussed there was to register the legacy savevm > handler only for reading incoming state (NULL for writing new state); > but I am no longer sure that could work due to new VMSTATE_PCI(). > > Dmitry, why is vmxnet using such a non-standard way of registering > VMState for MSI-X, and can you please help to fix that for the benefit > of all PCI devices? > > Thanks, > Andreas > > >> > >> Signed-off-by: Andreas Färber > >> --- > >> hw/misc/ivshmem.c | 7 ++- > >> hw/net/vmxnet3.c | 27 +++ > >> hw/pci/pci.c | 8 > >> hw/usb/hcd-xhci.c | 1 - > >> hw/virtio/virtio-pci.c | 19 +++ > >> include/hw/pci/msix.h | 7 --- > >> 6 files changed, 28 insertions(+), 41 deletions(-) > >> > >> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > >> index 4a74856..de997cd 100644 > >> --- a/hw/misc/ivshmem.c > >> +++ b/hw/misc/ivshmem.c > >> @@ -599,9 +599,7 @@ static void ivshmem_save(QEMUFile* f, void *opaque) > >> IVSHMEM_DPRINTF("ivshmem_save\n"); > >> pci_device_save(pci_dev, f); > >> > >> -if (ivshmem_has_feature(proxy, IVSHMEM_MSI)) { > >> -msix_save(pci_dev, f); > >> -} else { > >> +if (!ivshmem_has_feature(proxy, IVSHMEM_MSI)) { > >> qemu_put_be32(f, proxy->intrstatus); > >> qemu_put_be32(f, proxy->intrmask); > >> } > >> @@ -631,8 +629,7 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int > >> version_id) > >> } > >> > >> if (ivshmem_has_feature(proxy, IVSHMEM_MSI)) { > >> -msix_load(pci_dev, f); > >> - ivshmem_use_msix(proxy); > >> +ivshmem_use_msix(proxy); > >> } else { > >> proxy->intrstatus = qemu_get_be32(f); > >> proxy->intrmask = qemu_get_be32(f); > >> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c > >> index 3bad83c..471ca24 100644 > >> --- a/hw/net/vmxnet3.c > >> +++ b/hw/net/vmxnet3.c > >> @@ -2031,21 +2031,6 @@ vmxnet3_cleanup_msi(VMXNET3State *s) > >> } > >> } > >> > >> -static void > >> -vmxnet3_msix_save(QEMUFile *f, void *opaque) > >> -{ > >> -PCIDevice *d = PCI_DEVICE(opaque); > >> -msix_save(d, f); > >> -} > >> - > >> -static int > >> -vmxnet3_msix_load(QEMUFile *f, void *opaque, int version_id) > >> -{ > >> -PCIDevice *d = PCI_DEVICE(opaque); > >> -msix_load(d, f); > >> -return 0; > >> -} > >> - > >> static const MemoryRegionOps b0_ops = { > >> .read = vmxnet3_io_bar0_read, > >> .write = vmxnet3_io_bar0_write, > >> @@ -2103,9 +2088,6 @@ static int vmxnet3_pci_init(PCIDevice *pci_dev) > >> > >> vmxnet3_net_init(s); > >> > >> -register_savevm(dev, "vmxnet3-msix", -1, 1, > >> -vmxnet3_msix_save, vmxnet3_msix_load, s); > >> - > >> add_boot_device_path(s->conf.bootindex, dev, "/ethernet-phy@0"); > >> > >> return 0; > >> @@ -2114,13 +2096,10 @@ static int vmxnet3_pci_init(PCIDevice *pci_dev) > >> > >> static void vmxnet3_pci_uninit(PCIDevice *pci_dev) > >> { > >> -DeviceState *dev = DEVICE(pci_dev); > >> VMXNET3State *s = VMXNET3(pci_dev); > >> > >> VMW_CBPRN("Starting uninit..."); > >> > >> -unregister_savevm(dev, "vmxnet3-msix", s); > >> - > >> vmxnet3_net_uninit(s); > >> > >> vmxnet3_cleanup_msix(s); > >> @@ -2372,9 +2351,9 @@ const VMStateInfo int_state_info = { > >> > >> static const VMStateDescription vmstate_vmxnet3 = { > >> .name = "vmxnet3", > >> -.version_id = 1, > >> -.minimum_version_id = 1, > >> -.minimum_version_id_old = 1, > >> +.version_id = 2, > >> +.minimum_version_id = 2, > >> +.minimum_version_id_old = 2, > >> .pre_save = vmxnet3_pre_save, > >> .post_load = vmxnet3_post_load, > >> .fields = (VMStateField[]) { > >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c > >> index b790d98..bd6078b 100644 > >> --- a/hw/pci/pci.c > >> +++ b/hw/pci/pci.c > >> @@ -481,6 +481,13 @@ static bool pci_device_aer_needed(void *opaque, int > >> version_id) > >> return pci_is_express(s) &
Re: [Qemu-devel] [PATCH] Unnecessary comma.
On 16 April 2014 18:32, Stefan Weil wrote: > Am 16.04.2014 15:43, schrieb Igor Ryzhov: >> Signed-off-by: Igor Ryzhov >> --- >> net/net.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/net.c b/net/net.c >> index e3ef1e4..60a07f1 100644 >> --- a/net/net.c >> +++ b/net/net.c >> @@ -473,7 +473,7 @@ ssize_t qemu_deliver_packet(NetClientState *sender, >> >> if (ret == 0) { >> nc->receive_disabled = 1; >> -}; >> +} >> >> return ret; >> } >> > > Reviewed-by: Stefan Weil > > CC'ing qemu-trivial > > PS. The "." at the end of your subject line is unnecessary, too. :-) "net/net.c: Remove unnecessary semicolon" would probably be better (since it lets people know which area the patch is affecting and also gets the name of the punctuation mark right ;-)). thanks -- PMM
Re: [Qemu-devel] [PATCH 2/7] megasas: Enable MSI-X support
On Wed, Apr 16, 2014 at 07:47:37PM +0200, Andreas Färber wrote: > Am 16.04.2014 19:40, schrieb Michael S. Tsirkin: > > On Wed, Apr 16, 2014 at 06:48:08PM +0200, Andreas Färber wrote: > >> Am 16.04.2014 18:32, schrieb Alexander Graf: > >>> > >>> On 16.04.14 16:44, Hannes Reinecke wrote: > MSI-X support has been fixed in qemu, so we can enable it again. > > Signed-off-by: Hannes Reinecke > --- > hw/scsi/megasas.c | 19 ++- > 1 file changed, 6 insertions(+), 13 deletions(-) > > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c > index 1781525..df45286 100644 > --- a/hw/scsi/megasas.c > +++ b/hw/scsi/megasas.c > @@ -2084,7 +2084,8 @@ static const VMStateDescription vmstate_megasas = { > .minimum_version_id = 0, > .minimum_version_id_old = 0, > .fields = (VMStateField[]) { > -VMSTATE_PCI_DEVICE(parent_obj, MegasasState), > +VMSTATE_PCIE_DEVICE(parent_obj, MegasasState), > +VMSTATE_MSIX(parent_obj, MegasasState), > >>> > >>> This requires a version change for vmstate, no? > >> > >> The PCI -> PCIE change yes. mst might have objections for bumping an x86 > >> PCI device? > > > > Yes. > > It should be possible to make this conditional on having a pcie > > capability, and disable pcie capability for old pc versions. > > I did have a patch merging their vmstate_ structs based on some test in > the series I ping'ed, unfortunately the difference was mostly config > space size, so not immediately obvious how to set via compat_props, but > likely solvable somehow. Check pci_is_express ? > >> The MSIX addition should be safe AFAICT since old versions would not > >> enable MSI-X. > > > > Yes but I don't see a code like this - where's code in PC > > disabling msix for old versions? > > The default value for use_msix (use-msix?) in v2 is already false, so no > need to set it to false again in compat_props? > > Andreas Ah, I didn't notice that. Sure, if it's off by default there's no need for compat code. > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
[Qemu-devel] [PATCH v2] qmp: object-add: Validate class before creating object
Currently it is very easy to crash QEMU by issuing an object-add command using an abstract class or a class that doesn't support TYPE_USER_CREATABLE as parameter. Example: with the following QMP command: (QEMU) object-add qom-type=cpu id=foo QEMU aborts at: ERROR:qom/object.c:335:object_initialize_with_type: assertion failed: (type->abstract == false) This patch moves the check for TYPE_USER_CREATABLE before object_new(), and adds a check to prevent the code from trying to instantiate abstract classes. Signed-off-by: Eduardo Habkost --- Changes v2: * Change ordering: first check for TYPE_USER_CREATABLE and then check if class is abstract. This makes the ordering of checks closer to what's already done on device_add. --- qmp.c | 21 ++--- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/qmp.c b/qmp.c index 87a28f7..9a93ab1 100644 --- a/qmp.c +++ b/qmp.c @@ -540,14 +540,27 @@ void object_add(const char *type, const char *id, const QDict *qdict, Visitor *v, Error **errp) { Object *obj; +ObjectClass *klass; const QDictEntry *e; Error *local_err = NULL; -if (!object_class_by_name(type)) { +klass = object_class_by_name(type); +if (!klass) { error_setg(errp, "invalid class name"); return; } +if (!object_class_dynamic_cast(klass, TYPE_USER_CREATABLE)) { +error_setg(errp, "object type '%s' isn't supported by object-add", + type); +return; +} + +if (object_class_is_abstract(klass)) { +error_setg(errp, "object type '%s' is abstract", type); +return; +} + obj = object_new(type); if (qdict) { for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) { @@ -558,12 +571,6 @@ void object_add(const char *type, const char *id, const QDict *qdict, } } -if (!object_dynamic_cast(obj, TYPE_USER_CREATABLE)) { -error_setg(&local_err, "object type '%s' isn't supported by object-add", - type); -goto out; -} - user_creatable_complete(obj, &local_err); if (local_err) { goto out; -- 1.9.0
Re: [Qemu-devel] [PATCH 2/7] megasas: Enable MSI-X support
Am 16.04.2014 19:40, schrieb Michael S. Tsirkin: > On Wed, Apr 16, 2014 at 06:48:08PM +0200, Andreas Färber wrote: >> Am 16.04.2014 18:32, schrieb Alexander Graf: >>> >>> On 16.04.14 16:44, Hannes Reinecke wrote: MSI-X support has been fixed in qemu, so we can enable it again. Signed-off-by: Hannes Reinecke --- hw/scsi/megasas.c | 19 ++- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index 1781525..df45286 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -2084,7 +2084,8 @@ static const VMStateDescription vmstate_megasas = { .minimum_version_id = 0, .minimum_version_id_old = 0, .fields = (VMStateField[]) { -VMSTATE_PCI_DEVICE(parent_obj, MegasasState), +VMSTATE_PCIE_DEVICE(parent_obj, MegasasState), +VMSTATE_MSIX(parent_obj, MegasasState), >>> >>> This requires a version change for vmstate, no? >> >> The PCI -> PCIE change yes. mst might have objections for bumping an x86 >> PCI device? > > Yes. > It should be possible to make this conditional on having a pcie > capability, and disable pcie capability for old pc versions. I did have a patch merging their vmstate_ structs based on some test in the series I ping'ed, unfortunately the difference was mostly config space size, so not immediately obvious how to set via compat_props, but likely solvable somehow. >> The MSIX addition should be safe AFAICT since old versions would not >> enable MSI-X. > > Yes but I don't see a code like this - where's code in PC > disabling msix for old versions? The default value for use_msix (use-msix?) in v2 is already false, so no need to set it to false again in compat_props? Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
[Qemu-devel] [PULL for-2.1 07/25] tcg-aarch64: Use adrp in tcg_out_movi
Loading an qemu pointer as an immediate happens often. E.g. - exit_tb $0x7fa8140013 + exit_tb $0x7f81ee0013 ... - : d2800260mov x0, #0x13 - : f2b50280movkx0, #0xa814, lsl #16 - : f2c00fe0movkx0, #0x7f, lsl #32 + : 90ff1000adrpx0, 0x7f81ee + : 91004c00add x0, x0, #0x13 Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 23 +++ 1 file changed, 23 insertions(+) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index a08f6c7..1337a13 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -294,6 +294,10 @@ typedef enum { I3405_MOVZ = 0x5280, I3405_MOVK = 0x7280, +/* PC relative addressing instructions. */ +I3406_ADR = 0x1000, +I3406_ADRP = 0x9000, + /* Add/subtract shifted register instructions (without a shift). */ I3502_ADD = 0x0b00, I3502_ADDS = 0x2b00, @@ -457,6 +461,12 @@ static void tcg_out_insn_3405(TCGContext *s, AArch64Insn insn, TCGType ext, tcg_out32(s, insn | ext << 31 | shift << (21 - 4) | half << 5 | rd); } +static void tcg_out_insn_3406(TCGContext *s, AArch64Insn insn, + TCGReg rd, int64_t disp) +{ +tcg_out32(s, insn | (disp & 3) << 29 | (disp & 0x1c) << (5 - 2) | rd); +} + /* This function is for both 3.5.2 (Add/Subtract shifted register), for the rare occasion when we actually want to supply a shift amount. */ static inline void tcg_out_insn_3502S(TCGContext *s, AArch64Insn insn, @@ -596,6 +606,19 @@ static void tcg_out_movi(TCGContext *s, TCGType type, TCGReg rd, return; } +/* Look for host pointer values within 4G of the PC. This happens + often when loading pointers to QEMU's own data structures. */ +if (type == TCG_TYPE_I64) { +tcg_target_long disp = (value >> 12) - ((intptr_t)s->code_ptr >> 12); +if (disp == sextract64(disp, 0, 21)) { +tcg_out_insn(s, 3406, ADRP, rd, disp); +if (value & 0xfff) { +tcg_out_insn(s, 3401, ADDI, type, rd, rd, value & 0xfff); +} +return; +} +} + /* Would it take fewer insns to begin with MOVN? For the value and its inverse, count the number of 16-bit lanes that are 0. */ for (i = wantinv = imask = 0; i < 64; i += 16) { -- 1.9.0
Re: [Qemu-devel] [PATCH 2/7] megasas: Enable MSI-X support
On Wed, Apr 16, 2014 at 06:48:08PM +0200, Andreas Färber wrote: > Am 16.04.2014 18:32, schrieb Alexander Graf: > > > > On 16.04.14 16:44, Hannes Reinecke wrote: > >> MSI-X support has been fixed in qemu, so we can enable it again. > >> > >> Signed-off-by: Hannes Reinecke > >> --- > >> hw/scsi/megasas.c | 19 ++- > >> 1 file changed, 6 insertions(+), 13 deletions(-) > >> > >> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c > >> index 1781525..df45286 100644 > >> --- a/hw/scsi/megasas.c > >> +++ b/hw/scsi/megasas.c > >> @@ -2084,7 +2084,8 @@ static const VMStateDescription vmstate_megasas = { > >> .minimum_version_id = 0, > >> .minimum_version_id_old = 0, > >> .fields = (VMStateField[]) { > >> -VMSTATE_PCI_DEVICE(parent_obj, MegasasState), > >> +VMSTATE_PCIE_DEVICE(parent_obj, MegasasState), > >> +VMSTATE_MSIX(parent_obj, MegasasState), > > > > This requires a version change for vmstate, no? > > The PCI -> PCIE change yes. mst might have objections for bumping an x86 > PCI device? Yes. It should be possible to make this conditional on having a pcie capability, and disable pcie capability for old pc versions. > The MSIX addition should be safe AFAICT since old versions would not > enable MSI-X. > > Regards, > Andreas Yes but I don't see a code like this - where's code in PC disabling msix for old versions? > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH] qmp: object-add: Validate class before creating object
On Wed, Apr 16, 2014 at 08:53:23AM +0200, Markus Armbruster wrote: > Eduardo Habkost writes: > > > Currently it is very easy to crash QEMU by issuing an object-add command > > using an abstract class or a class that doesn't support > > TYPE_USER_CREATABLE as parameter. > > > > Example: with the following QMP command: > > > > (QEMU) object-add qom-type=cpu id=foo > > > > QEMU aborts at: > > > > ERROR:qom/object.c:335:object_initialize_with_type: assertion failed: > > (type->abstract == false) > > > > This patch moves the check for TYPE_USER_CREATABLE before object_new(), > > and adds a check to prevent the code from trying to instantiate abstract > > classes. > > > > Signed-off-by: Eduardo Habkost > > Related device_add fixes: > > 061e84f qdev-monitor: Avoid device_add crashing on non-device driver name > 2fa4e56 qdev-monitor: Fix crash when device_add is called with abstract driver > 7ea5e78 qdev: Do not let the user try to device_add when it cannot work > > > --- > > qmp.c | 21 ++--- > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/qmp.c b/qmp.c > > index 87a28f7..e7ecbb7 100644 > > --- a/qmp.c > > +++ b/qmp.c > > @@ -540,14 +540,27 @@ void object_add(const char *type, const char *id, > > const QDict *qdict, > > Visitor *v, Error **errp) > > { > > Object *obj; > > +ObjectClass *klass; > > const QDictEntry *e; > > Error *local_err = NULL; > > > > -if (!object_class_by_name(type)) { > > +klass = object_class_by_name(type); > > +if (!klass) { > > error_setg(errp, "invalid class name"); > > return; > > } > > > > +if (object_class_is_abstract(klass)) { > > +error_setg(errp, "object type '%s' is abstract", type); > > +return; > > +} > > + > > +if (!object_class_dynamic_cast(klass, TYPE_USER_CREATABLE)) { > > +error_setg(errp, "object type '%s' isn't supported by object-add", > > + type); > > +return; > > +} > > + > > obj = object_new(type); > > if (qdict) { > > for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) { > > For comparison, this is qdev_device_add(): > > oc = object_class_by_name(driver); > if (!oc) { > const char *typename = find_typename_by_alias(driver); > > if (typename) { > driver = typename; > oc = object_class_by_name(driver); > } > } > > if (!object_class_dynamic_cast(oc, TYPE_DEVICE)) { > qerror_report(ERROR_CLASS_GENERIC_ERROR, > "'%s' is not a valid device model name", driver); > return NULL; > } > > if (object_class_is_abstract(oc)) { > qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", > "non-abstract device type"); > return NULL; > } > > dc = DEVICE_CLASS(oc); > if (dc->cannot_instantiate_with_device_add_yet) { > qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver", > "pluggable device type"); > return NULL; > } > > Got the "subtype of" and the "not abstract" check in the opposite order, > and the additional cannot_instantiate_with_device_add_yet check. > > Does the different order matter? The order probably doesn't matter very much. But it makes sense to keep it similar to device_add for consistency. I will send v2. > > @@ -558,12 +571,6 @@ void object_add(const char *type, const char *id, > > const QDict *qdict, > > } > > } > > > > -if (!object_dynamic_cast(obj, TYPE_USER_CREATABLE)) { > > -error_setg(&local_err, "object type '%s' isn't supported by > > object-add", > > - type); > > -goto out; > > -} > > - > > user_creatable_complete(obj, &local_err); > > if (local_err) { > > goto out; -- Eduardo
[Qemu-devel] [PULL for-2.1 00/25] tcg-aarch64 improvements
Thanks for the patience during 5 iterations of this patch set, but it's all reviewed now awaiting the opening of version 2.1. r~ The following changes since commit 851627352c52b5beebf119785885391fa05a44c5: Update version for v2.0.0-rc3 release (2014-04-14 17:45:11 +0100) are available in the git repository at: git://github.com/rth7680/qemu.git tcg-aarch-6-5 for you to fetch changes up to b825025f08823453929ad02cb16dcfbab7eab327: tcg-aarch64: Use tcg_out_mov in preference to tcg_out_movr (2014-04-16 12:13:02 -0400) Richard Henderson (25): tcg-aarch64: Properly detect SIGSEGV writes tcg-aarch64: Use intptr_t apropriately tcg-aarch64: Use TCGType and TCGMemOp constants tcg-aarch64: Use MOVN in tcg_out_movi tcg-aarch64: Use ORRI in tcg_out_movi tcg-aarch64: Special case small constants in tcg_out_movi tcg-aarch64: Use adrp in tcg_out_movi tcg-aarch64: Use symbolic names for branches tcg-aarch64: Create tcg_out_brcond tcg-aarch64: Use CBZ and CBNZ tcg-aarch64: Reuse LR in translated code tcg-aarch64: Introduce tcg_out_insn_3314 tcg-aarch64: Implement tcg_register_jit tcg-aarch64: Avoid add with zero in tlb load tcg-aarch64: Use tcg_out_call for qemu_ld/st tcg-aarch64: Use ADR to pass the return address to the ld/st helpers tcg-aarch64: Use TCGMemOp in qemu_ld/st tcg-aarch64: Pass qemu_ld/st arguments directly tcg-aarch64: Implement TCG_TARGET_HAS_new_ldst tcg-aarch64: Support stores of zero tcg-aarch64: Introduce tcg_out_insn_3507 tcg-aarch64: Merge aarch64_ldst_get_data/type into tcg_out_op tcg-aarch64: Introduce tcg_out_insn_3312, _3310, _3313 tcg-aarch64: Prefer unsigned offsets before signed offsets for ldst tcg-aarch64: Use tcg_out_mov in preference to tcg_out_movr tcg/aarch64/tcg-target.c | 1127 -- tcg/aarch64/tcg-target.h | 34 +- user-exec.c | 25 +- 3 files changed, 628 insertions(+), 558 deletions(-)
Re: [Qemu-devel] [PATCH] Unnecessary comma.
Am 16.04.2014 15:43, schrieb Igor Ryzhov: > Signed-off-by: Igor Ryzhov > --- > net/net.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/net.c b/net/net.c > index e3ef1e4..60a07f1 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -473,7 +473,7 @@ ssize_t qemu_deliver_packet(NetClientState *sender, > > if (ret == 0) { > nc->receive_disabled = 1; > -}; > +} > > return ret; > } > Reviewed-by: Stefan Weil CC'ing qemu-trivial PS. The "." at the end of your subject line is unnecessary, too. :-)
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Wed, Apr 16, 2014 at 05:42:22PM +0100, Peter Maydell wrote: > On 16 April 2014 17:34, Michael S. Tsirkin wrote: > > so it looks like virtio is currently compiled per-target. > > So why isn't it reasonable to keep it per-target for > > purpose of this enhancement? > > What am I missing? > > "virtio" is more than one C file. Currently per-target: > hw/virtio/virtio.c > hw/virtio/virtio-balloon.c > hw/scsi/virtio-scsi.c > hw/block/virtio-blk.c > hw/char/virtio-serial-bus.c > hw/net/virtio-net.c > > Currently built once: > hw/char/virtio-console.c > hw/virtio/virtio-bus.c > hw/virtio/virtio-mmio.c > hw/virtio/virtio-pci.c > hw/virtio/virtio-rng.c > > If we can move files from the first group to the second > that's cool. But doesn't have to be part of this series, yes? I would say let's at least have virtio 1.0 out and implemented in qemu and linux guests, then we can start deprecate virtio 0.X in favor of it. > Moving files from the second to the first is > what I'd like to avoid... > > thanks > -- PMM Absolutely. Looks like we are in agreement after all. So as far as I could see the only issue is with config access e.g. in virtio-pci? That one already has a branch around virtio_is_big_endian, so it's not a problem, there's no need to optimize that. -- MST
[Qemu-devel] [PULL for-2.1 21/25] tcg-aarch64: Introduce tcg_out_insn_3507
Cleaning up the implementation of REV and REV16 at the same time. Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 57 1 file changed, 33 insertions(+), 24 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index caaf8a2..0846835 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -327,6 +327,11 @@ typedef enum { I3506_CSEL = 0x1a80, I3506_CSINC = 0x1a800400, +/* Data-processing (1 source) instructions. */ +I3507_REV16 = 0x5ac00400, +I3507_REV32 = 0x5ac00800, +I3507_REV64 = 0x5ac00c00, + /* Data-processing (2 source) instructions. */ I3508_LSLV = 0x1ac02000, I3508_LSRV = 0x1ac02400, @@ -545,6 +550,12 @@ static void tcg_out_insn_3506(TCGContext *s, AArch64Insn insn, TCGType ext, | tcg_cond_to_aarch64[c] << 12); } +static void tcg_out_insn_3507(TCGContext *s, AArch64Insn insn, TCGType ext, + TCGReg rd, TCGReg rn) +{ +tcg_out32(s, insn | ext << 31 | rn << 5 | rd); +} + static void tcg_out_insn_3509(TCGContext *s, AArch64Insn insn, TCGType ext, TCGReg rd, TCGReg rn, TCGReg rm, TCGReg ra) { @@ -960,20 +971,19 @@ static void tcg_out_brcond(TCGContext *s, TCGMemOp ext, TCGCond c, TCGArg a, } } -static inline void tcg_out_rev(TCGContext *s, TCGType ext, - TCGReg rd, TCGReg rm) +static inline void tcg_out_rev64(TCGContext *s, TCGReg rd, TCGReg rn) { -/* using REV 0x5ac00800 */ -unsigned int base = ext ? 0xdac00c00 : 0x5ac00800; -tcg_out32(s, base | rm << 5 | rd); +tcg_out_insn(s, 3507, REV64, TCG_TYPE_I64, rd, rn); } -static inline void tcg_out_rev16(TCGContext *s, TCGType ext, - TCGReg rd, TCGReg rm) +static inline void tcg_out_rev32(TCGContext *s, TCGReg rd, TCGReg rn) { -/* using REV16 0x5ac00400 */ -unsigned int base = ext ? 0xdac00400 : 0x5ac00400; -tcg_out32(s, base | rm << 5 | rd); +tcg_out_insn(s, 3507, REV32, TCG_TYPE_I32, rd, rn); +} + +static inline void tcg_out_rev16(TCGContext *s, TCGReg rd, TCGReg rn) +{ +tcg_out_insn(s, 3507, REV16, TCG_TYPE_I32, rd, rn); } static inline void tcg_out_sxt(TCGContext *s, TCGType ext, TCGMemOp s_bits, @@ -1205,13 +1215,13 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, TCGMemOp memop, case MO_UW: tcg_out_ldst_r(s, LDST_16, LDST_LD, data_r, addr_r, off_r); if (bswap) { -tcg_out_rev16(s, TCG_TYPE_I32, data_r, data_r); +tcg_out_rev16(s, data_r, data_r); } break; case MO_SW: if (bswap) { tcg_out_ldst_r(s, LDST_16, LDST_LD, data_r, addr_r, off_r); -tcg_out_rev16(s, TCG_TYPE_I32, data_r, data_r); +tcg_out_rev16(s, data_r, data_r); tcg_out_sxt(s, TCG_TYPE_I64, MO_16, data_r, data_r); } else { tcg_out_ldst_r(s, LDST_16, LDST_LD_S_X, data_r, addr_r, off_r); @@ -1220,13 +1230,13 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, TCGMemOp memop, case MO_UL: tcg_out_ldst_r(s, LDST_32, LDST_LD, data_r, addr_r, off_r); if (bswap) { -tcg_out_rev(s, TCG_TYPE_I32, data_r, data_r); +tcg_out_rev32(s, data_r, data_r); } break; case MO_SL: if (bswap) { tcg_out_ldst_r(s, LDST_32, LDST_LD, data_r, addr_r, off_r); -tcg_out_rev(s, TCG_TYPE_I32, data_r, data_r); +tcg_out_rev32(s, data_r, data_r); tcg_out_sxt(s, TCG_TYPE_I64, MO_32, data_r, data_r); } else { tcg_out_ldst_r(s, LDST_32, LDST_LD_S_X, data_r, addr_r, off_r); @@ -1235,7 +1245,7 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, TCGMemOp memop, case MO_Q: tcg_out_ldst_r(s, LDST_64, LDST_LD, data_r, addr_r, off_r); if (bswap) { -tcg_out_rev(s, TCG_TYPE_I64, data_r, data_r); +tcg_out_rev64(s, data_r, data_r); } break; default: @@ -1254,21 +1264,21 @@ static void tcg_out_qemu_st_direct(TCGContext *s, TCGMemOp memop, break; case MO_16: if (bswap && data_r != TCG_REG_XZR) { -tcg_out_rev16(s, TCG_TYPE_I32, TCG_REG_TMP, data_r); +tcg_out_rev16(s, TCG_REG_TMP, data_r); data_r = TCG_REG_TMP; } tcg_out_ldst_r(s, LDST_16, LDST_ST, data_r, addr_r, off_r); break; case MO_32: if (bswap && data_r != TCG_REG_XZR) { -tcg_out_rev(s, TCG_TYPE_I32, TCG_REG_TMP, data_r); +tcg_out_rev32(s, TCG_REG_TMP, data_r); data_r = TCG_REG_TMP; } tcg_out_ldst_r(s, LDST_32, LDST_ST, data_r, addr_r, off_r); break; case MO_64: if (bswap && data_r != TCG_REG_XZR) { -tcg_out_rev(s, TCG_TYPE_
Re: [Qemu-devel] [PATCH v1 1/3] qdev: Expose the qdev id string as a prop
Am 15.04.2014 23:39, schrieb Peter Crosthwaite: > On Wed, Apr 16, 2014 at 2:16 AM, Andreas Färber wrote: >> Am 15.04.2014 04:21, schrieb Peter Crosthwaite: >>> So clients can set the top level id string. >>> >>> Signed-off-by: Peter Crosthwaite >> >> Anthony had nack'ed Paolo's attempt to generalize the qdev id to QOM, so >> I'm not sure if we should really do this even if just on device level. >> The id= is used to construct the canonical path, and we can't change >> that through a qdev setter. > > How can we change it? The problem I am trying to solve, is getting > meaningful device instance names instead of the anonymous defaults. > This includes in the canonical path. I am completely open to proposals > :) Possibly this is just a mixup, see below. On yesterday's KVM call "instance names" referred to VMState names and instance_ids and qdev/qbus paths. Then there's the device IDs that this patch was touching. And there's the canonical QOM composition tree paths, that for user-created devices may include the device ID. > Using a dynamic property might allow us to >> unparent while keeping a reference and then add it as a child<> again, >> but that still feels awkward. > > Eww. > > Having it as a getter only seem more >> secure and predictable. >> > > Sure, once it's committed to the canonical path it definitely needs a > read-only semantic. > Shouldn't be hugely different to the > writable-until-realize semantic of qdev props though? It is to some degree. The canonical paths get set up as part of or right after instance_init. Only legacy devices without canonical path get a path as part of realize, but that needs to be fixed for recursive realization: Devices need to be accessible somewhere in the tree to be user-modified via qom-set, and they will need to be in the tree to be realized. >> Another issue is bus naming. If supplied NULL, the bus will be based on >> ID. If the ID changes, bus naming will be inconsistent. >> > > Or rather - what the user intends. If the board has 2 USB busses named > "foo" and "bar", then those should be both the canonical path and bus > names. Rather than the homogenised default names "usb0", "usb1". Let's not discuss this here again, it's being discussed between ppc and libvirt already. If the device supplies a hardcoded bus name then (while it may not be unique) we don't have to care in the context of IDs here. The interesting case is if we create a device with id=foo, and the bus uses NULL as indicated above, then it will get foo.0, foo.1, etc. unless I've missed something. IDs are unique, so if you have only one bus per device you get foo.0 and bar.0; if a device author chooses not to use NULL as bus name and rolls their own then we can't help them on the DeviceState level. >> Why would clients set the ID after having chosen a different ID > > I'm lost here - what is the mechanism by which you can validly set > per-instance IDs? Sorry, maybe I don't fully get your question... Either the user specifies ,id=foo on command line or HMP, or she doesn't. The corresponding mechanism in config files is [device "foo"], see docs/q35-chipset.cfg for an example. qdev paths used for VMState currently still depend on a bus, which Alex or Alexey wanted to look into fixing. By default -1 is supplied by DeviceState for registering dc->vmsd, which can be overridden by inlining the corresponding qdev.c code or more conveniently - Alex' suggestion yesterday - by moving that default value of -1 into an overrideable instance field. As for canonical QOM paths, it's the job of machines and devices to set up those - unless a peripheral device is added by the user, in which case see previous explanations by Markus and me. You might remember my big MPCore refactoring? That was supposed to be the building block for you to create a proper SoC container device for Zynq and thereby constructing meaningful canonical paths. I.e., a ZedBoard or MicroZed or Parallella board instantiates the SoC as /machine/zynq, the Zynq SoC makes available /machine/zynq/cortex or whatever (-> PMM; recursively ./scu etc.) plus memory-controller (this series), uart and whatever Zynq peripherals. You can't expect me to dictate paths for each model, that's no generic task; you need to take care of naming your favorite device models yourself so that *no* /machine/unassigned/device[n] remains. This may involve, such as in Anthony's case of i440fx or in case of function-driven Exynos/Zynq/... code, refactoring parts of devices into child<> devices for aggregation rather than just assigning names to existing devices on /machine. BTW that's also why - e.g. in the Xilinx context where you discussed inlining with Edgar - having qdev-style void create-foo wrappers hiding the object creation is undesirable since, however wrapped into helpers, we need to access the Object for adding as child<> property from its caller for uniqueness, usually. Hope that explains all device naming. Now, the unanswered question i
[Qemu-devel] [PULL for-2.1 12/25] tcg-aarch64: Introduce tcg_out_insn_3314
Combines 4 other inline functions and tidies the prologue. Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 100 --- 1 file changed, 33 insertions(+), 67 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index e36909e..5cffe50 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -284,6 +284,10 @@ typedef enum { I3207_BLR = 0xd63f, I3207_RET = 0xd65f, +/* Load/store register pair instructions. */ +I3314_LDP = 0x2840, +I3314_STP = 0x2800, + /* Add/subtract immediate instructions. */ I3401_ADDI = 0x1100, I3401_ADDSI = 0x3100, @@ -457,6 +461,20 @@ static void tcg_out_insn_3207(TCGContext *s, AArch64Insn insn, TCGReg rn) tcg_out32(s, insn | rn << 5); } +static void tcg_out_insn_3314(TCGContext *s, AArch64Insn insn, + TCGReg r1, TCGReg r2, TCGReg rn, + tcg_target_long ofs, bool pre, bool w) +{ +insn |= 1u << 31; /* ext */ +insn |= pre << 24; +insn |= w << 23; + +assert(ofs >= -0x200 && ofs < 0x200 && (ofs & 7) == 0); +insn |= (ofs & (0x7f << 3)) << (15 - 3); + +tcg_out32(s, insn | r2 << 10 | rn << 5 | r1); +} + static void tcg_out_insn_3401(TCGContext *s, AArch64Insn insn, TCGType ext, TCGReg rd, TCGReg rn, uint64_t aimm) { @@ -1292,56 +1310,6 @@ static void tcg_out_qemu_st(TCGContext *s, const TCGArg *args, int opc) static uint8_t *tb_ret_addr; -/* callee stack use example: - stp x29, x30, [sp,#-32]! - mov x29, sp - stp x1, x2, [sp,#16] - ... - ldp x1, x2, [sp,#16] - ldp x29, x30, [sp],#32 - ret -*/ - -/* push r1 and r2, and alloc stack space for a total of - alloc_n elements (1 element=16 bytes, must be between 1 and 31. */ -static inline void tcg_out_push_pair(TCGContext *s, TCGReg addr, - TCGReg r1, TCGReg r2, int alloc_n) -{ -/* using indexed scaled simm7 STP 0x2880 | (ext) | 0x0100 (pre-idx) - | alloc_n * (-1) << 16 | r2 << 10 | addr << 5 | r1 */ -assert(alloc_n > 0 && alloc_n < 0x20); -alloc_n = (-alloc_n) & 0x3f; -tcg_out32(s, 0xa980 | alloc_n << 16 | r2 << 10 | addr << 5 | r1); -} - -/* dealloc stack space for a total of alloc_n elements and pop r1, r2. */ -static inline void tcg_out_pop_pair(TCGContext *s, TCGReg addr, -TCGReg r1, TCGReg r2, int alloc_n) -{ -/* using indexed scaled simm7 LDP 0x28c0 | (ext) | nothing (post-idx) - | alloc_n << 16 | r2 << 10 | addr << 5 | r1 */ -assert(alloc_n > 0 && alloc_n < 0x20); -tcg_out32(s, 0xa8c0 | alloc_n << 16 | r2 << 10 | addr << 5 | r1); -} - -static inline void tcg_out_store_pair(TCGContext *s, TCGReg addr, - TCGReg r1, TCGReg r2, int idx) -{ -/* using register pair offset simm7 STP 0x2900 | (ext) - | idx << 16 | r2 << 10 | addr << 5 | r1 */ -assert(idx > 0 && idx < 0x20); -tcg_out32(s, 0xa900 | idx << 16 | r2 << 10 | addr << 5 | r1); -} - -static inline void tcg_out_load_pair(TCGContext *s, TCGReg addr, - TCGReg r1, TCGReg r2, int idx) -{ -/* using register pair offset simm7 LDP 0x2940 | (ext) - | idx << 16 | r2 << 10 | addr << 5 | r1 */ -assert(idx > 0 && idx < 0x20); -tcg_out32(s, 0xa940 | idx << 16 | r2 << 10 | addr << 5 | r1); -} - static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg args[TCG_MAX_OP_ARGS], const int const_args[TCG_MAX_OP_ARGS]) @@ -1887,33 +1855,32 @@ static void tcg_target_qemu_prologue(TCGContext *s) TCGReg r; /* save pairs (FP, LR) and (X19, X20) .. (X27, X28) */ -frame_size_callee_saved = (1) + (TCG_REG_X28 - TCG_REG_X19) / 2 + 1; +frame_size_callee_saved = 16 + (TCG_REG_X28 - TCG_REG_X19 + 1) * 8; /* frame size requirement for TCG local variables */ frame_size_tcg_locals = TCG_STATIC_CALL_ARGS_SIZE + CPU_TEMP_BUF_NLONGS * sizeof(long) + (TCG_TARGET_STACK_ALIGN - 1); frame_size_tcg_locals &= ~(TCG_TARGET_STACK_ALIGN - 1); -frame_size_tcg_locals /= TCG_TARGET_STACK_ALIGN; -/* push (FP, LR) and update sp */ -tcg_out_push_pair(s, TCG_REG_SP, - TCG_REG_FP, TCG_REG_LR, frame_size_callee_saved); +/* Push (FP, LR) and allocate space for all saved registers. */ +tcg_out_insn(s, 3314, STP, TCG_REG_FP, TCG_REG_LR, + TCG_REG_SP, -frame_size_callee_saved, 1, 1); /* Set up frame pointer for canonical unwinding. */ tcg_out_movr_sp(s, TCG_TYPE_I64, TCG_REG_FP, TCG_REG_SP); /* Store callee-preserved regs x19..x28. */ for (r = TCG_REG_X19; r <= TCG_REG_X27; r += 2) { -int idx = (r -
[Qemu-devel] [Bug 1308542] [NEW] hang in qemu_gluster_init
Public bug reported: In qemu_gluster_init, if the call to either glfs_set_volfile_server or glfs_set_logging fails into the "out" case, glfs_fini is called without having first calling glfs_init. This causes glfs_lock to spin forever on this bit: while (!fs->init) pthread_cond_wait (&fs->cond, &fs->mutex); And here's the bottom part of the backtrace when hung: #0 pthread_cond_wait@@GLIBC_2.3.2 () at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:183 #1 0x7feceebf58c3 in glfs_lock (fs=0x7fecf15660b0) at glfs-internal.h:156 #2 glfs_active_subvol (fs=0x7fecf15660b0) at glfs-resolve.c:799 #3 0x7feceebeb5b4 in glfs_fini (fs=0x7fecf15660b0) at glfs.c:652 #4 0x7fecf0043c73 in qemu_gluster_init (gconf=, filename=) at /usr/src/debug/qemu-kvm-0.12.1.2/block/gluster.c:229 I believe this can be fixed by simply moving the call to glfs_init after the call to glfs_new but before the calls to glfs_set_volfile_server or glfs_set_logging. ** Affects: qemu Importance: Undecided Status: New -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1308542 Title: hang in qemu_gluster_init Status in QEMU: New Bug description: In qemu_gluster_init, if the call to either glfs_set_volfile_server or glfs_set_logging fails into the "out" case, glfs_fini is called without having first calling glfs_init. This causes glfs_lock to spin forever on this bit: while (!fs->init) pthread_cond_wait (&fs->cond, &fs->mutex); And here's the bottom part of the backtrace when hung: #0 pthread_cond_wait@@GLIBC_2.3.2 () at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:183 #1 0x7feceebf58c3 in glfs_lock (fs=0x7fecf15660b0) at glfs-internal.h:156 #2 glfs_active_subvol (fs=0x7fecf15660b0) at glfs-resolve.c:799 #3 0x7feceebeb5b4 in glfs_fini (fs=0x7fecf15660b0) at glfs.c:652 #4 0x7fecf0043c73 in qemu_gluster_init (gconf=, filename=) at /usr/src/debug/qemu-kvm-0.12.1.2/block/gluster.c:229 I believe this can be fixed by simply moving the call to glfs_init after the call to glfs_new but before the calls to glfs_set_volfile_server or glfs_set_logging. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1308542/+subscriptions
[Qemu-devel] [PATCH] Unnecessary comma.
Signed-off-by: Igor Ryzhov --- net/net.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/net.c b/net/net.c index e3ef1e4..60a07f1 100644 --- a/net/net.c +++ b/net/net.c @@ -473,7 +473,7 @@ ssize_t qemu_deliver_packet(NetClientState *sender, if (ret == 0) { nc->receive_disabled = 1; -}; +} return ret; } -- 1.8.3.2
[Qemu-devel] [PULL for-2.1 06/25] tcg-aarch64: Special case small constants in tcg_out_movi
Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index c1d9895..a08f6c7 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -578,6 +578,16 @@ static void tcg_out_movi(TCGContext *s, TCGType type, TCGReg rd, type = TCG_TYPE_I32; } +/* Speed things up by handling the common case of small positive + and negative values specially. */ +if ((value & ~0xull) == 0) { +tcg_out_insn(s, 3405, MOVZ, type, rd, value, 0); +return; +} else if ((ivalue & ~0xull) == 0) { +tcg_out_insn(s, 3405, MOVN, type, rd, ivalue, 0); +return; +} + /* Check for bitfield immediates. For the benefit of 32-bit quantities, use the sign-extended value. That lets us match rotated values such as 0xffff with the same 64-bit logic matching 0xffff. */ -- 1.9.0
[Qemu-devel] [PATCH for-2.1 07/14] tcg-aarch64: Remove w constraint
Now redundant with the type parameter to tcg_target_const_match. Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 40 ++-- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index e028dd6..88f58ff 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -110,11 +110,10 @@ static inline void patch_reloc(uint8_t *code_ptr, int type, } } -#define TCG_CT_CONST_IS32 0x100 -#define TCG_CT_CONST_AIMM 0x200 -#define TCG_CT_CONST_LIMM 0x400 -#define TCG_CT_CONST_ZERO 0x800 -#define TCG_CT_CONST_MONE 0x1000 +#define TCG_CT_CONST_AIMM 0x100 +#define TCG_CT_CONST_LIMM 0x200 +#define TCG_CT_CONST_ZERO 0x400 +#define TCG_CT_CONST_MONE 0x800 /* parse target specific constraints */ static int target_parse_constraint(TCGArgConstraint *ct, @@ -139,9 +138,6 @@ static int target_parse_constraint(TCGArgConstraint *ct, tcg_regset_reset_reg(ct->u.regs, TCG_REG_X3); #endif break; -case 'w': /* The operand should be considered 32-bit. */ -ct->ct |= TCG_CT_CONST_IS32; -break; case 'A': /* Valid for arithmetic immediate (positive or negative). */ ct->ct |= TCG_CT_CONST_AIMM; break; @@ -196,7 +192,7 @@ static int tcg_target_const_match(tcg_target_long val, TCGType type, if (ct & TCG_CT_CONST) { return 1; } -if (ct & TCG_CT_CONST_IS32) { +if (type == TCG_TYPE_I32) { val = (int32_t)val; } if ((ct & TCG_CT_CONST_AIMM) && (is_aimm(val) || is_aimm(-val))) { @@ -1650,9 +1646,9 @@ static const TCGTargetOpDef aarch64_op_defs[] = { { INDEX_op_st32_i64, { "r", "r" } }, { INDEX_op_st_i64, { "r", "r" } }, -{ INDEX_op_add_i32, { "r", "r", "rwA" } }, +{ INDEX_op_add_i32, { "r", "r", "rA" } }, { INDEX_op_add_i64, { "r", "r", "rA" } }, -{ INDEX_op_sub_i32, { "r", "r", "rwA" } }, +{ INDEX_op_sub_i32, { "r", "r", "rA" } }, { INDEX_op_sub_i64, { "r", "r", "rA" } }, { INDEX_op_mul_i32, { "r", "r", "r" } }, { INDEX_op_mul_i64, { "r", "r", "r" } }, @@ -1664,17 +1660,17 @@ static const TCGTargetOpDef aarch64_op_defs[] = { { INDEX_op_rem_i64, { "r", "r", "r" } }, { INDEX_op_remu_i32, { "r", "r", "r" } }, { INDEX_op_remu_i64, { "r", "r", "r" } }, -{ INDEX_op_and_i32, { "r", "r", "rwL" } }, +{ INDEX_op_and_i32, { "r", "r", "rL" } }, { INDEX_op_and_i64, { "r", "r", "rL" } }, -{ INDEX_op_or_i32, { "r", "r", "rwL" } }, +{ INDEX_op_or_i32, { "r", "r", "rL" } }, { INDEX_op_or_i64, { "r", "r", "rL" } }, -{ INDEX_op_xor_i32, { "r", "r", "rwL" } }, +{ INDEX_op_xor_i32, { "r", "r", "rL" } }, { INDEX_op_xor_i64, { "r", "r", "rL" } }, -{ INDEX_op_andc_i32, { "r", "r", "rwL" } }, +{ INDEX_op_andc_i32, { "r", "r", "rL" } }, { INDEX_op_andc_i64, { "r", "r", "rL" } }, -{ INDEX_op_orc_i32, { "r", "r", "rwL" } }, +{ INDEX_op_orc_i32, { "r", "r", "rL" } }, { INDEX_op_orc_i64, { "r", "r", "rL" } }, -{ INDEX_op_eqv_i32, { "r", "r", "rwL" } }, +{ INDEX_op_eqv_i32, { "r", "r", "rL" } }, { INDEX_op_eqv_i64, { "r", "r", "rL" } }, { INDEX_op_neg_i32, { "r", "r" } }, @@ -1693,11 +1689,11 @@ static const TCGTargetOpDef aarch64_op_defs[] = { { INDEX_op_rotl_i64, { "r", "r", "ri" } }, { INDEX_op_rotr_i64, { "r", "r", "ri" } }, -{ INDEX_op_brcond_i32, { "r", "rwA" } }, +{ INDEX_op_brcond_i32, { "r", "rA" } }, { INDEX_op_brcond_i64, { "r", "rA" } }, -{ INDEX_op_setcond_i32, { "r", "r", "rwA" } }, +{ INDEX_op_setcond_i32, { "r", "r", "rA" } }, { INDEX_op_setcond_i64, { "r", "r", "rA" } }, -{ INDEX_op_movcond_i32, { "r", "r", "rwA", "rZ", "rZ" } }, +{ INDEX_op_movcond_i32, { "r", "r", "rA", "rZ", "rZ" } }, { INDEX_op_movcond_i64, { "r", "r", "rA", "rZ", "rZ" } }, { INDEX_op_qemu_ld8u, { "r", "l" } }, @@ -1736,9 +1732,9 @@ static const TCGTargetOpDef aarch64_op_defs[] = { { INDEX_op_deposit_i32, { "r", "0", "rZ" } }, { INDEX_op_deposit_i64, { "r", "0", "rZ" } }, -{ INDEX_op_add2_i32, { "r", "r", "rZ", "rZ", "rwA", "rwMZ" } }, +{ INDEX_op_add2_i32, { "r", "r", "rZ", "rZ", "rA", "rMZ" } }, { INDEX_op_add2_i64, { "r", "r", "rZ", "rZ", "rA", "rMZ" } }, -{ INDEX_op_sub2_i32, { "r", "r", "rZ", "rZ", "rwA", "rwMZ" } }, +{ INDEX_op_sub2_i32, { "r", "r", "rZ", "rZ", "rA", "rMZ" } }, { INDEX_op_sub2_i64, { "r", "r", "rZ", "rZ", "rA", "rMZ" } }, { INDEX_op_muluh_i64, { "r", "r", "r" } }, -- 1.9.0
[Qemu-devel] [PATCH for-2.1 14/14] tcg: Use HOST_WORDS_BIGENDIAN
Instead of rolling a local TCG_TARGET_WORDS_BIGENDIAN. Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.h | 1 - tcg/arm/tcg-target.h | 1 - tcg/i386/tcg-target.h| 2 -- tcg/mips/tcg-target.c| 12 ++-- tcg/mips/tcg-target.h| 4 tcg/ppc/tcg-target.h | 1 - tcg/ppc64/tcg-target.h | 1 - tcg/s390/tcg-target.h| 2 -- tcg/sparc/tcg-target.h | 2 -- tcg/tcg-op.h | 4 ++-- tcg/tcg.c| 8 tcg/tci/tcg-target.h | 6 -- 12 files changed, 12 insertions(+), 32 deletions(-) diff --git a/tcg/aarch64/tcg-target.h b/tcg/aarch64/tcg-target.h index 988983e..646a6a5 100644 --- a/tcg/aarch64/tcg-target.h +++ b/tcg/aarch64/tcg-target.h @@ -13,7 +13,6 @@ #ifndef TCG_TARGET_AARCH64 #define TCG_TARGET_AARCH64 1 -#undef TCG_TARGET_WORDS_BIGENDIAN #undef TCG_TARGET_STACK_GROWSUP typedef enum { diff --git a/tcg/arm/tcg-target.h b/tcg/arm/tcg-target.h index 35de34e..1bc5dac 100644 --- a/tcg/arm/tcg-target.h +++ b/tcg/arm/tcg-target.h @@ -25,7 +25,6 @@ #ifndef TCG_TARGET_ARM #define TCG_TARGET_ARM 1 -#undef TCG_TARGET_WORDS_BIGENDIAN #undef TCG_TARGET_STACK_GROWSUP typedef enum { diff --git a/tcg/i386/tcg-target.h b/tcg/i386/tcg-target.h index bdf..ababca0 100644 --- a/tcg/i386/tcg-target.h +++ b/tcg/i386/tcg-target.h @@ -24,8 +24,6 @@ #ifndef TCG_TARGET_I386 #define TCG_TARGET_I386 1 -#undef TCG_TARGET_WORDS_BIGENDIAN - #ifdef __x86_64__ # define TCG_TARGET_REG_BITS 64 # define TCG_TARGET_NB_REGS 16 diff --git a/tcg/mips/tcg-target.c b/tcg/mips/tcg-target.c index 76bc222..37241b2 100644 --- a/tcg/mips/tcg-target.c +++ b/tcg/mips/tcg-target.c @@ -26,7 +26,7 @@ #include "tcg-be-null.h" -#if defined(TCG_TARGET_WORDS_BIGENDIAN) == defined(TARGET_WORDS_BIGENDIAN) +#if defined(HOST_WORDS_BIGENDIAN) == defined(TARGET_WORDS_BIGENDIAN) # define TCG_NEED_BSWAP 0 #else # define TCG_NEED_BSWAP 1 @@ -589,7 +589,7 @@ static inline void tcg_out_call_iarg_reg64(TCGContext *s, int *arg_num, { (*arg_num) = (*arg_num + 1) & ~1; -#if defined(TCG_TARGET_WORDS_BIGENDIAN) +#if defined(HOST_WORDS_BIGENDIAN) tcg_out_call_iarg_reg32(s, arg_num, arg_high); tcg_out_call_iarg_reg32(s, arg_num, arg_low); #else @@ -964,7 +964,7 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, #if defined(CONFIG_SOFTMMU) # if TARGET_LONG_BITS == 64 addr_regh = *args++; -# if defined(TCG_TARGET_WORDS_BIGENDIAN) +# if defined(HOST_WORDS_BIGENDIAN) addr_memh = 0; addr_meml = 4; # else @@ -979,7 +979,7 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, #endif if (opc == 3) { -#if defined(TCG_TARGET_WORDS_BIGENDIAN) +#if defined(HOST_WORDS_BIGENDIAN) data_reg1 = data_regh; data_reg2 = data_regl; #else @@ -1152,7 +1152,7 @@ static void tcg_out_qemu_st(TCGContext *s, const TCGArg *args, #if defined(CONFIG_SOFTMMU) # if TARGET_LONG_BITS == 64 addr_regh = *args++; -# if defined(TCG_TARGET_WORDS_BIGENDIAN) +# if defined(HOST_WORDS_BIGENDIAN) addr_memh = 0; addr_meml = 4; # else @@ -1167,7 +1167,7 @@ static void tcg_out_qemu_st(TCGContext *s, const TCGArg *args, #endif if (opc == 3) { -#if defined(TCG_TARGET_WORDS_BIGENDIAN) +#if defined(HOST_WORDS_BIGENDIAN) data_reg1 = data_regh; data_reg2 = data_regl; #else diff --git a/tcg/mips/tcg-target.h b/tcg/mips/tcg-target.h index c3822d0..9576db5 100644 --- a/tcg/mips/tcg-target.h +++ b/tcg/mips/tcg-target.h @@ -26,10 +26,6 @@ #ifndef TCG_TARGET_MIPS #define TCG_TARGET_MIPS 1 -#ifdef __MIPSEB__ -# define TCG_TARGET_WORDS_BIGENDIAN -#endif - #define TCG_TARGET_NB_REGS 32 typedef enum { diff --git a/tcg/ppc/tcg-target.h b/tcg/ppc/tcg-target.h index 1168912..0d4f595 100644 --- a/tcg/ppc/tcg-target.h +++ b/tcg/ppc/tcg-target.h @@ -24,7 +24,6 @@ #ifndef TCG_TARGET_PPC #define TCG_TARGET_PPC 1 -#define TCG_TARGET_WORDS_BIGENDIAN #define TCG_TARGET_NB_REGS 32 typedef enum { diff --git a/tcg/ppc64/tcg-target.h b/tcg/ppc64/tcg-target.h index 7ee50b6..78bbf7a 100644 --- a/tcg/ppc64/tcg-target.h +++ b/tcg/ppc64/tcg-target.h @@ -24,7 +24,6 @@ #ifndef TCG_TARGET_PPC64 #define TCG_TARGET_PPC64 1 -#define TCG_TARGET_WORDS_BIGENDIAN #define TCG_TARGET_NB_REGS 32 typedef enum { diff --git a/tcg/s390/tcg-target.h b/tcg/s390/tcg-target.h index 10adb77..b3bfdcc 100644 --- a/tcg/s390/tcg-target.h +++ b/tcg/s390/tcg-target.h @@ -24,8 +24,6 @@ #ifndef TCG_TARGET_S390 #define TCG_TARGET_S390 1 -#define TCG_TARGET_WORDS_BIGENDIAN - typedef enum TCGReg { TCG_REG_R0 = 0, TCG_REG_R1, diff --git a/tcg/sparc/tcg-target.h b/tcg/sparc/tcg-target.h index 3abf1b4..4519c64 100644 --- a/tcg/sparc/tcg-target.h +++ b/tcg/sparc/tcg-target.h @@ -32,8 +32,6 @@ # error Unknown pointer size for tcg target #endif -#define TCG_TARGET_WORDS_BIGENDIAN - #define TCG_TARGET_NB_REGS 32 typedef enum { diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h index d43f
[Qemu-devel] [PATCH for-2.1 10/14] tcg-s390: Remove W constraint
Now redundant with the type parameter to tcg_target_const_match. Signed-off-by: Richard Henderson --- tcg/s390/tcg-target.c | 43 +++ 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/tcg/s390/tcg-target.c b/tcg/s390/tcg-target.c index feeaf97..1d912a7 100644 --- a/tcg/s390/tcg-target.c +++ b/tcg/s390/tcg-target.c @@ -38,11 +38,10 @@ a 32-bit displacement here Just In Case. */ #define USE_LONG_BRANCHES 0 -#define TCG_CT_CONST_320x0100 -#define TCG_CT_CONST_MULI 0x0800 -#define TCG_CT_CONST_ORI 0x2000 -#define TCG_CT_CONST_XORI 0x4000 -#define TCG_CT_CONST_CMPI 0x8000 +#define TCG_CT_CONST_MULI 0x100 +#define TCG_CT_CONST_ORI 0x200 +#define TCG_CT_CONST_XORI 0x400 +#define TCG_CT_CONST_CMPI 0x800 /* Several places within the instruction set 0 means "no register" rather than TCG_REG_R0. */ @@ -407,9 +406,6 @@ static int target_parse_constraint(TCGArgConstraint *ct, const char **pct_str) tcg_regset_clear(ct->u.regs); tcg_regset_set_reg(ct->u.regs, TCG_REG_R3); break; -case 'W': /* force 32-bit ("word") immediate */ -ct->ct |= TCG_CT_CONST_32; -break; case 'K': ct->ct |= TCG_CT_CONST_MULI; break; @@ -437,10 +433,10 @@ static int target_parse_constraint(TCGArgConstraint *ct, const char **pct_str) can load efficiently, and the immediate load plus the reg-reg OR is smaller than the sequential OI's. */ -static int tcg_match_ori(int ct, tcg_target_long val) +static int tcg_match_ori(TCGType type, tcg_target_long val) { if (facilities & FACILITY_EXT_IMM) { -if (ct & TCG_CT_CONST_32) { +if (type == TCG_TYPE_I32) { /* All 32-bit ORs can be performed with 1 48-bit insn. */ return 1; } @@ -466,13 +462,13 @@ static int tcg_match_ori(int ct, tcg_target_long val) extended-immediate facility. That said, there are a few patterns for which it is better to load the value into a register first. */ -static int tcg_match_xori(int ct, tcg_target_long val) +static int tcg_match_xori(TCGType type, tcg_target_long val) { if ((facilities & FACILITY_EXT_IMM) == 0) { return 0; } -if (ct & TCG_CT_CONST_32) { +if (type == TCG_TYPE_I32) { /* All 32-bit XORs can be performed with 1 48-bit insn. */ return 1; } @@ -487,11 +483,11 @@ static int tcg_match_xori(int ct, tcg_target_long val) /* Imediates to be used with comparisons. */ -static int tcg_match_cmpi(int ct, tcg_target_long val) +static int tcg_match_cmpi(TCGType type, tcg_target_long val) { if (facilities & FACILITY_EXT_IMM) { /* The COMPARE IMMEDIATE instruction is available. */ -if (ct & TCG_CT_CONST_32) { +if (type == TCG_TYPE_I32) { /* We have a 32-bit immediate and can compare against anything. */ return 1; } else { @@ -524,8 +520,7 @@ static int tcg_target_const_match(tcg_target_long val, TCGType type, return 1; } -/* Handle the modifiers. */ -if (ct & TCG_CT_CONST_32) { +if (type == TCG_TYPE_I32) { val = (int32_t)val; } @@ -541,11 +536,11 @@ static int tcg_target_const_match(tcg_target_long val, TCGType type, return val == (int16_t)val; } } else if (ct & TCG_CT_CONST_ORI) { -return tcg_match_ori(ct, val); +return tcg_match_ori(type, val); } else if (ct & TCG_CT_CONST_XORI) { -return tcg_match_xori(ct, val); +return tcg_match_xori(type, val); } else if (ct & TCG_CT_CONST_CMPI) { -return tcg_match_cmpi(ct, val); +return tcg_match_cmpi(type, val); } return 0; @@ -2112,8 +2107,8 @@ static const TCGTargetOpDef s390_op_defs[] = { { INDEX_op_divu2_i32, { "b", "a", "0", "1", "r" } }, { INDEX_op_and_i32, { "r", "0", "ri" } }, -{ INDEX_op_or_i32, { "r", "0", "rWO" } }, -{ INDEX_op_xor_i32, { "r", "0", "rWX" } }, +{ INDEX_op_or_i32, { "r", "0", "rO" } }, +{ INDEX_op_xor_i32, { "r", "0", "rX" } }, { INDEX_op_neg_i32, { "r", "r" } }, @@ -2135,9 +2130,9 @@ static const TCGTargetOpDef s390_op_defs[] = { { INDEX_op_add2_i32, { "r", "r", "0", "1", "r", "r" } }, { INDEX_op_sub2_i32, { "r", "r", "0", "1", "r", "r" } }, -{ INDEX_op_brcond_i32, { "r", "rWC" } }, -{ INDEX_op_setcond_i32, { "r", "r", "rWC" } }, -{ INDEX_op_movcond_i32, { "r", "r", "rWC", "r", "0" } }, +{ INDEX_op_brcond_i32, { "r", "rC" } }, +{ INDEX_op_setcond_i32, { "r", "r", "rC" } }, +{ INDEX_op_movcond_i32, { "r", "r", "rC", "r", "0" } }, { INDEX_op_deposit_i32, { "r", "0", "r" } }, { INDEX_op_qemu_ld8u, { "r", "L" } }, -- 1.9.0
[Qemu-devel] [PATCH for-2.1 08/14] tcg-ppc64: Use the type parameter to tcg_target_const_match
Signed-off-by: Richard Henderson --- tcg/ppc64/tcg-target.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tcg/ppc64/tcg-target.c b/tcg/ppc64/tcg-target.c index a533698..45b1c06 100644 --- a/tcg/ppc64/tcg-target.c +++ b/tcg/ppc64/tcg-target.c @@ -296,7 +296,15 @@ static int tcg_target_const_match(tcg_target_long val, TCGType type, int ct = arg_ct->ct; if (ct & TCG_CT_CONST) { return 1; -} else if ((ct & TCG_CT_CONST_S16) && val == (int16_t)val) { +} + +/* The only 32-bit constraint we use aside from + TCG_CT_CONST is TCG_CT_CONST_S16. */ +if (type == TCG_TYPE_I32) { +val = (int32_t)val; +} + +if ((ct & TCG_CT_CONST_S16) && val == (int16_t)val) { return 1; } else if ((ct & TCG_CT_CONST_U16) && val == (uint16_t)val) { return 1; -- 1.9.0
[Qemu-devel] [PULL for-2.1 03/25] tcg-aarch64: Use TCGType and TCGMemOp constants
Rather than raw constants that could mean anything. Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 73 +--- 1 file changed, 38 insertions(+), 35 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index 6938248..5e6d10b 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -595,7 +595,7 @@ static inline void tcg_out_mov(TCGContext *s, TCGType type, TCGReg ret, TCGReg arg) { if (ret != arg) { -tcg_out_movr(s, type == TCG_TYPE_I64, ret, arg); +tcg_out_movr(s, type, ret, arg); } } @@ -828,19 +828,19 @@ static inline void tcg_out_rev16(TCGContext *s, TCGType ext, tcg_out32(s, base | rm << 5 | rd); } -static inline void tcg_out_sxt(TCGContext *s, TCGType ext, int s_bits, +static inline void tcg_out_sxt(TCGContext *s, TCGType ext, TCGMemOp s_bits, TCGReg rd, TCGReg rn) { /* Using ALIASes SXTB, SXTH, SXTW, of SBFM Xd, Xn, #0, #7|15|31 */ -int bits = 8 * (1 << s_bits) - 1; +int bits = (8 << s_bits) - 1; tcg_out_sbfm(s, ext, rd, rn, 0, bits); } -static inline void tcg_out_uxt(TCGContext *s, int s_bits, +static inline void tcg_out_uxt(TCGContext *s, TCGMemOp s_bits, TCGReg rd, TCGReg rn) { /* Using ALIASes UXTB, UXTH of UBFM Wd, Wn, #0, #7|15 */ -int bits = 8 * (1 << s_bits) - 1; +int bits = (8 << s_bits) - 1; tcg_out_ubfm(s, 0, rd, rn, 0, bits); } @@ -949,19 +949,21 @@ static const void * const qemu_st_helpers[4] = { static void tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) { +TCGMemOp opc = lb->opc; +TCGMemOp size = opc & MO_SIZE; + reloc_pc19(lb->label_ptr[0], (intptr_t)s->code_ptr); -tcg_out_movr(s, 1, TCG_REG_X0, TCG_AREG0); -tcg_out_movr(s, (TARGET_LONG_BITS == 64), TCG_REG_X1, lb->addrlo_reg); +tcg_out_movr(s, TCG_TYPE_I64, TCG_REG_X0, TCG_AREG0); +tcg_out_movr(s, TARGET_LONG_BITS == 64, TCG_REG_X1, lb->addrlo_reg); tcg_out_movi(s, TCG_TYPE_I32, TCG_REG_X2, lb->mem_index); tcg_out_movi(s, TCG_TYPE_PTR, TCG_REG_X3, (intptr_t)lb->raddr); -tcg_out_movi(s, TCG_TYPE_I64, TCG_REG_TMP, - (intptr_t)qemu_ld_helpers[lb->opc & 3]); +tcg_out_movi(s, TCG_TYPE_I64, TCG_REG_TMP, (intptr_t)qemu_ld_helpers[size]); tcg_out_callr(s, TCG_REG_TMP); -if (lb->opc & 0x04) { -tcg_out_sxt(s, 1, lb->opc & 3, lb->datalo_reg, TCG_REG_X0); +if (opc & MO_SIGN) { +tcg_out_sxt(s, TCG_TYPE_I64, size, lb->datalo_reg, TCG_REG_X0); } else { -tcg_out_movr(s, 1, lb->datalo_reg, TCG_REG_X0); +tcg_out_movr(s, TCG_TYPE_I64, lb->datalo_reg, TCG_REG_X0); } tcg_out_goto(s, (intptr_t)lb->raddr); @@ -969,15 +971,16 @@ static void tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) static void tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) { +TCGMemOp size = lb->opc; + reloc_pc19(lb->label_ptr[0], (intptr_t)s->code_ptr); -tcg_out_movr(s, 1, TCG_REG_X0, TCG_AREG0); -tcg_out_movr(s, (TARGET_LONG_BITS == 64), TCG_REG_X1, lb->addrlo_reg); -tcg_out_movr(s, 1, TCG_REG_X2, lb->datalo_reg); +tcg_out_movr(s, TCG_TYPE_I64, TCG_REG_X0, TCG_AREG0); +tcg_out_movr(s, TARGET_LONG_BITS == 64, TCG_REG_X1, lb->addrlo_reg); +tcg_out_movr(s, size == MO_64, TCG_REG_X2, lb->datalo_reg); tcg_out_movi(s, TCG_TYPE_I32, TCG_REG_X3, lb->mem_index); tcg_out_movi(s, TCG_TYPE_PTR, TCG_REG_X4, (intptr_t)lb->raddr); -tcg_out_movi(s, TCG_TYPE_I64, TCG_REG_TMP, - (intptr_t)qemu_st_helpers[lb->opc & 3]); +tcg_out_movi(s, TCG_TYPE_I64, TCG_REG_TMP, (intptr_t)qemu_st_helpers[size]); tcg_out_callr(s, TCG_REG_TMP); tcg_out_goto(s, (intptr_t)lb->raddr); } @@ -1061,14 +1064,14 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, int opc, TCGReg data_r, case 1: tcg_out_ldst_r(s, LDST_16, LDST_LD, data_r, addr_r, off_r); if (TCG_LDST_BSWAP) { -tcg_out_rev16(s, 0, data_r, data_r); +tcg_out_rev16(s, TCG_TYPE_I32, data_r, data_r); } break; case 1 | 4: if (TCG_LDST_BSWAP) { tcg_out_ldst_r(s, LDST_16, LDST_LD, data_r, addr_r, off_r); -tcg_out_rev16(s, 0, data_r, data_r); -tcg_out_sxt(s, 1, 1, data_r, data_r); +tcg_out_rev16(s, TCG_TYPE_I32, data_r, data_r); +tcg_out_sxt(s, TCG_TYPE_I64, MO_16, data_r, data_r); } else { tcg_out_ldst_r(s, LDST_16, LDST_LD_S_X, data_r, addr_r, off_r); } @@ -1076,14 +1079,14 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, int opc, TCGReg data_r, case 2: tcg_out_ldst_r(s, LDST_32, LDST_LD, data_r, addr_r, off_r); if (TCG_LDST_BSWAP) { -tcg_out_rev(s, 0, data_r, data_r); +tcg_out_rev(s,
[Qemu-devel] [PATCH for-2.1 09/14] tcg-sparc: Use the type parameter to tcg_target_const_match
Signed-off-by: Richard Henderson --- tcg/sparc/tcg-target.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tcg/sparc/tcg-target.c b/tcg/sparc/tcg-target.c index 73121e1..35089b8 100644 --- a/tcg/sparc/tcg-target.c +++ b/tcg/sparc/tcg-target.c @@ -334,7 +334,13 @@ static inline int tcg_target_const_match(tcg_target_long val, TCGType type, if (ct & TCG_CT_CONST) { return 1; -} else if ((ct & TCG_CT_CONST_ZERO) && val == 0) { +} + +if (type == TCG_TYPE_I32) { +val = (int32_t)val; +} + +if ((ct & TCG_CT_CONST_ZERO) && val == 0) { return 1; } else if ((ct & TCG_CT_CONST_S11) && check_fit_tl(val, 11)) { return 1; -- 1.9.0
[Qemu-devel] [PATCH for-2.1 00/14] tcg: collection of patches
A few of these have been reviewed before, but didn't make the 2.0 cut. The TCGType parameter to tcg_target_const_match patches arose out of the review of one of my outstanding sparc backend patch sets. The mulu2_i32 vs muluh_i32 patches arose out of some cleanup work I am doing in the powerpc backends. In particular, it will streamline the ppc32 backend, letting the register allocator handle the case of overlapping inputs and outputs to mulu2_i32. The HOST_WORDS_BIGENDIAN patch is my answer to handing endianness for ppc64le. r~ Richard Henderson (13): tcg: Use "unspecified behavior" for shifts tcg: Mask shift quantities while folding tci: Mask shift counts to avoid undefined behavior tcg: Fix out of range shift in deposit optimizations tcg: Add TCGType parameter to tcg_target_const_match tcg-aarch64: Remove w constraint tcg-ppc64: Use the type parameter to tcg_target_const_match tcg-sparc: Use the type parameter to tcg_target_const_match tcg-s390: Remove W constraint tcg: Relax requirement for mulu2_i32 on 32-bit hosts tcg: Use tcg_gen_mulu2_i32 in tcg_gen_muls2_i32 tcg: Fix fallback from muls2_i64 to mulu2_i64 tcg: Use HOST_WORDS_BIGENDIAN Stefan Weil (1): tcg: Fix warning (1 bit signed bitfield entry) and replace int by bool tcg/README | 18 +- tcg/aarch64/tcg-target.c | 48 ++-- tcg/aarch64/tcg-target.h | 1 - tcg/arm/tcg-target.c | 8 tcg/arm/tcg-target.h | 2 +- tcg/i386/tcg-target.c| 8 tcg/i386/tcg-target.h| 2 -- tcg/ia64/tcg-target.c| 2 +- tcg/mips/tcg-target.c| 14 +++--- tcg/mips/tcg-target.h| 5 + tcg/optimize.c | 45 - tcg/ppc/tcg-target.c | 8 tcg/ppc/tcg-target.h | 2 +- tcg/ppc64/tcg-target.c | 12 ++-- tcg/ppc64/tcg-target.h | 1 - tcg/s390/tcg-target.c| 45 - tcg/s390/tcg-target.h| 2 -- tcg/sparc/tcg-target.c | 10 -- tcg/sparc/tcg-target.h | 2 -- tcg/tcg-be-ldst.h| 2 +- tcg/tcg-op.h | 48 ++-- tcg/tcg.c| 14 +++--- tcg/tcg.h| 8 +++- tcg/tci/tcg-target.c | 2 +- tcg/tci/tcg-target.h | 8 ++-- tci.c| 20 ++-- 26 files changed, 170 insertions(+), 167 deletions(-) -- 1.9.0
[Qemu-devel] [PATCH for-2.1 13/14] tcg: Fix fallback from muls2_i64 to mulu2_i64
Brown Bag sez, don't put the fallback code into the wrong function. Also, check for muluh_i64 and use tcg_gen_mulu2_i64 instead of raw ops. Signed-off-by: Richard Henderson --- tcg/tcg-op.h | 38 ++ 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h index 08dd09e..d43f45d 100644 --- a/tcg/tcg-op.h +++ b/tcg/tcg-op.h @@ -2520,26 +2520,6 @@ static inline void tcg_gen_mulu2_i64(TCGv_i64 rl, TCGv_i64 rh, tcg_gen_op3_i64(INDEX_op_muluh_i64, rh, arg1, arg2); tcg_gen_mov_i64(rl, t); tcg_temp_free_i64(t); -} else if (TCG_TARGET_HAS_mulu2_i64) { -TCGv_i64 t0 = tcg_temp_new_i64(); -TCGv_i64 t1 = tcg_temp_new_i64(); -TCGv_i64 t2 = tcg_temp_new_i64(); -TCGv_i64 t3 = tcg_temp_new_i64(); -tcg_gen_op4_i64(INDEX_op_mulu2_i64, t0, t1, arg1, arg2); -/* Allow the optimizer room to replace mulu2 with two moves. */ -tcg_gen_op0(INDEX_op_nop); -/* Adjust for negative inputs. */ -tcg_gen_sari_i64(t2, arg1, 63); -tcg_gen_sari_i64(t3, arg2, 63); -tcg_gen_and_i64(t2, t2, arg2); -tcg_gen_and_i64(t3, t3, arg1); -tcg_gen_sub_i64(rh, t1, t2); -tcg_gen_sub_i64(rh, rh, t3); -tcg_gen_mov_i64(rl, t0); -tcg_temp_free_i64(t0); -tcg_temp_free_i64(t1); -tcg_temp_free_i64(t2); -tcg_temp_free_i64(t3); } else { TCGv_i64 t0 = tcg_temp_new_i64(); int sizemask = 0; @@ -2567,6 +2547,24 @@ static inline void tcg_gen_muls2_i64(TCGv_i64 rl, TCGv_i64 rh, tcg_gen_op3_i64(INDEX_op_mulsh_i64, rh, arg1, arg2); tcg_gen_mov_i64(rl, t); tcg_temp_free_i64(t); +} else if (TCG_TARGET_HAS_mulu2_i64 || TCG_TARGET_HAS_muluh_i64) { +TCGv_i64 t0 = tcg_temp_new_i64(); +TCGv_i64 t1 = tcg_temp_new_i64(); +TCGv_i64 t2 = tcg_temp_new_i64(); +TCGv_i64 t3 = tcg_temp_new_i64(); +tcg_gen_mulu2_i64(t0, t1, arg1, arg2); +/* Adjust for negative inputs. */ +tcg_gen_sari_i64(t2, arg1, 63); +tcg_gen_sari_i64(t3, arg2, 63); +tcg_gen_and_i64(t2, t2, arg2); +tcg_gen_and_i64(t3, t3, arg1); +tcg_gen_sub_i64(rh, t1, t2); +tcg_gen_sub_i64(rh, rh, t3); +tcg_gen_mov_i64(rl, t0); +tcg_temp_free_i64(t0); +tcg_temp_free_i64(t1); +tcg_temp_free_i64(t2); +tcg_temp_free_i64(t3); } else { TCGv_i64 t0 = tcg_temp_new_i64(); int sizemask = 0; -- 1.9.0
[Qemu-devel] [PATCH for-2.1 12/14] tcg: Use tcg_gen_mulu2_i32 in tcg_gen_muls2_i32
Rather than hard-coding use of mulu2_i32, allow muluh_i32. Signed-off-by: Richard Henderson --- tcg/tcg-op.h | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h index 7eabf22..08dd09e 100644 --- a/tcg/tcg-op.h +++ b/tcg/tcg-op.h @@ -2437,14 +2437,12 @@ static inline void tcg_gen_muls2_i32(TCGv_i32 rl, TCGv_i32 rh, tcg_gen_op3_i32(INDEX_op_mulsh_i32, rh, arg1, arg2); tcg_gen_mov_i32(rl, t); tcg_temp_free_i32(t); -} else if (TCG_TARGET_REG_BITS == 32 && TCG_TARGET_HAS_mulu2_i32) { +} else if (TCG_TARGET_REG_BITS == 32) { TCGv_i32 t0 = tcg_temp_new_i32(); TCGv_i32 t1 = tcg_temp_new_i32(); TCGv_i32 t2 = tcg_temp_new_i32(); TCGv_i32 t3 = tcg_temp_new_i32(); -tcg_gen_op4_i32(INDEX_op_mulu2_i32, t0, t1, arg1, arg2); -/* Allow the optimizer room to replace mulu2 with two moves. */ -tcg_gen_op0(INDEX_op_nop); +tcg_gen_mulu2_i32(t0, t1, arg1, arg2); /* Adjust for negative inputs. */ tcg_gen_sari_i32(t2, arg1, 31); tcg_gen_sari_i32(t3, arg2, 31); -- 1.9.0
[Qemu-devel] [PATCH for-2.1 05/14] tcg: Fix out of range shift in deposit optimizations
By inspection, for a deposit(x, y, 0, 64), we'd have a shift of (1<<64) and everything else falls apart. But we can reuse the existing deposit logic to get this right. Signed-off-by: Richard Henderson --- tcg/optimize.c | 10 -- 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tcg/optimize.c b/tcg/optimize.c index 2fb708e..c447062 100644 --- a/tcg/optimize.c +++ b/tcg/optimize.c @@ -843,9 +843,8 @@ static TCGArg *tcg_constant_folding(TCGContext *s, uint16_t *tcg_opc_ptr, break; CASE_OP_32_64(deposit): -tmp = ((1ull << args[4]) - 1); -mask = ((temps[args[1]].mask & ~(tmp << args[3])) -| ((temps[args[2]].mask & tmp) << args[3])); +mask = deposit64(temps[args[1]].mask, args[3], args[4], + temps[args[2]].mask); break; CASE_OP_32_64(or): @@ -1060,9 +1059,8 @@ static TCGArg *tcg_constant_folding(TCGContext *s, uint16_t *tcg_opc_ptr, if (temps[args[1]].state == TCG_TEMP_CONST && temps[args[2]].state == TCG_TEMP_CONST) { s->gen_opc_buf[op_index] = op_to_movi(op); -tmp = ((1ull << args[4]) - 1); -tmp = (temps[args[1]].val & ~(tmp << args[3])) - | ((temps[args[2]].val & tmp) << args[3]); +tmp = deposit64(temps[args[1]].val, args[3], args[4], +temps[args[2]].val); tcg_opt_gen_movi(gen_args, args[0], tmp); gen_args += 2; args += 5; -- 1.9.0
[Qemu-devel] [PATCH for-2.1 06/14] tcg: Add TCGType parameter to tcg_target_const_match
Most 64-bit targets need to be able to ignore the high bits of a TCG_TYPE_I32 value. Suggested-by: Stuart Brady Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 2 +- tcg/arm/tcg-target.c | 2 +- tcg/i386/tcg-target.c| 2 +- tcg/ia64/tcg-target.c| 2 +- tcg/mips/tcg-target.c| 2 +- tcg/ppc/tcg-target.c | 2 +- tcg/ppc64/tcg-target.c | 2 +- tcg/s390/tcg-target.c| 2 +- tcg/sparc/tcg-target.c | 2 +- tcg/tcg.c| 6 +++--- tcg/tci/tcg-target.c | 2 +- 11 files changed, 13 insertions(+), 13 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index cee3704..e028dd6 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -188,7 +188,7 @@ static inline bool is_limm(uint64_t val) return (val & (val - 1)) == 0; } -static int tcg_target_const_match(tcg_target_long val, +static int tcg_target_const_match(tcg_target_long val, TCGType type, const TCGArgConstraint *arg_ct) { int ct = arg_ct->ct; diff --git a/tcg/arm/tcg-target.c b/tcg/arm/tcg-target.c index 5a09226..7535175 100644 --- a/tcg/arm/tcg-target.c +++ b/tcg/arm/tcg-target.c @@ -261,7 +261,7 @@ static inline int check_fit_imm(uint32_t imm) * mov operand2: values represented with x << (2 * y), x < 0x100 * add, sub, eor...: ditto */ -static inline int tcg_target_const_match(tcg_target_long val, +static inline int tcg_target_const_match(tcg_target_long val, TCGType type, const TCGArgConstraint *arg_ct) { int ct; diff --git a/tcg/i386/tcg-target.c b/tcg/i386/tcg-target.c index 0c2347c..34ece1f 100644 --- a/tcg/i386/tcg-target.c +++ b/tcg/i386/tcg-target.c @@ -257,7 +257,7 @@ static int target_parse_constraint(TCGArgConstraint *ct, const char **pct_str) } /* test if a constant matches the constraint */ -static inline int tcg_target_const_match(tcg_target_long val, +static inline int tcg_target_const_match(tcg_target_long val, TCGType type, const TCGArgConstraint *arg_ct) { int ct = arg_ct->ct; diff --git a/tcg/ia64/tcg-target.c b/tcg/ia64/tcg-target.c index 2d8e00c..c640184 100644 --- a/tcg/ia64/tcg-target.c +++ b/tcg/ia64/tcg-target.c @@ -832,7 +832,7 @@ static int target_parse_constraint(TCGArgConstraint *ct, const char **pct_str) } /* test if a constant matches the constraint */ -static inline int tcg_target_const_match(tcg_target_long val, +static inline int tcg_target_const_match(tcg_target_long val, TCGType type, const TCGArgConstraint *arg_ct) { int ct; diff --git a/tcg/mips/tcg-target.c b/tcg/mips/tcg-target.c index 40551cd..76bc222 100644 --- a/tcg/mips/tcg-target.c +++ b/tcg/mips/tcg-target.c @@ -253,7 +253,7 @@ static int target_parse_constraint(TCGArgConstraint *ct, const char **pct_str) } /* test if a constant matches the constraint */ -static inline int tcg_target_const_match(tcg_target_long val, +static inline int tcg_target_const_match(tcg_target_long val, TCGType type, const TCGArgConstraint *arg_ct) { int ct; diff --git a/tcg/ppc/tcg-target.c b/tcg/ppc/tcg-target.c index 06bedd9..83d9340 100644 --- a/tcg/ppc/tcg-target.c +++ b/tcg/ppc/tcg-target.c @@ -298,7 +298,7 @@ static int target_parse_constraint(TCGArgConstraint *ct, const char **pct_str) } /* test if a constant matches the constraint */ -static int tcg_target_const_match(tcg_target_long val, +static int tcg_target_const_match(tcg_target_long val, TCGType type, const TCGArgConstraint *arg_ct) { int ct; diff --git a/tcg/ppc64/tcg-target.c b/tcg/ppc64/tcg-target.c index 06e440f..a533698 100644 --- a/tcg/ppc64/tcg-target.c +++ b/tcg/ppc64/tcg-target.c @@ -290,7 +290,7 @@ static int target_parse_constraint(TCGArgConstraint *ct, const char **pct_str) } /* test if a constant matches the constraint */ -static int tcg_target_const_match(tcg_target_long val, +static int tcg_target_const_match(tcg_target_long val, TCGType type, const TCGArgConstraint *arg_ct) { int ct = arg_ct->ct; diff --git a/tcg/s390/tcg-target.c b/tcg/s390/tcg-target.c index 907d9d1..feeaf97 100644 --- a/tcg/s390/tcg-target.c +++ b/tcg/s390/tcg-target.c @@ -515,7 +515,7 @@ static int tcg_match_cmpi(int ct, tcg_target_long val) } /* Test if a constant matches the constraint. */ -static int tcg_target_const_match(tcg_target_long val, +static int tcg_target_const_match(tcg_target_long val, TCGType type, const TCGArgConstraint *arg_ct) { int ct = arg_ct->ct; diff --git a/tcg/sparc/tcg-target.c b/tcg/sparc/tcg-target.c index 152335c..73121e1 100644 --- a/tcg/sparc/tcg-target.c +++ b/tcg/sparc/tcg-target.c @@ -327,7 +327,7 @@ static int target_parse_constraint(TCGArgConstraint *ct, const char **pct_str) } /* test if a constant ma
[Qemu-devel] [PATCH for-2.1 11/14] tcg: Relax requirement for mulu2_i32 on 32-bit hosts
Instead require either mulu2_i32 or muluh_i32. The code in tcg-op.h already supports looking for both. Previous incomplete conversion? Signed-off-by: Richard Henderson --- tcg/arm/tcg-target.h | 1 + tcg/mips/tcg-target.h | 1 + tcg/ppc/tcg-target.h | 1 + tcg/tcg.h | 8 +++- tcg/tci/tcg-target.h | 2 ++ 5 files changed, 12 insertions(+), 1 deletion(-) diff --git a/tcg/arm/tcg-target.h b/tcg/arm/tcg-target.h index 3746b6e..35de34e 100644 --- a/tcg/arm/tcg-target.h +++ b/tcg/arm/tcg-target.h @@ -79,6 +79,7 @@ extern bool use_idiv_instructions; #define TCG_TARGET_HAS_nor_i32 0 #define TCG_TARGET_HAS_deposit_i32 1 #define TCG_TARGET_HAS_movcond_i32 1 +#define TCG_TARGET_HAS_mulu2_i321 #define TCG_TARGET_HAS_muls2_i321 #define TCG_TARGET_HAS_muluh_i320 #define TCG_TARGET_HAS_mulsh_i320 diff --git a/tcg/mips/tcg-target.h b/tcg/mips/tcg-target.h index 683c6af..c3822d0 100644 --- a/tcg/mips/tcg-target.h +++ b/tcg/mips/tcg-target.h @@ -109,6 +109,7 @@ extern bool use_mips32r2_instructions; #define TCG_TARGET_HAS_orc_i32 0 #define TCG_TARGET_HAS_eqv_i32 0 #define TCG_TARGET_HAS_nand_i32 0 +#define TCG_TARGET_HAS_mulu2_i321 #define TCG_TARGET_HAS_muls2_i321 #define TCG_TARGET_HAS_muluh_i321 #define TCG_TARGET_HAS_mulsh_i321 diff --git a/tcg/ppc/tcg-target.h b/tcg/ppc/tcg-target.h index e3395e3..1168912 100644 --- a/tcg/ppc/tcg-target.h +++ b/tcg/ppc/tcg-target.h @@ -95,6 +95,7 @@ typedef enum { #define TCG_TARGET_HAS_nor_i32 1 #define TCG_TARGET_HAS_deposit_i32 1 #define TCG_TARGET_HAS_movcond_i32 1 +#define TCG_TARGET_HAS_mulu2_i321 #define TCG_TARGET_HAS_muls2_i320 #define TCG_TARGET_HAS_muluh_i320 #define TCG_TARGET_HAS_mulsh_i320 diff --git a/tcg/tcg.h b/tcg/tcg.h index f7efcb4..0bb6677 100644 --- a/tcg/tcg.h +++ b/tcg/tcg.h @@ -97,7 +97,6 @@ typedef uint64_t TCGRegSet; /* Turn some undef macros into true macros. */ #define TCG_TARGET_HAS_add2_i32 1 #define TCG_TARGET_HAS_sub2_i32 1 -#define TCG_TARGET_HAS_mulu2_i321 #endif #ifndef TCG_TARGET_deposit_i32_valid @@ -121,6 +120,13 @@ typedef uint64_t TCGRegSet; #define TCG_TARGET_HAS_rem_i64 0 #endif +/* For 32-bit targets, some sort of unsigned widening multiply is required. */ +#if TCG_TARGET_REG_BITS == 32 \ +&& !(defined(TCG_TARGET_HAS_mulu2_i32) \ + || defined(TCG_TARGET_HAS_muluh_i32)) +# error "Missing unsigned widening multiply" +#endif + typedef enum TCGOpcode { #define DEF(name, oargs, iargs, cargs, flags) INDEX_op_ ## name, #include "tcg-opc.h" diff --git a/tcg/tci/tcg-target.h b/tcg/tci/tcg-target.h index 6e1da8c..7869119 100644 --- a/tcg/tci/tcg-target.h +++ b/tcg/tci/tcg-target.h @@ -118,6 +118,8 @@ #define TCG_TARGET_HAS_mulu2_i640 #define TCG_TARGET_HAS_muluh_i640 #define TCG_TARGET_HAS_mulsh_i640 +#else +#define TCG_TARGET_HAS_mulu2_i321 #endif /* TCG_TARGET_REG_BITS == 64 */ #define TCG_TARGET_HAS_new_ldst 0 -- 1.9.0
[Qemu-devel] [PATCH for-2.1 04/14] tci: Mask shift counts to avoid undefined behavior
TCG now requires unspecified behavior rather than a potential crash, bring the C shift within the letter of the law. Signed-off-by: Richard Henderson --- tci.c | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tci.c b/tci.c index 0202ed9..6523ab8 100644 --- a/tci.c +++ b/tci.c @@ -669,32 +669,32 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr) t0 = *tb_ptr++; t1 = tci_read_ri32(&tb_ptr); t2 = tci_read_ri32(&tb_ptr); -tci_write_reg32(t0, t1 << t2); +tci_write_reg32(t0, t1 << (t2 & 31)); break; case INDEX_op_shr_i32: t0 = *tb_ptr++; t1 = tci_read_ri32(&tb_ptr); t2 = tci_read_ri32(&tb_ptr); -tci_write_reg32(t0, t1 >> t2); +tci_write_reg32(t0, t1 >> (t2 & 31)); break; case INDEX_op_sar_i32: t0 = *tb_ptr++; t1 = tci_read_ri32(&tb_ptr); t2 = tci_read_ri32(&tb_ptr); -tci_write_reg32(t0, ((int32_t)t1 >> t2)); +tci_write_reg32(t0, ((int32_t)t1 >> (t2 & 31))); break; #if TCG_TARGET_HAS_rot_i32 case INDEX_op_rotl_i32: t0 = *tb_ptr++; t1 = tci_read_ri32(&tb_ptr); t2 = tci_read_ri32(&tb_ptr); -tci_write_reg32(t0, rol32(t1, t2)); +tci_write_reg32(t0, rol32(t1, t2 & 31)); break; case INDEX_op_rotr_i32: t0 = *tb_ptr++; t1 = tci_read_ri32(&tb_ptr); t2 = tci_read_ri32(&tb_ptr); -tci_write_reg32(t0, ror32(t1, t2)); +tci_write_reg32(t0, ror32(t1, t2 & 31)); break; #endif #if TCG_TARGET_HAS_deposit_i32 @@ -936,32 +936,32 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr) t0 = *tb_ptr++; t1 = tci_read_ri64(&tb_ptr); t2 = tci_read_ri64(&tb_ptr); -tci_write_reg64(t0, t1 << t2); +tci_write_reg64(t0, t1 << (t2 & 63)); break; case INDEX_op_shr_i64: t0 = *tb_ptr++; t1 = tci_read_ri64(&tb_ptr); t2 = tci_read_ri64(&tb_ptr); -tci_write_reg64(t0, t1 >> t2); +tci_write_reg64(t0, t1 >> (t2 & 63)); break; case INDEX_op_sar_i64: t0 = *tb_ptr++; t1 = tci_read_ri64(&tb_ptr); t2 = tci_read_ri64(&tb_ptr); -tci_write_reg64(t0, ((int64_t)t1 >> t2)); +tci_write_reg64(t0, ((int64_t)t1 >> (t2 & 63))); break; #if TCG_TARGET_HAS_rot_i64 case INDEX_op_rotl_i64: t0 = *tb_ptr++; t1 = tci_read_ri64(&tb_ptr); t2 = tci_read_ri64(&tb_ptr); -tci_write_reg64(t0, rol64(t1, t2)); +tci_write_reg64(t0, rol64(t1, t2 & 63)); break; case INDEX_op_rotr_i64: t0 = *tb_ptr++; t1 = tci_read_ri64(&tb_ptr); t2 = tci_read_ri64(&tb_ptr); -tci_write_reg64(t0, ror64(t1, t2)); +tci_write_reg64(t0, ror64(t1, t2 & 63)); break; #endif #if TCG_TARGET_HAS_deposit_i64 -- 1.9.0
[Qemu-devel] [PATCH for-2.1 03/14] tcg: Mask shift quantities while folding
The TCG result would be undefined, but we can at least produce one plausible result and avoid triggering the wrath of analysis tools. Reviewed-by: Peter Maydell Signed-off-by: Richard Henderson --- tcg/optimize.c | 35 --- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/tcg/optimize.c b/tcg/optimize.c index 743..2fb708e 100644 --- a/tcg/optimize.c +++ b/tcg/optimize.c @@ -220,34 +220,34 @@ static TCGArg do_constant_folding_2(TCGOpcode op, TCGArg x, TCGArg y) return x ^ y; case INDEX_op_shl_i32: -return (uint32_t)x << (uint32_t)y; +return (uint32_t)x << (y & 31); case INDEX_op_shl_i64: -return (uint64_t)x << (uint64_t)y; +return (uint64_t)x << (y & 63); case INDEX_op_shr_i32: -return (uint32_t)x >> (uint32_t)y; +return (uint32_t)x >> (y & 31); case INDEX_op_shr_i64: -return (uint64_t)x >> (uint64_t)y; +return (uint64_t)x >> (y & 63); case INDEX_op_sar_i32: -return (int32_t)x >> (int32_t)y; +return (int32_t)x >> (y & 31); case INDEX_op_sar_i64: -return (int64_t)x >> (int64_t)y; +return (int64_t)x >> (y & 63); case INDEX_op_rotr_i32: -return ror32(x, y); +return ror32(x, y & 31); case INDEX_op_rotr_i64: -return ror64(x, y); +return ror64(x, y & 63); case INDEX_op_rotl_i32: -return rol32(x, y); +return rol32(x, y & 31); case INDEX_op_rotl_i64: -return rol64(x, y); +return rol64(x, y & 63); CASE_OP_32_64(not): return ~x; @@ -806,29 +806,34 @@ static TCGArg *tcg_constant_folding(TCGContext *s, uint16_t *tcg_opc_ptr, case INDEX_op_sar_i32: if (temps[args[2]].state == TCG_TEMP_CONST) { -mask = (int32_t)temps[args[1]].mask >> temps[args[2]].val; +tmp = temps[args[2]].val & 31; +mask = (int32_t)temps[args[1]].mask >> tmp; } break; case INDEX_op_sar_i64: if (temps[args[2]].state == TCG_TEMP_CONST) { -mask = (int64_t)temps[args[1]].mask >> temps[args[2]].val; +tmp = temps[args[2]].val & 63; +mask = (int64_t)temps[args[1]].mask >> tmp; } break; case INDEX_op_shr_i32: if (temps[args[2]].state == TCG_TEMP_CONST) { -mask = (uint32_t)temps[args[1]].mask >> temps[args[2]].val; +tmp = temps[args[2]].val & 31; +mask = (uint32_t)temps[args[1]].mask >> tmp; } break; case INDEX_op_shr_i64: if (temps[args[2]].state == TCG_TEMP_CONST) { -mask = (uint64_t)temps[args[1]].mask >> temps[args[2]].val; +tmp = temps[args[2]].val & 63; +mask = (uint64_t)temps[args[1]].mask >> tmp; } break; CASE_OP_32_64(shl): if (temps[args[2]].state == TCG_TEMP_CONST) { -mask = temps[args[1]].mask << temps[args[2]].val; +tmp = temps[args[2]].val & (TCG_TARGET_REG_BITS - 1); +mask = temps[args[1]].mask << tmp; } break; -- 1.9.0
[Qemu-devel] [PATCH for-2.1 01/14] tcg: Fix warning (1 bit signed bitfield entry) and replace int by bool
From: Stefan Weil Static code analyzers complain about signed bitfields with only a single bit. is_ld is used as a boolean value, so make it bool. ppc64 already used bool for the 2nd argument is_ld of the local function add_qemu_ldst_label. Modify all other TCG targets to do follow this example. Signed-off-by: Stefan Weil Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 6 +++--- tcg/arm/tcg-target.c | 6 +++--- tcg/i386/tcg-target.c| 6 +++--- tcg/ppc/tcg-target.c | 6 +++--- tcg/tcg-be-ldst.h| 2 +- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index 661a5af..cee3704 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -982,7 +982,7 @@ static void tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) tcg_out_goto(s, (tcg_target_long)lb->raddr); } -static void add_qemu_ldst_label(TCGContext *s, int is_ld, int opc, +static void add_qemu_ldst_label(TCGContext *s, bool is_ld, int opc, TCGReg data_reg, TCGReg addr_reg, int mem_index, uint8_t *raddr, uint8_t *label_ptr) @@ -1150,7 +1150,7 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, int opc) s_bits = opc & 3; tcg_out_tlb_read(s, addr_reg, s_bits, &label_ptr, mem_index, 1); tcg_out_qemu_ld_direct(s, opc, data_reg, addr_reg, TCG_REG_X1); -add_qemu_ldst_label(s, 1, opc, data_reg, addr_reg, +add_qemu_ldst_label(s, true, opc, data_reg, addr_reg, mem_index, s->code_ptr, label_ptr); #else /* !CONFIG_SOFTMMU */ tcg_out_qemu_ld_direct(s, opc, data_reg, addr_reg, @@ -1174,7 +1174,7 @@ static void tcg_out_qemu_st(TCGContext *s, const TCGArg *args, int opc) tcg_out_tlb_read(s, addr_reg, s_bits, &label_ptr, mem_index, 0); tcg_out_qemu_st_direct(s, opc, data_reg, addr_reg, TCG_REG_X1); -add_qemu_ldst_label(s, 0, opc, data_reg, addr_reg, +add_qemu_ldst_label(s, false, opc, data_reg, addr_reg, mem_index, s->code_ptr, label_ptr); #else /* !CONFIG_SOFTMMU */ tcg_out_qemu_st_direct(s, opc, data_reg, addr_reg, diff --git a/tcg/arm/tcg-target.c b/tcg/arm/tcg-target.c index a65fc65..5a09226 100644 --- a/tcg/arm/tcg-target.c +++ b/tcg/arm/tcg-target.c @@ -1253,7 +1253,7 @@ static TCGReg tcg_out_tlb_read(TCGContext *s, TCGReg addrlo, TCGReg addrhi, /* Record the context of a call to the out of line helper code for the slow path for a load or store, so that we can later generate the correct helper code. */ -static void add_qemu_ldst_label(TCGContext *s, int is_ld, TCGMemOp opc, +static void add_qemu_ldst_label(TCGContext *s, bool is_ld, TCGMemOp opc, TCGReg datalo, TCGReg datahi, TCGReg addrlo, TCGReg addrhi, int mem_index, uint8_t *raddr, uint8_t *label_ptr) @@ -1519,7 +1519,7 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, bool is64) tcg_out_qemu_ld_index(s, opc, datalo, datahi, addrlo, addend); -add_qemu_ldst_label(s, 1, opc, datalo, datahi, addrlo, addrhi, +add_qemu_ldst_label(s, true, opc, datalo, datahi, addrlo, addrhi, mem_index, s->code_ptr, label_ptr); #else /* !CONFIG_SOFTMMU */ if (GUEST_BASE) { @@ -1647,7 +1647,7 @@ static void tcg_out_qemu_st(TCGContext *s, const TCGArg *args, bool is64) label_ptr = s->code_ptr; tcg_out_bl_noaddr(s, COND_NE); -add_qemu_ldst_label(s, 0, opc, datalo, datahi, addrlo, addrhi, +add_qemu_ldst_label(s, false, opc, datalo, datahi, addrlo, addrhi, mem_index, s->code_ptr, label_ptr); #else /* !CONFIG_SOFTMMU */ if (GUEST_BASE) { diff --git a/tcg/i386/tcg-target.c b/tcg/i386/tcg-target.c index f832282..0c2347c 100644 --- a/tcg/i386/tcg-target.c +++ b/tcg/i386/tcg-target.c @@ -1244,7 +1244,7 @@ static inline void tcg_out_tlb_load(TCGContext *s, TCGReg addrlo, TCGReg addrhi, * Record the context of a call to the out of line helper code for the slow path * for a load or store, so that we can later generate the correct helper code */ -static void add_qemu_ldst_label(TCGContext *s, int is_ld, TCGMemOp opc, +static void add_qemu_ldst_label(TCGContext *s, bool is_ld, TCGMemOp opc, TCGReg datalo, TCGReg datahi, TCGReg addrlo, TCGReg addrhi, int mem_index, uint8_t *raddr, @@ -1554,7 +1554,7 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, bool is64) tcg_out_qemu_ld_direct(s, datalo, datahi, TCG_REG_L1, 0, 0, opc); /* Record the current context of a load into ldst label */ -add_qemu_ldst_label(s, 1, opc, datalo, datahi, addrlo, addrhi, +add_qemu_ldst_label(s, true, opc, datalo, datahi, addrlo, addrhi,
[Qemu-devel] [PATCH for-2.1 02/14] tcg: Use "unspecified behavior" for shifts
Change the definition such that shifts are not allowed to crash for any input. Reviewed-by: Peter Maydell Signed-off-by: Richard Henderson --- tcg/README | 18 +- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/tcg/README b/tcg/README index f178212..776e925 100644 --- a/tcg/README +++ b/tcg/README @@ -36,6 +36,12 @@ or a memory location which is stored in a register outside QEMU TBs A TCG "basic block" corresponds to a list of instructions terminated by a branch instruction. +An operation with "undefined behavior" may result in a crash. + +An operation with "unspecified behavior" shall not crash. However, +the result may be one of several possibilities so may be considered +an "undefined result". + 3) Intermediate representation 3.1) Introduction @@ -239,23 +245,25 @@ t0=t1|~t2 * shl_i32/i64 t0, t1, t2 -t0=t1 << t2. Undefined behavior if t2 < 0 or t2 >= 32 (resp 64) +t0=t1 << t2. Unspecified behavior if t2 < 0 or t2 >= 32 (resp 64) * shr_i32/i64 t0, t1, t2 -t0=t1 >> t2 (unsigned). Undefined behavior if t2 < 0 or t2 >= 32 (resp 64) +t0=t1 >> t2 (unsigned). Unspecified behavior if t2 < 0 or t2 >= 32 (resp 64) * sar_i32/i64 t0, t1, t2 -t0=t1 >> t2 (signed). Undefined behavior if t2 < 0 or t2 >= 32 (resp 64) +t0=t1 >> t2 (signed). Unspecified behavior if t2 < 0 or t2 >= 32 (resp 64) * rotl_i32/i64 t0, t1, t2 -Rotation of t2 bits to the left. Undefined behavior if t2 < 0 or t2 >= 32 (resp 64) +Rotation of t2 bits to the left. +Unspecified behavior if t2 < 0 or t2 >= 32 (resp 64) * rotr_i32/i64 t0, t1, t2 -Rotation of t2 bits to the right. Undefined behavior if t2 < 0 or t2 >= 32 (resp 64) +Rotation of t2 bits to the right. +Unspecified behavior if t2 < 0 or t2 >= 32 (resp 64) * Misc -- 1.9.0
Re: [Qemu-devel] [PATCH 2/7] megasas: Enable MSI-X support
Am 16.04.2014 18:32, schrieb Alexander Graf: > > On 16.04.14 16:44, Hannes Reinecke wrote: >> MSI-X support has been fixed in qemu, so we can enable it again. >> >> Signed-off-by: Hannes Reinecke >> --- >> hw/scsi/megasas.c | 19 ++- >> 1 file changed, 6 insertions(+), 13 deletions(-) >> >> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c >> index 1781525..df45286 100644 >> --- a/hw/scsi/megasas.c >> +++ b/hw/scsi/megasas.c >> @@ -2084,7 +2084,8 @@ static const VMStateDescription vmstate_megasas = { >> .minimum_version_id = 0, >> .minimum_version_id_old = 0, >> .fields = (VMStateField[]) { >> -VMSTATE_PCI_DEVICE(parent_obj, MegasasState), >> +VMSTATE_PCIE_DEVICE(parent_obj, MegasasState), >> +VMSTATE_MSIX(parent_obj, MegasasState), > > This requires a version change for vmstate, no? The PCI -> PCIE change yes. mst might have objections for bumping an x86 PCI device? The MSIX addition should be safe AFAICT since old versions would not enable MSI-X. Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On 16 April 2014 17:34, Michael S. Tsirkin wrote: > so it looks like virtio is currently compiled per-target. > So why isn't it reasonable to keep it per-target for > purpose of this enhancement? > What am I missing? "virtio" is more than one C file. Currently per-target: hw/virtio/virtio.c hw/virtio/virtio-balloon.c hw/scsi/virtio-scsi.c hw/block/virtio-blk.c hw/char/virtio-serial-bus.c hw/net/virtio-net.c Currently built once: hw/char/virtio-console.c hw/virtio/virtio-bus.c hw/virtio/virtio-mmio.c hw/virtio/virtio-pci.c hw/virtio/virtio-rng.c If we can move files from the first group to the second that's cool. Moving files from the second to the first is what I'd like to avoid... thanks -- PMM
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Tue, Apr 15, 2014 at 03:22:56PM +0200, Greg Kurz wrote: > On Tue, 15 Apr 2014 13:35:03 +0200 > Alexander Graf wrote: > > > On 04/15/2014 10:40 AM, Greg Kurz wrote: > > > On Mon, 14 Apr 2014 15:08:23 +0200 > > > Alexander Graf wrote: > > > > > >> On 14.04.14 14:55, Michael S. Tsirkin wrote: > > >>> On Mon, Apr 14, 2014 at 02:40:04PM +0200, Alexander Graf wrote: > > On 14.04.14 14:37, Michael S. Tsirkin wrote: > > > On Mon, Apr 14, 2014 at 02:29:20PM +0200, Alexander Graf wrote: > > >> On 14.04.14 14:24, Michael S. Tsirkin wrote: > > >>> On Mon, Apr 14, 2014 at 02:16:03PM +0200, Alexander Graf wrote: > > On 14.04.14 13:58, Greg Kurz wrote: > > > From: Rusty Russell > > > > > > virtio data structures are defined as "target endian", which > > > assumes > > > that's a fixed value. In fact, that actually means it's > > > platform-specific. > > > The OASIS virtio 1.0 spec will fix this, by making it all little > > > endian. > > > > > > We introduce memory accessors to be used accross the virtio code > > > where > > > needed. These accessors should support both legacy and 1.0 > > > devices. > > > A good way to do it is to introduce a per-device property to > > > store the > > > endianness. We choose to set this flag at device reset time > > > because it > > > is reasonnable to assume the endianness won't change unless we > > > reboot or > > > kexec another kernel. And it is also reasonnable to assume the > > > new kernel > > > will reset the devices before using them (otherwise it will > > > break). > > > > > > We reuse the virtio_is_big_endian() helper since it provides the > > > right > > > value for legacy devices with most of the targets, that have fixed > > > endianness. It can then be overriden to support endian-ambivalent > > > targets. > > > > > > To support migration, we need to set the flag in virtio_load() as > > > well. > > > > > > (a) One solution would be to add it to the stream, but it have > > > some > > > drawbacks: > > > - since this only affects a few targets, the field should be put > > > into a > > > subsection > > > - virtio migration code should be ported to vmstate to be able to > > > introduce > > > such a subsection > > > > > > (b) If we assume the following to be true: > > > - target endianness falls under some cpu state > > > - cpu state is always restored before virtio devices state > > > because they > > > get initialized in this order in main(). > > > Then an alternative is to rely on virtio_is_big_endian() again at > > > load time. No need to mess around with the migration stream in > > > this case. > > > > > > This patch implements (b). > > > > > > Note that the tswap helpers are implemented in virtio.c so that > > > virtio-access.h stays platform independant. Most of the virtio > > > code > > > will be buildable under common-obj instead of obj then, and spare > > > some cycles when building for multiple targets. > > > > > > Signed-off-by: Rusty Russell > > > [ ldq_phys() API change, > > > relicensed virtio-access.h to GPLv2+ on Rusty's request, > > > introduce a per-device is_big_endian flag (supersedes > > > needs_byteswap global) > > > add VirtIODevice * arg to virtio helpers, > > > use the existing virtio_is_big_endian() helper, > > > virtio-pci: use the device is_big_endian flag, > > > introduce virtio tswap16 and tswap64 helpers, > > > move calls to tswap* out of virtio-access.h to make it > > > platform independant, > > > migration support, > > > Greg Kurz ] > > > Cc: Cédric Le Goater > > > Signed-off-by: Greg Kurz > > > --- > > > > > > Changes since v6: > > > - merge the virtio_needs_byteswap() helper from v6 and existing > > > virtio_is_big_endian() > > > - virtio-pci: now supports endianness changes > > > - virtio-access.h fixes (target independant) > > > > > >exec.c|2 - > > >hw/virtio/Makefile.objs |2 - > > >hw/virtio/virtio-pci.c| 11 +-- > > >hw/virtio/virtio.c| 35 + > > >include/hw/virtio/virtio-access.h | 138 > > > + > > >include/hw/virtio/virtio.h|2 + > > >vl.c
Re: [Qemu-devel] [PATCH 2/7] megasas: Enable MSI-X support
On 16.04.14 16:44, Hannes Reinecke wrote: MSI-X support has been fixed in qemu, so we can enable it again. Signed-off-by: Hannes Reinecke --- hw/scsi/megasas.c | 19 ++- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index 1781525..df45286 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -2084,7 +2084,8 @@ static const VMStateDescription vmstate_megasas = { .minimum_version_id = 0, .minimum_version_id_old = 0, .fields = (VMStateField[]) { -VMSTATE_PCI_DEVICE(parent_obj, MegasasState), +VMSTATE_PCIE_DEVICE(parent_obj, MegasasState), +VMSTATE_MSIX(parent_obj, MegasasState), This requires a version change for vmstate, no? Alex
[Qemu-devel] [PULL for-2.1 24/25] tcg-aarch64: Prefer unsigned offsets before signed offsets for ldst
The assembler seems to prefer them, perhaps we should too. Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index 3235824..7ff4be7 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -669,11 +669,6 @@ static void tcg_out_ldst(TCGContext *s, AArch64Insn insn, { TCGMemOp size = (uint32_t)insn >> 30; -if (offset >= -256 && offset < 256) { -tcg_out_insn_3312(s, insn, rd, rn, offset); -return; -} - /* If the offset is naturally aligned and in range, then we can use the scaled uimm12 encoding */ if (offset >= 0 && !(offset & ((1 << size) - 1))) { @@ -684,6 +679,12 @@ static void tcg_out_ldst(TCGContext *s, AArch64Insn insn, } } +/* Small signed offsets can use the unscaled encoding. */ +if (offset >= -256 && offset < 256) { +tcg_out_insn_3312(s, insn, rd, rn, offset); +return; +} + /* Worst-case scenario, move offset to temp register, use reg offset. */ tcg_out_movi(s, TCG_TYPE_I64, TCG_REG_TMP, offset); tcg_out_ldst_r(s, insn, rd, rn, TCG_REG_TMP); -- 1.9.0
Re: [Qemu-devel] [PATCH 6/9] s390x/virtio-ccw: Wire up irq routing and irqfds.
On 16.04.14 16:44, Cornelia Huck wrote: On Wed, 16 Apr 2014 13:29:05 +0200 Alexander Graf wrote: On 14.04.14 18:48, Cornelia Huck wrote: Make use of the new s390 adapter irq routing support to enable real in-kernel irqfds for virtio-ccw with adapter interrupts. Note that s390 doesn't provide the common KVM_CAP_IRQCHIP capability, but rather needs KVM_CAP_S390_IRQCHIP to be enabled. This is to ensure backward compatibility. Reviewed-by: Thomas Huth Signed-off-by: Cornelia Huck --- hw/s390x/virtio-ccw.c | 165 hw/s390x/virtio-ccw.h |2 + include/hw/s390x/adapter.h | 23 ++ include/qemu/typedefs.h|1 + include/sysemu/kvm.h |2 + kvm-all.c | 38 +- kvm-stub.c |5 ++ target-s390x/kvm.c |5 ++ 8 files changed, 228 insertions(+), 13 deletions(-) create mode 100644 include/hw/s390x/adapter.h diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index 69efa6c..5612ccc 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -21,6 +21,7 @@ #include "hw/sysbus.h" #include "qemu/bitops.h" #include "hw/virtio/virtio-bus.h" +#include "hw/s390x/adapter.h" #include "ioinst.h" #include "css.h" @@ -48,7 +49,7 @@ static IndAddr *get_indicator(hwaddr ind_addr, int len) return indicator; } -static void release_indicator(IndAddr *indicator) +static void release_indicator(uint32_t adapter_id, IndAddr *indicator) { assert(indicator->refcnt > 0); indicator->refcnt--; @@ -56,9 +57,31 @@ static void release_indicator(IndAddr *indicator) return; } QTAILQ_REMOVE(&indicator_addresses, indicator, sibling); +if (indicator->map) { +s390_io_adapter_map(adapter_id, indicator->map, false); +} g_free(indicator); } +static int map_indicator(uint32_t adapter_id, IndAddr *indicator) +{ +int ret; + +if (indicator->map) { +return 0; /* already mapped is not an error */ +} +indicator->map = indicator->addr; +ret = s390_io_adapter_map(adapter_id, indicator->map, true); +if ((ret != 0) && (ret != -ENOSYS)) { +goto out_err; +} +return 0; + +out_err: +indicator->map = 0; +return -EFAULT; +} + static void virtio_ccw_bus_new(VirtioBusState *bus, size_t bus_size, VirtioCcwDevice *dev); @@ -733,7 +756,7 @@ static int virtio_ccw_exit(VirtioCcwDevice *dev) g_free(sch); } if (dev->indicators) { -release_indicator(dev->indicators); +release_indicator(dev->adapter_id, dev->indicators); dev->indicators = NULL; } return 0; @@ -1034,15 +1057,15 @@ static void virtio_ccw_reset(DeviceState *d) virtio_reset(vdev); css_reset_sch(dev->sch); if (dev->indicators) { -release_indicator(dev->indicators); +release_indicator(dev->adapter_id, dev->indicators); dev->indicators = NULL; } if (dev->indicators2) { -release_indicator(dev->indicators2); +release_indicator(dev->adapter_id, dev->indicators2); dev->indicators2 = NULL; } if (dev->summary_indicator) { -release_indicator(dev->summary_indicator); +release_indicator(dev->adapter_id, dev->summary_indicator); dev->summary_indicator = NULL; } } @@ -1078,6 +1101,100 @@ static int virtio_ccw_set_host_notifier(DeviceState *d, int n, bool assign) return virtio_ccw_set_guest2host_notifier(dev, n, assign, false); } +static int virtio_ccw_get_adapter_info(VirtioCcwDevice *dev, + AdapterInfo *adapter) +{ +int r; + +if (!dev->sch->thinint_active) { +return -EINVAL; +} + +r = map_indicator(dev->adapter_id, dev->summary_indicator); +if (r) { +return r; +} +r = map_indicator(dev->adapter_id, dev->indicators); +if (r) { +return r; +} +adapter->summary_addr = dev->summary_indicator->map; +adapter->ind_addr = dev->indicators->map; +adapter->ind_offset = dev->ind_bit; +adapter->summary_offset = 7; +adapter->adapter_id = dev->adapter_id; + +return 0; +} + +static int virtio_ccw_setup_irqroutes(VirtioCcwDevice *dev, int nvqs) +{ +int i; +VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); +int ret; +AdapterInfo adapter; + +ret = virtio_ccw_get_adapter_info(dev, &adapter); +if (ret) { +return ret; +} +for (i = 0; i < nvqs; i++) { +if (!virtio_queue_get_num(vdev, i)) { +break; +} +ret = kvm_irqchip_add_adapter_route(kvm_state, &adapter); Why is interrupt routing code in virtio-ccw.c? Shouldn't that live in the flic? It needs information about the virtio-ccw device (and the virtio-device). Could we somehow manage to get a workflow like thi
[Qemu-devel] [PULL for-2.1 13/25] tcg-aarch64: Implement tcg_register_jit
Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 84 +++- 1 file changed, 69 insertions(+), 15 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index 5cffe50..4414bd1 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -1848,24 +1848,29 @@ static void tcg_target_init(TCGContext *s) tcg_add_target_add_op_defs(aarch64_op_defs); } +/* Saving pairs: (X19, X20) .. (X27, X28), (X29(fp), X30(lr)). */ +#define PUSH_SIZE ((30 - 19 + 1) * 8) + +#define FRAME_SIZE \ +((PUSH_SIZE \ + + TCG_STATIC_CALL_ARGS_SIZE \ + + CPU_TEMP_BUF_NLONGS * sizeof(long) \ + + TCG_TARGET_STACK_ALIGN - 1) \ + & ~(TCG_TARGET_STACK_ALIGN - 1)) + +/* We're expecting a 2 byte uleb128 encoded value. */ +QEMU_BUILD_BUG_ON(FRAME_SIZE >= (1 << 14)); + +/* We're expecting to use a single ADDI insn. */ +QEMU_BUILD_BUG_ON(FRAME_SIZE - PUSH_SIZE > 0xfff); + static void tcg_target_qemu_prologue(TCGContext *s) { -/* NB: frame sizes are in 16 byte stack units! */ -int frame_size_callee_saved, frame_size_tcg_locals; TCGReg r; -/* save pairs (FP, LR) and (X19, X20) .. (X27, X28) */ -frame_size_callee_saved = 16 + (TCG_REG_X28 - TCG_REG_X19 + 1) * 8; - -/* frame size requirement for TCG local variables */ -frame_size_tcg_locals = TCG_STATIC_CALL_ARGS_SIZE -+ CPU_TEMP_BUF_NLONGS * sizeof(long) -+ (TCG_TARGET_STACK_ALIGN - 1); -frame_size_tcg_locals &= ~(TCG_TARGET_STACK_ALIGN - 1); - /* Push (FP, LR) and allocate space for all saved registers. */ tcg_out_insn(s, 3314, STP, TCG_REG_FP, TCG_REG_LR, - TCG_REG_SP, -frame_size_callee_saved, 1, 1); + TCG_REG_SP, -PUSH_SIZE, 1, 1); /* Set up frame pointer for canonical unwinding. */ tcg_out_movr_sp(s, TCG_TYPE_I64, TCG_REG_FP, TCG_REG_SP); @@ -1878,7 +1883,7 @@ static void tcg_target_qemu_prologue(TCGContext *s) /* Make stack space for TCG locals. */ tcg_out_insn(s, 3401, SUBI, TCG_TYPE_I64, TCG_REG_SP, TCG_REG_SP, - frame_size_tcg_locals); + FRAME_SIZE - PUSH_SIZE); /* Inform TCG about how to find TCG locals with register, offset, size. */ tcg_set_frame(s, TCG_REG_SP, TCG_STATIC_CALL_ARGS_SIZE, @@ -1898,7 +1903,7 @@ static void tcg_target_qemu_prologue(TCGContext *s) /* Remove TCG locals stack space. */ tcg_out_insn(s, 3401, ADDI, TCG_TYPE_I64, TCG_REG_SP, TCG_REG_SP, - frame_size_tcg_locals); + FRAME_SIZE - PUSH_SIZE); /* Restore registers x19..x28. */ for (r = TCG_REG_X19; r <= TCG_REG_X27; r += 2) { @@ -1908,6 +1913,55 @@ static void tcg_target_qemu_prologue(TCGContext *s) /* Pop (FP, LR), restore SP to previous frame. */ tcg_out_insn(s, 3314, LDP, TCG_REG_FP, TCG_REG_LR, - TCG_REG_SP, frame_size_callee_saved, 0, 1); + TCG_REG_SP, PUSH_SIZE, 0, 1); tcg_out_insn(s, 3207, RET, TCG_REG_LR); } + +typedef struct { +DebugFrameCIE cie; +DebugFrameFDEHeader fde; +uint8_t fde_def_cfa[4]; +uint8_t fde_reg_ofs[24]; +} DebugFrame; + +#define ELF_HOST_MACHINE EM_AARCH64 + +static DebugFrame debug_frame = { +.cie.len = sizeof(DebugFrameCIE)-4, /* length after .len member */ +.cie.id = -1, +.cie.version = 1, +.cie.code_align = 1, +.cie.data_align = 0x78, /* sleb128 -8 */ +.cie.return_column = TCG_REG_LR, + +/* Total FDE size does not include the "len" member. */ +.fde.len = sizeof(DebugFrame) - offsetof(DebugFrame, fde.cie_offset), + +.fde_def_cfa = { +12, TCG_REG_SP, /* DW_CFA_def_cfa sp, ... */ +(FRAME_SIZE & 0x7f) | 0x80, /* ... uleb128 FRAME_SIZE */ +(FRAME_SIZE >> 7) +}, +.fde_reg_ofs = { +0x80 + 28, 1, /* DW_CFA_offset, x28, -8 */ +0x80 + 27, 2, /* DW_CFA_offset, x27, -16 */ +0x80 + 26, 3, /* DW_CFA_offset, x26, -24 */ +0x80 + 25, 4, /* DW_CFA_offset, x25, -32 */ +0x80 + 24, 5, /* DW_CFA_offset, x24, -40 */ +0x80 + 23, 6, /* DW_CFA_offset, x23, -48 */ +0x80 + 22, 7, /* DW_CFA_offset, x22, -56 */ +0x80 + 21, 8, /* DW_CFA_offset, x21, -64 */ +0x80 + 20, 9, /* DW_CFA_offset, x20, -72 */ +0x80 + 19, 10, /* DW_CFA_offset, x1p, -80 */ +0x80 + 30, 11, /* DW_CFA_offset, lr, -88 */ +0x80 + 29, 12, /* DW_CFA_offset, fp, -96 */ +} +}; + +void tcg_register_jit(void *buf, size_t buf_size) +{ +debug_frame.fde.func_start = (intptr_t)buf; +debug_frame.fde.func_len = buf_size; + +tcg_register_jit_int(buf, buf_size, &debug_frame, sizeof(debug_fr
[Qemu-devel] [PULL for-2.1 18/25] tcg-aarch64: Pass qemu_ld/st arguments directly
Instead of passing them the "args" array. Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 49 +--- 1 file changed, 17 insertions(+), 32 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index 68305ea..3a2955f 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -1271,20 +1271,13 @@ static void tcg_out_qemu_st_direct(TCGContext *s, TCGMemOp memop, } } -static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, TCGMemOp memop) +static void tcg_out_qemu_ld(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, +TCGMemOp memop, int mem_index) { -TCGReg addr_reg, data_reg; #ifdef CONFIG_SOFTMMU -int mem_index; -TCGMemOp s_bits; +TCGMemOp s_bits = memop & MO_SIZE; uint8_t *label_ptr; -#endif -data_reg = args[0]; -addr_reg = args[1]; -#ifdef CONFIG_SOFTMMU -mem_index = args[2]; -s_bits = memop & MO_SIZE; tcg_out_tlb_read(s, addr_reg, s_bits, &label_ptr, mem_index, 1); tcg_out_qemu_ld_direct(s, memop, data_reg, addr_reg, TCG_REG_X1); add_qemu_ldst_label(s, 1, memop, data_reg, addr_reg, @@ -1295,20 +1288,12 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, TCGMemOp memop) #endif /* CONFIG_SOFTMMU */ } -static void tcg_out_qemu_st(TCGContext *s, const TCGArg *args, TCGMemOp memop) +static void tcg_out_qemu_st(TCGContext *s, TCGReg data_reg, TCGReg addr_reg, +TCGMemOp memop, int mem_index) { -TCGReg addr_reg, data_reg; #ifdef CONFIG_SOFTMMU -int mem_index; -TCGMemOp s_bits; +TCGMemOp s_bits = memop & MO_SIZE; uint8_t *label_ptr; -#endif -data_reg = args[0]; -addr_reg = args[1]; - -#ifdef CONFIG_SOFTMMU -mem_index = args[2]; -s_bits = memop & MO_SIZE; tcg_out_tlb_read(s, addr_reg, s_bits, &label_ptr, mem_index, 0); tcg_out_qemu_st_direct(s, memop, data_reg, addr_reg, TCG_REG_X1); @@ -1588,38 +1573,38 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, break; case INDEX_op_qemu_ld8u: -tcg_out_qemu_ld(s, args, MO_UB); +tcg_out_qemu_ld(s, a0, a1, MO_UB, a2); break; case INDEX_op_qemu_ld8s: -tcg_out_qemu_ld(s, args, MO_SB); +tcg_out_qemu_ld(s, a0, a1, MO_SB, a2); break; case INDEX_op_qemu_ld16u: -tcg_out_qemu_ld(s, args, MO_TEUW); +tcg_out_qemu_ld(s, a0, a1, MO_TEUW, a2); break; case INDEX_op_qemu_ld16s: -tcg_out_qemu_ld(s, args, MO_TESW); +tcg_out_qemu_ld(s, a0, a1, MO_TESW, a2); break; case INDEX_op_qemu_ld32u: case INDEX_op_qemu_ld32: -tcg_out_qemu_ld(s, args, MO_TEUL); +tcg_out_qemu_ld(s, a0, a1, MO_TEUL, a2); break; case INDEX_op_qemu_ld32s: -tcg_out_qemu_ld(s, args, MO_TESL); +tcg_out_qemu_ld(s, a0, a1, MO_TESL, a2); break; case INDEX_op_qemu_ld64: -tcg_out_qemu_ld(s, args, MO_TEQ); +tcg_out_qemu_ld(s, a0, a1, MO_TEQ, a2); break; case INDEX_op_qemu_st8: -tcg_out_qemu_st(s, args, MO_UB); +tcg_out_qemu_st(s, a0, a1, MO_UB, a2); break; case INDEX_op_qemu_st16: -tcg_out_qemu_st(s, args, MO_TEUW); +tcg_out_qemu_st(s, a0, a1, MO_TEUW, a2); break; case INDEX_op_qemu_st32: -tcg_out_qemu_st(s, args, MO_TEUL); +tcg_out_qemu_st(s, a0, a1, MO_TEUL, a2); break; case INDEX_op_qemu_st64: -tcg_out_qemu_st(s, args, MO_TEQ); +tcg_out_qemu_st(s, a0, a1, MO_TEQ, a2); break; case INDEX_op_bswap32_i64: -- 1.9.0
[Qemu-devel] [PULL for-2.1 19/25] tcg-aarch64: Implement TCG_TARGET_HAS_new_ldst
Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 89 tcg/aarch64/tcg-target.h | 2 +- 2 files changed, 31 insertions(+), 60 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index 3a2955f..34e477d 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -1047,21 +1047,27 @@ static inline void tcg_out_addsub2(TCGContext *s, int ext, TCGReg rl, /* helper signature: helper_ret_ld_mmu(CPUState *env, target_ulong addr, * int mmu_idx, uintptr_t ra) */ -static const void * const qemu_ld_helpers[4] = { -helper_ret_ldub_mmu, -helper_ret_lduw_mmu, -helper_ret_ldul_mmu, -helper_ret_ldq_mmu, +static const void * const qemu_ld_helpers[16] = { +[MO_UB] = helper_ret_ldub_mmu, +[MO_LEUW] = helper_le_lduw_mmu, +[MO_LEUL] = helper_le_ldul_mmu, +[MO_LEQ] = helper_le_ldq_mmu, +[MO_BEUW] = helper_be_lduw_mmu, +[MO_BEUL] = helper_be_ldul_mmu, +[MO_BEQ] = helper_be_ldq_mmu, }; /* helper signature: helper_ret_st_mmu(CPUState *env, target_ulong addr, * uintxx_t val, int mmu_idx, uintptr_t ra) */ -static const void * const qemu_st_helpers[4] = { -helper_ret_stb_mmu, -helper_ret_stw_mmu, -helper_ret_stl_mmu, -helper_ret_stq_mmu, +static const void * const qemu_st_helpers[16] = { +[MO_UB] = helper_ret_stb_mmu, +[MO_LEUW] = helper_le_stw_mmu, +[MO_LEUL] = helper_le_stl_mmu, +[MO_LEQ] = helper_le_stq_mmu, +[MO_BEUW] = helper_be_stw_mmu, +[MO_BEUL] = helper_be_stl_mmu, +[MO_BEQ] = helper_be_stq_mmu, }; static inline void tcg_out_adr(TCGContext *s, TCGReg rd, uintptr_t addr) @@ -1082,7 +1088,7 @@ static void tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) tcg_out_movr(s, TARGET_LONG_BITS == 64, TCG_REG_X1, lb->addrlo_reg); tcg_out_movi(s, TCG_TYPE_I32, TCG_REG_X2, lb->mem_index); tcg_out_adr(s, TCG_REG_X3, (intptr_t)lb->raddr); -tcg_out_call(s, (intptr_t)qemu_ld_helpers[size]); +tcg_out_call(s, (intptr_t)qemu_ld_helpers[opc & ~MO_SIGN]); if (opc & MO_SIGN) { tcg_out_sxt(s, TCG_TYPE_I64, size, lb->datalo_reg, TCG_REG_X0); } else { @@ -1094,7 +1100,8 @@ static void tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) static void tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) { -TCGMemOp size = lb->opc; +TCGMemOp opc = lb->opc; +TCGMemOp size = opc & MO_SIZE; reloc_pc19(lb->label_ptr[0], (intptr_t)s->code_ptr); @@ -1103,7 +1110,7 @@ static void tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) tcg_out_movr(s, size == MO_64, TCG_REG_X2, lb->datalo_reg); tcg_out_movi(s, TCG_TYPE_I32, TCG_REG_X3, lb->mem_index); tcg_out_adr(s, TCG_REG_X4, (intptr_t)lb->raddr); -tcg_out_call(s, (intptr_t)qemu_st_helpers[size]); +tcg_out_call(s, (intptr_t)qemu_st_helpers[opc]); tcg_out_goto(s, (intptr_t)lb->raddr); } @@ -1572,39 +1579,13 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, tcg_out_insn(s, 3506, CSEL, ext, a0, REG0(3), REG0(4), args[5]); break; -case INDEX_op_qemu_ld8u: -tcg_out_qemu_ld(s, a0, a1, MO_UB, a2); -break; -case INDEX_op_qemu_ld8s: -tcg_out_qemu_ld(s, a0, a1, MO_SB, a2); -break; -case INDEX_op_qemu_ld16u: -tcg_out_qemu_ld(s, a0, a1, MO_TEUW, a2); -break; -case INDEX_op_qemu_ld16s: -tcg_out_qemu_ld(s, a0, a1, MO_TESW, a2); -break; -case INDEX_op_qemu_ld32u: -case INDEX_op_qemu_ld32: -tcg_out_qemu_ld(s, a0, a1, MO_TEUL, a2); -break; -case INDEX_op_qemu_ld32s: -tcg_out_qemu_ld(s, a0, a1, MO_TESL, a2); +case INDEX_op_qemu_ld_i32: +case INDEX_op_qemu_ld_i64: +tcg_out_qemu_ld(s, a0, a1, a2, args[3]); break; -case INDEX_op_qemu_ld64: -tcg_out_qemu_ld(s, a0, a1, MO_TEQ, a2); -break; -case INDEX_op_qemu_st8: -tcg_out_qemu_st(s, a0, a1, MO_UB, a2); -break; -case INDEX_op_qemu_st16: -tcg_out_qemu_st(s, a0, a1, MO_TEUW, a2); -break; -case INDEX_op_qemu_st32: -tcg_out_qemu_st(s, a0, a1, MO_TEUL, a2); -break; -case INDEX_op_qemu_st64: -tcg_out_qemu_st(s, a0, a1, MO_TEQ, a2); +case INDEX_op_qemu_st_i32: +case INDEX_op_qemu_st_i64: +tcg_out_qemu_st(s, a0, a1, a2, args[3]); break; case INDEX_op_bswap32_i64: @@ -1770,20 +1751,10 @@ static const TCGTargetOpDef aarch64_op_defs[] = { { INDEX_op_movcond_i32, { "r", "r", "rwA", "rZ", "rZ" } }, { INDEX_op_movcond_i64, { "r", "r", "rA", "rZ", "rZ" } }, -{ INDEX_op_qemu_ld8u, { "r", "l" } }, -{ INDEX_op_qemu_ld8s, { "r", "l" } }, -{ INDEX_op_qemu_ld16u, { "r", "l" } }, -{ INDEX_op_qemu_ld16s, { "r", "l" } }, -{ INDEX_op_qemu
[Qemu-devel] [PULL for-2.1 25/25] tcg-aarch64: Use tcg_out_mov in preference to tcg_out_movr
It's the more canonical interface. Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 16 +++- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index 7ff4be7..73ed658 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -979,9 +979,7 @@ static inline void tcg_out_addsub2(TCGContext *s, int ext, TCGReg rl, } tcg_out_insn_3503(s, insn, ext, rh, ah, bh); -if (rl != orig_rl) { -tcg_out_movr(s, ext, orig_rl, rl); -} +tcg_out_mov(s, ext, orig_rl, rl); } #ifdef CONFIG_SOFTMMU @@ -1025,15 +1023,15 @@ static void tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) reloc_pc19(lb->label_ptr[0], (intptr_t)s->code_ptr); -tcg_out_movr(s, TCG_TYPE_I64, TCG_REG_X0, TCG_AREG0); -tcg_out_movr(s, TARGET_LONG_BITS == 64, TCG_REG_X1, lb->addrlo_reg); +tcg_out_mov(s, TCG_TYPE_I64, TCG_REG_X0, TCG_AREG0); +tcg_out_mov(s, TARGET_LONG_BITS == 64, TCG_REG_X1, lb->addrlo_reg); tcg_out_movi(s, TCG_TYPE_I32, TCG_REG_X2, lb->mem_index); tcg_out_adr(s, TCG_REG_X3, (intptr_t)lb->raddr); tcg_out_call(s, (intptr_t)qemu_ld_helpers[opc & ~MO_SIGN]); if (opc & MO_SIGN) { tcg_out_sxt(s, TCG_TYPE_I64, size, lb->datalo_reg, TCG_REG_X0); } else { -tcg_out_movr(s, TCG_TYPE_I64, lb->datalo_reg, TCG_REG_X0); +tcg_out_mov(s, size == MO_64, lb->datalo_reg, TCG_REG_X0); } tcg_out_goto(s, (intptr_t)lb->raddr); @@ -1046,9 +1044,9 @@ static void tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) reloc_pc19(lb->label_ptr[0], (intptr_t)s->code_ptr); -tcg_out_movr(s, TCG_TYPE_I64, TCG_REG_X0, TCG_AREG0); -tcg_out_movr(s, TARGET_LONG_BITS == 64, TCG_REG_X1, lb->addrlo_reg); -tcg_out_movr(s, size == MO_64, TCG_REG_X2, lb->datalo_reg); +tcg_out_mov(s, TCG_TYPE_I64, TCG_REG_X0, TCG_AREG0); +tcg_out_mov(s, TARGET_LONG_BITS == 64, TCG_REG_X1, lb->addrlo_reg); +tcg_out_mov(s, size == MO_64, TCG_REG_X2, lb->datalo_reg); tcg_out_movi(s, TCG_TYPE_I32, TCG_REG_X3, lb->mem_index); tcg_out_adr(s, TCG_REG_X4, (intptr_t)lb->raddr); tcg_out_call(s, (intptr_t)qemu_st_helpers[opc]); -- 1.9.0
[Qemu-devel] [PULL for-2.1 20/25] tcg-aarch64: Support stores of zero
Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 35 +++ 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index 34e477d..caaf8a2 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -1253,21 +1253,21 @@ static void tcg_out_qemu_st_direct(TCGContext *s, TCGMemOp memop, tcg_out_ldst_r(s, LDST_8, LDST_ST, data_r, addr_r, off_r); break; case MO_16: -if (bswap) { +if (bswap && data_r != TCG_REG_XZR) { tcg_out_rev16(s, TCG_TYPE_I32, TCG_REG_TMP, data_r); data_r = TCG_REG_TMP; } tcg_out_ldst_r(s, LDST_16, LDST_ST, data_r, addr_r, off_r); break; case MO_32: -if (bswap) { +if (bswap && data_r != TCG_REG_XZR) { tcg_out_rev(s, TCG_TYPE_I32, TCG_REG_TMP, data_r); data_r = TCG_REG_TMP; } tcg_out_ldst_r(s, LDST_32, LDST_ST, data_r, addr_r, off_r); break; case MO_64: -if (bswap) { +if (bswap && data_r != TCG_REG_XZR) { tcg_out_rev(s, TCG_TYPE_I64, TCG_REG_TMP, data_r); data_r = TCG_REG_TMP; } @@ -1364,8 +1364,6 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, case INDEX_op_ld_i32: case INDEX_op_ld_i64: -case INDEX_op_st_i32: -case INDEX_op_st_i64: case INDEX_op_ld8u_i32: case INDEX_op_ld8s_i32: case INDEX_op_ld16u_i32: @@ -1376,13 +1374,18 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, case INDEX_op_ld16s_i64: case INDEX_op_ld32u_i64: case INDEX_op_ld32s_i64: +tcg_out_ldst(s, aarch64_ldst_get_data(opc), aarch64_ldst_get_type(opc), + a0, a1, a2); +break; +case INDEX_op_st_i32: +case INDEX_op_st_i64: case INDEX_op_st8_i32: case INDEX_op_st8_i64: case INDEX_op_st16_i32: case INDEX_op_st16_i64: case INDEX_op_st32_i64: tcg_out_ldst(s, aarch64_ldst_get_data(opc), aarch64_ldst_get_type(opc), - a0, a1, a2); + REG0(0), a1, a2); break; case INDEX_op_add_i32: @@ -1585,7 +1588,7 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, break; case INDEX_op_qemu_st_i32: case INDEX_op_qemu_st_i64: -tcg_out_qemu_st(s, a0, a1, a2, args[3]); +tcg_out_qemu_st(s, REG0(0), a1, a2, args[3]); break; case INDEX_op_bswap32_i64: @@ -1693,13 +1696,13 @@ static const TCGTargetOpDef aarch64_op_defs[] = { { INDEX_op_ld32s_i64, { "r", "r" } }, { INDEX_op_ld_i64, { "r", "r" } }, -{ INDEX_op_st8_i32, { "r", "r" } }, -{ INDEX_op_st16_i32, { "r", "r" } }, -{ INDEX_op_st_i32, { "r", "r" } }, -{ INDEX_op_st8_i64, { "r", "r" } }, -{ INDEX_op_st16_i64, { "r", "r" } }, -{ INDEX_op_st32_i64, { "r", "r" } }, -{ INDEX_op_st_i64, { "r", "r" } }, +{ INDEX_op_st8_i32, { "rZ", "r" } }, +{ INDEX_op_st16_i32, { "rZ", "r" } }, +{ INDEX_op_st_i32, { "rZ", "r" } }, +{ INDEX_op_st8_i64, { "rZ", "r" } }, +{ INDEX_op_st16_i64, { "rZ", "r" } }, +{ INDEX_op_st32_i64, { "rZ", "r" } }, +{ INDEX_op_st_i64, { "rZ", "r" } }, { INDEX_op_add_i32, { "r", "r", "rwA" } }, { INDEX_op_add_i64, { "r", "r", "rA" } }, @@ -1753,8 +1756,8 @@ static const TCGTargetOpDef aarch64_op_defs[] = { { INDEX_op_qemu_ld_i32, { "r", "l" } }, { INDEX_op_qemu_ld_i64, { "r", "l" } }, -{ INDEX_op_qemu_st_i32, { "l", "l" } }, -{ INDEX_op_qemu_st_i64, { "l", "l" } }, +{ INDEX_op_qemu_st_i32, { "lZ", "l" } }, +{ INDEX_op_qemu_st_i64, { "lZ", "l" } }, { INDEX_op_bswap16_i32, { "r", "r" } }, { INDEX_op_bswap32_i32, { "r", "r" } }, -- 1.9.0
[Qemu-devel] [PULL for-2.1 16/25] tcg-aarch64: Use ADR to pass the return address to the ld/st helpers
Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index 4729d11..5d19e27 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -1070,6 +1070,13 @@ static const void * const qemu_st_helpers[4] = { helper_ret_stq_mmu, }; +static inline void tcg_out_adr(TCGContext *s, TCGReg rd, uintptr_t addr) +{ +addr -= (uintptr_t)s->code_ptr; +assert(addr == sextract64(addr, 0, 21)); +tcg_out_insn(s, 3406, ADR, rd, addr); +} + static void tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) { TCGMemOp opc = lb->opc; @@ -1080,7 +1087,7 @@ static void tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) tcg_out_movr(s, TCG_TYPE_I64, TCG_REG_X0, TCG_AREG0); tcg_out_movr(s, TARGET_LONG_BITS == 64, TCG_REG_X1, lb->addrlo_reg); tcg_out_movi(s, TCG_TYPE_I32, TCG_REG_X2, lb->mem_index); -tcg_out_movi(s, TCG_TYPE_PTR, TCG_REG_X3, (intptr_t)lb->raddr); +tcg_out_adr(s, TCG_REG_X3, (intptr_t)lb->raddr); tcg_out_call(s, (intptr_t)qemu_ld_helpers[size]); if (opc & MO_SIGN) { tcg_out_sxt(s, TCG_TYPE_I64, size, lb->datalo_reg, TCG_REG_X0); @@ -1101,7 +1108,7 @@ static void tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) tcg_out_movr(s, TARGET_LONG_BITS == 64, TCG_REG_X1, lb->addrlo_reg); tcg_out_movr(s, size == MO_64, TCG_REG_X2, lb->datalo_reg); tcg_out_movi(s, TCG_TYPE_I32, TCG_REG_X3, lb->mem_index); -tcg_out_movi(s, TCG_TYPE_PTR, TCG_REG_X4, (intptr_t)lb->raddr); +tcg_out_adr(s, TCG_REG_X4, (intptr_t)lb->raddr); tcg_out_call(s, (intptr_t)qemu_st_helpers[size]); tcg_out_goto(s, (intptr_t)lb->raddr); } -- 1.9.0
[Qemu-devel] [PULL for-2.1 23/25] tcg-aarch64: Introduce tcg_out_insn_3312, _3310, _3313
Replace aarch64_ldst_op_data with AArch64LdstType, as it wasn't encoded for the proper shift for the field and was confusing. Merge aarch64_ldst_op_data, AArch64LdstType, and a few stray opcode bits into a single I3312_* argument, eliminating some magic numbers from the helper functions. Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 176 --- 1 file changed, 89 insertions(+), 87 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index 7f72df5..3235824 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -242,19 +242,12 @@ static const enum aarch64_cond_code tcg_cond_to_aarch64[] = { [TCG_COND_LEU] = COND_LS, }; -/* opcodes for LDR / STR instructions with base + simm9 addressing */ -enum aarch64_ldst_op_data { /* size of the data moved */ -LDST_8 = 0x38, -LDST_16 = 0x78, -LDST_32 = 0xb8, -LDST_64 = 0xf8, -}; -enum aarch64_ldst_op_type { /* type of operation */ -LDST_ST = 0x0,/* store */ -LDST_LD = 0x4,/* load */ -LDST_LD_S_X = 0x8, /* load and sign-extend into Xt */ -LDST_LD_S_W = 0xc, /* load and sign-extend into Wt */ -}; +typedef enum { +LDST_ST = 0,/* store */ +LDST_LD = 1,/* load */ +LDST_LD_S_X = 2, /* load and sign-extend into Xt */ +LDST_LD_S_W = 3, /* load and sign-extend into Wt */ +} AArch64LdstType; /* We encode the format of the insn into the beginning of the name, so that we can have the preprocessor help "typecheck" the insn vs the output @@ -278,6 +271,28 @@ typedef enum { I3207_BLR = 0xd63f, I3207_RET = 0xd65f, +/* Load/store register. Described here as 3.3.12, but the helper + that emits them can transform to 3.3.10 or 3.3.13. */ +I3312_STRB = 0x3800 | LDST_ST << 22 | MO_8 << 30, +I3312_STRH = 0x3800 | LDST_ST << 22 | MO_16 << 30, +I3312_STRW = 0x3800 | LDST_ST << 22 | MO_32 << 30, +I3312_STRX = 0x3800 | LDST_ST << 22 | MO_64 << 30, + +I3312_LDRB = 0x3800 | LDST_LD << 22 | MO_8 << 30, +I3312_LDRH = 0x3800 | LDST_LD << 22 | MO_16 << 30, +I3312_LDRW = 0x3800 | LDST_LD << 22 | MO_32 << 30, +I3312_LDRX = 0x3800 | LDST_LD << 22 | MO_64 << 30, + +I3312_LDRSBW= 0x3800 | LDST_LD_S_W << 22 | MO_8 << 30, +I3312_LDRSHW= 0x3800 | LDST_LD_S_W << 22 | MO_16 << 30, + +I3312_LDRSBX= 0x3800 | LDST_LD_S_X << 22 | MO_8 << 30, +I3312_LDRSHX= 0x3800 | LDST_LD_S_X << 22 | MO_16 << 30, +I3312_LDRSWX= 0x3800 | LDST_LD_S_X << 22 | MO_32 << 30, + +I3312_TO_I3310 = 0x00206800, +I3312_TO_I3313 = 0x0100, + /* Load/store register pair instructions. */ I3314_LDP = 0x2840, I3314_STP = 0x2800, @@ -490,26 +505,25 @@ static void tcg_out_insn_3509(TCGContext *s, AArch64Insn insn, TCGType ext, tcg_out32(s, insn | ext << 31 | rm << 16 | ra << 10 | rn << 5 | rd); } +static void tcg_out_insn_3310(TCGContext *s, AArch64Insn insn, + TCGReg rd, TCGReg base, TCGReg regoff) +{ +/* Note the AArch64Insn constants above are for C3.3.12. Adjust. */ +tcg_out32(s, insn | I3312_TO_I3310 | regoff << 16 | base << 5 | rd); +} -static inline void tcg_out_ldst_9(TCGContext *s, - enum aarch64_ldst_op_data op_data, - enum aarch64_ldst_op_type op_type, - TCGReg rd, TCGReg rn, intptr_t offset) + +static void tcg_out_insn_3312(TCGContext *s, AArch64Insn insn, + TCGReg rd, TCGReg rn, intptr_t offset) { -/* use LDUR with BASE register with 9bit signed unscaled offset */ -tcg_out32(s, op_data << 24 | op_type << 20 - | (offset & 0x1ff) << 12 | rn << 5 | rd); +tcg_out32(s, insn | (offset & 0x1ff) << 12 | rn << 5 | rd); } -/* tcg_out_ldst_12 expects a scaled unsigned immediate offset */ -static inline void tcg_out_ldst_12(TCGContext *s, - enum aarch64_ldst_op_data op_data, - enum aarch64_ldst_op_type op_type, - TCGReg rd, TCGReg rn, - tcg_target_ulong scaled_uimm) +static void tcg_out_insn_3313(TCGContext *s, AArch64Insn insn, + TCGReg rd, TCGReg rn, uintptr_t scaled_uimm) { -tcg_out32(s, (op_data | 1) << 24 - | op_type << 20 | scaled_uimm << 10 | rn << 5 | rd); +/* Note the AArch64Insn constants above are for C3.3.12. Adjust. */ +tcg_out32(s, insn | I3312_TO_I3313 | scaled_uimm << 10 | rn << 5 | rd); } /* Register to register move using ORR (shifted register with no shift). */ @@ -647,44 +661,32 @@ static void tcg_out_movi(TCGContext *s, TCGType type, TCGReg rd, } } -static i
Re: [Qemu-devel] Change of TEXT_OFFSET for multi_v7_defconfig
On 04/15/2014 06:44 AM, Daniel Thompson wrote: > Hi Folks > > I've just been rebasing some of my development branches against v3.15rc1 > and observed some boot regressions due to TEXT_OFFSET changing from > 0x8000 to 0x208000. > > Now the boot regression turned out to be fault in the JTAG boot tools I > was using (it had internally hardcoded to TEXT_OFFSET to 0x8000 when > calculating what physical load address to use). I've fixed the JTAG > loader and my own boards now boots fine. Your tools are not alone in being affected by this change. QEMU is considering changing their hard-coded value to 0x8000 [1], which I was eager to see until being reminded of this (that patch would still be an improvement, but not enough for users of new multiplatform kernels). The boot-wrapper [2] (the default bootloader for ARM's proprietary models which could potentially be used on other systems) is also affected. > However this did get me looking at what had causes the offset to change. > I think that as some of the Qualcomm platforms have been converted to > multi-arch then, for the first time some older code in arch/arm/Kernel > gets enabled on the multi_v7 kernels: > > --- cut here --- > textofs-y := 0x8000 > textofs-$(CONFIG_ARCH_CLPS711X) := 0x00028000 > # We don't want the htc bootloader to corrupt kernel during resume > textofs-$(CONFIG_PM_H1940) := 0x00108000 > # SA DMA bug: we don't want the kernel to live in precious DMA-able > memory > ifeq ($(CONFIG_ARCH_SA1100),y) > textofs-$(CONFIG_SA) := 0x00208000 > endif > textofs-$(CONFIG_ARCH_MSM7X30) := 0x00208000 > textofs-$(CONFIG_ARCH_MSM8X60) := 0x00208000 > textofs-$(CONFIG_ARCH_MSM8960) := 0x00208000 > --- cut here --- > > It strikes me as a bit of a "bad smell" that enabling architecture > options in the multi-arch kernel causes the text offset to jump about > (not least because more than one platform might seek to change the text > offset). > > Do the ARCH_QCOM platforms (ARCH_MSM8X60 & ARCH_MSM8960) still require a > special text offset? [Stephen Boyd replied yes (3).] So, what do we do? My current thinking is that even if we temporarily removed variance (the jumping about) by maybe building every image with the maximum offset that any image could have, there would still be variance between images built before and after that change, and maybe also when some new platform gets added with an even higher offset. So if there's going to be variance, could we maybe make it no longer a problem? It seems to me that if external/uncompressed image loaders could query the text offset in a straightforward manner, variance between images could be easily dealt with. Would reading it out of ELF metadata be a reasonable mechanism? Could the ELF format vmlinux be a suitable general replacement for the raw Image? Now at least with my current .config, the vmlinux only has virtual addresses. Documentation/arm/Booting says the MMU has to be off at boot time so this still might not be the ideal input for image loaders. Tools could hard-code the phsyical-to-virtual offset instead of the TEXT_OFFSET. Is that less likely to vary? Or could we patch up the linker script to set zero-based ELF load memory addresses (LMAs) [4] so that the physical addresses are almost right, you just might have to add a system-specific RAM offset, perhaps pulled out of the device tree? If that won't work, we could generate some kind of vmlinux-phys with physical addresses. The latter two options might also simplify external debugging before __turn_mmu_on(). I like the sound of the LMA approach best, assuming it doesn't break existing stuff (I notice a few AT directives in vmlinux.lds.S). Some of this might transfer to arm64 as well. What do you all think? 1. http://lists.nongnu.org/archive/html/qemu-devel/2014-03/msg04715.html 2. http://linux-arm.org/git?p=boot-wrapper.git;a=summary 3. http://archive.arm.linux.org.uk/lurker/message/20140415.175318.5032983c.en.html 4. https://sourceware.org/binutils/docs/ld/Output-Section-LMA.html Thanks, Christopher -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation.
[Qemu-devel] [PULL for-2.1 09/25] tcg-aarch64: Create tcg_out_brcond
Rearrange code to put the compare and branch in the same place. Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 34 ++ 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index 8b15d3b..5889a98 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -865,18 +865,6 @@ static inline void tcg_out_goto_cond_noaddr(TCGContext *s, TCGCond c) tcg_out_insn(s, 3202, B_C, c, old); } -static inline void tcg_out_goto_cond(TCGContext *s, TCGCond c, intptr_t target) -{ -intptr_t offset = (target - (intptr_t)s->code_ptr) / 4; - -if (offset < -0x4 || offset >= 0x4) { -/* out of 19bit range */ -tcg_abort(); -} - -tcg_out_insn(s, 3202, B_C, c, offset); -} - static inline void tcg_out_callr(TCGContext *s, TCGReg reg) { tcg_out_insn(s, 3207, BLR, reg); @@ -920,17 +908,24 @@ static inline void tcg_out_goto_label(TCGContext *s, int label_index) } } -static inline void tcg_out_goto_label_cond(TCGContext *s, - TCGCond c, int label_index) +static void tcg_out_brcond(TCGContext *s, TCGMemOp ext, TCGCond c, TCGArg a, + TCGArg b, bool b_const, int label) { -TCGLabel *l = &s->labels[label_index]; +TCGLabel *l = &s->labels[label]; +intptr_t offset; + +tcg_out_cmp(s, ext, a, b, b_const); if (!l->has_value) { -tcg_out_reloc(s, s->code_ptr, R_AARCH64_CONDBR19, label_index, 0); -tcg_out_goto_cond_noaddr(s, c); +tcg_out_reloc(s, s->code_ptr, R_AARCH64_CONDBR19, label, 0); +offset = tcg_in32(s) >> 5; } else { -tcg_out_goto_cond(s, c, l->u.value); +offset = l->u.value - (uintptr_t)s->code_ptr; +offset >>= 2; +assert(offset >= -0x4 && offset < 0x4); } + +tcg_out_insn(s, 3202, B_C, c, offset); } static inline void tcg_out_rev(TCGContext *s, TCGType ext, @@ -1571,8 +1566,7 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, a1 = (int32_t)a1; /* FALLTHRU */ case INDEX_op_brcond_i64: -tcg_out_cmp(s, ext, a0, a1, const_args[1]); -tcg_out_goto_label_cond(s, a2, args[3]); +tcg_out_brcond(s, ext, a2, a0, a1, const_args[1], args[3]); break; case INDEX_op_setcond_i32: -- 1.9.0
[Qemu-devel] [PULL for-2.1 22/25] tcg-aarch64: Merge aarch64_ldst_get_data/type into tcg_out_op
Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 115 +-- 1 file changed, 32 insertions(+), 83 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index 0846835..7f72df5 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -356,78 +356,6 @@ typedef enum { I3510_ANDS = 0x6a00, } AArch64Insn; -static inline enum aarch64_ldst_op_data -aarch64_ldst_get_data(TCGOpcode tcg_op) -{ -switch (tcg_op) { -case INDEX_op_ld8u_i32: -case INDEX_op_ld8s_i32: -case INDEX_op_ld8u_i64: -case INDEX_op_ld8s_i64: -case INDEX_op_st8_i32: -case INDEX_op_st8_i64: -return LDST_8; - -case INDEX_op_ld16u_i32: -case INDEX_op_ld16s_i32: -case INDEX_op_ld16u_i64: -case INDEX_op_ld16s_i64: -case INDEX_op_st16_i32: -case INDEX_op_st16_i64: -return LDST_16; - -case INDEX_op_ld_i32: -case INDEX_op_st_i32: -case INDEX_op_ld32u_i64: -case INDEX_op_ld32s_i64: -case INDEX_op_st32_i64: -return LDST_32; - -case INDEX_op_ld_i64: -case INDEX_op_st_i64: -return LDST_64; - -default: -tcg_abort(); -} -} - -static inline enum aarch64_ldst_op_type -aarch64_ldst_get_type(TCGOpcode tcg_op) -{ -switch (tcg_op) { -case INDEX_op_st8_i32: -case INDEX_op_st16_i32: -case INDEX_op_st8_i64: -case INDEX_op_st16_i64: -case INDEX_op_st_i32: -case INDEX_op_st32_i64: -case INDEX_op_st_i64: -return LDST_ST; - -case INDEX_op_ld8u_i32: -case INDEX_op_ld16u_i32: -case INDEX_op_ld8u_i64: -case INDEX_op_ld16u_i64: -case INDEX_op_ld_i32: -case INDEX_op_ld32u_i64: -case INDEX_op_ld_i64: -return LDST_LD; - -case INDEX_op_ld8s_i32: -case INDEX_op_ld16s_i32: -return LDST_LD_S_W; - -case INDEX_op_ld8s_i64: -case INDEX_op_ld16s_i64: -case INDEX_op_ld32s_i64: -return LDST_LD_S_X; - -default: -tcg_abort(); -} -} - static inline uint32_t tcg_in32(TCGContext *s) { uint32_t v = *(uint32_t *)s->code_ptr; @@ -1372,30 +1300,51 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, tcg_out_goto_label(s, a0); break; -case INDEX_op_ld_i32: -case INDEX_op_ld_i64: case INDEX_op_ld8u_i32: -case INDEX_op_ld8s_i32: -case INDEX_op_ld16u_i32: -case INDEX_op_ld16s_i32: case INDEX_op_ld8u_i64: +tcg_out_ldst(s, LDST_8, LDST_LD, a0, a1, a2); +break; +case INDEX_op_ld8s_i32: +tcg_out_ldst(s, LDST_8, LDST_LD_S_W, a0, a1, a2); +break; case INDEX_op_ld8s_i64: +tcg_out_ldst(s, LDST_8, LDST_LD_S_X, a0, a1, a2); +break; +case INDEX_op_ld16u_i32: case INDEX_op_ld16u_i64: +tcg_out_ldst(s, LDST_16, LDST_LD, a0, a1, a2); +break; +case INDEX_op_ld16s_i32: +tcg_out_ldst(s, LDST_16, LDST_LD_S_W, a0, a1, a2); +break; case INDEX_op_ld16s_i64: +tcg_out_ldst(s, LDST_16, LDST_LD_S_X, a0, a1, a2); +break; +case INDEX_op_ld_i32: case INDEX_op_ld32u_i64: +tcg_out_ldst(s, LDST_32, LDST_LD, a0, a1, a2); +break; case INDEX_op_ld32s_i64: -tcg_out_ldst(s, aarch64_ldst_get_data(opc), aarch64_ldst_get_type(opc), - a0, a1, a2); +tcg_out_ldst(s, LDST_32, LDST_LD_S_X, a0, a1, a2); break; -case INDEX_op_st_i32: -case INDEX_op_st_i64: +case INDEX_op_ld_i64: +tcg_out_ldst(s, LDST_64, LDST_LD, a0, a1, a2); +break; + case INDEX_op_st8_i32: case INDEX_op_st8_i64: +tcg_out_ldst(s, LDST_8, LDST_ST, REG0(0), a1, a2); +break; case INDEX_op_st16_i32: case INDEX_op_st16_i64: +tcg_out_ldst(s, LDST_16, LDST_ST, REG0(0), a1, a2); +break; +case INDEX_op_st_i32: case INDEX_op_st32_i64: -tcg_out_ldst(s, aarch64_ldst_get_data(opc), aarch64_ldst_get_type(opc), - REG0(0), a1, a2); +tcg_out_ldst(s, LDST_32, LDST_ST, REG0(0), a1, a2); +break; +case INDEX_op_st_i64: +tcg_out_ldst(s, LDST_64, LDST_ST, REG0(0), a1, a2); break; case INDEX_op_add_i32: -- 1.9.0
[Qemu-devel] [PULL for-2.1 15/25] tcg-aarch64: Use tcg_out_call for qemu_ld/st
In some cases, a direct branch will be in range. Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index 5186311..4729d11 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -1081,8 +1081,7 @@ static void tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) tcg_out_movr(s, TARGET_LONG_BITS == 64, TCG_REG_X1, lb->addrlo_reg); tcg_out_movi(s, TCG_TYPE_I32, TCG_REG_X2, lb->mem_index); tcg_out_movi(s, TCG_TYPE_PTR, TCG_REG_X3, (intptr_t)lb->raddr); -tcg_out_movi(s, TCG_TYPE_I64, TCG_REG_TMP, (intptr_t)qemu_ld_helpers[size]); -tcg_out_callr(s, TCG_REG_TMP); +tcg_out_call(s, (intptr_t)qemu_ld_helpers[size]); if (opc & MO_SIGN) { tcg_out_sxt(s, TCG_TYPE_I64, size, lb->datalo_reg, TCG_REG_X0); } else { @@ -1103,8 +1102,7 @@ static void tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) tcg_out_movr(s, size == MO_64, TCG_REG_X2, lb->datalo_reg); tcg_out_movi(s, TCG_TYPE_I32, TCG_REG_X3, lb->mem_index); tcg_out_movi(s, TCG_TYPE_PTR, TCG_REG_X4, (intptr_t)lb->raddr); -tcg_out_movi(s, TCG_TYPE_I64, TCG_REG_TMP, (intptr_t)qemu_st_helpers[size]); -tcg_out_callr(s, TCG_REG_TMP); +tcg_out_call(s, (intptr_t)qemu_st_helpers[size]); tcg_out_goto(s, (intptr_t)lb->raddr); } -- 1.9.0
[Qemu-devel] [PULL for-2.1 14/25] tcg-aarch64: Avoid add with zero in tlb load
Some guest env are small enough to reach the tlb with only a 12-bit addition. Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 28 +++- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index 4414bd1..5186311 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -1128,47 +1128,57 @@ static void add_qemu_ldst_label(TCGContext *s, int is_ld, int opc, slow path for the failure case, which will be patched later when finalizing the slow path. Generated code returns the host addend in X1, clobbers X0,X2,X3,TMP. */ -static void tcg_out_tlb_read(TCGContext *s, TCGReg addr_reg, -int s_bits, uint8_t **label_ptr, int mem_index, int is_read) +static void tcg_out_tlb_read(TCGContext *s, TCGReg addr_reg, int s_bits, + uint8_t **label_ptr, int mem_index, bool is_read) { TCGReg base = TCG_AREG0; int tlb_offset = is_read ? offsetof(CPUArchState, tlb_table[mem_index][0].addr_read) : offsetof(CPUArchState, tlb_table[mem_index][0].addr_write); + /* Extract the TLB index from the address into X0. X0 = addr_reg */ -tcg_out_ubfm(s, (TARGET_LONG_BITS == 64), TCG_REG_X0, addr_reg, +tcg_out_ubfm(s, TARGET_LONG_BITS == 64, TCG_REG_X0, addr_reg, TARGET_PAGE_BITS, TARGET_PAGE_BITS + CPU_TLB_BITS); + /* Store the page mask part of the address and the low s_bits into X3. Later this allows checking for equality and alignment at the same time. X3 = addr_reg & (PAGE_MASK | ((1 << s_bits) - 1)) */ tcg_out_logicali(s, I3404_ANDI, TARGET_LONG_BITS == 64, TCG_REG_X3, addr_reg, TARGET_PAGE_MASK | ((1 << s_bits) - 1)); + /* Add any "high bits" from the tlb offset to the env address into X2, to take advantage of the LSL12 form of the ADDI instruction. X2 = env + (tlb_offset & 0xfff000) */ -tcg_out_insn(s, 3401, ADDI, TCG_TYPE_I64, TCG_REG_X2, base, - tlb_offset & 0xfff000); +if (tlb_offset & 0xfff000) { +tcg_out_insn(s, 3401, ADDI, TCG_TYPE_I64, TCG_REG_X2, base, + tlb_offset & 0xfff000); +base = TCG_REG_X2; +} + /* Merge the tlb index contribution into X2. X2 = X2 + (X0 << CPU_TLB_ENTRY_BITS) */ -tcg_out_insn(s, 3502S, ADD_LSL, 1, TCG_REG_X2, TCG_REG_X2, +tcg_out_insn(s, 3502S, ADD_LSL, TCG_TYPE_I64, TCG_REG_X2, base, TCG_REG_X0, CPU_TLB_ENTRY_BITS); + /* Merge "low bits" from tlb offset, load the tlb comparator into X0. X0 = load [X2 + (tlb_offset & 0x000fff)] */ tcg_out_ldst(s, TARGET_LONG_BITS == 64 ? LDST_64 : LDST_32, - LDST_LD, TCG_REG_X0, TCG_REG_X2, - (tlb_offset & 0xfff)); + LDST_LD, TCG_REG_X0, TCG_REG_X2, tlb_offset & 0xfff); + /* Load the tlb addend. Do that early to avoid stalling. X1 = load [X2 + (tlb_offset & 0xfff) + offsetof(addend)] */ tcg_out_ldst(s, LDST_64, LDST_LD, TCG_REG_X1, TCG_REG_X2, (tlb_offset & 0xfff) + (offsetof(CPUTLBEntry, addend)) - (is_read ? offsetof(CPUTLBEntry, addr_read) : offsetof(CPUTLBEntry, addr_write))); + /* Perform the address comparison. */ tcg_out_cmp(s, (TARGET_LONG_BITS == 64), TCG_REG_X0, TCG_REG_X3, 0); -*label_ptr = s->code_ptr; + /* If not equal, we jump to the slow path. */ +*label_ptr = s->code_ptr; tcg_out_goto_cond_noaddr(s, TCG_COND_NE); } -- 1.9.0
[Qemu-devel] [PULL for-2.1 17/25] tcg-aarch64: Use TCGMemOp in qemu_ld/st
Making the bswap conditional on the memop instead of a compile-time test. Reviewed-by: Claudio Fontana Signed-off-by: Richard Henderson --- tcg/aarch64/tcg-target.c | 131 +++ 1 file changed, 63 insertions(+), 68 deletions(-) diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c index 5d19e27..68305ea 100644 --- a/tcg/aarch64/tcg-target.c +++ b/tcg/aarch64/tcg-target.c @@ -27,12 +27,6 @@ static const char * const tcg_target_reg_names[TCG_TARGET_NB_REGS] = { }; #endif /* NDEBUG */ -#ifdef TARGET_WORDS_BIGENDIAN - #define TCG_LDST_BSWAP 1 -#else - #define TCG_LDST_BSWAP 0 -#endif - static const int tcg_target_reg_alloc_order[] = { TCG_REG_X20, TCG_REG_X21, TCG_REG_X22, TCG_REG_X23, TCG_REG_X24, TCG_REG_X25, TCG_REG_X26, TCG_REG_X27, @@ -1113,7 +1107,7 @@ static void tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *lb) tcg_out_goto(s, (intptr_t)lb->raddr); } -static void add_qemu_ldst_label(TCGContext *s, int is_ld, int opc, +static void add_qemu_ldst_label(TCGContext *s, int is_ld, TCGMemOp opc, TCGReg data_reg, TCGReg addr_reg, int mem_index, uint8_t *raddr, uint8_t *label_ptr) @@ -1133,7 +1127,7 @@ static void add_qemu_ldst_label(TCGContext *s, int is_ld, int opc, slow path for the failure case, which will be patched later when finalizing the slow path. Generated code returns the host addend in X1, clobbers X0,X2,X3,TMP. */ -static void tcg_out_tlb_read(TCGContext *s, TCGReg addr_reg, int s_bits, +static void tcg_out_tlb_read(TCGContext *s, TCGReg addr_reg, TCGMemOp s_bits, uint8_t **label_ptr, int mem_index, bool is_read) { TCGReg base = TCG_AREG0; @@ -1189,24 +1183,26 @@ static void tcg_out_tlb_read(TCGContext *s, TCGReg addr_reg, int s_bits, #endif /* CONFIG_SOFTMMU */ -static void tcg_out_qemu_ld_direct(TCGContext *s, int opc, TCGReg data_r, - TCGReg addr_r, TCGReg off_r) +static void tcg_out_qemu_ld_direct(TCGContext *s, TCGMemOp memop, + TCGReg data_r, TCGReg addr_r, TCGReg off_r) { -switch (opc) { -case 0: +const TCGMemOp bswap = memop & MO_BSWAP; + +switch (memop & MO_SSIZE) { +case MO_UB: tcg_out_ldst_r(s, LDST_8, LDST_LD, data_r, addr_r, off_r); break; -case 0 | 4: +case MO_SB: tcg_out_ldst_r(s, LDST_8, LDST_LD_S_X, data_r, addr_r, off_r); break; -case 1: +case MO_UW: tcg_out_ldst_r(s, LDST_16, LDST_LD, data_r, addr_r, off_r); -if (TCG_LDST_BSWAP) { +if (bswap) { tcg_out_rev16(s, TCG_TYPE_I32, data_r, data_r); } break; -case 1 | 4: -if (TCG_LDST_BSWAP) { +case MO_SW: +if (bswap) { tcg_out_ldst_r(s, LDST_16, LDST_LD, data_r, addr_r, off_r); tcg_out_rev16(s, TCG_TYPE_I32, data_r, data_r); tcg_out_sxt(s, TCG_TYPE_I64, MO_16, data_r, data_r); @@ -1214,14 +1210,14 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, int opc, TCGReg data_r, tcg_out_ldst_r(s, LDST_16, LDST_LD_S_X, data_r, addr_r, off_r); } break; -case 2: +case MO_UL: tcg_out_ldst_r(s, LDST_32, LDST_LD, data_r, addr_r, off_r); -if (TCG_LDST_BSWAP) { +if (bswap) { tcg_out_rev(s, TCG_TYPE_I32, data_r, data_r); } break; -case 2 | 4: -if (TCG_LDST_BSWAP) { +case MO_SL: +if (bswap) { tcg_out_ldst_r(s, LDST_32, LDST_LD, data_r, addr_r, off_r); tcg_out_rev(s, TCG_TYPE_I32, data_r, data_r); tcg_out_sxt(s, TCG_TYPE_I64, MO_32, data_r, data_r); @@ -1229,9 +1225,9 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, int opc, TCGReg data_r, tcg_out_ldst_r(s, LDST_32, LDST_LD_S_X, data_r, addr_r, off_r); } break; -case 3: +case MO_Q: tcg_out_ldst_r(s, LDST_64, LDST_LD, data_r, addr_r, off_r); -if (TCG_LDST_BSWAP) { +if (bswap) { tcg_out_rev(s, TCG_TYPE_I64, data_r, data_r); } break; @@ -1240,47 +1236,47 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, int opc, TCGReg data_r, } } -static void tcg_out_qemu_st_direct(TCGContext *s, int opc, TCGReg data_r, - TCGReg addr_r, TCGReg off_r) +static void tcg_out_qemu_st_direct(TCGContext *s, TCGMemOp memop, + TCGReg data_r, TCGReg addr_r, TCGReg off_r) { -switch (opc) { -case 0: +const TCGMemOp bswap = memop & MO_BSWAP; + +switch (memop & MO_SIZE) { +case MO_8: tcg_out_ldst_r(s, LDST_8, LDST_ST, data_r, addr_r, off_r); break; -case 1: -if (TCG_LDST_BSWAP) { +case MO_16: +if (bswap) { tcg_out_rev16(s, TCG_