Re: [PATCH 04/38] target/riscv: 16-bit Addition & Subtraction Instructions
On 2021/2/13 2:03, Richard Henderson wrote:
On 2/12/21 7:02 AM, LIU Zhiwei wrote:
+if (a->rd && a->rs1 && a->rs2) {
+#ifdef TARGET_RISCV64
+f64(vece, offsetof(CPURISCVState, gpr[a->rd]),
+offsetof(CPURISCVState, gpr[a->rs1]),
+offsetof(CPURISCVState, gpr[a->rs2]),
+8, 8);
+#else
This is not legal tcg.
You cannot reference as memory anything which has an associated tcg_global_mem.
Thanks.
Do you mean referring a global TCGTemp as memory will cause not
consistent between TCGContext::temps and
CPUArchState field?
Zhiwei
Which is true for all of the gprs -- see riscv_translate_init.
r~
Re: [PATCH 04/38] target/riscv: 16-bit Addition & Subtraction Instructions
On 2021/2/13 3:02, Richard Henderson wrote:
On 2/12/21 7:02 AM, LIU Zhiwei wrote:
+static void tcg_gen_simd_add16(TCGv d, TCGv a, TCGv b)
+{
+TCGv t1 = tcg_temp_new();
+TCGv t2 = tcg_temp_new();
+
+tcg_gen_andi_tl(t1, a, ~0x);
+tcg_gen_add_tl(t2, a, b);
+tcg_gen_add_tl(t1, t1, b);
+tcg_gen_deposit_tl(d, t1, t2, 0, 16);
+
+tcg_temp_free(t1);
+tcg_temp_free(t2);
+}
I will note that there are some helper functions, e.g. tcg_gen_vec_add16_i64
(see the end of include/tcg/tcg-op-gvec.h), but those are explicitly i64, and
you'll still need these for rv32.
Hi Richard,
Yes, that's really what I need.
Do you mind continue to review the other patches in v1? Or should I
send a v2 to fix current error at first?
Zhiwei
r~
Re: [PATCH] qsd: Document FUSE exports
On 17.02.21 17:26, Eric Blake wrote: On 2/17/21 5:58 AM, Max Reitz wrote: Implementing FUSE exports required no changes to the storage daemon, so we forgot to document them there. Considering that both NBD and vhost-user-blk exports are documented in its man page (and NBD exports in its --help text), we should probably do the same for FUSE. Signed-off-by: Max Reitz --- docs/tools/qemu-storage-daemon.rst | 19 +++ storage-daemon/qemu-storage-daemon.c | 4 2 files changed, 23 insertions(+) @@ -142,6 +153,14 @@ domain socket ``vhost-user-blk.sock``:: --blockdev driver=qcow2,node-name=qcow2,file=file \ --export type=vhost-user-blk,id=export,addr.type=unix,addr.path=vhost-user-blk.sock,node-name=qcow2 +Export a qcow2 image file ``disk.qcow2`` via FUSE on itself, so the disk image +file will then appear as a raw image:: + + $ qemu-storage-daemon \ + --blockdev driver=file,node-name=file,filename=disk.qcow2 \ + --blockdev driver=qcow2,node-name=qcow2,file=file \ + --export type=fuse,id=export,node-name=qcow2,mountpoint=disk.qcow2,writable=on + Should the example also mention how to unmount the file when you're done? Just as with other exports, the export goes away when it is deleted, which happens e.g. when the QSD exits. I.e., fuse_export_delete() calls fuse_session_unmount(). Otherwise looks good to me. Any documentation is better than none, even if we can add more, so Reviewed-by: Eric Blake Thanks! Max
Re: [PULL 09/19] libqos/qgraph: add qos_node_create_driver_named()
On Montag, 15. Februar 2021 15:06:41 CET Christian Schoenebeck wrote: > On Montag, 15. Februar 2021 14:16:16 CET Paolo Bonzini wrote: > > From: qemu_oss--- via > > > > So far the qos subsystem of the qtest framework had the limitation > > that only one instance of the same official QEMU (QMP) driver name > > could be created for qtests. That's because a) the created qos > > node names must always be unique, b) the node name must match the > > official QEMU driver name being instantiated and c) all nodes are > > in a global space shared by all tests. > > > > This patch removes this limitation by introducing a new function > > qos_node_create_driver_named() which allows test case authors to > > specify a node name being different from the actual associated > > QEMU driver name. It fills the new 'qemu_name' field of > > QOSGraphNode for that purpose. > > > > Adjust build_driver_cmd_line() and qos_graph_node_set_availability() > > to correctly deal with either accessing node name vs. node's > > qemu_name correctly. > > > > Signed-off-by: Christian Schoenebeck > > Message-Id: > > <3be962ff38f3396f8040deaa5ffdab525c4e0b16.1611704181.git.qemu_oss@crudebyt > > e > > .com> Signed-off-by: Paolo Bonzini > > Just a side note: The odd "From:" line was because of a temporary issue with > the mailman version running GNU lists, which caused mailman to rewrite > certain sender addresses. The problem with mailman had been fixed in the > meantime. > > I personally don't care about it, but just that you know that this did not > happen by purpose or something. Paolo, do you want me to resend these patches as v2 for fixing the author rewrite issue? Best regards, Christian Schoenebeck
Re: [PULL 09/19] libqos/qgraph: add qos_node_create_driver_named()
On 18/02/21 10:10, Christian Schoenebeck wrote: On Montag, 15. Februar 2021 15:06:41 CET Christian Schoenebeck wrote: On Montag, 15. Februar 2021 14:16:16 CET Paolo Bonzini wrote: From: qemu_oss--- via So far the qos subsystem of the qtest framework had the limitation that only one instance of the same official QEMU (QMP) driver name could be created for qtests. That's because a) the created qos node names must always be unique, b) the node name must match the official QEMU driver name being instantiated and c) all nodes are in a global space shared by all tests. This patch removes this limitation by introducing a new function qos_node_create_driver_named() which allows test case authors to specify a node name being different from the actual associated QEMU driver name. It fills the new 'qemu_name' field of QOSGraphNode for that purpose. Adjust build_driver_cmd_line() and qos_graph_node_set_availability() to correctly deal with either accessing node name vs. node's qemu_name correctly. Signed-off-by: Christian Schoenebeck Message-Id: <3be962ff38f3396f8040deaa5ffdab525c4e0b16.1611704181.git.qemu_oss@crudebyt e .com> Signed-off-by: Paolo Bonzini Just a side note: The odd "From:" line was because of a temporary issue with the mailman version running GNU lists, which caused mailman to rewrite certain sender addresses. The problem with mailman had been fixed in the meantime. I personally don't care about it, but just that you know that this did not happen by purpose or something. Paolo, do you want me to resend these patches as v2 for fixing the author rewrite issue? The patches already in and with the right author. Paolo
Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw
On 18/02/2021 03.22, Halil Pasic wrote:
Since the virtio-gpu-ccw device depends on the hw-display-virtio-gpu
module, which provides the type virtio-gpu-device, packaging the
hw-display-virtio-gpu module as a separate package that may or may not
be installed along with the qemu package leads to problems. Namely if
the hw-display-virtio-gpu is absent, qemu continues to advertise
virtio-gpu-ccw, but it aborts not only when one attempts using
virtio-gpu-ccw, but also when libvirtd's capability probing tries
to instantiate the type to introspect it.
Let us thus introduce a module named hw-s390x-virtio-gpu-ccw that
is going to provide the virtio-gpu-ccw device. The hw-s390x prefix
was chosen because it is not a portable device.
With virtio-gpu-ccw built as a module, the correct way to package a
modularized qemu is to require that hw-display-virtio-gpu must be
installed whenever the module hw-s390x-virtio-gpu-ccw.
Signed-off-by: Halil Pasic
---
hw/s390x/meson.build | 17 -
util/module.c| 1 +
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index 2a7818d94b..153b1309fb 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -34,7 +34,6 @@ virtio_ss.add(files('virtio-ccw.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_BALLOON', if_true:
files('virtio-ccw-balloon.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-ccw-blk.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true:
files('virtio-ccw-crypto.c'))
-virtio_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true: files('virtio-ccw-gpu.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true:
files('virtio-ccw-input.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio-ccw-net.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-ccw-rng.c'))
@@ -46,3 +45,19 @@ virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true:
files('vhost-user-fs-ccw.c'
s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
hw_arch += {'s390x': s390x_ss}
+
+if target.startswith('s390x')
+ hw_s390x_modules = {}
+
+ hw_s390x_modules_c_args = ['-DNEED_CPU_H',
+ '-DCONFIG_TARGET="@0@-config-target.h"'.format(target)]
+ hw_s390x_modules_inc = [include_directories('../../target' /
config_target['TARGET_BASE_ARCH'])]
+ hw_s390x_modules_dependencies = declare_dependency(
+ include_directories: hw_s390x_modules_inc, compile_args:
hw_s390x_modules_c_args)
Basically the patch looks fine to me, but I wonder why all that above lines
(related to hw_s390x_modules_dependencies) are requred at all? The other
display modules in hw/display/meson.build also do not need to re-define
c_args for example?
Thomas
+ virtio_gpu_ccw_ss = ss.source_set()
+ virtio_gpu_ccw_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true:
[files('virtio-ccw-gpu.c'), pixman, hw_s390x_modules_dependencies])
+ hw_s390x_modules += {'virtio-gpu-ccw': virtio_gpu_ccw_ss}
+
+ modules += {'hw-s390x': hw_s390x_modules}
+endif
diff --git a/util/module.c b/util/module.c
index c65060c167..cbe89fede6 100644
--- a/util/module.c
+++ b/util/module.c
@@ -304,6 +304,7 @@ static struct {
{ "virtio-gpu-pci-base", "hw-", "display-virtio-gpu-pci" },
{ "virtio-gpu-pci","hw-", "display-virtio-gpu-pci" },
{ "vhost-user-gpu-pci","hw-", "display-virtio-gpu-pci" },
+{ "virtio-gpu-ccw","hw-", "s390x-virtio-gpu-ccw" },
{ "virtio-vga-base", "hw-", "display-virtio-vga"},
{ "virtio-vga","hw-", "display-virtio-vga"},
{ "vhost-user-vga","hw-", "display-virtio-vga"},
base-commit: 1af5629673bb5c1592d993f9fb6119a62845f576
Re: [PULL 09/19] libqos/qgraph: add qos_node_create_driver_named()
On Donnerstag, 18. Februar 2021 10:14:12 CET Paolo Bonzini wrote: > On 18/02/21 10:10, Christian Schoenebeck wrote: > > On Montag, 15. Februar 2021 15:06:41 CET Christian Schoenebeck wrote: > >> On Montag, 15. Februar 2021 14:16:16 CET Paolo Bonzini wrote: > >>> From: qemu_oss--- via > >>> > >>> So far the qos subsystem of the qtest framework had the limitation > >>> that only one instance of the same official QEMU (QMP) driver name > >>> could be created for qtests. That's because a) the created qos > >>> node names must always be unique, b) the node name must match the > >>> official QEMU driver name being instantiated and c) all nodes are > >>> in a global space shared by all tests. > >>> > >>> This patch removes this limitation by introducing a new function > >>> qos_node_create_driver_named() which allows test case authors to > >>> specify a node name being different from the actual associated > >>> QEMU driver name. It fills the new 'qemu_name' field of > >>> QOSGraphNode for that purpose. > >>> > >>> Adjust build_driver_cmd_line() and qos_graph_node_set_availability() > >>> to correctly deal with either accessing node name vs. node's > >>> qemu_name correctly. > >>> > >>> Signed-off-by: Christian Schoenebeck > >>> Message-Id: > >>> <3be962ff38f3396f8040deaa5ffdab525c4e0b16.1611704181.git.qemu_oss@crudeb > >>> yt > >>> e > >>> .com> Signed-off-by: Paolo Bonzini > >> > >> Just a side note: The odd "From:" line was because of a temporary issue > >> with the mailman version running GNU lists, which caused mailman to > >> rewrite certain sender addresses. The problem with mailman had been > >> fixed in the meantime. > >> > >> I personally don't care about it, but just that you know that this did > >> not > >> happen by purpose or something. > > > > Paolo, do you want me to resend these patches as v2 for fixing the author > > rewrite issue? > > The patches already in and with the right author. > > Paolo Right, I just noticed that. Sorry Paolo for the trouble. :/ Best regards, Christian Schoenebeck
Re: [PATCH 0/3] virtio-net: graceful drop of vhost for TAP
On Tue, Feb 09, 2021 at 10:04:30AM -0500, Michael S. Tsirkin wrote: > On Tue, Feb 09, 2021 at 02:51:05PM +, Daniel P. Berrangé wrote: > > On Tue, Feb 09, 2021 at 09:34:20AM -0500, Michael S. Tsirkin wrote: > > > On Thu, Feb 04, 2021 at 10:29:12PM +0200, Yuri Benditovich wrote: > > > > This set of patches introduces graceful switch from tap-vhost to > > > > tap-no-vhost depending on guest features. Before that the features > > > > that vhost does not support were silently cleared in get_features. > > > > This creates potential problem of migration from the machine where > > > > some of virtio-net features are supported by the vhost kernel to the > > > > machine where they are not supported (packed ring as an example). > > > > > > I still worry that adding new features will silently disable vhost for > > > people. > > > Can we limit the change to when a VM is migrated in? > > > > Some management applications expect bi-directional live migration to > > work, so taking specific actions on incoming migration only feels > > dangerous. > > Could you be more specific? > > Bi-directional migration is currently broken > when migrating new kernel->old kernel. > > This seems to be the motivation for this patch, though I wish > it was spelled out more explicitly. > > People don't complain much, but I'm fine with fixing that > with a userspace fallback. > > > I'd rather not force the fallback on others though: vhost is generally > specified explicitly by user while features are generally set > automatically, so this patch will make us override what user specified, > not nice. > > > > IMHO if the features we're adding cannot be expected to exist in > > host kernels in general, then the feature should defualt to off > > and require explicit user config to enable. > > Downstream distros which can guarantee newer kernels can flip the > > default in their custom machine types if they desire. > > > > Regards, > > Daniel > > Unfortunately that will basically mean we are stuck with no new features > for years. We did what this patch is trying to change for years now, in > particular KVM also seems to happily disable CPU features not supported > by kernel so I wonder why we can't keep doing it, with tweaks for some > corner cases. I should say the kernel's continual changing in CPU features that are exposed has been responsible for a *huge* number of bugs with live migration compatibility. libvirt, QEMU & apps have needed to introduce a lot of extra code to try to cope with the changing CPU features across migration and i still goes wrong to this very day, because we have to migrate from prehistoric QEMU versions to quite modern versions. IOW, the CPU features approach is a perfect example of why we should *not* introduce a kernel dependancy in more areas of QEMU feature enablement, and instead should strictly tie feature defaults to the machine type versions. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 0/3] virtio-net: graceful drop of vhost for TAP
On Wed, Feb 10, 2021 at 02:19:59PM +0800, Jason Wang wrote: > > On 2021/2/9 下午11:04, Michael S. Tsirkin wrote: > > On Tue, Feb 09, 2021 at 02:51:05PM +, Daniel P. Berrangé wrote: > > > On Tue, Feb 09, 2021 at 09:34:20AM -0500, Michael S. Tsirkin wrote: > > > > On Thu, Feb 04, 2021 at 10:29:12PM +0200, Yuri Benditovich wrote: > > > > > This set of patches introduces graceful switch from tap-vhost to > > > > > tap-no-vhost depending on guest features. Before that the features > > > > > that vhost does not support were silently cleared in get_features. > > > > > This creates potential problem of migration from the machine where > > > > > some of virtio-net features are supported by the vhost kernel to the > > > > > machine where they are not supported (packed ring as an example). > > > > I still worry that adding new features will silently disable vhost for > > > > people. > > > > Can we limit the change to when a VM is migrated in? > > > Some management applications expect bi-directional live migration to > > > work, so taking specific actions on incoming migration only feels > > > dangerous. > > Could you be more specific? > > > > Bi-directional migration is currently broken > > when migrating new kernel->old kernel. > > > > This seems to be the motivation for this patch, though I wish > > it was spelled out more explicitly. > > > > People don't complain much, but I'm fine with fixing that > > with a userspace fallback. > > > > > > I'd rather not force the fallback on others though: vhost is generally > > specified explicitly by user while features are generally set > > automatically, so this patch will make us override what user specified, > > not nice. > > > > > > > IMHO if the features we're adding cannot be expected to exist in > > > host kernels in general, then the feature should defualt to off > > > and require explicit user config to enable. > > > Downstream distros which can guarantee newer kernels can flip the > > > default in their custom machine types if they desire. > > > > > > Regards, > > > Daniel > > Unfortunately that will basically mean we are stuck with no new features > > for years. We did what this patch is trying to change for years now, in > > particular KVM also seems to happily disable CPU features not supported > > by kernel so I wonder why we can't keep doing it, with tweaks for some > > corner cases. > > > It's probably not the corner case. > > So my understanding is when a feature is turned on via command line, it > should not be cleared silently otherwise we may break migration for sure. > > E.g when packed=on is specified, we should disable vhost instead of clear it > from the device. If something is explicitly turned on by the user, they expect that feature to be honoured, or an error to be raised. If something is not explicitly turned on by the user, the behaviour wrt the default should be stable for any given machine type version. IOW, if you disable vhost by default when packed=on is set, then you can't later switch to letting vhost be enabled with packed=on, unless you tie that change to a new machine type. If the user has explicitly said packed=on *and* vhost=on, then should must honour that, or raise an error if the combination is unsupportable. Silently disabling vhost, then vhost=on is not ok. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v2 7/7] tests/avocado: add boot_xen tests
On 2/17/21 9:46 PM, Cleber Rosa wrote:
> On Thu, Feb 11, 2021 at 05:19:45PM +, Alex Bennée wrote:
>> These tests make sure we can boot the Xen hypervisor with a Dom0
>> kernel using the guest-loader. We currently have to use a kernel I
>> built myself because there are issues using the Debian kernel images.
>>
>> Signed-off-by: Alex Bennée
>> ---
>> MAINTAINERS | 1 +
>> tests/acceptance/boot_xen.py | 117 +++
>> 2 files changed, 118 insertions(+)
>> create mode 100644 tests/acceptance/boot_xen.py
>> +class BootXen(BootXenBase):
>> +
>> +@skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
>> +def test_arm64_xen_411_and_dom0(self):
>> +"""
>> +:avocado: tags=arch:aarch64
>> +:avocado: tags=accel:tcg
>> +:avocado: tags=cpu:cortex-a57
>> +:avocado: tags=machine:virt
>> +"""
>> +xen_url = ('https://deb.debian.org/debian/'
>> + 'pool/main/x/xen/'
>> +
>> 'xen-hypervisor-4.11-arm64_4.11.4+37-g3263f257ca-1_arm64.deb')
>> +xen_sha1 = '034e634d4416adbad1212d59b62bccdcda63e62a'
>
> This URL is already giving 404s because of a new pacakge. I found
> this to work (but yeah, won't probably last long):
>
> xen_url = ('http://deb.debian.org/debian/'
>'pool/main/x/xen/'
>
> 'xen-hypervisor-4.11-arm64_4.11.4+57-g41a822c392-2_arm64.deb')
> xen_sha1 = 'b5a6810fc67fd50fa36afdfdfe88ce3153dd3a55'
This is not the same package version... Please understand the developer
has to download the Debian package sources, check again the set of
downstream changes between 37 and 57. Each distrib number might contain
multiple downstream patches. Then the testing has to be done again,
often enabling tracing or doing single-stepping in gdb. This has a
cost in productivity. This is why I insist I prefer to use archived
well tested artifacts, rather than changing package URL randomly.
>> +xen_deb = self.fetch_asset(xen_url, asset_hash=xen_sha1)
>> +xen_path = self.extract_from_deb(xen_deb, "/boot/xen-4.11-arm64")
>> +
>> +self.launch_xen(xen_path)
>> +
>> +@skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
>> +def test_arm64_xen_414_and_dom0(self):
>> +"""
>> +:avocado: tags=arch:aarch64
>> +:avocado: tags=accel:tcg
>> +:avocado: tags=cpu:cortex-a57
>> +:avocado: tags=machine:virt
>> +"""
>> +xen_url = ('https://deb.debian.org/debian/'
>> + 'pool/main/x/xen/'
>> +
>> 'xen-hypervisor-4.14-arm64_4.14.0+80-gd101b417b7-1_arm64.deb')
>> +xen_sha1 = 'b9d209dd689ed2b393e625303a225badefec1160'
>
> Likewise here:
>
> xen_url = ('https://deb.debian.org/debian/'
>'pool/main/x/xen/'
>
> 'xen-hypervisor-4.14-arm64_4.14.0+88-g1d1d1f5391-2_arm64.deb')
> xen_sha1 = 'f316049beaadd50482644e4955c4cdd63e3a07d5'
Likewise not the same package version.
Regards,
Phil.
[PULL 01/23] hw/virtio/pci: include vdev name in registered PCI sections
When viewing/debugging memory regions it is sometimes hard to figure
out which PCI device something belongs to. Make the names unique by
including the vdev name in the name string.
Signed-off-by: Alex Bennée
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Michael S. Tsirkin
Message-Id: <20210213130325.14781-2-alex.ben...@linaro.org>
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 094c36aa3e..883045a223 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1423,7 +1423,8 @@ static void virtio_pci_device_write(void *opaque, hwaddr
addr,
}
}
-static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
+static void virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy,
+ const char *vdev_name)
{
static const MemoryRegionOps common_ops = {
.read = virtio_pci_common_read,
@@ -1470,36 +1471,41 @@ static void
virtio_pci_modern_regions_init(VirtIOPCIProxy *proxy)
},
.endianness = DEVICE_LITTLE_ENDIAN,
};
+g_autoptr(GString) name = g_string_new(NULL);
-
+g_string_printf(name, "virtio-pci-common-%s", vdev_name);
memory_region_init_io(&proxy->common.mr, OBJECT(proxy),
&common_ops,
proxy,
- "virtio-pci-common",
+ name->str,
proxy->common.size);
+g_string_printf(name, "virtio-pci-isr-%s", vdev_name);
memory_region_init_io(&proxy->isr.mr, OBJECT(proxy),
&isr_ops,
proxy,
- "virtio-pci-isr",
+ name->str,
proxy->isr.size);
+g_string_printf(name, "virtio-pci-device-%s", vdev_name);
memory_region_init_io(&proxy->device.mr, OBJECT(proxy),
&device_ops,
proxy,
- "virtio-pci-device",
+ name->str,
proxy->device.size);
+g_string_printf(name, "virtio-pci-notify-%s", vdev_name);
memory_region_init_io(&proxy->notify.mr, OBJECT(proxy),
¬ify_ops,
proxy,
- "virtio-pci-notify",
+ name->str,
proxy->notify.size);
+g_string_printf(name, "virtio-pci-notify-pio-%s", vdev_name);
memory_region_init_io(&proxy->notify_pio.mr, OBJECT(proxy),
¬ify_pio_ops,
proxy,
- "virtio-pci-notify-pio",
+ name->str,
proxy->notify_pio.size);
}
@@ -1654,7 +1660,7 @@ static void virtio_pci_device_plugged(DeviceState *d,
Error **errp)
struct virtio_pci_cfg_cap *cfg_mask;
-virtio_pci_modern_regions_init(proxy);
+virtio_pci_modern_regions_init(proxy, vdev->name);
virtio_pci_modern_mem_region_map(proxy, &proxy->common, &cap);
virtio_pci_modern_mem_region_map(proxy, &proxy->isr, &cap);
--
2.20.1
[PULL 00/23] plugin updates (hwprofile, CF_NOCACHE, io_recompile)
The following changes since commit 1af5629673bb5c1592d993f9fb6119a62845f576:
Merge remote-tracking branch
'remotes/dgilbert-gitlab/tags/pull-virtiofs-20210216' into staging (2021-02-17
14:44:18 +)
are available in the Git repository at:
https://github.com/stsquad/qemu.git tags/pull-plugin-updates-180221-1
for you to fetch changes up to df55e2a701d02bc01b9425843c667fd0cb4cdfa9:
tests/acceptance: add a memory callback check (2021-02-18 08:19:23 +)
Plugin updates:
- expose vdev name in PCI memory registration
- new hwprofile plugin
- bunch of style cleanups to contrib/plugins
- fix call signature of inline instrumentation
- re-factor the io_recompile code to push specialisation into hooks
- add some acceptance tests for the plugins
- clean-up and remove CF_NOCACHE handling from TCG
- fix instrumentation of cpu_io_recompile sections
- expand tests to check inline and cb count the same
Alex Bennée (14):
hw/virtio/pci: include vdev name in registered PCI sections
plugins: add API to return a name for a IO device
plugins: new hwprofile plugin
accel/tcg/plugin-gen: fix the call signature for inline callbacks
tests/plugin: expand insn test to detect duplicate instructions
tests/acceptance: add a new set of tests to exercise plugins
accel/tcg: actually cache our partial icount TB
accel/tcg: cache single instruction TB on pending replay exception
accel/tcg: re-factor non-RAM execution code
accel/tcg: remove CF_NOCACHE and special cases
accel/tcg: allow plugin instrumentation to be disable via cflags
tests/acceptance: add a new tests to detect counting errors
tests/plugin: allow memory plugin to do both inline and callbacks
tests/acceptance: add a memory callback check
Richard Henderson (4):
exec: Move TranslationBlock typedef to qemu/typedefs.h
accel/tcg: Create io_recompile_replay_branch hook
target/mips: Create mips_io_recompile_replay_branch
target/sh4: Create superh_io_recompile_replay_branch
zhouyang (5):
contrib: Don't use '#' flag of printf format
contrib: Fix some code style problems, ERROR: "foo * bar" should be "foo
*bar"
contrib: Add spaces around operator
contrib: space required after that ','
contrib: Open brace '{' following struct go on the same line
docs/devel/tcg-plugins.rst | 34
include/exec/exec-all.h | 9 +-
include/exec/plugin-gen.h| 4 +-
include/exec/tb-context.h| 1 -
include/hw/core/cpu.h| 4 +-
include/hw/core/tcg-cpu-ops.h| 13 +-
include/qemu/plugin.h| 4 +
include/qemu/qemu-plugin.h | 6 +
include/qemu/typedefs.h | 1 +
target/arm/internals.h | 3 +-
accel/tcg/cpu-exec.c | 61 ++-
accel/tcg/plugin-gen.c | 35 ++--
accel/tcg/translate-all.c| 130 +
accel/tcg/translator.c | 5 +-
contrib/ivshmem-server/main.c| 2 +-
contrib/plugins/hotblocks.c | 2 +-
contrib/plugins/hotpages.c | 2 +-
contrib/plugins/howvec.c | 19 +-
contrib/plugins/hwprofile.c | 305 +++
contrib/plugins/lockstep.c | 6 +-
hw/virtio/virtio-pci.c | 22 ++-
plugins/api.c| 56 --
target/cris/translate.c | 2 +-
target/lm32/translate.c | 2 +-
target/mips/cpu.c| 18 ++
target/moxie/translate.c | 2 +-
target/sh4/cpu.c | 18 ++
target/unicore32/translate.c | 2 +-
tests/plugin/insn.c | 12 +-
tests/plugin/mem.c | 27 ++-
MAINTAINERS | 1 +
contrib/plugins/Makefile | 1 +
tests/acceptance/tcg_plugins.py | 148 +++
tests/tcg/i386/Makefile.softmmu-target | 10 +
tests/tcg/i386/Makefile.target | 7 +
tests/tcg/x86_64/Makefile.softmmu-target | 10 +
36 files changed, 769 insertions(+), 215 deletions(-)
create mode 100644 contrib/plugins/hwprofile.c
create mode 100644 tests/acceptance/tcg_plugins.py
--
2.20.1
[PULL 12/23] target/mips: Create mips_io_recompile_replay_branch
From: Richard Henderson
Move the code from accel/tcg/translate-all.c to target/mips/cpu.c.
Signed-off-by: Richard Henderson
Signed-off-by: Alex Bennée
Reviewed-by: Alex Bennée
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20210208233906.479571-4-richard.hender...@linaro.org>
Message-Id: <20210213130325.14781-13-alex.ben...@linaro.org>
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 99ca6f36b9..9fea5c0e59 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2418,7 +2418,7 @@ void tb_check_watchpoint(CPUState *cpu, uintptr_t retaddr)
*/
void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
{
-#if defined(TARGET_MIPS) || defined(TARGET_SH4)
+#if defined(TARGET_SH4)
CPUArchState *env = cpu->env_ptr;
#endif
TranslationBlock *tb;
@@ -2444,15 +2444,7 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
cpu_neg(cpu)->icount_decr.u16.low++;
n = 2;
}
-#if defined(TARGET_MIPS)
-if ((env->hflags & MIPS_HFLAG_BMASK) != 0
-&& env->active_tc.PC != tb->pc) {
-env->active_tc.PC -= (env->hflags & MIPS_HFLAG_B16 ? 2 : 4);
-cpu_neg(cpu)->icount_decr.u16.low++;
-env->hflags &= ~MIPS_HFLAG_BMASK;
-n = 2;
-}
-#elif defined(TARGET_SH4)
+#if defined(TARGET_SH4)
if ((env->flags & ((DELAY_SLOT | DELAY_SLOT_CONDITIONAL))) != 0
&& env->pc != tb->pc) {
env->pc -= 2;
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index ad163ead62..bf70c77295 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -268,6 +268,23 @@ static void mips_cpu_synchronize_from_tb(CPUState *cs,
env->hflags &= ~MIPS_HFLAG_BMASK;
env->hflags |= tb->flags & MIPS_HFLAG_BMASK;
}
+
+# ifndef CONFIG_USER_ONLY
+static bool mips_io_recompile_replay_branch(CPUState *cs,
+const TranslationBlock *tb)
+{
+MIPSCPU *cpu = MIPS_CPU(cs);
+CPUMIPSState *env = &cpu->env;
+
+if ((env->hflags & MIPS_HFLAG_BMASK) != 0
+&& env->active_tc.PC != tb->pc) {
+env->active_tc.PC -= (env->hflags & MIPS_HFLAG_B16 ? 2 : 4);
+env->hflags &= ~MIPS_HFLAG_BMASK;
+return true;
+}
+return false;
+}
+# endif /* !CONFIG_USER_ONLY */
#endif /* CONFIG_TCG */
static bool mips_cpu_has_work(CPUState *cs)
@@ -679,6 +696,7 @@ static struct TCGCPUOps mips_tcg_ops = {
.do_interrupt = mips_cpu_do_interrupt,
.do_transaction_failed = mips_cpu_do_transaction_failed,
.do_unaligned_access = mips_cpu_do_unaligned_access,
+.io_recompile_replay_branch = mips_io_recompile_replay_branch,
#endif /* !CONFIG_USER_ONLY */
};
#endif /* CONFIG_TCG */
--
2.20.1
[PULL 10/23] exec: Move TranslationBlock typedef to qemu/typedefs.h
From: Richard Henderson
This also means we don't need an extra declaration of
the structure in hw/core/cpu.h.
Signed-off-by: Richard Henderson
Signed-off-by: Alex Bennée
Reviewed-by: Alex Bennée
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20210208233906.479571-2-richard.hender...@linaro.org>
Message-Id: <20210213130325.14781-11-alex.ben...@linaro.org>
diff --git a/include/exec/tb-context.h b/include/exec/tb-context.h
index ec4c13b455..cc33979113 100644
--- a/include/exec/tb-context.h
+++ b/include/exec/tb-context.h
@@ -26,7 +26,6 @@
#define CODE_GEN_HTABLE_BITS 15
#define CODE_GEN_HTABLE_SIZE (1 << CODE_GEN_HTABLE_BITS)
-typedef struct TranslationBlock TranslationBlock;
typedef struct TBContext TBContext;
struct TBContext {
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 38d813c389..c005d3dc2d 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -74,8 +74,6 @@ typedef enum MMUAccessType {
typedef struct CPUWatchpoint CPUWatchpoint;
-struct TranslationBlock;
-
/* see tcg-cpu-ops.h */
struct TCGCPUOps;
@@ -375,7 +373,7 @@ struct CPUState {
IcountDecr *icount_decr_ptr;
/* Accessed in parallel; all accesses must be atomic */
-struct TranslationBlock *tb_jmp_cache[TB_JMP_CACHE_SIZE];
+TranslationBlock *tb_jmp_cache[TB_JMP_CACHE_SIZE];
struct GDBRegisterState *gdb_regs;
int gdb_num_regs;
diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
index ccc97d1894..ac3bb051f2 100644
--- a/include/hw/core/tcg-cpu-ops.h
+++ b/include/hw/core/tcg-cpu-ops.h
@@ -30,8 +30,7 @@ struct TCGCPUOps {
* If more state needs to be restored, the target must implement a
* function to restore all the state, and register it here.
*/
-void (*synchronize_from_tb)(CPUState *cpu,
-const struct TranslationBlock *tb);
+void (*synchronize_from_tb)(CPUState *cpu, const TranslationBlock *tb);
/** @cpu_exec_enter: Callback for cpu_exec preparation */
void (*cpu_exec_enter)(CPUState *cpu);
/** @cpu_exec_exit: Callback for cpu_exec cleanup */
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index dc39b05c30..ee60eb3de4 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -120,6 +120,7 @@ typedef struct ReservedRegion ReservedRegion;
typedef struct SavedIOTLB SavedIOTLB;
typedef struct SHPCDevice SHPCDevice;
typedef struct SSIBus SSIBus;
+typedef struct TranslationBlock TranslationBlock;
typedef struct VirtIODevice VirtIODevice;
typedef struct Visitor Visitor;
typedef struct VMChangeStateEntry VMChangeStateEntry;
diff --git a/target/arm/internals.h b/target/arm/internals.h
index c38d541017..05cebc8597 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -173,8 +173,7 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu);
void arm_translate_init(void);
#ifdef CONFIG_TCG
-void arm_cpu_synchronize_from_tb(CPUState *cs,
- const struct TranslationBlock *tb);
+void arm_cpu_synchronize_from_tb(CPUState *cs, const TranslationBlock *tb);
#endif /* CONFIG_TCG */
diff --git a/target/cris/translate.c b/target/cris/translate.c
index c893f877ab..65c168c0c7 100644
--- a/target/cris/translate.c
+++ b/target/cris/translate.c
@@ -132,7 +132,7 @@ typedef struct DisasContext {
int delayed_branch;
-struct TranslationBlock *tb;
+TranslationBlock *tb;
int singlestep_enabled;
} DisasContext;
diff --git a/target/lm32/translate.c b/target/lm32/translate.c
index 030b232d66..20c70d03f1 100644
--- a/target/lm32/translate.c
+++ b/target/lm32/translate.c
@@ -93,7 +93,7 @@ typedef struct DisasContext {
unsigned int tb_flags, synced_flags; /* tb dependent flags. */
int is_jmp;
-struct TranslationBlock *tb;
+TranslationBlock *tb;
int singlestep_enabled;
uint32_t features;
diff --git a/target/moxie/translate.c b/target/moxie/translate.c
index d5fb27dfb8..24a742b25e 100644
--- a/target/moxie/translate.c
+++ b/target/moxie/translate.c
@@ -36,7 +36,7 @@
/* This is the state at translation time. */
typedef struct DisasContext {
-struct TranslationBlock *tb;
+TranslationBlock *tb;
target_ulong pc, saved_pc;
uint32_t opcode;
uint32_t fp_status;
diff --git a/target/unicore32/translate.c b/target/unicore32/translate.c
index 962f9877a0..370709c9ea 100644
--- a/target/unicore32/translate.c
+++ b/target/unicore32/translate.c
@@ -34,7 +34,7 @@ typedef struct DisasContext {
int condjmp;
/* The label that will be jumped to when the instruction is skipped. */
TCGLabel *condlabel;
-struct TranslationBlock *tb;
+TranslationBlock *tb;
int singlestep_enabled;
#ifndef CONFIG_USER_ONLY
int user;
--
2.20.1
[PULL 04/23] contrib: Don't use '#' flag of printf format
From: zhouyang
I am reading contrib related code and found some style problems while
check the code using checkpatch.pl. This commit fixs the misuse of
'#' flag of printf format
Signed-off-by: zhouyang
Signed-off-by: Alex Bennée
Message-Id: <20210118031004.1662363-2-zhouyang...@huawei.com>
Message-Id: <20210213130325.14781-5-alex.ben...@linaro.org>
diff --git a/contrib/plugins/hotblocks.c b/contrib/plugins/hotblocks.c
index 37435a3fc7..4b08340143 100644
--- a/contrib/plugins/hotblocks.c
+++ b/contrib/plugins/hotblocks.c
@@ -63,7 +63,7 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
for (i = 0; i < limit && it->next; i++, it = it->next) {
ExecCount *rec = (ExecCount *) it->data;
-g_string_append_printf(report, "%#016"PRIx64", %d, %ld,
%"PRId64"\n",
+g_string_append_printf(report, "0x%016"PRIx64", %d, %ld,
%"PRId64"\n",
rec->start_addr, rec->trans_count,
rec->insns, rec->exec_count);
}
diff --git a/contrib/plugins/hotpages.c b/contrib/plugins/hotpages.c
index ecd6c18732..eacc678eac 100644
--- a/contrib/plugins/hotpages.c
+++ b/contrib/plugins/hotpages.c
@@ -88,7 +88,7 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
for (i = 0; i < limit && it->next; i++, it = it->next) {
PageCounters *rec = (PageCounters *) it->data;
g_string_append_printf(report,
- "%#016"PRIx64", 0x%04x, %"PRId64
+ "0x%016"PRIx64", 0x%04x, %"PRId64
", 0x%04x, %"PRId64"\n",
rec->page_address,
rec->cpu_read, rec->reads,
diff --git a/contrib/plugins/howvec.c b/contrib/plugins/howvec.c
index 3b9a6939f2..6e602aaccf 100644
--- a/contrib/plugins/howvec.c
+++ b/contrib/plugins/howvec.c
@@ -209,7 +209,7 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
i++, counts = g_list_next(counts)) {
InsnExecCount *rec = (InsnExecCount *) counts->data;
g_string_append_printf(report,
- "Instr: %-24s\t(%ld hits)\t(op=%#08x/%s)\n",
+ "Instr: %-24s\t(%ld
hits)\t(op=0x%08x/%s)\n",
rec->insn,
rec->count,
rec->opcode,
diff --git a/contrib/plugins/lockstep.c b/contrib/plugins/lockstep.c
index 5aad50869d..7fd35eb669 100644
--- a/contrib/plugins/lockstep.c
+++ b/contrib/plugins/lockstep.c
@@ -134,7 +134,7 @@ static void report_divergance(ExecState *us, ExecState
*them)
/* Output short log entry of going out of sync... */
if (verbose || divrec.distance == 1 || diverged) {
-g_string_printf(out, "@ %#016lx vs %#016lx (%d/%d since last)\n",
+g_string_printf(out, "@ 0x%016lx vs 0x%016lx (%d/%d since last)\n",
us->pc, them->pc, g_slist_length(divergence_log),
divrec.distance);
qemu_plugin_outs(out->str);
@@ -144,7 +144,7 @@ static void report_divergance(ExecState *us, ExecState
*them)
int i;
GSList *entry;
-g_string_printf(out, "Δ insn_count @ %#016lx (%ld) vs %#016lx (%ld)\n",
+g_string_printf(out, "Δ insn_count @ 0x%016lx (%ld) vs 0x%016lx
(%ld)\n",
us->pc, us->insn_count, them->pc, them->insn_count);
for (entry = log, i = 0;
@@ -152,7 +152,7 @@ static void report_divergance(ExecState *us, ExecState
*them)
entry = g_slist_next(entry), i++) {
ExecInfo *prev = (ExecInfo *) entry->data;
g_string_append_printf(out,
- " previously @ %#016lx/%ld (%ld insns)\n",
+ " previously @ 0x%016lx/%ld (%ld insns)\n",
prev->block->pc, prev->block->insns,
prev->insn_count);
}
--
2.20.1
[PULL 03/23] plugins: new hwprofile plugin
This is a plugin intended to help with profiling access to various
bits of system hardware. It only really makes sense for system
emulation.
It takes advantage of the recently exposed helper API that allows us
to see the device name (memory region name) associated with a device.
You can specify arg=read or arg=write to limit the tracking to just
reads or writes (by default it does both).
The pattern option:
-plugin ./tests/plugin/libhwprofile.so,arg=pattern
will allow you to see the access pattern to devices, eg:
gic_cpu @ 0xffc01004
off:, 8, 1, 8, 1
off:, 4, 1, 4, 1
off:, 2, 1, 2, 1
off:, 1, 1, 1, 1
The source option:
-plugin ./tests/plugin/libhwprofile.so,arg=source
will track the virtual source address of the instruction making the
access:
pl011 @ 0xffc010031000
pc:ffc0104c785c, 1, 4, 0, 0
pc:ffc0104c7898, 1, 4, 0, 0
pc:ffc010512bcc, 2, 1867, 0, 0
You cannot mix source and pattern.
Finally the match option allow you to limit the tracking to just the
devices you care about.
Signed-off-by: Alex Bennée
Tested-by: Robert Foley
Reviewed-by: Robert Foley
Message-Id: <20210213130325.14781-4-alex.ben...@linaro.org>
diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
index 0568dfa6a4..39ce86ed96 100644
--- a/docs/devel/tcg-plugins.rst
+++ b/docs/devel/tcg-plugins.rst
@@ -280,3 +280,37 @@ which will eventually report::
previously @ 0x00ffd08098/5 (809900593 insns)
previously @ 0x00ffd080c0/1 (809900588 insns)
+- contrib/plugins/hwprofile
+
+The hwprofile tool can only be used with system emulation and allows
+the user to see what hardware is accessed how often. It has a number of
options:
+
+ * arg=read or arg=write
+
+ By default the plugin tracks both reads and writes. You can use one
+ of these options to limit the tracking to just one class of accesses.
+
+ * arg=source
+
+ Will include a detailed break down of what the guest PC that made the
+ access was. Not compatible with arg=pattern. Example output::
+
+ cirrus-low-memory @ 0xfd0a
+pc:fc005cdc, 1, 256
+pc:fc005ce8, 1, 256
+pc:fc005cec, 1, 256
+
+ * arg=pattern
+
+ Instead break down the accesses based on the offset into the HW
+ region. This can be useful for seeing the most used registers of a
+ device. Example output::
+
+pci0-conf @ 0xfd01fe00
+ off:0004, 1, 1
+ off:0010, 1, 3
+ off:0014, 1, 3
+ off:0018, 1, 2
+ off:001c, 1, 2
+ off:0020, 1, 2
+ ...
diff --git a/contrib/plugins/hwprofile.c b/contrib/plugins/hwprofile.c
new file mode 100644
index 00..6dac1d5f85
--- /dev/null
+++ b/contrib/plugins/hwprofile.c
@@ -0,0 +1,305 @@
+/*
+ * Copyright (C) 2020, Alex Bennée
+ *
+ * HW Profile - breakdown access patterns for IO to devices
+ *
+ * License: GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+
+typedef struct {
+uint64_t cpu_read;
+uint64_t cpu_write;
+uint64_t reads;
+uint64_t writes;
+} IOCounts;
+
+typedef struct {
+uint64_t off_or_pc;
+IOCounts counts;
+} IOLocationCounts;
+
+typedef struct {
+const char *name;
+uint64_t base;
+IOCounts totals;
+GHashTable *detail;
+} DeviceCounts;
+
+static GMutex lock;
+static GHashTable *devices;
+
+/* track the access pattern to a piece of HW */
+static bool pattern;
+/* track the source address of access to HW */
+static bool source;
+/* track only matched regions of HW */
+static bool check_match;
+static gchar **matches;
+
+static enum qemu_plugin_mem_rw rw = QEMU_PLUGIN_MEM_RW;
+
+static inline bool track_reads(void)
+{
+return rw == QEMU_PLUGIN_MEM_RW || rw == QEMU_PLUGIN_MEM_R;
+}
+
+static inline bool track_writes(void)
+{
+return rw == QEMU_PLUGIN_MEM_RW || rw == QEMU_PLUGIN_MEM_W;
+}
+
+static void plugin_init(void)
+{
+devices = g_hash_table_new(NULL, NULL);
+}
+
+static gint sort_cmp(gconstpointer a, gconstpointer b)
+{
+DeviceCounts *ea = (DeviceCounts *) a;
+DeviceCounts *eb = (DeviceCounts *) b;
+return ea->totals.reads + ea->totals.writes >
+ eb->totals.reads + eb->totals.writes ? -1 : 1;
+}
+
+static gint sort_loc(gconstpointer a, gconstpointer b)
+{
+IOLocationCounts *ea = (IOLocationCounts *) a;
+IOLocationCounts *eb = (IOLocationCounts *) b;
+return ea->off_or_pc > eb->off_or_pc;
+}
+
+static void fmt_iocount_record(GString *s, IOCounts *rec)
+{
+if (track_reads()) {
+g_string_append_printf(s, ", %"PRIx64", %"PRId64,
+ rec->cpu_read, rec->reads);
+}
+if (track_writes()) {
+g_string_append_printf(s, ", %"PRIx64",
[PULL 07/23] contrib: space required after that ','
From: zhouyang
I am reading contrib related code and found some style problems while
check the code using checkpatch.pl. This commit fixs the issue below:
ERROR: space required after that ','
Signed-off-by: zhouyang
Signed-off-by: Alex Bennée
Message-Id: <20210118031004.1662363-5-zhouyang...@huawei.com>
Message-Id: <20210213130325.14781-8-alex.ben...@linaro.org>
diff --git a/contrib/plugins/howvec.c b/contrib/plugins/howvec.c
index 2f892da17d..9d6fa33297 100644
--- a/contrib/plugins/howvec.c
+++ b/contrib/plugins/howvec.c
@@ -68,7 +68,7 @@ static InsnClassExecCount aarch64_insn_classes[] = {
{ "Reserved","res",0x1e00, 0x, COUNT_CLASS},
/* Data Processing Immediate */
{ " PCrel addr","pcrel", 0x1f00, 0x1000, COUNT_CLASS},
-{ " Add/Sub (imm,tags)","asit", 0x1f80, 0x1180, COUNT_CLASS},
+{ " Add/Sub (imm,tags)", "asit", 0x1f80, 0x1180, COUNT_CLASS},
{ " Add/Sub (imm)", "asi",0x1f00, 0x1100, COUNT_CLASS},
{ " Logical (imm)", "logi", 0x1f80, 0x1200, COUNT_CLASS},
{ " Move Wide (imm)", "movwi", 0x1f80, 0x1280, COUNT_CLASS},
@@ -91,17 +91,17 @@ static InsnClassExecCount aarch64_insn_classes[] = {
{ "Branches","branch", 0x1c00, 0x1400, COUNT_CLASS},
/* Loads and Stores */
{ " AdvSimd ldstmult", "advlsm", 0xbfbf, 0x0c00, COUNT_CLASS},
-{ " AdvSimd ldstmult++","advlsmp",0xbfb0, 0x0c80, COUNT_CLASS},
+{ " AdvSimd ldstmult++", "advlsmp", 0xbfb0, 0x0c80, COUNT_CLASS},
{ " AdvSimd ldst", "advlss", 0xbf9f, 0x0d00, COUNT_CLASS},
-{ " AdvSimd ldst++","advlssp",0xbf80, 0x0d80, COUNT_CLASS},
+{ " AdvSimd ldst++","advlssp", 0xbf80, 0x0d80, COUNT_CLASS},
{ " ldst excl", "ldstx", 0x3f00, 0x0800, COUNT_CLASS},
{ "Prefetch","prfm", 0xff00, 0xd800, COUNT_CLASS},
{ " Load Reg (lit)","ldlit", 0x1b00, 0x1800, COUNT_CLASS},
-{ " ldst noalloc pair", "ldstnap",0x3b80, 0x2800, COUNT_CLASS},
+{ " ldst noalloc pair", "ldstnap", 0x3b80, 0x2800, COUNT_CLASS},
{ " ldst pair", "ldstp", 0x3800, 0x2800, COUNT_CLASS},
{ " ldst reg", "ldstr", 0x3b20, 0x3800, COUNT_CLASS},
{ " Atomic ldst", "atomic", 0x3b200c00, 0x3820, COUNT_CLASS},
-{ " ldst reg (reg off)","ldstro", 0x3b200b00, 0x38200800, COUNT_CLASS},
+{ " ldst reg (reg off)", "ldstro", 0x3b200b00, 0x38200800, COUNT_CLASS},
{ " ldst reg (pac)","ldstpa", 0x3b200200, 0x38200800, COUNT_CLASS},
{ " ldst reg (imm)","ldsti", 0x3b00, 0x3900, COUNT_CLASS},
{ "Loads & Stores", "ldst", 0x0a00, 0x0800, COUNT_CLASS},
@@ -202,7 +202,7 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
counts = g_hash_table_get_values(insns);
if (counts && g_list_next(counts)) {
-g_string_append_printf(report,"Individual Instructions:\n");
+g_string_append_printf(report, "Individual Instructions:\n");
counts = g_list_sort(counts, cmp_exec_count);
for (i = 0; i < limit && g_list_next(counts);
--
2.20.1
[PULL 06/23] contrib: Add spaces around operator
From: zhouyang I am reading contrib related code and found some style problems while check the code using checkpatch.pl. This commit fixs the issue below: ERROR: spaces required around that '*' Signed-off-by: zhouyang Signed-off-by: Alex Bennée Message-Id: <20210118031004.1662363-4-zhouyang...@huawei.com> Message-Id: <20210213130325.14781-7-alex.ben...@linaro.org> diff --git a/contrib/ivshmem-server/main.c b/contrib/ivshmem-server/main.c index ee08c4ced0..224dbeb547 100644 --- a/contrib/ivshmem-server/main.c +++ b/contrib/ivshmem-server/main.c @@ -17,7 +17,7 @@ #define IVSHMEM_SERVER_DEFAULT_PID_FILE "/var/run/ivshmem-server.pid" #define IVSHMEM_SERVER_DEFAULT_UNIX_SOCK_PATH "/tmp/ivshmem_socket" #define IVSHMEM_SERVER_DEFAULT_SHM_PATH "ivshmem" -#define IVSHMEM_SERVER_DEFAULT_SHM_SIZE (4*1024*1024) +#define IVSHMEM_SERVER_DEFAULT_SHM_SIZE (4 * 1024 * 1024) #define IVSHMEM_SERVER_DEFAULT_N_VECTORS 1 /* used to quit on signal SIGTERM */ -- 2.20.1
[PULL 22/23] tests/plugin: allow memory plugin to do both inline and callbacks
This is going to be useful for acceptance tests that check both types
are being called the same number of times, especially when icount is
enabled.
Signed-off-by: Alex Bennée
Reviewed-by: Richard Henderson
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20210213130325.14781-23-alex.ben...@linaro.org>
diff --git a/tests/plugin/mem.c b/tests/plugin/mem.c
index 4725bd851d..afd1d27e5c 100644
--- a/tests/plugin/mem.c
+++ b/tests/plugin/mem.c
@@ -16,9 +16,10 @@
QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
-static uint64_t mem_count;
+static uint64_t inline_mem_count;
+static uint64_t cb_mem_count;
static uint64_t io_count;
-static bool do_inline;
+static bool do_inline, do_callback;
static bool do_haddr;
static enum qemu_plugin_mem_rw rw = QEMU_PLUGIN_MEM_RW;
@@ -26,7 +27,12 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
{
g_autoptr(GString) out = g_string_new("");
-g_string_printf(out, "mem accesses: %" PRIu64 "\n", mem_count);
+if (do_inline) {
+g_string_printf(out, "inline mem accesses: %" PRIu64 "\n",
inline_mem_count);
+}
+if (do_callback) {
+g_string_append_printf(out, "callback mem accesses: %" PRIu64 "\n",
cb_mem_count);
+}
if (do_haddr) {
g_string_append_printf(out, "io accesses: %" PRIu64 "\n", io_count);
}
@@ -42,10 +48,10 @@ static void vcpu_mem(unsigned int cpu_index,
qemu_plugin_meminfo_t meminfo,
if (qemu_plugin_hwaddr_is_io(hwaddr)) {
io_count++;
} else {
-mem_count++;
+cb_mem_count++;
}
} else {
-mem_count++;
+cb_mem_count++;
}
}
@@ -60,8 +66,9 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct
qemu_plugin_tb *tb)
if (do_inline) {
qemu_plugin_register_vcpu_mem_inline(insn, rw,
QEMU_PLUGIN_INLINE_ADD_U64,
- &mem_count, 1);
-} else {
+ &inline_mem_count, 1);
+}
+if (do_callback) {
qemu_plugin_register_vcpu_mem_cb(insn, vcpu_mem,
QEMU_PLUGIN_CB_NO_REGS,
rw, NULL);
@@ -90,6 +97,12 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t
id,
}
if (!strcmp(argv[0], "inline")) {
do_inline = true;
+do_callback = false;
+} else if (!strcmp(argv[0], "both")) {
+do_inline = true;
+do_callback = true;
+} else {
+do_callback = true;
}
}
--
2.20.1
[PULL 02/23] plugins: add API to return a name for a IO device
This may well end up being anonymous but it should always be unique.
Signed-off-by: Alex Bennée
Reviewed-by: Clement Deschamps
Reviewed-by: Emilio G. Cota
Reviewed-by: Richard Henderson
Message-Id: <20210213130325.14781-3-alex.ben...@linaro.org>
diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 5775e82c4e..c66507fe8f 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -330,6 +330,12 @@ struct qemu_plugin_hwaddr
*qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
bool qemu_plugin_hwaddr_is_io(const struct qemu_plugin_hwaddr *haddr);
uint64_t qemu_plugin_hwaddr_device_offset(const struct qemu_plugin_hwaddr
*haddr);
+/*
+ * Returns a string representing the device. The string is valid for
+ * the lifetime of the plugin.
+ */
+const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h);
+
typedef void
(*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index,
qemu_plugin_meminfo_t info, uint64_t vaddr,
diff --git a/plugins/api.c b/plugins/api.c
index bbdc5a4eb4..5dc8e6f934 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -303,6 +303,26 @@ uint64_t qemu_plugin_hwaddr_device_offset(const struct
qemu_plugin_hwaddr *haddr
return 0;
}
+const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h)
+{
+#ifdef CONFIG_SOFTMMU
+if (h && h->is_io) {
+MemoryRegionSection *mrs = h->v.io.section;
+if (!mrs->mr->name) {
+unsigned long maddr = 0x & (uintptr_t) mrs->mr;
+g_autofree char *temp = g_strdup_printf("anon%08lx", maddr);
+return g_intern_string(temp);
+} else {
+return g_intern_string(mrs->mr->name);
+}
+} else {
+return g_intern_static_string("RAM");
+}
+#else
+return g_intern_static_string("Invalid");
+#endif
+}
+
/*
* Queries to the number and potential maximum number of vCPUs there
* will be. This helps the plugin dimension per-vcpu arrays.
--
2.20.1
[PULL 15/23] tests/acceptance: add a new set of tests to exercise plugins
This is just a simple test to count the instructions executed by a
kernel. However a later test will detect a failure condition when
icount is enabled.
Signed-off-by: Alex Bennée
Tested-by: Wainer dos Santos Moschetta
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20210213130325.14781-16-alex.ben...@linaro.org>
diff --git a/MAINTAINERS b/MAINTAINERS
index 68ee271792..1d711e0cb8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2903,6 +2903,7 @@ S: Maintained
F: docs/devel/tcg-plugins.rst
F: plugins/
F: tests/plugin/
+F: tests/acceptance/tcg_plugins.py
F: contrib/plugins/
AArch64 TCG target
diff --git a/tests/acceptance/tcg_plugins.py b/tests/acceptance/tcg_plugins.py
new file mode 100644
index 00..adec40d3a5
--- /dev/null
+++ b/tests/acceptance/tcg_plugins.py
@@ -0,0 +1,91 @@
+# TCG Plugins tests
+#
+# These are a little more involved than the basic tests run by check-tcg.
+#
+# Copyright (c) 2021 Linaro
+#
+# Author:
+# Alex Bennée
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import tempfile
+import mmap
+import re
+
+from boot_linux_console import LinuxKernelTest
+
+
+class PluginKernelBase(LinuxKernelTest):
+"""
+Boots a Linux kernel with a TCG plugin enabled.
+"""
+
+timeout = 120
+KERNEL_COMMON_COMMAND_LINE = 'printk.time=1 panic=-1 '
+
+def run_vm(self, kernel_path, kernel_command_line,
+ plugin, plugin_log, console_pattern, args):
+
+vm = self.get_vm()
+vm.set_console()
+vm.add_args('-kernel', kernel_path,
+'-append', kernel_command_line,
+'-plugin', plugin,
+'-d', 'plugin',
+'-D', plugin_log,
+'-net', 'none',
+'-no-reboot')
+if args:
+vm.add_args(*args)
+
+try:
+vm.launch()
+except:
+# TODO: probably fails because plugins not enabled but we
+# can't currently probe for the feature.
+self.cancel("TCG Plugins not enabled?")
+
+self.wait_for_console_pattern(console_pattern, vm)
+# ensure logs are flushed
+vm.shutdown()
+
+
+class PluginKernelNormal(PluginKernelBase):
+
+def _grab_aarch64_kernel(self):
+kernel_url = ('http://security.debian.org/'
+ 'debian-security/pool/updates/main/l/linux-signed-arm64/'
+ 'linux-image-4.19.0-12-arm64_4.19.152-1_arm64.deb')
+kernel_sha1 = '2036c2792f80ac9c4ccaae742b2e0a28385b6010'
+kernel_deb = self.fetch_asset(kernel_url, asset_hash=kernel_sha1)
+kernel_path = self.extract_from_deb(kernel_deb,
+"/boot/vmlinuz-4.19.0-12-arm64")
+return kernel_path
+
+def test_aarch64_virt_insn(self):
+"""
+:avocado: tags=accel:tcg
+:avocado: tags=arch:aarch64
+:avocado: tags=machine:virt
+:avocado: tags=cpu:cortex-a57
+"""
+kernel_path = self._grab_aarch64_kernel()
+kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
+ 'console=ttyAMA0')
+console_pattern = 'Kernel panic - not syncing: VFS:'
+
+plugin_log = tempfile.NamedTemporaryFile(mode="r+t", prefix="plugin",
+ suffix=".log")
+
+self.run_vm(kernel_path, kernel_command_line,
+"tests/plugin/libinsn.so", plugin_log.name,
+console_pattern,
+args=('-cpu', 'cortex-a53'))
+
+with plugin_log as lf, \
+ mmap.mmap(lf.fileno(), 0, access=mmap.ACCESS_READ) as s:
+
+m = re.search(br"insns: (?P\d+)", s)
+if "count" not in m.groupdict():
+self.fail("Failed to find instruction count")
--
2.20.1
[PULL 11/23] accel/tcg: Create io_recompile_replay_branch hook
From: Richard Henderson
Create a hook in which to split out the mips and
sh4 ifdefs from cpu_io_recompile.
[AJB: s/stoped/stopped/]
Signed-off-by: Richard Henderson
Signed-off-by: Alex Bennée
Reviewed-by: Alex Bennée
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20210208233906.479571-3-richard.hender...@linaro.org>
Message-Id: <20210213130325.14781-12-alex.ben...@linaro.org>
diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
index ac3bb051f2..72d791438c 100644
--- a/include/hw/core/tcg-cpu-ops.h
+++ b/include/hw/core/tcg-cpu-ops.h
@@ -88,6 +88,16 @@ struct TCGCPUOps {
*/
bool (*debug_check_watchpoint)(CPUState *cpu, CPUWatchpoint *wp);
+/**
+ * @io_recompile_replay_branch: Callback for cpu_io_recompile.
+ *
+ * The cpu has been stopped, and cpu_restore_state_from_tb has been
+ * called. If the faulting instruction is in a delay slot, and the
+ * target architecture requires re-execution of the branch, then
+ * adjust the cpu state as required and return true.
+ */
+bool (*io_recompile_replay_branch)(CPUState *cpu,
+ const TranslationBlock *tb);
#endif /* CONFIG_SOFTMMU */
#endif /* NEED_CPU_H */
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 2c34adccce..99ca6f36b9 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -60,6 +60,7 @@
#include "sysemu/cpu-timers.h"
#include "sysemu/tcg.h"
#include "qapi/error.h"
+#include "hw/core/tcg-cpu-ops.h"
#include "internal.h"
/* #define DEBUG_TB_INVALIDATE */
@@ -2421,6 +2422,7 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
CPUArchState *env = cpu->env_ptr;
#endif
TranslationBlock *tb;
+CPUClass *cc;
uint32_t n;
tb = tcg_tb_lookup(retaddr);
@@ -2430,11 +2432,18 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
}
cpu_restore_state_from_tb(cpu, tb, retaddr, true);
-/* On MIPS and SH, delay slot instructions can only be restarted if
- they were already the first instruction in the TB. If this is not
- the first instruction in a TB then re-execute the preceding
- branch. */
+/*
+ * Some guests must re-execute the branch when re-executing a delay
+ * slot instruction. When this is the case, adjust icount and N
+ * to account for the re-execution of the branch.
+ */
n = 1;
+cc = CPU_GET_CLASS(cpu);
+if (cc->tcg_ops->io_recompile_replay_branch &&
+cc->tcg_ops->io_recompile_replay_branch(cpu, tb)) {
+cpu_neg(cpu)->icount_decr.u16.low++;
+n = 2;
+}
#if defined(TARGET_MIPS)
if ((env->hflags & MIPS_HFLAG_BMASK) != 0
&& env->active_tc.PC != tb->pc) {
--
2.20.1
[PULL 21/23] tests/acceptance: add a new tests to detect counting errors
The insn plugin has a simple heuristic to detect if an instruction is
detected running twice in a row. Check the plugin log after the run
and pass accordingly.
Signed-off-by: Alex Bennée
Tested-by: Wainer dos Santos Moschetta
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20210213130325.14781-22-alex.ben...@linaro.org>
diff --git a/tests/acceptance/tcg_plugins.py b/tests/acceptance/tcg_plugins.py
index adec40d3a5..b1ba10498f 100644
--- a/tests/acceptance/tcg_plugins.py
+++ b/tests/acceptance/tcg_plugins.py
@@ -89,3 +89,29 @@ def test_aarch64_virt_insn(self):
m = re.search(br"insns: (?P\d+)", s)
if "count" not in m.groupdict():
self.fail("Failed to find instruction count")
+
+def test_aarch64_virt_insn_icount(self):
+"""
+:avocado: tags=accel:tcg
+:avocado: tags=arch:aarch64
+:avocado: tags=machine:virt
+:avocado: tags=cpu:cortex-a57
+"""
+kernel_path = self._grab_aarch64_kernel()
+kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
+ 'console=ttyAMA0')
+console_pattern = 'Kernel panic - not syncing: VFS:'
+
+plugin_log = tempfile.NamedTemporaryFile(mode="r+t", prefix="plugin",
+ suffix=".log")
+
+self.run_vm(kernel_path, kernel_command_line,
+"tests/plugin/libinsn.so", plugin_log.name,
+console_pattern,
+args=('-cpu', 'cortex-a53', '-icount', 'shift=1'))
+
+with plugin_log as lf, \
+ mmap.mmap(lf.fileno(), 0, access=mmap.ACCESS_READ) as s:
+m = re.search(br"detected repeat execution @
(?P0x[0-9A-Fa-f]+)", s)
+if m is not None and "addr" in m.groupdict():
+self.fail("detected repeated instructions")
--
2.20.1
[PULL 05/23] contrib: Fix some code style problems, ERROR: "foo * bar" should be "foo *bar"
From: zhouyang
I am reading contrib related code and found some style problems while
check the code using checkpatch.pl. This commit fixs the issue below:
ERROR: "foo * bar" should be "foo *bar"
Signed-off-by: zhouyang
Signed-off-by: Alex Bennée
Message-Id: <20210118031004.1662363-3-zhouyang...@huawei.com>
Message-Id: <20210213130325.14781-6-alex.ben...@linaro.org>
diff --git a/contrib/plugins/howvec.c b/contrib/plugins/howvec.c
index 6e602aaccf..2f892da17d 100644
--- a/contrib/plugins/howvec.c
+++ b/contrib/plugins/howvec.c
@@ -235,7 +235,7 @@ static void vcpu_insn_exec_before(unsigned int cpu_index,
void *udata)
(*count)++;
}
-static uint64_t * find_counter(struct qemu_plugin_insn *insn)
+static uint64_t *find_counter(struct qemu_plugin_insn *insn)
{
int i;
uint64_t *cnt = NULL;
--
2.20.1
[PULL 18/23] accel/tcg: re-factor non-RAM execution code
There is no real need to use CF_NOCACHE here. As long as the TB isn't
linked to other TBs or included in the QHT or jump cache then it will
only get executed once.
Signed-off-by: Alex Bennée
Reviewed-by: Richard Henderson
Message-Id: <20210213130325.14781-19-alex.ben...@linaro.org>
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index c0b98e76b9..72b3c663c5 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1779,7 +1779,8 @@ static inline void tb_page_add(PageDesc *p,
TranslationBlock *tb,
#endif
}
-/* add a new TB and link it to the physical page tables. phys_page2 is
+/*
+ * Add a new TB and link it to the physical page tables. phys_page2 is
* (-1) to indicate that only one page contains the TB.
*
* Called with mmap_lock held for user-mode emulation.
@@ -1798,17 +1799,6 @@ tb_link_page(TranslationBlock *tb, tb_page_addr_t
phys_pc,
assert_memory_lock();
-if (phys_pc == -1) {
-/*
- * If the TB is not associated with a physical RAM page then
- * it must be a temporary one-insn TB, and we have nothing to do
- * except fill in the page_addr[] fields.
- */
-assert(tb->cflags & CF_NOCACHE);
-tb->page_addr[0] = tb->page_addr[1] = -1;
-return tb;
-}
-
/*
* Add the TB to the page list, acquiring first the pages's locks.
* We keep the locks held until after inserting the TB in the hash table,
@@ -1881,9 +1871,8 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
phys_pc = get_page_addr_code(env, pc);
if (phys_pc == -1) {
-/* Generate a temporary TB with 1 insn in it */
-cflags &= ~CF_COUNT_MASK;
-cflags |= CF_NOCACHE | 1;
+/* Generate a one-shot TB with 1 insn in it */
+cflags = (cflags & ~CF_COUNT_MASK) | 1;
}
cflags &= ~CF_CLUSTER_MASK;
@@ -2097,6 +2086,17 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
tb_reset_jump(tb, 1);
}
+/*
+ * If the TB is not associated with a physical RAM page then
+ * it must be a temporary one-insn TB, and we have nothing to do
+ * except fill in the page_addr[] fields. Return early before
+ * attempting to link to other TBs or add to the lookup table.
+ */
+if (phys_pc == -1) {
+tb->page_addr[0] = tb->page_addr[1] = -1;
+return tb;
+}
+
/* check next page if needed */
virt_page2 = (pc + tb->size - 1) & TARGET_PAGE_MASK;
phys_page2 = -1;
--
2.20.1
[PULL 19/23] accel/tcg: remove CF_NOCACHE and special cases
Now we no longer generate CF_NOCACHE blocks we can remove a bunch of
the special case handling for them. While we are at it we can remove
the unused tb->orig_tb field and save a few bytes on the TB structure.
Signed-off-by: Alex Bennée
Reviewed-by: Richard Henderson
Message-Id: <20210213130325.14781-20-alex.ben...@linaro.org>
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index d30c7a84f6..665fe68607 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -454,7 +454,6 @@ struct TranslationBlock {
uint32_t cflags;/* compile flags */
#define CF_COUNT_MASK 0x7fff
#define CF_LAST_IO 0x8000 /* Last insn may be an IO access. */
-#define CF_NOCACHE 0x0001 /* To be freed after execution */
#define CF_USE_ICOUNT 0x0002
#define CF_INVALID 0x0004 /* TB is stale. Set with @jmp_lock held */
#define CF_PARALLEL0x0008 /* Generate code for a parallel context */
@@ -469,8 +468,6 @@ struct TranslationBlock {
struct tb_tc tc;
-/* original tb when cflags has CF_NOCACHE */
-struct TranslationBlock *orig_tb;
/* first and second physical page containing code. The lower bit
of the pointer tells the index in page_next[].
The list is protected by the TB's page('s) lock(s) */
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 72b3c663c5..464b3c3394 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -410,12 +410,6 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc,
bool will_exit)
TranslationBlock *tb = tcg_tb_lookup(host_pc);
if (tb) {
cpu_restore_state_from_tb(cpu, tb, host_pc, will_exit);
-if (tb_cflags(tb) & CF_NOCACHE) {
-/* one-shot translation, invalidate it immediately */
-tb_phys_invalidate(tb, -1);
-tcg_tb_remove(tb);
-tb_destroy(tb);
-}
return true;
}
}
@@ -1634,8 +1628,7 @@ static void do_tb_phys_invalidate(TranslationBlock *tb,
bool rm_from_page_list)
phys_pc = tb->page_addr[0] + (tb->pc & ~TARGET_PAGE_MASK);
h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb_cflags(tb) & CF_HASH_MASK,
tb->trace_vcpu_dstate);
-if (!(tb->cflags & CF_NOCACHE) &&
-!qht_remove(&tb_ctx.htable, tb, h)) {
+if (!qht_remove(&tb_ctx.htable, tb, h)) {
return;
}
@@ -1796,6 +1789,8 @@ tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
{
PageDesc *p;
PageDesc *p2 = NULL;
+void *existing_tb = NULL;
+uint32_t h;
assert_memory_lock();
@@ -1815,25 +1810,20 @@ tb_link_page(TranslationBlock *tb, tb_page_addr_t
phys_pc,
tb->page_addr[1] = -1;
}
-if (!(tb->cflags & CF_NOCACHE)) {
-void *existing_tb = NULL;
-uint32_t h;
-
-/* add in the hash table */
-h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb->cflags & CF_HASH_MASK,
- tb->trace_vcpu_dstate);
-qht_insert(&tb_ctx.htable, tb, h, &existing_tb);
+/* add in the hash table */
+h = tb_hash_func(phys_pc, tb->pc, tb->flags, tb->cflags & CF_HASH_MASK,
+ tb->trace_vcpu_dstate);
+qht_insert(&tb_ctx.htable, tb, h, &existing_tb);
-/* remove TB from the page(s) if we couldn't insert it */
-if (unlikely(existing_tb)) {
-tb_page_remove(p, tb);
-invalidate_page_bitmap(p);
-if (p2) {
-tb_page_remove(p2, tb);
-invalidate_page_bitmap(p2);
-}
-tb = existing_tb;
+/* remove TB from the page(s) if we couldn't insert it */
+if (unlikely(existing_tb)) {
+tb_page_remove(p, tb);
+invalidate_page_bitmap(p);
+if (p2) {
+tb_page_remove(p2, tb);
+invalidate_page_bitmap(p2);
}
+tb = existing_tb;
}
if (p2 && p2 != p) {
@@ -1906,7 +1896,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
tb->cs_base = cs_base;
tb->flags = flags;
tb->cflags = cflags;
-tb->orig_tb = NULL;
tb->trace_vcpu_dstate = *cpu->trace_dstate;
tcg_ctx->tb_cflags = cflags;
tb_overflow:
@@ -2445,16 +2434,6 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
/* Generate a new TB executing the I/O insn. */
cpu->cflags_next_tb = curr_cflags() | CF_LAST_IO | n;
-if (tb_cflags(tb) & CF_NOCACHE) {
-if (tb->orig_tb) {
-/* Invalidate original TB if this TB was generated in
- * cpu_exec_nocache() */
-tb_phys_invalidate(tb->orig_tb, -1);
-}
-tcg_tb_remove(tb);
-tb_destroy(tb);
-}
-
qemu_log_mask_and_addr(CPU_LOG_EXEC, tb->pc,
"cpu_io_recompile: rewound execution of TB to "
TARGET_FMT_lx "\n", tb->pc);
--
2.20.1
[PULL 08/23] contrib: Open brace '{' following struct go on the same line
From: zhouyang
I found some style problems whil check the code using checkpatch.pl.
This commit fixs the issue below:
ERROR: that open brace { should be on the previous line
Signed-off-by: zhouyang
Signed-off-by: Alex Bennée
Message-Id: <20210118031004.1662363-6-zhouyang...@huawei.com>
Message-Id: <20210213130325.14781-9-alex.ben...@linaro.org>
diff --git a/contrib/plugins/howvec.c b/contrib/plugins/howvec.c
index 9d6fa33297..600f7facc1 100644
--- a/contrib/plugins/howvec.c
+++ b/contrib/plugins/howvec.c
@@ -145,8 +145,7 @@ typedef struct {
int table_sz;
} ClassSelector;
-static ClassSelector class_tables[] =
-{
+static ClassSelector class_tables[] = {
{ "aarch64", aarch64_insn_classes, ARRAY_SIZE(aarch64_insn_classes) },
{ "sparc", sparc32_insn_classes, ARRAY_SIZE(sparc32_insn_classes) },
{ "sparc64", sparc64_insn_classes, ARRAY_SIZE(sparc64_insn_classes) },
--
2.20.1
[PULL 14/23] tests/plugin: expand insn test to detect duplicate instructions
A duplicate insn is one that is appears to be executed twice in a row.
This is currently possible due to -icount and cpu_io_recompile()
causing a re-translation of a block. On it's own this won't trigger
any tests though.
The heuristics that the plugin use can't deal with the x86 rep
instruction which (validly) will look like executing the same
instruction several times. To avoid problems later we tweak the rules
for x86 to run the "inline" version of the plugin. This also has the
advantage of increasing coverage of the plugin code (see bugfix in
previous commit).
Signed-off-by: Alex Bennée
Reviewed-by: Richard Henderson
Message-Id: <20210213130325.14781-15-alex.ben...@linaro.org>
diff --git a/tests/plugin/insn.c b/tests/plugin/insn.c
index a9a6e41237..c253980ec8 100644
--- a/tests/plugin/insn.c
+++ b/tests/plugin/insn.c
@@ -21,6 +21,14 @@ static bool do_inline;
static void vcpu_insn_exec_before(unsigned int cpu_index, void *udata)
{
+static uint64_t last_pc;
+uint64_t this_pc = GPOINTER_TO_UINT(udata);
+if (this_pc == last_pc) {
+g_autofree gchar *out = g_strdup_printf("detected repeat execution @
0x%"
+PRIx64 "\n", this_pc);
+qemu_plugin_outs(out);
+}
+last_pc = this_pc;
insn_count++;
}
@@ -36,8 +44,10 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct
qemu_plugin_tb *tb)
qemu_plugin_register_vcpu_insn_exec_inline(
insn, QEMU_PLUGIN_INLINE_ADD_U64, &insn_count, 1);
} else {
+uint64_t vaddr = qemu_plugin_insn_vaddr(insn);
qemu_plugin_register_vcpu_insn_exec_cb(
-insn, vcpu_insn_exec_before, QEMU_PLUGIN_CB_NO_REGS, NULL);
+insn, vcpu_insn_exec_before, QEMU_PLUGIN_CB_NO_REGS,
+GUINT_TO_POINTER(vaddr));
}
}
}
diff --git a/tests/tcg/i386/Makefile.softmmu-target
b/tests/tcg/i386/Makefile.softmmu-target
index 5266f2335a..fa9b1b9f90 100644
--- a/tests/tcg/i386/Makefile.softmmu-target
+++ b/tests/tcg/i386/Makefile.softmmu-target
@@ -33,5 +33,15 @@ EXTRA_RUNS+=$(MULTIARCH_RUNS)
memory: CFLAGS+=-DCHECK_UNALIGNED=1
+# non-inline runs will trigger the duplicate instruction heuristics in
libinsn.so
+run-plugin-%-with-libinsn.so:
+ $(call run-test, $@, \
+ $(QEMU) -monitor none -display none \
+ -chardev file$(COMMA)path=$@.out$(COMMA)id=output \
+ -plugin ../../plugin/libinsn.so$(COMMA)arg=inline \
+ -d plugin -D $*-with-libinsn.so.pout \
+ $(QEMU_OPTS) $*, \
+ "$* on $(TARGET_NAME)")
+
# Running
QEMU_OPTS+=-device isa-debugcon,chardev=output -device
isa-debug-exit,iobase=0xf4,iosize=0x4 -kernel
diff --git a/tests/tcg/i386/Makefile.target b/tests/tcg/i386/Makefile.target
index ad187cb2c9..c4a6f91966 100644
--- a/tests/tcg/i386/Makefile.target
+++ b/tests/tcg/i386/Makefile.target
@@ -48,6 +48,13 @@ else
SKIP_I386_TESTS+=test-i386-fprem
endif
+# non-inline runs will trigger the duplicate instruction heuristics in
libinsn.so
+run-plugin-%-with-libinsn.so:
+ $(call run-test, $@, $(QEMU) $(QEMU_OPTS) \
+ -plugin ../../plugin/libinsn.so$(COMMA)arg=inline \
+ -d plugin -D $*-with-libinsn.so.pout $*, \
+ "$* (inline) on $(TARGET_NAME)")
+
# Update TESTS
I386_TESTS:=$(filter-out $(SKIP_I386_TESTS), $(ALL_X86_TESTS))
TESTS=$(MULTIARCH_TESTS) $(I386_TESTS)
diff --git a/tests/tcg/x86_64/Makefile.softmmu-target
b/tests/tcg/x86_64/Makefile.softmmu-target
index 1bd763f2e6..9896319f0e 100644
--- a/tests/tcg/x86_64/Makefile.softmmu-target
+++ b/tests/tcg/x86_64/Makefile.softmmu-target
@@ -33,5 +33,15 @@ EXTRA_RUNS+=$(MULTIARCH_RUNS)
memory: CFLAGS+=-DCHECK_UNALIGNED=1
+# non-inline runs will trigger the duplicate instruction heuristics in
libinsn.so
+run-plugin-%-with-libinsn.so:
+ $(call run-test, $@, \
+ $(QEMU) -monitor none -display none \
+ -chardev file$(COMMA)path=$@.out$(COMMA)id=output \
+ -plugin ../../plugin/libinsn.so$(COMMA)arg=inline \
+ -d plugin -D $*-with-libinsn.so.pout \
+ $(QEMU_OPTS) $*, \
+ "$* on $(TARGET_NAME)")
+
# Running
QEMU_OPTS+=-device isa-debugcon,chardev=output -device
isa-debug-exit,iobase=0xf4,iosize=0x4 -kernel
--
2.20.1
[PULL 16/23] accel/tcg: actually cache our partial icount TB
When we exit a block under icount with instructions left to execute we
might need a shorter than normal block to take us to the next
deterministic event. Instead of creating a throwaway block on demand
we use the existing compile flags mechanism to ensure we fetch (or
compile and fetch) a block with exactly the number of instructions we
need.
Signed-off-by: Alex Bennée
Reviewed-by: Richard Henderson
Message-Id: <20210213130325.14781-17-alex.ben...@linaro.org>
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index f2819eec7d..d24c1bdb74 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -730,16 +730,17 @@ static inline void cpu_loop_exec_tb(CPUState *cpu,
TranslationBlock *tb,
/* Ensure global icount has gone forward */
icount_update(cpu);
/* Refill decrementer and continue execution. */
-insns_left = MIN(0x, cpu->icount_budget);
+insns_left = MIN(CF_COUNT_MASK, cpu->icount_budget);
cpu_neg(cpu)->icount_decr.u16.low = insns_left;
cpu->icount_extra = cpu->icount_budget - insns_left;
-if (!cpu->icount_extra && insns_left < tb->icount) {
-/* Execute any remaining instructions, then let the main loop
- * handle the next event.
- */
-if (insns_left > 0) {
-cpu_exec_nocache(cpu, insns_left, tb, false);
-}
+
+/*
+ * If the next tb has more instructions than we have left to
+ * execute we need to ensure we find/generate a TB with exactly
+ * insns_left instructions in it.
+ */
+if (!cpu->icount_extra && insns_left > 0 && insns_left < tb->icount) {
+cpu->cflags_next_tb = (tb->cflags & ~CF_COUNT_MASK) | insns_left;
}
#endif
}
--
2.20.1
[PULL 13/23] target/sh4: Create superh_io_recompile_replay_branch
From: Richard Henderson
Move the code from accel/tcg/translate-all.c to target/sh4/cpu.c.
Signed-off-by: Richard Henderson
Signed-off-by: Alex Bennée
Reviewed-by: Alex Bennée
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20210208233906.479571-5-richard.hender...@linaro.org>
Message-Id: <20210213130325.14781-14-alex.ben...@linaro.org>
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 9fea5c0e59..c0b98e76b9 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2418,9 +2418,6 @@ void tb_check_watchpoint(CPUState *cpu, uintptr_t retaddr)
*/
void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
{
-#if defined(TARGET_SH4)
-CPUArchState *env = cpu->env_ptr;
-#endif
TranslationBlock *tb;
CPUClass *cc;
uint32_t n;
@@ -2444,15 +2441,6 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
cpu_neg(cpu)->icount_decr.u16.low++;
n = 2;
}
-#if defined(TARGET_SH4)
-if ((env->flags & ((DELAY_SLOT | DELAY_SLOT_CONDITIONAL))) != 0
-&& env->pc != tb->pc) {
-env->pc -= 2;
-cpu_neg(cpu)->icount_decr.u16.low++;
-env->flags &= ~(DELAY_SLOT | DELAY_SLOT_CONDITIONAL);
-n = 2;
-}
-#endif
/* Generate a new TB executing the I/O insn. */
cpu->cflags_next_tb = curr_cflags() | CF_LAST_IO | n;
diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index a78d283bc8..ac65c88f1f 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -43,6 +43,23 @@ static void superh_cpu_synchronize_from_tb(CPUState *cs,
cpu->env.flags = tb->flags & TB_FLAG_ENVFLAGS_MASK;
}
+#ifndef CONFIG_USER_ONLY
+static bool superh_io_recompile_replay_branch(CPUState *cs,
+ const TranslationBlock *tb)
+{
+SuperHCPU *cpu = SUPERH_CPU(cs);
+CPUSH4State *env = &cpu->env;
+
+if ((env->flags & ((DELAY_SLOT | DELAY_SLOT_CONDITIONAL))) != 0
+&& env->pc != tb->pc) {
+env->pc -= 2;
+env->flags &= ~(DELAY_SLOT | DELAY_SLOT_CONDITIONAL);
+return true;
+}
+return false;
+}
+#endif
+
static bool superh_cpu_has_work(CPUState *cs)
{
return cs->interrupt_request & CPU_INTERRUPT_HARD;
@@ -217,6 +234,7 @@ static struct TCGCPUOps superh_tcg_ops = {
#ifndef CONFIG_USER_ONLY
.do_interrupt = superh_cpu_do_interrupt,
.do_unaligned_access = superh_cpu_do_unaligned_access,
+.io_recompile_replay_branch = superh_io_recompile_replay_branch,
#endif /* !CONFIG_USER_ONLY */
};
--
2.20.1
[PULL 23/23] tests/acceptance: add a memory callback check
This test makes sure that the inline and callback based memory checks
count the same number of accesses.
Signed-off-by: Alex Bennée
Reviewed-by: Philippe Mathieu-Daudé
Message-Id: <20210213130325.14781-24-alex.ben...@linaro.org>
diff --git a/tests/acceptance/tcg_plugins.py b/tests/acceptance/tcg_plugins.py
index b1ba10498f..c21bf9e52a 100644
--- a/tests/acceptance/tcg_plugins.py
+++ b/tests/acceptance/tcg_plugins.py
@@ -115,3 +115,34 @@ def test_aarch64_virt_insn_icount(self):
m = re.search(br"detected repeat execution @
(?P0x[0-9A-Fa-f]+)", s)
if m is not None and "addr" in m.groupdict():
self.fail("detected repeated instructions")
+
+def test_aarch64_virt_mem_icount(self):
+"""
+:avocado: tags=accel:tcg
+:avocado: tags=arch:aarch64
+:avocado: tags=machine:virt
+:avocado: tags=cpu:cortex-a57
+"""
+kernel_path = self._grab_aarch64_kernel()
+kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
+ 'console=ttyAMA0')
+console_pattern = 'Kernel panic - not syncing: VFS:'
+
+plugin_log = tempfile.NamedTemporaryFile(mode="r+t", prefix="plugin",
+ suffix=".log")
+
+self.run_vm(kernel_path, kernel_command_line,
+"tests/plugin/libmem.so,arg=both", plugin_log.name,
+console_pattern,
+args=('-cpu', 'cortex-a53', '-icount', 'shift=1'))
+
+with plugin_log as lf, \
+ mmap.mmap(lf.fileno(), 0, access=mmap.ACCESS_READ) as s:
+m = re.findall(br"mem accesses: (?P\d+)", s)
+if m is None or len(m) != 2:
+self.fail("no memory access counts found")
+else:
+inline = int(m[0])
+callback = int(m[1])
+if inline != callback:
+self.fail("mismatched access counts")
--
2.20.1
[PULL 09/23] accel/tcg/plugin-gen: fix the call signature for inline callbacks
A recent change to the handling of constants in TCG changed the
pattern of ops emitted for a constant add. We no longer emit a mov and
the constant can be applied directly to the TCG_op_add arguments. This
was causing SEGVs when running the insn plugin with arg=inline. Fix
this by updating copy_add_i64 to do the right thing while also adding
a comment at the top of the append section as an aide memoir if
something like this happens again.
Signed-off-by: Alex Bennée
Reviewed-by: Richard Henderson
Cc: Emilio G. Cota
Message-Id: <20210213130325.14781-10-alex.ben...@linaro.org>
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index e5dc9d0ca9..8a1bb801e0 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -320,22 +320,6 @@ static TCGOp *copy_const_ptr(TCGOp **begin_op, TCGOp *op,
void *ptr)
return op;
}
-static TCGOp *copy_const_i64(TCGOp **begin_op, TCGOp *op, uint64_t v)
-{
-if (TCG_TARGET_REG_BITS == 32) {
-/* 2x mov_i32 */
-op = copy_op(begin_op, op, INDEX_op_mov_i32);
-op->args[1] = tcgv_i32_arg(tcg_constant_i32(v));
-op = copy_op(begin_op, op, INDEX_op_mov_i32);
-op->args[1] = tcgv_i32_arg(tcg_constant_i32(v >> 32));
-} else {
-/* mov_i64 */
-op = copy_op(begin_op, op, INDEX_op_mov_i64);
-op->args[1] = tcgv_i64_arg(tcg_constant_i64(v));
-}
-return op;
-}
-
static TCGOp *copy_extu_tl_i64(TCGOp **begin_op, TCGOp *op)
{
if (TARGET_LONG_BITS == 32) {
@@ -374,14 +358,17 @@ static TCGOp *copy_st_i64(TCGOp **begin_op, TCGOp *op)
return op;
}
-static TCGOp *copy_add_i64(TCGOp **begin_op, TCGOp *op)
+static TCGOp *copy_add_i64(TCGOp **begin_op, TCGOp *op, uint64_t v)
{
if (TCG_TARGET_REG_BITS == 32) {
/* all 32-bit backends must implement add2_i32 */
g_assert(TCG_TARGET_HAS_add2_i32);
op = copy_op(begin_op, op, INDEX_op_add2_i32);
+op->args[4] = tcgv_i32_arg(tcg_constant_i32(v));
+op->args[5] = tcgv_i32_arg(tcg_constant_i32(v >> 32));
} else {
op = copy_op(begin_op, op, INDEX_op_add_i64);
+op->args[2] = tcgv_i64_arg(tcg_constant_i64(v));
}
return op;
}
@@ -431,6 +418,12 @@ static TCGOp *copy_call(TCGOp **begin_op, TCGOp *op, void
*empty_func,
return op;
}
+/*
+ * When we append/replace ops here we are sensitive to changing patterns of
+ * TCGOps generated by the tcg_gen_FOO calls when we generated the
+ * empty callbacks. This will assert very quickly in a debug build as
+ * we assert the ops we are replacing are the correct ones.
+ */
static TCGOp *append_udata_cb(const struct qemu_plugin_dyn_cb *cb,
TCGOp *begin_op, TCGOp *op, int *cb_idx)
{
@@ -462,11 +455,8 @@ static TCGOp *append_inline_cb(const struct
qemu_plugin_dyn_cb *cb,
/* ld_i64 */
op = copy_ld_i64(&begin_op, op);
-/* const_i64 */
-op = copy_const_i64(&begin_op, op, cb->inline_insn.imm);
-
/* add_i64 */
-op = copy_add_i64(&begin_op, op);
+op = copy_add_i64(&begin_op, op, cb->inline_insn.imm);
/* st_i64 */
op = copy_st_i64(&begin_op, op);
--
2.20.1
[PULL 20/23] accel/tcg: allow plugin instrumentation to be disable via cflags
When icount is enabled and we recompile an MMIO access we end up
double counting the instruction execution. To avoid this we introduce
the CF_MEMI cflag which only allows memory instrumentation for the
next TB (which won't yet have been counted). As this is part of the
hashed compile flags we will only execute the generated TB while
coming out of a cpu_io_recompile.
While we are at it delete the old TODO. We might as well keep the
translation handy as it's likely you will repeatedly hit it on each
MMIO access.
Reported-by: Aaron Lindsay
Signed-off-by: Alex Bennée
Tested-by: Aaron Lindsay
Reviewed-by: Richard Henderson
Message-Id: <20210213130325.14781-21-alex.ben...@linaro.org>
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 665fe68607..b7b3c0ef12 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -454,14 +454,14 @@ struct TranslationBlock {
uint32_t cflags;/* compile flags */
#define CF_COUNT_MASK 0x7fff
#define CF_LAST_IO 0x8000 /* Last insn may be an IO access. */
+#define CF_MEMI_ONLY 0x0001 /* Only instrument memory ops */
#define CF_USE_ICOUNT 0x0002
#define CF_INVALID 0x0004 /* TB is stale. Set with @jmp_lock held */
#define CF_PARALLEL0x0008 /* Generate code for a parallel context */
#define CF_CLUSTER_MASK 0xff00 /* Top 8 bits are cluster ID */
#define CF_CLUSTER_SHIFT 24
-/* cflags' mask for hashing/comparison */
-#define CF_HASH_MASK \
-(CF_COUNT_MASK | CF_LAST_IO | CF_USE_ICOUNT | CF_PARALLEL |
CF_CLUSTER_MASK)
+/* cflags' mask for hashing/comparison, basically ignore CF_INVALID */
+#define CF_HASH_MASK (~CF_INVALID)
/* Per-vCPU dynamic tracing state used to generate this TB */
uint32_t trace_vcpu_dstate;
diff --git a/include/exec/plugin-gen.h b/include/exec/plugin-gen.h
index 4834a9e2f4..b1b72b5d90 100644
--- a/include/exec/plugin-gen.h
+++ b/include/exec/plugin-gen.h
@@ -19,7 +19,7 @@ struct DisasContextBase;
#ifdef CONFIG_PLUGIN
-bool plugin_gen_tb_start(CPUState *cpu, const TranslationBlock *tb);
+bool plugin_gen_tb_start(CPUState *cpu, const TranslationBlock *tb, bool
supress);
void plugin_gen_tb_end(CPUState *cpu);
void plugin_gen_insn_start(CPUState *cpu, const struct DisasContextBase *db);
void plugin_gen_insn_end(void);
@@ -41,7 +41,7 @@ static inline void plugin_insn_append(const void *from,
size_t size)
#else /* !CONFIG_PLUGIN */
static inline
-bool plugin_gen_tb_start(CPUState *cpu, const TranslationBlock *tb)
+bool plugin_gen_tb_start(CPUState *cpu, const TranslationBlock *tb, bool
supress)
{
return false;
}
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index 841deed79c..c5a79a89f0 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -92,6 +92,7 @@ struct qemu_plugin_dyn_cb {
};
};
+/* Internal context for instrumenting an instruction */
struct qemu_plugin_insn {
GByteArray *data;
uint64_t vaddr;
@@ -99,6 +100,7 @@ struct qemu_plugin_insn {
GArray *cbs[PLUGIN_N_CB_TYPES][PLUGIN_N_CB_SUBTYPES];
bool calls_helpers;
bool mem_helper;
+bool mem_only;
};
/*
@@ -128,6 +130,7 @@ static inline struct qemu_plugin_insn
*qemu_plugin_insn_alloc(void)
return insn;
}
+/* Internal context for this TranslationBlock */
struct qemu_plugin_tb {
GPtrArray *insns;
size_t n;
@@ -135,6 +138,7 @@ struct qemu_plugin_tb {
uint64_t vaddr2;
void *haddr1;
void *haddr2;
+bool mem_only;
GArray *cbs[PLUGIN_N_CB_SUBTYPES];
};
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 8a1bb801e0..c3dc3effe7 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -842,7 +842,7 @@ static void plugin_gen_inject(const struct qemu_plugin_tb
*plugin_tb)
pr_ops();
}
-bool plugin_gen_tb_start(CPUState *cpu, const TranslationBlock *tb)
+bool plugin_gen_tb_start(CPUState *cpu, const TranslationBlock *tb, bool
mem_only)
{
struct qemu_plugin_tb *ptb = tcg_ctx->plugin_tb;
bool ret = false;
@@ -855,6 +855,7 @@ bool plugin_gen_tb_start(CPUState *cpu, const
TranslationBlock *tb)
ptb->vaddr2 = -1;
get_page_addr_code_hostp(cpu->env_ptr, tb->pc, &ptb->haddr1);
ptb->haddr2 = NULL;
+ptb->mem_only = mem_only;
plugin_gen_empty_callback(PLUGIN_GEN_FROM_TB);
}
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 464b3c3394..bbd919a393 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2400,7 +2400,8 @@ void tb_check_watchpoint(CPUState *cpu, uintptr_t retaddr)
}
#ifndef CONFIG_USER_ONLY
-/* in deterministic execution mode, instructions doing device I/Os
+/*
+ * In deterministic execution mode, instructions doing device I/Os
* must be at the end of the TB.
*
* Called by softmmu_template.h, with iothread mutex not held.
@@ -2431,19 +2432,18 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
n = 2;
}
-/* Genera
[PULL 17/23] accel/tcg: cache single instruction TB on pending replay exception
Again there is no reason to jump through the nocache hoops to execute
a single instruction block. We do have to add an additional wrinkle to
the cpu_handle_interrupt case to ensure we let through a TB where we
have specifically disabled icount for the block.
As the last user of cpu_exec_nocache we can now remove the function.
Further clean-up will follow in subsequent patches.
Signed-off-by: Alex Bennée
Reviewed-by: Richard Henderson
Message-Id: <20210213130325.14781-18-alex.ben...@linaro.org>
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index d24c1bdb74..16e4fe3ccd 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -224,40 +224,6 @@ cpu_tb_exec(CPUState *cpu, TranslationBlock *itb, int
*tb_exit)
return last_tb;
}
-#ifndef CONFIG_USER_ONLY
-/* Execute the code without caching the generated code. An interpreter
- could be used if available. */
-static void cpu_exec_nocache(CPUState *cpu, int max_cycles,
- TranslationBlock *orig_tb, bool ignore_icount)
-{
-TranslationBlock *tb;
-uint32_t cflags = curr_cflags() | CF_NOCACHE;
-int tb_exit;
-
-if (ignore_icount) {
-cflags &= ~CF_USE_ICOUNT;
-}
-
-/* Should never happen.
- We only end up here when an existing TB is too long. */
-cflags |= MIN(max_cycles, CF_COUNT_MASK);
-
-mmap_lock();
-tb = tb_gen_code(cpu, orig_tb->pc, orig_tb->cs_base,
- orig_tb->flags, cflags);
-tb->orig_tb = orig_tb;
-mmap_unlock();
-
-/* execute the generated code */
-trace_exec_tb_nocache(tb, tb->pc);
-cpu_tb_exec(cpu, tb, &tb_exit);
-
-mmap_lock();
-tb_phys_invalidate(tb, -1);
-mmap_unlock();
-tcg_tb_remove(tb);
-}
-#endif
static void cpu_exec_enter(CPUState *cpu)
{
@@ -524,15 +490,12 @@ static inline bool cpu_handle_exception(CPUState *cpu,
int *ret)
#ifndef CONFIG_USER_ONLY
if (replay_has_exception()
&& cpu_neg(cpu)->icount_decr.u16.low + cpu->icount_extra == 0) {
-/* try to cause an exception pending in the log */
-cpu_exec_nocache(cpu, 1, tb_find(cpu, NULL, 0, curr_cflags()),
true);
+/* Execute just one insn to trigger exception pending in the log */
+cpu->cflags_next_tb = (curr_cflags() & ~CF_USE_ICOUNT) | 1;
}
#endif
-if (cpu->exception_index < 0) {
-return false;
-}
+return false;
}
-
if (cpu->exception_index >= EXCP_INTERRUPT) {
/* exit request from the cpu execution loop */
*ret = cpu->exception_index;
@@ -688,6 +651,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
/* Finally, check if we need to exit to the main loop. */
if (unlikely(qatomic_read(&cpu->exit_request))
|| (icount_enabled()
+&& (cpu->cflags_next_tb == -1 || cpu->cflags_next_tb &
CF_USE_ICOUNT)
&& cpu_neg(cpu)->icount_decr.u16.low + cpu->icount_extra == 0)) {
qatomic_set(&cpu->exit_request, 0);
if (cpu->exception_index == -1) {
--
2.20.1
RE: [RFC v7 26/26] vfio/pci: Implement return_page_response page response callback
Hi Eric,
> -Original Message-
> From: Eric Auger [mailto:eric.au...@redhat.com]
> Sent: 16 November 2020 18:14
> To: eric.auger@gmail.com; eric.au...@redhat.com;
> qemu-devel@nongnu.org; qemu-...@nongnu.org;
> alex.william...@redhat.com
> Cc: peter.mayd...@linaro.org; jean-phili...@linaro.org; pet...@redhat.com;
> jacob.jun@linux.intel.com; yi.l@intel.com; Shameerali Kolothum Thodi
> ; t...@semihalf.com;
> nicoleots...@gmail.com; yuzenghui ;
> zhangfei@gmail.com; vivek.gau...@arm.com
> Subject: [RFC v7 26/26] vfio/pci: Implement return_page_response page
> response callback
>
> This patch implements the page response path. The
> response s written into the page response ring buffer and then
> update header's head index is updated. This path is not used
> by this series. It is introduced here as a POC for vSVA/ARM
> integration.
>
> Signed-off-by: Eric Auger
> ---
> hw/vfio/pci.h | 2 +
> hw/vfio/pci.c | 121
> ++
> 2 files changed, 123 insertions(+)
>
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 350e9e9005..ce0472611e 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -147,6 +147,8 @@ struct VFIOPCIDevice {
> VFIOPCIExtIRQ *ext_irqs;
> VFIORegion dma_fault_region;
> uint32_t fault_tail_index;
> +VFIORegion dma_fault_response_region;
> +uint32_t fault_response_head_index;
> int (*resetfn)(struct VFIOPCIDevice *);
> uint32_t vendor_id;
> uint32_t device_id;
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 4e3495bb60..797acd9c73 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2631,6 +2631,61 @@ out:
> g_free(fault_region_info);
> }
>
> +static void vfio_init_fault_response_regions(VFIOPCIDevice *vdev, Error
> **errp)
> +{
> +struct vfio_region_info *fault_region_info = NULL;
> +struct vfio_region_info_cap_fault *cap_fault;
> +VFIODevice *vbasedev = &vdev->vbasedev;
> +struct vfio_info_cap_header *hdr;
> +char *fault_region_name;
> +int ret;
> +
> +ret = vfio_get_dev_region_info(&vdev->vbasedev,
> + VFIO_REGION_TYPE_NESTED,
> +
> VFIO_REGION_SUBTYPE_NESTED_DMA_FAULT_RESPONSE,
> + &fault_region_info);
> +if (ret) {
> +goto out;
> +}
> +
> +hdr = vfio_get_region_info_cap(fault_region_info,
> +
> VFIO_REGION_INFO_CAP_DMA_FAULT);
VFIO_REGION_INFO_CAP_DMA_FAULT_RESPONSE ?
> +if (!hdr) {
> +error_setg(errp, "failed to retrieve DMA FAULT RESPONSE
> capability");
> +goto out;
> +}
> +cap_fault = container_of(hdr, struct vfio_region_info_cap_fault,
> + header);
> +if (cap_fault->version != 1) {
> +error_setg(errp, "Unsupported DMA FAULT RESPONSE API
> version %d",
> + cap_fault->version);
> +goto out;
> +}
> +
> +fault_region_name = g_strdup_printf("%s DMA FAULT RESPONSE %d",
> +vbasedev->name,
> +fault_region_info->index);
> +
> +ret = vfio_region_setup(OBJECT(vdev), vbasedev,
> +&vdev->dma_fault_response_region,
> +fault_region_info->index,
> +fault_region_name);
> +g_free(fault_region_name);
> +if (ret) {
> +error_setg_errno(errp, -ret,
> + "failed to set up the DMA FAULT RESPONSE
> region %d",
> + fault_region_info->index);
> +goto out;
> +}
> +
> +ret = vfio_region_mmap(&vdev->dma_fault_response_region);
> +if (ret) {
> +error_setg_errno(errp, -ret, "Failed to mmap the DMA FAULT
> RESPONSE queue");
> +}
> +out:
> +g_free(fault_region_info);
> +}
> +
> static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
> {
> VFIODevice *vbasedev = &vdev->vbasedev;
> @@ -2706,6 +2761,12 @@ static void vfio_populate_device(VFIOPCIDevice
> *vdev, Error **errp)
> return;
> }
>
> +vfio_init_fault_response_regions(vdev, &err);
> +if (err) {
> +error_propagate(errp, err);
> +return;
> +}
> +
> irq_info.index = VFIO_PCI_ERR_IRQ_INDEX;
>
> ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
> @@ -2884,8 +2945,68 @@ static int vfio_iommu_set_pasid_table(PCIBus
> *bus, int32_t devfn,
> return ioctl(container->fd, VFIO_IOMMU_SET_PASID_TABLE, &info);
> }
>
> +static int vfio_iommu_return_page_response(PCIBus *bus, int32_t devfn,
> + IOMMUPageResponse
> *resp)
> +{
> +PCIDevice *pdev = bus->devices[devfn];
> +VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> +struct iommu_page_response *response = &resp->resp;
> +struct vfio_region_dma_fault_response header;
> +struct iommu_page_response *queue;
> +char *queue_buffer = NULL;
> +ssize_t b
Re: [PATCH] Correct CRIS TCG register lifetime management
On Thu, Feb 18, 2021 at 08:56:43AM +, Stefan Sandström wrote:
> From: Stefan Sandstrom
>
> Add and fix deallocation of temporary TCG registers in CRIS code
> generation.
Thanks Stefan,
Unfortunately, this patch does not apply. I'm not sure why.
Perhaps it got corrupted by the email systems along the way.
git am -s ~/Mail/stefan.sandstrom
Applying: Correct CRIS TCG register lifetime management
error: corrupt patch at line 11
Patch failed at 0001 Correct CRIS TCG register lifetime management
hint: Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-mailer: Apple Mail (2.3654.40.0.2.32)
x-ms-exchange-messagesentrepresentingtype: 1
x-ms-exchange-transport-fromentityheader: Hosted
How did you send out the patch? Can you try git-send-email?
Best regards,
Edgar
>
> Change-Id: I17fce5d95bdc4418337ba885d53ba97afb1bafcc
> Signed-off-by: Stefan Sandstrom
> ---
> target/cris/translate.c | 146 ++--
> target/cris/translate_v10.c.inc | 70 ---
> 2 files changed, 156 insertions(+), 60 deletions(-)
>
> diff --git a/target/cris/translate.c b/target/cris/translate.c
> index c893f87..ae903a5 100644
> --- a/target/cris/translate.c
> +++ b/target/cris/translate.c
> @@ -177,9 +177,13 @@ static inline void t_gen_mov_TN_preg(TCGv tn, int r)
> {
> assert(r >= 0 && r <= 15);
> if (r == PR_BZ || r == PR_WZ || r == PR_DZ) {
> -tcg_gen_mov_tl(tn, tcg_const_tl(0));
> +TCGv c0 = tcg_const_tl(0);
> +tcg_gen_mov_tl(tn, c0);
> +tcg_temp_free(c0);
> } else if (r == PR_VR) {
> -tcg_gen_mov_tl(tn, tcg_const_tl(32));
> +TCGv c32 = tcg_const_tl(32);
> +tcg_gen_mov_tl(tn, c32);
> +tcg_temp_free(c32);
> } else {
> tcg_gen_mov_tl(tn, cpu_PR[r]);
> }
> @@ -255,8 +259,10 @@ static int cris_fetch(CPUCRISState *env, DisasContext
> *dc, uint32_t addr,
>
> static void cris_lock_irq(DisasContext *dc)
> {
> +TCGv c1 = tcg_const_tl(1);
> dc->clear_locked_irq = 0;
> -t_gen_mov_env_TN(locked_irq, tcg_const_tl(1));
> +t_gen_mov_env_TN(locked_irq, c1);
> +tcg_temp_free(c1);
> }
>
> static inline void t_gen_raise_exception(uint32_t index)
> @@ -885,8 +891,10 @@ static void gen_tst_cc (DisasContext *dc, TCGv cc, int
> cond)
> case CC_EQ:
> if ((arith_opt || move_opt)
> && dc->cc_x_uptodate != (2 | X_FLAG)) {
> +TCGv c0 = tcg_const_tl(0);
> tcg_gen_setcond_tl(TCG_COND_EQ, cc,
> -cc_result, tcg_const_tl(0));
> +cc_result, c0);
> +tcg_temp_free(c0);
> } else {
> cris_evaluate_flags(dc);
> tcg_gen_andi_tl(cc,
> @@ -1330,14 +1338,17 @@ static int dec_addoq(CPUCRISState *env, DisasContext
> *dc)
> }
> static int dec_addq(CPUCRISState *env, DisasContext *dc)
> {
> +TCGv c;
> LOG_DIS("addq %u, $r%u\n", dc->op1, dc->op2);
>
> dc->op1 = EXTRACT_FIELD(dc->ir, 0, 5);
>
> cris_cc_mask(dc, CC_MASK_NZVC);
>
> +c = tcg_const_tl(dc->op1);
> cris_alu(dc, CC_OP_ADD,
> -cpu_R[dc->op2], cpu_R[dc->op2], tcg_const_tl(dc->op1), 4);
> +cpu_R[dc->op2], cpu_R[dc->op2], c, 4);
> +tcg_temp_free(c);
> return 2;
> }
> static int dec_moveq(CPUCRISState *env, DisasContext *dc)
> @@ -1353,62 +1364,77 @@ static int dec_moveq(CPUCRISState *env, DisasContext
> *dc)
> }
> static int dec_subq(CPUCRISState *env, DisasContext *dc)
> {
> +TCGv c;
> dc->op1 = EXTRACT_FIELD(dc->ir, 0, 5);
>
> LOG_DIS("subq %u, $r%u\n", dc->op1, dc->op2);
>
> cris_cc_mask(dc, CC_MASK_NZVC);
> +c = tcg_const_tl(dc->op1);
> cris_alu(dc, CC_OP_SUB,
> -cpu_R[dc->op2], cpu_R[dc->op2], tcg_const_tl(dc->op1), 4);
> +cpu_R[dc->op2], cpu_R[dc->op2], c, 4);
> +tcg_temp_free(c);
> return 2;
> }
> static int dec_cmpq(CPUCRISState *env, DisasContext *dc)
> {
> uint32_t imm;
> +TCGv c;
> dc->op1 = EXTRACT_FIELD(dc->ir, 0, 5);
> imm = sign_extend(dc->op1, 5);
>
> LOG_DIS("cmpq %d, $r%d\n", imm, dc->op2);
> cris_cc_mask(dc, CC_MASK_NZVC);
>
> +c = tcg_const_tl(imm);
> cris_alu(dc, CC_OP_CMP,
> -cpu_R[dc->op2], cpu_R[dc->op2], tcg_const_tl(imm), 4);
> +cpu_R[dc->op2], cpu_R[dc->op2], c, 4);
> +tcg_temp_free(c);
> return 2;
> }
> static int dec_andq(CPUCRISState *env, DisasContext *dc)
> {
> uint32_t imm;
> +TCGv c;
> dc->op1 = EXTRACT_FIELD(dc->ir, 0, 5);
> imm = sign_extend(dc->op1, 5);
>
> LOG_DIS("andq %d, $r%d\n", imm, dc->op2);
> cris_cc_mask(dc, CC_MASK_NZ);
>
> +c = tcg_const_tl(imm);
> cris_alu(dc, CC_OP_AND,
> -cpu_R[dc->op2], cpu_R[dc->op2],
Re: [PATCH v2 5/6] qapi: Add support for aliases
Kevin Wolf writes:
> Am 17.02.2021 um 16:23 hat Markus Armbruster geschrieben:
>> Kevin Wolf writes:
>>
>> > Introduce alias definitions for object types (structs and unions). This
>> > allows using the same QAPI type and visitor for many syntax variations
>> > that exist in the external representation, like between QMP and the
>> > command line. It also provides a new tool for evolving the schema while
>> > maintaining backwards compatibility during a deprecation period.
>> >
>> > Signed-off-by: Kevin Wolf
>> [...]
>> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
>> > index 22e62df901..e370485f6e 100644
>> > --- a/scripts/qapi/visit.py
>> > +++ b/scripts/qapi/visit.py
>> > @@ -26,6 +26,7 @@ from .common import (
>> > from .gen import QAPISchemaModularCVisitor, ifcontext
>> > from .schema import (
>> > QAPISchema,
>> > +QAPISchemaAlias,
>> > QAPISchemaEnumMember,
>> > QAPISchemaEnumType,
>> > QAPISchemaFeature,
>> > @@ -60,7 +61,8 @@ bool visit_type_%(c_name)s_members(Visitor *v,
>> > %(c_name)s *obj, Error **errp);
>> > def gen_visit_object_members(name: str,
>> > base: Optional[QAPISchemaObjectType],
>> > members: List[QAPISchemaObjectTypeMember],
>> > - variants: Optional[QAPISchemaVariants]) ->
>> > str:
>> > + variants: Optional[QAPISchemaVariants],
>> > + aliases: List[QAPISchemaAlias]) -> str:
>> > ret = mcgen('''
>> >
>> > bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error
>> > **errp)
>> > @@ -68,6 +70,24 @@ bool visit_type_%(c_name)s_members(Visitor *v,
>> > %(c_name)s *obj, Error **errp)
>> > ''',
>> > c_name=c_name(name))
>> >
>> > +if aliases:
>> > +ret += mcgen('''
>> > +visit_start_alias_scope(v);
>> > +''')
>> > +
>> > +for a in aliases:
>> > +if a.name:
>> > +name = '"%s"' % a.name
>> > +else:
>> > +name = "NULL"
>> > +
>> > +source = ", ".join('"%s"' % x for x in a.source)
>> > +
>> > +ret += mcgen('''
>> > +visit_define_alias(v, %(name)s, (const char * []) { %(source)s, NULL
>> > });
>> > +''',
>> > + name=name, source=source)
>> > +
>> > if base:
>> > ret += mcgen('''
>> > if (!visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, errp)) {
>> > @@ -133,6 +153,11 @@ bool visit_type_%(c_name)s_members(Visitor *v,
>> > %(c_name)s *obj, Error **errp)
>> > }
>> > ''')
>> >
>> > +if aliases:
>> > +ret += mcgen('''
>> > +visit_end_alias_scope(v);
>> > +''')
>> > +
>> > ret += mcgen('''
>> > return true;
>> > }
>>
>> The visit_type_FOO_members() are primarily helpers for the
>> visit_type_FOO(). But they also get called
>>
>> * by visit_type_BAR_members() when
>>
>> - struct or union BAR has 'base': 'FOO'
>> - union or alternate BAR a variant 'FOO'
>>
>> * by qmp_marshal_CMD() when
>>
>> - CMD has 'boxed': true, 'data': 'FOO'
>>
>> Have you considered these cases?
>>
>> How's the test coverage?
>
> What is the difference between these cases? The visiting should work the
> same, no matter from where it was started.
>
> I did consider the struct base/union variant case and this is why I
> introduced visit_start/end_alias_scope so that aliases wouldn't leak to
> the outer level.
Good!
The qmp_marshal_CMD()'s code around visit_type_FOO_members() is fairly
close to visit_type_FOO()'s code: it avoids allocation and drops cases
that can't happen with the visitor at hand. Can't see why aliases would
not work.
"Can't see why it would not work" is of course a sensible precondition
for testing, not an excuse for not testing.
> Now that I'm trying to think of a test case, this probably only protects
> against weird corner cases: The input object is the same anyway, so I
> guess the only way for this to make a difference is when the base struct
> defines an alias for a member that it doesn't even have (so the alias
> would remain unused when applied to the base struct independently), but
> that exists in the struct in which it is embedded.
It's best to strangle such weird corner cases in the crib, before they
can conspire with other weird corner cases to make a real mess.
> I hope adding a test case that checks that this is an error should be
> easy. Would the right place be tests/test-qobject-input-visitor.c?
Sounds good to me.
> Can you think of any other specific differences that need to be tested?
I think you nailed the negative tests.
The positive tests are limited to frontend tests now, i.e. put some
valid use into qapi-schema-test.json, then check the resulting
qapi-schema-test.out looks sane. Covered there, I think:
* empty 'aliases' have no effect: AliasStruct0
Aside: we could choose to outlaw instead.
* alias a struct member: AliasStruct1
* alias a member of a struct member: Al
Re: [RFC PATCH 0/3] hw/pflash_cfi01: Reduce memory consumption when flash image is smaller than region
On Tuesday, 2021-02-16 at 15:53:48 GMT, David Edmondson wrote: > On Tuesday, 2021-02-16 at 16:44:58 +01, Philippe Mathieu-Daudé wrote: > >> On 2/16/21 4:22 PM, David Edmondson wrote: >>> On Tuesday, 2021-02-16 at 16:03:05 +01, Philippe Mathieu-Daudé wrote: >>> I am not a block expert, but I wonder if something like this could be used: - create a raw (parent) block image of 64MiB - add a raw (child) block with your 768kB of VARS file - add a null-co (child) block of 63Mib + 256kiB - pass the parent block to the pflash device >>> >>> I'm not clear how this would behave if there is a write to the device at >>> (say) 1MiB. >> >> Discarded. >> >>> More philosophically, how should it behave? >> >> null-co: reads return zeroes, writes are discarded. >> >>> My expectation was that if the machine says that there is 64MiB of >>> writable flash, we have to allow writes throughout the full 64MiB and >>> (significantly) persist them to the backing block device. >>> >>> Just because the backing block device started out 768KiB big doesn't >>> mean that we should not write to the remaining extent if that's what the >>> VM attempts. >>> >>> Would the above approach achieve that? (It doesn't sound like it.) >> >> Well this was a simple suggestion if you know your guest won't access >> anything beside these 768KiB (IIRC AAVMF "consumes" the flash devices >> and doesn't expose them to the guest via ACPI/other). > > If that's the case, mirroring the approach in the patch that I sent > should work - we run the 768kiB as a subregion with IO/ROMD behaviour, > fail or discard writes to the rest and read as zero. > >> If you are into memory optimization, this is worth considering. >> Else it doesn't make sense. >> >> AAVMF is designed for virtual world. Is the virtual world interested in >> doing firmware upgrade on the CODE flash? Unlikely, if you want to >> upgrade AAVMF you'd do it on the host. Why not mount the CODE flash as >> ROM? I suppose because AAVMF does CFI MMIO accesses to detect the flash, >> but I wonder what is the point if this flash will be ever written... > > I don't expect that the CODE flash will ever be written by the guest, no > (it's generally presented to the guest as read-only), and currently the > VARS flash is also often presented read-only as well, but I don't think > that is a situation that we can maintain given increasing use (DBX > updates, for example). > > If it's acceptable to limit flash writes to the extent of the underlying > block device then things are straightforward. It's not clear to me that > this is the case. Looking at the AAVMF implementation, it seems to me that if the initial VARS image is prepared as being 768KiB in size (which it is), none of AAVMF itself will attempt to write outside of that extent. If that is correct, it would support the idea of allowing writes only to the sub-region of the 64MiB that is covered by the backing block device. This would allow the same approach as used for read-only devices to be used for read-write devices - it's mostly a matter of deleting some code from the patch that I sent. The additional patch on top of the previous series would be: https://p.dme.org/0004-hw-pflash_cfi01-Allow-devices-to-have-a-smaller-back.patch.html (Presumably this would just be squashed into the previous patch in reality.) If AAVMF is not going to try to use any of the region outside of the VARS, do we need to persist writes to the rest of the region? If so, would it be acceptable for those writes to persist only for the lifetime of a QEMU instance? (Memory could be allocated "on demand" so that a read after write would behave, but the content would be thrown away when QEMU exits.) dme. -- Everyone I know, goes away in the end.
Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw
On Thu, 18 Feb 2021 10:23:16 +0100
Thomas Huth wrote:
> > Since the virtio-gpu-ccw device depends on the hw-display-virtio-gpu
> > module, which provides the type virtio-gpu-device, packaging the
> > hw-display-virtio-gpu module as a separate package that may or may not
> > be installed along with the qemu package leads to problems. Namely if
> > the hw-display-virtio-gpu is absent, qemu continues to advertise
> > virtio-gpu-ccw, but it aborts not only when one attempts using
> > virtio-gpu-ccw, but also when libvirtd's capability probing tries
> > to instantiate the type to introspect it.
> >
> > Let us thus introduce a module named hw-s390x-virtio-gpu-ccw that
> > is going to provide the virtio-gpu-ccw device. The hw-s390x prefix
> > was chosen because it is not a portable device.
> >
> > With virtio-gpu-ccw built as a module, the correct way to package a
> > modularized qemu is to require that hw-display-virtio-gpu must be
> > installed whenever the module hw-s390x-virtio-gpu-ccw.
> >
> > Signed-off-by: Halil Pasic
> > ---
> > hw/s390x/meson.build | 17 -
> > util/module.c| 1 +
> > 2 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> > index 2a7818d94b..153b1309fb 100644
> > --- a/hw/s390x/meson.build
> > +++ b/hw/s390x/meson.build
> > @@ -34,7 +34,6 @@ virtio_ss.add(files('virtio-ccw.c'))
> > virtio_ss.add(when: 'CONFIG_VIRTIO_BALLOON', if_true:
> > files('virtio-ccw-balloon.c'))
> > virtio_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true:
> > files('virtio-ccw-blk.c'))
> > virtio_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true:
> > files('virtio-ccw-crypto.c'))
> > -virtio_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true:
> > files('virtio-ccw-gpu.c'))
> > virtio_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true:
> > files('virtio-ccw-input.c'))
> > virtio_ss.add(when: 'CONFIG_VIRTIO_NET', if_true:
> > files('virtio-ccw-net.c'))
> > virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true:
> > files('virtio-ccw-rng.c'))
> > @@ -46,3 +45,19 @@ virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true:
> > files('vhost-user-fs-ccw.c'
> > s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
> >
> > hw_arch += {'s390x': s390x_ss}
> > +
> > +if target.startswith('s390x')
> > + hw_s390x_modules = {}
> > +
> > + hw_s390x_modules_c_args = ['-DNEED_CPU_H',
> > + '-DCONFIG_TARGET="@0@-config-target.h"'.format(target)]
> > + hw_s390x_modules_inc = [include_directories('../../target' /
> > config_target['TARGET_BASE_ARCH'])]
> > + hw_s390x_modules_dependencies = declare_dependency(
> > + include_directories: hw_s390x_modules_inc, compile_args:
> > hw_s390x_modules_c_args)
>
> Basically the patch looks fine to me, but I wonder why all that above lines
> (related to hw_s390x_modules_dependencies) are requred at all? The other
> display modules in hw/display/meson.build also do not need to re-define
> c_args for example?
The explanation is simple. Unlike most devices, the ccw devices aren't
portable. In particular both css.c and css.h includes "cpu.h", and
virtio-ccw-gpu.c includes "qemu/osdep.h". Furthermore osdep.h contains:
#ifdef NEED_CPU_H
#include CONFIG_TARGET
#else
#include "exec/poison.h"
#endif
so if we don't have NEED_CPU_H, among others CONFIG_KVM is poisoned, and
CONFIG_KVM is used in "css.h". Frankly, I can't tell under what circumstances
does css need "cpu.h". I managed to build the s390x-softmmu target
without it, but decided to put it back. Regarding "osdep.h", I just
assumed includes are done the way they are done for a good reason. Maybe
the includes can be changed in a way that the things you ask about become
unnecessary, but with the code as is they are necessary. Try to drop them
and check out what happens.
Regards,
Halil
Re: [RFC v7 26/26] vfio/pci: Implement return_page_response page response callback
Hi Shameer,
On 2/18/21 11:19 AM, Shameerali Kolothum Thodi wrote:
> Hi Eric,
>
>> -Original Message-
>> From: Eric Auger [mailto:eric.au...@redhat.com]
>> Sent: 16 November 2020 18:14
>> To: eric.auger@gmail.com; eric.au...@redhat.com;
>> qemu-devel@nongnu.org; qemu-...@nongnu.org;
>> alex.william...@redhat.com
>> Cc: peter.mayd...@linaro.org; jean-phili...@linaro.org; pet...@redhat.com;
>> jacob.jun@linux.intel.com; yi.l@intel.com; Shameerali Kolothum Thodi
>> ; t...@semihalf.com;
>> nicoleots...@gmail.com; yuzenghui ;
>> zhangfei@gmail.com; vivek.gau...@arm.com
>> Subject: [RFC v7 26/26] vfio/pci: Implement return_page_response page
>> response callback
>>
>> This patch implements the page response path. The
>> response s written into the page response ring buffer and then
>> update header's head index is updated. This path is not used
>> by this series. It is introduced here as a POC for vSVA/ARM
>> integration.
>>
>> Signed-off-by: Eric Auger
>> ---
>> hw/vfio/pci.h | 2 +
>> hw/vfio/pci.c | 121
>> ++
>> 2 files changed, 123 insertions(+)
>>
>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>> index 350e9e9005..ce0472611e 100644
>> --- a/hw/vfio/pci.h
>> +++ b/hw/vfio/pci.h
>> @@ -147,6 +147,8 @@ struct VFIOPCIDevice {
>> VFIOPCIExtIRQ *ext_irqs;
>> VFIORegion dma_fault_region;
>> uint32_t fault_tail_index;
>> +VFIORegion dma_fault_response_region;
>> +uint32_t fault_response_head_index;
>> int (*resetfn)(struct VFIOPCIDevice *);
>> uint32_t vendor_id;
>> uint32_t device_id;
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 4e3495bb60..797acd9c73 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2631,6 +2631,61 @@ out:
>> g_free(fault_region_info);
>> }
>>
>> +static void vfio_init_fault_response_regions(VFIOPCIDevice *vdev, Error
>> **errp)
>> +{
>> +struct vfio_region_info *fault_region_info = NULL;
>> +struct vfio_region_info_cap_fault *cap_fault;
>> +VFIODevice *vbasedev = &vdev->vbasedev;
>> +struct vfio_info_cap_header *hdr;
>> +char *fault_region_name;
>> +int ret;
>> +
>> +ret = vfio_get_dev_region_info(&vdev->vbasedev,
>> + VFIO_REGION_TYPE_NESTED,
>> +
>> VFIO_REGION_SUBTYPE_NESTED_DMA_FAULT_RESPONSE,
>> + &fault_region_info);
>> +if (ret) {
>> +goto out;
>> +}
>> +
>> +hdr = vfio_get_region_info_cap(fault_region_info,
>> +
>> VFIO_REGION_INFO_CAP_DMA_FAULT);
>
> VFIO_REGION_INFO_CAP_DMA_FAULT_RESPONSE ?
yes!
>
>> +if (!hdr) {
>> +error_setg(errp, "failed to retrieve DMA FAULT RESPONSE
>> capability");
>> +goto out;
>> +}
>> +cap_fault = container_of(hdr, struct vfio_region_info_cap_fault,
>> + header);
>> +if (cap_fault->version != 1) {
>> +error_setg(errp, "Unsupported DMA FAULT RESPONSE API
>> version %d",
>> + cap_fault->version);
>> +goto out;
>> +}
>> +
>> +fault_region_name = g_strdup_printf("%s DMA FAULT RESPONSE %d",
>> +vbasedev->name,
>> +fault_region_info->index);
>> +
>> +ret = vfio_region_setup(OBJECT(vdev), vbasedev,
>> +&vdev->dma_fault_response_region,
>> +fault_region_info->index,
>> +fault_region_name);
>> +g_free(fault_region_name);
>> +if (ret) {
>> +error_setg_errno(errp, -ret,
>> + "failed to set up the DMA FAULT RESPONSE
>> region %d",
>> + fault_region_info->index);
>> +goto out;
>> +}
>> +
>> +ret = vfio_region_mmap(&vdev->dma_fault_response_region);
>> +if (ret) {
>> +error_setg_errno(errp, -ret, "Failed to mmap the DMA FAULT
>> RESPONSE queue");
>> +}
>> +out:
>> +g_free(fault_region_info);
>> +}
>> +
>> static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
>> {
>> VFIODevice *vbasedev = &vdev->vbasedev;
>> @@ -2706,6 +2761,12 @@ static void vfio_populate_device(VFIOPCIDevice
>> *vdev, Error **errp)
>> return;
>> }
>>
>> +vfio_init_fault_response_regions(vdev, &err);
>> +if (err) {
>> +error_propagate(errp, err);
>> +return;
>> +}
>> +
>> irq_info.index = VFIO_PCI_ERR_IRQ_INDEX;
>>
>> ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
>> @@ -2884,8 +2945,68 @@ static int vfio_iommu_set_pasid_table(PCIBus
>> *bus, int32_t devfn,
>> return ioctl(container->fd, VFIO_IOMMU_SET_PASID_TABLE, &info);
>> }
>>
>> +static int vfio_iommu_return_page_response(PCIBus *bus, int32_t devfn,
>> + IOMMUPageResponse
>> *resp)
>> +{
>> +PCIDevice *pdev = bus->devices[devfn];
>> +VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev
Re: [PATCH 08/24] DAX: virtio-fs: Fill in slave commands for mapping
* Stefan Hajnoczi (stefa...@redhat.com) wrote:
> On Tue, Feb 09, 2021 at 07:02:08PM +, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert"
> >
> > Fill in definitions for map, unmap and sync commands.
> >
> > Signed-off-by: Dr. David Alan Gilbert
> > with fix by misono.tomoh...@fujitsu.com
> > ---
> > hw/virtio/vhost-user-fs.c | 115 --
> > 1 file changed, 111 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> > index 78401d2ff1..5f2fca4d82 100644
> > --- a/hw/virtio/vhost-user-fs.c
> > +++ b/hw/virtio/vhost-user-fs.c
> > @@ -37,15 +37,122 @@
> > uint64_t vhost_user_fs_slave_map(struct vhost_dev *dev,
> > VhostUserFSSlaveMsg *sm,
> > int fd)
> > {
> > -/* TODO */
> > -return (uint64_t)-1;
> > +VHostUserFS *fs = VHOST_USER_FS(dev->vdev);
> > +if (!fs) {
> > +/* Shouldn't happen - but seen on error path */
> > +error_report("Bad fs ptr");
> > +return (uint64_t)-1;
> > +}
>
> If a non-vhost-user-fs vhost-user device backend sends this message
> VHOST_USER_FS() -> object_dynamic_cast_assert() there will either be an
> assertion failure (CONFIG_QOM_CAST_DEBUG) or the pointer will be
> silently cast to the wrong type (!CONFIG_QOM_CAST_DEBUG).
>
> Both of these outcomes are not suitable for input validation. We need to
> fail cleanly here:
>
> VhostUserFS *fs = (VHostUserFS *)object_dynamic_cast(OBJECT(dev->vdev),
>TYPE_VHOST_USER_FS);
> if (!fs) {
> ...handle failure...
> }
>
> > uint64_t vhost_user_fs_slave_unmap(struct vhost_dev *dev,
> > VhostUserFSSlaveMsg *sm)
> > {
> > -/* TODO */
> > -return (uint64_t)-1;
> > +VHostUserFS *fs = VHOST_USER_FS(dev->vdev);
> > +if (!fs) {
> > +/* Shouldn't happen - but seen on error path */
> > +error_report("Bad fs ptr");
> > +return (uint64_t)-1;
> > +}
>
> Same here.
Thanks, fixed.
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: Measuring the impact of buffer copy for virtio-gpu guests
On Wed, Feb 17, 2021 at 01:46:28PM +, Alex Bennée wrote: > Hi Gerd, > > I was in a discussion with the AGL folks today talking about approaches > to achieving zero-copy when running VirGL virtio guests. AIUI (which is > probably not very much) the reasons for copy can be due to a number of > reasons: > > - the GPA not being mapped to a HPA that is accessible to the final HW > - the guest allocation of a buffer not meeting stride/alignment requirements > - data needing to be transformed for consumption by the real hardware? With the current qemu code base each ressource has both a guest and host buffer and the data is copied over when the guest asks for it. virtio-gpu got a new feature (VIRTIO_GPU_F_RESOURCE_BLOB) to improve that. For blob resources we have stride/alignment negotiation, and they can also be allocated by the host and mapped into the guest address space instead of living in guest ram. linux guest support is there in the kernel and mesa, host side is supported by crosvm. qemu doesn't support blob resources though. > I'm curious if it's possible to measure the effect of these extra copies > and where do they occur? Do all resources get copied from the guest buffer to > host or does this only occur when there is a mismatch in the buffer > requirements? Without blob resources a copy is required whenever the guest cpu wants access to the resource (i.e. glWritePixels / glReadPixels + simliar). For resources which are a gpu render target and never touched by the cpu this is not needed. For these you wouldn't even need guest ram backing storage (VIRTIO_GPU_CMD_RESOURCE_ATTACH_BACKING), linux doesn't implement that optimization though. > Are there any functions where I could add trace points to measure this? > If this occurs in the kernel I wonder if I could use an eBPF probe to > count the number of bytes copied? Copy happens in qemu or virglrenderer, in response to VIRTIO_GPU_CMD_TRANSFER_* commands from the guest. There are tracepoint already in qemu (trace_virtio_gpu_cmd_res_xfer_*), they log only the resource id though, not the amount of data transfered. Tracing on the guest side by adding trace points to the kernel shouldn't be hard too. take care, Gerd
[PATCH] virtio-iommu: Default to bypass during boot
Currently the virtio-iommu device must be programmed before it allows
DMA from any PCI device. This can make the VM entirely unusable when a
virtio-iommu driver isn't present, for example in a bootloader that
loads the OS from storage.
Similarly to the other vIOMMU implementations, default to DMA bypassing
the IOMMU during boot. Add a "boot-bypass" option that lets users change
this behavior.
Signed-off-by: Jean-Philippe Brucker
---
include/hw/virtio/virtio-iommu.h | 1 +
hw/virtio/virtio-iommu.c | 23 +--
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
index 273e35c04bc..4c66989ca4e 100644
--- a/include/hw/virtio/virtio-iommu.h
+++ b/include/hw/virtio/virtio-iommu.h
@@ -58,6 +58,7 @@ struct VirtIOIOMMU {
GTree *domains;
QemuMutex mutex;
GTree *endpoints;
+bool boot_bypass;
};
#endif
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index c2883a2f6c8..cd08dc39eca 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -689,6 +689,25 @@ static void virtio_iommu_report_fault(VirtIOIOMMU *viommu,
uint8_t reason,
}
+static bool virtio_iommu_bypass_is_allowed(VirtIOIOMMU *s)
+{
+VirtIODevice *vdev = &s->parent_obj;
+
+/*
+ * Allow bypass if:
+ * - boot_bypass is enabled and the BYPASS feature hasn't yet been
+ * acknowledged.
+ * - the BYPASS feature has been negotiated.
+ */
+if (s->boot_bypass && !(vdev->status & VIRTIO_CONFIG_S_FEATURES_OK)) {
+return true;
+}
+if (virtio_vdev_has_feature(vdev, VIRTIO_IOMMU_F_BYPASS)) {
+return true;
+}
+return false;
+}
+
static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
IOMMUAccessFlags flag,
int iommu_idx)
@@ -715,8 +734,7 @@ static IOMMUTLBEntry
virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
.perm = IOMMU_NONE,
};
-bypass_allowed = virtio_vdev_has_feature(&s->parent_obj,
- VIRTIO_IOMMU_F_BYPASS);
+bypass_allowed = virtio_iommu_bypass_is_allowed(s);
sid = virtio_iommu_get_bdf(sdev);
@@ -1156,6 +1174,7 @@ static const VMStateDescription vmstate_virtio_iommu = {
static Property virtio_iommu_properties[] = {
DEFINE_PROP_LINK("primary-bus", VirtIOIOMMU, primary_bus, "PCI", PCIBus *),
+DEFINE_PROP_BOOL("boot-bypass", VirtIOIOMMU, boot_bypass, true),
DEFINE_PROP_END_OF_LIST(),
};
--
2.30.0
Re: [PULL 00/35] hexagon initial commit
On Wed, 17 Feb 2021 at 23:40, Richard Henderson wrote: > > The following changes since commit f0f75dc174b6c79eb78a161d1c0921f82d7f1bf0: > > Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' > into staging (2021-02-17 13:04:48 +) > > are available in the Git repository at: > > https://gitlab.com/rth7680/qemu.git tags/pull-hex-20210217 > > for you to fetch changes up to 91bdc01a235a0065411e29ad78859587fee85bc6: > > Hexagon build infrastructure (2021-02-17 12:55:21 -0800) > > > Initial commit for the Qualcomm Hexagon processor. > > Fails to build on the FreeBSD and NetBSD VMs: Configuring config-host.h using configuration Program scripts/hxtool found: YES Program scripts/shaderinclude.pl found: YES Program scripts/qapi-gen.py found: YES Program scripts/qemu-version.sh found: YES Run-time dependency threads found: YES Program keycodemapdb/tools/keymap-gen found: YES Program scripts/decodetree.py found: YES Program python found: NO ../src/target/hexagon/meson.build:20:0: ERROR: python3 not found A full log can be found at /usr/home/qemu/qemu-test.FAG4lS/build/meson-logs/meson-log.txt ERROR: meson setup failed Something isn't honouring the configure --python option. (On these BSDs, python3 isn't "python3", it's eg "python3.7".) The top level meson.build does get this right: Program sh found: YES Program python3 found: YES (/usr/local/bin/python3.7) Program bzip2 found: YES I think target/hexagon/meson.build should be using the python that the top level meson.build found, not going out and trying to find its own one. thanks -- PMM
RE: [RFC v7 26/26] vfio/pci: Implement return_page_response page response callback
Hi Eric,
> -Original Message-
> From: Auger Eric [mailto:eric.au...@redhat.com]
> Sent: 18 February 2021 10:42
> To: Shameerali Kolothum Thodi ;
> eric.auger@gmail.com; qemu-devel@nongnu.org; qemu-...@nongnu.org;
> alex.william...@redhat.com
> Cc: peter.mayd...@linaro.org; jacob.jun@linux.intel.com; Zengtao (B)
> ; jean-phili...@linaro.org; t...@semihalf.com;
> pet...@redhat.com; nicoleots...@gmail.com; vivek.gau...@arm.com;
> yi.l@intel.com; zhangfei@gmail.com; yuzenghui
> ; qubingbing
> Subject: Re: [RFC v7 26/26] vfio/pci: Implement return_page_response page
> response callback
>
[...]
> > Also, I just noted that this patch breaks the dev hot add/del functionality.
> > device_add works fine but device_del is not removing the dev cleanly.Thank
> you for reporting this!
>
> The test matrix becomes bigger and bigger :-( I Need to write some
> avocado-vt tests or alike.
>
> I am currently working on the respin. At the moment I investigate the
> DPDK issue that you reported and I was able to reproduce.
Ok. Good to know that it is reproducible.
> I intend to rebase on top of Jean-Philippe's
> [PATCH v12 00/10] iommu: I/O page faults for SMMUv3
>
> Is that good enough for your SVA integration or do you want I prepare a
> rebase on some extended code?
Could you please try to base it on
https://jpbrucker.net/git/linux/log/?h=sva/current
I think that has the latest from Jean-Philippe and will be easy to add
uacce/zip specific patches to test SVA/vSVA.
Thanks,
Shameer
> Thanks
>
> Eric
> >
> > The below one fixes it. Please check.
> >
> > Thanks,
> > Shameer
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 797acd9c73..92c1d48316 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -3470,6 +3470,7 @@ static void vfio_instance_finalize(Object *obj)
> > vfio_display_finalize(vdev);
> > vfio_bars_finalize(vdev);
> > vfio_region_finalize(&vdev->dma_fault_region);
> > +vfio_region_finalize(&vdev->dma_fault_response_region);
> > g_free(vdev->emulated_config_bits);
> > g_free(vdev->rom);
> > /*
> > @@ -3491,6 +3492,7 @@ static void vfio_exitfn(PCIDevice *pdev)
> > vfio_unregister_err_notifier(vdev);
> > vfio_unregister_ext_irq_notifiers(vdev);
> > vfio_region_exit(&vdev->dma_fault_region);
> > +vfio_region_exit(&vdev->dma_fault_response_region);
> > pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
> > if (vdev->irqchip_change_notifier.notify) {
> >
> kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_not
> >
> >
> >
Re: [Rust-VMM] Call for Google Summer of Code 2021 project ideas
On 2/17/21 1:20 PM, Stefan Hajnoczi wrote: > CAUTION: This email originated from outside of the organization. Do not click > links or open attachments unless you can confirm the sender and know the > content is safe. > > > > Thanks, I have published the rust-vmm project ideas on the wiki: > https://wiki.qemu.org/Google_Summer_of_Code_2021 Thanks, Stefan! > > Please see Sergio's reply about virtio-consoile is libkrun. Maybe it > affects the project idea? I synced offline with Sergio. It seems I've misread his comment. The code is already available in libkrun, and to port it to rust-vmm will likely take just a couple of days. We explored the option of also requesting to implement VIRTIO_CONSOLE_F_MULTIPORT to make it an appropriate GSoC project, but we decided this is not a good idea since it doesn't look like that feature is useful for the projects consuming rust-vmm. It also adds complexity, and we would need to maintain that as well. Since it would still be nice to have virtio-console in rust-vmm, I'll just open an issue in vm-virtio and label it with "help wanted" so people can pick it up. Can we remove the virtio-console project from the list of GSoC ideas? Also, i...@amazon.com will like to help with mentoring the GSoC projects. Can he be added as a co-mentor of: "Mocking framework for Virtio Queues"? Sorry for the ping-pong, and thanks again for everything! Andreea Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005. 0x9B51563C1FA36782.asc Description: application/pgp-keys
[PULL 2/2] usb/pcap: set flag_setup
Without that wireshark complains about invalid control setup data for non-control transfers. Signed-off-by: Gerd Hoffmann Message-Id: <20210216144939.841873-1-kra...@redhat.com> Signed-off-by: Gerd Hoffmann --- hw/usb/pcap.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/usb/pcap.c b/hw/usb/pcap.c index 4350989d3a71..dbff00be252e 100644 --- a/hw/usb/pcap.c +++ b/hw/usb/pcap.c @@ -127,6 +127,7 @@ static void do_usb_pcap_ctrl(FILE *fp, USBPacket *p, bool setup) .xfer_type = usbmon_xfer_type[USB_ENDPOINT_XFER_CONTROL], .epnum = in ? 0x80 : 0, .devnum = dev->addr, +.flag_setup = setup ? 0 : '-', .flag_data = '=', .length = dev->setup_len, }; @@ -169,6 +170,7 @@ static void do_usb_pcap_data(FILE *fp, USBPacket *p, bool setup) .xfer_type = usbmon_xfer_type[p->ep->type], .epnum = usbmon_epnum(p), .devnum = p->ep->dev->addr, +.flag_setup = '-', .flag_data = '=', .length = p->iov.size, }; -- 2.29.2
[PULL 1/2] usb-host: use correct altsetting in usb_host_ep_update
From: Nick Rosbrook
In order to keep track of the alternate setting that should be used for
a given interface, the USBDevice struct keeps an array of alternate
setting values, which is indexed by the interface number. In
usb_host_set_interface, when this array is updated, usb_host_ep_update
is called as a result. However, when usb_host_ep_update accesses the
active libusb_config_descriptor, it indexes udev->altsetting with the
loop variable, rather than the interface number.
With the simple trace backend enable, this behavior can be seen:
[...]
usb_xhci_xfer_start 0.440 pid=1215 xfer=0x5596a4b85930 slotid=0x1 epid=0x1
streamid=0x0
usb_packet_state_change 1.703 pid=1215 bus=0x1 port=b'1' ep=0x0
p=0x5596a4b85938 o=b'undef' n=b'setup'
usb_host_req_control 2.269 pid=1215 bus=0x1 addr=0x5 p=0x5596a4b85938
req=0x10b value=0x1 index=0xd
usb_host_set_interface 0.449 pid=1215 bus=0x1 addr=0x5 interface=0xd alt=0x1
usb_host_parse_config 2542.648 pid=1215 bus=0x1 addr=0x5 value=0x2 active=0x1
usb_host_parse_interface 1.804 pid=1215 bus=0x1 addr=0x5 num=0xc alt=0x0
active=0x1
usb_host_parse_endpoint 2.012 pid=1215 bus=0x1 addr=0x5 ep=0x2 dir=b'in'
type=b'int' active=0x1
usb_host_parse_interface 1.598 pid=1215 bus=0x1 addr=0x5 num=0xd alt=0x0
active=0x1
usb_host_req_emulated 3.593 pid=1215 bus=0x1 addr=0x5 p=0x5596a4b85938
status=0x0
usb_packet_state_change 2.550 pid=1215 bus=0x1 port=b'1' ep=0x0
p=0x5596a4b85938 o=b'setup' n=b'complete'
usb_xhci_xfer_success 4.298 pid=1215 xfer=0x5596a4b85930 bytes=0x0
[...]
In particular, it is seen that although usb_host_set_interface sets the
alternate setting of interface 0xd to 0x1, usb_host_ep_update uses 0x0
as the alternate setting due to using the incorrect index to
udev->altsetting.
Fix this problem by getting the interface number from the active
libusb_config_descriptor, and then using that as the index to
udev->altsetting.
Signed-off-by: Nick Rosbrook
Message-Id: <20210201213021.500277-1-rosbro...@ainfosec.com>
Signed-off-by: Gerd Hoffmann
---
hw/usb/host-libusb.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 7dde3d12069e..354713a77dc9 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -836,7 +836,7 @@ static void usb_host_ep_update(USBHostDevice *s)
struct libusb_ss_endpoint_companion_descriptor *endp_ss_comp;
#endif
uint8_t devep, type;
-int pid, ep;
+int pid, ep, alt;
int rc, i, e;
usb_ep_reset(udev);
@@ -848,8 +848,20 @@ static void usb_host_ep_update(USBHostDevice *s)
conf->bConfigurationValue, true);
for (i = 0; i < conf->bNumInterfaces; i++) {
-assert(udev->altsetting[i] < conf->interface[i].num_altsetting);
-intf = &conf->interface[i].altsetting[udev->altsetting[i]];
+/*
+ * The udev->altsetting array indexes alternate settings
+ * by the interface number. Get the 0th alternate setting
+ * first so that we can grab the interface number, and
+ * then correct the alternate setting value if necessary.
+ */
+intf = &conf->interface[i].altsetting[0];
+alt = udev->altsetting[intf->bInterfaceNumber];
+
+if (alt != 0) {
+assert(alt < conf->interface[i].num_altsetting);
+intf = &conf->interface[i].altsetting[alt];
+}
+
trace_usb_host_parse_interface(s->bus_num, s->addr,
intf->bInterfaceNumber,
intf->bAlternateSetting, true);
--
2.29.2
Re: [PATCH] usb-host: use correct altsetting in usb_host_ep_update
On Thu, Feb 11, 2021 at 11:05:41AM -0500, Nick Rosbrook wrote: > Hi, > > Just wanted to ping this. Patchwork link is here: > https://patchwork.kernel.org/project/qemu-devel/patch/20210201213021.500277-1-rosbro...@ainfosec.com/. Pull request sent now. Not much usb activity these days ... thanks, Gerd
[PULL 0/2] Usb 20210218 patches
The following changes since commit 18543229fd7a2c79dcd6818c7b1f0f62512b5220: Merge remote-tracking branch 'remotes/cleber-gitlab/tags/python-next-pull-r= equest' into staging (2021-02-16 14:37:57 +) are available in the Git repository at: git://git.kraxel.org/qemu tags/usb-20210218-pull-request for you to fetch changes up to 6ba5a437ad48f10931592f649b5b7375968f362d: usb/pcap: set flag_setup (2021-02-17 14:29:12 +0100) usb: two bugfixes. Gerd Hoffmann (1): usb/pcap: set flag_setup Nick Rosbrook (1): usb-host: use correct altsetting in usb_host_ep_update hw/usb/host-libusb.c | 18 +++--- hw/usb/pcap.c| 2 ++ 2 files changed, 17 insertions(+), 3 deletions(-) --=20 2.29.2
Re: [PATCH 01/24] DAX: vhost-user: Rework slave return values
* Stefan Hajnoczi (stefa...@redhat.com) wrote:
> On Tue, Feb 09, 2021 at 07:02:01PM +, Dr. David Alan Gilbert (git) wrote:
> > +static uint64_t vhost_user_slave_handle_vring_host_notifier(
> > +struct vhost_dev *dev,
> > + VhostUserVringArea *area,
> > + int fd)
>
> Indentation looks off. Only worth changing if you respin.
Done.
> > @@ -1398,7 +1399,8 @@ static void slave_read(void *opaque)
> > struct vhost_user *u = dev->opaque;
> > VhostUserHeader hdr = { 0, };
> > VhostUserPayload payload = { 0, };
> > -int size, ret = 0;
> > +int size;
> > +uint64_t ret = 0;
> > struct iovec iov;
> > struct msghdr msgh;
> > int fd[VHOST_USER_SLAVE_MAX_FDS];
> > @@ -1472,7 +1474,7 @@ static void slave_read(void *opaque)
> > break;
> > default:
> > error_report("Received unexpected msg type: %d.", hdr.request);
> > -ret = -EINVAL;
> > +ret = (uint64_t)-EINVAL;
>
> The !!ret was removed below so it would have previously been true (1).
> Now it has changed value.
>
> If there is no specific reason to change the value, please keep it true
> (1) just in case a vhost-user device backend depends on that value.
Done; although moving to errnos there feels a bit better to me; but yes
the callers aren't audited.
Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw
On Thu, 18 Feb 2021 11:34:38 +0100
Halil Pasic wrote:
> On Thu, 18 Feb 2021 10:23:16 +0100
> Thomas Huth wrote:
>
> > > Since the virtio-gpu-ccw device depends on the hw-display-virtio-gpu
> > > module, which provides the type virtio-gpu-device, packaging the
> > > hw-display-virtio-gpu module as a separate package that may or may not
> > > be installed along with the qemu package leads to problems. Namely if
> > > the hw-display-virtio-gpu is absent, qemu continues to advertise
> > > virtio-gpu-ccw, but it aborts not only when one attempts using
> > > virtio-gpu-ccw, but also when libvirtd's capability probing tries
> > > to instantiate the type to introspect it.
> > >
> > > Let us thus introduce a module named hw-s390x-virtio-gpu-ccw that
> > > is going to provide the virtio-gpu-ccw device. The hw-s390x prefix
> > > was chosen because it is not a portable device.
> > >
> > > With virtio-gpu-ccw built as a module, the correct way to package a
> > > modularized qemu is to require that hw-display-virtio-gpu must be
> > > installed whenever the module hw-s390x-virtio-gpu-ccw.
> > >
> > > Signed-off-by: Halil Pasic
> > > ---
> > > hw/s390x/meson.build | 17 -
> > > util/module.c| 1 +
> > > 2 files changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> > > index 2a7818d94b..153b1309fb 100644
> > > --- a/hw/s390x/meson.build
> > > +++ b/hw/s390x/meson.build
> > > @@ -34,7 +34,6 @@ virtio_ss.add(files('virtio-ccw.c'))
> > > virtio_ss.add(when: 'CONFIG_VIRTIO_BALLOON', if_true:
> > > files('virtio-ccw-balloon.c'))
> > > virtio_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true:
> > > files('virtio-ccw-blk.c'))
> > > virtio_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true:
> > > files('virtio-ccw-crypto.c'))
> > > -virtio_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true:
> > > files('virtio-ccw-gpu.c'))
> > > virtio_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true:
> > > files('virtio-ccw-input.c'))
> > > virtio_ss.add(when: 'CONFIG_VIRTIO_NET', if_true:
> > > files('virtio-ccw-net.c'))
> > > virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true:
> > > files('virtio-ccw-rng.c'))
> > > @@ -46,3 +45,19 @@ virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true:
> > > files('vhost-user-fs-ccw.c'
> > > s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
> > >
> > > hw_arch += {'s390x': s390x_ss}
> > > +
> > > +if target.startswith('s390x')
> > > + hw_s390x_modules = {}
> > > +
> > > + hw_s390x_modules_c_args = ['-DNEED_CPU_H',
> > > + '-DCONFIG_TARGET="@0@-config-target.h"'.format(target)]
> > > + hw_s390x_modules_inc = [include_directories('../../target' /
> > > config_target['TARGET_BASE_ARCH'])]
> > > + hw_s390x_modules_dependencies = declare_dependency(
> > > +include_directories: hw_s390x_modules_inc, compile_args:
> > > hw_s390x_modules_c_args)
> >
> > Basically the patch looks fine to me, but I wonder why all that above lines
> > (related to hw_s390x_modules_dependencies) are requred at all? The other
> > display modules in hw/display/meson.build also do not need to re-define
> > c_args for example?
>
> The explanation is simple. Unlike most devices, the ccw devices aren't
> portable. In particular both css.c and css.h includes "cpu.h", and
> virtio-ccw-gpu.c includes "qemu/osdep.h". Furthermore osdep.h contains:
> #ifdef NEED_CPU_H
> #include CONFIG_TARGET
> #else
> #include "exec/poison.h"
> #endif
> so if we don't have NEED_CPU_H, among others CONFIG_KVM is poisoned, and
> CONFIG_KVM is used in "css.h". Frankly, I can't tell under what circumstances
> does css need "cpu.h".
s390_crw_mchk() and s390_io_interrupt() are in cpu.h. Nowadays, they
use the flic to inject interrupts; but their earlier implementations
had a dummy cpu state.
I'm wondering whether s390_flic.h is a better place for functions
injecting floating interrupts, now that we don't have the non-flic
support anymore.
> I managed to build the s390x-softmmu target
> without it, but decided to put it back. Regarding "osdep.h", I just
> assumed includes are done the way they are done for a good reason. Maybe
> the includes can be changed in a way that the things you ask about become
> unnecessary, but with the code as is they are necessary. Try to drop them
> and check out what happens.
>
> Regards,
> Halil
>
Re: [PATCH 1/5] qcow2-bitmap: make bytes_covered_by_bitmap_cluster() public
On 2/16/21 7:45 PM, Vladimir Sementsov-Ogievskiy wrote:
> Rename bytes_covered_by_bitmap_cluster() to
> bdrv_dirty_bitmap_serialization_coverage() and make it public. It is
> needed as we are going to make load_bitmap_data() public in the next
> commit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> include/block/dirty-bitmap.h | 2 ++
> block/dirty-bitmap.c | 13 +
> block/qcow2-bitmap.c | 16 ++--
> 3 files changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 36e8da4fc2..f581cf9fd7 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -57,6 +57,8 @@ void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
> uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
>uint64_t offset, uint64_t
> bytes);
> uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap
> *bitmap);
> +uint64_t bdrv_dirty_bitmap_serialization_coverage(int serialized_chunk_size,
> +const BdrvDirtyBitmap *bitmap);
> void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
>uint8_t *buf, uint64_t offset,
>uint64_t bytes);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 9b9cd71238..a0eaa28785 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -726,6 +726,19 @@ uint64_t bdrv_dirty_bitmap_serialization_align(const
> BdrvDirtyBitmap *bitmap)
> return hbitmap_serialization_align(bitmap->bitmap);
> }
>
> +/* Return the disk size covered by a chunk of serialized bitmap data. */
> +uint64_t bdrv_dirty_bitmap_serialization_coverage(int serialized_chunk_size,
> + const BdrvDirtyBitmap
> *bitmap)
> +{
> +uint64_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
> +uint64_t limit = granularity * (serialized_chunk_size << 3);
> +
> +assert(QEMU_IS_ALIGNED(limit,
> + bdrv_dirty_bitmap_serialization_align(bitmap)));
> +return limit;
> +}
> +
> +
> void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
>uint8_t *buf, uint64_t offset,
>uint64_t bytes)
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 5eef82fa55..42d81c44cd 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -278,18 +278,6 @@ static int free_bitmap_clusters(BlockDriverState *bs,
> Qcow2BitmapTable *tb)
> return 0;
> }
>
> -/* Return the disk size covered by a single qcow2 cluster of bitmap data. */
> -static uint64_t bytes_covered_by_bitmap_cluster(const BDRVQcow2State *s,
> -const BdrvDirtyBitmap
> *bitmap)
> -{
> -uint64_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
> -uint64_t limit = granularity * (s->cluster_size << 3);
> -
> -assert(QEMU_IS_ALIGNED(limit,
> - bdrv_dirty_bitmap_serialization_align(bitmap)));
> -return limit;
> -}
> -
> /* load_bitmap_data
> * @bitmap_table entries must satisfy specification constraints.
> * @bitmap must be cleared */
> @@ -312,7 +300,7 @@ static int load_bitmap_data(BlockDriverState *bs,
> }
>
> buf = g_malloc(s->cluster_size);
> -limit = bytes_covered_by_bitmap_cluster(s, bitmap);
> +limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size,
> bitmap);
> for (i = 0, offset = 0; i < tab_size; ++i, offset += limit) {
> uint64_t count = MIN(bm_size - offset, limit);
> uint64_t entry = bitmap_table[i];
> @@ -1303,7 +1291,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
> }
>
> buf = g_malloc(s->cluster_size);
> -limit = bytes_covered_by_bitmap_cluster(s, bitmap);
> +limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size,
> bitmap);
> assert(DIV_ROUND_UP(bm_size, limit) == tb_size);
>
> offset = 0;
Reviewed-by: Denis V. Lunev
Editing QEMU POWER Platform wiki page
Hi there, I would like to edit the wiki page at [0] as it contains some outdated information. Could anyone that has access to the wiki please help me create a user so that I can edit it? 0. https://wiki.qemu.org/Documentation/Platforms/POWER Cheers, Leo
Re: [PATCH] usb-host: use correct altsetting in usb_host_ep_update
On Thu, Feb 18, 2021 at 12:52:51PM +0100, Gerd Hoffmann wrote: > On Thu, Feb 11, 2021 at 11:05:41AM -0500, Nick Rosbrook wrote: > > Hi, > > > > Just wanted to ping this. Patchwork link is here: > > https://patchwork.kernel.org/project/qemu-devel/patch/20210201213021.500277-1-rosbro...@ainfosec.com/. > > Pull request sent now. > Not much usb activity these days ... Thanks! NR
Re: [PULL 00/19] riscv-to-apply queue
On Thu, 18 Feb 2021 at 01:59, Alistair Francis wrote:
>
> The following changes since commit 1af5629673bb5c1592d993f9fb6119a62845f576:
>
> Merge remote-tracking branch
> 'remotes/dgilbert-gitlab/tags/pull-virtiofs-20210216' into staging
> (2021-02-17 14:44:18 +)
>
> are available in the Git repository at:
>
> g...@github.com:alistair23/qemu.git tags/pull-riscv-to-apply-20210217-1
>
> for you to fetch changes up to d0867d2dad4125d2295b28d6f91fa49cf034ffd2:
>
> hw/riscv: virt: Map high mmio for PCIe (2021-02-17 17:47:19 -0800)
>
>
> RISC-V PR for 6.0
>
> This PR is a collection of RISC-V patches:
> - Improvements to SiFive U OTP
> - Upgrade OpenSBI to v0.9
> - Support the QMP dump-guest-memory
> - Add support for the SiFive SPI controller (sifive_u)
> - Initial RISC-V system documentation
> - A fix for the Goldfish RTC
> - MAINTAINERS updates
> - Support for high PCIe memory in the virt machine
Fails to compile, 32 bit hosts:
../../hw/riscv/virt.c: In function 'virt_machine_init':
../../hw/riscv/virt.c:621:43: error: comparison is always false due to
limited range of data type [-Werror=type-limits]
if ((uint64_t)(machine->ram_size) > 10 * GiB) {
^
../../hw/riscv/virt.c:623:33: error: large integer implicitly
truncated to unsigned type [-Werror=overflow]
machine->ram_size = 10 * GiB;
^~
-- PMM
Re: [RFC v7 26/26] vfio/pci: Implement return_page_response page response callback
Hi Shameer,
On 2/18/21 12:46 PM, Shameerali Kolothum Thodi wrote:
>
> Hi Eric,
>
>> -Original Message-
>> From: Auger Eric [mailto:eric.au...@redhat.com]
>> Sent: 18 February 2021 10:42
>> To: Shameerali Kolothum Thodi ;
>> eric.auger@gmail.com; qemu-devel@nongnu.org; qemu-...@nongnu.org;
>> alex.william...@redhat.com
>> Cc: peter.mayd...@linaro.org; jacob.jun@linux.intel.com; Zengtao (B)
>> ; jean-phili...@linaro.org; t...@semihalf.com;
>> pet...@redhat.com; nicoleots...@gmail.com; vivek.gau...@arm.com;
>> yi.l@intel.com; zhangfei@gmail.com; yuzenghui
>> ; qubingbing
>> Subject: Re: [RFC v7 26/26] vfio/pci: Implement return_page_response page
>> response callback
>>
> [...]
>
>>> Also, I just noted that this patch breaks the dev hot add/del functionality.
>>> device_add works fine but device_del is not removing the dev cleanly.Thank
>> you for reporting this!
>>
>> The test matrix becomes bigger and bigger :-( I Need to write some
>> avocado-vt tests or alike.
>>
>> I am currently working on the respin. At the moment I investigate the
>> DPDK issue that you reported and I was able to reproduce.
>
> Ok. Good to know that it is reproducible.
>
>> I intend to rebase on top of Jean-Philippe's
>> [PATCH v12 00/10] iommu: I/O page faults for SMMUv3
>>
>> Is that good enough for your SVA integration or do you want I prepare a
>> rebase on some extended code?
>
> Could you please try to base it on
> https://jpbrucker.net/git/linux/log/?h=sva/current
OK. At least I will provide a branch.
Eric
>
> I think that has the latest from Jean-Philippe and will be easy to add
> uacce/zip specific patches to test SVA/vSVA.
>
> Thanks,
> Shameer
>
>
>> Thanks
>>
>> Eric
>>>
>>> The below one fixes it. Please check.
>>>
>>> Thanks,
>>> Shameer
>>>
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index 797acd9c73..92c1d48316 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -3470,6 +3470,7 @@ static void vfio_instance_finalize(Object *obj)
>>> vfio_display_finalize(vdev);
>>> vfio_bars_finalize(vdev);
>>> vfio_region_finalize(&vdev->dma_fault_region);
>>> +vfio_region_finalize(&vdev->dma_fault_response_region);
>>> g_free(vdev->emulated_config_bits);
>>> g_free(vdev->rom);
>>> /*
>>> @@ -3491,6 +3492,7 @@ static void vfio_exitfn(PCIDevice *pdev)
>>> vfio_unregister_err_notifier(vdev);
>>> vfio_unregister_ext_irq_notifiers(vdev);
>>> vfio_region_exit(&vdev->dma_fault_region);
>>> +vfio_region_exit(&vdev->dma_fault_response_region);
>>> pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>>> if (vdev->irqchip_change_notifier.notify) {
>>>
>> kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_not
>>>
>>>
>>>
>
Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw
Hi, > > The explanation is simple. Unlike most devices, the ccw devices aren't > > portable. In particular both css.c and css.h includes "cpu.h", and > > virtio-ccw-gpu.c includes "qemu/osdep.h". Furthermore osdep.h contains: > > #ifdef NEED_CPU_H > > #include CONFIG_TARGET > > #else > > #include "exec/poison.h" > > #endif > > so if we don't have NEED_CPU_H, among others CONFIG_KVM is poisoned, and > > CONFIG_KVM is used in "css.h". Frankly, I can't tell under what > > circumstances > > does css need "cpu.h". > > s390_crw_mchk() and s390_io_interrupt() are in cpu.h. Nowadays, they > use the flic to inject interrupts; but their earlier implementations > had a dummy cpu state. > > I'm wondering whether s390_flic.h is a better place for functions > injecting floating interrupts, now that we don't have the non-flic > support anymore. Sounds like the easiest way forward. Alternatively add support for target-specific modules (which we don't really have right now). (modular-izing virtio-gpu-ccw makes sense indeed, virtio-gpu-pci is a module for pretty much the same reasons). take care, Gerd
Re: [PATCH v2 4/6] qapi: Apply aliases in qobject-input-visitor
Kevin Wolf writes:
> Am 17.02.2021 um 16:32 hat Markus Armbruster geschrieben:
>> Kevin Wolf writes:
>>
>> > When looking for an object in a struct in the external representation,
>> > check not only the currently visited struct, but also whether an alias
>> > in the current StackObject matches and try to fetch the value from the
>> > alias then. Providing two values for the same object through different
>> > aliases is an error.
>> >
>> > Signed-off-by: Kevin Wolf
>>
>> Looking just at qobject_input_try_get_object() for now.
>
> :-(
>
> This patch doesn't even feel that complicated to me.
I suspect it's just me having an unusually obtuse week.
The code is straightforward enough. What I'm missing is a bit of "how
does this accomplish the goal" and "why is this safe" here and there.
> Old: Get the value from the QDict of the current StackObject with the
> given name.
>
> New: First do alias resolution (with find_object_member), which results
> in a StackObject and a name, and that's the QDict and key where you get
> the value from.
This part I understand.
We simultaneously walk the QAPI type and the input QObject, consuming
input as we go.
Whenever the walk leaves a QAPI object type, we check the corresponding
QObject has been consumed completely.
With aliases, we additionally look for input in a certain enclosing
input object (i.e. up the recursion stack). If found, consume.
> Minor complication: Aliases can refer to members of nested objects that
> may not be provided in the input. But we want these to work.
>
> For example, my chardev series defines aliases to flatten
> SocketAddressLegacy (old syntax, I haven't rebased it yet):
>
> { 'union': 'SocketAddressLegacy',
> 'data': {
> 'inet': 'InetSocketAddress',
> 'unix': 'UnixSocketAddress',
> 'vsock': 'VsockSocketAddress',
> 'fd': 'String' },
> 'aliases': [
> {'source': ['data']},
> {'alias': 'fd', 'source': ['data', 'str']}
> ]}
>
> Of course, the idea is that this input should work:
>
> { 'type': 'inet', 'hostname': 'localhost', 'port': '1234' }
>
> However, without implicit objects, parsing 'data' fails because it
> expects an object, but none is given (we specified all of its members on
> the top level through aliases). What we would have to give is:
>
> { 'type': 'inet', 'hostname': 'localhost', 'port': '1234', 'data': {} }
>
> And _that_ would work. Visiting 'data' succeeds because we now have the
> object that the visitor expects, and when visiting its members, the
> aliases fill in all of the mandatory values.
>
> So what this patch does is to implicitly assume the 'data': {} instead
> of erroring out when we know that aliases exist that can still provide
> values for the content of 'data'.
Aliases exist than can still provide, but will they? What if input is
{"type": "inet"}
?
Your explanation makes me guess this is equivalent to
{"type": "inet", "data": {}}
which fails the visit, because mandatory members of "data" are missing.
Fine.
If we make the members optional, it succeeds: qobject_input_optional()
checks both the regular and the aliased input, finds neither, and
returns false. Still fine.
What if "data" is optional, too? Hmmm...
Example:
{ 'struct': 'Outer',
'data': { '*inner': 'Inner' },
{ 'struct': 'Inner',
'data': { '*true-name': 'str' } }
For input {}, we get an Outer object with
.has_inner = false,
.inner = NULL
Now add
'aliases': [ { 'name': 'alias-name',
'source': [ 'inner', 'true-name' ] } ] }
What do we get now? Is it
.has_inner = true,
.inner = { .has_true_name = false,
.true_name = NULL }
perhaps?
I'll study the rest of your reply next.
Re: [PATCH 2/5] parallels.txt: fix bitmap L1 table description
On 2/16/21 7:45 PM, Vladimir Sementsov-Ogievskiy wrote: > Actually L1 table entry offset is in 512 bytes sectors. Fix the spec. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > docs/interop/parallels.txt | 15 +-- > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/docs/interop/parallels.txt b/docs/interop/parallels.txt > index f15bf35bd1..ebbdd1b25b 100644 > --- a/docs/interop/parallels.txt > +++ b/docs/interop/parallels.txt > @@ -209,15 +209,14 @@ of its data area are: >The number of entries in the L1 table of the bitmap. > >variable: l1_table (8 * l1_size bytes) > - L1 offset table (in bytes) this change is unclear. First, we have specified here the size of this table. It is (8 * l1_size bytes). Thus it would be MUCH better to say l1_table (size: 8 * l1_size bytes) or L1 offset table (l1_table), size: 8 * l1_size bytes or something like this. > > A dirty bitmap is stored using a one-level structure for the mapping to host > -clusters - an L1 table. > +clusters - an L1 table. Each L1 table entry is a 64bit integer described > +below: > > -Given an offset in bytes into the bitmap data, the offset in bytes into the > -image file can be obtained as follows: > +Given an offset in bytes into the bitmap data, corresponding L1 entry is > > -offset = l1_table[offset / cluster_size] + (offset % cluster_size) > +l1_table[offset / cluster_size] Dirty bitmap is stored in the array of clusters inside Parallels Image file. Offsets of these clusters are saved in L1 offset table here. > > If an L1 table entry is 0, the corresponding cluster of the bitmap is assumed > to be zero. If L1 table entry is 0, all bits in the corresponding cluster of the bitmap are assumed to be 0. > @@ -225,4 +224,8 @@ to be zero. > If an L1 table entry is 1, the corresponding cluster of the bitmap is assumed > to have all bits set. If L1 table entry is 1, all bits in the corresponding cluster of the bitmap are assumed to be 1. > > -If an L1 table entry is not 0 or 1, it allocates a cluster from the data > area. > +If an L1 table entry is not 0 or 1, it contains corresponding cluster offset > +(in 512b sectors). Given an offset in bytes into the bitmap data the offset > in > +bytes into the image file can be obtained as follows: > + > +offset = l1_table[offset / cluster_size] * 512 + (offset % cluster_size) looks good
[PATCH v4] net/macos: implement vmnet-based netdev
From: Phillip Tennen
This patch implements a new netdev device, reachable via -netdev
vmnet-macos, that’s backed by macOS’s vmnet framework.
The vmnet framework provides native bridging support, and its usage in
this patch is intended as a replacement for attempts to use a tap device
via the tuntaposx kernel extension. Notably, the tap/tuntaposx approach
never would have worked in the first place, as QEMU interacts with the
tap device via poll(), and macOS does not support polling device files.
vmnet requires either a special entitlement, granted via a provisioning
profile, or root access. Otherwise attempts to create the virtual
interface will fail with a “generic error” status code. QEMU may not
currently be signed with an entitlement granted in a provisioning
profile, as this would necessitate pre-signed binary build distribution,
rather than source-code distribution. As such, using this netdev
currently requires that qemu be run with root access. I’ve opened a
feedback report with Apple to allow the use of the relevant entitlement
with this use case:
https://openradar.appspot.com/radar?id=5007417364447232
vmnet offers three operating modes, all of which are supported by this
patch via the “mode=host|shared|bridge” option:
* "Host" mode: Allows the vmnet interface to communicate with other
* vmnet
interfaces that are in host mode and also with the native host.
* "Shared" mode: Allows traffic originating from the vmnet interface to
reach the Internet through a NAT. The vmnet interface can also
communicate with the native host.
* "Bridged" mode: Bridges the vmnet interface with a physical network
interface.
Each of these modes also provide some extra configuration that’s
supported by this patch:
* "Bridged" mode: The user may specify the physical interface to bridge
with. Defaults to en0.
* "Host" mode / "Shared" mode: The user may specify the DHCP range and
subnet. Allocated by vmnet if not provided.
vmnet also offers some extra configuration options that are not
supported by this patch:
* Enable isolation from other VMs using vmnet
* Port forwarding rules
* Enabling TCP segmentation offload
* Only applicable in "shared" mode: specifying the NAT IPv6 prefix
* Only available in "host" mode: specifying the IP address for the VM
within an isolated network
Note that this patch requires macOS 10.15 as a minimum, as this is when
bridging support was implemented in vmnet.framework.
Signed-off-by: Phillip Tennen
---
configure | 2 +-
net/clients.h | 6 +
net/meson.build | 1 +
net/net.c | 3 +
net/vmnet-macos.c | 447 ++
qapi/net.json | 120 -
qemu-options.hx | 9 +
7 files changed, 585 insertions(+), 3 deletions(-)
create mode 100644 net/vmnet-macos.c
diff --git a/configure b/configure
index 4afd22bdf5..f449198db1 100755
--- a/configure
+++ b/configure
@@ -778,7 +778,7 @@ Darwin)
fi
audio_drv_list="coreaudio try-sdl"
audio_possible_drivers="coreaudio sdl"
- QEMU_LDFLAGS="-framework CoreFoundation -framework IOKit $QEMU_LDFLAGS"
+ QEMU_LDFLAGS="-framework CoreFoundation -framework IOKit -framework vmnet
$QEMU_LDFLAGS"
# Disable attempts to use ObjectiveC features in os/object.h since they
# won't work when we're compiling with gcc as a C compiler.
QEMU_CFLAGS="-DOS_OBJECT_USE_OBJC=0 $QEMU_CFLAGS"
diff --git a/net/clients.h b/net/clients.h
index 92f9b59aed..463a9b2f67 100644
--- a/net/clients.h
+++ b/net/clients.h
@@ -63,4 +63,10 @@ int net_init_vhost_user(const Netdev *netdev, const char
*name,
int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp);
+
+#ifdef CONFIG_DARWIN
+int net_init_vmnet_macos(const Netdev *netdev, const char *name,
+NetClientState *peer, Error **errp);
+#endif
+
#endif /* QEMU_NET_CLIENTS_H */
diff --git a/net/meson.build b/net/meson.build
index 1076b0a7ab..8c7c32f775 100644
--- a/net/meson.build
+++ b/net/meson.build
@@ -37,5 +37,6 @@ endif
softmmu_ss.add(when: 'CONFIG_POSIX', if_true: files(tap_posix))
softmmu_ss.add(when: 'CONFIG_WIN32', if_true: files('tap-win32.c'))
softmmu_ss.add(when: 'CONFIG_VHOST_NET_VDPA', if_true: files('vhost-vdpa.c'))
+softmmu_ss.add(when: 'CONFIG_DARWIN', if_true: files('vmnet-macos.c'))
subdir('can')
diff --git a/net/net.c b/net/net.c
index c1cd9c75f6..e68a410a89 100644
--- a/net/net.c
+++ b/net/net.c
@@ -977,6 +977,9 @@ static int (* const
net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
#ifdef CONFIG_L2TPV3
[NET_CLIENT_DRIVER_L2TPV3]= net_init_l2tpv3,
#endif
+#ifdef CONFIG_DARWIN
+[NET_CLIENT_DRIVER_VMNET_MACOS] = net_init_vmnet_macos,
+#endif
};
diff --git a/net/vmnet-macos.c b/net/vmnet-macos.c
new file mode 100644
index 00..1a762751dd
--- /dev/null
+++ b/net/vmnet-macos.c
@@ -0,0 +1,447 @@
+/*
+ * vmnet.framework backed netdev for macOS 10.15+ hosts
+ *
+ * Copyright (c) 2021 Phillip Tenn
Re: [PATCH] virtio-ccw: commands on revision-less devices
On Wed, 17 Feb 2021 15:39:36 +0100 Halil Pasic wrote: > On Tue, 16 Feb 2021 16:54:05 +0100 > Cornelia Huck wrote: > > > On Tue, 16 Feb 2021 15:19:45 +0100 > > Halil Pasic wrote: > > > > > On Tue, 16 Feb 2021 12:18:30 +0100 > > > Cornelia Huck wrote: > > > > > > > The virtio standard specifies that any non-transitional device must > > > > reject commands prior to revision setting (which we do) and else > > > > assume revision 0 (legacy) if the driver sends a non-revision-setting > > > > command first. We neglected to do the latter. > > > > > > Huh, I my opinion, it ain't very clear what is specified by the virtio > > > standard (which starts with version 1.0) for the described situation. > > > > > > The corresponding device normative section (4.3.2.1.1 Device > > > Requirements: Setting the Virtio Revision) says that: "A device MUST > > > treat the revision as unset from the time the associated subchannel has > > > been enabled until a revision has been successfully set by the driver. > > > This implies that revisions are not persistent across disabling and > > > enabling of the associated subchannel.". It doesn't say anything more > > > about the situation where the first command is not SET_VIRTIO_REV. > > > > > > The section "4.3.2.1.3 Legacy Interfaces: A Note on Setting the Virtio > > > Revision" which is to my best knowledge not normative, as none of the > > > legacy-interface stuff is normative, but a mere advice on how to deal > > > with legacy then says: "A legacy driver will not issue the > > > CCW_CMD_SET_VIRTIO_REV prior to issuing other virtio-ccw specific > > > channel commands." ... "A transitional device MUST assume > > > in this case that the driver is a legacy driver and continue as if the > > > driver selected revision 0. This implies that the device MUST reject any > > > command not valid for revision 0, including a subsequent > > > CCW_CMD_SET_VIRTIO_REV." > > > > > > Do we agree that the legacy interface sections in general, and 4.3.2.1.3 > > > in particular is non-normative? > > > > IMHO, normative and non-normative are not something that applies to the > > legacy sections. The legacy sections are supposed to give guidance on > > how to write transitional devices/drivers that can deal with legacy > > implementations. If you want to write something that strictly only > > adheres to normative statements, you have to write a non-transitional > > device/driver. Legacy support was never an official standard, so > > 'normative' is meaningless in that context. > > Exactly, so the legacy stuff is not normative, and strictly speaking not > included in the standard (i.e. standardized). > > But then I find usage of keywords like MUST in legacy interface sections > misreading. I believe some Oasis guy complained about keyword usage > outside of normative sections before. We can certainly discuss whether we want to change something in the legacy sections in the spec -- but that's outside the scope of this patch. > > > > > > > > > In my opinion the normative 'must threat as unset until set by driver' > > > and 'if first cmd is not _REV, must continue as if the driver selected > > > revision 0' is in a slight collision. > > > > I don't think there's a collision. If we want to accommodate legacy > > drivers, we have to deal with their lack of revision handling, and > > therefore treat non-_REV commands as implicitly selecting revision 0. > > > > If we want to implement a non-transitional device, the implicit > > selection of revision 0 goes away, of course. In fact, I'm currently > > trying to make legacy support optional for virtio-ccw, so that's why I > > had been looking at the revision handling :) > > Do you mean optional like build time configurable in QEMU? I think the > legacy support is already optional when it comes to the spec. > > Let me explain how do I interpret device compliance with respect to > virtio revisions and first command is a non-_REV. > > 1) If the first virtio command after the virtio-ccw device is enabled is > a non-_REV command, the virtio-ccw device not answering it with a > command reject does not preclude the device form being virtio 1.0 > conform. I.e. this behavior is conform, because does not violate > any of the sections linked in "7.3.3 Clause 17: Channel I/O Device > Conformance" in general, and thus does not violate "4.3.2.1.1 Device > Requirements: Setting the Virtio Revision" in particular. If you disagree, > please point me to the corresponding device normative section. > > 2) Rejecting the first command which which happens to be a non-_REV > however does not preclude virtio (1.0) conformance either. The device > is free to do whatever, because in my reading it ain't specified what > needs to be done. If it does that, however, it would be a pretty useless transitional device, as a legacy driver won't be able to work. > > 3) It is OK-ish, that the device is free to do anything there, because > a virtio 1.0 c
[PATCH 0/3] Some SMMUv3 bug fixes
Hi, This series fixes three issues: - top SID computation overflow when handling SMMU_CMD_CFGI_ALL - internal IOTLB handling (changes related to range invalidation) - smmu_iotlb_inv_iova with asid = -1 - non power of 2 invalidation range handling. Best Regards Eric This series can be found at: https://github.com/eauger/qemu/tree/viommu_fixes_for_6 Eric Auger (3): hw/arm/smmu-common: Fix smmu_iotlb_inv_iova when asid is not set hw/arm/smmuv3: Enforce invalidation on a power of two range hw/arm/smmuv3: Fix SMMU_CMD_CFGI_STE_RANGE handling hw/arm/smmu-common.c | 32 ++-- hw/arm/smmu-internal.h | 5 hw/arm/smmuv3.c| 56 -- 3 files changed, 62 insertions(+), 31 deletions(-) -- 2.26.2
[PATCH 3/3] hw/arm/smmuv3: Fix SMMU_CMD_CFGI_STE_RANGE handling
If the whole SID range (32b) is invalidated (SMMU_CMD_CFGI_ALL),
@end overflows and we fail to handle the command properly.
Once this gets fixed, the current code really is awkward in the
sense it loops over the whole range instead of removing the
currently cached configs through a hash table lookup.
Fix both the overflow and the lookup.
Signed-off-by: Eric Auger
---
hw/arm/smmu-internal.h | 5 +
hw/arm/smmuv3.c| 34 --
2 files changed, 25 insertions(+), 14 deletions(-)
diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
index 55147f29be..2d75b31953 100644
--- a/hw/arm/smmu-internal.h
+++ b/hw/arm/smmu-internal.h
@@ -104,4 +104,9 @@ typedef struct SMMUIOTLBPageInvInfo {
uint64_t mask;
} SMMUIOTLBPageInvInfo;
+typedef struct SMMUSIDRange {
+uint32_t start;
+uint32_t end;
+} SMMUSIDRange;
+
#endif
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 0bb8065b16..032cc3b300 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -32,6 +32,7 @@
#include "hw/arm/smmuv3.h"
#include "smmuv3-internal.h"
+#include "smmu-internal.h"
/**
* smmuv3_trigger_irq - pulse @irq if enabled and update
@@ -893,6 +894,20 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
}
}
+static gboolean
+smmuv3_invalidate_ste(gpointer key, gpointer value, gpointer user_data)
+{
+SMMUDevice *sdev = (SMMUDevice *)key;
+uint32_t sid = smmu_get_sid(sdev);
+SMMUSIDRange *sid_range = (SMMUSIDRange *)user_data;
+
+if (sid < sid_range->start || sid > sid_range->end) {
+return false;
+}
+trace_smmuv3_config_cache_inv(sid);
+return true;
+}
+
static int smmuv3_cmdq_consume(SMMUv3State *s)
{
SMMUState *bs = ARM_SMMU(s);
@@ -963,27 +978,18 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
}
case SMMU_CMD_CFGI_STE_RANGE: /* same as SMMU_CMD_CFGI_ALL */
{
-uint32_t start = CMD_SID(&cmd), end, i;
+uint32_t start = CMD_SID(&cmd);
uint8_t range = CMD_STE_RANGE(&cmd);
+uint64_t end = start + (1ULL << (range + 1)) - 1;
+SMMUSIDRange sid_range = {start, end};
if (CMD_SSEC(&cmd)) {
cmd_error = SMMU_CERROR_ILL;
break;
}
-
-end = start + (1 << (range + 1)) - 1;
trace_smmuv3_cmdq_cfgi_ste_range(start, end);
-
-for (i = start; i <= end; i++) {
-IOMMUMemoryRegion *mr = smmu_iommu_mr(bs, i);
-SMMUDevice *sdev;
-
-if (!mr) {
-continue;
-}
-sdev = container_of(mr, SMMUDevice, iommu);
-smmuv3_flush_config(sdev);
-}
+g_hash_table_foreach_remove(bs->configs, smmuv3_invalidate_ste,
+&sid_range);
break;
}
case SMMU_CMD_CFGI_CD:
--
2.26.2
[PATCH 2/3] hw/arm/smmuv3: Enforce invalidation on a power of two range
As of today, the driver can invalide a number of pages that is
not a power of 2. However IOTLB unmap notifications and internal
IOTLB invalidations work with masks leading to erroneous
invalidations.
In case the range is not a power of 2, split invalidations into
power of 2 invalidations.
When looking for a single page entry in the vSMMU internal IOTLB,
let's make sure that if the entry is not found using a
g_hash_table_remove() we iterate over all the entries to find a
potential range that overlaps it.
Signed-off-by: Eric Auger
---
hw/arm/smmu-common.c | 30 ++
hw/arm/smmuv3.c | 22 ++
2 files changed, 36 insertions(+), 16 deletions(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index e9ca3aebb2..84d2c62c26 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -151,22 +151,28 @@ inline void
smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova,
uint8_t tg, uint64_t num_pages, uint8_t ttl)
{
+/* if tg is not set we use 4KB range invalidation */
+uint8_t granule = tg ? tg * 2 + 10 : 12;
+
if (ttl && (num_pages == 1) && (asid >= 0)) {
SMMUIOTLBKey key = smmu_get_iotlb_key(asid, iova, tg, ttl);
-g_hash_table_remove(s->iotlb, &key);
-} else {
-/* if tg is not set we use 4KB range invalidation */
-uint8_t granule = tg ? tg * 2 + 10 : 12;
-
-SMMUIOTLBPageInvInfo info = {
-.asid = asid, .iova = iova,
-.mask = (num_pages * 1 << granule) - 1};
-
-g_hash_table_foreach_remove(s->iotlb,
-smmu_hash_remove_by_asid_iova,
-&info);
+if (g_hash_table_remove(s->iotlb, &key)) {
+return;
+}
+/*
+ * if the entry is not found, let's see if it does not
+ * belong to a larger IOTLB entry
+ */
}
+
+SMMUIOTLBPageInvInfo info = {
+.asid = asid, .iova = iova,
+.mask = (num_pages * 1 << granule) - 1};
+
+g_hash_table_foreach_remove(s->iotlb,
+smmu_hash_remove_by_asid_iova,
+&info);
}
inline void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index bd1f97000d..0bb8065b16 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -861,7 +861,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
uint16_t vmid = CMD_VMID(cmd);
bool leaf = CMD_LEAF(cmd);
uint8_t tg = CMD_TG(cmd);
-hwaddr num_pages = 1;
+uint64_t num_pages = 1;
int asid = -1;
if (tg) {
@@ -874,9 +874,23 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
if (type == SMMU_CMD_TLBI_NH_VA) {
asid = CMD_ASID(cmd);
}
-trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf);
-smmuv3_inv_notifiers_iova(s, asid, addr, tg, num_pages);
-smmu_iotlb_inv_iova(s, asid, addr, tg, num_pages, ttl);
+
+/* Split invalidations into ^2 range invalidations */
+while (num_pages) {
+uint8_t granule = tg * 2 + 10;
+uint8_t highest_bit;
+uint64_t pow2pages;
+
+highest_bit = 64 - clz64(num_pages) - 1;
+pow2pages = BIT_ULL(highest_bit);
+
+trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, pow2pages, ttl,
leaf);
+smmuv3_inv_notifiers_iova(s, asid, addr, tg, pow2pages);
+smmu_iotlb_inv_iova(s, asid, addr, tg, pow2pages, ttl);
+
+num_pages &= ~pow2pages;
+addr += pow2pages * BIT_ULL(granule);
+}
}
static int smmuv3_cmdq_consume(SMMUv3State *s)
--
2.26.2
[PATCH 1/3] hw/arm/smmu-common: Fix smmu_iotlb_inv_iova when asid is not set
If the asid is not set, do not attempt to locate the key directly
as all inserted keys have a valid asid.
Use g_hash_table_foreach_remove instead.
Signed-off-by: Eric Auger
---
hw/arm/smmu-common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 405d5c5325..e9ca3aebb2 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -151,7 +151,7 @@ inline void
smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova,
uint8_t tg, uint64_t num_pages, uint8_t ttl)
{
-if (ttl && (num_pages == 1)) {
+if (ttl && (num_pages == 1) && (asid >= 0)) {
SMMUIOTLBKey key = smmu_get_iotlb_key(asid, iova, tg, ttl);
g_hash_table_remove(s->iotlb, &key);
--
2.26.2
Re: [PULL 00/19] riscv-to-apply queue
On Thu, Feb 18, 2021 at 9:26 PM Peter Maydell wrote:
>
> On Thu, 18 Feb 2021 at 01:59, Alistair Francis
> wrote:
> >
> > The following changes since commit 1af5629673bb5c1592d993f9fb6119a62845f576:
> >
> > Merge remote-tracking branch
> > 'remotes/dgilbert-gitlab/tags/pull-virtiofs-20210216' into staging
> > (2021-02-17 14:44:18 +)
> >
> > are available in the Git repository at:
> >
> > g...@github.com:alistair23/qemu.git tags/pull-riscv-to-apply-20210217-1
> >
> > for you to fetch changes up to d0867d2dad4125d2295b28d6f91fa49cf034ffd2:
> >
> > hw/riscv: virt: Map high mmio for PCIe (2021-02-17 17:47:19 -0800)
> >
> >
> > RISC-V PR for 6.0
> >
> > This PR is a collection of RISC-V patches:
> > - Improvements to SiFive U OTP
> > - Upgrade OpenSBI to v0.9
> > - Support the QMP dump-guest-memory
> > - Add support for the SiFive SPI controller (sifive_u)
> > - Initial RISC-V system documentation
> > - A fix for the Goldfish RTC
> > - MAINTAINERS updates
> > - Support for high PCIe memory in the virt machine
>
> Fails to compile, 32 bit hosts:
>
> ../../hw/riscv/virt.c: In function 'virt_machine_init':
> ../../hw/riscv/virt.c:621:43: error: comparison is always false due to
> limited range of data type [-Werror=type-limits]
> if ((uint64_t)(machine->ram_size) > 10 * GiB) {
>^
> ../../hw/riscv/virt.c:623:33: error: large integer implicitly
> truncated to unsigned type [-Werror=overflow]
> machine->ram_size = 10 * GiB;
> ^~
This kind of error is tricky. I wonder whether we should deprecate
32-bit host support though.
Regards,
Bin
[PATCH] net: eepro100: validate various address values
From: Prasad J Pandit
While processing controller commands, eepro100 emulator gets
command unit(CU) base address OR receive unit (RU) base address
OR command block (CB) address from guest. If these values are not
checked, it may lead to an infinite loop kind of issues. Add checks
to avoid it.
Reported-by: Ruhr-University Bochum
Signed-off-by: Prasad J Pandit
---
hw/net/eepro100.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 16e95ef9cc..afa1c9b2aa 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -843,7 +843,8 @@ static void action_command(EEPRO100State *s)
bool bit_i;
bool bit_nc;
uint16_t ok_status = STATUS_OK;
-s->cb_address = s->cu_base + s->cu_offset;
+s->cb_address = s->cu_base + s->cu_offset; /* uint32_t overflow */
+assert (s->cb_address >= s->cu_base);
read_cb(s);
bit_el = ((s->tx.command & COMMAND_EL) != 0);
bit_s = ((s->tx.command & COMMAND_S) != 0);
@@ -860,6 +861,7 @@ static void action_command(EEPRO100State *s)
}
s->cu_offset = s->tx.link;
+assert(s->cu_offset > 0);
TRACE(OTHER,
logout("val=(cu start), status=0x%04x, command=0x%04x,
link=0x%08x\n",
s->tx.status, s->tx.command, s->tx.link));
@@ -990,8 +992,10 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t
val)
break;
case CU_CMD_BASE:
/* Load CU base. */
+assert(get_cu_state(s) == cu_idle);
TRACE(OTHER, logout("val=0x%02x (CU base address)\n", val));
s->cu_base = e100_read_reg4(s, SCBPointer);
+assert(!s->cu_base);
break;
case CU_DUMPSTATS:
/* Dump and reset statistical counters. */
@@ -1048,8 +1052,10 @@ static void eepro100_ru_command(EEPRO100State * s,
uint8_t val)
break;
case RX_ADDR_LOAD:
/* Load RU base. */
+assert(get_ru_state(s) == ru_idle);
TRACE(OTHER, logout("val=0x%02x (RU base address)\n", val));
s->ru_base = e100_read_reg4(s, SCBPointer);
+assert(!s->ru_base);
break;
default:
logout("val=0x%02x (undefined RU command)\n", val);
--
2.29.2
Re: [PATCH] Correct CRIS TCG register lifetime management
Hi Edgar,
Yes, it seems like our mail-server messed it up.
I’ve tried to resend it, but I’m not sure if mails directly from sendmail are
let through to outside addresses. Please let me know if you’ve got the last
mail (sent a few minutes ago).
Best regards,
-stefan
> On 18 Feb 2021, at 11:24, Edgar E. Iglesias wrote:
>
> On Thu, Feb 18, 2021 at 08:56:43AM +, Stefan Sandström wrote:
>> From: Stefan Sandstrom
>>
>> Add and fix deallocation of temporary TCG registers in CRIS code
>> generation.
>
> Thanks Stefan,
>
> Unfortunately, this patch does not apply. I'm not sure why.
> Perhaps it got corrupted by the email systems along the way.
>
> git am -s ~/Mail/stefan.sandstrom
> Applying: Correct CRIS TCG register lifetime management
> error: corrupt patch at line 11
> Patch failed at 0001 Correct CRIS TCG register lifetime management
> hint: Use 'git am --show-current-patch' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
>
> X-MS-Has-Attach:
> X-MS-TNEF-Correlator:
> x-mailer: Apple Mail (2.3654.40.0.2.32)
> x-ms-exchange-messagesentrepresentingtype: 1
> x-ms-exchange-transport-fromentityheader: Hosted
>
> How did you send out the patch? Can you try git-send-email?
>
> Best regards,
> Edgar
>
>
>
>>
>> Change-Id: I17fce5d95bdc4418337ba885d53ba97afb1bafcc
>> Signed-off-by: Stefan Sandstrom
>> ---
>> target/cris/translate.c | 146
>> ++--
>> target/cris/translate_v10.c.inc | 70 ---
>> 2 files changed, 156 insertions(+), 60 deletions(-)
>>
>> diff --git a/target/cris/translate.c b/target/cris/translate.c
>> index c893f87..ae903a5 100644
>> --- a/target/cris/translate.c
>> +++ b/target/cris/translate.c
>> @@ -177,9 +177,13 @@ static inline void t_gen_mov_TN_preg(TCGv tn, int r)
>> {
>>assert(r >= 0 && r <= 15);
>>if (r == PR_BZ || r == PR_WZ || r == PR_DZ) {
>> -tcg_gen_mov_tl(tn, tcg_const_tl(0));
>> +TCGv c0 = tcg_const_tl(0);
>> +tcg_gen_mov_tl(tn, c0);
>> +tcg_temp_free(c0);
>>} else if (r == PR_VR) {
>> -tcg_gen_mov_tl(tn, tcg_const_tl(32));
>> +TCGv c32 = tcg_const_tl(32);
>> +tcg_gen_mov_tl(tn, c32);
>> +tcg_temp_free(c32);
>>} else {
>>tcg_gen_mov_tl(tn, cpu_PR[r]);
>>}
>> @@ -255,8 +259,10 @@ static int cris_fetch(CPUCRISState *env, DisasContext
>> *dc, uint32_t addr,
>>
>> static void cris_lock_irq(DisasContext *dc)
>> {
>> +TCGv c1 = tcg_const_tl(1);
>>dc->clear_locked_irq = 0;
>> -t_gen_mov_env_TN(locked_irq, tcg_const_tl(1));
>> +t_gen_mov_env_TN(locked_irq, c1);
>> +tcg_temp_free(c1);
>> }
>>
>> static inline void t_gen_raise_exception(uint32_t index)
>> @@ -885,8 +891,10 @@ static void gen_tst_cc (DisasContext *dc, TCGv cc, int
>> cond)
>>case CC_EQ:
>>if ((arith_opt || move_opt)
>>&& dc->cc_x_uptodate != (2 | X_FLAG)) {
>> +TCGv c0 = tcg_const_tl(0);
>>tcg_gen_setcond_tl(TCG_COND_EQ, cc,
>> -cc_result, tcg_const_tl(0));
>> +cc_result, c0);
>> +tcg_temp_free(c0);
>>} else {
>>cris_evaluate_flags(dc);
>>tcg_gen_andi_tl(cc,
>> @@ -1330,14 +1338,17 @@ static int dec_addoq(CPUCRISState *env, DisasContext
>> *dc)
>> }
>> static int dec_addq(CPUCRISState *env, DisasContext *dc)
>> {
>> +TCGv c;
>>LOG_DIS("addq %u, $r%u\n", dc->op1, dc->op2);
>>
>>dc->op1 = EXTRACT_FIELD(dc->ir, 0, 5);
>>
>>cris_cc_mask(dc, CC_MASK_NZVC);
>>
>> +c = tcg_const_tl(dc->op1);
>>cris_alu(dc, CC_OP_ADD,
>> -cpu_R[dc->op2], cpu_R[dc->op2], tcg_const_tl(dc->op1), 4);
>> +cpu_R[dc->op2], cpu_R[dc->op2], c, 4);
>> +tcg_temp_free(c);
>>return 2;
>> }
>> static int dec_moveq(CPUCRISState *env, DisasContext *dc)
>> @@ -1353,62 +1364,77 @@ static int dec_moveq(CPUCRISState *env, DisasContext
>> *dc)
>> }
>> static int dec_subq(CPUCRISState *env, DisasContext *dc)
>> {
>> +TCGv c;
>>dc->op1 = EXTRACT_FIELD(dc->ir, 0, 5);
>>
>>LOG_DIS("subq %u, $r%u\n", dc->op1, dc->op2);
>>
>>cris_cc_mask(dc, CC_MASK_NZVC);
>> +c = tcg_const_tl(dc->op1);
>>cris_alu(dc, CC_OP_SUB,
>> -cpu_R[dc->op2], cpu_R[dc->op2], tcg_const_tl(dc->op1), 4);
>> +cpu_R[dc->op2], cpu_R[dc->op2], c, 4);
>> +tcg_temp_free(c);
>>return 2;
>> }
>> static int dec_cmpq(CPUCRISState *env, DisasContext *dc)
>> {
>>uint32_t imm;
>> +TCGv c;
>>dc->op1 = EXTRACT_FIELD(dc->ir, 0, 5);
>>imm = sign_extend(dc->op1, 5);
>>
>>LOG_DIS("cmpq %d, $r%d\n", imm, dc->op2);
>>cris_cc_mask(dc, CC_MASK_NZVC);
>>
>> +c = tcg_const_tl(imm);
>>cris_alu(dc, CC_OP_CMP,
>> -cpu_R[dc->op2], cpu_R[dc->op2], tcg_const_tl(imm), 4);
>>
[PATCH] Correct CRIS TCG register lifetime management
From: Stefan Sandstrom
Add and fix deallocation of temporary TCG registers in CRIS code
generation.
Change-Id: I17fce5d95bdc4418337ba885d53ba97afb1bafcc
Signed-off-by: Stefan Sandstrom
---
target/cris/translate.c | 146 ++--
target/cris/translate_v10.c.inc | 70 ---
2 files changed, 156 insertions(+), 60 deletions(-)
diff --git a/target/cris/translate.c b/target/cris/translate.c
index c893f87..ae903a5 100644
--- a/target/cris/translate.c
+++ b/target/cris/translate.c
@@ -177,9 +177,13 @@ static inline void t_gen_mov_TN_preg(TCGv tn, int r)
{
assert(r >= 0 && r <= 15);
if (r == PR_BZ || r == PR_WZ || r == PR_DZ) {
-tcg_gen_mov_tl(tn, tcg_const_tl(0));
+TCGv c0 = tcg_const_tl(0);
+tcg_gen_mov_tl(tn, c0);
+tcg_temp_free(c0);
} else if (r == PR_VR) {
-tcg_gen_mov_tl(tn, tcg_const_tl(32));
+TCGv c32 = tcg_const_tl(32);
+tcg_gen_mov_tl(tn, c32);
+tcg_temp_free(c32);
} else {
tcg_gen_mov_tl(tn, cpu_PR[r]);
}
@@ -255,8 +259,10 @@ static int cris_fetch(CPUCRISState *env, DisasContext *dc,
uint32_t addr,
static void cris_lock_irq(DisasContext *dc)
{
+TCGv c1 = tcg_const_tl(1);
dc->clear_locked_irq = 0;
-t_gen_mov_env_TN(locked_irq, tcg_const_tl(1));
+t_gen_mov_env_TN(locked_irq, c1);
+tcg_temp_free(c1);
}
static inline void t_gen_raise_exception(uint32_t index)
@@ -885,8 +891,10 @@ static void gen_tst_cc (DisasContext *dc, TCGv cc, int
cond)
case CC_EQ:
if ((arith_opt || move_opt)
&& dc->cc_x_uptodate != (2 | X_FLAG)) {
+TCGv c0 = tcg_const_tl(0);
tcg_gen_setcond_tl(TCG_COND_EQ, cc,
-cc_result, tcg_const_tl(0));
+cc_result, c0);
+tcg_temp_free(c0);
} else {
cris_evaluate_flags(dc);
tcg_gen_andi_tl(cc,
@@ -1330,14 +1338,17 @@ static int dec_addoq(CPUCRISState *env, DisasContext
*dc)
}
static int dec_addq(CPUCRISState *env, DisasContext *dc)
{
+TCGv c;
LOG_DIS("addq %u, $r%u\n", dc->op1, dc->op2);
dc->op1 = EXTRACT_FIELD(dc->ir, 0, 5);
cris_cc_mask(dc, CC_MASK_NZVC);
+c = tcg_const_tl(dc->op1);
cris_alu(dc, CC_OP_ADD,
-cpu_R[dc->op2], cpu_R[dc->op2], tcg_const_tl(dc->op1), 4);
+cpu_R[dc->op2], cpu_R[dc->op2], c, 4);
+tcg_temp_free(c);
return 2;
}
static int dec_moveq(CPUCRISState *env, DisasContext *dc)
@@ -1353,62 +1364,77 @@ static int dec_moveq(CPUCRISState *env, DisasContext
*dc)
}
static int dec_subq(CPUCRISState *env, DisasContext *dc)
{
+TCGv c;
dc->op1 = EXTRACT_FIELD(dc->ir, 0, 5);
LOG_DIS("subq %u, $r%u\n", dc->op1, dc->op2);
cris_cc_mask(dc, CC_MASK_NZVC);
+c = tcg_const_tl(dc->op1);
cris_alu(dc, CC_OP_SUB,
-cpu_R[dc->op2], cpu_R[dc->op2], tcg_const_tl(dc->op1), 4);
+cpu_R[dc->op2], cpu_R[dc->op2], c, 4);
+tcg_temp_free(c);
return 2;
}
static int dec_cmpq(CPUCRISState *env, DisasContext *dc)
{
uint32_t imm;
+TCGv c;
dc->op1 = EXTRACT_FIELD(dc->ir, 0, 5);
imm = sign_extend(dc->op1, 5);
LOG_DIS("cmpq %d, $r%d\n", imm, dc->op2);
cris_cc_mask(dc, CC_MASK_NZVC);
+c = tcg_const_tl(imm);
cris_alu(dc, CC_OP_CMP,
-cpu_R[dc->op2], cpu_R[dc->op2], tcg_const_tl(imm), 4);
+cpu_R[dc->op2], cpu_R[dc->op2], c, 4);
+tcg_temp_free(c);
return 2;
}
static int dec_andq(CPUCRISState *env, DisasContext *dc)
{
uint32_t imm;
+TCGv c;
dc->op1 = EXTRACT_FIELD(dc->ir, 0, 5);
imm = sign_extend(dc->op1, 5);
LOG_DIS("andq %d, $r%d\n", imm, dc->op2);
cris_cc_mask(dc, CC_MASK_NZ);
+c = tcg_const_tl(imm);
cris_alu(dc, CC_OP_AND,
-cpu_R[dc->op2], cpu_R[dc->op2], tcg_const_tl(imm), 4);
+cpu_R[dc->op2], cpu_R[dc->op2], c, 4);
+tcg_temp_free(c);
return 2;
}
static int dec_orq(CPUCRISState *env, DisasContext *dc)
{
uint32_t imm;
+TCGv c;
dc->op1 = EXTRACT_FIELD(dc->ir, 0, 5);
imm = sign_extend(dc->op1, 5);
LOG_DIS("orq %d, $r%d\n", imm, dc->op2);
cris_cc_mask(dc, CC_MASK_NZ);
+c = tcg_const_tl(imm);
cris_alu(dc, CC_OP_OR,
-cpu_R[dc->op2], cpu_R[dc->op2], tcg_const_tl(imm), 4);
+cpu_R[dc->op2], cpu_R[dc->op2], c, 4);
+tcg_temp_free(c);
return 2;
}
static int dec_btstq(CPUCRISState *env, DisasContext *dc)
{
+TCGv c;
dc->op1 = EXTRACT_FIELD(dc->ir, 0, 4);
LOG_DIS("btstq %u, $r%d\n", dc->op1, dc->op2);
cris_cc_mask(dc, CC_MASK_NZ);
+c = tcg_const_tl(dc->op1);
cris_evaluate_flags(dc);
-gen_helper_btst(cpu_PR[PR_CCS], cpu_env, cpu_R[dc->op2],
-tcg_const_tl(dc->op1), cpu_PR[PR_CCS]);
+gen_helper_btst(cpu_PR[PR_CCS], cpu_env, cpu_R[dc->op2],
+c, cpu_PR[PR_CCS]);
+tcg_temp_free(c);
cris_alu(dc, CC_OP_MOVE,
cpu_R[dc->op2], cpu_R[dc->op2], cpu_R[dc->op2], 4);
[PATCH] virtio-iommu: Handle non power of 2 range invalidations
Unmap notifiers work with an address mask assuming an
invalidation range of a power of 2. Nothing mandates this
in the VIRTIO-IOMMU spec.
So in case the range is not a power of 2, split it into
several invalidations.
Signed-off-by: Eric Auger
---
hw/virtio/virtio-iommu.c | 23 ---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index c2883a2f6c..a4104c99ad 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -155,6 +155,8 @@ static void virtio_iommu_notify_unmap(IOMMUMemoryRegion
*mr, hwaddr virt_start,
hwaddr virt_end)
{
IOMMUTLBEvent event;
+uint64_t mask = virt_end - virt_start;
+uint64_t size;
if (!(mr->iommu_notify_flags & IOMMU_NOTIFIER_UNMAP)) {
return;
@@ -164,12 +166,27 @@ static void virtio_iommu_notify_unmap(IOMMUMemoryRegion
*mr, hwaddr virt_start,
event.type = IOMMU_NOTIFIER_UNMAP;
event.entry.target_as = &address_space_memory;
-event.entry.addr_mask = virt_end - virt_start;
-event.entry.iova = virt_start;
event.entry.perm = IOMMU_NONE;
event.entry.translated_addr = 0;
+event.entry.addr_mask = mask;
+event.entry.iova = virt_start;
-memory_region_notify_iommu(mr, 0, event);
+if (mask == UINT64_MAX) {
+memory_region_notify_iommu(mr, 0, event);
+}
+
+size = mask + 1;
+
+while (size) {
+uint8_t highest_bit = 64 - clz64(size) - 1;
+uint64_t pow2size = BIT_ULL(highest_bit);
+
+event.entry.addr_mask = pow2size - 1;
+event.entry.iova = virt_start;
+memory_region_notify_iommu(mr, 0, event);
+size &= ~pow2size;
+virt_start += pow2size;
+}
}
static gboolean virtio_iommu_notify_unmap_cb(gpointer key, gpointer value,
--
2.26.2
Re: [PATCH] net: eepro100: validate various address values
Patchew URL:
https://patchew.org/QEMU/20210218140629.373646-1-ppan...@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20210218140629.373646-1-ppan...@redhat.com
Subject: [PATCH] net: eepro100: validate various address values
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/20210218140629.373646-1-ppan...@redhat.com ->
patchew/20210218140629.373646-1-ppan...@redhat.com
Switched to a new branch 'test'
12d net: eepro100: validate various address values
=== OUTPUT BEGIN ===
ERROR: space prohibited between function name and open parenthesis '('
#30: FILE: hw/net/eepro100.c:847:
+assert (s->cb_address >= s->cu_base);
total: 1 errors, 0 warnings, 36 lines checked
Commit 12d7c7da (net: eepro100: validate various address values) has style
problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20210218140629.373646-1-ppan...@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
Re: [PULL 00/19] riscv-to-apply queue
On Thu, 18 Feb 2021 at 14:07, Bin Meng wrote:
> On Thu, Feb 18, 2021 at 9:26 PM Peter Maydell
> wrote:
> > Fails to compile, 32 bit hosts:
> >
> > ../../hw/riscv/virt.c: In function 'virt_machine_init':
> > ../../hw/riscv/virt.c:621:43: error: comparison is always false due to
> > limited range of data type [-Werror=type-limits]
> > if ((uint64_t)(machine->ram_size) > 10 * GiB) {
> >^
> > ../../hw/riscv/virt.c:623:33: error: large integer implicitly
> > truncated to unsigned type [-Werror=overflow]
> > machine->ram_size = 10 * GiB;
> > ^~
>
> This kind of error is tricky. I wonder whether we should deprecate
> 32-bit host support though.
32-bit host is still not uncommon outside the x86 world...
The thing that makes this particular check awkward is that
machine->ram_size is a ram_addr_t, whose size is 64 bits if
either (a) the host is 64 bits or (b) CONFIG_XEN_BACKEND is
enabled, so it's effectively only 32-bits on 32-bit-not-x86.
It might be a good idea if we decided that we would just make
ram_addr_t 64-bits everywhere, to avoid this kind of "we
have an unusual config only on some more-obscure hosts" issue.
(We did that for hwaddr back in commit 4be403c8158e1 in 2012,
when it was still called target_phys_addr_t.) This change
would probably be a performance hit for 32-bit-non-x86 hosts;
it would be interesting to see whether it was measurably
significant.
-- PMM
Re: [PATCH] net: eepro100: validate various address values
On Thu, 18 Feb 2021 at 14:13, P J P wrote: > > From: Prasad J Pandit > > While processing controller commands, eepro100 emulator gets > command unit(CU) base address OR receive unit (RU) base address > OR command block (CB) address from guest. If these values are not > checked, it may lead to an infinite loop kind of issues. Add checks > to avoid it. > > Reported-by: Ruhr-University Bochum > Signed-off-by: Prasad J Pandit > --- > hw/net/eepro100.c | 8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c > index 16e95ef9cc..afa1c9b2aa 100644 > --- a/hw/net/eepro100.c > +++ b/hw/net/eepro100.c > @@ -843,7 +843,8 @@ static void action_command(EEPRO100State *s) > bool bit_i; > bool bit_nc; > uint16_t ok_status = STATUS_OK; > -s->cb_address = s->cu_base + s->cu_offset; > +s->cb_address = s->cu_base + s->cu_offset; /* uint32_t overflow */ > +assert (s->cb_address >= s->cu_base); We get these values from the guest; you can't just assert() on them. You need to do something else. My reading of the 8255x data sheet is that there is nothing in the hardware that forbids the guest from programming the device such that the cu_base + cu_offset wraps around: http://www.intel.com/content/dam/doc/manual/8255x-10-100-mbps-ethernet-controller-software-dev-manual.pdf -- page 30 says that this is all doing 32-bit arithmetic on addresses and doesn't say that there is any special case handling by the device of overflow of that addition. Your commit message isn't very clear about what the failure case is here, but I think the fix has to be something different from this. thanks -- PMM
Re: [RFC PATCH v2 1/3] vfio: Move the saving of the config space to the right place in VFIO migration
On 12/9/2020 1:39 PM, Shenming Lu wrote:
On ARM64 the VFIO SET_IRQS ioctl is dependent on the VM interrupt
setup, if the restoring of the VFIO PCI device config space is
before the VGIC, an error might occur in the kernel.
So we move the saving of the config space to the non-iterable
process, so that it will be called after the VGIC according to
their priorities.
As for the possible dependence of the device specific migration
data on it's config space, we can let the vendor driver to
include any config info it needs in its own data stream.
(Should we note this in the header file linux-headers/linux/vfio.h?)
Signed-off-by: Shenming Lu
---
hw/vfio/migration.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 00daa50ed8..3b9de1353a 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -575,11 +575,6 @@ static int vfio_save_complete_precopy(QEMUFile *f, void
*opaque)
return ret;
}
-ret = vfio_save_device_config_state(f, opaque);
-if (ret) {
-return ret;
-}
-
ret = vfio_update_pending(vbasedev);
if (ret) {
return ret;
@@ -620,6 +615,19 @@ static int vfio_save_complete_precopy(QEMUFile *f, void
*opaque)
return ret;
}
+static void vfio_save_state(QEMUFile *f, void *opaque)
+{
+VFIODevice *vbasedev = opaque;
+int ret;
+
+/* The device specific data is migrated in the iterable process. */
+ret = vfio_save_device_config_state(f, opaque);
+if (ret) {
+error_report("%s: Failed to save device config space",
+ vbasedev->name);
+}
+}
+
Since error is not propagated, set error in migration stream for
migration to fail, use qemu_file_set_error() on error.
Thanks,
Kirti
static int vfio_load_setup(QEMUFile *f, void *opaque)
{
VFIODevice *vbasedev = opaque;
@@ -670,11 +678,7 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int
version_id)
switch (data) {
case VFIO_MIG_FLAG_DEV_CONFIG_STATE:
{
-ret = vfio_load_device_config_state(f, opaque);
-if (ret) {
-return ret;
-}
-break;
+return vfio_load_device_config_state(f, opaque);
}
case VFIO_MIG_FLAG_DEV_SETUP_STATE:
{
@@ -720,6 +724,7 @@ static SaveVMHandlers savevm_vfio_handlers = {
.save_live_pending = vfio_save_pending,
.save_live_iterate = vfio_save_iterate,
.save_live_complete_precopy = vfio_save_complete_precopy,
+.save_state = vfio_save_state,
.load_setup = vfio_load_setup,
.load_cleanup = vfio_load_cleanup,
.load_state = vfio_load_state,
Re: [PATCH 1/1] hw/s390x: modularize virtio-gpu-ccw
On 18/02/2021 11.34, Halil Pasic wrote:
On Thu, 18 Feb 2021 10:23:16 +0100
Thomas Huth wrote:
Since the virtio-gpu-ccw device depends on the hw-display-virtio-gpu
module, which provides the type virtio-gpu-device, packaging the
hw-display-virtio-gpu module as a separate package that may or may not
be installed along with the qemu package leads to problems. Namely if
the hw-display-virtio-gpu is absent, qemu continues to advertise
virtio-gpu-ccw, but it aborts not only when one attempts using
virtio-gpu-ccw, but also when libvirtd's capability probing tries
to instantiate the type to introspect it.
Let us thus introduce a module named hw-s390x-virtio-gpu-ccw that
is going to provide the virtio-gpu-ccw device. The hw-s390x prefix
was chosen because it is not a portable device.
With virtio-gpu-ccw built as a module, the correct way to package a
modularized qemu is to require that hw-display-virtio-gpu must be
installed whenever the module hw-s390x-virtio-gpu-ccw.
Signed-off-by: Halil Pasic
---
hw/s390x/meson.build | 17 -
util/module.c| 1 +
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index 2a7818d94b..153b1309fb 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -34,7 +34,6 @@ virtio_ss.add(files('virtio-ccw.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_BALLOON', if_true:
files('virtio-ccw-balloon.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-ccw-blk.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true:
files('virtio-ccw-crypto.c'))
-virtio_ss.add(when: 'CONFIG_VIRTIO_GPU', if_true: files('virtio-ccw-gpu.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_INPUT', if_true:
files('virtio-ccw-input.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio-ccw-net.c'))
virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-ccw-rng.c'))
@@ -46,3 +45,19 @@ virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true:
files('vhost-user-fs-ccw.c'
s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
hw_arch += {'s390x': s390x_ss}
+
+if target.startswith('s390x')
+ hw_s390x_modules = {}
+
+ hw_s390x_modules_c_args = ['-DNEED_CPU_H',
+ '-DCONFIG_TARGET="@0@-config-target.h"'.format(target)]
+ hw_s390x_modules_inc = [include_directories('../../target' /
config_target['TARGET_BASE_ARCH'])]
+ hw_s390x_modules_dependencies = declare_dependency(
+ include_directories: hw_s390x_modules_inc, compile_args:
hw_s390x_modules_c_args)
Basically the patch looks fine to me, but I wonder why all that above lines
(related to hw_s390x_modules_dependencies) are requred at all? The other
display modules in hw/display/meson.build also do not need to re-define
c_args for example?
The explanation is simple. Unlike most devices, the ccw devices aren't
portable. In particular both css.c and css.h includes "cpu.h", and
virtio-ccw-gpu.c includes "qemu/osdep.h". Furthermore osdep.h contains:
#ifdef NEED_CPU_H
#include CONFIG_TARGET
#else
#include "exec/poison.h"
#endif
so if we don't have NEED_CPU_H, among others CONFIG_KVM is poisoned, and
CONFIG_KVM is used in "css.h". Frankly, I can't tell under what circumstances
does css need "cpu.h".
As far as I can see, the only real reason right now is the CONFIG_KVM
section in css.h. I think you could simply move that into another header
file instead (cpu.h ?)
Thomas
Re: [RFC PATCH v2 1/3] vfio: Move the saving of the config space to the right place in VFIO migration
On 1/27/2021 3:06 AM, Alex Williamson wrote: On Thu, 10 Dec 2020 10:21:21 +0800 Shenming Lu wrote: On 2020/12/10 2:34, Alex Williamson wrote: On Wed, 9 Dec 2020 13:29:47 +0100 Cornelia Huck wrote: On Wed, 9 Dec 2020 16:09:17 +0800 Shenming Lu wrote: On ARM64 the VFIO SET_IRQS ioctl is dependent on the VM interrupt setup, if the restoring of the VFIO PCI device config space is before the VGIC, an error might occur in the kernel. So we move the saving of the config space to the non-iterable process, so that it will be called after the VGIC according to their priorities. As for the possible dependence of the device specific migration data on it's config space, we can let the vendor driver to include any config info it needs in its own data stream. (Should we note this in the header file linux-headers/linux/vfio.h?) Given that the header is our primary source about how this interface should act, we need to properly document expectations about what will be saved/restored when there (well, in the source file in the kernel.) That goes in both directions: what a userspace must implement, and what a vendor driver can rely on. Yeah, in order to make the vendor driver and QEMU cooperate better, we might need to document some expectations about the data section in the migration region... [Related, but not a todo for you: I think we're still missing proper documentation of the whole migration feature.] Yes, we never saw anything past v1 of the documentation patch. Thanks, I'll get back on this and send next version soon. By the way, is there anything unproper with this patch? Wish your suggestion. :-) I'm really hoping for some feedback from Kirti, I understand the NVIDIA vGPU driver to have some dependency on this. Thanks, NVIDIA driver doesn't use device config space value/information during device data restoration, so we are good with this change. Thanks, Kirti Alex Signed-off-by: Shenming Lu --- hw/vfio/migration.c | 25 +++-- 1 file changed, 15 insertions(+), 10 deletions(-) .
[PATCH v2 0/5] target/sh4: Pass MMUAccessType to get_physical_address()
Taking notes while reviewing commit 671a0a1265a
("use MMUAccessType instead of int in mmu_translate").
Since v1:
- Do not provide unuseful MMU index argument (Richard)
Missing review: patch 3 (Remove unused 'int access_type' argument)
Philippe Mathieu-Daudé (5):
target/sh4: Fix code style for checkpatch.pl
target/sh4: Replace magic value by MMUAccessType definitions
target/sh4: Remove unused 'int access_type' argument
target/sh4: Let get_physical_address() use MMUAccessType access_type
target/sh4: Remove unused definitions
target/sh4/cpu.h| 11 -
target/sh4/helper.c | 101 ++--
2 files changed, 50 insertions(+), 62 deletions(-)
--
2.26.2
[PATCH v2 1/5] target/sh4: Fix code style for checkpatch.pl
We are going to move this code, fix its style first.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Richard Henderson
---
target/sh4/helper.c | 82 ++---
1 file changed, 41 insertions(+), 41 deletions(-)
diff --git a/target/sh4/helper.c b/target/sh4/helper.c
index 408478ce5dc..fc816137766 100644
--- a/target/sh4/helper.c
+++ b/target/sh4/helper.c
@@ -330,8 +330,8 @@ static int find_utlb_entry(CPUSH4State * env, target_ulong
address, int use_asid
MMU_IADDR_ERROR, MMU_DADDR_ERROR_READ, MMU_DADDR_ERROR_WRITE.
*/
static int get_mmu_address(CPUSH4State * env, target_ulong * physical,
- int *prot, target_ulong address,
- int rw, int access_type)
+ int *prot, target_ulong address,
+ int rw, int access_type)
{
int use_asid, n;
tlb_t *matching = NULL;
@@ -340,12 +340,12 @@ static int get_mmu_address(CPUSH4State * env,
target_ulong * physical,
if (rw == 2) {
n = find_itlb_entry(env, address, use_asid);
- if (n >= 0) {
- matching = &env->itlb[n];
+if (n >= 0) {
+matching = &env->itlb[n];
if (!(env->sr & (1u << SR_MD)) && !(matching->pr & 2)) {
- n = MMU_ITLB_VIOLATION;
+n = MMU_ITLB_VIOLATION;
} else {
- *prot = PAGE_EXEC;
+*prot = PAGE_EXEC;
}
} else {
n = find_utlb_entry(env, address, use_asid);
@@ -365,14 +365,14 @@ static int get_mmu_address(CPUSH4State * env,
target_ulong * physical,
} else if (n == MMU_DTLB_MISS) {
n = MMU_ITLB_MISS;
}
- }
+}
} else {
- n = find_utlb_entry(env, address, use_asid);
- if (n >= 0) {
- matching = &env->utlb[n];
+n = find_utlb_entry(env, address, use_asid);
+if (n >= 0) {
+matching = &env->utlb[n];
if (!(env->sr & (1u << SR_MD)) && !(matching->pr & 2)) {
-n = (rw == 1) ? MMU_DTLB_VIOLATION_WRITE :
-MMU_DTLB_VIOLATION_READ;
+n = (rw == 1)
+? MMU_DTLB_VIOLATION_WRITE : MMU_DTLB_VIOLATION_READ;
} else if ((rw == 1) && !(matching->pr & 1)) {
n = MMU_DTLB_VIOLATION_WRITE;
} else if ((rw == 1) && !matching->d) {
@@ -383,15 +383,15 @@ static int get_mmu_address(CPUSH4State * env,
target_ulong * physical,
*prot |= PAGE_WRITE;
}
}
- } else if (n == MMU_DTLB_MISS) {
- n = (rw == 1) ? MMU_DTLB_MISS_WRITE :
- MMU_DTLB_MISS_READ;
- }
+} else if (n == MMU_DTLB_MISS) {
+n = (rw == 1)
+? MMU_DTLB_MISS_WRITE : MMU_DTLB_MISS_READ;
+}
}
if (n >= 0) {
- n = MMU_OK;
- *physical = ((matching->ppn << 10) & ~(matching->size - 1)) |
- (address & (matching->size - 1));
+n = MMU_OK;
+*physical = ((matching->ppn << 10) & ~(matching->size - 1))
+| (address & (matching->size - 1));
}
return n;
}
@@ -401,34 +401,34 @@ static int get_physical_address(CPUSH4State * env,
target_ulong * physical,
int rw, int access_type)
{
/* P1, P2 and P4 areas do not use translation */
-if ((address >= 0x8000 && address < 0xc000) ||
- address >= 0xe000) {
+if ((address >= 0x8000 && address < 0xc000) || address >=
0xe000) {
if (!(env->sr & (1u << SR_MD))
- && (address < 0xe000 || address >= 0xe400)) {
- /* Unauthorized access in user mode (only store queues are
available) */
+&& (address < 0xe000 || address >= 0xe400)) {
+/* Unauthorized access in user mode (only store queues are
available) */
qemu_log_mask(LOG_GUEST_ERROR, "Unauthorized access\n");
- if (rw == 0)
- return MMU_DADDR_ERROR_READ;
- else if (rw == 1)
- return MMU_DADDR_ERROR_WRITE;
- else
- return MMU_IADDR_ERROR;
- }
- if (address >= 0x8000 && address < 0xc000) {
- /* Mask upper 3 bits for P1 and P2 areas */
- *physical = address & 0x1fff;
- } else {
- *physical = address;
- }
- *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
- return MMU_OK;
+if (rw == 0) {
+return MMU_DADDR_ERROR_READ;
+} else if (rw == 1) {
+return MMU_DADDR_ERROR_WRITE;
+} else {
+return MMU_IADDR_ERROR;
+}
+}
+if (address >= 0x8000 && address < 0xc000) {
+/* Mask upper 3 bits for P1 and P2 areas */
+*physical = address & 0x1fff;
+} else {
+*phys
[PATCH v2 2/5] target/sh4: Replace magic value by MMUAccessType definitions
Replace the 0/1/2 magic values by the corresponding MMUAccessType.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Richard Henderson
---
target/sh4/helper.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/target/sh4/helper.c b/target/sh4/helper.c
index fc816137766..4303ebf018b 100644
--- a/target/sh4/helper.c
+++ b/target/sh4/helper.c
@@ -338,7 +338,7 @@ static int get_mmu_address(CPUSH4State * env, target_ulong
* physical,
use_asid = !(env->mmucr & MMUCR_SV) || !(env->sr & (1u << SR_MD));
-if (rw == 2) {
+if (rw == MMU_INST_FETCH) {
n = find_itlb_entry(env, address, use_asid);
if (n >= 0) {
matching = &env->itlb[n];
@@ -371,11 +371,11 @@ static int get_mmu_address(CPUSH4State * env,
target_ulong * physical,
if (n >= 0) {
matching = &env->utlb[n];
if (!(env->sr & (1u << SR_MD)) && !(matching->pr & 2)) {
-n = (rw == 1)
+n = (rw == MMU_DATA_STORE)
? MMU_DTLB_VIOLATION_WRITE : MMU_DTLB_VIOLATION_READ;
-} else if ((rw == 1) && !(matching->pr & 1)) {
+} else if ((rw == MMU_DATA_STORE) && !(matching->pr & 1)) {
n = MMU_DTLB_VIOLATION_WRITE;
-} else if ((rw == 1) && !matching->d) {
+} else if ((rw == MMU_DATA_STORE) && !matching->d) {
n = MMU_DTLB_INITIAL_WRITE;
} else {
*prot = PAGE_READ;
@@ -384,7 +384,7 @@ static int get_mmu_address(CPUSH4State * env, target_ulong
* physical,
}
}
} else if (n == MMU_DTLB_MISS) {
-n = (rw == 1)
+n = (rw == MMU_DATA_STORE)
? MMU_DTLB_MISS_WRITE : MMU_DTLB_MISS_READ;
}
}
@@ -406,9 +406,9 @@ static int get_physical_address(CPUSH4State * env,
target_ulong * physical,
&& (address < 0xe000 || address >= 0xe400)) {
/* Unauthorized access in user mode (only store queues are
available) */
qemu_log_mask(LOG_GUEST_ERROR, "Unauthorized access\n");
-if (rw == 0) {
+if (rw == MMU_DATA_LOAD) {
return MMU_DADDR_ERROR_READ;
-} else if (rw == 1) {
+} else if (rw == MMU_DATA_STORE) {
return MMU_DADDR_ERROR_WRITE;
} else {
return MMU_IADDR_ERROR;
@@ -441,7 +441,7 @@ hwaddr superh_cpu_get_phys_page_debug(CPUState *cs, vaddr
addr)
target_ulong physical;
int prot;
-get_physical_address(&cpu->env, &physical, &prot, addr, 0, 0);
+get_physical_address(&cpu->env, &physical, &prot, addr, MMU_DATA_LOAD, 0);
return physical;
}
--
2.26.2
[PATCH v2 3/5] target/sh4: Remove unused 'int access_type' argument
get_mmu_address() and get_physical_address() don't use their
'int access_type' argument: remove it along with ACCESS_INT
in superh_cpu_tlb_fill().
Suggested-by: Richard Henderson
Signed-off-by: Philippe Mathieu-Daudé
---
target/sh4/helper.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/target/sh4/helper.c b/target/sh4/helper.c
index 4303ebf018b..b49efe84916 100644
--- a/target/sh4/helper.c
+++ b/target/sh4/helper.c
@@ -331,7 +331,7 @@ static int find_utlb_entry(CPUSH4State * env, target_ulong
address, int use_asid
*/
static int get_mmu_address(CPUSH4State * env, target_ulong * physical,
int *prot, target_ulong address,
- int rw, int access_type)
+ int rw)
{
int use_asid, n;
tlb_t *matching = NULL;
@@ -398,7 +398,7 @@ static int get_mmu_address(CPUSH4State * env, target_ulong
* physical,
static int get_physical_address(CPUSH4State * env, target_ulong * physical,
int *prot, target_ulong address,
-int rw, int access_type)
+int rw)
{
/* P1, P2 and P4 areas do not use translation */
if ((address >= 0x8000 && address < 0xc000) || address >=
0xe000) {
@@ -432,7 +432,7 @@ static int get_physical_address(CPUSH4State * env,
target_ulong * physical,
}
/* We need to resort to the MMU */
-return get_mmu_address(env, physical, prot, address, rw, access_type);
+return get_mmu_address(env, physical, prot, address, rw);
}
hwaddr superh_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
@@ -441,7 +441,8 @@ hwaddr superh_cpu_get_phys_page_debug(CPUState *cs, vaddr
addr)
target_ulong physical;
int prot;
-get_physical_address(&cpu->env, &physical, &prot, addr, MMU_DATA_LOAD, 0);
+get_physical_address(&cpu->env, &physical, &prot, addr, MMU_DATA_LOAD);
+
return physical;
}
@@ -813,11 +814,9 @@ bool superh_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
MMU_DTLB_VIOLATION_READ);
#else
target_ulong physical;
-int prot, sh_access_type;
+int prot;
-sh_access_type = ACCESS_INT;
-ret = get_physical_address(env, &physical, &prot, address,
- access_type, sh_access_type);
+ret = get_physical_address(env, &physical, &prot, address, access_type);
if (ret == MMU_OK) {
address &= TARGET_PAGE_MASK;
--
2.26.2
[PATCH v2 5/5] target/sh4: Remove unused definitions
Remove these confusing and unused definitions.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Richard Henderson
---
target/sh4/cpu.h | 11 ---
1 file changed, 11 deletions(-)
diff --git a/target/sh4/cpu.h b/target/sh4/cpu.h
index 714e3b56413..01c43440822 100644
--- a/target/sh4/cpu.h
+++ b/target/sh4/cpu.h
@@ -271,17 +271,6 @@ typedef SuperHCPU ArchCPU;
#include "exec/cpu-all.h"
-/* Memory access type */
-enum {
-/* Privilege */
-ACCESS_PRIV = 0x01,
-/* Direction */
-ACCESS_WRITE = 0x02,
-/* Type of instruction */
-ACCESS_CODE = 0x10,
-ACCESS_INT = 0x20
-};
-
/* MMU control register */
#define MMUCR0x1F10
#define MMUCR_AT (1<<0)
--
2.26.2
[PATCH v2 4/5] target/sh4: Let get_physical_address() use MMUAccessType access_type
superh_cpu_tlb_fill() already provides a access_type variable of
type MMUAccessType, and it is passed along, but casted as integer
and renamed 'rw'.
Simply replace 'int rw' by 'MMUAccessType access_type'.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Richard Henderson
---
target/sh4/helper.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/target/sh4/helper.c b/target/sh4/helper.c
index b49efe84916..bd8e034f174 100644
--- a/target/sh4/helper.c
+++ b/target/sh4/helper.c
@@ -331,14 +331,14 @@ static int find_utlb_entry(CPUSH4State * env,
target_ulong address, int use_asid
*/
static int get_mmu_address(CPUSH4State * env, target_ulong * physical,
int *prot, target_ulong address,
- int rw)
+ MMUAccessType access_type)
{
int use_asid, n;
tlb_t *matching = NULL;
use_asid = !(env->mmucr & MMUCR_SV) || !(env->sr & (1u << SR_MD));
-if (rw == MMU_INST_FETCH) {
+if (access_type == MMU_INST_FETCH) {
n = find_itlb_entry(env, address, use_asid);
if (n >= 0) {
matching = &env->itlb[n];
@@ -371,11 +371,11 @@ static int get_mmu_address(CPUSH4State * env,
target_ulong * physical,
if (n >= 0) {
matching = &env->utlb[n];
if (!(env->sr & (1u << SR_MD)) && !(matching->pr & 2)) {
-n = (rw == MMU_DATA_STORE)
+n = (access_type == MMU_DATA_STORE)
? MMU_DTLB_VIOLATION_WRITE : MMU_DTLB_VIOLATION_READ;
-} else if ((rw == MMU_DATA_STORE) && !(matching->pr & 1)) {
+} else if ((access_type == MMU_DATA_STORE) && !(matching->pr & 1))
{
n = MMU_DTLB_VIOLATION_WRITE;
-} else if ((rw == MMU_DATA_STORE) && !matching->d) {
+} else if ((access_type == MMU_DATA_STORE) && !matching->d) {
n = MMU_DTLB_INITIAL_WRITE;
} else {
*prot = PAGE_READ;
@@ -384,7 +384,7 @@ static int get_mmu_address(CPUSH4State * env, target_ulong
* physical,
}
}
} else if (n == MMU_DTLB_MISS) {
-n = (rw == MMU_DATA_STORE)
+n = (access_type == MMU_DATA_STORE)
? MMU_DTLB_MISS_WRITE : MMU_DTLB_MISS_READ;
}
}
@@ -398,7 +398,7 @@ static int get_mmu_address(CPUSH4State * env, target_ulong
* physical,
static int get_physical_address(CPUSH4State * env, target_ulong * physical,
int *prot, target_ulong address,
-int rw)
+MMUAccessType access_type)
{
/* P1, P2 and P4 areas do not use translation */
if ((address >= 0x8000 && address < 0xc000) || address >=
0xe000) {
@@ -406,9 +406,9 @@ static int get_physical_address(CPUSH4State * env,
target_ulong * physical,
&& (address < 0xe000 || address >= 0xe400)) {
/* Unauthorized access in user mode (only store queues are
available) */
qemu_log_mask(LOG_GUEST_ERROR, "Unauthorized access\n");
-if (rw == MMU_DATA_LOAD) {
+if (access_type == MMU_DATA_LOAD) {
return MMU_DADDR_ERROR_READ;
-} else if (rw == MMU_DATA_STORE) {
+} else if (access_type == MMU_DATA_STORE) {
return MMU_DADDR_ERROR_WRITE;
} else {
return MMU_IADDR_ERROR;
@@ -432,7 +432,7 @@ static int get_physical_address(CPUSH4State * env,
target_ulong * physical,
}
/* We need to resort to the MMU */
-return get_mmu_address(env, physical, prot, address, rw);
+return get_mmu_address(env, physical, prot, address, access_type);
}
hwaddr superh_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
--
2.26.2
Re: [PATCH] Correct CRIS TCG register lifetime management
Hi Stefan, On 2/18/21 1:16 PM, Stefan Sandström wrote: > Hi Edgar, > > Yes, it seems like our mail-server messed it up. > > I’ve tried to resend it, but I’m not sure if mails directly from sendmail are > let through to outside addresses. Please let me know if you’ve got the last > mail (sent a few minutes ago). You might try adding: [format] headers = "Content-Transfer-Encoding: 8bit\n" or: [format] headers = "Content-Type: text/plain; charset=\"utf-8\"\nContent-Transfer-Encoding: 8bit\n" in your ~/.gitconfig Then you can check if your patch can be applied properly checking on patchew: https://patchew.org/QEMU/ For example for this post, as Edgar mentioned: https://patchew.org/QEMU/a13a6e43-eeaf-4781-87e1-3295f698a...@axis.com/ Switched to a new branch 'a13a6e43-eeaf-4781-87e1-3295f698a...@axis.com' Applying: Correct CRIS TCG register lifetime management error: corrupt patch at line 11 error: could not build fake ancestor hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0001 Correct CRIS TCG register lifetime management When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Failed to apply patch: [PATCH] Correct CRIS TCG register lifetime management Regards, Phil.
[PATCH] hw/ppc: e500: Add missing #address-cells and #size-cells in the eTSEC node
From: Bin Meng
Per devicetree spec v0.3 [1] chapter 2.3.5:
The #address-cells and #size-cells properties are not inherited
from ancestors in the devicetree. They shall be explicitly defined.
If missing, a client program should assume a default value of 2
for #address-cells, and a value of 1 for #size-cells.
These properties are currently missing, causing the property
to be incorrectly parsed using the default values.
[1]
https://github.com/devicetree-org/devicetree-specification/releases/download/v0.3/devicetree-specification-v0.3.pdf
Fixes: fdfb7f2cdb2d ("e500: Add support for eTSEC in device tree")
Signed-off-by: Bin Meng
---
hw/ppc/e500.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 01517a6..c84a021 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -236,6 +236,8 @@ static int create_devtree_etsec(SysBusDevice *sbdev,
PlatformDevtreeData *data)
qemu_fdt_setprop_string(fdt, node, "model", "eTSEC");
qemu_fdt_setprop(fdt, node, "local-mac-address", etsec->conf.macaddr.a, 6);
qemu_fdt_setprop_cells(fdt, node, "fixed-link", 0, 1, 1000, 0, 0);
+qemu_fdt_setprop_cells(fdt, node, "#size-cells", 1);
+qemu_fdt_setprop_cells(fdt, node, "#address-cells", 1);
qemu_fdt_add_subnode(fdt, group);
qemu_fdt_setprop_cells(fdt, group, "reg", mmio0, 0x1000);
--
2.7.4
Re: [PATCH 2/7] block/qcow2: introduce cache for compressed writes
On 11.02.21 13:49, Vladimir Sementsov-Ogievskiy wrote:
you may jump first to my last inline answer
10.02.2021 20:07, Max Reitz wrote:
On 29.01.21 17:50, Vladimir Sementsov-Ogievskiy wrote:
Compressed writes and O_DIRECT are not friends: they works too slow,
because compressed writes does many small unaligned to 512 writes.
Let's introduce an internal cache, so that compressed writes may work
well when O_DIRECT is on.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/qcow2.h | 29 +
block/qcow2-compressed-write-cache.c | 770 +++
block/meson.build | 1 +
3 files changed, 800 insertions(+)
create mode 100644 block/qcow2-compressed-write-cache.c
diff --git a/block/qcow2.h b/block/qcow2.h
index 0678073b74..fbdedf89fa 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -322,6 +322,8 @@ typedef struct Qcow2BitmapHeaderExt {
uint64_t bitmap_directory_offset;
} QEMU_PACKED Qcow2BitmapHeaderExt;
+typedef struct Qcow2CompressedWriteCache Qcow2CompressedWriteCache;
+
#define QCOW2_MAX_THREADS 4
typedef struct BDRVQcow2State {
@@ -1010,4 +1012,31 @@ int coroutine_fn
qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
uint64_t guest_offset, void *buf, size_t len);
+Qcow2CompressedWriteCache *qcow2_compressed_cache_new(BdrvChild
*data_file,
+ int64_t
cluster_size,
+ int64_t
cache_size);
+void qcow2_compressed_cache_free(Qcow2CompressedWriteCache *s);
+int coroutine_fn
+qcow2_compressed_cache_co_read(Qcow2CompressedWriteCache *s, int64_t
offset,
+ int64_t bytes, void *buf);
+int coroutine_fn
+qcow2_compressed_cache_co_write(Qcow2CompressedWriteCache *s,
int64_t offset,
+ int64_t bytes, void *buf);
+void coroutine_fn
+qcow2_compressed_cache_co_set_cluster_end(Qcow2CompressedWriteCache *s,
+ int64_t cluster_data_end);
+int coroutine_fn
+qcow2_compressed_cache_co_flush(Qcow2CompressedWriteCache *s);
+int qcow2_compressed_cache_flush(BlockDriverState *bs,
+ Qcow2CompressedWriteCache *state);
+int coroutine_fn
+qcow2_compressed_cache_co_stop_flush(Qcow2CompressedWriteCache *s);
+int qcow2_compressed_cache_stop_flush(BlockDriverState *bs,
+ Qcow2CompressedWriteCache *s);
+void qcow2_compressed_cache_set_size(Qcow2CompressedWriteCache *s,
+ int64_t size);
+void coroutine_fn
+qcow2_compressed_cache_co_discard(Qcow2CompressedWriteCache *s,
+ int64_t cluster_offset);
+
It would be nice if these functions had their interface documented
somewhere.
I tried to comment dificult things in .c... Is there a prefernce, where
to document
how and what function does, in .h or in .c ?
No, but my problem was that I found even the things you probably didn’t
consider difficult not completely obvious; i.e., I would’ve liked a full
documentation on the function interface. (And I don’t think there is
such documentation in the .c file.)
#endif
diff --git a/block/qcow2-compressed-write-cache.c
b/block/qcow2-compressed-write-cache.c
new file mode 100644
index 00..7bb92cb550
--- /dev/null
+++ b/block/qcow2-compressed-write-cache.c
@@ -0,0 +1,770 @@
+/*
+ * Write cache for qcow2 compressed writes
+ *
+ * Copyright (c) 2021 Virtuozzo International GmbH.
+ *
+ * Permission is hereby granted, free of charge, to any person
obtaining a copy
+ * of this software and associated documentation files (the
"Software"), to deal
+ * in the Software without restriction, including without limitation
the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense,
and/or sell
+ * copies of the Software, and to permit persons to whom the
Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be
included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+
+#include "block/block_int.h"
+#include "block/block-gen.h"
+#include "qemu/coroutine.h"
+#include "qapi/qapi-events-block-core.h"
+#include "qcow2.h"
+
+typedef struct CacheExtent {
+ int64_t offset;
+ int64_t bytes;
+ void *buf;
+ QLIST_ENTRY(CacheExtent) next;
+} CacheEx
Re: [PATCH v2 1/3] virtiofsd: Add an option to enable/disable posix acls
On Wed, Feb 17, 2021 at 06:30:44PM -0500, Vivek Goyal wrote:
> fuse has an option FUSE_POSIX_ACL which needs to be opted in by fuse
> server to enable posix acls.
>
> Add virtiofsd option "-o posix_acl/no_posix_acl" to let users enable/disable
> posix acl support. By default it is disabled as of now.
>
> Currently even if file server has not opted in for FUSE_POSIX_ACL, user can
> still query acl and set acl, and system.posix_acl_access and
> system.posix_acl_default xattrs show up listxattr response.
>
> Miklos said this is confusing. So he said lets block and filter
> system.posix_acl_access and system.posix_acl_default xattrs in
> getxattr/setxattr/listxattr if user has explicitly disabled
> posix acls using -o no_posix_acl.
I forgot to block acl xattrs in lo_removexattr(). Will respin this patch.
Vivek
>
> As of now continuing to keeping the existing behavior if user did not
> specify any option to disable acl support due to concerns about backward
> compatibility.
>
> v2: block system.posix_acl_access and system.posix_acl_default xattrs
> if user explicitly disabled acls. (Miklos)
>
> Signed-off-by: Vivek Goyal
> ---
> tools/virtiofsd/passthrough_ll.c | 95 +++-
> 1 file changed, 94 insertions(+), 1 deletion(-)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c
> b/tools/virtiofsd/passthrough_ll.c
> index 58d24c0010..26cdfbd1f0 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -169,6 +169,7 @@ struct lo_data {
> /* An O_PATH file descriptor to /proc/self/fd/ */
> int proc_self_fd;
> int user_killpriv_v2, killpriv_v2;
> +int user_posix_acl;
> };
>
> static const struct fuse_opt lo_opts[] = {
> @@ -201,6 +202,8 @@ static const struct fuse_opt lo_opts[] = {
> { "announce_submounts", offsetof(struct lo_data, announce_submounts), 1
> },
> { "killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 1 },
> { "no_killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 0 },
> +{ "posix_acl", offsetof(struct lo_data, user_posix_acl), 1 },
> +{ "no_posix_acl", offsetof(struct lo_data, user_posix_acl), 0 },
> FUSE_OPT_END
> };
> static bool use_syslog = false;
> @@ -661,6 +664,23 @@ static void lo_init(void *userdata, struct
> fuse_conn_info *conn)
> conn->want &= ~FUSE_CAP_HANDLE_KILLPRIV_V2;
> lo->killpriv_v2 = 0;
> }
> +
> +if (lo->user_posix_acl == 1) {
> +/*
> + * User explicitly asked for this option. Enable it unconditionally.
> + * If connection does not have this capability, it should fail
> + * in fuse_lowlevel.c
> + */
> +fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n");
> +conn->want |= FUSE_CAP_POSIX_ACL;
> +} else {
> +/*
> + * Either user specified to disable posix_acl, or did not specify
> + * anything. In both the cases do not enable posix acl.
> + */
> +fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling posix_acl\n");
> +conn->want &= ~FUSE_CAP_POSIX_ACL;
> +}
> }
>
> static void lo_getattr(fuse_req_t req, fuse_ino_t ino,
> @@ -2612,6 +2632,63 @@ static int xattr_map_server(const struct lo_data *lo,
> const char *server_name,
> return -ENODATA;
> }
>
> +static bool block_xattr(struct lo_data *lo, const char *name)
> +{
> +/*
> + * If user explicitly enabled posix_acl or did not provide any option,
> + * do not block acl. Otherwise block system.posix_acl_access and
> + * system.posix_acl_default xattrs.
> + */
> +if (lo->user_posix_acl) {
> +return false;
> +}
> +if (!strcmp(name, "system.posix_acl_access") ||
> +!strcmp(name, "system.posix_acl_default"))
> +return true;
> +
> +return false;
> +}
> +
> +/*
> + * Returns number of bytes in xattr_list after filtering on success. This
> + * could be zero as well if nothing is left after filtering.
> + *
> + * Returns negative error code on failure.
> + * xattr_list is modified in place.
> + */
> +static int remove_blocked_xattrs(struct lo_data *lo, char *xattr_list,
> + unsigned in_size)
> +{
> +size_t out_index, in_index;
> +
> +/*
> + * As of now we only filter out acl xattrs. If acls are enabled or
> + * they have not been explicitly disabled, there is nothing to
> + * filter.
> + */
> +if (lo->user_posix_acl) {
> +return in_size;
> +}
> +
> +out_index = 0;
> +in_index = 0;
> +while (in_index < in_size) {
> +char *in_ptr = xattr_list + in_index;
> +
> +/* Length of current attribute name */
> +size_t in_len = strlen(xattr_list + in_index) + 1;
> +
> +if (!block_xattr(lo, in_ptr)) {
> +if (in_index != out_index) {
> +memmove(xattr_list + out_index, xattr_list + in_index,
> in_len);
> +}
> +out_index += in_len;
> +}
Re: [PULL 00/23] plugin updates (hwprofile, CF_NOCACHE, io_recompile)
On Thu, 18 Feb 2021 at 09:47, Alex Bennée wrote: > > The following changes since commit 1af5629673bb5c1592d993f9fb6119a62845f576: > > Merge remote-tracking branch > 'remotes/dgilbert-gitlab/tags/pull-virtiofs-20210216' into staging > (2021-02-17 14:44:18 +) > > are available in the Git repository at: > > https://github.com/stsquad/qemu.git tags/pull-plugin-updates-180221-1 > > for you to fetch changes up to df55e2a701d02bc01b9425843c667fd0cb4cdfa9: > > tests/acceptance: add a memory callback check (2021-02-18 08:19:23 +) > > > Plugin updates: > > - expose vdev name in PCI memory registration > - new hwprofile plugin > - bunch of style cleanups to contrib/plugins > - fix call signature of inline instrumentation > - re-factor the io_recompile code to push specialisation into hooks > - add some acceptance tests for the plugins > - clean-up and remove CF_NOCACHE handling from TCG > - fix instrumentation of cpu_io_recompile sections > - expand tests to check inline and cb count the same > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0 for any user-visible changes. -- PMM
Re: [PATCH v2 7/7] tests/avocado: add boot_xen tests
On Wed, Feb 17, 2021 at 10:22:50PM +, Alex Bennée wrote: > > I think the solution is to use archive links here. There is a snapshot > archive of sid (we've used it in the past) but I suspect there isn't an > archive of old stable packages for a reason. > If the packages you need are available on archives (which I wasn't aware of), then without a doubt it is the best solution. I assume you're going to look into that (I'll keep an eye on the list about it). Regards, - Cleber. signature.asc Description: PGP signature
[PATCH 1/2] sev: use explicit indices for mapping firmware error codes to strings
This can help lower any margin for error when making future additions to
the list, especially if they're made out of order.
While doing so, make capitalization of ASID consistent with its usage in
the SEV firmware spec (Asid -> ASID).
Signed-off-by: Connor Kuehl
---
target/i386/sev.c | 46 +++---
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 0f414df02f..0de690058e 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -88,29 +88,29 @@ static SevGuestState *sev_guest;
static Error *sev_mig_blocker;
static const char *const sev_fw_errlist[] = {
-"",
-"Platform state is invalid",
-"Guest state is invalid",
-"Platform configuration is invalid",
-"Buffer too small",
-"Platform is already owned",
-"Certificate is invalid",
-"Policy is not allowed",
-"Guest is not active",
-"Invalid address",
-"Bad signature",
-"Bad measurement",
-"Asid is already owned",
-"Invalid ASID",
-"WBINVD is required",
-"DF_FLUSH is required",
-"Guest handle is invalid",
-"Invalid command",
-"Guest is active",
-"Hardware error",
-"Hardware unsafe",
-"Feature not supported",
-"Invalid parameter"
+[SEV_RET_SUCCESS]= "",
+[SEV_RET_INVALID_PLATFORM_STATE] = "Platform state is invalid",
+[SEV_RET_INVALID_GUEST_STATE]= "Guest state is invalid",
+[SEV_RET_INAVLID_CONFIG] = "Platform configuration is invalid",
+[SEV_RET_INVALID_LEN]= "Buffer too small",
+[SEV_RET_ALREADY_OWNED] = "Platform is already owned",
+[SEV_RET_INVALID_CERTIFICATE]= "Certificate is invalid",
+[SEV_RET_POLICY_FAILURE] = "Policy is not allowed",
+[SEV_RET_INACTIVE] = "Guest is not active",
+[SEV_RET_INVALID_ADDRESS]= "Invalid address",
+[SEV_RET_BAD_SIGNATURE] = "Bad signature",
+[SEV_RET_BAD_MEASUREMENT]= "Bad measurement",
+[SEV_RET_ASID_OWNED] = "ASID is already owned",
+[SEV_RET_INVALID_ASID] = "Invalid ASID",
+[SEV_RET_WBINVD_REQUIRED]= "WBINVD is required",
+[SEV_RET_DFFLUSH_REQUIRED] = "DF_FLUSH is required",
+[SEV_RET_INVALID_GUEST] = "Guest handle is invalid",
+[SEV_RET_INVALID_COMMAND]= "Invalid command",
+[SEV_RET_ACTIVE] = "Guest is active",
+[SEV_RET_HWSEV_RET_PLATFORM] = "Hardware error",
+[SEV_RET_HWSEV_RET_UNSAFE] = "Hardware unsafe",
+[SEV_RET_UNSUPPORTED]= "Feature not supported",
+[SEV_RET_INVALID_PARAM] = "Invalid parameter",
};
#define SEV_FW_MAX_ERROR ARRAY_SIZE(sev_fw_errlist)
--
2.29.2
[PATCH 0/2] SEV firmware error list touchups
Connor Kuehl (2): sev: use explicit indices for mapping firmware error codes to strings sev: add missing firmware error conditions target/i386/sev.c | 48 --- 1 file changed, 25 insertions(+), 23 deletions(-) -- 2.29.2
[PATCH 2/2] sev: add missing firmware error conditions
The SEV userspace header[1] exports a couple of other error conditions that
aren't listed in QEMU's SEV implementation, so let's just round out the
list.
[1] linux-headers/linux/psp-sev.h
Signed-off-by: Connor Kuehl
---
target/i386/sev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 0de690058e..37690ae809 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -111,6 +111,8 @@ static const char *const sev_fw_errlist[] = {
[SEV_RET_HWSEV_RET_UNSAFE] = "Hardware unsafe",
[SEV_RET_UNSUPPORTED]= "Feature not supported",
[SEV_RET_INVALID_PARAM] = "Invalid parameter",
+[SEV_RET_RESOURCE_LIMIT] = "Required firmware resource depleted",
+[SEV_RET_SECURE_DATA_INVALID]= "Part-specific integrity check failure",
};
#define SEV_FW_MAX_ERROR ARRAY_SIZE(sev_fw_errlist)
--
2.29.2
Re: [PATCH v2 7/7] tests/avocado: add boot_xen tests
On Thu, Feb 18, 2021 at 10:43:54AM +0100, Philippe Mathieu-Daudé wrote:
> On 2/17/21 9:46 PM, Cleber Rosa wrote:
> > On Thu, Feb 11, 2021 at 05:19:45PM +, Alex Bennée wrote:
> >> These tests make sure we can boot the Xen hypervisor with a Dom0
> >> kernel using the guest-loader. We currently have to use a kernel I
> >> built myself because there are issues using the Debian kernel images.
> >>
> >> Signed-off-by: Alex Bennée
> >> ---
> >> MAINTAINERS | 1 +
> >> tests/acceptance/boot_xen.py | 117 +++
> >> 2 files changed, 118 insertions(+)
> >> create mode 100644 tests/acceptance/boot_xen.py
>
> >> +class BootXen(BootXenBase):
> >> +
> >> +@skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
> >> +def test_arm64_xen_411_and_dom0(self):
> >> +"""
> >> +:avocado: tags=arch:aarch64
> >> +:avocado: tags=accel:tcg
> >> +:avocado: tags=cpu:cortex-a57
> >> +:avocado: tags=machine:virt
> >> +"""
> >> +xen_url = ('https://deb.debian.org/debian/'
> >> + 'pool/main/x/xen/'
> >> +
> >> 'xen-hypervisor-4.11-arm64_4.11.4+37-g3263f257ca-1_arm64.deb')
> >> +xen_sha1 = '034e634d4416adbad1212d59b62bccdcda63e62a'
> >
> > This URL is already giving 404s because of a new pacakge. I found
> > this to work (but yeah, won't probably last long):
> >
> > xen_url = ('http://deb.debian.org/debian/'
> >'pool/main/x/xen/'
> >
> > 'xen-hypervisor-4.11-arm64_4.11.4+57-g41a822c392-2_arm64.deb')
> > xen_sha1 = 'b5a6810fc67fd50fa36afdfdfe88ce3153dd3a55'
>
> This is not the same package version... Please understand the developer
> has to download the Debian package sources, check again the set of
> downstream changes between 37 and 57. Each distrib number might contain
> multiple downstream patches. Then the testing has to be done again,
> often enabling tracing or doing single-stepping in gdb. This has a
> cost in productivity. This is why I insist I prefer to use archived
> well tested artifacts, rather than changing package URL randomly.
>
I understand it's not the same version... but from my different and
limited PoV it was the obvious thing to suggest during a review. Of
course using stable archived versions is much better (I believe Alex
will look into that for these packages).
Best,
- Cleber.
signature.asc
Description: PGP signature
Re: [PATCH 0/2] SEV firmware error list touchups
On 2/18/21 4:16 PM, Connor Kuehl wrote:
> Connor Kuehl (2):
> sev: use explicit indices for mapping firmware error codes to strings
> sev: add missing firmware error conditions
>
> target/i386/sev.c | 48 ---
> 1 file changed, 25 insertions(+), 23 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé
To avoid this problem in future (new error code added on the Linux
kernel side) would it be acceptable to add a 3rd patch as:
-- >8 --
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 0f414df02f3..e086d3198e8 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -155,9 +155,12 @@ sev_platform_ioctl(int fd, int cmd, void *data, int
*error)
static const char *
fw_error_to_str(int code)
{
+QEMU_BUILD_BUG_ON(SEV_RET_SECURE_DATA_INVALID + 1 == SEV_RET_MAX);
+
if (code < 0 || code >= SEV_FW_MAX_ERROR) {
return "unknown error";
}
+assert(sev_fw_errlist[code]);
return sev_fw_errlist[code];
}
---
which triggers a build error if scripts/update-linux-headers.sh
added another sev_ret_code entry?
[PATCH v2 00/38] ppc: qemu: Convert qemu-ppce500 to driver model and enable additional driver support
At present when building qemu-ppce500 the following warnings are seen:
= WARNING ==
This board does not use CONFIG_DM. CONFIG_DM will be
compulsory starting with the v2020.01 release.
Failure to update may result in board removal.
UPD include/generated/timestamp_autogenerated.h
See doc/driver-model/migration.rst for more info.
= WARNING ==
This board does not use CONFIG_DM_PCI Please update
the board to use CONFIG_DM_PCI before the v2019.07 release.
Failure to update by the deadline may result in board removal.
See doc/driver-model/migration.rst for more info.
= WARNING ==
This board does not use CONFIG_DM_ETH (Driver Model
for Ethernet drivers). Please update the board to use
CONFIG_DM_ETH before the v2020.07 release. Failure to
update by the deadline may result in board removal.
See doc/driver-model/migration.rst for more info.
The conversion of qemu-ppce500 board to driver model is long overdue.
When testing the exisitng qemu-ppce500 support, PCI was found broken.
This is caused by 2 separate issues:
- One issue was caused by U-Boot:
Commit e002474158d1 ("pci: pci-uclass: Dynamically allocate the PCI regions")
Patch #1 updated the non-DM fsl_pci_init driver to dynamically allocate the
PCI regions, to keep in sync with the pci uclass driver
- One issue was caused by QEMU:
commit e6b4e5f4795b ("PPC: e500: Move CCSR and MMIO space to upper end of
address space")
commit cb3778a0455a ("PPC: e500 pci host: Add support for ATMUs")
Patch #3-4 fixed this issue to keep in sync with latest QEMU upstream
Patch #5-8, #34-36 are minor fixes and clean-ups.
Starting from patch#9, these are driver model conversion patches.
Patch #11-17 are mainly related to CONFIG_ADDR_MAP, a library to support targets
that have non-identity virtual-physical address mappings. A new command
'addrmap'
is introduced to aid debugging, and a fix to arch/powerpc/asm/include/io.h is
made to correct the usage of CONFIG_ADDR_MAP as it can only be used in the post-
relocation phase. Also the initialization of this library is moved a bit earlier
in the post-relocation phase otherwise device drivers won't work.
Patch #19-21 are 85xx PCI driver fixes. It adds support to controller register
physical address beyond 32-bit, as well as support to 64-bit bus and cpu address
as current upstream QEMU uses 64-bit cpu address.
Starting from patch#24, these are additional driver support patches.
Patch #24, #26 are minor fix to the 'virtio' command and BLK driver dependency.
Patch #25 enables the VirtIO NET support as by default a VirtIO standard PCI
networking device is connected as an ethernet interface at PCI address 0.1.0.
Patch #27 enables the VirtIO BLK driver support.
Patch #28-30 enables the GPIO support.
Patch #31-32 enables poweroff via GPIO.
Patch #33 enables RTC over the I2C bus.
Patch #37 moves the qemu-ppce500 boards codes to board/emulation as that is the
place for other QEMU targets like x86, arm, riscv.
Patch #38 adds a reST document to describe how to build and run U-Boot for the
QEMU ppce500 machine.
I hope we can make this series to U-Boot v2021.04 release.
This series is available at u-boot-x86/qemu-ppc for testing.
This cover letter is cc'ed to QEMU mailing list for a heads-up.
A future patch will be sent to QEMU mailing list to bring its in-tree
U-Boot source codes up-to-date.
Changes in v2:
- drop the revert patch of commit e002474158d1
- new patch: pci: fsl_pci_init: Dynamically allocate the PCI regions
- add more details in the commit message, and put some comments
in the codes to explain why
- add doc/usage/addrmap.rst
- new patch: test: cmd: Add a basic test for 'addrmap' command
- new patch: virtio: Fix VirtIO BLK driver dependency
- new patch: ppc: qemu: Enable VirtIO BLK support
- new patch: ppc: mpc85xx: Add 'gpibe' register to 'struct ccsr_gpio'
- new patch: gpio: mpc8xxx: Support controller register physical address beyond
32-bit
- new patch: ppc: qemu: Enable GPIO support
- new patch: dm: sysreset: Add a Kconfig option for the 'reset' command
- new patch: ppc: qemu: Enable support for power off via GPIO
- new patch: ppc: qemu: Enable RTC support via I2C
- new patch: ppc: qemu: Delete the temporary FDT virtual-physical mapping after
U-Boot is relocated
- new patch: ppc: qemu: Drop a custom env variable 'fdt_addr_r'
- new patch: ppc: qemu: Drop fixed_sdram()
- add descriptions for VirtIO BLK, RTC and power off
Bin Meng (38):
pci: fsl_pci_init: Dynamically allocate the PCI regions
ppc: qemu: Update MAINTAINERS for correct email address
common: fdt_support: Support special case of PCI address in
fdt_read_prop()
ppc: qemu: Support non-identity PCI bus address
ppc: qemu: Fix CONFIG_SYS_PCI_MAP_END
Re: [PATCH 3/7] block/qcow2: use compressed write cache
On 11.02.21 13:53, Vladimir Sementsov-Ogievskiy wrote: 10.02.2021 20:11, Max Reitz wrote: On 29.01.21 17:50, Vladimir Sementsov-Ogievskiy wrote: Introduce a new option: compressed-cache-size, with default to 64 clusters (to be not less than 64 default max-workers for backup job). Signed-off-by: Vladimir Sementsov-Ogievskiy --- qapi/block-core.json | 8 +++- block/qcow2.h | 4 ++ block/qcow2-refcount.c | 13 +++ block/qcow2.c | 87 -- 4 files changed, 108 insertions(+), 4 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index 9f555d5c1d..e0be6657f3 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3202,6 +3202,11 @@ # an image, the data file name is loaded from the image # file. (since 4.0) # +# @compressed-cache-size: The maximum size of compressed write cache in +# bytes. If positive must be not less than +# cluster size. 0 disables the feature. Default +# is 64 * cluster_size. (since 6.0) Do we need this, really? If you don’t use compression, the cache won’t use any memory, right? Do you plan on using this option? I’d just set it to a sane default. OK for me OTOH, “a sane default” poses two questions, namely whether 64 * cluster_size is reasonable – with subclusters, the cluster size may be rather high, so 64 * cluster_size may well be like 128 MB. Are 64 clusters really necessary for a reasonable performance? Second, I think I could live with a rather high default if clusters are flushed as soon as they are full. OTOH, as I briefly touched on, in practice, I suppose compressed images are just written to constantly, so even if clusters are flushed as soon as they are full, the cache will still remain full all the time. Different topic: Why is the cache disableable? I thought there are no downsides? to compare performance for example.. Well :D Doesn’t seem like a reason to expose it to the outside, though, I don’t know. Max
Re: [PATCH 1/1] hw/s390x: fix build for virtio-9p-ccw
On Thu, 18 Feb 2021 04:40:59 +0100
Halil Pasic wrote:
> Commit 2c44220d05 ("meson: convert hw/arch*"), which migrated the old
> Makefile.objs to meson.build accidentally excluded virtio-ccw-9p.c and
> thus the virtio-9p-ccw device from the build (and potentially also
> included the file virtio-ccw-blk.c twice in the source set). And since
> CONFIG_VIRTFS can't be used the way it was used here (see commit
> 2c9dce0196 ("meson: do not use CONFIG_VIRTFS")), the preconditions have
> to be written differently.
>
> Let's fix this!
>
> Signed-off-by: Halil Pasic
> Fixes: 2c44220d05 ("meson: convert hw/arch*")
> Reported-by: Jakob Naucke
> Cc: qemu-sta...@nongnu.org
> ---
> hw/s390x/meson.build | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> index 2a7818d94b..91495b5631 100644
> --- a/hw/s390x/meson.build
> +++ b/hw/s390x/meson.build
> @@ -40,7 +40,9 @@ virtio_ss.add(when: 'CONFIG_VIRTIO_NET', if_true:
> files('virtio-ccw-net.c'))
> virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-ccw-rng.c'))
> virtio_ss.add(when: 'CONFIG_VIRTIO_SCSI', if_true:
> files('virtio-ccw-scsi.c'))
> virtio_ss.add(when: 'CONFIG_VIRTIO_SERIAL', if_true:
> files('virtio-ccw-serial.c'))
> -virtio_ss.add(when: ['CONFIG_VIRTIO_9P', 'CONFIG_VIRTFS'], if_true:
> files('virtio-ccw-blk.c'))
> +if have_virtfs
> + virtio_ss.add(when: 'CONFIG_VIRTIO_9P', if_true: files('virtio-ccw-9p.c'))
> +endif
> virtio_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true:
> files('vhost-vsock-ccw.c'))
> virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true:
> files('vhost-user-fs-ccw.c'))
> s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
>
> base-commit: 1af5629673bb5c1592d993f9fb6119a62845f576
Thanks, applied.
Re: [PATCH v2 4/6] qapi: Apply aliases in qobject-input-visitor
Am 18.02.2021 um 14:39 hat Markus Armbruster geschrieben:
> Kevin Wolf writes:
>
> > Am 17.02.2021 um 16:32 hat Markus Armbruster geschrieben:
> >> Kevin Wolf writes:
> >>
> >> > When looking for an object in a struct in the external representation,
> >> > check not only the currently visited struct, but also whether an alias
> >> > in the current StackObject matches and try to fetch the value from the
> >> > alias then. Providing two values for the same object through different
> >> > aliases is an error.
> >> >
> >> > Signed-off-by: Kevin Wolf
> >>
> >> Looking just at qobject_input_try_get_object() for now.
> >
> > :-(
> >
> > This patch doesn't even feel that complicated to me.
>
> I suspect it's just me having an unusually obtuse week.
>
> The code is straightforward enough. What I'm missing is a bit of "how
> does this accomplish the goal" and "why is this safe" here and there.
>
> > Old: Get the value from the QDict of the current StackObject with the
> > given name.
> >
> > New: First do alias resolution (with find_object_member), which results
> > in a StackObject and a name, and that's the QDict and key where you get
> > the value from.
>
> This part I understand.
>
> We simultaneously walk the QAPI type and the input QObject, consuming
> input as we go.
>
> Whenever the walk leaves a QAPI object type, we check the corresponding
> QObject has been consumed completely.
>
> With aliases, we additionally look for input in a certain enclosing
> input object (i.e. up the recursion stack). If found, consume.
>
> > Minor complication: Aliases can refer to members of nested objects that
> > may not be provided in the input. But we want these to work.
> >
> > For example, my chardev series defines aliases to flatten
> > SocketAddressLegacy (old syntax, I haven't rebased it yet):
> >
> > { 'union': 'SocketAddressLegacy',
> > 'data': {
> > 'inet': 'InetSocketAddress',
> > 'unix': 'UnixSocketAddress',
> > 'vsock': 'VsockSocketAddress',
> > 'fd': 'String' },
> > 'aliases': [
> > {'source': ['data']},
> > {'alias': 'fd', 'source': ['data', 'str']}
> > ]}
> >
> > Of course, the idea is that this input should work:
> >
> > { 'type': 'inet', 'hostname': 'localhost', 'port': '1234' }
> >
> > However, without implicit objects, parsing 'data' fails because it
> > expects an object, but none is given (we specified all of its members on
> > the top level through aliases). What we would have to give is:
> >
> > { 'type': 'inet', 'hostname': 'localhost', 'port': '1234', 'data': {} }
> >
> > And _that_ would work. Visiting 'data' succeeds because we now have the
> > object that the visitor expects, and when visiting its members, the
> > aliases fill in all of the mandatory values.
> >
> > So what this patch does is to implicitly assume the 'data': {} instead
> > of erroring out when we know that aliases exist that can still provide
> > values for the content of 'data'.
>
> Aliases exist than can still provide, but will they? What if input is
>
> {"type": "inet"}
>
> ?
>
> Your explanation makes me guess this is equivalent to
>
> {"type": "inet", "data": {}}
>
> which fails the visit, because mandatory members of "data" are missing.
> Fine.
Okay, if you want the gory details, then the answer is yes for this
case, but it depends.
If we're aliasing a single member, then we can easily check whether the
alias is actually specified. If it's not in the input, no implicit
object.
But in our example, it is a wildcard alias and we don't know yet which
aliases it will use. This depends on what the visitor for the implicit
object will do (future tense).
> If we make the members optional, it succeeds: qobject_input_optional()
> checks both the regular and the aliased input, finds neither, and
> returns false. Still fine.
>
> What if "data" is optional, too? Hmmm...
Yes, don't use optional objects in the middle of the path of a wildcard
alias unless there is no semantic difference between empty object and
absent object. This is documented in the code, but it might actually
still be missing from qapi-code-gen.txt.
> Example:
>
> { 'struct': 'Outer',
> 'data': { '*inner': 'Inner' },
>
> { 'struct': 'Inner',
> 'data': { '*true-name': 'str' } }
>
> For input {}, we get an Outer object with
>
> .has_inner = false,
> .inner = NULL
>
> Now add
>
> 'aliases': [ { 'name': 'alias-name',
> 'source': [ 'inner', 'true-name' ] } ] }
>
> What do we get now? Is it
>
> .has_inner = true,
> .inner = { .has_true_name = false,
> .true_name = NULL }
>
> perhaps?
I think this is the result you would get if you had used a wildcard
alias. But since you used a single-member alias, we would see that
'alias-name' is not in the input and actually still return the original
result:
.has_inner = false,
.inner = NULL
Kevin
Re: [PATCH] net: eepro100: validate various address values
Am 18.02.21 um 15:41 schrieb Peter Maydell: On Thu, 18 Feb 2021 at 14:13, P J P wrote: From: Prasad J Pandit While processing controller commands, eepro100 emulator gets command unit(CU) base address OR receive unit (RU) base address OR command block (CB) address from guest. If these values are not checked, it may lead to an infinite loop kind of issues. Add checks to avoid it. Reported-by: Ruhr-University Bochum Signed-off-by: Prasad J Pandit --- hw/net/eepro100.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c index 16e95ef9cc..afa1c9b2aa 100644 --- a/hw/net/eepro100.c +++ b/hw/net/eepro100.c @@ -843,7 +843,8 @@ static void action_command(EEPRO100State *s) bool bit_i; bool bit_nc; uint16_t ok_status = STATUS_OK; -s->cb_address = s->cu_base + s->cu_offset; +s->cb_address = s->cu_base + s->cu_offset; /* uint32_t overflow */ +assert (s->cb_address >= s->cu_base); We get these values from the guest; you can't just assert() on them. You need to do something else. My reading of the 8255x data sheet is that there is nothing in the hardware that forbids the guest from programming the device such that the cu_base + cu_offset wraps around: http://www.intel.com/content/dam/doc/manual/8255x-10-100-mbps-ethernet-controller-software-dev-manual.pdf -- page 30 says that this is all doing 32-bit arithmetic on addresses and doesn't say that there is any special case handling by the device of overflow of that addition. Your commit message isn't very clear about what the failure case is here, but I think the fix has to be something different from this. I agree with Peter. The hardware emulation in QEMU should try to do the same as the real hardware. Assertions (not with assert(), but with g_assert() and related functions) are good to catch implementation errors in QEMU code, but not to catch bad programming in guest code. In this special case the emulation code already includes a hack to catch endless loops caused by buggy guest code: it has a limit of 16 cycles and then prints a log message. This is a compromise to emulate the real hardware as good as possible without making QEMU running an endless loop or requiring an extra thread to emulate the hardware loop until it is stopped by new I/O operations. Stefan
Re: [PATCH] Correct CRIS TCG register lifetime management
On 2/18/21 12:56 AM, Stefan Sandström wrote:
> From: Stefan Sandstrom
>
> Add and fix deallocation of temporary TCG registers in CRIS code
> generation.
>
> Change-Id: I17fce5d95bdc4418337ba885d53ba97afb1bafcc
> Signed-off-by: Stefan Sandstrom
> ---
> target/cris/translate.c | 146 ++--
> target/cris/translate_v10.c.inc | 70 ---
> 2 files changed, 156 insertions(+), 60 deletions(-)
>
> diff --git a/target/cris/translate.c b/target/cris/translate.c
> index c893f87..ae903a5 100644
> --- a/target/cris/translate.c
> +++ b/target/cris/translate.c
> @@ -177,9 +177,13 @@ static inline void t_gen_mov_TN_preg(TCGv tn, int r)
> {
> assert(r >= 0 && r <= 15);
> if (r == PR_BZ || r == PR_WZ || r == PR_DZ) {
> -tcg_gen_mov_tl(tn, tcg_const_tl(0));
> +TCGv c0 = tcg_const_tl(0);
> +tcg_gen_mov_tl(tn, c0);
> +tcg_temp_free(c0);
In cases like this, you should just use tcg_gen_movi_tl(tn, 0).
> } else if (r == PR_VR) {
> -tcg_gen_mov_tl(tn, tcg_const_tl(32));
> +TCGv c32 = tcg_const_tl(32);
> +tcg_gen_mov_tl(tn, c32);
> +tcg_temp_free(c32);
movi
> static void cris_lock_irq(DisasContext *dc)
> {
> +TCGv c1 = tcg_const_tl(1);
> dc->clear_locked_irq = 0;
> -t_gen_mov_env_TN(locked_irq, tcg_const_tl(1));
> +t_gen_mov_env_TN(locked_irq, c1);
> +tcg_temp_free(c1);
> }
good.
> @@ -885,8 +891,10 @@ static void gen_tst_cc (DisasContext *dc, TCGv cc, int
> cond)
> case CC_EQ:
> if ((arith_opt || move_opt)
> && dc->cc_x_uptodate != (2 | X_FLAG)) {
> +TCGv c0 = tcg_const_tl(0);
> tcg_gen_setcond_tl(TCG_COND_EQ, cc,
> -cc_result, tcg_const_tl(0));
> +cc_result, c0);
> +tcg_temp_free(c0);
setcondi
> -tcg_gen_shl_tl(t0, cpu_R[dc->op2], tcg_const_tl(dc->zzsize));
> +c = tcg_const_tl(dc->zzsize);
> +tcg_gen_shl_tl(t0, cpu_R[dc->op2], c);
shli
> @@ -3023,14 +3083,16 @@ static unsigned int crisv32_decoder(CPUCRISState
> *env, DisasContext *dc)
> /* Single-stepping ? */
> if (dc->tb_flags & S_FLAG) {
> TCGLabel *l1 = gen_new_label();
> +TCGv c = tcg_const_tl(3);
> tcg_gen_brcondi_tl(TCG_COND_NE, cpu_PR[PR_SPC], dc->pc, l1);
> /* We treat SPC as a break with an odd trap vector. */
> cris_evaluate_flags(dc);
> -t_gen_mov_env_TN(trap_vector, tcg_const_tl(3));
> +t_gen_mov_env_TN(trap_vector, c);
Here, you cannot lift the const allocation above the branch.
There are enough uses of t_gen_mov_env_TN with a constant that it would be
worthwhile creating a new function that takes the constant.
r~
