Re: [PATCH] 'channel' and 'addr' in qmp_migrate() are not auto-freed. migrate_uri_parse() allocates memory which is returned to 'channel', which is leaked because there is no code for freeing 'channel
On 28/11/23 12:46 pm, Markus Armbruster wrote:
Your commit message is all in one line. You need to format it like
migration: Plug memory leak
'channel' and 'addr' in qmp_migrate() are not auto-freed.
migrate_uri_parse() allocates memory which is returned to 'channel',
which is leaked because there is no code for freeing 'channel' or
'addr'. So, free addr and channel to avoid memory leak. 'addr'
does shallow copying of channel->addr, hence free 'channel' itself
and deep free contents of 'addr'.
Het Gala writes:
Yeah, I made the changes in v2 patchset.
---
migration/migration.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/migration/migration.c b/migration/migration.c
index 28a34c9068..29efb51b62 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2004,6 +2004,8 @@ void qmp_migrate(const char *uri, bool has_channels,
MIGRATION_STATUS_FAILED);
block_cleanup_parameters();
}
+g_free(channel);
+qapi_free_MigrationAddress(addr);
if (local_err) {
if (!resume_requested) {
2. hmp_migrate()
hmp_migrate() allocates @channel with migrate_uri_parse(), adds it to
list @caps, passes @caps to qmp_migrate(), then frees @caps with
qapi_free_MigrationChannelList().
Markus, sorry if I was not able to put point clearly, what I meant is
that the local 'channel' variable used in qmp_migrate() i.e.
'MigrationChannel *channel = NULL', is defined in qmp_migrate() and if
the user opts for 'uri' then '@channels' coming from hmp_migrate() will
be NULL, and then migrate_uri_parse() will populate memory into
'channel', and that is not getting freed after it's use is over.
I think, that is where memory leak might be happening ?
Regards,
Het Gala
[PULL 2/2] seabios: update binaries to 1.16.3 release
Signed-off-by: Gerd Hoffmann
---
pc-bios/bios-256k.bin | Bin 262144 -> 262144 bytes
pc-bios/bios-microvm.bin | Bin 131072 -> 131072 bytes
pc-bios/bios.bin | Bin 131072 -> 131072 bytes
pc-bios/vgabios-ati.bin | Bin 39424 -> 39424 bytes
pc-bios/vgabios-bochs-display.bin | Bin 28672 -> 28672 bytes
pc-bios/vgabios-cirrus.bin| Bin 38912 -> 38912 bytes
pc-bios/vgabios-qxl.bin | Bin 39424 -> 39424 bytes
pc-bios/vgabios-ramfb.bin | Bin 28672 -> 28672 bytes
pc-bios/vgabios-stdvga.bin| Bin 39424 -> 39424 bytes
pc-bios/vgabios-virtio.bin| Bin 39424 -> 39424 bytes
pc-bios/vgabios-vmware.bin| Bin 39424 -> 39424 bytes
pc-bios/vgabios.bin | Bin 38912 -> 38912 bytes
12 files changed, 0 insertions(+), 0 deletions(-)
diff --git a/pc-bios/bios-256k.bin b/pc-bios/bios-256k.bin
index
8d1dc0dff3af2a7db8a35d8eb4d2ac26bfe9d6c7..48c3707d55d3884c54efa9b3c70ab5c3382139c0
100644
GIT binary patch
delta 33002
zcmZ_03tW`N_XqsU!-5MgF355R0bLOh6%_BF2#A`RD2jMDQ(G->xv1p@aG`u$*Zh?=
znwFuKmo(`H%Vr6pfTiXw!%K=~`K+skT8h_wzvo%B`oHh{(J#Bt%$YN1&di)Sb7r2+
z&-cm4|7|RyY)S-EiFq4_2*PIIQ{Z#p0Pq8F8u$h1KU5GhfaifMUTOb1%3hV`L0iI(}
zDX4^6{k*_ypJuTmy+{x&K;%+E7!6DY)&cWS??Pa89{#)y
zMCS{_AV31v0h@r56@p*`4g$x3hk)872tGg%kO?dW3V<~L2TFj0KyaZT3;>=3#sE`*
z*MR>4=@Qa_RjULcFLe$6tQ7?L9rW3Ig0LKT9}u}9JOak8M@u(ArJDqy;sZgL_94^;
zn1Sfc(9lOvE5Lzf#h?e~0~dklErQSsSPeV`nr{^Z6YvgD4Q$?q{;$|32nnAEf)U6E
zb^5rp$V|^)-T^
ztrdhxz-r(!eg7VxKL|p@Q9;;#Ob`wM_kpg*1tAF-3OoR`Col$pA5J14c=;5@5bzb?
zb6ODQ0EdC#p9G;5un4#U+yjQ5p{~cDUBCgr?`O;eU=#2g(6tUkz)!%avw|=d*arLr
zbUr5t-vKe_1)&_c1;k#!SO8W4?*Y4kJAmdF*bZPiunc$$*bbEcg8tWD6oe?C7cc}^
z2&@D?2Ab7lq+h`_`va9-#bC2TsOxAE@Y@YRaQhQ|2*|+120;k9g$n-?gl>1Cg?obF
zbsuFPLa4`ra2sGw%rd}BU_vls!s{%L3B5%oXx*4_4p^&VLOpN`ur_1DHG(G-CV4So
zVRI%7(=cH=upH>>&4kvzOi=qVp)!gIn$}DR0MdbZz__+d*aw(mnNSRz2JQoOaZI=b
zgmz>?VqO9h<^UGpJkX&F6S9Hnz-{1gA`^}Sd%7{11tiT1D+=Q>7B%cGCs2%yOSp;GvPYW0Q?0s0*?Tp
zFB4S2yn#$81WJH|Km(u}#DpLq4oCq;0R~_xuol<`Sb-xzJ&^ev6Q%);LzoablnHtu
z4u~JhQiVi(=?NqO2Z37P2yh%Y4XB2pVSo>y1A>4^Ko7(K@nw%=SUXjo18Y~0p@Ogh
zs0PjhzXNlI2|^L@1n_(wvmEFJ3;}Y0nZU~77_*sz5C9AUvVj@E62J^>01g6iSobo3
zEMNxEYb0hnaC;=i^`tBm0R9BVWn-ZPE&;+A*f3x#@Hy}^FgHgK-UQYG#lUBPdYmA<
z2&@I7#$%ZQ76EI4uYps*9Y8ff5W;~pU@5SMo+swPc1?sC0bT>v0K0)o;0$mF@SOyE
z1`GoXz%pPXPy!qR&H#^ql*woza1uzGg0&0y5I6$d03HLwUl4?dsqhwn`M~Bp{P_a-
z4G4V^vlf^F%mvl~w*jwdf)ECb0;U5W0;hmRpcz*0NMHf5ANT?I8Mp_e%z!3<&ww+4
z$4k&APyw6(k`1tH8}a9JK%9w6fX+a7U^=i4*bS5dhk;Xo9T+wX3nW~FJwPq+D{vJ^
zeFZ`Se*%4Gg9i{X2Rwo2fKPy$xfl=g&<()IdFcO5`0_Py8qm(i_g68n7QjjaY9qP@
z@XAGB0UrR5fx`IdjPBgwgGzp2cSm&Cg2co85o(5!3M;y5QGsxu}Kj20e68}g{TDh8we^w
z9|Nxd&r4Xe0UK}&{|*`a2#j^!rl^uF2Gb^36Qr5fA#{^z%Rh8weUNDVxZUm
zuv`EOfMTEqxCs0WM7@nU4IBVIeh1wJxUYjPc~=m|1Ji+TfNQ{Cz_$0$5J1DB&GlH6
zfj%41|2Z2V39tqD8n6Sc-^V}#J_q`3#Co&|pTN7oEx-vZ{Q%Pk@cR&b4h#cwft#D5
zosXbVU>k5~3xozjwn9t5FyI4VH!yS?`gH0xEP{XyU?0P01-b(nKo;;hFz6Gw9Kd?u
zA~1bBd=sD?_!S7*0pA8#0Gt8Rc0%pIyTIRoW*52^s04<8it>ONSeUmP#snw_t^<*K
zFpYp|z-z$z&%mq%+9}20EJMEnVOF#RSOr*tD>j(yZ!m@Tq2<6hU>)%Gw`de_4ru)y
zOfOImJOuprgEx@pc>oN7U4RYfQzZzm0#3mDAo>ib2HXxobwEZnS`K*CKq#Ob2(E=?
z1j>QGfhR!6!w>{8d@l%Z0QY`CnWHd5$H41H40<5pNA!OZzDzw1CMTdm;0?g-6ihCV
z3%mn-2P{4f0|ey!gxLUOpMjQuO+fh1C<~+l#WrK%VCf*l3^&Fa#J6+yEW{
zo;P91fEu74=R$dBA(X?|`2d6W-0kpA*2}
zKo2<167UXi5m>2#Fu>P<6L{i-3Vh)w0`~#WmT>2QML;b+9t^%I$(%Skic=vz|=wiSFco|p-NWgkvM0*Ge>;S$4&bDWHf{ZVbvG9U{3c$Yu3INxEPvVdU
z{0KOJDIMXV$HQv}ICcTMfYZPqz+mhGrU09OuK>@k*d+knfdN1+@H(&wumUx}PhHXf
zKG-%y0DXW(z=a|Q$Ab5W?R6@?jVUH{59^<)8_EQ5d9pY;pQ%7oH<
za#acE9fJ$c=eD#Z3Rtsb`8D1=l|}fDMRxCWy_?a`lFg7jnDO+hEmDCfqx|!_DZQP~iyImbuybg0ws_U90qb#03a{lJ
z16dcZgHN9i^3;KBNO;An7J{VGCsdf1>Q&m(ea-5TVAklhdo@KBeAhtMpY7w<2eKeG
zrObU0>%xM+0*9i*MykqE(HQgb@Jx}F)mV-@8j*;@SrUdwq75s73^El?BWvClZ@Q_MVcD-
zo@hLMclc6L#UkHlNfikdTa%Dm;~s>+QG>mODszguEb%$!#XPB))EMfXR5ox3+ss%x
zzdDrt-ZFMDCaemKY(Dq{QS$l4VJtut2lKnbSmY4eEV}ZE2>IP1om4$bn8lRjgp)}7
z=Km7;g2;5x$Khv5iD@E}AM+QUX9Hq0K|3bf#WK>xG8swF7~1r%#F#<+`17oz?mk#O
zrPsK!{@}jDnJ(l6a-2qe7{SG_Dc{Qb4M#6+<~hUJJ7VHseq%VBB5DSe4bNmf#JqfH
z!mRmH>E*qeGE1VbWYl9A>kbttIq#suxinO-p`;c6N%Em2(|?k5+EUG-LQ2w^wR?4M
z(PI%kCgL&iEwB!vq&J==d4MG8(3@q=vsjJD%>2}7mdY;kfNU1QDtY&87R}!06SG-c
ze`5-4-MDfrA%e|MY5I734d>acJBHS=Y_?r@9P`|>QCn7P*6F3X$>S&6#ok;R!(t=7
zq0^NIOtsF!ler!9muT6-Q1b#^uY0dXO1IHtH~FbCY$99B6LQ$=0h?huif$NvF`u+$
z9@72slO|8JuPUp|Vc&>sXxXyyted;v2^hk{iq~3H$SPBPGi_=9qc{1XDQqg6%VS<(
z$zno3{?ZHVUH>+)MN+!nrzTymgU~AJxa`w|`%Yzn!8NF7+}NUu+?Kr(^|{{WG!N6k
zuJc+`!qJW)s<}IhT44G;idj5)ykvCFv0vzA?dS0F6Jwy
zu}xJYOD-!U-oW>3UH<
zf*zQGrbefT)#>^OCY_W0v-zdz%wN0ZX_g=&wSl|MU@>e7kDI}w^NPUGHsv_6J84$w
z<+Y=kFxEbIBvoq+l3%#!bmEKFwzQ~)V8s$B*oUGSRoPi1q-sg0mmfcNI?+|=cG-x<
zYzNEL*p?Q8b^}Cn1Val79IHSFR>x4geMqa5KH(>1A87HRs+jszj5tM)`sopU1zeyaI#Nk
zZ!%w2K?GCriPLHBrlck(@xqyGam)2oBUJ#?q72cLvKF)0Y*EDOvg{QM7+PKAKmT$%
z$D#B5Y=uNWk|f;B_rAi4iEiXW||Zv%~MMUtjoKBJ^}199N(Ij1kv#R
zfVY^-x@sm+>-tmc#OL|2x$JqL<1_&LpuF*lcDC|Q=dxgR1zJ#}(pT^!b6F;<<>B*K
z&xk41SGQ61ScTM%l~bxJU+1b{AJ2{RSZs^OFM12b7f@Vslfp0X-Sb##n4Su1+)(Ri
zl(EyGD35U!vv9BZ?1Sc!G>B+vk~i@$=EGOey@7-y)TgtCpqU-Z$-_W1KjydRvs
[PULL 0/2] Firmware/seabios 20231128 patches
The following changes since commit 4705fc0c8511d073bee4751c3c974aab2b10a970: Merge tag 'pull-for-8.2-fixes-231123-1' of https://gitlab.com/stsquad/qemu into staging (2023-11-24 08:00:18 -0500) are available in the Git repository at: https://gitlab.com/kraxel/qemu.git tags/firmware/seabios-20231128-pull-request for you to fetch changes up to eb0ce1346eca79f066e571dc5845f99f9ec730c3: seabios: update binaries to 1.16.3 release (2023-11-28 08:49:26 +0100) seabios: update to 1.16.3 release This adds one bugfix compared to the snapshot merged during the 8.2 devel cycle. Gerd Hoffmann (2): seabios: update submodule to 1.16.3 release seabios: update binaries to 1.16.3 release pc-bios/bios-256k.bin | Bin 262144 -> 262144 bytes pc-bios/bios-microvm.bin | Bin 131072 -> 131072 bytes pc-bios/bios.bin | Bin 131072 -> 131072 bytes pc-bios/vgabios-ati.bin | Bin 39424 -> 39424 bytes pc-bios/vgabios-bochs-display.bin | Bin 28672 -> 28672 bytes pc-bios/vgabios-cirrus.bin| Bin 38912 -> 38912 bytes pc-bios/vgabios-qxl.bin | Bin 39424 -> 39424 bytes pc-bios/vgabios-ramfb.bin | Bin 28672 -> 28672 bytes pc-bios/vgabios-stdvga.bin| Bin 39424 -> 39424 bytes pc-bios/vgabios-virtio.bin| Bin 39424 -> 39424 bytes pc-bios/vgabios-vmware.bin| Bin 39424 -> 39424 bytes pc-bios/vgabios.bin | Bin 38912 -> 38912 bytes roms/seabios | 2 +- 13 files changed, 1 insertion(+), 1 deletion(-) -- 2.43.0
[PULL 1/2] seabios: update submodule to 1.16.3 release
git shortlog 1e1da7a96300..rel-1.16.3 - Gerd Hoffmann (1): limit address space used for pci devices. Signed-off-by: Gerd Hoffmann --- roms/seabios | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roms/seabios b/roms/seabios index 1e1da7a96300..a6ed6b701f0a 16 --- a/roms/seabios +++ b/roms/seabios @@ -1 +1 @@ -Subproject commit 1e1da7a963007d03a4e0e9a9e0ff17990bb1608d +Subproject commit a6ed6b701f0a57db0569ab98b0661c12a6ec3ff8 -- 2.43.0
Re: [PATCH] util/qemu-config: Remove unnecessary traversal
Shiyuan Gao via writes:
> From: Gao Shiyuan
>
> No remove QemuOptsList from *_config_groups, so no need to
> traverse from the beginning every time.
>
> No functional changes.
>
> Signed-off-by: Gao Shiyuan
> ---
> include/qemu/config-file.h | 3 +++
> util/qemu-config.c | 18 --
> 2 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
> index b82a778123..223ef7bc8f 100644
> --- a/include/qemu/config-file.h
> +++ b/include/qemu/config-file.h
> @@ -1,6 +1,9 @@
> #ifndef QEMU_CONFIG_FILE_H
> #define QEMU_CONFIG_FILE_H
>
> +#define MAX_VM_CONFIG_GROUPS48
> +#define MAX_DRIVE_CONFIG_GROUPS 5
These are not used outside qemu-config.c, so it's better to define them
there.
> +
> typedef void QEMUConfigCB(const char *group, QDict *qdict, void *opaque,
> Error **errp);
>
> void qemu_load_module_for_opts(const char *group);
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index 42076efe1e..d7bab2047f 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -9,8 +9,8 @@
> #include "qemu/config-file.h"
> #include "hw/boards.h"
>
> -static QemuOptsList *vm_config_groups[48];
> -static QemuOptsList *drive_config_groups[5];
> +static QemuOptsList *vm_config_groups[MAX_VM_CONFIG_GROUPS];
> +static QemuOptsList *drive_config_groups[MAX_DRIVE_CONFIG_GROUPS];
>
> static QemuOptsList *find_list(QemuOptsList **lists, const char *group,
> Error **errp)
> @@ -260,11 +260,10 @@ QemuOptsList *qemu_find_opts_err(const char *group,
> Error **errp)
>
> void qemu_add_drive_opts(QemuOptsList *list)
> {
> -int entries, i;
> +static int i;
> +static int entries = MAX_DRIVE_CONFIG_GROUPS - 1; /* keep list NULL
> terminated */
>
> -entries = ARRAY_SIZE(drive_config_groups);
> -entries--; /* keep list NULL terminated */
> -for (i = 0; i < entries; i++) {
> +for (; i < entries; i++) {
> if (drive_config_groups[i] == NULL) {
> drive_config_groups[i] = list;
> return;
> @@ -276,11 +275,10 @@ void qemu_add_drive_opts(QemuOptsList *list)
>
> void qemu_add_opts(QemuOptsList *list)
> {
> -int entries, i;
> +static int i;
> +static int entries = MAX_VM_CONFIG_GROUPS - 1; /* keep list NULL
> terminated */
>
> -entries = ARRAY_SIZE(vm_config_groups);
> -entries--; /* keep list NULL terminated */
> -for (i = 0; i < entries; i++) {
> +for (; i < entries; i++) {
> if (vm_config_groups[i] == NULL) {
> vm_config_groups[i] = list;
> return;
These two functions run at most 4 and 47 times, respectively. I don't
think speeding them up matters. Keeping them simple does.
Your patch conflates two separate changes.
1. Replace use of ARRAY_SIZE() by new macros. Doesn't feel like an
improvement to me.
2. Optimize appending to arrays drive_config_groups[] and
vm_config_groups[].
Let's have a look at 2. without 1.:
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 42076efe1e..7875359eb3 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -260,11 +260,13 @@ QemuOptsList *qemu_find_opts_err(const char *group, Error
**errp)
void qemu_add_drive_opts(QemuOptsList *list)
{
-int entries, i;
+static int i;
+int entries;
entries = ARRAY_SIZE(drive_config_groups);
entries--; /* keep list NULL terminated */
-for (i = 0; i < entries; i++) {
+
+for (; i < entries; i++) {
if (drive_config_groups[i] == NULL) {
drive_config_groups[i] = list;
return;
@@ -276,11 +278,13 @@ void qemu_add_drive_opts(QemuOptsList *list)
void qemu_add_opts(QemuOptsList *list)
{
-int entries, i;
+static int i;
+int entries;
entries = ARRAY_SIZE(vm_config_groups);
entries--; /* keep list NULL terminated */
-for (i = 0; i < entries; i++) {
+
+for (; i < entries; i++) {
if (vm_config_groups[i] == NULL) {
vm_config_groups[i] = list;
return;
This is a more focused patch.
It makes the functions faster, which doesn't matter. Does it make the
functions easier to understand? That would matter.
Re: [RFC] Flexible SR-IOV support for virtio-net
On 2023/11/18 21:10, Akihiko Odaki wrote:
Hi,
We are planning to add PCIe SR-IOV support to the virtio-net driver
for Windows ("NetKVM")[1], and we want a SR-IOV feature for virtio-net
emulation code in QEMU to test it. I expect there are other people
interested in such a feature, considering that people are using igb[2]
to test SR-IOV support in VMs.
Washizu Yui have already proposed an RFC patch to add a SR-IOV feature
to virtio-net emulation[3][4] but it's preliminary and has no
configurability for VFs.
Now I'm proposing to add SR-IOV support to virtio-net with full
configurability for VFs by following the implementation of virtio-net
failover[5]. I'm planning to write patches myself, but I know there
are people interested in such patches so I'd like to let you know the
idea beforehand.
The idea:
The problem when implementing configurability for VFs is that SR-IOV
VFs can be realized and unrealized at runtime with a request from the
guest. So a naive implementation cannot deal with a command line like
the following:
-device virtio-net-pci,addr=0x0.0x0,sriov=on
-device virtio-net-pci,addr=0x0.0x1
-device virtio-net-pci,addr=0x0.0x2
This will realize the virtio-net functions in 0x0.0x1 and 0x0.0x2 when
the guest starts instead of when the guest requests to enable VFs.
However, reviewing the virtio-net emulation code, I realized the
virtio-net failover also "hides" devices when the guest starts. The
following command line hides hostdev0 when the guest starts, and adds
it when the guest requests VIRTIO_NET_F_STANDBY feature:
-device virtio-net-pci,netdev=hostnet1,id=net1,mac=52:54:00:6f:55:cc, \
bus=root2,failover=on
-device vfiopci,host=5e:00.2,id=hostdev0,bus=root1,failover_pair_id=net1
So it should be also possible to do similar to "hide" VFs and
realize/unrealize them when the guest requests.
There are two things I hate with this idea when contrasting it with
the conventional multifunction feature[6] though. One is that the PF
must be added before VFs; a similar limitation is imposed for failover.
Another is that it will be specific to virtio-net. I was considering
to implement a "generic" SR-IOV feature that will work on various
devices, but I realized that will need lots of configuration
validations. We may eventually want it, but probably it's better to
avoid such a big leap as the first step.
Please tell me if you have questions or suggestions.
Hi, Odaki-san
The idea appears to be practical and convenient.
I have some things I want to confirm.
I understood your idea can make deices for VFs,
created by qdev_new or qdev_realize function, invisible from guest OS.
Is my understanding correct ?
And, if your idea is realized,
will it be possible to specify the backend device for the virtio-pci-net
device ?
Could you provide insights into the next steps
beyond the implementation details ?
About when do you expect your implementation
to be merged into qemu ?
Do you have a timeline for this plan ?
Moreover, is there any way
we can collaborate on the implementation you're planning ?
Regards,
Yui Washizu
Regards,
Akihiko Odaki
[1] https://github.com/virtio-win/kvm-guest-drivers-windows
[2] https://qemu.readthedocs.io/en/v8.1.0/system/devices/igb.html
[3]
https://patchew.org/QEMU/1689731808-3009-1-git-send-email-yui.wash...@gmail.com/
[4]
https://netdevconf.info/0x17/sessions/talk/unleashing-sr-iov-offload-on-virtual-machines.html
[5] https://qemu.readthedocs.io/en/v8.1.0/system/virtio-net-failover.html
[6] https://gitlab.com/qemu-project/qemu/-/blob/v8.1.2/docs/pcie.txt
Re: [PULL 0/2] Firmware/seabios 20231128 patches
28.11.2023 11:17, Gerd Hoffmann : seabios: update to 1.16.3 release Gerd, please also push this tag to seabios master git branch. Thanks, /mjt
Re: [PATCH v2 for-8.2] ppc/amigaone: Allow running AmigaOS without firmware image
On 28/11/23 08:07, Cédric Le Goater wrote:
On 11/28/23 02:47, Nicholas Piggin wrote:
On Tue Nov 28, 2023 at 2:37 AM AEST, Cédric Le Goater wrote:
I'm not sure, I don't think it's necessary if your minimal patch works.
I'll do a PR for 8.2 for SLOF and Skiboot updates, so happy to include
this as well.
I think this amigaone patch could still be merged since it's only
touching a new machine and it's fixing an issue of missing firmware.
ARM does something similar with roms. See hw/arm/boot.c file.
It will need a "Fixes" tag.
Fixes: d9656f860a ("hw/ppc: Add emulation of AmigaOne XE board")
Re: [PATCH 18/19] qapi/schema: remove unnecessary asserts
John Snow writes:
> With strict typing enabled, these runtime statements aren't necessary
> anymore.
>
> Signed-off-by: John Snow
> ---
> scripts/qapi/schema.py | 23 ---
> 1 file changed, 23 deletions(-)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 5d19b59def0..b5f377e68b8 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -78,9 +78,7 @@ def __init__(
class QAPISchemaEntity:
meta: str
def __init__(
self,
name: str,
info: Optional[QAPISourceInfo],
doc: Optional[QAPIDoc],
> ifcond: Optional[QAPISchemaIfCond] = None,
> features: Optional[List[QAPISchemaFeature]] = None,
> ):
> -assert name is None or isinstance(name, str)
Yup, because name: str.
> for f in features or []:
> -assert isinstance(f, QAPISchemaFeature)
Yup, because features: Optional[List[QAPISchemaFeature]].
> f.set_defined_in(name)
> self.name = name
> self._module: Optional[QAPISchemaModule] = None
> @@ -145,7 +143,6 @@ def visit(self, visitor: QAPISchemaVisitor) -> None:
> assert self._checked
>
> def describe(self) -> str:
> -assert self.meta
Yup, because QAPISchemaEntity has meta: str.
> return "%s '%s'" % (self.meta, self.name)
>
>
> @@ -359,7 +356,6 @@ def check(self, schema: QAPISchema) -> None:
> f"feature '{feat.name}' is not supported for types")
>
> def describe(self) -> str:
> -assert self.meta
Likewise.
> return "%s type '%s'" % (self.meta, self.name)
>
>
> @@ -368,7 +364,6 @@ class QAPISchemaBuiltinType(QAPISchemaType):
class QAPISchemaBuiltinType(QAPISchemaType):
meta = 'built-in'
>
> def __init__(self, name: str, json_type: str, c_type: str):
> super().__init__(name, None, None)
> -assert not c_type or isinstance(c_type, str)
Yup, because c_type: str.
Odd: the assertion accepts None, but the type doesn't. Turns out None
was possible until commit 2d21291ae64 (qapi: Pseudo-type '**' is now
unused, drop it). The assertion should have been adjusted then.
Probably not worth a commit message mention now.
> assert json_type in ('string', 'number', 'int', 'boolean', 'null',
> 'value')
> self._json_type_name = json_type
> @@ -411,9 +406,7 @@ def __init__(
class QAPISchemaEnumType(QAPISchemaType):
meta = 'enum'
def __init__(
self,
name: str,
info: Optional[QAPISourceInfo],
doc: Optional[QAPIDoc],
ifcond: Optional[QAPISchemaIfCond],
features: Optional[List[QAPISchemaFeature]],
members: List[QAPISchemaEnumMember],
prefix: Optional[str],
> ):
> super().__init__(name, info, doc, ifcond, features)
> for m in members:
> -assert isinstance(m, QAPISchemaEnumMember)
Yup, because members: List[QAPISchemaEnumMember].
> m.set_defined_in(name)
> -assert prefix is None or isinstance(prefix, str)
Yup, because prefix: Optional[str].
> self.members = members
> self.prefix = prefix
>
> @@ -456,7 +449,6 @@ def __init__(
class QAPISchemaArrayType(QAPISchemaType):
meta = 'array'
def __init__(
> self, name: str, info: Optional[QAPISourceInfo], element_type: str
> ):
> super().__init__(name, info, None)
> -assert isinstance(element_type, str)
Yup, because element_type: str.
> self._element_type_name = element_type
> self._element_type: Optional[QAPISchemaType] = None
>
> @@ -517,7 +509,6 @@ def visit(self, visitor: QAPISchemaVisitor) -> None:
> self.element_type)
>
> def describe(self) -> str:
> -assert self.meta
Yup, because QAPISchemaEntity has meta: str.
> return "%s type ['%s']" % (self.meta, self._element_type_name)
>
>
> @@ -537,12 +528,9 @@ def __init__(
class QAPISchemaObjectType(QAPISchemaType):
def __init__(
self,
name: str,
info: Optional[QAPISourceInfo],
doc: Optional[QAPIDoc],
ifcond: Optional[QAPISchemaIfCond],
features: Optional[List[QAPISchemaFeature]],
base: Optional[str],
local_members: List[QAPISchemaObjectTypeMember],
variants: Optional[QAPISchemaVariants],
):
# struct has local_members, optional base, and no variants
> # union has base, variants, and no local_members
> super().__init__(name, info, doc, ifcond, features)
> self.meta = 'union' if variants else 'struct'
> -assert base is None or isinstance(base, str)
Yup, because base: Optional[str].
> for m in local_members:
> -assert isinstance(m, QAPISchemaObjectTypeMember)
Yup, because local_member
Re: [PULL 0/2] Firmware/seabios 20231128 patches
On Tue, Nov 28, 2023 at 11:54:34AM +0300, Michael Tokarev wrote: > 28.11.2023 11:17, Gerd Hoffmann : > > > seabios: update to 1.16.3 release > > Gerd, please also push this tag to seabios master git branch. Can't push to master branch right now (the tag is in the repo though), trying to sort this out with the coreboot admins ATM take care, Gerd
Re: [PATCH v7 4/8] hw/arm/virt: Hide host CPU model for tcg
On 27/11/23 00:12, Gavin Shan wrote: The 'host' CPU model isn't available until KVM or HVF is enabled. For example, the following error messages are seen when the guest is started with option '-cpu cortex-a8' on tcg after the next commit is applied to check the CPU type in machine_run_board_init(). ERROR:../hw/core/machine.c:1423:is_cpu_type_supported: \ assertion failed: (model != NULL) Bail out! ERROR:../hw/core/machine.c:1423:is_cpu_type_supported: \ assertion failed: (model != NULL) Aborted (core dumped) Hide 'host' CPU model until KVM or HVF is enabled. With this applied, the valid CPU models can be shown. qemu-system-aarch64: Invalid CPU type: cortex-a8 The valid types are: cortex-a7, cortex-a15, cortex-a35, \ cortex-a55, cortex-a72, cortex-a76, cortex-a710, a64fx, \ neoverse-n1, neoverse-v1, neoverse-n2, cortex-a53, \ cortex-a57, max Signed-off-by: Gavin Shan Reviewed-by: Richard Henderson --- hw/arm/virt.c | 2 ++ 1 file changed, 2 insertions(+) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v7 7/8] hw/arm: Check CPU type in machine_run_board_init()
On 27/11/23 00:12, Gavin Shan wrote: Set mc->valid_cpu_types so that the user specified CPU type can be validated in machine_run_board_init(). We needn't to do it by ourselves. Signed-off-by: Gavin Shan Reviewed-by: Richard Henderson --- hw/arm/bananapi_m2u.c | 12 ++-- hw/arm/cubieboard.c | 12 ++-- hw/arm/mps2-tz.c| 26 -- hw/arm/mps2.c | 26 -- hw/arm/msf2-som.c | 12 ++-- hw/arm/musca.c | 12 +--- hw/arm/npcm7xx_boards.c | 12 +--- hw/arm/orangepi.c | 12 ++-- 8 files changed, 74 insertions(+), 50 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [RFC] Flexible SR-IOV support for virtio-net
On 2023/11/28 17:47, Yui Washizu wrote:
On 2023/11/18 21:10, Akihiko Odaki wrote:
Hi,
We are planning to add PCIe SR-IOV support to the virtio-net driver
for Windows ("NetKVM")[1], and we want a SR-IOV feature for virtio-net
emulation code in QEMU to test it. I expect there are other people
interested in such a feature, considering that people are using igb[2]
to test SR-IOV support in VMs.
Washizu Yui have already proposed an RFC patch to add a SR-IOV feature
to virtio-net emulation[3][4] but it's preliminary and has no
configurability for VFs.
Now I'm proposing to add SR-IOV support to virtio-net with full
configurability for VFs by following the implementation of virtio-net
failover[5]. I'm planning to write patches myself, but I know there
are people interested in such patches so I'd like to let you know the
idea beforehand.
The idea:
The problem when implementing configurability for VFs is that SR-IOV
VFs can be realized and unrealized at runtime with a request from the
guest. So a naive implementation cannot deal with a command line like
the following:
-device virtio-net-pci,addr=0x0.0x0,sriov=on
-device virtio-net-pci,addr=0x0.0x1
-device virtio-net-pci,addr=0x0.0x2
This will realize the virtio-net functions in 0x0.0x1 and 0x0.0x2 when
the guest starts instead of when the guest requests to enable VFs.
However, reviewing the virtio-net emulation code, I realized the
virtio-net failover also "hides" devices when the guest starts. The
following command line hides hostdev0 when the guest starts, and adds
it when the guest requests VIRTIO_NET_F_STANDBY feature:
-device virtio-net-pci,netdev=hostnet1,id=net1,mac=52:54:00:6f:55:cc, \
bus=root2,failover=on
-device vfiopci,host=5e:00.2,id=hostdev0,bus=root1,failover_pair_id=net1
So it should be also possible to do similar to "hide" VFs and
realize/unrealize them when the guest requests.
There are two things I hate with this idea when contrasting it with
the conventional multifunction feature[6] though. One is that the PF
must be added before VFs; a similar limitation is imposed for failover.
Another is that it will be specific to virtio-net. I was considering
to implement a "generic" SR-IOV feature that will work on various
devices, but I realized that will need lots of configuration
validations. We may eventually want it, but probably it's better to
avoid such a big leap as the first step.
Please tell me if you have questions or suggestions.
Hi, Odaki-san
Hi,
The idea appears to be practical and convenient.
I have some things I want to confirm.
I understood your idea can make deices for VFs,
created by qdev_new or qdev_realize function, invisible from guest OS.
Is my understanding correct ?
Yes, the guest will request to enable VFs with the standard SR-IOV
capability, and the virtio-net implementation will use appropriate
QEMU-internal APIs to create and realize VFs accordingly.
And, if your idea is realized,
will it be possible to specify the backend device for the virtio-pci-net
device ?
Yes, you can specify netdev like conventional virtio-net devices.
Could you provide insights into the next steps
beyond the implementation details ?
About when do you expect your implementation
to be merged into qemu ?
Do you have a timeline for this plan ?
Moreover, is there any way
we can collaborate on the implementation you're planning ?
I intend to upstream my implementation. The flexibility of this design
will make the SR-IOV support useful for many people and make it suitable
for upstreaming. I also expect the implementation will be clean enough
for upstreaming. I'll submit it to the mailing list when I finish the
implementation so I'd like you to test and review it.
By the way, I started the implementation and realized it may be better
to change the design so I present the design changes below:
First I intend to change the CLI. The interface in my last proposal
expects there is only one PF in a bus and it is marked with "sriov"
property. However, the specification allows to have multiple PFs in a
bus so it's better to design the CLI so that it allows to have multiple
PFs though I'm not going to implement such a feature at first.
The new CLI will instead add "sriov-pf" property to VFs, which
designates the PF paired with them. The below is an example of a command
line conforming to the new interface:
-device virtio-net-pci,addr=0x0.0x3,netdev=tap3,sriov-pf=pf1
-device virtio-net-pci,addr=0x0.0x2,netdev=tap2,id=pf1
-device virtio-net-pci,addr=0x0.0x1,netdev=tap1,sriov-pf=pf0
-device virtio-net-pci,addr=0x0.0x0,netdev=tap0,id=pf0
Another design change is *not* to use the "device hiding" API of
failover. It is because fully-realized devices are useful when
validating the configuration. In particular, VFs must have a consistent
BAR configuration, and that can be validated only after they are realized.
So I'm now considering to have "prototype VFs" realized before the PF
Re: [PATCH v7 2/8] machine: Introduce helper is_cpu_type_supported()
Hi Gavin,
On 27/11/23 00:12, Gavin Shan wrote:
The logic, to check if the specified CPU type is supported in
machine_run_board_init(), is independent enough. Factor it out into
helper is_cpu_type_supported(). machine_run_board_init() looks a bit
clean with this. Since we're here, @machine_class is renamed to @mc
to avoid multiple line spanning of code. The error messages and comments
are tweaked a bit either.
No functional change intended.
Signed-off-by: Gavin Shan
---
hw/core/machine.c | 90 +++
1 file changed, 51 insertions(+), 39 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index b3ef325936..05e1922b89 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1387,13 +1387,57 @@ out:
return r;
}
+static void is_cpu_type_supported(const MachineState *machine, Error **errp)
Functions taking an Error** last argument should return a boolean value.
+{
+MachineClass *mc = MACHINE_GET_CLASS(machine);
+ObjectClass *oc = object_class_by_name(machine->cpu_type);
+CPUClass *cc;
+int i;
+
+/*
+ * Check if the user specified CPU type is supported when the valid
+ * CPU types have been determined. Note that the user specified CPU
+ * type is provided through '-cpu' option.
+ */
+if (mc->valid_cpu_types && machine->cpu_type) {
+for (i = 0; mc->valid_cpu_types[i]; i++) {
+if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
+break;
+}
+}
+
+/* The user specified CPU type isn't valid */
+if (!mc->valid_cpu_types[i]) {
+error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
+if (!mc->valid_cpu_types[1]) {
+error_append_hint(errp, "The only valid type is: %s",
+ mc->valid_cpu_types[0]);
+} else {
+error_append_hint(errp, "The valid types are: %s",
+ mc->valid_cpu_types[0]);
+}
+
+for (i = 1; mc->valid_cpu_types[i]; i++) {
+error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
+}
+
+error_append_hint(errp, "\n");
+return;
+}
+}
+
+/* Check if CPU type is deprecated and warn if so */
+cc = CPU_CLASS(oc);
+if (cc && cc->deprecation_note) {
+warn_report("CPU model %s is deprecated -- %s",
+machine->cpu_type, cc->deprecation_note);
Why did you move the deprecation warning within the is_supported check?
+}
+}
void machine_run_board_init(MachineState *machine, const char *mem_path, Error **errp)
{
ERRP_GUARD();
MachineClass *machine_class = MACHINE_GET_CLASS(machine);
-ObjectClass *oc = object_class_by_name(machine->cpu_type);
-CPUClass *cc;
Error *local_err = NULL;
/* This checkpoint is required by replay to separate prior clock
@@ -1449,43 +1493,11 @@ void machine_run_board_init(MachineState *machine,
const char *mem_path, Error *
machine->ram = machine_consume_memdev(machine, machine->memdev);
}
-/* If the machine supports the valid_cpu_types check and the user
- * specified a CPU with -cpu check here that the user CPU is supported.
- */
-if (machine_class->valid_cpu_types && machine->cpu_type) {
-int i;
-
-for (i = 0; machine_class->valid_cpu_types[i]; i++) {
-if (object_class_dynamic_cast(oc,
- machine_class->valid_cpu_types[i])) {
-/* The user specified CPU is in the valid field, we are
- * good to go.
- */
-break;
-}
-}
-
-if (!machine_class->valid_cpu_types[i]) {
-/* The user specified CPU is not valid */
-error_setg(&local_err, "Invalid CPU type: %s", machine->cpu_type);
-error_append_hint(&local_err, "The valid types are: %s",
- machine_class->valid_cpu_types[0]);
-for (i = 1; machine_class->valid_cpu_types[i]; i++) {
-error_append_hint(&local_err, ", %s",
- machine_class->valid_cpu_types[i]);
-}
-error_append_hint(&local_err, "\n");
-
-error_propagate(errp, local_err);
-return;
-}
-}
-
-/* Check if CPU type is deprecated and warn if so */
-cc = CPU_CLASS(oc);
-if (cc && cc->deprecation_note) {
-warn_report("CPU model %s is deprecated -- %s", machine->cpu_type,
-cc->deprecation_note);
+/* Check if the CPU type is supported */
+is_cpu_type_supported(machine, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
This becomes:
if (!is_cpu_type_supported(machine, errp)) {
+return;
}
if (machine->cgs) {
Re: [PATCH v7 1/8] machine: Use error handling when CPU type is checked
Hi Gavin,
On 27/11/23 00:12, Gavin Shan wrote:
QEMU will be terminated if the specified CPU type isn't supported
in machine_run_board_init(). The list of supported CPU type names
is tracked by mc->valid_cpu_types.
The error handling can be used to propagate error messages, to be
consistent how the errors are handled for other situations in the
same function.
No functional change intended.
Suggested-by: Igor Mammedov
Signed-off-by: Gavin Shan
---
v7: Add 'return' after error_propagate() to avoid calling into
mc->init() in the failing case(Marcin)
---
hw/core/machine.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 0c17398141..b3ef325936 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1394,6 +1394,7 @@ void machine_run_board_init(MachineState *machine, const
char *mem_path, Error *
MachineClass *machine_class = MACHINE_GET_CLASS(machine);
ObjectClass *oc = object_class_by_name(machine->cpu_type);
CPUClass *cc;
+Error *local_err = NULL;
IIUC the big comment in "include/qapi/error.h", since commit ae7c80a7bd
("error: New macro ERRP_GUARD()") we want to use the new macro instead.
/* This checkpoint is required by replay to separate prior clock
reading from the other reads, because timer polling functions query
@@ -1466,15 +1467,17 @@ void machine_run_board_init(MachineState *machine,
const char *mem_path, Error *
if (!machine_class->valid_cpu_types[i]) {
/* The user specified CPU is not valid */
-error_report("Invalid CPU type: %s", machine->cpu_type);
-error_printf("The valid types are: %s",
- machine_class->valid_cpu_types[0]);
+error_setg(&local_err, "Invalid CPU type: %s", machine->cpu_type);
+error_append_hint(&local_err, "The valid types are: %s",
+ machine_class->valid_cpu_types[0]);
for (i = 1; machine_class->valid_cpu_types[i]; i++) {
-error_printf(", %s", machine_class->valid_cpu_types[i]);
+error_append_hint(&local_err, ", %s",
+ machine_class->valid_cpu_types[i]);
}
-error_printf("\n");
+error_append_hint(&local_err, "\n");
-exit(1);
+error_propagate(errp, local_err);
+return;
}
}
Re: [PATCH v7 3/8] machine: Print CPU model name instead of CPU type
Hi Gavin,
On 27/11/23 00:12, Gavin Shan wrote:
The names of supported CPU models instead of CPU types should be
printed when the user specified CPU type isn't supported, to be
consistent with the output from '-cpu ?'.
Correct the error messages to print CPU model names instead of CPU
type names.
Signed-off-by: Gavin Shan
---
hw/core/machine.c | 21 +++--
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 05e1922b89..898c25552a 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1392,6 +1392,7 @@ static void is_cpu_type_supported(const MachineState
*machine, Error **errp)
MachineClass *mc = MACHINE_GET_CLASS(machine);
ObjectClass *oc = object_class_by_name(machine->cpu_type);
CPUClass *cc;
+char *model;
int i;
/*
@@ -1408,17 +1409,25 @@ static void is_cpu_type_supported(const MachineState
*machine, Error **errp)
/* The user specified CPU type isn't valid */
if (!mc->valid_cpu_types[i]) {
-error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
+model = cpu_model_from_type(machine->cpu_type);
+g_assert(model != NULL);
+error_setg(errp, "Invalid CPU type: %s", model);
+g_free(model);
g_autofree char *requested = cpu_model_from_type(machine->cpu_type);
error_setg(errp, "Invalid CPU type: %s", requested);
+
+model = cpu_model_from_type(mc->valid_cpu_types[0]);
+g_assert(model != NULL);
if (!mc->valid_cpu_types[1]) {
-error_append_hint(errp, "The only valid type is: %s",
- mc->valid_cpu_types[0]);
+error_append_hint(errp, "The only valid type is: %s", model);
g_autofree char *model = cpu_model_from_type(mc->valid_cpu_types[0]);
error_append_hint(errp, "The only valid type is: %s\n", model);
} else {
-error_append_hint(errp, "The valid types are: %s",
- mc->valid_cpu_types[0]);
+error_append_hint(errp, "The valid types are: %s", model);
Please move all the enumeration in this ladder, this makes the logic
simpler to follow:
error_append_hint(errp, "The valid types are: ");
for (i = 0; mc->valid_cpu_types[i]; i++) {
g_autofree char *model =
cpu_model_from_type(mc->valid_cpu_types[i]);
error_append_hint(errp, ", %s", model);
}
error_append_hint(errp, "\n");
}
+g_free(model);
for (i = 1; mc->valid_cpu_types[i]; i++) {
-error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
+model = cpu_model_from_type(mc->valid_cpu_types[i]);
+g_assert(model != NULL);
+error_append_hint(errp, ", %s", model);
+g_free(model);
}
error_append_hint(errp, "\n");
Re: [PATCH] 'channel' and 'addr' in qmp_migrate() are not auto-freed. migrate_uri_parse() allocates memory which is returned to 'channel', which is leaked because there is no code for freeing 'channel
Het Gala writes:
> On 28/11/23 12:46 pm, Markus Armbruster wrote:
>> Your commit message is all in one line. You need to format it like
>>
>> migration: Plug memory leak
>>
>> 'channel' and 'addr' in qmp_migrate() are not auto-freed.
>> migrate_uri_parse() allocates memory which is returned to 'channel',
>> which is leaked because there is no code for freeing 'channel' or
>> 'addr'. So, free addr and channel to avoid memory leak. 'addr'
>> does shallow copying of channel->addr, hence free 'channel' itself
>> and deep free contents of 'addr'.
>>
>> Het Gala writes:
> Yeah, I made the changes in v2 patchset.
>>> ---
>>> migration/migration.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index 28a34c9068..29efb51b62 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -2004,6 +2004,8 @@ void qmp_migrate(const char *uri, bool has_channels,
>>> MIGRATION_STATUS_FAILED);
>>> block_cleanup_parameters();
>>> }
>>> +g_free(channel);
>>> +qapi_free_MigrationAddress(addr);
>>> if (local_err) {
>>> if (!resume_requested) {
>> 2. hmp_migrate()
>>
>> hmp_migrate() allocates @channel with migrate_uri_parse(), adds it to
>> list @caps, passes @caps to qmp_migrate(), then frees @caps with
>> qapi_free_MigrationChannelList().
>
> Markus, sorry if I was not able to put point clearly, what I meant is that
> the local 'channel' variable used in qmp_migrate() i.e.
>
> 'MigrationChannel *channel = NULL', is defined in qmp_migrate() and if the
> user opts for 'uri' then '@channels' coming from hmp_migrate() will be NULL,
> and then migrate_uri_parse() will populate memory into 'channel', and that is
> not getting freed after it's use is over.
>
> I think, that is where memory leak might be happening ?
Aha!
if (uri && has_channels) {
error_setg(errp, "'uri' and 'channels' arguments are mutually "
"exclusive; exactly one of the two should be present in "
"'migrate' qmp command ");
return;
} else if (channels) {
/* To verify that Migrate channel list has only item */
if (channels->next) {
error_setg(errp, "Channel list has more than one entries");
return;
}
channel = channels->value;
} else if (uri) {
/* caller uses the old URI syntax */
if (!migrate_uri_parse(uri, &channel, errp)) {
return;
}
} else {
error_setg(errp, "neither 'uri' or 'channels' argument are "
"specified in 'migrate' qmp command ");
return;
}
At this point, @channel is either channels->value, or from
migrate_uri_parse().
We must not free in the former case, we must free in the latter case,
Before your patch, we don't free. Memory leak in the latter case.
Afterwards, we free. Double-free in the former case.
You could guard the free, like so:
if (uri) {
qapi_free_MigrationChannel(channel);
}
By the way, I the conditional shown above is harder to understand than
necessary. I like to get the errors out of the way at the beginning,
like this:
if (uri && has_channels) {
error_setg(errp, "'uri' and 'channels' arguments are mutually "
"exclusive; exactly one of the two should be present in "
"'migrate' qmp command ");
return;
}
if (!uri && !has_channels) {
error_setg(errp, "neither 'uri' or 'channels' argument are "
"specified in 'migrate' qmp command ");
return;
}
if (channels) {
/* To verify that Migrate channel list has only item */
Or even
if (!uri == !has_channels) {
error_setg(errp, "need either 'uri' or 'channels' argument")
return;
}
Suggestion, not demand. If you do it, separate patch.
Re: [PATCH 1/2] hw/cpu/core: Cleanup unused included header in core.c
On 27/11/23 15:56, Zhao Liu wrote: From: Zhao Liu Remove unused header (qemu/module.h and sysemu/cpus.h) in core.c, and reorder the remaining header files (except qemu/osdep.h) in alphabetical order. Tested by "./configure" and then "make". Signed-off-by: Zhao Liu --- hw/cpu/core.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 2/2] hw/cpu/cluster: Cleanup unused included header in cluster.c
On 27/11/23 15:56, Zhao Liu wrote: From: Zhao Liu Remove unused header (qemu/module.h and qemu/cutils.h) in cluster.c, and reorder the remaining header files (except qemu/osdep.h) in alphabetical order. Tested by "./configure" and then "make". Signed-off-by: Zhao Liu --- hw/cpu/cluster.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH for-9.0 v2 4/8] target/riscv/cpu.c: add riscv_cpu_is_32bit()
On 27/11/23 12:37, Daniel Henrique Barboza wrote: Next patch will need to retrieve if a given RISCVCPU is 32 or 64 bit. The existing helper riscv_is_32bit() (hw/riscv/boot.c) will always check the first CPU of a given hart array, not any given CPU. Create a helper to retrieve the info for any given CPU, not the first CPU of the hart array. The helper is using the same 32 bit check that riscv_cpu_satp_mode_finalize() was doing. Signed-off-by: Daniel Henrique Barboza Reviewed-by: Andrew Jones --- target/riscv/cpu.c | 7 ++- target/riscv/cpu.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH] accel/kvm: Turn DPRINTF macro use into tracepoints
On Mon, 27 Nov 2023 at 19:44, Jai Arora wrote:
>
> To remove DPRINTF macros and use tracepoints
> for logging.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1827
>
> Signed-off-by: Jai Arora
> ---
> accel/kvm/kvm-all.c| 32 ++--
> accel/kvm/trace-events | 2 +-
> 2 files changed, 11 insertions(+), 23 deletions(-)
Hi; thanks for sending in this patch.
(I've CC'd the KVM maintainer.)
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index e39a810a4e..d0dd7e54c3 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -69,16 +69,6 @@
> #define KVM_GUESTDBG_BLOCKIRQ 0
> #endif
>
> -//#define DEBUG_KVM
> -
> -#ifdef DEBUG_KVM
> -#define DPRINTF(fmt, ...) \
> -do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
> -#else
> -#define DPRINTF(fmt, ...) \
> -do { } while (0)
> -#endif
> -
> struct KVMParkedVcpu {
> unsigned long vcpu_id;
> int kvm_fd;
> @@ -331,7 +321,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
> struct KVMParkedVcpu *vcpu = NULL;
> int ret = 0;
>
> -DPRINTF("kvm_destroy_vcpu\n");
> +trace_kvm_dprintf("kvm_destroy_vcpu\n");
Rather than using a single tracepoint for every line
that was previously a DPRINTF, it's preferable to
use tracepoints whose name indicates what they're doing.
Users can turn tracepoints on and off individually, and they
might, for instance, only want to trace when the vcpu is
destroyed and not also the very large number of events
that will happen for kvm_cpu_exec entry/exits.
If you look at the current set of events in accel/kvm/trace-events
you can see the general style: the tracepoint name itself
tells you the "what has happened", the arguments provide
more info (eg which CPU did we just destroy), and it's
rarely necessary to pass a string into the tracepoint function.
thanks
-- PMM
[RFC PATCH v2 05/10] vdpa: factor out stop path of vhost_vdpa_dev_start
This makes easier to build an error path in next patches. No functional
change.
Signed-off-by: Eugenio Pérez
---
hw/virtio/vhost-vdpa.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 57a8043cd4..449c3794b2 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1302,7 +1302,7 @@ static void vhost_vdpa_suspend(struct vhost_dev *dev)
static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
{
struct vhost_vdpa *v = dev->opaque;
-bool ok;
+bool ok = true;
trace_vhost_vdpa_dev_start(dev, started);
if (started) {
@@ -1313,8 +1313,7 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev,
bool started)
}
} else {
vhost_vdpa_suspend(dev);
-vhost_vdpa_svqs_stop(dev);
-vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
+goto out_stop;
}
if (dev->vq_index + dev->nvqs != dev->vq_index_end) {
@@ -1333,6 +1332,11 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev,
bool started)
}
return 0;
+
+out_stop:
+vhost_vdpa_svqs_stop(dev);
+vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
+return ok ? 0 : -1;
}
static void vhost_vdpa_reset_status(struct vhost_dev *dev)
--
2.39.3
[RFC PATCH v2 09/10] vdpa: add vhost_vdpa_net_load_setup NetClient callback
So the vDPA backend knows when a migration incoming starts. NicState
argument is needed so we can get the dma address space.
Signed-off-by: Eugenio Pérez
---
RFC v2:
* Solve git conflict with .set_steering_ebpf
* Fix x-svq=on use case which did not allocated iova_tree.
---
include/net/net.h | 6 ++
net/vhost-vdpa.c | 33 +
2 files changed, 39 insertions(+)
diff --git a/include/net/net.h b/include/net/net.h
index ffbd2c8d56..68282dde31 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -42,6 +42,7 @@ typedef struct NICConf {
/* Net clients */
+struct NICState;
typedef void (NetPoll)(NetClientState *, bool enable);
typedef bool (NetCanReceive)(NetClientState *);
typedef int (NetStart)(NetClientState *);
@@ -69,6 +70,9 @@ typedef void (SocketReadStateFinalize)(SocketReadState *rs);
typedef void (NetAnnounce)(NetClientState *);
typedef bool (SetSteeringEBPF)(NetClientState *, int);
typedef bool (NetCheckPeerType)(NetClientState *, ObjectClass *, Error **);
+/* This can be called before start & pair, so get also the peer */
+typedef int (NetMigrationLoadSetup)(NetClientState *, struct NICState *);
+typedef int (NetMigrationLoadCleanup)(NetClientState *, struct NICState *);
typedef struct NetClientInfo {
NetClientDriver type;
@@ -98,6 +102,8 @@ typedef struct NetClientInfo {
NetAnnounce *announce;
SetSteeringEBPF *set_steering_ebpf;
NetCheckPeerType *check_peer_type;
+NetMigrationLoadSetup *load_setup;
+NetMigrationLoadCleanup *load_cleanup;
} NetClientInfo;
struct NetClientState {
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index a37de7860e..90f41280d2 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -407,6 +407,37 @@ static void vhost_vdpa_net_client_stop(NetClientState *nc)
}
}
+static int vhost_vdpa_net_load_setup(NetClientState *nc, NICState *nic)
+{
+VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
+VirtIONet *n = qemu_get_nic_opaque(&nic->ncs[0]);
+VhostVDPAShared *shared = s->vhost_vdpa.shared;
+int r;
+
+if (s->always_svq) {
+/* iova tree is needed because of SVQ */
+shared->iova_tree = vhost_iova_tree_new(shared->iova_range.first,
+shared->iova_range.last);
+}
+
+r = vhost_vdpa_load_setup(shared, n->parent_obj.dma_as);
+if (unlikely(r < 0)) {
+g_clear_pointer(&s->vhost_vdpa.shared->iova_tree,
+vhost_iova_tree_delete);
+}
+
+return r;
+}
+
+static int vhost_vdpa_net_load_cleanup(NetClientState *nc, NICState *nic)
+{
+VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
+VirtIONet *n = qemu_get_nic_opaque(&nic->ncs[0]);
+
+return vhost_vdpa_load_cleanup(s->vhost_vdpa.shared,
+ n->parent_obj.status & VIRTIO_CONFIG_S_DRIVER_OK);
+}
+
static NetClientInfo net_vhost_vdpa_info = {
.type = NET_CLIENT_DRIVER_VHOST_VDPA,
.size = sizeof(VhostVDPAState),
@@ -419,6 +450,8 @@ static NetClientInfo net_vhost_vdpa_info = {
.has_ufo = vhost_vdpa_has_ufo,
.check_peer_type = vhost_vdpa_check_peer_type,
.set_steering_ebpf = vhost_vdpa_set_steering_ebpf,
+.load_setup = vhost_vdpa_net_load_setup,
+.load_cleanup = vhost_vdpa_net_load_cleanup,
};
static int64_t vhost_vdpa_get_vring_group(int device_fd, unsigned vq_index,
--
2.39.3
[RFC PATCH v2 03/10] vdpa: merge _begin_batch into _batch_begin_once
There was only one call. This way we can make the begin and end of the
batch symmetrical.
Signed-off-by: Eugenio Pérez
---
hw/virtio/vhost-vdpa.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index bf9771870a..a533fc5bc7 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -143,7 +143,7 @@ int vhost_vdpa_dma_unmap(VhostVDPAShared *s, uint32_t asid,
hwaddr iova,
return ret;
}
-static void vhost_vdpa_listener_begin_batch(VhostVDPAShared *s)
+static void vhost_vdpa_iotlb_batch_begin_once(VhostVDPAShared *s)
{
int fd = s->device_fd;
struct vhost_msg_v2 msg = {
@@ -151,21 +151,16 @@ static void
vhost_vdpa_listener_begin_batch(VhostVDPAShared *s)
.iotlb.type = VHOST_IOTLB_BATCH_BEGIN,
};
-trace_vhost_vdpa_listener_begin_batch(s, fd, msg.type, msg.iotlb.type);
-if (write(fd, &msg, sizeof(msg)) != sizeof(msg)) {
-error_report("failed to write, fd=%d, errno=%d (%s)",
- fd, errno, strerror(errno));
-}
-}
-
-static void vhost_vdpa_iotlb_batch_begin_once(VhostVDPAShared *s)
-{
if (!(s->backend_cap & (0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH)) ||
s->iotlb_batch_begin_sent) {
return;
}
-vhost_vdpa_listener_begin_batch(s);
+trace_vhost_vdpa_listener_begin_batch(s, fd, msg.type, msg.iotlb.type);
+if (write(fd, &msg, sizeof(msg)) != sizeof(msg)) {
+error_report("failed to write, fd=%d, errno=%d (%s)",
+ fd, errno, strerror(errno));
+}
s->iotlb_batch_begin_sent = true;
}
--
2.39.3
[RFC PATCH v2 07/10] vdpa: set backend capabilities at vhost_vdpa_init
The backend does not reset them until the vdpa file descriptor is closed
so there is no harm in doing it only once.
This allows the destination of a live migration to premap memory in
batches, using VHOST_BACKEND_F_IOTLB_BATCH.
Signed-off-by: Eugenio Pérez
---
hw/virtio/vhost-vdpa.c | 50 --
1 file changed, 19 insertions(+), 31 deletions(-)
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 449c3794b2..43f7c382b1 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -587,11 +587,25 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void
*opaque, Error **errp)
struct vhost_vdpa *v = opaque;
assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
trace_vhost_vdpa_init(dev, v->shared, opaque);
+uint64_t backend_features;
+uint64_t qemu_backend_features = 0x1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2 |
+ 0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH |
+ 0x1ULL << VHOST_BACKEND_F_IOTLB_ASID |
+ 0x1ULL << VHOST_BACKEND_F_SUSPEND;
int ret;
v->dev = dev;
dev->opaque = opaque ;
v->shared->listener = vhost_vdpa_memory_listener;
+
+if (vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, &backend_features)) {
+return -EFAULT;
+}
+
+backend_features &= qemu_backend_features;
+
+dev->backend_cap = backend_features;
+v->shared->backend_cap = backend_features;
vhost_vdpa_init_svq(dev, v);
error_propagate(&dev->migration_blocker, v->migration_blocker);
@@ -599,6 +613,11 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void
*opaque, Error **errp)
return 0;
}
+ret = vhost_vdpa_call(dev, VHOST_SET_BACKEND_FEATURES, &backend_features);
+if (ret) {
+return -EFAULT;
+}
+
/*
* If dev->shadow_vqs_enabled at initialization that means the device has
* been started with x-svq=on, so don't block migration
@@ -829,36 +848,6 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev,
return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
}
-static int vhost_vdpa_set_backend_cap(struct vhost_dev *dev)
-{
-struct vhost_vdpa *v = dev->opaque;
-
-uint64_t features;
-uint64_t f = 0x1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2 |
-0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH |
-0x1ULL << VHOST_BACKEND_F_IOTLB_ASID |
-0x1ULL << VHOST_BACKEND_F_SUSPEND;
-int r;
-
-if (vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, &features)) {
-return -EFAULT;
-}
-
-features &= f;
-
-if (vhost_vdpa_first_dev(dev)) {
-r = vhost_vdpa_call(dev, VHOST_SET_BACKEND_FEATURES, &features);
-if (r) {
-return -EFAULT;
-}
-}
-
-dev->backend_cap = features;
-v->shared->backend_cap = features;
-
-return 0;
-}
-
static int vhost_vdpa_get_device_id(struct vhost_dev *dev,
uint32_t *device_id)
{
@@ -1512,7 +1501,6 @@ const VhostOps vdpa_ops = {
.vhost_set_vring_kick = vhost_vdpa_set_vring_kick,
.vhost_set_vring_call = vhost_vdpa_set_vring_call,
.vhost_get_features = vhost_vdpa_get_features,
-.vhost_set_backend_cap = vhost_vdpa_set_backend_cap,
.vhost_set_owner = vhost_vdpa_set_owner,
.vhost_set_vring_endian = NULL,
.vhost_backend_memslots_limit = vhost_vdpa_memslots_limit,
--
2.39.3
[RFC PATCH v2 06/10] vdpa: check for iova tree initialized at net_client_start
To map the guest memory while it is migrating we need to create the
iova_tree, as long as the destination uses x-svq=on. Checking to not
override it.
The function vhost_vdpa_net_client_stop clear it if the device is
stopped. If the guest starts the device again, the iova tree is
recreated by vhost_vdpa_net_data_start_first or vhost_vdpa_net_cvq_start
if needed, so old behavior is kept.
Signed-off-by: Eugenio Pérez
---
net/vhost-vdpa.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 3fb209cd35..a37de7860e 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -341,7 +341,9 @@ static void vhost_vdpa_net_data_start_first(VhostVDPAState
*s)
migration_add_notifier(&s->migration_state,
vdpa_net_migration_state_notifier);
-if (v->shadow_vqs_enabled) {
+
+/* iova_tree may be initialized by vhost_vdpa_net_load_setup */
+if (v->shadow_vqs_enabled && !v->shared->iova_tree) {
v->shared->iova_tree = vhost_iova_tree_new(v->shared->iova_range.first,
v->shared->iova_range.last);
}
--
2.39.3
[RFC PATCH v2 00/10] Map memory at destination .load_setup in vDPA-net migration
Current memory operations like pinning may take a lot of time at the destination. Currently they are done after the source of the migration is stopped, and before the workload is resumed at the destination. This is a period where neigher traffic can flow, nor the VM workload can continue (downtime). We can do better as we know the memory layout of the guest RAM at the destination from the moment the migration starts. Moving that operation allows QEMU to communicate the kernel the maps while the workload is still running in the source, so Linux can start mapping them. Also, the destination of the guest memory may finish before the destination QEMU maps all the memory. In this case, the rest of the memory will be mapped at the same time as before applying this series, when the device is starting. So we're only improving with this series. RFC TODO: We should be able to not finish the migration while the memory is still not mapped, but I still need to find how. Suggestions are welcome. Note that further devices setup at the end of the migration may alter the guest memory layout. But same as the previous point, many operations are still done incrementally, like memory pinning, so we're saving time anyway. Only tested with vdpa_sim. I'm sending this before full benchmark, as some work like [1] can be based on it, and Si-Wei agreed on benchmark this series with his experience. This needs to be applied on top of [2], which perform some code reorganization that allows to map the memory without knowing the queue layout the guest configure on the device. Future directions on top of this series may include: * Iterative migration of virtio-net devices, as it may reduce downtime per [1]. vhost-vdpa net can apply the configuration through CVQ in the destination while the source is still migrating. * Move more things ahead of migration time, like DRIVER_OK. * Check that the devices of the destination are valid, and cancel the migration in case it is not. RFC v2: * Delegate map to another thread so it does no block QMP. * Fix not allocating iova_tree if x-svq=on at the destination. * Rebased on latest master. * More cleanups of current code, that might be split from this series too. [1] https://lore.kernel.org/qemu-devel/6c8ebb97-d546-3f1c-4cdd-54e23a566...@nvidia.com/T/ [2] https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg05331.html Eugenio Pérez (10): vdpa: do not set virtio status bits if unneeded vdpa: make batch_begin_once early return vdpa: merge _begin_batch into _batch_begin_once vdpa: extract out _dma_end_batch from _listener_commit vdpa: factor out stop path of vhost_vdpa_dev_start vdpa: check for iova tree initialized at net_client_start vdpa: set backend capabilities at vhost_vdpa_init vdpa: add vhost_vdpa_load_setup vdpa: add vhost_vdpa_net_load_setup NetClient callback virtio_net: register incremental migration handlers include/hw/virtio/vhost-vdpa.h | 25 include/net/net.h | 6 + hw/net/virtio-net.c| 35 + hw/virtio/vhost-vdpa.c | 257 +++-- net/vhost-vdpa.c | 37 - 5 files changed, 312 insertions(+), 48 deletions(-) -- 2.39.3
[RFC PATCH v2 08/10] vdpa: add vhost_vdpa_load_setup
Callers can use this function to setup the incoming migration thread.
This thread is able to map the guest memory while the migration is
ongoing, without blocking QMP or other important tasks. While this
allows the destination QEMU not to block, it expands the mapping time
during migration instead of making it pre-migration.
This thread joins at vdpa backend device start, so it could happen that
the guest memory is so large that we still have guest memory to map
before this time. This can be improved in later iterations, when the
destination device can inform QEMU that it is not ready to complete the
migration.
If the device is not started, the clean of the mapped memory is done at
.load_cleanup. This is far from ideal, as the destination machine has
mapped all the guest ram for nothing, and now it needs to unmap it.
However, we don't have information about the state of the device so its
the best we can do. Once iterative migration is supported, this will be
improved as we know the virtio state of the device.
TODO RFC: if the VM migrates before finishing all the maps, the source
will stop but the destination is still not ready to continue, and it
will wait until all guest RAM is mapped. It is still an improvement
over doing all the map when the migration finish, but it should be easy
to forbid the guest to stop until a condition is met.
TODO RFC: The memory unmapping if the device is not started is weird
too, as ideally nothing would be mapped. This can be fixed when we
migrate the device state iteratively, and we know for sure if the device
is started or not. At this moment we don't have such information so
there is no better alternative.
Other options considered:
* Coroutines: Overkill? What thread can I assign them, as vdpa does not
have any dedicated iothread for the moment?
* QemuEvent or Mutex + Cond: Need to synchronize list access
then, complicating the synchronization. As maps ops are heavier enough,
it is not worth.
Signed-off-by: Eugenio Pérez
---
RFC v2:
* Use a dedicated thread for map instead of doing all in .load_setup,
blocking QMP etc.
Signed-off-by: Eugenio Pérez
---
include/hw/virtio/vhost-vdpa.h | 25 +
hw/virtio/vhost-vdpa.c | 167 -
2 files changed, 191 insertions(+), 1 deletion(-)
diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
index 8f54e5edd4..6533ad211c 100644
--- a/include/hw/virtio/vhost-vdpa.h
+++ b/include/hw/virtio/vhost-vdpa.h
@@ -17,6 +17,7 @@
#include "hw/virtio/vhost-iova-tree.h"
#include "hw/virtio/vhost-shadow-virtqueue.h"
#include "hw/virtio/virtio.h"
+#include "qemu/thread.h"
#include "standard-headers/linux/vhost_types.h"
/*
@@ -43,8 +44,30 @@ typedef struct vhost_vdpa_shared {
/* Copy of backend features */
uint64_t backend_cap;
+/*
+ * Thread to map memory in QEMU incoming migration.
+ *
+ * Incoming migration calls devices ->load_setup in the main thread, but
+ * map operations can take a long time. This forbids the main thread to
+ * serve other requests like QMP.
+ *
+ * It works by fetching jobs from map_queue until it receives
+ * VhostVDPAShared, signalling the end of thread job. From that point,
+ * thread is joined and maps requests are synchronous again. These new
+ * maps are not served from main thread, so there is no danger there.
+ */
+QemuThread map_thread;
+GAsyncQueue *map_queue;
+bool map_thread_enabled;
+
bool iotlb_batch_begin_sent;
+/*
+ * The memory listener has been registered, so DMA maps have been sent to
+ * the device.
+ */
+bool listener_registered;
+
/* Vdpa must send shadow addresses as IOTLB key for data queues, not GPA */
bool shadow_data;
} VhostVDPAShared;
@@ -73,6 +96,8 @@ int vhost_vdpa_dma_map(VhostVDPAShared *s, uint32_t asid,
hwaddr iova,
hwaddr size, void *vaddr, bool readonly);
int vhost_vdpa_dma_unmap(VhostVDPAShared *s, uint32_t asid, hwaddr iova,
hwaddr size);
+int vhost_vdpa_load_setup(VhostVDPAShared *s, AddressSpace *dma_as);
+int vhost_vdpa_load_cleanup(VhostVDPAShared *s, bool vhost_will_start);
typedef struct vdpa_iommu {
VhostVDPAShared *dev_shared;
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 43f7c382b1..24844b5dfa 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -101,6 +101,15 @@ int vhost_vdpa_dma_map(VhostVDPAShared *s, uint32_t asid,
hwaddr iova,
msg.iotlb.perm = readonly ? VHOST_ACCESS_RO : VHOST_ACCESS_RW;
msg.iotlb.type = VHOST_IOTLB_UPDATE;
+if (s->map_thread_enabled && !qemu_thread_is_self(&s->map_thread)) {
+struct vhost_msg_v2 *new_msg = g_new(struct vhost_msg_v2, 1);
+
+*new_msg = msg;
+g_async_queue_push(s->map_queue, new_msg);
+
+return 0;
+}
+
trace_vhost_vdpa_dma_map(s, fd, msg.type, msg.asid, msg.iotlb.iova,
[RFC PATCH v2 02/10] vdpa: make batch_begin_once early return
Prefer early return so it is easier to merge
vhost_vdpa_listener_begin_batch here and make iotlb baches begin and end
symmetrical.
Signed-off-by: Eugenio Pérez
---
hw/virtio/vhost-vdpa.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index cc252fc2d8..bf9771870a 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -160,11 +160,12 @@ static void
vhost_vdpa_listener_begin_batch(VhostVDPAShared *s)
static void vhost_vdpa_iotlb_batch_begin_once(VhostVDPAShared *s)
{
-if (s->backend_cap & (0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH) &&
-!s->iotlb_batch_begin_sent) {
-vhost_vdpa_listener_begin_batch(s);
+if (!(s->backend_cap & (0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH)) ||
+s->iotlb_batch_begin_sent) {
+return;
}
+vhost_vdpa_listener_begin_batch(s);
s->iotlb_batch_begin_sent = true;
}
--
2.39.3
[RFC PATCH v2 01/10] vdpa: do not set virtio status bits if unneeded
Next commits will set DRIVER and ACKNOWLEDGE flags repeatedly in the
case of a migration destination. Let's save ioctls with this.
Signed-off-by: Eugenio Pérez
---
hw/virtio/vhost-vdpa.c | 4
1 file changed, 4 insertions(+)
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 7500c2fc82..cc252fc2d8 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -510,6 +510,10 @@ static int vhost_vdpa_add_status(struct vhost_dev *dev,
uint8_t status)
if (ret < 0) {
return ret;
}
+if ((s & status) == status) {
+/* Don't set bits already set */
+return 0;
+}
s |= status;
--
2.39.3
[RFC PATCH v2 04/10] vdpa: extract out _dma_end_batch from _listener_commit
So we can call out vhost_vdpa_dma_end_batch out of the listener
callbacks.
Signed-off-by: Eugenio Pérez
---
hw/virtio/vhost-vdpa.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index a533fc5bc7..57a8043cd4 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -164,9 +164,8 @@ static void
vhost_vdpa_iotlb_batch_begin_once(VhostVDPAShared *s)
s->iotlb_batch_begin_sent = true;
}
-static void vhost_vdpa_listener_commit(MemoryListener *listener)
+static void vhost_vdpa_dma_end_batch(VhostVDPAShared *s)
{
-VhostVDPAShared *s = container_of(listener, VhostVDPAShared, listener);
struct vhost_msg_v2 msg = {};
int fd = s->device_fd;
@@ -190,6 +189,13 @@ static void vhost_vdpa_listener_commit(MemoryListener
*listener)
s->iotlb_batch_begin_sent = false;
}
+static void vhost_vdpa_listener_commit(MemoryListener *listener)
+{
+VhostVDPAShared *s = container_of(listener, VhostVDPAShared, listener);
+
+vhost_vdpa_dma_end_batch(s);
+}
+
static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
{
struct vdpa_iommu *iommu = container_of(n, struct vdpa_iommu, n);
--
2.39.3
[RFC PATCH v2 10/10] virtio_net: register incremental migration handlers
This way VirtIONet can detect when the incoming migration starts.
While registering in the backend (nc->peer) seems more logical, we need
nic dma address space, and we cannot get it from the backend.
Signed-off-by: Eugenio Pérez
---
This could be done in vhost_vdpa or VirtIODevice struct, but future
series will add state restore through CVQ so it's easier to start in
VirtIONet directly. If we need to make this more generic, we can move
to VirtIODevice and expose callbacks from VirtIONet class.
Also, the pointer may not be the best id, but there are not a lot of
things initialized in n.
---
hw/net/virtio-net.c | 35 +++
1 file changed, 35 insertions(+)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 80c56f0cfc..374d0b4ec8 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -38,6 +38,7 @@
#include "qapi/qapi-events-migration.h"
#include "hw/virtio/virtio-access.h"
#include "migration/misc.h"
+#include "migration/register.h"
#include "standard-headers/linux/ethtool.h"
#include "sysemu/sysemu.h"
#include "trace.h"
@@ -3810,9 +3811,39 @@ static void virtio_net_device_unrealize(DeviceState *dev)
virtio_cleanup(vdev);
}
+static int virtio_net_load_setup(QEMUFile *f, void *opaque)
+{
+VirtIONet *n = opaque;
+NetClientState *nc = qemu_get_subqueue(n->nic, 0);
+
+if (nc->peer->info->load_setup) {
+return nc->peer->info->load_setup(nc->peer, n->nic);
+}
+
+return 0;
+}
+
+static int virtio_net_load_cleanup(void *opaque)
+{
+VirtIONet *n = opaque;
+NetClientState *nc = qemu_get_subqueue(n->nic, 0);
+
+if (nc->peer->info->load_cleanup) {
+return nc->peer->info->load_cleanup(nc->peer, n->nic);
+}
+
+return 0;
+}
+
+static const SaveVMHandlers savevm_virtio_net_handlers = {
+.load_setup = virtio_net_load_setup,
+.load_cleanup = virtio_net_load_cleanup,
+};
+
static void virtio_net_instance_init(Object *obj)
{
VirtIONet *n = VIRTIO_NET(obj);
+g_autoptr(GString) id = g_string_new(NULL);
/*
* The default config_size is sizeof(struct virtio_net_config).
@@ -3824,6 +3855,10 @@ static void virtio_net_instance_init(Object *obj)
DEVICE(n));
ebpf_rss_init(&n->ebpf_rss);
+
+g_string_printf(id, "%p", n);
+register_savevm_live(id->str, VMSTATE_INSTANCE_ID_ANY, 1,
+ &savevm_virtio_net_handlers, n);
}
static int virtio_net_pre_save(void *opaque)
--
2.39.3
Re: [PATCH for-8.2] ui/vnc-clipboard: fix inflate_buffer
Hi
On Mon, Nov 27, 2023 at 2:52 PM Fiona Ebner wrote:
>
> Am 27.11.23 um 10:15 schrieb Marc-André Lureau:
> >
> > It seems like a bug in tigervnc then. For some reason, the compressed
> > data doesn't trigger Z_STREAM_END on the decompression side. Have you
> > investigated or reported an issue to them?
> >
>
> This was with noVNC. A colleague tested with TigerVNC. I haven't stepped
> through with GDB there, but it might be similar. No, I haven't
> reported/investigated for the VNC clients yet. Unfortunately, I've got
> my hands full with other things at the moment, so it will be a while
> until I can do that.
>
> Even if it's a bug in the clients, this was working before d921fea338
> ("ui/vnc-clipboard: fix infinite loop in inflate_buffer
> (CVE-2023-3255)") so I still feel like it might be worth handling in QEMU.
>
> But is it really a client error? What I don't understand is why the
> return value of inflate() is Z_BUF_ERROR even though all the input was
> handled.
>
> From https://www.zlib.net/manual.html
>
> "inflate() returns [...] Z_BUF_ERROR if no progress was possible or if
> there was not enough room in the output buffer when Z_FINISH is used."
At the end of the input stream, subsequent calls could not make
progress. We never reached Z_STREAM_END
It seems to me the callers do not flush the streams with Z_FINISH
(https://github.com/TigerVNC/tigervnc/blob/master/common/rdr/ZlibOutStream.cxx),
and this is what marks the end of a zlib stream ultimately...
>
> > 51ret = inflate(&stream, Z_FINISH);
> > (gdb) p stream
> > $23 = {next_in = 0x57652708 "", avail_in = 5, total_in = 12, next_out =
> > 0x57627378 "", avail_out = 8, total_out = 8, msg = 0x0, state =
> > 0x578df5c0, zalloc = 0x77bc1560, zfree = 0x77bc1570,
> > opaque = 0x0, data_type = 5, adler = 71434672, reserved = 0}
> > (gdb) n
> > 52switch (ret) {
> > (gdb) p stream
> > $24 = {next_in = 0x5765270d "", avail_in = 0, total_in = 17, next_out =
> > 0x57627379 "", avail_out = 7, total_out = 9, msg = 0x0, state =
> > 0x578df5c0, zalloc = 0x77bc1560, zfree = 0x77bc1570,
> > opaque = 0x0, data_type = 128, adler = 99746224, reserved = 0}
> > (gdb) p ret
> > $25 = -5
> > (gdb) p out + 4
> > $26 = (uint8_t *) 0x57627374 "fish"
>
> Progress was made and there was enough space for the output (avail_out =
> 7 after the call), so it really shouldn't return Z_BUF_ERROR, right?
>
> zlib version is 1:1.2.13.dfsg-1 (Debian 12 Bookworm)
It's hard to make the best decision.
We could return the uncompressed data so far, that would fix the
regression. But potentially, we have incomplete data returned. Clients
should be fixed to include Z_STREAM_END marker (using Z_FINISH).
--
Marc-André Lureau
Re: [PATCH for-6.1 v6 14/17] accel/tcg: Move breakpoint recognition outside translation
Hi,
On 20/7/21 21:54, Richard Henderson wrote:
Trigger breakpoints before beginning translation of a TB
that would begin with a BP. Thus we never generate code
for the BP at all.
Single-step instructions within a page containing a BP so
that we are sure to check each insn for the BP as above.
We no longer need to flush any TBs when changing BPs.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/286
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/404
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/489
Reviewed-by: Alex Bennée
Signed-off-by: Richard Henderson
---
accel/tcg/cpu-exec.c | 91 --
accel/tcg/translator.c | 24 +--
cpu.c | 20 --
3 files changed, 89 insertions(+), 46 deletions(-)
diff --git a/cpu.c b/cpu.c
index 91d9e38acb..d6ae5ae581 100644
--- a/cpu.c
+++ b/cpu.c
@@ -225,11 +225,6 @@ void tb_invalidate_phys_addr(target_ulong addr)
tb_invalidate_phys_page_range(addr, addr + 1);
mmap_unlock();
}
-
-static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
-{
-tb_invalidate_phys_addr(pc);
-}
This patch removed the last use of tb_invalidate_phys_addr() in
user emulation:
void tb_invalidate_phys_addr(hwaddr addr)
{
mmap_lock();
tb_invalidate_phys_page(addr);
mmap_unlock();
}
Do we still need it?
(In sysemu there is a single use in Xtensa tb_invalidate_virtual_addr).
#else
void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr, MemTxAttrs attrs)
{
@@ -250,17 +245,6 @@ void tb_invalidate_phys_addr(AddressSpace *as, hwaddr
addr, MemTxAttrs attrs)
ram_addr = memory_region_get_ram_addr(mr) + addr;
tb_invalidate_phys_page_range(ram_addr, ram_addr + 1);
}
-
-static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
-{
-/*
- * There may not be a virtual to physical translation for the pc
- * right now, but there may exist cached TB for this pc.
- * Flush the whole TB cache to force re-translation of such TBs.
- * This is heavyweight, but we're debugging anyway.
- */
-tb_flush(cpu);
-}
#endif
Re: [PATCH v2 4/4] target/arm: Add support for DC CVAP & DC CVADP ins
Hi,
On 6/11/19 00:41, Beata Michalska wrote:
ARMv8.2 introduced support for Data Cache Clean instructions
to PoP (point-of-persistence) - DC CVAP and PoDP (point-of-deep-persistence)
- DV CVADP. Both specify conceptual points in a memory system where all writes
that are to reach them are considered persistent.
The support provided considers both to be actually the same so there is no
distinction between the two. If none is available (there is no backing store
for given memory) both will result in Data Cache Clean up to the point of
coherency. Otherwise sync for the specified range shall be performed.
Signed-off-by: Beata Michalska
---
linux-user/elfload.c | 2 ++
target/arm/cpu.h | 10 ++
target/arm/cpu64.c | 1 +
target/arm/helper.c | 56
4 files changed, 69 insertions(+)
+#ifndef CONFIG_USER_ONLY
+static void dccvap_writefn(CPUARMState *env, const ARMCPRegInfo *opaque,
+ uint64_t value)
+{
+ARMCPU *cpu = env_archcpu(env);
+/* CTR_EL0 System register -> DminLine, bits [19:16] */
+uint64_t dline_size = 4 << ((cpu->ctr >> 16) & 0xF);
+uint64_t vaddr_in = (uint64_t) value;
+uint64_t vaddr = vaddr_in & ~(dline_size - 1);
+void *haddr;
+int mem_idx = cpu_mmu_index(env, false);
+
+/* This won't be crossing page boundaries */
+haddr = probe_read(env, vaddr, dline_size, mem_idx, GETPC());
+if (haddr) {
+
+ram_addr_t offset;
+MemoryRegion *mr;
+
+/* RCU lock is already being held */
+mr = memory_region_from_host(haddr, &offset);
+
+if (mr) {
+memory_region_do_writeback(mr, offset, dline_size);
+}
+}
+}
+#ifndef CONFIG_USER_ONLY
+/* Data Cache clean instructions up to PoP */
+if (cpu_isar_feature(aa64_dcpop, cpu)) {
Am I correct understanding this is a TCG-only feature?
+define_one_arm_cp_reg(cpu, dcpop_reg);
+
+if (cpu_isar_feature(aa64_dcpodp, cpu)) {
+define_one_arm_cp_reg(cpu, dcpodp_reg);
+}
+}
+#endif /*CONFIG_USER_ONLY*/
#endif
/*
Re: [PATCH v2 4/4] target/arm: Add support for DC CVAP & DC CVADP ins
On Tue, 28 Nov 2023 at 11:24, Philippe Mathieu-Daudé wrote:
>
> Hi,
>
> On 6/11/19 00:41, Beata Michalska wrote:
> > ARMv8.2 introduced support for Data Cache Clean instructions
> > to PoP (point-of-persistence) - DC CVAP and PoDP (point-of-deep-persistence)
> > - DV CVADP. Both specify conceptual points in a memory system where all
> > writes
> > that are to reach them are considered persistent.
> > The support provided considers both to be actually the same so there is no
> > distinction between the two. If none is available (there is no backing store
> > for given memory) both will result in Data Cache Clean up to the point of
> > coherency. Otherwise sync for the specified range shall be performed.
> >
> > Signed-off-by: Beata Michalska
> > ---
> > linux-user/elfload.c | 2 ++
> > target/arm/cpu.h | 10 ++
> > target/arm/cpu64.c | 1 +
> > target/arm/helper.c | 56
> >
> > 4 files changed, 69 insertions(+)
>
>
> > +#ifndef CONFIG_USER_ONLY
> > +static void dccvap_writefn(CPUARMState *env, const ARMCPRegInfo *opaque,
> > + uint64_t value)
> > +{
> > +ARMCPU *cpu = env_archcpu(env);
> > +/* CTR_EL0 System register -> DminLine, bits [19:16] */
> > +uint64_t dline_size = 4 << ((cpu->ctr >> 16) & 0xF);
> > +uint64_t vaddr_in = (uint64_t) value;
> > +uint64_t vaddr = vaddr_in & ~(dline_size - 1);
> > +void *haddr;
> > +int mem_idx = cpu_mmu_index(env, false);
> > +
> > +/* This won't be crossing page boundaries */
> > +haddr = probe_read(env, vaddr, dline_size, mem_idx, GETPC());
> > +if (haddr) {
> > +
> > +ram_addr_t offset;
> > +MemoryRegion *mr;
> > +
> > +/* RCU lock is already being held */
> > +mr = memory_region_from_host(haddr, &offset);
> > +
> > +if (mr) {
> > +memory_region_do_writeback(mr, offset, dline_size);
> > +}
> > +}
> > +}
>
>
> > +#ifndef CONFIG_USER_ONLY
> > +/* Data Cache clean instructions up to PoP */
> > +if (cpu_isar_feature(aa64_dcpop, cpu)) {
>
> Am I correct understanding this is a TCG-only feature?
For KVM, whether the vCPU implements these cache
maintenance instructions is up to it -- like all insns,
QEMU doesn't ever see if the guest executes them or not
(either the host CPU just implements them, or the host
kernel traps and handles them). The code in this patch is
specifically for the QEMU TCG emulation of them.
thanks
-- PMM
Re: [PATCH] 'channel' and 'addr' in qmp_migrate() are not auto-freed. migrate_uri_parse() allocates memory which is returned to 'channel', which is leaked because there is no code for freeing 'channel
On 28/11/23 3:29 pm, Markus Armbruster wrote:
Het Gala writes:
On 28/11/23 12:46 pm, Markus Armbruster wrote:
Your commit message is all in one line. You need to format it like
migration: Plug memory leak
'channel' and 'addr' in qmp_migrate() are not auto-freed.
migrate_uri_parse() allocates memory which is returned to 'channel',
which is leaked because there is no code for freeing 'channel' or
'addr'. So, free addr and channel to avoid memory leak. 'addr'
does shallow copying of channel->addr, hence free 'channel' itself
and deep free contents of 'addr'.
Het Gala writes:
Yeah, I made the changes in v2 patchset.
---
migration/migration.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/migration/migration.c b/migration/migration.c
index 28a34c9068..29efb51b62 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2004,6 +2004,8 @@ void qmp_migrate(const char *uri, bool has_channels,
MIGRATION_STATUS_FAILED);
block_cleanup_parameters();
}
+g_free(channel);
+qapi_free_MigrationAddress(addr);
if (local_err) {
if (!resume_requested) {
2. hmp_migrate()
hmp_migrate() allocates @channel with migrate_uri_parse(), adds it to
list @caps, passes @caps to qmp_migrate(), then frees @caps with
qapi_free_MigrationChannelList().
Markus, sorry if I was not able to put point clearly, what I meant is that the
local 'channel' variable used in qmp_migrate() i.e.
'MigrationChannel *channel = NULL', is defined in qmp_migrate() and if the user
opts for 'uri' then '@channels' coming from hmp_migrate() will be NULL, and
then migrate_uri_parse() will populate memory into 'channel', and that is not
getting freed after it's use is over.
I think, that is where memory leak might be happening ?
Aha!
if (uri && has_channels) {
error_setg(errp, "'uri' and 'channels' arguments are mutually "
"exclusive; exactly one of the two should be present in "
"'migrate' qmp command ");
return;
} else if (channels) {
/* To verify that Migrate channel list has only item */
if (channels->next) {
error_setg(errp, "Channel list has more than one entries");
return;
}
channel = channels->value;
} else if (uri) {
/* caller uses the old URI syntax */
if (!migrate_uri_parse(uri, &channel, errp)) {
return;
}
} else {
error_setg(errp, "neither 'uri' or 'channels' argument are "
"specified in 'migrate' qmp command ");
return;
}
At this point, @channel is either channels->value, or from
migrate_uri_parse().
We must not free in the former case, we must free in the latter case,
Before your patch, we don't free. Memory leak in the latter case.
Afterwards, we free. Double-free in the former case.
You could guard the free, like so:
if (uri) {
qapi_free_MigrationChannel(channel);
}
Yeah, you explained it right. The above solution seems fine to me.
I am just curious to ask this: can we use g_autoptr() for local vaiable
'channel' and 'addr' ? As we are not passing these variables to the
caller function, nor we are trying to transfer their ownership to
another variable, so use of g_steal_pointer() also might not be required ?
By the way, I the conditional shown above is harder to understand than
necessary. I like to get the errors out of the way at the beginning,
like this:
if (uri && has_channels) {
error_setg(errp, "'uri' and 'channels' arguments are mutually "
"exclusive; exactly one of the two should be present in "
"'migrate' qmp command ");
return;
}
if (!uri && !has_channels) {
error_setg(errp, "neither 'uri' or 'channels' argument are "
"specified in 'migrate' qmp command ");
return;
}
if (channels) {
/* To verify that Migrate channel list has only item */
Or even
if (!uri == !has_channels) {
error_setg(errp, "need either 'uri' or 'channels' argument")
return;
}
Suggestion, not demand. If you do it, separate patch.
Yeah, I probably opted for 'if, else if' block because I found it easy
to have all 4 options in that manner.
'!uri == !has_channels' is same as '!uri && !has_channels' right ?
Now looking at the Qemu code, it is better to have conditional
statements the way you mentioned. Will do it in a separate patch.
Regards,
Het Gala
Re: [PATCH v2 4/4] target/arm: Add support for DC CVAP & DC CVADP ins
On 28/11/23 12:34, Peter Maydell wrote:
On Tue, 28 Nov 2023 at 11:24, Philippe Mathieu-Daudé wrote:
Hi,
On 6/11/19 00:41, Beata Michalska wrote:
ARMv8.2 introduced support for Data Cache Clean instructions
to PoP (point-of-persistence) - DC CVAP and PoDP (point-of-deep-persistence)
- DV CVADP. Both specify conceptual points in a memory system where all writes
that are to reach them are considered persistent.
The support provided considers both to be actually the same so there is no
distinction between the two. If none is available (there is no backing store
for given memory) both will result in Data Cache Clean up to the point of
coherency. Otherwise sync for the specified range shall be performed.
Signed-off-by: Beata Michalska
---
linux-user/elfload.c | 2 ++
target/arm/cpu.h | 10 ++
target/arm/cpu64.c | 1 +
target/arm/helper.c | 56
4 files changed, 69 insertions(+)
+#ifndef CONFIG_USER_ONLY
+static void dccvap_writefn(CPUARMState *env, const ARMCPRegInfo *opaque,
+ uint64_t value)
+{
+ARMCPU *cpu = env_archcpu(env);
+/* CTR_EL0 System register -> DminLine, bits [19:16] */
+uint64_t dline_size = 4 << ((cpu->ctr >> 16) & 0xF);
+uint64_t vaddr_in = (uint64_t) value;
+uint64_t vaddr = vaddr_in & ~(dline_size - 1);
+void *haddr;
+int mem_idx = cpu_mmu_index(env, false);
+
+/* This won't be crossing page boundaries */
+haddr = probe_read(env, vaddr, dline_size, mem_idx, GETPC());
+if (haddr) {
+
+ram_addr_t offset;
+MemoryRegion *mr;
+
+/* RCU lock is already being held */
+mr = memory_region_from_host(haddr, &offset);
+
+if (mr) {
+memory_region_do_writeback(mr, offset, dline_size);
+}
+}
+}
+#ifndef CONFIG_USER_ONLY
+/* Data Cache clean instructions up to PoP */
+if (cpu_isar_feature(aa64_dcpop, cpu)) {
Am I correct understanding this is a TCG-only feature?
For KVM, whether the vCPU implements these cache
maintenance instructions is up to it -- like all insns,
QEMU doesn't ever see if the guest executes them or not
(either the host CPU just implements them, or the host
kernel traps and handles them). The code in this patch is
specifically for the QEMU TCG emulation of them.
Thank you Peter. In this case I'm compiling HVF, but this is the
same reasoning. I'll add #ifdef'ry similar to ats_write() (commit
9fb005b02d "target/arm: Restrict the Address Translate write operation
to TCG accel"):
-- >8 --
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 99c7da9ca4..a05e613e10 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7629,6 +7629,7 @@ static const ARMCPRegInfo rndr_reginfo[] = {
static void dccvap_writefn(CPUARMState *env, const ARMCPRegInfo *opaque,
uint64_t value)
{
+#ifdef CONFIG_TCG
ARMCPU *cpu = env_archcpu(env);
/* CTR_EL0 System register -> DminLine, bits [19:16] */
uint64_t dline_size = 4 << ((cpu->ctr >> 16) & 0xF);
@@ -7653,6 +7654,10 @@ static void dccvap_writefn(CPUARMState *env,
const ARMCPRegInfo *opaque,
}
#endif /*CONFIG_USER_ONLY*/
}
+#else
+/* Handled by hardware accelerator. */
+g_assert_not_reached();
+#endif /* CONFIG_TCG */
}
---
Regards,
Phil.
Re: [PATCH] hw/cxl/mbox: Remove dead code
On Fri, 10 Nov 2023 15:26:40 -0800
Davidlohr Bueso wrote:
> Two functions were reported to have dead code, remove the bogus
> branches altogether, as well as a misplaced qemu_log call.
>
> Reported-by: Peter Maydell
> Signed-off-by: Davidlohr Bueso
LGTM.
Michael, if you want to pick this up directly that would be great.
If not I'll roll it into next series of fixes I send you.
Acked-by: Jonathan Cameron
Thanks,
Jonathan
> ---
> hw/cxl/cxl-mailbox-utils.c | 43 +-
> 1 file changed, 15 insertions(+), 28 deletions(-)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index b36557509710..39642ed93ee6 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -1001,15 +1001,8 @@ static CXLRetCode cmd_sanitize_overwrite(const struct
> cxl_cmd *cmd,
>
> cxl_dev_disable_media(&ct3d->cxl_dstate);
>
> -if (secs > 2) {
> -/* sanitize when done */
> -return CXL_MBOX_BG_STARTED;
> -} else {
> -__do_sanitization(ct3d);
> -cxl_dev_enable_media(&ct3d->cxl_dstate);
> -
> -return CXL_MBOX_SUCCESS;
> -}
> +/* sanitize when done */
> +return CXL_MBOX_BG_STARTED;
> }
>
> static CXLRetCode cmd_get_security_state(const struct cxl_cmd *cmd,
> @@ -1387,27 +1380,21 @@ static void bg_timercb(void *opaque)
>
> cci->bg.complete_pct = 100;
> cci->bg.ret_code = ret;
> -if (ret == CXL_MBOX_SUCCESS) {
> -switch (cci->bg.opcode) {
> -case 0x4400: /* sanitize */
> -{
> -CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> -
> -__do_sanitization(ct3d);
> -cxl_dev_enable_media(&ct3d->cxl_dstate);
> -}
> +switch (cci->bg.opcode) {
> +case 0x4400: /* sanitize */
> +{
> +CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> +
> +__do_sanitization(ct3d);
> +cxl_dev_enable_media(&ct3d->cxl_dstate);
> +}
> +break;
> +case 0x4304: /* TODO: scan media */
> +break;
> +default:
> +__builtin_unreachable();
> break;
> -case 0x4304: /* TODO: scan media */
> -break;
> -default:
> -__builtin_unreachable();
> -break;
> -}
> }
> -
> -qemu_log("Background command %04xh finished: %s\n",
> - cci->bg.opcode,
> - ret == CXL_MBOX_SUCCESS ? "success" : "aborted");
> } else {
> /* estimate only */
> cci->bg.complete_pct = 100 * now / total_time;
Re: [PATCH 19/19] qapi/schema: refactor entity lookup helpers
John Snow writes:
> This is not a clear win, but I was confused and I couldn't help myself.
>
> Before:
>
> lookup_entity(self, name: str, typ: Optional[type] = None
> ) -> Optional[QAPISchemaEntity]: ...
>
> lookup_type(self, name: str) -> Optional[QAPISchemaType]: ...
>
> resolve_type(self, name: str, info: Optional[QAPISourceInfo],
> what: Union[str, Callable[[Optional[QAPISourceInfo]], str]]
> ) -> QAPISchemaType: ...
>
> After:
>
> get_entity(self, name: str) -> Optional[QAPISchemaEntity]: ...
> get_typed_entity(self, name: str, typ: Type[_EntityType]
> ) -> Optional[_EntityType]: ...
> lookup_type(self, name: str) -> QAPISchemaType: ...
> resolve_type(self, name: str, info: Optional[QAPISourceInfo],
> what: Union[str, Callable[[Optional[QAPISourceInfo]], str]]
> ) -> QAPISchemaType: ...
.resolve_type()'s type remains the same.
> In essence, any function that can return a None value becomes "get ..."
> to encourage association with the dict.get() function which has the same
> behavior. Any function named "lookup" or "resolve" by contrast is no
> longer allowed to return a None value.
.resolve_type() doesn't before the patch.
> This means that any callers to resolve_type or lookup_type don't have to
> check that the function worked, they can just assume it did.
>
> Callers to resolve_type will be greeted with a QAPISemError if something
> has gone wrong, as they have in the past. Callers to lookup_type will be
> greeted with a KeyError if the entity does not exist, or a TypeError if
> it does, but is the wrong type.
Talking about .resolve_type() so much suggests you're changing it.
You're not.
Here's my own summary of the change, just to make sure I got it:
1. Split .lookup_entity() into .get_entity() and .get_typed_entity().
schema.lookup_entity(name) and schema.lookup_entity(name, None)
become schema.get_entity(name).
schema.lookup_entity(name, typ) where typ is not None becomes
schema.get_typed_entity().
2. Tighten .get_typed_entity()'s type from Optional[QAPISchemaEntity] to
Optional[_EntityType], where Entity is argument @typ.
3. Change .lookup_type()'s behavior for "not found" from "return None"
to "throw KeyError if doesn't exist, throw TypeError if exists, but
not a type".
Correct?
> get_entity and get_typed_entity remain for any callers who are
> specifically interested in the negative case. These functions have only
> a single caller each.
.get_entity()'s single caller being QAPIDocDirective.run(), and its
other single caller being QAPISchema._make_implicit_object_type() ;-P
> Signed-off-by: John Snow
> ---
> docs/sphinx/qapidoc.py | 2 +-
> scripts/qapi/introspect.py | 8 ++
> scripts/qapi/schema.py | 52 --
> 3 files changed, 36 insertions(+), 26 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 8f3b9997a15..96deadbf7fc 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -508,7 +508,7 @@ def run(self):
vis = QAPISchemaGenRSTVisitor(self)
> vis.visit_begin(schema)
> for doc in schema.docs:
> if doc.symbol:
> -vis.symbol(doc, schema.lookup_entity(doc.symbol))
> +vis.symbol(doc, schema.get_entity(doc.symbol))
@vis is a QAPISchemaGenRSTVisitor, and vis.symbol is
def symbol(self, doc, entity):
[...]
self._cur_doc = doc
entity.visit(self)
self._cur_doc = None
When you add type hints to qapidoc.py, parameter @entity will be
QAPISchemaEntity. Type error since .get_entity() returns
Optional[QAPISchemaEntity]. I'm fine with addressing that when adding
types to qapidoc.py.
> else:
> vis.freeform(doc)
> return vis.get_document_nodes()
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 42981bce163..67c7d89aae0 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -227,14 +227,10 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>
> # Map the various integer types to plain int
> if typ.json_type() == 'int':
> -tmp = self._schema.lookup_type('int')
> -assert tmp is not None
> -typ = tmp
> +typ = self._schema.lookup_type('int')
> elif (isinstance(typ, QAPISchemaArrayType) and
>typ.element_type.json_type() == 'int'):
> -tmp = self._schema.lookup_type('intList')
> -assert tmp is not None
> -typ = tmp
> +typ = self._schema.lookup_type('intList')
> # Add type to work queue if new
> if typ not in self._used_types:
> self._used_types.append(typ)
Readability improvement here, due to tighter typing of .lookup_type():
it now returns QAPISchemaType instead of Optional[QAPISch
Re: [PATCH] 'channel' and 'addr' in qmp_migrate() are not auto-freed. migrate_uri_parse() allocates memory which is returned to 'channel', which is leaked because there is no code for freeing 'channel
Het Gala writes:
> On 28/11/23 3:29 pm, Markus Armbruster wrote:
>> Het Gala writes:
>>
>>> On 28/11/23 12:46 pm, Markus Armbruster wrote:
Your commit message is all in one line. You need to format it like
migration: Plug memory leak
'channel' and 'addr' in qmp_migrate() are not auto-freed.
migrate_uri_parse() allocates memory which is returned to 'channel',
which is leaked because there is no code for freeing 'channel' or
'addr'. So, free addr and channel to avoid memory leak. 'addr'
does shallow copying of channel->addr, hence free 'channel' itself
and deep free contents of 'addr'.
Het Gala writes:
>>> Yeah, I made the changes in v2 patchset.
> ---
>migration/migration.c | 2 ++
>1 file changed, 2 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 28a34c9068..29efb51b62 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2004,6 +2004,8 @@ void qmp_migrate(const char *uri, bool has_channels,
> MIGRATION_STATUS_FAILED);
>block_cleanup_parameters();
>}
> +g_free(channel);
> +qapi_free_MigrationAddress(addr);
> if (local_err) {
>if (!resume_requested) {
2. hmp_migrate()
hmp_migrate() allocates @channel with migrate_uri_parse(), adds it to
list @caps, passes @caps to qmp_migrate(), then frees @caps with
qapi_free_MigrationChannelList().
>>> Markus, sorry if I was not able to put point clearly, what I meant is that
>>> the local 'channel' variable used in qmp_migrate() i.e.
>>>
>>> 'MigrationChannel *channel = NULL', is defined in qmp_migrate() and if the
>>> user opts for 'uri' then '@channels' coming from hmp_migrate() will be
>>> NULL, and then migrate_uri_parse() will populate memory into 'channel', and
>>> that is not getting freed after it's use is over.
>>>
>>> I think, that is where memory leak might be happening ?
>> Aha!
>>
>> if (uri && has_channels) {
>> error_setg(errp, "'uri' and 'channels' arguments are mutually "
>> "exclusive; exactly one of the two should be present in "
>> "'migrate' qmp command ");
>> return;
>> } else if (channels) {
>> /* To verify that Migrate channel list has only item */
>> if (channels->next) {
>> error_setg(errp, "Channel list has more than one entries");
>> return;
>> }
>> channel = channels->value;
>> } else if (uri) {
>> /* caller uses the old URI syntax */
>> if (!migrate_uri_parse(uri, &channel, errp)) {
>> return;
>> }
>> } else {
>> error_setg(errp, "neither 'uri' or 'channels' argument are "
>> "specified in 'migrate' qmp command ");
>> return;
>> }
>>
>> At this point, @channel is either channels->value, or from
>> migrate_uri_parse().
>>
>> We must not free in the former case, we must free in the latter case,
>>
>> Before your patch, we don't free. Memory leak in the latter case.
>>
>> Afterwards, we free. Double-free in the former case.
>>
>> You could guard the free, like so:
>>
>> if (uri) {
>> qapi_free_MigrationChannel(channel);
>> }
>
> Yeah, you explained it right. The above solution seems fine to me.
>
> I am just curious to ask this: can we use g_autoptr() for local vaiable
> 'channel' and 'addr' ? As we are not passing these variables to the caller
> function, nor we are trying to transfer their ownership to another variable,
> so use of g_steal_pointer() also might not be required ?
You could try something like
diff --git a/migration/migration.c b/migration/migration.c
index 28a34c9068..7faa9c2ebd 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1932,7 +1932,7 @@ void qmp_migrate(const char *uri, bool has_channels,
bool resume_requested;
Error *local_err = NULL;
MigrationState *s = migrate_get_current();
-MigrationChannel *channel = NULL;
+g_autoptr(MigrationChannel) channel = NULL;
MigrationAddress *addr = NULL;
/*
@@ -1949,18 +1949,18 @@ void qmp_migrate(const char *uri, bool has_channels,
error_setg(errp, "Channel list has more than one entries");
return;
}
-channel = channels->value;
+addr = channels->value->addr;
} else if (uri) {
/* caller uses the old URI syntax */
if (!migrate_uri_parse(uri, &channel, errp)) {
return;
}
+addr = channel->addr;
} else {
error_setg(errp, "neither 'uri' or 'channels' argument are "
"specified in 'migrate' qmp command ");
return;
}
-addr = channel->addr;
/* transport mechanism not suitable for migration
Re: [PATCH v2 for-8.2] ppc/amigaone: Allow running AmigaOS without firmware image
On Tue, 28 Nov 2023, Cédric Le Goater wrote:
On 11/28/23 02:47, Nicholas Piggin wrote:
On Tue Nov 28, 2023 at 2:37 AM AEST, Cédric Le Goater wrote:
I'm not sure, I don't think it's necessary if your minimal patch works.
I'll do a PR for 8.2 for SLOF and Skiboot updates, so happy to include
this as well.
I think this is a bit late for 8.2 to change FW images, well, at least
SLOF and skiboot. Are the new versions fixing something critical ?
Ah okay. Well then I can put them in next instead.
SLOF has a fix for virtio console over reboots, pretty minimal.
I see that commit dd4d4ea0add9 has :
Fixes: cf28264 ("virtio-serial: Rework shutdown sequence")
Looks good for 8.2
skiboot has some bug fixes but it's a bigger change and maybe not
so important for QEMU.> Could they be merged in next release
yes. it seems skiboot should be merged with chiptod support in 9.0.
and SLOF tagged with stable?
I think this amigaone patch could still be merged since it's only
touching a new machine and it's fixing an issue of missing firmware.
ARM does something similar with roms. See hw/arm/boot.c file.
It will need a "Fixes" tag.
That would be
Fixes: d9656f860a38f83efc9710c515eab6a5b015134c
as the only commit for this machine so far was the one that added it.
Regards,
BALATON Zoltan
Re: [PATCH v1 2/2] hw/mem/cxl_type3: allocate more vectors for MSI-X
On Tue, 28 Nov 2023 09:27:28 +0900
Hyeonggon Yoo <42.hye...@gmail.com> wrote:
> On Tue, Nov 28, 2023 at 2:53 AM Davidlohr Bueso wrote:
> >
> > On Mon, 27 Nov 2023, Hyeonggon Yoo wrote:
> >
> > >commit 43efb0bfad2b ("hw/cxl/mbox: Wire up interrupts for background
> > >completion") enables notifying background command completion via MSI-X
> > >interrupt (vector number 9).
> > >
> > >However, the commit uses vector number 9 but the maximum number of
> > >entries is less thus resulting in error below. Fix it by passing
> > >nentries = 10 when calling msix_init_exclusive_bar().
> >
> > Hmm yeah this was already set to 10 in Jonathan's tree, thanks for
> > reporting.
>
> Oh, yeah, it's based on the mainline tree. I should have checked Jonathan's.
>
> hmm it's already 10 there but vector number 9 is already being used by PCIe
> DOE.
> So I think it should change msix_num = 11 and use vector number 10 for
> background command completion interrupt instead?
>
> https://gitlab.com/jic23/qemu/-/commit/2823f19188664a6d48a965ea8170c9efa23cddab
Whilst I clearly messed up a rebase as this wasn't intended, it should be fine
to have multiple things sharing a vector.
On my todo list is making the case of too few vectors being available work for
all the cases in which case everything may end up on one vector.
So we do need to expand the vectors to cover what we are asking for, but
moving this to 11 is a nice to have rather than required.
Jonathan
>
> Thanks!
>
> --
> Hyeonggon
Re: [PATCH v3] ppc/amigaone: Allow running AmigaOS without firmware image
On 11/28/23 02:32, BALATON Zoltan wrote:
The machine uses a modified U-Boot under GPL license but the sources
of it are lost with only a binary available so it cannot be included
in QEMU. Allow running without the firmware image which can be used
when calling a boot loader directly and thus simplifying booting
guests. We need a small routine that AmigaOS calls from ROM which is
added in this case to allow booting AmigaOS without external firmware
image.
Signed-off-by: BALATON Zoltan
Since this is 8.2 material :
Fixes: d9656f860a38 ("hw/ppc: Add emulation of AmigaOne XE board")
Thanks,
C.
---
v3: Instead of -bios none do this when no -bios option given, use
constants for address and rom_add_blob_fixed() to add dummy_fw.
This makes both code and usage a bit simpler.
hw/ppc/amigaone.c | 35 +++
1 file changed, 23 insertions(+), 12 deletions(-)
diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
index 992a55e632..ddfa09457a 100644
--- a/hw/ppc/amigaone.c
+++ b/hw/ppc/amigaone.c
@@ -36,10 +36,19 @@
* -device VGA,romfile=VGABIOS-lgpl-latest.bin
* from http://www.nongnu.org/vgabios/ instead.
*/
-#define PROM_FILENAME "u-boot-amigaone.bin"
#define PROM_ADDR 0xfff0
#define PROM_SIZE (512 * KiB)
+/* AmigaOS calls this routine from ROM, use this if no firmware loaded */
+static const char dummy_fw[] = {
+0x38, 0x00, 0x00, 0x08, /* li r0,8 */
+0x7c, 0x09, 0x03, 0xa6, /* mtctr r0 */
+0x54, 0x63, 0xf8, 0x7e, /* srwir3,r3,1 */
+0x42, 0x00, 0xff, 0xfc, /* bdnz0x8 */
+0x7c, 0x63, 0x18, 0xf8, /* not r3,r3 */
+0x4e, 0x80, 0x00, 0x20, /* blr */
+};
+
static void amigaone_cpu_reset(void *opaque)
{
PowerPCCPU *cpu = opaque;
@@ -60,8 +69,6 @@ static void amigaone_init(MachineState *machine)
PowerPCCPU *cpu;
CPUPPCState *env;
MemoryRegion *rom, *pci_mem, *mr;
-const char *fwname = machine->firmware ?: PROM_FILENAME;
-char *filename;
ssize_t sz;
PCIBus *pci_bus;
Object *via;
@@ -94,20 +101,24 @@ static void amigaone_init(MachineState *machine)
}
/* allocate and load firmware */
-filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, fwname);
-if (filename) {
-rom = g_new(MemoryRegion, 1);
-memory_region_init_rom(rom, NULL, "rom", PROM_SIZE, &error_fatal);
-memory_region_add_subregion(get_system_memory(), PROM_ADDR, rom);
+rom = g_new(MemoryRegion, 1);
+memory_region_init_rom(rom, NULL, "rom", PROM_SIZE, &error_fatal);
+memory_region_add_subregion(get_system_memory(), PROM_ADDR, rom);
+if (!machine->firmware) {
+rom_add_blob_fixed("dummy-fw", dummy_fw, sizeof(dummy_fw),
+ PROM_ADDR + PROM_SIZE - 0x80);
+} else {
+g_autofree char *filename = qemu_find_file(QEMU_FILE_TYPE_BIOS,
+ machine->firmware);
+if (!filename) {
+error_report("Could not find firmware '%s'", machine->firmware);
+exit(1);
+}
sz = load_image_targphys(filename, PROM_ADDR, PROM_SIZE);
if (sz <= 0 || sz > PROM_SIZE) {
error_report("Could not load firmware '%s'", filename);
exit(1);
}
-g_free(filename);
-} else if (!qtest_enabled()) {
-error_report("Could not find firmware '%s'", fwname);
-exit(1);
}
/* Articia S */
Re: [PATCH v3 0/4] Fix IRQ routing in via south bridge
On Sun, 26 Nov 2023, BALATON Zoltan wrote: Philippe, Could this be merged for 8.2 as it fixes USB on the amigaone machine? This would be useful as usb-storage is the simplest way to share data with the host with these machines. Philippe, do you have some time to look at this now for 8.2 please? I still hope this could be fixed for the amigaone machine on release and dont' have to wait until the next one for USB to work on that machine. Regards, BALATON Zoltan This is a slight change from v2 adding more comments and improving commit messages and clean things a bit but otherwise should be the same as previous versions. Even v1 worked the same as this one and v2, the additional check to avoid stuck bits is just paranoia, it does not happen in practice as IRQ mappings are quite static, they are set once at boot and don't change afterwards. The rest is just some explanation on how we got here but can be skipped if not interested in history. This is going back to my original implementation of this IRQ routing that I submitted already for 8.0 in the beginning of this year (https://patchew.org/QEMU/cover.1677004414.git.bala...@eik.bme.hu/) but Mark had concerns about that because he wanted to use PCI interrupt routing instead. I've already told back then that won't work but I could not convince reviewers about that. Now with the amigaone machine this can also be seen and that's why this series is needed now. The routing code in PCIBus cannot be used as that only considers the 4 PCI interrupts but there are about 12 interrupt sources in this chip that need to be routed to ISA IRQs (the embedded chip functions and the 4 PCI interrupts that are coming in through 4 pins of the chip). Also the chip does not own the PCIBus, that's part of the north bridge so it should not change the PCI interrupt routing of a bus it does not own. (Piix calling pci_bus_irqs() I think is also wrong because the PCI bus in that machine is also owned by the north bridge and piix should not take over routing of IRQs on a bus it's connected to.) Another concern from Mark was that this makes chip functions specific to the chip and they cannot be used as individual PCI devices. Well, yes, they are chip functions, are not user creatable and don't exist as individual devices so it does not make sense to use them without the actual VIA chip they are a function of so that's not a real concern. These functions are also not actual PCI devices, they are PCIDevice because they appear as PCI functions of the chip but are connected internally and this series also models that correctly. This seems to be supported by comments in Linux about how these VIA chip function aren't following PCI standards and use ISA IRQs instead: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/pci/quirks.c?h=v6.5.6#n1172 Therefore I think Mark's proposals are not improving this model so I went back to the original approach which was tested to work and is also simpler and easier to understand than trying to reuse PCI intrrupt routing which does not work and would be more complex anyway for no good reason. Regards, BALATON Zoltan BALATON Zoltan (4): hw/isa/vt82c686: Bring back via_isa_set_irq() hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts hw/isa/vt82c686: Route PIRQ inputs using via_isa_set_irq() hw/audio/via-ac97: Route interrupts using via_isa_set_irq() hw/audio/via-ac97.c| 8 ++-- hw/isa/vt82c686.c | 79 +- hw/usb/vt82c686-uhci-pci.c | 9 + include/hw/isa/vt82c686.h | 2 + 4 files changed, 67 insertions(+), 31 deletions(-)
Re: [PATCH 1/1] accel/tcg: Fix the comment for CPUTLBEntryFull
On 01/09/2023 07:01, LIU Zhiwei wrote:
When memory region is ram, the lower TARGET_PAGE_BITS is not the
physical section number. Instead, its value is always 0.
Add comment and assert to make it clear.
Signed-off-by: LIU Zhiwei
---
accel/tcg/cputlb.c | 11 +++
include/exec/cpu-defs.h | 12 ++--
2 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index d68fa6867c..a1ebf75068 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1192,6 +1192,7 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx,
write_flags = read_flags;
if (is_ram) {
iotlb = memory_region_get_ram_addr(section->mr) + xlat;
+assert(!(iotlb & ~TARGET_PAGE_MASK));
/*
* Computing is_clean is expensive; avoid all that unless
* the page is actually writable.
@@ -1254,10 +1255,12 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx,
/* refill the tlb */
/*
- * At this point iotlb contains a physical section number in the lower
- * TARGET_PAGE_BITS, and either
- * + the ram_addr_t of the page base of the target RAM (RAM)
- * + the offset within section->mr of the page base (I/O, ROMD)
+ * When memory region is ram, iotlb contains a TARGET_PAGE_BITS
+ * aligned ram_addr_t of the page base of the target RAM.
+ * Otherwise, iotlb contains
+ * - a physical section number in the lower TARGET_PAGE_BITS
+ * - the offset within section->mr of the page base (I/O, ROMD) with the
+ *TARGET_PAGE_BITS masked off.
* We subtract addr_page (which is page aligned and thus won't
* disturb the low bits) to give an offset which can be added to the
* (non-page-aligned) vaddr of the eventual memory access to get
diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index fb4c8d480f..350287852e 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -100,12 +100,12 @@
typedef struct CPUTLBEntryFull {
/*
* @xlat_section contains:
- * - in the lower TARGET_PAGE_BITS, a physical section number
- * - with the lower TARGET_PAGE_BITS masked off, an offset which
- *must be added to the virtual address to obtain:
- * + the ram_addr_t of the target RAM (if the physical section
- * number is PHYS_SECTION_NOTDIRTY or PHYS_SECTION_ROM)
- * + the offset within the target MemoryRegion (otherwise)
+ * - For ram, an offset which must be added to the virtual address
+ *to obtain the ram_addr_t of the target RAM
+ * - For other memory regions,
+ * + in the lower TARGET_PAGE_BITS, the physical section number
+ * + with the TARGET_PAGE_BITS masked off, the offset within
+ * the target MemoryRegion
*/
hwaddr xlat_section;
Someone sent me a test case that triggers the assert() introduced by this commit
dff1ab6 ("accel/tcg: Fix the comment for CPUTLBEntryFull") for qemu-system-m68k which
is still present in git master. The reproducer is easy:
1. Grab the machine ROM file from
https://www.ilande.co.uk/tmp/qemu/tQuadra800.rom
2. Create an empty declaration ROM greater than 4K:
dd if=/dev/zero of=/tmp/badrom bs=512 count=12
3. Start QEMU like this:
qemu-system-m68k -M q800 -bios tQuadra800.rom \
-device nubus-macfb,romfile=/tmp/badrom
The QEMU process hits the assert() with the following backtrace:
(gdb) bt
#0 0x758a9d3c in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#1 0x7585af32 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#2 0x75845472 in abort () from /lib/x86_64-linux-gnu/libc.so.6
#3 0x75845395 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#4 0x75853e32 in __assert_fail () from /lib/x86_64-linux-gnu/libc.so.6
#5 0x55942e0a in tlb_set_page_full (cpu=0x5618d4a0, mmu_idx=0,
addr=4244631552, full=0x7fffe7d7f7c0) at ../accel/tcg/cputlb.c:1171
#6 0x559432a0 in tlb_set_page_with_attrs (cpu=0x5618d4a0,
addr=4244631552, paddr=4244631552, attrs=..., prot=7, mmu_idx=0, size=4096) at
../accel/tcg/cputlb.c:1290
#7 0x55943305 in tlb_set_page (cpu=0x5618d4a0, addr=4244631552,
paddr=4244631552, prot=7, mmu_idx=0, size=4096) at ../accel/tcg/cputlb.c:1297
#8 0x5588aade in m68k_cpu_tlb_fill (cs=0x5618d4a0, address=4244635647,
size=1, qemu_access_type=MMU_DATA_LOAD, mmu_idx=0, probe=false,
retaddr=140734805255937) at ../target/m68k/helper.c:1018
#9 0x55943367 in tlb_fill (cpu=0x5618d4a0, addr=4244635647, size=1,
access_type=MMU_DATA_LOAD, mmu_idx=0, retaddr=140734805255937) at
../accel/tcg/cputlb.c:1315
#10 0x55945d78 in mmu_lookup1 (cpu=0x5618d4a0, data=0x7fffe7d7fa00,
mmu_idx=0, access_type=MMU_DATA_LOAD, ra=140734805255937) at ../accel/tcg/cputlb.c:1713
#11 0x55946081 in mmu_lookup (cpu=0x5618d4a0, addr=4244635647, oi=3712,
ra=140734805255937, type=MMU_DATA_LOAD, l=0x
Re: [PATCH v2] docs/s390: Fix wrong command example in s390-cpu-topology.rst
On 27/11/23 14:49, Zhao Liu wrote: From: Zhao Liu From s390_possible_cpu_arch_ids() in hw/s390x/s390-virtio-ccw.c, the "core-id" is the index of possible_cpus->cpus[], so it should only be less than possible_cpus->len, which is equal to ms->smp.max_cpus. Fix the wrong "core-id" 112, because it isn't less than maxcpus (36) in -smp, and the valid core ids are 0-35 inclusive. Signed-off-by: Zhao Liu Reviewed-by: Nina Schoetterl-Glausch --- Changes since v1 RFC: * Fixed typo. (Nina) * Polished the description of the reason for fixing the wrong core-id. (Nina) --- docs/devel/s390-cpu-topology.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) Patch queued, thanks! Phil.
Re: [PATCH] target/hexagon/idef-parser/prepare: use env to invoke bash
On 23/11/23 22:15, Samuel Tardieu wrote: This file is the only one involved in the compilation process which still uses the /bin/bash path. Signed-off-by: Samuel Tardieu --- target/hexagon/idef-parser/prepare | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Patch queued, thanks! Phil.
Re: [PATCH v2] avr: Fix wrong initial value of stack pointer
On 27/11/23 20:22, Philippe Mathieu-Daudé wrote:
Hi Gihun,
On 27/11/23 03:54, Gihun Nam wrote:
The current implementation initializes the stack pointer of AVR devices
to 0. Although older AVR devices used to be like that, newer ones set
it to RAMEND.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1525
Signed-off-by: Gihun Nam
---
Edit code to use QOM property and add more description to commit message
about the changes
Thanks for the detailed help, Mr. Peter!
P.S. I don't understand how replies work with git send-email, so
if I've done something wrong, please bear with me.
hw/avr/atmega.c | 4
target/avr/cpu.c | 10 +-
target/avr/cpu.h | 3 +++
3 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/target/avr/cpu.h b/target/avr/cpu.h
index 8a17862737..7960c5c57a 100644
--- a/target/avr/cpu.h
+++ b/target/avr/cpu.h
@@ -145,6 +145,9 @@ struct ArchCPU {
CPUState parent_obj;
CPUAVRState env;
+
+ /* Initial value of stack pointer */
+ uint32_t init_sp;
Hmm the stack is 16-bit wide. I suppose AVRCPU::sp is 32-bit
wide because tcg_global_mem_new_i32() forces us to (the smaller
TCG register is 16-bit).
Preferably using uint16_t/DEFINE_PROP_UINT16/qdev_prop_set_uint16:
Reviewed-by: Philippe Mathieu-Daudé
Since this is a fix, I'll queue the patch as it is. We can reduce
the property to 16-bit later, if we find it helpful.
Thanks!
Phil.
Re: [PULL 00/10] Misc bug fixes for QEMU 8.2.0-rc2
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/8.2 for any user-visible changes. signature.asc Description: PGP signature
Re: [PULL 0/1] QGA build bug fixes for QEMU 8.2.0-rc2
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/8.2 for any user-visible changes. signature.asc Description: PGP signature
Re: [PATCH V5 02/12] cpus: stop vm in suspended state
On 11/22/2023 11:21 AM, Peter Xu wrote:
> On Wed, Nov 22, 2023 at 09:38:06AM +, Daniel P. Berrangé wrote:
>> On Mon, Nov 20, 2023 at 04:44:50PM -0500, Peter Xu wrote:
>>> On Mon, Nov 20, 2023 at 03:55:54PM -0500, Steven Sistare wrote:
If we drop force, then all calls to vm_stop will completely stop the
suspended state, eg an hmp "stop" command. This causes two problems.
First, that is a change in user-visible behavior for something that
currently works,
>>>
>>> IMHO it depends on what should be the correct behavior. IOW, when VM is in
>>> SUSPENDED state and then the user sends "stop" QMP command, what should we
>>> expect?
>>
>> I would say that from a mgmtm app POV "stop" is initiating a state
>> transition, from RUN_STATE_RUNNING to RUN_STATE_PAUSED and "cont"
>> is doing the reverse from PAUSED to RUNNING.
>>
>> It is a little more complicated than that as there are some other
>> states like INMIGRATE that are conceptually equiv to RUNNING,
>> and states where the transition simply doesn't make sense.
>
> In the qemu impl, INMIGRATE is, imho, more equiv of PAUSED - do_vm_stop()
> mostly ignores every state except RUNNING (putting bdrv operations aside).
> IOW, anything besides "running" is treated as "not running".
>
> But then Paolo fixed that in 1e9981465f ("qmp: handle stop/cont in
> INMIGRATE state"), wiring that to autostart.
>
> Now we seem to find that "suspended" should actually fall within (where
> "vm" is running, but "vcpu" is not), and it seems we should treat "vm" and
> "vcpu" differently.
>
>>
>> So my question is if we're in "SUSPENDED" and someone issues "stop",
>> what state do we go into, and perhaps more importantly what state
>> do we go to in a subsequent "cont".
>
> I think we must stop the "vm", not only the "vcpu". I discussed this bit
> in the other thread more or less: it's because qemu_system_wakeup_request()
> can be called in many places, e.g. acpi_pm_tmr_timer().
>
> It means even after the VM is "stop"ped by the admin QMP (where qmp_stop()
> ignores SUSPENDED, keep the "vm" running), it can silently got waken up
> without admin even noticing it. I'm not sure what Libvirt will behave if
> it suddenly receives a QAPI_EVENT_WAKEUP randomly after a "stop".
>
>>
>> If you say SUSPENDED ---(stop)---> PAUSED ---(cont)---> SUSPENDED
>> then we create a problem, because the decision for the transition
>> out of PAUSED needs memory of the previous state.
>
> Right, that's why I think we at least need one more boolean to remember the
> suspended state, then when we switch from any STOP states into any RUN
> states, we know where to go. Here STOP states I meant anything except
> RUNNING and SUSPENDED, while RUN -> RUNNING or SUSPENDED.
>
>>
>>> My understanding is we should expect to fully stop the VM, including the
>>> ticks, for example. Keeping the ticks running even after QMP "stop"
>>> doesn't sound right, isn't it?
>>
>> The "stop" QMP command is documented as
>>
>> "Stop all guest VCPU execution"
>>
>> the devil is in the detail though, and we've not documented any detail.
>>
>> Whether or not timers keep running across stop/cont I think can be
>> argued to be an impl detail, as long as the headline goal "vcpus
>> don't execute" is satisfied.
>
> "stop" was since qemu v0.14, so I guess we can't blame the missing of
> details or any form of inaccuracy.. Obviously we do more than "stop vCPU
> executions" in the current implementation.
>
> But after we reach a consensus on how we should fix the current suspended
> problem, we may want to update the documentation to start containing more
> information.
>
>>
vs the migration code where we are fixing brokenness.
>>>
>>> This is not a migration-only bug if above holds, IMO.
>>>
Second, it does not quite work, because the state becomes
RUN_STATE_PAUSED, so the suspended state is forgotten, and the hmp "cont"
will try to set the running state. I could fix that by introducing a new
state RUN_STATE_SUSPENDED_STOPPED, but again it is a user-visible change
in existing behavior. (I even implemented that while developing, then I
realized it was not needed to fix the migration bugs.)
>>>
>>> Good point.
>>
>> We have added new guest states periodically. It is a user visible
>> change, but you could argue that it is implementing a new feature
>> ie the ability to "stop" a "suspended" guest, and so is justified.
>>
>> S3 is so little used in virt, so I'm not surprised we're finding
>> long standing edge cases that have never been thought about before.
>>
>>> Now with above comments, what's your thoughts on whether we should change
>>> the user behavior? My answer is still a yes.
>>>
>>> Maybe SUSPENDED should not be a RunState at all? SUSPENDED is guest visible
>>> behavior, while something like QMP "stop" is not guest visible. Maybe we
>>> should remember it separately?
>>
>> Yes, every time I look at this area I come away thinking that
>> the RunState enum is
Re: [PATCH v3 0/4] Fix IRQ routing in via south bridge
Hi Zoltan, On 28/11/23 13:47, BALATON Zoltan wrote: On Sun, 26 Nov 2023, BALATON Zoltan wrote: Philippe, Could this be merged for 8.2 as it fixes USB on the amigaone machine? This would be useful as usb-storage is the simplest way to share data with the host with these machines. Philippe, do you have some time to look at this now for 8.2 please? I still hope this could be fixed for the amigaone machine on release and dont' have to wait until the next one for USB to work on that machine. Thanks for your detailed cover and patch descriptions. I just finished to run my tests and they all passed. I couldn't spend much time reviewing the patches, but having a quick look I don't think the way you model it is correct. This is a tricky setup and apparently we don't fully understand it (I understand what you explained, but some pieces don't make sense to me). That said, I understand it help you and the AmigaOne users, and nobody objected. So, while being a bit reluctant, I am queuing this series; and will send a PR in a few. We'll have time to improve this model later. Regards, Phil.
[PULL 6/7] hw/avr/atmega: Fix wrong initial value of stack pointer
From: Gihun Nam
The current implementation initializes the stack pointer of AVR devices
to 0. Although older AVR devices used to be like that, newer ones set
it to RAMEND.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1525
Signed-off-by: Gihun Nam
Reviewed-by: Philippe Mathieu-Daudé
Message-ID:
Signed-off-by: Philippe Mathieu-Daudé
---
target/avr/cpu.h | 3 +++
hw/avr/atmega.c | 4
target/avr/cpu.c | 10 +-
3 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/target/avr/cpu.h b/target/avr/cpu.h
index 8a17862737..7960c5c57a 100644
--- a/target/avr/cpu.h
+++ b/target/avr/cpu.h
@@ -145,6 +145,9 @@ struct ArchCPU {
CPUState parent_obj;
CPUAVRState env;
+
+/* Initial value of stack pointer */
+uint32_t init_sp;
};
/**
diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
index a34803e642..31c8992d75 100644
--- a/hw/avr/atmega.c
+++ b/hw/avr/atmega.c
@@ -233,6 +233,10 @@ static void atmega_realize(DeviceState *dev, Error **errp)
/* CPU */
object_initialize_child(OBJECT(dev), "cpu", &s->cpu, mc->cpu_type);
+
+object_property_set_uint(OBJECT(&s->cpu), "init-sp",
+ mc->io_size + mc->sram_size - 1, &error_abort);
+
qdev_realize(DEVICE(&s->cpu), NULL, &error_abort);
cpudev = DEVICE(&s->cpu);
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 44de1e18d1..999c010ded 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -25,6 +25,7 @@
#include "cpu.h"
#include "disas/dis-asm.h"
#include "tcg/debug-assert.h"
+#include "hw/qdev-properties.h"
static void avr_cpu_set_pc(CPUState *cs, vaddr value)
{
@@ -95,7 +96,7 @@ static void avr_cpu_reset_hold(Object *obj)
env->rampY = 0;
env->rampZ = 0;
env->eind = 0;
-env->sp = 0;
+env->sp = cpu->init_sp;
env->skip = 0;
@@ -152,6 +153,11 @@ static void avr_cpu_initfn(Object *obj)
sizeof(cpu->env.intsrc) * 8);
}
+static Property avr_cpu_properties[] = {
+DEFINE_PROP_UINT32("init-sp", AVRCPU, init_sp, 0),
+DEFINE_PROP_END_OF_LIST()
+};
+
static ObjectClass *avr_cpu_class_by_name(const char *cpu_model)
{
ObjectClass *oc;
@@ -228,6 +234,8 @@ static void avr_cpu_class_init(ObjectClass *oc, void *data)
device_class_set_parent_realize(dc, avr_cpu_realizefn,
&mcc->parent_realize);
+device_class_set_props(dc, avr_cpu_properties);
+
resettable_class_set_parent_phases(rc, NULL, avr_cpu_reset_hold, NULL,
&mcc->parent_phases);
--
2.41.0
[PULL 1/7] target/hexagon/idef-parser/prepare: use env to invoke bash
From: Samuel Tardieu This file is the only one involved in the compilation process which still uses the /bin/bash path. Signed-off-by: Samuel Tardieu Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Anton Johansson Message-ID: <20231123211506.636533-1-...@rfc1149.net> Signed-off-by: Philippe Mathieu-Daudé --- target/hexagon/idef-parser/prepare | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/hexagon/idef-parser/prepare b/target/hexagon/idef-parser/prepare index 72d6fcbd21..cb3622d4f8 100755 --- a/target/hexagon/idef-parser/prepare +++ b/target/hexagon/idef-parser/prepare @@ -1,4 +1,4 @@ -#!/bin/bash +#!/usr/bin/env bash # # Copyright(c) 2019-2021 rev.ng Labs Srl. All Rights Reserved. -- 2.41.0
[PULL 0/7] Misc fixes for 2023-11-28
The following changes since commit e867b01cd6658a64c16052117dbb18093a2f9772: Merge tag 'qga-pull-2023-11-25' of https://github.com/kostyanf14/qemu into staging (2023-11-27 08:59:00 -0500) are available in the Git repository at: https://github.com/philmd/qemu.git tags/misc-next-20231128 for you to fetch changes up to 0180a744636e6951996240b96a250d20ad0fad0d: docs/s390: Fix wrong command example in s390-cpu-topology.rst (2023-11-28 14:27:18 +0100) Misc fixes for 8.2 * buildsys: Invoke bash via 'env' (Samuel) * doc: Fix example in s390-cpu-topology.rst (Zhao) * HW: Fix AVR ATMega reset stack (Gihun) and VT82C686 IRQ routing (Zoltan) BALATON Zoltan (4): hw/isa/vt82c686: Bring back via_isa_set_irq() hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts hw/isa/vt82c686: Route PIRQ inputs using via_isa_set_irq() hw/audio/via-ac97: Route interrupts using via_isa_set_irq() Gihun Nam (1): hw/avr/atmega: Fix wrong initial value of stack pointer Samuel Tardieu (1): target/hexagon/idef-parser/prepare: use env to invoke bash Zhao Liu (1): docs/s390: Fix wrong command example in s390-cpu-topology.rst docs/devel/s390-cpu-topology.rst | 6 +-- include/hw/isa/vt82c686.h | 2 + target/avr/cpu.h | 3 ++ hw/audio/via-ac97.c| 8 +-- hw/avr/atmega.c| 4 ++ hw/isa/vt82c686.c | 79 -- hw/usb/vt82c686-uhci-pci.c | 9 target/avr/cpu.c | 10 +++- target/hexagon/idef-parser/prepare | 2 +- 9 files changed, 87 insertions(+), 36 deletions(-) -- 2.41.0
[PULL 3/7] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts
From: BALATON Zoltan
This device is part of a superio/ISA bridge chip and IRQs from it are
routed to an ISA interrupt. Use via_isa_set_irq() function to implement
this in a vt82c686-uhci-pci specific irq handler.
This reverts commit 422a6e8075752bc5342afd3eace23a4990dd7d98.
Signed-off-by: BALATON Zoltan
Message-ID:
Signed-off-by: Philippe Mathieu-Daudé
---
hw/usb/vt82c686-uhci-pci.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
index b4884c9011..6162806172 100644
--- a/hw/usb/vt82c686-uhci-pci.c
+++ b/hw/usb/vt82c686-uhci-pci.c
@@ -1,7 +1,14 @@
#include "qemu/osdep.h"
+#include "hw/irq.h"
#include "hw/isa/vt82c686.h"
#include "hcd-uhci.h"
+static void uhci_isa_set_irq(void *opaque, int irq_num, int level)
+{
+UHCIState *s = opaque;
+via_isa_set_irq(&s->dev, 0, level);
+}
+
static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
{
UHCIState *s = UHCI(dev);
@@ -15,6 +22,8 @@ static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error
**errp)
pci_set_long(pci_conf + 0xc0, 0x2000);
usb_uhci_common_realize(dev, errp);
+object_unref(s->irq);
+s->irq = qemu_allocate_irq(uhci_isa_set_irq, s, 0);
}
static UHCIInfo uhci_info[] = {
--
2.41.0
[PULL 2/7] hw/isa/vt82c686: Bring back via_isa_set_irq()
From: BALATON Zoltan
The VIA integrated south bridge chips combine several functions and
allow routing their interrupts to any of the ISA IRQs also allowing
multiple sources to share the same ISA IRQ. E.g. pegasos2 firmware
configures everything to use IRQ 9 but amigaone routes them to
separate ISA IRQs so the current simplified routing does not work.
Bring back via_isa_set_irq() and change it to take the component that
wants to change an IRQ and keep track of interrupt status of each
source separately and do the mapping to ISA IRQ within the ISA bridge.
This may not handle cases when an ISA IRQ is controlled by devices
directly, not going through via_isa_set_irq() such as serial, parallel
or keyboard but these IRQs being conventionally fixed are not likely
to be change by guests or share with other devices so this does not
cause a problem in practice.
This reverts commit 4e5a20b6da9b1f6d2e9621ed7eb8b239560104ae.
Signed-off-by: BALATON Zoltan
Message-ID:
<1c3902d4166234bef0a476026441eaac3dd6cda5.1701035944.git.bala...@eik.bme.hu>
Signed-off-by: Philippe Mathieu-Daudé
---
include/hw/isa/vt82c686.h | 2 ++
hw/isa/vt82c686.c | 41 +++
2 files changed, 43 insertions(+)
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index b6e95b2851..da1722daf2 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -34,4 +34,6 @@ struct ViaAC97State {
uint32_t ac97_cmd;
};
+void via_isa_set_irq(PCIDevice *d, int n, int level);
+
#endif
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 57bdfb4e78..6fad8293e6 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -549,6 +549,7 @@ struct ViaISAState {
PCIDevice dev;
qemu_irq cpu_intr;
qemu_irq *isa_irqs_in;
+uint16_t irq_state[ISA_NUM_IRQS];
ViaSuperIOState via_sio;
MC146818RtcState rtc;
PCIIDEState ide;
@@ -592,6 +593,46 @@ static const TypeInfo via_isa_info = {
},
};
+void via_isa_set_irq(PCIDevice *d, int pin, int level)
+{
+ViaISAState *s = VIA_ISA(pci_get_function_0(d));
+uint8_t irq = d->config[PCI_INTERRUPT_LINE], max_irq = 15;
+int f = PCI_FUNC(d->devfn);
+uint16_t mask = BIT(f);
+
+switch (f) {
+case 2: /* USB ports 0-1 */
+case 3: /* USB ports 2-3 */
+max_irq = 14;
+break;
+}
+
+/* Keep track of the state of all sources */
+if (level) {
+s->irq_state[0] |= mask;
+} else {
+s->irq_state[0] &= ~mask;
+}
+if (irq == 0 || irq == 0xff) {
+return; /* disabled */
+}
+if (unlikely(irq > max_irq || irq == 2)) {
+qemu_log_mask(LOG_GUEST_ERROR, "Invalid ISA IRQ routing %d for %d",
+ irq, f);
+return;
+}
+/* Record source state at mapped IRQ */
+if (level) {
+s->irq_state[irq] |= mask;
+} else {
+s->irq_state[irq] &= ~mask;
+}
+/* Make sure there are no stuck bits if mapping has changed */
+s->irq_state[irq] &= s->irq_state[0];
+/* ISA IRQ level is the OR of all sources routed to it */
+qemu_set_irq(s->isa_irqs_in[irq], !!s->irq_state[irq]);
+}
+
static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
{
ViaISAState *s = opaque;
--
2.41.0
[PULL 7/7] docs/s390: Fix wrong command example in s390-cpu-topology.rst
From: Zhao Liu
>From s390_possible_cpu_arch_ids() in hw/s390x/s390-virtio-ccw.c, the
"core-id" is the index of possible_cpus->cpus[], so it should only be
less than possible_cpus->len, which is equal to ms->smp.max_cpus.
Fix the wrong "core-id" 112, because it isn't less than maxcpus (36) in
-smp, and the valid core ids are 0-35 inclusive.
Signed-off-by: Zhao Liu
Reviewed-by: Nina Schoetterl-Glausch
Message-ID: <20231127134917.568552-1-zhao1@linux.intel.com>
Signed-off-by: Philippe Mathieu-Daudé
---
docs/devel/s390-cpu-topology.rst | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/docs/devel/s390-cpu-topology.rst b/docs/devel/s390-cpu-topology.rst
index 9eab28d5e5..48313b92d4 100644
--- a/docs/devel/s390-cpu-topology.rst
+++ b/docs/devel/s390-cpu-topology.rst
@@ -15,7 +15,7 @@ have default values:
-smp 1,drawers=3,books=3,sockets=2,cores=2,maxcpus=36 \
-device z14-s390x-cpu,core-id=19,entitlement=high \
-device z14-s390x-cpu,core-id=11,entitlement=low \
--device z14-s390x-cpu,core-id=112,entitlement=high \
+-device z14-s390x-cpu,core-id=12,entitlement=high \
...
Additions to query-cpus-fast
@@ -78,7 +78,7 @@ modifiers for all configured vCPUs.
"dedicated": true,
"thread-id": 537005,
"props": {
-"core-id": 112,
+"core-id": 12,
"socket-id": 0,
"drawer-id": 3,
"book-id": 2
@@ -86,7 +86,7 @@ modifiers for all configured vCPUs.
"cpu-state": "operating",
"entitlement": "high",
"qom-path": "/machine/peripheral-anon/device[2]",
- "cpu-index": 112,
+ "cpu-index": 12,
"target": "s390x"
}
]
--
2.41.0
[PULL 5/7] hw/audio/via-ac97: Route interrupts using via_isa_set_irq()
From: BALATON Zoltan
This device is a function of VIA south bridge and should allow setting
interrupt routing within that chip. This is implemented in
via_isa_set_irq().
Fixes: eb604411a78b82c468e2b8d81a9401eb8b9c7658
Signed-off-by: BALATON Zoltan
Message-ID:
<5329840e4be6dd8ae143d07cbfe61d8d2d106654.1701035944.git.bala...@eik.bme.hu>
Signed-off-by: Philippe Mathieu-Daudé
---
hw/audio/via-ac97.c | 8
hw/isa/vt82c686.c | 1 +
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/audio/via-ac97.c b/hw/audio/via-ac97.c
index 30095a4c7a..4c127a1def 100644
--- a/hw/audio/via-ac97.c
+++ b/hw/audio/via-ac97.c
@@ -211,14 +211,14 @@ static void out_cb(void *opaque, int avail)
AUD_set_active_out(s->vo, 0);
}
if (c->type & STAT_EOL) {
-pci_set_irq(&s->dev, 1);
+via_isa_set_irq(&s->dev, 0, 1);
}
}
if (CLEN_IS_FLAG(c)) {
c->stat |= STAT_FLAG;
c->stat |= STAT_PAUSED;
if (c->type & STAT_FLAG) {
-pci_set_irq(&s->dev, 1);
+via_isa_set_irq(&s->dev, 0, 1);
}
}
if (CLEN_IS_STOP(c)) {
@@ -305,13 +305,13 @@ static void sgd_write(void *opaque, hwaddr addr, uint64_t
val, unsigned size)
if (val & STAT_EOL) {
s->aur.stat &= ~(STAT_EOL | STAT_PAUSED);
if (s->aur.type & STAT_EOL) {
-pci_set_irq(&s->dev, 0);
+via_isa_set_irq(&s->dev, 0, 0);
}
}
if (val & STAT_FLAG) {
s->aur.stat &= ~(STAT_FLAG | STAT_PAUSED);
if (s->aur.type & STAT_FLAG) {
-pci_set_irq(&s->dev, 0);
+via_isa_set_irq(&s->dev, 0, 0);
}
}
break;
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index a3eb6769fc..9c2333a277 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -622,6 +622,7 @@ void via_isa_set_irq(PCIDevice *d, int pin, int level)
break;
case 2: /* USB ports 0-1 */
case 3: /* USB ports 2-3 */
+case 5: /* AC97 audio */
max_irq = 14;
break;
}
--
2.41.0
[PULL 4/7] hw/isa/vt82c686: Route PIRQ inputs using via_isa_set_irq()
From: BALATON Zoltan
The chip has 4 pins (called PIRQA-D in VT82C686B and PINTA-D in
VT8231) that are meant to be connected to PCI IRQ lines and allow
routing PCI interrupts to the ISA PIC. Route these in
via_isa_set_irq() to make it possible to share them with internal
functions that can also be routed to the same ISA IRQs.
Fixes: 2fdadd02e675caca4aba4ae26317701fe2c4c901
Signed-off-by: BALATON Zoltan
Message-ID:
<8c4513d8b78fac40e6d4e65a0a4b3a7f2f278a4b.1701035944.git.bala...@eik.bme.hu>
Signed-off-by: Philippe Mathieu-Daudé
---
hw/isa/vt82c686.c | 67 ++-
1 file changed, 25 insertions(+), 42 deletions(-)
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 6fad8293e6..a3eb6769fc 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -593,6 +593,21 @@ static const TypeInfo via_isa_info = {
},
};
+static int via_isa_get_pci_irq(const ViaISAState *s, int pin)
+{
+switch (pin) {
+case 0:
+return s->dev.config[0x55] >> 4;
+case 1:
+return s->dev.config[0x56] & 0xf;
+case 2:
+return s->dev.config[0x56] >> 4;
+case 3:
+return s->dev.config[0x57] >> 4;
+}
+return 0;
+}
+
void via_isa_set_irq(PCIDevice *d, int pin, int level)
{
ViaISAState *s = VIA_ISA(pci_get_function_0(d));
@@ -601,6 +616,10 @@ void via_isa_set_irq(PCIDevice *d, int pin, int level)
uint16_t mask = BIT(f);
switch (f) {
+case 0: /* PIRQ/PINT inputs */
+irq = via_isa_get_pci_irq(s, pin);
+f = 8 + pin; /* Use function 8-11 for PCI interrupt inputs */
+break;
case 2: /* USB ports 0-1 */
case 3: /* USB ports 2-3 */
max_irq = 14;
@@ -633,52 +652,17 @@ void via_isa_set_irq(PCIDevice *d, int pin, int level)
qemu_set_irq(s->isa_irqs_in[irq], !!s->irq_state[irq]);
}
+static void via_isa_pirq(void *opaque, int pin, int level)
+{
+via_isa_set_irq(opaque, pin, level);
+}
+
static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
{
ViaISAState *s = opaque;
qemu_set_irq(s->cpu_intr, level);
}
-static int via_isa_get_pci_irq(const ViaISAState *s, int irq_num)
-{
-switch (irq_num) {
-case 0:
-return s->dev.config[0x55] >> 4;
-case 1:
-return s->dev.config[0x56] & 0xf;
-case 2:
-return s->dev.config[0x56] >> 4;
-case 3:
-return s->dev.config[0x57] >> 4;
-}
-return 0;
-}
-
-static void via_isa_set_pci_irq(void *opaque, int irq_num, int level)
-{
-ViaISAState *s = opaque;
-PCIBus *bus = pci_get_bus(&s->dev);
-int i, pic_level, pic_irq = via_isa_get_pci_irq(s, irq_num);
-
-/* IRQ 0: disabled, IRQ 2,8,13: reserved */
-if (!pic_irq) {
-return;
-}
-if (unlikely(pic_irq == 2 || pic_irq == 8 || pic_irq == 13)) {
-qemu_log_mask(LOG_GUEST_ERROR, "Invalid ISA IRQ routing");
-}
-
-/* The pic level is the logical OR of all the PCI irqs mapped to it. */
-pic_level = 0;
-for (i = 0; i < PCI_NUM_PINS; i++) {
-if (pic_irq == via_isa_get_pci_irq(s, i)) {
-pic_level |= pci_bus_get_irq_level(bus, i);
-}
-}
-/* Now we change the pic irq level according to the via irq mappings. */
-qemu_set_irq(s->isa_irqs_in[pic_irq], pic_level);
-}
-
static void via_isa_realize(PCIDevice *d, Error **errp)
{
ViaISAState *s = VIA_ISA(d);
@@ -689,6 +673,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
int i;
qdev_init_gpio_out(dev, &s->cpu_intr, 1);
+qdev_init_gpio_in_named(dev, via_isa_pirq, "pirq", PCI_NUM_PINS);
isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
errp);
@@ -702,8 +687,6 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
i8254_pit_init(isa_bus, 0x40, 0, NULL);
i8257_dma_init(isa_bus, 0);
-qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", PCI_NUM_PINS);
-
/* RTC */
qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
--
2.41.0
Re: [PULL 31/53] hw/i386/pc: Default to use SMBIOS 3.0 for newer machine models
Am 26.06.23 um 14:29 schrieb Michael S. Tsirkin: > From: Suravee Suthikulpanit > > Currently, pc-q35 and pc-i44fx machine models are default to use SMBIOS 2.8 > (32-bit entry point). Since SMBIOS 3.0 (64-bit entry point) is now fully > supported since QEMU 7.0, default to use SMBIOS 3.0 for newer machine > models. This is necessary to avoid the following message when launching > a VM with large number of vcpus. > >"SMBIOS 2.1 table length 66822 exceeds 65535" > > Signed-off-by: Suravee Suthikulpanit > Message-Id: <20230607205717.737749-2-suravee.suthikulpa...@amd.com> > Reviewed-by: Michael S. Tsirkin > Signed-off-by: Michael S. Tsirkin > Reviewed-by: Daniel P. Berrangé > Reviewed-by: Igor Mammedov Hi, we received some reports about the new default causing issues for certain guest OSes [0][1]. Namely, for Juniper vSRX, where boot fails and Microsoft Windows, where querying an UUID set via QEMU cmdline -smbios 'type=1,uuid=a4656bd0-a07d-48e0-9dfd-bdc84667a8d0' in Powershell with get-wmiobject win32_computersystemproduct | Select-Object -expandProperty UUID doesn't return any value anymore and can trip up some guest applications. The original report is about Windows 10 and I reproduced this with Windows Server 2019 and the German (but I hope it doesn't matter this time) version of Windows Server 2022. Using machine type 8.0 or the machine option smbios-entry-point-type=32 are workarounds. Since Windows is widely used, that seems a bit unfortunate. Just wanted to ask if you are aware of the issue and if there is something else that can be done other than specifying the more specific machine commandline for those OSes? Best Regards, Fiona [0]: https://forum.proxmox.com/threads/136942/ [1]: https://forum.proxmox.com/threads/136227/post-610277
Re: [PATCH-for-9.0 03/11] target/arm: Declare ARM_CPU_TYPE_NAME/SUFFIX in 'cpu-qom.h'
On 11/22/23 12:30, Philippe Mathieu-Daudé wrote:
Missed in commit 2d56be5a29 ("target: Declare
FOO_CPU_TYPE_NAME/SUFFIX in 'cpu-qom.h'"). See
it for more details.
Signed-off-by: Philippe Mathieu-Daudé
---
target/arm/cpu-qom.h | 3 +++
target/arm/cpu.h | 2 --
2 files changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Richard Henderson
r~
Re: [PATCH-for-9.0 04/11] target/arm: Move ARM_CPU_IRQ/FIQ definitions to 'cpu-qom.h'
On 11/22/23 12:30, Philippe Mathieu-Daudé wrote: The ARM_CPU_IRQ/FIQ definitions are meant for the ARM CPU QOM model. Move them to "cpu-qom.h" so any QOM code can use them. Signed-off-by: Philippe Mathieu-Daudé --- Or do these definitions belong to cpu-defs.h? I think they belong with the qom bits. Reviewed-by: Richard Henderson r~
Re: [PATCH-for-9.0 05/11] target/arm: Move GTIMER definitions to 'cpu-defs.h'
On 11/22/23 12:30, Philippe Mathieu-Daudé wrote: To allow GTIMER_* definitions to be used by non-ARM specific hardware models, move them to a new target agnostic "cpu-defs.h" header. Signed-off-by: Philippe Mathieu-Daudé --- target/arm/cpu-defs.h | 19 +++ target/arm/cpu.h | 8 +--- hw/arm/bcm2836.c | 1 + 3 files changed, 21 insertions(+), 7 deletions(-) create mode 100644 target/arm/cpu-defs.h diff --git a/target/arm/cpu-defs.h b/target/arm/cpu-defs.h new file mode 100644 index 00..1ad76aff14 --- /dev/null +++ b/target/arm/cpu-defs.h @@ -0,0 +1,19 @@ +/* + * ARM "target agnostic" CPU definitions + * + * Copyright (c) 2003 Fabrice Bellard + * + * SPDX-License-Identifier: LGPL-2.1-or-later + */ + +#ifndef ARM_CPU_DEFS_H +#define ARM_CPU_DEFS_H + +#define GTIMER_PHYS 0 +#define GTIMER_VIRT 1 +#define GTIMER_HYP 2 +#define GTIMER_SEC 3 +#define GTIMER_HYPVIRT 4 +#define NUM_GTIMERS 5 + +#endif Hmm. cpu-defs.h is pretty generic. Without looking forward in the patch series, perhaps better as gtimer.h? Is hw/arm/bcm2836.c really "non-arm-specific"? Or did you mean "non-ARMCPU-specific"? r~
Re: [PATCH-for-9.0 06/11] hw/arm/bcm2836: Simplify use of 'reset-cbar' property
On 11/22/23 12:30, Philippe Mathieu-Daudé wrote:
bcm2836_realize() is called by
- bcm2836_class_init() which sets:
bc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a7")
- bcm2837_class_init() which sets:
bc->cpu_type = ARM_CPU_TYPE_NAME("cortex-a53")
Both Cortex-A7 / A53 have the ARM_FEATURE_CBAR set. If it isn't,
then this is a programming error: use &error_abort.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Richard Henderson
r~
Re: [PATCH-for-9.0 07/11] hw/arm/bcm2836: Simplify access to 'start-powered-off' property
On 11/22/23 12:30, Philippe Mathieu-Daudé wrote:
All ARM CPUs have the 'start-powered-off' property since commit
5de164304a ("arm: Allow secondary KVM CPUs to be booted via PSCI").
Note: since commit c1b701587e ("target/arm: Move start-powered-off
property to generic CPUState"), all CPUs for all targets have it.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Richard Henderson
r~
Re: [PATCH-for-9.0 08/11] hw/arm/bcm2836: Use ARM_CPU 'mp-affinity' property
On 11/22/23 12:30, Philippe Mathieu-Daudé wrote:
The 'mp-affinity' property is present since commit 15a21fe028
("target-arm: Add mp-affinity property for ARM CPU class").
Use it and remove a /* TODO */ comment. Since all ARM CPUs
have this property, use &error_abort, because this call can
not fail.
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Richard Henderson
r~
Re: [RFC PATCH-for-9.0 09/11] hw/arm/bcm2836: Allocate ARM CPU state with object_new()
On 11/22/23 12:30, Philippe Mathieu-Daudé wrote:
The ARMCPU type is forward declared as a pointer to all hw/ files.
Its declaration is restricted to target/arm/ files. By using a
pointer in BCM283XState instead of embedding the whole CPU state,
we don't need to include "cpu.h" which is target-specific.
Signed-off-by: Philippe Mathieu-Daudé
---
include/hw/arm/bcm2836.h | 4 ++--
hw/arm/bcm2836.c | 19 ++-
hw/arm/raspi.c | 2 +-
3 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/include/hw/arm/bcm2836.h b/include/hw/arm/bcm2836.h
index 6f90cabfa3..784bab0aad 100644
--- a/include/hw/arm/bcm2836.h
+++ b/include/hw/arm/bcm2836.h
@@ -14,7 +14,7 @@
#include "hw/arm/bcm2835_peripherals.h"
#include "hw/intc/bcm2836_control.h"
-#include "target/arm/cpu.h"
+#include "target/arm/cpu-qom.h"
#include "qom/object.h"
#define TYPE_BCM283X "bcm283x"
@@ -38,7 +38,7 @@ struct BCM283XState {
uint32_t enabled_cpus;
struct {
-ARMCPU core;
+ARMCPU *core;
} cpu[BCM283X_NCPUS];
I'd be tempted to drop the unused struct:
ARMCPU *cpu[BCM283X_NCPUS];
while you're at it. Anyway,
Reviewed-by: Richard Henderson
r~
[PULL 0/4] Block layer patches
The following changes since commit e867b01cd6658a64c16052117dbb18093a2f9772: Merge tag 'qga-pull-2023-11-25' of https://github.com/kostyanf14/qemu into staging (2023-11-27 08:59:00 -0500) are available in the Git repository at: https://repo.or.cz/qemu/kevin.git tags/for-upstream for you to fetch changes up to 6e081324facf9aeece9c286774bab5af3b8d6099: ide/via: Fix BAR4 value in legacy mode (2023-11-28 14:56:32 +0100) Block layer patches - ide/via: Fix BAR4 value in legacy mode - export/vhost-user-blk: Fix consecutive drains - vmdk: Don't corrupt desc file in vmdk_write_cid - iotests: fix default machine type detection Andrey Drobyshev (1): iotests: fix default machine type detection BALATON Zoltan (1): ide/via: Fix BAR4 value in legacy mode Fam Zheng (1): vmdk: Don't corrupt desc file in vmdk_write_cid Kevin Wolf (1): export/vhost-user-blk: Fix consecutive drains include/qemu/vhost-user-server.h | 1 + block/export/vhost-user-blk-server.c | 9 +++-- block/vmdk.c | 28 ++ hw/ide/via.c | 17 ++-- util/vhost-user-server.c | 39 tests/qemu-iotests/testenv.py| 2 +- tests/qemu-iotests/059 | 2 ++ tests/qemu-iotests/059.out | 4 8 files changed, 77 insertions(+), 25 deletions(-)
[PULL 1/4] iotests: fix default machine type detection
From: Andrey Drobyshev
The machine type is being detected based on "-M help" output, and we're
searching for the line ending with " (default)". However, in downstream
one of the machine types s marked as deprecated might become the
default, in which case this logic breaks as the line would now end with
" (default) (deprecated)". To fix potential issues here, let's relax
that requirement and detect the mere presence of " (default)" line
instead.
Signed-off-by: Andrey Drobyshev
Message-ID: <20231122121538.32903-1-andrey.drobys...@virtuozzo.com>
Reviewed-by: Kevin Wolf
Signed-off-by: Kevin Wolf
---
tests/qemu-iotests/testenv.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index e67ebd254b..3ff38f2661 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -40,7 +40,7 @@ def get_default_machine(qemu_prog: str) -> str:
machines = outp.split('\n')
try:
-default_machine = next(m for m in machines if m.endswith(' (default)'))
+default_machine = next(m for m in machines if ' (default)' in m)
except StopIteration:
return ''
default_machine = default_machine.split(' ', 1)[0]
--
2.43.0
[PULL 4/4] ide/via: Fix BAR4 value in legacy mode
From: BALATON Zoltan
Return default value in legacy mode for BAR4 when unset. This can't be
set in reset method because BARs are cleared on reset so we return it
instead when BARs are read in legacy mode. This fixes UDMA on amigaone
with AmigaOS.
Signed-off-by: BALATON Zoltan
Message-ID: <20231125140135.af6a075a...@zero.eik.bme.hu>
Reviewed-by: Kevin Wolf
Signed-off-by: Kevin Wolf
---
hw/ide/via.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 2d3124ebd7..3f3c484253 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -163,14 +163,19 @@ static uint32_t via_ide_cfg_read(PCIDevice *pd, uint32_t
addr, int len)
uint32_t val = pci_default_read_config(pd, addr, len);
uint8_t mode = pd->config[PCI_CLASS_PROG];
-if ((mode & 0xf) == 0xa && ranges_overlap(addr, len,
- PCI_BASE_ADDRESS_0, 16)) {
-/* BARs always read back zero in legacy mode */
-for (int i = addr; i < addr + len; i++) {
-if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 16) {
-val &= ~(0xffULL << ((i - addr) << 3));
+if ((mode & 0xf) == 0xa) {
+if (ranges_overlap(addr, len, PCI_BASE_ADDRESS_0, 16)) {
+/* BARs 0-3 always read back zero in legacy mode */
+for (int i = addr; i < addr + len; i++) {
+if (i >= PCI_BASE_ADDRESS_0 && i < PCI_BASE_ADDRESS_0 + 16) {
+val &= ~(0xffULL << ((i - addr) << 3));
+}
}
}
+if (addr == PCI_BASE_ADDRESS_4 && val == PCI_BASE_ADDRESS_SPACE_IO) {
+/* BAR4 default value if unset */
+val = 0xcc00 | PCI_BASE_ADDRESS_SPACE_IO;
+}
}
return val;
--
2.43.0
[PULL 2/4] vmdk: Don't corrupt desc file in vmdk_write_cid
From: Fam Zheng
If the text description file is larger than DESC_SIZE, we force the last
byte in the buffer to be 0 and write it out.
This results in a corruption.
Try to allocate a big buffer in this case.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1923
Signed-off-by: Fam Zheng
Message-ID: <20231124115654.3239137-1-...@euphon.net>
Reviewed-by: Kevin Wolf
Reviewed-by: Eric Blake
Signed-off-by: Kevin Wolf
---
block/vmdk.c | 28
tests/qemu-iotests/059 | 2 ++
tests/qemu-iotests/059.out | 4
3 files changed, 26 insertions(+), 8 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index d87f6d9aaa..d6971c7067 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -351,29 +351,41 @@ vmdk_write_cid(BlockDriverState *bs, uint32_t cid)
BDRVVmdkState *s = bs->opaque;
int ret = 0;
-desc = g_malloc0(DESC_SIZE);
-tmp_desc = g_malloc0(DESC_SIZE);
-ret = bdrv_co_pread(bs->file, s->desc_offset, DESC_SIZE, desc, 0);
+size_t desc_buf_size;
+
+if (s->desc_offset == 0) {
+desc_buf_size = bdrv_getlength(bs->file->bs);
+if (desc_buf_size > 16ULL << 20) {
+error_report("VMDK description file too big");
+return -EFBIG;
+}
+} else {
+desc_buf_size = DESC_SIZE;
+}
+
+desc = g_malloc0(desc_buf_size);
+tmp_desc = g_malloc0(desc_buf_size);
+ret = bdrv_co_pread(bs->file, s->desc_offset, desc_buf_size, desc, 0);
if (ret < 0) {
goto out;
}
-desc[DESC_SIZE - 1] = '\0';
+desc[desc_buf_size - 1] = '\0';
tmp_str = strstr(desc, "parentCID");
if (tmp_str == NULL) {
ret = -EINVAL;
goto out;
}
-pstrcpy(tmp_desc, DESC_SIZE, tmp_str);
+pstrcpy(tmp_desc, desc_buf_size, tmp_str);
p_name = strstr(desc, "CID");
if (p_name != NULL) {
p_name += sizeof("CID");
-snprintf(p_name, DESC_SIZE - (p_name - desc), "%" PRIx32 "\n", cid);
-pstrcat(desc, DESC_SIZE, tmp_desc);
+snprintf(p_name, desc_buf_size - (p_name - desc), "%" PRIx32 "\n",
cid);
+pstrcat(desc, desc_buf_size, tmp_desc);
}
-ret = bdrv_co_pwrite_sync(bs->file, s->desc_offset, DESC_SIZE, desc, 0);
+ret = bdrv_co_pwrite_sync(bs->file, s->desc_offset, desc_buf_size, desc,
0);
out:
g_free(desc);
diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index 2bcb1f7f9c..0634c7bd92 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -84,6 +84,8 @@ echo
echo "=== Testing big twoGbMaxExtentFlat ==="
_make_test_img -o "subformat=twoGbMaxExtentFlat" 1000G
_img_info --format-specific | _filter_img_info --format-specific
+$QEMU_IO -c "write 990G 512 -P 89" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read 990G 512 -P 89" "$TEST_IMG" | _filter_qemu_io
_cleanup_test_img
echo
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 2b83c0c8b6..275ee7c778 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -2032,6 +2032,10 @@ Format specific information:
virtual size: 2147483648
filename: TEST_DIR/t-f500.IMGFMT
format: FLAT
+wrote 512/512 bytes at offset 1063004405760
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 1063004405760
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
=== Testing malformed VMFS extent description line ===
qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Invalid extent line: RW 12582912
VMFS "dummy.IMGFMT" 1
--
2.43.0
[PULL 3/4] export/vhost-user-blk: Fix consecutive drains
The vhost-user-blk export implement AioContext switches in its drain
implementation. This means that on drain_begin, it detaches the server
from its AioContext and on drain_end, attaches it again and schedules
the server->co_trip coroutine in the updated AioContext.
However, nothing guarantees that server->co_trip is even safe to be
scheduled. Not only is it unclear that the coroutine is actually in a
state where it can be reentered externally without causing problems, but
with two consecutive drains, it is possible that the scheduled coroutine
didn't have a chance yet to run and trying to schedule an already
scheduled coroutine a second time crashes with an assertion failure.
Following the model of NBD, this commit makes the vhost-user-blk export
shut down server->co_trip during drain so that resuming the export means
creating and scheduling a new coroutine, which is always safe.
There is one exception: If the drain call didn't poll (for example, this
happens in the context of bdrv_graph_wrlock()), then the coroutine
didn't have a chance to shut down. However, in this case the AioContext
can't have changed; changing the AioContext always involves a polling
drain. So in this case we can simply assert that the AioContext is
unchanged and just leave the coroutine running or wake it up if it has
yielded to wait for the AioContext to be attached again.
Fixes: e1054cd4aad03a493a5d1cded7508f7c348205bf
Fixes: https://issues.redhat.com/browse/RHEL-1708
Signed-off-by: Kevin Wolf
Message-ID: <20231127115755.22846-1-kw...@redhat.com>
Reviewed-by: Stefan Hajnoczi
Signed-off-by: Kevin Wolf
---
include/qemu/vhost-user-server.h | 1 +
block/export/vhost-user-blk-server.c | 9 +--
util/vhost-user-server.c | 39 ++--
3 files changed, 39 insertions(+), 10 deletions(-)
diff --git a/include/qemu/vhost-user-server.h b/include/qemu/vhost-user-server.h
index 64ad701015..0417ec0533 100644
--- a/include/qemu/vhost-user-server.h
+++ b/include/qemu/vhost-user-server.h
@@ -45,6 +45,7 @@ typedef struct {
/* Protected by ctx lock */
bool in_qio_channel_yield;
bool wait_idle;
+bool quiescing;
VuDev vu_dev;
QIOChannel *ioc; /* The I/O channel with the client */
QIOChannelSocket *sioc; /* The underlying data channel with the client */
diff --git a/block/export/vhost-user-blk-server.c
b/block/export/vhost-user-blk-server.c
index fe2cee3a78..16f48388d3 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -283,6 +283,7 @@ static void vu_blk_drained_begin(void *opaque)
{
VuBlkExport *vexp = opaque;
+vexp->vu_server.quiescing = true;
vhost_user_server_detach_aio_context(&vexp->vu_server);
}
@@ -291,19 +292,23 @@ static void vu_blk_drained_end(void *opaque)
{
VuBlkExport *vexp = opaque;
+vexp->vu_server.quiescing = false;
vhost_user_server_attach_aio_context(&vexp->vu_server, vexp->export.ctx);
}
/*
- * Ensures that bdrv_drained_begin() waits until in-flight requests complete.
+ * Ensures that bdrv_drained_begin() waits until in-flight requests complete
+ * and the server->co_trip coroutine has terminated. It will be restarted in
+ * vhost_user_server_attach_aio_context().
*
* Called with vexp->export.ctx acquired.
*/
static bool vu_blk_drained_poll(void *opaque)
{
VuBlkExport *vexp = opaque;
+VuServer *server = &vexp->vu_server;
-return vhost_user_server_has_in_flight(&vexp->vu_server);
+return server->co_trip || vhost_user_server_has_in_flight(server);
}
static const BlockDevOps vu_blk_dev_ops = {
diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
index 5ccc6d24a0..a9a48fffb8 100644
--- a/util/vhost-user-server.c
+++ b/util/vhost-user-server.c
@@ -132,8 +132,7 @@ vu_message_read(VuDev *vu_dev, int conn_fd, VhostUserMsg
*vmsg)
qio_channel_yield(ioc, G_IO_IN);
server->in_qio_channel_yield = false;
} else {
-/* Wait until attached to an AioContext again */
-qemu_coroutine_yield();
+return false;
}
continue;
} else {
@@ -201,8 +200,16 @@ static coroutine_fn void vu_client_trip(void *opaque)
VuServer *server = opaque;
VuDev *vu_dev = &server->vu_dev;
-while (!vu_dev->broken && vu_dispatch(vu_dev)) {
-/* Keep running */
+while (!vu_dev->broken) {
+if (server->quiescing) {
+server->co_trip = NULL;
+aio_wait_kick();
+return;
+}
+/* vu_dispatch() returns false if server->ctx went away */
+if (!vu_dispatch(vu_dev) && server->ctx) {
+break;
+}
}
if (vhost_user_server_has_in_flight(server)) {
@@ -353,8 +360,7 @@ static void vu_accept(QIONetListener *listener,
QIOChannelSocket *sioc,
qio_channel_set_follow_coroutine_ctx(server->ioc, true);
-
Re: [EXTERNAL] [PATCH v3 2/5] xen: backends: don't overwrite XenStore nodes created by toolstack
On Tue, 2023-11-28 at 01:20 +, Volodymyr Babchuk wrote:
> Hi David,
>
> Thank you for the review
>
> David Woodhouse writes:
>
> > [[S/MIME Signed Part:Undecided]]
> > On Fri, 2023-11-24 at 23:24 +, Volodymyr Babchuk wrote:
> > > Xen PV devices in QEMU can be created in two ways: either by QEMU
> > > itself, if they were passed via command line, or by Xen toolstack. In
> > > the latter case, QEMU scans XenStore entries and configures devices
> > > accordingly.
> > >
> > > In the second case we don't want QEMU to write/delete front-end
> > > entries for two reasons: it might have no access to those entries if
> > > it is running in un-privileged domain and it is just incorrect to
> > > overwrite entries already provided by Xen toolstack, because
> > > toolstack
> > > manages those nodes. For example, it might read backend- or frontend-
> > > state to be sure that they are both disconnected and it is safe to
> > > destroy a domain.
> > >
> > > This patch checks presence of xendev->backend to check if Xen PV
> > > device was configured by Xen toolstack to decide if it should touch
> > > frontend entries in XenStore. Also, when we need to remove XenStore
> > > entries during device teardown only if they weren't created by Xen
> > > toolstack. If they were created by toolstack, then it is toolstack's
> > > job to do proper clean-up.
> > >
> > > Suggested-by: Paul Durrant
> > > Suggested-by: David Woodhouse
> > > Co-Authored-by: Oleksandr Tyshchenko
> > > Signed-off-by: Volodymyr Babchuk
> >
> > Reviewed-by: David Woodhouse
> >
> > ... albeit with a couple of suggestions...
> >
> > > diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> > > index bef8a3a621..b52abf 100644
> > > --- a/hw/char/xen_console.c
> > > +++ b/hw/char/xen_console.c
> > > @@ -450,7 +450,7 @@ static void xen_console_realize(XenDevice *xendev,
> > > Error **errp)
> > >
> > > trace_xen_console_realize(con->dev, object_get_typename(OBJECT(cs)));
> > >
> > > - if (CHARDEV_IS_PTY(cs)) {
> > > + if (CHARDEV_IS_PTY(cs) && !xendev->backend) {
> > > /* Strip the leading 'pty:' */
> > > xen_device_frontend_printf(xendev, "tty", "%s", cs->filename +
> > > 4);
> > > }
> >
> >
> > It's kind of weird that that one is a frontend node at all; surely it
> > should have been a backend node?
>
> Yeah, AFAIK, console was the first PV driver, so it is a bit strange.
> As I see, this frontend entry is used by "xl console" tool to find PTY
> device to attach to. So yes, it should be in backend part of the
> xenstore. But I don't believe we can fix this right now.
Agreed.
> > But it is known only to QEMU once it
> > actually opens /dev/ptmx and creates a new pty. It can't be populated
> > by the toolstack in advance.
> >
> > So shouldn't the toolstack have made it writable by the driver domain?
>
> Maybe it can lead to a weird situation when user in Dom-0 tries to use
> "xl console" command to attach to a console that is absent in Dom-0,
> because "tty" entry points to PTY in the driver domain.
True, but that possibility exists the moment you have console provided
in a driver domain.
...
> > Perhaps here you should create the "mac" node if it doesn't exist (or
> > is that "if it doesn't match netdev->conf.macaddr"?) and just
> > gracefully accept failure too?
> >
>
> I am not sure that I got this right. conf.maccadr can be sent in two
> ways: via xen_net_device_create(), which will fail if toolstack didn't
> provided a MAC address, or via qemu_macaddr_default_if_unset(), which is
> the case for Xen emulation.
The latter happens with hotplug under Xen too, not just Xen emulation.
But the key point you make is that xen_net_device_create() will refuse
to drive a device which the toolstack created, if the "mac" node isn't
present. So creating it for ourselves based on 'if (!xendev->backend)'
as you did in your patch is reasonable enough. Thanks.
smime.p7s
Description: S/MIME cryptographic signature
Re: [PULL 31/53] hw/i386/pc: Default to use SMBIOS 3.0 for newer machine models
On Tue, Nov 28, 2023 at 02:57:17PM +0100, Fiona Ebner wrote: > Am 26.06.23 um 14:29 schrieb Michael S. Tsirkin: > > From: Suravee Suthikulpanit > > > > Currently, pc-q35 and pc-i44fx machine models are default to use SMBIOS 2.8 > > (32-bit entry point). Since SMBIOS 3.0 (64-bit entry point) is now fully > > supported since QEMU 7.0, default to use SMBIOS 3.0 for newer machine > > models. This is necessary to avoid the following message when launching > > a VM with large number of vcpus. > > > >"SMBIOS 2.1 table length 66822 exceeds 65535" > > > > Signed-off-by: Suravee Suthikulpanit > > Message-Id: <20230607205717.737749-2-suravee.suthikulpa...@amd.com> > > Reviewed-by: Michael S. Tsirkin > > Signed-off-by: Michael S. Tsirkin > > Reviewed-by: Daniel P. Berrangé > > Reviewed-by: Igor Mammedov > > Hi, > > we received some reports about the new default causing issues for > certain guest OSes [0][1]. Namely, for Juniper vSRX, where boot fails > and Microsoft Windows, where querying an UUID set via QEMU cmdline > -smbios 'type=1,uuid=a4656bd0-a07d-48e0-9dfd-bdc84667a8d0' > in Powershell with > get-wmiobject win32_computersystemproduct | Select-Object > -expandProperty UUID > doesn't return any value anymore and can trip up some guest > applications. The original report is about Windows 10 and I reproduced > this with Windows Server 2019 and the German (but I hope it doesn't > matter this time) version of Windows Server 2022. > > Using machine type 8.0 or the machine option smbios-entry-point-type=32 > are workarounds. > > Since Windows is widely used, that seems a bit unfortunate. Just wanted > to ask if you are aware of the issue and if there is something else that > can be done other than specifying the more specific machine commandline > for those OSes? I don't recall seeing this issue mentioned before. Could you file a bug at https://gitlab.com/qemu-project/qemu With 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 v3 0/4] Fix IRQ routing in via south bridge
On Tue, 28 Nov 2023, Philippe Mathieu-Daudé wrote: On 28/11/23 13:47, BALATON Zoltan wrote: On Sun, 26 Nov 2023, BALATON Zoltan wrote: Philippe, Could this be merged for 8.2 as it fixes USB on the amigaone machine? This would be useful as usb-storage is the simplest way to share data with the host with these machines. Philippe, do you have some time to look at this now for 8.2 please? I still hope this could be fixed for the amigaone machine on release and dont' have to wait until the next one for USB to work on that machine. Thanks for your detailed cover and patch descriptions. I just finished to run my tests and they all passed. I couldn't spend much time reviewing the patches, but having a quick look I don't think the way you model it is correct. This is a tricky setup and apparently we don't fully understand it (I understand what you explained, but some pieces don't make sense to me). That said, I understand it help you and the AmigaOne users, and nobody objected. So, while being a bit reluctant, I am queuing this series; and will send a PR in a few. We'll have time to improve this model later. Thanks very much. I'm open to further discussion and improving this model, just wanted to have something working in master now. The discussion about this seemed never ending, it started before 8.0 and still could not get to a conclusion yet so until then this should work for now and allow users to use it and does not prevent improving it later. So I'm still interested in your review and why do you think this is not modelling it correctly but we have more time for that now and can change this further as a follow up. I think the current way makes it easier to add Bernhard's SCI interrupt as well for which I had a review proposal before. The main disagreement seemsd to be if the chip functions should be PCI devices or not. I think they aren't like regular PCI devices and clearly don't use PCI interrupts so Mark's and Bernhard's idea to use PCI bus interrupt routing for these does not work because they need to be independently routable to ISA interrupts. So whatever we do we'll need to distinguish the interrupt sources and keep track of their state individually because more than one of them can control a single ISA IRQ. Doing this in the ISA bridge seems like the best place because that already owns the ISA interrupts so no other component will need access to them and it can keep track of state of IRQ sources at one place. Regards, BALATON Zoltan
Re: [PATCH v6 1/3] hw/ppc: Add pnv nest pervasive common chiplet model
On 11/27/23 18:13, Chalapathi V wrote:
A POWER10 chip is divided into logical pieces called chiplets. Chiplets
are broadly divided into "core chiplets" (with the processor cores) and
"nest chiplets" (with everything else). Each chiplet has an attachment
to the pervasive bus (PIB) and with chiplet-specific registers. All nest
chiplets have a common basic set of registers and This model will provide
the registers functionality for common registers of nest chiplet (Pervasive
Chiplet, PB Chiplet, PCI Chiplets, MC Chiplet, PAU Chiplets)
This commit implement the read/write functions of chiplet control registers.
Signed-off-by: Chalapathi V
---
include/hw/ppc/pnv_nest_pervasive.h | 36 +
include/hw/ppc/pnv_xscom.h | 3 +
hw/ppc/pnv_nest_pervasive.c | 219
hw/ppc/meson.build | 1 +
4 files changed, 259 insertions(+)
create mode 100644 include/hw/ppc/pnv_nest_pervasive.h
create mode 100644 hw/ppc/pnv_nest_pervasive.c
diff --git a/include/hw/ppc/pnv_nest_pervasive.h
b/include/hw/ppc/pnv_nest_pervasive.h
new file mode 100644
index 00..9f11531f52
--- /dev/null
+++ b/include/hw/ppc/pnv_nest_pervasive.h
@@ -0,0 +1,36 @@
+/*
+ * QEMU PowerPC nest pervasive common chiplet model
+ *
+ * Copyright (c) 2023, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#ifndef PPC_PNV_NEST_PERVASIVE_H
+#define PPC_PNV_NEST_PERVASIVE_H
+
+#define TYPE_PNV_NEST_PERVASIVE "pnv-nest-chiplet-pervasive"
+#define PNV_NEST_PERVASIVE(obj) OBJECT_CHECK(PnvNestChipletPervasive, (obj),
TYPE_PNV_NEST_PERVASIVE)
+
+typedef struct PnvPervasiveCtrlRegs {
+#define CPLT_CTRL_SIZE 6
+uint64_t cplt_ctrl[CPLT_CTRL_SIZE];
+uint64_t cplt_cfg0;
+uint64_t cplt_cfg1;
+uint64_t cplt_stat0;
+uint64_t cplt_mask0;
+uint64_t ctrl_protect_mode;
+uint64_t ctrl_atomic_lock;
+} PnvPervasiveCtrlRegs;
+
+typedef struct PnvNestChipletPervasive {
+DeviceState parent;
+char*parent_obj_name;
+MemoryRegionxscom_ctrl_regs;
+PnvPervasiveCtrlRegscontrol_regs;
+} PnvNestChipletPervasive;
+
+#endif /*PPC_PNV_NEST_PERVASIVE_H */
diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
index f5becbab41..3e15706dec 100644
--- a/include/hw/ppc/pnv_xscom.h
+++ b/include/hw/ppc/pnv_xscom.h
@@ -170,6 +170,9 @@ struct PnvXScomInterfaceClass {
#define PNV10_XSCOM_XIVE2_BASE 0x2010800
#define PNV10_XSCOM_XIVE2_SIZE 0x400
+#define PNV10_XSCOM_N1_CHIPLET_CTRL_REGS_BASE 0x300
+#define PNV10_XSCOM_CHIPLET_CTRL_REGS_SIZE 0x400
+
#define PNV10_XSCOM_PEC_NEST_BASE 0x3011800 /* index goes downwards ... */
#define PNV10_XSCOM_PEC_NEST_SIZE 0x100
diff --git a/hw/ppc/pnv_nest_pervasive.c b/hw/ppc/pnv_nest_pervasive.c
new file mode 100644
index 00..0575f87e8f
--- /dev/null
+++ b/hw/ppc/pnv_nest_pervasive.c
@@ -0,0 +1,219 @@
+/*
+ * QEMU PowerPC nest pervasive common chiplet model
+ *
+ * Copyright (c) 2023, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/qdev-properties.h"
+#include "hw/ppc/pnv.h"
+#include "hw/ppc/pnv_xscom.h"
+#include "hw/ppc/pnv_nest_pervasive.h"
+
+/*
+ * Status, configuration, and control units in POWER chips is provided
+ * by the pervasive subsystem, which connects registers to the SCOM bus,
+ * which can be programmed by processor cores, other units on the chip,
+ * BMCs, or other POWER chips.
+ *
+ * A POWER10 chip is divided into logical pieces called chiplets. Chiplets
+ * are broadly divided into "core chiplets" (with the processor cores) and
+ * "nest chiplets" (with everything else). Each chiplet has an attachment
+ * to the nest_pervasiveasive bus (PIB) and with chiplet-specific registers.
+ * All nest chiplets have a common basic set of registers.
+ *
+ * This model will provide the registers fuctionality for common registers of
+ * nest unit (PB Chiplet, PCI Chiplets, MC Chiplet, PAU Chiplets)
+ *
+ * Currently this model provide the read/write fuctionality of chiplet control
+ * scom registers.
+ */
+
+#define CPLT_CONF0 0x08
+#define CPLT_CONF0_OR0x18
+#define CPLT_CONF0_CLEAR 0x28
+#define CPLT_CONF1 0x09
+#define CPLT_CONF1_OR0x19
+#define CPLT_CONF1_CLEAR 0x29
+#define CPLT_STAT0 0x100
+#define CPLT_MASK0 0x101
+#define CPLT_PROTECT_MODE0x3FE
+#define CPLT_ATOMIC_CLOCK0x3FF
+
+static uint64_t pnv_chiplet_ctrl_read(void *opaque, hwaddr addr, unsigned size)
+{
+PnvNestChipletPervasive *nest_pervasive = PNV_NEST_PERVASIVE(opaque);
+int reg = addr >> 3;
+uin
[PATCH] pc-bios/optionrom: Fix pvh.img ld build failure on fedora rawhide
binutils 2.39 shows some warnings when building pvh.img /usr/bin/ld: warning: pvh.o: missing .note.GNU-stack section implies executable stack /usr/bin/ld: NOTE: This behaviour is deprecated and will be removed in a future version of the linker /usr/bin/ld: warning: pvh.img has a LOAD segment with RWX permissions The latter of which is fatal on Fedora rawhide for some reason. Add linker options to suppress the errors Signed-off-by: Cole Robinson --- pc-bios/optionrom/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile index 30d07026c7..f54ed39b54 100644 --- a/pc-bios/optionrom/Makefile +++ b/pc-bios/optionrom/Makefile @@ -36,7 +36,7 @@ config-cc.mak: Makefile $(call cc-option,-Wno-array-bounds)) 3> config-cc.mak -include config-cc.mak -override LDFLAGS = -nostdlib -Wl,--build-id=none,-T,$(SRC_DIR)/flat.lds +override LDFLAGS = -nostdlib -Wl,--build-id=none,-T,$(SRC_DIR)/flat.lds -Wl,--no-warn-rwx-segments -Wl,--no-warn-execstack pvh.img: pvh.o pvh_main.o
Re: [PATCH v6 4/5] vfio-user: Message-based DMA support
> On Nov 1, 2023, at 9:16 AM, Mattias Nissler wrote:
>
> Wire up support for DMA for the case where the vfio-user client does not
> provide mmap()-able file descriptors, but DMA requests must be performed
> via the VFIO-user protocol. This installs an indirect memory region,
> which already works for pci_dma_{read,write}, and pci_dma_map works
> thanks to the existing DMA bounce buffering support.
>
> Note that while simple scenarios work with this patch, there's a known
> race condition in libvfio-user that will mess up the communication
> channel. See https://github.com/nutanix/libvfio-user/issues/279 for
> details as well as a proposed fix.
>
> Signed-off-by: Mattias Nissler
Reviewed-by: Jagannathan Raman
> ---
> hw/remote/trace-events| 2 +
> hw/remote/vfio-user-obj.c | 100 --
> 2 files changed, 87 insertions(+), 15 deletions(-)
>
> diff --git a/hw/remote/trace-events b/hw/remote/trace-events
> index 0d1b7d56a5..358a68fb34 100644
> --- a/hw/remote/trace-events
> +++ b/hw/remote/trace-events
> @@ -9,6 +9,8 @@ vfu_cfg_read(uint32_t offset, uint32_t val) "vfu: cfg: 0x%x
> -> 0x%x"
> vfu_cfg_write(uint32_t offset, uint32_t val) "vfu: cfg: 0x%x <- 0x%x"
> vfu_dma_register(uint64_t gpa, size_t len) "vfu: registering GPA 0x%"PRIx64",
> %zu bytes"
> vfu_dma_unregister(uint64_t gpa) "vfu: unregistering GPA 0x%"PRIx64""
> +vfu_dma_read(uint64_t gpa, size_t len) "vfu: DMA read 0x%"PRIx64", %zu bytes"
> +vfu_dma_write(uint64_t gpa, size_t len) "vfu: DMA write 0x%"PRIx64", %zu
> bytes"
> vfu_bar_register(int i, uint64_t addr, uint64_t size) "vfu: BAR %d: addr
> 0x%"PRIx64" size 0x%"PRIx64""
> vfu_bar_rw_enter(const char *op, uint64_t addr) "vfu: %s request for BAR
> address 0x%"PRIx64""
> vfu_bar_rw_exit(const char *op, uint64_t addr) "vfu: Finished %s of BAR
> address 0x%"PRIx64""
> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
> index 8b10c32a3c..9f5e385668 100644
> --- a/hw/remote/vfio-user-obj.c
> +++ b/hw/remote/vfio-user-obj.c
> @@ -300,6 +300,63 @@ static ssize_t vfu_object_cfg_access(vfu_ctx_t *vfu_ctx,
> char * const buf,
> return count;
> }
>
> +static MemTxResult vfu_dma_read(void *opaque, hwaddr addr, uint64_t *val,
> +unsigned size, MemTxAttrs attrs)
> +{
> +MemoryRegion *region = opaque;
> +vfu_ctx_t *vfu_ctx = VFU_OBJECT(region->owner)->vfu_ctx;
> +uint8_t buf[sizeof(uint64_t)];
> +
> +trace_vfu_dma_read(region->addr + addr, size);
> +
> +g_autofree dma_sg_t *sg = g_malloc0(dma_sg_size());
> +vfu_dma_addr_t vfu_addr = (vfu_dma_addr_t)(region->addr + addr);
> +if (vfu_addr_to_sgl(vfu_ctx, vfu_addr, size, sg, 1, PROT_READ) < 0 ||
> +vfu_sgl_read(vfu_ctx, sg, 1, buf) != 0) {
> +return MEMTX_ERROR;
> +}
> +
> +*val = ldn_he_p(buf, size);
> +
> +return MEMTX_OK;
> +}
> +
> +static MemTxResult vfu_dma_write(void *opaque, hwaddr addr, uint64_t val,
> + unsigned size, MemTxAttrs attrs)
> +{
> +MemoryRegion *region = opaque;
> +vfu_ctx_t *vfu_ctx = VFU_OBJECT(region->owner)->vfu_ctx;
> +uint8_t buf[sizeof(uint64_t)];
> +
> +trace_vfu_dma_write(region->addr + addr, size);
> +
> +stn_he_p(buf, size, val);
> +
> +g_autofree dma_sg_t *sg = g_malloc0(dma_sg_size());
> +vfu_dma_addr_t vfu_addr = (vfu_dma_addr_t)(region->addr + addr);
> +if (vfu_addr_to_sgl(vfu_ctx, vfu_addr, size, sg, 1, PROT_WRITE) < 0 ||
> +vfu_sgl_write(vfu_ctx, sg, 1, buf) != 0) {
> +return MEMTX_ERROR;
> +}
> +
> +return MEMTX_OK;
> +}
> +
> +static const MemoryRegionOps vfu_dma_ops = {
> +.read_with_attrs = vfu_dma_read,
> +.write_with_attrs = vfu_dma_write,
> +.endianness = DEVICE_HOST_ENDIAN,
> +.valid = {
> +.min_access_size = 1,
> +.max_access_size = 8,
> +.unaligned = true,
> +},
> +.impl = {
> +.min_access_size = 1,
> +.max_access_size = 8,
> +},
> +};
> +
> static void dma_register(vfu_ctx_t *vfu_ctx, vfu_dma_info_t *info)
> {
> VfuObject *o = vfu_get_private(vfu_ctx);
> @@ -308,17 +365,30 @@ static void dma_register(vfu_ctx_t *vfu_ctx,
> vfu_dma_info_t *info)
> g_autofree char *name = NULL;
> struct iovec *iov = &info->iova;
>
> -if (!info->vaddr) {
> -return;
> -}
> -
> name = g_strdup_printf("mem-%s-%"PRIx64"", o->device,
> - (uint64_t)info->vaddr);
> + (uint64_t)iov->iov_base);
>
> subregion = g_new0(MemoryRegion, 1);
>
> -memory_region_init_ram_ptr(subregion, NULL, name,
> - iov->iov_len, info->vaddr);
> +if (info->vaddr) {
> +memory_region_init_ram_ptr(subregion, OBJECT(o), name,
> + iov->iov_len, info->vaddr);
> +} else {
> +/*
> + * Note that I/O regions' MemoryRegionOps handle accesses of at most
> 8
> + *
Re: [PATCH v6 5/5] vfio-user: Fix config space access byte order
> On Nov 1, 2023, at 9:16 AM, Mattias Nissler wrote:
>
> PCI config space is little-endian, so on a big-endian host we need to
> perform byte swaps for values as they are passed to and received from
> the generic PCI config space access machinery.
>
> Signed-off-by: Mattias Nissler
Reviewed-by: Jagannathan Raman
> ---
> hw/remote/vfio-user-obj.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
> index 9f5e385668..46a2036bd1 100644
> --- a/hw/remote/vfio-user-obj.c
> +++ b/hw/remote/vfio-user-obj.c
> @@ -281,7 +281,7 @@ static ssize_t vfu_object_cfg_access(vfu_ctx_t *vfu_ctx,
> char * const buf,
> while (bytes > 0) {
> len = (bytes > pci_access_width) ? pci_access_width : bytes;
> if (is_write) {
> -memcpy(&val, ptr, len);
> +val = ldn_le_p(ptr, len);
> pci_host_config_write_common(o->pci_dev, offset,
> pci_config_size(o->pci_dev),
> val, len);
> @@ -289,7 +289,7 @@ static ssize_t vfu_object_cfg_access(vfu_ctx_t *vfu_ctx,
> char * const buf,
> } else {
> val = pci_host_config_read_common(o->pci_dev, offset,
> pci_config_size(o->pci_dev),
> len);
> -memcpy(ptr, &val, len);
> +stn_le_p(ptr, len, val);
> trace_vfu_cfg_read(offset, val);
> }
> offset += len;
> --
> 2.34.1
>
Re: [PULL 31/53] hw/i386/pc: Default to use SMBIOS 3.0 for newer machine models
Am 28.11.23 um 15:13 schrieb Daniel P. Berrangé: > On Tue, Nov 28, 2023 at 02:57:17PM +0100, Fiona Ebner wrote: >> we received some reports about the new default causing issues for >> certain guest OSes [0][1]. Namely, for Juniper vSRX, where boot fails >> and Microsoft Windows, where querying an UUID set via QEMU cmdline >> -smbios 'type=1,uuid=a4656bd0-a07d-48e0-9dfd-bdc84667a8d0' >> in Powershell with >> get-wmiobject win32_computersystemproduct | Select-Object >> -expandProperty UUID >> doesn't return any value anymore and can trip up some guest >> applications. The original report is about Windows 10 and I reproduced >> this with Windows Server 2019 and the German (but I hope it doesn't >> matter this time) version of Windows Server 2022. >> >> Using machine type 8.0 or the machine option smbios-entry-point-type=32 >> are workarounds. >> >> Since Windows is widely used, that seems a bit unfortunate. Just wanted >> to ask if you are aware of the issue and if there is something else that >> can be done other than specifying the more specific machine commandline >> for those OSes? > > I don't recall seeing this issue mentioned before. Could you file a > bug at https://gitlab.com/qemu-project/qemu > I made one for the Windows issue: https://gitlab.com/qemu-project/qemu/-/issues/2008 It's not clear to me if this is a bug in QEMU or just a bug/limitation of the guest OS when 64 bit entry is used by SMBIOS. I didn't create one for vSRX, because I'm not using it myself and since it's based on FreeBSD and FreeBSD 13.1 can boot just fine with both 32 and 64 bit entry, it might be an issue on their side. Best Regards, Fiona
Re: [PATCH v6 3/7] qemu: add support for iOS host
(Cc'ing Ariadne, libucontext maintainer) On 21/1/21 19:53, Peter Maydell wrote: On Tue, 5 Jan 2021 at 02:25, Joelle van Dyne wrote: This introduces support for building for iOS hosts. When the correct Xcode toolchain is used, iOS host will be detected automatically. * block: disable features not supported by iOS sandbox * slirp: disable SMB features for iOS * osdep: disable system() calls for iOS +``ucontext`` support is broken on iOS. The implementation from ``libucontext`` +is used instead. Just a note since it came up in another thread today, but looking at libucontext its aarch64 backend doesn't handle the floating point registers. I think if the *context routines don't save/restore the callee-saves fp regs (v8-v15, FPCR) then it's liable to result in tricky-to-track down bugs where some kept-in-a-callee-saves-fp-register data from a function further up the callstack gets corrupted, depending on what the compiler happens to do. It would be good to work with the libucontext maintainers to add that functionality. Per https://github.com/kaniini/libucontext/blob/master/README.md#caveats this is a design choice: Only basic GPR registers are saved and restored when context swapping. The glibc implementation uses hardware capability detection to save/restore other register groups, such as the FPU registers or vector processing (AltiVec/AVX/NEON) registers. Adding this capability detection would significantly increase the complexity of the project and thus is not implemented. Support for compiling in code to save/restore FPU registers or vector registers may be added in a later release as a build-time setting -- for now, we assume a soft-float ABI with no optional processor features. Minor update in 2022: https://github.com/kaniini/libucontext/commit/5244775fb93ab9 This is a work in progress, as newer compilers will spill even non-floating-point state through floating point registers when allowed to do so. Regards, Phil.
[PATCH v2] crypto: Introduce SM4 symmetric cipher algorithm
Introduce the SM4 cipher algorithms (OSCCA GB/T 32907-2016).
SM4 (GBT.32907-2016) is a cryptographic standard issued by the
Organization of State Commercial Administration of China (OSCCA)
as an authorized cryptographic algorithms for the use within China.
Use the crypto-sm4 meson build option for enabling this feature.
Signed-off-by: Hyman Huang
---
crypto/block-luks.c | 11
crypto/cipher-gcrypt.c.inc | 8 ++
crypto/cipher-nettle.c.inc | 49 +
crypto/cipher.c | 6
meson.build | 23
meson_options.txt | 2 ++
qapi/crypto.json| 5 +++-
scripts/meson-buildoptions.sh | 3 ++
tests/unit/test-crypto-cipher.c | 13 +
9 files changed, 119 insertions(+), 1 deletion(-)
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index fb01ec38bb..f0813d69b4 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -95,12 +95,23 @@ qcrypto_block_luks_cipher_size_map_twofish[] = {
{ 0, 0 },
};
+#ifdef CONFIG_CRYPTO_SM4
+static const QCryptoBlockLUKSCipherSizeMap
+qcrypto_block_luks_cipher_size_map_sm4[] = {
+{ 16, QCRYPTO_CIPHER_ALG_SM4},
+{ 0, 0 },
+};
+#endif
+
static const QCryptoBlockLUKSCipherNameMap
qcrypto_block_luks_cipher_name_map[] = {
{ "aes", qcrypto_block_luks_cipher_size_map_aes },
{ "cast5", qcrypto_block_luks_cipher_size_map_cast5 },
{ "serpent", qcrypto_block_luks_cipher_size_map_serpent },
{ "twofish", qcrypto_block_luks_cipher_size_map_twofish },
+#ifdef CONFIG_CRYPTO_SM4
+{ "sm4", qcrypto_block_luks_cipher_size_map_sm4},
+#endif
};
QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSKeySlot) != 48);
diff --git a/crypto/cipher-gcrypt.c.inc b/crypto/cipher-gcrypt.c.inc
index a6a0117717..1377cbaf14 100644
--- a/crypto/cipher-gcrypt.c.inc
+++ b/crypto/cipher-gcrypt.c.inc
@@ -35,6 +35,9 @@ bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg,
case QCRYPTO_CIPHER_ALG_SERPENT_256:
case QCRYPTO_CIPHER_ALG_TWOFISH_128:
case QCRYPTO_CIPHER_ALG_TWOFISH_256:
+#ifdef CONFIG_CRYPTO_SM4
+case QCRYPTO_CIPHER_ALG_SM4:
+#endif
break;
default:
return false;
@@ -219,6 +222,11 @@ static QCryptoCipher
*qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
case QCRYPTO_CIPHER_ALG_TWOFISH_256:
gcryalg = GCRY_CIPHER_TWOFISH;
break;
+#ifdef CONFIG_CRYPTO_SM4
+case QCRYPTO_CIPHER_ALG_SM4:
+gcryalg = GCRY_CIPHER_SM4;
+break;
+#endif
default:
error_setg(errp, "Unsupported cipher algorithm %s",
QCryptoCipherAlgorithm_str(alg));
diff --git a/crypto/cipher-nettle.c.inc b/crypto/cipher-nettle.c.inc
index 24cc61f87b..42b39e18a2 100644
--- a/crypto/cipher-nettle.c.inc
+++ b/crypto/cipher-nettle.c.inc
@@ -33,6 +33,9 @@
#ifndef CONFIG_QEMU_PRIVATE_XTS
#include
#endif
+#ifdef CONFIG_CRYPTO_SM4
+#include
+#endif
static inline bool qcrypto_length_check(size_t len, size_t blocksize,
Error **errp)
@@ -426,6 +429,30 @@ DEFINE_ECB_CBC_CTR_XTS(qcrypto_nettle_twofish,
QCryptoNettleTwofish, TWOFISH_BLOCK_SIZE,
twofish_encrypt_native, twofish_decrypt_native)
+#ifdef CONFIG_CRYPTO_SM4
+typedef struct QCryptoNettleSm4 {
+QCryptoCipher base;
+struct sm4_ctx key[2];
+} QCryptoNettleSm4;
+
+static void sm4_encrypt_native(void *ctx, size_t length,
+ uint8_t *dst, const uint8_t *src)
+{
+struct sm4_ctx *keys = ctx;
+sm4_crypt(&keys[0], length, dst, src);
+}
+
+static void sm4_decrypt_native(void *ctx, size_t length,
+ uint8_t *dst, const uint8_t *src)
+{
+struct sm4_ctx *keys = ctx;
+sm4_crypt(&keys[1], length, dst, src);
+}
+
+DEFINE_ECB(qcrypto_nettle_sm4,
+ QCryptoNettleSm4, SM4_BLOCK_SIZE,
+ sm4_encrypt_native, sm4_decrypt_native)
+#endif
bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg,
QCryptoCipherMode mode)
@@ -443,6 +470,9 @@ bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg,
case QCRYPTO_CIPHER_ALG_TWOFISH_128:
case QCRYPTO_CIPHER_ALG_TWOFISH_192:
case QCRYPTO_CIPHER_ALG_TWOFISH_256:
+#ifdef CONFIG_CRYPTO_SM4
+case QCRYPTO_CIPHER_ALG_SM4:
+#endif
break;
default:
return false;
@@ -701,6 +731,25 @@ static QCryptoCipher
*qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
return &ctx->base;
}
+#ifdef CONFIG_CRYPTO_SM4
+case QCRYPTO_CIPHER_ALG_SM4:
+{
+QCryptoNettleSm4 *ctx = g_new0(QCryptoNettleSm4, 1);
+
+switch (mode) {
+case QCRYPTO_CIPHER_MODE_ECB:
+ctx->base.driver = &qcrypto_nettle_sm4_driver_ecb;
+break;
+default:
+goto bad_cipher_mode;
+}
+
+sm4_set_encrypt_key(&ctx->ke
Re: [PATCH 01/22] target/i386: Only realize existing APIC device
On Mon, 18 Sep 2023 18:02:34 +0200
Philippe Mathieu-Daudé wrote:
> APIC state is created under a certain condition,
> use the same condition to realize it.
> Having a NULL APIC state is a bug: use assert().
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> target/i386/cpu-sysemu.c | 9 +++--
> target/i386/cpu.c| 8 +---
> 2 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
> index 2375e48178..6a164d3769 100644
> --- a/target/i386/cpu-sysemu.c
> +++ b/target/i386/cpu-sysemu.c
> @@ -272,9 +272,7 @@ void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
> APICCommonState *apic;
> APICCommonClass *apic_class = apic_get_class(errp);
>
> -if (!apic_class) {
> -return;
> -}
> +assert(apic_class);
if errp doesn't lead to error_fatal/abort, wouldn't that effectively mask
following error
apic_get_class():
error_setg(errp, "KVM does not support userspace APIC");
return NULL;
???
>
> cpu->apic_state =
> DEVICE(object_new_with_class(OBJECT_CLASS(apic_class)));
> object_property_add_child(OBJECT(cpu), "lapic",
> @@ -293,9 +291,8 @@ void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
> APICCommonState *apic;
> static bool apic_mmio_map_once;
>
> -if (cpu->apic_state == NULL) {
> -return;
> -}
> +assert(cpu->apic_state);
it would be better to explode in one place only inside apic_get_class(),
provided !kvm_irqchip_in_kernel() case is dealt with properly.
> qdev_realize(DEVICE(cpu->apic_state), NULL, errp);
>
> /* Map APIC MMIO area */
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b2a20365e1..a23d4795e0 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7448,9 +7448,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error
> **errp)
> }
>
> #ifndef CONFIG_USER_ONLY
> -x86_cpu_apic_realize(cpu, &local_err);
> -if (local_err != NULL) {
> -goto out;
> +if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || ms->smp.cpus > 1) {
> +x86_cpu_apic_realize(cpu, &local_err);
> +if (local_err != NULL) {
> +goto out;
> +}
better move 'if (cpu->apic_state == NULL) {' from x86_cpu_apic_realize()
to the caller, instead of repeating test again.
> }
> #endif /* !CONFIG_USER_ONLY */
> cpu_reset(cs);
Re: [PATCH 02/22] hw/intc/apic: Pass CPU using QOM link property
On Mon, 18 Sep 2023 18:02:35 +0200
Philippe Mathieu-Daudé wrote:
> QOM objects shouldn't access each other internals fields
> except using the QOM API.
>
> Declare the 'cpu' and 'base-addr' properties, set them
> using object_property_set_link() and qdev_prop_set_uint32()
> respectively.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Igor Mammedov
> ---
> hw/intc/apic_common.c| 2 ++
> target/i386/cpu-sysemu.c | 11 ++-
> 2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index 68ad30e2f5..e28f7402ab 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -394,6 +394,8 @@ static Property apic_properties_common[] = {
> true),
> DEFINE_PROP_BOOL("legacy-instance-id", APICCommonState,
> legacy_instance_id,
> false),
> +DEFINE_PROP_LINK("cpu", APICCommonState, cpu, TYPE_X86_CPU, X86CPU *),
> +DEFINE_PROP_UINT32("base-addr", APICCommonState, apicbase, 0),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
> index 6a164d3769..6edfb7e2af 100644
> --- a/target/i386/cpu-sysemu.c
> +++ b/target/i386/cpu-sysemu.c
> @@ -269,7 +269,6 @@ APICCommonClass *apic_get_class(Error **errp)
>
> void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
> {
> -APICCommonState *apic;
> APICCommonClass *apic_class = apic_get_class(errp);
>
> assert(apic_class);
> @@ -279,11 +278,13 @@ void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>OBJECT(cpu->apic_state));
> object_unref(OBJECT(cpu->apic_state));
>
> +if (!object_property_set_link(OBJECT(cpu->apic_state), "cpu",
> + OBJECT(cpu), errp)) {
> +return;
> +}
> qdev_prop_set_uint32(cpu->apic_state, "id", cpu->apic_id);
> -/* TODO: convert to link<> */
> -apic = APIC_COMMON(cpu->apic_state);
> -apic->cpu = cpu;
> -apic->apicbase = APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE;
> +qdev_prop_set_uint32(cpu->apic_state, "base-addr",
> + APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE);
> }
>
> void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
Re: [PATCH for-8.2] target/arm: Disable SME if SVE is disabled
On Mon, Nov 27 2023, Peter Maydell wrote:
> There is no architectural requirement that SME implies SVE, but
> our implementation currently assumes it. (FEAT_SME_FA64 does
> imply SVE.) So if you try to run a CPU with eg "-cpu max,sve=off"
> you quickly run into an assert when the guest tries to write to
> SMCR_EL1:
>
> #6 0x74b38e96 in __GI___assert_fail
> (assertion=0x566e69cb "sm", file=0x566e5b24
> "../../target/arm/helper.c", line=6865, function=0x566e82f0
> <__PRETTY_FUNCTION__.31> "sve_vqm1_for_el_sm") at ./assert/assert.c:101
> #7 0x55ee33aa in sve_vqm1_for_el_sm (env=0x57d291f0, el=2,
> sm=false) at ../../target/arm/helper.c:6865
> #8 0x55ee3407 in sve_vqm1_for_el (env=0x57d291f0, el=2) at
> ../../target/arm/helper.c:6871
> #9 0x55ee3724 in smcr_write (env=0x57d291f0, ri=0x57da23b0,
> value=2147483663) at ../../target/arm/helper.c:6995
> #10 0x55fd1dba in helper_set_cp_reg64 (env=0x57d291f0,
> rip=0x57da23b0, value=2147483663) at ../../target/arm/tcg/op_helper.c:839
> #11 0x7fff60056781 in code_gen_buffer ()
>
> Avoid this unsupported and slightly odd combination by
> disabling SME when SVE is not present.
>
> Cc: qemu-sta...@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2005
> Signed-off-by: Peter Maydell
> ---
> '-cpu sve=off,sme=on,sme_fa64=off' crashes in the same way, so just
> turning off FA64 isn't sufficient. Maybe we should support
> SME-no-SVE, but for 8.2 at least turning off SME is better than
> letting users hit an assertion.
> ---
> target/arm/cpu.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 25e9d2ae7b8..0fe268ac785 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1743,6 +1743,15 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error
> **errp)
> return;
> }
>
> +/*
> + * FEAT_SME is not architecturally dependent on FEAT_SVE (unless
> + * FEAT_SME_FA64 is present). However our implementation currently
> + * assumes it, so if the user asked for sve=off then turn off SME
> also.
> + */
Might be worth adding a note here that KVM currently does not support
SME anyway? It took me a moment to remember that.
> +if (cpu_isar_feature(aa64_sme, cpu) && !cpu_isar_feature(aa64_sve,
> cpu)) {
> +object_property_set_bool(OBJECT(cpu), "sme", false,
> &error_abort);
> +}
> +
> arm_cpu_sme_finalize(cpu, &local_err);
> if (local_err != NULL) {
> error_propagate(errp, local_err);
[Call for Presentations] FOSDEM 2024: Virt & IaaS Devroom
[Cross-posted to KVM, Rust-VMM, QEMU, and libvirt lists) Hi, the CFP for the "Virt & IaaS" DevRoom is out[+]. Something new this year is a new talk-submission system: so you need to create a new account, even if you've had an account with the older talk-submission system. Details in the "Submit Your Proposal" section below. We are excited to announce that the call for proposals is now open for the Virtualization and Cloud infrastructure devroom at the upcoming FOSDEM 2024, to be hosted on February 3rd 2024. This devroom is a collaborative effort, and is organized by dedicated folks from projects such as OpenStack, Xen Project, KubeVirt, QEMU, KVM, and Foreman. We would like to invite all those who are involved in these fields to submit your proposals by December 8th, 2023. Important Dates --- Submission deadline: 8th December 2023 Acceptance notifications: 10th December 2023 Final schedule announcement: 15th December 2023 Devroom: 3rd February 2024 About the Devroom - The Virtualization & IaaS devroom will feature session topics such as open source hypervisors or virtual machine managers such as Xen Project, KVM, bhyve and VirtualBox as well as Infrastructure-as-a-Service projects such as KubeVirt, Apache CloudStack, OpenStack, QEMU and OpenNebula. This devroom will host presentations that focus on topics of shared interest, such as KVM; libvirt; shared storage; virtualized networking; cloud security; clustering and high availability; interfacing with multiple hypervisors; hyperconverged deployments; and scaling across hundreds or thousands of servers. Presentations in this devroom will be aimed at developers working on these platforms who are looking to collaborate and improve shared infrastructure or solve common problems. We seek topics that encourage dialog between projects and continued work post-FOSDEM. Submit Your Proposal All submissions must be made via the Pretalx event planning site[1]. It is a new submission system so you will need to create an account. If you submitted proposals for FOSDEM in previous years, you won’t be able to use your existing account. During submission please make sure to select Virtualization and Cloud infrastructure from the Track list. Please fill out all the required fields, and provide a meaningful abstract and description of your proposed session. Submission Guidelines - We expect more proposals than we can possibly accept, so it is vitally important that you submit your proposal on or before the deadline. Late submissions are unlikely to be considered. All presentation slots are 30 minutes, with 20 minutes planned for presentations, and 10 minutes for Q&A. All presentations will be recorded and made available under Creative Commons licenses. In the Submission notes field, please indicate that you agree that your presentation will be licensed under the CC-By-SA-4.0 or CC-By-4.0 license and that you agree to have your presentation recorded. For example: "If my presentation is accepted for FOSDEM, I hereby agree to license all recordings, slides, and other associated materials under the Creative Commons Attribution Share-Alike 4.0 International License. Sincerely, ." In the Submission notes field, please also confirm that if your talk is accepted, you will be able to attend FOSDEM and deliver your presentation. We will not consider proposals from prospective speakers who are unsure whether they will be able to secure funds for travel and lodging to attend FOSDEM. (Sadly, we are not able to offer travel funding for prospective speakers.) Code of Conduct --- Following the release of the updated code of conduct for FOSDEM, we'd like to remind all speakers and attendees that all of the presentations and discussions in our devroom are held under the guidelines set in the CoC and we expect attendees, speakers, and volunteers to follow the CoC at all times. If you submit a proposal and it is accepted, you will be required to confirm that you accept the FOSDEM CoC. If you have any questions about the CoC or wish to have one of the devroom organizers review your presentation slides or any other content for CoC compliance, please email us and we will do our best to assist you. Questions? -- If you have any questions about this devroom, please send your questions to our devroom mailing list[2]. You can also subscribe to the list to receive updates about important dates, session announcements, and to connect with other attendees. See you all at FOSDEM! [1] https://pretalx.fosdem.org/fosdem-2024/cfp [2] virtualization-devroom-manager at fosdem.org === [+] This email is a slightly formatted version of the official announce: https://lists.fosdem.org/pipermail/fosdem/2023q4/003481.html -- /kashyap
Re: [PATCH 03/22] target/i386/kvm: Correct comment in kvm_cpu_realize()
On Mon, 18 Sep 2023 18:02:36 +0200
Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Igor Mammedov
> ---
> target/i386/kvm/kvm-cpu.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
> index 7237378a7d..1fe62ce176 100644
> --- a/target/i386/kvm/kvm-cpu.c
> +++ b/target/i386/kvm/kvm-cpu.c
> @@ -37,6 +37,7 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
> * -> cpu_exec_realizefn():
> *-> accel_cpu_realizefn()
> * kvm_cpu_realizefn() -> host_cpu_realizefn()
> + * -> cpu_common_realizefn()
> * -> check/update ucode_rev, phys_bits, mwait
> */
> if (cpu->max_features) {
Re: [PATCH] qemu/timer: Don't use RDTSC on i486
On 11/26/23 09:56, Paolo Bonzini wrote: Il sab 25 nov 2023, 13:23 Petr Cvek mailto:petrcve...@gmail.com>> ha scritto: GCC defines __i386__ for i386 and i486, which both lack RDTSC instruction. The i386 seems to be impossible to distinguish, but i486 can be identified by checking for undefined __i486__. As far as I know QEMU cannot be run on i486 anyway since TCG assumes the presence of CPUID. Have you actually tried? TCG does not assume CPUID. We might have problems without cmpxchg8b, but if so that's in accel/tcg/ not tcg/. r~
Re: [PATCH v2] crypto: Introduce SM4 symmetric cipher algorithm
Hi Hyman,
On 28/11/23 16:24, Hyman Huang wrote:
Introduce the SM4 cipher algorithms (OSCCA GB/T 32907-2016).
SM4 (GBT.32907-2016) is a cryptographic standard issued by the
Organization of State Commercial Administration of China (OSCCA)
as an authorized cryptographic algorithms for the use within China.
Use the crypto-sm4 meson build option for enabling this feature.
Signed-off-by: Hyman Huang
---
crypto/block-luks.c | 11
crypto/cipher-gcrypt.c.inc | 8 ++
crypto/cipher-nettle.c.inc | 49 +
crypto/cipher.c | 6
meson.build | 23
meson_options.txt | 2 ++
qapi/crypto.json| 5 +++-
scripts/meson-buildoptions.sh | 3 ++
tests/unit/test-crypto-cipher.c | 13 +
9 files changed, 119 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index ec01f8b138..256d3257bb 100644
--- a/meson.build
+++ b/meson.build
@@ -1480,6 +1480,7 @@ endif
gcrypt = not_found
nettle = not_found
hogweed = not_found
+crypto_sm4 = not_found
xts = 'none'
if get_option('nettle').enabled() and get_option('gcrypt').enabled()
@@ -1514,6 +1515,26 @@ if not gnutls_crypto.found()
xts = 'private'
endif
endif
+ if get_option('crypto_sm4').enabled()
We want to detect it by default (not only when explicitly enabled) ...
+if get_option('gcrypt').enabled()
+ # SM4 ALG is available in libgcrypt >= 1.9
+ crypto_sm4 = dependency('libgcrypt', version: '>=1.9',
+ method: 'config-tool',
+ required: get_option('gcrypt'))
+ # SM4 ALG static compilation
+ if crypto_sm4.found() and get_option('prefer_static')
+crypto_sm4 = declare_dependency(dependencies: [
+ crypto_sm4,
+ cc.find_library('gpg-error', required: true)],
+ version: crypto_sm4.version())
+ endif
+else
+ # SM4 ALG is available in nettle >= 3.9
+ crypto_sm4 = dependency('nettle', version: '>=3.9',
+ method: 'pkg-config',
+ required: get_option('nettle'))
+endif
... and if it was forced with --enable-crypto_sm4 AND not found,
display an error.
IIUC your config you try to find the best effort implementation then
if not found, keep going silently.
+ endif
endif
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 680fa3f581..f189f34829 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -106,6 +106,7 @@ meson_options_help() {
printf "%s\n" ' colo-proxy colo-proxy support'
printf "%s\n" ' coreaudio CoreAudio sound support'
printf "%s\n" ' crypto-afalgLinux AF_ALG crypto backend driver'
+ printf "%s\n" ' crypto-sm4 SM4 symmetric cipher algorithm support'
printf "%s\n" ' curlCURL block device driver'
printf "%s\n" ' curses curses UI'
printf "%s\n" ' dbus-display-display dbus support'
@@ -282,6 +283,8 @@ _meson_option_parse() {
--disable-coroutine-pool) printf "%s" -Dcoroutine_pool=false ;;
--enable-crypto-afalg) printf "%s" -Dcrypto_afalg=enabled ;;
--disable-crypto-afalg) printf "%s" -Dcrypto_afalg=disabled ;;
+--enable-crypto-sm4) printf "%s" -Dcrypto_sm4=enabled ;;
+--disable-crypto-sm4) printf "%s" -Dcrypto_sm4=disabled ;;
--enable-curl) printf "%s" -Dcurl=enabled ;;
--disable-curl) printf "%s" -Dcurl=disabled ;;
--enable-curses) printf "%s" -Dcurses=enabled ;;
Re: [RFC PATCH 04/22] exec/cpu: Never call cpu_reset() before cpu_realize()
On Mon, 18 Sep 2023 18:02:37 +0200
Philippe Mathieu-Daudé wrote:
> QDev instance is expected to be in an unknown state until full
> object realization. Thus we shouldn't call DeviceReset() on an
> unrealized instance. Move the cpu_reset() call from *before*
> the parent realize() handler (effectively cpu_common_realizefn)
> to *after* it.
looking at:
--cpu_reset() <- brings cpu to known (after reset|poweron) state
cpu_common_realizefn()
if (dev->hotplugged) {
cpu_synchronize_post_init(cpu);
cpu_resume(cpu);
}
++cpu_reset() <-- looks to be too late, we already told hypervisor to run
undefined CPU, didn't we?
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> RFC:
> I haven't audited all the call sites, but plan to do it,
> amending the result to this patch description. This used
> to be a problem on some targets, but as of today's master
> this series pass make check-{qtest,avocado}.
> ---
> target/arm/cpu.c | 2 +-
> target/avr/cpu.c | 2 +-
> target/cris/cpu.c | 2 +-
> target/hexagon/cpu.c | 3 +--
> target/i386/cpu.c | 2 +-
> target/loongarch/cpu.c | 2 +-
> target/m68k/cpu.c | 2 +-
> target/mips/cpu.c | 2 +-
> target/nios2/cpu.c | 2 +-
> target/openrisc/cpu.c | 2 +-
> target/riscv/cpu.c | 2 +-
> target/rx/cpu.c| 2 +-
> target/s390x/cpu.c | 2 +-
> target/sh4/cpu.c | 2 +-
> target/tricore/cpu.c | 2 +-
> 15 files changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index b9e09a702d..6aca036b85 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2278,9 +2278,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error
> **errp)
> }
>
> qemu_init_vcpu(cs);
> -cpu_reset(cs);
>
> acc->parent_realize(dev, errp);
> +cpu_reset(cs);
> }
>
> static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index 8f741f258c..84d353f30e 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -120,9 +120,9 @@ static void avr_cpu_realizefn(DeviceState *dev, Error
> **errp)
> return;
> }
> qemu_init_vcpu(cs);
> -cpu_reset(cs);
>
> mcc->parent_realize(dev, errp);
> +cpu_reset(cs);
> }
>
> static void avr_cpu_set_int(void *opaque, int irq, int level)
> diff --git a/target/cris/cpu.c b/target/cris/cpu.c
> index a6a93c2359..079872a5cc 100644
> --- a/target/cris/cpu.c
> +++ b/target/cris/cpu.c
> @@ -152,10 +152,10 @@ static void cris_cpu_realizefn(DeviceState *dev, Error
> **errp)
> return;
> }
>
> -cpu_reset(cs);
> qemu_init_vcpu(cs);
>
> ccc->parent_realize(dev, errp);
> +cpu_reset(cs);
> }
>
> #ifndef CONFIG_USER_ONLY
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
> index f155936289..7edc32701f 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -346,9 +346,8 @@ static void hexagon_cpu_realize(DeviceState *dev, Error
> **errp)
> "hexagon-hvx.xml", 0);
>
> qemu_init_vcpu(cs);
> -cpu_reset(cs);
> -
> mcc->parent_realize(dev, errp);
> +cpu_reset(cs);
> }
>
> static void hexagon_cpu_init(Object *obj)
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index a23d4795e0..7faaa6915f 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7455,9 +7455,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error
> **errp)
> }
> }
> #endif /* !CONFIG_USER_ONLY */
> -cpu_reset(cs);
>
> xcc->parent_realize(dev, &local_err);
> +cpu_reset(cs);
>
> out:
> if (local_err != NULL) {
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index 65f9320e34..8029e70e76 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -565,10 +565,10 @@ static void loongarch_cpu_realizefn(DeviceState *dev,
> Error **errp)
>
> loongarch_cpu_register_gdb_regs_for_features(cs);
>
> -cpu_reset(cs);
> qemu_init_vcpu(cs);
>
> lacc->parent_realize(dev, errp);
> +cpu_reset(cs);
> }
>
> #ifndef CONFIG_USER_ONLY
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index 70d58471dc..2bc0a62f0e 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -321,10 +321,10 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error
> **errp)
>
> m68k_cpu_init_gdb(cpu);
>
> -cpu_reset(cs);
> qemu_init_vcpu(cs);
>
> mcc->parent_realize(dev, errp);
> +cpu_reset(cs);
> }
>
> static void m68k_cpu_initfn(Object *obj)
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index 63da1948fd..8d6f633f72 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -492,10 +492,10 @@ static void mips_cpu_realizefn(DeviceState *dev, Error
> **errp)
Re: [PULL 31/53] hw/i386/pc: Default to use SMBIOS 3.0 for newer machine models
On Tue, Nov 28, 2023 at 03:53:21PM +0100, Fiona Ebner wrote: > Am 28.11.23 um 15:13 schrieb Daniel P. Berrangé: > > On Tue, Nov 28, 2023 at 02:57:17PM +0100, Fiona Ebner wrote: > >> we received some reports about the new default causing issues for > >> certain guest OSes [0][1]. Namely, for Juniper vSRX, where boot fails > >> and Microsoft Windows, where querying an UUID set via QEMU cmdline > >> -smbios 'type=1,uuid=a4656bd0-a07d-48e0-9dfd-bdc84667a8d0' > >> in Powershell with > >> get-wmiobject win32_computersystemproduct | Select-Object > >> -expandProperty UUID > >> doesn't return any value anymore and can trip up some guest > >> applications. The original report is about Windows 10 and I reproduced > >> this with Windows Server 2019 and the German (but I hope it doesn't > >> matter this time) version of Windows Server 2022. > >> > >> Using machine type 8.0 or the machine option smbios-entry-point-type=32 > >> are workarounds. > >> > >> Since Windows is widely used, that seems a bit unfortunate. Just wanted > >> to ask if you are aware of the issue and if there is something else that > >> can be done other than specifying the more specific machine commandline > >> for those OSes? > > > > I don't recall seeing this issue mentioned before. Could you file a > > bug at https://gitlab.com/qemu-project/qemu > > > > I made one for the Windows issue: > https://gitlab.com/qemu-project/qemu/-/issues/2008 > > It's not clear to me if this is a bug in QEMU or just a bug/limitation > of the guest OS when 64 bit entry is used by SMBIOS. > > I didn't create one for vSRX, because I'm not using it myself and since > it's based on FreeBSD and FreeBSD 13.1 can boot just fine with both 32 > and 64 bit entry, it might be an issue on their side. > > Best Regards, > Fiona I would be inclined to limit this to when we have too many VCPUs then. Igor WDYT? -- MST
Re: [PULL 31/53] hw/i386/pc: Default to use SMBIOS 3.0 for newer machine models
On Tue, Nov 28, 2023 at 11:00:29AM -0500, Michael S. Tsirkin wrote: > On Tue, Nov 28, 2023 at 03:53:21PM +0100, Fiona Ebner wrote: > > Am 28.11.23 um 15:13 schrieb Daniel P. Berrangé: > > > On Tue, Nov 28, 2023 at 02:57:17PM +0100, Fiona Ebner wrote: > > >> we received some reports about the new default causing issues for > > >> certain guest OSes [0][1]. Namely, for Juniper vSRX, where boot fails > > >> and Microsoft Windows, where querying an UUID set via QEMU cmdline > > >> -smbios 'type=1,uuid=a4656bd0-a07d-48e0-9dfd-bdc84667a8d0' > > >> in Powershell with > > >> get-wmiobject win32_computersystemproduct | Select-Object > > >> -expandProperty UUID > > >> doesn't return any value anymore and can trip up some guest > > >> applications. The original report is about Windows 10 and I reproduced > > >> this with Windows Server 2019 and the German (but I hope it doesn't > > >> matter this time) version of Windows Server 2022. > > >> > > >> Using machine type 8.0 or the machine option smbios-entry-point-type=32 > > >> are workarounds. > > >> > > >> Since Windows is widely used, that seems a bit unfortunate. Just wanted > > >> to ask if you are aware of the issue and if there is something else that > > >> can be done other than specifying the more specific machine commandline > > >> for those OSes? > > > > > > I don't recall seeing this issue mentioned before. Could you file a > > > bug at https://gitlab.com/qemu-project/qemu > > > > > > > I made one for the Windows issue: > > https://gitlab.com/qemu-project/qemu/-/issues/2008 > > > > It's not clear to me if this is a bug in QEMU or just a bug/limitation > > of the guest OS when 64 bit entry is used by SMBIOS. > > > > I didn't create one for vSRX, because I'm not using it myself and since > > it's based on FreeBSD and FreeBSD 13.1 can boot just fine with both 32 > > and 64 bit entry, it might be an issue on their side. > > > > Best Regards, > > Fiona > > I would be inclined to limit this to when we have too many VCPUs then. IIRC, it wasn't merely number of vCPUs that was a problem - a number of other aspects can also influence the SMBIOS table size. With 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 05/22] exec/cpu: Call qemu_init_vcpu() once in cpu_common_realize()
On Mon, 18 Sep 2023 18:02:38 +0200
Philippe Mathieu-Daudé wrote:
> qemu_init_vcpu() is called in each ${target}_cpu_realize() before
> the call to parent_realize(), which is cpu_common_realizefn().
> Call it once there.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/core/cpu-common.c| 3 +++
> target/alpha/cpu.c | 2 --
> target/arm/cpu.c| 2 --
> target/avr/cpu.c| 1 -
> target/cris/cpu.c | 2 --
> target/hexagon/cpu.c| 1 -
> target/hppa/cpu.c | 1 -
> target/i386/cpu.c | 4 +---
> target/loongarch/cpu.c | 2 --
> target/m68k/cpu.c | 2 --
> target/microblaze/cpu.c | 2 --
> target/mips/cpu.c | 2 --
> target/nios2/cpu.c | 1 -
> target/openrisc/cpu.c | 2 --
> target/ppc/cpu_init.c | 1 -
> target/riscv/cpu.c | 2 --
> target/rx/cpu.c | 2 --
> target/s390x/cpu.c | 1 -
> target/sh4/cpu.c| 2 --
> target/sparc/cpu.c | 2 --
> target/tricore/cpu.c| 1 -
> target/xtensa/cpu.c | 2 --
> 22 files changed, 4 insertions(+), 36 deletions(-)
>
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index ced66c2b34..a3b8de7054 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -204,6 +204,9 @@ static void cpu_common_realizefn(DeviceState *dev, Error
> **errp)
> }
> }
>
> +/* Create CPU address space and vCPU thread */
> +qemu_init_vcpu(cpu);
> +
> if (dev->hotplugged) {
> cpu_synchronize_post_init(cpu);
> cpu_resume(cpu);
> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
> index 270ae787b1..eb78318bb8 100644
> --- a/target/alpha/cpu.c
> +++ b/target/alpha/cpu.c
> @@ -82,8 +82,6 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error
> **errp)
> return;
> }
>
> -qemu_init_vcpu(cs);
> -
> acc->parent_realize(dev, errp);
> }
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 6aca036b85..fc3772025c 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2277,8 +2277,6 @@ static void arm_cpu_realizefn(DeviceState *dev, Error
> **errp)
> }
> }
>
> -qemu_init_vcpu(cs);
> -
> acc->parent_realize(dev, errp);
> cpu_reset(cs);
> }
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index 84d353f30e..d3460b3960 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -119,7 +119,6 @@ static void avr_cpu_realizefn(DeviceState *dev, Error
> **errp)
> error_propagate(errp, local_err);
> return;
> }
> -qemu_init_vcpu(cs);
>
> mcc->parent_realize(dev, errp);
> cpu_reset(cs);
> diff --git a/target/cris/cpu.c b/target/cris/cpu.c
> index 079872a5cc..671693a362 100644
> --- a/target/cris/cpu.c
> +++ b/target/cris/cpu.c
> @@ -152,8 +152,6 @@ static void cris_cpu_realizefn(DeviceState *dev, Error
> **errp)
> return;
> }
>
> -qemu_init_vcpu(cs);
> -
> ccc->parent_realize(dev, errp);
> cpu_reset(cs);
> }
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
> index 7edc32701f..5b9bb3fe83 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -345,7 +345,6 @@ static void hexagon_cpu_realize(DeviceState *dev, Error
> **errp)
> NUM_VREGS + NUM_QREGS,
> "hexagon-hvx.xml", 0);
>
> -qemu_init_vcpu(cs);
> mcc->parent_realize(dev, errp);
> cpu_reset(cs);
> }
> diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
> index 11022f9c99..49082bd2ba 100644
> --- a/target/hppa/cpu.c
> +++ b/target/hppa/cpu.c
> @@ -131,7 +131,6 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error
> **errp)
> return;
> }
>
> -qemu_init_vcpu(cs);
> acc->parent_realize(dev, errp);
>
> #ifndef CONFIG_USER_ONLY
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 7faaa6915f..cb41d30aab 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7425,15 +7425,13 @@ static void x86_cpu_realizefn(DeviceState *dev, Error
> **errp)
>
> mce_init(cpu);
>
> -qemu_init_vcpu(cs);
> -
> /*
> * Most Intel and certain AMD CPUs support hyperthreading. Even though
> QEMU
> * fixes this issue by adjusting CPUID__0001_EBX and
> CPUID_8000_0008_ECX
> * based on inputs (sockets,cores,threads), it is still better to give
> * users a warning.
> *
> - * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise
> + * NOTE: the following code has to follow cpu_common_realize(). Otherwise
Does moving qemu_init_vcpu() past check and altering comment here
makes cs->nr_threads somehow correct for following if()?
> * cs->nr_threads hasn't be populated yet and the checking is incorrect.
> */
> if (IS_AMD_CPU(env) &&
missing context:
if (IS_AMD_CPU(env) &&
!(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
cs->nr_threads > 1 && !ht_warned) {
> diff --git a/target/loongarch/cpu.c b/targ
Re: [PATCH v2] crypto: Introduce SM4 symmetric cipher algorithm
On Tue, Nov 28, 2023 at 04:57:20PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Hyman,
>
> On 28/11/23 16:24, Hyman Huang wrote:
> > Introduce the SM4 cipher algorithms (OSCCA GB/T 32907-2016).
> >
> > SM4 (GBT.32907-2016) is a cryptographic standard issued by the
> > Organization of State Commercial Administration of China (OSCCA)
> > as an authorized cryptographic algorithms for the use within China.
> >
> > Use the crypto-sm4 meson build option for enabling this feature.
> >
> > Signed-off-by: Hyman Huang
> > ---
> > crypto/block-luks.c | 11
> > crypto/cipher-gcrypt.c.inc | 8 ++
> > crypto/cipher-nettle.c.inc | 49 +
> > crypto/cipher.c | 6
> > meson.build | 23
> > meson_options.txt | 2 ++
> > qapi/crypto.json| 5 +++-
> > scripts/meson-buildoptions.sh | 3 ++
> > tests/unit/test-crypto-cipher.c | 13 +
> > 9 files changed, 119 insertions(+), 1 deletion(-)
>
>
> > diff --git a/meson.build b/meson.build
> > index ec01f8b138..256d3257bb 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1480,6 +1480,7 @@ endif
> > gcrypt = not_found
> > nettle = not_found
> > hogweed = not_found
> > +crypto_sm4 = not_found
> > xts = 'none'
> > if get_option('nettle').enabled() and get_option('gcrypt').enabled()
> > @@ -1514,6 +1515,26 @@ if not gnutls_crypto.found()
> > xts = 'private'
> > endif
> > endif
> > + if get_option('crypto_sm4').enabled()
>
> We want to detect it by default (not only when explicitly enabled) ...
>
> > +if get_option('gcrypt').enabled()
> > + # SM4 ALG is available in libgcrypt >= 1.9
> > + crypto_sm4 = dependency('libgcrypt', version: '>=1.9',
> > + method: 'config-tool',
> > + required: get_option('gcrypt'))
> > + # SM4 ALG static compilation
> > + if crypto_sm4.found() and get_option('prefer_static')
> > +crypto_sm4 = declare_dependency(dependencies: [
> > + crypto_sm4,
> > + cc.find_library('gpg-error', required: true)],
> > + version: crypto_sm4.version())
> > + endif
> > +else
> > + # SM4 ALG is available in nettle >= 3.9
> > + crypto_sm4 = dependency('nettle', version: '>=3.9',
> > + method: 'pkg-config',
> > + required: get_option('nettle'))
> > +endif
>
> ... and if it was forced with --enable-crypto_sm4 AND not found,
> display an error.
>
> IIUC your config you try to find the best effort implementation then
> if not found, keep going silently.
Yes, ignore the get_option() calls, and instead look at .found()
in the library we just detected
ie
if nettle.found()
check sm4 in nettle
endif
if gcrypt.found()
check sm4 in crypt
endif
With 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: [PULL 00/13] target-arm queue
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/8.2 for any user-visible changes. signature.asc Description: PGP signature
Re: [PATCH for-8.2 0/2] qdev array property fixes
On Tue, 21 Nov 2023 at 12:35, Kevin Wolf wrote: > > Kevin Wolf (2): > qdev: Fix crash in array property getter > string-output-visitor: Support lists for non-integer types > > hw/core/qdev-properties.c| 33 ++--- > qapi/string-output-visitor.c | 24 > 2 files changed, 46 insertions(+), 11 deletions(-) Applied, thanks! Stefan > > -- > 2.42.0 > >
Re: [PULL 0/2] Firmware/seabios 20231128 patches
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/8.2 for any user-visible changes. signature.asc Description: PGP signature
Re: [PATCH-for-9.0 05/11] target/arm: Move GTIMER definitions to 'cpu-defs.h'
On 28/11/23 15:02, Richard Henderson wrote: On 11/22/23 12:30, Philippe Mathieu-Daudé wrote: To allow GTIMER_* definitions to be used by non-ARM specific hardware models, move them to a new target agnostic "cpu-defs.h" header. Signed-off-by: Philippe Mathieu-Daudé --- target/arm/cpu-defs.h | 19 +++ target/arm/cpu.h | 8 +--- hw/arm/bcm2836.c | 1 + 3 files changed, 21 insertions(+), 7 deletions(-) create mode 100644 target/arm/cpu-defs.h diff --git a/target/arm/cpu-defs.h b/target/arm/cpu-defs.h new file mode 100644 index 00..1ad76aff14 --- /dev/null +++ b/target/arm/cpu-defs.h @@ -0,0 +1,19 @@ +/* + * ARM "target agnostic" CPU definitions + * + * Copyright (c) 2003 Fabrice Bellard + * + * SPDX-License-Identifier: LGPL-2.1-or-later + */ + +#ifndef ARM_CPU_DEFS_H +#define ARM_CPU_DEFS_H + +#define GTIMER_PHYS 0 +#define GTIMER_VIRT 1 +#define GTIMER_HYP 2 +#define GTIMER_SEC 3 +#define GTIMER_HYPVIRT 4 +#define NUM_GTIMERS 5 + +#endif Hmm. cpu-defs.h is pretty generic. Without looking forward in the patch series, perhaps better as gtimer.h? - target specific parameters used by accel/ (stay) in "cpu-param.h" (Ideally the single header included by accel/) - target accelerator implementation details (stay) in "cpu.h" Shouldn't be used outside of target/ - architecture definitions in "arch-defs.h" (used by target/ and hw/) - QEMU specific implementation details in "cpu-qom.h" (mostly used by hw/) Is hw/arm/bcm2836.c really "non-arm-specific"? Or did you mean "non-ARMCPU-specific"? I'd like to eventually have it instantiate a CPU which is not ARM based.
