[Bug 1883984] Re: QEMU S/390x sqxbr (128-bit IEEE 754 square root) crashes qemu-system-s390x
** Description changed:
+ [Impact]
+
+ * An instruction was described wrong so that on usage the program would
+crash.
+
+ [Test Case]
+
+ * Run s390x in emulation and there use this program:
+For simplicity and speed you can use KVM guest as usual on s390x, that
+after prep&install&compile of the test you run in qemu-tcg like:
+
+$ sudo qemu-system-s390x -machine s390-ccw-virtio,accel=tcg -cpu
max,zpci=on -serial mon:stdio -display none -m 4096 -nic
user,model=virtio,hostfwd=tcp::-:22 -drive
file=/var/lib/uvtool/libvirt/images/focal-sqxbr.qcow,if=none,id=drive-virtio-disk0,format=qcow2,cache=none
-device
virtio-blk-ccw,devno=fe.0.0001,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,scsi=off
+Obviously is you have no s390x access you need to use emulation right
+away.
+
+ * Build and run failing program
+$ sudo apt install clang
+$ cat > bug-sqrtl-one-line.c << EOF
+ int main(void) { volatile long double x, r; x = 4.0L; __asm__
+ __volatile__("sqxbr %0, %1" : "=f" (r) : "f" (x)); return (0);}
+ EOF
+$ cc bug-sqrtl-one-line.c
+$ ./a.out
+Segmentation fault (core dumped)
+
+qemu is dead by now as long as the bug is present
+
+ [Regression Potential]
+
+ * The change only modifies 128 bit square root on s390x so regressions
+should be limited to exactly that - which formerly before this fix was
+a broken instruction.
+
+ [Other Info]
+
+ * n/a
+
+ ---
+
In porting software to guest Ubuntu 18.04 and 20.04 VMs for S/390x, I
discovered
that some of my own numerical programs, and also a GNU configure script for at
least one package with CC=clang, would cause an instant crash of the VM,
sometimes
also destroying recently opened files, and producing long strings of NUL
characters
in /var/log/syslog in the S/390 guest O/S.
Further detective work narrowed the cause of the crash down to a single IBM
S/390
instruction: sqxbr (128-bit IEEE 754 square root). Here is a one-line program
- that when compiled and run on a VM hosted on QEMUcc emulator version 4.2.0
- (Debian 1:4.2-3ubuntu6.1) [hosted on Ubuntu 20.04 on a Dell Precision 7920
- workstation with an Intel Xeon Platinum 8253 CPU], and also on QEMU emulator
+ that when compiled and run on a VM hosted on QEMUcc emulator version 4.2.0
+ (Debian 1:4.2-3ubuntu6.1) [hosted on Ubuntu 20.04 on a Dell Precision 7920
+ workstation with an Intel Xeon Platinum 8253 CPU], and also on QEMU emulator
version 5.0.0, reproducibly produces a VM crash under qemu-system-s390x.
% cat bug-sqrtl-one-line.c
int main(void) { volatile long double x, r; x = 4.0L; __asm__
__volatile__("sqxbr %0, %1" : "=f" (r) : "f" (x)); return (0);}
% cc bug-sqrtl-one-line.c && ./a.out
Segmentation fault (core dumped)
The problem code may be the function float128_sqrt() defined in
qemu-5.0.0/fpu/softfloat.c
starting at line 7619. I have NOT attempted to run the qemu-system-s390x
executable
under a debugger. However, I observe that S/390 is the only CPU family that
I know of,
except possibly for a Fujitsu SPARC-64, that has a 128-bit square root in
hardware.
Thus, this instruction bug may not have been seen before.
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1883984
Title:
QEMU S/390x sqxbr (128-bit IEEE 754 square root) crashes qemu-system-
s390x
Status in QEMU:
Fix Committed
Status in qemu package in Ubuntu:
Fix Released
Status in qemu source package in Focal:
Triaged
Bug description:
[Impact]
* An instruction was described wrong so that on usage the program would
crash.
[Test Case]
* Run s390x in emulation and there use this program:
For simplicity and speed you can use KVM guest as usual on s390x, that
after prep&install&compile of the test you run in qemu-tcg like:
$ sudo qemu-system-s390x -machine s390-ccw-virtio,accel=tcg -cpu
max,zpci=on -serial mon:stdio -display none -m 4096 -nic
user,model=virtio,hostfwd=tcp::-:22 -drive
file=/var/lib/uvtool/libvirt/images/focal-sqxbr.qcow,if=none,id=drive-virtio-disk0,format=qcow2,cache=none
-device
virtio-blk-ccw,devno=fe.0.0001,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,scsi=off
Obviously is you have no s390x access you need to use emulation right
away.
* Build and run failing program
$ sudo apt install clang
$ cat > bug-sqrtl-one-line.c << EOF
int main(void) { volatile long double x, r; x = 4.0L; __asm__
__volatile__("sqxbr %0, %1" : "=f" (r) : "f" (x)); return (0);}
EOF
$ cc bug-sqrtl-one-line.c
$ ./a.out
Segmentation fault (core dumped)
qemu is dead by now as long as the bug is present
[Regression Potential]
* The change only modifies 128 bit square root on s390x so regressions
should be limited to exactly that - which formerly before this fix was
a broken instruction.
Re: [ovirt-devel] Re: device compatibility interface for live migration with assigned devices
On Wed, Aug 19, 2020 at 02:57:34PM +0800, Jason Wang wrote:
>
> On 2020/8/19 上午11:30, Yan Zhao wrote:
> > hi All,
> > could we decide that sysfs is the interface that every VFIO vendor driver
> > needs to provide in order to support vfio live migration, otherwise the
> > userspace management tool would not list the device into the compatible
> > list?
> >
> > if that's true, let's move to the standardizing of the sysfs interface.
> > (1) content
> > common part: (must)
> > - software_version: (in major.minor.bugfix scheme)
>
>
> This can not work for devices whose features can be negotiated/advertised
> independently. (E.g virtio devices)
>
sorry, I don't understand here, why virtio devices need to use vfio interface?
I think this thread is discussing about vfio related devices.
>
> > - device_api: vfio-pci or vfio-ccw ...
> > - type: mdev type for mdev device or
> > a signature for physical device which is a counterpart for
> >mdev type.
> >
> > device api specific part: (must)
> >- pci id: pci id of mdev parent device or pci id of physical pci
> > device (device_api is vfio-pci)API here.
>
>
> So this assumes a PCI device which is probably not true.
>
for device_api of vfio-pci, why it's not true?
for vfio-ccw, it's subchannel_type.
>
> >- subchannel_type (device_api is vfio-ccw)
> > vendor driver specific part: (optional)
> >- aggregator
> >- chpid_type
> >- remote_url
>
>
> For "remote_url", just wonder if it's better to integrate or reuse the
> existing NVME management interface instead of duplicating it here. Otherwise
> it could be a burden for mgmt to learn. E.g vendor A may use "remote_url"
> but vendor B may use a different attribute.
>
it's vendor driver specific.
vendor specific attributes are inevitable, and that's why we are
discussing here of a way to standardizing of it.
our goal is that mgmt can use it without understanding the meaning of vendor
specific attributes.
>
> >
> > NOTE: vendors are free to add attributes in this part with a
> > restriction that this attribute is able to be configured with the same
> > name in sysfs too. e.g.
>
>
> Sysfs works well for common attributes belongs to a class, but I'm not sure
> it can work well for device/vendor specific attributes. Does this mean mgmt
> need to iterate all the attributes in both src and dst?
>
no. just attributes under migration directory.
>
> > for aggregator, there must be a sysfs attribute in device node
> > /sys/devices/pci:00/:00:02.0/882cc4da-dede-11e7-9180-078a62063ab1/intel_vgpu/aggregator,
> > so that the userspace tool is able to configure the target device
> > according to source device's aggregator attribute.
> >
> >
> > (2) where and structure
> > proposal 1:
> > |- [path to device]
> >|--- migration
> >| |--- self
> >| ||-software_version
> >| ||-device_api
> >| ||-type
> >| ||-[pci_id or subchannel_type]
> >| ||-
> >| |--- compatible
> >| ||-software_version
> >| ||-device_api
> >| ||-type
> >| ||-[pci_id or subchannel_type]
> >| ||-
> > multiple compatible is allowed.
> > attributes should be ASCII text files, preferably with only one value
> > per file.
> >
> >
> > proposal 2: use bin_attribute.
> > |- [path to device]
> >|--- migration
> >| |--- self
> >| |--- compatible
> >
> > so we can continue use multiline format. e.g.
> > cat compatible
> >software_version=0.1.0
> >device_api=vfio_pci
> >type=i915-GVTg_V5_{val1:int:1,2,4,8}
> >pci_id=80865963
> >aggregator={val1}/2
>
>
> So basically two questions:
>
> - how hard to standardize sysfs API for dealing with compatibility check (to
> make it work for most types of devices)
sorry, I just know we are in the process of standardizing of it :)
> - how hard for the mgmt to learn with a vendor specific attributes (vs
> existing management API)
what is existing management API?
Thanks
Re: [RFC v3 1/1] memory: Skip bad range assertion if notifier supports arbitrary masks
On 2020/8/18 下午10:24, Eugenio Perez Martin wrote:
On Wed, Aug 12, 2020 at 10:49 AM Eugenio Perez Martin
wrote:
On Wed, Aug 12, 2020 at 4:24 AM Jason Wang wrote:
On 2020/8/12 上午1:55, Eugenio Pérez wrote:
Signed-off-by: Eugenio Pérez
---
hw/virtio/vhost.c | 2 +-
include/exec/memory.h | 2 ++
softmmu/memory.c | 10 --
3 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 1a1384e7a6..e74ad9e09b 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -729,7 +729,7 @@ static void vhost_iommu_region_add(MemoryListener *listener,
iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr,
MEMTXATTRS_UNSPECIFIED);
iommu_notifier_init(&iommu->n, vhost_iommu_unmap_notify,
-IOMMU_NOTIFIER_UNMAP,
+IOMMU_NOTIFIER_UNMAP | IOMMU_NOTIFIER_IOTLB,
I think we can safely drop IOMMU_NOTIFIER_UNMAP here since device IOTLB
is sufficient.
Btw, IOMMU_NOTIFIER_IOTLB is kind of confusing, maybe something like
IOMMU_NOTIFIER_DEVIOTLB.
Got it, will change.
section->offset_within_region,
int128_get64(end),
iommu_idx);
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 307e527835..4d94c1e984 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -87,6 +87,8 @@ typedef enum {
IOMMU_NOTIFIER_UNMAP = 0x1,
/* Notify entry changes (newly created entries) */
IOMMU_NOTIFIER_MAP = 0x2,
+/* Notify changes on IOTLB entries */
+IOMMU_NOTIFIER_IOTLB = 0x04,
} IOMMUNotifierFlag;
#define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_MAP | IOMMU_NOTIFIER_UNMAP)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index af25987518..e2c5f6d0e7 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1895,6 +1895,7 @@ void memory_region_notify_one(IOMMUNotifier *notifier,
(we probably need a better name of this function, at least something
like "memory_region_iommu_notify_one").
Ok will change.
{
IOMMUNotifierFlag request_flags;
hwaddr entry_end = entry->iova + entry->addr_mask;
+IOMMUTLBEntry tmp = *entry;
/*
* Skip the notification if the notification does not overlap
@@ -1904,7 +1905,12 @@ void memory_region_notify_one(IOMMUNotifier *notifier,
return;
}
-assert(entry->iova >= notifier->start && entry_end <= notifier->end);
+if (notifier->notifier_flags & IOMMU_NOTIFIER_IOTLB) {
+tmp.iova = MAX(tmp.iova, notifier->start);
+tmp.addr_mask = MIN(entry_end, notifier->end) - tmp.iova;
Any reason for doing such re-calculation here, a comment would be helpful.
It was proposed by Peter, but I understand as limiting the
address+range we pass to the notifier. Although vhost seems to support
it as long as it contains (notifier->start, notifier->end) in range, a
future notifier might not.
Yes, actually, I feel confused after reading the codes. Is
notifier->start IOVA or GPA?
In vfio.c, we did:
iommu_notifier_init(&giommu->n, vfio_iommu_map_notify,
IOMMU_NOTIFIER_ALL,
section->offset_within_region,
int128_get64(llend),
iommu_idx);
So it looks to me the start and end are GPA, but the assertion above
check it against IOVA which seems to be wrong
Thanks
It could be done as iommu_entry_crop(IOMMUTLBEntry *entry, const
IOMMUNotifier *notifier) though.
Re: [ovirt-devel] Re: device compatibility interface for live migration with assigned devices
On 2020/8/19 下午2:59, Yan Zhao wrote:
On Wed, Aug 19, 2020 at 02:57:34PM +0800, Jason Wang wrote:
On 2020/8/19 上午11:30, Yan Zhao wrote:
hi All,
could we decide that sysfs is the interface that every VFIO vendor driver
needs to provide in order to support vfio live migration, otherwise the
userspace management tool would not list the device into the compatible
list?
if that's true, let's move to the standardizing of the sysfs interface.
(1) content
common part: (must)
- software_version: (in major.minor.bugfix scheme)
This can not work for devices whose features can be negotiated/advertised
independently. (E.g virtio devices)
sorry, I don't understand here, why virtio devices need to use vfio interface?
I don't see any reason that virtio devices can't be used by VFIO. Do you?
Actually, virtio devices have been used by VFIO for many years:
- passthrough a hardware virtio devices to userspace(VM) drivers
- using virtio PMD inside guest
I think this thread is discussing about vfio related devices.
- device_api: vfio-pci or vfio-ccw ...
- type: mdev type for mdev device or
a signature for physical device which is a counterpart for
mdev type.
device api specific part: (must)
- pci id: pci id of mdev parent device or pci id of physical pci
device (device_api is vfio-pci)API here.
So this assumes a PCI device which is probably not true.
for device_api of vfio-pci, why it's not true?
for vfio-ccw, it's subchannel_type.
Ok but having two different attributes for the same file is not good
idea. How mgmt know there will be a 3rd type?
- subchannel_type (device_api is vfio-ccw)
vendor driver specific part: (optional)
- aggregator
- chpid_type
- remote_url
For "remote_url", just wonder if it's better to integrate or reuse the
existing NVME management interface instead of duplicating it here. Otherwise
it could be a burden for mgmt to learn. E.g vendor A may use "remote_url"
but vendor B may use a different attribute.
it's vendor driver specific.
vendor specific attributes are inevitable, and that's why we are
discussing here of a way to standardizing of it.
Well, then you will end up with a very long list to discuss. E.g for
networking devices, you will have "mac", "v(x)lan" and a lot of other.
Note that "remote_url" is not vendor specific but NVME (class/subsystem)
specific.
The point is that if vendor/class specific part is unavoidable, why not
making all of the attributes vendor specific?
our goal is that mgmt can use it without understanding the meaning of vendor
specific attributes.
I'm not sure this is the correct design of uAPI. Is there something
similar in the existing uAPIs?
And it might be hard to work for virtio devices.
NOTE: vendors are free to add attributes in this part with a
restriction that this attribute is able to be configured with the same
name in sysfs too. e.g.
Sysfs works well for common attributes belongs to a class, but I'm not sure
it can work well for device/vendor specific attributes. Does this mean mgmt
need to iterate all the attributes in both src and dst?
no. just attributes under migration directory.
for aggregator, there must be a sysfs attribute in device node
/sys/devices/pci:00/:00:02.0/882cc4da-dede-11e7-9180-078a62063ab1/intel_vgpu/aggregator,
so that the userspace tool is able to configure the target device
according to source device's aggregator attribute.
(2) where and structure
proposal 1:
|- [path to device]
|--- migration
| |--- self
| ||-software_version
| ||-device_api
| ||-type
| ||-[pci_id or subchannel_type]
| ||-
| |--- compatible
| ||-software_version
| ||-device_api
| ||-type
| ||-[pci_id or subchannel_type]
| ||-
multiple compatible is allowed.
attributes should be ASCII text files, preferably with only one value
per file.
proposal 2: use bin_attribute.
|- [path to device]
|--- migration
| |--- self
| |--- compatible
so we can continue use multiline format. e.g.
cat compatible
software_version=0.1.0
device_api=vfio_pci
type=i915-GVTg_V5_{val1:int:1,2,4,8}
pci_id=80865963
aggregator={val1}/2
So basically two questions:
- how hard to standardize sysfs API for dealing with compatibility check (to
make it work for most types of devices)
sorry, I just know we are in the process of standardizing of it :)
It's not easy. As I said, the current design can't work for virtio
devices and it's not hard to find other examples. I remember some Intel
devices have bitmask based capability registers.
- how hard for the mgmt to learn with a vendor specific attributes (vs
existing management API)
what is existing management API?
It depends on the type of devices. E.g for NVME, we've already had one
(/sys/kernel/config/nvme)?
Thanks
Tha
[Bug 1883984] Re: QEMU S/390x sqxbr (128-bit IEEE 754 square root) crashes qemu-system-s390x
Note: final upstream commit link
https://git.qemu.org/?p=qemu.git;a=commit;h=9bf728a09bf7509b27543664f9cca6f4f337f608
** Changed in: qemu (Ubuntu)
Assignee: Christian Ehrhardt (paelzer) => (unassigned)
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1883984
Title:
QEMU S/390x sqxbr (128-bit IEEE 754 square root) crashes qemu-system-
s390x
Status in QEMU:
Fix Committed
Status in qemu package in Ubuntu:
Fix Released
Status in qemu source package in Focal:
Triaged
Bug description:
[Impact]
* An instruction was described wrong so that on usage the program would
crash.
[Test Case]
* Run s390x in emulation and there use this program:
For simplicity and speed you can use KVM guest as usual on s390x, that
after prep&install&compile of the test you run in qemu-tcg like:
$ sudo qemu-system-s390x -machine s390-ccw-virtio,accel=tcg -cpu
max,zpci=on -serial mon:stdio -display none -m 4096 -nic
user,model=virtio,hostfwd=tcp::-:22 -drive
file=/var/lib/uvtool/libvirt/images/focal-sqxbr.qcow,if=none,id=drive-virtio-disk0,format=qcow2,cache=none
-device
virtio-blk-ccw,devno=fe.0.0001,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,scsi=off
Obviously is you have no s390x access you need to use emulation right
away.
* Build and run failing program
$ sudo apt install clang
$ cat > bug-sqrtl-one-line.c << EOF
int main(void) { volatile long double x, r; x = 4.0L; __asm__
__volatile__("sqxbr %0, %1" : "=f" (r) : "f" (x)); return (0);}
EOF
$ cc bug-sqrtl-one-line.c
$ ./a.out
Segmentation fault (core dumped)
qemu is dead by now as long as the bug is present
[Regression Potential]
* The change only modifies 128 bit square root on s390x so regressions
should be limited to exactly that - which formerly before this fix was
a broken instruction.
[Other Info]
* n/a
---
In porting software to guest Ubuntu 18.04 and 20.04 VMs for S/390x, I
discovered
that some of my own numerical programs, and also a GNU configure script for at
least one package with CC=clang, would cause an instant crash of the VM,
sometimes
also destroying recently opened files, and producing long strings of NUL
characters
in /var/log/syslog in the S/390 guest O/S.
Further detective work narrowed the cause of the crash down to a single IBM
S/390
instruction: sqxbr (128-bit IEEE 754 square root). Here is a one-line program
that when compiled and run on a VM hosted on QEMUcc emulator version 4.2.0
(Debian 1:4.2-3ubuntu6.1) [hosted on Ubuntu 20.04 on a Dell Precision 7920
workstation with an Intel Xeon Platinum 8253 CPU], and also on QEMU emulator
version 5.0.0, reproducibly produces a VM crash under qemu-system-s390x.
% cat bug-sqrtl-one-line.c
int main(void) { volatile long double x, r; x = 4.0L; __asm__
__volatile__("sqxbr %0, %1" : "=f" (r) : "f" (x)); return (0);}
% cc bug-sqrtl-one-line.c && ./a.out
Segmentation fault (core dumped)
The problem code may be the function float128_sqrt() defined in
qemu-5.0.0/fpu/softfloat.c
starting at line 7619. I have NOT attempted to run the qemu-system-s390x
executable
under a debugger. However, I observe that S/390 is the only CPU family that
I know of,
except possibly for a Fujitsu SPARC-64, that has a 128-bit square root in
hardware.
Thus, this instruction bug may not have been seen before.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1883984/+subscriptions
Re: [PATCH] hw/m68k: QOMify the mcf5206 system integration module
Patchew URL: https://patchew.org/QEMU/20200819065201.4045-1-h...@tuxfamily.org/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === TESTcheck-unit: tests/test-char Unexpected error in object_property_try_add() at /tmp/qemu-test/src/qom/object.c:1181: attempt to add duplicate property 'serial-id' to object (type 'container') ERROR test-char - too few tests run (expected 38, got 9) make: *** [check-unit] Error 1 make: *** Waiting for unfinished jobs TESTiotest-qcow2: 024 TESTiotest-qcow2: 025 --- raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=c7043a273bd442bb9a9adffcf0104667', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-n8gugtlv/src/docker-src.2020-08-19-03.37.05.10441:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2. filter=--filter=label=com.qemu.instance.uuid=c7043a273bd442bb9a9adffcf0104667 make[1]: *** [docker-run] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-n8gugtlv/src' make: *** [docker-run-test-quick@centos7] Error 2 real13m36.209s user0m8.909s The full log is available at http://patchew.org/logs/20200819065201.4045-1-h...@tuxfamily.org/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v8 17/20] multi-process: heartbeat messages to remote
On Fri, Aug 14, 2020 at 04:01:47PM -0700, Elena Ufimtseva wrote:
> On Tue, Aug 11, 2020 at 03:41:30PM +0100, Stefan Hajnoczi wrote:
> > On Fri, Jul 31, 2020 at 02:20:24PM -0400, Jagannathan Raman wrote:
> > > @@ -343,3 +349,49 @@ static void probe_pci_info(PCIDevice *dev, Error
> > > **errp)
> > > }
> > > }
> > > }
> > > +
> > > +static void hb_msg(PCIProxyDev *dev)
> > > +{
> > > +DeviceState *ds = DEVICE(dev);
> > > +Error *local_err = NULL;
> > > +MPQemuMsg msg = { 0 };
> > > +
> > > +msg.cmd = PROXY_PING;
> > > +msg.bytestream = 0;
> > > +msg.size = 0;
> > > +
> > > +(void)mpqemu_msg_send_and_await_reply(&msg, dev->ioc, &local_err);
> > > +if (local_err) {
> > > +error_report_err(local_err);
> > > +qio_channel_close(dev->ioc, &local_err);
> > > +error_setg(&error_fatal, "Lost contact with device %s", ds->id);
> > > +}
> > > +}
> >
> > Here is my feedback from the last revision. Was this addressed?
> >
>
> Hi Stefan,
>
> Thank you for reviewing the patchset. In this version we decided to
> shutdown the guest when the heartbeat did not get a reply from the
> remote by setting the error_fatal.
> Should we approach it differently or you prefer us to get rid of the
> heartbeat in this form?
I think the only case that this patch handles is when the mpqemu channel
is closed.
The VM hangs when the channel is still open but the remote is
unresponsive. (mpqemu_msg_send_and_await_reply() calls aio_poll() with
the global mutex held so vcpus cannot make progress.)
The heartbeat mechanism needs to handle the case where the other side
isn't responding. It can't hang QEMU.
I suggest dropping this patch. It can be done later.
Stefan
signature.asc
Description: PGP signature
[PATCH] tests: docker: support mxe-based mingw builds
This can be run with docker-test-mingw@ubuntu1804-mxe, and is the setup
that Peter uses to test cross-compilation.
Signed-off-by: Paolo Bonzini
---
tests/docker/common.rc| 18 ++-
.../docker/dockerfiles/ubuntu1804-mxe.docker | 54 +++
tests/docker/test-mingw | 11 +++-
3 files changed, 80 insertions(+), 3 deletions(-)
create mode 100644 tests/docker/dockerfiles/ubuntu1804-mxe.docker
diff --git a/tests/docker/common.rc b/tests/docker/common.rc
index ebc5b97ecf..402f6603b6 100755
--- a/tests/docker/common.rc
+++ b/tests/docker/common.rc
@@ -15,10 +15,26 @@
# overriden by TARGET_LIST if the user sets it.
DEF_TARGET_LIST=${DEF_TARGET_LIST:-"x86_64-softmmu,aarch64-softmmu"}
+has()
+{
+echo "$FEATURES" | grep -wq -e "$1"
+}
+
+requires_any()
+{
+for c in $@; do
+if has "$c"; then
+return
+fi
+done
+echo "None of prerequisites '$*' is present, skip"
+exit 0
+}
+
requires()
{
for c in $@; do
-if ! echo "$FEATURES" | grep -wq -e "$c"; then
+if ! has "$c"; then
echo "Prerequisite '$c' not present, skip"
exit 0
fi
diff --git a/tests/docker/dockerfiles/ubuntu1804-mxe.docker
b/tests/docker/dockerfiles/ubuntu1804-mxe.docker
new file mode 100644
index 00..91895db80d
--- /dev/null
+++ b/tests/docker/dockerfiles/ubuntu1804-mxe.docker
@@ -0,0 +1,54 @@
+FROM ubuntu:18.04
+ENV PACKAGES \
+ccache \
+gcc \
+gettext \
+git \
+gnupg \
+gnupg2 \
+make \
+nsis \
+python3-yaml \
+python3-sphinx \
+python3-setuptools \
+texinfo
+RUN apt-get update && \
+DEBIAN_FRONTEND=noninteractive apt-get -y install $PACKAGES
+
+ENV MXE_PACKAGES \
+mxe-i686-w64-mingw32.shared-bzip2 \
+mxe-i686-w64-mingw32.shared-curl \
+mxe-i686-w64-mingw32.shared-glib \
+mxe-i686-w64-mingw32.shared-gcc \
+mxe-i686-w64-mingw32.shared-glib \
+mxe-i686-w64-mingw32.shared-gmp \
+mxe-i686-w64-mingw32.shared-gnutls \
+mxe-i686-w64-mingw32.shared-gtk3 \
+mxe-i686-w64-mingw32.shared-libjpeg-turbo \
+mxe-i686-w64-mingw32.shared-libpng \
+mxe-i686-w64-mingw32.shared-nettle \
+mxe-i686-w64-mingw32.shared-nsis \
+mxe-i686-w64-mingw32.shared-pixman \
+mxe-i686-w64-mingw32.shared-pkgconf \
+mxe-i686-w64-mingw32.shared-sdl2 \
+mxe-x86-64-w64-mingw32.shared-bzip2 \
+mxe-x86-64-w64-mingw32.shared-curl \
+mxe-x86-64-w64-mingw32.shared-gcc \
+mxe-x86-64-w64-mingw32.shared-glib \
+mxe-x86-64-w64-mingw32.shared-gmp \
+mxe-x86-64-w64-mingw32.shared-gnutls \
+mxe-x86-64-w64-mingw32.shared-gtk3 \
+mxe-x86-64-w64-mingw32.shared-libjpeg-turbo \
+mxe-x86-64-w64-mingw32.shared-libpng \
+mxe-x86-64-w64-mingw32.shared-nettle \
+mxe-x86-64-w64-mingw32.shared-nsis \
+mxe-x86-64-w64-mingw32.shared-pixman \
+mxe-x86-64-w64-mingw32.shared-pkgconf \
+mxe-x86-64-w64-mingw32.shared-sdl2
+
+RUN echo "deb http://pkg.mxe.cc/repos/apt bionic main" > \
+ /etc/apt/sources.list.d/mxeapt.list && \
+ apt-key adv --keyserver keyserver.ubuntu.com --recv-keys C6BF758A33A3A276
&& \
+ apt-get update && \
+ DEBIAN_FRONTEND=noninteractive apt-get install -y $MXE_PACKAGES
+ENV FEATURES mxe
diff --git a/tests/docker/test-mingw b/tests/docker/test-mingw
index c30eb654eb..9e2fadb11a 100755
--- a/tests/docker/test-mingw
+++ b/tests/docker/test-mingw
@@ -13,11 +13,18 @@
. common.rc
-requires mingw dtc
+requires dtc
+requires_any mingw mxe
cd "$BUILD_DIR"
-for prefix in x86_64-w64-mingw32- i686-w64-mingw32-; do
+if has mingw; then
+ prefixes='x86_64-w64-mingw32- i686-w64-mingw32-'
+else
+ prefixes='x86_64-w64-mingw32.shared- i686-w64-mingw32.shared-'
+ export PATH=/usr/lib/mxe/usr/bin:$PATH
+fi
+for prefix in $prefixes; do
TARGET_LIST=${TARGET_LIST:-$DEF_TARGET_LIST} \
build_qemu --cross-prefix=$prefix \
--enable-trace-backends=simple \
--
2.26.2
Re: [PULL 147/150] meson: convert po/
On 19/08/20 03:56, Brad Smith wrote: > > > This last part is redundant. If glib2 and/or gtk+3 is installed then > gettext > is installed. > > The package name is wrong as well as gettext changed from gettext to > gettext-runtime relatively recently. Are you sure gettext-runtime includes xgettext, and is installed if glib2 is? Paolo > >> diff --git a/tests/vm/freebsd b/tests/vm/freebsd >> index b34b14fc53..5f866e09c4 100755 >> --- a/tests/vm/freebsd >> +++ b/tests/vm/freebsd >> @@ -39,6 +39,7 @@ class FreeBSDVM(basevm.BaseVM): >> "bash", >> "gmake", >> "gsed", >> + "gettext", >> # libs: crypto >> "gnutls", >> diff --git a/tests/vm/netbsd b/tests/vm/netbsd >> index 93d48b6fdd..ffb65a89be 100755 >> --- a/tests/vm/netbsd >> +++ b/tests/vm/netbsd >> @@ -37,6 +37,7 @@ class NetBSDVM(basevm.BaseVM): >> "bash", >> "gmake", >> "gsed", >> + "gettext", >> # libs: crypto >> "gnutls", >> diff --git a/tests/vm/openbsd b/tests/vm/openbsd >> index 7e27fda642..8356646f21 100755 >> --- a/tests/vm/openbsd >> +++ b/tests/vm/openbsd >> @@ -36,6 +36,7 @@ class OpenBSDVM(basevm.BaseVM): >> "bash", >> "gmake", >> "gsed", >> + "gettext", >> # libs: usb >> "libusb1",
Re: [RFC PATCH v3 1/5] block/nvme: Use an array of EventNotifier
On Tue, Aug 18, 2020 at 06:45:05PM +0200, Philippe Mathieu-Daudé wrote: > In preparation of using multiple IRQ (thus multiple eventfds) > make BDRVNVMeState::irq_notifier an array (for now of a single > element, the admin queue notifier). > > Signed-off-by: Philippe Mathieu-Daudé > --- > block/nvme.c | 28 ++-- > 1 file changed, 18 insertions(+), 10 deletions(-) This looks like an intermediate step before using multiple irqs. I think it makes the code confusing because on one hand INDEX_ADMIN gives the impression that INDEX_IO() should be used for io queues, while on the other hand only a single EventNotifier is allocated and we actually can't use INDEX_IO() yet. If this intermediate patch is really necessary, please don't use INDEX_ADMIN. Define a new constant instead: /* This driver shares a single MSIX IRQ for the admin and I/O queues */ #define MSIX_SHARED_IRQ_IDX 0 In the future the array index can be changed to INDEX_ADMIN and INDEX_IO(n) when there are multiple EventNotifiers. I think that would make the code clearer. signature.asc Description: PGP signature
Re: [PATCH] tests: docker: support mxe-based mingw builds
Hi
On Wed, Aug 19, 2020 at 12:03 PM Paolo Bonzini wrote:
> This can be run with docker-test-mingw@ubuntu1804-mxe, and is the setup
> that Peter uses to test cross-compilation.
>
> Signed-off-by: Paolo Bonzini
>
I wish I had this a few times, thanks!
Reviewed-by: Marc-André Lureau
> ---
> tests/docker/common.rc| 18 ++-
> .../docker/dockerfiles/ubuntu1804-mxe.docker | 54 +++
> tests/docker/test-mingw | 11 +++-
> 3 files changed, 80 insertions(+), 3 deletions(-)
> create mode 100644 tests/docker/dockerfiles/ubuntu1804-mxe.docker
>
> diff --git a/tests/docker/common.rc b/tests/docker/common.rc
> index ebc5b97ecf..402f6603b6 100755
> --- a/tests/docker/common.rc
> +++ b/tests/docker/common.rc
> @@ -15,10 +15,26 @@
> # overriden by TARGET_LIST if the user sets it.
> DEF_TARGET_LIST=${DEF_TARGET_LIST:-"x86_64-softmmu,aarch64-softmmu"}
>
> +has()
> +{
> +echo "$FEATURES" | grep -wq -e "$1"
> +}
> +
> +requires_any()
> +{
> +for c in $@; do
> +if has "$c"; then
> +return
> +fi
> +done
> +echo "None of prerequisites '$*' is present, skip"
> +exit 0
> +}
> +
> requires()
> {
> for c in $@; do
> -if ! echo "$FEATURES" | grep -wq -e "$c"; then
> +if ! has "$c"; then
> echo "Prerequisite '$c' not present, skip"
> exit 0
> fi
> diff --git a/tests/docker/dockerfiles/ubuntu1804-mxe.docker
> b/tests/docker/dockerfiles/ubuntu1804-mxe.docker
> new file mode 100644
> index 00..91895db80d
> --- /dev/null
> +++ b/tests/docker/dockerfiles/ubuntu1804-mxe.docker
> @@ -0,0 +1,54 @@
> +FROM ubuntu:18.04
> +ENV PACKAGES \
> +ccache \
> +gcc \
> +gettext \
> +git \
> +gnupg \
> +gnupg2 \
> +make \
> +nsis \
> +python3-yaml \
> +python3-sphinx \
> +python3-setuptools \
> +texinfo
> +RUN apt-get update && \
> +DEBIAN_FRONTEND=noninteractive apt-get -y install $PACKAGES
> +
> +ENV MXE_PACKAGES \
> +mxe-i686-w64-mingw32.shared-bzip2 \
> +mxe-i686-w64-mingw32.shared-curl \
> +mxe-i686-w64-mingw32.shared-glib \
> +mxe-i686-w64-mingw32.shared-gcc \
> +mxe-i686-w64-mingw32.shared-glib \
> +mxe-i686-w64-mingw32.shared-gmp \
> +mxe-i686-w64-mingw32.shared-gnutls \
> +mxe-i686-w64-mingw32.shared-gtk3 \
> +mxe-i686-w64-mingw32.shared-libjpeg-turbo \
> +mxe-i686-w64-mingw32.shared-libpng \
> +mxe-i686-w64-mingw32.shared-nettle \
> +mxe-i686-w64-mingw32.shared-nsis \
> +mxe-i686-w64-mingw32.shared-pixman \
> +mxe-i686-w64-mingw32.shared-pkgconf \
> +mxe-i686-w64-mingw32.shared-sdl2 \
> +mxe-x86-64-w64-mingw32.shared-bzip2 \
> +mxe-x86-64-w64-mingw32.shared-curl \
> +mxe-x86-64-w64-mingw32.shared-gcc \
> +mxe-x86-64-w64-mingw32.shared-glib \
> +mxe-x86-64-w64-mingw32.shared-gmp \
> +mxe-x86-64-w64-mingw32.shared-gnutls \
> +mxe-x86-64-w64-mingw32.shared-gtk3 \
> +mxe-x86-64-w64-mingw32.shared-libjpeg-turbo \
> +mxe-x86-64-w64-mingw32.shared-libpng \
> +mxe-x86-64-w64-mingw32.shared-nettle \
> +mxe-x86-64-w64-mingw32.shared-nsis \
> +mxe-x86-64-w64-mingw32.shared-pixman \
> +mxe-x86-64-w64-mingw32.shared-pkgconf \
> +mxe-x86-64-w64-mingw32.shared-sdl2
> +
> +RUN echo "deb http://pkg.mxe.cc/repos/apt bionic main" > \
> + /etc/apt/sources.list.d/mxeapt.list && \
> + apt-key adv --keyserver keyserver.ubuntu.com --recv-keys
> C6BF758A33A3A276 && \
> + apt-get update && \
> + DEBIAN_FRONTEND=noninteractive apt-get install -y $MXE_PACKAGES
> +ENV FEATURES mxe
> diff --git a/tests/docker/test-mingw b/tests/docker/test-mingw
> index c30eb654eb..9e2fadb11a 100755
> --- a/tests/docker/test-mingw
> +++ b/tests/docker/test-mingw
> @@ -13,11 +13,18 @@
>
> . common.rc
>
> -requires mingw dtc
> +requires dtc
> +requires_any mingw mxe
>
> cd "$BUILD_DIR"
>
> -for prefix in x86_64-w64-mingw32- i686-w64-mingw32-; do
> +if has mingw; then
> + prefixes='x86_64-w64-mingw32- i686-w64-mingw32-'
> +else
> + prefixes='x86_64-w64-mingw32.shared- i686-w64-mingw32.shared-'
> + export PATH=/usr/lib/mxe/usr/bin:$PATH
> +fi
> +for prefix in $prefixes; do
> TARGET_LIST=${TARGET_LIST:-$DEF_TARGET_LIST} \
> build_qemu --cross-prefix=$prefix \
> --enable-trace-backends=simple \
> --
> 2.26.2
>
>
>
--
Marc-André Lureau
[Bug 1253563] Re: bad performance with rng-egd backend
Let's close this bug, it's passed 6 years. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to the bug report. https://bugs.launchpad.net/bugs/1253563 Title: bad performance with rng-egd backend Status in QEMU: Incomplete Bug description: 1. create listen socket # cat /dev/random | nc -l localhost 1024 2. start vm with rng-egd backend ./x86_64-softmmu/qemu-system-x86_64 --enable-kvm -mon chardev=qmp,mode=control,pretty=on -chardev socket,id=qmp,host=localhost,port=1234,server,nowait -m 2000 -device virtio-net-pci,netdev=h1,id=vnet0 -netdev tap,id=h1 -vnc :0 -drive file=/images/RHEL-64-virtio.qcow2 \ -chardev socket,host=localhost,port=1024,id=chr0 \ -object rng-egd,chardev=chr0,id=rng0 \ -device virtio-rng-pci,rng=rng0,max-bytes=1024000,period=1000 (guest) # dd if=/dev/hwrng of=/dev/null note: cancelling dd process by Ctrl+c, it will return the read speed. Problem: the speed is around 1k/s === If I use rng-random backend (filename=/dev/random), the speed is about 350k/s). It seems that when the request entry is added to the list, we don't read the data from queue list immediately. The chr_read() is delayed, the virtio_notify() is delayed. the next request will also be delayed. It effects the speed. I tried to change rng_egd_chr_can_read() always returns 1, the speed is improved to (about 400k/s) Problem: we can't poll the content in time currently Any thoughts? Thanks, Amos To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1253563/+subscriptions
Re: [PATCH] tests: docker: support mxe-based mingw builds
On Wed, Aug 19, 2020 at 04:02:06AM -0400, Paolo Bonzini wrote:
> This can be run with docker-test-mingw@ubuntu1804-mxe, and is the setup
> that Peter uses to test cross-compilation.
We already have docker containers with MXE based on Debian:
debian-win32-cross.docker
debian-win64-cross.docker
your image uses a different naming convention, and puts both
32 and 64 bit in the same image.
I feel like we should have the Ubuntu variant follow the same
structure and naming as the Debian variant for consistency.
>
> Signed-off-by: Paolo Bonzini
> ---
> tests/docker/common.rc| 18 ++-
> .../docker/dockerfiles/ubuntu1804-mxe.docker | 54 +++
> tests/docker/test-mingw | 11 +++-
> 3 files changed, 80 insertions(+), 3 deletions(-)
> create mode 100644 tests/docker/dockerfiles/ubuntu1804-mxe.docker
>
> diff --git a/tests/docker/common.rc b/tests/docker/common.rc
> index ebc5b97ecf..402f6603b6 100755
> --- a/tests/docker/common.rc
> +++ b/tests/docker/common.rc
> @@ -15,10 +15,26 @@
> # overriden by TARGET_LIST if the user sets it.
> DEF_TARGET_LIST=${DEF_TARGET_LIST:-"x86_64-softmmu,aarch64-softmmu"}
>
> +has()
> +{
> +echo "$FEATURES" | grep -wq -e "$1"
> +}
> +
> +requires_any()
> +{
> +for c in $@; do
> +if has "$c"; then
> +return
> +fi
> +done
> +echo "None of prerequisites '$*' is present, skip"
> +exit 0
> +}
> +
> requires()
> {
> for c in $@; do
> -if ! echo "$FEATURES" | grep -wq -e "$c"; then
> +if ! has "$c"; then
> echo "Prerequisite '$c' not present, skip"
> exit 0
> fi
> diff --git a/tests/docker/dockerfiles/ubuntu1804-mxe.docker
> b/tests/docker/dockerfiles/ubuntu1804-mxe.docker
> new file mode 100644
> index 00..91895db80d
> --- /dev/null
> +++ b/tests/docker/dockerfiles/ubuntu1804-mxe.docker
> @@ -0,0 +1,54 @@
> +FROM ubuntu:18.04
> +ENV PACKAGES \
> +ccache \
> +gcc \
> +gettext \
> +git \
> +gnupg \
> +gnupg2 \
> +make \
> +nsis \
> +python3-yaml \
> +python3-sphinx \
> +python3-setuptools \
> +texinfo
> +RUN apt-get update && \
> +DEBIAN_FRONTEND=noninteractive apt-get -y install $PACKAGES
> +
> +ENV MXE_PACKAGES \
> +mxe-i686-w64-mingw32.shared-bzip2 \
> +mxe-i686-w64-mingw32.shared-curl \
> +mxe-i686-w64-mingw32.shared-glib \
> +mxe-i686-w64-mingw32.shared-gcc \
> +mxe-i686-w64-mingw32.shared-glib \
> +mxe-i686-w64-mingw32.shared-gmp \
> +mxe-i686-w64-mingw32.shared-gnutls \
> +mxe-i686-w64-mingw32.shared-gtk3 \
> +mxe-i686-w64-mingw32.shared-libjpeg-turbo \
> +mxe-i686-w64-mingw32.shared-libpng \
> +mxe-i686-w64-mingw32.shared-nettle \
> +mxe-i686-w64-mingw32.shared-nsis \
> +mxe-i686-w64-mingw32.shared-pixman \
> +mxe-i686-w64-mingw32.shared-pkgconf \
> +mxe-i686-w64-mingw32.shared-sdl2 \
> +mxe-x86-64-w64-mingw32.shared-bzip2 \
> +mxe-x86-64-w64-mingw32.shared-curl \
> +mxe-x86-64-w64-mingw32.shared-gcc \
> +mxe-x86-64-w64-mingw32.shared-glib \
> +mxe-x86-64-w64-mingw32.shared-gmp \
> +mxe-x86-64-w64-mingw32.shared-gnutls \
> +mxe-x86-64-w64-mingw32.shared-gtk3 \
> +mxe-x86-64-w64-mingw32.shared-libjpeg-turbo \
> +mxe-x86-64-w64-mingw32.shared-libpng \
> +mxe-x86-64-w64-mingw32.shared-nettle \
> +mxe-x86-64-w64-mingw32.shared-nsis \
> +mxe-x86-64-w64-mingw32.shared-pixman \
> +mxe-x86-64-w64-mingw32.shared-pkgconf \
> +mxe-x86-64-w64-mingw32.shared-sdl2
> +
> +RUN echo "deb http://pkg.mxe.cc/repos/apt bionic main" > \
> + /etc/apt/sources.list.d/mxeapt.list && \
> + apt-key adv --keyserver keyserver.ubuntu.com --recv-keys C6BF758A33A3A276
> && \
> + apt-get update && \
> + DEBIAN_FRONTEND=noninteractive apt-get install -y $MXE_PACKAGES
> +ENV FEATURES mxe
> diff --git a/tests/docker/test-mingw b/tests/docker/test-mingw
> index c30eb654eb..9e2fadb11a 100755
> --- a/tests/docker/test-mingw
> +++ b/tests/docker/test-mingw
> @@ -13,11 +13,18 @@
>
> . common.rc
>
> -requires mingw dtc
> +requires dtc
> +requires_any mingw mxe
>
> cd "$BUILD_DIR"
>
> -for prefix in x86_64-w64-mingw32- i686-w64-mingw32-; do
> +if has mingw; then
> + prefixes='x86_64-w64-mingw32- i686-w64-mingw32-'
> +else
> + prefixes='x86_64-w64-mingw32.shared- i686-w64-mingw32.shared-'
> + export PATH=/usr/lib/mxe/usr/bin:$PATH
> +fi
> +for prefix in $prefixes; do
> TARGET_LIST=${TARGET_LIST:-$DEF_TARGET_LIST} \
> build_qemu --cross-prefix=$prefix \
> --enable-trace-backends=simple \
> --
> 2.26.2
>
>
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: [RFC v3 1/1] memory: Skip bad range assertion if notifier supports arbitrary masks
On Wed, Aug 19, 2020 at 9:15 AM Jason Wang wrote:
>
>
> On 2020/8/18 下午10:24, Eugenio Perez Martin wrote:
> > On Wed, Aug 12, 2020 at 10:49 AM Eugenio Perez Martin
> > wrote:
> >> On Wed, Aug 12, 2020 at 4:24 AM Jason Wang wrote:
> >>> On 2020/8/12 上午1:55, Eugenio Pérez wrote:
> Signed-off-by: Eugenio Pérez
> ---
> hw/virtio/vhost.c | 2 +-
> include/exec/memory.h | 2 ++
> softmmu/memory.c | 10 --
> 3 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 1a1384e7a6..e74ad9e09b 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -729,7 +729,7 @@ static void vhost_iommu_region_add(MemoryListener
> *listener,
> iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr,
>
> MEMTXATTRS_UNSPECIFIED);
> iommu_notifier_init(&iommu->n, vhost_iommu_unmap_notify,
> -IOMMU_NOTIFIER_UNMAP,
> +IOMMU_NOTIFIER_UNMAP | IOMMU_NOTIFIER_IOTLB,
> >>> I think we can safely drop IOMMU_NOTIFIER_UNMAP here since device IOTLB
> >>> is sufficient.
> >>>
> >>> Btw, IOMMU_NOTIFIER_IOTLB is kind of confusing, maybe something like
> >>> IOMMU_NOTIFIER_DEVIOTLB.
> >>>
> >> Got it, will change.
> >>
> section->offset_within_region,
> int128_get64(end),
> iommu_idx);
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 307e527835..4d94c1e984 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -87,6 +87,8 @@ typedef enum {
> IOMMU_NOTIFIER_UNMAP = 0x1,
> /* Notify entry changes (newly created entries) */
> IOMMU_NOTIFIER_MAP = 0x2,
> +/* Notify changes on IOTLB entries */
> +IOMMU_NOTIFIER_IOTLB = 0x04,
> } IOMMUNotifierFlag;
>
> #define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_MAP | IOMMU_NOTIFIER_UNMAP)
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index af25987518..e2c5f6d0e7 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1895,6 +1895,7 @@ void memory_region_notify_one(IOMMUNotifier
> *notifier,
> >>> (we probably need a better name of this function, at least something
> >>> like "memory_region_iommu_notify_one").
> >>>
> >> Ok will change.
> >>
> {
> IOMMUNotifierFlag request_flags;
> hwaddr entry_end = entry->iova + entry->addr_mask;
> +IOMMUTLBEntry tmp = *entry;
>
> /*
> * Skip the notification if the notification does not overlap
> @@ -1904,7 +1905,12 @@ void memory_region_notify_one(IOMMUNotifier
> *notifier,
> return;
> }
>
> -assert(entry->iova >= notifier->start && entry_end <=
> notifier->end);
> +if (notifier->notifier_flags & IOMMU_NOTIFIER_IOTLB) {
> +tmp.iova = MAX(tmp.iova, notifier->start);
> +tmp.addr_mask = MIN(entry_end, notifier->end) - tmp.iova;
> >>> Any reason for doing such re-calculation here, a comment would be helpful.
> >>>
> >> It was proposed by Peter, but I understand as limiting the
> >> address+range we pass to the notifier. Although vhost seems to support
> >> it as long as it contains (notifier->start, notifier->end) in range, a
> >> future notifier might not.
>
>
> Yes, actually, I feel confused after reading the codes. Is
> notifier->start IOVA or GPA?
>
> In vfio.c, we did:
>
> iommu_notifier_init(&giommu->n, vfio_iommu_map_notify,
> IOMMU_NOTIFIER_ALL,
> section->offset_within_region,
> int128_get64(llend),
> iommu_idx);
>
> So it looks to me the start and end are GPA, but the assertion above
> check it against IOVA which seems to be wrong
>
> Thanks
>
I see.
I didn't go so deep, I just assumed that:
* all the addresses were GPA in the vhost-net+virtio-net case,
although the name iova in IOMMUTLBEntry.
* memory region was initialized with IOVA addresses in case of VFIO.
Maybe the comment should warn about the bad "iova" name, if I'm right?
I assumed that nothing changed in the VFIO case since its notifier has
no IOMMU_NOTIFIER_DEVIOTLB flag and the new conditional in
memory_region_notify_one_iommu, but I will test with a device
passthrough and DPDK again. Do you think another test would be needed?
Maybe Peter can go deeper on this.
Thanks!
>
> >>
> >> It could be done as iommu_entry_crop(IOMMUTLBEntry *entry, const
> >> IOMMUNotifier *notifier) though.
>
[PATCH V3] Introduce a new flag for i440fx to disable PCI hotplug on the root bus
We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which
we can turn on or off PCI device hotplug on the root bus. This flag can be
used to prevent all PCI devices from getting hotplugged or unplugged from the
root PCI bus.
The existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI bridges
remain as is and can be used along with this new flag to control PCI hotplug
on PCI bridges.
This change has been tested using a Windows 2012R2 server image on a Ubuntu
18.04 host using the latest master qemu from upstream.
Signed-off-by: Ani Sinha
---
hw/acpi/piix4.c | 8 ++--
hw/i386/acpi-build.c | 26 +++---
2 files changed, 25 insertions(+), 9 deletions(-)
Change Log:
V2..V3: removed some AMLs which are not needed when hotplug on the root
bus is disabled. Also prevented calling device initialization routine
related to hotplug when the hotplug is disabled for both pci root bus
and pci bridges.
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 26bac4f16c..4f436e5bf3 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -78,6 +78,7 @@ typedef struct PIIX4PMState {
AcpiPciHpState acpi_pci_hotplug;
bool use_acpi_hotplug_bridge;
+bool use_acpi_root_pci_hotplug;
uint8_t disable_s3;
uint8_t disable_s4;
@@ -595,8 +596,9 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion
*parent,
"acpi-gpe0", GPE_LEN);
memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
-acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
-s->use_acpi_hotplug_bridge);
+if (s->use_acpi_hotplug_bridge || s->use_acpi_root_pci_hotplug)
+acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
+s->use_acpi_hotplug_bridge);
s->cpu_hotplug_legacy = true;
object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
@@ -635,6 +637,8 @@ static Property piix4_pm_properties[] = {
DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState,
use_acpi_hotplug_bridge, true),
+DEFINE_PROP_BOOL("acpi-root-pci-hotplug", PIIX4PMState,
+ use_acpi_root_pci_hotplug, true),
DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
acpi_memory_hotplug.is_enabled, true),
DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b7bc2a..19a1702ad1 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -95,6 +95,7 @@ typedef struct AcpiPmInfo {
bool s3_disabled;
bool s4_disabled;
bool pcihp_bridge_en;
+bool pcihp_root_en;
uint8_t s4_val;
AcpiFadtData fadt;
uint16_t cpu_hp_io_base;
@@ -245,6 +246,9 @@ static void acpi_get_pm_info(MachineState *machine,
AcpiPmInfo *pm)
pm->pcihp_bridge_en =
object_property_get_bool(obj, "acpi-pci-hotplug-with-bridge-support",
NULL);
+pm->pcihp_root_en =
+object_property_get_bool(obj, "acpi-root-pci-hotplug", NULL);
+
}
static void acpi_get_misc_info(AcpiMiscInfo *info)
@@ -337,15 +341,18 @@ static void build_append_pcihp_notify_entry(Aml *method,
int slot)
}
static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
- bool pcihp_bridge_en)
+ bool pcihp_bridge_en,
+ bool pcihp_root_en)
{
Aml *dev, *notify_method = NULL, *method;
QObject *bsel;
PCIBus *sec;
int i;
+bool root_bus = pci_bus_is_root(bus);
+bool root_pcihp_disabled = (root_bus && !pcihp_root_en);
bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
NULL);
-if (bsel) {
+if (bsel && !root_pcihp_disabled) {
uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel));
aml_append(parent_scope, aml_name_decl("BSEL", aml_int(bsel_val)));
@@ -361,6 +368,9 @@ static void build_append_pci_bus_devices(Aml *parent_scope,
PCIBus *bus,
bool bridge_in_acpi;
if (!pdev) {
+/* skip if pci hotplug for the root bus is disabled */
+if (root_pcihp_disabled)
+continue;
if (bsel) { /* add hotplug slots for non present devices */
dev = aml_device("S%.02X", PCI_DEVFN(slot, 0));
aml_append(dev, aml_name_decl("_SUN", aml_int(slot)));
@@ -419,7 +429,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope,
PCIBus *bus,
method = aml_method("_S3D", 0, AML_NOTSERIALIZED);
aml_append(method, aml_return(aml_int(s3d)));
aml_append(dev, method);
-} else if (hotplug_enabled_dev) {
+} else if (hotplug_enabled_dev && !root_pcihp_disabled) {
/* add _SUN/_EJ0 to make slot hotpluggable */
aml_append
Re: [ovirt-devel] Re: device compatibility interface for live migration with assigned devices
On Wed, Aug 19, 2020 at 03:39:50PM +0800, Jason Wang wrote: > > On 2020/8/19 下午2:59, Yan Zhao wrote: > > On Wed, Aug 19, 2020 at 02:57:34PM +0800, Jason Wang wrote: > > > On 2020/8/19 上午11:30, Yan Zhao wrote: > > > > hi All, > > > > could we decide that sysfs is the interface that every VFIO vendor > > > > driver > > > > needs to provide in order to support vfio live migration, otherwise the > > > > userspace management tool would not list the device into the compatible > > > > list? > > > > > > > > if that's true, let's move to the standardizing of the sysfs interface. > > > > (1) content > > > > common part: (must) > > > > - software_version: (in major.minor.bugfix scheme) > > > > > > This can not work for devices whose features can be negotiated/advertised > > > independently. (E.g virtio devices) > > > > > sorry, I don't understand here, why virtio devices need to use vfio > > interface? > > > I don't see any reason that virtio devices can't be used by VFIO. Do you? > > Actually, virtio devices have been used by VFIO for many years: > > - passthrough a hardware virtio devices to userspace(VM) drivers > - using virtio PMD inside guest > So, what's different for it vs passing through a physical hardware via VFIO? even though the features are negotiated dynamically, could you explain why it would cause software_version not work? > > > I think this thread is discussing about vfio related devices. > > > > > > - device_api: vfio-pci or vfio-ccw ... > > > > - type: mdev type for mdev device or > > > > a signature for physical device which is a counterpart for > > > >mdev type. > > > > > > > > device api specific part: (must) > > > > - pci id: pci id of mdev parent device or pci id of physical pci > > > > device (device_api is vfio-pci)API here. > > > > > > So this assumes a PCI device which is probably not true. > > > > > for device_api of vfio-pci, why it's not true? > > > > for vfio-ccw, it's subchannel_type. > > > Ok but having two different attributes for the same file is not good idea. > How mgmt know there will be a 3rd type? that's why some attributes need to be common. e.g. device_api: it's common because mgmt need to know it's a pci device or a ccw device. and the api type is already defined vfio.h. (The field is agreed by and actually suggested by Alex in previous mail) type: mdev_type for mdev. if mgmt does not understand it, it would not be able to create one compatible mdev device. software_version: mgmt can compare the major and minor if it understands this fields. > > > > > > > > - subchannel_type (device_api is vfio-ccw) > > > > vendor driver specific part: (optional) > > > > - aggregator > > > > - chpid_type > > > > - remote_url > > > > > > For "remote_url", just wonder if it's better to integrate or reuse the > > > existing NVME management interface instead of duplicating it here. > > > Otherwise > > > it could be a burden for mgmt to learn. E.g vendor A may use "remote_url" > > > but vendor B may use a different attribute. > > > > > it's vendor driver specific. > > vendor specific attributes are inevitable, and that's why we are > > discussing here of a way to standardizing of it. > > > Well, then you will end up with a very long list to discuss. E.g for > networking devices, you will have "mac", "v(x)lan" and a lot of other. > > Note that "remote_url" is not vendor specific but NVME (class/subsystem) > specific. > yes, it's just NVMe specific. I added it as an example to show what is vendor specific. if one attribute is vendor specific across all vendors, then it's not vendor specific, it's already common attribute, right? > The point is that if vendor/class specific part is unavoidable, why not > making all of the attributes vendor specific? > some parts need to be common, as I listed above. > > > our goal is that mgmt can use it without understanding the meaning of vendor > > specific attributes. > > > I'm not sure this is the correct design of uAPI. Is there something similar > in the existing uAPIs? > > And it might be hard to work for virtio devices. > > > > > > > > NOTE: vendors are free to add attributes in this part with a > > > > restriction that this attribute is able to be configured with the same > > > > name in sysfs too. e.g. > > > > > > Sysfs works well for common attributes belongs to a class, but I'm not > > > sure > > > it can work well for device/vendor specific attributes. Does this mean > > > mgmt > > > need to iterate all the attributes in both src and dst? > > > > > no. just attributes under migration directory. > > > > > > for aggregator, there must be a sysfs attribute in device node > > > > /sys/devices/pci:00/:00:02.0/882cc4da-dede-11e7-9180-078a62063ab1/intel_vgpu/aggregator, > > > > so that the userspace tool is able to configure the target device > > > > according to source device's aggregator attribute. >
Re: [RFC PATCH 19/22] block/export: Move strong user reference to block_exports
On 13.08.20 18:29, Kevin Wolf wrote: > The reference owned by the user/monitor that is created when adding the > export and dropped when removing it was tied to the 'exports' list in > nbd/server.c. Every block export will have a user reference, so move it > to the block export level and tie it to the 'block_exports' list in > block/export/export.c instead. This is necessary for introducing a QMP > command for removing exports. > > Note that exports are present in block_exports even after the user has > requested shutdown. This is different from NBD's exports where exports > are immediately removed on a shutdown request, even if they are still in > the process of shutting down. In order to avoid that the user still > interacts with an export that is shutting down (and possibly removes it > a second time), we need to remember if the user actually still owns it. > > Signed-off-by: Kevin Wolf > --- > include/block/export.h | 8 > block/export/export.c | 4 > blockdev-nbd.c | 5 - > nbd/server.c | 2 -- > 4 files changed, 12 insertions(+), 7 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 1/7] x86: lpc9: let firmware negotiate 'CPU hotplug with SMI' features
Hi Igor,
(CC'ing Daniel, Cornelia, David, Peter)
On 08/18/20 14:22, Igor Mammedov wrote:
> It will allow firmware to notify QEMU that firmware requires SMI
> being triggered on CPU hot[un]plug, so that it would be able to account
> for hotplugged CPU and relocate it to new SMM base and/or safely remove
> CPU on unplug.
>
> Using negotiated features, follow up patches will insert SMI upcall
> into AML code, to make sure that firmware processes hotplug before
> guest OS would attempt to use new CPU.
>
> Signed-off-by: Igor Mammedov
> Reviewed-by: Laszlo Ersek
> ---
> v2:
> - rebase on top of 5.1 (move compat values to 5.1 machine)
> - make "x-smi-cpu-hotunplug" false by default (Laszlo Ersek
> )
> ---
> include/hw/i386/ich9.h | 2 ++
> include/hw/i386/pc.h | 3 +++
> hw/i386/pc.c | 6 +-
> hw/i386/pc_piix.c | 1 +
> hw/i386/pc_q35.c | 1 +
> hw/isa/lpc_ich9.c | 13 +
> 6 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> index a98d10b252..d1bb3f7bf0 100644
> --- a/include/hw/i386/ich9.h
> +++ b/include/hw/i386/ich9.h
> @@ -247,5 +247,7 @@ typedef struct ICH9LPCState {
>
> /* bit positions used in fw_cfg SMI feature negotiation */
> #define ICH9_LPC_SMI_F_BROADCAST_BIT0
> +#define ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT 1
> +#define ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT 2
>
> #endif /* HW_ICH9_H */
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 3d7ed3a55e..fe52e165b2 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -193,6 +193,9 @@ void pc_system_firmware_init(PCMachineState *pcms,
> MemoryRegion *rom_memory);
> void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> const CPUArchIdList *apic_ids, GArray *entry);
>
> +extern GlobalProperty pc_compat_5_1[];
> +extern const size_t pc_compat_5_1_len;
> +
> extern GlobalProperty pc_compat_5_0[];
> extern const size_t pc_compat_5_0_len;
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 47c5ca3e34..99c6bdbab4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -97,8 +97,12 @@
> #include "fw_cfg.h"
> #include "trace.h"
>
> -GlobalProperty pc_compat_5_0[] = {
> +GlobalProperty pc_compat_5_1[] = {
> +{ "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
> };
> +const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
> +
> +GlobalProperty pc_compat_5_0[] = { };
> const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
>
> GlobalProperty pc_compat_4_2[] = {
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index b789e83f9a..d56f2e1b96 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -433,6 +433,7 @@ static void pc_i440fx_5_1_machine_options(MachineClass *m)
> m->alias = "pc";
> m->is_default = true;
> pcmc->default_cpu_version = 1;
> +compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
> }
>
> DEFINE_I440FX_MACHINE(v5_1, "pc-i440fx-5.1", NULL,
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index a3e607a544..0ca1146a59 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -359,6 +359,7 @@ static void pc_q35_5_1_machine_options(MachineClass *m)
> pc_q35_machine_options(m);
> m->alias = "q35";
> pcmc->default_cpu_version = 1;
> +compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
> }
>
> DEFINE_Q35_MACHINE(v5_1, "pc-q35-5.1", NULL,
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index cd6e169d47..19f32bed3e 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -373,6 +373,15 @@ static void smi_features_ok_callback(void *opaque)
> /* guest requests invalid features, leave @features_ok at zero */
> return;
> }
> +if (!(guest_features & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT)) &&
> +guest_features & (BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT) |
> + BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT))) {
> +/*
> + * cpu hot-[un]plug with SMI requires SMI broadcast,
> + * leave @features_ok at zero
> + */
> +return;
> +}
>
> /* valid feature subset requested, lock it down, report success */
> lpc->smi_negotiated_features = guest_features;
> @@ -747,6 +756,10 @@ static Property ich9_lpc_properties[] = {
> DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true),
> DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
>ICH9_LPC_SMI_F_BROADCAST_BIT, true),
> +DEFINE_PROP_BIT64("x-smi-cpu-hotplug", ICH9LPCState, smi_host_features,
> + ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT, true),
> +DEFINE_PROP_BIT64("x-smi-cpu-hotunplug", ICH9LPCState, smi_host_features,
> + ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT, false),
> DEFINE_PROP_END_OF_LIST(),
> };
>
>
this patch does the right thing for the 5.1 PC machine types, but it
does not introduce any 5.2
Re: [PATCH v2] qemu-img: Explicit number replaced by a constant
On Wed 19 Aug 2020 03:36:07 AM CEST, Yi Li wrote: > Signed-off-by: Yi Li Reviewed-by: Alberto Garcia Berto
Re: [PATCH v2 1/7] x86: lpc9: let firmware negotiate 'CPU hotplug with SMI' features
correcting myself a bit, with Drew and Thomas CC'd as well:
On 08/19/20 10:39, Laszlo Ersek wrote:
> Hi Igor,
>
> (CC'ing Daniel, Cornelia, David, Peter)
>
> On 08/18/20 14:22, Igor Mammedov wrote:
>> It will allow firmware to notify QEMU that firmware requires SMI
>> being triggered on CPU hot[un]plug, so that it would be able to account
>> for hotplugged CPU and relocate it to new SMM base and/or safely remove
>> CPU on unplug.
>>
>> Using negotiated features, follow up patches will insert SMI upcall
>> into AML code, to make sure that firmware processes hotplug before
>> guest OS would attempt to use new CPU.
>>
>> Signed-off-by: Igor Mammedov
>> Reviewed-by: Laszlo Ersek
>> ---
>> v2:
>> - rebase on top of 5.1 (move compat values to 5.1 machine)
>> - make "x-smi-cpu-hotunplug" false by default (Laszlo Ersek
>> )
>> ---
>> include/hw/i386/ich9.h | 2 ++
>> include/hw/i386/pc.h | 3 +++
>> hw/i386/pc.c | 6 +-
>> hw/i386/pc_piix.c | 1 +
>> hw/i386/pc_q35.c | 1 +
>> hw/isa/lpc_ich9.c | 13 +
>> 6 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
>> index a98d10b252..d1bb3f7bf0 100644
>> --- a/include/hw/i386/ich9.h
>> +++ b/include/hw/i386/ich9.h
>> @@ -247,5 +247,7 @@ typedef struct ICH9LPCState {
>>
>> /* bit positions used in fw_cfg SMI feature negotiation */
>> #define ICH9_LPC_SMI_F_BROADCAST_BIT0
>> +#define ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT 1
>> +#define ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT 2
>>
>> #endif /* HW_ICH9_H */
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 3d7ed3a55e..fe52e165b2 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -193,6 +193,9 @@ void pc_system_firmware_init(PCMachineState *pcms,
>> MemoryRegion *rom_memory);
>> void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
>> const CPUArchIdList *apic_ids, GArray *entry);
>>
>> +extern GlobalProperty pc_compat_5_1[];
>> +extern const size_t pc_compat_5_1_len;
>> +
>> extern GlobalProperty pc_compat_5_0[];
>> extern const size_t pc_compat_5_0_len;
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 47c5ca3e34..99c6bdbab4 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -97,8 +97,12 @@
>> #include "fw_cfg.h"
>> #include "trace.h"
>>
>> -GlobalProperty pc_compat_5_0[] = {
>> +GlobalProperty pc_compat_5_1[] = {
>> +{ "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
>> };
>> +const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
>> +
>> +GlobalProperty pc_compat_5_0[] = { };
>> const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
>>
>> GlobalProperty pc_compat_4_2[] = {
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index b789e83f9a..d56f2e1b96 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -433,6 +433,7 @@ static void pc_i440fx_5_1_machine_options(MachineClass
>> *m)
>> m->alias = "pc";
>> m->is_default = true;
>> pcmc->default_cpu_version = 1;
>> +compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
>> }
>>
>> DEFINE_I440FX_MACHINE(v5_1, "pc-i440fx-5.1", NULL,
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index a3e607a544..0ca1146a59 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -359,6 +359,7 @@ static void pc_q35_5_1_machine_options(MachineClass *m)
>> pc_q35_machine_options(m);
>> m->alias = "q35";
>> pcmc->default_cpu_version = 1;
>> +compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
>> }
>>
>> DEFINE_Q35_MACHINE(v5_1, "pc-q35-5.1", NULL,
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index cd6e169d47..19f32bed3e 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -373,6 +373,15 @@ static void smi_features_ok_callback(void *opaque)
>> /* guest requests invalid features, leave @features_ok at zero */
>> return;
>> }
>> +if (!(guest_features & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT)) &&
>> +guest_features & (BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT) |
>> + BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT))) {
>> +/*
>> + * cpu hot-[un]plug with SMI requires SMI broadcast,
>> + * leave @features_ok at zero
>> + */
>> +return;
>> +}
>>
>> /* valid feature subset requested, lock it down, report success */
>> lpc->smi_negotiated_features = guest_features;
>> @@ -747,6 +756,10 @@ static Property ich9_lpc_properties[] = {
>> DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true),
>> DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
>>ICH9_LPC_SMI_F_BROADCAST_BIT, true),
>> +DEFINE_PROP_BIT64("x-smi-cpu-hotplug", ICH9LPCState, smi_host_features,
>> + ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT, true),
>> +DEFINE_PROP_BIT64("x-smi-cpu-hotunplug", ICH9LP
Re: [PATCH v2] qemu-img: Explicit number replaced by a constant
On Wed, Aug 19, 2020 at 09:36:07AM +0800, Yi Li wrote:
> Signed-off-by: Yi Li
> ---
> qemu-img.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 5308773811..aa2e31c8ae 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1200,10 +1200,10 @@ static int is_allocated_sectors(const uint8_t *buf,
> int n, int *pnum,
> *pnum = 0;
> return 0;
> }
> -is_zero = buffer_is_zero(buf, 512);
> +is_zero = buffer_is_zero(buf, BDRV_SECTOR_SIZE);
> for(i = 1; i < n; i++) {
> -buf += 512;
> -if (is_zero != buffer_is_zero(buf, 512)) {
> +buf += BDRV_SECTOR_SIZE;
> +if (is_zero != buffer_is_zero(buf, BDRV_SECTOR_SIZE)) {
> break;
> }
> }
> @@ -2489,8 +2489,8 @@ static int img_convert(int argc, char **argv)
> }
> }
>
> -qemu_opt_set_number(opts, BLOCK_OPT_SIZE, s.total_sectors * 512,
> -&error_abort);
> +qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
> +s.total_sectors * BDRV_SECTOR_SIZE,
> &error_abort);
> ret = add_old_style_options(out_fmt, opts, out_baseimg, NULL);
> if (ret < 0) {
> goto out;
> --
> 2.25.3
>
>
>
Reviewed-by: Stefano Garzarella
Re: [PATCH v2 1/7] x86: lpc9: let firmware negotiate 'CPU hotplug with SMI' features
On Wed, 19 Aug 2020 10:39:04 +0200
Laszlo Ersek wrote:
> Hi Igor,
>
> (CC'ing Daniel, Cornelia, David, Peter)
>
> On 08/18/20 14:22, Igor Mammedov wrote:
> > It will allow firmware to notify QEMU that firmware requires SMI
> > being triggered on CPU hot[un]plug, so that it would be able to account
> > for hotplugged CPU and relocate it to new SMM base and/or safely remove
> > CPU on unplug.
> >
> > Using negotiated features, follow up patches will insert SMI upcall
> > into AML code, to make sure that firmware processes hotplug before
> > guest OS would attempt to use new CPU.
> >
> > Signed-off-by: Igor Mammedov
> > Reviewed-by: Laszlo Ersek
> > ---
> > v2:
> > - rebase on top of 5.1 (move compat values to 5.1 machine)
> > - make "x-smi-cpu-hotunplug" false by default (Laszlo Ersek
> > )
> > ---
> > include/hw/i386/ich9.h | 2 ++
> > include/hw/i386/pc.h | 3 +++
> > hw/i386/pc.c | 6 +-
> > hw/i386/pc_piix.c | 1 +
> > hw/i386/pc_q35.c | 1 +
> > hw/isa/lpc_ich9.c | 13 +
> > 6 files changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> > index a98d10b252..d1bb3f7bf0 100644
> > --- a/include/hw/i386/ich9.h
> > +++ b/include/hw/i386/ich9.h
> > @@ -247,5 +247,7 @@ typedef struct ICH9LPCState {
> >
> > /* bit positions used in fw_cfg SMI feature negotiation */
> > #define ICH9_LPC_SMI_F_BROADCAST_BIT0
> > +#define ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT 1
> > +#define ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT 2
> >
> > #endif /* HW_ICH9_H */
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 3d7ed3a55e..fe52e165b2 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -193,6 +193,9 @@ void pc_system_firmware_init(PCMachineState *pcms,
> > MemoryRegion *rom_memory);
> > void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> > const CPUArchIdList *apic_ids, GArray *entry);
> >
> > +extern GlobalProperty pc_compat_5_1[];
> > +extern const size_t pc_compat_5_1_len;
> > +
> > extern GlobalProperty pc_compat_5_0[];
> > extern const size_t pc_compat_5_0_len;
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 47c5ca3e34..99c6bdbab4 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -97,8 +97,12 @@
> > #include "fw_cfg.h"
> > #include "trace.h"
> >
> > -GlobalProperty pc_compat_5_0[] = {
> > +GlobalProperty pc_compat_5_1[] = {
> > +{ "ICH9-LPC", "x-smi-cpu-hotplug", "off" },
> > };
> > +const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
> > +
> > +GlobalProperty pc_compat_5_0[] = { };
> > const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
> >
> > GlobalProperty pc_compat_4_2[] = {
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index b789e83f9a..d56f2e1b96 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -433,6 +433,7 @@ static void pc_i440fx_5_1_machine_options(MachineClass
> > *m)
> > m->alias = "pc";
> > m->is_default = true;
> > pcmc->default_cpu_version = 1;
> > +compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
> > }
> >
> > DEFINE_I440FX_MACHINE(v5_1, "pc-i440fx-5.1", NULL,
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index a3e607a544..0ca1146a59 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -359,6 +359,7 @@ static void pc_q35_5_1_machine_options(MachineClass *m)
> > pc_q35_machine_options(m);
> > m->alias = "q35";
> > pcmc->default_cpu_version = 1;
> > +compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
> > }
> >
> > DEFINE_Q35_MACHINE(v5_1, "pc-q35-5.1", NULL,
> > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> > index cd6e169d47..19f32bed3e 100644
> > --- a/hw/isa/lpc_ich9.c
> > +++ b/hw/isa/lpc_ich9.c
> > @@ -373,6 +373,15 @@ static void smi_features_ok_callback(void *opaque)
> > /* guest requests invalid features, leave @features_ok at zero */
> > return;
> > }
> > +if (!(guest_features & BIT_ULL(ICH9_LPC_SMI_F_BROADCAST_BIT)) &&
> > +guest_features & (BIT_ULL(ICH9_LPC_SMI_F_CPU_HOTPLUG_BIT) |
> > + BIT_ULL(ICH9_LPC_SMI_F_CPU_HOT_UNPLUG_BIT))) {
> > +/*
> > + * cpu hot-[un]plug with SMI requires SMI broadcast,
> > + * leave @features_ok at zero
> > + */
> > +return;
> > +}
> >
> > /* valid feature subset requested, lock it down, report success */
> > lpc->smi_negotiated_features = guest_features;
> > @@ -747,6 +756,10 @@ static Property ich9_lpc_properties[] = {
> > DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true),
> > DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
> >ICH9_LPC_SMI_F_BROADCAST_BIT, true),
> > +DEFINE_PROP_BIT64("x-smi-cpu-hotplug", ICH9LPCState, smi_host_features,
> > + ICH9_LPC_SMI_F_C
Re: [PATCH] tests: docker: support mxe-based mingw builds
Patchew URL: https://patchew.org/QEMU/20200819080206.27423-1-pbonz...@redhat.com/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === TESTcheck-unit: tests/test-char Unexpected error in object_property_try_add() at /tmp/qemu-test/src/qom/object.c:1181: attempt to add duplicate property 'serial-id' to object (type 'container') ERROR test-char - too few tests run (expected 38, got 9) make: *** [check-unit] Error 1 make: *** Waiting for unfinished jobs TESTcheck-qtest-x86_64: tests/qtest/hd-geo-test qemu-system-aarch64: -accel kvm: invalid accelerator kvm --- raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=238b70bd18a14cdabb8a765c46c87ecc', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-oytm7a2n/src/docker-src.2020-08-19-04.46.55.6123:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2. filter=--filter=label=com.qemu.instance.uuid=238b70bd18a14cdabb8a765c46c87ecc make[1]: *** [docker-run] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-oytm7a2n/src' make: *** [docker-run-test-quick@centos7] Error 2 real12m56.910s user0m9.512s The full log is available at http://patchew.org/logs/20200819080206.27423-1-pbonz...@redhat.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH-for-5.2] memory: Add trace events to audit MemoryRegionOps fields
On Tue, Aug 18, 2020 at 09:56:37AM +0200, Philippe Mathieu-Daudé wrote: > On 8/18/20 8:32 AM, Paolo Bonzini wrote: > > On 06/08/20 17:26, Philippe Mathieu-Daudé wrote: > >> Add trace events to audit MemoryRegionOps field such: > >> - are all the valid/impl fields provided? > >> - is the region a power of two? > >> > >> These cases are accepted, but it is interesting to list them. > >> > >> Example: > >> > >> $ qemu-system-i386 -S -trace memory_region_io_check\* > >> memory_region_io_check_odd_size mr name:'dma-page' size:0x3 > > (a) > > >> memory_region_io_check_access_size_incomplete mr name:'acpi-tmr' > >> min/max:[valid:1/4 impl:4/0] > >> memory_region_io_check_access_size_incomplete mr name:'acpi-evt' > >> min/max:[valid:1/2 impl:2/0] > >> memory_region_io_check_access_size_incomplete mr name:'acpi-cnt' > >> min/max:[valid:1/2 impl:2/0] > > (b) > > > > > Can they be detected using Coccinelle instead? > > For static declarations, probably. Regarding the MemoryRegionOps checks, the following grep command-line shows that all MemoryRegionOps definitions are global: $ git grep 'MemoryRegionOps [^*]' hw/ This means static checking is possible. > > (a) is not really fixable (because some datasheets don't > count the reserved space in the device address map [1]), > but is interesting to audit. > > I believe (b) has to be updated per maintainers preference, > not by an individual developer. IIUC Michael said [2] while > there is no bus information in MemoryRegionOps (and way to > report a bus specific error), it is pointless to blindly fill > the zero access sizes. > > Meanwhile I prefer to share my debugging helpers as trace > events instead of ./configure --enable-maintainer and #ifdef'ry. Can they be turned into a CI check instead of debugging helpers? For example, all existing violations are exempt but new MemoryRegionOps must comply. This way it's not necessary to audit and fix everything right now but code quality is improved in new code. Static checking is nice because there is no need to execute QEMU and trigger all the code paths that lead to MemoryRegionOps usage. Here are more static checker ideas: 1. Rename memory_region_init_io() to memory_region_init_io_unsafe() (or "legacy", "nocheck", etc) and then add a checkpatch.pl error if a new memory_region_init_io_unsafe() call is added. 2. Walk the debuginfo looking for MemoryRegionOps structs and check them. For example, a Python script that uses a DWARF parser. There can be list of exempted source files that are allowed to violate the rules (existing code). 3. Use __attribute__((__section__())) on MemoryRegionOps so they can easily be located in ELF files and check them. For example, a C program that opens the ELF and finds the "memoryregions" section. Stefan signature.asc Description: PGP signature
Re: [PATCH v2 1/7] x86: lpc9: let firmware negotiate 'CPU hotplug with SMI' features
On 08/19/20 10:58, Cornelia Huck wrote: > I consider any patch that adds compat options without adding all compat > machines first to be buggy. We had that in the past, and it had been > painful to sort it out again later. That's why I usually post a compat > machines patch during hardfreeze, to be picked up by anyone who needs > it. > > (See > https://lore.kernel.org/qemu-devel/20200728094645.272149-1-coh...@redhat.com/ > -- this is the patch that Daniel re-posted.) Indeed, thank you -- in the end, I did notice that Daniel's patch started with "From: Cornelia" :) and then I checked my qemu-devel folders for more emails with the same subject. > Just pick my original patch, it has a bunch of acks already. True, but Igor's series wouldn't apply without manual conflict resolution, and I can't do that if I want to respond with a Tested-by. (Of course, picking up your patch and including it in v3 is feasible for Igor.) Thanks! Laszlo
Re: [ovirt-devel] Re: device compatibility interface for live migration with assigned devices
On 2020/8/19 下午4:13, Yan Zhao wrote: On Wed, Aug 19, 2020 at 03:39:50PM +0800, Jason Wang wrote: On 2020/8/19 下午2:59, Yan Zhao wrote: On Wed, Aug 19, 2020 at 02:57:34PM +0800, Jason Wang wrote: On 2020/8/19 上午11:30, Yan Zhao wrote: hi All, could we decide that sysfs is the interface that every VFIO vendor driver needs to provide in order to support vfio live migration, otherwise the userspace management tool would not list the device into the compatible list? if that's true, let's move to the standardizing of the sysfs interface. (1) content common part: (must) - software_version: (in major.minor.bugfix scheme) This can not work for devices whose features can be negotiated/advertised independently. (E.g virtio devices) sorry, I don't understand here, why virtio devices need to use vfio interface? I don't see any reason that virtio devices can't be used by VFIO. Do you? Actually, virtio devices have been used by VFIO for many years: - passthrough a hardware virtio devices to userspace(VM) drivers - using virtio PMD inside guest So, what's different for it vs passing through a physical hardware via VFIO? The difference is in the guest, the device could be either real hardware or emulated ones. even though the features are negotiated dynamically, could you explain why it would cause software_version not work? Virtio device 1 supports feature A, B, C Virtio device 2 supports feature B, C, D So you can't migrate a guest from device 1 to device 2. And it's impossible to model the features with versions. I think this thread is discussing about vfio related devices. - device_api: vfio-pci or vfio-ccw ... - type: mdev type for mdev device or a signature for physical device which is a counterpart for mdev type. device api specific part: (must) - pci id: pci id of mdev parent device or pci id of physical pci device (device_api is vfio-pci)API here. So this assumes a PCI device which is probably not true. for device_api of vfio-pci, why it's not true? for vfio-ccw, it's subchannel_type. Ok but having two different attributes for the same file is not good idea. How mgmt know there will be a 3rd type? that's why some attributes need to be common. e.g. device_api: it's common because mgmt need to know it's a pci device or a ccw device. and the api type is already defined vfio.h. (The field is agreed by and actually suggested by Alex in previous mail) type: mdev_type for mdev. if mgmt does not understand it, it would not be able to create one compatible mdev device. software_version: mgmt can compare the major and minor if it understands this fields. I think it would be helpful if you can describe how mgmt is expected to work step by step with the proposed sysfs API. This can help people to understand. Thanks for the patience. Since sysfs is uABI, when accepted, we need support it forever. That's why we need to be careful. - subchannel_type (device_api is vfio-ccw) vendor driver specific part: (optional) - aggregator - chpid_type - remote_url For "remote_url", just wonder if it's better to integrate or reuse the existing NVME management interface instead of duplicating it here. Otherwise it could be a burden for mgmt to learn. E.g vendor A may use "remote_url" but vendor B may use a different attribute. it's vendor driver specific. vendor specific attributes are inevitable, and that's why we are discussing here of a way to standardizing of it. Well, then you will end up with a very long list to discuss. E.g for networking devices, you will have "mac", "v(x)lan" and a lot of other. Note that "remote_url" is not vendor specific but NVME (class/subsystem) specific. yes, it's just NVMe specific. I added it as an example to show what is vendor specific. if one attribute is vendor specific across all vendors, then it's not vendor specific, it's already common attribute, right? It's common but the issue is about naming and mgmt overhead. Unless you have a unified API per class (NVME, ethernet, etc), you can't prevent vendor from using another name instead of "remote_url". The point is that if vendor/class specific part is unavoidable, why not making all of the attributes vendor specific? some parts need to be common, as I listed above. This is hard, unless VFIO knows the type of device (e.g it's a NVME or networking device). our goal is that mgmt can use it without understanding the meaning of vendor specific attributes. I'm not sure this is the correct design of uAPI. Is there something similar in the existing uAPIs? And it might be hard to work for virtio devices. NOTE: vendors are free to add attributes in this part with a restriction that this attribute is able to be configured with the same name in sysfs too. e.g. Sysfs works well for common attributes belongs to a class, but I'm not sure it can work well for de
Re: [PATCH] pc-bios: s390x: Only set lowcore iplb address on list-directed IPL
On 8/17/20 7:51 PM, Jason J. Herne wrote:
> On 8/17/20 12:30 PM, Cornelia Huck wrote:
>> On Mon, 17 Aug 2020 10:17:34 -0400
>> "Jason J. Herne" wrote:
>>
>>> The POP states that the IPLB location is only written to 0x14 for
>>> list-directed IPL. Some operating systems expect 0x14 to not change on
>>> boot and will fail IPL if it does change.
>>>
>>> Fixes: 9bfc04f9ef6802fff0
>>
>> Should be
>>
>> Fixes: 9bfc04f9ef68 ("pc-bios: s390x: Save iplb location in lowcore")
>>
>>> Signed-off-by: Jason J. Herne
>>> Reviewed-by: Janosch Frank
>>> ---
>>> pc-bios/s390-ccw/jump2ipl.c | 5 -
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>>> index 767012bf0c..5e3e13f4b0 100644
>>> --- a/pc-bios/s390-ccw/jump2ipl.c
>>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>>> @@ -33,7 +33,10 @@ void jump_to_IPL_code(uint64_t address)
>>> {
>>> /* store the subsystem information _after_ the bootmap was loaded */
>>> write_subsystem_identification();
>>> -write_iplb_location();
>>> +
>>> +if (iplb.pbt != S390_IPL_TYPE_CCW) {
>>> +write_iplb_location();
>>> +}
>>
>> What happens for ipl types other than CCW and FCP? IOW, should that
>> rather be a positive check for S390_IPL_TYPE_FCP?
>>
>>>
>>> /* prevent unknown IPL types in the guest */
>>> if (iplb.pbt == S390_IPL_TYPE_QEMU_SCSI) {
>>
>
> Based on my (admittedly limited) understanding of the architecture and
> code, I believe write_iplb_location() should be called at least for
> S390_IPL_TYPE_FCP but I'm not 100% sure on S390_IPL_TYPE_QEMU_SCSI.
> Perhaps Janosch has an idea?
>
> It was originally unconditional, and my new conditional excludes vfio
> CCW which is definitely a step in the right direction, in any case :).
If I remember correctly the problem was that ZIPL used the IPLB lowcore
ptr without checking how it was booted (CCW or FCP). That was fixed in
mid of July by testing if diag308 gives back a config or not.
So we might have a deadlock situation here which I need to think about
for a bit. I'm setting Viktor CC to get a bit more information about the
state of the zipl backports into the distros.
signature.asc
Description: OpenPGP digital signature
Re: [RFC v3 1/1] memory: Skip bad range assertion if notifier supports arbitrary masks
On 2020/8/19 下午4:22, Eugenio Perez Martin wrote:
On Wed, Aug 19, 2020 at 9:15 AM Jason Wang wrote:
On 2020/8/18 下午10:24, Eugenio Perez Martin wrote:
On Wed, Aug 12, 2020 at 10:49 AM Eugenio Perez Martin
wrote:
On Wed, Aug 12, 2020 at 4:24 AM Jason Wang wrote:
On 2020/8/12 上午1:55, Eugenio Pérez wrote:
Signed-off-by: Eugenio Pérez
---
hw/virtio/vhost.c | 2 +-
include/exec/memory.h | 2 ++
softmmu/memory.c | 10 --
3 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 1a1384e7a6..e74ad9e09b 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -729,7 +729,7 @@ static void vhost_iommu_region_add(MemoryListener *listener,
iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr,
MEMTXATTRS_UNSPECIFIED);
iommu_notifier_init(&iommu->n, vhost_iommu_unmap_notify,
-IOMMU_NOTIFIER_UNMAP,
+IOMMU_NOTIFIER_UNMAP | IOMMU_NOTIFIER_IOTLB,
I think we can safely drop IOMMU_NOTIFIER_UNMAP here since device IOTLB
is sufficient.
Btw, IOMMU_NOTIFIER_IOTLB is kind of confusing, maybe something like
IOMMU_NOTIFIER_DEVIOTLB.
Got it, will change.
section->offset_within_region,
int128_get64(end),
iommu_idx);
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 307e527835..4d94c1e984 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -87,6 +87,8 @@ typedef enum {
IOMMU_NOTIFIER_UNMAP = 0x1,
/* Notify entry changes (newly created entries) */
IOMMU_NOTIFIER_MAP = 0x2,
+/* Notify changes on IOTLB entries */
+IOMMU_NOTIFIER_IOTLB = 0x04,
} IOMMUNotifierFlag;
#define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_MAP | IOMMU_NOTIFIER_UNMAP)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index af25987518..e2c5f6d0e7 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1895,6 +1895,7 @@ void memory_region_notify_one(IOMMUNotifier *notifier,
(we probably need a better name of this function, at least something
like "memory_region_iommu_notify_one").
Ok will change.
{
IOMMUNotifierFlag request_flags;
hwaddr entry_end = entry->iova + entry->addr_mask;
+IOMMUTLBEntry tmp = *entry;
/*
* Skip the notification if the notification does not overlap
@@ -1904,7 +1905,12 @@ void memory_region_notify_one(IOMMUNotifier *notifier,
return;
}
-assert(entry->iova >= notifier->start && entry_end <= notifier->end);
+if (notifier->notifier_flags & IOMMU_NOTIFIER_IOTLB) {
+tmp.iova = MAX(tmp.iova, notifier->start);
+tmp.addr_mask = MIN(entry_end, notifier->end) - tmp.iova;
Any reason for doing such re-calculation here, a comment would be helpful.
It was proposed by Peter, but I understand as limiting the
address+range we pass to the notifier. Although vhost seems to support
it as long as it contains (notifier->start, notifier->end) in range, a
future notifier might not.
Yes, actually, I feel confused after reading the codes. Is
notifier->start IOVA or GPA?
In vfio.c, we did:
iommu_notifier_init(&giommu->n, vfio_iommu_map_notify,
IOMMU_NOTIFIER_ALL,
section->offset_within_region,
int128_get64(llend),
iommu_idx);
So it looks to me the start and end are GPA, but the assertion above
check it against IOVA which seems to be wrong
Thanks
I see.
I didn't go so deep, I just assumed that:
* all the addresses were GPA in the vhost-net+virtio-net case,
although the name iova in IOMMUTLBEntry.
* memory region was initialized with IOVA addresses in case of VFIO.
If there's no vIOMMU, IOVA = GPA, so we're fine. But if vIOMMU is
enabled, IOVA allocation is done inside guest so the start/end here not
IOVA anymore.
Maybe the comment should warn about the bad "iova" name, if I'm right?
I assumed that nothing changed in the VFIO case since its notifier has
no IOMMU_NOTIFIER_DEVIOTLB flag and the new conditional in
memory_region_notify_one_iommu, but I will test with a device
passthrough and DPDK again. Do you think another test would be needed?
I'm not sure if it's easy, but it might be interesting to teach DPDK to
use IOVA which is outside the range of [start, end] here.
Maybe Peter can go deeper on this.
Yes, or wait for Peter's comment.
Thanks
Thanks!
It could be done as iommu_entry_crop(IOMMUTLBEntry *entry, const
IOMMUNotifier *notifier) though.
Re: What is bs->reqs_lock for?
18.08.2020 09:16, Paolo Bonzini wrote: On 13/08/20 18:34, Vladimir Sementsov-Ogievskiy wrote: I thought bs is attached to one aio context and aio context attached to one iothread. For now yes, but with multiqueue there would be many iothreads sending requests to the AioContext. The BDS would still have a "home" aiocontext to request socket readiness events, but io_uring/linux_aio/threadpool requests could be issued from any iothread. And all normal request processing of the bs is done in this one iothread. And when we need to access bs externally, we do it in aio_context_acquire / aio_context_release, which protects from parallel access to BlockDriverState fields... But you say, that block/io.c is not protected by AioContext lock.. Does it mean that everything must be thread-safe in block/io.c and all block drivers? Yes. Are tracked_requests different from other fields? A lot of other BlockDriverState fields are not protected by any mutex.. For example: total_sectors, file, backing.. Rules are documented in include/block/block_int.h. I should have guessed on my own.. It seems however that never_freeze was blindly added at the end. Thanks for your answers! -- Best regards, Vladimir
Re: device compatibility interface for live migration with assigned devices
On 2020/8/19 下午1:58, Parav Pandit wrote:
From: Yan Zhao
Sent: Wednesday, August 19, 2020 9:01 AM
On Tue, Aug 18, 2020 at 09:39:24AM +, Parav Pandit wrote:
Please refer to my previous email which has more example and details.
hi Parav,
the example is based on a new vdpa tool running over netlink, not based on
devlink, right?
Right.
For vfio migration compatibility, we have to deal with both mdev and physical
pci devices, I don't think it's a good idea to write a new tool for it, given
we are
able to retrieve the same info from sysfs and there's already an mdevctl from
mdev attribute should be visible in the mdev's sysfs tree.
I do not propose to write a new mdev tool over netlink. I am sorry if I implied
that with my suggestion of vdpa tool.
If underlying device is vdpa, mdev might be able to understand vdpa device and
query from it and populate in mdev sysfs tree.
Note that vdpa is bus independent so it can't work now and the support
of mdev on top of vDPA have been rejected (and duplicated with vhost-vDPA).
Thanks
The vdpa tool I propose is usable even without mdevs.
vdpa tool's role is to create one or more vdpa devices and place on the "vdpa"
bus which is the lowest layer here.
Additionally this tool let user query virtqueue stats, db stats.
When a user creates vdpa net device, user may need to configure features of the
vdpa device such as VIRTIO_NET_F_MAC, default VIRTIO_NET_F_MTU.
These are vdpa level features, attributes. Mdev is layer above it.
Alex
(https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
com%2Fmdevctl%2Fmdevctl&data=02%7C01%7Cparav%40nvidia.com%7C
0c2691d430304f5ea11308d843f2d84e%7C43083d15727340c1b7db39efd9ccc17
a%7C0%7C0%7C637334057571911357&sdata=KxH7PwxmKyy9JODut8BWr
LQyOBylW00%2Fyzc4rEvjUvA%3D&reserved=0).
Sorry for above link mangling. Our mail server is still transitioning due to
company acquisition.
I am less familiar on below points to comment.
hi All,
could we decide that sysfs is the interface that every VFIO vendor driver needs
to provide in order to support vfio live migration, otherwise the userspace
management tool would not list the device into the compatible list?
if that's true, let's move to the standardizing of the sysfs interface.
(1) content
common part: (must)
- software_version: (in major.minor.bugfix scheme)
- device_api: vfio-pci or vfio-ccw ...
- type: mdev type for mdev device or
a signature for physical device which is a counterpart for
mdev type.
device api specific part: (must)
- pci id: pci id of mdev parent device or pci id of physical pci
device (device_api is vfio-pci)
- subchannel_type (device_api is vfio-ccw)
vendor driver specific part: (optional)
- aggregator
- chpid_type
- remote_url
NOTE: vendors are free to add attributes in this part with a restriction that
this
attribute is able to be configured with the same name in sysfs too. e.g.
for aggregator, there must be a sysfs attribute in device node
/sys/devices/pci:00/:00:02.0/882cc4da-dede-11e7-9180-
078a62063ab1/intel_vgpu/aggregator,
so that the userspace tool is able to configure the target device according to
source device's aggregator attribute.
(2) where and structure
proposal 1:
|- [path to device]
|--- migration
| |--- self
| ||-software_version
| ||-device_api
| ||-type
| ||-[pci_id or subchannel_type]
| ||-
| |--- compatible
| ||-software_version
| ||-device_api
| ||-type
| ||-[pci_id or subchannel_type]
| ||-
multiple compatible is allowed.
attributes should be ASCII text files, preferably with only one value per file.
proposal 2: use bin_attribute.
|- [path to device]
|--- migration
| |--- self
| |--- compatible
so we can continue use multiline format. e.g.
cat compatible
software_version=0.1.0
device_api=vfio_pci
type=i915-GVTg_V5_{val1:int:1,2,4,8}
pci_id=80865963
aggregator={val1}/2
Thanks
Yan
Re: [PATCH] pc-bios: s390x: Only set lowcore iplb address on list-directed IPL
On Wed, 19 Aug 2020 11:32:34 +0200
Janosch Frank wrote:
> On 8/17/20 7:51 PM, Jason J. Herne wrote:
> > On 8/17/20 12:30 PM, Cornelia Huck wrote:
> >> On Mon, 17 Aug 2020 10:17:34 -0400
> >> "Jason J. Herne" wrote:
> >>
> >>> The POP states that the IPLB location is only written to 0x14 for
> >>> list-directed IPL. Some operating systems expect 0x14 to not change on
> >>> boot and will fail IPL if it does change.
> >>>
> >>> Fixes: 9bfc04f9ef6802fff0
> >>
> >> Should be
> >>
> >> Fixes: 9bfc04f9ef68 ("pc-bios: s390x: Save iplb location in lowcore")
> >>
> >>> Signed-off-by: Jason J. Herne
> >>> Reviewed-by: Janosch Frank
> >>> ---
> >>> pc-bios/s390-ccw/jump2ipl.c | 5 -
> >>> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> >>> index 767012bf0c..5e3e13f4b0 100644
> >>> --- a/pc-bios/s390-ccw/jump2ipl.c
> >>> +++ b/pc-bios/s390-ccw/jump2ipl.c
> >>> @@ -33,7 +33,10 @@ void jump_to_IPL_code(uint64_t address)
> >>> {
> >>> /* store the subsystem information _after_ the bootmap was loaded */
> >>> write_subsystem_identification();
> >>> -write_iplb_location();
> >>> +
> >>> +if (iplb.pbt != S390_IPL_TYPE_CCW) {
> >>> +write_iplb_location();
> >>> +}
> >>
> >> What happens for ipl types other than CCW and FCP? IOW, should that
> >> rather be a positive check for S390_IPL_TYPE_FCP?
> >>
> >>>
> >>> /* prevent unknown IPL types in the guest */
> >>> if (iplb.pbt == S390_IPL_TYPE_QEMU_SCSI) {
> >>
> >
> > Based on my (admittedly limited) understanding of the architecture and
> > code, I believe write_iplb_location() should be called at least for
> > S390_IPL_TYPE_FCP but I'm not 100% sure on S390_IPL_TYPE_QEMU_SCSI.
> > Perhaps Janosch has an idea?
> >
> > It was originally unconditional, and my new conditional excludes vfio
> > CCW which is definitely a step in the right direction, in any case :).
>
> If I remember correctly the problem was that ZIPL used the IPLB lowcore
> ptr without checking how it was booted (CCW or FCP). That was fixed in
> mid of July by testing if diag308 gives back a config or not.
So we have the problem that old zipl relies on the presence of a value
that must not be there if you follow the architecture? Nasty.
(Is it really "must not change" vs "don't expect anything here"? Not
sure if I'm looking at the right part of the documentation.)
> So we might have a deadlock situation here which I need to think about
> for a bit. I'm setting Viktor CC to get a bit more information about the
> state of the zipl backports into the distros.
Changing this is problematic unless everything we support as a guest is
fixed. Does the guest go boom in a way that it is at least easy to
figure out what went wrong? What breaks when the value continues to be
set?
pgp0Tbkt8Tc8X.pgp
Description: OpenPGP digital signature
[PULL v4 000/150] Meson-based build system
The following changes since commit d0ed6a69d399ae193959225cdeaa9382746c91cc:
Update version for v5.1.0 release (2020-08-11 17:07:03 +0100)
are available in the Git repository at:
https://gitlab.com/bonzini/qemu.git tags/for-upstream
for you to fetch changes up to ab7ac9b093f9d6d7878028b87ad68f70c923e180:
docs: convert build system documentation to rST (2020-08-19 05:22:55 -0400)
v3->v4: fixed aarch32 and MXE builds
The MXE failure seems to be compounded by a Make bug, that causes it not
to obey private target-specific variables. Fortunately it can be avoided
easily, in fact while simplifying a tiny bit the ninja2make converter.
I do wish this was found earlier than last minute, at the same time it's
an easy fix and the fixed version rules has really trivial rules; thus,
it follows even more the principle that one should never need to look
at Makefile.ninja. If this alternative implementation had come to my
mind, I would definitely have chosen it from the beginning.
New build system.
Missing:
* converting configure tests
* converting unit tests
* converting some remaining parts of the installation
Marc-André Lureau (90):
optionrom: simplify Makefile
build-sys hack: ensure target directory is there
configure: expand path variables for meson configure
configure: generate Meson cross file
build-sys hack: link with whole .fa archives
build-sys: add meson submodule
meson: enable pie
meson: use coverage option
meson: add remaining generated tcg trace helpers
meson: add version.o
contrib/vhost-user-input: convert to meson
contrib/vhost-user-gpu: convert to meson
contrib/ivshmem: convert to meson
contrib/elf2dmp: convert to meson
meson: add macos dependencies
meson: convert vss-win32
meson: add msi generation
meson: add qemu-bridge-helper
meson: add qemu-keymap
meson: add qemu-edid
meson: add virtfs-proxy-helper
meson: keymap-gen
meson: generate qemu-version.h
meson: generate shader headers
meson: generate hxtool files
meson: handle edk2 bios and descriptors
meson: convert qom directory to Meson (tools part)
meson: convert authz directory to Meson
meson: convert crypto directory to Meson
meson: convert io directory to Meson
meson: convert target/s390x/gen-features.h
meson: add modules infrastructure
meson: convert chardev directory to Meson (tools part)
meson: convert block
meson: qemu-{img,io,nbd}
meson: qemu-pr-helper
meson: convert ui directory to Meson
meson: convert trace/
meson: convert dump/
meson: convert replay directory to Meson
meson: convert migration directory to Meson
meson: convert net directory to Meson
meson: convert backends directory to Meson
meson: convert fsdev/
meson: convert disas directory to Meson
meson: convert qapi-specific to meson
meson: convert hw/xen
meson: convert hw/core
meson: convert hw/smbios
meson: convert hw/mem
meson: convert hw/watchdog
meson: convert hw/virtio
meson: convert hw/vfio
meson: convert hw/ssi
meson: convert hw/sd
meson: convert hw/scsi
meson: convert hw/pcmcia
meson: convert hw/pci-host
meson: convert hw/pci-bridge
meson: convert hw/pci
meson: convert hw/nvram
meson: convert hw/rdma
meson: convert hw/net
meson: convert hw/misc
meson: convert hw/isa
meson: convert hw/ipmi
meson: convert hw/ipack
meson: convert hw/intc
meson: convert hw/input
meson: convert hw/ide
meson: convert hw/i2c
meson: convert hw/hyperv
meson: convert hw/gpio
meson: convert hw/dma
meson: convert hw/display
meson: convert hw/cpu
meson: convert hw/char
meson: convert hw/block
meson: convert hw/audio
meson: convert hw/adc
meson: convert hw/acpi
meson: convert hw/9pfs, cleanup
meson: convert hw/arch*
meson: accel
meson: linux-user
meson: bsd-user
meson: cpu-emu
meson: convert systemtap files
rules.mak: remove version.o
meson: convert po/
Paolo Bonzini (59):
oss-fuzz/build: remove LIB_FUZZING_ENGINE
trace: switch position of headers to what Meson requires
meson: rename included C source files to .c.inc
meson: rename .inc.h files to .h.inc
tests/vm: do not pollute configure with --efi-aarch64
tests/vm: check for Python YAML parser in the Makefile
tests/docker: add test script for static linux-user builds
nsis: use "make DESTDIR=" instead of "make prefix="
configure: do not include $(...) variables in config-host.mak
configure: prepare CFLAGS/CXXFLAGS/LDFLAGS for
Re: [PATCH v5 1/5] block/io: fix bdrv_co_block_status_above
Thanks a lot for reviewing!
18.08.2020 17:15, Alberto Garcia wrote:
On Wed 10 Jun 2020 02:04:22 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
+ * The top layer deferred to this layer, and because this layer is
+ * short, any zeroes that we synthesize beyond EOF behave as if
they
+ * were allocated at this layer
*/
+assert(ret & BDRV_BLOCK_EOF);
*pnum = bytes;
+if (file) {
+*file = p;
+}
+return BDRV_BLOCK_ZERO | BDRV_BLOCK_ALLOCATED;
You don't add BDRV_BLOCK_EOF to the return code here ?
No we shouldn't, as this is the end of backing file when the top layer is
larger.
+res = bdrv_block_status_above(bs, NULL, offset, bytes, &nr, NULL,
NULL);
+offset += nr;
+bytes -= nr;
+} while (res >= 0 && (res & BDRV_BLOCK_ZERO) && nr && bytes);
About this last "... && nr && bytes", I think 'nr' already implies
'bytes', maybe you want to use an assertion instead?
No, on the last iteration, bytes would be 0 and nr is a last chunk. So, if we
check
only nr, we'll do one extra call of bdrv_block_status_above with bytes=0, I
don't
want do it.
Berto
--
Best regards,
Vladimir
Re: virtio-vsock requires 'disable-legacy=on' in QEMU 5.1
Hi,
On 8/13/20 12:37 PM, Cornelia Huck wrote:
> On Thu, 13 Aug 2020 12:24:30 +0200
> Stefano Garzarella wrote:
>
>> On Thu, Aug 13, 2020 at 11:28:20AM +0200, Cornelia Huck wrote:
>>> On Thu, 13 Aug 2020 11:16:56 +0200
>>> Stefano Garzarella wrote:
>>>
Hi,
Qinghua discovered that virtio-vsock-pci requires 'disable-legacy=on' in
QEMU 5.1:
$ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5
qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=5:
device is modern-only, use disable-legacy=on
For info that's the same for virtio-iommu. + Jean-Philippe.
Reading this thread to better understand what is the best thing to do
now ;-)
Thanks
Eric
Bisecting I found that this behaviour starts from this commit:
9b3a35ec82 ("virtio: verify that legacy support is not accidentally on")
>>>
>>> Oh, I had heard that from others already, was still trying to figure
>>> out what to do.
>>>
IIUC virtio-vsock is modern-only, so I tried this patch and it works:
diff --git a/hw/virtio/vhost-user-vsock-pci.c
b/hw/virtio/vhost-user-vsock-pci.c
index f4cf95873d..6e4cc874cd 100644
--- a/hw/virtio/vhost-user-vsock-pci.c
+++ b/hw/virtio/vhost-user-vsock-pci.c
@@ -40,6 +40,7 @@ static void vhost_user_vsock_pci_realize(VirtIOPCIProxy
*vpci_dev, Error **errp)
VHostUserVSockPCI *dev = VHOST_USER_VSOCK_PCI(vpci_dev);
DeviceState *vdev = DEVICE(&dev->vdev);
+virtio_pci_force_virtio_1(vpci_dev);
qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
}
diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c
index a815278e69..f641b974e9 100644
--- a/hw/virtio/vhost-vsock-pci.c
+++ b/hw/virtio/vhost-vsock-pci.c
@@ -44,6 +44,7 @@ static void vhost_vsock_pci_realize(VirtIOPCIProxy
*vpci_dev, Error **errp)
VHostVSockPCI *dev = VHOST_VSOCK_PCI(vpci_dev);
DeviceState *vdev = DEVICE(&dev->vdev);
+virtio_pci_force_virtio_1(vpci_dev);
qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
}
Do you think this is the right approach or is there a better way to
solve this issue?
>>>
>>> We basically have three possible ways to deal with this:
>>>
>>> - Force it to modern (i.e., what you have been doing; would need the
>>> equivalent changes in ccw as well.)
>>
>> Oo, thanks for pointing out ccw!
>> I don't know ccw well, in this case should we set dev->max_rev to 1 or 2
>> to force to modern?
>
> No, ->max_rev is the wrong side of the limit :) You want
>
> ccw_dev->force_revision_1 = true;
>
> in _instance_init() (see e.g. virtio-ccw-gpu.c).
>
>>
>>> Pro: looks like the cleanest approach.
>>> Con: not sure if we would need backwards compatibility support,
>>> which looks hairy.
>>
>> Not sure too.
>
> Yes, I'm not sure at all how to handle user-specified values for
> legacy/modern.
>
>>
>>> - Add vsock to the list of devices with legacy support.
>>> Pro: Existing setups continue to work.
>>> Con: If vsock is really virtio-1-only, we still carry around
>>> possibly broken legacy support.
>>
>> I'm not sure it is virtio-1-only, but virtio-vsock was introduced in
>> 2016, so I supposed it is modern-only.
>
> Yes, I would guess so as well.
>
>>
>> How can I verify that? Maybe forcing legacy mode and run some tests.
>
> Probably yes. The likeliest area with issues is probably endianness, so
> maybe with something big endian in the mix?
>
>>
>>> - Do nothing, have users force legacy off. Bad idea, as ccw has no way
>>> to do that on the command line.
>>>
>>> The first option is probably best.
>>>
>>
>> Yeah, I agree with you!
>
> Yes, it's really a pity we only noticed this after the release; this
> was supposed to stop new devices with legacy support creeping in, not
> to break existing command lines :(
>
>
Re: [PATCH v6 1/4] copy-on-read: Support preadv/pwritev_part functions
19.08.2020 00:24, Andrey Shinkevich wrote: Add support for the recently introduced functions bdrv_co_preadv_part() and bdrv_co_pwritev_part() to the COR-filter driver. Signed-off-by: Andrey Shinkevich Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [RFC PATCH 20/22] block/export: Add block-export-del
On 13.08.20 18:29, Kevin Wolf wrote: > Implement a new QMP command block-export-del and make nbd-server-remove > a wrapper around it. > > Signed-off-by: Kevin Wolf > --- > qapi/block-export.json | 30 +++ > include/block/nbd.h| 1 - > block/export/export.c | 54 ++ > block/monitor/block-hmp-cmds.c | 2 +- > blockdev-nbd.c | 28 -- > nbd/server.c | 14 - > 6 files changed, 79 insertions(+), 50 deletions(-) [...] > diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c > index 6c823234a9..10165252cf 100644 > --- a/block/monitor/block-hmp-cmds.c > +++ b/block/monitor/block-hmp-cmds.c > @@ -477,7 +477,7 @@ void hmp_nbd_server_remove(Monitor *mon, const QDict > *qdict) > Error *err = NULL; > > /* Rely on NBD_SERVER_REMOVE_MODE_SAFE being the default */ This comment needs adjustment, too. With that done: Reviewed-by: Max Reitz > -qmp_nbd_server_remove(name, force, NBD_SERVER_REMOVE_MODE_HARD, &err); > +qmp_nbd_server_remove(name, force, BLOCK_EXPORT_REMOVE_MODE_HARD, &err); > hmp_handle_error(mon, err); > } signature.asc Description: OpenPGP digital signature
Re: [PATCH V2] Introduce a new flag for piix to disable root bus PCI hotplug
On Tue, 11 Aug 2020 18:42:08 +0530
Ani Sinha wrote:
> We introduce a new global flag for PIIX with which we can turn on or off PCI
> device hotplug on the root bus. This flag can be used to prevent all PCI
> devices from getting hotplugged or unplugged from the root PCI bus.
Tested-by: Igor Mammedov
somewhere in intial versions there were mentionig why we are doing it
(i.e for Windows sake because ...)
I suggest to add it to commit message so reason for this won't be lost.
Also giving example how to use option in commit message would be good.
with this changes:
Reviewed-by: Igor Mammedov
>
> Signed-off-by: Ani Sinha
> ---
> hw/acpi/piix4.c | 3 +++
> hw/i386/acpi-build.c | 20
> 2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 26bac4f..94ec35a 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -78,6 +78,7 @@ typedef struct PIIX4PMState {
>
> AcpiPciHpState acpi_pci_hotplug;
> bool use_acpi_hotplug_bridge;
> +bool use_acpi_root_pci_hotplug;
>
> uint8_t disable_s3;
> uint8_t disable_s4;
> @@ -635,6 +636,8 @@ static Property piix4_pm_properties[] = {
> DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
> DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState,
> use_acpi_hotplug_bridge, true),
> +DEFINE_PROP_BOOL("acpi-root-pci-hotplug", PIIX4PMState,
> + use_acpi_root_pci_hotplug, true),
> DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
> acpi_memory_hotplug.is_enabled, true),
> DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b7bcbbb..a82e5c1 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -95,6 +95,7 @@ typedef struct AcpiPmInfo {
> bool s3_disabled;
> bool s4_disabled;
> bool pcihp_bridge_en;
> +bool pcihp_root_en;
> uint8_t s4_val;
> AcpiFadtData fadt;
> uint16_t cpu_hp_io_base;
> @@ -245,6 +246,9 @@ static void acpi_get_pm_info(MachineState *machine,
> AcpiPmInfo *pm)
> pm->pcihp_bridge_en =
> object_property_get_bool(obj, "acpi-pci-hotplug-with-bridge-support",
> NULL);
> +pm->pcihp_root_en =
> +object_property_get_bool(obj, "acpi-root-pci-hotplug", NULL);
> +
> }
>
> static void acpi_get_misc_info(AcpiMiscInfo *info)
> @@ -337,12 +341,15 @@ static void build_append_pcihp_notify_entry(Aml
> *method, int slot)
> }
>
> static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> - bool pcihp_bridge_en)
> + bool pcihp_bridge_en,
> + bool pcihp_root_en)
> {
> Aml *dev, *notify_method = NULL, *method;
> QObject *bsel;
> PCIBus *sec;
> int i;
> +bool root_bus = pci_bus_is_root(bus);
> +bool root_pcihp_disabled = (root_bus && !pcihp_root_en);
>
> bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> NULL);
> if (bsel) {
> @@ -361,6 +368,9 @@ static void build_append_pci_bus_devices(Aml
> *parent_scope, PCIBus *bus,
> bool bridge_in_acpi;
>
> if (!pdev) {
> +/* skip if pci hotplug for the root bus is disabled */
> +if (root_pcihp_disabled)
> +continue;
> if (bsel) { /* add hotplug slots for non present devices */
> dev = aml_device("S%.02X", PCI_DEVFN(slot, 0));
> aml_append(dev, aml_name_decl("_SUN", aml_int(slot)));
> @@ -419,7 +429,7 @@ static void build_append_pci_bus_devices(Aml
> *parent_scope, PCIBus *bus,
> method = aml_method("_S3D", 0, AML_NOTSERIALIZED);
> aml_append(method, aml_return(aml_int(s3d)));
> aml_append(dev, method);
> -} else if (hotplug_enabled_dev) {
> +} else if (hotplug_enabled_dev && !root_pcihp_disabled) {
> /* add _SUN/_EJ0 to make slot hotpluggable */
> aml_append(dev, aml_name_decl("_SUN", aml_int(slot)));
>
> @@ -439,7 +449,8 @@ static void build_append_pci_bus_devices(Aml
> *parent_scope, PCIBus *bus,
> */
> PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
>
> -build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en);
> +build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en,
> + pcihp_root_en);
> }
> /* slot descriptor has been composed, add it into parent context */
> aml_append(parent_scope, dev);
> @@ -1818,7 +1829,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> if (bus) {
> Aml *scope = aml_scope("PCI0");
> /* Scan all PCI buses. Generate tables to support hotplug. */
> -bui
[PATCH v2 02/21] m25p80: Add support for mx25l25635f
The mx25l25635f is an extenstion of the mx25l25635e. It includes QPI
support, 4-Byte Address Command Set and faster transfers. See this
document for more details :
https://www.macronix.com/Lists/ApplicationNote/Attachments/1892/AN0200V1_MGRT_MX25L25635E_25735E%20to%20MX25L25635F_25735F.pdf
Both devices have the same 3bytes JEDEC ID: 0xc22019. They can be
distinguished with the QPIID command which is only available on
mx25l25635f and their Serial Flash Discoverable Parameters. However,
some FW use the JEDEC ID. For instance, on a SuperMicro P9 Boston, the
BMC FW reports :
BMC flash ID: 0xc21920c2
jedec_id: 0xc21920c2
flash type: MX25L25635F
ReadClk=0x32, WriteClk=0x85, EraseClk=0x85
[smcfw_spi] cpuclk: 19800 MHz, RefCLK: 2400 MHz, AXI-AHB ratio: 2:1
platform_flash: MX25L25635F (32768 Kbytes)
Define the mx25l25635f with an extended JEDEC ID.
Reviewed-by: Joel Stanley
Signed-off-by: Cédric Le Goater
---
hw/block/m25p80.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 605ff55c6756..1696ab1f7821 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -218,6 +218,7 @@ static const FlashPartInfo known_devices[] = {
{ INFO("mx25l12805d", 0xc22018, 0, 64 << 10, 256, 0) },
{ INFO("mx25l12855e", 0xc22618, 0, 64 << 10, 256, 0) },
{ INFO6("mx25l25635e", 0xc22019, 0xc22019, 64 << 10, 512, 0) },
+{ INFO("mx25l25635f", 0xc22019, 0xc200, 64 << 10, 512, 0) },
{ INFO("mx25l25655e", 0xc22619, 0, 64 << 10, 512, 0) },
{ INFO("mx66u51235f", 0xc2253a, 0, 64 << 10, 1024, ER_4K | ER_32K) },
{ INFO("mx66u1g45g", 0xc2253b, 0, 64 << 10, 2048, ER_4K | ER_32K) },
--
2.25.4
[PATCH v2 13/21] ftgmac100: Check for invalid len and address before doing a DMA transfer
According to the Aspeed specs, no interrupts are raised in that case
but a "Tx-packets lost" status seems like a good modeling choice for
all implementations. It is covered by the Linux kernel.
Cc: Frederic Konrad
Reviewed-by: Joel Stanley
Signed-off-by: Cédric Le Goater
---
hw/net/ftgmac100.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 014980d30aca..280aa3d3a1e2 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -507,6 +507,15 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t
tx_ring,
}
len = FTGMAC100_TXDES0_TXBUF_SIZE(bd.des0);
+if (!len) {
+/*
+ * 0 is an invalid size, however the HW does not raise any
+ * interrupt. Flag an error because the guest is buggy.
+ */
+qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid segment size\n",
+ __func__);
+}
+
if (frame_size + len > sizeof(s->frame)) {
qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too big : %d bytes\n",
__func__, len);
--
2.25.4
[PATCH v2 01/21] m25p80: Return the JEDEC ID twice for mx25l25635e
The mx25l25635e returns the JEDEC ID twice when issuing a RDID command :
[2.512027] aspeed-smc 1e63.spi: reading JEDEC ID C2:20:19:C2:20:19
This can break some firmware testing for this condition on the
supermicrox11-bmc machine.
Reported-by: erik-smit
Signed-off-by: Cédric Le Goater
---
hw/block/m25p80.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 82270884416e..605ff55c6756 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -217,7 +217,7 @@ static const FlashPartInfo known_devices[] = {
{ INFO("mx25l6405d", 0xc22017, 0, 64 << 10, 128, 0) },
{ INFO("mx25l12805d", 0xc22018, 0, 64 << 10, 256, 0) },
{ INFO("mx25l12855e", 0xc22618, 0, 64 << 10, 256, 0) },
-{ INFO("mx25l25635e", 0xc22019, 0, 64 << 10, 512, 0) },
+{ INFO6("mx25l25635e", 0xc22019, 0xc22019, 64 << 10, 512, 0) },
{ INFO("mx25l25655e", 0xc22619, 0, 64 << 10, 512, 0) },
{ INFO("mx66u51235f", 0xc2253a, 0, 64 << 10, 1024, ER_4K | ER_32K) },
{ INFO("mx66u1g45g", 0xc2253b, 0, 64 << 10, 2048, ER_4K | ER_32K) },
--
2.25.4
[PATCH v2 04/21] aspeed/scu: Fix valid access size on AST2400
The read access size of the SCU registers can be 1/2/4 bytes and write
is 4 bytes and all Aspeed models would need a .valid.accepts() handler.
For the moment, set the min access size to 1 byte to cover both read
and write operations on the AST2400 but keep the min access size of
the other SoCs to 4 bytes as this is an unusual access size.
This fixes support for some old firmware doing 2 bytes reads on the
AST2400 SoC.
Reported-by: erik-smit
Reviewed-by: Joel Stanley
Signed-off-by: Cédric Le Goater
---
hw/misc/aspeed_scu.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index ec4fef900e27..764222404bef 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -328,9 +328,10 @@ static const MemoryRegionOps aspeed_ast2400_scu_ops = {
.read = aspeed_scu_read,
.write = aspeed_ast2400_scu_write,
.endianness = DEVICE_LITTLE_ENDIAN,
-.valid.min_access_size = 4,
-.valid.max_access_size = 4,
-.valid.unaligned = false,
+.valid = {
+.min_access_size = 1,
+.max_access_size = 4,
+},
};
static const MemoryRegionOps aspeed_ast2500_scu_ops = {
--
2.25.4
[PATCH v2 07/21] aspeed/smc: Fix max_slaves of the legacy SMC device
The legacy controller only has one slave.
Reviewed-by: Joel Stanley
Signed-off-by: Cédric Le Goater
---
hw/ssi/aspeed_smc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 0646e0dca72e..8c79a5552f93 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -259,7 +259,7 @@ static const AspeedSMCController controllers[] = {
.r_timings = R_TIMINGS,
.nregs_timings = 1,
.conf_enable_w0= CONF_ENABLE_W0,
-.max_slaves= 5,
+.max_slaves= 1,
.segments = aspeed_segments_legacy,
.flash_window_base = ASPEED_SOC_SMC_FLASH_BASE,
.flash_window_size = 0x600,
--
2.25.4
[PATCH v2 18/21] aspeed/sdmc: Simplify calculation of RAM bits
Changes in commit 533eb415df2e ("arm/aspeed: actually check RAM size")
introduced a 'valid_ram_sizes' array which can be used to compute the
associated bit field value encoding the RAM size. The field is simply
the index of the array.
Reviewed-by: Joel Stanley
Signed-off-by: Cédric Le Goater
---
hw/misc/aspeed_sdmc.c | 79 ++-
1 file changed, 25 insertions(+), 54 deletions(-)
diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 81c73450ab5d..08f856cbda7e 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -159,57 +159,6 @@ static const MemoryRegionOps aspeed_sdmc_ops = {
.valid.max_access_size = 4,
};
-static int ast2400_rambits(AspeedSDMCState *s)
-{
-switch (s->ram_size >> 20) {
-case 64:
-return ASPEED_SDMC_DRAM_64MB;
-case 128:
-return ASPEED_SDMC_DRAM_128MB;
-case 256:
-return ASPEED_SDMC_DRAM_256MB;
-case 512:
-return ASPEED_SDMC_DRAM_512MB;
-default:
-g_assert_not_reached();
-break;
-}
-}
-
-static int ast2500_rambits(AspeedSDMCState *s)
-{
-switch (s->ram_size >> 20) {
-case 128:
-return ASPEED_SDMC_AST2500_128MB;
-case 256:
-return ASPEED_SDMC_AST2500_256MB;
-case 512:
-return ASPEED_SDMC_AST2500_512MB;
-case 1024:
-return ASPEED_SDMC_AST2500_1024MB;
-default:
-g_assert_not_reached();
-break;
-}
-}
-
-static int ast2600_rambits(AspeedSDMCState *s)
-{
-switch (s->ram_size >> 20) {
-case 256:
-return ASPEED_SDMC_AST2600_256MB;
-case 512:
-return ASPEED_SDMC_AST2600_512MB;
-case 1024:
-return ASPEED_SDMC_AST2600_1024MB;
-case 2048:
-return ASPEED_SDMC_AST2600_2048MB;
-default:
-g_assert_not_reached();
-break;
-}
-}
-
static void aspeed_sdmc_reset(DeviceState *dev)
{
AspeedSDMCState *s = ASPEED_SDMC(dev);
@@ -324,10 +273,32 @@ static const TypeInfo aspeed_sdmc_info = {
.abstract = true,
};
+static int aspeed_sdmc_get_ram_bits(AspeedSDMCState *s)
+{
+AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
+int i;
+
+/*
+ * The bitfield value encoding the RAM size is the index of the
+ * possible RAM size array
+ */
+for (i = 0; asc->valid_ram_sizes[i]; i++) {
+if (s->ram_size == asc->valid_ram_sizes[i]) {
+return i;
+}
+}
+
+/*
+ * Invalid RAM sizes should have been excluded when setting the
+ * SoC RAM size.
+ */
+g_assert_not_reached();
+}
+
static uint32_t aspeed_2400_sdmc_compute_conf(AspeedSDMCState *s, uint32_t
data)
{
uint32_t fixed_conf = ASPEED_SDMC_VGA_COMPAT |
-ASPEED_SDMC_DRAM_SIZE(ast2400_rambits(s));
+ASPEED_SDMC_DRAM_SIZE(aspeed_sdmc_get_ram_bits(s));
/* Make sure readonly bits are kept */
data &= ~ASPEED_SDMC_READONLY_MASK;
@@ -385,7 +356,7 @@ static uint32_t
aspeed_2500_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
uint32_t fixed_conf = ASPEED_SDMC_HW_VERSION(1) |
ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
ASPEED_SDMC_CACHE_INITIAL_DONE |
-ASPEED_SDMC_DRAM_SIZE(ast2500_rambits(s));
+ASPEED_SDMC_DRAM_SIZE(aspeed_sdmc_get_ram_bits(s));
/* Make sure readonly bits are kept */
data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
@@ -451,7 +422,7 @@ static uint32_t
aspeed_2600_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
{
uint32_t fixed_conf = ASPEED_SDMC_HW_VERSION(3) |
ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
-ASPEED_SDMC_DRAM_SIZE(ast2600_rambits(s));
+ASPEED_SDMC_DRAM_SIZE(aspeed_sdmc_get_ram_bits(s));
/* Make sure readonly bits are kept (use ast2500 mask) */
data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
--
2.25.4
[PATCH v2 16/21] aspeed/sdmc: Perform memory training
From: Joel Stanley
This allows qemu to run the "normal" power on reset boot path through
u-boot, where the DDR is trained.
An enhancement would be to have the SCU bit stick across qemu reboots,
but be unset on initial boot.
Proper modelling would be to discard all writes to the phy setting regs
at offset 0x100 - 0x400 and to model the phy status regs at offset
0x400.
The status regs model would only need to account for offets 0x00,
0x50, 0x68 and 0x7c.
Signed-off-by: Joel Stanley
[ clg: checkpatch fixes ]
Signed-off-by: Cédric Le Goater
---
include/hw/misc/aspeed_sdmc.h | 13 -
hw/misc/aspeed_scu.c | 2 +-
hw/misc/aspeed_sdmc.c | 19 +--
3 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
index cea1e67fe365..c6226957dd3d 100644
--- a/include/hw/misc/aspeed_sdmc.h
+++ b/include/hw/misc/aspeed_sdmc.h
@@ -17,7 +17,18 @@
#define TYPE_ASPEED_2500_SDMC TYPE_ASPEED_SDMC "-ast2500"
#define TYPE_ASPEED_2600_SDMC TYPE_ASPEED_SDMC "-ast2600"
-#define ASPEED_SDMC_NR_REGS (0x174 >> 2)
+/*
+ * SDMC has 174 documented registers. In addition the u-boot device tree
+ * describes the following regions:
+ * - PHY status regs at offset 0x400, length 0x200
+ * - PHY setting regs at offset 0x100, length 0x300
+ *
+ * There are two sets of MRS (Mode Registers) configuration in ast2600 memory
+ * system: one is in the SDRAM MC (memory controller) which is used in run
+ * time, and the other is in the DDR-PHY IP which is used during DDR-PHY
+ * training.
+ */
+#define ASPEED_SDMC_NR_REGS (0x500 >> 2)
typedef struct AspeedSDMCState {
/*< private >*/
diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index 764222404bef..dc6dd87c22f4 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -656,7 +656,7 @@ static const uint32_t
ast2600_a1_resets[ASPEED_AST2600_SCU_NR_REGS] = {
[AST2600_SYS_RST_CTRL2] = 0xFFFC,
[AST2600_CLK_STOP_CTRL] = 0x7F8A,
[AST2600_CLK_STOP_CTRL2]= 0xFFF0FFF0,
-[AST2600_SDRAM_HANDSHAKE] = 0x0040, /* SoC completed DRAM init */
+[AST2600_SDRAM_HANDSHAKE] = 0x,
[AST2600_HPLL_PARAM]= 0x1000405F,
[AST2600_CHIP_ID0] = 0x1234ABCD,
[AST2600_CHIP_ID1] = 0x,
diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 855848b7d23a..ff2809a09965 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -113,7 +113,7 @@ static uint64_t aspeed_sdmc_read(void *opaque, hwaddr addr,
unsigned size)
if (addr >= ARRAY_SIZE(s->regs)) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
- __func__, addr);
+ __func__, addr * 4);
return 0;
}
@@ -206,6 +206,19 @@ static void aspeed_sdmc_reset(DeviceState *dev)
/* Set ram size bit and defaults values */
s->regs[R_CONF] = asc->compute_conf(s, 0);
+
+/*
+ * PHY status:
+ * - set phy status ok (set bit 1)
+ * - initial PVT calibration ok (clear bit 3)
+ * - runtime calibration ok (clear bit 5)
+ */
+s->regs[0x100] = BIT(1);
+
+/* PHY eye window: set all as passing */
+s->regs[0x100 | (0x68 / 4)] = 0xff;
+s->regs[0x100 | (0x7c / 4)] = 0xff;
+s->regs[0x100 | (0x50 / 4)] = 0xfff;
}
static void aspeed_sdmc_get_ram_size(Object *obj, Visitor *v, const char *name,
@@ -443,7 +456,9 @@ static void aspeed_2600_sdmc_write(AspeedSDMCState *s,
uint32_t reg,
}
if (reg != R_PROT && s->regs[R_PROT] == PROT_SOFTLOCKED) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n", __func__);
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: SDMC is locked! (write to MCR%02x blocked)\n",
+ __func__, reg * 4);
return;
}
--
2.25.4
[PATCH v2 00/21] aspeed: cleanups and some extensions
Hello, This series includes various fixes improving the support of Aspeed machines. Extra attention was given to the robustness of the ftgmac100 model. A small kernel module tester was created for this purpose : https://github.com/legoater/ftgmac100-test/ Changes in v2 : - definitions for some new flash models in m25p80 by Igor - All Joel's comments should have been addressed - A better fix of the integer overflow in ftgmac100_do_tx suggested by Peter. This needs a couple more reviewed-by before I can send a PR. Thanks, C. Cédric Le Goater (16): m25p80: Return the JEDEC ID twice for mx25l25635e m25p80: Add support for mx25l25635f m25p80: Add support for n25q512ax3 aspeed/scu: Fix valid access size on AST2400 aspeed/smc: Fix MemoryRegionOps definition aspeed/smc: Fix max_slaves of the legacy SMC device aspeed/sdhci: Fix reset sequence ftgmac100: Fix registers that can be read ftgmac100: Fix interrupt status "Packet transmitted on ethernet" ftgmac100: Fix interrupt status "Packet moved to RX FIFO" ftgmac100: Change interrupt status when a DMA error occurs ftgmac100: Check for invalid len and address before doing a DMA transfer ftgmac100: Fix integer overflow in ftgmac100_do_tx() ftgmac100: Improve software reset aspeed/sdmc: Simplify calculation of RAM bits aspeed/smc: Open AHB window of the second chip of the AST2600 FMC controller Igor Kononenko (2): arm: aspeed: add strap define `25HZ` of AST2500 hw: add a number of SPI-flash's of m25p80 family Joel Stanley (2): aspeed/sdmc: Perform memory training aspeed/sdmc: Allow writes to unprotected registers erik-smit (1): hw/arm/aspeed: Add board model for Supermicro X11 BMC include/hw/misc/aspeed_scu.h | 1 + include/hw/misc/aspeed_sdmc.h | 13 +++- hw/arm/aspeed.c | 35 ++ hw/block/m25p80.c | 6 +- hw/misc/aspeed_scu.c | 9 +-- hw/misc/aspeed_sdmc.c | 125 +++--- hw/net/ftgmac100.c| 95 ++ hw/sd/aspeed_sdhci.c | 14 +++- hw/ssi/aspeed_smc.c | 6 +- 9 files changed, 209 insertions(+), 95 deletions(-) -- 2.25.4
[PATCH v2 03/21] m25p80: Add support for n25q512ax3
Datasheet available here :
https://www.micron.com/-/media/client/global/Documents/Products/Data%20Sheet/NOR%20Flash/Serial%20NOR/N25Q/n25q_512mb_1ce_3v_65nm.pdf
Reviewed-by: Joel Stanley
Signed-off-by: Cédric Le Goater
---
hw/block/m25p80.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 1696ab1f7821..8a3fd959e218 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -238,6 +238,7 @@ static const FlashPartInfo known_devices[] = {
{ INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) },
{ INFO("n25q256a",0x20ba19, 0, 64 << 10, 512, ER_4K) },
{ INFO("n25q512a",0x20ba20, 0, 64 << 10, 1024, ER_4K) },
+{ INFO("n25q512ax3", 0x20ba20, 0x1000, 64 << 10, 1024, ER_4K) },
{ INFO_STACKED("n25q00",0x20ba21, 0x1000, 64 << 10, 2048, ER_4K, 4) },
{ INFO_STACKED("n25q00a", 0x20bb21, 0x1000, 64 << 10, 2048, ER_4K, 4) },
{ INFO_STACKED("mt25ql01g", 0x20ba21, 0x1040, 64 << 10, 2048, ER_4K, 2) },
--
2.25.4
[PATCH v2 05/21] hw/arm/aspeed: Add board model for Supermicro X11 BMC
From: erik-smit
The BMC Firmware can be downloaded from :
https://www.supermicro.com/en/products/motherboard/X11SSL-F
Signed-off-by: erik-smit
Reviewed-by: Joel Stanley
Reviewed-by: Cédric Le Goater
[ clg: Modified commit log ]
Message-Id: <20200715173418.186-1-erik.lucas.s...@gmail.com>
Signed-off-by: Cédric Le Goater
---
hw/arm/aspeed.c | 35 +++
1 file changed, 35 insertions(+)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index fcb1a7cd8729..d17a4885a03c 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -57,6 +57,20 @@ struct AspeedMachineState {
SCU_HW_STRAP_VGA_SIZE_SET(VGA_16M_DRAM) | \
SCU_AST2400_HW_STRAP_BOOT_MODE(AST2400_SPI_BOOT))
+/* TODO: Find the actual hardware value */
+#define SUPERMICROX11_BMC_HW_STRAP1 ( \
+SCU_AST2400_HW_STRAP_DRAM_SIZE(DRAM_SIZE_128MB) | \
+SCU_AST2400_HW_STRAP_DRAM_CONFIG(2) | \
+SCU_AST2400_HW_STRAP_ACPI_DIS | \
+SCU_AST2400_HW_STRAP_SET_CLK_SOURCE(AST2400_CLK_48M_IN) | \
+SCU_HW_STRAP_VGA_CLASS_CODE | \
+SCU_HW_STRAP_LPC_RESET_PIN |\
+SCU_HW_STRAP_SPI_MODE(SCU_HW_STRAP_SPI_M_S_EN) |\
+SCU_AST2400_HW_STRAP_SET_CPU_AHB_RATIO(AST2400_CPU_AHB_RATIO_2_1) | \
+SCU_HW_STRAP_SPI_WIDTH |\
+SCU_HW_STRAP_VGA_SIZE_SET(VGA_16M_DRAM) | \
+SCU_AST2400_HW_STRAP_BOOT_MODE(AST2400_SPI_BOOT))
+
/* AST2500 evb hardware value: 0xF100C2E6 */
#define AST2500_EVB_HW_STRAP1 ((\
AST2500_HW_STRAP1_DEFAULTS |\
@@ -603,6 +617,23 @@ static void aspeed_machine_palmetto_class_init(ObjectClass
*oc, void *data)
aspeed_soc_num_cpus(amc->soc_name);
};
+static void aspeed_machine_supermicrox11_bmc_class_init(ObjectClass *oc,
+void *data)
+{
+MachineClass *mc = MACHINE_CLASS(oc);
+AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
+
+mc->desc = "Supermicro X11 BMC (ARM926EJ-S)";
+amc->soc_name = "ast2400-a1";
+amc->hw_strap1 = SUPERMICROX11_BMC_HW_STRAP1;
+amc->fmc_model = "mx25l25635e";
+amc->spi_model = "mx25l25635e";
+amc->num_cs= 1;
+amc->macs_mask = ASPEED_MAC0_ON | ASPEED_MAC1_ON;
+amc->i2c_init = palmetto_bmc_i2c_init;
+mc->default_ram_size = 256 * MiB;
+}
+
static void aspeed_machine_ast2500_evb_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
@@ -731,6 +762,10 @@ static const TypeInfo aspeed_machine_types[] = {
.name = MACHINE_TYPE_NAME("palmetto-bmc"),
.parent= TYPE_ASPEED_MACHINE,
.class_init= aspeed_machine_palmetto_class_init,
+}, {
+.name = MACHINE_TYPE_NAME("supermicrox11-bmc"),
+.parent= TYPE_ASPEED_MACHINE,
+.class_init= aspeed_machine_supermicrox11_bmc_class_init,
}, {
.name = MACHINE_TYPE_NAME("ast2500-evb"),
.parent= TYPE_ASPEED_MACHINE,
--
2.25.4
[PATCH v2 11/21] ftgmac100: Fix interrupt status "Packet moved to RX FIFO"
As we don't model the RX or TX FIFO, raise the "Packet moved to RX
FIFO" interrupt status bit as soon as we are handling a RX packet.
Cc: Frederic Konrad
Reviewed-by: Joel Stanley
Signed-off-by: Cédric Le Goater
---
hw/net/ftgmac100.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index aa3c05ef9882..5c0fe2d8cb75 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -950,6 +950,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const
uint8_t *buf,
break;
}
+s->isr |= FTGMAC100_INT_RPKT_FIFO;
addr = s->rx_descriptor;
while (size > 0) {
if (!ftgmac100_can_receive(nc)) {
@@ -1001,8 +1002,6 @@ static ssize_t ftgmac100_receive(NetClientState *nc,
const uint8_t *buf,
/* Last buffer in frame. */
bd.des0 |= flags | FTGMAC100_RXDES0_LRS;
s->isr |= FTGMAC100_INT_RPKT_BUF;
-} else {
-s->isr |= FTGMAC100_INT_RPKT_FIFO;
}
ftgmac100_write_bd(&bd, addr);
if (bd.des0 & s->rxdes0_edorr) {
--
2.25.4
[PATCH v2 06/21] aspeed/smc: Fix MemoryRegionOps definition
Unaligned access support is a leftover from the initial commit. There
is no such need on this device register mapping. Remove it.
Cc: Michael S. Tsirkin
Reviewed-by: Joel Stanley
Signed-off-by: Cédric Le Goater
---
hw/ssi/aspeed_smc.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 4fab1f5f855e..0646e0dca72e 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -1299,10 +1299,8 @@ static const MemoryRegionOps aspeed_smc_ops = {
.read = aspeed_smc_read,
.write = aspeed_smc_write,
.endianness = DEVICE_LITTLE_ENDIAN,
-.valid.unaligned = true,
};
-
/*
* Initialize the custom address spaces for DMAs
*/
--
2.25.4
[PATCH v2 17/21] aspeed/sdmc: Allow writes to unprotected registers
From: Joel Stanley
A subset of registers are not protected by the lock behaviour, so allow
unconditionally writing to those.
Signed-off-by: Joel Stanley
Signed-off-by: Cédric Le Goater
---
hw/misc/aspeed_sdmc.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index ff2809a09965..81c73450ab5d 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -33,15 +33,28 @@
/* Configuration Register */
#define R_CONF(0x04 / 4)
+/* Interrupt control/status */
+#define R_ISR (0x50 / 4)
+
/* Control/Status Register #1 (ast2500) */
#define R_STATUS1 (0x60 / 4)
#define PHY_BUSY_STATE BIT(0)
#define PHY_PLL_LOCK_STATUS BIT(4)
+/* Reserved */
+#define R_MCR6C (0x6c / 4)
+
#define R_ECC_TEST_CTRL (0x70 / 4)
#define ECC_TEST_FINISHED BIT(12)
#define ECC_TEST_FAIL BIT(13)
+#define R_TEST_START_LEN (0x74 / 4)
+#define R_TEST_FAIL_DQ(0x78 / 4)
+#define R_TEST_INIT_VAL (0x7c / 4)
+#define R_DRAM_SW (0x88 / 4)
+#define R_DRAM_TIME (0x8c / 4)
+#define R_ECC_ERR_INJECT (0xb4 / 4)
+
/*
* Configuration register Ox4 (for Aspeed AST2400 SOC)
*
@@ -449,6 +462,20 @@ static uint32_t
aspeed_2600_sdmc_compute_conf(AspeedSDMCState *s, uint32_t data)
static void aspeed_2600_sdmc_write(AspeedSDMCState *s, uint32_t reg,
uint32_t data)
{
+/* Unprotected registers */
+switch (reg) {
+case R_ISR:
+case R_MCR6C:
+case R_TEST_START_LEN:
+case R_TEST_FAIL_DQ:
+case R_TEST_INIT_VAL:
+case R_DRAM_SW:
+case R_DRAM_TIME:
+case R_ECC_ERR_INJECT:
+s->regs[reg] = data;
+return;
+}
+
if (s->regs[R_PROT] == PROT_HARDLOCKED) {
qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked until system
reset!\n",
__func__);
--
2.25.4
[PATCH v2 12/21] ftgmac100: Change interrupt status when a DMA error occurs
The model uses today the "Normal priority transmit buffer unavailable"
interrupt status which it is not appropriate. According to the Aspeed
specs, no interrupts are raised in that case. An "AHB error" status
seems like a better modeling choice for all implementations since it
is covered by the Linux kernel.
Cc: Frederic Konrad
Reviewed-by: Joel Stanley
Signed-off-by: Cédric Le Goater
---
hw/net/ftgmac100.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 5c0fe2d8cb75..014980d30aca 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -517,7 +517,7 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t
tx_ring,
if (dma_memory_read(&address_space_memory, bd.des3, ptr, len)) {
qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to read packet @
0x%x\n",
__func__, bd.des3);
-s->isr |= FTGMAC100_INT_NO_NPTXBUF;
+s->isr |= FTGMAC100_INT_AHB_ERR;
break;
}
--
2.25.4
[PATCH v2 15/21] ftgmac100: Improve software reset
The software reset of the MAC needs a finer granularity. Some settings
in MACCR are kept.
Cc: Frederic Konrad
Fixes: bd44300d1afc ("net: add FTGMAC100 support")
Signed-off-by: Cédric Le Goater
---
hw/net/ftgmac100.c | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 7c9fa720df03..782ff192cedc 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -649,10 +649,8 @@ static uint32_t ftgmac100_rxpoll(FTGMAC100State *s)
return cnt / div[speed];
}
-static void ftgmac100_reset(DeviceState *d)
+static void ftgmac100_do_reset(FTGMAC100State *s, bool sw_reset)
{
-FTGMAC100State *s = FTGMAC100(d);
-
/* Reset the FTGMAC100 */
s->isr = 0;
s->ier = 0;
@@ -671,7 +669,12 @@ static void ftgmac100_reset(DeviceState *d)
s->fear1 = 0;
s->tpafcr = 0xf1;
-s->maccr = 0;
+if (sw_reset) {
+s->maccr &= FTGMAC100_MACCR_GIGA_MODE | FTGMAC100_MACCR_FAST_MODE;
+} else {
+s->maccr = 0;
+}
+
s->phycr = 0;
s->phydata = 0;
s->fcr = 0x400;
@@ -680,6 +683,11 @@ static void ftgmac100_reset(DeviceState *d)
phy_reset(s);
}
+static void ftgmac100_reset(DeviceState *d)
+{
+ftgmac100_do_reset(FTGMAC100(d), false);
+}
+
static uint64_t ftgmac100_read(void *opaque, hwaddr addr, unsigned size)
{
FTGMAC100State *s = FTGMAC100(opaque);
@@ -824,7 +832,7 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
case FTGMAC100_MACCR: /* MAC Device control */
s->maccr = value;
if (value & FTGMAC100_MACCR_SW_RST) {
-ftgmac100_reset(DEVICE(s));
+ftgmac100_do_reset(s, true);
}
if (ftgmac100_can_receive(qemu_get_queue(s->nic))) {
--
2.25.4
[PATCH v2 10/21] ftgmac100: Fix interrupt status "Packet transmitted on ethernet"
The second field of the TX descriptor has a set of flags to choose
when the transmit interrupt is raised : after the packet has been sent
on the ethernet or after it has been moved into the TX FIFO. But we
don't model that today.
Simply raise the "Packet transmitted on ethernet" interrupt status bit
as soon as the packet is sent by QEMU.
Cc: Frederic Konrad
Reviewed-by: Joel Stanley
Signed-off-by: Cédric Le Goater
---
hw/net/ftgmac100.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 0348fcf45676..aa3c05ef9882 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -547,9 +547,7 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t
tx_ring,
qemu_send_packet(qemu_get_queue(s->nic), s->frame, frame_size);
ptr = s->frame;
frame_size = 0;
-if (flags & FTGMAC100_TXDES1_TXIC) {
-s->isr |= FTGMAC100_INT_XPKT_ETH;
-}
+s->isr |= FTGMAC100_INT_XPKT_ETH;
}
if (flags & FTGMAC100_TXDES1_TX2FIC) {
--
2.25.4
[PATCH v2 14/21] ftgmac100: Fix integer overflow in ftgmac100_do_tx()
When inserting the VLAN tag in packets, memmove() can generate an
integer overflow for packets whose length is less than 12 bytes.
Move the VLAN insertion when the last segment of the frame is reached
and check length against the size of the ethernet header (14 bytes) to
avoid the crash. Return FTGMAC100_INT_XPKT_LOST status if the frame is
too small. This seems like a good modeling choice even if Aspeed does
not specify anything in that case.
Cc: Frederic Konrad
Cc: Mauro Matteo Cascella
Reported-by: Ziming Zhang
Signed-off-by: Cédric Le Goater
---
hw/net/ftgmac100.c | 55 --
1 file changed, 39 insertions(+), 16 deletions(-)
diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 280aa3d3a1e2..7c9fa720df03 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -481,6 +481,37 @@ static int ftgmac100_write_bd(FTGMAC100Desc *bd,
dma_addr_t addr)
return 0;
}
+static int ftgmac100_insert_vlan(FTGMAC100State *s, int frame_size,
+ uint8_t vlan_tci)
+{
+uint8_t *vlan_hdr = s->frame + (ETH_ALEN * 2);
+uint8_t *payload = vlan_hdr + sizeof(struct vlan_header);
+
+if (frame_size < sizeof(struct eth_header)) {
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: frame too small for VLAN insertion : %d bytes\n",
+ __func__, frame_size);
+s->isr |= FTGMAC100_INT_XPKT_LOST;
+goto out;
+}
+
+if (frame_size + sizeof(struct vlan_header) > sizeof(s->frame)) {
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: frame too big : %d bytes\n",
+ __func__, frame_size);
+s->isr |= FTGMAC100_INT_XPKT_LOST;
+frame_size -= sizeof(struct vlan_header);
+}
+
+memmove(payload, vlan_hdr, frame_size - (ETH_ALEN * 2));
+stw_be_p(vlan_hdr, ETH_P_VLAN);
+stw_be_p(vlan_hdr + 2, vlan_tci);
+frame_size += sizeof(struct vlan_header);
+
+out:
+return frame_size;
+}
+
static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
uint32_t tx_descriptor)
{
@@ -530,25 +561,17 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t
tx_ring,
break;
}
-/* Check for VLAN */
-if (bd.des0 & FTGMAC100_TXDES0_FTS &&
-bd.des1 & FTGMAC100_TXDES1_INS_VLANTAG &&
-be16_to_cpu(PKT_GET_ETH_HDR(ptr)->h_proto) != ETH_P_VLAN) {
-if (frame_size + len + 4 > sizeof(s->frame)) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too big : %d
bytes\n",
- __func__, len);
-s->isr |= FTGMAC100_INT_XPKT_LOST;
-len = sizeof(s->frame) - frame_size - 4;
-}
-memmove(ptr + 16, ptr + 12, len - 12);
-stw_be_p(ptr + 12, ETH_P_VLAN);
-stw_be_p(ptr + 14, bd.des1);
-len += 4;
-}
-
ptr += len;
frame_size += len;
if (bd.des0 & FTGMAC100_TXDES0_LTS) {
+
+/* Check for VLAN */
+if (flags & FTGMAC100_TXDES1_INS_VLANTAG &&
+be16_to_cpu(PKT_GET_ETH_HDR(s->frame)->h_proto) != ETH_P_VLAN)
{
+frame_size = ftgmac100_insert_vlan(s, frame_size,
+
FTGMAC100_TXDES1_VLANTAG_CI(flags));
+}
+
if (flags & FTGMAC100_TXDES1_IP_CHKSUM) {
net_checksum_calculate(s->frame, frame_size);
}
--
2.25.4
[PATCH v2 19/21] aspeed/smc: Open AHB window of the second chip of the AST2600 FMC controller
This change works around the HW default values to be able to test the
Tacoma board with -kernel command line option. This was required when
we had both flash chips enabled in the device tree, otherwise Linux
would fail to probe the entire controller leaving it with no rootfs.
Reviewed-by: Joel Stanley
Signed-off-by: Cédric Le Goater
---
hw/ssi/aspeed_smc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 8c79a5552f93..795784e5f364 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -230,7 +230,7 @@ static void aspeed_smc_reg_to_segment(const AspeedSMCState
*s, uint32_t reg,
static const AspeedSegments aspeed_segments_ast2600_fmc[] = {
{ 0x0, 128 * MiB }, /* start address is readonly */
-{ 0x0, 0 }, /* disabled */
+{ 128 * MiB, 128 * MiB }, /* default is disabled but needed for -kernel */
{ 0x0, 0 }, /* disabled */
};
--
2.25.4
[PATCH v2 08/21] aspeed/sdhci: Fix reset sequence
BIT(0) of the ASPEED_SDHCI_INFO register is set by SW and polled until
the bit is cleared by HW.
Use the number of supported slots to define the default value of this
register (The AST2600 eMMC Controller only has one). Fix the reset
sequence by clearing automatically the RESET bit.
Cc: Eddie James
Fixes: 2bea128c3d0b ("hw/sd/aspeed_sdhci: New device")
Signed-off-by: Cédric Le Goater
---
hw/sd/aspeed_sdhci.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c
index 22cafce0fbdc..4f24b7d2f942 100644
--- a/hw/sd/aspeed_sdhci.c
+++ b/hw/sd/aspeed_sdhci.c
@@ -16,7 +16,9 @@
#include "hw/qdev-properties.h"
#define ASPEED_SDHCI_INFO0x00
-#define ASPEED_SDHCI_INFO_RESET 0x0003
+#define ASPEED_SDHCI_INFO_SLOT1 (1 << 17)
+#define ASPEED_SDHCI_INFO_SLOT0 (1 << 16)
+#define ASPEED_SDHCI_INFO_RESET (1 << 0)
#define ASPEED_SDHCI_DEBOUNCE0x04
#define ASPEED_SDHCI_DEBOUNCE_RESET 0x0005
#define ASPEED_SDHCI_BUS 0x08
@@ -67,6 +69,10 @@ static void aspeed_sdhci_write(void *opaque, hwaddr addr,
uint64_t val,
AspeedSDHCIState *sdhci = opaque;
switch (addr) {
+case ASPEED_SDHCI_INFO:
+/* The RESET bit automatically clears. */
+sdhci->regs[TO_REG(addr)] = (uint32_t)val & ~ASPEED_SDHCI_INFO_RESET;
+break;
case ASPEED_SDHCI_SDIO_140:
sdhci->slots[0].capareg = (uint64_t)(uint32_t)val;
break;
@@ -155,7 +161,11 @@ static void aspeed_sdhci_reset(DeviceState *dev)
AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
memset(sdhci->regs, 0, ASPEED_SDHCI_REG_SIZE);
-sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] = ASPEED_SDHCI_INFO_RESET;
+
+sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] = ASPEED_SDHCI_INFO_SLOT0;
+if (sdhci->num_slots == 2) {
+sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] |= ASPEED_SDHCI_INFO_SLOT1;
+}
sdhci->regs[TO_REG(ASPEED_SDHCI_DEBOUNCE)] = ASPEED_SDHCI_DEBOUNCE_RESET;
}
--
2.25.4
Re: [PATCH-for-5.2] memory: Add trace events to audit MemoryRegionOps fields
On 8/19/20 11:14 AM, Stefan Hajnoczi wrote: > On Tue, Aug 18, 2020 at 09:56:37AM +0200, Philippe Mathieu-Daudé wrote: >> On 8/18/20 8:32 AM, Paolo Bonzini wrote: >>> On 06/08/20 17:26, Philippe Mathieu-Daudé wrote: Add trace events to audit MemoryRegionOps field such: - are all the valid/impl fields provided? - is the region a power of two? These cases are accepted, but it is interesting to list them. Example: $ qemu-system-i386 -S -trace memory_region_io_check\* memory_region_io_check_odd_size mr name:'dma-page' size:0x3 >> >> (a) >> memory_region_io_check_access_size_incomplete mr name:'acpi-tmr' min/max:[valid:1/4 impl:4/0] memory_region_io_check_access_size_incomplete mr name:'acpi-evt' min/max:[valid:1/2 impl:2/0] memory_region_io_check_access_size_incomplete mr name:'acpi-cnt' min/max:[valid:1/2 impl:2/0] >> >> (b) >> >>> >>> Can they be detected using Coccinelle instead? >> >> For static declarations, probably. > > Regarding the MemoryRegionOps checks, the following grep command-line > shows that all MemoryRegionOps definitions are global: > > $ git grep 'MemoryRegionOps [^*]' hw/ > > This means static checking is possible. > >> >> (a) is not really fixable (because some datasheets don't >> count the reserved space in the device address map [1]), >> but is interesting to audit. >> >> I believe (b) has to be updated per maintainers preference, >> not by an individual developer. IIUC Michael said [2] while >> there is no bus information in MemoryRegionOps (and way to >> report a bus specific error), it is pointless to blindly fill >> the zero access sizes. >> >> Meanwhile I prefer to share my debugging helpers as trace >> events instead of ./configure --enable-maintainer and #ifdef'ry. > > Can they be turned into a CI check instead of debugging helpers? For > example, all existing violations are exempt but new MemoryRegionOps must > comply. This way it's not necessary to audit and fix everything right > now but code quality is improved in new code. > > Static checking is nice because there is no need to execute QEMU and > trigger all the code paths that lead to MemoryRegionOps usage. > > Here are more static checker ideas: > > 1. Rename memory_region_init_io() to memory_region_init_io_unsafe() (or >"legacy", "nocheck", etc) and then add a checkpatch.pl error if a new >memory_region_init_io_unsafe() call is added. > > 2. Walk the debuginfo looking for MemoryRegionOps structs and check >them. For example, a Python script that uses a DWARF parser. There >can be list of exempted source files that are allowed to violate the >rules (existing code). > > 3. Use __attribute__((__section__())) on MemoryRegionOps so they can >easily be located in ELF files and check them. For example, a C >program that opens the ELF and finds the "memoryregions" section. Thanks for the tips! >From the list 1. seems the easier to implement to me (no brain effort). But for now I'm not sure the check has to be enforced, because I'm not sure what we really want to do. First we need to figure out the 'bus' component of a a MemoryRegion (where it sits), as it affects the MemoryRegionOps possible range values. > > Stefan >
[PATCH v2 20/21] arm: aspeed: add strap define `25HZ` of AST2500
From: Igor Kononenko Provide a definition for the "25Hz reference clock input mode" strap Signed-off-by: Igor Kononenko Reviewed-by: Cédric Le Goater Message-Id: <20200811203502.20382-1-i.konone...@yadro.com> Signed-off-by: Cédric Le Goater --- include/hw/misc/aspeed_scu.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/hw/misc/aspeed_scu.h b/include/hw/misc/aspeed_scu.h index a6739bb846b6..9cd530afa23a 100644 --- a/include/hw/misc/aspeed_scu.h +++ b/include/hw/misc/aspeed_scu.h @@ -286,6 +286,7 @@ uint32_t aspeed_scu_get_apb_freq(AspeedSCUState *s); #define SCU_AST2500_HW_STRAP_ESPI_FLASH_ENABLE (0x1 << 26) #define SCU_AST2500_HW_STRAP_ESPI_ENABLE (0x1 << 25) #define SCU_AST2500_HW_STRAP_DDR4_ENABLE (0x1 << 24) +#define SCU_AST2500_HW_STRAP_25HZ_CLOCK_MODE (0x1 << 23) #define SCU_AST2500_HW_STRAP_ACPI_ENABLE (0x1 << 19) #define SCU_AST2500_HW_STRAP_USBCKI_FREQ (0x1 << 18) -- 2.25.4
[PATCH v2 09/21] ftgmac100: Fix registers that can be read
Receive Ring Base Address Register (RXR_BADR) and the Normal Priority Transmit Receive Ring Base Address Register (NPTXR_BADR) can also be read. Cc: Frederic Konrad Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Joel Stanley Signed-off-by: Cédric Le Goater --- hw/net/ftgmac100.c | 4 1 file changed, 4 insertions(+) diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c index 5f4b26fc5f3c..0348fcf45676 100644 --- a/hw/net/ftgmac100.c +++ b/hw/net/ftgmac100.c @@ -669,6 +669,10 @@ static uint64_t ftgmac100_read(void *opaque, hwaddr addr, unsigned size) return s->math[0]; case FTGMAC100_MATH1: return s->math[1]; +case FTGMAC100_RXR_BADR: +return s->rx_ring; +case FTGMAC100_NPTXR_BADR: +return s->tx_ring; case FTGMAC100_ITC: return s->itc; case FTGMAC100_DBLAC: -- 2.25.4
Re: [PATCH v6 2/4] copy-on-read: add filter append/drop functions
19.08.2020 00:24, Andrey Shinkevich wrote:
Provide API for the COR-filter insertion/removal.
Also, drop the filter child permissions for an inactive state when the
filter node is being removed.
Signed-off-by: Andrey Shinkevich
---
block/copy-on-read.c | 103 +++
block/copy-on-read.h | 36 ++
2 files changed, 139 insertions(+)
create mode 100644 block/copy-on-read.h
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index cb03e0f..150d9b7 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -23,11 +23,21 @@
#include "qemu/osdep.h"
#include "block/block_int.h"
#include "qemu/module.h"
+#include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
+#include "block/copy-on-read.h"
+
+
+typedef struct BDRVStateCOR {
+bool active;
+} BDRVStateCOR;
static int cor_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
+BDRVStateCOR *state = bs->opaque;
+
bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
false, errp);
@@ -42,6 +52,8 @@ static int cor_open(BlockDriverState *bs, QDict *options, int
flags,
((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
bs->file->bs->supported_zero_flags);
+state->active = true;
We don't need to call bdrv_child_refresh_perms() here, as permissions will be
updated soon, when the filter node get its parent, yes?
Let's add at least a comment on this.
+
return 0;
}
@@ -57,6 +69,17 @@ static void cor_child_perm(BlockDriverState *bs, BdrvChild *c,
uint64_t perm, uint64_t shared,
uint64_t *nperm, uint64_t *nshared)
{
+BDRVStateCOR *s = bs->opaque;
+
+if (!s->active) {
+/*
+ * While the filter is being removed
+ */
+*nperm = 0;
+*nshared = BLK_PERM_ALL;
+return;
+}
+
*nperm = perm & PERM_PASSTHROUGH;
*nshared = (shared & PERM_PASSTHROUGH) | PERM_UNCHANGED;
@@ -135,6 +158,7 @@ static void cor_lock_medium(BlockDriverState *bs, bool locked)
static BlockDriver bdrv_copy_on_read = {
.format_name= "copy-on-read",
+.instance_size = sizeof(BDRVStateCOR),
.bdrv_open = cor_open,
.bdrv_child_perm= cor_child_perm,
@@ -159,4 +183,83 @@ static void bdrv_copy_on_read_init(void)
bdrv_register(&bdrv_copy_on_read);
}
+
+static BlockDriverState *create_filter_node(BlockDriverState *bs,
+const char *filter_node_name,
+Error **errp)
+{
+QDict *opts = qdict_new();
+
+qdict_put_str(opts, "driver", "copy-on-read");
+qdict_put_str(opts, "file", bdrv_get_node_name(bs));
+if (filter_node_name) {
+qdict_put_str(opts, "node-name", filter_node_name);
+}
+
+return bdrv_open(NULL, NULL, opts, BDRV_O_RDWR, errp);
+}
+
+
+BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
+ const char *filter_node_name,
+ Error **errp)
+{
+BlockDriverState *cor_filter_bs;
+BDRVStateCOR *state;
+Error *local_err = NULL;
+
+cor_filter_bs = create_filter_node(bs, filter_node_name, errp);
+if (cor_filter_bs == NULL) {
+error_prepend(errp, "Could not create filter node: ");
+return NULL;
+}
+
+if (!filter_node_name) {
+cor_filter_bs->implicit = true;
+}
+
+bdrv_drained_begin(bs);
+bdrv_replace_node(bs, cor_filter_bs, &local_err);
+bdrv_drained_end(bs);
+
+if (local_err) {
+bdrv_unref(cor_filter_bs);
+error_propagate(errp, local_err);
+return NULL;
+}
+
+state = cor_filter_bs->opaque;
+state->active = true;
hmm stop. it's already active, after create_filter_node, so you don't need to
set it again, isn't it?
+
+return cor_filter_bs;
+}
+
+
+void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs)
+{
+BdrvChild *child;
+BlockDriverState *bs;
+BDRVStateCOR *s = cor_filter_bs->opaque;
+
+child = bdrv_filter_child(cor_filter_bs);
+if (!child) {
+return;
+}
+bs = child->bs;
+
+/* Retain the BDS until we complete the graph change. */
+bdrv_ref(bs);
+/* Hold a guest back from writing while permissions are being reset. */
+bdrv_drained_begin(bs);
+/* Drop permissions before the graph change. */
+s->active = false;
+bdrv_child_refresh_perms(cor_filter_bs, child, &error_abort);
+bdrv_replace_node(cor_filter_bs, bs, &error_abort);
+
+bdrv_drained_end(bs);
+bdrv_unref(bs);
+bdrv_unref(cor_filter_bs);
+}
+
+
block_init(bdrv_copy_on_read_init);
diff --git a/b
[PATCH v2 21/21] hw: add a number of SPI-flash's of m25p80 family
From: Igor Kononenko
Support a following SPI flashes:
* mx66l51235f
* mt25ql512ab
Signed-off-by: Igor Kononenko
Reviewed-by: Cédric Le Goater
Message-Id: <20200811203724.20699-1-i.konone...@yadro.com>
Signed-off-by: Cédric Le Goater
---
hw/block/m25p80.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 8a3fd959e218..d812e9fb6cfb 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -220,6 +220,7 @@ static const FlashPartInfo known_devices[] = {
{ INFO6("mx25l25635e", 0xc22019, 0xc22019, 64 << 10, 512, 0) },
{ INFO("mx25l25635f", 0xc22019, 0xc200, 64 << 10, 512, 0) },
{ INFO("mx25l25655e", 0xc22619, 0, 64 << 10, 512, 0) },
+{ INFO("mx66l51235f", 0xc2201a, 0, 64 << 10, 1024, ER_4K | ER_32K) },
{ INFO("mx66u51235f", 0xc2253a, 0, 64 << 10, 1024, ER_4K | ER_32K) },
{ INFO("mx66u1g45g", 0xc2253b, 0, 64 << 10, 2048, ER_4K | ER_32K) },
{ INFO("mx66l1g45g", 0xc2201b, 0, 64 << 10, 2048, ER_4K | ER_32K) },
@@ -239,6 +240,7 @@ static const FlashPartInfo known_devices[] = {
{ INFO("n25q256a",0x20ba19, 0, 64 << 10, 512, ER_4K) },
{ INFO("n25q512a",0x20ba20, 0, 64 << 10, 1024, ER_4K) },
{ INFO("n25q512ax3", 0x20ba20, 0x1000, 64 << 10, 1024, ER_4K) },
+{ INFO("mt25ql512ab", 0x20ba20, 0x1044, 64 << 10, 1024, ER_4K | ER_32K) },
{ INFO_STACKED("n25q00",0x20ba21, 0x1000, 64 << 10, 2048, ER_4K, 4) },
{ INFO_STACKED("n25q00a", 0x20bb21, 0x1000, 64 << 10, 2048, ER_4K, 4) },
{ INFO_STACKED("mt25ql01g", 0x20ba21, 0x1040, 64 << 10, 2048, ER_4K, 2) },
--
2.25.4
Re: [PATCH 00/18] hw/riscv: Add Microchip PolarFire SoC Icicle Kit board support
On 8/19/20 2:34 AM, Bin Meng wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On Tue, Aug 18, 2020 at 9:55 PM Anup Patel wrote: >> On Tue, Aug 18, 2020 at 6:39 PM wrote: >>> On 8/18/20 7:17 AM, Anup Patel wrote: EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe On Tue, Aug 18, 2020 at 1:23 AM wrote: > On 8/17/20 8:28 PM, Alistair Francis wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the content is safe >> >> On Mon, Aug 17, 2020 at 11:12 AM via wrote: >>> Hi Anup, >>> >>> On 8/17/20 11:30 AM, Bin Meng wrote: EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe Hi Anup, On Sat, Aug 15, 2020 at 1:44 AM Anup Patel wrote: > On Fri, Aug 14, 2020 at 10:12 PM Bin Meng wrote: >> From: Bin Meng >> >> This adds support for Microchip PolarFire SoC Icicle Kit board. >> The Icicle Kit board integrates a PolarFire SoC, with one SiFive's >> E51 plus four U54 cores and many on-chip peripherals and an FPGA. > Nice Work !!! This is very helpful. Thanks! > The Microchip HSS is quite convoluted. It has: > 1. DDR Init > 2. Boot device support > 3. SBI support using OpenSBI as library > 4. Simple TEE support > > I think point 1) and 2) above should be part of U-Boot SPL. > The point 3) can be OpenSBI FW_DYNAMIC. > > Lastly,for point 4), we are working on a new OpenSBI feature using > which we can run independent Secure OS and Non-Secure OS using > U-Boot_SPL+OpenSBI (for both SiFive Unleashed and Microchip > PolarFire). > > Do you have plans for adding U-Boot SPL support for this board ?? + Cyril Jean from Microchip I will have to leave this question to Cyril to comment. >>> I currently do not have a plan to support U-Boot SPL. The idea of the >>> HSS is to contain all the silicon specific initialization and >>> configuration code within the HSS before jumping to U-Boot S-mode. I >>> would rather keep all this within the HSS for the time being. I would >>> wait until we reach production silicon before attempting to move this to >>> U-Boot SPL as the HSS is likely to contain some opaque silicon related >>> changes for another while. >> That is unfortunate, a lot of work has gone into making the boot flow >> simple and easy to use. >> >> QEMU now includes OpenSBI by default to make it easy for users to boot >> Linux. The Icicle Kit board is now the most difficult QEMU board to >> boot Linux on. Not to mention it makes it hard to impossible to >> support it in standard tool flows such as meta-riscv. >> >> Alistair > If it is such a problem we can add a U-Boot SPL stage and the HSS can be > treated as standard SoC ROM code. It's not mandatory for U-Boot SPL to have stable DRAM calibration settings from the start itself. The initial U-Boot SPL support for most platforms (accross architectures) usually include basic working DRAM calibration settings which is later on updated with separate patches. Also, we don't need all U-Boot drivers to be upstreamed in one-go as this can be done in phases. The advantage we have with PolarFire SoC Icicle board is that we already have a U-Boot S-mode. and we believe the OpenSBI generic platform will work fine for PolarFire SoC Icicle board so the only thing missing right now is the U-Boot SPL support for OpenSource boot-flow. It will certainly accelerate open-source development if we have boot-flow U-Boot_SPL => OpenSBI (FW_DYNAMIC) => U-Boot_S-mode working on PolarFire SoC Icicle board. Initially, we just need DRAM, SD/eMMC, and Serial port support for U-Boot SPL and U-Boot S-mode. Later on, more patches can add ethernet and other booting device drivers to U-Boot. Regarding security services of HSS, we are working on a OpenSBI feature which will allow HSS security services to run as independent binary in M-mode (not linked to OpenSBI) and OpenSBI FW_DYNAMIC will be a separate binary acting as a secure monitor. Regards, Anup >>> What I have in mind is that the external memory will be up and running >>> by the time we get to U-Boot SPL. In the case of PolarFire SoC the ROM >>> code equivalent brings up the DDR memory interface so we do not need to >>> worry about this as part of U-Boot. >> Keeping DRAM configuration as part of a separate ROM booting stage prior >> to the U-Boot SPL sounds good to me. This will lead to following boot-flow: >> >> ROM/HSS (M-mode) => U-Boot SPL (M-mode)
Re: [PATCH v6 3/4] qapi: add filter-node-name to block-stream
19.08.2020 00:24, Andrey Shinkevich wrote:
Provide the possibility to pass the 'filter-node-name' parameter to the
block-stream job as it is done for the commit block job. That will be
needed for further iotests implementations.
and for users as well. I think, the last sentence may be dropped.
Signed-off-by: Andrey Shinkevich
---
block/monitor/block-hmp-cmds.c | 4 ++--
block/stream.c | 4 +++-
blockdev.c | 8 +++-
include/block/block_int.h | 7 ++-
qapi/block-core.json | 6 ++
5 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 4d3db5e..4e66775 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -507,8 +507,8 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
false, NULL, qdict_haskey(qdict, "speed"), speed, true,
- BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
- &error);
+ BLOCKDEV_ON_ERROR_REPORT, false, NULL, false, false,
false,
+ false, &error);
hmp_handle_error(mon, error);
}
diff --git a/block/stream.c b/block/stream.c
index b9c1141..8bf6b6d 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -221,7 +221,9 @@ static const BlockJobDriver stream_job_driver = {
void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, const char *backing_file_str,
int creation_flags, int64_t speed,
- BlockdevOnError on_error, Error **errp)
+ BlockdevOnError on_error,
+ const char *filter_node_name,
+ Error **errp)
{
StreamBlockJob *s;
BlockDriverState *iter;
diff --git a/blockdev.c b/blockdev.c
index 237fffb..800ecb3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2476,6 +2476,7 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
bool has_backing_file, const char *backing_file,
bool has_speed, int64_t speed,
bool has_on_error, BlockdevOnError on_error,
+ bool has_filter_node_name, const char *filter_node_name,
bool has_auto_finalize, bool auto_finalize,
bool has_auto_dismiss, bool auto_dismiss,
Error **errp)
@@ -2491,6 +2492,10 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
on_error = BLOCKDEV_ON_ERROR_REPORT;
}
+if (!has_filter_node_name) {
+filter_node_name = NULL;
+}
this works automatically, you don't need to initialize it by hand
+
bs = bdrv_lookup_bs(device, device, errp);
if (!bs) {
return;
@@ -2558,7 +2563,8 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
}
stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
- job_flags, has_speed ? speed : 0, on_error, &local_err);
+ job_flags, has_speed ? speed : 0, on_error,
+ filter_node_name, &local_err);
if (local_err) {
error_propagate(errp, local_err);
goto out;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 465a601..3efde33 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1122,6 +1122,9 @@ int is_windows_drive(const char *filename);
* See @BlockJobCreateFlags
* @speed: The maximum speed, in bytes per second, or 0 for unlimited.
* @on_error: The action to take upon error.
+ * @filter_node_name: The node name that should be assigned to the filter
+ * driver that the commit job inserts into the graph above @bs. NULL means
+ * that a node name should be autogenerated.
* @errp: Error object.
*
* Start a streaming operation on @bs. Clusters that are unallocated
@@ -1134,7 +1137,9 @@ int is_windows_drive(const char *filename);
void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, const char *backing_file_str,
int creation_flags, int64_t speed,
- BlockdevOnError on_error, Error **errp);
+ BlockdevOnError on_error,
+ const char *filter_node_name,
+ Error **errp);
/**
* commit_start:
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0b8ccd3..1db6ce1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2524,6 +2524,11 @@
#'stop' and 'enospc' can only be used if the block device
#supports io-status (see BlockInfo). Since 1.3.
#
+# @filter-node-name: the node name that should be assigned to the
+#filter driv
[PATCH V4] Introduce a new flag for i440fx to disable PCI hotplug on the root bus
We introduce a new global flag 'acpi-root-pci-hotplug' for i440fx with which
we can turn on or off PCI device hotplug on the root bus. This flag can be
used to prevent all PCI devices from getting hotplugged or unplugged from the
root PCI bus.
This feature is targetted mostly towards Windows VMs. It is useful in cases
where some hypervisor admins want to deploy guest VMs in a way so that the
users of the guest OSes are not able to hot-eject certain PCI devices from
the Windows system tray. Laine has explained the use case here in detail:
https://www.redhat.com/archives/libvir-list/2020-February/msg00110.html
Julia has resolved this issue for PCIE buses with the following commit:
530a0963184e57e71a5b538 ("pcie_root_port: Add hotplug disabling option")
This commit attempts to introduce similar behavior for PCI root buses used in
i440fx machine types (although in this case, we do not have a per-slot
capability
to turn hotplug on or off).
Usage:
-global PIIX4_PM.acpi-root-pci-hotplug=off
By default, this option is enabled which means that hotplug is turned on for the
PCI root bus.
The previously existing flag 'acpi-pci-hotplug-with-bridge-support' for PCI-PCI
bridges remain as is and can be used along with this new flag to control PCI
hotplug on PCI bridges.
This change has been tested using a Windows 2012R2 server guest image on a
Ubuntu
18.04 host using the latest master qemu from upstream.
Signed-off-by: Ani Sinha
---
hw/acpi/piix4.c | 8 ++--
hw/i386/acpi-build.c | 26 +++---
2 files changed, 25 insertions(+), 9 deletions(-)
Change Log:
V3..V4: Added use case detail in the commit log as per Igor's suggestion. Also
added an example of qemu commandline showing how to use the option.
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 26bac4f16c..4f436e5bf3 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -78,6 +78,7 @@ typedef struct PIIX4PMState {
AcpiPciHpState acpi_pci_hotplug;
bool use_acpi_hotplug_bridge;
+bool use_acpi_root_pci_hotplug;
uint8_t disable_s3;
uint8_t disable_s4;
@@ -595,8 +596,9 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion
*parent,
"acpi-gpe0", GPE_LEN);
memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
-acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
-s->use_acpi_hotplug_bridge);
+if (s->use_acpi_hotplug_bridge || s->use_acpi_root_pci_hotplug)
+acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
+s->use_acpi_hotplug_bridge);
s->cpu_hotplug_legacy = true;
object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
@@ -635,6 +637,8 @@ static Property piix4_pm_properties[] = {
DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState,
use_acpi_hotplug_bridge, true),
+DEFINE_PROP_BOOL("acpi-root-pci-hotplug", PIIX4PMState,
+ use_acpi_root_pci_hotplug, true),
DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
acpi_memory_hotplug.is_enabled, true),
DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b7bc2a..19a1702ad1 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -95,6 +95,7 @@ typedef struct AcpiPmInfo {
bool s3_disabled;
bool s4_disabled;
bool pcihp_bridge_en;
+bool pcihp_root_en;
uint8_t s4_val;
AcpiFadtData fadt;
uint16_t cpu_hp_io_base;
@@ -245,6 +246,9 @@ static void acpi_get_pm_info(MachineState *machine,
AcpiPmInfo *pm)
pm->pcihp_bridge_en =
object_property_get_bool(obj, "acpi-pci-hotplug-with-bridge-support",
NULL);
+pm->pcihp_root_en =
+object_property_get_bool(obj, "acpi-root-pci-hotplug", NULL);
+
}
static void acpi_get_misc_info(AcpiMiscInfo *info)
@@ -337,15 +341,18 @@ static void build_append_pcihp_notify_entry(Aml *method,
int slot)
}
static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
- bool pcihp_bridge_en)
+ bool pcihp_bridge_en,
+ bool pcihp_root_en)
{
Aml *dev, *notify_method = NULL, *method;
QObject *bsel;
PCIBus *sec;
int i;
+bool root_bus = pci_bus_is_root(bus);
+bool root_pcihp_disabled = (root_bus && !pcihp_root_en);
bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
NULL);
-if (bsel) {
+if (bsel && !root_pcihp_disabled) {
uint64_t bsel_val = qnum_get_uint(qobject_to(QNum, bsel));
aml_append(parent_scope, aml_name_decl("BSEL", aml_int(bsel_val)));
@@ -361,6 +368,9 @@ static void build_append_pci_bus_devices(Aml *parent_scope,
PCIBus *bus,
bool bridge_in_acpi;
if (!pdev
Re: [PATCH V2] Introduce a new flag for piix to disable root bus PCI hotplug
On Wed, Aug 19, 2020 at 3:30 PM Igor Mammedov wrote:
>
> On Tue, 11 Aug 2020 18:42:08 +0530
> Ani Sinha wrote:
>
> > We introduce a new global flag for PIIX with which we can turn on or off PCI
> > device hotplug on the root bus. This flag can be used to prevent all PCI
> > devices from getting hotplugged or unplugged from the root PCI bus.
>
> Tested-by: Igor Mammedov
>
> somewhere in intial versions there were mentionig why we are doing it
> (i.e for Windows sake because ...)
> I suggest to add it to commit message so reason for this won't be lost.
> Also giving example how to use option in commit message would be good.
>
> with this changes:
> Reviewed-by: Igor Mammedov
Thanks Igor, I have sent a V4 with your suggestions to the commit
message and some more
rework on the patch. The V4 has again been tested by me in the same
setup. Looks good so far.
>
>
>
> >
> > Signed-off-by: Ani Sinha
> > ---
> > hw/acpi/piix4.c | 3 +++
> > hw/i386/acpi-build.c | 20
> > 2 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index 26bac4f..94ec35a 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -78,6 +78,7 @@ typedef struct PIIX4PMState {
> >
> > AcpiPciHpState acpi_pci_hotplug;
> > bool use_acpi_hotplug_bridge;
> > +bool use_acpi_root_pci_hotplug;
> >
> > uint8_t disable_s3;
> > uint8_t disable_s4;
> > @@ -635,6 +636,8 @@ static Property piix4_pm_properties[] = {
> > DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
> > DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState,
> > use_acpi_hotplug_bridge, true),
> > +DEFINE_PROP_BOOL("acpi-root-pci-hotplug", PIIX4PMState,
> > + use_acpi_root_pci_hotplug, true),
> > DEFINE_PROP_BOOL("memory-hotplug-support", PIIX4PMState,
> > acpi_memory_hotplug.is_enabled, true),
> > DEFINE_PROP_END_OF_LIST(),
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index b7bcbbb..a82e5c1 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -95,6 +95,7 @@ typedef struct AcpiPmInfo {
> > bool s3_disabled;
> > bool s4_disabled;
> > bool pcihp_bridge_en;
> > +bool pcihp_root_en;
> > uint8_t s4_val;
> > AcpiFadtData fadt;
> > uint16_t cpu_hp_io_base;
> > @@ -245,6 +246,9 @@ static void acpi_get_pm_info(MachineState *machine,
> > AcpiPmInfo *pm)
> > pm->pcihp_bridge_en =
> > object_property_get_bool(obj,
> > "acpi-pci-hotplug-with-bridge-support",
> > NULL);
> > +pm->pcihp_root_en =
> > +object_property_get_bool(obj, "acpi-root-pci-hotplug", NULL);
> > +
> > }
> >
> > static void acpi_get_misc_info(AcpiMiscInfo *info)
> > @@ -337,12 +341,15 @@ static void build_append_pcihp_notify_entry(Aml
> > *method, int slot)
> > }
> >
> > static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> > - bool pcihp_bridge_en)
> > + bool pcihp_bridge_en,
> > + bool pcihp_root_en)
> > {
> > Aml *dev, *notify_method = NULL, *method;
> > QObject *bsel;
> > PCIBus *sec;
> > int i;
> > +bool root_bus = pci_bus_is_root(bus);
> > +bool root_pcihp_disabled = (root_bus && !pcihp_root_en);
> >
> > bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> > NULL);
> > if (bsel) {
> > @@ -361,6 +368,9 @@ static void build_append_pci_bus_devices(Aml
> > *parent_scope, PCIBus *bus,
> > bool bridge_in_acpi;
> >
> > if (!pdev) {
> > +/* skip if pci hotplug for the root bus is disabled */
> > +if (root_pcihp_disabled)
> > +continue;
> > if (bsel) { /* add hotplug slots for non present devices */
> > dev = aml_device("S%.02X", PCI_DEVFN(slot, 0));
> > aml_append(dev, aml_name_decl("_SUN", aml_int(slot)));
> > @@ -419,7 +429,7 @@ static void build_append_pci_bus_devices(Aml
> > *parent_scope, PCIBus *bus,
> > method = aml_method("_S3D", 0, AML_NOTSERIALIZED);
> > aml_append(method, aml_return(aml_int(s3d)));
> > aml_append(dev, method);
> > -} else if (hotplug_enabled_dev) {
> > +} else if (hotplug_enabled_dev && !root_pcihp_disabled) {
> > /* add _SUN/_EJ0 to make slot hotpluggable */
> > aml_append(dev, aml_name_decl("_SUN", aml_int(slot)));
> >
> > @@ -439,7 +449,8 @@ static void build_append_pci_bus_devices(Aml
> > *parent_scope, PCIBus *bus,
> > */
> > PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
> >
> > -build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en);
> > +build_append_pci_bus_devices(dev, sec_b
Re: [PATCH] tests: docker: support mxe-based mingw builds
On 19/08/20 10:20, Daniel P. Berrangé wrote:
> We already have docker containers with MXE based on Debian:
>
> debian-win32-cross.docker
> debian-win64-cross.docker
>
> your image uses a different naming convention, and puts both
> 32 and 64 bit in the same image.
Yeah, that's what test-mingw expects.
> I feel like we should have the Ubuntu variant follow the same
> structure and naming as the Debian variant for consistency.
My patch follows the Fedora variant, so that test-mingw runs. That
ensures that NSIS is covered as well. One possibility could be:
- create fedora-win*-cross dockerfiles
- add ENV FEATURES $FEATURES mingw to the win*-cross dockerfiles
- look for the feature in test-full and test-quick, and run "make
installer" if so.
- drop test-mingw completely, and adjust Patchew to use
docker-test-quick@fedora-win{32,64}-cross instead
Paolo
Re: [PATCH v6 4/4] block: apply COR-filter to block-stream jobs
19.08.2020 00:24, Andrey Shinkevich wrote:
The patch completes the series with the COR-filter insertion to any
block-stream operation. It also makes changes to the iotests 030.
The test case 'test_stream_parallel' was deleted due to multiple
errors.
"case deleted due to errors" is a bad reasoning.
If you remove the case, you should give detailed explanation, why it is
removed, what is the problem with it, what are the consequences, what is
not supported anymore.
Also, good to note here, that adding a filter here makes possible to
implement discarding already copied regions from backing files during
the job, to reduce disk over-usage.
Signed-off-by: Andrey Shinkevich
---
block/stream.c | 76 --
tests/qemu-iotests/030 | 50 +++---
tests/qemu-iotests/030.out | 4 +--
3 files changed, 61 insertions(+), 69 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index 8bf6b6d..0b11979 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -19,6 +19,7 @@
#include "qapi/qmp/qerror.h"
#include "qemu/ratelimit.h"
#include "sysemu/block-backend.h"
+#include "block/copy-on-read.h"
enum {
/*
@@ -33,8 +34,11 @@ typedef struct StreamBlockJob {
BlockJob common;
BlockDriverState *base_overlay; /* COW overlay (stream from this) */
BlockDriverState *above_base; /* Node directly above the base */
+BlockDriverState *cor_filter_bs;
+BlockDriverState *target_bs;
BlockdevOnError on_error;
char *backing_file_str;
+char *base_fmt;
bool bs_read_only;
bool chain_frozen;
} StreamBlockJob;
@@ -53,34 +57,26 @@ static void stream_abort(Job *job)
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
if (s->chain_frozen) {
-BlockJob *bjob = &s->common;
-bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->above_base);
+bdrv_unfreeze_backing_chain(s->cor_filter_bs, s->above_base);
}
}
static int stream_prepare(Job *job)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
-BlockJob *bjob = &s->common;
-BlockDriverState *bs = blk_bs(bjob->blk);
+BlockDriverState *bs = s->target_bs;
BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
Error *local_err = NULL;
int ret = 0;
-bdrv_unfreeze_backing_chain(bs, s->above_base);
+bdrv_unfreeze_backing_chain(s->cor_filter_bs, s->above_base);
s->chain_frozen = false;
if (bdrv_cow_child(unfiltered_bs)) {
-const char *base_id = NULL, *base_fmt = NULL;
-if (base) {
-base_id = s->backing_file_str;
-if (base->drv) {
-base_fmt = base->drv->format_name;
-}
-}
bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
-ret = bdrv_change_backing_file(unfiltered_bs, base_id, base_fmt);
+ret = bdrv_change_backing_file(unfiltered_bs, s->backing_file_str,
+ s->base_fmt);
if (local_err) {
error_report_err(local_err);
return -EPERM;
@@ -94,7 +90,9 @@ static void stream_clean(Job *job)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockJob *bjob = &s->common;
-BlockDriverState *bs = blk_bs(bjob->blk);
+BlockDriverState *bs = s->target_bs;
+
+bdrv_cor_filter_drop(s->cor_filter_bs);
/* Reopen the image back in read-only mode if necessary */
if (s->bs_read_only) {
@@ -104,13 +102,14 @@ static void stream_clean(Job *job)
}
g_free(s->backing_file_str);
+g_free(s->base_fmt);
}
static int coroutine_fn stream_run(Job *job, Error **errp)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockBackend *blk = s->common.blk;
-BlockDriverState *bs = blk_bs(blk);
+BlockDriverState *bs = s->target_bs;
BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
bool enable_cor = !bdrv_cow_child(s->base_overlay);
int64_t len;
@@ -231,6 +230,12 @@ void stream_start(const char *job_id, BlockDriverState *bs,
int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
BlockDriverState *base_overlay = bdrv_find_overlay(bs, base);
BlockDriverState *above_base;
+BlockDriverState *cor_filter_bs = NULL;
+char *base_fmt = NULL;
+
+if (base && base->drv) {
+base_fmt = g_strdup(base->drv->format_name);
+}
if (!base_overlay) {
error_setg(errp, "'%s' is not in the backing chain of '%s'",
@@ -264,17 +269,36 @@ void stream_start(const char *job_id, BlockDriverState
*bs,
}
}
-/* Prevent concurrent jobs trying to modify the graph structure here, we
- * already have our own plans. Also don't allow resize as the image size is
- * queried only at the
Re: [PULL v4 000/150] Meson-based build system
On Wed, 19 Aug 2020 at 10:49, Paolo Bonzini wrote: > > The following changes since commit d0ed6a69d399ae193959225cdeaa9382746c91cc: > > Update version for v5.1.0 release (2020-08-11 17:07:03 +0100) > > are available in the Git repository at: > > https://gitlab.com/bonzini/qemu.git tags/for-upstream > > for you to fetch changes up to ab7ac9b093f9d6d7878028b87ad68f70c923e180: > > docs: convert build system documentation to rST (2020-08-19 05:22:55 -0400) > > v3->v4: fixed aarch32 and MXE builds > > The MXE failure seems to be compounded by a Make bug, that causes it not > to obey private target-specific variables. Fortunately it can be avoided > easily, in fact while simplifying a tiny bit the ninja2make converter. > > I do wish this was found earlier than last minute, at the same time it's > an easy fix and the fixed version rules has really trivial rules; thus, > it follows even more the principle that one should never need to look > at Makefile.ninja. If this alternative implementation had come to my > mind, I would definitely have chosen it from the beginning. Hi; this fails to link in the clang sanitizer build; configure rune: '../../configure' '--skip-meson' '--skip-meson' '--cc=clang' '--cxx=clang++' '--enable-gtk' '--extra-cflags=-fsanitize=undefined -fno-sanitize=shift-base -Werror' "$@" (Side note: where have those two --skip-meson arguments come from? I never passed them to configure.) Link failures look like Linking target qemu-alpha libcommon.fa.p/disas_alpha.c.o: In function `extract_za': /home/petmay01/linaro/qemu-for-merges/build/clang/../../disas/alpha.c:483: undefined reference to `__ubsan_handle_type_mismatch_v1' libcommon.fa.p/disas_alpha.c.o: In function `extract_zb': /home/petmay01/linaro/qemu-for-merges/build/clang/../../disas/alpha.c:498: undefined reference to `__ubsan_handle_type_mismatch_v1' libcommon.fa.p/disas_alpha.c.o: In function `extract_zc': /home/petmay01/linaro/qemu-for-merges/build/clang/../../disas/alpha.c:513: undefined reference to `__ubsan_handle_type_mismatch_v1' libcommon.fa.p/disas_alpha.c.o: In function `extract_rba': /home/petmay01/linaro/qemu-for-merges/build/clang/../../disas/alpha.c:446: undefined reference to `__ubsan_handle_type_mismatch_v1' libcommon.fa.p/disas_alpha.c.o: In function `extract_rca': /home/petmay01/linaro/qemu-for-merges/build/clang/../../disas/alpha.c:465: undefined reference to `__ubsan_handle_type_mismatch_v1' libcommon.fa.p/disas_alpha.c.o:/home/petmay01/linaro/qemu-for-merges/build/clang/../../disas/alpha.c:524: more undefined references to `__ubsan_handle_type_mismatch_v1' follow Other links fail for the same error but for disas/i386.c or disas/cris.c. thanks -- PMM
Re: [PATCH] pc-bios: s390x: Only set lowcore iplb address on list-directed IPL
On 8/19/20 11:45 AM, Cornelia Huck wrote:
> On Wed, 19 Aug 2020 11:32:34 +0200
> Janosch Frank wrote:
>
>> On 8/17/20 7:51 PM, Jason J. Herne wrote:
>>> On 8/17/20 12:30 PM, Cornelia Huck wrote:
On Mon, 17 Aug 2020 10:17:34 -0400
"Jason J. Herne" wrote:
> The POP states that the IPLB location is only written to 0x14 for
> list-directed IPL. Some operating systems expect 0x14 to not change on
> boot and will fail IPL if it does change.
>
> Fixes: 9bfc04f9ef6802fff0
Should be
Fixes: 9bfc04f9ef68 ("pc-bios: s390x: Save iplb location in lowcore")
> Signed-off-by: Jason J. Herne
> Reviewed-by: Janosch Frank
> ---
> pc-bios/s390-ccw/jump2ipl.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index 767012bf0c..5e3e13f4b0 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -33,7 +33,10 @@ void jump_to_IPL_code(uint64_t address)
> {
> /* store the subsystem information _after_ the bootmap was loaded */
> write_subsystem_identification();
> -write_iplb_location();
> +
> +if (iplb.pbt != S390_IPL_TYPE_CCW) {
> +write_iplb_location();
> +}
What happens for ipl types other than CCW and FCP? IOW, should that
rather be a positive check for S390_IPL_TYPE_FCP?
>
> /* prevent unknown IPL types in the guest */
> if (iplb.pbt == S390_IPL_TYPE_QEMU_SCSI) {
>>>
>>> Based on my (admittedly limited) understanding of the architecture and
>>> code, I believe write_iplb_location() should be called at least for
>>> S390_IPL_TYPE_FCP but I'm not 100% sure on S390_IPL_TYPE_QEMU_SCSI.
>>> Perhaps Janosch has an idea?
>>>
>>> It was originally unconditional, and my new conditional excludes vfio
>>> CCW which is definitely a step in the right direction, in any case :).
>>
>> If I remember correctly the problem was that ZIPL used the IPLB lowcore
>> ptr without checking how it was booted (CCW or FCP). That was fixed in
>> mid of July by testing if diag308 gives back a config or not.
>
> So we have the problem that old zipl relies on the presence of a value
> that must not be there if you follow the architecture? Nasty.
>
> (Is it really "must not change" vs "don't expect anything here"? Not
> sure if I'm looking at the right part of the documentation.)
Well if the loaded program overwrites absolute 0x0, we shouldn't modify
it if we are not explicitly allowed to, no?
We already talked about saving the exception new addresses and restoring
them before jumping to the new kernel. I think we might need to go a
step further and use a non zero prefix for the bios to avoid any changes
to absolute 0x0.
However that wouldn't fix this dilemma.
>
>> So we might have a deadlock situation here which I need to think about
>> for a bit. I'm setting Viktor CC to get a bit more information about the
>> state of the zipl backports into the distros.
>
> Changing this is problematic unless everything we support as a guest is
> fixed. Does the guest go boom in a way that it is at least easy to
> figure out what went wrong? What breaks when the value continues to be
> set?
>
I think it goes into disabled wait because of the failed secure boot
verification or gets a PGM Addressing and then goes into disabled wait,
so no, not very user friendly.
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v7 27/47] blkverify: Use bdrv_sum_allocated_file_size()
Am 25.06.2020 um 17:21 hat Max Reitz geschrieben: > blkverify is a filter, so bdrv_get_allocated_file_size()'s default > implementation will return only the size of its filtered child. > However, because both of its children are disk images, it makes more > sense to sum both of their allocated sizes. Hm, but so are the children of, say, backup-top. I don't think you're suggesting that backup-top should add the sizes of both images, even though the backup job is actively increasing the allocated size of the non-primary node, much like blkverify. So I believe returning only the allocated size of the primary child in blkverify would be more consistent with what we do elsewhere. Kevin
[Bug 1886155] Re: error: argument 2 of ‘__atomic_load’ discards ‘const’ qualifier
Which means that given an argument of type T * const this defines a
local variable that is also T * const, and then tries to store the
result of the atomic load into that const variable:
```
#define atomic_rcu_read(ptr) \
({\
typeof_strip_qual(*ptr) _val; \
atomic_rcu_read__nocheck(ptr, &_val); \
_val; \
})
```
GCC 11 correctly diagnoses that write to a const variable.
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1886155
Title:
error: argument 2 of ‘__atomic_load’ discards ‘const’ qualifier
Status in QEMU:
New
Bug description:
GCC 11 reports the following errors:
[ 125s] In file included from
/home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/seqlock.h:17,
[ 125s] from
/home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/qht.h:10,
[ 125s] from
/home/abuild/rpmbuild/BUILD/qemu-5.0.0/util/qht.c:69:
[ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/util/qht.c: In function
'qht_do_lookup':
[ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:153:5:
error: argument 2 of '__atomic_load' discards 'const' qualifier
[-Werror=incompatible-pointer-types]
[ 125s] 153 | __atomic_load(ptr, valptr, __ATOMIC_RELAXED); \
[ 125s] | ^
[ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:161:5:
note: in expansion of macro 'atomic_rcu_read__nocheck'
[ 125s] 161 | atomic_rcu_read__nocheck(ptr, &_val); \
[ 125s] | ^~~~
[ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/util/qht.c:499:27: note: in
expansion of macro 'atomic_rcu_read'
[ 125s] 499 | void *p = atomic_rcu_read(&b->pointers[i]);
[ 125s] | ^~~
[ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:153:5:
error: argument 2 of '__atomic_load' discards 'const' qualifier
[-Werror=incompatible-pointer-types]
[ 125s] 153 | __atomic_load(ptr, valptr, __ATOMIC_RELAXED); \
[ 125s] | ^
[ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:161:5:
note: in expansion of macro 'atomic_rcu_read__nocheck'
[ 125s] 161 | atomic_rcu_read__nocheck(ptr, &_val); \
[ 125s] | ^~~~
[ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/util/qht.c:506:13: note: in
expansion of macro 'atomic_rcu_read'
[ 125s] 506 | b = atomic_rcu_read(&b->next);
[ 125s] | ^~~
[ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/util/qht.c: In function
'qht_lookup_custom':
[ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:153:5:
error: argument 2 of '__atomic_load' discards 'const' qualifier
[-Werror=incompatible-pointer-types]
[ 125s] 153 | __atomic_load(ptr, valptr, __ATOMIC_RELAXED); \
[ 125s] | ^
[ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:161:5:
note: in expansion of macro 'atomic_rcu_read__nocheck'
[ 125s] 161 | atomic_rcu_read__nocheck(ptr, &_val); \
[ 125s] | ^~~~
[ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/util/qht.c:534:11: note: in
expansion of macro 'atomic_rcu_read'
[ 125s] 534 | map = atomic_rcu_read(&ht->map);
[ 125s] | ^~~
[ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/util/qht.c: In function
'qht_statistics_init':
[ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:153:5:
error: argument 2 of '__atomic_load' discards 'const' qualifier
[-Werror=incompatible-pointer-types]
[ 125s] 153 | __atomic_load(ptr, valptr, __ATOMIC_RELAXED); \
[ 125s] | ^
[ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:161:5:
note: in expansion of macro 'atomic_rcu_read__nocheck'
[ 125s] 161 | atomic_rcu_read__nocheck(ptr, &_val); \
[ 125s] | ^~~~
[ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/util/qht.c:907:11: note: in
expansion of macro 'atomic_rcu_read'
[ 125s] 907 | map = atomic_rcu_read(&ht->map);
[ 125s] | ^~~
[ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:153:5:
error: argument 2 of '__atomic_load' discards 'const' qualifier
[-Werror=incompatible-pointer-types]
[ 125s] 153 | __atomic_load(ptr, valptr, __ATOMIC_RELAXED); \
[ 125s] | ^
[ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:161:5:
note: in expansion of macro 'atomic_rcu_read__nocheck'
[ 125s] 161 |
[Bug 1886155] Re: error: argument 2 of ‘__atomic_load’ discards ‘const’ qualifier
It looks like `typeof_strip_qual` doesn't work for pointer types. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1886155 Title: error: argument 2 of ‘__atomic_load’ discards ‘const’ qualifier Status in QEMU: New Bug description: GCC 11 reports the following errors: [ 125s] In file included from /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/seqlock.h:17, [ 125s] from /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/qht.h:10, [ 125s] from /home/abuild/rpmbuild/BUILD/qemu-5.0.0/util/qht.c:69: [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/util/qht.c: In function 'qht_do_lookup': [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:153:5: error: argument 2 of '__atomic_load' discards 'const' qualifier [-Werror=incompatible-pointer-types] [ 125s] 153 | __atomic_load(ptr, valptr, __ATOMIC_RELAXED); \ [ 125s] | ^ [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:161:5: note: in expansion of macro 'atomic_rcu_read__nocheck' [ 125s] 161 | atomic_rcu_read__nocheck(ptr, &_val); \ [ 125s] | ^~~~ [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/util/qht.c:499:27: note: in expansion of macro 'atomic_rcu_read' [ 125s] 499 | void *p = atomic_rcu_read(&b->pointers[i]); [ 125s] | ^~~ [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:153:5: error: argument 2 of '__atomic_load' discards 'const' qualifier [-Werror=incompatible-pointer-types] [ 125s] 153 | __atomic_load(ptr, valptr, __ATOMIC_RELAXED); \ [ 125s] | ^ [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:161:5: note: in expansion of macro 'atomic_rcu_read__nocheck' [ 125s] 161 | atomic_rcu_read__nocheck(ptr, &_val); \ [ 125s] | ^~~~ [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/util/qht.c:506:13: note: in expansion of macro 'atomic_rcu_read' [ 125s] 506 | b = atomic_rcu_read(&b->next); [ 125s] | ^~~ [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/util/qht.c: In function 'qht_lookup_custom': [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:153:5: error: argument 2 of '__atomic_load' discards 'const' qualifier [-Werror=incompatible-pointer-types] [ 125s] 153 | __atomic_load(ptr, valptr, __ATOMIC_RELAXED); \ [ 125s] | ^ [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:161:5: note: in expansion of macro 'atomic_rcu_read__nocheck' [ 125s] 161 | atomic_rcu_read__nocheck(ptr, &_val); \ [ 125s] | ^~~~ [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/util/qht.c:534:11: note: in expansion of macro 'atomic_rcu_read' [ 125s] 534 | map = atomic_rcu_read(&ht->map); [ 125s] | ^~~ [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/util/qht.c: In function 'qht_statistics_init': [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:153:5: error: argument 2 of '__atomic_load' discards 'const' qualifier [-Werror=incompatible-pointer-types] [ 125s] 153 | __atomic_load(ptr, valptr, __ATOMIC_RELAXED); \ [ 125s] | ^ [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:161:5: note: in expansion of macro 'atomic_rcu_read__nocheck' [ 125s] 161 | atomic_rcu_read__nocheck(ptr, &_val); \ [ 125s] | ^~~~ [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/util/qht.c:907:11: note: in expansion of macro 'atomic_rcu_read' [ 125s] 907 | map = atomic_rcu_read(&ht->map); [ 125s] | ^~~ [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:153:5: error: argument 2 of '__atomic_load' discards 'const' qualifier [-Werror=incompatible-pointer-types] [ 125s] 153 | __atomic_load(ptr, valptr, __ATOMIC_RELAXED); \ [ 125s] | ^ [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/include/qemu/atomic.h:161:5: note: in expansion of macro 'atomic_rcu_read__nocheck' [ 125s] 161 | atomic_rcu_read__nocheck(ptr, &_val); \ [ 125s] | ^~~~ [ 125s] /home/abuild/rpmbuild/BUILD/qemu-5.0.0/util/qht.c:941:21: note: in expansion of macro 'atomic_rcu_read' [ 125s] 941 | b = atomic_rcu_read(&b->next); [ 125s] | ^~~ To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1886155/+subscriptions
[PATCH 0/3] vhost-vsock: force virtio version 1
Recenlty changes in QEMU 5.1 requires to set 'disable-legacy=3Don' on vhost-vsock-pci and vhost-user-vsock-pci devices: $ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=3D5 qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=3D5: device is modern-only, use disable-legacy=3Don As discussed with Cornelia [1], this series force virtio version 1 to vhost-vsock-pci, vhost-user-vsock-pci, and vhost-vsock-ccw devices. virtio-vsock was introduced after the release of VIRTIO 1.0 specifications, so it should be 'modern-only'. In addition Cornelia verified that forcing a legacy mode on vhost-vsock-pci device using x86-64 host and s390x guest, so with different endianness, produces strange behaviours. About migration, QEMU 5.1 already requires that the source and destination specify 'disable-legacy=3Don', otherwise the migration fails in this way: qemu-system-x86_64: get_pci_config_device: Bad config data: i=3D0x2 read:= 12 device: 53 cmask: ff wmask: 0 w1cmask:0 qemu-system-x86_64: Failed to load PCIDevice:config qemu-system-x86_64: Failed to load virtio-vhost_vsock:virtio qemu-system-x86_64: error while loading state for instance 0x0 of device = ':00:03.0/virtio-vhost_vsock' qemu-system-x86_64: load of migration failed: Invalid argument With this series applied there is the same requirements. Alternatively we have to detach and re-attach the device manually. Thanks, Stefano [1] https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg02515.html Stefano Garzarella (3): vhost-vsock-pci: force virtio version 1 vhost-user-vsock-pci: force virtio version 1 vhost-vsock-ccw: force virtio version 1 hw/s390x/vhost-vsock-ccw.c | 2 ++ hw/virtio/vhost-user-vsock-pci.c | 2 +- hw/virtio/vhost-vsock-pci.c | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) --=20 2.26.2
[PATCH 2/3] vhost-user-vsock-pci: force virtio version 1
Commit 9b3a35ec82 ("virtio: verify that legacy support is not
accidentally on") added a safety checks that requires to set
'disable-legacy=on' on vhost-user-vsock-pci device:
$ ./qemu-system-x86_64 ... \
-chardev socket,id=char0,reconnect=0,path=/tmp/vhost4.socket \
-device vhost-user-vsock-pci,chardev=char0
qemu-system-x86_64: -device vhost-user-vsock-pci,chardev=char0:
device is modern-only, use disable-legacy=on
virtio-vsock was introduced after the release of VIRTIO 1.0
specifications, so it should be 'modern-only'.
This patch forces virtio version 1 and remove 'transitional_name'
properties, as done for vhost-vsock-pci, removing the need to specify
'disable-legacy=on' on vhost-user-vsock-pci device.
Cc: qemu-sta...@nongnu.org
Suggested-by: Cornelia Huck
Signed-off-by: Stefano Garzarella
---
hw/virtio/vhost-user-vsock-pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/virtio/vhost-user-vsock-pci.c b/hw/virtio/vhost-user-vsock-pci.c
index f4cf95873d..3e17cf0480 100644
--- a/hw/virtio/vhost-user-vsock-pci.c
+++ b/hw/virtio/vhost-user-vsock-pci.c
@@ -40,6 +40,7 @@ static void vhost_user_vsock_pci_realize(VirtIOPCIProxy
*vpci_dev, Error **errp)
VHostUserVSockPCI *dev = VHOST_USER_VSOCK_PCI(vpci_dev);
DeviceState *vdev = DEVICE(&dev->vdev);
+virtio_pci_force_virtio_1(vpci_dev);
qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
}
@@ -68,7 +69,6 @@ static void vhost_user_vsock_pci_instance_init(Object *obj)
static const VirtioPCIDeviceTypeInfo vhost_user_vsock_pci_info = {
.base_name = TYPE_VHOST_USER_VSOCK_PCI,
.generic_name = "vhost-user-vsock-pci",
-.transitional_name = "vhost-user-vsock-pci-transitional",
.non_transitional_name = "vhost-user-vsock-pci-non-transitional",
.instance_size = sizeof(VHostUserVSockPCI),
.instance_init = vhost_user_vsock_pci_instance_init,
--
2.26.2
[PATCH 3/3] vhost-vsock-ccw: force virtio version 1
virtio-vsock was introduced after the release of VIRTIO 1.0
specifications, so it should be 'modern-only'.
This patch forces virtio version 1 as done for vhost-vsock-pci.
Cc: qemu-sta...@nongnu.org
Suggested-by: Cornelia Huck
Signed-off-by: Stefano Garzarella
---
hw/s390x/vhost-vsock-ccw.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/s390x/vhost-vsock-ccw.c b/hw/s390x/vhost-vsock-ccw.c
index 0822ecca89..0759143efa 100644
--- a/hw/s390x/vhost-vsock-ccw.c
+++ b/hw/s390x/vhost-vsock-ccw.c
@@ -40,7 +40,9 @@ static void vhost_vsock_ccw_class_init(ObjectClass *klass,
void *data)
static void vhost_vsock_ccw_instance_init(Object *obj)
{
VHostVSockCCWState *dev = VHOST_VSOCK_CCW(obj);
+VirtioCcwDevice *ccw_dev = VIRTIO_CCW_DEVICE(obj);
+ccw_dev->force_revision_1 = true;
virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
TYPE_VHOST_VSOCK);
}
--
2.26.2
[PATCH 1/3] vhost-vsock-pci: force virtio version 1
Commit 9b3a35ec82 ("virtio: verify that legacy support is not
accidentally on") added a safety checks that requires to set
'disable-legacy=on' on vhost-vsock-pci device:
$ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5
qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=5:
device is modern-only, use disable-legacy=on
virtio-vsock was introduced after the release of VIRTIO 1.0
specifications, so it should be 'modern-only'.
In addition Cornelia verified that forcing a legacy mode on
vhost-vsock-pci device using x86-64 host and s390x guest, so with
different endianness, produces strange behaviours.
This patch forces virtio version 1 and remove 'transitional_name'
properties removing the need to specify 'disable-legacy=on' on
vhost-vsock-pci device.
Cc: qemu-sta...@nongnu.org
Reported-by: Qian Cai
Reported-by: Qinghua Cheng
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1868449
Suggested-by: Cornelia Huck
Signed-off-by: Stefano Garzarella
---
hw/virtio/vhost-vsock-pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/virtio/vhost-vsock-pci.c b/hw/virtio/vhost-vsock-pci.c
index a815278e69..d1fcad0472 100644
--- a/hw/virtio/vhost-vsock-pci.c
+++ b/hw/virtio/vhost-vsock-pci.c
@@ -44,6 +44,7 @@ static void vhost_vsock_pci_realize(VirtIOPCIProxy *vpci_dev,
Error **errp)
VHostVSockPCI *dev = VHOST_VSOCK_PCI(vpci_dev);
DeviceState *vdev = DEVICE(&dev->vdev);
+virtio_pci_force_virtio_1(vpci_dev);
qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
}
@@ -72,7 +73,6 @@ static void vhost_vsock_pci_instance_init(Object *obj)
static const VirtioPCIDeviceTypeInfo vhost_vsock_pci_info = {
.base_name = TYPE_VHOST_VSOCK_PCI,
.generic_name = "vhost-vsock-pci",
-.transitional_name = "vhost-vsock-pci-transitional",
.non_transitional_name = "vhost-vsock-pci-non-transitional",
.instance_size = sizeof(VHostVSockPCI),
.instance_init = vhost_vsock_pci_instance_init,
--
2.26.2
Re: [RFC PATCH 21/22] block/export: Move blk to BlockExport
On 13.08.20 18:29, Kevin Wolf wrote: > Every block export has a BlockBackend representing the disk that is > exported. It should live in BlockExport therefore. > > Signed-off-by: Kevin Wolf > --- > include/block/export.h | 3 +++ > block/export/export.c | 3 +++ > nbd/server.c | 44 ++ > 3 files changed, 29 insertions(+), 21 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [PATCH v7 25/47] block: Def. impl.s for get_allocated_file_size
Am 25.06.2020 um 17:21 hat Max Reitz geschrieben:
> If every BlockDriver were to implement bdrv_get_allocated_file_size(),
> there are basically three ways it would be handled:
> (1) For protocol drivers: Figure out the actual allocated file size in
> some protocol-specific way
> (2) For protocol drivers: If that is not possible (or we just have not
> bothered to implement it yet), return -ENOTSUP
> (3) For drivers with children: Return the sum of some or all their
> children's sizes
>
> For the drivers we have, case (3) boils down to either:
> (a) The sum of all children's sizes
> (b) The size of the primary child
>
> (2), (3a) and (3b) can be implemented generically, so this patch adds
> such generic implementations for drivers to use.
>
> Signed-off-by: Max Reitz
> ---
> include/block/block_int.h | 5
> block.c | 51 +++
> 2 files changed, 56 insertions(+)
>
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 5da793bfc3..c963ee9f28 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1318,6 +1318,11 @@ int coroutine_fn
> bdrv_co_block_status_from_backing(BlockDriverState *bs,
> int64_t *pnum,
> int64_t *map,
> BlockDriverState **file);
> +
> +int64_t bdrv_sum_allocated_file_size(BlockDriverState *bs);
> +int64_t bdrv_primary_allocated_file_size(BlockDriverState *bs);
> +int64_t bdrv_notsup_allocated_file_size(BlockDriverState *bs);
> +
> const char *bdrv_get_parent_name(const BlockDriverState *bs);
> void blk_dev_change_media_cb(BlockBackend *blk, bool load, Error **errp);
> bool blk_dev_has_removable_media(BlockBackend *blk);
> diff --git a/block.c b/block.c
> index 1c71ecab7c..fc01ce90b3 100644
> --- a/block.c
> +++ b/block.c
> @@ -5003,6 +5003,57 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState
> *bs)
> return -ENOTSUP;
> }
>
> +/**
> + * Implementation of BlockDriver.bdrv_get_allocated_file_size() for
> + * block drivers that want it to sum all children they store data on.
> + * (This excludes backing children.)
> + */
> +int64_t bdrv_sum_allocated_file_size(BlockDriverState *bs)
> +{
> +BdrvChild *child;
> +int64_t child_size, sum = 0;
> +
> +QLIST_FOREACH(child, &bs->children, next) {
> +if (child->role & (BDRV_CHILD_DATA | BDRV_CHILD_METADATA |
> + BDRV_CHILD_FILTERED))
> +{
> +child_size = bdrv_get_allocated_file_size(child->bs);
> +if (child_size < 0) {
> +return child_size;
> +}
> +sum += child_size;
> +}
> +}
> +
> +return sum;
> +}
The only user apart from bdrv_get_allocated_file_size() is blkverify. As
I argued that blkverify shouldn't use it, this can become static.
> +/**
> + * Implementation of BlockDriver.bdrv_get_allocated_file_size() for
> + * block drivers that want it to return only the size of a node's
> + * primary child.
> + */
> +int64_t bdrv_primary_allocated_file_size(BlockDriverState *bs)
> +{
> +BlockDriverState *primary_bs;
> +
> +primary_bs = bdrv_primary_bs(bs);
> +if (!primary_bs) {
> +return -ENOTSUP;
> +}
> +
> +return bdrv_get_allocated_file_size(primary_bs);
> +}
This can become static, too (never used as a callback), and possibly
even be inlined.
> +/**
> + * Implementation of BlockDriver.bdrv_get_allocated_file_size() for
> + * protocol block drivers that just do not support it.
> + */
> +int64_t bdrv_notsup_allocated_file_size(BlockDriverState *bs)
> +{
> +return -ENOTSUP;
> +}
Also never used as a callback. I think inlining it would almost
certainly make more sense.
Kevin
Re: [RFC PATCH 22/22] block/export: Add query-block-exports
On 13.08.20 18:29, Kevin Wolf wrote:
> This adds a simple QMP command to query the list of block exports.
>
> Signed-off-by: Kevin Wolf
> ---
> qapi/block-export.json | 33 +
> block/export/export.c | 23 +++
> 2 files changed, 56 insertions(+)
>
> diff --git a/qapi/block-export.json b/qapi/block-export.json
> index a067de2ba3..0b184bbd7c 100644
> --- a/qapi/block-export.json
> +++ b/qapi/block-export.json
> @@ -226,3 +226,36 @@
> ##
> { 'command': 'block-export-del',
>'data': { 'id': 'str', '*mode': 'BlockExportRemoveMode' } }
> +
> +##
> +# @BlockExportInfo:
> +#
> +# Information about a single block export.
> +#
> +# @id: The unique identifier for the block export
> +#
> +# @type: This field is returned only for compatibility reasons, it should
> +#not be used (always returns 'unknown')
Äh?
I don’t understand. It looks like it definitely doesn’t always return
“unknown”. Also, the “compatibility reasons” aren’t really immediately
clear to me... :?
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 01/11] hw/arm/virt-acpi-build:Remove dead assignment in build_madt()
On Thu, 13 Aug 2020 15:37:02 +0800 Chen Qun wrote: > Clang static code analyzer show warning: > hw/arm/virt-acpi-build.c:641:5: warning: Value stored to 'madt' is never read > madt = acpi_data_push(table_data, sizeof *madt); > ^ Reviewed-by: Igor Mammedov > > Reported-by: Euler Robot > Signed-off-by: Chen Qun > --- > Cc: Shannon Zhao > Cc: Peter Maydell > Cc: "Michael S. Tsirkin" > Cc: Igor Mammedov > Cc: qemu-...@nongnu.org > --- > hw/arm/virt-acpi-build.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 91f0df7b13..f830f9b779 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -633,12 +633,11 @@ build_madt(GArray *table_data, BIOSLinker *linker, > VirtMachineState *vms) > int madt_start = table_data->len; > const MemMapEntry *memmap = vms->memmap; > const int *irqmap = vms->irqmap; > -AcpiMultipleApicTable *madt; > AcpiMadtGenericDistributor *gicd; > AcpiMadtGenericMsiFrame *gic_msi; > int i; > > -madt = acpi_data_push(table_data, sizeof *madt); > +acpi_data_push(table_data, sizeof(AcpiMultipleApicTable)); > > gicd = acpi_data_push(table_data, sizeof *gicd); > gicd->type = ACPI_APIC_GENERIC_DISTRIBUTOR;
Re: [PATCH 0/3] vhost-vsock: force virtio version 1
On 19.08.20 12:51, Stefano Garzarella wrote: > Recenlty changes in QEMU 5.1 requires to set 'disable-legacy=3Don' > on vhost-vsock-pci and vhost-user-vsock-pci devices: > > $ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=3D5 > qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=3D5: > device is modern-only, use disable-legacy=3Don Something seemed to messed up your encoding (= vs. =3D). The patches look fine, though. (did you copy and paste this from somewhere?) > > As discussed with Cornelia [1], this series force virtio version 1 > to vhost-vsock-pci, vhost-user-vsock-pci, and vhost-vsock-ccw devices. > > virtio-vsock was introduced after the release of VIRTIO 1.0 specifications, > so it should be 'modern-only'. > In addition Cornelia verified that forcing a legacy mode on vhost-vsock-pci > device using x86-64 host and s390x guest, so with different endianness, > produces strange behaviours. > > About migration, QEMU 5.1 already requires that the source and destination > specify 'disable-legacy=3Don', otherwise the migration fails in this way: > qemu-system-x86_64: get_pci_config_device: Bad config data: i=3D0x2 read:= > 12 device: 53 cmask: ff wmask: 0 w1cmask:0 > qemu-system-x86_64: Failed to load PCIDevice:config > qemu-system-x86_64: Failed to load virtio-vhost_vsock:virtio > qemu-system-x86_64: error while loading state for instance 0x0 of device = > ':00:03.0/virtio-vhost_vsock' > qemu-system-x86_64: load of migration failed: Invalid argument > > With this series applied there is the same requirements. Alternatively > we have to detach and re-attach the device manually. > > Thanks, > Stefano > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg02515.html > > Stefano Garzarella (3): > vhost-vsock-pci: force virtio version 1 > vhost-user-vsock-pci: force virtio version 1 > vhost-vsock-ccw: force virtio version 1 > > hw/s390x/vhost-vsock-ccw.c | 2 ++ > hw/virtio/vhost-user-vsock-pci.c | 2 +- > hw/virtio/vhost-vsock-pci.c | 2 +- > 3 files changed, 4 insertions(+), 2 deletions(-) > > --=20 ^ also here > 2.26.2 > -- Thanks, David / dhildenb
Re: [PATCH] tests: docker: support mxe-based mingw builds
On Wed, Aug 19, 2020 at 12:42:44PM +0200, Paolo Bonzini wrote:
> On 19/08/20 10:20, Daniel P. Berrangé wrote:
> > We already have docker containers with MXE based on Debian:
> >
> > debian-win32-cross.docker
> > debian-win64-cross.docker
> >
> > your image uses a different naming convention, and puts both
> > 32 and 64 bit in the same image.
>
> Yeah, that's what test-mingw expects.
Ah, I missed that.
>
> > I feel like we should have the Ubuntu variant follow the same
> > structure and naming as the Debian variant for consistency.
>
> My patch follows the Fedora variant, so that test-mingw runs. That
> ensures that NSIS is covered as well. One possibility could be:
>
> - create fedora-win*-cross dockerfiles
Yeah, I think that'd make sense, as it'd enable a simple trick we
do in libvirt.
In all the dockerfile recipe we set an env
ENV MESON_OPTS "--cross-file=/usr/share/mingw/toolchain-mingw32.meson"
And in other linux-cross builds, we do similar
ENV MESON_OPTS "--cross-file=i686-linux-gnu"
So now from host side can just invoke "meson $MESON_OPTS" and it will
do the right thing according to whatever the container image was
installed with, regardless of whether it is a cross, or native build.
This obviously only works if you have separate images for win32
and win64.
> - add ENV FEATURES $FEATURES mingw to the win*-cross dockerfiles
>
> - look for the feature in test-full and test-quick, and run "make
> installer" if so.
I'd suggest that "make installer" should be a part of "make" not
a separate thing that needs running manually.
eg if we're configure'ing for mingw, configure should check whether
we have the NSIS tools available and if so, then enable NSIS as a
standard build output. We could have a configure option to enable/disable
NSIS explicitly.
This eliminates the second bit of special casing for mingw
> - drop test-mingw completely, and adjust Patchew to use
> docker-test-quick@fedora-win{32,64}-cross instead
Yes, dropping test-mingw would be better, but I'm not sure you can
use 'test-quick' as that runs unit tests which would require wine
to be present. 'test-build' would be closer to what test-mingw
does.
Anyway, if the images and make rules are created & setup in a suitable
way, there should be no need to do anything special in the host side
for cross-builds - just pick the image you want and it should "just work".
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] vhost-vsock: force virtio version 1
On Wed, Aug 19, 2020 at 01:06:55PM +0200, David Hildenbrand wrote: > On 19.08.20 12:51, Stefano Garzarella wrote: > > Recenlty changes in QEMU 5.1 requires to set 'disable-legacy=3Don' > > on vhost-vsock-pci and vhost-user-vsock-pci devices: > > > > $ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=3D5 > > qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=3D5: > > device is modern-only, use disable-legacy=3Don > > Something seemed to messed up your encoding (= vs. =3D). The patches > look fine, though. (did you copy and paste this from somewhere?) mmm, I used git-publish and I copied it from the first commint of the series. Re-opening the cover letter with git-publish everything looks okay, so maybe the issue happened when sending it... > > > > > As discussed with Cornelia [1], this series force virtio version 1 > > to vhost-vsock-pci, vhost-user-vsock-pci, and vhost-vsock-ccw devices. > > > > virtio-vsock was introduced after the release of VIRTIO 1.0 specifications, > > so it should be 'modern-only'. > > In addition Cornelia verified that forcing a legacy mode on vhost-vsock-pci > > device using x86-64 host and s390x guest, so with different endianness, > > produces strange behaviours. > > > > About migration, QEMU 5.1 already requires that the source and destination > > specify 'disable-legacy=3Don', otherwise the migration fails in this way: > > qemu-system-x86_64: get_pci_config_device: Bad config data: i=3D0x2 > > read:= > > 12 device: 53 cmask: ff wmask: 0 w1cmask:0 > > qemu-system-x86_64: Failed to load PCIDevice:config > > qemu-system-x86_64: Failed to load virtio-vhost_vsock:virtio > > qemu-system-x86_64: error while loading state for instance 0x0 of > > device = > > ':00:03.0/virtio-vhost_vsock' > > qemu-system-x86_64: load of migration failed: Invalid argument > > > > With this series applied there is the same requirements. Alternatively > > we have to detach and re-attach the device manually. > > > > Thanks, > > Stefano > > > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg02515.html > > > > Stefano Garzarella (3): > > vhost-vsock-pci: force virtio version 1 > > vhost-user-vsock-pci: force virtio version 1 > > vhost-vsock-ccw: force virtio version 1 > > > > hw/s390x/vhost-vsock-ccw.c | 2 ++ > > hw/virtio/vhost-user-vsock-pci.c | 2 +- > > hw/virtio/vhost-vsock-pci.c | 2 +- > > 3 files changed, 4 insertions(+), 2 deletions(-) > > > > --=20 > > ^ also here Everywhere :-( I'll check my configuration. Thanks for pointing that out, Stefano
Re: [PATCH v4 2/3] hw/i386: Update the EPYC topology to use socket/dies/core/thread model
On Fri, 14 Aug 2020 16:39:33 -0500
Babu Moger wrote:
> Update the EPYC topology to use socket/dies/core/thread model. The EPYC
> model does not use the smp dies to build the topology. Instead, it uses
> numa nodes to build the topology. Internally both are similar concept
> which divides the cores on L3 boundary. Combining both into one terminology
> makes it simple to program.
>
> Add a new check to error out when smp dies are not provided when EPYC
> model is numa configured. Next task is to remove node_id, nr_nodes and
> nodes_per_pkg from EPYC topology which will be done in next patch.
>
> Signed-off-by: Babu Moger
> ---
> hw/i386/x86.c |8
> 1 file changed, 8 insertions(+)
>
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 67bee1bcb8..e90c42d2fc 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -138,6 +138,14 @@ void x86_cpus_init(X86MachineState *x86ms, int
> default_cpu_version)
>
> /* Check for apicid encoding */
> if (cpu_x86_use_epyc_apic_id_encoding(ms->cpu_type)) {
> +if ((ms->numa_state->num_nodes > 0) &&
> +ms->numa_state->num_nodes != (ms->smp.sockets *
> x86ms->smp_dies)) {
this case is gated by (ms->numa_state->num_nodes > 0) so it won't work in case
-smp dies=>1 but there is no -numa options at all
we need to error out and ask to provide numa nodes corresponding to
(ms->numa_state->num_nodes == 0) && (ms->smp.sockets * x86ms->smp_dies)
or better alternative would be to enable autonuma when EPYC cpu is enabled,
that will insure that this patch will work even if user hasn't specified -numa
option,
since it will create a single numa node automatically.
The later will take care of (-smp 1,dies=1) case, which is broken due to
lack of explicit -numa we end up with CPU_UNSET_NUMA_NODE_ID in
CPUID_Fn801E_ECX.
> +error_setg(&error_fatal, "Numa configuration here requires smp "
> + "'dies' parameter. Configure the cpu topology
> properly "
> + "with max_cpus = sockets * dies * cores * threads.
> Dies"
> + " is equivalent to number of numa nodes in a
> socket.");
> +return;
> +}
> x86_set_epyc_topo_handlers(ms);
> }
>
>
Re: [PATCH] audio/jack: fix use after free segfault
On Mittwoch, 19. August 2020 00:20:07 CEST Geoffrey McRae wrote: > > Could you please describe in more detail how you ran into this > > situation with > > your 2nd audio device? > > Sure. Run a Windows guest with two audio devices, let it boot up, then > restart > the jack service to trigger the recovery routine, then attempt to use > the 2nd > (non-primary) audio device. Ie, go to windows audio settings to test the > microphone of the second audio device. > > When windows try to use the 2nd audio device it goes through the > recovery > routine triggering this fault. I still don't quite get how this correlates. So you are forcing a restart of jackd on host side in between, for what purpose? To simulate the Windows client being kicked by jackd? What latencies do you achieve BTW with Windows guests? > I am aware and since these libraries are interchangeable I had assumed > that > JACK1 will have the same fault. If not I suppose we need to detect which > is in > use and change this code appropriately. I haven't checked this in the JACK1 code base yet, but I assume JACK1 does not behave like JACK2 here, because the JACK API is very clear that it is the client's responsibility to free itself. So it looks like a JACK2-only-bug to me. Very weird that there is no jack_client_version() in the shared weak API (i.e. missing on JACK1 side). Best regards, Christian Schoenebeck
Re: [PATCH v5 1/5] block/io: fix bdrv_co_block_status_above
On Wed 19 Aug 2020 11:48:25 AM CEST, Vladimir Sementsov-Ogievskiy wrote:
>>> + * The top layer deferred to this layer, and because this
>>> layer is
>>> + * short, any zeroes that we synthesize beyond EOF behave as
>>> if they
>>> + * were allocated at this layer
>>>*/
>>> +assert(ret & BDRV_BLOCK_EOF);
>>> *pnum = bytes;
>>> +if (file) {
>>> +*file = p;
>>> +}
>>> +return BDRV_BLOCK_ZERO | BDRV_BLOCK_ALLOCATED;
>>
>> You don't add BDRV_BLOCK_EOF to the return code here ?
>
> No we shouldn't, as this is the end of backing file when the top layer
> is larger.
Ok, but maybe the original request also reaches EOF on the top layer.
An example that does not actually involve short backing files: let's say
that the size of the active image is 1MB and the backing file is 2MB.
We do 'write -z 960k 63k', that zeroes the last cluster minus a 1KB
tail, so qcow2_co_pwrite_zeroes() calls is_zero() to check if that last
1KB already contains zeroes.
bdrv_co_block_status_above() on the top layer returns BDRV_BLOCK_EOF: no
allocation, no zeros, we simply reached EOF. So we go to the backing
image to see what's there. This is also unallocated, there's no backing
file this time and want_zero is set, so this returns BDRV_BLOCK_ZERO.
However the backing file is longer so bdrv_co_block_status() does not
return BDRV_BLOCK_EOF this time.
If the backing file would have been the same size (1MB) we would have
BDRV_BLOCK_ZERO | BDRV_BLOCK_EOF, but if it's longer or (with your
patch) shorter then BDRV_BLOCK_EOF disappears.
Now, I don't see anyone else using BDRV_BLOCK_EOF for anything so this
does not have any practical effect at the moment, but is this worth
fixing?.
>>> +res = bdrv_block_status_above(bs, NULL, offset, bytes, &nr, NULL,
>>> NULL);
>>> +offset += nr;
>>> +bytes -= nr;
>>> +} while (res >= 0 && (res & BDRV_BLOCK_ZERO) && nr && bytes);
>>
>> About this last "... && nr && bytes", I think 'nr' already implies
>> 'bytes', maybe you want to use an assertion instead?
>
> No, on the last iteration, bytes would be 0 and nr is a last
> chunk. So, if we check only nr, we'll do one extra call of
> bdrv_block_status_above with bytes=0, I don't want do it.
You're right !
Berto
Re: [PATCH] tests: docker: support mxe-based mingw builds
On 19/08/20 13:09, Daniel P. Berrangé wrote: > On Wed, Aug 19, 2020 at 12:42:44PM +0200, Paolo Bonzini wrote: >> On 19/08/20 10:20, Daniel P. Berrangé wrote: >>> We already have docker containers with MXE based on Debian: >>> >>> debian-win32-cross.docker >>> debian-win64-cross.docker >>> >>> your image uses a different naming convention, and puts both >>> 32 and 64 bit in the same image. >> >> Yeah, that's what test-mingw expects. > > Ah, I missed that. > >> >>> I feel like we should have the Ubuntu variant follow the same >>> structure and naming as the Debian variant for consistency. >> >> My patch follows the Fedora variant, so that test-mingw runs. That >> ensures that NSIS is covered as well. One possibility could be: >> >> - create fedora-win*-cross dockerfiles > > Yeah, I think that'd make sense, as it'd enable a simple trick we > do in libvirt. > > In all the dockerfile recipe we set an env > > ENV MESON_OPTS "--cross-file=/usr/share/mingw/toolchain-mingw32.meson" > > And in other linux-cross builds, we do similar > > ENV MESON_OPTS "--cross-file=i686-linux-gnu" Yeah, that's the same as QEMU's QEMU_CONFIGURE_OPTS. >> - add ENV FEATURES $FEATURES mingw to the win*-cross dockerfiles >> >> - look for the feature in test-full and test-quick, and run "make >> installer" if so. > > I'd suggest that "make installer" should be a part of "make" not > a separate thing that needs running manually. > > eg if we're configure'ing for mingw, configure should check whether > we have the NSIS tools available and if so, then enable NSIS as a > standard build output. We could have a configure option to enable/disable > NSIS explicitly. > > This eliminates the second bit of special casing for mingw Yeah, that might be an idea. I'm not sure if it would work however because "make installer" invokes "make install", so there could be recursive (and non-reentrant) invocations. Paolo
Re: [PATCH] audio/jack: fix use after free segfault
On 2020-08-19 21:30, Christian Schoenebeck wrote: On Mittwoch, 19. August 2020 00:20:07 CEST Geoffrey McRae wrote: > Could you please describe in more detail how you ran into this > situation with > your 2nd audio device? Sure. Run a Windows guest with two audio devices, let it boot up, then restart the jack service to trigger the recovery routine, then attempt to use the 2nd (non-primary) audio device. Ie, go to windows audio settings to test the microphone of the second audio device. When windows try to use the 2nd audio device it goes through the recovery routine triggering this fault. I still don't quite get how this correlates. So you are forcing a restart of jackd on host side in between, for what purpose? To simulate the Windows client being kicked by jackd? For many reasons jack may need to be stopped and started again, such as hardware changes when switching to a USB audio device, or tuning the period size, etc. QEMU should be able to recover if the jack server goes away, it's that simple. The following sequence is what triggers this fault. client1 = jack_client_open(); client2 = jack_client_open(); client1 gets a shutdown signal jack_client_close(client1); client1 = jack_client_open(); client2 gets a shutdown signal jack_client_close(client2); client2 = jack_client_open(); One would expect this sequence to work fine as it conforms to the JACK documentation and common design practice, however, the call to `jack_client_open` notices that there is the 2nd session and frees it out from under the application. This has been resolved in the v5 patch as suggested by Gerd by scheduling a QEMUBH to perform the closures so they occur in order before an attempt to open again. Even still this is clearly a design flaw in the Jack2 library. What latencies do you achieve BTW with Windows guests? Never tested, it's not the reason why I use jack. Suffice to say it's far better than PulseAudio, I get no stuttering issues like is commonly reported for ALSA and PA, and allows for a high degree of reconfigurability. The guest VM overall performs far better also as windows is never waiting on the audio device due to the decoupling provided by the ring buffer in my implementation. I am aware and since these libraries are interchangeable I had assumed that JACK1 will have the same fault. If not I suppose we need to detect which is in use and change this code appropriately. I haven't checked this in the JACK1 code base yet, but I assume JACK1 does not behave like JACK2 here, because the JACK API is very clear that it is the client's responsibility to free itself. So it looks like a JACK2-only-bug to me. Confirmed, this was investigated today. Very weird that there is no jack_client_version() in the shared weak API (i.e. missing on JACK1 side). I raised this as an issue today: https://github.com/jackaudio/jack2/issues/628 The developer there seems to feel that allowing the application to know the jack client version is a bad thing. Best regards, Christian Schoenebeck
Re: [PATCH 1/3] vhost-vsock-pci: force virtio version 1
On Wed, 19 Aug 2020 12:51:54 +0200
Stefano Garzarella wrote:
> Commit 9b3a35ec82 ("virtio: verify that legacy support is not
> accidentally on") added a safety checks that requires to set
Nit: s/checks/check/ (also in patch 2)
> 'disable-legacy=on' on vhost-vsock-pci device:
>
> $ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5
> qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=5:
> device is modern-only, use disable-legacy=on
>
> virtio-vsock was introduced after the release of VIRTIO 1.0
> specifications, so it should be 'modern-only'.
> In addition Cornelia verified that forcing a legacy mode on
> vhost-vsock-pci device using x86-64 host and s390x guest, so with
> different endianness, produces strange behaviours.
>
> This patch forces virtio version 1 and remove 'transitional_name'
> properties removing the need to specify 'disable-legacy=on' on
"removes the 'transitional_name' property" ?
(Unless you want to merge with patch 2, which might make sense.)
> vhost-vsock-pci device.
>
> Cc: qemu-sta...@nongnu.org
> Reported-by: Qian Cai
> Reported-by: Qinghua Cheng
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1868449
> Suggested-by: Cornelia Huck
> Signed-off-by: Stefano Garzarella
> ---
> hw/virtio/vhost-vsock-pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Cornelia Huck
Re: [RFC PATCH 19/22] block/export: Move strong user reference to block_exports
On 13.08.20 18:29, Kevin Wolf wrote: > The reference owned by the user/monitor that is created when adding the > export and dropped when removing it was tied to the 'exports' list in > nbd/server.c. Every block export will have a user reference, so move it > to the block export level and tie it to the 'block_exports' list in > block/export/export.c instead. This is necessary for introducing a QMP > command for removing exports. > > Note that exports are present in block_exports even after the user has > requested shutdown. This is different from NBD's exports where exports > are immediately removed on a shutdown request, even if they are still in > the process of shutting down. In order to avoid that the user still > interacts with an export that is shutting down (and possibly removes it > a second time), we need to remember if the user actually still owns it. > > Signed-off-by: Kevin Wolf > --- > include/block/export.h | 8 > block/export/export.c | 4 > blockdev-nbd.c | 5 - > nbd/server.c | 2 -- > 4 files changed, 12 insertions(+), 7 deletions(-) With this patch, there’s an abort in iotest 281. Perhaps because blk_exp_unref() is now done by blk_exp_request_shutdown() outside of where the AIO context is locked? Max signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/3] vhost-user-vsock-pci: force virtio version 1
On Wed, 19 Aug 2020 12:51:55 +0200
Stefano Garzarella wrote:
> Commit 9b3a35ec82 ("virtio: verify that legacy support is not
> accidentally on") added a safety checks that requires to set
> 'disable-legacy=on' on vhost-user-vsock-pci device:
>
> $ ./qemu-system-x86_64 ... \
> -chardev socket,id=char0,reconnect=0,path=/tmp/vhost4.socket \
> -device vhost-user-vsock-pci,chardev=char0
> qemu-system-x86_64: -device vhost-user-vsock-pci,chardev=char0:
> device is modern-only, use disable-legacy=on
>
> virtio-vsock was introduced after the release of VIRTIO 1.0
> specifications, so it should be 'modern-only'.
>
> This patch forces virtio version 1 and remove 'transitional_name'
> properties, as done for vhost-vsock-pci, removing the need to specify
> 'disable-legacy=on' on vhost-user-vsock-pci device.
>
> Cc: qemu-sta...@nongnu.org
> Suggested-by: Cornelia Huck
> Signed-off-by: Stefano Garzarella
> ---
> hw/virtio/vhost-user-vsock-pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Same nits for the patch description as for patch 1, otherwise
Reviewed-by: Cornelia Huck
Re: [PATCH 3/3] vhost-vsock-ccw: force virtio version 1
On Wed, 19 Aug 2020 12:51:56 +0200 Stefano Garzarella wrote: > virtio-vsock was introduced after the release of VIRTIO 1.0 > specifications, so it should be 'modern-only'. > > This patch forces virtio version 1 as done for vhost-vsock-pci. > > Cc: qemu-sta...@nongnu.org > Suggested-by: Cornelia Huck > Signed-off-by: Stefano Garzarella > --- > hw/s390x/vhost-vsock-ccw.c | 2 ++ > 1 file changed, 2 insertions(+) Reviewed-by: Cornelia Huck
Re: [PATCH v5 1/5] block/io: fix bdrv_co_block_status_above
19.08.2020 14:34, Alberto Garcia wrote:
On Wed 19 Aug 2020 11:48:25 AM CEST, Vladimir Sementsov-Ogievskiy wrote:
+ * The top layer deferred to this layer, and because this layer is
+ * short, any zeroes that we synthesize beyond EOF behave as if
they
+ * were allocated at this layer
*/
+assert(ret & BDRV_BLOCK_EOF);
*pnum = bytes;
+if (file) {
+*file = p;
+}
+return BDRV_BLOCK_ZERO | BDRV_BLOCK_ALLOCATED;
You don't add BDRV_BLOCK_EOF to the return code here ?
No we shouldn't, as this is the end of backing file when the top layer
is larger.
Ok, but maybe the original request also reaches EOF on the top layer.
An example that does not actually involve short backing files: let's say
that the size of the active image is 1MB and the backing file is 2MB.
We do 'write -z 960k 63k', that zeroes the last cluster minus a 1KB
tail, so qcow2_co_pwrite_zeroes() calls is_zero() to check if that last
1KB already contains zeroes.
bdrv_co_block_status_above() on the top layer returns BDRV_BLOCK_EOF: no
allocation, no zeros, we simply reached EOF. So we go to the backing
image to see what's there. This is also unallocated, there's no backing
file this time and want_zero is set, so this returns BDRV_BLOCK_ZERO.
However the backing file is longer so bdrv_co_block_status() does not
return BDRV_BLOCK_EOF this time.
If the backing file would have been the same size (1MB) we would have
BDRV_BLOCK_ZERO | BDRV_BLOCK_EOF, but if it's longer or (with your
patch) shorter then BDRV_BLOCK_EOF disappears.
Now, I don't see anyone else using BDRV_BLOCK_EOF for anything so this
does not have any practical effect at the moment, but is this worth
fixing?.
That's the point of course, I'll fix.
So, if we get EOF on top layer, we should add it to final ret anyway,
regardless of backing chain statuses.
+res = bdrv_block_status_above(bs, NULL, offset, bytes, &nr, NULL,
NULL);
+offset += nr;
+bytes -= nr;
+} while (res >= 0 && (res & BDRV_BLOCK_ZERO) && nr && bytes);
About this last "... && nr && bytes", I think 'nr' already implies
'bytes', maybe you want to use an assertion instead?
No, on the last iteration, bytes would be 0 and nr is a last
chunk. So, if we check only nr, we'll do one extra call of
bdrv_block_status_above with bytes=0, I don't want do it.
You're right !
Berto
--
Best regards,
Vladimir
Re: [RFC PATCH 22/22] block/export: Add query-block-exports
Am 19.08.2020 um 13:04 hat Max Reitz geschrieben:
> On 13.08.20 18:29, Kevin Wolf wrote:
> > This adds a simple QMP command to query the list of block exports.
> >
> > Signed-off-by: Kevin Wolf
> > ---
> > qapi/block-export.json | 33 +
> > block/export/export.c | 23 +++
> > 2 files changed, 56 insertions(+)
> >
> > diff --git a/qapi/block-export.json b/qapi/block-export.json
> > index a067de2ba3..0b184bbd7c 100644
> > --- a/qapi/block-export.json
> > +++ b/qapi/block-export.json
> > @@ -226,3 +226,36 @@
> > ##
> > { 'command': 'block-export-del',
> >'data': { 'id': 'str', '*mode': 'BlockExportRemoveMode' } }
> > +
> > +##
> > +# @BlockExportInfo:
> > +#
> > +# Information about a single block export.
> > +#
> > +# @id: The unique identifier for the block export
> > +#
> > +# @type: This field is returned only for compatibility reasons, it should
> > +#not be used (always returns 'unknown')
>
> Äh?
>
> I don’t understand. It looks like it definitely doesn’t always return
> “unknown”. Also, the “compatibility reasons” aren’t really immediately
> clear to me... :?
Oops, this seems to be copied from BlockInfo and I forgot to change it...
Kevin
signature.asc
Description: PGP signature
Re: [PATCH 1/3] vhost-vsock-pci: force virtio version 1
On Wed, Aug 19, 2020 at 01:55:42PM +0200, Cornelia Huck wrote:
> On Wed, 19 Aug 2020 12:51:54 +0200
> Stefano Garzarella wrote:
>
> > Commit 9b3a35ec82 ("virtio: verify that legacy support is not
> > accidentally on") added a safety checks that requires to set
>
> Nit: s/checks/check/ (also in patch 2)
I'll fix.
>
> > 'disable-legacy=on' on vhost-vsock-pci device:
> >
> > $ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5
> > qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=5:
> > device is modern-only, use disable-legacy=on
> >
> > virtio-vsock was introduced after the release of VIRTIO 1.0
> > specifications, so it should be 'modern-only'.
> > In addition Cornelia verified that forcing a legacy mode on
> > vhost-vsock-pci device using x86-64 host and s390x guest, so with
> > different endianness, produces strange behaviours.
> >
> > This patch forces virtio version 1 and remove 'transitional_name'
> > properties removing the need to specify 'disable-legacy=on' on
>
> "removes the 'transitional_name' property" ?
It is better, I'll fix.
>
> (Unless you want to merge with patch 2, which might make sense.)
I left seprated because vhost-user-vsock-pci was introduced in QEMU 5.1,
so I wanted to make it easier to backport on others stable branches.
(I'm not sure if we continue to support 4.2).
Does it make sense to keep them separated?
>
> > vhost-vsock-pci device.
> >
> > Cc: qemu-sta...@nongnu.org
> > Reported-by: Qian Cai
> > Reported-by: Qinghua Cheng
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1868449
> > Suggested-by: Cornelia Huck
> > Signed-off-by: Stefano Garzarella
> > ---
> > hw/virtio/vhost-vsock-pci.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Reviewed-by: Cornelia Huck
>
Thanks,
Stefano
Re: [PATCH v4 3/3] hw/i386: Remove node_id, nr_nodes and nodes_per_pkg from topology
On Fri, 14 Aug 2020 16:39:40 -0500
Babu Moger wrote:
> Remove node_id, nr_nodes and nodes_per_pkg from topology. Use
> die_id, nr_dies and dies_per_pkg which is already available.
> Removes the confusion over two variables.
>
> With node_id removed in topology the uninitialized memory issue
> with -device and CPU hotplug will be fixed.
>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1828750
> Signed-off-by: Babu Moger
> ---
> hw/i386/pc.c |1 -
> hw/i386/x86.c |1 -
> include/hw/i386/topology.h | 40 +---
> target/i386/cpu.c | 11 +++
> target/i386/cpu.h |1 -
> 5 files changed, 12 insertions(+), 42 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 47c5ca3e34..0ae5cb3af4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1498,7 +1498,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> init_topo_info(&topo_info, x86ms);
>
> env->nr_dies = x86ms->smp_dies;
> -env->nr_nodes = topo_info.nodes_per_pkg;
> env->pkg_offset = x86ms->apicid_pkg_offset(&topo_info);
>
> /*
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index e90c42d2fc..4efa1f8b87 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -62,7 +62,6 @@ inline void init_topo_info(X86CPUTopoInfo *topo_info,
> {
> MachineState *ms = MACHINE(x86ms);
>
> -topo_info->nodes_per_pkg = ms->numa_state->num_nodes / ms->smp.sockets;
> topo_info->dies_per_pkg = x86ms->smp_dies;
> topo_info->cores_per_die = ms->smp.cores;
> topo_info->threads_per_core = ms->smp.threads;
> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
> index 07239f95f4..05ddde7ba0 100644
> --- a/include/hw/i386/topology.h
> +++ b/include/hw/i386/topology.h
> @@ -47,14 +47,12 @@ typedef uint32_t apic_id_t;
>
> typedef struct X86CPUTopoIDs {
> unsigned pkg_id;
> -unsigned node_id;
> unsigned die_id;
> unsigned core_id;
> unsigned smt_id;
> } X86CPUTopoIDs;
>
> typedef struct X86CPUTopoInfo {
> -unsigned nodes_per_pkg;
> unsigned dies_per_pkg;
> unsigned cores_per_die;
> unsigned threads_per_core;
> @@ -89,11 +87,6 @@ static inline unsigned apicid_die_width(X86CPUTopoInfo
> *topo_info)
> return apicid_bitwidth_for_count(topo_info->dies_per_pkg);
> }
>
> -/* Bit width of the node_id field per socket */
> -static inline unsigned apicid_node_width_epyc(X86CPUTopoInfo *topo_info)
> -{
> -return apicid_bitwidth_for_count(MAX(topo_info->nodes_per_pkg, 1));
> -}
> /* Bit offset of the Core_ID field
> */
> static inline unsigned apicid_core_offset(X86CPUTopoInfo *topo_info)
> @@ -114,30 +107,23 @@ static inline unsigned apicid_pkg_offset(X86CPUTopoInfo
> *topo_info)
> return apicid_die_offset(topo_info) + apicid_die_width(topo_info);
> }
>
> -#define NODE_ID_OFFSET 3 /* Minimum node_id offset if numa configured */
> +#define EPYC_DIE_OFFSET 3 /* Minimum die_id offset if numa configured */
^^
from EPYC's pov NUMA is always configured, there no 'if'
but I have a question, is it possible to drop EPYC_DIE_OFFSET
and reuse apicid_die_offset(), will it break something?
The reason for question is that, we (deviating from spec)
do allow for unbound core/threads number so die_id, already could
be shifted beyound ApicId[5:4], likewise we do allow for unbound
sockets so ApicId[6] is also out of spec.
If we are fine with ApicId being encoded in these cases out of
spec, then why should we insist on DIE_OFFSET minumum at all?
Why not just allow existing apicid_die_width() to encode die_id?
In this case it will produce SPECed ApicId if user has provided
-smp that matches currently released EPYC's and in all other cases
it will be out of spec ApicId like when we set sockets/cores/trheads
to out of spec values.
> /*
> - * Bit offset of the node_id field
> - *
> - * Make sure nodes_per_pkg > 0 if numa configured else zero.
> + * Bit offset of the die_id field
> */
> -static inline unsigned apicid_node_offset_epyc(X86CPUTopoInfo *topo_info)
> +static inline unsigned apicid_die_offset_epyc(X86CPUTopoInfo *topo_info)
> {
> -unsigned offset = apicid_die_offset(topo_info) +
> - apicid_die_width(topo_info);
> +unsigned offset = apicid_core_offset(topo_info) +
> + apicid_core_width(topo_info);
>
> -if (topo_info->nodes_per_pkg) {
> -return MAX(NODE_ID_OFFSET, offset);
> -} else {
> -return offset;
> -}
> +return MAX(EPYC_DIE_OFFSET, offset);
> }
>
> /* Bit offset of the Pkg_ID (socket ID) field */
> static inline unsigned apicid_pkg_offset_epyc(X86CPUTopoInfo *topo_info)
> {
> -return apicid_node_offset_epyc(topo_info) +
> - apicid_node_width_epyc(topo_info);
> +return apicid_die_offset_epyc(topo_info) + apicid_die_width(topo_info);
> }
>
> /*
> @@ -150,8 +136,7
Re: [PATCH 1/3] vhost-vsock-pci: force virtio version 1
On Wed, 19 Aug 2020 14:09:10 +0200
Stefano Garzarella wrote:
> On Wed, Aug 19, 2020 at 01:55:42PM +0200, Cornelia Huck wrote:
> > On Wed, 19 Aug 2020 12:51:54 +0200
> > Stefano Garzarella wrote:
> >
> > > Commit 9b3a35ec82 ("virtio: verify that legacy support is not
> > > accidentally on") added a safety checks that requires to set
> >
> > Nit: s/checks/check/ (also in patch 2)
>
> I'll fix.
>
> >
> > > 'disable-legacy=on' on vhost-vsock-pci device:
> > >
> > > $ ./qemu-system-x86_64 ... -device vhost-vsock-pci,guest-cid=5
> > > qemu-system-x86_64: -device vhost-vsock-pci,guest-cid=5:
> > > device is modern-only, use disable-legacy=on
> > >
> > > virtio-vsock was introduced after the release of VIRTIO 1.0
> > > specifications, so it should be 'modern-only'.
> > > In addition Cornelia verified that forcing a legacy mode on
> > > vhost-vsock-pci device using x86-64 host and s390x guest, so with
> > > different endianness, produces strange behaviours.
> > >
> > > This patch forces virtio version 1 and remove 'transitional_name'
> > > properties removing the need to specify 'disable-legacy=on' on
> >
> > "removes the 'transitional_name' property" ?
>
> It is better, I'll fix.
>
> >
> > (Unless you want to merge with patch 2, which might make sense.)
>
> I left seprated because vhost-user-vsock-pci was introduced in QEMU 5.1,
> so I wanted to make it easier to backport on others stable branches.
> (I'm not sure if we continue to support 4.2).
>
> Does it make sense to keep them separated?
Yes, indeed, it makes sense for stable backporting purposes.
Re: [PATCH v2 1/7] x86: lpc9: let firmware negotiate 'CPU hotplug with SMI' features
On Wed, 19 Aug 2020 11:14:45 +0200 Laszlo Ersek wrote: > On 08/19/20 10:58, Cornelia Huck wrote: > > > I consider any patch that adds compat options without adding all compat > > machines first to be buggy. We had that in the past, and it had been > > painful to sort it out again later. That's why I usually post a compat > > machines patch during hardfreeze, to be picked up by anyone who needs > > it. > > > > (See > > https://lore.kernel.org/qemu-devel/20200728094645.272149-1-coh...@redhat.com/ > > -- this is the patch that Daniel re-posted.) > > Indeed, thank you -- in the end, I did notice that Daniel's patch > started with "From: Cornelia" :) and then I checked my qemu-devel > folders for more emails with the same subject. > > > Just pick my original patch, it has a bunch of acks already. > > True, but Igor's series wouldn't apply without manual conflict > resolution, and I can't do that if I want to respond with a Tested-by. > > (Of course, picking up your patch and including it in v3 is feasible for > Igor.) I'll rebase this path on top 5.2 machine patch and post it here. > > Thanks! > Laszlo
Re: [PATCH v7 14/47] stream: Deal with filters
On 10.08.20 13:04, Vladimir Sementsov-Ogievskiy wrote: > 10.08.2020 11:12, Max Reitz wrote: >> On 07.08.20 12:29, Vladimir Sementsov-Ogievskiy wrote: [...] >>> But, with our proposed way (freeze only chain up to base_overlay >>> inclusively, and use backing(base_overlay) as final backing), all will >>> work as expected, and two parallel jobs will work.. >> >> I don’t think it will work as expected because users can no longer >> specify which node should be the base node after streaming. And the >> QAPI schema says that base-node is to become the backing file of the top >> node after streaming. > > But this will never work with either way: base node may disappear during > stream. Even with you way, they only stable thing is "above-base", which > backing child may be completely another node at stream finish. Yeah, but after c624b015bf, that’s just how it is. I thought the best would be an approach where if there are no graph changes during the job, you would indeed end up with @base as the backing file afterwards. [...] >> Well, that still leaves the problem that users should be able to specify >> which node is to become the base after streaming, and that that node >> maybe shouldn’t be restricted to immediate children of COW images. > > And again, this is impossible even with your way. I have an idea: > > What about making the whole thing explicit? > > We add an optional parameter to stream-job: bottom-node, which is > mutally exclusive with specifying base. > > Then, if user specified base node, we freeze base as well, so it can't > disappear. User will not be able to start parallel stream with this base > node as top (because new stream can not insert a filter into frozen > chain), but for sure it's rare case, used only in iotest 30 :)). > Benefit: user have guarantee of what would be final backing node. Sounds very nice to me, but... > Otherwise, if user specified bottom-node, we use the way of this patch. > So user can run parallel streams (iotest 30 will have to use bottom-node > argument). No guarantee of final base-node, it would be backing of > bottom-node at job finish. > > But, this is incompatible change, and we probably should wait for 3 > releases for deprecation of old behavior.. ...yeah. Hm. What a pain, but right, we can just deprecate it. Unfortunately, I don’t think there’s any way we could issue a warning (we’d want to deprecate using the @base node in something outside of the stream job, and we can’t really detect this case to issue a warning). So it would be a deprecation found only in the deprecation notes and the QAPI spec... > Anyway, I feel now, that you convinced me. I'm not sure that we will not > have to change it make filter work, but not reason to change something > now. Andrey, could you try to rebase your series on top of this and fix > iotest 30 by just specifying exact node-names in it?.. > > > Hmmm. My thought goes further. Seems, that in this way, introducing > explicit filter would be incompatible change anyway: it will break > scenario with parallel stream jobs, when user specifies filenames, not > node names (user will have to specify filter-node name as base for > another stream job, as you said). So, it's incompatible anyway. > > What do you think of it? Could we break this scenario in one release > without deprecation and don't care? I don’t know, but I’m afraid I don’t think we can. > Than I think my idea about base vs > bottom-node arguments for stream job may be applied. Or what to do? > > If we can't break this scenario without a deprecation, we'll have to > implement "implicit" filter, like for mirror, when filter-node-name is > not specified. And for this implicit filter we'll need additional logic > (closer to what I've proposed in a previous mail). Or, try to keep > stream without a filter (not insert it at all and behave the old way), > when filter-node-name is not specified. Than new features based on > filter will be available only when filter-node-name is specified, but > this is OK. The latter seems better for me. If that works... OK. So what I think we can do is first just take this patch as part of this series. Then, we add @bottom-node separately and deprecate @base not being frozen. If it’s feasible to not add a stream filter node until the deprecation period is over, then that should work. Max signature.asc Description: OpenPGP digital signature
[Bug 1883984] Re: QEMU S/390x sqxbr (128-bit IEEE 754 square root) crashes qemu-system-s390x
** Merge proposal linked:
https://code.launchpad.net/~paelzer/ubuntu/+source/qemu/+git/qemu/+merge/389527
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1883984
Title:
QEMU S/390x sqxbr (128-bit IEEE 754 square root) crashes qemu-system-
s390x
Status in QEMU:
Fix Committed
Status in qemu package in Ubuntu:
Fix Released
Status in qemu source package in Focal:
Triaged
Bug description:
[Impact]
* An instruction was described wrong so that on usage the program would
crash.
[Test Case]
* Run s390x in emulation and there use this program:
For simplicity and speed you can use KVM guest as usual on s390x, that
after prep&install&compile of the test you run in qemu-tcg like:
$ sudo qemu-system-s390x -machine s390-ccw-virtio,accel=tcg -cpu
max,zpci=on -serial mon:stdio -display none -m 4096 -nic
user,model=virtio,hostfwd=tcp::-:22 -drive
file=/var/lib/uvtool/libvirt/images/focal-sqxbr.qcow,if=none,id=drive-virtio-disk0,format=qcow2,cache=none
-device
virtio-blk-ccw,devno=fe.0.0001,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,scsi=off
Obviously is you have no s390x access you need to use emulation right
away.
* Build and run failing program
$ sudo apt install clang
$ cat > bug-sqrtl-one-line.c << EOF
int main(void) { volatile long double x, r; x = 4.0L; __asm__
__volatile__("sqxbr %0, %1" : "=f" (r) : "f" (x)); return (0);}
EOF
$ cc bug-sqrtl-one-line.c
$ ./a.out
Segmentation fault (core dumped)
qemu is dead by now as long as the bug is present
[Regression Potential]
* The change only modifies 128 bit square root on s390x so regressions
should be limited to exactly that - which formerly before this fix was
a broken instruction.
[Other Info]
* n/a
---
In porting software to guest Ubuntu 18.04 and 20.04 VMs for S/390x, I
discovered
that some of my own numerical programs, and also a GNU configure script for at
least one package with CC=clang, would cause an instant crash of the VM,
sometimes
also destroying recently opened files, and producing long strings of NUL
characters
in /var/log/syslog in the S/390 guest O/S.
Further detective work narrowed the cause of the crash down to a single IBM
S/390
instruction: sqxbr (128-bit IEEE 754 square root). Here is a one-line program
that when compiled and run on a VM hosted on QEMUcc emulator version 4.2.0
(Debian 1:4.2-3ubuntu6.1) [hosted on Ubuntu 20.04 on a Dell Precision 7920
workstation with an Intel Xeon Platinum 8253 CPU], and also on QEMU emulator
version 5.0.0, reproducibly produces a VM crash under qemu-system-s390x.
% cat bug-sqrtl-one-line.c
int main(void) { volatile long double x, r; x = 4.0L; __asm__
__volatile__("sqxbr %0, %1" : "=f" (r) : "f" (x)); return (0);}
% cc bug-sqrtl-one-line.c && ./a.out
Segmentation fault (core dumped)
The problem code may be the function float128_sqrt() defined in
qemu-5.0.0/fpu/softfloat.c
starting at line 7619. I have NOT attempted to run the qemu-system-s390x
executable
under a debugger. However, I observe that S/390 is the only CPU family that
I know of,
except possibly for a Fujitsu SPARC-64, that has a 128-bit square root in
hardware.
Thus, this instruction bug may not have been seen before.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1883984/+subscriptions
