Re: [PATCH v3 04/10] vfio: Query and store the maximum number of DMA mappings
> Let's query the maximum number of DMA mappings by querying the available
> mappings when creating the container.
>
> In addition, count the number of DMA mappings and warn when we would
> exceed it. This is a preparation for RamDiscardMgr which might
> create quite some DMA mappings over time, and we at least want to warn
> early that the QEMU setup might be problematic. Use "reserved"
> terminology, so we can use this to reserve mappings before they are
> actually created.
>
> Note: don't reserve vIOMMU DMA mappings - using the vIOMMU region size
> divided by the mapping page size might be a bad indication of what will
> happen in practice - we might end up warning all the time.
>
> Cc: Paolo Bonzini
> Cc: "Michael S. Tsirkin"
> Cc: Alex Williamson
> Cc: Dr. David Alan Gilbert
> Cc: Igor Mammedov
> Cc: Pankaj Gupta
> Cc: Peter Xu
> Cc: Auger Eric
> Cc: Wei Yang
> Cc: teawater
> Cc: Marek Kedzierski
> Signed-off-by: David Hildenbrand
> ---
> hw/vfio/common.c | 34 ++
> include/hw/vfio/vfio-common.h | 2 ++
> 2 files changed, 36 insertions(+)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 6ff1daa763..5ad88d476f 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -288,6 +288,26 @@ const MemoryRegionOps vfio_region_ops = {
> },
> };
>
> +static void vfio_container_dma_reserve(VFIOContainer *container,
> + unsigned long dma_mappings)
> +{
> +bool warned = container->dma_reserved > container->dma_max;
> +
> +container->dma_reserved += dma_mappings;
> +if (!warned && container->dma_max &&
> +container->dma_reserved > container->dma_max) {
> +warn_report("%s: possibly running out of DMA mappings. "
> +" Maximum number of DMA mappings: %d", __func__,
> +container->dma_max);
> +}
> +}
> +
> +static void vfio_container_dma_unreserve(VFIOContainer *container,
> + unsigned long dma_mappings)
> +{
> +container->dma_reserved -= dma_mappings;
> +}
> +
> /*
> * Device state interfaces
> */
> @@ -835,6 +855,9 @@ static void vfio_listener_region_add(MemoryListener
> *listener,
> }
> }
>
> +/* We'll need one DMA mapping. */
> +vfio_container_dma_reserve(container, 1);
> +
> ret = vfio_dma_map(container, iova, int128_get64(llsize),
> vaddr, section->readonly);
> if (ret) {
> @@ -879,6 +902,7 @@ static void vfio_listener_region_del(MemoryListener
> *listener,
> MemoryRegionSection *section)
> {
> VFIOContainer *container = container_of(listener, VFIOContainer,
> listener);
> +bool unreserve_on_unmap = true;
> hwaddr iova, end;
> Int128 llend, llsize;
> int ret;
> @@ -919,6 +943,7 @@ static void vfio_listener_region_del(MemoryListener
> *listener,
> * based IOMMU where a big unmap flattens a large range of IO-PTEs.
> * That may not be true for all IOMMU types.
> */
> +unreserve_on_unmap = false;
> }
>
> iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
> @@ -970,6 +995,11 @@ static void vfio_listener_region_del(MemoryListener
> *listener,
> "0x%"HWADDR_PRIx") = %d (%m)",
> container, iova, int128_get64(llsize), ret);
> }
> +
> +/* We previously reserved one DMA mapping. */
> +if (unreserve_on_unmap) {
> +vfio_container_dma_unreserve(container, 1);
> +}
> }
>
> memory_region_unref(section->mr);
> @@ -1735,6 +1765,7 @@ static int vfio_connect_container(VFIOGroup *group,
> AddressSpace *as,
> container->fd = fd;
> container->error = NULL;
> container->dirty_pages_supported = false;
> +container->dma_max = 0;
> QLIST_INIT(>giommu_list);
> QLIST_INIT(>hostwin_list);
>
> @@ -1765,7 +1796,10 @@ static int vfio_connect_container(VFIOGroup *group,
> AddressSpace *as,
> vfio_host_win_add(container, 0, (hwaddr)-1, info->iova_pgsizes);
> container->pgsizes = info->iova_pgsizes;
>
> +/* The default in the kernel ("dma_entry_limit") is 65535. */
> +container->dma_max = 65535;
> if (!ret) {
> +vfio_get_info_dma_avail(info, >dma_max);
> vfio_get_iommu_info_migration(container, info);
> }
> g_free(info);
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 6141162d7a..fed0e85f66 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -88,6 +88,8 @@ typedef struct VFIOContainer {
> uint64_t dirty_pgsizes;
> uint64_t max_dirty_bitmap_size;
> unsigned long pgsizes;
> +unsigned int dma_max;
> +unsigned long dma_reserved;
> QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
> QLIST_HEAD(, VFIOHostDMAWindow)
[Bug 1907776] Re: Mounting VFat drive yields error messages.
I have just noticed that the error does not appear when mounting the VFat drive in the installed instance of Arch Linux. The reported error messages occurred when using the "LiveUSB". -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1907776 Title: Mounting VFat drive yields error messages. Status in QEMU: New Bug description: Mounting a virtual Fat drive results in error messages (see attached image). * https://www.qemu.org/docs/master/system/images.html#virtual-fat- disk-images The "drive" is however mounted, and as a test, saving a text file to the drive is successfully stored in the directory `/tmp`, which can be read after shutdown of Qemu. Archlinux 5.9.11-arch2-1 (64-bit) qemu-headless 5.1.0-3 qemu-system-x86_64 -boot order=d -name test \ -enable-kvm -m 2G -cpu host -k sv \ -daemonize \ -drive if=pflash,format=raw,readonly,file=/usr/share/edk2-ovmf/x64/OVMF_CODE.fd \ -drive if=pflash,format=raw,file=~/vm/OVMF_VARS.local.fd \ -drive if=ide,format=raw,media=disk,index=1,file=fat:rw:/tmp \ -vnc :1 \ -cdrom /obj/archlinux/release/2020.10.01-x86_64.iso \ -drive format=raw,index=0,media=disk,file=~/vm/qemu.raw To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1907776/+subscriptions
RE: [PATCH v3] migration: Don't allow migration if vm is in POSTMIGRATE
Ping.
It seems no one handle this patch.
> -Original Message-
> From: Pankaj Gupta [mailto:pankaj.gupta.li...@gmail.com]
> Sent: Wednesday, December 09, 2020 10:21 PM
> To: tuguoyi (Cloud)
> Cc: Juan Quintela ; Dr. David Alan Gilbert
> ; vsement...@virtuozzo.com;
> qemu-devel@nongnu.org; Li Zhang
> Subject: Re: [PATCH v3] migration: Don't allow migration if vm is in
> POSTMIGRATE
>
> > The following steps will cause qemu assertion failure:
> > - pause vm by executing 'virsh suspend'
> > - create external snapshot of memory and disk using 'virsh
> snapshot-create-as'
> > - doing the above operation again will cause qemu crash
> >
> > The backtrace looks like:
> > #0 0x7fbf958c5c37 in raise () from /lib/x86_64-linux-gnu/libc.so.6
> > #1 0x7fbf958c9028 in abort () from /lib/x86_64-linux-gnu/libc.so.6
> > #2 0x7fbf958bebf6 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
> > #3 0x7fbf958beca2 in __assert_fail () from
> /lib/x86_64-linux-gnu/libc.so.6
> > #4 0x55ca8decd39d in bdrv_inactivate_recurse (bs=0x55ca90c80400)
> at /build/qemu-5.0/block.c:5724
> > #5 0x55ca8dece967 in bdrv_inactivate_all () at
> /build//qemu-5.0/block.c:5792
> > #6 0x55ca8de5539d in
> qemu_savevm_state_complete_precopy_non_iterable (inactivate_disks=true,
> in_postcopy=false, f=0x55ca907044b0)
> > at /build/qemu-5.0/migration/savevm.c:1401
> > #7 qemu_savevm_state_complete_precopy (f=0x55ca907044b0,
> iterable_only=iterable_only@entry=false,
> inactivate_disks=inactivate_disks@entry=true)
> > at /build/qemu-5.0/migration/savevm.c:1453
> > #8 0x55ca8de4f581 in migration_completion (s=0x55ca8f64d9f0) at
> /build/qemu-5.0/migration/migration.c:2941
> > #9 migration_iteration_run (s=0x55ca8f64d9f0) at
> /build/qemu-5.0/migration/migration.c:3295
> > #10 migration_thread (opaque=opaque@entry=0x55ca8f64d9f0) at
> /build/qemu-5.0/migration/migration.c:3459
> > #11 0x55ca8dfc6716 in qemu_thread_start (args=) at
> /build/qemu-5.0/util/qemu-thread-posix.c:519
> > #12 0x7fbf95c5f184 in start_thread () from
> /lib/x86_64-linux-gnu/libpthread.so.0
> > #13 0x7fbf9598cbed in clone () from /lib/x86_64-linux-gnu/libc.so.6
> >
> > When the first migration completes, bs->open_flags will set
> BDRV_O_INACTIVE
> > flag by bdrv_inactivate_all(), and during the second migration the
> > bdrv_inactivate_recurse assert that the bs->open_flags is already
> > BDRV_O_INACTIVE enabled which cause crash.
> >
> > As Vladimir suggested, this patch makes migrate_prepare check the state of
> vm and
> > return error if it is in RUN_STATE_POSTMIGRATE state.
> >
> > Signed-off-by: Tuguoyi
> Similar issue is reported by Li Zhang(+CC) with almost same patch[3]
> to fix this.
>
> Reported-by: Li Zhang
> Reviewed-by: Pankaj Gupta
>
> [3] https://marc.info/?l=qemu-devel=160749859831357=2
> > ---
> > migration/migration.c | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 87a9b59..5e33962 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2115,6 +2115,12 @@ static bool migrate_prepare(MigrationState *s,
> bool blk, bool blk_inc,
> > return false;
> > }
> >
> > +if (runstate_check(RUN_STATE_POSTMIGRATE)) {
> > +error_setg(errp, "Can't migrate the vm that was paused due to "
> > + "previous migration");
> > +return false;
> > +}
> > +
> > if (migration_is_blocked(errp)) {
> > return false;
> > }
> > --
> > 2.7.4
> >
> > [Patch v2]:
> https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg01318.html
> > [Patch v1]:
> https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg05950.html
Re: [PATCH v2 1/3] net: checksum: Skip fragmented IP packets
On 2020/12/17 下午1:41, Bin Meng wrote: Hi Jason, On Fri, Dec 11, 2020 at 5:35 PM Bin Meng wrote: From: Markus Carlstedt To calculate the TCP/UDP checksum we need the whole datagram. Unless the hardware has some logic to collect all fragments before sending the whole datagram first, it can only be done by the network stack, which is normally the case for the NICs we have seen so far. Skip these fragmented IP packets to avoid checksum corruption. Signed-off-by: Markus Carlstedt Signed-off-by: Bin Meng --- (no changes since v1) net/checksum.c | 4 1 file changed, 4 insertions(+) Ping? Regards, Bin Queued. Thanks
Re: [PATCH 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond
John Snow writes: > On 12/16/20 3:26 AM, Markus Armbruster wrote: >> John Snow writes: >> >>> We already assert this in end_if, but that's opaque to mypy. Do it in >>> _wrap_ifcond instead. Same effect at runtime, but mypy can now infer >>> the type in _wrap_ifcond's body. >>> >>> Signed-off-by: John Snow >>> --- >>> scripts/qapi/gen.py | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py >>> index b40f18eee3cd..a6dc991b1d03 100644 >>> --- a/scripts/qapi/gen.py >>> +++ b/scripts/qapi/gen.py >>> @@ -130,11 +130,11 @@ def start_if(self, ifcond: List[str]) -> None: >>> self._start_if = (ifcond, self._body, self._preamble) >>> >>> def end_if(self) -> None: >>> -assert self._start_if >>> self._wrap_ifcond() >>> self._start_if = None >>> >>> def _wrap_ifcond(self) -> None: >>> +assert self._start_if >>> self._body = _wrap_ifcond(self._start_if[0], >>> self._start_if[1], self._body) >>> self._preamble = _wrap_ifcond(self._start_if[0], >> >> Drawback: the public method's precondition is now more opaque. Do we >> care? >> > > Ish. If you call end_if before start_if, what did you want to have happen? Point. > Or more to the point: do you want the assertion in both places? What about inlining QAPIGenCCode._wrap_ifcond() into .end_if()?
[PATCH] docs/devel/migration: Improve debugging section a bit
Fix typos, and make the example work out of the box.
Signed-off-by: Markus Armbruster
---
docs/devel/migration.rst | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
index 49112bb27a..ad381b89b2 100644
--- a/docs/devel/migration.rst
+++ b/docs/devel/migration.rst
@@ -53,22 +53,23 @@ savevm/loadvm functionality.
Debugging
=
-The migration stream can be analyzed thanks to `scripts/analyze_migration.py`.
+The migration stream can be analyzed thanks to `scripts/analyze-migration.py`.
Example usage:
.. code-block:: shell
- $ qemu-system-x86_64
- (qemu) migrate "exec:cat > mig"
- $ ./scripts/analyze_migration.py -f mig
+ $ qemu-system-x86_64 -display none -monitor stdio
+ (qemu) migrate "exec:cat > mig"
+ (qemu) q
+ $ ./scripts/analyze-migration.py -f mig
{
"ram (3)": {
"section sizes": {
"pc.ram": "0x0800",
...
-See also ``analyze_migration.py -h`` help for more options.
+See also ``analyze-migration.py -h`` help for more options.
Common infrastructure
=
--
2.26.2
Re: [PATCH 16/20] migration: Replace migration's JSON writer by the general one
"Dr. David Alan Gilbert" writes: > * Markus Armbruster (arm...@redhat.com) wrote: >> Commit 8118f0950f "migration: Append JSON description of migration >> stream" needs a JSON writer. The existing qobject_to_json() wasn't a >> good fit, because it requires building a QObject to convert. Instead, >> migration got its very own JSON writer, in commit 190c882ce2 "QJSON: >> Add JSON writer". It tacitly limits numbers to int64_t, and strings >> contents to characters that don't need escaping, unlike >> qobject_to_json(). >> >> The previous commit factored the JSON writer out of qobject_to_json(). >> Replace migration's JSON writer by it. >> >> Cc: Juan Quintela >> Cc: Dr. David Alan Gilbert >> Signed-off-by: Markus Armbruster > > (Copying in Alex) > > This looks OK to me, so: > > Reviewed-by: Dr. David Alan Gilbert > > but, can I just check, have you checked scripts/analyze-migration.py is > still happy with the output? Good point. I just did, following instructions in docs/devel/migration.rst. It prints stuff and succeeds. Anything else you'd like me to try?
Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
John Snow writes: > On 12/16/20 2:51 AM, Markus Armbruster wrote: >> Is it too late to delay the introduction of type hints until after >> the >> data structure cleanups? >> If yes, I have to spend more time replying below. >> > > No, I re-ordered the series again to delay typing until the end, or > close to it. Thanks! > Though I abandoned plans to slacken List[...] inputs to Iterable[...] > or Sequence[...] like I had mentioned; I think it could still be done, > but it's fine to just use List[...] for the inputs for now. We can > worry about optimizing type flexibility later, I think. Shouldn't be a problem now I know what to expect. > Let's just get the dog hunting at all first. Yes.
Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
Eduardo Habkost writes: > On Wed, Dec 16, 2020 at 08:51:10AM +0100, Markus Armbruster wrote: > [...] >> You guys clearly struggled with the tree data structure. Documentation >> would have helped[*]. Since you're going to replace it (PATCH 09), >> adding it now makes little sense. >> >> *My* struggle is with the type annotations. >> >> The initial state is messy to type, in part due to mypy's surprising >> inability to deal with recursive types, in part because the tree data >> structure is messier than it could be. >> >> Much of the series is cleanup that benefits the typing. Makes sense. >> >> What makes review hard for me: you add (fairly complicated) typing >> first, then evolve it along with the cleanups. I have to first grok the >> complicated typing (a struggle), then for each cleanup grok the type >> changes in addition to the code changes. >> >> I believe adding the typing before the cleanups is a mistake. > > Possibly my fault, as I remember asking John to do that (in > earlier versions of these patches, type annotations were added > after cleanup). > >> >> I share the desire to have type annotations that help with understanding >> the code. I understand the desire to have them sooner rather than >> later. I just think they're a costly distraction at this stage for this >> code. Once you understand the data structure, the cleanups are fairly >> straightforward. >> > > I expected the type annotations to be a simple and helpful tool > for understanding the data structure before refactoring. In the > case of introspect.py, I was completely wrong about "simple", and > I'm not entirely sure about "helpful". Quite excusable. We lack the mypy experience to predict such outcomes. > I wasn't expecting them to be an obstacle for patch review, > though. If the type annotations look good at the end of the > series, do we care about the intermediate state? Bisectability > isn't an issue because type annotations are ignored by the Python > interpreter. I don't worry about bisectability. The issue is reviewability. Here's the best case for me reviewing a single patch. First, the commit message convinces me this makes sense. Then I read the patch mostly in order. It does what the commit message made me expect, I think I understand how it does it, and it doesn't touch anything I know to be subtle. Here's the best case for me reviewing a patch series: every patch in order is a best case review. As soon as review deviates from this best case, I slow down. A lot. If there is something I didn't expect, maybe I'm misunderstanding the patch's purpose. If I feel confused about how the patch achieves its purpose, I better figure it out. If something subtle is being touched, I better recall its subtleties and carefully check the patch. Slow and exhausting work. This way of review can be overly careful. But even deciding "this isn't important, let it go" is slow, unless I do it wholesale. All we get then is "looks good at a glance". But that's maybe an Acked-by, certainly not a Reviewed-by. Me finding the patch where the type hints start to be "serious" is slow. Me mentally separating changes to type hints from other changes in patches before that point is slow. Me examining the type hints at that point (which need not be entirely visible in the patch) is slow. If the annotations in the intermediate state don't have to be good, do they have to be there? If John can take them out, review will be easier and faster.
Re: [PATCH v4 12/16] target/riscv: cpu: Remove compile time XLEN checks
On Thu, Dec 17, 2020 at 2:22 AM Alistair Francis wrote: > > Signed-off-by: Alistair Francis > Reviewed-by: Bin Meng > Reviewed-by: Richard Henderson > Reviewed-by: Palmer Dabbelt > Acked-by: Palmer Dabbelt > --- > target/riscv/cpu.c | 19 ++- > 1 file changed, 10 insertions(+), 9 deletions(-) > Tested-by: Bin Meng
Re: [PATCH v4 16/16] hw/riscv: Use the CPU to determine if 32-bit
On Thu, Dec 17, 2020 at 2:23 AM Alistair Francis
wrote:
>
> Instead of using string compares to determine if a RISC-V machine is
> using 32-bit or 64-bit CPUs we can use the initalised CPUs. This avoids
> us having to maintain a list of CPU names to compare against.
>
> This commit also fixes the name of the function to match the
> riscv_cpu_is_32bit() function.
>
> Signed-off-by: Alistair Francis
> ---
> include/hw/riscv/boot.h | 8 +---
> hw/riscv/boot.c | 31 ++-
> hw/riscv/sifive_u.c | 10 +-
> hw/riscv/spike.c| 8
> hw/riscv/virt.c | 9 +
> 5 files changed, 29 insertions(+), 37 deletions(-)
>
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index b6d37a91d6..20ff5fe5e5 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -22,10 +22,11 @@
>
> #include "exec/cpu-defs.h"
> #include "hw/loader.h"
> +#include "hw/riscv/riscv_hart.h"
>
> -bool riscv_is_32_bit(MachineState *machine);
> +bool riscv_is_32bit(RISCVHartArrayState harts);
>
> -target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
> +target_ulong riscv_calc_kernel_start_addr(RISCVHartArrayState harts,
>target_ulong firmware_end_addr);
> target_ulong riscv_find_and_load_firmware(MachineState *machine,
>const char
> *default_machine_firmware,
> @@ -41,7 +42,8 @@ target_ulong riscv_load_kernel(const char *kernel_filename,
> hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
> uint64_t kernel_entry, hwaddr *start);
> uint32_t riscv_load_fdt(hwaddr dram_start, uint64_t dram_size, void *fdt);
> -void riscv_setup_rom_reset_vec(MachineState *machine, hwaddr saddr,
> +void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState
> harts,
> + hwaddr saddr,
> hwaddr rom_base, hwaddr rom_size,
> uint64_t kernel_entry,
> uint32_t fdt_load_addr, void *fdt);
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 6bce6fb485..83586aef41 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -33,28 +33,16 @@
>
> #include
>
> -bool riscv_is_32_bit(MachineState *machine)
> +bool riscv_is_32bit(RISCVHartArrayState harts)
> {
> -/*
> - * To determine if the CPU is 32-bit we need to check a few different
> CPUs.
> - *
> - * If the CPU starts with rv32
> - * If the CPU is a sifive 3 seriries CPU (E31, U34)
> - * If it's the Ibex CPU
> - */
> -if (!strncmp(machine->cpu_type, "rv32", 4) ||
> -(!strncmp(machine->cpu_type, "sifive", 6) &&
> -machine->cpu_type[8] == '3') ||
> -!strncmp(machine->cpu_type, "lowrisc-ibex", 12)) {
> -return true;
> -} else {
> -return false;
> -}
> +RISCVCPU hart = harts.harts[0];
What happens if something like ARM big.LITTLE needs to be supported on RISC-V?
> +
> +return riscv_cpu_is_32bit();
> }
>
[snip]
Regards,
Bin
Re: [PATCH v4 14/16] target/riscv: csr: Remove compile time XLEN checks
On Thu, Dec 17, 2020 at 2:23 AM Alistair Francis wrote: > > Signed-off-by: Alistair Francis > Reviewed-by: Bin Meng > Reviewed-by: Palmer Dabbelt > Acked-by: Palmer Dabbelt > --- > target/riscv/cpu_bits.h | 4 +- > target/riscv/csr.c | 176 +--- > 2 files changed, 92 insertions(+), 88 deletions(-) > Tested-by: Bin Meng
Re: [PATCH v4 13/16] target/riscv: cpu_helper: Remove compile time XLEN checks
On Thu, Dec 17, 2020 at 2:23 AM Alistair Francis wrote: > > Signed-off-by: Alistair Francis > Reviewed-by: Richard Henderson > Reviewed-by: Palmer Dabbelt > Acked-by: Palmer Dabbelt > --- > target/riscv/cpu_helper.c | 12 +++- > 1 file changed, 7 insertions(+), 5 deletions(-) > Reviewed-by: Bin Meng Tested-by: Bin Meng
Re: [PATCH v4 08/16] hw/riscv: sifive_u: Remove compile time XLEN checks
On Thu, Dec 17, 2020 at 2:22 AM Alistair Francis wrote: > > Signed-off-by: Alistair Francis > Reviewed-by: Palmer Dabbelt > Acked-by: Palmer Dabbelt > --- > hw/riscv/sifive_u.c | 55 - > 1 file changed, 30 insertions(+), 25 deletions(-) > Tested with 64-bit sifive_u Reviewed-by: Bin Meng Tested-by: Bin Meng 32-bit is blocked by: http://lists.infradead.org/pipermail/linux-riscv/2020-December/003927.html Regards, Bin
[Bug 1908489] [NEW] qemu 4.2 bootloops with -cpu host and nested hypervisor
Public bug reported: I've noticed that after upgrading from Ubuntu 18.04 to 20.04 that nested virtualization isn't working anymore. I have a simple repro where I create a Windows 10 2004 guest and enable Hyper-V in it. This worked fine in 18.04 and specifically qemu <4.2 (I specifically tested Qemu 2.11-4.1 which work fine). The -cpu arg I'm passing is simply: -cpu host,l3-cache=on,hv_relaxed,hv_spinlocks=0x1fff,hv_vapic,hv_time Using that Windows won't boot because the nested hypervisor (Hyper-V) is unable to be initialize and so it just boot loops. Using the exact same qemu command works fine with 4.1 and lower. Switching to a named CPU model like Skylake-Client-noTSX-IBRS instead of host lets the VM boot but causes some weird behaviour later trying to use nested VMs. If I had to guess I think it would probably be related to this change https://github.com/qemu/qemu/commit/20a78b02d31534ae478779c2f2816c273601e869 which would line up with 4.2 being the first bad version but unsure. For now I just have to keep an older build of QEMU to work around this. Let me know if there's anything else needed. I can also try out any patches. I already have at least a dozen copies of qemu lying around now. ** Affects: qemu Importance: Undecided Status: New -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1908489 Title: qemu 4.2 bootloops with -cpu host and nested hypervisor Status in QEMU: New Bug description: I've noticed that after upgrading from Ubuntu 18.04 to 20.04 that nested virtualization isn't working anymore. I have a simple repro where I create a Windows 10 2004 guest and enable Hyper-V in it. This worked fine in 18.04 and specifically qemu <4.2 (I specifically tested Qemu 2.11-4.1 which work fine). The -cpu arg I'm passing is simply: -cpu host,l3-cache=on,hv_relaxed,hv_spinlocks=0x1fff,hv_vapic,hv_time Using that Windows won't boot because the nested hypervisor (Hyper-V) is unable to be initialize and so it just boot loops. Using the exact same qemu command works fine with 4.1 and lower. Switching to a named CPU model like Skylake-Client-noTSX-IBRS instead of host lets the VM boot but causes some weird behaviour later trying to use nested VMs. If I had to guess I think it would probably be related to this change https://github.com/qemu/qemu/commit/20a78b02d31534ae478779c2f2816c273601e869 which would line up with 4.2 being the first bad version but unsure. For now I just have to keep an older build of QEMU to work around this. Let me know if there's anything else needed. I can also try out any patches. I already have at least a dozen copies of qemu lying around now. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1908489/+subscriptions
Re: [for-6.0 v5 00/13] Generalize memory encryption models
On Tue, Dec 08, 2020 at 01:43:08PM +0100, Cornelia Huck wrote:
> On Tue, 8 Dec 2020 13:57:28 +1100
> David Gibson wrote:
>
> > On Fri, Dec 04, 2020 at 02:12:29PM +0100, Cornelia Huck wrote:
> > > On Fri, 4 Dec 2020 13:07:27 +
> > > "Dr. David Alan Gilbert" wrote:
> > >
> > > > * Cornelia Huck (coh...@redhat.com) wrote:
> > > > > On Fri, 4 Dec 2020 09:06:50 +0100
> > > > > Christian Borntraeger wrote:
> > > > >
> > > > > > On 04.12.20 06:44, David Gibson wrote:
> > > > > > > A number of hardware platforms are implementing mechanisms
> > > > > > > whereby the
> > > > > > > hypervisor does not have unfettered access to guest memory, in
> > > > > > > order
> > > > > > > to mitigate the security impact of a compromised hypervisor.
> > > > > > >
> > > > > > > AMD's SEV implements this with in-cpu memory encryption, and
> > > > > > > Intel has
> > > > > > > its own memory encryption mechanism. POWER has an upcoming
> > > > > > > mechanism
> > > > > > > to accomplish this in a different way, using a new memory
> > > > > > > protection
> > > > > > > level plus a small trusted ultravisor. s390 also has a protected
> > > > > > > execution environment.
> > > > > > >
> > > > > > > The current code (committed or draft) for these features has each
> > > > > > > platform's version configured entirely differently. That doesn't
> > > > > > > seem
> > > > > > > ideal for users, or particularly for management layers.
> > > > > > >
> > > > > > > AMD SEV introduces a notionally generic machine option
> > > > > > > "machine-encryption", but it doesn't actually cover any cases
> > > > > > > other
> > > > > > > than SEV.
> > > > > > >
> > > > > > > This series is a proposal to at least partially unify
> > > > > > > configuration
> > > > > > > for these mechanisms, by renaming and generalizing AMD's
> > > > > > > "memory-encryption" property. It is replaced by a
> > > > > > > "securable-guest-memory" property pointing to a platform specific
> > > > > > >
> > > > > >
> > > > > > Can we do "securable-guest" ?
> > > > > > s390x also protects registers and integrity. memory is only one
> > > > > > piece
> > > > > > of the puzzle and what we protect might differ from platform to
> > > > > > platform.
> > > > > >
> > > > >
> > > > > I agree. Even technologies that currently only do memory encryption
> > > > > may
> > > > > be enhanced with more protections later.
> > > >
> > > > There's already SEV-ES patches onlist for this on the SEV side.
> > > >
> > > >
> > > >
> > > > Perhaps 'confidential guest' is actually what we need, since the
> > > > marketing folks seem to have started labelling this whole idea
> > > > 'confidential computing'.
> >
> > That's not a bad idea, much as I usually hate marketing terms. But it
> > does seem to be becoming a general term for this style of thing, and
> > it doesn't overlap too badly with other terms ("secure" and
> > "protected" are also used for hypervisor-from-guest and
> > guest-from-guest protection).
> >
> > > It's more like a 'possibly confidential guest', though.
> >
> > Hmm. What about "Confidential Guest Facility" or "Confidential Guest
> > Mechanism"? The implication being that the facility is there, whether
> > or not the guest actually uses it.
> >
>
> "Confidential Guest Enablement"? The others generally sound fine to me
> as well, though; not sure if "Facility" might be a bit confusing, as
> that term is already a bit overloaded.
Well, "facility" is a bit overloaded, but IMO "enablement" is even
more so. I think I'll go with "confidential guest support" in the
next spin.
--
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: [PULL 00/12] Compile QEMU with -Wimplicit-fallthrough
On 17/12/2020 00.01, no-re...@patchew.org wrote: > Patchew URL: https://patchew.org/QEMU/20201216172949.57380-1-th...@redhat.com/ > > > > Hi, > > This series seems to have some coding style problems. See output below for > more information: > > Type: series > Message-id: 20201216172949.57380-1-th...@redhat.com > Subject: [PULL 00/12] Compile QEMU with -Wimplicit-fallthrough > > === 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 > From https://github.com/patchew-project/qemu > * [new tag] patchew/20201216172949.57380-1-th...@redhat.com -> > patchew/20201216172949.57380-1-th...@redhat.com > Switched to a new branch 'test' > 7bedbc8 configure: Compile with -Wimplicit-fallthrough=2 > e14bb9d tests/fp: Do not emit implicit-fallthrough warnings in the softfloat > tests > ebd3c45 tcg/optimize: Add fallthrough annotations > cfe5662 target/sparc/win_helper: silence the compiler warnings > 8ef9335 target/sparc/translate: silence the compiler warnings > 4588bf9 accel/tcg/user-exec: silence the compiler warnings > be2108e hw/intc/arm_gicv3_kvm: silence the compiler warnings > 7d033d0 target/i386: silence the compiler warnings in gen_shiftd_rm_T1 > 284b00a hw/timer/renesas_tmr: silence the compiler warnings > c3d2957 hw/rtc/twl92230: Silence warnings about missing fallthrough statements > 1b1609c target/unicore32/translate: Add missing fallthrough annotations > 99bc0f0 disas/libvixl: Fix fall-through annotation for GCC >= 7 > > === OUTPUT BEGIN === > 1/12 Checking commit 99bc0f0e92b7 (disas/libvixl: Fix fall-through annotation > for GCC >= 7) > ERROR: do not use C99 // comments > #49: FILE: disas/libvixl/vixl/globals.h:111: > +// Fallthrough annotation for Clang and C++11(201103L). > > ERROR: do not use C99 // comments > #52: FILE: disas/libvixl/vixl/globals.h:114: > +// Fallthrough annotation for GCC >= 7. Well, libvixl is C++ code and the upstream patch used these comments, too, so this error can be ignored. > total: 2 errors, 0 warnings, 24 lines checked > > Patch 1/12 has style problems, please review. If any of these errors > are false positives report them to the maintainer, see > CHECKPATCH in MAINTAINERS. > > 2/12 Checking commit 1b1609c7573a (target/unicore32/translate: Add missing > fallthrough annotations) > 3/12 Checking commit c3d2957383b8 (hw/rtc/twl92230: Silence warnings about > missing fallthrough statements) > 4/12 Checking commit 284b00aef566 (hw/timer/renesas_tmr: silence the compiler > warnings) > 5/12 Checking commit 7d033d02b90d (target/i386: silence the compiler warnings > in gen_shiftd_rm_T1) > 6/12 Checking commit be2108e641c9 (hw/intc/arm_gicv3_kvm: silence the > compiler warnings) > 7/12 Checking commit 4588bf97482b (accel/tcg/user-exec: silence the compiler > warnings) > 8/12 Checking commit 8ef9335f2838 (target/sparc/translate: silence the > compiler warnings) > 9/12 Checking commit cfe56623ece8 (target/sparc/win_helper: silence the > compiler warnings) > 10/12 Checking commit ebd3c45fd052 (tcg/optimize: Add fallthrough annotations) > WARNING: architecture specific defines should be avoided > #35: FILE: include/qemu/compiler.h:230: > +#if __has_attribute(fallthrough) Maybe we should teach checkpatch.pl to allow these in compiler.h? Thomas
Re: [for-6.0 v5 11/13] spapr: PEF: prevent migration
On Mon, Dec 14, 2020 at 06:22:40PM +0100, Cornelia Huck wrote:
> On Fri, 4 Dec 2020 16:44:13 +1100
> David Gibson wrote:
>
> > We haven't yet implemented the fairly involved handshaking that will be
> > needed to migrate PEF protected guests. For now, just use a migration
> > blocker so we get a meaningful error if someone attempts this (this is the
> > same approach used by AMD SEV).
> >
> > Signed-off-by: David Gibson
> > Reviewed-by: Dr. David Alan Gilbert
> > ---
> > hw/ppc/pef.c | 9 +
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/hw/ppc/pef.c b/hw/ppc/pef.c
> > index 3ae3059cfe..edc3e744ba 100644
> > --- a/hw/ppc/pef.c
> > +++ b/hw/ppc/pef.c
> > @@ -38,7 +38,11 @@ struct PefGuestState {
> > };
> >
> > #ifdef CONFIG_KVM
> > +static Error *pef_mig_blocker;
> > +
> > static int kvmppc_svm_init(Error **errp)
>
> This looks weird?
Oops. Not sure how that made it past even my rudimentary compile
testing.
> > +
> > +int kvmppc_svm_init(SecurableGuestMemory *sgm, Error **errp)
> > {
> > if (!kvm_check_extension(kvm_state, KVM_CAP_PPC_SECURABLE_GUEST)) {
> > error_setg(errp,
> > @@ -54,6 +58,11 @@ static int kvmppc_svm_init(Error **errp)
> > }
> > }
> >
> > +/* add migration blocker */
> > +error_setg(_mig_blocker, "PEF: Migration is not implemented");
> > +/* NB: This can fail if --only-migratable is used */
> > +migrate_add_blocker(pef_mig_blocker, _fatal);
>
> Just so that I understand: is PEF something that is enabled by the host
> (and the guest is either secured or doesn't start), or is it using a
> model like s390x PV where the guest initiates the transition into
> secured mode?
Like s390x PV it's initiated by the guest.
> Asking because s390x adds the migration blocker only when the
> transition is actually happening (i.e. guests that do not transition
> into secure mode remain migratable.) This has the side effect that you
> might be able to start a machine with --only-migratable that
> transitions into a non-migratable machine via a guest action, if I'm
> not mistaken. Without the new object, I don't see a way to block with
> --only-migratable; with it, we should be able to do that. Not sure what
> the desirable behaviour is here.
Hm, I'm not sure what the best option is here either.
>
> > +
> > return 0;
> > }
> >
>
--
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: [for-6.0 v5 13/13] s390: Recognize securable-guest-memory option
On Tue, Dec 15, 2020 at 12:45:26PM +0100, Cornelia Huck wrote: > On Fri, 4 Dec 2020 16:44:15 +1100 > David Gibson wrote: > > > At least some s390 cpu models support "Protected Virtualization" (PV), > > a mechanism to protect guests from eavesdropping by a compromised > > hypervisor. > > > > This is similar in function to other mechanisms like AMD's SEV and > > POWER's PEF, which are controlled bythe "securable-guest-memory" machine > > s/bythe/by the/ > > > option. s390 is a slightly special case, because we already supported > > PV, simply by using a CPU model with the required feature > > (S390_FEAT_UNPACK). > > > > To integrate this with the option used by other platforms, we > > implement the following compromise: > > > > - When the securable-guest-memory option is set, s390 will recognize it, > >verify that the CPU can support PV (failing if not) and set virtio > >default options necessary for encrypted or protected guests, as on > >other platforms. i.e. if securable-guest-memory is set, we will > >either create a guest capable of entering PV mode, or fail outright > > s/outright/outright./ > > > > > - If securable-guest-memory is not set, guest's might still be able to > > s/guest's/guests/ All those corrected, thanks. > >enter PV mode, if the CPU has the right model. This may be a > >little surprising, but shouldn't actually be harmful. > > > > To start a guest supporting Protected Virtualization using the new > > option use the command line arguments: > > -object s390-pv-guest,id=pv0 -machine securable-guest-memory=pv0 > > > > Signed-off-by: David Gibson > > --- > > hw/s390x/pv.c | 58 +++ > > include/hw/s390x/pv.h | 1 + > > target/s390x/kvm.c| 3 +++ > > 3 files changed, 62 insertions(+) > > > > Modulo any naming changes etc., I think this should work for s390. I > don't have the hardware to test this, however, and would appreciate > someone with a PV setup giving this a go. Makes sense. -- 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: [for-6.0 v5 08/13] securable guest memory: Introduce sgm "ready" flag
On Mon, Dec 14, 2020 at 06:00:36PM +0100, Cornelia Huck wrote:
> On Fri, 4 Dec 2020 16:44:10 +1100
> David Gibson wrote:
>
> > The platform specific details of mechanisms for implementing securable
> > guest memory may require setup at various points during initialization.
> > Thus, it's not really feasible to have a single sgm initialization hook,
> > but instead each mechanism needs its own initialization calls in arch or
> > machine specific code.
> >
> > However, to make it harder to have a bug where a mechanism isn't properly
> > initialized under some circumstances, we want to have a common place,
> > relatively late in boot, where we verify that sgm has been initialized if
> > it was requested.
> >
> > This patch introduces a ready flag to the SecurableGuestMemory base type
> > to accomplish this, which we verify just before the machine specific
> > initialization function.
> >
> > Signed-off-by: David Gibson
> > ---
> > hw/core/machine.c | 8
> > include/exec/securable-guest-memory.h | 2 ++
> > target/i386/sev.c | 2 ++
> > 3 files changed, 12 insertions(+)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 816ea3ae3e..a67a27d03c 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -1155,6 +1155,14 @@ void machine_run_board_init(MachineState *machine)
> > }
> >
> > if (machine->sgm) {
> > +/*
> > + * Where securable guest memory is initialized depends on the
> > + * specific mechanism in use. But, we need to make sure it's
> > + * ready by now. If it isn't, that's a bug in the
> > + * implementation of that sgm mechanism.
> > + */
> > +assert(machine->sgm->ready);
>
> Under which circumstances might we arrive here with 'ready' not set?
>
> - programming error, setup is happening too late -> assert() seems
> appropriate
Yes, this is designed to catch programming errors. In particular I'm
concerned about:
* Re-arranging the init code, and either entirely forgetting the sgm
setup, or accidentally moving it too late
* The sgm setup is buried in the machine setup code, conditional on
various things, and changes mean we no longer either call it or
(correctly) fail
* User has specified an sgm scheme designed for a machine type other
than the one they selected. The arch/machine init code hasn't
correctly accounted for that possibility and ignores it, instead
of correctly throwing an error
> - we tried to set it up, but some error happened -> should we rely on
> the setup code to error out first? (i.e. we won't end up here, unless
> there's a programming error, in which case the assert() looks
> fine)
Yes, that's my intention.
> Is there a possible use case for "we could not set it up, but we
> support an unsecured guest (as long as it is clear what happens)"?
I don't think so. My feeling is that if you specify that you want the
feature, qemu needs to either give it to you, or fail, not silently
degrade the features presented to the guest.
> Likely only for guests that transition themselves, but one could
> argue that QEMU should simply be invoked a second time without the
> sgm stuff being specified in the error case.
Right - I think whatever error we give here is likely to be easier to
diagnose than the guest itself throwing an error when it fails to
transition to secure mode (plus we should catch it always, rather than
only if we run a guest which tries to go secure).
--
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: [for-6.0 v5 12/13] securable guest memory: Alter virtio default properties for protected guests
On Tue, Dec 08, 2020 at 01:50:05PM +0100, Cornelia Huck wrote: > On Tue, 8 Dec 2020 11:28:29 +0100 > Halil Pasic wrote: > > > On Tue, 8 Dec 2020 12:54:03 +1100 > > David Gibson wrote: > > > > > > > >>> + * Virtio devices can't count on directly accessing guest > > > > > >>> + * memory, so they need iommu_platform=on to use normal > > > > > >>> DMA > > > > > >>> + * mechanisms. That requires also disabling legacy > > > > > >>> virtio > > > > > >>> + * support for those virtio pci devices which allow it. > > > > > >>> + */ > > > > > >>> +object_register_sugar_prop(TYPE_VIRTIO_PCI, > > > > > >>> "disable-legacy", > > > > > >>> + "on", true); > > > > > >>> +object_register_sugar_prop(TYPE_VIRTIO_DEVICE, > > > > > >>> "iommu_platform", > > > > > >>> + "on", false); > > > > > >> > > > > > >> I have not followed all the history (sorry). Should we also set > > > > > >> iommu_platform > > > > > >> for virtio-ccw? Halil? > > > > > >> > > > > > > > > > > > > That line should add iommu_platform for all virtio devices, > > > > > > shouldn't > > > > > > it? > > > > > > > > > > Yes, sorry. Was misreading that with the line above. > > > > > > > > > > > > > I believe this is the best we can get. In a sense it is still a > > > > pessimization, > > > > > > I'm not really clear on what you're getting at here. > > > > By pessimiziation, I mean that we are going to indicate > > _F_PLATFORM_ACCESS even if it isn't necessary, because the guest never > > opted in for confidential/memory protection/memory encryption. We have > > discussed this before, and I don't see a better solution that works for > > everybody. > > If you consider specifying the secure guest option as a way to tell > QEMU to make everything ready for running a secure guest, I'd certainly > consider it necessary. If you do not want to force it, you should not > do the secure guest preparation setup. Right, that's my feeling as well. I'm also of the opinion that !F_PLATFORM_ACCESS is kind of a nasty hack that has some other problems (e.g. it means an L1 can't safely pass the device into an L2). -- 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 v2 1/3] net: checksum: Skip fragmented IP packets
Hi Jason, On Fri, Dec 11, 2020 at 5:35 PM Bin Meng wrote: > > From: Markus Carlstedt > > To calculate the TCP/UDP checksum we need the whole datagram. Unless > the hardware has some logic to collect all fragments before sending > the whole datagram first, it can only be done by the network stack, > which is normally the case for the NICs we have seen so far. > > Skip these fragmented IP packets to avoid checksum corruption. > > Signed-off-by: Markus Carlstedt > Signed-off-by: Bin Meng > --- > > (no changes since v1) > > net/checksum.c | 4 > 1 file changed, 4 insertions(+) > Ping? Regards, Bin
[PATCH 1/2] hw/ssi: imx_spi: Correct the burst length > 32 bit transfer logic
From: Bin Meng
For the ECSPIx_CONREG register BURST_LENGTH field, the manual says:
0x020 A SPI burst contains the 1 LSB in first word and all 32 bits in second
word.
0x021 A SPI burst contains the 2 LSB in first word and all 32 bits in second
word.
Current logic uses either s->burst_length or 32, whichever smaller,
to determine how many bits it should read from the tx fifo each time.
For example, for a 48 bit burst length, current logic transfers the
first 32 bit from the first word in the tx fifo, followed by a 16
bit from the second word in the tx fifo, which is wrong. The correct
logic should be: transfer the first 16 bit from the first word in
the tx fifo, followed by a 32 bit from the second word in the tx fifo.
With this change, SPI flash can be successfully probed by U-Boot on
imx6 sabrelite board.
=> sf probe
SF: Detected sst25vf016b with page size 256 Bytes, erase size 4 KiB, total 2
MiB
Fixes: c906a3a01582 ("i.MX: Add the Freescale SPI Controller")
Signed-off-by: Bin Meng
---
hw/ssi/imx_spi.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 85c172e..509fb9f 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -178,7 +178,10 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
DPRINTF("data tx:0x%08x\n", tx);
-tx_burst = MIN(s->burst_length, 32);
+tx_burst = s->burst_length % 32;
+if (tx_burst == 0) {
+tx_burst = 32;
+}
rx = 0;
--
2.7.4
[PATCH 2/2] hw/ssi: imx_spi: Correct tx and rx fifo endianness
From: Bin Meng
The endianness of data exchange between tx and rx fifo is incorrect.
Earlier bytes are supposed to show up on MSB and later bytes on LSB,
ie: in big endian. The manual does not explicitly say this, but the
U-Boot and Linux driver codes have a swap on the data transferred
to tx fifo and from rx fifo.
With this change, U-Boot read from / write to SPI flash tests pass.
=> sf test 1ff000 1000
SPI flash test:
0 erase: 0 ticks, 4096000 KiB/s 32768.000 Mbps
1 check: 3 ticks, 1333 KiB/s 10.664 Mbps
2 write: 235 ticks, 17 KiB/s 0.136 Mbps
3 read: 2 ticks, 2000 KiB/s 16.000 Mbps
Test passed
0 erase: 0 ticks, 4096000 KiB/s 32768.000 Mbps
1 check: 3 ticks, 1333 KiB/s 10.664 Mbps
2 write: 235 ticks, 17 KiB/s 0.136 Mbps
3 read: 2 ticks, 2000 KiB/s 16.000 Mbps
Fixes: c906a3a01582 ("i.MX: Add the Freescale SPI Controller")
Signed-off-by: Bin Meng
---
hw/ssi/imx_spi.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 509fb9f..71f0902 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -156,13 +156,14 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
{
uint32_t tx;
uint32_t rx;
+uint32_t data;
+uint8_t byte;
DPRINTF("Begin: TX Fifo Size = %d, RX Fifo Size = %d\n",
fifo32_num_used(>tx_fifo), fifo32_num_used(>rx_fifo));
while (!fifo32_is_empty(>tx_fifo)) {
int tx_burst = 0;
-int index = 0;
if (s->burst_length <= 0) {
s->burst_length = imx_spi_burst_length(s);
@@ -183,10 +184,18 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
tx_burst = 32;
}
+data = 0;
+for (int i = 0; i < tx_burst / 8; i++) {
+byte = tx & 0xff;
+tx = tx >> 8;
+data = (data << 8) | byte;
+}
+tx = data;
+
rx = 0;
while (tx_burst > 0) {
-uint8_t byte = tx & 0xff;
+byte = tx & 0xff;
DPRINTF("writing 0x%02x\n", (uint32_t)byte);
@@ -196,12 +205,11 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
DPRINTF("0x%02x read\n", (uint32_t)byte);
tx = tx >> 8;
-rx |= (byte << (index * 8));
+rx = (rx << 8) | byte;
/* Remove 8 bits from the actual burst */
tx_burst -= 8;
s->burst_length -= 8;
-index++;
}
DPRINTF("data rx:0x%08x\n", rx);
--
2.7.4
hexagon sysemu - library loading path feature
My team is working on sysemu support for Hexagon. We've made some good progress so far and we'll work on upstreaming after Taylor’s hexagon linux-user patch series lands. The only use case we have focused on with sysemu is booting/running elf programs. Both "-device loader,file=..." or "-kernel" are effective and work similarly. We have implemented "angel calls" (semihosting) to do host I/O. We have not yet tried using the QEMU semihosting features/cmdline args, but may explore that option. One feature we'd like to integrate is a guest library search path feature. The existing hexagon simulator program distributed in the Hexagon SDK has a command line option, “--usefs". The manual states that it “Cause[s] the simulator to search for files in the directory with the specified path. It is used for accessing shared object files that are loaded during program execution.” If the guest OS has a loader that tries to resolve an executable or library's DT_NEEDED shared object libraries, we would want QEMU angel calls to be able to search a user specified host-path for the toolchain language support libraries. This feature is like the functionality in QEMU’s “QEMU_LD_PREFIX” environment variable used by linux-userspace. So, one idea was to just (ab)use this interface to mean the same thing for sysemu. We could make it a target-specific hexagon feature, if it doesn’t make sense to have it as target-independent. And if it makes sense we could qualify it like HEXAGON_QEMU_LD_PREFIX. If not this environment variable, is there an existing QEMU feature that maps well here? Or is there a better interface that we should consider instead? -Brian
Re:[PATCH v2 0/5] Fix some style problems in contrib
kindly ping
>v1 -> v2:
>Changed the "From:" and "Signed-off-by:" lines from "zhouyang (T)"
>to my real name "zhouyang".
>
>I found some style problems while check the code using checkpatch.pl
>and fixed them, please review.
>
>zhouyang (5):
> contrib: Don't use '#' flag of printf format
> contrib: Fix some code style problems, ERROR: "foo * bar" should be
>"foo *bar"
> contrib: Add spaces around operator
> contrib: space required after that ','
> contrib: Open brace '{' following struct go on the same line
>
> contrib/ivshmem-server/main.c | 2 +-
> contrib/plugins/hotblocks.c | 2 +-
> contrib/plugins/hotpages.c| 2 +-
> contrib/plugins/howvec.c | 19 +--
> contrib/plugins/lockstep.c| 6 +++---
> 5 files changed, 15 insertions(+), 16 deletions(-)
>
>--
>2.23.0
Re: [PATCH] tests/acceptance: Test PMON with Loongson-3A1000 CPU
在 2020/12/17 上午2:17, Philippe Mathieu-Daudé 写道:
Test the PMON firmware. As the firmware is not redistributable,
it has to be downloaded manually first. Then it can be used by
providing its path via the PMON_PATH environment variable:
We have a PMON port for loongson3-virt machine[1] and it's redistributable.
You can also fetch prebuilt binary from GitHub action artifacts, I can
also make
a release on GitHub to make it easier.
Thanks.
[1] https://github.com/loongson-community/pmon
- Jiaxun
$ AVOCADO_ALLOW_UNTRUSTED_CODE=1 \
PMON_PATH=/images/pmon \
avocado --show=app,console \
run -t machine:loongson3-virt tests/acceptance
JOB ID : 363e66a2d20b1c0e3f515653f9137483b83b2984
JOB LOG:
/home/phil/avocado/job-results/job-2020-12-16T19.02-363e66a/job.log
(1/2)
tests/acceptance/machine_mips_fuloong3.py:MipsFuloong3.test_pmon_BLD_serial_console:
console: PMON2000 MIPS Initializing. Standby...
console:
console: Shut down other cores
console: 0xbfe00190 :
console: CPU CLK SEL :
console: MEM CLK SEL :
console: Change the driver
console: Soft CLK SEL adjust begin
console: HT :
console: DDR_DIV:0002
console: BBGEN start :
console: BBGEN config value :
console: MC RESET
console: Fix L1xbar illegal access at NODE 0
console: Fix L2xbar in NODE 0
console: 32 bit PCI space translate to 64 bit HT space
console: Waiting HyperTransport bus to be up.
PASS (0.10 s)
(2/2)
tests/acceptance/machine_mips_fuloong3.py:MipsFuloong3.test_pmon_A1101_serial_console:
console: PMON2000 MIPS Initializing. Standby...
console: 0xbfe00190 :
console: CPU CLK SEL :
console: CPU clk frequency = SYSCLK x 0x001e / 1
console: MEM CLK SEL :
console: DDR clk frequency = MEMCLK x 0x001e / 3
console: Fix L1xbar illegal access
console: Fix L2xbar illegal access
console: Init tlb...
console: godson2 caches found
PASS (0.12 s)
RESULTS: PASS 2 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 |
CANCEL 0
JOB TIME : 0.58 s
Signed-off-by: Philippe Mathieu-Daudé
---
Based-on: <20201215125716.477023-1-chenhua...@kernel.org>
---
MAINTAINERS | 1 +
tests/acceptance/machine_mips_loongson3v.py | 66 +
2 files changed, 67 insertions(+)
create mode 100644 tests/acceptance/machine_mips_loongson3v.py
diff --git a/MAINTAINERS b/MAINTAINERS
index f75fa2a7142..9a02d44f997 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1166,6 +1166,7 @@ M: Huacai Chen
R: Jiaxun Yang
S: Maintained
F: hw/intc/loongson_liointc.c
+F: tests/acceptance/machine_mips_loongson3v.py
Boston
M: Paul Burton
diff --git a/tests/acceptance/machine_mips_loongson3v.py
b/tests/acceptance/machine_mips_loongson3v.py
new file mode 100644
index 000..8e698bbc99b
--- /dev/null
+++ b/tests/acceptance/machine_mips_loongson3v.py
@@ -0,0 +1,66 @@
+# Functional tests for the Generic Loongson-3 Platform.
+#
+# Copyright (c) 2020 Philippe Mathieu-Daudé
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or later.
+# See the COPYING file in the top-level directory.
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import os
+import time
+
+from avocado import skipUnless
+from avocado_qemu import Test
+from avocado_qemu import wait_for_console_pattern
+
+class MipsFuloong3(Test):
+
+timeout = 60
+
+@skipUnless(os.getenv('PMON_PATH'), 'PMON_PATH not available')
+@skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')
+def test_pmon_BLD_serial_console(self):
+"""
+:avocado: tags=arch:mips64el
+:avocado: tags=endian:little
+:avocado: tags=machine:loongson3-virt
+:avocado: tags=cpu:Loongson-3A1000
+:avocado: tags=device:liointc
+:avocado: tags=device:goldfish_rtc
+"""
+pmon_name = 'pmon_BLD-3A3000-780EMATX-1w-V1.10.bin'
+pmon_hash = '38916ee03ed09a86997b40c687c83e92'
+pmon_path = self.fetch_asset('file://' + os.path.join(
+os.getenv('PMON_PATH'), pmon_name),
+ asset_hash=pmon_hash, algorithm='md5')
+
+self.vm.set_console()
+self.vm.add_args('-bios', pmon_path)
+self.vm.launch()
+wait_for_console_pattern(self, 'PMON2000 MIPS Initializing.
Standby...')
+wait_for_console_pattern(self, 'Shut down other cores')
+wait_for_console_pattern(self, 'Waiting HyperTransport bus to be up.')
+
+@skipUnless(os.getenv('PMON_PATH'), 'PMON_PATH not available')
+@skipUnless(os.getenv('AVOCADO_ALLOW_UNTRUSTED_CODE'), 'untrusted code')
+def test_pmon_A1101_serial_console(self):
+"""
+:avocado: tags=arch:mips64el
+:avocado: tags=endian:little
+:avocado: tags=machine:loongson3-virt
+
[PATCH v2 09/12] qapi/gen: move write method to QAPIGenC, make fname a str
QAPIGenC and QAPIGenH in particular depend on fname being defined, but
we have a usage of QAPIGenCCode that isn't intended to be associated
with a particular file.
No problem, move the write method down to the class that actually needs
it, and keep QAPIGenCCode more abstract.
Signed-off-by: John Snow
---
Possibly un-needed in concert with a forthcoming patch by Markus, but I
didn't have it in my hands at time of publishing.
Signed-off-by: John Snow
---
scripts/qapi/commands.py | 2 +-
scripts/qapi/gen.py | 54
2 files changed, 28 insertions(+), 28 deletions(-)
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 71744f48a35..b346676d15a 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -258,7 +258,7 @@ def __init__(self, prefix: str):
super().__init__(
prefix, 'qapi-commands',
' * Schema-defined QAPI/QMP commands', None, __doc__)
-self._regy = QAPIGenCCode(None)
+self._regy = QAPIGenCCode()
self._visited_ret_types: Dict[QAPIGenC, Set[QAPISchemaType]] = {}
def _begin_user_module(self, name: str) -> None:
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 12ea98fb61e..2dd99635e74 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -36,8 +36,7 @@
class QAPIGen:
-def __init__(self, fname: Optional[str]):
-self.fname = fname
+def __init__(self) -> None:
self._preamble = ''
self._body = ''
@@ -58,28 +57,6 @@ def _bottom(self) -> str:
# pylint: disable=no-self-use
return ''
-def write(self, output_dir: str) -> None:
-# Include paths starting with ../ are used to reuse modules of the main
-# schema in specialised schemas. Don't overwrite the files that are
-# already generated for the main schema.
-if self.fname.startswith('../'):
-return
-pathname = os.path.join(output_dir, self.fname)
-odir = os.path.dirname(pathname)
-
-if odir:
-os.makedirs(odir, exist_ok=True)
-
-# use os.open for O_CREAT to create and read a non-existant file
-fd = os.open(pathname, os.O_RDWR | os.O_CREAT, 0o666)
-with os.fdopen(fd, 'r+', encoding='utf-8') as fp:
-text = self.get_content()
-oldtext = fp.read(len(text) + 1)
-if text != oldtext:
-fp.seek(0)
-fp.truncate(0)
-fp.write(text)
-
def _wrap_ifcond(ifcond: List[str], before: str, after: str) -> str:
if before == after:
@@ -121,8 +98,8 @@ def build_params(arg_type: Optional[QAPISchemaObjectType],
class QAPIGenCCode(QAPIGen):
-def __init__(self, fname: Optional[str]):
-super().__init__(fname)
+def __init__(self) -> None:
+super().__init__()
self._start_if: Optional[Tuple[List[str], str, str]] = None
def start_if(self, ifcond: List[str]) -> None:
@@ -147,11 +124,34 @@ def get_content(self) -> str:
class QAPIGenC(QAPIGenCCode):
def __init__(self, fname: str, blurb: str, pydoc: str):
-super().__init__(fname)
+super().__init__()
+self.fname = fname
self._blurb = blurb
self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
re.MULTILINE))
+def write(self, output_dir: str) -> None:
+# Include paths starting with ../ are used to reuse modules of the main
+# schema in specialised schemas. Don't overwrite the files that are
+# already generated for the main schema.
+if self.fname.startswith('../'):
+return
+pathname = os.path.join(output_dir, self.fname)
+odir = os.path.dirname(pathname)
+
+if odir:
+os.makedirs(odir, exist_ok=True)
+
+# use os.open for O_CREAT to create and read a non-existant file
+fd = os.open(pathname, os.O_RDWR | os.O_CREAT, 0o666)
+with os.fdopen(fd, 'r+', encoding='utf-8') as fp:
+text = self.get_content()
+oldtext = fp.read(len(text) + 1)
+if text != oldtext:
+fp.seek(0)
+fp.truncate(0)
+fp.write(text)
+
def _top(self) -> str:
return mcgen('''
/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
--
2.26.2
[PATCH v2 06/12] qapi/source: Add builtin null-object sentinel
We use None to represent an object that has no source information
because it's a builtin. This complicates interface typing, since many
interfaces expect that there is an info object available to print errors
with.
Introduce a special QAPISourceInfo that represents these built-ins so
that if an error should so happen to occur relating to one of these
builtins that we will be able to print its information, and interface
typing becomes simpler: you will always have a source info object.
This object will evaluate as False, so "if info" remains a valid
idiomatic construct.
NB: It was intentional to not allow empty constructors or similar to
create "empty" source info objects; callers must explicitly invoke
'builtin()' to pro-actively opt into using the sentinel. This should
prevent use-by-accident.
Signed-off-by: John Snow
---
scripts/qapi/source.py | 20 +++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index d7a79a9b8ae..a049b73b57b 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -11,7 +11,12 @@
import copy
import sys
-from typing import List, Optional, TypeVar
+from typing import (
+List,
+Optional,
+Type,
+TypeVar,
+)
class QAPISchemaPragma:
@@ -41,6 +46,17 @@ def __init__(self, fname: str, line: int,
self.defn_meta: Optional[str] = None
self.defn_name: Optional[str] = None
+@classmethod
+def builtin(cls: Type[T]) -> T:
+"""
+Create an instance corresponding to a built-in definition.
+"""
+return cls('', -1, None)
+
+def __bool__(self) -> bool:
+# "if info" is false if @info corresponds to a built-in definition.
+return bool(self.fname)
+
def set_defn(self, meta: str, name: str) -> None:
self.defn_meta = meta
self.defn_name = name
@@ -73,4 +89,6 @@ def include_path(self) -> str:
return ret
def __str__(self) -> str:
+if not bool(self):
+return '[builtin]'
return self.include_path() + self.in_defn() + self.loc()
--
2.26.2
[PATCH v2 10/12] tests/qapi-schema: Add quotes to module name in test output
A forthcoming patch is going to allow the empty string as a name for the builtin module, and quotes will help us see that in test output. Without this, git will be upset about trailing empty spaces in test output, so the quotes are necessary. Signed-off-by: John Snow --- tests/qapi-schema/comments.out | 4 ++-- tests/qapi-schema/doc-good.out | 4 ++-- tests/qapi-schema/empty.out | 4 ++-- tests/qapi-schema/event-case.out | 4 ++-- tests/qapi-schema/include-repetition.out | 8 tests/qapi-schema/include-simple.out | 6 +++--- tests/qapi-schema/indented-expr.out | 4 ++-- tests/qapi-schema/qapi-schema-test.out | 8 tests/qapi-schema/test-qapi.py | 2 +- 9 files changed, 22 insertions(+), 22 deletions(-) diff --git a/tests/qapi-schema/comments.out b/tests/qapi-schema/comments.out index 273f0f54e16..08aba8354e2 100644 --- a/tests/qapi-schema/comments.out +++ b/tests/qapi-schema/comments.out @@ -1,4 +1,4 @@ -module None +module "None" object q_empty enum QType prefix QTYPE @@ -9,7 +9,7 @@ enum QType member qdict member qlist member qbool -module comments.json +module "comments.json" enum Status member good member bad diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out index 419284dae29..83a3d9bd69b 100644 --- a/tests/qapi-schema/doc-good.out +++ b/tests/qapi-schema/doc-good.out @@ -1,4 +1,4 @@ -module None +module "None" object q_empty enum QType prefix QTYPE @@ -9,7 +9,7 @@ enum QType member qdict member qlist member qbool -module doc-good.json +module "doc-good.json" enum Enum member one if ['defined(IFONE)'] diff --git a/tests/qapi-schema/empty.out b/tests/qapi-schema/empty.out index 69666c39ad2..0dac23c80c1 100644 --- a/tests/qapi-schema/empty.out +++ b/tests/qapi-schema/empty.out @@ -1,4 +1,4 @@ -module None +module "None" object q_empty enum QType prefix QTYPE @@ -9,4 +9,4 @@ enum QType member qdict member qlist member qbool -module empty.json +module "empty.json" diff --git a/tests/qapi-schema/event-case.out b/tests/qapi-schema/event-case.out index 42ae519656d..ace511ba5a9 100644 --- a/tests/qapi-schema/event-case.out +++ b/tests/qapi-schema/event-case.out @@ -1,4 +1,4 @@ -module None +module "None" object q_empty enum QType prefix QTYPE @@ -9,6 +9,6 @@ enum QType member qdict member qlist member qbool -module event-case.json +module "event-case.json" event oops None boxed=False diff --git a/tests/qapi-schema/include-repetition.out b/tests/qapi-schema/include-repetition.out index 0b654ddebb6..f7ab4987943 100644 --- a/tests/qapi-schema/include-repetition.out +++ b/tests/qapi-schema/include-repetition.out @@ -1,4 +1,4 @@ -module None +module "None" object q_empty enum QType prefix QTYPE @@ -9,15 +9,15 @@ enum QType member qdict member qlist member qbool -module include-repetition.json +module "include-repetition.json" include comments.json include include-repetition-sub.json include comments.json -module comments.json +module "comments.json" enum Status member good member bad member ugly -module include-repetition-sub.json +module "include-repetition-sub.json" include comments.json include comments.json diff --git a/tests/qapi-schema/include-simple.out b/tests/qapi-schema/include-simple.out index 061f81e5090..81bdeb887b6 100644 --- a/tests/qapi-schema/include-simple.out +++ b/tests/qapi-schema/include-simple.out @@ -1,4 +1,4 @@ -module None +module "None" object q_empty enum QType prefix QTYPE @@ -9,9 +9,9 @@ enum QType member qdict member qlist member qbool -module include-simple.json +module "include-simple.json" include include-simple-sub.json -module include-simple-sub.json +module "include-simple-sub.json" enum Status member good member bad diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out index 04356775cd1..361a58185e6 100644 --- a/tests/qapi-schema/indented-expr.out +++ b/tests/qapi-schema/indented-expr.out @@ -1,4 +1,4 @@ -module None +module "None" object q_empty enum QType prefix QTYPE @@ -9,7 +9,7 @@ enum QType member qdict member qlist member qbool -module indented-expr.json +module "indented-expr.json" command eins None -> None gen=True success_response=True boxed=False oob=False preconfig=False command zwei None -> None diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 8868ca0dca9..4f5ab9fd596 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -1,4 +1,4 @@ -module None +module "None" object q_empty enum QType prefix QTYPE @@ -9,7 +9,7 @@ enum QType member qdict member qlist member qbool -module qapi-schema-test.json +module "qapi-schema-test.json" object TestStruct member integer: int optional=False
[PATCH v2 11/12] qapi/schema: Name the builtin module "" instead of None
Instead of using None as the built-in module filename, use an empty
string instead. This allows us to clarify the type of various interfaces
dealing with module names as always taking a string, which saves us from
having to use Optional[str] everywhere.
Signed-off-by: John Snow
---
scripts/qapi/gen.py | 6 +++---
scripts/qapi/schema.py | 12 ++--
scripts/qapi/types.py| 2 +-
scripts/qapi/visit.py| 2 +-
tests/qapi-schema/comments.out | 2 +-
tests/qapi-schema/doc-good.out | 2 +-
tests/qapi-schema/empty.out | 2 +-
tests/qapi-schema/event-case.out | 2 +-
tests/qapi-schema/include-repetition.out | 2 +-
tests/qapi-schema/include-simple.out | 2 +-
tests/qapi-schema/indented-expr.out | 2 +-
tests/qapi-schema/qapi-schema-test.out | 2 +-
12 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 2dd99635e74..1933090a2a0 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -310,14 +310,14 @@ def write(self, output_dir: str, opt_builtins: bool =
False) -> None:
genc.write(output_dir)
genh.write(output_dir)
-def _begin_system_module(self, name: None) -> None:
+def _begin_system_module(self, name: str) -> None:
pass
def _begin_user_module(self, name: str) -> None:
pass
-def visit_module(self, name: Optional[str]) -> None:
-if name is None:
+def visit_module(self, name: str) -> None:
+if not name:
if self._builtin_blurb:
self._add_system_module('builtin', self._builtin_blurb)
self._begin_system_module(name)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 0449771dfe5..0235208966a 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -69,7 +69,7 @@ def check_doc(self):
def _set_module(self, schema, info):
assert self._checked
-self._module = schema.module_by_fname(info.fname if info else None)
+self._module = schema.module_by_fname(info.fname)
self._module.add_entity(self)
def set_module(self, schema):
@@ -826,7 +826,7 @@ def __init__(self, fname):
self._entity_dict = {}
self._module_dict = OrderedDict()
self._schema_dir = os.path.dirname(fname)
-self._make_module(None) # built-ins
+self._make_module(QAPISourceInfo.builtin().fname) # built-ins
self._make_module(fname)
self._predefining = True
self._def_predefineds()
@@ -871,10 +871,10 @@ def resolve_type(self, name, info, what):
info, "%s uses unknown type '%s'" % (what, name))
return typ
-def _module_name(self, fname):
-if fname is None:
-return None
-return os.path.relpath(fname, self._schema_dir)
+def _module_name(self, fname: str) -> str:
+if fname:
+return os.path.relpath(fname, self._schema_dir)
+return fname
def _make_module(self, fname):
name = self._module_name(fname)
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index a3a16284006..12eeea3aaff 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -272,7 +272,7 @@ def __init__(self, prefix: str):
prefix, 'qapi-types', ' * Schema-defined QAPI types',
' * Built-in QAPI types', __doc__)
-def _begin_system_module(self, name: None) -> None:
+def _begin_system_module(self, name: str) -> None:
self._genc.preamble_add(mcgen('''
#include "qemu/osdep.h"
#include "qapi/dealloc-visitor.h"
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 3f49c307c56..76e34ee7f02 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -305,7 +305,7 @@ def __init__(self, prefix: str):
prefix, 'qapi-visit', ' * Schema-defined QAPI visitors',
' * Built-in QAPI visitors', __doc__)
-def _begin_system_module(self, name: None) -> None:
+def _begin_system_module(self, name: str) -> None:
self._genc.preamble_add(mcgen('''
#include "qemu/osdep.h"
#include "qapi/error.h"
diff --git a/tests/qapi-schema/comments.out b/tests/qapi-schema/comments.out
index 08aba8354e2..02000c06e5e 100644
--- a/tests/qapi-schema/comments.out
+++ b/tests/qapi-schema/comments.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 83a3d9bd69b..494533d7479 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -1,4 +1,4 @@
-module "None"
+module ""
object q_empty
enum QType
prefix QTYPE
diff --git a/tests/qapi-schema/empty.out b/tests/qapi-schema/empty.out
index 0dac23c80c1..059caa4e1d2 100644
--- a/tests/qapi-schema/empty.out
+++ b/tests/qapi-schema/empty.out
@@ -1,4 +1,4 @@
[PATCH v2 08/12] qapi/gen: write _genc/_genh access shims
Many places assume they can access these fields without checking them
first to ensure they are defined. Eliminating the _genc and _genh fields
and replacing them with functional properties that check for correct
state can ease the typing overhead by eliminating the Optional[T] return
type.
Signed-off-by: John Snow
---
scripts/qapi/gen.py | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 476d0adac4e..12ea98fb61e 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -243,11 +243,20 @@ def __init__(self,
self._user_blurb = user_blurb
self._builtin_blurb = builtin_blurb
self._pydoc = pydoc
-self._genc: Optional[QAPIGenC] = None
-self._genh: Optional[QAPIGenH] = None
+self._current_module: Optional[str] = None
self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
self._main_module: Optional[str] = None
+@property
+def _genc(self) -> QAPIGenC:
+assert self._current_module is not None
+return self._module[self._current_module][0]
+
+@property
+def _genh(self) -> QAPIGenH:
+assert self._current_module is not None
+return self._module[self._current_module][1]
+
@staticmethod
def _is_user_module(name: str) -> bool:
return not name.startswith('./')
@@ -282,7 +291,7 @@ def _add_module(self, name: str, blurb: str) -> None:
genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
self._module[name] = (genc, genh)
-self._genc, self._genh = self._module[name]
+self._current_module = name
def _add_user_module(self, name: str, blurb: str) -> None:
assert self._is_user_module(name)
@@ -315,8 +324,7 @@ def visit_module(self, name: Optional[str]) -> None:
else:
# The built-in module has not been created. No code may
# be generated.
-self._genc = None
-self._genh = None
+self._current_module = None
else:
self._add_user_module(name, self._user_blurb)
self._begin_user_module(name)
--
2.26.2
[PATCH v2 05/12] qapi/gen: use './builtin' for the built-in module name
Use this in preference to 'None', which helps remove some edge cases in
the typing.
Signed-off-by: John Snow
---
scripts/qapi/gen.py | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index a6dc991b1d0..476d0adac4e 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -245,23 +245,23 @@ def __init__(self,
self._pydoc = pydoc
self._genc: Optional[QAPIGenC] = None
self._genh: Optional[QAPIGenH] = None
-self._module: Dict[Optional[str], Tuple[QAPIGenC, QAPIGenH]] = {}
+self._module: Dict[str, Tuple[QAPIGenC, QAPIGenH]] = {}
self._main_module: Optional[str] = None
@staticmethod
-def _is_user_module(name: Optional[str]) -> bool:
-return bool(name and not name.startswith('./'))
+def _is_user_module(name: str) -> bool:
+return not name.startswith('./')
@staticmethod
-def _is_builtin_module(name: Optional[str]) -> bool:
-return not name
+def _is_builtin_module(name: str) -> bool:
+return name == './builtin'
-def _module_dirname(self, name: Optional[str]) -> str:
+def _module_dirname(self, name: str) -> str:
if self._is_user_module(name):
return os.path.dirname(name)
return ''
-def _module_basename(self, what: str, name: Optional[str]) -> str:
+def _module_basename(self, what: str, name: str) -> str:
ret = '' if self._is_builtin_module(name) else self._prefix
if self._is_user_module(name):
basename = os.path.basename(name)
@@ -269,15 +269,15 @@ def _module_basename(self, what: str, name:
Optional[str]) -> str:
if name != self._main_module:
ret += '-' + os.path.splitext(basename)[0]
else:
-name = name[2:] if name else 'builtin'
-ret += re.sub(r'-', '-' + name + '-', what)
+assert name.startswith('./')
+ret += re.sub(r'-', '-' + name[2:] + '-', what)
return ret
-def _module_filename(self, what: str, name: Optional[str]) -> str:
+def _module_filename(self, what: str, name: str) -> str:
return os.path.join(self._module_dirname(name),
self._module_basename(what, name))
-def _add_module(self, name: Optional[str], blurb: str) -> None:
+def _add_module(self, name: str, blurb: str) -> None:
basename = self._module_filename(self._what, name)
genc = QAPIGenC(basename + '.c', blurb, self._pydoc)
genh = QAPIGenH(basename + '.h', blurb, self._pydoc)
@@ -290,8 +290,8 @@ def _add_user_module(self, name: str, blurb: str) -> None:
self._main_module = name
self._add_module(name, blurb)
-def _add_system_module(self, name: Optional[str], blurb: str) -> None:
-self._add_module(name and './' + name, blurb)
+def _add_system_module(self, name: str, blurb: str) -> None:
+self._add_module('./' + name, blurb)
def write(self, output_dir: str, opt_builtins: bool = False) -> None:
for name in self._module:
@@ -310,7 +310,7 @@ def _begin_user_module(self, name: str) -> None:
def visit_module(self, name: Optional[str]) -> None:
if name is None:
if self._builtin_blurb:
-self._add_system_module(None, self._builtin_blurb)
+self._add_system_module('builtin', self._builtin_blurb)
self._begin_system_module(name)
else:
# The built-in module has not been created. No code may
--
2.26.2
[PATCH v2 07/12] qapi/schema: make QAPISourceInfo mandatory
Signed-off-by: John Snow
---
The event_enum_members change might become irrelevant after a
forthcoming (?) patch by Markus, but didn't have it in-hand at time of
publishing.
Signed-off-by: John Snow
---
scripts/qapi/events.py | 2 +-
scripts/qapi/schema.py | 25 ++---
scripts/qapi/types.py | 9 +
scripts/qapi/visit.py | 6 +++---
4 files changed, 23 insertions(+), 19 deletions(-)
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 9851653b9d1..9ba4f109028 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -225,7 +225,7 @@ def visit_event(self,
self._event_emit_name))
# Note: we generate the enum member regardless of @ifcond, to
# keep the enumeration usable in target-independent code.
-self._event_enum_members.append(QAPISchemaEnumMember(name, None))
+self._event_enum_members.append(QAPISchemaEnumMember(name, info))
def gen_events(schema: QAPISchema,
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 720449feee4..0449771dfe5 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -23,6 +23,7 @@
from .error import QAPIError, QAPISemError
from .expr import check_exprs
from .parser import QAPISchemaParser
+from .source import QAPISourceInfo
class QAPISchemaEntity:
@@ -36,10 +37,10 @@ def __init__(self, name, info, doc, ifcond=None,
features=None):
self.name = name
self._module = None
# For explicitly defined entities, info points to the (explicit)
-# definition. For builtins (and their arrays), info is None.
-# For implicitly defined entities, info points to a place that
-# triggered the implicit definition (there may be more than one
-# such place).
+# definition. For built-in types (and their arrays), info is a
+# special object that evaluates to False. For implicitly defined
+# entities, info points to a place that triggered the implicit
+# definition (there may be more than one such place).
self.info = info
self.doc = doc
self._ifcond = ifcond or []
@@ -68,7 +69,7 @@ def check_doc(self):
def _set_module(self, schema, info):
assert self._checked
-self._module = schema.module_by_fname(info and info.fname)
+self._module = schema.module_by_fname(info.fname if info else None)
self._module.add_entity(self)
def set_module(self, schema):
@@ -209,7 +210,7 @@ class QAPISchemaBuiltinType(QAPISchemaType):
meta = 'built-in'
def __init__(self, name, json_type, c_type):
-super().__init__(name, None, None)
+super().__init__(name, QAPISourceInfo.builtin(), None)
assert not c_type or isinstance(c_type, str)
assert json_type in ('string', 'number', 'int', 'boolean', 'null',
'value')
@@ -897,7 +898,7 @@ def _def_builtin_type(self, name, json_type, c_type):
# be nice, but we can't as long as their generated code
# (qapi-builtin-types.[ch]) may be shared by some other
# schema.
-self._make_array_type(name, None)
+self._make_array_type(name, QAPISourceInfo.builtin())
def _def_predefineds(self):
for t in [('str','string', 'char' + POINTER_SUFFIX),
@@ -917,16 +918,18 @@ def _def_predefineds(self):
('null', 'null','QNull' + POINTER_SUFFIX)]:
self._def_builtin_type(*t)
self.the_empty_object_type = QAPISchemaObjectType(
-'q_empty', None, None, None, None, None, [], None)
+'q_empty', QAPISourceInfo.builtin(),
+None, None, None, None, [], None)
self._def_entity(self.the_empty_object_type)
qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist',
'qbool']
qtype_values = self._make_enum_members(
-[{'name': n} for n in qtypes], None)
+[{'name': n} for n in qtypes], QAPISourceInfo.builtin())
-self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
-qtype_values, 'QTYPE'))
+self._def_entity(QAPISchemaEnumType(
+'QType', QAPISourceInfo.builtin(), None,
+None, None, qtype_values, 'QTYPE'))
def _make_features(self, features, info):
if features is None:
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 2b4916cdaa1..a3a16284006 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -71,7 +71,8 @@ def gen_enum(name: str,
members: List[QAPISchemaEnumMember],
prefix: Optional[str] = None) -> str:
# append automatically generated _MAX value
-enum_members = members + [QAPISchemaEnumMember('_MAX', None)]
+enum_members = members + [
+QAPISchemaEnumMember('_MAX', QAPISourceInfo.builtin())]
ret = mcgen('''
@@
[PATCH v2 04/12] qapi/gen: assert that _start_if is not None in _wrap_ifcond
We already assert this in end_if, but that's opaque to mypy. Do it in _wrap_ifcond instead. Same effect at runtime, but mypy can now infer the type in _wrap_ifcond's body. Signed-off-by: John Snow --- scripts/qapi/gen.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index b40f18eee3c..a6dc991b1d0 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -130,11 +130,11 @@ def start_if(self, ifcond: List[str]) -> None: self._start_if = (ifcond, self._body, self._preamble) def end_if(self) -> None: -assert self._start_if self._wrap_ifcond() self._start_if = None def _wrap_ifcond(self) -> None: +assert self._start_if self._body = _wrap_ifcond(self._start_if[0], self._start_if[1], self._body) self._preamble = _wrap_ifcond(self._start_if[0], -- 2.26.2
[PATCH v2 00/12] qapi: static typing conversion, pt1.5
Hi, this patchset enables strict optional checking in mypy for everything we have typed so far. In general, this patchset seeks to eliminate Optional[T] in favor of T wherever possible. Optional types used for object properties, function/method parameters and return values where we expect, in most cases, to be non-None is troublesome as it requires peppering the code with assertions about state. If and whenever possible, prefer using non-Optional types. Ironing out these issues allows us to be even stricter with our type checking, which improves consistency in subclass interface types and should make review a little nicer. This series is based on (but does not require) the 'qapi: sphinx-autodoc for qapi module' series. V2: 001/12:[] [--] 'qapi/commands: assert arg_type is not None' 002/12:[] [--] 'qapi/events: fix visit_event typing' 003/12:[0003] [FC] 'qapi/main: handle theoretical None-return from re.match()' 004/12:[] [--] 'qapi/gen: assert that _start_if is not None in _wrap_ifcond' 005/12:[0003] [FC] 'qapi/gen: use './builtin' for the built-in module name' 006/12:[0004] [FC] 'qapi/source: Add builtin null-object sentinel' 007/12:[0024] [FC] 'qapi/schema: make QAPISourceInfo mandatory' 008/12:[] [--] 'qapi/gen: write _genc/_genh access shims' 009/12:[] [--] 'qapi/gen: move write method to QAPIGenC, make fname a str' 010/12:[] [--] 'tests/qapi-schema: Add quotes to module name in test output' 011/12:[0004] [FC] 'qapi/schema: Name the builtin module "" instead of None' 012/12:[] [--] 'qapi: enable strict-optional checks' - This revision isn't quite complete, but due to deadlines and timezones, opted to send the "revision so far". It keeps some imperfect fixes that Markus is devising better alternatives for. John Snow (12): qapi/commands: assert arg_type is not None qapi/events: fix visit_event typing qapi/main: handle theoretical None-return from re.match() qapi/gen: assert that _start_if is not None in _wrap_ifcond qapi/gen: use './builtin' for the built-in module name qapi/source: Add builtin null-object sentinel qapi/schema: make QAPISourceInfo mandatory qapi/gen: write _genc/_genh access shims qapi/gen: move write method to QAPIGenC, make fname a str tests/qapi-schema: Add quotes to module name in test output qapi/schema: Name the builtin module "" instead of None qapi: enable strict-optional checks scripts/qapi/commands.py | 11 ++- scripts/qapi/events.py | 14 +-- scripts/qapi/gen.py | 108 --- scripts/qapi/main.py | 2 + scripts/qapi/mypy.ini| 1 - scripts/qapi/schema.py | 35 scripts/qapi/source.py | 20 - scripts/qapi/types.py| 11 +-- scripts/qapi/visit.py| 8 +- tests/qapi-schema/comments.out | 4 +- tests/qapi-schema/doc-good.out | 4 +- tests/qapi-schema/empty.out | 4 +- tests/qapi-schema/event-case.out | 4 +- tests/qapi-schema/include-repetition.out | 8 +- tests/qapi-schema/include-simple.out | 6 +- tests/qapi-schema/indented-expr.out | 4 +- tests/qapi-schema/qapi-schema-test.out | 8 +- tests/qapi-schema/test-qapi.py | 2 +- 18 files changed, 145 insertions(+), 109 deletions(-) -- 2.26.2
[PATCH v2 03/12] qapi/main: handle theoretical None-return from re.match()
Mypy cannot understand that this match can never be None, so help it along. Signed-off-by: John Snow --- scripts/qapi/main.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py index 42517210b80..703e7ed1ed5 100644 --- a/scripts/qapi/main.py +++ b/scripts/qapi/main.py @@ -23,6 +23,8 @@ def invalid_prefix_char(prefix: str) -> Optional[str]: match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix) +# match cannot be None, but mypy cannot infer that. +assert match is not None if match.end() != len(prefix): return prefix[match.end()] return None -- 2.26.2
[PATCH v2 02/12] qapi/events: fix visit_event typing
Actually, the arg_type can indeed be Optional.
Signed-off-by: John Snow
---
scripts/qapi/events.py | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 599f3d1f564..9851653b9d1 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -12,7 +12,7 @@
See the COPYING file in the top-level directory.
"""
-from typing import List
+from typing import List, Optional
from .common import c_enum_const, c_name, mcgen
from .gen import QAPISchemaModularCVisitor, build_params, ifcontext
@@ -27,7 +27,7 @@
def build_event_send_proto(name: str,
- arg_type: QAPISchemaObjectType,
+ arg_type: Optional[QAPISchemaObjectType],
boxed: bool) -> str:
return 'void qapi_event_send_%(c_name)s(%(param)s)' % {
'c_name': c_name(name.lower()),
@@ -35,7 +35,7 @@ def build_event_send_proto(name: str,
def gen_event_send_decl(name: str,
-arg_type: QAPISchemaObjectType,
+arg_type: Optional[QAPISchemaObjectType],
boxed: bool) -> str:
return mcgen('''
@@ -78,7 +78,7 @@ def gen_param_var(typ: QAPISchemaObjectType) -> str:
def gen_event_send(name: str,
- arg_type: QAPISchemaObjectType,
+ arg_type: Optional[QAPISchemaObjectType],
boxed: bool,
event_enum_name: str,
event_emit: str) -> str:
@@ -99,6 +99,7 @@ def gen_event_send(name: str,
proto=build_event_send_proto(name, arg_type, boxed))
if have_args:
+assert arg_type is not None
ret += mcgen('''
QObject *obj;
Visitor *v;
@@ -114,6 +115,7 @@ def gen_event_send(name: str,
name=name)
if have_args:
+assert arg_type is not None
ret += mcgen('''
v = qobject_output_visitor_new();
''')
@@ -214,7 +216,7 @@ def visit_event(self,
info: QAPISourceInfo,
ifcond: List[str],
features: List[QAPISchemaFeature],
-arg_type: QAPISchemaObjectType,
+arg_type: Optional[QAPISchemaObjectType],
boxed: bool) -> None:
with ifcontext(ifcond, self._genh, self._genc):
self._genh.add(gen_event_send_decl(name, arg_type, boxed))
--
2.26.2
[PATCH v2 01/12] qapi/commands: assert arg_type is not None
when boxed is true, expr.py asserts that we must have
arguments. Ultimately, this should mean that if boxed is True, that
arg_type should be defined. Mypy cannot infer this, and does not support
'stateful' type inference, e.g.:
```
if x:
assert y is not None
...
if x:
y.etc()
```
does not work, because mypy does not statefully remember the conditional
assertion in the second block. Help mypy out by creating a new local
that it can track more easily.
Signed-off-by: John Snow
---
scripts/qapi/commands.py | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 50978090b44..71744f48a35 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -126,6 +126,9 @@ def gen_marshal(name: str,
boxed: bool,
ret_type: Optional[QAPISchemaType]) -> str:
have_args = boxed or (arg_type and not arg_type.is_empty())
+if have_args:
+assert arg_type is not None
+arg_type_c_name = arg_type.c_name()
ret = mcgen('''
@@ -147,7 +150,7 @@ def gen_marshal(name: str,
ret += mcgen('''
%(c_name)s arg = {0};
''',
- c_name=arg_type.c_name())
+ c_name=arg_type_c_name)
ret += mcgen('''
@@ -163,7 +166,7 @@ def gen_marshal(name: str,
ok = visit_check_struct(v, errp);
}
''',
- c_arg_type=arg_type.c_name())
+ c_arg_type=arg_type_c_name)
else:
ret += mcgen('''
ok = visit_check_struct(v, errp);
@@ -193,7 +196,7 @@ def gen_marshal(name: str,
ret += mcgen('''
visit_type_%(c_arg_type)s_members(v, , NULL);
''',
- c_arg_type=arg_type.c_name())
+ c_arg_type=arg_type_c_name)
ret += mcgen('''
visit_end_struct(v, NULL);
--
2.26.2
[PATCH v3 10/13] qapi/introspect.py: improve readability of _tree_to_qlit
Subjective, but I find getting rid of the comprehensions helps. Also,
divide the sections into scalar and non-scalar sections, and remove
old-style string formatting.
Signed-off-by: John Snow
---
scripts/qapi/introspect.py | 33 ++---
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index c72bd27..60ec326d2c7 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -89,7 +89,7 @@ def indent(level):
ret = ''
if obj.comment:
-ret += indent(level) + '/* %s */\n' % obj.comment
+ret += indent(level) + f"/* {obj.comment} */\n"
if obj.ifcond:
ret += gen_if(obj.ifcond)
ret += _tree_to_qlit(obj.value, level, suppress_first_indent)
@@ -100,33 +100,36 @@ def indent(level):
ret = ''
if not suppress_first_indent:
ret += indent(level)
+
+# Scalars:
if obj is None:
ret += 'QLIT_QNULL'
elif isinstance(obj, str):
-ret += 'QLIT_QSTR(' + to_c_string(obj) + ')'
+ret += f"QLIT_QSTR({to_c_string(obj)})"
+elif isinstance(obj, bool):
+ret += f"QLIT_QBOOL({str(obj).lower()})"
+
+# Non-scalars:
elif isinstance(obj, list):
-elts = [_tree_to_qlit(elt, level + 1).strip('\n')
-for elt in obj]
-elts.append(indent(level + 1) + "{}")
ret += 'QLIT_QLIST(((QLitObject[]) {\n'
-ret += '\n'.join(elts) + '\n'
+for value in obj:
+ret += _tree_to_qlit(value, level + 1).strip('\n') + '\n'
+ret += indent(level + 1) + '{}\n'
ret += indent(level) + '}))'
elif isinstance(obj, dict):
-elts = []
-for key, value in sorted(obj.items()):
-elts.append(indent(level + 1) + '{ %s, %s }' %
-(to_c_string(key),
- _tree_to_qlit(value, level + 1, True)))
-elts.append(indent(level + 1) + '{}')
ret += 'QLIT_QDICT(((QLitDictEntry[]) {\n'
-ret += ',\n'.join(elts) + '\n'
+for key, value in sorted(obj.items()):
+ret += indent(level + 1) + "{{ {:s}, {:s} }},\n".format(
+to_c_string(key),
+_tree_to_qlit(value, level + 1, suppress_first_indent=True)
+)
+ret += indent(level + 1) + '{}\n'
ret += indent(level) + '}))'
-elif isinstance(obj, bool):
-ret += 'QLIT_QBOOL(%s)' % ('true' if obj else 'false')
else:
raise NotImplementedError(
f"type '{type(obj).__name__}' not implemented"
)
+
if level > 0:
ret += ','
return ret
--
2.26.2
[PATCH v3 13/13] qapi/introspect.py: Add docstring to _tree_to_qlit
Signed-off-by: John Snow Reviewed-by: Cleber Rosa Signed-off-by: John Snow --- scripts/qapi/introspect.py | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index 428397a6954..8d5b8513fc8 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -1,10 +1,11 @@ """ QAPI introspection generator -Copyright (C) 2015-2018 Red Hat, Inc. +Copyright (C) 2015-2020 Red Hat, Inc. Authors: Markus Armbruster + John Snow This work is licensed under the terms of the GNU GPL, version 2. See the COPYING file in the top-level directory. @@ -96,6 +97,13 @@ def __init__(self, value: _NodeT, ifcond: Iterable[str], def _tree_to_qlit(obj: TreeValue, level: int = 0, suppress_first_indent: bool = False) -> str: +""" +Convert the type tree into a QLIT C string, recursively. + +:param obj: The value to convert. +:param level: The indentation level for this particular value. +:param suppress_first_indent: True for dict value children. +""" def indent(level: int) -> str: return level * 4 * ' ' -- 2.26.2
[PATCH v3 07/13] qapi/introspect.py: Introduce preliminary tree typing
The types will be used in forthcoming patches to add typing. These types
describe the layout and structure of the objects passed to
_tree_to_qlit, but lack the power to describe annotations until the next
commit.
Signed-off-by: John Snow
---
scripts/qapi/introspect.py | 30 +-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 0aa3b77109f..b82efe16f6e 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -10,7 +10,13 @@
See the COPYING file in the top-level directory.
"""
-from typing import Optional
+from typing import (
+Any,
+Dict,
+List,
+Optional,
+Union,
+)
from .common import (
c_name,
@@ -26,6 +32,28 @@
)
+# This module constructs a tree data structure that is used to
+# generate the introspection information for QEMU. It behaves similarly
+# to a JSON value.
+#
+# A complexity over JSON is that our values may or may not be annotated.
+#
+# Un-annotated values may be:
+# Scalar: str, bool, None.
+# Non-scalar: List, Dict
+# _value = Union[str, bool, None, Dict[str, TreeValue], List[TreeValue]]
+#
+# With optional annotations, the type of all values is:
+# TreeValue = Union[_value, Annotated[_value]]
+#
+# Sadly, mypy does not support recursive types, so we must approximate this.
+_stub = Any
+_scalar = Union[str, bool, None]
+_nonscalar = Union[Dict[str, _stub], List[_stub]]
+_value = Union[_scalar, _nonscalar]
+# TreeValue = Union[_value, 'Annotated[_value]']
+
+
def _make_tree(obj, ifcond, comment=None):
extra = {
'if': ifcond,
--
2.26.2
[PATCH v3 08/13] qapi/introspect.py: create a typed 'Annotated' data strutcure
Presently, we use a tuple to attach a dict containing annotations
(comments and compile-time conditionals) to a tree node. This is
undesirable because dicts are difficult to strongly type; promoting it
to a real class allows us to name the values and types of the
annotations we are expecting.
In terms of typing, the Annotated type serves as a generic container
where the annotated node's type is preserved, allowing for greater
specificity than we'd be able to provide without a generic.
Signed-off-by: John Snow
---
scripts/qapi/introspect.py | 77 ++
1 file changed, 44 insertions(+), 33 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index b82efe16f6e..2b90a52f016 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -13,8 +13,12 @@
from typing import (
Any,
Dict,
+Generic,
+Iterable,
List,
Optional,
+Tuple,
+TypeVar,
Union,
)
@@ -51,15 +55,25 @@
_scalar = Union[str, bool, None]
_nonscalar = Union[Dict[str, _stub], List[_stub]]
_value = Union[_scalar, _nonscalar]
-# TreeValue = Union[_value, 'Annotated[_value]']
+TreeValue = Union[_value, 'Annotated[_value]']
-def _make_tree(obj, ifcond, comment=None):
-extra = {
-'if': ifcond,
-'comment': comment,
-}
-return (obj, extra)
+_NodeT = TypeVar('_NodeT', bound=TreeValue)
+
+
+class Annotated(Generic[_NodeT]):
+"""
+Annotated generally contains a SchemaInfo-like type (as a dict),
+But it also used to wrap comments/ifconds around scalar leaf values,
+for the benefit of features and enums.
+"""
+# Remove after 3.7 adds @dataclass:
+# pylint: disable=too-few-public-methods
+def __init__(self, value: _NodeT, ifcond: Iterable[str],
+ comment: Optional[str] = None):
+self.value = value
+self.comment: Optional[str] = comment
+self.ifcond: Tuple[str, ...] = tuple(ifcond)
def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
@@ -67,24 +81,20 @@ def _tree_to_qlit(obj, level=0,
suppress_first_indent=False):
def indent(level):
return level * 4 * ' '
-if isinstance(obj, tuple):
-ifobj, extra = obj
-ifcond = extra.get('if')
-comment = extra.get('comment')
-
+if isinstance(obj, Annotated):
# NB: _tree_to_qlit is called recursively on the values of a key:value
# pair; those values can't be decorated with comments or conditionals.
msg = "dict values cannot have attached comments or if-conditionals."
assert not suppress_first_indent, msg
ret = ''
-if comment:
-ret += indent(level) + '/* %s */\n' % comment
-if ifcond:
-ret += gen_if(ifcond)
-ret += _tree_to_qlit(ifobj, level)
-if ifcond:
-ret += '\n' + gen_endif(ifcond)
+if obj.comment:
+ret += indent(level) + '/* %s */\n' % obj.comment
+if obj.ifcond:
+ret += gen_if(obj.ifcond)
+ret += _tree_to_qlit(obj.value, level, suppress_first_indent)
+if obj.ifcond:
+ret += '\n' + gen_endif(obj.ifcond)
return ret
ret = ''
@@ -201,7 +211,7 @@ def _use_type(self, typ):
@staticmethod
def _gen_features(features):
-return [_make_tree(f.name, f.ifcond) for f in features]
+return [Annotated(f.name, f.ifcond) for f in features]
def _gen_tree(self, name, mtype, obj, ifcond, features):
comment: Optional[str] = None
@@ -215,7 +225,7 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
obj['meta-type'] = mtype
if features:
obj['features'] = self._gen_features(features)
-self._trees.append(_make_tree(obj, ifcond, comment))
+self._trees.append(Annotated(obj, ifcond, comment))
def _gen_member(self, member):
obj = {'name': member.name, 'type': self._use_type(member.type)}
@@ -223,7 +233,7 @@ def _gen_member(self, member):
obj['default'] = None
if member.features:
obj['features'] = self._gen_features(member.features)
-return _make_tree(obj, member.ifcond)
+return Annotated(obj, member.ifcond)
def _gen_variants(self, tag_name, variants):
return {'tag': tag_name,
@@ -231,16 +241,17 @@ def _gen_variants(self, tag_name, variants):
def _gen_variant(self, variant):
obj = {'case': variant.name, 'type': self._use_type(variant.type)}
-return _make_tree(obj, variant.ifcond)
+return Annotated(obj, variant.ifcond)
def visit_builtin_type(self, name, info, json_type):
self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
def visit_enum_type(self, name, info, ifcond, features, members, prefix):
-self._gen_tree(name, 'enum',
- {'values': [_make_tree(m.name, m.ifcond, None)
-
[PATCH v3 12/13] qapi/instrospect.py: add introspect.json dummy types
Add some aliases that declare intent for some of the "dictly-typed"
objects we pass around in introspect.py.
Signed-off-by: John Snow
---
This patch is optional, it can be dropped if desired.
Signed-off-by: John Snow
---
scripts/qapi/introspect.py | 48 --
1 file changed, 30 insertions(+), 18 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 590898baf93..428397a6954 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -66,8 +66,14 @@
_value = Union[_scalar, _nonscalar]
TreeValue = Union[_value, 'Annotated[_value]']
-# This is a (strict) alias for an arbitrary object non-scalar, as above:
-_DObject = Dict[str, object]
+# We lack precise object types, so SchemaInfo, children, and related types are
+# typed here loosely as simply python Dicts.
+SchemaInfo = Dict[str, object]
+SchemaInfoObject = Dict[str, object]
+SchemaInfoObjectVariant = Dict[str, object]
+SchemaInfoObjectMember = Dict[str, object]
+SchemaInfoCommand = Dict[str, object]
+
_NodeT = TypeVar('_NodeT', bound=TreeValue)
@@ -160,7 +166,7 @@ def __init__(self, prefix: str, unmask: bool):
' * QAPI/QMP schema introspection', __doc__)
self._unmask = unmask
self._schema: Optional[QAPISchema] = None
-self._trees: List[Annotated[_DObject]] = []
+self._trees: List[Annotated[SchemaInfo]] = []
self._used_types: List[QAPISchemaType] = []
self._name_map: Dict[str, str] = {}
self._genc.add(mcgen('''
@@ -232,9 +238,18 @@ def _gen_features(features: List[QAPISchemaFeature]
) -> List[Annotated[str]]:
return [Annotated(f.name, f.ifcond) for f in features]
-def _gen_tree(self, name: str, mtype: str, obj: _DObject,
+def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
ifcond: List[str],
features: Optional[List[QAPISchemaFeature]]) -> None:
+"""
+Build and append a SchemaInfo object to self._trees.
+
+:param name: The entity's name.
+:param mtype: The entity's meta-type.
+:param obj: Additional entity fields, as appropriate for the meta-type.
+:param ifcond: List of conditionals that apply to this entire entity.
+:param features: Optional features field for SchemaInfo.
+"""
comment: Optional[str] = None
if mtype not in ('command', 'event', 'builtin', 'array'):
if not self._unmask:
@@ -249,8 +264,8 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
self._trees.append(Annotated(obj, ifcond, comment))
def _gen_member(self, member: QAPISchemaObjectTypeMember
-) -> Annotated[_DObject]:
-obj: _DObject = {
+) -> Annotated[SchemaInfoObjectMember]:
+obj: SchemaInfoObjectMember = {
'name': member.name,
'type': self._use_type(member.type)
}
@@ -260,13 +275,9 @@ def _gen_member(self, member: QAPISchemaObjectTypeMember
obj['features'] = self._gen_features(member.features)
return Annotated(obj, member.ifcond)
-def _gen_variants(self, tag_name: str,
- variants: List[QAPISchemaVariant]) -> _DObject:
-return {'tag': tag_name,
-'variants': [self._gen_variant(v) for v in variants]}
-
-def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated[_DObject]:
-obj: _DObject = {
+def _gen_variant(self, variant: QAPISchemaVariant
+ ) -> Annotated[SchemaInfoObjectVariant]:
+obj: SchemaInfoObjectVariant = {
'case': variant.name,
'type': self._use_type(variant.type)
}
@@ -298,11 +309,12 @@ def visit_object_type_flat(self, name: str, info:
QAPISourceInfo,
features: List[QAPISchemaFeature],
members: List[QAPISchemaObjectTypeMember],
variants: Optional[QAPISchemaVariants]) -> None:
-obj: _DObject = {'members': [self._gen_member(m) for m in members]}
+obj: SchemaInfoObject = {
+'members': [self._gen_member(m) for m in members]
+}
if variants:
-obj.update(self._gen_variants(variants.tag_member.name,
- variants.variants))
-
+obj['tag'] = variants.tag_member.name
+obj['variants'] = [self._gen_variant(v) for v in variants.variants]
self._gen_tree(name, 'object', obj, ifcond, features)
def visit_alternate_type(self, name: str, info: QAPISourceInfo,
@@ -326,7 +338,7 @@ def visit_command(self, name: str, info: QAPISourceInfo,
ifcond: List[str],
arg_type = arg_type or self._schema.the_empty_object_type
ret_type = ret_type or self._schema.the_empty_object_type
-obj: _DObject = {
+obj: SchemaInfoCommand
[PATCH v3 09/13] qapi/introspect.py: improve _tree_to_qlit error message
Trivial; make the error message just a pinch more explicit in case we
trip this by accident in the future.
Signed-off-by: John Snow
---
scripts/qapi/introspect.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 2b90a52f016..c72bd27 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -124,7 +124,9 @@ def indent(level):
elif isinstance(obj, bool):
ret += 'QLIT_QBOOL(%s)' % ('true' if obj else 'false')
else:
-assert False# not implemented
+raise NotImplementedError(
+f"type '{type(obj).__name__}' not implemented"
+)
if level > 0:
ret += ','
return ret
--
2.26.2
[PATCH v3 05/13] qapi/introspect.py: Unify return type of _make_tree()
Returning two different types conditionally can be complicated to
type. Return one type for consistency.
Signed-off-by: John Snow
---
scripts/qapi/introspect.py | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index ccdf4f1c0d0..d3fbf694ad2 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -29,9 +29,7 @@ def _make_tree(obj, ifcond, extra=None):
extra = {}
if ifcond:
extra['if'] = ifcond
-if extra:
-return (obj, extra)
-return obj
+return (obj, extra)
def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
--
2.26.2
[PATCH v2 12/12] qapi: enable strict-optional checks
In the modules that we are checking so far, we can be stricter about the difference between Optional[T] and T types. Enable that check. Enabling it now will assist review on further typing and cleanup work. Signed-off-by: John Snow --- scripts/qapi/mypy.ini | 1 - 1 file changed, 1 deletion(-) diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini index 74fc6c82153..04bd5db5278 100644 --- a/scripts/qapi/mypy.ini +++ b/scripts/qapi/mypy.ini @@ -1,6 +1,5 @@ [mypy] strict = True -strict_optional = False disallow_untyped_calls = False python_version = 3.6 -- 2.26.2
[PATCH v3 06/13] qapi/introspect.py: replace 'extra' dict with 'comment' argument
This is only used to pass in a dictionary with a comment already set, so
skip the runaround and just accept the comment.
This works because _tree_to_qlit() treats 'if': None; 'comment': None
exactly like absent 'if'; 'comment'.
Signed-off-by: John Snow
---
scripts/qapi/introspect.py | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index d3fbf694ad2..0aa3b77109f 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -10,6 +10,8 @@
See the COPYING file in the top-level directory.
"""
+from typing import Optional
+
from .common import (
c_name,
gen_endif,
@@ -24,11 +26,11 @@
)
-def _make_tree(obj, ifcond, extra=None):
-if extra is None:
-extra = {}
-if ifcond:
-extra['if'] = ifcond
+def _make_tree(obj, ifcond, comment=None):
+extra = {
+'if': ifcond,
+'comment': comment,
+}
return (obj, extra)
@@ -174,18 +176,18 @@ def _gen_features(features):
return [_make_tree(f.name, f.ifcond) for f in features]
def _gen_tree(self, name, mtype, obj, ifcond, features):
-extra = None
+comment: Optional[str] = None
if mtype not in ('command', 'event', 'builtin', 'array'):
if not self._unmask:
# Output a comment to make it easy to map masked names
# back to the source when reading the generated output.
-extra = {'comment': '"%s" = %s' % (self._name(name), name)}
+comment = f'"{self._name(name)}" = {name}'
name = self._name(name)
obj['name'] = name
obj['meta-type'] = mtype
if features:
obj['features'] = self._gen_features(features)
-self._trees.append(_make_tree(obj, ifcond, extra))
+self._trees.append(_make_tree(obj, ifcond, comment))
def _gen_member(self, member):
obj = {'name': member.name, 'type': self._use_type(member.type)}
--
2.26.2
[PATCH v3 03/13] qapi/introspect.py: add _gen_features helper
_make_tree might receive a dict (a SchemaInfo object) or some other type
(usually, a string) for its obj parameter. Adding features information
should arguably be performed by the caller at such a time when we know
the type of the object and don't have to re-interrogate it.
Signed-off-by: John Snow
---
scripts/qapi/introspect.py | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 3295a15c98e..4749f65ea3c 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -24,15 +24,11 @@
)
-def _make_tree(obj, ifcond, features, extra=None):
+def _make_tree(obj, ifcond, extra=None):
if extra is None:
extra = {}
if ifcond:
extra['if'] = ifcond
-if features:
-obj['features'] = [
-_make_tree(f.name, f.ifcond, None) for f in features
-]
if extra:
return (obj, extra)
return obj
@@ -169,6 +165,10 @@ def _use_type(self, typ):
return '[' + self._use_type(typ.element_type) + ']'
return self._name(typ.name)
+@staticmethod
+def _gen_features(features):
+return [_make_tree(f.name, f.ifcond) for f in features]
+
def _gen_tree(self, name, mtype, obj, ifcond, features):
extra = None
if mtype not in ('command', 'event', 'builtin', 'array'):
@@ -179,13 +179,17 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
name = self._name(name)
obj['name'] = name
obj['meta-type'] = mtype
-self._trees.append(_make_tree(obj, ifcond, features, extra))
+if features:
+obj['features'] = self._gen_features(features)
+self._trees.append(_make_tree(obj, ifcond, extra))
def _gen_member(self, member):
obj = {'name': member.name, 'type': self._use_type(member.type)}
if member.optional:
obj['default'] = None
-return _make_tree(obj, member.ifcond, member.features)
+if member.features:
+obj['features'] = self._gen_features(member.features)
+return _make_tree(obj, member.ifcond)
def _gen_variants(self, tag_name, variants):
return {'tag': tag_name,
@@ -193,7 +197,7 @@ def _gen_variants(self, tag_name, variants):
def _gen_variant(self, variant):
obj = {'case': variant.name, 'type': self._use_type(variant.type)}
-return _make_tree(obj, variant.ifcond, None)
+return _make_tree(obj, variant.ifcond)
def visit_builtin_type(self, name, info, json_type):
self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
--
2.26.2
[PATCH v3 04/13] qapi/introspect.py: guard against ifcond/comment misuse
_tree_to_qlit is called recursively on dict values alone; at such a
point in generating output it is too late to apply an ifcond. Similarly,
comments do not necessarily have a "tidy" place they can be printed in
such a circumstance.
Forbid this usage.
Signed-off-by: John Snow
---
scripts/qapi/introspect.py | 6 ++
1 file changed, 6 insertions(+)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 4749f65ea3c..ccdf4f1c0d0 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -43,6 +43,12 @@ def indent(level):
ifobj, extra = obj
ifcond = extra.get('if')
comment = extra.get('comment')
+
+# NB: _tree_to_qlit is called recursively on the values of a key:value
+# pair; those values can't be decorated with comments or conditionals.
+msg = "dict values cannot have attached comments or if-conditionals."
+assert not suppress_first_indent, msg
+
ret = ''
if comment:
ret += indent(level) + '/* %s */\n' % comment
--
2.26.2
[PATCH v3 02/13] qapi/introspect.py: use _make_tree for features nodes
At present, we open-code this in _make_tree itself; but if the structure
of the tree changes, this is brittle. Use an explicit recursive call to
_make_tree when appropriate to help keep the interior node typing
consistent.
Signed-off-by: John Snow
---
scripts/qapi/introspect.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 43ab4be1f77..3295a15c98e 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -30,7 +30,9 @@ def _make_tree(obj, ifcond, features, extra=None):
if ifcond:
extra['if'] = ifcond
if features:
-obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
+obj['features'] = [
+_make_tree(f.name, f.ifcond, None) for f in features
+]
if extra:
return (obj, extra)
return obj
--
2.26.2
[PATCH v3 11/13] qapi/introspect.py: add type hint annotations
Signed-off-by: John Snow
---
scripts/qapi/introspect.py | 114 ++---
scripts/qapi/mypy.ini | 5 --
scripts/qapi/schema.py | 2 +-
3 files changed, 81 insertions(+), 40 deletions(-)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 60ec326d2c7..590898baf93 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -30,10 +30,19 @@
)
from .gen import QAPISchemaMonolithicCVisitor
from .schema import (
+QAPISchema,
QAPISchemaArrayType,
QAPISchemaBuiltinType,
+QAPISchemaEntity,
+QAPISchemaEnumMember,
+QAPISchemaFeature,
+QAPISchemaObjectType,
+QAPISchemaObjectTypeMember,
QAPISchemaType,
+QAPISchemaVariant,
+QAPISchemaVariants,
)
+from .source import QAPISourceInfo
# This module constructs a tree data structure that is used to
@@ -57,6 +66,8 @@
_value = Union[_scalar, _nonscalar]
TreeValue = Union[_value, 'Annotated[_value]']
+# This is a (strict) alias for an arbitrary object non-scalar, as above:
+_DObject = Dict[str, object]
_NodeT = TypeVar('_NodeT', bound=TreeValue)
@@ -76,9 +87,11 @@ def __init__(self, value: _NodeT, ifcond: Iterable[str],
self.ifcond: Tuple[str, ...] = tuple(ifcond)
-def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
+def _tree_to_qlit(obj: TreeValue,
+ level: int = 0,
+ suppress_first_indent: bool = False) -> str:
-def indent(level):
+def indent(level: int) -> str:
return level * 4 * ' '
if isinstance(obj, Annotated):
@@ -135,21 +148,21 @@ def indent(level):
return ret
-def to_c_string(string):
+def to_c_string(string: str) -> str:
return '"' + string.replace('\\', r'\\').replace('"', r'\"') + '"'
class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
-def __init__(self, prefix, unmask):
+def __init__(self, prefix: str, unmask: bool):
super().__init__(
prefix, 'qapi-introspect',
' * QAPI/QMP schema introspection', __doc__)
self._unmask = unmask
-self._schema = None
-self._trees = []
-self._used_types = []
-self._name_map = {}
+self._schema: Optional[QAPISchema] = None
+self._trees: List[Annotated[_DObject]] = []
+self._used_types: List[QAPISchemaType] = []
+self._name_map: Dict[str, str] = {}
self._genc.add(mcgen('''
#include "qemu/osdep.h"
#include "%(prefix)sqapi-introspect.h"
@@ -157,10 +170,10 @@ def __init__(self, prefix, unmask):
''',
prefix=prefix))
-def visit_begin(self, schema):
+def visit_begin(self, schema: QAPISchema) -> None:
self._schema = schema
-def visit_end(self):
+def visit_end(self) -> None:
# visit the types that are actually used
for typ in self._used_types:
typ.visit(self)
@@ -182,18 +195,18 @@ def visit_end(self):
self._used_types = []
self._name_map = {}
-def visit_needed(self, entity):
+def visit_needed(self, entity: QAPISchemaEntity) -> bool:
# Ignore types on first pass; visit_end() will pick up used types
return not isinstance(entity, QAPISchemaType)
-def _name(self, name):
+def _name(self, name: str) -> str:
if self._unmask:
return name
if name not in self._name_map:
self._name_map[name] = '%d' % len(self._name_map)
return self._name_map[name]
-def _use_type(self, typ):
+def _use_type(self, typ: QAPISchemaType) -> str:
assert self._schema is not None
# Map the various integer types to plain int
@@ -215,10 +228,13 @@ def _use_type(self, typ):
return self._name(typ.name)
@staticmethod
-def _gen_features(features):
+def _gen_features(features: List[QAPISchemaFeature]
+ ) -> List[Annotated[str]]:
return [Annotated(f.name, f.ifcond) for f in features]
-def _gen_tree(self, name, mtype, obj, ifcond, features):
+def _gen_tree(self, name: str, mtype: str, obj: _DObject,
+ ifcond: List[str],
+ features: Optional[List[QAPISchemaFeature]]) -> None:
comment: Optional[str] = None
if mtype not in ('command', 'event', 'builtin', 'array'):
if not self._unmask:
@@ -232,47 +248,67 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
obj['features'] = self._gen_features(features)
self._trees.append(Annotated(obj, ifcond, comment))
-def _gen_member(self, member):
-obj = {'name': member.name, 'type': self._use_type(member.type)}
+def _gen_member(self, member: QAPISchemaObjectTypeMember
+) -> Annotated[_DObject]:
+obj: _DObject = {
+'name': member.name,
+'type': self._use_type(member.type)
+}
if member.optional:
[PATCH v3 00/13] qapi: static typing conversion, pt2
Hi, this series adds static type hints to the QAPI module. This is part two, and covers introspect.py. Part 2: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt2 Everything: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6 - Requires Python 3.6+ - Requires mypy 0.770 or newer (for type analysis only) - Requires pylint 2.6.0 or newer (for lint checking only) Type hints are added in patches that add *only* type hints and change no other behavior. Any necessary changes to behavior to accommodate typing are split out into their own tiny patches. Every commit should pass with: - flake8 qapi/ - pylint --rcfile=qapi/pylintrc qapi/ - mypy --config-file=qapi/mypy.ini qapi/ V3: - Dropped all the R-Bs again... - Re-re-ordered to put type annotations last again. - Rebased on top of "pt1.5". - Ensured compliance with strict-optional typing. - Forgive me if I missed a specific critique; I probably just lost it in the shuffle. V2: - Dropped all R-B from previous series; enough has changed. - pt2 is now introspect.py, expr.py is pushed to pt3. - Reworked again to have less confusing (?) type names - Added an assertion to prevent future accidental breakage John Snow (13): qapi/introspect.py: assert schema is not None qapi/introspect.py: use _make_tree for features nodes qapi/introspect.py: add _gen_features helper qapi/introspect.py: guard against ifcond/comment misuse qapi/introspect.py: Unify return type of _make_tree() qapi/introspect.py: replace 'extra' dict with 'comment' argument qapi/introspect.py: Introduce preliminary tree typing qapi/introspect.py: create a typed 'Annotated' data strutcure qapi/introspect.py: improve _tree_to_qlit error message qapi/introspect.py: improve readability of _tree_to_qlit qapi/introspect.py: add type hint annotations qapi/instrospect.py: add introspect.json dummy types qapi/introspect.py: Add docstring to _tree_to_qlit scripts/qapi/introspect.py | 309 ++--- scripts/qapi/mypy.ini | 5 - scripts/qapi/schema.py | 2 +- 3 files changed, 219 insertions(+), 97 deletions(-) -- 2.26.2
[PATCH v3 01/13] qapi/introspect.py: assert schema is not None
The introspect visitor is stateful, but expects that it will have a
schema to refer to. Add assertions that state this.
Signed-off-by: John Snow
---
scripts/qapi/introspect.py | 5 +
1 file changed, 5 insertions(+)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index fafec94e022..43ab4be1f77 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -147,6 +147,8 @@ def _name(self, name):
return self._name_map[name]
def _use_type(self, typ):
+assert self._schema is not None
+
# Map the various integer types to plain int
if typ.json_type() == 'int':
typ = self._schema.lookup_type('int')
@@ -225,6 +227,8 @@ def visit_alternate_type(self, name, info, ifcond,
features, variants):
def visit_command(self, name, info, ifcond, features,
arg_type, ret_type, gen, success_response, boxed,
allow_oob, allow_preconfig, coroutine):
+assert self._schema is not None
+
arg_type = arg_type or self._schema.the_empty_object_type
ret_type = ret_type or self._schema.the_empty_object_type
obj = {'arg-type': self._use_type(arg_type),
@@ -234,6 +238,7 @@ def visit_command(self, name, info, ifcond, features,
self._gen_tree(name, 'command', obj, ifcond, features)
def visit_event(self, name, info, ifcond, features, arg_type, boxed):
+assert self._schema is not None
arg_type = arg_type or self._schema.the_empty_object_type
self._gen_tree(name, 'event', {'arg-type': self._use_type(arg_type)},
ifcond, features)
--
2.26.2
[PATCH v2 1/2] accel: kvm: Fix memory waste under mismatch page size
When handle dirty log, we face qemu_real_host_page_size and TARGET_PAGE_SIZE. The first one is the granule of KVM dirty bitmap, and the second one is the granule of QEMU dirty bitmap. As qemu_real_host_page_size >= TARGET_PAGE_SIZE (kvm_init() enforced it), misuse TARGET_PAGE_SIZE to init kvmslot dirty_bmap may waste memory. For example, when qemu_real_host_page_size is 64K and TARGET_PAGE_SIZE is 4K, it wastes 93.75% (15/16) memory. Signed-off-by: Keqian Zhu Reviewed-by: Andrew Jones Reviewed-by: Peter Xu --- accel/kvm/kvm-all.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) --- v2 - Address Andrew's comment (qemu_real_host_page_size >= TARGET_PAGE_SIZE is a rule). - Add Andrew and Peter's R-b. diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 389eaace72..f6b16a8df8 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -620,8 +620,12 @@ static void kvm_memslot_init_dirty_bitmap(KVMSlot *mem) * too, in most cases). * So for now, let's align to 64 instead of HOST_LONG_BITS here, in * a hope that sizeof(long) won't become >8 any time soon. + * + * Note: the granule of kvm dirty log is qemu_real_host_page_size. + * And mem->memory_size is aligned to it (otherwise this mem can't + * be registered to KVM). */ -hwaddr bitmap_size = ALIGN(((mem->memory_size) >> TARGET_PAGE_BITS), +hwaddr bitmap_size = ALIGN(mem->memory_size / qemu_real_host_page_size, /*HOST_LONG_BITS*/ 64) / 8; mem->dirty_bmap = g_malloc0(bitmap_size); } -- 2.23.0
[PATCH v2 2/2] accel: kvm: Add aligment assert for kvm_log_clear_one_slot
The parameters start and size are transfered from QEMU memory
emulation layer. It can promise that they are TARGET_PAGE_SIZE
aligned. However, KVM needs they are qemu_real_page_size aligned.
Though no caller breaks this aligned requirement currently, we'd
better add an explicit assert to avoid future breaking.
Signed-off-by: Keqian Zhu
---
accel/kvm/kvm-all.c | 7 +++
1 file changed, 7 insertions(+)
---
v2
- Address Andrew's commment (Use assert instead of return err).
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index f6b16a8df8..73b195cc41 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -692,6 +692,10 @@ out:
#define KVM_CLEAR_LOG_ALIGN (qemu_real_host_page_size << KVM_CLEAR_LOG_SHIFT)
#define KVM_CLEAR_LOG_MASK (-KVM_CLEAR_LOG_ALIGN)
+/*
+ * As the granule of kvm dirty log is qemu_real_host_page_size,
+ * @start and @size are expected and restricted to align to it.
+ */
static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id, uint64_t start,
uint64_t size)
{
@@ -701,6 +705,9 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int as_id,
uint64_t start,
unsigned long *bmap_clear = NULL, psize = qemu_real_host_page_size;
int ret;
+/* Make sure start and size are qemu_real_host_page_size aligned */
+assert(QEMU_IS_ALIGNED(start | size, psize));
+
/*
* We need to extend either the start or the size or both to
* satisfy the KVM interface requirement. Firstly, do the start
--
2.23.0
[PATCH v2 0/2] accel: kvm: Some bugfixes for kvm dirty log
Hi all, This series fixes memory waste and adds alignment check for unmatched qemu_real_host_page_size and TARGET_PAGE_SIZE. Thanks. Keqian Zhu (2): accel: kvm: Fix memory waste under mismatch page size accel: kvm: Add aligment assert for kvm_log_clear_one_slot accel/kvm/kvm-all.c | 13 - 1 file changed, 12 insertions(+), 1 deletion(-) -- 2.23.0
Re: [PATCH v5 0/2] MTE support for KVM guest
On Wed, 16 Dec 2020 at 18:23, Steven Price wrote: > > On 16/12/2020 07:31, Haibo Xu wrote: > [...] > > Hi Steve, > > Hi Haibo > > > I have finished verifying the POC on a FVP setup, and the MTE test case can > > be migrated from one VM to another successfully. Since the test case is very > > simple which just maps one page with MTE enabled and does some memory > > access, so I can't say it's OK for other cases. > > That's great progress. > > > > > BTW, I noticed that you have sent out patch set v6 which mentions that > > mapping > > all the guest memory with PROT_MTE was not feasible. So what's the plan for > > the > > next step? Will new KVM APIs which can facilitate the tag store and recover > > be > > available? > > I'm currently rebasing on top of the KASAN MTE patch series. My plan for > now is to switch back to not requiring the VMM to supply PROT_MTE (so > KVM 'upgrades' the pages as necessary) and I'll add an RFC patch on the > end of the series to add an KVM API for doing bulk read/write of tags. > That way the VMM can map guest memory without PROT_MTE (so device 'DMA' > accesses will be unchecked), and use the new API for migration. > Great! Will have a try with the new API in my POC! > Thanks, > > Steve
Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations
On 12/16/20 2:51 AM, Markus Armbruster wrote: Is it too late to delay the introduction of type hints until after the data structure cleanups? If yes, I have to spend more time replying below. No, I re-ordered the series again to delay typing until the end, or close to it. Though I abandoned plans to slacken List[...] inputs to Iterable[...] or Sequence[...] like I had mentioned; I think it could still be done, but it's fine to just use List[...] for the inputs for now. We can worry about optimizing type flexibility later, I think. Let's just get the dog hunting at all first. --js
Re: [PATCH v2 09/11] qapi/introspect.py: create a typed 'Annotated' data strutcure
On 12/16/20 2:08 AM, Markus Armbruster wrote: We all have our phobias. I find "isinstance(x, extremely_common_stdlib_type)" to be extremely fragile and likely to frustrate. You're applying programming-in-the-large reasoning to a programming-in-the-small case. "Surely, they won't use my proof of concept code in production!" Ah, alas, ... Say you're writing a piece of code you expect to be used in contexts you prudently refuse to predict. The code deals with a bunch of basic Python types. Reserving another basic Python type for internal use may well be unwise then, because it can make your code break confusingly when this other type appears in input. Which it shouldn't, but making your reusable code harder to misuse, and misuses easier to diagnose are laudable goals. This is not such a piece of code. All the users it will ever have are in the same file of 200-something LOC. I'm just saying that this type of code has bitten me in the ass before. You're right that it's not likely to bite someone explicitly here, but that's indeed why it came in the "Also, ..." section. I've reworked the commit message a bit by now, but I suspect you'll still want to take the red marker to it a bit. --js
Re: [PATCH] tcg: Add tcg_gen_bswap_tl alias
On Thu, Dec 17, 2020 at 2:05 AM Richard Henderson < richard.hender...@linaro.org> wrote: > The alias is intended to indicate that the bswap is for the > entire target_long. This should avoid ifdefs on some targets. > > Signed-off-by: Richard Henderson > --- > include/tcg/tcg-op.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/tcg/tcg-op.h b/include/tcg/tcg-op.h > index 5abf17fecc..5b3bdacc39 100644 > --- a/include/tcg/tcg-op.h > +++ b/include/tcg/tcg-op.h > @@ -1085,6 +1085,7 @@ void tcg_gen_stl_vec(TCGv_vec r, TCGv_ptr base, > TCGArg offset, TCGType t); > #define tcg_gen_bswap16_tl tcg_gen_bswap16_i64 > #define tcg_gen_bswap32_tl tcg_gen_bswap32_i64 > #define tcg_gen_bswap64_tl tcg_gen_bswap64_i64 > +#define tcg_gen_bswap_tl tcg_gen_bswap64_i64 > #define tcg_gen_concat_tl_i64 tcg_gen_concat32_i64 > #define tcg_gen_extr_i64_tl tcg_gen_extr32_i64 > #define tcg_gen_andc_tl tcg_gen_andc_i64 > @@ -1197,6 +1198,7 @@ void tcg_gen_stl_vec(TCGv_vec r, TCGv_ptr base, > TCGArg offset, TCGType t); > #define tcg_gen_ext32s_tl tcg_gen_mov_i32 > #define tcg_gen_bswap16_tl tcg_gen_bswap16_i32 > #define tcg_gen_bswap32_tl tcg_gen_bswap32_i32 > +#define tcg_gen_bswap_tl tcg_gen_bswap32_i32 > #define tcg_gen_concat_tl_i64 tcg_gen_concat_i32_i64 > #define tcg_gen_extr_i64_tl tcg_gen_extr_i64_i32 > #define tcg_gen_andc_tl tcg_gen_andc_i32 > -- > 2.25.1 > > Thanks, I'll apply this one to my RISC-V B-extension patchset. Reviewed-by: Frank Chang
Re: [PATCH 1/2] accel: kvm: Fix memory waste under mismatch page size
On 2020/12/17 4:48, Peter Xu wrote: > On Wed, Dec 16, 2020 at 04:21:17PM +0800, Keqian Zhu wrote: >> One more thing, we should consider whether @start and @size is psize aligned >> (my second >> patch). Do you agree to add assert on them directly? > > Yes I think the 2nd patch is okay, but please address Drew's comments. > > Returning -EINVAL is the same as abort() currently - it'll just abort() at > kvm_log_clear() instead. OK, I will send v2 soon. Thanks, Keqian > > Thanks, >
[PATCH v4 2/6] hw/timer: Refactor NPCM7XX Timer to use CLK clock
This patch makes NPCM7XX Timer to use a the timer clock generated by the
CLK module instead of the magic number TIMER_REF_HZ.
Reviewed-by: Havard Skinnemoen
Reviewed-by: Tyrone Ting
Signed-off-by: Hao Wu
---
hw/arm/npcm7xx.c | 5 +
hw/timer/npcm7xx_timer.c | 25 ++---
include/hw/misc/npcm7xx_clk.h| 6 --
include/hw/timer/npcm7xx_timer.h | 1 +
4 files changed, 20 insertions(+), 17 deletions(-)
diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
index 47e2b6fc40..fabfb1697b 100644
--- a/hw/arm/npcm7xx.c
+++ b/hw/arm/npcm7xx.c
@@ -22,6 +22,7 @@
#include "hw/char/serial.h"
#include "hw/loader.h"
#include "hw/misc/unimp.h"
+#include "hw/qdev-clock.h"
#include "hw/qdev-properties.h"
#include "qapi/error.h"
#include "qemu/units.h"
@@ -420,6 +421,10 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
int first_irq;
int j;
+/* Connect the timer clock. */
+qdev_connect_clock_in(DEVICE(>tim[i]), "clock", qdev_get_clock_out(
+DEVICE(>clk), "timer-clock"));
+
sysbus_realize(sbd, _abort);
sysbus_mmio_map(sbd, 0, npcm7xx_tim_addr[i]);
diff --git a/hw/timer/npcm7xx_timer.c b/hw/timer/npcm7xx_timer.c
index d24445bd6e..6e990d611a 100644
--- a/hw/timer/npcm7xx_timer.c
+++ b/hw/timer/npcm7xx_timer.c
@@ -17,8 +17,8 @@
#include "qemu/osdep.h"
#include "hw/irq.h"
+#include "hw/qdev-clock.h"
#include "hw/qdev-properties.h"
-#include "hw/misc/npcm7xx_clk.h"
#include "hw/timer/npcm7xx_timer.h"
#include "migration/vmstate.h"
#include "qemu/bitops.h"
@@ -130,7 +130,7 @@ static int64_t npcm7xx_timer_count_to_ns(NPCM7xxTimer *t,
uint32_t count)
{
int64_t ns = count;
-ns *= NANOSECONDS_PER_SECOND / NPCM7XX_TIMER_REF_HZ;
+ns *= clock_get_ns(t->ctrl->clock);
ns *= npcm7xx_tcsr_prescaler(t->tcsr);
return ns;
@@ -141,7 +141,7 @@ static uint32_t npcm7xx_timer_ns_to_count(NPCM7xxTimer *t,
int64_t ns)
{
int64_t count;
-count = ns / (NANOSECONDS_PER_SECOND / NPCM7XX_TIMER_REF_HZ);
+count = ns / clock_get_ns(t->ctrl->clock);
count /= npcm7xx_tcsr_prescaler(t->tcsr);
return count;
@@ -167,7 +167,7 @@ static void
npcm7xx_watchdog_timer_reset_cycles(NPCM7xxWatchdogTimer *t,
int64_t cycles)
{
uint32_t prescaler = npcm7xx_watchdog_timer_prescaler(t);
-int64_t ns = (NANOSECONDS_PER_SECOND / NPCM7XX_TIMER_REF_HZ) * cycles;
+int64_t ns = clock_get_ns(t->ctrl->clock) * cycles;
/*
* The reset function always clears the current timer. The caller of the
@@ -606,10 +606,11 @@ static void npcm7xx_timer_hold_reset(Object *obj)
qemu_irq_lower(s->watchdog_timer.irq);
}
-static void npcm7xx_timer_realize(DeviceState *dev, Error **errp)
+static void npcm7xx_timer_init(Object *obj)
{
-NPCM7xxTimerCtrlState *s = NPCM7XX_TIMER(dev);
-SysBusDevice *sbd = >parent;
+NPCM7xxTimerCtrlState *s = NPCM7XX_TIMER(obj);
+DeviceState *dev = DEVICE(obj);
+SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
int i;
NPCM7xxWatchdogTimer *w;
@@ -627,11 +628,12 @@ static void npcm7xx_timer_realize(DeviceState *dev, Error
**errp)
npcm7xx_watchdog_timer_expired, w);
sysbus_init_irq(sbd, >irq);
-memory_region_init_io(>iomem, OBJECT(s), _timer_ops, s,
+memory_region_init_io(>iomem, obj, _timer_ops, s,
TYPE_NPCM7XX_TIMER, 4 * KiB);
sysbus_init_mmio(sbd, >iomem);
qdev_init_gpio_out_named(dev, >reset_signal,
NPCM7XX_WATCHDOG_RESET_GPIO_OUT, 1);
+s->clock = qdev_init_clock_in(dev, "clock", NULL, NULL);
}
static const VMStateDescription vmstate_npcm7xx_base_timer = {
@@ -675,10 +677,11 @@ static const VMStateDescription
vmstate_npcm7xx_watchdog_timer = {
static const VMStateDescription vmstate_npcm7xx_timer_ctrl = {
.name = "npcm7xx-timer-ctrl",
-.version_id = 1,
-.minimum_version_id = 1,
+.version_id = 2,
+.minimum_version_id = 2,
.fields = (VMStateField[]) {
VMSTATE_UINT32(tisr, NPCM7xxTimerCtrlState),
+VMSTATE_CLOCK(clock, NPCM7xxTimerCtrlState),
VMSTATE_STRUCT_ARRAY(timer, NPCM7xxTimerCtrlState,
NPCM7XX_TIMERS_PER_CTRL, 0, vmstate_npcm7xx_timer,
NPCM7xxTimer),
@@ -697,7 +700,6 @@ static void npcm7xx_timer_class_init(ObjectClass *klass,
void *data)
QEMU_BUILD_BUG_ON(NPCM7XX_TIMER_REGS_END > NPCM7XX_TIMER_NR_REGS);
dc->desc = "NPCM7xx Timer Controller";
-dc->realize = npcm7xx_timer_realize;
dc->vmsd = _npcm7xx_timer_ctrl;
rc->phases.enter = npcm7xx_timer_enter_reset;
rc->phases.hold = npcm7xx_timer_hold_reset;
@@ -708,6 +710,7 @@ static const TypeInfo npcm7xx_timer_info = {
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(NPCM7xxTimerCtrlState),
.class_init = npcm7xx_timer_class_init,
+.instance_init = npcm7xx_timer_init,
[PATCH v4 5/6] hw/misc: Add QTest for NPCM7XX PWM Module
We add a qtest for the PWM in the previous patch. It proves it works as
expected.
Reviewed-by: Havard Skinnemoen
Reviewed-by: Tyrone Ting
Signed-off-by: Hao Wu
---
tests/qtest/meson.build| 1 +
tests/qtest/npcm7xx_pwm-test.c | 490 +
2 files changed, 491 insertions(+)
create mode 100644 tests/qtest/npcm7xx_pwm-test.c
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 955710d1c5..0b5467f084 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -136,6 +136,7 @@ qtests_sparc64 = \
qtests_npcm7xx = \
['npcm7xx_adc-test',
'npcm7xx_gpio-test',
+ 'npcm7xx_pwm-test',
'npcm7xx_rng-test',
'npcm7xx_timer-test',
'npcm7xx_watchdog_timer-test']
diff --git a/tests/qtest/npcm7xx_pwm-test.c b/tests/qtest/npcm7xx_pwm-test.c
new file mode 100644
index 00..33fbdf5f54
--- /dev/null
+++ b/tests/qtest/npcm7xx_pwm-test.c
@@ -0,0 +1,490 @@
+/*
+ * QTests for Nuvoton NPCM7xx PWM Modules.
+ *
+ * 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 "qemu/bitops.h"
+#include "libqos/libqtest.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qnum.h"
+
+#define REF_HZ 2500
+
+/* Register field definitions. */
+#define CH_EN BIT(0)
+#define CH_INV BIT(2)
+#define CH_MOD BIT(3)
+
+/* Registers shared between all PWMs in a module */
+#define PPR 0x00
+#define CSR 0x04
+#define PCR 0x08
+#define PIER0x3c
+#define PIIR0x40
+
+/* CLK module related */
+#define CLK_BA 0xf0801000
+#define CLKSEL 0x04
+#define CLKDIV1 0x08
+#define CLKDIV2 0x2c
+#define PLLCON0 0x0c
+#define PLLCON1 0x10
+#define PLL_INDV(rv)extract32((rv), 0, 6)
+#define PLL_FBDV(rv)extract32((rv), 16, 12)
+#define PLL_OTDV1(rv) extract32((rv), 8, 3)
+#define PLL_OTDV2(rv) extract32((rv), 13, 3)
+#define APB3CKDIV(rv) extract32((rv), 28, 2)
+#define CLK2CKDIV(rv) extract32((rv), 0, 1)
+#define CLK4CKDIV(rv) extract32((rv), 26, 2)
+#define CPUCKSEL(rv)extract32((rv), 0, 2)
+
+#define MAX_DUTY100
+
+typedef struct PWMModule {
+int irq;
+uint64_t base_addr;
+} PWMModule;
+
+typedef struct PWM {
+uint32_t cnr_offset;
+uint32_t cmr_offset;
+uint32_t pdr_offset;
+uint32_t pwdr_offset;
+} PWM;
+
+typedef struct TestData {
+const PWMModule *module;
+const PWM *pwm;
+} TestData;
+
+static const PWMModule pwm_module_list[] = {
+{
+.irq= 93,
+.base_addr = 0xf0103000
+},
+{
+.irq= 94,
+.base_addr = 0xf0104000
+}
+};
+
+static const PWM pwm_list[] = {
+{
+.cnr_offset = 0x0c,
+.cmr_offset = 0x10,
+.pdr_offset = 0x14,
+.pwdr_offset= 0x44,
+},
+{
+.cnr_offset = 0x18,
+.cmr_offset = 0x1c,
+.pdr_offset = 0x20,
+.pwdr_offset= 0x48,
+},
+{
+.cnr_offset = 0x24,
+.cmr_offset = 0x28,
+.pdr_offset = 0x2c,
+.pwdr_offset= 0x4c,
+},
+{
+.cnr_offset = 0x30,
+.cmr_offset = 0x34,
+.pdr_offset = 0x38,
+.pwdr_offset= 0x50,
+},
+};
+
+static const int ppr_base[] = { 0, 0, 8, 8 };
+static const int csr_base[] = { 0, 4, 8, 12 };
+static const int pcr_base[] = { 0, 8, 12, 16 };
+
+static const uint32_t ppr_list[] = {
+0,
+1,
+10,
+100,
+255, /* Max possible value. */
+};
+
+static const uint32_t csr_list[] = {
+0,
+1,
+2,
+3,
+4, /* Max possible value. */
+};
+
+static const uint32_t cnr_list[] = {
+0,
+1,
+50,
+100,
+150,
+200,
+1000,
+1,
+65535, /* Max possible value. */
+};
+
+static const uint32_t cmr_list[] = {
+0,
+1,
+10,
+50,
+100,
+150,
+200,
+1000,
+1,
+65535, /* Max possible value. */
+};
+
+/* Returns the index of the PWM module. */
+static int pwm_module_index(const PWMModule *module)
+{
+ptrdiff_t diff = module - pwm_module_list;
+
+g_assert_true(diff >= 0 && diff < ARRAY_SIZE(pwm_module_list));
+
+return diff;
+}
+
+/* Returns the index of the PWM entry. */
+static int pwm_index(const PWM *pwm)
+{
+ptrdiff_t diff = pwm - pwm_list;
+
+g_assert_true(diff >= 0 && diff < ARRAY_SIZE(pwm_list));
+
+return diff;
+}
+
+static
[PATCH v4 1/6] hw/misc: Add clock converter in NPCM7XX CLK module
This patch allows NPCM7XX CLK module to compute clocks that are used by
other NPCM7XX modules.
Add a new struct NPCM7xxClockConverterState which represents a
single converter. Each clock converter in CLK module represents one
converter in NPCM7XX CLK Module(PLL, SEL or Divider). Each converter
takes one or more input clocks and converts them into one output clock.
They form a clock hierarchy in the CLK module and are responsible for
outputing clocks for various other modules in an NPCM7XX SoC.
Each converter has a function pointer called "convert" which represents
the unique logic for that converter.
The clock contains two initialization information: ConverterInitInfo and
ConverterConnectionInfo. They represent the vertices and edges in the
clock diagram respectively.
Reviewed-by: Havard Skinnemoen
Reviewed-by: Tyrone Ting
Signed-off-by: Hao Wu
---
hw/misc/npcm7xx_clk.c | 795 +-
include/hw/misc/npcm7xx_clk.h | 140 +-
2 files changed, 927 insertions(+), 8 deletions(-)
diff --git a/hw/misc/npcm7xx_clk.c b/hw/misc/npcm7xx_clk.c
index 6732437fe2..48bc9bdda5 100644
--- a/hw/misc/npcm7xx_clk.c
+++ b/hw/misc/npcm7xx_clk.c
@@ -18,6 +18,7 @@
#include "hw/misc/npcm7xx_clk.h"
#include "hw/timer/npcm7xx_timer.h"
+#include "hw/qdev-clock.h"
#include "migration/vmstate.h"
#include "qemu/error-report.h"
#include "qemu/log.h"
@@ -27,9 +28,22 @@
#include "trace.h"
#include "sysemu/watchdog.h"
+/*
+ * The reference clock hz, and the SECCNT and CNTR25M registers in this module,
+ * is always 25 MHz.
+ */
+#define NPCM7XX_CLOCK_REF_HZ(2500)
+
+/* Register Field Definitions */
+#define NPCM7XX_CLK_WDRCR_CA9C BIT(0) /* Cortex A9 Cores */
+
#define PLLCON_LOKI BIT(31)
#define PLLCON_LOKS BIT(30)
#define PLLCON_PWDENBIT(12)
+#define PLLCON_FBDV(con) extract32((con), 16, 12)
+#define PLLCON_OTDV2(con) extract32((con), 13, 3)
+#define PLLCON_OTDV1(con) extract32((con), 8, 3)
+#define PLLCON_INDV(con) extract32((con), 0, 6)
enum NPCM7xxCLKRegisters {
NPCM7XX_CLK_CLKEN1,
@@ -89,12 +103,609 @@ static const uint32_t
cold_reset_values[NPCM7XX_CLK_NR_REGS] = {
[NPCM7XX_CLK_AHBCKFI] = 0x00c8,
};
-/* Register Field Definitions */
-#define NPCM7XX_CLK_WDRCR_CA9C BIT(0) /* Cortex A9 Cores */
-
/* The number of watchdogs that can trigger a reset. */
#define NPCM7XX_NR_WATCHDOGS(3)
+/* Clock converter functions */
+
+#define TYPE_NPCM7XX_CLOCK_PLL "npcm7xx-clock-pll"
+#define NPCM7XX_CLOCK_PLL(obj) OBJECT_CHECK(NPCM7xxClockPLLState, \
+(obj), TYPE_NPCM7XX_CLOCK_PLL)
+#define TYPE_NPCM7XX_CLOCK_SEL "npcm7xx-clock-sel"
+#define NPCM7XX_CLOCK_SEL(obj) OBJECT_CHECK(NPCM7xxClockSELState, \
+(obj), TYPE_NPCM7XX_CLOCK_SEL)
+#define TYPE_NPCM7XX_CLOCK_DIVIDER "npcm7xx-clock-divider"
+#define NPCM7XX_CLOCK_DIVIDER(obj) OBJECT_CHECK(NPCM7xxClockDividerState, \
+(obj), TYPE_NPCM7XX_CLOCK_DIVIDER)
+
+static void npcm7xx_clk_update_pll(void *opaque)
+{
+NPCM7xxClockPLLState *s = opaque;
+uint32_t con = s->clk->regs[s->reg];
+uint64_t freq;
+
+/* The PLL is grounded if it is not locked yet. */
+if (con & PLLCON_LOKI) {
+freq = clock_get_hz(s->clock_in);
+freq *= PLLCON_FBDV(con);
+freq /= PLLCON_INDV(con) * PLLCON_OTDV1(con) * PLLCON_OTDV2(con);
+} else {
+freq = 0;
+}
+
+clock_update_hz(s->clock_out, freq);
+}
+
+static void npcm7xx_clk_update_sel(void *opaque)
+{
+NPCM7xxClockSELState *s = opaque;
+uint32_t index = extract32(s->clk->regs[NPCM7XX_CLK_CLKSEL], s->offset,
+s->len);
+
+if (index >= s->input_size) {
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: SEL index: %u out of range\n",
+ __func__, index);
+index = 0;
+}
+clock_update_hz(s->clock_out, clock_get_hz(s->clock_in[index]));
+}
+
+static void npcm7xx_clk_update_divider(void *opaque)
+{
+NPCM7xxClockDividerState *s = opaque;
+uint32_t freq;
+
+freq = s->divide(s);
+clock_update_hz(s->clock_out, freq);
+}
+
+static uint32_t divide_by_constant(NPCM7xxClockDividerState *s)
+{
+return clock_get_hz(s->clock_in) / s->divisor;
+}
+
+static uint32_t divide_by_reg_divisor(NPCM7xxClockDividerState *s)
+{
+return clock_get_hz(s->clock_in) /
+(extract32(s->clk->regs[s->reg], s->offset, s->len) + 1);
+}
+
+static uint32_t divide_by_reg_divisor_times_2(NPCM7xxClockDividerState *s)
+{
+return divide_by_reg_divisor(s) / 2;
+}
+
+static uint32_t shift_by_reg_divisor(NPCM7xxClockDividerState *s)
+{
+return clock_get_hz(s->clock_in) >>
+extract32(s->clk->regs[s->reg], s->offset, s->len);
+}
+
+static NPCM7xxClockPLL find_pll_by_reg(enum NPCM7xxCLKRegisters reg)
+{
+switch (reg) {
+case NPCM7XX_CLK_PLLCON0:
+return NPCM7XX_CLOCK_PLL0;
+case NPCM7XX_CLK_PLLCON1:
+return NPCM7XX_CLOCK_PLL1;
+case NPCM7XX_CLK_PLLCON2:
+return
[PATCH v4 6/6] hw/*: Use type casting for SysBusDevice in NPCM7XX
A device shouldn't access its parent object which is QOM internal.
Instead it should use type cast for this purporse. This patch fixes this
issue for all NPCM7XX Devices.
Signed-off-by: Hao Wu
---
hw/arm/npcm7xx_boards.c | 2 +-
hw/mem/npcm7xx_mc.c | 2 +-
hw/misc/npcm7xx_clk.c | 2 +-
hw/misc/npcm7xx_gcr.c | 2 +-
hw/misc/npcm7xx_rng.c | 2 +-
hw/nvram/npcm7xx_otp.c | 2 +-
hw/ssi/npcm7xx_fiu.c| 2 +-
7 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
index 306260fa67..3fdd5cab01 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -82,7 +82,7 @@ static NPCM7xxState *npcm7xx_create_soc(MachineState *machine,
uint32_t hw_straps)
{
NPCM7xxMachineClass *nmc = NPCM7XX_MACHINE_GET_CLASS(machine);
-MachineClass *mc = >parent;
+MachineClass *mc = MACHINE_CLASS(nmc);
Object *obj;
if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) {
diff --git a/hw/mem/npcm7xx_mc.c b/hw/mem/npcm7xx_mc.c
index 0435d06ab4..abc5af5620 100644
--- a/hw/mem/npcm7xx_mc.c
+++ b/hw/mem/npcm7xx_mc.c
@@ -62,7 +62,7 @@ static void npcm7xx_mc_realize(DeviceState *dev, Error **errp)
memory_region_init_io(>mmio, OBJECT(s), _mc_ops, s, "regs",
NPCM7XX_MC_REGS_SIZE);
-sysbus_init_mmio(>parent, >mmio);
+sysbus_init_mmio(SYS_BUS_DEVICE(s), >mmio);
}
static void npcm7xx_mc_class_init(ObjectClass *klass, void *data)
diff --git a/hw/misc/npcm7xx_clk.c b/hw/misc/npcm7xx_clk.c
index 48bc9bdda5..0bcae9ce95 100644
--- a/hw/misc/npcm7xx_clk.c
+++ b/hw/misc/npcm7xx_clk.c
@@ -913,7 +913,7 @@ static void npcm7xx_clk_init(Object *obj)
memory_region_init_io(>iomem, obj, _clk_ops, s,
TYPE_NPCM7XX_CLK, 4 * KiB);
-sysbus_init_mmio(>parent, >iomem);
+sysbus_init_mmio(SYS_BUS_DEVICE(s), >iomem);
}
static int npcm7xx_clk_post_load(void *opaque, int version_id)
diff --git a/hw/misc/npcm7xx_gcr.c b/hw/misc/npcm7xx_gcr.c
index 745f690809..eace9e1967 100644
--- a/hw/misc/npcm7xx_gcr.c
+++ b/hw/misc/npcm7xx_gcr.c
@@ -220,7 +220,7 @@ static void npcm7xx_gcr_init(Object *obj)
memory_region_init_io(>iomem, obj, _gcr_ops, s,
TYPE_NPCM7XX_GCR, 4 * KiB);
-sysbus_init_mmio(>parent, >iomem);
+sysbus_init_mmio(SYS_BUS_DEVICE(s), >iomem);
}
static const VMStateDescription vmstate_npcm7xx_gcr = {
diff --git a/hw/misc/npcm7xx_rng.c b/hw/misc/npcm7xx_rng.c
index f650f3401f..b01df7cdb2 100644
--- a/hw/misc/npcm7xx_rng.c
+++ b/hw/misc/npcm7xx_rng.c
@@ -143,7 +143,7 @@ static void npcm7xx_rng_init(Object *obj)
memory_region_init_io(>iomem, obj, _rng_ops, s, "regs",
NPCM7XX_RNG_REGS_SIZE);
-sysbus_init_mmio(>parent, >iomem);
+sysbus_init_mmio(SYS_BUS_DEVICE(s), >iomem);
}
static const VMStateDescription vmstate_npcm7xx_rng = {
diff --git a/hw/nvram/npcm7xx_otp.c b/hw/nvram/npcm7xx_otp.c
index b16ca530ba..c61f2fc1aa 100644
--- a/hw/nvram/npcm7xx_otp.c
+++ b/hw/nvram/npcm7xx_otp.c
@@ -371,7 +371,7 @@ static void npcm7xx_otp_realize(DeviceState *dev, Error
**errp)
{
NPCM7xxOTPClass *oc = NPCM7XX_OTP_GET_CLASS(dev);
NPCM7xxOTPState *s = NPCM7XX_OTP(dev);
-SysBusDevice *sbd = >parent;
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
memset(s->array, 0, sizeof(s->array));
diff --git a/hw/ssi/npcm7xx_fiu.c b/hw/ssi/npcm7xx_fiu.c
index 5040132b07..4eedb2927e 100644
--- a/hw/ssi/npcm7xx_fiu.c
+++ b/hw/ssi/npcm7xx_fiu.c
@@ -498,7 +498,7 @@ static void npcm7xx_fiu_hold_reset(Object *obj)
static void npcm7xx_fiu_realize(DeviceState *dev, Error **errp)
{
NPCM7xxFIUState *s = NPCM7XX_FIU(dev);
-SysBusDevice *sbd = >parent;
+SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
int i;
if (s->cs_count <= 0) {
--
2.29.2.684.gfbc64c5ab5-goog
[PATCH v4 4/6] hw/misc: Add a PWM module for NPCM7XX
The PWM module is part of NPCM7XX module. Each NPCM7XX module has two
identical PWM modules. Each module contains 4 PWM entries. Each PWM has
two outputs: frequency and duty_cycle. Both are computed using inputs
from software side.
This module does not model detail pulse signals since it is expensive.
It also does not model interrupts and watchdogs that are dependant on
the detail models. The interfaces for these are left in the module so
that anyone in need for these functionalities can implement on their
own.
The user can read the duty cycle and frequency using qom-get command.
Reviewed-by: Havard Skinnemoen
Reviewed-by: Tyrone Ting
Signed-off-by: Hao Wu
---
docs/system/arm/nuvoton.rst | 2 +-
hw/arm/npcm7xx.c | 26 +-
hw/misc/meson.build | 1 +
hw/misc/npcm7xx_pwm.c | 559 ++
hw/misc/trace-events | 6 +
include/hw/arm/npcm7xx.h | 2 +
include/hw/misc/npcm7xx_pwm.h | 106 +++
7 files changed, 699 insertions(+), 3 deletions(-)
create mode 100644 hw/misc/npcm7xx_pwm.c
create mode 100644 include/hw/misc/npcm7xx_pwm.h
diff --git a/docs/system/arm/nuvoton.rst b/docs/system/arm/nuvoton.rst
index 35829f8d0b..a1786342e2 100644
--- a/docs/system/arm/nuvoton.rst
+++ b/docs/system/arm/nuvoton.rst
@@ -42,6 +42,7 @@ Supported devices
* USB host (USBH)
* GPIO controller
* Analog to Digital Converter (ADC)
+ * Pulse Width Modulation (PWM)
Missing devices
---
@@ -61,7 +62,6 @@ Missing devices
* Peripheral SPI controller (PSPI)
* SD/MMC host
* PECI interface
- * Pulse Width Modulation (PWM)
* Tachometer
* PCI and PCIe root complex and bridges
* VDM and MCTP support
diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
index b22a8c966d..72040d4079 100644
--- a/hw/arm/npcm7xx.c
+++ b/hw/arm/npcm7xx.c
@@ -102,6 +102,8 @@ enum NPCM7xxInterrupt {
NPCM7XX_WDG2_IRQ, /* Timer Module 2 Watchdog */
NPCM7XX_EHCI_IRQ= 61,
NPCM7XX_OHCI_IRQ= 62,
+NPCM7XX_PWM0_IRQ= 93, /* PWM module 0 */
+NPCM7XX_PWM1_IRQ, /* PWM module 1 */
NPCM7XX_GPIO0_IRQ = 116,
NPCM7XX_GPIO1_IRQ,
NPCM7XX_GPIO2_IRQ,
@@ -144,6 +146,12 @@ static const hwaddr npcm7xx_fiu3_flash_addr[] = {
0xb800, /* CS3 */
};
+/* Register base address for each PWM Module */
+static const hwaddr npcm7xx_pwm_addr[] = {
+0xf0103000,
+0xf0104000,
+};
+
static const struct {
hwaddr regs_addr;
uint32_t unconnected_pins;
@@ -353,6 +361,10 @@ static void npcm7xx_init(Object *obj)
object_initialize_child(obj, npcm7xx_fiu[i].name, >fiu[i],
TYPE_NPCM7XX_FIU);
}
+
+for (i = 0; i < ARRAY_SIZE(s->pwm); i++) {
+object_initialize_child(obj, "pwm[*]", >pwm[i], TYPE_NPCM7XX_PWM);
+}
}
static void npcm7xx_realize(DeviceState *dev, Error **errp)
@@ -513,6 +525,18 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(>ohci), 0,
npcm7xx_irq(s, NPCM7XX_OHCI_IRQ));
+/* PWM Modules. Cannot fail. */
+QEMU_BUILD_BUG_ON(ARRAY_SIZE(npcm7xx_pwm_addr) != ARRAY_SIZE(s->pwm));
+for (i = 0; i < ARRAY_SIZE(s->pwm); i++) {
+SysBusDevice *sbd = SYS_BUS_DEVICE(>pwm[i]);
+
+qdev_connect_clock_in(DEVICE(>pwm[i]), "clock", qdev_get_clock_out(
+DEVICE(>clk), "apb3-clock"));
+sysbus_realize(sbd, _abort);
+sysbus_mmio_map(sbd, 0, npcm7xx_pwm_addr[i]);
+sysbus_connect_irq(sbd, i, npcm7xx_irq(s, NPCM7XX_PWM0_IRQ + i));
+}
+
/*
* Flash Interface Unit (FIU). Can fail if incorrect number of chip selects
* specified, but this is a programming error.
@@ -580,8 +604,6 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
create_unimplemented_device("npcm7xx.peci", 0xf010, 4 * KiB);
create_unimplemented_device("npcm7xx.siox[1]", 0xf0101000, 4 * KiB);
create_unimplemented_device("npcm7xx.siox[2]", 0xf0102000, 4 * KiB);
-create_unimplemented_device("npcm7xx.pwm[0]", 0xf0103000, 4 * KiB);
-create_unimplemented_device("npcm7xx.pwm[1]", 0xf0104000, 4 * KiB);
create_unimplemented_device("npcm7xx.mft[0]", 0xf018, 4 * KiB);
create_unimplemented_device("npcm7xx.mft[1]", 0xf0181000, 4 * KiB);
create_unimplemented_device("npcm7xx.mft[2]", 0xf0182000, 4 * KiB);
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index ce15ffceb9..607cd38a21 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -64,6 +64,7 @@ softmmu_ss.add(when: 'CONFIG_MAINSTONE', if_true:
files('mst_fpga.c'))
softmmu_ss.add(when: 'CONFIG_NPCM7XX', if_true: files(
'npcm7xx_clk.c',
'npcm7xx_gcr.c',
+ 'npcm7xx_pwm.c',
'npcm7xx_rng.c',
))
softmmu_ss.add(when: 'CONFIG_OMAP', if_true: files(
diff --git a/hw/misc/npcm7xx_pwm.c
[PATCH v4 3/6] hw/adc: Add an ADC module for NPCM7XX
The ADC is part of NPCM7XX Module. Its behavior is controled by the
ADC_CON register. It converts one of the eight analog inputs into a
digital input and stores it in the ADC_DATA register when enabled.
Users can alter input value by using qom-set QMP command.
Reviewed-by: Havard Skinnemoen
Reviewed-by: Tyrone Ting
Signed-off-by: Hao Wu
---
docs/system/arm/nuvoton.rst| 2 +-
hw/adc/meson.build | 1 +
hw/adc/npcm7xx_adc.c | 321 ++
hw/adc/trace-events| 5 +
hw/arm/npcm7xx.c | 24 +-
include/hw/adc/npcm7xx_adc.h | 72 ++
include/hw/arm/npcm7xx.h | 2 +
meson.build| 1 +
tests/qtest/meson.build| 3 +-
tests/qtest/npcm7xx_adc-test.c | 400 +
10 files changed, 828 insertions(+), 3 deletions(-)
create mode 100644 hw/adc/npcm7xx_adc.c
create mode 100644 hw/adc/trace-events
create mode 100644 include/hw/adc/npcm7xx_adc.h
create mode 100644 tests/qtest/npcm7xx_adc-test.c
diff --git a/docs/system/arm/nuvoton.rst b/docs/system/arm/nuvoton.rst
index b00d405d52..35829f8d0b 100644
--- a/docs/system/arm/nuvoton.rst
+++ b/docs/system/arm/nuvoton.rst
@@ -41,6 +41,7 @@ Supported devices
* Random Number Generator (RNG)
* USB host (USBH)
* GPIO controller
+ * Analog to Digital Converter (ADC)
Missing devices
---
@@ -58,7 +59,6 @@ Missing devices
* USB device (USBD)
* SMBus controller (SMBF)
* Peripheral SPI controller (PSPI)
- * Analog to Digital Converter (ADC)
* SD/MMC host
* PECI interface
* Pulse Width Modulation (PWM)
diff --git a/hw/adc/meson.build b/hw/adc/meson.build
index 0d62ae96ae..6ddee23813 100644
--- a/hw/adc/meson.build
+++ b/hw/adc/meson.build
@@ -1 +1,2 @@
softmmu_ss.add(when: 'CONFIG_STM32F2XX_ADC', if_true: files('stm32f2xx_adc.c'))
+softmmu_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx_adc.c'))
diff --git a/hw/adc/npcm7xx_adc.c b/hw/adc/npcm7xx_adc.c
new file mode 100644
index 00..f213b6a6df
--- /dev/null
+++ b/hw/adc/npcm7xx_adc.c
@@ -0,0 +1,321 @@
+/*
+ * Nuvoton NPCM7xx ADC Module
+ *
+ * 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 "hw/adc/npcm7xx_adc.h"
+#include "hw/qdev-clock.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qemu/timer.h"
+#include "qemu/units.h"
+#include "trace.h"
+
+/* 32-bit register indices. */
+enum NPCM7xxADCRegisters {
+NPCM7XX_ADC_CON,
+NPCM7XX_ADC_DATA,
+NPCM7XX_ADC_REGS_END,
+};
+
+/* Register field definitions. */
+#define NPCM7XX_ADC_CON_MUX(rv) extract32(rv, 24, 4)
+#define NPCM7XX_ADC_CON_INT_EN BIT(21)
+#define NPCM7XX_ADC_CON_REFSEL BIT(19)
+#define NPCM7XX_ADC_CON_INT BIT(18)
+#define NPCM7XX_ADC_CON_EN BIT(17)
+#define NPCM7XX_ADC_CON_RST BIT(16)
+#define NPCM7XX_ADC_CON_CONVBIT(14)
+#define NPCM7XX_ADC_CON_DIV(rv) extract32(rv, 1, 8)
+
+#define NPCM7XX_ADC_MAX_RESULT 1023
+#define NPCM7XX_ADC_DEFAULT_IREF200
+#define NPCM7XX_ADC_CONV_CYCLES 20
+#define NPCM7XX_ADC_RESET_CYCLES10
+#define NPCM7XX_ADC_R0_INPUT50
+#define NPCM7XX_ADC_R1_INPUT150
+
+static void npcm7xx_adc_reset(NPCM7xxADCState *s)
+{
+timer_del(>conv_timer);
+timer_del(>reset_timer);
+s->con = 0x000c0001;
+s->data = 0x;
+}
+
+static uint32_t npcm7xx_adc_convert(uint32_t input, uint32_t ref)
+{
+uint32_t result;
+
+result = input * (NPCM7XX_ADC_MAX_RESULT + 1) / ref;
+if (result > NPCM7XX_ADC_MAX_RESULT) {
+result = NPCM7XX_ADC_MAX_RESULT;
+}
+
+return result;
+}
+
+static uint32_t npcm7xx_adc_prescaler(NPCM7xxADCState *s)
+{
+return 2 * (NPCM7XX_ADC_CON_DIV(s->con) + 1);
+}
+
+static void npcm7xx_adc_start_timer(Clock *clk, QEMUTimer *timer,
+uint32_t cycles, uint32_t prescaler)
+{
+int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+int64_t freq = clock_get_hz(clk);
+int64_t ns;
+
+ns = (NANOSECONDS_PER_SECOND * cycles * prescaler / freq);
+ns += now;
+timer_mod(timer, ns);
+}
+
+static void npcm7xx_adc_start_reset(NPCM7xxADCState *s)
+{
+uint32_t prescaler = npcm7xx_adc_prescaler(s);
+
+npcm7xx_adc_start_timer(s->clock, >reset_timer,
NPCM7XX_ADC_RESET_CYCLES,
+prescaler);
+}
+
+static void npcm7xx_adc_start_convert(NPCM7xxADCState *s)
+{
+uint32_t prescaler = npcm7xx_adc_prescaler(s);
+
+
[PATCH v4 0/6] Additional NPCM7xx devices
This patch series include a few more NPCM7XX devices including - Analog Digital Converter (ADC) - Pulse Width Modulation (PWM) We also modified the CLK module to generate clock values using qdev_clock. These clocks are used to determine various clocks in NPCM7XX devices. Thank you for your review. Changes since v3: - Use type casting instead of accessing parent object in all devices. Changes since v2: - Split PWM test into a separate patch in the patch set - Add trace events for PWM's update_freq/update_duty - Add trace events for ioread/iowrite in ADC and PWM - Use timer_get_ns in hw/timer/npcm7xx_timer.c - Update commit message in ADC/PWM to mention qom-get/set method for usage - Fix typos Changes since v1: - We removed the IPMI and KCS related code from this patch set. Hao Wu (6): hw/misc: Add clock converter in NPCM7XX CLK module hw/timer: Refactor NPCM7XX Timer to use CLK clock hw/adc: Add an ADC module for NPCM7XX hw/misc: Add a PWM module for NPCM7XX hw/misc: Add QTest for NPCM7XX PWM Module hw/*: Use type casting for SysBusDevice in NPCM7XX docs/system/arm/nuvoton.rst | 4 +- hw/adc/meson.build | 1 + hw/adc/npcm7xx_adc.c | 321 + hw/adc/trace-events | 5 + hw/arm/npcm7xx.c | 55 ++- hw/arm/npcm7xx_boards.c | 2 +- hw/mem/npcm7xx_mc.c | 2 +- hw/misc/meson.build | 1 + hw/misc/npcm7xx_clk.c| 797 ++- hw/misc/npcm7xx_gcr.c| 2 +- hw/misc/npcm7xx_pwm.c| 559 ++ hw/misc/npcm7xx_rng.c| 2 +- hw/misc/trace-events | 6 + hw/nvram/npcm7xx_otp.c | 2 +- hw/ssi/npcm7xx_fiu.c | 2 +- hw/timer/npcm7xx_timer.c | 25 +- include/hw/adc/npcm7xx_adc.h | 72 +++ include/hw/arm/npcm7xx.h | 4 + include/hw/misc/npcm7xx_clk.h| 146 +- include/hw/misc/npcm7xx_pwm.h| 106 include/hw/timer/npcm7xx_timer.h | 1 + meson.build | 1 + tests/qtest/meson.build | 4 +- tests/qtest/npcm7xx_adc-test.c | 400 tests/qtest/npcm7xx_pwm-test.c | 490 +++ 25 files changed, 2972 insertions(+), 38 deletions(-) create mode 100644 hw/adc/npcm7xx_adc.c create mode 100644 hw/adc/trace-events create mode 100644 hw/misc/npcm7xx_pwm.c create mode 100644 include/hw/adc/npcm7xx_adc.h create mode 100644 include/hw/misc/npcm7xx_pwm.h create mode 100644 tests/qtest/npcm7xx_adc-test.c create mode 100644 tests/qtest/npcm7xx_pwm-test.c -- 2.29.2.684.gfbc64c5ab5-goog
[Bug 1213196] Re: -serial tcp should hang up when DTR goes low
Sent in a patch for this. https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg04658.html DTR controls the socket. DCD reflects the state of the socket. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1213196 Title: -serial tcp should hang up when DTR goes low Status in QEMU: New Bug description: In keeping with the spirit of serial modem control signals, de- asserting DTR should cause the TCP connection to break; asserting DTR should cause QEMU to initiate a new connection or for it to accept another (in server mode; this may involve waiting for one to arrive, too). To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1213196/+subscriptions
Re: [PATCH 03/11] target/mips/mips-defs: Use ISA_MIPS32R2 definition to check Release 2
On 12/16/20 5:34 PM, Philippe Mathieu-Daudé wrote: > On 12/16/20 4:16 PM, Jiaxun Yang wrote: >> 在 2020/12/16 21:43, Philippe Mathieu-Daudé 写道: >>> Use the single ISA_MIPS32R2 definition to check if the Release 2 >>> ISA is supported, whether the CPU support 32/64-bit. >>> >>> For now we keep '32' in the definition name, we will rename it >>> as ISA_MIPS_R2 in few commits. >>> >>> Signed-off-by: Philippe Mathieu-Daudé >>> --- >>> target/mips/mips-defs.h | 3 +-- >>> linux-user/mips/cpu_loop.c | 1 - >>> target/mips/translate.c | 4 ++-- >>> 3 files changed, 3 insertions(+), 5 deletions(-) >>> >>> diff --git a/target/mips/mips-defs.h b/target/mips/mips-defs.h >>> index 2756e72a9d6..9cfa4c346bf 100644 >>> --- a/target/mips/mips-defs.h >>> +++ b/target/mips/mips-defs.h >>> @@ -24,7 +24,6 @@ >>> #define ISA_MIPS5 0x0010ULL >>> #define ISA_MIPS32 0x0020ULL >>> #define ISA_MIPS32R2 0x0040ULL >>> -#define ISA_MIPS64R2 0x0100ULL >>> #define ISA_MIPS32R3 0x0200ULL >>> #define ISA_MIPS64R3 0x0400ULL >>> #define ISA_MIPS32R5 0x0800ULL >>> @@ -81,7 +80,7 @@ >>> /* MIPS Technologies "Release 2" */ >>> #define CPU_MIPS32R2 (CPU_MIPS32 | ISA_MIPS32R2) >>> -#define CPU_MIPS64R2 (CPU_MIPS64 | CPU_MIPS32R2 | ISA_MIPS64R2) >>> +#define CPU_MIPS64R2 (CPU_MIPS64 | ISA_MIPS32R2) >>> /* MIPS Technologies "Release 3" */ >>> #define CPU_MIPS32R3 (CPU_MIPS32R2 | ISA_MIPS32R3) >>> diff --git a/linux-user/mips/cpu_loop.c b/linux-user/mips/cpu_loop.c >>> index b58dbeb83d1..a2aa8167210 100644 >>> --- a/linux-user/mips/cpu_loop.c >>> +++ b/linux-user/mips/cpu_loop.c >>> @@ -386,7 +386,6 @@ void target_cpu_copy_regs(CPUArchState *env, >>> struct target_pt_regs *regs) >>> prog_req.fre &= interp_req.fre; >>> bool cpu_has_mips_r2_r6 = env->insn_flags & ISA_MIPS32R2 || >>> - env->insn_flags & ISA_MIPS64R2 || >>> env->insn_flags & ISA_MIPS32R6 || >>> env->insn_flags & ISA_MIPS64R6; >>> diff --git a/target/mips/translate.c b/target/mips/translate.c >>> index 8c0ecfa17e1..0923dfdf451 100644 >>> --- a/target/mips/translate.c >>> +++ b/target/mips/translate.c >>> @@ -28212,7 +28212,7 @@ static void decode_opc_special3(CPUMIPSState >>> *env, DisasContext *ctx) >>> case OPC_DINSM: >>> case OPC_DINSU: >>> case OPC_DINS: >>> - check_insn(ctx, ISA_MIPS64R2); >>> + check_insn(ctx, ISA_MIPS32R2); >>> check_mips_64(ctx); >> > > Sorry I respin v2 before noticing this reply. > >> After this change, 32-bit CPUs emulated with TARGET_MIPS64 >> and got CP0.Status.KX and CP0.Status.UX accidentally set won't >> emit RI exception. > > 32-bit CPUs shouldn't have MIPS_HFLAG_64 set, regardless > the build used. So check_mips_64() will emit it... > > Anyhow, I'd rather remove 32-bit CPUs from 64-bit build unless > we are sure this works. $ qemu-system-mips64el -M malta -S -monitor stdio \ -bios /dev/null -smp 2 -cpu 4Kc QEMU 5.2.50 monitor - type 'help' for more information (qemu) info qom-tree /machine (malta-machine) /peripheral (container) /peripheral-anon (container) /unattached (container) /bios.1fc[0] (qemu:memory-region) /device[0] (mips-malta) /cpu-refclk (clock) /cpu[0] (4Kc-mips64-cpu) /clk-in (clock) /cpu[1] (4Kc-mips64-cpu) /clk-in (clock) "4Kc-mips64"???
Re: [PATCH v2 02/12] target/mips/mips-defs: Use ISA_MIPS3 for ISA_MIPS64
On 12/16/20 8:08 PM, Richard Henderson wrote:
> On 12/16/20 10:27 AM, Philippe Mathieu-Daudé wrote:
>> MIPS 64-bit ISA is introduced with MIPS3.
>> No need for another bit/definition to check for 64-bit.
>>
>> Suggested-by: Jiaxun Yang
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> target/mips/mips-defs.h | 2 +-
>> hw/mips/boston.c| 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/mips/mips-defs.h b/target/mips/mips-defs.h
>> index f4d76e562d1..ab621a750d5 100644
>> --- a/target/mips/mips-defs.h
>> +++ b/target/mips/mips-defs.h
>> @@ -19,7 +19,7 @@
>> */
>> #define ISA_MIPS1 0x0001ULL
>> #define ISA_MIPS2 0x0002ULL
>> -#define ISA_MIPS3 0x0004ULL
>> +#define ISA_MIPS3 0x0004ULL /* 64-bit */
>> #define ISA_MIPS4 0x0008ULL
>> #define ISA_MIPS5 0x0010ULL
>> #define ISA_MIPS320x0020ULL
>> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
>> index c3b94c68e1b..f44f681fab5 100644
>> --- a/hw/mips/boston.c
>> +++ b/hw/mips/boston.c
>> @@ -463,7 +463,7 @@ static void boston_mach_init(MachineState *machine)
>> exit(1);
>> }
>>
>> -is_64b = cpu_type_supports_isa(machine->cpu_type, ISA_MIPS64);
>> +is_64b = cpu_type_supports_isa(machine->cpu_type, ISA_MIPS3);
>
> Find this slightly confusing.
>
> After all of the renaming, I would expect ISA_MIPS64R6 -> ISA_MIPS_R6 |
> ISA_MIPS_64, not ISA_MIPS_R6 | ISA_MIPS3.
Well all the ISA_* definitions now match:
https://images.anandtech.com/doci/8457/MIPS%20ISA%20Evolution.JPG
Except ISA_NANOMIPS32, which is listed in
MD01251-2B-nanoMIPS32PRA-06.09.pdf as an extension, similar to microMIPS:
MIPS32, microMIPS32, and nanoMIPS32 Operating Modes
Release 2 of the MIPS32 Architecture added support for 64-bit
coprocessors (and, in particular, 64-bit floating-point units)
with 32-bit CPUs. Thus, certain floating-point instructions
that previously were enabled by 64-bit operations on a MIPS64
processor now are enabled by new 64-bit floating-point operations.
Release 3 introduced the microMIPS instruction set, allowing all
microMIPS processors to implement a 64-bit floating-point unit.
Release 6 introduces the nanoMIPS instruction set. The nanoMIPS
instruction set provides access to the same instruction set extensions
(example, COP1 floating-point instructions) that microMIPS had access
to.
I'd rather keep one definitions per ISA. Eventually if you want
a definition to check if a CPU is 32/64-bit we can add an alias:
-- >8 --
diff --git a/target/mips/mips-defs.h b/target/mips/mips-defs.h
index 376262fa250..2c3f4277cfe 100644
--- a/target/mips/mips-defs.h
+++ b/target/mips/mips-defs.h
@@ -65,6 +65,8 @@
#define CPU_LOONGSON2E (CPU_MIPS3 | INSN_LOONGSON2E)
#define CPU_LOONGSON2F (CPU_MIPS3 | INSN_LOONGSON2F | ASE_LMMI)
+#define CPU_MIPS64 (ISA_MIPS3)
+
/* MIPS Technologies "Release 1" */
#define CPU_MIPS32R1(CPU_MIPS2 | ISA_MIPS_R1)
#define CPU_MIPS64R1(CPU_MIPS5 | CPU_MIPS32R1)
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index f44f681fab5..9f56099e42f 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -463,7 +463,7 @@ static void boston_mach_init(MachineState *machine)
exit(1);
}
-is_64b = cpu_type_supports_isa(machine->cpu_type, ISA_MIPS3);
+is_64b = cpu_type_supports_isa(machine->cpu_type, CPU_MIPS64);
object_initialize_child(OBJECT(machine), "cps", >cps,
TYPE_MIPS_CPS);
object_property_set_str(OBJECT(>cps), "cpu-type", machine->cpu_type,
---
But for the Boston case, it is simpler to add an inline function in
cpu.h:
-- >8 --
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -1302,6 +1302,11 @@ static inline bool ase_mt_available(CPUMIPSState
*env)
return env->CP0_Config3 & (1 << CP0C3_MT);
}
+static inline bool cpu_type_is_64bit(const char *cpu_type)
+{
+return cpu_type_supports_isa(cpu_type, CPU_MIPS64);
+}
+
void cpu_set_exception_base(int vp_index, target_ulong address);
/* addr.c */
---
Note, I'd still use ISA_MIPS3 in this cpu_type_is_64bit().
Or I could add the ISA_MIPS_64 alias and call it a day...
>
>
> r~
>
[Bug 1879531] Re: Stack-overflow in _eth_get_rss_ex_dst_addr
Minimized Reproducer: cat << EOF | ./qemu-system-i386 -M pc-q35-5.0 \ -accel qtest -monitor none \ -serial none -nographic -qtest stdio outl 0xcf8 0x80001010 outl 0xcfc 0xe102 outl 0xcf8 0x80001004 outw 0xcfc 0x7 write 0x25 0x1 0x86 write 0x26 0x1 0xdd write 0x4f 0x1 0x2b write 0xe1020030 0x4 0x190002e1 write 0xe102003a 0x2 0x0807 write 0xe1020048 0x4 0x12077cdd write 0xe1020400 0x4 0xba077cdd write 0xe1020420 0x4 0x190002e1 write 0xe1020428 0x4 0x3509d807 write 0xe1020438 0x1 0xe2 EOF -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1879531 Title: Stack-overflow in _eth_get_rss_ex_dst_addr Status in QEMU: New Bug description: Hello, While fuzzing, I found a 1-byte stack-overflow (read) through the e1000e. ==10318==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffdb76c16c2 at pc 0x55594f1a69e1 bp 0x7ffdb76c15a0 sp 0x7ffdb76c1598 READ of size 1 at 0x7ffdb76c16c2 thread T0 #0 0x55594f1a69e0 in _eth_get_rss_ex_dst_addr /home/alxndr/Development/qemu/net/eth.c:410:17 #1 0x55594f1a39da in eth_parse_ipv6_hdr /home/alxndr/Development/qemu/net/eth.c:532:17 #2 0x55594ebc34f2 in net_tx_pkt_parse_headers /home/alxndr/Development/qemu/hw/net/net_tx_pkt.c:228:14 #3 0x55594ebc2149 in net_tx_pkt_parse /home/alxndr/Development/qemu/hw/net/net_tx_pkt.c:273:9 #4 0x55594ec1ba76 in e1000e_process_tx_desc /home/alxndr/Development/qemu/hw/net/e1000e_core.c:737:29 #5 0x55594ec1aea4 in e1000e_start_xmit /home/alxndr/Development/qemu/hw/net/e1000e_core.c:934:9 #6 0x55594ec0e70e in e1000e_set_tdt /home/alxndr/Development/qemu/hw/net/e1000e_core.c:2451:9 #7 0x55594ebec435 in e1000e_core_write /home/alxndr/Development/qemu/hw/net/e1000e_core.c:3261:9 #8 0x55594ebdf11b in e1000e_mmio_write /home/alxndr/Development/qemu/hw/net/e1000e.c:109:5 #9 0x55594dfd98b1 in memory_region_write_accessor /home/alxndr/Development/qemu/memory.c:483:5 #10 0x55594dfd9211 in access_with_adjusted_size /home/alxndr/Development/qemu/memory.c:544:18 #11 0x55594dfd7c30 in memory_region_dispatch_write /home/alxndr/Development/qemu/memory.c:1476:16 #12 0x55594dde24b8 in flatview_write_continue /home/alxndr/Development/qemu/exec.c:3137:23 #13 0x55594ddd12dc in flatview_write /home/alxndr/Development/qemu/exec.c:3177:14 #14 0x55594ddd0dec in address_space_write /home/alxndr/Development/qemu/exec.c:3268:18 #15 0x55594dfcdbdc in qtest_process_command /home/alxndr/Development/qemu/qtest.c:567:9 #16 0x55594dfc3700 in qtest_process_inbuf /home/alxndr/Development/qemu/qtest.c:710:9 #17 0x55594dfc2cc8 in qtest_read /home/alxndr/Development/qemu/qtest.c:722:5 #18 0x55594f74b259 in qemu_chr_be_write_impl /home/alxndr/Development/qemu/chardev/char.c:183:9 #19 0x55594f74b3ee in qemu_chr_be_write /home/alxndr/Development/qemu/chardev/char.c:195:9 #20 0x55594f7556fc in fd_chr_read /home/alxndr/Development/qemu/chardev/char-fd.c:68:9 #21 0x55594f7ea488 in qio_channel_fd_source_dispatch /home/alxndr/Development/qemu/io/channel-watch.c:84:12 #22 0x7f43f6c1d897 in g_main_context_dispatch (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4e897) #23 0x55594f9dea5d in glib_pollfds_poll /home/alxndr/Development/qemu/util/main-loop.c:219:9 #24 0x55594f9dd1d7 in os_host_main_loop_wait /home/alxndr/Development/qemu/util/main-loop.c:242:5 #25 0x55594f9dcd6e in main_loop_wait /home/alxndr/Development/qemu/util/main-loop.c:518:11 #26 0x55594e44cd01 in qemu_main_loop /home/alxndr/Development/qemu/softmmu/vl.c:1664:9 #27 0x55594f803c21 in main /home/alxndr/Development/qemu/softmmu/main.c:49:5 #28 0x7f43f57b4e0a in __libc_start_main /build/glibc-GwnBeO/glibc-2.30/csu/../csu/libc-start.c:308:16 #29 0x55594dd03889 in _start (/home/alxndr/Development/qemu/build/i386-softmmu/qemu-system-i386+0xdbd889) Address 0x7ffdb76c16c2 is located in stack of thread T0 at offset 34 in frame #0 0x55594f1a303f in eth_parse_ipv6_hdr /home/alxndr/Development/qemu/net/eth.c:486 This frame has 1 object(s): [32, 34) 'ext_hdr' (line 487) <== Memory access at offset 34 overflows this variable HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork (longjmp and C++ exceptions *are* supported) SUMMARY: AddressSanitizer: stack-buffer-overflow /home/alxndr/Development/qemu/net/eth.c:410:17 in _eth_get_rss_ex_dst_addr Shadow bytes around the buggy address: 0x100036ed0280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x100036ed0290: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x100036ed02a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x100036ed02b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x100036ed02c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Re: [PULL 00/12] Compile QEMU with -Wimplicit-fallthrough
Patchew URL: https://patchew.org/QEMU/20201216172949.57380-1-th...@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20201216172949.57380-1-th...@redhat.com Subject: [PULL 00/12] Compile QEMU with -Wimplicit-fallthrough === 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 From https://github.com/patchew-project/qemu * [new tag] patchew/20201216172949.57380-1-th...@redhat.com -> patchew/20201216172949.57380-1-th...@redhat.com Switched to a new branch 'test' 7bedbc8 configure: Compile with -Wimplicit-fallthrough=2 e14bb9d tests/fp: Do not emit implicit-fallthrough warnings in the softfloat tests ebd3c45 tcg/optimize: Add fallthrough annotations cfe5662 target/sparc/win_helper: silence the compiler warnings 8ef9335 target/sparc/translate: silence the compiler warnings 4588bf9 accel/tcg/user-exec: silence the compiler warnings be2108e hw/intc/arm_gicv3_kvm: silence the compiler warnings 7d033d0 target/i386: silence the compiler warnings in gen_shiftd_rm_T1 284b00a hw/timer/renesas_tmr: silence the compiler warnings c3d2957 hw/rtc/twl92230: Silence warnings about missing fallthrough statements 1b1609c target/unicore32/translate: Add missing fallthrough annotations 99bc0f0 disas/libvixl: Fix fall-through annotation for GCC >= 7 === OUTPUT BEGIN === 1/12 Checking commit 99bc0f0e92b7 (disas/libvixl: Fix fall-through annotation for GCC >= 7) ERROR: do not use C99 // comments #49: FILE: disas/libvixl/vixl/globals.h:111: +// Fallthrough annotation for Clang and C++11(201103L). ERROR: do not use C99 // comments #52: FILE: disas/libvixl/vixl/globals.h:114: +// Fallthrough annotation for GCC >= 7. total: 2 errors, 0 warnings, 24 lines checked Patch 1/12 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 2/12 Checking commit 1b1609c7573a (target/unicore32/translate: Add missing fallthrough annotations) 3/12 Checking commit c3d2957383b8 (hw/rtc/twl92230: Silence warnings about missing fallthrough statements) 4/12 Checking commit 284b00aef566 (hw/timer/renesas_tmr: silence the compiler warnings) 5/12 Checking commit 7d033d02b90d (target/i386: silence the compiler warnings in gen_shiftd_rm_T1) 6/12 Checking commit be2108e641c9 (hw/intc/arm_gicv3_kvm: silence the compiler warnings) 7/12 Checking commit 4588bf97482b (accel/tcg/user-exec: silence the compiler warnings) 8/12 Checking commit 8ef9335f2838 (target/sparc/translate: silence the compiler warnings) 9/12 Checking commit cfe56623ece8 (target/sparc/win_helper: silence the compiler warnings) 10/12 Checking commit ebd3c45fd052 (tcg/optimize: Add fallthrough annotations) WARNING: architecture specific defines should be avoided #35: FILE: include/qemu/compiler.h:230: +#if __has_attribute(fallthrough) total: 0 errors, 1 warnings, 43 lines checked Patch 10/12 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 11/12 Checking commit e14bb9ddd6f3 (tests/fp: Do not emit implicit-fallthrough warnings in the softfloat tests) 12/12 Checking commit 7bedbc83bcfa (configure: Compile with -Wimplicit-fallthrough=2) === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20201216172949.57380-1-th...@redhat.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
[PATCH v1 1/1] chardev: enable guest socket status/crontrol via DTR and DCD
This patch adds a 'modemctl' option to "-chardev socket" to enable control
of the socket via the guest serial port.
The default state of the option is disabled.
1. disconnect a connected socket when DTR transitions to low, also reject
new connections while DTR is low.
2. provide socket connection status through the carrier detect line (CD or
DCD) on the guest serial port
Buglink: https://bugs.launchpad.net/qemu/+bug/1213196
Signed-off-by: Darrin M. Gorski
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 213a4c8dd0..94dd28e0cd 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -36,6 +36,7 @@
#include "qapi/qapi-visit-sockets.h"
#include "chardev/char-io.h"
+#include "chardev/char-serial.h"
#include "qom/object.h"
/***/
@@ -85,6 +86,9 @@ struct SocketChardev {
bool connect_err_reported;
QIOTask *connect_task;
+
+bool is_modemctl;
+int modem_state;
};
typedef struct SocketChardev SocketChardev;
@@ -98,12 +102,18 @@ static void tcp_chr_change_state(SocketChardev *s,
TCPChardevState state)
{
switch (state) {
case TCP_CHARDEV_STATE_DISCONNECTED:
+if(s->is_modemctl) {
+s->modem_state &= ~(CHR_TIOCM_CAR);
+}
break;
case TCP_CHARDEV_STATE_CONNECTING:
assert(s->state == TCP_CHARDEV_STATE_DISCONNECTED);
break;
case TCP_CHARDEV_STATE_CONNECTED:
assert(s->state == TCP_CHARDEV_STATE_CONNECTING);
+if(s->is_modemctl) {
+s->modem_state |= CHR_TIOCM_CAR;
+}
break;
}
s->state = state;
@@ -947,6 +957,12 @@ static void tcp_chr_accept(QIONetListener *listener,
tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
tcp_chr_set_client_ioc_name(chr, cioc);
tcp_chr_new_client(chr, cioc);
+
+if(s->is_modemctl) {
+if(!(s->modem_state & CHR_TIOCM_DTR)) {
+tcp_chr_disconnect(chr); /* disconnect if DTR is low */
+}
+}
}
@@ -1322,12 +1338,17 @@ static void qmp_chardev_open_socket(Chardev *chr,
bool is_tn3270 = sock->has_tn3270 ? sock->tn3270 : false;
bool is_waitconnect = sock->has_wait? sock->wait: false;
bool is_websock = sock->has_websocket ? sock->websocket : false;
+bool is_modemctl = sock->has_modemctl ? sock->modemctl : false;
int64_t reconnect = sock->has_reconnect ? sock->reconnect : 0;
SocketAddress *addr;
s->is_listen = is_listen;
s->is_telnet = is_telnet;
s->is_tn3270 = is_tn3270;
+s->is_modemctl = is_modemctl;
+if(is_modemctl) {
+ s->modem_state = CHR_TIOCM_CTS | CHR_TIOCM_DSR;
+}
s->is_websock = is_websock;
s->do_nodelay = do_nodelay;
if (sock->tls_creds) {
@@ -1448,6 +1469,8 @@ static void qemu_chr_parse_socket(QemuOpts *opts,
ChardevBackend *backend,
sock->tls_creds = g_strdup(qemu_opt_get(opts, "tls-creds"));
sock->has_tls_authz = qemu_opt_get(opts, "tls-authz");
sock->tls_authz = g_strdup(qemu_opt_get(opts, "tls-authz"));
+sock->has_modemctl = qemu_opt_get(opts, "modemctl");
+sock->modemctl = qemu_opt_get_bool(opts, "modemctl", false);
addr = g_new0(SocketAddressLegacy, 1);
if (path) {
@@ -1501,6 +1524,51 @@ char_socket_get_connected(Object *obj, Error **errp)
return s->state == TCP_CHARDEV_STATE_CONNECTED;
}
+/* ioctl support: allow guest to control/track socket state
+ * via modem control lines (DCD/DTR)
+ */
+static int
+char_socket_ioctl(Chardev *chr, int cmd, void *arg)
+{
+SocketChardev *s = SOCKET_CHARDEV(chr);
+
+if(!(s->is_modemctl)) {
+return -ENOTSUP;
+}
+
+switch (cmd) {
+case CHR_IOCTL_SERIAL_GET_TIOCM:
+{
+int *targ = (int *)arg;
+*targ = s->modem_state;
+}
+break;
+case CHR_IOCTL_SERIAL_SET_TIOCM:
+{
+int sarg = *(int *)arg;
+if (sarg & CHR_TIOCM_RTS) {
+s->modem_state |= CHR_TIOCM_RTS;
+} else {
+s->modem_state &= ~(CHR_TIOCM_RTS);
+}
+if (sarg & CHR_TIOCM_DTR) {
+s->modem_state |= CHR_TIOCM_DTR;
+} else {
+s->modem_state &= ~(CHR_TIOCM_DTR);
+/* disconnect if DTR goes low */
+if(s->state == TCP_CHARDEV_STATE_CONNECTED) {
+tcp_chr_disconnect(chr);
+}
+}
+}
+break;
+default:
+return -ENOTSUP;
+}
+
+return 0;
+}
+
static void char_socket_class_init(ObjectClass *oc, void *data)
{
ChardevClass *cc = CHARDEV_CLASS(oc);
@@ -1516,6 +1584,7 @@ static void char_socket_class_init(ObjectClass *oc,
void *data)
cc->chr_add_client = tcp_chr_add_client;
cc->chr_add_watch = tcp_chr_add_watch;
Re: [PATCH v4 3/3] target/arm: Use object_property_add_bool for "sve" property
On 12/16/20 11:12 PM, Richard Henderson wrote: > The interface for object_property_add_bool is simpler, > making the code easier to understand. > > Reviewed-by: Andrew Jones > Signed-off-by: Richard Henderson > --- > target/arm/cpu64.c | 24 ++-- > 1 file changed, 10 insertions(+), 14 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
[Bug 1908450] [NEW] ide/core.c ATA Major Version reporting incorrect
Public bug reported: @@ -165,7 +165,7 @@ static void ide_identify(IDEState *s) put_le16(p + 76, (1 << 8)); } put_le16(p + 80, 0xf0); /* ata3 -> ata6 supported */ - put_le16(p + 80, 0xf0); /* ata3 -> ata6 supported */ + put_le16(p + 80, ((1 << 6) | (1 << 5) (1 << 4) (1 << 3)); /* ata3 -> ata6 supported */ put_le16(p + 81, 0x16); /* conforms to ata5 */ /* 14=NOP supported, 5=WCACHE supported, 0=SMART supported */ put_le16(p + 82, (1 << 14) | (1 << 5) | 1); This field Major Version Number field is presently reporting support for ATA-4 through ATA-7. Bitfield[80] is defined in the ATA-6 specification below. 0xF0 = (1<<7) | (1<<6) | (1 << 5) | (1 << 4) // 4-7 - current settings 0x78 = (1<<6) | (1<<5) | (1 << 4) | (1 << 3) // 3-6 - new settings Either the comment is wrong, or the field is wrong. If the field is wrong it can cause errors in drivers that check support vs conformity. This will not break most guests, since the conformity field is set to ATA-5. I'm not sure whether this component supports ATA-7, but since it's commented as if it supports up through 6, correcting the field assignment seems more correct. ATA/ATAPI-6 Specification https://web.archive.org/web/20200124094822/https://www.t13.org/Documents/UploadedDocuments/project/d1410r3b-ATA-ATAPI-6.pdf Page 116 80 - M Major version number h or h = device does not report version F 15 Reserved F 14 Reserved for ATA/ATAPI-14 F 13 Reserved for ATA/ATAPI-13 F 12 Reserved for ATA/ATAPI-12 F 11 Reserved for ATA/ATAPI-11 F 10 Reserved for ATA/ATAPI-10 F 9 Reserved for ATA/ATAPI-9 F 8 Reserved for ATA/ATAPI-8 F 7 Reserved for ATA/ATAPI-7 F 6 1 = supports ATA/ATAPI-6 F 5 1 = supports ATA/ATAPI-5 F 4 1 = supports ATA/ATAPI-4 F 3 1 = supports ATA-3 X 2 Obsolete X 1 Obsolete F 0 Reserved ** Affects: qemu Importance: Undecided Status: New ** Tags: ata atapi ide identify x86 -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1908450 Title: ide/core.c ATA Major Version reporting incorrect Status in QEMU: New Bug description: @@ -165,7 +165,7 @@ static void ide_identify(IDEState *s) put_le16(p + 76, (1 << 8)); } put_le16(p + 80, 0xf0); /* ata3 -> ata6 supported */ - put_le16(p + 80, 0xf0); /* ata3 -> ata6 supported */ + put_le16(p + 80, ((1 << 6) | (1 << 5) (1 << 4) (1 << 3)); /* ata3 -> ata6 supported */ put_le16(p + 81, 0x16); /* conforms to ata5 */ /* 14=NOP supported, 5=WCACHE supported, 0=SMART supported */ put_le16(p + 82, (1 << 14) | (1 << 5) | 1); This field Major Version Number field is presently reporting support for ATA-4 through ATA-7. Bitfield[80] is defined in the ATA-6 specification below. 0xF0 = (1<<7) | (1<<6) | (1 << 5) | (1 << 4) // 4-7 - current settings 0x78 = (1<<6) | (1<<5) | (1 << 4) | (1 << 3) // 3-6 - new settings Either the comment is wrong, or the field is wrong. If the field is wrong it can cause errors in drivers that check support vs conformity. This will not break most guests, since the conformity field is set to ATA-5. I'm not sure whether this component supports ATA-7, but since it's commented as if it supports up through 6, correcting the field assignment seems more correct. ATA/ATAPI-6 Specification https://web.archive.org/web/20200124094822/https://www.t13.org/Documents/UploadedDocuments/project/d1410r3b-ATA-ATAPI-6.pdf Page 116 80 - M Major version number h or h = device does not report version F 15 Reserved F 14 Reserved for ATA/ATAPI-14 F 13 Reserved for ATA/ATAPI-13 F 12 Reserved for ATA/ATAPI-12 F 11 Reserved for ATA/ATAPI-11 F 10 Reserved for ATA/ATAPI-10 F 9 Reserved for ATA/ATAPI-9 F 8 Reserved for ATA/ATAPI-8 F 7 Reserved for ATA/ATAPI-7 F 6 1 = supports ATA/ATAPI-6 F 5 1 = supports ATA/ATAPI-5 F 4 1 = supports ATA/ATAPI-4 F 3 1 = supports ATA-3 X 2 Obsolete X 1 Obsolete F 0 Reserved To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1908450/+subscriptions
Re: [PATCH v3 0/3] target/arm: Implement an IMPDEF pauth algorithm
On 10/26/20 2:35 PM, Peter Maydell wrote:
> Anyway, I tried to rebase it on current master, but it
> fails 'make check':
>
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> QTEST_QEMU_IMG=./qemu-img
> G_TEST_DBUS_DAEMON=/home/petmay01/linaro/qemu-from-laptop/qemu/tests/dbus-vmstate-daemon.sh
> QTEST_QEMU_BINARY=./qemu-system-aarch64 tests/qtest/arm-cpu-features
> --tap -k
> **
> ERROR:../../tests/qtest/arm-cpu-features.c:439:pauth_tests_default:
> assertion failed: (_error)
> ERROR qtest-aarch64: arm-cpu-features - Bail out!
> ERROR:../../tests/qtest/arm-cpu-features.c:439:pauth_tests_default:
> assertion failed: (_error)
>
> That's the "can't enable pauth-impdef without pauth" test, I think.
Odd. I can't reproduce this. Hopefully it's some difference in your rebase
from mine, so I'll just re-post mine.
r~
[PATCH v4 2/3] target/arm: Add cpu properties to control pauth
The crypto overhead of emulating pauth can be significant for
some workloads. Add two boolean properties that allows the
feature to be turned off, on with the architected algorithm,
or on with an implementation defined algorithm.
We need two intermediate booleans to control the state while
parsing properties lest we clobber ID_AA64ISAR1 into an invalid
intermediate state.
Tested-by: Mark Rutland
Reviewed-by: Andrew Jones
Signed-off-by: Richard Henderson
---
v2: Use boolean properties instead of an enum (drjones).
v3: Add tests (drjones).
---
target/arm/cpu.h | 10 +
target/arm/cpu.c | 13 +++
target/arm/cpu64.c | 40 ++
target/arm/monitor.c | 3 ++-
tests/qtest/arm-cpu-features.c | 13 +++
5 files changed, 74 insertions(+), 5 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 70e9618d13..06f5169f45 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -197,9 +197,11 @@ typedef struct {
#ifdef TARGET_AARCH64
# define ARM_MAX_VQ16
void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
+void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp);
#else
# define ARM_MAX_VQ1
static inline void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp) { }
+static inline void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp) { }
#endif
typedef struct ARMVectorReg {
@@ -947,6 +949,14 @@ struct ARMCPU {
uint64_t reset_cbar;
uint32_t reset_auxcr;
bool reset_hivecs;
+
+/*
+ * Intermediate values used during property parsing.
+ * Once finalized, the values should be read from ID_AA64ISAR1.
+ */
+bool prop_pauth;
+bool prop_pauth_impdef;
+
/* DCZ blocksize, in log_2(words), ie low 4 bits of DCZID_EL0 */
uint32_t dcz_blocksize;
uint64_t rvbar;
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index d6188f6566..5c5fb16114 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1321,6 +1321,19 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
error_propagate(errp, local_err);
return;
}
+
+/*
+ * KVM does not support modifications to this feature.
+ * We have not registered the cpu properties when KVM
+ * is in use, so the user will not be able to set them.
+ */
+if (!kvm_enabled()) {
+arm_cpu_pauth_finalize(cpu, _err);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+return;
+}
+}
}
if (kvm_enabled()) {
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 7cf9fc4bc6..d9feaa9cdb 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -28,6 +28,8 @@
#include "sysemu/kvm.h"
#include "kvm_arm.h"
#include "qapi/visitor.h"
+#include "hw/qdev-properties.h"
+
#ifndef CONFIG_USER_ONLY
static uint64_t a57_a53_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
@@ -572,6 +574,36 @@ void aarch64_add_sve_properties(Object *obj)
}
}
+void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp)
+{
+int arch_val = 0, impdef_val = 0;
+uint64_t t;
+
+/* TODO: Handle HaveEnhancedPAC, HaveEnhancedPAC2, HaveFPAC. */
+if (cpu->prop_pauth) {
+if (cpu->prop_pauth_impdef) {
+impdef_val = 1;
+} else {
+arch_val = 1;
+}
+} else if (cpu->prop_pauth_impdef) {
+error_setg(errp, "cannot enable pauth-impdef without pauth");
+error_append_hint(errp, "Add pauth=on to the CPU property list.\n");
+}
+
+t = cpu->isar.id_aa64isar1;
+t = FIELD_DP64(t, ID_AA64ISAR1, APA, arch_val);
+t = FIELD_DP64(t, ID_AA64ISAR1, GPA, arch_val);
+t = FIELD_DP64(t, ID_AA64ISAR1, API, impdef_val);
+t = FIELD_DP64(t, ID_AA64ISAR1, GPI, impdef_val);
+cpu->isar.id_aa64isar1 = t;
+}
+
+static Property arm_cpu_pauth_property =
+DEFINE_PROP_BOOL("pauth", ARMCPU, prop_pauth, true);
+static Property arm_cpu_pauth_impdef_property =
+DEFINE_PROP_BOOL("pauth-impdef", ARMCPU, prop_pauth_impdef, false);
+
/* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
* otherwise, a CPU with as many features enabled as our emulation supports.
* The version of '-cpu max' for qemu-system-arm is defined in cpu.c;
@@ -627,10 +659,6 @@ static void aarch64_max_initfn(Object *obj)
t = FIELD_DP64(t, ID_AA64ISAR1, DPB, 2);
t = FIELD_DP64(t, ID_AA64ISAR1, JSCVT, 1);
t = FIELD_DP64(t, ID_AA64ISAR1, FCMA, 1);
-t = FIELD_DP64(t, ID_AA64ISAR1, APA, 1); /* PAuth, architected only */
-t = FIELD_DP64(t, ID_AA64ISAR1, API, 0);
-t = FIELD_DP64(t, ID_AA64ISAR1, GPA, 1);
-t = FIELD_DP64(t, ID_AA64ISAR1, GPI, 0);
t = FIELD_DP64(t, ID_AA64ISAR1, SB, 1);
t = FIELD_DP64(t, ID_AA64ISAR1, SPECRES, 1);
t = FIELD_DP64(t, ID_AA64ISAR1, FRINTTS, 1);
@@ -720,6 +748,10 @@ static void
[PATCH v4 3/3] target/arm: Use object_property_add_bool for "sve" property
The interface for object_property_add_bool is simpler,
making the code easier to understand.
Reviewed-by: Andrew Jones
Signed-off-by: Richard Henderson
---
target/arm/cpu64.c | 24 ++--
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index d9feaa9cdb..8e1fad00bb 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -488,6 +488,12 @@ static void cpu_max_set_sve_max_vq(Object *obj, Visitor
*v, const char *name,
cpu->sve_max_vq = max_vq;
}
+/*
+ * Note that cpu_arm_get/set_sve_vq cannot use the simpler
+ * object_property_add_bool interface because they make use
+ * of the contents of "name" to determine which bit on which
+ * to operate.
+ */
static void cpu_arm_get_sve_vq(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
@@ -529,26 +535,17 @@ static void cpu_arm_set_sve_vq(Object *obj, Visitor *v,
const char *name,
set_bit(vq - 1, cpu->sve_vq_init);
}
-static void cpu_arm_get_sve(Object *obj, Visitor *v, const char *name,
-void *opaque, Error **errp)
+static bool cpu_arm_get_sve(Object *obj, Error **errp)
{
ARMCPU *cpu = ARM_CPU(obj);
-bool value = cpu_isar_feature(aa64_sve, cpu);
-
-visit_type_bool(v, name, , errp);
+return cpu_isar_feature(aa64_sve, cpu);
}
-static void cpu_arm_set_sve(Object *obj, Visitor *v, const char *name,
-void *opaque, Error **errp)
+static void cpu_arm_set_sve(Object *obj, bool value, Error **errp)
{
ARMCPU *cpu = ARM_CPU(obj);
-bool value;
uint64_t t;
-if (!visit_type_bool(v, name, , errp)) {
-return;
-}
-
if (value && kvm_enabled() && !kvm_arm_sve_supported()) {
error_setg(errp, "'sve' feature not supported by KVM on this host");
return;
@@ -563,8 +560,7 @@ void aarch64_add_sve_properties(Object *obj)
{
uint32_t vq;
-object_property_add(obj, "sve", "bool", cpu_arm_get_sve,
-cpu_arm_set_sve, NULL, NULL);
+object_property_add_bool(obj, "sve", cpu_arm_get_sve, cpu_arm_set_sve);
for (vq = 1; vq <= ARM_MAX_VQ; ++vq) {
char name[8];
--
2.25.1
[PATCH v4 0/3] target/arm: Implement an IMPDEF pauth algorithm
The architected pauth algorithm is quite slow without hardware support, and boot times for kernels that enable use of the feature have been significantly impacted. Version 1 blurb at https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg02172.html which contains larger study of the tradeoffs. Version 2 changes: * Use boolean properties, for qmp_query_cpu_model_expansion (drjones). * Move XXH64 implementation to xxhash.h (ajb). * Include a small cleanup to parsing the "sve" property that I noticed along the way. Version 3 changes: * Swap order of patches (drjones). * Add properties test case (drjones). Version 4 changes: * Rebase. r~ Richard Henderson (3): target/arm: Implement an IMPDEF pauth algorithm target/arm: Add cpu properties to control pauth target/arm: Use object_property_add_bool for "sve" property include/qemu/xxhash.h | 82 ++ target/arm/cpu.h | 25 +-- target/arm/cpu.c | 13 ++ target/arm/cpu64.c | 64 ++ target/arm/monitor.c | 3 +- target/arm/pauth_helper.c | 41 ++--- tests/qtest/arm-cpu-features.c | 13 ++ 7 files changed, 213 insertions(+), 28 deletions(-) -- 2.25.1
[PATCH v4 1/3] target/arm: Implement an IMPDEF pauth algorithm
Without hardware acceleration, a cryptographically strong
algorithm is too expensive for pauth_computepac.
Even with hardware accel, we are not currently expecting
to link the linux-user binaries to any crypto libraries,
and doing so would generally make the --static build fail.
So choose XXH64 as a reasonably quick and decent hash.
Tested-by: Mark Rutland
Signed-off-by: Richard Henderson
---
v2: Move the XXH64 bits to xxhash.h (ajb).
Create isar_feature_aa64_pauth_arch and fixup a comment
in isar_feature_aa64_pauth that no longer applies.
---
include/qemu/xxhash.h | 82 +++
target/arm/cpu.h | 15 +--
target/arm/pauth_helper.c | 41 +---
3 files changed, 129 insertions(+), 9 deletions(-)
diff --git a/include/qemu/xxhash.h b/include/qemu/xxhash.h
index 076f1f6054..cf45859a19 100644
--- a/include/qemu/xxhash.h
+++ b/include/qemu/xxhash.h
@@ -119,4 +119,86 @@ static inline uint32_t qemu_xxhash6(uint64_t ab, uint64_t
cd, uint32_t e,
return qemu_xxhash7(ab, cd, e, f, 0);
}
+/*
+ * Component parts of the XXH64 algorithm from
+ * https://github.com/Cyan4973/xxHash/blob/v0.8.0/xxhash.h
+ *
+ * The complete algorithm looks like
+ *
+ * i = 0;
+ * if (len >= 32) {
+ * v1 = seed + XXH_PRIME64_1 + XXH_PRIME64_2;
+ * v2 = seed + XXH_PRIME64_2;
+ * v3 = seed + 0;
+ * v4 = seed - XXH_PRIME64_1;
+ * do {
+ * v1 = XXH64_round(v1, get64bits(input + i));
+ * v2 = XXH64_round(v2, get64bits(input + i + 8));
+ * v3 = XXH64_round(v3, get64bits(input + i + 16));
+ * v4 = XXH64_round(v4, get64bits(input + i + 24));
+ * } while ((i += 32) <= len);
+ * h64 = XXH64_mergerounds(v1, v2, v3, v4);
+ * } else {
+ * h64 = seed + XXH_PRIME64_5;
+ * }
+ * h64 += len;
+ *
+ * for (; i + 8 <= len; i += 8) {
+ * h64 ^= XXH64_round(0, get64bits(input + i));
+ * h64 = rol64(h64, 27) * XXH_PRIME64_1 + XXH_PRIME64_4;
+ * }
+ * for (; i + 4 <= len; i += 4) {
+ * h64 ^= get32bits(input + i) * PRIME64_1;
+ * h64 = rol64(h64, 23) * XXH_PRIME64_2 + XXH_PRIME64_3;
+ * }
+ * for (; i < len; i += 1) {
+ * h64 ^= get8bits(input + i) * XXH_PRIME64_5;
+ * h64 = rol64(h64, 11) * XXH_PRIME64_1;
+ * }
+ *
+ * return XXH64_avalanche(h64)
+ *
+ * Exposing the pieces instead allows for simplified usage when
+ * the length is a known constant and the inputs are in registers.
+ */
+#define XXH_PRIME64_1 0x9E3779B185EBCA87ULL
+#define XXH_PRIME64_2 0xC2B2AE3D27D4EB4FULL
+#define XXH_PRIME64_3 0x165667B19E3779F9ULL
+#define XXH_PRIME64_4 0x85EBCA77C2B2AE63ULL
+#define XXH_PRIME64_5 0x27D4EB2F165667C5ULL
+
+static inline uint64_t XXH64_round(uint64_t acc, uint64_t input)
+{
+return rol64(acc + input * XXH_PRIME64_2, 31) * XXH_PRIME64_1;
+}
+
+static inline uint64_t XXH64_mergeround(uint64_t acc, uint64_t val)
+{
+return (acc ^ XXH64_round(0, val)) * XXH_PRIME64_1 + XXH_PRIME64_4;
+}
+
+static inline uint64_t XXH64_mergerounds(uint64_t v1, uint64_t v2,
+ uint64_t v3, uint64_t v4)
+{
+uint64_t h64;
+
+h64 = rol64(v1, 1) + rol64(v2, 7) + rol64(v3, 12) + rol64(v4, 18);
+h64 = XXH64_mergeround(h64, v1);
+h64 = XXH64_mergeround(h64, v2);
+h64 = XXH64_mergeround(h64, v3);
+h64 = XXH64_mergeround(h64, v4);
+
+return h64;
+}
+
+static inline uint64_t XXH64_avalanche(uint64_t h64)
+{
+h64 ^= h64 >> 33;
+h64 *= XXH_PRIME64_2;
+h64 ^= h64 >> 29;
+h64 *= XXH_PRIME64_3;
+h64 ^= h64 >> 32;
+return h64;
+}
+
#endif /* QEMU_XXHASH_H */
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 7e6c881a7e..70e9618d13 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3852,10 +3852,8 @@ static inline bool isar_feature_aa64_fcma(const
ARMISARegisters *id)
static inline bool isar_feature_aa64_pauth(const ARMISARegisters *id)
{
/*
- * Note that while QEMU will only implement the architected algorithm
- * QARMA, and thus APA+GPA, the host cpu for kvm may use implementation
- * defined algorithms, and thus API+GPI, and this predicate controls
- * migration of the 128-bit keys.
+ * Return true if any form of pauth is enabled, as this
+ * predicate controls migration of the 128-bit keys.
*/
return (id->id_aa64isar1 &
(FIELD_DP64(0, ID_AA64ISAR1, APA, 0xf) |
@@ -3864,6 +3862,15 @@ static inline bool isar_feature_aa64_pauth(const
ARMISARegisters *id)
FIELD_DP64(0, ID_AA64ISAR1, GPI, 0xf))) != 0;
}
+static inline bool isar_feature_aa64_pauth_arch(const ARMISARegisters *id)
+{
+/*
+ * Return true if pauth is enabled with the architected QARMA algorithm.
+ * QEMU will always set APA+GPA to the same value.
+ */
+return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, APA) != 0;
+}
+
static inline bool isar_feature_aa64_sb(const ARMISARegisters *id)
{
return FIELD_EX64(id->id_aa64isar1,
Re: [PATCH v3 4/4] target/arm: Implement Cortex-M55 model
On 12/10/20 2:14 PM, Peter Maydell wrote: > Now that we have implemented all the features needed by the v8.1M > architecture, we can add the model of the Cortex-M55. This is the > configuration without MVE support; we'll add MVE later. > > Signed-off-by: Peter Maydell > --- > target/arm/cpu_tcg.c | 42 ++ > 1 file changed, 42 insertions(+) Reviewed-by: Richard Henderson r~
Re: [PATCH v3 3/4] target/arm: Implement FPCXT_NS fp system register
On 12/10/20 2:14 PM, Peter Maydell wrote: > Implement the v8.1M FPCXT_NS floating-point system register. This is > a little more complicated than FPCXT_S, because it has specific > handling for "current FP state is inactive", and it only wants to do > PreserveFPState(), not the full set of actions done by > ExecuteFPCheck() which vfp_access_check() implements. > > Signed-off-by: Peter Maydell > --- > Changes since v2: refactored along lines suggested by RTH > --- > target/arm/translate-vfp.c.inc | 102 - > 1 file changed, 99 insertions(+), 3 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH v3 2/4] target/arm: Correct store of FPSCR value via FPCXT_S
On 12/10/20 2:14 PM, Peter Maydell wrote: > In commit 64f863baeedc8659 we implemented the v8.1M FPCXT_S register, > but we got the write behaviour wrong. On read, this register reads > bits [27:0] of FPSCR plus the CONTROL.SFPA bit. On write, it doesn't > just write back those bits -- it writes a value to the whole FPSCR, > whose upper 4 bits are zeroes. > > We also incorrectly implemented the write-to-FPSCR as a simple store > to vfp.xregs; this skips the "update the softfloat flags" part of > the vfp_set_fpscr helper so the value would read back correctly but > not actually take effect. > > Fix both of these things by doing a complete write to the FPSCR > using the helper function. > > Signed-off-by: Peter Maydell > --- > target/arm/translate-vfp.c.inc | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) Reviewed-by: Richard Henderson r~
[Bug 1906193] Re: riscv32 user mode emulation: fork return values broken
Here's qemu's own strace log:
farino ~ # /usr/bin/qemu-riscv32 -strace /chroot/riscv-ilp32/tmp/wait-test-short
10123 brk(NULL) = 0x00073000
10123 brk(0x00073880) = 0x00073880
10123 uname(0x407ffed8) = 0
10123 readlinkat(AT_FDCWD,"/proc/self/exe",0x407feff0,4096) = 39
10123 brk(0x00094880) = 0x00094880
10123 brk(0x00095000) = 0x00095000
10123 mprotect(0x0006e000,8192,PROT_READ) = 0
10123
clone(CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|0x11,child_stack=0x,parent_tidptr=0x,tls=0x,child_tidptr=0x00073068)
= 10125
10123
clone(CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|0x11,child_stack=0x,parent_tidptr=0x,tls=0x,child_tidptr=0x00073068)
= 0
10125 exit_group(42)
10123 waitid(0,-1,0x407fff8c,0x4) = 0
10123 statx(1,"",AT_EMPTY_PATH,STATX_BASIC_STATS,0x407ff8e8) = 0
child wants to return 42 (0x2A), parent received 40 (0x28), difference -2
10123 write(1,0x73ad0,74) = 74
10123 exit_group(0)
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1906193
Title:
riscv32 user mode emulation: fork return values broken
Status in QEMU:
New
Bug description:
When running in a chroot with riscv32 (on x86_64; qemu git master as
of today):
The following short program forks; the child immediately returns with
exit(42). The parent checks for the return value - and obtains 40!
gcc-10.2
===
#include
#include
#include
#include
main(c, v)
int c;
char **v;
{
pid_t pid, p;
int s, i, n;
s = 0;
pid = fork();
if (pid == 0)
exit(42);
/* wait for the process */
p = wait();
if (p != pid)
exit (255);
if (WIFEXITED(s))
{
int r=WEXITSTATUS(s);
if (r!=42) {
printf("child wants to return %i (0x%X), parent received %i (0x%X),
difference %i\n",42,42,r,r,r-42);
}
}
}
===
(riscv-ilp32 chroot) farino /tmp # ./wait-test-short
child wants to return 42 (0x2A), parent received 40 (0x28), difference -2
===
(riscv-ilp32 chroot) farino /tmp # gcc --version
gcc (Gentoo 10.2.0-r1 p2) 10.2.0
Copyright (C) 2020 Free Software Foundation, Inc.
Dies ist freie Software; die Kopierbedingungen stehen in den Quellen. Es
gibt KEINE Garantie; auch nicht für MARKTGÄNGIGKEIT oder FÜR SPEZIELLE ZWECKE.
(riscv-ilp32 chroot) farino /tmp # ld --version
GNU ld (Gentoo 2.34 p6) 2.34.0
Copyright (C) 2020 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or (at your option) a later version.
This program has absolutely no warranty.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1906193/+subscriptions
Bug: qemu-system-ppc -M mac99 boots into compat-monitor, not openbios.
Hi all, It seems a qemu-system-ppc from current master no longer boots into openbios, but into to the compat monitor. Command line to reproduce: /home/hsp/src/qemu-master/build/qemu-system-ppc \ -L pc-bios \ -M mac99,via=pmu -m 1024 -boot c \ -drive file=/home/hsp/Mac-disks/9.2.img,format=raw,media=disk Bisecting leads to this commit: commit b4e1a342112e50e05b609e857f38c1f2b7aafdc4 Author: Paolo Bonzini Date: Tue Oct 27 08:44:23 2020 -0400 vl: remove separate preconfig main_loop Move post-preconfig initialization to the x-exit-preconfig. If preconfig is not requested, just exit preconfig mode immediately with the QMP command. As a result, the preconfig loop will run with accel_setup_post and os_setup_post restrictions (xen_restrict, chroot, etc.) already done. Reviewed-by: Igor Mammedov Signed-off-by: Paolo Bonzini include/sysemu/runstate.h | 1 - monitor/qmp-cmds.c| 9 - softmmu/vl.c | 95 --- 3 files changed, 41 insertions(+), 64 deletions(-) Thanks for looking into this, Best, Howard
Re: [PATCH v3 1/4] hw/intc/armv7m_nvic: Correct handling of CCR.BFHFNMIGN
On 12/10/20 2:14 PM, Peter Maydell wrote: > The CCR is a register most of whose bits are banked between security > states but where BFHFNMIGN is not, and we keep it in the non-secure > entry of the v7m.ccr[] array. The logic which tries to handle this > bit fails to implement the "RAZ/WI from Nonsecure if AIRCR.BFHFNMINS > is zero" requirement; correct the omission. > > Signed-off-by: Peter Maydell > --- > Changes since v2: get the "WI" bit right > --- > hw/intc/armv7m_nvic.c | 15 +++ > 1 file changed, 15 insertions(+) Reviewed-by: Richard Henderson r~
[Bug 1906193] Re: riscv32 user mode emulation: fork return values broken
Here's the (abbreviated) output of strace'ing qemu:
farino ~ # strace -f /usr/bin/qemu-riscv32
/chroot/riscv-ilp32/tmp/wait-test-short
execve("/usr/bin/qemu-riscv32", ["/usr/bin/qemu-riscv32",
"/chroot/riscv-ilp32/tmp/wait-tes"...], 0x7ffd95fb1330 /* 40 vars */) = 0
[...]
[pid 16569] uname({sysname="Linux", nodename="farino", ...}) = 0
[pid 16569] lstat("/chroot", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
[pid 16569] lstat("/chroot/riscv-ilp32", {st_mode=S_IFDIR|S_ISGID|0755,
st_size=4096, ...}) = 0
[pid 16569] lstat("/chroot/riscv-ilp32/tmp", {st_mode=S_IFDIR|S_ISVTX|0777,
st_size=4096, ...}) = 0
[pid 16569] lstat("/chroot/riscv-ilp32/tmp/wait-test-short",
{st_mode=S_IFREG|0755, st_size=445632, ...}) = 0
[pid 16569] mmap(0x413f1000, 135168, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x413f1000
[pid 16569] mprotect(0x413eb000, 8192, PROT_READ) = 0
[pid 16569] rt_sigprocmask(SIG_SETMASK, ~[RTMIN RT_1], NULL, 8) = 0
[pid 16569] clone(child_stack=NULL,
flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x1339710)
= 16571
strace: Process 16571 attached
[pid 16571] set_robust_list(0x1339720, 24
[pid 16569] rt_sigprocmask(SIG_SETMASK, ~[RTMIN RT_1], NULL, 8) = 0
[pid 16571] <... set_robust_list resumed>) = 0
[pid 16569] rt_sigprocmask(SIG_SETMASK, [], NULL, 8) = 0
[pid 16571] rt_sigprocmask(SIG_SETMASK, ~[ILL FPE SEGV RTMIN RT_1], ~[KILL STOP
RTMIN RT_1], 8) = 0
[pid 16571] rt_sigprocmask(SIG_BLOCK, ~[], ~[ILL FPE KILL SEGV STOP RTMIN
RT_1], 8) = 0
[pid 16571] clone(child_stack=0x7fe5b73871f0,
flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
parent_tid=[16572], tls=0x7fe5b7387640, child_tidptr=0x7fe5b7387910) = 16572
[pid 16571] rt_sigprocmask(SIG_SETMASK, ~[ILL FPE KILL SEGV STOP RTMIN RT_1],
NULL, 8) = 0
[pid 16571] rt_sigprocmask(SIG_SETMASK, ~[KILL STOP RTMIN RT_1], NULL, 8) = 0
[pid 16571] gettid()= 16571
[pid 16571] rt_sigprocmask(SIG_SETMASK, ~[RTMIN RT_1], NULL, 8) = 0
[pid 16571] rt_sigprocmask(SIG_SETMASK, [], NULL, 8) = 0
[pid 16569] waitid(P_ALL, -1,
[pid 16571] exit_group(42) = ?
strace: Process 16572 attached
[pid 16572] +++ exited with 42 +++
[pid 16571] +++ exited with 42 +++
[pid 16569] <... waitid resumed>{si_signo=SIGCHLD, si_code=CLD_EXITED,
si_pid=16571, si_uid=0, si_status=42, si_utime=3472328296226648184,
si_stime=3475143045726351408}, WEXITED, NULL) = 0
[pid 16569] --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=16571,
si_uid=0, si_status=42, si_utime=0, si_stime=0} ---
[pid 16569] statx(1, "", AT_STATX_SYNC_AS_STAT|AT_EMPTY_PATH,
STATX_BASIC_STATS, {stx_mask=STATX_BASIC_STATS|STATX_MNT_ID, stx_attributes=0,
stx_mode=S_IFCHR|0600, stx_size=0, ...}) = 0
[pid 16569] write(1, "child wants to return 42 (0x2A),"..., 74child wants to
return 42 (0x2A), parent received 40 (0x28), difference -2
) = 74
[pid 16569] brk(0x13c1000) = 0x13c1000
[pid 16569] brk(0x13c) = 0x13c
[pid 16569] exit_group(0) = ?
[pid 16570] <... futex resumed>)= ?
[pid 16570] +++ exited with 0 +++
+++ exited with 0 +++
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1906193
Title:
riscv32 user mode emulation: fork return values broken
Status in QEMU:
New
Bug description:
When running in a chroot with riscv32 (on x86_64; qemu git master as
of today):
The following short program forks; the child immediately returns with
exit(42). The parent checks for the return value - and obtains 40!
gcc-10.2
===
#include
#include
#include
#include
main(c, v)
int c;
char **v;
{
pid_t pid, p;
int s, i, n;
s = 0;
pid = fork();
if (pid == 0)
exit(42);
/* wait for the process */
p = wait();
if (p != pid)
exit (255);
if (WIFEXITED(s))
{
int r=WEXITSTATUS(s);
if (r!=42) {
printf("child wants to return %i (0x%X), parent received %i (0x%X),
difference %i\n",42,42,r,r,r-42);
}
}
}
===
(riscv-ilp32 chroot) farino /tmp # ./wait-test-short
child wants to return 42 (0x2A), parent received 40 (0x28), difference -2
===
(riscv-ilp32 chroot) farino /tmp # gcc --version
gcc (Gentoo 10.2.0-r1 p2) 10.2.0
Copyright (C) 2020 Free Software Foundation, Inc.
Dies ist freie Software; die Kopierbedingungen stehen in den Quellen. Es
gibt KEINE Garantie; auch nicht für MARKTGÄNGIGKEIT oder FÜR SPEZIELLE ZWECKE.
(riscv-ilp32 chroot) farino /tmp # ld --version
GNU ld (Gentoo 2.34 p6) 2.34.0
Copyright (C) 2020 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU
Re: [PATCH v6 0/4] migration: UFFD write-tracking migration/snapshots
On Tue, Dec 15, 2020 at 10:53:13PM +0300, Andrey Gruzdev wrote: > First are series of runs without scan-rate-limiting.patch. > Windows 10: > > msecs : count distribution > 0 -> 1 : 131913 || > 2 -> 3 : 106 || > 4 -> 7 : 362 || > 8 -> 15 : 619 || > 16 -> 31 : 28 || > 32 -> 63 : 1|| > 64 -> 127: 2|| > > > msecs : count distribution > 0 -> 1 : 199273 || > 2 -> 3 : 190 || > 4 -> 7 : 425 || > 8 -> 15 : 927 || > 16 -> 31 : 69 || > 32 -> 63 : 3|| > 64 -> 127: 16 || >128 -> 255: 2|| > > Ubuntu 20.04: > > msecs : count distribution > 0 -> 1 : 104954 || > 2 -> 3 : 9|| > > msecs : count distribution > 0 -> 1 : 147159 || > 2 -> 3 : 13 || > 4 -> 7 : 0|| > 8 -> 15 : 0|| > 16 -> 31 : 0|| > 32 -> 63 : 0|| > 64 -> 127: 1|| > > > Here are runs with scan-rate-limiting.patch. > Windows 10: > > msecs : count distribution > 0 -> 1 : 234492 || > 2 -> 3 : 66 || > 4 -> 7 : 219 || > 8 -> 15 : 109 || > 16 -> 31 : 0|| > 32 -> 63 : 0|| > 64 -> 127: 1|| > > msecs : count distribution > 0 -> 1 : 183171 || > 2 -> 3 : 109 || > 4 -> 7 : 281 || > 8 -> 15 : 444 || > 16 -> 31 : 3|| > 32 -> 63 : 1|| > > Ubuntu 20.04: > > msecs : count distribution > 0 -> 1 : 92224|| > 2 -> 3 : 9|| > 4 -> 7 : 0|| > 8 -> 15 : 0|| > 16 -> 31 : 1|| > 32 -> 63 : 0|| > 64 -> 127: 1|| > > msecs : count distribution > 0 -> 1 : 97021|| > 2 -> 3 : 7|| > 4 -> 7 : 0|| > 8 -> 15 : 0|| > 16 -> 31 : 0|| > 32 -> 63 : 0|| > 64 -> 127: 0|| >128 -> 255: 1|| > > So, initial variant of rate-limiting makes some positive effect, but not very > noticible.
Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types
On Thu, Nov 19, 2020 at 11:32:21AM +0100, Vitaly Kuznetsov wrote:
> Enabling Hyper-V emulation for a Windows VM is a tiring experience as it
> requires listing all currently supported enlightenments ("hv_*" CPU
> features) explicitly. We do have a 'hv_passthrough' mode enabling
> everything but it can't be used in production as it prevents migration.
>
> Introduce a simple 'hyperv=on' option for all x86 machine types enabling
> all currently supported Hyper-V enlightenments. Later, when new
> enlightenments get implemented, we will be adding them to newer machine
> types only (by disabling them for legacy machine types) thus preserving
> migration.
>
> Signed-off-by: Vitaly Kuznetsov
> Reviewed-by: Eduardo Habkost
[...]
> ---
> docs/hyperv.txt | 8
> hw/i386/x86.c | 30 ++
> include/hw/i386/x86.h | 7 +++
> target/i386/cpu.c | 14 ++
> 4 files changed, 59 insertions(+)
>
> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
> index 5df00da54fc4..1a76a07f8417 100644
> --- a/docs/hyperv.txt
> +++ b/docs/hyperv.txt
> @@ -29,6 +29,14 @@ When any set of the Hyper-V enlightenments is enabled,
> QEMU changes hypervisor
> identification (CPUID 0x4000..0x400A) to Hyper-V. KVM identification
> and features are kept in leaves 0x4100..0x4101.
>
> +Hyper-V enlightenments can be enabled in bulk by specifying 'hyperv=on' to an
> +x86 machine type:
> +
> + qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split,hyperv=on
> ...
> +
> +Note, new enlightenments are only added to the latest (in-develompent)
> machine
> +type, older machine types keep the list of the supported features intact to
> +safeguard migration.
>
> 3. Existing enlightenments
> ===
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 5944fc44edca..57f27d56ecc6 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -1171,6 +1171,20 @@ static void x86_machine_set_acpi(Object *obj, Visitor
> *v, const char *name,
> visit_type_OnOffAuto(v, name, >acpi, errp);
> }
>
> +static bool x86_machine_get_hyperv(Object *obj, Error **errp)
> +{
> +X86MachineState *x86ms = X86_MACHINE(obj);
> +
> +return x86ms->hyperv_enabled;
> +}
> +
> +static void x86_machine_set_hyperv(Object *obj, bool value, Error **errp)
> +{
> +X86MachineState *x86ms = X86_MACHINE(obj);
> +
> +x86ms->hyperv_enabled = value;
> +}
> +
> static void x86_machine_initfn(Object *obj)
> {
> X86MachineState *x86ms = X86_MACHINE(obj);
> @@ -1194,6 +1208,16 @@ static void x86_machine_class_init(ObjectClass *oc,
> void *data)
> x86mc->save_tsc_khz = true;
> nc->nmi_monitor_handler = x86_nmi;
>
> +/* Hyper-V features enabled with 'hyperv=on' */
> +x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
> +BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
> +BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
> +BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
> +BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
> +BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
> +BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
> +BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
> +
> object_class_property_add(oc, X86_MACHINE_SMM, "OnOffAuto",
> x86_machine_get_smm, x86_machine_set_smm,
> NULL, NULL);
> @@ -1205,6 +1229,12 @@ static void x86_machine_class_init(ObjectClass *oc,
> void *data)
> NULL, NULL);
> object_class_property_set_description(oc, X86_MACHINE_ACPI,
> "Enable ACPI");
> +
> +object_class_property_add_bool(oc, X86_MACHINE_HYPERV,
> +x86_machine_get_hyperv, x86_machine_set_hyperv);
> +
> +object_class_property_set_description(oc, X86_MACHINE_HYPERV,
> +"Enable Hyper-V enlightenments");
> }
>
> static const TypeInfo x86_machine_info = {
> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
> index 739fac50871b..598abd1be806 100644
> --- a/include/hw/i386/x86.h
> +++ b/include/hw/i386/x86.h
> @@ -38,6 +38,9 @@ struct X86MachineClass {
> bool save_tsc_khz;
> /* Enables contiguous-apic-ID mode */
> bool compat_apic_id_mode;
> +
> +/* Hyper-V features enabled with 'hyperv=on' */
> +uint64_t default_hyperv_features;
> };
>
> struct X86MachineState {
> @@ -71,10 +74,14 @@ struct X86MachineState {
> * will be translated to MSI messages in the address space.
> */
> AddressSpace *ioapic_as;
> +
> +/* Hyper-V emulation */
> +bool hyperv_enabled;
> };
>
> #define X86_MACHINE_SMM "smm"
> #define X86_MACHINE_ACPI "acpi"
> +#define X86_MACHINE_HYPERV "hyperv"
>
> #define TYPE_X86_MACHINE MACHINE_TYPE_NAME("x86")
> OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE)
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 83aca942d87c..63a931679d73 100644
[PATCH] fuzz: fix the generic-fuzz-floppy config
On the pc-i440fx machine, the floppy drive relies on the i8257 DMA
controller. Add this device to the floppy fuzzer config, and silence the
warning about a missing format specifier for the null-co:// drive.
Signed-off-by: Alexander Bulekov
---
tests/qtest/fuzz/generic_fuzz_configs.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h
b/tests/qtest/fuzz/generic_fuzz_configs.h
index b4c5fefeca..0848c11308 100644
--- a/tests/qtest/fuzz/generic_fuzz_configs.h
+++ b/tests/qtest/fuzz/generic_fuzz_configs.h
@@ -92,9 +92,9 @@ const generic_fuzz_config predefined_configs[] = {
},{
.name = "floppy",
.args = "-machine pc -nodefaults -device floppy,id=floppy0 "
-"-drive id=disk0,file=null-co://,file.read-zeroes=on,if=none "
+"-drive
id=disk0,file=null-co://,file.read-zeroes=on,if=none,format=raw "
"-device floppy,drive=disk0,drive-type=288",
-.objects = "fd* floppy*",
+.objects = "fd* floppy* i8257",
},{
.name = "xhci",
.args = "-machine q35 -nodefaults "
--
2.29.2
Re: [PATCH 1/2] accel: kvm: Fix memory waste under mismatch page size
On Wed, Dec 16, 2020 at 04:21:17PM +0800, Keqian Zhu wrote: > One more thing, we should consider whether @start and @size is psize aligned > (my second > patch). Do you agree to add assert on them directly? Yes I think the 2nd patch is okay, but please address Drew's comments. Returning -EINVAL is the same as abort() currently - it'll just abort() at kvm_log_clear() instead. Thanks, -- Peter Xu
Re: [PATCH v12 00/23] i386 cleanup PART 1
On Sat, 12 Dec 2020 16:55:07 +0100, Claudio Fontana wrote:
> The series has been split into two separate parts,
> and this is PART 1.
>
> v11 -> v12:
>
> * "cpu: Move synchronize_from_tb() to tcg_ops":
> removed review tags, as there is currently a bunch of conflicting
> requirements (Eduardo, Richard, Philippe).
>
> [...]
Queued the following:
[01/23] i386: move kvm accel files into kvm/
[02/23] i386: move whpx accel files into whpx/
[03/23] i386: move hax accel files into hax/
[04/23] i386: hvf: remove stale MAINTAINERS entry for old hvf stubs
[05/23] i386: move TCG accel files into tcg/
[06/23] i386: move cpu dump out of helper.c into cpu-dump.c
[07/23] i386: move TCG cpu class initialization to tcg/
[08/23] i386: tcg: remove inline from cpu_load_eflags
[09/23] tcg: cpu_exec_{enter,exit} helpers
[10/23] tcg: make CPUClass.cpu_exec_* optional
[11/23] tcg: Make CPUClass.debug_excp_handler optional
[12/23] cpu: Remove unnecessary noop methods
Thanks!
--
Eduardo
Re: [PATCH v12 16/23] cpu: Move synchronize_from_tb() to tcg_ops
On Mon, Dec 14, 2020 at 05:24:00PM -0500, Eduardo Habkost wrote: > On Mon, Dec 14, 2020 at 10:56:13PM +0100, Philippe Mathieu-Daudé wrote: > > Hi Claudio, Eduardo. > > > > On 12/14/20 8:10 PM, Eduardo Habkost wrote: > > > On Sat, Dec 12, 2020 at 04:55:23PM +0100, Claudio Fontana wrote: > > >> From: Eduardo Habkost > > >> > > >> since tcg_cpu_ops.h is only included in cpu.h, > > >> and as a standalone header it is not really useful, > > >> as tcg_cpu_ops.h starts requiring cpu.h defines, > > >> enums, etc, as well as (later on in the series), > > >> additional definitions coming from memattr.h. > > >> > > >> Therefore rename it to tcg_cpu_ops.h.inc, to warn > > >> any potential user that this file is not a standalone > > >> header, but rather a partition of cpu.h that is > > >> included conditionally if CONFIG_TCG is true. > > > > > > What's the benefit of moving definitions to a separate file, if > > > the new file is not a standalone header? > > > > Claudio, I haven't been following every respin. If you did that > > change just to please me then the circular dependency remarked by > > Richard, then if it simplify the series I'm OK if you have to > > remove the includes. > > > > Eduardo, if you are happy with patches 1-8 (x86 specific), maybe > > you can queue them already. The rest is more TCG generic and > > will likely go via Richard/Paolo trees IMO. > > Patches 01-06 are queued. Patches 07 and 08 need review. Patches 07-12 are now queued too. -- Eduardo
Re: [PATCH] x86/cpu: Add AVX512_FP16 cpu feature
On Thu, Dec 17, 2020 at 06:40:02AM +0800, Cathy Zhang wrote: > AVX512 Half-precision floating point (FP16) has better performance > compared to FP32 if the presicion or magnitude requirements are met. > It's defined as CPUID.(EAX=7,ECX=0):EDX[bit 23]. > > Refer to > https://software.intel.com/content/www/us/en/develop/download/\ > intel-architecture-instruction-set-extensions-programming-reference.html > > Signed-off-by: Cathy Zhang Queued, thanks! -- Eduardo
Re: [PATCH v1 6/6] gdbstub: ensure we clean-up when terminated
On 12/14/20 9:30 AM, Alex Bennée wrote: > If you kill the inferior from GDB we end up leaving our socket lying > around. Fix this by calling gdb_exit() first. > > Signed-off-by: Alex Bennée > --- > gdbstub.c | 2 ++ > 1 file changed, 2 insertions(+) Reviewed-by: Richard Henderson r~
Re: [PATCH v1 5/6] gdbstub: drop gdbserver_cleanup in favour of gdb_exit
On 12/14/20 9:30 AM, Alex Bennée wrote: > Despite it's name it didn't actually clean-up so let us document > gdb_exit() better and use that. > > Signed-off-by: Alex Bennée > --- > include/exec/gdbstub.h | 14 +++--- > gdbstub.c | 7 --- > softmmu/vl.c | 2 +- > 3 files changed, 12 insertions(+), 11 deletions(-) Reviewed-by: Richard Henderson r~
Re: [PATCH v3 4/5] hw/misc: Add a PWM module for NPCM7XX
Thanks for the review. We can add a patch in this patchset to fix this
issue.
On Wed, Dec 16, 2020 at 11:02 AM Peter Maydell
wrote:
> On Tue, 15 Dec 2020 at 00:13, Hao Wu wrote:
> >
> > The PWM module is part of NPCM7XX module. Each NPCM7XX module has two
> > identical PWM modules. Each module contains 4 PWM entries. Each PWM has
> > two outputs: frequency and duty_cycle. Both are computed using inputs
> > from software side.
> >
> > This module does not model detail pulse signals since it is expensive.
> > It also does not model interrupts and watchdogs that are dependant on
> > the detail models. The interfaces for these are left in the module so
> > that anyone in need for these functionalities can implement on their
> > own.
> >
> > The user can read the duty cycle and frequency using qom-get command.
> >
> > Reviewed-by: Havard Skinnemoen
> > Reviewed-by: Tyrone Ting
> > Signed-off-by: Hao Wu
>
>
> > +static void npcm7xx_pwm_init(Object *obj)
> > +{
> > +NPCM7xxPWMState *s = NPCM7XX_PWM(obj);
> > +SysBusDevice *sbd = >parent;
>
> This isn't right. A device shouldn't be poking around
> in the 'parent' or 'parentobj' member of its struct --
> that is a QOM internal. If you want "this device, cast
> to a SysBusDevice", the way to write that is:
>SysBusDevice *sbd = SYS_BUS_DEVICE(s);
>
> (or you could pass 'obj'; same thing).
>
> Looking at the code currently in the tree it also is making this
> same mistake:
>
> $ git grep -- '->parent' hw/*/npcm*
> hw/arm/npcm7xx_boards.c:MachineClass *mc = >parent;
> hw/mem/npcm7xx_mc.c:sysbus_init_mmio(>parent, >mmio);
> hw/misc/npcm7xx_clk.c:sysbus_init_mmio(>parent, >iomem);
> hw/misc/npcm7xx_gcr.c:sysbus_init_mmio(>parent, >iomem);
> hw/misc/npcm7xx_rng.c:sysbus_init_mmio(>parent, >iomem);
> hw/nvram/npcm7xx_otp.c:SysBusDevice *sbd = >parent;
> hw/ssi/npcm7xx_fiu.c:SysBusDevice *sbd = >parent;
> hw/timer/npcm7xx_timer.c:SysBusDevice *sbd = >parent;
>
> These all should be using QOM cast macros. Would somebody
> who's working on these devices like to send a patch ?
>
> thanks
> -- PMM
>
Re: [PATCH v1 4/6] gdbstub: drop CPUEnv from gdb_exit()
On 12/14/20 9:30 AM, Alex Bennée wrote: > gdb_exit() has never needed anything from env and I doubt we are going > to start now. > > Signed-off-by: Alex Bennée > --- > include/exec/gdbstub.h| 2 +- > bsd-user/syscall.c| 6 +++--- > gdbstub.c | 2 +- > linux-user/exit.c | 2 +- > target/arm/arm-semi.c | 2 +- > target/m68k/m68k-semi.c | 2 +- > target/nios2/nios2-semi.c | 2 +- > 7 files changed, 9 insertions(+), 9 deletions(-) Reviewed-by: Richard Henderson r~
