Re: [PATCH v3 2/2] vhost-user: fix lost reconnect again
> 2024年5月14日 21:58，Raphael Norwitz 写道：
>
> Code looks good. Just a question on the error case you're trying to fix.
>
> On Tue, May 14, 2024 at 2:12 AM Li Feng wrote:
>>
>> When the vhost-user is reconnecting to the backend, and if the vhost-user
>> fails
>> at the get_features in vhost_dev_init(), then the reconnect will fail
>> and it will not be retriggered forever.
>>
>> The reason is:
>> When the vhost-user fail at get_features, the vhost_dev_cleanup will be
>> called
>> immediately.
>>
>> vhost_dev_cleanup calls 'memset(hdev, 0, sizeof(struct vhost_dev))'.
>>
>> The reconnect path is:
>> vhost_user_blk_event
>> vhost_user_async_close(.. vhost_user_blk_disconnect ..)
>> qemu_chr_fe_set_handlers <- clear the notifier callback
>> schedule vhost_user_async_close_bh
>>
>> The vhost->vdev is null, so the vhost_user_blk_disconnect will not be
>> called, then the event fd callback will not be reinstalled.
>>
>> With this patch, the vhost_user_blk_disconnect will call the
>> vhost_dev_cleanup() again, it's safe.
>>
>> In addition, the CLOSE event may occur in a scenario where connected is
>> false.
>> At this time, the event handler will be cleared. We need to ensure that the
>> event handler can remain installed.
>
> Following on from the prior patch, why would "connected" be false when
> a CLOSE event happens?
In OPEN event handling, vhost_user_blk_connect calls vhost_dev_init and
encounters
an error such that s->connected remains false.
Next, after the CLOSE event arrives, it is found that s->connected is false, so
nothing
is done, but the event handler will be cleaned up in `vhost_user_async_close`
before the CLOSE event is executed.
Thanks,
Li
>
>>
>> All vhost-user devices have this issue, including vhost-user-blk/scsi.
>>
>> Fixes: 71e076a07d ("hw/virtio: generalise CHR_EVENT_CLOSED handling")
>>
>> Signed-off-by: Li Feng
>> ---
>> hw/block/vhost-user-blk.c | 3 ++-
>> hw/scsi/vhost-user-scsi.c | 3 ++-
>> hw/virtio/vhost-user-base.c | 3 ++-
>> hw/virtio/vhost-user.c | 10 +-
>> 4 files changed, 7 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>> index 41d1ac3a5a..c6842ced48 100644
>> --- a/hw/block/vhost-user-blk.c
>> +++ b/hw/block/vhost-user-blk.c
>> @@ -353,7 +353,7 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
>> VHostUserBlk *s = VHOST_USER_BLK(vdev);
>>
>> if (!s->connected) {
>> -return;
>> +goto done;
>> }
>> s->connected = false;
>>
>> @@ -361,6 +361,7 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
>>
>> vhost_dev_cleanup(>dev);
>>
>> +done:
>> /* Re-instate the event handler for new connections */
>> qemu_chr_fe_set_handlers(>chardev, NULL, NULL, vhost_user_blk_event,
>> NULL, dev, NULL, true);
>> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
>> index 48a59e020e..b49a11d23b 100644
>> --- a/hw/scsi/vhost-user-scsi.c
>> +++ b/hw/scsi/vhost-user-scsi.c
>> @@ -181,7 +181,7 @@ static void vhost_user_scsi_disconnect(DeviceState *dev)
>> VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
>>
>> if (!s->connected) {
>> -return;
>> +goto done;
>> }
>> s->connected = false;
>>
>> @@ -189,6 +189,7 @@ static void vhost_user_scsi_disconnect(DeviceState *dev)
>>
>> vhost_dev_cleanup(>dev);
>>
>> +done:
>> /* Re-instate the event handler for new connections */
>> qemu_chr_fe_set_handlers(>conf.chardev, NULL, NULL,
>> vhost_user_scsi_event, NULL, dev, NULL, true);
>> diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
>> index 4b54255682..11e72b1e3b 100644
>> --- a/hw/virtio/vhost-user-base.c
>> +++ b/hw/virtio/vhost-user-base.c
>> @@ -225,13 +225,14 @@ static void vub_disconnect(DeviceState *dev)
>> VHostUserBase *vub = VHOST_USER_BASE(vdev);
>>
>> if (!vub->connected) {
>> -return;
>> +goto done;
>> }
>> vub->connected = false;
>>
>> vub_stop(vdev);
>> vhost_dev_cleanup(>vhost_dev);
>>
>> +done:
>> /* Re-instate the event handler for new connections */
>> qemu_chr_fe_set_handlers(>chardev,
>> NULL, NULL, vub_event,
>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> index c929097e87..c407ea8939 100644
>> --- a/hw/virtio/vhost-user.c
>> +++ b/hw/virtio/vhost-user.c
>> @@ -2781,16 +2781,8 @@ typedef struct {
>> static void vhost_user_async_close_bh(void *opaque)
>> {
>> VhostAsyncCallback *data = opaque;
>> -struct vhost_dev *vhost = data->vhost;
>>
>> -/*
>> - * If the vhost_dev has been cleared in the meantime there is
>> - * nothing left to do as some other path has completed the
>> - * cleanup.
>> - */
>> -if (vhost->vdev) {
>> -data->cb(data->dev);
>> -}
>> +data->cb(data->dev);
>>
>> g_free(data);
>> }
>> --
>> 2.45.0
>>
Re: [PATCH v3 1/2] Revert "vhost-user: fix lost reconnect"
> 2024年5月14日 21:58，Raphael Norwitz 写道：
>
> The code for these two patches looks fine. Just some questions on the
> failure case you're trying to fix.
>
>
> On Tue, May 14, 2024 at 2:12 AM Li Feng wrote:
>>
>> This reverts commit f02a4b8e6431598612466f76aac64ab492849abf.
>>
>> Since the current patch cannot completely fix the lost reconnect
>> problem, there is a scenario that is not considered:
>> - When the virtio-blk driver is removed from the guest os,
>> s->connected has no chance to be set to false, resulting in
>
> Why would the virtio-blk driver being removed (unloaded?) in the guest
> effect s->connected? Isn't this variable just tracking whether Qemu is
> connected to the backend process? What does it have to do with the
> guest driver state?
Unload the virtio-blk, it will trigger ‘vhost_user_blk_stop’, and in
`vhost_dev_stop`
it will set the `hdev->vdev = NULL;`.
Next if kill the backend, the CLOSE event will be triggered, and the
`vhost->vdev`
has been set to null before, then the `vhost_user_blk_disconnect` will not have
a
chance to execute.So that he s->connected is still true.
static void vhost_user_async_close_bh(void *opaque)
{
VhostAsyncCallback *data = opaque;
struct vhost_dev *vhost = data->vhost;
/*
* If the vhost_dev has been cleared in the meantime there is
* nothing left to do as some other path has completed the
* cleanup.
*/
if (vhost->vdev) { < HERE vdev is null.
data->cb(data->dev);
} else if (data->event_cb) {
qemu_chr_fe_set_handlers(data->cd, NULL, NULL, data->event_cb,
NULL, data->dev, NULL, true);
}
g_free(data);
}
Thanks,
Li
>
>> subsequent reconnection not being executed.
>>
>> The next patch will completely fix this issue with a better approach.
>>
>> Signed-off-by: Li Feng
>> ---
>> hw/block/vhost-user-blk.c | 2 +-
>> hw/scsi/vhost-user-scsi.c | 3 +--
>> hw/virtio/vhost-user-base.c| 2 +-
>> hw/virtio/vhost-user.c | 10 ++
>> include/hw/virtio/vhost-user.h | 3 +--
>> 5 files changed, 6 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>> index 9e6bbc6950..41d1ac3a5a 100644
>> --- a/hw/block/vhost-user-blk.c
>> +++ b/hw/block/vhost-user-blk.c
>> @@ -384,7 +384,7 @@ static void vhost_user_blk_event(void *opaque,
>> QEMUChrEvent event)
>> case CHR_EVENT_CLOSED:
>> /* defer close until later to avoid circular close */
>> vhost_user_async_close(dev, >chardev, >dev,
>> - vhost_user_blk_disconnect,
>> vhost_user_blk_event);
>> + vhost_user_blk_disconnect);
>> break;
>> case CHR_EVENT_BREAK:
>> case CHR_EVENT_MUX_IN:
>> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
>> index a63b1f4948..48a59e020e 100644
>> --- a/hw/scsi/vhost-user-scsi.c
>> +++ b/hw/scsi/vhost-user-scsi.c
>> @@ -214,8 +214,7 @@ static void vhost_user_scsi_event(void *opaque,
>> QEMUChrEvent event)
>> case CHR_EVENT_CLOSED:
>> /* defer close until later to avoid circular close */
>> vhost_user_async_close(dev, >conf.chardev, >dev,
>> - vhost_user_scsi_disconnect,
>> - vhost_user_scsi_event);
>> + vhost_user_scsi_disconnect);
>> break;
>> case CHR_EVENT_BREAK:
>> case CHR_EVENT_MUX_IN:
>> diff --git a/hw/virtio/vhost-user-base.c b/hw/virtio/vhost-user-base.c
>> index a83167191e..4b54255682 100644
>> --- a/hw/virtio/vhost-user-base.c
>> +++ b/hw/virtio/vhost-user-base.c
>> @@ -254,7 +254,7 @@ static void vub_event(void *opaque, QEMUChrEvent event)
>> case CHR_EVENT_CLOSED:
>> /* defer close until later to avoid circular close */
>> vhost_user_async_close(dev, >chardev, >vhost_dev,
>> - vub_disconnect, vub_event);
>> + vub_disconnect);
>> break;
>> case CHR_EVENT_BREAK:
>> case CHR_EVENT_MUX_IN:
>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> index cdf9af4a4b..c929097e87 100644
>> --- a/hw/virtio/vhost-user.c
>> +++ b/hw/virtio/vhost-user.c
>> @@ -2776,7 +2776,6 @@ typedef struct {
>> DeviceState *dev;
>> CharBackend *cd;
>> struct vhost_dev *vhost;
>> -IOEventHandler *event_cb;
>> } VhostAsyncCallback;
>>
>> static void vhost_user_async_close_bh(void *opaque)
>> @@ -2791,10 +2790,7 @@ static void vhost_user_async_close_bh(void *opaque)
>> */
>> if (vhost->vdev) {
>> data->cb(data->dev);
>> -} else if (data->event_cb) {
>> -qemu_chr_fe_set_handlers(data->cd, NULL, NULL, data->event_cb,
>> - NULL, data->dev, NULL, true);
>> - }
>> +}
>>
>> g_free(data);
>> }
>> @@ -2806,8 +2802,7 @@ static void vhost_user_async_close_bh(void *opaque)
>> */
RE: [PATCH v3 07/16] aspeed/smc: fix dma moving incorrect data length issue
Hi Cedric,
Sorry reply you late.
> On 4/30/24 10:51, Jamin Lin wrote:
> > Hi Cedric,
> >> On 4/19/24 15:41, Cédric Le Goater wrote:
> >>> On 4/16/24 11:18, Jamin Lin wrote:
> DMA length is from 1 byte to 32MB for AST2600 and AST10x0 and DMA
> length is from 4 bytes to 32MB for AST2500.
>
> In other words, if "R_DMA_LEN" is 0, it should move at least 1 byte
> data for AST2600 and AST10x0 and 4 bytes data for AST2500.
> > To support all ASPEED SOCs, adds dma_start_length parameter to
> > store
> the start length, add helper routines function to compute the dma
> length and update DMA_LENGTH mask to "1FF" to fix dma moving
> incorrect data length issue.
> >>>
> >>> OK. There are two problems to address, the "zero" length transfer
> >>> and the DMA length unit, which is missing today. Newer SoC use a 1
> >>> bit / byte and older ones, AST2400 and AST2500, use 1 bit / 4 bytes.
> >>>
> >>> We can introduce a AspeedSMCClass::dma_len_unit and rework the loop
> to :
> >>>
> >>> do {
> >>>
> >>>
> >>>
> >>> if (s->regs[R_DMA_LEN]) {
> >>> s->regs[R_DMA_LEN] -= 4 / asc->dma_len_unit;
> >>> }
> >>> } while (s->regs[R_DMA_LEN]);
> >>>
> >>> It should fix the current implementation.
> >>
> >>
> >> I checked what FW is doing on a QEMU ast2500-evb machine :
> >>
> >> U-Boot 2019.04-v00.04.12 (Sep 29 2022 - 10:40:37 +)
> >> ...
> >>
> >> Loading Kernel Image ... aspeed_smc_write @0x88 size 4:
> >> 0x80001000
> >> aspeed_smc_write @0x84 size 4: 0x20100130
> >> aspeed_smc_write @0x8c size 4: 0x3c6770
> >> aspeed_smc_write @0x80 size 4: 0x1
> >> aspeed_smc_dma_rw read flash:@0x00100130 dram:@0x1000
> >> size:0x003c6774
> >> aspeed_smc_read @0x8 size 4: 0x800
> >> aspeed_smc_write @0x80 size 4: 0x0
> >> OK
> >> Loading Ramdisk to 8fef2000, end 8604 ...
> >> aspeed_smc_write
> >> @0x88 size 4: 0x8fef2000
> >> aspeed_smc_write @0x84 size 4: 0x204cdde0
> >> aspeed_smc_write @0x8c size 4: 0x10d604
> >> aspeed_smc_write @0x80 size 4: 0x1
> >> aspeed_smc_dma_rw read flash:@0x004cdde0 dram:@0xfef2000
> >> size:0x0010d608
> >> aspeed_smc_read @0x8 size 4: 0x800
> >> aspeed_smc_write @0x80 size 4: 0x0
> >> OK
> >> Loading Device Tree to 8fee7000, end 8fef135e ...
> >> aspeed_smc_write
> >> @0x88 size 4: 0x8fee7000
> >> aspeed_smc_write @0x84 size 4: 0x204c69b4
> >> aspeed_smc_write @0x8c size 4: 0x7360
> >> aspeed_smc_write @0x80 size 4: 0x1
> >> aspeed_smc_dma_rw read flash:@0x004c69b4 dram:@0xfee7000
> >> size:0x7364
> >> aspeed_smc_read @0x8 size 4: 0x800
> >> aspeed_smc_write @0x80 size 4: 0x0
> >> OK
> >>
> >> Starting kernel ...
> >>
> >> It seems that the R_DMA_LEN register is set by FW without taking into
> >> account the length unit ( bit / 4 bytes). Would you know why ?
> >>
After I discussed with designers, my explanation as below.
AST2500 datasheet description:
FMC8C: DMA Length Register
31:25 Reserved(0)
24:2 RW DMA_LENGTH
From 4bytes to 32MB
0: 4bytes
0x7F: 32MB
1:0 Reserved
If users set this register 0, SPI DMA controller would move 4 bytes data.
If users set this register bits[24:2] 0x7F, SPI DMC controller would move
32MB data because this register includes reserved bits [1:0].
Ex: bits 24:2 --> 0x7f
bits 1:0 --> 0 Reserved
bits[24:0] is (0x1FC + start length 4) --> 200 --> 32MB
111 00
LENGTH[24:2] 7 FF F FFReserved [1:0]
That was why AST2500 DMA length should 4 bytes alignment and you do not see
handling length units because it includes reserved bits 1 and 0.
If user want to move 3 bytes data, our firmware set this register 4 ensure it
4bytes alignment and the value of register as following
bits[2:0] = "b100" Finally, SPI DMA controller move 8 bytes data(4bytes input
count + start length 4bytes)
And that was why it set DMA_LENGTH 0x01FC
https://github.com/qemu/qemu/blob/master/hw/ssi/aspeed_smc.c#L187
#define DMA_LENGTH(val) ((val) & 0x01FC)
I want to change it to 0x01FF to support all ASPEED SOCs because AST2600,
AST2700 and AST1030 use this register bit 0 and 1 whose unit is 1byte rather
than 4 bytes.
> > https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2019.04/l
> > ib/string.c#L559 This line make user input data length 4 bytes
> > alignment.
> >
> https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2019.04/a
> > rch/arm/mach-aspeed/ast2500/utils.S#L35
>
> yes. I don't see any 1bit / 4bytes conversion when setting the DMA_LEN
> register. Am I mistaking ? That's not what the specs says.
>
> > This line set the value of count parameter to AST_FMC_DNA_LENGTH.
> > AST_FMC_DMA_LENGTH is 4 bytes alignment value.
> > Example: input 4
> > ((4+3)/4) * 4 -->
[RFC PATCH v2] target/loongarch/kvm: Add pmu support
This patch adds PMU support, We just sets some cpucfg6 default value
to PMU config on kvm mode, and then check the PMU config with kvm ioctl
KVM_GET_DEVICE_ATTR.
e.g
'... -cpu max,pmu=on' (enable PMU)'
'... -cpu max,pmu=off' (disable PMU)'
Signed-off-by: Song Gao
---
v2:
- Drop the property 'pmnum'.
- Link to v1:
https://patchew.org/QEMU/20240514094630.988617-1-gaos...@loongson.cn/
This patch adds the 'RFC' heading because it requires
the kernel to merge into patch[1] first
[1]: https://lore.kernel.org/all/20240507120140.3119714-1-gaos...@loongson.cn/
target/loongarch/cpu.c| 27 ++
target/loongarch/kvm/kvm.c| 53 ++-
target/loongarch/loongarch-qmp-cmds.c | 2 +-
3 files changed, 80 insertions(+), 2 deletions(-)
diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index a0cad53676..fde0d2a816 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -421,6 +421,14 @@ static void loongarch_la464_initfn(Object *obj)
data = FIELD_DP32(data, CPUCFG5, CC_DIV, 1);
env->cpucfg[5] = data;
+if (kvm_enabled()) {
+data = 0;
+data = FIELD_DP32(data, CPUCFG6, PMP, 1);
+data = FIELD_DP32(data, CPUCFG6, PMNUM, 3);
+data = FIELD_DP32(data, CPUCFG6, PMBITS, 63);
+env->cpucfg[6] = data;
+}
+
data = 0;
data = FIELD_DP32(data, CPUCFG16, L1_IUPRE, 1);
data = FIELD_DP32(data, CPUCFG16, L1_DPRE, 1);
@@ -643,6 +651,20 @@ static void loongarch_set_lasx(Object *obj, bool value,
Error **errp)
}
}
+static bool loongarch_get_pmu(Object *obj, Error **errp)
+{
+LoongArchCPU *cpu = LOONGARCH_CPU(obj);
+
+return !!(FIELD_EX32(cpu->env.cpucfg[6], CPUCFG6, PMP));
+}
+
+static void loongarch_set_pmu(Object *obj, bool value, Error **errp)
+{
+LoongArchCPU *cpu = LOONGARCH_CPU(obj);
+
+cpu->env.cpucfg[6] = FIELD_DP32(cpu->env.cpucfg[6], CPUCFG6, PMP, value);
+}
+
void loongarch_cpu_post_init(Object *obj)
{
LoongArchCPU *cpu = LOONGARCH_CPU(obj);
@@ -655,6 +677,11 @@ void loongarch_cpu_post_init(Object *obj)
object_property_add_bool(obj, "lasx", loongarch_get_lasx,
loongarch_set_lasx);
}
+
+if (kvm_enabled()) {
+object_property_add_bool(obj, "pmu", loongarch_get_pmu,
+ loongarch_set_pmu);
+}
}
static void loongarch_cpu_init(Object *obj)
diff --git a/target/loongarch/kvm/kvm.c b/target/loongarch/kvm/kvm.c
index bc75552d0f..3e46203b15 100644
--- a/target/loongarch/kvm/kvm.c
+++ b/target/loongarch/kvm/kvm.c
@@ -556,6 +556,51 @@ static int kvm_check_cpucfg2(CPUState *cs)
return ret;
}
+static int kvm_check_cpucfg6(CPUState *cs)
+{
+int ret;
+uint64_t val;
+struct kvm_device_attr attr = {
+.group = KVM_LOONGARCH_VCPU_CPUCFG,
+.attr = 6,
+.addr = (uint64_t),
+};
+LoongArchCPU *cpu = LOONGARCH_CPU(cs);
+CPULoongArchState *env = >env;
+
+ret = kvm_vcpu_ioctl(cs, KVM_HAS_DEVICE_ATTR, );
+if (!ret) {
+kvm_vcpu_ioctl(cs, KVM_GET_DEVICE_ATTR, );
+
+if (FIELD_EX32(env->cpucfg[6], CPUCFG6, PMP)) {
+ /* Check PMP */
+ if (!FIELD_EX32(val, CPUCFG6, PMP)) {
+ error_report("'pmu' feature not supported by KVM on this host"
+ " Please disable 'pmu' with "
+ "'... -cpu XXX,pmu=off ...'\n");
+ exit(EXIT_FAILURE);
+ }
+ /* Check PMNUM */
+ int guest_pmnum = FIELD_EX32(env->cpucfg[6], CPUCFG6, PMNUM);
+ int host_pmnum = FIELD_EX32(val, CPUCFG6, PMNUM);
+ if (guest_pmnum > host_pmnum){
+ error_report("The guest pmnum %d larger than KVM support
%d\n",
+ guest_pmnum, host_pmnum);
+ exit(EXIT_FAILURE);
+ }
+ /* Check PMBITS */
+ int guest_pmbits = FIELD_EX32(env->cpucfg[6], CPUCFG6, PMBITS);
+ int host_pmbits = FIELD_EX32(val, CPUCFG6, PMBITS);
+ if (guest_pmbits != host_pmbits) {
+ exit_report("The host not support PMBITS %d\n", guest_pmbits);
+ exit(EXIT_FAILURE);
+ }
+}
+}
+
+return ret;
+}
+
static int kvm_loongarch_put_cpucfg(CPUState *cs)
{
int i, ret = 0;
@@ -568,7 +613,13 @@ static int kvm_loongarch_put_cpucfg(CPUState *cs)
if (ret) {
return ret;
}
- }
+}
+if (i == 6) {
+ret = kvm_check_cpucfg6(cs);
+if (ret) {
+return ret;
+}
+}
val = env->cpucfg[i];
ret = kvm_set_one_reg(cs, KVM_IOC_CPUCFG(i), );
if (ret < 0) {
diff --git a/target/loongarch/loongarch-qmp-cmds.c
b/target/loongarch/loongarch-qmp-cmds.c
index 8721a5eb13..7e1a6fd734 100644
---
[PATCH 08/20] qapi/parser: differentiate intro and outro paragraphs
Add a semantic tag to paragraphs that appear *before* tagged
sections/members/features and those that appear after. This will control
how they are inlined when doc sections are merged and flattened.
Signed-off-by: John Snow
---
scripts/qapi/parser.py | 22 +-
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index cf4cbca1c1f..b1794f71e12 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -503,6 +503,10 @@ def get_doc(self) -> 'QAPIDoc':
self.accept(False)
line = self.get_doc_line()
no_more_args = False
+# Paragraphs before members/features/tagged are "intro" paragraphs.
+# Any appearing subsequently are "outro" paragraphs.
+# This is only semantic metadata for the doc generator.
+intro = True
while line is not None:
# Blank lines
@@ -532,6 +536,7 @@ def get_doc(self) -> 'QAPIDoc':
raise QAPIParseError(
self, 'feature descriptions expected')
no_more_args = True
+intro = False
elif match := self._match_at_name_colon(line):
# description
if no_more_args:
@@ -547,6 +552,7 @@ def get_doc(self) -> 'QAPIDoc':
doc.append_line(text)
line = self.get_doc_indented(doc)
no_more_args = True
+intro = False
elif match := re.match(
r'(Returns|Errors|Since|Notes?|Examples?|TODO): *',
line):
@@ -557,13 +563,14 @@ def get_doc(self) -> 'QAPIDoc':
doc.append_line(text)
line = self.get_doc_indented(doc)
no_more_args = True
+intro = False
elif line.startswith('='):
raise QAPIParseError(
self,
"unexpected '=' markup in definition documentation")
else:
# tag-less paragraph
-doc.ensure_untagged_section(self.info)
+doc.ensure_untagged_section(self.info, intro)
doc.append_line(line)
line = self.get_doc_paragraph(doc)
else:
@@ -617,7 +624,7 @@ def __init__(
self,
info: QAPISourceInfo,
tag: Optional[str] = None,
-kind: str = 'paragraph',
+kind: str = 'intro-paragraph',
):
# section source info, i.e. where it begins
self.info = info
@@ -625,7 +632,7 @@ def __init__(
self.tag = tag
# section text without tag
self.text = ''
-# section type - {paragraph, feature, member, tagged}
+# section type - {-paragraph, feature, member, tagged}
self.kind = kind
def append_line(self, line: str) -> None:
@@ -666,7 +673,11 @@ def end(self) -> None:
raise QAPISemError(
section.info, "text required after '%s:'" % section.tag)
-def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
+def ensure_untagged_section(
+self,
+info: QAPISourceInfo,
+intro: bool = True,
+) -> None:
if self.all_sections and not self.all_sections[-1].tag:
section = self.all_sections[-1]
# Section is empty so far; update info to start *here*.
@@ -677,7 +688,8 @@ def ensure_untagged_section(self, info: QAPISourceInfo) ->
None:
self.all_sections[-1].text += '\n'
return
# start new section
-section = self.Section(info)
+kind = ("intro" if intro else "outro") + "-paragraph"
+section = self.Section(info, kind=kind)
self.sections.append(section)
self.all_sections.append(section)
--
2.44.0
[PATCH 11/20] qapi/schema: add doc_visible property to QAPISchemaDefinition
The intent here is to mark only certain definitions as visible in the end-user docs. All commands and events are inherently visible. Everything else is visible only if it is a member (or a branch member) of a type that is visible, or if it is named as a return type for a command. Notably, this excludes arg_type for commands and events, and any base_types specified for structures/unions. Those objects may still be marked visible if they are named as members from a visible type. This does not necessarily match the data revealed by introspection: in this case, we want anything that we are cross-referencing in generated documentation to be available to target. Some internal and built-in types may be marked visible with this approach, but if they do not have a documentation block, they'll be skipped by the generator anyway. This includes array types and built-in primitives which do not get their own documentation objects. This information is not yet used by qapidoc, which continues to render documentation exactly as it has. This information will be used by the new qapidoc (the "transmogrifier"), to be introduced later. The new generator verifies that all of the objects that should be rendered *are* by failing if any cross-references are missing, verifying everything is in place. Signed-off-by: John Snow --- scripts/qapi/schema.py | 40 1 file changed, 40 insertions(+) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index e15e64ea8cb..6025b4e9354 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -131,6 +131,7 @@ def __init__( self.doc = doc self._ifcond = ifcond or QAPISchemaIfCond() self.features = features or [] +self.doc_visible = False def __repr__(self) -> str: return "<%s:%s at 0x%x>" % (type(self).__name__, self.name, @@ -146,6 +147,10 @@ def check(self, schema: QAPISchema) -> None: for f in self.features: f.check_clash(self.info, seen) +def mark_visible(self, mark_self: bool = True) -> None: +if mark_self: +self.doc_visible = True + def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None: super().connect_doc(doc) doc = doc or self.doc @@ -483,6 +488,10 @@ def check(self, schema: QAPISchema) -> None: self.info.defn_meta if self.info else None) assert not isinstance(self.element_type, QAPISchemaArrayType) +def mark_visible(self, mark_self: bool = True) -> None: +super().mark_visible(mark_self) +self.element_type.mark_visible() + def set_module(self, schema: QAPISchema) -> None: self._set_module(schema, self.element_type.info) @@ -607,6 +616,17 @@ def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None: for m in self.local_members: m.connect_doc(doc) +def mark_visible(self, mark_self: bool = True) -> None: +# Mark this object and its members as visible in the user-facing docs. +if self.doc_visible: +return + +super().mark_visible(mark_self) +for m in self.members: +m.type.mark_visible() +for var in self.branches or []: +var.type.mark_visible(False) + def is_implicit(self) -> bool: # See QAPISchema._make_implicit_object_type(), as well as # _def_predefineds() @@ -698,6 +718,11 @@ def check(self, schema: QAPISchema) -> None: % (v.describe(self.info), types_seen[qt])) types_seen[qt] = v.name +def mark_visible(self, mark_self: bool = True) -> None: +super().mark_visible(mark_self) +for var in self.alternatives: +var.type.mark_visible() + def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None: super().connect_doc(doc) doc = doc or self.doc @@ -1056,6 +1081,13 @@ def check(self, schema: QAPISchema) -> None: "command's 'returns' cannot take %s" % self.ret_type.describe()) +def mark_visible(self, mark_self: bool = True) -> None: +super().mark_visible(mark_self) +if self.arg_type: +self.arg_type.mark_visible(False) +if self.ret_type: +self.ret_type.mark_visible() + def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None: super().connect_doc(doc) doc = doc or self.doc @@ -1112,6 +1144,11 @@ def check(self, schema: QAPISchema) -> None: self.info, "conditional event arguments require 'boxed': true") +def mark_visible(self, mark_self: bool = True) -> None: +super().mark_visible(mark_self) +if self.arg_type: +self.arg_type.mark_visible(False) + def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None: super().connect_doc(doc) doc = doc or self.doc @@ -1488,6 +1525,9 @@ def check(self) -> None:
[PATCH 18/20] qapi: ensure all errors sections are uniformly typset
Transactions have the only instance of an Errors section that isn't a rST list; turn it into one. Signed-off-by: John Snow --- qapi/transaction.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qapi/transaction.json b/qapi/transaction.json index 5749c133d4a..07afc269d54 100644 --- a/qapi/transaction.json +++ b/qapi/transaction.json @@ -235,7 +235,7 @@ # additional detail. # # Errors: -# Any errors from commands in the transaction +# - Any errors from commands in the transaction # # Note: The transaction aborts on the first failure. Therefore, there # will be information on only one failed operation returned in an -- 2.44.0
[PATCH 09/20] qapi/parser: add undocumented stub members to all_sections
This helps simplify the doc generator if it doesn't have to check for
undocumented members.
Signed-off-by: John Snow
---
scripts/qapi/parser.py | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index b1794f71e12..3cd8e7ee295 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -740,8 +740,24 @@ def connect_member(self, member: 'QAPISchemaMember') ->
None:
raise QAPISemError(member.info,
"%s '%s' lacks documentation"
% (member.role, member.name))
-self.args[member.name] = QAPIDoc.ArgSection(
-self.info, '@' + member.name, 'member')
+
+# Insert stub documentation section for missing member docs.
+section = QAPIDoc.ArgSection(
+self.info, f"@{member.name}", "member")
+self.args[member.name] = section
+
+# Determine where to insert stub doc.
+index = 0
+for i, sect in enumerate(self.all_sections):
+# insert after these:
+if sect.kind in ('intro-paragraph', 'member'):
+index = i + 1
+# but before these:
+elif sect.kind in ('tagged', 'feature', 'outro-paragraph'):
+index = i
+break
+self.all_sections.insert(index, section)
+
self.args[member.name].connect(member)
def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
--
2.44.0
[PATCH 05/20] qapi/parser: adjust info location for doc body section
Instead of using the info object for the doc block as a whole, update the info pointer for each call to ensure_untagged_section when the existing section is otherwise empty. This way, Sphinx error information will match precisely to where the text actually starts. Signed-off-by: John Snow --- scripts/qapi/parser.py | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 8cdd5334ec6..41b9319e5cb 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -662,8 +662,13 @@ def end(self) -> None: def ensure_untagged_section(self, info: QAPISourceInfo) -> None: if self.all_sections and not self.all_sections[-1].tag: -# extend current section -self.all_sections[-1].text += '\n' +section = self.all_sections[-1] +# Section is empty so far; update info to start *here*. +if not section.text: +section.info = info +else: +# extend current section +self.all_sections[-1].text += '\n' return # start new section section = self.Section(info) -- 2.44.0
[PATCH 19/20] qapi: convert "Note" sections to plain rST
We do not need a dedicated section for notes. By eliminating a specially
parsed section, these notes can be treated as normal rST paragraphs in
the new QMP reference manual, and can be placed and styled much more
flexibly.
Update the QAPI parser to now prohibit special "Note" sections while
suggesting a new syntax.
The exact formatting to use is a matter of taste, but a good candidate
is simply:
.. note:: lorem ipsum ...
but there are other choices, too. The Sphinx readthedocs theme offers
theming for the following forms (capitalization unimportant); all are
adorned with a (!) symbol in the title bar for rendered HTML docs.
These are rendered in orange:
.. Attention:: ...
.. Caution:: ...
.. WARNING:: ...
These are rendered in red:
.. DANGER:: ...
.. Error:: ...
These are rendered in green:
.. Hint:: ...
.. Important:: ...
.. Tip:: ...
These are rendered in blue:
.. Note:: ...
.. admonition:: custom title
admonition body text
This patch uses ".. notes::" almost everywhere, with just two "caution"
directives. ".. admonition:: notes" is used in a few places where we had
an ordered list of multiple notes.
Signed-off-by: John Snow
---
qapi/block-core.json | 30 +++
qapi/block.json | 2 +-
qapi/char.json| 12 +--
qapi/control.json | 15 ++--
qapi/dump.json| 2 +-
qapi/introspect.json | 6 +-
qapi/machine-target.json | 26 +++---
qapi/machine.json | 47 +-
qapi/migration.json | 12 +--
qapi/misc.json| 88 +--
qapi/net.json | 6 +-
qapi/pci.json | 7 +-
qapi/qdev.json| 30 +++
qapi/qom.json | 19 ++--
qapi/rocker.json | 16 ++--
qapi/run-state.json | 18 ++--
qapi/sockets.json | 10 +--
qapi/stats.json | 22 ++---
qapi/transaction.json | 8 +-
qapi/ui.json | 29 +++---
qapi/virtio.json | 12 +--
qga/qapi-schema.json | 48 +-
scripts/qapi/parser.py| 9 ++
tests/qapi-schema/doc-empty-section.err | 2 +-
tests/qapi-schema/doc-empty-section.json | 2 +-
tests/qapi-schema/doc-good.json | 6 +-
tests/qapi-schema/doc-good.out| 10 ++-
tests/qapi-schema/doc-good.txt| 14 ++-
.../qapi-schema/doc-interleaved-section.json | 2 +-
29 files changed, 258 insertions(+), 252 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 64fe5240cc9..530af40404d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1510,7 +1510,7 @@
# @mode: whether and how QEMU should create a new image, default is
# 'absolute-paths'.
#
-# Note: Either @device or @node-name must be set but not both.
+# .. note:: Either @device or @node-name must be set but not both.
#
##
{ 'struct': 'BlockdevSnapshotSync',
@@ -1616,9 +1616,9 @@
#
# @unstable: Member @x-perf is experimental.
#
-# Note: @on-source-error and @on-target-error only affect background
-# I/O. If an error occurs during a guest write request, the
-# device's rerror/werror actions will be used.
+# .. note:: @on-source-error and @on-target-error only affect background
+#I/O. If an error occurs during a guest write request, the device's
+#rerror/werror actions will be used.
#
# Since: 4.2
##
@@ -5534,8 +5534,8 @@
# after this event and must be repaired (Since 2.2; before, every
# BLOCK_IMAGE_CORRUPTED event was fatal)
#
-# Note: If action is "stop", a STOP event will eventually follow the
-# BLOCK_IO_ERROR event.
+# .. note:: If action is "stop", a STOP event will eventually follow the
+#BLOCK_IO_ERROR event.
#
# Example:
#
@@ -5581,8 +5581,8 @@
# field is a debugging aid for humans, it should not be parsed by
# applications) (since: 2.2)
#
-# Note: If action is "stop", a STOP event will eventually follow the
-# BLOCK_IO_ERROR event
+# .. note:: If action is "stop", a STOP event will eventually follow the
+#BLOCK_IO_ERROR event.
#
# Since: 0.13
#
@@ -5720,8 +5720,8 @@
#
# @speed: rate limit, bytes per second
#
-# Note: The "ready to complete" status is always reset by a
-# @BLOCK_JOB_ERROR event
+# .. note:: The "ready to complete" status is always reset by a
+#@BLOCK_JOB_ERROR event.
#
# Since: 1.3
#
@@ -5974,7 +5974,7 @@
#
# @sectors-count: failed read operation sector count
#
-# Note: This event is rate-limited.
+# .. note:: This event is rate-limited.
#
# Since: 2.0
#
@@ -6005,7 +6005,7 @@
#
#
[PATCH 16/20] qapi: rewrite StatsFilter comment
Rewrite the StatsFilter intro paragraph to be more meaningful to end-users when it is inlined in generated documentation. Signed-off-by: John Snow --- qapi/stats.json | 8 ++-- 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/qapi/stats.json b/qapi/stats.json index 578b52c7ef7..c4a9f3ff70e 100644 --- a/qapi/stats.json +++ b/qapi/stats.json @@ -112,10 +112,6 @@ ## # @StatsFilter: # -# The arguments to the query-stats command; specifies a target for -# which to request statistics and optionally the required subset of -# information for that target. -# # @target: the kind of objects to query. Note that each possible # target may enable additional filtering options # @@ -183,8 +179,8 @@ # Return runtime-collected statistics for objects such as the VM or # its vCPUs. # -# The arguments are a StatsFilter and specify the provider and objects -# to return statistics about. +# The arguments specify a target for which to request statistics and +# optionally the required subset of information for that target. # # Returns: a list of StatsResult, one for each provider and object # (e.g., for each vCPU). -- 2.44.0
[PATCH 06/20] qapi/parser: fix comment parsing immediately following a doc block
If a comment immediately follows a doc block, the parser doesn't ignore that token appropriately. Fix that. Signed-off-by: John Snow --- scripts/qapi/parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 41b9319e5cb..161768b8b96 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -587,7 +587,7 @@ def get_doc(self) -> 'QAPIDoc': line = self.get_doc_line() first = False -self.accept(False) +self.accept() doc.end() return doc -- 2.44.0
[PATCH 07/20] qapi/parser: add semantic 'kind' parameter to QAPIDoc.Section
When iterating all_sections, this is helpful to be able to distinguish
"members" from "features"; the only other way to do so is to
cross-reference these sections against QAPIDoc.args or QAPIDoc.features,
but if the desired end goal for QAPIDoc is to remove everything except
all_sections, we need *something* accessible to distinguish them.
To keep types simple, add this semantic parameter to the base Section
and not just ArgSection; we can use this to filter out paragraphs and
tagged sections, too.
Signed-off-by: John Snow
---
scripts/qapi/parser.py | 25 -
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 161768b8b96..cf4cbca1c1f 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -613,21 +613,27 @@ class QAPIDoc:
class Section:
# pylint: disable=too-few-public-methods
-def __init__(self, info: QAPISourceInfo,
- tag: Optional[str] = None):
+def __init__(
+self,
+info: QAPISourceInfo,
+tag: Optional[str] = None,
+kind: str = 'paragraph',
+):
# section source info, i.e. where it begins
self.info = info
# section tag, if any ('Returns', '@name', ...)
self.tag = tag
# section text without tag
self.text = ''
+# section type - {paragraph, feature, member, tagged}
+self.kind = kind
def append_line(self, line: str) -> None:
self.text += line + '\n'
class ArgSection(Section):
-def __init__(self, info: QAPISourceInfo, tag: str):
-super().__init__(info, tag)
+def __init__(self, info: QAPISourceInfo, tag: str, kind: str):
+super().__init__(info, tag, kind)
self.member: Optional['QAPISchemaMember'] = None
def connect(self, member: 'QAPISchemaMember') -> None:
@@ -676,7 +682,7 @@ def ensure_untagged_section(self, info: QAPISourceInfo) ->
None:
self.all_sections.append(section)
def new_tagged_section(self, info: QAPISourceInfo, tag: str) -> None:
-section = self.Section(info, tag)
+section = self.Section(info, tag, "tagged")
if tag == 'Returns':
if self.returns:
raise QAPISemError(
@@ -696,20 +702,21 @@ def new_tagged_section(self, info: QAPISourceInfo, tag:
str) -> None:
self.all_sections.append(section)
def _new_description(self, info: QAPISourceInfo, name: str,
+ kind: str,
desc: Dict[str, ArgSection]) -> None:
if not name:
raise QAPISemError(info, "invalid parameter name")
if name in desc:
raise QAPISemError(info, "'%s' parameter name duplicated" % name)
-section = self.ArgSection(info, '@' + name)
+section = self.ArgSection(info, '@' + name, kind)
self.all_sections.append(section)
desc[name] = section
def new_argument(self, info: QAPISourceInfo, name: str) -> None:
-self._new_description(info, name, self.args)
+self._new_description(info, name, 'member', self.args)
def new_feature(self, info: QAPISourceInfo, name: str) -> None:
-self._new_description(info, name, self.features)
+self._new_description(info, name, 'feature', self.features)
def append_line(self, line: str) -> None:
self.all_sections[-1].append_line(line)
@@ -722,7 +729,7 @@ def connect_member(self, member: 'QAPISchemaMember') ->
None:
"%s '%s' lacks documentation"
% (member.role, member.name))
self.args[member.name] = QAPIDoc.ArgSection(
-self.info, '@' + member.name)
+self.info, '@' + member.name, 'member')
self.args[member.name].connect(member)
def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
--
2.44.0
[PATCH 01/20] [DO-NOT-MERGE]: Add some ad-hoc linting helpers.
These aren't ready for upstream inclusion, because they do not properly manage version dependencies, execution environment and so on. These are just the tools I use in my Own Special Environment :tm: for testing and debugging. They've been tested only on Fedora 38 for right now, which means: Python 3.11.4-1.fc38 pylint 2.17.4-2.fc38 mypy 1.4.0-1.fc38 isort 5.12.0-1.fc38 flake8 5.0.3-2.fc38 "Soon" :tm: I'll move the qapi generator code under the python/ directory to take advantage of the more robust linting infrastructure there, but that's going to happen after this series. Sorry about that! Signed-off-by: John Snow --- scripts/qapi-lint.sh | 51 +++ scripts/qapi/Makefile | 5 + 2 files changed, 56 insertions(+) create mode 100755 scripts/qapi-lint.sh create mode 100644 scripts/qapi/Makefile diff --git a/scripts/qapi-lint.sh b/scripts/qapi-lint.sh new file mode 100755 index 000..528b31627eb --- /dev/null +++ b/scripts/qapi-lint.sh @@ -0,0 +1,51 @@ +#!/usr/bin/env bash +set -e + +if [[ -f qapi/.flake8 ]]; then +echo "flake8 --config=qapi/.flake8 qapi/" +flake8 --config=qapi/.flake8 qapi/ +fi +if [[ -f qapi/pylintrc ]]; then +echo "pylint --rcfile=qapi/pylintrc qapi/" +pylint --rcfile=qapi/pylintrc qapi/ +fi +if [[ -f qapi/mypy.ini ]]; then +echo "mypy --config-file=qapi/mypy.ini qapi/" +mypy --config-file=qapi/mypy.ini qapi/ +fi + +if [[ -f qapi/.isort.cfg ]]; then +pushd qapi +echo "isort -c ." +isort -c . +popd +fi + +if [[ -f ../docs/sphinx/qapi-domain.py ]]; then +pushd ../docs/sphinx + +echo "mypy --strict qapi-domain.py" +mypy --strict qapi-domain.py +echo "flake8 qapi-domain.py --max-line-length=99" +flake8 qapi-domain.py --max-line-length=99 +echo "isort qapi-domain.py" +isort qapi-domain.py +echo "black --check qapi-domain.py" +black --check qapi-domain.py + +echo "flake8 qapidoc.py --max-line-length=99" +flake8 qapidoc.py --max-line-length=99 +echo "isort qapidoc.py" +isort qapidoc.py +echo "black --check qapidoc.py" +black --check qapidoc.py + +popd +fi + +pushd ../build +make -j13 +make check-qapi-schema +make docs +make sphinxdocs +popd diff --git a/scripts/qapi/Makefile b/scripts/qapi/Makefile new file mode 100644 index 000..314e8a5505e --- /dev/null +++ b/scripts/qapi/Makefile @@ -0,0 +1,5 @@ +check: + isort -c . + flake8 . + cd .. && pylint --rcfile=qapi/pylintrc qapi + cd .. && mypy -p qapi --config-file=qapi/mypy.ini -- 2.44.0
[PATCH 04/20] qapi/parser: preserve indentation in QAPIDoc sections
Prior to this patch, a section like this:
@name: lorem ipsum
dolor sit amet
consectetur adipiscing elit
would be parsed as:
"lorem ipsum\ndolor sit amet\n consectetur adipiscing elit"
We want to preserve the indentation for even the first body line so that
the entire block can be parsed directly as rST. This patch would now
parse that segment as:
"lorem ipsum\n dolor sit amet\n consectetur adipiscing elit"
This understandably breaks qapidoc.py; so a new function is added there
to re-dedent the text. Once the new generator is merged, this function
will not be needed any longer and can be dropped.
(I verified this patch changes absolutely nothing by comparing the
md5sums of the QMP ref html pages both before and after the change, so
it's certified inert. QAPI test output has been updated to reflect the
new strategy of preserving indents for rST.)
Signed-off-by: John Snow
---
docs/sphinx/qapidoc.py | 36 +-
scripts/qapi/parser.py | 8 ++--
tests/qapi-schema/doc-good.out | 32 +++---
3 files changed, 53 insertions(+), 23 deletions(-)
diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 1655682d4c7..2e3ffcbafb7 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -26,6 +26,7 @@
import os
import re
+import textwrap
from docutils import nodes
from docutils.parsers.rst import Directive, directives
@@ -51,6 +52,28 @@
__version__ = "1.0"
+def dedent(text: str) -> str:
+# Temporary: In service of the new QAPI domain, the QAPI doc parser
+# now preserves indents in args/members/features text. QAPIDoc does
+# not handle this well, so undo that change here.
+
+# QAPIDoc is being rewritten and will be replaced soon,
+# but this function is here in the interim as transition glue.
+
+lines = text.splitlines(True)
+if len(lines) > 1:
+if re.match(r"\s+", lines[0]):
+# First line is indented; description started on
+# the line after the name. dedent the whole block.
+return textwrap.dedent(text)
+else:
+# Descr started on same line. Dedent line 2+.
+return lines[0] + textwrap.dedent("".join(lines[1:]))
+else:
+# Descr was a single line; dedent entire line.
+return textwrap.dedent(text)
+
+
# fmt: off
# Function borrowed from pydash, which is under the MIT license
def intersperse(iterable, separator):
@@ -169,7 +192,7 @@ def _nodes_for_members(self, doc, what, base=None,
branches=None):
term = self._nodes_for_one_member(section.member)
# TODO drop fallbacks when undocumented members are outlawed
if section.text:
-defn = section.text
+defn = dedent(section.text)
else:
defn = [nodes.Text('Not documented')]
@@ -207,7 +230,7 @@ def _nodes_for_enum_values(self, doc):
termtext.extend(self._nodes_for_ifcond(section.member.ifcond))
# TODO drop fallbacks when undocumented members are outlawed
if section.text:
-defn = section.text
+defn = dedent(section.text)
else:
defn = [nodes.Text('Not documented')]
@@ -242,7 +265,7 @@ def _nodes_for_features(self, doc):
dlnode = nodes.definition_list()
for section in doc.features.values():
dlnode += self._make_dlitem(
-[nodes.literal('', section.member.name)], section.text)
+[nodes.literal('', section.member.name)], dedent(section.text))
seen_item = True
if not seen_item:
@@ -265,9 +288,12 @@ def _nodes_for_sections(self, doc):
continue
snode = self._make_section(section.tag)
if section.tag and section.tag.startswith('Example'):
-snode += self._nodes_for_example(section.text)
+snode += self._nodes_for_example(dedent(section.text))
else:
-self._parse_text_into_node(section.text, snode)
+self._parse_text_into_node(
+dedent(section.text) if section.tag else section.text,
+snode,
+)
nodelist.append(snode)
return nodelist
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 7b13a583ac1..8cdd5334ec6 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -448,7 +448,10 @@ def get_doc_indented(self, doc: 'QAPIDoc') ->
Optional[str]:
indent = must_match(r'\s*', line).end()
if not indent:
return line
-doc.append_line(line[indent:])
+
+# Preserve the indent, it's needed for rST formatting.
+doc.append_line(line)
+
prev_line_blank = False
while True:
self.accept(False)
@@ -465,7 +468,8 @@ def get_doc_indented(self, doc: 'QAPIDoc') ->
[PATCH 02/20] qapi: linter fixups
Fix minor irritants to pylint/flake8 et al.
(Yes, these need to be guarded by the Python tests. That's a work in
progress, a series that's quite likely to follow once I finish this
Sphinx project. Please pardon the temporary irritation.)
Signed-off-by: John Snow
---
scripts/qapi/introspect.py | 8
scripts/qapi/schema.py | 6 +++---
scripts/qapi/visit.py | 5 +++--
3 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 86c075a6ad2..ac14b20f308 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -27,8 +27,8 @@
from .schema import (
QAPISchema,
QAPISchemaAlternatives,
-QAPISchemaBranches,
QAPISchemaArrayType,
+QAPISchemaBranches,
QAPISchemaBuiltinType,
QAPISchemaEntity,
QAPISchemaEnumMember,
@@ -233,9 +233,9 @@ def _use_type(self, typ: QAPISchemaType) -> str:
typ = type_int
elif (isinstance(typ, QAPISchemaArrayType) and
typ.element_type.json_type() == 'int'):
-type_intList = self._schema.lookup_type('intList')
-assert type_intList
-typ = type_intList
+type_intlist = self._schema.lookup_type('intList')
+assert type_intlist
+typ = type_intlist
# Add type to work queue if new
if typ not in self._used_types:
self._used_types.append(typ)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 721c470d2b8..d65c35f6ee6 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -730,6 +730,7 @@ def set_defined_in(self, name: str) -> None:
for v in self.variants:
v.set_defined_in(name)
+# pylint: disable=unused-argument
def check(
self, schema: QAPISchema, seen: Dict[str, QAPISchemaMember]
) -> None:
@@ -1166,7 +1167,7 @@ def _def_definition(self, defn: QAPISchemaDefinition) ->
None:
defn.info, "%s is already defined" % other_defn.describe())
self._entity_dict[defn.name] = defn
-def lookup_entity(self,name: str) -> Optional[QAPISchemaEntity]:
+def lookup_entity(self, name: str) -> Optional[QAPISchemaEntity]:
return self._entity_dict.get(name)
def lookup_type(self, name: str) -> Optional[QAPISchemaType]:
@@ -1302,11 +1303,10 @@ def _make_implicit_object_type(
name = 'q_obj_%s-%s' % (name, role)
typ = self.lookup_entity(name)
if typ:
-assert(isinstance(typ, QAPISchemaObjectType))
+assert isinstance(typ, QAPISchemaObjectType)
# The implicit object type has multiple users. This can
# only be a duplicate definition, which will be flagged
# later.
-pass
else:
self._def_definition(QAPISchemaObjectType(
name, info, None, ifcond, None, None, members, None))
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index e766acaac92..12f92e429f6 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -280,8 +280,9 @@ def gen_visit_alternate(name: str,
abort();
default:
assert(visit_is_input(v));
-error_setg(errp, "Invalid parameter type for '%%s', expected:
%(name)s",
- name ? name : "null");
+error_setg(errp,
+ "Invalid parameter type for '%%s', expected: %(name)s",
+ name ? name : "null");
/* Avoid passing invalid *obj to qapi_free_%(c_name)s() */
g_free(*obj);
*obj = NULL;
--
2.44.0
[PATCH 20/20] qapi: convert "Example" sections to rST
Eliminate the "Example" sections in QAPI doc blocks, converting them
into QMP example code blocks. This is generally done by converting
"Example:" or "Examples:" lines into ".. code-block:: QMP" lines.
This patch does also allow for the use of the rST syntax "Example::" by
exempting double-colon syntax from the QAPI doc parser, but that form is
not used by this conversion patch. The phrase "Example" here is not
special, it is the double-colon syntax that transforms the following
block into a code-block. By default, *this* form does not apply QMP
highlighting.
This patch has several benefits:
1. Example sections can now be written more arbitrarily, mixing
explanatory paragraphs and code blocks however desired.
2. Example sections can now use fully arbitrary rST.
3. All code blocks are now lexed and validated as QMP; increasing
usability of the docs and ensuring validity of example snippets.
4. Each code-block can be captioned independently without bypassing the
QMP lexer/validator.
For any sections with more than one example, examples are split up into
multiple code-block regions. If annotations are present, those
annotations are converted into code-block captions instead, e.g.
```
Examples:
1. Lorem Ipsum
-> { "foo": "bar" }
```
Is rewritten as:
```
.. code-block:: QMP
:caption: Example: Lorem Ipsum
-> { "foo": "bar" }
```
This process was only semi-automated:
1. Replace "Examples?:" sections with sed:
sed -i 's|# Example:|# .. code-block:: QMP|' *.json
sed -i 's|# Examples:|# .. code-block:: QMP|' *.json
2. Identify sections that no longer parse successfully by attempting the
doc build, convert annotations into captions manually.
(Tedious, oh well.)
3. Add captions where still needed:
sed -zi 's|# .. code-block:: QMP\n#\n|# .. code-block:: QMP\n#:caption:
Example\n#\n|g' *.json
Not fully ideal, but hopefully not something that has to be done very
often. (Or ever again.)
Signed-off-by: John Snow
---
qapi/acpi.json | 6 +-
qapi/block-core.json| 120 --
qapi/block.json | 60 +++--
qapi/char.json | 36 ++--
qapi/control.json | 16 ++--
qapi/dump.json | 12 ++-
qapi/machine-target.json| 3 +-
qapi/machine.json | 79 ++---
qapi/migration.json | 145 +++-
qapi/misc-target.json | 33 +---
qapi/misc.json | 48 +++
qapi/net.json | 30 +--
qapi/pci.json | 6 +-
qapi/qapi-schema.json | 6 +-
qapi/qdev.json | 15 +++-
qapi/qom.json | 20 +++--
qapi/replay.json| 12 ++-
qapi/rocker.json| 12 ++-
qapi/run-state.json | 45 ++
qapi/tpm.json | 9 +-
qapi/trace.json | 6 +-
qapi/transaction.json | 3 +-
qapi/ui.json| 62 +-
qapi/virtio.json| 38 +
qapi/yank.json | 6 +-
scripts/qapi/parser.py | 15 +++-
tests/qapi-schema/doc-good.json | 12 +--
tests/qapi-schema/doc-good.out | 17 ++--
tests/qapi-schema/doc-good.txt | 17 +---
29 files changed, 574 insertions(+), 315 deletions(-)
diff --git a/qapi/acpi.json b/qapi/acpi.json
index aa4dbe57943..3da01f1b7fc 100644
--- a/qapi/acpi.json
+++ b/qapi/acpi.json
@@ -111,7 +111,8 @@
#
# Since: 2.1
#
-# Example:
+# .. code-block:: QMP
+#:caption: Example
#
# -> { "execute": "query-acpi-ospm-status" }
# <- { "return": [ { "device": "d1", "slot": "0", "slot-type": "DIMM",
"source": 1, "status": 0},
@@ -131,7 +132,8 @@
#
# Since: 2.1
#
-# Example:
+# .. code-block:: QMP
+#:caption: Example
#
# <- { "event": "ACPI_DEVICE_OST",
# "data": { "info": { "device": "d1", "slot": "0",
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 530af40404d..bb0447207df 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -763,7 +763,8 @@
#
# Since: 0.14
#
-# Example:
+# .. code-block:: QMP
+#:caption: Example
#
# -> { "execute": "query-block" }
# <- {
@@ -1167,7 +1168,8 @@
#
# Since: 0.14
#
-# Example:
+# .. code-block:: QMP
+#:caption: Example
#
# -> { "execute": "query-blockstats" }
# <- {
@@ -1460,7 +1462,8 @@
#
# Since: 0.14
#
-# Example:
+# .. code-block:: QMP
+#:caption: Example
#
# -> { "execute": "block_resize",
# "arguments": { "device": "scratch", "size": 1073741824 } }
@@ -1678,7 +1681,8 @@
#
# Since: 0.14
#
-# Example:
+# .. code-block:: QMP
+#:caption: Example
#
# -> { "execute": "blockdev-snapshot-sync",
# "arguments": { "device": "ide-hd0",
@@ -1711,7 +1715,8 @@
#
# Since: 2.5
#
-# Example:
+# .. code-block:: QMP
+#:caption: Example
#
# -> {
[PATCH 14/20] qapi: fix non-compliant JSON examples
If we parse all examples as QMP, we need them to conform to a standard
so that they render correctly. Once the QMP lexer is active for
examples, these will produce warning messages and fail the build.
The QMP lexer still supports elisions, but they must be represented as
the value "...", so two examples have been adjusted to support that
format here.
Signed-off-by: John Snow
---
qapi/control.json | 3 ++-
qapi/machine.json | 2 +-
qapi/migration.json | 2 +-
qapi/misc.json | 3 ++-
qapi/net.json | 6 +++---
qapi/rocker.json| 2 +-
qapi/ui.json| 2 +-
7 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/qapi/control.json b/qapi/control.json
index 6bdbf077c2e..10c906fa0e7 100644
--- a/qapi/control.json
+++ b/qapi/control.json
@@ -145,7 +145,8 @@
# },
# {
#"name":"system_powerdown"
-# }
+# },
+# ...
# ]
#}
#
diff --git a/qapi/machine.json b/qapi/machine.json
index bce6e1bbc41..64a77557571 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1057,7 +1057,7 @@
#"vcpus-count": 1 },
# { "props": { "core-id": 0 }, "type": "POWER8-spapr-cpu-core",
#"vcpus-count": 1, "qom-path": "/machine/unattached/device[0]"}
-#]}'
+#]}
#
# For pc machine type started with -smp 1,maxcpus=2:
#
diff --git a/qapi/migration.json b/qapi/migration.json
index a351fd37143..89047d46c7c 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -2078,7 +2078,7 @@
# Example:
#
# -> {"execute": "calc-dirty-rate", "arguments": {"calc-time": 1,
-# 'sample-pages': 512} }
+# "sample-pages": 512} }
# <- { "return": {} }
#
# Measure dirty rate using dirty bitmap for 500 milliseconds:
diff --git a/qapi/misc.json b/qapi/misc.json
index ec30e5c570a..4b41e15dcd4 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -287,7 +287,8 @@
#
# Example:
#
-# -> { "execute": "get-win32-socket", "arguments": { "info": "abcd123..",
fdname": "skclient" } }
+# -> { "execute": "get-win32-socket",
+# "arguments": { "info": "abcd123..", "fdname": "skclient" } }
# <- { "return": {} }
##
{ 'command': 'get-win32-socket', 'data': {'info': 'str', 'fdname': 'str'},
'if': 'CONFIG_WIN32' }
diff --git a/qapi/net.json b/qapi/net.json
index 0f5a259475e..c19df435a53 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -1003,9 +1003,9 @@
#
# Example:
#
-# <- { 'event': 'NETDEV_STREAM_DISCONNECTED',
-# 'data': {'netdev-id': 'netdev0'},
-# 'timestamp': {'seconds': 1663330937, 'microseconds': 526695} }
+# <- { "event": "NETDEV_STREAM_DISCONNECTED",
+# "data": {"netdev-id": "netdev0"},
+# "timestamp": {"seconds": 1663330937, "microseconds": 526695} }
##
{ 'event': 'NETDEV_STREAM_DISCONNECTED',
'data': { 'netdev-id': 'str' } }
diff --git a/qapi/rocker.json b/qapi/rocker.json
index 5635cf174fd..f5225eb62cc 100644
--- a/qapi/rocker.json
+++ b/qapi/rocker.json
@@ -250,7 +250,7 @@
# "action": {"goto-tbl": 10},
# "mask": {"in-pport": 4294901760}
# },
-# {...more...},
+# {...},
#]}
##
{ 'command': 'query-rocker-of-dpa-flows',
diff --git a/qapi/ui.json b/qapi/ui.json
index f610bce118a..c12f5292571 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -361,7 +361,7 @@
#"channel-id": 0,
#"tls": false
# },
-# [ ... more channels follow ... ]
+# ...
# ]
# }
#}
--
2.44.0
[PATCH 15/20] qapi: remove developer factoring comments from QAPI doc blocks
This is part of a project to overhaul the QMP reference manual. One goal
of this overhaul is to "inline" inherited argument sections into command
reference sections. A consequence of this design decision is that
inherited doc block sections need to be merged with the inheritor's
sections.
When documentation is written for types whose primary purpose is to be
inherited by other types, we need to know how to merge those paragraphs
into the descendent. Much of the time, these paragraphs aren't actually
useful to end users.
Either remove information that's of little to no use to *either*
audience, and convert what's left to garden-variety comments to prevent
it from showing up in rendered documentation.
Signed-off-by: John Snow
---
qapi/audio.json| 5 ++---
qapi/block-core.json | 47 ++
qapi/block-export.json | 10 -
qapi/char.json | 5 ++---
qapi/crypto.json | 33 -
qapi/machine.json | 10 -
qapi/net.json | 7 ++-
qapi/qom.json | 30 +++
qapi/ui.json | 14 -
9 files changed, 59 insertions(+), 102 deletions(-)
diff --git a/qapi/audio.json b/qapi/audio.json
index 519697c0cd8..ee09cd231b6 100644
--- a/qapi/audio.json
+++ b/qapi/audio.json
@@ -10,11 +10,10 @@
# = Audio
##
-##
-# @AudiodevPerDirectionOptions:
-#
# General audio backend options that are used for both playback and
# recording.
+##
+# @AudiodevPerDirectionOptions:
#
# @mixing-engine: use QEMU's mixing engine to mix all streams inside
# QEMU and convert audio formats when not supported by the
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 746d1694c25..64fe5240cc9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -267,10 +267,9 @@
'file': 'ImageInfoSpecificFileWrapper'
} }
-##
-# @BlockNodeInfo:
-#
# Information about a QEMU image file
+##
+# @BlockNodeInfo:
#
# @filename: name of the image file
#
@@ -1494,8 +1493,6 @@
##
# @BlockdevSnapshotSync:
#
-# Either @device or @node-name must be set but not both.
-#
# @device: the name of the device to take a snapshot of.
#
# @node-name: graph node name to generate the snapshot from (Since
@@ -1512,6 +1509,9 @@
#
# @mode: whether and how QEMU should create a new image, default is
# 'absolute-paths'.
+#
+# Note: Either @device or @node-name must be set but not both.
+#
##
{ 'struct': 'BlockdevSnapshotSync',
'data': { '*device': 'str', '*node-name': 'str',
@@ -2139,10 +2139,9 @@
'data': 'DriveMirror',
'allow-preconfig': true }
-##
-# @DriveMirror:
-#
# A set of parameters describing drive mirror setup.
+##
+# @DriveMirror:
#
# @job-id: identifier for the newly-created block job. If omitted,
# the device name will be used. (Since 2.7)
@@ -2553,10 +2552,9 @@
'*auto-finalize': 'bool', '*auto-dismiss': 'bool' },
'allow-preconfig': true }
-##
-# @BlockIOThrottle:
-#
# A set of parameters describing block throttling.
+##
+# @BlockIOThrottle:
#
# @device: Block device name
#
@@ -3073,10 +3071,9 @@
{ 'struct': 'BlockJobChangeOptionsMirror',
'data': { 'copy-mode' : 'MirrorCopyMode' } }
-##
-# @BlockJobChangeOptions:
-#
# Block job options that can be changed after job creation.
+##
+# @BlockJobChangeOptions:
#
# @id: The job identifier
#
@@ -3332,11 +3329,10 @@
'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool',
'*label': 'str', '*rw': 'bool' } }
-##
-# @BlockdevOptionsGenericFormat:
-#
# Driver specific block device options for image format that have no
# option besides their data source.
+##
+# @BlockdevOptionsGenericFormat:
#
# @file: reference to or definition of the data source block device
#
@@ -3363,11 +3359,10 @@
'data': { '*key-secret': 'str',
'*header': 'BlockdevRef'} }
-##
-# @BlockdevOptionsGenericCOWFormat:
-#
# Driver specific block device options for image format that have no
# option besides their data source and an optional backing file.
+##
+# @BlockdevOptionsGenericCOWFormat:
#
# @backing: reference to or definition of the backing file block
# device, null disables the backing file entirely. Defaults to
@@ -4385,11 +4380,10 @@
'*page-cache-size': 'int',
'*debug': 'int' } }
-##
-# @BlockdevOptionsCurlBase:
-#
# Driver specific block device options shared by all protocols
# supported by the curl backend.
+##
+# @BlockdevOptionsCurlBase:
#
# @url: URL of the image file
#
@@ -4645,11 +4639,10 @@
'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
'*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32' } }
-##
-# @BlockdevOptions:
-#
# Options for creating a block device. Many options are available for
-# all block devices, independent of the block driver:
+# all block devices, independent of the block driver.
+##
+# @BlockdevOptions:
#
# @driver: block driver name
[PATCH 17/20] qapi: rewrite BlockExportOptions doc block
Rephrase this paragraph so that it can apply to any commands that
inherit from this object.
Signed-off-by: John Snow
---
qapi/block-export.json | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index dc328097a94..550763a9f6a 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -342,9 +342,6 @@
##
# @BlockExportOptions:
#
-# Describes a block export, i.e. how single node should be exported on
-# an external interface.
-#
# @type: Block export type
#
# @id: A unique identifier for the block export (across all export
@@ -396,6 +393,9 @@
#
# Creates a new block export.
#
+# Arguments describe a block export, i.e. how single node should be
+# exported on an external interface.
+#
# Since: 5.2
##
{ 'command': 'block-export-add',
--
2.44.0
[PATCH 03/20] docs/qapidoc: delint a tiny portion of the module
In the coming patches, it's helpful to have a linting baseline. However, there's no need to shuffle around the deck chairs too much, because most of this code will be removed once the new qapidoc generator (the "transmogrifier") is in place. To ease my pain: just turn off the black auto-formatter for most, but not all, of qapidoc.py. This will help ensure that *new* code follows a coding standard without bothering too much with cleaning up the existing code. For manual checking for now, try "black --check qapidoc.py" from the docs/sphinx directory. "pip install black" (without root permissions) if you do not have it installed otherwise. Signed-off-by: John Snow --- docs/sphinx/qapidoc.py | 16 +--- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py index f270b494f01..1655682d4c7 100644 --- a/docs/sphinx/qapidoc.py +++ b/docs/sphinx/qapidoc.py @@ -28,28 +28,30 @@ import re from docutils import nodes +from docutils.parsers.rst import Directive, directives from docutils.statemachine import ViewList -from docutils.parsers.rst import directives, Directive -from sphinx.errors import ExtensionError -from sphinx.util.nodes import nested_parse_with_titles -import sphinx -from qapi.gen import QAPISchemaVisitor from qapi.error import QAPIError, QAPISemError +from qapi.gen import QAPISchemaVisitor from qapi.schema import QAPISchema +import sphinx +from sphinx.errors import ExtensionError +from sphinx.util.nodes import nested_parse_with_titles + # Sphinx up to 1.6 uses AutodocReporter; 1.7 and later # use switch_source_input. Check borrowed from kerneldoc.py. -Use_SSI = sphinx.__version__[:3] >= '1.7' +Use_SSI = sphinx.__version__[:3] >= "1.7" if Use_SSI: from sphinx.util.docutils import switch_source_input else: from sphinx.ext.autodoc import AutodocReporter -__version__ = '1.0' +__version__ = "1.0" +# fmt: off # Function borrowed from pydash, which is under the MIT license def intersperse(iterable, separator): """Yield the members of *iterable* interspersed with *separator*.""" -- 2.44.0
[PATCH 00/20] qapi: new sphinx qapi domain pre-requisites
Howdy - this patch series is the first batch of patches meant to prepare the QAPI documentation for a new Sphinx module that adds cross-references, an index, improved inlining, elision of types unseen on the wire, and other goodies. This series addresses just existing code and documentation that needs to be changed and doesn't introduce anything new just yet - except the rST conversion of Notes and Examples sections, which DOES impact the existing QAPI documentation generation. If you're CC'd on this series, it's *probably* because I've adjusted some QAPI documentation that you're the maintainer of - In most cases, these changes are purely mechanical (converting QAPI sections into pure rST) and probably nothing too interesting. In a small handful of cases (patches 15-17), I've been a bit more invasive and you may want to take a quick peek. Overview: Patches 1-3: linter/typing cleanup Patches 4-12: QAPI generator fixes/miscellany Patch 13: qapidoc.py fix (to prepare for rST conversion) Patches 14-20: QAPI documentation modifications, rST conversion Sorry, --js John Snow (20): [DO-NOT-MERGE]: Add some ad-hoc linting helpers. qapi: linter fixups docs/qapidoc: delint a tiny portion of the module qapi/parser: preserve indentation in QAPIDoc sections qapi/parser: adjust info location for doc body section qapi/parser: fix comment parsing immediately following a doc block qapi/parser: add semantic 'kind' parameter to QAPIDoc.Section qapi/parser: differentiate intro and outro paragraphs qapi/parser: add undocumented stub members to all_sections qapi/schema: add __iter__ method to QAPISchemaVariants qapi/schema: add doc_visible property to QAPISchemaDefinition qapi/source: allow multi-line QAPISourceInfo advancing docs/qapidoc: fix nested parsing under untagged sections qapi: fix non-compliant JSON examples qapi: remove developer factoring comments from QAPI doc blocks qapi: rewrite StatsFilter comment qapi: rewrite BlockExportOptions doc block qapi: ensure all errors sections are uniformly typset qapi: convert "Note" sections to plain rST qapi: convert "Example" sections to rST docs/sphinx/qapidoc.py| 62 -- qapi/acpi.json| 6 +- qapi/audio.json | 5 +- qapi/block-core.json | 195 ++ qapi/block-export.json| 16 +- qapi/block.json | 62 +++--- qapi/char.json| 53 +++-- qapi/control.json | 32 +-- qapi/crypto.json | 33 ++- qapi/dump.json| 14 +- qapi/introspect.json | 6 +- qapi/machine-target.json | 29 +-- qapi/machine.json | 138 +++-- qapi/migration.json | 159 +- qapi/misc-target.json | 33 ++- qapi/misc.json| 139 +++-- qapi/net.json | 49 +++-- qapi/pci.json | 11 +- qapi/qapi-schema.json | 6 +- qapi/qdev.json| 45 ++-- qapi/qom.json | 69 +++ qapi/replay.json | 12 +- qapi/rocker.json | 30 +-- qapi/run-state.json | 63 +++--- qapi/sockets.json | 10 +- qapi/stats.json | 30 ++- qapi/tpm.json | 9 +- qapi/trace.json | 6 +- qapi/transaction.json | 13 +- qapi/ui.json | 107 +- qapi/virtio.json | 50 ++--- qapi/yank.json| 6 +- qga/qapi-schema.json | 48 ++--- scripts/qapi-lint.sh | 51 + scripts/qapi/Makefile | 5 + scripts/qapi/introspect.py| 12 +- scripts/qapi/parser.py| 104 -- scripts/qapi/schema.py| 54 - scripts/qapi/source.py| 4 +- scripts/qapi/types.py | 4 +- scripts/qapi/visit.py | 9 +- tests/qapi-schema/doc-empty-section.err | 2 +- tests/qapi-schema/doc-empty-section.json | 2 +- tests/qapi-schema/doc-good.json | 18 +- tests/qapi-schema/doc-good.out| 61 +++--- tests/qapi-schema/doc-good.txt| 31 +-- .../qapi-schema/doc-interleaved-section.json | 2 +- 47 files changed, 1152 insertions(+), 753 deletions(-) create mode 100755 scripts/qapi-lint.sh
[PATCH 13/20] docs/qapidoc: fix nested parsing under untagged sections
Sphinx does not like sections without titles, because it wants to
convert every section into a reference. When there is no title, it
struggles to do this and transforms the tree inproperly.
Depending on the rST used, this may result in an assertion error deep in
the docutils HTMLWriter.
When parsing an untagged section (free paragraphs), skip making a hollow
section and instead append the parse results to the prior section.
Many Bothans died to bring us this information.
Signed-off-by: John Snow
---
docs/sphinx/qapidoc.py | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 34e95bd168d..cfc0cf169ef 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -286,14 +286,20 @@ def _nodes_for_sections(self, doc):
if section.tag and section.tag == 'TODO':
# Hide TODO: sections
continue
+
+if not section.tag:
+# Sphinx cannot handle sectionless titles;
+# Instead, just append the results to the prior section.
+container = nodes.container()
+self._parse_text_into_node(section.text, container)
+nodelist += container.children
+continue
+
snode = self._make_section(section.tag)
-if section.tag and section.tag.startswith('Example'):
+if section.tag.startswith('Example'):
snode += self._nodes_for_example(dedent(section.text))
else:
-self._parse_text_into_node(
-dedent(section.text) if section.tag else section.text,
-snode,
-)
+self._parse_text_into_node(dedent(section.text), snode)
nodelist.append(snode)
return nodelist
--
2.44.0
[PATCH 10/20] qapi/schema: add __iter__ method to QAPISchemaVariants
This just makes it easier to do something like:
for var in variants:
...
Instead of the more cumbersome and repetitive:
for var in variants.variants:
...
Especially in conjunction with entities that aren't guaranteed to have
variants. Compare:
for var in variants.variants if variants else []:
...
against:
for var in variants or []:
...
Update callsites to reflect the new usage pattern.
Signed-off-by: John Snow
---
docs/sphinx/qapidoc.py | 2 +-
scripts/qapi/introspect.py | 4 ++--
scripts/qapi/schema.py | 8 ++--
scripts/qapi/types.py | 4 ++--
scripts/qapi/visit.py | 4 ++--
5 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 2e3ffcbafb7..34e95bd168d 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -204,7 +204,7 @@ def _nodes_for_members(self, doc, what, base=None,
branches=None):
None)
if branches:
-for v in branches.variants:
+for v in branches:
if v.type.name == 'q_empty':
continue
assert not v.type.is_implicit()
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index ac14b20f308..6ec34e055d3 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -342,7 +342,7 @@ def visit_object_type_flat(self, name: str, info:
Optional[QAPISourceInfo],
}
if branches:
obj['tag'] = branches.tag_member.name
-obj['variants'] = [self._gen_variant(v) for v in branches.variants]
+obj['variants'] = [self._gen_variant(v) for v in branches]
self._gen_tree(name, 'object', obj, ifcond, features)
def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
@@ -353,7 +353,7 @@ def visit_alternate_type(self, name: str, info:
Optional[QAPISourceInfo],
name, 'alternate',
{'members': [Annotated({'type': self._use_type(m.type)},
m.ifcond)
- for m in alternatives.variants]},
+ for m in alternatives]},
ifcond, features
)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index d65c35f6ee6..e15e64ea8cb 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -26,6 +26,7 @@
Any,
Callable,
Dict,
+Iterator,
List,
Optional,
Union,
@@ -669,7 +670,7 @@ def check(self, schema: QAPISchema) -> None:
# so we have to check for potential name collisions ourselves.
seen: Dict[str, QAPISchemaMember] = {}
types_seen: Dict[str, str] = {}
-for v in self.alternatives.variants:
+for v in self.alternatives:
v.check_clash(self.info, seen)
qtype = v.type.alternate_qtype()
if not qtype:
@@ -700,7 +701,7 @@ def check(self, schema: QAPISchema) -> None:
def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
super().connect_doc(doc)
doc = doc or self.doc
-for v in self.alternatives.variants:
+for v in self.alternatives:
v.connect_doc(doc)
def c_type(self) -> str:
@@ -726,6 +727,9 @@ def __init__(
self.tag_member: QAPISchemaObjectTypeMember
self.variants = variants
+def __iter__(self) -> Iterator[QAPISchemaVariant]:
+return iter(self.variants)
+
def set_defined_in(self, name: str) -> None:
for v in self.variants:
v.set_defined_in(name)
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 0dd0b00ada3..ad36b55488f 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -166,7 +166,7 @@ def gen_object(name: str, ifcond: QAPISchemaIfCond,
objects_seen.add(name)
ret = ''
-for var in variants.variants if variants else ():
+for var in variants or ():
obj = var.type
if not isinstance(obj, QAPISchemaObjectType):
continue
@@ -234,7 +234,7 @@ def gen_variants(variants: QAPISchemaVariants) -> str:
''',
c_name=c_name(variants.tag_member.name))
-for var in variants.variants:
+for var in variants:
if var.type.name == 'q_empty':
continue
ret += var.ifcond.gen_if()
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 12f92e429f6..1eca452378c 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -141,7 +141,7 @@ def gen_visit_object_members(name: str,
''',
c_name=c_name(tag_member.name))
-for var in branches.variants:
+for var in branches:
case_str = c_enum_const(tag_member.type.name, var.name,
tag_member.type.prefix)
ret += var.ifcond.gen_if()
@@ -246,7 +246,7 @@ def gen_visit_alternate(name: str,
''',
c_name=c_name(name))
-for
[PATCH 12/20] qapi/source: allow multi-line QAPISourceInfo advancing
This is for the sake of the new rST generator (the "transmogrifier") so we can advance multiple lines on occasion while keeping the generated<-->source mappings accurate. next_line now simply takes an optional n parameter which chooses the number of lines to advance. Signed-off-by: John Snow --- scripts/qapi/source.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py index 7b379fdc925..ffdc3f482ac 100644 --- a/scripts/qapi/source.py +++ b/scripts/qapi/source.py @@ -47,9 +47,9 @@ def set_defn(self, meta: str, name: str) -> None: self.defn_meta = meta self.defn_name = name -def next_line(self: T) -> T: +def next_line(self: T, n: int = 1) -> T: info = copy.copy(self) -info.line += 1 +info.line += n return info def loc(self) -> str: -- 2.44.0
Re: [PATCH v2 41/45] target/hppa: Implement CF_PCREL
* Richard Henderson : > Now that the groundwork has been laid, enabling CF_PCREL within the > translator proper is a simple matter of updating copy_iaoq_entry > and install_iaq_entries. > > We also need to modify the unwind info, since we no longer have > absolute addresses to install. > > As expected, this reduces the runtime overhead of compilation when > running a Linux kernel with address space randomization enabled. Ah! I was wondering why you tried to convert to CF_PCREL at all. So, that's the overall reason. > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller
Re: [PATCH v2 40/45] target/hppa: Adjust priv for B,GATE at runtime
* Richard Henderson : > Do not compile in the priv change based on the first > translation; look up the PTE at execution time. > > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller
Re: [PATCH v2 39/45] target/hppa: Drop tlb_entry return from hppa_get_physical_address
* Richard Henderson : > The return-by-reference is never used. > > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller
Re: [PATCH v2 38/45] target/hppa: Implement PSW_X
* Richard Henderson : > Use PAGE_WRITE_INV to temporarily enable write permission > on for a given page, driven by PSW_X being set. > > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller
Re: [PATCH v2 37/45] target/hppa: Implement PSW_B
* Richard Henderson : > PSW_B causes B,GATE to trap as an illegal instruction, removing > the sequential execution test that was merely an approximation. > > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller
Re: [PATCH v2 36/45] target/hppa: Manage PSW_X and PSW_B in translator
* Richard Henderson : > PSW_X is cleared after every instruction, and only set by RFI. > PSW_B is cleared after every non-branch, or branch not taken, > and only set by taken branches. We can clear both bits with a > single store, at most once per TB. Taken branches set PSW_B, > at most once per TB. > > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller
Re: [PATCH v2 35/45] target/hppa: Split PSW X and B into their own field
* Richard Henderson : > Generally, both of these bits are cleared at the end of each > instruction. By separating these, we will be able to clear > both with a single insn, instead of 2 or 3. > > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller
Re: [PATCH v2 34/45] target/hppa: Improve hppa_cpu_dump_state
* Richard Henderson : > Print both raw IAQ_Front and IAQ_Back as well as the GVAs. > Print control registers in system mode. > Print floating point register if CPU_DUMP_FPU. > > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller
Re: [PATCH v2 33/45] target/hppa: Do not mask in copy_iaoq_entry
* Richard Henderson : > As with loads and stores, code offsets are kept intact until the > full gva is formed. In qemu, this is in cpu_get_tb_cpu_state. > > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller
Re: [PATCH v2 32/45] target/hppa: Store full iaoq_f and page offset of iaoq_b in TB
* Richard Henderson : > In preparation for CF_PCREL. store the iaoq_f in 3 parts: high > bits in cs_base, middle bits in pc, and low bits in priv. > For iaoq_b, set a bit for either of space or page differing, > else the page offset. > > Install iaq entries before goto_tb. The change to not record > the full direct branch difference in TB means that we have to > store at least iaoq_b before goto_tb. But we since we'll need > both updated before goto_tb for CF_PCREL, do that now. Does this sentence make sense? ^^ For me as non-native english speaker it sounds wrong, or missing commas, but maybe I'm just wrong...? Other than that...: Reviewed-by: Helge Deller > Signed-off-by: Richard Henderson
Re: [PATCH v2 03/15] hw/riscv: add RISC-V IOMMU base emulation
Hi Jason,
On 5/1/24 08:57, Jason Chien wrote:
Daniel Henrique Barboza 於 2024/3/8 上午 12:03 寫道:
From: Tomasz Jeznach
The RISC-V IOMMU specification is now ratified as-per the RISC-V
international process. The latest frozen specifcation can be found
at:
https://github.com/riscv-non-isa/riscv-iommu/releases/download/v1.0/riscv-iommu.pdf
Add the foundation of the device emulation for RISC-V IOMMU, which
includes an IOMMU that has no capabilities but MSI interrupt support and
fault queue interfaces. We'll add add more features incrementally in the
next patches.
Co-developed-by: Sebastien Boeuf
Signed-off-by: Sebastien Boeuf
Signed-off-by: Tomasz Jeznach
Signed-off-by: Daniel Henrique Barboza
---
hw/riscv/Kconfig |4 +
hw/riscv/meson.build |1 +
hw/riscv/riscv-iommu.c | 1492 ++
hw/riscv/riscv-iommu.h | 141
hw/riscv/trace-events| 11 +
hw/riscv/trace.h |2 +
include/hw/riscv/iommu.h | 36 +
meson.build |1 +
8 files changed, 1688 insertions(+)
create mode 100644 hw/riscv/riscv-iommu.c
create mode 100644 hw/riscv/riscv-iommu.h
create mode 100644 hw/riscv/trace-events
create mode 100644 hw/riscv/trace.h
create mode 100644 include/hw/riscv/iommu.h
diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
index 5d644eb7b1..faf6a10029 100644
--- a/hw/riscv/Kconfig
+++ b/hw/riscv/Kconfig
@@ -1,3 +1,6 @@
+config RISCV_IOMMU
+bool
+
(...)
+
+/* IOMMU index for transactions without PASID specified. */
+#define RISCV_IOMMU_NOPASID 0
+
+static void riscv_iommu_notify(RISCVIOMMUState *s, int vec)
+{
+const uint32_t ipsr =
+riscv_iommu_reg_mod32(s, RISCV_IOMMU_REG_IPSR, (1 << vec), 0);
+const uint32_t ivec = riscv_iommu_reg_get32(s, RISCV_IOMMU_REG_IVEC);
+if (s->notify && !(ipsr & (1 << vec))) {
+s->notify(s, (ivec >> (vec * 4)) & 0x0F);
+}
+}
The RISC-V IOMMU also supports WSI.
+
I mentioned in the review with Frank that this impl does not support WSI, but
it really seems clearer to do the check here nevertheless. I'll add it.
+static void riscv_iommu_fault(RISCVIOMMUState *s,
+ struct riscv_iommu_fq_record *ev)
+{
(...)
+
+/*
+ * Check supported device id width (in bits).
+ * See IOMMU Specification, Chapter 6. Software guidelines.
+ * - if extended device-context format is used:
+ * 1LVL: 6, 2LVL: 15, 3LVL: 24
+ * - if base device-context format is used:
+ * 1LVL: 7, 2LVL: 16, 3LVL: 24
+ */
+if (ctx->devid >= (1 << (depth * 9 + 6 + (dc_fmt && depth != 2 {
+return RISCV_IOMMU_FQ_CAUSE_DDT_INVALID;
The cause should be 260 not 258.
From the RISC-V IOMMU Architecture Spec v1.0.0 section 2.3:
If the device_id is wider than that supported by the IOMMU mode, as determined by the
following checks then stop and report "Transaction type disallowed" (cause =
260).
a. ddtp.iommu_mode is 2LVL and DDI[2] is not 0
b. ddtp.iommu_mode is 1LVL and either DDI[2] is not 0 or DDI[1] is not 0
Changed.
+}
+
+/* Device directory tree walk */
+for (; depth-- > 0; ) {
+/*
+ * Select device id index bits based on device directory tree level
+ * and device context format.
+ * See IOMMU Specification, Chapter 2. Data Structures.
+ * - if extended device-context format is used:
+ * device index: [23:15][14:6][5:0]
+ * - if base device-context format is used:
+ * device index: [23:16][15:7][6:0]
+ */
+const int split = depth * 9 + 6 + dc_fmt;
+addr |= ((ctx->devid >> split) << 3) & ~TARGET_PAGE_MASK;
+if (dma_memory_read(s->target_as, addr, , sizeof(de),
+MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
+return RISCV_IOMMU_FQ_CAUSE_DDT_LOAD_FAULT;
+}
+le64_to_cpus();
+if (!(de & RISCV_IOMMU_DDTE_VALID)) {
+/* invalid directory entry */
+return RISCV_IOMMU_FQ_CAUSE_DDT_INVALID;
+}
+if (de & ~(RISCV_IOMMU_DDTE_PPN | RISCV_IOMMU_DDTE_VALID)) {
+/* reserved bits set */
+return RISCV_IOMMU_FQ_CAUSE_DDT_INVALID;
The cause should be 259 not 258.
From RISC-V IOMMU Architecture Spec v1.0.0 section 2.3.1:
If any bits or encoding that are reserved for future standard use are set within ddte,
stop and report "DDT entry misconfigured" (cause = 259).
Changed
+}
+addr = PPN_PHYS(get_field(de, RISCV_IOMMU_DDTE_PPN));
+}
+
+/* index into device context entry page */
+addr |= (ctx->devid * dc_len) & ~TARGET_PAGE_MASK;
+
+memset(, 0, sizeof(dc));
+if (dma_memory_read(s->target_as, addr, , dc_len,
+MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
+return RISCV_IOMMU_FQ_CAUSE_DDT_LOAD_FAULT;
+}
+
+/* Set translation context. */
+ctx->tc = le64_to_cpu(dc.tc);
+ctx->ta = le64_to_cpu(dc.ta);
+
Re: [PATCH v2 31/45] linux-user/hppa: Force all code addresses to PRIV_USER
* Richard Henderson : > The kernel does this along the return path to user mode. > > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller > linux-user/hppa/target_cpu.h | 4 ++-- > target/hppa/cpu.h| 3 +++ > linux-user/elfload.c | 4 ++-- > linux-user/hppa/cpu_loop.c | 14 +++--- > linux-user/hppa/signal.c | 6 -- > target/hppa/cpu.c| 7 +-- > target/hppa/gdbstub.c| 6 ++ > target/hppa/translate.c | 4 ++-- > 8 files changed, 31 insertions(+), 17 deletions(-)
Re: [PATCH v2 30/45] target/hppa: Use delay_excp for conditional trap on overflow
* Richard Henderson : > Signed-off-by: Richard Henderson > --- > target/hppa/helper.h | 1 - > target/hppa/int_helper.c | 2 +- > target/hppa/op_helper.c | 7 --- > target/hppa/translate.c | 21 + > 4 files changed, 14 insertions(+), 17 deletions(-) Reviewed-by: Helge Deller
Re: [PATCH v2 29/45] target/hppa: Use delay_excp for conditional traps
* Richard Henderson : > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller > target/hppa/helper.h | 1 - > target/hppa/int_helper.c | 2 +- > target/hppa/op_helper.c | 7 --- > target/hppa/translate.c | 41 ++-- > 4 files changed, 32 insertions(+), 19 deletions(-)
Re: [PATCH v2 28/45] target/hppa: Introduce DisasDelayException
* Richard Henderson : > Allow an exception to be emitted at the end of the TranslationBlock, > leaving only the conditional branch inline. Use it for simple > exception instructions like break, which happen to be nullified. > > Signed-off-by: Richard Henderson > --- > target/hppa/translate.c | 60 + > 1 file changed, 55 insertions(+), 5 deletions(-) Reviewed-by: Helge Deller
Re: [PATCH v2 27/45] target/hppa: Remove cond_free
* Richard Henderson : > Now that we do not need to free tcg temporaries, the only > thing cond_free does is reset the condition to never. > Instead, simply write a new condition over the old, which > may be simply cond_make_f() for the never condition. > > The do_*_cond functions do the right thing with c or cf == 0, > so there's no need for a special case anymore. > > Signed-off-by: Richard Henderson > --- > target/hppa/translate.c | 102 +++- > 1 file changed, 27 insertions(+), 75 deletions(-) Reviewed-by: Helge Deller
Re: [PATCH v2 26/45] target/hppa: Use TCG_COND_TST* in trans_ftest
* Richard Henderson : > Signed-off-by: Richard Henderson > --- > target/hppa/translate.c | 22 ++ > 1 file changed, 6 insertions(+), 16 deletions(-) Reviewed-by: Helge Deller
Re: [PATCH v2 25/45] target/hppa: Use registerfields.h for FPSR
* Richard Henderson : > Define all of the context dependent field definitions. > Use FIELD_EX32 and FIELD_DP32 with named fields instead > of extract32 and deposit32 with raw constants. > > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller > --- > target/hppa/cpu.h| 25 + > target/hppa/fpu_helper.c | 26 +- > target/hppa/translate.c | 18 -- > 3 files changed, 46 insertions(+), 23 deletions(-)
Re: [PATCH 4/9] migration: Add direct-io parameter
Markus Armbruster writes:
> Peter Xu writes:
>
>> On Fri, May 03, 2024 at 05:49:32PM -0300, Fabiano Rosas wrote:
>>> Peter Xu writes:
>>>
>>> > On Fri, Apr 26, 2024 at 11:20:37AM -0300, Fabiano Rosas wrote:
>>> >> Add the direct-io migration parameter that tells the migration code to
>>> >> use O_DIRECT when opening the migration stream file whenever possible.
>>> >>
>>> >> This is currently only used with the mapped-ram migration that has a
>>> >> clear window guaranteed to perform aligned writes.
>>> >>
>>> >> Acked-by: Markus Armbruster
>>> >> Signed-off-by: Fabiano Rosas
>>> >> ---
>>> >> include/qemu/osdep.h | 2 ++
>>> >> migration/migration-hmp-cmds.c | 11 +++
>>> >> migration/options.c| 30 ++
>>> >> migration/options.h| 1 +
>>> >> qapi/migration.json| 18 +++---
>>> >> util/osdep.c | 9 +
>>> >> 6 files changed, 68 insertions(+), 3 deletions(-)
>>> >>
>>> >> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>>> >> index c7053cdc2b..645c14a65d 100644
>>> >> --- a/include/qemu/osdep.h
>>> >> +++ b/include/qemu/osdep.h
>>> >> @@ -612,6 +612,8 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t
>>> >> len, bool exclusive);
>>> >> bool qemu_has_ofd_lock(void);
>>> >> #endif
>>> >>
>>> >> +bool qemu_has_direct_io(void);
>>> >> +
>>> >> #if defined(__HAIKU__) && defined(__i386__)
>>> >> #define FMT_pid "%ld"
>>> >> #elif defined(WIN64)
>>> >> diff --git a/migration/migration-hmp-cmds.c
>>> >> b/migration/migration-hmp-cmds.c
>>> >> index 7e96ae6ffd..8496a2b34e 100644
>>> >> --- a/migration/migration-hmp-cmds.c
>>> >> +++ b/migration/migration-hmp-cmds.c
>>> >> @@ -397,6 +397,13 @@ void hmp_info_migrate_parameters(Monitor *mon,
>>> >> const QDict *qdict)
>>> >> monitor_printf(mon, "%s: %s\n",
>>> >> MigrationParameter_str(MIGRATION_PARAMETER_MODE),
>>> >> qapi_enum_lookup(_lookup, params->mode));
>>> >> +
>>> >> +if (params->has_direct_io) {
>>> >> +monitor_printf(mon, "%s: %s\n",
>>> >> + MigrationParameter_str(
>>> >> + MIGRATION_PARAMETER_DIRECT_IO),
>>> >> + params->direct_io ? "on" : "off");
>>> >> +}
>>> >
>>> > This will be the first parameter to optionally display here. I think it's
>>> > a sign of misuse of has_direct_io field..
>
> Yes. For similar existing parameters, we do
>
>assert(params->has_FOO);
>monitor_printf(mon, "%s: '%s'\n",
> MigrationParameter_str(MIGRATION_PARAMETER_FOO),
> ... params->FOO as string ...)
>
>>> > IMHO has_direct_io should be best to be kept as "whether direct_io field
>>> > is
>>> > valid" and that's all of it. It hopefully shouldn't contain more
>>> > information than that, or otherwise it'll be another small challenge we
>>> > need to overcome when we can remove all these has_* fields, and can also
>>> > be
>>> > easily overlooked.
>>>
>>> I don't think I understand why we have those has_* fields. I thought my
>>> usage of 'params->has_direct_io = qemu_has_direct_io()' was the correct
>>> one, i.e. checking whether QEMU has any support for that parameter. Can
>>> you help me out here?
>>
>> Here params is the pointer to "struct MigrationParameters", which is
>> defined in qapi/migration.json. And we have had "has_*" only because we
>> allow optional fields with asterisks:
>>
>> { 'struct': 'MigrationParameters',
>> 'data': { '*announce-initial': 'size',
>> ...
>> } }
>>
>> So that's why it better only means "whether this field existed", because
>> it's how it is defined.
>>
>> IIRC we (or say, Markus) used to have some attempts deduplicates those
>> *MigrationParameter* things, and if success we have chance to drop has_*
>> fields (in which case we simply always have them; that "has_" makes more
>> sense only if in a QMP session to allow user only specify one or more
>> things if not all).
>
> qapi/migration.json:
>
> ##
> # @MigrationParameters:
> #
> --> # The optional members aren't actually optional.
> #
>
> In other words, the has_FOO generated for the members of
> MigrationParameters must all be true.
>
> These members became optional when we attempted to de-duplicate
> MigrationParameters and MigrateSetParameters, but failed (see commit
> de63ab61241 "migrate: Share common MigrationParameters struct" and
> commit 1bda8b3c695 "migration: Unshare MigrationParameters struct for
> now").
So what's the blocker for merging MigrationSetParameters and
MigrationParameters? Just the tls_* options?
Re: [PATCH 02/11] scripts/update-linux-header.sh: be more src tree friendly
On 5/14/24 10:42, Alex Bennée wrote: Running "install_headers" in the Linux source tree is fairly unfriendly as out-of-tree builds will start complaining about the kernel source being non-pristine. As we have a temporary directory for the install we should also do the build step here. So now we have: $tmpdir/ $blddir/ $hdrdir/ Signed-off-by: Alex Bennée Message-Id: <20240510111802.241284-1-alex.ben...@linaro.org> --- scripts/update-linux-headers.sh | 80 + 1 file changed, 41 insertions(+), 39 deletions(-) diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh index 36f3e91fe4..8963c39189 100755 --- a/scripts/update-linux-headers.sh +++ b/scripts/update-linux-headers.sh @@ -27,6 +27,8 @@ # types like "__u64". This work is done in the cp_portable function. tmpdir=$(mktemp -d) +hdrdir="$tmpdir/headers" +blddir="$tmpdir/build" linux="$1" output="$2" @@ -110,56 +112,56 @@ for arch in $ARCHLIST; do arch_var=ARCH fi -make -C "$linux" INSTALL_HDR_PATH="$tmpdir" $arch_var=$arch headers_install +make -C "$linux" O="$blddir" INSTALL_HDR_PATH="$hdrdir" $arch_var=$arch headers_install rm -rf "$output/linux-headers/asm-$arch" mkdir -p "$output/linux-headers/asm-$arch" for header in kvm.h unistd.h bitsperlong.h mman.h; do -cp "$tmpdir/include/asm/$header" "$output/linux-headers/asm-$arch" +cp "$hdrdir/include/asm/$header" "$output/linux-headers/asm-$arch" done if [ $arch = mips ]; then -cp "$tmpdir/include/asm/sgidefs.h" "$output/linux-headers/asm-mips/" -cp "$tmpdir/include/asm/unistd_o32.h" "$output/linux-headers/asm-mips/" -cp "$tmpdir/include/asm/unistd_n32.h" "$output/linux-headers/asm-mips/" -cp "$tmpdir/include/asm/unistd_n64.h" "$output/linux-headers/asm-mips/" +cp "$hdrdir/include/asm/sgidefs.h" "$output/linux-headers/asm-mips/" +cp "$hdrdir/include/asm/unistd_o32.h" "$output/linux-headers/asm-mips/" +cp "$hdrdir/include/asm/unistd_n32.h" "$output/linux-headers/asm-mips/" +cp "$hdrdir/include/asm/unistd_n64.h" "$output/linux-headers/asm-mips/" fi if [ $arch = powerpc ]; then -cp "$tmpdir/include/asm/unistd_32.h" "$output/linux-headers/asm-powerpc/" -cp "$tmpdir/include/asm/unistd_64.h" "$output/linux-headers/asm-powerpc/" +cp "$hdrdir/include/asm/unistd_32.h" "$output/linux-headers/asm-powerpc/" +cp "$hdrdir/include/asm/unistd_64.h" "$output/linux-headers/asm-powerpc/" fi rm -rf "$output/include/standard-headers/asm-$arch" mkdir -p "$output/include/standard-headers/asm-$arch" if [ $arch = s390 ]; then -cp_portable "$tmpdir/include/asm/virtio-ccw.h" "$output/include/standard-headers/asm-s390/" -cp "$tmpdir/include/asm/unistd_32.h" "$output/linux-headers/asm-s390/" -cp "$tmpdir/include/asm/unistd_64.h" "$output/linux-headers/asm-s390/" +cp_portable "$hdrdir/include/asm/virtio-ccw.h" "$output/include/standard-headers/asm-s390/" +cp "$hdrdir/include/asm/unistd_32.h" "$output/linux-headers/asm-s390/" +cp "$hdrdir/include/asm/unistd_64.h" "$output/linux-headers/asm-s390/" fi if [ $arch = arm ]; then -cp "$tmpdir/include/asm/unistd-eabi.h" "$output/linux-headers/asm-arm/" -cp "$tmpdir/include/asm/unistd-oabi.h" "$output/linux-headers/asm-arm/" -cp "$tmpdir/include/asm/unistd-common.h" "$output/linux-headers/asm-arm/" +cp "$hdrdir/include/asm/unistd-eabi.h" "$output/linux-headers/asm-arm/" +cp "$hdrdir/include/asm/unistd-oabi.h" "$output/linux-headers/asm-arm/" +cp "$hdrdir/include/asm/unistd-common.h" "$output/linux-headers/asm-arm/" fi if [ $arch = arm64 ]; then -cp "$tmpdir/include/asm/sve_context.h" "$output/linux-headers/asm-arm64/" +cp "$hdrdir/include/asm/sve_context.h" "$output/linux-headers/asm-arm64/" fi if [ $arch = x86 ]; then -cp "$tmpdir/include/asm/unistd_32.h" "$output/linux-headers/asm-x86/" -cp "$tmpdir/include/asm/unistd_x32.h" "$output/linux-headers/asm-x86/" -cp "$tmpdir/include/asm/unistd_64.h" "$output/linux-headers/asm-x86/" -cp_portable "$tmpdir/include/asm/kvm_para.h" "$output/include/standard-headers/asm-$arch" +cp "$hdrdir/include/asm/unistd_32.h" "$output/linux-headers/asm-x86/" +cp "$hdrdir/include/asm/unistd_x32.h" "$output/linux-headers/asm-x86/" +cp "$hdrdir/include/asm/unistd_64.h" "$output/linux-headers/asm-x86/" +cp_portable "$hdrdir/include/asm/kvm_para.h" "$output/include/standard-headers/asm-$arch" # Remove everything except the macros from bootparam.h avoiding the # unnecessary import of several video/ist/etc headers sed -e '/__ASSEMBLY__/,/__ASSEMBLY__/d' \ - "$tmpdir/include/asm/bootparam.h" >
Re: [PATCH 01/11] tests/tcg: don't append QEMU_OPTS for armv6m-undef test
On 5/14/24 10:42, Alex Bennée wrote: We don't want to build on the default machine setup here but define a custom one for the microbit. Signed-off-by: Alex Bennée --- tests/tcg/arm/Makefile.softmmu-target | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tcg/arm/Makefile.softmmu-target b/tests/tcg/arm/Makefile.softmmu-target index 4c9264057f..39e01ce49d 100644 --- a/tests/tcg/arm/Makefile.softmmu-target +++ b/tests/tcg/arm/Makefile.softmmu-target @@ -16,7 +16,7 @@ test-armv6m-undef: test-armv6m-undef.S $< -o $@ -nostdlib -N -static \ -T $(ARM_SRC)/$@.ld -run-test-armv6m-undef: QEMU_OPTS+=-semihosting -M microbit -kernel +run-test-armv6m-undef: QEMU_OPTS=-semihosting-config enable=on,target=native,chardev=output -M microbit -kernel ARM_TESTS+=test-armv6m-undef Reviewed-by: Pierrick Bouvier
[PATCH 01/11] tests/tcg: don't append QEMU_OPTS for armv6m-undef test
We don't want to build on the default machine setup here but define a custom one for the microbit. Signed-off-by: Alex Bennée --- tests/tcg/arm/Makefile.softmmu-target | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tcg/arm/Makefile.softmmu-target b/tests/tcg/arm/Makefile.softmmu-target index 4c9264057f..39e01ce49d 100644 --- a/tests/tcg/arm/Makefile.softmmu-target +++ b/tests/tcg/arm/Makefile.softmmu-target @@ -16,7 +16,7 @@ test-armv6m-undef: test-armv6m-undef.S $< -o $@ -nostdlib -N -static \ -T $(ARM_SRC)/$@.ld -run-test-armv6m-undef: QEMU_OPTS+=-semihosting -M microbit -kernel +run-test-armv6m-undef: QEMU_OPTS=-semihosting-config enable=on,target=native,chardev=output -M microbit -kernel ARM_TESTS+=test-armv6m-undef -- 2.39.2
[PATCH 09/11] plugins: distinct types for callbacks
From: Pierrick Bouvier
To prevent errors when writing new types of callbacks or inline
operations, we split callbacks data to distinct types.
Reviewed-by: Richard Henderson
Signed-off-by: Pierrick Bouvier
Message-Id: <20240502211522.346467-8-pierrick.bouv...@linaro.org>
Signed-off-by: Alex Bennée
---
include/qemu/plugin.h | 46 ++---
plugins/plugin.h | 2 +-
accel/tcg/plugin-gen.c | 58 +---
plugins/core.c | 76 ++
4 files changed, 98 insertions(+), 84 deletions(-)
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index c7b3b1cd66..98d27dded9 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -74,34 +74,40 @@ enum plugin_dyn_cb_type {
PLUGIN_CB_INLINE_STORE_U64,
};
+struct qemu_plugin_regular_cb {
+union qemu_plugin_cb_sig f;
+TCGHelperInfo *info;
+void *userp;
+enum qemu_plugin_mem_rw rw;
+};
+
+struct qemu_plugin_inline_cb {
+qemu_plugin_u64 entry;
+enum qemu_plugin_op op;
+uint64_t imm;
+enum qemu_plugin_mem_rw rw;
+};
+
+struct qemu_plugin_conditional_cb {
+union qemu_plugin_cb_sig f;
+TCGHelperInfo *info;
+void *userp;
+qemu_plugin_u64 entry;
+enum qemu_plugin_cond cond;
+uint64_t imm;
+};
+
/*
* A dynamic callback has an insertion point that is determined at run-time.
* Usually the insertion point is somewhere in the code cache; think for
* instance of a callback to be called upon the execution of a particular TB.
*/
struct qemu_plugin_dyn_cb {
-void *userp;
enum plugin_dyn_cb_type type;
-/* @rw applies to mem callbacks only (both regular and inline) */
-enum qemu_plugin_mem_rw rw;
-/* fields specific to each dyn_cb type go here */
union {
-struct {
-union qemu_plugin_cb_sig f;
-TCGHelperInfo *info;
-} regular;
-struct {
-union qemu_plugin_cb_sig f;
-TCGHelperInfo *info;
-qemu_plugin_u64 entry;
-enum qemu_plugin_cond cond;
-uint64_t imm;
-} cond;
-struct {
-qemu_plugin_u64 entry;
-enum qemu_plugin_op op;
-uint64_t imm;
-} inline_insn;
+struct qemu_plugin_regular_cb regular;
+struct qemu_plugin_conditional_cb cond;
+struct qemu_plugin_inline_cb inline_insn;
};
};
diff --git a/plugins/plugin.h b/plugins/plugin.h
index 7d4b4e21f7..80d5daa917 100644
--- a/plugins/plugin.h
+++ b/plugins/plugin.h
@@ -108,7 +108,7 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
enum qemu_plugin_mem_rw rw,
void *udata);
-void exec_inline_op(struct qemu_plugin_dyn_cb *cb, int cpu_index);
+void exec_inline_op(struct qemu_plugin_inline_cb *cb, int cpu_index);
int plugin_num_vcpus(void);
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index f2190f3511..e018728573 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -101,13 +101,13 @@ static void gen_disable_mem_helper(void)
offsetof(ArchCPU, env));
}
-static void gen_udata_cb(struct qemu_plugin_dyn_cb *cb)
+static void gen_udata_cb(struct qemu_plugin_regular_cb *cb)
{
TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
tcg_gen_ld_i32(cpu_index, tcg_env,
-offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
-tcg_gen_call2(cb->regular.f.vcpu_udata, cb->regular.info, NULL,
+tcg_gen_call2(cb->f.vcpu_udata, cb->info, NULL,
tcgv_i32_temp(cpu_index),
tcgv_ptr_temp(tcg_constant_ptr(cb->userp)));
tcg_temp_free_i32(cpu_index);
@@ -153,21 +153,21 @@ static TCGCond plugin_cond_to_tcgcond(enum
qemu_plugin_cond cond)
}
}
-static void gen_udata_cond_cb(struct qemu_plugin_dyn_cb *cb)
+static void gen_udata_cond_cb(struct qemu_plugin_conditional_cb *cb)
{
-TCGv_ptr ptr = gen_plugin_u64_ptr(cb->cond.entry);
+TCGv_ptr ptr = gen_plugin_u64_ptr(cb->entry);
TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
TCGv_i64 val = tcg_temp_ebb_new_i64();
TCGLabel *after_cb = gen_new_label();
/* Condition should be negated, as calling the cb is the "else" path */
-TCGCond cond = tcg_invert_cond(plugin_cond_to_tcgcond(cb->cond.cond));
+TCGCond cond = tcg_invert_cond(plugin_cond_to_tcgcond(cb->cond));
tcg_gen_ld_i64(val, ptr, 0);
-tcg_gen_brcondi_i64(cond, val, cb->cond.imm, after_cb);
+tcg_gen_brcondi_i64(cond, val, cb->imm, after_cb);
tcg_gen_ld_i32(cpu_index, tcg_env,
-offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
-tcg_gen_call2(cb->cond.f.vcpu_udata, cb->cond.info, NULL,
+tcg_gen_call2(cb->f.vcpu_udata, cb->info, NULL,
tcgv_i32_temp(cpu_index),
tcgv_ptr_temp(tcg_constant_ptr(cb->userp)));
gen_set_label(after_cb);
@@ -177,37 +177,37
[PATCH 11/11] plugins: remove op from qemu_plugin_inline_cb
From: Pierrick Bouvier
This field is not needed as the callback type already holds this
information.
Reviewed-by: Richard Henderson
Signed-off-by: Pierrick Bouvier
Message-Id: <20240502211522.346467-10-pierrick.bouv...@linaro.org>
Signed-off-by: Alex Bennée
---
include/qemu/plugin.h | 1 -
plugins/plugin.h | 4 +++-
plugins/core.c| 13 +++--
3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index 98d27dded9..796fc13706 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -83,7 +83,6 @@ struct qemu_plugin_regular_cb {
struct qemu_plugin_inline_cb {
qemu_plugin_u64 entry;
-enum qemu_plugin_op op;
uint64_t imm;
enum qemu_plugin_mem_rw rw;
};
diff --git a/plugins/plugin.h b/plugins/plugin.h
index 80d5daa917..30e2299a54 100644
--- a/plugins/plugin.h
+++ b/plugins/plugin.h
@@ -108,7 +108,9 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
enum qemu_plugin_mem_rw rw,
void *udata);
-void exec_inline_op(struct qemu_plugin_inline_cb *cb, int cpu_index);
+void exec_inline_op(enum plugin_dyn_cb_type type,
+struct qemu_plugin_inline_cb *cb,
+int cpu_index);
int plugin_num_vcpus(void);
diff --git a/plugins/core.c b/plugins/core.c
index 1c85edc5e5..0726bc7f25 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -338,7 +338,6 @@ void plugin_register_inline_op_on_entry(GArray **arr,
struct qemu_plugin_inline_cb inline_cb = { .rw = rw,
.entry = entry,
- .op = op,
.imm = imm };
dyn_cb = plugin_get_dyn_cb(arr);
dyn_cb->type = op_to_cb_type(op);
@@ -557,7 +556,9 @@ void qemu_plugin_flush_cb(void)
plugin_cb__simple(QEMU_PLUGIN_EV_FLUSH);
}
-void exec_inline_op(struct qemu_plugin_inline_cb *cb, int cpu_index)
+void exec_inline_op(enum plugin_dyn_cb_type type,
+struct qemu_plugin_inline_cb *cb,
+int cpu_index)
{
char *ptr = cb->entry.score->data->data;
size_t elem_size = g_array_get_element_size(
@@ -565,11 +566,11 @@ void exec_inline_op(struct qemu_plugin_inline_cb *cb, int
cpu_index)
size_t offset = cb->entry.offset;
uint64_t *val = (uint64_t *)(ptr + offset + cpu_index * elem_size);
-switch (cb->op) {
-case QEMU_PLUGIN_INLINE_ADD_U64:
+switch (type) {
+case PLUGIN_CB_INLINE_ADD_U64:
*val += cb->imm;
break;
-case QEMU_PLUGIN_INLINE_STORE_U64:
+case PLUGIN_CB_INLINE_STORE_U64:
*val = cb->imm;
break;
default:
@@ -601,7 +602,7 @@ void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr,
case PLUGIN_CB_INLINE_ADD_U64:
case PLUGIN_CB_INLINE_STORE_U64:
if (rw && cb->inline_insn.rw) {
-exec_inline_op(>inline_insn, cpu->cpu_index);
+exec_inline_op(cb->type, >inline_insn, cpu->cpu_index);
}
break;
default:
--
2.39.2
[PATCH 02/11] scripts/update-linux-header.sh: be more src tree friendly
Running "install_headers" in the Linux source tree is fairly unfriendly as out-of-tree builds will start complaining about the kernel source being non-pristine. As we have a temporary directory for the install we should also do the build step here. So now we have: $tmpdir/ $blddir/ $hdrdir/ Signed-off-by: Alex Bennée Message-Id: <20240510111802.241284-1-alex.ben...@linaro.org> --- scripts/update-linux-headers.sh | 80 + 1 file changed, 41 insertions(+), 39 deletions(-) diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh index 36f3e91fe4..8963c39189 100755 --- a/scripts/update-linux-headers.sh +++ b/scripts/update-linux-headers.sh @@ -27,6 +27,8 @@ # types like "__u64". This work is done in the cp_portable function. tmpdir=$(mktemp -d) +hdrdir="$tmpdir/headers" +blddir="$tmpdir/build" linux="$1" output="$2" @@ -110,56 +112,56 @@ for arch in $ARCHLIST; do arch_var=ARCH fi -make -C "$linux" INSTALL_HDR_PATH="$tmpdir" $arch_var=$arch headers_install +make -C "$linux" O="$blddir" INSTALL_HDR_PATH="$hdrdir" $arch_var=$arch headers_install rm -rf "$output/linux-headers/asm-$arch" mkdir -p "$output/linux-headers/asm-$arch" for header in kvm.h unistd.h bitsperlong.h mman.h; do -cp "$tmpdir/include/asm/$header" "$output/linux-headers/asm-$arch" +cp "$hdrdir/include/asm/$header" "$output/linux-headers/asm-$arch" done if [ $arch = mips ]; then -cp "$tmpdir/include/asm/sgidefs.h" "$output/linux-headers/asm-mips/" -cp "$tmpdir/include/asm/unistd_o32.h" "$output/linux-headers/asm-mips/" -cp "$tmpdir/include/asm/unistd_n32.h" "$output/linux-headers/asm-mips/" -cp "$tmpdir/include/asm/unistd_n64.h" "$output/linux-headers/asm-mips/" +cp "$hdrdir/include/asm/sgidefs.h" "$output/linux-headers/asm-mips/" +cp "$hdrdir/include/asm/unistd_o32.h" "$output/linux-headers/asm-mips/" +cp "$hdrdir/include/asm/unistd_n32.h" "$output/linux-headers/asm-mips/" +cp "$hdrdir/include/asm/unistd_n64.h" "$output/linux-headers/asm-mips/" fi if [ $arch = powerpc ]; then -cp "$tmpdir/include/asm/unistd_32.h" "$output/linux-headers/asm-powerpc/" -cp "$tmpdir/include/asm/unistd_64.h" "$output/linux-headers/asm-powerpc/" +cp "$hdrdir/include/asm/unistd_32.h" "$output/linux-headers/asm-powerpc/" +cp "$hdrdir/include/asm/unistd_64.h" "$output/linux-headers/asm-powerpc/" fi rm -rf "$output/include/standard-headers/asm-$arch" mkdir -p "$output/include/standard-headers/asm-$arch" if [ $arch = s390 ]; then -cp_portable "$tmpdir/include/asm/virtio-ccw.h" "$output/include/standard-headers/asm-s390/" -cp "$tmpdir/include/asm/unistd_32.h" "$output/linux-headers/asm-s390/" -cp "$tmpdir/include/asm/unistd_64.h" "$output/linux-headers/asm-s390/" +cp_portable "$hdrdir/include/asm/virtio-ccw.h" "$output/include/standard-headers/asm-s390/" +cp "$hdrdir/include/asm/unistd_32.h" "$output/linux-headers/asm-s390/" +cp "$hdrdir/include/asm/unistd_64.h" "$output/linux-headers/asm-s390/" fi if [ $arch = arm ]; then -cp "$tmpdir/include/asm/unistd-eabi.h" "$output/linux-headers/asm-arm/" -cp "$tmpdir/include/asm/unistd-oabi.h" "$output/linux-headers/asm-arm/" -cp "$tmpdir/include/asm/unistd-common.h" "$output/linux-headers/asm-arm/" +cp "$hdrdir/include/asm/unistd-eabi.h" "$output/linux-headers/asm-arm/" +cp "$hdrdir/include/asm/unistd-oabi.h" "$output/linux-headers/asm-arm/" +cp "$hdrdir/include/asm/unistd-common.h" "$output/linux-headers/asm-arm/" fi if [ $arch = arm64 ]; then -cp "$tmpdir/include/asm/sve_context.h" "$output/linux-headers/asm-arm64/" +cp "$hdrdir/include/asm/sve_context.h" "$output/linux-headers/asm-arm64/" fi if [ $arch = x86 ]; then -cp "$tmpdir/include/asm/unistd_32.h" "$output/linux-headers/asm-x86/" -cp "$tmpdir/include/asm/unistd_x32.h" "$output/linux-headers/asm-x86/" -cp "$tmpdir/include/asm/unistd_64.h" "$output/linux-headers/asm-x86/" -cp_portable "$tmpdir/include/asm/kvm_para.h" "$output/include/standard-headers/asm-$arch" +cp "$hdrdir/include/asm/unistd_32.h" "$output/linux-headers/asm-x86/" +cp "$hdrdir/include/asm/unistd_x32.h" "$output/linux-headers/asm-x86/" +cp "$hdrdir/include/asm/unistd_64.h" "$output/linux-headers/asm-x86/" +cp_portable "$hdrdir/include/asm/kvm_para.h" "$output/include/standard-headers/asm-$arch" # Remove everything except the macros from bootparam.h avoiding the # unnecessary import of several video/ist/etc headers sed -e '/__ASSEMBLY__/,/__ASSEMBLY__/d' \ - "$tmpdir/include/asm/bootparam.h" > "$tmpdir/bootparam.h" -cp_portable "$tmpdir/bootparam.h" \ +
[PATCH 05/11] plugins: add new inline op STORE_U64
From: Pierrick Bouvier
This new operation can store an immediate u64 value to a given
scoreboard.
Reviewed-by: Richard Henderson
Signed-off-by: Pierrick Bouvier
Message-Id: <20240502211522.346467-4-pierrick.bouv...@linaro.org>
Signed-off-by: Alex Bennée
---
include/qemu/plugin.h | 1 +
include/qemu/qemu-plugin.h | 4 ++--
accel/tcg/plugin-gen.c | 13 +
plugins/core.c | 6 ++
4 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index 93da98b76c..6c21a30105 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -70,6 +70,7 @@ enum plugin_dyn_cb_type {
PLUGIN_CB_REGULAR,
PLUGIN_CB_MEM_REGULAR,
PLUGIN_CB_INLINE_ADD_U64,
+PLUGIN_CB_INLINE_STORE_U64,
};
/*
diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 4fc6c3739b..c5cac897a0 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -305,12 +305,12 @@ void qemu_plugin_register_vcpu_tb_exec_cb(struct
qemu_plugin_tb *tb,
* enum qemu_plugin_op - describes an inline op
*
* @QEMU_PLUGIN_INLINE_ADD_U64: add an immediate value uint64_t
- *
- * Note: currently only a single inline op is supported.
+ * @QEMU_PLUGIN_INLINE_STORE_U64: store an immediate value uint64_t
*/
enum qemu_plugin_op {
QEMU_PLUGIN_INLINE_ADD_U64,
+QEMU_PLUGIN_INLINE_STORE_U64,
};
/**
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 97868781fe..88976289eb 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -145,6 +145,16 @@ static void gen_inline_add_u64_cb(struct
qemu_plugin_dyn_cb *cb)
tcg_temp_free_ptr(ptr);
}
+static void gen_inline_store_u64_cb(struct qemu_plugin_dyn_cb *cb)
+{
+TCGv_ptr ptr = gen_plugin_u64_ptr(cb->inline_insn.entry);
+TCGv_i64 val = tcg_constant_i64(cb->inline_insn.imm);
+
+tcg_gen_st_i64(val, ptr, 0);
+
+tcg_temp_free_ptr(ptr);
+}
+
static void gen_mem_cb(struct qemu_plugin_dyn_cb *cb,
qemu_plugin_meminfo_t meminfo, TCGv_i64 addr)
{
@@ -170,6 +180,9 @@ static void inject_cb(struct qemu_plugin_dyn_cb *cb)
case PLUGIN_CB_INLINE_ADD_U64:
gen_inline_add_u64_cb(cb);
break;
+case PLUGIN_CB_INLINE_STORE_U64:
+gen_inline_store_u64_cb(cb);
+break;
default:
g_assert_not_reached();
}
diff --git a/plugins/core.c b/plugins/core.c
index 59771eda8f..848d482fc4 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -321,6 +321,8 @@ static enum plugin_dyn_cb_type op_to_cb_type(enum
qemu_plugin_op op)
switch (op) {
case QEMU_PLUGIN_INLINE_ADD_U64:
return PLUGIN_CB_INLINE_ADD_U64;
+case QEMU_PLUGIN_INLINE_STORE_U64:
+return PLUGIN_CB_INLINE_STORE_U64;
default:
g_assert_not_reached();
}
@@ -535,6 +537,9 @@ void exec_inline_op(struct qemu_plugin_dyn_cb *cb, int
cpu_index)
case QEMU_PLUGIN_INLINE_ADD_U64:
*val += cb->inline_insn.imm;
break;
+case QEMU_PLUGIN_INLINE_STORE_U64:
+*val = cb->inline_insn.imm;
+break;
default:
g_assert_not_reached();
}
@@ -562,6 +567,7 @@ void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr,
vaddr, cb->userp);
break;
case PLUGIN_CB_INLINE_ADD_U64:
+case PLUGIN_CB_INLINE_STORE_U64:
exec_inline_op(cb, cpu->cpu_index);
break;
default:
--
2.39.2
[PATCH 00/11] maintainer updates (plugins, testing) pre-PR
This is mostly plugin related stuff which is all ready to go however I have a few miscellaneous testing updates which would appreciate the review. Thanks. Alex Bennée (2): tests/tcg: don't append QEMU_OPTS for armv6m-undef test scripts/update-linux-header.sh: be more src tree friendly Pierrick Bouvier (9): plugins: prepare introduction of new inline ops plugins: extract generate ptr for qemu_plugin_u64 plugins: add new inline op STORE_U64 tests/plugin/inline: add test for STORE_U64 inline op plugins: conditional callbacks tests/plugin/inline: add test for conditional callback plugins: distinct types for callbacks plugins: extract cpu_index generate plugins: remove op from qemu_plugin_inline_cb include/qemu/plugin.h | 42 +--- include/qemu/qemu-plugin.h| 80 ++- plugins/plugin.h | 12 ++- accel/tcg/plugin-gen.c| 136 -- plugins/api.c | 39 plugins/core.c| 109 +++-- tests/plugin/inline.c | 130 ++-- plugins/qemu-plugins.symbols | 2 + scripts/update-linux-headers.sh | 80 +++ tests/tcg/arm/Makefile.softmmu-target | 2 +- 10 files changed, 508 insertions(+), 124 deletions(-) -- 2.39.2
[PATCH 04/11] plugins: extract generate ptr for qemu_plugin_u64
From: Pierrick Bouvier
Plugin operations can access a scoreboard. This function factorizes code
generation for accessing entry associated to a given vcpu.
Reviewed-by: Richard Henderson
Signed-off-by: Pierrick Bouvier
Message-Id: <20240502211522.346467-3-pierrick.bouv...@linaro.org>
Signed-off-by: Alex Bennée
---
accel/tcg/plugin-gen.c | 27 ++-
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 4069d51daf..97868781fe 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -113,24 +113,33 @@ static void gen_udata_cb(struct qemu_plugin_dyn_cb *cb)
tcg_temp_free_i32(cpu_index);
}
-static void gen_inline_add_u64_cb(struct qemu_plugin_dyn_cb *cb)
+static TCGv_ptr gen_plugin_u64_ptr(qemu_plugin_u64 entry)
{
-GArray *arr = cb->inline_insn.entry.score->data;
-size_t offset = cb->inline_insn.entry.offset;
-TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
-TCGv_i64 val = tcg_temp_ebb_new_i64();
TCGv_ptr ptr = tcg_temp_ebb_new_ptr();
+GArray *arr = entry.score->data;
+char *base_ptr = arr->data + entry.offset;
+size_t entry_size = g_array_get_element_size(arr);
+
+TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
tcg_gen_ld_i32(cpu_index, tcg_env,
-offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
-tcg_gen_muli_i32(cpu_index, cpu_index, g_array_get_element_size(arr));
+tcg_gen_muli_i32(cpu_index, cpu_index, entry_size);
tcg_gen_ext_i32_ptr(ptr, cpu_index);
tcg_temp_free_i32(cpu_index);
+tcg_gen_addi_ptr(ptr, ptr, (intptr_t) base_ptr);
+
+return ptr;
+}
+
+static void gen_inline_add_u64_cb(struct qemu_plugin_dyn_cb *cb)
+{
+TCGv_ptr ptr = gen_plugin_u64_ptr(cb->inline_insn.entry);
+TCGv_i64 val = tcg_temp_ebb_new_i64();
-tcg_gen_addi_ptr(ptr, ptr, (intptr_t)arr->data);
-tcg_gen_ld_i64(val, ptr, offset);
+tcg_gen_ld_i64(val, ptr, 0);
tcg_gen_addi_i64(val, val, cb->inline_insn.imm);
-tcg_gen_st_i64(val, ptr, offset);
+tcg_gen_st_i64(val, ptr, 0);
tcg_temp_free_i64(val);
tcg_temp_free_ptr(ptr);
--
2.39.2
[PATCH 08/11] tests/plugin/inline: add test for conditional callback
From: Pierrick Bouvier
Count number of tb and insn executed using a conditional callback. We
ensure the callback has been called expected number of time (per vcpu).
Reviewed-by: Richard Henderson
Signed-off-by: Pierrick Bouvier
Message-Id: <20240502211522.346467-7-pierrick.bouv...@linaro.org>
Signed-off-by: Alex Bennée
---
tests/plugin/inline.c | 89 +--
1 file changed, 86 insertions(+), 3 deletions(-)
diff --git a/tests/plugin/inline.c b/tests/plugin/inline.c
index 103c3a22f6..cd63827b7d 100644
--- a/tests/plugin/inline.c
+++ b/tests/plugin/inline.c
@@ -20,8 +20,14 @@ typedef struct {
uint64_t count_insn_inline;
uint64_t count_mem;
uint64_t count_mem_inline;
+uint64_t tb_cond_num_trigger;
+uint64_t tb_cond_track_count;
+uint64_t insn_cond_num_trigger;
+uint64_t insn_cond_track_count;
} CPUCount;
+static const uint64_t cond_trigger_limit = 100;
+
typedef struct {
uint64_t data_insn;
uint64_t data_tb;
@@ -35,6 +41,10 @@ static qemu_plugin_u64 count_insn;
static qemu_plugin_u64 count_insn_inline;
static qemu_plugin_u64 count_mem;
static qemu_plugin_u64 count_mem_inline;
+static qemu_plugin_u64 tb_cond_num_trigger;
+static qemu_plugin_u64 tb_cond_track_count;
+static qemu_plugin_u64 insn_cond_num_trigger;
+static qemu_plugin_u64 insn_cond_track_count;
static struct qemu_plugin_scoreboard *data;
static qemu_plugin_u64 data_insn;
static qemu_plugin_u64 data_tb;
@@ -56,12 +66,19 @@ static void stats_insn(void)
const uint64_t per_vcpu = qemu_plugin_u64_sum(count_insn);
const uint64_t inl_per_vcpu =
qemu_plugin_u64_sum(count_insn_inline);
+const uint64_t cond_num_trigger =
+qemu_plugin_u64_sum(insn_cond_num_trigger);
+const uint64_t cond_track_left =
qemu_plugin_u64_sum(insn_cond_track_count);
+const uint64_t conditional =
+cond_num_trigger * cond_trigger_limit + cond_track_left;
printf("insn: %" PRIu64 "\n", expected);
printf("insn: %" PRIu64 " (per vcpu)\n", per_vcpu);
printf("insn: %" PRIu64 " (per vcpu inline)\n", inl_per_vcpu);
+printf("insn: %" PRIu64 " (cond cb)\n", conditional);
g_assert(expected > 0);
g_assert(per_vcpu == expected);
g_assert(inl_per_vcpu == expected);
+g_assert(conditional == expected);
}
static void stats_tb(void)
@@ -70,12 +87,18 @@ static void stats_tb(void)
const uint64_t per_vcpu = qemu_plugin_u64_sum(count_tb);
const uint64_t inl_per_vcpu =
qemu_plugin_u64_sum(count_tb_inline);
+const uint64_t cond_num_trigger = qemu_plugin_u64_sum(tb_cond_num_trigger);
+const uint64_t cond_track_left = qemu_plugin_u64_sum(tb_cond_track_count);
+const uint64_t conditional =
+cond_num_trigger * cond_trigger_limit + cond_track_left;
printf("tb: %" PRIu64 "\n", expected);
printf("tb: %" PRIu64 " (per vcpu)\n", per_vcpu);
printf("tb: %" PRIu64 " (per vcpu inline)\n", inl_per_vcpu);
+printf("tb: %" PRIu64 " (conditional cb)\n", conditional);
g_assert(expected > 0);
g_assert(per_vcpu == expected);
g_assert(inl_per_vcpu == expected);
+g_assert(conditional == expected);
}
static void stats_mem(void)
@@ -104,14 +127,35 @@ static void plugin_exit(qemu_plugin_id_t id, void *udata)
const uint64_t insn_inline = qemu_plugin_u64_get(count_insn_inline, i);
const uint64_t mem = qemu_plugin_u64_get(count_mem, i);
const uint64_t mem_inline = qemu_plugin_u64_get(count_mem_inline, i);
-printf("cpu %d: tb (%" PRIu64 ", %" PRIu64 ") | "
- "insn (%" PRIu64 ", %" PRIu64 ") | "
+const uint64_t tb_cond_trigger =
+qemu_plugin_u64_get(tb_cond_num_trigger, i);
+const uint64_t tb_cond_left =
+qemu_plugin_u64_get(tb_cond_track_count, i);
+const uint64_t insn_cond_trigger =
+qemu_plugin_u64_get(insn_cond_num_trigger, i);
+const uint64_t insn_cond_left =
+qemu_plugin_u64_get(insn_cond_track_count, i);
+printf("cpu %d: tb (%" PRIu64 ", %" PRIu64
+ ", %" PRIu64 " * %" PRIu64 " + %" PRIu64
+ ") | "
+ "insn (%" PRIu64 ", %" PRIu64
+ ", %" PRIu64 " * %" PRIu64 " + %" PRIu64
+ ") | "
"mem (%" PRIu64 ", %" PRIu64 ")"
"\n",
- i, tb, tb_inline, insn, insn_inline, mem, mem_inline);
+ i,
+ tb, tb_inline,
+ tb_cond_trigger, cond_trigger_limit, tb_cond_left,
+ insn, insn_inline,
+ insn_cond_trigger, cond_trigger_limit, insn_cond_left,
+ mem, mem_inline);
g_assert(tb == tb_inline);
g_assert(insn == insn_inline);
g_assert(mem == mem_inline);
+g_assert(tb_cond_trigger == tb / cond_trigger_limit);
+g_assert(tb_cond_left == tb % cond_trigger_limit);
+g_assert(insn_cond_trigger == insn /
[PATCH 03/11] plugins: prepare introduction of new inline ops
From: Pierrick Bouvier
Until now, only add_u64 was available, and all functions assumed this or
were named uniquely.
Reviewed-by: Richard Henderson
Signed-off-by: Pierrick Bouvier
Message-Id: <20240502211522.346467-2-pierrick.bouv...@linaro.org>
Signed-off-by: Alex Bennée
---
include/qemu/plugin.h | 2 +-
accel/tcg/plugin-gen.c | 6 +++---
plugins/core.c | 14 --
3 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index b535bfd5de..93da98b76c 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -69,7 +69,7 @@ union qemu_plugin_cb_sig {
enum plugin_dyn_cb_type {
PLUGIN_CB_REGULAR,
PLUGIN_CB_MEM_REGULAR,
-PLUGIN_CB_INLINE,
+PLUGIN_CB_INLINE_ADD_U64,
};
/*
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 49f5d1c2e4..4069d51daf 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -113,7 +113,7 @@ static void gen_udata_cb(struct qemu_plugin_dyn_cb *cb)
tcg_temp_free_i32(cpu_index);
}
-static void gen_inline_cb(struct qemu_plugin_dyn_cb *cb)
+static void gen_inline_add_u64_cb(struct qemu_plugin_dyn_cb *cb)
{
GArray *arr = cb->inline_insn.entry.score->data;
size_t offset = cb->inline_insn.entry.offset;
@@ -158,8 +158,8 @@ static void inject_cb(struct qemu_plugin_dyn_cb *cb)
case PLUGIN_CB_REGULAR:
gen_udata_cb(cb);
break;
-case PLUGIN_CB_INLINE:
-gen_inline_cb(cb);
+case PLUGIN_CB_INLINE_ADD_U64:
+gen_inline_add_u64_cb(cb);
break;
default:
g_assert_not_reached();
diff --git a/plugins/core.c b/plugins/core.c
index 1e58a57bf1..59771eda8f 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -316,6 +316,16 @@ static struct qemu_plugin_dyn_cb *plugin_get_dyn_cb(GArray
**arr)
return _array_index(cbs, struct qemu_plugin_dyn_cb, cbs->len - 1);
}
+static enum plugin_dyn_cb_type op_to_cb_type(enum qemu_plugin_op op)
+{
+switch (op) {
+case QEMU_PLUGIN_INLINE_ADD_U64:
+return PLUGIN_CB_INLINE_ADD_U64;
+default:
+g_assert_not_reached();
+}
+}
+
void plugin_register_inline_op_on_entry(GArray **arr,
enum qemu_plugin_mem_rw rw,
enum qemu_plugin_op op,
@@ -326,7 +336,7 @@ void plugin_register_inline_op_on_entry(GArray **arr,
dyn_cb = plugin_get_dyn_cb(arr);
dyn_cb->userp = NULL;
-dyn_cb->type = PLUGIN_CB_INLINE;
+dyn_cb->type = op_to_cb_type(op);
dyn_cb->rw = rw;
dyn_cb->inline_insn.entry = entry;
dyn_cb->inline_insn.op = op;
@@ -551,7 +561,7 @@ void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr,
cb->regular.f.vcpu_mem(cpu->cpu_index, make_plugin_meminfo(oi, rw),
vaddr, cb->userp);
break;
-case PLUGIN_CB_INLINE:
+case PLUGIN_CB_INLINE_ADD_U64:
exec_inline_op(cb, cpu->cpu_index);
break;
default:
--
2.39.2
[PATCH 06/11] tests/plugin/inline: add test for STORE_U64 inline op
From: Pierrick Bouvier
Reviewed-by: Richard Henderson
Signed-off-by: Pierrick Bouvier
Message-Id: <20240502211522.346467-5-pierrick.bouv...@linaro.org>
Signed-off-by: Alex Bennée
---
tests/plugin/inline.c | 41 +
1 file changed, 37 insertions(+), 4 deletions(-)
diff --git a/tests/plugin/inline.c b/tests/plugin/inline.c
index 0163e9b51c..103c3a22f6 100644
--- a/tests/plugin/inline.c
+++ b/tests/plugin/inline.c
@@ -22,6 +22,12 @@ typedef struct {
uint64_t count_mem_inline;
} CPUCount;
+typedef struct {
+uint64_t data_insn;
+uint64_t data_tb;
+uint64_t data_mem;
+} CPUData;
+
static struct qemu_plugin_scoreboard *counts;
static qemu_plugin_u64 count_tb;
static qemu_plugin_u64 count_tb_inline;
@@ -29,6 +35,10 @@ static qemu_plugin_u64 count_insn;
static qemu_plugin_u64 count_insn_inline;
static qemu_plugin_u64 count_mem;
static qemu_plugin_u64 count_mem_inline;
+static struct qemu_plugin_scoreboard *data;
+static qemu_plugin_u64 data_insn;
+static qemu_plugin_u64 data_tb;
+static qemu_plugin_u64 data_mem;
static uint64_t global_count_tb;
static uint64_t global_count_insn;
@@ -109,11 +119,13 @@ static void plugin_exit(qemu_plugin_id_t id, void *udata)
stats_mem();
qemu_plugin_scoreboard_free(counts);
+qemu_plugin_scoreboard_free(data);
}
static void vcpu_tb_exec(unsigned int cpu_index, void *udata)
{
qemu_plugin_u64_add(count_tb, cpu_index, 1);
+g_assert(qemu_plugin_u64_get(data_tb, cpu_index) == (uintptr_t) udata);
g_mutex_lock(_lock);
max_cpu_index = MAX(max_cpu_index, cpu_index);
global_count_tb++;
@@ -123,6 +135,7 @@ static void vcpu_tb_exec(unsigned int cpu_index, void
*udata)
static void vcpu_insn_exec(unsigned int cpu_index, void *udata)
{
qemu_plugin_u64_add(count_insn, cpu_index, 1);
+g_assert(qemu_plugin_u64_get(data_insn, cpu_index) == (uintptr_t) udata);
g_mutex_lock(_lock);
global_count_insn++;
g_mutex_unlock(_lock);
@@ -131,9 +144,10 @@ static void vcpu_insn_exec(unsigned int cpu_index, void
*udata)
static void vcpu_mem_access(unsigned int cpu_index,
qemu_plugin_meminfo_t info,
uint64_t vaddr,
-void *userdata)
+void *udata)
{
qemu_plugin_u64_add(count_mem, cpu_index, 1);
+g_assert(qemu_plugin_u64_get(data_mem, cpu_index) == (uintptr_t) udata);
g_mutex_lock(_lock);
global_count_mem++;
g_mutex_unlock(_lock);
@@ -141,20 +155,34 @@ static void vcpu_mem_access(unsigned int cpu_index,
static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
{
+void *tb_store = tb;
+qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu(
+tb, QEMU_PLUGIN_INLINE_STORE_U64, data_tb, (uintptr_t) tb_store);
qemu_plugin_register_vcpu_tb_exec_cb(
-tb, vcpu_tb_exec, QEMU_PLUGIN_CB_NO_REGS, 0);
+tb, vcpu_tb_exec, QEMU_PLUGIN_CB_NO_REGS, tb_store);
qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu(
tb, QEMU_PLUGIN_INLINE_ADD_U64, count_tb_inline, 1);
for (int idx = 0; idx < qemu_plugin_tb_n_insns(tb); ++idx) {
struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, idx);
+void *insn_store = insn;
+void *mem_store = (char *)insn_store + 0xff;
+
+qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu(
+insn, QEMU_PLUGIN_INLINE_STORE_U64, data_insn,
+(uintptr_t) insn_store);
qemu_plugin_register_vcpu_insn_exec_cb(
-insn, vcpu_insn_exec, QEMU_PLUGIN_CB_NO_REGS, 0);
+insn, vcpu_insn_exec, QEMU_PLUGIN_CB_NO_REGS, insn_store);
qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu(
insn, QEMU_PLUGIN_INLINE_ADD_U64, count_insn_inline, 1);
+
+qemu_plugin_register_vcpu_mem_inline_per_vcpu(
+insn, QEMU_PLUGIN_MEM_RW,
+QEMU_PLUGIN_INLINE_STORE_U64,
+data_mem, (uintptr_t) mem_store);
qemu_plugin_register_vcpu_mem_cb(insn, _mem_access,
QEMU_PLUGIN_CB_NO_REGS,
- QEMU_PLUGIN_MEM_RW, 0);
+ QEMU_PLUGIN_MEM_RW, mem_store);
qemu_plugin_register_vcpu_mem_inline_per_vcpu(
insn, QEMU_PLUGIN_MEM_RW,
QEMU_PLUGIN_INLINE_ADD_U64,
@@ -179,6 +207,11 @@ int qemu_plugin_install(qemu_plugin_id_t id, const
qemu_info_t *info,
counts, CPUCount, count_insn_inline);
count_mem_inline = qemu_plugin_scoreboard_u64_in_struct(
counts, CPUCount, count_mem_inline);
+data = qemu_plugin_scoreboard_new(sizeof(CPUData));
+data_insn = qemu_plugin_scoreboard_u64_in_struct(data, CPUData, data_insn);
+data_tb = qemu_plugin_scoreboard_u64_in_struct(data, CPUData, data_tb);
+data_mem = qemu_plugin_scoreboard_u64_in_struct(data, CPUData, data_mem);
+
[PATCH 07/11] plugins: conditional callbacks
From: Pierrick Bouvier
Extend plugins API to support callback called with a given criteria
(evaluated inline).
Added functions:
- qemu_plugin_register_vcpu_tb_exec_cond_cb
- qemu_plugin_register_vcpu_insn_exec_cond_cb
They expect as parameter a condition, a qemu_plugin_u64_t (op1) and an
immediate (op2). Callback is called if op1 |cond| op2 is true.
Reviewed-by: Richard Henderson
Signed-off-by: Pierrick Bouvier
Message-Id: <20240502211522.346467-6-pierrick.bouv...@linaro.org>
Signed-off-by: Alex Bennée
---
include/qemu/plugin.h| 8
include/qemu/qemu-plugin.h | 76
plugins/plugin.h | 8
accel/tcg/plugin-gen.c | 48 +++
plugins/api.c| 39 ++
plugins/core.c | 32 +++
plugins/qemu-plugins.symbols | 2 +
7 files changed, 213 insertions(+)
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index 6c21a30105..c7b3b1cd66 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -68,6 +68,7 @@ union qemu_plugin_cb_sig {
enum plugin_dyn_cb_type {
PLUGIN_CB_REGULAR,
+PLUGIN_CB_COND,
PLUGIN_CB_MEM_REGULAR,
PLUGIN_CB_INLINE_ADD_U64,
PLUGIN_CB_INLINE_STORE_U64,
@@ -89,6 +90,13 @@ struct qemu_plugin_dyn_cb {
union qemu_plugin_cb_sig f;
TCGHelperInfo *info;
} regular;
+struct {
+union qemu_plugin_cb_sig f;
+TCGHelperInfo *info;
+qemu_plugin_u64 entry;
+enum qemu_plugin_cond cond;
+uint64_t imm;
+} cond;
struct {
qemu_plugin_u64 entry;
enum qemu_plugin_op op;
diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index c5cac897a0..337de25ece 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -262,6 +262,29 @@ enum qemu_plugin_mem_rw {
QEMU_PLUGIN_MEM_RW,
};
+/**
+ * enum qemu_plugin_cond - condition to enable callback
+ *
+ * @QEMU_PLUGIN_COND_NEVER: false
+ * @QEMU_PLUGIN_COND_ALWAYS: true
+ * @QEMU_PLUGIN_COND_EQ: is equal?
+ * @QEMU_PLUGIN_COND_NE: is not equal?
+ * @QEMU_PLUGIN_COND_LT: is less than?
+ * @QEMU_PLUGIN_COND_LE: is less than or equal?
+ * @QEMU_PLUGIN_COND_GT: is greater than?
+ * @QEMU_PLUGIN_COND_GE: is greater than or equal?
+ */
+enum qemu_plugin_cond {
+QEMU_PLUGIN_COND_NEVER,
+QEMU_PLUGIN_COND_ALWAYS,
+QEMU_PLUGIN_COND_EQ,
+QEMU_PLUGIN_COND_NE,
+QEMU_PLUGIN_COND_LT,
+QEMU_PLUGIN_COND_LE,
+QEMU_PLUGIN_COND_GT,
+QEMU_PLUGIN_COND_GE,
+};
+
/**
* typedef qemu_plugin_vcpu_tb_trans_cb_t - translation callback
* @id: unique plugin id
@@ -301,6 +324,32 @@ void qemu_plugin_register_vcpu_tb_exec_cb(struct
qemu_plugin_tb *tb,
enum qemu_plugin_cb_flags flags,
void *userdata);
+/**
+ * qemu_plugin_register_vcpu_tb_exec_cond_cb() - register conditional callback
+ * @tb: the opaque qemu_plugin_tb handle for the translation
+ * @cb: callback function
+ * @cond: condition to enable callback
+ * @entry: first operand for condition
+ * @imm: second operand for condition
+ * @flags: does the plugin read or write the CPU's registers?
+ * @userdata: any plugin data to pass to the @cb?
+ *
+ * The @cb function is called when a translated unit executes if
+ * entry @cond imm is true.
+ * If condition is QEMU_PLUGIN_COND_ALWAYS, condition is never interpreted and
+ * this function is equivalent to qemu_plugin_register_vcpu_tb_exec_cb.
+ * If condition QEMU_PLUGIN_COND_NEVER, condition is never interpreted and
+ * callback is never installed.
+ */
+QEMU_PLUGIN_API
+void qemu_plugin_register_vcpu_tb_exec_cond_cb(struct qemu_plugin_tb *tb,
+ qemu_plugin_vcpu_udata_cb_t cb,
+ enum qemu_plugin_cb_flags flags,
+ enum qemu_plugin_cond cond,
+ qemu_plugin_u64 entry,
+ uint64_t imm,
+ void *userdata);
+
/**
* enum qemu_plugin_op - describes an inline op
*
@@ -344,6 +393,33 @@ void qemu_plugin_register_vcpu_insn_exec_cb(struct
qemu_plugin_insn *insn,
enum qemu_plugin_cb_flags flags,
void *userdata);
+/**
+ * qemu_plugin_register_vcpu_insn_exec_cond_cb() - conditional insn execution
cb
+ * @insn: the opaque qemu_plugin_insn handle for an instruction
+ * @cb: callback function
+ * @flags: does the plugin read or write the CPU's registers?
+ * @cond: condition to enable callback
+ * @entry: first operand for condition
+ * @imm: second operand for condition
+ * @userdata: any plugin data to pass to the @cb?
+ *
+ * The @cb function is called when an
[PATCH 10/11] plugins: extract cpu_index generate
From: Pierrick Bouvier
Factorizes function to access current cpu index for a given vcpu.
Reviewed-by: Richard Henderson
Signed-off-by: Pierrick Bouvier
Message-Id: <20240502211522.346467-9-pierrick.bouv...@linaro.org>
Signed-off-by: Alex Bennée
---
accel/tcg/plugin-gen.c | 24 +++-
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index e018728573..c9b298667f 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -101,12 +101,17 @@ static void gen_disable_mem_helper(void)
offsetof(ArchCPU, env));
}
-static void gen_udata_cb(struct qemu_plugin_regular_cb *cb)
+static TCGv_i32 gen_cpu_index(void)
{
TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
-
tcg_gen_ld_i32(cpu_index, tcg_env,
-offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
+return cpu_index;
+}
+
+static void gen_udata_cb(struct qemu_plugin_regular_cb *cb)
+{
+TCGv_i32 cpu_index = gen_cpu_index();
tcg_gen_call2(cb->f.vcpu_udata, cb->info, NULL,
tcgv_i32_temp(cpu_index),
tcgv_ptr_temp(tcg_constant_ptr(cb->userp)));
@@ -121,9 +126,7 @@ static TCGv_ptr gen_plugin_u64_ptr(qemu_plugin_u64 entry)
char *base_ptr = arr->data + entry.offset;
size_t entry_size = g_array_get_element_size(arr);
-TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
-tcg_gen_ld_i32(cpu_index, tcg_env,
- -offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
+TCGv_i32 cpu_index = gen_cpu_index();
tcg_gen_muli_i32(cpu_index, cpu_index, entry_size);
tcg_gen_ext_i32_ptr(ptr, cpu_index);
tcg_temp_free_i32(cpu_index);
@@ -156,7 +159,6 @@ static TCGCond plugin_cond_to_tcgcond(enum qemu_plugin_cond
cond)
static void gen_udata_cond_cb(struct qemu_plugin_conditional_cb *cb)
{
TCGv_ptr ptr = gen_plugin_u64_ptr(cb->entry);
-TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
TCGv_i64 val = tcg_temp_ebb_new_i64();
TCGLabel *after_cb = gen_new_label();
@@ -165,15 +167,14 @@ static void gen_udata_cond_cb(struct
qemu_plugin_conditional_cb *cb)
tcg_gen_ld_i64(val, ptr, 0);
tcg_gen_brcondi_i64(cond, val, cb->imm, after_cb);
-tcg_gen_ld_i32(cpu_index, tcg_env,
- -offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
+TCGv_i32 cpu_index = gen_cpu_index();
tcg_gen_call2(cb->f.vcpu_udata, cb->info, NULL,
tcgv_i32_temp(cpu_index),
tcgv_ptr_temp(tcg_constant_ptr(cb->userp)));
+tcg_temp_free_i32(cpu_index);
gen_set_label(after_cb);
tcg_temp_free_i64(val);
-tcg_temp_free_i32(cpu_index);
tcg_temp_free_ptr(ptr);
}
@@ -203,10 +204,7 @@ static void gen_inline_store_u64_cb(struct
qemu_plugin_inline_cb *cb)
static void gen_mem_cb(struct qemu_plugin_regular_cb *cb,
qemu_plugin_meminfo_t meminfo, TCGv_i64 addr)
{
-TCGv_i32 cpu_index = tcg_temp_ebb_new_i32();
-
-tcg_gen_ld_i32(cpu_index, tcg_env,
- -offsetof(ArchCPU, env) + offsetof(CPUState, cpu_index));
+TCGv_i32 cpu_index = gen_cpu_index();
tcg_gen_call4(cb->f.vcpu_mem, cb->info, NULL,
tcgv_i32_temp(cpu_index),
tcgv_i32_temp(tcg_constant_i32(meminfo)),
--
2.39.2
Re: [PATCH 3/3] qerror: QERR_QGA_COMMAND_FAILED is no longer used, drop
Reviewed-by: Konstantin Kostiuk On Tue, May 14, 2024 at 1:58 PM Markus Armbruster wrote: > Signed-off-by: Markus Armbruster > --- > include/qapi/qmp/qerror.h | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h > index 00b18e9082..390590bb81 100644 > --- a/include/qapi/qmp/qerror.h > +++ b/include/qapi/qmp/qerror.h > @@ -29,9 +29,6 @@ > #define QERR_PROPERTY_VALUE_OUT_OF_RANGE \ > "Property %s.%s doesn't take value %" PRId64 " (minimum: %" PRId64 ", > maximum: %" PRId64 ")" > > -#define QERR_QGA_COMMAND_FAILED \ > -"Guest agent command failed, error was '%s'" > - > #define QERR_UNSUPPORTED \ > "this feature or command is not currently supported" > > -- > 2.45.0 > >
Re: [PATCH 2/3] qga: Shorten several error messages
Reviewed-by: Konstantin Kostiuk
On Tue, May 14, 2024 at 1:58 PM Markus Armbruster wrote:
> Some, but not all error messages are of the form
>
> Guest agent command failed, error was ''
>
> For instance, command guest-exec can fail with an error message like
>
> Guest agent command failed, error was 'Failed to execute child process
> “/bin/invalid-cmd42�€? (No such file or directory)'
>
> Shorten this to just just the actual error message. The guest-exec
> example becomes
>
> Failed to execute child process “/bin/invalid-cmd42�€? (No such file
> or directory)
>
> Signed-off-by: Markus Armbruster
> ---
> qga/commands-win32.c | 24
> qga/commands.c | 5 ++---
> 2 files changed, 10 insertions(+), 19 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index ed31077457..0d1b836e87 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -278,8 +278,7 @@ static void acquire_privilege(const char *name, Error
> **errp)
> TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY, ))
> {
> if (!LookupPrivilegeValue(NULL, name, [0].Luid)) {
> -error_setg(errp, QERR_QGA_COMMAND_FAILED,
> - "no luid for requested privilege");
> +error_setg(errp, "no luid for requested privilege");
> goto out;
> }
>
> @@ -287,14 +286,12 @@ static void acquire_privilege(const char *name,
> Error **errp)
> priv.Privileges[0].Attributes = SE_PRIVILEGE_ENABLED;
>
> if (!AdjustTokenPrivileges(token, FALSE, , 0, NULL, 0)) {
> -error_setg(errp, QERR_QGA_COMMAND_FAILED,
> - "unable to acquire requested privilege");
> +error_setg(errp, "unable to acquire requested privilege");
> goto out;
> }
>
> } else {
> -error_setg(errp, QERR_QGA_COMMAND_FAILED,
> - "failed to open privilege token");
> +error_setg(errp, "failed to open privilege token");
> }
>
> out:
> @@ -308,8 +305,7 @@ static void execute_async(DWORD WINAPI
> (*func)(LPVOID), LPVOID opaque,
> {
> HANDLE thread = CreateThread(NULL, 0, func, opaque, 0, NULL);
> if (!thread) {
> -error_setg(errp, QERR_QGA_COMMAND_FAILED,
> - "failed to dispatch asynchronous command");
> +error_setg(errp, "failed to dispatch asynchronous command");
> }
> }
>
> @@ -1418,22 +1414,19 @@ static void check_suspend_mode(GuestSuspendMode
> mode, Error **errp)
>
> ZeroMemory(_pwr_caps, sizeof(sys_pwr_caps));
> if (!GetPwrCapabilities(_pwr_caps)) {
> -error_setg(errp, QERR_QGA_COMMAND_FAILED,
> - "failed to determine guest suspend capabilities");
> +error_setg(errp, "failed to determine guest suspend
> capabilities");
> return;
> }
>
> switch (mode) {
> case GUEST_SUSPEND_MODE_DISK:
> if (!sys_pwr_caps.SystemS4) {
> -error_setg(errp, QERR_QGA_COMMAND_FAILED,
> - "suspend-to-disk not supported by OS");
> +error_setg(errp, "suspend-to-disk not supported by OS");
> }
> break;
> case GUEST_SUSPEND_MODE_RAM:
> if (!sys_pwr_caps.SystemS3) {
> -error_setg(errp, QERR_QGA_COMMAND_FAILED,
> - "suspend-to-ram not supported by OS");
> +error_setg(errp, "suspend-to-ram not supported by OS");
> }
> break;
> default:
> @@ -2175,8 +2168,7 @@ static void ga_get_win_version(RTL_OSVERSIONINFOEXW
> *info, Error **errp)
> HMODULE module = GetModuleHandle("ntdll");
> PVOID fun = GetProcAddress(module, "RtlGetVersion");
> if (fun == NULL) {
> -error_setg(errp, QERR_QGA_COMMAND_FAILED,
> -"Failed to get address of RtlGetVersion");
> +error_setg(errp, "Failed to get address of RtlGetVersion");
> return;
> }
>
> diff --git a/qga/commands.c b/qga/commands.c
> index 88c1c99fe5..27b16313ea 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -475,7 +475,7 @@ GuestExec *qmp_guest_exec(const char *path,
> guest_exec_task_setup, _merge, , input_data ? _fd
> : NULL,
> has_output ? _fd : NULL, has_output ? _fd : NULL,
> );
> if (!ret) {
> -error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
> +error_setg(errp, "%s", gerr->message);
> g_error_free(gerr);
> goto done;
> }
> @@ -586,8 +586,7 @@ GuestTimezone *qmp_guest_get_timezone(Error **errp)
> info = g_new0(GuestTimezone, 1);
> tz = g_time_zone_new_local();
> if (tz == NULL) {
> -error_setg(errp, QERR_QGA_COMMAND_FAILED,
> - "Couldn't retrieve local timezone");
> +error_setg(errp, "Couldn't retrieve local timezone");
> goto error;
> }
>
> --
> 2.45.0
>
>
Re: [PATCH 1/3] qga-win32: Improve guest-set-user-password, guest-file-open errors
Reviewed-by: Konstantin Kostiuk
On Tue, May 14, 2024 at 2:03 PM Markus Armbruster wrote:
> When guest-set-user-password's argument @password can't be converted
> from UTF-8 to UTF-16, we report something like
>
> Guest agent command failed, error was 'Invalid sequence in conversion
> input'
>
> Improve this to
>
> can't convert 'password' to UTF-16: Invalid sequence in conversion
> input
>
> Likewise for argument @username, and guest-file-open argument @path,
> even though I'm not sure you can actually get invalid input past the
> QMP core there.
>
> Signed-off-by: Markus Armbruster
> ---
> qga/commands-win32.c | 17 +
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 6fee0e1e6f..ed31077457 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -217,6 +217,9 @@ int64_t qmp_guest_file_open(const char *path, const
> char *mode, Error **errp)
>
> w_path = g_utf8_to_utf16(path, -1, NULL, NULL, );
> if (!w_path) {
> +error_setg(errp, "can't convert 'path' to UTF-16: %s",
> + gerr->message);
> +g_error_free(gerr);
> goto done;
> }
>
> @@ -244,10 +247,6 @@ int64_t qmp_guest_file_open(const char *path, const
> char *mode, Error **errp)
> slog("guest-file-open, handle: % " PRId64, fd);
>
> done:
> -if (gerr) {
> -error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
> -g_error_free(gerr);
> -}
> g_free(w_path);
> return fd;
> }
> @@ -1946,11 +1945,17 @@ void qmp_guest_set_user_password(const char
> *username,
>
> user = g_utf8_to_utf16(username, -1, NULL, NULL, );
> if (!user) {
> +error_setg(errp, "can't convert 'username' to UTF-16: %s",
> + gerr->message);
> +g_error_free(gerr);
> goto done;
> }
>
> wpass = g_utf8_to_utf16(rawpasswddata, -1, NULL, NULL, );
> if (!wpass) {
> +error_setg(errp, "can't convert 'password' to UTF-16: %s",
> + gerr->message);
> +g_error_free(gerr);
> goto done;
> }
>
> @@ -1966,10 +1971,6 @@ void qmp_guest_set_user_password(const char
> *username,
> }
>
> done:
> -if (gerr) {
> -error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
> -g_error_free(gerr);
> -}
> g_free(user);
> g_free(wpass);
> g_free(rawpasswddata);
> --
> 2.45.0
>
>
Re: [PATCH 0/3] Assorted fixes for PMU
On Tue, May 14, 2024 at 2:16 AM Peter Maydell wrote: > > On Mon, 29 Apr 2024 at 20:29, Atish Patra wrote: > > > > This series contains few miscallenous fixes related to hpmcounters > > and related code. The first patch fixes an issue with cycle/instret > > counters overcouting while the remaining two are more for specification > > compliance. > > I've noticed a number of riscv patchsets from various people > recently where the subject line for the cover letter doesn't > include any indication that it's a riscv related patchset. > For instance this one is just "Assorted fixes for PMU", which > could easily be an Arm PMU series. Could you all try to make sure > that cover letter subject lines indicate the architecture or > other subcomponent they affect, please? This helps people who > are skimming over the mailing list looking for patches relevant > to them. > Makes sense. I will include RISC-V in the series cover letter as well. Thanks for the feedback. > thanks > -- PMM > -- Regards, Atish
Re: [PATCH 0/3] Assorted fixes for PMU
On Tue, May 14, 2024 at 3:18 AM Alistair Francis wrote: > > On Tue, May 14, 2024 at 5:15 PM Atish Kumar Patra wrote: > > > > On Mon, May 13, 2024 at 11:29 PM Alistair Francis > > wrote: > > > > > > On Tue, Apr 30, 2024 at 5:29 AM Atish Patra wrote: > > > > > > > > This series contains few miscallenous fixes related to hpmcounters > > > > and related code. The first patch fixes an issue with cycle/instret > > > > counters overcouting while the remaining two are more for specification > > > > compliance. > > > > > > > > Signed-off-by: Atish Patra > > > > --- > > > > Atish Patra (3): > > > > target/riscv: Save counter values during countinhibit update > > > > target/riscv: Enforce WARL behavior for scounteren/hcounteren > > > > target/riscv: Fix the predicate functions for mhpmeventhX CSRs > > > > > > Thanks! > > > > > > Applied to riscv-to-apply.next > > > > > > > Hi Alistair, > > Thanks for your review. But the patch 1 had some comments about > > vmstate which needs updating. > > Ah, I did read the comments but forgot that you were going to bump the > version. > > > We also found a few more fixes that I was planning to include in v2. > > I found that patch `target/riscv: Save counter values during > countinhibit update` gives me this error as well > > ../target/riscv/csr.c: In function ‘write_mcountinhibit’: > ../target/riscv/csr.c:1981:44: error: too few arguments to function > ‘get_ticks’ > 1981 | counter->mhpmcounter_val = get_ticks(false) - > |^ > ../target/riscv/csr.c:765:21: note: declared here > 765 | static target_ulong get_ticks(bool shift, bool instructions) > | ^ > ../target/riscv/csr.c:1985:49: error: too few arguments to function > ‘get_ticks’ > 1985 | counter->mhpmcounterh_val = get_ticks(false) - > | ^ > ../target/riscv/csr.c:765:21: note: declared here > 765 | static target_ulong get_ticks(bool shift, bool instructions) > | ^ > Yeah. Clement's patch got in. I will rebase and update the series based on the riscv-to-apply.next. > > > > > > I can send a separate fixes series on top riscv-to-apply.next or this > > series can be dropped for the time being. > > I'm going to drop it due to the build error above > > Alistair > > > You can queue it v2 later. Let me know what you prefer. > > > > > > > Alistair > > > > > > > > > > > target/riscv/cpu.h | 1 - > > > > target/riscv/csr.c | 111 > > > > ++--- > > > > target/riscv/machine.c | 1 - > > > > 3 files changed, 68 insertions(+), 45 deletions(-) > > > > --- > > > > base-commit: 1642f979a71a5667a05070be2df82f48bd43ad7a > > > > change-id: 20240428-countinhibit_fix-c6a1c11f4375 > > > > -- > > > > Regards, > > > > Atish patra > > > > > > > > > -- Regards, Atish
Re: [PATCH v2 24/45] target/hppa: Use TCG_COND_TST* in trans_bb_imm
* Richard Henderson :
> Signed-off-by: Richard Henderson
Reviewed-by: Helge Deller
> ---
> target/hppa/translate.c | 12 +++-
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index 47f4b23d1b..d8973a63df 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -3515,18 +3515,12 @@ static bool trans_bb_sar(DisasContext *ctx,
> arg_bb_sar *a)
>
> static bool trans_bb_imm(DisasContext *ctx, arg_bb_imm *a)
> {
> -TCGv_i64 tmp, tcg_r;
> DisasCond cond;
> -int p;
> +int p = a->p | (a->d ? 0 : 32);
>
> nullify_over(ctx);
> -
> -tmp = tcg_temp_new_i64();
> -tcg_r = load_gpr(ctx, a->r);
> -p = a->p | (a->d ? 0 : 32);
> -tcg_gen_shli_i64(tmp, tcg_r, p);
> -
> -cond = cond_make_ti(a->c ? TCG_COND_GE : TCG_COND_LT, tmp, 0);
> +cond = cond_make_vi(a->c ? TCG_COND_TSTEQ : TCG_COND_TSTNE,
> +load_gpr(ctx, a->r), 1ull << (63 - p));
I wonder if this actually fixes a bug...
Before it tested against all values >= tmp (even for which the bit
wasn't set), and now it correctly just checks the bit.
> return do_cbranch(ctx, a->disp, a->n, );
> }
>
> --
> 2.34.1
>
Re: [PATCH v2 23/45] target/hppa: Use TCG_COND_TST* in do_unit_addsub
* Richard Henderson : > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller
Re: [PATCH v2 22/45] target/hppa: Use TCG_COND_TST* in do_unit_zero_cond
* Richard Henderson : > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller
Re: [PATCH v2 21/45] target/hppa: Use TCG_COND_TST* in do_log_cond
* Richard Henderson : > We can directly test bits of a 32-bit comparison without > zero or sign-extending an intermediate result. > We can directly test bit 0 for odd/even. > > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller
Re: [PATCH v2 20/45] target/hppa: Use TCG_COND_TST* in do_cond
* Richard Henderson : > We can directly test bits of a 32-bit comparison without > zero or sign-extending an intermediate result. > We can directly test bit 0 for odd/even. > > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller
Re: [PATCH v2 19/45] target/hppa: Rename cond_make_* helpers
* Richard Henderson : > Use 'v' for a variable that needs copying, 't' for a temp that > doesn't need copying, and 'i' for an immediate, and use this > naming for both arguments of the comparison. So: > >cond_make_tmp -> cond_make_tt >cond_make_0_tmp -> cond_make_ti >cond_make_0 -> cond_make_vi >cond_make -> cond_make_vv > > Pass 0 explictly, rather than implicitly in the function name. > > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller
Re: [PATCH v9 0/6] Pointer Masking update for Zjpm v1.0
>The cover letter says that this implements version 1.0 of the spec, this sounds like it doesn't. Yeah, sorry about the confusion. Yes, the patch is actually for v0.8 but as you've correctly mentioned v0.8 has not so much differences to v1.0. > Instead of constantly removing and re-adding the code I was talking about only one removing the existing code and replacing it with current patches and then making updates on top of them. >Do you mind fixing that and addressing the other comments/concerns Sure пн, 13 мая 2024 г. в 14:32, Alistair Francis : > On Mon, May 13, 2024 at 9:14 PM Alistair Francis > wrote: > > > > On Mon, May 13, 2024 at 9:05 PM Alexey Baturo > wrote: > > > > > > Hi, > > > > > > > Hi, any change from v0.8 to v1.0? > > > Not in the patches that were sent. I'd still suggest applying a > step-by-step approach with cleaning up old code and establishing the new > mechanisms first and then updating the code to match the spec 100%. Also I > heard Martin has some arch compliance tests for J-ext somewhere. > > > > The cover letter says that this implements version 1.0 of the spec, > > this sounds like it doesn't. > > > > Also, it's better to make the changes on top of the current code. > > Instead of constantly removing and re-adding the code. Which is then > > hard to review. Especially as I'm guessing there isn't a huge > > difference between v0.8 and v1.0. > > > > > @Alistair Francis @liwei does this approach sound reasonable to you? > > > > > > >Also, this needs another rebase > > > Sure, no problem at all. I'll rebase and re-send them later today. > > Sorry, it did apply correctly! That was my mistake. > > But this series generates a warning. Do you mind fixing that and > addressing the other comments/concerns > > Alistair > > > > > Thanks. Can you be very clear which version of the spec you have > > developed and tested against as well. > > > > Alistair >
Re: [PATCH v2 10/11] vfio/cpr: Make vfio_cpr_register_container() return bool
On 5/7/24 08:42, Zhenzhong Duan wrote:
This is to follow the coding standand to return bool if 'Error **'
is used to pass error.
Suggested-by: Cédric Le Goater
Signed-off-by: Zhenzhong Duan
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
include/hw/vfio/vfio-common.h | 2 +-
hw/vfio/container.c | 3 +--
hw/vfio/cpr.c | 4 ++--
hw/vfio/iommufd.c | 3 +--
4 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index a7b6fc8f46..e4c60374fa 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -205,7 +205,7 @@ void vfio_detach_device(VFIODevice *vbasedev);
int vfio_kvm_device_add_fd(int fd, Error **errp);
int vfio_kvm_device_del_fd(int fd, Error **errp);
-int vfio_cpr_register_container(VFIOContainerBase *bcontainer, Error **errp);
+bool vfio_cpr_register_container(VFIOContainerBase *bcontainer, Error **errp);
void vfio_cpr_unregister_container(VFIOContainerBase *bcontainer);
extern const MemoryRegionOps vfio_region_ops;
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index b02583ea16..86266f3b83 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -616,8 +616,7 @@ static bool vfio_connect_container(VFIOGroup *group,
AddressSpace *as,
goto free_container_exit;
}
-ret = vfio_cpr_register_container(bcontainer, errp);
-if (ret) {
+if (!vfio_cpr_register_container(bcontainer, errp)) {
goto free_container_exit;
}
diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c
index 392c2dd95d..87e51fcee1 100644
--- a/hw/vfio/cpr.c
+++ b/hw/vfio/cpr.c
@@ -25,12 +25,12 @@ static int vfio_cpr_reboot_notifier(NotifierWithReturn
*notifier,
return 0;
}
-int vfio_cpr_register_container(VFIOContainerBase *bcontainer, Error **errp)
+bool vfio_cpr_register_container(VFIOContainerBase *bcontainer, Error **errp)
{
migration_add_notifier_mode(>cpr_reboot_notifier,
vfio_cpr_reboot_notifier,
MIG_MODE_CPR_REBOOT);
-return 0;
+return true;
}
void vfio_cpr_unregister_container(VFIOContainerBase *bcontainer)
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 84c86b970e..6a446b16dc 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -396,8 +396,7 @@ found_container:
goto err_listener_register;
}
-ret = vfio_cpr_register_container(bcontainer, errp);
-if (ret) {
+if (!vfio_cpr_register_container(bcontainer, errp)) {
goto err_listener_register;
}
Re: [PATCH v2 11/11] backends/iommufd: Make iommufd_backend_*() return bool
On 5/7/24 08:42, Zhenzhong Duan wrote:
This is to follow the coding standand to return bool if 'Error **'
is used to pass error.
The changed functions include:
iommufd_backend_connect
iommufd_backend_alloc_ioas
By this chance, simplify the functions a bit by avoiding duplicate
recordings, e.g., log through either error interface or trace, not
both.
Suggested-by: Cédric Le Goater
Signed-off-by: Zhenzhong Duan
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
include/sysemu/iommufd.h | 6 +++---
backends/iommufd.c | 29 +
hw/vfio/iommufd.c| 5 ++---
backends/trace-events| 4 ++--
4 files changed, 20 insertions(+), 24 deletions(-)
diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
index 9af27ebd6c..293bfbe967 100644
--- a/include/sysemu/iommufd.h
+++ b/include/sysemu/iommufd.h
@@ -23,11 +23,11 @@ struct IOMMUFDBackend {
/*< public >*/
};
-int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp);
+bool iommufd_backend_connect(IOMMUFDBackend *be, Error **errp);
void iommufd_backend_disconnect(IOMMUFDBackend *be);
-int iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
- Error **errp);
+bool iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
+Error **errp);
void iommufd_backend_free_id(IOMMUFDBackend *be, uint32_t id);
int iommufd_backend_map_dma(IOMMUFDBackend *be, uint32_t ioas_id, hwaddr iova,
ram_addr_t size, void *vaddr, bool readonly);
diff --git a/backends/iommufd.c b/backends/iommufd.c
index 76a0204852..c506afbdac 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -72,24 +72,22 @@ static void iommufd_backend_class_init(ObjectClass *oc,
void *data)
object_class_property_add_str(oc, "fd", NULL, iommufd_backend_set_fd);
}
-int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp)
+bool iommufd_backend_connect(IOMMUFDBackend *be, Error **errp)
{
-int fd, ret = 0;
+int fd;
if (be->owned && !be->users) {
fd = qemu_open_old("/dev/iommu", O_RDWR);
if (fd < 0) {
error_setg_errno(errp, errno, "/dev/iommu opening failed");
-ret = fd;
-goto out;
+return false;
}
be->fd = fd;
}
be->users++;
-out:
-trace_iommufd_backend_connect(be->fd, be->owned,
- be->users, ret);
-return ret;
+
+trace_iommufd_backend_connect(be->fd, be->owned, be->users);
+return true;
}
void iommufd_backend_disconnect(IOMMUFDBackend *be)
@@ -106,25 +104,24 @@ out:
trace_iommufd_backend_disconnect(be->fd, be->users);
}
-int iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
- Error **errp)
+bool iommufd_backend_alloc_ioas(IOMMUFDBackend *be, uint32_t *ioas_id,
+Error **errp)
{
-int ret, fd = be->fd;
+int fd = be->fd;
struct iommu_ioas_alloc alloc_data = {
.size = sizeof(alloc_data),
.flags = 0,
};
-ret = ioctl(fd, IOMMU_IOAS_ALLOC, _data);
-if (ret) {
+if (ioctl(fd, IOMMU_IOAS_ALLOC, _data)) {
error_setg_errno(errp, errno, "Failed to allocate ioas");
-return ret;
+return false;
}
*ioas_id = alloc_data.out_ioas_id;
-trace_iommufd_backend_alloc_ioas(fd, *ioas_id, ret);
+trace_iommufd_backend_alloc_ioas(fd, *ioas_id);
-return ret;
+return true;
}
void iommufd_backend_free_id(IOMMUFDBackend *be, uint32_t id)
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 6a446b16dc..554f9a6292 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -71,7 +71,7 @@ static bool iommufd_cdev_connect_and_bind(VFIODevice
*vbasedev, Error **errp)
.flags = 0,
};
-if (iommufd_backend_connect(iommufd, errp)) {
+if (!iommufd_backend_connect(iommufd, errp)) {
return false;
}
@@ -346,8 +346,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
}
/* Need to allocate a new dedicated container */
-ret = iommufd_backend_alloc_ioas(vbasedev->iommufd, _id, errp);
-if (ret < 0) {
+if (!iommufd_backend_alloc_ioas(vbasedev->iommufd, _id, errp)) {
goto err_alloc_ioas;
}
diff --git a/backends/trace-events b/backends/trace-events
index d45c6e31a6..211e6f374a 100644
--- a/backends/trace-events
+++ b/backends/trace-events
@@ -7,11 +7,11 @@ dbus_vmstate_loading(const char *id) "id: %s"
dbus_vmstate_saving(const char *id) "id: %s"
# iommufd.c
-iommufd_backend_connect(int fd, bool owned, uint32_t users, int ret) "fd=%d
owned=%d users=%d (%d)"
+iommufd_backend_connect(int fd, bool owned, uint32_t users) "fd=%d owned=%d
users=%d"
iommufd_backend_disconnect(int fd, uint32_t users) "fd=%d users=%d"
Re: [PATCH v2 09/11] vfio/iommufd: Make iommufd_cdev_*() return bool
On 5/7/24 08:42, Zhenzhong Duan wrote:
This is to follow the coding standand to return bool if 'Error **'
is used to pass error.
The changed functions include:
iommufd_cdev_kvm_device_add
iommufd_cdev_connect_and_bind
iommufd_cdev_attach_ioas_hwpt
iommufd_cdev_detach_ioas_hwpt
iommufd_cdev_attach_container
iommufd_cdev_get_info_iova_range
After the change, all functions in hw/vfio/iommufd.c follows the
standand.
Suggested-by: Cédric Le Goater
Signed-off-by: Zhenzhong Duan
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/vfio/iommufd.c | 88 +--
1 file changed, 39 insertions(+), 49 deletions(-)
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 4c6992fca1..84c86b970e 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -49,9 +49,9 @@ static int iommufd_cdev_unmap(const VFIOContainerBase
*bcontainer,
container->ioas_id, iova, size);
}
-static int iommufd_cdev_kvm_device_add(VFIODevice *vbasedev, Error **errp)
+static bool iommufd_cdev_kvm_device_add(VFIODevice *vbasedev, Error **errp)
{
-return vfio_kvm_device_add_fd(vbasedev->fd, errp);
+return !vfio_kvm_device_add_fd(vbasedev->fd, errp);
}
static void iommufd_cdev_kvm_device_del(VFIODevice *vbasedev)
@@ -63,18 +63,16 @@ static void iommufd_cdev_kvm_device_del(VFIODevice
*vbasedev)
}
}
-static int iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp)
+static bool iommufd_cdev_connect_and_bind(VFIODevice *vbasedev, Error **errp)
{
IOMMUFDBackend *iommufd = vbasedev->iommufd;
struct vfio_device_bind_iommufd bind = {
.argsz = sizeof(bind),
.flags = 0,
};
-int ret;
-ret = iommufd_backend_connect(iommufd, errp);
-if (ret) {
-return ret;
+if (iommufd_backend_connect(iommufd, errp)) {
+return false;
}
/*
@@ -82,15 +80,13 @@ static int iommufd_cdev_connect_and_bind(VFIODevice
*vbasedev, Error **errp)
* in KVM. Especially for some emulated devices, it requires
* to have kvm information in the device open.
*/
-ret = iommufd_cdev_kvm_device_add(vbasedev, errp);
-if (ret) {
+if (!iommufd_cdev_kvm_device_add(vbasedev, errp)) {
goto err_kvm_device_add;
}
/* Bind device to iommufd */
bind.iommufd = iommufd->fd;
-ret = ioctl(vbasedev->fd, VFIO_DEVICE_BIND_IOMMUFD, );
-if (ret) {
+if (ioctl(vbasedev->fd, VFIO_DEVICE_BIND_IOMMUFD, )) {
error_setg_errno(errp, errno, "error bind device fd=%d to iommufd=%d",
vbasedev->fd, bind.iommufd);
goto err_bind;
@@ -99,12 +95,12 @@ static int iommufd_cdev_connect_and_bind(VFIODevice
*vbasedev, Error **errp)
vbasedev->devid = bind.out_devid;
trace_iommufd_cdev_connect_and_bind(bind.iommufd, vbasedev->name,
vbasedev->fd, vbasedev->devid);
-return ret;
+return true;
err_bind:
iommufd_cdev_kvm_device_del(vbasedev);
err_kvm_device_add:
iommufd_backend_disconnect(iommufd);
-return ret;
+return false;
}
static void iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev)
@@ -176,10 +172,10 @@ out:
return ret;
}
-static int iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id,
+static bool iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id,
Error **errp)
{
-int ret, iommufd = vbasedev->iommufd->fd;
+int iommufd = vbasedev->iommufd->fd;
struct vfio_device_attach_iommufd_pt attach_data = {
.argsz = sizeof(attach_data),
.flags = 0,
@@ -187,38 +183,38 @@ static int iommufd_cdev_attach_ioas_hwpt(VFIODevice
*vbasedev, uint32_t id,
};
/* Attach device to an IOAS or hwpt within iommufd */
-ret = ioctl(vbasedev->fd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, _data);
-if (ret) {
+if (ioctl(vbasedev->fd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, _data)) {
error_setg_errno(errp, errno,
"[iommufd=%d] error attach %s (%d) to id=%d",
iommufd, vbasedev->name, vbasedev->fd, id);
-} else {
-trace_iommufd_cdev_attach_ioas_hwpt(iommufd, vbasedev->name,
-vbasedev->fd, id);
+return false;
}
-return ret;
+
+trace_iommufd_cdev_attach_ioas_hwpt(iommufd, vbasedev->name,
+vbasedev->fd, id);
+return true;
}
-static int iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp)
+static bool iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp)
{
-int ret, iommufd = vbasedev->iommufd->fd;
+int iommufd = vbasedev->iommufd->fd;
struct vfio_device_detach_iommufd_pt detach_data = {
.argsz = sizeof(detach_data),
.flags = 0,
};
-ret =
Re: [PATCH v2 08/11] vfio/container: Make vfio_get_device() return bool
On 5/7/24 08:42, Zhenzhong Duan wrote:
This is to follow the coding standand to return bool if 'Error **'
is used to pass error.
Suggested-by: Cédric Le Goater
Signed-off-by: Zhenzhong Duan
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/vfio/container.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 5fb4bee082..b02583ea16 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -800,8 +800,8 @@ static void vfio_put_group(VFIOGroup *group)
g_free(group);
}
-static int vfio_get_device(VFIOGroup *group, const char *name,
- VFIODevice *vbasedev, Error **errp)
+static bool vfio_get_device(VFIOGroup *group, const char *name,
+VFIODevice *vbasedev, Error **errp)
{
g_autofree struct vfio_device_info *info = NULL;
int fd;
@@ -813,14 +813,14 @@ static int vfio_get_device(VFIOGroup *group, const char
*name,
error_append_hint(errp,
"Verify all devices in group %d are bound to vfio-
"
"or pci-stub and not already in use\n", group->groupid);
-return fd;
+return false;
}
info = vfio_get_device_info(fd);
if (!info) {
error_setg_errno(errp, errno, "error getting device info");
close(fd);
-return -1;
+return false;
}
/*
@@ -835,7 +835,7 @@ static int vfio_get_device(VFIOGroup *group, const char
*name,
error_setg(errp, "Inconsistent setting of support for discarding "
"RAM (e.g., balloon) within group");
close(fd);
-return -1;
+return false;
}
if (!group->ram_block_discard_allowed) {
@@ -856,7 +856,7 @@ static int vfio_get_device(VFIOGroup *group, const char
*name,
vbasedev->reset_works = !!(info->flags & VFIO_DEVICE_FLAGS_RESET);
-return 0;
+return true;
}
static void vfio_put_base_device(VFIODevice *vbasedev)
@@ -909,7 +909,6 @@ static bool vfio_legacy_attach_device(const char *name,
VFIODevice *vbasedev,
VFIODevice *vbasedev_iter;
VFIOGroup *group;
VFIOContainerBase *bcontainer;
-int ret;
if (groupid < 0) {
return false;
@@ -929,8 +928,7 @@ static bool vfio_legacy_attach_device(const char *name,
VFIODevice *vbasedev,
return false;
}
}
-ret = vfio_get_device(group, name, vbasedev, errp);
-if (ret) {
+if (!vfio_get_device(group, name, vbasedev, errp)) {
vfio_put_group(group);
return false;
}
Re: [PATCH v2 06/11] vfio/container: Make vfio_connect_container() return bool
On 5/7/24 08:42, Zhenzhong Duan wrote:
This is to follow the coding standand to return bool if 'Error **'
is used to pass error.
Suggested-by: Cédric Le Goater
Signed-off-by: Zhenzhong Duan
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/vfio/container.c | 18 +++---
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 85a8a369dc..0a7edfcc43 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -534,8 +534,8 @@ static bool vfio_legacy_setup(VFIOContainerBase
*bcontainer, Error **errp)
return true;
}
-static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
- Error **errp)
+static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
+ Error **errp)
{
VFIOContainer *container;
VFIOContainerBase *bcontainer;
@@ -587,19 +587,18 @@ static int vfio_connect_container(VFIOGroup *group,
AddressSpace *as,
error_report("vfio: error disconnecting group %d from"
" container", group->groupid);
}
-return ret;
+return false;
}
group->container = container;
QLIST_INSERT_HEAD(>group_list, group, container_next);
vfio_kvm_device_add_group(group);
-return 0;
+return true;
}
}
fd = qemu_open_old("/dev/vfio/vfio", O_RDWR);
if (fd < 0) {
error_setg_errno(errp, errno, "failed to open /dev/vfio/vfio");
-ret = -errno;
goto put_space_exit;
}
@@ -607,7 +606,6 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
if (ret != VFIO_API_VERSION) {
error_setg(errp, "supported vfio version: %d, "
"reported version: %d", VFIO_API_VERSION, ret);
-ret = -EINVAL;
goto close_fd_exit;
}
@@ -634,7 +632,6 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
assert(bcontainer->ops->setup);
if (!bcontainer->ops->setup(bcontainer, errp)) {
-ret = -EINVAL;
goto enable_discards_exit;
}
@@ -650,7 +647,6 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
memory_listener_register(>listener, bcontainer->space->as);
if (bcontainer->error) {
-ret = -1;
error_propagate_prepend(errp, bcontainer->error,
"memory listener initialization failed: ");
goto listener_release_exit;
@@ -658,7 +654,7 @@ static int vfio_connect_container(VFIOGroup *group,
AddressSpace *as,
bcontainer->initialized = true;
-return 0;
+return true;
listener_release_exit:
QLIST_REMOVE(group, container_next);
QLIST_REMOVE(bcontainer, next);
@@ -683,7 +679,7 @@ close_fd_exit:
put_space_exit:
vfio_put_address_space(space);
-return ret;
+return false;
}
static void vfio_disconnect_container(VFIOGroup *group)
@@ -770,7 +766,7 @@ static VFIOGroup *vfio_get_group(int groupid, AddressSpace
*as, Error **errp)
group->groupid = groupid;
QLIST_INIT(>device_list);
-if (vfio_connect_container(group, as, errp)) {
+if (!vfio_connect_container(group, as, errp)) {
error_prepend(errp, "failed to setup container for group %d: ",
groupid);
goto close_fd_exit;
Re: [PATCH v2 07/11] vfio/container: Make vfio_set_iommu() return bool
On 5/7/24 08:42, Zhenzhong Duan wrote:
This is to follow the coding standand to return bool if 'Error **'
is used to pass error.
Suggested-by: Cédric Le Goater
Signed-off-by: Zhenzhong Duan
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/vfio/container.c | 22 ++
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 0a7edfcc43..5fb4bee082 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -391,21 +391,20 @@ static const VFIOIOMMUClass *vfio_get_iommu_class(int
iommu_type, Error **errp)
return VFIO_IOMMU_CLASS(klass);
}
-static int vfio_set_iommu(VFIOContainer *container, int group_fd,
- VFIOAddressSpace *space, Error **errp)
+static bool vfio_set_iommu(VFIOContainer *container, int group_fd,
+ VFIOAddressSpace *space, Error **errp)
{
-int iommu_type, ret;
+int iommu_type;
const VFIOIOMMUClass *vioc;
iommu_type = vfio_get_iommu_type(container, errp);
if (iommu_type < 0) {
-return iommu_type;
+return false;
}
-ret = ioctl(group_fd, VFIO_GROUP_SET_CONTAINER, >fd);
-if (ret) {
+if (ioctl(group_fd, VFIO_GROUP_SET_CONTAINER, >fd)) {
error_setg_errno(errp, errno, "Failed to set group container");
-return -errno;
+return false;
}
while (ioctl(container->fd, VFIO_SET_IOMMU, iommu_type)) {
@@ -420,7 +419,7 @@ static int vfio_set_iommu(VFIOContainer *container, int
group_fd,
continue;
}
error_setg_errno(errp, errno, "Failed to set iommu for container");
-return -errno;
+return false;
}
container->iommu_type = iommu_type;
@@ -428,11 +427,11 @@ static int vfio_set_iommu(VFIOContainer *container, int
group_fd,
vioc = vfio_get_iommu_class(iommu_type, errp);
if (!vioc) {
error_setg(errp, "No available IOMMU models");
-return -EINVAL;
+return false;
}
vfio_container_init(>bcontainer, space, vioc);
-return 0;
+return true;
}
static int vfio_get_iommu_info(VFIOContainer *container,
@@ -613,8 +612,7 @@ static bool vfio_connect_container(VFIOGroup *group,
AddressSpace *as,
container->fd = fd;
bcontainer = >bcontainer;
-ret = vfio_set_iommu(container, group->fd, space, errp);
-if (ret) {
+if (!vfio_set_iommu(container, group->fd, space, errp)) {
goto free_container_exit;
}
Re: [PATCH v2 18/45] target/hppa: Use displacements in DisasIAQE
* Richard Henderson : > This is a first step in enabling CF_PCREL, but for now > we regenerate the absolute address before writeback. > > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller
Re: [PATCH v2 17/45] target/hppa: Introduce and use DisasIAQE for branch management
* Richard Henderson : > Wrap offset and space together in one structure, ensuring > that they're copied together as required. > > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller
Re: [PATCH v2 02/11] vfio/pci: Use g_autofree in iommufd_cdev_get_info_iova_range()
On 5/7/24 08:42, Zhenzhong Duan wrote:
Local pointer info is freed before return from
iommufd_cdev_get_info_iova_range().
Use 'g_autofree' to avoid the g_free() calls.
Signed-off-by: Zhenzhong Duan
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/vfio/iommufd.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 8827ffe636..c644127972 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -258,7 +258,7 @@ static int
iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container,
uint32_t ioas_id, Error **errp)
{
VFIOContainerBase *bcontainer = >bcontainer;
-struct iommu_ioas_iova_ranges *info;
+g_autofree struct iommu_ioas_iova_ranges *info = NULL;
struct iommu_iova_range *iova_ranges;
int ret, sz, fd = container->be->fd;
@@ -291,12 +291,10 @@ static int iommufd_cdev_get_info_iova_range(VFIOIOMMUFDContainer *container,
}
bcontainer->pgsizes = info->out_iova_alignment;
-g_free(info);
return 0;
error:
ret = -errno;
-g_free(info);
error_setg_errno(errp, errno, "Cannot get IOVA ranges");
return ret;
}
Re: [PATCH v2 01/11] vfio/pci: Use g_autofree in vfio_realize
On 5/7/24 08:42, Zhenzhong Duan wrote:
Local pointer name is allocated before vfio_attach_device() call
and freed after the call.
Same for tmp when calling realpath().
Use 'g_autofree' to avoid the g_free() calls.
Signed-off-by: Zhenzhong Duan
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/vfio/pci.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 64780d1b79..576b21e2bb 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2946,12 +2946,13 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
ERRP_GUARD();
VFIOPCIDevice *vdev = VFIO_PCI(pdev);
VFIODevice *vbasedev = >vbasedev;
-char *tmp, *subsys;
+char *subsys;
Error *err = NULL;
int i, ret;
bool is_mdev;
char uuid[UUID_STR_LEN];
-char *name;
+g_autofree char *name = NULL;
+g_autofree char *tmp = NULL;
if (vbasedev->fd < 0 && !vbasedev->sysfsdev) {
if (!(~vdev->host.domain || ~vdev->host.bus ||
@@ -2982,7 +2983,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
*/
tmp = g_strdup_printf("%s/subsystem", vbasedev->sysfsdev);
subsys = realpath(tmp, NULL);
-g_free(tmp);
is_mdev = subsys && (strcmp(subsys, "/sys/bus/mdev") == 0);
free(subsys);
@@ -3003,7 +3003,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
ret = vfio_attach_device(name, vbasedev,
pci_device_iommu_address_space(pdev), errp);
-g_free(name);
if (ret) {
goto error;
}
Re: [PATCH v2 08/45] target/hppa: Add install_link
On 5/14/24 16:37, Helge Deller wrote:
* Richard Henderson :
Add a common routine for writing the return address.
Signed-off-by: Richard Henderson
Reviewed-by: Helge Deller
---
target/hppa/translate.c | 54 +++--
1 file changed, 31 insertions(+), 23 deletions(-)
diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 08d5e2a4bc..f816b337ee 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -634,6 +634,23 @@ static void install_iaq_entries(DisasContext *ctx,
uint64_t bi, TCGv_i64 bv,
}
}
+static void install_link(DisasContext *ctx, unsigned link, bool with_sr0)
+{
+tcg_debug_assert(ctx->null_cond.c == TCG_COND_NEVER);
+if (link) {
Just wondering:
Doesn't it makes it easier to write here:
if (!link) {
return;
}
and then don't indent the few following lines?
I see you change and partly revert it again in patch #17...
so forget this remark for now.
Helge
Re: [PATCH v2 14/45] target/hppa: Add space argument to do_ibranch
* Richard Henderson : > This allows unification of BE, BLR, BV, BVE with a common helper. > Since we can now track space with IAQ_Next, we can now let the > TranslationBlock continue across the delay slot with BE, BVE. > > Signed-off-by: Richard Henderson Reviewed-by: Helge Deller
[PATCH v6 7/9] memory: Add Error** argument to memory_get_xlat_addr()
Let the callers do the reporting. This will be useful in
vfio_iommu_map_dirty_notify().
Cc: Michael S. Tsirkin
Cc: Paolo Bonzini
Cc: David Hildenbrand
Reviewed-by: Peter Xu
Signed-off-by: Cédric Le Goater
---
Changes in v6:
- Fixed memory_get_xlat_addr documentation (Avihai)
include/exec/memory.h | 15 ++-
hw/vfio/common.c | 13 +
hw/virtio/vhost-vdpa.c | 5 -
system/memory.c| 10 +-
4 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index
d417d7f363dbbca6553c449582a93d5da73cca40..9cdd64e9c69b63f9d27cebc2e8cb366e22ed7577
100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -774,9 +774,22 @@ void
ram_discard_manager_register_listener(RamDiscardManager *rdm,
void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
RamDiscardListener *rdl);
+/**
+ * memory_get_xlat_addr: Extract addresses from a TLB entry
+ *
+ * @iotlb: pointer to an #IOMMUTLBEntry
+ * @vaddr: virtual address
+ * @ram_addr: RAM address
+ * @read_only: indicates if writes are allowed
+ * @mr_has_discard_manager: indicates memory is controlled by a
+ * RamDiscardManager
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: true on success, else false setting @errp with error.
+ */
bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
ram_addr_t *ram_addr, bool *read_only,
- bool *mr_has_discard_manager);
+ bool *mr_has_discard_manager, Error **errp);
typedef struct CoalescedMemoryRange CoalescedMemoryRange;
typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index
4e2ef3d3034e72aa6a546bcb9ea1f31a0bbd5b1b..919c4c52bc1590fd6c0bda37ba5881f58ff2
100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -253,12 +253,13 @@ static bool
vfio_listener_skipped_section(MemoryRegionSection *section)
/* Called with rcu_read_lock held. */
static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
- ram_addr_t *ram_addr, bool *read_only)
+ ram_addr_t *ram_addr, bool *read_only,
+ Error **errp)
{
bool ret, mr_has_discard_manager;
ret = memory_get_xlat_addr(iotlb, vaddr, ram_addr, read_only,
- _has_discard_manager);
+ _has_discard_manager, errp);
if (ret && mr_has_discard_manager) {
/*
* Malicious VMs might trigger discarding of IOMMU-mapped memory. The
@@ -288,6 +289,7 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n,
IOMMUTLBEntry *iotlb)
hwaddr iova = iotlb->iova + giommu->iommu_offset;
void *vaddr;
int ret;
+Error *local_err = NULL;
trace_vfio_iommu_map_notify(iotlb->perm == IOMMU_NONE ? "UNMAP" : "MAP",
iova, iova + iotlb->addr_mask);
@@ -304,7 +306,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n,
IOMMUTLBEntry *iotlb)
if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
bool read_only;
-if (!vfio_get_xlat_addr(iotlb, , NULL, _only)) {
+if (!vfio_get_xlat_addr(iotlb, , NULL, _only, _err)) {
+error_report_err(local_err);
goto out;
}
/*
@@ -1213,6 +1216,7 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n,
IOMMUTLBEntry *iotlb)
VFIOContainerBase *bcontainer = giommu->bcontainer;
hwaddr iova = iotlb->iova + giommu->iommu_offset;
ram_addr_t translated_addr;
+Error *local_err = NULL;
int ret = -EINVAL;
trace_vfio_iommu_map_dirty_notify(iova, iova + iotlb->addr_mask);
@@ -1224,7 +1228,8 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n,
IOMMUTLBEntry *iotlb)
}
rcu_read_lock();
-if (!vfio_get_xlat_addr(iotlb, NULL, _addr, NULL)) {
+if (!vfio_get_xlat_addr(iotlb, NULL, _addr, NULL, _err)) {
+error_report_err(local_err);
goto out_unlock;
}
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index
e827b9175fc61f1ef419e48d90a440b00449312a..ed99ab87457d8f31b98ace960713f48d47b27102
100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -208,6 +208,7 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n,
IOMMUTLBEntry *iotlb)
void *vaddr;
int ret;
Int128 llend;
+Error *local_err = NULL;
if (iotlb->target_as != _space_memory) {
error_report("Wrong target AS \"%s\", only system memory is allowed",
@@ -227,7 +228,9 @@ static void vhost_vdpa_iommu_map_notify(IOMMUNotifier *n,
IOMMUTLBEntry *iotlb)
if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
bool read_only;
-if (!memory_get_xlat_addr(iotlb, , NULL, _only, NULL)) {
+if (!memory_get_xlat_addr(iotlb,
[PATCH v6 0/9] vfio: Improve error reporting (part 2
Hello,
The motivation behind these changes is to improve error reporting to
the upper management layer (libvirt) with a more detailed error, this
to let it decide, depending on the reported error, whether to try
migration again later. It would be useful in cases where migration
fails due to lack of HW resources on the host. For instance, some
adapters can only initiate a limited number of simultaneous dirty
tracking requests and this imposes a limit on the the number of VMs
that can be migrated simultaneously.
We are not quite ready for such a mechanism but what we can do first is
to cleanup the error reporting in the early save_setup sequence. This
is what the following changes propose, by adding an Error** argument to
various handlers and propagating it to the core migration subsystem.
The first part [1] of this series modifying the core migration
subsystem is now merged. This is the second part changing VFIO which
was already proposed in March. See [2].
Thanks,
C.
[1] [PATCH for-9.1 v5 00/14] migration: Improve error reporting
https://lore.kernel.org/qemu-devel/20240320064911.545001-1-...@redhat.com/
[2] [PATCH v4 00/25] migration: Improve error reporting
https://lore.kernel.org/qemu-devel/20240306133441.2351700-1-...@redhat.com/
Changes in v6:
- Commit log improvements (Avihai)
- Modified some titles (Avihai)
- vfio_migration_set_state() : Dropped the error_setg_errno()
change when setting device in recover state fails (Avihai)
- vfio_migration_state_notifier() : report local error (Avihai)
- vfio_save_device_config_state() : Set errp if the migration
stream is in error (Avihai)
- vfio_save_state() : Changed error prefix (Avihai)
- vfio_iommu_map_dirty_notify() : Modified goto label (Avihai)
- Fixed memory_get_xlat_addr documentation (Avihai)
- Fixed line wrapping (Avihai)
- Fixed query_dirty_bitmap documentation (Avihai)
- Dropped last patch from v5 :
vfio: Extend vfio_set_migration_error() with Error* argument
Changes in v5:
- Rebased on 20c64c8a51a4 ("migration: migration_file_set_error")
- Fixed typo in set_dirty_page_tracking documentation
- Used error_setg_errno() in vfio_devices_dma_logging_start()
- Replaced error_setg() by error_setg_errno() in vfio_migration_set_state()
- Replaced error_setg() by error_setg_errno() in
vfio_devices_query_dirty_bitmap() and vfio_legacy_query_dirty_bitmap()
- ':' -> '-' in vfio_iommu_map_dirty_notify()
Cédric Le Goater (9):
vfio: Add Error** argument to .set_dirty_page_tracking() handler
vfio: Add Error** argument to vfio_devices_dma_logging_start()
migration: Extend migration_file_set_error() with Error* argument
vfio/migration: Add an Error** argument to vfio_migration_set_state()
vfio/migration: Add Error** argument to .vfio_save_config() handler
vfio: Reverse test on vfio_get_xlat_addr()
memory: Add Error** argument to memory_get_xlat_addr()
vfio: Add Error** argument to .get_dirty_bitmap() handler
vfio: Also trace event failures in vfio_save_complete_precopy()
include/exec/memory.h | 15 +++-
include/hw/vfio/vfio-common.h | 29 ++-
include/hw/vfio/vfio-container-base.h | 39 +++--
include/migration/misc.h | 2 +-
hw/vfio/common.c | 112 +-
hw/vfio/container-base.c | 11 +--
hw/vfio/container.c | 20 +++--
hw/vfio/migration.c | 105 +++-
hw/vfio/pci.c | 5 +-
hw/virtio/vhost-vdpa.c| 5 +-
migration/migration.c | 6 +-
system/memory.c | 10 +--
12 files changed, 246 insertions(+), 113 deletions(-)
--
2.45.0
[PATCH v6 9/9] vfio: Also trace event failures in vfio_save_complete_precopy()
vfio_save_complete_precopy() currently returns before doing the trace
event. Change that.
Reviewed-by: Avihai Horon
Signed-off-by: Cédric Le Goater
---
hw/vfio/migration.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index
d089fa9b937e725307c1a56755495d5b8fae2065..b06d887df53155e676bf13fa03adaf0627841994
100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -585,9 +585,6 @@ static int vfio_save_complete_precopy(QEMUFile *f, void
*opaque)
qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
ret = qemu_file_get_error(f);
-if (ret) {
-return ret;
-}
trace_vfio_save_complete_precopy(vbasedev->name, ret);
--
2.45.0
Re: CPR/liveupdate: test results using prior bug fix
14.05.2024 16:54, Michael Tokarev пишет: On 5/14/24 16:39, Michael Galaxy wrote: Steve, OK, so it does not look like this bugfix you wrote was included in 8.2.4 (which was released yesterday). Unfortunately, that means that anyone using CPR in that release will still (eventually) encounter the bug like I did. 8.2.4 is basically a "bugfix" release for 8.2.3 which I somewhat screwed up (in a minor way), plus a few currently (at the time) queued up changes. 8.2.3 was a big release though. I would recommend that y'all consider cherry-picking, perhaps, the relevant commits for a possible 8.2.5 ? Please Cc changes which are relevant for -stable to, well, qemu-sta...@nongnu.org :) Which changes needs to be picked up? Please also note this particular change does not apply cleanly to stable-8.2 branch due to other changes in this area between 8.2 and 9.0, in particular, in postcopy_start(). So if this is to be picked up for stable-8.2, I need help from someone who better understands this code to select changes to pick. Thanks, /mjt -- GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24. New key: rsa4096/61AD3D98ECDF2C8E 9D8B E14E 3F2A 9DD7 9199 28F1 61AD 3D98 ECDF 2C8E Old key: rsa2048/457CE0A0804465C5 6EE1 95D1 886E 8FFB 810D 4324 457C E0A0 8044 65C5 Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt
[PATCH v6 4/9] vfio/migration: Add an Error** argument to vfio_migration_set_state()
Add an Error** argument to vfio_migration_set_state() and adjust
callers, including vfio_save_setup(). The error will be propagated up
to qemu_savevm_state_setup() where the save_setup() handler is
executed.
Modify vfio_vmstate_change_prepare() and vfio_vmstate_change() to
store a reported error under the migration stream if a migration is in
progress.
Signed-off-by: Cédric Le Goater
---
Changes in v6:
- Commit log improvements (Avihai)
- vfio_migration_set_state() : Dropped the error_setg_errno() change
when setting device in recover state fails (Avihai)
- vfio_migration_state_notifier() : report local error (Avihai)
Changes in v5:
- Replaced error_setg() by error_setg_errno() in vfio_migration_set_state()
- Rebased on 20c64c8a51a4 ("migration: migration_file_set_error")
hw/vfio/migration.c | 77 -
1 file changed, 48 insertions(+), 29 deletions(-)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index
bf2fd0759ba6e4fb103cc5c1a43edb180a3d0de4..bf11135167ebb162dd41415656bdacfa0e1ca550
100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -82,7 +82,8 @@ static const char *mig_state_to_str(enum
vfio_device_mig_state state)
static int vfio_migration_set_state(VFIODevice *vbasedev,
enum vfio_device_mig_state new_state,
-enum vfio_device_mig_state recover_state)
+enum vfio_device_mig_state recover_state,
+Error **errp)
{
VFIOMigration *migration = vbasedev->migration;
uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
@@ -102,18 +103,19 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
ret = -errno;
if (recover_state == VFIO_DEVICE_STATE_ERROR) {
-error_report("%s: Failed setting device state to %s, err: %s. "
- "Recover state is ERROR. Resetting device",
- vbasedev->name, mig_state_to_str(new_state),
- strerror(errno));
+error_setg_errno(errp, errno,
+ "%s: Failed setting device state to %s. "
+ "Recover state is ERROR. Resetting device",
+ vbasedev->name, mig_state_to_str(new_state));
goto reset_device;
}
-error_report(
-"%s: Failed setting device state to %s, err: %s. Setting device in
recover state %s",
- vbasedev->name, mig_state_to_str(new_state),
- strerror(errno), mig_state_to_str(recover_state));
+error_setg_errno(errp, errno,
+ "%s: Failed setting device state to %s. "
+ "Setting device in recover state %s",
+ vbasedev->name, mig_state_to_str(new_state),
+ mig_state_to_str(recover_state));
mig_state->device_state = recover_state;
if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
@@ -137,7 +139,7 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
* This can happen if the device is asynchronously reset and
* terminates a data transfer.
*/
-error_report("%s: data_fd out of sync", vbasedev->name);
+error_setg(errp, "%s: data_fd out of sync", vbasedev->name);
close(mig_state->data_fd);
return -EBADF;
@@ -168,10 +170,11 @@ reset_device:
*/
static int
vfio_migration_set_state_or_reset(VFIODevice *vbasedev,
- enum vfio_device_mig_state new_state)
+ enum vfio_device_mig_state new_state,
+ Error **errp)
{
return vfio_migration_set_state(vbasedev, new_state,
-VFIO_DEVICE_STATE_ERROR);
+VFIO_DEVICE_STATE_ERROR, errp);
}
static int vfio_load_buffer(QEMUFile *f, VFIODevice *vbasedev,
@@ -399,10 +402,8 @@ static int vfio_save_setup(QEMUFile *f, void *opaque,
Error **errp)
switch (migration->device_state) {
case VFIO_DEVICE_STATE_RUNNING:
ret = vfio_migration_set_state(vbasedev,
VFIO_DEVICE_STATE_PRE_COPY,
- VFIO_DEVICE_STATE_RUNNING);
+ VFIO_DEVICE_STATE_RUNNING, errp);
if (ret) {
-error_setg(errp, "%s: Failed to set new PRE_COPY state",
- vbasedev->name);
return ret;
}
@@ -435,13 +436,20 @@ static void vfio_save_cleanup(void *opaque)
{
VFIODevice *vbasedev = opaque;
VFIOMigration *migration = vbasedev->migration;
+Error *local_err = NULL;
+int ret;
/*
* Changing device state from STOP_COPY to STOP can take time. Do it here,
[PATCH v6 2/9] vfio: Add Error** argument to vfio_devices_dma_logging_start()
This allows to update the Error argument of the VFIO log_global_start()
handler. Errors for container based logging will also be propagated to
qemu_savevm_state_setup() when the ram save_setup() handler is executed.
The vfio_set_migration_error() call becomes redundant in
vfio_listener_log_global_start(). Remove it.
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Avihai Horon
Signed-off-by: Cédric Le Goater
---
Changes in v6:
- Commit log improvements (Avihai)
Changes in v5:
- Used error_setg_errno() in vfio_devices_dma_logging_start()
hw/vfio/common.c | 26 +++---
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index
485e53916491f1164d29e739fb7106c0c77df737..b5102f54a6474a50c6366e8fbce23812d55e384e
100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1027,7 +1027,8 @@ static void vfio_device_feature_dma_logging_start_destroy(
g_free(feature);
}
-static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
+static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
+ Error **errp)
{
struct vfio_device_feature *feature;
VFIODirtyRanges ranges;
@@ -1038,6 +1039,7 @@ static int
vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
feature = vfio_device_feature_dma_logging_start_create(bcontainer,
);
if (!feature) {
+error_setg_errno(errp, errno, "Failed to prepare DMA logging");
return -errno;
}
@@ -1049,8 +1051,8 @@ static int
vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer)
ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature);
if (ret) {
ret = -errno;
-error_report("%s: Failed to start DMA logging, err %d (%s)",
- vbasedev->name, ret, strerror(errno));
+error_setg_errno(errp, errno, "%s: Failed to start DMA logging",
+ vbasedev->name);
goto out;
}
vbasedev->dirty_tracking = true;
@@ -1069,20 +1071,19 @@ out:
static bool vfio_listener_log_global_start(MemoryListener *listener,
Error **errp)
{
+ERRP_GUARD();
VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
listener);
int ret;
if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
-ret = vfio_devices_dma_logging_start(bcontainer);
+ret = vfio_devices_dma_logging_start(bcontainer, errp);
} else {
-ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
+ret = vfio_container_set_dirty_page_tracking(bcontainer, true, errp);
}
if (ret) {
-error_report("vfio: Could not start dirty page tracking, err: %d (%s)",
- ret, strerror(-ret));
-vfio_set_migration_error(ret);
+error_prepend(errp, "vfio: Could not start dirty page tracking - ");
}
return !ret;
}
@@ -1091,17 +1092,20 @@ static void
vfio_listener_log_global_stop(MemoryListener *listener)
{
VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
listener);
+Error *local_err = NULL;
int ret = 0;
if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
vfio_devices_dma_logging_stop(bcontainer);
} else {
-ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
+ret = vfio_container_set_dirty_page_tracking(bcontainer, false,
+ _err);
}
if (ret) {
-error_report("vfio: Could not stop dirty page tracking, err: %d (%s)",
- ret, strerror(-ret));
+error_prepend(_err,
+ "vfio: Could not stop dirty page tracking - ");
+error_report_err(local_err);
vfio_set_migration_error(ret);
}
}
--
2.45.0
[PATCH v6 8/9] vfio: Add Error** argument to .get_dirty_bitmap() handler
Let the callers do the error reporting. Add documentation while at it.
Signed-off-by: Cédric Le Goater
---
Changes in v6:
- Fixed the line wrapping (Avihai)
- Fixed query_dirty_bitmap documentation (Avihai)
Changes in v5:
- Replaced error_setg() by error_setg_errno() in
vfio_devices_query_dirty_bitmap() and vfio_legacy_query_dirty_bitmap()
- ':' -> '-' in vfio_iommu_map_dirty_notify()
include/hw/vfio/vfio-common.h | 4 +-
include/hw/vfio/vfio-container-base.h | 21 +++--
hw/vfio/common.c | 61 ++-
hw/vfio/container-base.c | 7 +--
hw/vfio/container.c | 14 +++---
5 files changed, 72 insertions(+), 35 deletions(-)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index
46f88493634b5634a9c14a5caa33a463fbf2c50d..68911d36676667352e94a97895828aff4b194b57
100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -274,9 +274,9 @@ bool
vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer);
int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
VFIOBitmap *vbmap, hwaddr iova,
-hwaddr size);
+hwaddr size, Error **errp);
int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova,
- uint64_t size, ram_addr_t ram_addr);
+ uint64_t size, ram_addr_t ram_addr, Error **errp);
/* Returns 0 on success, or a negative errno. */
int vfio_device_get_name(VFIODevice *vbasedev, Error **errp);
diff --git a/include/hw/vfio/vfio-container-base.h
b/include/hw/vfio/vfio-container-base.h
index
326ceea52a2030eec9dad289a9845866c4a8c090..42cfbf32dc737606c3f620d126f35d85d4833534
100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -84,8 +84,8 @@ void vfio_container_del_section_window(VFIOContainerBase
*bcontainer,
int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
bool start, Error **errp);
int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
- VFIOBitmap *vbmap,
- hwaddr iova, hwaddr size);
+ VFIOBitmap *vbmap, hwaddr iova,
+ hwaddr size, Error **errp);
void vfio_container_init(VFIOContainerBase *bcontainer,
VFIOAddressSpace *space,
@@ -138,9 +138,22 @@ struct VFIOIOMMUClass {
*/
int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
bool start, Error **errp);
+/**
+ * @query_dirty_bitmap
+ *
+ * Get bitmap of dirty pages from container
+ *
+ * @bcontainer: #VFIOContainerBase from which to get dirty pages
+ * @vbmap: #VFIOBitmap internal bitmap structure
+ * @iova: iova base address
+ * @size: size of iova range
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Returns zero to indicate success and negative for error
+ */
int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
- VFIOBitmap *vbmap,
- hwaddr iova, hwaddr size);
+ VFIOBitmap *vbmap, hwaddr iova, hwaddr size,
+ Error **errp);
/* PCI specific */
int (*pci_hot_reset)(VFIODevice *vbasedev, bool single);
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index
919c4c52bc1590fd6c0bda37ba5881f58ff2..9b5123d45fc1f9d5be4bbbf92898f89cd11e1363
100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1140,8 +1140,8 @@ static int vfio_device_dma_logging_report(VFIODevice
*vbasedev, hwaddr iova,
}
int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
-VFIOBitmap *vbmap, hwaddr iova,
-hwaddr size)
+VFIOBitmap *vbmap, hwaddr iova, hwaddr
size,
+Error **errp)
{
VFIODevice *vbasedev;
int ret;
@@ -1150,10 +1150,10 @@ int vfio_devices_query_dirty_bitmap(const
VFIOContainerBase *bcontainer,
ret = vfio_device_dma_logging_report(vbasedev, iova, size,
vbmap->bitmap);
if (ret) {
-error_report("%s: Failed to get DMA logging report, iova: "
- "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx
- ", err: %d (%s)",
- vbasedev->name, iova, size, ret, strerror(-ret));
+error_setg_errno(errp, -ret,
+ "%s: Failed to get DMA logging report, iova: "
+ "0x%"
[PATCH v6 6/9] vfio: Reverse test on vfio_get_xlat_addr()
It will simplify the changes coming after.
Reviewed-by: Avihai Horon
Signed-off-by: Cédric Le Goater
---
Changes in v6:
- Modified title (Avihai)
- vfio_iommu_map_dirty_notify() : Modified goto label (Avihai)
hw/vfio/common.c | 22 +-
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index
ed5ee6349ced78b3bde68d2ee506f78ba1a9dd9c..4e2ef3d3034e72aa6a546bcb9ea1f31a0bbd5b1b
100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1224,16 +1224,20 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier
*n, IOMMUTLBEntry *iotlb)
}
rcu_read_lock();
-if (vfio_get_xlat_addr(iotlb, NULL, _addr, NULL)) {
-ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
-translated_addr);
-if (ret) {
-error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
- "0x%"HWADDR_PRIx") = %d (%s)",
- bcontainer, iova, iotlb->addr_mask + 1, ret,
- strerror(-ret));
-}
+if (!vfio_get_xlat_addr(iotlb, NULL, _addr, NULL)) {
+goto out_unlock;
}
+
+ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1,
+translated_addr);
+if (ret) {
+error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", "
+ "0x%"HWADDR_PRIx") = %d (%s)",
+ bcontainer, iova, iotlb->addr_mask + 1, ret,
+ strerror(-ret));
+}
+
+out_unlock:
rcu_read_unlock();
out:
--
2.45.0
[PATCH v6 3/9] migration: Extend migration_file_set_error() with Error* argument
Use it to update the current error of the migration stream if
available and if not, simply print out the error. Next changes will
update with an error to report.
Reviewed-by: Avihai Horon
Acked-by: Fabiano Rosas
Signed-off-by: Cédric Le Goater
---
Changes in v6:
- Commit log improvements (Avihai)
include/migration/misc.h | 2 +-
hw/vfio/common.c | 2 +-
hw/vfio/migration.c | 4 ++--
migration/migration.c| 6 --
4 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/include/migration/misc.h b/include/migration/misc.h
index
bf7339cc1e6430226127fb6a878d06b458170858..bfadc5613bac614a316e5aed7da95d8c7845cf42
100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -97,7 +97,7 @@ void migration_add_notifier_mode(NotifierWithReturn *notify,
void migration_remove_notifier(NotifierWithReturn *notify);
bool migration_is_running(void);
-void migration_file_set_error(int err);
+void migration_file_set_error(int ret, Error *err);
/* True if incoming migration entered POSTCOPY_INCOMING_DISCARD */
bool migration_in_incoming_postcopy(void);
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index
b5102f54a6474a50c6366e8fbce23812d55e384e..ed5ee6349ced78b3bde68d2ee506f78ba1a9dd9c
100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -150,7 +150,7 @@ bool vfio_viommu_preset(VFIODevice *vbasedev)
static void vfio_set_migration_error(int err)
{
if (migration_is_setup_or_active()) {
-migration_file_set_error(err);
+migration_file_set_error(err, NULL);
}
}
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index
06ae40969b6c19037e190008e14f28be646278cd..bf2fd0759ba6e4fb103cc5c1a43edb180a3d0de4
100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -726,7 +726,7 @@ static void vfio_vmstate_change_prepare(void *opaque, bool
running,
* Migration should be aborted in this case, but vm_state_notify()
* currently does not support reporting failures.
*/
-migration_file_set_error(ret);
+migration_file_set_error(ret, NULL);
}
trace_vfio_vmstate_change_prepare(vbasedev->name, running,
@@ -756,7 +756,7 @@ static void vfio_vmstate_change(void *opaque, bool running,
RunState state)
* Migration should be aborted in this case, but vm_state_notify()
* currently does not support reporting failures.
*/
-migration_file_set_error(ret);
+migration_file_set_error(ret, NULL);
}
trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
diff --git a/migration/migration.c b/migration/migration.c
index
e88b24f1e6cbe82dad3f890c00e264d2ab6ad355..70d66a441bf04761decf91dbe57ce52c57fde58f
100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2994,13 +2994,15 @@ static MigThrError postcopy_pause(MigrationState *s)
}
}
-void migration_file_set_error(int err)
+void migration_file_set_error(int ret, Error *err)
{
MigrationState *s = current_migration;
WITH_QEMU_LOCK_GUARD(>qemu_file_lock) {
if (s->to_dst_file) {
-qemu_file_set_error(s->to_dst_file, err);
+qemu_file_set_error_obj(s->to_dst_file, ret, err);
+} else if (err) {
+error_report_err(err);
}
}
}
--
2.45.0
[PATCH v6 1/9] vfio: Add Error** argument to .set_dirty_page_tracking() handler
We will use the Error object to improve error reporting in the
.log_global*() handlers of VFIO. Add documentation while at it.
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Avihai Horon
Signed-off-by: Cédric Le Goater
---
Changes in v5:
- Fixed typo in set_dirty_page_tracking documentation
include/hw/vfio/vfio-container-base.h | 18 --
hw/vfio/common.c | 4 ++--
hw/vfio/container-base.c | 4 ++--
hw/vfio/container.c | 6 +++---
4 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/include/hw/vfio/vfio-container-base.h
b/include/hw/vfio/vfio-container-base.h
index
3582d5f97a37877b2adfc0d0b06996c82403f8b7..326ceea52a2030eec9dad289a9845866c4a8c090
100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -82,7 +82,7 @@ int vfio_container_add_section_window(VFIOContainerBase
*bcontainer,
void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
MemoryRegionSection *section);
int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
- bool start);
+ bool start, Error **errp);
int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
VFIOBitmap *vbmap,
hwaddr iova, hwaddr size);
@@ -121,9 +121,23 @@ struct VFIOIOMMUClass {
int (*attach_device)(const char *name, VFIODevice *vbasedev,
AddressSpace *as, Error **errp);
void (*detach_device)(VFIODevice *vbasedev);
+
/* migration feature */
+
+/**
+ * @set_dirty_page_tracking
+ *
+ * Start or stop dirty pages tracking on VFIO container
+ *
+ * @bcontainer: #VFIOContainerBase on which to de/activate dirty
+ * page tracking
+ * @start: indicates whether to start or stop dirty pages tracking
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Returns zero to indicate success and negative for error
+ */
int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
- bool start);
+ bool start, Error **errp);
int (*query_dirty_bitmap)(const VFIOContainerBase *bcontainer,
VFIOBitmap *vbmap,
hwaddr iova, hwaddr size);
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index
8f9cbdc0264044ce587877a7d19d14b28527291b..485e53916491f1164d29e739fb7106c0c77df737
100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1076,7 +1076,7 @@ static bool vfio_listener_log_global_start(MemoryListener
*listener,
if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
ret = vfio_devices_dma_logging_start(bcontainer);
} else {
-ret = vfio_container_set_dirty_page_tracking(bcontainer, true);
+ret = vfio_container_set_dirty_page_tracking(bcontainer, true, NULL);
}
if (ret) {
@@ -1096,7 +1096,7 @@ static void vfio_listener_log_global_stop(MemoryListener
*listener)
if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
vfio_devices_dma_logging_stop(bcontainer);
} else {
-ret = vfio_container_set_dirty_page_tracking(bcontainer, false);
+ret = vfio_container_set_dirty_page_tracking(bcontainer, false, NULL);
}
if (ret) {
diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
index
913ae49077c4f09b7b27517c1231cfbe4befb7fb..7c0764121d24b02b6c4e66e368d7dff78a6d65aa
100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -53,14 +53,14 @@ void vfio_container_del_section_window(VFIOContainerBase
*bcontainer,
}
int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
- bool start)
+ bool start, Error **errp)
{
if (!bcontainer->dirty_pages_supported) {
return 0;
}
g_assert(bcontainer->ops->set_dirty_page_tracking);
-return bcontainer->ops->set_dirty_page_tracking(bcontainer, start);
+return bcontainer->ops->set_dirty_page_tracking(bcontainer, start, errp);
}
int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index
77bdec276ec49cb9cd767c0de42ec801b4421572..c35221fbe7dc5453050f97cd186fc958e24f28f7
100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -209,7 +209,7 @@ static int vfio_legacy_dma_map(const VFIOContainerBase
*bcontainer, hwaddr iova,
static int
vfio_legacy_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
-bool start)
+bool start, Error **errp)
{
const VFIOContainer *container = container_of(bcontainer,
[PATCH v6 5/9] vfio/migration: Add Error** argument to .vfio_save_config() handler
Use vmstate_save_state_with_err() to improve error reporting in the
callers and store a reported error under the migration stream. Add
documentation while at it.
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Cédric Le Goater
---
Changes in v6:
- Modified title (Avihai)
- vfio_save_device_config_state() : Set errp if the migration stream
is in error (Avihai)
- vfio_save_state() : Changed error prefix (Avihai)
include/hw/vfio/vfio-common.h | 25 -
hw/vfio/migration.c | 25 ++---
hw/vfio/pci.c | 5 +++--
3 files changed, 45 insertions(+), 10 deletions(-)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index
b9da6c08ef41174610eb92726c590309a53696a3..46f88493634b5634a9c14a5caa33a463fbf2c50d
100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -133,7 +133,30 @@ struct VFIODeviceOps {
int (*vfio_hot_reset_multi)(VFIODevice *vdev);
void (*vfio_eoi)(VFIODevice *vdev);
Object *(*vfio_get_object)(VFIODevice *vdev);
-void (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f);
+
+/**
+ * @vfio_save_config
+ *
+ * Save device config state
+ *
+ * @vdev: #VFIODevice for which to save the config
+ * @f: #QEMUFile where to send the data
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Returns zero to indicate success and negative for error
+ */
+int (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f, Error **errp);
+
+/**
+ * @vfio_load_config
+ *
+ * Load device config state
+ *
+ * @vdev: #VFIODevice for which to load the config
+ * @f: #QEMUFile where to get the data
+ *
+ * Returns zero to indicate success and negative for error
+ */
int (*vfio_load_config)(VFIODevice *vdev, QEMUFile *f);
};
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index
bf11135167ebb162dd41415656bdacfa0e1ca550..d089fa9b937e725307c1a56755495d5b8fae2065
100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -189,21 +189,30 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice
*vbasedev,
return ret;
}
-static int vfio_save_device_config_state(QEMUFile *f, void *opaque)
+static int vfio_save_device_config_state(QEMUFile *f, void *opaque,
+ Error **errp)
{
VFIODevice *vbasedev = opaque;
+int ret;
qemu_put_be64(f, VFIO_MIG_FLAG_DEV_CONFIG_STATE);
if (vbasedev->ops && vbasedev->ops->vfio_save_config) {
-vbasedev->ops->vfio_save_config(vbasedev, f);
+ret = vbasedev->ops->vfio_save_config(vbasedev, f, errp);
+if (ret) {
+return ret;
+}
}
qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
trace_vfio_save_device_config_state(vbasedev->name);
-return qemu_file_get_error(f);
+ret = qemu_file_get_error(f);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to save state");
+}
+return ret;
}
static int vfio_load_device_config_state(QEMUFile *f, void *opaque)
@@ -588,13 +597,15 @@ static int vfio_save_complete_precopy(QEMUFile *f, void
*opaque)
static void vfio_save_state(QEMUFile *f, void *opaque)
{
VFIODevice *vbasedev = opaque;
+Error *local_err = NULL;
int ret;
-ret = vfio_save_device_config_state(f, opaque);
+ret = vfio_save_device_config_state(f, opaque, _err);
if (ret) {
-error_report("%s: Failed to save device config space",
- vbasedev->name);
-qemu_file_set_error(f, ret);
+error_prepend(_err,
+ "vfio: Failed to save device config space of %s - ",
+ vbasedev->name);
+qemu_file_set_error_obj(f, ret, local_err);
}
}
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index
64780d1b793345c8e8996fe6b7987059ce831c11..fc6e54e871508bb0e2a3ac9079a195c086531f21
100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2586,11 +2586,12 @@ static const VMStateDescription vmstate_vfio_pci_config
= {
}
};
-static void vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f)
+static int vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f, Error
**errp)
{
VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
-vmstate_save_state(f, _vfio_pci_config, vdev, NULL);
+return vmstate_save_state_with_err(f, _vfio_pci_config, vdev, NULL,
+ errp);
}
static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
--
2.45.0
Re: [PATCH v2 13/45] target/hppa: Add space arguments to install_iaq_entries
* Richard Henderson :
> Move space assighments to a central location.
>
> Signed-off-by: Richard Henderson
Reviewed-by: Helge Deller
> ---
> target/hppa/translate.c | 58 +++--
> 1 file changed, 27 insertions(+), 31 deletions(-)
>
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index d24220c60f..05383dcd04 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -624,8 +624,9 @@ static void copy_iaoq_entry(DisasContext *ctx, TCGv_i64
> dest,
> }
> }
>
> -static void install_iaq_entries(DisasContext *ctx, uint64_t bi, TCGv_i64 bv,
> -uint64_t ni, TCGv_i64 nv)
> +static void install_iaq_entries(DisasContext *ctx,
> +uint64_t bi, TCGv_i64 bv, TCGv_i64 bs,
> +uint64_t ni, TCGv_i64 nv, TCGv_i64 ns)
> {
> copy_iaoq_entry(ctx, cpu_iaoq_f, bi, bv);
>
> @@ -639,6 +640,12 @@ static void install_iaq_entries(DisasContext *ctx,
> uint64_t bi, TCGv_i64 bv,
> tcg_gen_andi_i64(cpu_iaoq_b, cpu_iaoq_b,
> gva_offset_mask(ctx->tb_flags));
> }
> +if (bs) {
> +tcg_gen_mov_i64(cpu_iasq_f, bs);
> +}
> +if (ns || bs) {
> +tcg_gen_mov_i64(cpu_iasq_b, ns ? ns : bs);
> +}
> }
>
> static void install_link(DisasContext *ctx, unsigned link, bool with_sr0)
> @@ -670,7 +677,8 @@ static void gen_excp_1(int exception)
>
> static void gen_excp(DisasContext *ctx, int exception)
> {
> -install_iaq_entries(ctx, ctx->iaoq_f, cpu_iaoq_f, ctx->iaoq_b,
> cpu_iaoq_b);
> +install_iaq_entries(ctx, ctx->iaoq_f, cpu_iaoq_f, NULL,
> +ctx->iaoq_b, cpu_iaoq_b, NULL);
> nullify_save(ctx);
> gen_excp_1(exception);
> ctx->base.is_jmp = DISAS_NORETURN;
> @@ -724,10 +732,11 @@ static void gen_goto_tb(DisasContext *ctx, int which,
> {
> if (use_goto_tb(ctx, b, n)) {
> tcg_gen_goto_tb(which);
> -install_iaq_entries(ctx, b, NULL, n, NULL);
> +install_iaq_entries(ctx, b, NULL, NULL, n, NULL, NULL);
> tcg_gen_exit_tb(ctx->base.tb, which);
> } else {
> -install_iaq_entries(ctx, b, cpu_iaoq_b, n, ctx->iaoq_n_var);
> +install_iaq_entries(ctx, b, cpu_iaoq_b, ctx->iasq_b,
> +n, ctx->iaoq_n_var, ctx->iasq_n);
> tcg_gen_lookup_and_goto_ptr();
> }
> }
> @@ -1916,7 +1925,7 @@ static bool do_ibranch(DisasContext *ctx, TCGv_i64 dest,
> install_link(ctx, link, false);
> if (is_n) {
> if (use_nullify_skip(ctx)) {
> -install_iaq_entries(ctx, -1, next, -1, NULL);
> +install_iaq_entries(ctx, -1, next, NULL, -1, NULL, NULL);
> nullify_set(ctx, 0);
> ctx->base.is_jmp = DISAS_IAQ_N_UPDATED;
> return true;
> @@ -1935,10 +1944,11 @@ static bool do_ibranch(DisasContext *ctx, TCGv_i64
> dest,
>
> install_link(ctx, link, false);
> if (is_n && use_nullify_skip(ctx)) {
> -install_iaq_entries(ctx, -1, next, -1, NULL);
> +install_iaq_entries(ctx, -1, next, NULL, -1, NULL, NULL);
> nullify_set(ctx, 0);
> } else {
> -install_iaq_entries(ctx, ctx->iaoq_b, cpu_iaoq_b, -1, next);
> +install_iaq_entries(ctx, ctx->iaoq_b, cpu_iaoq_b, ctx->iasq_b,
> +-1, next, NULL);
> nullify_set(ctx, is_n);
> }
>
> @@ -2026,7 +2036,7 @@ static void do_page_zero(DisasContext *ctx)
> tcg_gen_st_i64(cpu_gr[26], tcg_env, offsetof(CPUHPPAState, cr[27]));
> tmp = tcg_temp_new_i64();
> tcg_gen_ori_i64(tmp, cpu_gr[31], 3);
> -install_iaq_entries(ctx, -1, tmp, -1, NULL);
> +install_iaq_entries(ctx, -1, tmp, NULL, -1, NULL, NULL);
> ctx->base.is_jmp = DISAS_IAQ_N_UPDATED;
> break;
>
> @@ -2770,8 +2780,8 @@ static bool trans_or(DisasContext *ctx, arg_rrr_cf_d *a)
> nullify_over(ctx);
>
> /* Advance the instruction queue. */
> -install_iaq_entries(ctx, ctx->iaoq_b, cpu_iaoq_b,
> -ctx->iaoq_n, ctx->iaoq_n_var);
> +install_iaq_entries(ctx, ctx->iaoq_b, cpu_iaoq_b, ctx->iasq_b,
> +ctx->iaoq_n, ctx->iaoq_n_var, ctx->iasq_n);
> nullify_set(ctx, 0);
>
> /* Tell the qemu main loop to halt until this cpu has work. */
> @@ -3921,16 +3931,11 @@ static bool trans_be(DisasContext *ctx, arg_be *a)
> load_spr(ctx, new_spc, a->sp);
> install_link(ctx, a->l, true);
> if (a->n && use_nullify_skip(ctx)) {
> -install_iaq_entries(ctx, -1, tmp, -1, NULL);
> -tcg_gen_mov_i64(cpu_iasq_f, new_spc);
> -tcg_gen_mov_i64(cpu_iasq_b, new_spc);
> +install_iaq_entries(ctx, -1, tmp, new_spc, -1, NULL, new_spc);
> nullify_set(ctx, 0);
> } else {
> -
Re: [PATCH v2 12/45] target/hppa: Add IASQ entries to DisasContext
* Richard Henderson :
> Add variable to track space changes to IAQ. So far, no such changes
> are introduced, but the new checks vs ctx->iasq_b may eliminate an
> unnecessary copy to cpu_iasq_f with e.g. BLR.
>
> Signed-off-by: Richard Henderson
Reviewed-by: Helge Deller
> ---
> target/hppa/translate.c | 39 ++-
> 1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index 13a48d1b6c..d24220c60f 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -50,6 +50,13 @@ typedef struct DisasContext {
> uint64_t iaoq_b;
> uint64_t iaoq_n;
> TCGv_i64 iaoq_n_var;
> +/*
> + * Null when IASQ_Back unchanged from IASQ_Front,
> + * or cpu_iasq_b, when IASQ_Back has been changed.
> + */
> +TCGv_i64 iasq_b;
> +/* Null when IASQ_Next unchanged from IASQ_Back, or set by branch. */
> +TCGv_i64 iasq_n;
>
> DisasCond null_cond;
> TCGLabel *null_lab;
> @@ -3916,12 +3923,12 @@ static bool trans_be(DisasContext *ctx, arg_be *a)
> if (a->n && use_nullify_skip(ctx)) {
> install_iaq_entries(ctx, -1, tmp, -1, NULL);
> tcg_gen_mov_i64(cpu_iasq_f, new_spc);
> -tcg_gen_mov_i64(cpu_iasq_b, cpu_iasq_f);
> +tcg_gen_mov_i64(cpu_iasq_b, new_spc);
> nullify_set(ctx, 0);
> } else {
> install_iaq_entries(ctx, ctx->iaoq_b, cpu_iaoq_b, -1, tmp);
> -if (ctx->iaoq_b == -1) {
> -tcg_gen_mov_i64(cpu_iasq_f, cpu_iasq_b);
> +if (ctx->iasq_b) {
> +tcg_gen_mov_i64(cpu_iasq_f, ctx->iasq_b);
> }
> tcg_gen_mov_i64(cpu_iasq_b, new_spc);
> nullify_set(ctx, a->n);
> @@ -4035,8 +4042,8 @@ static bool trans_bve(DisasContext *ctx, arg_bve *a)
>
> install_link(ctx, a->l, false);
> install_iaq_entries(ctx, ctx->iaoq_b, cpu_iaoq_b, -1, dest);
> -if (ctx->iaoq_b == -1) {
> -tcg_gen_mov_i64(cpu_iasq_f, cpu_iasq_b);
> +if (ctx->iasq_b) {
> +tcg_gen_mov_i64(cpu_iasq_f, ctx->iasq_b);
> }
> tcg_gen_mov_i64(cpu_iasq_b, space_select(ctx, 0, dest));
> nullify_set(ctx, a->n);
> @@ -4617,6 +4624,7 @@ static void hppa_tr_init_disas_context(DisasContextBase
> *dcbase, CPUState *cs)
> ctx->mmu_idx = MMU_USER_IDX;
> ctx->iaoq_f = ctx->base.pc_first | ctx->privilege;
> ctx->iaoq_b = ctx->base.tb->cs_base | ctx->privilege;
> +ctx->iasq_b = NULL;
> ctx->unalign = (ctx->tb_flags & TB_FLAG_UNALIGN ? MO_UNALN : MO_ALIGN);
> #else
> ctx->privilege = (ctx->tb_flags >> TB_FLAG_PRIV_SHIFT) & 3;
> @@ -4631,6 +4639,7 @@ static void hppa_tr_init_disas_context(DisasContextBase
> *dcbase, CPUState *cs)
>
> ctx->iaoq_f = (ctx->base.pc_first & ~iasq_f) + ctx->privilege;
> ctx->iaoq_b = (diff ? ctx->iaoq_f + diff : -1);
> +ctx->iasq_b = (diff ? NULL : cpu_iasq_b);
> #endif
>
> ctx->zero = tcg_constant_i64(0);
> @@ -4683,6 +4692,7 @@ static void hppa_tr_translate_insn(DisasContextBase
> *dcbase, CPUState *cs)
>
> /* Set up the IA queue for the next insn.
> This will be overwritten by a branch. */
> +ctx->iasq_n = NULL;
> ctx->iaoq_n_var = NULL;
> ctx->iaoq_n = ctx->iaoq_b == -1 ? -1 : ctx->iaoq_b + 4;
>
> @@ -4705,7 +4715,7 @@ static void hppa_tr_translate_insn(DisasContextBase
> *dcbase, CPUState *cs)
> return;
> }
> /* Note this also detects a priority change. */
> -if (ctx->iaoq_b != ctx->iaoq_f + 4) {
> +if (ctx->iaoq_b != ctx->iaoq_f + 4 || ctx->iasq_b) {
> ctx->base.is_jmp = DISAS_IAQ_N_STALE;
> return;
> }
> @@ -4725,6 +4735,10 @@ static void hppa_tr_translate_insn(DisasContextBase
> *dcbase, CPUState *cs)
> gva_offset_mask(ctx->tb_flags));
> }
> }
> +if (ctx->iasq_n) {
> +tcg_gen_mov_i64(cpu_iasq_b, ctx->iasq_n);
> +ctx->iasq_b = cpu_iasq_b;
> +}
> }
>
> static void hppa_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
> @@ -4733,14 +4747,15 @@ static void hppa_tr_tb_stop(DisasContextBase *dcbase,
> CPUState *cs)
> DisasJumpType is_jmp = ctx->base.is_jmp;
> uint64_t fi, bi;
> TCGv_i64 fv, bv;
> -TCGv_i64 fs;
> +TCGv_i64 fs, bs;
>
> /* Assume the insn queue has not been advanced. */
> fi = ctx->iaoq_b;
> fv = cpu_iaoq_b;
> -fs = fi == -1 ? cpu_iasq_b : NULL;
> +fs = ctx->iasq_b;
> bi = ctx->iaoq_n;
> bv = ctx->iaoq_n_var;
> +bs = ctx->iasq_n;
>
> switch (is_jmp) {
> case DISAS_NORETURN:
> @@ -4749,12 +4764,15 @@ static void hppa_tr_tb_stop(DisasContextBase *dcbase,
> CPUState *cs)
> /* The insn queue has not been advanced. */
> bi = fi;
> bv = fv;
> +bs = fs;
> fi = ctx->iaoq_f;
> fv = NULL;
> fs = NULL;
> /* FALLTHRU */
> case DISAS_IAQ_N_STALE:
> -
Re: [PATCH-for-9.1 v2 2/3] migration: Remove RDMA protocol handling
Hello Peter and all, I did a comparison of the VM live-migration speeds between RDMA and TCP/IP on our servers and plotted the results to get an initial impression. Unfortunately, the Ethernet NICs are not the recent ones, therefore, it may not make much sense. I can do it on servers with more recent Ethernet NICs and keep you updated. It seems that the benefits of RDMA becomes obviously when the VM has large memory and is running memory-intensive workload. Best regards, Yu Zhang @ IONOS Cloud On Thu, May 9, 2024 at 4:14 PM Peter Xu wrote: > > On Thu, May 09, 2024 at 04:58:34PM +0800, Zheng Chuan via wrote: > > That's a good news to see the socket abstraction for RDMA! > > When I was developed the series above, the most pain is the RDMA migration > > has no QIOChannel abstraction and i need to take a 'fake channel' > > for it which is awkward in code implementation. > > So, as far as I know, we can do this by > > i. the first thing is that we need to evaluate the rsocket is good enough > > to satisfy our QIOChannel fundamental abstraction > > ii. if it works right, then we will continue to see if it can give us > > opportunity to hide the detail of rdma protocol > > into rsocket by remove most of code in rdma.c and also some hack in > > migration main process. > > iii. implement the advanced features like multi-fd and multi-uri for rdma > > migration. > > > > Since I am not familiar with rsocket, I need some times to look at it and > > do some quick verify with rdma migration based on rsocket. > > But, yes, I am willing to involved in this refactor work and to see if we > > can make this migration feature more better:） > > Based on what we have now, it looks like we'd better halt the deprecation > process a bit, so I think we shouldn't need to rush it at least in 9.1 > then, and we'll need to see how it goes on the refactoring. > > It'll be perfect if rsocket works, otherwise supporting multifd with little > overhead / exported APIs would also be a good thing in general with > whatever approach. And obviously all based on the facts that we can get > resources from companies to support this feature first. > > Note that so far nobody yet compared with rdma v.s. nic perf, so I hope if > any of us can provide some test results please do so. Many people are > saying RDMA is better, but I yet didn't see any numbers comparing it with > modern TCP networks. I don't want to have old impressions floating around > even if things might have changed.. When we have consolidated results, we > should share them out and also reflect that in QEMU's migration docs when a > rdma document page is ready. > > Chuan, please check the whole thread discussion, it may help to understand > what we are looking for on rdma migrations [1]. Meanwhile please feel free > to sync with Jinpu's team and see how to move forward with such a project. > > [1] https://lore.kernel.org/qemu-devel/87frwatp7n@suse.de/ > > Thanks, > > -- > Peter Xu >
Re: [PATCH v2 11/45] target/hppa: Simplify TB end
* Richard Henderson :
> Minimize the amount of code in hppa_tr_translate_insn advancing the
> insn queue for the next insn. Move the goto_tb path to hppa_tr_tb_stop.
>
> Signed-off-by: Richard Henderson
Reviewed-by: Helge Deller
> ---
> target/hppa/translate.c | 109 +---
> 1 file changed, 57 insertions(+), 52 deletions(-)
>
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index ca979f4137..13a48d1b6c 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -4699,54 +4699,31 @@ static void hppa_tr_translate_insn(DisasContextBase
> *dcbase, CPUState *cs)
> }
> }
>
> -/* Advance the insn queue. Note that this check also detects
> - a priority change within the instruction queue. */
> -if (ret == DISAS_NEXT && ctx->iaoq_b != ctx->iaoq_f + 4) {
> -if (use_goto_tb(ctx, ctx->iaoq_b, ctx->iaoq_n)
> -&& (ctx->null_cond.c == TCG_COND_NEVER
> -|| ctx->null_cond.c == TCG_COND_ALWAYS)) {
> -nullify_set(ctx, ctx->null_cond.c == TCG_COND_ALWAYS);
> -gen_goto_tb(ctx, 0, ctx->iaoq_b, ctx->iaoq_n);
> -ctx->base.is_jmp = ret = DISAS_NORETURN;
> -} else {
> -ctx->base.is_jmp = ret = DISAS_IAQ_N_STALE;
> -}
> +/* If the TranslationBlock must end, do so. */
> +ctx->base.pc_next += 4;
> +if (ret != DISAS_NEXT) {
> +return;
> }
> +/* Note this also detects a priority change. */
> +if (ctx->iaoq_b != ctx->iaoq_f + 4) {
> +ctx->base.is_jmp = DISAS_IAQ_N_STALE;
> +return;
> +}
> +
> +/*
> + * Advance the insn queue.
> + * The only exit now is DISAS_TOO_MANY from the translator loop.
> + */
> ctx->iaoq_f = ctx->iaoq_b;
> ctx->iaoq_b = ctx->iaoq_n;
> -ctx->base.pc_next += 4;
> -
> -switch (ret) {
> -case DISAS_NORETURN:
> -case DISAS_IAQ_N_UPDATED:
> -break;
> -
> -case DISAS_NEXT:
> -case DISAS_IAQ_N_STALE:
> -case DISAS_IAQ_N_STALE_EXIT:
> -if (ctx->iaoq_f == -1) {
> -install_iaq_entries(ctx, -1, cpu_iaoq_b,
> -ctx->iaoq_n, ctx->iaoq_n_var);
> -#ifndef CONFIG_USER_ONLY
> -tcg_gen_mov_i64(cpu_iasq_f, cpu_iasq_b);
> -#endif
> -nullify_save(ctx);
> -ctx->base.is_jmp = (ret == DISAS_IAQ_N_STALE_EXIT
> -? DISAS_EXIT
> -: DISAS_IAQ_N_UPDATED);
> -} else if (ctx->iaoq_b == -1) {
> -if (ctx->iaoq_n_var) {
> -copy_iaoq_entry(ctx, cpu_iaoq_b, -1, ctx->iaoq_n_var);
> -} else {
> -tcg_gen_addi_i64(cpu_iaoq_b, cpu_iaoq_b, 4);
> -tcg_gen_andi_i64(cpu_iaoq_b, cpu_iaoq_b,
> - gva_offset_mask(ctx->tb_flags));
> -}
> +if (ctx->iaoq_b == -1) {
> +if (ctx->iaoq_n_var) {
> +copy_iaoq_entry(ctx, cpu_iaoq_b, -1, ctx->iaoq_n_var);
> +} else {
> +tcg_gen_addi_i64(cpu_iaoq_b, cpu_iaoq_b, 4);
> +tcg_gen_andi_i64(cpu_iaoq_b, cpu_iaoq_b,
> + gva_offset_mask(ctx->tb_flags));
> }
> -break;
> -
> -default:
> -g_assert_not_reached();
> }
> }
>
> @@ -4754,23 +4731,51 @@ static void hppa_tr_tb_stop(DisasContextBase *dcbase,
> CPUState *cs)
> {
> DisasContext *ctx = container_of(dcbase, DisasContext, base);
> DisasJumpType is_jmp = ctx->base.is_jmp;
> +uint64_t fi, bi;
> +TCGv_i64 fv, bv;
> +TCGv_i64 fs;
> +
> +/* Assume the insn queue has not been advanced. */
> +fi = ctx->iaoq_b;
> +fv = cpu_iaoq_b;
> +fs = fi == -1 ? cpu_iasq_b : NULL;
> +bi = ctx->iaoq_n;
> +bv = ctx->iaoq_n_var;
>
> switch (is_jmp) {
> case DISAS_NORETURN:
> break;
> case DISAS_TOO_MANY:
> -case DISAS_IAQ_N_STALE:
> -case DISAS_IAQ_N_STALE_EXIT:
> -install_iaq_entries(ctx, ctx->iaoq_f, cpu_iaoq_f,
> -ctx->iaoq_b, cpu_iaoq_b);
> -nullify_save(ctx);
> +/* The insn queue has not been advanced. */
> +bi = fi;
> +bv = fv;
> +fi = ctx->iaoq_f;
> +fv = NULL;
> +fs = NULL;
> /* FALLTHRU */
> -case DISAS_IAQ_N_UPDATED:
> -if (is_jmp != DISAS_IAQ_N_STALE_EXIT) {
> -tcg_gen_lookup_and_goto_ptr();
> +case DISAS_IAQ_N_STALE:
> +if (use_goto_tb(ctx, fi, bi)
> +&& (ctx->null_cond.c == TCG_COND_NEVER
> +|| ctx->null_cond.c == TCG_COND_ALWAYS)) {
> +nullify_set(ctx, ctx->null_cond.c == TCG_COND_ALWAYS);
> +gen_goto_tb(ctx, 0, fi, bi);
> break;
> }
> /* FALLTHRU */
> +case DISAS_IAQ_N_STALE_EXIT:
> +install_iaq_entries(ctx, fi, fv, bi, bv);
> +if (fs) {
> +
Re: [PATCH v2 10/45] target/hppa: Skip nullified insns in unconditional dbranch path
* Richard Henderson :
> Signed-off-by: Richard Henderson
Reviewed-by: Helge Deller
> ---
> target/hppa/translate.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index a9196050dc..ca979f4137 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -1805,11 +1805,17 @@ static bool do_dbranch(DisasContext *ctx, int64_t
> disp,
>
> if (ctx->null_cond.c == TCG_COND_NEVER && ctx->null_lab == NULL) {
> install_link(ctx, link, false);
> -ctx->iaoq_n = dest;
> -ctx->iaoq_n_var = NULL;
> if (is_n) {
> +if (use_nullify_skip(ctx)) {
> +nullify_set(ctx, 0);
> +gen_goto_tb(ctx, 0, dest, dest + 4);
> +ctx->base.is_jmp = DISAS_NORETURN;
> +return true;
> +}
> ctx->null_cond.c = TCG_COND_ALWAYS;
> }
> +ctx->iaoq_n = dest;
> +ctx->iaoq_n_var = NULL;
> } else {
> nullify_over(ctx);
>
> --
> 2.34.1
>
Re: [PATCH v2 09/45] target/hppa: Delay computation of IAQ_Next
* Richard Henderson :
> We no longer have to allocate a temp and perform an
> addition before translation of the rest of the insn.
>
> Signed-off-by: Richard Henderson
Reviewed-by: Helge Deller
> ---
> target/hppa/translate.c | 26 ++
> 1 file changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index f816b337ee..a9196050dc 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -1806,6 +1806,7 @@ static bool do_dbranch(DisasContext *ctx, int64_t disp,
> if (ctx->null_cond.c == TCG_COND_NEVER && ctx->null_lab == NULL) {
> install_link(ctx, link, false);
> ctx->iaoq_n = dest;
> +ctx->iaoq_n_var = NULL;
> if (is_n) {
> ctx->null_cond.c = TCG_COND_ALWAYS;
> }
> @@ -1862,11 +1863,6 @@ static bool do_cbranch(DisasContext *ctx, int64_t
> disp, bool is_n,
> ctx->null_lab = NULL;
> }
> nullify_set(ctx, n);
> -if (ctx->iaoq_n == -1) {
> -/* The temporary iaoq_n_var died at the branch above.
> - Regenerate it here instead of saving it. */
> -tcg_gen_addi_i64(ctx->iaoq_n_var, cpu_iaoq_b, 4);
> -}
> gen_goto_tb(ctx, 0, ctx->iaoq_b, ctx->iaoq_n);
> }
>
> @@ -4630,8 +4626,6 @@ static void hppa_tr_init_disas_context(DisasContextBase
> *dcbase, CPUState *cs)
> ctx->iaoq_f = (ctx->base.pc_first & ~iasq_f) + ctx->privilege;
> ctx->iaoq_b = (diff ? ctx->iaoq_f + diff : -1);
> #endif
> -ctx->iaoq_n = -1;
> -ctx->iaoq_n_var = NULL;
>
> ctx->zero = tcg_constant_i64(0);
>
> @@ -4683,14 +4677,8 @@ static void hppa_tr_translate_insn(DisasContextBase
> *dcbase, CPUState *cs)
>
> /* Set up the IA queue for the next insn.
> This will be overwritten by a branch. */
> -if (ctx->iaoq_b == -1) {
> -ctx->iaoq_n = -1;
> -ctx->iaoq_n_var = tcg_temp_new_i64();
> -tcg_gen_addi_i64(ctx->iaoq_n_var, cpu_iaoq_b, 4);
> -} else {
> -ctx->iaoq_n = ctx->iaoq_b + 4;
> -ctx->iaoq_n_var = NULL;
> -}
> +ctx->iaoq_n_var = NULL;
> +ctx->iaoq_n = ctx->iaoq_b == -1 ? -1 : ctx->iaoq_b + 4;
>
> if (unlikely(ctx->null_cond.c == TCG_COND_ALWAYS)) {
> ctx->null_cond.c = TCG_COND_NEVER;
> @@ -4741,7 +4729,13 @@ static void hppa_tr_translate_insn(DisasContextBase
> *dcbase, CPUState *cs)
> ? DISAS_EXIT
> : DISAS_IAQ_N_UPDATED);
> } else if (ctx->iaoq_b == -1) {
> -copy_iaoq_entry(ctx, cpu_iaoq_b, -1, ctx->iaoq_n_var);
> +if (ctx->iaoq_n_var) {
> +copy_iaoq_entry(ctx, cpu_iaoq_b, -1, ctx->iaoq_n_var);
> +} else {
> +tcg_gen_addi_i64(cpu_iaoq_b, cpu_iaoq_b, 4);
> +tcg_gen_andi_i64(cpu_iaoq_b, cpu_iaoq_b,
> + gva_offset_mask(ctx->tb_flags));
> +}
> }
> break;
>
> --
> 2.34.1
>
Re: [PATCH v2 08/45] target/hppa: Add install_link
* Richard Henderson :
> Add a common routine for writing the return address.
>
> Signed-off-by: Richard Henderson
Reviewed-by: Helge Deller
> ---
> target/hppa/translate.c | 54 +++--
> 1 file changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index 08d5e2a4bc..f816b337ee 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -634,6 +634,23 @@ static void install_iaq_entries(DisasContext *ctx,
> uint64_t bi, TCGv_i64 bv,
> }
> }
>
> +static void install_link(DisasContext *ctx, unsigned link, bool with_sr0)
> +{
> +tcg_debug_assert(ctx->null_cond.c == TCG_COND_NEVER);
> +if (link) {
Just wondering:
Doesn't it makes it easier to write here:
if (!link) {
return;
}
and then don't indent the few following lines?
> +if (ctx->iaoq_b == -1) {
> +tcg_gen_addi_i64(cpu_gr[link], cpu_iaoq_b, 4);
> +} else {
> +tcg_gen_movi_i64(cpu_gr[link], ctx->iaoq_b + 4);
> +}
> +#ifndef CONFIG_USER_ONLY
> +if (with_sr0) {
> +tcg_gen_mov_i64(cpu_sr[0], cpu_iasq_b);
> +}
> +#endif
> +}
> +}
> +
> static inline uint64_t iaoq_dest(DisasContext *ctx, int64_t disp)
> {
> return ctx->iaoq_f + disp + 8;
> @@ -1787,9 +1804,7 @@ static bool do_dbranch(DisasContext *ctx, int64_t disp,
> uint64_t dest = iaoq_dest(ctx, disp);
>
> if (ctx->null_cond.c == TCG_COND_NEVER && ctx->null_lab == NULL) {
> -if (link != 0) {
> -copy_iaoq_entry(ctx, cpu_gr[link], ctx->iaoq_n, ctx->iaoq_n_var);
> -}
> +install_link(ctx, link, false);
> ctx->iaoq_n = dest;
> if (is_n) {
> ctx->null_cond.c = TCG_COND_ALWAYS;
> @@ -1797,10 +1812,7 @@ static bool do_dbranch(DisasContext *ctx, int64_t disp,
> } else {
> nullify_over(ctx);
>
> -if (link != 0) {
> -copy_iaoq_entry(ctx, cpu_gr[link], ctx->iaoq_n, ctx->iaoq_n_var);
> -}
> -
> +install_link(ctx, link, false);
> if (is_n && use_nullify_skip(ctx)) {
> nullify_set(ctx, 0);
> gen_goto_tb(ctx, 0, dest, dest + 4);
> @@ -1892,9 +1904,7 @@ static bool do_ibranch(DisasContext *ctx, TCGv_i64 dest,
> next = tcg_temp_new_i64();
> tcg_gen_mov_i64(next, dest);
>
> -if (link != 0) {
> -copy_iaoq_entry(ctx, cpu_gr[link], ctx->iaoq_n, ctx->iaoq_n_var);
> -}
> +install_link(ctx, link, false);
> if (is_n) {
> if (use_nullify_skip(ctx)) {
> install_iaq_entries(ctx, -1, next, -1, NULL);
> @@ -1911,16 +1921,17 @@ static bool do_ibranch(DisasContext *ctx, TCGv_i64
> dest,
>
> nullify_over(ctx);
>
> +next = tcg_temp_new_i64();
> +tcg_gen_mov_i64(next, dest);
> +
> +install_link(ctx, link, false);
> if (is_n && use_nullify_skip(ctx)) {
> -install_iaq_entries(ctx, -1, dest, -1, NULL);
> +install_iaq_entries(ctx, -1, next, -1, NULL);
> nullify_set(ctx, 0);
> } else {
> -install_iaq_entries(ctx, ctx->iaoq_b, cpu_iaoq_b, -1, dest);
> +install_iaq_entries(ctx, ctx->iaoq_b, cpu_iaoq_b, -1, next);
> nullify_set(ctx, is_n);
> }
> -if (link != 0) {
> -copy_iaoq_entry(ctx, cpu_gr[link], ctx->iaoq_n, ctx->iaoq_n_var);
> -}
>
> tcg_gen_lookup_and_goto_ptr();
> ctx->base.is_jmp = DISAS_NORETURN;
> @@ -3899,10 +3910,7 @@ static bool trans_be(DisasContext *ctx, arg_be *a)
> nullify_over(ctx);
>
> load_spr(ctx, new_spc, a->sp);
> -if (a->l) {
> -copy_iaoq_entry(ctx, cpu_gr[31], ctx->iaoq_n, ctx->iaoq_n_var);
> -tcg_gen_mov_i64(cpu_sr[0], cpu_iasq_b);
> -}
> +install_link(ctx, a->l, true);
> if (a->n && use_nullify_skip(ctx)) {
> install_iaq_entries(ctx, -1, tmp, -1, NULL);
> tcg_gen_mov_i64(cpu_iasq_f, new_spc);
> @@ -4019,16 +4027,16 @@ static bool trans_bve(DisasContext *ctx, arg_bve *a)
> return do_ibranch(ctx, dest, a->l, a->n);
> #else
> nullify_over(ctx);
> -dest = do_ibranch_priv(ctx, load_gpr(ctx, a->b));
> +dest = tcg_temp_new_i64();
> +tcg_gen_mov_i64(dest, load_gpr(ctx, a->b));
> +dest = do_ibranch_priv(ctx, dest);
>
> +install_link(ctx, a->l, false);
> install_iaq_entries(ctx, ctx->iaoq_b, cpu_iaoq_b, -1, dest);
> if (ctx->iaoq_b == -1) {
> tcg_gen_mov_i64(cpu_iasq_f, cpu_iasq_b);
> }
> tcg_gen_mov_i64(cpu_iasq_b, space_select(ctx, 0, dest));
> -if (a->l) {
> -copy_iaoq_entry(ctx, cpu_gr[a->l], ctx->iaoq_n, ctx->iaoq_n_var);
> -}
> nullify_set(ctx, a->n);
> tcg_gen_lookup_and_goto_ptr();
> ctx->base.is_jmp = DISAS_NORETURN;
> --
> 2.34.1
>
