Re: [PATCH v3 1/2] hw/net: Added plen fix for IPv6
On 2020/7/16 上午11:53, and...@daynix.com wrote: From: Andrew Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1708065 With network backend with 'virtual header' - there was an issue in 'plen' field. Overall, during TSO, 'plen' would be changed, but with 'vheader' this field should be set to the size of the payload itself instead of '0'. Signed-off-by: Andrew Melnychenko Applied. Thanks --- hw/net/net_tx_pkt.c | 23 +++ hw/net/net_tx_pkt.h | 14 ++ include/net/eth.h | 1 + 3 files changed, 38 insertions(+) diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c index 331c73cfc0..9560e4a49e 100644 --- a/hw/net/net_tx_pkt.c +++ b/hw/net/net_tx_pkt.c @@ -626,6 +626,7 @@ bool net_tx_pkt_send(struct NetTxPkt *pkt, NetClientState *nc) if (pkt->has_virt_hdr || pkt->virt_hdr.gso_type == VIRTIO_NET_HDR_GSO_NONE) { +net_tx_pkt_fix_ip6_payload_len(pkt); net_tx_pkt_sendv(pkt, nc, pkt->vec, pkt->payload_frags + NET_TX_PKT_PL_START_FRAG); return true; @@ -644,3 +645,25 @@ bool net_tx_pkt_send_loopback(struct NetTxPkt *pkt, NetClientState *nc) return res; } + +void net_tx_pkt_fix_ip6_payload_len(struct NetTxPkt *pkt) +{ +struct iovec *l2 = &pkt->vec[NET_TX_PKT_L2HDR_FRAG]; +if (eth_get_l3_proto(l2, 1, l2->iov_len) == ETH_P_IPV6) { +struct ip6_header *ip6 = (struct ip6_header *) pkt->l3_hdr; +/* + * TODO: if qemu would support >64K packets - add jumbo option check + * something like that: + * 'if (ip6->ip6_plen == 0 && !has_jumbo_option(ip6)) {' + */ +if (ip6->ip6_plen == 0) { +if (pkt->payload_len <= ETH_MAX_IP_DGRAM_LEN) { +ip6->ip6_plen = htons(pkt->payload_len); +} +/* + * TODO: if qemu would support >64K packets + * add jumbo option for packets greater then 65,535 bytes + */ +} +} +} diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h index 212ecc62fc..4ec8bbe9bd 100644 --- a/hw/net/net_tx_pkt.h +++ b/hw/net/net_tx_pkt.h @@ -187,4 +187,18 @@ bool net_tx_pkt_parse(struct NetTxPkt *pkt); */ bool net_tx_pkt_has_fragments(struct NetTxPkt *pkt); +/** + * Fix IPv6 'plen' field. + * If ipv6 payload length field is 0 - then there should be Hop-by-Hop + * option for packets greater than 65,535. + * For packets with a payload less than 65,535: fix 'plen' field. + * For backends with vheader, we need just one packet with proper + * payload size. For now, qemu drops every packet with size greater 64K + * (see net_tx_pkt_send()) so, there is no reason to add jumbo option to ip6 + * hop-by-hop extension if it's missed + * + * @pktpacket + */ +void net_tx_pkt_fix_ip6_payload_len(struct NetTxPkt *pkt); + #endif diff --git a/include/net/eth.h b/include/net/eth.h index 7f45c678e7..0671be6916 100644 --- a/include/net/eth.h +++ b/include/net/eth.h @@ -186,6 +186,7 @@ struct tcp_hdr { #define ip6_nxt ip6_ctlun.ip6_un1.ip6_un1_nxt #define ip6_ecn_acc ip6_ctlun.ip6_un3.ip6_un3_ecn +#define ip6_plen ip6_ctlun.ip6_un1.ip6_un1_plen #define PKT_GET_ETH_HDR(p)\ ((struct eth_header *)(p))
[PATCH v6 13/13] tests/acceptance: console boot tests for quanta-gsj
This adds two acceptance tests for the quanta-gsj machine. One test downloads a lightly patched openbmc flash image from github and verifies that it boots all the way to the login prompt. The other test downloads a kernel, initrd and dtb built from the same openbmc source and verifies that the kernel detects all CPUs and boots to the point where it can't find the root filesystem (because we have no flash image in this case). Signed-off-by: Havard Skinnemoen --- tests/acceptance/boot_linux_console.py | 65 ++ 1 file changed, 65 insertions(+) diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py index 73cc69c499..1d82fc7ff8 100644 --- a/tests/acceptance/boot_linux_console.py +++ b/tests/acceptance/boot_linux_console.py @@ -569,6 +569,71 @@ class BootLinuxConsole(LinuxKernelTest): 'sda') # cubieboard's reboot is not functioning; omit reboot test. +def test_arm_quanta_gsj(self): +""" +:avocado: tags=arch:arm +:avocado: tags=machine:quanta-gsj +""" +# 25 MiB compressed, 32 MiB uncompressed. +image_url = ( +'https://github.com/hskinnemoen/openbmc/releases/download/' +'20200711-gsj-qemu-0/obmc-phosphor-image-gsj.static.mtd.gz') +image_hash = '14895e634923345cb5c8776037ff7876df96f6b1' +image_path_gz = self.fetch_asset(image_url, asset_hash=image_hash) +image_name = os.path.splitext(os.path.basename(image_path_gz))[0] +image_path = os.path.join(self.workdir, image_name) +archive.gzip_uncompress(image_path_gz, image_path) + +self.vm.set_console() +drive_args = 'file=' + image_path + ',if=mtd,bus=0,unit=0' +self.vm.add_args('-drive', drive_args) +self.vm.launch() + +self.wait_for_console_pattern('> BootBlock by Nuvoton') +self.wait_for_console_pattern('>Device: Poleg BMC NPCM730') +self.wait_for_console_pattern('>Skip DDR init.') +self.wait_for_console_pattern('U-Boot ') +self.wait_for_console_pattern('Booting Linux on physical CPU 0x0') +self.wait_for_console_pattern('CPU1: thread -1, cpu 1, socket 0') +self.wait_for_console_pattern('OpenBMC Project Reference Distro') +self.wait_for_console_pattern('gsj login:') + +def test_arm_quanta_gsj_initrd(self): +""" +:avocado: tags=arch:arm +:avocado: tags=machine:quanta-gsj +""" +initrd_url = ( +'https://github.com/hskinnemoen/openbmc/releases/download/' +'20200711-gsj-qemu-0/obmc-phosphor-initramfs-gsj.cpio.xz') +initrd_hash = '98fefe5d7e56727b1eb17d5c00311b1b5c945300' +initrd_path = self.fetch_asset(initrd_url, asset_hash=initrd_hash) +kernel_url = ( +'https://github.com/hskinnemoen/openbmc/releases/download/' +'20200711-gsj-qemu-0/uImage-gsj.bin') +kernel_hash = 'fa67b2f141d56d39b3c54305c0e8a899c99eb2c7' +kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash) +dtb_url = ( +'https://github.com/hskinnemoen/openbmc/releases/download/' +'20200711-gsj-qemu-0/nuvoton-npcm730-gsj.dtb') +dtb_hash = '18315f7006d7b688d8312d5c727eecd819aa36a4' +dtb_path = self.fetch_asset(dtb_url, asset_hash=dtb_hash) + +self.vm.set_console() +kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE + + 'console=ttyS0,115200n8 ' + 'earlycon=uart8250,mmio32,0xf0001000') +self.vm.add_args('-kernel', kernel_path, + '-initrd', initrd_path, + '-dtb', dtb_path, + '-append', kernel_command_line) +self.vm.launch() + +self.wait_for_console_pattern('Booting Linux on physical CPU 0x0') +self.wait_for_console_pattern('CPU1: thread -1, cpu 1, socket 0') +self.wait_for_console_pattern( +'Give root password for system maintenance') + def test_arm_orangepi(self): """ :avocado: tags=arch:arm -- 2.28.0.rc0.105.gf9edc3c819-goog
Re: [PATCH v4 for-5.2 1/2] spapr: Use error_append_hint() in spapr_caps.c
On Thu, Jul 16, 2020 at 07:11:11PM +0200, Greg Kurz wrote: > We have a dedicated error API for hints. Use it instead of embedding > the hint in the error message, as recommanded in the "qapi/error.h" > header file. > > Since spapr_caps_apply() passes &error_fatal, all functions must > also call the ERRP_GUARD() macro for error_append_hint() to be > functional. > > While here, have cap_fwnmi_apply(), which already uses > error_append_hint(), to call ERRP_GUARD() as well. > > Signed-off-by: Greg Kurz > Reviewed-by: Vladimir Sementsov-Ogievskiy > Reviewed-by: Laurent Vivier Applied to ppc-for-5.2. > --- > hw/ppc/spapr_caps.c | 89 > +-- > 1 file changed, 50 insertions(+), 39 deletions(-) > > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > index 3225fc5a2edc..275f5bd0342c 100644 > --- a/hw/ppc/spapr_caps.c > +++ b/hw/ppc/spapr_caps.c > @@ -180,24 +180,24 @@ static void spapr_cap_set_pagesize(Object *obj, Visitor > *v, const char *name, > > static void cap_htm_apply(SpaprMachineState *spapr, uint8_t val, Error > **errp) > { > +ERRP_GUARD(); > if (!val) { > /* TODO: We don't support disabling htm yet */ > return; > } > if (tcg_enabled()) { > -error_setg(errp, > - "No Transactional Memory support in TCG," > - " try appending -machine cap-htm=off"); > +error_setg(errp, "No Transactional Memory support in TCG"); > +error_append_hint(errp, "Try appending -machine cap-htm=off\n"); > } else if (kvm_enabled() && !kvmppc_has_cap_htm()) { > error_setg(errp, > -"KVM implementation does not support Transactional Memory," > - " try appending -machine cap-htm=off" > -); > + "KVM implementation does not support Transactional > Memory"); > +error_append_hint(errp, "Try appending -machine cap-htm=off\n"); > } > } > > static void cap_vsx_apply(SpaprMachineState *spapr, uint8_t val, Error > **errp) > { > +ERRP_GUARD(); > PowerPCCPU *cpu = POWERPC_CPU(first_cpu); > CPUPPCState *env = &cpu->env; > > @@ -209,13 +209,14 @@ static void cap_vsx_apply(SpaprMachineState *spapr, > uint8_t val, Error **errp) > * rid of anything that doesn't do VMX */ > g_assert(env->insns_flags & PPC_ALTIVEC); > if (!(env->insns_flags2 & PPC2_VSX)) { > -error_setg(errp, "VSX support not available," > - " try appending -machine cap-vsx=off"); > +error_setg(errp, "VSX support not available"); > +error_append_hint(errp, "Try appending -machine cap-vsx=off\n"); > } > } > > static void cap_dfp_apply(SpaprMachineState *spapr, uint8_t val, Error > **errp) > { > +ERRP_GUARD(); > PowerPCCPU *cpu = POWERPC_CPU(first_cpu); > CPUPPCState *env = &cpu->env; > > @@ -224,8 +225,8 @@ static void cap_dfp_apply(SpaprMachineState *spapr, > uint8_t val, Error **errp) > return; > } > if (!(env->insns_flags2 & PPC2_DFP)) { > -error_setg(errp, "DFP support not available," > - " try appending -machine cap-dfp=off"); > +error_setg(errp, "DFP support not available"); > +error_append_hint(errp, "Try appending -machine cap-dfp=off\n"); > } > } > > @@ -239,6 +240,7 @@ SpaprCapPossible cap_cfpc_possible = { > static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, > Error **errp) > { > +ERRP_GUARD(); > uint8_t kvm_val = kvmppc_get_cap_safe_cache(); > > if (tcg_enabled() && val) { > @@ -247,9 +249,9 @@ static void cap_safe_cache_apply(SpaprMachineState > *spapr, uint8_t val, > cap_cfpc_possible.vals[val]); > } else if (kvm_enabled() && (val > kvm_val)) { > error_setg(errp, > - "Requested safe cache capability level not supported by > kvm," > - " try appending -machine cap-cfpc=%s", > - cap_cfpc_possible.vals[kvm_val]); > + "Requested safe cache capability level not supported by > KVM"); > +error_append_hint(errp, "Try appending -machine cap-cfpc=%s\n", > + cap_cfpc_possible.vals[kvm_val]); > } > } > > @@ -263,6 +265,7 @@ SpaprCapPossible cap_sbbc_possible = { > static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t > val, > Error **errp) > { > +ERRP_GUARD(); > uint8_t kvm_val = kvmppc_get_cap_safe_bounds_check(); > > if (tcg_enabled() && val) { > @@ -271,9 +274,9 @@ static void cap_safe_bounds_check_apply(SpaprMachineState > *spapr, uint8_t val, > cap_sbbc_possible.vals[val]); > } else if (kvm_enabled() && (val > kvm_val)) { > error_setg(errp, > -"Requested safe bounds check capability level not supported by kvm," > - " t
[PATCH v6 07/13] hw/arm: Load -bios image as a boot ROM for npcm7xx
If a -bios option is specified on the command line, load the image into the internal ROM memory region, which contains the first instructions run by the CPU after reset. If -bios is not specified, the vbootrom included with qemu is loaded by default. Reviewed-by: Tyrone Ting Signed-off-by: Havard Skinnemoen --- hw/arm/npcm7xx_boards.c | 31 +++ 1 file changed, 31 insertions(+) diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c index 0b9dce2b35..f32557e0e1 100644 --- a/hw/arm/npcm7xx_boards.c +++ b/hw/arm/npcm7xx_boards.c @@ -18,12 +18,41 @@ #include "hw/arm/npcm7xx.h" #include "hw/core/cpu.h" +#include "hw/loader.h" #include "qapi/error.h" +#include "qemu-common.h" #include "qemu/units.h" +#include "sysemu/sysemu.h" #define NPCM750_EVB_POWER_ON_STRAPS 0x1ff7 #define QUANTA_GSJ_POWER_ON_STRAPS 0x1fff +static const char npcm7xx_default_bootrom[] = "npcm7xx_bootrom.bin"; + +static void npcm7xx_load_bootrom(NPCM7xxState *soc) +{ +g_autofree char *filename = NULL; +const char *bootrom; +int ret; + +if (bios_name) { +bootrom = bios_name; +} else { +bootrom = npcm7xx_default_bootrom; +} + +filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bootrom); +if (!filename) { +error_report("Could not find ROM image '%s'", bootrom); +exit(1); +} +ret = load_image_mr(filename, &soc->irom); +if (ret < 0) { +error_report("Failed to load ROM image '%s'", filename); +exit(1); +} +} + static void npcm7xx_connect_dram(NPCM7xxState *soc, MemoryRegion *dram) { memory_region_add_subregion(get_system_memory(), NPCM7XX_DRAM_BA, dram); @@ -60,6 +89,7 @@ static void npcm750_evb_init(MachineState *machine) npcm7xx_connect_dram(soc, machine->ram); qdev_realize(DEVICE(soc), NULL, &error_fatal); +npcm7xx_load_bootrom(soc); npcm7xx_load_kernel(machine, soc); } @@ -71,6 +101,7 @@ static void quanta_gsj_init(MachineState *machine) npcm7xx_connect_dram(soc, machine->ram); qdev_realize(DEVICE(soc), NULL, &error_fatal); +npcm7xx_load_bootrom(soc); npcm7xx_load_kernel(machine, soc); } -- 2.28.0.rc0.105.gf9edc3c819-goog
Re: [PATCH v3 2/2] hw/net: Added basic IPv6 software fragmentation
On 2020/7/16 上午11:53, and...@daynix.com wrote: From: Andrew The basic IPv6 fragmentation - adding 'frag' extension to the packet, overall shares some logic with IPv4. It works, but there are still issues with a combination of extensions - in the future, it would require refactoring work to implement workflow with IPv6 and extension. "Jumbo option" isn't added yet, until qemu supports packets greater than 64K. Signed-off-by: Andrew Melnychenko Queued for 5.2 Thanks --- hw/net/net_tx_pkt.c | 7 ++-- include/net/eth.h | 14 +-- net/eth.c | 99 ++--- 3 files changed, 109 insertions(+), 11 deletions(-) diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c index 9560e4a49e..74044c6618 100644 --- a/hw/net/net_tx_pkt.c +++ b/hw/net/net_tx_pkt.c @@ -589,10 +589,11 @@ static bool net_tx_pkt_do_sw_fragmentation(struct NetTxPkt *pkt, more_frags = (fragment_offset + fragment_len < pkt->payload_len); -eth_setup_ip4_fragmentation(l2_iov_base, l2_iov_len, l3_iov_base, -l3_iov_len, fragment_len, fragment_offset, more_frags); +eth_setup_ip_fragmentation(l2_iov_base, l2_iov_len, l3_iov_base, +&l3_iov_len, ETH_MAX_IP_DGRAM_LEN, +fragment_len, fragment_offset, more_frags); -eth_fix_ip4_checksum(l3_iov_base, l3_iov_len); +fragment[NET_TX_PKT_FRAGMENT_L3_HDR_POS].iov_len = l3_iov_len; net_tx_pkt_sendv(pkt, nc, fragment, dst_idx); diff --git a/include/net/eth.h b/include/net/eth.h index 0671be6916..05c75ac9fc 100644 --- a/include/net/eth.h +++ b/include/net/eth.h @@ -139,6 +139,14 @@ struct ip6_ext_hdr_routing { uint8_t rsvd[4]; }; +struct ip6_ext_hdr_fragment { +uint8_t nxt; +uint8_t res0; +uint16_toff; +uint32_tid; +}; + + struct ip6_option_hdr { #define IP6_OPT_PAD1 (0x00) #define IP6_OPT_HOME (0xC9) @@ -399,9 +407,9 @@ void eth_get_protocols(const struct iovec *iov, int iovcnt, eth_ip4_hdr_info *ip4hdr_info, eth_l4_hdr_info *l4hdr_info); -void eth_setup_ip4_fragmentation(const void *l2hdr, size_t l2hdr_len, - void *l3hdr, size_t l3hdr_len, - size_t l3payload_len, +void eth_setup_ip_fragmentation(const void *l2hdr, size_t l2hdr_len, + void *l3hdr, size_t *l3hdr_len, + size_t l3hdr_max_len, size_t l3payload_len, size_t frag_offset, bool more_frags); void diff --git a/net/eth.c b/net/eth.c index 0c1d413ee2..067111526d 100644 --- a/net/eth.c +++ b/net/eth.c @@ -314,10 +314,65 @@ eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff, return 0; } +static bool eth_is_ip6_extension_header_type(uint8_t hdr_type); + +static void *eth_ip6_find_ext(struct ip6_header *ip6, uint8_t ext_type) +{ +uint8_t curr_ext_hdr_type = ip6->ip6_nxt; +struct ip6_ext_hdr *ext_hdr = (struct ip6_ext_hdr *)(ip6 + 1); +for (; eth_is_ip6_extension_header_type(curr_ext_hdr_type);) { +if (curr_ext_hdr_type == ext_type) { +return ext_hdr; +} +curr_ext_hdr_type = ext_hdr->ip6r_nxt; +ext_hdr = (struct ip6_ext_hdr *)(((uint8_t *)ext_hdr) ++ (ext_hdr->ip6r_len + 1) * IP6_EXT_GRANULARITY); +} + +return NULL; +} + +/* + * To add an extension - there is should be + * enough memory 'behind' the ip6 header. + */ +static void *eth_ip6_add_ext_nonsafe(struct ip6_header *ip6, uint8_t ext_type) +{ +uint8_t curr_ext_hdr_type = ip6->ip6_nxt; +struct ip6_ext_hdr *ext_hdr = (struct ip6_ext_hdr *)(ip6 + 1); +struct ip6_ext_hdr *ext_hdr_prev = NULL; + +if (!eth_is_ip6_extension_header_type(curr_ext_hdr_type)) { +ext_hdr->ip6r_nxt = ip6->ip6_nxt; +ip6->ip6_nxt = ext_type; +return ext_hdr; +} + +ext_hdr_prev = ext_hdr; +curr_ext_hdr_type = ext_hdr->ip6r_nxt; +ext_hdr = (struct ip6_ext_hdr *)(((uint8_t *)ext_hdr) ++ (ext_hdr->ip6r_len + 1) * IP6_EXT_GRANULARITY); + +for (; eth_is_ip6_extension_header_type(curr_ext_hdr_type);) { +ext_hdr_prev = ext_hdr; +curr_ext_hdr_type = ext_hdr->ip6r_nxt; +ext_hdr = (struct ip6_ext_hdr *)(((uint8_t *)ext_hdr) ++ (ext_hdr->ip6r_len + 1) * IP6_EXT_GRANULARITY); +} + +ext_hdr->ip6r_nxt = ext_hdr_prev->ip6r_nxt; +ext_hdr_prev->ip6r_nxt = ext_type; + +return ext_hdr; +} + +/* When IP6_FRAGMENT added, first 'id' would be 0x71656d75 */ +static const uint32_t s_first_fragment_identificator = 0x71656d75; /* 'qemu' */ + void -eth_setup_ip4_fragmentation(const void *l2hdr, size_t l2hdr_len, -void *l3hdr, size_t l3hdr_len, -size_t l3payload_len, +eth_setup_ip_fragmentation(const void *l2hdr, size_t l2hdr_len, +
Re: [PULL SUBSYSTEM qemu-pseries] pseries: Update SLOF firmware image
On Fri, Jul 17, 2020 at 01:27:05PM +1000, Alexey Kardashevskiy wrote: > The following changes since commit 1038a309ec829f05a3a3e52a9951cfdb24dfd02c: > > spapr: Add a new level of NUMA for GPUs (2020-07-17 10:36:28 +1000) > > are available in the Git repository at: > > g...@github.com:aik/qemu.git tags/qemu-slof-20200717 > > for you to fetch changes up to 7f5258dd8327d574de455a2271788474fa25548d: > > pseries: Update SLOF firmware image (2020-07-17 13:23:00 +1000) Merged, thanks. > > > Alexey Kardashevskiy (1): > pseries: Update SLOF firmware image > > pc-bios/README | 2 +- > pc-bios/slof.bin | Bin 965112 -> 968368 bytes > roms/SLOF| 2 +- > 3 files changed, 2 insertions(+), 2 deletions(-) > > > *** Note: this is not for master, this is for pseries > > This adds tcgbios (this was posted earlier [1] but got lost) > and fixes FDT update at ibm,client-architecture-support > for huge guests. > > The full list of changes: > > Alexey Kardashevskiy (4): > make: Define default rule for .c when V=1 or V=2 > version: update to 20200513 > fdt: Avoid recursion when traversing tree > version: update to 20200717 > > Gustavo Romero (1): > board-qemu: Fix comment about SLOF start address > > Stefan Berger (6): > tcgbios: Only write logs for PCRs that are allocated > tcgbios: Fix the vendorInfoSize to be of type uint8_t > tcgbios: Add support for SHA3 type of algorithms > elf: Implement elf_get_file_size to determine size of an ELF image > tcgbios: Implement tpm_hash_log_extend_event_buffer > tcgbios: Measure the bootloader file read from disk > > [1] > https://patchwork.ozlabs.org/project/qemu-devel/patch/20200513024355.121476-1-...@ozlabs.ru/ > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
[PATCH v6 11/13] hw/arm: Wire up BMC boot flash for npcm750-evb and quanta-gsj
This allows these NPCM7xx-based boards to boot from a flash image, e.g. one built with OpenBMC. For example like this: IMAGE=${OPENBMC}/build/tmp/deploy/images/gsj/image-bmc qemu-system-arm -machine quanta-gsj -nographic \ -drive file=${IMAGE},if=mtd,bus=0,unit=0,format=raw,snapshot=on Reviewed-by: Tyrone Ting Reviewed-by: Cédric Le Goater Tested-by: Cédric Le Goater Signed-off-by: Havard Skinnemoen --- hw/arm/npcm7xx_boards.c | 20 1 file changed, 20 insertions(+) diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c index f32557e0e1..565ee6f671 100644 --- a/hw/arm/npcm7xx_boards.c +++ b/hw/arm/npcm7xx_boards.c @@ -19,6 +19,7 @@ #include "hw/arm/npcm7xx.h" #include "hw/core/cpu.h" #include "hw/loader.h" +#include "hw/qdev-properties.h" #include "qapi/error.h" #include "qemu-common.h" #include "qemu/units.h" @@ -53,6 +54,22 @@ static void npcm7xx_load_bootrom(NPCM7xxState *soc) } } +static void npcm7xx_connect_flash(NPCM7xxFIUState *fiu, int cs_no, + const char *flash_type, DriveInfo *dinfo) +{ +DeviceState *flash; +qemu_irq flash_cs; + +flash = qdev_new(flash_type); +if (dinfo) { +qdev_prop_set_drive(flash, "drive", blk_by_legacy_dinfo(dinfo)); +} +qdev_realize_and_unref(flash, BUS(fiu->spi), &error_fatal); + +flash_cs = qdev_get_gpio_in_named(flash, SSI_GPIO_CS, 0); +qdev_connect_gpio_out_named(DEVICE(fiu), "cs", cs_no, flash_cs); +} + static void npcm7xx_connect_dram(NPCM7xxState *soc, MemoryRegion *dram) { memory_region_add_subregion(get_system_memory(), NPCM7XX_DRAM_BA, dram); @@ -90,6 +107,7 @@ static void npcm750_evb_init(MachineState *machine) qdev_realize(DEVICE(soc), NULL, &error_fatal); npcm7xx_load_bootrom(soc); +npcm7xx_connect_flash(&soc->fiu[0], 0, "w25q256", drive_get(IF_MTD, 0, 0)); npcm7xx_load_kernel(machine, soc); } @@ -102,6 +120,8 @@ static void quanta_gsj_init(MachineState *machine) qdev_realize(DEVICE(soc), NULL, &error_fatal); npcm7xx_load_bootrom(soc); +npcm7xx_connect_flash(&soc->fiu[0], 0, "mx25l25635e", + drive_get(IF_MTD, 0, 0)); npcm7xx_load_kernel(machine, soc); } -- 2.28.0.rc0.105.gf9edc3c819-goog
[PATCH v6 06/13] roms: Add virtual Boot ROM for NPCM7xx SoCs
This is a minimalistic boot ROM written specifically for use with QEMU. It supports loading the second-stage loader from SPI flash into RAM, SMP boot, and not much else. Signed-off-by: Havard Skinnemoen --- Makefile| 1 + .gitmodules | 3 +++ pc-bios/npcm7xx_bootrom.bin | Bin 0 -> 768 bytes roms/Makefile | 7 +++ roms/vbootrom | 1 + 5 files changed, 12 insertions(+) create mode 100644 pc-bios/npcm7xx_bootrom.bin create mode 16 roms/vbootrom diff --git a/Makefile b/Makefile index 32345c610e..56473c788d 100644 --- a/Makefile +++ b/Makefile @@ -838,6 +838,7 @@ s390-ccw.img s390-netboot.img \ slof.bin skiboot.lid \ palcode-clipper \ u-boot.e500 u-boot-sam460-20100605.bin \ +npcm7xx_bootrom.bin \ qemu_vga.ndrv \ edk2-licenses.txt \ hppa-firmware.img \ diff --git a/.gitmodules b/.gitmodules index 9c0501a4d4..c95eaf8284 100644 --- a/.gitmodules +++ b/.gitmodules @@ -58,3 +58,6 @@ [submodule "roms/qboot"] path = roms/qboot url = https://github.com/bonzini/qboot +[submodule "roms/vbootrom"] + path = roms/vbootrom + url = https://github.com/google/vbootrom.git diff --git a/pc-bios/npcm7xx_bootrom.bin b/pc-bios/npcm7xx_bootrom.bin new file mode 100644 index ..38f89d1b97b0c2e133af2a9fbed0521be132065b GIT binary patch literal 768 zcmd5)JxClu6n-9S05p1#kf90Sj5Z(jG8}+)IZIp~iXK=T&)dL`%d-q*8aR#mq{7 z9`=6;Dr(H0ACe72R5x?!)^86Qj-X%{+!K9iZNA@*wkBAV&iZ(l^I9?!Gz=S2I_*1d zr+tTQDHjvyzKnw(hu00yX`u!FvC;DilBe_YlkeSUVHA-crNk+k jtiF_MudA
[PATCH v6 12/13] docs/system: Add Nuvoton machine documentation
Reviewed-by: Cédric Le Goater Signed-off-by: Havard Skinnemoen --- docs/system/arm/nuvoton.rst | 90 + docs/system/target-arm.rst | 1 + 2 files changed, 91 insertions(+) create mode 100644 docs/system/arm/nuvoton.rst diff --git a/docs/system/arm/nuvoton.rst b/docs/system/arm/nuvoton.rst new file mode 100644 index 00..36bf901122 --- /dev/null +++ b/docs/system/arm/nuvoton.rst @@ -0,0 +1,90 @@ +Nuvoton iBMC boards (``npcm750-evb``, ``quanta-gsj``) += + +The `Nuvoton iBMC`_ chips (NPCM7xx) are a family of ARM-based SoCs that are +designed to be used as Baseboard Management Controllers (BMCs) in various +servers. They all feature one or two ARM Cortex A9 CPU cores, as well as an +assortment of peripherals targeted for either Enterprise or Data Center / +Hyperscale applications. The former is a superset of the latter, so NPCM750 has +all the peripherals of NPCM730 and more. + +.. _Nuvoton iBMC: https://www.nuvoton.com/products/cloud-computing/ibmc/ + +The NPCM750 SoC has two Cortex A9 cores and is targeted for the Enterprise +segment. The following machines are based on this chip : + +- ``npcm750-evb`` Nuvoton NPCM750 Evaluation board + +The NPCM730 SoC has two Cortex A9 cores and is targeted for Data Center and +Hyperscale applications. The following machines are based on this chip : + +- ``quanta-gsj``Quanta GSJ server BMC + +There are also two more SoCs, NPCM710 and NPCM705, which are single-core +variants of NPCM750 and NPCM730, respectively. These are currently not +supported by QEMU. + +Supported devices +- + + * SMP (Dual Core Cortex-A9) + * Cortex-A9MPCore built-in peripherals: SCU, GIC, Global Timer, Private Timer + and Watchdog. + * SRAM, ROM and DRAM mappings + * System Global Control Registers (GCR) + * Clock and reset controller (CLK) + * Timer controller (TIM) + * Serial ports (16550-based) + * DDR4 memory controller (dummy interface indicating memory training is done) + * OTP controllers (no protection features) + * Flash Interface Unit (FIU; no protection features) + +Missing devices +--- + + * GPIO controller + * LPC/eSPI host-to-BMC interface, including + + * Keyboard and mouse controller interface (KBCI) + * Keyboard Controller Style (KCS) channels + * BIOS POST code FIFO + * System Wake-up Control (SWC) + * Shared memory (SHM) + * eSPI slave interface + + * Ethernet controllers (GMAC and EMC) + * USB host (USBH) + * USB device (USBD) + * SMBus controller (SMBF) + * Peripheral SPI controller (PSPI) + * Analog to Digital Converter (ADC) + * SD/MMC host + * Random Number Generator (RNG) + * PECI interface + * Pulse Width Modulation (PWM) + * Tachometer + * PCI and PCIe root complex and bridges + * VDM and MCTP support + * Serial I/O expansion + * LPC/eSPI host + * Coprocessor + * Graphics + * Video capture + * Encoding compression engine + * Security features + +Boot options + + +The Nuvoton machines can boot from an OpenBMC firmware image, or directly into +a kernel using the ``-kernel`` option. OpenBMC images for `quanta-gsj` and +possibly others can be downloaded from the OpenPOWER jenkins : + + https://openpower.xyz/ + +The firmware image should be attached as an MTD drive. Example : + +.. code-block:: bash + + $ qemu-system-arm -machine quanta-gsj -nographic \ + -drive file=image-bmc,if=mtd,bus=0,unit=0,format=raw diff --git a/docs/system/target-arm.rst b/docs/system/target-arm.rst index 1bd477a293..38a9daa9b9 100644 --- a/docs/system/target-arm.rst +++ b/docs/system/target-arm.rst @@ -84,6 +84,7 @@ undocumented; you can get a complete list by running arm/aspeed arm/musicpal arm/nseries + arm/nuvoton arm/orangepi arm/palm arm/xscale -- 2.28.0.rc0.105.gf9edc3c819-goog
[PATCH v6 05/13] hw/arm: Add two NPCM7xx-based machines
This adds two new machines, both supported by OpenBMC: - npcm750-evb: Nuvoton NPCM750 Evaluation Board. - quanta-gsj: A board with a NPCM730 chip. They rely on the NPCM7xx SoC device to do the heavy lifting. They are almost completely identical at the moment, apart from the SoC type, which currently only changes the reset contents of one register (GCR.MDLR), but they might grow apart a bit more as more functionality is added. Both machines can boot the Linux kernel into /bin/sh. Reviewed-by: Tyrone Ting Reviewed-by: Joel Stanley Reviewed-by: Cédric Le Goater Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Havard Skinnemoen --- default-configs/arm-softmmu.mak | 1 + include/hw/arm/npcm7xx.h| 19 + hw/arm/npcm7xx_boards.c | 144 hw/arm/Makefile.objs| 2 +- 4 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 hw/arm/npcm7xx_boards.c diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak index 8fc09a4a51..9a94ebd0be 100644 --- a/default-configs/arm-softmmu.mak +++ b/default-configs/arm-softmmu.mak @@ -27,6 +27,7 @@ CONFIG_GUMSTIX=y CONFIG_SPITZ=y CONFIG_TOSA=y CONFIG_Z2=y +CONFIG_NPCM7XX=y CONFIG_COLLIE=y CONFIG_ASPEED_SOC=y CONFIG_NETDUINO2=y diff --git a/include/hw/arm/npcm7xx.h b/include/hw/arm/npcm7xx.h index e68d9c79e6..ba7495869d 100644 --- a/include/hw/arm/npcm7xx.h +++ b/include/hw/arm/npcm7xx.h @@ -35,6 +35,25 @@ #define NPCM7XX_SMP_BOOTREG_ADDR(0xf080013c) /* GCR.SCRPAD */ #define NPCM7XX_GIC_CPU_IF_ADDR (0xf03fe100) /* GIC within A9 */ +typedef struct NPCM7xxMachine { +MachineStateparent; +} NPCM7xxMachine; + +#define TYPE_NPCM7XX_MACHINE MACHINE_TYPE_NAME("npcm7xx") +#define NPCM7XX_MACHINE(obj)\ +OBJECT_CHECK(NPCM7xxMachine, (obj), TYPE_NPCM7XX_MACHINE) + +typedef struct NPCM7xxMachineClass { +MachineClassparent; + +const char *soc_type; +} NPCM7xxMachineClass; + +#define NPCM7XX_MACHINE_CLASS(klass)\ +OBJECT_CLASS_CHECK(NPCM7xxMachineClass, (klass), TYPE_NPCM7XX_MACHINE) +#define NPCM7XX_MACHINE_GET_CLASS(obj) \ +OBJECT_GET_CLASS(NPCM7xxMachineClass, (obj), TYPE_NPCM7XX_MACHINE) + typedef struct NPCM7xxState { DeviceState parent; diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c new file mode 100644 index 00..0b9dce2b35 --- /dev/null +++ b/hw/arm/npcm7xx_boards.c @@ -0,0 +1,144 @@ +/* + * Machine definitions for boards featuring an NPCM7xx SoC. + * + * Copyright 2020 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ + +#include "qemu/osdep.h" + +#include "hw/arm/npcm7xx.h" +#include "hw/core/cpu.h" +#include "qapi/error.h" +#include "qemu/units.h" + +#define NPCM750_EVB_POWER_ON_STRAPS 0x1ff7 +#define QUANTA_GSJ_POWER_ON_STRAPS 0x1fff + +static void npcm7xx_connect_dram(NPCM7xxState *soc, MemoryRegion *dram) +{ +memory_region_add_subregion(get_system_memory(), NPCM7XX_DRAM_BA, dram); + +object_property_set_link(OBJECT(soc), "dram-mr", OBJECT(dram), + &error_abort); +} + +static NPCM7xxState *npcm7xx_create_soc(MachineState *machine, +uint32_t hw_straps) +{ +NPCM7xxMachineClass *nmc = NPCM7XX_MACHINE_GET_CLASS(machine); +MachineClass *mc = &nmc->parent; +Object *obj; + +if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) { +error_report("This board can only be used with %s", + mc->default_cpu_type); +exit(1); +} + +obj = object_new_with_props(nmc->soc_type, OBJECT(machine), "soc", +&error_abort, NULL); +object_property_set_uint(obj, "power-on-straps", hw_straps, &error_abort); + +return NPCM7XX(obj); +} + +static void npcm750_evb_init(MachineState *machine) +{ +NPCM7xxState *soc; + +soc = npcm7xx_create_soc(machine, NPCM750_EVB_POWER_ON_STRAPS); +npcm7xx_connect_dram(soc, machine->ram); +qdev_realize(DEVICE(soc), NULL, &error_fatal); + +npcm7xx_load_kernel(machine, soc); +} + +static void quanta_gsj_init(MachineState *machine) +{ +NPCM7xxState *soc; + +soc = npcm7xx_create_soc(machine, QUANTA_GSJ_POWER_ON_STRAPS); +npcm7xx_connect_dram(soc, machine->ram); +qdev_realize(DEVICE(soc), NULL, &error_fatal); + +npcm7xx_load_kernel(machine, soc); +} + +static
[PATCH v6 04/13] hw/arm: Add NPCM730 and NPCM750 SoC models
The Nuvoton NPCM7xx SoC family are used to implement Baseboard Management Controllers in servers. While the family includes four SoCs, this patch implements limited support for two of them: NPCM730 (targeted for Data Center applications) and NPCM750 (targeted for Enterprise applications). This patch includes little more than the bare minimum needed to boot a Linux kernel built with NPCM7xx support in direct-kernel mode: - Two Cortex-A9 CPU cores with built-in periperhals. - Global Configuration Registers. - Clock Management. - 3 Timer Modules with 5 timers each. - 4 serial ports. The chips themselves have a lot more features, some of which will be added to the model at a later stage. Reviewed-by: Tyrone Ting Reviewed-by: Joel Stanley Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Havard Skinnemoen --- include/hw/arm/npcm7xx.h | 85 hw/arm/npcm7xx.c | 407 +++ hw/arm/Kconfig | 5 + hw/arm/Makefile.objs | 1 + 4 files changed, 498 insertions(+) create mode 100644 include/hw/arm/npcm7xx.h create mode 100644 hw/arm/npcm7xx.c diff --git a/include/hw/arm/npcm7xx.h b/include/hw/arm/npcm7xx.h new file mode 100644 index 00..e68d9c79e6 --- /dev/null +++ b/include/hw/arm/npcm7xx.h @@ -0,0 +1,85 @@ +/* + * Nuvoton NPCM7xx SoC family. + * + * Copyright 2020 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ +#ifndef NPCM7XX_H +#define NPCM7XX_H + +#include "hw/boards.h" +#include "hw/cpu/a9mpcore.h" +#include "hw/misc/npcm7xx_clk.h" +#include "hw/misc/npcm7xx_gcr.h" +#include "hw/timer/npcm7xx_timer.h" +#include "target/arm/cpu.h" + +#define NPCM7XX_MAX_NUM_CPUS(2) + +/* The first half of the address space is reserved for DDR4 DRAM. */ +#define NPCM7XX_DRAM_BA (0x) +#define NPCM7XX_DRAM_SZ (2 * GiB) + +/* Magic addresses for setting up direct kernel booting and SMP boot stubs. */ +#define NPCM7XX_LOADER_START(0x) /* Start of SDRAM */ +#define NPCM7XX_SMP_LOADER_START(0x) /* Boot ROM */ +#define NPCM7XX_SMP_BOOTREG_ADDR(0xf080013c) /* GCR.SCRPAD */ +#define NPCM7XX_GIC_CPU_IF_ADDR (0xf03fe100) /* GIC within A9 */ + +typedef struct NPCM7xxState { +DeviceState parent; + +ARMCPU cpu[NPCM7XX_MAX_NUM_CPUS]; +A9MPPrivState a9mpcore; + +MemoryRegionsram; +MemoryRegionirom; +MemoryRegionram3; +MemoryRegion*dram; + +NPCM7xxGCRState gcr; +NPCM7xxCLKState clk; +NPCM7xxTimerCtrlState tim[3]; +} NPCM7xxState; + +#define TYPE_NPCM7XX"npcm7xx" +#define NPCM7XX(obj)OBJECT_CHECK(NPCM7xxState, (obj), TYPE_NPCM7XX) + +#define TYPE_NPCM730"npcm730" +#define TYPE_NPCM750"npcm750" + +typedef struct NPCM7xxClass { +DeviceClass parent; + +/* Bitmask of modules that are permanently disabled on this chip. */ +uint32_tdisabled_modules; +/* Number of CPU cores enabled in this SoC class (may be 1 or 2). */ +uint32_tnum_cpus; +} NPCM7xxClass; + +#define NPCM7XX_CLASS(klass)\ +OBJECT_CLASS_CHECK(NPCM7xxClass, (klass), TYPE_NPCM7XX) +#define NPCM7XX_GET_CLASS(obj) \ +OBJECT_GET_CLASS(NPCM7xxClass, (obj), TYPE_NPCM7XX) + +/** + * npcm7xx_load_kernel - Loads memory with everything needed to boot + * @machine - The machine containing the SoC to be booted. + * @soc - The SoC containing the CPU to be booted. + * + * This will set up the ARM boot info structure for the specific NPCM7xx + * derivative and call arm_load_kernel() to set up loading of the kernel, etc. + * into memory, if requested by the user. + */ +void npcm7xx_load_kernel(MachineState *machine, NPCM7xxState *soc); + +#endif /* NPCM7XX_H */ diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c new file mode 100644 index 00..9669ac5fa0 --- /dev/null +++ b/hw/arm/npcm7xx.c @@ -0,0 +1,407 @@ +/* + * Nuvoton NPCM7xx SoC family. + * + * Copyright 2020 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * F
[PATCH v6 10/13] hw/ssi: NPCM7xx Flash Interface Unit device model
This implements a device model for the NPCM7xx SPI flash controller. Direct reads and writes, and user-mode transactions have been tested in various modes. Protection features are not implemented yet. All the FIU instances are available in the SoC's address space, regardless of whether or not they're connected to actual flash chips. Reviewed-by: Tyrone Ting Reviewed-by: Cédric Le Goater Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Havard Skinnemoen --- include/hw/arm/npcm7xx.h | 2 + include/hw/ssi/npcm7xx_fiu.h | 100 +++ hw/arm/npcm7xx.c | 58 hw/ssi/npcm7xx_fiu.c | 539 +++ hw/arm/Kconfig | 1 + hw/ssi/Makefile.objs | 1 + hw/ssi/trace-events | 11 + 7 files changed, 712 insertions(+) create mode 100644 include/hw/ssi/npcm7xx_fiu.h create mode 100644 hw/ssi/npcm7xx_fiu.c diff --git a/include/hw/arm/npcm7xx.h b/include/hw/arm/npcm7xx.h index 9fa84a0702..78d0d78c52 100644 --- a/include/hw/arm/npcm7xx.h +++ b/include/hw/arm/npcm7xx.h @@ -23,6 +23,7 @@ #include "hw/misc/npcm7xx_gcr.h" #include "hw/nvram/npcm7xx_otp.h" #include "hw/timer/npcm7xx_timer.h" +#include "hw/ssi/npcm7xx_fiu.h" #include "target/arm/cpu.h" #define NPCM7XX_MAX_NUM_CPUS(2) @@ -73,6 +74,7 @@ typedef struct NPCM7xxState { NPCM7xxOTPState key_storage; NPCM7xxOTPState fuse_array; NPCM7xxMCState mc; +NPCM7xxFIUState fiu[2]; } NPCM7xxState; #define TYPE_NPCM7XX"npcm7xx" diff --git a/include/hw/ssi/npcm7xx_fiu.h b/include/hw/ssi/npcm7xx_fiu.h new file mode 100644 index 00..b867bd0429 --- /dev/null +++ b/include/hw/ssi/npcm7xx_fiu.h @@ -0,0 +1,100 @@ +/* + * Nuvoton NPCM7xx Flash Interface Unit (FIU) + * + * Copyright 2020 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ +#ifndef NPCM7XX_FIU_H +#define NPCM7XX_FIU_H + +#include "hw/ssi/ssi.h" +#include "hw/sysbus.h" + +/** + * enum NPCM7xxFIURegister - 32-bit FIU register indices. + */ +enum NPCM7xxFIURegister { +NPCM7XX_FIU_DRD_CFG, +NPCM7XX_FIU_DWR_CFG, +NPCM7XX_FIU_UMA_CFG, +NPCM7XX_FIU_UMA_CTS, +NPCM7XX_FIU_UMA_CMD, +NPCM7XX_FIU_UMA_ADDR, +NPCM7XX_FIU_PRT_CFG, +NPCM7XX_FIU_UMA_DW0 = 0x0020 / sizeof(uint32_t), +NPCM7XX_FIU_UMA_DW1, +NPCM7XX_FIU_UMA_DW2, +NPCM7XX_FIU_UMA_DW3, +NPCM7XX_FIU_UMA_DR0, +NPCM7XX_FIU_UMA_DR1, +NPCM7XX_FIU_UMA_DR2, +NPCM7XX_FIU_UMA_DR3, +NPCM7XX_FIU_PRT_CMD0, +NPCM7XX_FIU_PRT_CMD1, +NPCM7XX_FIU_PRT_CMD2, +NPCM7XX_FIU_PRT_CMD3, +NPCM7XX_FIU_PRT_CMD4, +NPCM7XX_FIU_PRT_CMD5, +NPCM7XX_FIU_PRT_CMD6, +NPCM7XX_FIU_PRT_CMD7, +NPCM7XX_FIU_PRT_CMD8, +NPCM7XX_FIU_PRT_CMD9, +NPCM7XX_FIU_CFG = 0x78 / sizeof(uint32_t), +NPCM7XX_FIU_NR_REGS, +}; + +typedef struct NPCM7xxFIUState NPCM7xxFIUState; + +/** + * struct NPCM7xxFIUFlash - Per-chipselect flash controller state. + * @direct_access: Memory region for direct flash access. + * @fiu: Pointer to flash controller shared state. + */ +typedef struct NPCM7xxFIUFlash { +MemoryRegion direct_access; +NPCM7xxFIUState *fiu; +} NPCM7xxFIUFlash; + +/** + * NPCM7xxFIUState - Device state for one Flash Interface Unit. + * @parent: System bus device. + * @mmio: Memory region for register access. + * @cs_count: Number of flash chips that may be connected to this module. + * @active_cs: Currently active chip select, or -1 if no chip is selected. + * @cs_lines: GPIO lines that may be wired to flash chips. + * @flash: Array of @cs_count per-flash-chip state objects. + * @spi: The SPI bus mastered by this controller. + * @regs: Register contents. + * + * Each FIU has a shared bank of registers, and controls up to four chip + * selects. Each chip select has a dedicated memory region which may be used to + * read and write the flash connected to that chip select as if it were memory. + */ +struct NPCM7xxFIUState { +SysBusDevice parent; + +MemoryRegion mmio; + +int32_t cs_count; +int32_t active_cs; +qemu_irq *cs_lines; +NPCM7xxFIUFlash *flash; + +SSIBus *spi; + +uint32_t regs[NPCM7XX_FIU_NR_REGS]; +}; + +#define TYPE_NPCM7XX_FIU "npcm7xx-fiu" +#define NPCM7XX_FIU(obj) OBJECT_CHECK(NPCM7xxFIUState, (obj), TYPE_NPCM7XX_FIU) + +#endif /* NPCM7XX_FIU_H */ diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c index 6bb1693833..7884b2b03d 100644 --- a/hw/arm/npcm7xx.c +++ b/hw/arm/npcm7xx.c @@ -99,6 +99,39 @@ static const hwaddr npcm7xx_uart_addr[] = { 0xf0004
[PATCH v6 03/13] hw/timer: Add NPCM7xx Timer device model
The NPCM730 and NPCM750 SoCs have three timer modules each holding five timers and some shared registers (e.g. interrupt status). Each timer runs at 25 MHz divided by a prescaler, and counts down from a configurable initial value to zero. When zero is reached, the interrupt flag for the timer is set, and the timer is disabled (one-shot mode) or reloaded from its initial value (periodic mode). This implementation is sufficient to boot a Linux kernel configured for NPCM750. Note that the kernel does not seem to actually turn on the interrupts. Reviewed-by: Tyrone Ting Reviewed-by: Joel Stanley Signed-off-by: Havard Skinnemoen --- include/hw/timer/npcm7xx_timer.h | 96 ++ hw/timer/npcm7xx_timer.c | 489 +++ hw/timer/Makefile.objs | 1 + hw/timer/trace-events| 5 + 4 files changed, 591 insertions(+) create mode 100644 include/hw/timer/npcm7xx_timer.h create mode 100644 hw/timer/npcm7xx_timer.c diff --git a/include/hw/timer/npcm7xx_timer.h b/include/hw/timer/npcm7xx_timer.h new file mode 100644 index 00..94900a7877 --- /dev/null +++ b/include/hw/timer/npcm7xx_timer.h @@ -0,0 +1,96 @@ +/* + * Nuvoton NPCM7xx Timer Controller + * + * Copyright 2020 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ +#ifndef NPCM7XX_TIMER_H +#define NPCM7XX_TIMER_H + +#include "exec/memory.h" +#include "hw/sysbus.h" +#include "qemu/timer.h" + +/* Each Timer Module (TIM) instance holds five 25 MHz timers. */ +#define NPCM7XX_TIMERS_PER_CTRL (5) + +/** + * enum NPCM7xxTimerRegisters - 32-bit register indices. + */ +enum NPCM7xxTimerRegisters { +NPCM7XX_TIMER_TCSR0, +NPCM7XX_TIMER_TCSR1, +NPCM7XX_TIMER_TICR0, +NPCM7XX_TIMER_TICR1, +NPCM7XX_TIMER_TDR0, +NPCM7XX_TIMER_TDR1, +NPCM7XX_TIMER_TISR, +NPCM7XX_TIMER_WTCR, +NPCM7XX_TIMER_TCSR2, +NPCM7XX_TIMER_TCSR3, +NPCM7XX_TIMER_TICR2, +NPCM7XX_TIMER_TICR3, +NPCM7XX_TIMER_TDR2, +NPCM7XX_TIMER_TDR3, +NPCM7XX_TIMER_TCSR4 = 0x0040 / sizeof(uint32_t), +NPCM7XX_TIMER_TICR4 = 0x0048 / sizeof(uint32_t), +NPCM7XX_TIMER_TDR4 = 0x0050 / sizeof(uint32_t), +NPCM7XX_TIMER_NR_REGS, +}; + +typedef struct NPCM7xxTimerCtrlState NPCM7xxTimerCtrlState; + +/** + * struct NPCM7xxTimer - Individual timer state. + * @irq: GIC interrupt line to fire on expiration (if enabled). + * @qtimer: QEMU timer that notifies us on expiration. + * @expires_ns: Absolute virtual expiration time. + * @remaining_ns: Remaining time until expiration if timer is paused. + * @tcsr: The Timer Control and Status Register. + * @ticr: The Timer Initial Count Register. + */ +typedef struct NPCM7xxTimer { +NPCM7xxTimerCtrlState *ctrl; + +qemu_irqirq; +QEMUTimer qtimer; +int64_t expires_ns; +int64_t remaining_ns; + +uint32_ttcsr; +uint32_tticr; +} NPCM7xxTimer; + +/** + * struct NPCM7xxTimerCtrlState - Timer Module device state. + * @parent: System bus device. + * @iomem: Memory region through which registers are accessed. + * @tisr: The Timer Interrupt Status Register. + * @wtcr: The Watchdog Timer Control Register. + * @timer: The five individual timers managed by this module. + */ +struct NPCM7xxTimerCtrlState { +SysBusDevice parent; + +MemoryRegion iomem; + +uint32_ttisr; +uint32_twtcr; + +NPCM7xxTimer timer[NPCM7XX_TIMERS_PER_CTRL]; +}; + +#define TYPE_NPCM7XX_TIMER "npcm7xx-timer" +#define NPCM7XX_TIMER(obj) \ +OBJECT_CHECK(NPCM7xxTimerCtrlState, (obj), TYPE_NPCM7XX_TIMER) + +#endif /* NPCM7XX_TIMER_H */ diff --git a/hw/timer/npcm7xx_timer.c b/hw/timer/npcm7xx_timer.c new file mode 100644 index 00..9f9a7048f2 --- /dev/null +++ b/hw/timer/npcm7xx_timer.c @@ -0,0 +1,489 @@ +/* + * Nuvoton NPCM7xx Timer Controller + * + * Copyright 2020 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ + +#include "qemu/osdep.h" + +#include "hw/irq.h" +#include "hw/misc/npcm7xx_clk.h" +#include "hw/timer/n
Re: [PATCH v4 for-5.2 2/2] spapr: Forbid nested KVM-HV in pre-power9 compat mode
On Thu, Jul 16, 2020 at 07:11:21PM +0200, Greg Kurz wrote: > Nested KVM HV only works if the kernel is using the radix MMU mode, ie. > the CPU is POWER9 and it is not running in some pre-power9 compat mode. > Otherwise, the KVM HV module fails to load in the guest with -ENODEV. > It might be painful for a user to discover this late that nested cannot > work with their setup. Erroring out at machine init instead seems to be > the best we can do. > > Signed-off-by: Greg Kurz > Reviewed-by: Laurent Vivier Applied to ppc-for-5.2. > --- > hw/ppc/spapr_caps.c | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > index 275f5bd0342c..10a80a8159f4 100644 > --- a/hw/ppc/spapr_caps.c > +++ b/hw/ppc/spapr_caps.c > @@ -382,6 +382,8 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState > *spapr, > uint8_t val, Error **errp) > { > ERRP_GUARD(); > +PowerPCCPU *cpu = POWERPC_CPU(first_cpu); > + > if (!val) { > /* capability disabled by default */ > return; > @@ -391,6 +393,14 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState > *spapr, > error_setg(errp, "No Nested KVM-HV support in TCG"); > error_append_hint(errp, "Try appending -machine > cap-nested-hv=off\n"); > } else if (kvm_enabled()) { > +if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00, 0, > + spapr->max_compat_pvr)) { > +error_setg(errp, "Nested KVM-HV only supported on POWER9"); > +error_append_hint(errp, > + "Try appending -machine > max-cpu-compat=power9\n"); > +return; > +} > + > if (!kvmppc_has_cap_nested_kvm_hv()) { > error_setg(errp, > "KVM implementation does not support Nested KVM-HV"); > > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
[PATCH v6 08/13] hw/nvram: NPCM7xx OTP device model
This supports reading and writing OTP fuses and keys. Only fuse reading has been tested. Protection is not implemented. Reviewed-by: Avi Fishman Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Havard Skinnemoen --- include/hw/arm/npcm7xx.h | 3 + include/hw/nvram/npcm7xx_otp.h | 88 +++ hw/arm/npcm7xx.c | 29 +++ hw/nvram/npcm7xx_otp.c | 424 + hw/nvram/Makefile.objs | 1 + 5 files changed, 545 insertions(+) create mode 100644 include/hw/nvram/npcm7xx_otp.h create mode 100644 hw/nvram/npcm7xx_otp.c diff --git a/include/hw/arm/npcm7xx.h b/include/hw/arm/npcm7xx.h index ba7495869d..5816a07a72 100644 --- a/include/hw/arm/npcm7xx.h +++ b/include/hw/arm/npcm7xx.h @@ -20,6 +20,7 @@ #include "hw/cpu/a9mpcore.h" #include "hw/misc/npcm7xx_clk.h" #include "hw/misc/npcm7xx_gcr.h" +#include "hw/nvram/npcm7xx_otp.h" #include "hw/timer/npcm7xx_timer.h" #include "target/arm/cpu.h" @@ -68,6 +69,8 @@ typedef struct NPCM7xxState { NPCM7xxGCRState gcr; NPCM7xxCLKState clk; NPCM7xxTimerCtrlState tim[3]; +NPCM7xxOTPState key_storage; +NPCM7xxOTPState fuse_array; } NPCM7xxState; #define TYPE_NPCM7XX"npcm7xx" diff --git a/include/hw/nvram/npcm7xx_otp.h b/include/hw/nvram/npcm7xx_otp.h new file mode 100644 index 00..c4c1b751d4 --- /dev/null +++ b/include/hw/nvram/npcm7xx_otp.h @@ -0,0 +1,88 @@ +/* + * Nuvoton NPCM7xx OTP (Fuse Array) Interface + * + * Copyright 2020 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ +#ifndef NPCM7XX_OTP_H +#define NPCM7XX_OTP_H + +#include "exec/memory.h" +#include "hw/sysbus.h" + +/* Each OTP module holds 8192 bits of one-time programmable storage */ +#define NPCM7XX_OTP_ARRAY_BITS (8192) +#define NPCM7XX_OTP_ARRAY_BYTES (NPCM7XX_OTP_ARRAY_BITS / BITS_PER_BYTE) + +/* Fuse array offsets */ +#define NPCM7XX_FUSE_FUSTRAP (0) +#define NPCM7XX_FUSE_CP_FUSTRAP (12) +#define NPCM7XX_FUSE_DAC_CALIB (16) +#define NPCM7XX_FUSE_ADC_CALIB (24) +#define NPCM7XX_FUSE_DERIVATIVE (64) +#define NPCM7XX_FUSE_TEST_SIG (72) +#define NPCM7XX_FUSE_DIE_LOCATION (74) +#define NPCM7XX_FUSE_GP1 (80) +#define NPCM7XX_FUSE_GP2 (128) + +/** + * enum NPCM7xxOTPRegister - 32-bit register indices. + */ +typedef enum NPCM7xxOTPRegister { +NPCM7XX_OTP_FST, +NPCM7XX_OTP_FADDR, +NPCM7XX_OTP_FDATA, +NPCM7XX_OTP_FCFG, +/* Offset 0x10 is FKEYIND in OTP1, FUSTRAP in OTP2 */ +NPCM7XX_OTP_FKEYIND = 0x0010 / sizeof(uint32_t), +NPCM7XX_OTP_FUSTRAP = 0x0010 / sizeof(uint32_t), +NPCM7XX_OTP_FCTL, +NPCM7XX_OTP_NR_REGS, +} NPCM7xxOTPRegister; + +/** + * struct NPCM7xxOTPState - Device state for one OTP module. + * @parent: System bus device. + * @mmio: Memory region through which registers are accessed. + * @regs: Register contents. + * @array: OTP storage array. + */ +typedef struct NPCM7xxOTPState { +SysBusDevice parent; + +MemoryRegion mmio; +uint32_t regs[NPCM7XX_OTP_NR_REGS]; +uint8_t array[NPCM7XX_OTP_ARRAY_BYTES]; +} NPCM7xxOTPState; + +#define TYPE_NPCM7XX_OTP "npcm7xx-otp" +#define NPCM7XX_OTP(obj) OBJECT_CHECK(NPCM7xxOTPState, (obj), TYPE_NPCM7XX_OTP) + +#define TYPE_NPCM7XX_KEY_STORAGE "npcm7xx-key-storage" +#define TYPE_NPCM7XX_FUSE_ARRAY "npcm7xx-fuse-array" + +typedef struct NPCM7xxOTPClass NPCM7xxOTPClass; + +/** + * npcm7xx_otp_array_write - ECC encode and write data to OTP array. + * @s: OTP module. + * @data: Data to be encoded and written. + * @offset: Offset of first byte to be written in the OTP array. + * @len: Number of bytes before ECC encoding. + * + * Each nibble of data is encoded into a byte, so the number of bytes written + * to the array will be @len * 2. + */ +extern void npcm7xx_otp_array_write(NPCM7xxOTPState *s, const void *data, +unsigned int offset, unsigned int len); + +#endif /* NPCM7XX_OTP_H */ diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c index 9669ac5fa0..9166002598 100644 --- a/hw/arm/npcm7xx.c +++ b/hw/arm/npcm7xx.c @@ -34,6 +34,10 @@ #define NPCM7XX_MMIO_BA (0x8000) #define NPCM7XX_MMIO_SZ (0x7ffd) +/* OTP key storage and fuse strap array */ +#define NPCM7XX_OTP1_BA (0xf0189000) +#define NPCM7XX_OTP2_BA (0xf018a000) + /* Core system modules. */ #define NPCM7XX_L2C_BA (0xf03fc000) #define NPCM7XX_CPUP_BA (0xf03fe000) @@ -144,6 +148,20 @@ void npcm7xx_load_kernel(MachineState *machine, NPCM7xxState *soc) arm_load_kernel(&soc->
[PATCH v6 02/13] hw/misc: Add NPCM7xx Clock Controller device model
Enough functionality to boot the Linux kernel has been implemented. This includes: - Correct power-on reset values so the various clock rates can be accurately calculated. - Clock enables stick around when written. In addition, a best effort attempt to implement SECCNT and CNTR25M was made even though I don't think the kernel needs them. Reviewed-by: Tyrone Ting Reviewed-by: Joel Stanley Reviewed-by: Cédric Le Goater Signed-off-by: Havard Skinnemoen --- include/hw/misc/npcm7xx_clk.h | 72 +++ hw/misc/npcm7xx_clk.c | 234 ++ hw/misc/Makefile.objs | 1 + hw/misc/trace-events | 4 + 4 files changed, 311 insertions(+) create mode 100644 include/hw/misc/npcm7xx_clk.h create mode 100644 hw/misc/npcm7xx_clk.c diff --git a/include/hw/misc/npcm7xx_clk.h b/include/hw/misc/npcm7xx_clk.h new file mode 100644 index 00..40ed64ff15 --- /dev/null +++ b/include/hw/misc/npcm7xx_clk.h @@ -0,0 +1,72 @@ +/* + * Nuvoton NPCM7xx Clock Control Registers. + * + * Copyright 2020 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ +#ifndef NPCM7XX_CLK_H +#define NPCM7XX_CLK_H + +#include "exec/memory.h" +#include "hw/sysbus.h" + +/* + * The reference clock frequency for the timer modules, and the SECCNT and + * CNTR25M registers in this module, is always 25 MHz. + */ +#define NPCM7XX_TIMER_REF_HZ(2500) + +enum NPCM7xxCLKRegisters { +NPCM7XX_CLK_CLKEN1, +NPCM7XX_CLK_CLKSEL, +NPCM7XX_CLK_CLKDIV1, +NPCM7XX_CLK_PLLCON0, +NPCM7XX_CLK_PLLCON1, +NPCM7XX_CLK_SWRSTR, +NPCM7XX_CLK_IPSRST1 = 0x20 / sizeof(uint32_t), +NPCM7XX_CLK_IPSRST2, +NPCM7XX_CLK_CLKEN2, +NPCM7XX_CLK_CLKDIV2, +NPCM7XX_CLK_CLKEN3, +NPCM7XX_CLK_IPSRST3, +NPCM7XX_CLK_WD0RCR, +NPCM7XX_CLK_WD1RCR, +NPCM7XX_CLK_WD2RCR, +NPCM7XX_CLK_SWRSTC1, +NPCM7XX_CLK_SWRSTC2, +NPCM7XX_CLK_SWRSTC3, +NPCM7XX_CLK_SWRSTC4, +NPCM7XX_CLK_PLLCON2, +NPCM7XX_CLK_CLKDIV3, +NPCM7XX_CLK_CORSTC, +NPCM7XX_CLK_PLLCONG, +NPCM7XX_CLK_AHBCKFI, +NPCM7XX_CLK_SECCNT, +NPCM7XX_CLK_CNTR25M, +NPCM7XX_CLK_NR_REGS, +}; + +typedef struct NPCM7xxCLKState { +SysBusDevice parent; + +MemoryRegion iomem; + +uint32_t regs[NPCM7XX_CLK_NR_REGS]; + +/* Time reference for SECCNT and CNTR25M, initialized by power on reset */ +int64_t ref_ns; +} NPCM7xxCLKState; + +#define TYPE_NPCM7XX_CLK "npcm7xx-clk" +#define NPCM7XX_CLK(obj) OBJECT_CHECK(NPCM7xxCLKState, (obj), TYPE_NPCM7XX_CLK) + +#endif /* NPCM7XX_CLK_H */ diff --git a/hw/misc/npcm7xx_clk.c b/hw/misc/npcm7xx_clk.c new file mode 100644 index 00..55beecc06f --- /dev/null +++ b/hw/misc/npcm7xx_clk.c @@ -0,0 +1,234 @@ +/* + * Nuvoton NPCM7xx Clock Control Registers. + * + * Copyright 2020 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ + +#include "qemu/osdep.h" + +#include "hw/misc/npcm7xx_clk.h" +#include "migration/vmstate.h" +#include "qemu/error-report.h" +#include "qemu/log.h" +#include "qemu/module.h" +#include "qemu/timer.h" +#include "qemu/units.h" +#include "trace.h" + +#define PLLCON_LOKI BIT(31) +#define PLLCON_LOKS BIT(30) +#define PLLCON_PWDENBIT(12) + +/* + * These reset values were taken from version 0.91 of the NPCM750R data sheet. + * + * All are loaded on power-up reset. CLKENx and SWRSTR should also be loaded on + * core domain reset, but this reset type is not yet supported by QEMU. + */ +static const uint32_t cold_reset_values[NPCM7XX_CLK_NR_REGS] = { +[NPCM7XX_CLK_CLKEN1]= 0x, +[NPCM7XX_CLK_CLKSEL]= 0x004a, +[NPCM7XX_CLK_CLKDIV1] = 0x5413f855, +[NPCM7XX_CLK_PLLCON0] = 0x00222101 | PLLCON_LOKI, +[NPCM7XX_CLK_PLLCON1] = 0x00202101 | PLLCON_LOKI, +[NPCM7XX_CLK_IPSRST1] = 0x1000, +[NPCM7XX_CLK_IPSRST2] = 0x8000, +[NPCM7XX_CLK_CLKEN2]= 0x, +[NPCM7XX_CLK_CLKDIV2] = 0xaa4f8f9f, +[NPCM7XX_CLK_CLKEN3]= 0x, +[NPCM7XX_
[PATCH v6 09/13] hw/mem: Stubbed out NPCM7xx Memory Controller model
This just implements the bare minimum to cause the boot block to skip memory initialization. Reviewed-by: Tyrone Ting Reviewed-by: Cédric Le Goater Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Havard Skinnemoen --- include/hw/arm/npcm7xx.h| 2 + include/hw/mem/npcm7xx_mc.h | 36 hw/arm/npcm7xx.c| 6 +++ hw/mem/npcm7xx_mc.c | 84 + hw/mem/Makefile.objs| 1 + 5 files changed, 129 insertions(+) create mode 100644 include/hw/mem/npcm7xx_mc.h create mode 100644 hw/mem/npcm7xx_mc.c diff --git a/include/hw/arm/npcm7xx.h b/include/hw/arm/npcm7xx.h index 5816a07a72..9fa84a0702 100644 --- a/include/hw/arm/npcm7xx.h +++ b/include/hw/arm/npcm7xx.h @@ -18,6 +18,7 @@ #include "hw/boards.h" #include "hw/cpu/a9mpcore.h" +#include "hw/mem/npcm7xx_mc.h" #include "hw/misc/npcm7xx_clk.h" #include "hw/misc/npcm7xx_gcr.h" #include "hw/nvram/npcm7xx_otp.h" @@ -71,6 +72,7 @@ typedef struct NPCM7xxState { NPCM7xxTimerCtrlState tim[3]; NPCM7xxOTPState key_storage; NPCM7xxOTPState fuse_array; +NPCM7xxMCState mc; } NPCM7xxState; #define TYPE_NPCM7XX"npcm7xx" diff --git a/include/hw/mem/npcm7xx_mc.h b/include/hw/mem/npcm7xx_mc.h new file mode 100644 index 00..7ed38be243 --- /dev/null +++ b/include/hw/mem/npcm7xx_mc.h @@ -0,0 +1,36 @@ +/* + * Nuvoton NPCM7xx Memory Controller stub + * + * Copyright 2020 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ +#ifndef NPCM7XX_MC_H +#define NPCM7XX_MC_H + +#include "exec/memory.h" +#include "hw/sysbus.h" + +/** + * struct NPCM7xxMCState - Device state for the memory controller. + * @parent: System bus device. + * @mmio: Memory region through which registers are accessed. + */ +typedef struct NPCM7xxMCState { +SysBusDevice parent; + +MemoryRegion mmio; +} NPCM7xxMCState; + +#define TYPE_NPCM7XX_MC "npcm7xx-mc" +#define NPCM7XX_MC(obj) OBJECT_CHECK(NPCM7xxMCState, (obj), TYPE_NPCM7XX_MC) + +#endif /* NPCM7XX_MC_H */ diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c index 9166002598..6bb1693833 100644 --- a/hw/arm/npcm7xx.c +++ b/hw/arm/npcm7xx.c @@ -43,6 +43,7 @@ #define NPCM7XX_CPUP_BA (0xf03fe000) #define NPCM7XX_GCR_BA (0xf080) #define NPCM7XX_CLK_BA (0xf0801000) +#define NPCM7XX_MC_BA (0xf0824000) /* Internal AHB SRAM */ #define NPCM7XX_RAM3_BA (0xc0008000) @@ -186,6 +187,7 @@ static void npcm7xx_init(Object *obj) TYPE_NPCM7XX_KEY_STORAGE); object_initialize_child(obj, "otp2", &s->fuse_array, TYPE_NPCM7XX_FUSE_ARRAY); +object_initialize_child(obj, "mc", &s->mc, TYPE_NPCM7XX_MC); for (i = 0; i < ARRAY_SIZE(s->tim); i++) { object_initialize_child(obj, "tim[*]", &s->tim[i], TYPE_NPCM7XX_TIMER); @@ -261,6 +263,10 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp) sysbus_mmio_map(SYS_BUS_DEVICE(&s->fuse_array), 0, NPCM7XX_OTP2_BA); npcm7xx_init_fuses(s); +/* Fake Memory Controller (MC). Cannot fail. */ +sysbus_realize(SYS_BUS_DEVICE(&s->mc), &error_abort); +sysbus_mmio_map(SYS_BUS_DEVICE(&s->mc), 0, NPCM7XX_MC_BA); + /* Timer Modules (TIM). Cannot fail. */ QEMU_BUILD_BUG_ON(ARRAY_SIZE(npcm7xx_tim_addr) != ARRAY_SIZE(s->tim)); for (i = 0; i < ARRAY_SIZE(s->tim); i++) { diff --git a/hw/mem/npcm7xx_mc.c b/hw/mem/npcm7xx_mc.c new file mode 100644 index 00..0435d06ab4 --- /dev/null +++ b/hw/mem/npcm7xx_mc.c @@ -0,0 +1,84 @@ +/* + * Nuvoton NPCM7xx Memory Controller stub + * + * Copyright 2020 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ + +#include "qemu/osdep.h" + +#include "hw/mem/npcm7xx_mc.h" +#include "qapi/error.h" +#include "qemu/log.h" +#include "qemu/module.h" +#include "qemu/units.h" + +#define NPCM7XX_MC_REGS_SIZE (4 * KiB) + +static uint64_t npcm7xx_mc_read(void *opaque, hwaddr addr, unsigned int size) +{ +/* + * If bits 8..11 @ offset 0 are not zero, the boot block thinks the memor
[PATCH v6 01/13] hw/misc: Add NPCM7xx System Global Control Registers device model
Implement a device model for the System Global Control Registers in the NPCM730 and NPCM750 BMC SoCs. This is primarily used to enable SMP boot (the boot ROM spins reading the SCRPAD register) and DDR memory initialization; other registers are best effort for now. The reset values of the MDLR and PWRON registers are determined by the SoC variant (730 vs 750) and board straps respectively. Reviewed-by: Joel Stanley Reviewed-by: Cédric Le Goater Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Havard Skinnemoen --- include/hw/misc/npcm7xx_gcr.h | 76 hw/misc/npcm7xx_gcr.c | 227 ++ MAINTAINERS | 8 ++ hw/arm/Kconfig| 3 + hw/misc/Makefile.objs | 1 + hw/misc/trace-events | 4 + 6 files changed, 319 insertions(+) create mode 100644 include/hw/misc/npcm7xx_gcr.h create mode 100644 hw/misc/npcm7xx_gcr.c diff --git a/include/hw/misc/npcm7xx_gcr.h b/include/hw/misc/npcm7xx_gcr.h new file mode 100644 index 00..4884676be2 --- /dev/null +++ b/include/hw/misc/npcm7xx_gcr.h @@ -0,0 +1,76 @@ +/* + * Nuvoton NPCM7xx System Global Control Registers. + * + * Copyright 2020 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ +#ifndef NPCM7XX_GCR_H +#define NPCM7XX_GCR_H + +#include "exec/memory.h" +#include "hw/sysbus.h" + +enum NPCM7xxGCRRegisters { +NPCM7XX_GCR_PDID, +NPCM7XX_GCR_PWRON, +NPCM7XX_GCR_MFSEL1 = 0x0c / sizeof(uint32_t), +NPCM7XX_GCR_MFSEL2, +NPCM7XX_GCR_MISCPE, +NPCM7XX_GCR_SPSWC = 0x038 / sizeof(uint32_t), +NPCM7XX_GCR_INTCR, +NPCM7XX_GCR_INTSR, +NPCM7XX_GCR_HIFCR = 0x050 / sizeof(uint32_t), +NPCM7XX_GCR_INTCR2 = 0x060 / sizeof(uint32_t), +NPCM7XX_GCR_MFSEL3, +NPCM7XX_GCR_SRCNT, +NPCM7XX_GCR_RESSR, +NPCM7XX_GCR_RLOCKR1, +NPCM7XX_GCR_FLOCKR1, +NPCM7XX_GCR_DSCNT, +NPCM7XX_GCR_MDLR, +NPCM7XX_GCR_SCRPAD3, +NPCM7XX_GCR_SCRPAD2, +NPCM7XX_GCR_DAVCLVLR= 0x098 / sizeof(uint32_t), +NPCM7XX_GCR_INTCR3, +NPCM7XX_GCR_VSINTR = 0x0ac / sizeof(uint32_t), +NPCM7XX_GCR_MFSEL4, +NPCM7XX_GCR_CPBPNTR = 0x0c4 / sizeof(uint32_t), +NPCM7XX_GCR_CPCTL = 0x0d0 / sizeof(uint32_t), +NPCM7XX_GCR_CP2BST, +NPCM7XX_GCR_B2CPNT, +NPCM7XX_GCR_CPPCTL, +NPCM7XX_GCR_I2CSEGSEL, +NPCM7XX_GCR_I2CSEGCTL, +NPCM7XX_GCR_VSRCR, +NPCM7XX_GCR_MLOCKR, +NPCM7XX_GCR_SCRPAD = 0x013c / sizeof(uint32_t), +NPCM7XX_GCR_USB1PHYCTL, +NPCM7XX_GCR_USB2PHYCTL, +NPCM7XX_GCR_NR_REGS, +}; + +typedef struct NPCM7xxGCRState { +SysBusDevice parent; + +MemoryRegion iomem; + +uint32_t regs[NPCM7XX_GCR_NR_REGS]; + +uint32_t reset_pwron; +uint32_t reset_mdlr; +uint32_t reset_intcr3; +} NPCM7xxGCRState; + +#define TYPE_NPCM7XX_GCR "npcm7xx-gcr" +#define NPCM7XX_GCR(obj) OBJECT_CHECK(NPCM7xxGCRState, (obj), TYPE_NPCM7XX_GCR) + +#endif /* NPCM7XX_GCR_H */ diff --git a/hw/misc/npcm7xx_gcr.c b/hw/misc/npcm7xx_gcr.c new file mode 100644 index 00..dc08d2ebcc --- /dev/null +++ b/hw/misc/npcm7xx_gcr.c @@ -0,0 +1,227 @@ +/* + * Nuvoton NPCM7xx System Global Control Registers. + * + * Copyright 2020 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ + +#include "qemu/osdep.h" + +#include "hw/misc/npcm7xx_gcr.h" +#include "hw/qdev-properties.h" +#include "migration/vmstate.h" +#include "qapi/error.h" +#include "qemu/log.h" +#include "qemu/module.h" +#include "qemu/units.h" + +#include "trace.h" + +#define NPCM7XX_GCR_MIN_DRAM_SIZE (128 * MiB) +#define NPCM7XX_GCR_MAX_DRAM_SIZE (2 * GiB) + +static const uint32_t cold_reset_values[NPCM7XX_GCR_NR_REGS] = { +[NPCM7XX_GCR_PDID] = 0x04a92750, /* Poleg A1 */ +[NPCM7XX_GCR_MISCPE]= 0x, +[NPCM7XX_GCR_SPSWC] = 0x0003, +[NPCM7XX_GCR_INTCR] = 0x035e, +[NPCM7XX_GCR_HIFCR] = 0x004e, +[NPCM7XX_GCR_INTCR2]= (1U << 19), /* DDR initializ
[PATCH v6 00/13] Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines
I also pushed this and the previous two patchsets to my qemu fork on github. The branches are named npcm7xx-v[1-6]. https://github.com/hskinnemoen/qemu This patch series models enough of the Nuvoton NPCM730 and NPCM750 SoCs to boot an OpenBMC image built for quanta-gsj. This includes device models for: - Global Configuration Registers - Clock Control - Timers - Fuses - Memory Controller - Flash Controller These modules, along with the existing Cortex A9 CPU cores and built-in peripherals, are integrated into a NPCM730 or NPCM750 SoC, which in turn form the foundation for the quanta-gsj and npcm750-evb machines, respectively. The two SoCs are very similar; the only difference is that NPCM730 is missing some peripherals that NPCM750 has, and which are not considered essential for datacenter use (e.g. graphics controllers). For more information, see https://www.nuvoton.com/products/cloud-computing/ibmc/ Both quanta-gsj and npcm750-evb correspond to real boards supported by OpenBMC. At the end of the series, qemu can boot an OpenBMC image built for one of these boards with some minor modifications. The patches in this series were developed by Google and reviewed by Nuvoton. We will be maintaining the machine and peripheral support together. The data sheet for these SoCs is not generally available. Please let me know if more comments are needed to understand the device behavior. Changes since v5: - Boot ROM included, as a git submodule and a binary blob, and loaded by default, so the -bios option is usually not necessary anymore. - Two acceptance tests added (openbmc image boot, and direct kernel boot). - npcm7xx_load_kernel() moved to SoC code. - NPCM7XX_TIMER_REF_HZ definition moved to CLK header. - Comments added clarifying available SPI flash chip selects. - Error handling adjustments: - Errors from CPU and GCR realization are propagated through the SoC since they may be triggered by user-configurable parameters. - Machine init uses error_fatal instead of error_abort for SoC realization flash init. This makes error messages more helpful. - Comments added to indicate whether peripherals may fail to realize. - Use ERRP_GUARD() instead of Error *err when possible. - Default CPU type is now set, and attempting to set it to anything else will fail. - Format string fixes (use HWADDR_PRIx, etc.) - Simplified memory size encoding and error checking in npcm7xx_gcr. - Encapsulate non-obvious pointer subtraction into helper functions in the FIU and TIMER modules. - Incorporate review feedback into the FIU module: - Add select/deselect trace events. - Use npcm7xx_fiu_{de,}select() consistently. - Use extract/deposit in more places for consistency. - Use -Wimplicit-fallthrough compatible fallthrough comments. - Use qdev_init_gpio_out_named instead of sysbus_init_irq for chip selects. - Incorporate review feedback into the TIMER module: - Assert that we never pause a timer that has already expired, instead of trying to handle it. This should be safe since QEMU_CLOCK_VIRTUAL is stopped while this code is running. - Simplify the switch blocks in the read and write handlers. I made a change to error out if a flash drive was not specified, but reverted it because it caused make check to fail (qom-test). When specifying a NULL block device, the m25p flash device initializes its in-memory storage with 0xff and doesn't attempt to write anything back. This seems correct to me. Changes since v4: - OTP cleanups suggested by Philippe Mathieu-Daudé. - Added fuse array definitions based on public Nuvoton bootblock code. - Moved class structure to .c file since it's only used internally. - Readability improvements. - Split the first patch and folded parts of it into three other patches so that CONFIG_NPCM7XX is only enabled after the initial NPCM7xx machine support is added. - DRAM init moved to machine init code. - Consistently use lower-case hex literals. - Switched to fine-grained unimplemented devices, based on public bootblock source code. Added a tiny SRAM that got left out previously. - Simplified error handling in npcm7xx_realize() since the board code will abort anyway, and SoCs are not hot-pluggable. Changes since v3: - License headers are now GPL v2-or-later throughout. - Added vmstate throughout (except the memory controller, which doesn't really have any state worth saving). Successfully booted a gsj image with two stop/savevm/quit/loadvm cycles along the way. - JFFS2 really doesn't like it if I let qemu keep running after savevm, and then jump back in time with loadvm. I assume this is expected. - Fixed an error API violation in npcm7xx_realize, removed pointless error check after object_property_set_link(). - Switched the OTP device to use an embedded array instead of a g_m
Re: Slow down with: 'Make "info qom-tree" show children sorted'
On Thu, 16 Jul 2020 07:37:17 +0200 Markus Armbruster wrote: > David Gibson writes: > > [...] > [...] > [...] > [...] > [...] > [...] > [...] > [...] > [...] > > > > ... as you say, 256 shouldn't really be a problem. I was concerned > > about LMB DRCs rather than PCI DRCs. To have that show up, you might > > need to create a machine with a large difference between initial memory > > and maxmem - I think you'll get a DRC object for every 256MiB in there, > > which can easily get into the thousands for large (potential) memory > > VMs. > > Okay, I can reproduce: with -m 256,128G, /machine has 549 children, of > which 511 are spapr-drc-lmb. Ok. > > I don't know what the config was that showed up this problem in the > > first place, and whether that could be the case there. > > Thomas reported device-introspect-test -m slow has become much slower > for ppc64. Bisection traced it to my commit e8c9e65816 "qom: Make "info > qom-tree" show children sorted". Uses default memory size, no > spapr-drc-lmb as far as I remember. Ok, I think that nixes my theory. > >> > Though avoiding a n^2 behaviour here is probably a good > >> > idea anyway. > >> > >> Agreed. > > Patch posted: > > Subject: [PATCH for-5.1 5/5] qom: Make info qom-tree sort children more > efficiently > Message-Id: <20200714160202.3121879-6-arm...@redhat.com> > -- David Gibson Principal Software Engineer, Virtualization, Red Hat pgpNJKUKCBNgO.pgp Description: OpenPGP digital signature
Re: [PATCH] net: check payload length limit for all frames
On 2020/7/17 下午1:06, P J P wrote: Hello Jason, all +-- On Fri, 17 Jul 2020, Jason Wang wrote --+ | On 2020/7/17 上午9:21, Alexander Bulekov wrote: | > On 200717 0853, Li Qiang wrote: | >> Which issue are you trying to solve, any reference linking? | >> I also send a patch related this part and also a UAF. | > | > I reported a UAF privately to QEMU-Security in May. I believe the one Li | > is referring to is this one https://bugs.launchpad.net/qemu/+bug/1886362 | > | > When I saw Prasad's email, I was worried that I reported the same bug | > twice, but I can still reproduce LP#1886362 with Prasad's patch. | > | > On the other hand, I cannot reproduce either issue with Li's patch: | > Message-Id: <20200716161453.61295-1-liq...@163.com> | > | > Based on this, I think there were two distinct issues. Yes, LP#1886362 looks different that the one fixed here. | Could you describe the issue you saw in details? (E.g the calltrace?) The | commit log does not explain where we can get OOB or UAF. I should've included the backtrace in the commit message. It crossed my mind after I sent the patch. Sorry. Thanks but I don't see a direct relation between 64K limit and this calltrace. Maybe you can elaborate more on this? Thanks === 1040323==ERROR: AddressSanitizer: heap-use-after-free on address 0x606344d8 at pc 0x5571b8fb9ce7 bp 0x7ffede5a2290 sp 0x7ffede5a2280 READ of size 8 at 0x606344d8 thread T0 #0 0x5571b8fb9ce6 in e1000e_write_packet_to_guest hw/net/e1000e_core.c:1587 #1 0x5571b8fba8fc in e1000e_receive_iov hw/net/e1000e_core.c:1709 #2 0x5571b8f9cb08 in e1000e_nc_receive_iov hw/net/e1000e.c:213 #3 0x5571b8f90285 in net_tx_pkt_sendv hw/net/net_tx_pkt.c:544 #4 0x5571b8f90aaa in net_tx_pkt_send hw/net/net_tx_pkt.c:620 #5 0x5571b8f90b2e in net_tx_pkt_send_loopback hw/net/net_tx_pkt.c:633 #6 0x5571b8fb3c3b in e1000e_tx_pkt_send hw/net/e1000e_core.c:664 #7 0x5571b8fb43dd in e1000e_process_tx_desc hw/net/e1000e_core.c:743 #8 0x5571b8fb5b4f in e1000e_start_xmit hw/net/e1000e_core.c:934 #9 0x5571b8fbe2ad in e1000e_set_tdt hw/net/e1000e_core.c:2451 #10 0x5571b8fc01e0 in e1000e_core_write hw/net/e1000e_core.c:3265 #11 0x5571b8f9c387 in e1000e_mmio_write hw/net/e1000e.c:109 ... 0x606344d8 is located 24 bytes inside of 64-byte region [0x606344c0,0x60634500) freed by thread T0 here: #0 0x7f89e3b87307 in __interceptor_free (/lib64/libasan.so.6+0xb0307) #1 0x7f89e37c7a6c in g_free (/lib64/libglib-2.0.so.0+0x58a6c) #2 0x5571b8f95fc7 in net_rx_pkt_pull_data hw/net/net_rx_pkt.c:103 #3 0x5571b8f969b3 in net_rx_pkt_attach_iovec_ex hw/net/net_rx_pkt.c:158 #4 0x5571b8fba6bc in e1000e_receive_iov hw/net/e1000e_core.c:1695 #5 0x5571b8f9cb08 in e1000e_nc_receive_iov hw/net/e1000e.c:213 #6 0x5571b8f90285 in net_tx_pkt_sendv hw/net/net_tx_pkt.c:544 #7 0x5571b8f90aaa in net_tx_pkt_send hw/net/net_tx_pkt.c:620 #8 0x5571b8f90b2e in net_tx_pkt_send_loopback hw/net/net_tx_pkt.c:633 #9 0x5571b8fb3c3b in e1000e_tx_pkt_send hw/net/e1000e_core.c:664 #10 0x5571b8fb43dd in e1000e_process_tx_desc hw/net/e1000e_core.c:743 #11 0x5571b8fb5b4f in e1000e_start_xmit hw/net/e1000e_core.c:934 #12 0x5571b8fbe011 in e1000e_set_tctl hw/net/e1000e_core.c:2431 #13 0x5571b8fc01e0 in e1000e_core_write hw/net/e1000e_core.c:3265 #14 0x5571b8f9c387 in e1000e_mmio_write hw/net/e1000e.c:109 ... previously allocated by thread T0 here: #0 0x7f89e3b87667 in __interceptor_malloc (/lib64/libasan.so.6+0xb0667) #1 0x7f89e37c7978 in g_malloc (/lib64/libglib-2.0.so.0+0x58978) #2 0x5571b8f95fc7 in net_rx_pkt_pull_data hw/net/net_rx_pkt.c:103 #3 0x5571b8f969b3 in net_rx_pkt_attach_iovec_ex hw/net/net_rx_pkt.c:158 #4 0x5571b8fba6bc in e1000e_receive_iov hw/net/e1000e_core.c:1695 #5 0x5571b8f9cb08 in e1000e_nc_receive_iov hw/net/e1000e.c:213 #6 0x5571b8f90285 in net_tx_pkt_sendv hw/net/net_tx_pkt.c:544 #7 0x5571b8f90aaa in net_tx_pkt_send hw/net/net_tx_pkt.c:620 #8 0x5571b8f90b2e in net_tx_pkt_send_loopback hw/net/net_tx_pkt.c:633 #9 0x5571b8fb3c3b in e1000e_tx_pkt_send hw/net/e1000e_core.c:664 #10 0x5571b8fb43dd in e1000e_process_tx_desc hw/net/e1000e_core.c:743 #11 0x5571b8fb5b4f in e1000e_start_xmit hw/net/e1000e_core.c:934 #12 0x5571b8fbe2ad in e1000e_set_tdt hw/net/e1000e_core.c:2451 #13 0x5571b8fc01e0 in e1000e_core_write hw/net/e1000e_core.c:3265 #14 0x5571b8f9c387 in e1000e_mmio_write hw/net/e1000e.c:109 === | >>> --- a/hw/net/net_tx_pkt.c | >>> +++ b/hw/net/net_tx_pkt.c | >>> @@ -607,12 +607,10 @@ bool net_tx_pkt_send(struct NetTxPkt *pkt, | >>> NetClientState *nc) | >>>* Since underlying infrastructure does not support IP datagrams | >>>longer | >>>* than 64K we should drop such packets and don't even try to send | >>>*/ | >>> -if (VIRTIO_NET_HDR_GSO_NONE != pkt->virt_hdr.gso_ty
Re: [PATCH] gitlab-ci.yml: Add oss-fuzz build tests
On 16/07/2020 18.33, Alexander Bulekov wrote: > This tries to build and run the fuzzers with the same build-script used > by oss-fuzz. This doesn't guarantee that the builds on oss-fuzz will > also succeed, since oss-fuzz provides its own compiler and fuzzer vars, > but it can catch changes that are not compatible with the the > ./scripts/oss-fuzz/build.sh script. > The strange way of finding fuzzer binaries stems from the method used by > oss-fuzz: > https://github.com/google/oss-fuzz/blob/master/infra/base-images/base-runner/targets_list > > Signed-off-by: Alexander Bulekov > --- > > Similar to Thomas' patch: > >> Note: This patch needs two other patches merged first to work correctly: > >> - 'fuzz: Expect the cmdline in a freeable GString' from Alexander > >> - 'qom: Plug memory leak in "info qom-tree"' from Markus > > Otherwise the test will fail due to detected memory leaks. > > Fair warning: I haven't been able to trigger this new job yet. I tried > to run the pipeline with these changes on my forked repo on gitlab, but > did not reach the build-oss-fuzz. I think this is due to some failures > in the Containers-layer-2 stage: > > ... > Error response from daemon: manifest for > registry.gitlab.com/a1xndr/qemu/qemu/debian-all-test-cross:latest not > found: manifest unknown: manifest unknown > #2 [internal] load .dockerignore > #2 transferring context: > #2 transferring context: 2B 0.1s done > #2 DONE 0.1s > #1 [internal] load build definition from tmpg8j4xoop.docker > #1 transferring dockerfile: 2.21kB 0.1s done > #1 DONE 0.2s > #3 [internal] load metadata for docker.io/qemu/debian10:latest > #3 ERROR: pull access denied, repository does not exist or may require > authorization: server message: insufficient_scope: authorization failed These look like the problems that we've seen with the main repo until two days ago, too, e.g.: https://gitlab.com/qemu-project/qemu/-/jobs/640410842 Maybe Alex (Bennée) can comment on how to resolve them? > > .gitlab-ci.yml | 14 ++ > 1 file changed, 14 insertions(+) > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > index e96f8794b9..a50df420c9 100644 > --- a/.gitlab-ci.yml > +++ b/.gitlab-ci.yml > @@ -182,6 +182,20 @@ build-fuzzer: > || exit 1 ; >done As mentioned in my other mail, I think you can replace my build-fuzzer job once this is working. > +build-oss-fuzz: > + <<: *native_build_job_definition > + variables: > +IMAGE: fedora > + script: > +- OUT_DIR="./build" CC=clang-9 CXX=clang++-9 CFLAGS="-fsanitize=address" > + LIB_FUZZING_ENGINE="-fsanitize=fuzzer" CFL That "CFL" at the end seems to be a typo (leftover from "CFLAGS")? Also the fedora container does not have clang-9 : https://gitlab.com/huth/qemu/-/jobs/643383032#L28 I think it is at clang 10 already, so maybe just use CC=clang (without version number)? > + ./scripts/oss-fuzz/build.sh > +- for fuzzer in $(find ./build-oss-fuzz/DEST_DIR/ -executable -type f); > do > +grep "LLVMFuzzerTestOneInput" ${fuzzer} > /dev/null 2>&1 || continue > ; > +echo Testing ${fuzzer} ... ; > +"${fuzzer}" -runs=1000 || exit 1 ; > + done Should we exclude the virtio-net tests, since they could leak network traffic to the host? Thomas
Re: [PATCH] e1000e: using bottom half to send packets
On 2020/7/17 下午12:46, Li Qiang wrote: Jason Wang 于2020年7月17日周五 上午11:10写道: On 2020/7/17 上午12:14, Li Qiang wrote: Alexander Bulekov reported a UAF bug related e1000e packets send. -->https://bugs.launchpad.net/qemu/+bug/1886362 This is because the guest trigger a e1000e packet send and set the data's address to e1000e's MMIO address. So when the e1000e do DMA it will write the MMIO again and trigger re-entrancy and finally causes this UAF. Paolo suggested to use a bottom half whenever MMIO is doing complicate things in here: -->https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg03342.html Reference here: 'The easiest solution is to delay processing of descriptors to a bottom half whenever MMIO is doing something complicated. This is also better for latency because it will free the vCPU thread more quickly and leave the work to the I/O thread.' I think several things were missed in this patch (take virtio-net as a reference), do we need the following things: Thanks Jason, In fact I know this, I'm scared for touching this but I want to try. Thanks for your advice. - Cancel the bh when VM is stopped. Ok. I think add a vm state change notifier for e1000e can address this. - A throttle to prevent bh from executing too much timer? Ok, I think add a config timeout and add a timer in e1000e can address this. Sorry, a typo. I meant we probably need a tx_burst as what virtio-net did. - A flag to record whether or not this a pending tx (and migrate it?) Is just a flag enough? Could you explain more about the idea behind processing the virtio-net/e1000e using bh like this? Virtio-net use a tx_waiting variable to record whether or not there's a pending bh. (E.g bh is cancelled due to vmstop, we need reschedule it after vmresume). Maybe we can do something simpler by just schecule bh unconditionally during vm resuming. For example, if the guest trigger a lot of packets send and if the bh is scheduled in IO thread. So will we lost packets? We don't since we don't populate virtqueue which means packets are queued there. Thanks How we avoid this in virtio-net. Thanks, Li Qiang Thanks This patch fixes this UAF. Signed-off-by: Li Qiang --- hw/net/e1000e_core.c | 25 + hw/net/e1000e_core.h | 2 ++ 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c index bcd186cac5..6165b04b68 100644 --- a/hw/net/e1000e_core.c +++ b/hw/net/e1000e_core.c @@ -2423,32 +2423,27 @@ e1000e_set_dbal(E1000ECore *core, int index, uint32_t val) static void e1000e_set_tctl(E1000ECore *core, int index, uint32_t val) { -E1000E_TxRing txr; core->mac[index] = val; if (core->mac[TARC0] & E1000_TARC_ENABLE) { -e1000e_tx_ring_init(core, &txr, 0); -e1000e_start_xmit(core, &txr); +qemu_bh_schedule(core->tx[0].tx_bh); } if (core->mac[TARC1] & E1000_TARC_ENABLE) { -e1000e_tx_ring_init(core, &txr, 1); -e1000e_start_xmit(core, &txr); +qemu_bh_schedule(core->tx[1].tx_bh); } } static void e1000e_set_tdt(E1000ECore *core, int index, uint32_t val) { -E1000E_TxRing txr; int qidx = e1000e_mq_queue_idx(TDT, index); uint32_t tarc_reg = (qidx == 0) ? TARC0 : TARC1; core->mac[index] = val & 0x; if (core->mac[tarc_reg] & E1000_TARC_ENABLE) { -e1000e_tx_ring_init(core, &txr, qidx); -e1000e_start_xmit(core, &txr); +qemu_bh_schedule(core->tx[qidx].tx_bh); } } @@ -3322,6 +3317,16 @@ e1000e_vm_state_change(void *opaque, int running, RunState state) } } +static void e1000e_core_tx_bh(void *opaque) +{ +struct e1000e_tx *tx = opaque; +E1000ECore *core = tx->core; +E1000E_TxRing txr; + +e1000e_tx_ring_init(core, &txr, tx - &core->tx[0]); +e1000e_start_xmit(core, &txr); +} + void e1000e_core_pci_realize(E1000ECore *core, const uint16_t *eeprom_templ, @@ -3340,6 +3345,8 @@ e1000e_core_pci_realize(E1000ECore *core, for (i = 0; i < E1000E_NUM_QUEUES; i++) { net_tx_pkt_init(&core->tx[i].tx_pkt, core->owner, E1000E_MAX_TX_FRAGS, core->has_vnet); +core->tx[i].core = core; +core->tx[i].tx_bh = qemu_bh_new(e1000e_core_tx_bh, &core->tx[i]); } net_rx_pkt_init(&core->rx_pkt, core->has_vnet); @@ -3367,6 +3374,8 @@ e1000e_core_pci_uninit(E1000ECore *core) for (i = 0; i < E1000E_NUM_QUEUES; i++) { net_tx_pkt_reset(core->tx[i].tx_pkt); net_tx_pkt_uninit(core->tx[i].tx_pkt); +qemu_bh_delete(core->tx[i].tx_bh); +core->tx[i].tx_bh = NULL; } net_rx_pkt_uninit(core->rx_pkt); diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h index aee32f7e48..94ddc6afc2 100644 --- a/hw/net/e1000e_core.h +++ b/hw/net/e1000e_core.h @@ -77,6 +77,8 @@ struct E1000Core {
Re: [PATCH] gitlab-ci.yml: Add fuzzer tests
On 16/07/2020 18.46, Alexander Bulekov wrote: > On 200716 1209, Thomas Huth wrote: >> So far we neither compile-tested nor run any of the new fuzzers in our CI, >> which led to some build failures of the fuzzer code in the past weeks. >> To avoid this problem, add a job to compile the fuzzer code and run some >> loops (which likely don't find any new bugs via fuzzing, but at least we >> know that the code can still be run). >> >> A nice side-effect of this test is that the leak tests are enabled here, >> so we should now notice some of the memory leaks in our code base earlier. >> >> Signed-off-by: Thomas Huth > > Thank you for this, Thomas. I just sent a patch to add a job that runs > similar tests with the build-script that we use on oss-fuzz > Patch: <20200716163330.29141-1-alx...@bu.edu> Good idea! ... but it does not work quite yet, I gave it a try and ended up here: https://gitlab.com/huth/qemu/-/jobs/643353500 > I know that these jobs have a lot of overlap, but there are enough > quirks in the oss-fuzz build-script that I, personally, don't think > they are redundant. I think we should finally go with your approach and compile the fuzzing test via the script. But since that seems to need some more work first, let's go with my patch now, so that we have something for 5.1-rc1, and then when your patch is ready, replace my "build-fuzzer" job with yours, ok? >> +build-fuzzer: >> + <<: *native_build_job_definition >> + variables: >> +IMAGE: fedora >> + script: >> +- mkdir build >> +- cd build >> +- ../configure --cc=clang --cxx=clang++ --enable-fuzzing >> + --target-list=x86_64-softmmu > > https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg02310.html > When/if this gets merged, enable-fuzzing won't build with > AddressSanitizer, by default, so I would add --enable-sanitizers, just > to be safe. Ok, thanks, I'll add that. >> +- make -j"$JOBS" all check-build x86_64-softmmu/fuzz >> +- make check >> +- for fuzzer in i440fx-qos-fork-fuzz i440fx-qos-noreset-fuzz >> +i440fx-qtest-reboot-fuzz virtio-scsi-flags-fuzz virtio-scsi-fuzz ; >> do > > Any reason for these particular fuzzers? I know the virtio-net ones find > crashes pretty quickly, but I dont think that causes a non-zero exit. I did not include the virtio-net fuzzers because I read that warning "May result in network traffic emitted from the process. Run in an isolated network environment." in the help text ... so I was not sure whether they are really suitable for the CI on the gitlab machines? Thomas
Re: [RFC PATCH-for-5.2 v2 2/2] block/block-backend: Let blk_attach_dev() provide helpful error
Daniel P. Berrangé writes: > On Thu, Jul 16, 2020 at 02:37:04PM +0200, Philippe Mathieu-Daudé wrote: >> Let blk_attach_dev() take an Error* object to return helpful >> information. Adapt the callers. >> >> $ qemu-system-arm -M n800 >> qemu-system-arm: sd_init failed: cannot attach blk 'sd0' to device >> 'sd-card' >> blk 'sd0' is already attached by device 'omap2-mmc' >> Drive 'sd0' is already in use because it has been automatically connected >> to another device >> (do you need 'if=none' in the drive options?) >> >> Signed-off-by: Philippe Mathieu-Daudé >> --- >> v2: Rebased after 668f62ec62 ("error: Eliminate error_propagate()") >> --- >> include/sysemu/block-backend.h | 2 +- >> block/block-backend.c| 11 ++- >> hw/block/fdc.c | 4 +--- >> hw/block/swim.c | 4 +--- >> hw/block/xen-block.c | 5 +++-- >> hw/core/qdev-properties-system.c | 16 +--- >> hw/ide/qdev.c| 4 +--- >> hw/scsi/scsi-disk.c | 4 +--- >> 8 files changed, 27 insertions(+), 23 deletions(-) >> >> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h >> index 8203d7f6f9..118fbad0b4 100644 >> --- a/include/sysemu/block-backend.h >> +++ b/include/sysemu/block-backend.h >> @@ -113,7 +113,7 @@ BlockDeviceIoStatus blk_iostatus(const BlockBackend >> *blk); >> void blk_iostatus_disable(BlockBackend *blk); >> void blk_iostatus_reset(BlockBackend *blk); >> void blk_iostatus_set_err(BlockBackend *blk, int error); >> -int blk_attach_dev(BlockBackend *blk, DeviceState *dev); >> +int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp); >> void blk_detach_dev(BlockBackend *blk, DeviceState *dev); >> DeviceState *blk_get_attached_dev(BlockBackend *blk); >> char *blk_get_attached_dev_id(BlockBackend *blk); >> diff --git a/block/block-backend.c b/block/block-backend.c >> index 63ff940ef9..b7be0a4619 100644 >> --- a/block/block-backend.c >> +++ b/block/block-backend.c >> @@ -884,12 +884,21 @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm, >> uint64_t *shared_perm) >> >> /* >> * Attach device model @dev to @blk. >> + * >> + * @blk: Block backend >> + * @dev: Device to attach the block backend to >> + * @errp: pointer to NULL initialized error object >> + * >> * Return 0 on success, -EBUSY when a device model is attached already. >> */ >> -int blk_attach_dev(BlockBackend *blk, DeviceState *dev) >> +int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp) >> { >> trace_blk_attach_dev(blk_name(blk), object_get_typename(OBJECT(dev))); >> if (blk->dev) { >> +error_setg(errp, "cannot attach blk '%s' to device '%s'", >> + blk_name(blk), object_get_typename(OBJECT(dev))); >> +error_append_hint(errp, "blk '%s' is already attached by device >> '%s'\n", >> + blk_name(blk), >> object_get_typename(OBJECT(blk->dev))); > > I would have a preference for expanding the main error message and not > using a hint. Any hint is completely thrown away when using QMP :-( Hints work best in cases like error message hint suggesting things to try to fix it, in CLI syntax error message rejecting a configuration value hint listing possible values, in CLI syntax Why "in CLI syntax"? Well, we need to use *some* syntax to be useful. Hints have always been phrased for the CLI, simply because the hint feature grew out of my disgust over the loss of lovingly written CLI hints in the conversion to Error. Hints in CLI syntax would be misleading QMP. We never extended QMP to transport hints. Hints may tempt you in a case like error message that is painfully long, because it really tries hard to explain, which is laudable in theory, but sadly illegible in practice; also, interminable sentences, meh, this is an error message, not a Joyce novel to get something like terse error message Explanation that is rather long, because it really tries hard to explain. It can be multiple sentences, and lines are wrapped at a reasonable length. Comes out okay in the CLI, but the explanation is lost in QMP. I don't have a good solution to offer for errors that genuinely need explaining.
Re: sysbus_create_simple Vs qdev_create
Eduardo Habkost writes: > I'd also note that the use of "parent" in the code is also > ambiguous. It can mean: > > * QOM parent type, i.e. TypeInfo.parent. Related fields: > * parent_class members of class structs > * parent_obj members of object structs I hate the use of "parent" and "child" for a super- / subtype relation. Correcting the terminology there would be short term pain for long term gain. Worthwhile? > * QOM composition tree parent object, i.e. Object::parent > * qdev device parent bus, i.e. DeviceState::parent_bus > * parent device of of qdev bus, i.e. BusState::parent These are tree relations. Use of "parent" and "child" is perfectly fine.
Re: [PATCH] net: check payload length limit for all frames
Hello Jason, all +-- On Fri, 17 Jul 2020, Jason Wang wrote --+ | On 2020/7/17 上午9:21, Alexander Bulekov wrote: | > On 200717 0853, Li Qiang wrote: | >> Which issue are you trying to solve, any reference linking? | >> I also send a patch related this part and also a UAF. | > | > I reported a UAF privately to QEMU-Security in May. I believe the one Li | > is referring to is this one https://bugs.launchpad.net/qemu/+bug/1886362 | > | > When I saw Prasad's email, I was worried that I reported the same bug | > twice, but I can still reproduce LP#1886362 with Prasad's patch. | > | > On the other hand, I cannot reproduce either issue with Li's patch: | > Message-Id: <20200716161453.61295-1-liq...@163.com> | > | > Based on this, I think there were two distinct issues. Yes, LP#1886362 looks different that the one fixed here. | Could you describe the issue you saw in details? (E.g the calltrace?) The | commit log does not explain where we can get OOB or UAF. I should've included the backtrace in the commit message. It crossed my mind after I sent the patch. Sorry. === 1040323==ERROR: AddressSanitizer: heap-use-after-free on address 0x606344d8 at pc 0x5571b8fb9ce7 bp 0x7ffede5a2290 sp 0x7ffede5a2280 READ of size 8 at 0x606344d8 thread T0 #0 0x5571b8fb9ce6 in e1000e_write_packet_to_guest hw/net/e1000e_core.c:1587 #1 0x5571b8fba8fc in e1000e_receive_iov hw/net/e1000e_core.c:1709 #2 0x5571b8f9cb08 in e1000e_nc_receive_iov hw/net/e1000e.c:213 #3 0x5571b8f90285 in net_tx_pkt_sendv hw/net/net_tx_pkt.c:544 #4 0x5571b8f90aaa in net_tx_pkt_send hw/net/net_tx_pkt.c:620 #5 0x5571b8f90b2e in net_tx_pkt_send_loopback hw/net/net_tx_pkt.c:633 #6 0x5571b8fb3c3b in e1000e_tx_pkt_send hw/net/e1000e_core.c:664 #7 0x5571b8fb43dd in e1000e_process_tx_desc hw/net/e1000e_core.c:743 #8 0x5571b8fb5b4f in e1000e_start_xmit hw/net/e1000e_core.c:934 #9 0x5571b8fbe2ad in e1000e_set_tdt hw/net/e1000e_core.c:2451 #10 0x5571b8fc01e0 in e1000e_core_write hw/net/e1000e_core.c:3265 #11 0x5571b8f9c387 in e1000e_mmio_write hw/net/e1000e.c:109 ... 0x606344d8 is located 24 bytes inside of 64-byte region [0x606344c0,0x60634500) freed by thread T0 here: #0 0x7f89e3b87307 in __interceptor_free (/lib64/libasan.so.6+0xb0307) #1 0x7f89e37c7a6c in g_free (/lib64/libglib-2.0.so.0+0x58a6c) #2 0x5571b8f95fc7 in net_rx_pkt_pull_data hw/net/net_rx_pkt.c:103 #3 0x5571b8f969b3 in net_rx_pkt_attach_iovec_ex hw/net/net_rx_pkt.c:158 #4 0x5571b8fba6bc in e1000e_receive_iov hw/net/e1000e_core.c:1695 #5 0x5571b8f9cb08 in e1000e_nc_receive_iov hw/net/e1000e.c:213 #6 0x5571b8f90285 in net_tx_pkt_sendv hw/net/net_tx_pkt.c:544 #7 0x5571b8f90aaa in net_tx_pkt_send hw/net/net_tx_pkt.c:620 #8 0x5571b8f90b2e in net_tx_pkt_send_loopback hw/net/net_tx_pkt.c:633 #9 0x5571b8fb3c3b in e1000e_tx_pkt_send hw/net/e1000e_core.c:664 #10 0x5571b8fb43dd in e1000e_process_tx_desc hw/net/e1000e_core.c:743 #11 0x5571b8fb5b4f in e1000e_start_xmit hw/net/e1000e_core.c:934 #12 0x5571b8fbe011 in e1000e_set_tctl hw/net/e1000e_core.c:2431 #13 0x5571b8fc01e0 in e1000e_core_write hw/net/e1000e_core.c:3265 #14 0x5571b8f9c387 in e1000e_mmio_write hw/net/e1000e.c:109 ... previously allocated by thread T0 here: #0 0x7f89e3b87667 in __interceptor_malloc (/lib64/libasan.so.6+0xb0667) #1 0x7f89e37c7978 in g_malloc (/lib64/libglib-2.0.so.0+0x58978) #2 0x5571b8f95fc7 in net_rx_pkt_pull_data hw/net/net_rx_pkt.c:103 #3 0x5571b8f969b3 in net_rx_pkt_attach_iovec_ex hw/net/net_rx_pkt.c:158 #4 0x5571b8fba6bc in e1000e_receive_iov hw/net/e1000e_core.c:1695 #5 0x5571b8f9cb08 in e1000e_nc_receive_iov hw/net/e1000e.c:213 #6 0x5571b8f90285 in net_tx_pkt_sendv hw/net/net_tx_pkt.c:544 #7 0x5571b8f90aaa in net_tx_pkt_send hw/net/net_tx_pkt.c:620 #8 0x5571b8f90b2e in net_tx_pkt_send_loopback hw/net/net_tx_pkt.c:633 #9 0x5571b8fb3c3b in e1000e_tx_pkt_send hw/net/e1000e_core.c:664 #10 0x5571b8fb43dd in e1000e_process_tx_desc hw/net/e1000e_core.c:743 #11 0x5571b8fb5b4f in e1000e_start_xmit hw/net/e1000e_core.c:934 #12 0x5571b8fbe2ad in e1000e_set_tdt hw/net/e1000e_core.c:2451 #13 0x5571b8fc01e0 in e1000e_core_write hw/net/e1000e_core.c:3265 #14 0x5571b8f9c387 in e1000e_mmio_write hw/net/e1000e.c:109 === | >>> --- a/hw/net/net_tx_pkt.c | >>> +++ b/hw/net/net_tx_pkt.c | >>> @@ -607,12 +607,10 @@ bool net_tx_pkt_send(struct NetTxPkt *pkt, | >>> NetClientState *nc) | >>>* Since underlying infrastructure does not support IP datagrams | >>>longer | >>>* than 64K we should drop such packets and don't even try to send | >>>*/ | >>> -if (VIRTIO_NET_HDR_GSO_NONE != pkt->virt_hdr.gso_type) { | >>> -if (pkt->payload_len > | >>> -ETH_MAX_IP_DGRAM_LEN - | >>> -pkt->vec[NET_TX_PKT_L3HDR_FRAG].iov_len) { | >>> -return false; | >>> -} | >>> +if (pkt->p
Re: [PATCH] e1000e: using bottom half to send packets
Jason Wang 于2020年7月17日周五 上午11:10写道: > > > On 2020/7/17 上午12:14, Li Qiang wrote: > > Alexander Bulekov reported a UAF bug related e1000e packets send. > > > > -->https://bugs.launchpad.net/qemu/+bug/1886362 > > > > This is because the guest trigger a e1000e packet send and set the > > data's address to e1000e's MMIO address. So when the e1000e do DMA > > it will write the MMIO again and trigger re-entrancy and finally > > causes this UAF. > > > > Paolo suggested to use a bottom half whenever MMIO is doing complicate > > things in here: > > -->https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg03342.html > > > > Reference here: > > 'The easiest solution is to delay processing of descriptors to a bottom > > half whenever MMIO is doing something complicated. This is also better > > for latency because it will free the vCPU thread more quickly and leave > > the work to the I/O thread.' > > > I think several things were missed in this patch (take virtio-net as a > reference), do we need the following things: > Thanks Jason, In fact I know this, I'm scared for touching this but I want to try. Thanks for your advice. > - Cancel the bh when VM is stopped. Ok. I think add a vm state change notifier for e1000e can address this. > - A throttle to prevent bh from executing too much timer? Ok, I think add a config timeout and add a timer in e1000e can address this. > - A flag to record whether or not this a pending tx (and migrate it?) Is just a flag enough? Could you explain more about the idea behind processing the virtio-net/e1000e using bh like this? For example, if the guest trigger a lot of packets send and if the bh is scheduled in IO thread. So will we lost packets? How we avoid this in virtio-net. Thanks, Li Qiang > > Thanks > > > > > > This patch fixes this UAF. > > > > Signed-off-by: Li Qiang > > --- > > hw/net/e1000e_core.c | 25 + > > hw/net/e1000e_core.h | 2 ++ > > 2 files changed, 19 insertions(+), 8 deletions(-) > > > > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c > > index bcd186cac5..6165b04b68 100644 > > --- a/hw/net/e1000e_core.c > > +++ b/hw/net/e1000e_core.c > > @@ -2423,32 +2423,27 @@ e1000e_set_dbal(E1000ECore *core, int index, > > uint32_t val) > > static void > > e1000e_set_tctl(E1000ECore *core, int index, uint32_t val) > > { > > -E1000E_TxRing txr; > > core->mac[index] = val; > > > > if (core->mac[TARC0] & E1000_TARC_ENABLE) { > > -e1000e_tx_ring_init(core, &txr, 0); > > -e1000e_start_xmit(core, &txr); > > +qemu_bh_schedule(core->tx[0].tx_bh); > > } > > > > if (core->mac[TARC1] & E1000_TARC_ENABLE) { > > -e1000e_tx_ring_init(core, &txr, 1); > > -e1000e_start_xmit(core, &txr); > > +qemu_bh_schedule(core->tx[1].tx_bh); > > } > > } > > > > static void > > e1000e_set_tdt(E1000ECore *core, int index, uint32_t val) > > { > > -E1000E_TxRing txr; > > int qidx = e1000e_mq_queue_idx(TDT, index); > > uint32_t tarc_reg = (qidx == 0) ? TARC0 : TARC1; > > > > core->mac[index] = val & 0x; > > > > if (core->mac[tarc_reg] & E1000_TARC_ENABLE) { > > -e1000e_tx_ring_init(core, &txr, qidx); > > -e1000e_start_xmit(core, &txr); > > +qemu_bh_schedule(core->tx[qidx].tx_bh); > > } > > } > > > > @@ -3322,6 +3317,16 @@ e1000e_vm_state_change(void *opaque, int running, > > RunState state) > > } > > } > > > > +static void e1000e_core_tx_bh(void *opaque) > > +{ > > +struct e1000e_tx *tx = opaque; > > +E1000ECore *core = tx->core; > > +E1000E_TxRing txr; > > + > > +e1000e_tx_ring_init(core, &txr, tx - &core->tx[0]); > > +e1000e_start_xmit(core, &txr); > > +} > > + > > void > > e1000e_core_pci_realize(E1000ECore *core, > > const uint16_t *eeprom_templ, > > @@ -3340,6 +3345,8 @@ e1000e_core_pci_realize(E1000ECore *core, > > for (i = 0; i < E1000E_NUM_QUEUES; i++) { > > net_tx_pkt_init(&core->tx[i].tx_pkt, core->owner, > > E1000E_MAX_TX_FRAGS, core->has_vnet); > > +core->tx[i].core = core; > > +core->tx[i].tx_bh = qemu_bh_new(e1000e_core_tx_bh, &core->tx[i]); > > } > > > > net_rx_pkt_init(&core->rx_pkt, core->has_vnet); > > @@ -3367,6 +3374,8 @@ e1000e_core_pci_uninit(E1000ECore *core) > > for (i = 0; i < E1000E_NUM_QUEUES; i++) { > > net_tx_pkt_reset(core->tx[i].tx_pkt); > > net_tx_pkt_uninit(core->tx[i].tx_pkt); > > +qemu_bh_delete(core->tx[i].tx_bh); > > +core->tx[i].tx_bh = NULL; > > } > > > > net_rx_pkt_uninit(core->rx_pkt); > > diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h > > index aee32f7e48..94ddc6afc2 100644 > > --- a/hw/net/e1000e_core.h > > +++ b/hw/net/e1000e_core.h > > @@ -77,6 +77,8 @@ struct E1000Core { > > unsigned char sum_needed; > > bool cptse; > > struct N
[PATCH] Fix vhost-user buffer over-read on ram hot-unplug
The VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS vhost-user protocol feature introduced a shadow-table, used by the backend to dynamically determine how a vdev's memory regions have changed since the last vhost_user_set_mem_table() call. On hot-remove, a memmove() operation is used to overwrite the removed shadow region descriptor(s). The size parameter of this memmove was off by 1 such that if a VM with a backend supporting the VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS filled it's shadow-table (by performing the maximum number of supported hot-add operatons) and attempted to remove the last region, Qemu would read an out of bounds value and potentially crash. This change fixes the memmove() bounds such that this erroneous read can never happen. Signed-off-by: Peter Turschmid Signed-off-by: Raphael Norwitz --- hw/virtio/vhost-user.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 3123121..d7e2423 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -672,7 +672,7 @@ static int send_remove_regions(struct vhost_dev *dev, memmove(&u->shadow_regions[shadow_reg_idx], &u->shadow_regions[shadow_reg_idx + 1], sizeof(struct vhost_memory_region) * -(u->num_shadow_regions - shadow_reg_idx)); +(u->num_shadow_regions - shadow_reg_idx - 1)); u->num_shadow_regions--; } -- 1.8.3.1
Re: [virtio-dev] [RFC for Linux v4 0/2] virtio_balloon: Add VIRTIO_BALLOON_F_CONT_PAGES to report continuous pages
> 2020年7月16日 18:45,Michael S. Tsirkin 写道: > > On Thu, Jul 16, 2020 at 03:01:18PM +0800, teawater wrote: >> >> >>> 2020年7月16日 14:38,Michael S. Tsirkin 写道: >>> >>> On Thu, Jul 16, 2020 at 10:41:50AM +0800, Hui Zhu wrote: The first, second and third version are in [1], [2] and [3]. Code of current version for Linux and qemu is available in [4] and [5]. Update of this version: 1. Report continuous pages will increase the speed. So added deflate continuous pages. 2. According to the comments from David in [6], added 2 new vqs inflate_cont_vq and deflate_cont_vq to report continuous pages with format 32 bits pfn and 32 bits size. Following is the introduction of the function. These patches add VIRTIO_BALLOON_F_CONT_PAGES to virtio_balloon. With this flag, balloon tries to use continuous pages to inflate and deflate. Opening this flag can bring two benefits: 1. Report continuous pages will increase memory report size of each time call tell_host. Then it will increase the speed of balloon inflate and deflate. 2. Host THPs will be splitted when qemu release the page of balloon inflate. Inflate balloon with continuous pages will let QEMU release the pages of same THPs. That will help decrease the splitted THPs number in the host. Following is an example in a VM with 1G memory 1CPU. This test setups an environment that has a lot of fragmentation pages. Then inflate balloon will split the THPs. >> >> // This is the THP number before VM execution in the host. // None use THP. cat /proc/meminfo | grep AnonHugePages: AnonHugePages: 0 kB >> These lines are from host. >> // After VM start, use usemem // (https://git.kernel.org/pub/scm/linux/kernel/git/wfg/vm-scalability.git) // punch-holes function generates 400m fragmentation pages in the guest // kernel. usemem --punch-holes -s -1 800m & >> These lines are from guest. They setups the environment that has a lot of >> fragmentation pages. >> // This is the THP number after this command in the host. // Some THP is used by VM because usemem will access 800M memory // in the guest. cat /proc/meminfo | grep AnonHugePages: AnonHugePages:911360 kB >> These lines are from host. >> // Connect to the QEMU monitor, setup balloon, and set it size to 600M. (qemu) device_add virtio-balloon-pci,id=balloon1 (qemu) info balloon balloon: actual=1024 (qemu) balloon 600 (qemu) info balloon balloon: actual=600 >> These lines are from host. >> // This is the THP number after inflate the balloon in the host. cat /proc/meminfo | grep AnonHugePages: AnonHugePages: 88064 kB >> These lines are from host. >> // Set the size back to 1024M in the QEMU monitor. (qemu) balloon 1024 (qemu) info balloon balloon: actual=1024 >> These lines are from host. >> // Use usemem to increase the memory usage of QEMU. killall usemem usemem 800m >> These lines are from guest. >> // This is the THP number after this operation. cat /proc/meminfo | grep AnonHugePages: AnonHugePages: 65536 kB >> These lines are from host. >> >> >> Following example change to use continuous pages balloon. The number of splitted THPs is decreased. // This is the THP number before VM execution in the host. // None use THP. cat /proc/meminfo | grep AnonHugePages: AnonHugePages: 0 kB >> These lines are from host. >> // After VM start, use usemem punch-holes function generates 400M // fragmentation pages in the guest kernel. usemem --punch-holes -s -1 800m & >> These lines are from guest. They setups the environment that has a lot of >> fragmentation pages. >> // This is the THP number after this command in the host. // Some THP is used by VM because usemem will access 800M memory // in the guest. cat /proc/meminfo | grep AnonHugePages: AnonHugePages:911360 kB >> These lines are from host. >> // Connect to the QEMU monitor, setup balloon, and set it size to 600M. (qemu) device_add virtio-balloon-pci,id=balloon1,cont-pages=on (qemu) info balloon balloon: actual=1024 (qemu) balloon 600 (qemu) info balloon balloon: actual=600 >> These lines are from host. >> // This is the THP number after inflate the balloon in the host. cat /proc/meminfo | grep AnonHugePages: AnonHugePages:616448 kB // Set the size back to 1024M in the QEMU monitor. (qemu) balloon 1024 (qemu) info balloon balloon: actual=1024 >> These lines are from host. >> // Use usemem to increase the memory usage of QEMU. killall usemem usemem 800m >> These lines are from guest. >> // This is the THP number after this operation. cat /proc/mem
[PULL SUBSYSTEM qemu-pseries] pseries: Update SLOF firmware image
The following changes since commit 1038a309ec829f05a3a3e52a9951cfdb24dfd02c: spapr: Add a new level of NUMA for GPUs (2020-07-17 10:36:28 +1000) are available in the Git repository at: g...@github.com:aik/qemu.git tags/qemu-slof-20200717 for you to fetch changes up to 7f5258dd8327d574de455a2271788474fa25548d: pseries: Update SLOF firmware image (2020-07-17 13:23:00 +1000) Alexey Kardashevskiy (1): pseries: Update SLOF firmware image pc-bios/README | 2 +- pc-bios/slof.bin | Bin 965112 -> 968368 bytes roms/SLOF| 2 +- 3 files changed, 2 insertions(+), 2 deletions(-) *** Note: this is not for master, this is for pseries This adds tcgbios (this was posted earlier [1] but got lost) and fixes FDT update at ibm,client-architecture-support for huge guests. The full list of changes: Alexey Kardashevskiy (4): make: Define default rule for .c when V=1 or V=2 version: update to 20200513 fdt: Avoid recursion when traversing tree version: update to 20200717 Gustavo Romero (1): board-qemu: Fix comment about SLOF start address Stefan Berger (6): tcgbios: Only write logs for PCRs that are allocated tcgbios: Fix the vendorInfoSize to be of type uint8_t tcgbios: Add support for SHA3 type of algorithms elf: Implement elf_get_file_size to determine size of an ELF image tcgbios: Implement tpm_hash_log_extend_event_buffer tcgbios: Measure the bootloader file read from disk [1] https://patchwork.ozlabs.org/project/qemu-devel/patch/20200513024355.121476-1-...@ozlabs.ru/
Re: [PATCH] net: check payload length limit for all frames
On 2020/7/17 上午9:21, Alexander Bulekov wrote: On 200717 0853, Li Qiang wrote: P J P 于2020年7月17日周五 上午3:26写道: From: Prasad J Pandit While sending packets, the check that packet 'payload_len' is within 64kB limit, seems to happen only for GSO frames. It may lead to use-after-free or out-of-bounds access like issues when sending non-GSO frames. Check the 'payload_len' limit for all packets, irrespective of the gso type. Hello Prasad, Which issue are you trying to solve, any reference linking? I also send a patch related this part and also a UAF. Thanks, Li Qiang Hi Li, Prasad, I reported a UAF privately to QEMU-Security in May. I believe the one Li is referring to is this one https://bugs.launchpad.net/qemu/+bug/1886362 When I saw Prasad's email, I was worried that I reported the same bug twice, but I can still reproduce LP#1886362 with Prasad's patch. On the other hand, I cannot reproduce either issue with Li's patch: Message-Id: <20200716161453.61295-1-liq...@163.com> Based on this, I think there were two distinct issues. Both of the crashes rely on e1000e tx loopback into e1000e MMIO. Since Li's patch adds a TX bh, it seems to mitigate such types of issues. Sorry about any confusion. -Alex Could you describe the issue you saw in details? (E.g the calltrace?) The commit log does not explain where we can get OOB or UAF. Thanks Reported-by: Alexander Bulekov Signed-off-by: Prasad J Pandit --- hw/net/net_tx_pkt.c | 10 -- 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c index 162f802dd7..e66998a8f9 100644 --- a/hw/net/net_tx_pkt.c +++ b/hw/net/net_tx_pkt.c @@ -607,12 +607,10 @@ bool net_tx_pkt_send(struct NetTxPkt *pkt, NetClientState *nc) * Since underlying infrastructure does not support IP datagrams longer * than 64K we should drop such packets and don't even try to send */ -if (VIRTIO_NET_HDR_GSO_NONE != pkt->virt_hdr.gso_type) { -if (pkt->payload_len > -ETH_MAX_IP_DGRAM_LEN - -pkt->vec[NET_TX_PKT_L3HDR_FRAG].iov_len) { -return false; -} +if (pkt->payload_len > +ETH_MAX_IP_DGRAM_LEN - +pkt->vec[NET_TX_PKT_L3HDR_FRAG].iov_len) { +return false; } if (pkt->has_virt_hdr || -- 2.26.2
Re: [PATCH] e1000e: using bottom half to send packets
On 2020/7/17 上午12:14, Li Qiang wrote: Alexander Bulekov reported a UAF bug related e1000e packets send. -->https://bugs.launchpad.net/qemu/+bug/1886362 This is because the guest trigger a e1000e packet send and set the data's address to e1000e's MMIO address. So when the e1000e do DMA it will write the MMIO again and trigger re-entrancy and finally causes this UAF. Paolo suggested to use a bottom half whenever MMIO is doing complicate things in here: -->https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg03342.html Reference here: 'The easiest solution is to delay processing of descriptors to a bottom half whenever MMIO is doing something complicated. This is also better for latency because it will free the vCPU thread more quickly and leave the work to the I/O thread.' I think several things were missed in this patch (take virtio-net as a reference), do we need the following things: - Cancel the bh when VM is stopped. - A throttle to prevent bh from executing too much timer? - A flag to record whether or not this a pending tx (and migrate it?) Thanks This patch fixes this UAF. Signed-off-by: Li Qiang --- hw/net/e1000e_core.c | 25 + hw/net/e1000e_core.h | 2 ++ 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c index bcd186cac5..6165b04b68 100644 --- a/hw/net/e1000e_core.c +++ b/hw/net/e1000e_core.c @@ -2423,32 +2423,27 @@ e1000e_set_dbal(E1000ECore *core, int index, uint32_t val) static void e1000e_set_tctl(E1000ECore *core, int index, uint32_t val) { -E1000E_TxRing txr; core->mac[index] = val; if (core->mac[TARC0] & E1000_TARC_ENABLE) { -e1000e_tx_ring_init(core, &txr, 0); -e1000e_start_xmit(core, &txr); +qemu_bh_schedule(core->tx[0].tx_bh); } if (core->mac[TARC1] & E1000_TARC_ENABLE) { -e1000e_tx_ring_init(core, &txr, 1); -e1000e_start_xmit(core, &txr); +qemu_bh_schedule(core->tx[1].tx_bh); } } static void e1000e_set_tdt(E1000ECore *core, int index, uint32_t val) { -E1000E_TxRing txr; int qidx = e1000e_mq_queue_idx(TDT, index); uint32_t tarc_reg = (qidx == 0) ? TARC0 : TARC1; core->mac[index] = val & 0x; if (core->mac[tarc_reg] & E1000_TARC_ENABLE) { -e1000e_tx_ring_init(core, &txr, qidx); -e1000e_start_xmit(core, &txr); +qemu_bh_schedule(core->tx[qidx].tx_bh); } } @@ -3322,6 +3317,16 @@ e1000e_vm_state_change(void *opaque, int running, RunState state) } } +static void e1000e_core_tx_bh(void *opaque) +{ +struct e1000e_tx *tx = opaque; +E1000ECore *core = tx->core; +E1000E_TxRing txr; + +e1000e_tx_ring_init(core, &txr, tx - &core->tx[0]); +e1000e_start_xmit(core, &txr); +} + void e1000e_core_pci_realize(E1000ECore *core, const uint16_t *eeprom_templ, @@ -3340,6 +3345,8 @@ e1000e_core_pci_realize(E1000ECore *core, for (i = 0; i < E1000E_NUM_QUEUES; i++) { net_tx_pkt_init(&core->tx[i].tx_pkt, core->owner, E1000E_MAX_TX_FRAGS, core->has_vnet); +core->tx[i].core = core; +core->tx[i].tx_bh = qemu_bh_new(e1000e_core_tx_bh, &core->tx[i]); } net_rx_pkt_init(&core->rx_pkt, core->has_vnet); @@ -3367,6 +3374,8 @@ e1000e_core_pci_uninit(E1000ECore *core) for (i = 0; i < E1000E_NUM_QUEUES; i++) { net_tx_pkt_reset(core->tx[i].tx_pkt); net_tx_pkt_uninit(core->tx[i].tx_pkt); +qemu_bh_delete(core->tx[i].tx_bh); +core->tx[i].tx_bh = NULL; } net_rx_pkt_uninit(core->rx_pkt); diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h index aee32f7e48..94ddc6afc2 100644 --- a/hw/net/e1000e_core.h +++ b/hw/net/e1000e_core.h @@ -77,6 +77,8 @@ struct E1000Core { unsigned char sum_needed; bool cptse; struct NetTxPkt *tx_pkt; +QEMUBH *tx_bh; +E1000ECore *core; } tx[E1000E_NUM_QUEUES]; struct NetRxPkt *rx_pkt;
Re: [PATCH] net: check payload length limit for all frames
On 200717 0853, Li Qiang wrote: > P J P 于2020年7月17日周五 上午3:26写道: > > > > From: Prasad J Pandit > > > > While sending packets, the check that packet 'payload_len' > > is within 64kB limit, seems to happen only for GSO frames. > > It may lead to use-after-free or out-of-bounds access like > > issues when sending non-GSO frames. Check the 'payload_len' > > limit for all packets, irrespective of the gso type. > > > > Hello Prasad, > Which issue are you trying to solve, any reference linking? > > I also send a patch related this part and also a UAF. > > Thanks, > Li Qiang Hi Li, Prasad, I reported a UAF privately to QEMU-Security in May. I believe the one Li is referring to is this one https://bugs.launchpad.net/qemu/+bug/1886362 When I saw Prasad's email, I was worried that I reported the same bug twice, but I can still reproduce LP#1886362 with Prasad's patch. On the other hand, I cannot reproduce either issue with Li's patch: Message-Id: <20200716161453.61295-1-liq...@163.com> Based on this, I think there were two distinct issues. Both of the crashes rely on e1000e tx loopback into e1000e MMIO. Since Li's patch adds a TX bh, it seems to mitigate such types of issues. Sorry about any confusion. -Alex > > Reported-by: Alexander Bulekov > > Signed-off-by: Prasad J Pandit > > --- > > hw/net/net_tx_pkt.c | 10 -- > > 1 file changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c > > index 162f802dd7..e66998a8f9 100644 > > --- a/hw/net/net_tx_pkt.c > > +++ b/hw/net/net_tx_pkt.c > > @@ -607,12 +607,10 @@ bool net_tx_pkt_send(struct NetTxPkt *pkt, > > NetClientState *nc) > > * Since underlying infrastructure does not support IP datagrams longer > > * than 64K we should drop such packets and don't even try to send > > */ > > -if (VIRTIO_NET_HDR_GSO_NONE != pkt->virt_hdr.gso_type) { > > -if (pkt->payload_len > > > -ETH_MAX_IP_DGRAM_LEN - > > -pkt->vec[NET_TX_PKT_L3HDR_FRAG].iov_len) { > > -return false; > > -} > > +if (pkt->payload_len > > > +ETH_MAX_IP_DGRAM_LEN - > > +pkt->vec[NET_TX_PKT_L3HDR_FRAG].iov_len) { > > +return false; > > } > > > > if (pkt->has_virt_hdr || > > -- > > 2.26.2 > > > >
[PATCH] usb: only build hcd-dwc2 host controller for RASPI target
The hcd-dwc2 host controller is currently built for all targets. Since for now hcd-dwc2 is only implemented on RASPI, restrict its build to that target only. Signed-off-by: Paul Zimmerman --- Hi Gerd, Do we want to apply this before the 5.1.0 release? It seems a waste to build this code for every target when it's only used on one. Sorry I didn't realize this earlier. Thanks, Paul hw/arm/Kconfig | 1 + hw/usb/Kconfig | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig index 4a224a6351..bc3a423940 100644 --- a/hw/arm/Kconfig +++ b/hw/arm/Kconfig @@ -315,6 +315,7 @@ config RASPI select FRAMEBUFFER select PL011 # UART select SDHCI +select USB_DWC2 config STM32F205_SOC bool diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig index d4d8c37c28..5e63dc75f8 100644 --- a/hw/usb/Kconfig +++ b/hw/usb/Kconfig @@ -48,7 +48,6 @@ config USB_MUSB config USB_DWC2 bool -default y select USB config TUSB6010 -- 2.17.1
Re: [PATCH] net: check payload length limit for all frames
P J P 于2020年7月17日周五 上午3:26写道: > > From: Prasad J Pandit > > While sending packets, the check that packet 'payload_len' > is within 64kB limit, seems to happen only for GSO frames. > It may lead to use-after-free or out-of-bounds access like > issues when sending non-GSO frames. Check the 'payload_len' > limit for all packets, irrespective of the gso type. > Hello Prasad, Which issue are you trying to solve, any reference linking? I also send a patch related this part and also a UAF. Thanks, Li Qiang > Reported-by: Alexander Bulekov > Signed-off-by: Prasad J Pandit > --- > hw/net/net_tx_pkt.c | 10 -- > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c > index 162f802dd7..e66998a8f9 100644 > --- a/hw/net/net_tx_pkt.c > +++ b/hw/net/net_tx_pkt.c > @@ -607,12 +607,10 @@ bool net_tx_pkt_send(struct NetTxPkt *pkt, > NetClientState *nc) > * Since underlying infrastructure does not support IP datagrams longer > * than 64K we should drop such packets and don't even try to send > */ > -if (VIRTIO_NET_HDR_GSO_NONE != pkt->virt_hdr.gso_type) { > -if (pkt->payload_len > > -ETH_MAX_IP_DGRAM_LEN - > -pkt->vec[NET_TX_PKT_L3HDR_FRAG].iov_len) { > -return false; > -} > +if (pkt->payload_len > > +ETH_MAX_IP_DGRAM_LEN - > +pkt->vec[NET_TX_PKT_L3HDR_FRAG].iov_len) { > +return false; > } > > if (pkt->has_virt_hdr || > -- > 2.26.2 > >
Re: [PATCH] spapr_pci: Robustify support of PCI bridges
On Thu, Jul 16, 2020 at 04:57:54PM +0200, Greg Kurz wrote: > On Thu, 16 Jul 2020 16:23:52 +0200 > Markus Armbruster wrote: > > > David Gibson writes: > > > > > On Thu, Jul 16, 2020 at 12:32:44PM +0200, Greg Kurz wrote: > > >> On Thu, 16 Jul 2020 14:45:40 +1000 > > >> David Gibson wrote: > > >> > > >> > On Thu, Jul 09, 2020 at 07:12:47PM +0200, Greg Kurz wrote: > > >> > > Some recent error handling cleanups unveiled issues with our support > > >> > > of > > >> > > PCI bridges: > > >> > > > > >> > > 1) QEMU aborts when using non-standard PCI bridge types, > > >> > >unveiled by commit 7ef1553dac "spapr_pci: Drop some dead error > > >> > > handling" > > >> > > > > >> > > $ qemu-system-ppc64 -M pseries -device pcie-pci-bridge > > >> > > Unexpected error in object_property_find() at qom/object.c:1240: > > >> > > qemu-system-ppc64: -device pcie-pci-bridge: Property '.chassis_nr' > > >> > > not found > > >> > > Aborted (core dumped) > > >> > > > >> > Oops, I thought we had a check that we actually had a "pci-bridge" > > >> > device before continuing with the hotplug, but I guess not. > > >> > > >> Ah... are you suggesting we should explicitly check the actual type > > >> of the bridge rather than looking for the "chassis_nr" property ? > > > > > > Uh.. I thought about it, but I don't think it matters much which way > > > we do it. > > > > Would it make sense to add the "chassis_nr" property to *all* PCI > > bridge devices? > > > > I see that the "PCI Express to PCI/PCI-X Bridge Specification" mentions > a "Chassis Number Register" which looks very similar to the what exists > in standard PCI-to-PCI brdiges. This doesn't seem to be implemented in > our "pcie-pci-bridge" device model though, but of course I have no idea > why :) We could consider it, but I don't think there's a lot to be gained by it at this stage. I don't think there's really any reason to want to use bridges other than plain "pci-bridge" on the pseries machine. PCI is a bit weird on pseries, since it's explicitly paravirt. Although you can use extended config space, and thereby PCI-E devices on it, the topology really looks pretty much identical to vanilla PCI. So, I don't think there's any reason to use PCI-E bridges on pseries. Other than PCI-E bridges of various sorts, a quick scan suggests all the other bridge types in qemu are weird variants that are mostly specific to some particular platform. I don't see any reason we'd want those on pseries either. > Maybe Michael or Marcel (cc'd) can share some thoughts about that ? -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH] spapr_pci: Robustify support of PCI bridges
On Thu, Jul 16, 2020 at 04:42:00PM +0200, Greg Kurz wrote: > On Thu, 16 Jul 2020 16:01:18 +0200 > Markus Armbruster wrote: > > > David Gibson writes: > > > > > On Thu, Jul 09, 2020 at 07:12:47PM +0200, Greg Kurz wrote: > > >> Some recent error handling cleanups unveiled issues with our support of > > >> PCI bridges: > > >> > > >> 1) QEMU aborts when using non-standard PCI bridge types, > > >>unveiled by commit 7ef1553dac "spapr_pci: Drop some dead error > > >> handling" > > >> > > >> $ qemu-system-ppc64 -M pseries -device pcie-pci-bridge > > >> Unexpected error in object_property_find() at qom/object.c:1240: > > >> qemu-system-ppc64: -device pcie-pci-bridge: Property '.chassis_nr' not > > >> found > > >> Aborted (core dumped) > > > > > > Oops, I thought we had a check that we actually had a "pci-bridge" > > > device before continuing with the hotplug, but I guess not. > > > > > >> This happens because we assume all PCI bridge types to have a > > >> "chassis_nr" > > >> property. This property only exists with the standard PCI bridge type > > >> "pci-bridge" actually. We could possibly revert 7ef1553dac but it seems > > >> much simpler to check the presence of "chassis_nr" earlier. > > > > > > Hrm, right, 7ef1553dac was not really correct since add_drcs() really > > > can fail. > > > > Right. I failed to see that we can run into a bridge without a > > "chassis_nr" here. And I missed it on review, as well. > > >> 2) QEMU abort if same "chassis_nr" value is used several times, > > >>unveiled by commit d2623129a7de "qom: Drop parameter @errp of > > >>object_property_add() & friends" > > >> > > >> $ qemu-system-ppc64 -M pseries -device pci-bridge,chassis_nr=1 \ > > >> -device pci-bridge,chassis_nr=1 > > >> Unexpected error in object_property_try_add() at qom/object.c:1167: > > >> qemu-system-ppc64: -device pci-bridge,chassis_nr=1: attempt to add > > >> duplicate property '4100' to object (type 'container') > > >> Aborted (core dumped) > > > > Before d2623129a7de, the error got *ignored* in > > spapr_dr_connector_new(): > > > > SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type, > > uint32_t id) > > { > > SpaprDrc *drc = SPAPR_DR_CONNECTOR(object_new(type)); > > char *prop_name; > > > > drc->id = id; > > drc->owner = owner; > > prop_name = g_strdup_printf("dr-connector[%"PRIu32"]", > > spapr_drc_index(drc)); > > object_property_add_child(owner, prop_name, OBJECT(drc), > > &error_abort); > > object_unref(OBJECT(drc)); > > --->object_property_set_bool(OBJECT(drc), true, "realized", NULL); > > g_free(prop_name); > > > > return drc; > > } > > > > I doubt that's healthy. Indeed. > This isn't. The object_property_set_bool() was later converted to > qdev_realize() (thanks again for the cleanups!) but the problem > remains. Realize can fail and I see now reason we don't do proper > error handling when it comes to the DRCs. > > I'll look into fixing that. > > > >> This happens because we assume that "chassis_nr" values are unique, but > > >> nobody enforces that and we end up generating duplicate DRC ids. The PCI > > >> code doesn't really care for duplicate "chassis_nr" properties since it > > >> is only used to initialize the "Chassis Number Register" of the bridge, > > >> with no functional impact on QEMU. So, even if passing the same value > > >> several times might look weird, it never broke anything before, so > > >> I guess we don't necessarily want to enforce strict checking in the PCI > > >> code now. > > > > > > Yeah, I guess. I'm pretty sure that the chassis number of bridges is > > > supposed to be system-unique (well, unique within the PCI domain at > > > least, I guess) as part of the hardware spec. So specifying multiple > > > chassis ids the same is a user error, but we need a better failure > > > mode. > > > > > >> Workaround both issues in the PAPR code: check that the bridge has a > > >> unique and non null "chassis_nr" when plugging it into its parent bus. > > >> > > >> Fixes: 05929a6c5dfe ("spapr: Don't use bus number for building DRC ids") > > > > > > Arguably, it's really fixing 7ef1553dac. > > > > I agree 7ef1553dac broke the "use a bridge that doesn't have property > > 'chassis_nr' case. > > > > I suspect the "duplicate chassis_nr" case has always been broken, and > > d2623129a7de merely uncovered it. > > Yes. I agree. > > If we can trigger the abort with hot-plug, then d2623129a7de made things > > materially worse (new way to accidentally kill your guest and maybe lose > > data), and I'd add a Fixes: blaming it. > > > > Yes it does. > > David, > > Maybe consider folding a third Fixes: tag into this patch ? Done. > > >> Reported-by: Thomas Huth > > >> Signed-off-by: Greg Kurz > > > > > > I had a few misgivings about the details of this, bu
Re: [PATCH v4] spapr: Add a new level of NUMA for GPUs
On Thu, Jul 16, 2020 at 05:56:55PM -0500, Reza Arbab wrote: > NUMA nodes corresponding to GPU memory currently have the same > affinity/distance as normal memory nodes. Add a third NUMA associativity > reference point enabling us to give GPU nodes more distance. > > This is guest visible information, which shouldn't change under a > running guest across migration between different qemu versions, so make > the change effective only in new (pseries > 5.0) machine types. > > Before, `numactl -H` output in a guest with 4 GPUs (nodes 2-5): > > node distances: > node 0 1 2 3 4 5 > 0: 10 40 40 40 40 40 > 1: 40 10 40 40 40 40 > 2: 40 40 10 40 40 40 > 3: 40 40 40 10 40 40 > 4: 40 40 40 40 10 40 > 5: 40 40 40 40 40 10 > > After: > > node distances: > node 0 1 2 3 4 5 > 0: 10 40 80 80 80 80 > 1: 40 10 80 80 80 80 > 2: 80 80 10 80 80 80 > 3: 80 80 80 10 80 80 > 4: 80 80 80 80 10 80 > 5: 80 80 80 80 80 10 > > These are the same distances as on the host, mirroring the change made > to host firmware in skiboot commit f845a648b8cb ("numa/associativity: > Add a new level of NUMA for GPU's"). Applied to ppc-for-5.1. > > Signed-off-by: Reza Arbab > --- > v4: > * Use nvslot->numa_id for distinction at all levels of ibm,associativity > * Use ARRAY_SIZE(refpoints) > * Rebase > > v3: > * Squash into one patch > * Add PHB compat property > --- > hw/ppc/spapr.c | 21 +++-- > hw/ppc/spapr_pci.c | 2 ++ > hw/ppc/spapr_pci_nvlink2.c | 13 ++--- > include/hw/pci-host/spapr.h | 1 + > include/hw/ppc/spapr.h | 1 + > 5 files changed, 33 insertions(+), 5 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 299908cc7396..0ae293ec9431 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -890,10 +890,16 @@ static int spapr_dt_rng(void *fdt) > static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt) > { > MachineState *ms = MACHINE(spapr); > +SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms); > int rtas; > GString *hypertas = g_string_sized_new(256); > GString *qemu_hypertas = g_string_sized_new(256); > -uint32_t refpoints[] = { cpu_to_be32(0x4), cpu_to_be32(0x4) }; > +uint32_t refpoints[] = { > +cpu_to_be32(0x4), > +cpu_to_be32(0x4), > +cpu_to_be32(0x2), > +}; > +uint32_t nr_refpoints = ARRAY_SIZE(refpoints); > uint64_t max_device_addr = MACHINE(spapr)->device_memory->base + > memory_region_size(&MACHINE(spapr)->device_memory->mr); > uint32_t lrdr_capacity[] = { > @@ -945,8 +951,12 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void > *fdt) > qemu_hypertas->str, qemu_hypertas->len)); > g_string_free(qemu_hypertas, TRUE); > > +if (smc->pre_5_1_assoc_refpoints) { > +nr_refpoints = 2; > +} > + > _FDT(fdt_setprop(fdt, rtas, "ibm,associativity-reference-points", > - refpoints, sizeof(refpoints))); > + refpoints, nr_refpoints * sizeof(refpoints[0]))); > > _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains", > maxdomains, sizeof(maxdomains))); > @@ -4584,9 +4594,16 @@ DEFINE_SPAPR_MACHINE(5_1, "5.1", true); > */ > static void spapr_machine_5_0_class_options(MachineClass *mc) > { > +SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc); > +static GlobalProperty compat[] = { > +{ TYPE_SPAPR_PCI_HOST_BRIDGE, "pre-5.1-associativity", "on" }, > +}; > + > spapr_machine_5_1_class_options(mc); > compat_props_add(mc->compat_props, hw_compat_5_0, hw_compat_5_0_len); > +compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat)); > mc->numa_mem_supported = true; > +smc->pre_5_1_assoc_refpoints = true; > } > > DEFINE_SPAPR_MACHINE(5_0, "5.0", false); > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 2a6a48744aaa..16739334e35f 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -2035,6 +2035,8 @@ static Property spapr_phb_properties[] = { > pcie_ecs, true), > DEFINE_PROP_UINT64("gpa", SpaprPhbState, nv2_gpa_win_addr, 0), > DEFINE_PROP_UINT64("atsd", SpaprPhbState, nv2_atsd_win_addr, 0), > +DEFINE_PROP_BOOL("pre-5.1-associativity", SpaprPhbState, > + pre_5_1_assoc, false), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/hw/ppc/spapr_pci_nvlink2.c b/hw/ppc/spapr_pci_nvlink2.c > index dd8cd6db9654..76ae77ebc851 100644 > --- a/hw/ppc/spapr_pci_nvlink2.c > +++ b/hw/ppc/spapr_pci_nvlink2.c > @@ -362,9 +362,9 @@ void spapr_phb_nvgpu_ram_populate_dt(SpaprPhbState *sphb, > void *fdt) > &error_abort); > uint32_t associativity[] = { > cpu_to_be32(0x4), > -SPAPR_GPU_NUMA_ID, > -SPAPR_GPU_NUMA_ID, > -
Re: [PATCH qemu v9] spapr: Implement Open Firmware client interface
On Thu, Jul 16, 2020 at 07:04:56PM +1000, Alexey Kardashevskiy wrote: > Ping? I kinda realize it is not going to replace SLOF any time soon but > still... Yeah, I know. I just haven't had time to consider it. Priority starvation. > On 07/07/2020 10:34, Alexey Kardashevskiy wrote: > > Ping? > > > > > > On 24/06/2020 10:28, Alexey Kardashevskiy wrote: > >> Ping? > >> > >> On 02/06/2020 21:40, Alexey Kardashevskiy wrote: > >>> Ping? > >>> > >>> On 13/05/2020 13:58, Alexey Kardashevskiy wrote: > The PAPR platform which describes an OS environment that's presented by > a combination of a hypervisor and firmware. The features it specifies > require collaboration between the firmware and the hypervisor. > > Since the beginning, the runtime component of the firmware (RTAS) has > been implemented as a 20 byte shim which simply forwards it to > a hypercall implemented in qemu. The boot time firmware component is > SLOF - but a build that's specific to qemu, and has always needed to be > updated in sync with it. Even though we've managed to limit the amount > of runtime communication we need between qemu and SLOF, there's some, > and it has become increasingly awkward to handle as we've implemented > new features. > > This implements a boot time OF client interface (CI) which is > enabled by a new "x-vof" pseries machine option (stands for "Virtual Open > Firmware). When enabled, QEMU implements the custom H_OF_CLIENT hcall > which implements Open Firmware Client Interface (OF CI). This allows > using a smaller stateless firmware which does not have to manage > the device tree. > > The new "vof.bin" firmware image is included with source code under > pc-bios/. It also includes RTAS blob. > > This implements a handful of CI methods just to get -kernel/-initrd > working. In particular, this implements the device tree fetching and > simple memory allocator - "claim" (an OF CI memory allocator) and updates > "/memory@0/available" to report the client about available memory. > > This implements changing some device tree properties which we know how > to deal with, the rest is ignored. To allow changes, this skips > fdt_pack() when x-vof=on as not packing the blob leaves some room for > appending. > > In absence of SLOF, this assigns phandles to device tree nodes to make > device tree traversing work. > > When x-vof=on, this adds "/chosen" every time QEMU (re)builds a tree. > > This adds basic instances support which are managed by a hash map > ihandle -> [phandle]. > > Before the guest started, the used memory is: > 0..4000 - the initial firmware > 1..18 - stack > > This OF CI does not implement "interpret". > > With this basic support, this can only boot into kernel directly. > However this is just enough for the petitboot kernel and initradmdisk to > boot from any possible source. Note this requires reasonably recent guest > kernel with: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df5be5be8735 > > Signed-off-by: Alexey Kardashevskiy > --- > > > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
[PATCH v4] spapr: Add a new level of NUMA for GPUs
NUMA nodes corresponding to GPU memory currently have the same affinity/distance as normal memory nodes. Add a third NUMA associativity reference point enabling us to give GPU nodes more distance. This is guest visible information, which shouldn't change under a running guest across migration between different qemu versions, so make the change effective only in new (pseries > 5.0) machine types. Before, `numactl -H` output in a guest with 4 GPUs (nodes 2-5): node distances: node 0 1 2 3 4 5 0: 10 40 40 40 40 40 1: 40 10 40 40 40 40 2: 40 40 10 40 40 40 3: 40 40 40 10 40 40 4: 40 40 40 40 10 40 5: 40 40 40 40 40 10 After: node distances: node 0 1 2 3 4 5 0: 10 40 80 80 80 80 1: 40 10 80 80 80 80 2: 80 80 10 80 80 80 3: 80 80 80 10 80 80 4: 80 80 80 80 10 80 5: 80 80 80 80 80 10 These are the same distances as on the host, mirroring the change made to host firmware in skiboot commit f845a648b8cb ("numa/associativity: Add a new level of NUMA for GPU's"). Signed-off-by: Reza Arbab --- v4: * Use nvslot->numa_id for distinction at all levels of ibm,associativity * Use ARRAY_SIZE(refpoints) * Rebase v3: * Squash into one patch * Add PHB compat property --- hw/ppc/spapr.c | 21 +++-- hw/ppc/spapr_pci.c | 2 ++ hw/ppc/spapr_pci_nvlink2.c | 13 ++--- include/hw/pci-host/spapr.h | 1 + include/hw/ppc/spapr.h | 1 + 5 files changed, 33 insertions(+), 5 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 299908cc7396..0ae293ec9431 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -890,10 +890,16 @@ static int spapr_dt_rng(void *fdt) static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt) { MachineState *ms = MACHINE(spapr); +SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms); int rtas; GString *hypertas = g_string_sized_new(256); GString *qemu_hypertas = g_string_sized_new(256); -uint32_t refpoints[] = { cpu_to_be32(0x4), cpu_to_be32(0x4) }; +uint32_t refpoints[] = { +cpu_to_be32(0x4), +cpu_to_be32(0x4), +cpu_to_be32(0x2), +}; +uint32_t nr_refpoints = ARRAY_SIZE(refpoints); uint64_t max_device_addr = MACHINE(spapr)->device_memory->base + memory_region_size(&MACHINE(spapr)->device_memory->mr); uint32_t lrdr_capacity[] = { @@ -945,8 +951,12 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt) qemu_hypertas->str, qemu_hypertas->len)); g_string_free(qemu_hypertas, TRUE); +if (smc->pre_5_1_assoc_refpoints) { +nr_refpoints = 2; +} + _FDT(fdt_setprop(fdt, rtas, "ibm,associativity-reference-points", - refpoints, sizeof(refpoints))); + refpoints, nr_refpoints * sizeof(refpoints[0]))); _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains", maxdomains, sizeof(maxdomains))); @@ -4584,9 +4594,16 @@ DEFINE_SPAPR_MACHINE(5_1, "5.1", true); */ static void spapr_machine_5_0_class_options(MachineClass *mc) { +SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc); +static GlobalProperty compat[] = { +{ TYPE_SPAPR_PCI_HOST_BRIDGE, "pre-5.1-associativity", "on" }, +}; + spapr_machine_5_1_class_options(mc); compat_props_add(mc->compat_props, hw_compat_5_0, hw_compat_5_0_len); +compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat)); mc->numa_mem_supported = true; +smc->pre_5_1_assoc_refpoints = true; } DEFINE_SPAPR_MACHINE(5_0, "5.0", false); diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 2a6a48744aaa..16739334e35f 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -2035,6 +2035,8 @@ static Property spapr_phb_properties[] = { pcie_ecs, true), DEFINE_PROP_UINT64("gpa", SpaprPhbState, nv2_gpa_win_addr, 0), DEFINE_PROP_UINT64("atsd", SpaprPhbState, nv2_atsd_win_addr, 0), +DEFINE_PROP_BOOL("pre-5.1-associativity", SpaprPhbState, + pre_5_1_assoc, false), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/ppc/spapr_pci_nvlink2.c b/hw/ppc/spapr_pci_nvlink2.c index dd8cd6db9654..76ae77ebc851 100644 --- a/hw/ppc/spapr_pci_nvlink2.c +++ b/hw/ppc/spapr_pci_nvlink2.c @@ -362,9 +362,9 @@ void spapr_phb_nvgpu_ram_populate_dt(SpaprPhbState *sphb, void *fdt) &error_abort); uint32_t associativity[] = { cpu_to_be32(0x4), -SPAPR_GPU_NUMA_ID, -SPAPR_GPU_NUMA_ID, -SPAPR_GPU_NUMA_ID, +cpu_to_be32(nvslot->numa_id), +cpu_to_be32(nvslot->numa_id), +cpu_to_be32(nvslot->numa_id), cpu_to_be32(nvslot->numa_id) }; uint64_t size = object_property_get_uint(nv_mrobj, "size", NULL); @@ -375,6 +375,13 @@ void spapr_phb_nvgpu_ram_populate_dt(S
Re: TB Cache size grows out of control with qemu 5.0
On Thu, 16 Jul 2020, Alex Bennée wrote: Christian Ehrhardt writes: On Wed, Jul 15, 2020 at 5:58 PM BALATON Zoltan wrote: See commit 47a2def4533a2807e48954abd50b32ecb1aaf29a and the next two following it. Thank you Zoltan for pointing out this commit, I agree that this seems to be the trigger for the issues I'm seeing. Unfortunately the common CI host size is 1-2G. For example on Ubuntu Autopkgtests 1.5G. Those of them running guests do so in 0.5-1G size in TCG mode (as they often can't rely on having KVM available). The 1G TB buffer + 0.5G actual guest size + lack of dynamic downsizing on memory pressure (never existed) makes these systems go OOM-Killing the qemu process. Ooops. I admit the assumption was that most people running system emulation would be doing it on beefier machines. The patches indicated that the TB flushes on a full guest boot are a good indicator of the TB size efficiency. From my old checks I had: - Qemu 4.2 512M guest with 32M default overwritten by ram-size/4 TB flush count 14, 14, 16 - Qemu 5.0 512M guest with 1G default TB flush count 1, 1, 1 I agree that ram/4 seems odd, especially on huge guests that is a lot potentially wasted. And most environments have a bit of breathing room 1G is too big in small host systems and the common CI system falls into this category. So I tuned it down to 256M for a test. - Qemu 4.2 512M guest with tb-size 256M TB flush count 5, 5, 5 - Qemu 5.0 512M guest with tb-size 256M TB flush count 5, 5, 5 - Qemu 5.0 512M guest with 256M default in code TB flush count 5, 5, 5 So performance wise the results are as much in-between as you'd think from a TB size in between. And the memory consumption which (for me) is the actual current issue to fix would be back in line again as expected. So I'm glad you have the workaround. So on one hand I'm suggesting something like: --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -944,7 +944,7 @@ static void page_lock_pair(PageDesc **re * Users running large scale system emulation may want to tweak their * runtime setup via the tb-size control on the command line. */ -#define DEFAULT_CODE_GEN_BUFFER_SIZE_1 (1 * GiB) +#define DEFAULT_CODE_GEN_BUFFER_SIZE_1 (256 * MiB) The problem we have is any number we pick here is arbitrary. And while it did regress your use-case changing it again just pushes a performance regression onto someone else. The most (*) 64 bit desktop PCs have 16Gb of RAM, almost all have more than 8gb. And there is a workaround. * random number from Steams HW survey. #endif #endif OTOH I understand someone else might want to get the more speedy 1G especially for large guests. If someone used to run a 4G guest in TCG the TB Size was 1G all along. How about picking the smaller of (1G || ram-size/4) as default? This might then look like: --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -956,7 +956,12 @@ static inline size_t size_code_gen_buffe { /* Size the buffer. */ if (tb_size == 0) { -tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE; +unsigned long max_default = (unsigned long)(ram_size / 4); +if (max_default < DEFAULT_CODE_GEN_BUFFER_SIZE) { +tb_size = max_default; +} else { + tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE; +} } if (tb_size < MIN_CODE_GEN_BUFFER_SIZE) { tb_size = MIN_CODE_GEN_BUFFER_SIZE; This is a bit more tricky than it seems as ram_sizes is no more present in that context but it is enough to discuss it. That should serve all cases - small and large - better as a pure static default of 1G or always ram/4? I'm definitely against re-introducing ram_size into the mix. The original commit (a1b18df9a4) that broke this introduced an ordering dependency which we don't want to bring back. I'd be more amenable to something that took into account host memory and limited the default if it was smaller than a threshold. Is there a way to probe that that doesn't involve slurping /proc/meminfo? I agree that this should be dependent on host memory size not guest ram_size but it might be tricky to get that value because different host OSes would need different ways. Maybe a new qemu_host_mem_size portability function will be needed that implements this for different host OSes. POSIX may or may not have sysconf _SC_PHYS_PAGES and _SC_AVPHYS_PAGES and linux has sysinfo but don't know how reliable these are. Regards, BALATON Zoltan
Re: [GIT PULL] I2C updates
On Thu, Jul 16, 2020 at 09:45:41PM +0100, Peter Maydell wrote: > On Thu, 16 Jul 2020 at 18:49, Corey Minyard wrote: > > > > The following changes since commit 45db94cc90c286a9965a285ba19450f448760a09: > > > > Merge remote-tracking branch > > 'remotes/mcayland/tags/qemu-openbios-20200707' into staging (2020-07-10 > > 16:43:40 +0100) > > > > are available in the Git repository at: > > > > https://github.com/cminyard/qemu.git tags/for-qemu-i2c-5 > > > > for you to fetch changes up to 73d5f22ecbb76dfc785876779d47787084ff0f42: > > > > hw/i2c: Document the I2C qdev helpers (2020-07-16 12:30:54 -0500) > > > > > > Minor changes to: > > > > Add an SMBus config entry > > > > Cleanup/simplify/document some I2C interfaces > > > > > > Philippe Mathieu-Daudé (6): > > hw/i2c/Kconfig: Add an entry for the SMBus > > hw/i2c/aspeed_i2c: Simplify aspeed_i2c_get_bus() > > hw/i2c: Rename i2c_try_create_slave() as i2c_slave_new() > > hw/i2c: Rename i2c_realize_and_unref() as > > i2c_slave_realize_and_unref() > > hw/i2c: Rename i2c_create_slave() as i2c_slave_create_simple() > > hw/i2c: Document the I2C qdev helpers > > Hi; this failed to build on x86-64 Linux (incremental build): Hmm, I did test this, and I just rebuilt, then rebased on the end of master and rebuilt, without issue. It looks like the smbus code is not being included, but I don't see how that can be. -corey > > LINKi386-softmmu/qemu-system-i386 > ../hw/i2c/smbus_eeprom.o: In function `smbus_eeprom_vmstate_needed': > /home/petmay01/linaro/qemu-for-merges/hw/i2c/smbus_eeprom.c:94: > undefined reference to `smbus_vmstate_needed' > ../hw/i2c/smbus_eeprom.o:(.data.rel+0x50): undefined reference to > `vmstate_smbus_device' > ../hw/i2c/pm_smbus.o: In function `smb_transaction': > /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:93: undefined > reference to `smbus_quick_command' > /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:97: undefined > reference to `smbus_receive_byte' > /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:100: undefined > reference to `smbus_send_byte' > /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:105: undefined > reference to `smbus_read_byte' > /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:108: undefined > reference to `smbus_write_byte' > /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:114: undefined > reference to `smbus_read_word' > /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:117: undefined > reference to `smbus_write_word' > /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:149: undefined > reference to `smbus_read_block' > /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:174: undefined > reference to `smbus_write_block' > ../hw/i2c/pm_smbus.o: In function `smb_ioport_writeb': > /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:290: undefined > reference to `smbus_write_block' > ../hw/ipmi/smbus_ipmi.o:(.data.rel+0x50): undefined reference to > `vmstate_smbus_device' > collect2: error: ld returned 1 exit status > > (similarly for other qemu-system-* binary links) > > thanks > -- PMM
Re: sysbus_create_simple Vs qdev_create
On Wed, Jul 15, 2020 at 04:37:18PM +0200, Markus Armbruster wrote: > Pratik Parvati writes: > > > Hi Markus and Philippe, > > > > Thanks for your reply. Now I am pretty clear about Qdev and sysbus helper > > function. > > > > Can you please explain to me in brief on buses and device hierarchies (i.e. > > BusState and DeviceState) and how they are related to each other? As I can > > see, the DeviceState class inherits the BusState > > > > struct DeviceState { > > /*< private >*/ > > Object parent_obj; > > /*< public >*/ > > > > const char *id; > > char *canonical_path; > > bool realized; > > bool pending_deleted_event; > > QemuOpts *opts; > > int hotplugged; > > bool allow_unplug_during_migration; > > BusState *parent_bus; \\ BusState is inherited here > > QLIST_HEAD(, NamedGPIOList) gpios; > > QLIST_HEAD(, BusState) child_bus; > > int num_child_bus; > > int instance_id_alias; > > int alias_required_for_version; > > ResettableState reset; > > }; > > > > and BusState, in turn, inherits the DeviceState as > > > > /** > > * BusState: > > * @hotplug_handler: link to a hotplug handler associated with bus. > > * @reset: ResettableState for the bus; handled by Resettable interface. > > */struct BusState { > > Object obj; > > DeviceState *parent; \\ DeviceState is inherited here > > char *name; > > HotplugHandler *hotplug_handler; > > int max_index; > > bool realized; > > int num_children; > > QTAILQ_HEAD(, BusChild) children; > > QLIST_ENTRY(BusState) sibling; > > ResettableState reset; > > }; > > > > I am a bit confused. Can you brief me this relation! > > We sorely lack introductory documentation on both qdev and QOM. I > believe most developers are more or less confused about them most of the > time. I've done a bit of work on both, so I'm probably less confused > than average. I'm cc'ing maintainers in the hope of reducing average > confusion among participants in this thread. > > DeviceState does not inherit from BusState, and BusState does not > inherit from DeviceState. The relation you marked in the code is > actually "has a". > > A DeviceState may have a BusState, namely the bus the device is plugged > into. "May", because some devices are bus-less (their > DEVICE_GET_CLASS(dev)->bus_type is null), and the others get plugged > into their bus only at realize time. > > Example: PCI device "pci-serial" plugs into a PCI bus. > > Example: device "serial" does not plug into a bus (its used as component > device of "pci-serial" and other devices). > > Example: device "pc-dimm" does not plug into a bus. > > A bus has a DeviceState, namely the device providing this bus. > > Example: device "i440FX-pcihost" provides PCI bus "pci.0". > > Both DeviceState and BusState are QOM subtypes of Object. I prefer to > avoid use of "inherit" for that, because it can mean different things to > different people. I'd also note that the use of "parent" in the code is also ambiguous. It can mean: * QOM parent type, i.e. TypeInfo.parent. Related fields: * parent_class members of class structs * parent_obj members of object structs * QOM composition tree parent object, i.e. Object::parent * qdev device parent bus, i.e. DeviceState::parent_bus * parent device of of qdev bus, i.e. BusState::parent -- Eduardo
Re: [PATCH 2/2] configure: add support for Control-Flow Integrity
On 7/2/2020 5:52 AM, Daniel P. Berrangé wrote: The need to maintain this list of functions makes me feel very uneasy. How can we have any confidence that this list of functions is accurate ? How will maintainers ensure that they correctly update it as they are writing/changing code, and how will they test the result ? Hi Daniel, I gave it some thought and studied more of clang's options. It is possible to disable cfi on specific functions by using an __attribute__ keyword, instead of providing a list in an external file. In terms of maintaining, this is much better since we are removing the need to update the list. I would suggest defining a macro, __disable_cfi__, that can be prepended to a function. The macro would expand to nothing if cfi is disabled, or to the proper attribute if it is enabled. Here's example code snippet /* Disable CFI checks. * The callback function has been loaded from an external library so we do not * have type information */ __disable_cfi__ void qemu_plugin_vcpu_syscall_ret(CPUState *cpu, int64_t num, int64_t ret) { ... } This would take care of renaming and removal of functions that need cfi. It would also probably be beneficial to the developers since they can see immediately that the function they are working with needs to have CFI checks disabled, and why. If you think this is a better approach, I'll submit v2 with this approach instead of the external function list. For new code, however, the best thing is proper education and testing. I'll work on a document for docs/devel to detail what it is and how to make code cfi-safe. A good approach should be to test code changes with CFI enabled. CFI is, after all, a sanitizer and therefore it makes sense to use it also during development, together with ASan, UBSan and the likes. Unfortunately, since it works only with clang, I don't think this can ever be a hard requirement. Daniele
hw-display-qxl.so: undefined symbol: qemu_qxl_io_log_semaphore
Hi Gerd, I'm trying to build qemu 5.1.0-rc0 in Fedora. I'm hitting some issues. Using this configure line: ./configure --prefix=/usr --libdir=/usr/lib64 --sysconfdir=/etc --localstatedir=/var --libexecdir=/usr/libexec --interp-prefix=/usr/qemu-%M --with-pkgversion=qemu-5.1.0-0.1.rc0.fc33 '--extra-ldflags=-Wl,--build-id -Wl,-z,relro -Wl,-z,now' '--extra-cflags=-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection' --enable-trace-backend=dtrace --audio-drv-list=pa,sdl,alsa,oss --enable-kvm --target-list=x86_64-softmmu --enable-pie --enable-modules --enable-spice Build and then run: $ ./x86_64-softmmu/qemu-system-x86_64 -device \? | grep qxl Failed to open module: /home/crobinso/src/qemu/x86_64-softmmu/../hw-display-qxl.so: undefined symbol: qemu_qxl_io_log_semaphore That error breaks iotests 127: --- /home/crobinso/src/qemu/tests/qemu-iotests/127.out 2020-07-15 04:00:10.589138586 -0400 +++ /home/crobinso/src/qemu/tests/qemu-iotests/127.out.bad 2020-07-16 16:44:37.717248172 -0400 @@ -1,4 +1,5 @@ QA output created by 127 +Failed to open module: /home/crobinso/src/qemu/x86_64-softmmu/../hw-display-qxl.so: undefined symbol: qemu_qxl_io_log_semaphore Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536 Formatting 'TEST_DIR/t.IMGFMT.overlay0', fmt=IMGFMT size=65536 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT Formatting 'TEST_DIR/t.IMGFMT.overlay1', fmt=IMGFMT size=65536 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT Doing a build with every target and running 'make check' will show undefined symbol errors for other targets with hw-display-qxl too. Most reference the qxl_io_log call but some are like: Failed to open module: /home/crobinso/src/qemu/microblaze-softmmu/../hw-display-qxl.so: undefined symbol: vga_ioport_read Also as a side note though I think it's pre-existing: running the test suite with --enable-modules while there are host installed modules is very noisy with lots of repetitive warnings like: Failed to initialize module: /usr/lib64/qemu/audio-oss.so Note: only modules from the same build can be loaded. Failed to initialize module: /usr/lib64/qemu/audio-pa.so Note: only modules from the same build can be loaded. Failed to initialize module: /usr/lib64/qemu/audio-sdl.so Note: only modules from the same build can be loaded. Failed to initialize module: /usr/lib64/qemu/ui-curses.so Note: only modules from the same build can be loaded. Failed to initialize module: /usr/lib64/qemu/ui-gtk.so Note: only modules from the same build can be loaded. It would be nice if those could be avoided somehow. Maybe QEMU_MODULE_DIR can help? Thanks, Cole
Re: [RFC PATCH] tcg/cpu-exec: precise single-stepping after an exception
On 7/16/20 1:12 PM, Peter Maydell wrote: > On Thu, 16 Jul 2020 at 11:08, Luc Michel wrote: >> >> When single-stepping with a debugger attached to QEMU, and when an >> exception is raised, the debugger misses the first instruction after the >> exception: > > This is a long-standing bug; thanks for looking at it. > (https://bugs.launchpad.net/qemu/+bug/757702) > > >> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c >> index d95c4848a4..e85fab5d40 100644 >> --- a/accel/tcg/cpu-exec.c >> +++ b/accel/tcg/cpu-exec.c >> @@ -502,10 +502,21 @@ static inline bool cpu_handle_exception(CPUState *cpu, >> int *ret) >> CPUClass *cc = CPU_GET_CLASS(cpu); >> qemu_mutex_lock_iothread(); >> cc->do_interrupt(cpu); >> qemu_mutex_unlock_iothread(); >> cpu->exception_index = -1; >> + >> +if (unlikely(cpu->singlestep_enabled)) { >> +/* >> + * After processing the exception, ensure an EXCP_DEBUG is >> + * raised when single-stepping so that GDB doesn't miss the >> + * next instruction. >> + */ >> +cpu->exception_index = EXCP_DEBUG; >> +return cpu_handle_exception(cpu, ret); >> +} > > I like the idea of being able to do this generically in > the main loop. > > How about interrupts? If we are single-stepping and we > take an interrupt I guess we want to stop before the first > insn of the interrupt handler rather than after it, which > would imply a similar change to cpu_handle_interrupt(). Fair. I think something like this: if (cc->cpu_exec_interrupt(cpu, interrupt_request)) { replay_interrupt(); - cpu->exception_index = -1; + cpu->exception_index = + (cpu->singlestep_enabled ? EXCP_DEBUG : -1); *last_tb = NULL; } I'm not quite sure how to test this though... Probably best to keep this a separate patch anyway. r~
Re: qemu test-qga failure on mergebuild after VERSION file change: dependency issues??
On 7/16/20 10:15 PM, Peter Maydell wrote: > On Thu, 16 Jul 2020 at 20:52, Michael Roth wrote: >> But is it intermittent, environment-dependent? I'm trying to understand how >> to >> replicate Peter's result since it seems like it would be straightforward >> reproducer. > > I blew away all my build trees and recreated them from > scratch, and the issue went away. I'm suspicious that the > complete lack of .d files was induced by a failed earlier > pullreq attempt and left the build tree in a messed up state > where it wouldn't notice that it needed to rebuild files. If it happens again, can you try to revert aaa1b70a0b ("Makefile: simplify MINIKCONF rules") on top of the tag you are testing, and re-run the testing?
Re: [PATCH v5 10/11] hw/arm: Wire up BMC boot flash for npcm750-evb and quanta-gsj
On Wed, Jul 15, 2020 at 1:54 PM Havard Skinnemoen wrote: > > On Wed, Jul 15, 2020 at 3:57 AM Philippe Mathieu-Daudé > wrote: > > > > On 7/15/20 11:00 AM, Markus Armbruster wrote: > > > Now my point. Why first make up user configuration, then use that to > > > create a BlockBackend, when you could just go ahead and create the > > > BlockBackend? > > > > CLI issue mostly. > > > > We can solve it similarly to the recent "sdcard: Do not allow invalid SD > > card sizes" patch: > > > > if (!dinfo) { > > error_setg(errp, "Missing SPI flash drive"); > > error_append_hint(errp, "You can use a dummy drive using:\n"); > > error_append_hint(errp, "-drive if=mtd,driver=null-co," > > "read-ones=on,size=64M\n); > > return; > > } > > > > having npcm7xx_connect_flash() taking an Error* argument, > > and MachineClass::init() call it with &error_fatal. > > Erroring out if the user specifies a configuration that can't possibly > boot sounds good to me. Better than trying to come up with defaults > that are still not going to result in a bootable system. > > For testing recovery paths, I think it makes sense to explicitly > specify a null device as you suggest. Hmm, one problem. qom-test fails with qemu-system-aarch64: Missing SPI flash drive You can add a dummy drive using: -drive if=mtd,driver=null-co,read-zeroes=on,size=32M Broken pipe /usr/local/google/home/hskinnemoen/qemu/for-upstream/tests/qtest/libqtest.c:166: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0) ERROR qom-test - too few tests run (expected 68, got 7) So it looks like we might need a different solution to this, unless we want to make generic tests more machine-aware...
Re: [GIT PULL] I2C updates
On Thu, 16 Jul 2020 at 18:49, Corey Minyard wrote: > > The following changes since commit 45db94cc90c286a9965a285ba19450f448760a09: > > Merge remote-tracking branch 'remotes/mcayland/tags/qemu-openbios-20200707' > into staging (2020-07-10 16:43:40 +0100) > > are available in the Git repository at: > > https://github.com/cminyard/qemu.git tags/for-qemu-i2c-5 > > for you to fetch changes up to 73d5f22ecbb76dfc785876779d47787084ff0f42: > > hw/i2c: Document the I2C qdev helpers (2020-07-16 12:30:54 -0500) > > > Minor changes to: > > Add an SMBus config entry > > Cleanup/simplify/document some I2C interfaces > > > Philippe Mathieu-Daudé (6): > hw/i2c/Kconfig: Add an entry for the SMBus > hw/i2c/aspeed_i2c: Simplify aspeed_i2c_get_bus() > hw/i2c: Rename i2c_try_create_slave() as i2c_slave_new() > hw/i2c: Rename i2c_realize_and_unref() as i2c_slave_realize_and_unref() > hw/i2c: Rename i2c_create_slave() as i2c_slave_create_simple() > hw/i2c: Document the I2C qdev helpers Hi; this failed to build on x86-64 Linux (incremental build): LINKi386-softmmu/qemu-system-i386 ../hw/i2c/smbus_eeprom.o: In function `smbus_eeprom_vmstate_needed': /home/petmay01/linaro/qemu-for-merges/hw/i2c/smbus_eeprom.c:94: undefined reference to `smbus_vmstate_needed' ../hw/i2c/smbus_eeprom.o:(.data.rel+0x50): undefined reference to `vmstate_smbus_device' ../hw/i2c/pm_smbus.o: In function `smb_transaction': /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:93: undefined reference to `smbus_quick_command' /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:97: undefined reference to `smbus_receive_byte' /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:100: undefined reference to `smbus_send_byte' /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:105: undefined reference to `smbus_read_byte' /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:108: undefined reference to `smbus_write_byte' /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:114: undefined reference to `smbus_read_word' /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:117: undefined reference to `smbus_write_word' /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:149: undefined reference to `smbus_read_block' /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:174: undefined reference to `smbus_write_block' ../hw/i2c/pm_smbus.o: In function `smb_ioport_writeb': /home/petmay01/linaro/qemu-for-merges/hw/i2c/pm_smbus.c:290: undefined reference to `smbus_write_block' ../hw/ipmi/smbus_ipmi.o:(.data.rel+0x50): undefined reference to `vmstate_smbus_device' collect2: error: ld returned 1 exit status (similarly for other qemu-system-* binary links) thanks -- PMM
[Bug 1887854] Re: Spurious Data Abort on qemu-system-aarch64
Writing to SCTLR can cause QEMU to flush its TLB (as an internal implementation detail), so if adding SCTLR writes is sufficient to cause the problem to go away, I would be suspicious that your guest code is missing necessary TLB maintenance instructions. QEMU 3.1 and 4.1 are quite old -- can you reproduce with 5.0 or (ideally) head-of-git ? -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1887854 Title: Spurious Data Abort on qemu-system-aarch64 Status in QEMU: New Bug description: When running RTEMS test psxndbm01.exe built for AArch64-ilp32 (this code is not yet publically available), the test generates a spurious data abort (the MMU and alignment checks should be disabled according to bits 1, 0 of SCTLR_EL1). The abort information is as follows: Taking exception 4 [Data Abort] ...from EL1 to EL1 ...with ESR 0x25/0x9610 ...with FAR 0x104010ca28 ...with ELR 0x400195d8 ...to EL1 PC 0x40018200 PSTATE 0x3c5 The ESR indicates that a synchronous external abort has occurred. ESR EC field: 0b100101 From the ARMv8 technical manual: Data Abort taken without a change in Exception level. Used for MMU faults generated by data accesses, alignment faults other than those caused by Stack Pointer misalignment, and synchronous External aborts, including synchronous parity or ECC errors. Not used for debug related exceptions. ESR ISS field: 0b1 From the ARMv8 technical manual: Synchronous External abort, not on translation table walk or hardware update of translation table. The following command line is used to invoke qemu: qemu-system-aarch64 -machine virt -cpu cortex-a53 -m 256M -no-reboot -nographic -serial mon:stdio -kernel build/aarch64/a53_ilp32_qemu/testsuites/psxtests/psxndbm01.exe -D qemu.log -d in_asm,int,cpu_reset,unimp,guest_errors This occurs on Qemu 3.1.0 as distributed via Debian and on Qemu 4.1 as built by the RTEMS source builder (4.1+minor patches). Edit: This bug can be worked around by getting and setting SCTLR without changing its value before each data abort would occur. This test needs 6 of these workarounds to operate successfully. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1887854/+subscriptions
Re: qemu test-qga failure on mergebuild after VERSION file change: dependency issues??
On Thu, 16 Jul 2020 at 20:52, Michael Roth wrote: > But is it intermittent, environment-dependent? I'm trying to understand how to > replicate Peter's result since it seems like it would be straightforward > reproducer. I blew away all my build trees and recreated them from scratch, and the issue went away. I'm suspicious that the complete lack of .d files was induced by a failed earlier pullreq attempt and left the build tree in a messed up state where it wouldn't notice that it needed to rebuild files. -- PMM
Re: [PULL 0/2] Fixes 20200716 patches
On Thu, 16 Jul 2020 at 10:34, Gerd Hoffmann wrote: > > The following changes since commit 8746309137ba470d1b2e8f5ce86ac228625db940: > > Update version for v5.1.0-rc0 release (2020-07-15 19:08:07 +0100) > > are available in the Git repository at: > > git://git.kraxel.org/qemu tags/fixes-20200716-pull-request > > for you to fetch changes up to 4084e35068772cf4f81bbae5174019f277c61084: > > usb: fix storage regression (2020-07-16 10:20:27 +0200) > > > fixes: usb storage regression, vfio display ramfb bug > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1 for any user-visible changes. -- PMM
Re: [RFC PATCH] tcg/cpu-exec: precise single-stepping after an exception
On Thu, 16 Jul 2020 at 11:08, Luc Michel wrote: > > When single-stepping with a debugger attached to QEMU, and when an > exception is raised, the debugger misses the first instruction after the > exception: This is a long-standing bug; thanks for looking at it. (https://bugs.launchpad.net/qemu/+bug/757702) > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c > index d95c4848a4..e85fab5d40 100644 > --- a/accel/tcg/cpu-exec.c > +++ b/accel/tcg/cpu-exec.c > @@ -502,10 +502,21 @@ static inline bool cpu_handle_exception(CPUState *cpu, > int *ret) > CPUClass *cc = CPU_GET_CLASS(cpu); > qemu_mutex_lock_iothread(); > cc->do_interrupt(cpu); > qemu_mutex_unlock_iothread(); > cpu->exception_index = -1; > + > +if (unlikely(cpu->singlestep_enabled)) { > +/* > + * After processing the exception, ensure an EXCP_DEBUG is > + * raised when single-stepping so that GDB doesn't miss the > + * next instruction. > + */ > +cpu->exception_index = EXCP_DEBUG; > +return cpu_handle_exception(cpu, ret); > +} I like the idea of being able to do this generically in the main loop. How about interrupts? If we are single-stepping and we take an interrupt I guess we want to stop before the first insn of the interrupt handler rather than after it, which would imply a similar change to cpu_handle_interrupt(). thanks -- PMM
[Bug 1887854] Re: Spurious Data Abort on qemu-system-aarch64
** Description changed: When running RTEMS test psxndbm01.exe built for AArch64-ilp32 (this code is not yet publically available), the test generates a spurious data abort (the MMU and alignment checks should be disabled according to bits 1, 0 of SCTLR_EL1). The abort information is as follows: Taking exception 4 [Data Abort] ...from EL1 to EL1 ...with ESR 0x25/0x9610 ...with FAR 0x104010ca28 ...with ELR 0x400195d8 ...to EL1 PC 0x40018200 PSTATE 0x3c5 The ESR indicates that a synchronous external abort has occurred. ESR EC field: 0b100101 From the ARMv8 technical manual: Data Abort taken without a change in Exception level. Used for MMU faults generated by data accesses, alignment faults other than those caused by Stack Pointer misalignment, and synchronous External aborts, including synchronous parity or ECC errors. Not used for debug related exceptions. ESR ISS field: 0b1 From the ARMv8 technical manual: Synchronous External abort, not on translation table walk or hardware update of translation table. The following command line is used to invoke qemu: qemu-system-aarch64 -machine virt -cpu cortex-a53 -m 256M -no-reboot -nographic -serial mon:stdio -kernel build/aarch64/a53_ilp32_qemu/testsuites/psxtests/psxndbm01.exe -D qemu.log -d in_asm,int,cpu_reset,unimp,guest_errors This occurs on Qemu 3.1.0 as distributed via Debian and on Qemu 4.1 as built by the RTEMS source builder (4.1+minor patches). Edit: This bug can be worked around by getting and setting SCTLR without - changing its value. + changing its value before each data abort would occur. This test needs 6 + of these workarounds to operate successfully. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1887854 Title: Spurious Data Abort on qemu-system-aarch64 Status in QEMU: New Bug description: When running RTEMS test psxndbm01.exe built for AArch64-ilp32 (this code is not yet publically available), the test generates a spurious data abort (the MMU and alignment checks should be disabled according to bits 1, 0 of SCTLR_EL1). The abort information is as follows: Taking exception 4 [Data Abort] ...from EL1 to EL1 ...with ESR 0x25/0x9610 ...with FAR 0x104010ca28 ...with ELR 0x400195d8 ...to EL1 PC 0x40018200 PSTATE 0x3c5 The ESR indicates that a synchronous external abort has occurred. ESR EC field: 0b100101 From the ARMv8 technical manual: Data Abort taken without a change in Exception level. Used for MMU faults generated by data accesses, alignment faults other than those caused by Stack Pointer misalignment, and synchronous External aborts, including synchronous parity or ECC errors. Not used for debug related exceptions. ESR ISS field: 0b1 From the ARMv8 technical manual: Synchronous External abort, not on translation table walk or hardware update of translation table. The following command line is used to invoke qemu: qemu-system-aarch64 -machine virt -cpu cortex-a53 -m 256M -no-reboot -nographic -serial mon:stdio -kernel build/aarch64/a53_ilp32_qemu/testsuites/psxtests/psxndbm01.exe -D qemu.log -d in_asm,int,cpu_reset,unimp,guest_errors This occurs on Qemu 3.1.0 as distributed via Debian and on Qemu 4.1 as built by the RTEMS source builder (4.1+minor patches). Edit: This bug can be worked around by getting and setting SCTLR without changing its value before each data abort would occur. This test needs 6 of these workarounds to operate successfully. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1887854/+subscriptions
Re: qemu test-qga failure on mergebuild after VERSION file change: dependency issues??
On 7/16/20 9:52 PM, Michael Roth wrote: > Quoting Philippe Mathieu-Daudé (2020-07-16 12:59:28) >> On 7/16/20 7:55 PM, Michael Roth wrote: >>> Quoting Peter Maydell (2020-07-16 05:53:17) The first merge I tried to process after bumping VERSION for rc0 failed on test-qga like this: MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} tests/test-qga -m=quick -k --tap < /dev/null | ./scripts/tap-driv er.pl --test-name="test-qga" PASS 1 test-qga /qga/sync-delimited PASS 2 test-qga /qga/sync PASS 3 test-qga /qga/ping ** ERROR:/home/petmay01/linaro/qemu-for-merges/tests/test-qga.c:303:test_qga_info: assertion failed (version == QEMU_VERSION): ("5.0.9 0" == "5.0.50") ERROR test-qga - Bail out! ERROR:/home/petmay01/linaro/qemu-for-merges/tests/test-qga.c:303:test_qga_info: assertion failed (versio n == QEMU_VERSION): ("5.0.90" == "5.0.50") Aborted (core dumped) /home/petmay01/linaro/qemu-for-merges/tests/Makefile.include:659: recipe for target 'check-unit' failed Looking at timestamps on files, tests/test-qga.o never got rebuilt, even though config-host.h has been updated (and so has the new QEMU_VERSION). Any idea what's gone wrong here? Also weird: this build tree has no .d files in it. >>> >>> I've been trying to reproduce with: >>> >>> make >>> make check-unit >>> *bump VERSION >>> make check-unit >>> >>> but test-qga.o gets rebuilt as expected and the test passed. >>> >>> This is with ubuntu 18.04, x86, with out-of-tree build directory. Are you >>> aware >>> of any other factors that might be needed to reproduce this? >> >> The problem is not for qga, it affects all QEMU objects. > > But is it intermittent, environment-dependent? I'm trying to understand how to > replicate Peter's result since it seems like it would be straightforward > reproducer. How to reproduce: https://www.mail-archive.com/qemu-devel@nongnu.org/msg723531.html > >> >>> thanks -- PMM >>> >> >
Re: qemu test-qga failure on mergebuild after VERSION file change: dependency issues??
Quoting Philippe Mathieu-Daudé (2020-07-16 12:59:28) > On 7/16/20 7:55 PM, Michael Roth wrote: > > Quoting Peter Maydell (2020-07-16 05:53:17) > >> The first merge I tried to process after bumping VERSION for rc0 > >> failed on test-qga like this: > >> > >> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} > >> tests/test-qga -m=quick -k --tap < /dev/null | ./scripts/tap-driv > >> er.pl --test-name="test-qga" > >> PASS 1 test-qga /qga/sync-delimited > >> PASS 2 test-qga /qga/sync > >> PASS 3 test-qga /qga/ping > >> ** > >> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/test-qga.c:303:test_qga_info: > >> assertion failed (version == QEMU_VERSION): ("5.0.9 > >> 0" == "5.0.50") > >> ERROR test-qga - Bail out! > >> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/test-qga.c:303:test_qga_info: > >> assertion failed (versio > >> n == QEMU_VERSION): ("5.0.90" == "5.0.50") > >> Aborted (core dumped) > >> /home/petmay01/linaro/qemu-for-merges/tests/Makefile.include:659: > >> recipe for target 'check-unit' failed > >> > >> Looking at timestamps on files, tests/test-qga.o never got rebuilt, > >> even though config-host.h has been updated (and so has the new > >> QEMU_VERSION). Any idea what's gone wrong here? > >> > >> Also weird: this build tree has no .d files in it. > > > > I've been trying to reproduce with: > > > > make > > make check-unit > > *bump VERSION > > make check-unit > > > > but test-qga.o gets rebuilt as expected and the test passed. > > > > This is with ubuntu 18.04, x86, with out-of-tree build directory. Are you > > aware > > of any other factors that might be needed to reproduce this? > > The problem is not for qga, it affects all QEMU objects. But is it intermittent, environment-dependent? I'm trying to understand how to replicate Peter's result since it seems like it would be straightforward reproducer. > > > > >> > >> thanks > >> -- PMM > > >
[Bug 1887854] [NEW] Spurious Data Abort on qemu-system-aarch64
Public bug reported: When running RTEMS test psxndbm01.exe built for AArch64-ilp32 (this code is not yet publically available), the test generates a spurious data abort (the MMU and alignment checks should be disabled according to bits 1, 0 of SCTLR_EL1). The abort information is as follows: Taking exception 4 [Data Abort] ...from EL1 to EL1 ...with ESR 0x25/0x9610 ...with FAR 0x104010ca28 ...with ELR 0x400195d8 ...to EL1 PC 0x40018200 PSTATE 0x3c5 The ESR indicates that a synchronous external abort has occurred. ESR EC field: 0b100101 >From the ARMv8 technical manual: Data Abort taken without a change in Exception level. Used for MMU faults generated by data accesses, alignment faults other than those caused by Stack Pointer misalignment, and synchronous External aborts, including synchronous parity or ECC errors. Not used for debug related exceptions. ESR ISS field: 0b1 >From the ARMv8 technical manual: Synchronous External abort, not on translation table walk or hardware update of translation table. The following command line is used to invoke qemu: qemu-system-aarch64 -machine virt -cpu cortex-a53 -m 256M -no-reboot -nographic -serial mon:stdio -kernel build/aarch64/a53_ilp32_qemu/testsuites/psxtests/psxndbm01.exe -D qemu.log -d in_asm,int,cpu_reset,unimp,guest_errors This occurs on Qemu 3.1.0 as distributed via Debian and on Qemu 4.1 as built by the RTEMS source builder (4.1+minor patches). Edit: This bug can be worked around by getting and setting SCTLR without changing its value. ** Affects: qemu Importance: Undecided Status: New ** Attachment added: "Qemu execution log and binary" https://bugs.launchpad.net/bugs/1887854/+attachment/5393233/+files/data_abort.tar.gz ** Description changed: When running RTEMS test psxndbm01.exe built for AArch64-ilp32 (this code is not yet publically available), the test generates a spurious data abort (the MMU and alignment checks should be disabled according to bits 1, 0 of SCTLR_EL1). The abort information is as follows: Taking exception 4 [Data Abort] ...from EL1 to EL1 ...with ESR 0x25/0x9610 ...with FAR 0x104010ca28 ...with ELR 0x400195d8 ...to EL1 PC 0x40018200 PSTATE 0x3c5 The ESR indicates that a synchronous external abort has occurred. ESR EC field: 0b100101 From the ARMv8 technical manual: Data Abort taken without a change in Exception level. Used for MMU faults generated by data accesses, alignment faults other than those caused by Stack Pointer misalignment, and synchronous External aborts, including synchronous parity or ECC errors. Not used for debug related exceptions. ESR ISS field: 0b1 From the ARMv8 technical manual: Synchronous External abort, not on translation table walk or hardware update of translation table. The following command line is used to invoke qemu: qemu-system-aarch64 -machine virt -cpu cortex-a53 -m 256M -no-reboot -nographic -serial mon:stdio -kernel build/aarch64/a53_ilp32_qemu/testsuites/psxtests/psxndbm01.exe -D qemu.log -d in_asm,int,cpu_reset,unimp,guest_errors This occurs on Qemu 3.1.0 as distributed via Debian and on Qemu 4.1 as built by the RTEMS source builder (4.1+minor patches). + + Edit: This bug can be worked around by getting and setting SCTLR without + changing its value. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1887854 Title: Spurious Data Abort on qemu-system-aarch64 Status in QEMU: New Bug description: When running RTEMS test psxndbm01.exe built for AArch64-ilp32 (this code is not yet publically available), the test generates a spurious data abort (the MMU and alignment checks should be disabled according to bits 1, 0 of SCTLR_EL1). The abort information is as follows: Taking exception 4 [Data Abort] ...from EL1 to EL1 ...with ESR 0x25/0x9610 ...with FAR 0x104010ca28 ...with ELR 0x400195d8 ...to EL1 PC 0x40018200 PSTATE 0x3c5 The ESR indicates that a synchronous external abort has occurred. ESR EC field: 0b100101 From the ARMv8 technical manual: Data Abort taken without a change in Exception level. Used for MMU faults generated by data accesses, alignment faults other than those caused by Stack Pointer misalignment, and synchronous External aborts, including synchronous parity or ECC errors. Not used for debug related exceptions. ESR ISS field: 0b1 From the ARMv8 technical manual: Synchronous External abort, not on translation table walk or hardware update of translation table. The following command line is used to invoke qemu: qemu-system-aarch64 -machine virt -cpu cortex-a53 -m 256M -no-reboot -nographic -serial mon:stdio -kernel build/aarch64/a53_ilp32_qemu/testsuites/psxtests/psxndbm01.exe -D qemu.log -d in_asm,int,cpu_reset,unimp,guest_errors This occurs on Qemu 3.1.0 as distributed via Debian and on Qemu 4.1 as built by t
[PATCH v2] tcg/cpu-exec: precise single-stepping after an exception
When single-stepping with a debugger attached to QEMU, and when an exception is raised, the debugger misses the first instruction after the exception: $ qemu-system-aarch64 -M virt -display none -cpu cortex-a53 -s -S $ aarch64-linux-gnu-gdb GNU gdb (GDB) 9.2 [...] (gdb) tar rem :1234 Remote debugging using :1234 warning: No executable has been specified and target does not support determining executable automatically. Try using the "file" command. 0x in ?? () (gdb) # writing nop insns to 0x200 and 0x204 (gdb) set *0x200 = 0xd503201f (gdb) set *0x204 = 0xd503201f (gdb) # 0x0 address contains 0 which is an invalid opcode. (gdb) # The CPU should raise an exception and jump to 0x200 (gdb) si 0x0204 in ?? () With this commit, the same run steps correctly on the first instruction of the exception vector: (gdb) si 0x0200 in ?? () Signed-off-by: Luc Michel --- v2: - remove RFC tag - inline the recursive call to cpu_handle_exception [Richard] --- accel/tcg/cpu-exec.c | 12 1 file changed, 12 insertions(+) diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index d95c4848a4..59b1b5fe76 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -502,10 +502,22 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret) CPUClass *cc = CPU_GET_CLASS(cpu); qemu_mutex_lock_iothread(); cc->do_interrupt(cpu); qemu_mutex_unlock_iothread(); cpu->exception_index = -1; + +if (unlikely(cpu->singlestep_enabled)) { +/* + * After processing the exception, ensure an EXCP_DEBUG is + * raised when single-stepping so that GDB doesn't miss the + * next instruction. + */ +*ret = EXCP_DEBUG; +cpu_handle_debug_exception(cpu); +return true; +} + } else if (!replay_has_interrupt()) { /* give a chance to iothread in replay mode */ *ret = EXCP_INTERRUPT; return true; } -- 2.27.0
[PATCH] net: check payload length limit for all frames
From: Prasad J Pandit While sending packets, the check that packet 'payload_len' is within 64kB limit, seems to happen only for GSO frames. It may lead to use-after-free or out-of-bounds access like issues when sending non-GSO frames. Check the 'payload_len' limit for all packets, irrespective of the gso type. Reported-by: Alexander Bulekov Signed-off-by: Prasad J Pandit --- hw/net/net_tx_pkt.c | 10 -- 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c index 162f802dd7..e66998a8f9 100644 --- a/hw/net/net_tx_pkt.c +++ b/hw/net/net_tx_pkt.c @@ -607,12 +607,10 @@ bool net_tx_pkt_send(struct NetTxPkt *pkt, NetClientState *nc) * Since underlying infrastructure does not support IP datagrams longer * than 64K we should drop such packets and don't even try to send */ -if (VIRTIO_NET_HDR_GSO_NONE != pkt->virt_hdr.gso_type) { -if (pkt->payload_len > -ETH_MAX_IP_DGRAM_LEN - -pkt->vec[NET_TX_PKT_L3HDR_FRAG].iov_len) { -return false; -} +if (pkt->payload_len > +ETH_MAX_IP_DGRAM_LEN - +pkt->vec[NET_TX_PKT_L3HDR_FRAG].iov_len) { +return false; } if (pkt->has_virt_hdr || -- 2.26.2
Re: [PATCH for-5.1] i386: hvf: Explicitly set CR4 guest/host mask
On Thu, Jul 16, 2020 at 02:14:57PM -0400, Eduardo Habkost wrote: > On Tue, Jul 14, 2020 at 08:20:04PM +0200, Paolo Bonzini wrote: > > Hi Roman, please ask Peter to apply it directly because I won't be able to > > send a pull request in the next couple of weeks. > > > > Paolo > > > > Il mar 14 lug 2020, 12:39 Roman Bolshakov ha > > scritto: > > > > > On Tue, Jul 14, 2020 at 12:07:27PM +0300, Roman Bolshakov wrote: > > > > Removal of register reset omitted initialization of CR4 guest/host mask. > > > > x86_64 guests aren't booting without it. > > > > > > > > Fixes: 5009ef22c6bb2 ("i386: hvf: Don't duplicate register reset") > > > > Signed-off-by: Roman Bolshakov > > > > > > > > > > If one has a chance to test or review it, it'd be very helpful. That'd > > > allow to include it in RC0. > > > > > I'll queue it for my -rc1 pull request. > Thanks! -Roman
[PULL 4/6] target/i386: fix model number and add missing features for Icelake-Server CPU model
From: Chenyi Qiang Add the missing features(sha_ni, avx512ifma, rdpid, fsrm, vmx-rdseed-exit, vmx-pml, vmx-eptp-switching) and change the model number to 106 in the Icelake-Server-v4 CPU model. Signed-off-by: Chenyi Qiang Message-Id: <20200714084148.26690-3-chenyi.qi...@intel.com> Signed-off-by: Eduardo Habkost --- target/i386/cpu.c | 14 ++ 1 file changed, 14 insertions(+) diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 3885826bc4..132ef90421 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -3512,6 +3512,20 @@ static X86CPUDefinition builtin_x86_defs[] = { { /* end of list */ } }, }, +{ +.version = 4, +.props = (PropValue[]) { +{ "sha-ni", "on" }, +{ "avx512ifma", "on" }, +{ "rdpid", "on" }, +{ "fsrm", "on" }, +{ "vmx-rdseed-exit", "on" }, +{ "vmx-pml", "on" }, +{ "vmx-eptp-switching", "on" }, +{ "model", "106" }, +{ /* end of list */ } +}, +}, { /* end of list */ } } }, -- 2.26.2
[PULL 1/6] i368/cpu: Clear env->user_features after loading versioned CPU model
From: Xiaoyao Li Features defined in versioned CPU model are recorded in env->user_features since they are updated as property. It's unwated because they are not user specified. Simply clear env->user_features as a fix. It won't clear user specified features because user specified features are filled to env->user_features later in x86_cpu_expand_features(). Cc: Chenyi Qiang Suggested-by: Eduardo Habkost Signed-off-by: Xiaoyao Li Message-Id: <20200713174436.41070-2-xiaoyao...@intel.com> [ehabkost: fix coding style] Signed-off-by: Eduardo Habkost --- target/i386/cpu.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 1e5123251d..caf0334f3a 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -5159,6 +5159,13 @@ static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model) object_property_set_str(OBJECT(cpu), "vendor", vendor, &error_abort); x86_cpu_apply_version_props(cpu, model); + +/* + * Properties in versioned CPU model are not user specified features. + * We can simply clear env->user_features here since it will be filled later + * in x86_cpu_expand_features() based on plus_features and minus_features. + */ +memset(&env->user_features, 0, sizeof(env->user_features)); } #ifndef CONFIG_USER_ONLY -- 2.26.2
[PULL 6/6] i386: hvf: Explicitly set CR4 guest/host mask
From: Roman Bolshakov Removal of register reset omitted initialization of CR4 guest/host mask. x86_64 guests aren't booting without it. Fixes: 5009ef22c6bb2 ("i386: hvf: Don't duplicate register reset") Signed-off-by: Roman Bolshakov Message-Id: <20200714090726.41082-1-r.bolsha...@yadro.com> Signed-off-by: Eduardo Habkost --- target/i386/hvf/vmx.h | 1 + 1 file changed, 1 insertion(+) diff --git a/target/i386/hvf/vmx.h b/target/i386/hvf/vmx.h index 75ba1e2a5f..587b1b8375 100644 --- a/target/i386/hvf/vmx.h +++ b/target/i386/hvf/vmx.h @@ -166,6 +166,7 @@ static inline void macvm_set_cr4(hv_vcpuid_t vcpu, uint64_t cr4) wvmcs(vcpu, VMCS_GUEST_CR4, guest_cr4); wvmcs(vcpu, VMCS_CR4_SHADOW, cr4); +wvmcs(vcpu, VMCS_CR4_MASK, CR4_VMXE); hv_vcpu_invalidate_tlb(vcpu); hv_vcpu_flush(vcpu); -- 2.26.2
[PULL 0/6] x86 fixes for -rc1
The following changes since commit ee5128bb00f90dd301991d80d1db5224ce924c84: Merge remote-tracking branch 'remotes/jasowang/tags/net-pull-request' into staging (2020-07-16 13:12:05 +0100) are available in the Git repository at: git://github.com/ehabkost/qemu.git tags/x86-next-pull-request for you to fetch changes up to 818b9f111d64b40661d08f5e23236ac1ca5df505: i386: hvf: Explicitly set CR4 guest/host mask (2020-07-16 14:15:13 -0400) x86 fixes for -rc1 Fixes for x86 that missed hard freeze: * Don't trigger warnings for features set by CPU model versions (Xiaoyao Li) * Missing features in Icelake-Server, Skylake-Server, Cascadelake-Server CPU models (Chenyi Qiang) * Fix hvf x86_64 guest boot crash (Roman Bolshakov) Chenyi Qiang (3): target/i386: add fast short REP MOV support target/i386: fix model number and add missing features for Icelake-Server CPU model target/i386: add the missing vmx features for Skylake-Server and Cascadelake-Server CPU models Roman Bolshakov (1): i386: hvf: Explicitly set CR4 guest/host mask Xiaoyao Li (2): i368/cpu: Clear env->user_features after loading versioned CPU model i386/cpu: Don't add unavailable_features to env->user_features target/i386/cpu.h | 2 ++ target/i386/hvf/vmx.h | 1 + target/i386/cpu.c | 38 -- 3 files changed, 39 insertions(+), 2 deletions(-) -- 2.26.2
[PULL 5/6] target/i386: add the missing vmx features for Skylake-Server and Cascadelake-Server CPU models
From: Chenyi Qiang Add the missing vmx features in Skylake-Server and Cascadelake-Server CPU models based on the output of Paolo's script. Signed-off-by: Chenyi Qiang Message-Id: <20200714084148.26690-4-chenyi.qi...@intel.com> Signed-off-by: Eduardo Habkost --- target/i386/cpu.c | 14 ++ 1 file changed, 14 insertions(+) diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 132ef90421..588f32e136 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -3034,6 +3034,13 @@ static X86CPUDefinition builtin_x86_defs[] = { { /* end of list */ } } }, +{ +.version = 4, +.props = (PropValue[]) { +{ "vmx-eptp-switching", "on" }, +{ /* end of list */ } +} +}, { /* end of list */ } } }, @@ -3158,6 +3165,13 @@ static X86CPUDefinition builtin_x86_defs[] = { { /* end of list */ } }, }, +{ .version = 4, + .note = "ARCH_CAPABILITIES, no TSX", + .props = (PropValue[]) { + { "vmx-eptp-switching", "on" }, + { /* end of list */ } + }, +}, { /* end of list */ } } }, -- 2.26.2
[PULL 3/6] target/i386: add fast short REP MOV support
From: Chenyi Qiang For CPUs support fast short REP MOV[CPUID.(EAX=7,ECX=0):EDX(bit4)], e.g Icelake and Tigerlake, expose it to the guest VM. Reviewed-by: Eduardo Habkost Signed-off-by: Chenyi Qiang Message-Id: <20200714084148.26690-2-chenyi.qi...@intel.com> Signed-off-by: Eduardo Habkost --- target/i386/cpu.h | 2 ++ target/i386/cpu.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 37fffa5cac..e1a5c174dc 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -775,6 +775,8 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS]; #define CPUID_7_0_EDX_AVX512_4VNNIW (1U << 2) /* AVX512 Multiply Accumulation Single Precision */ #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) +/* Fast Short Rep Mov */ +#define CPUID_7_0_EDX_FSRM (1U << 4) /* AVX512 Vector Pair Intersection to a Pair of Mask Registers */ #define CPUID_7_0_EDX_AVX512_VP2INTERSECT (1U << 8) /* SERIALIZE instruction */ diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 93b62b2eca..3885826bc4 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -984,7 +984,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { .type = CPUID_FEATURE_WORD, .feat_names = { NULL, NULL, "avx512-4vnniw", "avx512-4fmaps", -NULL, NULL, NULL, NULL, +"fsrm", NULL, NULL, NULL, "avx512-vp2intersect", NULL, "md-clear", NULL, NULL, NULL, "serialize", NULL, "tsx-ldtrk", NULL, NULL /* pconfig */, NULL, -- 2.26.2
[PULL 2/6] i386/cpu: Don't add unavailable_features to env->user_features
From: Xiaoyao Li Features unavailable due to absent of their dependent features should not be added to env->user_features. env->user_features only contains the feature explicity specified with -feature/+feature by user. Fixes: 99e24dbdaa68 ("target/i386: introduce generic feature dependency mechanism") Signed-off-by: Xiaoyao Li Message-Id: <20200713174436.41070-3-xiaoyao...@intel.com> Signed-off-by: Eduardo Habkost --- target/i386/cpu.c | 1 - 1 file changed, 1 deletion(-) diff --git a/target/i386/cpu.c b/target/i386/cpu.c index caf0334f3a..93b62b2eca 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -6371,7 +6371,6 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp) unavailable_features & env->user_features[d->to.index], "This feature depends on other features that were not requested"); -env->user_features[d->to.index] |= unavailable_features; env->features[d->to.index] &= ~unavailable_features; } } -- 2.26.2
Re: [PATCH for-5.1] i386: hvf: Explicitly set CR4 guest/host mask
On Tue, Jul 14, 2020 at 08:20:04PM +0200, Paolo Bonzini wrote: > Hi Roman, please ask Peter to apply it directly because I won't be able to > send a pull request in the next couple of weeks. > > Paolo > > Il mar 14 lug 2020, 12:39 Roman Bolshakov ha > scritto: > > > On Tue, Jul 14, 2020 at 12:07:27PM +0300, Roman Bolshakov wrote: > > > Removal of register reset omitted initialization of CR4 guest/host mask. > > > x86_64 guests aren't booting without it. > > > > > > Fixes: 5009ef22c6bb2 ("i386: hvf: Don't duplicate register reset") > > > Signed-off-by: Roman Bolshakov > > > > > > > If one has a chance to test or review it, it'd be very helpful. That'd > > allow to include it in RC0. > > I'll queue it for my -rc1 pull request. -- Eduardo
Re: qemu test-qga failure on mergebuild after VERSION file change: dependency issues??
On 7/16/20 7:55 PM, Michael Roth wrote: > Quoting Peter Maydell (2020-07-16 05:53:17) >> The first merge I tried to process after bumping VERSION for rc0 >> failed on test-qga like this: >> >> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} >> tests/test-qga -m=quick -k --tap < /dev/null | ./scripts/tap-driv >> er.pl --test-name="test-qga" >> PASS 1 test-qga /qga/sync-delimited >> PASS 2 test-qga /qga/sync >> PASS 3 test-qga /qga/ping >> ** >> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/test-qga.c:303:test_qga_info: >> assertion failed (version == QEMU_VERSION): ("5.0.9 >> 0" == "5.0.50") >> ERROR test-qga - Bail out! >> ERROR:/home/petmay01/linaro/qemu-for-merges/tests/test-qga.c:303:test_qga_info: >> assertion failed (versio >> n == QEMU_VERSION): ("5.0.90" == "5.0.50") >> Aborted (core dumped) >> /home/petmay01/linaro/qemu-for-merges/tests/Makefile.include:659: >> recipe for target 'check-unit' failed >> >> Looking at timestamps on files, tests/test-qga.o never got rebuilt, >> even though config-host.h has been updated (and so has the new >> QEMU_VERSION). Any idea what's gone wrong here? >> >> Also weird: this build tree has no .d files in it. > > I've been trying to reproduce with: > > make > make check-unit > *bump VERSION > make check-unit > > but test-qga.o gets rebuilt as expected and the test passed. > > This is with ubuntu 18.04, x86, with out-of-tree build directory. Are you > aware > of any other factors that might be needed to reproduce this? The problem is not for qga, it affects all QEMU objects. > >> >> thanks >> -- PMM >
Re: [RFC PATCH] tcg/cpu-exec: precise single-stepping after an exception
On 7/16/20 3:04 AM, Luc Michel wrote: > When single-stepping with a debugger attached to QEMU, and when an > exception is raised, the debugger misses the first instruction after the > exception: > > $ qemu-system-aarch64 -M virt -display none -cpu cortex-a53 -s -S > > $ aarch64-linux-gnu-gdb > GNU gdb (GDB) 9.2 > [...] > (gdb) tar rem :1234 > Remote debugging using :1234 > warning: No executable has been specified and target does not support > determining executable automatically. Try using the "file" command. > 0x in ?? () > (gdb) # writing nop insns to 0x200 and 0x204 > (gdb) set *0x200 = 0xd503201f > (gdb) set *0x204 = 0xd503201f > (gdb) # 0x0 address contains 0 which is an invalid opcode. > (gdb) # The CPU should raise an exception and jump to 0x200 > (gdb) si > 0x0204 in ?? () > > With this commit, the same run steps correctly on the first instruction > of the exception vector: > > (gdb) si > 0x0200 in ?? () > > Signed-off-by: Luc Michel > --- > > RFC because I'm really not sure this is the good place to do that since > EXCP_DEBUG are usually raised in each target translate.c. It could also > have implications with record/replay I'm not aware of. > > --- > accel/tcg/cpu-exec.c | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c > index d95c4848a4..e85fab5d40 100644 > --- a/accel/tcg/cpu-exec.c > +++ b/accel/tcg/cpu-exec.c > @@ -502,10 +502,21 @@ static inline bool cpu_handle_exception(CPUState *cpu, > int *ret) > CPUClass *cc = CPU_GET_CLASS(cpu); > qemu_mutex_lock_iothread(); > cc->do_interrupt(cpu); > qemu_mutex_unlock_iothread(); > cpu->exception_index = -1; > + > +if (unlikely(cpu->singlestep_enabled)) { > +/* > + * After processing the exception, ensure an EXCP_DEBUG is > + * raised when single-stepping so that GDB doesn't miss the > + * next instruction. > + */ > +cpu->exception_index = EXCP_DEBUG; > +return cpu_handle_exception(cpu, ret); Plausible. Although recursion on an inline function is going to defeat the inline, in general. We could expand the recursion by hand with if (unlikely(cpu->singlestep_enabled)) { *ret = EXCP_DEBUG; cpu_handle_debug_exception(cpu); return true; } which might even be clearer. r~ > +} > + > } else if (!replay_has_interrupt()) { > /* give a chance to iothread in replay mode */ > *ret = EXCP_INTERRUPT; > return true; > } >
Re: qemu test-qga failure on mergebuild after VERSION file change: dependency issues??
Quoting Peter Maydell (2020-07-16 05:53:17) > The first merge I tried to process after bumping VERSION for rc0 > failed on test-qga like this: > > MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))} > tests/test-qga -m=quick -k --tap < /dev/null | ./scripts/tap-driv > er.pl --test-name="test-qga" > PASS 1 test-qga /qga/sync-delimited > PASS 2 test-qga /qga/sync > PASS 3 test-qga /qga/ping > ** > ERROR:/home/petmay01/linaro/qemu-for-merges/tests/test-qga.c:303:test_qga_info: > assertion failed (version == QEMU_VERSION): ("5.0.9 > 0" == "5.0.50") > ERROR test-qga - Bail out! > ERROR:/home/petmay01/linaro/qemu-for-merges/tests/test-qga.c:303:test_qga_info: > assertion failed (versio > n == QEMU_VERSION): ("5.0.90" == "5.0.50") > Aborted (core dumped) > /home/petmay01/linaro/qemu-for-merges/tests/Makefile.include:659: > recipe for target 'check-unit' failed > > Looking at timestamps on files, tests/test-qga.o never got rebuilt, > even though config-host.h has been updated (and so has the new > QEMU_VERSION). Any idea what's gone wrong here? > > Also weird: this build tree has no .d files in it. I've been trying to reproduce with: make make check-unit *bump VERSION make check-unit but test-qga.o gets rebuilt as expected and the test passed. This is with ubuntu 18.04, x86, with out-of-tree build directory. Are you aware of any other factors that might be needed to reproduce this? > > thanks > -- PMM
Re: [PULL v1 0/2] Merge tpm 2020/07/15 v1
On Wed, 15 Jul 2020 at 20:23, Stefan Berger wrote: > > Hello! > > This series fixes a couple of minor issues with the PPC64 TPM SPAPR interface > and a test case. > >Stefan > > The following changes since commit 8746309137ba470d1b2e8f5ce86ac228625db940: > > Update version for v5.1.0-rc0 release (2020-07-15 19:08:07 +0100) > > are available in the Git repository at: > > git://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2020-07-15-1 > > for you to fetch changes up to df8a7568932e4c3c930fdfeb228dd72b4bb71a1f: > > tests: tpm: Skip over pcrUpdateCounter byte in result comparison > (2020-07-15 14:57:33 -0400) > > --- > Stefan Berger (2): > tpm: tpm_spapr: Exit on TPM backend failures > tests: tpm: Skip over pcrUpdateCounter byte in result comparison Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1 for any user-visible changes. -- PMM
[GIT PULL] I2C updates
The following changes since commit 45db94cc90c286a9965a285ba19450f448760a09: Merge remote-tracking branch 'remotes/mcayland/tags/qemu-openbios-20200707' into staging (2020-07-10 16:43:40 +0100) are available in the Git repository at: https://github.com/cminyard/qemu.git tags/for-qemu-i2c-5 for you to fetch changes up to 73d5f22ecbb76dfc785876779d47787084ff0f42: hw/i2c: Document the I2C qdev helpers (2020-07-16 12:30:54 -0500) Minor changes to: Add an SMBus config entry Cleanup/simplify/document some I2C interfaces Philippe Mathieu-Daudé (6): hw/i2c/Kconfig: Add an entry for the SMBus hw/i2c/aspeed_i2c: Simplify aspeed_i2c_get_bus() hw/i2c: Rename i2c_try_create_slave() as i2c_slave_new() hw/i2c: Rename i2c_realize_and_unref() as i2c_slave_realize_and_unref() hw/i2c: Rename i2c_create_slave() as i2c_slave_create_simple() hw/i2c: Document the I2C qdev helpers hw/arm/aspeed.c | 82 +++-- hw/arm/musicpal.c | 4 +-- hw/arm/nseries.c| 8 ++--- hw/arm/pxa2xx.c | 5 +-- hw/arm/realview.c | 2 +- hw/arm/spitz.c | 4 +-- hw/arm/stellaris.c | 2 +- hw/arm/tosa.c | 2 +- hw/arm/versatilepb.c| 2 +- hw/arm/vexpress.c | 2 +- hw/arm/z2.c | 4 +-- hw/display/sii9022.c| 2 +- hw/i2c/Kconfig | 8 +++-- hw/i2c/Makefile.objs| 3 +- hw/i2c/aspeed_i2c.c | 3 +- hw/i2c/core.c | 15 - hw/ppc/e500.c | 2 +- hw/ppc/sam460ex.c | 2 +- include/hw/i2c/aspeed_i2c.h | 2 +- include/hw/i2c/i2c.h| 54 +++-- 20 files changed, 131 insertions(+), 77 deletions(-)
Re: [PATCH v3 3/9] vfio: add quirk device write method
On Tue, 30 Jun 2020 at 13:30, P J P wrote: > > From: Prasad J Pandit > > Add vfio quirk device mmio write method to avoid NULL pointer > dereference issue. > > Reported-by: Lei Sun > Reviewed-by: Li Qiang > Signed-off-by: Prasad J Pandit > --- > hw/vfio/pci-quirks.c | 8 > 1 file changed, 8 insertions(+) > > Update v3: Add Reviewed-by: ... > -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg09406.html > > diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c > index d304c81148..cc6d5dbc23 100644 > --- a/hw/vfio/pci-quirks.c > +++ b/hw/vfio/pci-quirks.c > @@ -14,6 +14,7 @@ > #include "config-devices.h" > #include "exec/memop.h" > #include "qemu/units.h" > +#include "qemu/log.h" > #include "qemu/error-report.h" > #include "qemu/main-loop.h" > #include "qemu/module.h" > @@ -264,8 +265,15 @@ static uint64_t vfio_ati_3c3_quirk_read(void *opaque, > return data; > } > > +static void vfio_ati_3c3_quirk_write(void *opaque, hwaddr addr, > +uint64_t data, unsigned size) > +{ > +qemu_log_mask(LOG_GUEST_ERROR, "%s not implemented\n", __func__); > +} > + > static const MemoryRegionOps vfio_ati_3c3_quirk = { > .read = vfio_ati_3c3_quirk_read, > +.write = vfio_ati_3c3_quirk_write, > .endianness = DEVICE_LITTLE_ENDIAN, > }; Alex (Williamson) -- as the vfio maintainer, do you have a view on whether we should be logging write accesses to port 0x3c3 here as guest-errors or unimplemented-QEMU-functionality? Guest-error seems plausible to me, anyway. Reviewed-by: Peter Maydell thanks -- PMM
Re: [PATCH v3 1/9] hw/pci-host: add pci-intack write method
On Tue, 30 Jun 2020 at 13:29, P J P wrote: > > From: Prasad J Pandit > > Add pci-intack mmio write method to avoid NULL pointer dereference > issue. > > Reported-by: Lei Sun > Reviewed-by: Li Qiang > Signed-off-by: Prasad J Pandit > --- > hw/pci-host/prep.c | 8 > 1 file changed, 8 insertions(+) > > Update v3: Add Reviewed-by: ... > -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg09395.html > > diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c > index 367e408b91..3c8ff6af03 100644 > --- a/hw/pci-host/prep.c > +++ b/hw/pci-host/prep.c > @@ -26,6 +26,7 @@ > #include "qemu/osdep.h" > #include "qemu-common.h" > #include "qemu/units.h" > +#include "qemu/log.h" > #include "qapi/error.h" > #include "hw/pci/pci.h" > #include "hw/pci/pci_bus.h" > @@ -119,8 +120,15 @@ static uint64_t raven_intack_read(void *opaque, hwaddr > addr, > return pic_read_irq(isa_pic); > } > > +static void raven_intack_write(void *opaque, hwaddr addr, > +uint64_t data, unsigned size) > +{ > +qemu_log_mask(LOG_UNIMP, "%s not implemented\n", __func__); > +} > + > static const MemoryRegionOps raven_intack_ops = { > .read = raven_intack_read, > +.write = raven_intack_write, > .valid = { > .max_access_size = 1, > }, I suspect this may be a read-only register (and so a guest error rather than unimp) but I'm not sure I've found the correct Raven PCI controller datasheet, and if I have then there's a lot of unimplemented functionality in our model. So UNIMP is fine. This controller is only used by the ppc '40p' machine. Reviewed-by: Peter Maydell thanks -- PMM
Re: [PATCH v3 9/9] memory: assert MemoryRegionOps callbacks are defined
On Tue, 30 Jun 2020 at 13:31, P J P wrote: > > From: Prasad J Pandit > > When registering a MemoryRegionOps object, assert that its > read/write callback methods are defined. This avoids potential > guest crash via a NULL pointer dereference. > > Suggested-by: Peter Maydell > Reviewed-by: Li Qiang > Signed-off-by: Prasad J Pandit > --- Reviewed-by: Peter Maydell thanks -- PMM
[PATCH v4 for-5.2 1/2] spapr: Use error_append_hint() in spapr_caps.c
We have a dedicated error API for hints. Use it instead of embedding the hint in the error message, as recommanded in the "qapi/error.h" header file. Since spapr_caps_apply() passes &error_fatal, all functions must also call the ERRP_GUARD() macro for error_append_hint() to be functional. While here, have cap_fwnmi_apply(), which already uses error_append_hint(), to call ERRP_GUARD() as well. Signed-off-by: Greg Kurz Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Laurent Vivier --- hw/ppc/spapr_caps.c | 89 +-- 1 file changed, 50 insertions(+), 39 deletions(-) diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c index 3225fc5a2edc..275f5bd0342c 100644 --- a/hw/ppc/spapr_caps.c +++ b/hw/ppc/spapr_caps.c @@ -180,24 +180,24 @@ static void spapr_cap_set_pagesize(Object *obj, Visitor *v, const char *name, static void cap_htm_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { +ERRP_GUARD(); if (!val) { /* TODO: We don't support disabling htm yet */ return; } if (tcg_enabled()) { -error_setg(errp, - "No Transactional Memory support in TCG," - " try appending -machine cap-htm=off"); +error_setg(errp, "No Transactional Memory support in TCG"); +error_append_hint(errp, "Try appending -machine cap-htm=off\n"); } else if (kvm_enabled() && !kvmppc_has_cap_htm()) { error_setg(errp, -"KVM implementation does not support Transactional Memory," - " try appending -machine cap-htm=off" -); + "KVM implementation does not support Transactional Memory"); +error_append_hint(errp, "Try appending -machine cap-htm=off\n"); } } static void cap_vsx_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { +ERRP_GUARD(); PowerPCCPU *cpu = POWERPC_CPU(first_cpu); CPUPPCState *env = &cpu->env; @@ -209,13 +209,14 @@ static void cap_vsx_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) * rid of anything that doesn't do VMX */ g_assert(env->insns_flags & PPC_ALTIVEC); if (!(env->insns_flags2 & PPC2_VSX)) { -error_setg(errp, "VSX support not available," - " try appending -machine cap-vsx=off"); +error_setg(errp, "VSX support not available"); +error_append_hint(errp, "Try appending -machine cap-vsx=off\n"); } } static void cap_dfp_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { +ERRP_GUARD(); PowerPCCPU *cpu = POWERPC_CPU(first_cpu); CPUPPCState *env = &cpu->env; @@ -224,8 +225,8 @@ static void cap_dfp_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) return; } if (!(env->insns_flags2 & PPC2_DFP)) { -error_setg(errp, "DFP support not available," - " try appending -machine cap-dfp=off"); +error_setg(errp, "DFP support not available"); +error_append_hint(errp, "Try appending -machine cap-dfp=off\n"); } } @@ -239,6 +240,7 @@ SpaprCapPossible cap_cfpc_possible = { static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { +ERRP_GUARD(); uint8_t kvm_val = kvmppc_get_cap_safe_cache(); if (tcg_enabled() && val) { @@ -247,9 +249,9 @@ static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val, cap_cfpc_possible.vals[val]); } else if (kvm_enabled() && (val > kvm_val)) { error_setg(errp, - "Requested safe cache capability level not supported by kvm," - " try appending -machine cap-cfpc=%s", - cap_cfpc_possible.vals[kvm_val]); + "Requested safe cache capability level not supported by KVM"); +error_append_hint(errp, "Try appending -machine cap-cfpc=%s\n", + cap_cfpc_possible.vals[kvm_val]); } } @@ -263,6 +265,7 @@ SpaprCapPossible cap_sbbc_possible = { static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { +ERRP_GUARD(); uint8_t kvm_val = kvmppc_get_cap_safe_bounds_check(); if (tcg_enabled() && val) { @@ -271,9 +274,9 @@ static void cap_safe_bounds_check_apply(SpaprMachineState *spapr, uint8_t val, cap_sbbc_possible.vals[val]); } else if (kvm_enabled() && (val > kvm_val)) { error_setg(errp, -"Requested safe bounds check capability level not supported by kvm," - " try appending -machine cap-sbbc=%s", - cap_sbbc_possible.vals[kvm_val]); +"Requested safe bounds check capability level not supported by KVM"); +error_append_hint(errp, "Try appending -machine cap-sbbc=%s\n", + cap_sbbc_possible.vals[kvm_val]); } } @@ -290,6 +293,7 @@ SpaprCapPossible cap_i
[PATCH v4 for-5.2 2/2] spapr: Forbid nested KVM-HV in pre-power9 compat mode
Nested KVM HV only works if the kernel is using the radix MMU mode, ie. the CPU is POWER9 and it is not running in some pre-power9 compat mode. Otherwise, the KVM HV module fails to load in the guest with -ENODEV. It might be painful for a user to discover this late that nested cannot work with their setup. Erroring out at machine init instead seems to be the best we can do. Signed-off-by: Greg Kurz Reviewed-by: Laurent Vivier --- hw/ppc/spapr_caps.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c index 275f5bd0342c..10a80a8159f4 100644 --- a/hw/ppc/spapr_caps.c +++ b/hw/ppc/spapr_caps.c @@ -382,6 +382,8 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { ERRP_GUARD(); +PowerPCCPU *cpu = POWERPC_CPU(first_cpu); + if (!val) { /* capability disabled by default */ return; @@ -391,6 +393,14 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr, error_setg(errp, "No Nested KVM-HV support in TCG"); error_append_hint(errp, "Try appending -machine cap-nested-hv=off\n"); } else if (kvm_enabled()) { +if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00, 0, + spapr->max_compat_pvr)) { +error_setg(errp, "Nested KVM-HV only supported on POWER9"); +error_append_hint(errp, + "Try appending -machine max-cpu-compat=power9\n"); +return; +} + if (!kvmppc_has_cap_nested_kvm_hv()) { error_setg(errp, "KVM implementation does not support Nested KVM-HV");
[PATCH v4 for-5.2 0/2] spapr: Improve error reporting in spapr_caps.c
Nested KVM HV only works if the kernel is using the radix MMU mode, ie. the CPU is POWER9 and it is not running in some pre-power9 compat mode. Otherwise, the KVM HV module fails to load in the guest with -ENODEV. It might be painful for a user to discover this late that nested cannot work with their setup. It seems a better fit for QEMU to do a sanity check when applying the nested-hv sPAPR capability and print out an informative error message. sPAPR capabilities are checked at machine init. If a capability cannot be used, an error message is printed and QEMU exits. In most places, the error message also contains an hint for the user. But we should use error_append_hint() for that, as explained in the "qapi/error.h" header. So this series first converts spapr_caps.c to using error_append_hint(). This requires to add some ERRP_GUARD() because spapr_caps_apply() passes &error_fatal. Then it adds a sanity check for the nested-hv case with an error message and hint. v4: - Same as v3 but rebased on ppc-for-5.2, updated changelogs and cover v3: - Add preliminary patch to use warn_report() instead of a convoluted error_setg()+warn_report_err() sequence v2: - Fix indentation and add some missing \n in patch 2 - Add ERRP_AUTO_PROPAGATE() to cap_nested_kvm_hv_apply() in patch 2 instead of patch 3 --- Greg Kurz (2): spapr: Use error_append_hint() in spapr_caps.c spapr: Forbid nested KVM-HV in pre-power9 compat mode hw/ppc/spapr_caps.c | 99 +++ 1 file changed, 60 insertions(+), 39 deletions(-) -- Greg
Re: [PATCH 2/2] python/qemu: Change ConsoleSocket to optionally drain socket.
On Thu, 16 Jul 2020 at 09:42, Alex Bennée wrote: > > > +self._drain_thread = None > > +socket.socket.__init__(self, socket.AF_UNIX, socket.SOCK_STREAM) > > +self.connect(address) > > +self._drain = drain > > We end up with two variables that represent the fact we have draining > happening. Could we rationalise it into: > > if drain: > self._drain_thread = self._thread_start() > else > self._drain_thread = None # if this is needed > > And then tests for: > > if not self._drain: > > become > > if self._drain_thread is None: Good point, this is simpler. Will update. > > +if self._drain and self._drain_thread is not None: > > +thread, self._drain_thread = self._drain_thread, None > Would self._drain ever not have self._drain_thread set? No, I believe that if drain is set, it results in _drain_thread also being set. This will be cleaned up once we drop the _drain. > > > thread.join() > > +socket.socket.close(self) > > > diff --git a/python/qemu/machine.py b/python/qemu/machine.py > > index 6769359766..62709d86e4 100644 > > --- a/python/qemu/machine.py > > +++ b/python/qemu/machine.py > > @@ -22,7 +22,6 @@ import logging > > import os > > import subprocess > > import shutil > > -import socket > > FYI minor patch conflict here with master OK, will rebase and fix this conflict. Thanks & Regards, -Rob > > > import tempfile > > from typing import Optional, Type > > from types import TracebackType > > @@ -591,12 +590,8 @@ class QEMUMachine: > > Returns a socket connected to the console > > """ > > if self._console_socket is None: > > -if self._drain_console: > > -self._console_socket = console_socket.ConsoleSocket( > > -self._console_address, > > -file=self._console_log_path) > > -else: > > -self._console_socket = socket.socket(socket.AF_UNIX, > > - socket.SOCK_STREAM) > > -self._console_socket.connect(self._console_address) > > +self._console_socket = console_socket.ConsoleSocket( > > +self._console_address, > > +file=self._console_log_path, > > +drain=self._drain_console) > > return self._console_socket > > > -- > Alex Bennée
Re: [PATCH v3 8/9] imx7-ccm: add digprog mmio write method
On Thu, 16 Jul 2020 at 17:55, P J P wrote: > > +-- On Thu, 16 Jul 2020, Peter Maydell wrote --+ > | > +static void imx7_digprog_write(void *opaque, hwaddr addr, > | > +uint64_t data, unsigned size) > | > +{ > | > +qemu_log_mask(LOG_UNIMP, "%s not implemented\n", __func__); > | > +} > | > | This covers a single register (DIGPROG) which is read-only (it returns a > | silicon revision number). So this is not a LOG_UNIMP, but a LOG_GUEST_ERROR: > | > | qemu_log_mask(LOG_GUEST_ERROR, "Guest write to read-only > | ANALOG_DIGPROG register\n"); > > Should this be g_assert_not_reached() in that case? No, because a malicious guest can write to the register (and cause the function to be called), it is merely that it is a bug in guest code for it to do that. -- PMM
Re: [PATCH v3 8/9] imx7-ccm: add digprog mmio write method
+-- On Thu, 16 Jul 2020, Peter Maydell wrote --+ | > +static void imx7_digprog_write(void *opaque, hwaddr addr, | > +uint64_t data, unsigned size) | > +{ | > +qemu_log_mask(LOG_UNIMP, "%s not implemented\n", __func__); | > +} | | This covers a single register (DIGPROG) which is read-only (it returns a | silicon revision number). So this is not a LOG_UNIMP, but a LOG_GUEST_ERROR: | | qemu_log_mask(LOG_GUEST_ERROR, "Guest write to read-only | ANALOG_DIGPROG register\n"); Should this be g_assert_not_reached() in that case? Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH] introduce VFIO-over-socket protocol specificaion
Patchew URL: https://patchew.org/QEMU/1594913503-52271-1-git-send-email-thanos.maka...@nutanix.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 1594913503-52271-1-git-send-email-thanos.maka...@nutanix.com Subject: [PATCH] introduce VFIO-over-socket protocol specificaion === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' aa6155e introduce VFIO-over-socket protocol specificaion === OUTPUT BEGIN === WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #20: new file mode 100644 ERROR: trailing whitespace #167: FILE: docs/devel/vfio-over-socket.rst:143: +configuration space. $ ERROR: trailing whitespace #360: FILE: docs/devel/vfio-over-socket.rst:336: +|| +-++ | $ ERROR: trailing whitespace #572: FILE: docs/devel/vfio-over-socket.rst:548: +be supported in future versions. $ ERROR: trailing whitespace #587: FILE: docs/devel/vfio-over-socket.rst:563: +| Command | 5| $ ERROR: trailing whitespace #874: FILE: docs/devel/vfio-over-socket.rst:850: +the interrupt. $ ERROR: trailing whitespace #878: FILE: docs/devel/vfio-over-socket.rst:854: +guest unmasks the interrupt. $ ERROR: trailing whitespace #908: FILE: docs/devel/vfio-over-socket.rst:884: +appears after the 16 byte message header. $ total: 7 errors, 1 warnings, 1135 lines checked Commit aa6155e147a9 (introduce VFIO-over-socket protocol specificaion) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/1594913503-52271-1-git-send-email-thanos.maka...@nutanix.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH] introduce VFIO-over-socket protocol specificaion
Patchew URL: https://patchew.org/QEMU/1594913503-52271-1-git-send-email-thanos.maka...@nutanix.com/ Hi, This series failed the 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. === TEST SCRIPT BEGIN === #! /bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-mingw@fedora J=14 NETWORK=1 === TEST SCRIPT END === CC block/qapi.o CC block/file-win32.o Warning, treated as error: /tmp/qemu-test/src/docs/devel/vfio-over-socket.rst:281:Malformed table. +--+-+---+ --- CC crypto/hmac.o CC crypto/hmac-nettle.o CC crypto/desrfb.o make: *** [Makefile:1090: docs/devel/index.html] Error 2 make: *** Waiting for unfinished jobs Traceback (most recent call last): File "./tests/docker/docker.py", line 708, in --- raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=739a80f85e9146d59e3826f0d63daa6c', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-gwtqm3ly/src/docker-src.2020-07-16-12.51.55.8263:/var/tmp/qemu:z,ro', 'qemu/fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2. filter=--filter=label=com.qemu.instance.uuid=739a80f85e9146d59e3826f0d63daa6c make[1]: *** [docker-run] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-gwtqm3ly/src' make: *** [docker-run-test-mingw@fedora] Error 2 real3m4.939s user0m8.716s The full log is available at http://patchew.org/logs/1594913503-52271-1-git-send-email-thanos.maka...@nutanix.com/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH v3 2/9] pci-host: add pcie-msi read method
On Tue, 30 Jun 2020 at 13:30, P J P wrote: > > From: Prasad J Pandit > > Add pcie-msi mmio read method to avoid NULL pointer dereference > issue. This change is specific to the designware pci host controller; it would be nice to have "designware" in the commit subject. > Reported-by: Lei Sun > Reviewed-by: Li Qiang > Signed-off-by: Prasad J Pandit > --- > hw/pci-host/designware.c | 9 + > 1 file changed, 9 insertions(+) > > Update v3: Add Reviewed-by: ... > -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg09400.html > +static uint64_t designware_pcie_root_msi_read(void *opaque, hwaddr addr, > + unsigned size) > +{ > +qemu_log_mask(LOG_UNIMP, "%s not implemented\n", __func__); > +return 0; This is the right change, but is missing an explanation of why it's right: Looking at the data sheet, in the real hardware MSI interrupts are handled by looking at writes to see whether they match the configured address; if so then the write gets quashed and never gets put out onto the AXI bus (to the CPU, memory, etc). This only happens for writes, so reads from the magic address are just allowed to pass through and will read from the system address space like any other read from any other address. That's not trivial to implement, though, and well-behaved guests won't care, so we can just explain why we're not doing anything with a suitable comment: /* * Attempts to read from the MSI address are undefined in * the PCI specifications. For this hardware, the datasheet * specifies that a read from the magic address is simply not * intercepted by the MSI controller, and will go out to the * AHB/AXI bus like any other PCI-device-initiated DMA read. * This is not trivial to implement in QEMU, so since * well-behaved guests won't ever ask a PCI device to DMA from * this address we just log the missing functionality. */ qemu_log_mask(LOG_UNIMP, "%s not implemented\n", __func__); return 0; > +} > + > static void designware_pcie_root_msi_write(void *opaque, hwaddr addr, > uint64_t val, unsigned len) > { > @@ -77,6 +85,7 @@ static void designware_pcie_root_msi_write(void *opaque, > hwaddr addr, > } > > static const MemoryRegionOps designware_pci_host_msi_ops = { > +.read = designware_pcie_root_msi_read, > .write = designware_pcie_root_msi_write, > .endianness = DEVICE_LITTLE_ENDIAN, > .valid = { With that comment, Reviewed-by: Peter Maydell thanks -- PMM
[PATCH] gitlab-ci.yml: Add oss-fuzz build tests
This tries to build and run the fuzzers with the same build-script used by oss-fuzz. This doesn't guarantee that the builds on oss-fuzz will also succeed, since oss-fuzz provides its own compiler and fuzzer vars, but it can catch changes that are not compatible with the the ./scripts/oss-fuzz/build.sh script. The strange way of finding fuzzer binaries stems from the method used by oss-fuzz: https://github.com/google/oss-fuzz/blob/master/infra/base-images/base-runner/targets_list Signed-off-by: Alexander Bulekov --- Similar to Thomas' patch: > Note: This patch needs two other patches merged first to work correctly: > - 'fuzz: Expect the cmdline in a freeable GString' from Alexander > - 'qom: Plug memory leak in "info qom-tree"' from Markus Otherwise the test will fail due to detected memory leaks. Fair warning: I haven't been able to trigger this new job yet. I tried to run the pipeline with these changes on my forked repo on gitlab, but did not reach the build-oss-fuzz. I think this is due to some failures in the Containers-layer-2 stage: ... Error response from daemon: manifest for registry.gitlab.com/a1xndr/qemu/qemu/debian-all-test-cross:latest not found: manifest unknown: manifest unknown #2 [internal] load .dockerignore #2 transferring context: #2 transferring context: 2B 0.1s done #2 DONE 0.1s #1 [internal] load build definition from tmpg8j4xoop.docker #1 transferring dockerfile: 2.21kB 0.1s done #1 DONE 0.2s #3 [internal] load metadata for docker.io/qemu/debian10:latest #3 ERROR: pull access denied, repository does not exist or may require authorization: server message: insufficient_scope: authorization failed #4 [1/3] FROM docker.io/qemu/debian10:latest #4 resolve docker.io/qemu/debian10:latest 0.1s done #4 ERROR: pull access denied, repository does not exist or may require authorization: server message: insufficient_scope: authorization failed -- > [internal] load metadata for docker.io/qemu/debian10:latest: -- -- > [1/3] FROM docker.io/qemu/debian10:latest: -- failed to solve with frontend dockerfile.v0: failed to build LLB: failed to load cache key: pull access denied, repository does not exist or may require authorization: server message: insufficient_scope: authorization failed ... .gitlab-ci.yml | 14 ++ 1 file changed, 14 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index e96f8794b9..a50df420c9 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -182,6 +182,20 @@ build-fuzzer: || exit 1 ; done +build-oss-fuzz: + <<: *native_build_job_definition + variables: +IMAGE: fedora + script: +- OUT_DIR="./build" CC=clang-9 CXX=clang++-9 CFLAGS="-fsanitize=address" + LIB_FUZZING_ENGINE="-fsanitize=fuzzer" CFL + ./scripts/oss-fuzz/build.sh +- for fuzzer in $(find ./build-oss-fuzz/DEST_DIR/ -executable -type f); do +grep "LLVMFuzzerTestOneInput" ${fuzzer} > /dev/null 2>&1 || continue ; +echo Testing ${fuzzer} ... ; +"${fuzzer}" -runs=1000 || exit 1 ; + done + build-tci: <<: *native_build_job_definition variables: -- 2.26.2
Re: [PATCH] gitlab-ci.yml: Add fuzzer tests
On 200716 1209, Thomas Huth wrote: > So far we neither compile-tested nor run any of the new fuzzers in our CI, > which led to some build failures of the fuzzer code in the past weeks. > To avoid this problem, add a job to compile the fuzzer code and run some > loops (which likely don't find any new bugs via fuzzing, but at least we > know that the code can still be run). > > A nice side-effect of this test is that the leak tests are enabled here, > so we should now notice some of the memory leaks in our code base earlier. > > Signed-off-by: Thomas Huth Thank you for this, Thomas. I just sent a patch to add a job that runs similar tests with the build-script that we use on oss-fuzz Patch: <20200716163330.29141-1-alx...@bu.edu> I know that these jobs have a lot of overlap, but there are enough quirks in the oss-fuzz build-script that I, personally, don't think they are redundant. A couple notes below, and I haven't been able to test on my own fork of qemu on gitlab, yet due to some pipeline errors, but otherwise Reviewed-by: Alexander Bulekov > --- > .gitlab-ci.yml | 20 +++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > index 5eeba2791b..e96f8794b9 100644 > --- a/.gitlab-ci.yml > +++ b/.gitlab-ci.yml > @@ -161,9 +161,27 @@ build-clang: > IMAGE: fedora > CONFIGURE_ARGS: --cc=clang --cxx=clang++ > TARGETS: alpha-softmmu arm-softmmu m68k-softmmu mips64-softmmu > - ppc-softmmu s390x-softmmu x86_64-softmmu arm-linux-user > + ppc-softmmu s390x-softmmu arm-linux-user > MAKE_CHECK_ARGS: check > > +build-fuzzer: > + <<: *native_build_job_definition > + variables: > +IMAGE: fedora > + script: > +- mkdir build > +- cd build > +- ../configure --cc=clang --cxx=clang++ --enable-fuzzing > + --target-list=x86_64-softmmu https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg02310.html When/if this gets merged, enable-fuzzing won't build with AddressSanitizer, by default, so I would add --enable-sanitizers, just to be safe. > +- make -j"$JOBS" all check-build x86_64-softmmu/fuzz > +- make check > +- for fuzzer in i440fx-qos-fork-fuzz i440fx-qos-noreset-fuzz > +i440fx-qtest-reboot-fuzz virtio-scsi-flags-fuzz virtio-scsi-fuzz ; do Any reason for these particular fuzzers? I know the virtio-net ones find crashes pretty quickly, but I dont think that causes a non-zero exit. > + echo Testing ${fuzzer} ... ; > + x86_64-softmmu/qemu-fuzz-x86_64 --fuzz-target=${fuzzer} -runs=1000 > +|| exit 1 ; > + done > + > build-tci: ><<: *native_build_job_definition >variables: > -- > 2.18.1 >
Re: [PATCH v3] spapr: Add a new level of NUMA for GPUs
On 7/16/20 1:00 PM, Reza Arbab wrote: On Thu, Jul 16, 2020 at 06:42:11AM -0300, Daniel Henrique Barboza wrote: Which would translate here to: uint32_t associativity[] = { cpu_to_be32(0x4), cpu_to_be32(nvslot->numa_id), cpu_to_be32(nvslot->numa_id), cpu_to_be32(nvslot->numa_id), cpu_to_be32(nvslot->numa_id), }; Sure, that's how it originally was in v1 of the patch. Yeah, Greg commented this in v2 about this chunk: This is a guest visible change. It should theoretically be controlled with a compat property of the PHB (look for "static GlobalProperty" in spapr.c). But since this code is only used for GPU passthrough and we don't support migration of such devices, I guess it's okay. Maybe just mention it in the changelog. -- By all means you can mention in changelog/code comment why the associativity of the GPU is nvslot->numa_id 4 times in a row, but I believe this format is still clearer for us to understand. It also makes it equal to skiboot. And it deprecates the SPAPR_GPU_NUMA_ID macro, allowing us to use its value (1) for other internal purposes regarding NUMA without collision with the GPU semantics. Thanks, DHB I'll send a v4 today. It's been a while so I need to rebase anyway.
Re: [PATCH v3 6/9] spapr_pci: add spapr msi read method
On Tue, 30 Jun 2020 at 13:31, P J P wrote: > > From: Prasad J Pandit > > Add spapr msi mmio read method to avoid NULL pointer dereference > issue. > > Reported-by: Lei Sun > Acked-by: David Gibson > Reviewed-by: Li Qiang > Signed-off-by: Prasad J Pandit > --- > hw/ppc/spapr_pci.c | 13 +++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > Update v3: Add Acked-by: ... > -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg08054.html > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 329002ac04..7033352834 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -52,6 +52,7 @@ > #include "sysemu/kvm.h" > #include "sysemu/hostmem.h" > #include "sysemu/numa.h" > +#include "qemu/log.h" > > /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */ > #define RTAS_QUERY_FN 0 > @@ -738,6 +739,12 @@ static PCIINTxRoute spapr_route_intx_pin_to_irq(void > *opaque, int pin) > return route; > } > > +static uint64_t spapr_msi_read(void *opaque, hwaddr addr, unsigned size) > +{ > +qemu_log_mask(LOG_UNIMP, "%s not implemented\n", __func__); > +return 0; > +} > + > /* > * MSI/MSIX memory region implementation. > * The handler handles both MSI and MSIX. > @@ -755,8 +762,10 @@ static void spapr_msi_write(void *opaque, hwaddr addr, > } > > static const MemoryRegionOps spapr_msi_ops = { > -/* There is no .read as the read result is undefined by PCI spec */ > -.read = NULL, > +/* .read result is undefined by PCI spec QEMU multiline comments should have the '/*' on a line of its own. > + * define .read method to avoid assert failure in memory_region_init_io > + */ If this is undefined behaviour per the PCI spec then LOG_UNIMP is the wrong thing -- this should either be LOG_GUEST_ERROR (if the guest can do this or program the h/w to do this) or assert() (if the only way this could happen would be a bug in a QEMU model of a PCI device). > +.read = spapr_msi_read, > .write = spapr_msi_write, > .endianness = DEVICE_LITTLE_ENDIAN > }; > -- > 2.26.2 thanks -- PMM
Re: [PATCH] target/i386: floatx80: avoid compound literals in static initializers
Laszlo Ersek writes: > Quoting ISO C99 6.7.8p4, "All the expressions in an initializer for an > object that has static storage duration shall be constant expressions or > string literals". > > The compound literal produced by the make_floatx80() macro is not such a > constant expression, per 6.6p7-9. (An implementation may accept it, > according to 6.6p10, but is not required to.) > > Therefore using "floatx80_zero" and make_floatx80() for initializing > "f2xm1_table" and "fpatan_table" is not portable. And gcc-4.8 in RHEL-7.6 > actually chokes on them: > >> target/i386/fpu_helper.c:871:5: error: initializer element is not constant >> { make_floatx80(0xbfff, 0x8000ULL), >> ^ > > We've had the make_floatx80_init() macro for this purpose since commit > 3bf7e40ab914 ("softfloat: fix for C99", 2012-03-17), so let's use that > macro again. > > Fixes: eca30647fc07 > Fixes: ff57bb7b6326 > Link: https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg06566.html > Link: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04714.html > Cc: Alex Bennée > Cc: Aurelien Jarno > Cc: Eduardo Habkost > Cc: Joseph Myers > Cc: Paolo Bonzini > Cc: Peter Maydell > Cc: Richard Henderson > Signed-off-by: Laszlo Ersek > --- > > Notes: > I can see that there are test cases under "tests/tcg/i386", but I don't > know how to run them. You can run the TCG tests with: make check-tcg or more specifically: make run-tcg-tests-i386-linux-user there is also a: make check-softfloat although in this case nothing is affected. softfloat bits: Acked-by: Alex Bennée > > include/fpu/softfloat.h | 1 + > target/i386/fpu_helper.c | 426 +++ > 2 files changed, 214 insertions(+), 213 deletions(-) > > diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h > index f1a19df066b7..659218b5c787 100644 > --- a/include/fpu/softfloat.h > +++ b/include/fpu/softfloat.h > @@ -822,6 +822,7 @@ static inline bool floatx80_invalid_encoding(floatx80 a) > } > > #define floatx80_zero make_floatx80(0x, 0xLL) > +#define floatx80_zero_init make_floatx80_init(0x, 0xLL) > #define floatx80_one make_floatx80(0x3fff, 0x8000LL) > #define floatx80_ln2 make_floatx80(0x3ffe, 0xb17217f7d1cf79acLL) > #define floatx80_pi make_floatx80(0x4000, 0xc90fdaa22168c235LL) > diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c > index f5e6c4b88d4e..4ea73874d836 100644 > --- a/target/i386/fpu_helper.c > +++ b/target/i386/fpu_helper.c > @@ -868,201 +868,201 @@ struct f2xm1_data { > }; > > static const struct f2xm1_data f2xm1_table[65] = { > -{ make_floatx80(0xbfff, 0x8000ULL), > - make_floatx80(0x3ffe, 0x8000ULL), > - make_floatx80(0xbffe, 0x8000ULL) }, > -{ make_floatx80(0xbffe, 0xf8002e7eULL), > - make_floatx80(0x3ffe, 0x82cd8698ac2b9160ULL), > - make_floatx80(0xbffd, 0xfa64f2cea7a8dd40ULL) }, > -{ make_floatx80(0xbffe, 0xefffe960ULL), > - make_floatx80(0x3ffe, 0x85aac367cc488345ULL), > - make_floatx80(0xbffd, 0xf4aa7930676ef976ULL) }, > -{ make_floatx80(0xbffe, 0xe8006f10ULL), > - make_floatx80(0x3ffe, 0x88980e8092da5c14ULL), > - make_floatx80(0xbffd, 0xeecfe2feda4b47d8ULL) }, > -{ make_floatx80(0xbffe, 0xe0008a45ULL), > - make_floatx80(0x3ffe, 0x8b95c1e3ea8ba2a5ULL), > - make_floatx80(0xbffd, 0xe8d47c382ae8bab6ULL) }, > -{ make_floatx80(0xbffe, 0xd7ff8a9eULL), > - make_floatx80(0x3ffe, 0x8ea4398b45cd8116ULL), > - make_floatx80(0xbffd, 0xe2b78ce97464fdd4ULL) }, > -{ make_floatx80(0xbffe, 0xd00019a0ULL), > - make_floatx80(0x3ffe, 0x91c3d373ab11b919ULL), > - make_floatx80(0xbffd, 0xdc785918a9dc8dceULL) }, > -{ make_floatx80(0xbffe, 0xc7ff14dfULL), > - make_floatx80(0x3ffe, 0x94f4efa8fef76836ULL), > - make_floatx80(0xbffd, 0xd61620ae02112f94ULL) }, > -{ make_floatx80(0xbffe, 0xc0006530ULL), > - make_floatx80(0x3ffe, 0x9837f0518db87fbbULL), > - make_floatx80(0xbffd, 0xcf901f5ce48f008aULL) }, > -{ make_floatx80(0xbffe, 0xb7ff1723ULL), > - make_floatx80(0x3ffe, 0x9b8d39b9d54eb74cULL), > - make_floatx80(0xbffd, 0xc8e58c8c55629168ULL) }, > -{ make_floatx80(0xbffe, 0xb000b5e1ULL), > - make_floatx80(0x3ffe, 0x9ef5326091a0c366ULL), > - make_floatx80(0xbffd, 0xc2159b3edcbe7934ULL) }, > -{ make_floatx80(0xbffe, 0xa8006f8aULL), > - make_floatx80(0x3ffe, 0xa27043030c49370aULL), > - make_floatx80(0xbffd, 0xbb1f79f9e76d91ecULL) }, > -{ make_floatx80(0xbffe, 0x9fff816aULL), > - make_floatx80(0x3ffe, 0xa5fed6a9b15171cfULL), > - make_floatx80(0xbffd, 0xb40252ac9d5d1c62ULL) }, > -{ make_floatx80(0xbffe, 0x97ffb621ULL), > - make_floatx80(0x3ffe, 0xa9a15ab4ea7c30e6ULL), > - make_floatx80(0xbffd, 0xacbd
Re: [PATCH v3 5/9] nvram: add nrf51_soc flash read method
On Tue, 30 Jun 2020 at 13:31, P J P wrote: > > From: Prasad J Pandit > > Add nrf51_soc mmio read method to avoid NULL pointer dereference > issue. > > Reported-by: Lei Sun > Signed-off-by: Prasad J Pandit > --- > hw/nvram/nrf51_nvm.c | 5 + > 1 file changed, 5 insertions(+) > > Update v3: use g_assert_not_reached() > -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg09560.html > > diff --git a/hw/nvram/nrf51_nvm.c b/hw/nvram/nrf51_nvm.c > index f2283c1a8d..82e89d965b 100644 > --- a/hw/nvram/nrf51_nvm.c > +++ b/hw/nvram/nrf51_nvm.c > @@ -273,6 +273,10 @@ static const MemoryRegionOps io_ops = { > .endianness = DEVICE_LITTLE_ENDIAN, > }; > > +static uint64_t flash_read(void *opaque, hwaddr offset, unsigned size) > +{ Could use a comment about why this is unreachable, since it's not totally obvious: /* * This is a rom_device MemoryRegion which is always in * romd_mode (we never put it in MMIO mode), so reads always * go directly to RAM and never come here. */ > +g_assert_not_reached(); > +} > Otherwise Reviewed-by: Peter Maydell thanks -- PMM
Re: [PATCH v2 5/9] nvram: add nrf51_soc flash read method
On Mon, 29 Jun 2020 at 12:18, Li Qiang wrote: > > P J P 于2020年6月25日周四 上午3:01写道: > > > > From: Prasad J Pandit > > > > Add nrf51_soc mmio read method to avoid NULL pointer dereference > > issue. > > > > Reported-by: Lei Sun > > Signed-off-by: Prasad J Pandit > > --- > > hw/nvram/nrf51_nvm.c | 8 > > 1 file changed, 8 insertions(+) > > > > Update v2: return ldl_le_p() > > -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg04972.html > > > > diff --git a/hw/nvram/nrf51_nvm.c b/hw/nvram/nrf51_nvm.c > > index f2283c1a8d..8000ed530a 100644 > > --- a/hw/nvram/nrf51_nvm.c > > +++ b/hw/nvram/nrf51_nvm.c > > @@ -273,6 +273,13 @@ static const MemoryRegionOps io_ops = { > > .endianness = DEVICE_LITTLE_ENDIAN, > > }; > > > > +static uint64_t flash_read(void *opaque, hwaddr offset, unsigned size) > > +{ > > +NRF51NVMState *s = NRF51_NVM(opaque); > > + > > +assert(offset + size <= s->flash_size); > > +return ldl_le_p(s->storage + offset); > > +} > > The 'flash_ops' is for ROM, though I don't see where it calls > 'memory_region_rom_device_set_romd' > to ROMD, so this MR is in MMIO mode and it needs a read callback. I think that 'romd mode' (ie reads-go-directly-to-RAM) is the default: memory_region_initfn() sets romd_mode to true. So unless the device actively calls memory_region_rom_device_set_romd(mr, false) then the read callback can't be reached. thanks -- PMM
Re: TB Cache size grows out of control with qemu 5.0
Christian Ehrhardt writes: > On Wed, Jul 15, 2020 at 5:58 PM BALATON Zoltan wrote: > >> See commit 47a2def4533a2807e48954abd50b32ecb1aaf29a and the next two >> following it. >> > > Thank you Zoltan for pointing out this commit, I agree that this seems to be > the trigger for the issues I'm seeing. Unfortunately the common CI host size > is 1-2G. For example on Ubuntu Autopkgtests 1.5G. > Those of them running guests do so in 0.5-1G size in TCG mode > (as they often can't rely on having KVM available). > > The 1G TB buffer + 0.5G actual guest size + lack of dynamic downsizing > on memory pressure (never existed) makes these systems go OOM-Killing > the qemu process. Ooops. I admit the assumption was that most people running system emulation would be doing it on beefier machines. > The patches indicated that the TB flushes on a full guest boot are a > good indicator of the TB size efficiency. From my old checks I had: > > - Qemu 4.2 512M guest with 32M default overwritten by ram-size/4 > TB flush count 14, 14, 16 > - Qemu 5.0 512M guest with 1G default > TB flush count 1, 1, 1 > > I agree that ram/4 seems odd, especially on huge guests that is a lot > potentially wasted. And most environments have a bit of breathing > room 1G is too big in small host systems and the common CI system falls > into this category. So I tuned it down to 256M for a test. > > - Qemu 4.2 512M guest with tb-size 256M > TB flush count 5, 5, 5 > - Qemu 5.0 512M guest with tb-size 256M > TB flush count 5, 5, 5 > - Qemu 5.0 512M guest with 256M default in code > TB flush count 5, 5, 5 > > So performance wise the results are as much in-between as you'd think from a > TB size in between. And the memory consumption which (for me) is the actual > current issue to fix would be back in line again as expected. So I'm glad you have the workaround. > > So on one hand I'm suggesting something like: > --- a/accel/tcg/translate-all.c > +++ b/accel/tcg/translate-all.c > @@ -944,7 +944,7 @@ static void page_lock_pair(PageDesc **re > * Users running large scale system emulation may want to tweak their > * runtime setup via the tb-size control on the command line. > */ > -#define DEFAULT_CODE_GEN_BUFFER_SIZE_1 (1 * GiB) > +#define DEFAULT_CODE_GEN_BUFFER_SIZE_1 (256 * MiB) The problem we have is any number we pick here is arbitrary. And while it did regress your use-case changing it again just pushes a performance regression onto someone else. The most (*) 64 bit desktop PCs have 16Gb of RAM, almost all have more than 8gb. And there is a workaround. * random number from Steams HW survey. > #endif > #endif > > OTOH I understand someone else might want to get the more speedy 1G > especially for large guests. If someone used to run a 4G guest in TCG the > TB Size was 1G all along. > How about picking the smaller of (1G || ram-size/4) as default? > > This might then look like: > --- a/accel/tcg/translate-all.c > +++ b/accel/tcg/translate-all.c > @@ -956,7 +956,12 @@ static inline size_t size_code_gen_buffe > { > /* Size the buffer. */ > if (tb_size == 0) { > -tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE; > +unsigned long max_default = (unsigned long)(ram_size / 4); > +if (max_default < DEFAULT_CODE_GEN_BUFFER_SIZE) { > +tb_size = max_default; > +} else { > + tb_size = DEFAULT_CODE_GEN_BUFFER_SIZE; > +} > } > if (tb_size < MIN_CODE_GEN_BUFFER_SIZE) { > tb_size = MIN_CODE_GEN_BUFFER_SIZE; > > This is a bit more tricky than it seems as ram_sizes is no more > present in that context but it is enough to discuss it. > That should serve all cases - small and large - better as a pure > static default of 1G or always ram/4? I'm definitely against re-introducing ram_size into the mix. The original commit (a1b18df9a4) that broke this introduced an ordering dependency which we don't want to bring back. I'd be more amenable to something that took into account host memory and limited the default if it was smaller than a threshold. Is there a way to probe that that doesn't involve slurping /proc/meminfo? > > P.S. I added Alex being the Author of the offending patch and Richard/Paolo > for being listed in the Maintainers file for TCG. -- Alex Bennée
Re: [PATCH v3 8/9] imx7-ccm: add digprog mmio write method
On Tue, 30 Jun 2020 at 13:31, P J P wrote: > > From: Prasad J Pandit > > Add digprog mmio write method to avoid assert failure during > initialisation. > > Reviewed-by: Li Qiang > Signed-off-by: Prasad J Pandit > --- > hw/misc/imx7_ccm.c | 7 +++ > 1 file changed, 7 insertions(+) > > Update v3: Add Reviewed-by: ... > -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg09452.html > > diff --git a/hw/misc/imx7_ccm.c b/hw/misc/imx7_ccm.c > index 02fc1ae8d0..5ac5ecf74c 100644 > --- a/hw/misc/imx7_ccm.c > +++ b/hw/misc/imx7_ccm.c > @@ -131,8 +131,15 @@ static const struct MemoryRegionOps imx7_set_clr_tog_ops > = { > }, > }; > > +static void imx7_digprog_write(void *opaque, hwaddr addr, > +uint64_t data, unsigned size) > +{ > +qemu_log_mask(LOG_UNIMP, "%s not implemented\n", __func__); > +} This covers a single register (DIGPROG) which is read-only (it returns a silicon revision number). So this is not a LOG_UNIMP, but a LOG_GUEST_ERROR: qemu_log_mask(LOG_GUEST_ERROR, "Guest write to read-only ANALOG_DIGPROG register\n"); thanks -- PMM
Re: [PATCH for-5.1] qapi: Fix visit_type_STRUCT() not to fail for null object
Markus Armbruster 于2020年7月16日周四 下午11:07写道: > > To make deallocating partially constructed objects work, the > visit_type_STRUCT() need to succeed without doing anything when passed > a null object. > > Commit cdd2b228b9 "qapi: Smooth visitor error checking in generated > code" broke that. To reproduce, run tests/test-qobject-input-visitor > with AddressSanitizer: > > ==4353==ERROR: LeakSanitizer: detected memory leaks > > Direct leak of 16 byte(s) in 1 object(s) allocated from: > #0 0x7f192d0c5d28 in __interceptor_calloc > (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xded28) > #1 0x7f192cd21b10 in g_malloc0 > (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x51b10) > #2 0x556725f6bbee in visit_next_list qapi/qapi-visit-core.c:86 > #3 0x556725f49e15 in visit_type_UserDefOneList > tests/test-qapi-visit.c:474 > #4 0x556725f4489b in test_visitor_in_fail_struct_in_list > tests/test-qobject-input-visitor.c:1086 > #5 0x7f192cd42f29 > (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x72f29) > > SUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s). > > Test case /visitor/input/fail/struct-in-list feeds a list with a bad > element to the QObject input visitor. Visiting that element duly > fails, and aborts the visit with the list only partially constructed: > the faulty object is null. Cleaning up the partially constructed list > visits that null object, fails, and aborts the visit before the list > node gets freed. > > Fix the the generated visit_type_STRUCT() to succeed for null objects. > > Fixes: cdd2b228b973d2a29edf7696ef6e8b08ec329019 > Reported-by: Li Qiang > Signed-off-by: Markus Armbruster Oh, I also sent this too. Not matter, just ignore my patch. Tested-by: Li Qiang Reviewed-by: Li Qiang > --- > scripts/qapi/visit.py | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py > index 3fb2f30510..cdabc5fa28 100644 > --- a/scripts/qapi/visit.py > +++ b/scripts/qapi/visit.py > @@ -249,6 +249,7 @@ bool visit_type_%(c_name)s(Visitor *v, const char *name, > %(c_name)s **obj, Error > if (!*obj) { > /* incomplete */ > assert(visit_is_dealloc(v)); > +ok = true; > goto out_obj; > } > if (!visit_type_%(c_name)s_members(v, *obj, errp)) { > -- > 2.26.2 > >
[PATCH] e1000e: using bottom half to send packets
Alexander Bulekov reported a UAF bug related e1000e packets send. -->https://bugs.launchpad.net/qemu/+bug/1886362 This is because the guest trigger a e1000e packet send and set the data's address to e1000e's MMIO address. So when the e1000e do DMA it will write the MMIO again and trigger re-entrancy and finally causes this UAF. Paolo suggested to use a bottom half whenever MMIO is doing complicate things in here: -->https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg03342.html Reference here: 'The easiest solution is to delay processing of descriptors to a bottom half whenever MMIO is doing something complicated. This is also better for latency because it will free the vCPU thread more quickly and leave the work to the I/O thread.' This patch fixes this UAF. Signed-off-by: Li Qiang --- hw/net/e1000e_core.c | 25 + hw/net/e1000e_core.h | 2 ++ 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c index bcd186cac5..6165b04b68 100644 --- a/hw/net/e1000e_core.c +++ b/hw/net/e1000e_core.c @@ -2423,32 +2423,27 @@ e1000e_set_dbal(E1000ECore *core, int index, uint32_t val) static void e1000e_set_tctl(E1000ECore *core, int index, uint32_t val) { -E1000E_TxRing txr; core->mac[index] = val; if (core->mac[TARC0] & E1000_TARC_ENABLE) { -e1000e_tx_ring_init(core, &txr, 0); -e1000e_start_xmit(core, &txr); +qemu_bh_schedule(core->tx[0].tx_bh); } if (core->mac[TARC1] & E1000_TARC_ENABLE) { -e1000e_tx_ring_init(core, &txr, 1); -e1000e_start_xmit(core, &txr); +qemu_bh_schedule(core->tx[1].tx_bh); } } static void e1000e_set_tdt(E1000ECore *core, int index, uint32_t val) { -E1000E_TxRing txr; int qidx = e1000e_mq_queue_idx(TDT, index); uint32_t tarc_reg = (qidx == 0) ? TARC0 : TARC1; core->mac[index] = val & 0x; if (core->mac[tarc_reg] & E1000_TARC_ENABLE) { -e1000e_tx_ring_init(core, &txr, qidx); -e1000e_start_xmit(core, &txr); +qemu_bh_schedule(core->tx[qidx].tx_bh); } } @@ -3322,6 +3317,16 @@ e1000e_vm_state_change(void *opaque, int running, RunState state) } } +static void e1000e_core_tx_bh(void *opaque) +{ +struct e1000e_tx *tx = opaque; +E1000ECore *core = tx->core; +E1000E_TxRing txr; + +e1000e_tx_ring_init(core, &txr, tx - &core->tx[0]); +e1000e_start_xmit(core, &txr); +} + void e1000e_core_pci_realize(E1000ECore *core, const uint16_t *eeprom_templ, @@ -3340,6 +3345,8 @@ e1000e_core_pci_realize(E1000ECore *core, for (i = 0; i < E1000E_NUM_QUEUES; i++) { net_tx_pkt_init(&core->tx[i].tx_pkt, core->owner, E1000E_MAX_TX_FRAGS, core->has_vnet); +core->tx[i].core = core; +core->tx[i].tx_bh = qemu_bh_new(e1000e_core_tx_bh, &core->tx[i]); } net_rx_pkt_init(&core->rx_pkt, core->has_vnet); @@ -3367,6 +3374,8 @@ e1000e_core_pci_uninit(E1000ECore *core) for (i = 0; i < E1000E_NUM_QUEUES; i++) { net_tx_pkt_reset(core->tx[i].tx_pkt); net_tx_pkt_uninit(core->tx[i].tx_pkt); +qemu_bh_delete(core->tx[i].tx_bh); +core->tx[i].tx_bh = NULL; } net_rx_pkt_uninit(core->rx_pkt); diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h index aee32f7e48..94ddc6afc2 100644 --- a/hw/net/e1000e_core.h +++ b/hw/net/e1000e_core.h @@ -77,6 +77,8 @@ struct E1000Core { unsigned char sum_needed; bool cptse; struct NetTxPkt *tx_pkt; +QEMUBH *tx_bh; +E1000ECore *core; } tx[E1000E_NUM_QUEUES]; struct NetRxPkt *rx_pkt; -- 2.17.1
Re: [PATCH v3 7/9] tz-ppc: add dummy read/write methods
On Tue, 30 Jun 2020 at 13:31, P J P wrote: > > From: Prasad J Pandit > > Add tz-ppc-dummy mmio read/write methods to avoid assert failure > during initialisation. > > Signed-off-by: Prasad J Pandit > -- Reviewed-by: Peter Maydell thanks -- PMM
Re: [PATCH v10 09/10] qcow2_format.py: collect fields to dump in JSON format
16.07.2020 18:52, Andrey Shinkevich wrote: On 16.07.2020 18:40, Vladimir Sementsov-Ogievskiy wrote: 16.07.2020 18:34, Andrey Shinkevich wrote: On 16.07.2020 13:24, Vladimir Sementsov-Ogievskiy wrote: 14.07.2020 00:36, Andrey Shinkevich wrote: As __dict__ is being extended with class members we do not want to print, make a light copy of the initial __dict__ and extend the copy by adding lists we have to print in the JSON output. Signed-off-by: Andrey Shinkevich --- tests/qemu-iotests/qcow2_format.py | 4 1 file changed, 4 insertions(+) diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py index e0e14b5..83c3482 100644 --- a/tests/qemu-iotests/qcow2_format.py +++ b/tests/qemu-iotests/qcow2_format.py @@ -109,6 +109,8 @@ class Qcow2Struct(metaclass=Qcow2StructMeta): self.__dict__ = dict((field[2], values[i]) for i, field in enumerate(self.fields)) + self.fields_dict = self.__dict__.copy() No, I don't like that. Keeping two copies of all the data is bad idea. If you want to select some fields, add a method (dump_json() ?) Which will collect the fields you want in a dict and return that new dict. But don't store object attributes twice. That is what I did in the versions before but it looks clumsy to me. Every single class lists almost all the items of the __dict__ again in the additional method. Most of them should be listed automatically by base class method, which will iterate through .fields Not really. It makes a light copy that stores only references to the desired fields. I'm not about memory consumption, I'm about the design. Keeping two representations of same thing is always painful to maintain. + def dump(self, dump_json=None): for f in self.fields: value = self.__dict__[f[2]] @@ -144,6 +146,7 @@ class Qcow2BitmapExt(Qcow2Struct): self.bitmap_directory = \ [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size) for _ in range(self.nb_bitmaps)] + self.fields_dict.update(bitmap_directory=self.bitmap_directory) def dump(self, dump_json=None): super().dump(dump_json) @@ -189,6 +192,7 @@ class Qcow2BitmapDirEntry(Qcow2Struct): table = [e[0] for e in struct.iter_unpack('>Q', fd.read(table_size))] self.bitmap_table = Qcow2BitmapTable(raw_table=table, cluster_size=self.cluster_size) + self.fields_dict.update(bitmap_table=self.bitmap_table) def dump(self, dump_json=None): print(f'{"Bitmap name":<25} {self.name}') -- Best regards, Vladimir
Re: [PATCH v3] spapr: Add a new level of NUMA for GPUs
On Thu, Jul 16, 2020 at 06:42:11AM -0300, Daniel Henrique Barboza wrote: Which would translate here to: uint32_t associativity[] = { cpu_to_be32(0x4), cpu_to_be32(nvslot->numa_id), cpu_to_be32(nvslot->numa_id), cpu_to_be32(nvslot->numa_id), cpu_to_be32(nvslot->numa_id), }; Sure, that's how it originally was in v1 of the patch. I'll send a v4 today. It's been a while so I need to rebase anyway. -- Reza Arbab