[Qemu-devel] [PATCH v2] qga: unset frozen state if no mount point is frozen
From: Chen Hanxiao
If we set mountpoints to qmp_guest_fsfreeze_freeze_list,
we may got nothing to freeze as all mountpoints are
not valid.
So call ga_unset_frozen in this senario.
Also, if we return 0 frozen fs, there is no need to call
guest-fsfreeze-thaw.
Cc: Michael Roth
Signed-off-by: Chen Hanxiao
---
v2:
remove has_mountpoints special case
add qapi-schema.json section
qga/commands-posix.c | 6 ++
qga/qapi-schema.json | 4 +++-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 967061444a..05cf9caa04 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1274,6 +1274,12 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool
has_mountpoints,
}
free_fs_mount_list(&mounts);
+/* We may not issue any FIFREEZE here.
+ * Just unset ga_state here and ready for the next call.
+ */
+if (i == 0) {
+ga_unset_frozen(ga_state);
+}
return i;
error:
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 17884c7c70..1045cef386 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -435,7 +435,9 @@
# for up to 10 seconds by VSS.
#
# Returns: Number of file systems currently frozen. On error, all filesystems
-# will be thawed.
+# will be thawed. If no filesystems are frozen as a result of this call,
+# then @guest-fsfreeze-status will remain "thawed" and calling
+# @guest-fsfreeze-thaw is not necessary.
#
# Since: 0.15.0
##
--
2.14.3
Re: [Qemu-devel] Qemu aborted in ide_restart_bh after migration
This issue occurred at a very low probability, If we inject delays in
address_space_write_continue like this, the issue occurred inevitably：
###
diff --git a/exec.c b/exec.c
index e8d7b33..b9779e0 100644
--- a/exec.c
+++ b/exec.c
@@ -3056,6 +3056,11 @@ static MemTxResult flatview_write_continue(FlatView *fv,
hwaddr addr,
if (release_lock) {
qemu_mutex_unlock_iothread();
+if (mr->name && !strcmp(mr->name, "ide") &&
atomic_xchg_my_flag(0)) {
+printf("start sleep");
+usleep(5 * 1000 * 1000);
+printf("end sleep");
+}
release_lock = false;
}
w00185384@HGHY1w001853841 /cygdrive/f/GitHome/opensource/qemu
$ git diff
diff --git a/exec.c b/exec.c
index e8d7b33..b9779e0 100644
--- a/exec.c
+++ b/exec.c
@@ -3056,6 +3056,11 @@ static MemTxResult flatview_write_continue(FlatView *fv,
hwaddr addr,
if (release_lock) {
qemu_mutex_unlock_iothread();
+if (mr->name && !strcmp(mr->name, "ide") &&
atomic_xchg_my_flag(0)) {
+printf("start sleep");
+usleep(5 * 1000 * 1000);
+printf("end sleep");
+}
release_lock = false;
}
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 1ab0a89..a1807c8 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -51,6 +51,11 @@ static void bmdma_start_dma(IDEDMA *dma, IDEState *s,
if (bm->status & BM_STATUS_DMAING) {
bm->dma_cb(bmdma_active_if(bm), 0);
}
+
+if (!(bm->status & BM_STATUS_DMAING) && bm->dma_cb &&
+IDE_DMA_ATAPI == bmdma_active_if(bm)->dma_cmd) {
+atomic_xchg_my_flag(1);
+}
}
/**
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index e1c8ae1..095d6a4 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -47,4 +47,6 @@ void info_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
const char *error_get_progname(void);
extern bool enable_timestamp_msg;
+int atomic_xchg_my_flag(int flag);
+
#endif
diff --git a/util/qemu-error.c b/util/qemu-error.c
index a25d3b9..969bdeb 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -310,3 +310,9 @@ void info_report(const char *fmt, ...)
vreport(REPORT_TYPE_INFO, fmt, ap);
va_end(ap);
}
+
+int atomic_xchg_my_flag(int flag)
+{
+static int my_flag = 0;
+return atomic_xchg(&my_flag, flag);
+}
###
Reproduce steps:
1) use 'virsh create vm.xml' to start a VM, and the VM configured empty IDE
CD-ROM.
2) when qemu process print "start sleep", execute "virsh reset vm; virsh
suspend vm" command.
3) use "virsh save vm vm.state" to save VM's state.
4) use "virsh restore vm.state" to restore VM's state.
5) execute "virsh resume vm", and then qemu process aborted at ide_restart_bh.
I think we should set bm->dma_cb to NULL in bmdma_reset function to avoid this
issues.
>> Empty IDE CD-ROM configured on the VM:
>>
>>
>>
>>
>>
>>
>> Make migration for this VM, then qemu aborted in ide_restart_bh.
>> IDEState expect end_transfer_func equal to ide_atapi_cmd, but it refer to
>> ide_dummy_transfer_stop.
>> I have no idea about this, can anyone help me?
>>
>
>Do you have an easy way to reproduce this? 2.8.1 is a bit old at this point,
>but I don't think we've changed anything in the IDE emulator substantively
>since then, so I'm curious to see if I can get this to reproduce.
>
>I'm surprised an empty CD-ROM would cause this, though, since it shouldn't
>really have any commands in-flight that might get us to a weird edge case, so
>I want to take a close look at this.
>
>Denis Lunev noted some issues with migration that I couldn't solve at the time
>either. A reproducer would be fantastic.
>
>> qemu version is 2.8.1
>> (gdb) bt
>> #0 0x7fcff7c4b157 in raise () from /usr/lib64/libc.so.6
>> #1 0x7fcff7c4c848 in abort () from /usr/lib64/libc.so.6
>> #2 0x7fcff7c441c6 in __assert_fail_base () from
>> /usr/lib64/libc.so.6
>> #3 0x7fcff7c44272 in __assert_fail () from /usr/lib64/libc.so.6
>> #4 0x006207ab in ide_restart_bh (opaque=0x38b3430) at
>>
>> (gdb)
>>
>
>--
>—js
>
[Qemu-devel] [PULL] Update OpenBIOS images
Hi Peter, This update for OpenBIOS contains a single fix which allows yaboot 1.3.17 to boot under qemu-system-ppc. Please pull. ATB, Mark. The following changes since commit a6e0344fa0e09413324835ae122c4cadd7890231: Merge remote-tracking branch 'remotes/kraxel/tags/ui-20180220-pull-request' into staging (2018-02-20 14:05:00 +) are available in the git repository at: https://github.com/mcayland/qemu.git tags/qemu-openbios-signed for you to fetch changes up to feb3174ff27f94d34247952d9779f09e9b1dfd39: Update OpenBIOS images to 54d959d9 built from submodule. (2018-02-22 07:55:47 +) Update OpenBIOS images Mark Cave-Ayland (1): Update OpenBIOS images to 54d959d9 built from submodule. pc-bios/openbios-ppc | Bin 754936 -> 754936 bytes pc-bios/openbios-sparc32 | Bin 382048 -> 382048 bytes pc-bios/openbios-sparc64 | Bin 1593408 -> 1593408 bytes roms/openbios| 2 +- 4 files changed, 1 insertion(+), 1 deletion(-)
Re: [Qemu-devel] [qemu-s390x] [PATCH v8 05/13] s390-ccw: move auxiliary IPL data to separate location
On 22.02.2018 05:40, Thomas Huth wrote:
> On 21.02.2018 20:35, Collin L. Walling wrote:
>> The s390-ccw firmware needs some information in support of the
>> boot process which is not available on the native machine.
>> Examples are the netboot firmware load address and now the
>> boot menu parameters.
>>
>> While storing that data in unused fields of the IPL parameter block
>> works, that approach could create problems if the parameter block
>> definition should change in the future. Because then a guest could
>> overwrite these fields using the set IPLB diagnose.
>>
>> In fact the data in question is of more global nature and not really
>> tied to an IPL device, so separating it is rather logical.
>>
>> This commit introduces a new structure to hold firmware relevant
>> IPL parameters set by QEMU. The data is stored at location 204 (dec)
>> and can contain up to 7 32-bit words. This area is available to
>> programming in the z/Architecture Principles of Operation and
>> can thus safely be used by the firmware until the IPL has completed.
>>
>> Signed-off-by: Viktor Mihajlovski
>> Signed-off-by: Collin L. Walling
>> ---
>> hw/s390x/ipl.c | 18 +-
>> hw/s390x/ipl.h | 25 +++--
>> pc-bios/s390-ccw/iplb.h | 18 --
>> pc-bios/s390-ccw/main.c | 6 +-
>> 4 files changed, 61 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 0d06fc1..79f5a58 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -399,6 +399,21 @@ void s390_reipl_request(void)
>> qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>> }
>>
>> +static void s390_ipl_prepare_qipl(S390CPU *cpu)
>> +{
>> +S390IPLState *ipl = get_ipl_device();
>> +uint8_t *addr;
>> +uint64_t len = 4096;
>> +
>> +addr = cpu_physical_memory_map(cpu->env.psa, &len, 1);
>> +if (!addr || len < QIPL_ADDRESS + sizeof(QemuIplParameters)) {
>> +error_report("Cannot set QEMU IPL parameters");
>> +return;
>> +}
>> +memcpy(addr + QIPL_ADDRESS, &ipl->qipl, sizeof(QemuIplParameters));
>> +cpu_physical_memory_unmap(addr, len, 1, len);
>> +}
>> +
>> void s390_ipl_prepare_cpu(S390CPU *cpu)
>> {
>> S390IPLState *ipl = get_ipl_device();
>> @@ -418,8 +433,9 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
>> error_report_err(err);
>> vm_stop(RUN_STATE_INTERNAL_ERROR);
>> }
>> -ipl->iplb.ccw.netboot_start_addr = cpu_to_be64(ipl->start_addr);
>> +ipl->qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr);
>> }
>> +s390_ipl_prepare_qipl(cpu);
>> }
>>
>> static void s390_ipl_reset(DeviceState *dev)
>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>> index 8a705e0..08926a3 100644
>> --- a/hw/s390x/ipl.h
>> +++ b/hw/s390x/ipl.h
>> @@ -16,8 +16,7 @@
>> #include "cpu.h"
>>
>> struct IplBlockCcw {
>> -uint64_t netboot_start_addr;
>> -uint8_t reserved0[77];
>> +uint8_t reserved0[85];
>> uint8_t ssid;
>> uint16_t devno;
>> uint8_t vm_flags;
>> @@ -90,6 +89,27 @@ void s390_ipl_prepare_cpu(S390CPU *cpu);
>> IplParameterBlock *s390_ipl_get_iplb(void);
>> void s390_reipl_request(void);
>>
>> +#define QIPL_ADDRESS 0xcc
>> +
>> +/*
>> + * The QEMU IPL Parameters will be stored at absolute address
>> + * 204 (0xcc) which means it is 32-bit word aligned but not
>> + * double-word aligned.
>> + * Placement of data fields in this area must account for
>> + * their alignment needs. E.g., netboot_start_address must
>> + * have an offset of n * 8 bytes within the struct in order
>> + * to keep it double-word aligned.
>
> Should that rather be "4 + n * 8" instead of "n * 8" ?
I wonder if I ever get that comment right. You're correct of course.
>
> Apart from that, patch looks good to me now, so once you've fixed the
> comment (if necessary):
>
> Reviewed-by: Thomas Huth
>
--
Regards,
Viktor Mihajlovski
[Qemu-devel] [PATCH v4] migration: change blocktime type to uint32_t
Initially int64_t was used, but on PowerPC architecture,
clang doesn't have atomic_*_8 function, so it produces
link time error.
QEMU is working with time as with 64bit value, but by
fact 32 bit is enough with CLOCK_REALTIME. In this case
blocktime will keep only 1200 hours time interval.
Signed-off-by: Alexey Perevalov
Acked-by: Eric Blake
---
hmp.c| 4 ++--
migration/postcopy-ram.c | 52
migration/trace-events | 4 ++--
qapi/migration.json | 4 ++--
4 files changed, 36 insertions(+), 28 deletions(-)
diff --git a/hmp.c b/hmp.c
index be091e0..ec90043 100644
--- a/hmp.c
+++ b/hmp.c
@@ -267,7 +267,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
}
if (info->has_postcopy_blocktime) {
-monitor_printf(mon, "postcopy blocktime: %" PRId64 "\n",
+monitor_printf(mon, "postcopy blocktime: %u\n",
info->postcopy_blocktime);
}
@@ -275,7 +275,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
Visitor *v;
char *str;
v = string_output_visitor_new(false, &str);
-visit_type_int64List(v, NULL, &info->postcopy_vcpu_blocktime, NULL);
+visit_type_uint32List(v, NULL, &info->postcopy_vcpu_blocktime, NULL);
visit_complete(v, &str);
monitor_printf(mon, "postcopy vcpu blocktime: %s\n", str);
g_free(str);
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 05475e0..c46225c 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -63,16 +63,17 @@ struct PostcopyDiscardState {
typedef struct PostcopyBlocktimeContext {
/* time when page fault initiated per vCPU */
-int64_t *page_fault_vcpu_time;
+uint32_t *page_fault_vcpu_time;
/* page address per vCPU */
uintptr_t *vcpu_addr;
-int64_t total_blocktime;
+uint32_t total_blocktime;
/* blocktime per vCPU */
-int64_t *vcpu_blocktime;
+uint32_t *vcpu_blocktime;
/* point in time when last page fault was initiated */
-int64_t last_begin;
+uint32_t last_begin;
/* number of vCPU are suspended */
int smp_cpus_down;
+uint64_t start_time;
/*
* Handler for exit event, necessary for
@@ -99,22 +100,23 @@ static void migration_exit_cb(Notifier *n, void *data)
static struct PostcopyBlocktimeContext *blocktime_context_new(void)
{
PostcopyBlocktimeContext *ctx = g_new0(PostcopyBlocktimeContext, 1);
-ctx->page_fault_vcpu_time = g_new0(int64_t, smp_cpus);
+ctx->page_fault_vcpu_time = g_new0(uint32_t, smp_cpus);
ctx->vcpu_addr = g_new0(uintptr_t, smp_cpus);
-ctx->vcpu_blocktime = g_new0(int64_t, smp_cpus);
+ctx->vcpu_blocktime = g_new0(uint32_t, smp_cpus);
ctx->exit_notifier.notify = migration_exit_cb;
+ctx->start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
qemu_add_exit_notifier(&ctx->exit_notifier);
return ctx;
}
-static int64List *get_vcpu_blocktime_list(PostcopyBlocktimeContext *ctx)
+static uint32List *get_vcpu_blocktime_list(PostcopyBlocktimeContext *ctx)
{
-int64List *list = NULL, *entry = NULL;
+uint32List *list = NULL, *entry = NULL;
int i;
for (i = smp_cpus - 1; i >= 0; i--) {
-entry = g_new0(int64List, 1);
+entry = g_new0(uint32List, 1);
entry->value = ctx->vcpu_blocktime[i];
entry->next = list;
list = entry;
@@ -145,7 +147,7 @@ void fill_destination_postcopy_migration_info(MigrationInfo
*info)
info->postcopy_vcpu_blocktime = get_vcpu_blocktime_list(bc);
}
-static uint64_t get_postcopy_total_blocktime(void)
+static uint32_t get_postcopy_total_blocktime(void)
{
MigrationIncomingState *mis = migration_incoming_get_current();
PostcopyBlocktimeContext *bc = mis->blocktime_ctx;
@@ -610,6 +612,13 @@ static int get_mem_fault_cpu_index(uint32_t pid)
return -1;
}
+static uint32_t get_low_time_offset(PostcopyBlocktimeContext *dc)
+{
+int64_t start_time_offset = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) -
+dc->start_time;
+return start_time_offset < 1 ? 1 : start_time_offset & UINT32_MAX;
+}
+
/*
* This function is being called when pagefault occurs. It
* tracks down vCPU blocking time.
@@ -624,7 +633,7 @@ static void mark_postcopy_blocktime_begin(uintptr_t addr,
uint32_t ptid,
int cpu, already_received;
MigrationIncomingState *mis = migration_incoming_get_current();
PostcopyBlocktimeContext *dc = mis->blocktime_ctx;
-int64_t now_ms;
+uint32_t low_time_offset;
if (!dc || ptid == 0) {
return;
@@ -634,14 +643,14 @@ static void mark_postcopy_blocktime_begin(uintptr_t addr,
uint32_t ptid,
return;
}
-now_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+low_time_offset = get_low_time_offset(dc);
if (dc->vcpu_addr[cpu] == 0) {
atomic_inc(&dc->smp_cpus_down);
}
-atomic_xchg__nocheck(&dc->last
[Qemu-devel] [PATCH v4] Fix build on ppc platform in migration/postcopy-ram.c
V4->V3 - common helper was introduced and sanity check for probable time jumps (comment from David) V2->V3 - use UINT32_MAX instead of 0x (comment from Philippe) - use lelative time to avoid milliseconds overflow in uint32 (comment from David) V2->V1 This is a second version: - comment from David about casting David was right, I tried to find it in standard, but it was implicitly described for me, so part of standard: 1. When a value with integer type is converted to another integer type other than _Bool, if the value can be represented by the new type, it is unchanged. 2. Otherwise, if the new type is unsigned, the value is converted by repeatedly adding or subtracting one more than the maximum value that can be represented in the new type until the value is in the range of the new type. Initial message: It was a problem with 64 atomics on ppc in migration/postcopy-ram.c reported by Philippe Mathieu-Daudé . Tested in Debian on qemu-system-ppc and in Ubuntu16.04 on i386. This commit is based on commit afd3397a8149d8b645004e459bf2002d78f5e267 Merge remote-tracking branch 'remotes/stefanha/tags/tracing-pull-request' into staging but with all necessary commit reverted in ee86981bda9ecd40c8daf81b7307b1d2aff68174 Alexey Perevalov (1): migration: change blocktime type to uint32_t hmp.c| 4 ++-- migration/postcopy-ram.c | 52 migration/trace-events | 4 ++-- qapi/migration.json | 4 ++-- 4 files changed, 36 insertions(+), 28 deletions(-) -- 2.7.4
Re: [Qemu-devel] [PATCH V4 3/3] tests: Add migration test for aarch64
On Wed, Feb 21, 2018 at 10:44:17PM -0600, Wei Huang wrote:
> This patch adds migration test support for aarch64. The test code, which
> implements the same functionality as x86, is booted as a kernel in qemu.
> Here are the design choices we make for aarch64:
>
> * We choose this -kernel approach because aarch64 QEMU doesn't provide a
>built-in fw like x86 does. So instead of relying on a boot loader, we
>use -kernel approach for aarch64.
> * The serial output is sent to PL011 directly.
> * The physical memory base for mach-virt machine is 0x4000. We change
>the start_address and end_address for aarch64.
>
> In addition to providing the binary, this patch also includes the source
> code and the build script in tests/migration/. So users can change the
> source and/or re-compile the binary as they wish.
>
> Signed-off-by: Wei Huang
> ---
> tests/Makefile.include | 1 +
> tests/migration-test.c | 47 +---
> tests/migration/Makefile | 12 +-
> tests/migration/aarch64-a-b-kernel.S | 71
>
> tests/migration/aarch64-a-b-kernel.h | 19 ++
> tests/migration/migration-test.h | 5 +++
> 6 files changed, 147 insertions(+), 8 deletions(-)
> create mode 100644 tests/migration/aarch64-a-b-kernel.S
> create mode 100644 tests/migration/aarch64-a-b-kernel.h
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index a1bcbffe12..df9f64438f 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -372,6 +372,7 @@ check-qtest-arm-y += tests/sdhci-test$(EXESUF)
> check-qtest-aarch64-y = tests/numa-test$(EXESUF)
> check-qtest-aarch64-y += tests/sdhci-test$(EXESUF)
> check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
> +check-qtest-aarch64-y += tests/migration-test$(EXESUF)
>
> check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index e2e06ed337..a4f6732a59 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -11,6 +11,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include
>
> #include "libqtest.h"
> #include "qapi/qmp/qdict.h"
> @@ -23,8 +24,8 @@
>
> #include "migration/migration-test.h"
>
> -const unsigned start_address = TEST_MEM_START;
> -const unsigned end_address = TEST_MEM_END;
> +unsigned start_address = TEST_MEM_START;
> +unsigned end_address = TEST_MEM_END;
> bool got_stop;
>
> #if defined(__linux__)
> @@ -81,12 +82,13 @@ static const char *tmpfs;
> * outputting a 'B' every so often if it's still running.
> */
> #include "tests/migration/x86-a-b-bootblock.h"
> +#include "tests/migration/aarch64-a-b-kernel.h"
>
> -static void init_bootfile_x86(const char *bootpath)
> +static void init_bootfile(const char *bootpath, void *content)
> {
> FILE *bootfile = fopen(bootpath, "wb");
>
> -g_assert_cmpint(fwrite(x86_bootsect, 512, 1, bootfile), ==, 1);
> +g_assert_cmpint(fwrite(content, 512, 1, bootfile), ==, 1);
> fclose(bootfile);
> }
>
> @@ -393,7 +395,7 @@ static void test_migrate_start(QTestState **from,
> QTestState **to,
> got_stop = false;
>
> if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> -init_bootfile_x86(bootpath);
> +init_bootfile(bootpath, x86_bootsect);
> cmd_src = g_strdup_printf("-machine accel=%s -m 150M"
>" -name source,debug-threads=on"
>" -serial file:%s/src_serial"
> @@ -422,6 +424,39 @@ static void test_migrate_start(QTestState **from,
> QTestState **to,
>" -serial file:%s/dest_serial"
>" -incoming %s",
>accel, tmpfs, uri);
> +} else if (strcmp(arch, "aarch64") == 0) {
> +const char *cpu;
> +const char *gic_ver;
> +struct utsname utsname;
> +
> +/* kvm and tcg need different cpu and gic-version configs */
> +if (access("/dev/kvm", F_OK) == 0 && uname(&utsname) == 0 &&
> +strcmp(utsname.machine, "aarch64") == 0) {
> +accel = "kvm";
> +cpu = "host";
> +gic_ver = "host";
> +} else {
> +accel = "tcg";
> +cpu = "cortex-a57";
> +gic_ver = "2";
> +}
> +
> +init_bootfile(bootpath, aarch64_kernel);
> +cmd_src = g_strdup_printf("-machine virt,accel=%s,gic-version=%s "
> + "-name vmsource,debug-threads=on -cpu %s "
> + "-m 150M -serial file:%s/src_serial "
> + "-kernel %s ",
> + accel, gic_ver, cpu, tmpfs, bootpath);
> +cmd_dst = g_strdup_printf("-machine virt,accel=%s,gic-version=%s "
> + "-name vmdest,debug-threads=on -cpu %s "
> +
Re: [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4)
ping? :) On 02/15/2018 07:05 PM, Philippe Mathieu-Daudé wrote: > Since v3: > - use assert() in sd_state_name() and sd_response_name() (Alistair review) > - added sdmmc-internal.h & sdmmc-common.c to reuse helpers with hw/sd/core.c > > Since v2: > - split again in 2... this part is cleanup/tracing > - add more tracepoints > - move some code reusable by sdbus in sdmmc-internal.h > > Since v1: > - rewrote mostly all patches to keep it simpler. > > $ git backport-diff > 001/11:[] [--] 'sdcard: reorder SDState struct members' > 002/11:[0003] [FC] 'sdcard: replace DPRINTF() by trace events' > 003/11:[0001] [FC] 'sdcard: add a trace event for command responses' > 004/11:[0007] [FC] 'sdcard: replace fprintf() by qemu_hexdump()' > 005/11:[] [--] 'sdcard: add more trace events' > 006/11:[] [--] 'sdcard: do not trace CMD55 when expecting ACMD' > 007/11:[0014] [FC] 'sdcard: define SDMMC_CMD_MAX instead of using the magic > '64'' > 008/11:[0020] [FC] 'sdcard: display command name when tracing CMD/ACMD' > 009/11:[] [--] 'sdcard: display protocol used when tracing' > 010/11:[] [-C] 'sdcard: use G_BYTE from cutils' > 011/11:[] [-C] 'sdcard: use the registerfields API to access the OCR > register' > > Philippe Mathieu-Daudé (11): > sdcard: reorder SDState struct members > sdcard: replace DPRINTF() by trace events > sdcard: add a trace event for command responses > sdcard: replace fprintf() by qemu_hexdump() > sdcard: add more trace events > sdcard: do not trace CMD55 when expecting ACMD > sdcard: define SDMMC_CMD_MAX instead of using the magic '64' > sdcard: display command name when tracing CMD/ACMD > sdcard: display protocol used when tracing > sdcard: use G_BYTE from cutils > sdcard: use the registerfields API to access the OCR register > > hw/sd/sdmmc-internal.h | 18 + > include/hw/sd/sd.h | 1 - > hw/sd/sd.c | 184 > ++--- > hw/sd/sdmmc-common.c | 72 +++ > hw/sd/Makefile.objs| 2 +- > hw/sd/trace-events | 20 ++ > 6 files changed, 241 insertions(+), 56 deletions(-) > create mode 100644 hw/sd/sdmmc-internal.h > create mode 100644 hw/sd/sdmmc-common.c >
Re: [Qemu-devel] [PATCH v4 00/20] SDCard: bugfixes, support UHS-I (part 5)
ping? :) On 02/15/2018 07:13 PM, Philippe Mathieu-Daudé wrote: > Some refactors, few bugfixes, better SD/SPI support. > > With this series apply, machines can use SD cards in UHS-I mode. > (mostly imported from Alistair Francis work) > > MMC mode split out for another series, > so UHS enabled MMC cards are still not usable: > > kernel: mmc0: SDHCI controller on PCI [:00:05.0] using ADMA > kernel: sd 0:0:0:0: Attached scsi generic sg0 type 0 > kernel: mmc0: Skipping voltage switch > [mmc kthread looping] > > Since v3: > - simpler SPI handling, improved descriptions (Alistair review) > - inverted patches 16/17 order > > Since v2: > - split again in 2... other part is cleanup/tracing > > Since v1: > - rewrote mostly all patches to keep it simpler. > > $ git backport-diff > 001/20:[] [-C] 'sdcard: Don't always set the high capacity bit' > 002/20:[] [-C] 'sdcard: update the CSD CRC register regardless the CSD > structure version' > 003/20:[] [-C] 'sdcard: fix the 'maximum data transfer rate' to 25MHz' > 004/20:[] [-C] 'sdcard: clean the SCR register and add few comments' > 005/20:[] [--] 'sdcard: remove commands from unsupported old MMC > specification' > 006/20:[] [--] 'sdcard: simplify using the ldst API' > 007/20:[0008] [FC] 'sdcard: use the correct masked OCR in the R3 reply' > 008/20:[] [-C] 'sdcard: use the registerfields API for the CARD_STATUS > register masks' > 009/20:[] [--] 'sdcard: handle CMD54 (SDIO)' > 010/20:[down] 'sdcard: handle the Security Specification commands' > 011/20:[down] 'sdcard: use a more descriptive label 'unimplemented_spi_cmd'' > 012/20:[0034] [FC] 'sdcard: handles more commands in SPI mode' > 013/20:[] [--] 'sdcard: check the card is in correct state for APP CMD > (CMD55)' > 014/20:[] [--] 'sdcard: warn if host uses an incorrect address for APP > CMD (CMD55)' > 015/20:[] [--] 'sdcard: simplify SEND_IF_COND (CMD8)' > 016/20:[] [--] 'sdcard: simplify SD_SEND_OP_COND (ACMD41)' > 017/20:[] [--] 'sdcard: add SD SEND_TUNING_BLOCK (CMD19)' > 018/20:[] [--] 'sdcard: implement the UHS-I SWITCH_FUNCTION entries (Spec > v3)' > 019/20:[] [-C] 'sdcard: add a 'uhs' property, update the OCR register > ACCEPT_SWITCH_1V8 bit' > 020/20:[] [--] 'sdcard: add an enum for the SD PHY Spec version' > > Based-on: 20180215220540.6556-12-f4...@amsat.org > > Philippe Mathieu-Daudé (20): > sdcard: Don't always set the high capacity bit > sdcard: update the CSD CRC register regardless the CSD structure > version > sdcard: fix the 'maximum data transfer rate' to 25MHz > sdcard: clean the SCR register and add few comments > sdcard: remove commands from unsupported old MMC specification > sdcard: simplify using the ldst API > sdcard: use the correct masked OCR in the R3 reply > sdcard: use the registerfields API for the CARD_STATUS register masks > sdcard: handle CMD54 (SDIO) > sdcard: handle the Security Specification commands > sdcard: use a more descriptive label 'unimplemented_spi_cmd' > sdcard: handles more commands in SPI mode > sdcard: check the card is in correct state for APP CMD (CMD55) > sdcard: warn if host uses an incorrect address for APP CMD (CMD55) > sdcard: simplify SEND_IF_COND (CMD8) > sdcard: simplify SD_SEND_OP_COND (ACMD41) > sdcard: add SD SEND_TUNING_BLOCK (CMD19) > sdcard: implement the UHS-I SWITCH_FUNCTION entries (Spec v3) > sdcard: add a 'uhs' property, update the OCR register > ACCEPT_SWITCH_1V8 bit > sdcard: add an enum for the SD PHY Spec version > > hw/sd/sd.c | 498 > - > hw/sd/trace-events | 1 + > 2 files changed, 343 insertions(+), 156 deletions(-) >
[Qemu-devel] [PATCH] net: Move the toeplitz functions from checksum.h to net_rx_pkt.c
The functions are only used in this single .c file, so there is
no need to put all this code in a header that is included from
multiple places.
Signed-off-by: Thomas Huth
---
hw/net/net_rx_pkt.c| 44
include/net/checksum.h | 44
2 files changed, 44 insertions(+), 44 deletions(-)
diff --git a/hw/net/net_rx_pkt.c b/hw/net/net_rx_pkt.c
index 98a5030..f66beb3 100644
--- a/hw/net/net_rx_pkt.c
+++ b/hw/net/net_rx_pkt.c
@@ -48,6 +48,50 @@ struct NetRxPkt {
eth_l4_hdr_info l4hdr_info;
};
+typedef struct toeplitz_key_st {
+uint32_t leftmost_32_bits;
+uint8_t *next_byte;
+} net_toeplitz_key;
+
+static inline
+void net_toeplitz_key_init(net_toeplitz_key *key, uint8_t *key_bytes)
+{
+key->leftmost_32_bits = be32_to_cpu(*(uint32_t *)key_bytes);
+key->next_byte = key_bytes + sizeof(uint32_t);
+}
+
+static inline
+void net_toeplitz_add(uint32_t *result,
+ uint8_t *input,
+ uint32_t len,
+ net_toeplitz_key *key)
+{
+register uint32_t accumulator = *result;
+register uint32_t leftmost_32_bits = key->leftmost_32_bits;
+register uint32_t byte;
+
+for (byte = 0; byte < len; byte++) {
+register uint8_t input_byte = input[byte];
+register uint8_t key_byte = *(key->next_byte++);
+register uint8_t bit;
+
+for (bit = 0; bit < 8; bit++) {
+if (input_byte & (1 << 7)) {
+accumulator ^= leftmost_32_bits;
+}
+
+leftmost_32_bits =
+(leftmost_32_bits << 1) | ((key_byte & (1 << 7)) >> 7);
+
+input_byte <<= 1;
+key_byte <<= 1;
+}
+}
+
+key->leftmost_32_bits = leftmost_32_bits;
+*result = accumulator;
+}
+
void net_rx_pkt_init(struct NetRxPkt **pkt, bool has_virt_hdr)
{
struct NetRxPkt *p = g_malloc0(sizeof *p);
diff --git a/include/net/checksum.h b/include/net/checksum.h
index 05a0d27..77a56c1 100644
--- a/include/net/checksum.h
+++ b/include/net/checksum.h
@@ -59,48 +59,4 @@ uint32_t net_checksum_add_iov(const struct iovec *iov,
uint32_t iov_off, uint32_t size,
uint32_t csum_offset);
-typedef struct toeplitz_key_st {
-uint32_t leftmost_32_bits;
-uint8_t *next_byte;
-} net_toeplitz_key;
-
-static inline
-void net_toeplitz_key_init(net_toeplitz_key *key, uint8_t *key_bytes)
-{
-key->leftmost_32_bits = be32_to_cpu(*(uint32_t *)key_bytes);
-key->next_byte = key_bytes + sizeof(uint32_t);
-}
-
-static inline
-void net_toeplitz_add(uint32_t *result,
- uint8_t *input,
- uint32_t len,
- net_toeplitz_key *key)
-{
-register uint32_t accumulator = *result;
-register uint32_t leftmost_32_bits = key->leftmost_32_bits;
-register uint32_t byte;
-
-for (byte = 0; byte < len; byte++) {
-register uint8_t input_byte = input[byte];
-register uint8_t key_byte = *(key->next_byte++);
-register uint8_t bit;
-
-for (bit = 0; bit < 8; bit++) {
-if (input_byte & (1 << 7)) {
-accumulator ^= leftmost_32_bits;
-}
-
-leftmost_32_bits =
-(leftmost_32_bits << 1) | ((key_byte & (1 << 7)) >> 7);
-
-input_byte <<= 1;
-key_byte <<= 1;
-}
-}
-
-key->leftmost_32_bits = leftmost_32_bits;
-*result = accumulator;
-}
-
#endif /* QEMU_NET_CHECKSUM_H */
--
1.8.3.1
Re: [Qemu-devel] [RFC PATCH v3 5/7] hw/sd/pl181: expose a SDBus and connect the SDCard to it
ping? :)
On 02/15/2018 11:29 PM, Philippe Mathieu-Daudé wrote:
> using the sdbus_*() API.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
>
> RFC because how pl181_sdbus_create_inplace() doing class_init(SDBus) in
> realize(pl181) seems weird...
>
> from http://lists.nongnu.org/archive/html/qemu-devel/2018-02/msg01268.html:
>
> I think you have to change that sd_set_cb() code now.
> If you look at sd_cardchange() it uses "is this SD card
> object on an SDBus" to determine whether to notify the
> controller via the old-API IRQ lines, or using the
> set_inserted() and set_readonly() callbacks on the SDBusClass.
>
> This previous series intended to enforce a cleaner OOP design, adding a
> TYPE_SD_BUS_MASTER_INTERFACE TypeInfo:
>
> http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg02322.html
> http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg02326.html
> http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg02327.html
> ---
>
> hw/sd/pl181.c | 59
> ---
> 1 file changed, 44 insertions(+), 15 deletions(-)
>
> diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
> index 3ba1f7dd23..97e85e4a64 100644
> --- a/hw/sd/pl181.c
> +++ b/hw/sd/pl181.c
> @@ -33,7 +33,7 @@ typedef struct PL181State {
> SysBusDevice parent_obj;
>
> MemoryRegion iomem;
> -SDState *card;
> +SDBus sdbus;
> uint32_t clock;
> uint32_t power;
> uint32_t cmdarg;
> @@ -179,7 +179,7 @@ static void pl181_send_command(PL181State *s)
> request.cmd = s->cmd & PL181_CMD_INDEX;
> request.arg = s->cmdarg;
> DPRINTF("Command %d %08x\n", request.cmd, request.arg);
> -rlen = sd_do_command(s->card, &request, response);
> +rlen = sdbus_do_command(&s->sdbus, &request, response);
> if (rlen < 0)
> goto error;
> if (s->cmd & PL181_CMD_RESPONSE) {
> @@ -223,12 +223,12 @@ static void pl181_fifo_run(PL181State *s)
> int is_read;
>
> is_read = (s->datactrl & PL181_DATA_DIRECTION) != 0;
> -if (s->datacnt != 0 && (!is_read || sd_data_ready(s->card))
> +if (s->datacnt != 0 && (!is_read || sdbus_data_ready(&s->sdbus))
> && !s->linux_hack) {
> if (is_read) {
> n = 0;
> while (s->datacnt && s->fifo_len < PL181_FIFO_LEN) {
> -value |= (uint32_t)sd_read_data(s->card) << (n * 8);
> +value |= (uint32_t)sdbus_read_data(&s->sdbus) << (n * 8);
> s->datacnt--;
> n++;
> if (n == 4) {
> @@ -249,7 +249,7 @@ static void pl181_fifo_run(PL181State *s)
> }
> n--;
> s->datacnt--;
> -sd_write_data(s->card, value & 0xff);
> +sdbus_write_data(&s->sdbus, value & 0xff);
> value >>= 8;
> }
> }
> @@ -477,13 +477,6 @@ static void pl181_reset(DeviceState *d)
> s->linux_hack = 0;
> s->mask[0] = 0;
> s->mask[1] = 0;
> -
> -/* We can assume our GPIO outputs have been wired up now */
> -sd_set_cb(s->card, s->cardstatus[0], s->cardstatus[1]);
> -/* Since we're still using the legacy SD API the card is not plugged
> - * into any bus, and we must reset it manually.
> - */
> -device_reset(DEVICE(s->card));
> }
>
> static void pl181_init(Object *obj)
> @@ -499,16 +492,52 @@ static void pl181_init(Object *obj)
> qdev_init_gpio_out(dev, s->cardstatus, 2);
> }
>
> +static void pl181_set_readonly(DeviceState *dev, bool level)
> +{
> +PL181State *s = (PL181State *)dev;
> +
> +qemu_set_irq(s->cardstatus[0], level);
> +}
> +
> +static void pl181_set_inserted(DeviceState *dev, bool level)
> +{
> +PL181State *s = (PL181State *)dev;
> +
> +qemu_set_irq(s->cardstatus[1], level);
> +}
> +
> +static void pl181_sdbus_create_inplace(SDBus *sdbus, DeviceState *dev)
> +{
> +SDBusClass *sbc;
> +
> +qbus_create_inplace(sdbus, sizeof(SDBus), TYPE_SD_BUS, dev, "sd-bus");
> +
> +/* pl181_sdbus_class_init */
> +sbc = SD_BUS_GET_CLASS(sdbus);
> +sbc->set_inserted = pl181_set_inserted;
> +sbc->set_readonly = pl181_set_readonly;
> +}
> +
> static void pl181_realize(DeviceState *dev, Error **errp)
> {
> PL181State *s = PL181(dev);
> +DeviceState *carddev;
> DriveInfo *dinfo;
> +Error *err = NULL;
> +
> +pl181_sdbus_create_inplace(&s->sdbus, dev);
>
> +/* Create and plug in the sd card */
> /* FIXME use a qdev drive property instead of drive_get_next() */
> dinfo = drive_get_next(IF_SD);
> -s->card = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, false);
> -if (s->card == NULL) {
> -error_setg(errp, "sd_init failed");
> +carddev = qdev_create(&s->sdbus.qbus, TYPE_SD_CARD);
> +if (dinfo) {
> +qdev_prop_set_drive(carddev, "drive", blk_by_legacy_dinfo(dinfo),
> &err);
> +}
> +object_property_set_bool(OBJECT(carddev), true
Re: [Qemu-devel] [PATCH V4 2/3] tests/migration: Add migration-test header file
On Wed, Feb 21, 2018 at 10:44:16PM -0600, Wei Huang wrote:
> This patch moves the settings related migration-test from the
> migration-test.c file to a seperate header file. It also renames the
> x86-a-b-bootblock.s file extension from .s to .S, allowing gcc
> pre-processor to include the C-style header file correctly.
>
> Signed-off-by: Wei Huang
> ---
> tests/migration-test.c| 9 +
> tests/migration/Makefile | 4 ++--
> tests/migration/migration-test.h | 19
> +++
> .../{x86-a-b-bootblock.s => x86-a-b-bootblock.S} | 7 ---
> tests/migration/x86-a-b-bootblock.h | 2 +-
> 5 files changed, 31 insertions(+), 10 deletions(-)
> create mode 100644 tests/migration/migration-test.h
> rename tests/migration/{x86-a-b-bootblock.s => x86-a-b-bootblock.S} (94%)
>
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index 74f9361bdd..e2e06ed337 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -21,10 +21,10 @@
> #include "sysemu/sysemu.h"
> #include "hw/nvram/chrp_nvram.h"
>
> -#define MIN_NVRAM_SIZE 8192 /* from spapr_nvram.c */
> +#include "migration/migration-test.h"
>
> -const unsigned start_address = 1024 * 1024;
> -const unsigned end_address = 100 * 1024 * 1024;
> +const unsigned start_address = TEST_MEM_START;
> +const unsigned end_address = TEST_MEM_END;
> bool got_stop;
>
> #if defined(__linux__)
> @@ -278,7 +278,8 @@ static void check_guests_ram(QTestState *who)
> qtest_memread(who, start_address, &first_byte, 1);
> last_byte = first_byte;
>
> -for (address = start_address + 4096; address < end_address; address +=
> 4096)
> +for (address = start_address + TEST_MEM_PAGE_SIZE; address < end_address;
> + address += TEST_MEM_PAGE_SIZE)
> {
> uint8_t b;
> qtest_memread(who, address, &b, 1);
There are several 1M / 100M comments in this file. Maybe not worth
changing, but now that we've defined those values, it seems they
should be rewritten more generically.
> diff --git a/tests/migration/Makefile b/tests/migration/Makefile
> index 1c07dd7be9..b768d0729d 100644
> --- a/tests/migration/Makefile
> +++ b/tests/migration/Makefile
> @@ -27,8 +27,8 @@ endef
>
> all: x86-a-b-bootblock.h
>
> -x86-a-b-bootblock.h: x86-a-b-bootblock.s
> - $(x86_64_cross_prefix)as --32 -march=i486 $< -o x86.o
> +x86-a-b-bootblock.h: x86-a-b-bootblock.S
> + $(x86_64_cross_prefix)gcc -m32 -march=i486 -c $< -o x86.o
> $(x86_64_cross_prefix)objcopy -O binary x86.o x86.boot
> dd if=x86.boot of=x86.bootsect bs=256 count=2 skip=124
> echo "$$__note" > $@
> diff --git a/tests/migration/migration-test.h
> b/tests/migration/migration-test.h
> new file mode 100644
> index 00..a618fe069e
> --- /dev/null
> +++ b/tests/migration/migration-test.h
> @@ -0,0 +1,19 @@
> +/*
> + * Copyright (c) 2018 Red Hat, Inc. and/or its affiliates
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#ifndef _TEST_MIGRATION_H_
> +#define _TEST_MIGRATION_H_
> +
> +/* Common */
> +#define TEST_MEM_START (1 * 1024 * 1024)
> +#define TEST_MEM_END(100 * 1024 * 1024)
> +#define TEST_MEM_PAGE_SIZE 4096
> +
> +/* PPC */
> +#define MIN_NVRAM_SIZE 8192 /* from spapr_nvram.c */
> +
> +#endif /* _TEST_MIGRATION_H_ */
> +
trailing blank line
> diff --git a/tests/migration/x86-a-b-bootblock.s
> b/tests/migration/x86-a-b-bootblock.S
> similarity index 94%
> rename from tests/migration/x86-a-b-bootblock.s
> rename to tests/migration/x86-a-b-bootblock.S
> index 98dbfab084..08b51f9e7f 100644
> --- a/tests/migration/x86-a-b-bootblock.s
> +++ b/tests/migration/x86-a-b-bootblock.S
> @@ -12,6 +12,7 @@
> #
> # Author: dgilb...@redhat.com
>
> +#include "migration-test.h"
>
> .code16
> .org 0x7c00
> @@ -45,11 +46,11 @@ start: # at 0x7c00 ?
> mov $0, %bl
> mainloop:
> # Start from 1MB
> -mov $(1024*1024),%eax
> +mov $TEST_MEM_START,%eax
> innerloop:
> incb (%eax)
> -add $4096,%eax
> -cmp $(100*1024*1024),%eax
> +add $TEST_MEM_PAGE_SIZE,%eax
> +cmp $TEST_MEM_END,%eax
> jl innerloop
>
> inc %bl
> diff --git a/tests/migration/x86-a-b-bootblock.h
> b/tests/migration/x86-a-b-bootblock.h
> index 9e8e2e028b..44e4b99506 100644
> --- a/tests/migration/x86-a-b-bootblock.h
> +++ b/tests/migration/x86-a-b-bootblock.h
> @@ -1,5 +1,5 @@
> /* This file is automatically generated from
> - * tests/migration/x86-a-b-bootblock.s, edit that and then run
> + * tests/migration/x86-a-b-bootblock.S, edit that and then run
> * "make x86-a-b-bootblock.h" inside tests/migration to update,
> * and then remember to send both in your patch submission.
> */
> --
> 2.14.3
>
>
Otherwise,
Reviewed-by: Andrew Jones
Re: [Qemu-devel] [PATCH v3 2/5] keymap: use glib hash for kbd_layout_t
On Thu, Feb 22, 2018 at 08:05:10AM +0100, Gerd Hoffmann wrote: > Drop home-grown lookup code, which is a strange mix of a lookup table > and a list. Use standard glib hash instead. > > Signed-off-by: Gerd Hoffmann > --- > ui/keymaps.c| 73 > - > ui/trace-events | 2 +- > 2 files changed, 32 insertions(+), 43 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH V4 1/3] tests/migration: Convert the boot block compilation script into Makefile
On Wed, Feb 21, 2018 at 10:44:15PM -0600, Wei Huang wrote:
> The x86 boot block header currently is generated with a shell script.
> To better support other CPUs (e.g. aarch64), we convert the script
> into Makefile. This allows us to 1) support cross-compilation easily,
> and 2) avoid creating a script file for every architecture.
>
> Signed-off-by: Wei Huang
> ---
> tests/migration/Makefile | 38
>
> tests/migration/rebuild-x86-bootblock.sh | 33 ---
> tests/migration/x86-a-b-bootblock.h | 2 +-
> tests/migration/x86-a-b-bootblock.s | 5 ++---
> 4 files changed, 41 insertions(+), 37 deletions(-)
> create mode 100644 tests/migration/Makefile
> delete mode 100755 tests/migration/rebuild-x86-bootblock.sh
>
> diff --git a/tests/migration/Makefile b/tests/migration/Makefile
> new file mode 100644
> index 00..1c07dd7be9
> --- /dev/null
> +++ b/tests/migration/Makefile
> @@ -0,0 +1,38 @@
> +#
> +# Copyright (c) 2016-2018 Red Hat, Inc. and/or its affiliates
> +#
> +# Authors:
> +# Dave Gilbert
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
> +# See the COPYING file in the top-level directory.
> +#
> +path := $(subst :, ,$(PATH))
> +system := $(shell uname -s | tr "A-Z" "a-z")
> +
> +cross-ld = $(firstword $(wildcard $(patsubst
> %,%/$(1)-*$(system)*-ld,$(path
> +cross-gcc = $(firstword $(wildcard $(patsubst %ld,%gcc,$(call
> cross-ld,$(1)
> +find-cross-prefix = $(subst gcc,,$(notdir $(call cross-gcc,$(1
> +
> +x86_64_cross_prefix := $(call find-cross-prefix,x86_64)
I see the above lines are copied from roms/Makefile. Is there no way to
avoid the duplication? Add them to $(SRC_PATH)/rules.mak and include them?
> +
> +export __note
> +override define __note
> +/* This file is automatically generated from
> + * tests/migration/$<, edit that and then run
> + * "make $@" inside tests/migration to update,
> + * and then remember to send both in your patch submission.
> + */
> +endef
> +
> +all: x86-a-b-bootblock.h
> +
> +x86-a-b-bootblock.h: x86-a-b-bootblock.s
> + $(x86_64_cross_prefix)as --32 -march=i486 $< -o x86.o
> + $(x86_64_cross_prefix)objcopy -O binary x86.o x86.boot
> + dd if=x86.boot of=x86.bootsect bs=256 count=2 skip=124
> + echo "$$__note" > $@
> + xxd -i x86.bootsect | sed -e 's/.*int.*//' >> $@
> +
> +clean:
> + rm -f *.bootsect *.boot *.o
> diff --git a/tests/migration/rebuild-x86-bootblock.sh
> b/tests/migration/rebuild-x86-bootblock.sh
> deleted file mode 100755
> index 86cec5d284..00
> --- a/tests/migration/rebuild-x86-bootblock.sh
> +++ /dev/null
> @@ -1,33 +0,0 @@
> -#!/bin/sh
> -# Copyright (c) 2016-2018 Red Hat, Inc. and/or its affiliates
> -# This work is licensed under the terms of the GNU GPL, version 2 or later.
> -# See the COPYING file in the top-level directory.
> -#
> -# Author: dgilb...@redhat.com
> -
> -ASMFILE=$PWD/tests/migration/x86-a-b-bootblock.s
> -HEADER=$PWD/tests/migration/x86-a-b-bootblock.h
> -
> -if [ ! -e "$ASMFILE" ]
> -then
> - echo "Couldn't find $ASMFILE" >&2
> - exit 1
> -fi
> -
> -ASM_WORK_DIR=$(mktemp -d --tmpdir X86BB.XX)
> -cd "$ASM_WORK_DIR" &&
> -as --32 -march=i486 "$ASMFILE" -o x86.o &&
> -objcopy -O binary x86.o x86.boot &&
> -dd if=x86.boot of=x86.bootsect bs=256 count=2 skip=124 &&
> -xxd -i x86.bootsect |
> -sed -e 's/.*int.*//' > x86.hex &&
> -cat - x86.hex < "$HEADER"
> -/* This file is automatically generated from
> - * tests/migration/x86-a-b-bootblock.s, edit that and then run
> - * tests/migration/rebuild-x86-bootblock.sh to update,
> - * and then remember to send both in your patch submission.
> - */
> -HERE
> -
> -rm x86.hex x86.bootsect x86.boot x86.o
> -cd .. && rmdir "$ASM_WORK_DIR"
> diff --git a/tests/migration/x86-a-b-bootblock.h
> b/tests/migration/x86-a-b-bootblock.h
> index 78a151fe2a..9e8e2e028b 100644
> --- a/tests/migration/x86-a-b-bootblock.h
> +++ b/tests/migration/x86-a-b-bootblock.h
> @@ -1,6 +1,6 @@
> /* This file is automatically generated from
> * tests/migration/x86-a-b-bootblock.s, edit that and then run
> - * tests/migration/rebuild-x86-bootblock.sh to update,
> + * "make x86-a-b-bootblock.h" inside tests/migration to update,
> * and then remember to send both in your patch submission.
> */
> unsigned char x86_bootsect[] = {
> diff --git a/tests/migration/x86-a-b-bootblock.s
> b/tests/migration/x86-a-b-bootblock.s
> index b1642641a7..98dbfab084 100644
> --- a/tests/migration/x86-a-b-bootblock.s
> +++ b/tests/migration/x86-a-b-bootblock.s
> @@ -3,9 +3,8 @@
> # range.
> # Outputs an initial 'A' on serial followed by repeated 'B's
> #
> -# run tests/migration/rebuild-x86-bootblock.sh
> -# to regenerate the hex, and remember to include both the .h and .s
> -# in any patches.
> +# In tests/migration dir, run 'make x86-a-b-bootblock.h' to regenerate
> +# the hex, and remember to include both the .h and .s in any patch
Re: [Qemu-devel] [PATCH v1 2/3] s390x/sclp: clean up sclp masks
On Wed, 21 Feb 2018 18:30:12 +0100 Cornelia Huck wrote: > On Wed, 21 Feb 2018 17:42:57 +0100 > Claudio Imbrenda wrote: > > > On Wed, 21 Feb 2018 16:20:05 +0100 > > Cornelia Huck wrote: > > > > > On Tue, 20 Feb 2018 19:45:01 +0100 > > > Claudio Imbrenda wrote: > > > > > diff --git a/include/hw/s390x/event-facility.h > > > > b/include/hw/s390x/event-facility.h index 5119b9b..0a8b47a > > > > 100644 --- a/include/hw/s390x/event-facility.h > > > > +++ b/include/hw/s390x/event-facility.h > > > > @@ -28,12 +28,14 @@ > > > > #define SCLP_EVENT_SIGNAL_QUIESCE 0x1d > > > > > > > > /* SCLP event masks */ > > > > -#define SCLP_EVENT_MASK_SIGNAL_QUIESCE 0x0008 > > > > -#define SCLP_EVENT_MASK_MSG_ASCII 0x0040 > > > > -#define SCLP_EVENT_MASK_CONFIG_MGT_DATA 0x1000 > > > > -#define SCLP_EVENT_MASK_OP_CMD 0x8000 > > > > -#define SCLP_EVENT_MASK_MSG 0x4000 > > > > -#define SCLP_EVENT_MASK_PMSGCMD 0x0080 > > > > +#define SCLPEVMSK(T) (1ULL << (sizeof(sccb_mask_t) * 8 - > > > > (T))) > > > > > > SCLP_EVMASK() would be a bit more readable, I think. > > > > I know, but then it looks ugly when trying to fit everything in 80 > > columns. > > I'd rather go slightly over 80 in that case (as long as you don't > cross 90). will do > > > > > > + > > > > +#define SCLP_EVENT_MASK_OP_CMD > > > > SCLPEVMSK(SCLP_EVENT_OPRTNS_COMMAND) +#define > > > > SCLP_EVENT_MASK_MSG SCLPEVMSK(SCLP_EVENT_MESSAGE) > > > > +#define SCLP_EVENT_MASK_CONFIG_MGT_DATA > > > > SCLPEVMSK(SCLP_EVENT_CONFIG_MGT_DATA) +#define > > > > SCLP_EVENT_MASK_PMSGCMD SCLPEVMSK(SCLP_EVENT_PMSGCMD) > > > > +#define SCLP_EVENT_MASK_MSG_ASCII > > > > SCLPEVMSK(SCLP_EVENT_ASCII_CONSOLE_DATA) +#define > > > > SCLP_EVENT_MASK_SIGNAL_QUIESCE > > > > SCLPEVMSK(SCLP_EVENT_SIGNAL_QUIESCE) #define > > > > SCLP_UNCONDITIONAL_READ 0x00 #define > > > > SCLP_SELECTIVE_READ 0x01 > > > > > >
Re: [Qemu-devel] [PATCH v4 0/7] vfio: add display support
Hi, > Nice! Seems to be the last missing gap for local spice with cursor > dmabuf support, we'll do more testing on that for sure. Btw, another > method might be to add direct cursor dmabuf passing for spice as gl > output, is that correct way? Passing on the cursor sprite needs the hotspot information, so spice client can position the cursor correctly. I didn't got hotspot information in my testing yet, but maybe the guest kernel was too old. Is that supported meanwhile? If so, which kernel version is needed? Spice already has support for setting pointers, we can simply use that, at least initially. Doesn't use dma-bufs. If it turns out to be a performance issue we can add support for using dma-bufs instead, but that needs spice server changes. > And really like to see dmabuf support could be in upstream soon. Do > you have any predict for which qemu version? I want have this in 2.12, at least the current, initial version. Improved cursor support might take a little longer. cheers, Gerd
Re: [Qemu-devel] [PATCH v1 1/3] s390x/sclp: proper support of larger send and receive masks
On Wed, 21 Feb 2018 18:06:36 +0100
Cornelia Huck wrote:
> On Wed, 21 Feb 2018 17:28:49 +0100
> Claudio Imbrenda wrote:
>
> > On Wed, 21 Feb 2018 16:12:59 +0100
> > Cornelia Huck wrote:
> >
> > > On Tue, 20 Feb 2018 19:45:00 +0100
> > > Claudio Imbrenda wrote:
> > >
> > > > The architecture allows the guests to ask for masks up to 1021
> > > > bytes in length. Part was fixed in
> > > > 67915de9f0383ccf4ab8c42dd02aa18dcd79b411 ("s390x/event-facility:
> > > > variable-length event masks"), but some issues were still
> > > > remaining, in particular regarding the handling of selective
> > > > reads.
> > > >
> > > > This patch fixes the handling of selective reads, whose size
> > > > will now match the length of the event mask, as per
> > > > architecture.
> > > >
> > > > The default behaviour is to be compliant with the architecture,
> > > > but when using older machine models the old behaviour is
> > > > selected, in order to be able to migrate toward older
> > > > versions.
> > >
> > > I remember trying to do this back when I still had access to the
> > > architecture, but somehow never finished this (don't remember
> > > why).
> > > >
> > > > Fixes: 67915de9f0383ccf4a ("s390x/event-facility:
> > > > variable-length event masks")
> > >
> > > Doesn't that rather fix the initial implementation? What am I
> > > missing?
> >
> > well that too. but I think this fixes the fix, since the fix was
> > incomplete?
> >
> > > > Signed-off-by: Claudio Imbrenda
> > > > ---
> > > > hw/s390x/event-facility.c | 90
> > > > +++---
> > > > hw/s390x/s390-virtio-ccw.c | 8 - 2 files changed, 84
> > > > insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/hw/s390x/event-facility.c
> > > > b/hw/s390x/event-facility.c index 155a694..2414614 100644
> > > > --- a/hw/s390x/event-facility.c
> > > > +++ b/hw/s390x/event-facility.c
> > > > @@ -31,6 +31,14 @@ struct SCLPEventFacility {
> > > > SCLPEventsBus sbus;
> > > > /* guest' receive mask */
> > > > unsigned int receive_mask;
> > > > +/*
> > > > + * when false, we keep the same broken, backwards
> > > > compatible behaviour as
> > > > + * before; when true, we implement the architecture
> > > > correctly. Needed for
> > > > + * migration toward older versions.
> > > > + */
> > > > +bool allow_all_mask_sizes;
> > >
> > > The comment does not really tell us what the old behaviour
> > > is ;)
> >
> > hmm, I'll fix that
> >
> > > So, if this is about extending the mask size, call this
> > > "allow_large_masks" or so?
> >
> > no, the old broken behaviour allowed only _exactly_ 4 bytes:
> >
> > if (be16_to_cpu(we_mask->mask_length) != 4) {
> > sccb->h.response_code =
> > cpu_to_be16(SCLP_RC_INVALID_MASK_LENGTH); goto out;
> > }
>
> Hm, I can't seem to find this in the code?
>
> >
> > With the 67915de9f0383ccf4a patch mentioned above, we allow any
> > size, but we don't keep track of the size itself, only the bits.
> > This is a problem for selective reads (see below)
>
> Oh, you meant before that patch...
yes
> >
> > > > +/* length of the receive mask */
> > > > +uint16_t mask_length;
> > > > };
> > > >
> > > > /* return true if any child has event pending set */
> > > > @@ -220,6 +228,17 @@ static uint16_t
> > > > handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb,
> > > > return rc; }
> > > >
> > > > +/* copy up to dst_len bytes and fill the rest of dst with
> > > > zeroes */ +static void copy_mask(uint8_t *dst, uint8_t *src,
> > > > uint16_t dst_len,
> > > > + uint16_t src_len)
> > > > +{
> > > > +int i;
> > > > +
> > > > +for (i = 0; i < dst_len; i++) {
> > > > +dst[i] = i < src_len ? src[i] : 0;
> > > > +}
> > > > +}
> > > > +
> > > > static void read_event_data(SCLPEventFacility *ef, SCCB *sccb)
> > > > {
> > > > unsigned int sclp_active_selection_mask;
> > > > @@ -240,7 +259,9 @@ static void
> > > > read_event_data(SCLPEventFacility *ef, SCCB *sccb)
> > > > sclp_active_selection_mask = sclp_cp_receive_mask; break;
> > > > case SCLP_SELECTIVE_READ:
> > > > -sclp_active_selection_mask = be32_to_cpu(red->mask);
> > > > +copy_mask((uint8_t *)&sclp_active_selection_mask,
> > > > (uint8_t *)&red->mask,
> > > > + sizeof(sclp_active_selection_mask),
> > > > ef->mask_length);
> > > > +sclp_active_selection_mask =
> > > > be32_to_cpu(sclp_active_selection_mask);
> > >
> > > Hm, this looks like a real bug fix for the commit referenced
> > > above. Split this out? We don't need compat support for that;
> > > maybe even cc:stable?
> >
> > I think we do. We can avoid keeping track of the mask size when
> > setting the mask size, because we can simply take the bits we need
> > and ignore the rest. But for selective reads we need the mask size,
> > so we have to keep track of it. (selective reads specify
Re: [Qemu-devel] [Xen-devel] [PATCH] intel_iommu: allow updating FEADDR and FEUADDR with one 64bit write
On Wed, Jan 24, 2018 at 03:18:48PM +0100, Marek Marczykowski-Górecki wrote: > Allow updating those two adjacent 32bit fields with one 64bit write. > This fixes qemu crash when booting Xen inside. > > See discussion on Xen side of the thing here: > http://xen.markmail.org/message/6mrmemrnmhxvaxba Xen code is wrong, see: https://marc.info/?l=xen-devel&m=150511273303712 Roger.
Re: [Qemu-devel] [PATCH v2 10/36] test-qemu-opts: Test qemu_opts_to_qdict_filtered()
Am 21.02.2018 um 21:57 hat Eric Blake geschrieben:
> On 02/21/2018 07:53 AM, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf
> > ---
> > tests/test-qemu-opts.c | 125
> > +
> > 1 file changed, 125 insertions(+)
> >
> > diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
> > index 6c3183390b..2c422abcd4 100644
> > --- a/tests/test-qemu-opts.c
> > +++ b/tests/test-qemu-opts.c
> > @@ -10,6 +10,7 @@
> > #include "qemu/osdep.h"
> > #include "qemu/cutils.h"
> > #include "qemu/option.h"
> > +#include "qemu/option_int.h"
> > #include "qapi/error.h"
> > #include "qapi/qmp/qdict.h"
> > #include "qapi/qmp/qstring.h"
> > @@ -868,6 +869,127 @@ static void test_opts_append(void)
> > qemu_opts_free(merged);
> > }
> > +static void test_opts_to_qdict_basic(void)
> > +{
> > +QemuOpts *opts;
> > +QDict *dict;
> > +
> > +opts = qemu_opts_parse(&opts_list_01,
> > "str1=foo,str2=,str3=bar,number1=42",
> > + false, &error_abort);
>
> Worth any additional craziness in regards to our QemuOpts parsing, like
> str1=foo,,bar,str2... for an option containing commas, or str2=,str1=foo,
> for supplying options in a different order than the list? But what you have
> is a good addition even if you don't tweak it.
This is not a test for parsing options string, but for converting an
already existing QemuOpts to a QDict. Parsing is already extensively
tested in /qemu-opts/opts_parse/*. I'm only using qemu_opts_parse() here
because it's the most convenient way to create a QemuOpts with multiple
options.
So the only things we need to consider in this test case are different
QemuOpts that result from the parsing. Escaped commas don't exist in
this representation any more and the associated QemuOptsList isn't
involved in the conversion to QDicts, so these wouldn't actually be new
cases for the thing we're testing here.
Kevin
[Qemu-devel] [PATCH] hw/net: Remove unnecessary header includes
Headers like "hw/loader.h" and "qemu/sockets.h" are not needed in
the hw/net/*.c files. And Some other headers are included via other
headers already, so we can drop them, too.
Signed-off-by: Thomas Huth
---
hw/net/e1000.c | 1 -
hw/net/lance.c | 3 ---
hw/net/ne2000.c| 2 --
hw/net/pcnet-pci.c | 1 -
hw/net/pcnet.c | 1 -
hw/net/rtl8139.c | 2 --
hw/net/xgmac.c | 1 -
7 files changed, 11 deletions(-)
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 804ec08..c7f1695 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -30,7 +30,6 @@
#include "hw/pci/pci.h"
#include "net/net.h"
#include "net/checksum.h"
-#include "hw/loader.h"
#include "sysemu/sysemu.h"
#include "sysemu/dma.h"
#include "qemu/iov.h"
diff --git a/hw/net/lance.c b/hw/net/lance.c
index 0028bc5..a08d5ac 100644
--- a/hw/net/lance.c
+++ b/hw/net/lance.c
@@ -36,10 +36,7 @@
*/
#include "qemu/osdep.h"
-#include "hw/sysbus.h"
-#include "net/net.h"
#include "qemu/timer.h"
-#include "qemu/sockets.h"
#include "hw/sparc/sparc32_dma.h"
#include "hw/net/lance.h"
#include "trace.h"
diff --git a/hw/net/ne2000.c b/hw/net/ne2000.c
index 687ef84..3a9fc89 100644
--- a/hw/net/ne2000.c
+++ b/hw/net/ne2000.c
@@ -23,10 +23,8 @@
*/
#include "qemu/osdep.h"
#include "hw/pci/pci.h"
-#include "net/net.h"
#include "net/eth.h"
#include "ne2000.h"
-#include "hw/loader.h"
#include "sysemu/sysemu.h"
/* debug NE2000 card */
diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c
index 0ae5ca4..70dc8b3 100644
--- a/hw/net/pcnet-pci.c
+++ b/hw/net/pcnet-pci.c
@@ -30,7 +30,6 @@
#include "qemu/osdep.h"
#include "hw/pci/pci.h"
#include "net/net.h"
-#include "hw/loader.h"
#include "qemu/timer.h"
#include "sysemu/dma.h"
#include "sysemu/sysemu.h"
diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
index 606b05c..0c44554 100644
--- a/hw/net/pcnet.c
+++ b/hw/net/pcnet.c
@@ -40,7 +40,6 @@
#include "net/net.h"
#include "net/eth.h"
#include "qemu/timer.h"
-#include "qemu/sockets.h"
#include "sysemu/sysemu.h"
#include "trace.h"
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 1cc95b8..46daa16 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -58,9 +58,7 @@
#include "qemu/timer.h"
#include "net/net.h"
#include "net/eth.h"
-#include "hw/loader.h"
#include "sysemu/sysemu.h"
-#include "qemu/iov.h"
/* debug RTL8139 card */
//#define DEBUG_RTL8139 1
diff --git a/hw/net/xgmac.c b/hw/net/xgmac.c
index 0843bf1..fa00156 100644
--- a/hw/net/xgmac.c
+++ b/hw/net/xgmac.c
@@ -28,7 +28,6 @@
#include "hw/sysbus.h"
#include "qemu/log.h"
#include "net/net.h"
-#include "net/checksum.h"
#ifdef DEBUG_XGMAC
#define DEBUGF_BRK(message, args...) do { \
--
1.8.3.1
Re: [Qemu-devel] [PATCH v7 09/23] monitor: allow using IO thread for parsing
On Wed, Feb 21, 2018 at 04:00:07PM +, Stefan Hajnoczi wrote:
> On Wed, Jan 24, 2018 at 01:39:43PM +0800, Peter Xu wrote:
> > @@ -4034,12 +4044,29 @@ static void sortcmdlist(void)
> > qsort((void *)info_cmds, array_num, elem_size, compare_mon_cmd);
> > }
> >
> > +static GMainContext *monitor_io_context_get(void)
> > +{
> > +return iothread_get_g_main_context(mon_global.mon_iothread);
> > +}
> > +
> > +static AioContext *monitor_aio_context_get(void)
> > +{
> > +return iothread_get_aio_context(mon_global.mon_iothread);
> > +}
>
> Please follow the X_get_Y() naming convention instead of X_Y_get(). For
> example, see qemu_get_aio_context() and iothread_get_aio_context().
Sure.
>
> > @@ -4082,11 +4109,41 @@ void error_vprintf_unless_qmp(const char *fmt,
> > va_list ap)
> > }
> > }
> >
> > +static void monitor_list_append(Monitor *mon)
> > +{
> > +qemu_mutex_lock(&monitor_lock);
> > +QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
> > +qemu_mutex_unlock(&monitor_lock);
> > +}
> > +
> > +static void monitor_qmp_setup_handlers(void *data)
>
> BH functions are usually declared like this:
>
> static void X_bh(void *opaque)
>
> This way it's immediately clear that this function is invoked as a BH.
>
> I suggest renaming the function to monitor_qmp_setup_handlers_bh().
> Using 'opaque' instead of 'data' is common, too.
Sure.
>
> > @@ -4099,24 +4156,55 @@ void monitor_init(Chardev *chr, int flags)
> > }
> >
> > if (monitor_is_qmp(mon)) {
> > -qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read,
> > monitor_qmp_read,
> > - monitor_qmp_event, NULL, mon, NULL, true);
> > qemu_chr_fe_set_echo(&mon->chr, true);
> > json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
> > +if (mon->use_io_thr) {
> > +/*
> > + * It's possible that we already have an IOWatchPoll
> > + * registered for the Chardev during chardev_init_func().
>
> When does this happen?
>
> This seems like a hack that breaks when certain -chardev options are
> used. For example, what happens if the chardev is a TCP connection with
> reconnect=5. In that case the socket will be connecting asynchronously
> and we cannot just remove the fd watch.
>
> How does this interact with TCP listen chardevs? It looks like the
> listener socket uses the main loop (see tcp_chr_disconnect()).
>
> I'm worried that the chardev layer isn't thread-safe and you haven't
> added anything to protect it or at least refuse to run in unsafe
> conditions.
Indeed, I did some more reading and noticed that the TCP typed chardev
is really special.
Firstly there can be the QIO thread that handles sync connecting when
"reconnect" is setup (I don't really understand why we only need the
threads when reconnect != 0, but anyway, I'll just assume we need the
threads). It's done in qmp_chardev_open_socket().
Secondly, TCP can support TLS or TELNET (tcp_chr_new_client() handles
the main logic of it), so there can be actually more than one GSource
created for a single TCP chardev. Meanwhile, the
chr_update_read_handler() calls never handles the re-setup of those
special GSources (TLS/TELNET), only the common GSource of TCP stream
read/write.
And the whole TCP channel is based on QIO stuff, which means I need to
add non-default context support to QIO stuff too... That's mostly
about qio_channel_add_watch(). I may need to pass in context
information, and switch to GSource for that function instead of the
old tags, just like what I did to chardev in general.
I'll think about these. I may possibly need some pre-requisite and
separated patches to fix existing problems before going on with OOB
again.
This is the worst thing I'd like to see - "surprises". :(
Thanks,
--
Peter Xu
Re: [Qemu-devel] [PATCH v3] specs/qcow2: Fix documentation of the compressed cluster descriptor
On Wed 21 Feb 2018 11:10:07 PM CET, Eric Blake wrote: >> This patch fixes several mistakes in the documentation of the >> compressed cluster descriptor: > > More things to consider, as followup patches: > > Note that both the L1 table, and the standard L2 descriptors, have a cap > on bit 55 as the maximum host offset (< 64PB). But for compressed > clusters, our maximum depends on cluster_bits, as follows: > > 512 byte cluster: bit 61 forms 'number clusters', leaving bits 60-0 for > computing the host offset. But even though this looks on the surface > like you could allocate 2EB of compressed clusters, it does not play > well with the rest of the L1/L2 system, so we should probably explicitly > document that bits 60-56 MUST be 0, if they are assigned to the 'host > offset field', making the maximum compressed cluster offset at 64PB. > > 2M cluster: bits 61-50 form 'number clusters', leaving bit 49 as the > maximum bit in the host offset (< 512 TB). But we never validate that > we don't overflow this value! I'm working on a patch. Good catch! > Meanwhile, the refcount table currently allows all the way up to bit > 63 to form an offset to a refcount block, although capping that at 55 > the way L1/L2 are capped would be reasonable (it gets weird to state > that your metadata must live below 64PB but that your data pointed to > by the metadata can live beyond that point). So it may also be worth > considering a spec patch that points out the 64PB maximum useful size, > and maybe even a comment that the maximum size may be further > constrained by the protocol layer (for example, ext4 has a 16TB cap on > individual file size). Sounds reasonable. Berto
[Qemu-devel] [PULL 8/9] keymap: record multiple keysym -> keycode mappings
Sometimes the same keysym can be created using different key
combinations. Record them all in the reverse keymap, not only
the first one.
Signed-off-by: Gerd Hoffmann
Reviewed-by: Daniel P. Berrangé
Message-id: 20180222070513.8740-5-kra...@redhat.com
---
ui/keymaps.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/ui/keymaps.c b/ui/keymaps.c
index cbdd65c767..1eba6d7057 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -29,7 +29,8 @@
#include "qemu/error-report.h"
struct keysym2code {
-uint16_t keycode;
+uint32_t count;
+uint16_t keycodes[4];
};
struct kbd_layout_t {
@@ -62,11 +63,18 @@ static void add_keysym(char *line, int keysym, int keycode,
kbd_layout_t *k)
keysym2code = g_hash_table_lookup(k->hash, GINT_TO_POINTER(keysym));
if (keysym2code) {
+if (keysym2code->count < ARRAY_SIZE(keysym2code->keycodes)) {
+keysym2code->keycodes[keysym2code->count++] = keycode;
+} else {
+warn_report("more than %zd keycodes for keysym %d",
+ARRAY_SIZE(keysym2code->keycodes), keysym);
+}
return;
}
keysym2code = g_new0(struct keysym2code, 1);
-keysym2code->keycode = keycode;
+keysym2code->keycodes[0] = keycode;
+keysym2code->count = 1;
g_hash_table_replace(k->hash, GINT_TO_POINTER(keysym), keysym2code);
trace_keymap_add(keysym, keycode, line);
}
@@ -185,7 +193,7 @@ int keysym2scancode(kbd_layout_t *k, int keysym)
return 0;
}
-return keysym2code->keycode;
+return keysym2code->keycodes[0];
}
int keycode_is_keypad(kbd_layout_t *k, int keycode)
--
2.9.3
[Qemu-devel] [PULL 4/9] egl-helpers: add alpha channel to texture format
Needed when rendering cursers which (unlike framebuffers) actually are transparent. Signed-off-by: Gerd Hoffmann Message-id: 20180220110433.20353-4-kra...@redhat.com --- ui/egl-helpers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c index 5fa60ef4e8..16dc3ded36 100644 --- a/ui/egl-helpers.c +++ b/ui/egl-helpers.c @@ -83,7 +83,7 @@ void egl_fb_setup_new_tex(egl_fb *fb, int width, int height) glGenTextures(1, &texture); glBindTexture(GL_TEXTURE_2D, texture); -glTexImage2D(GL_TEXTURE_2D, 0, GL_RGB, width, height, +glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, width, height, 0, GL_BGRA, GL_UNSIGNED_BYTE, 0); egl_fb_setup_for_tex(fb, width, height, texture, true); -- 2.9.3
[Qemu-devel] [PULL 5/9] keymap: make struct kbd_layout_t private to ui/keymaps.c
Also use kbd_layout_t pointers instead of void pointers.
Signed-off-by: Gerd Hoffmann
Reviewed-by: Daniel P. Berrangé
Message-id: 20180222070513.8740-2-kra...@redhat.com
---
ui/keymaps.h | 29 ++---
ui/keymaps.c | 32 +---
2 files changed, 31 insertions(+), 30 deletions(-)
diff --git a/ui/keymaps.h b/ui/keymaps.h
index 8757465529..17ec03387a 100644
--- a/ui/keymaps.h
+++ b/ui/keymaps.h
@@ -32,25 +32,6 @@ typedef struct {
int keysym;
} name2keysym_t;
-struct key_range {
-int start;
-int end;
-struct key_range *next;
-};
-
-#define MAX_NORMAL_KEYCODE 512
-#define MAX_EXTRA_COUNT 256
-typedef struct {
-uint16_t keysym2keycode[MAX_NORMAL_KEYCODE];
-struct {
- int keysym;
- uint16_t keycode;
-} keysym2keycode_extra[MAX_EXTRA_COUNT];
-int extra_count;
-struct key_range *keypad_range;
-struct key_range *numlock_range;
-} kbd_layout_t;
-
/* scancode without modifiers */
#define SCANCODE_KEYMASK 0xff
/* scancode without grey or up bit */
@@ -69,10 +50,12 @@ typedef struct {
#define SCANCODE_ALT0x400
#define SCANCODE_ALTGR 0x800
+typedef struct kbd_layout_t kbd_layout_t;
-void *init_keyboard_layout(const name2keysym_t *table, const char *language);
-int keysym2scancode(void *kbd_layout, int keysym);
-int keycode_is_keypad(void *kbd_layout, int keycode);
-int keysym_is_numlock(void *kbd_layout, int keysym);
+kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
+ const char *language);
+int keysym2scancode(kbd_layout_t *k, int keysym);
+int keycode_is_keypad(kbd_layout_t *k, int keycode);
+int keysym_is_numlock(kbd_layout_t *k, int keysym);
#endif /* QEMU_KEYMAPS_H */
diff --git a/ui/keymaps.c b/ui/keymaps.c
index f9762d1497..134958a197 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -28,6 +28,26 @@
#include "trace.h"
#include "qemu/error-report.h"
+#define MAX_NORMAL_KEYCODE 512
+#define MAX_EXTRA_COUNT 256
+
+struct key_range {
+int start;
+int end;
+struct key_range *next;
+};
+
+struct kbd_layout_t {
+uint16_t keysym2keycode[MAX_NORMAL_KEYCODE];
+struct {
+int keysym;
+uint16_t keycode;
+} keysym2keycode_extra[MAX_EXTRA_COUNT];
+int extra_count;
+struct key_range *keypad_range;
+struct key_range *numlock_range;
+};
+
static int get_keysym(const name2keysym_t *table,
const char *name)
{
@@ -186,15 +206,15 @@ static kbd_layout_t *parse_keyboard_layout(const
name2keysym_t *table,
}
-void *init_keyboard_layout(const name2keysym_t *table, const char *language)
+kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
+ const char *language)
{
return parse_keyboard_layout(table, language, NULL);
}
-int keysym2scancode(void *kbd_layout, int keysym)
+int keysym2scancode(kbd_layout_t *k, int keysym)
{
-kbd_layout_t *k = kbd_layout;
if (keysym < MAX_NORMAL_KEYCODE) {
if (k->keysym2keycode[keysym] == 0) {
trace_keymap_unmapped(keysym);
@@ -217,9 +237,8 @@ int keysym2scancode(void *kbd_layout, int keysym)
return 0;
}
-int keycode_is_keypad(void *kbd_layout, int keycode)
+int keycode_is_keypad(kbd_layout_t *k, int keycode)
{
-kbd_layout_t *k = kbd_layout;
struct key_range *kr;
for (kr = k->keypad_range; kr; kr = kr->next) {
@@ -230,9 +249,8 @@ int keycode_is_keypad(void *kbd_layout, int keycode)
return 0;
}
-int keysym_is_numlock(void *kbd_layout, int keysym)
+int keysym_is_numlock(kbd_layout_t *k, int keysym)
{
-kbd_layout_t *k = kbd_layout;
struct key_range *kr;
for (kr = k->numlock_range; kr; kr = kr->next) {
--
2.9.3
[Qemu-devel] [PULL 1/9] sdl2: fix hotkey keyup
After some hotkey was pressed sdl2 doesn't forward the first modifier
keyup event to the guest, resulting in stuck modifier keys.
Fix the logic in handle_keyup(). Also gui_key_modifier_pressed doesn't
need to be a global variable.
Reported-by: Howard Spoelstra
Tested-by: Howard Spoelstra
Signed-off-by: Gerd Hoffmann
Reviewed-by: Daniel P. Berrangé
Message-id: 20180220150444.784-1-kra...@redhat.com
---
ui/sdl2.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/ui/sdl2.c b/ui/sdl2.c
index 6e96a4a24c..b5a0fa1d13 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -39,7 +39,6 @@ static int gui_grab; /* if true, all keyboard/mouse events
are grabbed */
static int gui_saved_grab;
static int gui_fullscreen;
-static int gui_key_modifier_pressed;
static int gui_keysym;
static int gui_grab_code = KMOD_LALT | KMOD_LCTRL;
static SDL_Cursor *sdl_cursor_normal;
@@ -331,8 +330,7 @@ static void handle_keydown(SDL_Event *ev)
{
int win;
struct sdl2_console *scon = get_scon_from_window(ev->key.windowID);
-
-gui_key_modifier_pressed = get_mod_state();
+int gui_key_modifier_pressed = get_mod_state();
if (!scon->ignore_hotkeys && gui_key_modifier_pressed && !ev->key.repeat) {
switch (ev->key.keysym.scancode) {
@@ -413,18 +411,12 @@ static void handle_keydown(SDL_Event *ev)
static void handle_keyup(SDL_Event *ev)
{
-int mod_state;
struct sdl2_console *scon = get_scon_from_window(ev->key.windowID);
+int gui_key_modifier_pressed = get_mod_state();
scon->ignore_hotkeys = false;
-if (!alt_grab) {
-mod_state = (ev->key.keysym.mod & gui_grab_code);
-} else {
-mod_state = (ev->key.keysym.mod & (gui_grab_code | KMOD_LSHIFT));
-}
-if (!mod_state && gui_key_modifier_pressed) {
-gui_key_modifier_pressed = 0;
+if (!gui_key_modifier_pressed) {
gui_keysym = 0;
}
if (!gui_keysym) {
--
2.9.3
[Qemu-devel] [PULL 0/9] Ui 20180222 patches
The following changes since commit a6e0344fa0e09413324835ae122c4cadd7890231: Merge remote-tracking branch 'remotes/kraxel/tags/ui-20180220-pull-request' into staging (2018-02-20 14:05:00 +) are available in the git repository at: git://git.kraxel.org/qemu tags/ui-20180222-pull-request for you to fetch changes up to abb4f2c9655503f14dc55064f29c4f59b07e96ff: keymap: consider modifier state when picking a mapping (2018-02-22 10:35:32 +0100) ui: reverse keymap improvements. sdl2: hotkey fix. opengl: dmabuf fixes. Gerd Hoffmann (9): sdl2: fix hotkey keyup console/opengl: split up dpy_gl_cursor ops egl-headless: cursor_dmabuf: handle NULL cursor egl-helpers: add alpha channel to texture format keymap: make struct kbd_layout_t private to ui/keymaps.c keymap: use glib hash for kbd_layout_t keymap: numpad keysyms and keycodes are fixed keymap: record multiple keysym -> keycode mappings keymap: consider modifier state when picking a mapping include/ui/console.h | 13 ++-- ui/keymaps.h | 30 +++--- ui/console.c | 18 -- ui/curses.c | 3 +- ui/egl-headless.c| 30 ++ ui/egl-helpers.c | 2 +- ui/keymaps.c | 163 ++- ui/sdl.c | 6 +- ui/sdl2.c| 14 + ui/vnc.c | 9 ++- ui/trace-events | 2 +- 11 files changed, 151 insertions(+), 139 deletions(-) -- 2.9.3
[Qemu-devel] [PULL 6/9] keymap: use glib hash for kbd_layout_t
Drop home-grown lookup code, which is a strange mix of a lookup table
and a list. Use standard glib hash instead.
Signed-off-by: Gerd Hoffmann
Reviewed-by: Daniel P. Berrangé
Message-id: 20180222070513.8740-3-kra...@redhat.com
---
ui/keymaps.c| 73 -
ui/trace-events | 2 +-
2 files changed, 32 insertions(+), 43 deletions(-)
diff --git a/ui/keymaps.c b/ui/keymaps.c
index 134958a197..449c3dec22 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -28,22 +28,18 @@
#include "trace.h"
#include "qemu/error-report.h"
-#define MAX_NORMAL_KEYCODE 512
-#define MAX_EXTRA_COUNT 256
-
struct key_range {
int start;
int end;
struct key_range *next;
};
+struct keysym2code {
+uint16_t keycode;
+};
+
struct kbd_layout_t {
-uint16_t keysym2keycode[MAX_NORMAL_KEYCODE];
-struct {
-int keysym;
-uint16_t keycode;
-} keysym2keycode_extra[MAX_EXTRA_COUNT];
-int extra_count;
+GHashTable *hash;
struct key_range *keypad_range;
struct key_range *numlock_range;
};
@@ -91,23 +87,19 @@ static void add_to_key_range(struct key_range **krp, int
code) {
}
}
-static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k) {
-if (keysym < MAX_NORMAL_KEYCODE) {
-trace_keymap_add("normal", keysym, keycode, line);
-k->keysym2keycode[keysym] = keycode;
-} else {
-if (k->extra_count >= MAX_EXTRA_COUNT) {
-warn_report("Could not assign keysym %s (0x%x)"
-" because of memory constraints.", line, keysym);
-} else {
-trace_keymap_add("extra", keysym, keycode, line);
-k->keysym2keycode_extra[k->extra_count].
-keysym = keysym;
-k->keysym2keycode_extra[k->extra_count].
-keycode = keycode;
-k->extra_count++;
-}
+static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k)
+{
+struct keysym2code *keysym2code;
+
+keysym2code = g_hash_table_lookup(k->hash, GINT_TO_POINTER(keysym));
+if (keysym2code) {
+return;
}
+
+keysym2code = g_new0(struct keysym2code, 1);
+keysym2code->keycode = keycode;
+g_hash_table_replace(k->hash, GINT_TO_POINTER(keysym), keysym2code);
+trace_keymap_add(keysym, keycode, line);
}
static kbd_layout_t *parse_keyboard_layout(const name2keysym_t *table,
@@ -131,6 +123,7 @@ static kbd_layout_t *parse_keyboard_layout(const
name2keysym_t *table,
if (!k) {
k = g_new0(kbd_layout_t, 1);
+k->hash = g_hash_table_new(NULL, NULL);
}
for(;;) {
@@ -215,26 +208,22 @@ kbd_layout_t *init_keyboard_layout(const name2keysym_t
*table,
int keysym2scancode(kbd_layout_t *k, int keysym)
{
-if (keysym < MAX_NORMAL_KEYCODE) {
-if (k->keysym2keycode[keysym] == 0) {
-trace_keymap_unmapped(keysym);
-warn_report("no scancode found for keysym %d", keysym);
-}
-return k->keysym2keycode[keysym];
-} else {
-int i;
+struct keysym2code *keysym2code;
+
#ifdef XK_ISO_Left_Tab
-if (keysym == XK_ISO_Left_Tab) {
-keysym = XK_Tab;
-}
+if (keysym == XK_ISO_Left_Tab) {
+keysym = XK_Tab;
+}
#endif
-for (i = 0; i < k->extra_count; i++) {
-if (k->keysym2keycode_extra[i].keysym == keysym) {
-return k->keysym2keycode_extra[i].keycode;
-}
-}
+
+keysym2code = g_hash_table_lookup(k->hash, GINT_TO_POINTER(keysym));
+if (!keysym2code) {
+trace_keymap_unmapped(keysym);
+warn_report("no scancode found for keysym %d", keysym);
+return 0;
}
-return 0;
+
+return keysym2code->keycode;
}
int keycode_is_keypad(kbd_layout_t *k, int keycode)
diff --git a/ui/trace-events b/ui/trace-events
index 34229e6747..861b68a305 100644
--- a/ui/trace-events
+++ b/ui/trace-events
@@ -78,7 +78,7 @@ qemu_spice_create_update(uint32_t left, uint32_t right,
uint32_t top, uint32_t b
# ui/keymaps.c
keymap_parse(const char *file) "file %s"
-keymap_add(const char *type, int sym, int code, const char *line) "%-6s
sym=0x%04x code=0x%04x (line: %s)"
+keymap_add(int sym, int code, const char *line) "sym=0x%04x code=0x%04x (line:
%s)"
keymap_unmapped(int sym) "sym=0x%04x"
# ui/x_keymap.c
--
2.9.3
[Qemu-devel] [PULL 7/9] keymap: numpad keysyms and keycodes are fixed
No need to figure them at runtime from the keymap.
Signed-off-by: Gerd Hoffmann
Reviewed-by: Daniel P. Berrangé
Message-id: 20180222070513.8740-4-kra...@redhat.com
---
ui/keymaps.c | 61 +---
1 file changed, 9 insertions(+), 52 deletions(-)
diff --git a/ui/keymaps.c b/ui/keymaps.c
index 449c3dec22..cbdd65c767 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -28,20 +28,12 @@
#include "trace.h"
#include "qemu/error-report.h"
-struct key_range {
-int start;
-int end;
-struct key_range *next;
-};
-
struct keysym2code {
uint16_t keycode;
};
struct kbd_layout_t {
GHashTable *hash;
-struct key_range *keypad_range;
-struct key_range *numlock_range;
};
static int get_keysym(const name2keysym_t *table,
@@ -64,29 +56,6 @@ static int get_keysym(const name2keysym_t *table,
}
-static void add_to_key_range(struct key_range **krp, int code) {
-struct key_range *kr;
-for (kr = *krp; kr; kr = kr->next) {
-if (code >= kr->start && code <= kr->end) {
-break;
-}
-if (code == kr->start - 1) {
-kr->start--;
-break;
-}
-if (code == kr->end + 1) {
-kr->end++;
-break;
-}
-}
-if (kr == NULL) {
-kr = g_malloc0(sizeof(*kr));
-kr->start = kr->end = code;
-kr->next = *krp;
-*krp = kr;
-}
-}
-
static void add_keysym(char *line, int keysym, int keycode, kbd_layout_t *k)
{
struct keysym2code *keysym2code;
@@ -160,13 +129,6 @@ static kbd_layout_t *parse_keyboard_layout(const
name2keysym_t *table,
const char *rest = line + offset + 1;
int keycode = strtol(rest, NULL, 0);
-if (strstr(rest, "numlock")) {
-add_to_key_range(&k->keypad_range, keycode);
-add_to_key_range(&k->numlock_range, keysym);
-/* fprintf(stderr, "keypad keysym %04x keycode %d\n",
- keysym, keycode); */
-}
-
if (strstr(rest, "shift")) {
keycode |= SCANCODE_SHIFT;
}
@@ -228,24 +190,19 @@ int keysym2scancode(kbd_layout_t *k, int keysym)
int keycode_is_keypad(kbd_layout_t *k, int keycode)
{
-struct key_range *kr;
-
-for (kr = k->keypad_range; kr; kr = kr->next) {
-if (keycode >= kr->start && keycode <= kr->end) {
-return 1;
-}
+if (keycode >= 0x47 && keycode <= 0x53) {
+return true;
}
-return 0;
+return false;
}
int keysym_is_numlock(kbd_layout_t *k, int keysym)
{
-struct key_range *kr;
-
-for (kr = k->numlock_range; kr; kr = kr->next) {
-if (keysym >= kr->start && keysym <= kr->end) {
-return 1;
-}
+switch (keysym) {
+case 0xffb0 ... 0xffb9: /* KP_0 .. KP_9 */
+case 0xffac: /* KP_Separator */
+case 0xffae: /* KP_Decimal */
+return true;
}
-return 0;
+return false;
}
--
2.9.3
Re: [Qemu-devel] [PATCH v2 2/3] qcow2: Don't allow overflow during cluster allocation
On Thu 22 Feb 2018 12:39:52 AM CET, Eric Blake wrote:
> free_in_cluster = s->cluster_size - offset_into_cluster(s, offset);
> do {
> if (!offset || free_in_cluster < size) {
> -int64_t new_cluster = alloc_clusters_noref(bs, s->cluster_size);
> +int64_t new_cluster;
> +
> +new_cluster = alloc_clusters_noref(bs, s->cluster_size,
> + (1ULL << s->csize_shift) - 1);
(1ULL << s->csize_shift) - 1) is the same as s->cluster_offset_mask, but
I guess it's confusing to use that here, so your approach looks
appropriate.
Reviewed-by: Alberto Garcia
Berto
[Qemu-devel] [PULL 9/9] keymap: consider modifier state when picking a mapping
Pass the modifier state to the keymap lookup function. In case multiple
keysym -> keycode mappings exist look at the modifier state and prefer
the mapping where the modifier state matches.
Signed-off-by: Gerd Hoffmann
Reviewed-by: Daniel P. Berrangé
Message-id: 20180222070513.8740-6-kra...@redhat.com
---
ui/keymaps.h | 3 ++-
ui/curses.c | 3 ++-
ui/keymaps.c | 33 -
ui/sdl.c | 6 +-
ui/vnc.c | 9 +++--
5 files changed, 48 insertions(+), 6 deletions(-)
diff --git a/ui/keymaps.h b/ui/keymaps.h
index 17ec03387a..0693588225 100644
--- a/ui/keymaps.h
+++ b/ui/keymaps.h
@@ -54,7 +54,8 @@ typedef struct kbd_layout_t kbd_layout_t;
kbd_layout_t *init_keyboard_layout(const name2keysym_t *table,
const char *language);
-int keysym2scancode(kbd_layout_t *k, int keysym);
+int keysym2scancode(kbd_layout_t *k, int keysym,
+bool shift, bool altgr, bool ctrl);
int keycode_is_keypad(kbd_layout_t *k, int keycode);
int keysym_is_numlock(kbd_layout_t *k, int keysym);
diff --git a/ui/curses.c b/ui/curses.c
index 479b77bd03..597e47fd4a 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -271,7 +271,8 @@ static void curses_refresh(DisplayChangeListener *dcl)
keysym = chr;
}
-keycode = keysym2scancode(kbd_layout, keysym & KEYSYM_MASK);
+keycode = keysym2scancode(kbd_layout, keysym & KEYSYM_MASK,
+ false, false, false);
if (keycode == 0)
continue;
diff --git a/ui/keymaps.c b/ui/keymaps.c
index 1eba6d7057..43fe604724 100644
--- a/ui/keymaps.c
+++ b/ui/keymaps.c
@@ -176,8 +176,12 @@ kbd_layout_t *init_keyboard_layout(const name2keysym_t
*table,
}
-int keysym2scancode(kbd_layout_t *k, int keysym)
+int keysym2scancode(kbd_layout_t *k, int keysym,
+bool shift, bool altgr, bool ctrl)
{
+static const uint32_t mask =
+SCANCODE_SHIFT | SCANCODE_ALTGR | SCANCODE_CTRL;
+uint32_t mods, i;
struct keysym2code *keysym2code;
#ifdef XK_ISO_Left_Tab
@@ -193,6 +197,33 @@ int keysym2scancode(kbd_layout_t *k, int keysym)
return 0;
}
+if (keysym2code->count == 1) {
+return keysym2code->keycodes[0];
+}
+
+/*
+ * We have multiple keysym -> keycode mappings.
+ *
+ * Check whenever we find one mapping where the modifier state of
+ * the mapping matches the current user interface modifier state.
+ * If so, prefer that one.
+ */
+mods = 0;
+if (shift) {
+mods |= SCANCODE_SHIFT;
+}
+if (altgr) {
+mods |= SCANCODE_ALTGR;
+}
+if (ctrl) {
+mods |= SCANCODE_CTRL;
+}
+
+for (i = 0; i < keysym2code->count; i++) {
+if ((keysym2code->keycodes[i] & mask) == mods) {
+return keysym2code->keycodes[i];
+}
+}
return keysym2code->keycodes[0];
}
diff --git a/ui/sdl.c b/ui/sdl.c
index 963cdf77a7..c4ae7ab05d 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -201,6 +201,9 @@ static kbd_layout_t *kbd_layout = NULL;
static uint8_t sdl_keyevent_to_keycode_generic(const SDL_KeyboardEvent *ev)
{
+bool shift = modifiers_state[0x2a] || modifiers_state[0x36];
+bool altgr = modifiers_state[0xb8];
+bool ctrl = modifiers_state[0x1d] || modifiers_state[0x9d];
int keysym;
/* workaround for X11+SDL bug with AltGR */
keysym = ev->keysym.sym;
@@ -210,7 +213,8 @@ static uint8_t sdl_keyevent_to_keycode_generic(const
SDL_KeyboardEvent *ev)
if (keysym == 92 && ev->keysym.scancode == 133) {
keysym = 0xa5;
}
-return keysym2scancode(kbd_layout, keysym) & SCANCODE_KEYMASK;
+return keysym2scancode(kbd_layout, keysym,
+ shift, altgr, ctrl) & SCANCODE_KEYMASK;
}
diff --git a/ui/vnc.c b/ui/vnc.c
index a77b568b57..d19f86c7f4 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1734,7 +1734,8 @@ static void reset_keys(VncState *vs)
static void press_key(VncState *vs, int keysym)
{
-int keycode = keysym2scancode(vs->vd->kbd_layout, keysym) &
SCANCODE_KEYMASK;
+int keycode = keysym2scancode(vs->vd->kbd_layout, keysym,
+ false, false, false) & SCANCODE_KEYMASK;
qemu_input_event_send_key_number(vs->vd->dcl.con, keycode, true);
qemu_input_event_send_key_delay(vs->vd->key_delay_ms);
qemu_input_event_send_key_number(vs->vd->dcl.con, keycode, false);
@@ -1993,6 +1994,9 @@ static const char *code2name(int keycode)
static void key_event(VncState *vs, int down, uint32_t sym)
{
+bool shift = vs->modifiers_state[0x2a] || vs->modifiers_state[0x36];
+bool altgr = vs->modifiers_state[0xb8];
+bool ctrl = vs->modifiers_state[0x1d] || vs->modifiers_state[0x9d];
int keycode;
int lsym = sym;
@@ -2000,7 +2004,8 @@ static void key_event(VncState *vs, int down, uint32_t
sym)
lsym = lsym - 'A' + 'a';
}
-keycode =
[Qemu-devel] [PULL 3/9] egl-headless: cursor_dmabuf: handle NULL cursor
The cursor dmabuf can be NULL, in case no cursor defined by the guest.
Happens for example when linux guests show the framebuffer console.
Signed-off-by: Gerd Hoffmann
Message-id: 20180220110433.20353-3-kra...@redhat.com
---
ui/egl-headless.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/ui/egl-headless.c b/ui/egl-headless.c
index 00ff2c036a..b33e0b21fd 100644
--- a/ui/egl-headless.c
+++ b/ui/egl-headless.c
@@ -89,13 +89,16 @@ static void egl_cursor_dmabuf(DisplayChangeListener *dcl,
{
egl_dpy *edpy = container_of(dcl, egl_dpy, dcl);
-egl_dmabuf_import_texture(dmabuf);
-if (!dmabuf->texture) {
-return;
+if (dmabuf) {
+egl_dmabuf_import_texture(dmabuf);
+if (!dmabuf->texture) {
+return;
+}
+egl_fb_setup_for_tex(&edpy->cursor_fb, dmabuf->width, dmabuf->height,
+ dmabuf->texture, false);
+} else {
+egl_fb_destroy(&edpy->cursor_fb);
}
-
-egl_fb_setup_for_tex(&edpy->cursor_fb, dmabuf->width, dmabuf->height,
- dmabuf->texture, false);
}
static void egl_cursor_position(DisplayChangeListener *dcl,
--
2.9.3
[Qemu-devel] [PULL 2/9] console/opengl: split up dpy_gl_cursor ops
Split the cursor callback into two, one for setting the dmabuf,
one for setting the position. Also add hotspot information.
Signed-off-by: Gerd Hoffmann
Message-id: 20180220110433.20353-2-kra...@redhat.com
---
include/ui/console.h | 13 -
ui/console.c | 18 ++
ui/egl-headless.c| 17 -
3 files changed, 34 insertions(+), 14 deletions(-)
diff --git a/include/ui/console.h b/include/ui/console.h
index e6b1227bef..f29bacd625 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -230,8 +230,10 @@ typedef struct DisplayChangeListenerOps {
void (*dpy_gl_scanout_dmabuf)(DisplayChangeListener *dcl,
QemuDmaBuf *dmabuf);
void (*dpy_gl_cursor_dmabuf)(DisplayChangeListener *dcl,
- QemuDmaBuf *dmabuf,
- uint32_t pos_x, uint32_t pos_y);
+ QemuDmaBuf *dmabuf, bool have_hot,
+ uint32_t hot_x, uint32_t hot_y);
+void (*dpy_gl_cursor_position)(DisplayChangeListener *dcl,
+ uint32_t pos_x, uint32_t pos_y);
void (*dpy_gl_release_dmabuf)(DisplayChangeListener *dcl,
QemuDmaBuf *dmabuf);
void (*dpy_gl_update)(DisplayChangeListener *dcl,
@@ -304,9 +306,10 @@ void dpy_gl_scanout_texture(QemuConsole *con,
uint32_t x, uint32_t y, uint32_t w, uint32_t h);
void dpy_gl_scanout_dmabuf(QemuConsole *con,
QemuDmaBuf *dmabuf);
-void dpy_gl_cursor_dmabuf(QemuConsole *con,
- QemuDmaBuf *dmabuf,
- uint32_t pos_x, uint32_t pos_y);
+void dpy_gl_cursor_dmabuf(QemuConsole *con, QemuDmaBuf *dmabuf,
+ bool have_hot, uint32_t hot_x, uint32_t hot_y);
+void dpy_gl_cursor_position(QemuConsole *con,
+uint32_t pos_x, uint32_t pos_y);
void dpy_gl_release_dmabuf(QemuConsole *con,
QemuDmaBuf *dmabuf);
void dpy_gl_update(QemuConsole *con,
diff --git a/ui/console.c b/ui/console.c
index 36584d039e..e22931a396 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1760,14 +1760,24 @@ void dpy_gl_scanout_dmabuf(QemuConsole *con,
con->gl->ops->dpy_gl_scanout_dmabuf(con->gl, dmabuf);
}
-void dpy_gl_cursor_dmabuf(QemuConsole *con,
- QemuDmaBuf *dmabuf,
- uint32_t pos_x, uint32_t pos_y)
+void dpy_gl_cursor_dmabuf(QemuConsole *con, QemuDmaBuf *dmabuf,
+ bool have_hot, uint32_t hot_x, uint32_t hot_y)
{
assert(con->gl);
if (con->gl->ops->dpy_gl_cursor_dmabuf) {
-con->gl->ops->dpy_gl_cursor_dmabuf(con->gl, dmabuf, pos_x, pos_y);
+con->gl->ops->dpy_gl_cursor_dmabuf(con->gl, dmabuf,
+ have_hot, hot_x, hot_y);
+}
+}
+
+void dpy_gl_cursor_position(QemuConsole *con,
+uint32_t pos_x, uint32_t pos_y)
+{
+assert(con->gl);
+
+if (con->gl->ops->dpy_gl_cursor_position) {
+con->gl->ops->dpy_gl_cursor_position(con->gl, pos_x, pos_y);
}
}
diff --git a/ui/egl-headless.c b/ui/egl-headless.c
index 38b3766548..00ff2c036a 100644
--- a/ui/egl-headless.c
+++ b/ui/egl-headless.c
@@ -84,14 +84,11 @@ static void egl_scanout_dmabuf(DisplayChangeListener *dcl,
}
static void egl_cursor_dmabuf(DisplayChangeListener *dcl,
- QemuDmaBuf *dmabuf,
- uint32_t pos_x, uint32_t pos_y)
+ QemuDmaBuf *dmabuf, bool have_hot,
+ uint32_t hot_x, uint32_t hot_y)
{
egl_dpy *edpy = container_of(dcl, egl_dpy, dcl);
-edpy->pos_x = pos_x;
-edpy->pos_y = pos_y;
-
egl_dmabuf_import_texture(dmabuf);
if (!dmabuf->texture) {
return;
@@ -101,6 +98,15 @@ static void egl_cursor_dmabuf(DisplayChangeListener *dcl,
dmabuf->texture, false);
}
+static void egl_cursor_position(DisplayChangeListener *dcl,
+uint32_t pos_x, uint32_t pos_y)
+{
+egl_dpy *edpy = container_of(dcl, egl_dpy, dcl);
+
+edpy->pos_x = pos_x;
+edpy->pos_y = pos_y;
+}
+
static void egl_release_dmabuf(DisplayChangeListener *dcl,
QemuDmaBuf *dmabuf)
{
@@ -150,6 +156,7 @@ static const DisplayChangeListenerOps egl_ops = {
.dpy_gl_scanout_texture = egl_scanout_texture,
.dpy_gl_scanout_dmabuf = egl_scanout_dmabuf,
.dpy_gl_cursor_dmabuf= egl_cursor_dmabuf,
+.dpy_gl_cursor_position = egl_cursor_position,
.dpy_gl_release_dmabuf = egl_release_dmabuf,
.dpy_gl_update = egl_scanout_flush,
};
--
2.9.3
Re: [Qemu-devel] [PATCH v7 10/23] qmp: introduce QMPCapability
On Wed, Feb 21, 2018 at 04:17:18PM +, Stefan Hajnoczi wrote:
> On Wed, Jan 24, 2018 at 01:39:44PM +0800, Peter Xu wrote:
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 5c06745c79..2490d5188e 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -102,21 +102,47 @@
> > #
> > # Enable QMP capabilities.
> > #
> > -# Arguments: None.
> > +# Arguments:
> > +#
> > +# @enable:List of QMPCapabilities to enable, which is optional.
> > +# Client must not enable any capability that is not
> > +# mentioned in QMP greeting message.
>
> qapi-schema.json uses full sentences so I've added missing articles to
> the text. I also changed s/QMPCapabilities/QMPCapability values/ to
> avoid confusion about the type name:
>
> An optional list of QMPCapability values to enable. The client must
> not enable any capability that is not mentioned in the QMP greeting
> message.
>
> > #
> > # Example:
> > #
> > -# -> { "execute": "qmp_capabilities" }
> > +# -> { "execute": "qmp_capabilities",
> > +# "arguments": { "enable": [ "oob" ] } }
> > # <- { "return": {} }
> > #
> > # Notes: This command is valid exactly when first connecting: it must be
> > # issued before any other command will be accepted, and will fail once the
> > # monitor is accepting other commands. (see qemu docs/interop/qmp-spec.txt)
> > #
> > +# QMP client needs to explicitly enable QMP capabilities, otherwise
>
> s/QMP client/The QMP client/
Thanks; Fixing them all.
--
Peter Xu
Re: [Qemu-devel] [PATCH v7 13/23] monitor: let suspend/resume work even with QMPs
On Wed, Feb 21, 2018 at 04:50:58PM +, Stefan Hajnoczi wrote:
> On Wed, Jan 24, 2018 at 01:39:47PM +0800, Peter Xu wrote:
> > diff --git a/monitor.c b/monitor.c
> > index 60bcf67b3a..76137ba2a4 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -246,6 +246,21 @@ static inline bool monitor_is_qmp(const Monitor *mon)
> > return (mon->flags & MONITOR_USE_CONTROL);
> > }
> >
> > +/**
> > + * Whether @mon is using readline? Note: not all HMP monitors can are
> > + * using readline, e.g., gdbserver has non-interactive HMP monitor, so
>
> s/can are using/use/
>
> s/has non-interactive HMP monitor/has a non-interactive HMP monitor/
>
> > @@ -3994,19 +4009,43 @@ static void monitor_command_cb(void *opaque, const
> > char *cmdline,
> >
> > int monitor_suspend(Monitor *mon)
> > {
> > -if (!mon->rs)
> > +if (monitor_is_hmp_non_interactive(mon)) {
> > return -ENOTTY;
> > +}
> > +
> > +if (monitor_is_qmp(mon)) {
> > +/*
> > + * Kick iothread to make sure this takes effect. It'll be
> > + * evaluated again in prepare() of the watch object.
> > + */
> > +aio_notify(iothread_get_aio_context(mon_global.mon_iothread));
>
> This must be done after incrementing suspend_cnt to avoid the race
> condition between the iothread and our thread.
Fixed both places. Thanks,
--
Peter Xu
Re: [Qemu-devel] [PATCH v2 3/3] qcow2: Avoid memory over-allocation on compressed images
On Thu 22 Feb 2018 12:39:53 AM CET, Eric Blake wrote:
> +assert(!!s->cluster_data == !!s->cluster_cache);
> +assert(csize < 2 * s->cluster_size + 512);
> if (!s->cluster_data) {
> -/* one more sector for decompressed data alignment */
> -s->cluster_data = qemu_try_blockalign(bs->file->bs,
> -QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size + 512);
> +s->cluster_data = g_try_malloc(2 * s->cluster_size + 512);
> if (!s->cluster_data) {
> return -ENOMEM;
> }
Why the "+ 512" ?
nb_csectors is guaranteed to be at most twice the cluster size, you can
even assert that:
int max_csize = (s->csize_mask + 1) * 512;
assert(max_csize == s->cluster_size * 2);
s->cluster_data = qemu_try_blockalign(bs->file->bs, max_csize);
And csize is at most (max_csize - sector_offset), so you can change your
assertion to this:
assert(csize <= 2 * s->cluster_size);
Berto
Re: [Qemu-devel] [Qemu-block] Limiting coroutine stack usage
Am 20.02.2018 um 22:54 hat Paolo Bonzini geschrieben: > On 20/02/2018 18:04, Peter Lieven wrote: > > Hi, > > > > I remember we discussed a long time ago to limit the stack usage of all > > functions that are executed in a coroutine > > context to a very low value to be able to safely limit the coroutine > > stack size as well. > > IIRC the only issue was that hw/ide/atapi.c has mutual recursion between > ide_atapi_cmd_reply_end -> ide_transfer_start -> ahci_start_transfer -> > ide_atapi_cmd_reply_end. > > But perhaps it's not an issue, somebody needs to audit the code. I think John intended to get rid of the recursion sometime, but I doubt he has had the time so far. Kevin
Re: [Qemu-devel] [Qemu-block] Limiting coroutine stack usage
Am 22.02.2018 um 11:57 schrieb Kevin Wolf: > Am 20.02.2018 um 22:54 hat Paolo Bonzini geschrieben: >> On 20/02/2018 18:04, Peter Lieven wrote: >>> Hi, >>> >>> I remember we discussed a long time ago to limit the stack usage of all >>> functions that are executed in a coroutine >>> context to a very low value to be able to safely limit the coroutine >>> stack size as well. >> IIRC the only issue was that hw/ide/atapi.c has mutual recursion between >> ide_atapi_cmd_reply_end -> ide_transfer_start -> ahci_start_transfer -> >> ide_atapi_cmd_reply_end. >> >> But perhaps it's not an issue, somebody needs to audit the code. > I think John intended to get rid of the recursion sometime, but I doubt > he has had the time so far. Apart from this is is possible to define special cflags in the Makefile.objs just for a subdirectory? I have patches ready to make the block layer files and other coroutine users compile with -Wstack-size=2048. But I do not want to specify each file separately. Limiting the coroutine size to much could also lead to trouble in some third party libraries that are called from a coroutine context, or not? Thanks, Peter
Re: [Qemu-devel] [PATCH RFCv2] s390x/sclp: remove memory hotplug support
On 02/21/2018 06:39 PM, Cornelia Huck wrote: > On Tue, 20 Feb 2018 16:05:54 +0100 > David Hildenbrand wrote: > >> On 20.02.2018 15:57, Cornelia Huck wrote: >>> On Tue, 20 Feb 2018 13:16:37 +0100 >>> David Hildenbrand wrote: >>> On 20.02.2018 13:05, Christian Borntraeger wrote: > > > On 02/19/2018 06:42 PM, David Hildenbrand wrote: >> From an architecture point of view, nothing can be mapped into the >> address >> space on s390x. All there is is memory. Therefore there is also not >> really >> an interface to communicate such information to the guest. All we can do >> is >> specify the maximum ram address and guests can probe in that range if >> memory is available and usable (TPROT). > > In fact there is an interface in SCLP that describes the memory sizes > (maximum in > read scp info) and the details (read_storage_element0_info). I am asking > myself > if we should re-introduce read_storage_element_info and use that to avoid > tprot Yes, we could do that (basically V1 of this patch) but have to glue it to the a compatibility machine then. >>> >>> Actually, this makes quite a bit of sense (introduce the interface for >>> everyone in 2.12 and turn it off in compat machines). >> >> Jup, either 2.12 or 2.13, no need to hurry. >> >>> >>> Does real hardware have configurations where you can get the memory >>> sizes, but not the attach/deattach support? (Hardware with the feature, >>> but no standby memory defined?) We have different sclp facilities for attach/detach and information, so we can implement that. >> >> I would guess that "0" for standby memory is valid but only people with >> access to documentation can answer that :) > > So, should we go with this patch now and re-introduce the read > functions if the above is indeed true? Yes, go with this patch. Right now Linux guests will not make use of that, so we can re-add that if it turns out to be useful for future guests. Matt, last chance to complain with reasons why we want to keep the current standby memory solution in its current form. (Or please ack the patch if you agree)
[Qemu-devel] block migration and MAX_IN_FLIGHT_IO
Hi, I stumbled across the MAX_INFLIGHT_IO field that was introduced in 2015 and was curious what was the reason to choose 512MB as readahead? The question is that I found that the source VM gets very unresponsive I/O wise while the initial 512MB are read and furthermore seems to stay unreasponsive if we choose a high migration speed and have a fast storage on the destination VM. In our environment I modified this value to 16MB which seems to work much smoother. I wonder if we should make this a user configurable value or define a different rate limit for the block transfer in bulk stage at least? Peter
Re: [Qemu-devel] [PATCH v2] hw/char/stm32f2xx_usart: fix TXE/TC bit handling
On 15 February 2018 at 22:27, Alistair Francis wrote: > On Tue, Feb 13, 2018 at 12:54 PM, Richard Braun wrote: >> I/O currently being synchronous, there is no reason to ever clear the >> SR_TXE bit. However the SR_TC bit may be cleared by software writing >> to the SR register, so set it on each write. >> >> In addition, fix the reset value of the USART status register. >> >> Signed-off-by: Richard Braun > > Reviewed-by: Alistair Francis Applied to target-arm.next, thanks. -- PMM
Re: [Qemu-devel] [PATCH] Fix ast2500 protection register emulation
On 20 February 2018 at 13:26, Hugo Landau wrote:
> Some register blocks of the ast2500 are protected by protection key
> registers which require the right magic value to be written to those
> registers to allow those registers to be mutated.
>
> Register manuals indicate that writing the correct magic value to these
> registers should cause subsequent reads from those values to return 1,
> and writing any other value should cause subsequent reads to return 0.
>
> Previously, qemu implemented these registers incorrectly: the registers
> were handled as simple memory, meaning that writing some value x to a
> protection key register would result in subsequent reads from that
> register returning the same value x. The protection was implemented by
> ensuring that the current value of that register equaled the magic
> value.
>
> This modifies qemu to have the correct behaviour: attempts to write to a
> ast2500 protection register results in a transition to 1 or 0 depending
> on whether the written value is the correct magic. The protection logic
> is updated to ensure that the value of the register is nonzero.
>
> This bug caused deadlocks with u-boot HEAD: when u-boot is done with a
> protectable register block, it attempts to lock it by writing the
> bitwise inverse of the correct magic value, and then spinning forever
> until the register reads as zero. Since qemu implemented writes to these
> registers as ordinary memory writes, writing the inverse of the magic
> value resulted in subsequent reads returning that value, leading to
> u-boot spinning forever.
>
> Signed-off-by: Hugo Landau
> -if (addr != R_PROT && s->regs[R_PROT] != PROT_KEY_UNLOCK) {
> +if (addr == R_PROT) {
> + s->regs[addr] = (data == PROT_KEY_UNLOCK) ? 1 : 0;
> + return;
> +}
Applied to target-arm.next, thanks. I fixed up the incorrect indentation
in this part which checkpatch complains about.
-- PMM
Re: [Qemu-devel] [RFC, PATCH, v1] hw/audio/opl2lpt: add support for OPL2LPT
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180218144021.11641-1-vinc...@bernat.im
Subject: [Qemu-devel] [RFC, PATCH, v1] hw/audio/opl2lpt: add support for OPL2LPT
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
c83e23c953 hw/audio/opl2lpt: add support for OPL2LPT
=== OUTPUT BEGIN ===
Checking PATCH 1/1: hw/audio/opl2lpt: add support for OPL2LPT...
ERROR: spaces required around that '/' (ctx:VxV)
#154: FILE: hw/audio/opl2lpt.c:91:
+ diff/1000, nport, v);
^
ERROR: spaces required around that '/' (ctx:VxV)
#167: FILE: hw/audio/opl2lpt.c:104:
+ diff/1000, nport, v);
^
ERROR: initializer for struct MemoryRegionPortio should normally be const
#206: FILE: hw/audio/opl2lpt.c:143:
+static MemoryRegionPortio opl2lpt_portio_list[] = {
ERROR: space prohibited before that '++' (ctx:WxB)
#216: FILE: hw/audio/opl2lpt.c:153:
+for (int i = 0; i < 256; i ++) {
^
WARNING: line over 80 characters
#235: FILE: hw/audio/opl2lpt.c:172:
+portio_list_init(&s->port_list, OBJECT(s), opl2lpt_portio_list, s,
"opl2lpt");
total: 4 errors, 1 warnings, 231 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org
Re: [Qemu-devel] [PATCH] virtio-gpu-3d: add support for second capability set (v2)
Hi, This series failed docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. Type: series Message-id: 20180221015035.22964-1-airl...@gmail.com Subject: [Qemu-devel] [PATCH] virtio-gpu-3d: add support for second capability set (v2) === TEST SCRIPT BEGIN === #!/bin/bash set -e git submodule update --init dtc # Let docker tests dump environment info export SHOW_ENV=1 export J=8 time make docker-test-mingw@fedora === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 37ddbfb9f7 virtio-gpu-3d: add support for second capability set (v2) === OUTPUT BEGIN === Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-6y2lxqve/src/dtc'... Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42' BUILD fedora make[1]: Entering directory '/var/tmp/patchew-tester-tmp-6y2lxqve/src' GEN /var/tmp/patchew-tester-tmp-6y2lxqve/src/docker-src.2018-02-22-06.27.24.17208/qemu.tar Cloning into '/var/tmp/patchew-tester-tmp-6y2lxqve/src/docker-src.2018-02-22-06.27.24.17208/qemu.tar.vroot'... done. Checking out files: 46% (2733/5854) Checking out files: 47% (2752/5854) Checking out files: 48% (2810/5854) Checking out files: 49% (2869/5854) Checking out files: 50% (2927/5854) Checking out files: 51% (2986/5854) Checking out files: 52% (3045/5854) Checking out files: 53% (3103/5854) Checking out files: 54% (3162/5854) Checking out files: 55% (3220/5854) Checking out files: 56% (3279/5854) Checking out files: 57% (3337/5854) Checking out files: 58% (3396/5854) Checking out files: 59% (3454/5854) Checking out files: 60% (3513/5854) Checking out files: 61% (3571/5854) Checking out files: 62% (3630/5854) Checking out files: 63% (3689/5854) Checking out files: 64% (3747/5854) Checking out files: 65% (3806/5854) Checking out files: 66% (3864/5854) Checking out files: 67% (3923/5854) Checking out files: 68% (3981/5854) Checking out files: 69% (4040/5854) Checking out files: 70% (4098/5854) Checking out files: 71% (4157/5854) Checking out files: 72% (4215/5854) Checking out files: 73% (4274/5854) Checking out files: 74% (4332/5854) Checking out files: 75% (4391/5854) Checking out files: 76% (4450/5854) Checking out files: 77% (4508/5854) Checking out files: 78% (4567/5854) Checking out files: 79% (4625/5854) Checking out files: 80% (4684/5854) Checking out files: 81% (4742/5854) Checking out files: 82% (4801/5854) Checking out files: 83% (4859/5854) Checking out files: 84% (4918/5854) Checking out files: 85% (4976/5854) Checking out files: 86% (5035/5854) Checking out files: 87% (5093/5854) Checking out files: 88% (5152/5854) Checking out files: 89% (5211/5854) Checking out files: 90% (5269/5854) Checking out files: 91% (5328/5854) Checking out files: 92% (5386/5854) Checking out files: 93% (5445/5854) Checking out files: 94% (5503/5854) Checking out files: 95% (5562/5854) Checking out files: 96% (5620/5854) Checking out files: 97% (5679/5854) Checking out files: 98% (5737/5854) Checking out files: 99% (5796/5854) Checking out files: 100% (5854/5854) Checking out files: 100% (5854/5854), done. Your branch is up-to-date with 'origin/test'. Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-6y2lxqve/src/docker-src.2018-02-22-06.27.24.17208/qemu.tar.vroot/dtc'... Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42' Submodule 'ui/keycodemapdb' (git://git.qemu.org/keycodemapdb.git) registered for path 'ui/keycodemapdb' Cloning into '/var/tmp/patchew-tester-tmp-6y2lxqve/src/docker-src.2018-02-22-06.27.24.17208/qemu.tar.vroot/ui/keycodemapdb'... Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce' COPYRUNNER RUN test-mingw in qemu:fedora Packages installed: PyYAML-3.12-5.fc27.x86_64 SDL-devel-1.2.15-29.fc27.x86_64 bc-1.07.1-3.fc27.x86_64 bison-3.0.4-8.fc27.x86_64 bzip2-1.0.6-24.fc27.x86_64 ccache-3.3.5-1.fc27.x86_64 clang-5.0.1-1.fc27.x86_64 findutils-4.6.0-14.fc27.x86_64 flex-2.6.1-5.fc27.x86_64 gcc-7.3.1-2.fc27.x86_64 gcc-c++-7.3.1-2.fc27.x86_64 gettext-0.19.8.1-12.fc27.x86_64 git-2.14.3-2.fc27.x86_64 glib2-devel-2.54.3-2.fc27.x86_64 hostname-3.18-4.fc27.x86_64 libaio-devel-0.3.110-9.fc27.x86_64 libasan-7.3.1-2.fc27.x86_64 libfdt-devel-1.4.6-1.fc27.x86_64 libubsan-7.3.1-2.fc27.x86_64 make-4.2.1-4.fc27.x86_64 mingw32-SDL-1.2.15-9.fc27.noarch mingw32-bzip2-1.0.6-9.fc27.noarch mingw32-curl-7.54.1-2.fc27.noarch mingw32-glib2-2.54.1-1.fc27.noarch mingw32-gmp-6.1.2-2.fc27.noarch mingw32-gnutls-3.5.13-2.fc27.noarch mingw32-gtk2-2.24.31-4.fc27.noa
Re: [Qemu-devel] [Bug 1750229] Re: virtio-blk-pci regression: softlock in guest kernel at module loading
The same story with 4.15.4 host kernel. 2018-02-21 11:58 GMT+03:00 Matwey V. Kornilov : > Well, last_avail_idx equals to shadow_avail_idx and both of them are 1 > at the qemu side. So, only one request is transferred. > I wonder why, probably something is badly cached, but new avail_idx > (which is supposed to become 2) is never shown up. > > 2018-02-20 15:49 GMT+03:00 Matwey V. Kornilov : >> virtqueue_pop() returns NULL due to virtio_queue_empty_rcu() returns >> true all the time. >> >> 2018-02-20 14:47 GMT+03:00 Matwey V. Kornilov : >>> Well, I've found that on qemu side VirtQueue for virtio_blk device >>> infinitely calls virtio_blk_handle_vq() where virtio_blk_get_request() >>> (virtqueue_pop() in essens) always returns NULL. >>> >>> 2018-02-18 15:26 GMT+03:00 Matwey V. Kornilov : ** Attachment added: ".build.kernel.kvm" https://bugs.launchpad.net/qemu/+bug/1750229/+attachment/5057653/+files/.build.kernel.kvm -- You received this bug notification because you are subscribed to the bug report. https://bugs.launchpad.net/bugs/1750229 Title: virtio-blk-pci regression: softlock in guest kernel at module loading Status in QEMU: New Bug description: Hello, I am running qemu from master git branch on x86_64 host with kernel is 4.4.114. I've found that commit 9a4c0e220d8a "hw/virtio-pci: fix virtio behaviour" introduces an regression with the following command: qemu-system-x86_64 -enable-kvm -nodefaults -no-reboot -nographic -vga none -runas qemu -kernel .build.kernel.kvm -initrd .build.initrd.kvm -append 'panic=1 softlockup_panic=1 no-kvmclock nmi_watchdog=0 console=ttyS0 root=/dev/disk/by-id/virtio-0' -m 2048 -drive file=./root,format=raw,if=none,id=disk,serial=0,cache=unsafe -device virtio-blk-pci,drive=disk -serial stdio -smp 2 Starting from this commit to master the following happens with a wide variety of guest kernels (4.4 to 4.15): [ 62.428107] BUG: workqueue lockup - pool cpus=0 node=0 flags=0x0 nice=-20 stuck for 59s! [ 62.437426] Showing busy workqueues and worker pools: [ 62.443117] workqueue events: flags=0x0 [ 62.447512] pwq 0: cpus=0 node=0 flags=0x0 nice=0 active=1/256 [ 62.448161] pending: check_corruption [ 62.458570] workqueue kblockd: flags=0x18 [ 62.463082] pwq 1: cpus=0 node=0 flags=0x0 nice=-20 active=3/256 [ 62.463082] in-flight: 4:blk_mq_run_work_fn [ 62.463082] pending: blk_mq_run_work_fn, blk_mq_timeout_work [ 62.474831] pool 1: cpus=0 node=0 flags=0x0 nice=-20 hung=59s workers=2 idle: 214 [ 62.492121] INFO: rcu_preempt detected stalls on CPUs/tasks: [ 62.492121] Tasks blocked on level-0 rcu_node (CPUs 0-1): P4 [ 62.492121] (detected by 0, t=15002 jiffies, g=-130, c=-131, q=32) [ 62.492121] kworker/0:0HR running task0 4 2 0x8000 [ 62.492121] Workqueue: kblockd blk_mq_run_work_fn [ 62.492121] Call Trace: [ 62.492121] [ 62.492121] sched_show_task+0xdf/0x100 [ 62.492121] rcu_print_detail_task_stall_rnp+0x48/0x69 [ 62.492121] rcu_check_callbacks+0x93d/0x9d0 [ 62.492121] ? tick_sched_do_timer+0x40/0x40 [ 62.492121] update_process_times+0x28/0x50 [ 62.492121] tick_sched_handle+0x22/0x70 [ 62.492121] tick_sched_timer+0x34/0x70 [ 62.492121] __hrtimer_run_queues+0xcc/0x250 [ 62.492121] hrtimer_interrupt+0xab/0x1f0 [ 62.492121] smp_apic_timer_interrupt+0x62/0x150 [ 62.492121] apic_timer_interrupt+0xa2/0xb0 [ 62.492121] [ 62.492121] RIP: 0010:iowrite16+0x1d/0x30 [ 62.492121] RSP: 0018:a477c034fcc8 EFLAGS: 00010292 ORIG_RAX: ff11 [ 62.492121] RAX: a24fbdb0 RBX: 92a1f8f82000 RCX: 0001 [ 62.492121] RDX: a477c0371000 RSI: a477c0371000 RDI: [ 62.492121] RBP: 0001 R08: R09: 01080020 [ 62.492121] R10: dc7cc1e4fc00 R11: R12: [ 62.492121] R13: R14: 92a1f93f R15: 92a1f8e1aa80 [ 62.492121] ? vp_synchronize_vectors+0x60/0x60 [ 62.492121] vp_notify+0x12/0x20 [ 62.492121] virtqueue_notify+0x18/0x30 [ 62.492121] virtio_queue_rq+0x2f5/0x300 [virtio_blk] [ 62.492121] blk_mq_dispatch_rq_list+0x7e/0x4a0 [ 62.492121] blk_mq_do_dispatch_sched+0x4a/0xd0 [ 62.492121] blk_mq_sched_dispatch_requests+0x106/0x170 [ 62.492121] __blk_mq_run_hw_queue+0x80/0x90 [ 62.492121] process_one_work+0x1e3/0x420 [ 62.492121] work
Re: [Qemu-devel] [Qemu-block] Limiting coroutine stack usage
Am 22.02.2018 um 12:01 hat Peter Lieven geschrieben: > Am 22.02.2018 um 11:57 schrieb Kevin Wolf: > > Am 20.02.2018 um 22:54 hat Paolo Bonzini geschrieben: > >> On 20/02/2018 18:04, Peter Lieven wrote: > >>> Hi, > >>> > >>> I remember we discussed a long time ago to limit the stack usage of all > >>> functions that are executed in a coroutine > >>> context to a very low value to be able to safely limit the coroutine > >>> stack size as well. > >> IIRC the only issue was that hw/ide/atapi.c has mutual recursion between > >> ide_atapi_cmd_reply_end -> ide_transfer_start -> ahci_start_transfer -> > >> ide_atapi_cmd_reply_end. > >> > >> But perhaps it's not an issue, somebody needs to audit the code. > > I think John intended to get rid of the recursion sometime, but I doubt > > he has had the time so far. > > Apart from this is is possible to define special cflags in the > Makefile.objs just for a subdirectory? I have patches ready to make > the block layer files and other coroutine users compile with > -Wstack-size=2048. But I do not want to specify each file separately. Our Makefiles have lines like this: iscsi.o-cflags := $(LIBISCSI_CFLAGS) I don't think there is a direct mechanism to apply cflags to a whole directory or just to block-obj-y/block-obj-m, but just looping over them could work. I'm not a Makefile expert at all, but after some toying with a simple example, something like this might work: $(foreach x,$(block-obj-y),$(eval $x-cflags += -Wstack-size=2048)) > Limiting the coroutine size to much could also lead to trouble in some > third party libraries that are called from a coroutine context, or > not? Yes, this is true. Kevin
Re: [Qemu-devel] [PULL 00/22] re-factor softfloat and add fp16 functions
On 21 February 2018 at 11:05, Alex Bennée wrote: > The following changes since commit a6e0344fa0e09413324835ae122c4cadd7890231: > > Merge remote-tracking branch 'remotes/kraxel/tags/ui-20180220-pull-request' > into staging (2018-02-20 14:05:00 +) > > are available in the Git repository at: > > https://github.com/stsquad/qemu.git tags/pull-softfloat-refactor-210218-1 > > for you to fetch changes up to c13bb2da9eedfbc5886c8048df1bc1114b285fb0: > > fpu/softfloat: re-factor sqrt (2018-02-21 10:21:54 +) > > > This is the re-factor of softfloat: > > - shared common code path float16/32/64 > - well commented and easy to follow code > - added a bunch of float16 support > > While some operations are slower the key ones exercised by the > floating point dbt-bench are the same: https://i.imgur.com/oXNJNql.png Applied, thanks. -- PMM
[Qemu-devel] Patch 9894dc0cdcc broke something
Hello!
I hit unexpected disconnections because of this patch:
commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2
Author: Daniel P. Berrange
Date: Tue Jan 19 11:14:29 2016 +
char: convert from GIOChannel to QIOChannel
In preparation for introducing TLS support to the TCP chardev
backend, convert existing chardev code from using GIOChannel
to QIOChannel. This simplifies the chardev code by removing
most of the OS platform conditional code for dealing with
file descriptor passing.
Signed-off-by: Daniel P. Berrange
Message-Id: <1453202071-10289-3-git-send-email-berra...@redhat.com>
Signed-off-by: Paolo Bonzini
breaks tcp_chr_read:
-static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
+static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
{
CharDriverState *chr = opaque;
TCPCharDriver *s = chr->opaque;
@@ -2938,9 +2801,7 @@ static gboolean tcp_chr_read(GIOChannel *chan,
GIOCondition cond, void *opaque)
if (len > s->max_size)
len = s->max_size;
size = tcp_chr_recv(chr, (void *)buf, len);
-if (size == 0 ||
-(size < 0 &&
- socket_error() != EAGAIN && socket_error() != EWOULDBLOCK)) {
+if (size == 0 || size == -1) {
/* connection closed */
tcp_chr_disconnect(chr);
} else if (size > 0) {
since tcp_chr_recv returns -1 on blocking:
static ssize_t tcp_chr_recv(Chardev *chr, char *buf, size_t len)
{
...
if (ret == QIO_CHANNEL_ERR_BLOCK) {
errno = EAGAIN;
ret = -1;
} else if (ret == -1) {
errno = EIO;
}
This patch not just converts GIOChannel to QIOChannel
it also changes the logic which is not mentioned in the commit message.
Fix please :)
Re: [Qemu-devel] [Qemu-block] Limiting coroutine stack usage
On Thu, Feb 22, 2018 at 12:32:04PM +0100, Kevin Wolf wrote: > Am 22.02.2018 um 12:01 hat Peter Lieven geschrieben: > > Am 22.02.2018 um 11:57 schrieb Kevin Wolf: > > > Am 20.02.2018 um 22:54 hat Paolo Bonzini geschrieben: > > >> On 20/02/2018 18:04, Peter Lieven wrote: > > >>> Hi, > > >>> > > >>> I remember we discussed a long time ago to limit the stack usage of all > > >>> functions that are executed in a coroutine > > >>> context to a very low value to be able to safely limit the coroutine > > >>> stack size as well. > > >> IIRC the only issue was that hw/ide/atapi.c has mutual recursion between > > >> ide_atapi_cmd_reply_end -> ide_transfer_start -> ahci_start_transfer -> > > >> ide_atapi_cmd_reply_end. > > >> > > >> But perhaps it's not an issue, somebody needs to audit the code. > > > I think John intended to get rid of the recursion sometime, but I doubt > > > he has had the time so far. > > > > Apart from this is is possible to define special cflags in the > > Makefile.objs just for a subdirectory? I have patches ready to make > > the block layer files and other coroutine users compile with > > -Wstack-size=2048. But I do not want to specify each file separately. > > Our Makefiles have lines like this: > > iscsi.o-cflags := $(LIBISCSI_CFLAGS) > > I don't think there is a direct mechanism to apply cflags to a whole > directory or just to block-obj-y/block-obj-m, but just looping over them > could work. I'm not a Makefile expert at all, but after some toying with > a simple example, something like this might work: > > $(foreach x,$(block-obj-y),$(eval $x-cflags += -Wstack-size=2048)) You'll need it for anything block layer depends on too - so that's much of util/, crypto/ and io/ directories at least. So perhaps it would be shorter if we do the opposite - set -Wstack-size=2048 globally for everything in QEMU, and then override -Wstack-size=$BIGGER for the (hopefully) few sources that have a larger stack need ? Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [Qemu-block] Limiting coroutine stack usage
Am 22.02.2018 um 12:32 schrieb Kevin Wolf: > Am 22.02.2018 um 12:01 hat Peter Lieven geschrieben: >> Am 22.02.2018 um 11:57 schrieb Kevin Wolf: >>> Am 20.02.2018 um 22:54 hat Paolo Bonzini geschrieben: On 20/02/2018 18:04, Peter Lieven wrote: > Hi, > > I remember we discussed a long time ago to limit the stack usage of all > functions that are executed in a coroutine > context to a very low value to be able to safely limit the coroutine > stack size as well. IIRC the only issue was that hw/ide/atapi.c has mutual recursion between ide_atapi_cmd_reply_end -> ide_transfer_start -> ahci_start_transfer -> ide_atapi_cmd_reply_end. But perhaps it's not an issue, somebody needs to audit the code. >>> I think John intended to get rid of the recursion sometime, but I doubt >>> he has had the time so far. >> Apart from this is is possible to define special cflags in the >> Makefile.objs just for a subdirectory? I have patches ready to make >> the block layer files and other coroutine users compile with >> -Wstack-size=2048. But I do not want to specify each file separately. > Our Makefiles have lines like this: > > iscsi.o-cflags := $(LIBISCSI_CFLAGS) > > I don't think there is a direct mechanism to apply cflags to a whole > directory or just to block-obj-y/block-obj-m, but just looping over them > could work. I'm not a Makefile expert at all, but after some toying with > a simple example, something like this might work: > > $(foreach x,$(block-obj-y),$(eval $x-cflags += -Wstack-size=2048)) Thanks for the hint. If noone comes up with a better idea I will try this. Peter
Re: [Qemu-devel] Patch 9894dc0cdcc broke something
On Thu, Feb 22, 2018 at 02:38:04PM +0300, Aleksey Kuleshov wrote: > Hello! > > I hit unexpected disconnections because of this patch: > > commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2 > Author: Daniel P. Berrange > Date: Tue Jan 19 11:14:29 2016 + > > char: convert from GIOChannel to QIOChannel > > In preparation for introducing TLS support to the TCP chardev > backend, convert existing chardev code from using GIOChannel > to QIOChannel. This simplifies the chardev code by removing > most of the OS platform conditional code for dealing with > file descriptor passing. > > Signed-off-by: Daniel P. Berrange > Message-Id: <1453202071-10289-3-git-send-email-berra...@redhat.com> > Signed-off-by: Paolo Bonzini > > breaks tcp_chr_read: What version of QEMU are you using ? Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [RFC PATCH v3 5/7] hw/sd/pl181: expose a SDBus and connect the SDCard to it
On 16 February 2018 at 02:29, Philippe Mathieu-Daudé wrote: > using the sdbus_*() API. > > Signed-off-by: Philippe Mathieu-Daudé > --- > > RFC because how pl181_sdbus_create_inplace() doing class_init(SDBus) in > realize(pl181) seems weird... In the two other places where we set the set_inserted and set_readonly methods for the SDBus class (pxa2xx_mmci.c and sdhci.c), we do it by defining a subclass of TYPE_SD_BUS, whose class init function can then set up the method pointers. Is there a reason not to do pl181 the same way as those two examples? thanks -- PMM
Re: [Qemu-devel] [PATCH v3 0/7] SDHCI: convert legacy devices to the SDBus API (part 6)
On 16 February 2018 at 02:29, Philippe Mathieu-Daudé wrote: > Hi, > > Since v2: > - pl181: remove legacy sd_set_cb() (Peter) > > Since v1: > - rebased on /master (Peter sdcard reset() patches) > - fix milkymist-mmc from previous seris using instance_init (Michael Walle) > > This series convert 3 devices using the legacy SDCard API to the SDBus API: > - milkymist-mmc > - pl181 > - ssi-sd > > Then move the legacy API to a separate header "sdcard_legacy.h". > > Now the OMAP MMC is the last device using the legacy API, but need to get > QOM'ified first. > > Having a common sdbus interface simplify qtesting (next series) > > This series is not related to the previous set (4/5) and can be applied > independently. > > Regards, > > Phil. > > $ git backport-diff > 001/7:[] [--] 'hw/sd/milkymist-memcard: use qemu_log_mask()' > 002/7:[] [--] 'hw/sd/milkymist-memcard: split realize() out of > SysBusDevice init()' > 003/7:[] [--] 'hw/sd/milkymist-memcard: expose a SDBus and connect the > SDCard to it' > 004/7:[] [--] 'hw/sd/ssi-sd: use the SDBus API, connect the SDCard to the > bus' > 005/7:[0034] [FC] 'hw/sd/pl181: expose a SDBus and connect the SDCard to it' > 006/7:[down] 'hw/sd: make sd_data_ready() static' > 007/7:[0003] [FC] 'hw/sd: move sdcard legacy API to "hw/sd/sdcard_legacy.h"' I've applied patches 1-4 to target-arm.next; I had a review comment on 5 and 6,7 depend on that one. thanks -- PMM
Re: [Qemu-devel] [Qemu-block] Limiting coroutine stack usage
Am 22.02.2018 um 12:40 schrieb Daniel P. Berrangé: > On Thu, Feb 22, 2018 at 12:32:04PM +0100, Kevin Wolf wrote: >> Am 22.02.2018 um 12:01 hat Peter Lieven geschrieben: >>> Am 22.02.2018 um 11:57 schrieb Kevin Wolf: Am 20.02.2018 um 22:54 hat Paolo Bonzini geschrieben: > On 20/02/2018 18:04, Peter Lieven wrote: >> Hi, >> >> I remember we discussed a long time ago to limit the stack usage of all >> functions that are executed in a coroutine >> context to a very low value to be able to safely limit the coroutine >> stack size as well. > IIRC the only issue was that hw/ide/atapi.c has mutual recursion between > ide_atapi_cmd_reply_end -> ide_transfer_start -> ahci_start_transfer -> > ide_atapi_cmd_reply_end. > > But perhaps it's not an issue, somebody needs to audit the code. I think John intended to get rid of the recursion sometime, but I doubt he has had the time so far. >>> Apart from this is is possible to define special cflags in the >>> Makefile.objs just for a subdirectory? I have patches ready to make >>> the block layer files and other coroutine users compile with >>> -Wstack-size=2048. But I do not want to specify each file separately. >> Our Makefiles have lines like this: >> >> iscsi.o-cflags := $(LIBISCSI_CFLAGS) >> >> I don't think there is a direct mechanism to apply cflags to a whole >> directory or just to block-obj-y/block-obj-m, but just looping over them >> could work. I'm not a Makefile expert at all, but after some toying with >> a simple example, something like this might work: >> >> $(foreach x,$(block-obj-y),$(eval $x-cflags += -Wstack-size=2048)) > You'll need it for anything block layer depends on too - so that's much > of util/, crypto/ and io/ directories at least. > > So perhaps it would be shorter if we do the opposite - set -Wstack-size=2048 > globally for everything in QEMU, and then override -Wstack-size=$BIGGER > for the (hopefully) few sources that have a larger stack need ? I tried that already. 2048 is a strong limit for many functions. It breaks already as soon as some buffer has a size of PATH_MAX, but thats handleable. But there are some structs around that are very large. Generally, it would be a good idea to have a global limit, of course. Peter
Re: [Qemu-devel] [PATCH v8 00/13] Interactive Boot Menu for DASD and SCSI Guests on s390x
Series Acked-by: Christian Borntraeger menu on scsi and dasd bootmaps tested successfully. There is one thing that we might want to fix (can be an addon patch since this is a non-customer scenario (no libvirt)). If you start QEMU manually without a bootindex, the -boot menu=on is ignored if no drive has a bootindex. For example: -drive file=/dev/dasda,if=none,id=d1 -device virtio-blk-ccw,drive=d1,bootindex=1 -boot menu=on does work -drive file=/dev/dasda -boot menu=on does not instead it prints: qemu-system-s390x: boot menu is not supported for this device type. and the boots up the default entry. On 02/21/2018 08:35 PM, Collin L. Walling wrote: > Due to the introduction of the QemuIplParameter block and taking some time to > revist some areas, a few chunks of code have been reworked to better align > with > those changes. I also think the reworkings improve readability. The same > functionality is still there, the code just looks a little different (and > hopefully looks better). > > --- [v8] --- > > The tl;dr: > > cleaned up some early patches based on review, threw zipl boot option code > into its own patch, refactored s390_ipl_set_boot_menu to reflect latest > changes. > > The "pls explain": > > * cleaned up "s390-ccw: refactor boot map table code" > > - removed void ptr casting in a couple of spots > > * cleaned up "s390-ccw: set cp_receive mask..." patch > > - setting the mask consumes a service interrupt, so there is no need > for > a followup sclp_print > > * fixed uitoa based on feedback from v7 > > * fixed alignment concerns in QemuIplParameters and renamed flags field > > - reasoning: necessary; better readability > > - s/boot_menu_flags/qipl_flags so this field can be (potentially) > used for > other purposes in the future > > - when setting the boot menu parms in the bios, we take care to mask > out > the appropriate qipl_flags for boot menu specific flags > > - boot menu flags defines are renamed to be prefixed with "QIPL_FLAG_" > > - also updated comment above this struct > > * cleaned up "s390-ccw: read user input..." patch > > - defines for low core external interrupt code addr and > clock comparator interrupt code > > - take care to make sure buf remains null terminated when passed to > read_prompt > > * [NEW PATCH] "s390-ccw: use zipl values when no boot menu options are > present" > > - reasoning: better readability; further explanation of feature > > - *nothing new added to this patch series here* > > - zipl options flag setting and parsing *moved to* this patch > > - this attempts to better explain how these fields are used and how > they get > parsed > > - the commit message gives details on how to set these fields in the > zipl > configuration file > > - the zipl options are only set for CCW type IPL devices (since no > other devices actually support it) > > * refactored s390_ipl_set_boot_menu > > - reasoning: better readability > > - the idea is that we should take care to appropriately set the boot > menu > flags for each IPL device type from the beginning. We should not set > certain flags for devices that cannot support certain features (eg > SCSI > does not support zipl menus, so we should never set the > use_zipl_opts flag > for SCSI) > > - since there are no longer boot menu fields specific to each IPL > type, > the switch statement is simply used to detect if the IPL device type > is capable of a boot menu > > - since zipl flags are only set for CCW type IPL devices, I reworked > the logic and removed some indentation > > * s/menu_check_flags/menu_is_enabled > > - reasoning: better readability > > - no parameters > > - "if menu is enabled" reads better than "if these specific flag bits > are set" > > * removed menu.h > > - reasoning: file not needed; less to maintain > > - introduction of qipl and better understanding of zipl options led > to > this decision. by the end of these changes, this file ended up > housing 4 function declarations and no longer seemed necessary > > - all menu related function declarations are in s390-ccw.h > > - boot menu flags are defined in iplb.h (which aligns with > hw/s390x/ipl.h) > > --- [Summary] --- > > These patches implement a boot menu for ECKD DASD and SCSI guests on s390x. > The menu will only appear if the disk has been configured for IPL with the > zIPL tool and with the following QEMU command line options: > > -boot menu=on[,splash-time=X] and/or -machine loadparm='prompt' > > The following must be specified for the device to be IPL
Re: [Qemu-devel] [Qemu-block] Limiting coroutine stack usage
On Thu, Feb 22, 2018 at 12:51:58PM +0100, Peter Lieven wrote: > Am 22.02.2018 um 12:40 schrieb Daniel P. Berrangé: > > On Thu, Feb 22, 2018 at 12:32:04PM +0100, Kevin Wolf wrote: > >> Am 22.02.2018 um 12:01 hat Peter Lieven geschrieben: > >>> Am 22.02.2018 um 11:57 schrieb Kevin Wolf: > Am 20.02.2018 um 22:54 hat Paolo Bonzini geschrieben: > > On 20/02/2018 18:04, Peter Lieven wrote: > >> Hi, > >> > >> I remember we discussed a long time ago to limit the stack usage of all > >> functions that are executed in a coroutine > >> context to a very low value to be able to safely limit the coroutine > >> stack size as well. > > IIRC the only issue was that hw/ide/atapi.c has mutual recursion between > > ide_atapi_cmd_reply_end -> ide_transfer_start -> ahci_start_transfer -> > > ide_atapi_cmd_reply_end. > > > > But perhaps it's not an issue, somebody needs to audit the code. > I think John intended to get rid of the recursion sometime, but I doubt > he has had the time so far. > >>> Apart from this is is possible to define special cflags in the > >>> Makefile.objs just for a subdirectory? I have patches ready to make > >>> the block layer files and other coroutine users compile with > >>> -Wstack-size=2048. But I do not want to specify each file separately. > >> Our Makefiles have lines like this: > >> > >> iscsi.o-cflags := $(LIBISCSI_CFLAGS) > >> > >> I don't think there is a direct mechanism to apply cflags to a whole > >> directory or just to block-obj-y/block-obj-m, but just looping over them > >> could work. I'm not a Makefile expert at all, but after some toying with > >> a simple example, something like this might work: > >> > >> $(foreach x,$(block-obj-y),$(eval $x-cflags += -Wstack-size=2048)) > > You'll need it for anything block layer depends on too - so that's much > > of util/, crypto/ and io/ directories at least. > > > > So perhaps it would be shorter if we do the opposite - set -Wstack-size=2048 > > globally for everything in QEMU, and then override -Wstack-size=$BIGGER > > for the (hopefully) few sources that have a larger stack need ? > > I tried that already. 2048 is a strong limit for many functions. > It breaks already as soon as some buffer has a size of PATH_MAX, but > thats handleable. But there are some structs around that are very large. There are surprisingly few "char [PATH_MAX]" variables left in QEMU - we should have a final push to eliminate them regardless. > Generally, it would be a good idea to have a global limit, of course. We could at least put a limit on that matches the current worst case to prevent it getting worse than it already is. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v4 06/11] sdcard: do not trace CMD55 when expecting ACMD
On 15 February 2018 at 22:05, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé
> Acked-by: Alistair Francis
> ---
> hw/sd/sd.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 564f7a9bfd..af4df2b104 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -818,13 +818,15 @@ static void sd_lock_command(SDState *sd)
> sd->card_status &= ~CARD_IS_LOCKED;
> }
>
> -static sd_rsp_type_t sd_normal_command(SDState *sd,
> - SDRequest req)
> +static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
> {
> uint32_t rca = 0x;
> uint64_t addr = (sd->ocr & (1 << 30)) ? (uint64_t) req.arg << 9 :
> req.arg;
>
> -trace_sdcard_normal_command(req.cmd, req.arg, sd_state_name(sd->state));
> +if (req.cmd != 55 || sd->expecting_acmd) {
> +trace_sdcard_normal_command(req.cmd, req.arg,
> +sd_state_name(sd->state));
> +}
The commit message says "don't trace CMD55 when expecting ACMD",
but the code says "don't trace CMD55 when *not* expecting ACMD" --
which is correct?
thanks
-- PMM
Re: [Qemu-devel] [Qemu-block] Limiting coroutine stack usage
Am 22.02.2018 um 13:00 schrieb Daniel P. Berrangé: > On Thu, Feb 22, 2018 at 12:51:58PM +0100, Peter Lieven wrote: >> Am 22.02.2018 um 12:40 schrieb Daniel P. Berrangé: >>> On Thu, Feb 22, 2018 at 12:32:04PM +0100, Kevin Wolf wrote: Am 22.02.2018 um 12:01 hat Peter Lieven geschrieben: > Am 22.02.2018 um 11:57 schrieb Kevin Wolf: >> Am 20.02.2018 um 22:54 hat Paolo Bonzini geschrieben: >>> On 20/02/2018 18:04, Peter Lieven wrote: Hi, I remember we discussed a long time ago to limit the stack usage of all functions that are executed in a coroutine context to a very low value to be able to safely limit the coroutine stack size as well. >>> IIRC the only issue was that hw/ide/atapi.c has mutual recursion between >>> ide_atapi_cmd_reply_end -> ide_transfer_start -> ahci_start_transfer -> >>> ide_atapi_cmd_reply_end. >>> >>> But perhaps it's not an issue, somebody needs to audit the code. >> I think John intended to get rid of the recursion sometime, but I doubt >> he has had the time so far. > Apart from this is is possible to define special cflags in the > Makefile.objs just for a subdirectory? I have patches ready to make > the block layer files and other coroutine users compile with > -Wstack-size=2048. But I do not want to specify each file separately. Our Makefiles have lines like this: iscsi.o-cflags := $(LIBISCSI_CFLAGS) I don't think there is a direct mechanism to apply cflags to a whole directory or just to block-obj-y/block-obj-m, but just looping over them could work. I'm not a Makefile expert at all, but after some toying with a simple example, something like this might work: $(foreach x,$(block-obj-y),$(eval $x-cflags += -Wstack-size=2048)) >>> You'll need it for anything block layer depends on too - so that's much >>> of util/, crypto/ and io/ directories at least. >>> >>> So perhaps it would be shorter if we do the opposite - set -Wstack-size=2048 >>> globally for everything in QEMU, and then override -Wstack-size=$BIGGER >>> for the (hopefully) few sources that have a larger stack need ? >> I tried that already. 2048 is a strong limit for many functions. >> It breaks already as soon as some buffer has a size of PATH_MAX, but >> thats handleable. But there are some structs around that are very large. > There are surprisingly few "char [PATH_MAX]" variables left in QEMU - we > should have a final push to eliminate them regardless. > >> Generally, it would be a good idea to have a global limit, of course. > We could at least put a limit on that matches the current worst case to > prevent it getting worse than it already is. That would be a good idea, yes. How would you handle the override for a smaller -Wstack-usage ? > > Regards, > Daniel
Re: [Qemu-devel] [Qemu-block] Limiting coroutine stack usage
On Thu, Feb 22, 2018 at 01:02:05PM +0100, Peter Lieven wrote: > Am 22.02.2018 um 13:00 schrieb Daniel P. Berrangé: > > On Thu, Feb 22, 2018 at 12:51:58PM +0100, Peter Lieven wrote: > >> Am 22.02.2018 um 12:40 schrieb Daniel P. Berrangé: > >>> On Thu, Feb 22, 2018 at 12:32:04PM +0100, Kevin Wolf wrote: > Am 22.02.2018 um 12:01 hat Peter Lieven geschrieben: > > Am 22.02.2018 um 11:57 schrieb Kevin Wolf: > >> Am 20.02.2018 um 22:54 hat Paolo Bonzini geschrieben: > >>> On 20/02/2018 18:04, Peter Lieven wrote: > Hi, > > I remember we discussed a long time ago to limit the stack usage of > all > functions that are executed in a coroutine > context to a very low value to be able to safely limit the coroutine > stack size as well. > >>> IIRC the only issue was that hw/ide/atapi.c has mutual recursion > >>> between > >>> ide_atapi_cmd_reply_end -> ide_transfer_start -> ahci_start_transfer > >>> -> > >>> ide_atapi_cmd_reply_end. > >>> > >>> But perhaps it's not an issue, somebody needs to audit the code. > >> I think John intended to get rid of the recursion sometime, but I doubt > >> he has had the time so far. > > Apart from this is is possible to define special cflags in the > > Makefile.objs just for a subdirectory? I have patches ready to make > > the block layer files and other coroutine users compile with > > -Wstack-size=2048. But I do not want to specify each file separately. > Our Makefiles have lines like this: > > iscsi.o-cflags := $(LIBISCSI_CFLAGS) > > I don't think there is a direct mechanism to apply cflags to a whole > directory or just to block-obj-y/block-obj-m, but just looping over them > could work. I'm not a Makefile expert at all, but after some toying with > a simple example, something like this might work: > > $(foreach x,$(block-obj-y),$(eval $x-cflags += -Wstack-size=2048)) > >>> You'll need it for anything block layer depends on too - so that's much > >>> of util/, crypto/ and io/ directories at least. > >>> > >>> So perhaps it would be shorter if we do the opposite - set > >>> -Wstack-size=2048 > >>> globally for everything in QEMU, and then override -Wstack-size=$BIGGER > >>> for the (hopefully) few sources that have a larger stack need ? > >> I tried that already. 2048 is a strong limit for many functions. > >> It breaks already as soon as some buffer has a size of PATH_MAX, but > >> thats handleable. But there are some structs around that are very large. > > There are surprisingly few "char [PATH_MAX]" variables left in QEMU - we > > should have a final push to eliminate them regardless. > > > >> Generally, it would be a good idea to have a global limit, of course. > > We could at least put a limit on that matches the current worst case to > > prevent it getting worse than it already is. > > That would be a good idea, yes. > > How would you handle the override for a smaller -Wstack-usage ? If you have multiple -Wstack-size=$XXX flags to GCC, I expect the last one wins. So just need to double check that the per-object file CFLAGS occur after the global CFLAS in the compiler args Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] Patch 9894dc0cdcc broke something
I'm using 2.11.0. 22.02.2018, 14:45, "Daniel P. Berrangé" : > On Thu, Feb 22, 2018 at 02:38:04PM +0300, Aleksey Kuleshov wrote: >> Hello! >> >> I hit unexpected disconnections because of this patch: >> >> commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2 >> Author: Daniel P. Berrange >> Date: Tue Jan 19 11:14:29 2016 + >> >> char: convert from GIOChannel to QIOChannel >> >> In preparation for introducing TLS support to the TCP chardev >> backend, convert existing chardev code from using GIOChannel >> to QIOChannel. This simplifies the chardev code by removing >> most of the OS platform conditional code for dealing with >> file descriptor passing. >> >> Signed-off-by: Daniel P. Berrange >> Message-Id: <1453202071-10289-3-git-send-email-berra...@redhat.com> >> Signed-off-by: Paolo Bonzini >> >> breaks tcp_chr_read: > > What version of QEMU are you using ? > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [Qemu-block] Limiting coroutine stack usage
Am 22.02.2018 um 13:03 schrieb Daniel P. Berrangé: > On Thu, Feb 22, 2018 at 01:02:05PM +0100, Peter Lieven wrote: >> Am 22.02.2018 um 13:00 schrieb Daniel P. Berrangé: >>> On Thu, Feb 22, 2018 at 12:51:58PM +0100, Peter Lieven wrote: Am 22.02.2018 um 12:40 schrieb Daniel P. Berrangé: > On Thu, Feb 22, 2018 at 12:32:04PM +0100, Kevin Wolf wrote: >> Am 22.02.2018 um 12:01 hat Peter Lieven geschrieben: >>> Am 22.02.2018 um 11:57 schrieb Kevin Wolf: Am 20.02.2018 um 22:54 hat Paolo Bonzini geschrieben: > On 20/02/2018 18:04, Peter Lieven wrote: >> Hi, >> >> I remember we discussed a long time ago to limit the stack usage of >> all >> functions that are executed in a coroutine >> context to a very low value to be able to safely limit the coroutine >> stack size as well. > IIRC the only issue was that hw/ide/atapi.c has mutual recursion > between > ide_atapi_cmd_reply_end -> ide_transfer_start -> ahci_start_transfer > -> > ide_atapi_cmd_reply_end. > > But perhaps it's not an issue, somebody needs to audit the code. I think John intended to get rid of the recursion sometime, but I doubt he has had the time so far. >>> Apart from this is is possible to define special cflags in the >>> Makefile.objs just for a subdirectory? I have patches ready to make >>> the block layer files and other coroutine users compile with >>> -Wstack-size=2048. But I do not want to specify each file separately. >> Our Makefiles have lines like this: >> >> iscsi.o-cflags := $(LIBISCSI_CFLAGS) >> >> I don't think there is a direct mechanism to apply cflags to a whole >> directory or just to block-obj-y/block-obj-m, but just looping over them >> could work. I'm not a Makefile expert at all, but after some toying with >> a simple example, something like this might work: >> >> $(foreach x,$(block-obj-y),$(eval $x-cflags += -Wstack-size=2048)) > You'll need it for anything block layer depends on too - so that's much > of util/, crypto/ and io/ directories at least. > > So perhaps it would be shorter if we do the opposite - set > -Wstack-size=2048 > globally for everything in QEMU, and then override -Wstack-size=$BIGGER > for the (hopefully) few sources that have a larger stack need ? I tried that already. 2048 is a strong limit for many functions. It breaks already as soon as some buffer has a size of PATH_MAX, but thats handleable. But there are some structs around that are very large. >>> There are surprisingly few "char [PATH_MAX]" variables left in QEMU - we >>> should have a final push to eliminate them regardless. >>> Generally, it would be a good idea to have a global limit, of course. >>> We could at least put a limit on that matches the current worst case to >>> prevent it getting worse than it already is. >> That would be a good idea, yes. >> >> How would you handle the override for a smaller -Wstack-usage ? > If you have multiple -Wstack-size=$XXX flags to GCC, I expect the last > one wins. So just need to double check that the per-object file CFLAGS > occur after the global CFLAS in the compiler args I will check that, thanks. When I am at it, what would be the proper replacement for char[PATH_MAX] ? Peter
Re: [Qemu-devel] [PATCH v2 1/1] 390x/cpumodel: document S390FeatDef.bit not applicable
On 02/21/2018 05:56 PM, Halil Pasic wrote:
> The 'bit' field of the 'S390FeatDef' structure is not applicable to all
> its instances. Currently this field is not applicable, and remains
> unused, iff the feature is of type S390_FEAT_TYPE_MISC. Having the value 0
> specified for multiple such feature definition was a little confusing,
> as it's a perfectly legit bit value, and as the value of the bit
> field is usually ought to be unique for each feature of a given
> feature type.
>
> Let us introduce a specialized macro for defining features of type
> S390_FEAT_TYPE_MISC so, that one does not have to specify neither bit nor
> type (as the later is implied).
>
> Signed-off-by: Halil Pasic
Acked-by: Christian Borntraeger
> ---
>
> v1 -> v2
> * Specialized feature initializer macro for type MISC that does not
> require a bit value instead of defining a 'not a bit number' (that
> is extremal) bit number.
> ---
> target/s390x/cpu_features.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index a5619f2893..3b9e2745e9 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -23,6 +23,10 @@
> .desc = _desc, \
> }
>
> +/* S390FeatDef.bit is not applicable as there is no feature block. */
> +#define FEAT_INIT_MISC(_name, _desc) \
> +FEAT_INIT(_name, S390_FEAT_TYPE_MISC, 0, _desc)
> +
> /* indexed by feature number for easy lookup */
> static const S390FeatDef s390_features[] = {
> FEAT_INIT("esan3", S390_FEAT_TYPE_STFL, 0, "Instructions marked as n3"),
> @@ -123,8 +127,8 @@ static const S390FeatDef s390_features[] = {
> FEAT_INIT("ib", S390_FEAT_TYPE_SCLP_CPU, 42, "SIE: Intervention bypass
> facility"),
> FEAT_INIT("cei", S390_FEAT_TYPE_SCLP_CPU, 43, "SIE:
> Conditional-external-interception facility"),
>
> -FEAT_INIT("dateh2", S390_FEAT_TYPE_MISC, 0, "DAT-enhancement facility
> 2"),
> -FEAT_INIT("cmm", S390_FEAT_TYPE_MISC, 0,
> "Collaborative-memory-management facility"),
> +FEAT_INIT_MISC("dateh2", "DAT-enhancement facility 2"),
> +FEAT_INIT_MISC("cmm", "Collaborative-memory-management facility"),
>
> FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32 bit
> in general registers)"),
> FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64
> bit in parameter list)"),
>
[Qemu-devel] [PATCH v1] chardev: fix handling of EAGAIN for TCP chardev
When this commit was applied
commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2
Author: Daniel P. Berrange
Date: Tue Jan 19 11:14:29 2016 +
char: convert from GIOChannel to QIOChannel
The tcp_chr_recv() function was changed to return QIO_CHANNEL_ERR_BLOCK
which corresonds to -2. As such the handling for EAGAIN was able to be
removed from tcp_chr_read(). Unfortunately in a later commit:
commit b6572b4f97a7b126c7b24e165893ed9fe3d72e1f
Author: Marc-André Lureau
Date: Fri Mar 11 18:55:24 2016 +0100
char: translate from QIOChannel error to errno
The tcp_chr_recv() function was changed back to return -1, with errno
set to EAGAIN, without also re-addding support for this to tcp_chr_read()
Reported-by: Aleksey Kuleshov
Signed-off-by: Daniel P. Berrangé
---
chardev/char-socket.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index bdd6cff5f6..b15682c362 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -449,7 +449,7 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition
cond, void *opaque)
len = s->max_size;
}
size = tcp_chr_recv(chr, (void *)buf, len);
-if (size == 0 || size == -1) {
+if (size == 0 || (size == -1 && errno != EAGAIN)) {
/* connection closed */
tcp_chr_disconnect(chr);
} else if (size > 0) {
--
2.14.3
Re: [Qemu-devel] Patch 9894dc0cdcc broke something
On Thu, Feb 22, 2018 at 02:38:04PM +0300, Aleksey Kuleshov wrote:
> Hello!
>
> I hit unexpected disconnections because of this patch:
>
> commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2
> Author: Daniel P. Berrange
> Date: Tue Jan 19 11:14:29 2016 +
>
> char: convert from GIOChannel to QIOChannel
>
> In preparation for introducing TLS support to the TCP chardev
> backend, convert existing chardev code from using GIOChannel
> to QIOChannel. This simplifies the chardev code by removing
> most of the OS platform conditional code for dealing with
> file descriptor passing.
>
> Signed-off-by: Daniel P. Berrange
> Message-Id: <1453202071-10289-3-git-send-email-berra...@redhat.com>
> Signed-off-by: Paolo Bonzini
>
> breaks tcp_chr_read:
>
> -static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void
> *opaque)
> +static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void
> *opaque)
> {
> CharDriverState *chr = opaque;
> TCPCharDriver *s = chr->opaque;
> @@ -2938,9 +2801,7 @@ static gboolean tcp_chr_read(GIOChannel *chan,
> GIOCondition cond, void *opaque)
> if (len > s->max_size)
> len = s->max_size;
> size = tcp_chr_recv(chr, (void *)buf, len);
> -if (size == 0 ||
> -(size < 0 &&
> - socket_error() != EAGAIN && socket_error() != EWOULDBLOCK)) {
> +if (size == 0 || size == -1) {
> /* connection closed */
> tcp_chr_disconnect(chr);
> } else if (size > 0) {
>
> since tcp_chr_recv returns -1 on blocking:
Actually it did not do that in this patch - tcp_chr_recv returns -2
on blocking when that patch was written.
>
> static ssize_t tcp_chr_recv(Chardev *chr, char *buf, size_t len)
> {
> ...
> if (ret == QIO_CHANNEL_ERR_BLOCK) {
> errno = EAGAIN;
> ret = -1;
> } else if (ret == -1) {
> errno = EIO;
> }
This was caused by by a later change
commit b6572b4f97a7b126c7b24e165893ed9fe3d72e1f
Author: Marc-André Lureau
Date: Fri Mar 11 18:55:24 2016 +0100
char: translate from QIOChannel error to errno
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [Qemu-block] Limiting coroutine stack usage
On Thu, Feb 22, 2018 at 01:06:33PM +0100, Peter Lieven wrote: > Am 22.02.2018 um 13:03 schrieb Daniel P. Berrangé: > > On Thu, Feb 22, 2018 at 01:02:05PM +0100, Peter Lieven wrote: > >> Am 22.02.2018 um 13:00 schrieb Daniel P. Berrangé: > >>> On Thu, Feb 22, 2018 at 12:51:58PM +0100, Peter Lieven wrote: > Am 22.02.2018 um 12:40 schrieb Daniel P. Berrangé: > > On Thu, Feb 22, 2018 at 12:32:04PM +0100, Kevin Wolf wrote: > >> Am 22.02.2018 um 12:01 hat Peter Lieven geschrieben: > >>> Am 22.02.2018 um 11:57 schrieb Kevin Wolf: > Am 20.02.2018 um 22:54 hat Paolo Bonzini geschrieben: > > On 20/02/2018 18:04, Peter Lieven wrote: > >> Hi, > >> > >> I remember we discussed a long time ago to limit the stack usage > >> of all > >> functions that are executed in a coroutine > >> context to a very low value to be able to safely limit the > >> coroutine > >> stack size as well. > > IIRC the only issue was that hw/ide/atapi.c has mutual recursion > > between > > ide_atapi_cmd_reply_end -> ide_transfer_start -> > > ahci_start_transfer -> > > ide_atapi_cmd_reply_end. > > > > But perhaps it's not an issue, somebody needs to audit the code. > I think John intended to get rid of the recursion sometime, but I > doubt > he has had the time so far. > >>> Apart from this is is possible to define special cflags in the > >>> Makefile.objs just for a subdirectory? I have patches ready to make > >>> the block layer files and other coroutine users compile with > >>> -Wstack-size=2048. But I do not want to specify each file separately. > >> Our Makefiles have lines like this: > >> > >> iscsi.o-cflags := $(LIBISCSI_CFLAGS) > >> > >> I don't think there is a direct mechanism to apply cflags to a whole > >> directory or just to block-obj-y/block-obj-m, but just looping over > >> them > >> could work. I'm not a Makefile expert at all, but after some toying > >> with > >> a simple example, something like this might work: > >> > >> $(foreach x,$(block-obj-y),$(eval $x-cflags += -Wstack-size=2048)) > > You'll need it for anything block layer depends on too - so that's much > > of util/, crypto/ and io/ directories at least. > > > > So perhaps it would be shorter if we do the opposite - set > > -Wstack-size=2048 > > globally for everything in QEMU, and then override -Wstack-size=$BIGGER > > for the (hopefully) few sources that have a larger stack need ? > I tried that already. 2048 is a strong limit for many functions. > It breaks already as soon as some buffer has a size of PATH_MAX, but > thats handleable. But there are some structs around that are very large. > >>> There are surprisingly few "char [PATH_MAX]" variables left in QEMU - we > >>> should have a final push to eliminate them regardless. > >>> > Generally, it would be a good idea to have a global limit, of course. > >>> We could at least put a limit on that matches the current worst case to > >>> prevent it getting worse than it already is. > >> That would be a good idea, yes. > >> > >> How would you handle the override for a smaller -Wstack-usage ? > > If you have multiple -Wstack-size=$XXX flags to GCC, I expect the last > > one wins. So just need to double check that the per-object file CFLAGS > > occur after the global CFLAS in the compiler args > > I will check that, thanks. > > When I am at it, what would be the proper replacement for char[PATH_MAX] ? Generally code should dynamically allocate paths. If they need to sprintf a path, then g_strdup_printf() is the right approach. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [Qemu-block] Limiting coroutine stack usage
Am 22.02.2018 um 13:06 hat Peter Lieven geschrieben: > Am 22.02.2018 um 13:03 schrieb Daniel P. Berrangé: > > On Thu, Feb 22, 2018 at 01:02:05PM +0100, Peter Lieven wrote: > >> Am 22.02.2018 um 13:00 schrieb Daniel P. Berrangé: > >>> On Thu, Feb 22, 2018 at 12:51:58PM +0100, Peter Lieven wrote: > Am 22.02.2018 um 12:40 schrieb Daniel P. Berrangé: > > On Thu, Feb 22, 2018 at 12:32:04PM +0100, Kevin Wolf wrote: > >> Am 22.02.2018 um 12:01 hat Peter Lieven geschrieben: > >>> Am 22.02.2018 um 11:57 schrieb Kevin Wolf: > Am 20.02.2018 um 22:54 hat Paolo Bonzini geschrieben: > > On 20/02/2018 18:04, Peter Lieven wrote: > >> Hi, > >> > >> I remember we discussed a long time ago to limit the stack usage > >> of all > >> functions that are executed in a coroutine > >> context to a very low value to be able to safely limit the > >> coroutine > >> stack size as well. > > IIRC the only issue was that hw/ide/atapi.c has mutual recursion > > between > > ide_atapi_cmd_reply_end -> ide_transfer_start -> > > ahci_start_transfer -> > > ide_atapi_cmd_reply_end. > > > > But perhaps it's not an issue, somebody needs to audit the code. > I think John intended to get rid of the recursion sometime, but I > doubt > he has had the time so far. > >>> Apart from this is is possible to define special cflags in the > >>> Makefile.objs just for a subdirectory? I have patches ready to make > >>> the block layer files and other coroutine users compile with > >>> -Wstack-size=2048. But I do not want to specify each file separately. > >> Our Makefiles have lines like this: > >> > >> iscsi.o-cflags := $(LIBISCSI_CFLAGS) > >> > >> I don't think there is a direct mechanism to apply cflags to a whole > >> directory or just to block-obj-y/block-obj-m, but just looping over > >> them > >> could work. I'm not a Makefile expert at all, but after some toying > >> with > >> a simple example, something like this might work: > >> > >> $(foreach x,$(block-obj-y),$(eval $x-cflags += -Wstack-size=2048)) > > You'll need it for anything block layer depends on too - so that's much > > of util/, crypto/ and io/ directories at least. > > > > So perhaps it would be shorter if we do the opposite - set > > -Wstack-size=2048 > > globally for everything in QEMU, and then override -Wstack-size=$BIGGER > > for the (hopefully) few sources that have a larger stack need ? > I tried that already. 2048 is a strong limit for many functions. > It breaks already as soon as some buffer has a size of PATH_MAX, but > thats handleable. But there are some structs around that are very large. > >>> There are surprisingly few "char [PATH_MAX]" variables left in QEMU - we > >>> should have a final push to eliminate them regardless. > >>> > Generally, it would be a good idea to have a global limit, of course. > >>> We could at least put a limit on that matches the current worst case to > >>> prevent it getting worse than it already is. > >> That would be a good idea, yes. > >> > >> How would you handle the override for a smaller -Wstack-usage ? > > If you have multiple -Wstack-size=$XXX flags to GCC, I expect the last > > one wins. So just need to double check that the per-object file CFLAGS > > occur after the global CFLAS in the compiler args > > I will check that, thanks. > > When I am at it, what would be the proper replacement for char[PATH_MAX] ? g_malloc() with the exact size? Kevin
Re: [Qemu-devel] [PATCH v8 00/13] Interactive Boot Menu for DASD and SCSI Guests on s390x
On 22.02.2018 12:51, Christian Borntraeger wrote: > Series > Acked-by: Christian Borntraeger > > > menu on scsi and dasd bootmaps tested successfully. > > There is one thing that we might want to fix (can be an addon patch since > this is a non-customer > scenario (no libvirt)). > > If you start QEMU manually without a bootindex, the -boot menu=on is ignored > if no drive has a bootindex. > > For example: > > -drive file=/dev/dasda,if=none,id=d1 -device > virtio-blk-ccw,drive=d1,bootindex=1 -boot menu=on > does work > > -drive file=/dev/dasda -boot menu=on > does not > > instead it prints: > qemu-system-s390x: boot menu is not supported for this device type. > > and the boots up the default entry. > That should indeed be a separate patch, as it would move logic currently in the BIOS up to QEMU (find the first defined virtio disk and select it as boot disk). In fact it's more complicated than that, because it would have to properly account for -boot order=[acdn] and produce the respective IPLB. While it makes sense, I wouldn't rush that in but rather change the error message to indicate that -device bootindex is needed to activate the menu, at least for the time being. [...] -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [PATCH v8 01/26] block/mirror: Small absolute-paths simplification
Am 05.02.2018 um 16:18 hat Max Reitz geschrieben: > When invoking drive-mirror in absolute-paths mode, the target's backing > BDS is assigned to it in mirror_exit(). The current logic only does so > if the target does not have that backing BDS already; but it actually > cannot have a backing BDS at all (the BDS is opened with O_NO_BACKING in > qmp_drive_mirror()), so just assert that and assign the new backing BDS > unconditionally. > > Signed-off-by: Max Reitz > Reviewed-by: Alberto Garcia Introducing new assumptions that the graph stays unmodified between the start of a block job and its completion feels a bit adventurous when all of the blockdev work is moving towards making things more flexible. If we really do want to enforce this, I guess you finally found a use case for BLK_PERM_GRAPH_MOD. Kevin
Re: [Qemu-devel] [PATCH v8 02/26] block: Use children list in bdrv_refresh_filename
Am 05.02.2018 um 16:18 hat Max Reitz geschrieben: > bdrv_refresh_filename() should invoke itself recursively on all > children, not just on file. > > With that change, we can remove the manual invocations in blkverify, > quorum, commit, and mirror. > > Signed-off-by: Max Reitz > Reviewed-by: Alberto Garcia Reviewed-by: Kevin Wolf
Re: [Qemu-devel] [PATCH v1] chardev: fix handling of EAGAIN for TCP chardev
On Thu, Feb 22, 2018 at 1:13 PM, Daniel P. Berrangé wrote:
> When this commit was applied
>
> commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2
> Author: Daniel P. Berrange
> Date: Tue Jan 19 11:14:29 2016 +
>
> char: convert from GIOChannel to QIOChannel
>
> The tcp_chr_recv() function was changed to return QIO_CHANNEL_ERR_BLOCK
> which corresonds to -2. As such the handling for EAGAIN was able to be
> removed from tcp_chr_read(). Unfortunately in a later commit:
>
> commit b6572b4f97a7b126c7b24e165893ed9fe3d72e1f
> Author: Marc-André Lureau
> Date: Fri Mar 11 18:55:24 2016 +0100
>
> char: translate from QIOChannel error to errno
>
> The tcp_chr_recv() function was changed back to return -1, with errno
> set to EAGAIN, without also re-addding support for this to tcp_chr_read()
>
> Reported-by: Aleksey Kuleshov
> Signed-off-by: Daniel P. Berrangé
Reviewed-by: Marc-André Lureau
> ---
> chardev/char-socket.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index bdd6cff5f6..b15682c362 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -449,7 +449,7 @@ static gboolean tcp_chr_read(QIOChannel *chan,
> GIOCondition cond, void *opaque)
> len = s->max_size;
> }
> size = tcp_chr_recv(chr, (void *)buf, len);
> -if (size == 0 || size == -1) {
> +if (size == 0 || (size == -1 && errno != EAGAIN)) {
> /* connection closed */
> tcp_chr_disconnect(chr);
> } else if (size > 0) {
> --
> 2.14.3
>
>
--
Marc-André Lureau
Re: [Qemu-devel] [qemu-s390x] [PATCH v8 00/13] Interactive Boot Menu for DASD and SCSI Guests on s390x
On 22.02.2018 12:51, Christian Borntraeger wrote: > Series > Acked-by: Christian Borntraeger OK, thanks. I can pick up the patches from this series and fix the comment in patch 5 and the missing break in patch 13 manually, no need to resend so far. > menu on scsi and dasd bootmaps tested successfully. > > There is one thing that we might want to fix (can be an addon patch since > this is a non-customer > scenario (no libvirt)). I agree that this can be an add-on patch. Thomas
[Qemu-devel] [PATCH 3/9] acpi: reuse AcpiGenericAddress instead of Acpi20GenericAddress
Drop duplicate in form of Acpi20GenericAddress and reuse
AcpiGenericAddress.
Signed-off-by: Igor Mammedov
---
include/hw/acpi/acpi-defs.h | 17 +++--
1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 80c8099..9942bc5 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -40,18 +40,6 @@ enum {
ACPI_FADT_F_LOW_POWER_S0_IDLE_CAPABLE,
};
-/*
- * ACPI 2.0 Generic Address Space definition.
- */
-struct Acpi20GenericAddress {
-uint8_t address_space_id;
-uint8_t register_bit_width;
-uint8_t register_bit_offset;
-uint8_t reserved;
-uint64_t address;
-} QEMU_PACKED;
-typedef struct Acpi20GenericAddress Acpi20GenericAddress;
-
struct AcpiRsdpDescriptor {/* Root System Descriptor Pointer */
uint64_t signature; /* ACPI signature, contains "RSD PTR " */
uint8_t checksum; /* To make sum of struct == 0 */
@@ -167,7 +155,8 @@ struct AcpiGenericAddress {
uint8_t space_id;/* Address space where struct or register exists
*/
uint8_t bit_width; /* Size in bits of given register */
uint8_t bit_offset; /* Bit offset within the register */
-uint8_t access_width;/* Minimum Access size (ACPI 3.0) */
+uint8_t access_width;/* ACPI 3.0: Minimum Access size (ACPI 3.0),
+ACPI 2.0: Reserved, Table 5-1 */
uint64_t address;/* 64-bit address of struct or register */
} QEMU_PACKED;
@@ -456,7 +445,7 @@ typedef struct AcpiGenericTimerTable AcpiGenericTimerTable;
struct Acpi20Hpet {
ACPI_TABLE_HEADER_DEF/* ACPI common table header */
uint32_t timer_block_id;
-Acpi20GenericAddress addr;
+struct AcpiGenericAddress addr;
uint8_thpet_number;
uint16_t min_tick;
uint8_tpage_protect;
--
2.7.4
Re: [Qemu-devel] [PATCH v1] chardev: fix handling of EAGAIN for TCP chardev
Thank you, Daniel!
22.02.2018, 15:14, "Daniel P. Berrangé" :
> When this commit was applied
>
> commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2
> Author: Daniel P. Berrange
> Date: Tue Jan 19 11:14:29 2016 +
>
> char: convert from GIOChannel to QIOChannel
>
> The tcp_chr_recv() function was changed to return QIO_CHANNEL_ERR_BLOCK
> which corresonds to -2. As such the handling for EAGAIN was able to be
> removed from tcp_chr_read(). Unfortunately in a later commit:
>
> commit b6572b4f97a7b126c7b24e165893ed9fe3d72e1f
> Author: Marc-André Lureau
> Date: Fri Mar 11 18:55:24 2016 +0100
>
> char: translate from QIOChannel error to errno
>
> The tcp_chr_recv() function was changed back to return -1, with errno
> set to EAGAIN, without also re-addding support for this to tcp_chr_read()
>
> Reported-by: Aleksey Kuleshov
> Signed-off-by: Daniel P. Berrangé
> ---
> chardev/char-socket.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index bdd6cff5f6..b15682c362 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -449,7 +449,7 @@ static gboolean tcp_chr_read(QIOChannel *chan,
> GIOCondition cond, void *opaque)
> len = s->max_size;
> }
> size = tcp_chr_recv(chr, (void *)buf, len);
> - if (size == 0 || size == -1) {
> + if (size == 0 || (size == -1 && errno != EAGAIN)) {
> /* connection closed */
> tcp_chr_disconnect(chr);
> } else if (size > 0) {
> --
> 2.14.3
[Qemu-devel] [PATCH 4/9] acpi: add build_append_gas() helper for Generic Address Structure
it will help to add Generic Address Structure to ACPI tables
without using packed C structures and avoid endianness
issues as API doesn't need an explicit conversion.
Signed-off-by: Igor Mammedov
---
include/hw/acpi/aml-build.h | 20
hw/acpi/aml-build.c | 16
2 files changed, 36 insertions(+)
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 88d0738..8692ccc 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -78,6 +78,15 @@ typedef enum {
} AmlUpdateRule;
typedef enum {
+AML_AS_SYSTEM_MEMORY = 0X00,
+AML_AS_SYSTEM_IO = 0X01,
+AML_AS_PCI_CONFIG = 0X02,
+AML_AS_EMBEDDED_CTRL = 0X03,
+AML_AS_SMBUS = 0X04,
+AML_AS_FFH = 0X7F,
+} AmlAddressSpace;
+
+typedef enum {
AML_SYSTEM_MEMORY = 0X00,
AML_SYSTEM_IO = 0X01,
AML_PCI_CONFIG = 0X02,
@@ -389,6 +398,17 @@ int
build_append_named_dword(GArray *array, const char *name_format, ...)
GCC_FMT_ATTR(2, 3);
+void build_append_gas(GArray *table, AmlAddressSpace as,
+ uint8_t bit_width, uint8_t bit_offset,
+ uint8_t access_width, uint64_t address);
+
+static inline void
+build_append_gas_from_struct(GArray *table, const struct AcpiGenericAddress *s)
+{
+build_append_gas(table, s->space_id, s->bit_width, s->bit_offset,
+ s->access_width, s->address);
+}
+
void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
uint64_t len, int node, MemoryAffinityFlags flags);
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 36a6cc4..3fef5f6 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -258,6 +258,22 @@ static void build_append_int(GArray *table, uint64_t value)
}
}
+/* Generic Address Structure (GAS)
+ * ACPI 2.0/3.0: 5.2.3.1 Generic Address Structure
+ * 2.0 compat note:
+ *@access_width must be 0, see ACPI 2.0:Table 5-1
+ */
+void build_append_gas(GArray *table, AmlAddressSpace as,
+ uint8_t bit_width, uint8_t bit_offset,
+ uint8_t access_width, uint64_t address)
+{
+build_append_int_noprefix(table, as, 1);
+build_append_int_noprefix(table, bit_width, 1);
+build_append_int_noprefix(table, bit_offset, 1);
+build_append_int_noprefix(table, access_width, 1);
+build_append_int_noprefix(table, address, 8);
+}
+
/*
* Build NAME(, 0x) where 0x is encoded as a dword,
* and return the offset to 0x for runtime patching.
--
2.7.4
[Qemu-devel] [PATCH 2/9] pc: replace pm object initialization with one-liner in acpi_get_pm_info()
next patch will need it before it gets to piix4/lpc branches
that initializes 'obj' now.
Signed-off-by: Igor Mammedov
---
hw/i386/acpi-build.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index deb440f..b85fefe 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -128,7 +128,7 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
{
Object *piix = piix4_pm_find();
Object *lpc = ich9_lpc_find();
-Object *obj = NULL;
+Object *obj = piix ? piix : lpc;
QObject *o;
pm->force_rev1_fadt = false;
@@ -138,7 +138,6 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
if (piix) {
/* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
pm->force_rev1_fadt = true;
-obj = piix;
pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
pm->pcihp_io_base =
object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
@@ -146,7 +145,6 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
}
if (lpc) {
-obj = lpc;
pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
}
assert(obj);
--
2.7.4
[Qemu-devel] [PATCH 6/9] pc: acpi: use build_append_foo() API to construct FADT
build_append_foo() API doesn't need explicit endianness
conversions which eliminates a source of errors and
it makes build_fadt() look like declarative definition of
FADT table in ACPI spec, which makes it easy to review.
Also it allows easily extending FADT to support other
revisions which will be used by follow up patches
where build_fadt() will be reused for ARM target.
Signed-off-by: Igor Mammedov
---
hw/i386/acpi-build.c | 147 +--
1 file changed, 85 insertions(+), 62 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 706ba35..544a4bc 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -299,87 +299,110 @@ build_facs(GArray *table_data, BIOSLinker *linker)
facs->length = cpu_to_le32(sizeof(*facs));
}
-/* Load chipset information in FADT */
-static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiFadtData f)
-{
-fadt->model = f.int_model;
-fadt->reserved1 = 0;
-fadt->sci_int = cpu_to_le16(f.sci_int);
-fadt->smi_cmd = cpu_to_le32(f.smi_cmd);
-fadt->acpi_enable = f.acpi_enable_cmd;
-fadt->acpi_disable = f.acpi_disable_cmd;
-/* EVT, CNT, TMR offset matches hw/acpi/core.c */
-fadt->pm1a_evt_blk = cpu_to_le32(f.pm1_evt.address);
-fadt->pm1a_cnt_blk = cpu_to_le32(f.pm1_cnt.address);
-fadt->pm_tmr_blk = cpu_to_le32(f.pm_tmr.address);
-fadt->gpe0_blk = cpu_to_le32(f.gpe0_blk.address);
-/* EVT, CNT, TMR length matches hw/acpi/core.c */
-fadt->pm1_evt_len = f.pm1_evt.bit_width / 8;
-fadt->pm1_cnt_len = f.pm1_cnt.bit_width / 8;
-fadt->pm_tmr_len = f.pm_tmr.bit_width / 8;
-fadt->gpe0_blk_len = f.gpe0_blk.bit_width / 8;
-fadt->plvl2_lat = cpu_to_le16(f.c2_latency);
-fadt->plvl3_lat = cpu_to_le16(f.c3_latency);
-fadt->flags = cpu_to_le32(f.flags);
-fadt->century = f.rtc_century;
-if (f.rev == 1) {
-return;
-}
-
-fadt->reset_value = f.reset_val;
-fadt->reset_register = f.reset_reg;
-fadt->reset_register.address = cpu_to_le64(f.reset_reg.address);
-
-fadt->xpm1a_event_block = f.pm1_evt;
-fadt->xpm1a_event_block.address = cpu_to_le64(f.pm1_evt.address);
-
-fadt->xpm1a_control_block = f.pm1_cnt;
-fadt->xpm1a_control_block.address = cpu_to_le64(f.pm1_cnt.address);
-
-fadt->xpm_timer_block = f.pm_tmr;
-fadt->xpm_timer_block.address = cpu_to_le64(f.pm_tmr.address);
-
-fadt->xgpe0_block = f.gpe0_blk;
-fadt->xgpe0_block.address = cpu_to_le64(f.gpe0_blk.address);
-}
-
-
/* FADT */
static void
-build_fadt(GArray *table_data, BIOSLinker *linker, AcpiFadtData *f,
+build_fadt(GArray *tbl, BIOSLinker *linker, AcpiFadtData *f,
const char *oem_id, const char *oem_table_id)
{
-AcpiFadtDescriptorRev3 *fadt = acpi_data_push(table_data, sizeof(*fadt));
-unsigned fw_ctrl_offset = (char *)&fadt->firmware_ctrl - table_data->data;
-unsigned dsdt_entry_offset = (char *)&fadt->dsdt - table_data->data;
-unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - table_data->data;
-int fadt_size = sizeof(*fadt);
+int off;
+int fadt_start = tbl->len;
+
+acpi_data_push(tbl, sizeof(AcpiTableHeader));
-/* FACS address to be filled by Guest linker */
+/* FACS address to be filled by Guest linker at runtime */
+off = tbl->len;
+build_append_int_noprefix(tbl, 0, 4); /* FIRMWARE_CTRL */
if (f->facs_tbl_offset) {
bios_linker_loader_add_pointer(linker,
-ACPI_BUILD_TABLE_FILE, fw_ctrl_offset, sizeof(fadt->firmware_ctrl),
+ACPI_BUILD_TABLE_FILE, off, 4,
ACPI_BUILD_TABLE_FILE, *f->facs_tbl_offset);
}
-/* DSDT address to be filled by Guest linker */
-fadt_setup(fadt, *f);
+/* DSDT address to be filled by Guest linker at runtime */
+off = tbl->len;
+build_append_int_noprefix(tbl, 0, 4); /* DSDT */
if (f->dsdt_tbl_offset) {
bios_linker_loader_add_pointer(linker,
-ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
+ACPI_BUILD_TABLE_FILE, off, 4,
ACPI_BUILD_TABLE_FILE, *f->dsdt_tbl_offset);
}
+
+/* ACPI1.0: INT_MODEL, ACPI2.0+: Reserved */
+build_append_int_noprefix(tbl, f->int_model /* Multiple APIC */, 1);
+/* Preferred_PM_Profile */
+build_append_int_noprefix(tbl, 0 /* Unspecified */, 1);
+build_append_int_noprefix(tbl, f->sci_int, 2); /* SCI_INT */
+build_append_int_noprefix(tbl, f->smi_cmd, 4); /* SMI_CMD */
+build_append_int_noprefix(tbl, f->acpi_enable_cmd, 1); /* ACPI_ENABLE */
+build_append_int_noprefix(tbl, f->acpi_disable_cmd, 1); /* ACPI_DISABLE */
+build_append_int_noprefix(tbl, 0 /* not supported */, 1); /* S4BIOS_REQ */
+/* ACPI1.0: Reserved, ACPI2.0+: PSTATE_CNT */
+build_append_int_noprefix(tbl, 0, 1);
+build_append_int_noprefix(tbl, f->pm1_evt.address, 4); /* PM1a_EVT_BLK */
+build_append_int_noprefix(tbl, 0, 4); /* PM1b_EVT_BLK */
+build_append_int_noprefi
[Qemu-devel] [PATCH 0/9] generalize build_fadt()
Series first cleanups ACPI code around build_fadt() and then converts current packed structure approach to a build_append_FOO() API, getting rid of error prone explicit endianness conversions in code and making build_fadt() look more like APCI table declaration from spec, which should be easier to read/maintain. After that build_fadt() becomes generic enough that we could drop ARM specific implementation and reuse generic build_fadt(), reducing code duplication. PS: tested only x86 which has make check coverage, ARM was only slightly tested. git tree for testing: https://github.com/imammedo/qemu.git fadt_refactoring_v1 CC: "Michael S. Tsirkin" CC: Shannon Zhao CC: Peter Maydell CC: Andrew Jones CC: qemu-devel@nongnu.org CC: qemu-...@nongnu.org Igor Mammedov (9): acpi: remove unused acpi-dsdt.aml pc: replace pm object initialization with one-liner in acpi_get_pm_info() acpi: reuse AcpiGenericAddress instead of Acpi20GenericAddress acpi: add build_append_gas() helper for Generic Address Structure pc: acpi: isolate FADT specific data into AcpiFadtData structure pc: acpi: use build_append_foo() API to construct FADT acpi: move build_fadt() from i386 specific to generic ACPI source virt_arm: acpi: reuse common build_fadt() tests: acpi: don't read all fields in test_acpi_fadt_table() include/hw/acpi/acpi-defs.h | 136 +++--- include/hw/acpi/aml-build.h | 23 ++ Makefile| 1 - hw/acpi/aml-build.c | 140 +++ hw/arm/virt-acpi-build.c| 39 - hw/i386/acpi-build.c| 197 ++-- pc-bios/acpi-dsdt.aml | Bin 4405 -> 0 bytes tests/bios-tables-test.c| 82 -- 8 files changed, 292 insertions(+), 326 deletions(-) delete mode 100644 pc-bios/acpi-dsdt.aml -- 2.7.4
[Qemu-devel] [PATCH 5/9] pc: acpi: isolate FADT specific data into AcpiFadtData structure
move FADT data initialization out of fadt_setup() into dedicated
init_fadt_data() that will set common for pc/q35 values in
AcpiFadtData structure and acpi_get_pm_info() will complement
it with pc/q35 specific values initialization.
That will allow to get rid of fadt_setup() and generalize
build_fadt() so it could be easily extended for rev5 and
reused by ARM target.
While at it also move facs/dsdt/xdsdt offsets from build_fadt()
arg list into AcpiFadtData, as they belong to the same dataset.
Signed-off-by: Igor Mammedov
---
include/hw/acpi/acpi-defs.h | 28 ++
hw/i386/acpi-build.c| 204
2 files changed, 138 insertions(+), 94 deletions(-)
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 9942bc5..e0accc4 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -175,6 +175,34 @@ struct AcpiFadtDescriptorRev5_1 {
typedef struct AcpiFadtDescriptorRev5_1 AcpiFadtDescriptorRev5_1;
+typedef struct AcpiFadtData {
+struct AcpiGenericAddress pm1_cnt; /* PM1a_CNT_BLK */
+struct AcpiGenericAddress pm1_evt; /* PM1a_EVT_BLK */
+struct AcpiGenericAddress pm_tmr;/* PM_TMR_BLK */
+struct AcpiGenericAddress gpe0_blk; /* GPE0_BLK */
+struct AcpiGenericAddress reset_reg; /* RESET_REG */
+uint8_t reset_val; /* RESET_VALUE */
+uint8_t rev; /* Revision */
+uint32_t flags;/* Flags */
+uint32_t smi_cmd; /* SMI_CMD */
+uint16_t sci_int; /* SCI_INT */
+uint8_t int_model;/* INT_MODEL */
+uint8_t acpi_enable_cmd; /* ACPI_ENABLE */
+uint8_t acpi_disable_cmd; /* ACPI_DISABLE */
+uint8_t rtc_century; /* CENTURY */
+uint16_t c2_latency; /* P_LVL2_LAT */
+uint16_t c3_latency; /* P_LVL3_LAT */
+
+/*
+ * respective tables offsets within ACPI_BUILD_TABLE_FILE,
+ * NULL if table doesn't exist (in that case field's value
+ * won't be patched by linker and will be kept set to 0)
+ */
+unsigned *facs_tbl_offset; /* FACS offset in */
+unsigned *dsdt_tbl_offset;
+unsigned *xdsdt_tbl_offset;
+} AcpiFadtData;
+
#define ACPI_FADT_ARM_PSCI_COMPLIANT (1 << 0)
#define ACPI_FADT_ARM_PSCI_USE_HVC(1 << 1)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b85fefe..706ba35 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -91,17 +91,11 @@ typedef struct AcpiMcfgInfo {
} AcpiMcfgInfo;
typedef struct AcpiPmInfo {
-bool force_rev1_fadt;
bool s3_disabled;
bool s4_disabled;
bool pcihp_bridge_en;
uint8_t s4_val;
-uint16_t sci_int;
-uint8_t acpi_enable_cmd;
-uint8_t acpi_disable_cmd;
-uint32_t gpe0_blk;
-uint32_t gpe0_blk_len;
-uint32_t io_base;
+AcpiFadtData fadt;
uint16_t cpu_hp_io_base;
uint16_t pcihp_io_base;
uint16_t pcihp_io_len;
@@ -124,20 +118,60 @@ typedef struct AcpiBuildPciBusHotplugState {
bool pcihp_bridge_en;
} AcpiBuildPciBusHotplugState;
+#define ACPI_PORT_SMI_CMD 0x00b2 /* TODO: this is APM_CNT_IOPORT */
+
+static void init_fadt_data(Object *o, AcpiFadtData *data)
+{
+uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
+AmlAddressSpace as = AML_AS_SYSTEM_IO;
+AcpiFadtData fadt = {
+.rev = 3,
+.flags =
+(1 << ACPI_FADT_F_WBINVD) |
+(1 << ACPI_FADT_F_PROC_C1) |
+(1 << ACPI_FADT_F_SLP_BUTTON) |
+(1 << ACPI_FADT_F_RTC_S4) |
+(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK) |
+/* APIC destination mode ("Flat Logical") has an upper limit of 8
+ * CPUs for more than 8 CPUs, "Clustered Logical" mode has to be
+ * used
+ */
+((max_cpus > 8) ? (1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL) : 0),
+.int_model = 1 /* Multiple APIC */,
+.rtc_century = RTC_CENTURY,
+.c2_latency = 0xfff /* C2 state not supported */,
+.c3_latency = 0xfff /* C3 state not supported */,
+.smi_cmd = ACPI_PORT_SMI_CMD,
+.sci_int = object_property_get_uint(o, ACPI_PM_PROP_SCI_INT, NULL),
+.acpi_enable_cmd =
+object_property_get_uint(o, ACPI_PM_PROP_ACPI_ENABLE_CMD, NULL),
+.acpi_disable_cmd =
+object_property_get_uint(o, ACPI_PM_PROP_ACPI_DISABLE_CMD, NULL),
+.pm1_evt = { .space_id = as, .bit_width = 4 * 8, .address = io },
+.pm1_cnt = { .space_id = as, .bit_width = 2 * 8, .address = io + 0x04
},
+.pm_tmr = { .space_id = as, .bit_width = 4 * 8, .address = io + 0x08 },
+.gpe0_blk = { .space_id = as, .bit_width =
+object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK_LEN, NULL) * 8,
+.address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK, NULL)
+},
+};
+*data = fadt;
+}
+
static void acpi_get_pm_info(AcpiPmInfo *pm)
{
Object *piix = piix4_pm_find();
[Qemu-devel] [PATCH 8/9] virt_arm: acpi: reuse common build_fadt()
Extend generic build_fadt() to support rev5.1 FADT
and reuse it for 'virt' board, it would allow to
phase out usage of AcpiFadtDescriptorRev5_1 and
later ACPI_FADT_COMMON_DEF.
Signed-off-by: Igor Mammedov
---
include/hw/acpi/acpi-defs.h | 12 ++--
hw/acpi/aml-build.c | 21 -
hw/arm/virt-acpi-build.c| 33 -
3 files changed, 34 insertions(+), 32 deletions(-)
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index e0accc4..34e49dc 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -165,16 +165,6 @@ struct AcpiFadtDescriptorRev3 {
} QEMU_PACKED;
typedef struct AcpiFadtDescriptorRev3 AcpiFadtDescriptorRev3;
-struct AcpiFadtDescriptorRev5_1 {
-ACPI_FADT_COMMON_DEF
-/* 64-bit Sleep Control register (ACPI 5.0) */
-struct AcpiGenericAddress sleep_control;
-/* 64-bit Sleep Status register (ACPI 5.0) */
-struct AcpiGenericAddress sleep_status;
-} QEMU_PACKED;
-
-typedef struct AcpiFadtDescriptorRev5_1 AcpiFadtDescriptorRev5_1;
-
typedef struct AcpiFadtData {
struct AcpiGenericAddress pm1_cnt; /* PM1a_CNT_BLK */
struct AcpiGenericAddress pm1_evt; /* PM1a_EVT_BLK */
@@ -192,6 +182,8 @@ typedef struct AcpiFadtData {
uint8_t rtc_century; /* CENTURY */
uint16_t c2_latency; /* P_LVL2_LAT */
uint16_t c3_latency; /* P_LVL3_LAT */
+uint16_t arm_boot_arch;/* ARM_BOOT_ARCH */
+uint8_t minor_ver; /* FADT Minor Version */
/*
* respective tables offsets within ACPI_BUILD_TABLE_FILE,
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 17fb71b..5a33cd0 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1755,7 +1755,14 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const
AcpiFadtData *f,
build_append_gas_from_struct(tbl, &f->reset_reg); /* RESET_REG */
build_append_int_noprefix(tbl, f->reset_val, 1); /* RESET_VALUE */
-build_append_int_noprefix(tbl, 0, 3); /* Reserved, ACPI 3.0 */
+/* Since ACPI 5.1 */
+if ((f->rev >= 6) || ((f->rev == 5) && f->minor_ver > 0)) {
+build_append_int_noprefix(tbl, f->arm_boot_arch, 2); /* ARM_BOOT_ARCH
*/
+/* FADT Minor Version */
+build_append_int_noprefix(tbl, f->minor_ver, 1);
+} else {
+build_append_int_noprefix(tbl, 0, 3); /* Reserved upto ACPI 5.0 */
+}
build_append_int_noprefix(tbl, 0, 8); /* X_FIRMWARE_CTRL */
/* XDSDT address to be filled by Guest linker at runtime */
@@ -1779,6 +1786,18 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const
AcpiFadtData *f,
build_append_gas_from_struct(tbl, &f->gpe0_blk); /* X_GPE0_BLK */
build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0); /* X_GPE1_BLK */
+if (f->rev <= 4) {
+goto build_hdr;
+}
+
+/* SLEEP_CONTROL_REG */
+build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
+/* SLEEP_STATUS_REG */
+build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0);
+
+/* TODO: extra fields need to be added to support revisions above rev5 */
+assert(f->rev == 5);
+
build_hdr:
build_header(linker, tbl, (void *)(tbl->data + fadt_start),
"FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id);
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index b644da9..c7c6a57 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -654,39 +654,30 @@ build_madt(GArray *table_data, BIOSLinker *linker,
VirtMachineState *vms)
static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
VirtMachineState *vms, unsigned dsdt_tbl_offset)
{
-int fadt_start = table_data->len;
-AcpiFadtDescriptorRev5_1 *fadt = acpi_data_push(table_data, sizeof(*fadt));
-unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - table_data->data;
-uint16_t bootflags;
+/* ACPI v5.1 */
+AcpiFadtData fadt = {
+.rev = 5,
+.minor_ver = 1,
+.flags = 1 << ACPI_FADT_F_HW_REDUCED_ACPI,
+.xdsdt_tbl_offset = &dsdt_tbl_offset,
+};
switch (vms->psci_conduit) {
case QEMU_PSCI_CONDUIT_DISABLED:
-bootflags = 0;
+fadt.arm_boot_arch = 0;
break;
case QEMU_PSCI_CONDUIT_HVC:
-bootflags = ACPI_FADT_ARM_PSCI_COMPLIANT | ACPI_FADT_ARM_PSCI_USE_HVC;
+fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT |
+ ACPI_FADT_ARM_PSCI_USE_HVC;
break;
case QEMU_PSCI_CONDUIT_SMC:
-bootflags = ACPI_FADT_ARM_PSCI_COMPLIANT;
+fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT;
break;
default:
g_assert_not_reached();
}
-/* Hardware Reduced = 1 and use PSCI 0.2+ */
-fadt->flags = cpu_to_le32(1 << ACPI_FADT_F_HW_REDUCED_ACPI);
-fadt->arm_boot_flags = cpu_to_le16(bootflags);
-
-/* ACPI v5.1 (fadt->revision.fadt->minor_revision) */
-fadt->minor_revision =
[Qemu-devel] [PATCH 1/9] acpi: remove unused acpi-dsdt.aml
SeaBIOS blob which is currently shipped with QEMU
doesn't need acpi-dsdt.aml nor is able to use it
and code that loaded it QEMU was removed by
(commit 9fb7aaaf4c "pc: drop external DSDT loading")
in 2013.
Signed-off-by: Igor Mammedov
CC: Gerd Hoffmann
---
Makefile | 1 -
pc-bios/acpi-dsdt.aml | Bin 4405 -> 0 bytes
2 files changed, 1 deletion(-)
delete mode 100644 pc-bios/acpi-dsdt.aml
diff --git a/Makefile b/Makefile
index 90e05ac..f3a4cf9 100644
--- a/Makefile
+++ b/Makefile
@@ -648,7 +648,6 @@ bepocz
ifdef INSTALL_BLOBS
BLOBS=bios.bin bios-256k.bin sgabios.bin vgabios.bin vgabios-cirrus.bin \
vgabios-stdvga.bin vgabios-vmware.bin vgabios-qxl.bin vgabios-virtio.bin \
-acpi-dsdt.aml \
ppc_rom.bin openbios-sparc32 openbios-sparc64 openbios-ppc QEMU,tcx.bin
QEMU,cgthree.bin \
pxe-e1000.rom pxe-eepro100.rom pxe-ne2k_pci.rom \
pxe-pcnet.rom pxe-rtl8139.rom pxe-virtio.rom \
diff --git a/pc-bios/acpi-dsdt.aml b/pc-bios/acpi-dsdt.aml
deleted file mode 100644
index
558c10f51ccbbf9ec2f47a4a998a7055059a8963..
GIT binary patch
literal 0
HcmV?d1
literal 4405
zcmb7{O>f)C8OI;KNTx=#Ov$uk$9XY?b_=xIjb0q@_EJP5WkrsxFrpHqP*76NE}&CE
zqzSOzpn&5RO*WTe*G|Kam8znLL9
zdJ#bTx_LkF0GjuGY(WhGo!+3koGWcQ9rFPU5B+94((<~g4WH$C9dAv`{m^gT
zZEJrW$A5|A$IoMJl)(Ns&a3@V^7|L@K9JFq{e&^9IOQm8M#H0x!0S}3=w`>a8*i9l
zMGe0XR&=-HYff+FBQoL^UcVI@%=&fLZ0Lo8-=
z2|2%0`vJHO7J2;;UQ)-|gCMNeL^TdtX>}ZQ>$N1Pgb_W)N-Ls=$)m@-itRY~WHYgk
zgX+BqrWEW?)FoC3!tE_lT@6}k^@E_hy_E!2ipVPzkypAAJ^BL$Apdw8JA0Oxf|hkN
zXbsXi&~OfL^l_GN27^7ov3&C`59aWhLwfmMtLJY9eLvcCx1(^-fP`A&gqlWQ#LS5&
z_E*O-9LM?DYzmXYSH~mx^T>vO|2H#*DO<8=Sc*kf_+yTS?9DqcX}p{p+4*D-kG#yi
zb|d180Xv{$XK)dCIxy@PNAErQ+8n3KJVco~H$7En{Z-6#s#%p5=&X1+|
z>%skMU4%Dqj4^z*PT^@
zyarVauZe}VjsYb+#sOA%+7j58pBiUkMT(uE)EZc=I=hhhb|Mzl_$me4&!?nw8e>
zrn?k)tz9iS(8fRw?snkyc5t5KZ$5k#v#U?yN%1MgInZJNd^U)+Nr_tgvleDJyAxVO
zPWv%F!Kn)RgVL?v9+vVZzHHF#-SR=yHLN$FWK%oSQ8ZIwpzxryXxg(Ge)CX;b46Zg
zSP;*+ADX6;JTX4^)VU|xo+|Q8P4P9NjA+U|QIaS2hTGy7TG*Z{@=Q$);fbc)6D4`3
zS@1IOvr*fi{Ihn%A6i%jcqLexDH;OrNO!%zi
z70$fMiBjgo54$voXZO5vdW24=5!TK
zSK)M3PLwj|io&^~aIUDFC}qx7g>zNmTva(y%AB|xl-BJ9h4X^SiBjfVQ#jWY&NY=2
zrOdgmaIPzy>nbNone(E;c~RlKsB)r|IX4te+zyF%j(;^bR8EvK=Ou;nlEQgO_^uO`jgU*EWn+e7WD5_
zEWB0eR-;?pa+f=I@RvWyJ!OYu+`;CiEbnf2?s)wi8uTm00?U7yg&aRY9KcIzV;Q`6
zCiz!mc9@K*KBea2QFnpf=yXS7<8GMt+Vm$6i>qw;%L3#K{9WM*1%OT{c#v2UJ3ZpXq^
zT>AQZ($|Maw@suE&!;y<`g94=kqD@_qz
zV0*#s-WcMfm}eH?9)hk>GJY{)I`G1PBt~VzbmU(20$kE(UXx6W8+$q?xy%b%yZW%q
z{)wleKHuy9jcpE}*<9c)y5YFHS*&<~ODl{%!&5$OWOp*J;^)+h)37m&ChTd<7T}A0
zZU426&7a``5jTMQ$
Re: [Qemu-devel] [PATCH] Fix ast2500 protection register emulation
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180220132627.4163-1-hlan...@devever.net
Subject: [Qemu-devel] [PATCH] Fix ast2500 protection register emulation
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
8b610e4ed2 Fix ast2500 protection register emulation
=== OUTPUT BEGIN ===
Checking PATCH 1/1: Fix ast2500 protection register emulation...
ERROR: suspect code indent for conditional statements (4, 6)
#75: FILE: hw/misc/aspeed_sdmc.c:113:
+if (addr == R_PROT) {
+ s->regs[addr] = (data == PROT_KEY_UNLOCK) ? 1 : 0;
total: 1 errors, 0 warnings, 38 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org
[Qemu-devel] [PATCH 7/9] acpi: move build_fadt() from i386 specific to generic ACPI source
it will be extended and reused in follow up patch by ARM target
Signed-off-by: Igor Mammedov
---
include/hw/acpi/aml-build.h | 3 ++
hw/acpi/aml-build.c | 105 +++
hw/arm/virt-acpi-build.c| 6 +--
hw/i386/acpi-build.c| 106
4 files changed, 111 insertions(+), 109 deletions(-)
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 8692ccc..6c36903 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -413,4 +413,7 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem,
uint64_t base,
uint64_t len, int node, MemoryAffinityFlags flags);
void build_slit(GArray *table_data, BIOSLinker *linker);
+
+void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
+const char *oem_id, const char *oem_table_id);
#endif
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 3fef5f6..17fb71b 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1678,3 +1678,108 @@ void build_slit(GArray *table_data, BIOSLinker *linker)
"SLIT",
table_data->len - slit_start, 1, NULL, NULL);
}
+
+/* build rev1/rev3 FADT */
+void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
+const char *oem_id, const char *oem_table_id)
+{
+int off;
+int fadt_start = tbl->len;
+
+acpi_data_push(tbl, sizeof(AcpiTableHeader));
+
+/* FACS address to be filled by Guest linker at runtime */
+off = tbl->len;
+build_append_int_noprefix(tbl, 0, 4); /* FIRMWARE_CTRL */
+if (f->facs_tbl_offset) {
+bios_linker_loader_add_pointer(linker,
+ACPI_BUILD_TABLE_FILE, off, 4,
+ACPI_BUILD_TABLE_FILE, *f->facs_tbl_offset);
+}
+
+/* DSDT address to be filled by Guest linker at runtime */
+off = tbl->len;
+build_append_int_noprefix(tbl, 0, 4); /* DSDT */
+if (f->dsdt_tbl_offset) {
+bios_linker_loader_add_pointer(linker,
+ACPI_BUILD_TABLE_FILE, off, 4,
+ACPI_BUILD_TABLE_FILE, *f->dsdt_tbl_offset);
+}
+
+/* ACPI1.0: INT_MODEL, ACPI2.0+: Reserved */
+build_append_int_noprefix(tbl, f->int_model /* Multiple APIC */, 1);
+/* Preferred_PM_Profile */
+build_append_int_noprefix(tbl, 0 /* Unspecified */, 1);
+build_append_int_noprefix(tbl, f->sci_int, 2); /* SCI_INT */
+build_append_int_noprefix(tbl, f->smi_cmd, 4); /* SMI_CMD */
+build_append_int_noprefix(tbl, f->acpi_enable_cmd, 1); /* ACPI_ENABLE */
+build_append_int_noprefix(tbl, f->acpi_disable_cmd, 1); /* ACPI_DISABLE */
+build_append_int_noprefix(tbl, 0 /* not supported */, 1); /* S4BIOS_REQ */
+/* ACPI1.0: Reserved, ACPI2.0+: PSTATE_CNT */
+build_append_int_noprefix(tbl, 0, 1);
+build_append_int_noprefix(tbl, f->pm1_evt.address, 4); /* PM1a_EVT_BLK */
+build_append_int_noprefix(tbl, 0, 4); /* PM1b_EVT_BLK */
+build_append_int_noprefix(tbl, f->pm1_cnt.address, 4); /* PM1a_CNT_BLK */
+build_append_int_noprefix(tbl, 0, 4); /* PM1b_CNT_BLK */
+build_append_int_noprefix(tbl, 0, 4); /* PM2_CNT_BLK */
+build_append_int_noprefix(tbl, f->pm_tmr.address, 4); /* PM_TMR_BLK */
+build_append_int_noprefix(tbl, f->gpe0_blk.address, 4); /* GPE0_BLK */
+build_append_int_noprefix(tbl, 0, 4); /* GPE1_BLK */
+/* PM1_EVT_LEN */
+build_append_int_noprefix(tbl, f->pm1_evt.bit_width / 8, 1);
+/* PM1_CNT_LEN */
+build_append_int_noprefix(tbl, f->pm1_cnt.bit_width / 8, 1);
+build_append_int_noprefix(tbl, 0, 1); /* PM2_CNT_LEN */
+build_append_int_noprefix(tbl, f->pm_tmr.bit_width / 8, 1); /* PM_TMR_LEN
*/
+/* GPE0_BLK_LEN */
+build_append_int_noprefix(tbl, f->gpe0_blk.bit_width / 8, 1);
+build_append_int_noprefix(tbl, 0, 1); /* GPE1_BLK_LEN */
+build_append_int_noprefix(tbl, 0, 1); /* GPE1_BASE */
+build_append_int_noprefix(tbl, 0, 1); /* CST_CNT */
+build_append_int_noprefix(tbl, f->c2_latency, 2); /* P_LVL2_LAT */
+build_append_int_noprefix(tbl, f->c3_latency, 2); /* P_LVL3_LAT */
+build_append_int_noprefix(tbl, 0, 2); /* FLUSH_SIZE */
+build_append_int_noprefix(tbl, 0, 2); /* FLUSH_STRIDE */
+build_append_int_noprefix(tbl, 0, 1); /* DUTY_OFFSET */
+build_append_int_noprefix(tbl, 0, 1); /* DUTY_WIDTH */
+build_append_int_noprefix(tbl, 0, 1); /* DAY_ALRM */
+build_append_int_noprefix(tbl, 0, 1); /* MON_ALRM */
+build_append_int_noprefix(tbl, f->rtc_century, 1); /* CENTURY */
+build_append_int_noprefix(tbl, 0, 2); /* IAPC_BOOT_ARCH */
+build_append_int_noprefix(tbl, 0, 1); /* Reserved */
+build_append_int_noprefix(tbl, f->flags, 4); /* Flags */
+
+if (f->rev == 1) {
+goto build_hdr;
+}
+
+build_append_gas_from_struct(tbl, &f->reset_reg); /* RESET_REG */
+build_append_int_noprefix(tbl, f->reset_val, 1); /* RESET_VALUE */
+build_append_int_noprefix(tb
[Qemu-devel] [PATCH 9/9] tests: acpi: don't read all fields in test_acpi_fadt_table()
there is no point to read fields here but not actually
checking them so drop it and read only header + dsdt/facs
addresses since it's needed later to fetch that tables.
With this cleanup we can get rid of AcpiFadtDescriptorRev3/
ACPI_FADT_COMMON_DEF which have no users left.
Signed-off-by: Igor Mammedov
---
include/hw/acpi/acpi-defs.h | 81
tests/bios-tables-test.c| 82 ++---
2 files changed, 18 insertions(+), 145 deletions(-)
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 34e49dc..1f872ee 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -75,82 +75,6 @@ struct AcpiTableHeader {
} QEMU_PACKED;
typedef struct AcpiTableHeader AcpiTableHeader;
-/*
- * ACPI Fixed ACPI Description Table (FADT)
- */
-#define ACPI_FADT_COMMON_DEF /* FADT common definition */ \
-ACPI_TABLE_HEADER_DEF/* ACPI common table header */ \
-uint32_t firmware_ctrl; /* Physical address of FACS */ \
-uint32_t dsdt; /* Physical address of DSDT */ \
-uint8_t model;/* System Interrupt Model */ \
-uint8_t reserved1;/* Reserved */ \
-uint16_t sci_int; /* System vector of SCI interrupt */ \
-uint32_t smi_cmd; /* Port address of SMI command port */ \
-uint8_t acpi_enable; /* Value to write to smi_cmd to enable ACPI */ \
-uint8_t acpi_disable; /* Value to write to smi_cmd to disable ACPI */ \
-/* Value to write to SMI CMD to enter S4BIOS state */ \
-uint8_t S4bios_req; \
-uint8_t reserved2;/* Reserved - must be zero */ \
-/* Port address of Power Mgt 1a acpi_event Reg Blk */ \
-uint32_t pm1a_evt_blk; \
-/* Port address of Power Mgt 1b acpi_event Reg Blk */ \
-uint32_t pm1b_evt_blk; \
-uint32_t pm1a_cnt_blk; /* Port address of Power Mgt 1a Control Reg Blk */ \
-uint32_t pm1b_cnt_blk; /* Port address of Power Mgt 1b Control Reg Blk */ \
-uint32_t pm2_cnt_blk; /* Port address of Power Mgt 2 Control Reg Blk */ \
-uint32_t pm_tmr_blk; /* Port address of Power Mgt Timer Ctrl Reg Blk */ \
-/* Port addr of General Purpose acpi_event 0 Reg Blk */ \
-uint32_t gpe0_blk; \
-/* Port addr of General Purpose acpi_event 1 Reg Blk */ \
-uint32_t gpe1_blk; \
-uint8_t pm1_evt_len; /* Byte length of ports at pm1_x_evt_blk */ \
-uint8_t pm1_cnt_len; /* Byte length of ports at pm1_x_cnt_blk */ \
-uint8_t pm2_cnt_len; /* Byte Length of ports at pm2_cnt_blk */ \
-uint8_t pm_tmr_len; /* Byte Length of ports at pm_tm_blk */ \
-uint8_t gpe0_blk_len; /* Byte Length of ports at gpe0_blk */ \
-uint8_t gpe1_blk_len; /* Byte Length of ports at gpe1_blk */ \
-uint8_t gpe1_base;/* Offset in gpe model where gpe1 events start */ \
-uint8_t reserved3;/* Reserved */ \
-uint16_t plvl2_lat;/* Worst case HW latency to enter/exit C2 state */ \
-uint16_t plvl3_lat;/* Worst case HW latency to enter/exit C3 state */ \
-uint16_t flush_size; /* Size of area read to flush caches */ \
-uint16_t flush_stride; /* Stride used in flushing caches */ \
-uint8_t duty_offset; /* Bit location of duty cycle field in p_cnt reg */
\
-uint8_t duty_width; /* Bit width of duty cycle field in p_cnt reg */ \
-uint8_t day_alrm; /* Index to day-of-month alarm in RTC CMOS RAM */ \
-uint8_t mon_alrm; /* Index to month-of-year alarm in RTC CMOS RAM */ \
-uint8_t century; /* Index to century in RTC CMOS RAM */ \
-/* IA-PC Boot Architecture Flags (see below for individual flags) */ \
-uint16_t boot_flags; \
-uint8_t reserved;/* Reserved, must be zero */ \
-/* Miscellaneous flag bits (see below for individual flags) */ \
-uint32_t flags; \
-/* 64-bit address of the Reset register */ \
-struct AcpiGenericAddress reset_register; \
-/* Value to write to the reset_register port to reset the system */ \
-uint8_t reset_value; \
-/* ARM-Specific Boot Flags (see below for individual flags) (ACPI 5.1) */ \
-uint16_t arm_boot_flags; \
-uint8_t minor_revision; /* FADT Minor Revision (ACPI 5.1) */ \
-uint64_t x_facs; /* 64-bit physical address of FACS */ \
-uint64_t x_dsdt; /* 64-bit physical address of DSDT */ \
-/* 64-bit Extended Power Mgt 1a Event Reg Blk address */ \
-struct AcpiGenericAddress xpm1a_event_block; \
-/* 64-bit Extended Power Mgt 1b Event Reg Blk address */ \
-struct AcpiGenericAddress xpm1b_event_block; \
-/* 64-bit Extended Power Mgt 1a Control Reg Blk address */ \
-struct AcpiGenericAddress xpm1a_control_block; \
-/* 64-bit Extended Power Mgt 1b Control Reg Blk address */ \
-struct AcpiGenericAddress xpm1b_control_block; \
-/* 64-bit Extended Power Mgt 2 Control Reg Blk address */ \
-struct AcpiGenericAddress xpm2_control_block; \
-/* 64-bit Extended Power Mgt Timer Ctrl Reg Bl
Re: [Qemu-devel] [PATCH] virtio-gpu-3d: add support for second capability set (v2)
Hi, This series failed docker-quick@centos6 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. Type: series Message-id: 20180221015035.22964-1-airl...@gmail.com Subject: [Qemu-devel] [PATCH] virtio-gpu-3d: add support for second capability set (v2) === TEST SCRIPT BEGIN === #!/bin/bash set -e git submodule update --init dtc # Let docker tests dump environment info export SHOW_ENV=1 export J=8 time make docker-test-quick@centos6 === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 37ddbfb9f7 virtio-gpu-3d: add support for second capability set (v2) === OUTPUT BEGIN === Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-bpg1ky9u/src/dtc'... Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42' BUILD centos6 make[1]: Entering directory '/var/tmp/patchew-tester-tmp-bpg1ky9u/src' GEN /var/tmp/patchew-tester-tmp-bpg1ky9u/src/docker-src.2018-02-22-07.57.36.17939/qemu.tar Cloning into '/var/tmp/patchew-tester-tmp-bpg1ky9u/src/docker-src.2018-02-22-07.57.36.17939/qemu.tar.vroot'... done. Your branch is up-to-date with 'origin/test'. Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-bpg1ky9u/src/docker-src.2018-02-22-07.57.36.17939/qemu.tar.vroot/dtc'... Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42' Submodule 'ui/keycodemapdb' (git://git.qemu.org/keycodemapdb.git) registered for path 'ui/keycodemapdb' Cloning into '/var/tmp/patchew-tester-tmp-bpg1ky9u/src/docker-src.2018-02-22-07.57.36.17939/qemu.tar.vroot/ui/keycodemapdb'... Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce' COPYRUNNER RUN test-quick in qemu:centos6 Packages installed: SDL-devel-1.2.14-7.el6_7.1.x86_64 bison-2.4.1-5.el6.x86_64 bzip2-devel-1.0.5-7.el6_0.x86_64 ccache-3.1.6-2.el6.x86_64 csnappy-devel-0-6.20150729gitd7bc683.el6.x86_64 flex-2.5.35-9.el6.x86_64 gcc-4.4.7-18.el6.x86_64 gettext-0.17-18.el6.x86_64 git-1.7.1-9.el6_9.x86_64 glib2-devel-2.28.8-9.el6.x86_64 libepoxy-devel-1.2-3.el6.x86_64 libfdt-devel-1.4.0-1.el6.x86_64 librdmacm-devel-1.0.21-0.el6.x86_64 lzo-devel-2.03-3.1.el6_5.1.x86_64 make-3.81-23.el6.x86_64 mesa-libEGL-devel-11.0.7-4.el6.x86_64 mesa-libgbm-devel-11.0.7-4.el6.x86_64 package g++ is not installed pixman-devel-0.32.8-1.el6.x86_64 spice-glib-devel-0.26-8.el6.x86_64 spice-server-devel-0.12.4-16.el6.x86_64 tar-1.23-15.el6_8.x86_64 vte-devel-0.25.1-9.el6.x86_64 xen-devel-4.6.6-2.el6.x86_64 zlib-devel-1.2.3-29.el6.x86_64 Environment variables: PACKAGES=bison bzip2-devel ccache csnappy-devel flex g++ gcc gettext git glib2-devel libepoxy-devel libfdt-devel librdmacm-devel lzo-devel make mesa-libEGL-devel mesa-libgbm-devel pixman-devel SDL-devel spice-glib-devel spice-server-devel tar vte-devel xen-devel zlib-devel HOSTNAME=e4c50c4fd574 MAKEFLAGS= -j8 J=8 CCACHE_DIR=/var/tmp/ccache EXTRA_CONFIGURE_OPTS= V= SHOW_ENV=1 PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin PWD=/ TARGET_LIST= SHLVL=1 HOME=/root TEST_DIR=/tmp/qemu-test FEATURES= dtc DEBUG= _=/usr/bin/env Configure options: --enable-werror --target-list=x86_64-softmmu,aarch64-softmmu --prefix=/tmp/qemu-test/install No C++ compiler available; disabling C++ specific optional code Install prefix/tmp/qemu-test/install BIOS directory/tmp/qemu-test/install/share/qemu firmware path /tmp/qemu-test/install/share/qemu-firmware binary directory /tmp/qemu-test/install/bin library directory /tmp/qemu-test/install/lib module directory /tmp/qemu-test/install/lib/qemu libexec directory /tmp/qemu-test/install/libexec include directory /tmp/qemu-test/install/include config directory /tmp/qemu-test/install/etc local state directory /tmp/qemu-test/install/var Manual directory /tmp/qemu-test/install/share/man ELF interp prefix /usr/gnemul/qemu-%M Source path /tmp/qemu-test/src GIT binarygit GIT submodules C compilercc Host C compiler cc C++ compiler Objective-C compiler cc ARFLAGS rv CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g QEMU_CFLAGS -I/usr/include/pixman-1 -I$(SRC_PATH)/dtc/libfdt -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -DNCURSES_WIDECHAR -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration -Wold-style-defini
Re: [Qemu-devel] [PATCH v4 08/11] sdcard: display command name when tracing CMD/ACMD
On 15 February 2018 at 22:05, Philippe Mathieu-Daudé wrote:
> put the function in sdmmc-common.c since we will reuse it in hw/sd/core.c
>
As a general note, your commit messages are generally a bit
shorter than I'd prefer. This is to some extent a personal
style question, but if you're writing lots of patches where
the only commit message is the subject-line summary then I
think that it's worth being a bit more descriptive.
Also, the "body" part of the commit message should really
be able to stand alone as a description, rather than relying
on the subject line to make sense.
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/sd/sdmmc-internal.h | 3 +++
> hw/sd/sd.c | 13 +
> hw/sd/sdmmc-common.c | 72
> ++
> hw/sd/Makefile.objs| 2 +-
> hw/sd/trace-events | 8 +++---
> 5 files changed, 88 insertions(+), 10 deletions(-)
> create mode 100644 hw/sd/sdmmc-common.c
>
> diff --git a/hw/sd/sdmmc-internal.h b/hw/sd/sdmmc-internal.h
> index 0e96cb0081..02b730089b 100644
> --- a/hw/sd/sdmmc-internal.h
> +++ b/hw/sd/sdmmc-internal.h
> @@ -12,4 +12,7 @@
>
> #define SDMMC_CMD_MAX 64
>
> +const char *sd_cmd_name(uint8_t cmd);
> +const char *sd_acmd_name(uint8_t cmd);
Can we have doc comments for new function prototypes in header files,
please?
> diff --git a/hw/sd/sdmmc-common.c b/hw/sd/sdmmc-common.c
> new file mode 100644
> index 00..1d0198b1ad
> --- /dev/null
> +++ b/hw/sd/sdmmc-common.c
> @@ -0,0 +1,72 @@
> +/*
> + * SD/MMC cards common helpers
> + *
> + * Copyright (c) 2018 Philippe Mathieu-Daudé
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + * SPDX-License-Identifier: GPL-2.0-or-later
Ooh, a SPDX identifier. The project doesn't actually use these
at the moment, but it doesn't hurt to have one here I guess.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "sdmmc-internal.h"
> +
> +const char *sd_cmd_name(uint8_t cmd)
> +{
> +static const char *cmd_abbrev[SDMMC_CMD_MAX] = {
> + [0]= "GO_IDLE_STATE",
> + [2]= "ALL_SEND_CID",[3]= "SEND_RELATIVE_ADDR",
> + [4]= "SET_DSR", [5]= "IO_SEND_OP_COND",
> + [6]= "SWITCH_FUNC", [7]= "SELECT/DESELECT_CARD",
> + [8]= "SEND_IF_COND",[9]= "SEND_CSD",
> +[10]= "SEND_CID", [11]= "VOLTAGE_SWITCH",
> +[12]= "STOP_TRANSMISSION", [13]= "SEND_STATUS",
> +[15]= "GO_INACTIVE_STATE",
> +[16]= "SET_BLOCKLEN", [17]= "READ_SINGLE_BLOCK",
> +[18]= "READ_MULTIPLE_BLOCK",[19]= "SEND_TUNING_BLOCK",
> +[20]= "SPEED_CLASS_CONTROL",[21]= "DPS_spec",
> +[23]= "SET_BLOCK_COUNT",
> +[24]= "WRITE_BLOCK",[25]= "WRITE_MULTIPLE_BLOCK",
> +[26]= "MANUF_RSVD", [27]= "PROGRAM_CSD",
> +[28]= "SET_WRITE_PROT", [29]= "CLR_WRITE_PROT",
> +[30]= "SEND_WRITE_PROT",
> +[32]= "ERASE_WR_BLK_START", [33]= "ERASE_WR_BLK_END",
> +[34]= "SW_FUNC_RSVD", [35]= "SW_FUNC_RSVD",
> +[36]= "SW_FUNC_RSVD", [37]= "SW_FUNC_RSVD",
> +[38]= "ERASE",
> +[40]= "DPS_spec",
> +[42]= "LOCK_UNLOCK",[43]= "Q_MANAGEMENT",
> +[44]= "Q_TASK_INFO_A", [45]= "Q_TASK_INFO_B",
> +[46]= "Q_RD_TASK", [47]= "Q_WR_TASK",
> +[48]= "READ_EXTR_SINGLE", [49]= "WRITE_EXTR_SINGLE",
> +[50]= "SW_FUNC_RSVD", /* FIXME */
What's this FIXME for? We should either fix whatever it is, or if
that's not practical the comment needs to describe the problem
so that a future reader of the code knows what it means and might
be able to fix it.
> +[52]= "IO_RW_DIRECT", [53]= "IO_RW_EXTENDED",
> +[54]= "SDIO_RSVD", [55]= "APP_CMD",
> +[56]= "GEN_CMD",[57]= "SW_FUNC_RSVD",
> +[58]= "READ_EXTR_MULTI",[59]= "WRITE_EXTR_MULTI",
> +[60]= "MANUF_RSVD", [61]= "MANUF_RSVD",
> +[62]= "MANUF_RSVD", [63]= "MANUF_RSVD",
> +};
> +return cmd_abbrev[cmd] ? cmd_abbrev[cmd] : "UNKNOWN_CMD";
> +}
> +
> +const char *sd_acmd_name(uint8_t cmd)
> +{
> +static const char *acmd_abbrev[SDMMC_CMD_MAX] = {
> + [6] = "SET_BUS_WIDTH",
> +[13] = "SD_STATUS",
> +[14] = "DPS_spec", [15] = "DPS_spec",
> +[16] = "DPS_spec",
> +[18] = "SECU_spec",
> +[22] = "SEND_NUM_WR_BLOCKS",[23] = "SET_WR_BLK_ERASE_COUNT",
> +[41]
Re: [Qemu-devel] [PATCH v4 09/11] sdcard: display protocol used when tracing
On 15 February 2018 at 22:05, Philippe Mathieu-Daudé wrote: > put the function in sdmmc-common.c since we will reuse it in hw/sd/core.c > > Signed-off-by: Philippe Mathieu-Daudé Commit message talks about a function but the patch doesn't add any new functions ? (Also, sentences should start with a capital letter and end with a full stop, please.) Otherwise Reviewed-by: Peter Maydell thanks -- PMM
Re: [Qemu-devel] [PATCH] docs: document how to use the l2-cache-entry-size parameter
On Wed 21 Feb 2018 07:33:55 PM CET, Kevin Wolf wrote: [docs/qcow2-cache.txt] > While reviewing this, I read the whole document and stumbled across > these paragraphs: > >> The reason for this 1/4 ratio is to ensure that both caches cover the >> same amount of disk space. Note however that this is only valid with >> the default value of refcount_bits (16). If you are using a different >> value you might want to calculate both cache sizes yourself since >> QEMU will always use the same 1/4 ratio. > > Sounds like we should fix our defaults? We could do that, yes, we would be "breaking" compatibility with previous versions, but we're not talking about semantic changes here, so it's not a big deal in and on itself. Of course this would be a problem if the new defaults make things work slower, but I don't think that's the case here. Just to confirm: do you suggest reducing the refcount cache size (because it's not so necessary) or changing the formula so the number of refcount_bits is taken into account so that both caches cover the same amount of disk space in all cases? I suppose it's the former based on what you write below. > While we're at it, would l2-cache-entry-size = MIN(cluster_size, 64k) > make sense as a default? Any reason why you choose 64k, or is it because it's the default cluster size? In general I'd be cautious to reduce the default entry size unconditionally because with rotating HDDs it may even have a negative (although slight) effect on performance. But the larger the cluster, the larger the area mapped by L2 entries, so we need less metadata and it makes more sense to read less in general. In summary: while I think it's probably a good idea, it would be good to make some tests before changing the default. >> It's also worth mentioning that there's no strict need for both >> caches to cover the same amount of disk space. The refcount cache is >> used much less often than the L2 cache, so it's perfectly reasonable >> to keep it small. > > More precisely, it is only used for cluster allocation, not for read > or for rewrites. Usually this means that it's indeed accessed a lot > less, though especially in benchmarks, this isn't necessarily less > often. > > However, the more important part is that even for allocating writes > with random I/O, the refcount cache is still accessed sequentially and > we don't really take advantage of having more than a single refcount > block in memory. This only stops being true as soon as you add > something that can free clusters (discards, overwriting compressed > cluster, deleting internal snapshots). > > We have a minimum refcount block cache size of 4 clusters because of > the possible recursion during refcount table growth, which leaves some > room to hold the refcount block for an occasional discard (and > subsequent reallocation). > > So should we default to this minimum on the grounds that for most > people, refcounts blocks are probably only accessed sequentially in > practice? The remaining memory of the total cache size seems to help > the average case more if it's added to the L2 cache instead. That sounds like a good idea. We should double check that the minimum is indeed the required minimum, and run some tests, but otherwise I'm all for it. I think I can take care of this if you want. Berto
Re: [Qemu-devel] [PATCH v4 00/11] SDCard: housekeeping, add tracing (part 4)
On 15 February 2018 at 22:05, Philippe Mathieu-Daudé wrote: > Since v3: > - use assert() in sd_state_name() and sd_response_name() (Alistair review) > - added sdmmc-internal.h & sdmmc-common.c to reuse helpers with hw/sd/core.c > > Since v2: > - split again in 2... this part is cleanup/tracing > - add more tracepoints > - move some code reusable by sdbus in sdmmc-internal.h > > Since v1: > - rewrote mostly all patches to keep it simpler. > > $ git backport-diff > 001/11:[] [--] 'sdcard: reorder SDState struct members' > 002/11:[0003] [FC] 'sdcard: replace DPRINTF() by trace events' > 003/11:[0001] [FC] 'sdcard: add a trace event for command responses' > 004/11:[0007] [FC] 'sdcard: replace fprintf() by qemu_hexdump()' > 005/11:[] [--] 'sdcard: add more trace events' > 006/11:[] [--] 'sdcard: do not trace CMD55 when expecting ACMD' > 007/11:[0014] [FC] 'sdcard: define SDMMC_CMD_MAX instead of using the magic > '64'' > 008/11:[0020] [FC] 'sdcard: display command name when tracing CMD/ACMD' > 009/11:[] [--] 'sdcard: display protocol used when tracing' > 010/11:[] [-C] 'sdcard: use G_BYTE from cutils' > 011/11:[] [-C] 'sdcard: use the registerfields API to access the OCR > register' Hi. I've applied patches 1-5, 7, 10 and 11 to target-arm.next. The rest I've left you review comments for. -- PMM
Re: [Qemu-devel] [PATCH v8 03/26] block: Add BDS.backing_overridden
Am 05.02.2018 um 16:18 hat Max Reitz geschrieben: > If the backing file is overridden, this most probably does change the > guest-visible data of a BDS. Therefore, we will need to consider this in > bdrv_refresh_filename(). > > Adding a new field to the BDS is not nice, but it is very simple and > exactly keeps track of whether the backing file has been overridden. ...as long as we manage to actually keep it up to date all the time. First of all, what I'm missing here (or in fact in the comment in the code) is a definition what "overridden" really means. "specified by the user" is kind of vague: You consider the backing file relationship for snapshot=on as user specified, even though the user wasn't explicit about this. On the other hand, creating a live snapshot results in a node that isn't user specified. Isn't the real question to ask whether the default backing file (taken from the image header) would result in the same tree? The answer to this changes after more operations, like qmp_change_backing_file(). Considering that there are so many ways to change the answer, I think the simplest reliable option isn't a new BDS field that needs to updated everywhere, but looking at the current value of bs->options and bs->backing_file and see if they match. > This commit adds a FIXME which will be remedied by a follow-up commit. > Until then, the respective piece of code will not result in any behavior > that is worse than what we currently have. > > Signed-off-by: Max Reitz > Reviewed-by: Eric Blake > Reviewed-by: Alberto Garcia Kevin
Re: [Qemu-devel] [PATCH v3] qcow2: Replace align_offset() with ROUND_UP()
ping
On Thu 15 Feb 2018 02:10:08 PM CET, Alberto Garcia wrote:
> The align_offset() function is equivalent to the ROUND_UP() macro so
> there's no need to use the former. The ROUND_UP() name is also a bit
> more explicit.
>
> This patch uses ROUND_UP() instead of the slower QEMU_ALIGN_UP()
> because align_offset() already requires that the second parameter is a
> power of two.
>
> Signed-off-by: Alberto Garcia
> Reviewed-by: Eric Blake
> Reviewed-by: Philippe Mathieu-Daudé
> ---
> v3 is the same as v2, but rebased on top of the current master fixing
> a merge conflict.
> ---
> block/qcow2-bitmap.c | 4 ++--
> block/qcow2-cluster.c | 4 ++--
> block/qcow2-refcount.c | 4 ++--
> block/qcow2-snapshot.c | 10 +-
> block/qcow2.c | 14 +++---
> block/qcow2.h | 6 --
> 6 files changed, 18 insertions(+), 24 deletions(-)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 4f6fd863ea..5127276f90 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -413,8 +413,8 @@ static inline void
> bitmap_dir_entry_to_be(Qcow2BitmapDirEntry *entry)
>
> static inline int calc_dir_entry_size(size_t name_size, size_t
> extra_data_size)
> {
> -return align_offset(sizeof(Qcow2BitmapDirEntry) +
> -name_size + extra_data_size, 8);
> +int size = sizeof(Qcow2BitmapDirEntry) + name_size + extra_data_size;
> +return ROUND_UP(size, 8);
> }
>
> static inline int dir_entry_size(Qcow2BitmapDirEntry *entry)
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index e406b0f3b9..98908c4264 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -126,11 +126,11 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t
> min_size,
>
> new_l1_size2 = sizeof(uint64_t) * new_l1_size;
> new_l1_table = qemu_try_blockalign(bs->file->bs,
> - align_offset(new_l1_size2, 512));
> + ROUND_UP(new_l1_size2, 512));
> if (new_l1_table == NULL) {
> return -ENOMEM;
> }
> -memset(new_l1_table, 0, align_offset(new_l1_size2, 512));
> +memset(new_l1_table, 0, ROUND_UP(new_l1_size2, 512));
>
> if (s->l1_size) {
> memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index d46b69d7f3..126cca3276 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1204,7 +1204,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
> * l1_table_offset when it is the current s->l1_table_offset! Be careful
> * when changing this! */
> if (l1_table_offset != s->l1_table_offset) {
> -l1_table = g_try_malloc0(align_offset(l1_size2, 512));
> +l1_table = g_try_malloc0(ROUND_UP(l1_size2, 512));
> if (l1_size2 && l1_table == NULL) {
> ret = -ENOMEM;
> goto fail;
> @@ -2553,7 +2553,7 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs,
> int ign, int64_t offset,
> }
>
> /* align range to test to cluster boundaries */
> -size = align_offset(offset_into_cluster(s, offset) + size,
> s->cluster_size);
> +size = ROUND_UP(offset_into_cluster(s, offset) + size, s->cluster_size);
> offset = start_of_cluster(s, offset);
>
> if ((chk & QCOW2_OL_ACTIVE_L1) && s->l1_size) {
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 44243e0e95..cee25f582b 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -66,7 +66,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
>
> for(i = 0; i < s->nb_snapshots; i++) {
> /* Read statically sized part of the snapshot header */
> -offset = align_offset(offset, 8);
> +offset = ROUND_UP(offset, 8);
> ret = bdrv_pread(bs->file, offset, &h, sizeof(h));
> if (ret < 0) {
> goto fail;
> @@ -155,7 +155,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
> offset = 0;
> for(i = 0; i < s->nb_snapshots; i++) {
> sn = s->snapshots + i;
> -offset = align_offset(offset, 8);
> +offset = ROUND_UP(offset, 8);
> offset += sizeof(h);
> offset += sizeof(extra);
> offset += strlen(sn->id_str);
> @@ -215,7 +215,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
> assert(id_str_size <= UINT16_MAX && name_size <= UINT16_MAX);
> h.id_str_size = cpu_to_be16(id_str_size);
> h.name_size = cpu_to_be16(name_size);
> -offset = align_offset(offset, 8);
> +offset = ROUND_UP(offset, 8);
>
> ret = bdrv_pwrite(bs->file, offset, &h, sizeof(h));
> if (ret < 0) {
> @@ -441,7 +441,7 @@ int qcow2_snapshot_create(BlockDriverState *bs,
> QEMUSnapshotInfo *sn_info)
> /* The VM state isn't needed any more in the active L1 table; in fact, it
> * hurts by causing expensive COW for t
Re: [Qemu-devel] [PULL 19/20] tcg/i386: Add vector operations
On 2018-02-21 21:22, Richard Henderson wrote: > On 02/20/2018 08:44 AM, Max Reitz wrote: >> This patch makes make check with clang -m32 fail for me. (Interestingly >> enough, though, clang -m64 and gcc -m32 work fine.) > > What's really interesting is that gcc -m32 should *not* have worked, but does. > I'm not sure why. The assert is for the missing constraints for > INDEX_op_dup2_vec. > > Will fix. Thanks! Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 2/2] qcow2: Avoid memory over-allocation on compressed images
On Wed 21 Feb 2018 05:59:58 PM CET, Eric Blake wrote: > But as Berto has convinced me that an externally produced image can > convince us to read up to 4M (even though we don't need that much to > decompress), A (harmless but funny) consequence of the way this works is that for any valid compressed cluster you should be able to increase the value of the size field as much as you want without causing any user-visible effect. So if you're working with 2MB clusters but for a particular compressed cluster the size field is 0x0006 (7 sectors) you can still increase it to the maximum (0x1fff, or 8192 sectors) and it should work just the same. QEMU will read 4MB instead of ~4KB but since decompression stops once the original cluster has been restored there's no harm. I think I'll write a test case for this, it can be useful to verify that QEMU can handle this kind of scenarios. Berto
Re: [Qemu-devel] [PATCH 0/4] target/mips: translator loop conversion
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 1518746572-14747-1-git-send-email-c...@braap.org Subject: [Qemu-devel] [PATCH 0/4] target/mips: translator loop conversion === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/1519148406-15006-1-git-send-email-th...@redhat.com -> patchew/1519148406-15006-1-git-send-email-th...@redhat.com Switched to a new branch 'test' 655561a1db target/mips: convert to TranslatorOps 46e359caf8 target/mips: use *ctx for DisasContext 55256d7088 target/mips: convert to DisasContextBase ebad4470fe target/mips: convert to DisasJumpType === OUTPUT BEGIN === Checking PATCH 1/4: target/mips: convert to DisasJumpType... Checking PATCH 2/4: target/mips: convert to DisasContextBase... Checking PATCH 3/4: target/mips: use *ctx for DisasContext... ERROR: space prohibited after that '&' (ctx:WxW) #76: FILE: target/mips/translate.c:20220: +ctx->kscrexist = (env->CP0_Config4 >> CP0C4_KScrExist) & 0xff; ^ total: 1 errors, 0 warnings, 254 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 4/4: target/mips: convert to TranslatorOps... === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-de...@freelists.org
Re: [Qemu-devel] [PATCH 00/11] macio: remove legacy macio_init() function
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20180219181922.21586-1-mark.cave-ayl...@ilande.co.uk Subject: [Qemu-devel] [PATCH 00/11] macio: remove legacy macio_init() function === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20180215212552.26997-1-marcandre.lur...@redhat.com -> patchew/20180215212552.26997-1-marcandre.lur...@redhat.com Switched to a new branch 'test' b977ecc86a macio: remove macio_init() function 61db23bf85 macio: move setting of CUDA timebase frequency to macio_common_realize() c69bff3e41 mac_newworld: use object link to pass OpenPIC object to macio 0c0d0b3899 openpic: move OpenPIC state and related definitions to openpic.h 9e564cc429 mac_oldworld: use object link to pass heathrow PIC object to macio effea2820a macio: move macio related structures and defines into separate macio.h file 97c70c2c84 heathrow: change heathrow_pic_init() to return the heathrow device 71e75ff80c heathrow: convert to trace-events 408994eab6 heathrow: QOMify heathrow PIC 112e5b64e5 macio: move ESCC device within the macio device 076292acb3 macio: embed DBDMA device directly within macio === OUTPUT BEGIN === Checking PATCH 1/11: macio: embed DBDMA device directly within macio... Checking PATCH 2/11: macio: move ESCC device within the macio device... Checking PATCH 3/11: heathrow: QOMify heathrow PIC... Checking PATCH 4/11: heathrow: convert to trace-events... Checking PATCH 5/11: heathrow: change heathrow_pic_init() to return the heathrow device... Checking PATCH 6/11: macio: move macio related structures and defines into separate macio.h file... Checking PATCH 7/11: mac_oldworld: use object link to pass heathrow PIC object to macio... Checking PATCH 8/11: openpic: move OpenPIC state and related definitions to openpic.h... ERROR: "foo * bar" should be "foo *bar" #249: FILE: include/hw/ppc/openpic.h:57: +#define RAVEN_DBL_IRQ(RAVEN_IPI_IRQ + (RAVEN_MAX_CPU * RAVEN_MAX_IPI)) total: 1 errors, 0 warnings, 356 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 9/11: mac_newworld: use object link to pass OpenPIC object to macio... Checking PATCH 10/11: macio: move setting of CUDA timebase frequency to macio_common_realize()... Checking PATCH 11/11: macio: remove macio_init() function... === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-de...@freelists.org
Re: [Qemu-devel] [RFC PATCH 0/5] ui: add keyboard state and hotkey tracker
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20180221170820.15365-1-kra...@redhat.com Subject: [Qemu-devel] [RFC PATCH 0/5] ui: add keyboard state and hotkey tracker === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20180220225904.16129-1-...@redhat.com -> patchew/20180220225904.16129-1-...@redhat.com * [new tag] patchew/cover.1519036465.git.bala...@eik.bme.hu -> patchew/cover.1519036465.git.bala...@eik.bme.hu Switched to a new branch 'test' 9065fbc367 sdl2: use only QKeyCode in sdl2_process_key() 0c65c8c6bb kbd-state: register sdl2 hotkeys 67a2d8cd4a kbd-state: use state tracker for sdl2 a389e7e994 kbd-state: add hotkey registry d14a0b4fd8 kbd-state: add keyboard state tracker === OUTPUT BEGIN === Checking PATCH 1/5: kbd-state: add keyboard state tracker... ERROR: space required after that ',' (ctx:BxV) #86: FILE: ui/kbd-state.c:22: +QTAILQ_HEAD(,KbdHotkey) hotkeys; ^ total: 1 errors, 0 warnings, 149 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 2/5: kbd-state: add hotkey registry... Checking PATCH 3/5: kbd-state: use state tracker for sdl2... Checking PATCH 4/5: kbd-state: register sdl2 hotkeys... Checking PATCH 5/5: sdl2: use only QKeyCode in sdl2_process_key()... === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-de...@freelists.org
Re: [Qemu-devel] [PATCHv2 00/11] macio: remove legacy macio_init() function
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20180220184159.16483-1-mark.cave-ayl...@ilande.co.uk Subject: [Qemu-devel] [PATCHv2 00/11] macio: remove legacy macio_init() function === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20180221131537.31341-1-kra...@redhat.com -> patchew/20180221131537.31341-1-kra...@redhat.com Switched to a new branch 'test' d5e29e540b macio: remove macio_init() function d511fd4422 macio: move setting of CUDA timebase frequency to macio_common_realize() 4fb3902f19 mac_newworld: use object link to pass OpenPIC object to macio e67d1d3a8c openpic: move OpenPIC state and related definitions to openpic.h d3de622d69 mac_oldworld: use object link to pass heathrow PIC object to macio d43db08bb0 macio: move macio related structures and defines into separate macio.h file 4013ef1580 heathrow: change heathrow_pic_init() to return the heathrow device 7935a059a9 heathrow: convert to trace-events 72083726f5 heathrow: QOMify heathrow PIC bea15ddd19 macio: move ESCC device within the macio device 2c121e401b macio: embed DBDMA device directly within macio === OUTPUT BEGIN === Checking PATCH 1/11: macio: embed DBDMA device directly within macio... Checking PATCH 2/11: macio: move ESCC device within the macio device... Checking PATCH 3/11: heathrow: QOMify heathrow PIC... Checking PATCH 4/11: heathrow: convert to trace-events... Checking PATCH 5/11: heathrow: change heathrow_pic_init() to return the heathrow device... Checking PATCH 6/11: macio: move macio related structures and defines into separate macio.h file... Checking PATCH 7/11: mac_oldworld: use object link to pass heathrow PIC object to macio... Checking PATCH 8/11: openpic: move OpenPIC state and related definitions to openpic.h... ERROR: "foo * bar" should be "foo *bar" #248: FILE: include/hw/ppc/openpic.h:57: +#define RAVEN_DBL_IRQ(RAVEN_IPI_IRQ + (RAVEN_MAX_CPU * RAVEN_MAX_IPI)) total: 1 errors, 0 warnings, 356 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 9/11: mac_newworld: use object link to pass OpenPIC object to macio... Checking PATCH 10/11: macio: move setting of CUDA timebase frequency to macio_common_realize()... Checking PATCH 11/11: macio: remove macio_init() function... === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-de...@freelists.org
Re: [Qemu-devel] [PATCH v2 2/3] qcow2: Don't allow overflow during cluster allocation
On 02/22/2018 04:29 AM, Alberto Garcia wrote:
On Thu 22 Feb 2018 12:39:52 AM CET, Eric Blake wrote:
free_in_cluster = s->cluster_size - offset_into_cluster(s, offset);
do {
if (!offset || free_in_cluster < size) {
-int64_t new_cluster = alloc_clusters_noref(bs, s->cluster_size);
+int64_t new_cluster;
+
+new_cluster = alloc_clusters_noref(bs, s->cluster_size,
+ (1ULL << s->csize_shift) - 1);
(1ULL << s->csize_shift) - 1) is the same as s->cluster_offset_mask, but
I guess it's confusing to use that here, so your approach looks
appropriate.
Actually, s->cluster_offset_mask fits better - we want to ensure that
the allocated cluster fits within the mask! I'll adjust on respin.
Reviewed-by: Alberto Garcia
Thanks for bearing with me.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH] docs: document how to use the l2-cache-entry-size parameter
Am 22.02.2018 um 14:06 hat Alberto Garcia geschrieben: > On Wed 21 Feb 2018 07:33:55 PM CET, Kevin Wolf wrote: > > [docs/qcow2-cache.txt] > > While reviewing this, I read the whole document and stumbled across > > these paragraphs: > > > >> The reason for this 1/4 ratio is to ensure that both caches cover the > >> same amount of disk space. Note however that this is only valid with > >> the default value of refcount_bits (16). If you are using a different > >> value you might want to calculate both cache sizes yourself since > >> QEMU will always use the same 1/4 ratio. > > > > Sounds like we should fix our defaults? > > We could do that, yes, we would be "breaking" compatibility with > previous versions, but we're not talking about semantic changes here, so > it's not a big deal in and on itself. I don't think changing behaviour and breaking compatibility is the same thing. If there is an option, you should expect that its default can change in newer versions. If you want a specific setting, you should be explicit. > Of course this would be a problem if the new defaults make things work > slower, but I don't think that's the case here. > > Just to confirm: do you suggest reducing the refcount cache size > (because it's not so necessary) or changing the formula so the number of > refcount_bits is taken into account so that both caches cover the same > amount of disk space in all cases? > > I suppose it's the former based on what you write below. I was first thinking to make the ratio dynamic based on the refcount width, but then it occurred to me that a fixed absolute number probably makes even more sense. Either way should be better than the current default, though. > > While we're at it, would l2-cache-entry-size = MIN(cluster_size, 64k) > > make sense as a default? > > Any reason why you choose 64k, or is it because it's the default cluster > size? > > In general I'd be cautious to reduce the default entry size > unconditionally because with rotating HDDs it may even have a negative > (although slight) effect on performance. But the larger the cluster, the > larger the area mapped by L2 entries, so we need less metadata and it > makes more sense to read less in general. > > In summary: while I think it's probably a good idea, it would be good to > make some tests before changing the default. The exact value of 64k is more or less because it's the default cluster size, yes. Not changing anything for the default cluster size makes it a bit easier to justify. What I really want to fix is the 2 MB entry size with the maximum cluster size, because that's just unreasonably large. It's not completely clear what the ideal size is, but when we increased the default cluster size from 4k, the optimal values (on rotating hard disks back then) were 64k or 128k. So I assume that it's somewhere around these sizes that unnecessary I/O starts to hurt more than reading one big chunk instead of two smaller ones helps. Of course, guest I/O a few years ago and metadata I/O today aren't exactly the same thing, so I agree that a change should be measured first. But 64k-128k feels right as an educated guess where to start. > >> It's also worth mentioning that there's no strict need for both > >> caches to cover the same amount of disk space. The refcount cache is > >> used much less often than the L2 cache, so it's perfectly reasonable > >> to keep it small. > > > > More precisely, it is only used for cluster allocation, not for read > > or for rewrites. Usually this means that it's indeed accessed a lot > > less, though especially in benchmarks, this isn't necessarily less > > often. > > > > However, the more important part is that even for allocating writes > > with random I/O, the refcount cache is still accessed sequentially and > > we don't really take advantage of having more than a single refcount > > block in memory. This only stops being true as soon as you add > > something that can free clusters (discards, overwriting compressed > > cluster, deleting internal snapshots). > > > > We have a minimum refcount block cache size of 4 clusters because of > > the possible recursion during refcount table growth, which leaves some > > room to hold the refcount block for an occasional discard (and > > subsequent reallocation). > > > > So should we default to this minimum on the grounds that for most > > people, refcounts blocks are probably only accessed sequentially in > > practice? The remaining memory of the total cache size seems to help > > the average case more if it's added to the L2 cache instead. > > That sounds like a good idea. We should double check that the minimum is > indeed the required minimum, and run some tests, but otherwise I'm all > for it. > > I think I can take care of this if you want. That would be good. I'm pretty confident that the minimum is enough because it's also the default for 64k clusters. Maybe it's too high if I miscalculated back then, but I haven't seen a crash report r
Re: [Qemu-devel] [PATCH v2 3/3] qcow2: Avoid memory over-allocation on compressed images
On 02/22/2018 04:50 AM, Alberto Garcia wrote:
On Thu 22 Feb 2018 12:39:53 AM CET, Eric Blake wrote:
+assert(!!s->cluster_data == !!s->cluster_cache);
+assert(csize < 2 * s->cluster_size + 512);
if (!s->cluster_data) {
-/* one more sector for decompressed data alignment */
-s->cluster_data = qemu_try_blockalign(bs->file->bs,
-QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size + 512);
+s->cluster_data = g_try_malloc(2 * s->cluster_size + 512);
if (!s->cluster_data) {
return -ENOMEM;
}
Why the "+ 512" ?
I was thinking "number of sectors is up to two clusters, and we add one,
PLUS we must read from the initial sector containing the offset". But I
was obviously not careful enough - the maximum value (all 1s) is 512
bytes short of 2 full clusters, and we add at most 511 more bytes for
the initial sector containing the offset (our +1 covers the leading
sector). So you are right, I can tighten this down for a slightly
smaller allocation (and a nice power-of-2 allocation may be slightly
more efficient as well). v3 coming up.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v4 17/20] sdcard: add SD SEND_TUNING_BLOCK (CMD19)
On 15 February 2018 at 22:13, Philippe Mathieu-Daudé wrote:
> [based on a patch from Alistair Francis
> from qemu/xilinx tag xilinx-v2015.2]
> Signed-off-by: Philippe Mathieu-Daudé
> Reviewed-by: Alistair Francis
> ---
> hw/sd/sd.c | 24
> 1 file changed, 24 insertions(+)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 911aae6233..4b0bb7992d 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1166,6 +1166,14 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
> SDRequest req)
> }
> break;
>
> +case 19:/* CMD19: SEND_TUNING_BLOCK (SD) */
> +if (sd->state == sd_transfer_state) {
> +sd->state = sd_sendingdata_state;
> +sd->data_offset = 0;
> +return sd_r1;
> +}
> +break;
> +
> case 23:/* CMD23: SET_BLOCK_COUNT */
> switch (sd->state) {
> case sd_transfer_state:
> @@ -1889,6 +1897,15 @@ void sd_write_data(SDState *sd, uint8_t value)
> }
> }
>
> +#define SD_TUNING_BLOCK_SIZE64
> +
> +static const uint32_t sd_tunning_data[SD_TUNING_BLOCK_SIZE / 4] = {
Typo, s/tunning/tuning/.
> +0xFF0FFF00, 0x0FFCC3CC, 0xC33CCCFF, 0xFEFFFEEF,
> +0xFFDFFFDD, 0xFFFBFFFB, 0XBFFF7FFF, 0X77F7BDEF,
Can we have lowercase 'x' consistently here please?
> +0XFFF0FFF0, 0X0FFCCC3C, 0XCC33CCCF, 0XFFEFFFEE,
> +0XFFFDFFFD, 0XDFFFBFFF, 0XBBFFF7FF, 0XF77F7BDE,
> +};
A comment about what all these magic numbers are for would
be useful.
> +
> uint8_t sd_read_data(SDState *sd)
> {
> /* TODO: Append CRCs */
> @@ -1968,6 +1985,13 @@ uint8_t sd_read_data(SDState *sd)
> }
> break;
>
> +case 19:/* CMD19: SEND_TUNING_BLOCK (SD) */
> +if (sd->data_offset >= SD_TUNING_BLOCK_SIZE - 1) {
> +sd->state = sd_transfer_state;
> +}
> +ret = ((uint8_t *)(&sd_tunning_data))[sd->data_offset++];
If you're going to treat it as a uint8_t buffer it might be
cleaner to just declare it as a uint8_t buffer rather than
uint32_t: you'd get to avoid the cast here and the / 4 in
the array definition.
> +break;
> +
> case 22: /* ACMD22: SEND_NUM_WR_BLOCKS */
> ret = sd->data[sd->data_offset ++];
>
> --
> 2.16.1
>
thanks
-- PMM
